Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 66 additions & 15 deletions src/openhuman/inference/provider/claude_code/input_builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,16 @@
//! like:
//! { "type":"user", "message":{"role":"user","content":[{"type":"text","text":"..."}]} }
//!
//! 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'` otherwise (#5711) —
//! `type: "user"` on the envelope is not enough. Prior assistant turns
//! therefore cannot be replayed as themselves; they are folded into a
//! labelled transcript block carried by a `user` row.
//!
//! v1 piping policy:
//! - On a *new* CC session: send every history `ChatMessage` so claude
//! - On a *new* CC session: send the full prior conversation as one
//! transcript `user` row, then the latest user turn verbatim, so claude
//! has full context (system message is conveyed via
//! `--append-system-prompt`, not stdin).
//! - On a `--resume` of an existing CC session: claude already has prior
Expand All @@ -31,26 +39,69 @@ pub fn build_stdin(messages: &[ChatMessage], is_new_session: bool) -> Vec<u8> {
.collect()
};

for msg in to_emit {
let role = match msg.role.as_str() {
"user" => "user",
"assistant" => "assistant",
// The trailing user turn is the actual prompt and is sent verbatim.
// Everything before it is context, and has to reach the CLI as `user`
// rows, so it goes as one labelled transcript block rather than as
// rewritten turns.
// Only a trailing *user* turn is the prompt. If the conversation ends on
// an assistant turn (e.g. the user switched provider mid-thread), all of
// it is context and none of it is a fresh instruction.
let split = match to_emit.last() {
Some(last) if last.role == "user" => to_emit.len() - 1,
_ => to_emit.len(),
};
let (history, latest) = to_emit.split_at(split);

if let Some(transcript) = render_transcript(history) {
push_json_line(&mut out, &user_row(&transcript));
Comment on lines +55 to +56

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Avoid emitting context when no prompt is present

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 👍 / 👎.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 main today 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 deliver is 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.

}
for msg in latest {
push_json_line(&mut out, &user_row(&msg.content));
}

out.into_bytes()
}

/// One stdin row. `role` is always `"user"` — see the module docs.
fn user_row(text: &str) -> Value {
json!({
"type": "user",
"message": {
"role": "user",
"content": [{"type": "text", "text": text}],
},
})
}

/// Fold prior turns into a single labelled transcript, or `None` when there
/// is nothing to carry.
///
/// Labelling matters: without it the model receives what looks like several
/// consecutive user messages and can read its own past replies as fresh
/// instructions.
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",
Comment on lines +82 to +87

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Move transcript replay into the provider dialect

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 👍 / 👎.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// CC stdin doesn't accept `system` or `tool` rows. The system
// prompt is plumbed via `--append-system-prompt`; tool roles
// belong to the harness, not the CLI's input format.
_ => continue,
};
let line = json!({
"type": "user",
"message": {
"role": role,
"content": [{"type": "text", "text": msg.content}],
},
});
push_json_line(&mut out, &line);
body.push_str(speaker);
body.push_str(": ");
body.push_str(&msg.content);
body.push('\n');
}

out.into_bytes()
if body.is_empty() {
return None;
}
Some(format!(
"Earlier conversation, for context only — do not answer it again:\n\n{}",
body.trim_end()
))
}

fn push_json_line(buf: &mut String, v: &Value) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,21 +9,101 @@ fn msg(role: &str, content: &str) -> ChatMessage {
}
}

/// Every row's `message.role` must be `"user"`. The CLI validates this
/// before invoking the model and exits 1 with
/// `Expected message role 'user', got 'assistant'` otherwise (#5711).
/// Verified against Claude Code CLI 2.1.221.
fn assert_every_row_is_a_user_role(payload: &str) {
for (i, line) in payload.lines().enumerate() {
let row: Value = serde_json::from_str(line).unwrap_or_else(|e| {
panic!(
"row {i} is not valid JSON: {e}
{line}"
)
});
assert_eq!(
row["message"]["role"], "user",
"row {i} must carry role=user, the only role CC stdin accepts
{line}"
);
assert_eq!(row["type"], "user", "row {i} envelope type");
}
}

#[test]
fn new_session_pipes_full_user_history() {
fn new_session_never_emits_an_assistant_role() {
let history = vec![
msg("system", "you are helpful"),
msg("user", "first user"),
msg("assistant", "prior assistant"),
msg("user", "latest user"),
];
let s = String::from_utf8(build_stdin(&history, true)).unwrap();
assert_every_row_is_a_user_role(&s);
assert!(
!s.contains("\"role\":\"assistant\""),
"an assistant role row is what the CLI rejects:
{s}"
);
}

#[test]
fn new_session_carries_prior_turns_as_one_labelled_transcript() {
let history = vec![
msg("system", "you are helpful"),
msg("user", "hi"),
msg("assistant", "hello"),
msg("user", "how are you?"),
];
let bytes = build_stdin(&history, true);
let s = String::from_utf8(bytes).unwrap();
let s = String::from_utf8(build_stdin(&history, true)).unwrap();
let lines: Vec<_> = s.lines().collect();
assert_eq!(lines.len(), 3); // system filtered out
assert!(lines[0].contains("\"hi\""));
assert!(lines[1].contains("\"hello\""));
assert!(lines[2].contains("how are you"));

// One transcript row + the latest user turn. The system row is still
// filtered out — it rides `--append-system-prompt`.
assert_eq!(
lines.len(),
2,
"got:
{s}"
);
assert!(lines[0].contains("User: hi"));
assert!(lines[0].contains("Assistant: hello"));
assert!(
!lines[0].contains("you are helpful"),
"the system message must not leak into the transcript"
);

// The prompt itself is passed through untouched, not folded in.
let latest: Value = serde_json::from_str(lines[1]).unwrap();
assert_eq!(latest["message"]["content"][0]["text"], "how are you?");
}

#[test]
fn a_history_ending_on_an_assistant_turn_is_all_context() {
// Switching an existing conversation to the CC provider can leave the
// assistant speaking last; none of it is a fresh instruction.
let history = vec![msg("user", "hi"), msg("assistant", "hello")];
let s = String::from_utf8(build_stdin(&history, true)).unwrap();
let lines: Vec<_> = s.lines().collect();
assert_eq!(
lines.len(),
1,
"got:
{s}"
);
assert_every_row_is_a_user_role(&s);
assert!(lines[0].contains("User: hi"));
assert!(lines[0].contains("Assistant: hello"));
}

#[test]
fn a_single_user_turn_is_sent_verbatim_with_no_transcript() {
let history = vec![msg("user", "just this")];
let s = String::from_utf8(build_stdin(&history, true)).unwrap();
let lines: Vec<_> = s.lines().collect();
assert_eq!(lines.len(), 1);
let row: Value = serde_json::from_str(lines[0]).unwrap();
assert_eq!(row["message"]["content"][0]["text"], "just this");
}

#[test]
Expand All @@ -38,6 +118,7 @@ fn resume_pipes_only_last_user_turn() {
let lines: Vec<_> = s.lines().collect();
assert_eq!(lines.len(), 1);
assert!(lines[0].contains("\"follow-up\""));
assert_every_row_is_a_user_role(&s);
}

#[test]
Expand Down
Loading