diff --git a/apps/cli/src/legacy/cli/legacy-boolean-flag-defaults.unit.test.ts b/apps/cli/src/legacy/cli/legacy-boolean-flag-defaults.unit.test.ts new file mode 100644 index 0000000000..b4a032b883 --- /dev/null +++ b/apps/cli/src/legacy/cli/legacy-boolean-flag-defaults.unit.test.ts @@ -0,0 +1,71 @@ +import { describe, expect, it } from "vitest"; +import { Primitive, type Command } from "effect/unstable/cli"; +import { + legacyCommandInternals, + legacyFlattenSubcommands, + legacyUserGlobalFlagParams, +} from "../docs/legacy-docs-introspection.ts"; +import { legacyUnwrapParam } from "../shared/legacy-param-introspection.ts"; +import { legacyRoot } from "./root.ts"; + +/** + * `Flag.boolean(name)` builds a bare `Single` param, and a bare `Single` is + * *required* — omitting it fails the whole command with a missing-flag error + * before the handler ever runs. Every boolean flag therefore has to be closed + * off with `Flag.withDefault(false)` or `Flag.optional`. + * + * Nothing else catches this: handler integration tests build their flags record + * directly, so they never touch the parser, and the required-ness is invisible + * to the type checker because a required boolean flag still infers as + * `boolean`. The flag only misbehaves when a real invocation omits it, which is + * precisely the invocation no handler test makes — so the guard walks the + * command tree instead of waiting for a command to be exercised end to end. + */ + +/** + * The published getter for a primitive's kind — `Primitive.getTypeName`, whose + * own doc example pins `Primitive.boolean` to `"boolean"`. Reading + * `primitiveType._tag` instead would couple this guard to effect's runtime + * representation, which this repo forbids in tests as well as in source. + * + * Derived from `Primitive.boolean` rather than written as the literal + * `"boolean"`: were that name to change upstream, a hardcoded literal would + * match nothing and leave the guard silently passing every command, which is + * the one failure mode a regression test must not have. + */ +const BOOLEAN_TYPE_NAME = Primitive.getTypeName(Primitive.boolean); + +function booleanFlagsRequiringAValue(command: Command.Command.Any): ReadonlyArray { + const internals = legacyCommandInternals(command); + // All three parameter sets a command can be parsed with, not just its own: + // `Command.withSharedFlags` puts inherited flags on `contextConfig`, and the + // root's persistent flags arrive as `globalFlags`. A bare boolean introduced + // through either would break every command that inherits it while a guard + // reading only `config.flags` stayed green. + const params = [ + ...internals.config.flags, + ...internals.contextConfig.flags, + ...legacyUserGlobalFlagParams(command), + ]; + + // Throws rather than skipping if effect's internal shape moves, so this + // cannot quietly degrade into a test that inspects nothing. + const own = params.flatMap((flag) => { + const unwrapped = legacyUnwrapParam(flag); + if (unwrapped === undefined) { + throw new Error(`Unrecognizable flag param on "${command.name}".`); + } + const { single, isOptional } = unwrapped; + return Primitive.getTypeName(single.primitiveType) === BOOLEAN_TYPE_NAME && !isOptional + ? [`${command.name} --${single.name}`] + : []; + }); + + return [...own, ...legacyFlattenSubcommands(command).flatMap(booleanFlagsRequiringAValue)]; +} + +describe("legacy boolean flag wiring", () => { + it("gives every boolean flag a default, so omitting it is not a parse error", () => { + expect(booleanFlagsRequiringAValue(legacyRoot)).toEqual([]); + }); +}); diff --git a/apps/cli/src/legacy/commands/experimental/workers/delete/SIDE_EFFECTS.md b/apps/cli/src/legacy/commands/experimental/workers/delete/SIDE_EFFECTS.md index e9ebcabe0b..4b425f2fdd 100644 --- a/apps/cli/src/legacy/commands/experimental/workers/delete/SIDE_EFFECTS.md +++ b/apps/cli/src/legacy/commands/experimental/workers/delete/SIDE_EFFECTS.md @@ -77,11 +77,17 @@ wrapper emits for every command. ## Output Formats -| Mode | stdout | stderr | -| ----------------------------- | --------------------------------------------------------------------------------------------- | --------------------------------------------- | -| text (default) | the confirmation prompt, then what was deleted and kept | that nothing local was kept, when nothing was | -| `--output-format json` | one structured result carrying `worker_name`, `project_ref`, `kept_*` | as above | -| `--output-format stream-json` | the same result as a single terminal event | as above | -| `-o json` / `yaml` / `toml` | the same payload in that encoding, and nothing else | as above | -| `-o pretty` / `table` / `csv` | the text rendering — these fall through rather than encoding | as above | -| `-o env` | refused **before** the DELETE; discovering it at emit time deleted the worker and then failed | the error | +| Mode | stdout | stderr | +| ----------------------------- | --------------------------------------------------------------------------------------------- | ------------------------------------------------------------------- | +| text (default) | the confirmation prompt, then what was deleted and kept | that nothing local was kept when nothing was, and the redeploy hint | +| `--output-format json` | one structured result carrying `worker_name`, `project_ref`, `kept_*` | neither — both are text-only | +| `--output-format stream-json` | the same result as a single terminal event | neither — both are text-only | +| `-o json` / `yaml` / `toml` | the same payload in that encoding, and nothing else | neither — both are text-only | +| `-o pretty` / `table` / `csv` | the text rendering — these fall through rather than encoding | both, as in text | +| `-o env` | refused **before** the DELETE; discovering it at emit time deleted the worker and then failed | the error | + +A structured emission is the end of the run: the handler returns at +`legacyEmitWorkersMachineOutput` or at `output.success`, so nothing in the text +branch below it — the kept-nothing notice and the redeploy trailer — is reached. +`-o pretty`, `table` and `csv` are the exception, since they encode nothing and +fall through to that same text branch. diff --git a/apps/cli/src/legacy/commands/experimental/workers/delete/delete.handler.ts b/apps/cli/src/legacy/commands/experimental/workers/delete/delete.handler.ts index e5fc407967..f607dcc41f 100644 --- a/apps/cli/src/legacy/commands/experimental/workers/delete/delete.handler.ts +++ b/apps/cli/src/legacy/commands/experimental/workers/delete/delete.handler.ts @@ -1,5 +1,6 @@ import { Effect, Option } from "effect"; import { Output } from "../../../../../shared/output/output.service.ts"; +import { emitSuccessTrailer } from "../../../../../shared/cli/success-trailer.ts"; import { legacyAqua } from "../../../../shared/legacy-colors.ts"; import { legacyRenderWorkerDetails } from "../workers.format.ts"; import { @@ -232,8 +233,9 @@ export const legacyWorkersDelete = Effect.fn("legacy.experimental.workers.delete // alone is not enough to redeploy from, so `push` would fail on the very // command this line recommends. if (keptSource !== undefined) { - yield* output.raw( - `Redeploy it with supabase experimental workers push ${name}${refSuffix}.\n`, + // Trailer, like every other "what to run next" line in this shell. + yield* emitSuccessTrailer( + `Redeploy it with ${legacyAqua(`supabase experimental workers push ${name}${refSuffix}`)}.\n`, ); } } else { diff --git a/apps/cli/src/legacy/commands/experimental/workers/delete/delete.integration.test.ts b/apps/cli/src/legacy/commands/experimental/workers/delete/delete.integration.test.ts index 1f6e9cf899..3d0c59bc34 100644 --- a/apps/cli/src/legacy/commands/experimental/workers/delete/delete.integration.test.ts +++ b/apps/cli/src/legacy/commands/experimental/workers/delete/delete.integration.test.ts @@ -72,13 +72,14 @@ describe("legacy workers delete", () => { // Nothing local is touched — that is what makes `push` a one-command undo. expect(existsSync(join(repo.dir, "supabase", "workers", "api", "index.js"))).toBe(true); expect(readFileSync(join(repo.dir, "supabase", "config.toml"), "utf8")).toBe(CONFIG); - expect(out.stdoutText).toContain("supabase experimental workers push api"); + // The redeploy hint is a success trailer, which lands on stderr. + expect(out.stderrText).toContain("supabase experimental workers push api"); }).pipe(Effect.provide(layer), Effect.ensuring(Effect.sync(repo.cleanup))); }); - // The refusal used to live at emit time, which on this command is *after* the - // DELETE: `--yes -o env` removed the worker and then exited non-zero with no - // payload, which a script reads as "the delete failed" and may retry. + // The refusal has to precede the DELETE. At emit time `--yes -o env` would + // remove the worker and then exit non-zero with no payload, which a script + // reads as "the delete failed" and may retry. // Deletion never touches local files, so a malformed local config has no // business standing between the user and a worker they named explicitly. it.live("deletes a remote worker despite an unparseable local config", () => { @@ -327,8 +328,8 @@ describe("legacy workers delete", () => { }).pipe(Effect.provide(layer), Effect.ensuring(Effect.sync(repo.cleanup))); }); - // `interactive` follows stdout, so a plain `>` redirect reaches this branch - // even from a live terminal — the case that used to delete without asking. + // `interactive` follows stdout, so a plain `>` redirect reaches this branch even + // from a live terminal — the case where deleting without asking would be worst. it.live("refuses when stdout is redirected and no --yes was given", () => { const repo = project(); const { layer, http } = setupLegacyWorkers({ @@ -541,6 +542,63 @@ describe("legacy workers delete", () => { }).pipe(Effect.provide(layer), Effect.ensuring(Effect.sync(repo.cleanup))); }); + it.live("pluralizes the live instance count in the confirmation", () => { + const repo = project(); + const { layer, out } = setupLegacyWorkers({ + workdir: repo.dir, + promptTextResponses: ["api"], + routes: { + ...routes, + [getRoute]: { + status: 200, + body: { + data: workerResource({ + name: "api", + instances: 3, + instanceCounts: { declared: 3, live: 2, ready: 2, stale: 0 }, + }), + }, + }, + }, + }); + + return Effect.gen(function* () { + yield* legacyWorkersDelete({ name: "api", projectRef: Option.none() }); + + expect(out.stdoutText).toContain("2 running instances will be terminated"); + }).pipe(Effect.provide(layer), Effect.ensuring(Effect.sync(repo.cleanup))); + }); + + // Scaled to zero: there is a tally, and it says nothing is running. Warning + // about terminated instances there would invent a consequence. + it.live("promises no terminations when nothing is running", () => { + const repo = project(); + const { layer, out } = setupLegacyWorkers({ + workdir: repo.dir, + promptTextResponses: ["api"], + routes: { + ...routes, + [getRoute]: { + status: 200, + body: { + data: workerResource({ + name: "api", + instances: 2, + instanceCounts: { declared: 2, live: 0, ready: 0, stale: 0 }, + }), + }, + }, + }, + }); + + return Effect.gen(function* () { + yield* legacyWorkersDelete({ name: "api", projectRef: Option.none() }); + + expect(out.stdoutText).toContain("permanently deletes"); + expect(out.stdoutText).not.toContain("will be terminated"); + }).pipe(Effect.provide(layer), Effect.ensuring(Effect.sync(repo.cleanup))); + }); + // An orphan — deployed from another checkout — has no local entry and no local // directory, so there is nothing that was "kept" and `push` has no source to // redeploy from. @@ -604,7 +662,7 @@ describe("legacy workers delete", () => { }).pipe(Effect.provide(layer), Effect.ensuring(Effect.sync(repo.cleanup))); }); - // Deletion never reads the local source, so a `source` that no longer resolves + // Deletion never reads the local source, so a `source` that does not resolve // inside the project must not block removing the remote worker. it.live("deletes the remote worker even when the configured source is unusable", () => { const repo = project('project_id = "demo"\n\n[workers.api]\nsource = "../../elsewhere"\n'); diff --git a/apps/cli/src/legacy/commands/experimental/workers/list/SIDE_EFFECTS.md b/apps/cli/src/legacy/commands/experimental/workers/list/SIDE_EFFECTS.md index 322844cd65..b7ae13668c 100644 --- a/apps/cli/src/legacy/commands/experimental/workers/list/SIDE_EFFECTS.md +++ b/apps/cli/src/legacy/commands/experimental/workers/list/SIDE_EFFECTS.md @@ -67,3 +67,7 @@ wrapper emits for every command. | `-o json` / `yaml` / `toml` | the same payload in that encoding, and nothing else | as above | | `-o pretty` / `table` / `csv` | the text rendering — these fall through rather than encoding | as above | | `-o env` | refused before any request; the payload carries a `workers` array a flat `KEY=value` list cannot express | the error | + +The text table omits each worker's URL — it is the same host and prefix on +every row, and carrying it made the table 137 columns wide. Every machine +format still carries `url` per worker, and `workers status` renders it. diff --git a/apps/cli/src/legacy/commands/experimental/workers/list/list.handler.ts b/apps/cli/src/legacy/commands/experimental/workers/list/list.handler.ts index c1c5434b58..669972f199 100644 --- a/apps/cli/src/legacy/commands/experimental/workers/list/list.handler.ts +++ b/apps/cli/src/legacy/commands/experimental/workers/list/list.handler.ts @@ -1,5 +1,7 @@ import { Effect } from "effect"; import { Output } from "../../../../../shared/output/output.service.ts"; +import { legacyAqua, legacyYellow } from "../../../../shared/legacy-colors.ts"; +import { displayPath } from "../../../../../shared/workers/worker-paths.ts"; import { renderGlamourTable } from "../../../../output/legacy-glamour-table.ts"; import { legacyEmitWorkersMachineOutput, legacyRejectWorkersEnvOutput } from "../workers.output.ts"; import { LegacyPlatformApi } from "../../../../auth/legacy-platform-api.service.ts"; @@ -28,7 +30,15 @@ import type { LegacyWorkersListFlags } from "./list.command.ts"; * count from the spec. `status` is where the live tally lives. */ -const HEADERS = ["NAME", "RUNTIME", "SIZE", "STATE", "INSTANCES", "URL"] as const; +/** + * No URL column. Every worker's URL is the same 40-odd characters of host and + * prefix with the name on the end, which pushed the table past 130 columns to + * carry one derivable field — `renderGlamourTable` sizes each column to its + * widest cell and never wraps. `workers status` renders it, vertically, for the + * same reason (see `workers.format.ts`), and every machine format still carries + * `url` per worker. + */ +const HEADERS = ["NAME", "RUNTIME", "SIZE", "STATE", "INSTANCES"] as const; interface WorkerRow { readonly name: string; @@ -68,6 +78,14 @@ function runtimeLabel(row: WorkerRow): string { return runtimeLabelFor(row) ?? "-"; } +/** + * `api is` / `api, box are` — the subject of both advisories below, which only + * ever differ in the verb. + */ +function nameList(names: ReadonlyArray): string { + return `${names.join(", ")} ${names.length === 1 ? "is" : "are"}`; +} + function toCells(row: WorkerRow): ReadonlyArray { return [ row.name, @@ -75,7 +93,6 @@ function toCells(row: WorkerRow): ReadonlyArray { row.deployed === undefined ? "-" : formatApiSize(row.deployed.spec.size), stateLabel(row), row.deployed === undefined ? "-" : String(row.deployed.spec.instances), - row.url ?? "-", ]; } @@ -165,7 +182,7 @@ export const legacyWorkersList = Effect.fn("legacy.experimental.workers.list")(f if (rows.length === 0) { yield* output.raw( - "No workers found. Scaffold one with supabase experimental workers new .\n", + `No workers found. Scaffold one with ${legacyAqua("supabase experimental workers new ", process.stdout)}.\n`, ); return; } @@ -178,14 +195,20 @@ export const legacyWorkersList = Effect.fn("legacy.experimental.workers.list")(f // the source directory *before* inferring a runtime and fails with // `WorkerSourceMissingError`, so telling that user about runtime guessing // points them at the wrong prerequisite. + // + // Both are written the way this shell writes every other heads-up that is + // not a failure: a yellow `WARNING:` prefix, then the consequence on its own + // line (`start`'s Docker-on-Windows notice is the same two-line shape). A + // single long sentence re-flows differently at every terminal width, right + // under a table that lines its columns up. const unconfigured = rows .filter((row) => row.deployed !== undefined && !row.configured && row.local) .map((row) => row.name); if (unconfigured.length > 0) { + const configDisplay = displayPath(project.projectRoot, project.configPath); yield* output.raw( - `${unconfigured.join(", ")} ${ - unconfigured.length === 1 ? "is" : "are" - } deployed but absent from supabase/config.toml: pushing from here would have to guess the runtime.\n`, + `${legacyYellow("WARNING:")} ${nameList(unconfigured)} deployed but not in ${configDisplay}.\n` + + `Pushing from here would have to guess the runtime.\n`, "stderr", ); } @@ -195,9 +218,8 @@ export const legacyWorkersList = Effect.fn("legacy.experimental.workers.list")(f .map((row) => row.name); if (remoteOnly.length > 0) { yield* output.raw( - `${remoteOnly.join(", ")} ${ - remoteOnly.length === 1 ? "is" : "are" - } deployed but ${remoteOnly.length === 1 ? "has" : "have"} no source in this project: scaffold or restore it before pushing from here.\n`, + `${legacyYellow("WARNING:")} ${nameList(remoteOnly)} deployed with no source in this project.\n` + + `Scaffold or restore before pushing from here.\n`, "stderr", ); } diff --git a/apps/cli/src/legacy/commands/experimental/workers/list/list.integration.test.ts b/apps/cli/src/legacy/commands/experimental/workers/list/list.integration.test.ts index eb50d0da97..0bfb506676 100644 --- a/apps/cli/src/legacy/commands/experimental/workers/list/list.integration.test.ts +++ b/apps/cli/src/legacy/commands/experimental/workers/list/list.integration.test.ts @@ -70,7 +70,9 @@ describe("legacy workers list", () => { expect(rows).toHaveLength(3); // Sorted by name, so `api`, `box`, then the scaffolded-but-undeployed `old`. expect(rows[0]).toContain("2gb (1 vCPU)"); - expect(rows[0]).toContain(`https://${WORKERS_PROJECT_REF}.supabase.co/workers/v1/api`); + // The URL is deliberately not a column: one derivable field pushed the + // table past 130 columns. The machine payload still carries it. + expect(stdout).not.toContain("https://"); expect(rows[1]).toContain("sandbox"); expect(rows[2]).toContain("not deployed"); }).pipe(Effect.provide(layer), Effect.ensuring(Effect.sync(repo.cleanup))); @@ -122,6 +124,63 @@ describe("legacy workers list", () => { }).pipe(Effect.provide(layer), Effect.ensuring(Effect.sync(repo.cleanup))); }); + // Two of them, so the advisory has to read as a list rather than as one name + // with a stray verb. + it.live("calls out every deployed worker config.toml does not know about", () => { + const created = makeWorkersProject({ + "supabase/config.toml": `project_id = "demo"\n`, + "supabase/workers/stray/index.js": "export default {};\n", + "supabase/workers/spare/index.js": "export default {};\n", + }); + const repo = { + dir: created.dir, + cleanup: () => rmSync(created.dir, { recursive: true, force: true }), + }; + const { layer, out } = setupLegacyWorkers({ + workdir: repo.dir, + routes: { + [listRoute]: { + status: 200, + body: { + data: [ + workerResource({ name: "stray", runtime: "node" }), + workerResource({ name: "spare", runtime: "node" }), + ], + }, + }, + }, + }); + + return Effect.gen(function* () { + yield* legacyWorkersList({ projectRef: Option.none() }); + + expect(out.stderrText).toContain("spare, stray are deployed but not in"); + expect(out.stderrText).toContain("guess the runtime"); + }).pipe(Effect.provide(layer), Effect.ensuring(Effect.sync(repo.cleanup))); + }); + + // Deletion is asynchronous, so a worker can be listed while it is being torn + // down. Reporting its build state would show `active` for something on its + // way out. + it.live("shows a worker being torn down as deleting", () => { + const repo = project(`project_id = "demo"\n\n[workers.api]\nruntime = "node"\n`); + const { layer, out } = setupLegacyWorkers({ + workdir: repo.dir, + routes: { + [listRoute]: { + status: 200, + body: { data: [workerResource({ name: "api", runtime: "node", deleting: true })] }, + }, + }, + }); + + return Effect.gen(function* () { + yield* legacyWorkersList({ projectRef: Option.none() }); + + expect(out.stdoutText).toContain("deleting"); + }).pipe(Effect.provide(layer), Effect.ensuring(Effect.sync(repo.cleanup))); + }); + // Nothing local at all: `deployOneWorker` checks the source directory before // it ever infers a runtime, so "would have to guess the runtime" named the // wrong prerequisite for this one. @@ -386,10 +445,33 @@ describe("legacy workers list", () => { }).pipe(Effect.provide(layer), Effect.ensuring(Effect.sync(repo.cleanup))); }); + it.live("encodes YAML when -o yaml asks for it", () => { + const repo = project(); + const { layer, out } = setupLegacyWorkers({ + workdir: repo.dir, + goOutput: "yaml", + routes: { + [listRoute]: { + status: 200, + body: { data: [workerResource({ name: "api", runtime: "node" })] }, + }, + }, + }); + + return Effect.gen(function* () { + yield* legacyWorkersList({ projectRef: Option.none() }); + + expect(out.stdoutText).toContain("project_ref:"); + expect(out.stdoutText).toContain("name: api"); + // The table would have gone to stdout too, and broken the document. + expect(out.stdoutText).not.toContain("NAME"); + }).pipe(Effect.provide(layer), Effect.ensuring(Effect.sync(repo.cleanup))); + }); + // `pretty` is the human default; `table` and `csv` are accepted by the global // flag for `db query`'s benefit, and every resource command is meant to ignore - // them and render text. All three used to fall through to the TOML encoder, - // which is the trap the payload allowlist closes. + // them and render text. Falling through to the TOML encoder is the trap the + // payload allowlist closes. it.live.each(["pretty", "table", "csv"] as const)( "renders text rather than TOML for -o %s", (goOutput) => { diff --git a/apps/cli/src/legacy/commands/experimental/workers/new/SIDE_EFFECTS.md b/apps/cli/src/legacy/commands/experimental/workers/new/SIDE_EFFECTS.md index d614531471..d331367dbd 100644 --- a/apps/cli/src/legacy/commands/experimental/workers/new/SIDE_EFFECTS.md +++ b/apps/cli/src/legacy/commands/experimental/workers/new/SIDE_EFFECTS.md @@ -1,4 +1,4 @@ -# `supabase experimental workers new ` +# `supabase experimental workers new [name]` > **Local-disk only.** Nothing is deployed and no Management API route is > called; `workers push` is what talks to the platform. @@ -35,6 +35,16 @@ same resolver `start`/`stop`/`status` use) and never climbs to an ancestor. A therefore records the worker in that directory's own `config.toml` — created if absent — rather than in the ancestor project's. +The name is prompted for when the command line does not carry one, and the +prompt refuses a name that is not a DNS label or that `config.toml` already +records — so nothing is asked, and nothing written, for a name the command was +going to refuse. With `-o json|yaml|toml|env`, a redirected stdout, or a stdin +that is not a terminal, there is nowhere to ask, and the command fails instead +of defaulting: unlike the runtime and size, the name has no default to fall back +on. Every prompt is gated on both streams, so +`printf 'api\n' | supabase experimental workers new` takes that failure path +rather than reading the worker name off the pipe. + Writes to `config.toml` are append-only. A worker already recorded under `[workers.]` is refused outright — before the runtime and size prompts, and before anything reaches disk — because editing an entry the user owns is @@ -57,14 +67,15 @@ root. ## Exit Codes -| Code | Condition | -| ---- | ----------------------------------------------------------------------------------- | -| `0` | success | -| `1` | invalid worker name — the name must be a DNS label | -| `1` | bad `--source`: outside the project, or a path the CLI owns | -| `1` | destination exists and is not empty | -| `1` | the worker is already recorded in `config.toml`, in any form | -| `1` | the rendered `config.toml` would not parse, or `[workers]` is a sealed inline table | +| Code | Condition | +| ---- | -------------------------------------------------------------------------------------------------- | +| `0` | success | +| `1` | invalid worker name — the name must be a DNS label | +| `1` | no name given, and nowhere to ask for one — stdin or stdout is not a terminal, or `-o` is in force | +| `1` | bad `--source`: outside the project, or a path the CLI owns | +| `1` | destination exists and is not empty | +| `1` | the worker is already recorded in `config.toml`, in any form | +| `1` | the rendered `config.toml` would not parse, or `[workers]` is a sealed inline table | ## Environment Variables @@ -83,7 +94,9 @@ root. No custom events — only the `cli_command_executed` that the instrumentation wrapper emits for every command. -Nothing is emitted for a failure the parser catches, such as a missing worker -name or a `--runtime`/`--size` value outside the choice list. The wrapper is -installed by `Command.withHandler`, so a command that never reaches its handler -never reaches the instrumentation either — and `telemetry.json` is not written. +Nothing is emitted for a failure the parser catches, such as a +`--runtime`/`--size` value outside the choice list. The wrapper is installed by +`Command.withHandler`, so a command that never reaches its handler never reaches +the instrumentation either — and `telemetry.json` is not written. A missing name +is _not_ one of those: the argument is optional, so a bare `workers new` reaches +the handler, which asks for the name or fails for want of anywhere to ask. diff --git a/apps/cli/src/legacy/commands/experimental/workers/new/new.command.ts b/apps/cli/src/legacy/commands/experimental/workers/new/new.command.ts index 66bfd28d45..ce93798dd9 100644 --- a/apps/cli/src/legacy/commands/experimental/workers/new/new.command.ts +++ b/apps/cli/src/legacy/commands/experimental/workers/new/new.command.ts @@ -12,7 +12,10 @@ import { legacyWorkersNew } from "./new.handler.ts"; const config = { name: Argument.string("name").pipe( - Argument.withDescription("Worker name. Doubles as its directory, and its hostname."), + Argument.withDescription( + "Worker name. Doubles as its directory, and its hostname. Prompted when omitted.", + ), + Argument.optional, ), runtime: Flag.choice("runtime", WORKER_RUNTIMES).pipe( Flag.withDescription( @@ -51,6 +54,10 @@ export const legacyWorkersNewCommand = Command.make("new", config).pipe( ), Command.withShortDescription("Scaffold a worker locally"), Command.withExamples([ + { + command: "supabase experimental workers new", + description: "Prompt for the name, then for runtime and size", + }, { command: "supabase experimental workers new api", description: "Scaffold supabase/workers/api, prompting for runtime and size", diff --git a/apps/cli/src/legacy/commands/experimental/workers/new/new.handler.ts b/apps/cli/src/legacy/commands/experimental/workers/new/new.handler.ts index 018f1caaf1..79b195dc54 100644 --- a/apps/cli/src/legacy/commands/experimental/workers/new/new.handler.ts +++ b/apps/cli/src/legacy/commands/experimental/workers/new/new.handler.ts @@ -1,6 +1,8 @@ import { join, relative, sep } from "node:path"; import { Effect, FileSystem, Option } from "effect"; import { Output } from "../../../../../shared/output/output.service.ts"; +import { emitSuccessTrailer } from "../../../../../shared/cli/success-trailer.ts"; +import { legacyAqua, legacyBold } from "../../../../shared/legacy-colors.ts"; import { legacyRenderWorkerDetails } from "../workers.format.ts"; import { legacyEmitWorkersMachineOutput, @@ -8,6 +10,7 @@ import { } from "../workers.output.ts"; import { LegacyTelemetryState } from "../../../../telemetry/legacy-telemetry-state.service.ts"; import { RuntimeInfo } from "../../../../../shared/runtime/runtime-info.service.ts"; +import { Tty } from "../../../../../shared/runtime/tty.service.ts"; import { commitWorkerEntry, planWorkerEntry, @@ -33,18 +36,22 @@ import { } from "../../../../../shared/workers/worker-runtimes.ts"; import { WORKER_STACKS } from "../../../../../shared/workers/worker-stacks.ts"; import { - InvalidWorkerNameError, + MissingWorkerNameError, WorkerDirectoryExistsError, } from "../../../../../shared/workers/workers.errors.ts"; -import { legacyLoadWorkersProjectForEntryWrite } from "../workers.shared.ts"; +import { + legacyLoadWorkersProjectForEntryWrite, + legacyValidateWorkerName, + type LegacyWorkersProject, +} from "../workers.shared.ts"; import type { LegacyWorkersNewFlags } from "./new.command.ts"; /** - * `supabase experimental workers new ` — scaffold `supabase/workers//` from the + * `supabase experimental workers new [name]` — scaffold `supabase/workers//` from the * chosen runtime's starter files and record the choice in `config.toml`. * Nothing is deployed; this is entirely local-disk work. * - * The runtime and size are resolved *before* anything is written, so a + * The name, runtime and size are all resolved *before* anything is written, so a * cancelled prompt leaves nothing behind for this worker at all. */ @@ -53,10 +60,73 @@ function defaultFirst(values: ReadonlyArray, defaultValue: T): Array { return [defaultValue, ...values.filter((value) => value !== defaultValue)]; } +/** + * Whether this run has a terminal to ask on. + * + * `-o json|yaml|toml|env` leaves `output.format` as `text`, and the prompts go + * through Clack, which writes its terminal UI to stdout with no stream + * override — so a machine format is as non-interactive as a redirected stdout, + * whichever flag asked for it. + * + * `output.interactive` only tracks *stdout*, so on its own it still let + * `printf 'api\n' | supabase experimental workers new` feed the pipe straight + * into the name prompt instead of taking the documented non-interactive path. A + * prompt is only answerable from a keyboard, so stdin has to be a terminal too + * — the same pair `workers delete` guards its confirmation with. + */ +const canPromptFor = Effect.fnUntraced(function* (machineOutput: boolean) { + const output = yield* Output; + const tty = yield* Tty; + return output.format === "text" && output.interactive && !machineOutput && tty.stdinIsTty; +}); + +/** + * The worker name, asked for when the command line did not carry one. + * + * The name is the one input here that cannot be defaulted — it is the + * directory, the `config.toml` key and the hostname — so a bare + * `supabase experimental workers new` asks rather than failing the parse. The + * prompt validates against everything the command would otherwise refuse a + * moment later, so a mistyped or already-recorded name is corrected in place + * instead of ending the run. + */ +const resolveName = Effect.fnUntraced(function* (options: { + readonly explicit: Option.Option; + /** Whether there is a terminal to ask on — see `canPromptFor`. */ + readonly canPrompt: boolean; + readonly project: LegacyWorkersProject; +}) { + if (Option.isSome(options.explicit)) { + return options.explicit.value; + } + + if (options.canPrompt) { + const output = yield* Output; + return yield* output.promptText("What should this worker be called?", { + validate: (value) => { + const invalid = validateWorkerNameMessage(value); + if (invalid !== undefined) { + return invalid; + } + return options.project.section.workers[value] === undefined + ? undefined + : `"${value}" is already configured in ${options.project.configPath}.`; + }, + }); + } + + return yield* Effect.fail( + new MissingWorkerNameError({ + detail: "Worker name is required in non-interactive mode.", + suggestion: "Pass a worker name, for example `supabase experimental workers new api`.", + }), + ); +}); + const resolveRuntime = Effect.fnUntraced(function* (options: { readonly explicit: Option.Option; - /** `-o json|yaml|toml|env` — stdout belongs to the payload, so do not prompt. */ - readonly machineOutput: boolean; + /** Whether there is a terminal to ask on — see `canPromptFor`. */ + readonly canPrompt: boolean; }) { // `--runtime` is a choice flag, so the parser has already rejected anything // outside the catalog by the time it gets here. @@ -64,8 +134,8 @@ const resolveRuntime = Effect.fnUntraced(function* (options: { return options.explicit.value; } - const output = yield* Output; - if (output.format === "text" && output.interactive && !options.machineOutput) { + if (options.canPrompt) { + const output = yield* Output; const selected = yield* output.promptSelect( "Which runtime should this worker use?", defaultFirst([...WORKER_RUNTIMES], DEFAULT_WORKER_RUNTIME).map((runtime) => ({ @@ -82,15 +152,15 @@ const resolveRuntime = Effect.fnUntraced(function* (options: { const resolveSize = Effect.fnUntraced(function* (options: { readonly explicit: Option.Option; - /** `-o json|yaml|toml|env` — stdout belongs to the payload, so do not prompt. */ - readonly machineOutput: boolean; + /** Whether there is a terminal to ask on — see `canPromptFor`. */ + readonly canPrompt: boolean; }) { if (Option.isSome(options.explicit)) { return options.explicit.value; } - const output = yield* Output; - if (output.format === "text" && output.interactive && !options.machineOutput) { + if (options.canPrompt) { + const output = yield* Output; const selected = yield* output.promptSelect( "Which instance size should this worker use?", defaultFirst([...WORKER_SIZES], DEFAULT_WORKER_SIZE).map((size) => ({ @@ -134,21 +204,19 @@ export const legacyWorkersNew = Effect.fn("legacy.experimental.workers.new")(fun yield* Effect.gen(function* () { const project = yield* legacyLoadWorkersProjectForEntryWrite(); - const name = flags.name; - const invalid = validateWorkerNameMessage(name); - if (invalid !== undefined) { - return yield* Effect.fail( - new InvalidWorkerNameError({ - detail: `"${name}" is not a valid worker name. ${invalid}`, - suggestion: "Worker names become hostnames, so they must be DNS labels.", - }), - ); - } + // Decided once, before the first prompt rather than beside the last, since + // the name is now asked for too — every prompt below shares the answer. + const machineOutput = yield* legacyWorkersMachineOutputRequested(); + const canPrompt = yield* canPromptFor(machineOutput); + + const name = yield* resolveName({ explicit: flags.name, canPrompt, project }); + yield* legacyValidateWorkerName(name); // Refused before anything is asked or written. `new` creates a worker; // changing one that already exists is a `config.toml` edit, and the file is // the user's. Checking here rather than only in `planWorkerEntry` means the - // prompts never run for a name that was going to be refused anyway. + // runtime and size prompts never run for a name that was going to be + // refused anyway; the name prompt rejects it up front for the same reason. if (project.section.workers[name] !== undefined) { return yield* Effect.fail( new WorkerAlreadyConfiguredError({ @@ -159,14 +227,10 @@ export const legacyWorkersNew = Effect.fn("legacy.experimental.workers.new")(fun } // Resolved before anything is written, so cancelling either prompt leaves - // nothing behind — the name included. - // `-o` leaves `output.format` as `text`, and `promptSelect` goes through - // Clack, which writes its terminal UI to stdout with no stream override — so - // a prompt would land in front of the payload just as the notices did. With a - // machine format requested there is nowhere to ask, so the defaults stand. - const machineOutput = yield* legacyWorkersMachineOutputRequested(); - const runtime = yield* resolveRuntime({ explicit: flags.runtime, machineOutput }); - const size = yield* resolveSize({ explicit: flags.size, machineOutput }); + // nothing behind — the name included. With nowhere to ask, the defaults + // stand; only the name has nothing to fall back to. + const runtime = yield* resolveRuntime({ explicit: flags.runtime, canPrompt }); + const size = yield* resolveSize({ explicit: flags.size, canPrompt }); // Validated before anything is written: this is the directory the starter // files land in, so a value naming the project root, `supabase/`, or @@ -266,7 +330,7 @@ export const legacyWorkersNew = Effect.fn("legacy.experimental.workers.new")(fun // then the details. Guidance goes in a closing sentence rather than a // pseudo-row, since no other command puts a next step inside its output // table. - yield* output.raw(`Created new Worker at ${sourceDisplay}\n`); + yield* output.raw(`Created new Worker at ${legacyBold(sourceDisplay, process.stdout)}\n`); yield* output.raw( legacyRenderWorkerDetails([ ["Runtime", runtime], @@ -274,6 +338,11 @@ export const legacyWorkersNew = Effect.fn("legacy.experimental.workers.new")(fun ["Access", "public"], ]), ); - yield* output.raw(`Deploy it with supabase experimental workers push ${name}.\n`); + // On the success trailer rather than inline, the way `bootstrap` emits its + // "start your app" line: the shell prints trailers once at the end of the + // run, so the next step is the last thing on screen. + yield* emitSuccessTrailer( + `Deploy it with ${legacyAqua(`supabase experimental workers push ${name}`)}.\n`, + ); }).pipe(Effect.ensuring(telemetryState.flush)); }); diff --git a/apps/cli/src/legacy/commands/experimental/workers/new/new.integration.test.ts b/apps/cli/src/legacy/commands/experimental/workers/new/new.integration.test.ts index 6c9471aecf..07e4285ee2 100644 --- a/apps/cli/src/legacy/commands/experimental/workers/new/new.integration.test.ts +++ b/apps/cli/src/legacy/commands/experimental/workers/new/new.integration.test.ts @@ -13,6 +13,7 @@ import { import { InvalidWorkerNameError, InvalidWorkerSourceError, + MissingWorkerNameError, WorkerDirectoryExistsError, } from "../../../../../shared/workers/workers.errors.ts"; import { legacyWorkersNew } from "./new.handler.ts"; @@ -27,7 +28,7 @@ verify_jwt = false function flags(overrides: Partial = {}): LegacyWorkersNewFlags { return { - name: "api", + name: Option.some("api"), runtime: Option.none(), size: Option.none(), source: Option.none(), @@ -54,7 +55,7 @@ describe("legacy workers new", () => { const { layer, out } = setupLegacyWorkers({ workdir: repo.dir }); return Effect.gen(function* () { - yield* legacyWorkersNew(flags({ name: "api", runtime: Option.some("node") })); + yield* legacyWorkersNew(flags({ name: Option.some("api"), runtime: Option.some("node") })); const workerDir = join(repo.dir, "supabase", "workers", "api"); expect(existsSync(join(workerDir, "index.mjs"))).toBe(true); @@ -66,9 +67,89 @@ describe("legacy workers new", () => { // the shape `functions new` established. expect(out.stdoutText).toContain("Created new Worker at supabase/workers/api"); expect(out.stdoutText).toContain("Runtime"); - expect(out.stdoutText).toContain("supabase experimental workers push api"); + // The deploy hint is a success trailer, which lands on stderr. + expect(out.stderrText).toContain("supabase experimental workers push api"); }).pipe(Effect.provide(layer), Effect.ensuring(Effect.sync(repo.cleanup))); }); + it.live("asks for the name when the command line carries none", () => { + const repo = project(); + const { layer, out } = setupLegacyWorkers({ + workdir: repo.dir, + promptTextResponses: ["orders"], + promptSelectResponses: ["node", "2gb"], + }); + + return Effect.gen(function* () { + yield* legacyWorkersNew(flags({ name: Option.none() })); + + expect(out.promptTextCalls.map((call) => call.message)).toEqual([ + "What should this worker be called?", + ]); + expect(existsSync(join(repo.dir, "supabase", "workers", "orders", "index.mjs"))).toBe(true); + expect(repo.config()).toContain("[workers.orders]"); + }).pipe(Effect.provide(layer), Effect.ensuring(Effect.sync(repo.cleanup))); + }); + + // The prompt is the last place a mistyped or taken name can be corrected + // without ending the run, so it refuses both there rather than after asking. + it.live("refuses a bad or already-recorded name at the name prompt", () => { + const repo = project({ + "supabase/config.toml": `${CONFIG_WITH_COMMENTS}\n[workers.api]\nruntime = "node"\nsize = "2gb"\n`, + }); + const { layer, out } = setupLegacyWorkers({ + workdir: repo.dir, + promptTextResponses: ["orders"], + promptSelectResponses: ["node", "2gb"], + }); + + return Effect.gen(function* () { + yield* legacyWorkersNew(flags({ name: Option.none() })); + + const validate = out.promptTextCalls[0]?.opts?.validate; + expect(validate).toBeDefined(); + expect(validate?.("My_Worker")).toContain("lowercase letters"); + expect(validate?.("api")).toContain("already configured"); + expect(validate?.("orders")).toBeUndefined(); + expect(existsSync(join(repo.dir, "supabase", "workers", "orders", "index.mjs"))).toBe(true); + }).pipe(Effect.provide(layer), Effect.ensuring(Effect.sync(repo.cleanup))); + }); + + // Nowhere to ask means nothing to scaffold under: the name is the directory, + // the config key and the hostname, and none of those has a default. + it.live.each([ + { label: "not interactive", setup: { interactive: false } }, + // A TTY, but stdout was claimed by the payload, so a prompt would corrupt it. + { label: "-o json", setup: { goOutput: "json" as const } }, + // `printf 'orders\n' | supabase experimental workers new`: stdout is still a + // terminal, so `output.interactive` on its own would have fed the pipe + // straight into the name prompt instead of taking this documented path. + { label: "piped stdin", setup: { stdinIsTty: false } }, + ])("refuses a bare new when there is nowhere to ask ($label)", ({ setup }) => { + const repo = project(); + const { layer, out } = setupLegacyWorkers({ + workdir: repo.dir, + // An answer is waiting, so a prompt would succeed rather than fail some + // other way. + promptTextResponses: ["orders"], + ...setup, + }); + + return Effect.gen(function* () { + const error = yield* legacyWorkersNew(flags({ name: Option.none() })).pipe(Effect.flip); + + expect(error).toBeInstanceOf(MissingWorkerNameError); + if (!(error instanceof MissingWorkerNameError)) { + return yield* Effect.die("expected MissingWorkerNameError"); + } + // The retry has to name the path the command is actually registered at; + // `supabase workers new` is an unknown command. + expect(error.suggestion).toContain("supabase experimental workers new"); + expect(out.promptTextCalls).toEqual([]); + expect(existsSync(join(repo.dir, "supabase", "workers"))).toBe(false); + expect(repo.config()).toBe(CONFIG_WITH_COMMENTS); + }).pipe(Effect.provide(layer), Effect.ensuring(Effect.sync(repo.cleanup))); + }); + it.live("prompts for runtime and size when neither is given", () => { const repo = project(); const { layer, out } = setupLegacyWorkers({ @@ -77,7 +158,7 @@ describe("legacy workers new", () => { }); return Effect.gen(function* () { - yield* legacyWorkersNew(flags({ name: "api" })); + yield* legacyWorkersNew(flags({ name: Option.some("api") })); expect(out.promptSelectCalls.map((call) => call.message)).toEqual([ "Which runtime should this worker use?", @@ -89,12 +170,31 @@ describe("legacy workers new", () => { }).pipe(Effect.provide(layer), Effect.ensuring(Effect.sync(repo.cleanup))); }); + // The runtime and size prompts do have defaults to fall back on, so a piped + // stdin must leave them unasked rather than consuming the pipe. + it.live("takes the defaults without prompting when stdin is piped", () => { + const repo = project(); + const { layer, out } = setupLegacyWorkers({ + workdir: repo.dir, + stdinIsTty: false, + promptSelectResponses: ["node", "4gb"], + }); + + return Effect.gen(function* () { + yield* legacyWorkersNew(flags({ name: Option.some("api") })); + + expect(out.promptSelectCalls).toEqual([]); + expect(repo.config()).toContain('runtime = "deno"'); + expect(repo.config()).toContain('size = "2gb"'); + }).pipe(Effect.provide(layer), Effect.ensuring(Effect.sync(repo.cleanup))); + }); + it.live("falls back to the defaults without prompting when not interactive", () => { const repo = project(); const { layer, out } = setupLegacyWorkers({ workdir: repo.dir, format: "json" }); return Effect.gen(function* () { - yield* legacyWorkersNew(flags({ name: "api" })); + yield* legacyWorkersNew(flags({ name: Option.some("api") })); expect(out.promptSelectCalls).toHaveLength(0); expect(repo.config()).toContain('runtime = "deno"'); @@ -110,12 +210,12 @@ describe("legacy workers new", () => { return Effect.gen(function* () { yield* legacyWorkersNew( - flags({ name: "api", runtime: Option.some("deno"), size: Option.some("4gb") }), + flags({ name: Option.some("api"), runtime: Option.some("deno"), size: Option.some("4gb") }), ); const recorded = repo.config(); const error = yield* legacyWorkersNew( - flags({ name: "api", runtime: Option.some("node") }), + flags({ name: Option.some("api"), runtime: Option.some("node") }), ).pipe(Effect.flip); expect(error).toBeInstanceOf(WorkerAlreadyConfiguredError); @@ -136,7 +236,7 @@ describe("legacy workers new", () => { return Effect.gen(function* () { const error = yield* legacyWorkersNew( - flags({ name: "api", runtime: Option.some("node") }), + flags({ name: Option.some("api"), runtime: Option.some("node") }), ).pipe(Effect.flip); expect(error).toBeInstanceOf(WorkerAlreadyConfiguredError); @@ -154,7 +254,7 @@ describe("legacy workers new", () => { return Effect.gen(function* () { yield* legacyWorkersNew( flags({ - name: "api", + name: Option.some("api"), runtime: Option.some("node"), source: Option.some("packages/api"), }), @@ -174,7 +274,7 @@ describe("legacy workers new", () => { for (const source of [".", "..", "supabase", "supabase/functions"]) { const error = yield* legacyWorkersNew( flags({ - name: "api", + name: Option.some("api"), runtime: Option.some("node"), source: Option.some(source), }), @@ -194,7 +294,7 @@ describe("legacy workers new", () => { const { layer } = setupLegacyWorkers({ workdir: created.dir }); return Effect.gen(function* () { - yield* legacyWorkersNew(flags({ name: "api", runtime: Option.some("node") })); + yield* legacyWorkersNew(flags({ name: Option.some("api"), runtime: Option.some("node") })); expect(existsSync(join(created.dir, "supabase", "workers", "api", "index.mjs"))).toBe(true); expect(readFileSync(join(created.dir, "supabase", "config.toml"), "utf8")).toBe( @@ -212,7 +312,7 @@ describe("legacy workers new", () => { return Effect.gen(function* () { const error = yield* legacyWorkersNew( - flags({ name: "api", runtime: Option.some("node") }), + flags({ name: Option.some("api"), runtime: Option.some("node") }), ).pipe(Effect.flip); expect(error).toBeInstanceOf(WorkerDirectoryExistsError); @@ -228,7 +328,7 @@ describe("legacy workers new", () => { const { layer } = setupLegacyWorkers({ workdir: repo.dir }); return Effect.gen(function* () { - yield* legacyWorkersNew(flags({ name: "api", runtime: Option.some("node") })); + yield* legacyWorkersNew(flags({ name: Option.some("api"), runtime: Option.some("node") })); expect(existsSync(join(repo.dir, "supabase", "workers", "api", "index.mjs"))).toBe(true); }).pipe(Effect.provide(layer), Effect.ensuring(Effect.sync(repo.cleanup))); @@ -240,7 +340,7 @@ describe("legacy workers new", () => { return Effect.gen(function* () { const error = yield* legacyWorkersNew( - flags({ name: "api", runtime: Option.some("node") }), + flags({ name: Option.some("api"), runtime: Option.some("node") }), ).pipe(Effect.flip); expect(error).toBeInstanceOf(WorkerDirectoryExistsError); @@ -257,7 +357,9 @@ describe("legacy workers new", () => { const { layer } = setupLegacyWorkers({ workdir: repo.dir }); return Effect.gen(function* () { - const error = yield* legacyWorkersNew(flags({ name: "My_Worker" })).pipe(Effect.flip); + const error = yield* legacyWorkersNew(flags({ name: Option.some("My_Worker") })).pipe( + Effect.flip, + ); expect(error).toBeInstanceOf(InvalidWorkerNameError); expect(existsSync(join(repo.dir, "supabase", "workers"))).toBe(false); @@ -286,7 +388,7 @@ describe("legacy workers new", () => { return Effect.gen(function* () { const error = yield* legacyWorkersNew( - flags({ name: "api", runtime: Option.some("deno") }), + flags({ name: Option.some("api"), runtime: Option.some("deno") }), ).pipe(Effect.flip); expect(error).toBeInstanceOf(WorkerAlreadyConfiguredError); @@ -307,7 +409,7 @@ describe("legacy workers new", () => { const { layer } = setupLegacyWorkers({ workdir: repo.dir }); return Effect.gen(function* () { - yield* legacyWorkersNew(flags({ name: "api", runtime: Option.some("node") })); + yield* legacyWorkersNew(flags({ name: Option.some("api"), runtime: Option.some("node") })); const jsonPath = join(repo.dir, "supabase", "config.json"); expect(readFileSync(jsonPath, "utf8")).toBe(configJson); @@ -332,7 +434,7 @@ describe("legacy workers new", () => { const { layer } = setupLegacyWorkers({ workdir }); return Effect.gen(function* () { - yield* legacyWorkersNew(flags({ name: "api", runtime: Option.some("node") })); + yield* legacyWorkersNew(flags({ name: Option.some("api"), runtime: Option.some("node") })); // The ancestor project is untouched. expect(repo.config()).toBe(CONFIG_WITH_COMMENTS); @@ -357,7 +459,7 @@ describe("legacy workers new", () => { return Effect.gen(function* () { const error = yield* legacyWorkersNew( - flags({ name: "api", runtime: Option.some("node") }), + flags({ name: Option.some("api"), runtime: Option.some("node") }), ).pipe(Effect.flip); expect(error).toBeInstanceOf(WorkerConfigWriteUnsafeError); @@ -366,15 +468,15 @@ describe("legacy workers new", () => { }).pipe(Effect.provide(layer), Effect.ensuring(Effect.sync(repo.cleanup))); }); - // A plain file used to read as an empty directory, which then failed with a - // bare EEXIST from `makeDirectory` instead of naming what was in the way. + // A plain file must not read as an empty directory: that fails with a bare + // EEXIST from `makeDirectory` instead of naming what is in the way. it.live("refuses a plain file at the destination", () => { const repo = project({ "supabase/workers/api": "not a directory" }); const { layer } = setupLegacyWorkers({ workdir: repo.dir }); return Effect.gen(function* () { const error = yield* legacyWorkersNew( - flags({ name: "api", runtime: Option.some("node") }), + flags({ name: Option.some("api"), runtime: Option.some("node") }), ).pipe(Effect.flip); expect(error).toBeInstanceOf(WorkerDirectoryExistsError); @@ -396,7 +498,7 @@ describe("legacy workers new", () => { return Effect.gen(function* () { yield* legacyWorkersNew( flags({ - name: "api", + name: Option.some("api"), runtime: Option.some("node"), source: Option.some("generated"), }), @@ -423,7 +525,7 @@ describe("legacy workers new", () => { }); return Effect.gen(function* () { - yield* legacyWorkersNew(flags({ name: "api" })); + yield* legacyWorkersNew(flags({ name: Option.some("api") })); const payload: unknown = JSON.parse(out.stdoutText); // The defaults stand, because there was nowhere to ask. @@ -432,6 +534,30 @@ describe("legacy workers new", () => { }).pipe(Effect.provide(layer), Effect.ensuring(Effect.sync(repo.cleanup))); }); + // The prompts only ever offer values this CLI knows, so an unrecognized answer + // means the prompt layer handed back something off-menu. Recording it verbatim + // would put a runtime into config.toml that `push` then refuses; the default + // is the one answer that still scaffolds something deployable. + it.live("falls back to the defaults when a prompt answers off-menu", () => { + const repo = project(); + const { layer } = setupLegacyWorkers({ + workdir: repo.dir, + promptSelectResponses: ["cobol", "colossal"], + }); + + return Effect.gen(function* () { + yield* legacyWorkersNew({ + name: Option.some("api"), + runtime: Option.none(), + size: Option.none(), + source: Option.none(), + }); + + expect(repo.config()).toContain(`runtime = "deno"`); + expect(repo.config()).toContain(`size = "2gb"`); + }).pipe(Effect.provide(layer), Effect.ensuring(Effect.sync(repo.cleanup))); + }); + it.live("refuses --source pointed at the project config file", () => { const repo = project(); const { layer } = setupLegacyWorkers({ workdir: repo.dir }); @@ -439,7 +565,7 @@ describe("legacy workers new", () => { return Effect.gen(function* () { const error = yield* legacyWorkersNew( flags({ - name: "api", + name: Option.some("api"), runtime: Option.some("node"), source: Option.some(join("supabase", "config.toml")), }), diff --git a/apps/cli/src/legacy/commands/experimental/workers/push/SIDE_EFFECTS.md b/apps/cli/src/legacy/commands/experimental/workers/push/SIDE_EFFECTS.md index 4941f0b37f..30c0305291 100644 --- a/apps/cli/src/legacy/commands/experimental/workers/push/SIDE_EFFECTS.md +++ b/apps/cli/src/legacy/commands/experimental/workers/push/SIDE_EFFECTS.md @@ -72,6 +72,17 @@ payload always carries a `workers` array, which a flat `KEY=value` list cannot express, and discovering that at the end would fail the command with the remote project already changed. +A multi-worker run stops at the first failure, and names the workers it never +attempted on stderr in **every** format, machine ones included: that run is a +CI run, where nobody watched the loop and "what still needs deploying" is the +question the failure raises. The per-worker `Deploying Worker n/N:` announcement +is text-only by contrast, since it is progress rather than an outcome. + +Both retry suggestions — the one on a failed build and the one on a build that +never settled — carry an explicit `--project-ref` when the flag supplied the +ref, since they are copy-pasted verbatim. A suggestion that dropped it would +re-resolve against whatever this checkout happens to be linked to. + The presigned `PUT` above is the one request whose URL is itself a credential. `--debug` logs every request URL, so `legacyHttpClientLayer` redacts query strings that carry a signature. diff --git a/apps/cli/src/legacy/commands/experimental/workers/push/push.handler.ts b/apps/cli/src/legacy/commands/experimental/workers/push/push.handler.ts index 088132e053..cc90677741 100644 --- a/apps/cli/src/legacy/commands/experimental/workers/push/push.handler.ts +++ b/apps/cli/src/legacy/commands/experimental/workers/push/push.handler.ts @@ -6,6 +6,7 @@ import { legacyEmitWorkersMachineOutput, legacyRejectWorkersEnvOutput, legacyWorkersMachineOutputRequested, + legacyWorkersProjectRefSuffix, } from "../workers.output.ts"; import { legacyAqua } from "../../../../shared/legacy-colors.ts"; import { LegacyPlatformApi } from "../../../../auth/legacy-platform-api.service.ts"; @@ -178,6 +179,12 @@ const deployOneWorker = Effect.fnUntraced(function* (input: { readonly project: LegacyWorkersProject; readonly name: string; readonly projectRef: string; + /** + * ` --project-ref ` when the flag supplied the ref, `""` when the link + * did — the follow-up hint below is copy-pasted verbatim, so it has to carry + * whatever the user typed to reach this project. + */ + readonly refSuffix: string; readonly instances: Option.Option; readonly pollSchedule?: Schedule.Schedule; readonly pollRetrySchedule?: Schedule.Schedule; @@ -325,6 +332,7 @@ const deployOneWorker = Effect.fnUntraced(function* (input: { const settled = yield* awaitWorkerBuild(api, projectRef, name, { schedule: input.pollSchedule, retrySchedule: input.pollRetrySchedule, + refSuffix: input.refSuffix, onPoll: (polled) => polled.buildState === "building" ? deploying.message("Building worker...") : Effect.void, }).pipe(Effect.tapError(() => deploying.fail())); @@ -336,7 +344,7 @@ const deployOneWorker = Effect.fnUntraced(function* (input: { detail: `The build for "${name}" failed${ settled.stateReason === undefined ? "" : `: ${settled.stateReason}` }.`, - suggestion: `Fix the issue, then re-run \`supabase experimental workers push ${name}\`.`, + suggestion: `Fix the issue, then re-run \`supabase experimental workers push ${name}${input.refSuffix}\`.`, }), ); } @@ -383,6 +391,28 @@ const deployOneWorker = Effect.fnUntraced(function* (input: { }; }); +/** + * Names the workers a failed run never got to. + * + * The loop stops on the first failure, so everything after it was never + * attempted — and the error itself only names the worker that broke. Left + * unsaid, the user has to reconstruct the remainder from argument order, or + * from the discovery walk's ordering when the push was a bare `push`. + * + * Written on stderr in every format, unlike the per-worker announcements: a + * machine-format run is a CI run, which is exactly where nobody is watching the + * loop and "what still needs deploying" is the question the failure raises. + */ +const reportUnattempted = Effect.fnUntraced(function* (skipped: ReadonlyArray) { + if (skipped.length === 0) { + return; + } + const output = yield* Output; + // A label rather than a sentence, so it reads the same for one name or six + // and carries no verb to agree with the count. + yield* output.raw(`Not attempted: ${skipped.join(", ")}\n`, "stderr"); +}); + /** * `supabase experimental workers push [name...]` — deploy the named workers, or every worker * in the project when none are named, mirroring `supabase functions deploy`. @@ -439,26 +469,52 @@ export const legacyWorkersPush = Effect.fn("legacy.experimental.workers.push")(f yield* legacyRejectWorkersEnvOutput(); const machineOutput = yield* legacyWorkersMachineOutputRequested(); + // Computed once for the whole run, the way `status` and `delete` do: an + // explicit `--project-ref` has to survive into every hint this push emits. + const refSuffix = legacyWorkersProjectRefSuffix(flags.projectRef); const deployed: Array> = []; - for (const name of names) { - if (names.length > 1 && !machineOutput) { + for (const [index, name] of names.entries()) { + if (names.length > 1 && !machineOutput && output.format === "text") { // stderr, unblanked and labelled, the way `functions deploy` announces // each function: a bare name with a leading blank line put a section // header into whatever was consuming stdout. - yield* output.raw(`Deploying Worker: ${legacyAqua(name)}\n`, "stderr"); + // + // Counted, because each worker's package/upload/build takes minutes and + // the name alone says nothing about how much of the run is left. + // + // Text only, on both axes: `machineOutput` tracks `-o`, which leaves + // `output.format` as `text`, so neither check covers the other. This is + // progress rather than an outcome, and `--output-format json` asked for + // a stream of events — unlike the unattempted-workers report below, + // which every format gets because it says what still needs deploying. + yield* output.raw( + `Deploying Worker ${index + 1}/${names.length}: ${legacyAqua(name)}\n`, + "stderr", + ); } deployed.push( yield* deployOneWorker({ project, name, projectRef, + refSuffix, instances: flags.instances, machineOutput, ...(options.pollSchedule === undefined ? {} : { pollSchedule: options.pollSchedule }), ...(options.pollRetrySchedule === undefined ? {} : { pollRetrySchedule: options.pollRetrySchedule }), - }), + }).pipe(Effect.tapError(() => reportUnattempted(names.slice(index + 1)))), + ); + } + + // Only for a run that deployed several: one worker already said so itself, + // and repeating it as a summary reads like a second deploy. + if (names.length > 1 && !machineOutput && output.format === "text") { + yield* output.raw( + `Deployed ${names.length} Workers to project ${projectRef}: ${names + .map((name) => legacyAqua(name, process.stdout)) + .join(", ")}\n`, ); } diff --git a/apps/cli/src/legacy/commands/experimental/workers/push/push.integration.test.ts b/apps/cli/src/legacy/commands/experimental/workers/push/push.integration.test.ts index 1e7cb049d9..567d15c8c3 100644 --- a/apps/cli/src/legacy/commands/experimental/workers/push/push.integration.test.ts +++ b/apps/cli/src/legacy/commands/experimental/workers/push/push.integration.test.ts @@ -1,4 +1,4 @@ -import { chmodSync, readdirSync, rmSync, symlinkSync, writeFileSync } from "node:fs"; +import { chmodSync, readdirSync, rmSync, statSync, symlinkSync, writeFileSync } from "node:fs"; import { join } from "node:path"; import { describe, expect, it } from "@effect/vitest"; import { Effect, Option, Predicate, Schedule } from "effect"; @@ -14,10 +14,13 @@ import { LegacyProjectNotLinkedError } from "../../../../config/legacy-project-r import { LegacyWorkersEnvNotSupportedError } from "../workers.errors.ts"; import { NoWorkersToDeployError, + UnknownWorkerRuntimeError, + UnknownWorkerSizeError, WorkerBuildFailedError, WorkerBuildTimeoutError, WorkerProjectNotFoundError, WorkersUnavailableError, + WorkerSourceEscapingLinkError, WorkerSourceMissingError, WorkerUploadFailedError, } from "../../../../../shared/workers/workers.errors.ts"; @@ -96,6 +99,16 @@ function listableAsCurrentUser(path: string): boolean { } } +/** The same question one level down: can this path still be stat-ed? */ +function stattableAsCurrentUser(path: string): boolean { + try { + statSync(path); + return true; + } catch { + return false; + } +} + function push(flagOverrides: Partial = {}) { // Both schedules are injected: the outer poll and the per-read retry. The // production retry is spaced in seconds, so leaving it in place made the @@ -193,6 +206,42 @@ describe("legacy workers push", () => { }).pipe(Effect.provide(layer), Effect.ensuring(Effect.sync(repo.cleanup))); }); + // `[workers.*] runtime` and `size` are plain strings in the config schema, so + // an unrecognized value reaches the handler rather than failing the parse. + // Naming the accepted values beats echoing a schema error, and the refusal + // has to land before anything is packaged or uploaded. + it.live("names the runtimes on offer when config records one it does not know", () => { + const repo = project({ + "supabase/config.toml": `project_id = "demo"\n\n[workers.api]\nruntime = "cobol"\n`, + }); + const { layer, http } = setupLegacyWorkers({ workdir: repo.dir, routes: routes() }); + + return Effect.gen(function* () { + const error = yield* push().pipe(Effect.flip); + + expect(error).toBeInstanceOf(UnknownWorkerRuntimeError); + expect((error as UnknownWorkerRuntimeError).detail).toContain("cobol"); + expect((error as UnknownWorkerRuntimeError).suggestion).toContain("dockerfile, node, deno"); + expect(http.requests).toHaveLength(0); + }).pipe(Effect.provide(layer), Effect.ensuring(Effect.sync(repo.cleanup))); + }); + + it.live("names the sizes on offer when config records one it does not know", () => { + const repo = project({ + "supabase/config.toml": `project_id = "demo"\n\n[workers.api]\nruntime = "node"\nsize = "huge"\n`, + }); + const { layer, http } = setupLegacyWorkers({ workdir: repo.dir, routes: routes() }); + + return Effect.gen(function* () { + const error = yield* push().pipe(Effect.flip); + + expect(error).toBeInstanceOf(UnknownWorkerSizeError); + expect((error as UnknownWorkerSizeError).detail).toContain("huge"); + expect((error as UnknownWorkerSizeError).suggestion).toContain("2gb, 4gb"); + expect(http.requests).toHaveLength(0); + }).pipe(Effect.provide(layer), Effect.ensuring(Effect.sync(repo.cleanup))); + }); + it.live("sends the recorded size and the requested instance count", () => { const repo = project({ "supabase/config.toml": `project_id = "demo"\n\n[workers.api]\nruntime = "node"\nsize = "4gb"\n`, @@ -301,6 +350,76 @@ describe("legacy workers push", () => { }).pipe(Effect.provide(layer), Effect.ensuring(Effect.sync(repo.cleanup))); }); + // The reason is optional in the API contract, so the detail has to read as a + // sentence without one rather than trailing a bare colon. + it.live("reports a failed build that came with no reason", () => { + const repo = project(); + const { layer } = setupLegacyWorkers({ + workdir: repo.dir, + routes: routes({ + [`GET ${workersRoute("/api")}`]: { + status: 200, + body: { data: workerResource({ name: "api", runtime: "node", buildState: "failed" }) }, + }, + }), + }); + + return Effect.gen(function* () { + const error = yield* push().pipe(Effect.flip); + + expect(error).toBeInstanceOf(WorkerBuildFailedError); + expect((error as WorkerBuildFailedError).detail).toBe(`The build for "api" failed.`); + }).pipe(Effect.provide(layer), Effect.ensuring(Effect.sync(repo.cleanup))); + }); + + // Every deploy this CLI sends asks for public exposure, but the accepted spec + // is the platform's answer, not the request echoed back. A worker it did not + // expose has no URL to print, and inventing one from the ref would name an + // address that does not resolve. + it.live("omits the URL for a worker the platform did not expose publicly", () => { + const repo = project(); + const { layer, out } = setupLegacyWorkers({ + workdir: repo.dir, + routes: routes({ + [`GET ${workersRoute("/api")}`]: { + status: 200, + body: { + data: workerResource({ + name: "api", + runtime: "node", + buildState: "active", + exposure: "private", + }), + }, + }, + }), + }); + + return Effect.gen(function* () { + yield* push(); + + expect(out.stdoutText).toContain("Deployed Worker api"); + expect(out.stdoutText).toContain("private"); + expect(out.stdoutText).not.toContain("https://"); + expect(out.stdoutText).not.toContain("URL"); + }).pipe(Effect.provide(layer), Effect.ensuring(Effect.sync(repo.cleanup))); + }); + + // The schedules every other test injects are a seam: the command itself calls + // the handler with no options at all. The stubbed worker settles on the first + // poll, so the production schedules never get to space anything out. + it.live("deploys when called the way the command wires it, with no test seams", () => { + const repo = project(); + const { layer, out, http } = setupLegacyWorkers({ workdir: repo.dir, routes: routes() }); + + return Effect.gen(function* () { + yield* legacyWorkersPush(flags()); + + expect(http.routeKeys).toContain(`POST ${workersRoute("/api/deploy")}`); + expect(out.stdoutText).toContain("Deployed Worker api"); + }).pipe(Effect.provide(layer), Effect.ensuring(Effect.sync(repo.cleanup))); + }); + it.live("stops waiting on a build that never settles, and says where to look", () => { const repo = project(); const { layer } = setupLegacyWorkers({ @@ -314,9 +433,9 @@ describe("legacy workers push", () => { }); return Effect.gen(function* () { - const error = yield* legacyWorkersPush(flags(), { pollSchedule: Schedule.recurs(2) }).pipe( - Effect.flip, - ); + const error = yield* legacyWorkersPush(flags(), { + pollSchedule: Schedule.recurs(2), + }).pipe(Effect.flip); expect(error).toBeInstanceOf(WorkerBuildTimeoutError); expect((error as { suggestion: string }).suggestion).toContain( @@ -325,6 +444,84 @@ describe("legacy workers push", () => { }).pipe(Effect.provide(layer), Effect.ensuring(Effect.sync(repo.cleanup))); }); + // Every "run this next" string here is copy-pasted verbatim. From an unlinked + // checkout — or one linked elsewhere — dropping the `--project-ref` the user + // typed either fails to resolve or silently addresses a same-named worker in + // whatever project this checkout points at. + describe("carries an explicit --project-ref into its hints", () => { + const unlinked = (repoDir: string, routeOverrides = {}) => + setupLegacyWorkers({ + workdir: repoDir, + linked: false, + routes: routes(routeOverrides), + }); + const withRef = { projectRef: Option.some(WORKERS_PROJECT_REF) }; + + it.live("in the failed-build retry suggestion", () => { + const repo = project(); + const { layer } = unlinked(repo.dir, { + [`GET ${workersRoute("/api")}`]: { + status: 200, + body: { data: workerResource({ name: "api", runtime: "node", buildState: "failed" }) }, + }, + }); + + return Effect.gen(function* () { + const error = yield* push(withRef).pipe(Effect.flip); + + expect(error).toBeInstanceOf(WorkerBuildFailedError); + expect((error as WorkerBuildFailedError).suggestion).toContain( + `supabase experimental workers push api --project-ref ${WORKERS_PROJECT_REF}`, + ); + }).pipe(Effect.provide(layer), Effect.ensuring(Effect.sync(repo.cleanup))); + }); + + it.live("in the give-up-waiting suggestion", () => { + const repo = project(); + const { layer } = unlinked(repo.dir, { + [`GET ${workersRoute("/api")}`]: { + status: 200, + body: { data: workerResource({ name: "api", buildState: "building" }) }, + }, + }); + + return Effect.gen(function* () { + const error = yield* legacyWorkersPush(flags(withRef), { + pollSchedule: Schedule.recurs(2), + }).pipe(Effect.flip); + + expect(error).toBeInstanceOf(WorkerBuildTimeoutError); + expect((error as { suggestion: string }).suggestion).toContain( + `supabase experimental workers status api --project-ref ${WORKERS_PROJECT_REF}`, + ); + }).pipe(Effect.provide(layer), Effect.ensuring(Effect.sync(repo.cleanup))); + }); + + // The mirror image: when the link supplied the ref, repeating it back is + // noise on a command that already resolves to the right project. + it.live("but leaves it off when the link supplied the ref", () => { + const repo = project(); + const { layer } = setupLegacyWorkers({ + workdir: repo.dir, + routes: routes({ + [`GET ${workersRoute("/api")}`]: { + status: 200, + body: { data: workerResource({ name: "api", buildState: "failed" }) }, + }, + }), + }); + + return Effect.gen(function* () { + const error = yield* push().pipe(Effect.flip); + + expect((error as WorkerBuildFailedError).suggestion).toContain( + "supabase experimental workers push api", + ); + expect((error as WorkerBuildFailedError).suggestion).not.toContain("--project-ref"); + }).pipe(Effect.provide(layer), Effect.ensuring(Effect.sync(repo.cleanup))); + }); + }); + it.live("fails before deploying when the presigned upload is rejected", () => { const repo = project(); const { layer, http } = setupLegacyWorkers({ @@ -344,6 +541,26 @@ describe("legacy workers push", () => { // section, so it has to honour one: loading TOML-only left the section empty, // which meant a guessed runtime and default size and instance count for a // worker that had configured all three. + // The context is already uploaded by the time the deploy is refused, so the + // failure has to be reported as the deploy's, not the upload's. + it.live("reports a rejected deploy after the context has been uploaded", () => { + const repo = project(); + const { layer, http } = setupLegacyWorkers({ + workdir: repo.dir, + routes: routes({ + [`POST ${workersRoute("/api/deploy")}`]: { status: 500, body: { message: "boom" } }, + }), + }); + + return Effect.gen(function* () { + const error = yield* push().pipe(Effect.flip); + + expect(Predicate.isTagged(error, "WorkersApiUnexpectedStatusError")).toBe(true); + expect(http.routeKeys).toContain("PUT /deploy-context/api.tar.gz"); + expect(http.routeKeys).toContain(`POST ${workersRoute("/api/deploy")}`); + }).pipe(Effect.provide(layer), Effect.ensuring(Effect.sync(repo.cleanup))); + }); + it.live("deploys a worker configured in config.json, not just config.toml", () => { const created = makeWorkersProject({ "supabase/config.json": JSON.stringify({ @@ -623,6 +840,24 @@ describe("legacy workers push", () => { ); }); + // Packaging stores symlinks rather than following them, so a link out of the + // tree would package a path the build cannot resolve. It is refused while + // packaging — before a slot is minted — so nothing is uploaded for a context + // that could never build. + it.live("refuses a source that links outside itself, before minting a slot", () => { + const repo = project(); + symlinkSync("../../config.toml", join(repo.dir, "supabase", "workers", "api", "escape.toml")); + const { layer, http } = setupLegacyWorkers({ workdir: repo.dir, routes: routes() }); + + return Effect.gen(function* () { + const error = yield* push().pipe(Effect.flip); + + expect(error).toBeInstanceOf(WorkerSourceEscapingLinkError); + expect((error as WorkerSourceEscapingLinkError).detail).toContain("escape.toml"); + expect(http.requests).toHaveLength(0); + }).pipe(Effect.provide(layer), Effect.ensuring(Effect.sync(repo.cleanup))); + }); + it.live("rides out a transient failure while polling the build", () => { const repo = project(); const { layer, http } = setupLegacyWorkers({ @@ -702,6 +937,65 @@ describe("legacy workers push", () => { http.routeKeys.indexOf(`POST ${workersRoute("/web/deploy")}`), ); expect(out.stdoutText).toContain("web"); + // Each worker is announced with its place in the run, and the run closes + // by naming everything it deployed. + expect(out.stderrText).toContain("Deploying Worker 1/2: api"); + expect(out.stderrText).toContain("Deploying Worker 2/2: web"); + expect(out.stdoutText).toContain( + `Deployed 2 Workers to project ${WORKERS_PROJECT_REF}: api, web`, + ); + }).pipe(Effect.provide(layer), Effect.ensuring(Effect.sync(repo.cleanup))); + }); + + // The other half of that stat: an entry that is there but cannot be read is a + // real filesystem problem, not a name to skip. Dropping it would deploy a + // subset of the project and report success. Root ignores the permission bits, + // and CI sometimes runs as root, so this asserts the outcome that actually + // applies rather than skipping. + it.live("fails rather than skipping a workers entry it cannot stat", () => { + const repo = project(); + const workersRoot = join(repo.dir, "supabase", "workers"); + // Readable, so the listing still names `api`; not traversable, so stat-ing + // anything inside it fails with a permission error. + chmodSync(workersRoot, 0o600); + const stattable = stattableAsCurrentUser(join(workersRoot, "api")); + const { layer, http } = setupLegacyWorkers({ workdir: repo.dir, routes: routes() }); + + return Effect.gen(function* () { + if (stattable) { + yield* push({ names: [] }); + expect(http.routeKeys).toContain(`POST ${workersRoute("/api/deploy")}`); + return; + } + const error = yield* push({ names: [] }).pipe(Effect.flip); + + expect(error).not.toBeInstanceOf(NoWorkersToDeployError); + expect(Predicate.isTagged(error, "PlatformError")).toBe(true); + expect(http.requests).toHaveLength(0); + }).pipe( + Effect.provide(layer), + Effect.ensuring( + Effect.sync(() => { + chmodSync(workersRoot, 0o700); + repo.cleanup(); + }), + ), + ); + }); + + // A dangling link in the workers root is listed by the directory read but has + // nothing to stat. Discovery skips it rather than failing the whole run over a + // path that names no worker. + it.live("skips a dangling link in the workers root while discovering", () => { + const repo = project(); + symlinkSync("nowhere", join(repo.dir, "supabase", "workers", "ghost")); + const { layer, http } = setupLegacyWorkers({ workdir: repo.dir, routes: routes() }); + + return Effect.gen(function* () { + yield* push({ names: [] }); + + expect(http.routeKeys).toContain(`POST ${workersRoute("/api/deploy")}`); + expect(http.routeKeys.some((key) => key.includes("/ghost"))).toBe(false); }).pipe(Effect.provide(layer), Effect.ensuring(Effect.sync(repo.cleanup))); }); @@ -739,6 +1033,41 @@ describe("legacy workers push", () => { ); }); + it.live("names the workers a failed run never got to", () => { + const repo = project({ + "supabase/config.toml": `project_id = "demo"\n\n[workers.api]\nruntime = "node"\n\n[workers.web]\nruntime = "node"\n`, + "supabase/workers/web/index.js": "export default {};\n", + }); + const { layer, out, http } = setupLegacyWorkers({ + workdir: repo.dir, + routes: routes({ + // `api` sorts first, so the run stops before `web` is ever touched. + [`GET ${workersRoute("/api")}`]: { + status: 200, + body: { + data: workerResource({ + name: "api", + runtime: "node", + buildState: "failed", + stateReason: "error building image: exit status 1", + }), + }, + }, + }), + }); + + return Effect.gen(function* () { + const error = yield* push({ names: [] }).pipe(Effect.flip); + + expect(error).toBeInstanceOf(WorkerBuildFailedError); + expect(out.stderrText).toContain("Not attempted: web"); + // Named rather than deployed: the run really did stop. + expect(http.routeKeys).not.toContain(`POST ${workersRoute("/web/deploy")}`); + // No summary either — nothing finished. + expect(out.stdoutText).not.toContain("Deployed 2 Workers"); + }).pipe(Effect.provide(layer), Effect.ensuring(Effect.sync(repo.cleanup))); + }); + it.live("fails when there are no workers to deploy at all", () => { const repo = project({ "supabase/config.toml": `project_id = "demo"\n` }); rmSync(join(repo.dir, "supabase", "workers"), { recursive: true, force: true }); @@ -810,6 +1139,38 @@ describe("legacy workers push", () => { }).pipe(Effect.provide(layer), Effect.ensuring(Effect.sync(repo.cleanup))); }); + // `--output-format json` asked for a stream of events, so progress does not + // belong in it — unlike the "not attempted" report, which every format gets. + it.live("keeps per-worker progress out of json mode", () => { + const repo = project({ + "supabase/config.toml": `project_id = "demo"\n\n[workers.api]\nruntime = "node"\n\n[workers.web]\nruntime = "node"\n`, + "supabase/workers/web/index.js": "export default {};\n", + }); + const { layer, out } = setupLegacyWorkers({ + workdir: repo.dir, + format: "json", + routes: { + ...routes(), + [`POST ${workersRoute("/web/uploads")}`]: { status: 201, body: uploadSlot }, + [`POST ${workersRoute("/web/deploy")}`]: { + status: 202, + body: { data: workerResource({ name: "web", runtime: "node", buildState: "building" }) }, + }, + [`GET ${workersRoute("/web")}`]: { + status: 200, + body: { data: workerResource({ name: "web", runtime: "node", buildState: "active" }) }, + }, + }, + }); + + return Effect.gen(function* () { + yield* push({ names: [] }); + + expect(out.stderrText).not.toContain("Deploying Worker"); + expect(out.stdoutText).not.toContain("Deployed 2 Workers"); + }).pipe(Effect.provide(layer), Effect.ensuring(Effect.sync(repo.cleanup))); + }); + // `-o env` cannot express the `workers` array. Discovering that at emit time // meant failing with the project already changed, inviting a retry that // deployed all over again. @@ -829,8 +1190,8 @@ describe("legacy workers push", () => { }).pipe(Effect.provide(layer), Effect.ensuring(Effect.sync(repo.cleanup))); }); - // The "nothing to deploy" guard counts directory entries, so a tree of empty - // subdirectories used to package to zero files and deploy an image with no + // The "nothing to deploy" guard counts directory entries, so without this a tree + // of empty subdirectories packages to zero files and deploys an image with no // handler in it. it.live("refuses a source holding only empty directories, before minting a slot", () => { const repo = project({ "supabase/workers/api/nested/.keep": "" }); @@ -886,8 +1247,8 @@ describe("legacy workers push", () => { }).pipe(Effect.provide(layer), Effect.ensuring(Effect.sync(repo.cleanup))); }); - // A malformed config.toml used to fail outside the finalizers, so the run - // skipped the telemetry flush every invocation is supposed to perform. + // A malformed config.toml must fail inside the finalizers, or the run skips the + // telemetry flush every invocation is supposed to perform. it.live("flushes telemetry when the project config cannot be loaded", () => { const repo = project({ "supabase/config.toml": "project_id = [unclosed\n" }); const { layer, telemetry } = setupLegacyWorkers({ workdir: repo.dir, routes: routes() }); diff --git a/apps/cli/src/legacy/commands/experimental/workers/status/SIDE_EFFECTS.md b/apps/cli/src/legacy/commands/experimental/workers/status/SIDE_EFFECTS.md index f58c69978c..26c7c76a48 100644 --- a/apps/cli/src/legacy/commands/experimental/workers/status/SIDE_EFFECTS.md +++ b/apps/cli/src/legacy/commands/experimental/workers/status/SIDE_EFFECTS.md @@ -61,11 +61,17 @@ wrapper emits for every command. ## Output Formats -| Mode | stdout | stderr | -| ----------------------------- | ------------------------------------------------------------------------------------------------------ | ---------------------------- | -| text (default) | the details block, plus the build-retry line on a failure | an unreadable instance tally | -| `--output-format json` | one structured result carrying every reported field | as above | -| `--output-format stream-json` | the same result as a single terminal event | as above | -| `-o json` / `yaml` / `toml` | the same payload in that encoding, and nothing else | as above | -| `-o pretty` / `table` / `csv` | the text rendering — these fall through rather than encoding | as above | -| `-o env` | refused before any request; the payload nests an instance tally a flat `KEY=value` list cannot express | the error | +| Mode | stdout | stderr | +| ----------------------------- | ------------------------------------------------------------------------------------------------------ | ------------------------------------------------------------------- | +| text (default) | the details block | an unreadable instance tally, and the build-retry hint on a failure | +| `--output-format json` | one structured result carrying every reported field | neither — both are text-only | +| `--output-format stream-json` | the same result as a single terminal event | neither — both are text-only | +| `-o json` / `yaml` / `toml` | the same payload in that encoding, and nothing else | neither — both are text-only | +| `-o pretty` / `table` / `csv` | the text rendering — these fall through rather than encoding | both, as in text | +| `-o env` | refused before any request; the payload nests an instance tally a flat `KEY=value` list cannot express | the error | + +A structured emission is the end of the run: the handler returns at +`legacyEmitWorkersMachineOutput` or at `output.success`, so nothing in the text +branch below it — the instance tally and the build-retry trailer — is reached. +`-o pretty`, `table` and `csv` are the exception, since they encode nothing and +fall through to that same text branch. diff --git a/apps/cli/src/legacy/commands/experimental/workers/status/status.handler.ts b/apps/cli/src/legacy/commands/experimental/workers/status/status.handler.ts index 44aff12f4c..b0319080af 100644 --- a/apps/cli/src/legacy/commands/experimental/workers/status/status.handler.ts +++ b/apps/cli/src/legacy/commands/experimental/workers/status/status.handler.ts @@ -1,5 +1,7 @@ import { Effect, Option } from "effect"; import { Output } from "../../../../../shared/output/output.service.ts"; +import { emitSuccessTrailer } from "../../../../../shared/cli/success-trailer.ts"; +import { legacyAqua } from "../../../../shared/legacy-colors.ts"; import { legacyRenderWorkerDetails } from "../workers.format.ts"; import { legacyEmitWorkersMachineOutput, @@ -151,8 +153,10 @@ export const legacyWorkersStatus = Effect.fn("legacy.experimental.workers.status // Not while it is being torn down: deletion is asynchronous, so a push here // races the tombstone or resurrects the very worker the user is removing. if (record.buildState === "failed" && record.deleting !== true) { - yield* output.raw( - `Fix the issue, then re-run supabase experimental workers push ${name}${refSuffix}.\n`, + // Trailer, like every other "what to run next" line in this shell: the + // command reports a failed build but exits 0, so the trailer flushes. + yield* emitSuccessTrailer( + `Fix the issue, then re-run ${legacyAqua(`supabase experimental workers push ${name}${refSuffix}`)}.\n`, ); } }).pipe( diff --git a/apps/cli/src/legacy/commands/experimental/workers/status/status.integration.test.ts b/apps/cli/src/legacy/commands/experimental/workers/status/status.integration.test.ts index bcc0f709cc..ee525a8cd1 100644 --- a/apps/cli/src/legacy/commands/experimental/workers/status/status.integration.test.ts +++ b/apps/cli/src/legacy/commands/experimental/workers/status/status.integration.test.ts @@ -223,7 +223,8 @@ describe("legacy workers status", () => { expect(out.stdoutText).toContain("failed"); expect(out.stdoutText).toContain("exit status 1"); - expect(out.stdoutText).toContain("supabase experimental workers push api"); + // The retry hint is a success trailer, which lands on stderr. + expect(out.stderrText).toContain("supabase experimental workers push api"); }).pipe(Effect.provide(layer), Effect.ensuring(Effect.sync(repo.cleanup))); }); @@ -486,6 +487,32 @@ describe("legacy workers status", () => { }).pipe(Effect.provide(layer), Effect.ensuring(Effect.sync(repo.cleanup))); }); + // The URL is derived from the exposure the platform reports, not assumed: a + // worker it did not expose has no address to print, and the row is dropped + // rather than rendered empty. + it.live("omits the URL for a worker that is not publicly exposed", () => { + const repo = project(); + const { layer, out } = setupLegacyWorkers({ + workdir: repo.dir, + routes: { + [getRoute]: { + status: 200, + body: { + data: workerResource({ name: "api", runtime: "node", exposure: "private" }), + }, + }, + }, + }); + + return Effect.gen(function* () { + yield* legacyWorkersStatus({ name: "api", projectRef: Option.none() }); + + expect(out.stdoutText).toContain("private"); + expect(out.stdoutText).not.toContain("URL"); + expect(out.stdoutText).not.toContain("https://"); + }).pipe(Effect.provide(layer), Effect.ensuring(Effect.sync(repo.cleanup))); + }); + it.live("refuses -o env before making any request at all", () => { const repo = project(); const { layer, http } = setupLegacyWorkers({ diff --git a/apps/cli/src/legacy/commands/experimental/workers/workers.format.unit.test.ts b/apps/cli/src/legacy/commands/experimental/workers/workers.format.unit.test.ts new file mode 100644 index 0000000000..11fba19cf1 --- /dev/null +++ b/apps/cli/src/legacy/commands/experimental/workers/workers.format.unit.test.ts @@ -0,0 +1,34 @@ +import { describe, expect, it } from "vitest"; +import { legacyRenderWorkerDetails } from "./workers.format.ts"; + +describe("legacyRenderWorkerDetails", () => { + it("pads every label to the widest one", () => { + expect( + legacyRenderWorkerDetails([ + ["State", "active"], + ["Runtime", "node"], + ]), + ).toBe(" State active\n Runtime node\n"); + }); + + it("drops rows whose value is empty", () => { + expect( + legacyRenderWorkerDetails([ + ["State", "active"], + ["Image", ""], + ]), + ).toBe(" State active\n"); + }); + + // Several reported fields are optional in the API contract, so a worker can + // answer with nothing worth rendering. Returning "" rather than a bare newline + // keeps the caller from printing an empty block under its headline. + it("renders nothing at all when every value is empty", () => { + expect( + legacyRenderWorkerDetails([ + ["Image", ""], + ["URL", ""], + ]), + ).toBe(""); + }); +}); diff --git a/apps/cli/src/legacy/commands/experimental/workers/workers.output.integration.test.ts b/apps/cli/src/legacy/commands/experimental/workers/workers.output.integration.test.ts new file mode 100644 index 0000000000..bd56fba7b2 --- /dev/null +++ b/apps/cli/src/legacy/commands/experimental/workers/workers.output.integration.test.ts @@ -0,0 +1,40 @@ +import { rmSync } from "node:fs"; +import { describe, expect, it } from "@effect/vitest"; +import { Effect } from "effect"; +import { LegacyWorkersEnvNotSupportedError } from "./workers.errors.ts"; +import { + makeWorkersProject, + setupLegacyWorkers, +} from "../../../../../tests/helpers/legacy-workers.ts"; +import { legacyEmitWorkersMachineOutput } from "./workers.output.ts"; + +/** + * Every workers command refuses `-o env` up front, before it touches the + * network, so the encoder's own env branch is a backstop rather than a path a + * user reaches. It is worth pinning anyway: a new command that forgets the + * refusal must not silently emit TOML under a flag that asked for env — it + * raises the same refusal instead. + */ +describe("legacyEmitWorkersMachineOutput", () => { + it.live("refuses -o env rather than falling through to the TOML encoder", () => { + const created = makeWorkersProject({ "supabase/config.toml": `project_id = "demo"\n` }); + const { layer, out } = setupLegacyWorkers({ + workdir: created.dir, + goOutput: "env", + routes: {}, + }); + + return Effect.gen(function* () { + const error = yield* legacyEmitWorkersMachineOutput({ + project_ref: "demo", + workers: [], + }).pipe(Effect.flip); + + expect(error).toBeInstanceOf(LegacyWorkersEnvNotSupportedError); + expect(out.stdoutText).toBe(""); + }).pipe( + Effect.provide(layer), + Effect.ensuring(Effect.sync(() => rmSync(created.dir, { recursive: true, force: true }))), + ); + }); +}); diff --git a/apps/cli/src/legacy/commands/experimental/workers/workers.output.ts b/apps/cli/src/legacy/commands/experimental/workers/workers.output.ts index 3bf81b7c4c..95e83127dc 100644 --- a/apps/cli/src/legacy/commands/experimental/workers/workers.output.ts +++ b/apps/cli/src/legacy/commands/experimental/workers/workers.output.ts @@ -12,8 +12,8 @@ import { LegacyWorkersEnvNotSupportedError } from "./workers.errors.ts"; * machine-readable. * * The struct-shaped encoders elsewhere reproduce a payload shape their command - * already shipped. `workers` has none to match, so it serialises through the - * generic encoders and shapes its payload as the command reads best. + * is required to match. `workers` has none, so it serialises through the generic + * encoders and shapes its payload as the command reads best. * * Returns whether it emitted anything, so the caller can skip its text * rendering — `output.success` writes to stdout in text mode and would corrupt @@ -108,6 +108,13 @@ export const legacyRejectWorkersEnvOutput = Effect.fnUntraced(function* () { * * Keyed off the flag rather than the resolved ref: when the link supplied it, * appending it again is noise on a command that already resolves correctly. + * + * An empty `--project-ref ""` counts as "not supplied", the same reading + * `LegacyProjectRefResolver` gives it before falling back to the environment or + * the linked-project file. Carrying it through would suggest a command ending + * in a valueless `--project-ref`, which cannot be pasted back. */ export const legacyWorkersProjectRefSuffix = (projectRef: Option.Option): string => - Option.isSome(projectRef) ? ` --project-ref ${projectRef.value}` : ""; + Option.isSome(projectRef) && projectRef.value.length > 0 + ? ` --project-ref ${projectRef.value}` + : ""; diff --git a/apps/cli/src/legacy/commands/experimental/workers/workers.output.unit.test.ts b/apps/cli/src/legacy/commands/experimental/workers/workers.output.unit.test.ts new file mode 100644 index 0000000000..861459564c --- /dev/null +++ b/apps/cli/src/legacy/commands/experimental/workers/workers.output.unit.test.ts @@ -0,0 +1,22 @@ +import { Option } from "effect"; +import { describe, expect, it } from "vitest"; +import { legacyWorkersProjectRefSuffix } from "./workers.output.ts"; + +describe("legacyWorkersProjectRefSuffix", () => { + it("carries an explicit --project-ref into the suggestion", () => { + expect(legacyWorkersProjectRefSuffix(Option.some("abcdefghijklmnopqrst"))).toBe( + " --project-ref abcdefghijklmnopqrst", + ); + }); + + it("adds nothing when the ref came from the link", () => { + expect(legacyWorkersProjectRefSuffix(Option.none())).toBe(""); + }); + + // `--project-ref ""` resolves from the environment or the linked-project file, + // so echoing the flag back would suggest a command ending in a valueless + // `--project-ref` that cannot be pasted and re-run. + it("adds nothing when the flag was supplied empty", () => { + expect(legacyWorkersProjectRefSuffix(Option.some(""))).toBe(""); + }); +}); diff --git a/apps/cli/src/legacy/shared/legacy-upgrade-notice.ts b/apps/cli/src/legacy/shared/legacy-upgrade-notice.ts index 497ecf638d..027ca96ee8 100644 --- a/apps/cli/src/legacy/shared/legacy-upgrade-notice.ts +++ b/apps/cli/src/legacy/shared/legacy-upgrade-notice.ts @@ -18,14 +18,12 @@ import { lastGlobalFlagValue, rootFlagTokens, } from "../../shared/cli/run.ts"; -import { CLI_VERSION } from "../../shared/cli/version.ts"; +import { CLI_UPGRADE_GUIDE_URL, CLI_VERSION } from "../../shared/cli/version.ts"; import { legacyBold, legacyYellow } from "./legacy-colors.ts"; import { parseDotEnv } from "./legacy-dotenv.ts"; import { legacyCandidateDotenvFilenames } from "./legacy-project-environment.ts"; const LATEST_RELEASE_URL = "https://api.github.com/repos/supabase/cli/releases/latest"; -const UPGRADE_GUIDE_URL = - "https://supabase.com/docs/guides/cli/getting-started#updating-the-supabase-cli"; const CACHE_TTL_MS = 10 * 60 * 60 * 1000; /** No Go equivalent (its client sets no timeout); bounds this pre-exit hook's latency. */ const FETCH_TIMEOUT_MS = 3000; @@ -145,7 +143,7 @@ function comparePrerelease(left: string, right: string): number { export function legacyFormatUpgradeNotice(latestTag: string, currentVersion: string): string { return ( `A new version of Supabase CLI is available: ${legacyYellow(latestTag)} (currently installed v${currentVersion})\n` + - `We recommend updating regularly for new features and bug fixes: ${legacyBold(UPGRADE_GUIDE_URL)}` + `We recommend updating regularly for new features and bug fixes: ${legacyBold(CLI_UPGRADE_GUIDE_URL)}` ); } diff --git a/apps/cli/src/shared/cli/version.ts b/apps/cli/src/shared/cli/version.ts index 1e60d4c7d9..04c39c621f 100644 --- a/apps/cli/src/shared/cli/version.ts +++ b/apps/cli/src/shared/cli/version.ts @@ -5,3 +5,11 @@ declare const SUPABASE_CLI_VERSION: string | undefined; export const CLI_VERSION = typeof SUPABASE_CLI_VERSION === "string" ? SUPABASE_CLI_VERSION : "0.0.0-dev"; + +/** + * Where a user goes to get a newer CLI. There is no self-update command, so + * anything telling a user to upgrade has to send them here rather than name an + * invocation. + */ +export const CLI_UPGRADE_GUIDE_URL = + "https://supabase.com/docs/guides/cli/getting-started#updating-the-supabase-cli"; diff --git a/apps/cli/src/shared/workers/worker-package.unit.test.ts b/apps/cli/src/shared/workers/worker-package.unit.test.ts index d95169a2d7..7e53609a95 100644 --- a/apps/cli/src/shared/workers/worker-package.unit.test.ts +++ b/apps/cli/src/shared/workers/worker-package.unit.test.ts @@ -203,9 +203,9 @@ describe("packageWorkerDirectory", () => { expect(result.fileCount).toBe(0); }); - // A file that cannot be read used to be archived as zero bytes, so `push` - // reported success for a deploy that shipped an empty file. Failing is the - // only honest answer: the archive is the application. + // Archiving an unreadable file as zero bytes would report success for a deploy + // carrying an empty file. Failing is the only honest answer: the archive is the + // application. test("fails rather than archiving a file it cannot read as empty", async () => { const unreadable = join(dir, "secret.txt"); writeFileSync(unreadable, "important"); diff --git a/apps/cli/src/shared/workers/workers-api.ts b/apps/cli/src/shared/workers/workers-api.ts index e2ab533fc5..39c5bb2f51 100644 --- a/apps/cli/src/shared/workers/workers-api.ts +++ b/apps/cli/src/shared/workers/workers-api.ts @@ -12,6 +12,7 @@ import { Effect, Option, Schedule, Schema } from "effect"; import * as HttpClient from "effect/unstable/http/HttpClient"; import * as HttpClientError from "effect/unstable/http/HttpClientError"; import * as HttpClientRequest from "effect/unstable/http/HttpClientRequest"; +import { CLI_UPGRADE_GUIDE_URL } from "../cli/version.ts"; import { WorkerBuildTimeoutError, WorkersApiNetworkError, @@ -193,7 +194,7 @@ const decodeBody = ( new WorkersApiUnexpectedStatusError({ status, detail: `The Workers API returned a response this CLI could not read while trying to ${operation}: ${error.message}.`, - suggestion: "Update the CLI with `supabase update`, then retry.", + suggestion: `Update the CLI, then retry: ${CLI_UPGRADE_GUIDE_URL}`, }), ), ); @@ -453,6 +454,13 @@ export const awaitWorkerBuild = Effect.fnUntraced(function* ( readonly retrySchedule?: Schedule.Schedule; /** Called with each poll's result, for progress reporting. */ readonly onPoll?: (worker: WorkerRecord) => Effect.Effect; + /** + * ` --project-ref ` to append to the suggestion below, when the caller + * reached this project through the flag rather than the link. The suggestion + * is copy-pasted verbatim, so dropping it re-resolves against whatever this + * checkout happens to be linked to. + */ + readonly refSuffix?: string; } = {}, ) { const poll = Effect.gen(function* () { @@ -483,7 +491,7 @@ export const awaitWorkerBuild = Effect.fnUntraced(function* ( return yield* Effect.fail( new WorkerBuildTimeoutError({ detail: `"${name}" was still building when this command stopped waiting.`, - suggestion: `Check on it with \`supabase experimental workers status ${name}\`.`, + suggestion: `Check on it with \`supabase experimental workers status ${name}${options.refSuffix ?? ""}\`.`, }), ); } diff --git a/apps/cli/src/shared/workers/workers.errors.ts b/apps/cli/src/shared/workers/workers.errors.ts index 44826c9315..eda518866c 100644 --- a/apps/cli/src/shared/workers/workers.errors.ts +++ b/apps/cli/src/shared/workers/workers.errors.ts @@ -21,6 +21,22 @@ export class InvalidWorkerNameError extends Data.TaggedError("InvalidWorkerNameE } } +/** + * A bare `new` had no name to scaffold under, and nowhere to ask for one. + * + * The name is the one input this command cannot default — it is the directory, + * the `config.toml` key and the hostname all at once — so with `-o` in force or + * no interactive terminal there is nothing to do but say so. + */ +export class MissingWorkerNameError extends Data.TaggedError("MissingWorkerNameError")<{ + readonly detail: string; + readonly suggestion: string; +}> { + get [ErrorActionabilityId](): CliErrorActionabilityDeclaration { + return actionability.provideFlags; + } +} + /** * A symlink in the worker source points outside the build context. * diff --git a/apps/cli/tests/helpers/mocks.ts b/apps/cli/tests/helpers/mocks.ts index 6e766d28c2..02bec33028 100644 --- a/apps/cli/tests/helpers/mocks.ts +++ b/apps/cli/tests/helpers/mocks.ts @@ -283,6 +283,10 @@ export function mockOutput( } | undefined; }> = []; + const promptTextCalls: Array<{ + message: string; + opts?: { defaultValue?: string; validate?: (v: string) => string | undefined }; + }> = []; const promptTextResponses = [...(opts.promptTextResponses ?? [])]; const promptSelectResponses = [...(opts.promptSelectResponses ?? [])]; const promptPasswordResponses = [...(opts.promptPasswordResponses ?? [])]; @@ -387,10 +391,11 @@ export function mockOutput( promptText: (() => { let callCount = 0; return ( - _msg: string, + message: string, options?: { defaultValue?: string; validate?: (v: string) => string | undefined }, ) => { callCount++; + promptTextCalls.push({ message, opts: options }); // Exercise the validate callback to cover both branches (line 140) if (options?.validate) { options.validate(""); // truthy branch: returns error message @@ -451,6 +456,7 @@ export function mockOutput( events, promptConfirmCalls, promptSelectCalls, + promptTextCalls, rawChunks, get stdoutText() { return rawChunks