sdk/summary: expose turn outcome (stop_reason) on TurnRecord + summary breakdown (#437)#445
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (18)
📝 WalkthroughWalkthroughThis PR introduces structured stop-reason tracking across the ledger system: a new ChangesStop reason: definition, storage, retrieval, and presentation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7ffe553e36
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "end-turn" => Some(Self::EndTurn), | ||
| "max-tokens" | "length" => Some(Self::MaxTokens), | ||
| "pause-turn" => Some(Self::PauseTurn), | ||
| "stop-sequence" | "stop" => Some(Self::StopSequence), |
There was a problem hiding this comment.
stop finish reason as natural completion
The from_wire mapping currently collapses "stop" into StopSequence, but OpenAI-compatible providers commonly use "stop" for normal completion (not just custom stop-sequence hits). In sessions parsed through opencode, this will misclassify successful assistant endings as stop-sequence terminations, which then skews burn summary outcome buckets and any downstream logic that interprets non-EndTurn reasons as user-ended. This alias should map to EndTurn (or remain unknown) instead of StopSequence.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 5dab4df (now 5dab4df after rebase). "stop" now maps to StopReason::EndTurn — OpenAI / AI-SDK (and therefore opencode wrapping an OpenAI-shaped provider) emit a bare "stop" for ordinary end-of-turn completions, so collapsing it into StopSequence would have skewed the burn summary outcome buckets. Anthropic's actual stop-sequence outcome (stop_sequence / stop-sequence) still resolves to StopSequence.
The existing stop_reason_from_wire_normalizes_underscored_and_legacy_variants test now asserts both the new "stop" -> EndTurn mapping and that the stop_sequence / stop-sequence aliases still resolve to StopSequence. Branch HEAD is now 10d3cd7 (also includes a sibling fix to the migration pattern + a rebase onto current main).
Generated by Claude Code
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 19 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…`burn summary` (#437) BREAKING: `TurnRecord.stop_reason` switches from `Option<String>` to `Option<StopReason>`. Anthropic / opencode finish-reason strings are parsed into the enum at the reader boundary; unknown values fall back to `StopReason::Silent` so pre-3.0 ledgers replay cleanly. Schema bumps to v2 with a new denormalized `turns.stop_reason TEXT` column populated on insert; existing v1 ledgers are migrated in place via `ALTER TABLE` on `Ledger::open`. `burn summary` adds a `Turn outcomes: …` line and a `stopReasons` block in JSON output. Codex turns continue to carry `None` (the rollout schema doesn't include a stop reason field). https://claude.ai/code/session_01QEpNZbWEYNwxzqQjTN5LCY
OpenAI / AI-SDK convention emits a bare `"stop"` for ordinary end-of-turn completions, which is what opencode forwards when it wraps an OpenAI-shaped provider. The previous mapping collapsed `"stop"` into `StopSequence` alongside Anthropic's `stop_sequence`, which would misclassify successful assistant endings as stop-sequence terminations and skew the outcome buckets in `burn summary`. `stop_sequence` / `stop-sequence` still resolve to `StopSequence` (the actual Anthropic semantics). Addresses chatgpt-codex-connector review on #445. https://claude.ai/code/session_01QEpNZbWEYNwxzqQjTN5LCY
The v1 -> v2 `ALTER TABLE turns ADD COLUMN stop_reason` previously used a `column_exists` pre-check, which is race-prone under concurrent ledger opens: two processes can both observe the column missing, both issue the `ALTER`, and the second loses. Switch to try-then-swallow on the `duplicate column name` SQLite error so the migration is genuinely idempotent under contention. Deliberately does not catch `SqliteFailure(_, None)` — that shape is too broad and would mask real schema breakage. Keeps the pattern aligned with the matching migration in #444 so the two PRs don't diverge when both land. `column_exists` had no other callers, so the helper is dropped. Addresses cubic-dev-ai review on #445. https://claude.ai/code/session_01QEpNZbWEYNwxzqQjTN5LCY
willwashburn
force-pushed
the
claude/burn-3.0-437-stop-reason
branch
from
7ffe553 to
10d3cd7
Compare
2 participants
Closes #437.
Part of the burn 3.0 coordinated bump (see #430–#440 batch — do not merge alone).
Summary
StopReasonenum inreader/types.rs:EndTurn | MaxTokens | PauseTurn | StopSequence | ToolUse | Refusal | Silent. Kebab-case serde wire form.StopReason::from_wireaccepts Anthropic snake_case (end_turn,max_tokens, …) and opencode'stool-calls(maps toToolUse).TurnRecord.stop_reasonchanged fromOption<String>toOption<StopReason>with a lenient deserializer — unknown strings land inStopReason::Silentso pre-3.0 ledger replays don't error.TurnRecordboundary inclaude.rs,opencode.rs,opencode_stream.rs. Codex staysNone(no equivalent field); documented.stop_reason TEXTcolumn onturns+idx_turns_stop_reasonpartial index. IdempotentALTER TABLEmigration onLedger::open.StopReasonCountsstruct added to bothSummaryandSummaryGroupedReport. CLI emits:stopReasonsblock inburn summary --json.Why
Silentvsnoneare distinct bucketsSilent— row exists, carries an unrecognized stop_reason string (forward compat for future Anthropic values).none— nostop_reasonfield on the row at all (Codex, sidechain agents, malformed rows).Keeping them separate lets future regressions ("we stopped parsing X") stand out instead of vanishing into a combined bucket.
Merge note
This PR bumps schema v1→v2 by adding
turns.stop_reason. #436 (tool-output bytes) ALSO bumps v1→v2 fortool_result_events.output_bytes+output_truncated. Whichever lands second needs to become v3 with a chainedALTER TABLE. Both migrations are idempotent — mechanical reconcile.Test plan
stop_reason_serializes_kebab_casestop_reason_from_wire_normalizes_underscored_and_legacy_variantsturn_record_stop_reason_deserializes_legacy_strings_into_enum(legacymax_tokensround-trip)turn_record_stop_reason_unknown_string_falls_back_to_silentsummary_report_aggregates_stop_reasons_per_outcome(fixture withmax_tokensturn surfaces in summary)summary_legacy_surface_includes_stop_reason_counts_with_none_for_missing_fieldlegacy_v1_ledger_migrates_to_v2_on_open_and_adds_stop_reason_columnsummary,summary-json,state-status*cargo test --workspace— 794 tests, 0 failedBURN_GOLDEN=1 cargo test --test golden— passOut of scope
stop_reasonin hotspots / overhead (separate concerns).#[non_exhaustive]— not added; this is part of the 3.0 major bump.Generated by Claude Code