US-45.3: review dispatch, checkpoint-bound findings and a server-side round ceiling

.claude/skills/chain-of-custody/SKILL.md

@@ -145,6 +145,19 @@ column and not a `metadata` key on purpose**: `metadata` is cast by `Story.updat
 REPLACED wholesale by `PATCH /api/v1/stories/:id`, so one ordinary orchestrator request erased the
 marker and handed the launder path back. Never add `:lifecycle_entered_at` to a `cast` list.
 
+## Review on a change thread (US-45.3) — the judge is a PLACED dispatch, never an inferred key
+
+`Loopctl.Threads.Reviews` decides who may write a `finding` or `verdict` without inferring
+separation from the calling key, which #901 tried and had circumvented in three review rounds.
+`place/4` mints the reviewer's dispatch as a SIBLING of `implementer_dispatch_id` (same parent)
+and records it in `thread_reviews`; a judgement is accepted only when
+`Dispatches.dispatch_for_api_key/2` resolves the calling key to that exact row. Do not widen
+this to a lineage or agent comparison. Its refusals have their own codes
+(`review_dispatch_required`, `reviewer_not_separate`, ...) and never `self_review_blocked`,
+which escalates to the L6 halt. The round count and ceiling are computed from
+`thread_entries`; a verdict revokes the review's dispatch so its agent's one agent-role key
+slot (`api_keys_one_role_per_agent_idx`) frees for the next round.
+
 ## Claim lease and epoch fence (#803)
 
 A claim is not held forever. `claim_story/3` (and `BulkOperations.bulk_claim/4`, via

CHANGELOG.md

@@ -6,6 +6,27 @@ All notable changes to loopctl are documented here.
 
 ### Added
 
+- **Review on a change thread (epic 45, US-45.3; migration `20260926160000`, no manual
+  step).** `POST /api/v1/stories/:id/thread/reviews` (orchestrator key or higher) mints the
+  reviewer's dispatch as a sibling of the implementer's and returns its key once; a
+  `finding` (`/thread/findings`) or `verdict` (`/thread/verdicts`) is accepted only on that
+  key, bound to the review and the checkpoint it reads, and the claimant records the `fix`
+  that answers findings (`/thread/fixes`). `GET /thread/reviews/:review_id` is the review's
+  payload. The round count and its ceiling are computed from the thread: a verdict completes
+  its round and revokes the review's dispatch and key, round 3 is placeable only when a
+  round-2 finding's `introduced_by` names a checkpoint a round-1 fix is carried by, and there
+  is never a round 4; a ceiling reached with a critical, high or medium finding records a
+  `review_ceiling` escalation and escalates the story's delivery stage over a new control-only
+  `review_ceiling` stage edge (not runner-reportable; the runner contract is unchanged). The migration adds
+  the `thread_reviews` table and nullable `review_id`, `severity`, `location`,
+  `introduced_by` and `finding_ids` columns on `thread_entries`. New refusal codes, none of
+  them `self_review_blocked`: `review_dispatch_required`, `review_closed`,
+  `review_round_superseded`, `review_ceiling_reached`, `reviewer_not_separate`,
+  `reviewer_agent_busy`, `review_parent_inactive`, `unresolvable_dispatch_lineage`,
+  `caller_lineage_required` (a key no dispatch minted places a review only as the operator),
+  `implementer_dispatch_required`, `no_checkpoint`, and the `introduced_by_*`, `fix_*` and
+  `invalid_*` validation codes. MCP 2.106.0 ships the matching tools. The runner contract is
+  not bumped yet: `review` is not a dispatchable kind (AC-45.3.8 remains).
 - **Runners may report checkpoints and notes on a story's change thread (epic 45, US-45.2,
   runner contract 1.20.0). RE-VENDOR the contract to send them; a runner that does not gets
   today's behaviour.** Two new optional channel messages. `checkpoint` carries `{dispatch_id,

lib/loopctl/custody/context_surface.ex

@@ -41,7 +41,7 @@ defmodule Loopctl.Custody.ContextSurface do
   # Module names as STRINGS, mirroring `TierCapabilities`' two maps — the scan compares them
   # against `defmodule` lines in source text, and a compile-time reference would make this
   # module depend on every context it lists.
-  @halt_enforcing_contexts ["Loopctl.Delivery.Placement"]
+  @halt_enforcing_contexts ["Loopctl.Delivery.Placement", "Loopctl.Threads.Reviews"]
 
   @doc """
   Context modules that enforce the custody halt themselves. Bound to the modules that actually

lib/loopctl/delivery/escalations.ex

@@ -150,8 +150,31 @@ defmodule Loopctl.Delivery.Escalations do
   def escalate(tenant_id, story_id, opts) do
     epoch = Keyword.fetch!(opts, :claim_epoch)
 
-    with :ok <- claimant_and_epoch(tenant_id, story_id, Keyword.fetch!(opts, :agent_id), epoch),
-         {:ok, row} <- live_row(tenant_id, story_id),
+    with :ok <- claimant_and_epoch(tenant_id, story_id, Keyword.fetch!(opts, :agent_id), epoch) do
+      escalate_row(tenant_id, story_id, opts)
+    end
+  end
+
+  @doc """
+  Escalates `story_id` on loopctl's own decision rather than on behalf of its claimant: a
+  review ceiling reached by the thread's review (`Loopctl.Threads.Reviews`). The same replay
+  and `:stale_stage` recovery as `escalate/3`, without the claimant check, because the
+  principal deciding is not the claimant, and over the control-only `:review_ceiling` edge
+  rather than the session's `:session_escalated`. The actor is recorded as control does
+  elsewhere (`Loopctl.Delivery.TriageDispatcher`): a `control:` label and `actor_role: :agent`,
+  which keeps every human-only edge out of reach; there is no system role. Takes `escalate/3`'s
+  options except `:agent_id`; `:claim_epoch` is the story's current epoch, which the fence
+  still checks.
+  """
+  @spec escalate_as_control(Ecto.UUID.t(), Ecto.UUID.t(), keyword()) ::
+          {:ok, StoryStage.t()} | {:error, error()}
+  def escalate_as_control(tenant_id, story_id, opts),
+    do: escalate_row(tenant_id, story_id, Keyword.put(opts, :edge, :review_ceiling))
+
+  defp escalate_row(tenant_id, story_id, opts) do
+    epoch = Keyword.fetch!(opts, :claim_epoch)
+
+    with {:ok, row} <- live_row(tenant_id, story_id),
          :continue <- unless_already_escalated(row, epoch) do
       advance(tenant_id, story_id, row, opts)
     else
@@ -213,7 +236,8 @@ defmodule Loopctl.Delivery.Escalations do
   # cannot re-enter the recovery: with the recovery inside the only attempt function, a story
   # a runner keeps advancing would have recursed without bound.
   defp attempt(tenant_id, story_id, row, opts) do
-    transition = {row.stage, :escalated, :session_escalated}
+    # `:session_escalated` for a claimant; `escalate_as_control/3` names its own control edge.
+    transition = {row.stage, :escalated, Keyword.get(opts, :edge, :session_escalated)}
 
     advance_opts = [
       claim_epoch: Keyword.fetch!(opts, :claim_epoch),

lib/loopctl/delivery/placement.ex

@@ -1130,7 +1130,16 @@ defmodule Loopctl.Delivery.Placement do
   # unreachable clause that reads as a guard is worse than no clause — and if `resolve_caller/2`
   # ever hands this something else, that is a broken invariant inside this module and a crash
   # is the right answer, not a `:root_dispatch_forbidden` that hides it.
-  defp may_mint_session_dispatch([], role) do
+  @doc """
+  Whether a caller with this server-resolved `lineage` and key `role` may mint a custody
+  dispatch for someone else: an EMPTY lineage only for the tenant's operator (a `:user`-or-
+  higher key no dispatch minted), a lineaged caller from `:orchestrator` up. The ONE copy of
+  that positive operator test for every context path that mints without a controller
+  (`Loopctl.Threads.Reviews` too).
+  """
+  @spec may_mint_session_dispatch([Ecto.UUID.t()], atom()) ::
+          :ok | {:error, :root_dispatch_forbidden | :insufficient_role}
+  def may_mint_session_dispatch([], role) do
     if Role.role_at_least?(role, :user), do: :ok, else: {:error, :root_dispatch_forbidden}
   end
 
@@ -1140,7 +1149,7 @@ defmodule Loopctl.Delivery.Placement do
   # lineaged AGENT-role key, which the HTTP surface 403s, minted a child custody dispatch and a
   # live ephemeral key through this path until this clause tested the role too. `:user` clears
   # `:orchestrator` by hierarchy, so the clause above is strictly stronger and the two agree.
-  defp may_mint_session_dispatch([_ | _], role) do
+  def may_mint_session_dispatch([_ | _], role) do
     if Role.role_at_least?(role, :orchestrator), do: :ok, else: {:error, :insufficient_role}
   end
 

lib/loopctl/delivery/stage_machine.ex

@@ -63,6 +63,10 @@ defmodule Loopctl.Delivery.StageMachine do
     with no way out, and it is its own edge rather than `:session_escalated` because the
     escalated queue has to tell "the loop spent its budget on this" apart from "the session
     asked for Mark". Never retried: a budget kill retried is the same kill again, paid twice.
+  - `:review_ceiling` — an in-flight stage -> escalated, when a change thread's final review
+    round completes with a critical, high or medium finding (US-45.3). CONTROL takes it, from
+    `Loopctl.Threads.Reviews`, never a runner: it is loopctl's verdict about the review, and
+    the remedy is a rewrite, which is Mark's call.
   - `:runner_lost` — an in-flight stage -> queued. Taken by the claim reclaimer
     (`Loopctl.Progress.reclaim_expired_claim/3`), never asked for by a runner: a runner
     that could report itself lost is not lost.
@@ -172,6 +176,13 @@ defmodule Loopctl.Delivery.StageMachine do
   # and there the session's own `:session_escalated` is the way out. See the moduledoc entry.
   @budget_reported for from <- @in_flight, do: {from, :escalated, :budget_reported}
 
+  # A review ceiling reached (US-45.3). CONTROL takes it, from `Loopctl.Threads.Reviews` via
+  # `Loopctl.Delivery.Escalations.escalate_as_control/3`, when a thread's final review round
+  # completes with a material finding. Its own edge rather than `:session_escalated`, so the
+  # escalated queue shows the loop's own verdict rather than a session asking for Mark, and it
+  # is not in `@runner_reportable_edges`: a runner cannot assert a review outcome.
+  @review_ceiling for from <- @in_flight, do: {from, :escalated, :review_ceiling}
+
   @transitions @forward ++
                  [
                    {:ci, :implementing, :ci_red},
@@ -186,7 +197,8 @@ defmodule Loopctl.Delivery.StageMachine do
                  ] ++
                  @budget_exceeded ++
                  @released ++
-                 @human_resolution ++ @session_escalated ++ @budget_reported ++ @release_escalated
+                 @human_resolution ++
+                 @session_escalated ++ @budget_reported ++ @release_escalated ++ @review_ceiling
 
   # The part of the machine a RUNNER may report over the channel: one definition, from which
   # `runner_transitions/0`'s doc, the wire enums and the published
@@ -411,6 +423,7 @@ defmodule Loopctl.Delivery.StageMachine do
           | :merge_refused
           | :budget_exceeded
           | :budget_reported
+          | :review_ceiling
           | :runner_lost
           | :claim_released
           | :attempts_exhausted

lib/loopctl/dispatches.ex

@@ -840,6 +840,37 @@ defmodule Loopctl.Dispatches do
   def lineage_shares_prefix?([a | _], [b | _]) when a == b, do: true
   def lineage_shares_prefix?(_, _), do: false
 
+  @doc """
+  LINEAGE CEILING (parent half): whether a dispatch may be minted under a parent whose
+  lineage is `parent_lineage`, by a caller whose lineage is `caller_lineage`. The ONE copy,
+  called by `LoopctlWeb.DispatchController` and `Loopctl.Threads.Reviews.place/4`.
+
+  Rejecting only the parentless case would leave the
+  same escape open one step further out: any dispatch in the tenant is enumerable
+  via GET /api/v1/dispatches, so a caller could name a parent under a DIFFERENT root
+  and mint itself into that unrelated tree. A minted dispatch must therefore descend
+  from the caller's own dispatch — `parent.lineage_path` must have the caller's
+  lineage as a prefix.
+
+  The OPERATOR key may parent anywhere in its tenant: it is not inside any tree, so
+  it cannot escape one. The same reasoning covers a []-lineage NON-operator (a legacy
+  env-var key): it has no subtree to step outside of, and the ceiling exists to stop a
+  principal escaping ITS OWN tree. Refusing it here closed the only remaining mint it
+  had — root minting is already operator-only — leaving a legacy `:orchestrator` key
+  unable to obtain a lineage by any request at all, against the documented deprecation
+  window. Naming a parent gives it a lineage, which SUBJECTS it to the custody gates.
+  A caller that IS inside a tree must stay inside it.
+  """
+  @spec lineage_within_caller?(list(), list(), boolean()) :: boolean()
+  def lineage_within_caller?(_parent_lineage, _caller_lineage, true), do: true
+  def lineage_within_caller?(_parent_lineage, [], false), do: true
+
+  def lineage_within_caller?(parent_lineage, [_ | _] = caller_lineage, false)
+      when is_list(parent_lineage),
+      do: List.starts_with?(parent_lineage, caller_lineage)
+
+  def lineage_within_caller?(_parent_lineage, _caller_lineage, false), do: false
+
   @doc """
   True when the two dispatches lie on ONE root-to-leaf chain — identical, or one
   an ancestor of the other. Siblings are NOT a match.

