feat(cli): add supabase workers list, status and delete - #6263
feat(cli): add supabase workers list, status and delete#6263johnstonmatt wants to merge 1 commit into
Conversation
johnstonmatt
changed the title
c5f5bb7 to
6710630
Compare
6710630 to
4745b37
Compare
c4bae5d to
ae845d0
Compare
ae845d0 to
79874d9
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ae845d0021
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Supabase CLI previewnpx --yes https://pkg.pr.new/supabase/cli/supabase@6dbbaf7e45537285870aafcfb407c6acb153cc96Preview package for commit |
79874d9 to
460f176
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 460f176452
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…lly there Addresses the thirteen review findings on #6263. `status` emitted its payload twice in structured mode. `output.success` was called before the format branch and again inside it, and the JSON layer appends each success to stdout — so `JSON.parse(stdout)` failed outright and `stream-json` saw two terminal result events. The bigger theme is these commands stating local facts they had not checked. `legacyDescribeWorker` can always *compute* a source directory, because with no `[workers.<name>]` entry it falls back to the default path — so a worker deployed from somebody else's checkout got a path that looked like fact. `status` reported it as the worker's source, and `delete` claimed to have kept it along with a `config.toml` entry that did not exist, then pointed at a `push` that had no source to deploy. `legacyDescribeWorker` now also answers whether anything local establishes that path, and both commands say only what is true. `list` disagreed with itself and with `push`. The text column read an absent `spec.runtime` on a deployed worker as `dockerfile` — correct, that is what the API's omission means — while the payload fell back to the local config and reported a runtime the deployment had moved off. And the inventory unioned only config entries with API records, so a scaffolded directory with no entry was missing entirely: `list` could answer "No workers found" about a worker a bare `push` would discover and deploy. Both now read the same way, and the deployed-but-unconfigured warning is scoped to rows that are actually deployed. `delete` also prompted when it should not have. `-o json` leaves `output.format` as `text`, so the interactive check passed and the warning and prompt went to the stdout the payload was supposed to own; a machine format is now as non-interactive as a redirected stdout. Its confirmation counted `spec.instances`, the declared target, while calling them "running" — it uses the live tally when the API reports one and says "declared" when it does not, which for a destructive confirmation is the difference that matters. Smaller ones: `-o table` and `-o csv` fell through to the TOML encoder instead of rendering text like every other resource command; project loading, name validation and worker resolution moved inside the finalizers so those failures still flush telemetry; and `status`/`delete` validate against the API's DNS-label rule rather than the local reserved-name rule, since neither writes `[workers.<name>]` and a worker named `root` was visible in `list` but impossible to inspect or remove. The three `SIDE_EFFECTS.md` files now record the telemetry and linked-project writes rather than claiming nothing is written, and the Go-parity framing is out of the docs and the handler comments.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7aef5a8106
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // them; everything that can fail on its own — loading `config.toml`, | ||
| // validating the name, resolving the worker — belongs inside, so those | ||
| // failures still flush telemetry. Same shape as `config/push`. | ||
| const projectRef = yield* resolver.resolve(flags.projectRef); |
There was a problem hiding this comment.
The final tree still resolves the project ref before entering the effect carrying the finalizers, so an unlinked project, malformed ref, ref-file read failure, or failed interactive project selection exits without flushing telemetry. The earlier review prompted moving config loading and name validation inside the wrapper, but this remaining pre-wrapper call is fresh evidence that project-ref failures are still uncovered; wrap resolution in the finalized effect and run the cache only after a ref is available.
AGENTS.md reference: apps/cli/AGENTS.md:L288-L290
Useful? React with 👍 / 👎.
| if (record.buildState === "failed") { | ||
| yield* output.raw(`Fix the issue, then re-run supabase workers push ${name}.\n`); |
There was a problem hiding this comment.
When the API reports both deleting: true and the previous build_state: "failed", the details correctly show the state as deleting but this branch still tells the user to fix the build and push again. Because deletion is asynchronous, following that advice can conflict with the tombstone or recreate a worker the user is removing; only recommend another push when the worker is not deleting.
Useful? React with 👍 / 👎.
| | Path | Format | When | | ||
| | -------------------------------- | ------ | ------------------------------------- | | ||
| | `<workdir>/supabase/config.toml` | TOML | always, for the `[workers.*]` entries | |
There was a problem hiding this comment.
After adding unconfigured directories to the inventory, workers list reads the configured <workdir>/supabase/<workers.root>/ directory and stats each child through legacyDiscoverWorkerNames, but this compatibility checklist still claims that config.toml is the only file-system input. Document the root-directory enumeration and per-entry metadata reads, including the custom [workers] root case, so the primary E2E side-effect input reflects the command's actual filesystem dependency.
AGENTS.md reference: apps/cli/AGENTS.md:L355-L366
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 12ec5199ea
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| runtime: runtimeLabelFor(row), | ||
| size: row.deployed?.spec.size, | ||
| state: stateLabel(row), | ||
| instances: row.deployed?.spec.instances, | ||
| url: row.url, |
There was a problem hiding this comment.
When workers list -o toml includes an undeployed worker or a deployed private worker, these properties place undefined values inside the payload; legacyEmitWorkersMachineOutput then passes the nested object directly to the smol-toml encoder, which cannot represent undefined (the existing handling in push.handler.ts explicitly documents this limitation). The command therefore throws instead of emitting TOML. The status payload has the same problem with optional fields such as state_reason, deleting, and instances_error, so omit optional properties or recursively normalize the payload before TOML encoding.
AGENTS.md reference: apps/cli/AGENTS.md:L445-L449
Useful? React with 👍 / 👎.
…atus Two from the re-review of #6263, the first a regression the previous round introduced. Making `legacyDescribeWorker` confine the configured source was right for `push`, where that directory is what gets packaged and uploaded. It was wrong for `status` and `delete`, which only *report* on local state — so a `source` that no longer resolves inside the project (an in-project directory that became a symlink, say) failed before either API call and left the remote worker un-deletable until the user edited `config.toml` or moved workdir. Deletion never reads that path. `legacyDescribeWorkerForReporting` degrades instead: an unusable source reads the same as having nothing local, which is exactly what the output needs to say. `push` keeps the strict version. The redeploy advice also fired too readily. A retained `config.toml` entry alone made the "Kept" list non-empty, so `delete` recommended `workers push` for a worker whose source directory was gone — the command it names would have failed. The entry is still reported; the advice now waits on the source actually being there.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d2b7f5d359
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const sourceDisplay = | ||
| worker.entry !== undefined || worker.sourceExists | ||
| ? displayPath(project.projectRoot, worker.sourceDir) |
There was a problem hiding this comment.
When a configured source is unusable—for example, an in-project symlink resolving outside the project—legacyDescribeWorkerForReporting returns the worker's default directory while retaining the config entry. This condition consequently reports that default as the source even though the entry actually names a different path. Fresh evidence is the new best-effort fallback at workers.shared.ts:109-120, which creates exactly this entry-plus-fallback combination; omit the source when resolution failed rather than presenting the default path as established local state.
Useful? React with 👍 / 👎.
| `${orphans.join(", ")} ${ | ||
| orphans.length === 1 ? "is" : "are" | ||
| } deployed but absent from supabase/config.toml: pushing from here would have to guess the runtime.\n`, |
There was a problem hiding this comment.
For a deployed worker that has neither a config entry nor a local directory (row.local === false), this says a push merely needs to guess the runtime. In fact, deployOneWorker checks the default source directory before runtime inference and fails with WorkerSourceMissingError, so the suggested diagnosis sends users toward fixing the wrong prerequisite. Split remote-only workers from unconfigured local directories and tell the former that source must be scaffolded or restored.
Useful? React with 👍 / 👎.
|
|
||
| | Method | Path | Auth | Request body | Response (used fields) | | ||
| | -------- | ----------------------------------- | ------------ | ------------ | --------------------------------------- | | ||
| | `GET` | `/v2/projects/{ref}/workers/{name}` | Bearer token | none | `spec.instances` (for the confirmation) | |
There was a problem hiding this comment.
When the single-worker response contains an instance tally, the confirmation uses instances.live, but this checklist records only spec.instances as a consumed response field. That omission hides the live-versus-declared behavior from the compatibility document that drives E2E coverage; include instances.live and its fallback semantics in the response description.
AGENTS.md reference: apps/cli/AGENTS.md:L359-L366
Useful? React with 👍 / 👎.
| detail: `Nothing is deployed for "${name}" in project ${projectRef}.`, | ||
| suggestion: `Deploy it with \`supabase workers push ${name}\`.`, |
There was a problem hiding this comment.
root
When status root receives a 404, this branch now accepts the API-valid name but recommends supabase workers push root; that command still calls legacyValidateWorkerName and rejects root because it cannot be represented in local TOML. Fresh evidence is the API-only validator introduced for status and delete, which makes this previously unreachable error branch available for root; provide recovery guidance that does not direct users to a command guaranteed to fail. The same suggestion is emitted by the corresponding delete branch.
Useful? React with 👍 / 👎.
![depthfirst-app[bot]](https://avatars.githubusercontent.com/in/1021002?s=60&v=4){kind=small}
| ["Source", sourceDisplay ?? ""], | ||
| ]; | ||
|
|
||
| yield* output.raw(legacyRenderWorkerDetails(details)); |
There was a problem hiding this comment.
⚪ Severity: LOW
The API-controlled state_reason is inserted into details and sent through output.raw without terminal escaping. A worker owner or collaborator who can influence a build failure reason can make workers status emit ANSI/OSC control sequences, spoofing status output or manipulating terminal state when an operator inspects it.
Helpful? Add 👍 / 👎
💡 Fix Suggestion
Suggestion: Strip ANSI/OSC terminal control sequences from API-controlled string values before they are passed to legacyRenderWorkerDetails and written to the terminal via output.raw. Replace the single yield* call with a sanitization step that removes escape sequences from every detail row value. The same concern also applies to record.instancesError interpolated directly into output.raw on line 135, which should receive the same stripping treatment. Since no strip-ansi package is present in the project dependencies, use an inline regex that covers the major attack surfaces: CSI sequences (\x1B[…m), OSC sequences (\x1B]…\x07), and bare two-byte ESC sequences.
⚠️ Experimental Feature: This code suggestion is automatically generated. Please review carefully.
| yield* output.raw(legacyRenderWorkerDetails(details)); | |
| const safeDetails = details.map(([k, v]) => [k, v.replace(/\x1B(?:\[[0-?]*[ -\/]*[@-~]|\][^\x07]*\x07|[^[\]])/g, "")] as const); | |
| yield* output.raw(legacyRenderWorkerDetails(safeDetails)); |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7c5677e3f3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| [ | ||
| "Instances", | ||
| record.instances !== undefined | ||
| ? `${record.instances.ready}/${record.spec.instances} ready, ${record.instances.live} live, ${record.instances.stale} stale` |
There was a problem hiding this comment.
When the API's instance snapshot differs from the desired spec during scaling or reconciliation, this combines instances.ready with spec.instances even though the same snapshot supplies instances.declared. That can display an inconsistent readiness fraction, such as more ready instances than the denominator; use record.instances.declared so all values in the tally come from the same snapshot.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 74cc753c88
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const projectRef = yield* resolver.resolve(flags.projectRef); | ||
|
|
||
| yield* Effect.gen(function* () { | ||
| const project = yield* legacyLoadWorkersProject(); |
There was a problem hiding this comment.
When supabase/config.toml is malformed or its [workers] section is invalid, this mandatory load fails before either API request, even with an explicit valid --project-ref. The config is used only to describe optional local files that deletion deliberately does not modify, so this can strand a deployed worker until the user repairs unrelated local configuration; load the reporting metadata best-effort or perform it independently of the remote delete.
Useful? React with 👍 / 👎.
|
|
||
| | Path | Format | When | | ||
| | ---------------------------------------- | ---------- | ----------------------------------------------------------------------------------------------------------- | | ||
| | `<workdir>/supabase/config.toml` | TOML | always, for the `[workers.*]` entries | |
There was a problem hiding this comment.
All three new handlers call resolver.resolve, but their side-effect documents omit that, when --project-ref is absent, resolution consumes SUPABASE_PROJECT_ID, reads <workdir>/supabase/.temp/project-ref, and can call GET /v1/projects for the interactive project picker when neither source resolves. Add these conditional file, environment, and API inputs to each checklist so the documented compatibility contract and its E2E inputs match actual command startup.
AGENTS.md reference: apps/cli/AGENTS.md:L359-L366
Useful? React with 👍 / 👎.
| const fetching = yield* output.task("Fetching worker..."); | ||
| const found = yield* getWorker(api, projectRef, name).pipe( | ||
| Effect.tapError(() => fetching.fail()), | ||
| ); |
There was a problem hiding this comment.
For an OAuth credential granted edge_functions:write without edge_functions:read, this preliminary GET returns 403 and the DELETE is never attempted, including with --yes. The generated API contract assigns the retrieval endpoint edge_functions:read (packages/api/src/generated/openapi.json:12408-12414) but the deletion endpoint only edge_functions:write (packages/api/src/generated/openapi.json:12488-12494), so the command imposes a permission the requested operation does not require; skip this lookup when confirmation metadata is unnecessary or degrade gracefully when it is unavailable.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c70828770e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const deleting = yield* output.task("Deleting worker..."); | ||
| yield* deleteWorker(api, projectRef, name).pipe(Effect.tapError(() => deleting.fail())); |
There was a problem hiding this comment.
-o env before deleting the worker
With --yes -o env, the handler performs the DELETE here and only afterward reaches legacyEmitWorkersMachineOutput, which raises LegacyWorkersEnvNotSupportedError. The worker is therefore deleted even though the command exits with an error and emits no success payload; validate the requested output format before making any API calls.
AGENTS.md reference: apps/cli/AGENTS.md:L300-L300
Useful? React with 👍 / 👎.
| const projectRef = yield* resolver.resolve(flags.projectRef); | ||
|
|
||
| yield* Effect.gen(function* () { | ||
| const project = yield* legacyLoadWorkersProject(); |
There was a problem hiding this comment.
When an explicit valid --project-ref is provided but the local supabase/config.toml is malformed, this mandatory load fails before the GET request. Local configuration is used only to add the optional source-directory detail, so a broken or unrelated local checkout prevents inspecting an otherwise accessible remote worker; load this reporting metadata best-effort rather than making it a prerequisite.
Useful? React with 👍 / 👎.
| // still let the warning and the prompt run — onto the stdout the user had | ||
| // asked to carry a payload. A machine format is as non-interactive as a | ||
| // redirected stdout, whichever flag asked for it. | ||
| if (output.format !== "text" || machineOutput || !output.interactive) { |
There was a problem hiding this comment.
🟡 Severity: MEDIUM
The non-interactive safeguard checks only whether stdout is a TTY. output.promptText still reads stdin, so with a TTY stdout but redirected or piped stdin, input containing the worker name satisfies confirmation and reaches deleteWorker, enabling unattended destructive deletion without --yes.
Helpful? Add 👍 / 👎
💡 Fix Suggestion
Suggestion: The root cause is that output.interactive in apps/cli/src/shared/output/output.layer.ts (line 205) is set to tty.stdoutIsTty only, ignoring whether stdin is a TTY. This means that when stdout is a terminal but stdin is piped, output.interactive is true, the guard at line 93 of delete.handler.ts passes, and the confirmation prompt (output.promptText) can be satisfied by piped stdin input — enabling unattended destructive deletion without --yes.
The fix should be applied in apps/cli/src/shared/output/output.layer.ts at line 205. Change:
interactive: tty.stdoutIsTty,
to:
interactive: tty.stdoutIsTty && tty.stdinIsTty,
The Tty service already exposes stdinIsTty alongside stdoutIsTty, so this is a minimal, safe change that fixes the issue consistently for all commands that rely on output.interactive, not just the workers delete command. Additionally, update the doc comment in delete.handler.ts (lines 44–45) that states 'interactive tracks stdout' to reflect that it now tracks both stdin and stdout.
…lly there Addresses the thirteen review findings on #6263. `status` emitted its payload twice in structured mode. `output.success` was called before the format branch and again inside it, and the JSON layer appends each success to stdout — so `JSON.parse(stdout)` failed outright and `stream-json` saw two terminal result events. The bigger theme is these commands stating local facts they had not checked. `legacyDescribeWorker` can always *compute* a source directory, because with no `[workers.<name>]` entry it falls back to the default path — so a worker deployed from somebody else's checkout got a path that looked like fact. `status` reported it as the worker's source, and `delete` claimed to have kept it along with a `config.toml` entry that did not exist, then pointed at a `push` that had no source to deploy. `legacyDescribeWorker` now also answers whether anything local establishes that path, and both commands say only what is true. `list` disagreed with itself and with `push`. The text column read an absent `spec.runtime` on a deployed worker as `dockerfile` — correct, that is what the API's omission means — while the payload fell back to the local config and reported a runtime the deployment had moved off. And the inventory unioned only config entries with API records, so a scaffolded directory with no entry was missing entirely: `list` could answer "No workers found" about a worker a bare `push` would discover and deploy. Both now read the same way, and the deployed-but-unconfigured warning is scoped to rows that are actually deployed. `delete` also prompted when it should not have. `-o json` leaves `output.format` as `text`, so the interactive check passed and the warning and prompt went to the stdout the payload was supposed to own; a machine format is now as non-interactive as a redirected stdout. Its confirmation counted `spec.instances`, the declared target, while calling them "running" — it uses the live tally when the API reports one and says "declared" when it does not, which for a destructive confirmation is the difference that matters. Smaller ones: `-o table` and `-o csv` fell through to the TOML encoder instead of rendering text like every other resource command; project loading, name validation and worker resolution moved inside the finalizers so those failures still flush telemetry; and `status`/`delete` validate against the API's DNS-label rule rather than the local reserved-name rule, since neither writes `[workers.<name>]` and a worker named `root` was visible in `list` but impossible to inspect or remove. The three `SIDE_EFFECTS.md` files now record the telemetry and linked-project writes rather than claiming nothing is written, and the Go-parity framing is out of the docs and the handler comments.
…atus Two from the re-review of #6263, the first a regression the previous round introduced. Making `legacyDescribeWorker` confine the configured source was right for `push`, where that directory is what gets packaged and uploaded. It was wrong for `status` and `delete`, which only *report* on local state — so a `source` that no longer resolves inside the project (an in-project directory that became a symlink, say) failed before either API call and left the remote worker un-deletable until the user edited `config.toml` or moved workdir. Deletion never reads that path. `legacyDescribeWorkerForReporting` degrades instead: an unusable source reads the same as having nothing local, which is exactly what the output needs to say. `push` keeps the strict version. The redeploy advice also fired too readily. A retained `config.toml` entry alone made the "Kept" list non-empty, so `delete` recommended `workers push` for a worker whose source directory was gone — the command it names would have failed. The entry is still reported; the advice now waits on the source actually being there.
c708287 to
d12df47
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d12df47bcb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| | Path | Format | When | | ||
| | ---------------------------------------- | ---------- | ----------------------------------------------------------------------------------------------------------- | | ||
| | `<workdir>/supabase/config.toml` | TOML | always, to report the worker's source directory | |
There was a problem hiding this comment.
When status runs, legacyDescribeWorkerForReporting canonicalizes the configured/default source path and stats that directory, and those results determine whether the Source row is emitted. This checklist therefore omits filesystem inputs that affect observable output; add the source path and ancestor metadata reads here, and make the corresponding correction in delete/SIDE_EFFECTS.md, which uses the same reporting helper.
AGENTS.md reference: apps/cli/AGENTS.md:L359-L366
Useful? React with 👍 / 👎.
| // 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 workers push ${name}.\n`); |
There was a problem hiding this comment.
When deletion is run with --project-ref from a checkout linked to a different project—or from an unlinked checkout—this printed command drops the resolved ref. Following the suggested command therefore either fails project resolution or redeploys the worker into the checkout's linked project instead of the project it was just deleted from; include --project-ref ${projectRef} in the guidance.
Useful? React with 👍 / 👎.
d12df47 to
640c103
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 640c1032a9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return yield* Effect.fail( | ||
| new WorkerDeleteConfirmationRequiredError({ | ||
| detail: `Deleting "${name}" from project ${projectRef} needs confirmation, and there is no interactive terminal to ask on.`, | ||
| suggestion: `Re-run \`supabase workers delete ${name} --yes\` to confirm without a prompt.`, |
There was a problem hiding this comment.
When deletion is attempted with an explicit --project-ref from a checkout linked to another project, copying this suggested retry drops the selected ref; workers delete <name> --yes then resolves the checkout's linked project and can irreversibly delete a same-named worker there. Include --project-ref ${projectRef} here and in the confirmation-mismatch retry guidance.
Useful? React with 👍 / 👎.
| yield* output.raw(`Instance counts could not be read: ${record.instancesError}\n`, "stderr"); | ||
| } | ||
| if (record.buildState === "failed") { | ||
| yield* output.raw(`Fix the issue, then re-run supabase workers push ${name}.\n`); |
There was a problem hiding this comment.
When status is run with --project-ref from an unlinked checkout or one linked to a different project, this recovery command omits the resolved ref. Following it therefore either fails project resolution or deploys the repaired worker to the checkout's linked project rather than the project whose failed build was inspected; include --project-ref ${projectRef} in the advice.
Useful? React with 👍 / 👎.
| No custom events — only the `cli_command_executed` that the instrumentation | ||
| wrapper emits for every command. |
There was a problem hiding this comment.
This checklist ends after telemetry without documenting the command's stdout/stderr contract, and the new status and delete checklists have the same omission. Add the template's Output section for text, JSON, stream-JSON, and the supported legacy -o formats so the compatibility checklist that drives E2E coverage records the handlers' observable output behavior.
AGENTS.md reference: apps/cli/AGENTS.md:L366-L366
Useful? React with 👍 / 👎.
| // still let the warning and the prompt run — onto the stdout the user had | ||
| // asked to carry a payload. A machine format is as non-interactive as a | ||
| // redirected stdout, whichever flag asked for it. | ||
| if (output.format !== "text" || machineOutput || !output.interactive) { |
There was a problem hiding this comment.
When stdout is still a TTY but stdin is piped, such as printf 'api\n' | supabase workers delete api, output.interactive remains true because it only reflects stdout. This guard therefore permits promptText to consume the piped worker name and proceeds with the DELETE without --yes, contrary to the command's non-interactive safety rule; also require Tty.stdinIsTty before entering the prompt.
Useful? React with 👍 / 👎.
The three read-side verbs, sharing one API seam and one description of what is actually there. `list` is every worker in the project, deployed or not, rendered through `renderGlamourTable` so it sits beside `functions list` and `projects list` looking like them. Its inventory is the union of three sources, because any one alone misleads: the `[workers.*]` entries in `config.toml`, the directories under `supabase/workers/`, and what the API reports as deployed. Leaving the directories out let `list` answer "No workers found" about a worker a bare `push` would discover and deploy. A worker with no deployment shows as `not deployed`; a deployment with no local counterpart is called out on stderr, since pushing it from here would have to guess its runtime — stderr so the note never lands inside a `-o` payload. The runtime column only claims a runtime it can support, and text and payload agree on it: the API omits `spec.runtime` for a context-only build, so on a deployed worker its absence does mean `dockerfile`, while for one never deployed there is nothing to infer from and the column says so rather than falling back to a local entry the deployment may have moved off. The list endpoint makes no per-worker backend call, so the instance column shows the declared count and `status` is where the live one lives. `status` is one worker in detail: the size, access, image and URL a `push` printed once and then scrolled away, plus the live instance tally. The deployed spec is the truth here, not `config.toml` — a worker deployed from its own Dockerfile carries no `spec.runtime`, and letting a stale local entry answer instead would report a runtime that is not what is running. When the instance read-through fails the API says so rather than returning counts, and that failure is reported on stderr instead of printing numbers it does not have. A failed build points at the retry, with the reason the API gave. `delete` removes a worker from the linked project; its instances and image are torn down asynchronously. Whether it exists is asked of the API, never of a local directory, so `status` and `delete` answer that question the same way. Being the irreversible verb, an interactive session has to type the worker's name back before anything happens — the same confirm-by-typing pattern as GitHub's own repository deletion, rather than a bare y/n that is too easy to reflexively accept. `--yes` skips it for scripts, as does a non-interactive session or a machine output format, where there is nowhere to ask. The confirmation counts the live tally when the API reports one and says "declared" when it does not, which for a destructive prompt is the difference that matters. What it does not remove is worth saying out loud, so it says it: the worker's directory and its `config.toml` entry stay on disk — and the redeploy advice waits on that source actually being there, rather than naming a `push` that would fail. None of the three state a local fact it has not checked. `legacyDescribeWorker` can always *compute* a source directory, because with no `[workers.<name>]` entry it falls back to the default path, so a worker deployed from somebody else's checkout would otherwise get a path that looks like fact. It answers separately whether anything local establishes that path, and the reporting variant degrades rather than failing: a configured `source` that no longer resolves inside the project reads the same as having nothing local, which is what the output needs to say, and does not leave a remote worker un-deletable until the user edits `config.toml`. `push` keeps the strict version, since there that directory is what gets packaged and uploaded. Shell conventions across the three: `-o table` and `-o csv` render text like every other resource command rather than falling through to the TOML encoder; a machine format is as non-interactive as a redirected stdout, since `-o` leaves `output.format` as `text` and a prompt would land on the stdout the payload owns; project loading, name validation and worker resolution happen inside the finalizers so those failures still flush telemetry; and `status` and `delete` validate names against the API's DNS-label rule rather than any local naming rule, since neither writes `[workers.<name>]` and a worker visible in `list` should not be impossible to inspect or remove.
640c103 to
6dbbaf7
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6dbbaf7e45
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| name: string, | ||
| ) { | ||
| const described = yield* legacyDescribeWorker(project, name).pipe(Effect.option); | ||
| if (described._tag === "Some") { |
There was a problem hiding this comment.
Replace the new described._tag check—and the equivalent info._tag check at line 129—with Option.isSome; otherwise this reporting path is coupled to Effect's internal representation rather than its supported API. The newly added integration-test assertions that inspect error._tag should likewise use public predicates or instanceof.
AGENTS.md reference: AGENTS.md:L169-L176
Useful? React with 👍 / 👎.
| return; | ||
| } | ||
|
|
||
| yield* output.raw(renderGlamourTable([...HEADERS], rows.map(toCells))); |
There was a problem hiding this comment.
⚪ Severity: LOW
The deployed worker's API-provided runtime and size are copied into table cells and emitted through output.raw. A collaborator who can influence worker metadata can inject ANSI/OSC controls into workers list, spoofing inventory rows or altering terminal state when an operator views the output; renderGlamourTable performs no escaping.
Helpful? Add 👍 / 👎
💡 Fix Suggestion
Suggestion: Sanitize API-provided string values before they are passed to renderGlamourTable to remove ANSI/OSC control sequences. The two fields sourced from the API are spec.runtime (returned verbatim in runtimeLabel) and spec.size (returned verbatim in formatApiSize when the regex does not match). The recommended approach is to:
- Extract or reuse a control-character stripping helper (a
stripAnsi-style function already exists in the test helpers atapps/cli/src/tests/helpers/ansi.tsand could be moved to a shared utility). - Apply it to every API-provided cell value inside
toCells— specifically wrapruntimeLabel(row)andformatApiSize(row.deployed.spec.size)with the sanitizer, e.g.stripControl(runtimeLabel(row))andstripControl(formatApiSize(row.deployed.spec.size)). A minimal stripper that covers both ANSI CSI sequences and OSC sequences would be:const stripControl = (s: string) => s.replace(/(\x1b\[[0-9;]*[A-Za-z]|\x1b][^\x07]*(?:\x07|\x1b\\)|[\x00-\x1f\x7f])/g, ""); - Also consider applying the same sanitization to
row.nameandrow.urlsince those too originate from the API.
1 participant
Summary
Completes the command family with the three remaining subcommands:
workers list— this project's workers.workers status— one worker in detail.workers delete— remove a worker, with confirmation.All three reuse the API client and output helpers introduced in #6262, so this is
the smallest layer of the stack.
Stack 4 of 4, on top of
workers push(#6262).Linked issue
FUNC-753 (Linear). Supabase maintainer, exempt from the
open-for-contributionflow.Checklist