From 26092c037cf7e407fc893020482d3b9a40595d92 Mon Sep 17 00:00:00 2001 From: mikehasa Date: Fri, 31 Jul 2026 00:01:46 +0800 Subject: [PATCH 1/9] fix(mcp): harden the MCP input surface against five real rejection causes 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. --- src/agentacct/install_guide.py | 7 +- src/agentacct/mcp.py | 287 +++++++++++++++++++++-- tests/test_mcp.py | 406 +++++++++++++++++++++++++++++++++ 3 files changed, 674 insertions(+), 26 deletions(-) diff --git a/src/agentacct/install_guide.py b/src/agentacct/install_guide.py index b3bc180..98e57c2 100644 --- a/src/agentacct/install_guide.py +++ b/src/agentacct/install_guide.py @@ -257,7 +257,12 @@ def agent_notes(agent: str) -> tuple[str, ...]: # The per-task directive bullets, shared verbatim by every recording surface. _RECORDING_CONTRACT_LINES = ( - "- Open a section with `agentacct_record_section` (`section_status=started`) BEFORE your first other tool call, and again before each meaningful task; set `title` to a short human goal (e.g. \"add rate-limit to login\"). Complete it (`section_status=completed`) or block it (`blocked`) when done.", + # `section_title`, not `title`: the MCP tool's own argument name. The old + # `title` wording named a parameter agentacct_record_section rejected + # outright ("unexpected argument(s): title"), so the instruction shipped a + # call that could not succeed. mcp.py now also accepts `title` as an alias, + # which is what keeps already-rendered CLAUDE.md/AGENTS.md files working. + "- Open a section with `agentacct_record_section` (`section_status=started`) BEFORE your first other tool call, and again before each meaningful task; set `section_title` to a short human goal (e.g. \"add rate-limit to login\"). Complete it (`section_status=completed`) or block it (`blocked`) when done.", "- For a long task, send `section_status=checkpoint` updates rather than one giant section.", "- After running tests or a build, record the objective result with `agentacct_record_machine_check`.", "- Keep MCP/event evidence separate from token/cost claims: MCP events prove what work happened; a token or cost figure is only real if it comes from actual client usage the importer read — never fabricate one.", diff --git a/src/agentacct/mcp.py b/src/agentacct/mcp.py index 1f29100..939be19 100644 --- a/src/agentacct/mcp.py +++ b/src/agentacct/mcp.py @@ -3,6 +3,7 @@ import json import math import os +import re import sys import tempfile from collections.abc import Sequence @@ -25,6 +26,25 @@ EVIDENCE_TYPES = {"test", "build", "lint", "typecheck", "smoke", "benchmark", "browser", "security", "artifact", "other"} EVIDENCE_RESULTS = {"passed", "failed", "skipped", "error", "unknown"} +# Metadata byte budget. Deliberately identical to the HTTP (api.py) and CLI +# (cli.py) write paths: the number is load-bearing for cross-surface symmetry, +# so raise it in all three or in none. +METADATA_MAX_BYTES = 8192 +# A machine check's name feeds the check-identity hash and lands in metadata +# twice, so it needs its OWN limit: without one an over-long name surfaced as +# "metadata must be <= 8192 bytes", blaming a parameter the caller never sent. +# Never truncate it — a truncated name forks check identity. +MACHINE_CHECK_NAME_MAX_LENGTH = 240 + +# The files rule, published in every schema that takes `files`. It is the single +# biggest MCP rejection cause: agent harnesses hand out absolute paths, and the +# rule appeared in no description at all. +FILES_DESCRIPTION = ( + "Project-relative paths with forward slashes: no leading '/' or '~', no '.' or '..' segments. " + "An absolute path is relativized and accepted ONLY when it lies under the project_dir supplied " + "on this same call; otherwise it is rejected rather than guessed at." +) + # Join keys that stay valid for a whole client session, so sections recorded on # the same stdio server may inherit them from agentacct_attach_client_context. # Turn/message/request ids are per-turn and must never be inherited. @@ -65,7 +85,7 @@ "type": "object", "properties": { "run_id": {"type": "string", "default": "latest"}, - "name": {"type": "string", "default": "check"}, + "name": {"type": "string", "default": "check", "maxLength": MACHINE_CHECK_NAME_MAX_LENGTH}, "before_exit_code": {"type": ["integer", "null"]}, "after_exit_code": {"type": ["integer", "null"]}, "before_summary": {"type": ["string", "null"]}, @@ -81,7 +101,7 @@ "artifact_ref": {"type": ["string", "null"], "maxLength": 240}, "artifact_path": {"type": ["string", "null"], "maxLength": 500}, "artifact_url": {"type": ["string", "null"], "maxLength": 500}, - "files": {"type": "array", "items": {"type": "string", "maxLength": 240}, "maxItems": 50, "default": []}, + "files": {"type": "array", "items": {"type": "string", "maxLength": 240}, "maxItems": 50, "default": [], "description": FILES_DESCRIPTION}, "idempotency_key": {"type": ["string", "null"], "maxLength": 240}, "client": {"type": ["string", "null"], "maxLength": 80}, "client_session_id": {"type": ["string", "null"], "maxLength": 240}, @@ -151,7 +171,16 @@ "section_id": {"type": "string", "minLength": 1, "maxLength": 120}, "section_status": {"type": "string", "enum": ["started", "checkpoint", "completed", "blocked"]}, "run_id": {"type": ["string", "null"], "maxLength": 128, "pattern": "^[A-Za-z0-9][A-Za-z0-9_-]{0,127}$"}, - "section_title": {"type": ["string", "null"], "maxLength": 160}, + "section_title": { + "type": ["string", "null"], + "maxLength": 160, + "description": "Short human goal for this section. `title` is accepted as an alias; if both are supplied, `section_title` wins.", + }, + "title": { + "type": ["string", "null"], + "maxLength": 160, + "description": "Alias for `section_title` (the HTTP lane names this field `title`). Ignored when `section_title` is also supplied.", + }, "phase": {"type": ["string", "null"], "maxLength": 80}, "kind": {"type": ["string", "null"], "enum": ["planning", "implementation", "debugging", "testing", "review", "docs", "refactor", "research", "other", "unknown", None]}, "summary": {"type": ["string", "null"], "maxLength": 1200}, @@ -165,7 +194,7 @@ "message_id": {"type": ["string", "null"], "maxLength": 240}, "request_id": {"type": ["string", "null"], "maxLength": 240}, "client_event_timestamp": {"type": ["string", "null"], "maxLength": 80}, - "files": {"type": "array", "items": {"type": "string", "maxLength": 240}, "maxItems": 50, "default": []}, + "files": {"type": "array", "items": {"type": "string", "maxLength": 240}, "maxItems": 50, "default": [], "description": FILES_DESCRIPTION}, "blocker": {"type": ["string", "null"], "maxLength": 1200}, "next_step": {"type": ["string", "null"], "maxLength": 1200}, "idempotency_key": {"type": ["string", "null"], "maxLength": 240}, @@ -282,10 +311,24 @@ def _reject_unknown_keys(args: dict[str, Any], allowed: set[str]) -> None: raise InvalidParams("unexpected argument(s): " + ", ".join(unknown)) -def _optional_str(args: dict[str, Any], key: str, default: str) -> str: +def _limit_error(key: str, *, limit: int, received: int, unit: str = "characters") -> InvalidParams: + """The single shape every size/limit rejection in this module uses. + + It always states the limit AND what arrived. Without the received size a + caller can only shrink blindly — the real incident behind this helper cost + five retries (2039 -> 1904 -> 1664 -> 1614 -> 1477 -> 1172 accepted). The + HTTP lane already reports both via pydantic; this matches it. Sizes only: + never echo caller content back into an error string. + """ + return InvalidParams(f"{key} must be <= {limit} {unit} (received {received})") + + +def _optional_str(args: dict[str, Any], key: str, default: str, *, max_length: int | None = None) -> str: value = args.get(key, default) if not isinstance(value, str) or not value: raise InvalidParams(f"{key} must be a non-empty string") + if max_length is not None and len(value) > max_length: + raise _limit_error(key, limit=max_length, received=len(value)) return value @@ -355,7 +398,7 @@ def _optional_limited_str(args: dict[str, Any], key: str, default: str | None = if not isinstance(value, str) or not value: raise InvalidParams(f"{key} must be a non-empty string or null") if len(value) > max_length: - raise InvalidParams(f"{key} must be <= {max_length} characters") + raise _limit_error(key, limit=max_length, received=len(value)) return value @@ -389,13 +432,109 @@ def _optional_metadata(args: dict[str, Any]) -> dict[str, Any]: return value +def _json_utf8_size(value: Any) -> int: + """Real UTF-8 byte size of the JSON encoding. + + ``ensure_ascii=False`` is the whole point: with the default, the size check + bills every CJK character 6 bytes (\\uXXXX) and every emoji 12, i.e. 2-4x + what the text actually costs, so a Chinese-writing agent got roughly a + third of the advertised budget. Measured before this fix: 1200 CJK chars = + 7215 bytes, and 1200 CJK chars of summary plus 200 of next_step = 8432 + bytes, rejected. + """ + encoded = json.dumps(value, sort_keys=True, allow_nan=False, ensure_ascii=False) + try: + return len(encoded.encode("utf-8")) + except UnicodeEncodeError: + # Lone surrogates have no UTF-8 form. Measure the escaped encoding + # rather than failing a call on a pathological string; the caller's + # rejection, if any, must still be about SIZE. + return len(json.dumps(value, sort_keys=True, allow_nan=False).encode("utf-8")) + + +def _metadata_size_error(value: dict[str, Any], size: int) -> InvalidParams: + """Size rejection that names the field to shrink. + + The old message named `metadata` — a parameter most callers never passed, + since the bulk is usually a validated argument (summary, blocker, + next_step) the server merged in. Naming the largest field, with its byte + size, is the difference between a fixable error and a guessing game. The + key name is caller-controlled, so it is clipped before it reaches the + error string. + """ + largest_key = "" + largest_size = 0 + for key, item in value.items(): + item_size = _json_utf8_size({key: item}) + if item_size > largest_size: + largest_key, largest_size = str(key), item_size + detail = f"; largest field is {largest_key[:60]} at {largest_size} bytes" if largest_key else "" + return InvalidParams( + f"metadata must be <= {METADATA_MAX_BYTES} bytes when JSON encoded (received {size} bytes){detail}" + ) + + def _validate_metadata_size(value: dict[str, Any]) -> None: try: - encoded = json.dumps(value, sort_keys=True, allow_nan=False) + size = _json_utf8_size(value) except ValueError as exc: raise InvalidParams("metadata must be strict JSON") from exc - if len(encoded.encode("utf-8")) > 8192: - raise InvalidParams("metadata must be <= 8192 bytes when JSON encoded") + if size > METADATA_MAX_BYTES: + raise _metadata_size_error(value, size) + + +# --- Mangled tool-call detection (WARN ONLY) --------------------------------- +# When a client's tool-call parser mangles a call, arguments arrive absorbed +# into another argument's value as literal text ("...done."). Observed +# three times in one session, twice AFTER the agent had already diagnosed it. +# agentacct warns and records the suspicion; it never rejects and never +# repairs, because repairing would fabricate fields the agent never wrote. + +# Server-authored marker (listed in RESERVED_CONTEXT_STRIP_KEYS, so a caller +# cannot stamp its own events with it). +MANGLED_TOOL_CALL_METADATA_KEY = "mangled_tool_call_suspected_fields" + +# The free-text arguments a mangled parameter can be absorbed into, per tool. +SECTION_NARRATIVE_KEYS = ("summary", "blocker", "next_step", "section_title", "title") +MACHINE_CHECK_NARRATIVE_KEYS = ("summary", "before_summary", "after_summary", "resolution_summary", "command") + + +def _tool_property_names(tool_name: str) -> frozenset[str]: + for tool in TOOLS: + if tool.get("name") == tool_name: + return frozenset(tool["inputSchema"].get("properties", {})) + return frozenset() + + +def _detect_mangled_tool_call_fields(tool_name: str, args: dict[str, Any], narrative_keys: Sequence[str]) -> list[str]: + """This tool's own argument names that appear as CLOSING tags in free text. + + Closing tags only, and only for properties the call did NOT supply. The + looser forms were measured against the real ledger and are unusable: a + bare-word detector fires 277 times on "source" and 192 on "files" across + 1955 section events, and an opening-tag detector has a real false positive + on legitimate prose about MCP config. The closing-tag form scored 1 true + positive and 0 false positives on the same events. It will still fire on + meta-work about agentacct itself, which is acceptable for a warning. + """ + missing = _tool_property_names(tool_name) - set(args) + if not missing: + return [] + suspected: set[str] = set() + for key in narrative_keys: + value = args.get(key) + if isinstance(value, str): + suspected.update(name for name in missing if f"" in value) + return sorted(suspected) + + +def _mangled_tool_call_warnings(fields: Sequence[str]) -> list[str]: + return [ + f"Possible mangled tool call: a closing tag appears inside a free-text argument, but " + f"{field} was not supplied as an argument. agentacct recorded your text verbatim and did NOT " + f"repair it; re-send with {field} as its own argument if it was meant to be one." + for field in fields + ] # Context keys whose collision with free-form metadata is treated as an @@ -414,6 +553,7 @@ def _validate_metadata_size(value: dict[str, Any]) -> None: "client_session_id", "client_transcript_id", "parent_client_session_id", + MANGLED_TOOL_CALL_METADATA_KEY, *RESERVED_CLIENT_CONTEXT_PROVENANCE_KEYS, } ) @@ -470,27 +610,90 @@ def _optional_limited_str_list(args: dict[str, Any], key: str, *, max_items: int if not isinstance(value, list): raise InvalidParams(f"{key} must be an array") if len(value) > max_items: - raise InvalidParams(f"{key} must contain <= {max_items} items") + raise _limit_error(key, limit=max_items, received=len(value), unit="items") result: list[str] = [] for index, item in enumerate(value): if not isinstance(item, str) or not item: raise InvalidParams(f"{key}[{index}] must be a non-empty string") if len(item) > max_length: - raise InvalidParams(f"{key}[{index}] must be <= {max_length} characters") + raise _limit_error(f"{key}[{index}]", limit=max_length, received=len(item)) result.append(item) return result -def _optional_project_relative_files(args: dict[str, Any], key: str = "files") -> list[str]: +_WINDOWS_DRIVE_PREFIX = re.compile(r"^[A-Za-z]:/") + + +def _looks_absolute(path: str) -> bool: + """Absolute for validation purposes: a POSIX root, a home shortcut, or a + Windows drive letter. The drive-letter case matters because the value has + already had backslashes folded to forward slashes, so PurePosixPath alone + would read ``C:/repo/a.py`` as a relative path and store it as one.""" + + return path.startswith(("/", "~")) or bool(_WINDOWS_DRIVE_PREFIX.match(path)) + + +def _relative_to_project_dir(path: str, root: str | None) -> str | None: + """``path`` expressed relative to ``root``, or None when containment is not provable. + + Purely lexical: no filesystem access and no symlink resolution. That is the + correct boundary here — this decides which STRING lands in the ledger, not + which file gets opened, and project_dir routinely names a directory that + does not exist on the machine reading the store later. Both values are + caller-controlled, so nothing is inferred: a root that is not itself + absolute, or a path that is not under it, returns None and the caller + rejects. Any ``..`` surviving into the returned value is caught by the + escape check at the call site. + """ + if not root or not _looks_absolute(root) or root.startswith("~"): + return None + prefix = root + "/" + if not path.startswith(prefix): + return None + # lstrip: a redundant separator ("/repo//etc/passwd") is the same POSIX path + # as "/repo/etc/passwd", but leaving the leading slash on the remainder + # would rebuild an absolute-looking "//etc/passwd" in the ledger. + return path[len(prefix) :].lstrip("/") + + +def _optional_project_relative_files( + args: dict[str, Any], key: str = "files", *, project_dir: str | None = None +) -> list[str]: + """Validate AND normalize the files list into project-relative POSIX paths. + + Absolute paths are the single biggest MCP rejection cause — agent harnesses + instruct absolute paths and the rule was published nowhere — so an absolute + path is relativized instead of rejected when it provably lies under the + project_dir declared ON THIS CALL. Only the explicit argument counts: an + inherited or session-attached project_dir could belong to a different + repository, and a path relativized against the wrong root is a silently + wrong ledger row, which is worse than a rejection. + + Normalization also closes the gap where ``src/./a.py`` passed the old + validator (PurePosixPath drops "." segments before they were inspected) and + reached the ledger un-normalized, splitting one file into two rows. + """ files = _optional_limited_str_list(args, key, max_items=50, max_length=240) + root = project_dir.replace("\\", "/").rstrip("/") if project_dir else None + normalized_files: list[str] = [] for index, item in enumerate(files): - normalized = item.replace("\\", "/") - path = PurePosixPath(normalized) - if path.is_absolute() or normalized.startswith("~"): - raise InvalidParams(f"{key}[{index}] must be project-relative") - if any(part in {"", ".", ".."} for part in path.parts): - raise InvalidParams(f"{key}[{index}] must be a normalized project-relative path") - return files + candidate = item.replace("\\", "/") + if _looks_absolute(candidate): + relative = _relative_to_project_dir(candidate, root) + if relative is None: + raise InvalidParams( + f"{key}[{index}] must be project-relative: forward slashes, no leading '/' or '~', " + "no '.' or '..' segments. Pass the path relative to the repository root, or supply " + "project_dir on this call so an absolute path under it can be relativized." + ) + candidate = relative + parts = PurePosixPath(candidate).parts + if ".." in parts: + raise InvalidParams(f"{key}[{index}] must not escape the project directory: '..' segments are not allowed") + if not parts: + raise InvalidParams(f"{key}[{index}] must name a path inside the project, not the project root itself") + normalized_files.append("/".join(parts)) + return normalized_files def _client_context_metadata(args: dict[str, Any], *, require_client: bool, require_session: bool) -> dict[str, Any]: @@ -853,7 +1056,7 @@ def call_tool(self, name: Any, arguments: Any) -> dict[str, Any]: ) has_outcome_fields = any(key in arguments for key in {"before_exit_code", "after_exit_code", "before_summary", "after_summary"}) or not has_evidence_fields run_id = _optional_str(arguments, "run_id", "latest") - check_name = _optional_str(arguments, "name", "check") + check_name = _optional_str(arguments, "name", "check", max_length=MACHINE_CHECK_NAME_MAX_LENGTH) before_exit_code = _optional_nullable_int(arguments, "before_exit_code") after_exit_code = _optional_nullable_int(arguments, "after_exit_code") before_summary = _optional_nullable_str(arguments, "before_summary") @@ -936,6 +1139,10 @@ def call_tool(self, name: Any, arguments: Any) -> dict[str, Any]: raise InvalidParams( "a blocker resolution requires exit_code=0 or an artifact_ref/artifact_path/artifact_url" ) + evidence_project_dir = _optional_limited_str(arguments, "project_dir", None, max_length=1000) + mangled_fields = _detect_mangled_tool_call_fields( + "agentacct_record_machine_check", arguments, MACHINE_CHECK_NARRATIVE_KEYS + ) evidence_context = { "sentinel_semantic_kind": "evidence", "evidence_type": _optional_choice(arguments, "evidence_type", EVIDENCE_TYPES, "other"), @@ -949,7 +1156,7 @@ def call_tool(self, name: Any, arguments: Any) -> dict[str, Any]: "artifact_ref": _optional_limited_str(arguments, "artifact_ref", None, max_length=240), "artifact_path": _optional_limited_str(arguments, "artifact_path", None, max_length=500), "artifact_url": _optional_limited_str(arguments, "artifact_url", None, max_length=500), - "files": _optional_project_relative_files(arguments), + "files": _optional_project_relative_files(arguments, project_dir=evidence_project_dir), "idempotency_key": _optional_limited_str(arguments, "idempotency_key", None, max_length=240), "client": _optional_limited_str(arguments, "client", None, max_length=80), "client_session_id": _optional_limited_str(arguments, "client_session_id", None, max_length=240), @@ -957,11 +1164,14 @@ def call_tool(self, name: Any, arguments: Any) -> dict[str, Any]: # cannot smuggle join ids through free-form metadata. "client_transcript_id": None, "parent_client_session_id": None, - "project_dir": _optional_limited_str(arguments, "project_dir", None, max_length=1000), + "project_dir": evidence_project_dir, "resolves_blocked_event_id": resolves_blocked_event_id, "resolution_scope": resolution_scope, "resolution_summary": resolution_summary, "resolution_objective_basis": resolution_objective_basis, + # Server-authored; listed even when empty so a caller cannot + # stamp the marker through free-form metadata. + MANGLED_TOOL_CALL_METADATA_KEY: mangled_fields or None, } payload["event"] = self.service.record_event( { @@ -974,6 +1184,9 @@ def call_tool(self, name: Any, arguments: Any) -> dict[str, Any]: trusted_blocker_resolution=resolution_requested, transport="mcp", ) + recorded_mangled = payload["event"].get("metadata", {}).get(MANGLED_TOOL_CALL_METADATA_KEY) or [] + if recorded_mangled: + payload["warnings"] = _mangled_tool_call_warnings(recorded_mangled) elif name == "agentacct_record_event": allowed = { "source", @@ -1079,6 +1292,12 @@ def call_tool(self, name: Any, arguments: Any) -> dict[str, Any]: "section_status", "run_id", "section_title", + # `title` is the HTTP lane's name for the same field, and it is + # what every shipped instruction surface told agents to pass + # until this release. Rendered CLAUDE.md/AGENTS.md files are + # written once at onboard and never refreshed, so the alias is + # what keeps already-onboarded machines recording at all. + "title", "phase", "kind", "summary", @@ -1100,18 +1319,29 @@ def call_tool(self, name: Any, arguments: Any) -> dict[str, Any]: } _reject_unknown_keys(arguments, allowed) section_status = _required_choice(arguments, "section_status", {"started", "checkpoint", "completed", "blocked"}) + # Both spellings are validated even when only one is used, so a + # malformed alias is never silently ignored. section_title wins. + section_title = _optional_limited_str(arguments, "section_title", None, max_length=160) + section_title_alias = _optional_limited_str(arguments, "title", None, max_length=160) + section_project_dir = _optional_limited_str(arguments, "project_dir", None, max_length=1000) + mangled_fields = _detect_mangled_tool_call_fields( + "agentacct_record_section", arguments, SECTION_NARRATIVE_KEYS + ) context = { "sentinel_semantic_kind": "section", "usage_join_strategy": "agent_reported_section_context", "section_id": _required_limited_str(arguments, "section_id", max_length=120), "section_status": section_status, - "section_title": _optional_limited_str(arguments, "section_title", None, max_length=160), + "section_title": section_title if section_title is not None else section_title_alias, "phase": _optional_limited_str(arguments, "phase", None, max_length=80), "kind": _optional_choice(arguments, "kind", WORK_KINDS, "unknown"), "summary": _optional_limited_str(arguments, "summary", None, max_length=1200), - "files": _optional_project_relative_files(arguments), + "files": _optional_project_relative_files(arguments, project_dir=section_project_dir), "blocker": _optional_limited_str(arguments, "blocker", None, max_length=1200), "next_step": _optional_limited_str(arguments, "next_step", None, max_length=1200), + # Server-authored; listed even when empty so a caller cannot + # stamp the marker through free-form metadata. + MANGLED_TOOL_CALL_METADATA_KEY: mangled_fields or None, **_client_context_metadata(arguments, require_client=False, require_session=False), } section_source = _required_limited_str(arguments, "source", max_length=80) @@ -1212,10 +1442,17 @@ def call_tool(self, name: Any, arguments: Any) -> dict[str, Any]: ) recorded_metadata = recorded.get("metadata") if isinstance(recorded.get("metadata"), dict) else {} join_hint_quality = _context_join_hint_quality(recorded_metadata, run_id=recorded.get("run_id")) + # Read the marker back off the PERSISTED event so an idempotent + # replay warns exactly like the write that stored it. + recorded_mangled = recorded_metadata.get(MANGLED_TOOL_CALL_METADATA_KEY) or [] payload = { "event": recorded, "join_hint_quality": join_hint_quality, - "warnings": [*size_warnings, *_context_join_warnings(join_hint_quality)], + "warnings": [ + *size_warnings, + *_mangled_tool_call_warnings(recorded_mangled), + *_context_join_warnings(join_hint_quality), + ], } # Describe what was PERSISTED (idempotent replays return the stored # event), so payload and store never disagree about inheritance — diff --git a/tests/test_mcp.py b/tests/test_mcp.py index 92579a4..170ec8d 100644 --- a/tests/test_mcp.py +++ b/tests/test_mcp.py @@ -2693,3 +2693,409 @@ def test_refusal_note_with_attach_inherited_ids_does_not_claim_unattributed(tmp_ ) ) assert "stays unattributed" in bare["refused_client_context"]["note"] + + +# --- MCP input-surface hardening --------------------------------------------- + + +def test_section_accepts_title_alias_and_prefers_section_title(tmp_path): + """The shipped instruction surfaces told agents to pass `title` and the + schema rejected the whole call. The HTTP lane has always called this field + `title` and work_events reads `section_title or title`, so MCP was the + outlier: accept the alias, but let the canonical name win.""" + server = SentinelMCPServer(store_dir=tmp_path / "state") + + aliased = _tool_payload( + _call_tool( + server, + 1, + "agentacct_record_section", + {"source": "codex", "section_id": "alias", "section_status": "started", "title": "add rate-limit to login"}, + ) + ) + assert aliased["event"]["metadata"]["section_title"] == "add rate-limit to login" + + both = _tool_payload( + _call_tool( + server, + 2, + "agentacct_record_section", + { + "source": "codex", + "section_id": "alias-both", + "section_status": "started", + "section_title": "canonical", + "title": "alias", + }, + ) + ) + assert both["event"]["metadata"]["section_title"] == "canonical" + + # The alias is one specific key, not a hole in the unknown-key guard. + still_unknown = _call_tool( + server, + 3, + "agentacct_record_section", + {"source": "codex", "section_id": "alias-bad", "section_status": "started", "titel": "typo"}, + ) + assert still_unknown["error"]["code"] == -32602 + assert "titel" in still_unknown["error"]["message"] + + # A malformed alias is rejected rather than silently ignored. + bad_alias = _call_tool( + server, + 4, + "agentacct_record_section", + {"source": "codex", "section_id": "alias-long", "section_status": "started", "title": "t" * 161}, + ) + assert bad_alias["error"]["code"] == -32602 + assert "title" in bad_alias["error"]["message"] + + +def test_shipped_instructions_name_the_argument_the_schema_accepts(tmp_path): + """Every instruction surface is generated from one constant; the section tool + must accept whatever field name that constant tells agents to set.""" + from agentacct.install_guide import MCP_SERVER_INSTRUCTIONS + + assert "set `section_title`" in MCP_SERVER_INSTRUCTIONS + assert "set `title`" not in MCP_SERVER_INSTRUCTIONS + + server = SentinelMCPServer(store_dir=tmp_path / "state") + tools = server.handle_message({"jsonrpc": "2.0", "id": 1, "method": "tools/list", "params": {}}) + section_props = {tool["name"]: tool for tool in tools["result"]["tools"]}["agentacct_record_section"]["inputSchema"]["properties"] + assert "section_title" in section_props + assert section_props["title"]["maxLength"] == 160 + assert "section_title" in section_props["title"]["description"] + + +def test_limit_errors_state_the_limit_and_what_was_received(tmp_path): + """Blind shrinking cost five retries in the incident that prompted this: + 2039 -> 1904 -> 1664 -> 1614 -> 1477 -> 1172 accepted.""" + server = SentinelMCPServer(store_dir=tmp_path / "state") + + long_summary = _call_tool( + server, + 1, + "agentacct_record_section", + {"source": "codex", "section_id": "limits", "section_status": "started", "summary": "x" * 2039}, + ) + message = long_summary["error"]["message"] + assert "1200" in message and "2039" in message + + long_file = _call_tool( + server, + 2, + "agentacct_record_section", + {"source": "codex", "section_id": "limits", "section_status": "started", "files": ["a" * 300]}, + ) + file_message = long_file["error"]["message"] + assert "files[0]" in file_message + assert "240" in file_message and "300" in file_message + + too_many = _call_tool( + server, + 3, + "agentacct_record_section", + {"source": "codex", "section_id": "limits", "section_status": "started", "files": [f"f{index}.py" for index in range(51)]}, + ) + count_message = too_many["error"]["message"] + assert "50" in count_message and "51" in count_message + + # An over-long check name used to surface as "metadata must be <= 8192 + # bytes", blaming a parameter the caller never sent. + long_check_name = _call_tool( + server, + 4, + "agentacct_record_machine_check", + {"source": "codex", "name": "n" * 300, "result": "passed"}, + ) + name_message = long_check_name["error"]["message"] + assert name_message.startswith("name must be <= 240") + assert "300" in name_message + assert "metadata" not in name_message + + +def test_metadata_budget_measures_real_utf8_bytes_for_cjk(tmp_path): + """The old check json-encoded with ensure_ascii=True, so every CJK character + cost 6 bytes: a summary of 1200 CJK chars plus a next_step of 200 measured + 8432 bytes and was rejected, i.e. about a third of the advertised budget.""" + server = SentinelMCPServer(store_dir=tmp_path / "state") + + accepted = _call_tool( + server, + 1, + "agentacct_record_section", + { + "source": "codex", + "section_id": "cjk", + "section_status": "checkpoint", + "summary": "记" * 1200, + "next_step": "步" * 200, + }, + ) + assert "error" not in accepted + assert _tool_payload(accepted)["event"]["metadata"]["summary"] == "记" * 1200 + + # Still bounded: genuinely oversized metadata fails, and the message names + # the field to shrink instead of a parameter the caller never passed. + oversized = _call_tool( + server, + 2, + "agentacct_record_section", + { + "source": "codex", + "section_id": "cjk-big", + "section_status": "checkpoint", + "metadata": {"notes": "记" * 3000}, + }, + ) + message = oversized["error"]["message"] + assert "8192" in message + assert "notes" in message + # 3000 CJK chars * 3 real UTF-8 bytes, plus the JSON envelope. + assert "received 9013 bytes" in message + + +def test_files_absolute_under_project_dir_is_normalized_not_rejected(tmp_path): + """Roughly 468 sessions hit "files[0] must be project-relative" while the + harness instructed absolute paths and no schema published the rule.""" + server = SentinelMCPServer(store_dir=tmp_path / "state") + + accepted = _tool_payload( + _call_tool( + server, + 1, + "agentacct_record_section", + { + "source": "codex", + "section_id": "paths", + "section_status": "started", + "project_dir": "/repo/agentacct", + "files": ["/repo/agentacct/src/agentacct/mcp.py", "src/./a.py", "src//b.py"], + }, + ) + ) + assert accepted["event"]["metadata"]["files"] == ["src/agentacct/mcp.py", "src/a.py", "src/b.py"] + + outside = _call_tool( + server, + 2, + "agentacct_record_section", + { + "source": "codex", + "section_id": "paths", + "section_status": "started", + "project_dir": "/repo/agentacct", + "files": ["/etc/passwd"], + }, + ) + assert outside["error"]["code"] == -32602 + assert "project-relative" in outside["error"]["message"] + + # A sibling directory sharing the project_dir prefix is NOT inside it. + sibling = _call_tool( + server, + 3, + "agentacct_record_section", + { + "source": "codex", + "section_id": "paths", + "section_status": "started", + "project_dir": "/repo/agentacct", + "files": ["/repo/agentacct-secrets/key.pem"], + }, + ) + assert sibling["error"]["code"] == -32602 + + # Lexical containment must not be fooled by a traversal back out. + escape_via_root = _call_tool( + server, + 4, + "agentacct_record_section", + { + "source": "codex", + "section_id": "paths", + "section_status": "started", + "project_dir": "/repo/agentacct", + "files": ["/repo/agentacct/../../etc/passwd"], + }, + ) + assert escape_via_root["error"]["code"] == -32602 + assert "escape" in escape_via_root["error"]["message"] + + # A redundant separator is the same POSIX path, and must not rebuild an + # absolute-looking value out of the relativized remainder. + redundant = _tool_payload( + _call_tool( + server, + 90, + "agentacct_record_section", + { + "source": "codex", + "section_id": "paths", + "section_status": "started", + "project_dir": "/repo/agentacct", + "files": ["/repo/agentacct//etc/passwd"], + }, + ) + ) + assert redundant["event"]["metadata"]["files"] == ["etc/passwd"] + + for index, bad in enumerate( + ( + {"files": ["../outside.py"]}, + {"files": ["~/secrets.env"]}, + {"files": ["/repo/agentacct/src/a.py"]}, # absolute, but no project_dir on the call + {"files": ["."]}, + ), + start=5, + ): + response = _call_tool( + server, + index, + "agentacct_record_section", + {"source": "codex", "section_id": "paths", "section_status": "started", **bad}, + ) + assert response["error"]["code"] == -32602, bad + + +def test_files_rule_is_published_in_every_schema_that_takes_files(tmp_path): + server = SentinelMCPServer(store_dir=tmp_path / "state") + tools = server.handle_message({"jsonrpc": "2.0", "id": 1, "method": "tools/list", "params": {}}) + tool_by_name = {tool["name"]: tool for tool in tools["result"]["tools"]} + + for tool_name in ("agentacct_record_section", "agentacct_record_machine_check"): + description = tool_by_name[tool_name]["inputSchema"]["properties"]["files"]["description"] + assert "Project-relative" in description + assert "forward slashes" in description + assert "'..'" in description + assert "project_dir" in description + assert tool_by_name["agentacct_record_machine_check"]["inputSchema"]["properties"]["name"]["maxLength"] == 240 + + +def test_machine_check_normalizes_absolute_files_under_project_dir(tmp_path): + server = SentinelMCPServer(store_dir=tmp_path / "state") + + payload = _tool_payload( + _call_tool( + server, + 1, + "agentacct_record_machine_check", + { + "source": "codex", + "section_id": "s1", + "result": "passed", + "project_dir": "/repo/agentacct", + "files": ["/repo/agentacct/tests/test_mcp.py"], + }, + ) + ) + assert payload["event"]["metadata"]["files"] == ["tests/test_mcp.py"] + + +def test_mangled_tool_call_is_warned_about_never_repaired(tmp_path): + """A mangled tool call absorbs parameters into a narrative value as literal + text. Warn and stamp; never reject, never repair — repair would fabricate + fields the agent never wrote.""" + server = SentinelMCPServer(store_dir=tmp_path / "state") + + payload = _tool_payload( + _call_tool( + server, + 1, + "agentacct_record_section", + { + "source": "codex", + "section_id": "mangled", + "section_status": "completed", + "summary": "Fixed the validator.\nsrc/agentacct/mcp.py", + }, + ) + ) + metadata = payload["event"]["metadata"] + assert metadata["mangled_tool_call_suspected_fields"] == ["files"] + # Never repaired: no files were invented from the narrative text. + assert "files" not in metadata + assert metadata["summary"].endswith("") + assert any("" in warning and "did NOT" in warning for warning in payload["warnings"]) + + # The same detector guards the other narrative write path. + check = _tool_payload( + _call_tool( + server, + 2, + "agentacct_record_machine_check", + { + "source": "codex", + "section_id": "mangled", + "result": "passed", + "summary": "Suite green.\npytest -q", + }, + ) + ) + assert check["event"]["metadata"]["mangled_tool_call_suspected_fields"] == ["command"] + assert "command" not in check["event"]["metadata"] + assert any("" in warning for warning in check["warnings"]) + + +def test_mangle_detector_ignores_prose_that_merely_mentions_fields(tmp_path): + """A bare-word detector fires 277 times on "source" and 192 on "files" in the + real ledger, and an opening-tag detector has a real false positive on prose + about MCP config. Closing tags only, and only for unsupplied properties.""" + server = SentinelMCPServer(store_dir=tmp_path / "state") + + ordinary = _tool_payload( + _call_tool( + server, + 1, + "agentacct_record_section", + { + "source": "codex", + "section_id": "prose", + "section_status": "completed", + "summary": "Reviewed the files and the source list; and tags are discussed in the MCP config docs.", + "blocker": "Waiting on the next_step from review.", + }, + ) + ) + assert "mangled_tool_call_suspected_fields" not in ordinary["event"]["metadata"] + assert not any("mangled tool call" in warning for warning in ordinary["warnings"]) + + # A closing tag for an argument the call DID supply is not evidence of a + # mangled call — the parameter arrived where it belongs. + supplied = _tool_payload( + _call_tool( + server, + 2, + "agentacct_record_section", + { + "source": "codex", + "section_id": "prose-2", + "section_status": "completed", + "summary": "Documented the closing tag.", + "files": ["docs/mcp.md"], + }, + ) + ) + assert "mangled_tool_call_suspected_fields" not in supplied["event"]["metadata"] + + +def test_mangle_marker_is_server_authored_and_cannot_be_forged(tmp_path): + server = SentinelMCPServer(store_dir=tmp_path / "state") + + payload = _tool_payload( + _call_tool( + server, + 1, + "agentacct_record_section", + { + "source": "codex", + "section_id": "forge", + "section_status": "started", + "metadata": {"mangled_tool_call_suspected_fields": ["client_session_id"]}, + }, + ) + ) + metadata = payload["event"]["metadata"] + assert "mangled_tool_call_suspected_fields" not in metadata + assert "mangled_tool_call_suspected_fields" in metadata["reserved_context_keys_stripped"] From 33b61e979b7c8ec27f2f505d05bc0236f4ab4919 Mon Sep 17 00:00:00 2001 From: mikehasa Date: Fri, 31 Jul 2026 02:07:29 +0800 Subject: [PATCH 2/9] fix(mcp): undo the rejections the hardening pass introduced 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 ": ", 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 ``, 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. --- src/agentacct/api.py | 10 +- src/agentacct/cli.py | 11 +- src/agentacct/mcp.py | 171 ++++++++++++------- src/agentacct/storage.py | 31 ++++ tests/test_mcp.py | 360 +++++++++++++++++++++++++++++++++++++-- 5 files changed, 493 insertions(+), 90 deletions(-) diff --git a/src/agentacct/api.py b/src/agentacct/api.py index 158c131..4866db1 100644 --- a/src/agentacct/api.py +++ b/src/agentacct/api.py @@ -118,7 +118,7 @@ select_session_projection_envelopes, ) from .source_discovery import UsageSourceDiscovery, discover_usage_sources -from .storage import validate_run_id +from .storage import METADATA_MAX_BYTES, json_utf8_size, validate_run_id from .store_resolution import is_recognized_global_store from .supervisor import OwnedSupervisor, SupervisorAlreadyRunning, SupervisorError from .task_continuations import ClientSessionRef, ContinuationTaskError, ContinuationTaskStore @@ -295,9 +295,11 @@ def validate_optional_run_id(cls, value: str | None) -> str | None: @field_validator("metadata") @classmethod def validate_metadata_size(cls, value: dict[str, Any]) -> dict[str, Any]: - encoded = json.dumps(value, sort_keys=True) - if len(encoded.encode("utf-8")) > 8192: - raise ValueError("metadata must be <= 8192 bytes when JSON encoded") + # Real UTF-8 bytes, measured by the same helper the CLI and MCP lanes + # use: the escaped encoding billed CJK text 2x, so the same payload was + # accepted on one surface and rejected on another. + if json_utf8_size(value) > METADATA_MAX_BYTES: + raise ValueError(f"metadata must be <= {METADATA_MAX_BYTES} bytes when JSON encoded") return value diff --git a/src/agentacct/cli.py b/src/agentacct/cli.py index 747ce21..9fbad62 100644 --- a/src/agentacct/cli.py +++ b/src/agentacct/cli.py @@ -138,7 +138,7 @@ from .reports import build_run_report_payload from .runner import RunOptions, start_guarded_run from .service import SentinelService, SessionObservationConflict -from .storage import RunStore, validate_run_id +from .storage import METADATA_MAX_BYTES, RunStore, json_utf8_size, validate_run_id from .supervisor import OwnedSupervisor, SupervisorError from .source_discovery import discover_usage_sources from . import store_merge @@ -686,11 +686,14 @@ def _parse_metadata_json(value: str | None) -> dict[str, object]: if not isinstance(parsed, dict): raise typer.BadParameter("--metadata-json must decode to a JSON object") try: - encoded = json.dumps(parsed, sort_keys=True, allow_nan=False) + # Real UTF-8 bytes, measured by the same helper the HTTP and MCP lanes + # use: the escaped encoding billed CJK text 2x, so the same payload was + # accepted on one surface and rejected on another. + size = json_utf8_size(parsed, allow_nan=False) except ValueError as exc: raise typer.BadParameter(f"--metadata-json must be strict JSON: {exc}") from exc - if len(encoded.encode("utf-8")) > 8192: - raise typer.BadParameter("--metadata-json must be <= 8192 bytes when JSON encoded") + if size > METADATA_MAX_BYTES: + raise typer.BadParameter(f"--metadata-json must be <= {METADATA_MAX_BYTES} bytes when JSON encoded") return parsed diff --git a/src/agentacct/mcp.py b/src/agentacct/mcp.py index 939be19..91fbe6c 100644 --- a/src/agentacct/mcp.py +++ b/src/agentacct/mcp.py @@ -19,30 +19,35 @@ ) from .install_guide import MCP_SERVER_INSTRUCTIONS from .service import RESERVED_CLIENT_CONTEXT_PROVENANCE_KEYS, SentinelService -from .storage import validate_run_id +from .storage import METADATA_MAX_BYTES, json_utf8_size, validate_run_id WORK_KINDS = {"planning", "implementation", "debugging", "testing", "review", "docs", "refactor", "research", "other", "unknown"} EVIDENCE_TYPES = {"test", "build", "lint", "typecheck", "smoke", "benchmark", "browser", "security", "artifact", "other"} EVIDENCE_RESULTS = {"passed", "failed", "skipped", "error", "unknown"} -# Metadata byte budget. Deliberately identical to the HTTP (api.py) and CLI -# (cli.py) write paths: the number is load-bearing for cross-surface symmetry, -# so raise it in all three or in none. -METADATA_MAX_BYTES = 8192 -# A machine check's name feeds the check-identity hash and lands in metadata -# twice, so it needs its OWN limit: without one an over-long name surfaced as -# "metadata must be <= 8192 bytes", blaming a parameter the caller never sent. -# Never truncate it — a truncated name forks check identity. -MACHINE_CHECK_NAME_MAX_LENGTH = 240 +# A machine check's `name` has no length limit of its own, on purpose. A cap of +# 240 rejected a 241-8000 character band that recorded fine before it, and a +# ~300-character name (an agent recording the full pytest invocation it ran) is +# ordinary. The only ceiling is the shared metadata budget, which is where the +# name lands. It is never truncated either: `name` feeds the check-identity +# hash, so a truncated name forks the identity of the check it names. +# +# Measured caveat: past the budget the size error names the LARGEST metadata +# field, and for a machine check that is the `summary` the server synthesizes +# as ": " — marginally bigger than the name itself. So a name +# that blows the whole budget is still reported one step removed. That is the +# 8000+ case only, and it is no worse than the un-named "metadata must be <= +# 8192 bytes" it replaced; the band that actually regressed is fixed. # The files rule, published in every schema that takes `files`. It is the single # biggest MCP rejection cause: agent harnesses hand out absolute paths, and the # rule appeared in no description at all. FILES_DESCRIPTION = ( - "Project-relative paths with forward slashes: no leading '/' or '~', no '.' or '..' segments. " + "Project-relative paths with forward slashes: no leading '/' or '~', no '..' segments. " "An absolute path is relativized and accepted ONLY when it lies under the project_dir supplied " - "on this same call; otherwise it is rejected rather than guessed at." + "on this same call; otherwise it is rejected rather than guessed at. An entry naming the project " + "root itself ('.') is dropped rather than stored, and never fails the call." ) # Join keys that stay valid for a whole client session, so sections recorded on @@ -85,7 +90,7 @@ "type": "object", "properties": { "run_id": {"type": "string", "default": "latest"}, - "name": {"type": "string", "default": "check", "maxLength": MACHINE_CHECK_NAME_MAX_LENGTH}, + "name": {"type": "string", "default": "check"}, "before_exit_code": {"type": ["integer", "null"]}, "after_exit_code": {"type": ["integer", "null"]}, "before_summary": {"type": ["string", "null"]}, @@ -323,12 +328,10 @@ def _limit_error(key: str, *, limit: int, received: int, unit: str = "characters return InvalidParams(f"{key} must be <= {limit} {unit} (received {received})") -def _optional_str(args: dict[str, Any], key: str, default: str, *, max_length: int | None = None) -> str: +def _optional_str(args: dict[str, Any], key: str, default: str) -> str: value = args.get(key, default) if not isinstance(value, str) or not value: raise InvalidParams(f"{key} must be a non-empty string") - if max_length is not None and len(value) > max_length: - raise _limit_error(key, limit=max_length, received=len(value)) return value @@ -433,23 +436,14 @@ def _optional_metadata(args: dict[str, Any]) -> dict[str, Any]: def _json_utf8_size(value: Any) -> int: - """Real UTF-8 byte size of the JSON encoding. - - ``ensure_ascii=False`` is the whole point: with the default, the size check - bills every CJK character 6 bytes (\\uXXXX) and every emoji 12, i.e. 2-4x - what the text actually costs, so a Chinese-writing agent got roughly a - third of the advertised budget. Measured before this fix: 1200 CJK chars = - 7215 bytes, and 1200 CJK chars of summary plus 200 of next_step = 8432 - bytes, rejected. + """This lane's strictness applied to the shared measure. + + The measure itself lives in storage.py because all three write surfaces + have to reach the same accept/reject decision; ``allow_nan=False`` is the + part that is local to MCP and the CLI, whose callers may not send + non-standard JSON constants. """ - encoded = json.dumps(value, sort_keys=True, allow_nan=False, ensure_ascii=False) - try: - return len(encoded.encode("utf-8")) - except UnicodeEncodeError: - # Lone surrogates have no UTF-8 form. Measure the escaped encoding - # rather than failing a call on a pathological string; the caller's - # rejection, if any, must still be about SIZE. - return len(json.dumps(value, sort_keys=True, allow_nan=False).encode("utf-8")) + return json_utf8_size(value, allow_nan=False) def _metadata_size_error(value: dict[str, Any], size: int) -> InvalidParams: @@ -457,9 +451,10 @@ def _metadata_size_error(value: dict[str, Any], size: int) -> InvalidParams: The old message named `metadata` — a parameter most callers never passed, since the bulk is usually a validated argument (summary, blocker, - next_step) the server merged in. Naming the largest field, with its byte - size, is the difference between a fixable error and a guessing game. The - key name is caller-controlled, so it is clipped before it reaches the + next_step, name) the server merged in. Naming the largest field, with its + byte size, is the difference between a fixable error and a guessing game, + and it is why no argument needs a private length cap on top of this one. + The key name is caller-controlled, so it is clipped before it reaches the error string. """ largest_key = "" @@ -498,6 +493,15 @@ def _validate_metadata_size(value: dict[str, Any]) -> None: SECTION_NARRATIVE_KEYS = ("summary", "blocker", "next_step", "section_title", "title") MACHINE_CHECK_NARRATIVE_KEYS = ("summary", "before_summary", "after_summary", "resolution_summary", "command") +# Property names that are NOT eligible to be suspected, however they appear in +# free text. `title` is here because it is a plain English word and `` +# is the most common closing tag in HTML: "the page head has +# Report in it" is ordinary prose, not a mangled call. It is +# also the one property added after the detector was calibrated, so leaving it +# in the candidate set would have raised the false-positive rate above the +# measured figure without anyone re-measuring. +MANGLE_DETECTOR_INELIGIBLE_PROPERTIES = frozenset({"title"}) + def _tool_property_names(tool_name: str) -> frozenset[str]: for tool in TOOLS: @@ -509,15 +513,23 @@ def _tool_property_names(tool_name: str) -> frozenset[str]: def _detect_mangled_tool_call_fields(tool_name: str, args: dict[str, Any], narrative_keys: Sequence[str]) -> list[str]: """This tool's own argument names that appear as CLOSING tags in free text. - Closing tags only, and only for properties the call did NOT supply. The - looser forms were measured against the real ledger and are unusable: a - bare-word detector fires 277 times on "source" and 192 on "files" across - 1955 section events, and an opening-tag detector has a real false positive - on legitimate prose about MCP config. The closing-tag form scored 1 true - positive and 0 false positives on the same events. It will still fire on - meta-work about agentacct itself, which is acceptable for a warning. + Closing tags only, and only for properties the call did NOT supply and that + are not in MANGLE_DETECTOR_INELIGIBLE_PROPERTIES. The looser forms were + measured against the real ledger and are unusable: a bare-word detector + fires 277 times on "source" and 192 on "files" across 1955 section events, + and an opening-tag detector has a real false positive on legitimate prose + about MCP config. The closing-tag form scored 1 true positive and 0 false + positives on the same events. + + Read that calibration for exactly what it is: it was measured on the + section property set as it stood BEFORE this change, i.e. without `title`, + and it has not been re-measured since. That is the reason `title` is + excluded rather than merely noted — the alternative is quoting a + false-positive rate for a detector that no longer matches the one measured. + The detector will still fire on meta-work about agentacct itself, which is + acceptable for a warning that never rejects or repairs. """ - missing = _tool_property_names(tool_name) - set(args) + missing = _tool_property_names(tool_name) - set(args) - MANGLE_DETECTOR_INELIGIBLE_PROPERTIES if not missing: return [] suspected: set[str] = set() @@ -647,6 +659,11 @@ def _relative_to_project_dir(path: str, root: str | None) -> str | None: """ if not root or not _looks_absolute(root) or root.startswith("~"): return None + if path == root: + # The project root named absolutely. The empty remainder is what the + # caller sees dropped, so "/repo" and "/repo/" behave alike instead of + # one being fatal and the other cosmetic. + return "" prefix = root + "/" if not path.startswith(prefix): return None @@ -656,10 +673,18 @@ def _relative_to_project_dir(path: str, root: str | None) -> str | None: return path[len(prefix) :].lstrip("/") +def _not_project_relative_error(key: str, index: int) -> InvalidParams: + return InvalidParams( + f"{key}[{index}] must be project-relative: forward slashes, no leading '/' or '~', " + "no '..' segments. Pass the path relative to the repository root, or supply " + "project_dir on this call so an absolute path under it can be relativized." + ) + + def _optional_project_relative_files( args: dict[str, Any], key: str = "files", *, project_dir: str | None = None ) -> list[str]: - """Validate AND normalize the files list into project-relative POSIX paths. + """Validate the files list and normalize it into project-relative paths. Absolute paths are the single biggest MCP rejection cause — agent harnesses instruct absolute paths and the rule was published nowhere — so an absolute @@ -667,32 +692,48 @@ def _optional_project_relative_files( project_dir declared ON THIS CALL. Only the explicit argument counts: an inherited or session-attached project_dir could belong to a different repository, and a path relativized against the wrong root is a silently - wrong ledger row, which is worse than a rejection. - - Normalization also closes the gap where ``src/./a.py`` passed the old - validator (PurePosixPath drops "." segments before they were inspected) and - reached the ledger un-normalized, splitting one file into two rows. + wrong ledger row, which is worse than a rejection. The relativized + remainder is then put through the SAME check as any other value, so + "/repo/~/x" cannot deposit a "~/x" the published rule calls impossible. + + Two rules keep a stored value honest about the file it names: + + * Backslashes are folded for VALIDATION only. On POSIX ``weird\\name.py`` + is a legal filename, and folding it into the stored value would merge it + with a genuinely different file called ``weird/name.py``. What the caller + sent is what gets stored (modulo redundant "." and "/" segments, which + denote the same path either way — that normalization is what stops + ``src/./a.py`` and ``src/a.py`` becoming two rows for one file). + * An entry naming the project root itself (".", "./", or an absolute path + equal to project_dir) is DROPPED, not rejected. It names no file, so + storing it would be a row for a path that does not exist — but failing + the whole call, and losing a real section or machine check, over one + cosmetic entry is the behaviour this validator exists to stop. """ files = _optional_limited_str_list(args, key, max_items=50, max_length=240) root = project_dir.replace("\\", "/").rstrip("/") if project_dir else None normalized_files: list[str] = [] for index, item in enumerate(files): - candidate = item.replace("\\", "/") - if _looks_absolute(candidate): - relative = _relative_to_project_dir(candidate, root) + folded = item.replace("\\", "/") + stored = item + if _looks_absolute(folded): + relative = _relative_to_project_dir(folded, root) if relative is None: - raise InvalidParams( - f"{key}[{index}] must be project-relative: forward slashes, no leading '/' or '~', " - "no '.' or '..' segments. Pass the path relative to the repository root, or supply " - "project_dir on this call so an absolute path under it can be relativized." - ) - candidate = relative - parts = PurePosixPath(candidate).parts - if ".." in parts: + raise _not_project_relative_error(key, index) + # The caller sent an absolute path and asked, by supplying + # project_dir, to have it rewritten; here the remainder IS the + # value, so both forms move to it. + folded = stored = relative + if _looks_absolute(folded): + # "/repo/~/x" -> "~/x", "/repo/C:/x" -> "C:/x": still not a + # project-relative path, and the schema says so. + raise _not_project_relative_error(key, index) + if ".." in PurePosixPath(folded).parts: raise InvalidParams(f"{key}[{index}] must not escape the project directory: '..' segments are not allowed") - if not parts: - raise InvalidParams(f"{key}[{index}] must name a path inside the project, not the project root itself") - normalized_files.append("/".join(parts)) + stored_parts = PurePosixPath(stored).parts + if not stored_parts: + continue + normalized_files.append("/".join(stored_parts)) return normalized_files @@ -1056,7 +1097,7 @@ def call_tool(self, name: Any, arguments: Any) -> dict[str, Any]: ) has_outcome_fields = any(key in arguments for key in {"before_exit_code", "after_exit_code", "before_summary", "after_summary"}) or not has_evidence_fields run_id = _optional_str(arguments, "run_id", "latest") - check_name = _optional_str(arguments, "name", "check", max_length=MACHINE_CHECK_NAME_MAX_LENGTH) + check_name = _optional_str(arguments, "name", "check") before_exit_code = _optional_nullable_int(arguments, "before_exit_code") after_exit_code = _optional_nullable_int(arguments, "after_exit_code") before_summary = _optional_nullable_str(arguments, "before_summary") @@ -1423,7 +1464,7 @@ def call_tool(self, name: Any, arguments: Any) -> dict[str, Any]: for key in refusal_stamps: context.pop(key, None) metadata = _metadata_with_context(arguments, context) - size_warnings.append("Inherited client context was dropped: metadata would exceed 8192 bytes when JSON encoded. Trim metadata or pass join keys explicitly.") + size_warnings.append(f"Inherited client context was dropped: metadata would exceed {METADATA_MAX_BYTES} bytes when JSON encoded. Trim metadata or pass join keys explicitly.") # Provenance keys are server-authored: whatever this call did not # set itself is removed, so callers can never forge inheritance. for key in RESERVED_CLIENT_CONTEXT_PROVENANCE_KEYS: diff --git a/src/agentacct/storage.py b/src/agentacct/storage.py index a9e8fd7..dfbeefd 100644 --- a/src/agentacct/storage.py +++ b/src/agentacct/storage.py @@ -33,6 +33,37 @@ def validate_run_id(run_id: str) -> str: return run_id +# The metadata byte budget, shared by all three write surfaces (HTTP api.py, +# CLI cli.py, MCP mcp.py). It lives here, next to the other cross-surface +# validation primitive, because a caller must get the same accept/reject +# decision whichever lane it writes through: the same payload accepted on one +# and rejected on another is a ledger that disagrees with itself. +METADATA_MAX_BYTES = 8192 + + +def json_utf8_size(value: Any, *, allow_nan: bool = True) -> int: + """Real UTF-8 byte size of ``value``'s JSON encoding. + + ``ensure_ascii=False`` is the whole point. With the default, the size is + measured against the ``\\uXXXX`` escape form, which overcharges every + non-ASCII character against what it actually costs on disk. Measured per + character: CJK 6 bytes vs 3 (2x), emoji 12 vs 4 (3x), accented Latin 6 vs 2 + (3x), ASCII 1 vs 1. So an agent writing Chinese got half the advertised + budget — 2000 CJK characters are 6013 real UTF-8 bytes and 12013 escaped. + + ``allow_nan`` is the caller's own strictness, not part of the measurement: + the MCP and CLI lanes reject non-standard JSON constants outright and pass + False, the HTTP lane never has and passes the default. + """ + try: + return len(json.dumps(value, sort_keys=True, allow_nan=allow_nan, ensure_ascii=False).encode("utf-8")) + except UnicodeEncodeError: + # Lone surrogates survive json.loads but have no UTF-8 form. Fall back + # to the escaped encoding rather than failing the call on a + # pathological string: any rejection here must still be about SIZE. + return len(json.dumps(value, sort_keys=True, allow_nan=allow_nan).encode("utf-8")) + + @dataclass class StoredRun: run_id: str diff --git a/tests/test_mcp.py b/tests/test_mcp.py index 170ec8d..596f6eb 100644 --- a/tests/test_mcp.py +++ b/tests/test_mcp.py @@ -8,7 +8,7 @@ from agentacct.outcome import apply_judge_result, write_outcome from agentacct.reports import build_run_report_payload from agentacct.runner import RunOptions, start_guarded_run -from agentacct.storage import RunStore +from agentacct.storage import RunStore, json_utf8_size from agentacct.work_ledger import build_work_ledger @@ -2799,20 +2799,10 @@ def test_limit_errors_state_the_limit_and_what_was_received(tmp_path): {"source": "codex", "section_id": "limits", "section_status": "started", "files": [f"f{index}.py" for index in range(51)]}, ) count_message = too_many["error"]["message"] - assert "50" in count_message and "51" in count_message - - # An over-long check name used to surface as "metadata must be <= 8192 - # bytes", blaming a parameter the caller never sent. - long_check_name = _call_tool( - server, - 4, - "agentacct_record_machine_check", - {"source": "codex", "name": "n" * 300, "result": "passed"}, - ) - name_message = long_check_name["error"]["message"] - assert name_message.startswith("name must be <= 240") - assert "300" in name_message - assert "metadata" not in name_message + # Pinned exactly: "50" and "51" both appear in a message that says nothing + # useful ("files: 50/51 problem"), so a substring pair does not prove the + # limit and the received count were reported as such. + assert count_message == "files must be <= 50 items (received 51)" def test_metadata_budget_measures_real_utf8_bytes_for_cjk(tmp_path): @@ -2946,7 +2936,9 @@ def test_files_absolute_under_project_dir_is_normalized_not_rejected(tmp_path): {"files": ["../outside.py"]}, {"files": ["~/secrets.env"]}, {"files": ["/repo/agentacct/src/a.py"]}, # absolute, but no project_dir on the call - {"files": ["."]}, + # "." is NOT in this list: it names the project root, is dropped + # rather than rejected, and must never fail the call. Covered by + # test_files_entry_naming_the_project_root_is_dropped_not_fatal. ), start=5, ): @@ -2970,7 +2962,10 @@ def test_files_rule_is_published_in_every_schema_that_takes_files(tmp_path): assert "forward slashes" in description assert "'..'" in description assert "project_dir" in description - assert tool_by_name["agentacct_record_machine_check"]["inputSchema"]["properties"]["name"]["maxLength"] == 240 + # A machine check's name carries no private cap: the shared metadata budget + # is the only ceiling, and a cap here would reject names the CLI and HTTP + # lanes accept. See test_machine_check_name_has_no_private_length_cap. + assert "maxLength" not in tool_by_name["agentacct_record_machine_check"]["inputSchema"]["properties"]["name"] def test_machine_check_normalizes_absolute_files_under_project_dir(tmp_path): @@ -3099,3 +3094,334 @@ def test_mangle_marker_is_server_authored_and_cannot_be_forged(tmp_path): metadata = payload["event"]["metadata"] assert "mangled_tool_call_suspected_fields" not in metadata assert "mangled_tool_call_suspected_fields" in metadata["reserved_context_keys_stripped"] + + +def test_machine_check_name_has_no_private_length_cap(tmp_path): + """A 240-character cap turned a 241-8000 band that recorded fine on main + into hard rejections. The band is real: an agent recording the full pytest + invocation it ran routinely writes a ~300-character name. The only ceiling + is the shared metadata budget, whose error now reports a field and a byte + count instead of a bare "metadata must be <= 8192 bytes".""" + server = SentinelMCPServer(store_dir=tmp_path / "state") + + long_name = ".venv/bin/pytest -q " + "tests/test_mcp.py::test_a " * 12 + assert 240 < len(long_name) < 8000 + accepted = _tool_payload( + _call_tool( + server, + 1, + "agentacct_record_machine_check", + {"source": "codex", "name": long_name, "result": "passed"}, + ) + ) + # Never truncated: the name feeds the check-identity hash. + assert accepted["event"]["metadata"]["name"] == long_name + + # Still bounded, by the shared metadata budget: a name large enough to blow + # the whole budget is rejected as a SIZE problem that reports what arrived, + # not by a private cap of its own. + oversized = _call_tool( + server, + 2, + "agentacct_record_machine_check", + {"source": "codex", "name": "n" * 9000, "result": "passed"}, + ) + message = oversized["error"]["message"] + assert message.startswith("metadata must be <= 8192 bytes when JSON encoded (received ") + assert "name must be <=" not in message + # Measured, not assumed: at this size the largest field is the `summary` + # the server synthesizes as ": ", so the blame is still + # indirect in this extreme case. Recorded here so the next reader sees it. + assert "largest field is summary" in message + + +def test_metadata_budget_decision_is_identical_on_all_three_write_surfaces(tmp_path): + """Measuring real UTF-8 bytes on MCP alone broke the symmetry the shared + budget exists for: 2000 CJK characters are 6013 real bytes but 12013 + escaped, so MCP accepted a payload the HTTP lane rejected. Same payload, + same verdict, whichever lane the agent writes through.""" + from agentacct.api import EventRecordRequest + from agentacct.cli import _parse_metadata_json + from agentacct.mcp import _validate_metadata_size + + def verdicts(payload): + results = [] + for attempt in ( + lambda: _validate_metadata_size(payload), + lambda: EventRecordRequest(source="codex", event_type="probe", metadata=payload), + lambda: _parse_metadata_json(json.dumps(payload)), + ): + try: + attempt() + results.append("accepted") + except Exception: + results.append("rejected") + return results + + under = {"notes": "记" * 2000} + assert json_utf8_size(under) == 6013 + assert len(json.dumps(under, sort_keys=True).encode("utf-8")) == 12013 + assert verdicts(under) == ["accepted", "accepted", "accepted"] + + over = {"notes": "记" * 3000} + assert json_utf8_size(over) == 9013 + assert verdicts(over) == ["rejected", "rejected", "rejected"] + + +def test_metadata_size_survives_a_lone_surrogate_on_every_surface(tmp_path): + """json.loads accepts a lone surrogate but it has no UTF-8 form, so a + real-bytes measure raises UnicodeEncodeError mid-validation and fails the + call with a crash instead of a verdict.""" + from agentacct.api import EventRecordRequest + from agentacct.mcp import _validate_metadata_size + + lone_surrogate = json.loads('"\\ud800"') + assert _validate_metadata_size({"notes": lone_surrogate}) is None + assert EventRecordRequest(source="codex", event_type="probe", metadata={"notes": lone_surrogate}) + assert json_utf8_size({"notes": lone_surrogate}) > 0 + + server = SentinelMCPServer(store_dir=tmp_path / "state") + recorded = _tool_payload( + _call_tool( + server, + 1, + "agentacct_record_section", + {"source": "codex", "section_id": "surrogate", "section_status": "started", "summary": lone_surrogate}, + ) + ) + assert recorded["event"]["metadata"]["summary"] == lone_surrogate + + # Oversized-and-unencodable still fails as SIZE, never as an encoding crash. + oversized = _call_tool( + server, + 2, + "agentacct_record_section", + { + "source": "codex", + "section_id": "surrogate", + "section_status": "started", + "metadata": {"notes": lone_surrogate * 3000}, + }, + ) + assert "bytes" in oversized["error"]["message"] + + +def test_files_entry_naming_the_project_root_is_dropped_not_fatal(tmp_path): + """A whole-repo section with files=["."] recorded fine on main. Killing the + whole record to reject one cosmetic path entry is exactly the behaviour + this validator exists to remove.""" + server = SentinelMCPServer(store_dir=tmp_path / "state") + + for msg_id, entry in enumerate((".", "./"), start=1): + payload = _tool_payload( + _call_tool( + server, + msg_id, + "agentacct_record_section", + {"source": "codex", "section_id": "whole-repo", "section_status": "started", "files": [entry]}, + ) + ) + # Accepted, and no row is stored for a value that names no file. + assert "error" not in payload, entry + assert payload["event"]["metadata"].get("files", []) == [], entry + + mixed = _tool_payload( + _call_tool( + server, + 3, + "agentacct_record_section", + {"source": "codex", "section_id": "whole-repo", "section_status": "started", "files": [".", "src/a.py"]}, + ) + ) + assert mixed["event"]["metadata"]["files"] == ["src/a.py"] + + # An absolute path equal to the declared project root says the same thing, + # with or without the trailing slash: one must not be fatal while the other + # is cosmetic. + for msg_id, entry in enumerate(("/repo/agentacct/", "/repo/agentacct"), start=4): + at_root = _tool_payload( + _call_tool( + server, + msg_id, + "agentacct_record_machine_check", + { + "source": "codex", + "result": "passed", + "project_dir": "/repo/agentacct", + "files": [entry], + }, + ) + ) + assert "error" not in at_root, entry + assert at_root["event"]["metadata"].get("files", []) == [], entry + + +def test_files_backslash_is_folded_for_validation_but_never_for_storage(tmp_path): + """On macOS/Linux `weird\\name.py` is a legal filename. main folded + backslashes for VALIDATION and stored the original; storing the folded form + silently merges that file with a genuinely different `weird/name.py`.""" + server = SentinelMCPServer(store_dir=tmp_path / "state") + + stored = _tool_payload( + _call_tool( + server, + 1, + "agentacct_record_section", + { + "source": "codex", + "section_id": "backslash", + "section_status": "started", + "files": ["weird\\name.py", "weird/name.py"], + }, + ) + ) + # Two distinct real paths must stay two distinct rows. + assert stored["event"]["metadata"]["files"] == ["weird\\name.py", "weird/name.py"] + + # Folding still happens for validation, so a Windows-style traversal is + # caught rather than waved through as one opaque segment. + escaped = _call_tool( + server, + 2, + "agentacct_record_section", + { + "source": "codex", + "section_id": "backslash", + "section_status": "started", + "files": ["src\\..\\..\\etc\\passwd"], + }, + ) + assert escaped["error"]["code"] == -32602 + assert "escape" in escaped["error"]["message"] + + +def test_relativized_remainder_is_revalidated_against_the_published_rule(tmp_path): + """The remainder was only checked for '..', so project_dir="/repo" with + files=["/repo/~/x"] stored "~/x" -- a value the schema description this + server publishes says is impossible.""" + server = SentinelMCPServer(store_dir=tmp_path / "state") + + for msg_id, entry in enumerate(("/repo/~/x.py", "/repo/C:/x.py"), start=1): + response = _call_tool( + server, + msg_id, + "agentacct_record_section", + { + "source": "codex", + "section_id": "remainder", + "section_status": "started", + "project_dir": "/repo", + "files": [entry], + }, + ) + assert response["error"]["code"] == -32602, entry + assert "project-relative" in response["error"]["message"], entry + + +def test_windows_drive_letter_is_absolute_for_validation(tmp_path): + """Backslashes are folded before the absolute check, so PurePosixPath alone + reads "C:/repo/a.py" as a relative path and stores it as one.""" + server = SentinelMCPServer(store_dir=tmp_path / "state") + + rejected = _call_tool( + server, + 1, + "agentacct_record_section", + {"source": "codex", "section_id": "drive", "section_status": "started", "files": ["C:\\repo\\a.py"]}, + ) + assert rejected["error"]["code"] == -32602 + assert "project-relative" in rejected["error"]["message"] + + # With the drive-letter root declared on the call it relativizes normally. + accepted = _tool_payload( + _call_tool( + server, + 2, + "agentacct_record_section", + { + "source": "codex", + "section_id": "drive", + "section_status": "started", + "project_dir": "C:\\repo", + "files": ["C:\\repo\\src\\a.py"], + }, + ) + ) + assert accepted["event"]["metadata"]["files"] == ["src/a.py"] + + +def test_tilde_project_dir_never_anchors_a_relativization(tmp_path): + """"~" is a shell shortcut, not a root: two machines expand it to different + directories, so containment under it is not provable and must not be + claimed.""" + server = SentinelMCPServer(store_dir=tmp_path / "state") + + response = _call_tool( + server, + 1, + "agentacct_record_section", + { + "source": "codex", + "section_id": "tilde", + "section_status": "started", + "project_dir": "~/repo", + "files": ["~/repo/src/a.py"], + }, + ) + assert response["error"]["code"] == -32602 + assert "project-relative" in response["error"]["message"] + + # A relative project_dir is equally unusable as an anchor. + relative_root = _call_tool( + server, + 2, + "agentacct_record_section", + { + "source": "codex", + "section_id": "tilde", + "section_status": "started", + "project_dir": "repo", + "files": ["/repo/src/a.py"], + }, + ) + assert relative_root["error"]["code"] == -32602 + + +def test_mangle_detector_does_not_fire_on_a_closing_title_tag(tmp_path): + """Adding `title` as a record_section property widened the detector onto + ``, the most common closing tag in HTML prose. The 1-true-positive + /0-false-positive calibration was measured WITHOUT `title` in the property + set, so leaving it in would quote a rate for a detector nobody measured.""" + server = SentinelMCPServer(store_dir=tmp_path / "state") + + ordinary = _tool_payload( + _call_tool( + server, + 1, + "agentacct_record_section", + { + "source": "codex", + "section_id": "html", + "section_status": "completed", + "summary": "The page head has Report in it.", + }, + ) + ) + assert "mangled_tool_call_suspected_fields" not in ordinary["event"]["metadata"] + assert not [warning for warning in ordinary["warnings"] if "mangled" in warning] + + # The detector is still armed for every other unsupplied property. + mangled = _tool_payload( + _call_tool( + server, + 2, + "agentacct_record_section", + { + "source": "codex", + "section_id": "html", + "section_status": "completed", + "summary": "The page head has Report in it.", + }, + ) + ) + assert mangled["event"]["metadata"]["mangled_tool_call_suspected_fields"] == ["next_step"] From 45da449a35a6fdb401f0e0179e7d1c6067bb77b8 Mon Sep 17 00:00:00 2001 From: mikehasa Date: Fri, 31 Jul 2026 03:06:25 +0800 Subject: [PATCH 3/9] fix(mcp): stop failing Windows paths, calibrate the mangle detector, 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): ``, `` and `` never occur, and `` 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 `` 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. --- src/agentacct/mcp.py | 180 +++++++++++++++++++++++++----------- tests/test_mcp.py | 211 ++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 325 insertions(+), 66 deletions(-) diff --git a/src/agentacct/mcp.py b/src/agentacct/mcp.py index 91fbe6c..6be8424 100644 --- a/src/agentacct/mcp.py +++ b/src/agentacct/mcp.py @@ -27,18 +27,24 @@ EVIDENCE_RESULTS = {"passed", "failed", "skipped", "error", "unknown"} # A machine check's `name` has no length limit of its own, on purpose. A cap of -# 240 rejected a 241-8000 character band that recorded fine before it, and a +# 240 rejected a 241-4036 character band that recorded fine before it, and a # ~300-character name (an agent recording the full pytest invocation it ran) is # ordinary. The only ceiling is the shared metadata budget, which is where the # name lands. It is never truncated either: `name` feeds the check-identity # hash, so a truncated name forks the identity of the check it names. # +# 4036 is measured, not assumed: binary-searching a {source, name, result} call +# puts the largest accepted `name` at 4036 characters and the first rejection at +# 4037, identically on this branch and on the 0.5.2 release it branched from. +# The band is that much narrower than the 8000 first claimed here because `name` +# lands in the budget TWICE — once as itself, once inside the summary below. +# # Measured caveat: past the budget the size error names the LARGEST metadata # field, and for a machine check that is the `summary` the server synthesizes -# as "<name>: <result>" — marginally bigger than the name itself. So a name -# that blows the whole budget is still reported one step removed. That is the -# 8000+ case only, and it is no worse than the un-named "metadata must be <= -# 8192 bytes" it replaced; the band that actually regressed is fixed. +# as "<name>: <result>" — 4060 bytes against the name's own 4049 at the 4037 +# boundary. So the blame is one step removed for EVERY over-budget name, not +# just extreme ones. It is still no worse than the un-named "metadata must be +# <= 8192 bytes" it replaced; the band that actually regressed is fixed. # The files rule, published in every schema that takes `files`. It is the single # biggest MCP rejection cause: agent harnesses hand out absolute paths, and the @@ -46,8 +52,9 @@ FILES_DESCRIPTION = ( "Project-relative paths with forward slashes: no leading '/' or '~', no '..' segments. " "An absolute path is relativized and accepted ONLY when it lies under the project_dir supplied " - "on this same call; otherwise it is rejected rather than guessed at. An entry naming the project " - "root itself ('.') is dropped rather than stored, and never fails the call." + "on this same call. One that is not is rejected rather than guessed at, except a Windows path " + "(C:\\...), which is kept as-is with its separators normalized. An entry naming the project root " + "itself ('.') is dropped rather than stored. Neither dropping nor a Windows path fails the call." ) # Join keys that stay valid for a whole client session, so sections recorded on @@ -494,13 +501,28 @@ def _validate_metadata_size(value: dict[str, Any]) -> None: MACHINE_CHECK_NARRATIVE_KEYS = ("summary", "before_summary", "after_summary", "resolution_summary", "command") # Property names that are NOT eligible to be suspected, however they appear in -# free text. `title` is here because it is a plain English word and `` -# is the most common closing tag in HTML: "the page head has -# Report in it" is ordinary prose, not a mangled call. It is -# also the one property added after the detector was calibrated, so leaving it -# in the candidate set would have raised the false-positive rate above the -# measured figure without anyone re-measuring. -MANGLE_DETECTOR_INELIGIBLE_PROPERTIES = frozenset({"title"}) +# free text: the ones that are also element names in the HTML or SVG +# vocabularies, which is what an agent describing web work actually writes. +# Hand-picking `title` alone was the inconsistency — it excluded one markup +# name by that reasoning and left `summary` and `metadata`, which are markup +# names by the same reasoning, armed. +# +# Calibrated READ-ONLY against the real 6261-event global ledger, scoped to the +# 3535 events these two tools write (1956 section, 1579 evidence) and to the +# narrative values the detector actually reads: +# +# * ``, ``, ``: 0 occurrences. +# * ``: 1 occurrence per lane, both inside confirmed mangled calls +# that DID supply `summary`, so the detector could not have fired on it. +# * `` (opening form): 2 occurrences in ordinary prose — "tooltips = +# SVG <title>" and "dashboard <title>". That is the measured evidence that +# these names collide with real writing; only the closing form has yet to +# turn up in 3535 events. +# +# So excluding these four costs zero measured true positives. `source` is inert +# — it is required on both tools, so it is never an unsupplied property — and +# is listed only to keep the rule complete rather than case-by-case. +MANGLE_DETECTOR_INELIGIBLE_PROPERTIES = frozenset({"title", "summary", "metadata", "source"}) def _tool_property_names(tool_name: str) -> frozenset[str]: @@ -515,19 +537,33 @@ def _detect_mangled_tool_call_fields(tool_name: str, args: dict[str, Any], narra Closing tags only, and only for properties the call did NOT supply and that are not in MANGLE_DETECTOR_INELIGIBLE_PROPERTIES. The looser forms were - measured against the real ledger and are unusable: a bare-word detector - fires 277 times on "source" and 192 on "files" across 1955 section events, - and an opening-tag detector has a real false positive on legitimate prose - about MCP config. The closing-tag form scored 1 true positive and 0 false - positives on the same events. - - Read that calibration for exactly what it is: it was measured on the - section property set as it stood BEFORE this change, i.e. without `title`, - and it has not been re-measured since. That is the reason `title` is - excluded rather than merely noted — the alternative is quoting a - false-positive rate for a detector that no longer matches the one measured. - The detector will still fire on meta-work about agentacct itself, which is - acceptable for a warning that never rejects or repairs. + re-measured against the real 6261-event ledger with the CURRENT property + set, and are unusable: scoped to the 1956 section events, a bare-word + detector fires on "source" in 170 of them and on "files" in 130, and an + opening-tag detector fires on `<title>` in 2 events of ordinary prose about + SVG tooltips and the dashboard's page title. + + The closing-tag form scores 2 true positives and 0 false positives across + all 3535 section+evidence events. Exactly two events contain any closing + tag at all, and both are genuine mangled calls that absorbed + `</summary><next_step>...</next_step><files>[...]</files><project_dir>...` + into the summary; the names that fired are files, next_step and + project_dir. (The previous figure here, "1 true positive across 1955 + section events", undercounted: the second true positive is in the evidence + lane, and the bare-word counts it quoted were measured over all 6261 + events rather than the section subset it credited them to.) + + Two limits, stated because the numbers alone overstate the case. No + property name has EVER appeared as a closing tag in real prose, so this + ledger licenses no exclusion on its own — the ineligible set above rests on + the measured `<title>` prose plus the vocabulary rule that generalizes it. + And the corpus is agentacct dogfooding itself: only 51 of the 3535 events + mention SVG/HTML/XML at all, so it is weak evidence that the names left + armed are safe on markup-heavy prose. `client` is the known gap — it fires + on XML prose but is in neither vocabulary and has 0 occurrences here, so + nothing measured justifies removing it. The detector will also still fire + on meta-work about agentacct itself, which is acceptable for a warning that + never rejects and never repairs. """ missing = _tool_property_names(tool_name) - set(args) - MANGLE_DETECTOR_INELIGIBLE_PROPERTIES if not missing: @@ -645,8 +681,15 @@ def _looks_absolute(path: str) -> bool: return path.startswith(("/", "~")) or bool(_WINDOWS_DRIVE_PREFIX.match(path)) -def _relative_to_project_dir(path: str, root: str | None) -> str | None: - """``path`` expressed relative to ``root``, or None when containment is not provable. +def _relative_offset_under_project_dir(path: str, root: str | None) -> int | None: + """Index in ``path`` where the part relative to ``root`` begins, or None when + containment is not provable. + + An offset rather than the substring itself, because the caller holds two + index-aligned views of the same value — what arrived, and the + backslash-folded copy the containment check reads — and both have to be cut + at the same place for the stored path to keep the separators the caller + sent. See _optional_project_relative_files for why that matters. Purely lexical: no filesystem access and no symlink resolution. That is the correct boundary here — this decides which STRING lands in the ledger, not @@ -654,8 +697,8 @@ def _relative_to_project_dir(path: str, root: str | None) -> str | None: does not exist on the machine reading the store later. Both values are caller-controlled, so nothing is inferred: a root that is not itself absolute, or a path that is not under it, returns None and the caller - rejects. Any ``..`` surviving into the returned value is caught by the - escape check at the call site. + decides. Any ``..`` surviving past the offset is caught by the escape check + at the call site. """ if not root or not _looks_absolute(root) or root.startswith("~"): return None @@ -663,14 +706,17 @@ def _relative_to_project_dir(path: str, root: str | None) -> str | None: # The project root named absolutely. The empty remainder is what the # caller sees dropped, so "/repo" and "/repo/" behave alike instead of # one being fatal and the other cosmetic. - return "" + return len(path) prefix = root + "/" if not path.startswith(prefix): return None - # lstrip: a redundant separator ("/repo//etc/passwd") is the same POSIX path - # as "/repo/etc/passwd", but leaving the leading slash on the remainder - # would rebuild an absolute-looking "//etc/passwd" in the ledger. - return path[len(prefix) :].lstrip("/") + # A redundant separator ("/repo//etc/passwd") is the same POSIX path as + # "/repo/etc/passwd", but leaving the leading slash on the remainder would + # rebuild an absolute-looking "//etc/passwd" in the ledger. + offset = len(prefix) + while offset < len(path) and path[offset] == "/": + offset += 1 + return offset def _not_project_relative_error(key: str, index: int) -> InvalidParams: @@ -696,38 +742,62 @@ def _optional_project_relative_files( remainder is then put through the SAME check as any other value, so "/repo/~/x" cannot deposit a "~/x" the published rule calls impossible. - Two rules keep a stored value honest about the file it names: - - * Backslashes are folded for VALIDATION only. On POSIX ``weird\\name.py`` - is a legal filename, and folding it into the stored value would merge it - with a genuinely different file called ``weird/name.py``. What the caller - sent is what gets stored (modulo redundant "." and "/" segments, which - denote the same path either way — that normalization is what stops - ``src/./a.py`` and ``src/a.py`` becoming two rows for one file). + Three rules keep a stored value honest about the file it names: + + * Backslashes are folded for VALIDATION only, and on EVERY branch. On POSIX + ``weird\\name.py`` is a legal filename, and folding it into the stored + value would merge it with a genuinely different file called + ``weird/name.py``. That held for a relative entry but not for a + relativized one, so ``/repo/weird\\name.py`` under project_dir="/repo" + stored ``weird/name.py`` — the same silent merge, one branch over. Both + branches now cut the arrived value and the folded copy at the same index. + The exception is a Windows path, where a backslash IS a separator: there + the folded copy is the value, so ``C:\\repo\\src\\a.py`` under + project_dir="C:\\repo" still stores ``src/a.py`` rather than fragmenting + against the same file recorded with forward slashes. * An entry naming the project root itself (".", "./", or an absolute path equal to project_dir) is DROPPED, not rejected. It names no file, so storing it would be a row for a path that does not exist — but failing the whole call, and losing a real section or machine check, over one cosmetic entry is the behaviour this validator exists to stop. + * A Windows absolute path that no project_dir on the call can relativize is + STORED, not rejected — verbatim but for the separator folding above, so + ``C:\\repo\\src\\a.py`` lands as ``C:/repo/src/a.py``. 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 (measured: the 0.5.2 release stores it, with the + backslashes it arrived with) into a whole-call failure, which is exactly + the anti-pattern this validator exists to remove. It is the one absolute + form with no POSIX reading, so keeping it invents nothing; a POSIX + absolute path without a usable project_dir stays a rejection, as it was + before this branch. The escape check below still applies, so + ``C:\\repo\\..\\x`` is still fatal. """ files = _optional_limited_str_list(args, key, max_items=50, max_length=240) root = project_dir.replace("\\", "/").rstrip("/") if project_dir else None normalized_files: list[str] = [] for index, item in enumerate(files): folded = item.replace("\\", "/") - stored = item + # On a Windows path a backslash is a separator, so the folded copy IS + # the value; anywhere else it is an ordinary filename character and + # folding it would merge two different POSIX files. + windows_path = bool(_WINDOWS_DRIVE_PREFIX.match(folded)) + stored = folded if windows_path else item if _looks_absolute(folded): - relative = _relative_to_project_dir(folded, root) - if relative is None: - raise _not_project_relative_error(key, index) - # The caller sent an absolute path and asked, by supplying - # project_dir, to have it rewritten; here the remainder IS the - # value, so both forms move to it. - folded = stored = relative - if _looks_absolute(folded): - # "/repo/~/x" -> "~/x", "/repo/C:/x" -> "C:/x": still not a - # project-relative path, and the schema says so. + offset = _relative_offset_under_project_dir(folded, root) + if offset is not None: + # Cut both index-aligned views at the same place: `folded` is + # what the remaining checks read, `stored` is what the ledger + # gets, and only the latter keeps the caller's separators. + folded, stored = folded[offset:], stored[offset:] + if _looks_absolute(folded): + # "/repo/~/x" -> "~/x", "/repo/C:/x" -> "C:/x": still not a + # project-relative path, and the schema says so. + raise _not_project_relative_error(key, index) + elif not windows_path: raise _not_project_relative_error(key, index) + # else: a Windows absolute path this call cannot relativize. Stored + # as sent rather than rejected — see the third rule above. if ".." in PurePosixPath(folded).parts: raise InvalidParams(f"{key}[{index}] must not escape the project directory: '..' segments are not allowed") stored_parts = PurePosixPath(stored).parts diff --git a/tests/test_mcp.py b/tests/test_mcp.py index 596f6eb..2929d74 100644 --- a/tests/test_mcp.py +++ b/tests/test_mcp.py @@ -3097,15 +3097,18 @@ def test_mangle_marker_is_server_authored_and_cannot_be_forged(tmp_path): def test_machine_check_name_has_no_private_length_cap(tmp_path): - """A 240-character cap turned a 241-8000 band that recorded fine on main + """A 240-character cap turned a 241-4036 band that recorded fine on main into hard rejections. The band is real: an agent recording the full pytest invocation it ran routinely writes a ~300-character name. The only ceiling is the shared metadata budget, whose error now reports a field and a byte - count instead of a bare "metadata must be <= 8192 bytes".""" + count instead of a bare "metadata must be <= 8192 bytes". + + 4036, not the 8000 first claimed: see + test_machine_check_name_band_is_the_measured_one for the boundary.""" server = SentinelMCPServer(store_dir=tmp_path / "state") long_name = ".venv/bin/pytest -q " + "tests/test_mcp.py::test_a " * 12 - assert 240 < len(long_name) < 8000 + assert 240 < len(long_name) <= 4036 accepted = _tool_payload( _call_tool( server, @@ -3135,6 +3138,35 @@ def test_machine_check_name_has_no_private_length_cap(tmp_path): assert "largest field is summary" in message +def test_machine_check_name_band_is_the_measured_one(tmp_path): + """The band this validator restored was documented as "241-8000". Binary + searching a {source, name, result} call puts the real edge at 4036 accepted + / 4037 rejected -- identical on the 0.5.2 release this branched from, so the + number was wrong when it was written, not changed by the branch. It is half + of 8192 because `name` lands in the budget twice: once as itself, once + inside the "<name>: <result>" summary the server synthesizes.""" + server = SentinelMCPServer(store_dir=tmp_path / "state") + + def accepted(msg_id, length): + response = _call_tool( + server, + msg_id, + "agentacct_record_machine_check", + {"source": "codex", "name": "x" * length, "result": "passed"}, + ) + return "error" not in response, response + + ok, response = accepted(1, 4036) + assert ok + assert response["result"]["content"][0]["text"] + + ok, response = accepted(2, 4037) + assert not ok + # One character over the edge already blames the synthesized summary, so the + # indirection the comment warns about is the normal case, not an extreme. + assert "largest field is summary at 4060 bytes" in response["error"]["message"] + + def test_metadata_budget_decision_is_identical_on_all_three_write_surfaces(tmp_path): """Measuring real UTF-8 bytes on MCP alone broke the symmetry the shared budget exists for: 2000 CJK characters are 6013 real bytes but 12013 @@ -3320,17 +3352,23 @@ def test_relativized_remainder_is_revalidated_against_the_published_rule(tmp_pat def test_windows_drive_letter_is_absolute_for_validation(tmp_path): """Backslashes are folded before the absolute check, so PurePosixPath alone - reads "C:/repo/a.py" as a relative path and stores it as one.""" + reads "C:/repo/a.py" as a relative path and stores it as one. + + Absolute for validation means it is relativized against a drive-letter root + and still escape-checked -- NOT that it is rejected without one. See + test_windows_absolute_path_is_never_fatal for why.""" server = SentinelMCPServer(store_dir=tmp_path / "state") - rejected = _call_tool( - server, - 1, - "agentacct_record_section", - {"source": "codex", "section_id": "drive", "section_status": "started", "files": ["C:\\repo\\a.py"]}, + # Not filed as a relative path, and not rejected either: stored as sent. + stored = _tool_payload( + _call_tool( + server, + 1, + "agentacct_record_section", + {"source": "codex", "section_id": "drive", "section_status": "started", "files": ["C:\\repo\\a.py"]}, + ) ) - assert rejected["error"]["code"] == -32602 - assert "project-relative" in rejected["error"]["message"] + assert stored["event"]["metadata"]["files"] == ["C:/repo/a.py"] # With the drive-letter root declared on the call it relativizes normally. accepted = _tool_payload( @@ -3350,6 +3388,109 @@ def test_windows_drive_letter_is_absolute_for_validation(tmp_path): assert accepted["event"]["metadata"]["files"] == ["src/a.py"] +def test_windows_absolute_path_is_never_fatal(tmp_path): + """Recognizing drive letters as absolute was right; pairing it with a + rejection was not. Measured on the 0.5.2 release this branched from, both + shapes below RECORD, storing the backslashes they arrived with; on the first + cut of this branch both killed the whole call -- over a path-style detail, + which is the anti-pattern the validator exists to remove. It is the one + absolute form with no POSIX reading, so keeping it invents nothing. The + stored value differs from 0.5.2 only in separator: on a Windows path a + backslash IS a separator, so it is normalized like any other.""" + server = SentinelMCPServer(store_dir=tmp_path / "state") + + def files_for(args): + payload = _tool_payload( + _call_tool( + server, + files_for.msg_id, + "agentacct_record_section", + {"source": "codex", "section_id": "win", "section_status": "started", **args}, + ) + ) + files_for.msg_id += 1 + return payload["event"]["metadata"]["files"] + + files_for.msg_id = 1 + + # The shape agent harnesses actually hand out on Windows: no project_dir. + assert files_for({"files": ["C:\\repo\\src\\a.py"]}) == ["C:/repo/src/a.py"] + # And with a project_dir that cannot possibly contain it. + assert files_for({"files": ["C:\\repo\\src\\a.py"], "project_dir": "/other"}) == ["C:/repo/src/a.py"] + + # A POSIX absolute path with no usable project_dir stays a rejection: it is + # not a regression, it is what the release before this branch also did. + posix = _call_tool( + server, + 90, + "agentacct_record_section", + {"source": "codex", "section_id": "win", "section_status": "started", "files": ["/repo/src/a.py"]}, + ) + assert posix["error"]["code"] == -32602 + + # Non-fatal is not unchecked: the escape guard still rejects. + escaping = _call_tool( + server, + 91, + "agentacct_record_section", + {"source": "codex", "section_id": "win", "section_status": "started", "files": ["C:\\repo\\..\\..\\secrets"]}, + ) + assert escaping["error"]["code"] == -32602 + assert "escape" in escaping["error"]["message"] + + +def test_backslash_is_preserved_on_the_relativized_branch_too(tmp_path): + """On POSIX "weird\\name.py" is a legal filename and a genuinely different + file from "weird/name.py". The relative branch kept it; the relativized + branch folded it, silently merging the two. Same rule on both branches.""" + server = SentinelMCPServer(store_dir=tmp_path / "state") + + relative = _tool_payload( + _call_tool( + server, + 1, + "agentacct_record_section", + {"source": "codex", "section_id": "bs", "section_status": "started", "files": ["weird\\name.py"]}, + ) + ) + assert relative["event"]["metadata"]["files"] == ["weird\\name.py"] + + relativized = _tool_payload( + _call_tool( + server, + 2, + "agentacct_record_section", + { + "source": "codex", + "section_id": "bs", + "section_status": "started", + "project_dir": "/repo", + "files": ["/repo/weird\\name.py"], + }, + ) + ) + # Same file, same stored value, whichever way the caller spelled the path. + assert relativized["event"]["metadata"]["files"] == ["weird\\name.py"] + + # The Windows exception, where a backslash really is a separator: folding is + # correct there, so this must NOT fragment against a forward-slash "src/a.py". + windows = _tool_payload( + _call_tool( + server, + 3, + "agentacct_record_section", + { + "source": "codex", + "section_id": "bs", + "section_status": "started", + "project_dir": "C:\\repo", + "files": ["C:\\repo\\src\\a.py"], + }, + ) + ) + assert windows["event"]["metadata"]["files"] == ["src/a.py"] + + def test_tilde_project_dir_never_anchors_a_relativization(tmp_path): """"~" is a shell shortcut, not a root: two machines expand it to different directories, so containment under it is not provable and must not be @@ -3425,3 +3566,51 @@ def test_mangle_detector_does_not_fire_on_a_closing_title_tag(tmp_path): ) ) assert mangled["event"]["metadata"]["mangled_tool_call_suspected_fields"] == ["next_step"] + + +def test_mangle_detector_ineligible_set_is_calibrated_not_hand_picked(tmp_path): + """Excluding `title` alone was inconsistent: it removed one HTML/SVG element + name on the grounds that agents write about markup, and left `summary` and + `metadata` -- element names by the same reasoning -- armed to stamp a + server-authored accusation into the stored record of ordinary prose. + + The ineligible set is now every property name in those two vocabularies. + Measured READ-ONLY against the real 6261-event ledger, that costs nothing: + across the 3535 section+evidence events ``, `` and + `` never occur, and `` occurs only inside the two genuine + mangled calls, which both supplied `summary` so it could never have fired.""" + from agentacct.mcp import MANGLE_DETECTOR_INELIGIBLE_PROPERTIES + + server = SentinelMCPServer(store_dir=tmp_path / "state") + + def suspected(msg_id, summary): + payload = _tool_payload( + _call_tool( + server, + msg_id, + "agentacct_record_section", + {"source": "codex", "section_id": "svg", "section_status": "completed", "summary": summary}, + ) + ) + metadata = payload["event"]["metadata"] + # The stamp in the stored record and the warning to the caller always + # agree; other warnings (join hints) are not this test's business. + mangle_warnings = [warning for warning in payload["warnings"] if "mangled tool call" in warning] + assert bool(mangle_warnings) == ("mangled_tool_call_suspected_fields" in metadata) + return metadata.get("mangled_tool_call_suspected_fields") + + # Ordinary prose about markup: accused before, silent now. Each of these is + # a real thing an agent writes while doing web work. + assert suspected(1, "Stripped the block from the exported SVG.") is None + assert suspected(2, "The
disclosure needs its closing tag.") is None + assert suspected(3, "Removed the stale tag from the element.") is None + + # Still armed on every name that is not a markup element: these are the + # names the two real mangled calls in the ledger actually absorbed. + assert suspected(4, "Done.") == ["next_step"] + assert suspected(5, "Done.") == ["files"] + assert suspected(6, "Done.") == ["project_dir"] + + # Pinned last, so a regression shows up as the behaviour above rather than + # as a bare constant mismatch. + assert MANGLE_DETECTOR_INELIGIBLE_PROPERTIES == {"title", "summary", "metadata", "source"} From 93c8cfb341a6a0fd7f94c1bfeee4dab32445295d Mon Sep 17 00:00:00 2001 From: mikehasa Date: Fri, 31 Jul 2026 04:32:13 +0800 Subject: [PATCH 4/9] fix(mcp): correct the false justification for `source`'s mangle-detector exemption MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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; `` remains at 0 occurrences, as the bullet above already records. `` 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. --- src/agentacct/mcp.py | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/agentacct/mcp.py b/src/agentacct/mcp.py index 6be8424..0740b0f 100644 --- a/src/agentacct/mcp.py +++ b/src/agentacct/mcp.py @@ -519,9 +519,15 @@ def _validate_metadata_size(value: dict[str, Any]) -> None: # these names collide with real writing; only the closing form has yet to # turn up in 3535 events. # -# So excluding these four costs zero measured true positives. `source` is inert -# — it is required on both tools, so it is never an unsupplied property — and -# is listed only to keep the rule complete rather than case-by-case. +# So excluding these four costs zero measured true positives. `source` is in the +# set on the same footing as the other three: `` is an HTML element name, +# and it is the property name that collides with real writing most often here — +# a bare-word detector fires on it in 170 of the 1956 section events, ahead of +# `files` at 130 and every other property name. Its membership is load-bearing +# rather than bookkeeping: `source` is required on agentacct_record_section, but +# agentacct_record_machine_check declares no required properties and reads it as +# optional, so a machine-check call that omits it leaves `source` genuinely +# unsupplied and therefore eligible to be suspected. MANGLE_DETECTOR_INELIGIBLE_PROPERTIES = frozenset({"title", "summary", "metadata", "source"}) From c0e330bbb318a13d7fa776277af09557cb0e8b80 Mon Sep 17 00:00:00 2001 From: mikehasa Date: Fri, 31 Jul 2026 05:32:49 +0800 Subject: [PATCH 5/9] docs(changelog): record the MCP input-surface fixes --- CHANGELOG.md | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 265b27d..06f584c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,29 @@ follow [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ## [Unreleased] +### Fixed +- `agentacct_record_section` now accepts `title` as an alias for + `section_title`. The recording contract agentacct ships to every client told + agents to send `title`, which the schema then rejected outright, losing the + whole record; the instruction is corrected and the alias keeps + already-onboarded machines working without re-running `onboard`. +- Limit errors now say what was received, not only what is allowed. An agent + that overshoots a length limit could previously only shrink blindly and + retry. +- Metadata size is measured in real UTF-8 bytes on every write surface. It was + measured against an ASCII-escaped encoding, so each CJK character counted as + 6 bytes and each emoji as 12 — a Chinese-writing agent was refused at roughly + a third of the advertised budget, by an error naming a parameter it had never + sent. Size errors now name the field that actually overflowed. +- `files` entries: the project-relative rule is published in the schema (it was + enforced but documented nowhere), and an absolute path that provably lies + under the call's own `project_dir` is normalized instead of failing the whole + call. This was the single largest cause of refused recordings. Paths that + escape the project are still rejected. +- A tool call mangled in transit — parameters absorbed into a narrative field + as literal text — is now flagged with a warning and a marker on the stored + record instead of being kept silently. It never rejects and never repairs. + ## [0.5.2] - 2026-07-31 ### Fixed From 6bb842901ab38f250948d45ed19939b66e87928d Mon Sep 17 00:00:00 2001 From: mikehasa Date: Fri, 31 Jul 2026 00:24:32 +0800 Subject: [PATCH 6/9] feat(log_evidence): count and surface recording calls agentacct refused 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. --- src/agentacct/api.py | 109 ++++- src/agentacct/client_usage.py | 105 ++++- src/agentacct/log_evidence.py | 351 +++++++++++++++- src/agentacct/work_ledger.py | 14 +- tests/test_refused_recording_attempts.py | 489 +++++++++++++++++++++++ tests/test_rename_compat.py | 6 + 6 files changed, 1056 insertions(+), 18 deletions(-) create mode 100644 tests/test_refused_recording_attempts.py diff --git a/src/agentacct/api.py b/src/agentacct/api.py index 4866db1..e7bdde0 100644 --- a/src/agentacct/api.py +++ b/src/agentacct/api.py @@ -10,7 +10,8 @@ import secrets import threading import time -from dataclasses import dataclass, replace +# ``field`` is aliased: several helpers below use ``field`` as a loop variable. +from dataclasses import dataclass, field as dataclass_field, replace from datetime import date, datetime, timedelta, timezone from pathlib import Path from typing import Any, Iterable, Mapping @@ -149,6 +150,7 @@ usage_bucket_date, week_start, ) +from .log_evidence import summarize_refused_recording_attempts from .usage_truth import ( CODEX_REPLAY_QUARANTINE_STATE, is_diagnostic_event, @@ -857,6 +859,11 @@ class DashboardUsageView: excluded_saved_records: list[DashboardUsageRecord] local_by_client: dict[str, list[DashboardUsageRecord]] saved_by_client: dict[str, list[DashboardUsageRecord]] + # Recording calls agentacct REFUSED, derived from the same live client-log + # scan that produced local_records. Nothing about a refusal is stored, so + # this is re-derived on every scan — which is also why refusals that + # predate the feature are counted without a backfill. + refused_recording: dict[str, Any] = dataclass_field(default_factory=dict) def _safe_float(value: Any) -> float | None: @@ -1128,6 +1135,10 @@ def _build_usage_view(local_usage_preview: list[ClientUsageEvent], events: list[ excluded_saved_records=excluded_saved_records, local_by_client=_usage_records_by_client(local_records), saved_by_client=_usage_records_by_client(saved_records), + # Derived from the raw scan candidates (not the records), because the + # summary dedups per base session itself: one Claude Code transcript + # becomes several per-model records carrying the SAME refusal counts. + refused_recording=summarize_refused_recording_attempts(local_usage_preview), ) @@ -1812,6 +1823,97 @@ def _display_count_label(value: Any) -> str: return text +# Plain-language names for the frozen refusal reason vocabulary. Anything the +# vocabulary gains without a label here still renders (as its code), so a new +# reason can never be silently dropped from the table. +_REFUSED_RECORDING_REASON_LABELS = { + "narrative_over_limit": "Text longer than the field allows", + "files_not_project_relative": "File path was not project-relative", + "unknown_argument": "Argument agentacct does not accept", + "missing_argument": "Required argument was missing", + "invalid_argument": "Argument had the wrong type or value", + "value_over_limit": "Value above the allowed maximum", + "metadata_over_size": "Metadata above the size limit", + "incomplete_argument_group": "Arguments that must be sent together were not", + "no_runs": "No run existed to attach the record to", + "unknown_run_id": "Named run does not exist in this store", + "other": "Refused for a reason this build does not name yet", +} + + +def _refused_recording_reason_label(reason_code: Any) -> str: + code = str(reason_code or "other") + return _REFUSED_RECORDING_REASON_LABELS.get(code, code) + + +def _refused_recording_field_label(field_name: Any) -> str: + """No field means the refusal was about the call, not one argument. + + Deliberately NOT the dashboard's generic "Not reported": nothing is + missing here, and an allowlisted-only field is also how an agent-invented + argument name is kept out of this page. + """ + + return str(field_name) if field_name else "Not argument-specific" + + +def _refused_recording_html(summary: Mapping[str, Any], esc: Any) -> str: + """Refusals agentacct itself returned, derived live from the client logs. + + Renders ONLY the bounded {tool, field, reason} triple and its count. No + message text, no rejected value, no length, no path — the rejection path + runs before the redactor, so anything else here would leak user content. + """ + + total = int(summary.get("refused_attempt_total") or 0) + sessions = int(summary.get("sessions_with_refusals") or 0) + unclassified = int(summary.get("unclassified_outputs_skipped") or 0) + rows = [row for row in (summary.get("rows") or []) if isinstance(row, Mapping)] + if not total: + body = ( + '

No refused recording calls were found in the client logs ' + "scanned above. Recording calls that agentacct rejects are stored nowhere, so this " + "count comes from re-reading the client's own logs on every scan.

" + ) + else: + table_rows = "".join( + "" + f"{esc(_display_count_label(row.get('tool')))}" + f"{esc(_refused_recording_field_label(row.get('field')))}" + f"{esc(_refused_recording_reason_label(row.get('reason_code')))}" + f"{esc(_fmt_int(row.get('count')))}" + "" + for row in rows + ) + body = ( + f'

{esc(_fmt_int(total))} recording call(s) across ' + f"{esc(_fmt_int(sessions))} client session(s) were refused by agentacct. " + "These are attempts an agent made and this product rejected — not work you failed to " + "record. Until they are fixed, that work has no context in the ledger, so usage rows " + "from those sessions can look unexplained. Only the argument name and the refusal " + "reason are shown: the rejected values never leave the client log. This counts the " + "sessions in the local scan on this page, so it is a floor rather than an all-time " + "total.

" + f'
' + f"" + f"{table_rows}
ToolArgumentWhy agentacct refused itAttempts
" + ) + remainder_note = ( + f'

A further {esc(_fmt_int(unclassified))} scanned output(s) ' + "donated no recorded event but are NOT counted above: they were refused by the client " + "before agentacct saw them, or carried a shape this build does not recognise. They stay " + "out of the refusal total rather than inflating it.

