Skip to content

fix(agent): treat a fence whose info string is a call tag as a call - #30

Merged
senamakel merged 4 commits into
tinyhumansai:mainfrom
M3gA-Mind:fix/tag-on-fence-line
Sep 28, 2026
Merged

senamakel merged 4 commits into
tinyhumansai:mainfrom
M3gA-Mind:fix/tag-on-fence-line

Conversation

@M3gA-Mind

@M3gA-Mind M3gA-Mind commented Sep 28, 2026 •

Copy link
Copy Markdown
Contributor

Stacked on #29. This branch contains #29's commits (fdb223f, 32d6eb4) followed by this PR's own commits (396e15c, cc7107b). Please review only those two. Once #29 merges I'll rebase this onto main.

Summary

DeepSeek V4 Flash put the call opener on the fence line itself, ```<tool_call>, and never closed the fence. protected.rs read <tool_call> as the fence's language. It isn't in TOOL_CALL_LANGUAGES, so the unclosed fence protected everything to the end of the text as an example, and the call was dropped.

With this change, a fence whose info string starts with a complete call tag is a call fence and is not protected. A call tag here means a tag-family opener (<tool_call>, <|tool_call|>, DSML and attribute forms), or an <invoke>, bare or named, optionally DSML-prefixed. The predicate tagged::opens_with_call_tag uses TAG_RE plus one narrow FENCE_INVOKE_RE, anchored at offset 0. After review (cc7107b), <function …>, <function=…> and XML-namespaced tags are deliberately excluded: ```<xsl:function name="f"> is code, and with the broader grammar regex it dispatched f. The grammars then scan the fence line as they would anywhere else. The body is decoded by #29, and the earlier stray </tool_call> in the same turn is swept by the existing orphan-closer handling.

A language-tagged fence still protects its contents: ```xml, ```xml<tool_call> and ```text <tool_call> all stay examples. Bare ``` fences were already unprotected by design (protected.rs module docs), and this PR doesn't change that.

Related issue

Refs tinyhumansai/openhuman#6722. Together with #29 this covers the reproduction. Verified against the real user record via the harness-options probe (counts only): the record now yields exactly 1 call (InvokeXml), and narration-only records still yield 0.

API or behavior changes

A fence whose info string opens with a call tag is no longer reported by fence_ranges / open_fence_start, so calls inside it parse. There is no public signature change.

Validation

Commands actually run, with their outcome:

  • cargo fmt --all -- --check: clean
  • cargo clippy --all-targets --all-features -- -D warnings: ran without --all-features (cargo clippy --all-targets -- -D warnings): clean
  • cargo build --all-targets --all-features: not run with --all-features; the default-feature build via clippy/test succeeded
  • cargo test --all-features: ran cargo test (default features): all pass (113 + 334 + 20 + 1)

Also ran RUSTDOCFLAGS="-D warnings" cargo doc --no-deps: clean.

Tests

