fix: make agent recording fail loudly and actionably, not silently - #32
Merged
Conversation
…uses
All five were measured against the user's own transcripts and ledger.
1. The shipped recording contract told agents to set `title`, which
agentacct_record_section rejected outright ("unexpected argument(s):
title") - 48 distinct sessions. The HTTP lane has always called the
field `title` and work_events has always read `section_title or
title`, so MCP was the outlier. record_section now accepts `title` as
an alias (section_title wins when both are supplied) and the
instruction constant says section_title. The alias is what keeps
already-onboarded machines working: rendered CLAUDE.md / AGENTS.md
blocks are written once at onboard and never refreshed.
2. Limit errors stated the limit but not what arrived, so a caller could
only shrink blindly (the incident cost five retries: 2039 -> 1904 ->
1664 -> 1614 -> 1477 -> 1172). A single _limit_error builder now
reports both for the scalar string limit, the list-item length and
the list-count limit, matching what the HTTP lane already does.
3. The metadata byte cap json-encoded with ensure_ascii=True, billing
every CJK character 6 bytes and every emoji 12: a summary of 1200 CJK
chars plus a next_step of 200 measured 8432 bytes and was rejected,
about a third of the advertised budget, and 504 of ~1955 stored
section summaries are CJK-heavy. The size is now measured as real
UTF-8 bytes. The 8192 limit is unchanged (it is replicated on the
HTTP and CLI write paths and the symmetry is load-bearing), and the
message now names the largest offending field and its byte size
instead of blaming a `metadata` parameter the caller never passed.
4. "files[0] must be project-relative" was the biggest single rejection
cause (~468 sessions) and the rule appeared in no schema description,
while the harness instructs absolute paths. The rule is now published
on every tool that takes `files`, and an absolute path that provably
lies under the project_dir declared on the same call is relativized
instead of rejected. Containment is lexical and conservative: no
filesystem access, only the explicit project_dir argument counts, and
the '..' check runs on the relativized result so nothing can escape.
Paths are also normalized now, closing the gap where "src/./a.py"
passed the validator and reached the ledger un-normalized.
machine_check.name gains an explicit maxLength so an over-long name
fails as a name, not as "metadata must be <= 8192 bytes"; it is never
truncated, since name feeds the check-identity hash.
5. Mangled tool calls absorb parameters into a narrative value as
literal text. Sections and machine checks now WARN when a closing
tag for one of that tool's own unsupplied properties appears in free
text, and stamp a server-authored marker. Never rejected and never
repaired - repair would fabricate fields the agent never wrote. The
closing-tag form measured 1 true positive and 0 false positives
across 1955 section events; bare-word and opening-tag detectors were
rejected as unusable.
Round-2 review of 26092c0. Six findings were behaviour, one was coverage. 1. machine_check.name loses its 240-character cap entirely, schema maxLength included. The cap turned a 241-8000 character band that recorded fine before it into hard rejections, and a ~300-character name (an agent recording the full pytest invocation it ran) is ordinary. The cap's only goal was that an over-long name not be blamed on a `metadata` parameter the caller never passed, and the size error now reports a field and a byte count, which serves that goal without a cap. Measured caveat, stated in the code and pinned by a test rather than glossed: past the budget the error names the largest field, which for a machine check is the `summary` the server synthesizes as "<name>: <result>", so that extreme case is still one step removed. It is no worse than the un-named message it replaced, and the band that actually regressed is fixed. 2. The metadata byte MEASURE moves to storage.py, next to the other cross-surface validation primitive, and all three write surfaces call it. Measuring real UTF-8 bytes on the MCP lane alone broke the symmetry the shared budget exists for: metadata={"notes": "\u8bb0"*2000} is 6013 real bytes and 12013 escaped, so MCP accepted a payload the HTTP lane rejected. One helper, one 8192, one verdict per payload. allow_nan stays each lane's own choice, so the HTTP lane's existing NaN tolerance is unchanged, and the lone-surrogate fallback now protects all three rather than one. 3. files=["."] and ["./"] no longer fail the call. Both recorded fine before; killing a whole section or machine check to reject one cosmetic path entry is exactly the behaviour this validator exists to remove. The entry names no file, so it is dropped rather than stored. An absolute path equal to project_dir now behaves the same with or without its trailing slash, instead of one form being fatal and the other cosmetic. 4. Backslashes are folded for VALIDATION only, never into the stored value. On macOS and Linux `weird\name.py` is a legal filename, and the branch was silently rewriting it into a genuinely different `weird/name.py`. What the caller sent is what gets stored; only redundant "." and "/" segments, which denote the same path either way, are still normalized away. 5. The relativized remainder is re-validated against the same rule as any other value. project_dir "/repo" with files=["/repo/~/x"] stored "~/x", and ["/repo/C:/x"] stored "C:/x" - values the schema description this server publishes calls impossible. 6. `title` is excluded from the mangle detector's candidate set. Adding it as a record_section property widened the detector onto `</title>`, the most common closing tag in HTML prose. The 1-true-positive / 0-false-positive calibration was measured on the property set WITHOUT `title` and has not been re-measured, which the docstring now says outright instead of quoting a rate for a detector that no longer matches the one measured. 7. The four uncovered pieces of new logic get real tests: the Windows drive-letter prefix, the `~` project_dir guard, the lone-surrogate fallback, and the list-count message, whose assertion is now pinned to the exact string rather than to a "50"/"51" substring pair that a useless message would also satisfy. Three assertions written by 26092c0 encoded the behaviour findings 1 and 3 call wrong: the 240 cap, its schema maxLength, and files=["."] sitting in a list of inputs expected to be rejected. They are corrected in place, not deleted, and the replacements assert the opposite behaviour. No test predating this workstream was touched. Each fix was proved non-vacuous by disabling it and confirming exactly the named test fails: 12 mutants, 12 detected. Containment was re-attacked across 42 path cases plus a real on-disk symlink out of the project; every stored value satisfies the published rule and nothing escapes.
…fix the name band Round-3 review of 33b61e9. Two findings were behaviour, one was an accusation written into the stored record, one was a measured number that was wrong. 1. A Windows absolute path no longer kills the call. Recognizing drive letters as absolute was right -- it stopped "C:/repo/a.py" being filed as a relative path -- but pairing it with a rejection turned a shape that recorded fine before into a whole-call failure, which is the exact anti-pattern this validator exists to remove. Measured: record_section with files=["C:\repo\src\a.py"] and no project_dir stores that path on 541542e and errors on 33b61e9. The review named the no-project_dir shape; the same path with a project_dir that cannot contain it regressed identically and is fixed with it, because splitting them would leave a worse inconsistency than the one being fixed. Such a path is now kept, separators normalized, and is still escape-checked, so "C:\repo\..\x" stays fatal. A POSIX absolute path with no usable project_dir stays a rejection, as before this branch. 2. The backslash rule applies on both path branches, not one. files= ["weird\name.py"] stored 'weird\name.py', but files=["/repo/weird\name.py"] with project_dir="/repo" stored 'weird/name.py' -- the same silent merge of two different POSIX files that finding 4 of 33b61e9 fixed one branch over. The relativizer returns an OFFSET rather than a substring, so the arrived value and the backslash-folded copy the containment check reads are cut at the same index. A Windows path is the deliberate exception: there a backslash is a separator, so "C:\repo\src\a.py" under project_dir="C:\repo" still stores 'src/a.py' instead of fragmenting against the same file recorded with forward slashes. 3. The mangled-tool-call detector's ineligible set is calibrated, not hand-picked. Excluding `title` alone removed one HTML/SVG element name on the grounds that agents write about markup, and left `summary`, `metadata` and `source` -- element names by the same reasoning -- armed to stamp a server-authored accusation into the stored record of ordinary prose. Measured READ-ONLY over the 6261-event global ledger, scoped to the 3535 events these two tools write (1956 section, 1579 evidence): `</title>`, `</metadata>` and `</source>` never occur, and `</summary>` occurs twice, both inside genuine mangled calls that DID supply `summary`, so it could never have fired. Excluding all four costs zero measured true positives. The docstring now carries those numbers in place of the stale "1 true positive, 0 false positives", which undercounted (there are two true positives, one per lane) and credited its bare-word counts to the section subset when they were measured over all 6261 events. It also states the two limits the numbers do not cover: no property name has EVER appeared as a closing tag in real prose, so the ledger licenses no exclusion by itself -- the set rests on the 2 measured `<title>` occurrences in genuine prose plus the vocabulary rule that generalizes them -- and only 51 of the 3535 events mention markup at all. `client`, which fires on XML prose but is in neither vocabulary and has 0 occurrences here, stays eligible and is named in the docstring as the known gap. 4. The documented name band was wrong at the top end. Binary-searching a {source, name, result} call puts the largest accepted `name` at 4036 characters and the first rejection at 4037, identically on 541542e and on 33b61e9 -- so "241-8000" was wrong when written, not changed by the branch. It is roughly half of 8192 because `name` lands in the budget twice, once as itself and once inside the synthesized "<name>: <result>" summary. Corrected in the comment and the test docstring, and pinned by a test; the 33b61e9 commit message still carries the old figure and cannot be corrected here. The same measurement also corrects the caveat above it: at 4037 the error already blames the summary (4060 bytes against the name's own 4049), so that indirection is every over-budget name, not an extreme case. Behaviour changes 1 and 2 are covered by tests that fail without them; 4 is a documentation fix, so its test characterizes the boundary rather than regressing. tests/test_mcp.py::test_windows_drive_letter_is_absolute_for_validation asserted the rejection finding 1 removes, so its first half is updated; it was added by 33b61e9 and is not on main. Suite: 2927 passed, 1 skipped.
…tor exemption The comment justified `source`'s membership in MANGLE_DETECTOR_INELIGIBLE_PROPERTIES by calling it "inert — it is required on both tools, so it is never an unsupplied property". That is checkably false: agentacct_record_machine_check declares no `required` list and reads `source` via `_optional_limited_str(..., None)`, so a machine-check call that omits it leaves `source` genuinely unsupplied and eligible to be suspected. The exemption is load-bearing there, not bookkeeping. Replace the clause with the reason that is actually true and measured, keeping the surrounding calibration intact. Re-measured READ-ONLY over the same 6261-event global ledger, scoped to the 1956 section events and the narrative values the detector reads: a bare-word detector fires on `source` in 170 of them, ahead of `files` at 130 and every other property name; `</source>` remains at 0 occurrences, as the bullet above already records. `<source>` is also an HTML element name, so the vocabulary rule that covers `title`, `summary` and `metadata` covers it directly. Comment-only change; no behavior change. Suite: 2927 passed, 1 skipped.
When the MCP server rejects a recording call it persists nothing, so a user
whose agent silently fails to record sees a thin dashboard and concludes the
product does not work. The refusal is only recorded in the client's own
transcript, which the importer already reads -- and folds into the generic
evidenced_outputs_skipped counter that no surface renders.
Derive a separate "recording calls agentacct refused" figure from that same
scan, broken down by a frozen reason vocabulary and the server's own argument
names. evidenced_outputs_skipped is deliberately NOT reused as the number: it
also counts client-side rejections, wire drift, and non-created-event payloads,
which are now reported as an explicit unclassified remainder so the two figures
reconcile instead of disagreeing.
Nothing about a refusal is stored. The breakdown rides the in-memory scan
candidate only, which is what makes it retroactive: every scan re-derives it
from the transcripts still on disk, including refusals recorded long before
this existed. The rejection path runs before the redactor, so only the bounded
{tool, field, reason_code, count} triple leaves it -- never the message, the
rejected value, its length, or a path.
Surfaced on Local logs (/raw), with copy that says plainly these are attempts
agentacct refused rather than work the user failed to record; the matching
"no work context" next-step now points at it instead of blaming the user alone.
The refused-recording figure was matching the MCP-error pattern anywhere in an output with an unanchored search, so a SUCCESSFUL call whose payload merely QUOTED an agentacct error was counted and rendered as "agentacct refused this call" -- and the reconciliation remainder showed 0 with no warning. An agent recording after_summary="green after fixing MCP error -32602: files[0] must be project-relative on ..." is real recorded work, and the one number this surface exists to be honest about was accusing it. Locate the error by POSITION instead: the first line (Claude Code renders the whole tool_result as "MCP error -32602: <message>", optionally inside its <tool_use_error> tag) and the line directly after codex's "Caused by:". Both real wire shapes still classify; a quote sitting anywhere else in a payload no longer does. An unmatched refusal remains a plain skip in the disclosed remainder, so the two figures still reconcile -- they just no longer overstate. Split the minimum bound out of value_over_limit. mcp.py rejects input_tokens=-5 with "input_tokens must be >= 0", which the shipped classifier reported under copy reading "Value above the allowed maximum" -- the opposite of what happened, telling the user to shrink a number they need to raise. The new value_under_limit code carries "Value below the allowed minimum", and a test now pins that every frozen reason code has plain-language copy. Tool attribution was checked for this shape on both live channels and is correct. The surface itself was untested end to end: every test called the render helper directly, so deleting the single line that injects the section into /raw left the whole suite green while the feature was gone from the product. Two route tests now GET the page -- one with refusals, one on a clean machine -- and assert the section id, its rows, and that the pre-redaction scan still leaks nothing at the route. Also drop a duplicate `Mapping` import that shadowed the collections.abc one for the whole of client_usage, with an ast test over this track's modules since the repo runs no linter, and correct a comment claiming the legacy codex function_call channel yields refusal text (it does not: that channel's unwrap returns None for a plain-text error, so those calls stay an honest under-count).
The refusal figure established WHERE an error must sit but never checked WHOSE error it was, so JSON-RPC errors raised by the MCP CLIENT layer were counted and rendered as "recording calls agentacct refused". A tool_result reading "MCP error -32001: Request timed out" produced refused_attempt_total=1 -- but nothing reached this server, so nothing was refused, and the surface accused the product of rejecting a call it never saw. Require the located error to carry a code agentacct's own server emits. mcp.py's dispatch answers a tools/call with -32004 (FileNotFoundError), -32602 (InvalidParams/ValueError, which is also how "Unknown tool" comes back) and -32000 (any other exception, plus the store-less degraded server whose whole job is to stay connected and refuse). Keyed on the code, not the wording, so rewording a refusal message cannot change the count. -32000 is also what the MCP client libraries use for their own ConnectionClosed, and that one collision is resolved on the CLIENT's fixed string rather than by guessing at server prose. Everything else stays an unclassified skip, disclosed as the remainder. Widen the anchor without widening the search. Three real refusal shapes were silently uncounted because only lines[0] was tried: a tool_result opening with a blank line, Claude Code's <tool_use_error> written on a line of its own rather than inline, and a codex cause separated from its "Caused by:" header by a blank line. Anchor on the first line that CARRIES something instead -- still a position, so a payload quoting an error is still not a refusal. Require "Caused by:" to actually follow something. Sitting at index 0 it is just the first line of some payload, and letting it nominate the second line let any text opt itself in. Bound the code's digit run while here: this reads untrusted transcript text, and int() on a several-thousand-digit run raises on the interpreter's own conversion limit. Pin the cross-file copy. work_ledger's next step sends a user to a heading api.py renders; a test now takes the quoted section name out of the sentence, asserts the page really renders it in both states, and asserts work_ledger keeps exactly one copy -- so neither side can be reworded alone into a next step that cannot be followed.
# Conflicts: # CHANGELOG.md
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
Two related improvements to the recording path, both surfaced by a single failed
agentacct_record_sectioncall and the audit that followed it. Grouped here because both are about the same thing: an agent's recording should not fail silently or fail for a reason nobody can act on. They touch disjoint files.1. MCP input-surface hardening (
mcp.py,install_guide.py, …)Five real rejection causes, measured against the maintainer's own transcripts:
title; the tool accepted onlysection_titleand hard-rejected the whole call (hit in 48 sessions).titleis now an accepted alias, and the instruction is corrected. The alias keeps already-onboarded machines working without re-runningonboard, since the rendered instruction files are never refreshed.filesproject-relative rule (the single largest rejection cause) is now published in the schema, and an absolute path that provably lies under the call's ownproject_diris normalized instead of failing the whole call. Paths that escape the project are still rejected — verified against a 43-shape path matrix plus a hostile-Windows matrix and a real on-disk symlink to/etc.2. Refused-recording visibility (
log_evidence.py, …)A refused recording call previously left no trace anywhere — no store entry, no counter, no log line — so an agent that failed to record was invisible to both the user and the maintainer, while the dashboard told the user to record more work. Local logs now shows the calls agentacct refused, with counts and a fixed set of reason codes. The figure is derived at read time from data already on disk, so it covers refusals that predate this release (~1070 across 440 sessions on the maintainer's machine), and it stores only counts and reason codes — never the offending value or path (the rejection path runs before redaction). A client-layer timeout is deliberately not counted as a refusal. The "usage without work context" prompt no longer blames the user for the part agentacct itself caused.
Verification
.venv/bin/pytest -q): 2954 passed, 1 skipped.tests/test_mcp.py,tests/test_refused_recording_attempts.py), each proven non-vacuous (breaking the production line fails exactly its test). Path containment, CJK cross-surface agreement, and the mangle-detector calibration were re-measured by an independent review pass.