Phase C: Code review (internal/review) #6

New issue

Closed

opened 2026-09-03 23:44:29 +00:00 by crueber · 4 comments

crueber commented

2026-09-03 23:44:29 +00:00

Owner

Phase C: Code review — `internal/review`

Spec: docs/features/04_code_review.md (normative) + P1–P9 in docs/features/README.md.
Rollout: Wave C in docs/features/09_rollout.md §3 (parallel with pulls). Depends on 02 (thread pattern), 03 (PR objects, merge task, PR page).

Recommended implementation (verified against the doc)

New package internal/review: RouteProvider (Seam 1, both lanes via api.Lanes) + policy effect required-reviews (Seam 3, policy.RegisterEffect). Registers NO task kinds — the only long work is 03's merge task, which owns the gate.
Objects (repos/<o>/<r>/pulls/<num>/ sidecar subtree; num = shared P2 number; tid = 8-hex thread id):
- reviews/<seq:012x>.json (immutable Create: {kind:review, seq, at, by, state: APPROVED|CHANGES_REQUESTED|COMMENTED, commit_sha == PR head at submit or 409, body} + compensating review_dismissed (maintain only)).
- threads/<tid>/thread.json (CAS'd: anchor + resolved/resolved_by/resolved_at + counters) + threads/<tid>/events/<seq:012x>.json (review_thread_comment, immutable).
- review-requests.json (CAS'd current-state index; timeline audit via 03's review_requested/review_request_removed events).
- Counters next_review_seq + next_thread_num live on the PR header — ONE CAS arbitration point.
Anchor spec (normative, 04 §4): {path, side: NEW|OLD, old_start/old_lines, new_start/new_lines, commit_sha, context_sha} where context_sha = hex SHA-256 over path + "\n" + ≤3 context lines each side (trailing-whitespace-trimmed, LF-joined). Drift is DERIVED at view time (client recomputes from diff) — mismatch renders outdated, never relocated/mutated. Single implementation: anchorContextSha(hunk, range) in lib/diff.js.
Rollup (04 §6): review_summary {decision, latest{}, approvals, requested, threads_total, threads_unresolved} denormalized on PR header — pure function of immutable events recomputed inside the CAS loop (racers converge); latest-wins per reviewer, dismissal demotes to DISMISSED; staleness derived (commit_sha != head), never written.
Gate — required-reviews {min_approvals, dismiss_stale, bypass}: push-time half denies direct pushes at receive-pack (honest, enforceable); merge-time half evaluated by 03's merge task against the base ref (most-restrictive across matching rules; surviving approvals ≥ min, no surviving CHANGES_REQUESTED, stale dismissed when flagged). Gate re-derives by scanning review events — NEVER trusts the denormalized summary.
Rules: author self-approve/request-changes → 422 server-side; dismissal maintain-only; review-request add by author or triage+, self-remove allowed; review-suggest merges access.json ≥ read + team members + head-branch commit authors (20/page).

Acceptance criteria

Endpoints in 04 §7 with P6 gates; plain-text errors; no-store; discovery in /api/v1 endpoints[] with provenance review; SSE review (with fresh summary) + thread frames on the repo stream.
UI on 03's PR page per 04 §8 (summary bar, diff anchors, thread cards, finish-review modal, reviewers panel) + SDK repo.pulls.reviews/threads/requests/suggest (or deferred to Wave D with a note).
make cover ≥ 95% on internal/review; -race clean; table-driven httptest per handler.

# Phase C: Code review — `internal/review` **Spec:** `docs/features/04_code_review.md` (normative) + P1–P9 in `docs/features/README.md`. **Rollout:** Wave C in `docs/features/09_rollout.md` §3 (parallel with pulls). Depends on 02 (thread pattern), 03 (PR objects, merge task, PR page). ## Recommended implementation (verified against the doc) - **New package `internal/review`**: `RouteProvider` (Seam 1, both lanes via `api.Lanes`) + policy effect `required-reviews` (Seam 3, `policy.RegisterEffect`). **Registers NO task kinds** — the only long work is 03's merge task, which owns the gate. - **Objects** (`repos/<o>/<r>/pulls/<num>/` sidecar subtree; `num` = shared P2 number; `tid` = 8-hex thread id): - `reviews/<seq:012x>.json` (immutable `Create`: `{kind:review, seq, at, by, state: APPROVED|CHANGES_REQUESTED|COMMENTED, commit_sha == PR head at submit or 409, body}` + compensating `review_dismissed` (maintain only)). - `threads/<tid>/thread.json` (CAS'd: anchor + resolved/resolved_by/resolved_at + counters) + `threads/<tid>/events/<seq:012x>.json` (`review_thread_comment`, immutable). - `review-requests.json` (CAS'd current-state index; timeline audit via 03's `review_requested`/`review_request_removed` events). - Counters `next_review_seq` + `next_thread_num` live on the PR header — ONE CAS arbitration point. - **Anchor spec (normative, 04 §4):** `{path, side: NEW|OLD, old_start/old_lines, new_start/new_lines, commit_sha, context_sha}` where `context_sha` = hex SHA-256 over `path + "\n"` + ≤3 context lines each side (trailing-whitespace-trimmed, LF-joined). Drift is DERIVED at view time (client recomputes from diff) — mismatch renders *outdated*, never relocated/mutated. Single implementation: `anchorContextSha(hunk, range)` in `lib/diff.js`. - **Rollup (04 §6):** `review_summary {decision, latest{}, approvals, requested, threads_total, threads_unresolved}` denormalized on PR header — pure function of immutable events recomputed inside the CAS loop (racers converge); latest-wins per reviewer, dismissal demotes to DISMISSED; staleness derived (`commit_sha != head`), never written. - **Gate — `required-reviews {min_approvals, dismiss_stale, bypass}`:** push-time half denies direct pushes at receive-pack (honest, enforceable); merge-time half evaluated by 03's merge task against the base ref (most-restrictive across matching rules; surviving approvals ≥ min, no surviving CHANGES_REQUESTED, stale dismissed when flagged). Gate re-derives by scanning review events — NEVER trusts the denormalized summary. - **Rules:** author self-approve/request-changes → 422 server-side; dismissal maintain-only; review-request add by author or triage+, self-remove allowed; `review-suggest` merges access.json ≥ read + team members + head-branch commit authors (20/page). ## Acceptance criteria - [ ] Endpoints in 04 §7 with P6 gates; plain-text errors; `no-store`; discovery in `/api/v1 endpoints[]` with provenance `review`; SSE `review` (with fresh summary) + `thread` frames on the repo stream. - [ ] UI on 03's PR page per 04 §8 (summary bar, diff anchors, thread cards, finish-review modal, reviewers panel) + SDK `repo.pulls.reviews/threads/requests/suggest` (or deferred to Wave D with a note). - [ ] `make cover` ≥ 95% on `internal/review`; `-race` clean; table-driven httptest per handler.

crueber commented

2026-09-04 03:56:50 +00:00

Author

Owner

Wave C2 starting: implementing docs/features/04 (code review) on branch feat/review — new package internal/review, required-reviews gate wired into 03's merge task via a review-provided gate function (no merge fork), UI on the PR page + SDK.

crueber commented

2026-09-04 03:57:06 +00:00

Author

Owner

Wave C2 ready for review: #10 (branch feat/review). Summary, test/coverage results, and known gaps are in the PR description. Not merging per instructions.

Wave C2 ready for review: https://git.packden.us/crueber/walhub/pulls/10 (branch feat/review). Summary, test/coverage results, and known gaps are in the PR description. Not merging per instructions.

crueber commented

2026-09-04 03:59:03 +00:00

Author

Owner

Wave C2 review (PR #10, 719c3ac) — CLEAN, ready to merge.

Spec conformance (docs/features/04 + P1–P9):

§3 immutability: reviews/dismissals/thread-comments Create-only; dismissals compensating events, maintain-only (service.go:466), inert when targeting non-latest (model.go:559 isDismissed). commit_sha pin → 409 (service.go:348), author self-approve/request-changes → 422 (service.go:345). Both tested at service + HTTP layers.
§4 anchor/drift: Go DriftHash (model.go:322) and JS anchorContextSha (web/src/lib/diff.js:269, the only client impl, dep-free hand-rolled SHA-256) pin the same fixed vector 89e40705… in both suites — client+server agree.
§5 review-requests: CAS index, dedup, implicit removal on submit (service.go:441), author/triage+ auth + self-remove; suggest merges bindings → teams (Expander seam) → HeadAuthors, q-filtered, 20/page.
§6 rollup purity + gate-by-scan: Rollup/latestOf (model.go:353, service.go:580) shared by summary and gate; EvaluateGate (service.go:706) re-derives by event scan, never reads review_summary — poisoned-cache test pins it (gate_test.go:160). required-reviews combines most-restrictively; CHANGES_REQUESTED always blocks regardless of dismiss_stale.
§7 endpoints/auth/SSE: full §7 table on both lanes (http.go:49), read-gated everywhere, 401+WWW-Authenticate, plain-text errors, [] arrays, RFC3339, full-hex SHAs, no-store on JSON. review/thread frames via Streamer seam (nil until 06, documented deferral like 02/03).
§8: single anchorContextSha impl; §8Concurrency: no task kinds registered, no git subprocesses on review paths, no locks held across store calls (CAS loops only).

Seams/laws: no new deps (go.mod untouched; only internal+stdlib imports), core never imports review, pulls↔review one-directional via ReviewGate interface. Merge wiring is ONE call site (merge.go:161) with the live head; narration on failure. EvaluateProtect split verified by experiment: reverting pulls/policy.go:151 to Evaluate fails corr-g3 with "rejected by rule pr-gate" (the exact pre-fix symptom); restored, passes. Non-PR push behavior unchanged — receive-pack path (internal/api/policy.go:281) still uses full Evaluate, and pure-protect docs evaluate identically.

P6: requireRead on all paths, resolve = opener/participant/triage+ (threads.go:329). P5: LISTs bounded to one PR subtree. Overwritable families amended in 14 Decisions; E5 evidence plausible (formulas exact vs code).

Verification (scratch worktree /tmp/pr10, since removed): gofmt clean; go vet clean; go test -race review PASS (96.2% ≥95%); policy PASS (97.2%); pulls -race PASS (incl. merge-gate wiring); node --test diff-review + sdk-reviews 8/8. go build of affected packages OK (./... embed failure is only the unbuilt web/dist in a fresh worktree — environmental).

No fixes pushed — nothing to fix. MERGE RECOMMENDATION: ready to merge.

Wave C2 review (PR #10, 719c3ac) — CLEAN, ready to merge. Spec conformance (docs/features/04 + P1–P9): - §3 immutability: reviews/dismissals/thread-comments Create-only; dismissals compensating events, maintain-only (service.go:466), inert when targeting non-latest (model.go:559 isDismissed). commit_sha pin → 409 (service.go:348), author self-approve/request-changes → 422 (service.go:345). Both tested at service + HTTP layers. - §4 anchor/drift: Go DriftHash (model.go:322) and JS anchorContextSha (web/src/lib/diff.js:269, the only client impl, dep-free hand-rolled SHA-256) pin the same fixed vector 89e40705… in both suites — client+server agree. - §5 review-requests: CAS index, dedup, implicit removal on submit (service.go:441), author/triage+ auth + self-remove; suggest merges bindings → teams (Expander seam) → HeadAuthors, q-filtered, 20/page. - §6 rollup purity + gate-by-scan: Rollup/latestOf (model.go:353, service.go:580) shared by summary and gate; EvaluateGate (service.go:706) re-derives by event scan, never reads review_summary — poisoned-cache test pins it (gate_test.go:160). required-reviews combines most-restrictively; CHANGES_REQUESTED always blocks regardless of dismiss_stale. - §7 endpoints/auth/SSE: full §7 table on both lanes (http.go:49), read-gated everywhere, 401+WWW-Authenticate, plain-text errors, [] arrays, RFC3339, full-hex SHAs, no-store on JSON. review/thread frames via Streamer seam (nil until 06, documented deferral like 02/03). - §8: single anchorContextSha impl; §8Concurrency: no task kinds registered, no git subprocesses on review paths, no locks held across store calls (CAS loops only). Seams/laws: no new deps (go.mod untouched; only internal+stdlib imports), core never imports review, pulls↔review one-directional via ReviewGate interface. Merge wiring is ONE call site (merge.go:161) with the live head; narration on failure. EvaluateProtect split verified by experiment: reverting pulls/policy.go:151 to Evaluate fails corr-g3 with "rejected by rule pr-gate" (the exact pre-fix symptom); restored, passes. Non-PR push behavior unchanged — receive-pack path (internal/api/policy.go:281) still uses full Evaluate, and pure-protect docs evaluate identically. - P6: requireRead on all paths, resolve = opener/participant/triage+ (threads.go:329). P5: LISTs bounded to one PR subtree. Overwritable families amended in 14 Decisions; E5 evidence plausible (formulas exact vs code). Verification (scratch worktree /tmp/pr10, since removed): gofmt clean; go vet clean; go test -race review PASS (96.2% ≥95%); policy PASS (97.2%); pulls -race PASS (incl. merge-gate wiring); node --test diff-review + sdk-reviews 8/8. go build of affected packages OK (./... embed failure is only the unbuilt web/dist in a fresh worktree — environmental). No fixes pushed — nothing to fix. MERGE RECOMMENDATION: ready to merge.

crueber commented

2026-09-04 03:59:19 +00:00

Author

Owner

Wave C2 complete: PR #10 reviewed with no defects found, merged as 7e796be. internal/review 96.2% coverage, -race clean. Closing.

Wave C2 complete: PR #10 reviewed with no defects found, merged as 7e796be. internal/review 96.2% coverage, -race clean. Closing.

crueber closed this issue

2026-09-04 03:59:19 +00:00

crueber added this to the v1 milestone

2026-09-10 22:20:50 +00:00

crueber added the

pull requests