fix(cli): use Go's bare invalid-version error in db reset (CLI-1979) - #5969
Conversation
Go's reset.Run returns the bare repair.ErrInvalidVersion ("invalid
version number", reset.go:35-36); the "failed to parse <v>:" wrapper
belongs to migration repair only (repair.go:29). Drop the wrapper on
the db reset path, pin the exact message in the reset integration
test, and add a guard assertion that migration repair keeps its
wrapper.
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Supabase CLI previewnpx --yes https://pkg.pr.new/supabase/cli/supabase@40c81767e3ee4d02b31ce6ba0b714d67046dd2e7Preview package for commit |
kanadgupta
left a comment
kanadgupta
left a comment
There was a problem hiding this comment.
Tiny behavioral divergence that Claude discovered (see below) but otherwise LGTM
| return yield* Effect.fail( | ||
| new LegacyDbResetInvalidVersionError({ | ||
| message: `failed to parse ${v}: invalid version number`, | ||
| message: "invalid version number", |
There was a problem hiding this comment.
The message now byte-matches Go, but the condition still diverges from Go's strconv.Atoi in two edge cases, so this exact error isn't emitted in exactly the cases Go emits it:
- int64 overflow —
Atoi==ParseInt(s, 10, 0), which rejects values outside the int64 range, so Go'sdb reset --version 99999999999999999999fails with this bareinvalid version number(reset.go:35-36).INTEGER_PATTERN(/^[+-]?\d+$/u, line 51) accepts it, so the TS path falls through to the glob check and reportsglob supabase/migrations/…: file does not existinstead. - empty version — Go guards with
len(version) > 0(reset.go:34), so an empty--versionskips validation entirely and proceeds; hereOption.isSome+ the regex turn it into this error.
There's already a shared exact-Atoi mirror for this: legacyParseMigrationVersion (src/legacy/shared/legacy-migration-timestamp.format.ts:64), which migration repair uses for this same validation (repair.handler.ts:186 — repair's tests even pin the 20-digit overflow case). Replacing the INTEGER_PATTERN.test(v) check with legacyParseMigrationVersion(v) === undefined (plus a v.length > 0 guard mirroring reset.go:34) would close both gaps and drop the duplicated regex.
Both divergences predate this PR, so a follow-up is fine if you'd rather keep this diff minimal — but since the ticket's goal is byte-parity on this exact error path, worth tracking.
2 participants
What changed
supabase db reset --version <non-integer>in the TS legacy shell emittedfailed to parse <v>: invalid version number. The Go CLI'sreset.Runreturns the barerepair.ErrInvalidVersion=invalid version number(apps/cli-go/internal/db/reset/reset.go:35-36); thefailed to parse <v>:wrapper belongs tomigration repaironly (apps/cli-go/internal/migration/repair/repair.go:29). This drops the wrapper on the reset path only:reset.handler.ts: emit the bareinvalid version number(exit code and--debughint unchanged).reset.errors.ts: JSDoc now cites the correct Go source (reset.go:35-36) instead of the repair-path lines that led to the original conflation.reset.integration.test.ts: the previoustoContain("invalid version number")assertion passed for both the wrapped and bare messages (so it could not catch this divergence) — it now pins the exact tag + message.repair.integration.test.ts: guard assertion thatmigration repairkeeps itsfailed to parse <v>:wrapper (correct Go parity there), so a future unification can't regress either side.db reset --lastvalidation and messages are untouched (already at parity).SIDE_EFFECTS.mdfordb resetalready documented the bare message, so code and doc now agree.Why
Strict 1:1 Go parity for the legacy shell. Empirically confirmed on both binaries: Go prints
invalid version number, TS printedfailed to parse abc: invalid version number(both exit 1). Audit ref:apps/cli/docs/go-parity-audit-2026-07-24.md§3.2 (R1).Deliberately left open
Reviewers noted a pre-existing, adjacent divergence: the reset path's
INTEGER_PATTERNaccepts >int64 numeric versions (e.g. 20 digits) that Go'sstrconv.Atoirejects withinvalid version number; TS instead falls through to the globfile does not existerror.migration repairalready solves this via the sharedlegacyParseMigrationVersionhelper. Out of CLI-1979's scope (wrapper text only) — candidate for a follow-up issue.Fixes CLI-1979
https://linear.app/supabase/issue/CLI-1979/db-reset-version-non-integer-error-text-diverges-from-go