" + if unclassified + else "" + ) + return f""" +
Recording calls agentacct refused
+

Recording calls agentacct refused

+ {body} + {remainder_note} + """ + + def _display_provider_model(provider: Any, model: Any) -> str: provider_label = _display_count_label(provider) model_label = _display_count_label(model) @@ -11459,6 +11561,10 @@ def source_sessions_cell(source: UsageSourceDiscovery) -> str: ("/runs", "agentacct-launched command JSON"), ] ) + # Refusals are derived from the same live scan this page already consumes, + # so they cover every rejection still on disk — including ones recorded + # long before this surface existed. + refused_recording_html = _refused_recording_html(usage_view.refused_recording, esc) return f"""

Raw Data / Debug

@@ -11473,6 +11579,7 @@ def source_sessions_cell(source: UsageSourceDiscovery) -> str:
Debug JSON endpoints

Every GET rebuilds the work ledger from the store — no caching, by design (zero staleness); the measured envelope is fine to roughly 5,000 saved rows, revisit if stores grow beyond that.

{debug_endpoint_rows}
EndpointPurpose
+ {refused_recording_html}
Work Timeline

Work Timeline

View: {timeline_view_control}

Chronological mix of usage, MCP work, evidence, diagnostics, and proxy events. The default Grouped view collapses usage import rows into one entry per agent and day (counts and exact sums only — grouping never allocates usage to work); Everything shows the raw row-level mix.

diff --git a/src/agentacct/client_usage.py b/src/agentacct/client_usage.py index f334907..6d95936 100644 --- a/src/agentacct/client_usage.py +++ b/src/agentacct/client_usage.py @@ -16,7 +16,7 @@ from contextlib import contextmanager from dataclasses import dataclass, field, replace from pathlib import Path -from typing import Any, Callable, Literal +from typing import Any, Callable, Literal, Mapping from .confidence import COST_BASIS_CLIENT_SESSION, COST_BASIS_PRICING_TABLE, COST_CLIENT_REPORTED, COST_ESTIMATED_FROM_TOKENS, COST_UNKNOWN, USAGE_CLIENT_REPORTED from .env_compat import read_env_alias @@ -26,7 +26,10 @@ classify_claude_tool_use, classify_codex_function_call, classify_codex_mcp_invocation, + claude_tool_use_creation_tool, extract_created_event_id, + refused_recording_rows, + unwrap_codex_mcp_error, unwrap_codex_mcp_result, unwrap_codex_output_text, ) @@ -329,6 +332,14 @@ class ClientUsageEvent: # and from refreshable truth material. source_revision_at: int | None = None source_revision_basis: str | None = None + # Read-time diagnostic (log_evidence.py): the subset of + # ``evidenced_outputs_skipped`` this scan can prove was a recording call + # agentacct itself REFUSED, as bounded {tool, field, reason_code, count} + # rows. Derived fresh from the client log on every scan and deliberately + # never persisted — the transcripts on disk stay the record, so refusals + # that predate this field are counted without any backfill. Carries no + # message text, value, length, or path. + refused_recording_attempts: tuple[Mapping[str, Any], ...] = () def __post_init__(self) -> None: if self.client not in USAGE_EVENT_CLIENTS: @@ -590,6 +601,8 @@ class ClientSessionObservation: evidenced_outputs_skipped: int = 0 source_namespace_fingerprint: str | None = None source_parse_complete: bool = True + # Same read-time refusal diagnostic as ClientUsageEvent; never persisted. + refused_recording_attempts: tuple[Mapping[str, Any], ...] = () @property def session_identity(self) -> tuple[str, str]: @@ -1212,6 +1225,9 @@ def _discover_codex_usage_from_home( evidenced_outputs_skipped=_safe_nonnegative_int( evidence_source.get("evidenced_outputs_skipped") ), + refused_recording_attempts=_safe_refused_recording_attempts( + evidence_source.get("refused_recording_attempts") + ), source_parse_complete=parse_complete_by_session.get( row_id, False, @@ -1349,6 +1365,9 @@ def _discover_codex_usage_from_home( evidenced_event_ids=tuple(_safe_evidenced_ids(usage.get("evidenced_event_ids"))), evidenced_event_id_total=_safe_nonnegative_int(usage.get("evidenced_event_id_total")), evidenced_outputs_skipped=_safe_nonnegative_int(usage.get("evidenced_outputs_skipped")), + refused_recording_attempts=_safe_refused_recording_attempts( + usage.get("refused_recording_attempts") + ), source_parse_complete=parse_complete_by_session.get( row_id, False, @@ -1395,6 +1414,29 @@ def _safe_evidenced_ids(value: Any) -> list[str]: return [item for item in value if isinstance(item, str)] +def _safe_refused_recording_attempts(value: Any) -> tuple[Mapping[str, Any], ...]: + """Re-validate refusal rows against log_evidence's frozen vocabularies. + + The scan builds these rows itself, but they pass through a plain dict on + the way here; re-validating means no path from a client log to a rendered + surface can carry a tool, field, or reason agentacct does not define. + """ + + return tuple(refused_recording_rows(_refused_recording_row_counts(value))) + + +def _refused_recording_row_counts(value: Any) -> dict[tuple[Any, Any, Any], int]: + if not isinstance(value, (list, tuple)): + return {} + counts: dict[tuple[Any, Any, Any], int] = {} + for row in value: + if not isinstance(row, Mapping): + continue + key = (row.get("tool"), row.get("field"), row.get("reason_code")) + counts[key] = counts.get(key, 0) + _safe_nonnegative_int(row.get("count")) + return counts + + def _initial_claude_discovery_stats() -> dict[str, Any]: return { "discovered": 0, @@ -2161,6 +2203,9 @@ def record_error(code: str) -> None: evidenced_outputs_skipped=_safe_nonnegative_int( observation_metadata.get("evidenced_outputs_skipped") ), + refused_recording_attempts=_safe_refused_recording_attempts( + observation_metadata.get("refused_recording_attempts") + ), source_parse_complete=observation_parse_complete, ) ) @@ -2206,6 +2251,9 @@ def record_error(code: str) -> None: evidenced_event_ids=tuple(_safe_evidenced_ids(usage.get("evidenced_event_ids"))), evidenced_event_id_total=_safe_nonnegative_int(usage.get("evidenced_event_id_total")), evidenced_outputs_skipped=_safe_nonnegative_int(usage.get("evidenced_outputs_skipped")), + refused_recording_attempts=_safe_refused_recording_attempts( + usage.get("refused_recording_attempts") + ), source_parse_complete=selected_cohort_complete and not bool( parse_stats["malformed_transcript_lines"] or parse_stats["invalid_usage_rows"] @@ -6979,20 +7027,35 @@ def _read_codex_rollout_usage_uncached( saw_token_usage_schema_drift = False cache_write_tokens_applicable = False - def _skip_once(call_key: str | None) -> None: + def _skip_once( + call_key: str | None, + text: str | None = None, + *, + tool: str | None = None, + ) -> None: + # ``text``/``tool``, when the channel has them, let the accumulator + # classify an agentacct refusal into its bounded reason vocabulary. + # The dedup below still applies, so one refused call is counted once + # even when both codex channels carry it. if call_key is None: - evidence.record_skip() + evidence.record_skip(text, tool=tool) return if call_key in donated_call_ids or call_key in skip_counted_call_ids: return skip_counted_call_ids.add(call_key) - evidence.record_skip() - - def _donate_once(call_key: str, text: str | None) -> None: + evidence.record_skip(text, tool=tool) + + def _donate_once( + call_key: str, + text: str | None, + *, + refusal_text: str | None = None, + tool: str | None = None, + ) -> None: if not isinstance(text, str) or extract_created_event_id(text) is None: # Unwrap/shape failure: counted, but the call_id stays available # for the other channel's (possibly valid) representation. - _skip_once(call_key) + _skip_once(call_key, refusal_text if refusal_text is not None else text, tool=tool) return if call_key in donated_call_ids: return @@ -7073,7 +7136,14 @@ def _donate_once(call_key: str, text: str | None) -> None: elif payload_type == "function_call_output": call_id = payload.get("call_id") if isinstance(call_id, str) and call_id in accepted_calls: - _donate_once(call_id, unwrap_codex_output_text(payload.get("output"))) + # This channel's unwrap yields the server's refusal text + # verbatim when the call failed, so no separate error + # unwrap is needed. + _donate_once( + call_id, + unwrap_codex_output_text(payload.get("output")), + tool=accepted_calls[call_id], + ) elif isinstance(call_id, str) and call_id in rejected_calls: _skip_once(call_id) elif payload_type == "mcp_tool_call_end": @@ -7102,7 +7172,15 @@ def _donate_once(call_key: str, text: str | None) -> None: # shape drift, skipped-but-counted (never guessed). evidence.record_skip() elif verdict == "accepted": - _donate_once(call_key, unwrap_codex_mcp_result(payload.get("result"))) + # unwrap_codex_mcp_result reads the Ok branch only, + # so the Err branch is unwrapped separately to keep + # the refusal classifiable. + _donate_once( + call_key, + unwrap_codex_mcp_result(payload.get("result")), + refusal_text=unwrap_codex_mcp_error(payload.get("result")), + tool=str(invocation.get("tool")), + ) else: _skip_once(call_key) if model is None and payload_type not in _CODEX_MODEL_SCAN_EXCLUDED_PAYLOAD_TYPES: @@ -7380,7 +7458,14 @@ def _read_claude_project_usages( elif block_type == "tool_result": tool_use_id = block.get("tool_use_id") if isinstance(tool_use_id, str) and tool_use_id in accepted_tool_ids: - evidence.add_output_text(_claude_tool_result_text(block.get("content"))) + # A refused call's tool_result carries the server's + # error text here; the accumulator classifies it + # into the bounded refusal vocabulary and keeps no + # part of the message. + evidence.add_output_text( + _claude_tool_result_text(block.get("content")), + tool=claude_tool_use_creation_tool(accepted_tool_ids[tool_use_id]), + ) elif isinstance(tool_use_id, str) and tool_use_id in rejected_tool_ids: evidence.record_skip() usage_present = "usage" in message diff --git a/src/agentacct/log_evidence.py b/src/agentacct/log_evidence.py index 3bc4086..7976619 100644 --- a/src/agentacct/log_evidence.py +++ b/src/agentacct/log_evidence.py @@ -40,7 +40,7 @@ import math import re from collections import Counter -from typing import Any, Callable, Hashable +from typing import Any, Callable, Hashable, Iterable, Mapping from .join_rules import namespace_join_compatible from .usage_truth import ( @@ -148,6 +148,200 @@ def extract_created_event_id(text: str) -> str | None: return event_id +# --------------------------------------------------------------------------- +# Refused recording attempts (read-time, privacy-bounded) +# --------------------------------------------------------------------------- +# +# When the MCP server rejects a recording call it persists NOTHING: no event, +# no counter, no log line. The client's own transcript is therefore the only +# record that the attempt happened, and this importer already reads it — the +# refusal arrives here as the same output text whose created-event shape check +# failed, and is folded into ``evidenced_outputs_skipped``. +# +# ``evidenced_outputs_skipped`` is NOT that number and must never be presented +# as one. It also counts server-registration-name rejections, wire-shape drift, +# successful-but-not-created-event payloads, and refusals the CLIENT made +# before the call ever reached agentacct (codex's approval layer, Claude Code's +# input validator). Only the classification below claims "agentacct refused +# this"; everything else stays an unclassified skip, so the two numbers +# reconcile by subtraction instead of disagreeing. +# +# PRIVACY: the rejection path runs BEFORE the redactor (which only runs on a +# successful record_event), so the refusal text can carry raw user content — +# `unexpected argument(s): `, `unknown run_id: `, +# Claude Code's "You sent (first 200 of N characters) ". Nothing +# derived here may carry it out: the tool comes from the creation-tool +# allowlist, the field name from the frozen argument vocabulary below, the +# reason from the frozen reason vocabulary, and the message itself is never +# retained, echoed, measured, or hashed. + +# The documented tool arguments agentacct's own MCP schemas define. A refusal +# names a field only when it is one of these; an agent-invented key (which is +# what most `unexpected argument(s)` refusals carry) reports no field rather +# than leaking the key. Kept as a literal so this read path never imports the +# server module and never drifts into echoing whatever a caller sent. +RECORDING_ARGUMENT_NAMES = frozenset( + { + "after_exit_code", "after_summary", "artifact_path", "artifact_ref", + "artifact_url", "before_exit_code", "before_summary", "blocker", + "budget_usd", "cache_creation_input_tokens", "cache_read_input_tokens", + "cached_input_tokens", "client", "client_event_timestamp", + "client_session_id", "client_transcript_id", "command", "cost_basis", + "cost_confidence", "cost_currency", "cost_usd", "estimated_cost_usd", + "estimated_input_tokens", "estimated_output_tokens", "event_type", + "evidence_type", "exit_code", "files", "idempotency_key", + "input_tokens", "items", "kind", "limit", "message_id", "metadata", + "model", "name", "next_step", "output_tokens", + "parent_client_session_id", "phase", "project_dir", "provider", + "reasoning_output_tokens", "reporting_basis", "request_id", + "resolution_scope", "resolution_summary", "resolves_blocked_event_id", + "result", "rubric", "run_id", "section_id", "section_status", + "section_title", "source", "summary", "task_goal", "total_tokens", + "turn_id", "turn_index", "usage_confidence", "work_id", + "write_package", + } +) + +# Bounded reason vocabulary. Every recognised agentacct refusal maps to exactly +# one of these; an agentacct refusal whose wording is not matched lands in +# "other" so a server message change shows up as a rising "other" count instead +# of silently vanishing (or being guessed at). +REFUSED_RECORDING_REASON_CODES = ( + "narrative_over_limit", + "files_not_project_relative", + "unknown_argument", + "missing_argument", + "invalid_argument", + "value_over_limit", + "metadata_over_size", + "incomplete_argument_group", + "no_runs", + "unknown_run_id", + "other", +) + +# Both wire spellings of a JSON-RPC error from the agentacct MCP server: +# Claude Code renders "MCP error -32602: ", codex renders +# "Mcp error: -32602: " inside its own "tool call failed for ..." +# wrapper. The tool-name/namespace allowlist upstream is what proves the +# error came from THIS server; this pattern only locates the message. +_MCP_ERROR_MESSAGE_RE = re.compile(r"mcp error:?\s*-\d+:[ \t]*(?P[^\r\n]+)", re.IGNORECASE) +# codex wraps a refusal as: tool call failed for `/`. The +# server segment is matched loosely on purpose — historical rollouts carry +# whichever registration key was current when they were written — and the tool +# it yields is still checked against the creation-tool allowlist. +_CODEX_FAILED_TOOL_RE = re.compile(r"tool call failed for `[^`/\s]*/(?P[A-Za-z0-9_]+)`") +# Leading `` or `[]` of a refusal message. +_REFUSAL_FIELD_RE = re.compile(r"^(?P[a-z][a-z0-9_]*)(?:\[\d+\])?\b") +_UNEXPECTED_ARGUMENT_PREFIX = "unexpected argument(s):" +_MISSING_ARGUMENT_PREFIX = "missing required argument:" +_UNKNOWN_RUN_ID_PREFIX = "unknown run_id:" + + +def _refusal_reason_code(message: str) -> str: + """Bounded reason code for one agentacct refusal message. + + Matched on message SHAPE only — no fragment of the message is retained by + the caller, and the ordering runs most-specific first so the byte cap on + ``metadata`` is not swallowed by the generic "must be <= N" rules. + """ + + lowered = message.lower() + if lowered.startswith(_UNEXPECTED_ARGUMENT_PREFIX): + return "unknown_argument" + if lowered.startswith(_MISSING_ARGUMENT_PREFIX): + return "missing_argument" + if lowered.startswith(_UNKNOWN_RUN_ID_PREFIX): + return "unknown_run_id" + if lowered == "no runs found": + return "no_runs" + if "must be project-relative" in lowered or "must be a normalized project-relative path" in lowered: + return "files_not_project_relative" + if re.search(r"must be <= \d+ bytes when json encoded", lowered): + return "metadata_over_size" + if re.search(r"must be <= \d+ characters", lowered): + return "narrative_over_limit" + if re.search(r"must (?:be (?:<=|>=|>|<) \d|contain <= \d)", lowered): + return "value_over_limit" + if "must be supplied together" in lowered or "needs at least one join hint" in lowered: + return "incomplete_argument_group" + if "must be one of" in lowered or "must be a" in lowered or "must be an" in lowered: + return "invalid_argument" + if lowered.startswith("a blocker resolution requires"): + return "incomplete_argument_group" + return "other" + + +def _refusal_field_name(message: str, reason_code: str) -> str | None: + """Allowlisted argument name a refusal is about, or None. + + Never returns a token the server does not itself define, so an + agent-invented key (or any other caller-supplied text) cannot ride out of + the rejection path on this field. + """ + + if reason_code == "unknown_run_id": + return "run_id" + if reason_code == "unknown_argument": + remainder = message[len(_UNEXPECTED_ARGUMENT_PREFIX):] + elif reason_code == "missing_argument": + remainder = message[len(_MISSING_ARGUMENT_PREFIX):] + else: + remainder = message + match = _REFUSAL_FIELD_RE.match(remainder.strip()) + if match is None: + return None + field = match.group("field") + return field if field in RECORDING_ARGUMENT_NAMES else None + + +def classify_refused_recording(text: Any, *, tool: Any = None) -> tuple[str | None, str | None, str] | None: + """``(tool, field, reason_code)`` when agentacct itself refused the call. + + Returns None — deliberately, not "other" — when the text is not an + agentacct MCP error: a client-side refusal (codex's approval layer, Claude + Code's input validator), a successful-but-not-created-event payload, or + wire drift. Those are real skips but they are NOT recording attempts this + product rejected, and counting them as such would overstate the number the + whole surface exists to be honest about. + + ``tool`` is the caller's allowlisted creation-tool name; codex's own + "tool call failed for `/`" wrapper is used as a fallback and + is likewise accepted only when it names an allowlisted creation tool. + """ + + if not isinstance(text, str) or not text: + return None + match = _MCP_ERROR_MESSAGE_RE.search(text) + if match is None: + return None + message = match.group("message").strip() + if not message: + return None + reason_code = _refusal_reason_code(message) + field = _refusal_field_name(message, reason_code) + resolved_tool = tool if isinstance(tool, str) and tool in SENTINEL_CREATION_TOOLS else None + if resolved_tool is None: + wrapper = _CODEX_FAILED_TOOL_RE.search(text) + if wrapper is not None and wrapper.group("tool") in SENTINEL_CREATION_TOOLS: + resolved_tool = wrapper.group("tool") + return resolved_tool, field, reason_code + + +def unwrap_codex_mcp_error(result: Any) -> str | None: + """Codex mcp_tool_call_end ``result`` field -> the ``Err`` string. + + The mirror of :func:`unwrap_codex_mcp_result`: that one reads the success + branch and returns None for a failure, so the refusal text it discards has + to be recovered here. Any other shape returns None. + """ + + if not isinstance(result, dict): + return None + err = result.get("Err") + return err if isinstance(err, str) and err else None + + def codex_namespace_matches_sentinel(namespace: Any) -> bool: """True when a codex function_call namespace identifies the sentinel server. @@ -196,6 +390,23 @@ def classify_claude_tool_use(name: Any) -> str | None: return "accepted" if server in ACCEPTED_SERVER_KEYS else "rejected" +def claude_tool_use_creation_tool(name: Any) -> str | None: + """Bare creation-tool name of a ``mcp____`` block, else None. + + Read-only companion to :func:`classify_claude_tool_use` for callers that + already know a block was accepted and now need the tool name itself (the + refusal breakdown reports it). Returns only allowlisted names, so nothing + caller-supplied can ride out through this. + """ + + if not isinstance(name, str) or not name.startswith(_CLAUDE_TOOL_PREFIX): + return None + _server, separator, tool = name.removeprefix(_CLAUDE_TOOL_PREFIX).partition("__") + if not separator or tool not in SENTINEL_CREATION_TOOLS: + return None + return tool + + def classify_codex_mcp_invocation(server: Any, tool: Any) -> str | None: """"accepted" / "rejected" / None for a codex mcp_tool_call_end invocation. @@ -286,11 +497,15 @@ def __init__(self, *, cap: int = EVIDENCED_EVENT_IDS_CAP) -> None: self.evidenced_event_ids: list[str] = [] self._seen: set[str] = set() self.evidenced_outputs_skipped = 0 + # (tool, field, reason_code) -> count. Cardinality is bounded by + # construction: every component is drawn from a frozen vocabulary, so + # no caller-supplied text can grow this map. + self._refused: Counter[tuple[str | None, str | None, str]] = Counter() - def add_output_text(self, text: str | None) -> None: + def add_output_text(self, text: str | None, *, tool: str | None = None) -> None: event_id = extract_created_event_id(text) if isinstance(text, str) else None if event_id is None: - self.evidenced_outputs_skipped += 1 + self.record_skip(text, tool=tool) return if event_id in self._seen: return @@ -298,23 +513,149 @@ def add_output_text(self, text: str | None) -> None: if len(self.evidenced_event_ids) < self._cap: self.evidenced_event_ids.append(event_id) - def record_skip(self) -> None: + def record_skip(self, text: str | None = None, *, tool: str | None = None) -> None: + """Count one output that donated no event id. + + ``text`` is the refusal/output text when the caller has it. It is + classified and then dropped — only the bounded triple is kept, never + the message. + """ + self.evidenced_outputs_skipped += 1 + refusal = classify_refused_recording(text, tool=tool) + if refusal is not None: + self._refused[refusal] += 1 @property def evidenced_event_id_total(self) -> int: return len(self._seen) + @property + def refused_recording_attempt_total(self) -> int: + return sum(self._refused.values()) + @property def has_evidence(self) -> bool: return bool(self._seen) or self.evidenced_outputs_skipped > 0 + def refused_recording_attempts(self) -> list[dict[str, Any]]: + """Stable, privacy-bounded ``{tool, field, reason_code, count}`` rows.""" + + return refused_recording_rows(self._refused) + def as_usage_fields(self) -> dict[str, Any]: - return { + fields: dict[str, Any] = { "evidenced_event_ids": list(self.evidenced_event_ids), "evidenced_event_id_total": self.evidenced_event_id_total, "evidenced_outputs_skipped": self.evidenced_outputs_skipped, } + # Read-time diagnostic only: this rides the in-memory scan candidate so + # live surfaces can render it, and is deliberately NOT written into + # stored event metadata. Every scan re-derives it from the client logs + # still on disk, which is what makes already-recorded refusals visible + # retroactively without a migration. + if self._refused: + fields["refused_recording_attempts"] = self.refused_recording_attempts() + return fields + + +def refused_recording_rows( + counts: Mapping[tuple[str | None, str | None, str], int] | Counter[tuple[str | None, str | None, str]], +) -> list[dict[str, Any]]: + """``{tool, field, reason_code, count}`` rows, largest first. + + Re-validates every component against the frozen vocabularies, so a row + built from an untrusted mapping cannot smuggle caller text into a + rendered surface. An unrecognised reason is kept as "other" (never + dropped); an unrecognised tool/field becomes None. + """ + + rows: Counter[tuple[str | None, str | None, str]] = Counter() + for key, count in counts.items(): + if not isinstance(key, tuple) or len(key) != 3: + continue + tool, field, reason_code = key + safe_tool = tool if isinstance(tool, str) and tool in SENTINEL_CREATION_TOOLS else None + safe_field = field if isinstance(field, str) and field in RECORDING_ARGUMENT_NAMES else None + safe_reason = ( + reason_code + if isinstance(reason_code, str) and reason_code in REFUSED_RECORDING_REASON_CODES + else "other" + ) + rows[(safe_tool, safe_field, safe_reason)] += _safe_nonnegative_int(count) + return [ + {"tool": tool, "field": field, "reason_code": reason_code, "count": count} + for (tool, field, reason_code), count in sorted( + rows.items(), + key=lambda item: (-item[1], item[0][2], item[0][0] or "", item[0][1] or ""), + ) + if count > 0 + ] + + +def _refused_recording_counts(rows: Any) -> Counter[tuple[str | None, str | None, str]]: + counts: Counter[tuple[str | None, str | None, str]] = Counter() + if not isinstance(rows, (list, tuple)): + return counts + for row in rows: + if not isinstance(row, Mapping): + continue + reason_code = row.get("reason_code") + counts[(row.get("tool"), row.get("field"), reason_code if isinstance(reason_code, str) else "other")] += ( + _safe_nonnegative_int(row.get("count")) + ) + return counts + + +def summarize_refused_recording_attempts(candidates: Iterable[Any]) -> dict[str, Any]: + """Scan-wide "agentacct refused this recording call" summary. + + Input is the live client-log scan candidates (usage rows or session + observations); each carries the per-session breakdown its own transcript + scan derived. Candidates are deduped per (source namespace, client, BASE + session id) exactly as :func:`build_log_evidence_index` dedups donors: the + same evidence triple rides every per-model lane row of one Claude Code + transcript, so summing raw rows would multiply one session's refusals by + its model-lane count. + + ``unclassified_outputs_skipped`` is the honest remainder — skipped outputs + that are NOT provably agentacct refusals (client-side rejections, wire + drift, non-created-event payloads). It is reported rather than folded in, + so this summary can never overstate what agentacct itself refused. + """ + + seen: set[tuple[str, str, str]] = set() + counts: Counter[tuple[str | None, str | None, str]] = Counter() + outputs_skipped = 0 + sessions_with_refusals = 0 + for candidate in candidates: + client = str(getattr(candidate, "client", "") or "") + session_id = str(getattr(candidate, "client_session_id", "") or "") + namespace = str(getattr(candidate, "source_namespace_fingerprint", "") or "") + key = (namespace, client, normalized_local_usage_session_id(client, session_id)) + if key in seen: + continue + seen.add(key) + outputs_skipped += _safe_nonnegative_int(getattr(candidate, "evidenced_outputs_skipped", 0)) + session_counts = _refused_recording_counts(getattr(candidate, "refused_recording_attempts", ())) + if session_counts: + sessions_with_refusals += 1 + counts.update(session_counts) + rows = refused_recording_rows(counts) + refused_total = sum(int(row["count"]) for row in rows) + by_reason: Counter[str] = Counter() + for row in rows: + by_reason[str(row["reason_code"])] += int(row["count"]) + return { + "sessions_with_refusals": sessions_with_refusals, + "refused_attempt_total": refused_total, + "by_reason": dict( + sorted(by_reason.items(), key=lambda item: (-item[1], item[0])) + ), + "rows": rows, + "outputs_skipped": outputs_skipped, + "unclassified_outputs_skipped": max(outputs_skipped - refused_total, 0), + } # --------------------------------------------------------------------------- diff --git a/src/agentacct/work_ledger.py b/src/agentacct/work_ledger.py index 39f2095..d2b9671 100644 --- a/src/agentacct/work_ledger.py +++ b/src/agentacct/work_ledger.py @@ -1534,6 +1534,16 @@ def build_attention_items(work_items: list[dict[str, Any]], usage_reconciliation return sorted(items, key=lambda item: severity_rank.get(str(item.get("severity")), 0), reverse=True) +# One shared sentence so the attention group and the matching blind spot never +# drift. It deliberately stops short of blaming the user for all of it: part of +# "no work context" is recording calls agentacct itself refused, which the +# Local logs page now counts (see api._refused_recording_html). +_USAGE_WITHOUT_MCP_CONTEXT_NEXT_STEP = ( + "Enable MCP context attach at session start and record sections for meaningful work; " + "some of this missing context is recording calls agentacct refused, listed under " + "\"Recording calls agentacct refused\" in Local logs." +) + # Canonical, cause-level next steps for grouped attention display. Falls back # to the first item's own recommendation for unknown causes. _ATTENTION_GROUP_NEXT_STEPS = { @@ -1541,7 +1551,7 @@ def build_attention_items(work_items: list[dict[str, Any]], usage_reconciliation "completed_evidenced_work_without_attributed_usage": "Run local usage import and make sure MCP context includes client_session_id.", "ambiguous_same_session_attribution": "Attach client_transcript_id or another narrower join key so shared sessions can be disambiguated.", "missing_client_session_id": "MCP context is missing client_session_id; attach client context at session start.", - "usage_truth_without_mcp_context": "Enable MCP context attach at session start and record sections for meaningful work.", + "usage_truth_without_mcp_context": _USAGE_WITHOUT_MCP_CONTEXT_NEXT_STEP, } _ATTENTION_GROUP_TITLES = { @@ -3402,7 +3412,7 @@ def _ledger_blind_spots(work_items: list[dict[str, Any]], usage_reconciliation: "cache_read_tokens": _sum_row_key(usage_without_context, "cache_read_tokens"), "cache_creation_tokens": _sum_row_key(usage_without_context, "cache_creation_tokens"), "estimated_cost_usd": _sum_row_cost(usage_without_context), - "recommended_next_step": "Enable MCP context attach at session start and record sections for meaningful work.", + "recommended_next_step": _USAGE_WITHOUT_MCP_CONTEXT_NEXT_STEP, } ) diff --git a/tests/test_refused_recording_attempts.py b/tests/test_refused_recording_attempts.py new file mode 100644 index 0000000..44ea46a --- /dev/null +++ b/tests/test_refused_recording_attempts.py @@ -0,0 +1,489 @@ +"""Refused recording attempts — making agentacct's own rejections visible. + +When the MCP server rejects a recording call it persists NOTHING: no event, no +counter, no log line. The only record is the client's own transcript, which the +importer already reads. These tests pin the honesty and privacy rules for the +figure derived from it: + +- it counts calls **agentacct refused**, and nothing else. Client-side + refusals, non-created-event payloads, and wire drift stay in the existing + ``evidenced_outputs_skipped`` remainder instead of inflating it; +- every reason lands in a bounded vocabulary, and an unrecognised refusal + becomes "other" rather than being dropped or guessed at; +- nothing derived from a refusal carries user content — the rejection path + runs BEFORE the redactor, so the message, the rejected value, its length, + and any path must never reach a stored row or a rendered page; +- it is retroactive by construction: nothing is persisted, so a scan of + transcripts written long before this feature counts their refusals. +""" + +from __future__ import annotations + +import json +import sqlite3 +from pathlib import Path + +from agentacct.api import _refused_recording_html +from agentacct.client_usage import discover_claude_code_usage, discover_codex_usage +from agentacct.log_evidence import ( + REFUSED_RECORDING_REASON_CODES, + classify_refused_recording, + refused_recording_rows, + summarize_refused_recording_attempts, +) + +SESSION = "019f2303-6ae1-7000-8000-0000000000c1" +CLAUDE_SESSION = "c037bd88-0000-4000-8000-0000000000c1" + +# A real rejection carries the caller's own text back out. These stand in for +# the values a user would be furious to find in a world-readable file. +SECRET_PATH = "/Users/someone/private/clients/acme-contract.md" +SECRET_RUN = "run-with-a-customer-name-in-it" +SECRET_ARG = "internal_codename_bluebird" + + +# --------------------------------------------------------------------------- +# Codex rollout fixtures (wire shapes verified against real 0.14x rollouts) +# --------------------------------------------------------------------------- + + +def _mcp_err(tool: str, message: str, *, code: int = -32602) -> dict: + """Codex mcp_tool_call_end failure branch, verbatim wire shape.""" + + return { + "Err": ( + f"tool call error: tool call failed for `agent-sentinel/{tool}`\n\n" + f"Caused by:\n Mcp error: -{abs(code)}: {message}" + ) + } + + +def _mcp_ok_created(event_id: str) -> dict: + payload = {"event": {"event_id": event_id, "event_type": "section_started"}} + return {"Ok": {"content": [{"type": "text", "text": json.dumps(payload, indent=2, sort_keys=True)}]}} + + +def _mcp_tool_call_end(tool: str, call_id: str, result: dict) -> dict: + return { + "timestamp": "2026-07-05T10:00:01.000Z", + "type": "event_msg", + "payload": { + "type": "mcp_tool_call_end", + "call_id": call_id, + "invocation": {"server": "agent-sentinel", "tool": tool, "arguments": {}}, + "duration": {"secs": 1, "nanos": 0}, + "result": result, + }, + } + + +def _token_usage_line() -> dict: + return { + "type": "event_msg", + "payload": { + "info": { + "total_token_usage": { + "input_tokens": 900, + "cached_input_tokens": 100, + "output_tokens": 80, + "reasoning_output_tokens": 0, + } + }, + "model": "gpt-5.5", + }, + } + + +def _make_codex_home(root: Path, rollout_lines: list[dict], *, thread_id: str = SESSION) -> Path: + codex_home = root / "codex-home" + sessions_dir = codex_home / "sessions" / "2026" / "07" / "05" + sessions_dir.mkdir(parents=True, exist_ok=True) + rollout = sessions_dir / f"rollout-2026-07-05T10-00-00-{thread_id}.jsonl" + lines = [json.dumps({"type": "session_meta", "payload": {"id": thread_id}}), json.dumps(_token_usage_line())] + lines.extend(json.dumps(line) for line in rollout_lines) + rollout.write_text("\n".join(lines) + "\n", encoding="utf-8") + con = sqlite3.connect(codex_home / "state_5.sqlite") + try: + con.execute( + """ + create table threads ( + id text primary key, + rollout_path text not null, + created_at integer not null, + updated_at integer not null, + cwd text not null, + title text not null, + tokens_used integer not null, + model text, + cli_version text + ) + """ + ) + con.execute( + "insert into threads values (?, ?, ?, ?, ?, ?, ?, ?, ?)", + (thread_id, str(rollout), 100, 300, "/work/project", "t", 980, "gpt-5.5", "0.test"), + ) + con.execute("create table thread_spawn_edges (parent_thread_id text, child_thread_id text, status text)") + con.commit() + finally: + con.close() + return codex_home + + +def _claude_usage_line(session_id: str) -> dict: + return { + "type": "assistant", + "sessionId": session_id, + "cwd": "/work/project", + "message": { + "model": "claude-opus-4-8", + "usage": { + "input_tokens": 30, + "cache_creation_input_tokens": 0, + "cache_read_input_tokens": 0, + "output_tokens": 5, + }, + }, + } + + +def _claude_tool_use_line(session_id: str, *, tool_use_id: str, name: str) -> dict: + return { + "type": "assistant", + "sessionId": session_id, + "message": { + "role": "assistant", + "content": [{"type": "tool_use", "id": tool_use_id, "name": name, "input": {}}], + }, + } + + +def _claude_tool_result_line(session_id: str, *, tool_use_id: str, text: str) -> dict: + return { + "type": "user", + "sessionId": session_id, + "message": { + "role": "user", + "content": [ + {"type": "tool_result", "tool_use_id": tool_use_id, "content": [{"type": "text", "text": text}]} + ], + }, + } + + +def _rows_by_reason(summary: dict) -> dict[str, int]: + return dict(summary["by_reason"]) + + +# --------------------------------------------------------------------------- +# Classification: bounded vocabulary, unmatched text is kept as "other" +# --------------------------------------------------------------------------- + + +def test_every_observed_refusal_shape_maps_to_a_bounded_reason_and_field() -> None: + cases = [ + ("MCP error -32602: summary must be <= 1200 characters", None, "summary", "narrative_over_limit"), + ("MCP error -32602: files[0] must be project-relative", None, "files", "files_not_project_relative"), + ("MCP error -32602: metadata must be <= 8192 bytes when JSON encoded", None, "metadata", "metadata_over_size"), + ("MCP error -32602: unexpected argument(s): project_dir", None, "project_dir", "unknown_argument"), + ("MCP error -32602: missing required argument: section_id", None, "section_id", "missing_argument"), + ("MCP error -32602: kind must be one of: docs, other", None, "kind", "invalid_argument"), + ("MCP error -32602: limit must be <= 200", None, "limit", "value_over_limit"), + ("MCP error -32004: no runs found", None, None, "no_runs"), + (f"MCP error -32004: unknown run_id: {SECRET_RUN}", None, "run_id", "unknown_run_id"), + ( + "MCP error -32602: resolves_blocked_event_id, resolution_scope, and resolution_summary" + " must be supplied together", + None, + "resolves_blocked_event_id", + "incomplete_argument_group", + ), + ] + for text, tool, expected_field, expected_reason in cases: + result = classify_refused_recording(text, tool=tool) + assert result is not None, text + got_tool, got_field, got_reason = result + assert got_reason in REFUSED_RECORDING_REASON_CODES + assert (got_field, got_reason) == (expected_field, expected_reason), text + + +def test_unrecognised_refusal_text_lands_in_other_and_is_not_dropped() -> None: + result = classify_refused_recording("MCP error -32602: a brand new message this build never saw") + assert result is not None + tool, field, reason = result + assert reason == "other" + assert field is None + + +def test_client_side_refusals_are_not_counted_as_agentacct_refusals() -> None: + # codex's own approval layer never reached agentacct. + assert ( + classify_refused_recording( + '{"Err": "This action was rejected due to unacceptable risk.\\nReason: usage limit"}' + ) + is None + ) + # Claude Code's input validator, which echoes the payload it rejected. + assert ( + classify_refused_recording( + "InputValidationError: mcp__agentacct__sentinel_record_section was called" + f" with input that could not be parsed as JSON.\nYou sent {SECRET_PATH}" + ) + is None + ) + + +def test_agent_invented_argument_names_never_become_a_field() -> None: + result = classify_refused_recording(f"MCP error -32602: unexpected argument(s): {SECRET_ARG}") + assert result == (None, None, "unknown_argument") + + +def test_refused_recording_rows_revalidate_untrusted_input() -> None: + rows = refused_recording_rows( + { + ("not_a_real_tool", SECRET_ARG, "made_up_reason"): 3, + ("sentinel_record_section", "files", "files_not_project_relative"): 2, + } + ) + rendered = json.dumps(rows) + assert SECRET_ARG not in rendered + assert "not_a_real_tool" not in rendered + assert {"tool": None, "field": None, "reason_code": "other", "count": 3} in rows + + +# --------------------------------------------------------------------------- +# End-to-end over a client log: counts, breakdown, reconciliation +# --------------------------------------------------------------------------- + + +def test_codex_scan_counts_each_refusal_reason_and_reconciles_with_skips(tmp_path) -> None: + lines = [ + _mcp_tool_call_end("sentinel_record_section", "call_a", _mcp_err("sentinel_record_section", "files[0] must be project-relative")), + _mcp_tool_call_end("sentinel_record_section", "call_b", _mcp_err("sentinel_record_section", "files[1] must be project-relative")), + _mcp_tool_call_end("sentinel_record_machine_check", "call_c", _mcp_err("sentinel_record_machine_check", "no runs found", code=-32004)), + _mcp_tool_call_end("sentinel_record_section", "call_d", _mcp_err("sentinel_record_section", "summary must be <= 1200 characters")), + _mcp_tool_call_end("sentinel_record_section", "call_e", _mcp_err("sentinel_record_section", f"unexpected argument(s): {SECRET_ARG}")), + _mcp_tool_call_end("sentinel_record_machine_check", "call_f", _mcp_err("sentinel_record_machine_check", f"unknown run_id: {SECRET_RUN}", code=-32004)), + _mcp_tool_call_end("sentinel_record_event", "call_g", _mcp_err("sentinel_record_event", "an unmapped future message")), + # A refusal the CLIENT made: a real skip, but agentacct never saw it. + _mcp_tool_call_end( + "sentinel_record_section", + "call_h", + {"Err": "This action was rejected due to unacceptable risk.\nReason: usage limit"}, + ), + # A successful call: neither skipped nor refused. + _mcp_tool_call_end("sentinel_record_section", "call_i", _mcp_ok_created("evt_aaa111bbb222")), + ] + codex_home = _make_codex_home(tmp_path, lines) + + events = discover_codex_usage(codex_home=codex_home, limit_sessions=10) + assert len(events) == 1 + event = events[0] + + summary = summarize_refused_recording_attempts(events) + assert summary["refused_attempt_total"] == 7 + assert summary["sessions_with_refusals"] == 1 + assert _rows_by_reason(summary) == { + "files_not_project_relative": 2, + "narrative_over_limit": 1, + "no_runs": 1, + "other": 1, + "unknown_argument": 1, + "unknown_run_id": 1, + } + # The existing generic counter saw all eight non-donating outputs; the one + # it holds beyond the refusals is the client-side rejection, reported as a + # remainder instead of being folded into "agentacct refused this". + assert event.evidenced_outputs_skipped == 8 + assert summary["outputs_skipped"] == 8 + assert summary["unclassified_outputs_skipped"] == 1 + assert list(event.evidenced_event_ids) == ["evt_aaa111bbb222"] + + tools = {row["tool"] for row in summary["rows"]} + assert tools == {"sentinel_record_section", "sentinel_record_machine_check", "sentinel_record_event"} + + +def test_claude_scan_names_the_refused_tool_and_field(tmp_path) -> None: + claude_home = tmp_path / "claude-home" + project = claude_home / "projects" / "-work-project" + project.mkdir(parents=True) + lines = [ + _claude_usage_line(CLAUDE_SESSION), + _claude_tool_use_line(CLAUDE_SESSION, tool_use_id="toolu_a", name="mcp__agentacct__agentacct_record_section"), + _claude_tool_result_line( + CLAUDE_SESSION, + tool_use_id="toolu_a", + text="MCP error -32602: summary must be <= 1200 characters", + ), + _claude_tool_use_line(CLAUDE_SESSION, tool_use_id="toolu_b", name="mcp__agentacct__agentacct_record_machine_check"), + _claude_tool_result_line(CLAUDE_SESSION, tool_use_id="toolu_b", text="MCP error -32004: no runs found"), + ] + (project / f"{CLAUDE_SESSION}.jsonl").write_text( + "\n".join(json.dumps(line) for line in lines) + "\n", encoding="utf-8" + ) + + events = discover_claude_code_usage(claude_home=claude_home, limit_sessions=10) + summary = summarize_refused_recording_attempts(events) + + assert summary["refused_attempt_total"] == 2 + assert { + (row["tool"], row["field"], row["reason_code"]) for row in summary["rows"] + } == { + ("agentacct_record_section", "summary", "narrative_over_limit"), + ("agentacct_record_machine_check", None, "no_runs"), + } + + +def test_claude_model_lanes_do_not_multiply_one_session_of_refusals(tmp_path) -> None: + """The same evidence triple rides every per-model row of one transcript.""" + + claude_home = tmp_path / "claude-home" + project = claude_home / "projects" / "-work-project" + project.mkdir(parents=True) + opus = _claude_usage_line(CLAUDE_SESSION) + haiku = json.loads(json.dumps(opus)) + haiku["message"]["model"] = "claude-haiku-4-5" + lines = [ + opus, + haiku, + _claude_tool_use_line(CLAUDE_SESSION, tool_use_id="toolu_a", name="mcp__agentacct__agentacct_record_section"), + _claude_tool_result_line( + CLAUDE_SESSION, + tool_use_id="toolu_a", + text="MCP error -32602: summary must be <= 1200 characters", + ), + ] + (project / f"{CLAUDE_SESSION}.jsonl").write_text( + "\n".join(json.dumps(line) for line in lines) + "\n", encoding="utf-8" + ) + + events = discover_claude_code_usage(claude_home=claude_home, limit_sessions=10) + assert len({event.usage_row_lane for event in events}) == 2 + + summary = summarize_refused_recording_attempts(events) + assert summary["refused_attempt_total"] == 1 + assert summary["sessions_with_refusals"] == 1 + + +# --------------------------------------------------------------------------- +# Privacy: the rejection path is pre-redaction, so nothing may leak +# --------------------------------------------------------------------------- + + +def test_no_user_content_reaches_the_stored_row_or_the_rendered_page(tmp_path) -> None: + secrets = (SECRET_PATH, SECRET_RUN, SECRET_ARG) + lines = [ + _mcp_tool_call_end( + "sentinel_record_section", + "call_a", + _mcp_err("sentinel_record_section", f"files[0] must be project-relative: {SECRET_PATH}"), + ), + _mcp_tool_call_end( + "sentinel_record_machine_check", + "call_b", + _mcp_err("sentinel_record_machine_check", f"unknown run_id: {SECRET_RUN}", code=-32004), + ), + _mcp_tool_call_end( + "sentinel_record_section", + "call_c", + _mcp_err("sentinel_record_section", f"unexpected argument(s): {SECRET_ARG}"), + ), + _mcp_tool_call_end( + "sentinel_record_section", + "call_d", + _mcp_err("sentinel_record_section", "summary must be <= 1200 characters (received 4211)"), + ), + ] + codex_home = _make_codex_home(tmp_path, lines) + events = discover_codex_usage(codex_home=codex_home, limit_sessions=10) + event = events[0] + + # 1. Nothing about a refusal is persisted at all — not the message, and + # not even the bounded breakdown (it is re-derived on every scan). + stored = json.dumps(event.to_sentinel_event()) + for secret in secrets: + assert secret not in stored + assert "refused_recording" not in stored + + # 2. The in-memory breakdown carries only the bounded triple. + for row in event.refused_recording_attempts: + assert set(row) == {"tool", "field", "reason_code", "count"} + breakdown = json.dumps(list(event.refused_recording_attempts)) + for secret in secrets: + assert secret not in breakdown + # A "received N" length is user-derived measurement, never carried out. + assert "4211" not in breakdown + + # 3. The rendered page carries only the bounded triple too. + summary = summarize_refused_recording_attempts(events) + html = _refused_recording_html(summary, lambda value: str(value)) + for secret in secrets: + assert secret not in html + assert "4211" not in html + assert "Recording calls agentacct refused" in html + + +def test_rendered_copy_says_agentacct_refused_these_not_the_user(tmp_path) -> None: + lines = [ + _mcp_tool_call_end( + "sentinel_record_section", + "call_a", + _mcp_err("sentinel_record_section", "files[0] must be project-relative"), + ), + _mcp_tool_call_end( + "sentinel_record_section", + "call_b", + {"Err": "This action was rejected due to unacceptable risk.\nReason: usage limit"}, + ), + ] + codex_home = _make_codex_home(tmp_path, lines) + events = discover_codex_usage(codex_home=codex_home, limit_sessions=10) + summary = summarize_refused_recording_attempts(events) + + html = _refused_recording_html(summary, lambda value: str(value)) + assert "refused by agentacct" in html + assert "not work you failed to" in html + # The remainder is disclosed rather than quietly added to the total. + assert "1 scanned output(s) donated no recorded event but are NOT counted above" in html + + +def test_empty_scan_renders_an_honest_zero_state() -> None: + html = _refused_recording_html(summarize_refused_recording_attempts([]), lambda value: str(value)) + assert "No refused recording calls were found" in html + + +# --------------------------------------------------------------------------- +# Retroactivity: the whole point +# --------------------------------------------------------------------------- + + +def test_refusals_recorded_before_this_feature_existed_are_counted(tmp_path) -> None: + """Nothing is persisted, so there is nothing to backfill. + + The fixture is a pre-rename ``sentinel_*`` rollout with no agentacct store + anywhere: exactly the state of a transcript written months before this + surface existed. A plain scan still produces the full breakdown. + """ + + lines = [ + _mcp_tool_call_end( + "sentinel_record_section", + "call_old_a", + _mcp_err("sentinel_record_section", "files[0] must be project-relative"), + ), + _mcp_tool_call_end( + "sentinel_record_machine_check", + "call_old_b", + _mcp_err("sentinel_record_machine_check", "no runs found", code=-32004), + ), + ] + codex_home = _make_codex_home(tmp_path, lines) + + # No store, no import, no prior agentacct state of any kind. + assert not (tmp_path / "state").exists() + + summary = summarize_refused_recording_attempts( + discover_codex_usage(codex_home=codex_home, limit_sessions=10) + ) + assert summary["refused_attempt_total"] == 2 + assert _rows_by_reason(summary) == {"files_not_project_relative": 1, "no_runs": 1} diff --git a/tests/test_rename_compat.py b/tests/test_rename_compat.py index bbfa8ac..b48ce92 100644 --- a/tests/test_rename_compat.py +++ b/tests/test_rename_compat.py @@ -687,6 +687,12 @@ def test_packaging_excludes_tests_from_shipped_artifacts() -> None: # ENOENT that reads as a crash). These two forms carry that guidance. "mcp remove agent-sentinel", # `opencode mcp remove agent-sentinel` remediation command "agent-sentinel/agent-chronicle", # prose pairing in the "remove any stale ..." guidance + # Frozen refusal wire text: codex wraps every failed MCP call as + # "tool call failed for `/`", and historical rollouts + # carry the pre-rename key in that slot forever. log_evidence's refusal + # classifier must keep reading those, so the fixture pinning that behaviour + # keeps the observed string. + "`agent-sentinel/", ) From a5d6fe1deb8f233cbc267b06d49fb3b211520b02 Mon Sep 17 00:00:00 2001 From: mikehasa Date: Fri, 31 Jul 2026 01:16:32 +0800 Subject: [PATCH 7/9] fix(log_evidence): only count a refusal where the wire actually puts one 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: ", optionally inside its 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). --- src/agentacct/api.py | 1 + src/agentacct/client_usage.py | 13 +- src/agentacct/log_evidence.py | 68 +++++- tests/test_refused_recording_attempts.py | 277 ++++++++++++++++++++++- 4 files changed, 345 insertions(+), 14 deletions(-) diff --git a/src/agentacct/api.py b/src/agentacct/api.py index e7bdde0..445d46d 100644 --- a/src/agentacct/api.py +++ b/src/agentacct/api.py @@ -1833,6 +1833,7 @@ def _display_count_label(value: Any) -> str: "missing_argument": "Required argument was missing", "invalid_argument": "Argument had the wrong type or value", "value_over_limit": "Value above the allowed maximum", + "value_under_limit": "Value below the allowed minimum", "metadata_over_size": "Metadata above the size limit", "incomplete_argument_group": "Arguments that must be sent together were not", "no_runs": "No run existed to attach the record to", diff --git a/src/agentacct/client_usage.py b/src/agentacct/client_usage.py index 6d95936..2fa0d08 100644 --- a/src/agentacct/client_usage.py +++ b/src/agentacct/client_usage.py @@ -16,7 +16,7 @@ from contextlib import contextmanager from dataclasses import dataclass, field, replace from pathlib import Path -from typing import Any, Callable, Literal, Mapping +from typing import Any, Callable, Literal from .confidence import COST_BASIS_CLIENT_SESSION, COST_BASIS_PRICING_TABLE, COST_CLIENT_REPORTED, COST_ESTIMATED_FROM_TOKENS, COST_UNKNOWN, USAGE_CLIENT_REPORTED from .env_compat import read_env_alias @@ -7136,9 +7136,14 @@ def _donate_once( elif payload_type == "function_call_output": call_id = payload.get("call_id") if isinstance(call_id, str) and call_id in accepted_calls: - # This channel's unwrap yields the server's refusal text - # verbatim when the call failed, so no separate error - # unwrap is needed. + # No refusal text is passed on this legacy channel: a + # failed call's ``output`` is codex's own plain-text error, + # which unwrap_codex_output_text (JSON/content-block only) + # returns None for. Such a call stays a plain skip and is + # reported in the unclassified remainder rather than + # guessed at from an unverified wire shape — an + # UNDER-count, which is the honest direction for a figure + # that claims "agentacct refused this". _donate_once( call_id, unwrap_codex_output_text(payload.get("output")), diff --git a/src/agentacct/log_evidence.py b/src/agentacct/log_evidence.py index 7976619..2c32368 100644 --- a/src/agentacct/log_evidence.py +++ b/src/agentacct/log_evidence.py @@ -213,6 +213,7 @@ def extract_created_event_id(text: str) -> str | None: "missing_argument", "invalid_argument", "value_over_limit", + "value_under_limit", "metadata_over_size", "incomplete_argument_group", "no_runs", @@ -225,7 +226,20 @@ def extract_created_event_id(text: str) -> str | None: # "Mcp error: -32602: " inside its own "tool call failed for ..." # wrapper. The tool-name/namespace allowlist upstream is what proves the # error came from THIS server; this pattern only locates the message. +# +# Matched with .match at the two POSITIONS a real wire refusal can occupy — +# never .search over the whole output. A successful call's payload routinely +# quotes an agentacct error verbatim (an after_summary reading "green after +# fixing MCP error -32602: ..." is real recorded work), and a scan of the whole +# blob counts that success as a refusal: a number that lies in the one +# direction this surface exists to be honest about. _MCP_ERROR_MESSAGE_RE = re.compile(r"mcp error:?\s*-\d+:[ \t]*(?P[^\r\n]+)", re.IGNORECASE) +# The line codex's anyhow wrapper puts its cause on: "Caused by:\n ". +_CAUSED_BY_LINE = "caused by:" +# The tag Claude Code puts around a failed tool_result. Stripped from the FIRST +# line only, so the anchor still holds: nothing but an error is ever wrapped in +# it, and a payload that merely quotes an error does not carry it. +_CLAUDE_TOOL_ERROR_TAG = "" # codex wraps a refusal as: tool call failed for `/`. The # server segment is matched loosely on purpose — historical rollouts carry # whichever registration key was current when they were written — and the tool @@ -238,12 +252,47 @@ def extract_created_event_id(text: str) -> str | None: _UNKNOWN_RUN_ID_PREFIX = "unknown run_id:" +def _anchored_mcp_error_message(text: str) -> str | None: + """The refusal message when ``text`` IS a wire refusal, else None. + + Only two positions count. Claude Code puts the whole tool_result at + "MCP error -32602: ", so the FIRST line carries it. codex wraps + the same refusal in its own "tool call failed for ..." error and puts the + cause on the line directly after "Caused by:". Anywhere else the text is + payload — quoted, echoed, or narrated by a call that SUCCEEDED — and must + not be read as agentacct having refused anything. + """ + + lines = text.splitlines() + if not lines: + return None + candidates = [lines[0].strip().removeprefix(_CLAUDE_TOOL_ERROR_TAG)] + candidates.extend( + lines[index + 1] + for index, line in enumerate(lines[:-1]) + if line.strip().lower() == _CAUSED_BY_LINE + ) + for candidate in candidates: + match = _MCP_ERROR_MESSAGE_RE.match(candidate.strip()) + if match is None: + continue + message = match.group("message").strip() + if message: + return message + return None + + def _refusal_reason_code(message: str) -> str: """Bounded reason code for one agentacct refusal message. Matched on message SHAPE only — no fragment of the message is retained by the caller, and the ordering runs most-specific first so the byte cap on ``metadata`` is not swallowed by the generic "must be <= N" rules. + + Maximum and minimum bounds are separate codes on purpose: mcp.py rejects + ``input_tokens=-5`` with "input_tokens must be >= 0", and reporting that + under a code whose label reads "above the allowed maximum" would state the + opposite of what happened. """ lowered = message.lower() @@ -261,8 +310,10 @@ def _refusal_reason_code(message: str) -> str: return "metadata_over_size" if re.search(r"must be <= \d+ characters", lowered): return "narrative_over_limit" - if re.search(r"must (?:be (?:<=|>=|>|<) \d|contain <= \d)", lowered): + if re.search(r"must (?:be (?:<=|<) -?\d|contain <= \d)", lowered): return "value_over_limit" + if re.search(r"must be (?:a finite number )?(?:>=|>) -?\d", lowered): + return "value_under_limit" if "must be supplied together" in lowered or "needs at least one join hint" in lowered: return "incomplete_argument_group" if "must be one of" in lowered or "must be a" in lowered or "must be an" in lowered: @@ -300,10 +351,12 @@ def classify_refused_recording(text: Any, *, tool: Any = None) -> tuple[str | No Returns None — deliberately, not "other" — when the text is not an agentacct MCP error: a client-side refusal (codex's approval layer, Claude - Code's input validator), a successful-but-not-created-event payload, or - wire drift. Those are real skips but they are NOT recording attempts this - product rejected, and counting them as such would overstate the number the - whole surface exists to be honest about. + Code's input validator), a successful-but-not-created-event payload that + merely QUOTES an agentacct error, or wire drift. Those are real skips but + they are NOT recording attempts this product rejected, and counting them as + such would overstate the number the whole surface exists to be honest + about. The error is therefore located by position + (:func:`_anchored_mcp_error_message`), never found anywhere in the blob. ``tool`` is the caller's allowlisted creation-tool name; codex's own "tool call failed for `/`" wrapper is used as a fallback and @@ -312,10 +365,7 @@ def classify_refused_recording(text: Any, *, tool: Any = None) -> tuple[str | No if not isinstance(text, str) or not text: return None - match = _MCP_ERROR_MESSAGE_RE.search(text) - if match is None: - return None - message = match.group("message").strip() + message = _anchored_mcp_error_message(text) if not message: return None reason_code = _refusal_reason_code(message) diff --git a/tests/test_refused_recording_attempts.py b/tests/test_refused_recording_attempts.py index 44ea46a..3441026 100644 --- a/tests/test_refused_recording_attempts.py +++ b/tests/test_refused_recording_attempts.py @@ -19,11 +19,21 @@ from __future__ import annotations +import ast import json import sqlite3 from pathlib import Path -from agentacct.api import _refused_recording_html +from fastapi.testclient import TestClient + +import agentacct.client_usage as client_usage_module +import agentacct.log_evidence as log_evidence_module +from agentacct.api import ( + _REFUSED_RECORDING_REASON_LABELS, + UsageDiscoveryConfig, + _refused_recording_html, + create_local_api_app, +) from agentacct.client_usage import discover_claude_code_usage, discover_codex_usage from agentacct.log_evidence import ( REFUSED_RECORDING_REASON_CODES, @@ -233,11 +243,180 @@ def test_client_side_refusals_are_not_counted_as_agentacct_refusals() -> None: ) +def test_a_quoted_agentacct_error_inside_a_payload_is_not_a_refusal() -> None: + """Position, not presence: an error is a refusal only where the wire puts it. + + Agents narrate. "green after fixing MCP error -32602: ..." is a SUCCESSFUL + recording whose own text quotes an old failure, and a scan of the whole + output counts it as a call agentacct refused — the one direction this + figure must never lie in. + """ + + quoted = ( + "green after fixing MCP error -32602: files[0] must be project-relative" + f" on {SECRET_PATH}" + ) + assert classify_refused_recording(quoted, tool="agentacct_record_machine_check") is None + assert classify_refused_recording(json.dumps({"outcome": {"summary": quoted}})) is None + # A quote sitting mid-payload is not rescued by being on its own line. + assert classify_refused_recording(f"pytest passed\n{quoted}\nbuild ok") is None + # ...while both real wire positions still classify. + assert classify_refused_recording("MCP error -32602: files[0] must be project-relative") is not None + assert ( + classify_refused_recording( + "MCP error -32602: files[0] must be project-relative" + ) + is not None + ) + assert ( + classify_refused_recording( + "tool call error: tool call failed for `agent-sentinel/sentinel_record_section`" + "\n\nCaused by:\n Mcp error: -32602: files[0] must be project-relative" + ) + is not None + ) + + +def test_a_successful_call_quoting_an_error_is_not_counted_in_the_scan(tmp_path) -> None: + """The same rule, end to end: three successes must count as zero refusals.""" + + claude_home = tmp_path / "claude-home" + project = claude_home / "projects" / "-work-project" + project.mkdir(parents=True) + # An outcome-only machine_check response: a real success shape that donates + # no created-event id, so it reaches the refusal classifier as a skip. + succeeded = json.dumps( + { + "outcome": { + "machine_checks": { + "checks": [ + { + "name": "pytest", + "after": { + "exit_code": 0, + "summary": "green after fixing MCP error -32602: files[0] must be" + f" project-relative on {SECRET_PATH}", + }, + } + ] + } + } + } + ) + lines = [_claude_usage_line(CLAUDE_SESSION)] + for index in range(3): + lines.append( + _claude_tool_use_line( + CLAUDE_SESSION, + tool_use_id=f"toolu_{index}", + name="mcp__agentacct__agentacct_record_machine_check", + ) + ) + lines.append( + _claude_tool_result_line(CLAUDE_SESSION, tool_use_id=f"toolu_{index}", text=succeeded) + ) + (project / f"{CLAUDE_SESSION}.jsonl").write_text( + "\n".join(json.dumps(line) for line in lines) + "\n", encoding="utf-8" + ) + + events = discover_claude_code_usage(claude_home=claude_home, limit_sessions=10) + summary = summarize_refused_recording_attempts(events) + + assert summary["refused_attempt_total"] == 0 + assert summary["rows"] == [] + assert summary["sessions_with_refusals"] == 0 + # They stay in the generic skip counter, disclosed as the remainder — the + # two figures still reconcile, they just no longer accuse a success. + assert summary["outputs_skipped"] == 3 + assert summary["unclassified_outputs_skipped"] == 3 + + html = _refused_recording_html(summary, lambda value: str(value)) + assert "No refused recording calls were found" in html + assert "refused by agentacct" not in html + + def test_agent_invented_argument_names_never_become_a_field() -> None: result = classify_refused_recording(f"MCP error -32602: unexpected argument(s): {SECRET_ARG}") assert result == (None, None, "unknown_argument") +def test_minimum_bound_refusals_are_never_labelled_as_over_the_maximum() -> None: + """A label that inverts the refusal is worse than no label at all. + + ``input_tokens=-5`` is rejected with "input_tokens must be >= 0"; reporting + that as "Value above the allowed maximum" tells the user to shrink a number + they need to raise. + """ + + cases = [ + ("input_tokens must be >= 0", "input_tokens", "value_under_limit"), + ("turn_index must be >= 0", "turn_index", "value_under_limit"), + ("budget_usd must be > 0", "budget_usd", "value_under_limit"), + ("cost_usd must be a finite number >= 0", "cost_usd", "value_under_limit"), + # The maximum bound keeps its own code, so the two stay distinguishable. + ("limit must be <= 200", "limit", "value_over_limit"), + ] + for message, expected_field, expected_reason in cases: + result = classify_refused_recording(f"MCP error -32602: {message}") + assert result is not None, message + _tool, field, reason = result + assert reason in REFUSED_RECORDING_REASON_CODES + assert (field, reason) == (expected_field, expected_reason), message + + rows = refused_recording_rows( + {("agentacct_record_agent_usage_debug", "input_tokens", "value_under_limit"): 1} + ) + html = _refused_recording_html( + {"refused_attempt_total": 1, "sessions_with_refusals": 1, "rows": rows}, + lambda value: str(value), + ) + assert "Value below the allowed minimum" in html + assert "Value above the allowed maximum" not in html + + +def test_every_frozen_reason_code_has_plain_language_copy() -> None: + """Guard against the drift that produced the inverted label. + + A code the table has no wording for still renders (as the bare code), so + this is not about breakage — it is about the surface never again showing a + sentence that describes a different refusal than the one that happened. + """ + + for reason_code in REFUSED_RECORDING_REASON_CODES: + assert reason_code in _REFUSED_RECORDING_REASON_LABELS, reason_code + + +def test_minimum_bound_refusal_names_the_tool_that_was_refused(tmp_path) -> None: + """Tool attribution survives this shape on both live client channels.""" + + message = "input_tokens must be >= 0" + tool = "agentacct_record_agent_usage_debug" + + claude_home = tmp_path / "claude-home" + project = claude_home / "projects" / "-work-project" + project.mkdir(parents=True) + lines = [ + _claude_usage_line(CLAUDE_SESSION), + _claude_tool_use_line(CLAUDE_SESSION, tool_use_id="toolu_a", name=f"mcp__agentacct__{tool}"), + _claude_tool_result_line(CLAUDE_SESSION, tool_use_id="toolu_a", text=f"MCP error -32602: {message}"), + ] + (project / f"{CLAUDE_SESSION}.jsonl").write_text( + "\n".join(json.dumps(line) for line in lines) + "\n", encoding="utf-8" + ) + claude_summary = summarize_refused_recording_attempts( + discover_claude_code_usage(claude_home=claude_home, limit_sessions=10) + ) + + codex_home = _make_codex_home(tmp_path, [_mcp_tool_call_end(tool, "call_a", _mcp_err(tool, message))]) + codex_summary = summarize_refused_recording_attempts( + discover_codex_usage(codex_home=codex_home, limit_sessions=10) + ) + + expected = [{"tool": tool, "field": "input_tokens", "reason_code": "value_under_limit", "count": 1}] + assert claude_summary["rows"] == expected + assert codex_summary["rows"] == expected + + def test_refused_recording_rows_revalidate_untrusted_input() -> None: rows = refused_recording_rows( { @@ -452,6 +631,70 @@ def test_empty_scan_renders_an_honest_zero_state() -> None: assert "No refused recording calls were found" in html +# --------------------------------------------------------------------------- +# The surface itself: the section has to reach the page over HTTP +# --------------------------------------------------------------------------- + + +def _raw_page(store_root: Path, home: Path) -> str: + client = TestClient( + create_local_api_app( + store_dir=store_root, + usage_discovery=UsageDiscoveryConfig.from_home(home), + ) + ) + response = client.get("/raw", headers={"Accept": "text/html"}) + assert response.status_code == 200 + return response.text + + +def test_raw_route_renders_the_refusal_section_with_its_rows(tmp_path) -> None: + """Asserted over HTTP, not on the helper. + + Every other test here calls ``_refused_recording_html`` directly, so + deleting the one line that injects it into /raw leaves the whole suite + green while the feature is gone from the product. + """ + + home = tmp_path / "home" + project = home / ".claude" / "projects" / "-work-project" + project.mkdir(parents=True) + lines = [ + _claude_usage_line(CLAUDE_SESSION), + _claude_tool_use_line( + CLAUDE_SESSION, tool_use_id="toolu_a", name="mcp__agentacct__agentacct_record_section" + ), + _claude_tool_result_line( + CLAUDE_SESSION, + tool_use_id="toolu_a", + text=f"MCP error -32602: files[0] must be project-relative: {SECRET_PATH}", + ), + ] + (project / f"{CLAUDE_SESSION}.jsonl").write_text( + "\n".join(json.dumps(line) for line in lines) + "\n", encoding="utf-8" + ) + + page = _raw_page(tmp_path / "state", home) + + assert 'id="refused-recording-attempts"' in page + assert "Recording calls agentacct refused" in page + assert "1 recording call(s) across 1 client session(s) were refused by agentacct." in page + assert "agentacct_record_section" in page + assert "File path was not project-relative" in page + # The page is a rendering of the same pre-redaction scan, so the privacy + # rule has to hold at the route too, not only in the helper. + assert SECRET_PATH not in page + + +def test_raw_route_renders_the_refusal_section_on_a_clean_machine(tmp_path) -> None: + """No refusals is a state to report, not a reason to drop the section.""" + + page = _raw_page(tmp_path / "state", tmp_path / "home") + + assert 'id="refused-recording-attempts"' in page + assert "No refused recording calls were found" in page + + # --------------------------------------------------------------------------- # Retroactivity: the whole point # --------------------------------------------------------------------------- @@ -487,3 +730,35 @@ def test_refusals_recorded_before_this_feature_existed_are_counted(tmp_path) -> ) assert summary["refused_attempt_total"] == 2 assert _rows_by_reason(summary) == {"files_not_project_relative": 1, "no_runs": 1} + + +# --------------------------------------------------------------------------- +# Module hygiene: the repo runs no linter, so a shadowed import needs a test +# --------------------------------------------------------------------------- + + +def _rebound_module_level_imports(module) -> list[str]: + tree = ast.parse(Path(module.__file__).read_text(encoding="utf-8")) + seen: set[str] = set() + rebound: list[str] = [] + for node in tree.body: + if not isinstance(node, (ast.Import, ast.ImportFrom)): + continue + for alias in node.names: + name = alias.asname or alias.name.split(".")[0] + if name in seen: + rebound.append(name) + seen.add(name) + return rebound + + +def test_this_track_imports_no_name_twice() -> None: + """A second import of the same name silently replaces the first. + + ``from typing import Mapping`` after ``from collections.abc import Mapping`` + changes what every annotation in the file means, and no CI job here would + notice: the repo has no lint step. + """ + + for module in (client_usage_module, log_evidence_module): + assert _rebound_module_level_imports(module) == [], module.__name__ From 713ff659a92cae3d8087e94811222345de1095cb Mon Sep 17 00:00:00 2001 From: mikehasa Date: Fri, 31 Jul 2026 01:57:15 +0800 Subject: [PATCH 8/9] fix(log_evidence): a timed-out call is not a call agentacct refused 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 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. --- src/agentacct/api.py | 3 +- src/agentacct/log_evidence.py | 124 +++++++++--- tests/test_refused_recording_attempts.py | 231 +++++++++++++++++++++++ 3 files changed, 327 insertions(+), 31 deletions(-) diff --git a/src/agentacct/api.py b/src/agentacct/api.py index 445d46d..6beee3b 100644 --- a/src/agentacct/api.py +++ b/src/agentacct/api.py @@ -1902,7 +1902,8 @@ def _refused_recording_html(summary: Mapping[str, Any], esc: Any) -> str: remainder_note = ( f'

