feat(workers): add exposure control and new --instances flag - #6432
feat(workers): add exposure control and new --instances flag#6432johnstonmatt wants to merge 2 commits into
Conversation
![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.
| 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}
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.
| ), | ||
| Flag.optional, | ||
| ), | ||
| exposure: Flag.choice("exposure", WORKER_EXPOSURES).pipe( |
There was a problem hiding this comment.
🟠 MAJOR · testing · source: claude
The new value-consuming --exposure flag is absent from VALUE_CONSUMING_LONG_FLAGS, causing the static completeness test to fail.
Evidence: The flag is declared at push.command.ts:28 and new.command.ts:36, while legacy-db-target-flags.ts:74-176 lacks exposure. legacy-db-target-flags.unit.test.ts:262-275 requires every directly declared choice flag to appear in that set.
Suggested fix: Add exposure to VALUE_CONSUMING_LONG_FLAGS.
| * `planWorkerEntry`'s re-parse catches before the file is written. | ||
| */ | ||
| function renderPair(key: string, value: string | number): string { | ||
| return `${tomlKey(key)} = ${typeof value === "number" ? String(value) : quote(value)}`; |
There was a problem hiding this comment.
🟡 MINOR · error-handling · source: claude
Arbitrary numbers can render as valid TOML values that violate the worker schema, contrary to the comment claiming reparsing rejects them.
Evidence: toml-section.ts:70-71 renders every number with String(value). worker-config.ts:167-189 checks only TOML syntax and table presence, while packages/config/src/workers.ts:66-68 requires a non-negative integer. Values such as 2.5 and NaN therefore survive TOML parsing but fail config decoding.
Suggested fix: Validate numbers with Number.isSafeInteger before rendering, or narrow and accurately document the helper’s contract.
| // 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.
🔴 CRITICAL · security · source: 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.
Evidence: worker-config.ts:73-74 maps an empty string to undefined, and line 114 applies it to exposure. push.handler.ts:169-170 then selects DEFAULT_WORKER_EXPOSURE (public), bypassing the rejection at lines 172-179. worker-runtimes.unit.test.ts:58-62 establishes that an empty exposure is invalid.
Suggested fix: Preserve empty exposure strings through readWorkersSection so resolveExposure rejects them, or validate exposure during config decoding.
| @@ -335,7 +401,10 @@ export const legacyWorkersNew = Effect.fn("legacy.experimental.workers.new")(fun | |||
| legacyRenderWorkerDetails([ | |||
| ["Runtime", runtime], | |||
| ["Size", `${size} (${vcpuForSize(size)} vCPU)`], | |||
| ["Access", "public"], | |||
| ["Access", exposure], | |||
| // `declared`, the way `workers status` labels the same number: nothing | |||
| // is running yet, so a bare count would read as a live tally. | |||
| ["Instances", `${instances ?? DEFAULT_WORKER_INSTANCES} declared`], | |||
There was a problem hiding this comment.
⚪ NIT · test-coverage · source: claude
The new machine-output fields and human-readable exposure and instance rows are not asserted by tests.
Evidence: new.handler.ts:374-378 adds exposure and instances to the payload, and lines 404-407 add the text rows. new.integration.test.ts:484 and :640 assert only runtime and size; its text assertion at line 71 checks only Runtime.
Suggested fix: Assert exposure and instances in machine output and the Access and Instances rows in text output.
| | `<SUPABASE_HOME or ~/.supabase>/telemetry.json` | JSON | whenever the handler runs — flushed on success and on failure | | ||
| | Path | Format | When | | ||
| | ----------------------------------------------- | ------ | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | ||
| | `<workdir>/supabase/config.toml` | TOML | on success — appends `[workers.<name>]` with `runtime`, `size`, `exposure`, and `instances`/`source` when those differ from the default, preserving surrounding formatting | |
There was a problem hiding this comment.
⚪ NIT · documentation · source: claude
The Files Written table incorrectly implies that source is written only when it differs from a default.
Evidence: SIDE_EFFECTS.md:19 groups instances and source under “when those differ from the default,” but new.handler.ts:337-354 writes source whenever --source is present; no source default is compared.
Suggested fix: State separately that instances is written when non-default and source is written when --source is supplied.
2 participants
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.