Added to src/parse/test/engine.rs:

  • an_unclosed_fence_whose_info_string_is_a_call_tag_is_a_call: a sanitized shape of the real turn (a <todos> block, a stray </tool_call>, then an unclosed ```<tool_call> with a wrapped invoke)
  • a_fence_whose_info_string_is_a_named_invoke_is_a_call
  • a_closed_fence_whose_info_string_is_a_call_tag_is_a_call
  • a_language_fence_still_protects_a_call_tag_example: controls for ```xml, ```xml<tool_call>, ```text <tool_call>, ```<function name="f">, ```<xsl:function name="f"> and ```<function=shell>. The last three dispatched with the pre-review predicate: f(command="rm -rf /") was the first failure.

Red check: with #29 applied and this change absent, the first three fail on calls.len() == 1, and the control passes.

Documentation

I updated the protected.rs module docs to list the new exception.

Checklist

  • The change is focused on one logical change
  • No new #[allow(...)], #[ignore], or relaxed lints
  • No secrets, tokens, or .env contents in the diff or the description

Review status: tinysweeper/review timed out at the current head (900s, 'No code was reviewed'). It is advisory, not a required check. tinysweeper APPROVED the earlier head 0b100fa, and the fleet reviewer reviewed every head.

Told to call tools inside <tool_call> tags, DeepSeek V4 writes its
native invoke XML there. The tagged grammar claimed the block, found no
JSON body, and dropped the call as malformed, though the same invoke
parses bare. When a tag body opens with a named invoke, decode it with
invoke_xml. The anchor keeps an invoke quoted inside other body text
from executing.

Refs tinyhumansai/openhuman#6722
@tinysweeper

tinysweeper Bot commented Sep 28, 2026 •

Copy link
Copy Markdown

Tiny Sweeper review

Tiny 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
Priority: none
Reviewed head: cc7107b0d518
Updated: 1790600034 (Unix time)

Review snapshot

Change surface Files Review signal Count
Production 3 Active findings 0
Tests 2 Noted findings 0
Documentation 0 Resolved findings 0
Configuration 0 Pending checks/questions 0

Completeness: Complete
Test assessment: No supported feature-to-test mapping was available; this does not mean tests are absent or passed.

What changed

The review could not produce a supported behavioral summary; inspect the cited changed surface and lane details below.

Features

None identified with supported citations.

Tests

No supported feature-to-test mapping was produced. Test execution is not inferred.

Findings

No active actionable findings.

Before merge

None.

How this fits together

flowchart LR
  n0["find_re<br/>changed"]:::changed
  n1["scan_fences<br/>changed"]:::changed
  n2["...anges_cover_languages_and_unclosed_fences<br/>changed"]:::changed
  n3["probe_decided"]:::impacted
  n4["len"]:::impacted
  n5["fence_ranges"]:::impacted
  n6["next_opener"]:::impacted
  n7["decode_arguments"]:::impacted
  n8["is_closing_marker"]:::impacted
  n1 -->|calls| n4
  n2 -->|calls| n5
  n2 -->|tests| n5
  n3 -->|calls| n4
  n3 -->|calls| n6
  n5 -->|calls| n1
  n6 -->|calls| n0
  n6 -->|calls| n4
  n6 -->|calls| n8
  n7 -->|calls| 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
Loading
Agent review details

critique

  • Conclusion: Success
  • Scope reviewed: all assigned evidence
  • Lane summary: Reviewed 4 files; 0 findings. _The code index is behind this pull request (indexed at `10622c4fd911`), so retrieved context may be out of date._ _2 memory call(s) failed (model: cortex: v1/answer: timed out after 20s), so this review saw part of what the engine holds._

security

  • Conclusion: Success
  • Scope reviewed: all assigned evidence
  • Lane summary: Reviewed 4 files; 0 findings. _The code index is behind this pull request (indexed at `10622c4fd911`), so retrieved context may be out of date._ _2 memory call(s) failed (model: cortex: v1/answer: timed out after 20s), so this review saw part of what the engine holds._

tests

  • Conclusion: Success
  • Scope reviewed: all assigned evidence
  • Lane summary: The change correctly treats a fence whose info string is a call tag (e.g. ```<tool_call>) as a call rather than a protected example, matching an observed model output from DeepSeek V4 Flash. The new `decode_body` function in `invoke_xml.rs` and the registered `opens_with_call_tag` predicate work together to dispatch both the fence-line call-tag case and invoke-XML inside `<tool_call>` tags. The added tests cover the fence-line patterns, the wrapped-invoke decoding, and several negative cases (language fences, quoted invokes). No behaviour regression is introduced and the test coverage is adequate for the stated problem. _The code index is behind this pull request (indexed at `10622c4fd911`), so retrieved context may be out of date._ _2 memory call(s) failed (model: cortex: v1/answer: timed out after 20s), so this review saw part of what the engine holds._

commits

  • Conclusion: Neutral
  • Scope reviewed: all assigned evidence
  • Lane summary: Nothing sensitive found in what this pull request commits.

