feat(cli): add experimental stack restart - #6512
Conversation
![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
Only Claude's independent review was available; the Codex review did not complete. Seven minor/nit findings are confirmed, covering duplicated helpers, required branch coverage, consistency, and incomplete side-effect documentation. The runtime-mismatch finding is refuted by the stack API and implementation. No critical or major defects were found.
Findings
| Severity | Location | Category | Sources | Claim |
|---|---|---|---|---|
| 🟡 MINOR | apps/cli/src/commands/experimental/stack/restart/restart.handler.ts:83 |
code-duplication |
claude | The restart handler duplicates status serialization and rendering helpers already used by the start command, contrary to the trusted repository convention requiring shared family helpers to be hoisted. |
| 🟡 MINOR | apps/cli/src/commands/experimental/stack/restart/restart.handler.ts:129 |
test-coverage |
claude | The malformed --stack-id rejection branch is not exercised by the restart integration tests, violating the repository's 100% handler branch-coverage requirement. |
| 🟡 MINOR | apps/cli/src/commands/experimental/stack/restart/SIDE_EFFECTS.md:1 |
documentation |
claude | The new SIDE_EFFECTS.md does not provide the required structured coverage of files, API activity, environment variables, exit codes, telemetry, and output modes. |
| 🟡 MINOR | apps/cli/src/commands/experimental/stack/restart/restart.integration.test.ts:36 |
test-coverage |
claude | The restart fixture always uses empty endpoints and capabilities, leaving the renderer's endpoint and dormant-capability branches uncovered; the fixture also exposes unused missing-config and projectRoot affordances. |
| ⚪ NIT | apps/cli/src/commands/experimental/stack/restart/restart.handler.ts:117 |
consistency |
claude | The legacy output-flag error omits the suggestion field consistently supplied by sibling experimental stack commands. |
| ⚪ NIT | apps/cli/src/commands/experimental/stack/restart/restart.handler.ts:21 |
style |
claude | StackError is referenced through repeated inline import types despite an existing top-level import from the same module. |
| ⚪ NIT | apps/cli/src/commands/experimental/stack/restart/restart.handler.ts:144 |
error-handling |
claude | A missing explicitly named stack receives the generic current-project not-found message rather than identifying the requested name. |
Refuted findings (kept for transparency, not posted as review comments)
apps/cli/src/commands/experimental/stack/restart/restart.handler.ts:40(error-handling): StackRuntimeMismatchError cannot arise during restart and is therefore incorrectly classified as a flags error with inapplicable advice.
Refuted: Concrete package code contradicts both premises: runtime mismatch is part of openStack's public failure contract and is constructed outside createStack, while choosing another --stack name is an available restart action. Sibling start and prepare mappings also classify this error as flags-related.
Stats
Claude findings: 8 · Codex findings: 0 · Confirmed: 7 · Refuted: 1 · 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.
0f58457 to
37240ee
Compare
37240ee to
c98065a
Compare
c98065a to
c7e2e96
Compare
Supabase CLI previewnpx --yes https://pkg.pr.new/supabase/cli/supabase@c7e2e96f89c6ad02a57ff7161a5923d2c18385c1Preview package for commit |
|
/ai-review |
There was a problem hiding this comment.
🤖 AI Review
Only Claude's independent review was available; the Codex pass did not complete. All eight reported concerns were verified in the checked-out code. None is critical or major: three are minor consistency/UX/duplication issues and five are nits involving interruption cleanup, maintainability, style, documentation, and test coverage.
Findings
| Severity | Location | Category | Sources | Claim |
|---|---|---|---|---|
| 🟡 MINOR | apps/cli/src/commands/experimental/stack/restart/restart.handler.ts:49 |
error-handling |
claude | Restart classifies StackRuntimeError and StackCleanupError as user-actionable lifecycle/configuration failures, while start classifies the same errors from stack.start() as unknown failures with debug guidance, producing inconsistent telemetry and remediation. |
| 🟡 MINOR | apps/cli/src/commands/experimental/stack/restart/restart.handler.ts:101 |
duplication |
claude | Restart duplicates stack target validation and resolution logic already implemented across sibling handlers instead of sharing it, contrary to the trusted Hoist Before You Duplicate convention. |
| 🟡 MINOR | apps/cli/src/commands/experimental/stack/restart/restart.handler.ts:149 |
user-experience |
claude | If stop succeeds and start fails, the stack remains stopped, but the surfaced error does not disclose that state change or provide explicit recovery guidance. |
| ⚪ NIT | apps/cli/src/commands/experimental/stack/restart/restart.handler.ts:144 |
user-experience |
claude | Interrupting restart during prepare, stop, or start leaves its progress task unsettled because only ordinary success and typed-error paths finalize it. |
| ⚪ NIT | apps/cli/src/commands/experimental/stack/restart/restart.errors.ts:25 |
maintainability |
claude | The actionability getter uses a non-exhaustive if chain with a redundant port branch, so a future reason can silently fall through to unknown. |
| ⚪ NIT | apps/cli/src/commands/experimental/stack/stack.shared.ts:12 |
style |
claude | StackStatus is imported through a separate third import declaration from @supabase/stack/effect instead of being included in the existing import block. |
| ⚪ NIT | apps/cli/src/commands/experimental/stack/restart/restart.command.ts:17 |
documentation |
claude | The new restart command supplies no Command.withExamples metadata, so its generated help and reference material lack examples for current, named, or ID-based target selection. |
| ⚪ NIT | apps/cli/src/commands/experimental/stack/restart/restart.integration.test.ts:305 |
test-coverage |
claude | The tests omit the unnamed missing-target branch, inspectStack failure handling for --stack-id, and verification of the projectRoot/name query passed to findStack. |
Stats
Claude findings: 8 · Codex findings: 0 · Confirmed: 8 · 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.
| Match.tag( | ||
| "StackLifecycleConflictError", | ||
| "StackNotRunningError", | ||
| "StackMustBeStoppedError", | ||
| "StackOwnershipConflictError", | ||
| "StackUpgradeRequiredError", | ||
| "StackRuntimeError", | ||
| "StackCleanupError", | ||
| () => ({ | ||
| reason: "lifecycle" as const, | ||
| suggestion: "Run supabase experimental stack status to inspect the stack state.", | ||
| }), | ||
| ), |
There was a problem hiding this comment.
🟡 MINOR · error-handling · source: claude
Restart classifies StackRuntimeError and StackCleanupError as user-actionable lifecycle/configuration failures, while start classifies the same errors from stack.start() as unknown failures with debug guidance, producing inconsistent telemetry and remediation.
Evidence: restart.handler.ts:49-61 maps both tags to lifecycle; restart.errors.ts:25-34 maps lifecycle to actionability.invalidConfig. start.handler.ts:193-200 maps them to unknown, and packages/stack/src/public/Errors.ts:276-297 confirms both can be returned by start().
Suggested fix: Classify these start-phase failures consistently with start.handler.ts, using unknown and phase-appropriate debug guidance, then update the restart test.
| const target = yield* stackId !== undefined | ||
| ? Effect.gen(function* () { | ||
| const id = stackId; | ||
| if (!isStackId(id)) | ||
| return yield* new LegacyExperimentalStackRestartError({ | ||
| reason: "flags", | ||
| message: "--stack-id must be a lowercase SHA-256 stack id", | ||
| }); | ||
| const inspection = yield* catchStackError(api.inspectStack(id)); | ||
| return { id, projectRoot: inspection.descriptor.projectRoot }; | ||
| }) | ||
| : Effect.gen(function* () { | ||
| const found = yield* catchStackError( | ||
| api.findStack({ | ||
| projectRoot: settings.workdir, | ||
| ...(stackName === undefined ? {} : { name: stackName }), | ||
| }), | ||
| ); | ||
| if (Option.isNone(found)) | ||
| return yield* new LegacyExperimentalStackRestartError({ | ||
| reason: "not-found", | ||
| message: | ||
| stackName === undefined | ||
| ? "No managed stack exists for the selected project." | ||
| : `No managed stack named "${stackName}" was found for this project.`, | ||
| suggestion: | ||
| stackName === undefined | ||
| ? "Run supabase experimental stack start first." | ||
| : "Choose an existing --stack name or omit --stack for the current project.", | ||
| }); | ||
| return { id: found.value.id, projectRoot: found.value.projectRoot }; | ||
| }); |
There was a problem hiding this comment.
🟡 MINOR · duplication · source: claude
Restart duplicates stack target validation and resolution logic already implemented across sibling handlers instead of sharing it, contrary to the trusted Hoist Before You Duplicate convention.
Evidence: restart.handler.ts:15-23 repeats the mutual-exclusion validation found in start, stop, prepare, logs, and status. restart.handler.ts:101-132 closely repeats status.handler.ts:133-160. trusted/apps/cli/CLAUDE.md:233-250 requires overlapping same-family handler logic to be hoisted.
Suggested fix: Hoist common target validation/resolution into stack.shared.ts and refactor existing callers, or reuse LegacyExperimentalStackTargetResolver for its applicable validation and inspection behavior while sharing the existing-stack lookup separately.
| yield* catchStackError(stack.stop()).pipe(Effect.tapError((error) => task.fail(error.message))); | ||
| const status = yield* catchStackError(stack.start({ config })).pipe( | ||
| Effect.tapError((error) => task.fail(error.message)), | ||
| Effect.tap(() => task.clear()), | ||
| ); |
There was a problem hiding this comment.
🟡 MINOR · user-experience · source: claude
If stop succeeds and start fails, the stack remains stopped, but the surfaced error does not disclose that state change or provide explicit recovery guidance.
Evidence: restart.handler.ts:149-153 performs stop before start and applies the same phase-agnostic mapper to start failures. SIDE_EFFECTS.md:61-66 documents that a start failure leaves the stack stopped, and restart.integration.test.ts:242-257 verifies that resulting lifecycle.
Suggested fix: Map start-phase failures separately and append guidance explaining that the stack is stopped and can be recovered after fixing the reported problem.
| const task = yield* output.task("Preparing local Supabase stack..."); | ||
| yield* catchStackError(stack.prepare({ config })).pipe( | ||
| Effect.tapError((error) => task.fail(error.message)), | ||
| ); | ||
| yield* task.message("Restarting local Supabase stack..."); | ||
| yield* catchStackError(stack.stop()).pipe(Effect.tapError((error) => task.fail(error.message))); | ||
| const status = yield* catchStackError(stack.start({ config })).pipe( | ||
| Effect.tapError((error) => task.fail(error.message)), | ||
| Effect.tap(() => task.clear()), | ||
| ); |
There was a problem hiding this comment.
⚪ NIT · user-experience · source: claude
Interrupting restart during prepare, stop, or start leaves its progress task unsettled because only ordinary success and typed-error paths finalize it.
Evidence: restart.handler.ts:144-153 only calls task.fail through Effect.tapError and task.clear through Effect.tap. output.service.ts:7-14 exposes cancel and clear, while shared/cli/run.ts:831-849 interrupts the command fiber on a signal; no interruption finalizer settles this task.
Suggested fix: Attach Effect.onInterrupt to cancel or clear the task around the lifecycle sequence, and consider applying the same cleanup to sibling stack handlers.
| get [ErrorActionabilityId](): CliErrorActionabilityDeclaration { | ||
| if (this.reason === "flags" || this.reason === "not-found") return actionability.provideFlags; | ||
| if (this.reason === "invalid-config" || this.reason === "lifecycle") | ||
| return actionability.invalidConfig; | ||
| if (this.reason === "port") return actionability.invalidConfig; | ||
| if (this.reason === "docker") return actionability.dockerNotRunning; | ||
| if (this.reason === "registry" || this.reason === "artifact") | ||
| return actionability.externalNetwork; | ||
| return actionability.unknown; | ||
| } |
There was a problem hiding this comment.
⚪ NIT · maintainability · source: claude
The actionability getter uses a non-exhaustive if chain with a redundant port branch, so a future reason can silently fall through to unknown.
Evidence: restart.errors.ts:25-34 returns invalidConfig separately for port and for invalid-config/lifecycle, then defaults to unknown. start.errors.ts:33-50 uses an exhaustive switch over its closed reason union.
Suggested fix: Use an exhaustive switch and group reasons that share the same actionability.
| type StackRuntimePreference, | ||
| } from "@supabase/stack/effect"; | ||
| import type { StackId } from "@supabase/stack"; | ||
| import type { StackStatus } from "@supabase/stack/effect"; |
There was a problem hiding this comment.
⚪ NIT · style · source: claude
StackStatus is imported through a separate third import declaration from @supabase/stack/effect instead of being included in the existing import block.
Evidence: stack.shared.ts:2-10 imports values and StackRuntimePreference from @supabase/stack/effect, line 12 separately imports StackStatus, and line 13 separately imports StackNotFoundError from the same module.
Suggested fix: Combine the imports from @supabase/stack/effect into one declaration.
| export const legacyExperimentalStackRestartCommand = Command.make("restart", config).pipe( | ||
| Command.withDescription("Restart an existing managed local Supabase stack."), | ||
| Command.withShortDescription("Restart a managed local stack"), | ||
| Command.withHandler((flags) => | ||
| legacyExperimentalStackRestart(flags).pipe( | ||
| withLegacyCommandInstrumentation({ flags, config }), | ||
| withJsonErrorHandling, | ||
| ), | ||
| ), |
There was a problem hiding this comment.
⚪ NIT · documentation · source: claude
The new restart command supplies no Command.withExamples metadata, so its generated help and reference material lack examples for current, named, or ID-based target selection.
Evidence: restart.command.ts:17-25 includes descriptions and a handler but no examples. start.command.ts:32-41, stop.command.ts:20-25, prepare.command.ts:29-38, and logs.command.ts:44-53 provide examples; trusted/docs/adr/0003-self-documenting-cli.md identifies command examples as generated reference metadata.
Suggested fix: Add examples for restarting the current project stack and a named or ID-selected stack using the full experimental command path.
| it.effect("fails a missing named target without lifecycle calls", () => { | ||
| const fixture = makeFixture({ target: "name", missingTarget: true }); | ||
| return fixture.effect.pipe( | ||
| Effect.flip, | ||
| Effect.tap((error) => | ||
| Effect.sync(() => { | ||
| expect(error.message).toContain('No managed stack named "feature-a"'); | ||
| expect(error.suggestion).toContain("existing --stack name"); | ||
| expect(fixture.calls).toEqual([]); | ||
| }), | ||
| ), | ||
| ); | ||
| }); |
There was a problem hiding this comment.
⚪ NIT · test-coverage · source: claude
The tests omit the unnamed missing-target branch, inspectStack failure handling for --stack-id, and verification of the projectRoot/name query passed to findStack.
Evidence: restart.integration.test.ts:305-317 tests only a missing named target. The fixture at lines 131-141 makes inspectStack always succeed and makes findStack ignore its argument, leaving restart.handler.ts:109 and the distinct unnamed branch at lines 120-129 unverified.
Suggested fix: Add unnamed missing-target and failing inspectStack cases, and capture the findStack input to assert projectRoot and name.
1 participant
supabase experimental stack restartstops and starts an existing managed stack with the selected project’s configuration while preserving its identity and data. Validate configuration and prepare artifacts before stopping; if startup fails afterward, the same stack remains available for recovery.This PR builds on experimental stack prepare (#6511).