feat(message): carry a tool result's trusted-verbatim request into the transcript#77
Open
yh928 wants to merge 2 commits into
Open
feat(message): carry a tool result's trusted-verbatim request into the transcript#77yh928 wants to merge 2 commits into
yh928 wants to merge 2 commits into
Conversation
…e transcript A `ToolResult` can hold structured metadata in `raw`, but folding it into the transcript goes through `Message::tool(call_id, content)`, which keeps only those two fields. Everything else is dropped at that boundary, so a host has no way to learn anything the tool wanted to say about its own output. That matters for one property in particular: whether the content may be reshaped. Hosts summarise, truncate, batch, and re-frame tool output, and usually should — but some results are only useful intact. An input schema whose argument names the model must copy exactly, a signature, a diff: rewriting those produces content that still reads well and is wrong. Today a host can only guess from the text, and guessing wrong is silent. Add an explicit, opt-in request: - `ToolResult::mark_trusted_verbatim()` / `is_trusted_verbatim()`, backed by `raw` under `TRUSTED_VERBATIM_KEY`, so the opt-in costs nothing for results that never use it and round-trips through a host's own serialization. - `ToolMessage.trusted_verbatim: bool`, `#[serde(default)]` so transcripts persisted before this field still deserialize (and read as not-trusted). - `Message::tool_from_result(&ToolResult)`, which carries the flag across; the agent-loop fold site now uses it. The flag is advisory: the crate transports it, honouring it is the host's job. `Message::tool` is unchanged and still sets `false`, so nothing moves for callers that do not opt in. Tests cover the opt-in being off by default, idempotent marking that preserves unrelated `raw` metadata, non-boolean or non-object `raw` never tripping it, `tool_from_result` matching `tool` for an unmarked result, and a pre-field transcript still loading. Verified: `cargo fmt --check`, `cargo clippy --all-targets -- -D warnings`, `cargo build --all-targets`, `cargo test` (1205 lib tests + suites), and `cargo run --example basic_graph`.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthrough
ChangesTrusted verbatim propagation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ToolResult
participant finish_tool_call
participant Message_tool_from_result
participant Transcript
ToolResult->>finish_tool_call: completed result
finish_tool_call->>Message_tool_from_result: full ToolResult
Message_tool_from_result->>Message_tool_from_result: read is_trusted_verbatim()
Message_tool_from_result->>Transcript: append ToolMessage with trust metadata
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/harness/tool/types.rs`:
- Around line 132-139: The mark_trusted_verbatim method must explicitly handle
non-object raw values instead of silently leaving them unmarked. Choose and
implement a clear behavior, such as adding a fallible try_mark_trusted_verbatim
API or encoding the trust marker while preserving the existing value, update
Message::tool_from_result to propagate that outcome correctly, and add a
regression test covering string/array raw values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 33ac1689-5708-4b0b-8b61-dcf0e12be50f
📒 Files selected for processing (6)
src/harness/agent_loop/tools.rssrc/harness/message/mod.rssrc/harness/message/test.rssrc/harness/message/types.rssrc/harness/tool/test.rssrc/harness/tool/types.rs
|
| Filename | Overview |
|---|---|
| src/harness/tool/types.rs | Adds TRUSTED_VERBATIM_KEY constant and mark_trusted_verbatim/is_trusted_verbatim on ToolResult; handles the non-object raw case by returning false (addresses the previous thread concern). |
| src/harness/message/types.rs | Adds trusted_verbatim bool to ToolMessage with correct serde(default, skip_serializing_if) attributes and a private is_false helper; backward-compatible and wire-efficient. |
| src/harness/message/mod.rs | Adds Message::tool_from_result that carries trusted_verbatim across from ToolResult; Message::tool is unchanged and continues to yield trusted_verbatim: false. |
| src/harness/agent_loop/tools.rs | Agent loop now uses tool_from_result instead of Message::tool at the finish_tool_call fold site, correctly carrying the flag; error/invalid paths still use Message::tool (appropriate, as those have no ToolResult). |
| src/harness/middleware/library/context.rs | MicrocompactMiddleware replaces tool message bodies with a placeholder without checking trusted_verbatim, silently defeating the new flag and stripping it from the replacement message. |
| src/harness/tool/test.rs | Adds four targeted tests: opt-in idempotency, non-boolean key guards, refused marking on non-object raw, and the legacy-transcript serde path. |
| src/harness/message/test.rs | Adds tests covering tool_from_result flag carry-through, legacy deserialization, and wire format omission for unset flag. |
Sequence Diagram
sequenceDiagram
participant Tool
participant AgentLoop as "Agent Loop"
participant MsgFactory as "Message factory"
participant Transcript as "Transcript"
participant Middleware as "MicrocompactMiddleware"
participant Host
Tool->>Tool: "mark_trusted_verbatim() sets raw key"
Tool-->>AgentLoop: "ToolResult { trusted_verbatim in raw }"
AgentLoop->>MsgFactory: "tool_from_result(&result)"
MsgFactory->>MsgFactory: "is_trusted_verbatim() -> true"
MsgFactory-->>Transcript: "ToolMessage { trusted_verbatim: true }"
Transcript-->>Middleware: "ModelRequest with ToolMessage"
Note over Middleware: "Does NOT check trusted_verbatim - replaces body with placeholder and strips flag to false"
Middleware-->>Host: "ToolMessage { trusted_verbatim: false }"
Host->>Host: "Sees false, may reshape content"
Reviews (2): Last reviewed commit: "fix(tool): report a refused trusted-verb..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…flag Both review bots flagged the same defect, and they were right: when `raw` already held a non-object value, `mark_trusted_verbatim` did nothing at all. `get_or_insert_with` returns the existing scalar, `as_object_mut()` gives `None`, and the insert was skipped — with no return value, no panic, and no diagnostic. `is_trusted_verbatim()` then kept answering `false` while the tool author believed the opt-in had taken effect. The flag lives under a key in `raw`, so a scalar genuinely has nowhere to put it, and clobbering a value the tool deliberately set would be worse. So the request is now *refused and reported* rather than silently lost: `mark_trusted_verbatim` returns `bool`, and is `#[must_use]` precisely because the failure is otherwise invisible. A tool that wants both can move its value under a key of its own and call again. Second finding: the field serialized as `"trusted_verbatim":false` on every tool message, which contradicts this change's own "costs nothing unless you opt in" claim and diverges from the rest of the crate. Added `skip_serializing_if`, reusing the `is_false` helper convention already used by `graph::export::types` rather than an inline `std::ops::Not::not`. Tests: a new regression case walks a string, an array, and a number `raw`, asserting each refuses, leaves the tool's own value untouched, and that the call succeeds once room is made; a new wire test pins that an unset flag is omitted and a set one is present. The existing tests now assert the return value instead of discarding it — `#[must_use]` caught all three call sites. Verified: fmt, clippy (both feature sets), build (both), `cargo test` (1207), `cargo test --all-features`, and `cargo run --example basic_graph`.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A
ToolResultcan hold structured metadata inraw, but folding it into the transcript goes throughMessage::tool(call_id, content), which keeps only those two fields. Everything else is dropped at that boundary, so a host has no way to learn anything the tool wanted to say about its own output.That matters for one property in particular: whether the content may be reshaped. Hosts summarise, truncate, batch, and re-frame tool output, and usually should — but some results are only useful intact. An input schema whose argument names the model must copy exactly, a signature, a diff: rewriting those produces content that still reads well and is wrong. Today a host can only guess from the text, and guessing wrong is silent.
This adds an explicit, opt-in request that survives the fold:
ToolResult::mark_trusted_verbatim()/is_trusted_verbatim(), backed byrawunder a newTRUSTED_VERBATIM_KEY, so the opt-in costs nothing for results that never use it and round-trips through a host's own serialization.ToolMessage.trusted_verbatim: bool,#[serde(default)].Message::tool_from_result(&ToolResult), which carries the flag across. The agent-loop fold site (harness/agent_loop/tools.rs) now uses it.The flag is advisory: the crate transports it, honouring it is the host's job.
Why
raw-backed rather than a field onToolResultToolResultis constructed all over user code; adding a required field would be a breaking change for every literal, and anOption/Defaultfield would still widen a struct that most results never need.rawalready exists for exactly this kind of side-band metadata, and a host that serializes results through its own types round-trips the flag with no schema change.Concrete motivation
Downstream (OpenHuman) delivers a tool's full input schema to the model as a recoverable tool error, then verifies on the next turn that the model can still see it. The host's own
after_toolstages summarise and whitespace-collapse tool output; without a way to say "not this one", the delivered schema arrives reshaped and the verification can never match. That is a host problem to solve, but it needs one bit of information the crate currently discards.API Or Behavior Changes
Additive only.
TRUSTED_VERBATIM_KEY,ToolResult::mark_trusted_verbatim,ToolResult::is_trusted_verbatim,ToolMessage.trusted_verbatim,Message::tool_from_result.Message::toolis unchanged and still yieldstrusted_verbatim: false, so nothing moves for callers that do not opt in.ToolMessagegains a field. Struct-literal construction ofToolMessageoutside the crate would need the new field;Message::tool/tool_from_resultare unaffected.#[serde(default)]means transcripts persisted before this field still deserialize, reading as not-trusted.false. No effect unless a tool opts in.Tests
All run locally on this branch:
cargo fmt --checkcargo clippy --all-targets -- -D warningscargo clippy --all-targets --all-features -- -D warningscargo build --all-targetscargo build --all-targets --all-featurescargo test— 1205 lib tests + suites, 0 failedcargo test --all-features— 1277 lib tests + suites, 0 failedcargo run --example basic_graphNew coverage:
trusted_verbatim_is_opt_in_and_round_trips_through_raw— off by default; idempotent marking preserves unrelatedrawmetadata.unrelated_raw_metadata_never_sets_the_flag— a non-boolean value at the key, and a non-objectraw, both read asfalse, so the opt-in cannot be tripped by accident.tool_from_result_carries_trusted_verbatim_and_tool_does_not— an unmarked result folds identically toMessage::tool; a marked one carries the flag.a_transcript_stored_before_the_flag_existed_still_deserializes— theserde(default)path.Documentation
Rustdoc on every new item, including why the flag is advisory and why it lives in
raw. No wiki page seemed warranted for a single additive field — happy to add one if you would prefer it documented there.Summary by CodeRabbit
New Features
Bug Fixes
Tests