feat/review-rigor

isMergedOrClosed hides branches when prs=null but liveBranches shows deletion

apps/dev-ops/src/lib/pr-grouping.ts:63

When state.prs is null (GitHub PRs API failed) but liveBranches shows a branch no longer exists, the function falls through to the liveBranches check and returns true (hidden). The contract says each null signal means 'don't hide on this basis' — but prs=null + deleted branch does hide. Fix: gate the liveBranches check on `state.prs !== null` so branch deletion is only used as a fallback when the PR map exists but simply has no entry, not when the PR fetch itself failed.

Dedup key (slug) and classification key (branch) are on different fields

apps/dev-ops/src/lib/pr-grouping.ts:19

groupLatestPerPr deduplicates by r.slug (URL-slugified), isMergedOrClosed classifies by r.branch (raw git ref). These are derived from the same branch name but are distinct strings. If two branches produce the same slug, they'd be grouped as one but classified independently. Both should use the same key field.

PR pagination cap of 3 pages misses branches of older closed PRs

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

fetchPrsByBranch fetches up to 300 PRs (3 pages × 100, updated desc). A PR last updated >300 merged PRs ago won't appear in the map. The liveBranches fallback catches the common case (merged PRs usually auto-delete the branch), but a squash-merged PR with a kept branch would stay shown as active. Degradation direction (shown, not hidden) is safe. Flagging for awareness as the repo grows.

Module-level memo is process-scoped; correct for dev-ops EC2, no-op on Vercel cold starts

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

Each Vercel cold-start gets a fresh module (memo = null), so on high-traffic spikes every cold-start fires a GitHub API call. Low impact given the dashboard's traffic profile.

GITHUB_REPO env var injected unsanitized into GitHub API URL (SSRF surface)

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

githubRepo() reads process.env.GITHUB_REPO with no validation, then interpolates it into `https://api.github.com/repos/${repo}/pulls?...` and the branches URL. A compromised Vercel env could redirect token-bearing requests to another private repo. Fix: validate the value matches `/^[a-zA-Z0-9_.-]+\/[a-zA-Z0-9_.-]+$/` before use, falling back to DEFAULT_REPO on mismatch. Also affects prUrl() which renders this value into UI links.

Module-level memo can serve stale data across warm serverless invocations

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

The `let memo` pattern is designed for a long-lived process (the EC2 dev-ops box) but the app also deploys to Vercel where function instances are pooled. If DEVOPS_GITHUB_TOKEN is rotated, the old token's results persist in-process for up to 60s. Idiomatic fix: Next.js `unstable_cache` with `revalidate: 60` scopes correctly to both long-lived and serverless runtimes.

Token scope: branches API only needs metadata:read, not contents:read

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

The comment now requires `contents:read` for the branches endpoint. On fine-grained GitHub PATs, the branches list API only requires `metadata:read` — `contents:read` grants broader access (file contents, commits). Consider using a fine-grained PAT with only `pull_requests:read` + `metadata:read` and update the comment accordingly.

Silent error swallowing makes PAT misconfiguration invisible to operators

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

Both fetchers catch all exceptions and non-ok responses and return null, degrading to 'show all'. An expired/revoked PAT silently shows merged PRs with no operator signal. A `console.error` on non-ok HTTP status (logging the status code, not the token) would make misconfiguration observable in server logs.

Token is never logged or exposed to client — handling is correct

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

DEVOPS_GITHUB_TOKEN is server-only, not NEXT_PUBLIC_, used only in Authorization headers of outbound GitHub calls, and never serialized in responses. No issues.

Inconsistent variable name for RepoBranchState (state vs repoState)

apps/dev-ops/src/app/[slug]/page.tsx:29

page.tsx calls the result `state` while [slug]/page.tsx calls it `repoState`. `state` alone is ambiguous (React useState convention). Standardize on `repoState` across both pages.

isMergedOrClosed name is semantically wrong for the deleted-branch case

apps/dev-ops/src/lib/pr-grouping.ts:59

Returns true for merged PR, closed PR, and branch deleted with no PR (abandoned spike). 'Merged or closed' doesn't describe the third case. The docstring already says 'inactive (merged/closed/deleted)' — rename to `isInactive` or `isBranchInactive` to match. partitionByPrState already uses the neutral 'inactive' term.

Module-level memo unreliable on Vercel serverless (concurrent stampede gap)

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

Two concurrent requests arriving when memo is stale both pass the TTL guard simultaneously and make duplicate GitHub API calls. Use Next.js `unstable_cache` or store the in-flight Promise itself as a dedup key — the standard App Router pattern for this use case.

stateUnknown boolean is superfluous indirection

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

`const stateUnknown = state === null` adds a layer of indirection without semantic gain — inline `state === null` is already self-documenting given the `RepoBranchState | null` type. Remove the variable.

