fix(bridge): classify the terminal stop reason instead of testing it for truthiness - #4885
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 10 included reviews per hour; 2 remain after this review. 📝 WalkthroughWalkthroughThe bridge now classifies terminal stop reasons consistently. Non-truncated reasons produce clean completions and ChangesStop-reason phase normalization
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix · Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The terminal-phase normalization is covered across both response paths, with no actionable merge risk identified. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 3 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1📝 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 |
|
✅ Deterministic PR hygiene checks passed. |
리뷰 · 우선순위 77 / 80이 PR은 #4855를 닫는 실제 버그 수정이다. Anthropic(그리고 command-code)이 정상 턴을 지금 tip의 회귀는 기존 이 수정은 types.ts/config.ts 스플릿 캠페인과 무관하고, 중복 PR도 아니다. #4855 본문이 제안한 diff와 플랜 U1과 바이트 단위로 거의 같다. 디스플레이 결함이지 accounting 결함이 아닌 이유도 Codex 쪽 src/bridge/sse.ts:1174 - 지금은 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
Closes #4855
Summary
Anthropic ends a normal turn with
stop_reason: "end_turn"and the adapter forwards that string verbatim. The Responses bridge decided the terminal message phase from the raw truthiness ofstopReason, so"end_turn"took the "something went wrong" branch and every successful Anthropic-routed turn shipped its final assistant message with nophase. In Codex App the turn renders without the divider that separates activity from the answer, while native OpenAI turns in the same thread render it normally.The non-streaming path had the same gap for the same reason:
cleanDone = e.stopReason === undefinedis false for every Anthropic success, so the bufferedfinal_answerflush never ran.Both call sites now ask
isTruncatedStopReason, which is the classifier built for this and already imported in both files. That makes the phase decision agree with its three neighbours in the samecase "done":block — lines 1179, 1184 and 1196 already call it — and with lines 538 and 558 on the buffered side.This is not "treat every terminal as final". A clean stop and a cut-short stop stay distinguishable, and the distinction is now drawn by the same table that draws it everywhere else.
TRUNCATED_STOP_REASONSmaps Anthropic'srefusal,pause_turn,max_output_tokensandmodel_context_window_exceeded; it deliberately does not mapend_turn,stop_sequenceortool_use, and its header already states that an unknown reason is not truncation. Truncated turns still close without a phase, still fail an open tool call, still mark an in-flight search failed, and still suppress the compaction item. Theerrorandincompleteterminals are untouched, and the bufferedcleanDone && !errorEvent && !incompleteEventconjunction stays as written.Fixing the bridge rather than the Anthropic adapter also covers
command-code, which forwards raw AI SDK finish reasons and so returns a truthy"stop"on a clean turn.Verification
bun testin any form, nobun run test,test:changed,typecheck,bun x tsc,bun install,build:gui, orocxinvocation. A past local suite run deleted real~/.opencodexdata, so this lane verifies by static source reading and treats hosted CI at the exact head as the evidence of record.f1dfda8e48:src/bridge/sse.ts:1174,src/bridge/response-json.ts:530, and theTRUNCATED_STOP_REASONStable insrc/responses/truncated-stop-reason.ts.codex-rs/rollout-trace/src/reducer/conversation/normalize.rsmaps the phase string toConversationChannel::Finaland returnsNonefor a missing phase, andcodex-rs/thread-store/src/local/thread_history.rsresolvesfinal_agent_item_idbyjson_extract(item_json, '$.phase') = 'final_answer'before falling back to the last agent message on a completed turn. That fallback is why this is a display defect rather than an accounting one, which is how the issue describes it.tests/adapters/bridge.test.ts, which already drives bothbridgeToResponsesSSEandbuildResponseJSON, so no new test file and nolayout.jsonentry. Four cases per entry point:end_turnproducesfinal_answer,max_output_tokensproduces no phase,refusalproduces no phase and still reportscontent_filter, and an absentstopReasonkeeps its existing behaviour. The two truncation cases are what would catch an over-broad fix.structure/transports/responses.mdis updated in this commit because it owns this behaviour.Checklist
structure/transports/responses.mdis updated; nodocs-site/page documents the phase marker.Summary by CodeRabbit
Bug Fixes
Tests