ci(migrations): add migration safety linter

[declan-scale]

Summary

Adds a CI linter that fails PRs introducing the four migration anti-patterns the runtime timeouts cannot reliably catch on a quiet hour: blocking index builds, validating FK adds, in-band UPDATE/DELETE backfills, and SET/RESET overrides of the runner's default timeouts.

Closes the third sub-issue under the migration-safety umbrella, the linter-on-PR layer that complements the runtime defaults (already shipped) and the CLAUDE.md docs (companion PR).

What's caught

Anti-pattern	Rule	Why
op.create_index without postgresql_concurrently=True (or raw CREATE INDEX outside autocommit_block)	no-concurrently	Plain CREATE INDEX takes ShareLock for the entire build, blocking writes.
op.create_index(postgresql_concurrently=True) outside autocommit_block	concurrently-outside-autocommit	CONCURRENTLY cannot run inside a transaction; would fail at runtime.
op.create_foreign_key without postgresql_not_valid=True (or raw ADD CONSTRAINT FOREIGN KEY without NOT VALID)	fk-without-not-valid	Validating FK creation full-scans the table under ShareRowExclusiveLock.
op.execute("UPDATE ...") / DELETE data backfills	in-band-backfill	Hold row locks for the migration transaction; prevent autovacuum; should be operator-driven runbooks.
SET / RESET of lock_timeout / statement_timeout / idle_in_transaction_session_timeout	forbidden-set	The runner sets these defaults intentionally; quietly overriding them defeats the purpose.

Why custom AST instead of squawk

squawk operates on rendered SQL, which loses the surrounding Alembic context — autocommit_block wrapping and kwargs like postgresql_concurrently / postgresql_not_valid are exactly the signals needed to distinguish safe and unsafe shapes here. Custom AST inspection of the migration source is tighter, has zero external dependencies, runs in milliseconds, and produces messages that point at the correct fix.

The acceptance criteria in the linter ticket explicitly says "squawk or equivalent" — this delivers the equivalent rule set against the right surface.

Escape hatch

Two signals required, both deliberate:

1. # migration-unsafe-ack: <one-line reason> directive at the top of the migration file. The directive must include a non-empty reason on the same line — bare # migration-unsafe-ack: does not suppress.
2. migration-unsafe-ack label on the PR. When present, the workflow runs the linter in --warn-only mode so violations surface in CI logs without blocking merge.

Use the escape hatch for "this needs a maintenance window with traffic shifted away," not "I want to ship faster."

Scope

