Code Review

This pull request enhances diagnostic capabilities by tracking session aborts and title generation events. It introduces an InterruptMeta interface, updates the Runner and SessionRunState to handle cancellation metadata, and extends the assistant message schema to store these diagnostics. Additionally, the export functionality now collects and includes abort and title generation summaries. Reviewers suggested optimizing performance by consolidating tree walks in the export logic and using targeted message lookups instead of fetching entire session histories to reduce overhead.

Opus second-pass review — PR #563

Conditional pass. One P2 to address before merge, three P3 recommendations.

Strengths

• Commit split is clean: commit 1 is pure diagnostics surface (cancel metadata into assistant.diagnostics.abort, ensureTitle trace into title_generation, top-level aggregation in export); commit 2 is the single-intent build fix for #560.
• runner.ts change is backward compatible: cancel still exists (cancel = cancelWith()), so existing callsites stay green.
• ensureTitle() call timing is not changed — Effect.orDie is swapped for Effect.exit so success/failure paths can record traces, but the first-turn fork behavior matches the brief (diagnostic-only PR, no mitigation).
• Commit 2 contract test (test/script/build-node.test.ts) is the right lightweight gate: asserts the source contains OPENCODE_VERSION + Script.version, which is enough to lock the regression without standing up a heavier integration test.
• Field naming is consistent (snake_case in export schema / camelCase in InterruptMeta interface).

[P2] lastInterruptMeta race on concurrent cancels (packages/opencode/src/effect/runner.ts)

let lastInterruptMeta: InterruptMeta | undefined

const cancelWith = (meta) =>
  SynchronizedRef.modify(ref, (st) => {
    lastInterruptMeta = meta ? { ...meta, recordedAt: ... } : { recordedAt: ... }
    switch (st._tag) { ... }
  })

Scenario:

1. cancel1 (soft) calls cancelWith(metaA) → modify writes metaA to lastInterruptMeta, returns the cancel effect.
2. Before cancel1's onInterrupt reads lastInterruptMeta, a second cancel2 (hard) enters → SynchronizedRef serializes the modify, so metaB overwrites metaA.
3. cancel1's onInterrupt reads metaB and attributes it to the cancel that actually originated from metaA.

SynchronizedRef serializes the modify callback, but the modify only records meta + returns the effect; the actual fiber interrupt + Deferred.await happens after the modify returns, leaving a window for the second cancel to enter and overwrite meta.

Severity is "diagnostic false attribution" — the user-visible behavior is unaffected, but the next reproduction's abort metadata could be mis-attributed if cancels collide.

Suggested fix: bind meta to the cancel event rather than to the Runner closure. Options:

• Capture meta inside the state itself (e.g. { _tag: "Cancelling", meta }), and have onInterrupt read from state instead of a closure variable.
• Pass meta as a parameter into the cancel effect, then thread it to onInterrupt directly.
• Or, if you want to minimize churn, document the limitation with a comment: "best-effort recent meta; may be substituted under concurrent cancels".

[P3] recordTitleTrace polling for the assistant message (packages/opencode/src/session/prompt.ts)

for (let attempt = 0; attempt < 50; attempt++) {
  const messages = yield* sessions.messages({ sessionID: input.session.id })
  assistant = messages.find(m => m.info.role === "assistant" && m.info.parentID === firstInfo.id)
  if (assistant) break
  yield* Effect.sleep("10 millis")
}

50 × 10ms = 500ms upper bound. If the assistant message has not been persisted within 500ms (e.g. title generation completes very fast or assistant message creation is delayed for any reason), the trace is silently dropped.

This is intentional best-effort resilience for a diagnostic surface, but please add a single log line on dropout (e.g. elog.warn("title trace dropped: assistant not found after 500ms")) so a missing trace is observable from logs rather than just absent from the export.

[P3] InterruptMeta.viaCtxAbort is always false in v1

The single cancel callsite explicitly sets viaCtxAbort: false. There is no path in this PR that sets it to true. If this field is reserved for future ctx.abort-driven cancels, please annotate that so the next reproduction reader does not mistake "always false" as evidence that the race cannot originate from ctx.abort. Otherwise, hook into ctx.abort paths and set it correctly in a follow-up.

[P3] Missing explicit title_generation_state correlation in abort diagnostic

The diagnostics record abort.recorded_at and title_generation.completed_at as separate fields. To answer the actual RCA question ("was ensureTitle still in flight when the main run was aborted?"), the reader has to compute the time delta between two structured fields.

Consider adding a snapshot field to abort:

abort: {
  ...
  title_generation_state: "not_started" | "in_flight" | "completed_before_abort" | "completed_after_abort"
}

so the next reproduction directly shows the temporal relationship without manual joining. Not blocking; can be a follow-up.

Verdict

P2 should be addressed before merge (a few lines to bind meta to the cancel event or to add a documenting comment). The three P3 items can be folded into the same fixup commit or opened as follow-up issues — your call.

Once the P2 is resolved, this PR is ready for engineering final.

