feat/review-rigor

Async cache stampede in getOpenPrNumbers — concurrent cold requests each trigger a GitHub fetch

apps/dev-ops/src/lib/github.ts:22

The module-level `memo` is set only AFTER `fetchOpenPrNumbers()` resolves. Within a single Node.js instance, two concurrent requests that both arrive while `memo` is null will both enter `fetchOpenPrNumbers()` simultaneously (Node.js is single-threaded but `await` yields). Both will issue a GitHub API call before either has written to `memo`. The fix is to cache the in-flight Promise, not just the resolved value. Under normal load this is benign, but it could cause GitHub secondary-rate-limit hits during a cold start burst. Callers: `apps/dev-ops/src/app/page.tsx` (the only caller).

getLatestPerPr fetches all rows including full findings JSONB — will degrade as the table grows

apps/dev-ops/src/lib/code-reviews.ts:59

The list-page query `db.select().from(codeReviews).orderBy(desc(reviewedAt))` has no LIMIT and selects all columns including the `findings` JSONB column (which can be 50–200KB per review). `groupLatestPerPr` then deduplicates in memory. At 50 branches × 30 reviews each, that is 1,500 rows and potentially ~75MB of JSONB per page render. A `DISTINCT ON (slug)` or a `ROW_NUMBER()` window function would push deduplication to Postgres. The list page needs only `id, slug, branch, prNumber, sha, mode, verdict, summary, counts, producer, reviewedAt` — findings are not rendered there. Callers: `apps/dev-ops/src/app/page.tsx`.

vercel.json ignoreCommand loses turbo-ignore semantics on redeploy / force-push

apps/dev-ops/vercel.json:4

`turbo-ignore` compares the current commit against the last *deployed* commit (via Vercel's API). `git diff --quiet HEAD^ HEAD ./` compares only the two most recent commits. These diverge when Vercel's 'Redeploy' button is used on an older build, or when history is rewritten (squash merge, force-push). Also, `turbo-ignore` understands the turbo dependency graph — if `@batu/database` schema changes, turbo-ignore would trigger a rebuild; git-diff would not. The commit message acknowledges this as intentional, but the trade-off should be documented in a comment in `vercel.json`.

Membership property test generates internally inconsistent state (activeOwnersCount=0 with active owner)

domains/core/src/membership/__tests__/membership.decisions.property.test.ts:50

`fc.constantFrom(0, 1)` for `activeOwnersCount` while the membership under test has `role: 'owner', status: 'active'` means `activeOwnersCount=0` is impossible in production (the owner IS an active owner, so the count must be ≥1). The tests still pass because the decision code handles the inconsistent input correctly, but the property 'for all valid states' does not hold: the generator can emit states production code never produces. Use `fc.integer({ min: 1, max: 50 })` when the subject is an active owner to keep generators within the domain's value space (per the testing.md rule).

Token length leaked before constant-time comparison in bearerOk()

apps/dev-ops/src/app/api/code-reviews/route.ts:25

`bearerOk()` returns `false` immediately when `presented.length !== expected.length`, before entering the XOR loop. This reveals the exact byte-length of `DEVOPS_WRITE_TOKEN` to a remote attacker via response timing. A true constant-time check must always iterate over `max(presented.length, expected.length)`. Exploitation requires sub-millisecond HTTP timing resolution which is hard over the public internet, but it is a correctness gap in the stated constant-time guarantee.

GITHUB_REPO env var injected unvalidated into outbound GitHub API URL

apps/dev-ops/src/lib/github.ts:40

The `GITHUB_REPO` value is interpolated directly into the GitHub API request URL with no format validation (expected pattern: `owner/repo`). If the env var is misconfigured (e.g. extra path segments), the fetch URL is malformed but still dispatched. Validate the value matches `/^[a-zA-Z0-9_.-]+\/[a-zA-Z0-9_.-]+$/` before use, or hardcode the repo name and remove the override if it is only needed for staging.

GitHub API response cast without runtime validation

apps/dev-ops/src/lib/github.ts:52

`(await res.json()) as Array<{ number: number }>` is a type assertion with no runtime shape check. A malformed response that slips past the `!res.ok` guard would throw inside the try/catch and correctly fall back to `null`. However, an element with a non-numeric `number` field would call `open.add(NaN)` — which `Set.has()` handles correctly via SameValueZero, but makes the intent fragile. A Zod parse on the array items would make the boundary explicit.

Module-level memo cache may issue multiple concurrent GitHub API calls across warm instances

apps/dev-ops/src/lib/github.ts:16

Not a security vulnerability — the token is server-side only. Under load, multiple warm Vercel instances may each call GitHub simultaneously, consuming more of the token's rate-limit budget than expected. No action needed unless GitHub rate-limit errors become observable.

Module-level mutable state in Next.js server module

apps/dev-ops/src/lib/github.ts:16

`let memo` is module-level mutable state shared across concurrent requests with no lock. In Next.js with `force-dynamic`, this works today within a single warm instance, but it is fragile: it survives hot-reloads in dev (stale data), breaks under any platform that creates fresh module instances per invocation (edge runtime), and is shared across concurrent requests without a mutex. The conventional Next.js pattern is `unstable_cache` or `fetch` with `next: { revalidate: 60 }` for cross-request caching. For an internal dashboard the practical risk is low, but the pattern should not be replicated elsewhere.

critical severity accepted by schema but never rendered in counts display

apps/dev-ops/src/lib/schema.ts:56

`PayloadSchema` accepts `counts.critical` (optional) and `FindingSchema` accepts `severity: 'critical'`, but both `page.tsx` and `[slug]/page.tsx` render only `{counts.high}H · {counts.medium}M · {counts.low}L` — critical findings are silently dropped from the counts line. A reviewer posting critical-severity findings will see them in the FindingsView but not in the summary. Either remove `critical` from the schema surface, or add it to the rendered counts line.

CLAUDE.md documents 'active → decommissioned' as valid but the implementation blocks it

domains/core/CLAUDE.md:64

Pre-existing: `domains/core/CLAUDE.md` §Status Transitions says 'active → inactive, decommissioned' is valid. The implementation in `site.decisions.ts` only allows `active → inactive`; `active → decommissioned` is rejected. The property tests correctly reflect the implementation. The CLAUDE.md documentation should be corrected to: 'active → inactive only; inactive → active, decommissioned'. This predates the branch but is now more visible given the property tests explicitly encode the state machine.

Site property test: 'once decommissioned, NO sequence of transitions can reach active/inactive' is vacuously valid

domains/core/src/site/__tests__/site.decisions.property.test.ts:44

The test loop advances `current = next` only when `isValidStatusTransition(current, next)` returns true. Since decommissioned is terminal, that condition is always false — the loop body never runs, and `current` stays 'decommissioned' trivially. The test would pass even if the implementation were replaced with `function isValidStatusTransition() { return false; }`. To make the multi-step property meaningful it needs to start from a non-decommissioned state, allow arbitrary transitions (including ones that reach decommissioned), then assert that once decommissioned is reached no further transition succeeds.

Version-mismatch property only covers decideUpdateRole — remove/leave have no version guard and the gap is undocumented

domains/core/src/membership/__tests__/membership.decisions.property.test.ts:102

The 'a version mismatch always yields VersionConflict' property only tests `decideUpdateRole` (the only function with an `expectedVersion` param). `decideLeaveMembership` and `decideRemoveMember` have no version check — a caller can remove a member without optimistic locking. The property name implies a general version-guard invariant across all three operations, which is misleading. A comment or a complementary property ('remove/leave are intentionally version-unguarded') would document the gap and prevent a future reader from inferring the wrong invariant.

Membership idempotency property only checks ok=true, not the returned decision value

domains/core/src/membership/__tests__/membership.decisions.property.test.ts:124

The 'removing an already-removed membership is idempotent' property verifies `ok=true` but not the decision's value (the returned membership or `_tag`). A regression that returns `ok: true` with a wrong `_tag` (e.g. `RemoveMember` vs `AlreadyRemoved`) or a wrong membership object would pass this property. Adding `if (res.ok) expect(res.value._tag).toBe('RemoveMember')` closes the gap.

Mixed example-style assertions in property test file

domains/core/src/site/__tests__/site.decisions.property.test.ts:56

The fifth test 'active and inactive are mutually reachable' uses plain `expect()` calls rather than `fc.property()`, making it an example test embedded in a property test file. Per `testing.md` conventions, property test files contain `fc.assert(fc.property(...))` calls; example tests belong in `site.decisions.test.ts`. The test is functionally correct but breaks the file's stated invariant.

delta.test.ts: null file and null line edge cases not covered

apps/dev-ops/src/lib/delta.test.ts

`Finding.file` and `Finding.line` are both nullable. `deltaKey` guards with `?? ''`. The test fixture always supplies concrete values. There is no test for findings with `file=null, line=null, same title` (should produce the same key) nor for a finding with `file=null` vs `file='a.ts'` (should produce different keys). The loop's review panel does emit file-less findings for summary-level issues, so this gap has a real trigger path.

In-process memo cache is unlikely to hit in Vercel serverless

apps/dev-ops/src/lib/github.ts:16

The 60s TTL `memo` only persists within a single warm serverless instance. On Vercel, concurrent or cold requests may land on different instances, so the cache rarely fires in practice. `fetch` with `next: { revalidate: 60 }` on the GitHub API call would let the Next.js Data Cache absorb repeated hits across instances without the hand-rolled memo. The current code degrades gracefully (just extra GitHub calls), but the advertised caching behaviour is misleading.

deltaKey recomputed twice per finding in the classification loop

apps/dev-ops/src/lib/delta.ts:45

In `computeDelta`, `deltaKey(f)` is called once to check `prevKeys.has()` and again implicitly when building `newKeys` (`newFindings.map(deltaKey)` at line 58). Computing the key once per finding and building `newKeys` inline eliminates the second pass: `for (const f of current) { const k = deltaKey(f); if (prevKeys.has(k)) carriedFindings.push(f); else { newFindings.push(f); newKeys.add(k); } }`. At dashboard scale (tens of findings) this is negligible — a clarity win, not a performance fix.

deltaKey title collision risk for file-less findings from different lenses

apps/dev-ops/src/lib/delta.ts:9

When `file` is null and `line` is null, the key reduces to `:::<normalized-title>`. Two structurally-different findings from different lenses with identical normalized titles would collide and be treated as the same finding (falsely 'carried'). The design intentionally ignores `lens` to handle lens-reassignment churn, but this degenerate case is worth a comment or a test asserting the known limitation.

Show/hide merged-closed toggle triggers a full server round-trip

apps/dev-ops/src/app/page.tsx:132

`<Link href='/?show=all'>` causes a full server re-render (DB + GitHub API call) on each toggle. Since `inactive` rows are already fetched in the same render, a `useState` client toggle would avoid the round-trip entirely. The current approach is simple and correct; worth noting only if the GitHub call is observed to be slow under load.