Stop conditions: triage divergence (65% agreement) and a real truncation hole - #18
Merged
Conversation
…ion hole Re-triaged 20 of the 73 field findings. Strict agreement with the earlier read is 13/20, and all five divergences run the same direction - earlier true positive, now false positive. That is systematic optimism, not noise, so the "roughly 12 of 17 gating findings are genuine" claim does not survive. The 233 -> 73 headline is directionally right; its quality split is not trustworthy enough to optimise against. Two causes recur: a committed credential that is public by design is not a compromise, and a framework implementing its own documented capability is not an application granting one. Separately, the 1,039,776-line equivalence run proved nothing about lines over the 4096 cap - the harness truncated every line before comparing, so the only population that could show a regression was excluded. Testing past the cap shows a credential at offset 5,000 of a minified line is not detected at all. truncated_lines reports it and the scan still exits 0. That is a recall hole in the flagship category, on exactly the file shape where inlined keys occur. Nothing fixed, per instruction. Also lists the claim surfaces that live outside version control, where grep cannot reach. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q5nXA9BJKdmye9a9FaHdc6
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5081c36939
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Re-triaged a deterministic stratified sample of 20 of the 73 field findings (round-robin | ||
| by rule so every rule appears), reading each with four lines of surrounding context. | ||
|
|
||
| **Strict agreement: 13/20 = 65%. Divergence: 5/20 = 25%. Two further partials.** |
There was a problem hiding this comment.
The reported 13/20 strict-agreement rate counts rows #11 and #19 as agreements even though both changed from borderline to TP (weak); meanwhile, rows #6 and #15 changing from borderline to FP are treated as partials. Under the stated strict criterion, only 11 rows retain the same verdict, so the headline agreement rate should be 55%, or the document should define and apply a non-strict equivalence consistently.
Useful? React with 👍 / 👎.
Comment on lines
+805
to
+807
| **Methodological caveat, stated plainly:** this is the same reader re-reading, not an | ||
| independent triage. Genuine independence needs a second person. A same-reader re-read | ||
| diverging 25% is a floor on the error, not a measurement of it. |
There was a problem hiding this comment.
Without an independent adjudicator or ground truth, a same-reader reread disagreement does not establish a lower bound on classification error: either the earlier or later verdict may be wrong, and both may use the same flawed assumptions. The 25% figure supports label instability, but calling it a floor on error overstates the evidence used to justify this stop condition and directly conflicts with the following claim that it is not an error measurement.
Useful? React with 👍 / 👎.
…the rules A credential inlined in a minified bundle sat past the 4096-char bound and was invisible. That is the flagship category failing on the file shape where the leak most often is. Bounds now live per rule: AG001 declares UNBOUNDED and reads lines whole, everything else stays capped. That is only safe because it was measured. tools/measure_linearity.py reports growth exponents of 0.98-1.02 for every AG001 pattern, worst case 40ms on a 1MB line - and it validates itself against the pre-fix cubic AG004 pattern first, exiting without reporting if that control fails to register. The harness deciding whether a pattern may run unbounded is the last place a silent pass is acceptable. Truncation is declared rather than gated. Every report carries which lines were clipped, by how much, against which bound; --fail-on-incomplete opts into exit 2. Default exit codes unchanged. The five field projects turn out to contain 286 lines no bounded rule read in full, 270 in crewAI alone - previously invisible. From the triage: a credential published by design is capped at Low with a message saying so, because reporting a PostHog project key as a critical compromise is wrong even though a credential is committed. Vendored and site-packages paths downgrade like fixtures. shell=True with an argument list now describes the portability bug it actually is - POSIX passes only argv[0] and discards the rest - instead of an injection risk that is not there. Corpus precision falls from 100% to 72.2%, recall unchanged. The rules did not get worse. Six field false positives are folded in as true negatives, and the previous 100% was measured against cases written after the bugs were known. AG003 and AG006 scored perfectly while being ~98-100% false positives in the field; that gap was a corpus validity failure and is now five reproducible CI failures, none of them fixed yet. The 73 field findings ship as a dataset with the upstream commits, the sampling method, and the same-reader bias stated plainly - including that all five divergences moved the same direction. It is not cited in the README and must not be. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q5nXA9BJKdmye9a9FaHdc6
…, rewrite README Three things the previous unit left resting on memory rather than enforcement. Coverage computing against the tightest applied bound was caught by noticing, not by test. Now pinned by two rules bounded at 100 and 4096 over a 200-char line, verified to fail when min is flipped back to max. The companion test's docstring claimed it discriminated the same inversion; it does not, because positive bounds are filtered first. Corrected rather than left, and it now names the test that does. measure_linearity discovers unbounded rules and their patterns by introspection and stresses each with inputs built from its own literal runs, so a contributor's pattern is gated without anyone remembering to add it. --check runs in CI. It earned that immediately: the first env-assignment pattern written in this session used two unbounded quantifiers around an alternation and measured exponent 2.00 - 918ms on 32KB, roughly 16 minutes extrapolated to 1MB, on a rule that reads lines unbounded. Rewritten to capture the name once and test it in Python. Four of the five corpus failures are fixed and the fifth is accepted with reasoning. AG003's function_map clause is removed rather than narrowed: it flagged a function map's existence, not an over-broad one, and a clause that cannot express the breadth the rule is named for does not belong in it. AG002 needed two fixes - resolving module constants was useless while the check demanded every argument be fixed, since exec(code, namespace) passes a globals dict second. AG008 is a strict xfail: separating a tool deleting a caller's path from a function deleting its own temp file needs data flow, and a name-based guess would be a blind spot an attacker could name their way into. Corpus precision 72.2% -> 93.3%, recall unchanged. .env is now scanned. A secrets scanner that cannot read the canonical secrets file is indefensible. Values there are unquoted, which the quoted pattern misses entirely, so there is an env-only pattern - requiring quotes is what stops the general one matching password = get_password(). README rewritten around what make bench reproduces. Every number in it was checked against a command, which caught three wrong claims: the labelled count, an assertion that every true negative came from real-world measurement when 6 of 18 did not, and a recall-trade list crediting this unit with a narrowing from earlier work. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q5nXA9BJKdmye9a9FaHdc6
The push carrying 443e34c did not fire a pull_request synchronize event, so PR #18 shows no checks despite CI passing on a manual dispatch of the same tree. Empty commit to re-fire the event. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q5nXA9BJKdmye9a9FaHdc6
PR #18 was DIRTY, not un-triggered. main advanced with six Dependabot merges while this branch also edited ci.yml, and pull_request workflows run against refs/pull/N/merge - which GitHub cannot compute while a branch conflicts. That, not a dropped webhook, is why no checks reported. One conflict: this branch inserted the measure_linearity gate immediately above upload-artifact@v4, and main bumped that action to v7. Kept both. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q5nXA9BJKdmye9a9FaHdc6
.env support went in with two corpus files and no coverage of the shapes
that actually occur. Adding them found two false positives immediately.
`SERVICE_TOKEN=$OTHER_TOKEN` was reported as a critical committed secret.
The placeholder filter matched `${VAR}` but not a bare `$VAR`, and an
unbraced shell reference is a pointer to a value held elsewhere, not the
value. `sk-proj-replace-this-before-running` in a .env template was
reported too - it carries no word the filter recognised.
Both are value-shape problems, not file-classification ones, so the fix is
in the placeholder filter rather than in how templates are treated. That
distinction matters: a genuine credential committed to .env.example is a
real leak and is still reported, at reduced severity. Suppressing by
filename would have hidden it.
The new .env.local case covers commented-out credentials, empty values,
braced and unbraced interpolation, values under the length floor, and
ordinary configuration - each failing to fire for a different reason.
Verified non-vacuous: reverting the filter drops corpus precision to 82.4%
and names both cases.
AG001 declares UNBOUNDED, so the linearity gate was re-run after touching
its patterns: still linear, 9 patterns across 1 rule.
README now frames 93.3% over 33 files as a regression gate rather than a
precision estimate. A corpus whose negatives were selected from observed
failures is biased towards passing, and saying so is the difference between
evidence and marketing.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Q5nXA9BJKdmye9a9FaHdc6
tests/test_rules.py asserted AG006 should fire on `fetch(url)`. That assertion was wrong, and a wrong assertion is worse than no assertion, because it defends the defect against exactly the change that fixes it - narrowing AG006 broke this test, which is how it surfaced. It is now tests/corpus/true_negatives/js_fetch_local_url.js, so the rule is measured on the case rather than asserted about. The positive direction stays covered by `fetch(user_input)`. Recorded in DECISIONS.md as the sixth recall trade, and called out as the sharpest of them: a genuinely attacker-controlled URL reaching `fetch(url)` is now missed. It was taken because the rule cannot see provenance at all - `url` is the ordinary name for a variable holding a URL, and treating the name as evidence produced false positives on every call in the field sample and true positives on none. Closing that properly needs data flow, not a longer name list. The trade table said "five times" while listing six. Fixed. The README's field-derived-versus-written split is left unquantified here: the number is currently inferred by sniffing prose in the manifest, which already miscounts this new entry. Unit C replaces that with an explicit marker, and the count follows from it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q5nXA9BJKdmye9a9FaHdc6
…parately A corpus whose negatives were selected from failures it already saw is biased towards passing. The blended 93.3% flatters, and nothing in the repository distinguished the cases drawn from real projects from the ones composed while reading the rules. Every entry now declares origin: field or written, enforced by bench rather than inferred. The previous inference sniffed prose in `why` for "measured" or "observed", and it miscounted the moment an entry said "field finding" instead - which happened one commit ago. bench --field-only scores the 13 field-derived cases alone, and runs in CI next to the full pass. Both are in the README, because publishing only the higher one is the thing this split exists to prevent. Also added a "behave as labelled" count. Precision over the field-derived subset is structurally skewed - a field false positive becomes a corpus true negative, so that subset is nearly all negatives and its precision figure says less than it appears to. How many cases behave as labelled survives that: 33 of 34 overall, 12 of 13 field-derived, the same AG008 failure in both. Two labels corrected while doing this: .env.local and .env.sample are `written`, not `field`. The false positives they caught were found by composing them, not by scanning anything. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q5nXA9BJKdmye9a9FaHdc6
DECISIONS.md mentioned this in a clause. It deserves the numbers, because it is the strongest available argument for the gate being CI rather than a convention. The .env pattern written minutes after the gate itself used two unbounded quantifiers around an alternation - exactly the shape the gate exists to catch. It measured exponent 2.00, 918ms on 32KB, extrapolating to roughly 16 minutes on one 1MB line, on the only rule that reads lines unbounded. Rewritten: exponent 0.99, 9.3ms at 1MB. The point on record is not that a mistake happened. It is that the author of the gate, fully aware of why it existed, immediately wrote the thing it guards against and did not notice until a machine measured it. A review checklist could not have caught that - the reviewer wrote the checklist. CONTRIBUTING's harness-failure count goes from four to five: the equivalence run that truncated every line before comparing belongs on that list, since it excluded the only inputs that could have shown the regression. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q5nXA9BJKdmye9a9FaHdc6
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q5nXA9BJKdmye9a9FaHdc6
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
2 participants
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.
Documentation only. No code changed, nothing fixed — both outcomes are declared stop conditions in the queue.
1. Hand triage materially disagrees — 65% strict agreement
Re-triaged a deterministic stratified sample of 20 of the 73 field findings.
13/20 strict agreement. 5/20 divergence, and all five run the same direction: earlier TP, now FP. That is systematic optimism, not noise.
Two recurring causes:
phc_project key and a Supabase anon key are published deliberately. Reporting them as Critical "credential compromise" is wrong even though a credential is literally committed.allow_delegation = Trueinside crewAI's own_create_manager_agentis the framework implementing its documented mode.The claim that "roughly 12 of 17 gating findings are genuine" does not survive. On this sample only four are TPs and two of those are weak.
Caveat stated plainly: this is the same reader re-reading, not an independent triage. A same-reader re-read diverging 25% is a floor on the error, not a measurement of it.
2. Truncation is a real detection hole
The 1,039,776-line equivalence run proved nothing about over-cap lines — the harness contained
ln = ln[:4096], truncating every line before comparison. The only population that could show a regression was excluded by construction. Fourth occurrence of this class of harness error.Only 1 line in 1,039,808 was actually over-cap, so the equivalence claim is barely weakened — but detection past the cap is broken:
A credential past character 4096 of a minified line is not detected. The scan reports
truncated_lines=1and exits 0. Recall defect in the flagship category, on exactly the file shape where inlined keys occur.Not done
Queue items 2, 4, and 10 consume the field number and are blocked behind a re-triage decision. Items 5–9 are independent but the queue is ordered and both stop conditions fired at items 1 and 3.
🤖 Generated with Claude Code
https://claude.ai/code/session_01Q5nXA9BJKdmye9a9FaHdc6