Skip to content

fix: reject invalid SQL row limits - #19

Merged
haolpku merged 1 commit into
OpenDCAI:mainfrom
DevChiniwala:codex/reject-invalid-db-row-limit
Sep 20, 2026
Merged

haolpku merged 1 commit into
OpenDCAI:mainfrom
DevChiniwala:codex/reject-invalid-db-row-limit

Conversation

@DevChiniwala

Copy link
Copy Markdown
Contributor

DBConfig accepted row_limit=0 and negative values, while BaseSQLDialect.execute_readonly accepted them too. Invalid limits undermine the bounded-read contract and produce inconsistent slicing behavior. This validates row_limit >= 1 in configuration and rechecks the low-level execution boundary for direct callers. Added config and SQLite dialect regressions for zero and negative values. Validation: 27 config plus SQL safeguard tests passed; no schema or API response changes; targets main.

Copilot AI lite review requested due to automatic review settings September 17, 2026 10:41

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@haolpku

haolpku commented Sep 20, 2026

Copy link
Copy Markdown
Collaborator

The implementation itself passed the combined local suite, but this PR now conflicts with merged PR #18 in datamind/tests/test_config.py. The resolution is mechanical: import DBConfig, DataConfig, and Settings, and retain both the profile-path tests and test_db_config_rejects_nonpositive_row_limit. Please merge/rebase current main; I will merge this PR once the branch is updated. Combined validation before the base advanced: 248 passed, 5 optional claude_agent_sdk tests skipped, and the SQLite demo passed.

@haolpku
haolpku merged commit 2879524 into OpenDCAI:main Sep 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants