docs: post-arc sweep + fix the prod-drift false positive that filed #176

.github/workflows/prod-drift.yml

@@ -64,11 +64,26 @@ jobs:
             echo "state=in_sync" >> "$GITHUB_OUTPUT"
             echo "In sync: deployed=$DEPLOYED main=$MAIN"
           elif git rev-parse -q --verify "$DEPLOYED^{commit}" >/dev/null 2>&1 \
-            && [ -z "$(git diff --name-only "$DEPLOYED"..origin/main -- . | grep -vE '^(docs/|REBUILD/|[^/]+\.md$)')" ]; then
-            # Docs-only drift (#140 false-positive class): every file between the
-            # deployed build and main is markdown/docs — GitHub serves those from
-            # the repo directly, so nothing merged is missing from production.
-            # Treated as in-sync so the issue closes instead of nagging.
+            && [ -z "$(git diff --name-only "$DEPLOYED"..origin/main -- . | grep -vE '^(docs/|REBUILD/|\.github/|packages/core/generation-uplift/|[^/]+\.md$|\.env\.example$)')" ]; then
+            # Not-app-served drift (#140 false-positive class): every file between
+            # the deployed build and main is something production never runs, so
+            # nothing merged is missing from prod. Treated as in-sync so the issue
+            # closes instead of nagging. The excluded set, and why each is safe:
+            #   docs/, REBUILD/, root *.md  — GitHub serves these from the repo.
+            #   .github/                    — CI config; never enters an image.
+            #   packages/core/generation-uplift/ — operator-run benchmark
+            #       artifacts, deliberately outside src/ so neither the package
+            #       tsconfig nor vitest picks them up (GUIDELINES §4). Never built.
+            #   .env.example                — a template. The container reads the
+            #       real .env; a genuinely new runtime var also touches
+            #       deploy/docker-compose.yml, which is NOT excluded and so still
+            #       trips drift.
+            # Added 2026-07-31 after #176: v2.4.0 was a docs + benchmark release
+            # that correctly wasn't deployed, but .env.example and the
+            # generation-uplift/ artifacts survived the old filter and opened a
+            # false drift issue. Verified both ways — 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.
             echo "state=in_sync" >> "$GITHUB_OUTPUT"
             echo "Docs-only drift (deployed=$DEPLOYED main=$MAIN) — treated as in sync."
           else

README.md

@@ -210,6 +210,7 @@ Full walkthrough with examples: **[docs/HOW_IT_WORKS.md](./docs/HOW_IT_WORKS.md)
 | Embeddings | Provider-agnostic via `EMBEDDING_BASE_URL` (Gemini / OpenAI / Qwen3 — any OpenAI-compatible endpoint) |
 | LLM | Claude / GLM / OpenAI / Gemini (swap via env) |
 | Background jobs | pg-boss (no Redis required) |
+| Rate-limit state | In-process by default; Redis when `REDIS_URL` is set (needed for multi-replica, and it survives restarts) |
 | Protocol | Model Context Protocol (`@modelcontextprotocol/sdk`) |
 | Packaging | Turborepo + pnpm workspaces · Docker Compose |
 
@@ -248,7 +249,7 @@ REBUILD/       Phase-by-phase vibe-coding reconstruction guide (start: REBUILD/0
 | [USING_BRAIN](./docs/USING_BRAIN.md) | Daily workflow, trigger phrases, recipes |
 | [KNOWLEDGE](./docs/KNOWLEDGE.md) | The knowledge model (normative) |
 | [protocols/](./docs/protocols/meeting-miner.md) | Agent protocols (V2.0): meeting-miner · doc-harvest · doc-draft · report-draft |
-| [VALIDATION](./docs/VALIDATION.md) | **Does it measurably help?** — retrieval NDCG@5 0.45 vs 0.30 cosine baseline (2026-07-06); first generation-uplift read (2026-07-23): +33.3pp test pass-rate, n=6, small and honestly caveated |
+| [VALIDATION](./docs/VALIDATION.md) | **Does it measurably help?** — retrieval NDCG@5 0.45 vs 0.30 cosine baseline (2026-07-06); two generation-uplift reads (+33.3pp n=6, then +40pp n=5 with live retrieval), both small and honestly caveated. The more useful finding than either number: injected knowledge changes the output where a convention is *locally arbitrary* (a workspace subpath, a build-pipeline quirk) and makes no difference where it coincides with general good practice |
 | [SECURITY](./docs/SECURITY.md) | Auth modes, MCP gating, threat model |
 | [DEPLOY_CHECKLIST](./docs/DEPLOY_CHECKLIST.md) | Production deploy on a public VM |
 | [CICD](./docs/CICD.md) | CI checks + the two deploy scripts, for forkers |

docs/APPROACH.md

@@ -1602,3 +1602,108 @@ documented as a deviation in both `README.md` and `RESULTS.md`, with the
 original spec files committed so anyone with a working toolchain can re-grade
 the same outputs. A benchmark's credibility lives in its disclosed deltas from
 its own protocol.
+
+---
+
+## 5bf. Framing is load-bearing: three defects that hid behind their own descriptions (2026-07-28 → 2026-07-31, v2.3.1 → v2.5.1)
+
+Three separate defects shipped in this arc. None of them was hard to fix. All
+three were hard to *see*, and for the same reason: each was already written
+down, in language that made it sound smaller than it was. The through-line is
+that **a defect's recorded description is itself a piece of code that can be
+wrong**, and a wrong one is worse than no entry at all — it converts an open
+question into a settled one.
+
+**1. "A soft cap on LLM cost" was an application-level auth-limit bypass.**
+`KNOWN_ISSUES §0o`
+carried `rateLimitCheck`'s non-atomic get-then-set as *deferred*, framed as a
+cost-cap nicety. Both halves were wrong. The bucket advanced by **one per burst
+regardless of burst size** — every concurrent caller read the same pre-increment
+count — so a caller who simply kept requests in flight was never limited,
+repeatably. And the helper guards the **auth surface**: voucher redemption (the
+invite-code gate on a self-service Brain), register, forgot-password. Measured
+against a real Redis in a throwaway container, 50 concurrent clients moved the
+old counter to **2**, not 50.
+
+*Scoped honestly:* this was a bypass of the **application** limiter, not of every
+control. `deploy/Caddyfile` rate-limits `/api/*` at the edge to 10 events per IP
+per second, ordered before `reverse_proxy`, so an attacker was never wholly
+unbounded. But the edge limit is three-plus orders of magnitude looser than the
+control it was masking — 10/second against a voucher gate intended to allow
+10/**hour** — and being per-IP it does nothing against a distributed caller. So
+the finding stands; "unbounded" did not, and the distinction is exactly the kind
+this section is about. (Caught in review of this very write-up.)
+
+The mis-framing wasn't carelessness — it was
+written by someone looking at the one endpoint their PR touched, where "cost cap"
+is a fair description. Hence the rule that came out of it: *enumerate the call
+sites before you write the deferral rationale.*
+
+**2. The Brain could not see half of its own corpus, and the metric said zero.**
+Building a second generation-uplift suite whose treatment arm draws from the
+**live** retrieval path — rather than a hand-written block — turned up a null on
+the first probe. Chasing it: `scope: "user"` rows (the `brain_teach_knowledge`
+default) carry the `ownerProjectId` of whatever session wrote them, and
+the production filter (`buildRawProjectFilterV2`) resolves visibility from
+`Knowledge.visibility` plus `ownerProjectId` and never consults `scope`, so those
+rows matched no branch outside their writing project. **117 rows repo-wide** were
+`scope='user'` with a non-null `ownerProjectId` — the affected set. Separately, and
+not a partition of that 117: the corpus split roughly evenly across a catch-all
+`Default` project (101 active rows) and the real one (100), which is what made the
+starvation so large in practice. Fixing it moved a `Brain Platform` session's reach
+from 104 to 141 visible rows. The
+best-matching item in the entire corpus — 0.9009 similarity, exact trigger match,
+eleven successful uses — ranked **first** unscoped and was **absent** from the
+project-scoped session path. Meanwhile the duplicate-project detector reported
+zero, because it looks for *normalized name collisions* and `Brain Platform` vs
+`Default` will never collide. **An instrument that can only see one failure shape
+reports health during a different one.**
+
+**3. A watchdog and a standing rule that guaranteed each other's failure.** The
+`prod-drift` workflow files an issue when the deployed tag differs from `main`'s.
+The standing rule says don't redeploy for changes that touch nothing app-served.
+Following the second reliably trips the first. The workflow *did* carry a
+docs-only carve-out, but `.env.example` and the `generation-uplift/` benchmark
+artifacts survived its filter — so the carve-out existed and didn't cover the
+case it was written for.
+
+### What generalises
+
+**Read the whole helper, including the branches your case doesn't hit.** #174
+looked like an open design question, and it was reported as one. It wasn't: the
+no-active-project branch of `scope-filter.ts` already carried the exact fix,
+added 2026-05-12 after "5/5 retrieval misses traced to this branch." The
+active-project branches never got it. The project had decided; the decision had
+been applied to one branch out of three. Escalating a settled question costs the
+operator's attention and risks re-litigating reasoning that was already done.
+
+**Green tests do not detect a boundary no test asserts.** The first version of
+the #174 fix widened the shared filter for every caller, and the suite stayed
+green. The caller audit — not the tests — found that `action-items.ts` treats the
+project edge as the isolation line for tasks, and `meeting-extract.ts`'s
+supersession search is deliberately project-wide but *not* owner-scoped. Both
+boundaries lived only in prose comments. The fix became opt-in per call site,
+which is what `GUIDELINES §7` already asked for: give cross-scope behaviour an
+explicit path rather than quietly changing a shared function.
+
+**Widening recall is not free, and the pass rate won't tell you.** After the fix,
+the formerly-invisible item appeared in **all five** injected blocks regardless of
+topic — real dilution. It cost nothing measurable, because each task's own rule
+still ranked first. Both facts are invisible in the aggregate score; you only see
+them by diffing what actually got injected.
+
+**Publish the correction as loudly as the claim.** The first write-up of #174
+named the wrong mechanism — it cited the V1 filter and asserted that
+`Knowledge.visibility` does *not* govern retrieval, when production uses the V2
+helper and visibility is exactly what governs it. The symptom was real; the cause
+was not. Corrected in place, in the issue and in `KNOWN_ISSUES §0p`, and flagged
+as a correction rather than quietly rewritten — a repo whose differentiator is
+honest self-reporting cannot make stale *pessimism* an exception to that.
+
+**And the benchmark result worth keeping** is not the headline percentage
+(+33.3pp then +40pp, both small-n) but the shape underneath it, which two
+independent suites now agree on: injected knowledge changes the output where the
+convention is **locally arbitrary** — a workspace subpath, a build-pipeline
+quirk — and ties wherever it coincides with general good practice a strong model
+already applies. That is a directly actionable capture strategy, and a much more
+defensible claim than the number.

docs/DEPLOY_CHECKLIST.md

@@ -154,7 +154,7 @@ comm -23 \
 | `BRAIN_PUBLIC_HOSTNAME` / `BRAIN_MCP_PUBLIC_HOSTNAME` | Server deploy via `scripts/deploy.sh` (Caddy + DNS + TLS, `--profile edge`) | Bare local `docker compose up` (no TLS) |
 | `CADDY_EMAIL` | Caddy is in front of the stack (ACME HTTP-01 registration) | No reverse proxy; direct port exposure |
 | `AUTH_TRUST_HOST` | **Any** reverse proxy in front of the webapp (NextAuth needs to trust the forwarded host header) | Direct exposure of port 3000 |
-| `REDIS_URL` | Multi-replica deploy OR you want per-cluster rate-limit instead of per-replica | Single replica — in-memory rate limit is correct |
+| `REDIS_URL` | **Any server deploy.** Compose only passes it through (`REDIS_URL: ${REDIS_URL:-}`) — it does not supply it, so an unset value means the `redis` service runs with nothing connected. Required for multi-replica; on a single replica the in-memory limiter is *correct* but its state **resets on every deploy or `reload.sh web`**, silently clearing daily caps like `RATE_LIMIT_MEETING_EXTRACT_PER_DAY`. Confirm it took: `docker compose ... logs web \| grep "redis ready"` | Local dev only |
 | `SENTRY_DSN` + `SENTRY_TRACES_SAMPLE_RATE` | Shipping errors to Sentry | Local-only operation |
 | `SKIP_SEED` | Pilot deploy — you don't want the Alex demo persona in production data | First-time dev setup where the seed is the point |
 | `AUTH_GITHUB_ID` / `AUTH_GITHUB_SECRET` / `AUTH_SECRET` | OAuth mode (pilot / public) | Local dev with `ALLOW_DEV_AUTH=true` |

docs/GUIDELINES.md

@@ -40,7 +40,7 @@ Every write path in `packages/core` or `packages/db` must uphold the invariants
 
 1. Knowledge immutability — no in-place edit of persisted rows.
 2. Provenance — `sourceSessionIds` or `extractedBy` non-empty.
-3. Scope boundary — queries always filter by owner.
+3. Scope boundary — queries always filter by owner. **Project reach and user reach are separate axes: widening the first must never widen the second.** `scope-filter.ts`'s cross-project disjunct carries its own `ownerUserId` predicate rather than leaning on an outer `AND`, because it sits inside an `OR` where an outer clause would not constrain the sibling arms (#174). Any new branch that broadens visibility repeats that predicate, and is opt-in per call site — `action-items.ts` and `meeting-extract.ts` both depend on the project edge holding.
 4. Embedding required — nightly backfill for stragglers.
 5. Confidence/decay ranges — clamp, never trust caller.
 6. Anti-principles need evidence.
@@ -415,7 +415,13 @@ PATCH/POST endpoints return the updated view (`{ item: toKnowledgeItemView(row)
 Long-running endpoints that the user is waiting on (Oracle answers, future KEA spot-checks) stream Server-Sent Events. Contract: one route per streamed capability, Content-Type `text/event-stream`, `event: <kind>` + `data: <json>` pairs, a terminal `event: done`. Frontend consumers parse with a minimal `parseSSE(body: ReadableStream)` generator — do not pull in `EventSource` since we POST. See `apps/web/app/api/oracle/stream/route.ts` and `apps/web/lib/brain/use-oracle.ts` for the canonical shape. All new stream producers must forward `req.signal` into the underlying LLM SDK (cost/correctness).
 
 **Rate limiting (`apps/web/proxy.ts`)**
-Next 16 renamed `middleware.ts` → `proxy.ts`. One `proxy` runs for every `/api/*` request and enforces sliding windows keyed by client IP. Adding a new endpoint class: (a) add a `classify()` branch with a distinct name; (b) either reuse an existing `RATE_LIMIT_*` env or add one to `.env.example`; (c) verify the `x-ratelimit-*` headers in a local curl before merging. Store selection is automatic: Redis when `REDIS_URL` is set, in-memory otherwise — both implement the async `Store` interface in `packages/core/src/rate-limit.ts`. Keep `check()` signature stable across stores.
+Next 16 renamed `middleware.ts` → `proxy.ts`. One `proxy` runs for every `/api/*` request and enforces **fixed** windows keyed by client IP — `check()` opens a fresh bucket once `resetAt` passes, it does not slide. That matters for reasoning about burst behaviour: a caller can spend a full allowance at the very end of one window and another immediately at the start of the next, so the true worst case across a boundary is ~2× `max`. Size auth limits with that in mind. Adding a new endpoint class: (a) add a `classify()` branch with a distinct name; (b) either reuse an existing `RATE_LIMIT_*` env or add one to `.env.example`; (c) verify the `x-ratelimit-*` headers in a local curl before merging. Store selection is automatic: Redis when `REDIS_URL` is set, in-memory otherwise — both implement the async `Store` interface in `packages/core/src/rate-limit.ts`.
+
+**The `Store` contract is one atomic `increment(key, windowMs, now)` — do not reintroduce a get-then-set pair.** It used to be `{get, set}`, and that could not be composed safely: concurrent callers all read the same pre-increment count, so a burst advanced the bucket by **one regardless of its size** and a caller who kept requests in flight was never limited at all. Measured against a real Redis, 50 concurrent clients moved the old counter to **2**. That is an unbounded bypass, and the limiter guards the **auth surface** — voucher redemption (the invite-code gate), register, forgot-password — not just the LLM-cost caps. In-memory does its read-modify-write with no `await` between read and write and copies the bucket out; Redis runs `INCR` + conditional `PEXPIRE` + `PTTL` as one Lua script. A new store must be atomic by construction, and Redis errors degrade to the per-process limiter rather than letting a request through uncounted.
+
+**Know what that degradation costs.** Falling back to per-process state is the right call over returning 500s, but on a multi-replica deployment it means each replica counts independently — a caller spreading requests across N replicas gets roughly N× the intended allowance for as long as Redis is unreachable. On the single-replica reference instance the fallback is exact; anyone running multiple replicas should alarm on the `"redis error — falling back to in-memory rate limiter"` log line (emitted at most once a minute) and treat a sustained outage as a reason to shed auth traffic, not just to keep serving.
+
+**Set `REDIS_URL` on any server deploy.** Compose only passes the value through (`REDIS_URL: ${REDIS_URL:-}`); it does not supply it. Without it the limiter is correct on a single replica but its state resets on every deploy or `reload.sh web`, silently clearing daily caps such as `RATE_LIMIT_MEETING_EXTRACT_PER_DAY`. Confirm it took by grepping the web logs for `"redis ready"`.
 
 **Production auth enforcement**
 `apps/web/lib/brain/auth.ts` calls `refuseDevShimInProduction()` before falling through to the dev shim. When `NODE_ENV=production`, the shim throws `AuthError(500)` unless `ALLOW_DEV_AUTH_IN_PRODUCTION=true` is set explicitly (intended for VPN-only deploys). Never relax this — it's the final guard against shipping a deploy that serves every visitor as the first User row.