From a74e3f9555e047675f436f34e6853be6ca8887a0 Mon Sep 17 00:00:00 2001 From: Matt Johnston Date: Sat, 29 Aug 2026 00:11:51 -0300 Subject: [PATCH 1/3] feat(workers): bring the command family's output onto one shape The workers commands each grew their own way of saying "here is what happened" and "here is what to run next". This settles them on the shapes the rest of the legacy shell already uses, with no change to what any command does. - "What to run next" lines in `new`, `push`, `delete` and `status` move to `emitSuccessTrailer`, the way `stop`, `bootstrap`, `migration repair` and `gen signing-key` already emit theirs: printed once at the end of the run rather than inline, so a multi-worker push does not bury each worker's hint under the next worker's output. The commands within them are aqua'd, as every other follow-up hint in this shell writes them. - `list`'s two advisories take the yellow `WARNING:` prefix and the two-line consequence shape `start`'s Docker notice uses. Each was one long sentence that re-flowed at a different width under a table that lines its columns up. - `list` drops the URL column. Every worker's URL is the same host and prefix with the name on the end, and carrying it pushed the table past 130 columns for one derivable field, since `renderGlamourTable` sizes to the widest cell and never wraps. `status` still renders it vertically, and every machine format still carries `url` per worker. - `push` counts its per-worker announcements (`Deploying Worker 1/2:`) and closes a multi-worker run with a summary line. Each worker takes minutes; the name alone said nothing about how much of the run was left. - `push` names the workers a failed run never attempted. The loop stops at the first failure and the error only names the worker that broke, leaving the rest to be reconstructed from argument order. On stderr in every format, machine ones included: that run is a CI run. - Both of `push`'s retry suggestions carry an explicit `--project-ref` when the flag supplied the ref, via the `legacyWorkersProjectRefSuffix` helper `status` and `delete` already use. A suggestion is copy-pasted verbatim, so one that dropped it re-resolved against whatever this checkout was linked to. Adds unit coverage for `legacyRenderWorkerDetails`'s padding and empty-row dropping, and pins the shared `-o env` refusal so a new command that forgets its own up-front check cannot silently emit TOML instead. --- .../workers/delete/SIDE_EFFECTS.md | 16 +- .../workers/delete/delete.handler.ts | 8 +- .../workers/delete/delete.integration.test.ts | 60 +++- .../experimental/workers/list/SIDE_EFFECTS.md | 4 + .../experimental/workers/list/list.handler.ts | 42 ++- .../workers/list/list.integration.test.ts | 84 ++++- .../experimental/workers/new/new.handler.ts | 19 +- .../workers/new/new.integration.test.ts | 29 +- .../experimental/workers/push/SIDE_EFFECTS.md | 11 + .../experimental/workers/push/push.handler.ts | 60 +++- .../workers/push/push.integration.test.ts | 338 +++++++++++++++++- .../workers/status/SIDE_EFFECTS.md | 16 +- .../workers/status/status.handler.ts | 10 +- .../workers/status/status.integration.test.ts | 29 +- .../workers/workers.format.unit.test.ts | 34 ++ .../workers.output.integration.test.ts | 40 +++ apps/cli/src/shared/workers/workers-api.ts | 9 +- 17 files changed, 756 insertions(+), 53 deletions(-) create mode 100644 apps/cli/src/legacy/commands/experimental/workers/workers.format.unit.test.ts create mode 100644 apps/cli/src/legacy/commands/experimental/workers/workers.output.integration.test.ts 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..0b0d8cfc55 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,11 @@ 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_*` | 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 | 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..f767f9d4ac 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 { @@ -50,7 +51,7 @@ import type { LegacyWorkersDeleteFlags } from "./delete.command.ts"; * stdout, so merely redirecting output would otherwise delete unattended. This * refuses instead, and says which flag would have authorised it. */ -export const legacyWorkersDelete = Effect.fn("legacy.experimental.workers.delete")(function* ( +export const legacyWorkersDelete = Effect.fn("legacy.workers.delete")(function* ( flags: LegacyWorkersDeleteFlags, ) { const output = yield* Output; @@ -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..d8ce622e9f 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,7 +72,8 @@ 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))); }); @@ -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. 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..6b0dbfa6e6 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,11 +93,10 @@ 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 ?? "-", ]; } -export const legacyWorkersList = Effect.fn("legacy.experimental.workers.list")(function* ( +export const legacyWorkersList = Effect.fn("legacy.workers.list")(function* ( flags: LegacyWorkersListFlags, ) { const output = yield* Output; @@ -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). The + // single long sentence each of these used to be re-flowed 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..a5370c3226 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,6 +445,29 @@ 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, 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 ee353babda..8f6276a8e9 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, @@ -67,7 +69,7 @@ function defaultFirst(values: ReadonlyArray, defaultValue: T): Array { * whichever flag asked for it. * * `output.interactive` only tracks *stdout*, so on its own it still let - * `printf 'api\n' | supabase workers new` feed the pipe straight into the name + * `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. @@ -83,7 +85,7 @@ const canPromptFor = Effect.fnUntraced(function* (machineOutput: boolean) { * * 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 workers new` asks rather than failing the parse. The prompt + * `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. @@ -116,7 +118,7 @@ const resolveName = Effect.fnUntraced(function* (options: { return yield* Effect.fail( new MissingWorkerNameError({ detail: "Worker name is required in non-interactive mode.", - suggestion: "Pass a worker name, for example `supabase workers new api`.", + suggestion: "Pass a worker name, for example `supabase experimental workers new api`.", }), ); }); @@ -190,7 +192,7 @@ const destinationIsFree = Effect.fnUntraced(function* (target: string) { return entries.length === 0; }); -export const legacyWorkersNew = Effect.fn("legacy.experimental.workers.new")(function* ( +export const legacyWorkersNew = Effect.fn("legacy.workers.new")(function* ( flags: LegacyWorkersNewFlags, ) { const fs = yield* FileSystem.FileSystem; @@ -328,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], @@ -336,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 a9fbf2872a..8bd78bdb27 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 @@ -67,7 +67,8 @@ 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", () => { @@ -119,7 +120,7 @@ describe("legacy workers new", () => { { 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 workers new`: stdout is still a terminal, so + // `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 } }, @@ -527,6 +528,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 }); 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..bf6c06164c 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`. @@ -393,7 +423,7 @@ const deployOneWorker = Effect.fnUntraced(function* (input: { * the run, because a build that failed is usually the thing to fix before * spending minutes on the rest. */ -export const legacyWorkersPush = Effect.fn("legacy.experimental.workers.push")(function* ( +export const legacyWorkersPush = Effect.fn("legacy.workers.push")(function* ( flags: LegacyWorkersPushFlags, options: { readonly pollSchedule?: Schedule.Schedule; @@ -439,26 +469,46 @@ 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) { + for (const [index, name] of names.entries()) { if (names.length > 1 && !machineOutput) { // 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. + 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..2fd22f494a 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 @@ -143,6 +156,7 @@ describe("legacy workers push", () => { expect(out.stdoutText).toContain("Deployed Worker api"); expect(out.stdoutText).toContain("Runtime"); expect(out.stdoutText).toContain(`https://${WORKERS_PROJECT_REF}.supabase.co/workers/v1/api`); + expect(out.stdoutText).toContain("v1"); }).pipe(Effect.provide(layer), Effect.ensuring(Effect.sync(repo.cleanup))); }); @@ -193,6 +207,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 +351,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 +434,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 +445,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 +542,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 +841,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 +938,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 +1034,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 }); 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..47680c8daa 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,11 @@ 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 | 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 | 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..2ea6b2ddf9 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, @@ -30,7 +32,7 @@ import type { LegacyWorkersStatusFlags } from "./status.command.ts"; * scrolled away, plus the live instance tally, which is the only place it is * available — the list endpoint stays free of per-worker backend calls. */ -export const legacyWorkersStatus = Effect.fn("legacy.experimental.workers.status")(function* ( +export const legacyWorkersStatus = Effect.fn("legacy.workers.status")(function* ( flags: LegacyWorkersStatusFlags, ) { const output = yield* Output; @@ -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/shared/workers/workers-api.ts b/apps/cli/src/shared/workers/workers-api.ts index e2ab533fc5..4bdec8bfc4 100644 --- a/apps/cli/src/shared/workers/workers-api.ts +++ b/apps/cli/src/shared/workers/workers-api.ts @@ -453,6 +453,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 +490,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 ?? ""}\`.`, }), ); } From 548a185b205b73f2f698c8c4b54ebd588c97a6dd Mon Sep 17 00:00:00 2001 From: Matt Johnston Date: Sat, 29 Aug 2026 00:12:27 -0300 Subject: [PATCH 2/3] test(cli): guard every legacy boolean flag against a required default MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 has to be closed off with `Flag.withDefault(false)` or `Flag.optional`, and nothing in the existing suites notices when one is not. 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 exactly the invocation no handler test makes. So this walks the whole legacy command tree, including global flags, and asserts every boolean param carries a default or is optional. It reads the primitive kind through `Primitive.getTypeName` rather than `_tag`, since this repo forbids inspecting effect's runtime representation in tests as well as in source. --- .../legacy-boolean-flag-defaults.unit.test.ts | 71 +++++++++++++++++++ 1 file changed, 71 insertions(+) create mode 100644 apps/cli/src/legacy/cli/legacy-boolean-flag-defaults.unit.test.ts 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([]); + }); +}); From 1d7e06b6a4f07b15589acbd68584f5a7d1a3e643 Mon Sep 17 00:00:00 2001 From: Matt Johnston Date: Mon, 31 Aug 2026 19:42:13 -0300 Subject: [PATCH 3/3] chore(workers): describe behaviour rather than its history in comments Comments explaining why code is shaped a certain way now state the constraint directly instead of narrating what an earlier version did. The reasoning is unchanged; only the framing is. --- .../workers/delete/delete.integration.test.ts | 12 ++++++------ .../experimental/workers/list/list.handler.ts | 6 +++--- .../workers/list/list.integration.test.ts | 4 ++-- .../workers/push/push.integration.test.ts | 8 ++++---- 4 files changed, 15 insertions(+), 15 deletions(-) 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 d8ce622e9f..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 @@ -77,9 +77,9 @@ describe("legacy workers delete", () => { }).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", () => { @@ -328,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({ @@ -662,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/list.handler.ts b/apps/cli/src/legacy/commands/experimental/workers/list/list.handler.ts index 6b0dbfa6e6..5ab6fb40a0 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 @@ -198,9 +198,9 @@ export const legacyWorkersList = Effect.fn("legacy.workers.list")(function* ( // // 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). The - // single long sentence each of these used to be re-flowed differently at - // every terminal width, right under a table that lines its columns up. + // 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); 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 a5370c3226..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 @@ -470,8 +470,8 @@ describe("legacy workers list", () => { // `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/push/push.integration.test.ts b/apps/cli/src/legacy/commands/experimental/workers/push/push.integration.test.ts index 2fd22f494a..85184aeef7 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 @@ -1159,8 +1159,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": "" }); @@ -1216,8 +1216,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() });