fix(audit): a pointer is not a rule document, so stop scoring it (0.1.0-alpha.12) - #41
Merged
Conversation
robinhood-trader reported Low-Yield 1/2. The failing file is its 10-line CLAUDE.md, an `@AGENTS.md` shim — a redirect to the canonical document, which carries no rules of its own. Demanding rationale and examples of a redirect asks the maintainer to pad it. Pointers now leave the Low-Yield DENOMINATOR rather than being counted and forgiven, because they were never rule documents to score. robinhood-trader goes 1/2 -> 0/1. Scoped to Low-Yield deliberately, and this is the interesting constraint: the same file still counts toward Context Load Pressure, which held at 343 lines across the change. A shim is additive there — Claude loads the shim AND the file it imports — so a blanket removal from the scoring surface would have silently undercounted load by 11 lines. Filtering the whole surface was my first instinct and it was wrong. The signal is the import, not the length. A regression test pairs an `@AGENTS.md` shim with a non-pointer file of the same size that has neither Why nor examples; the first leaves the denominator (0/1), the second stays and fails (1/2). On the documentRole consolidation: no, not now, and the code is why. See the report for the full argument, but in short — the four predicates are consulted at four different stages and the same role earns OPPOSITE treatment across checks, so a shared enum would unify the classification while leaving four distinct policies behind. isSymlinkAlias is also consumed by exactly one caller (drift), while audit reaches the same outcome by fingerprint dedup, so folding them together would change audit behaviour nobody asked to change. What the code DID agree with is smaller and real: importsItsMirrorSource (audit.ts) and importsRootMirror (rule-loading.ts) were two implementations of the same regex, one re-reading the file from disk on every call. I introduced the second in #39 without collapsing the first. Both now delegate to isPointerDocument in scripts/lib/document-role.ts, which names the role without committing to the full refactor — the seam a future documentRole would grow from, if the fifth case ever justifies it. Corpus census across the same 138 files used for alpha.11: exactly ONE pointer document exists fleet-wide, robinhood-trader/CLAUDE.md. The per-file hasWhy pass rate is unchanged at 61/138 (44%), exactly as it should be — this change alters set membership, not per-file classification, so the alpha.11 instrument is expected to be flat here and its flatness is the evidence that nothing was loosened. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 49 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughChangesAlpha version alignment
Pointer document scoring
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/shim-denominator.test.ts`:
- Around line 68-80: The low-yield test helper only exposes the low-yield-rules
detail, so it cannot verify that context-load-pressure remains counted. Update
lowYieldDetail to expose both relevant Stage D check details, including
context-load-pressure, and extend the associated tests to assert the shim’s
lines remain counted while preserving the existing low-yield assertion.
- Around line 69-74: Update the Bun.spawn flow in the audit test to consume
proc.stderr concurrently with proc.stdout before awaiting proc.exited. Preserve
collecting stdout for parsing while ensuring stderr is fully drained so verbose
--json progress cannot block lowYieldDetail.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e0b74395-2440-4c7a-8b25-454ff9eb02e2
📒 Files selected for processing (14)
README.mddocs-site/public/llms-full.txtdocs-site/src/content/docs/reference/cli.mddocs/byok-trust-model.mddocs/first-user-proof-packet.mddocs/first-user-proof.mddocs/getting-started.mddocs/proofs/current-outside-tester-send-packet.mddocs/rubric.mdpackage.jsonscripts/audit.tsscripts/lib/document-role.tsscripts/lib/rule-loading.tsscripts/shim-denominator.test.ts
Two CodeRabbit findings on #41, both valid. The Low-Yield assertions alone would stay green if a later change also dropped pointers from summarizeLoad — which would violate the contract this fix rests on, since a shim is additive for context load. Now asserted directly: the same fixture with and without the shim differs by the shim's own lines (15 vs 10). Both fixture helpers also piped stderr without draining it. `--json` writes one progress line per discovered rule file there, so a full pipe buffer would deadlock the child mid-read. Latent at fixture size, real at repo size. Fixed in rule-load-fixture.test.ts too, which shipped with the same shape in alpha.10. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jaruesink
added a commit
that referenced
this pull request
Aug 9, 2026
…gnal main went red after #41 merged, and alpha.12 did not publish. The cause is not in #41's change: after normalization the single surviving difference was one added row in the PR-mining table. first differing line 346: checked-in="| Documentation | <normalized> comments | ..." fresh="| Error Handling | <normalized> comments | ..." Anvil mines its own PR history. A code review on #41 pushed a fifth theme over its threshold, the rows shifted, and Documentation was compared against a freshly inserted Error Handling. The same commit's branch build was green minutes earlier — the two runs disagreed purely on live GitHub state. normalizeVolatileReportFields already normalized the counts INSIDE each row, on the reasoning that PR churn should not force a hand refresh. It did not normalize WHICH rows exist, so the very next review reintroduced the chore it was written to end. Rows now collapse to a single placeholder. The line is drawn deliberately: rows changing is churn and normalizes away; the table vanishing entirely means mining itself broke and still fails loudly. A third test asserts a real scoring change is still caught through the mined table, so normalizing live data has not blinded the proof to the deterministic surface it exists to protect. Verified against the actual divergence rather than by refreshing the packet — every other differing line (dates, artifact paths, PR/comment/candidate counts) already normalized cleanly, and no Stage D, Low-Yield, or scoring line moved. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jaruesink
added a commit
that referenced
this pull request
Aug 9, 2026
Validation Date Coverage asked every canonical governance file for a `Last validated:` line. Two document kinds were never governance documents: - `docs/patterns/**` — a reference catalog read on demand. lc-classic-starter sat CRITICAL at 22% coverage, and 7 of its 9 governance files were catalog entries. Stamping ~20 of them changes no agent behaviour. - pointer documents — already named by isPointerDocument, which this check did not consult. Both leave the denominator rather than being counted and forgiven, and the exemption is stated in the check detail so a coverage figure that rose stays auditable: "100% ... (excludes 7 reference/pointer docs)". Scoped to this one coverage figure — both kinds remain on the scoring surface for every other check, exactly as the shim exclusion in #41 was scoped to Low-Yield. Two mistakes of mine are worth recording, because both were caught by the harness rather than by me: I also exempted `docs/bootstrap-templates/**`, which the brief never asked for. Anvil's own governance surface is 18 bootstrap templates, all dated, so the exemption emptied its denominator and an empty denominator read as 0% — turning Anvil's own verdict from PASS to CRITICAL. verify:self-audit-proof caught it, one turn after I loosened that proof's PR-churn normalization, which is decent evidence the loosening did not blind it. Templates are now treated as the governance documents they are. The empty-denominator case is guarded separately: when every governance file is exempt there is nothing left to measure, so the check falls back to the full set rather than inventing a 0%. Fleet sweep across 17 repos, before -> after: exactly one verdict moves, lc-classic-starter CRITICAL -> NEEDS WORK. Nothing else shifts. forge stays CRITICAL correctly — after exempting its 3 catalog entries, its two remaining `.devagent` files are ordinary instruction docs that genuinely carry no date. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jaruesink
added a commit
that referenced
this pull request
Aug 10, 2026
Answering the ask: exempt from rule-quality scoring too, but scoped per check rather than removed from the scoring surface — and the signal is stated, not dropped. postiz-app: CRITICAL -> NEEDS WORK. openclaw stays NEEDS WORK. arbor and every repo without an upstream remote are untouched, because the check never fires there. Two parts. Low-Yield now exempts upstream content alongside pointers. A finding about the quality of a vendor's documentation is not actionable: acting on it means rewriting someone else's docs to satisfy our audit. Same justification as the date exemption, so the same treatment. Scoped to Low-Yield DELIBERATELY, not to the whole scoring surface. Upstream's AGENTS.md really is loaded into our sessions, so it still counts toward Context Load Pressure, redundancy and conflict — postiz still reports 187 always-on lines. Dropping it from everything would understate load we actually pay, which is the mistake the pointer exclusion in #41 nearly made. The exclusion is named in the detail rather than silent — "0/0 scoring files miss Why or Examples (excludes 2 pointer/upstream docs)" — so a reader still learns that upstream ships rules we judged low-yield, without a blocking finding we have no standing to act on. That is the informational-but-unscored option, reached by reusing the mechanism already there. The rename is also fixed, and n=1 does not argue against it the way it argued against the basename discriminator. That check was a PROXY for renames and could collide; this one is content identity. Git's blob hash IS the content, so a file whose blob appears anywhere in the upstream tree contains zero bytes of ours no matter what it is called, and one edited character diverges the hash and drops the exemption. The bound comes from construction rather than sample size. Verified on the real case: HEAD:AGENTS.md and upstream/main:CLAUDE.md are both blob 2704017990140945f94d2ab68ef641f2a0fdb8a8. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
robinhood-traderreportedLow-Yield ⚠️ 1/2. The failing file is its 10-lineCLAUDE.md, an@AGENTS.mdshim — a redirect to the canonical document, carrying no rules of its own. Demanding rationale and examples of a redirect asks the maintainer to pad it.Pointers now leave the Low-Yield denominator rather than being counted and forgiven. 1/2 → 0/1.
The constraint that shaped the fix
Scoped to Low-Yield deliberately: the same file still counts toward Context Load Pressure, which held at 343 lines across the change. A shim is additive there — Claude loads the shim and the file it imports. Filtering pointers from the whole scoring surface was my first instinct and would have silently undercounted load by 11 lines.
That is also the answer to the design question.
On
documentRoleconsolidation: no, not nowThe four predicates are consulted at four different stages, and the same role earns opposite treatment across checks:
isAgentWorkspaceFileisSymlinkAliassummarizeLoadA shared enum would unify the classification while leaving four distinct policies behind — you'd trade four predicates for one enum plus four switch statements. And
isSymlinkAliashas exactly one caller while audit reaches the same outcome via fingerprint dedup, so folding them would change audit behaviour nobody asked to change.What the code did agree with is smaller and real:
importsItsMirrorSource(audit.ts) andimportsRootMirror(rule-loading.ts) were two implementations of the same regex, one re-reading the file from disk on every call. I introduced the second in #39 without collapsing the first. Both now delegate toisPointerDocumentinscripts/lib/document-role.ts— the seam a futuredocumentRolegrows from, if a fifth case justifies it.Fixtures
Corpus census (138 files, same instrument as alpha.11): exactly one pointer document exists fleet-wide. The per-file
hasWhypass rate is unchanged at 61/138 (44%) — expected, since this alters set membership rather than per-file classification, and its flatness is the evidence nothing was loosened.Regression test
Pairs an
@AGENTS.mdshim with a non-pointer file of the same size that has neither Why nor examples: the first leaves the denominator (0/1), the second stays and fails (1/2). The signal is the import, not the length.Verify
lint · format:check · release:pin --check · typecheck · 497 pass / 0 fail · verify:self-audit-proof — all green.
🤖 Generated with Claude Code