getLatestForSlug and getBySlugAndSha are exported but unused after refactor

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

Both functions are exported but no longer called by any file. The slug page now uses getHistoryBySlug for everything. Dead exports add surface area; remove them.

Test row helper includes sha field not in PrGroupable interface

apps/dev-ops/src/lib/pr-grouping.test.ts:13

`sha` is in the row helper's Partial type but is not a field of PrGroupable. It works via TypeScript structural typing but carries a phantom field. Assert by `slug` (present) instead of `sha` in partition tests.

Pure/impure separation is clean and well-enforced

apps/dev-ops/src/lib/pr-grouping.ts

All I/O is confined to github.ts. pr-grouping.ts is purely functional — no async, no env reads. PrGroupable is a correct minimal projection. FCIS-aligned.

ReadonlyMap / ReadonlySet used correctly on returned types

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

ReadonlyMap<string, PrRef> and ReadonlySet<string> on the interface fields prevent callers from mutating internal state. Internal builders use plain Map/Set (assignable to Readonly supertypes). Idiomatic.

Partial-signal failure (prs=null, liveBranches=Set) not tested

apps/dev-ops/src/lib/pr-grouping.test.ts:79

RepoBranchState allows each field to be null independently. No test builds state with prs=null and liveBranches=non-null — a real degraded-mode scenario (pulls API errors, branches API succeeds). In this state, state.prs?.get(r.branch) is undefined, the liveBranches fallback fires, and a deleted branch would be wrongly hidden (the medium correctness finding). Adding this test would both document the contract and catch the bug.

stored prNumber vs branch-map conflict not tested in isMergedOrClosed

apps/dev-ops/src/lib/pr-grouping.test.ts:84

isMergedOrClosed ignores r.prNumber entirely, keying only on r.branch. If a review has a stored prNumber (say 254) but the branch map shows that branch with a merged PR (266, open: false), the function returns true (hidden). This resolution order is probably correct but no test pins it. A test with row({ prNumber: 254, branch: 'feat/x' }) against state({ 'feat/x': { number: 266, open: false } }) would document the contract and prevent regression.

resolvePrNumber: stored prNumber short-circuit when state is present not tested

apps/dev-ops/src/lib/pr-grouping.test.ts:59

The 'prefers the stored prNumber' test passes null for state. A test with both prNumber=254 and state containing a different branch mapping would document the short-circuit behaviour when both sources are available.

Fork-PR scenario (head.ref ≠ review branch) not addressed in tests

apps/dev-ops/src/lib/pr-grouping.test.ts:84

The comment in github.ts explicitly limits to same-repo PRs. A test or inline comment documenting the fork miss (branch lookup returns undefined → falls to liveBranches → likely stays shown) would make the boundary explicit and prevent future 'fixes' that break the same-repo fast path.

Regression anchor for the exact bug (feat/tb-deploy) is present

apps/dev-ops/src/lib/pr-grouping.test.ts

Test at line 89 directly encodes the bug: prNumber=null, branch='feat/tb-deploy', PR merged, branch still live. Named comment '// The real case: feat/tb-deploy — PR #261 merged, branch not deleted'. The rigor bar (test fails before patch, passes after) is met for the primary bug path.

liveBranches second API call may be eliminable

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

The fetch exists to catch 'no PR record AND branch deleted' (abandoned pre-PR spikes). But when such a branch gets a PR and that PR merges, the prs map (with open: false) already catches it. The only remaining case is a branch pushed, reviewed, never got a PR, then deleted — a rare path. Cost: 1-6 extra HTTP round-trips per render cycle (up to 600 branches, 6 pages). Worth evaluating whether the abandoned-spike case justifies the extra call; if not, drop fetchLiveBranches and let those reviews stay shown (safe default).

fetchLiveBranches has asymmetric 6-page budget vs 3 pages for PRs

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

3 pages for PRs is documented as 'covers the whole repo today'. The same reasoning applies to branches — fewer active branches than total PRs. 6 pages (600 branches) doubles worst-case latency with no documented justification. Match to 3 pages or add a comment.

Concurrent warm-lambda memo stampede: two simultaneous stale reads fire double fetches

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

Two concurrent requests both finding memo stale both invoke fetchRepoBranchState in parallel, making 4 GitHub API calls instead of 2. Store the in-flight Promise in memo to deduplicate concurrent callers — the standard pattern for hand-rolled async caches.

PR sort=updated ordering is approximate for same-branch multi-PR case

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

Comment says 'sorted by last-updated desc so the first PR seen is the newest'. GitHub sort=updated orders by last update time, not creation time. For this repo's single-PR-per-branch workflow this is irrelevant, but sort=created&direction=desc would be more precisely correct if the guarantee matters.