fix(GATE-PR-SIZE-BINARY): retire the fail-closed binary guard — a golden is binary, so no golden-bearing PR could merge (#615) - #619
Merged
Conversation
…e every golden-bearing PR unmergeable (#615) `check-pr-size.py` errored on any path git reports as binary -- "is not reviewable as text" -- with no exemption route. `--pr-number` was accepted and validated, then never consulted for the decision. Parity goldens are binary by nature, so no PR that captures an oracle golden could reach main at all. This is a deliberate retirement of a rule, not a red run repaired. Arguing it here because there is no waiver registry: 1. It contradicted this checker's own model. The `asset` class exists for shipped artwork and the `SITE_ASSET` note says so outright -- "the binaries among them have no reviewable line budget at all, so they take the `asset` class the same way any other shipped artwork does." The classifier was built to give binaries a class; the guard then refused every one regardless. 2. It post-dated the precedent it rejected. The guard landed 450a1b6 (2026-08-10); the Tenstorrent golden precedent landed 971d550 (2026-08-09). The one post-guard golden commit on main, 5080983 with 16 .npy files, also trips it and reached main by a route the current PR-only ruleset no longer permits. A rule nothing in the tree can satisfy is not enforcing a standard, it is blocking a lane. 3. Its protection was already carried elsewhere, and is kept. `classify_path` runs directly above and raises on any path without a class. An unclassified binary is still refused -- now with a message naming the real defect, which the author can act on, instead of an unfixable property of the file. What was removed is only the blanket refusal of CLASSIFIED binaries. Not an allowlist instead: a roster of blessed binary paths is a shared must-write surface, so every golden-bearing PR would edit it -- exactly the lock AGENTS.md forbids. Classification already partitions by where a file lives, which is the derived-at-read-time shape. This removes no size rule because none was left: the per-class line budgets were retired 2026-08-10 and stay retired. Nothing in this file measures a diff. RED before, on the unmodified checker, for the intended reason -- all four subtests of the new case die on the guard: AssertionError: Lists differ: ["binary change 'website/static/fonts/sora-700.woff2' is not reviewable as text"] != [] 4 failed, 43 passed, 119 subtests passed GREEN after: 43 passed, 123 subtests passed. The wider governance suite holds at 92 passed, 125 subtests. `tests/scripts/test_cpu_kernel_bench.py` fails collection on unmodified main too (it wants a built benchmark binary) and is untouched by this change. `test_binary_changes_fail_closed_instead_of_becoming_free` is deleted because it states the retired rule and cannot survive it. Its replacement, `test_an_unclassified_binary_is_still_refused`, pins the half that stays and is green on both sides -- a rail, not the evidence. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: Claude:claude-opus-5 [Claude Code]
…d a mislabelled evidence number (#615) Repairs from the fresh review of #619. No behaviour change; the checker and its tests are untouched. Three places still described the guard as live. Retiring a rule and leaving its description standing is the same defect as leaving a refuted rationale in a spec, so these are fixed in the same flow rather than deferred: - `.github/workflows/ci.yml:403-407` still advertised "the fail-closed binary guard" as one of four things the required `pr-size` job enforces. That is the first place anyone auditing the gate from CI reads. - `website/README.md:60` still justified shipping no woff2 on the grounds that `check-pr-size.py` "refuses binary changes outright", and cited a waiver registry that was itself retired earlier. Both halves were false, and this PR's own new test asserts `website/static/fonts/sora-700.woff2` is accepted -- a shipped rationale contradicted by a test in the same commit. The decision not to self-host a face stands; it is now argued on its own merits. - Spec §5 item 3 said the multi-contract test would be REWRITTEN to keep a binary clause. What shipped deletes that clause, because cases 1 and 2 carry the coverage directly and duplicating it would hide which contract failed. The spec now records what shipped. Also corrected, and worth naming plainly: spec §7 recorded the stop-condition check as `8 failed, 20 passed, 2 skipped` on both trees. That was a THREE-FILE SUBSET mislabelled as the whole suite -- no stated command produced it. Run properly and sequentially on both trees, `tests/scripts/` is: HEAD 9 failed, 1276 passed, 3 skipped, 1510 subtests BASE 9 failed, 1275 passed, 3 skipped, 1506 subtests with IDENTICAL failure sets, and the +1/+4 delta being exactly this change (one test deleted, two added, four new subtests). The substantive claim the number was offered for -- that this adds no failure -- holds; the number did not. Two spec claims were wider than the evidence and are narrowed: - §4 now states the real scope. Goldens live under `tests/`, so they classify as `product`; permitting them necessarily permits binaries across `src/`, `scripts/`, `tools/` and `benchmarks/`, and the same delete admits them to `procedure`, `project_record`, `ci` and `vendored_dependency`. Measured, not inferred: a blob at `src/vllm/blob.dat` and binary bytes replacing `.agents/workflow.md` are refused at BASE and pass at HEAD. Recorded as an accepted trade with what still refuses them, not left implicit. - §3's appeal to the `SITE_ASSET` comment over-generalized: that comment speaks for `website/static/`, not for every class. §4 now names the narrower alternative (a derived regex class for the goldens directory) and says why it was not taken, instead of rebutting only the allowlist strawman. §1's scope list claimed the `SITE_ASSET` comment was in scope; it is untouched, correctly, because it never described the guard and reads correctly without it. Verified: 375 passed, 1072 subtests across every suite that reads `ci.yml` or `website/README.md` plus `test_check_pr_size`. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: Claude:claude-opus-5 [Claude Code]
The roadmap issue table conflicted: main added rows while this branch added one. Resolved by the keyed-record rule -- took origin/main's file WHOLESALE, then reapplied only the #615 row, asserting the insertion anchor was unique before writing. Verified: exactly 1 line added, 0 removed against origin/main's version, so no other key moved. Re-gated after the merge rather than trusting a clean merge: test_check_pr_size 43 passed / 123 subtests, check-agent-record OK, and the behaviour itself still holds -- a golden path is ACCEPTED, junk/blob.bin is still refused as an unclassified path. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: Claude:claude-opus-5 [Claude Code]
Second re-merge; main moved again while CI ran. Only `.agents/roadmap_v1.md` conflicted, the same keyed-record collision as before: main added issue rows while this branch added one. Resolved the same way -- took origin/main's file WHOLESALE, reapplied only the #615 row, anchor uniqueness asserted before the write. Verified 1 line added, 0 removed against origin/main's version. `.github/workflows/ci.yml` auto-merged; confirmed by inspection that this branch's `pr-size` job comment survived intact rather than being reverted to main's version, since a silent revert there is exactly what an auto-merge of two edits to the same comment block would look like. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: Claude:claude-opus-5 [Claude Code]
…ssification failed on main itself (#668) Found while re-merging main for #615, and fixed in the same flow per AGENTS.md: small, obvious, adds no semantics, and currently reds `main`. The secondary-oracle registry (AGENTS.md, "When vLLM has no implementation") landed `.agents/oracles/<id>.md` with no pattern in `check-pr-size.py`, so all eight tracked files are unclassified and `classify_path` RAISES. Reproduced on UNMODIFIED origin/main before touching anything, so this is not inherited from the binary-guard work in this branch: test_every_tracked_and_current_change_path_is_classified AssertionError: Lists differ: ['.agents/oracles/README.md', '.agents/ora[200 chars].md'] != [] First list contains 8 additional elements. Consequence: any PR recording an oracle pin is refused by a required check with "unclassified repository path". The whole point of one-file-per-oracle is that pinning is cheap; an unclassified path makes it impossible, so the registry was unusable as designed from the commit that introduced it. `.agents/oracles/*.md` is structurally identical to `.agents/specs/` and `.agents/claims/` -- one file per key, globbed for reading, deliberately not a shared table every change must write -- so it takes the same `procedure` class they already take. No new class, no new semantics. Deliberately a pattern and not a directory exemption: a non-.md file or a nested path under `.agents/oracles/` still fails closed, because AGENTS.md forbids hiding mutable files behind a blanket directory rule. RED before, on this branch with only the one-line wiring reverted -- all eight oracle files plus the tracked-paths test: 9 failed, 44 passed, 126 subtests passed GREEN after: 45 passed, 134 subtests passed. Two tests, not one. `test_every_secondary_oracle_file_classifies` asserts over `git ls-files .agents/oracles/` rather than a hardcoded sample, so a ninth oracle added without a class is caught here instead of in someone's PR. `test_oracles_is_a_pattern_not_a_blanket_directory_exemption` pins the fail- closed half, so a later widening to the whole directory turns it red. Closes #668. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: Claude:claude-opus-5 [Claude Code]
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.
Closes #615. Unblocks #431.
What
scripts/check-pr-size.pyerrored on any path git reports as binary —"is not reviewable as text"— with no exemption route.--pr-numberwas accepted and validated, then never consulted for the decision.Parity goldens (
*.npy,*.i32) are binary by nature, so no PR that captures an oracle golden could reach main at all.This drops that branch. It removes no size rule, because none was left: the per-class line budgets were retired 2026-08-10 and stay retired. Nothing in this file measures a diff.
Why this is a retirement, not a gate weakened to go green
Argued here and in the commit message, since there is no waiver registry.
assetclass exists for shipped artwork, and theSITE_ASSETcomment says so outright — "the binaries among them have no reviewable line budget at all, so they take theassetclass the same way any other shipped artwork does." The classifier was built to give binaries a class; the guard then refused every one regardless.450a1b696(2026-08-10); the Tenstorrent golden precedent landed971d55063(2026-08-09). The one post-guard golden commit on main (5080983fd, 16.npyfiles) also trips it, and reached main by a route the current PR-only ruleset no longer permits. A rule nothing in the tree can satisfy is not enforcing a standard, it is blocking a lane.classify_pathruns directly above and raises on any path without a class. An unclassified binary is still refused — now with a message naming the real defect, which an author can act on. Only the blanket refusal of classified binaries goes.Not an allowlist instead: a roster of blessed binary paths is a shared must-write surface, so every golden-bearing PR would edit it — exactly the lock AGENTS.md forbids. Classification already partitions by where a file lives, which is the derived-at-read-time shape.
Evidence
RED before, on the unmodified checker, for the intended reason — all four subtests die on the guard:
GREEN after:
43 passed, 123 subtests passed. Withtest_agent_record:92 passed, 125 subtests.It does what it is for:
The checker accepts its own diff, satisfying the
governance_checkerevidence contract it enforces:check-pr-size.py --base 7572b0f4e --head <head>→ OK, exit 0.Stop condition checked, not assumed.
tests/scripts/run sequentially on both trees — HEAD9 failed, 1276 passed, 3 skipped, 1510 subtests; BASE7572b0f4e9 failed, 1275 passed, 3 skipped, 1506 subtests— with identical failure sets (test_gen_vulkan_spirv,test_check_windows_portability,test_mlx_system_headers,test_now_render, all pre-existing). The +1 passed / +4 subtests is exactly this change: one test deleted, two added. This change adds no failure.(Corrected: an earlier revision of this body recorded
8 failed, 20 passed, 2 skippedfor that check. That was a three-file subset mislabelled as the whole suite — no stated command produced it. The claim it was offered for is unchanged; the number was wrong.)Tests
test_a_classified_binary_is_accepted— new, RED-first. Goldens and awebsite/static/asset produce no error. Asserts throughclassify_pathrather than a hardcoded class, so it survives a golden being reclassified.test_an_unclassified_binary_is_still_refused— new rail. Keeps the retirement scoped, and asserts the message no longer says "not reviewable as text". Green on both sides, so it is a pin, not evidence.test_binary_changes_fail_closed_instead_of_becoming_free— deleted. It states the retired rule and cannot survive it.test_retiring_the_budget_did_not_retire_the_other_contracts— reworded; its classification and checker-evidence clauses are untouched and still bite.Spec:
.agents/specs/gate-pr-size-binary.md.Found while reviewing external-contributor PRs — this was the single largest blocker across that batch.
🤖 Generated with Claude Code