diff --git a/.claude/skills/chain-of-custody/SKILL.md b/.claude/skills/chain-of-custody/SKILL.md index 40b2f2dee..14b8ae40f 100644 --- a/.claude/skills/chain-of-custody/SKILL.md +++ b/.claude/skills/chain-of-custody/SKILL.md @@ -22,10 +22,10 @@ gated, so escalating to a bigger key never clears a 403 there: | Endpoint | Gate | |----------|------| -| `verify` / `reject` / `force_unclaim` / `verify_all` | `exact_role: :orchestrator` (`story_verification_controller.ex:28-29`) — a superadmin key CANNOT verify | -| `contract` / `report` | `exact_role: [:agent, :orchestrator]` (`story_status_controller.ex:26-30`) — a human user key CANNOT report | +| `verify` / `reject` / `force_unclaim` / `verify_all` | `exact_role: :orchestrator` (`story_verification_controller.ex:30-31`) — a superadmin key CANNOT verify | +| `contract` / `report` | `exact_role: [:agent, :orchestrator]` (`story_status_controller.ex:48-52`) — a human user key CANNOT report | | `claim` / `start` / `request-review` / `unclaim` / `renew-claim` | `exact_role: :agent` (`story_status_controller.ex`) | -| `review-complete` | `exact_role: [:orchestrator, :user]` (`review_record_controller.ex:22-23`) — an agent key never reaches the controller | +| `review-complete` | `exact_role: [:orchestrator, :user]` (`review_record_controller.ex:24-25`) — an agent key never reaches the controller | **Anti-pattern: never "normalize" an `exact_role` custody gate to `role:`.** That is exactly what would let one high-privilege key both implement and report/verify. @@ -42,7 +42,7 @@ Human-rooted operations additionally pass `RequireHumanAnchor` It is not a per-request WebAuthn check — WebAuthn is how a tenant EARNS `:human_anchored`. Keying on tier rather than role is what closes the self-minted-key bypass. **The plug is in NO router pipeline — -it is opt-in per controller** (e.g. `story_verification_controller.ex:35`, `dispatch_controller.ex:16`, +it is opt-in per controller** (e.g. `story_verification_controller.ex:37`, `dispatch_controller.ex:16`, `artifact_report_controller.ex:30`), so a new human-rooted endpoint gets NO trust-tier gate unless you add the plug line yourself. @@ -96,8 +96,8 @@ caller's lineage is always resolved SERVER-SIDE from the authenticating key `get_dispatch_lineage/2`) fails **CLOSED**, and the `assigned_agent_id` equality check runs IN ADDITION to the lineage comparison rather than being short-circuited by it. `verifier_dispatch_id` is written only by the assign-verifier flow (`assign_rotating_verifier/3`, - `progress.ex:732-779`); that write is result-checked, and a failure flags `verifier_needed` plus a - `verifier_not_assigned` audit event (`flag_verifier_needed/5`, `progress.ex:784`) instead of + `progress.ex:755-802`); that write is result-checked, and a failure flags `verifier_needed` plus a + `verifier_not_assigned` audit event (`flag_verifier_needed/5`, `progress.ex:807`) instead of silently leaving the field nil. `request-review` is OPTIONAL, so most stories reach verify with no verifier dispatch — the CALLER-lineage step is what keeps that path lineage-gated. - **report** — `validate_not_self_report/3`. `nil` caller blocked @@ -115,13 +115,36 @@ caller's lineage is always resolved SERVER-SIDE from the authenticating key user-role key also has no `agent_id` and permitting on nil alone let an ANCESTOR of the implementer review its own subtree; then the reviewer's lineage; then plain equality. That nil permit has THREE parts, all of which must change - together: (a) the `exact_role: [:orchestrator, :user]` plug (`review_record_controller.ex:22-23`), + together: (a) the `exact_role: [:orchestrator, :user]` plug (`review_record_controller.ex:24-25`), which 403s an agent key before the controller runs — so the `:agent` branch of the controller cond is unreachable today; (b) `LoopctlWeb.ReviewRecordController.create/2` - (`lib/loopctl_web/controllers/review_record_controller.ex:92-116`), which rejects an orchestrator + (`lib/loopctl_web/controllers/review_record_controller.ex:125-151`), which rejects an orchestrator key with no `agent_id` and passes a literal `nil` (there is NO sentinel) for a user key; (c) the `Progress` permit itself. +**chg-dispatched reviews (US-47.5) — `Loopctl.Changes.ReviewCustody`.** On a story with a change +binding, review-complete additionally demands (after the three gates above, never instead) a +reviewer whose LIVE lineage is a STRICT descendant of the chg root of the source that delivered +the change (`chg_reviewer_required`; the root key too), a `commit_sha` that is an applied +`submitted` revision of the CURRENT binding (`commit_sha_not_bound`), and one verdict per reviewer +dispatch — a unique index, not a read (`chg_review_already_recorded`). Its mirror: a caller whose +MINTING lineage is rooted at any registered `chg_root_dispatch_id` (live or revoked source, either +mode) is refused `chg_root_cannot_verify` on verify / reject / verify-all / bulk verify+reject / +backfill / bulk mark-complete and `chg_root_review_only` on claim / bulk claim / report, before +any story is read. MINTING, because an expired chg key's live lineage is `[]` and reject admits +`[]`. Neither refusal records a custody violation. The lineage gate always passes a chg +reviewer, so it proves only that chg-server MINTED the key — separation of the job from the +implementer is chg-server's to construct (PRD §6). A landing whose revision (`event.sha`) no chg +review names is escalated over the control-only `:review_missing` in the landed event's own +transaction (`EventEffects.after_move/5`), FAIL CLOSED: a refused hold rolls the landing back +so chg redelivers it. The verify half is ONE function every path to `verified` calls — +verify, verify-all, bulk verify, backfill, bulk mark-complete — +`ReviewCustody.ensure_landed_review/3`: on a chg story it refuses `review_missing_escalated` +while the stage is escalated over `:review_missing`, and `chg_review_not_for_landed_revision` +unless a chg review names the LANDED revision (the `sha` of the newest applied `landed` event +of the current binding). Escalation in general does NOT block verify; only this edge does. +Neither refusal records a custody violation. + **Custody-orphan predicates are the ONLY guard for the never-dispatched case.** The DB CHECK `stories_reported_done_requires_agent` is satisfied whenever `implementer_dispatch_id IS NULL`, which is exactly the orphaned shape — do not delete `custody_orphaned?/1` or `custody_unattributed?/1` as @@ -248,9 +271,9 @@ correct behavior; do not add a workaround. a read-then-write. **Enforcement is conditional — this is the deprecation seam.** `Progress.maybe_consume_cap/6` -(`progress.ex:540-563`) is what actually gates the custody ops: a `nil` `cap_id` is rejected with +(`progress.ex:563-586`) is what actually gates the custody ops: a `nil` `cap_id` is rejected with `:missing_capability` **only for tenants that have an audit key** (`tenant_has_audit_key?/1`, -`progress.ex:812-817`); a pre-v2 (keyless) tenant returns `{:ok, :pre_v2_tenant}` and the operation +`progress.ex:835-840`); a pre-v2 (keyless) tenant returns `{:ok, :pre_v2_tenant}` and the operation proceeds with NO capability at all. So L1 strength is per-tenant. A REJECTED cap is split by `cap_refusal/4`: only `:invalid_signature` / `:replay` surface as `{:cap_rejected, _}`, which FallbackController answers with a plain 403 — it halts NOTHING and counts toward nothing (see the @@ -297,7 +320,7 @@ exist, but every one descends from the implementer's root — the single-root te not a shortage; its remedy is the operator minting an independently-rooted verifier tree. **Empty-lineage caveat, in BOTH directions.** When the implementer dispatch cannot be loaded, -`assign_rotating_verifier/3` passes `[]` (`progress.ex:732-736`), and with `[]` the rejection is +`assign_rotating_verifier/3` passes `[]` (`progress.ex:755-759`), and with `[]` the rejection is inert — selection can then pick a same-lineage (even the implementer's own) dispatch. The verify-time comparison is fail-closed on an empty lineage, so this is caught at verify rather than at selection; do not "simplify" either half. diff --git a/CHANGELOG.md b/CHANGELOG.md index 9474162a2..a4d08aa24 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -81,6 +81,30 @@ All notable changes to loopctl are documented here. Migration `20261019160000_add_change_stage_mapping` adds `story_stages.trunk_verified_sha` and the `change_revert_issues` table; additive, no deploy ordering. Each chained stage retraction now also names `release_id` and `trunk_verified_sha` under `retracted`. +- **Review custody for chg-dispatched reviews (US-47.5).** On a story bound to a chg change, + `POST /api/v1/stories/:id/review-complete` accepts the review only from a key whose dispatch + is a strict descendant of the intake source's `chg_root_dispatch_id` (409 + `chg_reviewer_required`, chg-server's root key included), requires a new body field + `commit_sha` naming a submitted revision of the story's current change (422 + `commit_sha_not_bound`), and records one verdict per reviewer dispatch (409 + `chg_review_already_recorded`, held by a unique index). These run after the existing custody + gates, whose codes are unchanged; on any other story `commit_sha` is ignored and not stored. + Review records now serialise `commit_sha` and `reviewer_dispatch_id`. A chg story that + reaches `merged` with no such review for its landed revision is escalated in the same + transaction over a new control-only edge `review_missing` (`merged -> escalated`; the merge + stands) and waits for a human; if that escalation cannot be written the whole `landed` event + rolls back (503 `busy` on contention, else 500 `change_event_failed`, logged and counted as + `[:loopctl, :changes, :review_hold_refused]`) and chg redelivers it, so a merge never commits + without its hold. Such a story is never verified: `POST /stories/:id/verify`, + `/epics/:id/verify-all`, `/stories/bulk/verify`, `/stories/:id/backfill` and + `/stories/bulk/mark-complete` refuse a story bound to a chg change with 409 + `review_missing_escalated` while it is escalated over `review_missing` (a human resolves it + first), and with 409 `chg_review_not_for_landed_revision` unless a chg review names its + LANDED revision — the revision commit of the newest applied `landed` event of its current + change (verify-all and the bulk paths report either per story). Stories with no change + binding are answered exactly as before. Migration `20261019170000_add_chg_review_custody_to_review_records` + adds the two nullable columns, a CHECK that they are set together, and the unique index; + additive, no deploy ordering. MCP: loopctl-mcp-server 3.3.0. - **Intake-source mode `chg` (US-47.2).** `POST /api/v1/intake/sources` and `PATCH /api/v1/intake/sources/:id` accept `mode: "chg"` beside `pr`, plus two fields a `chg` source must carry: `change_events_secret` (the HMAC key chg-server will sign its event @@ -419,6 +443,18 @@ All notable changes to loopctl are documented here. ### Changed +- **chg-server's keys never certify or implement (US-47.5).** A key whose MINTING lineage + (revoked or not) is rooted at a dispatch any intake source of the tenant registers as + `chg_root_dispatch_id` — live or revoked source, either mode — is refused 409 + `chg_root_cannot_verify` by `POST /stories/:id/verify`, `/reject` (exempt from + `caller_lineage_required` only), `/backfill`, `POST /epics/:id/verify-all` and + `POST /stories/bulk/verify`, `/bulk/reject` and `/bulk/mark-complete` (the bulk paths and + verify-all refuse the whole call), and 409 `chg_root_review_only` by `POST /stories/:id/claim`, + `/report` and `/bulk/claim`. No other caller's answer changes. +- **`POST /api/v1/stories/:id/review-complete` answers `caller_lineage_required` as the + documented 409.** The controller had no clause for it, so a key no dispatch minted reviewing + dispatch-minted work got a 500. + - **The app's HTTP listener no longer serves HTTP/2.** Bandit now closes a connection that opens with the HTTP/2 preface, which keeps CVE-2026-105659 (HIGH, Bandit's HTTP/2 stack, no patched release) unreachable. Fly is unaffected: its proxy already speaks HTTP/1.1 to diff --git a/CLAUDE.md b/CLAUDE.md index 21b5d072c..19d053265 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -124,10 +124,10 @@ does NOT clear a 403 there: | Endpoint | Gate | Consequence | |----------|------|-------------| -| `verify` / `reject` / `force_unclaim` / `verify_all` | `exact_role: :orchestrator` (`lib/loopctl_web/controllers/story_verification_controller.ex:28-29`) | a `:user` or `:superadmin` key CANNOT verify | -| `contract` / `report` | `exact_role: [:agent, :orchestrator]` (`lib/loopctl_web/controllers/story_status_controller.ex:26-30`) | a human `:user` key CANNOT report | -| `claim` / `start` / `request-review` / `unclaim` | `exact_role: :agent` (`:32-33`) | orchestrator/user keys cannot claim or start | -| `review-complete` | `exact_role: [:orchestrator, :user]` (`lib/loopctl_web/controllers/review_record_controller.ex:22-23`) | an `:agent` key is 403'd BEFORE any controller logic runs | +| `verify` / `reject` / `force_unclaim` / `verify_all` | `exact_role: :orchestrator` (`lib/loopctl_web/controllers/story_verification_controller.ex:30-31`) | a `:user` or `:superadmin` key CANNOT verify | +| `contract` / `report` | `exact_role: [:agent, :orchestrator]` (`lib/loopctl_web/controllers/story_status_controller.ex:48-52`) | a human `:user` key CANNOT report | +| `claim` / `start` / `request-review` / `unclaim` | `exact_role: :agent` (`:58-60`) | orchestrator/user keys cannot claim or start | +| `review-complete` | `exact_role: [:orchestrator, :user]` (`lib/loopctl_web/controllers/review_record_controller.ex:24-25`) | an `:agent` key is 403'd BEFORE any controller logic runs | **Anti-pattern: never convert an `exact_role` custody gate to `role:`.** That is precisely what lets one high-privilege key both implement and report/verify — the failure the product exists to @@ -176,9 +176,9 @@ in order: story that lacks a verifier dispatch. `verifier_dispatch_id` is written only by the assign-verifier flow -(`assign_rotating_verifier/3`, `progress.ex:732-779`); that write is checked, and a failure +(`assign_rotating_verifier/3`, `progress.ex:755-802`); that write is checked, and a failure flags `verifier_needed` plus a `verifier_not_assigned` audit event -(`flag_verifier_needed/5`, `progress.ex:784`) rather than silently leaving the field nil. +(`flag_verifier_needed/5`, `progress.ex:807`) rather than silently leaving the field nil. Because `request-review` is OPTIONAL, a story often reaches verify with no verifier dispatch at all — step 3 is what keeps that path lineage-gated instead of a bare agent-id inequality. @@ -203,11 +203,11 @@ user-role key also carries no `agent_id`, and permitting on nil alone let an ANC implementer rubber-stamp its own subtree without the lineage ever being read. That permit has THREE parts which must change together: 1. the `exact_role: [:orchestrator, :user]` plug - (`lib/loopctl_web/controllers/review_record_controller.ex:22-23`), which 403s an `:agent` key + (`lib/loopctl_web/controllers/review_record_controller.ex:24-25`), which 403s an `:agent` key before any controller code runs — so the `:agent` branch of the controller cond below is unreachable today; 2. `LoopctlWeb.ReviewRecordController.create/2` - (`lib/loopctl_web/controllers/review_record_controller.ex:92-116`), which rejects an + (`lib/loopctl_web/controllers/review_record_controller.ex:125-151`), which rejects an orchestrator key carrying no `agent_id` and passes a literal `nil` (there is NO "human operator" sentinel) for a user key; 3. the `Progress` permit itself. @@ -231,6 +231,46 @@ and whole-map-replaced by `PATCH /api/v1/stories/:id`** — one ordinary request marker and restored the claim → force-unclaim → backfill-to-verified launder. Never add `:lifecycle_entered_at` to a changeset `cast` list. +**chg-dispatched reviews (US-47.5)** — `Loopctl.Changes.ReviewCustody` +(`lib/loopctl/changes/review_custody.ex`). A story with a change binding is a chg story, and +its chg root is the `chg_root_dispatch_id` of the one intake source that delivered the bound +change's events (any other count: no root, nobody qualifies). On such a story +`record_review/4` runs `chg_review/4` (`lib/loopctl/progress.ex:2458`) AFTER +`validate_not_self_review/3`, never instead: the reviewer's LIVE lineage must be a STRICT +descendant of the root (`chg_reviewer_required`, the root's own key included), then +`commit_sha` must be the sha of an APPLIED `submitted` event of the CURRENT binding +(`commit_sha_not_bound`); one reviewer dispatch records one verdict, held by the unique index +`review_records_reviewer_dispatch_uidx` (`chg_review_already_recorded`). On any other story +`commit_sha` is not stored, so a review recorded before binding never counts as a chg review. +The other half: a caller whose MINTING lineage (`Dispatches.minting_lineage_for_api_key/2`, +revoked or not — an expired chg key's live lineage is `[]`, which reject would accept) is +rooted at a `chg_root_dispatch_id` ANY intake source of the tenant names, live or revoked, +either mode, is refused `chg_root_cannot_verify` by verify, reject, verify-all, bulk +verify/reject, backfill and bulk mark-complete (`refuse_chg_root_verifier/2`, +`progress.ex:2611`; `BulkOperations.refuse_chg_root/3`, `bulk_operations.ex:389`) and +`chg_root_review_only` by claim, bulk claim and report (`refuse_chg_root_actor/3`, +`progress.ex:272`). Both halves read the lineage through ONE function, +`ReviewCustody.caller_lineage/2` (minting lineage, else the call's live one), and the +controllers resolve it through ONE helper, `LoopctlWeb.CustodyLineage.chg_root_opts/1`. The refusal is judged before any story is read, so no other caller's +answer changes, and it records no custody violation. What the chg gate does NOT prove: every +chg reviewer key sits under chg's root and outside every implementer's chain, so lineage +always passes it — loopctl proves chg-server minted the key, and chg-server is trusted to run +the job apart from the implementer (PRD §6). A landing with no chg review for its landed +revision (`event.sha`, the revision commit — never `data.merge_sha`) is escalated in the +landed event's own transaction over the control-only edge `:review_missing` +(`EventEffects.after_move/5`, `lib/loopctl/changes/event_effects.ex:493`), and that hold +fails CLOSED: a refused escalation rolls the whole landing back (`:busy`, or +`change_event_failed`, logged and counted) so chg redelivers it — the merge never commits +without its hold. Such a story is never verified: every path that reaches `verified` — +verify and verify-all through `chg_landed_review/2` (`progress.ex:2698`), bulk verify +through `process_verify/3` (`bulk_operations.ex:421`), backfill and bulk mark-complete +through `guard_backfill/2` (`progress.ex:2792`) — calls one function, +`ReviewCustody.ensure_landed_review/3` (`lib/loopctl/changes/review_custody.ex:215`). It refuses a chg story with +`review_missing_escalated` while its stage is escalated over `:review_missing` (a human +resolves it) and with `chg_review_not_for_landed_revision` unless a chg review names its +LANDED revision: the `sha` of the newest APPLIED `landed` event of its current binding. A +story with no binding passes untouched. + **The MCP server must NEVER hold both implementer and reviewer keys in the same process.** The 409 errors are the system working correctly — do not add workarounds. ### Before Changing Any Role Requirement diff --git a/docs/user_stories/epic_47_change_protocol/PRD.md b/docs/user_stories/epic_47_change_protocol/PRD.md index f34ab036c..bb570673a 100644 --- a/docs/user_stories/epic_47_change_protocol/PRD.md +++ b/docs/user_stories/epic_47_change_protocol/PRD.md @@ -274,7 +274,9 @@ PR-shaped. For a `mode: :chg` source it reads instead: - the bound change's `landed` sha must be an ancestor of the repo's `verified/master` (compare API, by sha); - no `reverted` event may follow it; -- a `review_records` row must exist whose `commit_sha` is the landed revision (§6). +- a `review_records` row must exist whose `commit_sha` is the landed revision (§6) — enforced + on every verify path by 47.5 (`chg_review_not_for_landed_revision`), so 47.6 adds only the + two trunk checks above. The verifier is whoever verifies stories in the delivery loop today. `verify_story` keeps every existing gate (exact `:orchestrator`, lineage, recorded separation) and adds §6's @@ -303,7 +305,11 @@ chg must change is filed on mkreyman/chg. | default | where to overrule | effect of overruling | |---|---|---| | The change protocol lives in chg-server, not loopctl (chg design §5, §10) | here, or chg `docs/design.md` §10 | loopctl absorbs chg phase 2: this epic grows to ledger + lander + review dispatch, and landing inherits loopctl's outages | -| An auto-approved story-bound change escalates `review_missing` rather than verifying | 47.5 | a story could reach `verified` with no reviewer, which is a custody gap | +| An auto-approved story-bound change escalates `review_missing` rather than verifying, and every verify path (verify, verify-all, bulk verify, backfill, bulk mark-complete) refuses a chg story while it is escalated over `review_missing` (409 `review_missing_escalated`) and unless a chg review names its landed revision — the sha of the newest applied `landed` event of its current binding (409 `chg_review_not_for_landed_revision`) | 47.5 | a story could reach `verified` with no reviewer, which is a custody gap | +| A root counts as chg's for `chg_root_cannot_verify` while ANY intake source of the tenant names it, live or revoked, `chg` or `pr` mode, judged on the caller's MINTING lineage (`Loopctl.Changes.ReviewCustody`) | 47.5 | narrowing to live `chg` sources lets a revoked or switched source's reviewer tree verify; judging the live lineage lets an expired chg key reject | +| chg-rooted keys are also refused `claim`, bulk claim and `report` (`chg_root_review_only`) | 47.5 | an agent-role child of chg's root could claim a story and a sibling review it — siblings pass the lineage gate | +| `review_complete` keeps requiring `reported_done`, so chg's reviewer must record after the implementer reports | 47.5 | a review recorded before the report is refused 422 `story_not_reported_done`, and a landing with none escalates `review_missing` | +| The landing judges the review that exists WHEN `landed` applies; one recorded after escalates anyway | 47.5 | a human resolves it from `escalated`; the alternative is a re-check that un-escalates, which no control edge allows | | `reverted` returns the story to `implementing`, keeping its claim; after `done` it opens a new story through intake | 47.4 | alternative: release to `queued` for a fresh dispatch | ## 10. Keep / delete verdict on every piece #984 lists diff --git a/docs/user_stories/epic_47_change_protocol/us_47.5.json b/docs/user_stories/epic_47_change_protocol/us_47.5.json index e5bbfb304..fe4e91406 100644 --- a/docs/user_stories/epic_47_change_protocol/us_47.5.json +++ b/docs/user_stories/epic_47_change_protocol/us_47.5.json @@ -32,7 +32,7 @@ }, { "id": "AC-47.5.5", - "description": "A story that reaches merged with no review_records row for its landed revision escalates over a new control-only edge :review_missing (merged to escalated) through Escalations.escalate_as_control/3, and is never verified." + "description": "A story that reaches merged with no review_records row for its landed revision escalates over a new control-only edge :review_missing (merged to escalated) through Escalations.escalate_as_control/3, in the landed event's own transaction; if that escalation cannot be written the landed event rolls back (503 busy on contention, else 500 change_event_failed) so chg redelivers it. The story is never verified: verify_story, verify_all, bulk verify, backfill and bulk mark-complete refuse a story bound to a chg change with 409 review_missing_escalated while its stage is escalated over :review_missing and unresolved, and with 409 chg_review_not_for_landed_revision unless a review_records row with a reviewer dispatch names its landed revision (the sha of the newest applied landed event of its current binding). A story with no change binding is answered exactly as before." } ], "test_cases": [ @@ -123,6 +123,28 @@ "expected_results": [ "escalated over :review_missing" ] + }, + { + "id": "TC-47.5.6", + "verifies": [ + "AC-47.5.5" + ], + "name": "escalated or unreviewed landing is never verified", + "type": "integration", + "preconditions": [ + "a chg story escalated over :review_missing", + "a chg story whose only review names an earlier revision" + ], + "steps": [ + "verify_story", + "verify_all", + "bulk verify", + "backfill" + ], + "expected_results": [ + "409 review_missing_escalated", + "409 chg_review_not_for_landed_revision" + ] } ], "technical_notes": [ diff --git a/docs/user_stories/epic_47_change_protocol/us_47.6.json b/docs/user_stories/epic_47_change_protocol/us_47.6.json index 7d01b88f2..17098616f 100644 --- a/docs/user_stories/epic_47_change_protocol/us_47.6.json +++ b/docs/user_stories/epic_47_change_protocol/us_47.6.json @@ -16,7 +16,7 @@ "acceptance_criteria": [ { "id": "AC-47.6.1", - "description": "For a :chg source, the CI-evidence input passes only when the current binding's landed sha is an ancestor of verified/master (compare by sha), no reverted event follows it, and a review_records row names that sha." + "description": "For a :chg source, the CI-evidence input passes only when the current binding's landed sha is an ancestor of verified/master (compare by sha) and no reverted event follows it. That a review_records row names the landed revision is US-47.5's verify gate (AC-47.5.5, 409 chg_review_not_for_landed_revision), which already runs on every verify path; it is not re-implemented here." }, { "id": "AC-47.6.2", diff --git a/docs/user_stories/epic_47_change_protocol/us_47.8.json b/docs/user_stories/epic_47_change_protocol/us_47.8.json index 1780b644c..e5cbbd651 100644 --- a/docs/user_stories/epic_47_change_protocol/us_47.8.json +++ b/docs/user_stories/epic_47_change_protocol/us_47.8.json @@ -29,6 +29,10 @@ { "id": "AC-47.8.4", "description": "The result, with the 20 story ids, is recorded as a KB finding and posted on the loopctl channel." + }, + { + "id": "AC-47.8.5", + "description": "The pilot's implementing session reports the story done (report_story) when it runs chg submit, before chg dispatches review: review_complete requires reported_done (PRD section 9, US-47.5), so a review chg records before the report is refused 422 story_not_reported_done and the landing then escalates review_missing. The pilot measures how many landings escalate review_missing and why." } ], "test_cases": [ diff --git a/lib/loopctl/api_spec/messages.ex b/lib/loopctl/api_spec/messages.ex index 6ff6660bd..69a34a6af 100644 --- a/lib/loopctl/api_spec/messages.ex +++ b/lib/loopctl/api_spec/messages.ex @@ -31,4 +31,76 @@ defmodule Loopctl.ApiSpec.Messages do "threshold, or it could not be measured. Nothing from this request was " <> "enqueued. Retry after #{retry_after_seconds} seconds." end + + # ── US-47.5: review custody for chg-dispatched reviews (Epic 47 PRD §6) ────────────────── + # Each is rendered by `LoopctlWeb.FallbackController` AND quoted in the `operation/2` specs + # of the endpoints that can return it, so the two cannot drift. + + @doc "The 409 `chg_reviewer_required` message (review-complete on a chg story)." + @spec chg_reviewer_required() :: String.t() + def chg_reviewer_required do + "This story is bound to a chg change, so its review is accepted only from a key whose " <> + "dispatch chg-server minted for the review: a strict descendant of the intake " <> + "source's chg_root_dispatch_id. Neither chg-server's root key nor any key outside " <> + "that subtree can record it." + end + + @doc "The 422 `commit_sha_not_bound` message (review-complete on a chg story)." + @spec commit_sha_not_bound() :: String.t() + def commit_sha_not_bound do + "This story is bound to a chg change, so review-complete requires `commit_sha`: the " <> + "40- or 64-character lowercase hex sha of a submitted revision of the story's current " <> + "change (the revision commit chg reviewed). The sha sent is absent, malformed, or not " <> + "a revision loopctl has applied for that change." + end + + @doc "The 409 `chg_review_already_recorded` message (one verdict per reviewer dispatch)." + @spec chg_review_already_recorded() :: String.t() + def chg_review_already_recorded do + "This reviewer dispatch has already recorded its verdict. chg-server mints one reviewer " <> + "dispatch per (change, revision) review, and each records exactly one; a further " <> + "review needs a new dispatch." + end + + @doc "The 409 `chg_root_cannot_verify` message (verify, reject, verify-all, bulk, backfill)." + @spec chg_root_cannot_verify() :: String.t() + def chg_root_cannot_verify do + "This key descends from a dispatch an intake source registers as chg-server's " <> + "chg_root_dispatch_id. chg-server's keys may review a story and never verify, reject, " <> + "backfill or mark it complete: the process that mints the reviewers must not also " <> + "certify what they reviewed. Verify with an orchestrator key outside chg's tree." + end + + @doc "The 409 `chg_root_review_only` message (claim, bulk claim, report)." + @spec chg_root_review_only() :: String.t() + def chg_root_review_only do + "This key descends from a dispatch an intake source registers as chg-server's " <> + "chg_root_dispatch_id. chg-server's keys may only review: claiming or reporting a " <> + "story from chg's tree would let one tree implement and review the same work." + end + + @doc """ + The 409 `review_missing_escalated` message (verify, verify-all, bulk verify, backfill, bulk + mark-complete on a chg story held over `:review_missing`). + """ + @spec review_missing_escalated() :: String.t() + def review_missing_escalated do + "This story is bound to a chg change that landed with no chg review recorded for its " <> + "landed revision, so its delivery stage is escalated over review_missing. It is not " <> + "verified until a human resolves that escalation (POST /api/v1/stories/:id/stage/" <> + "resolve, MCP resolve_escalation), and then only on a chg review of the landed revision." + end + + @doc """ + The 409 `chg_review_not_for_landed_revision` message (verify, verify-all, bulk verify, + backfill, bulk mark-complete on a chg story). + """ + @spec chg_review_not_for_landed_revision() :: String.t() + def chg_review_not_for_landed_revision do + "This story is bound to a chg change, so it is verified only on a review a chg-minted " <> + "reviewer recorded for its LANDED revision: the revision commit (the envelope sha) of " <> + "the newest applied `landed` event of its current change. No such review exists — the " <> + "change has not landed, or no chg review names that revision (a review of an earlier " <> + "revision, or one recorded off the chg path, does not count)." + end end diff --git a/lib/loopctl/api_spec/schemas.ex b/lib/loopctl/api_spec/schemas.ex index 112cb6128..ecba5b36b 100644 --- a/lib/loopctl/api_spec/schemas.ex +++ b/lib/loopctl/api_spec/schemas.ex @@ -1919,6 +1919,22 @@ defmodule Loopctl.ApiSpec.Schemas do fixes_count: %Schema{type: :integer}, summary: %Schema{type: :string, nullable: true}, completed_at: %Schema{type: :string, format: :"date-time"}, + commit_sha: %Schema{ + type: :string, + nullable: true, + pattern: "^[0-9a-f]{40}([0-9a-f]{24})?$", + description: + "US-47.5: the chg revision this review covers. Set only on a story bound to a " <> + "chg change; null on every other review." + }, + reviewer_dispatch_id: %Schema{ + type: :string, + format: :uuid, + nullable: true, + description: + "US-47.5: the chg reviewer's own dispatch, set with `commit_sha`. One reviewer " <> + "dispatch records one verdict." + }, inserted_at: %Schema{type: :string, format: :"date-time"}, updated_at: %Schema{type: :string, format: :"date-time"} }, diff --git a/lib/loopctl/artifacts/review_record.ex b/lib/loopctl/artifacts/review_record.ex index 19c42213b..4f898e8ac 100644 --- a/lib/loopctl/artifacts/review_record.ex +++ b/lib/loopctl/artifacts/review_record.ex @@ -23,6 +23,14 @@ defmodule Loopctl.Artifacts.ReviewRecord do superseded report no longer qualifies. Set programmatically, never cast. Nullable: legacy pre-migration records are `NULL` and grandfathered as "matches". + - `commit_sha` -- for a story bound to a chg change ONLY (US-47.5, Epic 47 PRD §6): the + revision commit the chg reviewer reviewed, which `Loopctl.Progress.record_review/4` has + checked is the envelope `sha` of a `submitted` event of the story's CURRENT binding. A + landing whose revision no row names escalates over `:review_missing`. Set + programmatically, never cast; `NULL` on every other review. + - `reviewer_dispatch_id` -- the chg reviewer's own dispatch (the leaf of its server-resolved + lineage), written with `commit_sha` and never apart from it. Unique + (`review_records_reviewer_dispatch_uidx`): one reviewer dispatch records one verdict. """ use Loopctl.Schema @@ -42,6 +50,8 @@ defmodule Loopctl.Artifacts.ReviewRecord do :summary, :completed_at, :reviewed_report_at, + :commit_sha, + :reviewer_dispatch_id, :inserted_at, :updated_at ]} @@ -62,6 +72,11 @@ defmodule Loopctl.Artifacts.ReviewRecord do # Set programmatically in Loopctl.Progress.record_review/4 — NOT in cast. field :reviewed_report_at, :utc_datetime_usec + # US-47.5: a chg review's revision and reviewer dispatch. Set programmatically in + # Loopctl.Progress.record_review/4, together or not at all — NOT in cast. + field :commit_sha, :string + field :reviewer_dispatch_id, :binary_id + timestamps() end @@ -102,6 +117,9 @@ defmodule Loopctl.Artifacts.ReviewRecord do |> validate_number(:disproved_count, greater_than_or_equal_to: 0) |> validate_completed_at_not_future() |> validate_findings_math() + |> unique_constraint(:reviewer_dispatch_id, name: :review_records_reviewer_dispatch_uidx) + |> check_constraint(:commit_sha, name: :review_records_commit_sha) + |> check_constraint(:commit_sha, name: :review_records_chg_review_shape) end # Validates that completed_at is not more than 60 seconds in the future diff --git a/lib/loopctl/bulk_operations.ex b/lib/loopctl/bulk_operations.ex index a51552025..b196cd67c 100644 --- a/lib/loopctl/bulk_operations.ex +++ b/lib/loopctl/bulk_operations.ex @@ -19,8 +19,10 @@ defmodule Loopctl.BulkOperations do import Ecto.Query alias Loopctl.AdminRepo + alias Loopctl.ApiSpec.Messages alias Loopctl.Artifacts.VerificationResult alias Loopctl.Audit + alias Loopctl.Changes.ReviewCustody alias Loopctl.Delivery.Stages alias Loopctl.Dispatches alias Loopctl.Progress @@ -56,7 +58,8 @@ defmodule Loopctl.BulkOperations do @spec bulk_claim(Ecto.UUID.t(), [Ecto.UUID.t()], Ecto.UUID.t(), keyword()) :: {:ok, [map()]} | {:error, atom()} def bulk_claim(tenant_id, story_ids, agent_id, opts \\ []) do - with :ok <- validate_batch_size(story_ids) do + with :ok <- validate_batch_size(story_ids), + :ok <- refuse_chg_root(tenant_id, opts, :chg_root_review_only) do actor_id = Keyword.get(opts, :actor_id) actor_label = Keyword.get(opts, :actor_label) sorted_ids = Enum.sort(story_ids) @@ -108,7 +111,8 @@ defmodule Loopctl.BulkOperations do def bulk_verify(tenant_id, stories, orchestrator_agent_id, opts \\ []) do story_ids = Enum.map(stories, &(&1["story_id"] || &1[:story_id])) - with :ok <- validate_batch_size(story_ids) do + with :ok <- validate_batch_size(story_ids), + :ok <- refuse_chg_root(tenant_id, opts, :chg_root_cannot_verify) do actor_id = Keyword.get(opts, :actor_id) actor_label = Keyword.get(opts, :actor_label) @@ -162,7 +166,8 @@ defmodule Loopctl.BulkOperations do def bulk_reject(tenant_id, stories, orchestrator_agent_id, opts \\ []) do story_ids = Enum.map(stories, &(&1["story_id"] || &1[:story_id])) - with :ok <- validate_batch_size(story_ids) do + with :ok <- validate_batch_size(story_ids), + :ok <- refuse_chg_root(tenant_id, opts, :chg_root_cannot_verify) do actor_id = Keyword.get(opts, :actor_id) actor_label = Keyword.get(opts, :actor_label) @@ -229,7 +234,8 @@ defmodule Loopctl.BulkOperations do def bulk_mark_complete(tenant_id, stories, orchestrator_agent_id, opts \\ []) do story_ids = Enum.map(stories, &(&1["story_id"] || &1[:story_id])) - with :ok <- validate_batch_size(story_ids) do + with :ok <- validate_batch_size(story_ids), + :ok <- refuse_chg_root(tenant_id, opts, :chg_root_cannot_verify) do actor_id = Keyword.get(opts, :actor_id) actor_label = Keyword.get(opts, :actor_label) @@ -373,6 +379,29 @@ defmodule Loopctl.BulkOperations do end) end + # US-47.5 (AC-47.5.4): chg-server's keys review and never certify or implement. The WHOLE + # batch is refused, before any story is locked: it is the caller that may not act, on every + # story alike, so a per-story error list would only repeat it. The lineage judged is + # `ReviewCustody.caller_lineage/2`'s — the SAME function, with the SAME fallback, as the + # single-story path that refusal mirrors (`:verifier_lineage` for verify, reject and + # mark-complete as on `Progress.verify_story/4`; `:lineage` for claim as on + # `Progress.claim_story/3`) — so both halves judge one principal identically. + defp refuse_chg_root(tenant_id, opts, :chg_root_cannot_verify = refusal), + do: + ReviewCustody.refuse_chg_root( + tenant_id, + ReviewCustody.caller_lineage(opts, [:verifier_lineage]), + refusal + ) + + defp refuse_chg_root(tenant_id, opts, :chg_root_review_only = refusal), + do: + ReviewCustody.refuse_chg_root( + tenant_id, + ReviewCustody.caller_lineage(opts, [:lineage]), + refusal + ) + # =================================================================== # Private: Individual Story Processing # =================================================================== @@ -400,6 +429,9 @@ defmodule Loopctl.BulkOperations do with :ok <- validate_verify_preconditions(story), :ok <- Progress.ensure_verify_allowed(story, orchestrator_agent_id, ctx.caller_lineage), :ok <- Progress.ensure_review_conducted(tenant_id, story.id, story), + # US-47.5 (AC-47.5.5): the SAME gate `Progress.verify_story/4` runs, on the locked + # story — a chg story is verified only on a review of its landed revision. + :ok <- ReviewCustody.ensure_landed_review(tenant_id, story.id), {:ok, updated} <- apply_verification(story) do create_verification_result(tenant_id, story, orchestrator_agent_id, params) audit_verification(tenant_id, story, updated, actor_id, actor_label, orchestrator_agent_id) @@ -962,6 +994,13 @@ defmodule Loopctl.BulkOperations do "claimable again once a human resolves it to queued (resolve_escalation); a done or " <> "failed one never is" + # US-47.5: each leads with its code, so a bulk caller can branch on it. + defp format_reason(:review_missing_escalated), + do: "review_missing_escalated: " <> Messages.review_missing_escalated() + + defp format_reason(:chg_review_not_for_landed_revision), + do: "chg_review_not_for_landed_revision: " <> Messages.chg_review_not_for_landed_revision() + defp format_reason(:story_rejected), do: "story is rejected; investigate instead of marking it complete" diff --git a/lib/loopctl/changes/event_effects.ex b/lib/loopctl/changes/event_effects.ex index f99e4d66a..309676c44 100644 --- a/lib/loopctl/changes/event_effects.ex +++ b/lib/loopctl/changes/event_effects.ex @@ -29,7 +29,7 @@ defmodule Loopctl.Changes.EventEffects do | `review_findings` | `reviewing -> implementing`, `:review_findings` | | `review_findings`, at `ci` | `ci -> implementing`, `:change_review_findings` | | `review_approved` (`by: model` or `policy`) and `check_passed` on the same revision | `reviewing -> ci`, `:change_ready` | - | `landed` | `ci -> merged`, `:forward`, `merge_sha` = `data.merge_sha` (the new trunk commit) | + | `landed` | `ci -> merged`, `:forward`, `merge_sha` = `data.merge_sha` (the new trunk commit); then `merged -> escalated`, `:review_missing`, when no chg review names the landed revision (US-47.5) | | `deployed` | `merged -> deployed`, `:forward` | | `verified` | none: `trunk_verified_sha` = `data.sha` on the stage row | | `reverted`, at `merged` or `deployed` | `-> implementing`, `:reverted`, keeping the claim | @@ -94,6 +94,7 @@ defmodule Loopctl.Changes.EventEffects do alias Loopctl.Changes.Binding alias Loopctl.Changes.Event alias Loopctl.Changes.RevertIssues + alias Loopctl.Changes.ReviewCustody alias Loopctl.Delivery.Escalations alias Loopctl.Delivery.StageMachine alias Loopctl.Delivery.Stages @@ -120,9 +121,16 @@ defmodule Loopctl.Changes.EventEffects do @type pending :: [{:bind, Changes.pending_bind()} | {:entry, AuditChain.Entry.t()}] - @doc "Applies `event` for `tenant_id`, inside the caller's tenant transaction." - @spec apply_event(Ecto.UUID.t(), Event.t()) :: {:ok, map(), pending()} - def apply_event(tenant_id, %Event{} = event) do + @doc """ + Applies `event` for `tenant_id`, inside the caller's tenant transaction. + + `opts`: `:escalate` — the 3-arity escalation writer, default + `Loopctl.Delivery.Escalations.escalate_as_control/3`; an input for tests only (a refusal + injected where production would need a broken chain or a vanished row), production passes + nothing. + """ + @spec apply_event(Ecto.UUID.t(), Event.t(), keyword()) :: {:ok, map(), pending()} + def apply_event(tenant_id, %Event{} = event, opts \\ []) do story_id = target_story_id(tenant_id, event) locked = lock_target(tenant_id, story_id) {effect, pending} = bind(tenant_id, event) @@ -132,6 +140,9 @@ defmodule Loopctl.Changes.EventEffects do {:ok, effect, pending} %{} = target -> + target = + Map.put(target, :escalate, Keyword.get(opts, :escalate, &escalate_as_control/3)) + {stage, more} = stage_effect(tenant_id, event, target) {:ok, Map.put(effect, "stage", stage), pending ++ more} end @@ -432,7 +443,13 @@ defmodule Loopctl.Changes.EventEffects do {%{"outcome" => "reverted_after_done", "issue" => Atom.to_string(issue)}, []} end - defp move(tenant_id, event, %{story_id: story_id, claim_epoch: epoch}, transition, effects) do + defp move( + tenant_id, + event, + %{story_id: story_id, claim_epoch: epoch} = target, + transition, + effects + ) do {_from, to, edge} = transition opts = @@ -441,8 +458,13 @@ defmodule Loopctl.Changes.EventEffects do case Stages.advance_in_tenant_transaction(tenant_id, story_id, transition, opts) do {:ok, _row, entry} -> - {%{"outcome" => "moved", "to" => Atom.to_string(to), "edge" => Atom.to_string(edge)}, - entries(entry)} + moved = %{ + "outcome" => "moved", + "to" => Atom.to_string(to), + "edge" => Atom.to_string(edge) + } + + after_move(tenant_id, event, target, to, {moved, entries(entry)}) # Only a refusal decided before the database comes back here (an effect value the # machine will not store); the event is recorded with it rather than lost. @@ -451,6 +473,67 @@ defmodule Loopctl.Changes.EventEffects do end end + # ── review custody at the landing (US-47.5, AC-47.5.5) ───────────────────────────────────── + + # A landing is where a missing review becomes a fact: chg lands on its own authority, so the + # merge stands, but a story whose landed revision no chg reviewer recorded a verdict on is + # held for a human over `:review_missing` and never verified without one. Decided HERE, in + # the transaction that entered `merged`, on the revision the event names (`event.sha`, the + # revision commit — never `data.merge_sha`, the trunk commit a review never saw), so the + # merge and its escalation commit together or not at all. A review that commits after this + # read is too late for this landing: the escalation stands and a human resolves it. + # + # FAIL CLOSED: unlike the other escalations here, a refused hold is never folded into the + # event's outcome, because a merged story with no hold is one a later verify would read as + # unremarkable. The whole event rolls back instead — contention as `:busy` (a 503 chg + # redelivers after `retry-after`), anything else as `{:review_hold_refused, reason}`, which + # `Loopctl.Changes.Events` answers `change_event_failed` ("Redeliver it."). That one is + # also logged at error and counted under `[:loopctl, :changes, :review_hold_refused]`, + # because chg will keep redelivering an event that cannot apply until an operator looks. + defp after_move(tenant_id, %Event{kind: "landed"} = event, target, :merged, {moved, pending}) do + if ReviewCustody.reviewed_revision?(Repo, tenant_id, target.story_id, event.sha) do + {Map.put(moved, "review", "recorded"), pending} + else + case escalate_raw(tenant_id, event, target, :review_missing, review_missing_reason(event)) do + {:ok, _row, entry} -> + escalation = %{"outcome" => "escalated", "edge" => "review_missing"} + + {Map.merge(moved, %{"review" => "missing", "review_missing" => escalation}), + pending ++ entries(entry)} + + {:error, reason} -> + review_hold_refused(tenant_id, event, target, reason) + end + end + end + + defp after_move(_tenant_id, _event, _target, _to, result), do: result + + defp review_hold_refused(_tenant_id, _event, _target, :busy), do: Repo.rollback(:busy) + + defp review_hold_refused(tenant_id, event, target, reason) do + Logger.error( + "review_missing hold refused; the landing rolled back and chg will redeliver it: " <> + "tenant_id=#{tenant_id} story_id=#{target.story_id} change_id=#{event.change_id} " <> + "event_seq=#{event.event_seq} reason=#{inspect(reason)}" + ) + + :telemetry.execute([:loopctl, :changes, :review_hold_refused], %{count: 1}, %{ + tenant_id: tenant_id, + reason: reason + }) + + Repo.rollback({:review_hold_refused, reason}) + end + + defp review_missing_reason(%Event{} = event) do + "review_missing: chg landed change #{RevertIssues.safe_id(event.change_id)} revision " <> + "#{event.revision} (#{RevertIssues.safe_sha(event.sha)}) with no review recorded for " <> + "that revision by a reviewer chg-server minted — a policy auto-approval, or a review " <> + "loopctl never accepted. The merge stands; a human decides what the story is now, " <> + "and it is not verified without one." + end + # An edge the machine does not allow from here: a human decides what the story is now. defp outside(tenant_id, event, %{row: %StoryStage{stage: stage}} = target, why) do cond do @@ -467,12 +550,8 @@ defmodule Loopctl.Changes.EventEffects do end end - defp escalate(tenant_id, event, %{story_id: story_id, claim_epoch: epoch}, edge, reason) do - opts = - control_opts(event, epoch) ++ - [edge: edge, reason: reason, payload: event_data(event), in_tenant_transaction: true] - - case Escalations.escalate_as_control(tenant_id, story_id, opts) do + defp escalate(tenant_id, event, target, edge, reason) do + case escalate_raw(tenant_id, event, target, edge, reason) do {:ok, _row, entry} -> {%{"outcome" => "escalated", "edge" => Atom.to_string(edge)}, entries(entry)} @@ -481,6 +560,23 @@ defmodule Loopctl.Changes.EventEffects do end end + defp escalate_raw( + tenant_id, + event, + %{story_id: story_id, claim_epoch: epoch} = target, + edge, + reason + ) do + opts = + control_opts(event, epoch) ++ + [edge: edge, reason: reason, payload: event_data(event), in_tenant_transaction: true] + + Map.get(target, :escalate, &escalate_as_control/3).(tenant_id, story_id, opts) + end + + defp escalate_as_control(tenant_id, story_id, opts), + do: Escalations.escalate_as_control(tenant_id, story_id, opts) + defp entries(nil), do: [] defp entries(entry), do: [{:entry, entry}] diff --git a/lib/loopctl/changes/events.ex b/lib/loopctl/changes/events.ex index bc21f8a26..8d4d5fe15 100644 --- a/lib/loopctl/changes/events.ex +++ b/lib/loopctl/changes/events.ex @@ -121,10 +121,11 @@ defmodule Loopctl.Changes.Events do Receives one webhook delivery for `source_id`. `opts`: `:now` (unix seconds; the clock the window is judged by), `:lock_timeout_ms` - (`Capacity.lock_timeout_ms/1`) and `:before_hold` (a 0-arity function run after the + (`Capacity.lock_timeout_ms/1`), `:before_hold` (a 0-arity function run after the pre-check and before the event's transaction, so a test can commit a rotation or a - suspension in exactly that window). All are inputs for tests only; production passes - nothing. + suspension in exactly that window) and `:escalate` (passed to + `Loopctl.Changes.EventEffects.apply_event/3`). All are inputs for tests only; production + passes nothing. """ @spec receive_push(term(), push_input(), keyword()) :: {:ok, result()} @@ -143,7 +144,7 @@ defmodule Loopctl.Changes.Events do source.tenant_id |> in_held_source(source, opts, fn held -> reauthenticate!(held, input, now) - ingest_one(held, envelope, "push", DateTime.utc_now()) + ingest_one(held, envelope, "push", DateTime.utc_now(), effects_opts(opts)) end) |> finish_push(source, envelope) end @@ -175,7 +176,7 @@ defmodule Loopctl.Changes.Events do source.tenant_id |> in_held_source(source, opts, fn held -> - Enum.reduce(envelopes, {nil, []}, &ingest_pulled_one(held, &1, now, &2)) + Enum.reduce(envelopes, {nil, []}, &ingest_pulled_one(held, &1, now, &2, opts)) end) |> case do {:ok, {applied_seq, pending}} -> @@ -191,8 +192,8 @@ defmodule Loopctl.Changes.Events do end end - defp ingest_pulled_one(source, envelope, now, {applied_seq, pending}) do - case ingest_one(source, envelope, "pull", now) do + defp ingest_pulled_one(source, envelope, now, {applied_seq, pending}, opts) do + case ingest_one(source, envelope, "pull", now, effects_opts(opts)) do {:conflict, _existing} -> log_conflict(source, envelope, "pull") {applied_seq, pending} @@ -376,7 +377,7 @@ defmodule Loopctl.Changes.Events do # Inside the tenant transaction. Answers `{outcome, applied_seq, pending}` or # `{:conflict, existing}`. - defp ingest_one(%Source{} = source, envelope, via, now) do + defp ingest_one(%Source{} = source, envelope, via, now, effects_opts) do key = %{ tenant_id: source.tenant_id, repo: repo_key(source.repo_full_name), @@ -387,7 +388,7 @@ defmodule Loopctl.Changes.Events do case insert_event(source, key, envelope, via, now) do :inserted -> - {cursor, pending} = apply_ready(cursor, []) + {cursor, pending} = apply_ready(cursor, [], effects_opts) touch_cursor(cursor, source, now) outcome = if cursor.applied_seq >= envelope.event_seq, do: :applied, else: :held {outcome, cursor.applied_seq, pending} @@ -468,7 +469,7 @@ defmodule Loopctl.Changes.Events do end # Applies the stored events from `applied_seq + 1` for as long as the next one is there. - defp apply_ready(%Cursor{} = cursor, pending) do + defp apply_ready(%Cursor{} = cursor, pending, effects_opts) do next = cursor.applied_seq + 1 key = Map.take(cursor, [:tenant_id, :repo, :change_id]) @@ -477,13 +478,13 @@ defmodule Loopctl.Changes.Events do {cursor, pending} %Event{} = event -> - more = apply_event(event) - apply_ready(%{cursor | applied_seq: next}, pending ++ more) + more = apply_event(event, effects_opts) + apply_ready(%{cursor | applied_seq: next}, pending ++ more, effects_opts) end end - defp apply_event(%Event{tenant_id: tenant_id} = event) do - {:ok, effect, effects} = EventEffects.apply_event(tenant_id, event) + defp apply_event(%Event{tenant_id: tenant_id} = event, effects_opts) do + {:ok, effect, effects} = EventEffects.apply_event(tenant_id, event, effects_opts) now = DateTime.utc_now() {1, _} = @@ -513,6 +514,9 @@ defmodule Loopctl.Changes.Events do end end + # What of a caller's `opts` reaches `EventEffects.apply_event/3`. + defp effects_opts(opts), do: Keyword.take(opts, [:escalate]) + defp touch_cursor(%Cursor{} = cursor, source, now) do {1, _} = Repo.update_all( diff --git a/lib/loopctl/changes/review_custody.ex b/lib/loopctl/changes/review_custody.ex new file mode 100644 index 000000000..50dccb18d --- /dev/null +++ b/lib/loopctl/changes/review_custody.ex @@ -0,0 +1,320 @@ +defmodule Loopctl.Changes.ReviewCustody do + @moduledoc """ + Review custody for chg-dispatched reviews (US-47.5, Epic 47 PRD §6) — the one place that + says what a chg story is, who may review it, which revision a review names, and whose keys + may never certify anything. + + ## A chg story + + A story with a change binding (`Loopctl.Changes.Binding`). The binding is the truth, never + the project's intake mode: a binding is written only by a `submitted` event a chg source + delivered, and the custody rules below must keep applying to a story whose source was later + switched back to `pr` or revoked. Its **chg root** is the `chg_root_dispatch_id` of the intake + source that delivered the bound change's events. A change delivered through more than one + source, or through a source naming no root, has NO root — and then no reviewer qualifies + (fail closed, logged). + + ## Who may review one (AC-47.5.1) + + Only a key whose dispatch is a STRICT descendant of the story's chg root: its server-resolved + lineage is `[root, ..., self]` with `self != root`. The root's own key and every key outside + that subtree are refused `:chg_reviewer_required`. These checks run AFTER — never instead of — + `Loopctl.Progress`'s own review-complete gates, which are unchanged. + + What this does NOT prove, so nobody reads more into it: every chg reviewer key sits under + chg's root and outside every implementer's chain, so the lineage gate always passes a chg + review whoever ran the job. loopctl proves chg-server minted the reviewer's key; that the job + ran apart from the implementer's session is chg-server's to construct (PRD §6). + + ## Which revision (AC-47.5.2) + + `commit_sha` must be the envelope `sha` of an APPLIED `submitted` event of the story's + CURRENT binding — a revision of the change the story follows now — or the review is refused + `:commit_sha_not_bound`. An event held ahead of a gap is not applied, so the story has not + moved for it, and a review of it waits (chg retries) rather than covering a revision the + stage machine has not seen. + + ## Whose keys never certify (AC-47.5.4) + + A caller whose MINTING lineage (`Loopctl.Dispatches.minting_lineage_for_api_key/2`, revoked + or not) is rooted at a dispatch some intake source of the tenant names as + `chg_root_dispatch_id` — live or revoked source, `chg` or `pr` mode — is refused every + terminal custody act (`:chg_root_cannot_verify`) and the implementer's own acts, claim and + report (`:chg_root_review_only`). The MINTING lineage, not the live one: a chg key whose + dispatch has expired resolves to a live lineage of `[]`, which `reject` would otherwise + accept as an unlineaged caller. Every source, not only live `chg` ones: revoking a source, + or switching it to `pr`, must not hand its reviewer tree the power to verify. An operator + who CLEARS the column on a `pr` source (an audited write, `intake_source_chg_root_set`) + lifts the refusal for that root, which is the one way back. + + ## What may be verified (AC-47.5.5) + + A chg story reaches `verified` — by verify, verify-all, bulk verify, backfill or bulk + mark-complete — only through `ensure_landed_review/3`: never while its delivery stage is + `escalated` over `:review_missing` (a human resolves that first), and never without a chg + review naming its LANDED REVISION. The landed revision is the envelope `sha` (the revision + commit chg reviewed, never `data.merge_sha`, the trunk commit) of the APPLIED `landed` event + with the highest `event_seq` of the story's CURRENT binding's change. A story with no + current binding, or whose change has not landed, has no landed revision and is refused the + same way: nothing chg reviewed is on trunk to certify. + + ## Reads + + `AdminRepo` (or the caller's `Loopctl.Repo` tenant transaction) with an explicit `tenant_id` + predicate on every query — the custody-read convention (`.claude/skills/tenancy-rls`). + """ + + import Ecto.Query + + require Logger + + alias Loopctl.AdminRepo + alias Loopctl.Artifacts.ReviewRecord + alias Loopctl.Changes.Binding + alias Loopctl.Changes.Event + alias Loopctl.Delivery.StageEvent + alias Loopctl.Delivery.StoryStage + alias Loopctl.GitSha + alias Loopctl.Intake.Source + + @typedoc """ + A story bound to a chg change: its CURRENT binding (`nil` only if every binding is + superseded, which no writer produces) and its chg root (`nil` when it cannot be resolved to + exactly one). + """ + @type chg_story :: %{binding: Binding.t() | nil, root_id: Ecto.UUID.t() | nil} + + @typedoc "What a chg review records, programmatically, on its `review_records` row." + @type chg_review :: %{commit_sha: String.t(), reviewer_dispatch_id: Ecto.UUID.t()} + + @doc """ + `:not_chg` for a story with no change binding, else `{:chg, chg_story}`. Reads on `repo` + (default `AdminRepo`), tenant-scoped. + """ + @spec chg_story(Ecto.Repo.t(), Ecto.UUID.t(), Ecto.UUID.t()) :: :not_chg | {:chg, chg_story()} + def chg_story(repo \\ AdminRepo, tenant_id, story_id) do + newest = + repo.one( + from b in Binding, + where: b.tenant_id == ^tenant_id and b.story_id == ^story_id, + order_by: [desc: is_nil(b.superseded_at), desc: b.inserted_at, desc: b.id], + limit: 1 + ) + + case newest do + nil -> + :not_chg + + %Binding{} = binding -> + current = if is_nil(binding.superseded_at), do: binding + {:chg, %{binding: current, root_id: chg_root(repo, tenant_id, binding)}} + end + end + + # The root of the ONE source that delivered this change's events. Any other count is not a + # root anybody can be shown to descend from. + defp chg_root(repo, tenant_id, %Binding{} = binding) do + roots = + repo.all( + from s in Source, + join: e in Event, + on: e.source_id == s.id and e.tenant_id == s.tenant_id, + where: + s.tenant_id == ^tenant_id and e.tenant_id == ^tenant_id and e.repo == ^binding.repo and + e.change_id == ^binding.change_id, + distinct: true, + select: {s.id, s.chg_root_dispatch_id} + ) + + case roots do + [{_source_id, root_id}] when is_binary(root_id) -> + root_id + + other -> + Logger.warning( + "chg_root_unresolved: no single chg root for a bound change; no reviewer qualifies " <> + "tenant_id=#{tenant_id} story_id=#{binding.story_id} change_id=#{binding.change_id} " <> + "sources=#{length(other)}" + ) + + nil + end + end + + @doc """ + Decides a review of a chg story by a caller with `reviewer_lineage` (server-resolved, LIVE) + naming `commit_sha`: `{:ok, chg_review}` to record, `{:error, :chg_reviewer_required}`, or + `{:error, :commit_sha_not_bound}` — in that order, so a caller outside chg's subtree learns + nothing about the change's revisions. + """ + @spec authorize_review(Ecto.UUID.t(), chg_story(), [Ecto.UUID.t()], term()) :: + {:ok, chg_review()} | {:error, :chg_reviewer_required | :commit_sha_not_bound} + def authorize_review(tenant_id, %{} = chg, reviewer_lineage, commit_sha) do + with {:ok, dispatch_id} <- chg_reviewer(chg, reviewer_lineage), + :ok <- bound_revision(tenant_id, chg, commit_sha) do + {:ok, %{commit_sha: commit_sha, reviewer_dispatch_id: dispatch_id}} + end + end + + # A strict descendant of the root: the root first, at least one dispatch after it, and the + # caller's own dispatch last. The root alone (`[root]`) and an empty lineage do not match. + defp chg_reviewer(%{root_id: root}, [root | [_ | _] = below]) when is_binary(root), + do: {:ok, List.last(below)} + + defp chg_reviewer(_chg, _lineage), do: {:error, :chg_reviewer_required} + + defp bound_revision(tenant_id, %{binding: %Binding{} = binding}, commit_sha) do + bound? = + GitSha.valid?(commit_sha) and + AdminRepo.exists?( + from e in Event, + where: + e.tenant_id == ^tenant_id and e.repo == ^binding.repo and + e.change_id == ^binding.change_id and e.kind == "submitted" and + e.sha == ^commit_sha and not is_nil(e.applied_at) + ) + + if bound?, do: :ok, else: {:error, :commit_sha_not_bound} + end + + defp bound_revision(_tenant_id, _chg, _commit_sha), do: {:error, :commit_sha_not_bound} + + @doc """ + Whether `story_id` has a chg review recorded for the revision commit `sha` — what a + `landed` event asks before it lets the merge stand (AC-47.5.5). Reads on `repo`, the event's + own tenant transaction. + """ + @spec reviewed_revision?(Ecto.Repo.t(), Ecto.UUID.t(), Ecto.UUID.t(), String.t()) :: boolean() + def reviewed_revision?(repo, tenant_id, story_id, sha) when is_binary(sha) do + repo.exists?( + from r in ReviewRecord, + where: + r.tenant_id == ^tenant_id and r.story_id == ^story_id and r.commit_sha == ^sha and + not is_nil(r.reviewer_dispatch_id) + ) + end + + def reviewed_revision?(_repo, _tenant_id, _story_id, _sha), do: false + + @doc """ + The verify-side half of AC-47.5.5, called by EVERY path that reaches `verified` (single + verify and verify-all through `Loopctl.Progress.verify_story/4`, bulk verify, backfill and + bulk mark-complete): `:ok` for a story with no change binding — untouched — and for a chg + story only when it is not held over `:review_missing` and a chg review names its landed + revision (see the moduledoc). Otherwise, in this order: + + * `{:error, :review_missing_escalated}` — its stage row is `escalated`, entered over + `:review_missing` and not yet resolved by a human; + * `{:error, :chg_review_not_for_landed_revision}` — no `review_records` row with a + `reviewer_dispatch_id` names the landed revision, or there is no landed revision. + + Reads on `repo` (default `AdminRepo`, the verify transaction's), tenant-scoped. + """ + @spec ensure_landed_review(Ecto.Repo.t(), Ecto.UUID.t(), Ecto.UUID.t()) :: + :ok | {:error, :review_missing_escalated | :chg_review_not_for_landed_revision} + def ensure_landed_review(repo \\ AdminRepo, tenant_id, story_id) do + case chg_story(repo, tenant_id, story_id) do + :not_chg -> + :ok + + {:chg, chg} -> + cond do + review_missing_escalated?(repo, tenant_id, story_id) -> + {:error, :review_missing_escalated} + + reviewed_revision?(repo, tenant_id, story_id, landed_revision(repo, tenant_id, chg)) -> + :ok + + true -> + {:error, :chg_review_not_for_landed_revision} + end + end + end + + # The revision commit of the newest APPLIED `landed` event of the current binding's change. + defp landed_revision(repo, tenant_id, %{binding: %Binding{} = binding}) do + repo.one( + from e in Event, + where: + e.tenant_id == ^tenant_id and e.repo == ^binding.repo and + e.change_id == ^binding.change_id and e.kind == "landed" and + not is_nil(e.applied_at), + order_by: [desc: e.event_seq], + limit: 1, + select: e.sha + ) + end + + defp landed_revision(_repo, _tenant_id, _chg), do: nil + + # The row is `escalated` and the transition that put it there — its newest — was + # `:review_missing`. Nothing moves a row out of `escalated` without a transition + # (`:human_resolution`), so the newest transition of an escalated row is the one into it. + defp review_missing_escalated?(repo, tenant_id, story_id) do + edge = + repo.one( + from e in StageEvent, + join: s in StoryStage, + on: s.story_id == e.story_id and s.tenant_id == e.tenant_id, + where: + e.tenant_id == ^tenant_id and e.story_id == ^story_id and + e.event == "transitioned" and s.stage == :escalated, + order_by: [desc: e.inserted_at, desc: e.lock_version], + limit: 1, + select: e.edge + ) + + edge == "review_missing" + end + + @doc """ + True when `lineage`'s ROOT is a `chg_root_dispatch_id` some intake source of the tenant + names (any status, any mode — see the moduledoc). `[]` is never chg-rooted. + """ + @spec chg_rooted?(Ecto.UUID.t(), [Ecto.UUID.t()]) :: boolean() + def chg_rooted?(_tenant_id, []), do: false + + def chg_rooted?(tenant_id, [root | _]) when is_binary(tenant_id) and is_binary(root) do + AdminRepo.exists?( + from s in Source, where: s.tenant_id == ^tenant_id and s.chg_root_dispatch_id == ^root + ) + end + + def chg_rooted?(_tenant_id, _lineage), do: false + + @doc """ + `:ok`, or `{:error, refusal}` when the caller with minting `lineage` is chg-rooted. `refusal` + is `:chg_root_cannot_verify` (the default; verify, reject, verify-all, bulk verify/reject, + backfill, bulk mark-complete) or `:chg_root_review_only` (claim, bulk claim, report). + """ + @spec refuse_chg_root(Ecto.UUID.t(), [Ecto.UUID.t()], atom()) :: :ok | {:error, atom()} + def refuse_chg_root(tenant_id, lineage, refusal \\ :chg_root_cannot_verify) + when refusal in [:chg_root_cannot_verify, :chg_root_review_only] do + if chg_rooted?(tenant_id, lineage) do + Logger.warning( + "#{refusal}: a chg-server key attempted a custody act it may never perform " <> + "tenant_id=#{tenant_id} lineage_root=#{hd(lineage)}" + ) + + {:error, refusal} + else + :ok + end + end + + @doc """ + The lineage the chg-root refusal judges, from a custody call's `opts`: + `:caller_minting_lineage` (the controllers resolve it with + `Loopctl.Dispatches.minting_lineage_for_api_key/2`), else the first of `fallbacks` the opts + carry — the call's own live lineage — else `[]`. Never request data. + """ + @spec caller_lineage(keyword(), [atom()]) :: [Ecto.UUID.t()] + def caller_lineage(opts, fallbacks) do + Enum.find_value([:caller_minting_lineage | fallbacks], [], fn key -> + case Keyword.get(opts, key) do + [_ | _] = lineage -> lineage + _ -> nil + end + end) + end +end diff --git a/lib/loopctl/delivery/stage_machine.ex b/lib/loopctl/delivery/stage_machine.ex index 79e4e59bb..c80d75940 100644 --- a/lib/loopctl/delivery/stage_machine.ex +++ b/lib/loopctl/delivery/stage_machine.ex @@ -118,6 +118,11 @@ defmodule Loopctl.Delivery.StageMachine do story at `implementing`). Trunk holds a change the story did not expect, and only a human can say what the story is now. It is NOT taken from `escalated`: a story already there is waiting on a human, and `escalated` leaves only over `:human_resolution` + - `:review_missing` — merged -> escalated (US-47.5, Epic 47 PRD §6), taken in the SAME event + transaction as the `landed` that entered `merged`, when no chg review is recorded for the + landed revision (a policy auto-approval, or a review loopctl never accepted). chg lands on + its own authority, so the merge stands and nothing is cleared; the story is held for a + human and is never verified without one `done` and `failed` are terminal; `escalated` is terminal except for `:human_resolution`. """ @@ -206,7 +211,7 @@ defmodule Loopctl.Delivery.StageMachine do # The change protocol (US-47.4). The control-side escalations a change event can decide, # taken through `Loopctl.Delivery.Escalations.escalate_as_control/3` and nothing else. - @control_escalations [:change_outside_lifecycle, :reverted_after_verify] + @control_escalations [:change_outside_lifecycle, :reverted_after_verify, :review_missing] # From every stage a change event can find a story at that is not terminal: the event is # recorded either way, and a story `done` or `failed` has nowhere to go. NOT from @@ -230,7 +235,10 @@ defmodule Loopctl.Delivery.StageMachine do {:reviewing, :ci, :change_ready}, {:ci, :reviewing, :change_resubmitted}, {:ci, :implementing, :change_review_findings}, - {:verified, :escalated, :reverted_after_verify} + {:verified, :escalated, :reverted_after_verify}, + # US-47.5: a landing with no chg review recorded for its revision. + # `escalated` leaves over `:human_resolution`, so it is not absorbing. + {:merged, :escalated, :review_missing} ] ++ @reverted ++ @change_outside_lifecycle @@ -631,8 +639,8 @@ defmodule Loopctl.Delivery.StageMachine do def release_escalation_edges, do: @release_escalations @doc """ - The control-side escalations a chg change event decides (US-47.4): `:change_outside_lifecycle` - and `:reverted_after_verify`. Taken through + The control-side escalations a chg change event decides: `:change_outside_lifecycle` and + `:reverted_after_verify` (US-47.4), and `:review_missing` (US-47.5). Taken through `Loopctl.Delivery.Escalations.escalate_as_control/3`, never by a runner. """ @spec control_escalation_edges() :: [edge()] @@ -641,7 +649,7 @@ defmodule Loopctl.Delivery.StageMachine do @doc """ The change-protocol edges (US-47.4): `:change_ready`, `:change_resubmitted`, `:change_review_findings`, `:reverted`, `:reverted_after_verify` and - `:change_outside_lifecycle`. Control-only — none is runner-reportable, and + `:change_outside_lifecycle`, plus US-47.5's `:review_missing`. Control-only — none is runner-reportable, and `Loopctl.Delivery.Stages.advance/4` refuses every one of them: only the change-event seam (`Stages.advance_in_tenant_transaction/4`) takes them. """ diff --git a/lib/loopctl/progress.ex b/lib/loopctl/progress.ex index 0b8feb990..893a5afc6 100644 --- a/lib/loopctl/progress.ex +++ b/lib/loopctl/progress.ex @@ -22,6 +22,7 @@ defmodule Loopctl.Progress do alias Loopctl.Audit alias Loopctl.Audit.AuditLog alias Loopctl.Capabilities + alias Loopctl.Changes.ReviewCustody alias Loopctl.Delivery.DispatchLease alias Loopctl.Delivery.RunnerStages alias Loopctl.Delivery.Stages @@ -251,10 +252,32 @@ defmodule Loopctl.Progress do (`Loopctl.Delivery.Stages.held_story_ids/2`): `escalated`, which is claimable again once `Loopctl.Delivery.Escalations.resolve/3` sends it to `queued`, or `done` / `failed`, which never are + - `{:error, :chg_root_review_only}` (US-47.5) if the caller is chg-rooted + (`Loopctl.Changes.ReviewCustody`): chg-server's keys may review and nothing else """ @spec claim_story(Ecto.UUID.t(), Ecto.UUID.t(), keyword()) :: {:ok, Story.t()} | {:error, atom() | {:invalid_transition, map()}} def claim_story(tenant_id, story_id, opts \\ []) do + # US-47.5: chg-server's keys review and never implement — a chg-rooted agent-role CHILD + # clears `exact_role: :agent`, and claiming would let one chg tree implement a story and + # review it as a sibling, which the lineage gates read as separation. + with :ok <- refuse_chg_root_actor(tenant_id, opts, [:lineage]) do + do_claim_story(tenant_id, story_id, opts) + end + end + + # US-47.5: the claim and report half of the chg-root refusal (`refuse_chg_root_verifier/2` + # is the certification half), on the caller's MINTING lineage when the controller resolved + # it, else the call's own live lineage from `fallbacks`. + defp refuse_chg_root_actor(tenant_id, opts, fallbacks), + do: + ReviewCustody.refuse_chg_root( + tenant_id, + ReviewCustody.caller_lineage(opts, fallbacks), + :chg_root_review_only + ) + + defp do_claim_story(tenant_id, story_id, opts) do agent_id = Keyword.get(opts, :agent_id) actor_id = Keyword.get(opts, :actor_id) actor_label = Keyword.get(opts, :actor_label) @@ -1048,6 +1071,7 @@ defmodule Loopctl.Progress do failure that fails closed (LCP-1 §7.5) - `{:error, :stale_claim_epoch}` if `:claim_epoch` is given and is not the story's current epoch (#803); omitting it skips the check + - `{:error, :chg_root_review_only}` (US-47.5) if the caller is chg-rooted; checked first - `{:error, %Ecto.Changeset{}}` if artifact validation fails """ @spec report_story(Ecto.UUID.t(), Ecto.UUID.t(), keyword(), map() | nil) :: @@ -1061,7 +1085,8 @@ defmodule Loopctl.Progress do # SAME guard the standalone TokenUsage.create_report/3 path runs. A # cross-tenant (or non-existent) skill_version_id is rejected here, so no # report row is ever stored. The 3-tuple maps to a 422 via FallbackController. - with :ok <- validate_token_usage_ownership(tenant_id, token_usage_params) do + with :ok <- refuse_chg_root_actor(tenant_id, opts, [:reporter_lineage, :lineage]), + :ok <- validate_token_usage_ownership(tenant_id, token_usage_params) do do_report_story(tenant_id, story_id, opts, artifact_params) end end @@ -2364,21 +2389,29 @@ defmodule Loopctl.Progress do - `{:error, :missing_assigned_agent}` on a custody-orphaned story (fails closed) - `{:error, :unresolvable_dispatch_lineage}` if the story's declared implementer dispatch cannot be resolved (lineage-integrity failure, fails closed — LCP-1 §7.5) + - `{:error, :chg_reviewer_required}` (US-47.5) on a story bound to a chg change, when the + reviewer's dispatch is not a STRICT descendant of the change's chg root + (`Loopctl.Changes.ReviewCustody`) — the root's own key included + - `{:error, :commit_sha_not_bound}` (US-47.5) on such a story, when `params["commit_sha"]` + is absent, malformed, or not the sha of a `submitted` revision of its current binding + - `{:error, :chg_review_already_recorded}` (US-47.5) when that reviewer dispatch already + recorded a verdict - `{:error, %Ecto.Changeset{}}` on validation failure + + Every chg refusal runs AFTER the existing ones, so a non-chg story is answered exactly as + before and a chg story still meets `self_review_blocked`, `caller_lineage_required` and + `unresolvable_dispatch_lineage` first. """ @spec record_review(Ecto.UUID.t(), Ecto.UUID.t(), map(), keyword()) :: {:ok, ReviewRecord.t()} | {:error, atom() | Ecto.Changeset.t()} def record_review(tenant_id, story_id, params, opts \\ []) do reviewer_agent_id = Keyword.get(opts, :reviewer_agent_id) + reviewer_lineage = Keyword.get(opts, :reviewer_lineage, []) with {:ok, story} <- fetch_story_for_review(tenant_id, story_id), :ok <- validate_story_reported_done(story), - :ok <- - validate_not_self_review( - story, - reviewer_agent_id, - Keyword.get(opts, :reviewer_lineage, []) - ) do + :ok <- validate_not_self_review(story, reviewer_agent_id, reviewer_lineage), + {:ok, chg_review} <- chg_review(tenant_id, story, reviewer_lineage, params) do attrs = build_review_attrs(params) changeset = @@ -2390,7 +2423,11 @@ defmodule Loopctl.Progress do # snapshotting the story's CURRENT reported_done_at. Programmatic (never # cast from client input). verify_story/4 + bulk_verify later require # this to match the story's reported_done_at at verify time. - reviewed_report_at: story.reported_done_at + reviewed_report_at: story.reported_done_at, + # US-47.5: the revision and the reviewer dispatch of a chg review, both decided + # by `chg_review/4` — never cast. `nil` on every other story. + commit_sha: chg_review.commit_sha, + reviewer_dispatch_id: chg_review.reviewer_dispatch_id } |> ReviewRecord.create_changeset(attrs) @@ -2410,6 +2447,25 @@ defmodule Loopctl.Progress do end end + # US-47.5 (Epic 47 PRD §6): a story bound to a chg change takes its review only from a STRICT + # descendant of the change's chg root, naming a revision of its CURRENT binding. It runs + # AFTER `validate_not_self_review/3` and never instead of it, so on a chg story the existing + # refusals keep their codes, and on any other story this is a no-op that writes nothing: + # a `commit_sha` sent for a non-chg story is not stored, so a review recorded before a story + # was bound can never later count as the chg review of its landed revision. + @no_chg_review %{commit_sha: nil, reviewer_dispatch_id: nil} + + defp chg_review(tenant_id, story, reviewer_lineage, params) do + case ReviewCustody.chg_story(tenant_id, story.id) do + :not_chg -> + {:ok, @no_chg_review} + + {:chg, chg} -> + commit_sha = Map.get(params, "commit_sha") || Map.get(params, :commit_sha) + ReviewCustody.authorize_review(tenant_id, chg, reviewer_lineage, commit_sha) + end + end + defp build_review_attrs(params) do %{ review_type: Map.get(params, "review_type") || Map.get(params, :review_type), @@ -2468,8 +2524,13 @@ defmodule Loopctl.Progress do {:ok, review_record} end + # One reviewer dispatch records one verdict (AC-47.5.1), decided by the unique index + # `review_records_reviewer_dispatch_uidx` rather than a read-then-write, so two concurrent + # verdicts from one key cannot both land. defp handle_review_transaction({:error, :review_record, changeset, _}, _, _, _, _) do - {:error, changeset} + if Keyword.has_key?(changeset.errors, :reviewer_dispatch_id), + do: {:error, :chg_review_already_recorded}, + else: {:error, changeset} end defp handle_review_transaction({:error, :enqueue_knowledge_worker, reason, _}, _, _, _, _) do @@ -2522,16 +2583,43 @@ defmodule Loopctl.Progress do - `{:error, :unresolvable_dispatch_lineage}` if a dispatch the story references (implementer or verifier) cannot be resolved (lineage-integrity failure, fails closed — LCP-1 §7.4.1) + - `{:error, :chg_root_cannot_verify}` (US-47.5) if the caller's lineage is rooted at a + registered `chg_root_dispatch_id` (`Loopctl.Changes.ReviewCustody`), judged on + `:caller_minting_lineage` when given, else `:verifier_lineage`, before any story is read + - `{:error, :review_missing_escalated}` (US-47.5) if the story is bound to a chg change and + its delivery stage is `escalated` over `:review_missing`: a human resolves it first + - `{:error, :chg_review_not_for_landed_revision}` (US-47.5) if the story is bound to a chg + change and no chg review names its LANDED revision — the revision commit of the newest + applied `landed` event of its current binding — or it has not landed + (`Loopctl.Changes.ReviewCustody.ensure_landed_review/3`). Both run after + `review_not_conducted`, so a story with no binding is answered exactly as before """ @spec verify_story(Ecto.UUID.t(), Ecto.UUID.t(), map(), keyword()) :: {:ok, Story.t()} | {:error, atom() | {:invalid_transition, map()}} def verify_story(tenant_id, story_id, params, opts \\ []) do + verification_params = extract_verification_params(params) + + with :ok <- refuse_chg_root_verifier(tenant_id, opts) do + do_verify_story(tenant_id, story_id, opts, verification_params) + end + end + + # US-47.5 (AC-47.5.4): chg-server's keys can review and never certify. Judged on the caller's + # MINTING lineage (`:caller_minting_lineage`, revoked or not), falling back to its live + # `:verifier_lineage`, BEFORE any story is read: it is a refusal of the principal, the same on + # every story, so it never changes which code a non-chg-rooted caller meets. + defp refuse_chg_root_verifier(tenant_id, opts), + do: + ReviewCustody.refuse_chg_root( + tenant_id, + ReviewCustody.caller_lineage(opts, [:verifier_lineage]) + ) + + defp do_verify_story(tenant_id, story_id, opts, verification_params) do orchestrator_agent_id = Keyword.get(opts, :orchestrator_agent_id) actor_id = Keyword.get(opts, :actor_id) actor_label = Keyword.get(opts, :actor_label) - verification_params = extract_verification_params(params) - # #621: verify consumes NO capability, for the same reason report does not # (see start_story/3). A verify_cap was minted to the lineage # Dispatches.select_verifier/3 picks, and Capabilities.verify/2 demands an @@ -2567,6 +2655,9 @@ defmodule Loopctl.Progress do |> Multi.run(:check_review_record, fn _repo, %{lock: story} -> validate_review_record_exists(tenant_id, story_id, story) end) + |> Multi.run(:chg_landed_review, fn _repo, _changes -> + chg_landed_review(tenant_id, story_id) + end) |> Multi.run(:story, fn _repo, %{lock: story} -> apply_verified_status(story) end) @@ -2599,6 +2690,18 @@ defmodule Loopctl.Progress do unwrap_verification_transaction(multi) end + # US-47.5 (AC-47.5.5): a story bound to a chg change is verified only on a chg review of its + # LANDED revision, and never while it is held over `:review_missing` + # (`Loopctl.Changes.ReviewCustody.ensure_landed_review/3`). Read under the story lock, so a + # `landed` event — which locks the same story first — is either committed and seen here, or + # waits for this transaction. A no-op on every other story. + defp chg_landed_review(tenant_id, story_id) do + case ReviewCustody.ensure_landed_review(tenant_id, story_id) do + :ok -> {:ok, :landed_review_ok} + {:error, reason} -> {:error, reason} + end + end + @doc """ Chain-of-custody guard for the bulk-verify path. @@ -2672,14 +2775,27 @@ defmodule Loopctl.Progress do | :story_rejected | :story_has_dispatch_lineage | :story_entered_lifecycle - | :story_in_progress} + | :story_in_progress + | :review_missing_escalated + | :chg_review_not_for_landed_revision} def ensure_mark_complete_allowed(story, lifecycle_ids) do - case guard_backfillable(story, lifecycle_ids) do + case guard_backfill(story, lifecycle_ids) do {:ok, _} -> :ok {:error, reason} -> {:error, reason} end end + # Backfill and bulk mark-complete reach `verified` with no report and no verifier, so they + # take the same chg landed-review gate verify does (US-47.5, AC-47.5.5) after the + # lifecycle guard: a chg story never reaches `verified` without a review of its landed + # revision, by any path. + defp guard_backfill(story, lookup) do + with {:ok, _} = ok <- guard_backfillable(story, lookup), + {:ok, _} <- chg_landed_review(story.tenant_id, story.id) do + ok + end + end + @doc """ Backfills a story's status when the work was completed outside loopctl. @@ -2718,6 +2834,12 @@ defmodule Loopctl.Progress do markers are now clear; use report/review/verify * `{:error, :story_in_progress}` — story is mid-lifecycle (e.g. `:contracted`) with no dispatch lineage yet; it is being worked, not pre-existing done work + * `{:error, :chg_root_cannot_verify}` (US-47.5) — the caller is chg-rooted; backfill + reaches the same `verified` state as verify, so it takes the same refusal, first + * `{:error, :review_missing_escalated}` / `{:error, :chg_review_not_for_landed_revision}` + (US-47.5) — the story is bound to a chg change and is held over `:review_missing`, or + no chg review names its landed revision + (`Loopctl.Changes.ReviewCustody.ensure_landed_review/3`), as for `verify_story/4` * `{:error, %Ecto.Changeset{}}` — persistence error surfaced from Multi step """ @spec backfill_story(Ecto.UUID.t(), Ecto.UUID.t(), map(), keyword()) :: @@ -2734,11 +2856,15 @@ defmodule Loopctl.Progress do | :story_has_dispatch_lineage | :story_entered_lifecycle | :story_in_progress + | :chg_root_cannot_verify + | :review_missing_escalated + | :chg_review_not_for_landed_revision | Ecto.Changeset.t()} def backfill_story(tenant_id, story_id, params, opts \\ []) do reason = params |> Map.get("reason") |> normalize_string() - with :ok <- validate_backfill_reason(reason), + with :ok <- refuse_chg_root_verifier(tenant_id, opts), + :ok <- validate_backfill_reason(reason), {:ok, pr_number} <- cast_pr_number(Map.get(params, "pr_number")), {:ok, evidence_url} <- validate_evidence_url(params |> Map.get("evidence_url") |> normalize_string()) do @@ -2791,7 +2917,7 @@ defmodule Loopctl.Progress do multi = Multi.new() |> Multi.run(:lock, fn _repo, _changes -> lock_story(tenant_id, story_id) end) - |> Multi.run(:guard, fn _repo, %{lock: story} -> guard_backfillable(story) end) + |> Multi.run(:guard, fn _repo, %{lock: story} -> guard_backfill(story, :query) end) |> Multi.run(:story, fn _repo, %{lock: story} -> apply_backfill_status(story, reason, evidence_url, pr_number) end) @@ -2920,9 +3046,7 @@ defmodule Loopctl.Progress do # ACCURATE reason, not a false "dispatch lineage" claim. # # `lookup` is `:query` on the single-story path and a precomputed MapSet on the - # batch path. - defp guard_backfillable(story), do: guard_backfillable(story, :query) - + # batch path. Both reach it through `guard_backfill/2`. defp guard_backfillable(%{verified_status: :verified}, _lookup), do: {:error, :already_verified} @@ -3169,6 +3293,8 @@ defmodule Loopctl.Progress do - `{:error, :not_found}` if story not found in tenant - `{:error, :invalid_transition}` if story is not reported_done or verified - `{:error, :reason_required}` if reason is missing or blank + - `{:error, :chg_root_cannot_verify}` (US-47.5) if the caller is chg-rooted, as for + `verify_story/4`; checked before the reason """ @spec reject_story(Ecto.UUID.t(), Ecto.UUID.t(), map(), keyword()) :: {:ok, Story.t()} | {:error, atom() | {:invalid_transition, map()}} @@ -3180,7 +3306,11 @@ defmodule Loopctl.Progress do reason = Map.get(params, "reason") || Map.get(params, :reason) rejection_params = extract_rejection_params(params) - with :ok <- validate_reason(reason) do + # US-47.5: reject is exempt from `caller_lineage_required` only. A chg-rooted caller is + # refused here as on verify — sending work back is still a terminal custody decision, and + # one chg-server must never make about a story it reviewed. + with :ok <- refuse_chg_root_verifier(tenant_id, opts), + :ok <- validate_reason(reason) do multi = Multi.new() |> Multi.run(:lock, fn _repo, _changes -> lock_story(tenant_id, story_id) end) @@ -3251,10 +3381,20 @@ defmodule Loopctl.Progress do ## Returns - `{:ok, %{verified_count: integer, skipped_count: integer, errors: [map()]}}` on success + - `{:error, :chg_root_cannot_verify}` (US-47.5) if the caller is chg-rooted; nothing is + verified """ @spec verify_all_in_epic(Ecto.UUID.t(), Ecto.UUID.t(), map(), keyword()) :: - {:ok, map()} + {:ok, map()} | {:error, :chg_root_cannot_verify} def verify_all_in_epic(tenant_id, epic_id, params, opts \\ []) do + # US-47.5: the principal is refused once, for the whole epic, rather than once per story + # as a list of per-story errors — it is the caller, not any story, that cannot verify. + with :ok <- refuse_chg_root_verifier(tenant_id, opts) do + do_verify_all_in_epic(tenant_id, epic_id, params, opts) + end + end + + defp do_verify_all_in_epic(tenant_id, epic_id, params, opts) do stories_to_verify = fetch_eligible_stories_for_verify(tenant_id, epic_id) results = Enum.map(stories_to_verify, &verify_single(tenant_id, &1, params, opts)) @@ -3282,8 +3422,11 @@ defmodule Loopctl.Progress do |> AdminRepo.all() end + # The chg-root refusal already ran ONCE for the whole epic (`verify_all_in_epic/4`), so each + # story goes to the inner verify; every per-story gate — the chg landed-review check + # included — still runs there. defp verify_single(tenant_id, story, params, opts) do - case verify_story(tenant_id, story.id, params, opts) do + case do_verify_story(tenant_id, story.id, opts, extract_verification_params(params)) do {:ok, _updated} -> {:ok, story.id} {:error, reason} -> {:error, %{story_id: story.id, reason: inspect(reason)}} end diff --git a/lib/loopctl_web/controllers/bulk_operations_controller.ex b/lib/loopctl_web/controllers/bulk_operations_controller.ex index 6cca0f9ae..1537b9f87 100644 --- a/lib/loopctl_web/controllers/bulk_operations_controller.ex +++ b/lib/loopctl_web/controllers/bulk_operations_controller.ex @@ -12,10 +12,12 @@ defmodule LoopctlWeb.BulkOperationsController do use LoopctlWeb, :controller use OpenApiSpex.ControllerSpecs + alias Loopctl.ApiSpec.Messages alias Loopctl.ApiSpec.Schemas alias Loopctl.BulkOperations alias Loopctl.Dispatches alias LoopctlWeb.AuditContext + alias LoopctlWeb.CustodyLineage action_fallback LoopctlWeb.FallbackController @@ -39,10 +41,16 @@ defmodule LoopctlWeb.BulkOperationsController do operation(:claim, summary: "Bulk claim stories", - description: "Agent claims multiple pending stories. Partial-success semantics.", + description: + "Agent claims multiple pending stories. Partial-success semantics. A key descending " <> + "from an intake source's `chg_root_dispatch_id` is refused 409 " <> + "`chg_root_review_only` (US-47.5) for the whole batch.", request_body: {"Claim params", "application/json", Schemas.BulkClaimRequest}, responses: %{ 200 => {"Results", "application/json", Schemas.BulkResultResponse}, + 409 => + {"`chg_root_review_only` (" <> Messages.chg_root_review_only() <> ")", "application/json", + Schemas.ErrorResponse}, 422 => {"Invalid input", "application/json", Schemas.ErrorResponse}, 429 => {"Rate limit exceeded", "application/json", Schemas.RateLimitError} } @@ -50,10 +58,20 @@ defmodule LoopctlWeb.BulkOperationsController do operation(:verify, summary: "Bulk verify stories", - description: "Orchestrator verifies multiple reported_done stories.", + description: + "Orchestrator verifies multiple reported_done stories. A key descending from an " <> + "intake source's `chg_root_dispatch_id` is refused 409 `chg_root_cannot_verify` " <> + "(US-47.5) for the whole batch, and nothing is verified. A story bound to a chg " <> + "change is a per-story error whose reason leads with `review_missing_escalated` (" <> + Messages.review_missing_escalated() <> + ") or `chg_review_not_for_landed_revision` (" <> + Messages.chg_review_not_for_landed_revision() <> ").", request_body: {"Verify params", "application/json", Schemas.BulkVerifyRequest}, responses: %{ 200 => {"Results", "application/json", Schemas.BulkResultResponse}, + 409 => + {"`chg_root_cannot_verify` (" <> Messages.chg_root_cannot_verify() <> ")", + "application/json", Schemas.ErrorResponse}, 422 => {"Invalid input", "application/json", Schemas.ErrorResponse}, 429 => {"Rate limit exceeded", "application/json", Schemas.RateLimitError} } @@ -61,10 +79,16 @@ defmodule LoopctlWeb.BulkOperationsController do operation(:reject, summary: "Bulk reject stories", - description: "Orchestrator rejects multiple stories with reasons.", + description: + "Orchestrator rejects multiple stories with reasons. A key descending from an intake " <> + "source's `chg_root_dispatch_id` is refused 409 `chg_root_cannot_verify` (US-47.5) " <> + "for the whole batch, and nothing is rejected.", request_body: {"Reject params", "application/json", Schemas.BulkRejectRequest}, responses: %{ 200 => {"Results", "application/json", Schemas.BulkResultResponse}, + 409 => + {"`chg_root_cannot_verify` (" <> Messages.chg_root_cannot_verify() <> ")", + "application/json", Schemas.ErrorResponse}, 422 => {"Invalid input", "application/json", Schemas.ErrorResponse}, 429 => {"Rate limit exceeded", "application/json", Schemas.RateLimitError}, 500 => @@ -80,7 +104,10 @@ defmodule LoopctlWeb.BulkOperationsController do "ADMIN USE ONLY. Marks multiple stories as both reported_done AND verified in one step. " <> "Intended for importing pre-existing work. " <> "Skips the normal contract→claim→start→report→verify workflow. " <> - "Requires orchestrator role.", + "Requires orchestrator role. A key descending from an intake source's " <> + "`chg_root_dispatch_id` is refused 409 `chg_root_cannot_verify` (US-47.5). A story " <> + "bound to a chg change is a per-story error leading with `review_missing_escalated` " <> + "or `chg_review_not_for_landed_revision`, as on bulk verify.", request_body: {"Mark-complete params", "application/json", %OpenApiSpex.Schema{ @@ -120,6 +147,9 @@ defmodule LoopctlWeb.BulkOperationsController do }}, responses: %{ 200 => {"Results", "application/json", Schemas.BulkResultResponse}, + 409 => + {"`chg_root_cannot_verify` (" <> Messages.chg_root_cannot_verify() <> ")", + "application/json", Schemas.ErrorResponse}, 422 => {"Invalid input", "application/json", Schemas.ErrorResponse}, 429 => {"Rate limit exceeded", "application/json", Schemas.RateLimitError} } @@ -134,12 +164,17 @@ defmodule LoopctlWeb.BulkOperationsController do api_key = conn.assigns.current_api_key tenant_id = api_key.tenant_id agent_id = api_key.agent_id - audit_opts = AuditContext.from_conn(conn) + + audit_opts = + Keyword.merge(AuditContext.from_conn(conn), CustodyLineage.chg_root_opts(api_key)) case BulkOperations.bulk_claim(tenant_id, story_ids, agent_id, audit_opts) do {:ok, results} -> respond_with_results(conn, results) + {:error, :chg_root_review_only} = error -> + error + {:error, :empty_batch} -> {:error, :unprocessable_entity, "story_ids must not be empty"} @@ -169,6 +204,9 @@ defmodule LoopctlWeb.BulkOperationsController do {:ok, results} -> respond_with_results(conn, results) + {:error, :chg_root_cannot_verify} = error -> + error + {:error, :empty_batch} -> {:error, :unprocessable_entity, "stories must not be empty"} @@ -199,6 +237,9 @@ defmodule LoopctlWeb.BulkOperationsController do {:ok, results} -> respond_with_results(conn, results) + {:error, :chg_root_cannot_verify} = error -> + error + {:error, :empty_batch} -> {:error, :unprocessable_entity, "stories must not be empty"} @@ -229,7 +270,9 @@ defmodule LoopctlWeb.BulkOperationsController do with :ok <- validate_orchestrator_agent_linked(api_key) do tenant_id = api_key.tenant_id orchestrator_agent_id = api_key.agent_id - audit_opts = AuditContext.from_conn(conn) + + audit_opts = + Keyword.merge(AuditContext.from_conn(conn), CustodyLineage.chg_root_opts(api_key)) case BulkOperations.bulk_mark_complete( tenant_id, @@ -240,6 +283,9 @@ defmodule LoopctlWeb.BulkOperationsController do {:ok, results} -> respond_with_results(conn, results) + {:error, :chg_root_cannot_verify} = error -> + error + {:error, :empty_batch} -> {:error, :unprocessable_entity, "stories must not be empty"} @@ -265,9 +311,12 @@ defmodule LoopctlWeb.BulkOperationsController do # would move the custody gate with it. The lineage is never read from the request # body. defp custody_opts(conn, api_key) do - Keyword.merge(AuditContext.from_conn(conn), + conn + |> AuditContext.from_conn() + |> Keyword.merge( verifier_lineage: Dispatches.lineage_for_api_key(api_key.tenant_id, api_key.id) ) + |> Keyword.merge(CustodyLineage.chg_root_opts(api_key)) end defp respond_with_results(conn, results) do diff --git a/lib/loopctl_web/controllers/review_record_controller.ex b/lib/loopctl_web/controllers/review_record_controller.ex index 2f74a33f4..23936e627 100644 --- a/lib/loopctl_web/controllers/review_record_controller.ex +++ b/lib/loopctl_web/controllers/review_record_controller.ex @@ -13,6 +13,7 @@ defmodule LoopctlWeb.ReviewRecordController do use LoopctlWeb, :controller use OpenApiSpex.ControllerSpecs + alias Loopctl.ApiSpec.Messages alias Loopctl.ApiSpec.Schemas alias Loopctl.Dispatches alias Loopctl.Progress @@ -37,7 +38,15 @@ defmodule LoopctlWeb.ReviewRecordController do description: "Records that the review pipeline completed for a story. " <> "Must be called AFTER the story is in reported_done status and BEFORE verify. " <> - "Creates a review_record that verify uses as proof of independent review.", + "Creates a review_record that verify uses as proof of independent review. " <> + "A story BOUND TO A chg CHANGE (US-47.5) takes its review only from a key whose " <> + "dispatch is a strict descendant of the intake source's `chg_root_dispatch_id` " <> + "(409 `chg_reviewer_required` otherwise, chg-server's root key included), must " <> + "name the reviewed revision as `commit_sha` (422 `commit_sha_not_bound` when it is " <> + "absent, malformed or not a submitted revision of the story's current change), and " <> + "records one verdict per reviewer dispatch (409 `chg_review_already_recorded`). " <> + "These run after, never instead of, the existing custody gates. On any other story " <> + "`commit_sha` is ignored and not stored.", parameters: [id: [in: :path, type: :string, description: "Story UUID"]], request_body: {"Review completion params", "application/json", @@ -77,13 +86,32 @@ defmodule LoopctlWeb.ReviewRecordController do description: "When the review completed (defaults to now). Must be after reported_done_at.", example: "2026-03-30T01:44:41Z" + }, + commit_sha: %OpenApiSpex.Schema{ + type: :string, + pattern: "^[0-9a-f]{40}([0-9a-f]{24})?$", + description: + "REQUIRED for a story bound to a chg change (US-47.5): the revision commit " <> + "reviewed, the sha of a submitted revision of the story's current change. " <> + "Ignored and not stored for any other story.", + example: "3f786850e387550fdab836ed7e6dc881de23001b" } } }}, responses: %{ 201 => {"Review recorded", "application/json", Schemas.ReviewRecordResponse}, 404 => {"Story not found", "application/json", Schemas.ErrorResponse}, - 422 => {"Story not in reported_done status", "application/json", Schemas.ErrorResponse}, + 409 => + {"Custody refusal. `self_review_blocked`, `caller_lineage_required`, " <> + "`missing_assigned_agent`, `unresolvable_dispatch_lineage`; and on a story bound " <> + "to a chg change `chg_reviewer_required` (" <> + Messages.chg_reviewer_required() <> + ") or `chg_review_already_recorded` (" <> + Messages.chg_review_already_recorded() <> ")", "application/json", + Schemas.ErrorResponse}, + 422 => + {"Story not in reported_done status, a validation error, or `commit_sha_not_bound` (" <> + Messages.commit_sha_not_bound() <> ")", "application/json", Schemas.ErrorResponse}, 429 => {"Rate limit exceeded", "application/json", Schemas.RateLimitError} } ) @@ -131,29 +159,14 @@ defmodule LoopctlWeb.ReviewRecordController do # hash-chained audit entry, atomically with the review record. |> Keyword.put(:custody_claim, conn.assigns[:custody_signed_claim]) - case Progress.record_review(tenant_id, story_id, params, opts) do - {:ok, review_record} -> - conn - |> put_status(:created) - |> json(%{review_record: review_record}) - - {:error, :not_found} -> - {:error, :not_found} - - {:error, :story_not_reported_done} -> - {:error, :story_not_reported_done} - - {:error, :self_review_blocked} -> - {:error, :self_review_blocked} - - {:error, :unresolvable_dispatch_lineage} -> - {:error, :unresolvable_dispatch_lineage} - - {:error, :missing_assigned_agent} -> - {:error, :missing_assigned_agent} - - {:error, %Ecto.Changeset{} = changeset} -> - {:error, changeset} + # TOTAL over `Progress.record_review/4`'s returns: every refusal goes to + # `LoopctlWeb.FallbackController`, which has a clause for each. The enumerated `case` + # this replaces had none for `:caller_lineage_required`, so that documented 409 was an + # unhandled `CaseClauseError` — a 500 — on this route. + with {:ok, review_record} <- Progress.record_review(tenant_id, story_id, params, opts) do + conn + |> put_status(:created) + |> json(%{review_record: review_record}) end end end diff --git a/lib/loopctl_web/controllers/story_status_controller.ex b/lib/loopctl_web/controllers/story_status_controller.ex index ab75740b5..2f14f7490 100644 --- a/lib/loopctl_web/controllers/story_status_controller.ex +++ b/lib/loopctl_web/controllers/story_status_controller.ex @@ -18,6 +18,7 @@ defmodule LoopctlWeb.StoryStatusController do require Logger use OpenApiSpex.ControllerSpecs + alias Loopctl.ApiSpec.Messages alias Loopctl.ApiSpec.Schemas alias Loopctl.Capabilities alias Loopctl.Dispatches @@ -25,6 +26,7 @@ defmodule LoopctlWeb.StoryStatusController do alias Loopctl.Progress alias Loopctl.Progress.StateMachine alias LoopctlWeb.AuditContext + alias LoopctlWeb.CustodyLineage action_fallback LoopctlWeb.FallbackController @@ -130,7 +132,8 @@ defmodule LoopctlWeb.StoryStatusController do "cap has passed). It is placed_at + `wall_clock_seconds` + " <> "`DISPATCH_LEASE_GRACE_SECONDS`, and moves only forward, only while the claim is " <> "live: to a resume's time or the runner's acceptance + `wall_clock_seconds` + the " <> - "grace.", + "grace. A key descending from an intake source's `chg_root_dispatch_id` is refused " <> + "409 `chg_root_review_only` (US-47.5): chg-server's keys may only review.", parameters: [id: [in: :path, type: :string, description: "Story UUID"]], responses: %{ 200 => @@ -142,8 +145,9 @@ defmodule LoopctlWeb.StoryStatusController do {"Invalid transition, dependencies not met, or `story_held` — the story's delivery " <> "stage is `escalated`, `done` or `failed`, so it is not available to agents. An " <> "escalated story is claimable again only once a human resolves it to `queued` " <> - "(POST /stories/:id/stage/resolve); a done or failed one never is", "application/json", - Schemas.ErrorResponse}, + "(POST /stories/:id/stage/resolve); a done or failed one never is. Or " <> + "`chg_root_review_only` (" <> Messages.chg_root_review_only() <> ")", + "application/json", Schemas.ErrorResponse}, 429 => {"Rate limit exceeded", "application/json", Schemas.RateLimitError}, 503 => {"The claim's capability could not be minted; nothing was claimed. Retryable only " <> @@ -223,6 +227,8 @@ defmodule LoopctlWeb.StoryStatusController do "structural lineage separation, not by L1, because a capability can only be " <> "bound to a lineage known when it is minted, and the reporter is by definition " <> "a principal distinct from the implementer who started the work. " <> + "A key descending from an intake source's `chg_root_dispatch_id` is refused 409 " <> + "`chg_root_review_only` (US-47.5). " <> "Optionally includes an artifact report and/or a token usage record.", parameters: [id: [in: :path, type: :string, description: "Story UUID"]], request_body: @@ -289,8 +295,9 @@ defmodule LoopctlWeb.StoryStatusController do 400 => {"claim_epoch is not a non-negative integer", "application/json", Schemas.ErrorResponse}, 409 => - {"Invalid transition, self-report blocked, or stale_claim_epoch", "application/json", - Schemas.ErrorResponse}, + {"Invalid transition, self-report blocked, stale_claim_epoch, or " <> + "`chg_root_review_only` (" <> Messages.chg_root_review_only() <> ")", + "application/json", Schemas.ErrorResponse}, 429 => {"Rate limit exceeded", "application/json", Schemas.RateLimitError} } ) @@ -419,6 +426,7 @@ defmodule LoopctlWeb.StoryStatusController do AuditContext.from_conn(conn) |> Keyword.merge(agent_id: api_key.agent_id) |> Keyword.merge(custody_identity(api_key)) + |> Keyword.merge(CustodyLineage.chg_root_opts(api_key)) case Progress.claim_story(tenant_id, story_id, opts) do {:ok, story} -> @@ -574,6 +582,7 @@ defmodule LoopctlWeb.StoryStatusController do # gate fed by :reporter_lineage above is the enforcement. :dispatch_id is # still resolved server-side so the reporter's dispatch is recorded. |> Keyword.merge(custody_identity(api_key)) + |> Keyword.merge(CustodyLineage.chg_root_opts(api_key)) |> maybe_add_token_usage(params) |> Keyword.put(:claim_epoch, params["claim_epoch"]) @@ -597,7 +606,9 @@ defmodule LoopctlWeb.StoryStatusController do :unresolvable_dispatch_lineage, :missing_assigned_agent, # #803: the reporter presented an epoch from a claim that has ended. - :stale_claim_epoch + :stale_claim_epoch, + # US-47.5: chg-server's keys may only review. + :chg_root_review_only ] -> {:error, reason} diff --git a/lib/loopctl_web/controllers/story_verification_controller.ex b/lib/loopctl_web/controllers/story_verification_controller.ex index 6548ed7ca..6a9de8f47 100644 --- a/lib/loopctl_web/controllers/story_verification_controller.ex +++ b/lib/loopctl_web/controllers/story_verification_controller.ex @@ -15,6 +15,7 @@ defmodule LoopctlWeb.StoryVerificationController do require Logger + alias Loopctl.ApiSpec.Messages alias Loopctl.ApiSpec.Schemas alias Loopctl.Artifacts alias Loopctl.Dispatches @@ -23,6 +24,7 @@ defmodule LoopctlWeb.StoryVerificationController do alias Loopctl.Verification.VerificationRun alias Loopctl.WorkBreakdown.Stories alias LoopctlWeb.AuditContext + alias LoopctlWeb.CustodyLineage action_fallback LoopctlWeb.FallbackController @@ -59,13 +61,26 @@ defmodule LoopctlWeb.StoryVerificationController do description: "Orchestrator verifies a reported_done story. Creates verification_result with result=pass. " <> "Requires a review_record to exist (call POST /stories/:id/review-complete first). " <> - "The review_record must have been completed AFTER the story was reported done.", + "The review_record must have been completed AFTER the story was reported done. " <> + "A key descending from an intake source's `chg_root_dispatch_id` is refused " <> + "409 `chg_root_cannot_verify` (US-47.5) before any story is read. A story BOUND TO A " <> + "chg CHANGE is verified only on a chg review of its LANDED revision — the revision " <> + "commit of the newest applied `landed` event of its current change — " <> + "(409 `chg_review_not_for_landed_revision`), and never while its delivery stage is " <> + "escalated over `review_missing` (409 `review_missing_escalated`).", parameters: [id: [in: :path, type: :string, description: "Story UUID"]], request_body: {"Verification params", "application/json", Schemas.VerifyRequest}, responses: %{ 200 => {"Story verified", "application/json", Schemas.StoryStatusResponse}, 404 => {"Not found", "application/json", Schemas.ErrorResponse}, - 409 => {"Invalid transition", "application/json", Schemas.ErrorResponse}, + 409 => + {"Invalid transition, a custody refusal, `chg_root_cannot_verify` (" <> + Messages.chg_root_cannot_verify() <> + "), `review_missing_escalated` (" <> + Messages.review_missing_escalated() <> + ") or `chg_review_not_for_landed_revision` (" <> + Messages.chg_review_not_for_landed_revision() <> ")", "application/json", + Schemas.ErrorResponse}, 422 => {"No review record found", "application/json", Schemas.ErrorResponse}, 429 => {"Rate limit exceeded", "application/json", Schemas.RateLimitError} } @@ -79,13 +94,17 @@ defmodule LoopctlWeb.StoryVerificationController do "row was in flight: a reject spent an attempt, so it is re-contracted (`contracted`) " <> "below the retry ceiling `DISPATCH_MAX_ATTEMPTS` and its stage row escalated over " <> "`attempts_exhausted` at it, leaving it `pending` for a human. The story returned is " <> - "the story as the reset left it.", + "the story as the reset left it. A key descending from an intake source's " <> + "`chg_root_dispatch_id` is refused 409 `chg_root_cannot_verify` (US-47.5): reject is " <> + "exempt from `caller_lineage_required` only.", parameters: [id: [in: :path, type: :string, description: "Story UUID"]], request_body: {"Rejection params", "application/json", Schemas.RejectRequest}, responses: %{ 200 => {"Story rejected", "application/json", Schemas.StoryStatusResponse}, 404 => {"Not found", "application/json", Schemas.ErrorResponse}, - 409 => {"Invalid transition", "application/json", Schemas.ErrorResponse}, + 409 => + {"Invalid transition, a custody refusal, or `chg_root_cannot_verify` (" <> + Messages.chg_root_cannot_verify() <> ")", "application/json", Schemas.ErrorResponse}, 422 => {"Reason required", "application/json", Schemas.ErrorResponse}, 429 => {"Rate limit exceeded", "application/json", Schemas.RateLimitError}, 500 => @@ -107,6 +126,10 @@ defmodule LoopctlWeb.StoryVerificationController do "Under the LCP-1 `signed` custody profile an enrolled caller must present a valid " <> "`claim` signature (gate `verify`), as for POST /stories/:id/verify. " <> "Requires a non-empty `reason`; `evidence_url` and `pr_number` are optional but strongly recommended. " <> + "A key descending from an intake source's `chg_root_dispatch_id` is refused 409 " <> + "`chg_root_cannot_verify` (US-47.5). A story bound to a chg change takes the same " <> + "landed-review gate verify does: 409 `review_missing_escalated` or " <> + "`chg_review_not_for_landed_revision`. " <> "Emits a `story.backfilled` webhook event on success.", parameters: [id: [in: :path, type: :string, description: "Story UUID"]], request_body: @@ -128,6 +151,14 @@ defmodule LoopctlWeb.StoryVerificationController do 403 => {"Insufficient role (orchestrator+ required)", "application/json", Schemas.ErrorResponse}, 404 => {"Story not found", "application/json", Schemas.ErrorResponse}, + 409 => + {"`chg_root_cannot_verify` (" <> + Messages.chg_root_cannot_verify() <> + "), `review_missing_escalated` (" <> + Messages.review_missing_escalated() <> + ") or `chg_review_not_for_landed_revision` (" <> + Messages.chg_review_not_for_landed_revision() <> ")", "application/json", + Schemas.ErrorResponse}, 422 => {"Validation error: missing reason, already verified, already rejected, has dispatch lineage, or already entered the lifecycle", "application/json", Schemas.ErrorResponse}, @@ -202,7 +233,13 @@ defmodule LoopctlWeb.StoryVerificationController do "Orchestrator convenience endpoint that verifies all stories in the epic " <> "that have agent_status=reported_done and verified_status=unverified. " <> "Requires review_type and summary in the body (same as single verify). " <> - "Returns count of verified stories and any errors.", + "Returns count of verified stories and any errors. A key descending from an intake " <> + "source's `chg_root_dispatch_id` is refused 409 `chg_root_cannot_verify` (US-47.5) for " <> + "the whole epic, and nothing is verified. A story bound to a chg change is skipped " <> + "into `errors` with reason `:review_missing_escalated` (" <> + Messages.review_missing_escalated() <> + ") or `:chg_review_not_for_landed_revision` (" <> + Messages.chg_review_not_for_landed_revision() <> ").", parameters: [id: [in: :path, type: :string, description: "Epic UUID"]], request_body: {"Verification params", "application/json", Schemas.VerifyRequest}, responses: %{ @@ -218,6 +255,9 @@ defmodule LoopctlWeb.StoryVerificationController do } }}, 404 => {"Not found", "application/json", Schemas.ErrorResponse}, + 409 => + {"`chg_root_cannot_verify` (" <> Messages.chg_root_cannot_verify() <> ")", + "application/json", Schemas.ErrorResponse}, 429 => {"Rate limit exceeded", "application/json", Schemas.RateLimitError} } ) @@ -252,6 +292,7 @@ defmodule LoopctlWeb.StoryVerificationController do # under this exact key. verifier_lineage: Dispatches.lineage_for_api_key(tenant_id, api_key.id) ) + |> Keyword.merge(CustodyLineage.chg_root_opts(api_key)) case Progress.verify_story(tenant_id, story_id, params, opts) do {:ok, story} -> @@ -322,13 +363,20 @@ defmodule LoopctlWeb.StoryVerificationController do custody_claim: conn.assigns[:custody_signed_claim], verifier_lineage: Dispatches.lineage_for_api_key(tenant_id, api_key.id) ) + |> Keyword.merge(CustodyLineage.chg_root_opts(api_key)) case Progress.backfill_story(tenant_id, story_id, params, opts) do {:ok, story} -> json(conn, %{story: story}) - {:error, :not_found} -> - {:error, :not_found} + {:error, reason} + when reason in [ + :not_found, + :chg_root_cannot_verify, + :review_missing_escalated, + :chg_review_not_for_landed_revision + ] -> + {:error, reason} {:error, %Ecto.Changeset{} = changeset} -> {:error, changeset} @@ -396,7 +444,8 @@ defmodule LoopctlWeb.StoryVerificationController do :unresolvable_dispatch_lineage, :missing_assigned_agent, :not_found, - :audit_chain_append_failed + :audit_chain_append_failed, + :chg_root_cannot_verify ] or is_struct(reason, Ecto.Changeset) @doc """ @@ -418,6 +467,7 @@ defmodule LoopctlWeb.StoryVerificationController do # takes the same server-resolved caller lineage verify does. verifier_lineage: Dispatches.lineage_for_api_key(tenant_id, api_key.id) ) + |> Keyword.merge(CustodyLineage.chg_root_opts(api_key)) case Progress.reject_story(tenant_id, story_id, params, opts) do {:ok, story} -> @@ -543,9 +593,11 @@ defmodule LoopctlWeb.StoryVerificationController do # radius path while the single-story route enforced it. verifier_lineage: Dispatches.lineage_for_api_key(tenant_id, api_key.id) ) + |> Keyword.merge(CustodyLineage.chg_root_opts(api_key)) - {:ok, result} = Progress.verify_all_in_epic(tenant_id, epic_id, params, opts) - json(conn, result) + with {:ok, result} <- Progress.verify_all_in_epic(tenant_id, epic_id, params, opts) do + json(conn, result) + end end end diff --git a/lib/loopctl_web/custody_lineage.ex b/lib/loopctl_web/custody_lineage.ex new file mode 100644 index 000000000..54c6159f9 --- /dev/null +++ b/lib/loopctl_web/custody_lineage.ex @@ -0,0 +1,20 @@ +defmodule LoopctlWeb.CustodyLineage do + @moduledoc """ + The custody-call options a controller resolves from the AUTHENTICATING key, in one place so + every route hands the context the same principal. + + `chg_root_opts/1` (US-47.5) is the lineage `Loopctl.Changes.ReviewCustody` judges the + chg-root refusal on — `chg_root_cannot_verify` on verify, reject, verify-all, backfill and + the bulk paths; `chg_root_review_only` on claim, bulk claim and report. It is the caller's + MINTING lineage (`Loopctl.Dispatches.minting_lineage_for_api_key/2`), revoked or not, so a + chg key whose dispatch has expired — live lineage `[]`, which reject admits — is still + chg's. Never read from the request. + """ + + alias Loopctl.Dispatches + + @doc "`[caller_minting_lineage: lineage]` for `api_key`, resolved server-side." + @spec chg_root_opts(%{tenant_id: Ecto.UUID.t(), id: Ecto.UUID.t()}) :: keyword() + def chg_root_opts(%{tenant_id: tenant_id, id: api_key_id}), + do: [caller_minting_lineage: Dispatches.minting_lineage_for_api_key(tenant_id, api_key_id)] +end diff --git a/lib/loopctl_web/fallback_controller.ex b/lib/loopctl_web/fallback_controller.ex index 9adecd7e5..374539643 100644 --- a/lib/loopctl_web/fallback_controller.ex +++ b/lib/loopctl_web/fallback_controller.ex @@ -47,6 +47,13 @@ defmodule LoopctlWeb.FallbackController do - `{:error, :missing_assigned_agent}` -> 409 (reported_done story has no assigned agent/dispatch lineage; custody chain broken) - `{:error, :unresolvable_dispatch_lineage}` -> 409 (a dispatch the story references — implementer, or verifier on verify — could not be resolved; custody gate failed closed on an integrity error, tenant NOT halted) - `{:error, :caller_lineage_required}` -> 409 (a key no dispatch minted tried to report/review/verify dispatch-minted work; a configuration refusal, tenant NOT halted) + - `{:error, :chg_reviewer_required}` -> 409 (US-47.5: review-complete on a story bound to a chg change by a key that is not a strict descendant of its chg root) + - `{:error, :commit_sha_not_bound}` -> 422 (US-47.5: that review named no submitted revision of the story's current binding) + - `{:error, :chg_review_already_recorded}` -> 409 (US-47.5: the chg reviewer dispatch already recorded its one verdict) + - `{:error, :chg_root_cannot_verify}` -> 409 (US-47.5: a chg-rooted key tried to verify, reject, verify-all, bulk verify/reject, backfill or mark complete) + - `{:error, :chg_root_review_only}` -> 409 (US-47.5: a chg-rooted key tried to claim or report) + - `{:error, :review_missing_escalated}` -> 409 (US-47.5: a chg story held over `:review_missing` reached a verify path; a human resolves the escalation first) + - `{:error, :chg_review_not_for_landed_revision}` -> 409 (US-47.5: a chg story reached a verify path with no chg review of its landed revision) - `{:error, :rate_limited}` -> 429 with retry_after_seconds from header - `{:error, :ingestion_backlog_exceeded, retry_after}` -> 429 with `Retry-After` header and a machine-readable `code: "ingestion_backlog_exceeded"` (US-36.3 ingest backpressure — @@ -589,6 +596,42 @@ defmodule LoopctlWeb.FallbackController do }) end + # US-47.5 (Epic 47 PRD §6). None records a custody violation: each refuses a principal for + # being chg-server's, which is a structural fact about the key, not a byzantine self-claim, + # and a misconfigured chg-server would otherwise arm the tenant-wide halt on every retry. + def call(conn, {:error, code}) + when code in [ + :chg_reviewer_required, + :chg_review_already_recorded, + :chg_root_cannot_verify, + :chg_root_review_only, + :review_missing_escalated, + :chg_review_not_for_landed_revision + ] do + conn + |> put_status(:conflict) + |> json(%{ + error: %{ + status: 409, + code: Atom.to_string(code), + message: chg_custody_message(code), + remediation: %{learn_more: "https://loopctl.com/wiki/chain-of-custody"} + } + }) + end + + def call(conn, {:error, :commit_sha_not_bound}) do + conn + |> put_status(:unprocessable_entity) + |> json(%{ + error: %{ + status: 422, + code: "commit_sha_not_bound", + message: Messages.commit_sha_not_bound() + } + }) + end + def call(conn, {:error, :self_report_blocked}) do record_custody_violation(conn, "self_report_blocked") @@ -1320,4 +1363,17 @@ defmodule LoopctlWeb.FallbackController do # is a struct and therefore matches neither clause's map pattern. defp story_id_param(%Plug.Conn{params: %{"id" => id}}) when is_binary(id), do: id defp story_id_param(%Plug.Conn{}), do: nil + + # US-47.5: the 409 messages of the chg custody refusals, from `Loopctl.ApiSpec.Messages`. + defp chg_custody_message(:chg_reviewer_required), do: Messages.chg_reviewer_required() + + defp chg_custody_message(:chg_review_already_recorded), + do: Messages.chg_review_already_recorded() + + defp chg_custody_message(:chg_root_cannot_verify), do: Messages.chg_root_cannot_verify() + defp chg_custody_message(:chg_root_review_only), do: Messages.chg_root_review_only() + defp chg_custody_message(:review_missing_escalated), do: Messages.review_missing_escalated() + + defp chg_custody_message(:chg_review_not_for_landed_revision), + do: Messages.chg_review_not_for_landed_revision() end diff --git a/lib/loopctl_web/telemetry.ex b/lib/loopctl_web/telemetry.ex index a687f9913..7f4330d02 100644 --- a/lib/loopctl_web/telemetry.ex +++ b/lib/loopctl_web/telemetry.ex @@ -145,6 +145,12 @@ defmodule LoopctlWeb.Telemetry do tags: [:source, :reason], description: "chg change events refused 401, by source and reason." ), + # US-47.5: a landing whose `review_missing` hold could not be written, so the whole event + # rolled back and chg will redeliver it. Not contention (that is `:busy`): alert on any. + counter("loopctl.changes.review_hold_refused.count", + event_name: [:loopctl, :changes, :review_hold_refused], + description: "chg landed events rolled back because the review_missing hold was refused." + ), # A gap pull chg-server refused for its signature: under a one-sided secret rotation, # the only signal loopctl gets. Alert on any. counter("loopctl.changes.pull_refused.count", diff --git a/mcp-server/CHANGELOG.md b/mcp-server/CHANGELOG.md index f333a9ba3..224559c15 100644 --- a/mcp-server/CHANGELOG.md +++ b/mcp-server/CHANGELOG.md @@ -5,6 +5,33 @@ All notable changes to `loopctl-mcp-server` are documented here. Format: [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) Versioning: [Semantic Versioning](https://semver.org/spec/v2.0.0.html) +## 3.3.0 — 2026-10-19 (chg review custody) + +### Added + +- **`review_complete` takes `commit_sha`** (loopctl US-47.5, AC-47.5.2): the reviewed chg + revision, forwarded as given. REQUIRED on a story bound to a chg change, where it must be + the sha of a submitted revision of the story's current change (422 `commit_sha_not_bound` + otherwise); ignored and not stored on any other story. + +### Changed + +- **`review_complete` documents the chg review gates** (AC-47.5.1): on a story bound to a chg + change only a key chg-server minted for the review — a strict descendant of the intake + source's `chg_root_dispatch_id` — may record it (409 `chg_reviewer_required`, the root key + included), and one reviewer dispatch records one verdict (409 + `chg_review_already_recorded`). They run after the existing custody gates, never instead. +- **`verify_story`, `reject_story`, `verify_all_in_epic` and `bulk_mark_complete` document 409 + `chg_root_cannot_verify`** (AC-47.5.4): a key descending from a registered + `chg_root_dispatch_id` never certifies; the bulk paths refuse the whole call. +- **`claim_story` and `report_story` document 409 `chg_root_review_only`**: chg-server's keys + may only review. +- **`verify_story`, `verify_all_in_epic` and `bulk_mark_complete` document the chg + landed-review gate** (AC-47.5.5): a story bound to a chg change is verified only on a chg + review of its LANDED revision (409 `chg_review_not_for_landed_revision`) and never while its + stage is escalated over `review_missing` (409 `review_missing_escalated`); the epic and bulk + paths report either as a per-story error. + ## 3.2.0 — 2026-10-19 (change_binding_get) ### Added diff --git a/mcp-server/README.md b/mcp-server/README.md index e99b3fdfc..0154bd567 100644 --- a/mcp-server/README.md +++ b/mcp-server/README.md @@ -259,7 +259,7 @@ Epic 39 Repo Coordination Bus — a lightweight, tenant-isolated channel for age | Tool | Description | |---|---| | `contract_story` | Agent acknowledges a story's acceptance criteria. Transitions pending -> contracted. Refused 409 `story_held` when the story's delivery stage is `escalated`, `done` or `failed` — it is not available to agents even when it reads `pending`; an escalated story becomes available again only after `resolve_escalation` sends it to `queued`. | -| `claim_story` | Agent claims a contracted story with pessimistic locking. Transitions contracted -> assigned. On a loopctl with claim leases the result leads with the claim's `claim_epoch` and `claimed_until`; keep the epoch for `renew_story_claim`. Both are absent on an older server. Refused 409 `story_held` when the story's delivery stage is `escalated`, `done` or `failed` — even when it reads `pending` or `contracted`; an escalated story is claimable again only after `resolve_escalation` sends it to `queued`, a done or failed one never is. | +| `claim_story` | Agent claims a contracted story with pessimistic locking. Transitions contracted -> assigned. On a loopctl with claim leases the result leads with the claim's `claim_epoch` and `claimed_until`; keep the epoch for `renew_story_claim`. Both are absent on an older server. Refused 409 `story_held` when the story's delivery stage is `escalated`, `done` or `failed` — even when it reads `pending` or `contracted`; an escalated story is claimable again only after `resolve_escalation` sends it to `queued`, a done or failed one never is. Refused 409 `chg_root_review_only` when the key descends from an intake source's `chg_root_dispatch_id`: chg-server's keys may only review. | | `renew_story_claim` | Renew your claim's lease (`POST /stories/:id/renew-claim`, agent key). A claim not renewed before `claimed_until` (default 24 hours, `STORY_CLAIM_LEASE_SECONDS`) is released back to `pending` under you, so renew well inside it on any story held longer. A driver-placed claim — one a placement took for a runner dispatch — is capped at its dispatch deadline (`claim_lease_cap`: placed_at + `wall_clock_seconds` + `DISPATCH_LEASE_GRACE_SECONDS` at the claim, sent to the runner as `deadline_at`; moved forward to a resume's time or the runner's acceptance + `wall_clock_seconds` + the grace, only while the claim is live; never earlier than a `deadline_at` already sent), and its `claimed_until` already IS that cap: renewing it writes nothing and returns the claim as it stands, never moving `claimed_until` past it. Required: `story_id`, `claim_epoch` (from the claim). Refusals pass through: 400 epoch missing or malformed, 422 `not_claimed`, 409 `stale_claim_epoch` (the claim has ended: stop working it), 409 `not_claimant`, 409 `lease_cap_reached` (a driver-placed claim already at its cap: nothing is renewed, stop working it). Needs a loopctl server with claim leases. | | `escalate_story` | Park a story you hold for a HUMAN and stop (`POST /stories/:id/escalate`, agent key). This is what an unattended session calls INSTEAD of asking a question — a headless run has no way to ask, so escalating is the affordance. Use it when the request contradicts an existing story, KB decision or documented behaviour, when it inverts or removes behaviour a previous story deliberately added, when it is a workflow change rather than a defect fix, or when it needs a business call — not for an ordinary blocker. ONE-WAY from your side: only a human moves the story off `escalated`, so on success STOP and report. Required: `story_id`, `claim_epoch`, `reason` (verbatim, treated as untrusted data, length capped by the server in codepoints — a 400 names the bound); optional `payload` object. Idempotent under the same epoch. Refusals pass through: 409 `stale_claim_epoch`, 409 `not_claimant`, 404 `unknown_story_stage`, 400 for a malformed epoch or reason. An orchestrator or user key is 403'd on purpose. | | `start_story` | Agent starts work on a claimed story. Transitions assigned -> implementing. | @@ -269,22 +269,22 @@ Epic 39 Repo Coordination Bus — a lightweight, tenant-isolated channel for age | Tool | Description | |---|---| -| `report_story` | Reviewer confirms the implementation is done. Transitions implementing -> reported_done. Accepts optional `token_usage` object. Under the LCP-1 §9.3 signed profile, pass the `claim` from `custody_sign_claim`. | -| `review_complete` | Record that a review has been completed for a story. Required before verify. Under the signed profile, pass the `claim` from `custody_sign_claim`. | +| `report_story` | Reviewer confirms the implementation is done. Transitions implementing -> reported_done. Accepts optional `token_usage` object. Under the LCP-1 §9.3 signed profile, pass the `claim` from `custody_sign_claim`. Refused 409 `chg_root_review_only` for a key descending from an intake source's `chg_root_dispatch_id`. | +| `review_complete` | Record that a review has been completed for a story. Required before verify. Under the signed profile, pass the `claim` from `custody_sign_claim`. On a story bound to a chg change (loopctl US-47.5), after the existing custody gates: only a key chg-server minted for the review — a strict descendant of the intake source's `chg_root_dispatch_id` — may record it (409 `chg_reviewer_required`, the root key included); `commit_sha` is REQUIRED and must be the sha of a submitted revision of the story's current change (422 `commit_sha_not_bound`); one reviewer dispatch records one verdict (409 `chg_review_already_recorded`). `commit_sha` is ignored on any other story. | ### Verification Tools (orchestrator key) | Tool | Description | |---|---| -| `verify_story` | Orchestrator verifies a reported_done story. Transitions reported_done -> verified. Under the signed profile, pass the `claim` from `custody_sign_claim`. | -| `reject_story` | Orchestrator rejects a story with a reason. The reject releases the claim, and a delivery story may be re-contracted or escalated in the same transaction. A 500 `audit_chain_append_failed` (the escalation's chain entry was refused) means the WHOLE call rolled back: the story is unchanged, and a re-run meets the same server-side condition until an operator acts. | +| `verify_story` | Orchestrator verifies a reported_done story. Transitions reported_done -> verified. Under the signed profile, pass the `claim` from `custody_sign_claim`. Refused 409 `chg_root_cannot_verify` for a key descending from an intake source's `chg_root_dispatch_id` (loopctl US-47.5). | +| `reject_story` | Orchestrator rejects a story with a reason. The reject releases the claim, and a delivery story may be re-contracted or escalated in the same transaction. A 500 `audit_chain_append_failed` (the escalation's chain entry was refused) means the WHOLE call rolled back: the story is unchanged, and a re-run meets the same server-side condition until an operator acts. Refused 409 `chg_root_cannot_verify` for a key descending from an intake source's `chg_root_dispatch_id` (loopctl US-47.5). | ### Bulk Tools (orchestrator key) | Tool | Description | |---|---| -| `bulk_mark_complete` | **Backfill-only.** Bulk-marks pre-existing, never-dispatched stories (pending, unassigned, no dispatch lineage) as complete in a single call. Dispatched stories are refused — they must go through the normal report → review → verify flow. | -| `verify_all_in_epic` | Bulk verify all reported_done, unverified stories in an epic. | +| `bulk_mark_complete` | **Backfill-only.** Bulk-marks pre-existing, never-dispatched stories (pending, unassigned, no dispatch lineage) as complete in a single call. Dispatched stories are refused — they must go through the normal report → review → verify flow. Refused 409 `chg_root_cannot_verify` for the whole batch for a chg-rooted key (loopctl US-47.5). | +| `verify_all_in_epic` | Bulk verify all reported_done, unverified stories in an epic. Refused 409 `chg_root_cannot_verify` for the whole epic for a chg-rooted key (loopctl US-47.5). | ### Token Efficiency Tools @@ -664,6 +664,8 @@ loopctl enforces that nobody marks their own work as done. The API returns `409` The implementer's final action is `request_review`. All subsequent steps (report, review, verify) must come from different agents. +On a story bound to a chg change (loopctl US-47.5) the review comes from chg-server: `review_complete` takes it only from a key chg-server minted beneath its registered `chg_root_dispatch_id` (409 `chg_reviewer_required`), naming the reviewed revision as `commit_sha` (422 `commit_sha_not_bound`), one verdict per reviewer dispatch (409 `chg_review_already_recorded`). chg-server's keys never certify or implement: `verify_story`, `reject_story`, `verify_all_in_epic` and `bulk_mark_complete` answer 409 `chg_root_cannot_verify`, `claim_story` and `report_story` 409 `chg_root_review_only`. A chg story that lands with no review for its landed revision is escalated over `review_missing` and is not verified without a human. + ## Witness protocol (STH) Every authenticated request echoes the caller's last-known Signed Tree Head (STH) diff --git a/mcp-server/index.js b/mcp-server/index.js index 7f6476e8e..6f77076bd 100755 --- a/mcp-server/index.js +++ b/mcp-server/index.js @@ -1372,12 +1372,15 @@ async function reportStory({ story_id, artifact_type, artifact_path, token_usage return toContent(result); } -async function reviewComplete({ story_id, review_type, findings_count, fixes_count, disproved_count, summary, claim }) { +async function reviewComplete({ story_id, review_type, findings_count, fixes_count, disproved_count, summary, commit_sha, claim }) { const body = { review_type }; if (findings_count != null) body.findings_count = findings_count; if (fixes_count != null) body.fixes_count = fixes_count; if (disproved_count != null) body.disproved_count = disproved_count; if (summary) body.summary = summary; + // loopctl US-47.5: the reviewed chg revision. Forwarded as given and judged server-side + // (422 commit_sha_not_bound), so a malformed value reaches the one place that can say why. + if (commit_sha != null) body.commit_sha = commit_sha; if (claim) body.claim = claim; const result = await apiCall( @@ -1400,7 +1403,7 @@ async function verifyStory({ story_id, summary, review_type, claim }) { if (review_type) body.review_type = review_type; if (claim) body.claim = claim; - // `exact_role: :orchestrator` (`story_verification_controller.ex:29-30`). When + // `exact_role: :orchestrator` (`story_verification_controller.ex:30-31`). When // LOOPCTL_ORCH_KEY is set it is sent EXACTLY, because `resolveKey` would otherwise // discard it in favour of a global LOOPCTL_API_KEY on the very calls it was set for. // When it is NOT set, the global key still goes — see lib/custody-key.js for why the @@ -1418,7 +1421,7 @@ async function verifyStory({ story_id, summary, review_type, claim }) { } async function rejectStory({ story_id, reason }) { - // `exact_role: :orchestrator` (`story_verification_controller.ex:29-30`) — see + // `exact_role: :orchestrator` (`story_verification_controller.ex:30-31`) — see // lib/custody-key.js. const orch = orchestratorKeyArgs(); @@ -1436,7 +1439,7 @@ async function rejectStory({ story_id, reason }) { async function bulkMarkComplete({ stories }) { // stories: [{story_id, summary, review_type}] - // `exact_role: :orchestrator` (`bulk_operations_controller.ex:24-25`) — see + // `exact_role: :orchestrator` (`bulk_operations_controller.ex:25-26`) — see // lib/custody-key.js. const orch = orchestratorKeyArgs(); @@ -1451,7 +1454,7 @@ async function bulkMarkComplete({ stories }) { } async function verifyAllInEpic({ epic_id, review_type, summary }) { - // `exact_role: :orchestrator` (`story_verification_controller.ex:29-30`, `:verify_all`) — + // `exact_role: :orchestrator` (`story_verification_controller.ex:30-31`, `:verify_all`) — // see lib/custody-key.js. const orch = orchestratorKeyArgs(); @@ -3454,7 +3457,7 @@ async function resolveEscalation(args) { } // #846: force-unclaim is `exact_role: :orchestrator` -// (`story_verification_controller.ex:29-30`), so this is the one delivery-loop verb that wants +// (`story_verification_controller.ex:30-31`), so this is the one delivery-loop verb that wants // the ORCH key. Same selection as the other four custody verbs: when LOOPCTL_ORCH_KEY is set it // is sent EXACTLY, because `resolveKey` (`index.js:132-138`) would otherwise discard it for a // global LOOPCTL_API_KEY on the one call it was set for; when it is not set, a global key still @@ -4725,7 +4728,9 @@ const TOOLS = [ "reads pending or contracted — move on. An escalated story is claimable again only " + "after resolve_escalation sends it to queued; a done or failed one never is. Refused " + "409 dependencies_not_met when a story it depends on, or one in an epic its epic " + - "depends on, is not verified: list_blocked_stories names them.", + "depends on, is not verified: list_blocked_stories names them. Refused 409 " + + "chg_root_review_only when the key descends from an intake source's " + + "chg_root_dispatch_id: chg-server's keys may only review.", inputSchema: { type: "object", properties: { @@ -4871,7 +4876,9 @@ const TOOLS = [ description: "Reviewer (a DIFFERENT agent from the implementer) confirms the implementation is done. " + "Chain-of-custody: the implementing agent cannot call this. " + - "Transitions implementing -> reported_done. Uses the ORCH key.", + "Transitions implementing -> reported_done. Uses the ORCH key. Refused 409 " + + "chg_root_review_only when the key descends from an intake source's " + + "chg_root_dispatch_id: chg-server's keys may only review.", inputSchema: { type: "object", properties: { @@ -4906,7 +4913,16 @@ const TOOLS = [ name: "review_complete", description: "Record that a review has been completed for a story. " + - "Must be called before verify_story. Uses the ORCH key.", + "Must be called before verify_story. Uses the ORCH key. " + + "On a story BOUND TO A chg CHANGE (loopctl US-47.5) three more rules apply, after the " + + "existing custody gates (self_review_blocked, caller_lineage_required, " + + "unresolvable_dispatch_lineage), never instead of them: the key must be one chg-server " + + "minted for the review — a strict descendant of the intake source's " + + "chg_root_dispatch_id — else 409 chg_reviewer_required (chg-server's root key is " + + "refused too); commit_sha is REQUIRED and must be the sha of a submitted revision of " + + "the story's current change, else 422 commit_sha_not_bound; and one reviewer dispatch " + + "records one verdict, a second is 409 chg_review_already_recorded. On any other story " + + "commit_sha is ignored and not stored.", inputSchema: { type: "object", properties: { @@ -4935,6 +4951,13 @@ const TOOLS = [ type: "string", description: "Optional: summary of the review outcome.", }, + commit_sha: { + type: "string", + description: + "REQUIRED on a story bound to a chg change: the reviewed revision commit, 40 or " + + "64 lowercase hex characters, the sha of a submitted revision of the story's " + + "current change (422 commit_sha_not_bound otherwise). Ignored on any other story.", + }, claim: CLAIM_SCHEMA, }, required: ["story_id", "review_type"], @@ -4947,7 +4970,15 @@ const TOOLS = [ description: "Orchestrator verifies a reported_done story. " + "Requires a review_record to exist (call review_complete first). " + - "Transitions reported_done -> verified. Uses the ORCH key.", + "Transitions reported_done -> verified. Uses the ORCH key. Refused 409 " + + "chg_root_cannot_verify when the key descends from an intake source's " + + "chg_root_dispatch_id (loopctl US-47.5): chg-server's keys review and never verify — " + + "use an orchestrator key outside chg's tree. A story BOUND TO A chg CHANGE is verified " + + "only on a chg review of its LANDED revision (the revision commit of the newest " + + "applied landed event of its current change), else 409 " + + "chg_review_not_for_landed_revision — have chg dispatch a review naming that sha; and " + + "never while its stage is escalated over review_missing, else 409 " + + "review_missing_escalated — a human resolves it first (resolve_escalation).", inputSchema: { type: "object", properties: { @@ -4976,7 +5007,10 @@ const TOOLS = [ "The reject releases the claim, and a delivery story may be re-contracted or escalated " + "in the same transaction. A 500 `audit_chain_append_failed` (the escalation's chain " + "entry was refused) means the WHOLE call rolled back: the story is unchanged, and a " + - "re-run meets the same server-side condition until an operator acts.", + "re-run meets the same server-side condition until an operator acts. Refused 409 " + + "chg_root_cannot_verify when the key descends from an intake source's " + + "chg_root_dispatch_id (loopctl US-47.5): reject is a custody decision chg-server's " + + "keys never make.", inputSchema: { type: "object", properties: { @@ -4999,7 +5033,9 @@ const TOOLS = [ description: "Bulk mark multiple stories as complete (backfill-only for never-dispatched work). " + "ADMIN USE ONLY: backfill pre-existing completed work into pending stories that never entered the dispatch lifecycle. " + - "For dispatched stories, use the normal report/review/verify flow. Each story entry needs a story_id, summary, and review_type. Uses the ORCH key.", + "For dispatched stories, use the normal report/review/verify flow. Each story entry needs a story_id, summary, and review_type. Uses the ORCH key. " + + "Refused 409 chg_root_cannot_verify for the whole batch when the key descends from an intake source's chg_root_dispatch_id (loopctl US-47.5). " + + "A story bound to a chg change is a per-story error leading with review_missing_escalated or chg_review_not_for_landed_revision, the same landed-review gate verify_story applies.", inputSchema: { type: "object", properties: { @@ -5033,7 +5069,11 @@ const TOOLS = [ name: "verify_all_in_epic", description: "Bulk verify all reported_done, unverified stories in an epic. " + - "Convenience endpoint for the orchestrator after a review pass. Uses the ORCH key.", + "Convenience endpoint for the orchestrator after a review pass. Uses the ORCH key. " + + "Refused 409 chg_root_cannot_verify for the whole epic, verifying nothing, when the key " + + "descends from an intake source's chg_root_dispatch_id (loopctl US-47.5). A story bound " + + "to a chg change is skipped into errors with review_missing_escalated or " + + "chg_review_not_for_landed_revision, the same landed-review gate verify_story applies.", inputSchema: { type: "object", properties: { diff --git a/mcp-server/lib/custody-key.js b/mcp-server/lib/custody-key.js index 7f749cc40..d6a2eb20c 100644 --- a/mcp-server/lib/custody-key.js +++ b/mcp-server/lib/custody-key.js @@ -2,9 +2,9 @@ * Key selection for the `exact_role: :orchestrator` custody verbs (loopctl #846 review round 2). * * WHICH VERBS. `verify`, `reject` and `force_unclaim` plus the aggregate `verify_all` - * (`lib/loopctl_web/controllers/story_verification_controller.ex:29-30`), and bulk + * (`lib/loopctl_web/controllers/story_verification_controller.ex:30-31`), and bulk * `verify`/`reject`/`mark_complete` - * (`lib/loopctl_web/controllers/bulk_operations_controller.ex:24-25`). All are mounted + * (`lib/loopctl_web/controllers/bulk_operations_controller.ex:25-26`). All are mounted * `exact_role: :orchestrator`, where the role hierarchy does NOT apply: `RequireRole`'s * exact-role clause tests `api_key.role == exact_role` (`require_role.ex:65-75`), so a `:user` * or `:superadmin` key is refused there exactly as an `:agent` key is. diff --git a/mcp-server/lib/delivery-loop.js b/mcp-server/lib/delivery-loop.js index 97f4814e5..ce1163683 100644 --- a/mcp-server/lib/delivery-loop.js +++ b/mcp-server/lib/delivery-loop.js @@ -21,7 +21,7 @@ * - `resolve_escalation` is the HUMAN half of the escalation pair: a `:user` key no dispatch * minted. The agent key that raises an escalation is 403'd on it by design. * - `force_unclaim_story` is gated `exact_role: :orchestrator` - * (`story_verification_controller.ex:29-30`), so it needs an ORCHESTRATOR-ROLE key and a + * (`story_verification_controller.ex:30-31`), so it needs an ORCHESTRATOR-ROLE key and a * higher-privileged one does NOT substitute: `RequireRole`'s exact-role clause tests * `api_key.role == exact_role` (`require_role.ex:65-75`), so a `:user` or `:superadmin` * key is 403'd there exactly as an agent key is. That is a chain-of-custody gate, not an @@ -293,7 +293,7 @@ export async function resolveEscalation({ story_id, to, reason } = {}, { userKey * No request body — the story is named in the path. * * ORCHESTRATOR ROLE, and exactly that: the action is `exact_role: :orchestrator` - * (`story_verification_controller.ex:29-30`), so a user or superadmin key is refused — + * (`story_verification_controller.ex:30-31`), so a user or superadmin key is refused — * `RequireRole`'s exact-role clause tests `api_key.role == exact_role` * (`require_role.ex:65-75`). * diff --git a/mcp-server/package-lock.json b/mcp-server/package-lock.json index 0371f2e1d..a9f8fe9db 100644 --- a/mcp-server/package-lock.json +++ b/mcp-server/package-lock.json @@ -1,12 +1,12 @@ { "name": "loopctl-mcp-server", - "version": "3.2.0", + "version": "3.3.0", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "loopctl-mcp-server", - "version": "3.2.0", + "version": "3.3.0", "license": "MIT", "dependencies": { "@modelcontextprotocol/sdk": "^1.28.0" diff --git a/mcp-server/package.json b/mcp-server/package.json index 3f4bd2c5c..1c9cda52f 100644 --- a/mcp-server/package.json +++ b/mcp-server/package.json @@ -1,6 +1,6 @@ { "name": "loopctl-mcp-server", - "version": "3.2.0", + "version": "3.3.0", "description": "MCP server for loopctl — structural trust for AI development loops", "type": "module", "main": "index.js", diff --git a/mcp-server/test/chg_review_custody_tools.test.js b/mcp-server/test/chg_review_custody_tools.test.js new file mode 100644 index 000000000..dc345c6de --- /dev/null +++ b/mcp-server/test/chg_review_custody_tools.test.js @@ -0,0 +1,87 @@ +/** + * loopctl US-47.5 on the MCP side: review custody for chg-dispatched reviews. + * + * - `review_complete` declares `commit_sha` and its handler FORWARDS it (AC-47.5.2), so a chg + * reviewer can name the revision it reviewed through this server at all. + * - Each custody tool's description states the refusal the endpoint now answers and when + * (CLAUDE.md: "say in the tool description what the endpoint REFUSES"), because a session + * reads the description instead of the controller. + * + * Run: node --test test/*.test.js + */ + +import { test, describe } from "node:test"; +import assert from "node:assert/strict"; +import { readFileSync } from "node:fs"; +import { fileURLToPath } from "node:url"; +import path from "node:path"; + +import { loadTools, stripComments } from "./tool-surface.js"; + +const DIR = path.dirname(fileURLToPath(import.meta.url)); +const INDEX_SRC = readFileSync(path.join(DIR, "..", "index.js"), "utf8"); +const TOOLS = loadTools(); + +function tool(name) { + const found = TOOLS.find((t) => t.name === name); + assert.ok(found, `tool ${name} is not declared`); + return found; +} + +// One top-level async function's source, bounded at the next one, comments stripped — so an +// assertion cannot pass on a later function's body or on commented-out wiring. +function functionSource(name) { + const declaration = `async function ${name}(`; + const start = INDEX_SRC.indexOf(declaration); + assert.notEqual(start, -1, `index.js must define ${declaration}`); + const rest = INDEX_SRC.slice(start + declaration.length); + const next = rest.indexOf("\nasync function "); + return stripComments(next === -1 ? rest : rest.slice(0, next)); +} + +describe("review_complete carries commit_sha (AC-47.5.2)", () => { + test("the schema declares it, optional, as a string", () => { + const schema = tool("review_complete").inputSchema; + assert.equal(schema.properties.commit_sha.type, "string"); + assert.ok(!schema.required.includes("commit_sha"), "commit_sha is required only server-side"); + assert.match(schema.properties.commit_sha.description, /REQUIRED on a story bound to a chg change/); + }); + + test("the handler destructures it and puts it in the request body", () => { + const src = functionSource("reviewComplete"); + assert.match(src, /^\{[^}]*\bcommit_sha\b[^}]*\}/, "reviewComplete must take commit_sha"); + assert.match(src, /body\.commit_sha\s*=\s*commit_sha/, "reviewComplete must forward commit_sha"); + }); +}); + +describe("descriptions name the US-47.5 refusals", () => { + test("review_complete: chg_reviewer_required, commit_sha_not_bound, chg_review_already_recorded", () => { + const d = tool("review_complete").description; + assert.match(d, /409 chg_reviewer_required/); + assert.match(d, /422 commit_sha_not_bound/); + assert.match(d, /409 chg_review_already_recorded/); + assert.match(d, /commit_sha is REQUIRED/); + // The new gates are additive; the description must not read as replacing the old ones. + assert.match(d, /never instead of them/); + }); + + for (const name of ["verify_story", "reject_story", "verify_all_in_epic", "bulk_mark_complete"]) { + test(`${name}: 409 chg_root_cannot_verify`, () => { + assert.match(tool(name).description, /409 chg_root_cannot_verify/); + }); + } + + for (const name of ["verify_story", "verify_all_in_epic", "bulk_mark_complete"]) { + test(`${name}: the chg landed-review gate (review_missing_escalated, chg_review_not_for_landed_revision)`, () => { + const d = tool(name).description; + assert.match(d, /review_missing_escalated/); + assert.match(d, /chg_review_not_for_landed_revision/); + }); + } + + for (const name of ["claim_story", "report_story"]) { + test(`${name}: 409 chg_root_review_only`, () => { + assert.match(tool(name).description, /409 chg_root_review_only/); + }); + } +}); diff --git a/mcp-server/test/custody_key_pinning.test.js b/mcp-server/test/custody_key_pinning.test.js index 3d64fc08f..ac55fb31b 100644 --- a/mcp-server/test/custody_key_pinning.test.js +++ b/mcp-server/test/custody_key_pinning.test.js @@ -25,8 +25,8 @@ * * The five verbs below are `exact_role: :orchestrator` in loopctl: * - * - verify / reject / force_unclaim / verify-all — `story_verification_controller.ex:29-30` - * - bulk mark-complete — `bulk_operations_controller.ex:24-25` + * - verify / reject / force_unclaim / verify-all — `story_verification_controller.ex:30-31` + * - bulk mark-complete — `bulk_operations_controller.ex:25-26` * * WHAT IS DELIBERATELY NOT HERE. `report` (`exact_role: [:agent, :orchestrator]`) and * `review-complete` (`exact_role: [:orchestrator, :user]`) are LIST-form gates, and for them the @@ -154,7 +154,7 @@ const EXACT_ROLE_ORCHESTRATOR = [ ["bulk_mark_complete", "bulkMarkComplete"], ["verify_all_in_epic", "verifyAllInEpic"], // #846 round 2: force_unclaim_story is on the SAME plug line as verify and reject - // (`story_verification_controller.ex:29-30`) and was the one pinned custody verb missing + // (`story_verification_controller.ex:30-31`) and was the one pinned custody verb missing // from this table — and, before this round, the one sending no `keyHint`, so a missing key // was reported as an LLM-configuration problem. ["force_unclaim_story", "forceUnclaimStory"], diff --git a/mcp-server/test/delivery_loop_tools.test.js b/mcp-server/test/delivery_loop_tools.test.js index 73d08b7d7..1ddcc659e 100644 --- a/mcp-server/test/delivery_loop_tools.test.js +++ b/mcp-server/test/delivery_loop_tools.test.js @@ -524,7 +524,7 @@ describe("the wiring in index.js", () => { }); test("force_unclaim_story selects its key the way every exact_role verb does", () => { - // The endpoint is `exact_role: :orchestrator` (`story_verification_controller.ex:29-30`), + // The endpoint is `exact_role: :orchestrator` (`story_verification_controller.ex:30-31`), // so it needs an orchestrator-ROLE key; which env var holds one is a separate question // and `lib/custody-key.js` answers it — LOOPCTL_ORCH_KEY sent exactly when set, a global // LOOPCTL_API_KEY otherwise. What this pins is that the handler does not answer it itself. diff --git a/priv/repo/migrations/20261019170000_add_chg_review_custody_to_review_records.exs b/priv/repo/migrations/20261019170000_add_chg_review_custody_to_review_records.exs new file mode 100644 index 000000000..6bf45ba38 --- /dev/null +++ b/priv/repo/migrations/20261019170000_add_chg_review_custody_to_review_records.exs @@ -0,0 +1,64 @@ +defmodule Loopctl.Repo.Migrations.AddChgReviewCustodyToReviewRecords do + @moduledoc """ + US-47.5: review custody for chg-dispatched reviews (Epic 47 PRD §6). + + - `review_records.commit_sha` — the revision commit a chg reviewer reviewed: the envelope + `sha` of a `submitted` event of the story's current change binding. Required for a story + bound to a chg change and never written for any other (`Loopctl.Progress.record_review/4`), + so a verdict on revision 1 is never read as covering revision 2, and a review recorded + BEFORE the story was bound never counts as a chg review. + - `review_records.reviewer_dispatch_id` — the chg reviewer's own dispatch (the leaf of its + server-resolved lineage). Written for the same reviews and no others. + - `review_records_reviewer_dispatch_uidx` — ONE VERDICT PER REVIEWER DISPATCH, held by the + database rather than by a read-then-write: chg mints one child dispatch per + `(change_id, revision)` review, so a second verdict from the same key is a retry or a + replay, never a second review. Unique on the dispatch id alone (dispatch ids are global + UUIDs, so it is the same rule as `(tenant_id, reviewer_dispatch_id)`), which also serves the + referential check a dispatch delete runs (`20260928140000_index_foreign_keys_into_dispatches`). + - `review_records_chg_review_shape` — the two columns are set together or not at all. + + Additive: two nullable columns, a CHECK every existing row satisfies (both NULL) and a + partial index over no rows yet, so the code running before this release keeps working + against the new schema during Fly's rolling deploy. `review_records` is small (one row per + review), so the index is built in the migration's transaction. + + The reference to `dispatches` is single-column and `ON DELETE NO ACTION`, like every other + one (`20261019140000_add_chg_mode_to_intake_sources` says why). + """ + + use Ecto.Migration + + @sha "'^[0-9a-f]{40}([0-9a-f]{24})?$'" + + def up do + alter table(:review_records) do + add :commit_sha, :text, null: true + add :reviewer_dispatch_id, references(:dispatches, type: :binary_id, on_delete: :nothing) + end + + create constraint(:review_records, :review_records_commit_sha, check: "commit_sha ~ #{@sha}") + + create constraint(:review_records, :review_records_chg_review_shape, + check: "(commit_sha IS NULL) = (reviewer_dispatch_id IS NULL)" + ) + + create unique_index(:review_records, [:reviewer_dispatch_id], + where: "reviewer_dispatch_id IS NOT NULL", + name: :review_records_reviewer_dispatch_uidx + ) + end + + def down do + drop index(:review_records, [:reviewer_dispatch_id], + name: :review_records_reviewer_dispatch_uidx + ) + + drop constraint(:review_records, :review_records_chg_review_shape) + drop constraint(:review_records, :review_records_commit_sha) + + alter table(:review_records) do + remove :reviewer_dispatch_id + remove :commit_sha + end + end +end diff --git a/test/loopctl/changes/review_custody_lock_test.exs b/test/loopctl/changes/review_custody_lock_test.exs new file mode 100644 index 000000000..355194544 --- /dev/null +++ b/test/loopctl/changes/review_custody_lock_test.exs @@ -0,0 +1,206 @@ +defmodule Loopctl.Changes.ReviewCustodyLockTest do + @moduledoc """ + US-47.5 AC-47.5.1 under GENUINE concurrency: one chg reviewer dispatch records at most one + verdict, and the DATABASE decides it (`review_records_reviewer_dispatch_uidx`), not a read + before the write. + + `async: false` because the subject is committed transactions racing each other: inside the + SQL sandbox every allowed process shares one connection, which serialises the two + `record_review/4` calls on its own and would pass with no index at all. So the rows are + COMMITTED under `fixture(:committed_tenant)`, every process runs on production's + connections (`Loopctl.Test.ProductionTopology`), and the module sweeps exactly the tenants it + committed when it ends (`track_committed_tenants/0`). + """ + + use ExUnit.Case, async: false + + import Ecto.Query, only: [from: 2] + import Loopctl.Fixtures + + alias Loopctl.AdminRepo + alias Loopctl.Artifacts.ReviewRecord + alias Loopctl.Changes.Events + alias Loopctl.Changes.Signature + alias Loopctl.Dispatches + alias Loopctl.Progress + alias Loopctl.Repo + alias Loopctl.Test.ProductionTopology + + @ownership_timeout :timer.seconds(60) + @sha String.duplicate("a", 40) + + setup_all do + %{ledger: track_committed_tenants()} + end + + setup %{ledger: ledger} do + tenant = fixture(:committed_tenant, %{}) + track_committed_tenant(ledger, tenant.id) + :ok = ProductionTopology.checkout_unboxed!([Repo, AdminRepo]) + + # A recorded review enqueues the knowledge-extraction job, which the test Oban engine runs + # INLINE in the recording process — off the subject here. The tenant's own setting turns + # it off (an input, rather than global mocks for every racing process). + tenant = + tenant + |> Ecto.Changeset.change(settings: %{"knowledge_auto_extract" => false}) + |> AdminRepo.update!() + + %{secret: secret, source: source, root: root} = fixture(:chg_source, %{tenant_id: tenant.id}) + + impl_agent = fixture(:agent, %{tenant_id: tenant.id, agent_type: :implementer}) + + {:ok, %{dispatch: impl}} = + Dispatches.create_dispatch(tenant.id, %{role: :agent, agent_id: impl_agent.id}) + + story = + fixture(:story, %{tenant_id: tenant.id}) + |> Ecto.Changeset.change(%{ + agent_status: :reported_done, + reported_done_at: DateTime.add(DateTime.utc_now(), -60, :second), + assigned_agent_id: impl_agent.id, + implementer_dispatch_id: impl.id + }) + |> AdminRepo.update!() + + raw = + Jason.encode!( + build(:chg_event, %{ + change_id: "C1", + event_seq: 1, + kind: "submitted", + sha: @sha, + data: %{"story_id" => story.id} + }) + ) + + ts = Integer.to_string(System.os_time(:second)) + + {:ok, %{outcome: :applied}} = + Events.receive_push(source.id, %{ + raw_body: raw, + timestamp: ts, + signature: Signature.header(secret, Signature.delivery_string(ts, raw)) + }) + + reviewer = fixture(:chg_root_dispatch, %{tenant_id: tenant.id, parent: root}) + agent = fixture(:agent, %{tenant_id: tenant.id}) + + %{tenant: tenant, story: story, reviewer: reviewer, agent: agent} + end + + defp review(ctx) do + Progress.record_review( + ctx.tenant.id, + ctx.story.id, + %{"review_type" => "chg", "commit_sha" => @sha}, + reviewer_agent_id: ctx.agent.id, + reviewer_lineage: ctx.reviewer.lineage_path + ) + end + + defp review_in_task(ctx) do + Task.async(fn -> + :ok = + ProductionTopology.checkout_unboxed!([Repo, AdminRepo], + ownership_timeout: @ownership_timeout + ) + + review(ctx) + end) + end + + # Holds an UNCOMMITTED verdict of the same reviewer dispatch in its own session. + defp hold_verdict(ctx, outcome) do + parent = self() + + Task.async(fn -> + :ok = + ProductionTopology.checkout_unboxed!([AdminRepo], ownership_timeout: @ownership_timeout) + + AdminRepo.transaction(fn -> + AdminRepo.insert!(%ReviewRecord{ + tenant_id: ctx.tenant.id, + story_id: ctx.story.id, + review_type: "chg", + completed_at: DateTime.utc_now(), + reviewed_report_at: ctx.story.reported_done_at, + commit_sha: @sha, + reviewer_dispatch_id: ctx.reviewer.id + }) + + %{rows: [[pid]]} = AdminRepo.query!("SELECT pg_backend_pid()", []) + send(parent, {:holding, pid}) + + receive do + :commit -> :ok + :rollback -> AdminRepo.rollback(:held) + end + end) + end) + |> then(fn task -> + receive do + {:holding, pid} -> {task, pid, outcome} + after + 10_000 -> flunk("the holder never inserted its verdict") + end + end) + end + + defp release({task, _pid, outcome}) do + send(task.pid, outcome) + Task.await(task, 30_000) + end + + defp blocked_on?(backend) do + Enum.any?(1..200, fn _ -> + %{rows: [[n]]} = + AdminRepo.query!( + "SELECT count(*) FROM pg_stat_activity WHERE $1 = ANY(pg_blocking_pids(pid))", + [backend] + ) + + n > 0 or (Process.sleep(25) && false) + end) + end + + defp verdicts(ctx) do + AdminRepo.aggregate( + from(r in ReviewRecord, where: r.reviewer_dispatch_id == ^ctx.reviewer.id), + :count + ) + end + + test "a verdict racing an uncommitted one waits on the index and is refused when it commits", + ctx do + holding = hold_verdict(ctx, :commit) + racer = review_in_task(ctx) + + # The second verdict is BLOCKED on the first's unique-index entry: no read-then-write + # could have seen the uncommitted row, so this wait is the database deciding. + assert blocked_on?(elem(holding, 1)) + assert {:ok, _} = release(holding) + + assert {:error, :chg_review_already_recorded} = Task.await(racer, 30_000) + assert verdicts(ctx) == 1 + end + + test "the same race with the first verdict rolled back records the second", ctx do + holding = hold_verdict(ctx, :rollback) + racer = review_in_task(ctx) + + assert blocked_on?(elem(holding, 1)) + assert {:error, :held} = release(holding) + + assert {:ok, %ReviewRecord{}} = Task.await(racer, 30_000) + assert verdicts(ctx) == 1 + end + + test "two verdicts from one reviewer dispatch at once: exactly one is recorded", ctx do + results = [review_in_task(ctx), review_in_task(ctx)] |> Enum.map(&Task.await(&1, 30_000)) + + assert Enum.count(results, &match?({:ok, %ReviewRecord{}}, &1)) == 1 + assert Enum.count(results, &(&1 == {:error, :chg_review_already_recorded})) == 1 + assert verdicts(ctx) == 1 + end +end diff --git a/test/loopctl/changes/review_custody_test.exs b/test/loopctl/changes/review_custody_test.exs new file mode 100644 index 000000000..0c6b832a3 --- /dev/null +++ b/test/loopctl/changes/review_custody_test.exs @@ -0,0 +1,1079 @@ +defmodule Loopctl.Changes.ReviewCustodyTest do + @moduledoc """ + US-47.5: review custody for chg-dispatched reviews (Epic 47 PRD §6), at the context: + `Loopctl.Progress.record_review/4` on a story bound to a chg change (AC-47.5.1..3), the + chg-root refusal on every terminal custody path and on claim/report (AC-47.5.4), and the + `:review_missing` escalation a landing takes when no chg review names its revision + (AC-47.5.5). The HTTP surface, the response bodies and the OpenAPI declarations are + `LoopctlWeb.ChgReviewCustodyControllerTest`'s; the race on one reviewer dispatch's verdict is + `Loopctl.Changes.ReviewCustodyLockTest`'s. + + Every lineage here is a `dispatches.lineage_path` the fixture wrote, standing in for what + `Loopctl.Dispatches.lineage_for_api_key/2` resolves server-side from a key — the context never + reads it from params. + """ + + use Loopctl.DataCase, async: true + + import Ecto.Query + + alias Loopctl.AdminRepo + alias Loopctl.Artifacts.ReviewRecord + alias Loopctl.AuditChain.Entry + alias Loopctl.BulkOperations + alias Loopctl.Changes.Events + alias Loopctl.Changes.ReviewCustody + alias Loopctl.Changes.Signature + alias Loopctl.Delivery.Escalations + alias Loopctl.Delivery.Stages + alias Loopctl.Dispatches + alias Loopctl.Intake + alias Loopctl.PlanAssertions + alias Loopctl.Progress + alias Loopctl.WorkBreakdown.Story + + setup :verify_on_exit! + + @repo "mkreyman/infra" + + # ── fixtures ──────────────────────────────────────────────────────────────── + + setup do + %{secret: secret, source: source, root: root} = fixture(:chg_source, %{}) + tenant_id = source.tenant_id + + # The implementer is dispatch-minted under its OWN root — the delivery loop's, never chg's. + impl_agent = fixture(:agent, %{tenant_id: tenant_id, agent_type: :implementer}) + + {:ok, %{dispatch: impl}} = + Dispatches.create_dispatch(tenant_id, %{role: :agent, agent_id: impl_agent.id}) + + epic = fixture(:epic, %{tenant_id: tenant_id}) + + story = + fixture(:story, %{tenant_id: tenant_id, epic_id: epic.id}) + |> Ecto.Changeset.change(%{ + agent_status: :reported_done, + reported_done_at: DateTime.add(DateTime.utc_now(), -60, :second), + assigned_agent_id: impl_agent.id, + implementer_dispatch_id: impl.id + }) + |> AdminRepo.update!() + + %{ + tenant_id: tenant_id, + secret: secret, + source: source, + root: root, + impl: impl, + impl_agent: impl_agent, + epic: epic, + story: story + } + end + + defp push(ctx, event, opts \\ []) do + raw = Jason.encode!(event) + ts = Integer.to_string(System.os_time(:second)) + signature = Signature.header(ctx.secret, Signature.delivery_string(ts, raw)) + + Events.receive_push( + ctx.source.id, + %{raw_body: raw, timestamp: ts, signature: signature}, + opts + ) + end + + defp event(change_id, seq, kind, attrs) do + build(:chg_event, Map.merge(%{change_id: change_id, event_seq: seq, kind: kind}, attrs)) + end + + defp sha(char), do: String.duplicate(char, 40) + + # Binds `story` to `change_id` the way production does: chg reports `submitted` for it. + defp submit!(ctx, story, change_id, seq, revision, revision_sha) do + assert {:ok, %{outcome: :applied}} = + push( + ctx, + event(change_id, seq, "submitted", %{ + revision: revision, + sha: revision_sha, + data: %{"story_id" => story.id} + }) + ) + + :ok + end + + defp child(ctx, parent \\ nil), + do: fixture(:chg_root_dispatch, %{tenant_id: ctx.tenant_id, parent: parent || ctx.root}) + + defp reviewer_agent(ctx), do: fixture(:agent, %{tenant_id: ctx.tenant_id}) + + defp review(ctx, story, lineage, params, agent_id \\ :fresh) do + agent_id = if agent_id == :fresh, do: reviewer_agent(ctx).id, else: agent_id + + Progress.record_review( + ctx.tenant_id, + story.id, + Map.merge(%{"review_type" => "chg", "summary" => "chg review"}, params), + reviewer_agent_id: agent_id, + reviewer_lineage: lineage + ) + end + + # An independently-rooted orchestrator dispatch's child: outside chg's subtree. + defp outsider(ctx) do + {:ok, %{dispatch: other_root}} = + Dispatches.create_dispatch(ctx.tenant_id, %{ + role: :orchestrator, + agent_id: reviewer_agent(ctx).id + }) + + {:ok, %{dispatch: other}} = + Dispatches.create_dispatch(ctx.tenant_id, %{ + role: :orchestrator, + agent_id: reviewer_agent(ctx).id, + parent_dispatch_id: other_root.id + }) + + other + end + + # ── AC-47.5.1: who may review ─────────────────────────────────────────────── + + describe "AC-47.5.1: a chg story takes its review from a strict descendant of the chg root" do + test "TC-47.5.1: a child of the chg root naming the revision is recorded", ctx do + :ok = submit!(ctx, ctx.story, "C1", 1, 1, sha("a")) + reviewer = child(ctx) + + assert {:ok, %ReviewRecord{} = record} = + review(ctx, ctx.story, reviewer.lineage_path, %{"commit_sha" => sha("a")}) + + assert record.commit_sha == sha("a") + assert record.reviewer_dispatch_id == reviewer.id + assert AdminRepo.get!(ReviewRecord, record.id).commit_sha == sha("a") + end + + test "a grandchild is a strict descendant too, and records its own dispatch", ctx do + :ok = submit!(ctx, ctx.story, "C1", 1, 1, sha("a")) + grandchild = child(ctx, child(ctx)) + + assert {:ok, record} = + review(ctx, ctx.story, grandchild.lineage_path, %{"commit_sha" => sha("a")}) + + assert record.reviewer_dispatch_id == grandchild.id + end + + test "chg-server's ROOT key is refused chg_reviewer_required", ctx do + :ok = submit!(ctx, ctx.story, "C1", 1, 1, sha("a")) + + assert {:error, :chg_reviewer_required} = + review(ctx, ctx.story, ctx.root.lineage_path, %{"commit_sha" => sha("a")}) + end + + test "TC-47.5.2: an orchestrator outside chg's subtree is refused chg_reviewer_required", + ctx do + :ok = submit!(ctx, ctx.story, "C1", 1, 1, sha("a")) + + assert {:error, :chg_reviewer_required} = + review(ctx, ctx.story, outsider(ctx).lineage_path, %{"commit_sha" => sha("a")}) + end + + test "a child of ANOTHER registered chg root is refused: the change's own root decides", + ctx do + :ok = submit!(ctx, ctx.story, "C1", 1, 1, sha("a")) + + %{root: other_root} = + fixture(:chg_source, %{tenant_id: ctx.tenant_id, repo_full_name: "mkreyman/other"}) + + foreign = fixture(:chg_root_dispatch, %{tenant_id: ctx.tenant_id, parent: other_root}) + + assert {:error, :chg_reviewer_required} = + review(ctx, ctx.story, foreign.lineage_path, %{"commit_sha" => sha("a")}) + end + + test "a human operator (nil agent, no lineage) is refused on a chg story", ctx do + :ok = submit!(ctx, ctx.story, "C1", 1, 1, sha("a")) + + assert {:error, :chg_reviewer_required} = + review(ctx, ctx.story, [], %{"commit_sha" => sha("a")}, nil) + end + + test "a bound change whose events no source delivered has no root, so nobody qualifies", + ctx do + # A binding written without any event (US-47.1's `bind/4` directly): no source, no root. + fixture(:change_binding, %{tenant_id: ctx.tenant_id, story_id: ctx.story.id, repo: @repo}) + + assert {:error, :chg_reviewer_required} = + review(ctx, ctx.story, child(ctx).lineage_path, %{"commit_sha" => sha("a")}) + end + + test "one reviewer dispatch records one verdict; a new dispatch records its own", ctx do + :ok = submit!(ctx, ctx.story, "C1", 1, 1, sha("a")) + reviewer = child(ctx) + + assert {:ok, _} = review(ctx, ctx.story, reviewer.lineage_path, %{"commit_sha" => sha("a")}) + + assert {:error, :chg_review_already_recorded} = + review(ctx, ctx.story, reviewer.lineage_path, %{"commit_sha" => sha("a")}) + + assert {:ok, _} = + review(ctx, ctx.story, child(ctx).lineage_path, %{"commit_sha" => sha("a")}) + + assert AdminRepo.aggregate( + from(r in ReviewRecord, where: r.reviewer_dispatch_id == ^reviewer.id), + :count + ) == 1 + end + + test "the database holds the one-verdict rule, not only the context", ctx do + reviewer = child(ctx) + + fixture(:review_record, %{ + tenant_id: ctx.tenant_id, + story_id: ctx.story.id, + commit_sha: sha("a"), + reviewer_dispatch_id: reviewer.id + }) + + assert_raise Ecto.ConstraintError, ~r/review_records_reviewer_dispatch_uidx/, fn -> + AdminRepo.insert!(%ReviewRecord{ + tenant_id: ctx.tenant_id, + story_id: ctx.story.id, + review_type: "chg", + completed_at: DateTime.utc_now(), + commit_sha: sha("b"), + reviewer_dispatch_id: reviewer.id + }) + end + end + + test "the database refuses a commit_sha without a reviewer dispatch, and the reverse", ctx do + for {commit_sha, dispatch_id} <- [{sha("a"), nil}, {nil, child(ctx).id}] do + assert_raise Ecto.ConstraintError, ~r/review_records_chg_review_shape/, fn -> + AdminRepo.insert!(%ReviewRecord{ + tenant_id: ctx.tenant_id, + story_id: ctx.story.id, + review_type: "chg", + completed_at: DateTime.utc_now(), + commit_sha: commit_sha, + reviewer_dispatch_id: dispatch_id + }) + end + end + end + end + + # ── AC-47.5.2: which revision ─────────────────────────────────────────────── + + describe "AC-47.5.2: commit_sha must be a revision of the story's CURRENT binding" do + test "absent or malformed is commit_sha_not_bound", ctx do + :ok = submit!(ctx, ctx.story, "C1", 1, 1, sha("a")) + lineage = child(ctx).lineage_path + + for params <- [ + %{}, + %{"commit_sha" => nil}, + %{"commit_sha" => String.upcase(sha("a"))}, + %{"commit_sha" => "abc123"}, + %{"commit_sha" => 42} + ] do + assert {:error, :commit_sha_not_bound} = review(ctx, ctx.story, lineage, params), + inspect(params) + end + end + + test "TC-47.5.3: a revision of a SUPERSEDED binding is not bound; the current one is", ctx do + :ok = submit!(ctx, ctx.story, "C1", 1, 1, sha("a")) + # The story re-binds to a new change; C1 is superseded. + :ok = submit!(ctx, ctx.story, "C2", 1, 1, sha("b")) + + assert {:error, :commit_sha_not_bound} = + review(ctx, ctx.story, child(ctx).lineage_path, %{"commit_sha" => sha("a")}) + + assert {:ok, _} = + review(ctx, ctx.story, child(ctx).lineage_path, %{"commit_sha" => sha("b")}) + end + + test "every submitted revision of the current change is bound, not only the newest", ctx do + :ok = submit!(ctx, ctx.story, "C1", 1, 1, sha("a")) + :ok = submit!(ctx, ctx.story, "C1", 2, 2, sha("b")) + + for revision_sha <- [sha("a"), sha("b")] do + assert {:ok, _} = + review(ctx, ctx.story, child(ctx).lineage_path, %{"commit_sha" => revision_sha}) + end + end + + test "the sha of an event that is not a submitted revision is not bound", ctx do + :ok = submit!(ctx, ctx.story, "C1", 1, 1, sha("a")) + + assert {:ok, _} = + push(ctx, event("C1", 2, "check_passed", %{revision: 1, sha: sha("c")})) + + assert {:error, :commit_sha_not_bound} = + review(ctx, ctx.story, child(ctx).lineage_path, %{"commit_sha" => sha("c")}) + end + + test "a submitted revision held ahead of a gap is not bound until it applies", ctx do + :ok = submit!(ctx, ctx.story, "C1", 1, 1, sha("a")) + + fixture(:change_event, %{ + tenant_id: ctx.tenant_id, + source_id: ctx.source.id, + repo: @repo, + change_id: "C1", + event_seq: 5, + kind: "submitted", + revision: 3, + sha: sha("d"), + applied: false + }) + + assert {:error, :commit_sha_not_bound} = + review(ctx, ctx.story, child(ctx).lineage_path, %{"commit_sha" => sha("d")}) + end + + test "the reviewer is judged before the sha, so an outsider learns nothing about revisions", + ctx do + :ok = submit!(ctx, ctx.story, "C1", 1, 1, sha("a")) + + assert {:error, :chg_reviewer_required} = + review(ctx, ctx.story, outsider(ctx).lineage_path, %{"commit_sha" => "nope"}) + end + end + + # ── AC-47.5.3: the existing gates, unchanged, first ───────────────────────── + + describe "AC-47.5.3: existing refusals keep their codes; chg checks run in addition" do + test "a NON-chg story: any separated reviewer is recorded and commit_sha is not stored", + ctx do + reviewer = outsider(ctx) + + assert {:ok, first} = + review(ctx, ctx.story, reviewer.lineage_path, %{"commit_sha" => sha("a")}) + + assert first.commit_sha == nil + assert first.reviewer_dispatch_id == nil + + # No one-verdict rule off the chg path: an orchestrator dispatch reviews many stories, + # and may record again. + assert {:ok, _} = review(ctx, ctx.story, reviewer.lineage_path, %{}) + assert ReviewCustody.chg_story(ctx.tenant_id, ctx.story.id) == :not_chg + end + + test "a NON-chg story: a human operator is still permitted", ctx do + assert {:ok, _} = review(ctx, ctx.story, [], %{}, nil) + end + + test "a chg story: a caller on the implementer's chain is self_review_blocked", ctx do + :ok = submit!(ctx, ctx.story, "C1", 1, 1, sha("a")) + + {:ok, %{dispatch: sub}} = + Dispatches.create_dispatch(ctx.tenant_id, %{ + role: :orchestrator, + agent_id: reviewer_agent(ctx).id, + parent_dispatch_id: ctx.impl.id + }) + + assert {:error, :self_review_blocked} = + review(ctx, ctx.story, sub.lineage_path, %{"commit_sha" => sha("a")}) + + # The implementer's own agent id, even from inside chg's tree, is self-review too. + assert {:error, :self_review_blocked} = + review( + ctx, + ctx.story, + child(ctx).lineage_path, + %{"commit_sha" => sha("a")}, + ctx.impl_agent.id + ) + end + + test "a chg story: an unlineaged orchestrator is caller_lineage_required", ctx do + :ok = submit!(ctx, ctx.story, "C1", 1, 1, sha("a")) + + assert {:error, :caller_lineage_required} = + review(ctx, ctx.story, [], %{"commit_sha" => sha("a")}) + end + + test "a chg story: an unresolvable implementer dispatch is unresolvable_dispatch_lineage", + ctx do + :ok = submit!(ctx, ctx.story, "C1", 1, 1, sha("a")) + + # Another tenant's dispatch: the FK is table-wide, the read is tenant-scoped. + foreign = fixture(:chg_root_dispatch, %{tenant_id: fixture(:tenant).id}) + + story = + ctx.story + |> Ecto.Changeset.change(implementer_dispatch_id: foreign.id) + |> AdminRepo.update!() + + assert {:error, :unresolvable_dispatch_lineage} = + review(ctx, story, child(ctx).lineage_path, %{"commit_sha" => sha("a")}) + end + + test "story_not_reported_done is still the first answer on a chg story", ctx do + :ok = submit!(ctx, ctx.story, "C1", 1, 1, sha("a")) + + story = + ctx.story |> Ecto.Changeset.change(agent_status: :implementing) |> AdminRepo.update!() + + assert {:error, :story_not_reported_done} = + review(ctx, story, outsider(ctx).lineage_path, %{}) + end + end + + # ── AC-47.5.4: chg-server's keys never certify, implement or report ───────── + + describe "AC-47.5.4: every terminal custody path refuses a chg-rooted caller" do + # A story ANY separated orchestrator could verify: reviewed, reported, outside chg. + defp verifiable(ctx) do + {:ok, _} = review(ctx, ctx.story, outsider(ctx).lineage_path, %{}) + ctx.story + end + + defp chg_opts(lineage, extra \\ []) do + [ + orchestrator_agent_id: Ecto.UUID.generate(), + verifier_lineage: lineage, + caller_minting_lineage: lineage + ] ++ extra + end + + test "TC-47.5.4: verify_story — a chg child and the chg root alike", ctx do + story = verifiable(ctx) + + for lineage <- [child(ctx).lineage_path, ctx.root.lineage_path] do + assert {:error, :chg_root_cannot_verify} = + Progress.verify_story(ctx.tenant_id, story.id, %{}, chg_opts(lineage)) + end + + assert AdminRepo.get!(Story, story.id).verified_status == :unverified + + # The same story is verifiable by an orchestrator outside chg: the refusal is the caller's. + other = outsider(ctx) + + assert {:ok, %Story{verified_status: :verified}} = + Progress.verify_story(ctx.tenant_id, story.id, %{}, + orchestrator_agent_id: other.agent_id, + verifier_lineage: other.lineage_path + ) + end + + test "reject_story — though reject is exempt from caller_lineage_required", ctx do + story = verifiable(ctx) + + assert {:error, :chg_root_cannot_verify} = + Progress.reject_story( + ctx.tenant_id, + story.id, + %{"reason" => "no"}, + chg_opts(child(ctx).lineage_path) + ) + + assert AdminRepo.get!(Story, story.id).verified_status == :unverified + end + + test "an EXPIRED chg key is still chg's: the minting lineage decides, not the live one", + ctx do + story = verifiable(ctx) + expired = child(ctx) + + # An expired dispatch's key resolves to a LIVE lineage of [] — which reject accepts as an + # unlineaged caller. Its minting lineage still names chg's root. + base = [orchestrator_agent_id: reviewer_agent(ctx).id, verifier_lineage: []] + + assert {:error, :chg_root_cannot_verify} = + Progress.reject_story( + ctx.tenant_id, + story.id, + %{"reason" => "no"}, + base ++ [caller_minting_lineage: expired.lineage_path] + ) + + assert {:ok, %Story{verified_status: :rejected}} = + Progress.reject_story(ctx.tenant_id, story.id, %{"reason" => "no"}, base) + end + + test "verify_all_in_epic refuses the whole epic and verifies nothing", ctx do + story = verifiable(ctx) + + assert {:error, :chg_root_cannot_verify} = + Progress.verify_all_in_epic( + ctx.tenant_id, + ctx.epic.id, + %{"summary" => "all"}, + chg_opts(child(ctx).lineage_path) + ) + + assert AdminRepo.get!(Story, story.id).verified_status == :unverified + end + + test "TC-47.5.4: bulk verify and bulk reject refuse the whole batch", ctx do + story = verifiable(ctx) + opts = chg_opts(child(ctx).lineage_path) + + assert {:error, :chg_root_cannot_verify} = + BulkOperations.bulk_verify( + ctx.tenant_id, + [%{"story_id" => story.id}], + Ecto.UUID.generate(), + opts + ) + + assert {:error, :chg_root_cannot_verify} = + BulkOperations.bulk_reject( + ctx.tenant_id, + [%{"story_id" => story.id, "reason" => "no"}], + Ecto.UUID.generate(), + opts + ) + + assert AdminRepo.get!(Story, story.id).verified_status == :unverified + end + + test "backfill and bulk mark-complete, the paths that verify with no review at all", ctx do + pending = fixture(:story, %{tenant_id: ctx.tenant_id, epic_id: ctx.epic.id}) + opts = chg_opts(child(ctx).lineage_path) + + assert {:error, :chg_root_cannot_verify} = + Progress.backfill_story(ctx.tenant_id, pending.id, %{"reason" => "pre"}, opts) + + assert {:error, :chg_root_cannot_verify} = + BulkOperations.bulk_mark_complete( + ctx.tenant_id, + [%{"story_id" => pending.id, "summary" => "pre", "review_type" => "pre"}], + Ecto.UUID.generate(), + opts + ) + + assert AdminRepo.get!(Story, pending.id).verified_status == :unverified + end + + test "claim, bulk claim and report are chg_root_review_only", ctx do + contracted = + fixture(:story, %{tenant_id: ctx.tenant_id, epic_id: ctx.epic.id}) + |> Ecto.Changeset.change(agent_status: :contracted) + |> AdminRepo.update!() + + agent_child = + fixture(:chg_root_dispatch, %{tenant_id: ctx.tenant_id, parent: ctx.root, role: :agent}) + + agent = reviewer_agent(ctx) + + assert {:error, :chg_root_review_only} = + Progress.claim_story(ctx.tenant_id, contracted.id, + agent_id: agent.id, + lineage: agent_child.lineage_path, + caller_minting_lineage: agent_child.lineage_path + ) + + assert {:error, :chg_root_review_only} = + BulkOperations.bulk_claim(ctx.tenant_id, [contracted.id], agent.id, + caller_minting_lineage: agent_child.lineage_path + ) + + assert AdminRepo.get!(Story, contracted.id).agent_status == :contracted + + implementing = + ctx.story + |> Ecto.Changeset.change(agent_status: :implementing, reported_done_at: nil) + |> AdminRepo.update!() + + assert {:error, :chg_root_review_only} = + Progress.report_story(ctx.tenant_id, implementing.id, + agent_id: agent.id, + reporter_lineage: child(ctx).lineage_path + ) + + assert AdminRepo.get!(Story, implementing.id).agent_status == :implementing + end + + test "a revoked source, or one switched back to pr, still marks its root", ctx do + story = verifiable(ctx) + lineage = child(ctx).lineage_path + + {:ok, _} = Intake.revoke_source(ctx.tenant_id, ctx.source.id) + + assert {:error, :chg_root_cannot_verify} = + Progress.verify_story(ctx.tenant_id, story.id, %{}, chg_opts(lineage)) + + AdminRepo.update_all( + from(s in Loopctl.Intake.Source, where: s.id == ^ctx.source.id), + set: [mode: :pr, revoked_at: nil] + ) + + assert {:error, :chg_root_cannot_verify} = + Progress.verify_story(ctx.tenant_id, story.id, %{}, chg_opts(lineage)) + end + + test "tenant isolation: another tenant's chg root marks nobody here", ctx do + %{source: other_source, root: other_root} = fixture(:chg_source, %{}) + refute other_source.tenant_id == ctx.tenant_id + + foreign_lineage = + fixture(:chg_root_dispatch, %{tenant_id: other_source.tenant_id, parent: other_root}).lineage_path + + assert ReviewCustody.chg_rooted?(other_source.tenant_id, foreign_lineage) + refute ReviewCustody.chg_rooted?(ctx.tenant_id, foreign_lineage) + assert :ok = ReviewCustody.refuse_chg_root(ctx.tenant_id, foreign_lineage) + refute ReviewCustody.chg_rooted?(ctx.tenant_id, []) + end + end + + # ── AC-47.5.5: a landing with no review for its revision ──────────────────── + + describe "AC-47.5.5: a story that reaches merged with no chg review escalates :review_missing" do + defp at_ci(ctx, story) do + fixture(:story_stage, %{tenant_id: ctx.tenant_id, story_id: story.id, stage: :implementing}) + :ok = submit!(ctx, story, "C1", 1, 1, sha("a")) + assert {:ok, _} = push(ctx, event("C1", 2, "review_approved", %{data: %{"by" => "policy"}})) + assert {:ok, _} = push(ctx, event("C1", 3, "check_passed", %{})) + assert Stages.get(ctx.tenant_id, story.id).stage == :ci + story + end + + defp land(ctx, seq, revision, revision_sha, opts \\ []) do + push( + ctx, + event("C1", seq, "landed", %{ + revision: revision, + sha: revision_sha, + data: %{"merge_sha" => sha("c")} + }), + opts + ) + end + + defp transitions(ctx, story) do + for %{from: f, to: t, edge: e} <- Stages.list_transitions(ctx.tenant_id, story.id), + do: {f, t, e} + end + + defp landed_effect(ctx, seq) do + ctx.tenant_id + |> Events.list_events(@repo, "C1") + |> Enum.find(&(&1.event_seq == seq)) + |> Map.fetch!(:effect) + |> Map.fetch!("stage") + end + + test "TC-47.5.5: an auto-approved landing with no review is merged, then escalated", ctx do + story = at_ci(ctx, ctx.story) + + assert {:ok, %{outcome: :applied}} = land(ctx, 4, 1, sha("a")) + + row = Stages.get(ctx.tenant_id, story.id) + assert row.stage == :escalated + # The merge HELD — trunk has it — so nothing merge-keyed is cleared. + assert row.merge_sha == sha("c") + assert row.escalation_reason =~ "review_missing" + assert row.escalation_reason =~ "revision 1" + + assert Enum.take(transitions(ctx, story), -2) == [ + {"ci", "merged", "forward"}, + {"merged", "escalated", "review_missing"} + ] + + assert AdminRepo.all( + from e in Entry, + where: e.tenant_id == ^ctx.tenant_id and like(e.action, "story_stage_%"), + order_by: [asc: e.chain_position], + select: e.action + ) == ["story_stage_merged", "story_stage_escalated"] + + assert %{ + "outcome" => "moved", + "to" => "merged", + "review" => "missing", + "review_missing" => %{"outcome" => "escalated", "edge" => "review_missing"} + } = landed_effect(ctx, 4) + + # Never verified: the escalation leaves only over a human's resolution, and a later + # event moves nothing. + assert {:ok, _} = push(ctx, event("C1", 5, "deployed", %{data: %{"release" => "v1"}})) + assert Stages.get(ctx.tenant_id, story.id).stage == :escalated + end + + test "a chg review recorded for the landed revision lets the merge stand", ctx do + story = at_ci(ctx, ctx.story) + + assert {:ok, _} = review(ctx, story, child(ctx).lineage_path, %{"commit_sha" => sha("a")}) + assert {:ok, %{outcome: :applied}} = land(ctx, 4, 1, sha("a")) + + assert Stages.get(ctx.tenant_id, story.id).stage == :merged + assert landed_effect(ctx, 4)["review"] == "recorded" + refute {"merged", "escalated", "review_missing"} in transitions(ctx, story) + end + + test "a review of an EARLIER revision does not cover the landed one", ctx do + story = at_ci(ctx, ctx.story) + assert {:ok, _} = review(ctx, story, child(ctx).lineage_path, %{"commit_sha" => sha("a")}) + + # Revision 2 was pushed and landed; the only review names revision 1. + assert {:ok, _} = + push( + ctx, + event("C1", 4, "submitted", %{ + revision: 2, + sha: sha("b"), + data: %{"story_id" => story.id} + }) + ) + + assert {:ok, _} = + push( + ctx, + event("C1", 5, "review_approved", %{revision: 2, data: %{"by" => "policy"}}) + ) + + assert {:ok, _} = push(ctx, event("C1", 6, "check_passed", %{revision: 2})) + assert {:ok, _} = land(ctx, 7, 2, sha("b")) + + assert Stages.get(ctx.tenant_id, story.id).stage == :escalated + end + + test "a review with no commit_sha (recorded off the chg path) never counts", ctx do + story = at_ci(ctx, ctx.story) + fixture(:review_record, %{tenant_id: ctx.tenant_id, story_id: story.id}) + + assert {:ok, _} = land(ctx, 4, 1, sha("a")) + assert Stages.get(ctx.tenant_id, story.id).stage == :escalated + end + + # US-47.5 review round, finding 3: a refused hold must never leave a merge standing alone. + test "a REFUSED hold rolls the whole landing back, and chg's redelivery applies it", ctx do + story = at_ci(ctx, ctx.story) + refuse = [escalate: fn _tenant_id, _story_id, _opts -> {:error, :injected_refusal} end] + + assert {:error, :apply_failed} = land(ctx, 4, 1, sha("a"), refuse) + + # Nothing committed: not merged, not escalated, the event not applied, the cursor unmoved. + assert Stages.get(ctx.tenant_id, story.id).stage == :ci + refute {"ci", "merged", "forward"} in transitions(ctx, story) + assert Events.get_cursor(ctx.tenant_id, @repo, "C1").applied_seq == 3 + + # The redelivery, with nothing refusing, lands AND holds. + assert {:ok, %{outcome: :applied}} = land(ctx, 4, 1, sha("a")) + assert Stages.get(ctx.tenant_id, story.id).stage == :escalated + assert {"merged", "escalated", "review_missing"} in transitions(ctx, story) + end + + test "a hold refused for contention answers the retryable busy", ctx do + story = at_ci(ctx, ctx.story) + busy = [escalate: fn _tenant_id, _story_id, _opts -> {:error, :busy} end] + + assert {:error, :busy} = land(ctx, 4, 1, sha("a"), busy) + assert Stages.get(ctx.tenant_id, story.id).stage == :ci + end + + test "the seam is unused when the landed revision WAS reviewed: nothing to hold", ctx do + story = at_ci(ctx, ctx.story) + assert {:ok, _} = review(ctx, story, child(ctx).lineage_path, %{"commit_sha" => sha("a")}) + refuse = [escalate: fn _tenant_id, _story_id, _opts -> {:error, :injected_refusal} end] + + assert {:ok, %{outcome: :applied}} = land(ctx, 4, 1, sha("a"), refuse) + assert Stages.get(ctx.tenant_id, story.id).stage == :merged + end + + test "tenant isolation: another tenant's review of the same sha covers nothing here", ctx do + story = at_ci(ctx, ctx.story) + other = fixture(:tenant) + other_story = fixture(:story, %{tenant_id: other.id}) + + fixture(:review_record, %{ + tenant_id: other.id, + story_id: other_story.id, + commit_sha: sha("a"), + reviewer_dispatch_id: fixture(:chg_root_dispatch, %{tenant_id: other.id}).id + }) + + assert {:ok, _} = land(ctx, 4, 1, sha("a")) + assert Stages.get(ctx.tenant_id, story.id).stage == :escalated + refute ReviewCustody.reviewed_revision?(AdminRepo, ctx.tenant_id, other_story.id, sha("a")) + end + end + + # ── AC-47.5.5, the verify half: never verified without a review of the landed revision ── + + describe "AC-47.5.5: every verify path refuses a chg story held or unreviewed at its landing" do + # A chg-mode orchestrator OUTSIDE chg: the custody gates admit it, so the landed-review + # gate is the only thing in its way. + defp verifier_opts(ctx) do + other = outsider(ctx) + [orchestrator_agent_id: other.agent_id, verifier_lineage: other.lineage_path] + end + + defp verify(ctx, story), + do: Progress.verify_story(ctx.tenant_id, story.id, %{}, verifier_opts(ctx)) + + defp verify_all(ctx) do + {:ok, result} = + Progress.verify_all_in_epic( + ctx.tenant_id, + ctx.epic.id, + %{"summary" => "all"}, + verifier_opts(ctx) + ) + + result + end + + defp bulk_verify(ctx, story) do + opts = verifier_opts(ctx) + + {:ok, [result]} = + BulkOperations.bulk_verify( + ctx.tenant_id, + [%{"story_id" => story.id}], + opts[:orchestrator_agent_id], + opts + ) + + result + end + + # Bound to C1 with NO stage row, so a landing moves nothing and escalates nothing: the + # landed-revision check alone stands between the story and `verified`. + defp landed_unstaged(ctx, story, landed_sha) do + :ok = submit!(ctx, story, "C1", 1, 1, sha("a")) + + assert {:ok, _} = + push( + ctx, + event("C1", 2, "submitted", %{ + revision: 2, + sha: sha("b"), + data: %{"story_id" => story.id} + }) + ) + + assert {:ok, %{outcome: :applied}} = land(ctx, 3, 2, landed_sha) + story + end + + defp unverified!(story), + do: assert(AdminRepo.get!(Story, story.id).verified_status == :unverified) + + test "TC-47.5.6: escalated over review_missing — refused on every path, even once a chg " <> + "review of the landed revision exists; a human's resolution lifts it", + ctx do + story = at_ci(ctx, ctx.story) + assert {:ok, _} = land(ctx, 4, 1, sha("a")) + assert Stages.get(ctx.tenant_id, story.id).stage == :escalated + + # Recorded AFTER the landing, naming the landed revision: the escalation still holds. + assert {:ok, _} = review(ctx, story, child(ctx).lineage_path, %{"commit_sha" => sha("a")}) + + assert {:error, :review_missing_escalated} = verify(ctx, story) + + assert %{verified_count: 0, errors: [%{story_id: id, reason: ":review_missing_escalated"}]} = + verify_all(ctx) + + assert id == story.id + + assert %{status: "error", reason: "review_missing_escalated: " <> _} = + bulk_verify(ctx, story) + + unverified!(story) + + assert {:ok, _} = + Escalations.resolve(ctx.tenant_id, story.id, + to: :done, + actor_label: "test:operator", + actor_role: :user, + actor_lineage: [] + ) + + assert {:ok, %Story{verified_status: :verified}} = verify(ctx, story) + end + + test "a review of an EARLIER revision is chg_review_not_for_landed_revision; one of the " <> + "landed revision verifies", + ctx do + story = landed_unstaged(ctx, ctx.story, sha("b")) + assert {:ok, _} = review(ctx, story, child(ctx).lineage_path, %{"commit_sha" => sha("a")}) + + assert {:error, :chg_review_not_for_landed_revision} = verify(ctx, story) + + assert %{errors: [%{reason: ":chg_review_not_for_landed_revision"}]} = verify_all(ctx) + + assert %{status: "error", reason: "chg_review_not_for_landed_revision: " <> _} = + bulk_verify(ctx, story) + + unverified!(story) + + assert {:ok, _} = review(ctx, story, child(ctx).lineage_path, %{"commit_sha" => sha("b")}) + assert {:ok, %Story{verified_status: :verified}} = verify(ctx, story) + end + + test "the landed revision is the NEWEST landing's: a review of an earlier landing's " <> + "revision does not cover a re-landing", + ctx do + :ok = submit!(ctx, ctx.story, "C1", 1, 1, sha("a")) + assert {:ok, %{outcome: :applied}} = land(ctx, 2, 1, sha("a")) + + assert {:ok, _} = + review(ctx, ctx.story, child(ctx).lineage_path, %{"commit_sha" => sha("a")}) + + # Revision 2 was pushed and landed after it (the first landing reverted, say). + assert {:ok, _} = + push( + ctx, + event("C1", 3, "submitted", %{ + revision: 2, + sha: sha("b"), + data: %{"story_id" => ctx.story.id} + }) + ) + + assert {:ok, %{outcome: :applied}} = land(ctx, 4, 2, sha("b")) + + assert {:error, :chg_review_not_for_landed_revision} = verify(ctx, ctx.story) + unverified!(ctx.story) + end + + test "a review with no commit_sha does not count, and neither does a change not landed", + ctx do + # Bound, never landed: there is no landed revision for any review to name. + :ok = submit!(ctx, ctx.story, "C1", 1, 1, sha("a")) + + assert {:ok, _} = + review(ctx, ctx.story, child(ctx).lineage_path, %{"commit_sha" => sha("a")}) + + assert {:error, :chg_review_not_for_landed_revision} = verify(ctx, ctx.story) + + # A review recorded off the chg path (no commit_sha, no reviewer dispatch). + assert {:ok, %{outcome: :applied}} = + push( + ctx, + event("C1", 2, "landed", %{sha: sha("a"), data: %{"merge_sha" => sha("c")}}) + ) + + AdminRepo.delete_all(from r in ReviewRecord, where: r.story_id == ^ctx.story.id) + fixture(:review_record, %{tenant_id: ctx.tenant_id, story_id: ctx.story.id}) + + assert {:error, :chg_review_not_for_landed_revision} = verify(ctx, ctx.story) + unverified!(ctx.story) + end + + test "backfill and bulk mark-complete take the same gate", ctx do + pending = fixture(:story, %{tenant_id: ctx.tenant_id, epic_id: ctx.epic.id}) + landed_unstaged(ctx, pending, sha("b")) + + assert {:error, :chg_review_not_for_landed_revision} = + Progress.backfill_story(ctx.tenant_id, pending.id, %{"reason" => "pre"}, []) + + assert {:ok, [%{status: "error", reason: "chg_review_not_for_landed_revision: " <> _}]} = + BulkOperations.bulk_mark_complete( + ctx.tenant_id, + [%{"story_id" => pending.id, "summary" => "pre", "review_type" => "pre"}], + Ecto.UUID.generate(), + [] + ) + + unverified!(pending) + end + + test "a story with NO change binding is answered exactly as before on every path", ctx do + # Reviewed by a separated orchestrator, never bound: the gate is a no-op. + assert :ok = ReviewCustody.ensure_landed_review(ctx.tenant_id, ctx.story.id) + assert {:ok, _} = review(ctx, ctx.story, outsider(ctx).lineage_path, %{}) + assert %{status: "success"} = bulk_verify(ctx, ctx.story) + + other = fixture(:story, %{tenant_id: ctx.tenant_id, epic_id: ctx.epic.id}) + + assert {:ok, %Story{verified_status: :verified}} = + Progress.backfill_story(ctx.tenant_id, other.id, %{"reason" => "pre"}, []) + end + + test "tenant isolation: another tenant's landing and review decide nothing here", ctx do + story = landed_unstaged(ctx, ctx.story, sha("b")) + %{source: other_source} = fixture(:chg_source, %{}) + + # Read as the OTHER tenant, this story id has no binding: nothing of ours leaks there. + assert :ok = ReviewCustody.ensure_landed_review(other_source.tenant_id, story.id) + + fixture(:review_record, %{ + tenant_id: other_source.tenant_id, + story_id: fixture(:story, %{tenant_id: other_source.tenant_id}).id, + commit_sha: sha("b"), + reviewer_dispatch_id: fixture(:chg_root_dispatch, %{tenant_id: other_source.tenant_id}).id + }) + + assert {:error, :chg_review_not_for_landed_revision} = + ReviewCustody.ensure_landed_review(ctx.tenant_id, story.id) + end + + # Finding 4: verify-all judges the chg root ONCE, not once per story. + test "verify_all_in_epic asks whether the caller is chg-rooted once, whatever the epic size", + ctx do + for _ <- 1..2 do + story = fixture(:story, %{tenant_id: ctx.tenant_id, epic_id: ctx.epic.id}) + + story + |> Ecto.Changeset.change(%{ + agent_status: :reported_done, + reported_done_at: DateTime.add(DateTime.utc_now(), -60, :second), + assigned_agent_id: ctx.impl_agent.id, + implementer_dispatch_id: ctx.impl.id + }) + |> AdminRepo.update!() + |> then(&review(ctx, &1, outsider(ctx).lineage_path, %{})) + end + + queries = + PlanAssertions.capture_repo_queries(fn -> + assert %{verified_count: 2} = verify_all(ctx) + end) + + # One read of `intake_sources` — the top-level `chg_rooted?/2` — and none per story. + assert Enum.count(queries, fn {sql, _} -> sql =~ ~s(FROM "intake_sources") end) == 1 + end + end + + # ── finding 5: one principal, one judgement, on both halves of AC-47.5.4 ─────── + + describe "the bulk and single-story chg-root refusals judge the caller identically" do + test "an empty minting lineage with a chg-rooted LIVE lineage is refused on both", ctx do + story = ctx.story + {:ok, _} = review(ctx, story, outsider(ctx).lineage_path, %{}) + chg = child(ctx).lineage_path + + opts = [ + orchestrator_agent_id: reviewer_agent(ctx).id, + caller_minting_lineage: [], + verifier_lineage: chg + ] + + assert {:error, :chg_root_cannot_verify} = + Progress.verify_story(ctx.tenant_id, story.id, %{}, opts) + + assert {:error, :chg_root_cannot_verify} = + BulkOperations.bulk_verify( + ctx.tenant_id, + [%{"story_id" => story.id}], + opts[:orchestrator_agent_id], + opts + ) + + contracted = + fixture(:story, %{tenant_id: ctx.tenant_id, epic_id: ctx.epic.id}) + |> Ecto.Changeset.change(agent_status: :contracted) + |> AdminRepo.update!() + + agent = reviewer_agent(ctx) + claim_opts = [agent_id: agent.id, caller_minting_lineage: [], lineage: chg] + + assert {:error, :chg_root_review_only} = + Progress.claim_story(ctx.tenant_id, contracted.id, claim_opts) + + assert {:error, :chg_root_review_only} = + BulkOperations.bulk_claim(ctx.tenant_id, [contracted.id], agent.id, claim_opts) + + assert AdminRepo.get!(Story, story.id).verified_status == :unverified + end + end +end diff --git a/test/loopctl/changes/stage_mapping_test.exs b/test/loopctl/changes/stage_mapping_test.exs index 4dfed4dcb..1d4e9007a 100644 --- a/test/loopctl/changes/stage_mapping_test.exs +++ b/test/loopctl/changes/stage_mapping_test.exs @@ -29,8 +29,8 @@ defmodule Loopctl.Changes.StageMappingTest do @repo "mkreyman/infra" setup do - %{secret: secret, source: source} = fixture(:chg_source, %{}) - %{secret: secret, source: source, tenant_id: source.tenant_id} + %{secret: secret, source: source, root: root} = fixture(:chg_source, %{}) + %{secret: secret, source: source, root: root, tenant_id: source.tenant_id} end defp push(ctx, event) do @@ -46,6 +46,20 @@ defmodule Loopctl.Changes.StageMappingTest do defp sha(char), do: String.duplicate(char, 40) + # US-47.5: a chg review recorded for `story` at revision commit `revision_sha`, by a child of + # the source's chg root — what a landing reads before it lets the merge stand + # (AC-47.5.5; `Loopctl.Changes.ReviewCustodyTest` drives the gate itself). + defp reviewed!(ctx, story, revision_sha) do + reviewer = fixture(:chg_root_dispatch, %{tenant_id: ctx.tenant_id, parent: ctx.root}) + + fixture(:review_record, %{ + tenant_id: ctx.tenant_id, + story_id: story.id, + commit_sha: revision_sha, + reviewer_dispatch_id: reviewer.id + }) + end + # A story with an assigned agent and its stage row at `stage`. defp story_at(ctx, stage, row_attrs \\ %{}) do agent = fixture(:agent, %{tenant_id: ctx.tenant_id}) @@ -117,6 +131,7 @@ defmodule Loopctl.Changes.StageMappingTest do test "TC-47.4.1: two review rounds, landed, deployed — and pr_open never written", ctx do story = story_at(ctx, :implementing) merge_sha = sha("c") + reviewed!(ctx, story, sha("a")) events = [ submitted("C1", 1, story), @@ -192,7 +207,13 @@ defmodule Loopctl.Changes.StageMappingTest do assert {:ok, _} = push(ctx, submitted("C1", 1, story)) assert {:ok, _} = push(ctx, event("C1", 2, "review_approved", %{data: %{"by" => "model"}})) assert {:ok, _} = push(ctx, event("C1", 3, "check_passed")) - assert {:ok, _} = push(ctx, event("C1", 4, "landed", %{data: %{"merge_sha" => sha("d")}})) + reviewed!(ctx, story, sha("f")) + + assert {:ok, _} = + push( + ctx, + event("C1", 4, "landed", %{sha: sha("f"), data: %{"merge_sha" => sha("d")}}) + ) after_events = AdminRepo.get!(Story, story.id) assert row(ctx, story).stage == :merged @@ -210,8 +231,12 @@ defmodule Loopctl.Changes.StageMappingTest do assert {:ok, _} = push(ctx, event("C1", 2, "check_passed")) assert row(ctx, story).stage == :ci + # It lands like any other change: the edge into `merged` is taken and chained. With no + # chg review for the landed revision it is then held over `:review_missing` (US-47.5, + # TC-47.5.5 in `Loopctl.Changes.ReviewCustodyTest`). assert {:ok, _} = push(ctx, event("C1", 3, "landed", %{data: %{"merge_sha" => sha("e")}})) - assert row(ctx, story).stage == :merged + assert {"ci", "merged", "forward"} in transitions(ctx, story) + assert row(ctx, story).merge_sha == sha("e") end test "the pair is the pair whichever order it arrives in", ctx do diff --git a/test/loopctl/delivery/stage_machine_test.exs b/test/loopctl/delivery/stage_machine_test.exs index 9c459f21f..b04241ce9 100644 --- a/test/loopctl/delivery/stage_machine_test.exs +++ b/test/loopctl/delivery/stage_machine_test.exs @@ -384,7 +384,9 @@ defmodule Loopctl.Delivery.StageMachineTest do :change_review_findings, :reverted, :reverted_after_verify, - :change_outside_lifecycle + :change_outside_lifecycle, + # US-47.5 + :review_missing ] assert Enum.sort(StageMachine.change_protocol_edges()) == Enum.sort(new_edges) @@ -471,9 +473,9 @@ defmodule Loopctl.Delivery.StageMachineTest do assert StageMachine.chained?(:ci, :escalated, :change_outside_lifecycle) end - test "the control escalations are exactly the two a change event decides" do + test "the control escalations are exactly the three a change event decides" do assert Enum.sort(StageMachine.control_escalation_edges()) == - [:change_outside_lifecycle, :reverted_after_verify] + [:change_outside_lifecycle, :reverted_after_verify, :review_missing] for edge <- StageMachine.control_escalation_edges() do assert Enum.all?(StageMachine.transitions(), fn {_f, t, e} -> @@ -485,6 +487,27 @@ defmodule Loopctl.Delivery.StageMachineTest do end end + describe "the :review_missing edge (US-47.5, AC-47.5.5)" do + test "it is merged -> escalated and nothing else, chained, and clears nothing" do + assert [{:merged, :escalated, :review_missing}] == + for({_f, _t, :review_missing} = t <- StageMachine.transitions(), do: t) + + # Entering `escalated` is chained and needs a reason; the merge HELD, so nothing + # merge-keyed is cleared — trunk still has it. + assert StageMachine.chained?(:merged, :escalated, :review_missing) + assert StageMachine.reason_required?(:escalated, :review_missing) + assert StageMachine.clears(:merged, :escalated, :review_missing) == [] + refute StageMachine.human_only?(:review_missing) + end + + test "no runner can report it, and it is a control escalation" do + refute Enum.any?(StageMachine.runner_transitions(), &match?({_, _, :review_missing}, &1)) + refute :review_missing in StageMachine.runner_reportable_edges() + assert :review_missing in StageMachine.control_escalation_edges() + assert :review_missing in StageMachine.change_protocol_edges() + end + end + test "the migration's stage CHECK allows exactly StageMachine.stages/0" do [[definition]] = Repo.query!( diff --git a/test/loopctl/test_suite_hygiene_test.exs b/test/loopctl/test_suite_hygiene_test.exs index 3113c5959..5b09c64de 100644 --- a/test/loopctl/test_suite_hygiene_test.exs +++ b/test/loopctl/test_suite_hygiene_test.exs @@ -39,6 +39,7 @@ defmodule Loopctl.TestSuiteHygieneTest do "two connections over committed rows", "test/loopctl/changes_lock_test.exs" => @race, "test/loopctl/changes/events_lock_test.exs" => @race, + "test/loopctl/changes/review_custody_lock_test.exs" => @race, "test/loopctl/config_test_exs_partition_wiring_test.exs" => "the subject is config/test.exs's own read of the OS environment at load time", "test/loopctl/context_retriever/entity_definitions_rollback_test.exs" => @migration, diff --git a/test/loopctl_web/controllers/chg_review_custody_controller_test.exs b/test/loopctl_web/controllers/chg_review_custody_controller_test.exs new file mode 100644 index 000000000..34c7e117f --- /dev/null +++ b/test/loopctl_web/controllers/chg_review_custody_controller_test.exs @@ -0,0 +1,511 @@ +defmodule LoopctlWeb.ChgReviewCustodyControllerTest do + @moduledoc """ + US-47.5 over HTTP: real dispatch-minted keys, so the caller's lineage is the one the + controllers resolve SERVER-SIDE from the authenticating key (`Dispatches.lineage_for_api_key/2` + and `minting_lineage_for_api_key/2`), never a value a request could carry. The decisions + themselves are `Loopctl.Changes.ReviewCustodyTest`'s; this module holds the routes, the + response bodies, and the OpenAPI declarations each refusal now needs. + """ + + use LoopctlWeb.ConnCase, async: true + + alias Loopctl.AdminRepo + alias Loopctl.ApiSpec.Messages + alias Loopctl.Changes.Events + alias Loopctl.Changes.Signature + alias Loopctl.Dispatches + alias Loopctl.Progress + alias Loopctl.WorkBreakdown.Story + + setup :verify_on_exit! + + @repo "mkreyman/infra" + + setup do + tenant = fixture(:tenant) + agent = fn -> fixture(:agent, %{tenant_id: tenant.id, agent_type: :orchestrator}).id end + + # chg-server's root: a parentless orchestrator dispatch WITH a key, registered on a chg + # source as `chg_root_dispatch_id`. + {:ok, %{dispatch: root, raw_key: root_key}} = + Dispatches.create_dispatch(tenant.id, %{role: :orchestrator, agent_id: agent.()}) + + secret = "chg-secret-" <> Base.encode16(:crypto.strong_rand_bytes(16)) + + {_webhook_secret, source} = + fixture(:intake_source, %{ + tenant_id: tenant.id, + repo_full_name: @repo, + mode: "chg", + change_events_secret: secret, + chg_root_dispatch_id: root.id + }) + + {:ok, %{dispatch: chg_child, raw_key: child_key}} = + Dispatches.create_dispatch(tenant.id, %{ + role: :orchestrator, + agent_id: agent.(), + parent_dispatch_id: root.id + }) + + # The delivery loop's own tree: the implementer, and an orchestrator outside chg. + {:ok, %{dispatch: loop_root}} = + Dispatches.create_dispatch(tenant.id, %{role: :orchestrator, agent_id: agent.()}) + + impl_agent = fixture(:agent, %{tenant_id: tenant.id, agent_type: :implementer}) + + {:ok, %{dispatch: impl}} = + Dispatches.create_dispatch(tenant.id, %{ + role: :agent, + agent_id: impl_agent.id, + parent_dispatch_id: loop_root.id + }) + + {:ok, %{dispatch: orch, raw_key: orch_key}} = + Dispatches.create_dispatch(tenant.id, %{ + role: :orchestrator, + agent_id: agent.(), + parent_dispatch_id: loop_root.id + }) + + epic = fixture(:epic, %{tenant_id: tenant.id}) + + story = + fixture(:story, %{tenant_id: tenant.id, epic_id: epic.id}) + |> Ecto.Changeset.change(%{ + agent_status: :reported_done, + reported_done_at: DateTime.add(DateTime.utc_now(), -60, :second), + assigned_agent_id: impl_agent.id, + implementer_dispatch_id: impl.id + }) + |> AdminRepo.update!() + + %{ + tenant: tenant, + source: source, + secret: secret, + root: root, + root_key: root_key, + chg_child: chg_child, + child_key: child_key, + orch: orch, + orch_key: orch_key, + epic: epic, + story: story, + new_agent: agent + } + end + + defp auth(conn, key), do: put_req_header(conn, "authorization", "Bearer #{key}") + + defp sha(char), do: String.duplicate(char, 40) + + defp submit!(ctx, story, revision_sha), + do: push!(ctx, 1, "submitted", %{sha: revision_sha, data: %{"story_id" => story.id}}) + + defp push!(ctx, seq, kind, attrs) do + raw = + Jason.encode!( + build(:chg_event, Map.merge(%{change_id: "C1", event_seq: seq, kind: kind}, attrs)) + ) + + ts = Integer.to_string(System.os_time(:second)) + + assert {:ok, %{outcome: :applied}} = + Events.receive_push(ctx.source.id, %{ + raw_body: raw, + timestamp: ts, + signature: Signature.header(ctx.secret, Signature.delivery_string(ts, raw)) + }) + + :ok + end + + defp review_complete(conn, key, story, body) do + conn + |> auth(key) + |> post( + ~p"/api/v1/stories/#{story.id}/review-complete", + Map.merge(%{"review_type" => "chg", "summary" => "chg review"}, body) + ) + end + + defp error_of(conn, status) do + %{"error" => error} = json_response(conn, status) + error + end + + defp child_key(ctx, role) do + {:ok, %{raw_key: key}} = + Dispatches.create_dispatch(ctx.tenant.id, %{ + role: role, + agent_id: ctx.new_agent.(), + parent_dispatch_id: ctx.root.id + }) + + key + end + + # A review an orchestrator OUTSIDE chg recorded, so the story is verifiable by anyone the + # custody gates otherwise admit — the chg refusal is then the only thing in the way. + defp reviewed(ctx) do + {:ok, _} = + Progress.record_review(ctx.tenant.id, ctx.story.id, %{"review_type" => "enhanced"}, + reviewer_agent_id: ctx.orch.agent_id, + reviewer_lineage: ctx.orch.lineage_path + ) + + ctx.story + end + + describe "POST /stories/:id/review-complete on a chg story" do + test "TC-47.5.1: 201 from a chg child naming the revision", %{conn: conn} = ctx do + :ok = submit!(ctx, ctx.story, sha("a")) + + body = + conn + |> review_complete(ctx.child_key, ctx.story, %{"commit_sha" => sha("a")}) + |> json_response(201) + + assert body["review_record"]["commit_sha"] == sha("a") + assert body["review_record"]["reviewer_dispatch_id"] == ctx.chg_child.id + end + + test "TC-47.5.2: 409 chg_reviewer_required for an orchestrator outside chg, and for the root", + %{conn: conn} = ctx do + :ok = submit!(ctx, ctx.story, sha("a")) + + for key <- [ctx.orch_key, ctx.root_key] do + error = + conn + |> review_complete(key, ctx.story, %{"commit_sha" => sha("a")}) + |> error_of(409) + + assert error["code"] == "chg_reviewer_required" + assert error["message"] == Messages.chg_reviewer_required() + end + end + + test "TC-47.5.3: 422 commit_sha_not_bound for a sha the binding does not have", + %{conn: conn} = ctx do + :ok = submit!(ctx, ctx.story, sha("a")) + + error = + conn + |> review_complete(ctx.child_key, ctx.story, %{"commit_sha" => sha("b")}) + |> error_of(422) + + assert error["code"] == "commit_sha_not_bound" + assert error["message"] == Messages.commit_sha_not_bound() + end + + test "409 chg_review_already_recorded for the same reviewer dispatch's second verdict", + %{conn: conn} = ctx do + :ok = submit!(ctx, ctx.story, sha("a")) + + assert conn + |> review_complete(ctx.child_key, ctx.story, %{"commit_sha" => sha("a")}) + |> json_response(201) + + error = + build_conn() + |> review_complete(ctx.child_key, ctx.story, %{"commit_sha" => sha("a")}) + |> error_of(409) + + assert error["code"] == "chg_review_already_recorded" + end + + test "a non-chg story ignores commit_sha", %{conn: conn} = ctx do + body = + conn + |> review_complete(ctx.orch_key, ctx.story, %{"commit_sha" => sha("a")}) + |> json_response(201) + + assert body["review_record"]["commit_sha"] == nil + assert body["review_record"]["reviewer_dispatch_id"] == nil + end + + test "caller_lineage_required is a 409, not an unhandled 500", %{conn: conn} = ctx do + # A plain (legacy) orchestrator key on dispatch-minted work. Its 409 was documented and + # the controller's enumerated case had no clause for it. + {legacy_key, _} = + fixture(:api_key, %{ + tenant_id: ctx.tenant.id, + role: :orchestrator, + agent_id: ctx.new_agent.() + }) + + assert conn + |> review_complete(legacy_key, ctx.story, %{}) + |> error_of(409) + |> Map.fetch!("code") == "caller_lineage_required" + end + end + + describe "chg_root_cannot_verify on every terminal custody route" do + test "TC-47.5.4: verify and bulk verify both answer 409", %{conn: conn} = ctx do + story = reviewed(ctx) + + for {method_path, body} <- [ + {~p"/api/v1/stories/#{story.id}/verify", %{"summary" => "ok"}}, + {~p"/api/v1/stories/bulk/verify", %{"stories" => [%{"story_id" => story.id}]}} + ] do + error = conn |> auth(ctx.child_key) |> post(method_path, body) |> error_of(409) + assert error["code"] == "chg_root_cannot_verify", method_path + assert error["message"] == Messages.chg_root_cannot_verify() + end + + assert AdminRepo.get!(Story, story.id).verified_status == :unverified + end + + test "reject, bulk reject, verify-all, backfill and bulk mark-complete answer 409", + %{conn: conn} = ctx do + story = reviewed(ctx) + pending = fixture(:story, %{tenant_id: ctx.tenant.id, epic_id: ctx.epic.id}) + + for {path, body} <- [ + {~p"/api/v1/stories/#{story.id}/reject", %{"reason" => "no"}}, + {~p"/api/v1/stories/bulk/reject", + %{"stories" => [%{"story_id" => story.id, "reason" => "no"}]}}, + {~p"/api/v1/epics/#{ctx.epic.id}/verify-all", + %{"review_type" => "chg", "summary" => "all"}}, + {~p"/api/v1/stories/#{pending.id}/backfill", %{"reason" => "pre-existing"}}, + {~p"/api/v1/stories/bulk/mark-complete", + %{ + "stories" => [ + %{"story_id" => pending.id, "summary" => "pre", "review_type" => "pre"} + ] + }} + ] do + error = conn |> auth(ctx.child_key) |> post(path, body) |> error_of(409) + assert error["code"] == "chg_root_cannot_verify", path + end + + assert AdminRepo.get!(Story, story.id).verified_status == :unverified + assert AdminRepo.get!(Story, pending.id).verified_status == :unverified + end + + test "an EXPIRED chg key is refused too: the controller resolves the MINTING lineage", + %{conn: conn} = ctx do + story = reviewed(ctx) + + # The expired-dispatch sweep revokes the dispatch ROW and leaves its key usable + # (`Dispatches.revoke_dispatch_rows/3`), so the key's LIVE lineage is `[]` — which reject + # accepts as an unlineaged caller. Only the minting lineage still names chg's root. + 1 = Dispatches.revoke_dispatch_rows(ctx.tenant.id, [ctx.chg_child.id], DateTime.utc_now()) + assert Dispatches.lineage_for_api_key(ctx.tenant.id, ctx.chg_child.api_key_id) == [] + + error = + conn + |> auth(ctx.child_key) + |> post(~p"/api/v1/stories/#{story.id}/reject", %{"reason" => "no"}) + |> error_of(409) + + assert error["code"] == "chg_root_cannot_verify" + assert AdminRepo.get!(Story, story.id).verified_status == :unverified + end + + test "the same routes still serve an orchestrator outside chg", %{conn: conn} = ctx do + story = reviewed(ctx) + + assert conn + |> auth(ctx.orch_key) + |> post(~p"/api/v1/stories/#{story.id}/verify", %{"summary" => "ok"}) + |> json_response(202) + + assert AdminRepo.get!(Story, story.id).verified_status == :verified + end + end + + describe "a chg story's landed-review gate on the verify routes (AC-47.5.5)" do + test "409 chg_review_not_for_landed_revision on verify and backfill; per-story on the " <> + "bulk and epic routes", + %{conn: conn} = ctx do + # Bound and landed with NO stage row, so nothing escalates; the only review on record is + # one recorded off the chg path, which names no revision. + story = reviewed(ctx) + :ok = submit!(ctx, story, sha("a")) + :ok = push!(ctx, 2, "landed", %{sha: sha("a"), data: %{"merge_sha" => sha("c")}}) + + error = + conn + |> auth(ctx.orch_key) + |> post(~p"/api/v1/stories/#{story.id}/verify", %{"summary" => "ok"}) + |> error_of(409) + + assert error["code"] == "chg_review_not_for_landed_revision" + assert error["message"] == Messages.chg_review_not_for_landed_revision() + + assert %{"results" => [%{"status" => "error", "reason" => reason}]} = + build_conn() + |> auth(ctx.orch_key) + |> post(~p"/api/v1/stories/bulk/verify", %{ + "stories" => [%{"story_id" => story.id}] + }) + |> json_response(422) + + assert reason =~ "chg_review_not_for_landed_revision" + + assert %{ + "verified_count" => 0, + "errors" => [%{"reason" => ":chg_review_not_for_landed_revision"}] + } = + build_conn() + |> auth(ctx.orch_key) + |> post(~p"/api/v1/epics/#{ctx.epic.id}/verify-all", %{ + "review_type" => "chg", + "summary" => "all" + }) + |> json_response(200) + + pending = fixture(:story, %{tenant_id: ctx.tenant.id, epic_id: ctx.epic.id}) + c2 = %{change_id: "C2", sha: sha("b")} + :ok = push!(ctx, 1, "submitted", Map.put(c2, :data, %{"story_id" => pending.id})) + :ok = push!(ctx, 2, "landed", Map.put(c2, :data, %{"merge_sha" => sha("d")})) + + assert build_conn() + |> auth(ctx.orch_key) + |> post(~p"/api/v1/stories/#{pending.id}/backfill", %{"reason" => "pre-existing"}) + |> error_of(409) + |> Map.fetch!("code") == "chg_review_not_for_landed_revision" + + assert AdminRepo.get!(Story, story.id).verified_status == :unverified + assert AdminRepo.get!(Story, pending.id).verified_status == :unverified + end + + test "409 review_missing_escalated while the landing's hold stands", %{conn: conn} = ctx do + story = reviewed(ctx) + + fixture(:story_stage, %{ + tenant_id: ctx.tenant.id, + story_id: ctx.story.id, + stage: :implementing + }) + + :ok = submit!(ctx, ctx.story, sha("a")) + :ok = push!(ctx, 2, "review_approved", %{data: %{"by" => "policy"}}) + :ok = push!(ctx, 3, "check_passed", %{}) + :ok = push!(ctx, 4, "landed", %{sha: sha("a"), data: %{"merge_sha" => sha("c")}}) + + error = + conn + |> auth(ctx.orch_key) + |> post(~p"/api/v1/stories/#{story.id}/verify", %{"summary" => "ok"}) + |> error_of(409) + + assert error["code"] == "review_missing_escalated" + assert error["message"] == Messages.review_missing_escalated() + assert AdminRepo.get!(Story, story.id).verified_status == :unverified + end + end + + describe "chg_root_review_only on claim and report" do + test "a chg agent-role child cannot claim, singly or in bulk", %{conn: conn} = ctx do + contracted = + fixture(:story, %{tenant_id: ctx.tenant.id, epic_id: ctx.epic.id}) + |> Ecto.Changeset.change(agent_status: :contracted) + |> AdminRepo.update!() + + agent_key = child_key(ctx, :agent) + + for {path, body} <- [ + {~p"/api/v1/stories/#{contracted.id}/claim", %{}}, + {~p"/api/v1/stories/bulk/claim", %{"story_ids" => [contracted.id]}} + ] do + error = conn |> auth(agent_key) |> post(path, body) |> error_of(409) + assert error["code"] == "chg_root_review_only", path + assert error["message"] == Messages.chg_root_review_only() + end + + assert AdminRepo.get!(Story, contracted.id).agent_status == :contracted + end + + test "a chg orchestrator child cannot report", %{conn: conn} = ctx do + implementing = + ctx.story + |> Ecto.Changeset.change(agent_status: :implementing, reported_done_at: nil) + |> AdminRepo.update!() + + error = + conn + |> auth(ctx.child_key) + |> post(~p"/api/v1/stories/#{implementing.id}/report", %{}) + |> error_of(409) + + assert error["code"] == "chg_root_review_only" + end + end + + describe "one resolver for the chg-root lineage, at every custody route" do + test "chg_root_opts/1 resolves the MINTING lineage, an expired chg key's included", ctx do + 1 = Dispatches.revoke_dispatch_rows(ctx.tenant.id, [ctx.chg_child.id], DateTime.utc_now()) + key = %{tenant_id: ctx.tenant.id, id: ctx.chg_child.api_key_id} + + assert LoopctlWeb.CustodyLineage.chg_root_opts(key) == [ + caller_minting_lineage: ctx.chg_child.lineage_path + ] + end + + test "no custody controller resolves it inline" do + for file <- + ~w(story_verification_controller story_status_controller bulk_operations_controller) do + source = File.read!("lib/loopctl_web/controllers/#{file}.ex") + refute source =~ "minting_lineage_for_api_key", file + assert source =~ "CustodyLineage.chg_root_opts(api_key)", file + end + end + end + + describe "the OpenAPI spec declares each new refusal, with the message the body carries" do + @declared [ + {"/api/v1/stories/{id}/review-complete", 409, &Messages.chg_reviewer_required/0}, + {"/api/v1/stories/{id}/review-complete", 409, &Messages.chg_review_already_recorded/0}, + {"/api/v1/stories/{id}/review-complete", 422, &Messages.commit_sha_not_bound/0}, + {"/api/v1/stories/{id}/verify", 409, &Messages.chg_root_cannot_verify/0}, + {"/api/v1/stories/{id}/verify", 409, &Messages.review_missing_escalated/0}, + {"/api/v1/stories/{id}/verify", 409, &Messages.chg_review_not_for_landed_revision/0}, + {"/api/v1/stories/{id}/backfill", 409, &Messages.review_missing_escalated/0}, + {"/api/v1/stories/{id}/backfill", 409, &Messages.chg_review_not_for_landed_revision/0}, + {"/api/v1/stories/{id}/reject", 409, &Messages.chg_root_cannot_verify/0}, + {"/api/v1/epics/{id}/verify-all", 409, &Messages.chg_root_cannot_verify/0}, + {"/api/v1/stories/{id}/backfill", 409, &Messages.chg_root_cannot_verify/0}, + {"/api/v1/stories/bulk/verify", 409, &Messages.chg_root_cannot_verify/0}, + {"/api/v1/stories/bulk/reject", 409, &Messages.chg_root_cannot_verify/0}, + {"/api/v1/stories/bulk/mark-complete", 409, &Messages.chg_root_cannot_verify/0}, + {"/api/v1/stories/{id}/claim", 409, &Messages.chg_root_review_only/0}, + {"/api/v1/stories/bulk/claim", 409, &Messages.chg_root_review_only/0}, + {"/api/v1/stories/{id}/report", 409, &Messages.chg_root_review_only/0} + ] + + test "every route, status and message" do + paths = Loopctl.ApiSpec.spec().paths + + for {path, status, message} <- @declared do + response = paths[path].post.responses[status] + assert response, "#{path} does not declare #{status}" + assert response.description =~ message.(), "#{path} #{status} does not quote the message" + end + end + + test "verify-all and the bulk verify routes name the per-story landed-review codes" do + paths = Loopctl.ApiSpec.spec().paths + + for path <- [ + "/api/v1/epics/{id}/verify-all", + "/api/v1/stories/bulk/verify", + "/api/v1/stories/bulk/mark-complete" + ] do + description = paths[path].post.description + assert description =~ "review_missing_escalated", path + assert description =~ "chg_review_not_for_landed_revision", path + end + end + + test "review-complete declares commit_sha in its request body" do + body = Loopctl.ApiSpec.spec().paths["/api/v1/stories/{id}/review-complete"].post.requestBody + + assert %{commit_sha: %{type: :string}} = + body.content["application/json"].schema.properties + end + end +end diff --git a/test/support/fixtures.ex b/test/support/fixtures.ex index f7c539ef4..6d534dccc 100644 --- a/test/support/fixtures.ex +++ b/test/support/fixtures.ex @@ -1973,11 +1973,16 @@ defmodule Loopctl.Fixtures do data = build(:review_record, attrs) + # US-47.5: a chg review's revision and reviewer dispatch, set on the struct as + # `Progress.record_review/4` sets them (never cast). Pass both or neither — the + # `review_records_chg_review_shape` CHECK refuses one alone. changeset = %ReviewRecord{ tenant_id: tenant_id, story_id: story_id, - reviewer_agent_id: reviewer_agent_id + reviewer_agent_id: reviewer_agent_id, + commit_sha: Map.get(attrs, :commit_sha), + reviewer_dispatch_id: Map.get(attrs, :reviewer_dispatch_id) } |> ReviewRecord.create_changeset(data) @@ -3946,6 +3951,10 @@ defmodule Loopctl.Fixtures do [raw_ids] ) + # A chg review references its reviewer dispatch (US-47.5); the review rows would go with + # the tenant cascade anyway, so they go first. + AdminRepo.query!("DELETE FROM review_records WHERE tenant_id = ANY($1)", [raw_ids]) + AdminRepo.query!("DELETE FROM dispatches WHERE tenant_id = ANY($1)", [raw_ids]) AdminRepo.delete_all(from(t in Tenant, where: t.id in ^ids)) rescue