A further {esc(_fmt_int(unclassified))} scanned output(s) ' "donated no recorded event but are NOT counted above: they were refused by the client " - "before agentacct saw them, or carried a shape this build does not recognise. They stay " + "before agentacct saw them, failed in the client's transport before reaching it (a " + "timed-out or dropped call), or carried a shape this build does not recognise. They stay " "out of the refusal total rather than inflating it.

" if unclassified else "" diff --git a/src/agentacct/log_evidence.py b/src/agentacct/log_evidence.py index 2c32368..6487de4 100644 --- a/src/agentacct/log_evidence.py +++ b/src/agentacct/log_evidence.py @@ -160,11 +160,12 @@ def extract_created_event_id(text: str) -> str | None: # # ``evidenced_outputs_skipped`` is NOT that number and must never be presented # as one. It also counts server-registration-name rejections, wire-shape drift, -# successful-but-not-created-event payloads, and refusals the CLIENT made -# before the call ever reached agentacct (codex's approval layer, Claude Code's -# input validator). Only the classification below claims "agentacct refused -# this"; everything else stays an unclassified skip, so the two numbers -# reconcile by subtraction instead of disagreeing. +# successful-but-not-created-event payloads, refusals the CLIENT made before +# the call ever reached agentacct (codex's approval layer, Claude Code's input +# validator), and failures of the client's own transport that agentacct never +# saw at all (a timed-out or dropped call). Only the classification below +# claims "agentacct refused this"; everything else stays an unclassified skip, +# so the two numbers reconcile by subtraction instead of disagreeing. # # PRIVACY: the rejection path runs BEFORE the redactor (which only runs on a # successful record_event), so the refusal text can carry raw user content — @@ -221,11 +222,33 @@ def extract_created_event_id(text: str) -> str | None: "other", ) +# The JSON-RPC error codes agentacct's OWN server can answer a tools/call with. +# Enumerated from mcp.py's dispatch: FileNotFoundError -> -32004, InvalidParams +# and ValueError (which is also how "Unknown tool" comes back) -> -32602, and +# every other exception — plus the store-less degraded server, whose whole job +# is to stay connected and refuse — -> -32000. +# +# Every other code on the wire belongs to the CLIENT's transport layer, not to +# this product: -32001 request timed out, -32603 internal error, -32600/-32700 +# framing, -32601 method not found. A call that never got an answer is not a +# call agentacct refused, and counting one is the same overstatement the +# position anchor below exists to prevent. Keyed on the code rather than on the +# refusal wording so rewording a message in mcp.py cannot change the count. +AGENTACCT_MCP_ERROR_CODES = frozenset({-32000, -32004, -32602}) + +# -32000 is the JSON-RPC "implementation-defined server error" code, and the MCP +# client libraries reuse it for their own ConnectionClosed. agentacct's -32000s +# are the degraded-store refusal and unexpected server exceptions; the client's +# is this one fixed string, which no agentacct code path produces. Excluded by +# the CLIENT's wording — stable, and not prose this repo edits — because the +# code alone cannot separate the two. +_TRANSPORT_ERROR_MESSAGES = frozenset({"connection closed"}) + # Both wire spellings of a JSON-RPC error from the agentacct MCP server: # Claude Code renders "MCP error -32602: ", codex renders # "Mcp error: -32602: " inside its own "tool call failed for ..." # wrapper. The tool-name/namespace allowlist upstream is what proves the -# error came from THIS server; this pattern only locates the message. +# error came from THIS server; this pattern only locates the code and message. # # Matched with .match at the two POSITIONS a real wire refusal can occupy — # never .search over the whole output. A successful call's payload routinely @@ -233,12 +256,19 @@ def extract_created_event_id(text: str) -> str | None: # fixing MCP error -32602: ..." is real recorded work), and a scan of the whole # blob counts that success as a refusal: a number that lies in the one # direction this surface exists to be honest about. -_MCP_ERROR_MESSAGE_RE = re.compile(r"mcp error:?\s*-\d+:[ \t]*(?P[^\r\n]+)", re.IGNORECASE) +# The digit run is bounded: this reads untrusted transcript text, and int() on +# a several-thousand-digit run raises on the interpreter's own conversion limit. +# Every JSON-RPC code is five digits, so nothing real is lost. +_MCP_ERROR_MESSAGE_RE = re.compile( + r"mcp error:?\s*(?P-\d{1,6}):[ \t]*(?P[^\r\n]+)", re.IGNORECASE +) # The line codex's anyhow wrapper puts its cause on: "Caused by:\n ". _CAUSED_BY_LINE = "caused by:" -# The tag Claude Code puts around a failed tool_result. Stripped from the FIRST -# line only, so the anchor still holds: nothing but an error is ever wrapped in -# it, and a payload that merely quotes an error does not carry it. +# The tag Claude Code puts around a failed tool_result. Written both inline +# ("MCP error ...") and on a line of its own with the message +# beneath it. Stripped only where it leads the anchored line, so the anchor +# still holds: nothing but an error is ever wrapped in it, and a payload that +# merely quotes an error does not carry it. _CLAUDE_TOOL_ERROR_TAG = "" # codex wraps a refusal as: tool call failed for `/`. The # server segment is matched loosely on purpose — historical rollouts carry @@ -252,32 +282,63 @@ def extract_created_event_id(text: str) -> str | None: _UNKNOWN_RUN_ID_PREFIX = "unknown run_id:" +def _first_carrying_line(lines: list[str]) -> str | None: + """First line that carries anything, with the error tag peeled off. + + Blank lines and a bare ```` line carry no content, so + neither is the anchor: stopping on one only makes a real refusal invisible. + Skipping them widens WHAT is recognised without widening WHERE it is looked + for, which is the property the position anchor exists for. + """ + + for line in lines: + stripped = line.strip().removeprefix(_CLAUDE_TOOL_ERROR_TAG).strip() + if stripped: + return stripped + return None + + def _anchored_mcp_error_message(text: str) -> str | None: - """The refusal message when ``text`` IS a wire refusal, else None. + """The refusal message when ``text`` IS a wire refusal from agentacct. Only two positions count. Claude Code puts the whole tool_result at - "MCP error -32602: ", so the FIRST line carries it. codex wraps - the same refusal in its own "tool call failed for ..." error and puts the - cause on the line directly after "Caused by:". Anywhere else the text is + "MCP error -32602: ", so the first line that carries anything + holds it. codex wraps the same refusal in its own "tool call failed for + ..." error and puts the cause under "Caused by:". Anywhere else the text is payload — quoted, echoed, or narrated by a call that SUCCEEDED — and must not be read as agentacct having refused anything. + + A "Caused by:" that follows nothing is not a wrapper's cause header, so it + does not open an anchor; without that check the first line of any text + could nominate the second. + + The located error must also carry a code THIS server emits + (:data:`AGENTACCT_MCP_ERROR_CODES`). A client-layer failure — a timed-out + or dropped call — is rendered in the same place and the same shape, and it + is not a refusal: nothing reached agentacct to be refused. """ lines = text.splitlines() - if not lines: - return None - candidates = [lines[0].strip().removeprefix(_CLAUDE_TOOL_ERROR_TAG)] - candidates.extend( - lines[index + 1] - for index, line in enumerate(lines[:-1]) - if line.strip().lower() == _CAUSED_BY_LINE - ) + candidates: list[str] = [] + leading = _first_carrying_line(lines) + if leading is not None: + candidates.append(leading) + for index, line in enumerate(lines): + if line.strip().lower() != _CAUSED_BY_LINE: + continue + if not any(previous.strip() for previous in lines[:index]): + continue + cause = _first_carrying_line(lines[index + 1:]) + if cause is not None: + candidates.append(cause) for candidate in candidates: - match = _MCP_ERROR_MESSAGE_RE.match(candidate.strip()) + match = _MCP_ERROR_MESSAGE_RE.match(candidate) if match is None: continue + if int(match.group("code")) not in AGENTACCT_MCP_ERROR_CODES: + continue message = match.group("message").strip() - if message: + if message and message.lower() not in _TRANSPORT_ERROR_MESSAGES: return message return None @@ -351,12 +412,15 @@ def classify_refused_recording(text: Any, *, tool: Any = None) -> tuple[str | No Returns None — deliberately, not "other" — when the text is not an agentacct MCP error: a client-side refusal (codex's approval layer, Claude - Code's input validator), a successful-but-not-created-event payload that - merely QUOTES an agentacct error, or wire drift. Those are real skips but - they are NOT recording attempts this product rejected, and counting them as - such would overstate the number the whole surface exists to be honest - about. The error is therefore located by position - (:func:`_anchored_mcp_error_message`), never found anywhere in the blob. + Code's input validator), a client TRANSPORT failure that never reached this + server (request timed out, connection closed), a + successful-but-not-created-event payload that merely QUOTES an agentacct + error, or wire drift. Those are real skips but they are NOT recording + attempts this product rejected, and counting them as such would overstate + the number the whole surface exists to be honest about. The error is + therefore located by position AND required to carry a code this server + emits (:func:`_anchored_mcp_error_message`), never found anywhere in the + blob and never trusted just for looking like an MCP error. ``tool`` is the caller's allowlisted creation-tool name; codex's own "tool call failed for `/`" wrapper is used as a fallback and diff --git a/tests/test_refused_recording_attempts.py b/tests/test_refused_recording_attempts.py index 3441026..2af4807 100644 --- a/tests/test_refused_recording_attempts.py +++ b/tests/test_refused_recording_attempts.py @@ -21,6 +21,7 @@ import ast import json +import re import sqlite3 from pathlib import Path @@ -28,6 +29,8 @@ import agentacct.client_usage as client_usage_module import agentacct.log_evidence as log_evidence_module +import agentacct.mcp as mcp_module +import agentacct.work_ledger as work_ledger_module from agentacct.api import ( _REFUSED_RECORDING_REASON_LABELS, UsageDiscoveryConfig, @@ -36,6 +39,7 @@ ) from agentacct.client_usage import discover_claude_code_usage, discover_codex_usage from agentacct.log_evidence import ( + AGENTACCT_MCP_ERROR_CODES, REFUSED_RECORDING_REASON_CODES, classify_refused_recording, refused_recording_rows, @@ -335,6 +339,186 @@ def test_a_successful_call_quoting_an_error_is_not_counted_in_the_scan(tmp_path) assert "refused by agentacct" not in html +def test_a_client_transport_failure_is_not_a_refusal() -> None: + """WHOSE error it is, not only where it sits. + + A JSON-RPC error rendered in the anchor position can come from the client's + own transport instead of this server: the call timed out, or the connection + dropped. Nothing reached agentacct, so nothing was refused — reporting one + as "agentacct refused this recording call" is the same overstatement as + counting a quoted error, just arriving from the other side of the wire. + """ + + transport = [ + "MCP error -32001: Request timed out", + "MCP error -32603: Internal error", + "MCP error -32000: Connection closed", + "MCP error -32601: Method not found", + "MCP error -32700: Parse error", + "MCP error -32600: Invalid Request", + "MCP error -32001: Request timed out", + "tool call error: tool call failed for `agent-sentinel/sentinel_record_section`" + "\n\nCaused by:\n Mcp error: -32001: Request timed out", + ] + for text in transport: + assert classify_refused_recording(text, tool="agentacct_record_section") is None, text + + +def test_every_error_code_agentacct_itself_returns_still_classifies() -> None: + """The mirror of the transport test: the fix must not silence the server. + + mcp.py answers a tools/call with -32602 (InvalidParams and ValueError, + which is also how "Unknown tool" comes back), -32004 (FileNotFoundError) + and -32000 (any other exception, plus the store-less degraded server that + stays connected precisely so it can refuse). All three are agentacct + refusing a recording call and all three must be counted. + """ + + assert AGENTACCT_MCP_ERROR_CODES == {-32000, -32004, -32602} + for code in sorted(AGENTACCT_MCP_ERROR_CODES): + result = classify_refused_recording( + f"MCP error {code}: summary must be <= 1200 characters", + tool="agentacct_record_section", + ) + assert result == ("agentacct_record_section", "summary", "narrative_over_limit"), code + # The degraded server's own refusal carries no argument, but it is still a + # recording call this product turned down. + degraded = classify_refused_recording( + "MCP error -32000: agentacct is connected but has no usable store; restart the MCP" + " server with a writable --store-dir ", + tool="agentacct_record_section", + ) + assert degraded == ("agentacct_record_section", None, "other") + + +def test_the_accepted_code_set_still_matches_what_the_server_emits() -> None: + """The set is copied out of mcp.py, so it can drift out of it. + + A code the server starts returning that this list does not know about is a + silent under-count; a code it stops returning is dead weight. Read the + codes back off ``_error``'s call sites rather than off its wording, which + is the whole point of keying on the code. + + ``-32601`` is the one deliberate exclusion: it answers an unknown METHOD, + which no recording tool call can produce (an unknown TOOL comes back as a + ValueError, so -32602). + """ + + tree = ast.parse(Path(mcp_module.__file__).read_text(encoding="utf-8")) + emitted: set[int] = set() + for node in ast.walk(tree): + if not isinstance(node, ast.Call) or len(node.args) < 2: + continue + func = node.func + name = func.attr if isinstance(func, ast.Attribute) else getattr(func, "id", None) + if name != "_error": + continue + code = node.args[1] + if isinstance(code, ast.UnaryOp) and isinstance(code.op, ast.USub): + emitted.add(-code.operand.value) + + assert emitted, "no _error call sites found — the scan, not the server, is what broke" + assert emitted - {-32601} == set(AGENTACCT_MCP_ERROR_CODES) + + +def test_a_transport_failure_and_a_refusal_land_in_different_buckets(tmp_path) -> None: + """End to end on the real Claude Code channel, one session, two calls.""" + + claude_home = tmp_path / "claude-home" + project = claude_home / "projects" / "-work-project" + project.mkdir(parents=True) + lines = [_claude_usage_line(CLAUDE_SESSION)] + for tool_use_id, text in ( + ("toolu_timeout", "MCP error -32001: Request timed out"), + ("toolu_refused", f"MCP error -32602: files[0] must be project-relative: {SECRET_PATH}"), + ): + lines.append( + _claude_tool_use_line( + CLAUDE_SESSION, + tool_use_id=tool_use_id, + name="mcp__agentacct__agentacct_record_section", + ) + ) + lines.append(_claude_tool_result_line(CLAUDE_SESSION, tool_use_id=tool_use_id, text=text)) + (project / f"{CLAUDE_SESSION}.jsonl").write_text( + "\n".join(json.dumps(line) for line in lines) + "\n", encoding="utf-8" + ) + + summary = summarize_refused_recording_attempts( + discover_claude_code_usage(claude_home=claude_home, limit_sessions=10) + ) + + assert summary["outputs_skipped"] == 2 + assert summary["refused_attempt_total"] == 1 + assert _rows_by_reason(summary) == {"files_not_project_relative": 1} + # The timeout is not dropped — it is disclosed as the remainder, which is + # what keeps the two figures reconciling instead of disagreeing. + assert summary["unclassified_outputs_skipped"] == 1 + + +def test_a_refusal_still_counts_behind_blank_and_wrapper_only_lines() -> None: + """The anchor is a position, not a line number. + + Three real shapes put nothing on the first line: a tool_result that opens + with a newline, Claude Code's ```` written on a line of its + own rather than inline, and codex's cause separated from its "Caused by:" + header by a blank line. Each was a silent under-count. + """ + + message = "summary must be <= 1200 characters" + shapes = [ + f"\nMCP error -32602: {message}", + f"\nMCP error -32602: {message}\n", + f"\n\n\n MCP error -32602: {message}", + "tool call error: tool call failed for `agent-sentinel/sentinel_record_section`" + f"\n\nCaused by:\n\n Mcp error: -32602: {message}", + ] + for text in shapes: + result = classify_refused_recording(text, tool="agentacct_record_section") + assert result == ("agentacct_record_section", "summary", "narrative_over_limit"), text + + # Widening WHAT is recognised must not widen WHERE it is looked for: a + # payload that merely quotes an error is still not a refusal. + assert ( + classify_refused_recording( + f"\npytest passed\ngreen after fixing MCP error -32602: {message}\nbuild ok", + tool="agentacct_record_machine_check", + ) + is None + ) + + +def test_a_leading_caused_by_line_does_not_anchor_the_line_below_it() -> None: + """"Caused by:" is a wrapper's header — it has to wrap something. + + With nothing above it, it is just the first line of some payload, and + letting it nominate the second line lets any text opt itself in. + """ + + assert ( + classify_refused_recording( + "Caused by:\n Mcp error: -32602: summary must be <= 1200 characters", + tool="agentacct_record_section", + ) + is None + ) + assert ( + classify_refused_recording( + "\n\nCaused by:\n Mcp error: -32602: summary must be <= 1200 characters", + tool="agentacct_record_section", + ) + is None + ) + # codex's real shape always has its own wrapper line above the header. + assert ( + classify_refused_recording( + "tool call error: tool call failed for `agent-sentinel/sentinel_record_section`" + "\n\nCaused by:\n Mcp error: -32602: summary must be <= 1200 characters" + ) + is not None + ) + + def test_agent_invented_argument_names_never_become_a_field() -> None: result = classify_refused_recording(f"MCP error -32602: unexpected argument(s): {SECRET_ARG}") assert result == (None, None, "unknown_argument") @@ -695,6 +879,53 @@ def test_raw_route_renders_the_refusal_section_on_a_clean_machine(tmp_path) -> N assert "No refused recording calls were found" in page +# --------------------------------------------------------------------------- +# Cross-file copy: the next step points at a section that must exist +# --------------------------------------------------------------------------- + + +def _string_constants_containing(module, needle: str) -> list[str]: + tree = ast.parse(Path(module.__file__).read_text(encoding="utf-8")) + return [ + node.value + for node in ast.walk(tree) + if isinstance(node, ast.Constant) and isinstance(node.value, str) and needle in node.value + ] + + +def test_the_next_step_sentence_names_a_section_the_page_really_renders() -> None: + """The sentence sends the user somewhere; that somewhere is in another file. + + ``work_ledger`` tells a user with unexplained usage to look under a named + heading in Local logs, and ``api`` is what renders that heading. Rewording + either alone leaves the ledger directing people at a section that is not + there — a next step that cannot be followed is worse than none. + """ + + sentence = work_ledger_module._USAGE_WITHOUT_MCP_CONTEXT_NEXT_STEP + quoted = re.findall(r'"([^"]+)"', sentence) + assert quoted, sentence + heading = quoted[0] + + # Rendered in both states, so the reader who follows the pointer lands on + # the section whether or not they have refusals. + esc = lambda value: str(value) # noqa: E731 - the route's escaper, not under test here + assert heading in _refused_recording_html({}, esc) + assert heading in _refused_recording_html( + { + "refused_attempt_total": 1, + "sessions_with_refusals": 1, + "rows": refused_recording_rows({("agentacct_record_section", "summary", "narrative_over_limit"): 1}), + }, + esc, + ) + + # One copy of the sentence, so the attention group and the blind spot + # cannot be reworded apart from each other. + assert work_ledger_module._ATTENTION_GROUP_NEXT_STEPS["usage_truth_without_mcp_context"] is sentence + assert len(_string_constants_containing(work_ledger_module, heading)) == 1 + + # --------------------------------------------------------------------------- # Retroactivity: the whole point # --------------------------------------------------------------------------- From d6ca709b0195d5cc0170d1aaedbcb85a08e4560d Mon Sep 17 00:00:00 2001 From: mikehasa Date: Fri, 31 Jul 2026 05:37:27 +0800 Subject: [PATCH 9/9] docs(changelog): record the refused-recording visibility --- CHANGELOG.md | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 06f584c..4270633 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,19 @@ follow [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ## [Unreleased] +### Added +- Local logs now shows the recording calls agentacct refused, with counts and a + fixed set of reason codes. A refused call previously left no trace anywhere — + no store entry, no counter, no log line — so an agent that failed to record + was invisible to you and to the maintainer, while the dashboard told you to + record more work. The figure is derived at read time from data already on + disk, so it covers refusals that predate this release, and it stores only + counts and reason codes, never the offending value or path. + +### Changed +- The "usage without work context" prompt no longer blames you for all of it; + it now points at the refused-recording list for the part agentacct caused. + ### Fixed - `agentacct_record_section` now accepts `title` as an alias for `section_title`. The recording contract agentacct ships to every client told