fix(parse): sweep an orphaned tag-family closer with no opener - #27
Merged
Merged
Conversation
next_opener() never treats a closing marker as an opener — by design,
since positional pairing means a closer only ever pairs with an
opener that precedes it. But that leaves a real gap: a model that
made its actual tool call over a structured/native channel has no
`<tool_call>` opener anywhere in its *text* at all, yet sometimes
still types a habitual `</tool_call>` in its narrative out of trained
habit. With no opener to pair it with, the closer was never
recognized as protocol furniture and leaked into the visible chat
text verbatim.
Reproduces a real leak observed live, mid-answer, right after a
"Reasoning · 1 tool call" step:
Let me check the details on the top contenders to find the best
one for you.
</tool_call>
[... normal answer continues ...]
Fix: probe_decided now also checks for a closing tag-family marker
that sits before the next real opener (or has no opener after it at
all) and sweeps it as Decoded::Noise — the same treatment invoke_xml's
WRAPPER_RE already gives a bare wrapper closer.
Also extracted the doubled-opener-skip loop into its own
`tag_close_skipping_doubled_openers` helper: the orphan-closer check
pushed `probe_decided` over clippy's `too_many_lines` limit, and the
loop was a natural, self-contained unit to pull out — no behavior
change there, `cargo test` before and after the extraction is
byte-identical output.
Test plan:
- New test `an_orphaned_closer_with_no_opener_anywhere_is_removed_not_shown`
reproduces the exact leaked sequence — fails without the fix, passes
with it.
- `cargo test -p tinytools-agent`: 323 passed, no regressions (in
particular `a_doubled_opener_does_not_swallow_an_unrelated_closing_tag`
and the pipe/newline doubled-closer tests, which exercise the
extracted loop, still pass unchanged).
- `cargo fmt --all -- --check`: clean.
- `cargo clippy --all-targets --all-features -- -D warnings`: clean.
- `cargo build --all-targets --all-features`: clean.
Tiny Sweeper reviewTiny Sweeper reviewed this change across 6 lane(s) and found 0 active actionable finding(s). Detailed lane evidence and any incomplete work are listed below. State: Ready for maintainer review Review snapshot
Completeness: Complete What changedThe review could not produce a supported behavioral summary; inspect the cited changed surface and lane details below. FeaturesNone identified with supported citations. TestsNo supported feature-to-test mapping was produced. Test execution is not inferred. FindingsNo active actionable findings. Before mergeNone. How this fits togetherflowchart LR
n0["Tagged<br/>changed"]:::changed
n1["swallow_extra_closers<br/>changed"]:::changed
n2["two_adjacent_blocks_are_still_two_blocks<br/>changed"]:::changed
n3["probe_decided"]:::impacted
n4["parse_tool_calls"]:::impacted
n5["Grammar"]:::impacted
n6["parse"]:::impacted
n7["len"]:::impacted
n8["ok"]:::impacted
n0 -->|implements| n5
n1 -->|calls| n7
n2 -->|calls| n6
n3 -->|calls| n1
n3 -->|calls| n7
n3 -->|calls| n8
n6 -->|calls| n4
n6 -->|tests| n4
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
Agent review detailscritique
security
tests
commits
description
e2e
Evidence and run details
|
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: Comment |
![tinysweeper[bot]](https://avatars.githubusercontent.com/in/4526585?s=60&v=4){kind=small}
There was a problem hiding this comment.
tinysweeper found nothing blocking. Approving.
$0.0063 · 106,313 in / 13,461 out · 3,992 cached (4%) · ladder/vectors, gpt-5.6-luna, deepseek/deepseek-v4-flash · 406 embedded
critique: $0.0012 · 41,644 in / 2,325 out · 2,118 cached (5%) · gpt-5.6-luna
security: $0.0011 · 41,156 in / 1,295 out · 1,874 cached (5%) · gpt-5.6-luna
tests: $0.0016 · 14,160 in / 1,878 out · 0 cached (0%) · deepseek/deepseek-v4-flash
description: $0.0016 · 5,781 in / 5,949 out · 0 cached (0%) · deepseek/deepseek-v4-flash
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
2 participants
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
next_opener()never treats a closing marker as an opener — by design, since positional pairing means a closer only ever pairs with an opener that precedes it. But that leaves a real gap: a model that made its actual tool call over a structured/native channel has no<tool_call>opener anywhere in its text at all, yet sometimes still types a habitual</tool_call>in its narrative out of trained habit. With no opener to pair it with, the closer was never recognized as protocol furniture and leaked into the visible chat text verbatim.Real-world repro
Observed live, mid-answer, right after a "Reasoning · 1 tool call" step in OpenHuman:
That bare
</tool_call>rendered as literal bold text in the chat.Fix
probe_decidednow also checks for a closing tag-family marker that sits before the next real opener (or has no opener after it at all) and sweeps it asDecoded::Noise— the same treatmentinvoke_xml'sWRAPPER_REalready gives a bare wrapper closer.Also extracted the doubled-opener-skip loop into its own
tag_close_skipping_doubled_openershelper: the orphan-closer check pushedprobe_decidedover clippy'stoo_many_lineslimit, and the loop was a natural, self-contained unit to pull out — no behavior change there,cargo testbefore and after the extraction is byte-identical output.Test plan
an_orphaned_closer_with_no_opener_anywhere_is_removed_not_shownreproduces the exact leaked sequence — fails without the fix, passes with it.cargo test -p tinytools-agent: 323 passed, no regressions (in particulara_doubled_opener_does_not_swallow_an_unrelated_closing_tagand the pipe/newline doubled-closer tests, which exercise the extracted loop, still pass unchanged).cargo fmt --all -- --check: clean.cargo clippy --all-targets --all-features -- -D warnings: clean.cargo build --all-targets --all-features: clean.