fix(tools): refuse control markers and unknown keys, announce and restore a replaced user file (F36, F37, F40) - #438
Open
plombeer31 wants to merge 4 commits into
Open
plombeer31 wants to merge 4 commits into
plombeer31 wants to merge 4 commits into
Conversation
…, not run
Live, Gemma 4 31B under the GBNF grammar: the first call of a turn was
`os.fs.list {"path": ".}}]<tool_call|>thought<|channel>thought---…"}`.
The model opened another thought channel mid-call; the grammar admits
those bytes only inside a JSON string, so the markers landed in `path`.
The tool ran on the garbage (ENAMETOOLONG), the model read "the folder
is empty" and overwrote the user's input file.
- control-marker-guard: `findControlMarkers` walks every string argument
(nested objects and arrays included) for Gemma 4, Qwen/ChatML and
Gemma 3 markers plus the generic `<|name|>` form, one exported list
with a comment per family. Paths, commands, patterns, URLs and ids are
flagged on any occurrence; the writing tools' content arguments
(`os.fs.write.content`, `os.fs.edit.oldString/newString`,
`os.fs.patch.patch`) only when a marker starts a line — the line F24's
write-time check draws — so source that mentions `<think>` in a
comment is not corrupted. A patch line's start sits after its `+`/`-`
prefix.
- batch-executor: a flagged non-terminal call never reaches the
registry; its slot gets an error result naming the argument, marker,
offset and a short excerpt, with `details.corrupted` and the hits, so
the loop tracker counts it like any error and the trace row carries
it. No parse-recovery budget is spent: the model reads the result on
its next step. Terminals stay exempt — a reply is shown, not run.
- build-grammar: the reasoning profiles' string body excludes `<|` and
`|>` through a three-state rule over the last character (the naive
`"<" [^|] | "|" [^>]` shape lets `<<|` and `||>` through); the plain
profile's grammar stays byte-identical to the file. Pinned with a
small test-side GBNF interpreter that reads the emitted grammar.
…reversible (os.fs.restore)
Live, Gemma 4 31B, 2026-09-15: the model's first step wrote
`projects.json` over the user's data file without listing the folder —
the result said "(replace)" and the model did not react — and after a
corrupted listing it wrote a 9-row `sales.csv` over the user's 2,401-row
dataset. Both files were named by the request as inputs; both were
unrecoverable afterwards.
Warn-only throughout: the write still lands. What changes is that the
previous content is saved first and the result says what happened.
- fs-restore-store: `<stateDir>/restore/<sessionId>/` holds the copies
(`<n>-<basename>`, last 20 per session, files up to 5 MB) and a
`manifest.json` with the copy index and the set of paths this session
created (a write to a path that did not exist). On disk rather than in
`SessionState` because the tools consult it BEFORE a write and a tool
sees only its `ToolContext`; keyed by session id, so a resumed session
still knows what it created. Built by `registerOsTools` from `stateDir`
and shared by write / edit / patch / restore through
`FsDangerousToolOptions.restore`; absent (embedders, tests) turns the
guard off.
- fs-replace-guard: before `os.fs.write` replaces a file that existed and
was not created this session, the previous bytes are copied and a line
is prepended to the result. Loud — `⚠ replaced the user's file
\`sales.csv\` (2,401 lines → 10, header changed); the previous content
is saved — \`os.fs.restore {"path":"sales.csv"}\` brings it back` —
when the line count dropped by ≥ 80 % (integer: dropped/before ≥ 4/5)
OR, for .csv .tsv .json .jsonl .md .txt .yaml .yml .toml .ini, the
first line changed, OR the file was over 5 MB (announced, not read or
saved). Any other replacement of a pre-existing user file gets the
quiet `replaced the user's file \`x\` (N lines → M); previous content
saved`. An empty file and a file the agent created are silent.
`os.fs.edit` / `os.fs.patch` run the same guard on the shrink rule
only (`shrank the user's file …`); a patch that creates a file marks
it as the agent's. The "(replace)" wording now carries the counts:
`(replace, 2,401 lines → 10)`, `(replace, new file, 10 lines)`.
- os.fs.restore { path }: writes the newest saved copy back, approval-
gated like a write on the fs ladder, and names the bytes and lines
that came back; the copy stays so a second restore still works.
Registered as the other os.fs tools are — descriptor (rare), args
schema, grammar os-tool list, `approval_gated` + `FS_WRITE_CATEGORIES`,
builder role, the stable-prefix solo list, the write-tool sets of the
loop detector and the fusion worker report — and the `os.fs.write`
descriptor says a replaced pre-existing file can be brought back.
- fs-edit-diff / fs-patch-preview: the diff renderer and the dry-run
half split out so both tools stay under the 300-line cap.
…t run with them dropped
… a replaced user file (F36, F37, F40) One subsection under "Parallel tool calls per step" for the three checks the tool runner now makes before a call reaches the registry or right after a write lands: a control marker inside a string argument refuses the call (F37), an argument key the schema does not know refuses the call with the keys received, expected and nearest (F40), and a write that replaces a pre-existing user file saves the previous content and says so, with os.fs.restore to bring it back (F36). The approval-ladder row lists os.fs.restore beside the other fs writes.
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.
What
Before: a local model opened a thought channel mid-call and the markers landed inside
path(os.fs.list {"path": ".}}]<tool_call|>thought<|channel>…"}); the tool ran on the garbage, the model read "the folder is empty" and overwrote the user's input; a worker sentos.shell.run {"cmd":"python3","-e":"<script>"}— the unknown key was dropped,python3ran with no arguments (exit 0) and the worker reported the work done; a write over a pre-existing user file said "(replace)" and nothing else, and nothing could bring the file back.After:
findControlMarkersrefuses a non-terminal call whose string arguments carry Gemma/Qwen/ChatML control markers (content arguments only when a marker starts a line), and the reasoning profiles' grammar excludes<|/|>from string bodies;findUnknownArgumentsrefuses a call with top-level keys the tool's schema does not know, naming the keys received, expected and the nearest match, after the key normalisation has had its chance, andos.shell.runsays(ran with no arguments); beforeos.fs.writereplaces a file that existed and was not created this session the previous bytes are saved under the state dir and the result says so — loud when the file shrank by 80 % or more, a data file's first line changed or the file is over 5 MB — andos.fs.restore { path }brings the newest copy back, approval-gated like a write. AGENTS.md notes for all three.Why
A key the tool does not know is a call the model did not mean; a control marker in a path is a corrupted call — refusing costs one step, running costs the task. The replace guard is warn-only so the write still lands, but the user's file is no longer gone. Both refusals name what to re-emit so a slow model pays one regeneration.
How it was verified
npm run lintcleannpx vitest run --minWorkers=1 --maxWorkers=3 src/agent src/llm/grammar src/llm/provider/openai src/prompt src/tools src/tools/fusion src/tools/os— 203 files / 2706 tests green, 3 skipped (baseline failures: none)replaced the user's file … 2,401 lines → 9, header changed) with the original saved; a delegate call with a stray top-level key was refused naming the expected keys and re-emitted correctlyStacked on #437; merge in order.