fix(claude-code): stop emitting assistant roles on CC stdin (#5711) - #5816
Conversation
![tinysweeper[bot]](https://avatars.githubusercontent.com/in/4526585?s=60&v=4){kind=small}
tinysweeper
Bot
added
the
priority: p3
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review. 📝 WalkthroughWalkthrough
ChangesClaude Code input handling
Estimated code review effort: 2 (Simple) | ~15 minutes Merge Risk: ⚪ Minimal · up to This localized change reshapes Claude Code input so prior conversation context remains available without emitting unsupported assistant roles; no actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Linked Issues checkExplanation The implementation satisfies issue ✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
Warning Your free Security trial is over. An organization admin can upgrade to Advanced for continuous pull request security review or dismiss this notice. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![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: 1357accc30
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if let Some(transcript) = render_transcript(history) { | ||
| push_json_line(&mut out, &user_row(&transcript)); |
There was a problem hiding this comment.
When a new-session history ends with an assistant turn, latest is empty but this still emits the transcript as a role=user row. Because the driver aborts only when build_stdin returns no bytes, Claude treats that context-only row as a new prompt and can generate an unsolicited duplicate response—the exact provider-switch scenario covered by the new test—despite the comment claiming there is no fresh instruction. Require a trailing user turn before emitting anything.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Maintainer triage — not taking this one, and here is the reasoning so the thread is not left silent.
The suggestion is to return an empty payload (and so hit the driver's no input messages to deliver bail at driver.rs:341) when a new-session history ends on an assistant turn. That would be a behaviour change beyond the scope of this fix, and a regression on the case it names:
- On
maintoday this case never bails — the old builder emitted a row per message, so the payload was always non-empty. It emitted them with"role":"assistant", which is precisely the Claude Code rejects assistant history when starting a new CLI session #5711 crash. This PR keeps the payload non-empty and makes it legal; it does not introduce the "context with no trailing prompt" situation. - The scenario is a user switching an existing thread onto this provider. Making that hard-fail with
no input messages to deliveris strictly worse for that user than sending the conversation across as labelled context. - The emitted row is explicitly prefixed
Earlier conversation, for context only — do not answer it again:, so the "treated as a new prompt" framing overstates it. A model asked to continue will produce a turn either way; the question is only whether it does so with the thread's context or with an error.
If the project does want inference invoked on an assistant-terminated history to be a hard error, that is a decision for the harness that builds messages, not for this transport encoder — and it should be its own change with its own test.
| fn render_transcript(history: &[&ChatMessage]) -> Option<String> { | ||
| let mut body = String::new(); | ||
| for msg in history { | ||
| let speaker = match msg.role.as_str() { | ||
| "user" => "User", | ||
| "assistant" => "Assistant", |
There was a problem hiding this comment.
This introduces a second transcript-replay implementation that independently decides which roles survive and how turns are serialized. That can drift from the active tool dialect—for example, its handling of assistant/tool exchanges—and produce malformed next iterations when the dialect's tool grammar changes. The repository contract explicitly requires transcript replay and tool-call formatting to remain together in tinyagents::harness::tool_calling::dialect, so this Claude-specific behavior should be added or delegated there instead of implemented in the transport builder.
AGENTS.md reference: AGENTS.md:L487-L494
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Maintainer triage — not taking this one in this PR.
The AGENTS.md contract cited ("Tool calling lives in tinyagents") governs the tool-calling dialect: how a call is advertised, parsed, rendered back, and replayed, kept together as XmlDialect / PFormatDialect / NativeDialect. input_builder.rs is not a dialect — it is the transport encoder for the Claude Code CLI's --input-format stream-json stdin framing. It chooses no tool grammar, and the CLI executes its own tools rather than round-tripping them through the harness.
More to the point: this PR does not introduce a second implementation. src/openhuman/inference/provider/claude_code/input_builder.rs already exists on main and already decides which roles survive and how turns are serialised — the _ => continue that drops system and tool rows is unchanged by this diff, and so is the module's stated v1 piping policy. What changed is that assistant turns are no longer emitted with "role":"assistant", because the CLI rejects that outright (Error: Expected message role 'user', got 'assistant', exit 1, #5711).
Relocating stream-json framing into tinyagents::harness::tool_calling::dialect would be a cross-repo architectural change to a crate this repo vendors. That may well be worth doing, but it is not a prerequisite for fixing a crash, and holding a one-file bug fix behind it would leave #5711 broken on main. Worth its own issue if the maintainers want it.
1357acc to
54e3e32
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
How this change flows2 changed behaviours across 2 relationships. The code graph does not know these behaviours yet — normal for newly added code, and a cold index otherwise. 1 further behaviour left out to keep the diagram readable. flowchart LR
n0["build_stdin<br/>changed"]:::changed
n1["resume_pipes_only_last_user_turn<br/>changed"]:::changed
n1 -->|calls| n0
n1 -->|tests| n0
classDef changed fill:#0d4429,stroke:#238636,color:#e6edf3
classDef impacted fill:#161b22,stroke:#6e7681,color:#c9d1d9
classDef flagged fill:#5a1e02,stroke:#d93f0b,color:#ffffff
classDef blocking fill:#67060c,stroke:#f85149,color:#ffffff
Green: changed behaviour. Grey: surrounding behaviour. Arrows name the call, use, implementation, or test relationship. Orange: has findings. Red: has a finding that blocks the merge. |
…nsai#5711) Every stdin row must carry `message.role == "user"`. The CLI validates this before it invokes the model and exits 1 with `Error: Expected message role 'user', got 'assistant'` — `type: "user"` on the envelope is not enough. Replaying a prior assistant turn as itself therefore killed the turn outright on any new CC session with history. Prior turns are folded into one labelled transcript row instead, and only a trailing *user* turn is treated as the prompt: a conversation that ends on an assistant turn (the user switched provider mid-thread) is all context and carries no fresh instruction. The label matters — without it the model reads several consecutive user messages and can take its own past replies as new instructions. Rebased onto main's sibling-test layout: the tests now live in input_builder_tests.rs. `new_session_pipes_full_user_history` is gone deliberately — it asserted the assistant-role rows this change stops emitting. Its surviving intent, that history reaches a new session, is covered by `new_session_carries_prior_turns_as_one_labelled_transcript`.
54e3e32 to
3c26f14
Compare
…ode-assistant-role\n\nfix(claude-code): stop emitting assistant roles on CC stdin (tinyhumansai#5711)\n
3 participants
Closes #5711.
Reproduced first-hand
Against Claude Code CLI 2.1.221 (newer than the 2.1.207 in the report), Windows 11, using the issue's minimal input:
Not one non-hook stdout event was produced — the turn died before any model invocation, which matches the report. The same three lines with every
message.roleset to"user"exit 0 and produce a normal response, so the constraint is on the inner role, not thetype: "user"envelope.The fix
Prior turns are folded into one labelled transcript row carried as
role: "user"; the trailing user turn follows verbatim:{"type":"user","message":{"role":"user","content":[{"type":"text","text":"Earlier conversation, for context only — do not answer it again:\n\nUser: first user\nAssistant: prior assistant"}]}} {"type":"user","message":{"role":"user","content":[{"type":"text","text":"<the actual prompt>"}]}}I piped that exact payload to the real CLI: exit 0, model responded.
The issue offers two strategies; this is "serialize prior transcript context into a user message" rather than "send only the trailing user turn", because the latter silently discards the conversation the user can see on screen.
Why labelled, rather than rewriting each turn into its own bare
userrow: without labels the model receives several consecutive user messages and can read its own past replies as fresh instructions. Keeping the latest turn as its own verbatim row also means the actual prompt is never reworded — only the context around it is reshaped.One case the issue does not mention but this repo hits: a history ending on an assistant turn, which is exactly what switching an existing thread to this provider produces. It is now treated as all context and no prompt, instead of sending the assistant's own words to the model as if the user had typed them.
Verification
input_builder, 3 of which fail against the previous implementation (new_session_never_emits_an_assistant_role,new_session_carries_prior_turns_as_one_labelled_transcript,a_history_ending_on_an_assistant_turn_is_all_context) — verified by restoring the old builder with the new tests in place:3 passed; 3 failed.cargo test --lib claude_code— 45 passed.cargo fmt --all— clean.One test helper,
assert_every_row_is_a_user_role, parses each emitted line and asserts the invariant directly, so any future row-shaping change is held to the schema rather than to a string match.Note on
--append-system-promptThe system row is still filtered out and still rides
--append-system-prompt; a test pins that it does not leak into the transcript block.Summary by CodeRabbit