Uh oh!
There was an error while loading. Please reload this page.
fix(cli): stop skipping colliding schemas (CLI-2272) - #6394
Conversation
7ttp
commented
Aug 30, 2026
/ai-review |
There was a problem hiding this comment.
Superseded by a newer AI review
🤖 AI Review
The core pg_depend fix is sound and consistently applied across all six SQL copies. Of Claude's six findings, five are confirmed: the underlying duplication remains, the drift guard omits the Go SQL files, its regression regex is brittle, its source unescaping is incomplete, and the unchanged advisors query has a related cross-catalog OID bug. The test-organization finding is refuted because the guarded modules share one invariant and trusted conventions do not require module-matched test filenames. Codex reported no findings. No major or critical issues were found.
Findings
| Severity | Location | Category | Sources | Claim |
|---|---|---|---|---|
| 🟡 MINOR | apps/cli/src/legacy/commands/db/shared/legacy-migra.deno-templates.unit.test.ts:65 | duplication | claude | The new equality tests preserve four hand-copied SQL literals instead of consolidating their shared SQL definitions. |
| 🟡 MINOR | apps/cli-go/pkg/migration/queries/list.sql:6 | test-coverage | claude | The new drift guard covers only the four TypeScript copies and does not protect the two changed Go SQL files from regressing to the unconstrained join. |
| 🟡 MINOR | apps/cli/src/legacy/commands/db/advisors/advisors.lints-sql.ts:11 | correctness | claude | The advisors SQL has a related unconstrained pg_depend join that can hide an unindexed foreign key when an extension dependency from another catalog shares the table OID. |
| ⚪ NIT | apps/cli/src/legacy/commands/db/shared/legacy-migra.deno-templates.unit.test.ts:59 | test-quality | claude | The unconstrained-join regex is formatting-sensitive and can reject a corrected multiline join or miss an unconstrained join followed by same-line text. |
| ⚪ NIT | apps/cli/src/legacy/commands/db/shared/legacy-migra.deno-templates.unit.test.ts:49 | test-quality | claude | extractSqlLiteral performs unnecessary and incomplete template-literal unescaping. |
Findings outside the diff
- 🟡 MINOR
apps/cli/src/legacy/commands/db/advisors/advisors.lints-sql.ts:11— The advisors SQL has a related unconstrained pg_depend join that can hide an unindexed foreign key when an extension dependency from another catalog shares the table OID.
Refuted findings (kept for transparency, not posted as review comments)
apps/cli/src/legacy/commands/db/shared/legacy-migra.deno-templates.unit.test.ts:52(test-organization): The schema-listing guard is misplaced because it tests unrelated modules from a file named for migra Deno templates.
Refuted: The modules are related by the exact shared invariant under test, and the test is located in db/shared. The trusted testing guidance requires the .unit.test.ts project suffix but does not require every test filename to match one production module; the cited code-structure examples do not establish that rule.
Stats
Claude findings: 6 · Codex findings: 0 · Confirmed: 5 · Refuted: 1 · Uncertain: 0
Models: claude-opus-5 + gpt-5.6-sol · Trigger: manual · Workflow run
This review runs once per PR. A maintainer can request another with a /ai-review comment.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
TL;DR
fixes
db reset, db lint, db diff, andmigration downsilently skipping a user schema when its oid also appears in another catalogwhich was caused by joining
pg_dependon objid without theclassidthat scopesoidsto a single catalognow fixed by constraining the join to
pg_catalog.pg_namespacerows in every copy of the query.Resets now drop schemas that earlier versions silently skipped....
ref: