feat(workers): add exposure control and new --instances flag - #6432
Open
johnstonmatt wants to merge 67 commits into
Open
feat(workers): add exposure control and new --instances flag#6432johnstonmatt wants to merge 67 commits into
johnstonmatt wants to merge 67 commits into
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.
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.
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.
`status` reports the deployment; nothing reported the runtime. Once `push` succeeded and `status` said `active`, a misbehaving worker was a black box from the CLI. 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. `--source app|requests|builds` narrows to one of the three streams; without it all three are returned. `--tail` caps the rows. Three things about that endpoint are load-bearing and non-obvious, so they are documented at each site: - **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. `where source = 'worker_guest_logs'` matches nothing. The `in (...)` list over the three known streams is therefore a tenancy guard, not 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`, which returns 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 allows neither to be `null`, while the endpoint always sends one of them as an explicit `null`, so decoding any real response against it fails. Rendering is 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 guest lines report none rather than a guess. A guest message is tenant-controlled bytes, so escape sequences are stripped before it reaches a terminal while a stack trace's newlines and indentation survive. `mapRequestError`/`unexpectedStatus`/`decodeBody` move out of `workers-api.ts` into `workers-api-status.ts`, unchanged, now that a second seam needs them. The test helper records `urlParams`: `HttpClientRequest` keeps them off the URL, so without this no test could assert the emitted SQL or window.
Matches the only other log-line format this shell prints — the `--debug` HTTP logger, which uses Go's `log.LstdFlags` (`legacy-debug-logger.layer.ts`). Someone reading a tail is asking "what just happened", and the answer gets compared against their own clock. Text output only. The machine payload keeps both unambiguous forms, so nothing that is parsed, sorted, or pasted into an issue depends on the reader's zone: `timestamp` stays ISO-8601 UTC and `timestamp_ms` the raw epoch value. The unit tests derive their expected prefix from the same instant with the same field accessors, rather than hardcoding one: a literal `"14:45:32"` would have passed only on a UTC machine. One case additionally pins the zone choice itself — asserting the output is *not* the UTC rendering — guarded so it stays meaningful on a UTC machine, where the two coincide. Verified green under `TZ=Asia/Tokyo`, `TZ=UTC`, and the ambient zone.
Opt-in, matching `workers push --wait`: long-running behaviour in this family is asked for, never defaulted. **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. Six seconds is the arithmetic floor; ten leaves room for the history query, the deployed-worker check, and a retry in the same window. Measured at ~7 requests in the worst 60-second window. The interval is in `--follow`'s help text, because a 10-second tail is visibly not a live stream and would otherwise look broken. 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 and expected; dedupe on the Logflare-minted `id` is what makes it invisible, bounded so a long tail does not grow the set forever. `followWindow` clamps to the same sub-24h span as a bounded read, so a tail resumed after a laptop suspend cannot ask for a wider window — the server answers those by returning an *older* slice. Every poll sends both timestamp bounds. Advancing only `iso_timestamp_start` is the obvious implementation and is wrong: it yields a one-minute window. Output: - `-o json|yaml|toml` and `--output-format json` are refused up front, beside the `-o env` refusal and for the same reason — each promises one terminal payload and a tail has no last element. - `--output-format stream-json` emits one `log-entry` event per line instead of a single `result`, reusing the existing variant. `stream` splits error/warn to `stderr`; `source` separates backlog from live. - SIGINT exits 130, matching the local `supabase logs` command. - `--tail 0` skips the backlog and makes no history request, since the endpoint rejects `limit 0`. It also suppresses the not-deployed check, which would otherwise read "no rows" as "no worker" when no query was made at all. Both schedules are injectable, as `awaitWorkerBuild`'s are, so the cursor, dedupe and retry paths are tested without a wall clock. The SIGINT test forks the handler and synchronises on the mock's `awaitExit` — `exit` never returns, so the handler cannot be awaited. Stressed over five consecutive runs.
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 workers push` blocked on the server-side container build on every invocation. That build routinely runs for minutes, so the common case — a deploy that builds fine — was the slowest thing in the loop. The command now returns once the platform accepts the deploy, which is the last thing it 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. `--wait` opts into the build's verdict, for CI and for anyone who needs the image version before continuing. A deploy answered with a spec already in `failed` is still reported as a failure whether or not the build was waited on, rather than exiting zero on a worker that will never come up. Without `--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 names both `workers status` and `--wait` as ways to follow the build. The flag closes with `Flag.withDefault(false)`; the guard added in the previous commit is what keeps a boolean flag from shipping required again.
…/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
# Conflicts: # apps/cli/src/shared/workers/workers-api.ts
The guard's rationale pointed at `supabase workers push`, but the family is registered beneath the `experimental` parent, so that invocation does not exist. Name the real path, and describe the flag rather than when it was added.
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.
`--source` already means "the directory a worker's code lives in" across the family: it is the `config.toml` key, the flag on `workers new`, the `Source` row in `status`, and what `push` reads to find a worker. Reusing it on `logs` for a closed set of log streams gave one flag two meanings in one command family. `--kind` names the axis rather than the mechanism, cannot be read as a mode the way `--stream` collides with `--follow`, and reads correctly inside a command already called `logs`. The backend's `log_attributes['source']` key and the per-line `source` field on `log-entry` events are untouched — they are different things, and the rename is what makes them distinguishable. The command is unreleased, so this costs nothing now and would be a breaking flag change later.
Same defect the status and delete matrices carried: the no-logs line and its `status` trailer sit below the machine and structured early returns, so only the text branch reaches them, but every structured row claimed "as above".
`WORKER_LOG_STREAMS[flags.kind.value as WorkerLogKindChoice]` asserted the relationship rather than holding it: the choice list lived in the command and the stream map in the query module, so adding a word to one and not the other compiled, indexed the map as `undefined`, and reached the SQL as an empty stream name. The cast turned out to be unnecessary on its own — `Flag.choice` already infers the literal union — so removing it makes the index site type-checked. The list moves next to the map as `WORKER_LOG_KINDS`, constrained with `satisfies`, so drift now fails at the declaration as well as at the use.
`-o` outranks `--output-format` when both are set, and `-o pretty|table|csv` encode nothing and fall through to the text rendering. Branching on `output.format` alone therefore did the opposite of what the pair asked for: `-o pretty --output-format json --follow` was refused as a single-payload format, and the bounded path emitted JSON for a run that had asked for text. `legacyWorkersRenderFormat` resolves the two flags once, and the handler branches on that instead of consulting `output.format` at each emission.
Ref resolution sat above both finalizers, so an unlinked non-interactive checkout — or a declined project picker — failed before `telemetryState.flush` was installed and wrote no post-run event, even though the command had run. Telemetry now wraps the resolution; only the linked-project cache stays under the ref, since it has nothing to write without one.
laktek
approved these changes
Sep 2, 2026
![depthfirst-app[bot]](https://avatars.githubusercontent.com/in/1021002?s=60&v=4){kind=small}
| // Left as whatever string was written, like `runtime` and `size`: `push` | ||
| // is what names the accepted values, and dropping an unrecognized one here | ||
| // would silently deploy a worker at the default exposure instead. | ||
| exposure: stringOrUndefined(value["exposure"]), |
There was a problem hiding this comment.
🟡 Severity: MEDIUM
exposure = "" is valid under the new Schema.String field, but this reader converts the empty value to undefined. push then treats it as absent and sends public, so a malformed or mutated recorded policy silently re-exposes a worker instead of refusing deployment as the new validation contract requires.
Helpful? Add 👍 / 👎
💡 Fix Suggestion
Suggestion: Replace stringOrUndefined(value["exposure"]) with a type-only check that preserves empty strings. The current stringOrUndefined helper (line 73-74) explicitly converts "" to undefined, which defeats the intent documented in the comment: an unrecognized (or empty) recorded exposure value should reach resolveExposure and be rejected with UnknownWorkerExposureError, not silently fall through to the public default. Use typeof value["exposure"] === "string" ? value["exposure"] : undefined so any actual string — including "" — is passed through as-is, while non-string TOML values (arrays, tables, absent keys) are still normalised to undefined.
⚠️ Experimental Feature: This code suggestion is automatically generated. Please review carefully.
Suggested change
| exposure: stringOrUndefined(value["exposure"]), | |
| exposure: typeof value["exposure"] === "string" ? value["exposure"] : undefined, |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🤖 AI Review
Six Claude findings were adjudicated; Codex reported none. Five are confirmed, including a CI-breaking missing flag registration and a fail-open empty exposure that can deploy publicly. The explicit-default instance finding is refuted by the repository’s documented sparse-config convention.
Findings
| Severity | Location | Category | Sources | Claim |
|---|---|---|---|---|
| 🔴 CRITICAL | apps/cli/src/shared/workers/worker-config.ts:114 |
security |
claude | An explicitly recorded empty exposure is treated as absent, so a bare push fails open to public exposure instead of rejecting the unknown value. |
| 🟠 MAJOR | apps/cli/src/legacy/commands/experimental/workers/push/push.command.ts:28 |
testing |
claude | The new value-consuming --exposure flag is absent from VALUE_CONSUMING_LONG_FLAGS, causing the static completeness test to fail. |
| 🟡 MINOR | apps/cli/src/shared/workers/toml-section.ts:71 |
error-handling |
claude | Arbitrary numbers can render as valid TOML values that violate the worker schema, contrary to the comment claiming reparsing rejects them. |
| ⚪ NIT | apps/cli/src/legacy/commands/experimental/workers/new/new.handler.ts:374 |
test-coverage |
claude | The new machine-output fields and human-readable exposure and instance rows are not asserted by tests. |
| ⚪ NIT | apps/cli/src/legacy/commands/experimental/workers/new/SIDE_EFFECTS.md:19 |
documentation |
claude | The Files Written table incorrectly implies that source is written only when it differs from a default. |
Refuted findings (kept for transparency, not posted as review comments)
apps/cli/src/legacy/commands/experimental/workers/new/new.handler.ts:231(consistency): Omitting an explicit--instances 1contradicts the repository’s persistence rationale for explicit default values.
Refuted: The omission is deliberate, tested, documented in the handler, and consistent with the trusted sparse-config ADR’s accepted treatment of explicit defaults. Exposure follows a separately documented pinning policy, so the difference is not evidence of an implementation defect.
Stats
Claude findings: 6 · Codex findings: 0 · Confirmed: 5 · Refuted: 1 · Uncertain: 0
Models: claude-opus-5 + gpt-5.6-sol · Trigger: auto · Workflow run
This review runs once per PR. A maintainer can request another with a /ai-review comment.
1 task
Waiting on the server-side build goes back to being the default, so a plain `supabase experimental workers push` still reports the build's verdict and existing invocations keep their behaviour. `--no-wait` is the opt-out for an inner-loop redeploy or a CI step that only needs the deploy accepted. The early-return path is unchanged: it still leads the details block with `State`, drops `Image`, reports a deploy answered with `build_state: failed` as a failure, and emits the `workers status` success trailer. The trailer no longer suggests adding `--wait`, since reaching it means the caller asked not to wait.
`V2DeployAWorkerOutput` permits a terminal `active` or `failed` on the deploy response itself, and that verdict belongs to this deploy. Polling on top of it 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 `WorkerBuildTimeoutError` rather than the failure the platform had already reported, and a concurrent deployment could answer with a state from someone else's build. The wait now runs only when the deploy response left `build_state` at `building`, which also drops a redundant `GET` on the common terminal cases.
…ogs-command # Conflicts: # apps/cli/src/shared/workers/workers-api.ts
…indow `followWindow`'s `graceSeconds`/`spanMinutes` options and `logWindow`'s `spanMinutes` parameter had no caller and no test — both call sites pass nothing — so they were configurability for its own sake over the module's trickiest arithmetic. Both now read the module constants directly. `followWindow` had no direct unit tests at all. It has four now, including the suspend case the 24h clamp exists for: a cursor left days behind would otherwise ask for an over-wide span, which the server answers by rewriting `end` to `start + 24h` — returning an older slice rather than a truncated one, so a resumed tail would silently replay yesterday. The expectations are written as literals rather than read from the constants under test, so they cannot stay green through the change they exist to catch.
The `FOLLOW_MAX_PAGES` comment claimed rows past the bound were re-asked for on
the next poll. They are not. The drain walks `end` backwards, so the pages it
does fetch are the newest ones, and the cursor then advances to the newest row
printed — past a region it never reached. Only the part of that region inside
the next window's grace comes back.
Lowering the cursor cannot fix it: `followWindow` moves the window's floor, not
its ceiling, so a poll anchored at `now` would re-fetch the same newest pages
and never walk down to the gap. The bound stays, and the loss is now reported
instead of silent — once per run, on stderr, in every output format, since a
`stream-json` consumer cannot infer a hole from the events it receives.
Corrects the rate arithmetic in the same pass. Both the handler comment ("~7
requests in the worst 60-second window") and SIDE_EFFECTS ("6 requests a
minute") predate the multi-page drain and assume one request per poll. A quiet
tail does spend 6 a minute; a poll draining a burst spends up to 5, so a
sustained backlog reaches 30 against a limit of 10 and is throttled by the
retry rather than budgeted for.
…gnosis" This reverts b772491. The fix is sound but has nothing to do with workers logs; it landed here to stabilise this branch's CI. Moved to #6455 off develop so it does not merge or revert with the workers work. Raised in review on #6410. Until #6455 lands, the `packages/config` release-script test it stabilises can flake on a loaded runner.
Two findings from review, both reachable only under `--no-wait`. `runCli` drains success trailers on exit code 0 only, so a multi-worker run whose later worker fails discarded the follow-up hint for every worker already accepted — while their builds carried on running on the platform. The failure path now names them itself: `Still building: api`, a sibling to `Not attempted: web` and on the same terms, stderr in every format, because a machine-format run is a CI run where nobody watched the loop. `image_version` is optional-but-permitted on the deploy response, so a re-push of a worker that is already serving can echo the image it is serving now. Shown beside `State building` that named an image this deploy did not produce, and a script reading `image_version` next to `build_state: "building"` would take it for the new one. Both the row and the payload field are now omitted while the build is still running.
…eploy-wait-flag # Conflicts: # apps/cli/src/legacy/commands/experimental/workers/push/push.integration.test.ts
kanadgupta
requested changes
Sep 3, 2026
kanadgupta
left a comment
kanadgupta
left a comment
Member
There was a problem hiding this comment.
works as expected in my lightweight testing 👍 but there are two valid issues that claude flagged that i believe should be addressed prior to merge, see below
[!NOTE]
This review was drafted by an AI (Claude).This PR adds an
exposuredial (public/private) toworkers newandworkers push, recorded inconfig.tomland resolved with the same precedence as the size/runtime dials (flag override → recorded value →publicdefault, with an unrecognized recorded value refusing the deploy), plus an--instancesflag onworkers newthat is written only when it differs from the default of 1 and rendered as a bare TOML number. Thetoml-section.ts/worker-config.tsnumeric extension is minimal (onerenderPairbranch and a widened patch type), the closed-set/parse/refuse machinery mirrors the siblingruntime/sizedials closely, and integration/unit coverage is thorough — including the case-insensitive read, the typo refusal before anything is uploaded, and the bare-number round-trip. FUNC-859 is delivered as specced.Fix before merge
--exposureis missing fromVALUE_CONSUMING_LONG_FLAGS, so the static completeness test inlegacy-db-target-flags.unit.test.tsfails — masked on this PR because theTestjob was skipped in this stacked run — and bare-form--exposure privateis mis-parsed by the telemetry argv scanner. (Amplifies the unaddressed github-actions thread onpush.command.ts.)- A recorded
exposure = ""is read as absent byreadWorkersSectionand silently deploys at thepublicdefault, contradicting this PR's own refuse-don't-coerce contract for this dial. (Amplifies the unaddressed depthfirst and adjudicated-review threads onworker-config.ts:114.)Follow-up candidates
push --exposure privateis a one-deploy override by design, but the next barepushsilently re-exposes the worker; a stderr nudge to record the value (precedent: the runtime-guess nudge inresolveRuntime) would be cheap.resolveExposureinnew.handler.tsis the third copy of the explicit → prompt → default resolver shape; a parameterized closed-set dial resolver could collapse the three.- The
renderPairdoc comment overclaims: a finite non-integer (e.g.2.5) renders as valid TOML that only fails on the next config load, so the re-parse does not catch it. Current callers areFlag.integer-bounded, so this is a comment/latent-contract fix — the existing bot thread ontoml-section.tshas it right.newrecordsexposureinconfig.tomlonly, while the project loader prefersconfig.jsonwhen one exists — in such a project a scaffolded--exposure privateis invisible topush. That TOML-only writer is a documented pre-existing constraint from lower in the stack, butexposureraises its stakes; worth revisiting when the JSON path is addressed.Existing threads: the adjudicated review's CRITICAL (empty exposure) and MAJOR (flag registration) findings are correct and unaddressed — items 1–2 above chime in on them. Its refutation of the "explicit
--instances 1should be written" finding is right: the sparse-config treatment is deliberate, tested, and documented. The two NITs (missing output assertions,SIDE_EFFECTS.mdtable wording onsource) are fair but minor.Overall: 2 fix-before-merge issue(s), 4 follow-up candidate(s).
Base automatically changed from
FUNC-848/workers-deploy-wait-flag
to
develop
September 3, 2026 23:48
…-private-exposure # Conflicts: # apps/cli/src/legacy/commands/experimental/workers/push/SIDE_EFFECTS.md # apps/cli/src/legacy/commands/experimental/workers/push/push.command.ts # apps/cli/src/legacy/commands/experimental/workers/push/push.handler.ts # apps/cli/src/legacy/commands/experimental/workers/push/push.integration.test.ts
`VALUE_CONSUMING_LONG_FLAGS` is what tells the legacy argv walk to skip a flag's value when it scans for db-target flags. `--exposure` takes one and was missing, so `--exposure private` left `private` to be read as a positional. Caught by the repo-wide completeness guard in `legacy-db-target-flags.unit.test.ts`, which walks every directly-declared value-consuming flag in the command tree — it only fires on a full unit run, so a workers-scoped run stayed green.
…ivate-exposure #6371 was squash-merged into develop, so develop's copy of the workers push work shares no ancestry with the merge of it this branch already carries. A plain merge therefore re-conflicted all four push files against content the branch already had. Recorded with an explicit merge base of 539986a — the FUNC-848 tip that squash came from, verified content-identical to develop — which resolves cleanly and leaves the tree unchanged.
Contributor
Supabase CLI previewnpx --yes https://pkg.pr.new/supabase/cli/supabase@d67d67b5f9c5276347b0485d1fc2f092deb0392cPreview package for commit |
`readWorkersSection` collapsed every empty string to `undefined`, so `[workers.<name>] exposure = ""` reached `push` as "not recorded" — and absent means the `public` default. A config that plainly tried to say something about exposure therefore deployed the worker to the whole internet, which is the one outcome `resolveExposure` was written to prevent. Exposure now keeps whatever string was written, empty included, so a blank value reaches the same refusal any other unrecognized one gets. It is named as blank rather than reported as an unknown `""`, which reads like a parser quirk instead of an empty key. `runtime`, `size` and `source` keep the collapsing reader: their fallbacks are a marker-file guess, a default size and the conventional directory, none of which widens anything.
…rejects `renderPair` claimed that anything other than a whole number "renders as a token TOML does not accept, which `planWorkerEntry`'s re-parse catches". That re-parse is a syntax check, not a schema one, and the claim only holds for `NaN` and `Infinity` — `String()` renders those as tokens TOML has no reading for. A fraction, a negative count or a value past the safe integer range all render as perfectly valid TOML that the worker schema refuses, so they would reach the user's `config.toml` and surface later as a file the loader will not load. `isRenderableTomlNumber` is the guard the comment described, applied in `planWorkerEntry` before anything is rendered, so the refusal happens before anything reaches disk like every other unsafe write. The comment now describes what the code does.
…e doc The scaffold's machine payload was asserted for `runtime` and `size` only, so `exposure`, `instances`, `vcpu` and `worker_name` could change unnoticed — and those are the fields a caller reads to decide what it is about to deploy, since `instances` is deliberately left out of `config.toml` at the default. The human-readable `Access` and `Instances` rows had no assertion at all. The Files Written table also grouped `instances`/`source` as written "when those differ from the default". `exposure` is written unconditionally, `instances` only when it differs from 1, and `source` whenever `--source` was passed — even when it names the conventional directory.
`--exposure` decides one deploy and nothing writes it down. Every deploy sends a complete spec, so a worker taken off the internet with `--exposure private` goes back on it at the next bare `push` — silently, which is the opposite of what someone reaching for that flag was going for. Reported the way `resolveRuntime` reports a guess: on stderr, naming the `[workers.<name>] exposure` line that makes it stick, and unguarded by output format, since a CI run is where a quietly reverting exposure matters most. Silent when nothing drifts — the config already resolves to the same exposure, case included, or the flag restated the default a bare push would have picked anyway. A recorded value the CLI cannot read is nudged too: the next bare push refuses rather than deploying, which is still not what this run did.
kanadgupta
approved these changes
Sep 4, 2026
kanadgupta
left a comment
kanadgupta
left a comment
Member
There was a problem hiding this comment.
one tiny follow-up below worth noting but happy with this as is!
[!NOTE]
This review was drafted by an AI (Claude).Refresh pass following the submitted CHANGES_REQUESTED review (head was 2f4f766). Head is now d67d67b, retargeted onto
developafter the stack below merged, and the previously-skippedTestjob now runs and is green.Prior findings, accounted for:
--exposuremissing fromVALUE_CONSUMING_LONG_FLAGS— fixed in 4d0cc08 (one-line registration; the completeness guard now runs in CI and passes, and the telemetry argv scan parses bare--exposure privatecorrectly).- Blank recorded
exposure = ""failing open to public — fixed in 35ebbaf. A dedicatedrecordedStringOrUndefinedreader keeps the empty string forexposureonly, so it reachespush's refusal, now with its own "blank exposure" wording; unit + integration coverage including the nothing-was-uploaded assertion. Keeping the collapsing reader forruntime/size/source(whose absence-fallbacks widen nothing) is the right scoping, and the rationale is documented at the helper.- (follow-up a) One-deploy
--exposuresilently reverting — done in d67d67b, stronger than suggested: the stderr nudge fires on any drift from what a bare push would resolve to (including an unreadable recorded value), and stays quiet on case-insensitive agreement or a flag restating the default. Five integration tests cover the matrix. One wording issue in the nudge text — see the inline comment.- (follow-up b) Parameterizing the three dial resolvers — rebutted by the author, and I agree: he tried it and dropped it, and the nudge logic added in d67d67b makes
resolveExposurediverge further fromresolveRuntime/resolveSize, so a shared resolver would now need more knobs than it saves.- (follow-up c)
renderPairdoc overclaim — fixed in ce7c54d, beyond the comment fix:isRenderableTomlNumberguardsplanWorkerEntrybefore rendering, refusing1.5/-1/1e21/NaNwithWorkerConfigWriteUnsafeErrorbefore anything reaches disk, with table-driven tests. This also settles both bot threads ontoml-section.ts.- (follow-up d) TOML-only entry writer vs the JSON-preferring project loader — stands as a follow-up. No code change; the constraint remains documented in
new/SIDE_EFFECTS.mdas pre-existing from lower in the stack. Acceptable under the experimental bar, but worth revisiting when the JSON write path is addressed — the depthfirst thread onnew.handler.tswas resolved without a reply, so flagging it here so it isn't lost.The two NITs from the adjudicated review (missing output assertions,
SIDE_EFFECTS.mdtable wording) were both addressed in d9eca62.Delta since the last review: five tightly scoped commits (the four fixes above plus the test/doc commit). I re-checked the empty-exposure reader's blast radius —
entry.exposureis consumed only bypush'sresolveExposure, so the pass-through cannot leak a blank string anywhere else — and the number guard's placement ahead of both the render and the syntax-only re-parse. Effect usage, error typing, and the testing pyramid all hold; noascasts in production code.Retarget sanity: the diff vs
developcontains exactly this PR's 20 files (exposure + instances work, their tests, docs, and generated schemas) — no leftovers from the merged stack.New findings:
Follow-up candidates
- The drift nudge says "Pin it down by adding
[workers.<name>] exposure = …" even in the two branches where the table and often the key already exist; following it literally there produces a duplicate table/key (inline comment onpush.handler.ts).- Prior finding 6 above (config.json projects can't see a scaffolded
exposure) remains open.Overall: 0 fix-before-merge issue(s), 2 follow-up candidate(s).
| ? `records no exposure for ${options.name}` | ||
| : `records exposure = "${options.recorded}"` | ||
| }, so the next bare push will not use ${chosen}. ` + | ||
| `Pin it down by adding [workers.${options.name}] exposure = "${chosen}" to supabase/config.toml.\n`, |
Member
There was a problem hiding this comment.
small copy edit suggestion below 👇 i imagine it adds some complexity to this logic for what is a minor change so i'm cool with whatever you run with here
Follow-up candidate: this nudge fires in three cases, and in two of them
[workers.<name>]— and usually theexposurekey itself — already exists inconfig.toml(therecords exposure = "public"and unreadable-recorded branches). TakingPin it down by adding [workers.api] exposure = "private"literally there appends a duplicate table or key, which makes the file unparseable on the next load. The refusal path below already has the executable verb for that situation (Set [workers.api] exposure to …); switching to "set"/"change" whenoptions.recordedis defined — keeping "adding" for the no-entry case, matching the runtime nudge — would keep the instruction correct in all three branches.
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
3 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.
Adds an
exposuredial (public/private) for worker deployments alongside the existing runtime/size/instances dials, and letsworkers newset instance count up front instead of only throughpush.exposureas a closed set (public/private) onworkers newandworkers push, recorded inconfig.toml/config.jsonand resolved with the same precedence as size/runtime: flag override, then recorded value, thenpublicdefault; an unrecognized recorded value refuses the deploy rather than silently coercing it.--instancestoworkers new, written toconfig.tomlonly when it differs from the default of 1, and rendered as a bare TOML number rather than a quoted string.toml-section.tsandworker-config.tsto support writing numeric values, and update the config schema/docs (packages/config/src/workers.ts,config.schema.json,project-config.schema.json) to describeexposure.UnknownWorkerExposureErrorand corresponding unit/integration test coverage across both commands.