Repository navigation
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 30ba92cbd5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Supabase CLI previewnpx --yes https://pkg.pr.new/supabase/cli/supabase@2007a82521ba77fee5cb4ccf53b67bed2cf65850Preview package for commit |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ce99a402b3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 32335b4539
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8335bfffb9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7f55c997c2
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 605f369e0f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a6bd7327e7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Superseded by a newer AI review
🤖 AI Review
Both independent reviews completed (8 Claude findings, 4 Codex findings). After merging overlapping SQL-classifier findings, all 10 resulting findings are confirmed. The major issues are unsafe/incomplete classification of statements that must run outside the new explicit transaction and broken CALL/DO transaction-control workflows. The remaining findings concern localized error handling, documentation, redundant role restoration, cleanup robustness, maintainability, and regression-test coverage.
Findings
| Severity | Location | Category | Sources | Claim |
|---|---|---|---|---|
| 🟠 MAJOR | apps/cli/src/legacy/shared/legacy-migration-apply.ts:67 |
sql-parsing |
claude+codex | The new regex-based SQL classifiers are not token-aware: they can match action keywords inside literals and miss incompatible commands split by newlines or internal comments, causing unexpected commits or SQLSTATE 25001 failures. |
| 🟠 MAJOR | apps/cli/src/legacy/shared/legacy-migration-apply.ts:134 |
sql-compatibility |
claude | The incompatible-statement classifier omits ALTER TABLE … DETACH PARTITION … CONCURRENTLY, so PostgreSQL rejects that valid migration command inside the new explicit transaction. |
| 🟠 MAJOR | apps/cli/src/legacy/shared/legacy-migration-apply.ts:134 |
sql-compatibility |
codex | The incompatible-statement classifier omits CHECKPOINT, causing migrations containing it to fail inside the new explicit transaction. |
| 🟠 MAJOR | apps/cli/src/legacy/shared/legacy-db-connection.sql-pg.layer.ts:322 |
behavior-regression |
codex | The unconditional explicit transaction breaks top-level CALL and DO statements whose procedure or block performs transaction control. |
| 🟡 MINOR | apps/cli/src/legacy/shared/legacy-db-connection.sql-pg.layer.ts:249 |
error-handling |
claude | BEGIN-failure labeling depends on the localized PostgreSQL severity string, so non-English servers omit the new explanatory prefix. |
| 🟡 MINOR | apps/cli/src/legacy/commands/db/push/SIDE_EFFECTS.md:32 |
documentation |
claude | The affected SIDE_EFFECTS.md files document BEGIN and COMMIT but omit the new failure-path ROLLBACK database mutation. |
| 🟡 MINOR | apps/cli/src/legacy/shared/legacy-db-connection.sql-pg.integration.test.ts:568 |
test-coverage |
claude | No test verifies the user-visible transaction-block behavior that motivated the change against PostgreSQL; current batch tests only verify protocol framing through a fake server. |
| ⚪ NIT | apps/cli/src/legacy/shared/legacy-migration-apply.ts:799 |
correctness |
claude | When the last statement is both standalone-incompatible and role-reverting, the stepped-down role restore is sent twice with no intervening user statement. |
| ⚪ NIT | apps/cli/src/legacy/shared/legacy-db-connection.sql-pg.layer.ts:266 |
maintainability |
claude | legacyShouldDiscardBatchClient is documented as deciding the discard policy, but the rollback-dependent part of that policy is implemented separately at its call site and cannot be covered by its unit tests. |
| ⚪ NIT | apps/cli/src/legacy/shared/legacy-db-connection.sql-pg.layer.ts:1179 |
robustness |
claude | If activeClient.release throws, the checked-out client's error listener is not removed because listener cleanup is not protected by finally. |
Stats
Claude findings: 8 · Codex findings: 4 · Confirmed: 10 · Refuted: 0 · 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.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9cea8df10d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![depthfirst-app[bot]](https://avatars.githubusercontent.com/in/1021002?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f83f6723ec
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d3beb8dfc8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b0f6cba496
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a2704579e8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
avallete
left a comment
avallete
left a comment
There was a problem hiding this comment.
There are discussion about not using BEGIN/COMMIT at all.
The bug might actually be that we wrap thing into this kind of transaction. If that was absent, user could fix it by wrapping their LOCK TABLE + ALTER into their own controlled BEGIN ... COMMIT.
Or maybe we should make that explicit for the user via a config.toml option they can control instead (should we make their migrations transactionals + an explicit escape hatch for some migrations) or do they handle it manually for every files.
Umm the
users who want their own control also have that today: a file with authored well i think wrapping it or not is a bigger call, which needs to be taken care of separately. |
…n-batch-is-not-a-transaction
# Conflicts: # apps/cli/src/command-internal/db-connection.service.ts # apps/cli/src/command-internal/db-connection.sql-pg.integration.test.ts # apps/cli/src/command-internal/db-connection.sql-pg.layer.ts # apps/cli/src/command-internal/db-connection.sql-pg.unit.test.ts # apps/cli/src/command-internal/migration-apply.ts # apps/cli/src/command-internal/migration-apply.unit.test.ts
oxfmt line-wrapping drifted in the merge conflict resolution for db-connection.sql-pg.unit.test.ts.
|
Merged `develop` into this branch to resolve the merge conflicts introduced by #6525 (mechanical removal of the legacy-shell `Legacy`/`legacy` naming prefix across ~1400 files, unrelated to this PR). No functional changes to this PR's own logic — conflict resolution was limited to renaming identifiers/imports (e.g. `LegacyDbExecError` → `DbExecError`, `legacyIsPipelineIncompatible` → `isPipelineIncompatible`, `legacy-migration-apply.ts` → `migration-apply.ts`) to match develop's post-rename names, while preserving every bit of this PR's own added behavior (explicit-transaction batching, rollback handling, standalone-statement role restore, and their tests). Also renamed a few net-new private helpers/test tmpdir prefixes that happened to carry the old `legacy` naming convention themselves (e.g. `legacyIsConnectionEndingSqlState` → `isConnectionEndingSqlState`), for internal consistency with the rest of the now-de-legacied file. Ran `pnpm check:all` after the merge — all checks pass except a pre-existing, unrelated `cli-go` `golangci-lint` gosec finding in files this PR/merge never touches. Also ran the full unit + integration suites for every file touched by the conflict resolution (231 tests) — all green. @7ttp please double check the resolution when you get a chance, especially |
7ttp
added
do not merge
7ttp
changed the title
|
/ai-review |
There was a problem hiding this comment.
🤖 AI Review
Verified all five Claude findings against the checked-out code and trusted conventions. Confirmed recurring replay warnings, misleading wording for non-migration SQL files, and a SET LOCAL warning false negative. Refuted the contradictory-advice and warning-prefix claims. Codex reported no findings. No critical or major issues were confirmed; tests were not run.
Findings
| Severity | Location | Category | Sources | Claim |
|---|---|---|---|---|
| 🟡 MINOR | apps/cli/src/command-internal/migration-apply.ts:559 |
ux |
claude | The migration guidance repeats when historical migrations are replayed during reset, shadow-database operations, and migration squash, creating recurring warnings about previously applied files. |
| 🟡 MINOR | apps/cli/src/command-internal/migration-apply.ts:487 |
correctness |
claude | Any transaction-control statement suppresses the SET LOCAL warning for the entire file, including SET LOCAL statements that occur before BEGIN or after COMMIT and have no effect. |
| ⚪ NIT | apps/cli/src/command-internal/migration-apply.ts:560 |
ux |
claude | The split warning tells users to create a migration file even when the helper is executing a globals file or a declarative schema file. |
Refuted findings (kept for transparency, not posted as review comments)
apps/cli/src/command-internal/migration-apply.ts:488(ux): The SET LOCAL warning advises BEGIN/COMMIT even for transaction=false files, contradicting the advice to execute transaction-incompatible statements without BEGIN/COMMIT.
Refuted: The advice concerns different statements. SET LOCAL needs a transaction, whereas the 25001 hint concerns statements that cannot run inside one. At :501-506 and :549-550, directive-marked files execute every authored statement sequentially without rejecting or removing BEGIN/COMMIT. An explicit block around SET LOCAL and its dependent transactional statements therefore works while retaining the directive and leaving incompatible statements outside that block.apps/cli/src/command-internal/migration-apply.ts:483(consistency): Using the Warning: prefix instead of WARN: violates the repository's established raw-stderr warning convention, and the changed unit assertion hides that mismatch.
Refuted: Both prefixes already exist. Existing raw-stderr warnings use Warning: in command-internal/stack-shadow.ts:130-134 and :149-152, also present in the trusted default-branch copy. Trusted docs/adr/0027-cli-tracing-conventions.md:23-26 explicitly documents a Warning: stderr message. There is no exclusive WARN: convention, and the updated assertion checks the prefix actually emitted by this helper.
Stats
Claude findings: 5 · Codex findings: 0 · Confirmed: 3 · Refuted: 2 · Uncertain: 0
Models: claude-opus-5-5 + gpt-6.1-sol · Trigger: manual · Workflow run
This review runs once per PR. A maintainer can request another with a /ai-review comment.
…n-batch-is-not-a-transaction
This branch has not been deployed
3 participants
TL;DR
Add actionable migration transaction hints and warnings while preserving existing execution behavior.
whats biting?
The per-file pipeline does not add
BEGINorCOMMIT, so statements such asLOCK TABLEcan fail with SQLSTATE 25P01.SET LOCALmay have no effect without an authored transaction block, and automatically split files can remain partially applied after a failure.now fixed by
SQLSTATE 25P01 errors recommend authored
BEGIN; ... COMMIT;. SQLSTATE 25001 errors recommend a separate migration starting with-- pg-delta: transaction=false, withoutBEGINorCOMMIT.Per-file warnings flag bare
SET LOCALand automatic splitting, explain partial application, and preserve machine-readable output. Existing batching, authored boundaries, directive handling, and migration-history behavior remain in place.ref