Actionable comments posted: 2

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@packages/opencode/src/session/export.ts`:
- Around line 157-181: sanitizeDiagnostics currently misses redacting the newly
exported diagnostics error text fields; update sanitizeDiagnostics to also
redact diagnostics.aborts[].error_message and
diagnostics.title_generations[].error_message (and the duplicate handling around
the other occurrence at the later block) the same way other error fields are
sanitized — e.g., replace with undefined or a constant redaction placeholder —
so sanitized snapshots never include runtime error text; locate the sanitization
logic in sanitizeDiagnostics and add these two fields to the redact list for
both aborts and title_generations handling.

In `@packages/opencode/src/session/run-state.ts`:
- Around line 78-91: The interrupt metadata can be lost when a runner is removed
before interruptMeta() reads it; modify the cancel logic in
SessionRunState.cancel to persist the InterruptMeta outside the transient runner
(e.g., store it into a durable map on the InstanceState or embed it in the
status via status.set) before calling existing.cancelWith(meta), and change
SessionRunState.interruptMeta to first check the live runner
(data.runners.get(sessionID)) and then fall back to that persisted location (the
new data.interruptMetaMap or status entry) so the provenance survives runner
lifecycle races.

---

Nitpick comments:
In `@packages/opencode/test/script/build-node.test.ts`:
- Around line 5-12: Update the test "build-node injects both release version and
channel defines" to verify runtime/build output instead of only static source
strings: build or import the compiled output of script/build-node.ts (or require
the built module after invoking the build step used by your tooling), then
assert that the runtime exports or session metadata include the actual
OPENCODE_VERSION and OPENCODE_CHANNEL values and that Script.version and
Script.channel reflect them; locate references to OPENCODE_VERSION,
OPENCODE_CHANNEL, Script.version and Script.channel in the test and replace the
fs.readFile assertions with assertions that inspect the built module's exported
metadata or the runtime session object to confirm the real values.

Engineering final review from @GPT-X.

Current PR head reviewed: 8204e0bec.

I found one blocking issue before merge:

P2: sanitized exports leak the new diagnostics error text fields

The PR adds top-level export diagnostics:

• diagnostics.aborts[].error_message
• diagnostics.title_generations[].error_message

However, Export.sanitizeSnapshot() currently delegates to sanitizeDiagnostics(), and sanitizeDiagnostics() only redacts diagnostics.loop.last.attemptedInput. It returns the rest of diagnostics unchanged. That means a sanitized session export can still contain raw runtime error text from abort provenance or title generation diagnostics.

This is a real privacy/sanitization boundary issue, not just a presentation nit. The rest of the sanitize path already redacts assistant errors, tool errors, tool inputs, metadata, paths, and diffs; these new diagnostic error text fields need the same treatment.

Suggested minimal fix:

• Update sanitizeDiagnostics() to preserve the current loop redaction behavior while also mapping:
  • aborts[].error_message through redact("abort-error-message", ...)
  • title_generations[].error_message through redact("title-generation-error-message", ...)
• Add a regression test under export.test.ts that proves sanitizeSnapshot() redacts both new fields.

The Opus P2 fix for per-run interrupt metadata is otherwise clean: the metadata is now bound to the run/shell handle via per-handle refs, and the new runner regression test covers the overwrite race.

Other notes, non-blocking:

• The build-node.test.ts static source contract is acceptable for this PR boundary. A full build-output/runtime test would be heavier and is not necessary for the #560 one-line define regression.
• The deriveSnapshotDiagnostics() multiple walks and sessions.messages() polling comments are performance/ergonomics follow-ups, not merge blockers for this diagnostic PR.
• The P3 diagnostics ergonomics items are already tracked in #564.

Local verification I ran:

• bun --cwd packages/opencode test test/effect/runner.test.ts test/session/export.test.ts test/session/prompt-effect.test.ts test/script/build-node.test.ts --timeout 30000 -> 112 pass
• bun --cwd packages/opencode typecheck -> pass
• git diff --check dev...HEAD -> pass

Please fix the sanitize gap, rerun the targeted export tests/typecheck/diff-check, then ping me for a short re-final.

Actionable comments posted: 3

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@packages/opencode/src/session/prompt.ts`:
- Around line 365-395: The recordTitleTrace implementation (Effect.fn
"SessionPrompt.recordTitleTrace") races with message creation by polling only 50
times, so on slow turns it may never attach title_generation diagnostics to the
assistant message; change the flow to persist the trace durably first (e.g.,
save into the session or turn-level diagnostics via sessions.updateSession or a
dedicated traces store using sessionID/firstInfo.id) and then, when the
assistant message is found via sessions.messages, merge that persisted trace
into the assistant's diagnostics and remove/mark the persisted entry;
alternatively, delay attaching until the assistant exists by subscribing to
message creation events instead of fixed polling — update recordTitleTrace,
ensureTitle callers, and the sessions update/merge logic so the trace is
reliably stored and later attached to the assistant's diagnostics.
- Around line 1992-2021: The onInterrupt handler is updating whichever assistant
is globally last (via lastAssistant(input.sessionID)), which can annotate the
previous turn if the current turn's assistant scaffold hasn't been created yet;
instead, tie the abort to the active assistant for this run: record the current
run's assistant id when the assistant scaffold/parent is created (e.g., store
runAssistantId in the run scope) and in onInterrupt lookup and update that
specific assistant using sessions.updateMessage({ ... }) (or
sessions.getMessage(runAssistantId) before updating) rather than calling
lastAssistant(input.sessionID); ensure interruptedSessions.add(input.sessionID)
remains and return the updated assistant object fetched by id.
- Around line 451-454: The code records recordTitleTrace({ applied: true,
success: true }) unconditionally even when sessions.setTitle(...) failed; change
the flow so you only call recordTitleTrace with applied: true when
sessions.setTitle returns successfully. Specifically, run sessions.setTitle({
sessionID: input.session.id, title: t }) and check its success (handle the
Effect result or use Effect.fold/Effect.match/Effect.map/Effect.catch to detect
failure), log the error via elog.error on failure, and only invoke
recordTitleTrace({ completedAt, success: true, applied: true }) inside the
success branch — otherwise call recordTitleTrace with applied: false or success:
false as appropriate. Ensure the change touches the sessions.setTitle and
recordTitleTrace call sites shown.