Fix token_audit.py false-positive detectors - #1
Open
EdbertChan wants to merge 3 commits into
Open
Conversation
Claude tool errors were converted into synthetic __ERROR__-prefixed tool names and then rediscovered by prefix matching. Track error results directly from boolean is_error fields so ordinary output and tool names cannot collide with the detector. Normalize both supported file_path and path inputs before redundant-read comparisons. This keeps repeated reads detectable without collapsing different valid files into the same None key. Add regression coverage for false/absent error flags, genuine errors, repeated alias-path reads, and distinct alias-path reads.
…— Auditing a real session transcript with `token_audit.py` reported "19 tool errors" and "1 redundant read" that were mostly or entirely wrong on manual inspection. Two separate bugs were traced by an independent review pass (not yet verified against the actual current source -- read it first): 1. The tool-error detector matches on a substring (e.g. checking whether some string like "error" appears in tool-result content) instead of reading the transcript's actual structured `is_error` flag on each tool_result block. This flags normal tool output that merely mentions the word "error" (e.g. in file content, log lines, or a status message) as if the tool call itself failed. 2. The redundant-read detector has a filename-normalization bug: it appears to produce `None` or `(None, None)` for some inputs when extracting/normalizing the file path from a Read tool call, which breaks path-equality comparisons across reads and produces spurious "redundant read" matches (or misses real ones -- confirm which, empirically, by writing the repro first). Review claim: both detectors now key off the transcript's actual structured data (the real `is_error` flag; a correctly normalized file path) instead of a substring/lossy heuristic, and a real session transcript that previously mis-flagged both cases no longer does. Review lane: behavior Safety invariant: This only changes detection logic inside `token_audit.py`; it must not change the script's output format/schema in a way that breaks other tooling that consumes its output (check for other callers/consumers of this script's output before changing shape). Slice rationale: Both bugs live in the same script and were found in the same review pass; if the repo's own conventions call for splitting them into two separate reviewable changes once you see the actual diff size, do that instead of forcing one combined change. Goal: `token_audit.py` no longer flags a normal tool result that merely contains the word "error" as a tool error, and no longer produces spurious redundant-read matches (or misses) caused by a broken filename normalization. Motivation: A recent real audit run on a genuine session transcript reported these two findings, and manual inspection showed both were false positives -- undermining trust in every future run of this report until fixed. Implementation details: | Read the actual current source of `token_audit.py` first -- treat the bug descriptions above as a starting hypothesis from an independent review pass, not verified ground truth; confirm the exact mechanism by reading the real code and, ideally, reproducing against a real or realistic session transcript fixture before changing anything. For the tool-error detector: find wherever it currently classifies a tool call as failed, and change it to read the transcript's actual `is_error` field on the relevant tool_result content block instead of any substring/keyword match. For the redundant-read detector: find the file-path normalization function used to compare Read calls, and fix whatever produces `None` or `(None, None)` for valid inputs -- likely a missing case in extracting the path argument, or an unsafe attribute/key access that silently returns None instead of raising or handling the actual input shape. Non-goals: Does not change any other detector or report section in `token_audit.py`. Does not add new detectors. Does not touch other scripts in this repo unless the fix genuinely requires it (e.g. a shared helper both detectors call into). Acceptance criteria: - A new test (or fixture-based repro, matching however this repo already tests scripts in this directory -- check for an existing pattern first) that constructs a minimal transcript where a tool result's content happens to contain the word "error" but its real `is_error` field is false/absent, asserting the tool-error detector does NOT flag it -- proven failing before the fix, passing after. - A second test proving the tool-error detector still correctly flags a transcript entry whose `is_error` field is genuinely true (no regression in real detection). - A third test proving the redundant-read detector's path normalization no longer returns None/(None, None) for the specific input shape that broke it, with a case showing two reads of the literal same file are still correctly detected as redundant, and two reads of genuinely different files are not. - Test and fix committed together, following this repo's own existing commit/PR conventions (read any CONTRIBUTING/CLAUDE.md-equivalent docs, or infer from recent commit history, before assuming Invoker's specific PR-body schema applies here -- this is a different repo). Exit code: 0 Invoker-Finalize-Id: a59b419c-ab0a-4ca5-83c8-863a5502efc9
…tectors/g1.t1.a-aa6657970-d0039e5d — Auditing a real session transcript with `token_audit.py` reported "19 tool errors" and "1 redundant read" that were mostly or entirely wrong on manual inspection. Two separate bugs were traced by an independent review pass (not yet verified against the actual current source -- read it first): 1. The tool-error detector matches on a substring (e.g. checking whether some string like "error" appears in tool-result content) instead of reading the transcript's actual structured `is_error` flag on each tool_result block. This flags normal tool output that merely mentions the word "error" (e.g. in file content, log lines, or a status message) as if the tool call itself failed. 2. The redundant-read detector has a filename-normalization bug: it appears to produce `None` or `(None, None)` for some inputs when extracting/normalizing the file path from a Read tool call, which breaks path-equality comparisons across reads and produces spurious "redundant read" matches (or misses real ones -- confirm which, empirically, by writing the repro first). Review claim: both detectors now key off the transcript's actual structured data (the real `is_error` flag; a correctly normalized file path) instead of a substring/lossy heuristic, and a real session transcript that previously mis-flagged both cases no longer does. Review lane: behavior Safety invariant: This only changes detection logic inside `token_audit.py`; it must not change the script's output format/schema in a way that breaks other tooling that consumes its output (check for other callers/consumers of this script's output before changing shape). Slice rationale: Both bugs live in the same script and were found in the same review pass; if the repo's own conventions call for splitting them into two separate reviewable changes once you see the actual diff size, do that instead of forcing one combined change. Goal: `token_audit.py` no longer flags a normal tool result that merely contains the word "error" as a tool error, and no longer produces spurious redundant-read matches (or misses) caused by a broken filename normalization. Motivation: A recent real audit run on a genuine session transcript reported these two findings, and manual inspection showed both were false positives -- undermining trust in every future run of this report until fixed. Implementation details: | Read the actual current source of `token_audit.py` first -- treat the bug descriptions above as a starting hypothesis from an independent review pass, not verified ground truth; confirm the exact mechanism by reading the real code and, ideally, reproducing against a real or realistic session transcript fixture before changing anything. For the tool-error detector: find wherever it currently classifies a tool call as failed, and change it to read the transcript's actual `is_error` field on the relevant tool_result content block instead of any substring/keyword match. For the redundant-read detector: find the file-path normalization function used to compare Read calls, and fix whatever produces `None` or `(None, None)` for valid inputs -- likely a missing case in extracting the path argument, or an unsafe attribute/key access that silently returns None instead of raising or handling the actual input shape. Non-goals: Does not change any other detector or report section in `token_audit.py`. Does not add new detectors. Does not touch other scripts in this repo unless the fix genuinely requires it (e.g. a shared helper both detectors call into). Acceptance criteria: - A new test (or fixture-based repro, matching however this repo already tests scripts in this directory -- check for an existing pattern first) that constructs a minimal transcript where a tool result's content happens to contain the word "error" but its real `is_error` field is false/absent, asserting the tool-error detector does NOT flag it -- proven failing before the fix, passing after. - A second test proving the tool-error detector still correctly flags a transcript entry whose `is_error` field is genuinely true (no regression in real detection). - A third test proving the redundant-read detector's path normalization no longer returns None/(None, None) for the specific input shape that broke it, with a case showing two reads of the literal same file are still correctly detected as redundant, and two reads of genuinely different files are not. - Test and fix committed together, following this repo's own existing commit/PR conventions (read any CONTRIBUTING/CLAUDE.md-equivalent docs, or infer from recent commit history, before assuming Invoker's specific PR-body schema applies here -- this is a different repo).
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_78303f2b-e8f7-4090-a863-aad8d33b8c46) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
token_audit.pynow trusts Claude’s structuredis_errorflag, preventing ordinary output or tool names containing “error” from being reported as failures.Read and edit paths now normalize both
file_pathandpath, preserving genuine redundant-read detection without conflating missing or different files.Review Claim
Approve the corrected tool-error and redundant-read classifications while preserving the audit report’s existing output schema.
Review Lane
behaviorReview Unit
validation-policySafety Invariant
Only detector classification logic changes. Existing report headings, line formats, and consumer-facing output structure remain unchanged.
Slice Rationale
Both false positives originate in the same audit function and share one focused regression suite, so they form one locally reviewable correction.
Non-goals
token_audit.pyand its existing test file.Test Plan
Test Plan
python3 -m unittest discover -s skills/reflect/scripts/tests -v— 29 tests passed, including false and genuine errors plus repeated and distinct alias paths.Revert Plan
Revert Plan
git revert <PR merge commit SHA>Note
Low Risk
Classification-only changes in reflect audit scripting and tests; no auth, data, or consumer-facing API changes beyond more accurate counts.
Overview
Fixes false positives in Claude session
token_auditfor tool failures and redundant reads, without changing report headings or line formats.Tool errors now count only
tool_resultblocks withis_errorstrictlyTrue, tracked in a dedicated list instead of synthetic__ERROR__:entries on the tool-call sequence—so benign result text or odd tool names no longer inflate the error count.Redundant reads resolve file identity via a shared
_claude_file_path()that acceptsfile_pathorpath, normalizes withos.path.normpath, and skips reads with no path; Edit/Write use the same helper so alias keys still participate in “edited since” logic.Reviewed by Cursor Bugbot for commit 581c377. Bugbot is set up for automated code reviews on this repo. Configure here.