feat(cli): prompt for worker name if not provided - #6349
Conversation
johnstonmatt
changed the title
johnstonmatt
changed the title
![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: 12693d1a8b
ℹ️ 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@008f160e3e9e7d6c40064b2f79d87db946d9224bPreview package for commit |
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. |
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.
fecde22 to
d4e37d5
Compare
`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.
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.
edc7199 to
5c84df0
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5c84df0b91
ℹ️ 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.
…/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.
…40/select-workers-new-name
kanadgupta
left a comment
kanadgupta
left a comment
There was a problem hiding this comment.
no notes from me, confirmed that this works as expected! leaving my AI-assisted findings below in case they're helpful 👇
[!NOTE]
This review was drafted by an AI (Claude). It was refreshed after new commits landed (reviewed head:008f160e3); the earlier draft's findings were re-checked against the new head.What this PR does
Makes the
nameargument ofsupabase experimental workers newoptional (Argument.optional) and prompts for it when omitted. A sharedcanPromptFordecision (text format + interactive stdout + no-omachine format + stdin is a TTY) gates all three prompts (name, runtime, size). With nowhere to ask, runtime/size keep their defaults as before, while a missing name fails with a new typedMissingWorkerNameError. The handler's inline name validation is replaced with the sharedlegacyValidateWorkerName, the prompt'svalidaterejects non-DNS-label and already-configured names in place, andmockOutputgainspromptTextCallsso tests can assert on the prompt and exercise its validator. Docs (SIDE_EFFECTS.md, exit codes, telemetry note) are updated to match.Since the first draft of this review, the branch merged develop and added two commits:
9ae1f387ffixes theMissingWorkerNameErrorretry suggestion (and the surrounding prose) to name the realsupabase experimental workers newpath, pinned by a regression assertion;12c4d1ca1replaces another nonexistent-command suggestion (supabase updatein the workers API decode failure) with the shared upgrade-guide URL, hoisted intoshared/cli/version.tsso both callers read one constant.Does it fix the linked ticket (FUNC-840)?
Yes. A bare
supabase experimental workers newnow reaches the handler and walks through name, runtime and size prompts instead of failing the parse, with integration coverage for the prompted path, the prompt's validation, and each non-interactive refusal (interactive: false,-o json, piped stdin). The non-interactive failure mode is a deliberate, documented behavior, not a gap.Findings
Graded against the bar for experimental commands: "fix before merge" is reserved for correctness, data-loss, or security issues of real note; everything else is a follow-up candidate.
Fix before merge
- None. The one real defect in the earlier draft — the
MissingWorkerNameErrorsuggestion namingsupabase workers new api, which is not a registered command — was fixed in9ae1f387f, including the prose shorthand in the handler doc comments andSIDE_EFFECTS.md, with a test assertion keeping the retry path from drifting. That also resolves the open Codex P2 thread on the same line.Notes on quality and existing threads
- The diff is small and pattern-conformant: the stdin+stdout+format prompt gate matches
workers deleteandprojects deleteexactly (no shared helper exists in the repo to reuse, socanPromptForis not a DRY violation); the error is aData.TaggedErrorwith an actionability declaration like its siblings; replacing the inline validation withlegacyValidateWorkerNameis a genuine DRY win; tests follow the repo's integration-test pattern (stateful mocks, no e2e added).- Codex thread 1 (stdin gating, resolved earlier): the
canPromptForfix is correct and well tested — the "piped stdin" refusal case and the "defaults without prompting when stdin is piped" case both cover it.- Codex thread 2 (retry command missing
experimental): correct, and now resolved by9ae1f387f.12c4d1ca1reaches slightly beyond this command (workers-api.tsis not used bynew), but it is small, correct, and consolidates the upgrade URL into one constant — fine to ride along.Overall: 0 fix-before-merge issue(s), 0 follow-up candidate(s).
…base#6389) ## 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. - **Success trailers.** "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. - **`list` advisories.** Both 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, directly 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 — `renderGlamourTable` sizes each column to its widest cell and never wraps. `status` still renders it vertically, and every machine format still carries `url` per worker. - **`push` progress.** 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. - **`push` names 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-ref` survives into `push`'s retry suggestions**, 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. Also adds unit coverage for `legacyRenderWorkerDetails`, pins the shared `-o env` refusal, and adds a guard (own commit) asserting no legacy boolean flag ships required — `Flag.boolean` alone builds a *required* param, and nothing in the existing suites notices. ## Stack On top of the `workers new` name prompt (supabase#6349). Above it: `workers logs` (supabase#6410), then `push --wait` (supabase#6371) last, so the output work can ship independently of both. ## Linked issue [FUNC-851](https://linear.app/supabase/issue/FUNC-851/general-output-polish). Supabase maintainer, exempt from the `open-for-contribution` flow. ## Checklist - [x] The PR title follows [Conventional Commits](https://www.conventionalcommits.org/) --------- Co-authored-by: kanad <git@kanad.dev>
2 participants
Summary
Makes the
nameargument tosupabase experimental workers newoptional and prompts for itwhen it is omitted, so a bare
supabase experimental workers newwalks through name, runtimeand 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 andsize 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|envor nointeractive terminal, the command fails with a new
MissingWorkerNameErrorpointing 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.tomlalready 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:
-oleavesoutput.formatastext, and Clack writes itsterminal 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.
mockOutputnow recordspromptTextCallsso tests can assert on the prompt'smessage and exercise its
validatecallback.Stack
Bottom of the workers stack, on
develop. Above it: output polish (#6389),workers logs(#6410), andpush --wait(#6371).Linked issue
FUNC-840 (Linear). Supabase maintainer, exempt from the
open-for-contributionflow.Checklist