feat(cli): add supabase workers push - #6262
Conversation
aa0ea27 to
fa9be15
Compare
fa9be15 to
959520b
Compare
959520b to
d38a32b
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: 959520b26b
ℹ️ 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".
![depthfirst-app[bot]](https://avatars.githubusercontent.com/in/1021002?s=60&v=4){kind=small}
Supabase CLI previewnpx --yes https://pkg.pr.new/supabase/cli/supabase@c51d62ecbdd01bdc186b942a79d0570f9639a93cPreview package for commit |
d38a32b to
e7acc02
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e7acc025f1
ℹ️ 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".
… the upload URL Addresses the review findings on #6262. Two failures happened only after the remote project had already changed. `-o env` cannot encode the payload's `workers` array, but that was discovered at emit time — after every upload, deploy and build poll — so the command exited non-zero having deployed, inviting a retry that deployed again. And an absent optional `image_version` was left in the payload as `undefined`, which smol-toml cannot represent, so `-o toml` threw at the same point. Both are now settled before the first request: `legacyRejectUnsupportedWorkersOutput` runs up front, and the field is spread conditionally like `url` beside it. Packaging silently tolerated a filesystem it could not read. An unreadable file was archived as zero bytes and an unreadable directory dropped its whole subtree, so `push` reported success for an image built from an application with a hole in it. Both propagate now. The redundant `Number()` around `File.Info.mode` is gone too — it is a plain number, and wrapping it invited the reading that it was an `Option`. The presigned upload URL was reaching the `--debug` log. Its query string is a write-capable credential for the archive a deploy is about to build from, so it does not belong in terminal scrollback or a CI log. Fixed at the logging boundary rather than by giving the upload its own HTTP client: redaction in `legacyHttpClientLayer` keeps the client injectable for tests and covers every presigned URL the CLI might ever log, not just this one. Also: the build-poll read retries on a wall-clock budget instead of three back-to-back attempts, which a two-second blip exhausted while the surrounding poll still had minutes left; `WorkersApiUnexpectedStatusError` classifies from the status it carries, so a 401 reads as "log in" rather than as a service failure across every Workers endpoint; a source of nothing but empty directories is refused before an upload slot is minted, instead of deploying an image with no handler; the runtime guess is only reported once the source is known to exist; and `config.toml` loading moved inside the finalizers, so a malformed config still flushes telemetry — matching `config/push`, which already had it right. Two findings are deliberately left for follow-ups: streaming the build context rather than buffering it, and an ignore mechanism so `.env` and `.git` can be kept out of the uploaded archive. Both are new work rather than defect fixes. The `SIDE_EFFECTS.md` also now records the telemetry and linked-project writes, the cache-miss API route, and why there is no live suite yet.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 38fd3f44bf
ℹ️ 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".
…on a clock Four follow-ups on #6262, three of them corners the previous round left open. `fs.stat` was the last swallowed filesystem error in `collectEntries`. An entry that disappears between `readDirectory` and the stat, or whose metadata cannot be read, was dropped from the archive — the same "deploy an application with a hole in it" outcome as the reads that were fixed, just losing a whole entry instead of its contents. `createTar` throws `TarPathTooLongError` for a name USTAR cannot represent, and calling it directly inside the generator made that a defect rather than a typed failure. `withJsonErrorHandling` only catches the failure channel, so `--output-format json` would have terminated with no structured error payload for an error the class explicitly declares as user-actionable. Wrapped in `Effect.try`, narrowing to that class and letting anything else stay a defect — because anything else there really is a bug. The transient-failure integration test was waiting on the real clock. The outer poll schedule was injected but the per-read retry was not, and that one is spaced in seconds, so a 500-then-200 sequence sat for two seconds and more transient responses would have pushed it toward thirty. Both schedules are parameters now; that test went from ~2s to 22ms. `--instances` is bounded at the parser, the way `[workers.<name>] instances` is bounded in the config schema. `--instances=-1` was accepted by the integer flag and carried into the deploy spec, so an impossible scaling request reached the remote endpoint only after the build context had been packaged and uploaded. `Flag.filter` refuses it during parsing, consistent with how `--runtime` is validated.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eec654d4ca
ℹ️ 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".
| const requested = | ||
| flags.names.length > 0 | ||
| ? yield* Effect.forEach(flags.names, legacyValidateWorkerName) | ||
| : yield* legacyDiscoverWorkerNames(project); |
There was a problem hiding this comment.
When workers push is invoked without names and the workers root cannot be listed, or metadata for one entry cannot be read, legacyDiscoverWorkerNames in workers.shared.ts:122-128 converts those failures to []/None. This new call can therefore deploy only the config-listed or remaining workers and exit successfully while silently omitting directory-only workers, contrary to the promise to deploy every worker; preserve discovery failures in the Effect failure channel instead of treating them as absence.
AGENTS.md reference: AGENTS.md:L64-L68
Useful? React with 👍 / 👎.
| > here would be permanently skipped or permanently red. Revisit when the v2 | ||
| > Workers routes are available on that stack. | ||
|
|
||
| ## Files Read |
There was a problem hiding this comment.
After the prior cache and telemetry additions, the compatibility document still omits side effects performed by the resolver and auth layers: legacy-project-ref.layer.ts:87-95 consumes SUPABASE_PROJECT_ID, reads <workdir>/supabase/.temp/project-ref, and may call GET /v1/projects for interactive selection, while legacy-credentials.layer.ts:403-443 reads the profile and legacy keyring entries or <SUPABASE_HOME>/access-token. These happen before the Workers requests and leave the Files Read, API Routes, and Environment Variables sections incomplete.
AGENTS.md reference: apps/cli/AGENTS.md:L359-L366
Useful? React with 👍 / 👎.
| const info = yield* fs.stat(absolutePath); | ||
|
|
||
| const modified = info.mtime; | ||
| const mtime = modified._tag === "Some" ? Math.floor(modified.value.getTime() / 1000) : 0; |
There was a problem hiding this comment.
When a source file has a filesystem mtime before the Unix epoch, this calculation produces a negative value, and writeOctal later serializes it as a field such as 000000000-1. That is not a valid USTAR octal timestamp—GNU tar reports a malformed numeric time and exits unsuccessfully—so an otherwise valid source tree can produce a build context the server cannot extract; clamp pre-epoch timestamps to 0, as already done for absent mtimes, or encode signed values correctly.
Useful? React with 👍 / 👎.
| return yield* Effect.fail( | ||
| new WorkerBuildTimeoutError({ | ||
| detail: `"${name}" was still building when this command stopped waiting.`, | ||
| suggestion: `Check on it with \`supabase workers status ${name}\`.`, |
There was a problem hiding this comment.
When a build exceeds the polling budget, this recovery instruction tells the user to run supabase workers status <name>, but a repository-wide search shows that the Workers command registers only new and push; there is no workers status handler or alias in this commit. The user therefore waits ten minutes and is then directed to a command that only returns an unknown-command error, so the suggestion should point to an available dashboard/API path or the status command should ship with this workflow.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 715ed177bd
ℹ️ 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 yield* Effect.fail( | ||
| new WorkerSourceMissingError({ | ||
| detail: `${sourceDisplay} is empty, so there is nothing to deploy.`, | ||
| suggestion: `Add your code there, or re-scaffold it with \`supabase workers new ${name} --force\`.`, |
There was a problem hiding this comment.
When the source directory is empty, this recovery instruction recommends supabase workers new <name> --force, but new.command.ts no longer defines a force flag and the existing directory would prevent a normal re-scaffold. Following the suggestion therefore produces an unknown-option error instead of repairing the worker; either describe removing the empty directory before rerunning new, or provide another currently supported recovery path. The same stale suggestion also appears in the empty-subdirectory branch.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed: new.command.ts defines no --force flag, so this recovery command exits with an unknown-option error. Note the identical suggestion string also appears further down in this file (the fileCount === 0 guard, ~line 220) — both need the same rewording. Since new also refuses an existing directory, "remove the empty directory, then re-run supabase workers new <name>" is probably the honest instruction.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 49ee4a9a73
ℹ️ 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".
|
|
||
| for (const marker of MARKERS) { | ||
| for (const file of marker.files) { | ||
| const found = yield* fs.exists(join(dir, file)).pipe(Effect.orElseSucceed(() => false)); |
There was a problem hiding this comment.
When a worker has no configured runtime and an fs.exists call fails transiently or because the marker cannot be inspected, this converts the failure to false and continues classification. If the subsequent package walk succeeds, a Node or Dockerfile worker can be uploaded and deployed as the default Deno runtime instead of failing before remote side effects; preserve the filesystem failure in the Effect channel rather than treating it as marker absence.
AGENTS.md reference: AGENTS.md:L64-L68
Useful? React with 👍 / 👎.
| const archive = yield* Effect.try({ | ||
| try: () => gzipSync(createTar(entries)), | ||
| catch: (cause) => { | ||
| if (cause instanceof TarPathTooLongError) { |
There was a problem hiding this comment.
When a source entry has an mtime beyond USTAR's 11-digit octal limit (for example, a filesystem timestamp after roughly 2242), createTar throws the explicitly user-actionable TarFieldTooLargeError, but this catch only returns TarPathTooLongError and rethrows the former as a defect. Consequently --output-format json and stream-json terminate without their structured error payload; return both declared tar-limit errors through the typed failure channel.
AGENTS.md reference: apps/cli/AGENTS.md:L399-L409
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Reinforcing this one: the narrow contradicts the error's own declaration. TarFieldTooLargeError carries actionability.invalidInput and its doc comment says it is user-actionable, yet this catch rethrows it as a defect — so the class's classification can never take effect and --output json exits unstructured. Widening the narrow to both tar error classes is a one-liner; the "anything else is a bug" comment stays true for genuine gzipSync failures. (Also note an 8 GiB file trips this via the size field, which is a far more plausible input than a year-2242 mtime.)
| const spec: WorkerDeploySpec = { | ||
| // A plain Dockerfile build has no catalog runtime to name; the uploaded | ||
| // context carries its own Dockerfile and is built as-is. | ||
| ...(runtime === "dockerfile" ? {} : { runtime }), |
There was a problem hiding this comment.
When a worker is explicitly configured with runtime = "dockerfile" but its nonempty source has no usable top-level Dockerfile, this omits the catalog runtime and deploys a context that the server cannot build. The CLI already has all of the source metadata before minting the upload slot, so reject a missing or non-file Dockerfile locally rather than uploading, starting a deployment, and waiting for the inevitable remote build failure.
Useful? React with 👍 / 👎.
| contents: new Uint8Array(0), | ||
| mode: 0o777, | ||
| mtime: 0, | ||
| linkTarget: linkTarget.value, |
There was a problem hiding this comment.
When a worker source is a pnpm workspace package, dependency links under its node_modules commonly target the repository-level node_modules/.pnpm directory via paths such as ../../../node_modules/.pnpm/.... This archives only the symlink while the target lies outside the worker source and is therefore absent from the uploaded context, so the remote catalog build either rejects the link or produces an image with missing dependencies. Materialize confined external targets into the archive or reject these links before upload with actionable guidance.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 86d0f6bd28
ℹ️ 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".
| // An empty directory packages and deploys perfectly happily, producing an | ||
| // image with nothing in it — a success message for a worker that cannot | ||
| // serve anything. Refuse before uploading rather than after. | ||
| const contents = yield* fs.readDirectory(worker.sourceDir).pipe(Effect.orElseSucceed(() => [])); |
There was a problem hiding this comment.
When the source directory exists but cannot be listed because of permissions or an I/O error, this fallback converts the filesystem failure to an empty entry list. The command then incorrectly reports that the source is empty and offers source-editing remediation instead of exposing the actual filesystem error; preserve the PlatformError and only treat a successful empty listing as empty.
AGENTS.md reference: AGENTS.md:L64-L68
Useful? React with 👍 / 👎.
| }); | ||
| } | ||
|
|
||
| const body = yield* response.json.pipe(Effect.mapError(mapRequestError(operation))); |
There was a problem hiding this comment.
When any Workers endpoint returns a successful status with empty or malformed JSON, response.json fails with an HTTP decode error, but mapRequestError converts every HttpClientError into WorkersApiNetworkError. This tells users to check their network and records externalNetwork even though the server response was reached; the same pattern affects get, upload-slot creation, and deploy responses, so decode failures should retain an API-response classification such as WorkersApiUnexpectedStatusError.
AGENTS.md reference: apps/cli/AGENTS.md:L370-L372
Useful? React with 👍 / 👎.
| const worker = yield* getWorker(api, projectRef, name).pipe( | ||
| Effect.retry({ schedule: options.retrySchedule ?? WORKER_POLL_READ_RETRY }), | ||
| ); |
There was a problem hiding this comment.
When a build poll receives a deterministic failure such as 401, 403, or another non-retryable 4xx response, this unconditional retry applies the full 30-second schedule before returning the actionable error. This is especially plausible when credentials expire during a long build, and it makes the command appear hung despite retries being unable to change the outcome; filter the retry schedule to transport errors and transient statuses such as 429 or 5xx.
Useful? React with 👍 / 👎.
| : HttpClientRequest.put(slot.url) | ||
| ).pipe(HttpClientRequest.bodyUint8Array(archive, "application/gzip")); | ||
|
|
||
| const response = yield* client.execute(request).pipe( |
There was a problem hiding this comment.
When the object-store endpoint accepts the connection but stalls while receiving the archive or returning its response, this direct HttpClient.execute has no deadline and can leave workers push pending indefinitely. Unlike the Management API client, which wraps requests in a 60-second timeout, this upload bypasses that retry/timeout policy; apply an explicit upload deadline, potentially bounded by the slot's expiry, so the command eventually returns WorkerUploadFailedError.
Useful? React with 👍 / 👎.
… the upload URL Addresses the review findings on #6262. Two failures happened only after the remote project had already changed. `-o env` cannot encode the payload's `workers` array, but that was discovered at emit time — after every upload, deploy and build poll — so the command exited non-zero having deployed, inviting a retry that deployed again. And an absent optional `image_version` was left in the payload as `undefined`, which smol-toml cannot represent, so `-o toml` threw at the same point. Both are now settled before the first request: `legacyRejectUnsupportedWorkersOutput` runs up front, and the field is spread conditionally like `url` beside it. Packaging silently tolerated a filesystem it could not read. An unreadable file was archived as zero bytes and an unreadable directory dropped its whole subtree, so `push` reported success for an image built from an application with a hole in it. Both propagate now. The redundant `Number()` around `File.Info.mode` is gone too — it is a plain number, and wrapping it invited the reading that it was an `Option`. The presigned upload URL was reaching the `--debug` log. Its query string is a write-capable credential for the archive a deploy is about to build from, so it does not belong in terminal scrollback or a CI log. Fixed at the logging boundary rather than by giving the upload its own HTTP client: redaction in `legacyHttpClientLayer` keeps the client injectable for tests and covers every presigned URL the CLI might ever log, not just this one. Also: the build-poll read retries on a wall-clock budget instead of three back-to-back attempts, which a two-second blip exhausted while the surrounding poll still had minutes left; `WorkersApiUnexpectedStatusError` classifies from the status it carries, so a 401 reads as "log in" rather than as a service failure across every Workers endpoint; a source of nothing but empty directories is refused before an upload slot is minted, instead of deploying an image with no handler; the runtime guess is only reported once the source is known to exist; and `config.toml` loading moved inside the finalizers, so a malformed config still flushes telemetry — matching `config/push`, which already had it right. Two findings are deliberately left for follow-ups: streaming the build context rather than buffering it, and an ignore mechanism so `.env` and `.git` can be kept out of the uploaded archive. Both are new work rather than defect fixes. The `SIDE_EFFECTS.md` also now records the telemetry and linked-project writes, the cache-miss API route, and why there is no live suite yet.
…on a clock Four follow-ups on #6262, three of them corners the previous round left open. `fs.stat` was the last swallowed filesystem error in `collectEntries`. An entry that disappears between `readDirectory` and the stat, or whose metadata cannot be read, was dropped from the archive — the same "deploy an application with a hole in it" outcome as the reads that were fixed, just losing a whole entry instead of its contents. `createTar` throws `TarPathTooLongError` for a name USTAR cannot represent, and calling it directly inside the generator made that a defect rather than a typed failure. `withJsonErrorHandling` only catches the failure channel, so `--output-format json` would have terminated with no structured error payload for an error the class explicitly declares as user-actionable. Wrapped in `Effect.try`, narrowing to that class and letting anything else stay a defect — because anything else there really is a bug. The transient-failure integration test was waiting on the real clock. The outer poll schedule was injected but the per-read retry was not, and that one is spaced in seconds, so a 500-then-200 sequence sat for two seconds and more transient responses would have pushed it toward thirty. Both schedules are parameters now; that test went from ~2s to 22ms. `--instances` is bounded at the parser, the way `[workers.<name>] instances` is bounded in the config schema. `--instances=-1` was accepted by the integer flag and carried into the deploy spec, so an impossible scaling request reached the remote endpoint only after the build context had been packaged and uploaded. `Flag.filter` refuses it during parsing, consistent with how `--runtime` is validated.
86d0f6b to
f02a2c3
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f02a2c3e46
ℹ️ 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".
| // them; everything that can fail on its own — loading `config.toml`, | ||
| // validating names, discovering workers — belongs inside, so a malformed | ||
| // config still flushes telemetry. Same shape as `config/push`. | ||
| const projectRef = yield* resolver.resolve(flags.projectRef); |
There was a problem hiding this comment.
When resolver.resolve fails—for example for an unlinked noninteractive project, an invalid --project-ref, or a failed interactive project lookup—the handler exits here before reaching the Effect.ensuring(telemetryState.flush) below, so telemetry.json is not persisted on that invocation. Fresh evidence beyond the earlier config-loading issue is that project resolution remains outside every finalizer; wrap resolution in an outer telemetry finalizer while keeping the linked-project cache conditional on obtaining a ref.
AGENTS.md reference: apps/cli/AGENTS.md:L286-L290
Useful? React with 👍 / 👎.
Builds and deploys workers into the linked project, and brings the Management API seam with it. Registered under `deploy` as an alias, for anyone reaching for the `supabase functions` verb out of habit. Given no names it deploys every worker in the project, matching `supabase functions deploy`, whose conventions this command set otherwise mirrors. "Every worker" is the union of the directories under `supabase/workers/` and the `[workers.<name>]` entries, so one with a `source` pointing elsewhere is not missed, and the order is sorted rather than whatever the filesystem returned. Deploys run one at a time: each is a server-side container build, so interleaving them would both compete for the alpha's per-project capacity and shred the progress output; the first failure stops the run. The flow is mint an upload slot, PUT the `.tar.gz` build context straight at the presigned URL, deploy, then poll until `build_state` leaves `building`. The upload carries no Supabase credentials: the signature in the URL is the authorization, and the bytes never pass through the management API. That signature is also a write-capable credential for the archive a deploy is about to build from, so `legacyHttpClientLayer` redacts presigned URLs at the logging boundary — `--debug` scrollback and CI logs are not where it belongs, and redacting there covers every presigned URL the CLI might log rather than only this one. Polling is a `Schedule`, and the read inside it retries on a wall-clock budget so a blip of a second or two does not throw away a deploy that still has minutes of build ahead of it. Which spec is sent depends on the runtime: a `dockerfile` worker sends a context and no `spec.runtime`, a catalog runtime sends both, and a bare `sandbox` sends the runtime alone and skips packaging, so it has no URL. A directory with no `[workers.<name>] runtime` has one guessed from marker files once the source is known to exist, reported on stderr with a nudge to pin it down. Everything that can fail deterministically fails before the remote project changes. `-o env` and a `-o toml` payload carrying an absent optional are settled up front rather than at emit time, where the command would exit non-zero having already deployed and invite a retry that deployed again; `--instances` is bounded at the parser the way the config schema bounds `[workers.<name>] instances`, instead of carrying an impossible scaling request through a packaged upload; and a source of nothing but empty directories is refused before an upload slot is minted, rather than deployed as an image with no handler. The build context is packaged in-process rather than by shelling out to `tar`, whose BSD, GNU and absent-on-Windows variants each produce a different archive from the same tree. `tar.ts` writes USTAR directly: files, directories and symlinks, refusing a value too large for an octal header field instead of letting it spill into the next one and read back as a plausible but wrong size. Symlinks are stored as links rather than followed — anything pnpm installs is symlink-dense, so following them would inline every dependency and walk into a link pointing at an ancestor. Every filesystem error propagates: an unreadable file archived as zero bytes, a dropped subtree or an entry lost between `readDirectory` and its stat all mean a successful `push` reporting an image built from an application with a hole in it. The Workers routes answer 404 both for a project outside the alpha's allow-list and for a ref that names nothing this account can see, so the classification reads `error.code`: `not_found` raises `WorkerProjectNotFoundError` naming the ref, `supabase link` and `supabase login`, and anything unrecognized keeps the enrolment answer, since that is what the allow-list has historically returned and guessing the other way sends someone to check a ref that is fine. This is the first command in this shell to call a v2 Management API route; every other one here is a Go-parity port and uses v1 only. Two findings are deliberate follow-ups rather than defects: streaming the build context instead of buffering it, and an ignore mechanism so `.env` and `.git` can be kept out of the uploaded archive.
f02a2c3 to
905115b
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 905115bc4f
ℹ️ 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 push is invoked with the globally accepted -o table or -o csv, legacyWorkersMachineOutputRequested suppresses the normal deployment details and this call falls through to legacyEmitWorkersMachineOutput, whose final branch encodes TOML. These values are db-query-only according to shared/legacy/global-flags.ts:12-19, so Workers should treat them like an unset output flag rather than reporting a successful remote deployment in an unrelated format.
AGENTS.md reference: apps/cli/AGENTS.md:L300-L300
Useful? React with 👍 / 👎.
905115b to
f7724d1
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f7724d1d55
ℹ️ 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".
| // by file, keeps a broken link from vanishing, and stops a link pointing at | ||
| // an ancestor from being walked into. | ||
| const linkTarget = yield* fs.readLink(absolutePath).pipe(Effect.option); | ||
| if (linkTarget._tag === "Some") { |
There was a problem hiding this comment.
Replace the direct linkTarget._tag inspection with Option.isSome(linkTarget); the same new packaging path also inspects modified._tag for the mtime. These are statically known Option values, so depending on Effect's runtime representation rather than its public narrowing helpers makes this core packaging code brittle across Effect upgrades.
AGENTS.md reference: AGENTS.md:L169-L175
Useful? React with 👍 / 👎.
| const instanceCountOrUndefined = (value: unknown): number | undefined => | ||
| typeof value === "number" && Number.isInteger(value) && value >= 0 ? value : undefined; |
There was a problem hiding this comment.
Fresh evidence beyond the earlier negative-instance finding is that this normalization still accepts integers above Number.MAX_SAFE_INTEGER, while the generated V2DeployAWorkerInput schema caps spec.instances at 9007199254740991. A JSON project config containing a larger nonnegative integer therefore passes project loading and this guard, lets push package and upload the build context, and only then fails input decoding at deployWorker; use Number.isSafeInteger or apply the API's explicit upper bound before upload.
Useful? React with 👍 / 👎.
|
|
||
| | Path | Format | When | | ||
| | ---------------------------------------- | ---------- | ----------------------------------------------------------------------------------------------------------- | | ||
| | `<workdir>/supabase/config.toml` | TOML | always, for each worker's runtime, size, source | |
There was a problem hiding this comment.
When a project contains supabase/config.json, legacyLoadWorkersProject calls loadProjectConfig, which prefers that JSON file even when config.toml also exists; the worker runtime, size, instances, and source therefore come from JSON, and TOML may not be read at all. Listing only config.toml as an unconditional read makes this compatibility checklist inaccurate, so document both files and their JSON-over-TOML precedence.
AGENTS.md reference: apps/cli/AGENTS.md:L359-L366
Useful? React with 👍 / 👎.
f7724d1 to
160a3a5
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 160a3a5c26
ℹ️ 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".
| const stat = yield* fs.stat(worker.sourceDir).pipe(Effect.option); | ||
| if (stat._tag === "None" || stat.value.type !== "Directory") { |
There was a problem hiding this comment.
When fs.stat fails because the source is unreadable or an I/O error occurs, Effect.option converts that operational failure to None, so the command incorrectly reports that the directory does not exist and suggests scaffolding over it. Map only a genuine not-found result to WorkerSourceMissingError and preserve other filesystem failures in the typed failure channel.
AGENTS.md reference: AGENTS.md:L147-L152
Useful? React with 👍 / 👎.
| const archive = yield* Effect.try({ | ||
| try: () => gzipSync(createTar(entries)), |
There was a problem hiding this comment.
When a worker contains a large build context—especially the explicitly supported case with an installed node_modules—collectEntries retains every file buffer, createTar allocates another full uncompressed copy, gzipSync allocates the compressed result, and the subsequent new Uint8Array copies that result again. Valid contexts can therefore consume multiple times their size in memory and block interruption during synchronous compression, causing workers push to freeze or run out of memory before upload; stream tar entries through compression and into the request using an Effect-native boundary instead.
AGENTS.md reference: AGENTS.md:L62-L64
Useful? React with 👍 / 👎.
| get [ErrorActionabilityId](): CliErrorActionabilityDeclaration { | ||
| return actionability.provideFlags; |
There was a problem hiding this comment.
When config.toml contains an unknown runtime or size, these two errors are raised specifically for the recorded configuration values and tell the user to edit that file, but provideFlags records them as InvalidInput with a ProvideFlags remediation even though workers push has no runtime or size flag. This corrupts the actionability KPIs for both cases; classify UnknownWorkerRuntimeError and UnknownWorkerSizeError with actionability.invalidConfig instead.
AGENTS.md reference: apps/cli/AGENTS.md:L370-L386
Useful? React with 👍 / 👎.
| const fs = yield* FileSystem.FileSystem; | ||
| const absoluteDir = relativeDir === "" ? root : `${root}/${relativeDir}`; | ||
|
|
||
| const names = yield* fs.readDirectory(absoluteDir); |
There was a problem hiding this comment.
When a Dockerfile worker uses .dockerignore to exclude files such as .env, credentials, or .git, this unconditional walk still reads and places every excluded file in the archive sent to the presigned control-plane URL. Even if a later Docker build omits those files from COPY, the sensitive bytes have already crossed the remote boundary, contrary to the normal build-context semantics the ignore file is meant to provide; apply .dockerignore while collecting the context, before reading or uploading excluded entries.
Useful? React with 👍 / 👎.
160a3a5 to
905115b
Compare
The same finding as the `workers new` change one commit down the stack, applied to the three occurrences this branch adds: the symlink probe and the mtime fallback in `worker-package.ts`, and the source-directory check in `push.handler.ts`. `Option.isSome`/`isNone` are type guards, so the narrowing after each check is unchanged. `worker-package.unit.test.ts` read `exit._tag` for the same reason; the repo guidance names `Exit.isSuccess`/`Exit.isFailure` and applies to tests too. `push.integration.test.ts`'s `tagOf` keeps its `_tag` access. It classifies values that may be a `Data.TaggedError` or a plain `Error` subclass with no tag at all, which is the dynamic boundary the guidance carves out. Also corrects the `push.handler.ts` module docblock: the argument is variadic, so it is `[name...]`, matching the SIDE_EFFECTS title.
develop renamed `LegacyCliConfig` to `LegacyCliSettings` (and its module), which this branch's push handler still imported under the old path. The unresolved import widened the handler's requirements to `unknown`, so the 27 knock-on errors in `push.integration.test.ts` all came from this one line.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c51d62ecbd
ℹ️ 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 markSupabaseApiInputErrorAsUserInput(error); | ||
| } | ||
| if (HttpClientError.isHttpClientError(error)) { | ||
| const description = error.reason.description ?? error.reason._tag; |
There was a problem hiding this comment.
When a transport error has no description, this fallback—and the equivalent upload-error fallback near line 290—reads Effect's internal _tag representation directly. This makes Workers error reporting depend on runtime internals that may change across Effect upgrades; use a public matcher, predicate, or stable formatter for the reason variants instead.
AGENTS.md reference: AGENTS.md:L171-L177
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.
A few nits documented below that are worth calling out, but nothing blocking — once you're happy with it, ping me and I can merge it after #6261!
Well-built slice. The handler reads top-to-bottom as the command's own story, the USTAR writer is carefully documented and unit-tested against real header layout, the error taxonomy carries detail/suggestion pairs with actionability declarations throughout, and the integration suite (27 scenarios) is exactly the scenario-oriented shape the testing pyramid asks for — including regression pins from earlier review rounds (no-request-made assertions, telemetry flush on config failure). Redacting presigned URLs at the logging boundary, with tests in both directions, was the right call.
Ticket coverage (FUNC-753 slice): the core asks are delivered. Deploys run from any project directory via the resolved workdir (tested); the async contract is handled properly — deploy accepts 202,
awaitWorkerBuildpolls on a 2s/10-minute schedule with a wall-clock-bounded retry for transient reads, build failures surface with the server'sstate_reason, and a timeout points at follow-up. Runtime/size/instances come from config.toml with a reported best-effort runtime guess and an--instancesoverride. One gap against the ticket text: there is no interactive prompting for stack/size/instances anywhere in this slice. If guess-and-report deliberately replaced prompting, worth recording that on the ticket.Findings, ranked (none blocking):
tar.ts—writeOctalaccepts negative values and writes an invalid octal field (reachable via pre-epoch file mtimes fromworker-package.ts); the archive uploads and then fails remotely. Inline comment; one-line guard.- Both "re-scaffold it with
supabase workers new <name> --force" suggestions reference a flagnewdoes not define, so the recovery command itself errors. Pending reply on the existing thread; a second occurrence sits at thefileCount === 0guard.- The handler's
fs.stat/readDirectoryrecoveries squash permission and I/O errors into "missing"/"empty" misdiagnoses with a scaffold-over-it suggestion. Inline comment; overlaps an open bot thread on line 170.- The
Effect.trynarrow inworker-package.tsleavesTarFieldTooLargeErrora defect even though the class declares itself user-actionable, so--output jsonexits unstructured for it. Pending reply on the existing thread.- Doc nit:
tar.ts's why-not paragraph covers shelling out totarbut notBun.Archive, which the repo already uses to build tars inpgdata-snapshot.ts. Inline comment.Questions rather than demands:
- The presigned upload has no deadline, so a stalled connection hangs
pushindefinitely. A timeout is cheap, but any fixed value is wrong for some archive-size/link-speed combination — was unbounded deliberate?- Symlinks whose targets resolve outside the packaged tree (e.g. pnpm workspace
node_modulespointing at a repo-root.pnpm) are archived as dangling links, so the remote build sees a hole and fails cryptically. Fine for alpha-scale workers; a local warning when a link target escapes the tree may belong on the follow-up list alongside the streaming work.Out-of-scope observations (other PRs in the stack own these):
awaitWorkerBuild's timeout suggestion namessupabase workers status, which does not exist until the next PR — harmless if the stack merges together, misleading ifpushships first (open bot thread).legacyDiscoverWorkerNames(base PR) swallows stat failures, so a barepushcan silently skip a worker and still exit 0; and-o table/-o csvfall through to TOML in the shared workers output helper. Both live in #6261's files.Several threads from the latest Codex round remain open; most are the same papercut caliber as the above, and the fix cadence on earlier rounds has been thorough.
Recommendation: approve with nits — the golden path is correct, the async-deploy/polling contract the ticket demands is genuinely delivered and tested, and everything found is edge-case robustness or recovery-text polish rather than wrong deploys, data loss, or credential mishandling.
| const text = Math.floor(value) | ||
| .toString(8) | ||
| .padStart(length - 1, "0"); | ||
| if (text.length > length - 1) { |
There was a problem hiding this comment.
A negative value slips through this guard and corrupts the header instead of throwing: (-1).toString(8) is "-1", which pads to "000000000-1" — exactly length - 1 chars, so the check passes and an invalid octal field is written. GNU tar rejects the archive as malformed, so the build context uploads and then fails server-side.
The reachable path is a pre-epoch mtime: worker-package.ts:68 computes Math.floor(mtime.getTime() / 1000) with no lower bound, and files with pre-1970 timestamps do turn up in the wild (botched touch, some extractors). Two one-liners close it: if (value < 0 || text.length > length - 1) here, and/or Math.max(0, ...) at the mtime computation. (This makes the earlier bot finding on pre-epoch mtimes concrete — that thread lost its anchor when the file moved.)
| * compiled binary to machines where `tar` may be BSD tar, GNU tar, or absent | ||
| * (Windows), and each writes a different archive for the same directory. The | ||
| * server only ever untars what we send, so producing the bytes here keeps the | ||
| * upload identical on every platform and keeps packaging out of the process |
There was a problem hiding this comment.
The why-not paragraph covers shelling out to tar, but the repo already builds tar bytes in-process with Bun.Archive (legacy/shared/db-bootstrap/pgdata-snapshot.ts, legacyPgDataBaselineMarkerTar). I assume it was rejected because its creation API takes only path-to-contents pairs — no symlink entries, per-file modes, or pinned mtimes, all of which this packaging needs. Worth one sentence here so the next reader does not try to consolidate the two.
| // guessed. Doing that first meant reporting an inference about a path that | ||
| // does not exist, and only then failing on the path. | ||
| { | ||
| const stat = yield* fs.stat(worker.sourceDir).pipe(Effect.option); |
There was a problem hiding this comment.
Effect.option here swallows every stat failure, not just not-found: a permission or I/O error on the source directory is reported as "There is no worker source at ..." with a suggestion to scaffold over it — misdiagnosis plus a remediation that points the wrong way. Per the repo's Effect rules this should recover only the recognized condition (the SystemError not-found reason) and let other PlatformErrors propagate.
Same pattern two lines down at the readDirectory(...).pipe(Effect.orElseSucceed(() => [])) on line 170, where an unreadable directory is reported as empty — there is an open bot thread on that one already; the fix is the same narrow in both places.
| return yield* Effect.fail( | ||
| new WorkerSourceMissingError({ | ||
| detail: `${sourceDisplay} is empty, so there is nothing to deploy.`, | ||
| suggestion: `Add your code there, or re-scaffold it with \`supabase workers new ${name} --force\`.`, |
There was a problem hiding this comment.
Confirmed: new.command.ts defines no --force flag, so this recovery command exits with an unknown-option error. Note the identical suggestion string also appears further down in this file (the fileCount === 0 guard, ~line 220) — both need the same rewording. Since new also refuses an existing directory, "remove the empty directory, then re-run supabase workers new <name>" is probably the honest instruction.
| const archive = yield* Effect.try({ | ||
| try: () => gzipSync(createTar(entries)), | ||
| catch: (cause) => { | ||
| if (cause instanceof TarPathTooLongError) { |
There was a problem hiding this comment.
Reinforcing this one: the narrow contradicts the error's own declaration. TarFieldTooLargeError carries actionability.invalidInput and its doc comment says it is user-actionable, yet this catch rethrows it as a defect — so the class's classification can never take effect and --output json exits unstructured. Widening the narrow to both tar error classes is a one-liner; the "anything else is a bug" comment stays true for genuine gzipSync failures. (Also note an 8 GiB file trips this via the size field, which is a far more plausible input than a year-2242 mtime.)
2 participants
Summary
Adds
supabase workers push(aliaseddeploy) and the machinery it needs:workers-api.ts— the typed Workers Management API client.tar.ts/worker-package.ts— packaging a worker directory into the buildcontext that gets uploaded.
worker-classify.ts— best-effort runtime detection from marker files, so adirectory with no
[workers.<name>] runtimecan still deploy. The guess isalways reported with a nudge to pin it down, never applied silently.
Stack 3 of 4, on top of
workers new(#6261).Linked issue
FUNC-753 (Linear). Supabase maintainer, exempt from the
open-for-contributionflow.Checklist