Skip to content

fix/router-files-write — files:write through the router - #16

Merged
Kurtisone merged 9 commits into
mainfrom
fix/router-files-write
Aug 14, 2026
Merged

fix/router-files-write — files:write through the router#16
Kurtisone merged 9 commits into
mainfrom
fix/router-files-write

Conversation

@Kurtisone

@Kurtisone Kurtisone commented Aug 14, 2026

Copy link
Copy Markdown
Owner

Two preexisting bugs on main, found during the security lot 3 pre-merge
testing (so not lot 3 regressions). Together they made files:write from
chat fail for essentially any real file content.

Base: 1cdc26d. 553 tests green (baseline 514), ruff check +
ruff format --check clean, applied and verified with git am on a fresh
clone of main.

Diagnosis

1. The JSON object scanner counted braces without context.
_all_json_objects incremented on { and decremented on } including
inside string literals. But a router decision's content carries a
payload whose own content is file text — and real code has braces that
don't balance on their own (a truncated Go/C/Rust function, a } in a
comment, a Python dict). The depth either hit zero too early (unparseable
candidate) or never (scan abandoned), the object was dropped, and the
output fell through to the plain-text fallback.

hello.py worked — which is what masked the bug for so long — because
print('...') contains no brace at all.

2. The model does not hold double escaping.
Nesting a JSON payload inside a JSON string requires a second level of
escaping: \\n where a plain string needs \n. The 9B writes \n. The
outer parse then yields inner text carrying a raw newline, and the tool's
own json.loads dies on invalid control character.

The fixes

