Skip to content

feat(workers logs): add supabase experimental workers logs - #6410

Merged
johnstonmatt merged 45 commits into
developfrom
FUNC-853/workers-logs-command
Sep 3, 2026
Merged

feat(workers logs): add supabase experimental workers logs#6410
johnstonmatt merged 45 commits into
developfrom
FUNC-853/workers-logs-command

Conversation

@johnstonmatt

@johnstonmatt johnstonmatt commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Summary

Reads the project's unified logs stream rather than a worker route — there is no worker-scoped log endpoint — via v1GetProjectLogs, which the generated client already carries. --kind app|requests|builds narrows to one of the three streams; without it all three are returned, tagged per line. --tail caps the rows. --follow keeps printing until interrupted.

Three non-obvious things about that endpoint

Each is documented at its call site, because none is guessable from the API surface:

  • The filter is log_attributes, not the source column. Worker rows carry an empty top-level source, because the Workers Logflare source is not enrolled as a category in the generic logs path, so where source = 'worker_guest_logs' matches nothing. The in (...) list over the three known streams is a tenancy guard rather than a convenience — with source empty it is the only thing excluding a non-worker row that happens to carry a worker attribute.
  • Both timestamp bounds are always sent, spanning under 24h. One bound alone yields a one-minute window, silently; neither is an outright error; and a span over 24h is clamped to start + 24h, returning an older slice than the one asked for rather than a truncated one.
  • A failed query can arrive as HTTP 200 with a populated error, so the envelope is checked before result.

The response is decoded against a local schema rather than the generated V1GetProjectLogsOutput: that schema marks result/error optional but permits neither to be null, while the endpoint always sends one of them as an explicit null. Decoding a real response against it always fails — worth fixing in the spec separately.

Rendering

Per-stream, because event_message differs in kind: on the request stream it is only "GET /", with status and duration in log_attributes, so the request line is composed. severity_text is ignored — it is INFO on every row of every stream — so the level is derived, and app lines report none rather than a guess. An app message is tenant-controlled bytes, so escape sequences are stripped before it reaches a terminal while a stack trace's newlines and indentation survive.

--follow

The poll interval is set by the rate limit, not by responsiveness: the v1 analytics endpoints allow 10 requests per 60 seconds, so the two-second poll a live tail suggests would spend the whole allowance in ten seconds. It polls every 10 seconds, measured at ~7 requests in the worst 60-second window.

The cursor deliberately lags 60 seconds behind the newest line printed. Guest lines are relayed CloudWatch → subscription filter → Lambda → Logflare and arrive late and out of order, so a cursor sitting on the newest timestamp would drop every straggler permanently. Overlap is therefore guaranteed; dedupe on the Logflare-minted id is what makes it invisible.

-o json|yaml|toml and --output-format json are refused up front — each promises one terminal payload and a tail has no last element. --output-format stream-json emits one log-entry event per line. SIGINT exits 130.

Stack

On top of the workers output polish (#6389), with push --wait (#6371) stacked above so it can be rejected independently. Below those: the workers new name prompt (#6349).

Note

Replaces #6408, which GitHub marked merged during a stack reorder. It was never merged to develop; the branch and its commits are intact here.

Makes the `name` argument to `supabase workers new` optional and prompts for it
when it is omitted, so a bare `supabase workers new` walks through name, runtime
and size rather than failing the parse.

The name is the one input this command cannot default — it is the directory, the
`[workers.<name>]` key and the hostname all at once. So where the runtime and
size prompts fall back to a default when there is nowhere to ask, the name
prompt has nothing to fall back to: with `-o json|yaml|toml|env` or no
interactive terminal, the command fails with a new `MissingWorkerNameError`
pointing at `supabase workers new api`.

The prompt validates against everything the command would otherwise refuse a
moment later — a non-DNS-label name, and a name `config.toml` already records —
so a typo is corrected in place instead of ending the run. That also means the
project has to be loaded before the first prompt, and the machine-output check
moves up with it: `-o` leaves `output.format` as `text`, and Clack writes its
terminal UI to stdout, so a name prompt would land in front of the payload for
the same reason the runtime prompt would.

The handler's inline name validation is replaced by the shared
`legacyValidateWorkerName`, which the rest of the command family already uses,
so an explicitly-passed name and a prompted one are refused on identical terms.

`mockOutput` now records `promptTextCalls` so tests can assert on the prompt's
message and exercise its `validate` callback.
`output.interactive` is derived solely from `tty.stdoutIsTty`, so with stdin
piped or redirected and stdout still on a terminal it stayed true. A bare
`printf 'api\n' | supabase workers new` therefore opened Clack's name prompt and
read the worker name off the pipe instead of taking the documented
`MissingWorkerNameError` path — and the runtime and size prompts consumed
whatever followed rather than falling back to their defaults.

The three resolvers now share one `canPromptFor` decision, made once before the
first prompt, which pairs `output.interactive` with `tty.stdinIsTty` the way
`workers delete` already guards its confirmation. A prompt is only answerable
from a keyboard, so both streams have to be a terminal.
Makes the `name` argument to `supabase workers new` optional and prompts for it
when it is omitted, so a bare `supabase workers new` walks through name, runtime
and size rather than failing the parse.

The name is the one input this command cannot default — it is the directory, the
`[workers.<name>]` key and the hostname all at once. So where the runtime and
size prompts fall back to a default when there is nowhere to ask, the name
prompt has nothing to fall back to: with `-o json|yaml|toml|env` or no
interactive terminal, the command fails with a new `MissingWorkerNameError`
pointing at `supabase workers new api`.

The prompt validates against everything the command would otherwise refuse a
moment later — a non-DNS-label name, and a name `config.toml` already records —
so a typo is corrected in place instead of ending the run. That also means the
project has to be loaded before the first prompt, and the machine-output check
moves up with it: `-o` leaves `output.format` as `text`, and Clack writes its
terminal UI to stdout, so a name prompt would land in front of the payload for
the same reason the runtime prompt would.

The handler's inline name validation is replaced by the shared
`legacyValidateWorkerName`, which the rest of the command family already uses,
so an explicitly-passed name and a prompted one are refused on identical terms.

`mockOutput` now records `promptTextCalls` so tests can assert on the prompt's
message and exercise its `validate` callback.
`output.interactive` is derived solely from `tty.stdoutIsTty`, so with stdin
piped or redirected and stdout still on a terminal it stayed true. A bare
`printf 'api\n' | supabase workers new` therefore opened Clack's name prompt and
read the worker name off the pipe instead of taking the documented
`MissingWorkerNameError` path — and the runtime and size prompts consumed
whatever followed rather than falling back to their defaults.

The three resolvers now share one `canPromptFor` decision, made once before the
first prompt, which pairs `output.interactive` with `tty.stdinIsTty` the way
`workers delete` already guards its confirmation. A prompt is only answerable
from a keyboard, so both streams have to be a terminal.
@johnstonmatt
johnstonmatt requested a review from a team as a code owner August 31, 2026 22:34
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Aug 31, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-09-01T07:24:40.343859Z 4f05bc5 New commits
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

Comments explaining why code is shaped a certain way now state the constraint
directly instead of narrating what an earlier version did. The reasoning is
unchanged; only the framing is.
The workers commands each grew their own way of saying "here is what happened"
and "here is what to run next". This settles them on the shapes the rest of the
legacy shell already uses, with no change to what any command does.

- "What to run next" lines in `new`, `push`, `delete` and `status` move to
  `emitSuccessTrailer`, the way `stop`, `bootstrap`, `migration repair` and
  `gen signing-key` already emit theirs: printed once at the end of the run
  rather than inline, so a multi-worker push does not bury each worker's hint
  under the next worker's output. The commands within them are aqua'd, as every
  other follow-up hint in this shell writes them.
- `list`'s two advisories take the yellow `WARNING:` prefix and the two-line
  consequence shape `start`'s Docker notice uses. Each was one long sentence
  that re-flowed at a different width under a table that lines its columns up.
- `list` drops the URL column. Every worker's URL is the same host and prefix
  with the name on the end, and carrying it pushed the table past 130 columns
  for one derivable field, since `renderGlamourTable` sizes to the widest cell
  and never wraps. `status` still renders it vertically, and every machine
  format still carries `url` per worker.
- `push` counts its per-worker announcements (`Deploying Worker 1/2:`) and
  closes a multi-worker run with a summary line. Each worker takes minutes; the
  name alone said nothing about how much of the run was left.
- `push` names the workers a failed run never attempted. The loop stops at the
  first failure and the error only names the worker that broke, leaving the
  rest to be reconstructed from argument order. On stderr in every format,
  machine ones included: that run is a CI run.
- Both of `push`'s retry suggestions carry an explicit `--project-ref` when the
  flag supplied the ref, via the `legacyWorkersProjectRefSuffix` helper `status`
  and `delete` already use. A suggestion is copy-pasted verbatim, so one that
  dropped it re-resolved against whatever this checkout was linked to.

Adds unit coverage for `legacyRenderWorkerDetails`'s padding and empty-row
dropping, and pins the shared `-o env` refusal so a new command that forgets
its own up-front check cannot silently emit TOML instead.
`Flag.boolean(name)` builds a bare `Single` param, and a bare `Single` is
*required* — omitting it fails the whole command with a missing-flag error
before the handler ever runs. Every boolean flag has to be closed off with
`Flag.withDefault(false)` or `Flag.optional`, and nothing in the existing suites
notices when one is not.

Handler integration tests build their flags record directly, so they never
touch the parser, and the required-ness is invisible to the type checker
because a required boolean flag still infers as `boolean`. The flag only
misbehaves when a real invocation omits it, which is exactly the invocation no
handler test makes.

So this walks the whole legacy command tree, including global flags, and
asserts every boolean param carries a default or is optional. It reads the
primitive kind through `Primitive.getTypeName` rather than `_tag`, since this
repo forbids inspecting effect's runtime representation in tests as well as in
source.
Comments explaining why code is shaped a certain way now state the constraint
directly instead of narrating what an earlier version did. The reasoning is
unchanged; only the framing is.
`status` reports the deployment; nothing reported the runtime. Once `push`
succeeded and `status` said `active`, a misbehaving worker was a black box from
the CLI.

Reads the project's unified logs stream rather than a worker route — there is no
worker-scoped log endpoint — via `v1GetProjectLogs`, which the generated client
already carries. `--source app|requests|builds` narrows to one of the three
streams; without it all three are returned. `--tail` caps the rows.

Three things about that endpoint are load-bearing and non-obvious, so they are
documented at each site:

- **The filter is `log_attributes`, not the `source` column.** Worker rows carry
  an empty top-level `source`, because the Workers Logflare source is not
  enrolled as a category in the generic logs path. `where source =
  'worker_guest_logs'` matches nothing. The `in (...)` list over the three known
  streams is therefore a tenancy guard, not a convenience — with `source` empty
  it is the only thing excluding a non-worker row that happens to carry a
  `worker` attribute.

- **Both timestamp bounds are always sent, spanning under 24h.** One bound alone
  yields a one-minute window, silently; neither is an outright error; and a span
  over 24h is clamped to `start + 24h`, which returns an *older* slice than the
  one asked for rather than a truncated one.

- **A failed query can arrive as HTTP 200** with a populated `error`, so the
  envelope is checked before `result`.

The response is decoded against a local schema rather than the generated
`V1GetProjectLogsOutput`: that schema marks `result`/`error` optional but allows
neither to be `null`, while the endpoint always sends one of them as an explicit
`null`, so decoding any real response against it fails.

Rendering is per-stream, because `event_message` differs in kind — on the request
stream it is only `"GET /"`, with status and duration in `log_attributes`, so the
request line is composed. `severity_text` is ignored: it is `INFO` on every row
of every stream, so the level is derived, and guest lines report none rather than
a guess. A guest message is tenant-controlled bytes, so escape sequences are
stripped before it reaches a terminal while a stack trace's newlines and
indentation survive.

`mapRequestError`/`unexpectedStatus`/`decodeBody` move out of `workers-api.ts`
into `workers-api-status.ts`, unchanged, now that a second seam needs them.

The test helper records `urlParams`: `HttpClientRequest` keeps them off the URL,
so without this no test could assert the emitted SQL or window.
Matches the only other log-line format this shell prints — the `--debug` HTTP
logger, which uses Go's `log.LstdFlags` (`legacy-debug-logger.layer.ts`). Someone
reading a tail is asking "what just happened", and the answer gets compared
against their own clock.

Text output only. The machine payload keeps both unambiguous forms, so nothing
that is parsed, sorted, or pasted into an issue depends on the reader's zone:
`timestamp` stays ISO-8601 UTC and `timestamp_ms` the raw epoch value.

The unit tests derive their expected prefix from the same instant with the same
field accessors, rather than hardcoding one: a literal `"14:45:32"` would have
passed only on a UTC machine. One case additionally pins the zone choice itself
— asserting the output is *not* the UTC rendering — guarded so it stays
meaningful on a UTC machine, where the two coincide. Verified green under
`TZ=Asia/Tokyo`, `TZ=UTC`, and the ambient zone.
Opt-in, matching `workers push --wait`: long-running behaviour in this family is
asked for, never defaulted.

**The poll interval is set by the rate limit, not by responsiveness.** The v1
analytics endpoints allow 10 requests per 60 seconds, so the two-second poll a
live tail suggests would spend the whole allowance in ten seconds. Six seconds is
the arithmetic floor; ten leaves room for the history query, the deployed-worker
check, and a retry in the same window. Measured at ~7 requests in the worst
60-second window. The interval is in `--follow`'s help text, because a 10-second
tail is visibly not a live stream and would otherwise look broken.

The cursor deliberately lags 60 seconds behind the newest line printed. Guest
lines are relayed CloudWatch -> subscription filter -> Lambda -> Logflare and
arrive late and out of order, so a cursor sitting on the newest timestamp would
drop every straggler permanently. Overlap is therefore guaranteed and expected;
dedupe on the Logflare-minted `id` is what makes it invisible, bounded so a long
tail does not grow the set forever. `followWindow` clamps to the same sub-24h span
as a bounded read, so a tail resumed after a laptop suspend cannot ask for a wider
window — the server answers those by returning an *older* slice.

Every poll sends both timestamp bounds. Advancing only `iso_timestamp_start` is
the obvious implementation and is wrong: it yields a one-minute window.

Output:

- `-o json|yaml|toml` and `--output-format json` are refused up front, beside the
  `-o env` refusal and for the same reason — each promises one terminal payload
  and a tail has no last element.
- `--output-format stream-json` emits one `log-entry` event per line instead of a
  single `result`, reusing the existing variant. `stream` splits error/warn to
  `stderr`; `source` separates backlog from live.
- SIGINT exits 130, matching the local `supabase logs` command.
- `--tail 0` skips the backlog and makes no history request, since the endpoint
  rejects `limit 0`. It also suppresses the not-deployed check, which would
  otherwise read "no rows" as "no worker" when no query was made at all.

Both schedules are injectable, as `awaitWorkerBuild`'s are, so the cursor, dedupe
and retry paths are tested without a wall clock. The SIGINT test forks the handler
and synchronises on the mock's `awaitExit` — `exit` never returns, so the handler
cannot be awaited. Stressed over five consecutive runs.
Comments explaining why code is shaped a certain way now state the constraint
directly instead of narrating what an earlier version did. The reasoning is
unchanged; only the framing is.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6a0ae82d95

ℹ️ 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".

Comment thread apps/cli/src/legacy/commands/experimental/workers/logs/logs.handler.ts Outdated
Comment thread apps/cli/src/legacy/commands/experimental/workers/logs/logs.handler.ts Outdated
Comment thread apps/cli/src/legacy/commands/experimental/workers/logs/logs.handler.ts Outdated
Comment thread apps/cli/src/legacy/commands/experimental/workers/logs/logs.handler.ts Outdated
Comment thread apps/cli/src/legacy/commands/experimental/workers/logs/logs.handler.ts Outdated
Comment thread apps/cli/src/legacy/commands/experimental/workers/workers-logs.format.ts Outdated
Comment thread apps/cli/src/shared/workers/worker-logs-api.ts
Comment thread apps/cli/src/legacy/commands/experimental/workers/logs/logs.handler.ts Outdated
Comment thread apps/cli/src/legacy/commands/experimental/workers/logs/logs.handler.ts Outdated
Comment thread apps/cli/src/legacy/commands/experimental/workers/logs/logs.handler.ts Outdated
Comment thread apps/cli/src/legacy/commands/experimental/workers/workers-logs.format.ts Outdated

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c66065d71f

ℹ️ 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".

Comment thread apps/cli/src/legacy/commands/experimental/workers/workers-logs.format.ts Outdated
Comment thread apps/cli/src/legacy/commands/experimental/workers/logs/SIDE_EFFECTS.md Outdated
Comment thread apps/cli/src/legacy/commands/experimental/workers/logs/logs.handler.ts Outdated
@github-actions

github-actions Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Supabase CLI preview

npx --yes https://pkg.pr.new/supabase/cli/supabase@213f80408ee6dd2d903f71ade960bed8b2bda1b5

Preview package for commit 213f804.

…/supabase/cli into FUNC-840/select-workers-new-name

# Conflicts:
#	apps/cli/src/legacy/commands/experimental/workers/new/SIDE_EFFECTS.md
#	apps/cli/src/legacy/commands/experimental/workers/new/new.command.ts
#	apps/cli/src/legacy/commands/experimental/workers/new/new.handler.ts
`workers` is registered only beneath the `experimental` parent, so the
`MissingWorkerNameError` suggestion telling the user to run
`supabase workers new api` produced an unknown-command error when copied.

Name the real invocation path in the suggestion, and in the handler, doc
and test prose that described the piped-stdin case with the same stale
path. An assertion on the suggestion keeps the retry path from drifting
away from where the command is mounted.
The Workers API decode failure suggested `supabase update`, which is not a
command in either shell's root — the CLI has no self-update path, which is
why the post-command upgrade notice sends users to the docs instead.

Point the suggestion at that same upgrade guide, and hoist the URL from
`legacy-upgrade-notice.ts` into `shared/cli/version.ts` so both callers
read one constant rather than duplicating the link.
…olish

# Conflicts:
#	apps/cli/src/legacy/commands/experimental/workers/new/new.handler.ts
#	apps/cli/src/legacy/commands/experimental/workers/new/new.integration.test.ts
# Conflicts:
#	apps/cli/src/shared/workers/workers-api.ts
`-o` outranks `--output-format` when both are set, and `-o pretty|table|csv`
encode nothing and fall through to the text rendering. Branching on
`output.format` alone therefore did the opposite of what the pair asked for:
`-o pretty --output-format json --follow` was refused as a single-payload
format, and the bounded path emitted JSON for a run that had asked for text.

`legacyWorkersRenderFormat` resolves the two flags once, and the handler
branches on that instead of consulting `output.format` at each emission.
Ref resolution sat above both finalizers, so an unlinked non-interactive
checkout — or a declined project picker — failed before `telemetryState.flush`
was installed and wrote no post-run event, even though the command had run.

Telemetry now wraps the resolution; only the linked-project cache stays under
the ref, since it has nothing to write without one.
The production `ProcessControl.exit` calls `process.exit` synchronously, so
calling it from inside the race branch tore the runtime down before any of the
command's cleanup: no linked-project cache write, no telemetry flush, and no
post-run `cli_command_executed` from the instrumentation wrapper. Every
followed run was invisible to telemetry the moment it was interrupted.

The branch now records the code with `setExitCode` and returns, so the race
completes, the finalizers run, and `runCli` exits with the recorded code the
way it already does for a bounded run.
Each poll asked for `--tail` rows, but the query orders newest-first, so a burst
larger than that came back as its newest slice alone — and the cursor then
advanced past the rows that were never returned, dropping them permanently.
`--tail 1 --follow` lost almost everything; the default lost anything above 100
rows in a polling interval.

The poll now uses its own page size and walks `end` backwards while pages come
back full, so a burst is drained before the cursor moves. Bounded at five
requests, because the endpoint allows ten a minute; rows past that bound are not
lost, since the cursor still only advances over what was emitted.
Three holes in the branch that skips the history query:

The cursor starts at the invocation instant and `followWindow` reaches a grace
period behind it, so the first poll replayed up to a minute of the history the
run had just been told to skip. The grace is what makes a late-relayed line
visible at all, so it stays; a floor on the line's own timestamp is what keeps
skipped history out.

The deployed-worker check was gated on `--tail > 0`, so a tail with no history
query never asked whether the worker existed and a typo waited forever on logs
that could not arrive. It now runs for any follow.

That check also ran with no spinner: `--tail 0` has no "Fetching logs..." to
inherit, and the bounded path cleared it beforehand. It gets its own task.
…message

`stripControlSequences` was applied only to `event_message` on the guest stream,
on the premise that it was the one untrusted string. It was not: a request
`path`, `method` and `status` are chosen by whoever called the worker, a
`duration_ms` comes back on the same row, and a build `event` and `reason` are
relayed from the builder. All of them were interpolated raw into a line written
to a terminal.

The strip itself also kept carriage returns. A bare CR returns the cursor to
column zero, so even a sanitised guest line could overwrite the timestamp and
stream tag printed to its left and forge output that looks like the CLI's own.
CRLF now folds to a newline first, so real line breaks survive, and lone CRs go
with the other C0 controls.

The stream the tag derives from is left alone deliberately: the query only
returns rows whose stream is one of three literals, so it cannot carry anything.
Every exported token from `legacy/` carries the `Legacy` prefix, with no
exceptions — it is what keeps the two in-tree shells from bleeding into each
other at import sites. `WorkerLogLevel` was shipping bare.
The handler records 130 for an interrupted `--follow` and an integration test
asserts it, but the compatibility table listed only 0 and 1 — so the record E2E
coverage is derived from was incomplete.
`Schema.Number` accepted any finite value, and the payload build calls
`new Date(entry.timestampMs).toISOString()` unconditionally — text runs
construct it too. An out-of-range timestamp from an upstream projection
regression therefore threw `RangeError`, turning a recoverable bad row into a
defect. The bound moves onto the schema, where it fails through `decodeBody` as
the unreadable-response error the rest of the module already raises.
The follow loop retried every failure on a five-second schedule for up to a
minute. A 401, 402 or 404 answers the same way every time, so the reader waited
a minute to be told something the first attempt already knew — and the retries
spent most of the endpoint's ten-requests-per-minute allowance getting there, so
a rate limit could land on top of the real cause.

Server-side statuses still ride out, along with 408 and 429, which are the
server asking for exactly that. A decode failure carries the response's own
status, so a malformed 200 body reads as terminal: it will not parse better on
a second attempt.
`line` was set from `event_message`, which on the request stream is only
`"GET /"` — the status and duration live in `log_attributes` — and on the build
stream omits the structured failure reason. `log-entry` has no attributes field,
so a consumer had no way to recover either.

The composition text mode already does moves into `legacyWorkerLogText`, and
both callers use it. Coloring and the timestamp/tag prefix stay in the renderer,
since neither belongs in a machine event.
Three separate breaks, all from the same PR:

- `WORKER_LOG_CURSOR_GRACE_SECONDS` is only read by `logWindow` in its own
  file, so the `export` was dead and knip failed the quality job. Dropped.
- `--kind` is a value-consuming long flag, and the repo-wide completeness
  guard in `legacy-db-target-flags` requires every one of them to be
  registered or the DB-target scanner mis-reads the token after it.
- The colour test supplied a fake stream but not a fake environment. The gate
  checks `CI` before it asks the stream, so the assertion only held on a
  developer machine and failed in CI. Stubbed the same four variables
  `legacy-colors.unit.test.ts` stubs.
`git diff-tree --stdin` given a cwd outside any repository exits before it
reads a hash, so the write that follows can land on a dead process and raise
`EPIPE: broken pipe, send` instead of the exit-code error the caller reports.
Which one surfaces is a race against process startup — green on an idle
machine, red on a loaded CI runner.

The exit code and stderr are the diagnosis, so a broken pipe on stdin is
dropped and the reporting left to them.

@kanadgupta kanadgupta left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

everything looks sound per my testing! claude found a couple of little cleanups that seem good to know about but i'll defer to you on how to handle them, cool with merging either way. thanks!

[!NOTE]
This review was drafted by an AI (Claude) and is pending human vetting. It was refreshed after the fix commits landed (head b772491); the original pass reviewed c66065d.

What this PR does

Adds supabase experimental workers logs <name>: a bounded read of the project's unified logs via v1GetProjectLogs (filtered to the three worker streams through log_attributes), --kind narrowing, --tail, and a polling --follow with a lagging cursor, id-dedupe, and a bounded multi-page burst drain. Rendering is per-stream with derived levels, every externally influenced field is escape-stripped before it reaches a terminal, and the shared Workers status/error mapping is extracted into workers-api-status.ts (a genuine DRY win reused by both API seams). Structure conforms to the repo's conventions: pure SQL/window/format modules with unit tests, scenario-oriented integration tests with injected schedules (no sleeps), no e2e — consistent with the testing pyramid and e2e scope policy.

What the new commits addressed

CI is now green, and I verified each fix rather than taking the commit messages on faith. Everything the first review pass (mine and the bots') flagged as serious has landed:

  • --follow Ctrl+C finalizers: the race now records setExitCode(130) instead of calling the synchronous exit, so the ensuring finalizers (linked-project cache, telemetry flush) and the instrumentation wrapper run before runCli exits — with an integration test asserting exactly that (records exit 130 on SIGINT and still runs its finalizers).
  • Poll page size: decoupled from --tail (FOLLOW_PAGE_SIZE = 1000) plus a bounded multi-page drain (FOLLOW_MAX_PAGES), with burst-drain tests.
  • Red unit-test check: the colour tests now stub NO_COLOR/CLICOLOR/CLICOLOR_FORCE/CI, and the previously wrong env-independence claim in the header comment was corrected.
  • Red knip check: WORKER_LOG_CURSOR_GRACE_SECONDS is no longer exported (residue noted below).
  • Sanitisation: stripControlSequences now covers request status/method/path/duration, build event/reason, and CR/CRLF handling — closing the attacker-supplied-path hole and the Codex carriage-return one in one commit.
  • Also landed: telemetry flush wraps ref resolution, -o now outranks --output-format via legacyWorkersRenderFormat (applied family-wide), --tail 0 --follow filters pre-invocation lines and validates the worker, retries are gated on isRetryableFollowFailure, stream-json carries the composed line, ts_ms is schema-bounded to a representable Date, --source became --kind with choices tied to the stream map via satisfies (no cast), LegacyWorkerLogLevel prefix, SIDE_EFFECTS documents exit 130.

The two Codex threads the author closed as "leave as-is" (404 project-vs-alpha ambiguity; non-2xx query classification) both have reasonable recorded rationale; I have no objection to either.

Fix before merge

None found in this pass.

Follow-up candidates

  1. Drain-loop honesty (logs.handler.ts, inline): when FOLLOW_MAX_PAGES is exhausted mid-burst the cursor advances past the un-drained middle, so "rows beyond it are not lost" only holds within the 60s grace; capping the cursor at the drain boundary would make it true. Relatedly, the "~7 requests in the worst 60-second window" and SIDE_EFFECTS' "6 requests a minute" claims are stale now that a poll can spend up to 5 requests. Only bites above ~1000 rows/poll — follow-up material for an experimental command.
  2. Dead followWindow seams (worker-logs.sql.ts, inline): the graceSeconds/spanMinutes options are still used by no caller or test, and followWindow still has no direct unit tests despite being the file's trickiest arithmetic. Delete the seams or use them.

One note: the tip commit (b772491) fixes an EPIPE race in packages/config/scripts/semantic-release-path-filter.ts — a sound test-stability fix, but unrelated to this PR; ideally it rides its own PR or the base so it doesn't merge or revert with the workers work.

Ticket

Delivered. The command does what the branch name promises — bounded reads, --kind, --tail, --follow, machine formats, error mapping — CI is green, and the follow-mode correctness issues from the first pass are fixed and regression-tested.

Overall: 0 fix-before-merge issue(s), 2 follow-up candidate(s).

Comment thread apps/cli/src/shared/workers/worker-logs.sql.ts Outdated
Base automatically changed from FUNC-851/general-output-polish to develop September 3, 2026 05:42
pull Bot pushed a commit to chizee/cli that referenced this pull request Sep 3, 2026
## Summary

Makes the `name` argument to `supabase experimental workers new`
optional and prompts for it
when it is omitted, so a bare `supabase experimental workers new` walks
through name, runtime
and size rather than failing the parse.

The name is the one input this command cannot default — it is the
directory, the
`[workers.<name>]` key and the hostname all at once. So where the
runtime and
size prompts fall back to a default when there is nowhere to ask, the
name
prompt has nothing to fall back to: with `-o json|yaml|toml|env` or no
interactive terminal, the command fails with a new
`MissingWorkerNameError`
pointing at `supabase experimental workers new api`.

The prompt validates against everything the command would otherwise
refuse a
moment later — a non-DNS-label name, and a name `config.toml` already
records —
so a typo is corrected in place instead of ending the run. That also
means the
project has to be loaded before the first prompt, and the machine-output
check
moves up with it: `-o` leaves `output.format` as `text`, and Clack
writes its
terminal UI to stdout, so a name prompt would land in front of the
payload for
the same reason the runtime prompt would.

The handler's inline name validation is replaced by the shared
`legacyValidateWorkerName`, which the rest of the command family already
uses,
so an explicitly-passed name and a prompted one are refused on identical
terms.

`mockOutput` now records `promptTextCalls` so tests can assert on the
prompt's
message and exercise its `validate` callback.

## Stack

Bottom of the workers stack, on `develop`. Above it: output polish
(supabase#6389), `workers logs` (supabase#6410), and `push --wait` (supabase#6371).

## Linked issue

FUNC-840 (Linear). Supabase maintainer, exempt from the
`open-for-contribution` flow.

## Checklist

- [x] The PR title follows [Conventional
Commits](https://www.conventionalcommits.org/)
pull Bot pushed a commit to chizee/cli that referenced this pull request Sep 3, 2026
…base#6389)

## Summary

The workers commands each grew their own way of saying "here is what
happened" and "here is what to run next". This settles them on the
shapes the rest of the legacy shell already uses. **No command changes
what it does** — this is output, plus the coverage that pins it.

- **Success trailers.** "What to run next" lines in `new`, `push`,
`delete` and `status` move to `emitSuccessTrailer`, the way `stop`,
`bootstrap`, `migration repair` and `gen signing-key` already emit
theirs: printed once at the end of the run rather than inline, so a
multi-worker push does not bury each worker's hint under the next
worker's output. The commands within them are aqua'd.
- **`list` advisories.** Both take the yellow `WARNING:` prefix and the
two-line consequence shape `start`'s Docker notice uses. Each was one
long sentence that re-flowed at a different width, directly under a
table that lines its columns up.
- **`list` drops the URL column.** Every worker's URL is the same host
and prefix with the name on the end, and carrying it pushed the table
past 130 columns for one derivable field — `renderGlamourTable` sizes
each column to its widest cell and never wraps. `status` still renders
it vertically, and every machine format still carries `url` per worker.
- **`push` progress.** Per-worker announcements are counted (`Deploying
Worker 1/2:`) and a multi-worker run closes with a summary. Each worker
takes minutes; the name alone said nothing about how much of the run was
left.
- **`push` names what it never attempted.** The loop stops at the first
failure and the error only names the worker that broke, leaving the rest
to be reconstructed from argument order. On stderr in every format,
machine ones included: that run is a CI run.
- **`--project-ref` survives into `push`'s retry suggestions**, via the
`legacyWorkersProjectRefSuffix` helper `status` and `delete` already
use. A suggestion is copy-pasted verbatim, so one that dropped it
re-resolved against whatever this checkout was linked to.

Also adds unit coverage for `legacyRenderWorkerDetails`, pins the shared
`-o env` refusal, and adds a guard (own commit) asserting no legacy
boolean flag ships required — `Flag.boolean` alone builds a *required*
param, and nothing in the existing suites notices.

## Stack

On top of the `workers new` name prompt (supabase#6349). Above it: `workers
logs` (supabase#6410), then `push --wait` (supabase#6371) last, so the output work can
ship independently of both.

## Linked issue


[FUNC-851](https://linear.app/supabase/issue/FUNC-851/general-output-polish).
Supabase maintainer, exempt from the `open-for-contribution` flow.

## Checklist

- [x] The PR title follows [Conventional
Commits](https://www.conventionalcommits.org/)

---------

Co-authored-by: kanad <git@kanad.dev>
…ogs-command

# Conflicts:
#	apps/cli/src/shared/workers/workers-api.ts
…indow

`followWindow`'s `graceSeconds`/`spanMinutes` options and `logWindow`'s
`spanMinutes` parameter had no caller and no test — both call sites pass
nothing — so they were configurability for its own sake over the module's
trickiest arithmetic. Both now read the module constants directly.

`followWindow` had no direct unit tests at all. It has four now, including the
suspend case the 24h clamp exists for: a cursor left days behind would
otherwise ask for an over-wide span, which the server answers by rewriting
`end` to `start + 24h` — returning an older slice rather than a truncated one,
so a resumed tail would silently replay yesterday. The expectations are written
as literals rather than read from the constants under test, so they cannot stay
green through the change they exist to catch.
The `FOLLOW_MAX_PAGES` comment claimed rows past the bound were re-asked for on
the next poll. They are not. The drain walks `end` backwards, so the pages it
does fetch are the newest ones, and the cursor then advances to the newest row
printed — past a region it never reached. Only the part of that region inside
the next window's grace comes back.

Lowering the cursor cannot fix it: `followWindow` moves the window's floor, not
its ceiling, so a poll anchored at `now` would re-fetch the same newest pages
and never walk down to the gap. The bound stays, and the loss is now reported
instead of silent — once per run, on stderr, in every output format, since a
`stream-json` consumer cannot infer a hole from the events it receives.

Corrects the rate arithmetic in the same pass. Both the handler comment ("~7
requests in the worst 60-second window") and SIDE_EFFECTS ("6 requests a
minute") predate the multi-page drain and assume one request per poll. A quiet
tail does spend 6 a minute; a poll draining a burst spends up to 5, so a
sustained backlog reaches 30 against a limit of 10 and is throttled by the
retry rather than budgeted for.
…gnosis"

This reverts b772491.

The fix is sound but has nothing to do with workers logs; it landed here to
stabilise this branch's CI. Moved to #6455 off develop so it does not merge or
revert with the workers work. Raised in review on #6410.

Until #6455 lands, the `packages/config` release-script test it stabilises can
flake on a loaded runner.
@johnstonmatt
johnstonmatt added this pull request to the merge queue Sep 3, 2026
Merged via the queue into develop with commit d39af7a Sep 3, 2026
18 checks passed
@johnstonmatt
johnstonmatt deleted the FUNC-853/workers-logs-command branch September 3, 2026 23:15
pull Bot pushed a commit to chizee/cli that referenced this pull request Sep 4, 2026
…supabase#6371)

## Summary

`supabase experimental workers push` blocks on the server-side container
build. That build routinely runs for minutes, so the common case — a
deploy that builds fine — is the slowest thing in the loop.

`--no-wait` returns once the platform accepts the deploy, which is the
last thing the command can learn without waiting: the deploy response
arrives only after the spec and the uploaded context are accepted, and
it carries the accepted spec back. Waiting stays the default, so a plain
push still reports the build's verdict and existing invocations are
unchanged.

- Under `--no-wait` the details block leads with a `State` row — the one
row that says the worker is not serving yet — and drops `Image`, since
no image exists until the build produces one.
- A success trailer then points at `experimental workers status` for the
build's outcome. Text output only; machine callers read `build_state`
from the payload.
- A deploy answered with a spec already in `failed` is reported as a
failure whether or not the build was waited on, rather than exiting zero
on a worker that will never come up.

The second commit is a separate fix that fell out of the review: the
wait now runs only when the deploy response left `build_state` at
`building`. `V2DeployAWorkerOutput` permits a terminal `active` or
`failed` on the deploy response itself, and polling on top of that could
only contradict it — `awaitWorkerBuild` reads a post-deploy 404 as
"still building", so an already-failed deploy could burn the full poll
budget and surface as a timeout rather than the failure the platform had
already reported.

## Stack

Top of the workers stack, on top of `workers logs` (supabase#6410). Everything
below it — the name prompt (supabase#6349), the output polish (supabase#6389) and
`workers logs` — is independent of this flag, so this PR can be rejected
on its own without holding any of them up.

## Linked issue

FUNC-848 (Linear). Supabase maintainer, exempt from the
`open-for-contribution` flow.

## Checklist

- [x] The PR title follows [Conventional
Commits](https://www.conventionalcommits.org/)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants