fix(runtime): embed developer-loop placeholder in first result delivery (closes #6251)#6254
Conversation
When aggregate_developer_loops collapsed a long run of developer-tool steps, it inserted a standalone Role::User placeholder message right after the first step's Role::User tool-result delivery. session_repair intentionally skips merging pure tool-result deliveries, so Anthropic's Messages API received two consecutive User messages and returned 400. Instead of creating a separate User placeholder, append the aggregation notice as a Text block inside the first result delivery. This keeps the history alternating Assistant/User and avoids the API error. Closes librefang#6251
|
Thanks for your first pull request! 🎉 A maintainer will review it soon. While you wait, please make sure:
We aim to provide initial feedback within 7 days. See CONTRIBUTING.md for details. |
houko
left a comment
houko
left a comment
There was a problem hiding this comment.
Thanks — the Anthropic-side fix (avoiding the consecutive role: user turns that triggered the 400) is correct, but the chosen approach regresses OpenAI-compatible providers, which is the exact failure mode issue #6251 warned about.
The placeholder is appended as a separate ContentBlock::Text after the ToolResult (crates/librefang-runtime/src/compactor.rs, aggregate_developer_loops → append_block_to_message), producing Blocks([ToolResult, Text]). In the OpenAI driver, emission of the accumulated text parts is gated on !has_tool_results (crates/librefang-llm-drivers/src/drivers/openai.rs:940); since the message now contains a ToolResult, has_tool_results is true, so the placeholder Text is silently discarded and never reaches OpenAI / Groq / Moonshot / any OAI-compatible backend. Before this PR the standalone MessageContent::Text placeholder went through the (Role::User, MessageContent::Text) arm (openai.rs:841) and was sent. So this is a behavioral regression for those providers.
Issue #6251 explicitly recommended embedding the notice into the first ToolResult's content string for this reason. Please append the notice into the ToolResult content (e.g. add the [developer loop aggregated …] text to its content field) rather than adding a separate Text block — that survives both the Anthropic and the OpenAI translation paths. A driver-level round-trip test feeding Blocks([ToolResult, Text]) through both drivers and asserting the notice survives would lock this in (the current test only checks message structure / text_content(), which doesn't catch the driver-layer drop).
Heads-up: the real Rust CI (cargo check / clippy / nextest / secret-scan) hasn't run on this PR yet — it's held in action_required on the first-time-contributor approval gate, so the current green checks are only the lightweight housekeeping workflows.
…urvive OpenAI path The first librefang#6254 fix appended the aggregation notice as a separate `ContentBlock::Text` after the `ToolResult` in the first result delivery. That survives the Anthropic path but the OpenAI-compatible driver gates emission of accumulated text `parts` on `!has_tool_results`, so a `Text` block sharing a message with a `ToolResult` is silently dropped — the notice never reached OpenAI / Groq / Moonshot. This was the exact OpenAI-compatible regression librefang#6251 warned about. Fold the notice into the first `ToolResult`'s `content` string instead. That single shape survives both the Anthropic and the OpenAI translation paths. - `developer_loop_placeholder()` now returns the notice `String`. - Replace `append_block_to_message()` with `append_notice_to_first_tool_result()`, which appends the notice to the first `ToolResult.content` (with a Text-block fallback if no ToolResult is present). - Add driver-level round-trip tests in both `openai.rs` and `anthropic.rs` that feed a `ToolResult` carrying the notice through the request builder and assert the notice reaches the wire payload — the message-structure test alone did not catch the driver-layer drop. - Update the runtime tests to assert the notice lives in the ToolResult content. cargo fmt --all; cargo clippy -p librefang-runtime -p librefang-llm-drivers --all-targets -- -D warnings; cargo test -p librefang-runtime --lib aggregate_developer_loops (6 passed); cargo test -p librefang-llm-drivers --lib dev_loop_notice (2 passed).
|
Thanks for catching the OpenAI-compatible regression — you're right, and I've pushed the fix in 2d8cfc0. Change: the notice is now folded into the first
Driver-level round-trip tests (the part the old structure-only test missed):
Verification:
|
Addressed by the follow-up commit (reviewer's requested approach implemented + driver round-trip tests added); branch updated to current main. Re-reviewed and clean.
2 participants
Problem
When
aggregate_developer_loopscollapses a long run of developer-tool steps, it previously inserted a standaloneRole::Userplaceholder message immediately after the first step'sRole::Usertool-result delivery.session_repairintentionally skips merging pure tool-result deliveries, so the placeholder and the result delivery stayed as two consecutiveUsermessages. Anthropic's Messages API rejects this with:Fixes #6251.
Change
developer_loop_placeholder()now returns aContentBlock::Textinstead of a fullMessage.append_block_to_message()helper to safely append a block to eitherMessageContent::TextorMessageContent::Blocks.aggregate_developer_loops()embeds the aggregation notice inside the first result delivery instead of pushing a separateUsermessage.aggregate_developer_loops_avoids_consecutive_user_messagesto assert no adjacentUsermessages and that the placeholder lives in the first result delivery.Verification
cargo fmt --allcargo clippy -p librefang-runtime --all-targets -- -D warningscargo test -p librefang-runtime aggregate_developer_loops(6 passed)