• agentex/scripts/lint_migrations.py — the linter (Python 3.12 stdlib only; no external deps).
• .github/workflows/migration-lint.yml — workflow triggered on PRs touching agentex/database/migrations/alembic/versions/**.
• agentex/tests/unit/scripts/test_lint_migrations.py — 12 tests covering each rule, the autocommit_block detection, and the ack-directive escape hatch (including that bare directives don't suppress).

The CI step lints only files changed in the PR vs origin/<base>. Existing migrations are not retro-flagged. New migrations going forward must follow the rules.

Test plan

• All 12 linter unit tests pass locally
• Ran the linter against a synthetic bad migration with all four anti-patterns and confirmed each rule fires at the right line number
• Confirmed the rewritten migrations from the companion code PR pass the linter
• Confirmed the legacy migration corpus is not flagged by --base-ref mode (only NEW changes vs base ref are checked)
• First post-merge PR touching a migration exercises the workflow end-to-end (label-based escape hatch tested in a follow-up if needed)

Greptile Summary

• Adds a CI migration safety linter (migration_lint.py) and matching GitHub Actions workflow that blocks PRs introducing blocking index builds, validating FK adds, in-band backfills, or timeout overrides; previous-review gaps (AST-based autocommit_block span detection, per-occurrence concurrently checks, python-comment guards, raw-SQL op.execute paths, fresh-table exclusions) are all addressed in this revision with corresponding tests.
• The escape hatch has been simplified to a single MIGRATION_UNSAFE_ACK env var (set by the workflow when the migration-unsafe-ack PR label is present) plus per-line # noqa: migration-lint; the previous file-level directive that could bypass CI without the label is gone.
• uv.lock adds pyyaml to requires-dist (implying an undiffed pyproject.toml change) and downgrades aiohttp from 3.13.5 to 3.13.4; both appear unrelated to the linter and should be verified before merge.

Confidence Score: 5/5

Safe to merge once the uv.lock noise is confirmed or cleaned up — no logic defects remain in the linter itself.

All previously identified P1 issues are addressed in this revision. The only remaining finding is a P2 about unexplained lock file changes that do not affect linter correctness. All six rules have comprehensive tests, the AST-based autocommit_block detection is sound, and the CI workflow is correctly scoped with minimal permissions.

uv.lock — contains an unexplained pyyaml dependency addition and aiohttp downgrade that should be confirmed or reverted.

Important Files Changed

Filename	Overview
agentex/scripts/ci_tools/migration_lint.py	Core linter — 791-line new file implementing all six rules with AST-based autocommit_block span detection, fresh-table exclusions, and per-line noqa suppression; previous-review issues are all addressed in this revision.
.github/workflows/migration-lint.yml	New CI workflow triggered on PR paths, minimal read-only permissions, label-based escape hatch via MIGRATION_UNSAFE_ACK env var, full fetch-depth for reliable merge-base diff.
agentex/tests/unit/scripts/test_migration_lint.py	Comprehensive 812-line test suite covering all rules, fresh-table exclusions, AST-based autocommit_block detection (including the PEP 617 parenthesized form and column-0 multiline SQL), commented-out call guards, and CLI escape hatch.
uv.lock	Adds pyyaml as a required project dependency in requires-dist (implying a pyproject.toml change not visible in the diff) and downgrades aiohttp from 3.13.5 to 3.13.4 — both changes appear unrelated to the migration linter.

Comments Outside Diff (4)

1. agentex/scripts/lint_migrations.py, line 231-243 (link)

   op.execute(text(...)) silently bypasses all SQL checks

   _string_arg only handles bare string literals (ast.Constant) and f-strings (ast.JoinedStr). When the first argument is a text(...) call — op.execute(text("UPDATE spans SET ...")) — the function returns None and every downstream check (in-band-backfill, forbidden-set, raw CREATE INDEX, raw ADD CONSTRAINT FOREIGN KEY) is silently skipped. This is the SQLAlchemy 2.0-recommended calling convention for raw SQL in Alembic, so it's the pattern most likely to appear in new migrations going forward, and is not covered by any of the 12 tests.

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: agentex/scripts/lint_migrations.py
   Line: 231-243
   
   Comment:
   **`op.execute(text(...))` silently bypasses all SQL checks**
   
   `_string_arg` only handles bare string literals (`ast.Constant`) and f-strings (`ast.JoinedStr`). When the first argument is a `text(...)` call — `op.execute(text("UPDATE spans SET ..."))` — the function returns `None` and every downstream check (`in-band-backfill`, `forbidden-set`, raw `CREATE INDEX`, raw `ADD CONSTRAINT FOREIGN KEY`) is silently skipped. This is the SQLAlchemy 2.0-recommended calling convention for raw SQL in Alembic, so it's the pattern most likely to appear in new migrations going forward, and is not covered by any of the 12 tests.
   
   How can I resolve this? If you propose a fix, please make it concise.

   

2. agentex/scripts/lint_migrations.py, line 200-209 (link)

   File directive alone suppresses violations; PR label is not required

   The docstring (line 22–24) and the PR description both state "Two signals required, both deliberate: (1) file directive AND (2) PR label." In practice, when has_ack is True, _maybe_violation returns None for every check, check_file returns an empty list, and the process exits 0 — entirely independently of whether --warn-only was passed. A developer can merge a file containing # migration-unsafe-ack: reason without the PR ever receiving the label, bypassing the intended two-person visibility mechanism.

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: agentex/scripts/lint_migrations.py
   Line: 200-209
   
   Comment:
   **File directive alone suppresses violations; PR label is not required**
   
   The docstring (line 22–24) and the PR description both state "Two signals required, both deliberate: (1) file directive AND (2) PR label." In practice, when `has_ack` is `True`, `_maybe_violation` returns `None` for every check, `check_file` returns an empty list, and the process exits 0 — entirely independently of whether `--warn-only` was passed. A developer can merge a file containing `# migration-unsafe-ack: reason` without the PR ever receiving the label, bypassing the intended two-person visibility mechanism.
   
   How can I resolve this? If you propose a fix, please make it concise.

   

3. agentex/tests/unit/scripts/test_lint_migrations.py, line 25-34 (link)

   os.chdir in _check_relative is not thread-safe under parallel pytest

   os.chdir changes the process-wide working directory. If tests run with pytest-xdist or any -n parallelism, concurrent invocations of _check_relative will race on the CWD, causing one test to open a relative path while another test has already changed the directory, producing spurious FileNotFoundError failures. Prefer constructing an absolute path via path.resolve() before calling check_file, or passing the absolute path directly, to eliminate the CWD dependency.

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: agentex/tests/unit/scripts/test_lint_migrations.py
   Line: 25-34
   
   Comment:
   **`os.chdir` in `_check_relative` is not thread-safe under parallel pytest**
   
   `os.chdir` changes the process-wide working directory. If tests run with `pytest-xdist` or any `-n` parallelism, concurrent invocations of `_check_relative` will race on the CWD, causing one test to open a relative path while another test has already changed the directory, producing spurious `FileNotFoundError` failures. Prefer constructing an absolute path via `path.resolve()` before calling `check_file`, or passing the absolute path directly, to eliminate the CWD dependency.
   
   How can I resolve this? If you propose a fix, please make it concise.

   

4. agentex/scripts/ci_tools/migration_lint.py, line 284-301 (link)

   _check_transaction_nesting silently clears all findings when autocommit_block appears anywhere in the file

   The guard at line 288 short-circuits with no findings as soon as autocommit_block( appears anywhere in the source. A migration with two concurrent index creates — one correctly wrapped in autocommit_block and one accidentally outside — passes the linter entirely. At runtime the unwrapped index fails with ActiveSQLError: CREATE INDEX CONCURRENTLY cannot run inside a transaction block. The rule needs to track which specific postgresql_concurrently=True occurrences fall inside the block, not just whether the block exists somewhere in the file.

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: agentex/scripts/ci_tools/migration_lint.py
   Line: 284-301
   
   Comment:
   **`_check_transaction_nesting` silently clears all findings when `autocommit_block` appears anywhere in the file**
   
   The guard at line 288 short-circuits with no findings as soon as `autocommit_block(` appears *anywhere* in the source. A migration with two concurrent index creates — one correctly wrapped in `autocommit_block` and one accidentally outside — passes the linter entirely. At runtime the unwrapped index fails with `ActiveSQLError: CREATE INDEX CONCURRENTLY cannot run inside a transaction block`. The rule needs to track which specific `postgresql_concurrently=True` occurrences fall inside the block, not just whether the block exists somewhere in the file.
   
   How can I resolve this? If you propose a fix, please make it concise.

   

Prompt To Fix All With AI

Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
uv.lock:138
**Unrelated `pyyaml` dependency and `aiohttp` downgrade in lock file**

`pyyaml` appears in `requires-dist` (the section derived from `pyproject.toml`), which means `pyproject.toml` was edited on this branch to add the dependency — but that file is not in the diff. The linter is documented as stdlib-only with no external deps, so `pyyaml` is not needed for this PR. Separately, `aiohttp` is downgraded from `3.13.5` to `3.13.4` with no stated motivation. Both changes appear to be stale branch noise; please confirm they are intentional or rebase/regenerate the lock file against main.

_{Reviews (9): Last reviewed commit: "fix(migrations): comment-guard every ful..." | Re-trigger Greptile}

[socket-security]

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff	Package	Supply Chain Security	Vulnerability	Quality	Maintenance	License
	pypi/​aiohttp@​3.13.5 ⏵ 3.13.4

View full report