feat/review-live

needs attention

de04d30 · fullpre-PRreviewed 2026-07-05 05:09 UTC8H · 12M · 5L · 1I

The branch

Purpose

Give the dev-ops dashboard real-time visibility into Loop C review activity — which branches are being reviewed right now, which are idle, and how long reviews take.

Goal

Show created-at + last-updated timestamps per branch row and a live LoopStatus strip that auto-refreshes without manual reloads.

Sub-goals

SG-1: review_runs schema + migration + rrn_ prefix
SG-2: dev-ops write path POST /api/review-runs
SG-3: UI — Loop status strip + created/updated columns + auto-refresh
SG-4: loop-review wiring — post run lifecycle (running/done/failed)
SG-5: Framework learnings fold-back

The changes (whole branch)

What: Branch provisioned with scope.md and cleared intent.md. No implementation code committed yet.
Why: Operators running Loop C need live observability — a crashed/stalled review should show as stalled, not silently disappear; a healthy review should show the branch + SHA + elapsed time in real time.
Areas: .branch+79−67
Blast: 2 files, +79/-67 — .branch/ planning docs only. No production code changed.

plan-only-commit pre-implementation design-review

➖typecheck· no implementation files to check➖tests· no implementation files to check➖coderabbit· no .coderabbit.yaml

Findings · 26

correctness6

high

Concurrent running POSTs race condition on slug upsert

.branch/scope.md

Add WHERE EXCLUDED.started_at >= review_runs.started_at guard to the ON CONFLICT clause.

high

Preserve startedAt on terminal upsert needs explicit SQL guidance

.branch/scope.md

SQL must SET started_at = review_runs.started_at; caller must not send startedAt in terminal POST body.

medium

Stale detection without heartbeats can't distinguish slow from crashed

.branch/scope.md

Document the chosen cutoff rationale; consider 2–3 min heartbeat re-POSTs.

medium

router.refresh() re-fetches full RSC tree every 10s

.branch/scope.md

Scope polling to a dedicated /api/loop-status route or client-only strip to avoid re-running all server queries.

medium

Bearer token rotation plan missing from scope

.branch/scope.md

Specify SSM param name and runner injection method so rotation is a documented procedure.

low

review_runs.sha / code_reviews.sha join semantics unspecified

.branch/scope.md

security4

medium

service_role_all RLS — confirm UI never holds the service key client-side

Dashboard must read only through the bearer-authenticated API, never via a service-role Supabase client in the browser.

medium

post-run.mjs should validate DEVOPS_WRITE_URL is https-only before posting

.claude/skills/loop-review/lib/post-run.mjs

Validate URL scheme before posting branch/SHA metadata to prevent env-injection misdirection.

low

Verify crypto.timingSafeEqual in implementation, not just intent

apps/dev-ops/src/app/api/review-runs/route.ts

info

No TTL on stale running rows — hygiene concern

A periodic cleanup or auto-cancel for rows older than 2h would keep the table clean.

conventions3

medium

Upsert-on-slug diverges from code_reviews upsert-on-(slug,sha) — add schema comment

packages/database/src/schema/review-runs.ts

Intentional, but needs an explanatory comment explaining the different upsert strategy.

low

Schema file should export REVIEW_RUN_PREFIX = 'rrn' as const

packages/database/src/schema/review-runs.ts

low

Nullable mode and verdict columns should carry inline comment

packages/database/src/schema/review-runs.ts

tests6

high

Concurrent running POST test case missing from plan

.branch/scope.md

Assert exactly one row after two concurrent running POSTs, with the earlier startedAt preserved.

high

No integration test bridging write path to UI read query

.branch/scope.md

Write a review_runs row then fetch via the LoopStatus query to catch column projection and RLS gaps.

high

classifyRun edge cases not enumerated in test plan

.branch/scope.md

Add: status=running age>staleMin→stalled; unknown status→no throw; future finishedAt; staleMin=0.

medium

propose-never-merge safety is human-audit only

.branch/scope.md

A grep-based CI assertion verifying post-run.mjs has no gh pr merge / git push to main calls would make this machine-verifiable.

medium

AutoRefresh polling behavior not covered by any stated test method

apps/dev-ops/src/app/_components/AutoRefresh.tsx

vi.useFakeTimers + React Testing Library would cover the interval and cleanup-on-unmount paths.

medium

RLS correctness not verified at runtime — only schema syntax checked

.branch/scope.md

An integration test with two RLS-scoped connections asserting row visibility would give runtime confidence.

improvement7

high

DB table + migration + RLS + prefix may be over-engineered for a liveness signal

.branch/scope.md

A per-slug JSON file in LOOPS_DIR (same pattern as ledger.json) would serve the use case without schema migrations, RLS, shared-kernel coupling, or HTTP round-trips.

high

'cancelled' status is structurally unguaranteable under GHA SIGTERM

.branch/scope.md

Drop 'cancelled'; use 'failed' for all non-success terminations. The stale cutoff handles the UX gap.

high

runStaleMinutes=30 equals workflow timeout — zero detection buffer

.claude/skills/loops-engine/config.json

Set runStaleMinutes to 20–25, or extend workflow timeout to 40 to get a buffer before SIGTERM.

medium

getLatestPerBranch needs SQL GROUP BY rewrite in SG-3, not JS enrichment

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

Replace JS dedup with DISTINCT ON (slug) + aggregate subquery before adding firstSeen/lastUpdated/count.

medium

timeAgo belongs in dates.ts, not a new run-status.ts

apps/dev-ops/src/lib/run-status.ts

dates.ts already has resolveReviewedAt. Put timeAgo there; keep classifyRun in run-status.ts.

medium

startedAt semantics need explicit docs: reset-on-running vs preserved-on-terminal

.branch/scope.md

Without disambiguation, implementations conflate the run's startedAt with the branch's first-seen (code_reviews.createdAt).

low

rrn_ prefix in shared-kernel adds cross-package coupling for an internal-only table

packages/shared-kernel/src/types/ids.ts