Conversation
Contributor
There was a problem hiding this comment.
🤖 AI Review
The PR changes only comments, documentation, and tests. Of Claude's five findings, two documentation/test-fidelity nits are confirmed and three are refuted by code scope, existing coverage, or trusted repository conventions. Codex reported no findings. Tests could not run because pr/node_modules is absent; verdicts are based on source inspection.
Findings
| Severity | Location | Category | Sources | Claim |
|---|---|---|---|---|
| ⚪ NIT | apps/cli/src/commands/db/reset/SIDE_EFFECTS.md:207 |
documentation |
claude | The text-mode documentation omits that the stdin read also occurs on a TTY and is bounded to 100 ms. |
| ⚪ NIT | apps/cli/src/commands/db/reset/reset.integration.test.ts:426 |
test-coverage |
claude | The piped-input test does not model non-TTY stdin even though its fixture option is documented as non-TTY input. |
Refuted findings (kept for transparency, not posted as review comments)
apps/cli/src/commands/db/reset/reset.integration.test.ts:500(documentation): The comment abovemockStdinincorrectly says stdin is only a service dependency despite local bucket confirmations consumingpipedStdin.
Refuted: The finding drops the comment's explicit remote-reset scope. For that path, TTY mode routes the confirmation throughmockOutput, and stdin is only present to satisfy the Effect dependency. Local bucket tests use stdin separately.apps/cli/src/commands/db/reset/SIDE_EFFECTS.md:206(test-coverage): The machine-format bucket-confirmation behavior needs another reset integration test because none of the four new tests use a machine format.
Refuted: The underlying machine-format confirmation behavior and the local-reset JSON path already have integration coverage. Trusted/CLAUDE.md:171-172 explicitly prefers some test duplication avoidance and directs reviewers to reject redundant assertions.apps/cli/src/commands/db/reset/reset.integration.test.ts:463(maintainability): The inline recording HTTP client should be extracted because similar route-matching mocks exist elsewhere.
Refuted: There is no reusable recording-client factory being duplicated; the shared helper exposes a full storage-command setup with different routing capabilities. Trusted/CLAUDE.md:171 says to accept some test duplication rather than introduce unnecessary abstractions, and trusted/apps/cli/CLAUDE.md:35-39 says not to extract every helper used twice.
Stats
Claude findings: 5 · Codex findings: 0 · Confirmed: 2 · Refuted: 3 · Uncertain: 0
Models: claude-opus-5 + gpt-5.6-sol · Trigger: auto · Workflow run
This review runs once per PR. A maintainer can request another with a /ai-review comment.
Coly010
approved these changes
Sep 16, 2026
7ttp
deleted the
7ttp/cli-2433-supabase-db-reset-prompts-on-stderr-and-blocks-on-stdin
branch
September 16, 2026 08:59
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TL;DR
fixes the stale claim that
db resetbucket seeding takes confirmation defaults silently,which was caused by the call site comment and
SIDE_EFFECTS.mdnever describing the real print and scan contract and is now fixed by correcting both and pinning the behavior with four integration tests covering the overwrite and prune defaults, theSUPABASE_YESdotenv path, and the piped answer path...ref:
supabase db resetprompts on stderr and blocks on stdin during bucket seeding #6607