Migration guards miss the commonest case: columns added to SCHEMA with NO migration at all - #2734
Migration guards miss the commonest case: columns added to SCHEMA with NO migration at all#2734jaylfc wants to merge 1 commit into
Conversation
…rzgip)
The two existing static migration guards both pass on a change that
provably breaks every existing install: a new column added straight to
CREATE TABLE IF NOT EXISTS inside SCHEMA with no ALTER anywhere.
CREATE TABLE IF NOT EXISTS is a no-op on existing tables, so the column
is never created on upgrade and the first INSERT crashes with
"table <t> has no column named <c>". Neither existing guard has a
migration entry to inspect, so both clean by vacuity; CI cannot catch
it because tests build fresh DBs (which always have the column).
Add a third static guard, scripts/check_schema_column_migrations.py,
invoked as a new step in doc-gate.yml alongside the existing two.
Mirrors check_retrofit_migrations.py: same stdlib-only approach, same
clean/exit 0 vs violation/exit 1 contract. For every store file under
tinyagentos/ it flags a column present in a CREATE TABLE inside SCHEMA
which has NO matching ALTER TABLE <t> ADD [COLUMN] <c> in the same
file AND which is newly added relative to git show origin/dev:<path>
(the third condition keeps it from flagging every column in the repo).
Proof the new guard goes RED on the exact case from the task:
$ git fetch origin pull/2416/head:pr2416
$ git checkout pr2416 -- tinyagentos/auth_requests_store.py
$ python3 scripts/check_schema_column_migrations.py
SCHEMA-COLUMN VIOLATION: tinyagentos/auth_requests_store.py: table 'auth_requests', column 'kind' added to SCHEMA with no migration
detail: new column 'kind' in CREATE TABLE auth_requests with no ALTER TABLE auth_requests ADD COLUMN kind in this file
fix: add a guarded _post_init coroutine that ALTERs this column into place after a PRAGMA table_info check
SCHEMA-COLUMN VIOLATION: tinyagentos/auth_requests_store.py: table 'auth_requests', column 'purpose' added to SCHEMA with no migration
detail: new column 'purpose' in CREATE TABLE auth_requests with no ALTER TABLE auth_requests ADD COLUMN purpose in this file
fix: add a guarded _post_init coroutine that ALTERs this column into place after a PRAGMA table_info check
SCHEMA-COLUMN VIOLATION: tinyagentos/auth_requests_store.py: table 'auth_requests', column 'requested_by_agent' added to SCHEMA with no migration
detail: new column 'requested_by_agent' in CREATE TABLE auth_requests with no ALTER TABLE auth_requests ADD COLUMN requested_by_agent in this file
fix: add a guarded _post_init coroutine that ALTERs this column into place after a PRAGMA table_info check
SCHEMA-COLUMN VIOLATION: tinyagentos/auth_requests_store.py: table 'auth_requests', column 'requested_name' added to SCHEMA with no migration
detail: new column 'requested_name' in CREATE TABLE auth_requests with no ALTER TABLE auth_requests ADD COLUMN requested_name in this file
fix: add a guarded _post_init coroutine that ALTERs this column into place after a PRAGMA table_info check
SCHEMA-COLUMN VIOLATION: tinyagentos/auth_requests_store.py: table 'auth_requests', column 'requested_slug' added to SCHEMA with no migration
detail: new column 'requested_slug' in CREATE TABLE auth_requests with no ALTER TABLE auth_requests ADD COLUMN requested_slug in this file
fix: add a guarded _post_init coroutine that ALTERs this column into place after a PRAGMA table_info check
(exit 1)
$ git checkout HEAD -- tinyagentos/auth_requests_store.py
$ python3 scripts/check_schema_column_migrations.py
schema-column-guard: clean
(exit 0)
The migration runner's bookkeeping table (schema_migrations) is
excluded because it is created on first boot before any store init and
its columns are not subject to the upgrade-bricks-install pattern.
Adds pitfall 19 to docs/contributor-pitfalls.md describing the failure
mode and pointing at the new guard.
Tests: 12 new tests in tests/scripts/test_check_schema_column_migrations.py,
both passing and failing fixtures, plus split/inline-constraint/alter-
silences-violation coverage. Full tests/scripts/ suite: 159 passed.
Docs-Reviewed: the guard is wired into doc-gate.yml as the new
schema-column-guard step and described in the contributor-pitfalls doc
(pitfall 19), which already enumerates the two sibling guards
(pitfalls 11 and 18); the new entry closes the previously-undocumented
third variant.
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
|
Warning Review limit reachedNext included review available in 17 minutes. View limit detailsLimit details: You’ve used all 4 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (5)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| Prints ``schema-column-guard: clean`` and exits 0 when no violations, or | ||
| prints each violation and exits 1. | ||
|
|
||
| Dependency-light: stdlib only (ast + re + pathlib + subprocess). |
There was a problem hiding this comment.
WARNING: Stale dependency list in docstring.
The docstring claims the script uses ast + re + pathlib + subprocess, but ast is never imported or used. This is misleading for readers comparing it with the sibling check_retrofit_migrations.py (which actually uses ast). Either drop ast from the dependency-light line or explain why it is mentioned.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| return out | ||
|
|
||
|
|
||
| def _extract_schemas(source: str) -> list[str]: |
There was a problem hiding this comment.
WARNING: _extract_schemas matches any triple-quoted string containing CREATE TABLE, not specifically SCHEMA = """...""" constants.
A module/class docstring, a long comment, or an unrelated raw SQL string that happens to mention CREATE TABLE (e.g. example snippets in docstrings) will be picked up and its columns compared against origin/dev. This will produce false positives and silently fail CI on innocent docs. The sibling check_retrofit_migrations.py avoids this by using ast to resolve only SCHEMA/MIGRATIONS constant assignments — that proven pattern would be safer here.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
|
||
| origin_cols = _origin_dev_columns(path) | ||
|
|
||
| added_columns: set[tuple[str, str]] = set() |
There was a problem hiding this comment.
CRITICAL: _ADD_COLUMN_RE is matched against the entire file source, including docstrings and comments, which causes false negatives.
Any ALTER TABLE <t> ADD [COLUMN] <c> mentioned in a Python docstring, # comment, log message, test fixture, or unrelated string literal anywhere in the same file will mark (t, c) as "having a migration" and silence the real violation. A contributor who writes a comment like # See ALTER TABLE auth_requests ADD COLUMN kind in PR #2416 would bypass the guard. Constrain the search to SQL inside the actual _post_init body (e.g. parse the _post_init AST node, or strip comments/strings before regexing), or at minimum ignore text inside triple-quoted and #-prefixed lines.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| return columns | ||
|
|
||
|
|
||
| def _origin_dev_columns(path: Path) -> dict[str, set[str]] | None: |
There was a problem hiding this comment.
WARNING: Silent fallback to "treat file as new" when git show origin/dev:<path> fails masks CI misconfiguration.
_origin_dev_columns returns None on any failure (missing ref, transient network error, ref name change), and the caller then treats every column as new — i.e. every existing column on every store becomes a "SCHEMA-COLUMN VIOLATION". CI that loses its origin/dev ref (renamed branch, shallow clone, fork PR with restricted refs) would suddenly fail with thousands of violations rather than a single actionable error. Emit a loud warning and exit non-zero when the ref is missing, and only fall back to "new file" when the file genuinely does not exist on origin/dev.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| ) | ||
|
|
||
| # ALTER TABLE <table> ADD [COLUMN] <col> ... | ||
| _ADD_COLUMN_RE = re.compile( |
There was a problem hiding this comment.
WARNING: _ADD_COLUMN_RE does not anchor on SQL boundaries, so it will also match strings in unrelated contexts.
The same concern as the critical finding above, but worth a separate call-out: the regex matches ALTER\s+TABLE\s+(\w+)\s+ADD\s+(?:COLUMN\s+)?(\w+) literally anywhere in the file. There is no attempt to restrict to multi-line SQL string literals or to the body of a _post_init function. Even Python identifiers like ALTER_TABLE_ADD_COLUMN_HELPER = ... (which would not match because of underscores, but AlterTable foo add bar in a name would also be filtered out only by the lack of whitespace) are not a risk — but the broader false-negative window on docstrings/comments is real and should be closed.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| sys.exit(main()) No newline at end of file |
There was a problem hiding this comment.
SUGGESTION: Missing trailing newline at end of file.
The diff shows \ No newline at end of file. POSIX text files should end with a newline; many tools (cat, wc -l, some diff/merge drivers, several editors) misbehave on files without one. Add a final \n. Same applies to tests/scripts/test_check_schema_column_migrations.py (line 225) and changelog.d/tsk-hrzgip-schema-column-guard.md.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| ) | ||
|
|
||
|
|
||
| def _split_columns(body: str) -> set[str]: |
There was a problem hiding this comment.
SUGGESTION: _split_columns uses a hand-rolled paren-depth scanner; consider documenting the supported dialect edge cases or accepting the limitation.
The scanner splits on top-level commas and skips segments whose first token looks like a table-level constraint. Edge cases not handled: a column whose DEFAULT expression itself contains a top-level comma at paren-depth 0 (unusual but legal in some extensions), and a column whose name collides with one of the NON_COLUMN_KEYWORDS (e.g. a quoted column literally named select). Neither is fatal but they would silently drop or misclassify a column. A one-line comment in the function noting the limitation, or an explicit raise NotImplementedError for the rare case, would help future readers.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 6 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
SUGGESTION
Files Reviewed (5 files)
Fix these issues in Kilo Cloud Reviewed by minimax-m3:free · Input: 42.3K · Output: 7.2K · Cached: 287.5K |
1 participant
CARD TITLE (intent, not commit subject): Migration guards miss the commonest case: columns added to SCHEMA with NO migration at all
Autonomous build of board card tsk-hrzgip.
The two existing static migration guards both pass on a change that
provably breaks every existing install: a new column added straight to
CREATE TABLE IF NOT EXISTS inside SCHEMA with no ALTER anywhere.
CREATE TABLE IF NOT EXISTS is a no-op on existing tables, so the column
is never created on upgrade and the first INSERT crashes with
"table has no column named ". Neither existing guard has a
migration entry to inspect, so both clean by vacuity; CI cannot catch
it because tests build fresh DBs (which always have the column).
Add a third static guard, scripts/check_schema_column_migrations.py,
invoked as a new step in doc-gate.yml alongside the existing two.
Mirrors check_retrofit_migrations.py: same stdlib-only approach, same
clean/exit 0 vs violation/exit 1 contract. For every store file under
tinyagentos/ it flags a column present in a CREATE TABLE inside SCHEMA
which has NO matching ALTER TABLE ADD [COLUMN] in the same
file AND which is newly added relative to git show origin/dev:
(the third condition keeps it from flagging every column in the repo).
Proof the new guard goes RED on the exact case from the task:
$ git fetch origin pull/2416/head:pr2416
$ git checkout pr2416 -- tinyagentos/auth_requests_store.py
$ python3 scripts/check_schema_column_migrations.py
SCHEMA-COLUMN VIOLATION: tinyagentos/auth_requests_store.py: table 'auth_requests', column 'kind' added to SCHEMA with no migration
detail: new column 'kind' in CREATE TABLE auth_requests with no ALTER TABLE auth_requests ADD COLUMN kind in this file
fix: add a guarded _post_init coroutine that ALTERs this column into place after a PRAGMA table_info check
SCHEMA-COLUMN VIOLATION: tinyagentos/auth_requests_store.py: table 'auth_requests', column 'purpose' added to SCHEMA with no migration
detail: new column 'purpose' in CREATE TABLE auth_requests with no ALTER TABLE auth_requests ADD COLUMN purpose in this file
fix: add a guarded _post_init coroutine that ALTERs this column into place after a PRAGMA table_info check
SCHEMA-COLUMN VIOLATION: tinyagentos/auth_requests_store.py: table 'auth_requests', column 'requested_by_agent' added to SCHEMA with no migration
detail: new column 'requested_by_agent' in CREATE TABLE auth_requests with no ALTER TABLE auth_requests ADD COLUMN requested_by_agent in this file
fix: add a guarded _post_init coroutine that ALTERs this column into place after a PRAGMA table_info check
SCHEMA-COLUMN VIOLATION: tinyagentos/auth_requests_store.py: table 'auth_requests', column 'requested_name' added to SCHEMA with no migration
detail: new column 'requested_name' in CREATE TABLE auth_requests with no ALTER TABLE auth_requests ADD COLUMN requested_name in this file
fix: add a guarded _post_init coroutine that ALTERs this column into place after a PRAGMA table_info check
SCHEMA-COLUMN VIOLATION: tinyagentos/auth_requests_store.py: table 'auth_requests', column 'requested_slug' added to SCHEMA with no migration
detail: new column 'requested_slug' in CREATE TABLE auth_requests with no ALTER TABLE auth_requests ADD COLUMN requested_slug in this file
fix: add a guarded _post_init coroutine that ALTERs this column into place after a PRAGMA table_info check
(exit 1)
$ git checkout HEAD -- tinyagentos/auth_requests_store.py
$ python3 scripts/check_schema_column_migrations.py
schema-column-guard: clean
(exit 0)
The migration runner's bookkeeping table (schema_migrations) is
excluded because it is created on first boot before any store init and
its columns are not subject to the upgrade-bricks-install pattern.
Adds pitfall 19 to docs/contributor-pitfalls.md describing the failure
mode and pointing at the new guard.
Tests: 12 new tests in tests/scripts/test_check_schema_column_migrations.py,
both passing and failing fixtures, plus split/inline-constraint/alter-
silences-violation coverage. Full tests/scripts/ suite: 159 passed.
Docs-Reviewed: the guard is wired into doc-gate.yml as the new
schema-column-guard step and described in the contributor-pitfalls doc
(pitfall 19), which already enumerates the two sibling guards
(pitfalls 11 and 18); the new entry closes the previously-undocumented
third variant.
Files:
.github/workflows/doc-gate.yml | 3 +
changelog.d/tsk-hrzgip-schema-column-guard.md | 2 +
docs/contributor-pitfalls.md | 10 +
scripts/check_schema_column_migrations.py | 249 +++++++++++++++++++++
.../scripts/test_check_schema_column_migrations.py | 225 +++++++++++++++++++
5 files changed, 489 insertions(+)