docs: post-arc sweep + fix the prod-drift false positive that filed #176 - #179
Merged
Conversation
…tive Sweeps the doc set after the v2.3.1 -> v2.5.1 arc and fixes the watchdog that produced the only open issue. fix(ci): complete prod-drift's not-app-served exclusion set. The watchdog opened #176 for v2.4.0, a docs + benchmark release that was CORRECTLY not deployed per the standing "don't redeploy changes that touch nothing app-served" rule. It already had a docs-only carve-out; two paths survived it — .env.example (a template; the container reads the real .env) and packages/core/generation-uplift/** (operator-run artifacts kept outside src/ precisely so nothing builds them). So the repo's own rule and its watchdog were guaranteed to conflict. Added those plus .github/, and verified BOTH directions: v2.3.1..v2.4.0 now filters to empty, while v2.4.0..v2.5.0 (a real kra/oracle change) still reports drift — the detector is not blinded. docs updated for what actually shipped: - KNOWLEDGE: the scope table said `user` means "this user's work"; that described intent, not behaviour, until #174. Now documents real cross-project reach, pinned to ownerUserId, and why §12.19 retrieval is deliberately wider than listing. - GUIDELINES: invariant 3 now states project reach and user reach are separate axes and the cross-project disjunct carries its own ownerUserId predicate (it sits inside an OR, where an outer AND would not constrain siblings). Rate-limit section rewritten for the atomic Store contract — the old get-then-set let 50 concurrent clients move the counter to 2 — plus the REDIS_URL requirement. - KNOWN_ISSUES: Redis row closed (operator set it; "redis ready" verified under load), prod-drift row added, and a new row for the two-checkout hazard — /root/BrainPlatform and /root/ExternalBrain resolve to the SAME compose project `deploy`, so deploying from the wrong one targets live prod with a 4-line stub .env. - DEPLOY_CHECKLIST: REDIS_URL was listed as multi-replica-only with "single replica — in-memory is correct". Correct but misleading: the state resets on every deploy, silently clearing daily caps. - README: stack table gains rate-limit state; VALIDATION row carries both uplift reads and the finding that matters more than either number. - APPROACH §5bf: three defects that hid behind their own descriptions, and what generalises — read the whole helper including branches your case doesn't hit; green tests can't detect a boundary no test asserts; widening recall isn't free and the pass rate won't show it; publish the correction as loudly as the claim. Closes #176. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rqj5y9fT3XPUqnqKyustp9
|
Warning Review limit reached
Next review available in: 51 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 (2)
📝 WalkthroughWalkthroughThe pull request updates production drift detection and documents Redis-backed rate limiting, cross-project knowledge retrieval boundaries, live validation results, and corrected production deployment findings. ChangesProduction guidance
Estimated code review effort: 3 (Moderate) | ~20 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: 5
🧹 Nitpick comments (1)
.github/workflows/prod-drift.yml (1)
67-80: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAlign the benchmark exclusion with the verified non-runtime scope.
The workflow and issue record exclude the whole
packages/core/generation-uplift/subtree, while the PR objective identifiespackages/core/generation-uplift/suite-2/**. Narrow the pattern, or prove that every descendant is outside package, Docker, and runtime inputs before treating the whole tree as safe.
.github/workflows/prod-drift.yml#L67-L80: change the regex or add the repository check before excluding the whole directory.docs/KNOWN_ISSUES.md#L406-L406: document the actual exclusion path and its evidence.🤖 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 @.github/workflows/prod-drift.yml around lines 67 - 80, Narrow the benchmark exclusion in the drift-check condition around the git diff regex to the verified packages/core/generation-uplift/suite-2/** path, unless repository checks prove the entire subtree is outside package, Docker, and runtime inputs. Update docs/KNOWN_ISSUES.md at line 406 to document the same actual exclusion path and supporting evidence.
🤖 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 @.github/workflows/prod-drift.yml:
- Line 67: Normalize the deployed revision before the git diff check in the
workflow, using git rev-parse --verify "$DEPLOYED^{commit}" so annotated tags
resolve to their actual commit. Ensure the resulting commit SHA is used as the
left-hand revision in the existing git diff comparison while preserving the
current file exclusions.
In `@docs/APPROACH.md`:
- Around line 1617-1628: Update the “A soft cap on LLM cost” section in
APPROACH.md to qualify the issue as an application-level rate-limit bypass, not
an unbounded end-to-end bypass. State that deploy/Caddyfile applies a
10-events-per-IP-per-second rate_limit to /api/* before application requests,
while preserving the explanation that the application’s rateLimitCheck affected
authentication endpoints.
- Around line 1630-1642: Update the documented retrieval mechanism to identify
buildRawProjectFilterV2 and describe the defect as the missing user-scope
disjunct, replacing the claim that matching used project ownership alone.
Clarify whether the 101 Default and 100 real-project figures are total project
counts; otherwise replace them with distinct affected-row counts that reconcile
with the stated 117 affected rows.
In `@docs/GUIDELINES.md`:
- Around line 420-422: Update the Redis failure documentation near the Store
contract to state that the per-process fallback allows clients in multi-replica
deployments to obtain approximately one limit per process. Define the required
alerting or fail-closed policy specifically for authentication endpoints,
including voucher redemption, registration, and forgot-password.
- Line 418: Update the rate-limiter description in GUIDELINES.md to call the
window fixed-window rather than sliding-window, while preserving the existing
endpoint classification, environment configuration, store selection, and header
verification guidance.
---
Nitpick comments:
In @.github/workflows/prod-drift.yml:
- Around line 67-80: Narrow the benchmark exclusion in the drift-check condition
around the git diff regex to the verified
packages/core/generation-uplift/suite-2/** path, unless repository checks prove
the entire subtree is outside package, Docker, and runtime inputs. Update
docs/KNOWN_ISSUES.md at line 406 to document the same actual exclusion path and
supporting evidence.
🪄 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: 8f29606d-49bd-408d-aac5-b013a3ed8f6b
📒 Files selected for processing (7)
.github/workflows/prod-drift.ymlREADME.mddocs/APPROACH.mddocs/DEPLOY_CHECKLIST.mddocs/GUIDELINES.mddocs/KNOWLEDGE.mddocs/KNOWN_ISSUES.md
Four of five findings on #179 were right. Applied: - APPROACH: "unbounded auth bypass" overstated it. deploy/Caddyfile rate-limits /api/* at the edge to 10 events/IP/s, ordered before reverse_proxy, so an attacker was never wholly unbounded. Rescoped to an APPLICATION-level bypass — while noting the edge limit is 3+ orders of magnitude looser than the control it masked (10/second against a voucher gate meant to allow 10/hour) and, being per-IP, does nothing against a distributed caller. The finding stands; the word did not. Fitting catch on a section about framing. - APPROACH: "matched on project ownership alone" repeated the vague mechanism this arc already had to correct once. Now states that buildRawProjectFilterV2 resolves visibility from Knowledge.visibility plus ownerProjectId and never consults scope. Also disambiguated the counts, which read as contradictory: 117 is the repo-wide affected set (scope='user' with a non-null ownerProjectId); 101/100 are total active rows per project and NOT a partition of it. - GUIDELINES: the limiter is FIXED-window, not sliding — check() opens a fresh bucket once resetAt passes. The old wording gave wrong burst-boundary guarantees for auth limits; the real worst case across a boundary is ~2x max, which is what you must size against. - GUIDELINES: documented what the Redis-failure fallback costs. Per- process state is right over 500s, but on multi-replica each replica counts independently, so a caller spreading load gets ~Nx the allowance while Redis is down. Added the log line to alarm on. Rejected, with evidence: the annotated-tag concern on prod-drift.yml. Our tags are annotated (git cat-file -t v2.4.0 -> tag), but git diff peels them — the diff computed from tag names is byte-identical to the one from `git rev-parse <tag>^{commit}`. No change needed. 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
Jul 31, 2026
Review pass over the v2.3.0 -> v2.5.2 arc turned up three gaps, all
introduced by that arc.
1. buildKnowledgeWhereV2 and buildRawProjectFilterV2 DISAGREED on the
no-activeProjectId case. The raw helper admits scope IN
('user','global') unconditionally there — added 2026-05-12, on the
reasoning that with no active project there is no boundary to
enforce. The Prisma twin gated it on the new opt-in flag. They back
the same visibility rule on different query surfaces, so a caller's
results depended on which one their code path happened to hit. Same
inconsistency class CodeRabbit caught inside the raw helper on #179
— fixed there, and introduced between the two.
2. buildKnowledgeWhereV2 had ZERO test coverage despite backing the
knowledge listing route and action-items, and #177 added an
unreachable branch to it (no caller passes the flag; kra/oracle use
the raw helper). Six tests now pin: closed by default under an
active project, opened when opted in, applied to scope="all",
unconditional with no active project — asserted against the raw
helper in the same test so they cannot drift apart again — and the
disjunct pinned to ownerUserId.
3. Documented the behaviour change the parity fix causes at a
security-reviewed call site. action-items.ts passes projectId as
string | null; with null it now admits the caller's own user-scoped
rows. Acceptable — accessibleProjectIds stays [], the query ANDs
type: action_item, it is pinned to ownerUserId, and action items are
project-scoped by contract so the set is empty — but it is a real
change at a spot with an explicit isolation note, so it is stated
rather than left to be rediscovered.
Also notes on Limit.max that 0 blocks everything and is not a
"disabled" sentinel. Before the atomic rewrite the fresh-bucket path
returned ok:true unconditionally, so max:0 leaked one request per
window; that inconsistency is gone and nothing depended on it.
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.
Sweeps the doc set after the v2.3.1 → v2.5.1 arc, and fixes the watchdog that produced the only open issue.
fix(ci): complete
prod-drift's not-app-served exclusion setprod-driftopened #176 for v2.4.0 — a docs + benchmark release that was correctly not deployed, per the standing rule to skip redeploys for changes touching nothing app-served.The workflow already had a docs-only carve-out (#140). Two paths survived its filter:
Neither is app-served.
.env.exampleis a template — the container reads the real.env.generation-uplift/holds operator-run benchmark artifacts deliberately kept outsidesrc/so neither the package tsconfig nor vitest picks them up (GUIDELINES §4).So the repo's own rule and its own watchdog were guaranteed to conflict. Completed the exclusion set (also
.github/, which never enters an image) and verified both directions:v2.3.1..v2.4.0v2.4.0..v2.5.0kra.ts/oracle.tschangeThe negative test matters more than the positive one: it's the check that I didn't just silence the alarm.
Docs brought in line with what shipped
user= "this user's work". That described intent, not behaviour, until scope: "user" knowledge is invisible outside its capture project (117 rows, ~half the corpus) #174. Now documents real cross-project reach, pinned toownerUserId, plus why §12.19 retrieval is deliberately wider than listing (a rule taught once should apply everywhere you work; a listing is a browsing surface where project focus is the point).ownerUserIdpredicate because it sits inside anORwhere an outerANDwouldn't constrain siblings. Rate-limit section rewritten for the atomicStorecontract, with the measurement (50 concurrent clients moved the old counter to 2) and theREDIS_URLrequirement./root/BrainPlatformand/root/ExternalBrainresolve to the same compose projectdeploy, so deploying from the wrong one targets live prod with a 4-line stub.env.REDIS_URLwas listed as multi-replica-only, "single replica — in-memory is correct". Correct but misleading: state resets on every deploy, silently clearing daily caps.Test plan
${{ }}interpolation inrun:blocks (the healthz value was already injection-hardened).check-doc-refs.shpasses against the live ceiling (feat(core): suite-2 second reading — uncurated corpus after the #174 fix #178), not a stale hardcoded one.REDIS_URLline in.env, present in the running container,"redis ready"logged, 8 requests through the limiter with no fallback or error.DEPLOY_CHECKLIST) is fixed in this PR.Closes #176.
🤖 Generated with Claude Code
https://claude.ai/code/session_01Rqj5y9fT3XPUqnqKyustp9
Summary by CodeRabbit
Bug Fixes
Documentation