lib/loopctl/tenants/tier_capabilities.ex

@@ -168,6 +168,9 @@ defmodule Loopctl.Tenants.TierCapabilities do
       # US-45.1 — a story's change thread: the checkpoints its claimant reports, fenced on the
       # claim, are the record the merge gate will read.
       "LoopctlWeb.ThreadController",
+      # US-45.3 — review on a thread: placing a review mints the reviewer's dispatch, and its
+      # findings, verdicts and the claimant's fixes decide the round count and its ceiling.
+      "LoopctlWeb.ThreadReviewController",
       # LCP-1 §9.2 — TenantController mounts RequireHumanAnchor on
       # :register_owner_key (the custody owner key is the root of trust).
       "LoopctlWeb.TenantController"
@@ -213,7 +216,7 @@ defmodule Loopctl.Tenants.TierCapabilities do
   # what it is for — the mint is how it gets a lineage to claim under. Modules are STRINGS for
   # symmetry with the controller map, though these carry no cross-layer dependency.
   @gated_contexts %{
-    chain_of_custody: ["Loopctl.Delivery.Placement"]
+    chain_of_custody: ["Loopctl.Delivery.Placement", "Loopctl.Threads.Reviews"]
   }
 
   @learn_more "https://loopctl.com/wiki/chain-of-custody"