feat(cli): add supabase workers new - #6261
Conversation
johnstonmatt
changed the title
ca0599d to
0d433d5
Compare
3b8b0c0 to
ed12377
Compare
ed12377 to
0fce041
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ed12377e3d
ℹ️ 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@72ec632d1f0125d1e295f49b946b53368f668ad2Preview package for commit |
0fce041 to
673cf2b
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 673cf2bfc1
ℹ️ 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".
…tput Addresses the eighteen review findings on #6261. Path confinement was string-deep only. `isAtOrUnder` compared resolved paths lexically, so `--source` through an in-project symlink pointing outside it passed, and `--force` deleted the external target. The reserved-directory check was case-sensitive, so `[workers] root = "Functions"` aliased `supabase/functions` on Windows and default macOS filesystems — enough to delete an Edge Function. And `./.` or `services/..` were only rejected before `normalize` reduced them to `.`, which made `supabase/` itself the workers root. There is now one guard, `confineWorkerPath`, that answers containment on the filesystem's terms: it canonicalizes the longest existing prefix of the target (`realPath` fails outright on a path that isn't there yet) and the project root with it, so a project living under a symlink still compares like for like. Both `--source` and the `source` recorded in `config.toml` go through it — the latter previously had no confinement at all, which is a `push` exfiltration path. `workers new --force` could destroy a worker and leave nothing in its place. Every way the `config.toml` edit can fail is deterministic and knowable up front, but it was attempted after the previous directory had been removed and the replacement starter files written. `writeWorkerEntry` is now `planWorkerEntry` + `commitWorkerEntry`, and the handler plans before it touches anything. A regular file at the destination is also handled: it used to read as an empty directory, skip the removal, and fail `EEXIST`. The textual TOML writer missed literal-quoted keys (`'runtime' = 'node'`), appending a duplicate and invalidating the file; wrote control characters verbatim, which a Unix path may legally contain; and could mistake a `[workers.api]` line inside a multiline string for the table itself. The first two are fixed, the third now refuses rather than guessing. Also: `--source` resolves against the invocation directory rather than the project root, matching what a shell prompt implies and the resolver's own tests; persisted paths are normalized to forward slashes so a Windows-written `config.toml` still works elsewhere; the auto-name and config-reuse notices moved to stderr, since `-o json` leaves `output.format` as `text` and they were corrupting the payload; `as` casts in `worker-config.ts` and `worker-runtimes.ts` replaced with a record guard and predicate narrowing; decoded worker entries read into a null-prototype record so a worker named `constructor` is not reported as already configured; the formatter exports carry the mandatory `legacy` prefix; and the `workers` row is out of the frozen `go-cli-divergences.md`, with the Go-parity framing removed from the new comments, docs and helper name. `parseWorkerRuntime`'s claim to make `--runtime Dockerfile` work was simply false — `Flag.choice` validates before any handler runs. The comment is corrected rather than the flag loosened; the case-insensitivity is real and serves hand-written config values.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 66e459abca
ℹ️ 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".
…OML fixes Six follow-ups on #6261, most of them incomplete corners of the previous round. `.temp` was missing from the reserved roots. It holds CLI state — the linked-project reference among it — and `workers new <name> --force` resolves its destination *inside* the root, so `[workers] root = ".temp"` with a worker named after one of those files deleted it. `--source supabase/.temp/...` reached the same place. Both refused now. A committed `[workers] root` written on Windows meant a different directory on POSIX: `normalize` keeps backslashes as separators on Windows and treats them as ordinary filename characters elsewhere, so one config targeted `supabase/services/workers` on one machine and `supabase/services\workers` on another. Backslashes are read as separators wherever the value was written. This is the same class as the `--source` normalization already fixed, from the reading side rather than the writing side. The multiline-string guard only covered finding the header. The scan for the table's *end* still treated any bracketed line as the next table, so a bracketed line inside one of the table's own multiline strings truncated it early and the new keys landed inside the user's string — the same corruption, reached from the other end. Both ends refuse now. `findHeader` matched only the canonical `[workers.api]`. TOML calls `[workers."api"]`, `[workers.'api']`, `[workers . api]` and `[ workers.api ]` the same table, and the parser that produced the decoded config agreed — so `planWorkerEntry` decided an entry it could plainly see was dotted or inline configuration, and refused every update to it including a safe forced re-scaffold. Quoted dots are handled too: `workers."my.worker"` is two keys. `--source` no longer trims whitespace. A directory name may legally end in a space on Unix, and the shell only delivers one as a single argv entry if the user quoted it, so trimming pointed `--force` at a neighbouring directory instead of the one named. Only the trailing separator, which is syntax, still comes off; an all-whitespace argument is refused rather than trimmed into something else. `SIDE_EFFECTS.md` also now lists the profile files every invocation reads. Worth noting this is a repo-wide gap rather than a workers one — no other command's Files Read table mentions them either, including the canonical `backups/list` — so a sweep across the other 99 documents would be a reasonable follow-up.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 87768d36de
ℹ️ 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".
| ["Access", "public"], | ||
| ]), | ||
| ); | ||
| yield* output.raw(`Deploy it with supabase workers push ${name}.\n`); |
There was a problem hiding this comment.
Every successful text-mode scaffold tells the user to run supabase workers push <name>, but a repo-wide search of apps/cli/src shows that this change registers only workers new; there is no Command.make("push") or other push handler. Users following the command's sole deployment instruction therefore immediately receive an unknown-command error, so omit this guidance until push is registered or land the two command surfaces together.
Useful? React with 👍 / 👎.
…machine stdout Two more from the re-review of #6261, both gaps in the previous round rather than new ground. `--source supabase/config.toml --force` was authorized. Refusing `supabase/` itself and the reserved subdirectories left the files sitting directly in `supabase/` reachable: the path was inside the project, was not the supabase directory, and was in no reserved subdirectory — so `--force` removed the project's configuration and put a starter directory where it had been. `config.toml` and `config.json` are refused explicitly now. The prompts were still writing to stdout under `-o json`. Last round moved the auto-name and config-reuse notices to stderr but left `promptSelect`, which goes through Clack — and Clack writes its terminal UI to stdout with no stream override, while `-o` leaves `output.format` as `text`. So an interactive `workers new api -o json` with no `--runtime` rendered a selection UI in front of the payload. With a machine format requested there is nowhere to ask, so the defaults stand; the integration test supplies prompt answers that would be accepted, then asserts no prompt happened and stdout parses.
![depthfirst-app[bot]](https://avatars.githubusercontent.com/in/1021002?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: 2de742ab4b
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9e7af6c2d9
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8121432f02
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| | Path | Format | When | | ||
| | ----------------------------------------------- | ------ | ------------------------------------------------------------------------- | | ||
| | `<workdir>/supabase/config.toml` | TOML | always — appends/updates `[workers.<name>]` in place, preserving comments | |
There was a problem hiding this comment.
The append-only refactor now rejects every worker already present in the decoded config, so this command never updates an existing table, yet the compatibility checklist still says it “appends/updates” one. The same document consequently retains obsolete statements about resolving a recorded source and failing only when an entry cannot be edited safely; update these descriptions to reflect the unconditional existing-worker refusal because this file is the primary E2E compatibility input.
AGENTS.md reference: apps/cli/AGENTS.md:L359-L366
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
+1 — still stale at ea4ae3e. Concretely: Files Written says "appends/updates [workers.<name>] in place" (writes are append-only now and an existing entry is refused outright); Exit Codes still lists "unknown runtime/size" (those errors were deleted — --runtime/--size are choice flags and recorded values are no longer read) and "records a worker in a form that cannot be edited safely" (now: a worker that is already recorded, in any form); and the SUPABASE_ACCESS_TOKEN row claims a keyring/access-token-file fallback this command's runtime never builds. This file is the compatibility checklist, so it should describe the shipped refusal semantics.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 26bde1bc1f
ℹ️ 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".
| if (info.value.type !== "Directory") { | ||
| return false; | ||
| } | ||
| const entries = yield* fs.readDirectory(target).pipe(Effect.orElseSucceed(() => [])); |
There was a problem hiding this comment.
When an existing destination grants search/write permission but not read permission, stat succeeds while readDirectory fails, and this fallback misclassifies the directory as empty. The subsequent scaffold writes can then truncate existing starter-named files such as index.mjs, despite SIDE_EFFECTS.md promising that nothing at the destination is overwritten; propagate the listing error or conservatively treat the destination as occupied.
AGENTS.md reference: apps/cli/AGENTS.md:L359-L366
Useful? React with 👍 / 👎.
| } | ||
| } | ||
| for (const owned of RESERVED_SUPABASE_FILES) { | ||
| if (target === join(supabaseDir, owned)) { |
There was a problem hiding this comment.
Fresh evidence after the exact-file guard is that --source supabase/config.json/api still passes because only equality with config.json is checked. When config.json does not yet exist, the command successfully creates it as a directory and scaffolds beneath it; subsequent project loads prefer that path over config.toml and fail while trying to read the directory as JSON, breaking other CLI commands until it is removed. Reject paths at or below these reserved config paths.
AGENTS.md reference: apps/cli/AGENTS.md:L342-L346
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| const exists = yield* fs.exists(options.configPath); | ||
| const text = exists ? yield* fs.readFileString(options.configPath) : ""; |
There was a problem hiding this comment.
When two workers new invocations for different names overlap, both can read the same config.toml snapshot here and render separate complete replacements. Both scaffolds are then created, but the last commitWorkerEntry overwrites the first invocation's newly added table, leaving one worker directory orphaned; an editor save during scaffolding can likewise be silently discarded. Re-read and verify the file is unchanged at commit time, or serialize config updates with a lock.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aaf882552c
ℹ️ 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".
|
|
||
| // `-o` asks for a machine-readable stdout, so nothing human may be written | ||
| // to it — `output.success` logs to stdout in text mode. | ||
| if (yield* legacyEmitWorkersMachineOutput(payload)) { |
There was a problem hiding this comment.
When workers new <name> -o env is invoked, legacyEmitWorkersMachineOutput raises LegacyWorkersEnvNotSupportedError here, but only after the starter files and config.toml entry have been written on lines 230–236. The command therefore exits with an error despite having created the worker; call legacyRejectWorkersEnvOutput before any filesystem work instead of relying on the emitter's late backstop.
Useful? React with 👍 / 👎.
| command: "supabase workers new", | ||
| description: "Scaffold a worker, prompting for runtime and size", |
There was a problem hiding this comment.
This advertised example cannot run because name is now a required Argument.string rather than an optional argument. Users copying supabase workers new receive a missing-argument parse error before the runtime and size prompts described here; add a worker name to the example.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Still present at head — name is a required Argument.string now, so the first advertised example fails to parse before any prompt. While fixing it: the SIDE_EFFECTS.md title still reads supabase workers new [name] (optional-bracket notation from the auto-name design).
| /** Whether `candidate` is `parent` itself or sits underneath it. */ | ||
| function isAtOrUnder(parent: string, candidate: string): boolean { | ||
| const rel = relative(resolve(parent), resolve(candidate)); | ||
| return rel === "" || (!rel.startsWith("..") && !isAbsolute(rel)); |
There was a problem hiding this comment.
For an in-project destination whose first relative component merely begins with two dots, such as --source ..worker from the project root, relative() returns ..worker and this prefix check incorrectly classifies it as outside the project. Only reject rel === ".." or a prefix of ..${sep} so valid directory names beginning with .. remain usable.
Useful? React with 👍 / 👎.
| projectRoot: project.projectRoot, | ||
| target: join(project.workersDir, name), | ||
| subject: `The default directory for "${name}"`, | ||
| suggestion: "Point [workers] root at a directory inside supabase/.", |
There was a problem hiding this comment.
+1, and there are two more copies of the same ghost: the workerSourceDir default-branch suggestion in worker-paths.ts ("Point [workers] root at a directory inside supabase/.") and the handler's module docblock (new.handler.ts:42-49), which still describes scaffolding supabase/<root>/<name>/ and a name "only generated once both questions have been answered" — both removed designs. Since the default directory can only fail confinement when supabase/workers (or an ancestor) is a symlink escaping the project, naming that is the actionable advice.
| // is to create a worker has no business removing whatever happens to share | ||
| // its name — so it says what is in the way and leaves the choice to the user. | ||
| if (!(yield* destinationIsFree(destination))) { | ||
| const shown = displayPath(project.projectRoot, destination); |
There was a problem hiding this comment.
When the command is run from a nested directory, this computes shown relative to the project root rather than runtimeInfo.cwd. For example, from <project>/apps/web, an occupied default destination is reported as supabase/workers/api, so the recovery advice points at <project>/apps/web/supabase/workers/api if followed from the invoking shell instead of the actual destination; pass the invocation directory to displayPath.
Useful? React with 👍 / 👎.
…tput Addresses the eighteen review findings on #6261. Path confinement was string-deep only. `isAtOrUnder` compared resolved paths lexically, so `--source` through an in-project symlink pointing outside it passed, and `--force` deleted the external target. The reserved-directory check was case-sensitive, so `[workers] root = "Functions"` aliased `supabase/functions` on Windows and default macOS filesystems — enough to delete an Edge Function. And `./.` or `services/..` were only rejected before `normalize` reduced them to `.`, which made `supabase/` itself the workers root. There is now one guard, `confineWorkerPath`, that answers containment on the filesystem's terms: it canonicalizes the longest existing prefix of the target (`realPath` fails outright on a path that isn't there yet) and the project root with it, so a project living under a symlink still compares like for like. Both `--source` and the `source` recorded in `config.toml` go through it — the latter previously had no confinement at all, which is a `push` exfiltration path. `workers new --force` could destroy a worker and leave nothing in its place. Every way the `config.toml` edit can fail is deterministic and knowable up front, but it was attempted after the previous directory had been removed and the replacement starter files written. `writeWorkerEntry` is now `planWorkerEntry` + `commitWorkerEntry`, and the handler plans before it touches anything. A regular file at the destination is also handled: it used to read as an empty directory, skip the removal, and fail `EEXIST`. The textual TOML writer missed literal-quoted keys (`'runtime' = 'node'`), appending a duplicate and invalidating the file; wrote control characters verbatim, which a Unix path may legally contain; and could mistake a `[workers.api]` line inside a multiline string for the table itself. The first two are fixed, the third now refuses rather than guessing. Also: `--source` resolves against the invocation directory rather than the project root, matching what a shell prompt implies and the resolver's own tests; persisted paths are normalized to forward slashes so a Windows-written `config.toml` still works elsewhere; the auto-name and config-reuse notices moved to stderr, since `-o json` leaves `output.format` as `text` and they were corrupting the payload; `as` casts in `worker-config.ts` and `worker-runtimes.ts` replaced with a record guard and predicate narrowing; decoded worker entries read into a null-prototype record so a worker named `constructor` is not reported as already configured; the formatter exports carry the mandatory `legacy` prefix; and the `workers` row is out of the frozen `go-cli-divergences.md`, with the Go-parity framing removed from the new comments, docs and helper name. `parseWorkerRuntime`'s claim to make `--runtime Dockerfile` work was simply false — `Flag.choice` validates before any handler runs. The comment is corrected rather than the flag loosened; the case-insensitivity is real and serves hand-written config values.
0b1b9b3 to
ea4ae3e
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ea4ae3e050
ℹ️ 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".
|
|
||
| // `loadProjectConfig` returns null when the directory holds no project yet, | ||
| // which is what lets `workers new` scaffold into a bare one. | ||
| const loaded = yield* loadProjectConfig(projectRoot); |
There was a problem hiding this comment.
When a legacy-compatible config uses a lowercase or mixed-case environment reference in a typed field, such as [api] port = "env(api_port)", this call leaves goViperCompat at its default false. The reference is therefore not interpolated and schema decoding fails because port remains a string, even though other legacy commands accept the same config; load the project with goViperCompat: true.
Useful? React with 👍 / 👎.
| yield* fs.writeFileString(join(destination, filename), contents); | ||
| } | ||
|
|
||
| yield* commitWorkerEntry(configWrite); |
There was a problem hiding this comment.
If config.toml is readable but not writable, or this commit encounters an I/O failure, the error occurs only after the starter files have made the destination non-empty. No cleanup runs, so the config has no worker entry and every retry is rejected as an occupied destination, requiring manual removal; stage the scaffold and config transactionally or remove only the files created by this invocation on failure.
Useful? React with 👍 / 👎.
|
|
||
| for (;;) { | ||
| const real = yield* fs.realPath(cursor).pipe(Effect.option); | ||
| if (real._tag === "Some") { |
There was a problem hiding this comment.
real is a statically known Option, so reading real._tag couples production code to Effect's private runtime representation instead of using Option.isSome; the same new command repeats this pattern in new.handler.ts and workers.shared.ts, so replace all three checks with the public Option helpers.
AGENTS.md reference: AGENTS.md:L169-L171
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed — the rule is verbatim in the repo guidance ("Do not inspect Effect runtime representations through fields such as ._tag ... use Option.isSome, Option.isNone"), and these are new occurrences, not inherited ones: worker-paths.ts:67, new.handler.ts:116, workers.shared.ts:118. The tree has essentially one pre-existing production offender (legacy-upgrade-suggest.ts), so this is not an established pattern being matched. Rename-level fix.
|
|
||
| return { | ||
| configPath: options.configPath, | ||
| text: appendTomlSection(text, header, options.patch), |
There was a problem hiding this comment.
When a valid config represents the section as an inline table, such as workers = {} or workers = { other = { runtime = "node" } }, the requested name is absent and this appends [workers.api]. TOML inline tables are sealed and cannot be extended afterward, so the command writes an invalid config.toml after scaffolding the worker; detect this representation before filesystem changes or rewrite it without extending the inline table.
Useful? React with 👍 / 👎.
| if (goFormat === undefined || goFormat === "pretty") { | ||
| return false; |
There was a problem hiding this comment.
When users combine -o pretty with --output-format json or stream-json, this branch treats the explicit legacy flag as absent; the handler then follows output.format and emits a JSON envelope instead of the requested human-readable result. Handle pretty explicitly so --output retains its documented priority over --output-format.
AGENTS.md reference: apps/cli/AGENTS.md:L300-L300
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This one I would push back on: treating pretty like undefined and deferring to --output-format is the established pattern, not a deviation. The canonical handler the priority invariant points at does exactly this — backups/list/list.handler.ts:150-155: "goFmt is undefined or "pretty" — defer to TS --output-format for JSON/stream-json, otherwise render the Glamour-styled table". -o pretty selects the human rendering, and --output-format json still wins the envelope in every legacy command; changing workers here would make it the odd one out. No change needed.
Workers record their runtime, instance size, instance count and source directory in `supabase/config.toml`, keyed `[workers.<name>]`, next to the `[functions.<slug>]` entries already in the same file. The section is a plain `Schema.Record`: one sub-table per worker and no project-wide scalar sitting beside them, so there is nothing for the index signature to collide with. Worker names are DNS labels, matching what the Management API validates its `:name` path parameter against, since they end up in hostnames. `instances` is bounded as a non-negative integer to match `spec.instances` in the API's own input schema — a value that gets past the schema is dropped rather than sent, so leaving it unbounded silently deploys a different count than the config asked for. The section flows into the published `schema.json`, so editors offer completion for it in `config.toml`. That asset is served at PROJECT_CONFIG_SCHEMA_URL and stamped into every `config.toml` that `saveProjectConfig` writes, so a stale copy makes editors flag valid config as invalid. Most of that asset's diff is not workers. `toJsonSchemaDocument` changed how it emits unions between effect beta.107 and rc.108, and the bump landed on develop without the asset being regenerated, so inline `Infinity`/`NaN` unions collapse into `$defs` refs throughout — regenerating on the parent commit alone produces ~549 of those deletions. Nothing wires the generator into a script or CI job, so the drift is silent. Worth fixing separately.
ea4ae3e to
7baf0ee
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7baf0eeac3
ℹ️ 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".
| ["Access", "public"], | ||
| ]), | ||
| ); | ||
| yield* output.raw(`Deploy it with supabase workers push ${name}.\n`); |
There was a problem hiding this comment.
After every successful text-mode scaffold, this tells the user to run supabase workers push <name>, but workers.command.ts registers only the new subcommand, and a repo-wide search finds no workers push handler or proxy. Following the advertised next step therefore produces an unknown-subcommand error; omit this guidance until push exists or point users to an available deployment path.
Useful? React with 👍 / 👎.
| No custom events — only the `cli_command_executed` that the instrumentation | ||
| wrapper emits for every command. |
There was a problem hiding this comment.
When parsing fails—for example because the required worker name is missing or --runtime has an invalid choice—the Command.withHandler callback in new.command.ts never runs, so neither withLegacyCommandInstrumentation nor the handler's telemetry-state flush executes. This blanket statement therefore incorrectly promises cli_command_executed for failures that the same checklist lists; document the parse boundary or instrument those failures before treating the event as universal.
AGENTS.md reference: apps/cli/AGENTS.md:L359-L366
Useful? React with 👍 / 👎.
| * Worker names end up in hostnames, so they are DNS labels — the same pattern | ||
| * the Management API validates the `:name` path parameter against. | ||
| */ | ||
| const WORKER_NAME_PATTERN = /^[a-z0-9]([a-z0-9-]{0,61}[a-z0-9])?$/; |
There was a problem hiding this comment.
On Windows, this accepts reserved device names such as con, prn, aux, nul, com1, and lpt1 as valid worker names even though the name also becomes the default directory. A normal workers new con therefore gets past validation and fails later at makeDirectory or the starter-file writes with a raw filesystem error; reject Windows device basenames up front so every accepted default scaffold is portable.
Useful? React with 👍 / 👎.
kanadgupta
left a comment
kanadgupta
left a comment
There was a problem hiding this comment.
Played around locally and looks good! There's a lot of codex garbage in here (sorry!) but there are a few comments in here that are worth addressing that I've flagged below.
|
|
||
| for (;;) { | ||
| const real = yield* fs.realPath(cursor).pipe(Effect.option); | ||
| if (real._tag === "Some") { |
There was a problem hiding this comment.
Agreed — the rule is verbatim in the repo guidance ("Do not inspect Effect runtime representations through fields such as ._tag ... use Option.isSome, Option.isNone"), and these are new occurrences, not inherited ones: worker-paths.ts:67, new.handler.ts:116, workers.shared.ts:118. The tree has essentially one pre-existing production offender (legacy-upgrade-suggest.ts), so this is not an established pattern being matched. Rename-level fix.
| if (goFormat === undefined || goFormat === "pretty") { | ||
| return false; |
There was a problem hiding this comment.
This one I would push back on: treating pretty like undefined and deferring to --output-format is the established pattern, not a deviation. The canonical handler the priority invariant points at does exactly this — backups/list/list.handler.ts:150-155: "goFmt is undefined or "pretty" — defer to TS --output-format for JSON/stream-json, otherwise render the Glamour-styled table". -o pretty selects the human rendering, and --output-format json still wins the envelope in every legacy command; changing workers here would make it the odd one out. No change needed.
| command: "supabase workers new", | ||
| description: "Scaffold a worker, prompting for runtime and size", |
There was a problem hiding this comment.
Still present at head — name is a required Argument.string now, so the first advertised example fails to parse before any prompt. While fixing it: the SIDE_EFFECTS.md title still reads supabase workers new [name] (optional-bracket notation from the auto-name design).
| projectRoot: project.projectRoot, | ||
| target: join(project.workersDir, name), | ||
| subject: `The default directory for "${name}"`, | ||
| suggestion: "Point [workers] root at a directory inside supabase/.", |
There was a problem hiding this comment.
+1, and there are two more copies of the same ghost: the workerSourceDir default-branch suggestion in worker-paths.ts ("Point [workers] root at a directory inside supabase/.") and the handler's module docblock (new.handler.ts:42-49), which still describes scaffolding supabase/<root>/<name>/ and a name "only generated once both questions have been answered" — both removed designs. Since the default directory can only fail confinement when supabase/workers (or an ancestor) is a symlink escaping the project, naming that is the actionable advice.
|
|
||
| | Path | Format | When | | ||
| | ----------------------------------------------- | ------ | ------------------------------------------------------------------------- | | ||
| | `<workdir>/supabase/config.toml` | TOML | always — appends/updates `[workers.<name>]` in place, preserving comments | |
There was a problem hiding this comment.
+1 — still stale at ea4ae3e. Concretely: Files Written says "appends/updates [workers.<name>] in place" (writes are append-only now and an existing entry is refused outright); Exit Codes still lists "unknown runtime/size" (those errors were deleted — --runtime/--size are choice flags and recorded values are no longer read) and "records a worker in a form that cannot be edited safely" (now: a worker that is already recorded, in any form); and the SUPABASE_ACCESS_TOKEN row claims a keyring/access-token-file fallback this command's runtime never builds. This file is the compatibility checklist, so it should describe the shipped refusal semantics.
…/cli into FUNC-753/workers-config
…53/workers-config
7baf0ee to
0b1b9b3
Compare
The Go baseConfig is decoded with UnmarshalExact, so any top-level key it does not model is a hard parse error. Once the published JSON schema advertises [workers], a hand-written section breaks every Go-delegated path that calls flags.LoadConfig. Add an ignored Workers field so the delegated child accepts what the TS schema accepts.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b1b9b3376
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return { | ||
| projectRoot, | ||
| supabaseDir, | ||
| configPath: loaded?.path ?? join(supabaseDir, "config.toml"), |
There was a problem hiding this comment.
When a project contains supabase/config.json (including when both config formats exist), loadProjectConfig prefers JSON and this assigns that JSON path to configPath; planWorkerEntry then appends a TOML table to the JSON text and commitWorkerEntry writes the invalid hybrid back to config.json. Load this TOML-specific command with tomlOnly: true and keep the write target at supabase/config.toml, or implement a format-aware JSON update.
AGENTS.md reference: apps/cli/AGENTS.md:L359-L366
Useful? React with 👍 / 👎.
|
|
||
| // `loadProjectConfig` returns null when the directory holds no project yet, | ||
| // which is what lets `workers new` scaffold into a bare one. | ||
| const loaded = yield* loadProjectConfig(projectRoot); |
There was a problem hiding this comment.
When --workdir or SUPABASE_WORKDIR explicitly selects a directory below another Supabase project and the selected directory has no local config yet, this default loadProjectConfig call searches upward and loads the ancestor's config. The scaffold is created under the explicitly selected workdir, but its worker table is appended to the ancestor project's config, unexpectedly modifying a different project and leaving the two locations inconsistent; pass search: false because LegacyCliConfig.workdir has already resolved the intended project directory.
AGENTS.md reference: apps/cli/AGENTS.md:L359-L366
Useful? React with 👍 / 👎.
| if (target === supabaseDir) { | ||
| return yield* refuse("is the supabase directory itself"); | ||
| } |
There was a problem hiding this comment.
When a new or empty project uses --source supabase/workers, this guard accepts the workers root itself, so the starter files are written directly into the directory intended to contain per-worker subdirectories and the entry records that whole root as this worker's source. Subsequent default workers are then created inside the first worker's configured source, causing consumers of that source to include sibling workers; reject equality with workersDir(projectRoot) while continuing to allow individual descendants.
Useful? React with 👍 / 👎.
|
|
||
| const projectRoot = yield* canonicalize(options.projectRoot); | ||
| const target = yield* canonicalize(options.target); | ||
| const supabaseDir = join(projectRoot, "supabase"); |
There was a problem hiding this comment.
When <project>/supabase is itself a symlink to another directory inside the project, target is canonicalized through that symlink but supabaseDir remains the lexical symlink path, so every equality and containment check below compares different aliases. Inputs such as --source supabase/functions/api can consequently create a scaffold inside the CLI-owned Functions tree when that destination does not yet exist; canonicalize supabaseDir before deriving and checking all reserved paths.
AGENTS.md reference: apps/cli/AGENTS.md:L342-L346
Useful? React with 👍 / 👎.
# Conflicts: # apps/cli/package.json
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 66a83831d6
ℹ️ 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".
| if (!isAtOrUnder(projectRoot, target)) { | ||
| return yield* refuse("resolves outside the project"); | ||
| } | ||
| if (target === supabaseDir) { |
There was a problem hiding this comment.
On Windows in a project where the supabase directory does not exist yet, --source SUPABASE canonicalizes only the existing project-root ancestor and preserves the pending component's casing. The case-sensitive equality therefore misses that this is the reserved Supabase root, and the command succeeds by writing the starter directly beside the newly created config.toml; later deployment can treat the entire Supabase configuration directory as the worker source. Use a filesystem-aware containment/equality check instead of string equality.
Useful? React with 👍 / 👎.
| request: HttpClientRequest.HttpClientRequest, | ||
| ): Effect.Effect<HttpClientResponse.HttpClientResponse, HttpClientError.HttpClientError> => | ||
| Effect.sync(() => { | ||
| const bytes = request.body._tag === "Uint8Array" ? request.body.body : new Uint8Array(0); |
There was a problem hiding this comment.
Fresh evidence beyond the previously noted Option sites is that this new HTTP mock also branches directly on request.body._tag. This couples the shared worker tests to Effect's private request-body representation, so an internal representation change can break all consumers of this helper even when the public API remains compatible; narrow the body with a public predicate or Match helper instead.
AGENTS.md reference: AGENTS.md:L171-L178
Useful? React with 👍 / 👎.
|
|
||
| | Path | Format | When | | ||
| | ---------------------------------------- | ---------- | ----------------------------------------------------------------------------------------------------------- | | ||
| | `<workdir>/supabase/config.toml` | TOML | always, to refuse a worker that is already recorded | |
There was a problem hiding this comment.
legacyLoadWorkersProject calls loadProjectConfig, which reads and parses supabase/.env and supabase/.env.local when a project config exists before decoding it. Their values affect config interpolation, and a malformed dotenv file aborts the command before scaffolding, but neither read appears in this compatibility checklist; add both paths and their conditions to the Files Read table.
AGENTS.md reference: apps/cli/AGENTS.md:L359-L366
Useful? React with 👍 / 👎.
| | `cli_command_executed` | post-run, success or failure (via wrapper) | `exit_code`, `duration_ms`, `flags` | | ||
|
|
||
| No custom events — only the `cli_command_executed` that the instrumentation | ||
| wrapper emits for every command. |
There was a problem hiding this comment.
This checklist ends after telemetry and omits the template's required Output section, even though the command has distinct text, JSON, stream-JSON, and legacy -o payload behavior. Because this file is the primary E2E compatibility input, add the exact stdout shape for each supported output mode so future changes can preserve the command's observable contract.
AGENTS.md reference: apps/cli/AGENTS.md:L359-L366
Useful? React with 👍 / 👎.
The bare @supabase/config entrypoint is the pure, browser-safe surface after CLI-2231; loadProjectConfig now lives on ./effect. Importing it from the bare specifier broke the bun bundle and the docs-spec script.
`real._tag === "Some"` and `info._tag === "None"` couple production code to Effect's runtime representation, which the repo guidance rules out: "Do not inspect Effect runtime representations through fields such as `._tag` ... use `Option.isSome`, `Option.isNone`". Three call sites, all introduced by this command family rather than inherited: the canonicalize walk in `worker-paths.ts`, the destination-is-free check in `new.handler.ts`, and the scaffolded-directory scan in `workers.shared.ts`. `isSome`/`isNone` are declared as type guards, so the narrowing that followed each check still holds and nothing else moves.
`supabase workers new` cannot run: `name` is a required `Argument.string`, so the parser rejects the invocation before the runtime and size prompts the description advertises ever fire. Anyone copying the first example gets a missing-argument error. Left over from the generated-name design, which made the argument optional and assigned a name when it was omitted; that was removed in "drop [workers] root, generated names, and three other one-liners" but the example was not.
`[workers] root` was removed in "drop [workers] root, generated names, and three other one-liners" — `[workers.<name>] source` already puts a worker anywhere in the repo — but four references to it survived, two of them in suggestion strings a user actually sees. The two suggestions told users to point a config key at a directory when that key does not exist and is not the problem. The default directory is `supabase/workers/<name>` with an already-validated name, so it cannot be the project root, `supabase/`, or a directory the CLI owns; a symlink escaping the project is the only way it fails confinement. Both now name that, and offer the escape hatch each caller actually has — `--source` when scaffolding, a recorded `source` when resolving. The two docblocks: `resolveWorkerSource` explained the `functions/`/`migrations/` refusal by analogy to a key that is gone, and now gives the reason directly; `workerSourceDir` referred to a symlinked `[workers] root` where it means `supabase/workers`. Also updates the `new.handler.ts` module docblock, which still described scaffolding into `supabase/<root>/<name>/` and a name "only generated once both questions have been answered" — the removed generated-name design.
…_EFFECTS The checklist still described three designs that were removed and one capability the command never had. It is the compatibility contract and the primary input to the e2e suite, so it has to match what ships. - Title: `[name]` was optional-bracket notation from the generated-name design. `name` is a required argument. - Files Written: "appends/updates `[workers.<name>]` in place" — writes are append-only, and a worker already recorded is refused outright, before the prompts and before anything reaches disk. Says so, and drops "always" from the three rows that only happen on success. - Files Read: `config.toml` is decoded to answer the already-recorded question and then re-read as text to append to, which is two reads worth naming. - Exit codes: "unknown runtime/size" cannot happen — `--runtime` and `--size` are `Flag.choice`, so the parser rejects anything outside the catalog and the recorded values are never read back. "Reserved worker name" went with `RESERVED_WORKER_NAMES`; `validateWorkerNameMessage` is one pattern test. "Records a worker in a form that cannot be edited safely" is now simply a worker that is already recorded, in any form. - `SUPABASE_ACCESS_TOKEN`: the row advertised a keyring → `~/.supabase/access-token` fallback. This command's runtime layer is the CLI config, telemetry state and command runtime — no credentials service, no API client, and nothing that reads the token. Row removed rather than reworded. - Telemetry: `cli_command_executed` is emitted by the `Command.withHandler` wrapper, so a failure the parser catches — a missing name, a `--runtime` outside the choice list — never reaches it, and `telemetry.json` is not written either.
`legacyLoadWorkersProject` called `loadProjectConfig(projectRoot)` with no
options, and the loader prefers `supabase/config.json` when one exists. In a
JSON project `configPath` was therefore the JSON file, and `commitWorkerEntry`
appended a `[workers.<name>]` TOML table to it — after the scaffold was already
written, leaving the project config unparseable.
Two layers, per the review:
`tomlOnly: true` at the call site. The entry writer is a TOML text editor, so
the loader has to resolve the file that editor can actually edit. `functions
new` avoids the same trap by joining `supabase/config.toml` directly; this is
that, through the loader.
`planWorkerEntry` now parses what it rendered before returning it, and checks
the new table reads back out. Appending text to a file this code did not write
is a syntactic operation, and the only honest check is to read the result. That
also closes the sealed-inline-table case: `workers = {}` cannot be extended by
appending `[workers.api]`, and the name is absent from the decoded section, so
the already-configured check never fired. Both refusals land before the
scaffold, like every other refusal in this handler.
A JSON project now gets its worker recorded in `config.toml`, which the default
loader lists in `ignoredPaths`, and its `config.json` left byte-for-byte alone.
That gap is documented rather than fixed here: writing JSON means either losing
the comment preservation `appendTomlSection` exists for, or a second surgical
editor, and a holistic overhaul of config writing is planned.
Also drops the last `[workers] root` reference, in `worker-config.ts`'s
module docblock.
# Conflicts: # packages/config/src/io.unit.test.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 72ec632d1f
ℹ️ 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".
| | Variable | Purpose | Required? | | ||
| | ------------------ | --------------------------------------- | ------------------------------------------------------ | | ||
| | `SUPABASE_PROFILE` | built-in profile name or YAML file path | no (falls back to `~/.supabase/profile` -> `supabase`) | | ||
| | `SUPABASE_WORKDIR` | project directory the command acts on | no (falls back to `--workdir`, then the ancestor walk) | |
There was a problem hiding this comment.
When both --workdir and SUPABASE_WORKDIR are set, this row reverses the implemented precedence: resolveWorkdir checks the flag first, then the environment variable, then performs the ancestor walk. The compatibility checklist therefore identifies the wrong project as the command target for this scenario, which can produce incorrect E2E expectations about which config.toml and worker directory are modified; document the order as --workdir → SUPABASE_WORKDIR → ancestor search.
AGENTS.md reference: apps/cli/AGENTS.md:L359-L366
Useful? React with 👍 / 👎.
| const entries = yield* fs.readDirectory(target).pipe(Effect.orElseSucceed(() => [])); | ||
| return entries.length === 0; |
There was a problem hiding this comment.
When the destination directory exists but cannot be listed, this converts permission and I/O failures into an empty entry list and declares the directory free. On a write-only/searchable directory, the subsequent writes can still create files and can truncate an existing starter filename that the preflight could not see, violating the command's no-overwrite behavior; only an actually successful empty listing should return true, while readDirectory failures should abort before scaffolding.
Useful? React with 👍 / 👎.
|
|
||
| return yield* confineWorkerPath({ | ||
| projectRoot: options.projectRoot, | ||
| target: resolve(options.cwd, options.raw.replace(/[/\\]+$/, "")), |
There was a problem hiding this comment.
When --source is a filesystem root, the trailing-separator removal changes its meaning before resolution: on POSIX, / becomes an empty string and therefore resolves to cwd rather than / (and Windows drive roots have the analogous C:\ → C: problem). Invoked from an empty nested directory inside the project, workers new api --source / can consequently scaffold successfully into that nested directory and persist it as the source instead of rejecting the requested outside-project path; pass the raw path to resolve, which already normalizes harmless trailing separators without collapsing roots.
Useful? React with 👍 / 👎.
…upabase#6260) ## Summary Adds the `[workers]` section to the project config schema: a `Schema.Record` of worker name to `{ runtime, size, instances, source }`, mirroring the `[functions.<slug>]` convention in the same file. The same schema is used for the project config and for `[remotes.*]`, so a remote can carry its own worker overrides. Two constraints live at the schema level: - **Worker names are DNS labels**, matching what the Management API validates its `:name` path parameter against, since they end up in hostnames. - **`instances` is a non-negative integer**, matching `spec.instances` in the API's own input schema. A value that gets past the schema is dropped rather than sent, so leaving it unbounded means a `push` silently deploys a different count than the config asked for. There is no project-wide scalar in the table — an earlier revision had a `[workers] root` for relocating the grouping directory, and it was dropped because `[workers.<name>] source` already puts a worker anywhere in the repo. That keeps `workers` a plain record with nothing for the index signature to collide with, rather than a `StructWithRest` needing a key-pattern exclusion that vanished under the `disableChecks: true` `io.ts` uses for unselected remotes. No CLI surface consumes this yet — it lands first so the schema and its generated types are reviewable on their own. **Stack 1 of 4.** Followed by `workers new` (supabase#6261), `workers push` (supabase#6262), and `workers list`/`status`/`delete` (supabase#6263). ## Linked issue FUNC-753 (Linear). 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 Gupta <git@kanad.dev>
kanadgupta
left a comment
kanadgupta
left a comment
There was a problem hiding this comment.
two non-blocking comments, happy to merge afterwards!
There was a problem hiding this comment.
(nit, can definitely happen in a follow-up)
Product feedback: Node has native TypeScript support for all LTS versions. Do we need to have this in .mjs?
2 participants
Summary
Adds
supabase workers new, plus the project layout andconfig.tomlediting thewhole command family builds on:
shared/workers/— worker path resolution,config.tomlsection reading andpatching (
toml-section.tspreserves surrounding formatting), the runtime/sizeenvelope, and the starter files.
shared/workers/stacks/<runtime>/rather than string literals, and are embedded into the compiled binary through a
Bun macro — the directory is expanded at transpile time and inlined.
A completeness check inside the macro fails the build if
WORKER_RUNTIMESandthe directory drift apart.
Stack 2 of 4, on top of the config schema (#6260).
Reviewer note: the third commit is where the embedding mechanism is explained; the
starters are deliberately kept out of the type program (a
denostarter is notvalid under this workspace's Bun types), which is why
tsconfig.jsonexcludes thedirectory and nothing imports the files.
Linked issue
FUNC-753 (Linear). Supabase maintainer, exempt from the
open-for-contributionflow.Checklist