Deep review: fail-closed dispatch, total parse, and the TOML block writer's three data-loss paths - #152
Merged
Conversation
… objects `json.loads` hands the dispatcher whatever stdin held, and only a dict was ever survivable. Every adapter's `parse()` reached for `.get` on the payload, so a valid JSON list, string, number or null crashed `run(handler, agent=...)` and every generated bundle with an AttributeError (antigravity: TypeError from `"key" in raw`). The process died with exit 1, which every host reads as a non-blocking hook error and carries on from -- a silent allow with a traceback attached. `run()` without an agent named was also reachable: an event key holding a list or an object made `name in cfg["events"]` raise `TypeError: unhashable type` out of `detect()` itself, before any handler ran. Both were reproduced by execution on all twelve adapters. Fix, at the source rather than the boundary: a payload that is not an object names no event, and a name that is not text is not one any adapter can map. `_payload.wire_name_of()` is the one place an event key is read; it returns the string, None when absent, or an empty sentinel that maps to UNKNOWN when the value is not text. The shape-inferred families (cursor, windsurf, antigravity) and the vscode_copilot dialect module route through the same rule, so a non-object payload parses to UNKNOWN everywhere and the dispatcher allows it silently -- the documented outcome at the edge of our knowledge (ARCHITECTURE.md section 7), and the same outcome the unnamed `run()` already gave it. Bundles inline these modules and inherit the fix unchanged. Two siblings found by the same probe: windsurf's `parse()` indexed `tool_info` without checking it was an object, and `tool_input_of()` caught only JSONDecodeError while a JSON string nested past the interpreter's limit raises RecursionError. Both now return the empty value the caller expects. No wire output moves: every golden fixture, generated example and bundle is unchanged. tests/test_parse_is_total.py grows the hostile corpus to cover the five non-object documents and three non-text names, per adapter. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Jothi Mani Rajendran <250249270+jothimani-rajendran@users.noreply.github.com>
…iling open An exception out of the handler escaped `run()` -- and the bundled runtime's `main()` -- as a traceback and exit 1. Every host reads a non-zero, non-2 exit as a non-blocking hook error and carries on: the `crash` trial frozen in the claude_code 2.1.263 recording in data/recordings/ watched Claude Code run the tool (observed.runs == 1). So the one bug a policy author is most likely to ship, an unhandled exception on an unusual payload, turned an "enforced" gate into an allow with noise. The owner's rule is the opposite: a door that cannot decide refuses. `handle()` now catches the handler's exception (and a wrong return type) and answers with `Decision.deny`, rendered through the adapter like any other deny. That is the block path the recordings witnessed, not an improvised one, so it lands wherever the vendor can block and degrades exactly where the vendor cannot. The reason names only the exception's class: its message may quote the payload content the policy was inspecting (a secret it caught, a path), and the reason is fed back to the agent. The traceback goes to stderr from `run()`, and an in-process caller reads it off `decision.evidence` under `HANDLER_ERROR` / `HANDLER_TRACEBACK`. A fault past the handler -- in the adapter or dispatcher, on a payload it did decode -- exits 2 with nothing on stdout: the blocking-error code on every host that has one, with nothing for the host to misread as a verdict. `data/templates/runtime.py.tmpl` inlines the same rule (`_decide()` mirrors `handle()`), and the bundler hoists `traceback` for it. Bundle equivalence holds: a "raise" handler joins the per-agent subprocess matrix, and the unfilled-stub test now asserts what actually protects an adopter -- the stub refuses in dialect with NotImplementedError in the reason and the traceback on stderr -- rather than an exit-1 crash the host would have allowed past. Golden fixtures and generated examples are unchanged. ARCHITECTURE.md section 7 records the rule beside its fail-open counterpart. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Jothi Mani Rajendran <250249270+jothimani-rajendran@users.noreply.github.com>
…n its block
`block_bounds()` located the `# >>> agentseam >>> <owner>` / `# <<< agentseam
<<< <owner>` markers with a bare `str.find`. The shorter owner's marker is a
prefix of the longer owner's, so with owner `chock` asked about a config.toml
holding only `chock-java-security`'s block -- both real consumers of this
library -- every query and write went wrong at once, reproduced by execution:
- installed(owner="chock") returned True for a block it never wrote;
- install(owner="chock") replaced chock-java-security's block with its own,
and closed it with chock-java-security's END marker, so the file then held
a chock BEGIN paired with a foreign END;
- uninstall(owner="chock") deleted the other owner's hooks.
Ownership marking exists so uninstall is surgical (ARCHITECTURE.md section 7);
a prefix match makes it the opposite. A marker must now occupy a whole line,
which is how install writes it and what the format's comment syntax means.
That also stops a marker quoted inside a user's comment from being taken for a
block. The JSON path was already exact (`v.get(MARKER) == owner`) and is
untouched.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Signed-off-by: Jothi Mani Rajendran <250249270+jothimani-rajendran@users.noreply.github.com>
… it on a failed write
`write_block()` and `remove_block()` opened Kimi Code's config.toml with no
encoding, which means the platform locale. On a Windows code page, or any
non-UTF-8 locale, that broke two ways, both reproduced by execution under
`LC_ALL=C` with UTF-8 mode and locale coercion off:
- a user's config holding a single non-ASCII byte (an accented model name,
an existing hook echoing "café") made install() AND uninstall() raise
UnicodeDecodeError before doing anything;
- a command holding a single character the code page lacks (a check mark,
a CJK repository path) raised UnicodeEncodeError from fh.write() -- after
open(path, "w") had already truncated the file. The user's entire
config.toml was left zero bytes long. TOML is specified as UTF-8; the
locale has no business in it.
Reads now decode UTF-8 (BOM-tolerant, as `load()` and `installed()` already
were), and a file that is not UTF-8 raises ConfigUnreadableError with the file
untouched -- the guarantee the JSON path has given since the data-loss fix
there. Writes encode the whole text first and only then open the file, so an
unencodable command (a lone surrogate from an undecodable argv byte is the
UTF-8 case) fails with the config intact. `dump()` names its encoding too; its
output was already ASCII, so nothing changes there.
Reading with newline="" and writing bytes also makes the docstring's
"everything outside it is left byte-for-byte" true: text mode was rewriting
every line ending in the file to the platform's, so a CRLF config lost its
CRLFs on Linux and an LF checkout gained them on Windows. The block now takes
the file's own line ending, so a CRLF file stays one kind, re-install still
replaces rather than duplicates, and uninstall is an exact inverse.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Signed-off-by: Jothi Mani Rajendran <250249270+jothimani-rajendran@users.noreply.github.com>
`resolve()` substituted the owner for every `*` in the joined path, the caller's repo_root included. A checkout under `wild*card/` was therefore wired at `wildagentseamcard/.claude/settings.json`, a directory the agent never reads, and `installed()` -- which resolves the same way -- then reported the hook wired there. Reproduced by execution. No shipped CONFIG_PATH carries a `*`, so the substitution only ever reached the part of the path the caller supplied. It now applies to the adapter's own CONFIG_PATH alone, before the join and before `~` expansion, so neither a repository directory nor a home directory can trip it. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Jothi Mani Rajendran <250249270+jothimani-rajendran@users.noreply.github.com>
…p the trailing newline `_toml_string` escaped backslash and quote only. A command body or description carrying a lone carriage return, a form feed, any other C0 control or DEL rendered a commands/*.toml the Gemini extension loader rejects outright -- reproduced with tomllib, which refuses every one of them. This is the sibling of the 0.3.0 hook-entry fix (`_hook_entry._toml_char`), in the other TOML this package writes; the same escape set applies now: the short escapes where TOML has them, \uXXXX for the rest. A tab stays raw in both string forms and a line feed in the triple-quoted one; a carriage return is always escaped, because a reader may normalise a raw CRLF and the body would come back changed. The round-trip test also caught a second defect in the same function: the triple-quoted form put a newline before its closing delimiter. TOML trims only the newline after the opening delimiter, so every multi-line prompt came back with a trailing "\n" it never had. The body now runs straight into the closing delimiter, which is legal even when it ends in one or two quote marks. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Jothi Mani Rajendran <250249270+jothimani-rajendran@users.noreply.github.com>
…e refusal
Follow-up to the fail-closed handler change. The traceback it prints is a
courtesy to the operator; the verdict on stdout is the contract, and the
print was not yet safe enough to keep that order:
- With fd 2 closed when the hook starts (an IDE that spawns hooks with no
stderr), sys.stderr is None. `sys.stderr.write` then raises inside run()
-- exit 1, fail open again -- and in the bundle, `traceback.print_exc()`
falls through to `print(file=None)`, which is STDOUT: the traceback lands
inside the JSON the host parses. Reproduced in a subprocess with fd 2
closed before exec.
- A console whose code page cannot hold the payload text an exception
message quotes raises UnicodeEncodeError from the write on a non-default
stderr, with the same consequence.
Both runtimes now write the diagnostic through one best-effort `_report()`
that swallows any failure to write, so the refusal is emitted and the exit
code is the dialect's whatever became of stderr. The bundle test closes fd 2
and asserts stdout is exactly the library's verdict; the library test swaps
sys.stderr for a dead stream and for None.
tests/test_dispatch.py crossed the 300-line budget with the handler-failure
tests; they are split by activity into tests/test_dispatch_handler_failure.py,
unchanged.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Signed-off-by: Jothi Mani Rajendran <250249270+jothimani-rajendran@users.noreply.github.com>
jothimani-rajendran
marked this pull request as ready for review
September 20, 2026 22:30
jothimani-rajendran
added a commit
that referenced
this pull request
Sep 20, 2026
Everything under [Unreleased] since v0.3.0, rolled into a dated section: the seven findings of the pre-release review in #152 (a handler that raises now refuses in the vendor's dialect instead of crashing to a non-blocking error; Kimi Code's config.toml is read and written as UTF-8 and never truncated by a failed write; TOML block markers match whole lines so one owner cannot claim another's block; parse() and detect() are total over non-object payloads; Gemini command TOML escapes control characters; a diagnostic that cannot be written never pre-empts the refusal; a `*` in repo_root is a directory name). Fixes only, so a patch bump under semver. Version is declared in three places and all three move together: pyproject.toml, src/agentseam/__init__.py, and CITATION.cff, which tests/test_repo_standards.py holds equal to the package. The [Unreleased] compare link becomes the 0.3.1 link. docs/assets/social-preview.{svg,png} are regenerated, not edited: the card renders the version, and the brand-assets check would have failed on the stale one. No behaviour change in this commit. The v0.3.1 tag already on main points at a commit versioned 0.3.0, so it has to be moved to this one after the merge for release.yml to publish 0.3.1. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Jothi Mani Rajendran <250249270+jothimani-rajendran@users.noreply.github.com>
Merged
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 this changes
A pre-release review of 0.3.0's core paths (parse/claims, respond,
run(), install/uninstall, bundles, matrix) found seven defects, each reproduced by execution before the fix. Ranked by severity, with the commit that fixes each:run()and the bundledmain()as a traceback + exit 1. Every host reads that as a non-blocking error; thecrashtrial indata/recordings/witnessed Claude Code run the tool. An "enforced" gate failed open on the most common policy bug. Now refused in the vendor's own dialect (the witnessed block path), naming only the exception class; traceback to stderr; a fault past the handler exits 2.fix(dispatch): a handler that raises refuses in dialect…install()on Kimi Code'sconfig.tomlopened the file with no encoding (platform locale). Under a Windows code page / non-UTF-8 locale: one non-ASCII byte in the user's config →UnicodeDecodeErroron install and uninstall; one character the code page lacks in the command →UnicodeEncodeErrorafteropen("w")had truncated the file to 0 bytes. Reproduced underLC_ALL=Cwith UTF-8 mode off. Reads are UTF-8 now (non-UTF-8 →ConfigUnreadableError, untouched); writes encode before opening; bytes outside our block are preserved exactly (CRLF included).fix(install): read and write config.toml as UTF-8…block_bounds()matched owner markers by substring, so ownerchockmatched insidechock-java-security's begin and end markers:installed(owner="chock")→ True for a block it never wrote;install(owner="chock")replaced the other owner's block and closed it with the other owner's END marker;uninstall(owner="chock")deleted it. A marker must now occupy a whole line.fix(install): an owner that prefixes another owner's name…parse()reached for.geton the payload, so a valid JSON list/string/number/null crashedrun(handler, agent=…)and all 12 bundles (exit 1 = allow with noise); an event key holding a list/object raisedTypeError: unhashable typeout ofdetect()itself. Now UNKNOWN → silent allow everywhere, the documented edge-of-knowledge outcome. Siblings: windsurf indexed a non-objecttool_info;tool_input_of()letRecursionErrorpast itsJSONDecodeErrorguard.fix(adapters): parse() and detect() are total…packaging._toml_stringescaped only\and"; a lone CR, form feed, other C0 control or DEL in a Gemini command body/description rendered TOML the extension cannot load (sibling of the 0.3.0 hook-entry fix). Also: the triple-quoted form put a newline before the closing delimiter, which TOML keeps, so every multi-line prompt gained a trailing\n.fix(packaging): escape control characters in Gemini command TOML…sys.stderris None, sotraceback.print_exc()in the bundle prints to stdout, inside the verdict, andsys.stderr.writein the library raises (exit 1). Now best-effort through_report(); a non-dictevidencefrom a handler cannot trip it either.fix(dispatch): a diagnostic that cannot be written never pre-empts…resolve()substituted the owner for every*in the joined path, so a checkout underwild*card/was wired atwildagentseamcard/…andinstalled()agreed. Applies to the adapter'sCONFIG_PATHonly now.fix(install): a \*` in repo_root is a directory name…`No golden wire fixture, generated example page, or bundle output changed except where a handler raises (new behaviour, covered by the equivalence matrix: a
raisehandler joinedtest_bundler_subprocess.py's per-agent matrix).ARCHITECTURE.md§7 gains the fail-closed rule beside its fail-open counterpart;CHANGELOG.md[Unreleased]has an entry per finding.One test assertion moved rather than being added:
test_the_unmodified_handler_stub_fails_loudly_rather_than_allowing_everythingassertedcode != 0(the exit-1 crash the host allows past). It now asserts what actually protects an adopter: the unfilled stub refuses in dialect withNotImplementedErrorin the reason, the traceback still on stderr, and the exception's message kept out of the verdict. Two test files were split by activity for the 300-line budget (test_install_toml_block.py,test_dispatch_handler_failure.py).Claim check
MATRIXor vendor data changed)MATRIXrow carries averifiedrecord — n/a, none changedChecks
pytest -qpasses —1839 passed, 4 skipped(3.11);1838 passed, 4 skipped(3.13);1834 passed, 8 skipped(3.10, tomllib tests skip)ruff check .andruff format --check .pass —All checks passed!/161 files already formattedpython tests/check_stdlib_only.py→stdlib-only import OK 0.3.0; the only new import istracebackgit commit -s)python examples/generate.py --check→examples up to date (13 files)Notes for the reviewer
Checked and found sound (no change): every
can_block=True(agent, event) answers a deny with non-silent output (scripted over all adapters); no two adapters claim any shipped fixture; the JSON writers escape control characters and quotes (ensure_ascii), so the TOML bug has no JSON sibling;mark/strip_ownedownership is exact on the JSON side; symlinked configs are written through, not replaced;install()refuses unparseable JSON untouched; Cursor gates getfailClosedunlessfail_closed=False; bundles for all 12 agents compile, import nothing third-party and replay the golden set; grade ceilings by basis behave (avendor-docsfail-closed cell gradesbest-effort); a 60k-case mutation fuzz ofdetect/parse/respondover the shipped scenarios finds no exception after these fixes (it found #4 before them). There is no YAML writer in this package.Behaviour change to be aware of: a handler exception is now a refusal, not a crash. For an in-process caller of
handle()the exception no longer propagates; it is ondecision.evidence["handler_error"]/["handler_traceback"]anddecision.outcome == "deny".run()prints the traceback to stderr as before, so a terminal user sees the same thing; the host sees a deny instead of a non-blocking error.Two things reviewed and left for the owner (data judgement, no reproduction of harm):
claude_code's row islive-runwithverified.observed = [pre_tool, prompt_submit, stop], yet all twelve of its cells carrylive-runper-claim records (the nine others cite the row's method).cursorusesobservedto mean "seen fire";claude_codeuses it to mean "block witnessed".claim_basis()applies theobservedfilter only tolive-run-partialrows. No grade is affected today (the nine cells are non-blocking →detectregardless), so nothing was changed, but the two rows readobserveddifferently.dump()now writes\non every platform (it wrote CRLF on Windows via text mode). JSON does not care; noting it because it is a byte-level change on Windows.Nothing pushed as a tag, no release created, PR left as draft.
🤖 Generated with Claude Code