docs: say what these three functions do not check, and fix the preset mirror that drifted (#599, #600, #608) - #622
Merged
Merged
Conversation
… mirror that drifted (#599, #600, #608) Three issues with one shape: a name promises more than the function checks, and the documentation stops short of the boundary. #599 — has_bidi_conflict reads LETTERS, so it is structurally blind to the U+202x overrides and returns False for "invoice<RLO>gpj.exe", the best-known bidi spoof. The docstrings now say so directly and route the reader: inspect_anomalies kind `bidi` to detect, strip_bidi to remove. They also record that the two conditions are disjoint (a string can satisfy either, both or neither) and that strip_bidi does NOT close the real-letter case, because there is no format character to remove. docs/concepts/which-function.md gains the two bidi rows its threat-model table lacked. Its only previous mention of bidi was in a *cost* column, so the page a new user reaches first could not answer "how do I detect a bidi attack". #600 — get_pipeline() now states that profile names and PRESETS keys are disjoint namespaces, so get_pipeline("canonicalize") raising is the design rather than a bug. While there: PRESETS["ml_normalize"] was wrong. It listed seven steps; src/presets.rs declares nine, and the mirror was missing the transliterate step and the second demojize that #498 added after strip_accents. test_preset_steps_exact did not catch it because it compares the mirror to a literal in the test file, and both were written from the same wrong reading — the test pinned the drift instead of detecting it. Both are now correct against Rust. list_profiles() also said presets are "step-lists defined in Python". They are defined in Rust. That sentence is how the drift went unnoticed, so it is corrected, and a comment on PRESETS now states what the dict is, what it is not, and that it must move in the same commit as any Rust step change. A structural gate that parses the Rust arrays is deliberately NOT here. The step lists use composite variants (FixedPoint, ConfusablesNfcFixedPoint) that the mirror flattens, so a real gate needs per-preset expansion rules; that is its own issue, not a fragile parser bolted onto a docs change. #608 — ml_normalize's documented limits stopped at homoglyphs. All twelve bidi controls and every PUA code point also pass through unchanged: strip_control covers Cc, and bidi controls are Cf. Measured and added to docs/user-guide/llm-pipelines.md as executable assertions, alongside the contrast with llm_guardrail (clears bidi, not PUA) and rag_ingest (clears PUA). No behaviour change — the preset is tokenizer hygiene and THREAT_MODEL.md never lists it as a security mechanism. Signed-off-by: Richard Quinn <quinn.richard@gmail.com> Assisted-by: Claude Code:claude-opus-5
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The docs currently overstate ml_normalize behavior (Unicode Tags stripping) and there’s still an internal inconsistency in PRESETS documentation about where preset definitions live.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR clarifies the documented security/coverage boundaries of several bidi- and pipeline-related APIs, and fixes drift in the Python PRESETS["ml_normalize"] mirror so introspection/docs match what the Rust core actually runs.
Changes:
- Document
has_bidi_conflictas a letter-direction conflict check (not a bidi-override/RLO check) and route users to the correct detection/removal APIs. - Clarify the namespace split between policy profiles (
get_pipeline/list_profiles) and preset step recipes (PRESETS), and correctPRESETS["ml_normalize"]to match Rust. - Extend
ml_normalizedocumentation to explicitly call out additional blind spots (bidi controls and PUA).
File summaries
| File | Description |
|---|---|
tests/test_mutant_killers.py |
Updates the pinned expected step list for ml_normalize to include the previously-missing steps. |
src/scripts.rs |
Expands internal docs for bidi detection, clarifying what has_bidi_conflict does/doesn’t detect. |
src/api/safety.rs |
Updates public Rust API docs for has_bidi_conflict with explicit examples and guidance. |
python/disarm/_presets.py |
Clarifies PRESETS vs profiles, fixes ml_normalize preset mirror steps, and updates related docstrings/notes. |
python/disarm/_api.py |
Updates Python API docs for has_bidi_conflict to warn about RLO/override characters and show the correct check. |
docs/user-guide/llm-pipelines.md |
Adds executable doc-assertions documenting ml_normalize gaps and contrasts with safer profiles. |
docs/concepts/which-function.md |
Adds bidi detection/removal rows to the “which function” threat-model table. |
CHANGELOG.md |
Records the documentation clarifications and the PRESETS["ml_normalize"] mirror drift fix. |
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
📄 Docs preview: https://081f869f.disarm-docs.pages.dev |
…ags claim (#600, #608) Both from review on #622. The file said two contradictory things about where presets are defined. The docstring a reader lands on said "Defined here, in Python"; the new note said Rust. Rust is right, and that sentence is how the ml_normalize mirror drifted unnoticed in the first place. The llm-pipelines note claimed ml_normalize removes the Unicode Tags block. Measuring it gave a sharper answer than either the claim or the correction: U+E0061 and the cancel tag ARE removed, because the emoji step consumes tag sequences (a flag emoji still normalizes to "flag: scotland"), while U+E0001 LANGUAGE TAG is not part of such a sequence and survives. Both cases are now executable assertions, so Sybil holds them. Signed-off-by: Richard Quinn <quinn.richard@gmail.com> Assisted-by: Claude Code:claude-opus-5
raeq
enabled auto-merge (squash)
August 26, 2026 19:04
…q/disarm into docs/599-600-608-convergence
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
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.
Closes #599. Closes #600. Closes #608.
Three issues with one shape: the name promises more than the function checks, and the documentation stops short of the boundary. Grouping them costs one gate run instead of three and no compiled surface changes shape.
#599 —
has_bidi_conflictis not the RLO checkIt reads letters, so it is structurally blind to the
U+202xoverrides."invoice\u202Egpj.exe"— the best-known bidi spoof there is — returnsFalse.The docstrings now say that directly and route the reader rather than offering a bare "see also":
has_bidi_conflictinspect_anomalieskind"invoice\u202Egpj.exe"Falsebidi"varonis.com.ו"Truebidi_mixedTwo things the original issue got wrong, now recorded: the conditions are disjoint, not alternatives — a string can satisfy either, both or neither, so a
has_bidi_override()alias would be a false promise. Andstrip_bididoes not close this function's case: on a real-letter conflict it returns the input unchanged, because there is no format character to remove.docs/concepts/which-function.mdgains the two bidi rows its threat-model table lacked. Its only previous mention of bidi was in a cost column, so the page a new user reaches first could not answer "how do I detect a bidi attack".#600 — and a preset mirror that had drifted
get_pipeline()now states that profile names andPRESETSkeys are disjoint namespaces, soget_pipeline("canonicalize")raising is the design rather than a bug.Checking that claim turned up something worse.
PRESETS["ml_normalize"]listed seven steps;src/presets.rs:822declares[Step; 9]. The mirror was missing thetransliteratestep and the seconddemojizethat #498 added afterstrip_accents.test_preset_steps_exactdid not catch it because it compares the mirror against a literal in the test file, and both were written from the same wrong reading. The test pinned the drift instead of detecting it. Both are corrected against Rust here.The cause is also fixed:
list_profiles()said presets are "step-lists defined in Python". They are defined in Rust —PRESETSis a hand-maintained mirror that nothing executes. That sentence is how the drift went unnoticed.A structural gate is deliberately not in this PR. The step lists use composite variants (
FixedPoint,ConfusablesNfcFixedPoint) that the mirror flattens, so a gate that parses the Rust arrays needs per-preset expansion rules. That deserves its own issue rather than a fragile parser bolted onto a documentation change.#608 —
ml_normalize's other two blind spotsIts documented limits stopped at homoglyphs. All twelve bidi controls and every PUA code point also pass through unchanged —
strip_controlcoversCc, and bidi controls areCf.Added to
docs/user-guide/llm-pipelines.mdas executable assertions, with the contrast that makes them actionable:llm_guardrailclears bidi but not PUA,rag_ingestclears PUA. Doc-only, which is what the issue itself recommended — the preset is tokenizer hygiene andTHREAT_MODEL.mdnever lists it as a security mechanism.Verification
docs/fences are executed by Sybil, so the new assertions are live gates rather than prose:pytest docs/user-guide/llm-pipelines.md→ 12 passed.Full local gate: 4,705 pytest, 24 Rust test targets,
cargo fmt --check, clippy clean on both feature sets,scripts/perf_lint.shclean, mypy clean, ruff clean, language-consistency audit clean.