feat(dialect): honour trusted_verbatim when framing tool results - #145
Open
yh928 wants to merge 1 commit into
Open
feat(dialect): honour trusted_verbatim when framing tool results#145yh928 wants to merge 1 commit into
yh928 wants to merge 1 commit into
Conversation
`ToolResult::mark_trusted_verbatim` and `ToolMessage::trusted_verbatim`
already exist, and `middleware::library::context` already skips such results
during microcompact. The serializers did not: `text::format_results` wraps
every result in `<tool_result>`, prefixes the batch with `[Tool results]`, and
runs `neutralize_protocol_tags` over the body. So a result marked verbatim was
still reshaped on the way to the model, and the mark only held for as long as
nothing tried to render it.
That matters for the case the flag exists for — output a consumer identifies by
a leading marker, or re-hashes to confirm it arrived intact. Wrapped and
batched, the marker is no longer at byte 0 and the payload is no longer the
bytes that were produced, so neither check can succeed. The result is not
corrupt in any way a reader would notice; it is simply no longer the same
bytes, which is exactly what the flag promises.
Text dialects now close the open batch before a verbatim result, emit its
content as its own message, and reopen after. Order is preserved. An unmarked
round takes the same single-batch path it always did, allocating what it always
did. The same split applies on replay in `to_provider_messages`, or the
guarantee would end at the first restart.
The native dialect needs no split — it already passes content through untouched
in its own provider message — but now carries the flag onto `ToolResultEntry`
so a transcript it wrote and a text dialect later replays keeps the mark.
**Breaking:** `ToolDialect::format_results` returns `Vec<TranscriptEntry>`. A
single record cannot put a verbatim result at byte 0 *and* carry the rest of
the round, so the seam has to be able to say "two records". All four
implementors are in this crate; hosts adapt at one call site.
I considered an additive `format_results_split` with a defaulted
implementation, which would not break anyone. I did not take it: a host calling
the old method would silently get the unsplit behaviour, which is the precise
failure this change exists to remove, and a second method that is wrong to call
is worse than a signature that has to be updated.
`ToolOutcome::verbatim()` and `ToolResultEntry::{new, verbatim}` are the
constructors; the fields default to `false`, and the persisted one is
`#[serde(default)]` so existing transcripts deserialize unchanged.
942 tests pass; fmt and clippy clean.
|
Warning Review limit reached
On-demand reviews are free for the next 18 days. After that, they cost $0.25 per reviewed file. Or wait 54 minutes for your next included review. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (7)
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 |
![tinysweeper[bot]](https://avatars.githubusercontent.com/in/4526585?s=60&v=4){kind=small}
How this change flows2 changed behaviours across 7 relationships. 5 surrounding behaviours are shown (60 graph nodes walked). 45 further behaviours left out to keep the diagram readable. flowchart LR
n0["NativeDialect<br/>changed"]:::changed
n1["PFormatDialect<br/>changed"]:::changed
n2["ToolDialect"]:::impacted
n3["response"]:::impacted
n4["TranscriptEntry"]:::impacted
n5["DialectMessage"]:::impacted
n6["DialectResponse"]:::impacted
n0 -->|implements| n2
n1 -->|implements| n2
n2 -->|uses| n4
n2 -->|uses| n5
n2 -->|uses| n6
n3 -->|uses| n6
n4 -->|uses| n5
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. |
tinysweeper
Bot
added
the
priority: p3
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
trusted_verbatimis already a first-class idea in this crate —ToolResult::mark_trusted_verbatim,ToolMessage::trusted_verbatim, andmiddleware::library::contextskipping such results during microcompact. The serializers were the one place that ignored it. This makes the text dialects honour it.Problem
text::format_resultswraps every result in<tool_result name=… status=…>, prefixes the batch with[Tool results], and runsneutralize_protocol_tagsover the body. All three are right for an ordinary result. For one marked verbatim they are all wrong:neutralize_protocol_tagsrewrites the bytes.So a producer that marks a result verbatim gets it reshaped anyway, and the flag holds only until something renders it. That defeats the two things the flag is for: output a consumer identifies by a leading marker, and output it re-hashes to confirm it arrived intact. Neither survives, and nothing looks broken — the result reads fine, it is just no longer the same bytes.
pformatdelegates totext, so both text dialects are affected.nativeis not: it already passes content through untouched in its own provider message.Solution
Text dialects close the open batch before a verbatim result, emit its content as its own message, and reopen after. Order is preserved. An unmarked round takes the same single-batch path it always did, allocating what it always did — kept as its own branch precisely so the common case is unchanged.
The same split applies in
to_provider_messages, or the guarantee would end at the first restart: a durable transcript replayed after a reload has to put the verbatim entry back in its own turn.nativeneeds no split, but now carries the flag ontoToolResultEntry— otherwise a transcript it writes and a text dialect later replays loses the mark at the dialect boundary.Breaking change, and the alternative I rejected
ToolDialect::format_resultsreturnsVec<TranscriptEntry>instead of one. A single record cannot put a verbatim result at byte 0 and carry the rest of the round, so the seam has to be able to say "two records". All four implementors are in this crate; a host adapts at one call site.I considered an additive
format_results_splitwith a defaulted implementation, which breaks nobody. I did not take it: a host calling the old method would silently get the unsplit behaviour — which is precisely the failure this change removes — and a second method that is wrong to call is worse than a signature that has to be updated. Happy to switch if you'd rather stage it that way.ToolOutcome::verbatim()andToolResultEntry::{new, verbatim}are the constructors. Both fields default tofalse, and the persisted one is#[serde(default)], so existing transcripts deserialize unchanged.Tests
942 pass; fmt and clippy clean. New coverage for the split at byte 0, the absence of wrapping/neutralizing on a verbatim result, the unmarked round staying exactly one batched record, the replay path, and the native dialect carrying the flag.
Related
This is the blocker for tinyhumansai/openhuman#4861, which generalises a tool-contract gate that credits a delivered contract only while its payload still hashes to the recorded value. Without this the gate works on native providers and re-delivers forever on text ones. The change stands on its own here regardless: a host that marks a result verbatim and has it rewritten is a crate-level bug.