Skip to content

[Feat] Durable diagnostic run events - #140

Merged
daniel-lxs merged 1 commit into
developfrom
feat/diagnostic-events
Jul 10, 2026
Merged

[Feat] Durable diagnostic run events#140
daniel-lxs merged 1 commit into
developfrom
feat/diagnostic-events

Conversation

@daniel-lxs

Copy link
Copy Markdown
Member

PR 1 of the observability pair (PR 2, the OpenCode exit certificate, stacks on this). Motivation: sandbox logs die with the sandbox and Modal exposes none at all, so the last two incident investigations ended at "leading suspect" instead of proof.

What

A diagnostic rail on the existing task_run_events audit table — no schema change:

  • diagnostic added to runEventTypes; workers write through the existing sdk.taskRuns.recordEvent path.
  • Worker-side recorder (apps/worker/src/run-task/diagnostic-events.ts).
  • tasks.runEvents web tRPC reader (mirrors tasks.messageEnvelopes auth) so per-task events are queryable without DB access.

Nothing emits yet — the first producer (OpenCode death certificate) is the follow-up PR, kept separate so this rail can be reviewed on its own.

Guarantees (per the requirements)

  • Performance: nothing on hot paths. Recording happens only when a producer calls it (rare lifecycle moments); it's one fire-and-forget API call using the same client workers already hold.
  • Behavior: observer-only. The recorder never throws into the caller (build errors and persistence failures are caught and logged), so no task control flow can change.
  • Privacy: every string is capped (2k message / 4k detail values, bounded depth) and secret-scrubbed before leaving the process — provider key prefixes (sk-/ghp_/xox…), bearer headers, JWTs, long hex/base64 runs, and *_PASSWORD=/*_TOKEN= style assignments are redacted (tests cover each). Events land in the same org-scoped DB that already stores full task transcripts, behind the same auth.

Tests

6 new tests: scrubber coverage (keys, JWTs, assignments incl. prefixed names, plain-text passthrough), capped+scrubbed recording, never-throws on persistence failure and on cyclic details. Typecheck clean across types/sdk/worker/web; knip clean.

Not auto-merged — ready for review.

🤖 Generated with Claude Code

Sandbox logs do not survive the sandbox and Modal exposes none at all,
so runtime facts worth a post-mortem keep dying with the machine. This
adds a diagnostic rail on the existing task_run_events audit table:

- a `diagnostic` run event type, recorded by workers through the
  existing sdk.taskRuns.recordEvent path (no schema change),
- a worker-side recorder that is observer-only: fire-and-forget, never
  throws into the caller, caps every string, and scrubs token-shaped
  values and credential assignments before anything leaves the process,
- a `tasks.runEvents` web tRPC reader so per-task events are queryable
  without database access.

Nothing emits through the recorder yet; the OpenCode exit certificate
lands separately on top of this rail.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@roomote-roomote

roomote-roomote Bot commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Reviewed this PR. The diagnostic rail is well-scoped, observer-only, and the never-throw / scrubbing guarantees are backed by tests. 3 non-blocking things to consider (1 medium, 2 low). See task

  • apps/web/src/trpc/commands/tasks/run-events.ts:22-24 (medium) — The reader orders asc(createdAt) then limit(500), so on tasks with more than 500 run events it returns the oldest 500 and silently drops the newest. Since this rail exists for post-mortems and the interesting diagnostics (failures, exit certificates) happen at the end of a run, the reader can drop exactly the events it was built to surface. Consider ordering desc with the limit (reversing client-side for display) or paginating.
  • apps/worker/src/run-task/diagnostic-events.ts:100-106 (low)details is built as { kind: input.kind, ...sanitizedDetails }, so a caller-supplied details.kind silently overrides the recorder's own kind. Spread first and set kind last (or namespace it) so the recorder's classification always wins.
  • apps/worker/src/run-task/diagnostic-events.ts:33-34 (low) — The \b[A-Fa-f0-9]{40,}\b and [A-Za-z0-9+/]{48,} patterns will redact non-secret post-mortem evidence such as 40-char git SHAs and sha256 digests. Over-redaction is the safe direction, but it does blunt the debugging value the rail is meant to provide; consider narrowing these token-shaped rules or exempting known hash-length values.

@daniel-lxs
daniel-lxs merged commit d1142ad into develop Jul 10, 2026
1 check passed
@daniel-lxs
daniel-lxs deleted the feat/diagnostic-events branch July 10, 2026 22:34
daniel-lxs added a commit that referenced this pull request Jul 10, 2026
…icate (#147)

* [Fix] Address review findings on the diagnostic rail and death certificate

Review follow-up for #140 and #145, which merged before their bot
reviews were read:

- Deliberate harness terminations (reconnect, restart, teardown) are no
  longer certified as unexpected exits: ReconnectableHarness marks the
  subprocess before killing it, and certification skips marked exits,
  which also silences the Sentry alert that fired on every routine
  reconnect.
- The death tail no longer depends on the startup URL wait leaving its
  line listeners attached: dedicated readers feed the ring for the
  whole subprocess lifetime.
- The run-events reader returns the newest events under its limit
  (descending query, reversed to chronological), so a busy task cannot
  push the diagnostics it exists to surface out of the window.
- A caller-supplied details.kind can no longer override the recorder's
  classification.
- Hash-shaped values (git SHAs, digests) keep an 8-character prefix
  when redacted, preserving their evidentiary value without exposing
  credential-length material.
- The memory reading is named and documented as what it is: the sandbox
  observed just after the exit, by the surviving worker process — the
  exit code/signal remains the authoritative OOM signal.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* Mark expected exits only while the subprocess is alive

A crash surfaces as a disconnect first, and the disconnect cleanup then
kills the already-dead process; marking it there raced the exit
certification and could suppress the certificate for exactly the crash
the rail exists to record. The mark now applies only to a process that
is still alive at kill time, so deliberate teardowns stay quiet and
self-inflicted deaths stay certified.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
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.

1 participant