fix(otel): build trajectory spans from the events the runtime publishes - #81
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| if (part.type === "step-start") { | ||
| // The turn span opens here rather than under the `llm` gate below: disabling | ||
| // LLM traces must not also delete the turn. | ||
| openTurnSpan(sessionID, messageID, model) | ||
| if (!tracingEnabled("llm")) break |
There was a problem hiding this comment.
MEDIUM (confidence: high) — a turn that fails before its first LLM step produces no prompt span at all, so opencode.turn.outcome = "error" is never emitted for exactly the turns that matter most.
if (part.type === "step-start") {
// The turn span opens here rather than under the `llm` gate below: disabling
// LLM traces must not also delete the turn.
openTurnSpan(sessionID, messageID, model)openTurnSpan is reachable only from this branch. session.error is published from several points in packages/opencode/src/session/prompt.ts that run before any step-start part is written — model/provider resolution and permission paths at prompt.ts:322, prompt.ts:470, prompt.ts:625, prompt.ts:663. On those paths the sequence is:
session.error→turnSpans.get(sessionID)isundefined, so no span status is set;turnErrored.add(sessionID)(line 244).session.idle→endTurnSpanfinds no span, and line 172 drops the flag.
Net result: no prompt span, and the error outcome the PR body says downstream filters on is discarded. The new test "a session error is not reported as a completed turn" sends a step-start first, so it does not cover this shape.
There is already a live signal that fires before the LLM call: session.status with {type:"busy"}, published at packages/opencode/src/session/prompt.ts:1192 (via SessionStatus.set, packages/opencode/src/session/status.ts:41) — and this handler already branches on it at line 217 to emit session.prompt.started. Opening the turn there instead would both cover pre-step failures and make the turn span span preflight latency rather than starting at the first round trip. openTurnSpan's turnSpans.has(sessionID) guard already makes repeated busy events idempotent, which matters because processor.ts:973 re-publishes busy mid-run.
If you keep the step-start anchor, it is worth saying in the comment that pre-step failures are deliberately untraced.
There was a problem hiding this comment.
Fixed — the turn now opens on session.status busy. You were right that the step anchor loses exactly the turns that matter, and busy also puts preflight latency inside the span. New test: "a turn that dies before its first step still produces a span".
| turnSpans.get(sessionID)?.setStatus({ code: SpanStatusCode.ERROR, message }) | ||
| // Without this the turn closes as `completed` while its own status says ERROR, | ||
| // and `opencode.turn.outcome` is exactly what downstream filters on. | ||
| if (sessionID) turnErrored.add(sessionID) |
There was a problem hiding this comment.
MEDIUM (confidence: high) — a turn that recovers via auto-compaction will be reported as error.
turnSpans.get(sessionID)?.setStatus({ code: SpanStatusCode.ERROR, message })
// Without this the turn closes as `completed` while its own status says ERROR,
// and `opencode.turn.outcome` is exactly what downstream filters on.
if (sessionID) turnErrored.add(sessionID)session.error is not always terminal. In packages/opencode/src/session/processor.ts:926-936, a ContextOverflowError with auto-compaction enabled publishes Session.Event.Error and then sets ctx.needsCompaction = true and returns without setting the session idle — the run compacts and continues in the same turn:
ctx.needsCompaction = true
yield* events.publish(Session.Event.Error, { sessionID: ctx.sessionID, error })
returnturnErrored is sticky until endTurnSpan, so that recovered turn closes with span status ERROR and opencode.turn.outcome = "error". Since the attribute is what downstream filters on, successful auto-compacted turns get counted as failures — a new mislabel, because the old (dead) code took the outcome from session.turn.finished's own p.outcome rather than deriving it from error events.
Smallest correct change: don't mark the turn for the recoverable case, e.g. skip when p.error?.name === "ContextOverflowError" (the name from packages/schema/src/session-v1.ts:57) and auto-compaction is on, or clear turnErrored when a subsequent step-finish lands with a non-error reason. Either way the span status set on line 241 has the same problem and should be gated the same way.
There was a problem hiding this comment.
Fixed, and taken a step further than the suggestion. Rather than special-casing ContextOverflowError, the outcome now comes from the runtime's own verdict — the assistant message's error/finish, which processor.ts sets at :928/:952 when a turn really ends and leaves unset when it compacts and continues. The span status is gated the same way, as you noted it had to be. Same reasoning applied to tool errors: a failed tool no longer fails the turn, since the agent usually recovers and the error is already on the tool span. Two new tests cover the recovered and terminal overflow shapes.
| if (contextOwner !== sessionID) return | ||
| // Hand the slot to a session still mid-turn — when a subagent's turn outlives its | ||
| // parent's, SQL and raw-request capture should attach to the one still running | ||
| // rather than to nothing. | ||
| const [next] = turnSpans | ||
| contextOwner = next?.[0] | ||
| setCurrentSessionSpanContext(next?.[1].spanContext()) |
There was a problem hiding this comment.
MEDIUM (confidence: medium) — please confirm the intent here: the process-wide slot is now handed to an arbitrary other session rather than cleared.
const [next] = turnSpans
contextOwner = next?.[0]
setCurrentSessionSpanContext(next?.[1].spanContext())turnSpans iterates in insertion order, so next is the oldest still-open turn — which is the parent in the subagent case the comment describes, but under serve is just "whichever unrelated session started first". Everything that reads the slot then attributes to that session:
emitLogpassesgetSessionOtelContext()as the log context (line 113), so every log record — including ones for other sessions — lands under the owner's trace.getCurrentSessionTraceparent()feedsCLICKZETTA_TRACEPARENTinto spawned shells via theshell.envhook (otel/index.ts:66).recordRawProviderRequestand the SQL capture path attach to the owner.
Before this change, session.turn.finished / session.deleted called setCurrentSessionSpanContext(undefined), so the fallback was no attribution. Now it is wrong attribution, which is harder to notice downstream than a missing parent. The new test only asserts the handoff happens, not that it lands on a related session.
If the subagent case is the motivation, restricting the handoff to a known parent/child relation would express it; otherwise clearing (as before) is the safer default. Either way, worth a line saying which was chosen and why.
(Separately, setCurrentLlmSpan on line 347 is a single global too, so with concurrent sessions a raw provider request from session A can be recorded onto session B's chat span. That is pre-existing, not introduced here.)
There was a problem hiding this comment.
You're right, and I've reverted it — the handoff was my own addition beyond the fix. Under serve the next open turn is an unrelated session, so it traded a missing parent for a wrong one. The slot is cleared by its owner again, with a comment saying why, and the test now asserts the clear rather than the handoff.
| const sessionModels = new Map<string, Model>() | ||
| const sessionStartMs = new Map<string, number>() |
There was a problem hiding this comment.
LOW (confidence: high) — sessionModels is the one new per-session map with no pruning, so it grows for the life of the process.
const sessionModels = new Map<string, Model>()
const sessionStartMs = new Map<string, number>()Every other piece of new per-session state is bounded: turnSpans / turnErrored / stepSpans / toolSpans are cleared in endTurnSpan, and countedTools is explicitly keyed by session so it can be pruned there — the comment on line 53-57 calls that out as the reason ("instead of being held for the life of the process"). sessionModels gets neither treatment: the session.deleted branch deletes sessionStartMs (line 202) but not sessionModels, and nothing removes it on idle.
Entries are small (three strings), so this is growth rather than a leak with teeth — but in serve, which is the mode this PR is otherwise careful about, it is unbounded in the number of sessions the process ever sees. Adding sessionModels.delete(p.sessionID) alongside the sessionStartMs.delete on line 202 covers it without touching the turn lifecycle (the model must outlive individual turns, so pruning in endTurnSpan would be wrong).
There was a problem hiding this comment.
Fixed exactly as suggested — sessionModels.delete alongside the sessionStartMs.delete in session.deleted. Agreed that pruning in endTurnSpan would be wrong, since the model has to outlive individual turns.
| "gen_ai.usage.cache_read.input_tokens": tokens.cache?.read ?? 0, | ||
| "gen_ai.usage.cache_write.input_tokens": tokens.cache?.write ?? 0, |
There was a problem hiding this comment.
LOW (confidence: high) — unremarked attribute rename riding along in a fix PR.
"gen_ai.usage.cache_read.input_tokens": tokens.cache?.read ?? 0,
"gen_ai.usage.cache_write.input_tokens": tokens.cache?.write ?? 0,The old code wrote gen_ai.usage.cache_creation.input_tokens for the same value, on both the span and the log record. Nothing in the PR description mentions the rename, and it isn't part of the bug being fixed.
Impact is genuinely low: I grepped and no other file in this repo reads either name, and the old branch (v2.step.ended) never fired, so no historical rows carry cache_creation either. But it is an independent change mixed into the same PR, and the new test asserts cache_read while leaving cache_write unasserted — so neither spelling is pinned by a test. Either drop the rename or add it to the description and assert the new key in otel-span-build.test.ts next to the existing cache_read assertion.
There was a problem hiding this comment.
Dropped the rename — cache_creation.input_tokens is restored, and the test now asserts it next to cache_read. It wasn't part of this bug and shouldn't have ridden along.
| } | ||
| if (failed) m.errorCounter.add(1, { source: "tool", "gen_ai.tool.name": toolName }) | ||
| countedTools.delete(key) | ||
| if (durationMs) m.toolCallDuration.record(durationMs / 1000, { "gen_ai.tool.name": toolName }) |
There was a problem hiding this comment.
LOW (confidence: high) — zero-duration samples are dropped from the histogram but still written to the log, so the two disagree.
if (durationMs) m.toolCallDuration.record(durationMs / 1000, { "gen_ai.tool.name": toolName })durationMs here is state.time.end - state.time.start from the runtime, and both are millisecond integers (NonNegativeInt, packages/schema/src/session-v1.ts:283-300), so a fast tool legitimately yields 0. The truthiness check then skips the histogram, while the log record eight lines down uses durationMs != null and does emit opencode.tool.duration_ms: 0. Fast tools therefore vanish from opencode.tool.call.duration — which biases the distribution upward, in the direction that matters least to notice.
Same shape for the step duration on line 380 (if (durationMs) m.operationDuration.record(...)), though a measured wall-clock 0 is less likely there.
The pattern is inherited from the deleted if (p.durationMs) code, so this is carried over rather than introduced — but the PR is already changing where the duration comes from, and durationMs != null would make the metric and the log agree.
There was a problem hiding this comment.
Fixed — both duration histograms now use != null, so a zero-duration call is a sample instead of being dropped while still being logged.
| function publishedEvents(): Set<string> { | ||
| const names = new Set<string>() | ||
| for (const file of SCHEMAS) { | ||
| const source = readFileSync(file, "utf-8") | ||
| for (const m of source.matchAll(/type:\s*"([a-z0-9._]+)"/g)) names.add(m[1]!) | ||
| } | ||
| return names | ||
| } |
There was a problem hiding this comment.
LOW (confidence: high) — the guard is real but narrower than the docstring claims, in two ways worth writing down.
function publishedEvents(): Set<string> {
const names = new Set<string>()
for (const file of SCHEMAS) {
const source = readFileSync(file, "utf-8")
for (const m of source.matchAll(/type:\s*"([a-z0-9._]+)"/g)) names.add(m[1]!)
}
return names
}First, publishedEvents means "defined in one of three hand-listed schema files", not "published". I checked: v2.step.* and v2.tool.* appear nowhere in packages/schema, so the orphan test genuinely would have caught the original bug — that part works. But the failure mode the sibling test file's docstring describes as the harder half also has a name-level version this test cannot see: an event that is defined in the schema yet never reaches the plugin event hook in cz's runtime. The hook gets whatever events.listen delivers (packages/opencode/src/plugin/index.ts:251-258, no allowlist), so "defined" and "delivered" are close but not the same set. Naming the function schemaDefinedEvents and saying so in the comment would stop the next reader from over-trusting it.
Second, the fixed three-file list makes the orphan test fail spuriously the next time a subscription is added for an event defined elsewhere in packages/schema (permission-*.ts, installation-event.ts, mcp-event.ts, …) — a correct change would turn this red with a confusing message. Globbing packages/schema/src/*event*.ts plus session-v1.ts, or listing the whole directory, would remove that trap; the existing published.size > 20 assertion already covers the "paths went stale" case either way (it is 44 today).
Also minor: subscribedEvents matches every case "…" in the handler, so a future switch over any lowercase string literal in that file would be read as an event subscription.
There was a problem hiding this comment.
Left the fixed file list, with a comment saying it's deliberate: those three files are the surfaces a plugin can receive, so a subscription outside them is the mistake being guarded, and a legitimate new surface means adding the file. Also added session.status to the required set, since it is now the turn anchor.
Review summaryA. Upstream invasiveness — no issues foundAll three changed files are under The approach is also the hook-shaped one: the handler reads the plugin B. Clean fix, or a hole drilled around the problem?Substantially the clean fix. The root cause — six subscriptions to event names no runtime publishes — is addressed at the root by rebuilding spans from the surface a plugin actually receives, and I verified the payload shapes the new code reads against the schema: Two smaller things in this category:
I did not find a symptom-level patch, a swallowed failure, copy-pasted logic, or leftover debug code. The dedupe-by- C. Regression risk — see inlineTwo behavioral findings that could change what downstream sees, both anchored inline:
Plus one intent question and two low-severity items:
Other behavior changes I checked and consider safe:
I could not run |
Addresses the review on #81. The turn span opened on the first step, so a turn that died before reaching one had no span at all — and prompt.ts publishes session.error from model resolution and permission paths (:322, :470, :625, :663) that run before any part is written, which is exactly the set of turns whose `error` outcome matters. It now opens on `session.status` busy (prompt.ts:1192), the first signal of a turn, which also puts preflight latency inside the span. busy is re-published mid-run (processor.ts:973), so the existing per-session guard is what keeps it idempotent; model and message id are filled in when the first step lands. The outcome was derived from error events, which mislabels a recovered turn: a ContextOverflowError is published and then compacted away within the same turn (processor.ts:932-936), so a successful auto-compacted turn closed as `error` on the very attribute downstream filters on. The verdict now comes from the runtime itself — the assistant message sets `error`/`finish` on the paths that really end a turn (processor.ts:928, :952) and sets neither when it recovers. For the same reason a failed tool no longer fails the turn; the error stays on the tool span, where the agent recovering from it does not erase it. The session-span slot is cleared by its owner again instead of being handed to the next open turn. Under `serve` that next turn belongs to an unrelated session, and log records, CLICKZETTA_TRACEPARENT and raw-request capture all read the slot — so the handoff replaced a missing parent with a wrong one. Also from the review: the `cache_creation.input_tokens` spelling is restored, since renaming it to `cache_write` was an unrelated change riding along; `sessionModels` is pruned on `session.deleted`, the one new per-session map that grew unbounded; and a zero duration is recorded as a sample rather than dropped from the histogram while still being logged. Five new tests cover the shapes involved, including the two the review found uncovered: a turn that dies before its first step, and one that recovers by compacting. Each fix was mutation-checked — reverting it turns exactly its own test red.
| span.end() | ||
| toolSpans.delete(p.id) | ||
| if (failed) m.errorCounter.add(1, { source: "tool", "gen_ai.tool.name": toolName }) | ||
| countedTools.delete(key) |
There was a problem hiding this comment.
MEDIUM (confidence: high on the mechanism, medium on how often it fires)
if (failed) m.errorCounter.add(1, { source: "tool", "gen_ai.tool.name": toolName })
countedTools.delete(key)
if (durationMs != null) m.toolCallDuration.record(durationMs / 1000, { "gen_ai.tool.name": toolName })Deleting the key on settle defeats the dedupe above it for the one case that actually recurs: the runtime republishes already settled tool parts.
packages/opencode/src/session/compaction.ts:282-286stampspart.state.time.compactedon completed tool parts and callssession.updatePart(part).packages/opencode/src/session/session.ts:716-727(Session.fork, exposed asPOST /session/:id/fork) replays every historical part throughupdatePart.session.updatePartpublishesmessage.part.updatedunconditionally (session.ts:633-641), and the plugineventhook receives every published event for the directory (packages/opencode/src/plugin/index.ts:251-258).
Each republished completed state re-enters this branch with countedTools.has(key) === false, so opencode.tool.call.count increments again, opencode.tool.call.duration records the same duration again, and opencode.tool.finished is emitted again. A fork of a long session inflates all three by the full history.
Note endTurnSpan already clears countedTools for the session (line 192), so this delete isn't needed to bound memory. The smaller correct change is to make the settle path itself idempotent — keep the key (or move it into a settledTools set) and return early when a settled state arrives for a callID already settled — so metric and log fire once per call regardless of how many times the part is republished.
compaction.prune is config-gated (compaction.ts:248, off unless compaction.prune is set), but fork is not.
There was a problem hiding this comment.
Fixed as suggested — a settledTools set makes the settle path idempotent and the key is no longer deleted there. Verified both republish routes you named. Worth noting they need two different defences: compaction.prune is forked off the run loop at prompt.ts:1441, so it can land before idle and while the turn is still live — the live-turn gate below cannot catch that one, only this idempotency can. Test: "a settled tool republished by prune is recorded once". It asserts on the emitted log record, not the span; my first attempt asserted spans and the non-idempotent version passed, since a republished settled part has no span left to close.
| if (tokens.input) m.tokenUsage.record(tokens.input, { "gen_ai.token.type": "input", ...modelAttrs }) | ||
| if (tokens.output) m.tokenUsage.record(tokens.output, { "gen_ai.token.type": "output", ...modelAttrs }) | ||
| if (durationMs != null) m.operationDuration.record(durationMs / 1000, { "gen_ai.operation.name": "chat", ...modelAttrs }) |
There was a problem hiding this comment.
MEDIUM (confidence: high on the mechanism)
if (tokens.input) m.tokenUsage.record(tokens.input, { "gen_ai.token.type": "input", ...modelAttrs })
if (tokens.output) m.tokenUsage.record(tokens.output, { "gen_ai.token.type": "output", ...modelAttrs })Same root cause as the tool counter below, different consequence: nothing here gates part-derived telemetry on a turn actually running, and message.part.updated is a replayable stream, not a lifecycle event.
Session.fork (packages/opencode/src/session/session.ts:716-727) republishes every historical part of the source session under new part IDs. So a fork walks the whole history back through this branch: gen_ai.client.token.usage is re-recorded with the historical input/output counts, opencode.llm.step.finished is re-emitted for every past step, and each replayed step-start/step-finish pair opens and immediately closes a chat {model} span with wall-clock timing that has nothing to do with the original call. Token and cost dashboards are the main consumer of these metrics, so the inflation lands where it is least visible as a bug.
A cheap containment: keep a per-session "turn is running" set driven by session.status busy/idle — separate from turnSpans, so it survives OPENCODE_DISABLE_TRACES=prompt — and skip the span/metric/log work for part events belonging to a session with no live turn. Fork and prune both republish outside any busy window, so that one gate covers both.
There was a problem hiding this comment.
Fixed with the containment you proposed — a liveTurns set driven by session.status busy/idle, separate from turnSpans so it survives OPENCODE_DISABLE_TRACES=prompt, gating all part-derived work. Checked that it does not silence anything legitimate: subagent sessions go through the same busy wrapper (task.ts:188 -> ops.prompt -> ensureRunning, run-state.ts:64), and compaction's own summarisation runs inside the turn (compaction.ts:397-403). Test: "a forked session's history produces no spans, metrics or logs".
| // A step part carries no timings, so this one duration is measured. | ||
| const durationMs = entry ? Date.now() - entry.startedMs : undefined |
There was a problem hiding this comment.
MEDIUM (confidence: high)
// A step part carries no timings, so this one duration is measured.
const durationMs = entry ? Date.now() - entry.startedMs : undefinedentry is the span, so this couples a metric to the trace gate. With OPENCODE_DISABLE_TRACES=llm, step-start returns at line 358 before any stepSpans entry exists, so here entry is undefined → durationMs is undefined → gen_ai.client.operation.duration is never recorded and opencode.llm.duration_ms disappears from the llm.step.finished log record.
That contradicts this file's own stated invariant at lines 22-23:
// Parse OPENCODE_DISABLE_TRACES once. Value is comma-separated categories,
// e.g. "tool,llm". Logs and metrics are never affected.The old code took p.durationMs off the event payload, which was gate-independent. The tool branch below gets this right by reading state.time.start/end from the payload.
Smallest fix: record the step's start timestamp in its own map on step-start before the tracingEnabled("llm") check (the same way enrichTurnSpan is deliberately called before it), and read the duration from there rather than from the span entry.
There was a problem hiding this comment.
Fixed — the start time now lives in its own stepStartMs map, written before the tracingEnabled("llm") check, so the duration no longer depends on the span. You were right that this contradicted the invariant at the top of the file. Not covered by a test: OPENCODE_DISABLE_TRACES is parsed once at module load, so a same-process test cannot flip it, and I did not want a test-only seam in the handler for it.
Separately, checking this turned up that the outcome rule from the last round was itself wrong. On both terminal paths halt publishes the error and sets idle (processor.ts:926-931, :952-957), while the message carrying error is only published afterwards by ensuring(cleanup()) (processor.ts:913, :1027) — after the turn has closed, so the message can never be the verdict. It now records the error as pending and clears it when the next step starts: a recovered error is followed by another round trip, a terminal one by idle. That also removes the ContextOverflowError name check.
| const settled = status === "completed" || status === "error" | ||
| if (!settled) { | ||
| if (toolSpans.has(key) || !tracingEnabled("tool")) break | ||
| const args = _recordContent && state.input != null ? safeStringify(state.input) : undefined |
There was a problem hiding this comment.
MEDIUM (confidence: high on the change in effect, medium on whether it's intended)
const args = _recordContent && state.input != null ? safeStringify(state.input) : undefinedand at line 482:
entry.span.setAttribute("gen_ai.tool.call.result", String(state.output))Worth calling out explicitly because of what this PR fixes: these branches were dead, so tool arguments and results were never actually exported. After this change they are, by default — _recordContent is process.env.OPENCODE_OTEL_RECORD_CONTENT !== "0" (otel/index.ts:52), i.e. opt-out.
Two consequences:
- Content leaves the machine.
state.inputforbashis the full command line, which routinely carries tokens, passwords and connection strings;state.outputforread/grep/webfetchis the file or page content. The PR body says "traces remain without prompt/completion content", which is true ofgen_ai.input.messagesbut not of these two attributes — the trace now carries a good share of the same material by way of the tools. Given this repo's history with plaintext credentials in_positional, that seems worth stating in the PR description at minimum, and possibly worth defaultingOPENCODE_OTEL_RECORD_CONTENToff for the tools that can carry secrets. - No size cap.
String(state.output)on a whole-filereadbecomes a single span attribute; collectors commonly drop or truncate spans past an attribute-size limit, which can silently cost you the span you wanted. Upstream truncates tool output for the model (truncateToolOutputinsession/message-v2.ts); the same treatment here would be cheap.
Neither is a defect in the diff's logic — flagging the exposure that goes live with it.
There was a problem hiding this comment.
Both taken. The size cap is in: 8k chars on gen_ai.tool.call.arguments and gen_ai.tool.call.result, for exactly the reason you give — losing the span is worse than losing the text.
On the default: kept opt-out, deliberately, and the description now has a section stating the exposure rather than implying it away (the old "traces remain without prompt/completion content" line was misleading, since it was only ever true of gen_ai.input.messages). The trajectory is what this PR exists to restore, and shipping it with content off by default would deliver spans without what makes them useful; OPENCODE_OTEL_RECORD_CONTENT=0 turns both off and still leaves spans, timings, tokens, tool names and errors. Per-tool defaults would need a maintained list of which tools can carry secrets, and I did not want to add a boundary that quietly rots.
| if (!span || turnEnriched.has(sessionID)) return | ||
| turnEnriched.add(sessionID) |
There was a problem hiding this comment.
LOW (confidence: high on the logic, low on it firing in practice)
if (!span || turnEnriched.has(sessionID)) return
turnEnriched.add(sessionID)The latch is set before anything is known to have been written, so if model is undefined at the turn's first step the turn span never gets gen_ai.request.model / opencode.agent.name — not even from a later step in the same turn, once the model is known. That is the same silent-anonymity failure mode as the chat unknown regression the tests were written to catch, just on the turn span instead of the step span.
In the normal v1 path this shouldn't happen: prompt.ts:1289-1304 publishes the assistant message.updated (with flat modelID/providerID/agent) before the processor streams, so sessionModels is populated before the first step-start. So this is a robustness point, not an observed bug.
Latching on model?.modelID being present (or simply re-setAttributes each step, which is idempotent for identical values) removes the failure mode without costing anything.
There was a problem hiding this comment.
Fixed — the latch now keys on model?.modelID being present. Agreed on the failure mode being the same shape as chat unknown, which is the reason to close it even though the normal v1 ordering (prompt.ts:1289-1304 before the processor streams) means it should not fire. Test: "a turn whose model arrives late still gets it".
| // A ContextOverflowError is recovered from by compacting within the same turn, so | ||
| // it is not on its own a failed turn — see markTurnFailed. | ||
| if (name !== "ContextOverflowError") markTurnFailed(p.sessionID ?? "", message) |
There was a problem hiding this comment.
LOW–MEDIUM (confidence: medium-high on the ordering)
// A ContextOverflowError is recovered from by compacting within the same turn, so
// it is not on its own a failed turn — see markTurnFailed.
if (name !== "ContextOverflowError") markTurnFailed(p.sessionID ?? "", message)The reasoning is right for the recovering case, but there is one path where a ContextOverflowError is terminal and the fallback verdict arrives too late to be recorded.
processor.ts:926-932 (auto-compaction disabled, i.e. compaction.auto === false or OPENCODE_DISABLE_AUTOCOMPACT):
ctx.assistantMessage.error = error
ctx.assistantMessage.finish = "error"
yield* events.publish(Session.Event.Error, { sessionID: ctx.sessionID, error })
yield* status.set(ctx.sessionID, { type: "idle" }) // :931
returnThe assistant message is mutated in memory but not published here — the message.updated that carries error/finish comes from cleanup() (processor.ts:913-914), which is attached via Effect.ensuring at :1027 and therefore runs after halt. So the event order the handler sees is: session.error (filtered out by name) → session.status idle → message.updated with the error. endTurnSpan has already closed the turn as opencode.turn.outcome: "completed" by the time the verdict lands.
The late markTurnFailed then also adds the session to turnErrored with no span open (line 163-165); harmless, since openTurnSpan clears it on the next turn, but it does mean the flag briefly describes no turn.
The non-overflow paths are fine — halt publishes Session.Event.Error before setting idle at :957, so those turns are correctly marked.
Note otel-span-build.test.ts:193-214 covers this case by sending message.updated before session.idle, which is the opposite of the runtime order above — so the test passes but doesn't pin the real sequence. Worth reordering that test to match, whichever way you resolve the handler.
There was a problem hiding this comment.
Already fixed in 19f2a0f, which landed while this round was running — same conclusion, reached from the same place in processor.ts. The verdict no longer comes from the message at all: an error is recorded as pending and cleared by the next step start, so a recovered error resolves to completed and a terminal one to error without depending on anything that arrives after idle. The name check on ContextOverflowError is gone too, and the test was reordered to the real sequence (error → idle → message.updated) as you asked.
| */ | ||
| function openTurnSpan(sessionID: string) { | ||
| if (!sessionID || turnSpans.has(sessionID) || !tracingEnabled("prompt")) return | ||
| const span = tracer.startSpan("prompt", { attributes: { "opencode.session.id": sessionID } }) |
There was a problem hiding this comment.
MEDIUM — please confirm intent (confidence: high on the behavior, unsure whether it's what you want)
const span = tracer.startSpan("prompt", { attributes: { "opencode.session.id": sessionID } })No context argument, so the turn span is always a root span. Combined with per-session turn state, that means a subagent's trajectory becomes its own trace, with no parent and no link back to the parent session's turn: the task tool span sits in trace A while every chat/execute_tool span the subagent produced sits in trace B. In a trace viewer the parent turn shows a long-running execute_tool task with nothing under it.
The old code was worse (the child's steps landed under whichever prompt span happened to own the global slot), so this isn't a regression — but it may not be the end state you want either, and the PR body reads as if nesting was the problem being solved rather than the relationship being dropped.
The parent link is available: session.created carries info.parentID, so the handler could keep a child→parent map and either start the child's turn span in the parent turn's context or attach a span link. If you'd rather keep traces per session deliberately, a note in the comment above would save the next reader the same question.
There was a problem hiding this comment.
Good catch, and fixed rather than documented — the relationship was dropped by my change, not deliberately. session.created carries parentID, so a child turn is started in its parent turn's context and also records opencode.session.parent.id; parent and child now share one trace. Test asserts the child's parent span id and the shared trace id.
The first-come ownership of the process-wide slot you note at the end is unchanged — that one is genuinely pre-existing and I have left it alone.
| if (contextOwner !== sessionID) return | ||
| // Cleared rather than handed to another open turn. The slot is read by emitLog, by | ||
| // CLICKZETTA_TRACEPARENT for spawned shells and by raw-request capture, and under | ||
| // `serve` the next open turn is an unrelated session — attributing to it would turn a | ||
| // missing parent into a wrong one, which is far harder to notice downstream. | ||
| contextOwner = undefined | ||
| setCurrentSessionSpanContext(undefined) |
There was a problem hiding this comment.
LOW — description/code mismatch, please confirm which is intended
// Cleared rather than handed to another open turn. ...
contextOwner = undefined
setCurrentSessionSpanContext(undefined)The code (and otel-span-build.test.ts:272-293) clears the slot. The PR description says the opposite:
the process-wide session slot in
otel/context.tsis handed to a session still mid-turn rather than dropped.
The code's reasoning is the more defensible of the two — a wrong parent is harder to notice downstream than a missing one — so this reads like a stale line in the description rather than a code problem. Worth fixing the description so the next reader doesn't go looking for handoff logic that isn't there.
One consequence to be aware of either way: ownership is first-come and never re-acquired, so under serve a session that goes busy while another owns the slot will never own it, and its log records plus any CLICKZETTA_TRACEPARENT handed to spawned shells carry the other session's trace until that one goes idle. Same class of imprecision as before this PR, just with different winners.
There was a problem hiding this comment.
You're right, the description was stale — my earlier edit replaced a different sentence and left this one. Fixed: it now says the slot is cleared by its owner and gives the reason (a wrong parent is harder to notice than a missing one). The consequence you describe about first-come ownership is also unchanged and left alone as pre-existing.
| } catch {} | ||
| } | ||
|
|
||
| export function shutdown() { |
There was a problem hiding this comment.
LOW (confidence: high; pre-existing, not introduced here)
export function shutdown() {shutdown is imported in otel/index.ts:3 as shutdownHandlers and never called — the plugin only registers sdk.shutdown() on beforeExit/SIGTERM/SIGINT, and there is no dispose hook wiring this up. So in production the spans still open at exit are never ended and never exported.
That matters for two of the new comments, which lean on shutdown as the backstop:
- line 178: "instead of being left to leak until shutdown" — with
shutdownunwired, the alternative isn't a late close, it's silent loss. - the
sessionModels/sessionStartMsmaps are only cleared onsession.deletedor here, so under a long-livedservethey grow with every session the process ever sees. Small per entry, unbounded over time.
Wiring shutdownHandlers() into the plugin's dispose hook (or into the existing flush closure, before sdk.shutdown()) would make the comments true and cost one line.
There was a problem hiding this comment.
Confirmed and fixed — shutdownHandlers() now runs inside flush, before sdk.shutdown(). You were right that this made two of my comments false: without it the alternative to closing spans on turn end was silent loss, not a late export. sessionModels and sessionParents are pruned on session.deleted as well, so the long-lived serve case does not depend on shutdown for those.
| const orphans = subscribedEvents().filter((name) => !published.has(name)) | ||
| expect(orphans).toEqual([]) |
There was a problem hiding this comment.
LOW (confidence: high)
const orphans = subscribedEvents().filter((name) => !published.has(name))
expect(orphans).toEqual([])This asserts "defined in the schema", but the bug it is guarding against was "defined in the schema and never delivered on the path cz runs" — every dead v2.* subscription would have been caught by the name check, while session.next.step.started (a real schema entry, published only from the core runner) would sail through it. The comment block at the top of the file describes the rename, so the test does cover that specific history; it just doesn't cover the class.
Concretely: session.next.model.switched is subscribed at handlers.ts:311 and is a session.next.* name, which by the PR body's own reasoning a v1-path plugin should not receive. It happens to be fine — packages/opencode/src/session/prompt.ts:712 publishes it from the v1 prompt path unconditionally — but this test would have passed identically had it not been, which is the gap worth naming.
If you want the stronger guarantee, pinning the delivered surface (the set the 2026-08-24 instrumentation measured) as an explicit allowlist in this file, with session.next.model.switched listed alongside the prompt.ts:712 publish site as the reason it is admitted, would catch the next subscription to a schema name that never arrives.
There was a problem hiding this comment.
Agreed, and changed — the test now checks subscriptions against the measured delivered surface, not just the schema, with session.next.model.switched admitted explicitly and prompt.ts:712 named as the reason it is. The schema check is kept alongside it, since the two catch different mistakes: a name that exists nowhere, and a name that exists but never arrives.
|
Review summary Read A. Upstream invasiveness — no issues found All three changed files live under B. Clean fix, or a hole drilled around the problem? — mostly the clean fix; two caveats inline The diagnosis checks out. Grepping Two places do route around a property of the new data source rather than account for it:
C. Regression risk Behavior that changes, with test coverage noted. I could not run tests, so nothing below is a claim that anything passes.
The two new test files are a genuine improvement in coverage for this failure mode. Neither exercises metrics, which is where both inline findings land. |
…tcome rule Addresses the second review round on #81. Message parts are a replayable stream, not lifecycle events. `Session.fork` republishes every historical part through `updatePart` (session.ts:716-727) and `compaction.prune` stamps `time.compacted` on completed tool parts and writes them back (compaction.ts:282-286); `updatePart` publishes unconditionally, so a fork of a long session re-recorded its whole history — tokens, durations, tool counts, and a `chat` span per past step with wall-clock timing unrelated to the original call. Part-derived work is now gated on the session having a turn actually running, tracked from `session.status` busy/idle and deliberately separate from the span map so it survives `OPENCODE_DISABLE_TRACES=prompt`. Settling a tool is idempotent as well, which is the half the gate cannot cover: prune is forked off the run loop and can land before idle. Step duration was read from the span entry, so `OPENCODE_DISABLE_TRACES=llm` also removed `gen_ai.client.operation.duration` and `opencode.llm.duration_ms` — against this file's own contract that gates never affect logs or metrics. The start time now lives in its own map, written before the gate. The outcome rule from the previous round was wrong, found while checking the above. On both terminal paths `halt` publishes the error and sets the session idle (processor.ts:926-931, :952-957), and the assistant message carrying `error` is only published afterwards by `ensuring(cleanup())` (processor.ts:913, :1027) — after the turn has closed. So the message can never be the verdict. What is observable before idle is whether the turn continued: an error is recorded as pending and cleared by the next step start, since a recovered error is followed by another LLM round trip and a terminal one by idle. This drops the `ContextOverflowError` name check as well. Tests: a forked session's replayed history produces no spans and no log records, a settled tool republished by prune is recorded once, a terminal overflow fails the turn in the order the runtime actually emits it, and a late error does not bleed into the next turn. Log records are now captured alongside spans, because a duplicate settle leaves no span to count — asserting only on spans let the non-idempotent version pass. Not covered by a test: the step-duration gate independence. OPENCODE_DISABLE_TRACES is parsed once at module load, so a same-process test cannot flip it, and a test-only seam in the handler is not worth it.
| if (toolSpans.has(key) || !tracingEnabled("tool")) break | ||
| const args = _recordContent && state.input != null ? safeStringify(state.input) : undefined |
There was a problem hiding this comment.
HIGH (confidence: high) — gen_ai.tool.call.arguments will always be "{}" in production.
if (toolSpans.has(key) || !tracingEnabled("tool")) break
const args = _recordContent && state.input != null ? safeStringify(state.input) : undefinedThe span opens on the first non-settled state, and that state is always pending, whose input is literally {}. ensureToolCall creates the part with state: { status: "pending", input: {}, raw: "" } (packages/opencode/src/session/processor.ts:333) on tool-input-start, and also on the tool-call path when a provider streams no input deltas (processor.ts:472). The real arguments arrive on the next update, when the state transitions to running with input filled (processor.ts:508-514) — but by then toolSpans.has(key) is true, so that event returns early and the attribute is never refreshed.
So safeStringify({}) → "{}", which is truthy, and every execute_tool span ships gen_ai.tool.call.arguments = "{}". That is one of the two attributes this PR set out to restore ("execute_tool {tool}, arguments + result").
otel-span-build.test.ts:107 does not catch it because it feeds running as the tool's first state; the runtime always publishes pending first.
Smallest correct change is to refresh the attribute on the running transition instead of dropping the event:
| if (toolSpans.has(key) || !tracingEnabled("tool")) break | |
| const args = _recordContent && state.input != null ? safeStringify(state.input) : undefined | |
| const args = _recordContent && state.input != null ? safeStringify(state.input) : undefined | |
| const open = toolSpans.get(key) | |
| if (open) { | |
| if (args) open.span.setAttribute("gen_ai.tool.call.arguments", args) | |
| break | |
| } | |
| if (!tracingEnabled("tool")) break |
Worth a test that starts the tool at pending with input: {} and asserts the arguments after the running update — that shape is what the runtime emits, and it is currently untested.
There was a problem hiding this comment.
Confirmed and fixed — this is the best catch of the review. pending really is always first with input: {} (processor.ts:333), so the attribute this PR set out to restore was shipping "{}" on every span. Took your suggestion: the running transition refreshes the attribute instead of returning early.
The test is the part that stings — it fed running as the first state, so it asserted a shape the runtime never emits and passed on a broken handler. It now starts at pending with an empty input, which also means the mutation check bites: dropping the refresh turns that test red.
| if (settledTools.has(key)) break | ||
| settledTools.add(key) |
There was a problem hiding this comment.
MEDIUM (confidence: high on the mechanism) — the settle-once guard is scoped to the turn, but prune replays tool parts from earlier turns.
if (settledTools.has(key)) break
settledTools.add(key)settledTools and countedTools are purged per session in endTurn (lines 222-226). compaction.prune republishes settled tool parts from turns that already ended: it walks backwards skipping the two most recent user turns (if (turns < 2) continue, packages/opencode/src/session/compaction.ts:264), stamps time.compacted on completed parts and writes them back through session.updatePart (compaction.ts:284). It is forked into the run scope at prompt.ts:1440, so those parts land while a later turn is live — they pass the liveTurns gate with callIDs that are no longer in either dedupe set.
Per pruned part that means another opencode.tool.call.count increment, another opencode.tool.call.duration sample and another opencode.tool.finished log record, for a call that completed several turns ago. No span shows up (that call's span was closed and dropped long ago), which is exactly why the span assertion in otel-span-build.test.ts:356 passes — that test replays a part from the current turn, where the dedupe key is still present.
A compacted part is self-identifying, so the precise guard is the marker prune itself writes:
| if (settledTools.has(key)) break | |
| settledTools.add(key) | |
| if (settledTools.has(key) || state.time?.compacted != null) break | |
| settledTools.add(key) |
prune only stamps compacted on parts it rewrites and breaks out when it meets one that already carries it (compaction.ts:271), so a live settle never has it. Note the counter above at line 483 needs the same treatment — it fires before this guard, so a replayed part still increments opencode.tool.call.count even with the fix above.
How often this fires depends on compaction.prune being enabled in config, which is why I'd call it MEDIUM rather than HIGH.
There was a problem hiding this comment.
Confirmed and fixed with the marker you suggested. I had missed that prune's if (turns < 2) continue means it only ever rewrites parts from turns that already ended — my per-turn dedupe was structurally unable to catch it. state.time?.compacted != null now short-circuits before the counter as well, so it covers all three of the counter, the duration sample and the log record.
The test replays a turn later now instead of within the same turn, which is what made the old one pass against a handler that could not have worked.
| if (!contextOwner) { | ||
| contextOwner = sessionID | ||
| setCurrentSessionSpanContext(span.spanContext()) | ||
| } |
There was a problem hiding this comment.
MEDIUM (confidence: high on the mechanism, medium on impact) — this makes the process-wide session slot live for the first time, and it is first-writer-wins.
if (!contextOwner) {
contextOwner = sessionID
setCurrentSessionSpanContext(span.spanContext())
}Before this PR the only call that set a real value lived in the dead session.turn.started branch, so the slot in otel/context.ts was permanently undefined: getCurrentSessionTraceparent() returned nothing, emitLog fell back to context.active(), and chat.headers never set a traceparent. Now the slot is populated — and acquired by whichever session opens a turn first, for the whole process.
Under serve with concurrent sessions (or a subagent's child session running alongside its parent), a second session's telemetry is now attributed to the first session's turn rather than to nothing:
emitLogpassesgetSessionOtelContext()for every record, so session B'ssession.error/llm.step.finished/tool.finishedrecords carry A's trace_id and span_id.outbound-headers.ts:9stampscurrentTraceparent()on ClickZetta gateway requests, andtraceparent.ts:14falls through togetCurrentSessionTraceparent()— no OTel context manager is registered insetup.ts, soactiveTraceparent()is always undefined and the fallback is what's used. B's LLM calls go out under A's traceparent.- same for
CLICKZETTA_TRACEPARENTin theshell.envhook (otel/index.ts:66).
endTurn argues this exact case the other way — "attributing to it would turn a missing parent into a wrong one, which is far harder to notice downstream" — and that reasoning applies just as well at acquisition time as at release time. Both consumers know their sessionID, so a per-session lookup is available: emitLog could take sessionID and resolve turnSpans.get(sessionID), and the header/shell hooks could take the session from their own input.
For single-session TUI use this is a straight improvement, so I'm not calling it a regression outright. Is the single global slot an accepted limitation for now, or worth narrowing before it goes live?
There was a problem hiding this comment.
Narrowed where it was cheap, documented where it was not — and you're right that the slot goes live for the first time here, which I should have called out myself.
Narrowed: emitLog now takes the sessionID and resolves that session's own turn, so log records for a second concurrent session are no longer parented to whichever session acquired the slot first. All seven call sites pass it. Test asserts a record for session B lands under B's turn while A owns the slot.
Not narrowed: getCurrentSessionTraceparent for outbound gateway headers and CLICKZETTA_TRACEPARENT for spawned shells. Neither call site has a sessionID, so narrowing them means threading session context through outbound-headers.ts and the shell hook — a bigger change than this PR should carry. It stays first-writer-wins, and the comment where the slot is acquired now names the consequence: under serve, a second session's gateway calls carry the first session's traceparent. Happy to do it as a follow-up if you'd rather it not ship that way.
| } else if (_recordContent && state.output) { | ||
| entry.span.setAttribute("gen_ai.tool.call.result", String(state.output)) |
There was a problem hiding this comment.
MEDIUM (confidence: high that it is newly live; the judgement call is yours) — full tool output now leaves the process on a span attribute, for the first time and on by default.
} else if (_recordContent && state.output) {
entry.span.setAttribute("gen_ai.tool.call.result", String(state.output))
}The equivalent line existed before on v2.tool.ended, but that branch never ran, so no tool span and no tool output has ever been exported. This PR turns it on, and _recordContent defaults to true (otel/index.ts:52: process.env.OPENCODE_OTEL_RECORD_CONTENT !== "0").
ToolStateCompleted.output is unbounded: a read of a large file, grep over a repo, bash stdout. Two consequences:
- Content. A
readof.envor abashthat echoes a token now lands verbatim inotel_traces, and the same applies togen_ai.tool.call.argumentsonce the pending/running issue above is fixed (abashcommand line frequently carries a credential). This repo's review notes call out plaintext credentials reaching telemetry as a known past incident. - Size.
setup.tsconfiguresBasicTracerProviderwith nospanLimits, soattributeValueLengthLimitis unlimited, and theBatchSpanProcessorruns at defaults. A few multi-megabyte tool outputs in one batch is a large OTLP payload with no truncation anywhere in the path.
Upstream already has a precedent for a cap on exactly this value — TOOL_OUTPUT_MAX_CHARS = 2_000 in compaction.ts:33. A length cap here (and on the arguments attribute), or making content export opt-in rather than opt-out, would bound both problems. Since the PR body already flags prompt/completion content as a separate change with its own privacy surface, it seems worth deciding the same question for tool content rather than shipping it by default as a side effect.
| if (statusType === "busy") { | ||
| if (p.sessionID) liveTurns.add(p.sessionID) | ||
| openTurnSpan(p.sessionID ?? "") |
There was a problem hiding this comment.
LOW (confidence: high on the path, low on impact) — a pending error can outlive the turn it belongs to.
if (statusType === "busy") {
if (p.sessionID) liveTurns.add(p.sessionID)
openTurnSpan(p.sessionID ?? "")pendingErrors is only cleared by a step-start (line 397) or by endTurn, and neither necessarily follows an error published outside a busy window. createUserMessage publishes Session.Event.Error and then throws at prompt.ts:663 (agent-not-found), and it is called at prompt.ts:1160 — before loop(), therefore before the status.set(busy) at prompt.ts:1192. So there is no turn span to receive it and no idle to follow: the entry stays in the map. The next turn on that session inherits it and, if that turn ends without reaching a first step, closes as opencode.turn.outcome = "error" with span status ERROR.
Opening a turn is the natural reset point:
| if (statusType === "busy") { | |
| if (p.sessionID) liveTurns.add(p.sessionID) | |
| openTurnSpan(p.sessionID ?? "") | |
| if (statusType === "busy") { | |
| if (p.sessionID && !liveTurns.has(p.sessionID)) pendingErrors.delete(p.sessionID) | |
| if (p.sessionID) liveTurns.add(p.sessionID) | |
| openTurnSpan(p.sessionID ?? "") |
Guarded on liveTurns rather than on turnSpans so it still works under OPENCODE_DISABLE_TRACES=prompt, and so the mid-run busy re-publish at processor.ts:973 does not wipe a genuine pending error.
The window where this actually shows up is narrow — most turns that end do reach a step-start, which clears it — so LOW.
| for (const [key, entry] of stepSpans) { | ||
| if (entry.sessionID !== sessionID) continue | ||
| entry.span.end() | ||
| clearCurrentLlmSpan(entry.span) | ||
| stepSpans.delete(key) | ||
| stepStartMs.delete(key) | ||
| } |
There was a problem hiding this comment.
LOW (confidence: high) — stepStartMs is not cleaned up when OPENCODE_DISABLE_TRACES=llm.
for (const [key, entry] of stepSpans) {
if (entry.sessionID !== sessionID) continue
...
stepStartMs.delete(key)
}The start time is deliberately written before the llm gate (line 394) so the duration metric survives it — correct, and the reasoning in the comment holds. But the only two places it is removed are step-finish (line 438) and this loop, which is driven off stepSpans. With llm tracing disabled there is never a stepSpans entry, so any step that starts and never finishes — aborted turn, interrupted stream, a turn that dies mid-step — leaves its messageID in stepStartMs for the life of the process.
Small per entry, and only on that env setting, but unbounded over a long-lived serve. Since the map is keyed by messageID alone there's no way to sweep it per session; storing { sessionID, startedMs } as the value (mirroring stepSpans) would let endTurn clean it the same way it cleans the span maps, independent of the gate.
| // The model must outlive individual turns, so this is the only place it can go. | ||
| sessionModels.delete(p.sessionID) |
There was a problem hiding this comment.
LOW (confidence: high) — session.deleted is a weak eviction point for the two per-session maps.
// The model must outlive individual turns, so this is the only place it can go.
sessionModels.delete(p.sessionID)The comment is right that endTurn cannot own this — the model has to survive between turns. But session.deleted only fires when a session is actually deleted, which under serve or a long TUI run mostly never happens, so sessionModels (new here) and sessionStartMs (pre-existing) keep growing for the process lifetime.
Three short strings per session, so this is genuinely minor and may not be worth acting on. If you do want a bound, an LRU cap is the option that doesn't fight the "must outlive a turn" constraint.
|
Review summary I verified the diagnosis before reviewing the fix. It holds: no A. Upstream invasiveness — no issues found The PR touches three files, all under B. Clean fix, or a hole drilled around the problem? — mostly clean, one scoping hole Rebuilding spans from The one place the fix stops short of its own cause is the tool settle guard — C. Regression risk Everything the old code did on
No tests were deleted, skipped or loosened; both new files are additions. Nothing above blocks; they are suggestions to take or leave. |
…hutdown Addresses the third review round on #81. Making turn spans per-session dropped the relationship between them: every turn started as a root span, so a subagent — which runs in its own session — became a detached trace, and the parent turn showed an `execute_tool task` span with nothing under it. `session.created` carries `parentID` (task.ts:145 sets it), so a child turn is now started in its parent turn's context and records `opencode.session.parent.id`. Tool arguments and results are capped at 8k chars. These branches were dead before this PR, so this is the first time they export anything, and a whole-file `read` output otherwise becomes one oversized attribute — collectors drop or truncate the span rather than the text. The `OPENCODE_OTEL_RECORD_CONTENT` default is deliberately unchanged, and the exposure it carries (bash command lines, file and page content) is now stated in the PR description instead of being implied absent. `shutdown()` was imported by the plugin as `shutdownHandlers` and never called, so spans still open at exit were silently lost, and two comments in this file claimed shutdown as their backstop. It now runs inside `flush`, before the SDK stops accepting spans. The turn-enrichment latch was set before anything was written, so a first step that ran before the model was known left the turn permanently anonymous — the same silent failure as `chat unknown`. It latches on the model being present instead. The event-contract test asserted "defined in the schema", which is weaker than the property that actually broke: `session.next.*` are all real schema entries that a v1-path plugin never receives. Subscriptions are now checked against the measured delivered surface, with `session.next.model.switched` admitted explicitly and its publish site (prompt.ts:712) named as the reason. Verified: typecheck clean, 1148 pass / 65 skip / 0 fail. One earlier run of the same tree reported 4 failures and 1 file error whose names were lost before they could be read; two subsequent full runs are clean, so it is recorded here as unreproduced rather than as resolved.
| @@ -62,8 +76,12 @@ export const OtelPlugin: Plugin = Object.assign( | |||
| async event({ event }: { event: { type: string; properties: Record<string, any> } }) { | |||
There was a problem hiding this comment.
MEDIUM (confidence: high) — the root cause of the original bug is still here, and the new contract test works around it rather than removing it.
async event({ event }: { event: { type: string; properties: Record<string, any> } }) {packages/plugin/src/index.ts:224 declares the hook as event?: (input: { event: Event }) => Promise<void>, where Event is a fully discriminated union (packages/sdk/js/src/gen/types.gen.ts:704), and each member is exactly { type: "…"; properties: {…} } — e.g. EventSessionStatus at types.gen.ts:467. This annotation widens that union to { type: string }, which is precisely why case "v2.step.started" compiled and why bun run typecheck stayed green while every span branch was dead.
Typing handleEvent against the union instead would have made the original rename a compile error:
import type { Event } from "@opencode-ai/sdk"
async event({ event }: { event: Event }) { handleEvent(event) }session.next.model.switched lives in the v2 union (sdk/js/src/v2/gen/types.gen.ts:20) rather than the v1 one, so the parameter would need to be Event | EventV2 — but the narrowing is available, and it is checked by the compiler on every build rather than by a regex over source text.
otel-event-contract.test.ts still earns its place: "defined in the schema" and "delivered on the v1 path" are different properties and only the test can assert the second. The concern is that with the widening left in, the type system remains unable to catch the next rename, and the guard that replaced it is [...source.matchAll(/case\s+"([a-z0-9._]+)"/g)] over one file — it goes blind the moment a subscription is written as if (event.type === "…"), or moved to a helper.
There was a problem hiding this comment.
Fixed, and this is the root cause — thank you for naming it. handleEvent now takes the published union's type, and a bogus subscription is a compile error: dropping case "v2.step.started" back in produces error TS2678. That is the first time this class of bug is caught before runtime.
Typing it also surfaced something the review had inferred: session.next.model.switched is NOT in the v1 Event union — it belongs to the durable bus and the runtime delivers it anyway. It is admitted as a single named exception with the reason next to it, so any further out-of-contract subscription has to be written down.
97c9255 to
fdafe58
Compare
| function redactText(value: string): string { | ||
| const masked = value | ||
| .split(/(\s+)/) | ||
| .map((token) => (isSensitiveValue(token) ? "<redacted>" : token)) | ||
| .join("") | ||
| return redactSql(masked) | ||
| } |
There was a problem hiding this comment.
HIGH (confidence: high) — the redactor misses two credential shapes that this PR makes live by default.
function redactText(value: string): string {
const masked = value
.split(/(\s+)/)
.map((token) => (isSensitiveValue(token) ? "<redacted>" : token))
.join("")
return redactSql(masked)
}isSensitiveValue (telemetry.ts:49-52) only fires on a token that is literally KEY=VALUE with no whitespace, and redactSql (logger.ts:8,13) only rewrites literals inside single quotes (RE_QUOTED = /'([^']*)'/g). So:
readof~/.clickzetta/profiles.toml. That file is TOML:pat = "…",password = "…". Split on whitespace it becomes the tokenspassword,=,"…"—isSensitiveValue("password")is false (no=in the token) andisSensitiveValue("=")is false (eqIdx > 0fails). Double-quoted literals are invisible toredactSql. The PAT leaves the machine verbatim ingen_ai.tool.call.result.write/edit.write's parameters are{ content, filePath }(packages/opencode/src/tool/write.ts:20-22).contentis not inSENSITIVE_KEYS, soredactDeeprecurses into it and the whole file body lands ingen_ai.tool.call.arguments(up to the 8k cap) — including a.envor a credentials file being written.
The same gap covers any whitespace-separated flag spelling: --password hunter2, -p hunter2.
This is on by default rather than opt-in: otel-defaults.ts:46-48 sets OPENCODE_OTEL_RECORD_CONTENT=1 whenever ~/.clickzetta/profiles.toml is absent, and agent does not require a profile — so a fresh install on a user-added provider is inside that default.
The PR description's "nested input keys like password … are replaced" is accurate for redactDeep's object-key pass over state.input; it does not hold for credentials living inside a string value, which is where both cases above sit.
Two fixes, not mutually exclusive:
- Widen the predicate to
key\s*[:=]\s*"?value"?, which covers TOML/YAML/--flag valueas well asKEY=VALUE. - For the tools whose payload is by definition a file body (
read,write,edit), record path plus byte count instead of the body.
Either leaves the bash command line — the case the redactor was actually built for — working as documented.
There was a problem hiding this comment.
Confirmed both leaks and fixed. This is the most important finding on the PR — and it invalidated a claim in my own description, which said nested password keys are replaced without noting that a credential inside a string value was untouched. That is where both of your cases sit.
Took option 1, widened by shape: key = "…" / key: … / KEY=… / --flag …, only the value replaced so the key stays readable, plus PEM envelopes replaced whole. Tests use the real shapes — profiles.toml's pat = "…", a .env body through write, YAML token:, --password, an RSA block — and assert the non-secret parts survive (ws1, PORT=8080, cn-shanghai), because a redactor that eats the useful content just gets turned off. Reverting to the old predicate turns 7 tests red.
Not taken: recording path + byte count for read/write/edit. With the shape-based redaction those bodies keep their analytical value while the credential forms in them are covered, and the owner's call on this PR was to keep content on. The residual is stated in the comment: a secret with neither an assignment shape nor a PEM envelope still gets through.
| // Tool spans are deliberately left open across idle (see the gate in message.part.updated) | ||
| // so a late settlement can still land. A new turn is where the ones that never settled | ||
| // are given up on. | ||
| discardUnsettledTools(sessionID) |
There was a problem hiding this comment.
MEDIUM (confidence: high) — this cleanup is unreachable under OPENCODE_DISABLE_TRACES=prompt, which contradicts the contract this file states twice.
discardUnsettledTools(sessionID) is the only reclamation path for countedTools, settledTools and never-settled toolSpans outside session.deleted and shutdown(). But it sits after line 208:
if (!sessionID || turnSpans.has(sessionID) || !tracingEnabled("prompt")) returnWith prompt in OPENCODE_DISABLE_TRACES the function returns before reaching it, so for the life of the process:
countedToolsandsettledToolsaccumulate one entry per tool call and are never purged;- a tool span that never settles never gets
setStatus({ code: ERROR, message: "tool call never settled" })and is neverend()ed, so it is held and never exported (tool tracing can still be on — the two categories are independent).
session.deleted does not cover this: under cz-cli serve sessions persist and are deleted rarely.
This is the same reasoning the file already applies elsewhere and deliberately keeps outside the gates — liveTurns (lines 96-102, "it must survive OPENCODE_DISABLE_TRACES=prompt, because it also gates metrics and logs") and stepStartMs (lines 125-127, "this file's contract is that gates never affect metrics or logs").
Moving the call into the session.status busy branch fixes it, but it needs its own once-per-turn guard, since busy is re-published mid-run (processor.ts:973) and discarding then would give up on live calls:
if (p.sessionID && !liveTurns.has(p.sessionID)) {
liveTurns.add(p.sessionID)
discardUnsettledTools(p.sessionID)
}
openTurnSpan(p.sessionID ?? "")No test covers the disabled-gate path — disabledTraceCategories is parsed once at module load, which the PR description already notes makes it untestable in-process for the step-duration metric. The same limitation applies here.
There was a problem hiding this comment.
Fixed — discardUnsettledTools moved out of openTurnSpan into the busy branch, so it no longer sits behind the prompt gate. You're right that it was the only reclamation path outside session.deleted, and putting it there contradicted the contract this file states twice.
| } else if (_recordContent && state.output) { | ||
| entry.span.setAttribute("gen_ai.tool.call.result", cap(redactText(String(state.output)))) |
There was a problem hiding this comment.
LOW (confidence: high) — redaction runs on the full untruncated payload, then the result is thrown away.
entry.span.setAttribute("gen_ai.tool.call.result", cap(redactText(String(state.output))))cap() is outermost, so redactText tokenises and rewrites the whole string first: value.split(/(\s+)/) allocates an array with one element per token and per whitespace run over the entire output, then .map, then .join, then redactSql's matchAll scans it again — all synchronously inside handleEvent, on the event-bus path. Line 630 has the same shape for state.input, where write/edit content can be the largest payload of all.
Everything past CONTENT_MAX_CHARS is discarded regardless, so capping first bounds the work to 8k:
cap(redactText(x)) → redactText(cap(x))The only behavioural difference is a credential straddling the 8k boundary getting truncated instead of redacted, which is not a worse outcome. If you want to be certain of that, slice to a slightly larger window before redacting and cap after.
Not a correctness bug — a read output large enough to matter is uncommon and this costs one pass per tool call — but it is free to fix.
There was a problem hiding this comment.
Fixed with the same change as the cycle guard: capTo now runs on each string leaf inside redactDeep, before redactText touches it, so a write body or read result is truncated first and the redactor never scans text the cap is about to discard.
| } catch {} | ||
| } |
There was a problem hiding this comment.
LOW (confidence: high) — the bare catch is the same invisibility mechanism this PR exists to remove.
} catch {}
}The failure being fixed here was undetectable precisely because nothing ever failed loudly: subscribing to a dead event name compiled, loaded and exported cleanly. handleEvent is now ~340 lines reading a large number of optional fields off p and part, and any throw — today's or after the next payload-shape drift — is swallowed with no counter, no log record, no span, and no effect on the process.
otel-span-build.test.ts pins today's shapes, which is the right guard at build time. What is still missing is a runtime signal. Something like incrementing m.errorCounter with source: "otel-handler" in the catch, or emitting one ERROR log record, keeps the "telemetry never breaks the CLI" property while making a future drift visible in the same lake this PR is repairing.
(Noting per the review conventions that this is not a "you should add try/catch" comment — the catch is correct and deliberate. The point is that it should leave a trace.)
There was a problem hiding this comment.
Fixed — the catch increments opencode.error.count{source=otel_handler}. You're right that a silent catch is the same invisibility mechanism as the original bug; the tests pin today's shapes, and this is the signal for the next drift.
| sessionStartMs.delete(p.sessionID) | ||
| // The model must outlive individual turns, so this is the only place it can go. | ||
| sessionModels.delete(p.sessionID) | ||
| sessionParents.delete(p.sessionID) | ||
| discardUnsettledTools(p.sessionID) |
There was a problem hiding this comment.
LOW (confidence: medium) — session.deleted is the only reclamation point for three per-session maps, and it is not the normal end of a session.
sessionModels.delete(p.sessionID)
sessionParents.delete(p.sessionID)endTurn deliberately leaves sessionModels in place (correct — the model has to outlive a turn), so these two plus sessionStartMs are freed only here or in shutdown(). Under cz-cli serve sessions are long-lived and deleted rarely, so a server keeps three entries for every session it has ever observed. Bounded by session count, unbounded in time.
Small, and only sessionStartMs had this shape before the PR — sessionModels and sessionParents are new. If you want a bound without adding a lifecycle event, capping each map (LRU or a size ceiling that drops the oldest) is cheaper than tracking session teardown, since a stale model entry only costs a span name.
There was a problem hiding this comment.
Left as is, with the reasoning on record: sessionModels, sessionParents and sessionStartMs are reclaimed on session.deleted or the session's next turn, bounded by sessions-per-process. Two of the three are new here, so it is a fair flag — but a serve process that accumulates sessions without deleting them is a broader issue than this PR, and pruning them on idle would be wrong (the model has to outlive a turn). Worth a follow-up that puts an explicit bound on per-session state, not a change inside this one.
| export function getCurrentSessionTraceparent(): string | undefined { | ||
| if (!currentSessionSpanContext) return undefined | ||
| return serializeTraceparent({ | ||
| version: "00", | ||
| traceId: currentSessionSpanContext.traceId, | ||
| spanId: currentSessionSpanContext.spanId, | ||
| flags: currentSessionSpanContext.traceFlags === TraceFlags.SAMPLED ? "01" : "00", | ||
| }) | ||
| return currentSessionSpanContext ? serialize(currentSessionSpanContext) : undefined | ||
| } |
There was a problem hiding this comment.
LOW (confidence: high) — dead export left behind by the migration.
export function getCurrentSessionTraceparent(): string | undefined {
return currentSessionSpanContext ? serialize(currentSessionSpanContext) : undefined
}Both production callers moved to getSessionTraceparent in this PR (otel/index.ts:84, traceparent.ts:19). Grepping the tree, the only remaining mention is the prose reference in handlers.ts:231. Nothing calls it.
Worth deleting rather than leaving as a convenience: it is the one remaining public way to read the process-wide slot without naming a session, which is exactly the "one session's telemetry ends up under another's trace" failure the rest of this PR removes. A future caller reaching for the obvious-sounding name would reintroduce it.
Same category, pre-existing rather than introduced here: withSessionOtelContext (line 97) has no callers at all, and getSessionSpanRef (line 79) has none outside otel-span-build.test.ts:315.
There was a problem hiding this comment.
Deleted — both callers moved to getSessionTraceparent in this PR, and leaving it would keep a public way to read the process-wide slot without naming a session, which is the failure the rest of the change removes.
| // `sessionID` is required on this hook, so the header carries this session own trace. | ||
| output.headers.traceparent = currentTraceparent(input.sessionID) |
There was a problem hiding this comment.
Confirm intent (behavioural change, MEDIUM impact, confidence: high) — this header goes from effectively never being sent to being sent on every gateway request.
output.headers.traceparent = currentTraceparent(input.sessionID)Tracing the old path: currentTraceparent() was activeTraceparent() ?? getCurrentSessionTraceparent() ?? process.env.CLICKZETTA_TRACEPARENT. activeTraceparent() reads trace.getSpan(context.active()), and otel/setup.ts registers no ContextManager, so context.active() is always ROOT_CONTEXT and that term is always undefined. getCurrentSessionTraceparent() read the process-wide slot, whose only writer was the session.turn.started branch this PR deletes as dead. So in practice the gateway received a traceparent from this hook only when CLICKZETTA_TRACEPARENT happened to be inherited from a parent process.
After this PR it carries the calling session's live turn span on every request, and CLICKZETTA_TRACEPARENT is likewise now genuinely populated in spawned shell env (otel/index.ts:85).
That is clearly the point of the fix and the plumbing looks right (chat.headers input has a required sessionID, packages/plugin/src/index.ts:257-260). Raising it separately because it is a live outbound change with a downstream consumer that no test here can observe:
- does the gateway treat an unknown incoming
traceparentas a parent to join, or does it start a new root and drop the link? serialize()setsflags=01only when the local span was sampled; if the gateway honours the sampled bit, is deferring to the CLI's decision what you want?
Worth a note in the PR body either way, since it changes what an already-deployed collector will start stitching together.
There was a problem hiding this comment.
Confirmed — and worth being explicit that it is a behavioural change, since the old path could never produce a value: activeTraceparent() is always undefined with no ContextManager registered, and the process-wide slot's only writer was the dead session.turn.started branch. So this header goes from effectively absent to present on every gateway request, carrying the calling session's own turn. That is the intent — it is what makes gateway-side spans joinable to the agent trajectory — and the PR description now says so. The span-id minting question is answered on your other comment.
| function subscribedEvents(): string[] { | ||
| const source = readFileSync(HANDLERS, "utf-8") | ||
| return [...source.matchAll(/case\s+"([a-z0-9._]+)"/g)].map((m) => m[1]!) |
There was a problem hiding this comment.
LOW (confidence: high) — the guard is coupled to the handler's source text in a way that breaks on a legitimate refactor.
return [...source.matchAll(/case\s+"([a-z0-9._]+)"/g)].map((m) => m[1]!)The character class excludes -, and the pattern matches any case "…" in the file, not only event-type cases. Two consequences:
- if the
part.typeif-chain inmessage.part.updatedis ever turned into a nestedswitch,case "tool"matches and is reported as an orphan —every subscribed event is one the schema actually definesand…this path actually deliversboth go red for a refactor that changed no behaviour. (case "step-start"would be silently skipped instead, since-is outside the class.) - a handler that dispatches on anything other than a literal
switch(a lookup table,if (event.type === …)) is covered by nothing, with no failure to say so.
Given this suite's whole purpose is to survive drift in the handler, a structural anchor would be steadier than a regex over the source: export the list from handlers.ts (export const SUBSCRIBED = ["session.created", …] as const), switch on it there, and have the test import it. The rename this suite is built to catch is then caught by the same mechanism, without the test needing to parse TypeScript.
There was a problem hiding this comment.
Fixed the scoping — extraction is limited to the handleEvent body, so other switches in the file no longer register as subscriptions. This bit for real while I was adding serializePart: its case "text" / case "step-start" labels turned both assertions red on a change that touched no subscription. The - exclusion in the character class is now moot for event names, and the type guard added in this round makes the source-text scan a second line of defence rather than the only one.
| // The one `session.next.*` name that does reach a v1-path plugin: prompt.ts:712 | ||
| // publishes it from the v1 prompt path unconditionally. | ||
| "session.next.model.switched", |
There was a problem hiding this comment.
LOW (confidence: high) — the note is inaccurate, and the inaccuracy matters for how much weight a reader puts on this event.
// The one `session.next.*` name that does reach a v1-path plugin: prompt.ts:712
// publishes it from the v1 prompt path unconditionally.
"session.next.model.switched",That publish is not unconditional — it is inside the guard at packages/opencode/src/session/prompt.ts:707-711, which fires only when the resolved provider/model/variant differs from session.model:
if (
current?.model?.providerID !== info.model.providerID ||
current.model.id !== info.model.modelID ||
(current.model.variant === "default" ? undefined : current.model.variant) !== info.model.variant
) {
yield* events.publish(SessionEvent.ModelSwitched, { … })
}So on the common case — a session that never changes model — this event never arrives, and the assistant message.updated is the sole source for sessionModels. That is the design and handlers.ts:455-465 describes it correctly; it is only this comment that overstates the switch event's reliability. Worth fixing so nobody later treats it as a dependable pre-step source for the span name, which is exactly the assumption that produced chat unknown.
There was a problem hiding this comment.
Corrected — the note now says the publish is guarded by the model-difference check at prompt.ts:707-711, and that this event is a supplement rather than the primary source of model identity (message.updated is, and it fires for every assistant message). The inaccuracy mattered exactly as you say: read literally, my version implied model identity was guaranteed by this event.
|
Review summary Nine inline findings, one HIGH. Verdict per section below. I could not run tests, so nothing here claims anything passes. A. Upstream invasiveness — no issues found All eight changed files are under Worth noting positively, since it is the choice the ledger exists to police: this PR reaches the trajectory through the plugin surface already available ( B. Clean fix, with two exceptions The diagnosis holds up against the code. The cost of building lifecycle spans out of a state-replication stream is the four replay guards ( Two things do fall in this category:
Plus, in the same spirit but about coverage rather than shape: the content redactor does not reach TOML or whitespace-separated credentials, nor file bodies — that is the HIGH. C. Regression risk Enumerated, with what covers each:
New dependency edges: |
…ublishes
The OTel plugin subscribed to `session.turn.started`, `session.turn.finished`,
`session.preflight.*`, `v2.step.*` and `v2.tool.*`. Nothing publishes any of them.
cz's own patch on packages/opencode used to (`feat(otel): trace session turn
lifecycle`, 2026-06-08), and the re-baseline onto pure upstream v1.17.11 removed the
publishers while leaving the subscriber behind. Subscribing by string fails silently
in both directions — TypeScript saw `{ type: string }`, the runtime registered the
subscription, the exporter kept working — so every span-creating branch was dead
code and the lake held session lifecycle records with no trajectory under them, and
zero spans.
Spans are rebuilt from `message.part.updated`: `chat {model}` per LLM step with
usage, cost and content, `execute_tool {tool}` per call with arguments and result,
under a per-session `prompt` span. Model identity comes from the assistant
`message.updated` (flat `modelID`/`providerID`/`agent`, as active-model.ts reads
them). Prompt content is restored to what the pre-re-baseline handler exported —
`gen_ai.input.messages`, `gen_ai.system_instructions`, `gen_ai.output.messages`,
same names so historical and new rows line up — but sourced from the
`experimental.chat.messages.transform` and `experimental.chat.system.transform`
hooks instead of an upstream patch, so this stays inside packages/cz-cli.
Turn spans are per session, because `serve` drives several at once and a subagent
runs in its own session: a subagent's turn is parented to its parent's, so one
trajectory is one trace. A turn opens on `session.status` busy and closes on idle.
`opencode.turn.outcome` is preserved because downstream filters on it, and is
derived from whether the turn continued — `session.error` is not terminal on its own
(a ContextOverflowError is compacted away in the same turn), and the assistant
message carrying `error` is published only after the turn closed.
Message parts replicate state rather than announce it, so replays are guarded:
`Session.fork` republishes whole histories and `compaction.prune` rewrites settled
tool parts from earlier turns. Part-derived work requires a live turn, settling is
idempotent, and prune's own `time.compacted` marker identifies its rewrites. The one
thing that legitimately arrives after idle is a tool's settlement — on abort,
`cleanup()` republishes running calls as `Tool execution aborted` after `halt` set
the session idle — so tool spans outlive their turn, and a call that never settles is
closed as an error rather than left looking successful.
Content is redacted by assignment shape (`pat = "…"`, `token: …`, `KEY=…`,
`--password …`) and PEM envelope, using this repo's own `isSensitiveKey`/`redactSql`.
The `KEY=VALUE`-token predicate alone was not enough: profiles.toml would have
exported a PAT verbatim through a `read`, and a `.env` body through a `write`.
32KB for prompt content, 8KB for tool content, truncated at the leaf before
redaction. `OPENCODE_OTEL_RECORD_CONTENT=0` disables all of it.
The `event` hook is typed from the published `Event` union rather than
`{ type: string }`, which is what let a dead event name compile for months; a
subscription outside the union is now a type error, with
`session.next.model.switched` admitted as the one documented exception. Durations
come from the runtime's timestamps where it has them and from a map written before
the trace gate where it does not, so `OPENCODE_DISABLE_TRACES` never removes a
metric or a log field. Log records and each session's outbound traceparent resolve
that session's own turn rather than a process-wide slot owned by whoever opened a
turn first. `shutdown()` — imported by the plugin and never called — runs inside
`flush`, once. The handler's catch increments an error counter instead of failing
silently.
Two test files, for the two halves of this failure mode. otel-event-contract checks
subscribed names against the schema and against the surface this path actually
delivers. otel-span-build drives real v1 payload shapes through `handleEvent` and
asserts on exported spans and log records — the half a name check cannot see, where a
branch subscribes to a live event and reads a field the payload lacks. Every fix is
mutation-checked: reverting it turns its own test red.
Verified: typecheck clean; two consecutive full runs of the cz-cli suite at 1164
pass / 65 skip / 0 fail in a clean worktree off origin/main. Not verified: nothing
has been run against a live session, so the trajectory has not been observed end to
end in a collector.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
fdafe58 to
a2e42cb
Compare
| function redactText(value: string): string { | ||
| let out = value.replace(PEM_BLOCK, "<redacted:private-key>") | ||
| out = out.replace(ASSIGNMENT, (whole, key: string, sep: string) => | ||
| isSensitiveKey(key) ? `${key}${sep}<redacted>` : whole, | ||
| ) | ||
| out = out.replace(FLAG_VALUE, (whole, flag: string, sep: string) => | ||
| isSensitiveKey(flag.replace(/^-+/, "")) ? `${flag}${sep}<redacted>` : whole, | ||
| ) | ||
| return redactSql(out) | ||
| } |
There was a problem hiding this comment.
HIGH (confidence: high) — the redactor's key vocabulary is an exact-match set, so the credential spellings that dominate env files and shell exports are not recognised.
out = out.replace(ASSIGNMENT, (whole, key: string, sep: string) =>
isSensitiveKey(key) ? `${key}${sep}<redacted>` : whole,
)isSensitiveKey is SENSITIVE_KEYS.has(key.toLowerCase()) (telemetry.ts:54), and the set holds exactly credential, password, pat, token, secret, api-key, apikey, access-token, auth, authorization, cookie, x-api-key, x-auth-token, login, jdbc. That set was built for argv flag names, where the whole token is the key. Reused against arbitrary file and command text, everything with a prefix or an underscore falls through:
API_KEY=…—api_keyis notapi-keyorapikeyOPENAI_API_KEY=…,ANTHROPIC_API_KEY=…GITHUB_TOKEN=…,GH_TOKEN=…AWS_SECRET_ACCESS_KEY=…,AWS_ACCESS_KEY_ID=…DB_PASSWORD=…,db_pass=…
redactDeep's object-key branch (line 83) has the same gap for nested input keys.
This is not hypothetical for this PR: read, grep and write on a .env file are routine agent actions, state.output for read is the file body, content recording is opt-out, and otel-defaults.ts:46-47 sets OPENCODE_OTEL_RECORD_CONTENT=1 outright when ~/.clickzetta/profiles.toml is absent. The PR body's "inline TOKEN=… [is] replaced" is true only for that exact spelling. otel-span-build.test.ts:837 feeds API_KEY=k-999 through and asserts only that abc123xyz (the TOKEN= value) is gone — the gap is already in the test fixture.
Root cause: reusing an exact-match argv predicate as a free-text key predicate. Smaller correct change: keep isSensitiveKey as-is for argv, and match by substring here against the same vocabulary logger.ts:11 already uses for RE_SENSITIVE_COL (password|passwd|secret|api_key|apikey|token|credential|cookie|auth), normalising -/_ away — e.g. a local looksSensitiveKey(key) used by redactText and redactDeep instead of isSensitiveKey.
| * | ||
| * It is a redactor, not a secret scanner. A credential with no assignment shape and no PEM | ||
| * envelope — a bare positional, `-H "Authorization: Bearer …"` (whose key is `Authorization` | ||
| * so it IS caught, but `Bearer x` alone is not) — still gets through. Content recording is | ||
| * off entirely under OPENCODE_OTEL_RECORD_CONTENT=0. | ||
| */ | ||
| const PEM_BLOCK = /-----BEGIN [A-Z ]*PRIVATE KEY-----[\s\S]*?-----END [A-Z ]*PRIVATE KEY-----/g | ||
| //: `key = "value"` / `key: value` / `KEY=value`, as config files and env files write them. | ||
| const ASSIGNMENT = /([A-Za-z_][\w.-]{1,40})(\s*[:=]\s*)("[^"]*"|'[^']*'|`[^`]*`|[^\s,;)}\]]+)/g |
There was a problem hiding this comment.
HIGH (confidence: high) — ASSIGNMENT's value group stops at whitespace, so Authorization: Bearer <token> redacts the word Bearer and exports the token. The docstring directly above claims the opposite.
* envelope — a bare positional, `-H "Authorization: Bearer …"` (whose key is `Authorization`
* so it IS caught, but `Bearer x` alone is not) — still gets through.
const ASSIGNMENT = /([A-Za-z_][\w.-]{1,40})(\s*[:=]\s*)("[^"]*"|'[^']*'|`[^`]*`|[^\s,;)}\]]+)/gTrace curl -H "Authorization: Bearer sk-live-abc123" through redactText:
PEM_BLOCK— no match.ASSIGNMENT— keyAuthorization, sep": "… actually sep:, then the value alternatives:"[^"]*"fails (next char isB),'…'and`…`fail, so[^\s,;)}\]]+matchesBeareronly. Result:Authorization: <redacted> sk-live-abc123".FLAG_VALUE—-H→isSensitiveKey("H")false, no match.redactSql—RE_QUOTEDis/'([^']*)'/g(logger.ts:8), single quotes only, so the double-quoted header is untouched.
The bearer token ships in gen_ai.tool.call.arguments. The same shape leaks for any space-containing credential value: password: hunter 2, Proxy-Authorization: Basic …, api-key: sk abc.
Note the nested-object path is fine — redactDeep catches { headers: { Authorization: "Bearer …" } } by key. It is specifically the string case, i.e. a bash command line, which is the most likely place for one.
Root cause: a single value pattern is being asked to cover both "config assignment" (value ends at whitespace) and "header-style assignment" (value runs to end of line/quote). Smaller correct change: when the separator contains :, consume to end of line or closing quote rather than to the first space — e.g. add a ([A-Za-z_][\w.-]{1,40})(\s*:\s*)([^\n"']*) alternative applied only to sensitive keys. Either way please fix the docstring at lines 45-48, which currently tells a reader this case is handled.
| test("a .env body being written does not export its secrets", () => { | ||
| const { args } = toolCall("write", { filePath: ".env", content: "TOKEN=abc123xyz\nAPI_KEY=k-999\nPORT=8080" }, "ok") | ||
| expect(args).not.toContain("abc123xyz") | ||
| expect(args).toContain("PORT=8080") |
There was a problem hiding this comment.
MEDIUM (confidence: high) — this fixture contains the un-redacted case and does not assert on it, so the suite reads as green over the gap.
const { args } = toolCall("write", { filePath: ".env", content: "TOKEN=abc123xyz\nAPI_KEY=k-999\nPORT=8080" }, "ok")
expect(args).not.toContain("abc123xyz")
expect(args).toContain("PORT=8080")abc123xyz is asserted gone (TOKEN is in SENSITIVE_KEYS). k-999 is neither asserted gone nor asserted present — and it survives, because api_key is not in the set (api-key and apikey are). The test name says "a .env body being written does not export its secrets", which is stronger than what it checks.
Adding expect(args).not.toContain("k-999") turns this red and pins the redaction fix. If the author decides API_KEY is deliberately out of scope, the inverse assertion plus a comment would at least make the exposure a recorded decision rather than a silent one.
| function endTurn(sessionID: string) { | ||
| if (!sessionID) return | ||
| liveTurns.delete(sessionID) | ||
| for (const [key, entry] of stepSpans) { | ||
| if (entry.sessionID !== sessionID) continue | ||
| entry.span.end() | ||
| clearCurrentLlmSpan(entry.span) | ||
| stepSpans.delete(key) | ||
| } | ||
| // Iterated separately from stepSpans on purpose: with OPENCODE_DISABLE_TRACES=llm there | ||
| // are no step spans, so a loop keyed off them would never reclaim these. | ||
| for (const [key, started] of stepStartMs) if (started.sessionID === sessionID) stepStartMs.delete(key) | ||
| const failure = pendingErrors.get(sessionID) | ||
| const span = turnSpans.get(sessionID) | ||
| if (span) { | ||
| if (failure) span.setStatus({ code: SpanStatusCode.ERROR, message: failure }) | ||
| span.setAttribute("opencode.turn.outcome", failure ? "error" : "completed") | ||
| span.end() | ||
| turnSpans.delete(sessionID) | ||
| } | ||
| pendingErrors.delete(sessionID) | ||
| turnEnriched.delete(sessionID) | ||
| setSessionSpanContext(sessionID, undefined) |
There was a problem hiding this comment.
MEDIUM (confidence: high) — endTurn reclaims stepSpans, stepStartMs, pendingErrors and turnEnriched, but not the three maps that hold prompt/completion content. Under serve those grow for the life of the process.
messageOutput (line 190) is deleted in exactly one place, the step-finish branch at line 720. Any turn that ends without a step-finish for that message — abort, halt on a terminal error, a stream that dies mid-step — leaves its entry behind holding every text/reasoning/tool part accumulated for that message. endTurn closes the step span for that case (lines 377-382) but not its accumulator, so the two get out of sync.
sessionInput / sessionSystem (lines 184-185) are cleared only in the session.deleted branch (lines 502-503). Sessions are rarely deleted, so each live session retains up to 32 KB of serialized conversation plus 32 KB of system prompt indefinitely — serve with a few dozen sessions is megabytes of prompt text held after the turns that produced it are closed.
There is a correctness edge on the same cause: because sessionInput is never invalidated at turn end, a step-start whose experimental.chat.messages.transform did not fire will read the previous turn's value and label the new chat span with a stale gen_ai.input.messages. agent.ts:381 triggers experimental.chat.system.transform with no sessionID (so recordSystemInstructions no-ops there), which shows the hooks are not uniformly session-scoped; a step reached by a path that skips prompt.ts:1357 would inherit stale content rather than omit the attribute.
Smaller correct change: clear all three in endTurn alongside the existing reclamation — messageOutput for the session's messages, and sessionInput/sessionSystem by sessionID. That fixes the retention and makes staleness impossible in one edit. (sessionModels genuinely must outlive a turn, per the comment at line 499 — these three do not.)
No test covers this; otel-span-build.test.ts calls shutdown() in beforeEach, which clears every map, so the leak is invisible to the suite.
| // Outside openTurnSpan on purpose: that function returns early under | ||
| // OPENCODE_DISABLE_TRACES=prompt, and this is the only reclamation path for the | ||
| // tool maps outside `session.deleted`. Tool spans are deliberately left open | ||
| // across idle (see the gate in message.part.updated) so a late settlement can | ||
| // land; a new turn is where the ones that never settled are given up on. | ||
| discardUnsettledTools(p.sessionID ?? "") | ||
| openTurnSpan(p.sessionID ?? "") |
There was a problem hiding this comment.
MEDIUM (confidence: medium — please confirm the intent) — discardUnsettledTools runs on every busy, while openTurnSpan on the next line is guarded against exactly that. busy is re-published mid-turn.
discardUnsettledTools(p.sessionID ?? "")
openTurnSpan(p.sessionID ?? "")openTurnSpan's early return is turnSpans.has(sessionID), and the docstring at lines 251-252 says so explicitly ("busy is re-published mid-run (processor.ts:973), so the guard below is what makes it idempotent"). discardUnsettledTools has no equivalent guard, so each re-publish closes every still-open tool span for that session with SpanStatusCode.ERROR / "tool call never settled" and drops its countedTools / settledTools entries.
I confirmed two mid-turn busy publishers in this baseline:
prompt.ts:1192— top ofwhile (true), i.e. once per loop iteration. This one looks safe:processor.ts:1027Effect.ensuring(cleanup())is the outermost stage, andcleanupawaits each in-flight call for 250 ms and then force-settles the remainder as"Tool execution aborted"(processor.ts:879-911). So tool parts should be settled before the loop comes back around.processor.ts:973— insideprocess(), beforellm.stream(...).Effect.retry(line 994) sits insideEffect.ensuring(cleanup())in the pipe chain, so a retry re-enters the generator and re-publishesbusywithoutcleanuphaving run. If the stream failed after a tool call had started, that call's part is stillrunning.
On that path the sequence is: span closed as "tool call never settled", dedupe keys dropped, then the call settles for real → countedTools no longer has the key → m.toolCallCounter.add(1) a second time for the same callID, settledTools no longer has it → a second m.toolCallDuration sample and a second opencode.tool.finished log record. And the exported span says the call failed when it succeeded. The time.compacted guard at line 772 does not cover this, since a live re-settle carries no marker.
The two tests around this ("is given up on if a later turn starts without it ever settling", line 552, and "a call given up on … is not exported as a successful one", line 668) both drive idle → busy, which is the intended case; neither drives a second busy without an intervening idle.
If the retry path is genuinely unreachable with an open tool call, a sentence saying so next to this line would keep the asymmetry with openTurnSpan from reading as an oversight. Otherwise the smaller correct change is to gate the discard on the same condition openTurnSpan uses — only reclaim when there is no turn span for the session yet, i.e. when this busy really is a new turn.
| let _logger: Logger | undefined | ||
| let promptSpan: Span | undefined | ||
| let preflightSpan: Span | undefined | ||
| let _recordContent = true |
There was a problem hiding this comment.
MEDIUM (confidence: high) — _recordContent defaults to true and initHandlers is the only thing that can change it, but initHandlers is called only when an SDK exists (otel/index.ts:58-59). With no OPENCODE_OTLP_ENDPOINT, the plugin still returns all its hooks (the return {...} at index.ts:82 is outside the if (sdk)), so every content path in this file runs at full cost and nothing is ever exported.
let _logger: Logger | undefined
let _recordContent = trueConcretely, with telemetry off:
recordInputMessagesrunspromptAttron the whole conversation per LLM step —redactDeepwalk plus two global regex passes plusredactSqlplusJSON.stringify, up to 32 KB — and stores the result insessionInput.redactDeep/redactTextrun onstate.inputfor every tool state transition and onstate.outputon every settle, up to 8 KB each. Attribute values are computed beforesetAttributeis called, so a non-recording span does not save the work.messageOutput,sessionInputandsessionSystemaccumulate content that nothing will read.
Before this PR the content branches were the dead v2.* cases, so this cost is new on the no-exporter path. Users who have no collector configured now pay per-step redaction for nothing.
Smaller correct change: initialise let _recordContent = false so capture is opt-in via initHandlers, which is already the single entry point that knows whether an exporter exists. otel-span-build.test.ts calls initHandlers(logger, true) in beforeEach, so the suite is unaffected. (Returning the hooks only when sdk is set would also work but changes more — handleEvent's metric side effects would go with them.)
| export function shutdown() { | ||
| endPromptSpan() | ||
| endPreflightSpan() | ||
| for (const sessionID of [...turnSpans.keys()]) endTurn(sessionID) | ||
| clearSessionSpanContexts() | ||
| setCurrentLlmSpan(undefined) | ||
| for (const span of stepSpans.values()) span.end() | ||
| for (const span of toolSpans.values()) span.end() | ||
| for (const entry of stepSpans.values()) entry.span.end() | ||
| for (const entry of toolSpans.values()) entry.span.end() |
There was a problem hiding this comment.
LOW (confidence: high) — at shutdown, unsettled tool spans are ended with no status, which is the exact failure mode discardUnsettledTools was written to avoid.
for (const entry of toolSpans.values()) entry.span.end()discardUnsettledTools (lines 356-357) sets ERROR / "tool call never settled" first, with the rationale that "An unset status reads as OK in every backend, which would make a call that never settled indistinguishable from one that completed with empty output." That reasoning applies verbatim here, and this is the more likely path — otel/index.ts:77-79 wires flush to beforeExit/SIGTERM/SIGINT, so Ctrl-C during a long bash call lands exactly here. The result is a execute_tool bash span exported as successful with no result.
Same for the step spans on the line above, though a step has no equivalent "never settled" meaning.
Smaller correct change: reuse the existing helper — iterate the sessions in toolSpans and call discardUnsettledTools for each, instead of a bare .end() loop. That keeps one definition of what an abandoned call looks like.
| } else if (_recordContent && state.output) { | ||
| entry.span.setAttribute( | ||
| "gen_ai.tool.call.result", | ||
| cap(redactText(capTo(String(state.output), CONTENT_MAX_CHARS))), | ||
| ) |
There was a problem hiding this comment.
LOW (confidence: high) — capping twice makes the [truncated N chars] count wrong by orders of magnitude.
entry.span.setAttribute(
"gen_ai.tool.call.result",
cap(redactText(capTo(String(state.output), CONTENT_MAX_CHARS))),
)capTo appends its own suffix, so the inner call returns 8000 + ~24 chars. The outer cap then sees a string over the limit, slices back to 8000 — cutting off the inner suffix — and appends a fresh one computed from what it just removed. For a 100 KB read output the attribute ends in [truncated 24 chars] instead of [truncated 92000 chars], so a reader cannot tell how much was dropped.
promptAttr (line 99) has the same double-cap shape: redactDeep caps each string leaf at PROMPT_MAX_CHARS, then the outer capTo caps the serialized whole at the same limit, so the reported count reflects only the JSON overhead past the boundary rather than the conversation that was dropped.
The tool-content test at line 442 asserts value).toContain("[truncated") and length < 9000, both of which pass with the wrong number.
Smaller correct change: drop the inner capTo here and let cap do it once (redaction can only shrink or modestly grow the text, and the outer cap bounds the result either way). For promptAttr, either keep the leaf cap and drop the outer one, or make capTo idempotent by not re-truncating a string that already ends in its own marker.
| } from "./context" | ||
| import * as m from "./metrics" | ||
| import type { Event } from "@opencode-ai/sdk" | ||
| import { isSensitiveKey, isSensitiveValue } from "../../telemetry.js" |
There was a problem hiding this comment.
LOW (confidence: high) — isSensitiveValue is imported but never used, and telemetry.ts:49 was widened from function to export function in this PR only to supply it.
import { isSensitiveKey, isSensitiveValue } from "../../telemetry.js"rg isSensitiveValue packages/cz-cli finds it in telemetry.ts (definition plus three internal call sites), in this import, and in the docstring at line 39 that explains why it wasn't used — nowhere in this file's code. The docstring's reasoning is sound; the import and the export widening are what's left over from the revision that did call it.
Either drop both (revert telemetry.ts:49 to module-private, drop the import) or, if the export is meant to stay as public surface, that is worth its own line in the PR description since it enlarges telemetry.ts's API for no in-tree consumer.
| }, | ||
| }, turnContext(sessionID)) | ||
| stepSpans.set(key, { span, sessionID }) | ||
| setCurrentLlmSpan(span) |
There was a problem hiding this comment.
LOW (confidence: high) — this line arms a capture path that writes unredacted auth headers, and that path currently has no callers.
setCurrentLlmSpan(span)Before this PR setCurrentLlmSpan was reached only from the dead v2.step.started branch, so currentLlmSpan in otel/context.ts was permanently undefined and recordRawProviderRequest's guard at context.ts:66 always returned early. This line makes it live. recordRawProviderRequest then attaches:
"clickzetta.llm.raw_request.headers": JSON.stringify(event.headers),
"clickzetta.llm.raw_request.body": event.body ?? "",
"clickzetta.llm.raw_request.body_base64": event.bodyBase64 ?? "",No redaction, no cap — headers would carry Authorization, and body the entire prompt. rg recordRawProviderRequest packages finds only the definition, so nothing calls it today and this is not a live exposure in this PR. But the guard that had been holding it shut is now open, and whoever adds the first caller will not know that.
Worth either (a) routing those two attributes through redactDeep/cap now, while the file is being touched, or (b) deleting recordRawProviderRequest and setRawRequestCaptureEnabled as dead code and letting a future caller add them back deliberately. Also note currentLlmSpan is a single module-level slot while stepSpans is now per-session, so under serve two concurrent sessions' steps overwrite each other here — a third reason to prefer (b).
| // `compaction.prune` rewrites completed tool parts from turns that already ended | ||
| // (it skips the two most recent, compaction.ts:264) and stamps `time.compacted` | ||
| // on exactly what it rewrites (compaction.ts:284). Those callIDs left the dedupe | ||
| // sets when their turn ended, so the marker is what identifies them; a live | ||
| // settle never carries it. | ||
| if (state.time?.compacted != null) break |
There was a problem hiding this comment.
LOW (confidence: medium) — the replay guard rides on a field whose meaning is incidental to it.
if (state.time?.compacted != null) breaktime.compacted is compaction bookkeeping that happens to be stamped by the same write that republishes the part. Nothing in compaction.ts promises to keep stamping it, and nothing links the two — so if prune ever writes settled tool parts back without that field (or stamps it on a part it does not republish), this handler silently starts double-counting again, which is the same class of silent drift the event-contract test was added to catch.
otel-span-build.test.ts:365 covers the current behavior, but it asserts against a hand-written payload, so it verifies this handler's reading of the marker rather than that compaction.prune still produces it. A rename or removal upstream leaves both green.
Not asking for a redesign — the alternative (keeping settled callIDs in settledTools across turns rather than purging them in discardUnsettledTools) trades a bounded map for the marker, which may well be the worse deal. But this is worth an entry in the re-baseline notes, or a comment naming compaction.ts:284 as the coupling to re-verify, since the ledger's re-baseline checklist is the only thing that would otherwise catch it.
| const end = source.indexOf("} catch {}", start) | ||
| expect(end).toBeGreaterThan(start) | ||
| const body = source.slice(start, end) | ||
| return [...body.matchAll(/case\s+"([a-z0-9._]+)"/g)].map((m) => m[1]!) | ||
| } | ||
|
|
||
| /** | ||
| * Event names the schema defines. The file list is fixed on purpose: it is the set of | ||
| * surfaces a plugin can receive, so a subscription to something outside it is the very | ||
| * mistake this guards. A legitimate new surface means adding the file here. | ||
| */ | ||
| function publishedEvents(): Set<string> { | ||
| const names = new Set<string>() | ||
| for (const file of SCHEMAS) { | ||
| const source = readFileSync(file, "utf-8") | ||
| for (const m of source.matchAll(/type:\s*"([a-z0-9._]+)"/g)) names.add(m[1]!) | ||
| } | ||
| return names |
There was a problem hiding this comment.
LOW (confidence: high) — two fragilities in the source-scraping approach, worth noting since this suite is the thing standing between a rename and another silently drained trace.
const end = source.indexOf("} catch {}", start)
expect(end).toBeGreaterThan(start)The only } catch {} in handleEvent is now the inner empty catch at handlers.ts:865, inside the outer catch that increments errorCounter. That works today and fails loudly if it disappears, so it is not a false-green — but it does mean the test's boundary depends on a nested error-handling detail rather than on the switch it wants to read. Matching switch (event.type) { to its close, or just scanning to end-of-file (the only other switch is in serializePart, which is defined before handleEvent), would be less coupled.
for (const m of source.matchAll(/type:\s*"([a-z0-9._]+)"/g)) names.add(m[1]!)This collects every type: "…" literal in the three schema files, not just event names — session-v1.ts defines part types the same way, so "text", "tool", "step-start" and friends all land in published. A subscription to case "tool": would pass the "every subscribed event is one the schema actually defines" test. The DELIVERED check at line 99 catches it in practice, which is why this is LOW rather than a real hole — but it means test 2 is weaker than its name, and DELIVERED (a hand-maintained list) is doing the actual work.
|
Review summary A. Upstream invasiveness — no issues found All 8 changed files are under One suggestion rather than a finding: B. Clean fix vs. hole drilled around the problem Largely the clean fix, and it is the right diagnosis: the branches were subscribed to names nothing publishes, and rebuilding from Findings in this category:
C. Regression risk Findings raised inline: The behavioral changes I could not attach to a specific line, with their coverage:
No tests were deleted, skipped, or loosened. I could not run anything, so nothing above is a claim that the suite passes. |
2 participants
What was broken
The OTel plugin subscribed to six events that nothing publishes:
session.turn.started/session.turn.finishedsession.preflight.started/session.preflight.finishedv2.step.started/v2.step.endedv2.tool.called/v2.tool.endedpackages/schema/src/session-event.tsdefines the durable bus assession.next.*, and those are published from the core session runner (packages/core/src/session/runner/publish-llm-event.ts). cz runs the v1 session (packages/opencode/src/session/session.ts), so a plugin receives its message surface —message.part.updated,message.part.delta,message.updated,session.status,session.created,session.idle. Renaming the subscriptions would not have helped.Subscribing by string fails silently in both directions: TypeScript sees a
string, the runtime registers the subscription, the exporter keeps working. So every span-creating branch was dead code while session lifecycle records kept landing in the lake. Externally it looked like a sampling or exporter problem — sessions present,otel_tracesempty ofchatandexecute_toolspans.origin/mainhad it too; this is not re-baseline drift.What this does
Spans are rebuilt from
message.part.updated, keeping the names and attributes the dead branches would have written:step-start/step-finish→chat {model}, usage + cost on closetool→execute_tool {tool}, arguments + resultModel identity comes from the two events that carry it: assistant
message.updated(flatmodelID/providerID/agent— the same fieldsactive-model.tsalready reads) andsession.next.model.switchedon an explicit switch.The turn span is anchored on
session.statusbusy — the first signal of a turn — rather than on the first LLM step, becauseprompt.tspublishessession.errorfrom model resolution and permission paths that run before any part exists.opencode.turn.outcomecannot be taken from error events alone — aContextOverflowErroris published and then compacted away within the same turn — nor from the assistant message, which is published by a cleanup finalizer only after the turn has closed. It is derived from whether the turn continued: an error is pending until the next step starts, and a pending error at idle is a failed turn.Part events are a replayable stream.
Session.forkrepublishes every historical part andcompaction.prunerewrites settled tool parts, so all part-derived spans, metrics and logs are gated on the session having a live turn (tracked fromsession.status, separately from the spans so trace gates don't affect metrics), and settling a tool is idempotent for the prune case that can land before idle.Turn state is per session.
servedrives several sessions at once and a subagent nests a second inside the first, so one module-level span attributed one session's steps to another and closed on whichever went idle first. Steps and tools now parent to their own session's turn; a turn that ends closes what it left open; and the process-wide session slot inotel/context.tsis cleared by its owner rather than handed to another open turn — underservethat next turn belongs to an unrelated session, so a handoff would replace a missing parent with a wrong one. A subagent's turn is parented to its parent session's turn (session.createdcarriesparentID), so a subagent's trajectory stays in the same trace instead of becoming a detached one.Smaller corrections along the way:
llmtrace gate: disabling LLM traces must not delete the turn as well.time.start/time.endinstead of being measured around the event.Tests
Two files, for the two halves of this failure mode.
otel-event-contract.test.tschecks subscribed event names against the schema — a rename fails here instead of draining the trace.otel-span-build.test.tsdrives real v1 payload shapes throughhandleEventand asserts on the exported spans. This is the half a name check cannot see: a branch can subscribe to a live event and read a field the payload lacks. That happened here — an earlier revision read a nestedinfo.modeloff the assistant message and named every spanchat unknown, silently.Both were mutation-checked: reverting each fix turns specific tests red.
Verification
bun run typecheckcleanbun testinpackages/cz-cli: 1164 pass, 65 skip, 0 fail — run in a clean worktree offorigin/mainwith its ownnode_modulesContent exposure this makes live
Worth stating plainly, because the branches that wrote these were dead: tool arguments and results now really are exported.
gen_ai.tool.call.argumentscarriesstate.input— forbashthat is the whole command line, which routinely contains tokens and connection strings — andgen_ai.tool.call.resultcarriesstate.output, which forread/grep/webfetchis file or page content.Content is redacted by assignment shape, using this repo's own
isSensitiveKey/redactSql:pat = "…"(TOML),token: …(YAML),KEY=…(env),--password …(flags), plus PEM envelopes replaced whole. Only the value is replaced, so keys stay readable and non-secret content survives. TheKEY=VALUE-token predicate alone was NOT enough —readof profiles.toml would have exported a PAT verbatim andwriteof a.envits body — which is why the shape is matched rather than the token. It is still a redactor, not a secret scanner: a credential with neither an assignment shape nor a PEM envelope gets through.Prompt content is capped at 32KB (the pre-re-baseline limit) and tool content at 8KB, truncated at each string leaf before redaction runs.
Note that
otel-defaults.tsforcesOPENCODE_OTEL_RECORD_CONTENT=1when~/.clickzetta/profiles.tomlis absent, andagentdoes not require a profile, so a fresh install running on a user-added LLM provider is inside that default.This stays behind the existing
OPENCODE_OTEL_RECORD_CONTENTswitch, which is opt-out (on unless set to0), and the default is deliberately unchanged in this PR: the trajectory is the point of the fix, and turning it off silently would deliver spans without the content that makes them useful. Both attributes are capped at 8k chars, since a whole-filereadotherwise becomes one oversized attribute and collectors drop or truncate the span rather than the text.If the exposure is not acceptable for a given deployment,
OPENCODE_OTEL_RECORD_CONTENT=0turns both off and leaves spans, timings, token counts, tool names and errors intact.Restored to pre-re-baseline parity
git log -Sshows these events were never upstream's: cz patchedpackages/opencodeto publish them (feat(otel): trace session turn lifecycle, 2026-06-08), and the re-baseline onto pure upstream v1.17.11 dropped the publishers while keeping this subscriber. The pre-re-baseline handler also exportedgen_ai.input.messages,gen_ai.system_instructionsandgen_ai.output.messages, so those are restored here under the same attribute names — sourced from theexperimental.chat.messages.transformandexperimental.chat.system.transformhooks rather than an upstream patch, which keeps the change insidepackages/cz-cli. Two things are deliberately not restored:opencode.turn.parts(no equivalent source on the message surface) and thepreflightspan (its event was cz's own and no v1 boundary corresponds to it).Outbound
traceparenton gateway requests andCLICKZETTA_TRACEPARENTfor spawned shells go from effectively absent to present, since the process-wide slot's only writer was one of the dead branches. That is the point — it makes gateway-side spans joinable to the trajectory.Not addressed here
chatspans still carry nogen_ai.input.messages,gen_ai.output.messagesorgen_ai.system_instructions, so prompts and completions themselves are still absent from traces. The v1 text parts could supply them; that is a separate change with the same privacy surface as above.The step-duration metric's independence from
OPENCODE_DISABLE_TRACESis not covered by a test: the variable is parsed once at module load, so a same-process test cannot flip it, and a test-only seam in the handler was not worth adding.