docs(cli): record intentional start --ignore-health-check divergence from Go (CLI-1987) - #6007
Conversation
…from Go (CLI-1987) Go's start.IsUnhealthyError classifies any errors.Join-shaped error as "unhealthy", so with --ignore-health-check a total image-pull failure or a Docker daemon that dies during the pre-pull is swallowed: Go prints the error, skips rollback, prints the "Started supabase local development setup." banner + status table + security notice, and exits 0 with no container running. Per the CLI-1987 ruling (2026-07-30, option b) that quirk is intentionally NOT reproduced: the TS port keeps the scenario fatal — exit 1, no success banner, no status table. Docs + comments + one regression test only; no runtime change: - start.rollback.ts: divergence record on legacyIsUnhealthyStartError so a future "fix" toward Go's shape check doesn't happen silently - start/SIDE_EFFECTS.md: exit-code carve-out + Notes entry - docs/go-cli-porting-status.md: start entry divergence note (table re-padded by oxfmt) - start.integration.test.ts: pins pre-pull failure under --ignore-health-check to exit 1 with no banner/status table/rollback Fixes CLI-1987
|
@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". |
…7-parity-ruling-start-ignore-health-check-on-image-pulldaemon
![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: 5161af9fcc
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
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@d0b81a9a04c03086eb0ccebe106596a5d6758362Preview package for commit |
…e delta record (review: codex) - start.rollback.ts + SIDE_EFFECTS.md: the pre-pull failure is fatal because it propagates from before bring-up without reaching either legacyIsUnhealthyStartError call site — not because of the matcher exclusion (widening it would be a no-op); the observable delta also includes the security notice Go prints unconditionally at Run()'s tail - start.integration.test.ts: model the daemon-unreachable trigger (immediate inspect failure) instead of duplicating the flagless pull-exhaustion scenario through ~36s of real retry backoff
|
@codex review All three round findings addressed in 69bdc5f: divergence record re-attributed to control flow (matcher widening is a no-op — verified against the two call sites), delta record completed with the security notice (verified against start.go:84-87), and the new integration test re-modeled onto the immediate daemon-unreachable path (~25ms instead of ~36s of real retry backoff). |
|
Codex Review: Didn't find any major issues. Keep them coming! 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". |
kanadgupta
left a comment
kanadgupta
left a comment
There was a problem hiding this comment.
Tiny assertion enhancement suggestion from Claude below but non-blocking — LGTM!
…-quirk pin (CLI-1987)
2 participants
⚖ Parity ruling applied (CLI-1987, Colum, 2026-07-30)
Option (b) chosen: keep the TypeScript behaviour and document the intentional divergence. Option (a) — matching Go's quirk — was rejected. No runtime behaviour changes in this PR.
Go's
start.IsUnhealthyError(apps/cli-go/internal/db/start/start.go:227-231) classifies anyerrors.Join-shaped error as "unhealthy". That shape check accidentally also matchesensureImagesCached'serrors.Join(result...)(apps/cli-go/internal/start/start.go:257-260), so in Go, with--ignore-health-checkset, a total image-pull failure — or a Docker daemon that becomes unreachable during the pre-pull — is swallowed: Go prints the error, skips rollback, printsStarted supabase local development setup.+ the status table + the security notice, and exits 0 with zero containers running. Go's own comment onIsUnhealthyError("Health check always returns a joinError") shows the wider match was never intended.The TS port already behaves differently —
legacyIsUnhealthyStartErrormatches onlyLegacyHealthCheckTimeoutError, and the image pre-pull runs before the downgrade envelope — so the same scenario exits 1 with no success banner and no status table. Per the ruling, that behaviour is kept and is now documented + regression-pinned.--ignore-health-checkdowngrades health-check timeouts only.What changed
Docs, comments, and one regression test — the runtime is untouched:
apps/cli/src/legacy/commands/start/start.rollback.ts— divergence record inlegacyIsUnhealthyStartError's doc comment, including an explicit "do not fix this by widening the match toward Go's shape check" guard, so a future parity sweep can't silently reintroduce Go's exit-0 swallow.apps/cli/src/legacy/commands/start/SIDE_EFFECTS.md— the image-pull exit-code row now states the failure stays fatal even with--ignore-health-check, and a new "Notes" entry records the full carve-out (scenario, Go's quirk behaviour, TS behaviour, why rollback is not part of the divergence — nothing has been created yet in either CLI — and that the flag's Go-byte-matched help text "Ignore unhealthy services and exit 0" over-promises here).apps/cli/docs/go-cli-porting-status.md— the legacystartentry carries the same intentional-divergence note (the table's column padding was re-flowed byoxfmt; the substantive change is thestartrow only).apps/cli/src/legacy/commands/start/start.integration.test.ts— new test in the "image pull" block: pre-pull exhaustion under--ignore-health-checkstill fails withLegacyImagePrepullError, prints noStartedbanner, emits nothing on stdout (no status table), creates no container, and triggers no rollback. The daemon-unreachable trigger funnels through the sameLegacyImagePrepullErrorpath, so the one scenario pins both documented triggers.Overlap with CLI-1967
CLI-1967's doc-drift sweep also touches
startdocumentation. The CLI-1987 carve-out (SIDE_EFFECTS "Notes" entry, exit-code row, porting-statusstartrow,start.rollback.tscomment) is fully handled here — CLI-1967 should not re-document this divergence.Possible follow-up (not in scope here)
Self-review flagged a DX gap that would require a runtime change, so it is deliberately not part of this docs-only ruling PR: when
--ignore-health-checkis set and the pre-pull fails, the error output never explains why the flag didn't apply (and the flag's help text says "exit 0"). A TS-onlyerror.suggestion("--ignore-health-check only downgrades health-check timeouts; image pull failures are always fatal") on that path — analogous to the existing TS-onlyexec format errorsuggestion — would close it.Fixes CLI-1987