From 4fd34bc731626646c65c1931b48b475f7751439e Mon Sep 17 00:00:00 2001 From: Mark Kreyman Date: Mon, 19 Oct 2026 04:38:56 -0600 Subject: [PATCH 1/3] Verify chg stories from verified/master (US-47.6) A story bound to a chg change now reaches verified only when the landed trunk commit (data.merge_sha of the newest applied landed event) is an ancestor of verified/master, read through the forge compare API, and no reverted event follows that landing. A forge timeout or error answers 503 chg_trunk_unevaluated with Retry-After; every other gate is unchanged. The gate covers verify, verify-all, bulk verify, backfill and bulk mark-complete. The forge read runs outside the transaction and is applied under the story lock only if it was made for the same landing. mcp-server 3.4.0 documents the new refusals on the verify tools. --- .claude/skills/chain-of-custody/SKILL.md | 10 + CHANGELOG.md | 20 + CLAUDE.md | 8 +- .../epic_47_change_protocol/us_47.6.json | 8 +- lib/loopctl/api_spec/messages.ex | 36 ++ lib/loopctl/bulk_operations.ex | 27 +- lib/loopctl/changes/trunk_evidence.ex | 334 +++++++++++++ lib/loopctl/progress.ex | 60 ++- .../controllers/bulk_operations_controller.ex | 14 +- .../story_verification_controller.ex | 67 ++- lib/loopctl_web/fallback_controller.ex | 30 +- mcp-server/CHANGELOG.md | 12 + mcp-server/index.js | 22 +- mcp-server/package-lock.json | 4 +- mcp-server/package.json | 2 +- .../test/chg_review_custody_tools.test.js | 16 + test/loopctl/changes/review_custody_test.exs | 10 + test/loopctl/changes/trunk_evidence_test.exs | 452 ++++++++++++++++++ .../github_pull_request_source_test.exs | 25 + ...chg_trunk_verification_controller_test.exs | 259 ++++++++++ 20 files changed, 1373 insertions(+), 43 deletions(-) create mode 100644 lib/loopctl/changes/trunk_evidence.ex create mode 100644 test/loopctl/changes/trunk_evidence_test.exs create mode 100644 test/loopctl_web/controllers/chg_trunk_verification_controller_test.exs diff --git a/.claude/skills/chain-of-custody/SKILL.md b/.claude/skills/chain-of-custody/SKILL.md index 14b8ae40..5c77552e 100644 --- a/.claude/skills/chain-of-custody/SKILL.md +++ b/.claude/skills/chain-of-custody/SKILL.md @@ -145,6 +145,16 @@ unless a chg review names the LANDED revision (the `sha` of the newest applied ` of the current binding). Escalation in general does NOT block verify; only this edge does. Neither refusal records a custody violation. +**A chg story's CI evidence (US-47.6) — `Loopctl.Changes.TrunkEvidence`.** Right after that gate, +on the same five paths, the landing's TRUNK commit (`data.merge_sha`, never `event.sha`) must be in +`verified/master` (`PullRequestSource.contains?/3`; `chg_landing_not_trunk_verified`) and no applied +`reverted` may follow the landing (`chg_landing_reverted`). The forge is read BEFORE the +transaction (`assess/2`, `assess_many/2`) and applied under the lock only to the landing it was +made for (`ensure_trunk_verified/4`); any forge failure, a 404 included, is +`{:chg_trunk_unevaluated, reason, retry_after}` — a 503 with `Retry-After`, never a pass. Do not +move the read into the Multi (a lock held across a network call), and do not short-circuit on the +local `trunk_verified_sha`: the ref is the evidence. + **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 diff --git a/CHANGELOG.md b/CHANGELOG.md index a4d08aa2..d3f816e8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -105,6 +105,26 @@ All notable changes to loopctl are documented here. 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. +- **A chg story's CI evidence is `verified/master` (US-47.6).** On a story bound to a chg + change, every path that reaches `verified` (`POST /stories/:id/verify`, + `/epics/:id/verify-all`, `/stories/bulk/verify`, `/stories/:id/backfill`, + `/stories/bulk/mark-complete`) now also asks GitHub whether the trunk commit of the newest + applied `landed` event (`data.merge_sha`) is an ancestor of, or equal to, the repository's + `refs/heads/verified/master` (`GET /repos/:repo/compare/...verified/master`), + read before the verify transaction with the same bounded timeouts as the other forge reads. + 409 `chg_landing_not_trunk_verified` when it is not (or the landing named no valid trunk + commit); 409 `chg_landing_reverted` when an applied `reverted` event follows the landing; + 503 `chg_trunk_unevaluated` with `Retry-After` and an `error.reason` when nothing could be + decided — a timeout, an unreachable forge, a 5xx or rate limit (the forge's own delay), a + 401/403/422, a missing `verified/master` ref or commit (404, `trunk_ref_not_found`), no + credential for the repository (`credential_unavailable`, Retry-After 3600), or a new landing + in between (`landing_changed`). verify-all and the bulk paths report each per story. Stories + with no change binding are answered exactly as before and never reach the forge. + **Operator action:** the tenant's forge token (or the operator `GITHUB_TOKEN` lent through + `VERIFICATION_OPERATOR_TOKEN_TENANTS`) must be able to read the chg repository's commits — + a classic token's `repo` scope for a private repository, or a fine-grained token's + `Contents: read`; without it every chg verify answers 503 `forge_forbidden`/`trunk_ref_not_found`. + No migration. MCP: loopctl-mcp-server 3.4.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 diff --git a/CLAUDE.md b/CLAUDE.md index 19d05326..94c3992c 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -247,7 +247,7 @@ revoked or not — an expired chg key's live lineage is `[]`, which reject would 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 +`progress.ex:2619`; `BulkOperations.refuse_chg_root/3`, `bulk_operations.ex:394`) 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 @@ -262,9 +262,9 @@ landed event's own transaction over the control-only edge `:review_missing` 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, +verify and verify-all through `chg_landed_review/2` (`progress.ex:2713`), bulk verify +through `process_verify/3` (`bulk_operations.ex:426`), backfill and bulk mark-complete +through `guard_backfill/3` (`progress.ex:2824`) — 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 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 17098616..df1681c9 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,11 +16,11 @@ "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) 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." + "description": "For a :chg source, the CI-evidence input passes only when the current binding's landed sha \u2014 the TRUNK commit of its newest applied `landed` event, `data.merge_sha`, never the envelope `sha` (the revision commit, which is US-47.5's to certify) \u2014 is an ancestor of, or equal to, 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", - "description": "A compare call that times out or errors answers unevaluated with Retry-After; it is never read as a pass." + "description": "A compare call that times out or errors answers unevaluated with Retry-After; it is never read as a pass. A 404 (no verified/master ref yet, or the trunk commit not on the forge yet) is unevaluated too, never a refusal: chg has not verified it yet, which is not a failure of the story." }, { "id": "AC-47.6.3", @@ -81,7 +81,9 @@ } ], "technical_notes": [ - "Every forge call authenticates with the tenant's own GitHub token (#936)." + "Every forge call authenticates with the tenant's own GitHub token (#936).", + "The ancestor check reuses `PullRequestSource.contains?/3` (`GET /repos/:repo/compare/...verified/master`): the status describes the HEAD relative to the BASE, so `ahead` or `identical` means merge_sha is an ancestor of verified/master, and `behind` or `diverged` means it is not.", + "The forge read runs BEFORE the verify transaction; the answer is applied under the story lock only to the landing it was made for (a newer landing in between is unevaluated `landing_changed`). A local `trunk_verified_sha == merge_sha` is not taken as the answer: the ref is." ], "dependencies": [ "US-47.4", diff --git a/lib/loopctl/api_spec/messages.ex b/lib/loopctl/api_spec/messages.ex index 69a34a6a..8cf9280a 100644 --- a/lib/loopctl/api_spec/messages.ex +++ b/lib/loopctl/api_spec/messages.ex @@ -103,4 +103,40 @@ defmodule Loopctl.ApiSpec.Messages do "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 + + @doc """ + The 409 `chg_landing_not_trunk_verified` message (US-47.6; verify, verify-all, bulk verify, + backfill, bulk mark-complete on a chg story). + """ + @spec chg_landing_not_trunk_verified() :: String.t() + def chg_landing_not_trunk_verified do + "This story is bound to a chg change, so its CI evidence is chg's trunk verifier: the " <> + "trunk commit its newest applied `landed` event names (data.merge_sha) must be an " <> + "ancestor of, or equal to, the repository's verified/master. The forge says it is not " <> + "(chg has not verified that commit yet), or the landing named no valid trunk commit. " <> + "Retry once chg's `verified` event for a commit containing the landing has arrived." + end + + @doc """ + The 409 `chg_landing_reverted` message (US-47.6; every verify path, on a chg story). + """ + @spec chg_landing_reverted() :: String.t() + def chg_landing_reverted do + "This story is bound to a chg change whose newest landing was reverted: an applied " <> + "`reverted` event follows it. A reverted landing is never verified; the story is " <> + "verifiable again only after the change lands anew." + end + + @doc """ + The 503 `chg_trunk_unevaluated` message (US-47.6; every verify path, on a chg story). + """ + @spec chg_trunk_unevaluated() :: String.t() + def chg_trunk_unevaluated do + "This story is bound to a chg change, and whether its landing is in verified/master " <> + "could not be established: the forge timed out, failed, was rate-limited, refused the " <> + "credential, has no verified/master ref (or no such commit) yet, no credential reads " <> + "the repository, or a new landing arrived while this call ran. Nothing was decided and " <> + "nothing changed. `reason` names which; retry after `retry_after_seconds` (the " <> + "Retry-After header)." + end end diff --git a/lib/loopctl/bulk_operations.ex b/lib/loopctl/bulk_operations.ex index b196cd67..20e7f0dc 100644 --- a/lib/loopctl/bulk_operations.ex +++ b/lib/loopctl/bulk_operations.ex @@ -23,6 +23,7 @@ defmodule Loopctl.BulkOperations do alias Loopctl.Artifacts.VerificationResult alias Loopctl.Audit alias Loopctl.Changes.ReviewCustody + alias Loopctl.Changes.TrunkEvidence alias Loopctl.Delivery.Stages alias Loopctl.Dispatches alias Loopctl.Progress @@ -129,7 +130,9 @@ defmodule Loopctl.BulkOperations do orch_id: orchestrator_agent_id, actor_id: actor_id, actor_label: actor_label, - caller_lineage: caller_lineage(tenant_id, opts) + caller_lineage: caller_lineage(tenant_id, opts), + # US-47.6: the chg stories' forge reads, made BEFORE the locking transaction. + trunk: TrunkEvidence.assess_many(tenant_id, sorted_ids) } AdminRepo.transaction(fn -> @@ -251,7 +254,9 @@ defmodule Loopctl.BulkOperations do tenant_id: tenant_id, orch_id: orchestrator_agent_id, actor_id: actor_id, - actor_label: actor_label + actor_label: actor_label, + # US-47.6: the chg stories' forge reads, made BEFORE the locking transaction. + trunk: TrunkEvidence.assess_many(tenant_id, sorted_ids) } AdminRepo.transaction(fn -> @@ -291,7 +296,7 @@ defmodule Loopctl.BulkOperations do actor_label: actor_label } = ctx - with :ok <- Progress.ensure_mark_complete_allowed(story, ctx.lifecycle_ids), + with :ok <- Progress.ensure_mark_complete_allowed(story, ctx.lifecycle_ids, ctx.trunk), {:ok, updated} <- apply_mark_complete(story) do create_mark_complete_result(tenant_id, story, orchestrator_agent_id, params) @@ -432,6 +437,10 @@ defmodule Loopctl.BulkOperations do # 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), + # US-47.6 (AC-47.6.1): the SAME trunk-evidence gate, applied to the landing the + # pre-transaction assessment was made for. + :ok <- + TrunkEvidence.ensure_trunk_verified(tenant_id, story.id, Map.get(ctx.trunk, 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) @@ -1001,6 +1010,18 @@ defmodule Loopctl.BulkOperations do defp format_reason(:chg_review_not_for_landed_revision), do: "chg_review_not_for_landed_revision: " <> Messages.chg_review_not_for_landed_revision() + # US-47.6: each leads with its code, so a bulk caller can branch on it. + defp format_reason(:chg_landing_not_trunk_verified), + do: "chg_landing_not_trunk_verified: " <> Messages.chg_landing_not_trunk_verified() + + defp format_reason(:chg_landing_reverted), + do: "chg_landing_reverted: " <> Messages.chg_landing_reverted() + + defp format_reason({:chg_trunk_unevaluated, reason, retry_after}), + do: + "chg_trunk_unevaluated: #{reason}; retry after #{retry_after} seconds. " <> + Messages.chg_trunk_unevaluated() + defp format_reason(:story_rejected), do: "story is rejected; investigate instead of marking it complete" diff --git a/lib/loopctl/changes/trunk_evidence.ex b/lib/loopctl/changes/trunk_evidence.ex new file mode 100644 index 00000000..4e0801b3 --- /dev/null +++ b/lib/loopctl/changes/trunk_evidence.ex @@ -0,0 +1,334 @@ +defmodule Loopctl.Changes.TrunkEvidence do + @moduledoc """ + The CI-evidence input of story verification for a story bound to a chg change (US-47.6, + Epic 47 PRD §7). A chg story's CI is not a pull request's checks: chg lands the change on + trunk and its trunk verifier advances `refs/heads/verified/master` (ff-only) past every + commit whose full suite passed. So the evidence is two facts, both about the CURRENT + binding's newest APPLIED `landed` event: + + 1. its TRUNK commit — `data.merge_sha`, the squash chg wrote onto trunk, never the envelope + `sha` (the revision commit, which is what `Loopctl.Changes.ReviewCustody` certifies and + which is not on trunk whenever trunk moved) — is an ancestor of, or equal to, + `verified/master`, asked of the forge by sha (`PullRequestSource.contains?/3`); and + 2. no APPLIED `reverted` event of the same change has a higher `event_seq` than that + landing. A re-landing after a revert is a newer landing, so it is judged afresh. + + A story with no change binding is `:not_chg` and every verify path takes exactly the gates + it took before. + + ## No local short-circuit + + The `verified` event's `data.sha` (`story_stages.trunk_verified_sha`) is chg's claim at + delivery time, not the ref. Even `merge_sha == trunk_verified_sha` is not taken as the + answer: the ref is the one fact chg's deploy guard reads, and a verify that skipped it + would certify on a delivery the ref may since have contradicted. One bounded read per + verify is the whole cost. + + ## Two halves, and the forge call is outside every transaction + + `assess/3` (one story) and `assess_many/3` (a batch) run BEFORE the verify transaction and + make the forge call there, so no story lock is ever held across a network read. + `ensure_trunk_verified/4` runs INSIDE the transaction, under the story lock, after + `ReviewCustody.ensure_landed_review/3`. It re-reads the landing and the revert from the + database and applies the assessment only when it was made for the SAME landing; a landing + that arrived in between answers unevaluated (`landing_changed`), never the stale answer. + + ## What each answer means to the caller + + - `:ok` — not a chg story, or the landing is in `verified/master` and not reverted + - `{:error, :chg_landing_not_trunk_verified}` — the forge answered and the trunk commit is + NOT in `verified/master` (`behind` or `diverged`), or the landing named no valid trunk + commit, or there is no current landing at all + - `{:error, :chg_landing_reverted}` — an applied `reverted` follows the landing + - `{:error, {:chg_trunk_unevaluated, reason, retry_after_seconds}}` — nothing was decided. + Every forge failure lands here, never in a pass: a timeout, an unreachable forge, a 5xx + or a rate limit (`forge_unavailable`, with the forge's own delay when it named one), a + 401 / 403 / 422 / other status (`forge_unauthorized`, `forge_forbidden`, + `forge_rejected`), an unreadable body (`forge_unreadable`), a 404 (`trunk_ref_not_found`: + `verified/master` does not exist yet — chg has not verified anything — or the trunk commit + is not on the forge yet; neither is a failure of the story), no credential for the + repository (`credential_unavailable`), and `landing_changed`. + + ## Reads + + `AdminRepo` (or the verify transaction's repo) with an explicit `tenant_id` predicate on + every query, the custody-read convention. + """ + + import Ecto.Query + + require Logger + + alias Loopctl.AdminRepo + alias Loopctl.Changes.Binding + alias Loopctl.Changes.Event + alias Loopctl.Delivery.MergePrecondition + alias Loopctl.Delivery.PullRequestSource + alias Loopctl.GitSha + alias Loopctl.Intake.Source + alias Loopctl.Verification.Credential + + @verified_ref "verified/master" + + # The `Retry-After` an unevaluated answer carries when the forge named no delay — the merge + # precondition's floor, so the two forge-backed 503s read alike. + @default_retry_after_seconds 30 + + # A batch asks the forge for at most this many stories at once; each call is bounded by the + # forge client's own connect (2 s) and receive (5 s) timeouts, and the task by this. + @batch_concurrency 4 + @batch_task_timeout_ms 15_000 + + @type reason :: String.t() + + @type outcome :: + :ancestor + | :not_ancestor + | :no_trunk_commit + | :reverted + | {:unevaluated, reason(), pos_integer()} + + @typedoc """ + What `assess/3` found BEFORE the transaction: `:not_chg`, or the landing it judged + (`nil` when there was none) and the outcome. + """ + @type assessment :: :not_chg | %{landed_event_id: Ecto.UUID.t() | nil, outcome: outcome() | nil} + + @type refusal :: + :chg_landing_not_trunk_verified + | :chg_landing_reverted + | {:chg_trunk_unevaluated, reason(), pos_integer()} + + @doc "The ref a chg landing must be contained in." + @spec verified_ref() :: String.t() + def verified_ref, do: @verified_ref + + @doc "The `Retry-After` an unevaluated answer carries when the forge named none." + @spec default_retry_after_seconds() :: pos_integer() + def default_retry_after_seconds, do: @default_retry_after_seconds + + @doc """ + The assessment for one story, made OUTSIDE any transaction. Asks the forge only for a chg + story whose current landing is not already reverted and names a valid trunk commit. + """ + @spec assess(Ecto.UUID.t(), Ecto.UUID.t()) :: assessment() + def assess(tenant_id, story_id) do + case facts(AdminRepo, tenant_id, story_id) do + :not_chg -> :not_chg + {:chg, nil} -> %{landed_event_id: nil, outcome: nil} + {:chg, landing} -> %{landed_event_id: landing.id, outcome: judge(tenant_id, landing)} + end + end + + @doc """ + `assess/2` for a batch, as a map keyed by story id. One query finds the bound stories; only + they are assessed, the forge calls `@batch_concurrency` at a time. A call that outlives its + task bound is `forge_unavailable` for that story. + """ + @spec assess_many(Ecto.UUID.t(), [Ecto.UUID.t()]) :: %{optional(Ecto.UUID.t()) => assessment()} + def assess_many(tenant_id, story_ids) when is_list(story_ids) do + ids = Enum.filter(story_ids, &match?({:ok, _}, Ecto.UUID.cast(&1))) + + bound = + AdminRepo.all( + from b in Binding, + where: b.tenant_id == ^tenant_id and b.story_id in ^ids, + distinct: true, + select: b.story_id + ) + + landings = + Enum.map(bound, fn story_id -> {story_id, facts(AdminRepo, tenant_id, story_id)} end) + + landings + |> Task.async_stream( + fn + {story_id, {:chg, nil}} -> + {story_id, %{landed_event_id: nil, outcome: nil}} + + {story_id, {:chg, landing}} -> + {story_id, %{landed_event_id: landing.id, outcome: judge(tenant_id, landing)}} + + {story_id, :not_chg} -> + {story_id, :not_chg} + end, + max_concurrency: @batch_concurrency, + timeout: @batch_task_timeout_ms, + on_timeout: :kill_task, + ordered: true + ) + |> Enum.zip(landings) + |> Map.new(fn + {{:ok, {story_id, assessment}}, _landing} -> + {story_id, assessment} + + {{:exit, _reason}, {story_id, {:chg, landing}}} -> + {story_id, + %{ + landed_event_id: landing && landing.id, + outcome: {:unevaluated, "forge_unavailable", @default_retry_after_seconds} + }} + + {{:exit, _reason}, {story_id, :not_chg}} -> + {story_id, :not_chg} + end) + end + + @doc """ + The in-transaction half: `:ok` or a refusal (see the moduledoc), from the landing as the + database holds it NOW and the `assessment` made before the transaction. A chg story with no + assessment, or one made for a different landing, is unevaluated `landing_changed`. Reads + on `repo` (default `AdminRepo`), tenant-scoped. + """ + @spec ensure_trunk_verified(Ecto.Repo.t(), Ecto.UUID.t(), Ecto.UUID.t(), assessment() | nil) :: + :ok | {:error, refusal()} + def ensure_trunk_verified(repo \\ AdminRepo, tenant_id, story_id, assessment) do + case facts(repo, tenant_id, story_id) do + :not_chg -> + :ok + + {:chg, nil} -> + {:error, :chg_landing_not_trunk_verified} + + {:chg, %{reverted?: true}} -> + refuse(tenant_id, story_id, :chg_landing_reverted) + + {:chg, %{id: landed_id}} -> + case assessment do + %{landed_event_id: ^landed_id, outcome: outcome} -> + decide(tenant_id, story_id, outcome) + + _stale_or_missing -> + unevaluated(tenant_id, story_id, "landing_changed", @default_retry_after_seconds) + end + end + end + + # -- the facts ----------------------------------------------------------------------------- + + # `:not_chg` when the story has no binding at all; `{:chg, nil}` when its newest binding is + # superseded or its change has no applied landing; else the newest applied landing. + defp facts(repo, 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{superseded_at: nil} = binding -> {:chg, landing(repo, tenant_id, binding)} + %Binding{} -> {:chg, nil} + end + end + + defp landing(repo, tenant_id, %Binding{} = binding) do + case newest_landing(repo, tenant_id, binding) do + nil -> + nil + + %{} = landed -> + %{ + id: landed.id, + repo: landed.repo, + merge_sha: merge_sha(landed.data), + reverted?: reverted_after?(repo, tenant_id, binding, landed.event_seq) + } + end + end + + # The newest APPLIED `landed` event of the binding's change, with the repository of the + # source that delivered it — the intake source's own `repo_full_name`, which the credential + # is asked for, as every other forge read asks. + defp newest_landing(repo, tenant_id, binding) do + repo.one( + from e in Event, + join: s in Source, + on: s.id == e.source_id and s.tenant_id == e.tenant_id, + 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: %{id: e.id, event_seq: e.event_seq, data: e.data, repo: s.repo_full_name} + ) + end + + defp reverted_after?(repo, tenant_id, binding, landed_seq) do + repo.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 == "reverted" and + not is_nil(e.applied_at) and e.event_seq > ^landed_seq + ) + end + + defp merge_sha(%{"merge_sha" => sha}) when is_binary(sha) do + if GitSha.valid?(sha), do: sha + end + + defp merge_sha(_data), do: nil + + # -- the forge ----------------------------------------------------------------------------- + + defp judge(_tenant_id, %{reverted?: true}), do: :reverted + defp judge(_tenant_id, %{merge_sha: nil}), do: :no_trunk_commit + + defp judge(tenant_id, %{repo: repo, merge_sha: sha}) do + with {:ok, forge_repo} <- Credential.repo(tenant_id, repo), + {:ok, contained?} <- PullRequestSource.contains?(forge_repo, sha, @verified_ref) do + if contained?, do: :ancestor, else: :not_ancestor + else + {:error, reason} -> {:unevaluated, code(reason), retry_after(reason)} + end + end + + defp code(:credential_unavailable), do: "credential_unavailable" + defp code({:github_api_error, 404}), do: "trunk_ref_not_found" + defp code({:github_api_error, 401}), do: "forge_unauthorized" + defp code({:github_api_error, 403}), do: "forge_forbidden" + + defp code(reason) do + cond do + MergePrecondition.transient?(reason) -> "forge_unavailable" + match?({:github_api_error, _}, reason) -> "forge_rejected" + true -> "forge_unreadable" + end + end + + defp retry_after(:credential_unavailable), do: Credential.configuration_retry_seconds() + + defp retry_after(reason), + do: MergePrecondition.retry_after(reason) || @default_retry_after_seconds + + # -- deciding ------------------------------------------------------------------------------ + + defp decide(_tenant_id, _story_id, :ancestor), do: :ok + + defp decide(tenant_id, story_id, :reverted), + do: refuse(tenant_id, story_id, :chg_landing_reverted) + + defp decide(tenant_id, story_id, {:unevaluated, reason, retry_after}), + do: unevaluated(tenant_id, story_id, reason, retry_after) + + defp decide(tenant_id, story_id, _not_ancestor_or_no_commit), + do: refuse(tenant_id, story_id, :chg_landing_not_trunk_verified) + + defp refuse(tenant_id, story_id, code) do + Logger.info("#{code}: tenant_id=#{tenant_id} story_id=#{story_id}") + {:error, code} + end + + defp unevaluated(tenant_id, story_id, reason, retry_after) do + Logger.warning( + "chg_trunk_unevaluated: tenant_id=#{tenant_id} story_id=#{story_id} reason=#{reason} " <> + "retry_after=#{retry_after}" + ) + + {:error, {:chg_trunk_unevaluated, reason, retry_after}} + end +end diff --git a/lib/loopctl/progress.ex b/lib/loopctl/progress.ex index 893a5afc..4aae82bb 100644 --- a/lib/loopctl/progress.ex +++ b/lib/loopctl/progress.ex @@ -23,6 +23,7 @@ defmodule Loopctl.Progress do alias Loopctl.Audit.AuditLog alias Loopctl.Capabilities alias Loopctl.Changes.ReviewCustody + alias Loopctl.Changes.TrunkEvidence alias Loopctl.Delivery.DispatchLease alias Loopctl.Delivery.RunnerStages alias Loopctl.Delivery.Stages @@ -2593,9 +2594,16 @@ defmodule Loopctl.Progress do 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 + - `{:error, :chg_landing_not_trunk_verified}` / `{:error, :chg_landing_reverted}` / + `{:error, {:chg_trunk_unevaluated, reason, retry_after_seconds}}` (US-47.6) — a chg story's + CI-evidence input: its landing's trunk commit is not in `verified/master`, a revert follows + the landing, or the forge read could not establish it + (`Loopctl.Changes.TrunkEvidence`). The forge is read BEFORE the transaction; the answer is + applied after the landed-review gate, under the story lock """ @spec verify_story(Ecto.UUID.t(), Ecto.UUID.t(), map(), keyword()) :: - {:ok, Story.t()} | {:error, atom() | {:invalid_transition, map()}} + {:ok, Story.t()} + | {:error, atom() | {:invalid_transition, map()} | TrunkEvidence.refusal()} def verify_story(tenant_id, story_id, params, opts \\ []) do verification_params = extract_verification_params(params) @@ -2638,6 +2646,10 @@ defmodule Loopctl.Progress do # story's recorded verifier; the caller comparison binds the principal that # actually makes the call, on EVERY path including the one that skips the # optional request-review. + # US-47.6: a chg story's CI evidence is read from the forge HERE, before the transaction, + # so no story lock is held across a network read. It is applied under the lock below. + trunk = TrunkEvidence.assess(tenant_id, story_id) + multi = Multi.new() |> Multi.run(:lock, fn _repo, _changes -> lock_story(tenant_id, story_id) end) @@ -2658,6 +2670,9 @@ defmodule Loopctl.Progress do |> Multi.run(:chg_landed_review, fn _repo, _changes -> chg_landed_review(tenant_id, story_id) end) + |> Multi.run(:chg_trunk_evidence, fn _repo, _changes -> + chg_trunk_evidence(tenant_id, story_id, trunk) + end) |> Multi.run(:story, fn _repo, %{lock: story} -> apply_verified_status(story) end) @@ -2702,6 +2717,18 @@ defmodule Loopctl.Progress do end end + # US-47.6 (AC-47.6.1): a story bound to a chg change passes its CI-evidence input only when + # its landing's trunk commit is in `verified/master` and no revert follows it + # (`Loopctl.Changes.TrunkEvidence`). `trunk` is the assessment made before the transaction; + # it is applied here, under the story lock, only to the landing it was made for. A no-op on + # every other story. + defp chg_trunk_evidence(tenant_id, story_id, trunk) do + case TrunkEvidence.ensure_trunk_verified(tenant_id, story_id, trunk) do + :ok -> {:ok, :trunk_verified} + {:error, reason} -> {:error, reason} + end + end + @doc """ Chain-of-custody guard for the bulk-verify path. @@ -2767,8 +2794,12 @@ defmodule Loopctl.Progress do `lifecycle_ids` is the batch's precomputed lifecycle-history set (see `stories_with_lifecycle_history/2`), so a 50-story batch costs one audit query rather than 50 inside the locking transaction. + + `trunk` is the batch's `Loopctl.Changes.TrunkEvidence.assess_many/2` map, made BEFORE the + transaction (US-47.6). A chg story missing from it is unevaluated (`landing_changed`), never + passed, so the default only serves a caller whose batch holds no chg story. """ - @spec ensure_mark_complete_allowed(Story.t(), MapSet.t()) :: + @spec ensure_mark_complete_allowed(Story.t(), MapSet.t(), map()) :: :ok | {:error, :already_verified @@ -2777,9 +2808,10 @@ defmodule Loopctl.Progress do | :story_entered_lifecycle | :story_in_progress | :review_missing_escalated - | :chg_review_not_for_landed_revision} - def ensure_mark_complete_allowed(story, lifecycle_ids) do - case guard_backfill(story, lifecycle_ids) do + | :chg_review_not_for_landed_revision + | TrunkEvidence.refusal()} + def ensure_mark_complete_allowed(story, lifecycle_ids, trunk \\ %{}) do + case guard_backfill(story, lifecycle_ids, Map.get(trunk, story.id)) do {:ok, _} -> :ok {:error, reason} -> {:error, reason} end @@ -2788,10 +2820,11 @@ defmodule Loopctl.Progress do # 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 + # revision, by any path — nor (US-47.6) without its landing in `verified/master`. + defp guard_backfill(story, lookup, trunk) do with {:ok, _} = ok <- guard_backfillable(story, lookup), - {:ok, _} <- chg_landed_review(story.tenant_id, story.id) do + {:ok, _} <- chg_landed_review(story.tenant_id, story.id), + {:ok, _} <- chg_trunk_evidence(story.tenant_id, story.id, trunk) do ok end end @@ -2840,6 +2873,9 @@ defmodule Loopctl.Progress do (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, :chg_landing_not_trunk_verified | :chg_landing_reverted}` / + `{:error, {:chg_trunk_unevaluated, reason, retry_after_seconds}}` (US-47.6) — the same + trunk-evidence gate as `verify_story/4` * `{:error, %Ecto.Changeset{}}` — persistence error surfaced from Multi step """ @spec backfill_story(Ecto.UUID.t(), Ecto.UUID.t(), map(), keyword()) :: @@ -2859,6 +2895,7 @@ defmodule Loopctl.Progress do | :chg_root_cannot_verify | :review_missing_escalated | :chg_review_not_for_landed_revision + | TrunkEvidence.refusal() | Ecto.Changeset.t()} def backfill_story(tenant_id, story_id, params, opts \\ []) do reason = params |> Map.get("reason") |> normalize_string() @@ -2914,10 +2951,13 @@ defmodule Loopctl.Progress do actor_id = Keyword.get(opts, :actor_id) actor_label = Keyword.get(opts, :actor_label) + # US-47.6: the forge read happens before the transaction, as on verify. + trunk = TrunkEvidence.assess(tenant_id, story_id) + multi = Multi.new() |> Multi.run(:lock, fn _repo, _changes -> lock_story(tenant_id, story_id) end) - |> Multi.run(:guard, fn _repo, %{lock: story} -> guard_backfill(story, :query) end) + |> Multi.run(:guard, fn _repo, %{lock: story} -> guard_backfill(story, :query, trunk) end) |> Multi.run(:story, fn _repo, %{lock: story} -> apply_backfill_status(story, reason, evidence_url, pr_number) end) @@ -3046,7 +3086,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. Both reach it through `guard_backfill/2`. + # batch path. Both reach it through `guard_backfill/3`. defp guard_backfillable(%{verified_status: :verified}, _lookup), do: {:error, :already_verified} diff --git a/lib/loopctl_web/controllers/bulk_operations_controller.ex b/lib/loopctl_web/controllers/bulk_operations_controller.ex index 1537b9f8..ff1ee544 100644 --- a/lib/loopctl_web/controllers/bulk_operations_controller.ex +++ b/lib/loopctl_web/controllers/bulk_operations_controller.ex @@ -64,8 +64,14 @@ defmodule LoopctlWeb.BulkOperationsController do "(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() <> ").", + "), `chg_review_not_for_landed_revision` (" <> + Messages.chg_review_not_for_landed_revision() <> + "), `chg_landing_not_trunk_verified` (" <> + Messages.chg_landing_not_trunk_verified() <> + "), `chg_landing_reverted` (" <> + Messages.chg_landing_reverted() <> + ") or `chg_trunk_unevaluated` (US-47.6; " <> + Messages.chg_trunk_unevaluated() <> ").", request_body: {"Verify params", "application/json", Schemas.BulkVerifyRequest}, responses: %{ 200 => {"Results", "application/json", Schemas.BulkResultResponse}, @@ -107,7 +113,9 @@ defmodule LoopctlWeb.BulkOperationsController do "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.", + "or `chg_review_not_for_landed_revision`, or with US-47.6's " <> + "`chg_landing_not_trunk_verified`, `chg_landing_reverted` or `chg_trunk_unevaluated`, " <> + "as on bulk verify.", request_body: {"Mark-complete params", "application/json", %OpenApiSpex.Schema{ diff --git a/lib/loopctl_web/controllers/story_verification_controller.ex b/lib/loopctl_web/controllers/story_verification_controller.ex index 6a9de8f4..41c6c69d 100644 --- a/lib/loopctl_web/controllers/story_verification_controller.ex +++ b/lib/loopctl_web/controllers/story_verification_controller.ex @@ -56,6 +56,20 @@ defmodule LoopctlWeb.StoryVerificationController do tags(["Progress"]) + # US-47.6: the 503 a chg story's trunk-evidence read answers when nothing was decided. + @chg_trunk_unevaluated_response { + "`chg_trunk_unevaluated` (" <> + Messages.chg_trunk_unevaluated() <> + ") — always with a `Retry-After` header: the forge's own delay when it named one, " <> + "else #{Loopctl.Changes.TrunkEvidence.default_retry_after_seconds()} seconds " <> + "(#{Loopctl.Verification.Credential.configuration_retry_seconds()} for " <> + "`credential_unavailable`). `error.reason` is one of `forge_unavailable`, " <> + "`trunk_ref_not_found`, `forge_unauthorized`, `forge_forbidden`, `forge_rejected`, " <> + "`forge_unreadable`, `credential_unavailable`, `landing_changed`.", + "application/json", + Schemas.ErrorResponse + } + operation(:verify, summary: "Verify story", description: @@ -67,7 +81,15 @@ defmodule LoopctlWeb.StoryVerificationController do "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`).", + "escalated over `review_missing` (409 `review_missing_escalated`). Its CI evidence " <> + "is chg's trunk verifier (US-47.6): the trunk commit of that landing (`data.merge_sha`) " <> + "must be an ancestor of, or equal to, the repository's `verified/master`, asked of " <> + "GitHub by sha with the tenant's own forge credential (409 " <> + "`chg_landing_not_trunk_verified` when it is not), and no applied `reverted` event may " <> + "follow the landing (409 `chg_landing_reverted`). A forge read that times out or " <> + "fails, a missing `verified/master` ref or commit, or no credential for the " <> + "repository is 503 `chg_trunk_unevaluated` with `Retry-After` — never a pass. A story " <> + "with no change binding takes exactly the gates it took before.", parameters: [id: [in: :path, type: :string, description: "Story UUID"]], request_body: {"Verification params", "application/json", Schemas.VerifyRequest}, responses: %{ @@ -78,11 +100,15 @@ defmodule LoopctlWeb.StoryVerificationController do 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}, + "), `chg_review_not_for_landed_revision` (" <> + Messages.chg_review_not_for_landed_revision() <> + "), `chg_landing_not_trunk_verified` (" <> + Messages.chg_landing_not_trunk_verified() <> + ") or `chg_landing_reverted` (" <> + Messages.chg_landing_reverted() <> ")", "application/json", Schemas.ErrorResponse}, 422 => {"No review record found", "application/json", Schemas.ErrorResponse}, - 429 => {"Rate limit exceeded", "application/json", Schemas.RateLimitError} + 429 => {"Rate limit exceeded", "application/json", Schemas.RateLimitError}, + 503 => @chg_trunk_unevaluated_response } ) @@ -129,7 +155,9 @@ defmodule LoopctlWeb.StoryVerificationController do "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`. " <> + "`chg_review_not_for_landed_revision` — and the same trunk-evidence gate (US-47.6): " <> + "409 `chg_landing_not_trunk_verified` or `chg_landing_reverted`, 503 " <> + "`chg_trunk_unevaluated` with `Retry-After`. " <> "Emits a `story.backfilled` webhook event on success.", parameters: [id: [in: :path, type: :string, description: "Story UUID"]], request_body: @@ -156,9 +184,13 @@ defmodule LoopctlWeb.StoryVerificationController do 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}, + "), `chg_review_not_for_landed_revision` (" <> + Messages.chg_review_not_for_landed_revision() <> + "), `chg_landing_not_trunk_verified` (" <> + Messages.chg_landing_not_trunk_verified() <> + ") or `chg_landing_reverted` (" <> + Messages.chg_landing_reverted() <> ")", "application/json", Schemas.ErrorResponse}, + 503 => @chg_trunk_unevaluated_response, 422 => {"Validation error: missing reason, already verified, already rejected, has dispatch lineage, or already entered the lifecycle", "application/json", Schemas.ErrorResponse}, @@ -238,8 +270,14 @@ defmodule LoopctlWeb.StoryVerificationController do "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() <> ").", + "), `:chg_review_not_for_landed_revision` (" <> + Messages.chg_review_not_for_landed_revision() <> + "), `:chg_landing_not_trunk_verified` (" <> + Messages.chg_landing_not_trunk_verified() <> + "), `:chg_landing_reverted` (" <> + Messages.chg_landing_reverted() <> + ") or `{:chg_trunk_unevaluated, reason, retry_after_seconds}` (" <> + Messages.chg_trunk_unevaluated() <> ").", parameters: [id: [in: :path, type: :string, description: "Epic UUID"]], request_body: {"Verification params", "application/json", Schemas.VerifyRequest}, responses: %{ @@ -374,10 +412,15 @@ defmodule LoopctlWeb.StoryVerificationController do :not_found, :chg_root_cannot_verify, :review_missing_escalated, - :chg_review_not_for_landed_revision + :chg_review_not_for_landed_revision, + :chg_landing_not_trunk_verified, + :chg_landing_reverted ] -> {:error, reason} + {:error, {:chg_trunk_unevaluated, _reason, _retry_after}} = unevaluated -> + unevaluated + {:error, %Ecto.Changeset{} = changeset} -> {:error, changeset} diff --git a/lib/loopctl_web/fallback_controller.ex b/lib/loopctl_web/fallback_controller.ex index 37453964..2ef0a8bf 100644 --- a/lib/loopctl_web/fallback_controller.ex +++ b/lib/loopctl_web/fallback_controller.ex @@ -54,6 +54,8 @@ defmodule LoopctlWeb.FallbackController do - `{: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, :chg_landing_not_trunk_verified}` / `{:error, :chg_landing_reverted}` -> 409 (US-47.6: a chg story's landing is not in `verified/master`, or was reverted) + - `{:error, {:chg_trunk_unevaluated, reason, retry_after}}` -> 503 with `Retry-After` (US-47.6: the trunk-evidence read could not be made) - `{: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 — @@ -606,7 +608,9 @@ defmodule LoopctlWeb.FallbackController do :chg_root_cannot_verify, :chg_root_review_only, :review_missing_escalated, - :chg_review_not_for_landed_revision + :chg_review_not_for_landed_revision, + :chg_landing_not_trunk_verified, + :chg_landing_reverted ] do conn |> put_status(:conflict) @@ -620,6 +624,25 @@ defmodule LoopctlWeb.FallbackController do }) end + # US-47.6 (AC-47.6.2): a chg story's trunk evidence could not be established. Nothing was + # decided, so it is a 503 with `Retry-After`, never a pass and never a refusal. + def call(conn, {:error, {:chg_trunk_unevaluated, reason, retry_after}}) + when is_binary(reason) and is_integer(retry_after) and retry_after > 0 do + conn + |> put_resp_header("retry-after", Integer.to_string(retry_after)) + |> put_status(:service_unavailable) + |> json(%{ + error: %{ + status: 503, + code: "chg_trunk_unevaluated", + decision: "unevaluated", + reason: reason, + message: Messages.chg_trunk_unevaluated(), + retry_after_seconds: retry_after + } + }) + end + def call(conn, {:error, :commit_sha_not_bound}) do conn |> put_status(:unprocessable_entity) @@ -1376,4 +1399,9 @@ defmodule LoopctlWeb.FallbackController do defp chg_custody_message(:chg_review_not_for_landed_revision), do: Messages.chg_review_not_for_landed_revision() + + defp chg_custody_message(:chg_landing_not_trunk_verified), + do: Messages.chg_landing_not_trunk_verified() + + defp chg_custody_message(:chg_landing_reverted), do: Messages.chg_landing_reverted() end diff --git a/mcp-server/CHANGELOG.md b/mcp-server/CHANGELOG.md index 224559c1..f5782ed0 100644 --- a/mcp-server/CHANGELOG.md +++ b/mcp-server/CHANGELOG.md @@ -5,6 +5,18 @@ 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.4.0 — 2026-10-19 (chg trunk evidence) + +### Changed + +- **`verify_story`, `verify_all_in_epic`, `bulk_mark_complete` and `backfill_story` document the + chg trunk-evidence gate** (loopctl US-47.6): a story bound to a chg change is verified only + when the trunk commit of its newest landing (`data.merge_sha`) is an ancestor of the repo's + `verified/master` (409 `chg_landing_not_trunk_verified` otherwise) and no `reverted` event + follows the landing (409 `chg_landing_reverted`). A forge read that cannot answer is 503 + `chg_trunk_unevaluated` with `Retry-After`, never a pass; the bulk paths report each as a + per-story error. + ## 3.3.0 — 2026-10-19 (chg review custody) ### Added diff --git a/mcp-server/index.js b/mcp-server/index.js index 6f77076b..02da0dec 100755 --- a/mcp-server/index.js +++ b/mcp-server/index.js @@ -4278,7 +4278,10 @@ const TOOLS = [ "REFUSED for stories that have any loopctl dispatch lineage — non-pending agent_status, assigned_agent_id, implementer_dispatch_id, " + "or verifier_dispatch_id set. Also refused for stories already `:verified` (idempotent no-op when the same payload is sent) or `:rejected`. " + "Records a provenance marker in `metadata.backfill` plus an audit event and a `story.backfilled` webhook. " + - "REQUIRES `reason`. Strongly recommend passing `evidence_url` (http/https, no credentials in userinfo) and `pr_number`.", + "REQUIRES `reason`. Strongly recommend passing `evidence_url` (http/https, no credentials in userinfo) and `pr_number`. " + + "A story bound to a chg change takes verify_story's chg gates too: 409 review_missing_escalated, " + + "chg_review_not_for_landed_revision, chg_landing_not_trunk_verified or chg_landing_reverted, and 503 " + + "chg_trunk_unevaluated with Retry-After (loopctl US-47.6).", inputSchema: { type: "object", properties: { @@ -4978,7 +4981,15 @@ const TOOLS = [ "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).", + "review_missing_escalated — a human resolves it first (resolve_escalation). " + + "Its CI evidence is chg's trunk verifier (loopctl US-47.6): the trunk commit of that " + + "landing (data.merge_sha) must be an ancestor of the repo's verified/master, else 409 " + + "chg_landing_not_trunk_verified (retry once chg has verified it), and no reverted event " + + "may follow the landing, else 409 chg_landing_reverted. A forge read that times out or " + + "fails, a missing verified/master ref, or no forge credential for the repo is 503 " + + "chg_trunk_unevaluated with Retry-After (error.reason names which) — nothing was " + + "decided; retry after that many seconds. The forge credential (the tenant's own GitHub " + + "token) needs read access to the repo's contents.", inputSchema: { type: "object", properties: { @@ -5035,7 +5046,8 @@ const TOOLS = [ "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. " + "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.", + "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, " + + "or with chg_landing_not_trunk_verified, chg_landing_reverted or chg_trunk_unevaluated, the same trunk-evidence gate (loopctl US-47.6).", inputSchema: { type: "object", properties: { @@ -5073,7 +5085,9 @@ const TOOLS = [ "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.", + "chg_review_not_for_landed_revision, the same landed-review gate verify_story applies, " + + "or with chg_landing_not_trunk_verified, chg_landing_reverted or chg_trunk_unevaluated " + + "(the trunk-evidence gate verify_story applies, loopctl US-47.6).", inputSchema: { type: "object", properties: { diff --git a/mcp-server/package-lock.json b/mcp-server/package-lock.json index a9f8fe9d..0ad42a64 100644 --- a/mcp-server/package-lock.json +++ b/mcp-server/package-lock.json @@ -1,12 +1,12 @@ { "name": "loopctl-mcp-server", - "version": "3.3.0", + "version": "3.4.0", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "loopctl-mcp-server", - "version": "3.3.0", + "version": "3.4.0", "license": "MIT", "dependencies": { "@modelcontextprotocol/sdk": "^1.28.0" diff --git a/mcp-server/package.json b/mcp-server/package.json index 1c9cda52..7910aa26 100644 --- a/mcp-server/package.json +++ b/mcp-server/package.json @@ -1,6 +1,6 @@ { "name": "loopctl-mcp-server", - "version": "3.3.0", + "version": "3.4.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 index dc345c6d..f9e7da97 100644 --- a/mcp-server/test/chg_review_custody_tools.test.js +++ b/mcp-server/test/chg_review_custody_tools.test.js @@ -79,6 +79,22 @@ describe("descriptions name the US-47.5 refusals", () => { }); } + // loopctl US-47.6: the trunk-evidence gate, on every tool that reaches `verified`. + for (const name of ["verify_story", "verify_all_in_epic", "bulk_mark_complete", "backfill_story"]) { + test(`${name}: the chg trunk-evidence gate (US-47.6)`, () => { + const d = tool(name).description; + assert.match(d, /chg_landing_not_trunk_verified/); + assert.match(d, /chg_landing_reverted/); + assert.match(d, /chg_trunk_unevaluated/); + }); + } + + test("verify_story: the unevaluated answer is a 503 with Retry-After, never a pass", () => { + const d = tool("verify_story").description; + assert.match(d, /503 chg_trunk_unevaluated with Retry-After/); + assert.match(d, /verified\/master/); + }); + 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/test/loopctl/changes/review_custody_test.exs b/test/loopctl/changes/review_custody_test.exs index 0c6b832a..dc9647b0 100644 --- a/test/loopctl/changes/review_custody_test.exs +++ b/test/loopctl/changes/review_custody_test.exs @@ -857,6 +857,14 @@ defmodule Loopctl.Changes.ReviewCustodyTest do defp unverified!(story), do: assert(AdminRepo.get!(Story, story.id).verified_status == :unverified) + # US-47.6: a chg story that clears this gate still needs its landing in verified/master. + # These tests are about the landed-review gate, so the forge says it is. + defp trunk_verified!, + do: + stub(Loopctl.MockPullRequestSource, :contains?, fn _, _, "verified/master" -> + {:ok, true} + end) + 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 @@ -887,6 +895,7 @@ defmodule Loopctl.Changes.ReviewCustodyTest do actor_lineage: [] ) + trunk_verified!() assert {:ok, %Story{verified_status: :verified}} = verify(ctx, story) end @@ -906,6 +915,7 @@ defmodule Loopctl.Changes.ReviewCustodyTest do unverified!(story) assert {:ok, _} = review(ctx, story, child(ctx).lineage_path, %{"commit_sha" => sha("b")}) + trunk_verified!() assert {:ok, %Story{verified_status: :verified}} = verify(ctx, story) end diff --git a/test/loopctl/changes/trunk_evidence_test.exs b/test/loopctl/changes/trunk_evidence_test.exs new file mode 100644 index 00000000..42850edb --- /dev/null +++ b/test/loopctl/changes/trunk_evidence_test.exs @@ -0,0 +1,452 @@ +defmodule Loopctl.Changes.TrunkEvidenceTest do + @moduledoc """ + US-47.6: a chg story's CI-evidence input on every verify path (Epic 47 PRD §7) — its + landing's TRUNK commit (`data.merge_sha`) must be in `verified/master`, asked of the forge by + sha, and no applied `reverted` may follow the landing. The forge is the Mox + `Loopctl.MockPullRequestSource` (`contains?/3`, fail-closed by default in DataCase). The HTTP + surface is `LoopctlWeb.ChgTrunkVerificationControllerTest`'s. + """ + + use Loopctl.DataCase, async: true + + alias Loopctl.AdminRepo + alias Loopctl.BulkOperations + alias Loopctl.Changes.Events + alias Loopctl.Changes.Signature + alias Loopctl.Changes.TrunkEvidence + alias Loopctl.Delivery.ForgeRepo + alias Loopctl.Dispatches + alias Loopctl.MockPullRequestSource + alias Loopctl.MockVerificationCredential + alias Loopctl.Progress + alias Loopctl.WorkBreakdown.Story + + setup :verify_on_exit! + + @repo "mkreyman/infra" + + setup do + %{secret: secret, source: source, root: root} = fixture(:chg_source, %{}) + tenant_id = source.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}) + + epic = fixture(:epic, %{tenant_id: tenant_id}) + + %{ + tenant_id: tenant_id, + secret: secret, + source: source, + root: root, + impl: impl, + impl_agent: impl_agent, + epic: epic, + story: reported_done(tenant_id, epic, impl_agent, impl) + } + end + + defp reported_done(tenant_id, epic, impl_agent, impl) do + 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!() + end + + defp sha(char), do: String.duplicate(char, 40) + + defp push!(ctx, change_id, seq, kind, attrs) do + raw = + Jason.encode!( + build(:chg_event, Map.merge(%{change_id: change_id, 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 + + # Submitted as revision `rev_sha`, landed onto trunk as `merge_sha`, and reviewed by a chg + # child naming the landed revision: everything US-47.5 asks, so the trunk evidence is the + # only thing left between the story and `verified`. No stage row, so nothing escalates. + defp landed_and_reviewed(ctx, story, change_id, rev_sha, merge_sha) do + :ok = push!(ctx, change_id, 1, "submitted", %{sha: rev_sha, data: %{"story_id" => story.id}}) + :ok = push!(ctx, change_id, 2, "landed", %{sha: rev_sha, data: %{"merge_sha" => merge_sha}}) + review_landed!(ctx, story, rev_sha) + story + end + + defp review_landed!(ctx, story, rev_sha) do + child = fixture(:chg_root_dispatch, %{tenant_id: ctx.tenant_id, parent: ctx.root}) + + assert {:ok, _} = + Progress.record_review( + ctx.tenant_id, + story.id, + %{"review_type" => "chg", "summary" => "chg review", "commit_sha" => rev_sha}, + reviewer_agent_id: fixture(:agent, %{tenant_id: ctx.tenant_id}).id, + reviewer_lineage: child.lineage_path + ) + end + + # A never-dispatched story imported at `reported_done` — the one shape backfill and bulk + # mark-complete accept — bound and landed. Its chg review is written directly: review-complete + # refuses a custody-orphaned story, so on these paths the trunk gate is defence in depth + # behind US-47.5's, and this is how it is reached at all. + defp backfillable_landed(ctx, change_id, rev_sha, merge_sha) do + story = + fixture(:story, %{tenant_id: ctx.tenant_id, epic_id: ctx.epic.id}) + |> Ecto.Changeset.change(%{agent_status: :reported_done}) + |> AdminRepo.update!() + + :ok = push!(ctx, change_id, 1, "submitted", %{sha: rev_sha, data: %{"story_id" => story.id}}) + :ok = push!(ctx, change_id, 2, "landed", %{sha: rev_sha, data: %{"merge_sha" => merge_sha}}) + + fixture(:review_record, %{ + tenant_id: ctx.tenant_id, + story_id: story.id, + commit_sha: rev_sha, + reviewer_dispatch_id: + fixture(:chg_root_dispatch, %{tenant_id: ctx.tenant_id, parent: ctx.root}).id + }) + + AdminRepo.get!(Story, story.id) + end + + defp outsider(ctx) do + agent = fn -> fixture(:agent, %{tenant_id: ctx.tenant_id}).id end + + {:ok, %{dispatch: other_root}} = + Dispatches.create_dispatch(ctx.tenant_id, %{role: :orchestrator, agent_id: agent.()}) + + {:ok, %{dispatch: other}} = + Dispatches.create_dispatch(ctx.tenant_id, %{ + role: :orchestrator, + agent_id: agent.(), + parent_dispatch_id: other_root.id + }) + + other + end + + 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)) + + # The forge's answer for `merge_sha` against `verified/master`, asserting the call names the + # TRUNK commit and the ref — never the revision commit — and runs outside every transaction. + defp forge(merge_sha, answer, times \\ 1) do + expect(MockPullRequestSource, :contains?, times, fn %ForgeRepo{full_name: @repo}, + ^merge_sha, + "verified/master" -> + refute Loopctl.Repo.in_transaction?() + refute AdminRepo.in_transaction?() + answer + end) + end + + defp no_forge_call, do: expect(MockPullRequestSource, :contains?, 0, fn _, _, _ -> :unused end) + + defp unverified!(story), + do: assert(AdminRepo.get!(Story, story.id).verified_status == :unverified) + + describe "AC-47.6.1: the landing's trunk commit must be in verified/master" do + test "an ancestor (or the same commit) verifies; the forge is asked about merge_sha", ctx do + story = landed_and_reviewed(ctx, ctx.story, "C1", sha("a"), sha("c")) + forge(sha("c"), {:ok, true}) + + assert {:ok, %Story{verified_status: :verified}} = verify(ctx, story) + end + + test "TC-47.6.1: not yet in verified/master is chg_landing_not_trunk_verified", ctx do + story = landed_and_reviewed(ctx, ctx.story, "C1", sha("a"), sha("c")) + forge(sha("c"), {:ok, false}) + + assert {:error, :chg_landing_not_trunk_verified} = verify(ctx, story) + unverified!(story) + end + + test "a landing naming no valid trunk commit is refused without asking the forge", ctx do + story = landed_and_reviewed(ctx, ctx.story, "C1", sha("a"), "not-a-sha") + no_forge_call() + + assert {:error, :chg_landing_not_trunk_verified} = verify(ctx, story) + unverified!(story) + end + + test "TC-47.6.2: landed, verified, then reverted is chg_landing_reverted, forge unasked", + ctx do + story = landed_and_reviewed(ctx, ctx.story, "C1", sha("a"), sha("c")) + :ok = push!(ctx, "C1", 3, "verified", %{sha: sha("a"), data: %{"sha" => sha("c")}}) + + :ok = + push!(ctx, "C1", 4, "reverted", %{ + sha: sha("a"), + data: %{"landed_sha" => sha("c"), "revert_sha" => sha("d")} + }) + + no_forge_call() + + assert {:error, :chg_landing_reverted} = verify(ctx, story) + unverified!(story) + end + + test "a re-landing after the revert is a NEW landing, judged afresh", ctx do + story = landed_and_reviewed(ctx, ctx.story, "C1", sha("a"), sha("c")) + + :ok = + push!(ctx, "C1", 3, "reverted", %{sha: sha("a"), data: %{"landed_sha" => sha("c")}}) + + :ok = push!(ctx, "C1", 4, "submitted", %{sha: sha("b"), data: %{"story_id" => story.id}}) + :ok = push!(ctx, "C1", 5, "landed", %{sha: sha("b"), data: %{"merge_sha" => sha("e")}}) + review_landed!(ctx, story, sha("b")) + + forge(sha("e"), {:ok, true}) + assert {:ok, %Story{verified_status: :verified}} = verify(ctx, story) + end + + test "a local trunk_verified_sha equal to merge_sha is not taken as the answer", ctx do + story = landed_and_reviewed(ctx, ctx.story, "C1", sha("a"), sha("c")) + :ok = push!(ctx, "C1", 3, "verified", %{sha: sha("a"), data: %{"sha" => sha("c")}}) + forge(sha("c"), {:ok, false}) + + assert {:error, :chg_landing_not_trunk_verified} = verify(ctx, story) + end + end + + describe "AC-47.6.2: a compare that cannot answer is unevaluated, never a pass" do + for {name, answer, reason, retry} <- [ + {"TC-47.6.3: a timeout", {:error, {:github_unreachable, :timeout}}, "forge_unavailable", + 30}, + {"a 5xx", {:error, {:github_api_error, 502}}, "forge_unavailable", 30}, + {"a rate limit, with the forge's own delay", {:error, {:github_rate_limited, 429, 120}}, + "forge_unavailable", 120}, + {"a 404: verified/master (or the commit) does not exist yet", + {:error, {:github_api_error, 404}}, "trunk_ref_not_found", 30}, + {"a 401", {:error, {:github_api_error, 401}}, "forge_unauthorized", 30}, + {"a permission 403", {:error, {:github_api_error, 403}}, "forge_forbidden", 30}, + {"a 422", {:error, {:github_api_error, 422}}, "forge_rejected", 30}, + {"an unreadable body", {:error, {:unreadable_compare, :x}}, "forge_unreadable", 30} + ] do + test "#{name} is chg_trunk_unevaluated #{reason}", ctx do + story = landed_and_reviewed(ctx, ctx.story, "C1", sha("a"), sha("c")) + forge(sha("c"), unquote(Macro.escape(answer))) + + assert {:error, {:chg_trunk_unevaluated, unquote(reason), unquote(retry)}} = + verify(ctx, story) + + unverified!(story) + end + end + + test "no credential for the repository waits the configuration interval, forge unasked", + ctx do + story = landed_and_reviewed(ctx, ctx.story, "C1", sha("a"), sha("c")) + + stub(MockVerificationCredential, :for_read, fn _tenant, @repo -> + {:error, :credential_unavailable} + end) + + no_forge_call() + + assert {:error, {:chg_trunk_unevaluated, "credential_unavailable", 3600}} = + verify(ctx, story) + end + + test "an assessment made for an EARLIER landing is landing_changed, never applied", ctx do + story = landed_and_reviewed(ctx, ctx.story, "C1", sha("a"), sha("c")) + forge(sha("c"), {:ok, true}) + stale = TrunkEvidence.assess(ctx.tenant_id, story.id) + assert %{outcome: :ancestor} = stale + + :ok = push!(ctx, "C1", 3, "submitted", %{sha: sha("b"), data: %{"story_id" => story.id}}) + :ok = push!(ctx, "C1", 4, "landed", %{sha: sha("b"), data: %{"merge_sha" => sha("e")}}) + + assert {:error, {:chg_trunk_unevaluated, "landing_changed", 30}} = + TrunkEvidence.ensure_trunk_verified(ctx.tenant_id, story.id, stale) + + assert {:error, {:chg_trunk_unevaluated, "landing_changed", 30}} = + TrunkEvidence.ensure_trunk_verified(ctx.tenant_id, story.id, nil) + end + end + + describe "AC-47.6.3: every other gate unchanged" do + test "a custody refusal is answered before the trunk evidence, even with the forge down", + ctx do + story = landed_and_reviewed(ctx, ctx.story, "C1", sha("a"), sha("c")) + + stub(MockPullRequestSource, :contains?, fn _, _, _ -> {:error, {:github_api_error, 503}} end) + + assert {:error, :self_verify_blocked} = + Progress.verify_story(ctx.tenant_id, story.id, %{}, + orchestrator_agent_id: ctx.impl_agent.id, + verifier_lineage: ctx.impl.lineage_path + ) + end + + test "the landed-review gate still comes first", ctx do + # A review recorded off the chg path, before the binding: review_not_conducted is + # satisfied, US-47.5's landed-review gate is not. + off_chg = outsider(ctx) + + {:ok, _} = + Progress.record_review(ctx.tenant_id, ctx.story.id, %{"review_type" => "enhanced"}, + reviewer_agent_id: off_chg.agent_id, + reviewer_lineage: off_chg.lineage_path + ) + + :ok = + push!(ctx, "C1", 1, "submitted", %{sha: sha("a"), data: %{"story_id" => ctx.story.id}}) + + :ok = push!(ctx, "C1", 2, "landed", %{sha: sha("a"), data: %{"merge_sha" => sha("c")}}) + stub(MockPullRequestSource, :contains?, fn _, _, _ -> {:ok, true} end) + + assert {:error, :chg_review_not_for_landed_revision} = verify(ctx, ctx.story) + end + + test "a story with NO change binding never asks the forge, on any path", ctx do + no_forge_call() + outsider = outsider(ctx) + + {:ok, _} = + Progress.record_review(ctx.tenant_id, ctx.story.id, %{"review_type" => "enhanced"}, + reviewer_agent_id: outsider.agent_id, + reviewer_lineage: outsider.lineage_path + ) + + assert :not_chg = TrunkEvidence.assess(ctx.tenant_id, ctx.story.id) + assert {:ok, %Story{verified_status: :verified}} = 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"}, []) + + third = fixture(:story, %{tenant_id: ctx.tenant_id, epic_id: ctx.epic.id}) + + assert {:ok, [%{status: "success"}]} = + BulkOperations.bulk_mark_complete( + ctx.tenant_id, + [%{"story_id" => third.id, "summary" => "pre", "review_type" => "pre"}], + outsider.agent_id, + [] + ) + end + end + + describe "every verify path takes the trunk gate" do + test "verify-all and bulk verify refuse per story; a pass on the forge verifies", ctx do + story = landed_and_reviewed(ctx, ctx.story, "C1", sha("a"), sha("c")) + forge(sha("c"), {:ok, false}, 2) + opts = verifier_opts(ctx) + + assert {:ok, %{verified_count: 0, errors: [%{reason: ":chg_landing_not_trunk_verified"}]}} = + Progress.verify_all_in_epic(ctx.tenant_id, ctx.epic.id, %{"summary" => "a"}, opts) + + assert {:ok, [%{status: "error", reason: "chg_landing_not_trunk_verified: " <> _}]} = + BulkOperations.bulk_verify( + ctx.tenant_id, + [%{"story_id" => story.id}], + opts[:orchestrator_agent_id], + opts + ) + + unverified!(story) + + forge(sha("c"), {:error, {:github_unreachable, :timeout}}) + + assert {:ok, [%{status: "error", reason: "chg_trunk_unevaluated: forge_unavailable" <> _}]} = + BulkOperations.bulk_verify( + ctx.tenant_id, + [%{"story_id" => story.id}], + opts[:orchestrator_agent_id], + opts + ) + + forge(sha("c"), {:ok, true}) + + assert {:ok, [%{status: "success"}]} = + BulkOperations.bulk_verify( + ctx.tenant_id, + [%{"story_id" => story.id}], + opts[:orchestrator_agent_id], + opts + ) + end + + test "backfill and bulk mark-complete take the same gate", ctx do + pending = backfillable_landed(ctx, "C2", sha("a"), sha("c")) + forge(sha("c"), {:ok, false}, 2) + + assert {:error, :chg_landing_not_trunk_verified} = + Progress.backfill_story(ctx.tenant_id, pending.id, %{"reason" => "pre"}, []) + + assert {:ok, [%{status: "error", reason: "chg_landing_not_trunk_verified: " <> _}]} = + BulkOperations.bulk_mark_complete( + ctx.tenant_id, + [%{"story_id" => pending.id, "summary" => "pre", "review_type" => "pre"}], + Ecto.UUID.generate(), + [] + ) + + unverified!(pending) + end + + test "a chg story missing from a batch's assessment is never passed", ctx do + pending = backfillable_landed(ctx, "C2", sha("a"), sha("c")) + no_forge_call() + + assert {:error, {:chg_trunk_unevaluated, "landing_changed", 30}} = + Progress.ensure_mark_complete_allowed( + pending, + MapSet.new(), + %{} + ) + end + end + + describe "tenant isolation" do + test "another tenant reading this story id finds no binding and asks nothing", ctx do + story = landed_and_reviewed(ctx, ctx.story, "C1", sha("a"), sha("c")) + other_tenant = fixture(:tenant) + no_forge_call() + + assert :not_chg = TrunkEvidence.assess(other_tenant.id, story.id) + assert %{} == TrunkEvidence.assess_many(other_tenant.id, [story.id]) + assert :ok = TrunkEvidence.ensure_trunk_verified(other_tenant.id, story.id, nil) + end + + test "another tenant's revert of the same change id reverts nothing here", ctx do + story = landed_and_reviewed(ctx, ctx.story, "C1", sha("a"), sha("c")) + other = fixture(:chg_source, %{}) + other_story = fixture(:story, %{tenant_id: other.source.tenant_id}) + octx = %{source: other.source, secret: other.secret} + + :ok = + push!(octx, "C1", 1, "submitted", %{sha: sha("a"), data: %{"story_id" => other_story.id}}) + + :ok = push!(octx, "C1", 2, "landed", %{sha: sha("a"), data: %{"merge_sha" => sha("c")}}) + :ok = push!(octx, "C1", 3, "reverted", %{sha: sha("a"), data: %{"landed_sha" => sha("c")}}) + + forge(sha("c"), {:ok, true}) + assert {:ok, %Story{verified_status: :verified}} = verify(ctx, story) + end + end +end diff --git a/test/loopctl/delivery/github_pull_request_source_test.exs b/test/loopctl/delivery/github_pull_request_source_test.exs index 54816991..c6267afb 100644 --- a/test/loopctl/delivery/github_pull_request_source_test.exs +++ b/test/loopctl/delivery/github_pull_request_source_test.exs @@ -712,6 +712,31 @@ defmodule Loopctl.Delivery.GitHubPullRequestSourceTest do Source.contains?(@repo, @head, @merge_base) end + # US-47.6: a chg story's trunk evidence asks `contains?(repo, merge_sha, "verified/master")`. + # The slash-bearing branch name goes into the path as it is (GitHub resolves it as the + # branch `refs/heads/verified/master`), the landed commit is the BASE, and the verdict is + # the same `status` reading: `ahead`/`identical` is an ancestor, `behind`/`diverged` is not. + test "US-47.6: the verified/master ref is the HEAD of the comparison, the commit its BASE" do + stub(fn conn -> + assert conn.request_path == "/repos/acme/widgets/compare/#{@head}...verified/master" + json(conn, %{"status" => "ahead", "ahead_by" => 3, "behind_by" => 0}) + end) + + assert {:ok, true} = Source.contains?(@repo, @head, "verified/master") + end + + test "US-47.6: a missing ref is a 404 and an unreachable forge is :github_unreachable" do + stub(fn conn -> Plug.Conn.resp(conn, 404, ~s({"message":"Not Found"})) end) + + assert {:error, {:github_api_error, 404}} = + Source.contains?(@repo, @head, "verified/master") + + stub(fn conn -> Req.Test.transport_error(conn, :timeout) end) + + assert {:error, {:github_unreachable, _}} = + Source.contains?(@repo, @head, "verified/master") + end + test "both refs are validated: either one is spliced into the URL path" do stub(fn conn -> flunk("must not reach the forge: #{conn.request_path}") end) diff --git a/test/loopctl_web/controllers/chg_trunk_verification_controller_test.exs b/test/loopctl_web/controllers/chg_trunk_verification_controller_test.exs new file mode 100644 index 00000000..cc1829bf --- /dev/null +++ b/test/loopctl_web/controllers/chg_trunk_verification_controller_test.exs @@ -0,0 +1,259 @@ +defmodule LoopctlWeb.ChgTrunkVerificationControllerTest do + @moduledoc """ + US-47.6 over HTTP: the response a chg story's trunk-evidence gate gives on the verify routes — + the 409 refusals, the 503 `chg_trunk_unevaluated` with its `Retry-After`, and the OpenAPI + declarations. The decisions are `Loopctl.Changes.TrunkEvidenceTest`'s. Keys are + dispatch-minted, so the caller's lineage is resolved server-side from the key. + """ + + 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.MockPullRequestSource + alias Loopctl.Progress + alias Loopctl.WorkBreakdown.Story + + setup :verify_on_exit! + + setup do + %{secret: secret, source: source, root: root} = fixture(:chg_source, %{}) + tenant_id = source.tenant_id + agent = fn -> fixture(:agent, %{tenant_id: tenant_id, agent_type: :orchestrator}).id end + + {: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, %{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!() + + ctx = %{tenant_id: tenant_id, secret: secret, source: source, root: root, epic: epic} + + :ok = push!(ctx, 1, "submitted", %{sha: sha("a"), data: %{"story_id" => story.id}}) + :ok = push!(ctx, 2, "landed", %{sha: sha("a"), data: %{"merge_sha" => sha("c")}}) + child = fixture(:chg_root_dispatch, %{tenant_id: tenant_id, parent: root}) + + {:ok, _} = + Progress.record_review( + tenant_id, + story.id, + %{"review_type" => "chg", "summary" => "chg review", "commit_sha" => sha("a")}, + reviewer_agent_id: fixture(:agent, %{tenant_id: tenant_id}).id, + reviewer_lineage: child.lineage_path + ) + + Map.merge(ctx, %{story: story, orch_key: orch_key}) + end + + defp sha(char), do: String.duplicate(char, 40) + + 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 verify(conn, ctx) do + conn + |> put_req_header("authorization", "Bearer #{ctx.orch_key}") + |> post(~p"/api/v1/stories/#{ctx.story.id}/verify", %{"summary" => "ok"}) + end + + defp forge(answer), + do: + expect(MockPullRequestSource, :contains?, fn _repo, merge_sha, "verified/master" -> + assert merge_sha == sha("c") + answer + end) + + test "202 when the landing is in verified/master", %{conn: conn} = ctx do + forge({:ok, true}) + assert conn |> verify(ctx) |> json_response(202) + assert AdminRepo.get!(Story, ctx.story.id).verified_status == :verified + end + + test "TC-47.6.1: 409 chg_landing_not_trunk_verified", %{conn: conn} = ctx do + forge({:ok, false}) + %{"error" => error} = conn |> verify(ctx) |> json_response(409) + + assert error["code"] == "chg_landing_not_trunk_verified" + assert error["message"] == Messages.chg_landing_not_trunk_verified() + assert AdminRepo.get!(Story, ctx.story.id).verified_status == :unverified + end + + test "TC-47.6.2: 409 chg_landing_reverted", %{conn: conn} = ctx do + :ok = push!(ctx, 3, "verified", %{sha: sha("a"), data: %{"sha" => sha("c")}}) + :ok = push!(ctx, 4, "reverted", %{sha: sha("a"), data: %{"landed_sha" => sha("c")}}) + expect(MockPullRequestSource, :contains?, 0, fn _, _, _ -> :unused end) + + %{"error" => error} = conn |> verify(ctx) |> json_response(409) + assert error["code"] == "chg_landing_reverted" + assert error["message"] == Messages.chg_landing_reverted() + end + + test "TC-47.6.3: a slow forge is 503 chg_trunk_unevaluated with Retry-After", + %{conn: conn} = ctx do + forge({:error, {:github_unreachable, :timeout}}) + conn = verify(conn, ctx) + + assert %{"error" => error} = json_response(conn, 503) + assert get_resp_header(conn, "retry-after") == ["30"] + assert error["code"] == "chg_trunk_unevaluated" + assert error["decision"] == "unevaluated" + assert error["reason"] == "forge_unavailable" + assert error["retry_after_seconds"] == 30 + assert error["message"] == Messages.chg_trunk_unevaluated() + assert AdminRepo.get!(Story, ctx.story.id).verified_status == :unverified + end + + test "a missing verified/master ref is 503 trunk_ref_not_found, with the forge's delay " <> + "when a rate limit names one", + %{conn: conn} = ctx do + forge({:error, {:github_api_error, 404}}) + + assert %{"error" => %{"reason" => "trunk_ref_not_found"}} = + conn |> verify(ctx) |> json_response(503) + + forge({:error, {:github_rate_limited, 403, 240}}) + conn = verify(build_conn(), ctx) + assert json_response(conn, 503)["error"]["reason"] == "forge_unavailable" + assert get_resp_header(conn, "retry-after") == ["240"] + end + + test "backfill forwards the 503 tuple rather than crashing on it", %{conn: conn} = ctx do + pending = + fixture(:story, %{tenant_id: ctx.tenant_id, epic_id: ctx.epic.id}) + |> Ecto.Changeset.change(%{agent_status: :reported_done}) + |> AdminRepo.update!() + + c2 = fn seq, kind, attrs -> + raw = + Jason.encode!( + build(:chg_event, Map.merge(%{change_id: "C2", 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)) + }) + end + + c2.(1, "submitted", %{sha: sha("b"), data: %{"story_id" => pending.id}}) + c2.(2, "landed", %{sha: sha("b"), data: %{"merge_sha" => sha("c")}}) + + fixture(:review_record, %{ + tenant_id: ctx.tenant_id, + story_id: pending.id, + commit_sha: sha("b"), + reviewer_dispatch_id: + fixture(:chg_root_dispatch, %{tenant_id: ctx.tenant_id, parent: ctx.root}).id + }) + + forge({:error, {:github_api_error, 502}}) + + conn = + conn + |> put_req_header("authorization", "Bearer #{ctx.orch_key}") + |> post(~p"/api/v1/stories/#{pending.id}/backfill", %{"reason" => "pre-existing"}) + + assert json_response(conn, 503)["error"]["code"] == "chg_trunk_unevaluated" + assert get_resp_header(conn, "retry-after") == ["30"] + end + + describe "the OpenAPI spec declares each new answer, with its message" do + @declared [ + {"/api/v1/stories/{id}/verify", 409, &Messages.chg_landing_not_trunk_verified/0}, + {"/api/v1/stories/{id}/verify", 409, &Messages.chg_landing_reverted/0}, + {"/api/v1/stories/{id}/verify", 503, &Messages.chg_trunk_unevaluated/0}, + {"/api/v1/stories/{id}/backfill", 409, &Messages.chg_landing_not_trunk_verified/0}, + {"/api/v1/stories/{id}/backfill", 409, &Messages.chg_landing_reverted/0}, + {"/api/v1/stories/{id}/backfill", 503, &Messages.chg_trunk_unevaluated/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 routes name the per-story trunk 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" + ], + code <- ~w(chg_landing_not_trunk_verified chg_landing_reverted chg_trunk_unevaluated) do + assert paths[path].post.description =~ code, "#{path} does not name #{code}" + end + end + end + + test "tenant isolation: another tenant's key cannot reach this story", %{conn: conn} = ctx do + other = fixture(:tenant) + + {:ok, %{raw_key: other_key}} = + Dispatches.create_dispatch(other.id, %{ + role: :orchestrator, + agent_id: fixture(:agent, %{tenant_id: other.id, agent_type: :orchestrator}).id + }) + + expect(MockPullRequestSource, :contains?, 0, fn _, _, _ -> :unused end) + + conn + |> put_req_header("authorization", "Bearer #{other_key}") + |> post(~p"/api/v1/stories/#{ctx.story.id}/verify", %{"summary" => "ok"}) + |> json_response(404) + + assert AdminRepo.get!(Story, ctx.story.id).verified_status == :unverified + end +end From 7fd5e3f2691cf7a7717ada5d0b7e3462a17d90d3 Mon Sep 17 00:00:00 2001 From: Mark Kreyman Date: Mon, 19 Oct 2026 04:51:22 -0600 Subject: [PATCH 2/3] Record chg_landed_on_trunk for a chg story's CI run (US-47.6) The run queued after verify read CI like a pull request and recorded no_story_branch for a chg story. A chg source now records why CI is not read: verify required the landing on verified/master. --- lib/loopctl/api_spec/schemas.ex | 2 +- lib/loopctl/verification/ci_target.ex | 8 ++++- .../workers/verification_runner_worker.ex | 3 +- ...ication_runner_worker_integration_test.exs | 33 +++++++++++++++++++ 4 files changed, 43 insertions(+), 3 deletions(-) diff --git a/lib/loopctl/api_spec/schemas.ex b/lib/loopctl/api_spec/schemas.ex index ecba5b36..4e935dd4 100644 --- a/lib/loopctl/api_spec/schemas.ex +++ b/lib/loopctl/api_spec/schemas.ex @@ -3660,7 +3660,7 @@ defmodule Loopctl.ApiSpec.Schemas do "repository, and `fail` adds `failed_check` and `conclusion`. With no verdict " <> "(`error`), `ci_unavailable_reason` is a short code with no URL and no " <> "repository name (e.g. `credential_unavailable`, `no_intake_source`, " <> - "`no_required_checks`, `no_story_branch`, `empty_change`, " <> + "`no_required_checks`, `no_story_branch`, `chg_landed_on_trunk`, `empty_change`, " <> "`ci_definition_changed`, `ci_wait_exhausted`, `forge_unavailable`, " <> "`database_busy`, " <> "and for an abbreviated `commit_sha` `unresolved_sha` or " <> diff --git a/lib/loopctl/verification/ci_target.ex b/lib/loopctl/verification/ci_target.ex index fac215d3..a79f7600 100644 --- a/lib/loopctl/verification/ci_target.ex +++ b/lib/loopctl/verification/ci_target.ex @@ -6,7 +6,10 @@ defmodule Loopctl.Verification.CiTarget do - the REPOSITORY is the story's project's intake source (`Intake.source_for_project/2`), the one derivation the merge gate and the dispatcher use. No live source is - `no_intake_source`, more than one `ambiguous_intake_source` + `no_intake_source`, more than one `ambiguous_intake_source`. A `mode: :chg` source is + `chg_landed_on_trunk`: chg lands the change, not a pull request, so no story branch + carries CI to read, and verify already required the landing on verified/master + (US-47.6, `Loopctl.Changes.TrunkEvidence`) - the REQUIRED CHECKS are that source's `required_checks`. None (after dropping `local-gate`, which is never trusted) is `no_required_checks`: the opt-in - the BRANCH is the one the story's placement recorded: the branch the claim's dispatch put @@ -82,6 +85,9 @@ defmodule Loopctl.Verification.CiTarget do defp source(tenant_id, story) do case Intake.source_for_project(tenant_id, story.project_id) do + {:ok, %{mode: :chg}} -> + {:unconfigured, "chg_landed_on_trunk"} + {:ok, source} -> {:ok, source} diff --git a/lib/loopctl/workers/verification_runner_worker.ex b/lib/loopctl/workers/verification_runner_worker.ex index e0b829be..ff5e9883 100644 --- a/lib/loopctl/workers/verification_runner_worker.ex +++ b/lib/loopctl/workers/verification_runner_worker.ex @@ -16,7 +16,8 @@ defmodule Loopctl.Workers.VerificationRunnerWorker do #936, or an allowlist entry; `credential_unavailable`), before any read of the story, so a tenant with neither costs one indexed existence check and none of the story reads; the story's repository, required checks and branch (`Loopctl.Verification.CiTarget`; - `no_intake_source`, `ambiguous_intake_source`, `no_required_checks`, `no_story_branch`); + `no_intake_source`, `ambiguous_intake_source`, `chg_landed_on_trunk`, `no_required_checks`, + `no_story_branch`); and the credential for that tenant AND that repository (`Credential.for_read/2`; `credential_unavailable`), before any forge read. So a tenant with no token and no allowlist entry records `credential_unavailable` even when its source names no required diff --git a/test/loopctl/workers/verification_runner_worker_integration_test.exs b/test/loopctl/workers/verification_runner_worker_integration_test.exs index ddf40529..13973777 100644 --- a/test/loopctl/workers/verification_runner_worker_integration_test.exs +++ b/test/loopctl/workers/verification_runner_worker_integration_test.exs @@ -147,6 +147,39 @@ defmodule Loopctl.Workers.VerificationRunnerWorkerIntegrationTest do refute_received {:evidence, _} end + # US-47.6: chg lands the change; verify read verified/master, so there is no CI to read. + test "a chg source records chg_landed_on_trunk, no forge read", ctx do + root = fixture(:chg_root_dispatch, %{tenant_id: ctx.tenant_id}) + + [source] = + Loopctl.AdminRepo.all( + from(s in Loopctl.Intake.Source, where: s.tenant_id == ^ctx.tenant_id) + ) + + {:ok, %{mode: :chg}} = + Loopctl.Intake.update_source(ctx.tenant_id, source.id, %{ + mode: "chg", + change_events_secret: String.duplicate("s", 32), + chg_root_dispatch_id: root.id + }) + + stub_forge(ctx, %{evidence: build(:forge_evidence)}) + + run = fixture(:verification_run, ctx) + assert :ok = perform(ctx, run) + + reloaded = reload(ctx, run) + assert reloaded.status == "error" + + assert reloaded.ac_results == %{ + "source" => "ci", + "ci_unavailable_reason" => "chg_landed_on_trunk" + } + + refute_received {:compare, _} + refute_received {:evidence, _} + end + # #931 finding g: the rescue arm records a code, never the exception. test "a crash records internal_error with no exception text, and cancels", ctx do stub(MockVerificationCredential, :for_read, fn _tenant_id, _repo -> From b09c43709c3ec5c6ce26c468bb4e7a37929c681f Mon Sep 17 00:00:00 2001 From: Mark Kreyman Date: Mon, 19 Oct 2026 05:08:03 -0600 Subject: [PATCH 3/3] Review round on #1007: one landing read, local gates before the forge - Loopctl.Changes.Landings holds the newest-binding and newest-applied- landing reads; ReviewCustody and TrunkEvidence both use them, so the landed-review and trunk gates judge the same landing. - TrunkEvidence reads bindings, landings and reverts in one query each for any batch, and the dead :not_chg branches are gone. - assess/3 and assess_many/3 take an ask? callback: verify, verify-all, backfill and the bulk paths ask the forge only for a story their cheap local gates would pass. verify-all makes its forge reads in one batch. - CiTarget records chg_landed_on_trunk for a story with a change binding, not for every story of a chg-mode source. - The forge stub's transaction probe now reads the calling process's transaction state too, so the bulk paths' task workers are covered. - Doc arities corrected (assess/3, assess_many/3, ensure_mark_complete_allowed/3). --- .claude/skills/chain-of-custody/SKILL.md | 4 +- CLAUDE.md | 8 +- lib/loopctl/bulk_operations.ex | 15 +- lib/loopctl/changes/landings.ex | 93 +++++++++ lib/loopctl/changes/review_custody.ex | 25 +-- lib/loopctl/changes/trunk_evidence.ex | 195 +++++++++--------- lib/loopctl/progress.ex | 69 +++++-- lib/loopctl/verification/ci_target.ex | 22 +- test/loopctl/changes/trunk_evidence_test.exs | 65 +++++- ...ication_runner_worker_integration_test.exs | 37 ++-- 10 files changed, 369 insertions(+), 164 deletions(-) create mode 100644 lib/loopctl/changes/landings.ex diff --git a/.claude/skills/chain-of-custody/SKILL.md b/.claude/skills/chain-of-custody/SKILL.md index 5c77552e..0c2036ee 100644 --- a/.claude/skills/chain-of-custody/SKILL.md +++ b/.claude/skills/chain-of-custody/SKILL.md @@ -149,7 +149,7 @@ Neither refusal records a custody violation. on the same five paths, the landing's TRUNK commit (`data.merge_sha`, never `event.sha`) must be in `verified/master` (`PullRequestSource.contains?/3`; `chg_landing_not_trunk_verified`) and no applied `reverted` may follow the landing (`chg_landing_reverted`). The forge is read BEFORE the -transaction (`assess/2`, `assess_many/2`) and applied under the lock only to the landing it was +transaction (`assess/3`, `assess_many/3`) and applied under the lock only to the landing it was made for (`ensure_trunk_verified/4`); any forge failure, a 404 included, is `{:chg_trunk_unevaluated, reason, retry_after}` — a 503 with `Retry-After`, never a pass. Do not move the read into the Multi (a lock held across a network call), and do not short-circuit on the @@ -161,7 +161,7 @@ is exactly the orphaned shape — do not delete `custody_orphaned?/1` or `custod redundant with the constraint. **Backfill is the fourth way to reach `verified`, and its guard is `stories.lifecycle_entered_at`.** -`backfill_story/4` / `ensure_mark_complete_allowed/2` certify with no report, no review record and no +`backfill_story/4` / `ensure_mark_complete_allowed/3` certify with no report, no review record and no verifier, so they are confined to work that never entered the lifecycle. State alone cannot answer that: `force_unclaim_story/3` CLEARS `assigned_agent_id`, and a legacy bearer claim never writes `implementer_dispatch_id`, so claim -> force-unclaim leaves a worked story indistinguishable from diff --git a/CLAUDE.md b/CLAUDE.md index 94c3992c..fefd6dac 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -262,10 +262,10 @@ landed event's own transaction over the control-only edge `:review_missing` 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:2713`), bulk verify -through `process_verify/3` (`bulk_operations.ex:426`), backfill and bulk mark-complete -through `guard_backfill/3` (`progress.ex:2824`) — calls one function, -`ReviewCustody.ensure_landed_review/3` (`lib/loopctl/changes/review_custody.ex:215`). It refuses a chg story with +verify and verify-all through `chg_landed_review/2` (`progress.ex:2736`), bulk verify +through `process_verify/3` (`bulk_operations.ex:433`), backfill and bulk mark-complete +through `guard_backfill/3` (`progress.ex:2847`) — calls one function, +`ReviewCustody.ensure_landed_review/3` (`lib/loopctl/changes/review_custody.ex:208`). 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 diff --git a/lib/loopctl/bulk_operations.ex b/lib/loopctl/bulk_operations.ex index 20e7f0dc..910d148c 100644 --- a/lib/loopctl/bulk_operations.ex +++ b/lib/loopctl/bulk_operations.ex @@ -131,8 +131,9 @@ defmodule Loopctl.BulkOperations do actor_id: actor_id, actor_label: actor_label, caller_lineage: caller_lineage(tenant_id, opts), - # US-47.6: the chg stories' forge reads, made BEFORE the locking transaction. - trunk: TrunkEvidence.assess_many(tenant_id, sorted_ids) + # US-47.6: the chg stories' forge reads, made BEFORE the locking transaction, and + # only for a story the landed-review gate would let through. + trunk: TrunkEvidence.assess_many(tenant_id, sorted_ids, &landed_review_ok?(tenant_id, &1)) } AdminRepo.transaction(fn -> @@ -255,8 +256,9 @@ defmodule Loopctl.BulkOperations do orch_id: orchestrator_agent_id, actor_id: actor_id, actor_label: actor_label, - # US-47.6: the chg stories' forge reads, made BEFORE the locking transaction. - trunk: TrunkEvidence.assess_many(tenant_id, sorted_ids) + # US-47.6: the chg stories' forge reads, made BEFORE the locking transaction, and + # only for a story the landed-review gate would let through. + trunk: TrunkEvidence.assess_many(tenant_id, sorted_ids, &landed_review_ok?(tenant_id, &1)) } AdminRepo.transaction(fn -> @@ -423,6 +425,11 @@ defmodule Loopctl.BulkOperations do end end + # A story the landed-review gate refuses is refused again under the lock, so asking the + # forge about it would spend a credentialed call on an answer nobody reads. + defp landed_review_ok?(tenant_id, story_id), + do: ReviewCustody.ensure_landed_review(tenant_id, story_id) == :ok + defp process_verify(story, params, ctx) do %{ tenant_id: tenant_id, diff --git a/lib/loopctl/changes/landings.ex b/lib/loopctl/changes/landings.ex new file mode 100644 index 00000000..1d7e5c71 --- /dev/null +++ b/lib/loopctl/changes/landings.ex @@ -0,0 +1,93 @@ +defmodule Loopctl.Changes.Landings do + @moduledoc """ + The two reads every chg custody gate must agree on (Epic 47): which of a story's change + bindings is its NEWEST, and which `landed` event of that binding's change is its newest + APPLIED one. `Loopctl.Changes.ReviewCustody` (US-47.5, the landed-review gate) and + `Loopctl.Changes.TrunkEvidence` (US-47.6, the trunk gate) both read them here, so the two + gates can never judge different landings of the same story. + + The newest binding is the current one when it is unsuperseded; a story whose newest binding + is superseded has no current binding. Both reads are set-based, so a batch costs one query + each, and tenant-scoped on `repo` (default `AdminRepo`), the custody-read convention. + """ + + import Ecto.Query + + alias Loopctl.AdminRepo + alias Loopctl.Changes.Binding + alias Loopctl.Changes.Event + alias Loopctl.Intake.Source + + @type landing :: %{ + id: Ecto.UUID.t(), + sha: String.t() | nil, + event_seq: integer(), + data: map(), + source_repo: String.t() | nil + } + + @doc "Each story's newest binding, keyed by story id; a story with none is absent." + @spec newest_bindings(Ecto.Repo.t(), Ecto.UUID.t(), [Ecto.UUID.t()]) :: + %{optional(Ecto.UUID.t()) => Binding.t()} + def newest_bindings(repo \\ AdminRepo, tenant_id, story_ids) do + repo.all( + from b in Binding, + where: b.tenant_id == ^tenant_id and b.story_id in ^story_ids, + distinct: [asc: b.story_id], + order_by: [desc: is_nil(b.superseded_at), desc: b.inserted_at, desc: b.id] + ) + |> Map.new(&{&1.story_id, &1}) + end + + @doc "`newest_bindings/3` for one story: the binding, or nil." + @spec newest_binding(Ecto.Repo.t(), Ecto.UUID.t(), Ecto.UUID.t()) :: Binding.t() | nil + def newest_binding(repo \\ AdminRepo, tenant_id, story_id), + do: Map.get(newest_bindings(repo, tenant_id, [story_id]), story_id) + + @doc """ + The newest APPLIED `landed` event of each binding's change, keyed by `{repo, change_id}`, + with the `repo_full_name` of the intake source that delivered it (`source_repo`). + """ + @spec newest_applied_landings(Ecto.Repo.t(), Ecto.UUID.t(), [Binding.t()]) :: + %{optional({String.t(), String.t()}) => landing()} + def newest_applied_landings(repo \\ AdminRepo, tenant_id, bindings) + + def newest_applied_landings(_repo, _tenant_id, []), do: %{} + + def newest_applied_landings(repo, tenant_id, bindings) do + keys = MapSet.new(bindings, &{&1.repo, &1.change_id}) + + repo.all( + from e in Event, + left_join: s in Source, + on: s.id == e.source_id and s.tenant_id == e.tenant_id, + where: + e.tenant_id == ^tenant_id and e.repo in ^Enum.map(bindings, & &1.repo) and + e.change_id in ^Enum.map(bindings, & &1.change_id) and e.kind == "landed" and + not is_nil(e.applied_at), + distinct: [asc: e.repo, asc: e.change_id], + order_by: [desc: e.event_seq], + select: %{ + key: {e.repo, e.change_id}, + landing: %{ + id: e.id, + sha: e.sha, + event_seq: e.event_seq, + data: e.data, + source_repo: s.repo_full_name + } + } + ) + |> Enum.filter(&MapSet.member?(keys, &1.key)) + |> Map.new(&{&1.key, &1.landing}) + end + + @doc "`newest_applied_landings/3` for one binding: the landing, or nil." + @spec newest_applied_landing(Ecto.Repo.t(), Ecto.UUID.t(), Binding.t()) :: landing() | nil + def newest_applied_landing(repo \\ AdminRepo, tenant_id, %Binding{} = binding), + do: + Map.get( + newest_applied_landings(repo, tenant_id, [binding]), + {binding.repo, binding.change_id} + ) +end diff --git a/lib/loopctl/changes/review_custody.ex b/lib/loopctl/changes/review_custody.ex index 50dccb18..b01b97ca 100644 --- a/lib/loopctl/changes/review_custody.ex +++ b/lib/loopctl/changes/review_custody.ex @@ -72,6 +72,7 @@ defmodule Loopctl.Changes.ReviewCustody do alias Loopctl.Artifacts.ReviewRecord alias Loopctl.Changes.Binding alias Loopctl.Changes.Event + alias Loopctl.Changes.Landings alias Loopctl.Delivery.StageEvent alias Loopctl.Delivery.StoryStage alias Loopctl.GitSha @@ -93,15 +94,7 @@ defmodule Loopctl.Changes.ReviewCustody do """ @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 + case Landings.newest_binding(repo, tenant_id, story_id) do nil -> :not_chg @@ -233,16 +226,10 @@ defmodule Loopctl.Changes.ReviewCustody do # 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 - ) + case Landings.newest_applied_landing(repo, tenant_id, binding) do + %{sha: sha} -> sha + nil -> nil + end end defp landed_revision(_repo, _tenant_id, _chg), do: nil diff --git a/lib/loopctl/changes/trunk_evidence.ex b/lib/loopctl/changes/trunk_evidence.ex index 4e0801b3..cae97473 100644 --- a/lib/loopctl/changes/trunk_evidence.ex +++ b/lib/loopctl/changes/trunk_evidence.ex @@ -31,7 +31,10 @@ defmodule Loopctl.Changes.TrunkEvidence do `ensure_trunk_verified/4` runs INSIDE the transaction, under the story lock, after `ReviewCustody.ensure_landed_review/3`. It re-reads the landing and the revert from the database and applies the assessment only when it was made for the SAME landing; a landing - that arrived in between answers unevaluated (`landing_changed`), never the stale answer. + that arrived in between answers unevaluated (`landing_changed`), never the stale answer, as + does a story with no assessment at all (its local gates refused before the read and pass + now). The bindings and landings are read through `Loopctl.Changes.Landings`, the reads the + landed-review gate makes, so the two gates always judge the same landing. ## What each answer means to the caller @@ -62,10 +65,10 @@ defmodule Loopctl.Changes.TrunkEvidence do alias Loopctl.AdminRepo alias Loopctl.Changes.Binding alias Loopctl.Changes.Event + alias Loopctl.Changes.Landings alias Loopctl.Delivery.MergePrecondition alias Loopctl.Delivery.PullRequestSource alias Loopctl.GitSha - alias Loopctl.Intake.Source alias Loopctl.Verification.Credential @verified_ref "verified/master" @@ -109,71 +112,82 @@ defmodule Loopctl.Changes.TrunkEvidence do @doc """ The assessment for one story, made OUTSIDE any transaction. Asks the forge only for a chg - story whose current landing is not already reverted and names a valid trunk commit. + story whose current landing is not already reverted and names a valid trunk commit, and + only when `ask?` (called with no arguments, just before the forge read) answers true: the + caller passes its own cheap local gates there, so a verify they would refuse spends no + credentialed forge call. A declined ask is no assessment (`nil`), which the in-transaction + half answers as unevaluated `landing_changed` should those gates have passed since. """ - @spec assess(Ecto.UUID.t(), Ecto.UUID.t()) :: assessment() - def assess(tenant_id, story_id) do - case facts(AdminRepo, tenant_id, story_id) do - :not_chg -> :not_chg - {:chg, nil} -> %{landed_event_id: nil, outcome: nil} - {:chg, landing} -> %{landed_event_id: landing.id, outcome: judge(tenant_id, landing)} - end + @spec assess(Ecto.UUID.t(), Ecto.UUID.t(), (-> boolean())) :: assessment() | nil + def assess(tenant_id, story_id, ask? \\ fn -> true end) do + tenant_id + |> assess_many([story_id], fn _story_id -> ask?.() end) + |> Map.get(story_id, :not_chg) end @doc """ - `assess/2` for a batch, as a map keyed by story id. One query finds the bound stories; only - they are assessed, the forge calls `@batch_concurrency` at a time. A call that outlives its - task bound is `forge_unavailable` for that story. + `assess/3` for a batch, as a map keyed by story id; a story with no change binding is + absent (`:not_chg`). The bindings, landings and reverts are read in one query each; `ask?` + (called with the story id) is asked only for the stories that would make a forge call, and + those calls run `@batch_concurrency` at a time. A call that outlives its task bound is + `forge_unavailable` for that story. """ - @spec assess_many(Ecto.UUID.t(), [Ecto.UUID.t()]) :: %{optional(Ecto.UUID.t()) => assessment()} - def assess_many(tenant_id, story_ids) when is_list(story_ids) do + @spec assess_many(Ecto.UUID.t(), [Ecto.UUID.t()], (Ecto.UUID.t() -> boolean())) :: + %{optional(Ecto.UUID.t()) => assessment() | nil} + def assess_many(tenant_id, story_ids, ask? \\ fn _story_id -> true end) + when is_list(story_ids) do ids = Enum.filter(story_ids, &match?({:ok, _}, Ecto.UUID.cast(&1))) + facts = facts_many(AdminRepo, tenant_id, ids) - bound = - AdminRepo.all( - from b in Binding, - where: b.tenant_id == ^tenant_id and b.story_id in ^ids, - distinct: true, - select: b.story_id - ) + {to_forge, decided} = + Enum.split_with(facts, fn {story_id, landing} -> + needs_forge?(landing) and ask?.(story_id) + end) - landings = - Enum.map(bound, fn story_id -> {story_id, facts(AdminRepo, tenant_id, story_id)} end) + decided = + Map.new(decided, fn + {story_id, nil} -> {story_id, %{landed_event_id: nil, outcome: nil}} + {story_id, landing} -> {story_id, local_assessment(landing)} + end) - landings + to_forge |> Task.async_stream( - fn - {story_id, {:chg, nil}} -> - {story_id, %{landed_event_id: nil, outcome: nil}} - - {story_id, {:chg, landing}} -> - {story_id, %{landed_event_id: landing.id, outcome: judge(tenant_id, landing)}} - - {story_id, :not_chg} -> - {story_id, :not_chg} + fn {story_id, landing} -> + {story_id, %{landed_event_id: landing.id, outcome: judge(tenant_id, landing)}} end, max_concurrency: @batch_concurrency, timeout: @batch_task_timeout_ms, on_timeout: :kill_task, ordered: true ) - |> Enum.zip(landings) + |> Enum.zip(to_forge) |> Map.new(fn {{:ok, {story_id, assessment}}, _landing} -> {story_id, assessment} - {{:exit, _reason}, {story_id, {:chg, landing}}} -> + {{:exit, _reason}, {story_id, landing}} -> {story_id, %{ - landed_event_id: landing && landing.id, + landed_event_id: landing.id, outcome: {:unevaluated, "forge_unavailable", @default_retry_after_seconds} }} - - {{:exit, _reason}, {story_id, :not_chg}} -> - {story_id, :not_chg} end) + |> Map.merge(decided) end + # A landing the forge must be asked about: not reverted, and naming a valid trunk commit. + defp needs_forge?(%{reverted?: false, merge_sha: sha}) when is_binary(sha), do: true + defp needs_forge?(_landing), do: false + + # A landing decided without the forge, or one whose forge read the caller declined (`nil`). + defp local_assessment(%{reverted?: true} = landing), + do: %{landed_event_id: landing.id, outcome: :reverted} + + defp local_assessment(%{merge_sha: nil} = landing), + do: %{landed_event_id: landing.id, outcome: :no_trunk_commit} + + defp local_assessment(_declined), do: nil + @doc """ The in-transaction half: `:ok` or a refusal (see the moduledoc), from the landing as the database holds it NOW and the `assessment` made before the transaction. A chg story with no @@ -183,17 +197,17 @@ defmodule Loopctl.Changes.TrunkEvidence do @spec ensure_trunk_verified(Ecto.Repo.t(), Ecto.UUID.t(), Ecto.UUID.t(), assessment() | nil) :: :ok | {:error, refusal()} def ensure_trunk_verified(repo \\ AdminRepo, tenant_id, story_id, assessment) do - case facts(repo, tenant_id, story_id) do - :not_chg -> + case Map.fetch(facts_many(repo, tenant_id, [story_id]), story_id) do + :error -> :ok - {:chg, nil} -> + {:ok, nil} -> {:error, :chg_landing_not_trunk_verified} - {:chg, %{reverted?: true}} -> + {:ok, %{reverted?: true}} -> refuse(tenant_id, story_id, :chg_landing_reverted) - {:chg, %{id: landed_id}} -> + {:ok, %{id: landed_id}} -> case assessment do %{landed_event_id: ^landed_id, outcome: outcome} -> decide(tenant_id, story_id, outcome) @@ -206,65 +220,51 @@ defmodule Loopctl.Changes.TrunkEvidence do # -- the facts ----------------------------------------------------------------------------- - # `:not_chg` when the story has no binding at all; `{:chg, nil}` when its newest binding is - # superseded or its change has no applied landing; else the newest applied landing. - defp facts(repo, 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{superseded_at: nil} = binding -> {:chg, landing(repo, tenant_id, binding)} - %Binding{} -> {:chg, nil} - end + # Keyed by story id, for the stories that have a binding: `nil` when the newest binding is + # superseded or its change has no applied landing, else that landing. Three queries for any + # batch: the bindings and the landings through `Loopctl.Changes.Landings` — the reads the + # landed-review gate makes, so the two gates judge the same landing — and the reverts. + defp facts_many(repo, tenant_id, story_ids) do + bindings = Landings.newest_bindings(repo, tenant_id, story_ids) + current = for {_story_id, %Binding{superseded_at: nil} = b} <- bindings, do: b + landings = Landings.newest_applied_landings(repo, tenant_id, current) + reverts = newest_reverts(repo, tenant_id, current) + + Map.new(bindings, fn + {story_id, %Binding{superseded_at: nil} = binding} -> + key = {binding.repo, binding.change_id} + {story_id, landing(Map.get(landings, key), Map.get(reverts, key))} + + {story_id, %Binding{}} -> + {story_id, nil} + end) end - defp landing(repo, tenant_id, %Binding{} = binding) do - case newest_landing(repo, tenant_id, binding) do - nil -> - nil - - %{} = landed -> - %{ - id: landed.id, - repo: landed.repo, - merge_sha: merge_sha(landed.data), - reverted?: reverted_after?(repo, tenant_id, binding, landed.event_seq) - } - end - end + defp landing(nil, _revert_seq), do: nil - # The newest APPLIED `landed` event of the binding's change, with the repository of the - # source that delivered it — the intake source's own `repo_full_name`, which the credential - # is asked for, as every other forge read asks. - defp newest_landing(repo, tenant_id, binding) do - repo.one( - from e in Event, - join: s in Source, - on: s.id == e.source_id and s.tenant_id == e.tenant_id, - 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: %{id: e.id, event_seq: e.event_seq, data: e.data, repo: s.repo_full_name} - ) + defp landing(landed, revert_seq) do + %{ + id: landed.id, + repo: landed.source_repo, + merge_sha: merge_sha(landed.data), + reverted?: is_integer(revert_seq) and revert_seq > landed.event_seq + } end - defp reverted_after?(repo, tenant_id, binding, landed_seq) do - repo.exists?( + # The highest `event_seq` of an APPLIED `reverted` event of each binding's change. + defp newest_reverts(_repo, _tenant_id, []), do: %{} + + defp newest_reverts(repo, tenant_id, bindings) do + repo.all( 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 == "reverted" and - not is_nil(e.applied_at) and e.event_seq > ^landed_seq + e.tenant_id == ^tenant_id and e.repo in ^Enum.map(bindings, & &1.repo) and + e.change_id in ^Enum.map(bindings, & &1.change_id) and e.kind == "reverted" and + not is_nil(e.applied_at), + group_by: [e.repo, e.change_id], + select: {{e.repo, e.change_id}, max(e.event_seq)} ) + |> Map.new() end defp merge_sha(%{"merge_sha" => sha}) when is_binary(sha) do @@ -275,9 +275,6 @@ defmodule Loopctl.Changes.TrunkEvidence do # -- the forge ----------------------------------------------------------------------------- - defp judge(_tenant_id, %{reverted?: true}), do: :reverted - defp judge(_tenant_id, %{merge_sha: nil}), do: :no_trunk_commit - defp judge(tenant_id, %{repo: repo, merge_sha: sha}) do with {:ok, forge_repo} <- Credential.repo(tenant_id, repo), {:ok, contained?} <- PullRequestSource.contains?(forge_repo, sha, @verified_ref) do diff --git a/lib/loopctl/progress.ex b/lib/loopctl/progress.ex index 4aae82bb..2a78a0c3 100644 --- a/lib/loopctl/progress.ex +++ b/lib/loopctl/progress.ex @@ -2608,10 +2608,35 @@ defmodule Loopctl.Progress 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) + # US-47.6: a chg story's CI evidence is read from the forge HERE, before the + # transaction, so no story lock is held across a network read, and only when the + # story's cheap local gates would let it through. It is applied under the lock. + trunk = + TrunkEvidence.assess(tenant_id, story_id, fn -> + verify_worth_asking?(tenant_id, story_id) + end) + + do_verify_story(tenant_id, story_id, opts, verification_params, trunk) + end + end + + # The read-only gates `do_verify_story/5` runs before the trunk gate, on an unlocked read: + # a story they refuse is refused there again, under the lock, and never costs a forge call. + # The self-verify gate is not run here: its refusal escalates, which belongs to the + # transaction that answers the caller. + defp verify_worth_asking?(tenant_id, story_id) do + case AdminRepo.one(from s in Story, where: s.tenant_id == ^tenant_id and s.id == ^story_id) do + nil -> false + story -> verify_local_gates_pass?(tenant_id, story) end end + defp verify_local_gates_pass?(tenant_id, story) do + match?({:ok, _}, validate_verifiable(story)) and + match?({:ok, _}, validate_review_record_exists(tenant_id, story.id, story)) and + match?({:ok, _}, chg_landed_review(tenant_id, story.id)) + 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 @@ -2623,7 +2648,9 @@ defmodule Loopctl.Progress do ReviewCustody.caller_lineage(opts, [:verifier_lineage]) ) - defp do_verify_story(tenant_id, story_id, opts, verification_params) do + # `trunk` is the story's `TrunkEvidence` assessment, made by the caller before the + # transaction. + defp do_verify_story(tenant_id, story_id, opts, verification_params, trunk) do orchestrator_agent_id = Keyword.get(opts, :orchestrator_agent_id) actor_id = Keyword.get(opts, :actor_id) actor_label = Keyword.get(opts, :actor_label) @@ -2646,10 +2673,6 @@ defmodule Loopctl.Progress do # story's recorded verifier; the caller comparison binds the principal that # actually makes the call, on EVERY path including the one that skips the # optional request-review. - # US-47.6: a chg story's CI evidence is read from the forge HERE, before the transaction, - # so no story lock is held across a network read. It is applied under the lock below. - trunk = TrunkEvidence.assess(tenant_id, story_id) - multi = Multi.new() |> Multi.run(:lock, fn _repo, _changes -> lock_story(tenant_id, story_id) end) @@ -2795,7 +2818,7 @@ defmodule Loopctl.Progress do `stories_with_lifecycle_history/2`), so a 50-story batch costs one audit query rather than 50 inside the locking transaction. - `trunk` is the batch's `Loopctl.Changes.TrunkEvidence.assess_many/2` map, made BEFORE the + `trunk` is the batch's `Loopctl.Changes.TrunkEvidence.assess_many/3` map, made BEFORE the transaction (US-47.6). A chg story missing from it is unevaluated (`landing_changed`), never passed, so the default only serves a caller whose batch holds no chg story. """ @@ -2951,8 +2974,12 @@ defmodule Loopctl.Progress do actor_id = Keyword.get(opts, :actor_id) actor_label = Keyword.get(opts, :actor_label) - # US-47.6: the forge read happens before the transaction, as on verify. - trunk = TrunkEvidence.assess(tenant_id, story_id) + # US-47.6: the forge read happens before the transaction, as on verify, and only for a + # story the landed-review gate would let through. + trunk = + TrunkEvidence.assess(tenant_id, story_id, fn -> + match?({:ok, _}, chg_landed_review(tenant_id, story_id)) + end) multi = Multi.new() @@ -3178,7 +3205,7 @@ defmodule Loopctl.Progress do @doc """ The subset of `ids` whose audit log shows lifecycle history — the set - `backfill_story/4` and `ensure_mark_complete_allowed/2` refuse. + `backfill_story/4` and `ensure_mark_complete_allowed/3` refuse. Hoisted out of the per-story guard so the bulk mark-complete path pays ONE query per batch instead of one per story while holding `FOR UPDATE` locks on all of them @@ -3436,7 +3463,17 @@ defmodule Loopctl.Progress do 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)) + by_id = Map.new(stories_to_verify, &{&1.id, &1}) + + # US-47.6: the epic's forge reads are made once, before any story's transaction, a batch + # at a time (`TrunkEvidence.assess_many/3`), for the stories whose local gates pass. + trunk = + TrunkEvidence.assess_many(tenant_id, Map.keys(by_id), fn story_id -> + verify_local_gates_pass?(tenant_id, Map.fetch!(by_id, story_id)) + end) + + results = + Enum.map(stories_to_verify, &verify_single(tenant_id, &1, params, opts, trunk)) verified = Enum.count(results, &match?({:ok, _}, &1)) errors = results |> Enum.filter(&match?({:error, _}, &1)) |> Enum.map(&elem(&1, 1)) @@ -3465,8 +3502,14 @@ defmodule Loopctl.Progress do # 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 do_verify_story(tenant_id, story.id, opts, extract_verification_params(params)) do + defp verify_single(tenant_id, story, params, opts, trunk) do + case do_verify_story( + tenant_id, + story.id, + opts, + extract_verification_params(params), + Map.get(trunk, story.id, :not_chg) + ) do {:ok, _updated} -> {:ok, story.id} {:error, reason} -> {:error, %{story_id: story.id, reason: inspect(reason)}} end diff --git a/lib/loopctl/verification/ci_target.ex b/lib/loopctl/verification/ci_target.ex index a79f7600..2a62a3e1 100644 --- a/lib/loopctl/verification/ci_target.ex +++ b/lib/loopctl/verification/ci_target.ex @@ -6,10 +6,12 @@ defmodule Loopctl.Verification.CiTarget do - the REPOSITORY is the story's project's intake source (`Intake.source_for_project/2`), the one derivation the merge gate and the dispatcher use. No live source is - `no_intake_source`, more than one `ambiguous_intake_source`. A `mode: :chg` source is - `chg_landed_on_trunk`: chg lands the change, not a pull request, so no story branch - carries CI to read, and verify already required the landing on verified/master - (US-47.6, `Loopctl.Changes.TrunkEvidence`) + `no_intake_source`, more than one `ambiguous_intake_source` + - a story bound to a chg change is `chg_landed_on_trunk`, before any of the above: chg lands + it, not a pull request, so no story branch carries CI to read, and every verify path + already required its landing on verified/master (US-47.6, `Loopctl.Changes.TrunkEvidence`, + which keys on the same binding — never on the source's mode, which an unbound story + delivered by pull request shares after its project switches to chg) - the REQUIRED CHECKS are that source's `required_checks`. None (after dropping `local-gate`, which is never trusted) is `no_required_checks`: the opt-in - the BRANCH is the one the story's placement recorded: the branch the claim's dispatch put @@ -26,6 +28,7 @@ defmodule Loopctl.Verification.CiTarget do forge, so it neither counts toward nor ends the worker's forge-fault streak. """ + alias Loopctl.Changes.Landings alias Loopctl.Delivery.CiEvidence alias Loopctl.Delivery.DispatchPayload alias Loopctl.Delivery.Stages @@ -47,6 +50,7 @@ defmodule Loopctl.Verification.CiTarget do {:ok, t()} | {:unconfigured, String.t()} | {:wait, :database_busy} def gather(tenant_id, story_id, opts) do with {:story, {:ok, story}} <- {:story, Stories.get_story(tenant_id, story_id)}, + :ok <- not_chg_bound(tenant_id, story), {:ok, source} <- source(tenant_id, story), {:ok, required} <- required_checks(source), {:route, {:ok, route}} <- @@ -83,11 +87,15 @@ defmodule Loopctl.Verification.CiTarget do end end + defp not_chg_bound(tenant_id, story) do + case Landings.newest_binding(tenant_id, story.id) do + nil -> :ok + _binding -> {:unconfigured, "chg_landed_on_trunk"} + end + end + defp source(tenant_id, story) do case Intake.source_for_project(tenant_id, story.project_id) do - {:ok, %{mode: :chg}} -> - {:unconfigured, "chg_landed_on_trunk"} - {:ok, source} -> {:ok, source} diff --git a/test/loopctl/changes/trunk_evidence_test.exs b/test/loopctl/changes/trunk_evidence_test.exs index 42850edb..a82137f1 100644 --- a/test/loopctl/changes/trunk_evidence_test.exs +++ b/test/loopctl/changes/trunk_evidence_test.exs @@ -152,16 +152,36 @@ defmodule Loopctl.Changes.TrunkEvidenceTest do # The forge's answer for `merge_sha` against `verified/master`, asserting the call names the # TRUNK commit and the ref — never the revision commit — and runs outside every transaction. + # On a batch path the call runs in a `Task.async_stream` worker, which never joins its + # caller's transaction, so `in_transaction?/0` there proves nothing: the check reads the + # transaction state of the calling process AND of every process it was started for. defp forge(merge_sha, answer, times \\ 1) do expect(MockPullRequestSource, :contains?, times, fn %ForgeRepo{full_name: @repo}, ^merge_sha, "verified/master" -> - refute Loopctl.Repo.in_transaction?() - refute AdminRepo.in_transaction?() + refute any_caller_in_transaction?() answer end) end + # Ecto keeps a process's checked-out connection under `{Ecto.Adapters.SQL, pool}` in that + # process's dictionary, with `conn_mode: :transaction` while a transaction is open — the + # same read `Repo.in_transaction?/0` makes, here made on another process too. + defp any_caller_in_transaction? do + Enum.any?([self() | Process.get(:"$callers", [])], fn pid -> + case Process.info(pid, :dictionary) do + {:dictionary, dict} -> + Enum.any?( + dict, + &match?({{Ecto.Adapters.SQL, _}, %DBConnection{conn_mode: :transaction}}, &1) + ) + + nil -> + false + end + end) + end + defp no_forge_call, do: expect(MockPullRequestSource, :contains?, 0, fn _, _, _ -> :unused end) defp unverified!(story), @@ -316,11 +336,40 @@ defmodule Loopctl.Changes.TrunkEvidenceTest do push!(ctx, "C1", 1, "submitted", %{sha: sha("a"), data: %{"story_id" => ctx.story.id}}) :ok = push!(ctx, "C1", 2, "landed", %{sha: sha("a"), data: %{"merge_sha" => sha("c")}}) - stub(MockPullRequestSource, :contains?, fn _, _, _ -> {:ok, true} end) + no_forge_call() assert {:error, :chg_review_not_for_landed_revision} = verify(ctx, ctx.story) end + test "a story the local gates refuse costs no forge call, on any verify path", ctx do + # Landed but never reviewed: review_not_conducted on verify, the landed-review gate on + # the bulk paths. Each refusal is answered under the lock as before, unasked. + :ok = + push!(ctx, "C1", 1, "submitted", %{sha: sha("a"), data: %{"story_id" => ctx.story.id}}) + + :ok = push!(ctx, "C1", 2, "landed", %{sha: sha("a"), data: %{"merge_sha" => sha("c")}}) + no_forge_call() + opts = verifier_opts(ctx) + + assert {:error, :review_not_conducted} = verify(ctx, ctx.story) + + assert {:ok, %{verified_count: 0, errors: [%{reason: ":review_not_conducted"}]}} = + Progress.verify_all_in_epic(ctx.tenant_id, ctx.epic.id, %{"summary" => "a"}, opts) + + assert {:ok, [%{status: "error"}]} = + BulkOperations.bulk_verify( + ctx.tenant_id, + [%{"story_id" => ctx.story.id}], + opts[:orchestrator_agent_id], + opts + ) + + assert {:error, _refused} = + Progress.backfill_story(ctx.tenant_id, ctx.story.id, %{"reason" => "pre"}, []) + + unverified!(ctx.story) + end + test "a story with NO change binding never asks the forge, on any path", ctx do no_forge_call() outsider = outsider(ctx) @@ -449,4 +498,14 @@ defmodule Loopctl.Changes.TrunkEvidenceTest do assert {:ok, %Story{verified_status: :verified}} = verify(ctx, story) end end + + describe "the transaction probe the forge stub asserts with" do + test "sees a transaction open in the process a task was started for", _ctx do + AdminRepo.transaction(fn -> + assert Task.async(fn -> any_caller_in_transaction?() end) |> Task.await() + end) + + refute Task.async(fn -> any_caller_in_transaction?() end) |> Task.await() + end + end end diff --git a/test/loopctl/workers/verification_runner_worker_integration_test.exs b/test/loopctl/workers/verification_runner_worker_integration_test.exs index 13973777..72271719 100644 --- a/test/loopctl/workers/verification_runner_worker_integration_test.exs +++ b/test/loopctl/workers/verification_runner_worker_integration_test.exs @@ -147,8 +147,29 @@ defmodule Loopctl.Workers.VerificationRunnerWorkerIntegrationTest do refute_received {:evidence, _} end - # US-47.6: chg lands the change; verify read verified/master, so there is no CI to read. - test "a chg source records chg_landed_on_trunk, no forge read", ctx do + # US-47.6: a chg-bound story's verify read verified/master, so there is no CI to read. + test "a story bound to a chg change records chg_landed_on_trunk, no forge read", ctx do + fixture(:change_binding, %{tenant_id: ctx.tenant_id, story_id: ctx.story_id}) + stub_forge(ctx, %{evidence: build(:forge_evidence)}) + + run = fixture(:verification_run, ctx) + assert :ok = perform(ctx, run) + + reloaded = reload(ctx, run) + assert reloaded.status == "error" + + assert reloaded.ac_results == %{ + "source" => "ci", + "ci_unavailable_reason" => "chg_landed_on_trunk" + } + + refute_received {:compare, _} + refute_received {:evidence, _} + end + + # An unbound story is read as before, whatever its source's mode: a pull request it was + # delivered by still carries the CI. + test "an unbound story of a chg-mode source is read as before", ctx do root = fixture(:chg_root_dispatch, %{tenant_id: ctx.tenant_id}) [source] = @@ -167,17 +188,7 @@ defmodule Loopctl.Workers.VerificationRunnerWorkerIntegrationTest do run = fixture(:verification_run, ctx) assert :ok = perform(ctx, run) - - reloaded = reload(ctx, run) - assert reloaded.status == "error" - - assert reloaded.ac_results == %{ - "source" => "ci", - "ci_unavailable_reason" => "chg_landed_on_trunk" - } - - refute_received {:compare, _} - refute_received {:evidence, _} + assert reload(ctx, run).status == "pass" end # #931 finding g: the rescue arm records a code, never the exception.