fix(router) — string-aware scanner. Brace counting now skips string
literals, escapes included. Fixed along the way: an unclosed { used to
break and make every later object unreachable (its closing brace only
ever brought the depth from 2 to 1, never to 0); the search now resumes
one character further on.

feat(router) + fix(router)content MUST be an object for
JSON-payload tools.
First attempt: content ::= string | object for
every tool. Not enough — both branches stayed reachable, and against a
9B's prior for the escaped-string shape, the worked examples lost.
Confirmed on the first real files:write, which came back as an escaped
string and died on the unescaped quotes in import "fmt".

The actual fix: the grammar conditions the shape of content on the tool,
which GBNF can express because tool is pinned before content. root
splits into payload-call (object content, for
files/memory/review/sysadmin) and text-call (string content, for
chat/code and the rest). A branch with no tools is omitted (an empty
alternation is unsatisfiable).

This is why the constraint has to live in the grammar. In the string
shape, the file body needs double escaping that the grammar cannot
check — to schar, the whole payload is just characters. In the object
shape, the body is an ordinary JSON string, so schar (which excludes a
bare " and control characters) enforces its escaping during sampling.
The failure stops being caught after the fact and becomes impossible to
produce.

feat(router) — re-encoding mechanics. The parser re-encodes an
object with json.dumps, so RouterDecision.content stays a string and
no run(content: str) contract changes: tools still parse JSON text,
produced by Forge instead of the model. The object rule pulls in full JSON
values, so a numeric payload field doesn't have to be stringified to
satisfy sampling. The parser still accepts the string shape — providers
without GBNF have no other option.

feat(router) — the prompt teaches the object shape. A small local
model imitates its worked examples far more than it reads descriptions: as
long as they showed an escaped string, that is what it produced. All four
JSON-payload tools (files, memory, review, sysadmin) switch
together — two competing shapes side by side in the same prompt is how
routing gets unstable on a 9B (same lesson as the review/files
ambiguity in v3.10). The hello.py example becomes multi-line: it only
ever exercised the one case that was never the problem.

fix(router) — hyphenated rule names. The tool-conditioned grammar
was structurally valid, passed every test, and was rejected outright by
llama-server: expecting newline or end at _call. llama.cpp lexes rule
names with is_word_char(), which accepts [a-zA-Z0-9-] and not
underscore — so payload_call reads as the rule payload followed by
garbage. The result was a 400 on every completion: the router did not
degrade, it died (16 ms, no routing at all). The real gap was in the
tests, which only asserted the generated text — precisely the blind
spot. is_word_char() is now reimplemented in the test suite and every
rule name checked against it, verified by reverting the rename and
watching the test fail.

fix(tools) — shared lenient JSON loader. The string shape does not
go away (grammar disabled, a provider without GBNF, model drift).
forge/tool_payload.loads_payload() retries with strict=False,
deliberately as a second attempt: strict parsing succeeding is the
signal that the model is producing correct JSON, and defaulting to lenient
would hide the next escaping regression instead of surfacing it. The
recovery logs a warning and an event. Shared across all four tools rather
than fixed in files.py alone — this repo has twice been bitten by fixing
one copy of a shared behaviour and letting its twin diverge (review vs
research, see text_cleaning.py).

feat(files) — echo the content back when a write creates a file.
Modifying a file returned a real diff; creating one returned a byte count
and nothing else, so after "create hello.go" the content was never shown
anywhere and had to be opened by hand. A new file now comes back in a
fenced block (language hinted from the extension, capped at 4 KB with a
truncation notice). Beyond the display win, this puts the content in the
conversation, where a follow-up turn has something to refer back to.

feat(files) — one-step edit action. "Replace X with Y" was the
last file operation that still needed the router to chain: read with
done:false, then write the whole file back. Observed live, the run
stopped after the read and answered with the file's ORIGINAL content —
exactly the symptom the read example had been added to fix in v3.9. This
is the same non-chaining that already forced deterministic handling twice
(web_search in v3.10, memory:recall in fix/memory-recall); betting
on done:false a third time was not reasonable.

{"action":"edit","path":...,"find":...,"replace":...} does the
replacement in one dispatch. Beyond removing the chaining, it removes the
file content's round trip through the model entirely: the model supplies
two short strings and never has to reproduce a file it just read — which
is where a 9B quietly "fixes" things along the way (the v3.9 hallucination
bug). A find with no match is an error, not a silent no-op, so the model
can fall back to read-then-write for a change that isn't literal. edit
is confined to WORKSPACE_DIR like read and write, with its own test: a
new action is a new chance to reintroduce the v3.10 escape.

feat(ui) — label code blocks with their language. The fence tag was
parsed only to switch on diff; every other block rendered as a bare
<pre><code>. Blocks looked right, but nothing said whether they held Go
or Python — and now that a write echoes new file content back, that
happens on every file creation. A small badge in the corner of the block
shows the tag, displayed as written rather than mapped to a pretty name
(files.py already owns the extension → tag mapping; a second display map
would be one more copy to drift). renderDiff takes the badge as an
argument instead of building its own, so the two block kinds can't diverge
in how they label themselves.

Out of scope, fixed in passing

memory.run() called .get() straight off the parse, so a valid but
non-object JSON payload ("recall", a list) raised AttributeError out
of a tool whose contract is to return its errors as text. files,
review and sysadmin already guarded this.

Verification

End to end outside the tests, router → files.run() → file on disk:

content before after
balanced Go object found, write failed written
truncated JS (() => {) 0 objects, text fallback written
C with an extra } 0 objects, text fallback written
Python dict object found, write failed written
under-escaped string shape invalid control character written (with a warning)
genuinely malformed payload error error (unchanged)

The tests assert the invariant rather than the escaping they kept breaking
on: every worked example in the prompt must parse into the tool it
names
, and no JSON-payload example may re-encode its payload as a
string. A malformed future example now breaks a test instead of teaching
the model to fail.

Real-world validation

Three iterations were needed, each exposing a different cause:

run result cause
#76cccf2f failure content ::= string | object left the string shape reachable; the model took it and died on import "fmt"
#4862ec0e failure llama-server rejected the grammar (underscore in rule names) — 400 on every completion
#be63eb8d success hello.go written, import "fmt" intact
#127a948d success test.go created, content shown in a fenced block
#fa53a1e6 success replacement routed to files:edit in one step, correct diff

The router picks edit over a read, and extracts the literal string
from a natural-language sentence.

Known limitation, accepted

edit covers literal replacement, which is the common case. A
non-literal change — "add a function", "restructure this file" — still
depends on read→write chaining, i.e. on the behaviour that failed in run
#00b1c5f8. That path and the steering hint after a files:read are kept
for those cases.

The logical next step if it becomes a problem: a deterministic edit
graph modelled on research and recall — read, have the LLM produce
only the changed portion, write. That's a separate piece of work, not a
patch.

_all_json_objects counted { and } blindly, including braces inside
JSON string literals. A router decision whose nested payload carried
real file content -- a Go/C/Rust/JS snippet, a Python dict, anything
with braces that don't balance on their own -- either closed early
(unparseable candidate) or never closed (scan abandoned), so the
object was dropped and the output fell through to the plain-text
fallback. files:write therefore failed for most real file bodies;
hello.py worked only because print('...') has no brace.

Brace counting now skips string literals, escapes included. An
unclosed "{" also resumes the search one char later instead of
giving up on the rest of the text: with depth counting, a later
complete object was unreachable once an earlier brace never closed.

Found during the security lot 3 pre-merge tests; preexisting on main,
not a lot 3 regression.
Nesting a tool payload inside a JSON *string* requires double
escaping -- \\n where a plain string needs \n -- and the 9B model
does not hold it on multi-line content. The outer parse then yields
inner JSON text carrying a raw newline, and the tool's own
json.loads dies on an invalid control character. Combined with the
brace-scanning bug, files:write through the router failed for
essentially any real file body.

"content" may now be a real nested object. The parser re-encodes it
with json.dumps, so RouterDecision.content stays a string and every
tool's run(content) contract is untouched -- they still parse JSON
text, just text this module produced rather than the model. The
grammar gains the object alternative (with full JSON values, so a
payload field holding a number or a list doesn't have to be
stringified to satisfy sampling): the wrong shape becomes
unreachable during decoding rather than caught afterwards.

The string shape stays accepted -- grammar-disabled setups, other
providers, and model drift all still produce it.
The parser now accepts an object, but a small local model imitates
its worked examples far more than it reads descriptions -- so as long
as the examples showed an escaped string, that's what it kept
emitting, double escaping and all. All four tools whose content is
itself JSON (files, memory, review, sysadmin) switch together: two
competing shapes side by side in the same prompt is how routing gets
unstable on a 9B, and the parser re-encodes an object back to JSON
text anyway, so each tool's run(content) sees exactly what it did.

The write example also becomes multi-line. hello.py was the only
write that ever worked, and only by accident: a lone print() has no
brace to lose the parser's scanner and no newline to escape. The
example now shows the body shape that actually failed.

Tests assert the invariant rather than the escaping they kept
breaking on: every worked example must parse into the tool it names,
and no JSON-payload example may re-encode its payload as a string.
Belt and braces for the escaped-string shape, which does not go away
just because the prompt now teaches objects: grammar-disabled setups,
providers without GBNF sampling, older fine-tunes and plain drift all
still produce it, and in that shape the model writes \\n where \\\\n
was needed. The payload is otherwise perfectly good -- only its
escaping is wrong, and only in a way that is unambiguous to recover
from.

New forge/tool_payload.loads_payload() retries with strict=False,
deliberately as a SECOND attempt: strict parsing succeeding is the
signal that the model produced correct JSON, and defaulting to
lenient would hide the next escaping regression instead of surfacing
it. The recovery warns and logs an event, so a run that needed it is
visible even though it worked.

Shared by all four JSON-payload tools rather than fixed in files.py
where it was observed -- the failure applies verbatim to the other
three, and this repo has twice been bitten by fixing one copy of a
shared behaviour and letting its twin diverge (review vs research,
see text_cleaning.py).

Also closes a preexisting crash path found while touching that line:
memory.run() called .get() straight off the parse, so a valid but
non-object payload ('"recall"', a list) raised AttributeError out of
a tool whose contract is to return errors as text. files, review and
sysadmin already guarded this.
The previous commit offered "content ::= string | object" to every
tool and claimed that made the wrong shape unreachable. It did not:
both branches stayed reachable for every tool, so nothing but the
worked examples pushed the model toward the object -- and against a
9B's prior for the escaped-string shape, the examples lost. The first
real files:write confirmed it, coming back as an escaped string that
died on the unescaped quotes in `import "fmt"`.

That payload is not recoverable after the fact, and that is why the
constraint has to live in the grammar. In the string shape the file
body needs DOUBLE escaping and the grammar cannot check it -- to
schar the whole payload is just characters -- so under-escaped quotes
terminate the string early and the result is genuinely ambiguous. In
the object shape the body is a plain JSON string, so schar (which
excludes bare " and control characters) enforces its escaping during
sampling. The failure stops being caught and becomes impossible.

GBNF can condition on this because "tool" is pinned before
"content": root now splits into payload_call (object content, for
files/memory/review/sysadmin) and text_call (string content, for
chat/code and the rest). A branch with no tools is omitted, since an
empty alternation is unsatisfiable and a single-kind ENABLED_TOOLS is
legal. JSON_PAYLOAD_TOOLS lives in forge/tool_payload.py as the one
source of truth for grammar, prompt examples and tests.

The parser still ACCEPTS the string shape: providers without GBNF
sampling have no other option.

Also fixes the rule-reference test that should have caught this
class of change: it scanned bare identifiers without stripping string
literals, and passed only because `tool` and `content` were rule
names as well as JSON keys. It now strips literals and char classes
first, which also retires the hand-maintained keyword list.
The tool-conditioned grammar was structurally valid, passed every
test, and was rejected outright by llama-server:

  parse: error parsing grammar: expecting newline or end at _call

llama.cpp lexes rule names with is_word_char(), which accepts
[a-zA-Z0-9-] and NOT "_". `payload_call` therefore lexes as the rule
`payload` followed by garbage, and the whole grammar is refused --
so every completion came back 400 and the router stopped working
altogether. It did not degrade, it died: 16ms, no routing at all.

Rule names are now hyphenated, matching every grammar llama.cpp ships.

The real gap was in the tests, not the names. Nothing here could
catch this: the generated text is well-formed by every structural
measure, and there is no llama.cpp parser in this environment to
reject it -- these tests only ever asserted the grammar TEXT, which
is precisely the blind spot. is_word_char() is now reimplemented in
the test suite and every rule name is checked against it, verified by
reverting the rename and watching it fail.
Modifying a file returned a real diff; creating one returned a byte
count and nothing else, so after "crée hello.go" the content was
never shown anywhere and had to be opened by hand to be seen.

A new file now comes back in a fenced block (language hinted from the
extension, capped at 4 KB with a truncation notice). Beyond the
obvious display win, this puts the content in the conversation, where
a follow-up turn -- "now replace X with Y" -- has something to refer
back to instead of an empty acknowledgement.

Modification is untouched: it still diffs, which is the point of not
re-showing a file that mostly didn't change.
"Remplace Hello World par Bonjour à tous" was the last file
operation that still needed the router to chain: read with
done:false, then write the whole file back. Observed live, the run
stopped after the read and answered with the file's ORIGINAL content
-- exactly the symptom the read example had been added to fix in
v3.9.

This is the same non-chaining that already forced deterministic
handling twice: web_search in v3.10 and memory:recall in
fix/memory-recall. Betting on done:false a third time was not
reasonable.

{"action":"edit","path":...,"find":...,"replace":...} does the
replacement in one dispatch. Beyond removing the chaining, it removes
the file content's round trip through the model entirely: the model
supplies two short strings and never has to reproduce a file it just
read -- which is where a 9B quietly "fixes" things along the way
(the v3.9 hallucination bug).

A find with no match is an error, not a silent no-op, so the model
can fall back to read-then-write for a change that isn't literal --
that path is untouched, and the steering hint after a files:read
stays for it. Edit is confined to WORKSPACE_DIR like read and write,
covered by its own test: a new action is a new chance to reintroduce
the v3.10 escape.
The fence tag was parsed only to switch on "diff"; every other block
rendered as a bare <pre><code>. Blocks looked right but nothing said
whether they held Go or Python -- and now that files:write echoes new
file content back, that happens on every file creation.

A small badge in the corner of the block shows the tag. It sits
inside the <pre> rather than above it, so a long line scrolling
horizontally can't drag it out of view and a block with no tag
doesn't shift anything.

The tag is displayed as written rather than mapped to a pretty name:
files.py already owns the extension -> tag mapping, and a second
display map here would be one more copy to drift out of sync (see
review.py vs research.py in v3.10). It is escaped anyway -- \w+
can't carry markup, but the XSS hardening from lot 1 exists because
one interpolation trusted its input.

renderDiff takes the badge as an argument instead of building its
own, so the two block kinds can't diverge in how they label
themselves.
@Kurtisone
Kurtisone merged commit cb4f305 into main Aug 14, 2026
2 checks passed
@Kurtisone
Kurtisone deleted the fix/router-files-write branch August 14, 2026 16:21
Kurtisone added a commit that referenced this pull request Aug 15, 2026
PR-fix-router-files-write.md and its English version are the message
written for PR #16, not part of the project. They rode along in four
commits of that branch and reached main unnoticed.

Nothing references them -- no code, no docs, no CI -- so a plain
removal is enough; main is already pushed and a history rewrite would
cost more than the two files are worth. They stay in the history, they
just leave the tree.

.gitignore already carried LINKEDIN_POST.md for exactly this reason:
notes written for a one-off audience rather than for the repo. That
entry was a name, not a rule, so the next artifact of the same kind
matched nothing. Anchored /PR-*.md added so the next PR write-up
cannot repeat this.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant