From 48b09a2675cc5a23847c91599bab96b5ac5191c9 Mon Sep 17 00:00:00 2001 From: Colum Ferry Date: Wed, 8 Jul 2026 12:14:47 +0100 Subject: [PATCH] fix(cli): exit 0 for bare group commands to match Go cobra parity Invoking a legacy-shell group command with subcommands but no subcommand and no --help (e.g. `supabase branches`, `supabase completion`) exited 1. Go's cobra CLI exits 0 for the identical invocation: a non-Runnable command with no RunE returns flag.ErrHelp internally, which ExecuteC() maps to "print help, return nil error". CliError.ShowHelp already declares the correct exit code via Effect's own Runtime.errorExitCode marker (0 for a clean ShowHelp with no errors, 1 otherwise), so run.ts now delegates to Runtime.getErrorExitCode(Cause.squash(cause)) instead of hand-rolling ShowHelp classification. Fixes CLI-1906 --- apps/cli/src/shared/cli/run.e2e.test.ts | 24 +++++++ .../src/shared/cli/run.integration.test.ts | 66 +++++++++++++++++++ apps/cli/src/shared/cli/run.ts | 45 ++++++++----- apps/cli/src/shared/cli/run.unit.test.ts | 45 ++++++++++++- 4 files changed, 164 insertions(+), 16 deletions(-) create mode 100644 apps/cli/src/shared/cli/run.e2e.test.ts create mode 100644 apps/cli/src/shared/cli/run.integration.test.ts diff --git a/apps/cli/src/shared/cli/run.e2e.test.ts b/apps/cli/src/shared/cli/run.e2e.test.ts new file mode 100644 index 0000000000..1f399eb37e --- /dev/null +++ b/apps/cli/src/shared/cli/run.e2e.test.ts @@ -0,0 +1,24 @@ +import { describe, expect, test } from "vitest"; +import { runSupabase } from "../../../tests/helpers/cli.ts"; + +/** + * CLI-1906: the real bug here is the actual OS process exit code — + * `ProcessControl.exit` calls real `process.exit(code)`, so only a genuine + * subprocess run proves the shipped binary's exit code changed. Everything + * else about this fix (`exitCodeForFailure`'s classification) is covered by + * `run.unit.test.ts` and `run.integration.test.ts`; this is the one minimal + * case that observes the real subprocess boundary. + */ +describe("legacy CLI process exit codes (CLI-1906)", () => { + test("bare `branches` (no subcommand, no --help) exits 0", async () => { + const { exitCode } = await runSupabase(["branches"], { entrypoint: "legacy" }); + expect(exitCode).toBe(0); + }); + + test("a genuine parse error still exits 1", async () => { + const { exitCode } = await runSupabase(["branches", "--this-flag-does-not-exist"], { + entrypoint: "legacy", + }); + expect(exitCode).toBe(1); + }); +}); diff --git a/apps/cli/src/shared/cli/run.integration.test.ts b/apps/cli/src/shared/cli/run.integration.test.ts new file mode 100644 index 0000000000..9ff4f7fd4e --- /dev/null +++ b/apps/cli/src/shared/cli/run.integration.test.ts @@ -0,0 +1,66 @@ +import { describe, expect, test } from "@effect/vitest"; +import { BunServices } from "@effect/platform-bun"; +import { Effect, Exit, Layer } from "effect"; +import { CliOutput, Command } from "effect/unstable/cli"; +import { legacyBranchesCommand } from "../../legacy/commands/branches/branches.command.ts"; +import { textCliOutputFormatter } from "../output/text-formatter.ts"; +import { CliArgs } from "./cli-args.service.ts"; +import { exitCodeForFailure } from "./run.ts"; + +/** + * CLI-1906: `supabase branches` (a legacy "group" command — subcommands, no + * runnable handler of its own) used to exit 1 when invoked bare, even though + * the printed help was identical to `supabase branches --help`, which already + * exited 0. These tests run the real `legacyBranchesCommand` definition + * through `Command.runWith` (same technique as `version.integration.test.ts`) + * so the `ShowHelp` cause shape is the one the real CLI actually produces, not + * a hand-rolled stand-in. `legacyBranchesCommand` is exercised directly + * (rather than nested under `legacyRoot`) because `legacyRoot`'s + * `Command.provide` (see `Command.ts`'s `provide`/`withSubcommands`) wraps its + * *entire* handle — including the bare/`--help`/parse-error paths exercised + * here — in the production output/proxy layer graph (`Layer.unwrap` reading + * every global flag, resolving the Go proxy binary, etc). `Effect.provide` + * still *builds* that layer graph before running the wrapped handle even on + * these runs; it just never gets *consumed*, because the `ShowHelp` failure + * fires before any leaf subcommand handler body executes. Exercising + * `legacyBranchesCommand` directly avoids needing to provide or mock that + * unused graph for a test that only cares about the `ShowHelp` cause shape. + */ +describe("legacy group command exit codes (CLI-1906)", () => { + const layerFor = (args: ReadonlyArray) => + Layer.mergeAll( + CliOutput.layer(textCliOutputFormatter()), + Layer.succeed(CliArgs, { args }), + BunServices.layer, + ); + + const runBranches = (args: ReadonlyArray) => + Effect.runPromiseExit( + Command.runWith(legacyBranchesCommand, { version: "0.0.0-test" })(args).pipe( + Effect.provide(layerFor(args)), + ), + ); + + test("bare `branches` (no subcommand, no --help) fails with a clean ShowHelp that maps to exit 0", async () => { + const exit = await runBranches([]); + expect(Exit.isFailure(exit)).toBe(true); + if (!Exit.isFailure(exit)) return; + + expect(exitCodeForFailure(exit.cause)).toBe(0); + }); + + test("`branches --help` succeeds outright and exits 0", async () => { + const exit = await runBranches(["--help"]); + // The `--help` global flag is handled as a successful `GlobalFlag.Action`, so this + // never even reaches the ShowHelp-as-failure path bare `branches` goes through above. + expect(Exit.isSuccess(exit)).toBe(true); + }); + + test("`branches` with an unrecognized flag is a genuine parse error that still exits 1", async () => { + const exit = await runBranches(["--this-flag-does-not-exist"]); + expect(Exit.isFailure(exit)).toBe(true); + if (!Exit.isFailure(exit)) return; + + expect(exitCodeForFailure(exit.cause)).toBe(1); + }); +}); diff --git a/apps/cli/src/shared/cli/run.ts b/apps/cli/src/shared/cli/run.ts index 099c9a2ba8..48ab86fa05 100644 --- a/apps/cli/src/shared/cli/run.ts +++ b/apps/cli/src/shared/cli/run.ts @@ -1,7 +1,7 @@ import { BunServices } from "@effect/platform-bun"; import { ProjectConfigStore } from "@supabase/config"; import { unixHttpClientLayer } from "@supabase/stack"; -import { Cause, Effect, Exit, Fiber, Layer, Stdio } from "effect"; +import { Cause, Effect, Exit, Fiber, Layer, Runtime, Stdio } from "effect"; import { CliOutput, Command } from "effect/unstable/cli"; import { CLI_VERSION } from "./version.ts"; import { Credentials } from "../../next/auth/credentials.service.ts"; @@ -93,17 +93,26 @@ function formatterLayerFor( : CliOutput.layer(textCliOutputFormatter(context)); } -function isErrorRecord(error: unknown): error is Record { - return typeof error === "object" && error !== null; -} - -function isExplicitHelpCause(cause: Cause.Cause): boolean { - const error = Cause.findErrorOption(cause); - if (error._tag !== "Some" || !isErrorRecord(error.value)) return false; - if (error.value["_tag"] !== "ShowHelp") return false; - - const errors = error.value["errors"]; - return !Array.isArray(errors) || errors.length === 0; +/** + * Process exit code for a failed CLI run, matching Go cobra's exit-code + * mapping. Delegates to Effect's own `Runtime` exit-code protocol (the same + * one `Runtime.defaultTeardown` uses) rather than hand-rolling `ShowHelp` + * classification: `CliError.ShowHelp` declares + * `[Runtime.errorExitCode] = this.errors.length ? 1 : 0`, so a bare group + * command's default handler failing with `ShowHelp({ errors: [] })` (no + * subcommand given, e.g. `supabase branches`) reads as exit `0` here — matching + * Go cobra's non-`Runnable()` handling, which internally returns + * `flag.ErrHelp` and `ExecuteC()` maps that to "print help, return nil error". + * A `ShowHelp` with a non-empty `errors` array (a genuine parse/validation + * failure) reads as exit `1`, and any other failure (including a `Cause.die` + * defect with no typed `ShowHelp` marker at all) falls back to + * `Runtime.getErrorExitCode`'s default of `1`. An explicit `--help` invocation + * never reaches this function — it's handled earlier as a successful + * `GlobalFlag.Action` and exits 0 via the success path. + */ +export function exitCodeForFailure(cause: Cause.Cause): number { + if (Cause.hasInterruptsOnly(cause)) return 130; + return Runtime.getErrorExitCode(Cause.squash(cause)); } function projectContextLayerFor(runtimeLayer: Layer.Layer) { @@ -232,11 +241,17 @@ export async function runCli(rootCommand: Command.Command.Any, options: RunCliOp const output = yield* Output; const exit = yield* program.pipe(Effect.exit); if (Exit.isFailure(exit)) { - const interrupted = Cause.hasInterruptsOnly(exit.cause); - if (!interrupted && !isExplicitHelpCause(exit.cause)) { + const exitCode = exitCodeForFailure(exit.cause); + // Skip reporting for an interrupted run (130 — a signal, not a + // reportable error) and for a clean `ShowHelp` failure (0). Literal + // `--help` never reaches this branch — it's handled as a successful + // `GlobalFlag.Action` and exits 0 via the success path below. See + // `exitCodeForFailure` for why a "clean" ShowHelp failure (e.g. a bare + // group command with no subcommand) also maps to exit 0. + if (exitCode !== 0 && exitCode !== 130) { yield* output.fail(normalizeCause(exit.cause)); } - return yield* processControl.exit(interrupted ? 130 : 1); + return yield* processControl.exit(exitCode); } const exitCode = yield* processControl.getExitCode; return yield* processControl.exit(exitCode ?? 0); diff --git a/apps/cli/src/shared/cli/run.unit.test.ts b/apps/cli/src/shared/cli/run.unit.test.ts index bd2d80804e..f87d160533 100644 --- a/apps/cli/src/shared/cli/run.unit.test.ts +++ b/apps/cli/src/shared/cli/run.unit.test.ts @@ -1,6 +1,8 @@ +import { Cause } from "effect"; +import { CliError } from "effect/unstable/cli"; import { describe, expect, it } from "vitest"; -import { extractCommandPath, shouldUseGlobalSignalInterrupt } from "./run.ts"; +import { exitCodeForFailure, extractCommandPath, shouldUseGlobalSignalInterrupt } from "./run.ts"; describe("extractCommandPath", () => { it("returns positional command-path tokens", () => { @@ -45,3 +47,44 @@ describe("shouldUseGlobalSignalInterrupt", () => { expect(shouldUseGlobalSignalInterrupt([])).toBe(true); }); }); + +describe("exitCodeForFailure", () => { + // CLI-1906: a group command's default handler (e.g. bare `supabase branches`, which + // has subcommands but no runnable handler of its own) fails with exactly this shape: + // ShowHelp with an empty `errors` array. `CliError.ShowHelp` declares + // `[Runtime.errorExitCode] = this.errors.length ? 1 : 0`, so this reads as exit 0 — + // matching Go cobra's `flag.ErrHelp` handling for non-Runnable commands. Before + // CLI-1906, this case always returned 1. + it("exits 0 for a clean ShowHelp failure (bare group command)", () => { + const cause = Cause.fail(new CliError.ShowHelp({ commandPath: ["branches"], errors: [] })); + expect(exitCodeForFailure(cause)).toBe(0); + }); + + it("exits 1 for a ShowHelp cause carrying a genuine validation error", () => { + const cause = Cause.fail( + new CliError.ShowHelp({ + commandPath: ["branches"], + errors: [new CliError.UnrecognizedOption({ option: "--bogus", suggestions: [] })], + }), + ); + expect(exitCodeForFailure(cause)).toBe(1); + }); + + it("exits 1 for a non-ShowHelp failure", () => { + const cause = Cause.fail(new Error("boom")); + expect(exitCodeForFailure(cause)).toBe(1); + }); + + // `Cause.squash` on a `Die` cause returns the raw defect (a plain `Error`, with no + // `Runtime.errorExitCode` marker at all). This must still fall back to the default + // failure exit code (1), not silently pass through as a "clean" exit — this is the real + // unexpected-crash path through `runCli` that must keep exiting 1. + it("exits 1 for a defect with no typed failure", () => { + const cause = Cause.die(new Error("unexpected crash")); + expect(exitCodeForFailure(cause)).toBe(1); + }); + + it("exits 130 when interrupted, regardless of any other failure reason", () => { + expect(exitCodeForFailure(Cause.interrupt())).toBe(130); + }); +});