fix(backend): the recogniser had no idea what a string was - #62
Conversation
Three defects, all reachable from the UI, all one root cause: block extent
came from `line.count("{") - line.count("}")` and actions came from a regex
over raw text. Neither knows what a string or a comment IS. Reproduced all
three before touching anything:
1. `fileinto "Weird{Folder";` — the brace inside the STRING was counted, so
the block never closed and the NEXT rule was swallowed into it. One rule
came back carrying `subject is alpha` and BOTH actions. Mail matching
`beta` stopped being filed; mail matching `alpha` was filed twice.
2. `if A { if B { fileinto "X"; } }` came back as `if A { fileinto "X"; }`.
Condition B gone; the filing happened on A alone.
3. `# fileinto "Disabled";` came back as a live action, so commenting an
action out and reopening the editor RE-ENABLED it.
None of them tripped the RawBlock safety net, because the parser did not
fail. It succeeded, misread, and regenerated valid Sieve that routed mail
somewhere else. That is why this is the headline fix and not a bug report.
_LexicalMap borrows sievelib's LEXER — a `{` inside a string is part of one
`string` token, `# ...` is one `hash_comment` token — and gives the parser
per-line real-brace deltas plus a masked copy where comment bodies are
spaces. Same length, so every offset still lands where it did. Strings are
NOT masked; the regexes still need to read them.
Only the Lexer. sievelib's PARSER resets `RequireCommand.loaded_extensions`,
a class attribute, on every parse (parser.py:138), so it would need a
process-wide lock; the Lexer holds nothing across calls and a fresh one is
built per parse. Checked rather than assumed: 24,000 concurrent parses over
16 threads, 0 disagreements.
RECOGNITION DID NOT DROP — the acceptance criterion. Measured across all 60
fixtures, old parser vs new: rules 68 -> 68, raw 86 -> 82. One fixture moved,
`vendor/set-command.sieve`, from (0, 6) to (0, 2) — and it moved because it
contains `"${header}"`, so defect 1 was shattering a real third-party script
into six fragments. The corpus from `.3` found it; nothing else would have.
IMPORT LOCK. sieve_transform now imports from sievelib, which the `.9` lock
forbids, and that test says in terms: do not widen `_MAY_IMPORT`. So it is
not widened at the package. `_protocol_imports` now reports FULL dotted names
and a grant covers what it names and anything beneath it — every existing
entry keeps its exact meaning, and this one is written `{"sievelib.parser"}`.
Verified before granting: that module's whole import closure is re, sys,
typing, typing_extensions, collections.abc and two sibling sievelib modules.
It cannot open a socket, and `sievelib.managesieve`, which can, is not
granted. Mutation-checked — adding that import to sieve_transform fails the
lock and the message names who may.
WHAT THIS DOES NOT CLOSE, said plainly rather than left to be discovered: a
command we do not model is still dropped from a block we DO recognise.
`setflag "\Seen";` next to a `fileinto` regenerates with the setflag gone.
Nothing is mis-segmented there — it is the PROJECTION, which is `.11`. It is
pinned xfail(strict=True) naming that bead, so the day it is fixed this
suite says so.
`.11` will want to know whether a sievelib Parser gate costs recognition.
Measured: it rejects 0 of the 71 rules we recognise. That is a test, not a
sentence in this message.
Each of the three fixes mutation-checked separately; each mutation kills its
own tests and no others. 1113 tests, 4 xfailed, ruff clean.
Refs areyousievious-8fg.10
|
Claude finished @derekslenk's task in 12m 14s —— View job Code Review — PR #62
I traced the old vs. new block-collection logic against the changed fixtures — the 1.
2. The
3. Root Nice work on the PR overall — the fixture-driven acceptance criterion, the mutation-checked import lock grant scoped to |
Three findings from the review of #62. The first is a real regression I introduced, and I checked it rather than taking it on trust: json.loads(r'{"c": "keep;\ud800"}') -> 'keep;\ud800' old parse_sieve -> 1 entry new parse_sieve -> UnicodeEncodeError `SaveRawRequest.content` is a plain `str` and `json.loads` hands a lone surrogate through intact, so that reaches PUT /raw and comes back on the next GET as a 500. The character-counting parser this replaced never needed the text to be bytes at all. `text.encode()` ran BEFORE the try that exists to make exactly this fall back. Scanning is now `_scan`, guarded for the two ways the INPUT can be the problem — ParseError and UnicodeEncodeError — and nothing else. Second finding, same block: the old `except Exception` also covered the masking arithmetic. A bug there would have silently dropped the lexical model and let all three defects back in with no signal. That code now sits OUTSIDE the guard: it is our own arithmetic over tokens we already hold, and loud is the safer failure for it. Third: root AGENTS.md still said "The Sieve parser is hand-rolled (not sievelib)", which this branch made false. backend/AGENTS.md was updated and the root file was missed — precisely the drift that file exists to prevent. Both it and SieveParser's own docstring now draw the line where it actually falls: the LEXICAL questions are sievelib's, the PROJECTION is ours, and sievelib's AST and tosieve() remain unused. Both fixes pinned: the surrogate one by its own regression test, the narrow guard by a test that asserts _scan raises for each input problem and that only _scan is wrapped. 1115 tests, 4 xfailed, ruff clean. Refs areyousievious-8fg.10
|
Claude finished @derekslenk's task in 1m 54s —— View job Code Review — PR #62 (re-review after synchronize)
SummaryCommit 452b3d7 addresses all three findings from the prior review of this PR. Verified each against the current source: 1. Encode-before-try (regression, 500 on lone surrogates) — fixed. 2. Overly-broad 3. Root No new issues found in the fix commit — the exception narrowing is precise ( Nothing further to flag on this pass. |
1 participant
areyousievious-8fg.10 — the headline fix of the architecture-review epic. Unblocks
.11,.13and.15.Three defects, one root cause
Block extent came from
line.count("{") - line.count("}"); actions came from a regex over raw text. Neither knows what a string or a comment is. All three reproduced before touching anything:fileinto "Weird{Folder";then a second rulesubject is alphaand both actions — the second rule's condition goneif A { if B { fileinto "X"; } }if A { fileinto "X"; }— condition B gone# fileinto "Disabled";inside a blockfileinto "Disabled";Defect 1 means mail matching
betastopped being filed and mail matchingalphawas filed twice. Defect 2 means the inner action fired on the outer condition alone. Defect 3 means commenting an action out and reopening the editor re-enabled it.None of them tripped the RawBlock safety net, because the parser did not fail. It succeeded, misread, and regenerated valid Sieve that routed mail somewhere else. That is what makes this corruption rather than a bug anyone would have reported.
The fix
_LexicalMapborrows sievelib's Lexer — a{inside a string is part of onestringtoken,# ...is onehash_commenttoken — and gives the parser per-line real-brace deltas plus a masked copy where comment bodies become spaces. Same length, so every offset still lands where it did. Strings are not masked; the regexes still need to read them.Only the Lexer. sievelib's Parser resets
RequireCommand.loaded_extensions, a class attribute, on every parse (parser.py:138), so adopting it would need a process-wide lock. The Lexer holds nothing across calls and a fresh one is built per parse. Checked rather than assumed: 24,000 concurrent parses over 16 threads, 0 disagreements.Recognition did not drop
The acceptance criterion, measured across all 60 fixtures, old parser vs new:
One fixture moved:
vendor/set-command.sieve,(0, 6)→(0, 2). It moved because it contains"${header}"— defect 1 was shattering a real third-party script into six fragments. The corpus from.3found that; nothing else would have.The import lock
sieve_transformnow imports from sievelib, which the.9lock forbids — and that test says in terms "do not widen_MAY_IMPORT". So it isn't widened at the package._protocol_importsnow reports full dotted names, and a grant covers what it names and anything beneath it. Every existing entry keeps its exact meaning; this one is written{"sievelib.parser"}.Verified before granting: that module's entire import closure is
re,sys,typing,typing_extensions,collections.abcand two sibling sievelib modules. It cannot open a socket, andsievelib.managesieve— which can — is not granted. Mutation-checked: adding that import tosieve_transformfails the lock, and the message names who may.What this does NOT close
Said plainly rather than left to be discovered: a command we do not model is still dropped from a block we do recognise.
setflag "\Seen";next to afileintoregenerates with thesetflaggone. Nothing is mis-segmented there — it's the projection, which is.11. Pinnedxfail(strict=True)naming that bead, so the day it's fixed this suite says so.And
.11will want to know whether a sievelib Parser gate costs recognition. Measured: it rejects 0 of the 71 rules we recognise. That's a test in this PR, not a sentence in a commit message.Verification
1113 passed, 4 xfailed·ruff check+ruff format --checkclean · frontend untouched and greenLexer.posbehaviour this rests on is pinned by a test: every token's recorded offset must reproduce that token's bytesRefs areyousievious-8fg.10
🤖 Generated with Claude Code