docs: post-v2.6.1 sweep + a finding from reviewing what the bot didn't read - #186
Merged
Conversation
…idn't Independent review of this arc's PRs, prompted by the repo's own rule that a green CodeRabbit check with zero inline comments means a rate-limited bot, not a clean review. Four of six PRs were in that state. Reading them by hand found one real issue. NEW OPEN FINDING — the V1 scope helpers never got #174's cross-project reach, and three surfaces still call them. buildKnowledgeWhere (V1) is live at 7 call sites: dashboard, graph, and exporter.buildRulesBundle. It resolves visibility on ownerProjectId alone and never consults `scope`, so a scope:'user' rule taught in another project is invisible to it — while retrieval (V2 + opt-in) sees it. Concrete symptom: buildRulesBundle defaults to dataScope "project", so "Download rules bundle" omits rules the Brain is actively injecting into sessions. The exported set disagrees with the served set. Nothing in CI would ever say so, because nothing in CI knows the two should agree. Left OPEN deliberately: it changes what three user-facing surfaces show, which is a product call rather than a correctness one, and it is the unfinished half of a deliberate Phase-4 migration. Options recorded in the row. Also documented: - KNOWN_ISSUES: the coherence gate now exists (v2.6.0), plus the v2.6.1 follow-up where the gate's own script tripped prod-drift. - GUIDELINES: there are TWO generations of scope helper and the migration is unfinished — check which one a surface uses before reasoning about its visibility. Plus a rule for writing CI gates: gate on values not files, and let the NEGATIVE test drive the design, because a gate that cries wolf gets switched off. - KNOWLEDGE: names the three narrower surfaces explicitly next to the retrieval-vs-listing note. - APPROACH: the cries-wolf design lesson; exclusion lists as a liability that accrues with every new top-level path; and why independent review is not optional when the bot didn't read the diff. README verified current — no edits needed. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rqj5y9fT3XPUqnqKyustp9
|
Warning Review limit reached
Next review available in: 53 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: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe pull request adds documentation for V1/V2 scope-helper limitations, benchmark coherence gates, deployment exclusion validation, and independent review when automated checks produce no substantive findings. ChangesDocumentation guidance
Estimated code review effort: 1 (Trivial) | ~5 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 3
🤖 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 `@docs/APPROACH.md`:
- Line 1760: Update the text at the Markdown heading line beginning with “#174”
to “issue `#174`”, ensuring the hash is no longer parsed as an invalid heading and
the existing sentence remains intact.
- Around line 1762-1765: Update the benchmark-gate guidance in the surrounding
documentation to require tests for both outcomes: a KRA value change must fail
when VALIDATION.md is absent, while refactor-only changes must pass. Keep the
existing historical negative-case guidance and explicitly document these as
complementary regression scenarios.
In `@docs/GUIDELINES.md`:
- Around line 45-46: Correct the V1 scope terminology across all specified
documentation sites: in docs/GUIDELINES.md lines 45-46, limit “no cross-project
reach” to project-scoped V1 calls and distinguish persisted scope from request
dataScope; in docs/KNOWLEDGE.md lines 528-533, remove the claim of no
cross-project reach at all and direct new Knowledge listing paths to V2 helpers;
in docs/KNOWN_ISSUES.md line 384, describe the persisted scope-column limitation
instead of saying V1 never consults scope, and label the divergence as a
deferred decision with a known consistency gap.
🪄 Autofix (Beta)
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: CHILL
Plan: Pro Plus
Run ID: 4ccac113-8b8f-426b-8183-67154ad88432
📒 Files selected for processing (4)
docs/APPROACH.mddocs/GUIDELINES.mddocs/KNOWLEDGE.mddocs/KNOWN_ISSUES.md
| first.** The benchmark-doc coherence gate exists to stop a `kra.ts` retune | ||
| landing with stale published numbers. The obvious implementation — fail if | ||
| `kra.ts` changed without `VALIDATION.md` — would have blocked this very arc's | ||
| #174 work, which edited that file without moving a single weight. It compares |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the Markdown heading syntax.
Line 1760 starts with #174 without a space. Replace it with issue #174`` to satisfy markdownlint rule MD018.
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 1760-1760: No space after hash on atx style heading
(MD018, no-missing-space-atx)
🤖 Prompt for 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.
In `@docs/APPROACH.md` at line 1760, Update the text at the Markdown heading line
beginning with “#174” to “issue `#174`”, ensuring the hash is no longer parsed as
an invalid heading and the existing sentence remains intact.
Source: Linters/SAST tools
Three findings on #186, all valid. 1. My V1 claim was overstated — the fourth time this session. I wrote that V1 has "no cross-project reach at all". It does: under DataScope "all" buildKnowledgeWhere returns everything the caller owns, across projects. The real gap is narrower and more precise: under DataScope "project" it consults only ownerProjectId and never Knowledge.scope, so it cannot filter to a project WHILE admitting the caller's user-scoped rows from elsewhere — which is exactly what V2's opt-in adds. Review also caught that I was conflating two different things both called "scope": persisted Knowledge.scope (user/project/global) and request DataScope (project/all). Both now named explicitly wherever this is described. The symptom is correspondingly narrower: buildRulesBundle defaults to DataScope "project" and the route only sends "all" for ?scope=all, so the DEFAULT download omits those rules while ?scope=all returns them. That is a confusing inconsistency rather than an absence, which is a fairer description. 2. Documented BOTH benchmark-gate outcomes. The text emphasised the negative test; the positive one exists too (simulated 0.7 -> 0.65 retune must fail without VALIDATION.md) and is now stated. 3. MD018: "#174" at line start parsed as a markdown heading. Also records the pattern behind all four overclaims of this session — unbounded / zero coverage / wrong mechanism / "at all" — since naming it as a habit is more useful than four separate corrections. The tell is consistent: the absolute arrives before the verification, because it is the version that makes a finding sound worth reporting. Review caught all four, which argues for review rather than for trying harder. Corrected in KNOWN_ISSUES, GUIDELINES and KNOWLEDGE together — a retraction in one of three is not a retraction. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rqj5y9fT3XPUqnqKyustp9
6 tasks
bejranonda
added a commit
that referenced
this pull request
Aug 1, 2026
…187) Closes the open finding from #186: "Download rules bundle" shipped a strictly narrower set than kra.ts injects. Two things are called "scope" and the distinction is the fix. Under DataScope "all" the V1 helper already returns everything the caller owns. The gap was DataScope "project" — buildRulesBundle's default — where it resolved on ownerProjectId alone and never consulted Knowledge.scope, so it could not filter to a project WHILE admitting the caller's user-scoped rows from elsewhere. Fixed with an owner-anchored includeUserScopeAcrossProjects opt-in on V1, always enabled by the exporter. Not exposed as a parameter: there is no reading of "export my rules" that wants less than you are served. Deliberately NOT the V1->V2 migration I originally suggested. Checking before executing showed V2's `visibility: 'project'` arm carries no ownerUserId predicate — that absence is what makes Phase-4 org sharing work — so migrating a personal surface to it would ALSO start returning teammates' rules. That is a product decision about team sharing, not this defect, and it would have shipped inside a change labelled "finish the migration". Dashboard and graph keep the project-scoped view on purpose. They are browsing surfaces where project focus is the point, consistent with KNOWLEDGE §12.19's retrieval-is-wider-than-listing rule. A rules bundle is different in kind — it is the agent's configuration. Tests pin all four properties: closed by default, opened when opted in, no-op under DataScope "all", and the ownerUserId anchor present both on the outer AND and inside the new disjunct so it is safe read alone. GUIDELINES now records the general rule this produced: migrate a surface to V2 when you want org visibility there; add an owner-anchored opt-in to V1 when you only want the caller's own rows to reach further. Claude-Session: https://claude.ai/code/session_01Rqj5y9fT3XPUqnqKyustp9 Co-authored-by: Claude <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.
Independent review of this arc's PRs, prompted by the repo's own rule: a green CodeRabbit check with zero inline comments means a rate-limited bot, not a clean bill of health.
Reading the four unreviewed ones by hand found one real issue.
New open finding: the exported rules ≠ the served rules
buildKnowledgeWhere(V1) is still live at 7 call sites — the dashboard, the graph, andexporter.buildRulesBundle. It resolves visibility onownerProjectIdalone and never consultsscope, so ascope: 'user'rule taught from inside another project is invisible to it. Retrieval (kra.ts/oracle.ts, V2 + opt-in) does see those rows.Concrete symptom:
buildRulesBundledefaults todataScope: "project", so "Download rules bundle" omits rules the Brain is actively injecting into your sessions.Nothing in CI would ever have flagged this, because nothing in CI knows the two sets should agree.
Left open deliberately. It changes what three user-facing surfaces show — a product call, not a correctness one — and it's the unfinished half of a deliberate Phase-4 migration (
scope-filter.ts: V2 "replacesbuildRawProjectFilterfor callers that have been migrated"). Options recorded in the row: migrate the three call sites to V2 with the opt-in, or add the same opt-in to V1.Also documented
prod-drift.README verified current — no edits made.
Test plan
scope-filter.ts: confirmed all four V2 branches are mutually consistent and pinned by the cross-helper tests.buildRulesBundledefault scope) rather than reporting it as an abstract inconsistency.check-doc-refs.shagainst the live ceiling (fix(ci): exclude root scripts/ from prod-drift's app-served set #185).🤖 Generated with Claude Code
https://claude.ai/code/session_01Rqj5y9fT3XPUqnqKyustp9
Summary by CodeRabbit