US-45.3: review a change thread as a placed runner dispatch (contract 1.21.0)

[mkreyman]

Implements US-45.3 (review on a change thread). Replaces #905 (closed at its round-3 review ceiling) and the approach of #901.

Why a rewrite

#901 and #905 made the reviewer a principal holding an API key minted for the review, and every review round on them found another way that key surface could be abused or mis-bound: who may mint it, what it inherits, whether a returned key could be replayed by another caller, whether the judge is inferred from the key. Round 3 of #905 still found material trust-path defects, which is evidence about the design rather than the fixes. This PR removes the surface instead of defending it: no credential is minted for a review or returned to anyone.

Design

• A review is a runner dispatch of kind review, placed by Loopctl.Delivery.Placement.place_review/4 exactly as an implement dispatch is placed (caller resolution, halt, human anchor, runner accepting work and not exhausted, budgets through DispatchPayload.fill, Runners.dispatch). The checkpoint reviewed is the story's LATEST checkpoint under the current claim epoch; a caller never chooses it.
• Findings and the verdict arrive over the runner socket as review_finding and review_verdict (runner contract 1.21.0), attributed to the runner's accepted review-kind DispatchLedger row, through Loopctl.Delivery.RunnerReviews (same busy / broken-chain answers as RunnerThreads). There is no HTTP path for either.
• One narrow entry point per write in Loopctl.Threads: record_review/3, record_judgement/5, record_fix/4. Each re-checks its binding under the story's thread lock. insert_entry, the lock and replay stay private.
• Separation (Threads.Reviews.reviewer_separate/3): refused when the runner's agent is the claimant, recorded a checkpoint of the thread, or holds a dispatch on the implementer's lineage chain in either direction (Dispatches.lineage_same_chain?/2). Decided at placement AND under the lock on every judgement. Its code is reviewer_not_separate, never self_review_blocked (which feeds the L6 halt).
• Custody halt is enforced on judgement writes (Threads joins Loopctl.Delivery.Placement in ContextSurface's halt-enforcing set).
• Rounds from data: one verdict per review (partial unique index), round 3 only when a round-2 finding's introduced_by names a checkpoint a round-1 fix carried (fixes recorded before the round-2 verdict), never round 4.
• Durable review ceiling: a round-2 verdict with a critical/high/medium finding records an escalation entry in the verdict's own transaction; Loopctl.Workers.ReviewCeilingWorker (cron, every minute, plus a fast path after the verdict) moves the stage over the new :review_ceiling edge until it lands or the claim epoch moves.
• Fixes are fenced on the claimant under the current epoch with a live lease, carried by a checkpoint of the current claim recorded after every checkpoint their findings were found in.
• Kept from US-45.3: review dispatch, checkpoint-bound findings and a server-side round ceiling #905: review-scoped idempotency (thread_entries_review_idempotency_uidx), the fix CHECK with finding_ids IS NOT NULL, shared ThreadHTTP rendering with location_untrusted, bounded payloads (fixes_truncated), the distinct :review_ceiling control edge.

Surface

• POST /api/v1/stories/:id/thread/reviews (orchestrator+, human-anchored) requests a placement; GET .../thread/reviews/:review_id reads the payload; POST .../thread/fixes (exact agent, the claimant).
• MCP 2.106.0: thread_request_review, thread_review_get, thread_fix; thread_entry's description now says where judgements go.
• Runner contract 1.21.0: review in dispatchable_kinds (not implied_by_silence), RunnerReview payload, the two messages, their acks, refusal codes in every registry, permanence (tenant_halted is not permanent). priv/runner_contract/v1.json regenerated, digest updated.
• Migration 20260926160000: thread_reviews, judgement columns and CHECKs on thread_entries, runner_dispatches.kind widened to review; rollback refuses while review rows exist.
• Env: REVIEW_WALL_CLOCK_SECONDS, REVIEW_MAX_TURNS (no default; budget_unset otherwise), documented in deploy/FLY_SECRETS.md.
• Story us_45.3.json: AC-45.3.1 and AC-45.3.3 reworded to this design, technical notes record it.

Found and fixed while mutation-testing, before this PR: a caller-chosen dispatch_id that the runner ledger already held for an implement dispatch would have bound the review to that session (record_sent/3 does nothing on conflict). Threads.record_review/3 now refuses it dispatch_id_conflict under the lock, and RunnerReviews keeps the ledger-kind check as the backstop for the reverse order.

Acceptance criteria

AC	Where it is proven
45.3.1 review kind, placed through Placement, separation	review_placement_test.exs "place_review/4"; reviews_test.exs "record_review/3" (claimant, checkpoint recorder, implementer chain, ended-claim checkpoint)
45.3.2 payload	reviews_test.exs "payload/3 (TC-45.3.2)"; thread_review_controller_test.exs untrusted markers
45.3.3 judgements only from the placed runner's review dispatch; one verdict; no round without one	review_placement_test.exs "judgements over the socket"; reviews_test.exs "record_judgement/5", "the schema"; controller test "there is no HTTP path for a finding or a verdict"
45.3.4 severity, introduced_by canonical	reviews_test.exs severity / introduced_by tests
45.3.5 fix fence	reviews_test.exs "record_fix/4"; controller fix test
45.3.6 round 3 rule, ceiling escalation, no round 4	reviews_test.exs "the round ceiling (TC-45.3.5)"; review_placement_test.exs round-2 ceiling escalates the stage; review_ceiling_worker_test.exs
45.3.7 never self_review_blocked	refusal codes pinned in the contract test and on the wire (reviewer_not_separate)
45.3.8 contract 1.21.0	runner_contract_test.exs "review (contract 1.21.0, US-45.3)", digest; placement_test.exs credential-field pin now includes RunnerReview

Mutation proof

Every row is one ~/workspace/claude-config/bin/mutate.sh invocation (stdin from /dev/null, never piped); exit 0 = the check went red under the mutation. Each log was read to confirm the red was a test failure, not a compile error.

#	Mutation	Check	Exit
M1	channel routes review_finding under another name	review_placement_test	0 (5 failures)
M2	RunnerReviews accepts any ledger kind	review_placement_test	1 survived - fixed by the ledger-collision test, see M18
M3	judgement binding ignores the runner	reviews_test	0
M4	judgement skips the custody halt	review_placement_test	0
M5	separation ignores the implementer's lineage chain	reviews_test	0 (2)
M6	latest checkpoint taken regardless of claim epoch	reviews_test	0
M7	round-3 rule counts fixes AFTER the round-2 verdict	reviews_test	0 (2)
M8	ceiling never records an escalation	reviews_test + worker test	0 (4)
M9	worker candidates ignore the claim epoch	worker test	1 survived - test now also refutes the retry log; see M19
M10	verdict fast path skips reconcile/2	review_placement_test	0
M11	review dropped from dispatchable kinds	review_placement_test	0 (6)
M12	reviewer_not_separate not permanent for review_finding	runner_contract_test	0 (3)
M13	reviewer_not_separate not verbatim in Refusal	refusal_test + review_placement_test	0
M14	controller loses runner_not_connected mapping	thread_review_controller_test	0
M15	MCP thread_request_review on the claim key	node threads_tool.test.js	0 (2)
M16	MCP thread_fix not dispatched	node threads_tool.test.js	0
M17	MCP accepts an empty finding_ids	node threads_tool.test.js	0
M18	(M2 again, after the new test)	review_placement_test	0
M19	(M9 again, after the log assertion)	worker test	0
M20	record_review skips the ledger check	review_placement_test	0
M21	ledger check accepts any kind	review_placement_test	0
M22	migration: fix CHECK without finding_ids IS NOT NULL (run LAST; the check re-migrated the test DB under the mutation, then the DB was re-migrated and the live constraint confirmed restored)	reviews_test	0

Verification

• mix compile --warnings-as-errors, mix format, mix credo --strict (whole project, no issues), mix dialyzer (a spec moved: RunnerThreads.idempotency_key/1 now takes an open map), the pre-commit gate.
• Related suites: threads, thread/review controllers, runner channel (all files), test/loopctl/delivery, worker, api_spec, context_surface, tier_capabilities, oban config/topology, router snapshot, openapi, user story schema: 0 failures. MCP: node --test test/*.test.js 769/769, incl. route coverage.

Not tested / known limits

• No real runner has run a review: the runner side (re-vendoring 1.21.0, declaring review, sending the messages) is the runner repo's work.
• A review session that dies without a verdict frees its slot through the existing HealRunnerCapacityWorker, not through session_ended (which stays implement-only); that path has no review-specific test.
• The ledger-collision refusal is decided under the thread lock but reads the ledger outside the push, so a concurrent implement push under the same caller-chosen id can still land first; the RunnerReviews kind check (M18) is what refuses that session's judgements.

The review gate has not run yet.

Review round 3 (the ceiling), fixed in place (cc860ee)

No finding touched the authorship design, so the ten findings plus the rollback test were fixed in place, following #902's round 3. No round 4; every fix carries mutation proof.

8. Kind-blind ledger accessors: accepted_session defaults to implement rows, held/4 requires an explicit kind, the per-consumer kind checks are deleted, and the write-path under-lock checks keep direct tests.
9. The review payload is cast before record_review, so an invalid budget leaves no row, chain entry or push.
10. A resend is answered from its row before the halt check; a halt refuses new writes only.
11. Releasing the review slot never raises in the channel; on failure it logs and HealRunnerCapacityWorker frees it.
12. A retry answers from the row only while it is sent or accepted; refused or superseded is 409 review_dispatch_refused.
13. Only a MATERIAL round-2 finding introduced by a fix opens round 3.
14. A losing concurrent placement (:existing) never pushes (deterministic committed race test).
15. A malformed body UUID is 422 invalid_uuid naming the field.
16. The review slot is freed at session_ended, not at the verdict (contract 1.21.0 edited in place, digest 37fcafef).
17. The slot check reads one runner's row.
18. thread_reviews down() refusal test (thread_reviews_rollback_test.exs).

Mutations S1-S71 re-run after the fixes, all exit 0 except as noted: S7 went vacuous under fix 5 and S51 exposed an untested triage check; both got tests and their replacements S7b/S51b exit 0. S28 was retired (its target is gone) and replaced by S68. Migration mutations S69-S71 ran last, followed by a re-migrate. Session check: answer_recorded's sent/accepted guard mutated to accept any status, exit 0.

Known window, stated rather than hidden: a socket drop or lost slot between the pre-checks and the push leaves an unpushed row; a retry with the same dispatch_id pushes it.