description

  • Conclusion: Success
  • Scope reviewed: all assigned evidence
  • Lane summary: The PR correctly treats a fence whose info string is a call tag (e.g., ` ```<tool_call>` ) as a call rather than a protected example, and adds support for decode invoke XML inside `<tool_call>` tags. The code is consistent with the repository's rules, the new functions are well-tested, and no defects are introduced. Safe to merge. _The code index is behind this pull request (indexed at `10622c4fd911`), so retrieved context may be out of date._ _2 memory call(s) failed (model: cortex: v1/answer: timed out after 20s), so this review saw part of what the engine holds._

e2e

  • Conclusion: Neutral
  • Scope reviewed: all assigned evidence
  • Lane summary: No end-to-end harness in this repository: no e2e test files and no e2e workflow.
Evidence and run details
  • Models: ladder/vectors, gpt-5.6-luna, deepseek/deepseek-v4-flash, deepseek-v4-flash
  • Spend: $0.028740
  • Tokens: 241384 input · 15154 output · 2032 cached · 889 embedding
Head State Pass summary
0b100fac3a4e ready for maintainer review 0 active finding(s), 0 resolved finding(s) (at 1790594563)
cc7107b0d518 ready for maintainer review 0 active finding(s), 0 resolved finding(s) (at 1790600034)

tinysweeper 0.1.0

@coderabbitai

coderabbitai Bot commented Sep 28, 2026 •

Copy link
Copy Markdown

Important

  • 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: a348863e-3294-4e49-8bda-0e07bcc924e0


Comment @coderabbitai help to get the list of available commands.

@tinysweeper tinysweeper Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

tinysweeper found nothing blocking. Approving.

             $0.0250 · 174,895 in / 20,538 out · 1,901 cached (1%) · ladder/vectors, gpt-5.6-luna, deepseek-v4-flash, deepseek/deepseek-v4-flash · 804 embedded
critique:    $0.0107 · 62,287 in  / 4,978 out  · 0 cached (0%)     · gpt-5.6-luna
security:    $0.0115 · 84,412 in  / 2,140 out  · 1,901 cached (2%) · gpt-5.6-luna
tests:       $0.0005 · 15,810 in  / 6,974 out  · 0 cached (0%)     · deepseek-v4-flash
description: $0.0014 · 7,358 in   / 4,365 out  · 0 cached (0%)     · deepseek/deepseek-v4-flash

@M3gA-Mind

Copy link
Copy Markdown
Contributor Author

Review at 0b100fa (comment only), focused on whether a fenced EXAMPLE can become executable. I ran a temporary probe (not committed) over 17 fence info strings, each ending in an rm -rf / shell invoke. For each one I compared batch parse_tool_calls, a one-fragment StreamScrubber, and a one-char-per-fragment StreamScrubber, at this head and at #29's head (fdb223f, without the fence change).

fence info string #29 batch / stream-per-char #30 batch / stream-per-char
xml, html, text <tool_call>, <div>, <div> + <tool_call> on the next line, <tool_calls>, <function_calls>, </tool_call> 0 / 0 0 / 0 (still protected, example text shown)
<tool_call>, <TOOL_CALL>, <tool_call example="true">, <function=shell> 0 / 0 1 / 1 (intended)
``` <tool_call> (space), ~~~<tool_call> 0 / 1 1 / 1
<xsl:function name="f">, <function name="f"> 0 / 0 1 / 1

All four shapes on your list stay protected, in batch and in both stream modes. Batch and stream agree on every row at this head.

Bonus fix worth a test: at #29's head, ``` <tool_call> and ~~~<tool_call> gave 0 calls in batch but 1 when streamed char by char, so the two parsers disagreed about the same text. This PR makes them agree. A stream_matches_batch case for a split ```<tool_call> opener would pin that.

One widening to consider (low severity): opens_with_call_tag reuses NAMED_INVOKE_OPEN_RE. That regex also accepts <function name="…"> and any ns: prefix. So ```<xsl:function name="f"> (XSLT) and ```<function name="f"> now go from protected to a call named f. Real models rarely put code on the fence line, and an unknown name only yields UnknownTool. Still, the stated rule is "the info string is a call TAG". If the fence check only needs the DeepSeek forms, restrict it to TAG_RE plus <invoke …>/DSML, not function and not arbitrary namespaces. Otherwise, add a test that pins these as intended.

Accepted by design, just noting it: a model that genuinely quotes a call as ```<tool_call>\n<invoke name="shell">… in an explanation will now execute it. That is the trade-off this PR makes, and the tests document the xml/text escape hatch.

The tagged.rs/invoke_xml.rs hunks are #29's commit (fdb223f), and my #29 review applies to them unchanged.

Adds a closed JSON tag body quoting an invoke (fails without the anchor)
and a <tool_call><function=…> body (dropped on main). Narrows the
decode_body doc to what the anchor guarantees: only the first invoke is
anchored, as on the bare path.
DeepSeek V4 Flash wrote the opener as the fence info string,
```<tool_call>, and never closed the fence. The info string read as a
language, so the unclosed fence protected the call to end of text as
an example and it was dropped. A fence whose info string opens with a
complete call tag (tag-family opener, named invoke, bare <invoke>) is
now a call fence. A language-tagged fence still protects its contents.

Refs tinyhumansai/openhuman#6722
opens_with_call_tag reused NAMED_INVOKE_OPEN_RE, which also accepts
<function …> and any XML namespace, so an XSLT fence such as
```<xsl:function name="f"> became a call. Only a tag-family opener or
an (optionally DSML-prefixed) <invoke> now marks a call fence.
@M3gA-Mind
M3gA-Mind force-pushed the fix/tag-on-fence-line branch from 0b100fa to cc7107b Compare September 28, 2026 11:33
@M3gA-Mind

Copy link
Copy Markdown
Contributor Author

Rebased onto #29's 32d6eb4; the widening is addressed in cc7107b. The fence check now accepts only TAG_RE openers plus an <invoke> (bare or named, optionally DSML-prefixed) via a narrow FENCE_INVOKE_RE. There is no function and no arbitrary ns:. I added <function name="f">`, <xsl:function name="f"> and ````<function=shell> as protected controls. With the previous predicate the first of them dispatched f(command="rm -rf /"). Note that ````<function=shell>moves from 'intended call' in your table to protected, which is deliberate: the fence-line rule is now only the DeepSeek forms. I didn't add thestream_matches_batch` case for the split opener; it's a separate change if wanted.
fmt, clippy `-D warnings`, doc and `cargo test` (334 lib tests) are clean.

@M3gA-Mind

Copy link
Copy Markdown
Contributor Author

Re-checked at cc7107b with the same 17-case probe (batch, 1-fragment stream, 1-char stream). ```<function name="f">, ```<xsl:function name="f"> and ```<function=shell> are protected again (0 calls). Everything on the original list (xml, html, text <tool_call>, <div>, <tool_calls>, <function_calls>, </tool_call>) is still protected, <tool_call> fences still dispatch, and batch and stream agree on every row. The three new rows in a_language_fence_still_protects_a_call_tag_example pin the narrowing. 335/335 locally; CI is green at this head.

@senamakel
senamakel merged commit 8475ba1 into tinyhumansai:main Sep 28, 2026
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants