test(cli): cover services and storage mv - #6362
Conversation
Supabase CLI previewnpx --yes https://pkg.pr.new/supabase/cli/supabase@875d0c8361af99a7796257e9350510ea216b1271Preview package for commit |
![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: 77e38fdf83
ℹ️ 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: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
|
/ai-review |
![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
Two of Claude's four findings are confirmed: the services live test performs redundant live fetches that add flake surface, and the storage cleanup helper is duplicated contrary to the trusted hoisting convention. The cleanup-diagnostic and hardcoded-row-count findings are refuted by concrete surrounding code. Codex reported no findings. No critical or major issues were found.
Findings
| Severity | Location | Category | Sources | Claim |
|---|---|---|---|---|
| 🟡 MINOR | apps/cli/src/legacy/commands/services/services.live.test.ts:30 |
test-flakiness |
claude | The test compares remote data from two independent CLI invocations, unnecessarily exposing the assertion to a second fallible Management API fetch. |
| 🟡 MINOR | apps/cli/src/legacy/commands/storage/mv/mv.live.test.ts:8 |
duplication |
claude | The new storage live test copies the shared flags and cleanup helper into a fourth command test instead of hoisting them. |
Refuted findings (kept for transparency, not posted as review comments)
apps/cli/src/legacy/commands/db/pull/pull.live.test.ts:56(error-handling): Pushing the raw unlink error loses both cleanup context and the affected file path.
Refuted: The original filesystem error is preserved, and unlink is called with the full joined path, which native filesystem errors report with the failing operation. throwWithCleanup at tests/helpers/live.ts:125-134 labels the failure as live cleanup, and the same raw-unlink-error convention appears in db/reset/reset.live.test.ts:25 and db/push/push.live.test.ts:25.apps/cli/src/legacy/commands/services/services.live.test.ts:15(test-quality): The exact row-count assertion is an inappropriate magic-number check for a live test.
Refuted: The exact count validates that the black-box command emitted the complete service list and intentionally changes when that output contract changes. The test remains a single services golden path as required by trusted/apps/cli/CLAUDE.md:490-505, and the trusted policy does not restrict live assertions to remote-only fields.
Stats
Claude findings: 4 · Codex findings: 0 · Confirmed: 2 · Refuted: 2 · 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.
1 participant
TL;DR
adds live e2e coverage for the
servicescommand andstorage mvwhats introduced?
services: links the project and proves the postgres remote version lands in the json outputproves the table renders that value in the LINKED cell, where a plain substring match would pass vacuously
storage mv: uploads an object, moves it through the real move endpoint, and proves via the listing that the destination exists and the source is gonecloses the storage family, since ls, cp and rm already have coverage
db pull: deflakes the shipped live test, pull exits nonzero when nothing changed by design, so the test now seeds remote-only schema through db query and proves the initial pull writes it back as a migrationref: