feat(workers): bring the command family's output onto one shape - #6389
Merged
Conversation
Makes the `name` argument to `supabase workers new` optional and prompts for it when it is omitted, so a bare `supabase workers new` walks through name, runtime and size rather than failing the parse. The name is the one input this command cannot default — it is the directory, the `[workers.<name>]` key and the hostname all at once. So where the runtime and size prompts fall back to a default when there is nowhere to ask, the name prompt has nothing to fall back to: with `-o json|yaml|toml|env` or no interactive terminal, the command fails with a new `MissingWorkerNameError` pointing at `supabase workers new api`. The prompt validates against everything the command would otherwise refuse a moment later — a non-DNS-label name, and a name `config.toml` already records — so a typo is corrected in place instead of ending the run. That also means the project has to be loaded before the first prompt, and the machine-output check moves up with it: `-o` leaves `output.format` as `text`, and Clack writes its terminal UI to stdout, so a name prompt would land in front of the payload for the same reason the runtime prompt would. The handler's inline name validation is replaced by the shared `legacyValidateWorkerName`, which the rest of the command family already uses, so an explicitly-passed name and a prompted one are refused on identical terms. `mockOutput` now records `promptTextCalls` so tests can assert on the prompt's message and exercise its `validate` callback.
`output.interactive` is derived solely from `tty.stdoutIsTty`, so with stdin piped or redirected and stdout still on a terminal it stayed true. A bare `printf 'api\n' | supabase workers new` therefore opened Clack's name prompt and read the worker name off the pipe instead of taking the documented `MissingWorkerNameError` path — and the runtime and size prompts consumed whatever followed rather than falling back to their defaults. The three resolvers now share one `canPromptFor` decision, made once before the first prompt, which pairs `output.interactive` with `tty.stdinIsTty` the way `workers delete` already guards its confirmation. A prompt is only answerable from a keyboard, so both streams have to be a terminal.
1 task
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: cd157e17bf
ℹ️ 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".
johnstonmatt
added
the
run-ci
Contributor
Supabase CLI previewnpx --yes https://pkg.pr.new/supabase/cli/supabase@5bc206a434c7ce11ab999a4c8712f69d3c53fb1bPreview package for commit |
Makes the `name` argument to `supabase workers new` optional and prompts for it when it is omitted, so a bare `supabase workers new` walks through name, runtime and size rather than failing the parse. The name is the one input this command cannot default — it is the directory, the `[workers.<name>]` key and the hostname all at once. So where the runtime and size prompts fall back to a default when there is nowhere to ask, the name prompt has nothing to fall back to: with `-o json|yaml|toml|env` or no interactive terminal, the command fails with a new `MissingWorkerNameError` pointing at `supabase workers new api`. The prompt validates against everything the command would otherwise refuse a moment later — a non-DNS-label name, and a name `config.toml` already records — so a typo is corrected in place instead of ending the run. That also means the project has to be loaded before the first prompt, and the machine-output check moves up with it: `-o` leaves `output.format` as `text`, and Clack writes its terminal UI to stdout, so a name prompt would land in front of the payload for the same reason the runtime prompt would. The handler's inline name validation is replaced by the shared `legacyValidateWorkerName`, which the rest of the command family already uses, so an explicitly-passed name and a prompted one are refused on identical terms. `mockOutput` now records `promptTextCalls` so tests can assert on the prompt's message and exercise its `validate` callback.
`output.interactive` is derived solely from `tty.stdoutIsTty`, so with stdin piped or redirected and stdout still on a terminal it stayed true. A bare `printf 'api\n' | supabase workers new` therefore opened Clack's name prompt and read the worker name off the pipe instead of taking the documented `MissingWorkerNameError` path — and the runtime and size prompts consumed whatever followed rather than falling back to their defaults. The three resolvers now share one `canPromptFor` decision, made once before the first prompt, which pairs `output.interactive` with `tty.stdinIsTty` the way `workers delete` already guards its confirmation. A prompt is only answerable from a keyboard, so both streams have to be a terminal.
johnstonmatt
force-pushed
the
FUNC-851/general-output-polish
branch
from
August 31, 2026 22:34
cd157e1 to
8275c3f
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8275c3fb1e
ℹ️ 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".
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.
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.
`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.
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.
johnstonmatt
force-pushed
the
FUNC-851/general-output-polish
branch
from
August 31, 2026 22:48
8275c3f to
1d7e06b
Compare
1 task
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1d7e06b6a4
ℹ️ 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 into FUNC-840/select-workers-new-name # Conflicts: # apps/cli/src/legacy/commands/experimental/workers/new/SIDE_EFFECTS.md # apps/cli/src/legacy/commands/experimental/workers/new/new.command.ts # apps/cli/src/legacy/commands/experimental/workers/new/new.handler.ts
`workers` is registered only beneath the `experimental` parent, so the `MissingWorkerNameError` suggestion telling the user to run `supabase workers new api` produced an unknown-command error when copied. Name the real invocation path in the suggestion, and in the handler, doc and test prose that described the piped-stdin case with the same stale path. An assertion on the suggestion keeps the retry path from drifting away from where the command is mounted.
The Workers API decode failure suggested `supabase update`, which is not a command in either shell's root — the CLI has no self-update path, which is why the post-command upgrade notice sends users to the docs instead. Point the suggestion at that same upgrade guide, and hoist the URL from `legacy-upgrade-notice.ts` into `shared/cli/version.ts` so both callers read one constant rather than duplicating the link.
…olish # Conflicts: # apps/cli/src/legacy/commands/experimental/workers/new/new.handler.ts # apps/cli/src/legacy/commands/experimental/workers/new/new.integration.test.ts
The output work reintroduced `legacy.workers.*` as the `Effect.fn` span name for all five worker handlers, undoing the rename that landed with the move under `experimental`. Traces and the NDJSON exporter recorded a command route that no longer exists, so any filter keyed to the current names missed them.
`LegacyProjectRefResolver` treats `--project-ref ""` as absent and falls back to the environment or the linked-project file, but the suffix helper keyed off `Option.isSome` alone. Every suggestion that command emitted ended in a valueless `--project-ref`, which cannot be pasted back and re-run. Match the resolver's reading of the flag instead.
`SIDE_EFFECTS.md` claimed the `Deploying Worker n/N:` announcement was text-only, but the gate read `machineOutput`, which tracks `-o` alone. Under `--output-format json` or `stream-json` the flag stays false and the announcement was written to stderr, contradicting the documented matrix. Gate it on the format too, the way the run's closing summary already is. The "not attempted" report keeps reaching every format: it says what still needs deploying, which is exactly what an unwatched CI run has to know.
Both handlers return at the machine emission or at `output.success`, so the build-retry trailer, the redeploy trailer, and the two stderr notices beside them are only reachable in the text branch. The format matrices claimed "as above" for every structured row, which would drive E2E expectations for output those modes never produce.
…40/select-workers-new-name
…/supabase/cli into FUNC-851/general-output-polish
kanadgupta
reviewed
Sep 3, 2026
kanadgupta
left a comment
kanadgupta
left a comment
Member
There was a problem hiding this comment.
LGTM!
[!NOTE]
This review was drafted by an AI (Claude). Refreshed after the new commits (headb3ea7df57; previously reviewed at1d7e06b6a).What this PR does
Settles the
experimental workersfamily's human output onto the shell's shared shapes: "what to run next" hints innew,push,delete, andstatusmove toemitSuccessTrailer(verified against howstop,bootstrap,migration repair, andgen signing-keyemit theirs — usage, text-mode gating, and stderr flush semantics all match);list's two advisories take theWARNING:two-line shape and the table drops the URL column;pushcounts its per-worker announcements, closes multi-worker runs with a summary, names never-attempted workers on failure, and carries an explicit--project-refinto its retry suggestions. The rest of the diff is tests pinning existing behavior, plus a repo-wide boolean-flag-default guard (own commit).What the new commits addressed
All four previously open issues are fixed, and I verified each in the code:
- Span names restored to
legacy.experimental.workers.*across all five handlers (1f103f99a) — this was my one real concern with the previous revision.Deploying Worker n/N:now gated onoutput.format === "text"as well as-o(d99e6b518), with a clear comment on why both axes are needed and a new integration test pinning json mode; the push SIDE_EFFECTS "text-only" paragraph is now true as written.- Empty
--project-ref ""no longer echoed into retry suggestions (de593920d):legacyWorkersProjectRefSuffixnow mirrors the resolver's empty-means-absent reading, with a unit test. This also resolves what I had flagged as a cross-PR routing — the helper fix landed in this PR.- Status and delete SIDE_EFFECTS matrices corrected (
71af01ded): structured rows now say the trailer and stderr notices are text-only, with an accurate note about the-o pretty/table/csvfall-through. I checked the new rows against the handlers' early returns; they match.All four Codex threads are answered by the author with the fixing commits and can be resolved.
Remaining findings
These commands are experimental, so the bar applied here is: only correctness/data-loss/security issues of real note hold the merge.
Fix before merge: none.
Follow-up candidates (2):
- Vacuous
toContain("v1")assertion inpush.integration.test.ts:159— already implied by the URL asserted on the previous line, and the default stub carries no image version to pin.The repo-wide boolean-flag guard test is solid but unrelated to workers output; as its ownkanad edit: removed this feedback, don't think it's worth separating out into its own PRtest(cli)PR it would land independently of this stack.Does "no behavior change" hold?
Yes, now without caveats for command semantics: the non-test changes are output text, output streams, hint content, and the two output-gating fixes above. The span rename that previously contradicted the claim is reverted.
Overall: 0 fix-before-merge issue(s),
21 follow-up candidate(s).
…tegration.test.ts
pull Bot
pushed a commit
to chizee/cli
that referenced
this pull request
Sep 3, 2026
## Summary Makes the `name` argument to `supabase experimental workers new` optional and prompts for it when it is omitted, so a bare `supabase experimental workers new` walks through name, runtime and size rather than failing the parse. The name is the one input this command cannot default — it is the directory, the `[workers.<name>]` key and the hostname all at once. So where the runtime and size prompts fall back to a default when there is nowhere to ask, the name prompt has nothing to fall back to: with `-o json|yaml|toml|env` or no interactive terminal, the command fails with a new `MissingWorkerNameError` pointing at `supabase experimental workers new api`. The prompt validates against everything the command would otherwise refuse a moment later — a non-DNS-label name, and a name `config.toml` already records — so a typo is corrected in place instead of ending the run. That also means the project has to be loaded before the first prompt, and the machine-output check moves up with it: `-o` leaves `output.format` as `text`, and Clack writes its terminal UI to stdout, so a name prompt would land in front of the payload for the same reason the runtime prompt would. The handler's inline name validation is replaced by the shared `legacyValidateWorkerName`, which the rest of the command family already uses, so an explicitly-passed name and a prompted one are refused on identical terms. `mockOutput` now records `promptTextCalls` so tests can assert on the prompt's message and exercise its `validate` callback. ## Stack Bottom of the workers stack, on `develop`. Above it: output polish (supabase#6389), `workers logs` (supabase#6410), and `push --wait` (supabase#6371). ## Linked issue FUNC-840 (Linear). Supabase maintainer, exempt from the `open-for-contribution` flow. ## Checklist - [x] The PR title follows [Conventional Commits](https://www.conventionalcommits.org/)
pull Bot
pushed a commit
to chizee/cli
that referenced
this pull request
Sep 4, 2026
…e#6410) ## Summary Reads the project's unified logs stream rather than a worker route — there is no worker-scoped log endpoint — via `v1GetProjectLogs`, which the generated client already carries. `--kind app|requests|builds` narrows to one of the three streams; without it all three are returned, tagged per line. `--tail` caps the rows. `--follow` keeps printing until interrupted. ## Three non-obvious things about that endpoint Each is documented at its call site, because none is guessable from the API surface: - **The filter is `log_attributes`, not the `source` column.** Worker rows carry an empty top-level `source`, because the Workers Logflare source is not enrolled as a category in the generic logs path, so `where source = 'worker_guest_logs'` matches nothing. The `in (...)` list over the three known streams is a tenancy guard rather than a convenience — with `source` empty it is the only thing excluding a non-worker row that happens to carry a `worker` attribute. - **Both timestamp bounds are always sent, spanning under 24h.** One bound alone yields a one-minute window, silently; neither is an outright error; and a span over 24h is clamped to `start + 24h`, returning an *older* slice than the one asked for rather than a truncated one. - **A failed query can arrive as HTTP 200** with a populated `error`, so the envelope is checked before `result`. The response is decoded against a local schema rather than the generated `V1GetProjectLogsOutput`: that schema marks `result`/`error` optional but permits neither to be `null`, while the endpoint always sends one of them as an explicit `null`. Decoding a real response against it always fails — worth fixing in the spec separately. ## Rendering Per-stream, because `event_message` differs in kind: on the request stream it is only `"GET /"`, with status and duration in `log_attributes`, so the request line is composed. `severity_text` is ignored — it is `INFO` on every row of every stream — so the level is derived, and app lines report none rather than a guess. An app message is tenant-controlled bytes, so escape sequences are stripped before it reaches a terminal while a stack trace's newlines and indentation survive. ## `--follow` The poll interval is set by the rate limit, not by responsiveness: the v1 analytics endpoints allow 10 requests per 60 seconds, so the two-second poll a live tail suggests would spend the whole allowance in ten seconds. It polls every 10 seconds, measured at ~7 requests in the worst 60-second window. The cursor deliberately lags 60 seconds behind the newest line printed. Guest lines are relayed CloudWatch → subscription filter → Lambda → Logflare and arrive late and out of order, so a cursor sitting on the newest timestamp would drop every straggler permanently. Overlap is therefore guaranteed; dedupe on the Logflare-minted `id` is what makes it invisible. `-o json|yaml|toml` and `--output-format json` are refused up front — each promises one terminal payload and a tail has no last element. `--output-format stream-json` emits one `log-entry` event per line. SIGINT exits 130. ## Stack On top of the workers output polish (supabase#6389), with `push --wait` (supabase#6371) stacked above so it can be rejected independently. Below those: the `workers new` name prompt (supabase#6349). ## Note Replaces supabase#6408, which GitHub marked merged during a stack reorder. It was never merged to `develop`; the branch and its commits are intact here.
pull Bot
pushed a commit
to chizee/cli
that referenced
this pull request
Sep 4, 2026
…supabase#6371) ## Summary `supabase experimental workers push` blocks on the server-side container build. That build routinely runs for minutes, so the common case — a deploy that builds fine — is the slowest thing in the loop. `--no-wait` returns once the platform accepts the deploy, which is the last thing the command can learn without waiting: the deploy response arrives only after the spec and the uploaded context are accepted, and it carries the accepted spec back. Waiting stays the default, so a plain push still reports the build's verdict and existing invocations are unchanged. - Under `--no-wait` the details block leads with a `State` row — the one row that says the worker is not serving yet — and drops `Image`, since no image exists until the build produces one. - A success trailer then points at `experimental workers status` for the build's outcome. Text output only; machine callers read `build_state` from the payload. - A deploy answered with a spec already in `failed` is reported as a failure whether or not the build was waited on, rather than exiting zero on a worker that will never come up. The second commit is a separate fix that fell out of the review: the wait now runs only when the deploy response left `build_state` at `building`. `V2DeployAWorkerOutput` permits a terminal `active` or `failed` on the deploy response itself, and polling on top of that could only contradict it — `awaitWorkerBuild` reads a post-deploy 404 as "still building", so an already-failed deploy could burn the full poll budget and surface as a timeout rather than the failure the platform had already reported. ## Stack Top of the workers stack, on top of `workers logs` (supabase#6410). Everything below it — the name prompt (supabase#6349), the output polish (supabase#6389) and `workers logs` — is independent of this flag, so this PR can be rejected on its own without holding any of them up. ## Linked issue FUNC-848 (Linear). Supabase maintainer, exempt from the `open-for-contribution` flow. ## Checklist - [x] The PR title follows [Conventional Commits](https://www.conventionalcommits.org/)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
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. No command changes what it does — this is output, plus the coverage that pins it.
new,push,deleteandstatusmove toemitSuccessTrailer, the waystop,bootstrap,migration repairandgen signing-keyalready 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.listadvisories. Both take the yellowWARNING:prefix and the two-line consequence shapestart's Docker notice uses. Each was one long sentence that re-flowed at a different width, directly under a table that lines its columns up.listdrops 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 —renderGlamourTablesizes each column to its widest cell and never wraps.statusstill renders it vertically, and every machine format still carriesurlper worker.pushprogress. Per-worker announcements are counted (Deploying Worker 1/2:) and a multi-worker run closes with a summary. Each worker takes minutes; the name alone said nothing about how much of the run was left.pushnames what it 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.--project-refsurvives intopush's retry suggestions, via thelegacyWorkersProjectRefSuffixhelperstatusanddeletealready use. A suggestion is copy-pasted verbatim, so one that dropped it re-resolved against whatever this checkout was linked to.Also adds unit coverage for
legacyRenderWorkerDetails, pins the shared-o envrefusal, and adds a guard (own commit) asserting no legacy boolean flag ships required —Flag.booleanalone builds a required param, and nothing in the existing suites notices.Stack
On top of the
workers newname prompt (#6349). Above it:workers logs(#6410), thenpush --wait(#6371) last, so the output work can ship independently of both.Linked issue
FUNC-851. Supabase maintainer, exempt from the
open-for-contributionflow.Checklist