From 0b67ccadbe31b4835c52d7e041a191c355c764b1 Mon Sep 17 00:00:00 2001 From: Mark Kreyman Date: Sun, 27 Sep 2026 00:12:14 -0600 Subject: [PATCH 1/4] US-45.6 (v2): a thread merges only on trusted CI for the checkpoint's exact commit Rewrite of #909, whose third review round still found material defects in one design element: which CI results the gate trusts and against which policy. - intake_sources.required_checks (migration 20260927100000), read LIVE by the gate, so a renamed job can be corrected for stories already in flight. A thread source must name at least one; local-gate and non-string names are refused. - Only a check run created by GitHub Actions satisfies a required name. Commit statuses are read and recorded, never trusted: the implementer's runner can post them. - A checkpoint changing .github/workflows/ or .github/actions/ (renames included) is refused ci_definition_changed for a human: Actions runs the workflow files of the commit under test. - Per name, the latest run of each check suite counts and every suite must pass. - A check still running or not reported is a CI wait: unevaluated, Retry-After 300, never counted toward the unevaluated bound (and it clears the count), refused required_check_timed_out 24 hours after the story ENTERED ci (Stages.entered_at/3). A wait holds back only an allow; any other refusal is decided at once. - Evidence (required runs and local-gate only) is copied onto the checkpoint's gate_evidence["ci"] under a row lock; a record read no later than the stored one is :superseded (the allow path waits and re-evaluates), an identical judgement is not rewritten. An allow whose copy met contention is unevaluated, never an escalation. - Required checks with nothing read fail closed (ci_evidence_not_read). - MCP 2.108.0: intake_source_enroll/update take required_checks; merge_precondition names the reasons. Mutations A01-A47 (A31 re-run as A31b after strengthening its test), all exit 0. --- CHANGELOG.md | 28 ++ deploy/FLY_SECRETS.md | 2 +- docs/agent-delivery-loop.md | 2 +- lib/loopctl/delivery/ci_evidence.ex | 190 ++++++++++ .../delivery/github_pull_request_source.ex | 115 +++++- lib/loopctl/delivery/merge_precondition.ex | 339 +++++++++++++++++- .../delivery/merge_precondition/verdict.ex | 19 +- lib/loopctl/delivery/pull_request_source.ex | 10 + lib/loopctl/delivery/stages.ex | 19 + lib/loopctl/intake.ex | 31 +- lib/loopctl/intake/source.ex | 88 +++++ lib/loopctl/threads.ex | 85 +++++ .../controllers/intake_source_controller.ex | 43 +++ .../merge_precondition_controller.ex | 29 +- mcp-server/CHANGELOG.md | 16 + mcp-server/README.md | 6 +- mcp-server/index.js | 38 +- mcp-server/lib/intake-sources.js | 56 ++- mcp-server/package-lock.json | 4 +- mcp-server/package.json | 2 +- mcp-server/test/intake_source_tools.test.js | 68 ++++ ...000_add_intake_sources_required_checks.exs | 27 ++ test/loopctl/delivery/ci_evidence_test.exs | 138 +++++++ .../github_pull_request_source_test.exs | 106 ++++++ .../merge_precondition_integration_test.exs | 144 +++++++- .../merge_precondition_judge_test.exs | 193 +++++++++- test/loopctl/delivery/stages_test.exs | 31 ++ test/loopctl/intake_test.exs | 87 +++++ test/loopctl/threads_test.exs | 83 +++++ .../intake_source_controller_test.exs | 12 +- .../merge_precondition_controller_test.exs | 2 + test/support/data_case.ex | 5 + test/support/fixtures.ex | 12 +- 33 files changed, 1983 insertions(+), 47 deletions(-) create mode 100644 lib/loopctl/delivery/ci_evidence.ex create mode 100644 priv/repo/migrations/20260927100000_add_intake_sources_required_checks.exs create mode 100644 test/loopctl/delivery/ci_evidence_test.exs diff --git a/CHANGELOG.md b/CHANGELOG.md index 130a25db..49caf40f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,34 @@ All notable changes to loopctl are documented here. ### Added +- **A thread-mode merge requires green CI on the checkpoint's exact commit (epic 45, US-45.6, + migration `20260927100000`). A THREAD-mode source must now name its `required_checks`, and + the gate's `GITHUB_TOKEN` needs `checks: read` and `commit statuses: read` for it.** The + migration adds `intake_sources.required_checks` (text array, NOT NULL, default empty; no + backfill). `POST`/`PATCH /api/v1/intake/sources` and the `intake_source_enroll` / + `intake_source_update` MCP tools take it; a `thread` source naming none is 422, judged over + the source as it will be, `local-gate` is refused, and a change is recorded as + `intake_source_required_checks_set` on the audit chain. **A thread-mode source enrolled + before this migration has no required checks and every one of its stories is refused + `required_checks_unset` until one is named.** Name GitHub Actions JOB names, and make + sure each required job runs on every push to the thread branches (no path filter or + job-level `if:` that can skip it): a required check that never appears is refused after the + wait. The merge gate reads the checkpoint's commit from both the check-runs and the + commit-status APIs, but only a GitHub Actions check run can satisfy a required check — a + commit status, which the implementer's runner can post, is recorded and never trusted — so + a CI that reports only statuses cannot satisfy thread mode. Per name, the latest run of + each check suite counts and every suite must pass. A failed one refuses + `required_check_failed`; one still running or not yet reported answers `unevaluated` + (`required_check_pending` / `required_check_missing`, `Retry-After: 300`), never counted + toward the unevaluated bound, and refused `required_check_timed_out` 24 hours after the + story entered `ci`. A checkpoint that changes `.github/workflows/` or `.github/actions/` + is refused `ci_definition_changed` for a human, because Actions runs the workflow files of + the commit under test. A failed read is `ci_evidence_unavailable`. The list is read from + the source live, so correcting it reaches stories already in flight. A + `local-gate` status is recorded and never satisfies a required check. What was read is + returned as `ci_evidence` and copied onto the checkpoint's `gate_evidence` under `ci`; an + allow whose copy did not land is refused `ci_evidence_not_recorded`. MCP server 2.108.0. + - **Review on a change thread (epic 45, US-45.3, runner contract 1.21.0, migration `20260926160000`). RE-VENDOR the contract and declare `review` to take review dispatches.** `POST /api/v1/stories/:id/thread/reviews` (orchestrator or above, human-anchored tenants) diff --git a/deploy/FLY_SECRETS.md b/deploy/FLY_SECRETS.md index 72d51495..69d0180b 100644 --- a/deploy/FLY_SECRETS.md +++ b/deploy/FLY_SECRETS.md @@ -179,7 +179,7 @@ during an incident with `fly secrets set … && fly apps restart` — no deploy. | Variable | Default | Description | |----------------|---------|-------------| -| `GITHUB_TOKEN` | - | Bearer token for the CI status/test-result lookups that back independent story verification, AND (#803) for the merge precondition's reads of a pull request's state, diffstat, changed names and file tree. Optional: unset, the calls go out unauthenticated, which works for PUBLIC repos until GitHub's 60-requests/hour/IP anonymous limit bites — after that verification reports a `github_api_error` rather than a real CI verdict, and the merge precondition answers `unevaluated` (HTTP 503) rather than merging anything — an exhausted quota is transient, so the loop RETRIES rather than escalating, and only escalates once the same story has been unevaluable at one head several times running. Either way nothing merges without the token. Required for a private repo, where unauthenticated lookups 404. Needs only read access to checks, pull requests and contents. A blank value is treated as unset (it is trimmed), so a templated-but-empty secret degrades to the anonymous path rather than sending an empty bearer that GitHub 401s. **Since #805 it also needs `issues: write`** — the only WRITE scope loopctl asks for. That is what lets the delivery loop close the GitHub issue a story came from, with a `loopctl:resolution-*` label and a comment naming the outcome; without it those calls 403, which is NOT a rate limit and NOT transient, so the closure is recorded `abandoned` with `permanent_forge_failure` on its first attempt and the reporter is never told what happened to her issue. Nothing else breaks and nothing is retried in a loop. **Recovering the backlog after you fix the token:** `SELECT abandoned_reason, date_trunc('hour', updated_at) AS at, count(*) FROM intake_issue_closures WHERE status = 'abandoned' GROUP BY 1, 2 ORDER BY 2 DESC;` shows the damage and WHEN it happened. Then, from a remote console, count before you write and requeue only the window you just fixed — `IssueClosures.requeue_abandoned(abandoned_after: ~U[YYYY-MM-DDThh:mm:00Z], dry_run: true)`, then the same call without `dry_run`. **An unbounded call is refused** (`{:error, :bound_required}`) and `tenant_id:` is not a bound: a closure abandoned months ago by an unrelated outage still names a live issue, and waking it puts a fresh label, comment and close on a ticket the reporter has long since moved on from. Pass `unbounded: true` to mean it. It never re-drives a `closed_by_other` or `source_revoked` row — a human already closed that issue, or the tenant disconnected the repository | +| `GITHUB_TOKEN` | - | Bearer token for the CI status/test-result lookups that back independent story verification, AND (#803) for the merge precondition's reads of a pull request's state, diffstat, changed names and file tree. Optional: unset, the calls go out unauthenticated, which works for PUBLIC repos until GitHub's 60-requests/hour/IP anonymous limit bites — after that verification reports a `github_api_error` rather than a real CI verdict, and the merge precondition answers `unevaluated` (HTTP 503) rather than merging anything — an exhausted quota is transient, so the loop RETRIES rather than escalating, and only escalates once the same story has been unevaluable at one head several times running. Either way nothing merges without the token. Required for a private repo, where unauthenticated lookups 404. Needs only read access to checks, pull requests and contents. A blank value is treated as unset (it is trimmed), so a templated-but-empty secret degrades to the anonymous path rather than sending an empty bearer that GitHub 401s. **Since US-45.6 a THREAD-mode source also needs `checks: read` and `commit statuses: read`**: the merge gate reads CI for a checkpoint's exact commit from both APIs, and without them the read is refused `ci_evidence_unavailable` and nothing thread-mode merges (a pr-mode source never reads them). **Since #805 it also needs `issues: write`** — the only WRITE scope loopctl asks for. That is what lets the delivery loop close the GitHub issue a story came from, with a `loopctl:resolution-*` label and a comment naming the outcome; without it those calls 403, which is NOT a rate limit and NOT transient, so the closure is recorded `abandoned` with `permanent_forge_failure` on its first attempt and the reporter is never told what happened to her issue. Nothing else breaks and nothing is retried in a loop. **Recovering the backlog after you fix the token:** `SELECT abandoned_reason, date_trunc('hour', updated_at) AS at, count(*) FROM intake_issue_closures WHERE status = 'abandoned' GROUP BY 1, 2 ORDER BY 2 DESC;` shows the damage and WHEN it happened. Then, from a remote console, count before you write and requeue only the window you just fixed — `IssueClosures.requeue_abandoned(abandoned_after: ~U[YYYY-MM-DDThh:mm:00Z], dry_run: true)`, then the same call without `dry_run`. **An unbounded call is refused** (`{:error, :bound_required}`) and `tenant_id:` is not a bound: a closure abandoned months ago by an unrelated outage still names a live issue, and waking it puts a fresh label, comment and close on a ticket the reporter has long since moved on from. Pass `unbounded: true` to mean it. It never re-drives a `closed_by_other` or `source_revoked` row — a human already closed that issue, or the tenant disconnected the repository | #### Post-deploy verification (#803 §9) diff --git a/docs/agent-delivery-loop.md b/docs/agent-delivery-loop.md index 36e6ad67..afe9574b 100644 --- a/docs/agent-delivery-loop.md +++ b/docs/agent-delivery-loop.md @@ -122,7 +122,7 @@ Before a merge, an orchestrator or operator calls `merge_precondition` (`POST /a | `head_moved` | The pull request's head changed since CI ran. The story goes back to `implementing` over `base_moved`, and the recorded head is cleared, so the gate is not called again until the story is back at `ci`. | | `unevaluated` | 503 with `Retry-After`. After repeated `unevaluated` answers the story escalates. | -**Thread mode (epic 45, US-45.4).** A story whose claim was PLACED under an intake source with `mode: thread` (`intake_source_update`) has no pull request. The mode, and the base branch, are recorded on the implement dispatch at placement, so changing the source affects only stories placed afterwards; a change is always allowed. The gate judges the latest checkpoint the story's CURRENT claim recorded, on the branch that claim's dispatch ran on and against the base branch it was placed on, needs no `pr_number`, and also refuses `empty_change` (the checkpoint's tree equals the base's, or no file changed), `checkpoint_tree_mismatch`, `no_checkpoint_recorded`, `claim_ended` (the current claim recorded nothing but an earlier, released one did), and `thread_unreadable` (loopctl could not read the thread). The branch is judged first: a branch missing from a readable repository (`branch_missing`), one naming a commit nobody reported (`branch_head_unrecorded`), one naming an earlier checkpoint of the claim (`branch_head_regressed`), or a checkpoint that is not the recorded head means the head moved. The diff judged is the checkpoint's three-dot diff against its merge base, so a base that moved on is not a refusal, and a checkpoint the base already contains, its branch deleted or not, is `already_merged` only under a recorded allow naming it, and otherwise refused `checkpoint_on_base_without_allow`. A claim with no accepted dispatch (a session that claimed the story itself) is judged as a pull request against the source's current base branch, whatever the source's mode. **While the claim is live** that is `head_moved`, back to `implementing` like any moved head. **When it is not live** (reported, review requested, lease expired) the claimant cannot record a fix, so the gate refuses naming `claim_not_live` and the story escalates instead of looping. A repository the token cannot read refuses `pull_request_unavailable`. An allow is recorded naming the checkpoint id and sha and `base_sha`, the merge base the judged diff is relative to. **Thread mode needs the source's runners at runner contract 1.20.0 or later sending `checkpoint` messages; otherwise every story is refused `no_checkpoint_recorded`.** The merge executor (US-45.5) merges only while the base head still equals that `base_sha`, and otherwise takes its base-update path (US-45.5). This gate judges claimant checkpoints only; reading the executor's `base_update` checkpoints is US-45.5's (AC-45.5.9). +**Thread mode (epic 45, US-45.4).** A story whose claim was PLACED under an intake source with `mode: thread` (`intake_source_update`) has no pull request. The mode, and the base branch, are recorded on the implement dispatch at placement, so changing the source affects only stories placed afterwards; a change is always allowed. The gate judges the latest checkpoint the story's CURRENT claim recorded, on the branch that claim's dispatch ran on and against the base branch it was placed on, needs no `pr_number`, and also refuses `empty_change` (the checkpoint's tree equals the base's, or no file changed), `checkpoint_tree_mismatch`, `no_checkpoint_recorded`, `claim_ended` (the current claim recorded nothing but an earlier, released one did), and `thread_unreadable` (loopctl could not read the thread). The branch is judged first: a branch missing from a readable repository (`branch_missing`), one naming a commit nobody reported (`branch_head_unrecorded`), one naming an earlier checkpoint of the claim (`branch_head_regressed`), or a checkpoint that is not the recorded head means the head moved. The diff judged is the checkpoint's three-dot diff against its merge base, so a base that moved on is not a refusal, and a checkpoint the base already contains, its branch deleted or not, is `already_merged` only under a recorded allow naming it, and otherwise refused `checkpoint_on_base_without_allow`. A claim with no accepted dispatch (a session that claimed the story itself) is judged as a pull request against the source's current base branch, whatever the source's mode. **While the claim is live** that is `head_moved`, back to `implementing` like any moved head. **When it is not live** (reported, review requested, lease expired) the claimant cannot record a fix, so the gate refuses naming `claim_not_live` and the story escalates instead of looping. A repository the token cannot read refuses `pull_request_unavailable`. An allow is recorded naming the checkpoint id and sha and `base_sha`, the merge base the judged diff is relative to. **Thread mode needs the source's runners at runner contract 1.20.0 or later sending `checkpoint` messages; otherwise every story is refused `no_checkpoint_recorded`.** **CI is read by the checkpoint's exact SHA (US-45.6)**, from both the check-runs and the commit-status APIs (only a GitHub Actions check run can satisfy a required check; a commit status, which anyone with `statuses: write` could post, is recorded and never trusted), against the source's `required_checks` (a thread source must name at least one; set with `intake_source_update`): a failed one refuses `required_check_failed`; one still running or not yet reported answers `unevaluated` with a 300-second retry and never counts toward the unevaluated bound; 24 hours after the story entered `ci` both are refused `required_check_timed_out`, so a slow pipeline waits and a stuck or missing check still reaches a human. Per name the latest run of each check suite counts and every suite must pass; the required checks are the source's current list, so each required job must run on every push to the thread branches; and a checkpoint changing `.github/workflows/` or `.github/actions/` is refused `ci_definition_changed` for a human, because Actions runs the workflow files of the commit under test; a source requiring none refuses `required_checks_unset`. A `local-gate` status is recorded on the checkpoint and never satisfies a required check, because whoever pushed posts it. What was read is copied onto the checkpoint's `gate_evidence` under `ci`. The merge executor (US-45.5) merges only while the base head still equals that `base_sha`, and otherwise takes its base-update path (US-45.5). This gate judges claimant checkpoints only; reading the executor's `base_update` checkpoints is US-45.5's (AC-45.5.9). ## 7. After the merge diff --git a/lib/loopctl/delivery/ci_evidence.ex b/lib/loopctl/delivery/ci_evidence.ex new file mode 100644 index 00000000..790bc709 --- /dev/null +++ b/lib/loopctl/delivery/ci_evidence.ex @@ -0,0 +1,190 @@ +defmodule Loopctl.Delivery.CiEvidence do + @moduledoc """ + What CI says about ONE commit, judged against a list of required checks (US-45.6). Pure. + + A thread-mode story merges the checkpoint's exact commit with no pull request, so the merge + gate cannot lean on a forge rule to hold the merge to green CI: it reads the evidence for + the checkpoint's SHA itself (`Loopctl.Delivery.PullRequestSource.check_evidence/3`) and + this module decides what it says. + + ## What may satisfy a required check: a GitHub Actions check run, and nothing else + + Both the check-runs and the commit-status APIs are READ, by SHA, and both are recorded on + the checkpoint — GitHub Actions reports check runs, which the combined status never lists. + But only a CHECK RUN created by GitHub Actions (`@trusted_check_apps`) can satisfy a + required name (US-45.6 review round 2, finding 1). A commit status can be posted by anyone + holding `statuses: write` on the repository, which includes the implementer's own runner + (it posts `local-gate`), so letting a status satisfy a required name let the implementer + post `test = success` over a failing run and merge its own work — the self-attestation this + gate exists to refuse, one name over from `local-gate`. A check run needs a GitHub App to + create; the implementer holds none. Statuses are therefore evidence to READ, never to trust. + + Evidence for ANY OTHER commit never counts: nothing here reads a branch, a parent or a pull + request, only the one SHA the caller names. + + ## How one required check is judged + + Trusted runs under a name are grouped by CHECK SUITE (one workflow run's suite), and in + each suite only the LATEST run counts — the highest id, because ids only grow and a re-run + queued a moment ago has no timestamp yet — so a re-run that went green supersedes the + failure it re-ran. ACROSS suites nothing supersedes anything: two workflows that each have + a job `test` are two checks under one name, and a green one must never hide a red one. So + a name is `:failed` when any suite's latest run failed, `:pending` when none failed and + any is still running, and `:passed` only when every suite's latest run passed. Each run is: + + - not `completed` is `:pending` + - `completed` concluding `success`, `neutral` or `skipped` (what GitHub itself counts as + passing a required check) is `:passed` + - any other conclusion (`failure`, `cancelled`, `timed_out`, `action_required`, `stale`) is + `:failed` + - no trusted run under that name is `:missing`, whatever statuses say + + ## The local gate is recorded, never trusted + + `local-gate` (`Loopctl.Intake.Source.local_gate/0`) is reported under `local_gate` and never + looked up as a required check, even by a caller that lists it: the intake source refuses to + store it, and this module drops it from the list as the backstop. + """ + + alias Loopctl.Intake.Source + + @passing_conclusions ["success", "neutral", "skipped"] + + # The apps whose check runs may satisfy a required check. See the moduledoc. + @trusted_check_apps ["github-actions"] + + @doc "The GitHub App slugs whose check runs may satisfy a required check." + @spec trusted_check_apps() :: [String.t()] + def trusted_check_apps, do: @trusted_check_apps + + @type check_run :: %{ + required(:name) => String.t(), + required(:status) => String.t(), + required(:conclusion) => String.t() | nil, + optional(:id) => integer() | nil, + optional(:app) => String.t() | nil, + optional(:check_suite) => integer() | nil, + optional(:started_at) => String.t() | nil, + optional(:completed_at) => String.t() | nil, + optional(:url) => String.t() | nil + } + @type status :: %{ + required(:context) => String.t(), + required(:state) => String.t(), + optional(:at) => String.t() | nil, + optional(:url) => String.t() | nil + } + @type evidence :: %{check_runs: [check_run()], statuses: [status()]} + @type result :: %{ + passed: [String.t()], + pending: [String.t()], + missing: [String.t()], + failed: [{String.t(), String.t()}], + local_gate: String.t() | nil + } + + @doc "The required names this module will look up: `required` without `local-gate`." + @spec lookup_names([String.t()]) :: [String.t()] + def lookup_names(required), do: Enum.reject(required, &(&1 == Source.local_gate())) + + @doc "Judges `evidence` for one commit against `required`. See the moduledoc." + @spec judge([String.t()], evidence()) :: result() + def judge(required, %{check_runs: runs, statuses: statuses}) do + acc = %{passed: [], pending: [], missing: [], failed: []} + + judged = + required + |> lookup_names() + |> Enum.reduce(acc, fn name, acc -> + case check_state(name, runs) do + {:failed, conclusion} -> Map.update!(acc, :failed, &[{name, conclusion} | &1]) + state -> Map.update!(acc, state, &[name | &1]) + end + end) + |> Map.new(fn {key, names} -> {key, Enum.reverse(names)} end) + + Map.put(judged, :local_gate, local_gate_state(statuses)) + end + + defp check_state(name, runs) do + states = + runs + |> Enum.filter(&(&1.name == name and Map.get(&1, :app) in @trusted_check_apps)) + |> Enum.group_by(&Map.get(&1, :check_suite)) + |> Enum.map(fn {_suite, suite_runs} -> + suite_runs |> Enum.max_by(&(Map.get(&1, :id) || 0)) |> run_state() + end) + + cond do + states == [] -> :missing + failed = Enum.find(states, &match?({:failed, _}, &1)) -> failed + :pending in states -> :pending + true -> :passed + end + end + + defp run_state(%{status: "completed", conclusion: conclusion}) + when conclusion in @passing_conclusions, + do: :passed + + defp run_state(%{status: "completed", conclusion: conclusion}), + do: {:failed, conclusion || "none"} + + defp run_state(_running), do: :pending + + defp local_gate_state(statuses) do + local_gate = Source.local_gate() + + Enum.find_value(statuses, fn + %{context: ^local_gate, state: state} -> state + _other -> nil + end) + end + + @doc """ + The evidence and its judgement as stored on the checkpoint's `gate_evidence` under `"ci"` + (AC-45.6.1): string keys, so it reads back as it was written. + + Only what the judgement READ is kept: the runs under a required name and the `local-gate` + status. A commit can carry hundreds of unrelated runs, and keeping them made the record + change whenever any of them moved, so an unchanged judgement was rewritten on every poll. + """ + @spec to_record(String.t(), [String.t()], evidence(), result(), DateTime.t()) :: map() + def to_record(sha, required, %{check_runs: runs, statuses: statuses}, result, read_at) do + names = lookup_names(required) + runs = Enum.filter(runs, &(&1.name in names)) + statuses = Enum.filter(statuses, &(&1.context == Source.local_gate())) + + %{ + "sha" => sha, + "read_at" => fixed_width_iso8601(read_at), + "required" => required, + "check_runs" => + Enum.map(runs, fn run -> + %{ + "name" => run.name, + "app" => Map.get(run, :app), + "check_suite" => Map.get(run, :check_suite), + "status" => run.status, + "conclusion" => run.conclusion, + "url" => Map.get(run, :url) + } + end), + "statuses" => + Enum.map(statuses, fn status -> + %{"context" => status.context, "state" => status.state, "url" => Map.get(status, :url)} + end), + "local_gate" => result.local_gate, + "passed" => result.passed, + "pending" => result.pending, + "missing" => result.missing, + "failed" => Enum.map(result.failed, fn {name, why} -> %{"name" => name, "why" => why} end) + } + end + + # ALWAYS six fractional digits, so two records order correctly as TEXT: the evidence write + # compares `read_at` in SQL (`Loopctl.Threads.record_gate_evidence/5`), and + # `"...:00Z"` sorts after `"...:00.5Z"` although it is earlier. + defp fixed_width_iso8601(%DateTime{microsecond: {micro, _precision}} = at), + do: DateTime.to_iso8601(%{at | microsecond: {micro, 6}}) +end diff --git a/lib/loopctl/delivery/github_pull_request_source.ex b/lib/loopctl/delivery/github_pull_request_source.ex index 315e66cd..b252da25 100644 --- a/lib/loopctl/delivery/github_pull_request_source.ex +++ b/lib/loopctl/delivery/github_pull_request_source.ex @@ -24,6 +24,16 @@ defmodule Loopctl.Delivery.GitHubPullRequestSource do total of its own, so a list that reaches the cap is refused as truncated rather than presented as the whole diff + CI evidence for a thread checkpoint (US-45.6), by the checkpoint's exact SHA: + + - `GET /repos/:repo/commits/:sha/check-runs?filter=latest&per_page=100&page=:n` — every + check run on the commit, paged up to `@check_run_pages`; the required names are matched + locally + - `GET /repos/:repo/commits/:sha/status?per_page=100` — the latest status per context. + Both, because the combined status never lists check runs (GitHub Actions) and the + check-runs API never lists statuses. A list reporting more entries than it carried is + refused as truncated: a failure on a missing page would read as a pass + Post-deploy verification (#803 §9) adds three more, each bounded the same way: 4. `GET /repos/:repo/deployments?environment=:env&per_page=…` — a small page @@ -57,7 +67,10 @@ defmodule Loopctl.Delivery.GitHubPullRequestSource do — Req retries transient failures by DEFAULT, which would multiply the ceiling silently. Three calls for `pull_request/2` and one for `repo_files/2`, so a precondition that makes all five (a pull request plus two refs) waits at most 35 seconds before it has an answer, - and the answer to a timeout is an ESCALATION, never a pass. Nothing here runs inside a + and the answer to a timeout is an ESCALATION, never a pass. A THREAD-mode evaluation makes + more: the branch ref, the commit, the comparison, two trees, at most `@check_run_pages` + pages of check runs and the combined status — nine calls, 63 seconds at the most, plus one + repository read after a 404 on the branch. Nothing here runs inside a database transaction: the caller gathers every fact before it opens one, so a slow forge never holds a pooled connection. @@ -105,6 +118,10 @@ defmodule Loopctl.Delivery.GitHubPullRequestSource do @connect_timeout_ms 2_000 @receive_timeout_ms 5_000 @files_per_page 100 + # The combined status lists the LATEST status per context, one page of them. + @status_page_size 100 + # At most this many pages of 100 check runs per commit are read (US-45.6). + @check_run_pages 3 # GitHub's primary rate-limit window is an hour. Anything beyond that plus slack is not a # window rolling over, so it is not turned into a delay a caller would sleep on. @@ -191,6 +208,102 @@ defmodule Loopctl.Delivery.GitHubPullRequestSource do end end + @impl true + def check_evidence(repo, sha) do + with {:ok, repo} <- repo_name(repo), + {:ok, sha} <- ref(sha), + {:ok, runs} <- check_runs_page(repo, sha, 1, []), + {:ok, body} <- get(repo, "/commits/#{sha}/status?per_page=#{@status_page_size}"), + {:ok, statuses} <- commit_statuses(body) do + {:ok, %{check_runs: runs, statuses: statuses}} + end + end + + # EVERY check run on the commit, `filter=latest`, paged — never one call per required name + # (US-45.6 review round 1, finding 7): a request per name multiplied both the worst-case + # wait and the forge calls each poll spends by the length of the list. The names are + # matched here, by `CiEvidence`. A commit carrying more runs than `@check_run_pages` pages + # hold is refused as truncated rather than judged on the part that fitted. + defp check_runs_page(repo, sha, page, acc) do + query = URI.encode_query(%{"filter" => "latest", "per_page" => 100, "page" => page}) + + with {:ok, body} <- get(repo, "/commits/#{sha}/check-runs?" <> query), + {:ok, total, runs} <- check_runs(body) do + acc = acc ++ runs + + cond do + length(acc) >= total or runs == [] -> complete_runs(acc, total) + page >= @check_run_pages -> {:error, {:check_runs_truncated, total, length(acc)}} + true -> check_runs_page(repo, sha, page + 1, acc) + end + end + end + + defp complete_runs(runs, total) when length(runs) >= total, do: {:ok, runs} + defp complete_runs(runs, total), do: {:error, {:check_runs_truncated, total, length(runs)}} + + defp check_runs(%{"total_count" => total, "check_runs" => runs}) + when is_integer(total) and is_list(runs) do + if Enum.all?(runs, &check_run?/1), + do: {:ok, total, Enum.map(runs, &check_run_fact/1)}, + else: {:error, {:unreadable_check_runs, shape(runs)}} + end + + defp check_runs(body), do: {:error, {:unreadable_check_runs, shape(body)}} + + defp check_run_fact(run) do + %{ + id: run["id"], + # The creating App's slug: only a trusted App's run can satisfy a required check + # (`Loopctl.Delivery.CiEvidence`). + app: get_in(run, ["app", "slug"]), + # The suite (one workflow run) the run belongs to: runs are judged latest-per-suite. + check_suite: get_in(run, ["check_suite", "id"]), + name: run["name"], + status: run["status"], + conclusion: run["conclusion"], + started_at: run["started_at"], + completed_at: run["completed_at"], + url: run["html_url"] + } + end + + defp check_run?(%{"name" => name, "status" => status} = run) + when is_binary(name) and is_binary(status), + do: is_nil(run["conclusion"]) or is_binary(run["conclusion"]) + + defp check_run?(_run), do: false + + defp commit_statuses(%{"total_count" => total, "statuses" => statuses}) + when is_integer(total) and is_list(statuses) do + cond do + total > length(statuses) -> + {:error, {:statuses_truncated, total, length(statuses)}} + + Enum.all?(statuses, &status?/1) -> + {:ok, + Enum.map(statuses, fn status -> + %{ + context: status["context"], + state: status["state"], + at: status["updated_at"], + url: status["target_url"] + } + end)} + + true -> + {:error, {:unreadable_statuses, shape(statuses)}} + end + end + + defp commit_statuses(body), do: {:error, {:unreadable_statuses, shape(body)}} + + defp status?(%{"context" => context, "state" => state}) + when is_binary(context) and is_binary(state), + do: true + + defp status?(_status), do: false + @impl true def compare(repo, base, head) do with {:ok, repo} <- repo_name(repo), diff --git a/lib/loopctl/delivery/merge_precondition.ex b/lib/loopctl/delivery/merge_precondition.ex index 7bc31de5..1cb8c6ec 100644 --- a/lib/loopctl/delivery/merge_precondition.ex +++ b/lib/loopctl/delivery/merge_precondition.ex @@ -257,6 +257,7 @@ defmodule Loopctl.Delivery.MergePrecondition do require Logger alias Loopctl.Delivery.CheckpointSource + alias Loopctl.Delivery.CiEvidence alias Loopctl.Delivery.Claimant alias Loopctl.Delivery.DispatchPayload alias Loopctl.Delivery.GateAInput @@ -317,7 +318,13 @@ defmodule Loopctl.Delivery.MergePrecondition do optional(:checkpoint) => fact(map()), # Whether the claimant can still record a checkpoint (`Claimant.live?/2`). Absent is # NOT live: a thread-mode head that moved then escalates rather than looping. - optional(:claim_live?) => boolean() + optional(:claim_live?) => boolean(), + # THREAD mode (US-45.6): the checks the source requires, and what CI said about the + # checkpoint's exact commit (`{:ok, nil}` where nothing was read). + optional(:required_checks) => [String.t()], + optional(:ci_evidence) => fact(map() | nil), + optional(:ci_entered_at) => DateTime.t() | nil, + optional(:now) => DateTime.t() } @type error :: :not_found | :no_stage | :wrong_stage @@ -363,6 +370,8 @@ defmodule Loopctl.Delivery.MergePrecondition do """ @spec judge(facts()) :: Verdict.t() def judge(facts) do + # CI is judged ONCE per evaluation, and every step that turns on it reads that one result. + facts = Map.put(facts, :ci_result, judge_ci(facts)) {gate_a, gate_a_inputs} = gate_a(Map.get(facts, :gate_a_input, :missing)) gate_a = redact_gate_a(gate_a) custody = Map.get(facts, :custody, {:error, :custody_unknown}) @@ -380,7 +389,8 @@ defmodule Loopctl.Delivery.MergePrecondition do recorded_head_sha: Map.get(facts, :recorded_head_sha), mode: mode(facts), checkpoint_id: checkpoint_field(facts, :id), - checkpoint_sha: checkpoint_field(facts, :commit_sha) + checkpoint_sha: checkpoint_field(facts, :commit_sha), + ci_evidence: ci_record(facts) } # Computed ONCE, from the facts, and handed to every step that turns on it. @@ -429,20 +439,24 @@ defmodule Loopctl.Delivery.MergePrecondition do # evaluated, so nothing transitions. The reasons still carry whatever else is known — # a caller fixing custody should not have to wait for the forge to come back to hear # about it — but the DECISION is that there is no verdict yet. - case {transient, other} do - {[_ | _] = transient, other} -> + {ci_waits, forge} = Enum.split_with(transient, &ci_wait?/1) + + case {forge, other} do + {[_ | _] = forge, other} -> %{ base | decision: :unevaluated, - reasons: Enum.uniq(transient ++ other ++ carried), - retry_after: longest_retry_after(transient) + reasons: Enum.uniq(forge ++ ci_waits ++ other ++ carried), + retry_after: longest_retry_after(forge) } {[], [_ | _] = broken} -> refuse(base, broken ++ carried) {[], []} -> - decide(base, facts, value(facts, :pull_request), carried, moved) + base + |> decide(facts, value(facts, :pull_request), carried, moved) + |> await_ci(ci_waits) end end @@ -554,10 +568,40 @@ defmodule Loopctl.Delivery.MergePrecondition do @spec enforce(Ecto.UUID.t(), Ecto.UUID.t(), keyword()) :: {:ok, Verdict.t()} | {:error, error()} def enforce(tenant_id, story_id, opts) do with {:ok, verdict} <- evaluate(tenant_id, story_id, opts) do - {:ok, act(tenant_id, story_id, verdict, opts)} + {:ok, act(tenant_id, story_id, verdict, opts) |> note_ci_evidence(tenant_id, story_id)} + end + end + + # AC-45.6.1: the CI evidence the verdict read is copied onto the checkpoint it was read + # for, whatever the decision, so the thread shows why a checkpoint did or did not merge. + # An allow has ALREADY copied it (`record_allow/4`, where a copy that does not land is a + # refusal); for any other decision a copy that does not land is logged and changes nothing, + # because the decision it would explain was not an authorisation. + defp note_ci_evidence(%Verdict{decision: :allow} = verdict, _tenant_id, _story_id), + do: verdict + + defp note_ci_evidence(%Verdict{} = verdict, tenant_id, story_id) do + case copy_ci_evidence(tenant_id, story_id, verdict) do + # A newer read already stands, which is the record the thread should show. + ok when ok in [:ok, :superseded] -> + verdict + + {:error, reason} -> + Logger.warning( + "merge_gate ci evidence not recorded story_id=#{story_id} tenant_id=#{tenant_id} " <> + "checkpoint_id=#{verdict.checkpoint_id} reason=#{inspect(reason)}" + ) + + verdict end end + defp copy_ci_evidence(tenant_id, story_id, %Verdict{ci_evidence: %{} = record} = verdict) + when is_binary(verdict.checkpoint_id), + do: Threads.record_gate_evidence(tenant_id, story_id, verdict.checkpoint_id, "ci", record) + + defp copy_ci_evidence(_tenant_id, _story_id, %Verdict{}), do: :ok + # -- the judgement (pure) -------------------------------------------------------------- @input_facts [ @@ -571,12 +615,14 @@ defmodule Loopctl.Delivery.MergePrecondition do @thread_input_facts [ {:repo, :repository_unresolved}, {:checkpoint, :thread_unreadable}, - {:pull_request, :pull_request_unavailable} + {:pull_request, :pull_request_unavailable}, + {:ci_evidence, :ci_evidence_unavailable} ] @forge_facts [ {:checkpoint, :thread_unreadable}, {:pull_request, :pull_request_unavailable}, + {:ci_evidence, :ci_evidence_unavailable}, {:head_files, :head_files_unavailable}, {:base_files, :base_files_unavailable} ] @@ -619,12 +665,122 @@ defmodule Loopctl.Delivery.MergePrecondition do defp input_reason(kind, reason), do: {kind, reason} defp unevaluated_reasons(facts, moved) do - for {key, kind} <- consumed_facts(facts, moved), - reason = error_reason(facts, key), - transient?(reason), - do: {kind, reason} + forge = + for {key, kind} <- consumed_facts(facts, moved), + reason = error_reason(facts, key), + transient?(reason), + do: {kind, reason} + + forge ++ ci_wait_reasons(facts, moved) + end + + # US-45.6: a required check still running, or not reported on the commit at all, is not a + # verdict: the gate answers `:unevaluated` and the loop asks again. Only on the path that + # would otherwise be JUDGED (nothing moved, nothing merged), only when no required check + # has already FAILED — a failure is decisive whatever is still running, and is refused by + # `thread_reasons/2` — and only inside `ci_wait_limit_seconds/0` of the checkpoint being + # recorded. Past it the same checks are refused (`ci_timeout_reasons/1`). + defp ci_wait_reasons(facts, moved) do + with :thread <- mode(facts), + [] <- moved, + %{merged?: false} <- value(facts, :pull_request), + %{failed: []} = result <- ci_result(facts), + false <- ci_wait_exceeded?(facts) do + waiting_checks(result) + else + _judged_elsewhere -> [] + end + end + + defp waiting_checks(%{pending: pending, missing: missing}) do + Enum.map(pending, &{:required_check_pending, &1}) ++ + Enum.map(missing, &{:required_check_missing, &1}) + end + + # HOW LONG A THREAD WAITS FOR CI: a bound in TIME, never a count of polls, measured from + # when the story ENTERED `ci` — the moment the gate starts asking for CI at all. + # + # Polls were the wrong unit at both ends: a job gated by `needs:` has no check run until it + # starts, so a legitimately slow pipeline read as missing and escalated after a few polls, + # and a check that never finishes was never bounded at all. And the checkpoint's recording + # time was the wrong origin: a story can spend hours in review between recording its last + # checkpoint and reaching `ci`, and was then refused on its first evaluation with no wait. + # + # Twenty-four hours is GitHub's ceiling on how long a job may QUEUE for a self-hosted runner + # (this fleet's CI runs on one), which is longer than any job may RUN on a hosted one: past + # it nothing still waiting will start, and a human is told which check never came back. + @ci_wait_limit_seconds 24 * 60 * 60 + + @doc "How long a thread waits for its required checks before they are refused (US-45.6)." + @spec ci_wait_limit_seconds() :: pos_integer() + def ci_wait_limit_seconds, do: @ci_wait_limit_seconds + + defp ci_wait_exceeded?(facts) do + case {Map.get(facts, :ci_entered_at), Map.get(facts, :now)} do + {%DateTime{} = entered, %DateTime{} = now} -> + DateTime.diff(now, entered) > @ci_wait_limit_seconds + + _unknown -> + false + end + end + + # A CI wait asks again after five minutes. Each evaluation re-reads the whole thread from + # the forge (about nine calls), and a pipeline takes minutes to hours, so a tighter loop + # only spends the shared token's rate limit (round 2, finding 6). + @ci_wait_retry_after 300 + + # A CI wait holds back only an ALLOW (round 2, finding 3). Everything else the change was + # judged on is decided now: a refusal (a tree mismatch, an empty change, the size bound, a + # gate) or a moved head does not wait up to a day for CI to finish first. + defp await_ci(%Verdict{decision: :allow} = verdict, [_ | _] = ci_waits) do + %{verdict | decision: :unevaluated, reasons: ci_waits, retry_after: @ci_wait_retry_after} + end + + defp await_ci(verdict, _ci_waits), do: verdict + + defp ci_wait?({kind, _name}), + do: kind in [:required_check_pending, :required_check_missing, :ci_evidence_superseded] + + defp ci_wait?(_reason), do: false + + @doc """ + Whether an `:unevaluated` verdict counts toward `max_consecutive_unevaluated/0`: every one + does EXCEPT one that is only waiting for CI (US-45.6). A CI wait has its own bound, in TIME + (`ci_wait_limit_seconds/0`), because polls are the wrong unit for it: a pipeline waiting on + `needs:` outlasts a few polls legitimately. A wait that ALSO carries a transient forge fault + counts, as that fault always has. + """ + @spec counts_toward_unevaluated_bound?(Verdict.t()) :: boolean() + def counts_toward_unevaluated_bound?(%Verdict{reasons: reasons}) do + not Enum.any?(reasons, &ci_wait?/1) or + Enum.any?(reasons, fn + {_kind, reason} -> transient?(reason) + _other -> false + end) + end + + defp ci_result(facts), do: Map.get_lazy(facts, :ci_result, fn -> judge_ci(facts) end) + + defp judge_ci(facts) do + case value(facts, :ci_evidence) do + %{evidence: evidence} -> CiEvidence.judge(required_checks(facts), evidence) + _not_read -> nil + end end + defp ci_record(facts) do + case {value(facts, :ci_evidence), ci_result(facts)} do + {%{evidence: evidence, sha: sha, read_at: read_at}, %{} = result} -> + CiEvidence.to_record(sha, required_checks(facts), evidence, result, read_at) + + _not_read -> + nil + end + end + + defp required_checks(facts), do: Map.get(facts, :required_checks, []) + # A branch that reads neither file list must not be judged on one. Both the merged branch # and the head-moved branch decide from the pull request alone, so a rate-limited tree # call there would mask the decision — turning an ordinary push into an escalation once @@ -828,7 +984,64 @@ defmodule Loopctl.Delivery.MergePrecondition do tree = Map.get(pr, :head_tree_sha) tree_reasons(tree, checkpoint_field(facts, :tree_sha)) ++ - empty_change_reasons(tree, Map.get(pr, :base_tree_sha), Map.get(pr, :diffstat)) + empty_change_reasons(tree, Map.get(pr, :base_tree_sha), Map.get(pr, :diffstat)) ++ + ci_definition_reasons(Map.get(pr, :diff)) ++ + ci_reasons(facts) + else + [] + end + end + + # A THREAD THAT CHANGES ITS OWN CI IS NEVER MERGED ON THAT CI (US-45.6 review round 3). + # GitHub Actions runs the workflow files of the commit under test, so a checkpoint that + # edits `.github/workflows/` — or a composite action they call — can make its own required + # checks report green: the implementer attesting its own work through a check run it never + # needed an App to create. A human merges such a change. Matched on every name the diff + # carries, renames' old and new names included, so moving a workflow file is caught too. + @ci_definition_prefixes [".github/workflows/", ".github/actions/"] + + defp ci_definition_reasons({:ok, %{files: files} = diff}) do + renamed = for {from, to} <- Map.get(diff, :renames, []), name <- [from, to], do: name + + case Enum.filter(Enum.uniq(files ++ renamed), &ci_definition?/1) do + [] -> [] + touched -> [{:ci_definition_changed, Enum.sort(touched)}] + end + end + + defp ci_definition_reasons(_no_diff), do: [] + + defp ci_definition?(name) when is_binary(name), + do: Enum.any?(@ci_definition_prefixes, &String.starts_with?(name, &1)) + + defp ci_definition?(_name), do: false + + # US-45.6: a thread merges with no forge rule holding it to green CI, so a source that + # requires nothing would merge whatever CI said; and a required check that FAILED on the + # checkpoint's exact commit refuses. Waits are `ci_wait_reasons/2`'s. + defp ci_reasons(facts) do + cond do + CiEvidence.lookup_names(required_checks(facts)) == [] -> + [:required_checks_unset] + + result = ci_result(facts) -> + failed = for {name, why} <- result.failed, do: {:required_check_failed, name, why} + failed ++ ci_timeout_reasons(facts, result) + + # Checks are required and nothing was read: this path would otherwise ALLOW with no CI + # at all. Every way here refuses elsewhere today; this makes it fail CLOSED by itself + # rather than by coincidence (round 2, finding 5). + true -> + [:ci_evidence_not_read] + end + end + + # Past `ci_wait_limit_seconds/0` a check still running or never reported is refused, naming + # which state it was stuck in, so a human is told rather than the gate waiting for ever. + defp ci_timeout_reasons(facts, result) do + if ci_wait_exceeded?(facts) do + Enum.map(result.pending, &{:required_check_timed_out, &1, :pending}) ++ + Enum.map(result.missing, &{:required_check_timed_out, &1, :missing}) else [] end @@ -1172,7 +1385,16 @@ defmodule Loopctl.Delivery.MergePrecondition do # `trio_outputs` is recorded as having done so, and nothing it sent is read. gate_a_input: GateAInput.for_story(story.tenant_id, story.id), trio_outputs_ignored: Keyword.has_key?(opts, :trio_outputs), - effect_proof: Keyword.get(opts, :effect_proof) + effect_proof: Keyword.get(opts, :effect_proof), + # The source's CURRENT list, read live like the gate triggers (US-45.6). Deliberately + # NOT bound at placement: a list bound to the claim can never be corrected, so renaming + # a CI job stranded every story already placed. A source switched away from `thread` + # with its list cleared makes a thread already placed refuse `required_checks_unset`, + # which names the fix — setting the list again — and is not a dead end. + required_checks: source_required_checks(source), + # When the story entered `ci`: what a CI wait is measured from. + ci_entered_at: if(mode == :thread, do: Stages.entered_at(story.tenant_id, story.id, :ci)), + now: DateTime.utc_now() } # NOT fetched when the decision will not read them: the merged branch and a moved head @@ -1185,10 +1407,36 @@ defmodule Loopctl.Delivery.MergePrecondition do Map.merge(facts, %{ head_files: repo_files(repo, pull_request, :head_sha, skip?), - base_files: repo_files(repo, pull_request, :merge_base_sha, skip?) + base_files: repo_files(repo, pull_request, :merge_base_sha, skip?), + ci_evidence: ci_evidence(facts, pull_request, skip?) }) end + defp source_required_checks({:ok, %{required_checks: checks}}) when is_list(checks), do: checks + defp source_required_checks(_no_source), do: [] + + # US-45.6: the evidence for the CHECKPOINT'S exact commit, never the branch head's or a + # parent's. Read only where a thread is about to be judged: a moved head or a merged + # checkpoint decides without it, and a source requiring nothing is refused without a read. + defp ci_evidence(%{mode: :thread} = facts, {:ok, %{merged?: false}}, false = _skip?) do + names = CiEvidence.lookup_names(facts.required_checks) + + # A repository or checkpoint that could not be read is already refused as itself; only + # the evidence read's OWN failure is this fact's. + with {:ok, repo} <- facts.repo, + {:ok, %{commit_sha: sha}} <- facts.checkpoint, + [_ | _] <- names do + case source().check_evidence(repo, sha) do + {:ok, evidence} -> {:ok, %{evidence: evidence, sha: sha, read_at: DateTime.utc_now()}} + {:error, _reason} = error -> error + end + else + _nothing_to_read -> {:ok, nil} + end + end + + defp ci_evidence(_facts, _pull_request, _skip?), do: {:ok, nil} + defp fetch_story(tenant_id, story_id) do case Stories.get_story(tenant_id, story_id) do {:ok, story} -> {:ok, story} @@ -1342,7 +1590,15 @@ defmodule Loopctl.Delivery.MergePrecondition do # The same shape for the unevaluated backstop: past the bound it stops being "retry" and # becomes a refusal, which escalates through the ordinary path. defp act(tenant_id, story_id, %Verdict{decision: :unevaluated} = verdict, opts) do - case note_unevaluated(tenant_id, story_id, verdict, opts) do + # A pure CI wait is a COMPLETED evaluation — the forge answered — so it ends a run of + # transient faults rather than being skipped over; otherwise faults hours apart across a + # long pipeline accumulated as "consecutive" (round 2, finding 7). + counted = + if counts_toward_unevaluated_bound?(verdict), + do: note_unevaluated(tenant_id, story_id, verdict, opts), + else: clear_unevaluated(tenant_id, story_id, verdict, opts) + + case counted do %Verdict{decision: :unevaluated} = waiting -> waiting %Verdict{} = converted -> act(tenant_id, story_id, converted, opts) end @@ -1366,6 +1622,55 @@ defmodule Loopctl.Delivery.MergePrecondition do |> Keyword.take([:claim_epoch, :actor_label]) |> Keyword.merge(allow_event_data(verdict)) + # The evidence an allow rests on is recorded BEFORE the allow, and an allow whose evidence + # did not land is not an allow: a thread-mode merge is licensed by green CI on the exact + # commit, and the thread must be able to show it (AC-45.6.1). + with :ok <- evidence_for_allow(tenant_id, story_id, verdict) do + write_allow(tenant_id, story_id, verdict, head, write_opts, opts) + end + end + + # A copy that met contention (`:busy`) is a RETRY, as every transient fault in this module + # is: the verdict becomes `:unevaluated` rather than escalating a green change for a lock + # wait (US-45.6 review round 1, finding 4). Any other failure refuses. + defp evidence_for_allow(tenant_id, story_id, verdict), + do: allow_evidence_outcome(verdict, copy_ci_evidence(tenant_id, story_id, verdict)) + + @doc false + # Public so the classification can be tested without contending for a row lock. + @spec allow_evidence_outcome(Verdict.t(), :ok | :superseded | {:error, term()}) :: + :ok | Verdict.t() + def allow_evidence_outcome(verdict, copy_result) do + case copy_result do + :ok -> + :ok + + # A newer read is on the checkpoint (an overlapping evaluation): this allow rests on + # evidence that is no longer the thread's, so it is judged again rather than recorded. + # It is a CI wait like any other — not a fault — so it is not counted toward the + # unevaluated bound and asks again on the CI-wait interval (round 3). + :superseded -> + %{ + verdict + | decision: :unevaluated, + reasons: [{:ci_evidence_superseded, verdict.checkpoint_sha}], + retry_after: @ci_wait_retry_after + } + + {:error, reason} -> + if transient?(reason) do + %{ + verdict + | decision: :unevaluated, + reasons: Enum.uniq(verdict.reasons ++ [{:ci_evidence_not_recorded, reason}]) + } + else + refuse(verdict, [{:ci_evidence_not_recorded, reason}]) + end + end + end + + defp write_allow(tenant_id, story_id, verdict, head, write_opts, opts) do case Stages.record_effect(tenant_id, story_id, :merge_gate_allowed_sha, head, write_opts) do {:ok, _row} -> clear_unevaluated(tenant_id, story_id, verdict, opts) diff --git a/lib/loopctl/delivery/merge_precondition/verdict.ex b/lib/loopctl/delivery/merge_precondition/verdict.ex index 4b8d1368..b02fbc20 100644 --- a/lib/loopctl/delivery/merge_precondition/verdict.ex +++ b/lib/loopctl/delivery/merge_precondition/verdict.ex @@ -11,9 +11,11 @@ defmodule Loopctl.Delivery.MergePrecondition.Verdict do - `:head_moved` — the pull request's head is not the one CI ran on and the story was verified at, so the change goes back to `implementing`. New commits are ordinary; this is not an escalation - - `:unevaluated` — a TRANSIENT forge fault. Nothing was decided and nothing transitions; - the caller retries. Never an escalation, because one network blip must not park a - story until a human acts + - `:unevaluated` — nothing was decided yet, and nothing transitions; the caller retries. + Either a TRANSIENT fault (the forge, or database contention), which one network blip + must not turn into a parked story, or — THREAD mode (US-45.6) — required CI checks + still running or not yet reported on the checkpoint's commit. Only the first counts + toward the consecutive-unevaluated bound; a CI wait is bounded in time instead - `reasons` — every reason the decision is what it is, all of them rather than the first, so one escalation names the whole list. Empty on `:allow` and on an authorised `:already_merged` @@ -26,8 +28,8 @@ defmodule Loopctl.Delivery.MergePrecondition.Verdict do - `recorded_head_sha` — the head the STAGE ROW carries: what CI ran on and the story was verified at. Known even when the forge cannot be reached, which is why the consecutive-unevaluated count is kept per THIS head rather than the forge's - - `retry_after` — on `:unevaluated`, the seconds the FORGE asked a caller to wait, when it - said so at all. The endpoint sends it as `Retry-After`; the dominant cause of an + - `retry_after` — on `:unevaluated`, the seconds the forge asked a caller to wait, when it + said so at all, or loopctl's own 300 for a CI wait. The endpoint sends it as `Retry-After`; the dominant cause of an unevaluated verdict is a rate limit, so an unbounded retry would amplify the very condition it is waiting out - `repo`, `pr_number`, `head_sha`, `merge_base_sha` — what was judged, server-resolved. @@ -39,6 +41,9 @@ defmodule Loopctl.Delivery.MergePrecondition.Verdict do be read (the verdict is then `:unevaluated`) - `checkpoint_id`, `checkpoint_sha` — THREAD mode: the recorded checkpoint judged. An allow in thread mode is recorded naming both + - `ci_evidence` — THREAD mode (US-45.6): what CI said about the checkpoint's exact commit, + as `Loopctl.Delivery.CiEvidence.to_record/5` shapes it, and what `enforce/3` copies onto + the checkpoint's `gate_evidence`. nil when it was not read - `merge_sha` — set only on `:already_merged`: the sha the forge reports for a pull request that was merged before this evaluation ran - `diffstat` — `%{files: n, changed_lines: n}`. In pr mode the forge's own totals, never @@ -78,6 +83,7 @@ defmodule Loopctl.Delivery.MergePrecondition.Verdict do :recorded_head_sha, :checkpoint_id, :checkpoint_sha, + :ci_evidence, mode: :pr, custody: nil, gate_a_inputs: :missing, @@ -105,6 +111,7 @@ defmodule Loopctl.Delivery.MergePrecondition.Verdict do recorded_head_sha: String.t() | nil, mode: :pr | :thread | nil, checkpoint_id: Ecto.UUID.t() | nil, - checkpoint_sha: String.t() | nil + checkpoint_sha: String.t() | nil, + ci_evidence: map() | nil } end diff --git a/lib/loopctl/delivery/pull_request_source.ex b/lib/loopctl/delivery/pull_request_source.ex index 7d1cf5e9..98e9296f 100644 --- a/lib/loopctl/delivery/pull_request_source.ex +++ b/lib/loopctl/delivery/pull_request_source.ex @@ -201,6 +201,16 @@ defmodule Loopctl.Delivery.PullRequestSource do @doc "The three-dot comparison `base...head` (US-45.4). See `t:comparison/0`." @callback compare(repo(), String.t(), String.t()) :: {:ok, comparison()} | {:error, term()} + @doc """ + The CI evidence for ONE commit (US-45.6): EVERY latest check run on it (`filter=latest`), + each with the slug of the App that created it, and every commit status. The required + names are matched by `Loopctl.Delivery.CiEvidence`, never here, so this reads the same + thing whatever a source requires. A list the forge truncated is an error, never a partial + answer: a missing failure reads as a pass. + """ + @callback check_evidence(repo(), String.t()) :: + {:ok, Loopctl.Delivery.CiEvidence.evidence()} | {:error, term()} + @doc "Every file the repository holds at `ref`." @callback repo_files(repo(), String.t()) :: {:ok, [String.t()]} | {:error, term()} diff --git a/lib/loopctl/delivery/stages.ex b/lib/loopctl/delivery/stages.ex index a8b395df..3b55033b 100644 --- a/lib/loopctl/delivery/stages.ex +++ b/lib/loopctl/delivery/stages.ex @@ -308,6 +308,25 @@ defmodule Loopctl.Delivery.Stages do transitions end + @doc """ + When `story_id` last ENTERED `stage` — the newest transition into it — or nil when it never + has (US-45.6: what a merge gate's CI wait is measured from). + """ + @spec entered_at(Ecto.UUID.t(), Ecto.UUID.t(), atom()) :: DateTime.t() | nil + def entered_at(tenant_id, story_id, stage) do + {:ok, at} = + Repo.with_tenant(tenant_id, fn -> + Repo.one( + from e in StageEvent, + where: e.tenant_id == ^tenant_id and e.story_id == ^story_id, + where: e.event == "transitioned" and e.to_stage == ^Atom.to_string(stage), + select: max(e.inserted_at) + ) + end) + + at + end + @doc "A story's stage events, oldest first." @spec list_events(Ecto.UUID.t(), Ecto.UUID.t()) :: [StageEvent.t()] def list_events(tenant_id, story_id) do diff --git a/lib/loopctl/intake.ex b/lib/loopctl/intake.ex index 05daf60d..d1a3ae17 100644 --- a/lib/loopctl/intake.ex +++ b/lib/loopctl/intake.ex @@ -201,7 +201,9 @@ defmodule Loopctl.Intake do defp create_attrs(repo, attrs) do base = %{repo_full_name: repo} - Enum.reduce([base_branch: "base_branch", mode: "mode"], base, fn {key, string_key}, acc -> + fields = [base_branch: "base_branch", mode: "mode", required_checks: "required_checks"] + + Enum.reduce(fields, base, fn {key, string_key}, acc -> case fetch_either(attrs, key, string_key) do {:ok, value} -> Map.put(acc, key, value) :error -> acc @@ -385,7 +387,10 @@ defmodule Loopctl.Intake do | {:error, Ecto.Changeset.t() | :not_found | :nothing_to_update} def update_source(tenant_id, source_id, attrs, opts \\ []) when is_binary(tenant_id) and is_binary(source_id) and is_map(attrs) do - fields = for key <- [:target_epic_id, :base_branch, :mode], Map.has_key?(attrs, key), do: key + fields = + for key <- [:target_epic_id, :base_branch, :mode, :required_checks], + Map.has_key?(attrs, key), + do: key if fields == [] do {:error, :nothing_to_update} @@ -413,11 +418,27 @@ defmodule Loopctl.Intake do changeset = if mode?, do: cast_mode(changeset, Map.get(attrs, :mode)), else: changeset + # Judged over the source AS IT WILL BE whenever either half moves: a thread-mode source + # must name a required check, so switching to `thread` without one, or clearing the list + # on a thread source, is refused whichever field the request named (US-45.6). + changeset = + if mode? or :required_checks in fields, + do: cast_required_checks(changeset, attrs), + else: changeset + with {:ok, changeset} <- valid(changeset) do write_update(tenant_id, changeset, fields, opts) end end + defp cast_required_checks(changeset, attrs) do + changeset + |> Ecto.Changeset.cast(Map.take(attrs, [:required_checks]), [:required_checks], + empty_values: [] + ) + |> Source.validate_required_checks() + end + # `empty_values: []` for the reason `cast_base_branch/2` gives: a caller that SENT a value # gets an answer about it, so an explicit null or `""` is a 422, never a kept old value. defp cast_mode(changeset, value) do @@ -449,7 +470,8 @@ defmodule Loopctl.Intake do with {:ok, source} <- AdminRepo.update(changeset), :ok <- append_if(fields, :target_epic_id, tenant_id, source, opts), :ok <- append_if(fields, :base_branch, tenant_id, source, opts), - :ok <- append_if(fields, :mode, tenant_id, source, opts) do + :ok <- append_if(fields, :mode, tenant_id, source, opts), + :ok <- append_if(fields, :required_checks, tenant_id, source, opts) do source else {:error, reason} -> AdminRepo.rollback(reason) @@ -473,6 +495,9 @@ defmodule Loopctl.Intake do :mode -> {"intake_source_mode_set", %{"mode" => Atom.to_string(source.mode)}} + + :required_checks -> + {"intake_source_required_checks_set", %{"required_checks" => source.required_checks}} end case AuditChain.append(tenant_id, %{ diff --git a/lib/loopctl/intake/source.ex b/lib/loopctl/intake/source.ex index 1613fe5a..b838321e 100644 --- a/lib/loopctl/intake/source.ex +++ b/lib/loopctl/intake/source.ex @@ -42,6 +42,7 @@ defmodule Loopctl.Intake.Source do :repo_full_name, :base_branch, :mode, + :required_checks, :target_epic_id, :revoked_at, :inserted_at, @@ -72,6 +73,13 @@ defmodule Loopctl.Intake.Source do # is the change-thread route, where the gate reads the story's latest RECORDED checkpoint # and no pull request exists. NOT NULL, default `:pr`, so an existing source is unchanged. field :mode, Ecto.Enum, values: [:pr, :thread], default: :pr + + # THE CHECKS A THREAD-MODE CHECKPOINT MUST PASS ON ITS EXACT COMMIT (US-45.6). A pull + # request's required checks are its base branch's protection, enforced by GitHub at merge + # time; a thread has no pull request and the loopctl App pushes the squash itself, so the + # merge gate reads the checks by SHA and this names which ones it requires. Check-run + # names or commit-status contexts, as they appear on the commit. `pr` mode never reads it. + field :required_checks, {:array, :string}, default: [] field :webhook_secret, Loopctl.Vault.Binary, redact: true field :revoked_at, :utc_datetime_usec @@ -104,8 +112,10 @@ defmodule Loopctl.Intake.Source do # By PRESENCE, as the branch is: absent keeps the `:pr` default, and a caller that names # the field gets its value validated rather than a silent substitution. |> cast(attrs, [:mode], empty_values: []) + |> cast(attrs, [:required_checks], empty_values: []) |> validate_required([:repo_full_name]) |> validate_mode() + |> validate_required_checks() |> validate_base_branch() |> validate_format(:repo_full_name, @repo_format, message: "must be owner/name") |> check_constraint(:repo_full_name, name: :intake_sources_repo_shape) @@ -159,6 +169,84 @@ defmodule Loopctl.Intake.Source do |> check_constraint(:mode, name: :intake_sources_mode) end + # The status a session posts about its OWN work (claude-config#677). On the thread path it + # is the implementer attesting itself, the shape chain of custody refuses (PRD §5), so it is + # recorded on the checkpoint and can never be a required check. + @local_gate "local-gate" + @max_required_checks 20 + @max_check_name_bytes 200 + + @doc "The most required checks a source may name." + @spec max_required_checks() :: pos_integer() + def max_required_checks, do: @max_required_checks + + @doc "The longest required check name, in bytes." + @spec max_check_name_bytes() :: pos_integer() + def max_check_name_bytes, do: @max_check_name_bytes + + @doc "The status context a session's own local gate posts; never requirable." + @spec local_gate() :: String.t() + def local_gate, do: @local_gate + + @doc """ + Everything `required_checks` must satisfy, for every path that writes it (US-45.6): + + - a list of at most #{@max_required_checks} distinct, non-blank names of at most + #{@max_check_name_bytes} bytes each + - never `#{@local_gate}`: that status is posted by whoever pushed, so on the thread path it + is the implementer attesting its own work + - NON-EMPTY when the source is `thread`-mode: a thread merges with no pull request and no + forge rule, so a source requiring nothing would merge whatever CI said + + Runs over the source as it WILL BE, so an update naming only `mode` or only + `required_checks` is judged together with the field it did not name. + """ + @spec validate_required_checks(Ecto.Changeset.t()) :: Ecto.Changeset.t() + def validate_required_checks(%Ecto.Changeset{} = changeset) do + changeset + |> validate_required([:required_checks]) + |> validate_length(:required_checks, max: @max_required_checks) + |> validate_change(:required_checks, fn :required_checks, names -> check_names(names) end) + |> validate_thread_requires_checks() + end + + defp check_names(names) do + cond do + # `{:array, :string}` casts a JSON null element to nil, so the element type is checked + # here, before anything calls a String function on it (a 500 otherwise). + not Enum.all?(names, &usable_name?/1) -> + [required_checks: "each name must be 1 to #{@max_check_name_bytes} bytes and not blank"] + + @local_gate in names -> + [ + required_checks: + {"#{@local_gate} is posted by whoever pushed and can never be a required check", + [validation: :local_gate_not_requirable]} + ] + + Enum.uniq(names) != names -> + [required_checks: "names must be distinct"] + + true -> + [] + end + end + + defp usable_name?(name) when is_binary(name), + do: String.trim(name) != "" and byte_size(name) <= @max_check_name_bytes + + defp usable_name?(_not_a_string), do: false + + defp validate_thread_requires_checks(changeset) do + if get_field(changeset, :mode) == :thread and get_field(changeset, :required_checks) == [] do + add_error(changeset, :required_checks, "a thread-mode source must name at least one", + validation: :required_for_thread + ) + else + changeset + end + end + @doc "The modes a source may take." @spec modes() :: [atom()] def modes, do: Ecto.Enum.values(__MODULE__, :mode) diff --git a/lib/loopctl/threads.ex b/lib/loopctl/threads.ex index ed18d134..f6986e10 100644 --- a/lib/loopctl/threads.ex +++ b/lib/loopctl/threads.ex @@ -470,6 +470,91 @@ defmodule Loopctl.Threads do ) end + @doc """ + Copies the merge gate's evidence onto a checkpoint (US-45.6, AC-45.6.1): `record` is stored + under `key` in the checkpoint's `gate_evidence`, replacing that key and leaving every other + one. + + NEVER OVER A NEWER READ (US-45.6 review round 1, finding 6). Two evaluations can overlap, + and the slower one may have read earlier: without a guard it would write a stale "pending" + over the green record an allow was granted on. A record carrying `"read_at"` (ISO 8601 UTC, + which orders as text) replaces only a stored one read strictly earlier; one read no later + than what is stored changes nothing and is answered `:superseded`, so a caller about to act + on it knows it did not land. One identical to what is stored, `read_at` aside, is `:ok` + with no write. + + Not a thread WRITE in the claimant's sense — the gate, not a principal, records what the + forge said about a commit — so it takes no story lock and no claim fence. Its lock wait is + bounded and contention is `{:error, :busy}`, counted as `[:loopctl, :threads, :busy]`. + `{:error, :not_found}` when the story has no such checkpoint. + """ + @spec record_gate_evidence(Ecto.UUID.t(), Ecto.UUID.t(), Ecto.UUID.t(), String.t(), map()) :: + :ok | :superseded | {:error, :not_found | term()} + def record_gate_evidence(tenant_id, story_id, checkpoint_id, key, record) + when is_binary(key) and is_map(record) do + Stages.answering_busy(tenant_id, [:loopctl, :threads, :busy], "gate evidence write", fn -> + tenant_id + |> Repo.with_tenant(fn -> + write_gate_evidence(tenant_id, story_id, checkpoint_id, key, record) + end) + |> case do + {:ok, answer} -> answer + {:error, _reason} = error -> error + end + end) + end + + defp write_gate_evidence(tenant_id, story_id, checkpoint_id, key, record) do + Capacity.set_lock_timeout!(Repo) + + stored = + Repo.one( + from c in Checkpoint, + where: c.id == ^checkpoint_id and c.tenant_id == ^tenant_id, + where: c.story_id == ^story_id, + lock: "FOR UPDATE", + select: %{present: true, record: fragment("?->?", c.gate_evidence, ^key)} + ) + + case evidence_write(stored, record) do + :write -> + {1, _} = + from(c in Checkpoint, where: c.id == ^checkpoint_id) + |> update([c], + set: [gate_evidence: fragment("? || ?", c.gate_evidence, ^%{key => record})] + ) + |> Repo.update_all([]) + + :ok + + answer -> + answer + end + end + + # The decision, taken under the row lock so two evaluations cannot both think they are the + # newer one. A record read no LATER than the stored one is `:superseded`, never `:ok`: the + # allow path must not record an allow on evidence that did not land (round 2, finding 4). + # One that says exactly what is stored already, read_at aside, is `:ok` with no write, so a + # CI wait polled for hours rewrites the row only when the judgement moves (finding 6). + defp evidence_write(nil, _record), do: {:error, :not_found} + defp evidence_write(%{record: nil}, _record), do: :write + + defp evidence_write(%{record: stored}, record) do + cond do + Map.delete(stored, "read_at") == Map.delete(record, "read_at") -> :ok + not later?(Map.get(record, "read_at"), Map.get(stored, "read_at")) -> :superseded + true -> :write + end + end + + # ISO 8601 UTC with fixed-width fractions orders as text (`CiEvidence.to_record/5`). A record + # with no `read_at` makes no ordering claim and is written. + defp later?(nil, _stored), do: true + defp later?(_read_at, nil), do: true + defp later?(read_at, stored) when is_binary(read_at) and is_binary(stored), do: read_at > stored + defp later?(_read_at, _stored), do: true + defp locked_story(tenant_id, story_id), do: Repo.one(story_query(tenant_id, story_id) |> lock("FOR SHARE")) diff --git a/lib/loopctl_web/controllers/intake_source_controller.ex b/lib/loopctl_web/controllers/intake_source_controller.ex index 04c6159a..c1b3ee6c 100644 --- a/lib/loopctl_web/controllers/intake_source_controller.ex +++ b/lib/loopctl_web/controllers/intake_source_controller.ex @@ -30,6 +30,21 @@ defmodule LoopctlWeb.IntakeSourceController do tags(["Intake"]) + @required_checks_doc "The CI checks a THREAD-mode checkpoint must pass on its exact commit before " <> + "the merge gate allows it (US-45.6): GitHub Actions check-run names as " <> + "they appear on the commit. A `thread` source must name at " <> + "least one (422 otherwise, whichever of `mode` and `required_checks` " <> + "the request named); `pr` mode never reads it. At most " <> + "#{Source.max_required_checks()} distinct, non-blank names of at most " <> + "#{Source.max_check_name_bytes()} bytes. Only a GitHub Actions check " <> + "run satisfies one; a commit status is recorded, never trusted. Each " <> + "required job must run on every push to the thread branches (no path " <> + "filter or job-level `if:`): one that never appears is refused after " <> + "the merge gate's CI wait. " <> + "`local-gate` is refused (422): " <> + "that status is posted by whoever pushed, so it is recorded on the " <> + "checkpoint and can never satisfy a required check." + @source_schema %Schema{ type: :object, required: [ @@ -38,6 +53,7 @@ defmodule LoopctlWeb.IntakeSourceController do :repo_full_name, :base_branch, :mode, + :required_checks, :target_epic_id, :revoked_at, :inserted_at @@ -67,6 +83,11 @@ defmodule LoopctlWeb.IntakeSourceController do "The branch every dispatch for this repository is cut FROM. `master` unless the " <> "source named or was repointed to another." }, + required_checks: %Schema{ + type: :array, + items: %Schema{type: :string}, + description: @required_checks_doc + }, mode: %Schema{ type: :string, enum: ["pr", "thread"], @@ -110,6 +131,16 @@ defmodule LoopctlWeb.IntakeSourceController do description: "The repository, e.g. `mkreyman/home_care_billing`." }, project_id: %Schema{type: :string, format: :uuid}, + required_checks: %Schema{ + type: :array, + items: %Schema{ + type: :string, + minLength: 1, + maxLength: Source.max_check_name_bytes() + }, + maxItems: Source.max_required_checks(), + description: "Optional. " <> @required_checks_doc + }, mode: %Schema{ type: :string, enum: ["pr", "thread"], @@ -227,6 +258,16 @@ defmodule LoopctlWeb.IntakeSourceController do %Schema{ type: :object, properties: %{ + required_checks: %Schema{ + type: :array, + items: %Schema{ + type: :string, + minLength: 1, + maxLength: Source.max_check_name_bytes() + }, + maxItems: Source.max_required_checks(), + description: "Optional. " <> @required_checks_doc + }, mode: %Schema{ type: :string, enum: ["pr", "thread"], @@ -305,6 +346,7 @@ defmodule LoopctlWeb.IntakeSourceController do } |> put_if_present(params, "base_branch", :base_branch) |> put_if_present(params, "mode", :mode) + |> put_if_present(params, "required_checks", :required_checks) with {:ok, %{source: source, webhook_secret: secret}} <- Intake.create_source(tenant.id, attrs, actor_lineage: actor_lineage(conn)) do @@ -337,6 +379,7 @@ defmodule LoopctlWeb.IntakeSourceController do |> put_if_present(params, "target_epic_id", :target_epic_id) |> put_if_present(params, "base_branch", :base_branch) |> put_if_present(params, "mode", :mode) + |> put_if_present(params, "required_checks", :required_checks) case Intake.update_source(tenant.id, source_id, attrs, actor_lineage: actor_lineage(conn)) do {:ok, source} -> diff --git a/lib/loopctl_web/controllers/merge_precondition_controller.ex b/lib/loopctl_web/controllers/merge_precondition_controller.ex index 10b96c9d..3a7f2b92 100644 --- a/lib/loopctl_web/controllers/merge_precondition_controller.ex +++ b/lib/loopctl_web/controllers/merge_precondition_controller.ex @@ -155,6 +155,17 @@ defmodule LoopctlWeb.MergePreconditionController do "thread-mode allow records it, and the merge executor merges only while the " <> "base head still equals it, and otherwise takes its base-update path." }, + ci_evidence: %OpenApiSpex.Schema{ + type: :object, + nullable: true, + description: + "Thread mode (US-45.6): what CI said about the checkpoint's EXACT commit, read " <> + "from both the check-runs and the commit-status APIs — `sha`, `read_at`, " <> + "`required`, the raw `check_runs` and `statuses`, `local_gate` (recorded, " <> + "never counted), and the judgement `passed` / `pending` / `missing` / " <> + "`failed`. The same object is copied onto the checkpoint's `gate_evidence` " <> + "under `ci`. null when it was not read (pr mode, a moved or merged head)." + }, merge_sha: %OpenApiSpex.Schema{ type: :string, nullable: true, @@ -251,7 +262,22 @@ defmodule LoopctlWeb.MergePreconditionController do "refused `claim_ended`; a thread loopctl could not read is refused " <> "`thread_unreadable`. It also refuses `empty_change` (the checkpoint's tree equals " <> "the base branch's, or no file changed) and `checkpoint_tree_mismatch` (the forge's " <> - "tree for it is not the one recorded). The BRANCH is judged first: a branch missing " <> + "tree for it is not the one recorded). CI is read by the checkpoint's EXACT SHA " <> + "(US-45.6), from both the check-runs and the commit-status APIs (only a GitHub " <> + "Actions check run satisfies a required check; statuses are recorded, never " <> + "trusted), against the " <> + "source's `required_checks`: a failed one refuses `required_check_failed`, one still " <> + "running or not yet reported is `unevaluated` (`required_check_pending` / " <> + "`required_check_missing`, `Retry-After` 300; neither counts toward the unevaluated " <> + "bound, and 24 hours after the story entered `ci` both are refused " <> + "`required_check_timed_out`; per name the latest run of each check suite counts and " <> + "every suite must pass), the required checks are the source's current list, a " <> + "checkpoint changing `.github/workflows/` or `.github/actions/` is refused " <> + "`ci_definition_changed`, a source requiring none refuses `required_checks_unset`, a " <> + "failed read is `ci_evidence_unavailable`, and a `local-gate` status is recorded but " <> + "never satisfies a required check. `ci_evidence` is what was read; it is copied onto " <> + "the checkpoint, and an allow whose copy did not land is refused " <> + "`ci_evidence_not_recorded`. The BRANCH is judged first: a branch missing " <> "from a readable repository (`branch_missing`), one naming a commit nobody recorded " <> "(`branch_head_unrecorded`), one naming an EARLIER checkpoint of the claim " <> "(`branch_head_regressed`), a checkpoint that is not the head the stage row " <> @@ -446,6 +472,7 @@ defmodule LoopctlWeb.MergePreconditionController do recorded_head_sha: verdict.recorded_head_sha, merge_base_sha: verdict.merge_base_sha, base_sha: thread_base_sha(verdict), + ci_evidence: verdict.ci_evidence, merge_sha: verdict.merge_sha, diffstat: verdict.diffstat, hard_bound: MergePrecondition.hard_bound(), diff --git a/mcp-server/CHANGELOG.md b/mcp-server/CHANGELOG.md index dbfa144b..8c81b685 100644 --- a/mcp-server/CHANGELOG.md +++ b/mcp-server/CHANGELOG.md @@ -5,6 +5,22 @@ 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) +## 2.108.0 — 2026-09-27 (CI evidence by exact SHA) + +### Changed + +- **`intake_source_enroll`, `intake_source_update`** take `required_checks` (loopctl US-45.6): + the CI checks a thread-mode checkpoint must pass on its exact commit. A thread source must + name at least one; `local-gate` and malformed lists are refused locally. The source row in a + result now carries `required_checks`. +- **`merge_precondition`** says how CI is judged on a thread checkpoint and names the new + reasons: `required_check_failed`, `required_check_pending`, `required_check_missing`, + `required_checks_unset`, `required_check_timed_out`, `ci_evidence_unavailable`, + `ci_evidence_not_recorded`. A CI wait never counts toward the unevaluated bound and is + refused 24 hours after the story entered ci; a checkpoint changing CI definitions is + refused `ci_definition_changed`. Only a GitHub Actions check run + satisfies a required check; a commit status is recorded, never trusted. + ## 2.107.0 — 2026-09-26 (review on a change thread) ### Added diff --git a/mcp-server/README.md b/mcp-server/README.md index 6399a58a..e49eac64 100644 --- a/mcp-server/README.md +++ b/mcp-server/README.md @@ -524,10 +524,10 @@ The order to wire it up, the stage machine these tools move a story through, and | `thread_review_get` | **Read a review's payload** (`GET /api/v1/stories/:id/thread/reviews/:review_id`, any role): the story, the checkpoint it reads with its diff reference, the thread's latest entries, the latest fixes with the findings each answers (`fixes_truncated` when older ones exist), and the rounds. Bodies and locations are untrusted. | | `thread_fix` | **Record a fix on the story you hold** (`POST /api/v1/stories/:id/thread/fixes`), on the key `claim_story` claims with: the checkpoint carrying it (one your current claim recorded after every checkpoint its findings were found in) and the findings of completed rounds it answers. Refusals: 409 `not_claimant`, `stale_claim_epoch`, `claim_not_live`, `idempotency_key_reused`; 422 `fix_checkpoint_required`, `fix_checkpoint_not_current_claim`, `fix_checkpoint_not_after_findings`, `finding_ids_required`, `unknown_finding`, `secret_blocked`; 503 `tenant_halted`. | | `force_unclaim_story` | **Take a story back from the agent holding it** (`POST /api/v1/stories/:id/force-unclaim`). Resets it to `agent_status: pending` with `assigned_agent_id` cleared AND makes the delivery stage row follow the release back to `queued`. **A delivery story then goes to `escalated`, not back to the queue** (loopctl US-44.4): an operator taking a story back is a human decision, so a release that leaves the stage row at `queued` escalates it over `operator_released` in the same transaction, spending no attempt against the retry ceiling — it is not left in the queue behind the human's back. Put it back to work with `resolve_escalation` `to: queued`, which releases (a no-op by then) AND re-contracts. A story with no delivery stage row is simply left `pending`. A story parked at `claimed` with nobody on it is the residue of a compensation that did not complete, not what a refused dispatch normally leaves: placement releases the claim inline when a runner refuses, and the claim lease releases it unattended once `claimed_until` passes — reach for this to get it back now, or when both of those left it held. Requeues from any stage a claim holds; a stage no claim holds keeps its stage and is rebound to the new epoch; `done` and `failed` are untouched. For an ESCALATED story use `resolve_escalation` instead — that one releases AND re-contracts, so its `queued` really is placeable. Requires an orchestrator-ROLE key: the action is `exact_role: :orchestrator`, so a user or superadmin key is 403'd like any other non-member. Put it in `LOOPCTL_ORCH_KEY` (which is then the key sent, and a global `LOOPCTL_API_KEY` does not displace it) or, with no orchestrator key set, in `LOOPCTL_API_KEY`. The orchestrator key must be linked to a registered agent (400 otherwise) and the tenant must be human-anchored (403 `custody_tier_required` otherwise). Does not touch `verified_status`. Run again on an already-pending story it is still an operator's release: a row stranded behind the story's claim epoch is rebound, an in-flight row is requeued, and a row then at `queued` — including one already sitting there — is escalated over `operator_released`, as on the first run; a row already escalated, or anywhere else at that epoch, is left alone. Every 500 means the whole release rolled back and the story is still claimed: `audit_chain_append_failed` (the escalation's chain entry was refused) is a server-side condition a re-run meets again until an operator acts; `force_unclaim_failed` (the server log names the step) — call it again. A 422 means the release write itself was rejected. No request body. Required: `story_id` (refused locally if it is not a UUID). | -| `merge_precondition` | **Run the merge gate over a story's real pull request** (`POST /api/v1/stories/:id/merge-precondition`). Both delivery gates run again over the diff that exists, plus custody, the 12-file / 1000-line hard bound, the head-has-not-moved check and the self-deploy exclusion; the story must be at stage `ci`. **Gate A reads what triage persisted, never the caller**: the lens verdicts recorded with the story's most recent triage, or a human's re-queue of a Gate A escalation — `gate_a_inputs` on the answer says which, and `missing` refuses with `gate_a_inputs_missing`. This tool sends no trio. Decisions: `allow` (recorded against the head; the only thing that licenses a merge), `refuse` (the story is escalated before this returns), `already_merged`, `head_moved`, `unevaluated` (503 with `Retry-After`). **Thread mode** (the current claim was placed under a source with `mode: thread` — the mode is bound to the dispatch at placement): no pull request; the gate judges the latest checkpoint recorded under the story's CURRENT claim, on the branch that claim's dispatch ran on, and adds refusals `empty_change`, `checkpoint_tree_mismatch`, `no_checkpoint_recorded` and `claim_ended` (the claim was released after an earlier one recorded checkpoints); the source's runners must be at contract 1.20.0+ sending checkpoint messages, or every story is refused `no_checkpoint_recorded`; the branch is judged first, and a missing branch in a readable repository (`branch_missing`), a head nobody recorded (`branch_head_unrecorded`), an earlier checkpoint (`branch_head_regressed`), or a head the stage row did not record is `head_moved` back to implementing while the claim is LIVE, and a refusal naming `claim_not_live` (escalated) when it is not; an unreadable repository refuses `pull_request_unavailable`; the diff judged is the three-dot diff against the merge base, so a base that moved on is not a refusal, and a checkpoint the base already contains (its branch deleted or not) is `already_merged` only under a recorded allow naming it, otherwise `checkpoint_on_base_without_allow`; a thread loopctl could not read refuses `thread_unreadable`; the answer carries `mode` (bound at placement, `pr` when the claim has no accepted dispatch whatever the source says now, null only on ledger contention) and `base_sha` (that merge base), an allow records it, and the merge executor merges only while the base head still equals it. Requires an orchestrator- or user-ROLE key (`exact_role: [:orchestrator, :user]`, so an agent key is 403'd); `LOOPCTL_ORCH_KEY` is sent when set, else `LOOPCTL_API_KEY`. 403 `custody_tier_required` without a human anchor, 404 for an unknown story, 422 when the story is not at `ci`. Required: `story_id` (refused locally if not a UUID) and `claim_epoch` (a non-negative integer, refused locally otherwise); optional `effect_proof`. | -| `intake_source_enroll` | **Bind a GitHub repository to a work project and mint its webhook secret** (`POST /api/v1/intake/sources`) — the step that gives the delivery loop an input, and the row `place_dispatch` reads `repo` and `base_branch` from (its 409 `no_intake_source` means this has not been done; 409 `ambiguous_intake_source` means it has been done twice). **The secret is returned ONCE and can never be read again** — the column is encrypted at rest and redacted on the schema, so no other tool carries it and losing it costs a revoke, a re-enrolment and a reconfigured GitHub webhook. **This tool does not return it either:** a tool result lands in the transcript and the audit log, so it is written to the required `secret_file` with mode 0600 (the handling `runner_enroll` gives a runner credential) and the result carries the source row, the `webhook_url` and that path. The path is reserved before the request, so an existing file or an unwritable directory is refused with nothing enrolled, and any outcome that is not a clean creation withholds the response body — an unparsed 2xx body IS the secret. A 2xx carrying the source and the secret but no webhook path is RECOVERED rather than discarded (the path is derived from the source id, flagged `webhook_url_derived`), and an outcome that proves a source id but no usable secret REVOKES that source rather than leaving it holding the repository's unique slot. Then configure GitHub: Payload URL = the returned `webhook_url`, content type `application/json`, Secret = the contents of the file, and the **Issues** event only — `gh api repos/OWNER/REPO/hooks -f name=web -f config[url]= -f config[content_type]=json -f config[secret]="$(cat )" -f 'events[]=issues'`. Every later delivery failure is the SAME 401 `invalid_signature` — unknown or revoked source, suspended tenant, missing or wrong signature, or a payload whose `repository.full_name` does not match — so the webhook's Recent Deliveries tab cannot tell you which. `base_branch` is set HERE: it defaults to `master` when the body does not name it, so name `main` for a repository created on GitHub since 2020 or every dispatch is cut from a trunk that does not exist. It is not nullable (a dispatch must name a branch), so null or blank is refused rather than falling back to the default, and `intake_source_update` changes it afterwards. Refusals: 403 `api_key_mint_forbidden` if your key was minted by a dispatch (this mints a credential belonging to no lineage, so only an unlineaged operator key may), 403 `custody_tier_required` on an agent-rooted tenant, 422 for a repository that is not `owner/name`, an ACTIVE source already binding it, a project that is missing / archived / not a work project, or a `target_epic_id` outside that project. `mode` picks the merge route: `pr` (the default when omitted) or `thread`, where the merge gate evaluates the story's latest recorded checkpoint instead of a pull request — which needs the source's runners at contract 1.20.0+ sending checkpoint messages, or every story is refused `no_checkpoint_recorded`; any other value, null included, is refused. The mode is bound to each implement dispatch when it is placed, so it decides only what stories placed afterwards get; a change is always allowed. Required: `repo_full_name`, `project_id`, `secret_file`. Optional: `base_branch`, `mode`, and `target_epic_id` — **omitting the epic is not a neutral default**: with no epic every triaged report is escalated to a human and every record stays `pending_triage` until `intake_source_update` names one. Requires `LOOPCTL_USER_KEY`. | +| `merge_precondition` | **Run the merge gate over a story's real pull request** (`POST /api/v1/stories/:id/merge-precondition`). Both delivery gates run again over the diff that exists, plus custody, the 12-file / 1000-line hard bound, the head-has-not-moved check and the self-deploy exclusion; the story must be at stage `ci`. **Gate A reads what triage persisted, never the caller**: the lens verdicts recorded with the story's most recent triage, or a human's re-queue of a Gate A escalation — `gate_a_inputs` on the answer says which, and `missing` refuses with `gate_a_inputs_missing`. This tool sends no trio. Decisions: `allow` (recorded against the head; the only thing that licenses a merge), `refuse` (the story is escalated before this returns), `already_merged`, `head_moved`, `unevaluated` (503 with `Retry-After`). **Thread mode** (the current claim was placed under a source with `mode: thread` — the mode is bound to the dispatch at placement): no pull request; the gate judges the latest checkpoint recorded under the story's CURRENT claim, on the branch that claim's dispatch ran on, and adds refusals `empty_change`, `checkpoint_tree_mismatch`, `no_checkpoint_recorded` and `claim_ended` (the claim was released after an earlier one recorded checkpoints); the source's runners must be at contract 1.20.0+ sending checkpoint messages, or every story is refused `no_checkpoint_recorded`; the branch is judged first, and a missing branch in a readable repository (`branch_missing`), a head nobody recorded (`branch_head_unrecorded`), an earlier checkpoint (`branch_head_regressed`), or a head the stage row did not record is `head_moved` back to implementing while the claim is LIVE, and a refusal naming `claim_not_live` (escalated) when it is not; an unreadable repository refuses `pull_request_unavailable`; the diff judged is the three-dot diff against the merge base, so a base that moved on is not a refusal, and a checkpoint the base already contains (its branch deleted or not) is `already_merged` only under a recorded allow naming it, otherwise `checkpoint_on_base_without_allow`; a thread loopctl could not read refuses `thread_unreadable`; CI is read by the checkpoint's exact SHA from both the check-runs and the commit-status APIs (only a GitHub Actions check run satisfies a required check; statuses are recorded, never trusted) against the source's `required_checks` — a failed one refuses `required_check_failed`, one still running or not yet reported is `unevaluated` (`required_check_pending` / `required_check_missing`, retry after 300s; neither counts toward the unevaluated bound, and 24 hours after the story entered `ci` both are refused `required_check_timed_out`; per name the latest run of each check suite counts and every suite must pass; a checkpoint changing `.github/workflows/` or `.github/actions/` is refused `ci_definition_changed`), a source requiring none refuses `required_checks_unset`, a failed read is `ci_evidence_unavailable`, and `local-gate` is recorded but never counted; the answer's `ci_evidence` is what was read and is copied onto the checkpoint; the answer carries `mode` (bound at placement, `pr` when the claim has no accepted dispatch whatever the source says now, null only on ledger contention) and `base_sha` (that merge base), an allow records it, and the merge executor merges only while the base head still equals it. Requires an orchestrator- or user-ROLE key (`exact_role: [:orchestrator, :user]`, so an agent key is 403'd); `LOOPCTL_ORCH_KEY` is sent when set, else `LOOPCTL_API_KEY`. 403 `custody_tier_required` without a human anchor, 404 for an unknown story, 422 when the story is not at `ci`. Required: `story_id` (refused locally if not a UUID) and `claim_epoch` (a non-negative integer, refused locally otherwise); optional `effect_proof`. | +| `intake_source_enroll` | **Bind a GitHub repository to a work project and mint its webhook secret** (`POST /api/v1/intake/sources`) — the step that gives the delivery loop an input, and the row `place_dispatch` reads `repo` and `base_branch` from (its 409 `no_intake_source` means this has not been done; 409 `ambiguous_intake_source` means it has been done twice). **The secret is returned ONCE and can never be read again** — the column is encrypted at rest and redacted on the schema, so no other tool carries it and losing it costs a revoke, a re-enrolment and a reconfigured GitHub webhook. **This tool does not return it either:** a tool result lands in the transcript and the audit log, so it is written to the required `secret_file` with mode 0600 (the handling `runner_enroll` gives a runner credential) and the result carries the source row, the `webhook_url` and that path. The path is reserved before the request, so an existing file or an unwritable directory is refused with nothing enrolled, and any outcome that is not a clean creation withholds the response body — an unparsed 2xx body IS the secret. A 2xx carrying the source and the secret but no webhook path is RECOVERED rather than discarded (the path is derived from the source id, flagged `webhook_url_derived`), and an outcome that proves a source id but no usable secret REVOKES that source rather than leaving it holding the repository's unique slot. Then configure GitHub: Payload URL = the returned `webhook_url`, content type `application/json`, Secret = the contents of the file, and the **Issues** event only — `gh api repos/OWNER/REPO/hooks -f name=web -f config[url]= -f config[content_type]=json -f config[secret]="$(cat )" -f 'events[]=issues'`. Every later delivery failure is the SAME 401 `invalid_signature` — unknown or revoked source, suspended tenant, missing or wrong signature, or a payload whose `repository.full_name` does not match — so the webhook's Recent Deliveries tab cannot tell you which. `base_branch` is set HERE: it defaults to `master` when the body does not name it, so name `main` for a repository created on GitHub since 2020 or every dispatch is cut from a trunk that does not exist. It is not nullable (a dispatch must name a branch), so null or blank is refused rather than falling back to the default, and `intake_source_update` changes it afterwards. Refusals: 403 `api_key_mint_forbidden` if your key was minted by a dispatch (this mints a credential belonging to no lineage, so only an unlineaged operator key may), 403 `custody_tier_required` on an agent-rooted tenant, 422 for a repository that is not `owner/name`, an ACTIVE source already binding it, a project that is missing / archived / not a work project, or a `target_epic_id` outside that project. `mode` picks the merge route: `pr` (the default when omitted) or `thread`, where the merge gate evaluates the story's latest recorded checkpoint instead of a pull request — which needs the source's runners at contract 1.20.0+ sending checkpoint messages, or every story is refused `no_checkpoint_recorded`; any other value, null included, is refused. The mode is bound to each implement dispatch when it is placed, so it decides only what stories placed afterwards get; a change is always allowed. A `thread` source must also name `required_checks` (422 otherwise): the CI checks the merge gate requires on a checkpoint's exact commit, GitHub Actions check-run names, bounded in number and length by the server; `local-gate` is refused because whoever pushed posts it. Required: `repo_full_name`, `project_id`, `secret_file`. Optional: `base_branch`, `mode`, `required_checks`, and `target_epic_id` — **omitting the epic is not a neutral default**: with no epic every triaged report is escalated to a human and every record stays `pending_triage` until `intake_source_update` names one. Requires `LOOPCTL_USER_KEY`. | | `intake_source_list` | List the tenant's intake sources (`GET /api/v1/intake/sources`): `id` (the webhook URL is `/api/v1/intake/github/`), `project_id`, `repo_full_name`, `base_branch`, `mode` (`pr` or `thread`), `target_epic_id`, `revoked_at`, timestamps. **The webhook secret is not here and is not anywhere** — it is returned once by `intake_source_enroll`, so this is not how to recover one. Use it to find the source behind a 409 `no_intake_source` or `ambiguous_intake_source`, to check a repository's `base_branch` before placing work, and to identify a source left behind by an enrolment whose outcome was unknown. Optional: `include_revoked`. Requires `LOOPCTL_USER_KEY`; the only one of the four that does not also need a human-anchored tenant. | -| `intake_source_update` | **Set where a source's work lands** (`PATCH /api/v1/intake/sources/:id`): `target_epic_id`, the epic triaged stories are created in, `base_branch`, the branch every dispatch for this repository is cut from, and `mode` (`pr` or `thread`), whether the merge gate reads a pull request or the story's latest recorded thread checkpoint. Presence decides — a field you do not name is left exactly as it was, and naming none of them is 422 `nothing_to_update`. `mode` has no cleared state either, and a null or unknown value is refused locally; A change is always allowed and affects only stories placed afterwards: each implement dispatch records the mode, and the base branch, it was placed under, and the merge gate reads those. **Do not send `target_epic_id: null` to mean "not changing this":** an explicit null is the only way to CLEAR the epic, which returns the source to escalating every report instead of filing a story. `base_branch` has no cleared state and a null there is refused locally. This is how a source already pointed at the wrong trunk is corrected — `intake_source_enroll` takes `base_branch` itself, so a `main` repository no longer needs a second call — and the remedy for a source with no epic — every record from it stays `pending_triage` and is retried until one is named, then they promote on the next run with nothing lost. Revoked sources are 404, not a no-op: revoking clears the target so the epic can be deleted, and repointing one would put that block back. 422 when the epic is not in this source's project. Required: `source_id` (refused locally if it is not a UUID — the server answers the same 404 for malformed and unknown). Requires `LOOPCTL_USER_KEY` and a human-anchored tenant. | +| `intake_source_update` | **Set where a source's work lands** (`PATCH /api/v1/intake/sources/:id`): `target_epic_id`, the epic triaged stories are created in, `base_branch`, the branch every dispatch for this repository is cut from, `mode` (`pr` or `thread`), whether the merge gate reads a pull request or the story's latest recorded thread checkpoint, and `required_checks`, the CI checks a thread checkpoint must pass on its exact commit (a thread source must name at least one, judged over the source as it will be, so switching to `thread` without them or clearing them on a thread source is 422; `local-gate` is refused). Presence decides — a field you do not name is left exactly as it was, and naming none of them is 422 `nothing_to_update`. `mode` has no cleared state either, and a null or unknown value is refused locally; A change is always allowed and affects only stories placed afterwards: each implement dispatch records the mode, and the base branch, it was placed under, and the merge gate reads those. **Do not send `target_epic_id: null` to mean "not changing this":** an explicit null is the only way to CLEAR the epic, which returns the source to escalating every report instead of filing a story. `base_branch` has no cleared state and a null there is refused locally. This is how a source already pointed at the wrong trunk is corrected — `intake_source_enroll` takes `base_branch` itself, so a `main` repository no longer needs a second call — and the remedy for a source with no epic — every record from it stays `pending_triage` and is retried until one is named, then they promote on the next run with nothing lost. Revoked sources are 404, not a no-op: revoking clears the target so the epic can be deleted, and repointing one would put that block back. 422 when the epic is not in this source's project. Required: `source_id` (refused locally if it is not a UUID — the server answers the same 404 for malformed and unknown). Requires `LOOPCTL_USER_KEY` and a human-anchored tenant. | | `intake_source_revoke` | **Revoke an intake source** (`DELETE /api/v1/intake/sources/:id`). The verb is DELETE and the action is a revoke: `revoked_at` is stamped, the row is kept, and `intake_source_list` still returns it with `include_revoked`; nothing already received is discarded. Afterwards every delivery to that URL is refused 401 `invalid_signature` exactly as a wrong secret is, so the GitHub webhook does not know it has been cut off and should be deleted on the repository too. Idempotent — a second revoke answers 200 with the original `revoked_at`. It frees the repository's uniqueness slot (the index is partial on not-yet-revoked), so a source with a lost secret, a wrong project or a typo'd repository is corrected by revoking and enrolling again; and it clears `target_epic_id`, which is what makes that epic deletable and is the remedy an epic-delete refusal names. Required: `source_id` (refused locally if it is not a UUID). Requires `LOOPCTL_USER_KEY` and a human-anchored tenant. | ### Dispatch & Chain of Custody (v2) Tools diff --git a/mcp-server/index.js b/mcp-server/index.js index 865f441b..aa4ef09d 100755 --- a/mcp-server/index.js +++ b/mcp-server/index.js @@ -8503,6 +8503,18 @@ const TOOLS = [ "`thread_unreadable`. It adds refusals `empty_change` (the " + "checkpoint's tree equals the base's, or no file changed) and " + "`checkpoint_tree_mismatch` (the forge's tree is not the one the claimant recorded). " + + "CI IS READ BY THE CHECKPOINT'S EXACT SHA, from both the check-runs and the " + + "commit-status APIs, against the source's `required_checks`; only a GitHub Actions " + + "check run satisfies one (a status is recorded, never trusted): a failed one refuses " + + "`required_check_failed`, one still running or not yet reported answers `unevaluated` " + + "(`required_check_pending` / `required_check_missing`, retry after 300s; neither counts " + + "toward the unevaluated bound, and 24 hours after the story entered ci both are " + + "refused `required_check_timed_out`; per name the latest run of each check suite " + + "counts and every suite must pass), a checkpoint changing `.github/workflows/` or " + + "`.github/actions/` is refused `ci_definition_changed`, a source requiring none refuses " + + "`required_checks_unset`, and an evidence read that failed is `ci_evidence_unavailable`. " + + "`local-gate` is recorded, never counted. The answer's `ci_evidence` is what was read, " + + "and it is copied onto the checkpoint. " + "The branch is judged first: a branch missing from a readable repository " + "(`branch_missing`), one naming a commit nobody recorded (`branch_head_unrecorded`), " + "one naming an earlier checkpoint of the claim (`branch_head_regressed`), or a " + @@ -8659,7 +8671,8 @@ const TOOLS = [ "other than those two, null included, is refused. " + "The mode is BOUND to each implement dispatch when it is placed: a change affects " + "only stories placed afterwards, and a story already placed keeps the mode its " + - "dispatch recorded, so a change is always allowed.", + "dispatch recorded, so a change is always allowed. A `thread` source must name " + + "`required_checks`, the CI checks the merge gate requires on a checkpoint's exact commit.", inputSchema: { type: "object", properties: { @@ -8708,6 +8721,16 @@ const TOOLS = [ "`no_checkpoint_recorded`. Not nullable. Changed later with " + "intake_source_update.", }, + required_checks: { + type: "array", + items: { type: "string" }, + description: + "The CI checks a THREAD-mode checkpoint must pass on its exact commit before the " + + "merge gate allows it: GitHub Actions check-run names as they appear " + + "on the commit. A `thread` source must name at least one (422 otherwise); `pr` " + + "mode never reads it. Distinct, non-blank names, bounded in number and length by the server (422 past them). " + + "`local-gate` is refused: whoever pushed posts it, so it is only recorded.", + }, secret_file: { type: "string", description: @@ -8805,7 +8828,18 @@ const TOOLS = [ "`pr` or `thread`: whether the merge gate reads a pull request or the story's " + "latest recorded thread checkpoint. `thread` needs runners at contract 1.20.0 or " + "later sending checkpoint messages, or every story is refused " + - "`no_checkpoint_recorded`. Not nullable. Omit to leave the current value alone.", + "`no_checkpoint_recorded`. Not nullable. Omit to leave the current value alone. " + + "Switching to `thread` needs `required_checks` on the source (send both).", + }, + required_checks: { + type: "array", + items: { type: "string" }, + description: + "The CI checks a THREAD-mode checkpoint must pass on its exact commit before the " + + "merge gate allows it: GitHub Actions check-run names as they appear " + + "on the commit. A `thread` source must name at least one (422 otherwise); `pr` " + + "mode never reads it. Distinct, non-blank names, bounded in number and length by the server (422 past them). " + + "`local-gate` is refused. Omit to leave the current list alone.", }, }, required: ["source_id"], diff --git a/mcp-server/lib/intake-sources.js b/mcp-server/lib/intake-sources.js index f76887ce..6c60fb8f 100644 --- a/mcp-server/lib/intake-sources.js +++ b/mcp-server/lib/intake-sources.js @@ -161,6 +161,34 @@ export function modeRefusal(mode) { ); } +/** + * The CI checks a thread-mode checkpoint must pass on its exact commit (US-45.6). A list of + * distinct, non-blank names; `local-gate` is refused because whoever pushed posts it. The + * server holds the bounds and the "a thread source must name one" rule, which depends on the + * stored mode this client cannot see, so only the shape is judged here. + */ +export function requiredChecksRefusal(checks) { + const ok = + Array.isArray(checks) && + checks.every((name) => typeof name === "string" && name.trim() !== ""); + + if (!ok) { + return refuse( + "`required_checks` must be a list of GitHub Actions check-run names, as they appear " + + "on the commit (for example [\"test\", \"lint\"]).", + ); + } + + if (checks.includes("local-gate")) { + return refuse( + "`local-gate` can never be a required check: whoever pushed posts it, so on a thread " + + "it is the implementer attesting its own work. It is recorded on the checkpoint instead.", + ); + } + + return null; +} + export function sourcePath(sourceId) { return `${SOURCES_PATH}/${encodeURIComponent(sourceId)}`; } @@ -190,6 +218,7 @@ export function publicSource(source) { repo_full_name: source.repo_full_name, base_branch: source.base_branch, mode: source.mode, + required_checks: source.required_checks, target_epic_id: source.target_epic_id, revoked_at: source.revoked_at, inserted_at: source.inserted_at, @@ -216,7 +245,15 @@ export function webhookUrl(baseUrl, webhookPath) { * shape. It never throws. */ export async function enrollIntakeSource( - { repo_full_name, project_id, target_epic_id, base_branch, mode, secret_file } = {}, + { + repo_full_name, + project_id, + target_epic_id, + base_branch, + mode, + required_checks, + secret_file, + } = {}, { userKey, apiCall, baseUrl, fs = defaultFs, homedir = os.homedir() } = {}, ) { if (!userKey) return refuse(MISSING_USER_KEY); @@ -263,6 +300,14 @@ export async function enrollIntakeSource( if (badMode) return badMode; } + // OPTIONAL too (US-45.6): the CI checks a thread checkpoint must pass. Its SHAPE is judged + // here, before a secret file is reserved; whether a thread source names enough of them + // depends on the stored mode, which only the server knows. + if (required_checks !== undefined) { + const badChecks = requiredChecksRefusal(required_checks); + if (badChecks) return badChecks; + } + if (typeof secret_file !== "string" || secret_file.trim() === "") { return refuse( "`secret_file` is required: the path the webhook secret is written to (mode 0600). " + @@ -313,6 +358,7 @@ export async function enrollIntakeSource( } if (base_branch !== undefined) body.base_branch = base_branch; if (mode !== undefined) body.mode = mode; + if (required_checks !== undefined) body.required_checks = required_checks; let result; try { @@ -500,10 +546,16 @@ export async function updateIntakeSource(args = {}, { userKey, apiCall } = {}) { body.mode = args.mode; } + if (args.required_checks !== undefined) { + const badChecks = requiredChecksRefusal(args.required_checks); + if (badChecks) return badChecks; + body.required_checks = args.required_checks; + } + if (Object.keys(body).length === 0) { return refuse( "Nothing to update. Name at least one of `target_epic_id` (null clears it), " + - "`base_branch` or `mode`. A body carrying none of them is refused 422 " + + "`base_branch`, `mode` or `required_checks`. A body carrying none of them is refused 422 " + "`nothing_to_update`, because a field you do not send is left exactly as it was.", ); } diff --git a/mcp-server/package-lock.json b/mcp-server/package-lock.json index c8fed4ab..431f2aee 100644 --- a/mcp-server/package-lock.json +++ b/mcp-server/package-lock.json @@ -1,12 +1,12 @@ { "name": "loopctl-mcp-server", - "version": "2.107.0", + "version": "2.108.0", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "loopctl-mcp-server", - "version": "2.107.0", + "version": "2.108.0", "license": "MIT", "dependencies": { "@modelcontextprotocol/sdk": "^1.28.0" diff --git a/mcp-server/package.json b/mcp-server/package.json index ca12aab6..17eb226b 100644 --- a/mcp-server/package.json +++ b/mcp-server/package.json @@ -1,6 +1,6 @@ { "name": "loopctl-mcp-server", - "version": "2.107.0", + "version": "2.108.0", "description": "MCP server for loopctl — structural trust for AI development loops", "type": "module", "main": "index.js", diff --git a/mcp-server/test/intake_source_tools.test.js b/mcp-server/test/intake_source_tools.test.js index cb864a6b..88fc4fc2 100644 --- a/mcp-server/test/intake_source_tools.test.js +++ b/mcp-server/test/intake_source_tools.test.js @@ -168,6 +168,7 @@ describe("intake_source_enroll", () => { "mode", "project_id", "repo_full_name", + "required_checks", "revoked_at", "target_epic_id", "updated_at", @@ -348,6 +349,54 @@ describe("intake_source_enroll", () => { assert.equal(calls[1].body.mode, "thread"); }); + test("sends required_checks only when given (US-45.6)", async () => { + const { calls, apiCall } = fakeApi(created(), created()); + + await enrollIntakeSource( + { repo_full_name: REPO, project_id: PROJECT_ID, secret_file: secretFileIn("rc-a") }, + deps({ apiCall }), + ); + assert.ok(!("required_checks" in calls[0].body), "unnamed required_checks reached the server"); + + await enrollIntakeSource( + { + repo_full_name: REPO, + project_id: PROJECT_ID, + mode: "thread", + required_checks: ["test", "lint"], + secret_file: secretFileIn("rc-b"), + }, + deps({ apiCall }), + ); + assert.deepEqual(calls[1].body.required_checks, ["test", "lint"]); + }); + + test("malformed required_checks, or local-gate among them, are refused locally", async () => { + for (const [i, [bad, reason]] of [ + ["test", /must be a list/], + [[""], /must be a list/], + [[7], /must be a list/], + [["test", "local-gate"], /local-gate` can never be a required check/], + ].entries()) { + const { calls, apiCall } = fakeApi(created()); + + const result = await enrollIntakeSource( + { + repo_full_name: REPO, + project_id: PROJECT_ID, + mode: "thread", + required_checks: bad, + secret_file: secretFileIn(`rc-bad-${i}.secret`), + }, + deps({ apiCall }), + ); + + assert.equal(result.error, true, JSON.stringify(bad)); + assert.match(result.body, reason); + assert.equal(calls.length, 0, "a refused enrolment must not reach the API"); + } + }); + test("an unknown or null mode is refused locally, without enrolling anything", async () => { for (const [i, bad] of [null, "", "merge"].entries()) { const { calls, apiCall } = fakeApi(created()); @@ -750,6 +799,25 @@ describe("intake_source_update", () => { assert.equal(result.source.mode, "thread"); }); + test("required_checks alone is a complete update, forwarded as named (US-45.6)", async () => { + const { calls, apiCall } = fakeApi({ source: { ...created().source, required_checks: ["e2e"] } }); + + const result = await updateIntakeSource( + { source_id: SOURCE_ID, required_checks: ["e2e"] }, + deps({ apiCall }), + ); + + assert.deepEqual(calls[0].body, { required_checks: ["e2e"] }); + assert.deepEqual(result.source.required_checks, ["e2e"]); + + const refused = await updateIntakeSource( + { source_id: SOURCE_ID, required_checks: ["local-gate"] }, + deps({ apiCall }), + ); + assert.equal(refused.error, true); + assert.equal(calls.length, 1, "a refused update must not reach the API"); + }); + test("a null mode is refused locally on update, naming the reason", async () => { const { calls, apiCall } = fakeApi({ source: {} }); diff --git a/priv/repo/migrations/20260927100000_add_intake_sources_required_checks.exs b/priv/repo/migrations/20260927100000_add_intake_sources_required_checks.exs new file mode 100644 index 00000000..b93ab82a --- /dev/null +++ b/priv/repo/migrations/20260927100000_add_intake_sources_required_checks.exs @@ -0,0 +1,27 @@ +defmodule Loopctl.Repo.Migrations.AddIntakeSourcesRequiredChecks do + @moduledoc """ + US-45.6 (Epic 45, change threads): the CI checks a thread-mode checkpoint must pass on its + exact commit before the merge gate allows it. + + A pull request gets its required checks from the base branch's protection, enforced by + GitHub at merge time. A thread has no pull request, and the loopctl App pushes the squash + commit to the base itself, so no forge rule can hold the merge to a green checkpoint: the + gate reads the checks by SHA and needs to know which ones are required. They are named + here, per repository. + + NOT NULL, default the empty list, so every existing source is unchanged: a `pr`-mode source + never reads the column. A `thread`-mode source must name at least one check, which the + changeset enforces on every write (`Loopctl.Intake.Source.validate_required_checks/1`) and + the gate backstops by refusing `required_checks_unset`. + + No backfill and no manual step. + """ + + use Ecto.Migration + + def change do + alter table(:intake_sources) do + add :required_checks, {:array, :string}, null: false, default: [] + end + end +end diff --git a/test/loopctl/delivery/ci_evidence_test.exs b/test/loopctl/delivery/ci_evidence_test.exs new file mode 100644 index 00000000..9a35b1ef --- /dev/null +++ b/test/loopctl/delivery/ci_evidence_test.exs @@ -0,0 +1,138 @@ +defmodule Loopctl.Delivery.CiEvidenceTest do + @moduledoc """ + `Loopctl.Delivery.CiEvidence` (US-45.6): what CI said about one commit, judged against the + required checks. Pure, so every case is a table of evidence and an expected judgement. + """ + + use ExUnit.Case, async: true + + alias Loopctl.Delivery.CiEvidence + + defp run(name, status, conclusion \\ nil), + do: %{name: name, status: status, conclusion: conclusion, app: "github-actions"} + + defp status(context, state), do: %{context: context, state: state} + + defp judge(required, runs, statuses \\ []), + do: CiEvidence.judge(required, %{check_runs: runs, statuses: statuses}) + + test "a completed run concluding success, neutral or skipped passes" do + for conclusion <- ["success", "neutral", "skipped"] do + assert %{passed: ["test"], failed: [], pending: [], missing: []} = + judge(["test"], [run("test", "completed", conclusion)]), + conclusion + end + end + + test "any other conclusion fails, naming it" do + for conclusion <- ["failure", "cancelled", "timed_out", "action_required", "stale"] do + assert %{failed: [{"test", ^conclusion}], passed: []} = + judge(["test"], [run("test", "completed", conclusion)]) + end + end + + test "a run that has not completed is pending, whatever it concluded so far" do + for state <- ["queued", "in_progress", "waiting"] do + assert %{pending: ["test"], passed: [], failed: []} = judge(["test"], [run("test", state)]) + end + end + + # Review round 2, finding 1: a status can be posted by the implementer, so it never + # satisfies a required check; nor does a run some other App created. + test "a commit status never satisfies a required check, however green or new" do + statuses = [status("test", "success")] + assert %{missing: ["test"], passed: []} = judge(["test"], [], statuses) + + failing = Map.put(run("test", "completed", "failure"), :id, 1) + assert %{failed: [{"test", "failure"}]} = judge(["test"], [failing], statuses) + end + + # Round 3: runs are judged latest-per-SUITE, and across suites a green run never hides a red + # one under the same name. + test "a passing run in one suite never hides a failing run in another" do + ci_fail = %{run("test", "completed", "failure") | app: "github-actions"} + ci_fail = Map.merge(ci_fail, %{id: 100, check_suite: 1}) + other_pass = Map.merge(run("test", "completed", "success"), %{id: 105, check_suite: 2}) + + assert %{failed: [{"test", "failure"}], passed: []} = judge(["test"], [ci_fail, other_pass]) + + # Within ONE suite, a later re-run supersedes the failure it re-ran. + rerun = Map.merge(run("test", "completed", "success"), %{id: 101, check_suite: 1}) + assert %{passed: ["test"]} = judge(["test"], [ci_fail, rerun]) + + # And a suite still running keeps the name pending when none failed. + running = Map.merge(run("test", "in_progress"), %{id: 106, check_suite: 3}) + assert %{pending: ["test"]} = judge(["test"], [rerun, running]) + end + + test "only a GitHub Actions check run counts" do + other_app = %{run("test", "completed", "success") | app: "some-other-app"} + assert %{missing: ["test"], passed: []} = judge(["test"], [other_app]) + end + + test "a required name nothing reported is missing" do + assert %{missing: ["test"], passed: [], pending: [], failed: []} = + judge(["test"], [run("lint", "completed", "success")]) + end + + # Review round 1, finding 5: only the LATEST result under a name counts, as GitHub's own + # required-check rule judges it. + test "among runs of one name the highest id decides, whatever it concluded" do + old_fail = Map.put(run("test", "completed", "failure"), :id, 1) + new_pass = Map.put(run("test", "completed", "success"), :id, 2) + assert %{passed: ["test"], failed: []} = judge(["test"], [new_pass, old_fail]) + + rerun = Map.put(run("test", "queued"), :id, 3) + assert %{pending: ["test"], passed: []} = judge(["test"], [new_pass, rerun]) + end + + test "each required name is judged on its own" do + runs = [run("test", "completed", "success"), run("lint", "completed", "failure")] + + assert %{passed: ["test"], failed: [{"lint", "failure"}], missing: ["dialyzer"]} = + judge(["test", "lint", "dialyzer"], runs) + end + + # AC-45.6.2: the local gate is recorded and never satisfies a required check, even when a + # caller lists it. + test "local-gate is reported, never looked up as a required check" do + statuses = [status("local-gate", "success")] + + assert %{local_gate: "success", missing: ["test"], passed: []} = + judge(["test"], [], statuses) + + assert %{local_gate: "success", passed: [], missing: []} = + judge(["local-gate"], [], statuses) + + assert CiEvidence.lookup_names(["local-gate", "test"]) == ["test"] + end + + test "to_record/5 keeps the evidence and the judgement with string keys" do + evidence = %{ + check_runs: [run("test", "completed", "failure")], + statuses: [status("local-gate", "success")] + } + + result = CiEvidence.judge(["test"], evidence) + record = CiEvidence.to_record("abc", ["test"], evidence, result, ~U[2026-09-27 10:00:00Z]) + + assert record["sha"] == "abc" + # Always six fractional digits, so records order correctly as text. + assert record["read_at"] == "2026-09-27T10:00:00.000000Z" + assert record["local_gate"] == "success" + assert record["failed"] == [%{"name" => "test", "why" => "failure"}] + assert [%{"name" => "test", "conclusion" => "failure"}] = record["check_runs"] + + # Round 3: only what the judgement read is kept — required runs and local-gate. + noisy = %{ + evidence + | check_runs: [run("codeql", "completed", "success") | evidence.check_runs], + statuses: [status("other", "success") | evidence.statuses] + } + + slim = CiEvidence.to_record("abc", ["test"], noisy, result, ~U[2026-09-27 10:00:00Z]) + assert Enum.map(slim["check_runs"], & &1["name"]) == ["test"] + assert Enum.map(slim["statuses"], & &1["context"]) == ["local-gate"] + assert [%{"context" => "local-gate", "state" => "success"}] = record["statuses"] + 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 37e1b6ad..1473efcc 100644 --- a/test/loopctl/delivery/github_pull_request_source_test.exs +++ b/test/loopctl/delivery/github_pull_request_source_test.exs @@ -753,6 +753,112 @@ defmodule Loopctl.Delivery.GitHubPullRequestSourceTest do # -- helpers --------------------------------------------------------------------------- + describe "check_evidence/3 (US-45.6)" do + defp gh_run(id, name, conclusion), + do: %{ + "id" => id, + "name" => name, + "status" => "completed", + "conclusion" => conclusion, + "app" => %{"slug" => "github-actions"}, + "check_suite" => %{"id" => id * 10} + } + + # Review round 1, finding 7: ONE paged read of every run on the commit, never one per name. + test "reads every latest run on the exact SHA in one paged call, and every status" do + stub(fn conn -> + case String.split(conn.request_path, "/") do + [_, "repos", "acme", "widgets", "commits", @head, "check-runs"] -> + query = URI.decode_query(conn.query_string) + assert query["filter"] == "latest" + assert query["per_page"] == "100" + refute Map.has_key?(query, "check_name") + assert query["page"] == "1" + + json(conn, %{ + "total_count" => 2, + "check_runs" => [ + Map.put(gh_run(7, "test", "success"), "completed_at", "2026-09-27T10:00:00Z"), + gh_run(8, "lint / credo", "failure") + ] + }) + + [_, "repos", "acme", "widgets", "commits", @head, "status"] -> + assert conn.query_string == "per_page=100" + + json(conn, %{ + "total_count" => 1, + "statuses" => [ + %{"context" => "local-gate", "state" => "success", "updated_at" => "t"} + ] + }) + end + end) + + assert {:ok, %{check_runs: runs, statuses: statuses}} = + Source.check_evidence(@repo, @head) + + assert [ + %{ + id: 7, + name: "test", + app: "github-actions", + check_suite: 70, + completed_at: "2026-09-27T10:00:00Z" + }, + %{id: 8} + ] = runs + + assert [%{context: "local-gate", state: "success", at: "t"}] = statuses + end + + test "further pages are read until the total is reached" do + stub(fn conn -> + if String.ends_with?(conn.request_path, "/check-runs") do + page = URI.decode_query(conn.query_string)["page"] + run = gh_run(String.to_integer(page), "job-#{page}", "success") + json(conn, %{"total_count" => 2, "check_runs" => [run]}) + else + json(conn, %{"total_count" => 0, "statuses" => []}) + end + end) + + assert {:ok, %{check_runs: [%{name: "job-1"}, %{name: "job-2"}]}} = + Source.check_evidence(@repo, @head) + end + + test "a list the forge truncated is an error, never a partial answer" do + stub(fn conn -> + if String.ends_with?(conn.request_path, "/check-runs") do + json(conn, %{"total_count" => 1000, "check_runs" => [gh_run(1, "test", "success")]}) + else + json(conn, %{"total_count" => 0, "statuses" => []}) + end + end) + + assert {:error, {:check_runs_truncated, 1000, 3}} = + Source.check_evidence(@repo, @head) + + stub(fn conn -> + if String.ends_with?(conn.request_path, "/check-runs") do + json(conn, %{"total_count" => 0, "check_runs" => []}) + else + json(conn, %{"total_count" => 101, "statuses" => []}) + end + end) + + assert {:error, {:statuses_truncated, 101, 0}} = + Source.check_evidence(@repo, @head) + end + + test "an unreadable body is an error naming its shape" do + stub(fn conn -> json(conn, %{"message" => "nope"}) end) + + assert {:error, {:unreadable_check_runs, {:map, ["message"]}}} = + Source.check_evidence(@repo, @head) + end + end + defp stub(fun), do: Req.Test.stub(Source, fun) defp deployment_route(conn, statuses) do diff --git a/test/loopctl/delivery/merge_precondition_integration_test.exs b/test/loopctl/delivery/merge_precondition_integration_test.exs index a38cf711..b235bc3f 100644 --- a/test/loopctl/delivery/merge_precondition_integration_test.exs +++ b/test/loopctl/delivery/merge_precondition_integration_test.exs @@ -517,6 +517,111 @@ defmodule Loopctl.Delivery.MergePreconditionIntegrationTest do assert id == ctx.checkpoint.id end + # TC-45.6.1: green on the PARENT, pending on the checkpoint: the gate reads the + # checkpoint's own commit and nothing else, so it does not allow. + test "TC-45.6.1 CI is read for the checkpoint's exact SHA; green on another commit is not an allow", + ctx do + stub_thread(ctx) + parent = String.duplicate("b", 40) + + Mox.stub(MockPullRequestSource, :check_evidence, fn @repo, sha -> + cond do + sha == @head -> {:ok, %{check_runs: [ci_run("test", nil, "in_progress")], statuses: []}} + sha == parent -> {:ok, %{check_runs: [ci_run("test", "success")], statuses: []}} + end + end) + + assert {:ok, %Verdict{decision: :unevaluated} = verdict} = enforce(ctx) + assert {:required_check_pending, "test"} in verdict.reasons + assert Stages.get(ctx.tenant_id, ctx.story_id).merge_gate_allowed_sha == nil + end + + # AC-45.6.1: whatever the decision, the evidence read is copied onto the checkpoint. + test "the evidence is copied onto the checkpoint on an allow and on a refusal", ctx do + stub_thread(ctx) + + assert {:ok, %Verdict{decision: :allow}} = enforce(ctx) + + assert %{"ci" => %{"sha" => @head, "passed" => ["test"], "failed" => []}} = + checkpoint_evidence(ctx) + + stub_thread(ctx, ci: %{check_runs: [ci_run("test", "failure")], statuses: []}) + reset_allow(ctx) + + assert {:ok, %Verdict{decision: :refuse} = refused} = enforce(ctx) + assert {:required_check_failed, "test", "failure"} in refused.reasons + + assert %{"ci" => %{"failed" => [%{"name" => "test", "why" => "failure"}]}} = + checkpoint_evidence(ctx) + end + + # Review round 1, findings 1 and 2: a CI wait is bounded in TIME from the checkpoint, + # never by polls, so a slow pipeline never escalates and a stuck one still does. + test "a CI wait never reaches the unevaluated bound; past the time limit it escalates", ctx do + for ci <- [ + %{check_runs: [ci_run("test", nil, "queued")], statuses: []}, + %{check_runs: [], statuses: []} + ] do + stub_thread(ctx, ci: ci) + + for _ <- 1..(MergePrecondition.max_consecutive_unevaluated() + 2) do + assert {:ok, %Verdict{decision: :unevaluated}} = enforce(ctx) + end + + assert Stages.get(ctx.tenant_id, ctx.story_id).stage == :ci + end + + # The wait is measured from the story's entry into `ci`, not from the checkpoint. + entered = + DateTime.add(DateTime.utc_now(), -(MergePrecondition.ci_wait_limit_seconds() + 60)) + + enter_ci_at(ctx, entered) + + assert {:ok, %Verdict{decision: :refuse} = verdict} = enforce(ctx) + assert {:required_check_timed_out, "test", :missing} in verdict.reasons + assert Stages.get(ctx.tenant_id, ctx.story_id).stage == :escalated + end + + # Round 3: the required checks are the source's CURRENT list, so correcting the list + # (a renamed job) reaches a story already in flight. + test "the required checks are the source's current list, read live", ctx do + stub_thread(ctx, ci: %{check_runs: [ci_run("unit", "success")], statuses: []}) + + assert {:ok, %Verdict{decision: :unevaluated} = waiting} = enforce(ctx) + assert {:required_check_missing, "test"} in waiting.reasons + + {1, _} = + from(src in Loopctl.Intake.Source, where: src.project_id == ^ctx.project_id) + |> AdminRepo.update_all(set: [required_checks: ["unit"]]) + + assert {:ok, %Verdict{decision: :allow}} = enforce(ctx) + end + + # Review round 2, finding 7: a completed CI wait ends a run of transient faults. + test "a CI wait between forge faults resets their consecutive count", ctx do + stub_thread(ctx) + blips = MergePrecondition.max_consecutive_unevaluated() - 1 + + fault = fn -> + Mox.stub(MockPullRequestSource, :check_evidence, fn @repo, _sha -> + {:error, {:github_unreachable, :timeout}} + end) + + for _ <- 1..blips, do: assert({:ok, %Verdict{decision: :unevaluated}} = enforce(ctx)) + end + + fault.() + + Mox.stub(MockPullRequestSource, :check_evidence, fn @repo, _sha -> + {:ok, %{check_runs: [ci_run("test", nil, "queued")], statuses: []}} + end) + + assert {:ok, %Verdict{decision: :unevaluated}} = enforce(ctx) + + fault.() + assert Stages.get(ctx.tenant_id, ctx.story_id).stage == :ci + end + test "TC-45.4.3 a branch head nobody reported goes back to implementing, unescalated", ctx do pushed = String.duplicate("9", 40) make_claim_live(ctx) @@ -987,8 +1092,44 @@ defmodule Loopctl.Delivery.MergePreconditionIntegrationTest do end) Mox.stub(MockPullRequestSource, :repo_files, fn @repo, _ref -> {:ok, @repo_files} end) + + # US-45.6: CI on the CHECKPOINT's commit, green unless the test says otherwise. + ci = Keyword.get(opts, :ci, %{check_runs: [ci_run("test", "success")], statuses: []}) + Mox.stub(MockPullRequestSource, :check_evidence, fn @repo, ^head -> {:ok, ci} end) + end + + # Records the story's transition into `ci` at `at` (the CI wait's origin). + defp enter_ci_at(ctx, at) do + AdminRepo.insert!(%Loopctl.Delivery.StageEvent{ + tenant_id: ctx.tenant_id, + story_stage_id: Stages.get(ctx.tenant_id, ctx.story_id).id, + story_id: ctx.story_id, + event: "transitioned", + from_stage: "pr_open", + to_stage: "ci", + edge: "forward", + claim_epoch: 0, + lock_version: 0, + data: %{}, + inserted_at: at + }) + end + + defp checkpoint_evidence(ctx) do + AdminRepo.get!(Loopctl.Threads.Checkpoint, ctx.checkpoint.id).gate_evidence end + # A refusal test after an allow: the recorded allow is for the same head, and the story is + # back at `ci` with nothing allowed so the gate judges afresh. + defp reset_allow(ctx) do + {1, _} = + from(r in StoryStage, where: r.story_id == ^ctx.story_id) + |> AdminRepo.update_all(set: [merge_gate_allowed_sha: nil]) + end + + defp ci_run(name, conclusion, status \\ "completed"), + do: %{name: name, status: status, conclusion: conclusion, url: nil, app: "github-actions"} + # The comparison, relative to `@base_head` unless `:merge_base` says otherwise. defp comparison(base_tree, opts) do %{ @@ -1140,7 +1281,8 @@ defmodule Loopctl.Delivery.MergePreconditionIntegrationTest do fixture(:intake_source, %{ tenant_id: tenant.id, project_id: project.id, - repo_full_name: @repo + repo_full_name: @repo, + required_checks: ["test"] }) {:ok, %{dispatch: implementer}} = diff --git a/test/loopctl/delivery/merge_precondition_judge_test.exs b/test/loopctl/delivery/merge_precondition_judge_test.exs index c1401cb5..b9d7edc1 100644 --- a/test/loopctl/delivery/merge_precondition_judge_test.exs +++ b/test/loopctl/delivery/merge_precondition_judge_test.exs @@ -1200,6 +1200,180 @@ defmodule Loopctl.Delivery.MergePreconditionJudgeTest do end end + describe "CI evidence on the checkpoint's exact commit (US-45.6)" do + test "a failed required check refuses, naming it and its conclusion" do + verdict = + judge_thread(ci: %{check_runs: [run("test", "completed", "failure")], statuses: []}) + + assert verdict.decision == :refuse + assert {:required_check_failed, "test", "failure"} in verdict.reasons + end + + test "a pending required check is unevaluated, retried after five minutes, never allowed" do + verdict = judge_thread(ci: %{check_runs: [run("test", "in_progress", nil)], statuses: []}) + + assert verdict.decision == :unevaluated + assert {:required_check_pending, "test"} in verdict.reasons + assert verdict.retry_after == 300 + end + + test "a required check missing from the commit is unevaluated, never allowed" do + verdict = judge_thread(ci: %{check_runs: [], statuses: []}) + + assert verdict.decision == :unevaluated + assert {:required_check_missing, "test"} in verdict.reasons + end + + test "a failure decides even while another required check is still running" do + ci = %{ + check_runs: [run("test", "completed", "failure"), run("lint", "queued", nil)], + statuses: [] + } + + verdict = judge_thread(required_checks: ["test", "lint"], ci: ci) + + assert verdict.decision == :refuse + assert {:required_check_failed, "test", "failure"} in verdict.reasons + refute {:required_check_pending, "lint"} in verdict.reasons + end + + # TC-45.6.2: the local gate alone never satisfies a required check. + test "only a local-gate success is not an allow" do + ci = %{check_runs: [], statuses: [%{context: "local-gate", state: "success"}]} + verdict = judge_thread(ci: ci) + + assert verdict.decision == :unevaluated + assert {:required_check_missing, "test"} in verdict.reasons + assert verdict.ci_evidence["local_gate"] == "success" + end + + test "a thread source requiring no check is refused, not allowed" do + verdict = judge_thread(required_checks: []) + + assert verdict.decision == :refuse + assert :required_checks_unset in verdict.reasons + + # `local-gate` alone is the same as nothing: it is never looked up. + assert :required_checks_unset in judge_thread(required_checks: ["local-gate"]).reasons + end + + test "an evidence read that failed transiently is unevaluated; otherwise it refuses" do + transient = judge_thread([], ci_evidence: {:error, {:github_unreachable, :timeout}}) + assert transient.decision == :unevaluated + assert {:ci_evidence_unavailable, {:github_unreachable, :timeout}} in transient.reasons + + broken = judge_thread([], ci_evidence: {:error, {:check_runs_truncated, 120, 100}}) + assert broken.decision == :refuse + assert {:ci_evidence_unavailable, {:check_runs_truncated, 120, 100}} in broken.reasons + end + + test "an allow carries the evidence it rests on, for enforce/3 to copy" do + verdict = judge_thread([]) + + assert verdict.decision == :allow + assert %{"sha" => @head, "passed" => ["test"], "required" => ["test"]} = verdict.ci_evidence + end + + # Round 3: Actions runs the workflow files of the commit under test, so a thread that + # changes them could make its own required checks green. + test "a checkpoint changing CI definitions is refused for a human, renames included" do + for {files, renames} <- [ + {[".github/workflows/ci.yml"], []}, + {[".github/actions/setup/action.yml"], []}, + {["ci/new.yml"], [{".github/workflows/old.yml", "ci/new.yml"}]} + ] do + pr_diff = {:ok, %{files: files, renames: renames}} + verdict = judge_thread(diff_override: pr_diff) + + assert verdict.decision == :refuse, inspect(files) + assert Enum.any?(verdict.reasons, &match?({:ci_definition_changed, _}, &1)) + end + end + + # Review round 2, finding 3: a CI wait holds back only an allow. + test "a refusal is decided now, not held back while CI is still running" do + running = %{check_runs: [run("test", "queued", nil)], statuses: []} + verdict = judge_thread(head_tree_sha: String.duplicate("f", 40), ci: running) + + assert verdict.decision == :refuse + refute {:required_check_pending, "test"} in verdict.reasons + end + + # Review round 2, finding 5: required checks with no read fail closed. + test "required checks with nothing read refuse ci_evidence_not_read" do + verdict = judge_thread([], ci_evidence: {:ok, nil}) + + assert verdict.decision == :refuse + assert :ci_evidence_not_read in verdict.reasons + end + + test "an allow whose evidence was superseded by a newer read is judged again" do + superseded = MergePrecondition.allow_evidence_outcome(judge_thread([]), :superseded) + + # Round 3: a wait like any CI wait — not counted, asked again on the CI interval. + assert superseded.decision == :unevaluated + assert [{:ci_evidence_superseded, @head}] = superseded.reasons + assert superseded.retry_after == 300 + refute MergePrecondition.counts_toward_unevaluated_bound?(superseded) + end + + # Review round 1, finding 4: contention on the evidence write is a retry, not an escalation. + test "an allow whose evidence write met contention is unevaluated; other failures refuse" do + allowed = judge_thread([]) + + busy = MergePrecondition.allow_evidence_outcome(allowed, {:error, :busy}) + assert busy.decision == :unevaluated + assert {:ci_evidence_not_recorded, :busy} in busy.reasons + assert MergePrecondition.counts_toward_unevaluated_bound?(busy) + + gone = MergePrecondition.allow_evidence_outcome(allowed, {:error, :not_found}) + assert gone.decision == :refuse + assert {:ci_evidence_not_recorded, :not_found} in gone.reasons + + assert :ok = MergePrecondition.allow_evidence_outcome(allowed, :ok) + end + + test "a pr-mode verdict reads and carries no CI evidence" do + verdict = judge([]) + + assert verdict.mode == :pr + assert verdict.ci_evidence == nil + refute :required_checks_unset in verdict.reasons + end + + # Review round 1, findings 1 and 2: a CI wait is bounded in TIME, never by polls. + test "a CI wait, running or missing, is left out of the unevaluated bound" do + pending = judge_thread(ci: %{check_runs: [run("test", "queued", nil)], statuses: []}) + refute MergePrecondition.counts_toward_unevaluated_bound?(pending) + + missing = judge_thread(ci: %{check_runs: [], statuses: []}) + refute MergePrecondition.counts_toward_unevaluated_bound?(missing) + + forge = judge_thread([], ci_evidence: {:error, {:github_unreachable, :timeout}}) + assert MergePrecondition.counts_toward_unevaluated_bound?(forge) + end + + test "past the wait limit a check still running or never reported is refused" do + entered = ~U[2026-09-27 00:00:00Z] + late = DateTime.add(entered, MergePrecondition.ci_wait_limit_seconds() + 1) + on_time = DateTime.add(entered, MergePrecondition.ci_wait_limit_seconds()) + + stuck = %{check_runs: [run("test", "in_progress", nil)], statuses: []} + + refused = judge_thread([ci: stuck], ci_entered_at: entered, now: late) + assert refused.decision == :refuse + assert {:required_check_timed_out, "test", :pending} in refused.reasons + + never = + judge_thread([ci: %{check_runs: [], statuses: []}], ci_entered_at: entered, now: late) + + assert {:required_check_timed_out, "test", :missing} in never.reasons + + waiting = judge_thread([ci: stuck], ci_entered_at: entered, now: on_time) + assert waiting.decision == :unevaluated + end + end + # The facts of a thread story at its latest recorded checkpoint, as `gather/3` resolves them # through `Loopctl.Delivery.CheckpointSource`. defp judge_thread(overrides, fact_overrides \\ []) do @@ -1222,6 +1396,12 @@ defmodule Loopctl.Delivery.MergePreconditionJudgeTest do base_tree_sha: Keyword.get(overrides, :base_tree_sha, @base_tree) }) + pr = + case Keyword.fetch(overrides, :diff_override) do + {:ok, diff} -> Map.put(pr, :diff, diff) + :error -> pr + end + [ pr_number: {:ok, nil}, pull_request: {:ok, pr}, @@ -1232,12 +1412,23 @@ defmodule Loopctl.Delivery.MergePreconditionJudgeTest do |> Map.merge(%{ mode: Keyword.get(overrides, :mode, :thread), checkpoint: {:ok, checkpoint}, - claim_live?: Keyword.get(overrides, :claim_live?, true) + claim_live?: Keyword.get(overrides, :claim_live?, true), + required_checks: Keyword.get(overrides, :required_checks, ["test"]), + ci_evidence: {:ok, ci_read(Keyword.get(overrides, :ci, green_ci()))} }) |> Map.merge(Map.new(fact_overrides)) |> MergePrecondition.judge() end + # US-45.6: the checkpoint commit's CI as `gather/3` reads it. + defp green_ci, do: %{check_runs: [run("test", "completed", "success")], statuses: []} + + defp run(name, status, conclusion), + do: %{name: name, status: status, conclusion: conclusion, app: "github-actions"} + + defp ci_read(evidence), + do: %{evidence: evidence, sha: @head, read_at: ~U[2026-09-27 10:00:00Z]} + defp judge(opts), do: opts |> facts() |> MergePrecondition.judge() defp facts(opts) do diff --git a/test/loopctl/delivery/stages_test.exs b/test/loopctl/delivery/stages_test.exs index 69be8775..ed2312b1 100644 --- a/test/loopctl/delivery/stages_test.exs +++ b/test/loopctl/delivery/stages_test.exs @@ -433,6 +433,37 @@ defmodule Loopctl.Delivery.StagesTest do end end + describe "entered_at/3 (US-45.6)" do + test "the newest transition INTO the stage, nil before the story ever entered it" do + {story, _row} = at_stage(:pr_open) + opts = [claim_epoch: story.claim_epoch] + + assert Stages.entered_at(story.tenant_id, story.id, :ci) == nil + + {:ok, _} = Stages.advance(story.tenant_id, story.id, {:pr_open, :ci}, opts) + first = Stages.entered_at(story.tenant_id, story.id, :ci) + assert %DateTime{} = first + + {:ok, _} = Stages.advance(story.tenant_id, story.id, {:ci, :implementing, :ci_red}, opts) + + [:implementing, :reviewing, :pr_open, :ci] + |> Enum.chunk_every(2, 1, :discard) + |> Enum.each(fn [from, to] -> + {:ok, _} = Stages.advance(story.tenant_id, story.id, {from, to}, opts) + end) + + second = Stages.entered_at(story.tenant_id, story.id, :ci) + assert DateTime.compare(second, first) == :gt + + # A later transition OUT of the stage does not move when it was entered. + {:ok, _} = Stages.advance(story.tenant_id, story.id, {:ci, :implementing, :ci_red}, opts) + assert Stages.entered_at(story.tenant_id, story.id, :ci) == second + + # Another tenant's story is never read. + assert Stages.entered_at(fixture(:tenant).id, story.id, :ci) == nil + end + end + describe "note_unevaluated/4" do test "counts consecutive results at ONE head, and resets when the head moves" do {story, _row} = at_stage(:ci) diff --git a/test/loopctl/intake_test.exs b/test/loopctl/intake_test.exs index 113b0a71..3a53d0ec 100644 --- a/test/loopctl/intake_test.exs +++ b/test/loopctl/intake_test.exs @@ -297,6 +297,93 @@ defmodule Loopctl.IntakeTest do end end + describe "required_checks (US-45.6)" do + setup do + tenant = fixture(:tenant) + project = fixture(:project, %{tenant_id: tenant.id}) + %{tenant: tenant, project: project} + end + + defp enrol(ctx, attrs) do + Intake.create_source( + ctx.tenant.id, + Map.merge(%{repo_full_name: "mkreyman/infra", project_id: ctx.project.id}, attrs) + ) + end + + test "a thread source enrols with its required checks, and stores them", ctx do + assert {:ok, %{source: source}} = + enrol(ctx, %{mode: :thread, required_checks: ["test", "lint"]}) + + assert AdminRepo.get!(Source, source.id).required_checks == ["test", "lint"] + end + + test "a thread source naming no required check is refused at enrolment", ctx do + assert {:error, changeset} = enrol(ctx, %{mode: :thread}) + + assert %{required_checks: ["a thread-mode source must name at least one"]} = + errors_on(changeset) + + # A pr source needs none: it merges through the pull request's own protection. + assert {:ok, %{source: _}} = enrol(ctx, %{mode: :pr}) + end + + test "local-gate can never be a required check", ctx do + assert {:error, changeset} = + enrol(ctx, %{mode: :thread, required_checks: ["test", "local-gate"]}) + + assert %{required_checks: [message]} = errors_on(changeset) + assert message =~ "local-gate" + end + + test "blank, duplicate and oversized names are refused", ctx do + # A JSON null element casts to nil: it must be a 422, never a raise (round 1, finding 3). + for bad <- [[""], [" "], ["test", "test"], [String.duplicate("x", 201)], [nil], ["a", nil]] do + assert {:error, _changeset} = enrol(ctx, %{mode: :thread, required_checks: bad}), + inspect(bad) + end + end + + test "switching to thread, or clearing a thread source's checks, is judged as a whole", ctx do + {:ok, %{source: source}} = enrol(ctx, %{}) + + # `mode` alone on a source with no checks: refused, whichever field was named. + assert {:error, changeset} = + Intake.update_source(ctx.tenant.id, source.id, %{mode: :thread}) + + assert %{required_checks: [_]} = errors_on(changeset) + assert AdminRepo.get!(Source, source.id).mode == :pr + + assert {:ok, threaded} = + Intake.update_source(ctx.tenant.id, source.id, %{ + mode: :thread, + required_checks: ["test"] + }) + + assert threaded.required_checks == ["test"] + + assert {:error, _} = + Intake.update_source(ctx.tenant.id, source.id, %{required_checks: []}) + + assert AdminRepo.get!(Source, source.id).required_checks == ["test"] + end + + test "a required_checks change is recorded on the audit chain", ctx do + {:ok, %{source: source}} = enrol(ctx, %{mode: :thread, required_checks: ["test"]}) + + assert {:ok, _} = + Intake.update_source(ctx.tenant.id, source.id, %{required_checks: ["test", "e2e"]}) + + assert [%{payload: %{"required_checks" => ["test", "e2e"]}}] = + AdminRepo.all( + from e in Loopctl.AuditChain.Entry, + where: + e.tenant_id == ^ctx.tenant.id and + e.action == "intake_source_required_checks_set" + ) + end + end + describe "repoint_source/4" do test "an active source is repointed at an epic of its project, and the act is recorded" do tenant = fixture(:tenant) diff --git a/test/loopctl/threads_test.exs b/test/loopctl/threads_test.exs index 2b38a39a..dd1c3302 100644 --- a/test/loopctl/threads_test.exs +++ b/test/loopctl/threads_test.exs @@ -254,6 +254,89 @@ defmodule Loopctl.ThreadsTest do end end + describe "record_gate_evidence/5 (US-45.6)" do + test "stores the record under its key and keeps every other key" do + ctx = claimed_story() + {:ok, cp, :created} = checkpoint(ctx) + + assert :ok = + Threads.record_gate_evidence(ctx.tenant_id, ctx.story.id, cp.id, "other", %{ + "a" => 1 + }) + + assert :ok = + Threads.record_gate_evidence(ctx.tenant_id, ctx.story.id, cp.id, "ci", %{ + "sha" => @sha1 + }) + + assert :ok = + Threads.record_gate_evidence(ctx.tenant_id, ctx.story.id, cp.id, "ci", %{ + "sha" => @sha2 + }) + + assert %{"other" => %{"a" => 1}, "ci" => %{"sha" => @sha2}} = stored_evidence(ctx, cp.id) + end + + # Review round 1, finding 6: a slower evaluation that read earlier never overwrites. + test "a record read no later than the stored one changes nothing" do + ctx = claimed_story() + {:ok, cp, :created} = checkpoint(ctx) + newer = %{"read_at" => "2026-09-27T10:00:01.000000Z", "passed" => ["test"]} + older = %{"read_at" => "2026-09-27T10:00:00.000000Z", "pending" => ["test"]} + + assert :ok = Threads.record_gate_evidence(ctx.tenant_id, ctx.story.id, cp.id, "ci", newer) + + # Round 2, finding 4: the caller is told it did not land. + assert :superseded = + Threads.record_gate_evidence(ctx.tenant_id, ctx.story.id, cp.id, "ci", older) + + assert %{"ci" => ^newer} = stored_evidence(ctx, cp.id) + + # Round 2, finding 6: the same judgement read later is :ok and writes nothing. + same_later = %{newer | "read_at" => "2026-09-27T10:00:05.000000Z"} + + assert :ok = + Threads.record_gate_evidence(ctx.tenant_id, ctx.story.id, cp.id, "ci", same_later) + + assert %{"ci" => ^newer} = stored_evidence(ctx, cp.id) + + newest = %{"read_at" => "2026-09-27T10:00:10.000000Z", "failed" => ["test"]} + assert :ok = Threads.record_gate_evidence(ctx.tenant_id, ctx.story.id, cp.id, "ci", newest) + assert %{"ci" => ^newest} = stored_evidence(ctx, cp.id) + end + + test "another story's checkpoint, or another tenant's, is not_found and untouched" do + ctx = claimed_story() + {:ok, cp, :created} = checkpoint(ctx) + other = claimed_story() + + assert {:error, :not_found} = + Threads.record_gate_evidence(ctx.tenant_id, other.story.id, cp.id, "ci", %{ + "x" => 1 + }) + + assert {:error, :not_found} = + Threads.record_gate_evidence(other.tenant_id, ctx.story.id, cp.id, "ci", %{ + "x" => 1 + }) + + assert stored_evidence(ctx, cp.id) == %{} + end + end + + defp stored_evidence(ctx, checkpoint_id) do + {:ok, evidence} = + Repo.with_tenant(ctx.tenant_id, fn -> + Repo.one!( + from c in Loopctl.Threads.Checkpoint, + where: c.id == ^checkpoint_id, + select: c.gate_evidence + ) + end) + + evidence + end + describe "entries" do test "a retry of the same write is the same entry; another author's key is distinct" do ctx = claimed_story() diff --git a/test/loopctl_web/controllers/intake_source_controller_test.exs b/test/loopctl_web/controllers/intake_source_controller_test.exs index 2c03c338..1bdbad60 100644 --- a/test/loopctl_web/controllers/intake_source_controller_test.exs +++ b/test/loopctl_web/controllers/intake_source_controller_test.exs @@ -452,7 +452,9 @@ defmodule LoopctlWeb.IntakeSourceControllerTest do |> auth(ctx.operator_key) |> post( ~p"/api/v1/intake/sources", - Map.put(create_params(ctx, "mkreyman/infra"), "mode", "thread") + create_params(ctx, "mkreyman/infra") + |> Map.put("mode", "thread") + |> Map.put("required_checks", ["test"]) ) |> json_response(201) @@ -470,7 +472,8 @@ defmodule LoopctlWeb.IntakeSourceControllerTest do fixture(:intake_source, %{ tenant_id: ctx.tenant.id, project_id: ctx.project.id, - target_epic_id: epic.id + target_epic_id: epic.id, + required_checks: ["test"] }) body = @@ -540,7 +543,10 @@ defmodule LoopctlWeb.IntakeSourceControllerTest do body = conn |> auth(ctx.operator_key) - |> patch(~p"/api/v1/intake/sources/#{source.id}", %{"mode" => "thread"}) + |> patch(~p"/api/v1/intake/sources/#{source.id}", %{ + "mode" => "thread", + "required_checks" => ["test"] + }) |> json_response(200) assert body["source"]["mode"] == "thread" diff --git a/test/loopctl_web/controllers/merge_precondition_controller_test.exs b/test/loopctl_web/controllers/merge_precondition_controller_test.exs index a1a9aa38..e83ef7f0 100644 --- a/test/loopctl_web/controllers/merge_precondition_controller_test.exs +++ b/test/loopctl_web/controllers/merge_precondition_controller_test.exs @@ -127,6 +127,8 @@ defmodule LoopctlWeb.MergePreconditionControllerTest do assert data["merge_base_sha"] == @base # base_sha is a THREAD-mode field (the merge base an allow records); a pr verdict has none. assert Map.has_key?(data, "base_sha") and data["base_sha"] == nil + # ci_evidence is a THREAD-mode field too (US-45.6): a pr verdict reads no CI. + assert Map.has_key?(data, "ci_evidence") and data["ci_evidence"] == nil assert data["custody"] == "ok" assert data["gate_a_inputs"] == "persisted_triage" assert data["hard_bound"] == %{"max_files" => 12, "max_changed_lines" => 1000} diff --git a/test/support/data_case.ex b/test/support/data_case.ex index e32524bf..8b5f30af 100644 --- a/test/support/data_case.ex +++ b/test/support/data_case.ex @@ -225,6 +225,11 @@ defmodule Loopctl.DataCase do {:error, :not_stubbed} end) + # US-45.6: CI evidence for a checkpoint's commit, on the same fail-closed default. + Mox.stub(Loopctl.MockPullRequestSource, :check_evidence, fn _repo, _sha -> + {:error, :not_stubbed} + end) + # US-45.4: the thread-mode reads, on the same fail-closed default. Mox.stub(Loopctl.MockPullRequestSource, :branch_head, fn _repo, _branch -> {:error, :not_stubbed} diff --git a/test/support/fixtures.ex b/test/support/fixtures.ex index 9fa1bbb1..1c5a75e9 100644 --- a/test/support/fixtures.ex +++ b/test/support/fixtures.ex @@ -2653,8 +2653,11 @@ defmodule Loopctl.Fixtures do project_id: project_id, target_epic_id: Map.get(attrs, :target_epic_id) }, - # By presence, as the context reads it: omitted keeps the `pr` default. - Map.take(attrs, [:mode]) + # By presence, as the context reads it: omitted keeps the `pr` default. A THREAD + # source must name a required check (US-45.6), so one that names none gets `test`. + attrs + |> Map.take([:mode, :required_checks]) + |> default_thread_checks() ) ) @@ -3376,4 +3379,9 @@ defmodule Loopctl.Fixtures do :ok end + + defp default_thread_checks(%{mode: mode} = attrs) when mode in [:thread, "thread"], + do: Map.put_new(attrs, :required_checks, ["test"]) + + defp default_thread_checks(attrs), do: attrs end From 30889b9bab9cd33773b78b43d806a32eda8f0276 Mon Sep 17 00:00:00 2001 From: Mark Kreyman Date: Sun, 27 Sep 2026 00:43:23 -0600 Subject: [PATCH 2/4] US-45.6 v2 review round 1: trust only the thread's own push-run jobs 1+2. Only a JOB of a GitHub Actions workflow run that a PUSH of the thread branch at the checkpoint's exact commit triggered satisfies a required check (Actions runs + jobs APIs, filtered by the API and again locally), and only by concluding success: a job skipped because a needs: failed, or neutral, fails. A check run created by any other workflow, or a status, is never trusted. check_evidence/3 now takes the branch. 3. Jobs are de-duplicated by id and a short list is refused as truncated; more than 10 workflow runs per push is refused rather than read. 4. Commit statuses are read best effort ({:unread, reason}), feeding only local_gate. 5. Stages.entered_at/3 runs under answering_busy with a bounded lock wait; contention is the ci_entry_unreadable fact, a retry. 6. Scope stated in the CiEvidence moduledoc: scripts the jobs run are code under review, judged by the thread review (US-45.3), not by this gate. 7. A diff that could not be listed refuses ci_definition_unknown (fail closed). 8. A story with no recorded ci entry measures its wait from the checkpoint's recording. 9. A required check name with surrounding whitespace is refused. 10. OpenAPI interpolates ci_wait_retry_after/0 and ci_wait_limit_seconds/0; MCP, verdict and CHANGELOG no longer restate the numbers. Token scope is now actions: read. Per-workflow judgement uses the highest job id (ids only grow); the separate newest-run filter was redundant and removed (mutation B02 showed it could not fail). Mutations A01-A47 (A14-A18, A20-A22, A25 superseded by the B set; A19 by B01; A30 retargeted) and B01, B03-B15, all exit 0. --- CHANGELOG.md | 28 +-- deploy/FLY_SECRETS.md | 2 +- docs/agent-delivery-loop.md | 2 +- lib/loopctl/delivery/ci_evidence.ex | 161 +++++++------- .../delivery/github_pull_request_source.ex | 200 +++++++++++------- lib/loopctl/delivery/merge_precondition.ex | 59 +++++- .../delivery/merge_precondition/verdict.ex | 2 +- lib/loopctl/delivery/pull_request_source.ex | 13 +- lib/loopctl/delivery/stages.ex | 17 +- lib/loopctl/intake/source.ex | 10 +- lib/loopctl/threads.ex | 4 +- .../controllers/intake_source_controller.ex | 7 +- .../merge_precondition_controller.ex | 21 +- mcp-server/CHANGELOG.md | 6 +- mcp-server/README.md | 2 +- mcp-server/index.js | 25 ++- mcp-server/lib/intake-sources.js | 2 +- test/loopctl/delivery/ci_evidence_test.exs | 147 ++++++------- .../github_pull_request_source_test.exs | 186 +++++++++------- .../merge_precondition_integration_test.exs | 41 ++-- .../merge_precondition_judge_test.exs | 43 ++-- test/loopctl/delivery/stages_test.exs | 10 +- test/loopctl/intake_test.exs | 11 +- test/support/data_case.ex | 2 +- 24 files changed, 580 insertions(+), 421 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 49caf40f..87681f5e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,7 +8,8 @@ All notable changes to loopctl are documented here. - **A thread-mode merge requires green CI on the checkpoint's exact commit (epic 45, US-45.6, migration `20260927100000`). A THREAD-mode source must now name its `required_checks`, and - the gate's `GITHUB_TOKEN` needs `checks: read` and `commit statuses: read` for it.** The + the gate's `GITHUB_TOKEN` needs `actions: read` (and, best effort, `commit statuses: read`) + for it.** The migration adds `intake_sources.required_checks` (text array, NOT NULL, default empty; no backfill). `POST`/`PATCH /api/v1/intake/sources` and the `intake_source_enroll` / `intake_source_update` MCP tools take it; a `thread` source naming none is 422, judged over @@ -18,17 +19,20 @@ All notable changes to loopctl are documented here. `required_checks_unset` until one is named.** Name GitHub Actions JOB names, and make sure each required job runs on every push to the thread branches (no path filter or job-level `if:` that can skip it): a required check that never appears is refused after the - wait. The merge gate reads the checkpoint's commit from both the check-runs and the - commit-status APIs, but only a GitHub Actions check run can satisfy a required check — a - commit status, which the implementer's runner can post, is recorded and never trusted — so - a CI that reports only statuses cannot satisfy thread mode. Per name, the latest run of - each check suite counts and every suite must pass. A failed one refuses - `required_check_failed`; one still running or not yet reported answers `unevaluated` - (`required_check_pending` / `required_check_missing`, `Retry-After: 300`), never counted - toward the unevaluated bound, and refused `required_check_timed_out` 24 hours after the - story entered `ci`. A checkpoint that changes `.github/workflows/` or `.github/actions/` - is refused `ci_definition_changed` for a human, because Actions runs the workflow files of - the commit under test. A failed read is `ci_evidence_unavailable`. The list is read from + wait. The merge gate trusts ONLY the jobs of the GitHub Actions workflow runs that a PUSH + of the thread branch at the checkpoint's exact commit triggered (the Actions runs and jobs + APIs; the gate's `GITHUB_TOKEN` needs `actions: read`), and a job passes only by + concluding `success` — a skipped job fails. Commit statuses and check runs created any + other way are never trusted, because the implementer can create them; a CI that reports + only statuses cannot satisfy thread mode. Per name, the newest run of each workflow counts + and every workflow must pass. A failed one refuses `required_check_failed`; one still + running or not yet reported answers `unevaluated` (`required_check_pending` / + `required_check_missing`), never counted toward the unevaluated bound, and refused + `required_check_timed_out` once the wait passes `MergePrecondition.ci_wait_limit_seconds/0` + from the story's entry into `ci`. A checkpoint that changes `.github/workflows/` or + `.github/actions/` is refused `ci_definition_changed` for a human (`ci_definition_unknown` + when its diff could not be listed), because Actions runs the workflow files of the commit + under test. A failed read is `ci_evidence_unavailable`. The list is read from the source live, so correcting it reaches stories already in flight. A `local-gate` status is recorded and never satisfies a required check. What was read is returned as `ci_evidence` and copied onto the checkpoint's `gate_evidence` under `ci`; an diff --git a/deploy/FLY_SECRETS.md b/deploy/FLY_SECRETS.md index 69d0180b..eb84b96e 100644 --- a/deploy/FLY_SECRETS.md +++ b/deploy/FLY_SECRETS.md @@ -179,7 +179,7 @@ during an incident with `fly secrets set … && fly apps restart` — no deploy. | Variable | Default | Description | |----------------|---------|-------------| -| `GITHUB_TOKEN` | - | Bearer token for the CI status/test-result lookups that back independent story verification, AND (#803) for the merge precondition's reads of a pull request's state, diffstat, changed names and file tree. Optional: unset, the calls go out unauthenticated, which works for PUBLIC repos until GitHub's 60-requests/hour/IP anonymous limit bites — after that verification reports a `github_api_error` rather than a real CI verdict, and the merge precondition answers `unevaluated` (HTTP 503) rather than merging anything — an exhausted quota is transient, so the loop RETRIES rather than escalating, and only escalates once the same story has been unevaluable at one head several times running. Either way nothing merges without the token. Required for a private repo, where unauthenticated lookups 404. Needs only read access to checks, pull requests and contents. A blank value is treated as unset (it is trimmed), so a templated-but-empty secret degrades to the anonymous path rather than sending an empty bearer that GitHub 401s. **Since US-45.6 a THREAD-mode source also needs `checks: read` and `commit statuses: read`**: the merge gate reads CI for a checkpoint's exact commit from both APIs, and without them the read is refused `ci_evidence_unavailable` and nothing thread-mode merges (a pr-mode source never reads them). **Since #805 it also needs `issues: write`** — the only WRITE scope loopctl asks for. That is what lets the delivery loop close the GitHub issue a story came from, with a `loopctl:resolution-*` label and a comment naming the outcome; without it those calls 403, which is NOT a rate limit and NOT transient, so the closure is recorded `abandoned` with `permanent_forge_failure` on its first attempt and the reporter is never told what happened to her issue. Nothing else breaks and nothing is retried in a loop. **Recovering the backlog after you fix the token:** `SELECT abandoned_reason, date_trunc('hour', updated_at) AS at, count(*) FROM intake_issue_closures WHERE status = 'abandoned' GROUP BY 1, 2 ORDER BY 2 DESC;` shows the damage and WHEN it happened. Then, from a remote console, count before you write and requeue only the window you just fixed — `IssueClosures.requeue_abandoned(abandoned_after: ~U[YYYY-MM-DDThh:mm:00Z], dry_run: true)`, then the same call without `dry_run`. **An unbounded call is refused** (`{:error, :bound_required}`) and `tenant_id:` is not a bound: a closure abandoned months ago by an unrelated outage still names a live issue, and waking it puts a fresh label, comment and close on a ticket the reporter has long since moved on from. Pass `unbounded: true` to mean it. It never re-drives a `closed_by_other` or `source_revoked` row — a human already closed that issue, or the tenant disconnected the repository | +| `GITHUB_TOKEN` | - | Bearer token for the CI status/test-result lookups that back independent story verification, AND (#803) for the merge precondition's reads of a pull request's state, diffstat, changed names and file tree. Optional: unset, the calls go out unauthenticated, which works for PUBLIC repos until GitHub's 60-requests/hour/IP anonymous limit bites — after that verification reports a `github_api_error` rather than a real CI verdict, and the merge precondition answers `unevaluated` (HTTP 503) rather than merging anything — an exhausted quota is transient, so the loop RETRIES rather than escalating, and only escalates once the same story has been unevaluable at one head several times running. Either way nothing merges without the token. Required for a private repo, where unauthenticated lookups 404. Needs only read access to checks, pull requests and contents. A blank value is treated as unset (it is trimmed), so a templated-but-empty secret degrades to the anonymous path rather than sending an empty bearer that GitHub 401s. **Since US-45.6 a THREAD-mode source also needs `actions: read`**: the merge gate reads the workflow runs a push of the thread branch triggered, and their jobs, for a checkpoint's exact commit, and without it the read is refused `ci_evidence_unavailable` and nothing thread-mode merges. `commit statuses: read` is read best effort, only to record the `local-gate` state. A pr-mode source never reads either. **Since #805 it also needs `issues: write`** — the only WRITE scope loopctl asks for. That is what lets the delivery loop close the GitHub issue a story came from, with a `loopctl:resolution-*` label and a comment naming the outcome; without it those calls 403, which is NOT a rate limit and NOT transient, so the closure is recorded `abandoned` with `permanent_forge_failure` on its first attempt and the reporter is never told what happened to her issue. Nothing else breaks and nothing is retried in a loop. **Recovering the backlog after you fix the token:** `SELECT abandoned_reason, date_trunc('hour', updated_at) AS at, count(*) FROM intake_issue_closures WHERE status = 'abandoned' GROUP BY 1, 2 ORDER BY 2 DESC;` shows the damage and WHEN it happened. Then, from a remote console, count before you write and requeue only the window you just fixed — `IssueClosures.requeue_abandoned(abandoned_after: ~U[YYYY-MM-DDThh:mm:00Z], dry_run: true)`, then the same call without `dry_run`. **An unbounded call is refused** (`{:error, :bound_required}`) and `tenant_id:` is not a bound: a closure abandoned months ago by an unrelated outage still names a live issue, and waking it puts a fresh label, comment and close on a ticket the reporter has long since moved on from. Pass `unbounded: true` to mean it. It never re-drives a `closed_by_other` or `source_revoked` row — a human already closed that issue, or the tenant disconnected the repository | #### Post-deploy verification (#803 §9) diff --git a/docs/agent-delivery-loop.md b/docs/agent-delivery-loop.md index afe9574b..fa352bf9 100644 --- a/docs/agent-delivery-loop.md +++ b/docs/agent-delivery-loop.md @@ -122,7 +122,7 @@ Before a merge, an orchestrator or operator calls `merge_precondition` (`POST /a | `head_moved` | The pull request's head changed since CI ran. The story goes back to `implementing` over `base_moved`, and the recorded head is cleared, so the gate is not called again until the story is back at `ci`. | | `unevaluated` | 503 with `Retry-After`. After repeated `unevaluated` answers the story escalates. | -**Thread mode (epic 45, US-45.4).** A story whose claim was PLACED under an intake source with `mode: thread` (`intake_source_update`) has no pull request. The mode, and the base branch, are recorded on the implement dispatch at placement, so changing the source affects only stories placed afterwards; a change is always allowed. The gate judges the latest checkpoint the story's CURRENT claim recorded, on the branch that claim's dispatch ran on and against the base branch it was placed on, needs no `pr_number`, and also refuses `empty_change` (the checkpoint's tree equals the base's, or no file changed), `checkpoint_tree_mismatch`, `no_checkpoint_recorded`, `claim_ended` (the current claim recorded nothing but an earlier, released one did), and `thread_unreadable` (loopctl could not read the thread). The branch is judged first: a branch missing from a readable repository (`branch_missing`), one naming a commit nobody reported (`branch_head_unrecorded`), one naming an earlier checkpoint of the claim (`branch_head_regressed`), or a checkpoint that is not the recorded head means the head moved. The diff judged is the checkpoint's three-dot diff against its merge base, so a base that moved on is not a refusal, and a checkpoint the base already contains, its branch deleted or not, is `already_merged` only under a recorded allow naming it, and otherwise refused `checkpoint_on_base_without_allow`. A claim with no accepted dispatch (a session that claimed the story itself) is judged as a pull request against the source's current base branch, whatever the source's mode. **While the claim is live** that is `head_moved`, back to `implementing` like any moved head. **When it is not live** (reported, review requested, lease expired) the claimant cannot record a fix, so the gate refuses naming `claim_not_live` and the story escalates instead of looping. A repository the token cannot read refuses `pull_request_unavailable`. An allow is recorded naming the checkpoint id and sha and `base_sha`, the merge base the judged diff is relative to. **Thread mode needs the source's runners at runner contract 1.20.0 or later sending `checkpoint` messages; otherwise every story is refused `no_checkpoint_recorded`.** **CI is read by the checkpoint's exact SHA (US-45.6)**, from both the check-runs and the commit-status APIs (only a GitHub Actions check run can satisfy a required check; a commit status, which anyone with `statuses: write` could post, is recorded and never trusted), against the source's `required_checks` (a thread source must name at least one; set with `intake_source_update`): a failed one refuses `required_check_failed`; one still running or not yet reported answers `unevaluated` with a 300-second retry and never counts toward the unevaluated bound; 24 hours after the story entered `ci` both are refused `required_check_timed_out`, so a slow pipeline waits and a stuck or missing check still reaches a human. Per name the latest run of each check suite counts and every suite must pass; the required checks are the source's current list, so each required job must run on every push to the thread branches; and a checkpoint changing `.github/workflows/` or `.github/actions/` is refused `ci_definition_changed` for a human, because Actions runs the workflow files of the commit under test; a source requiring none refuses `required_checks_unset`. A `local-gate` status is recorded on the checkpoint and never satisfies a required check, because whoever pushed posts it. What was read is copied onto the checkpoint's `gate_evidence` under `ci`. The merge executor (US-45.5) merges only while the base head still equals that `base_sha`, and otherwise takes its base-update path (US-45.5). This gate judges claimant checkpoints only; reading the executor's `base_update` checkpoints is US-45.5's (AC-45.5.9). +**Thread mode (epic 45, US-45.4).** A story whose claim was PLACED under an intake source with `mode: thread` (`intake_source_update`) has no pull request. The mode, and the base branch, are recorded on the implement dispatch at placement, so changing the source affects only stories placed afterwards; a change is always allowed. The gate judges the latest checkpoint the story's CURRENT claim recorded, on the branch that claim's dispatch ran on and against the base branch it was placed on, needs no `pr_number`, and also refuses `empty_change` (the checkpoint's tree equals the base's, or no file changed), `checkpoint_tree_mismatch`, `no_checkpoint_recorded`, `claim_ended` (the current claim recorded nothing but an earlier, released one did), and `thread_unreadable` (loopctl could not read the thread). The branch is judged first: a branch missing from a readable repository (`branch_missing`), one naming a commit nobody reported (`branch_head_unrecorded`), one naming an earlier checkpoint of the claim (`branch_head_regressed`), or a checkpoint that is not the recorded head means the head moved. The diff judged is the checkpoint's three-dot diff against its merge base, so a base that moved on is not a refusal, and a checkpoint the base already contains, its branch deleted or not, is `already_merged` only under a recorded allow naming it, and otherwise refused `checkpoint_on_base_without_allow`. A claim with no accepted dispatch (a session that claimed the story itself) is judged as a pull request against the source's current base branch, whatever the source's mode. **While the claim is live** that is `head_moved`, back to `implementing` like any moved head. **When it is not live** (reported, review requested, lease expired) the claimant cannot record a fix, so the gate refuses naming `claim_not_live` and the story escalates instead of looping. A repository the token cannot read refuses `pull_request_unavailable`. An allow is recorded naming the checkpoint id and sha and `base_sha`, the merge base the judged diff is relative to. **Thread mode needs the source's runners at runner contract 1.20.0 or later sending `checkpoint` messages; otherwise every story is refused `no_checkpoint_recorded`.** **CI is read by the checkpoint's exact SHA (US-45.6)**, from both the check-runs and the commit-status APIs (only a job of a GitHub Actions workflow run that a push of the thread branch at that commit triggered satisfies a required check, and only by concluding `success` — a skipped job fails; commit statuses and check runs created any other way are never trusted, because the implementer can create them), against the source's `required_checks` (a thread source must name at least one; set with `intake_source_update`): a failed one refuses `required_check_failed`; one still running or not yet reported answers `unevaluated` with a `Retry-After` and never counts toward the unevaluated bound; past the gate's CI wait limit from the story's entry into `ci` both are refused `required_check_timed_out`, so a slow pipeline waits and a stuck or missing check still reaches a human. Per name the newest run of each workflow counts and every workflow must pass; the required checks are the source's current list, so each required job must run on every push to the thread branches; and a checkpoint changing `.github/workflows/` or `.github/actions/` is refused `ci_definition_changed` for a human, because Actions runs the workflow files of the commit under test; a source requiring none refuses `required_checks_unset`. A `local-gate` status is recorded on the checkpoint and never satisfies a required check, because whoever pushed posts it. What was read is copied onto the checkpoint's `gate_evidence` under `ci`. The merge executor (US-45.5) merges only while the base head still equals that `base_sha`, and otherwise takes its base-update path (US-45.5). This gate judges claimant checkpoints only; reading the executor's `base_update` checkpoints is US-45.5's (AC-45.5.9). ## 7. After the merge diff --git a/lib/loopctl/delivery/ci_evidence.ex b/lib/loopctl/delivery/ci_evidence.ex index 790bc709..7ba1302b 100644 --- a/lib/loopctl/delivery/ci_evidence.ex +++ b/lib/loopctl/delivery/ci_evidence.ex @@ -1,80 +1,73 @@ defmodule Loopctl.Delivery.CiEvidence do @moduledoc """ - What CI says about ONE commit, judged against a list of required checks (US-45.6). Pure. + What CI said about ONE commit of a thread, judged against the required checks (US-45.6). + Pure. - A thread-mode story merges the checkpoint's exact commit with no pull request, so the merge - gate cannot lean on a forge rule to hold the merge to green CI: it reads the evidence for - the checkpoint's SHA itself (`Loopctl.Delivery.PullRequestSource.check_evidence/3`) and - this module decides what it says. + A thread-mode story merges the checkpoint's exact commit with no pull request, so no forge + rule holds the merge to green CI: the merge gate reads the evidence itself + (`Loopctl.Delivery.PullRequestSource.check_evidence/3`) and this module decides what it + says. - ## What may satisfy a required check: a GitHub Actions check run, and nothing else + ## What may satisfy a required check: a job of the thread's own push run - Both the check-runs and the commit-status APIs are READ, by SHA, and both are recorded on - the checkpoint — GitHub Actions reports check runs, which the combined status never lists. - But only a CHECK RUN created by GitHub Actions (`@trusted_check_apps`) can satisfy a - required name (US-45.6 review round 2, finding 1). A commit status can be posted by anyone - holding `statuses: write` on the repository, which includes the implementer's own runner - (it posts `local-gate`), so letting a status satisfy a required name let the implementer - post `test = success` over a failing run and merge its own work — the self-attestation this - gate exists to refuse, one name over from `local-gate`. A check run needs a GitHub App to - create; the implementer holds none. Statuses are therefore evidence to READ, never to trust. + Only a JOB of a GitHub Actions WORKFLOW RUN that a PUSH of the thread's branch at exactly + this commit triggered. Anything weaker is something the implementer can produce itself: - Evidence for ANY OTHER commit never counts: nothing here reads a branch, a parent or a pull - request, only the one SHA the caller names. + - a commit STATUS can be posted by anyone holding `statuses: write`, which includes the + implementer's runner (it posts `local-gate`) + - a CHECK RUN, even one attributed to the `github-actions` app, can be created on any commit + under any name by a workflow on ANOTHER branch the implementer pushed, through its + `GITHUB_TOKEN` + - a workflow run triggered some other way (`workflow_dispatch`, a different branch) runs + workflow files the checkpoint's review never saw + + A push run of the thread branch runs the workflow files of the checkpoint's own tree, and a + checkpoint that CHANGES those files is refused before this is consulted + (`ci_definition_changed` in `Loopctl.Delivery.MergePrecondition`), so the definitions a + trusted job ran are the base's. + + What those jobs EXECUTE is repository code — test files, scripts — which the implementer + writes, exactly as it writes the change itself. Tampering there is a defect in the change, + and the review of the thread (US-45.3) is what judges it; this module proves that the + required jobs ran on this commit and passed, not that they test the right thing. ## How one required check is judged - Trusted runs under a name are grouped by CHECK SUITE (one workflow run's suite), and in - each suite only the LATEST run counts — the highest id, because ids only grow and a re-run - queued a moment ago has no timestamp yet — so a re-run that went green supersedes the - failure it re-ran. ACROSS suites nothing supersedes anything: two workflows that each have - a job `test` are two checks under one name, and a green one must never hide a red one. So - a name is `:failed` when any suite's latest run failed, `:pending` when none failed and - any is still running, and `:passed` only when every suite's latest run passed. Each run is: + Per workflow file, only the job with the name and the highest id counts: job ids only grow, + so that is the newest attempt of the newest run, and a re-run that went green supersedes + the failure it re-ran (the jobs read is `filter=latest` as well). ACROSS workflows nothing supersedes anything: two workflows + with a job `test` are two checks under one name, and a green one must never hide a red one. + So a name is `:failed` when any workflow's job failed, `:pending` when none failed and any + is still running, `:passed` only when every one passed, and `:missing` when no trusted job + carries it. A job: - not `completed` is `:pending` - - `completed` concluding `success`, `neutral` or `skipped` (what GitHub itself counts as - passing a required check) is `:passed` - - any other conclusion (`failure`, `cancelled`, `timed_out`, `action_required`, `stale`) is - `:failed` - - no trusted run under that name is `:missing`, whatever statuses say + - `completed` with conclusion `success` is `:passed` — and NOTHING ELSE is. A job GitHub + `skipped` did not run: a required job skipped because a job it `needs:` failed, or because + an `if:` the commit controls said so, must never read as green + - any other conclusion is `:failed`, naming it ## The local gate is recorded, never trusted - `local-gate` (`Loopctl.Intake.Source.local_gate/0`) is reported under `local_gate` and never - looked up as a required check, even by a caller that lists it: the intake source refuses to - store it, and this module drops it from the list as the backstop. + `local-gate` (`Loopctl.Intake.Source.local_gate/0`) is read from the commit statuses, best + effort, and reported under `local_gate` (`"unread"` when the statuses could not be read). It + is never looked up as a required check, even by a caller that lists it: the intake source + refuses to store it, and this module drops it from the list as the backstop. """ alias Loopctl.Intake.Source - @passing_conclusions ["success", "neutral", "skipped"] - - # The apps whose check runs may satisfy a required check. See the moduledoc. - @trusted_check_apps ["github-actions"] - - @doc "The GitHub App slugs whose check runs may satisfy a required check." - @spec trusted_check_apps() :: [String.t()] - def trusted_check_apps, do: @trusted_check_apps - - @type check_run :: %{ + @type job :: %{ required(:name) => String.t(), required(:status) => String.t(), required(:conclusion) => String.t() | nil, optional(:id) => integer() | nil, - optional(:app) => String.t() | nil, - optional(:check_suite) => integer() | nil, - optional(:started_at) => String.t() | nil, - optional(:completed_at) => String.t() | nil, - optional(:url) => String.t() | nil - } - @type status :: %{ - required(:context) => String.t(), - required(:state) => String.t(), - optional(:at) => String.t() | nil, + optional(:run_id) => integer() | nil, + optional(:workflow) => String.t() | nil, optional(:url) => String.t() | nil } - @type evidence :: %{check_runs: [check_run()], statuses: [status()]} + @type status :: %{required(:context) => String.t(), required(:state) => String.t()} + @type evidence :: %{jobs: [job()], statuses: [status()] | {:unread, term()}} @type result :: %{ passed: [String.t()], pending: [String.t()], @@ -89,14 +82,14 @@ defmodule Loopctl.Delivery.CiEvidence do @doc "Judges `evidence` for one commit against `required`. See the moduledoc." @spec judge([String.t()], evidence()) :: result() - def judge(required, %{check_runs: runs, statuses: statuses}) do + def judge(required, %{jobs: jobs, statuses: statuses}) do acc = %{passed: [], pending: [], missing: [], failed: []} judged = required |> lookup_names() |> Enum.reduce(acc, fn name, acc -> - case check_state(name, runs) do + case check_state(name, jobs) do {:failed, conclusion} -> Map.update!(acc, :failed, &[{name, conclusion} | &1]) state -> Map.update!(acc, state, &[name | &1]) end @@ -106,13 +99,13 @@ defmodule Loopctl.Delivery.CiEvidence do Map.put(judged, :local_gate, local_gate_state(statuses)) end - defp check_state(name, runs) do + defp check_state(name, jobs) do states = - runs - |> Enum.filter(&(&1.name == name and Map.get(&1, :app) in @trusted_check_apps)) - |> Enum.group_by(&Map.get(&1, :check_suite)) - |> Enum.map(fn {_suite, suite_runs} -> - suite_runs |> Enum.max_by(&(Map.get(&1, :id) || 0)) |> run_state() + jobs + |> Enum.filter(&(&1.name == name)) + |> Enum.group_by(&Map.get(&1, :workflow)) + |> Enum.map(fn {_workflow, named} -> + named |> Enum.max_by(&(Map.get(&1, :id) || 0)) |> job_state() end) cond do @@ -123,14 +116,14 @@ defmodule Loopctl.Delivery.CiEvidence do end end - defp run_state(%{status: "completed", conclusion: conclusion}) - when conclusion in @passing_conclusions, - do: :passed + defp job_state(%{status: "completed", conclusion: "success"}), do: :passed - defp run_state(%{status: "completed", conclusion: conclusion}), + defp job_state(%{status: "completed", conclusion: conclusion}), do: {:failed, conclusion || "none"} - defp run_state(_running), do: :pending + defp job_state(_running), do: :pending + + defp local_gate_state({:unread, _reason}), do: "unread" defp local_gate_state(statuses) do local_gate = Source.local_gate() @@ -145,35 +138,29 @@ defmodule Loopctl.Delivery.CiEvidence do The evidence and its judgement as stored on the checkpoint's `gate_evidence` under `"ci"` (AC-45.6.1): string keys, so it reads back as it was written. - Only what the judgement READ is kept: the runs under a required name and the `local-gate` - status. A commit can carry hundreds of unrelated runs, and keeping them made the record - change whenever any of them moved, so an unchanged judgement was rewritten on every poll. + Only what the judgement READ is kept: the jobs under a required name and the `local-gate` + state. A commit can carry many unrelated jobs, and keeping them made the record change + whenever any of them moved, so an unchanged judgement was rewritten on every poll. """ @spec to_record(String.t(), [String.t()], evidence(), result(), DateTime.t()) :: map() - def to_record(sha, required, %{check_runs: runs, statuses: statuses}, result, read_at) do + def to_record(sha, required, %{jobs: jobs}, result, read_at) do names = lookup_names(required) - runs = Enum.filter(runs, &(&1.name in names)) - statuses = Enum.filter(statuses, &(&1.context == Source.local_gate())) %{ "sha" => sha, "read_at" => fixed_width_iso8601(read_at), "required" => required, - "check_runs" => - Enum.map(runs, fn run -> + "jobs" => + for job <- jobs, job.name in names do %{ - "name" => run.name, - "app" => Map.get(run, :app), - "check_suite" => Map.get(run, :check_suite), - "status" => run.status, - "conclusion" => run.conclusion, - "url" => Map.get(run, :url) + "name" => job.name, + "workflow" => Map.get(job, :workflow), + "run_id" => Map.get(job, :run_id), + "status" => job.status, + "conclusion" => job.conclusion, + "url" => Map.get(job, :url) } - end), - "statuses" => - Enum.map(statuses, fn status -> - %{"context" => status.context, "state" => status.state, "url" => Map.get(status, :url)} - end), + end, "local_gate" => result.local_gate, "passed" => result.passed, "pending" => result.pending, @@ -183,8 +170,8 @@ defmodule Loopctl.Delivery.CiEvidence do end # ALWAYS six fractional digits, so two records order correctly as TEXT: the evidence write - # compares `read_at` in SQL (`Loopctl.Threads.record_gate_evidence/5`), and - # `"...:00Z"` sorts after `"...:00.5Z"` although it is earlier. + # compares `read_at` (`Loopctl.Threads.record_gate_evidence/5`), and `"...:00Z"` sorts after + # `"...:00.5Z"` although it is earlier. defp fixed_width_iso8601(%DateTime{microsecond: {micro, _precision}} = at), do: DateTime.to_iso8601(%{at | microsecond: {micro, 6}}) end diff --git a/lib/loopctl/delivery/github_pull_request_source.ex b/lib/loopctl/delivery/github_pull_request_source.ex index b252da25..a806872c 100644 --- a/lib/loopctl/delivery/github_pull_request_source.ex +++ b/lib/loopctl/delivery/github_pull_request_source.ex @@ -26,13 +26,14 @@ defmodule Loopctl.Delivery.GitHubPullRequestSource do CI evidence for a thread checkpoint (US-45.6), by the checkpoint's exact SHA: - - `GET /repos/:repo/commits/:sha/check-runs?filter=latest&per_page=100&page=:n` — every - check run on the commit, paged up to `@check_run_pages`; the required names are matched - locally - - `GET /repos/:repo/commits/:sha/status?per_page=100` — the latest status per context. - Both, because the combined status never lists check runs (GitHub Actions) and the - check-runs API never lists statuses. A list reporting more entries than it carried is - refused as truncated: a failure on a missing page would read as a pass + - `GET /repos/:repo/actions/runs?head_sha=:sha&branch=:branch&event=push` — the workflow + runs a PUSH of the thread branch at exactly this commit triggered: the only runs whose + jobs may satisfy a required check + - `GET /repos/:repo/actions/runs/:id/jobs?filter=latest` — one per such run, its jobs' latest + attempt + - `GET /repos/:repo/commits/:sha/status?per_page=100` — best effort, for the recorded + `local-gate` state only. A list reporting more entries than it carried is refused as + truncated Post-deploy verification (#803 §9) adds three more, each bounded the same way: @@ -68,9 +69,9 @@ defmodule Loopctl.Delivery.GitHubPullRequestSource do Three calls for `pull_request/2` and one for `repo_files/2`, so a precondition that makes all five (a pull request plus two refs) waits at most 35 seconds before it has an answer, and the answer to a timeout is an ESCALATION, never a pass. A THREAD-mode evaluation makes - more: the branch ref, the commit, the comparison, two trees, at most `@check_run_pages` - pages of check runs and the combined status — nine calls, 63 seconds at the most, plus one - repository read after a 404 on the branch. Nothing here runs inside a + more: the branch ref, the commit, the comparison, two trees, the workflow runs, one jobs + read per workflow run and the combined status — about eight calls for a repository with a + couple of workflows, plus one repository read after a 404 on the branch. Nothing here runs inside a database transaction: the caller gathers every fact before it opens one, so a slow forge never holds a pooled connection. @@ -120,8 +121,11 @@ defmodule Loopctl.Delivery.GitHubPullRequestSource do @files_per_page 100 # The combined status lists the LATEST status per context, one page of them. @status_page_size 100 - # At most this many pages of 100 check runs per commit are read (US-45.6). - @check_run_pages 3 + # US-45.6: the push-triggered workflow runs of one commit fit one page, and so do one run's + # jobs; more is refused as truncated rather than judged on a part. + @workflow_run_page 100 + @job_page 100 + @max_workflow_runs 10 # GitHub's primary rate-limit window is an hour. Anything beyond that plus slack is not a # window rolling over, so it is not turned into a delay a caller would sleep on. @@ -209,94 +213,138 @@ defmodule Loopctl.Delivery.GitHubPullRequestSource do end @impl true - def check_evidence(repo, sha) do + def check_evidence(repo, sha, branch) do with {:ok, repo} <- repo_name(repo), {:ok, sha} <- ref(sha), - {:ok, runs} <- check_runs_page(repo, sha, 1, []), - {:ok, body} <- get(repo, "/commits/#{sha}/status?per_page=#{@status_page_size}"), - {:ok, statuses} <- commit_statuses(body) do - {:ok, %{check_runs: runs, statuses: statuses}} + {:ok, branch} <- ref(branch), + {:ok, runs} <- push_runs(repo, sha, branch), + {:ok, jobs} <- jobs_of(repo, runs) do + {:ok, %{jobs: jobs, statuses: commit_statuses(repo, sha)}} + end + end + + # The WORKFLOW RUNS a push of `branch` at exactly `sha` triggered — the only runs whose jobs + # can satisfy a required check (`Loopctl.Delivery.CiEvidence`). Filtered by the API and then + # again here, so a filter the forge ignored cannot widen what is trusted. + defp push_runs(repo, sha, branch) do + query = + URI.encode_query(%{ + "head_sha" => sha, + "branch" => branch, + "event" => "push", + "per_page" => @workflow_run_page + }) + + with {:ok, body} <- get(repo, "/actions/runs?" <> query), + {:ok, runs} <- workflow_runs(body) do + {:ok, + Enum.filter(runs, fn run -> + run["head_sha"] == sha and run["head_branch"] == branch and run["event"] == "push" + end)} + |> bounded_runs() end end - # EVERY check run on the commit, `filter=latest`, paged — never one call per required name - # (US-45.6 review round 1, finding 7): a request per name multiplied both the worst-case - # wait and the forge calls each poll spends by the length of the list. The names are - # matched here, by `CiEvidence`. A commit carrying more runs than `@check_run_pages` pages - # hold is refused as truncated rather than judged on the part that fitted. - defp check_runs_page(repo, sha, page, acc) do - query = URI.encode_query(%{"filter" => "latest", "per_page" => 100, "page" => page}) + # One jobs read per run: a push that triggered more workflows than this is refused rather + # than costing an unbounded number of calls on every poll. + defp bounded_runs({:ok, runs}) when length(runs) > @max_workflow_runs, + do: {:error, {:too_many_workflow_runs, length(runs)}} - with {:ok, body} <- get(repo, "/commits/#{sha}/check-runs?" <> query), - {:ok, total, runs} <- check_runs(body) do - acc = acc ++ runs + defp bounded_runs(ok), do: ok - cond do - length(acc) >= total or runs == [] -> complete_runs(acc, total) - page >= @check_run_pages -> {:error, {:check_runs_truncated, total, length(acc)}} - true -> check_runs_page(repo, sha, page + 1, acc) - end + defp workflow_runs(%{"total_count" => total, "workflow_runs" => runs}) + when is_integer(total) and is_list(runs) do + cond do + not Enum.all?( + runs, + &match?(%{"id" => id, "path" => path} when is_integer(id) and is_binary(path), &1) + ) -> + {:error, {:unreadable_workflow_runs, shape(runs)}} + + total > length(runs) -> + {:error, {:workflow_runs_truncated, total, length(runs)}} + + true -> + {:ok, runs} end end - defp complete_runs(runs, total) when length(runs) >= total, do: {:ok, runs} - defp complete_runs(runs, total), do: {:error, {:check_runs_truncated, total, length(runs)}} + defp workflow_runs(body), do: {:error, {:unreadable_workflow_runs, shape(body)}} - defp check_runs(%{"total_count" => total, "check_runs" => runs}) - when is_integer(total) and is_list(runs) do - if Enum.all?(runs, &check_run?/1), - do: {:ok, total, Enum.map(runs, &check_run_fact/1)}, - else: {:error, {:unreadable_check_runs, shape(runs)}} + # Each run's jobs, latest attempt only (`filter=latest`), tagged with the run and its + # workflow file. De-duplicated by job id and checked against the total, so a page boundary + # that moved between reads can neither repeat a job nor hide one. + defp jobs_of(repo, runs) do + Enum.reduce_while(runs, {:ok, []}, fn run, {:ok, acc} -> + query = URI.encode_query(%{"filter" => "latest", "per_page" => @job_page}) + + with {:ok, body} <- get(repo, "/actions/runs/#{run["id"]}/jobs?" <> query), + {:ok, jobs} <- run_jobs(body, run) do + {:cont, {:ok, acc ++ jobs}} + else + {:error, _reason} = error -> {:halt, error} + end + end) + end + + defp run_jobs(%{"total_count" => total, "jobs" => jobs}, run) + when is_integer(total) and is_list(jobs) do + jobs = Enum.uniq_by(jobs, & &1["id"]) + + cond do + not Enum.all?(jobs, &job?/1) -> + {:error, {:unreadable_jobs, shape(jobs)}} + + total > length(jobs) -> + {:error, {:jobs_truncated, total, length(jobs)}} + + true -> + {:ok, Enum.map(jobs, &job_fact(&1, run))} + end end - defp check_runs(body), do: {:error, {:unreadable_check_runs, shape(body)}} + defp run_jobs(body, _run), do: {:error, {:unreadable_jobs, shape(body)}} + + defp job?(%{"id" => id, "name" => name, "status" => status} = job) + when is_integer(id) and is_binary(name) and is_binary(status), + do: is_nil(job["conclusion"]) or is_binary(job["conclusion"]) - defp check_run_fact(run) do + defp job?(_job), do: false + + defp job_fact(job, run) do %{ - id: run["id"], - # The creating App's slug: only a trusted App's run can satisfy a required check - # (`Loopctl.Delivery.CiEvidence`). - app: get_in(run, ["app", "slug"]), - # The suite (one workflow run) the run belongs to: runs are judged latest-per-suite. - check_suite: get_in(run, ["check_suite", "id"]), - name: run["name"], - status: run["status"], - conclusion: run["conclusion"], - started_at: run["started_at"], - completed_at: run["completed_at"], - url: run["html_url"] + id: job["id"], + name: job["name"], + status: job["status"], + conclusion: job["conclusion"], + run_id: run["id"], + workflow: run["path"], + url: job["html_url"] } end - defp check_run?(%{"name" => name, "status" => status} = run) - when is_binary(name) and is_binary(status), - do: is_nil(run["conclusion"]) or is_binary(run["conclusion"]) - - defp check_run?(_run), do: false + # The commit statuses, BEST EFFORT: they feed only the recorded `local-gate` field and never + # a decision, so a token without `commit statuses: read`, or a commit carrying more contexts + # than one page, records them as unread rather than failing the evaluation. + defp commit_statuses(repo, sha) do + with {:ok, body} <- get(repo, "/commits/#{sha}/status?per_page=#{@status_page_size}"), + {:ok, statuses} <- statuses_of(body) do + statuses + else + {:error, reason} -> {:unread, reason} + end + end - defp commit_statuses(%{"total_count" => total, "statuses" => statuses}) + defp statuses_of(%{"total_count" => total, "statuses" => statuses}) when is_integer(total) and is_list(statuses) do cond do - total > length(statuses) -> - {:error, {:statuses_truncated, total, length(statuses)}} - - Enum.all?(statuses, &status?/1) -> - {:ok, - Enum.map(statuses, fn status -> - %{ - context: status["context"], - state: status["state"], - at: status["updated_at"], - url: status["target_url"] - } - end)} - - true -> - {:error, {:unreadable_statuses, shape(statuses)}} + not Enum.all?(statuses, &status?/1) -> {:error, {:unreadable_statuses, shape(statuses)}} + total > length(statuses) -> {:error, {:statuses_truncated, total, length(statuses)}} + true -> {:ok, Enum.map(statuses, &%{context: &1["context"], state: &1["state"]})} end end - defp commit_statuses(body), do: {:error, {:unreadable_statuses, shape(body)}} + defp statuses_of(body), do: {:error, {:unreadable_statuses, shape(body)}} defp status?(%{"context" => context, "state" => state}) when is_binary(context) and is_binary(state), diff --git a/lib/loopctl/delivery/merge_precondition.ex b/lib/loopctl/delivery/merge_precondition.ex index 1cb8c6ec..6bae0843 100644 --- a/lib/loopctl/delivery/merge_precondition.ex +++ b/lib/loopctl/delivery/merge_precondition.ex @@ -323,7 +323,7 @@ defmodule Loopctl.Delivery.MergePrecondition do # checkpoint's exact commit (`{:ok, nil}` where nothing was read). optional(:required_checks) => [String.t()], optional(:ci_evidence) => fact(map() | nil), - optional(:ci_entered_at) => DateTime.t() | nil, + optional(:ci_entered_at) => fact(DateTime.t() | nil), optional(:now) => DateTime.t() } @@ -616,13 +616,15 @@ defmodule Loopctl.Delivery.MergePrecondition do {:repo, :repository_unresolved}, {:checkpoint, :thread_unreadable}, {:pull_request, :pull_request_unavailable}, - {:ci_evidence, :ci_evidence_unavailable} + {:ci_evidence, :ci_evidence_unavailable}, + {:ci_entered_at, :ci_entry_unreadable} ] @forge_facts [ {:checkpoint, :thread_unreadable}, {:pull_request, :pull_request_unavailable}, {:ci_evidence, :ci_evidence_unavailable}, + {:ci_entered_at, :ci_entry_unreadable}, {:head_files, :head_files_unavailable}, {:base_files, :base_files_unavailable} ] @@ -716,7 +718,7 @@ defmodule Loopctl.Delivery.MergePrecondition do def ci_wait_limit_seconds, do: @ci_wait_limit_seconds defp ci_wait_exceeded?(facts) do - case {Map.get(facts, :ci_entered_at), Map.get(facts, :now)} do + case {value(facts, :ci_entered_at), Map.get(facts, :now)} do {%DateTime{} = entered, %DateTime{} = now} -> DateTime.diff(now, entered) > @ci_wait_limit_seconds @@ -730,6 +732,10 @@ defmodule Loopctl.Delivery.MergePrecondition do # only spends the shared token's rate limit (round 2, finding 6). @ci_wait_retry_after 300 + @doc "The `Retry-After` a CI wait answers with, in seconds (US-45.6)." + @spec ci_wait_retry_after() :: pos_integer() + def ci_wait_retry_after, do: @ci_wait_retry_after + # A CI wait holds back only an ALLOW (round 2, finding 3). Everything else the change was # judged on is decided now: a refusal (a tree mismatch, an empty change, the size bound, a # gate) or a moved head does not wait up to a day for CI to finish first. @@ -1009,6 +1015,10 @@ defmodule Loopctl.Delivery.MergePrecondition do end end + # A diff that could not be listed (truncated at the compare cap, unreadable) may touch CI + # definitions for all the gate can tell: refused as unknown here, by this guard, rather than + # left to whichever other rule happens to refuse an unreadable diff. + defp ci_definition_reasons({:error, reason}), do: [{:ci_definition_unknown, reason}] defp ci_definition_reasons(_no_diff), do: [] defp ci_definition?(name) when is_binary(name), @@ -1393,7 +1403,7 @@ defmodule Loopctl.Delivery.MergePrecondition do # which names the fix — setting the list again — and is not a dead end. required_checks: source_required_checks(source), # When the story entered `ci`: what a CI wait is measured from. - ci_entered_at: if(mode == :thread, do: Stages.entered_at(story.tenant_id, story.id, :ci)), + ci_entered_at: ci_entered_at(mode, story, checkpoint), now: DateTime.utc_now() } @@ -1412,21 +1422,43 @@ defmodule Loopctl.Delivery.MergePrecondition do }) end + # When the CI wait started: the story's entry into `ci`, read under a bounded lock wait + # (contention is `{:error, :busy}`, a retry). A story that reached `ci` with no recorded + # transition — a backfill, a repair — falls back to its checkpoint's recording, which is + # always known, so the wait is bounded for every story (round 1 of #910, finding 8). + defp ci_entered_at(:thread, story, checkpoint) do + case Stages.entered_at(story.tenant_id, story.id, :ci) do + {:ok, %DateTime{} = entered} -> {:ok, entered} + {:ok, nil} -> {:ok, checkpoint_recorded_at(checkpoint)} + {:error, _reason} = error -> error + end + end + + defp ci_entered_at(_mode, _story, _checkpoint), do: {:ok, nil} + + defp checkpoint_recorded_at({:ok, %{recorded_at: %DateTime{} = at}}), do: at + defp checkpoint_recorded_at(_checkpoint), do: nil + defp source_required_checks({:ok, %{required_checks: checks}}) when is_list(checks), do: checks defp source_required_checks(_no_source), do: [] # US-45.6: the evidence for the CHECKPOINT'S exact commit, never the branch head's or a # parent's. Read only where a thread is about to be judged: a moved head or a merged # checkpoint decides without it, and a source requiring nothing is refused without a read. - defp ci_evidence(%{mode: :thread} = facts, {:ok, %{merged?: false}}, false = _skip?) do + defp ci_evidence( + %{mode: :thread} = facts, + {:ok, %{merged?: false, thread_branch: branch}}, + false = _skip? + ) do names = CiEvidence.lookup_names(facts.required_checks) # A repository or checkpoint that could not be read is already refused as itself; only - # the evidence read's OWN failure is this fact's. + # the evidence read's OWN failure is this fact's. The BRANCH is the one the thread is + # judged on: only the runs a push of it triggered are trusted (`CiEvidence`). with {:ok, repo} <- facts.repo, {:ok, %{commit_sha: sha}} <- facts.checkpoint, [_ | _] <- names do - case source().check_evidence(repo, sha) do + case source().check_evidence(repo, sha, branch) do {:ok, evidence} -> {:ok, %{evidence: evidence, sha: sha, read_at: DateTime.utc_now()}} {:error, _reason} = error -> error end @@ -1437,6 +1469,12 @@ defmodule Loopctl.Delivery.MergePrecondition do defp ci_evidence(_facts, _pull_request, _skip?), do: {:ok, nil} + # The branch travels with the thread's facts so the CI read can name it. + defp with_thread_branch({:ok, %{} = facts}, branch), + do: {:ok, Map.put(facts, :thread_branch, branch)} + + defp with_thread_branch(other, _branch), do: other + defp fetch_story(tenant_id, story_id) do case Stories.get_story(tenant_id, story_id) do {:ok, story} -> {:ok, story} @@ -1470,12 +1508,13 @@ defmodule Loopctl.Delivery.MergePrecondition do # The branch is resolved HERE, for a thread only: a pull request names its own head. pull_request = with {:ok, branch} <- DispatchPayload.thread_branch(route, story, stage.branch) do - CheckpointSource.pull_request( - repo, + repo + |> CheckpointSource.pull_request( placed_base_branch(route, source), branch, checkpoint ) + |> with_thread_branch(branch) end {{:ok, nil}, {:ok, checkpoint_fact(checkpoint, claim.earlier_shas)}, pull_request} @@ -1514,6 +1553,8 @@ defmodule Loopctl.Delivery.MergePrecondition do defp checkpoint_fact(%Checkpoint{} = checkpoint, earlier_shas) do %{ id: checkpoint.id, + # The CI wait's fallback origin, for a story with no recorded entry into `ci`. + recorded_at: checkpoint.inserted_at, commit_sha: checkpoint.commit_sha, tree_sha: checkpoint.tree_sha, earlier_shas: earlier_shas diff --git a/lib/loopctl/delivery/merge_precondition/verdict.ex b/lib/loopctl/delivery/merge_precondition/verdict.ex index b02fbc20..f2b9bbe2 100644 --- a/lib/loopctl/delivery/merge_precondition/verdict.ex +++ b/lib/loopctl/delivery/merge_precondition/verdict.ex @@ -29,7 +29,7 @@ defmodule Loopctl.Delivery.MergePrecondition.Verdict do verified at. Known even when the forge cannot be reached, which is why the consecutive-unevaluated count is kept per THIS head rather than the forge's - `retry_after` — on `:unevaluated`, the seconds the forge asked a caller to wait, when it - said so at all, or loopctl's own 300 for a CI wait. The endpoint sends it as `Retry-After`; the dominant cause of an + said so at all, or `MergePrecondition.ci_wait_retry_after/0` for a CI wait. The endpoint sends it as `Retry-After`; the dominant cause of an unevaluated verdict is a rate limit, so an unbounded retry would amplify the very condition it is waiting out - `repo`, `pr_number`, `head_sha`, `merge_base_sha` — what was judged, server-resolved. diff --git a/lib/loopctl/delivery/pull_request_source.ex b/lib/loopctl/delivery/pull_request_source.ex index 98e9296f..332a54c6 100644 --- a/lib/loopctl/delivery/pull_request_source.ex +++ b/lib/loopctl/delivery/pull_request_source.ex @@ -202,13 +202,14 @@ defmodule Loopctl.Delivery.PullRequestSource do @callback compare(repo(), String.t(), String.t()) :: {:ok, comparison()} | {:error, term()} @doc """ - The CI evidence for ONE commit (US-45.6): EVERY latest check run on it (`filter=latest`), - each with the slug of the App that created it, and every commit status. The required - names are matched by `Loopctl.Delivery.CiEvidence`, never here, so this reads the same - thing whatever a source requires. A list the forge truncated is an error, never a partial - answer: a missing failure reads as a pass. + The CI evidence for ONE commit of a thread (US-45.6): the jobs (latest attempt) of every + workflow run that a PUSH of `branch` at exactly `sha` triggered, each tagged with its run + and workflow file, and — best effort, `{:unread, reason}` when they could not be read — the + commit statuses. The required names are matched by `Loopctl.Delivery.CiEvidence`, never + here. A job list the forge truncated is an error, never a partial answer: a missing + failure reads as a pass. """ - @callback check_evidence(repo(), String.t()) :: + @callback check_evidence(repo(), String.t(), String.t()) :: {:ok, Loopctl.Delivery.CiEvidence.evidence()} | {:error, term()} @doc "Every file the repository holds at `ref`." diff --git a/lib/loopctl/delivery/stages.ex b/lib/loopctl/delivery/stages.ex index 3b55033b..e7fd98f5 100644 --- a/lib/loopctl/delivery/stages.ex +++ b/lib/loopctl/delivery/stages.ex @@ -144,6 +144,7 @@ defmodule Loopctl.Delivery.Stages do alias Loopctl.LocalGuc alias Loopctl.Progress alias Loopctl.Repo + alias Loopctl.Runners.Capacity alias Loopctl.Runners.DispatchLedger alias Loopctl.Runners.Runner alias Loopctl.WorkBreakdown.Story @@ -309,13 +310,18 @@ defmodule Loopctl.Delivery.Stages do end @doc """ - When `story_id` last ENTERED `stage` — the newest transition into it — or nil when it never - has (US-45.6: what a merge gate's CI wait is measured from). + When `story_id` last ENTERED `stage` — the newest transition into it — as `{:ok, at}`, or + `{:ok, nil}` when it never has (US-45.6: what a merge gate's CI wait is measured from). Its + lock wait is bounded and contention is `{:error, :busy}`, counted as + `[:loopctl, :delivery, :stage_read_busy]`. """ - @spec entered_at(Ecto.UUID.t(), Ecto.UUID.t(), atom()) :: DateTime.t() | nil + @spec entered_at(Ecto.UUID.t(), Ecto.UUID.t(), atom()) :: + {:ok, DateTime.t() | nil} | {:error, term()} def entered_at(tenant_id, story_id, stage) do - {:ok, at} = + answering_busy(tenant_id, [:loopctl, :delivery, :stage_read_busy], "stage entry read", fn -> Repo.with_tenant(tenant_id, fn -> + Capacity.set_lock_timeout!(Repo) + Repo.one( from e in StageEvent, where: e.tenant_id == ^tenant_id and e.story_id == ^story_id, @@ -323,8 +329,7 @@ defmodule Loopctl.Delivery.Stages do select: max(e.inserted_at) ) end) - - at + end) end @doc "A story's stage events, oldest first." diff --git a/lib/loopctl/intake/source.ex b/lib/loopctl/intake/source.ex index b838321e..7bc521b9 100644 --- a/lib/loopctl/intake/source.ex +++ b/lib/loopctl/intake/source.ex @@ -215,7 +215,11 @@ defmodule Loopctl.Intake.Source do # `{:array, :string}` casts a JSON null element to nil, so the element type is checked # here, before anything calls a String function on it (a 500 otherwise). not Enum.all?(names, &usable_name?/1) -> - [required_checks: "each name must be 1 to #{@max_check_name_bytes} bytes and not blank"] + [ + required_checks: + "each name must be a string of 1 to #{@max_check_name_bytes} bytes with no " <> + "surrounding whitespace" + ] @local_gate in names -> [ @@ -232,8 +236,10 @@ defmodule Loopctl.Intake.Source do end end + # Surrounding whitespace is refused, not trimmed: a job name never carries it, so `"test "` + # could never match and every thread would wait out the CI limit on a typo. defp usable_name?(name) when is_binary(name), - do: String.trim(name) != "" and byte_size(name) <= @max_check_name_bytes + do: name != "" and String.trim(name) == name and byte_size(name) <= @max_check_name_bytes defp usable_name?(_not_a_string), do: false diff --git a/lib/loopctl/threads.ex b/lib/loopctl/threads.ex index f6986e10..7dd24048 100644 --- a/lib/loopctl/threads.ex +++ b/lib/loopctl/threads.ex @@ -210,7 +210,9 @@ defmodule Loopctl.Threads do commit_sha: c.commit_sha, tree_sha: c.tree_sha, claim_epoch: c.claim_epoch, - merge_commit_sha: c.merge_commit_sha + merge_commit_sha: c.merge_commit_sha, + # The merge gate's CI-wait fallback origin (US-45.6). + inserted_at: c.inserted_at }} ) diff --git a/lib/loopctl_web/controllers/intake_source_controller.ex b/lib/loopctl_web/controllers/intake_source_controller.ex index c1b3ee6c..969c7408 100644 --- a/lib/loopctl_web/controllers/intake_source_controller.ex +++ b/lib/loopctl_web/controllers/intake_source_controller.ex @@ -31,13 +31,14 @@ defmodule LoopctlWeb.IntakeSourceController do tags(["Intake"]) @required_checks_doc "The CI checks a THREAD-mode checkpoint must pass on its exact commit before " <> - "the merge gate allows it (US-45.6): GitHub Actions check-run names as " <> + "the merge gate allows it (US-45.6): GitHub Actions JOB names as " <> "they appear on the commit. A `thread` source must name at " <> "least one (422 otherwise, whichever of `mode` and `required_checks` " <> "the request named); `pr` mode never reads it. At most " <> "#{Source.max_required_checks()} distinct, non-blank names of at most " <> - "#{Source.max_check_name_bytes()} bytes. Only a GitHub Actions check " <> - "run satisfies one; a commit status is recorded, never trusted. Each " <> + "#{Source.max_check_name_bytes()} bytes, no surrounding whitespace. Only a " <> + "job of a GitHub Actions workflow run that a push of the thread " <> + "branch triggered, concluding `success`, satisfies one. Each " <> "required job must run on every push to the thread branches (no path " <> "filter or job-level `if:`): one that never appears is refused after " <> "the merge gate's CI wait. " <> diff --git a/lib/loopctl_web/controllers/merge_precondition_controller.ex b/lib/loopctl_web/controllers/merge_precondition_controller.ex index 3a7f2b92..32bb65d8 100644 --- a/lib/loopctl_web/controllers/merge_precondition_controller.ex +++ b/lib/loopctl_web/controllers/merge_precondition_controller.ex @@ -263,17 +263,22 @@ defmodule LoopctlWeb.MergePreconditionController do "`thread_unreadable`. It also refuses `empty_change` (the checkpoint's tree equals " <> "the base branch's, or no file changed) and `checkpoint_tree_mismatch` (the forge's " <> "tree for it is not the one recorded). CI is read by the checkpoint's EXACT SHA " <> - "(US-45.6), from both the check-runs and the commit-status APIs (only a GitHub " <> - "Actions check run satisfies a required check; statuses are recorded, never " <> - "trusted), against the " <> + "(US-45.6): only a job of a GitHub Actions workflow run that a PUSH of the thread " <> + "branch at that commit triggered satisfies a required check, and only by concluding " <> + "`success` (a skipped job fails); commit statuses are recorded, never trusted; " <> + "against the " <> "source's `required_checks`: a failed one refuses `required_check_failed`, one still " <> "running or not yet reported is `unevaluated` (`required_check_pending` / " <> - "`required_check_missing`, `Retry-After` 300; neither counts toward the unevaluated " <> - "bound, and 24 hours after the story entered `ci` both are refused " <> - "`required_check_timed_out`; per name the latest run of each check suite counts and " <> - "every suite must pass), the required checks are the source's current list, a " <> + "`required_check_missing`, `Retry-After` #{MergePrecondition.ci_wait_retry_after()}; " <> + "neither counts toward the unevaluated bound, and " <> + "#{div(MergePrecondition.ci_wait_limit_seconds(), 3600)} hours after the story " <> + "entered `ci` (or, with no recorded entry, the checkpoint was recorded) both are " <> + "refused " <> + "`required_check_timed_out`; per name the latest run of each workflow counts and " <> + "every workflow must pass), the required checks are the source's current list, a " <> "checkpoint changing `.github/workflows/` or `.github/actions/` is refused " <> - "`ci_definition_changed`, a source requiring none refuses `required_checks_unset`, a " <> + "`ci_definition_changed` (and one whose diff could not be listed " <> + "`ci_definition_unknown`), a source requiring none refuses `required_checks_unset`, a " <> "failed read is `ci_evidence_unavailable`, and a `local-gate` status is recorded but " <> "never satisfies a required check. `ci_evidence` is what was read; it is copied onto " <> "the checkpoint, and an allow whose copy did not land is refused " <> diff --git a/mcp-server/CHANGELOG.md b/mcp-server/CHANGELOG.md index 8c81b685..146b1432 100644 --- a/mcp-server/CHANGELOG.md +++ b/mcp-server/CHANGELOG.md @@ -17,9 +17,9 @@ Versioning: [Semantic Versioning](https://semver.org/spec/v2.0.0.html) reasons: `required_check_failed`, `required_check_pending`, `required_check_missing`, `required_checks_unset`, `required_check_timed_out`, `ci_evidence_unavailable`, `ci_evidence_not_recorded`. A CI wait never counts toward the unevaluated bound and is - refused 24 hours after the story entered ci; a checkpoint changing CI definitions is - refused `ci_definition_changed`. Only a GitHub Actions check run - satisfies a required check; a commit status is recorded, never trusted. + refused past the gate's CI wait limit from the story's entry into ci; a checkpoint changing + CI definitions is refused `ci_definition_changed`. Only a job of a workflow run that a push + of the thread branch triggered, concluding `success`, satisfies a required check. ## 2.107.0 — 2026-09-26 (review on a change thread) diff --git a/mcp-server/README.md b/mcp-server/README.md index e49eac64..a78224c4 100644 --- a/mcp-server/README.md +++ b/mcp-server/README.md @@ -524,7 +524,7 @@ The order to wire it up, the stage machine these tools move a story through, and | `thread_review_get` | **Read a review's payload** (`GET /api/v1/stories/:id/thread/reviews/:review_id`, any role): the story, the checkpoint it reads with its diff reference, the thread's latest entries, the latest fixes with the findings each answers (`fixes_truncated` when older ones exist), and the rounds. Bodies and locations are untrusted. | | `thread_fix` | **Record a fix on the story you hold** (`POST /api/v1/stories/:id/thread/fixes`), on the key `claim_story` claims with: the checkpoint carrying it (one your current claim recorded after every checkpoint its findings were found in) and the findings of completed rounds it answers. Refusals: 409 `not_claimant`, `stale_claim_epoch`, `claim_not_live`, `idempotency_key_reused`; 422 `fix_checkpoint_required`, `fix_checkpoint_not_current_claim`, `fix_checkpoint_not_after_findings`, `finding_ids_required`, `unknown_finding`, `secret_blocked`; 503 `tenant_halted`. | | `force_unclaim_story` | **Take a story back from the agent holding it** (`POST /api/v1/stories/:id/force-unclaim`). Resets it to `agent_status: pending` with `assigned_agent_id` cleared AND makes the delivery stage row follow the release back to `queued`. **A delivery story then goes to `escalated`, not back to the queue** (loopctl US-44.4): an operator taking a story back is a human decision, so a release that leaves the stage row at `queued` escalates it over `operator_released` in the same transaction, spending no attempt against the retry ceiling — it is not left in the queue behind the human's back. Put it back to work with `resolve_escalation` `to: queued`, which releases (a no-op by then) AND re-contracts. A story with no delivery stage row is simply left `pending`. A story parked at `claimed` with nobody on it is the residue of a compensation that did not complete, not what a refused dispatch normally leaves: placement releases the claim inline when a runner refuses, and the claim lease releases it unattended once `claimed_until` passes — reach for this to get it back now, or when both of those left it held. Requeues from any stage a claim holds; a stage no claim holds keeps its stage and is rebound to the new epoch; `done` and `failed` are untouched. For an ESCALATED story use `resolve_escalation` instead — that one releases AND re-contracts, so its `queued` really is placeable. Requires an orchestrator-ROLE key: the action is `exact_role: :orchestrator`, so a user or superadmin key is 403'd like any other non-member. Put it in `LOOPCTL_ORCH_KEY` (which is then the key sent, and a global `LOOPCTL_API_KEY` does not displace it) or, with no orchestrator key set, in `LOOPCTL_API_KEY`. The orchestrator key must be linked to a registered agent (400 otherwise) and the tenant must be human-anchored (403 `custody_tier_required` otherwise). Does not touch `verified_status`. Run again on an already-pending story it is still an operator's release: a row stranded behind the story's claim epoch is rebound, an in-flight row is requeued, and a row then at `queued` — including one already sitting there — is escalated over `operator_released`, as on the first run; a row already escalated, or anywhere else at that epoch, is left alone. Every 500 means the whole release rolled back and the story is still claimed: `audit_chain_append_failed` (the escalation's chain entry was refused) is a server-side condition a re-run meets again until an operator acts; `force_unclaim_failed` (the server log names the step) — call it again. A 422 means the release write itself was rejected. No request body. Required: `story_id` (refused locally if it is not a UUID). | -| `merge_precondition` | **Run the merge gate over a story's real pull request** (`POST /api/v1/stories/:id/merge-precondition`). Both delivery gates run again over the diff that exists, plus custody, the 12-file / 1000-line hard bound, the head-has-not-moved check and the self-deploy exclusion; the story must be at stage `ci`. **Gate A reads what triage persisted, never the caller**: the lens verdicts recorded with the story's most recent triage, or a human's re-queue of a Gate A escalation — `gate_a_inputs` on the answer says which, and `missing` refuses with `gate_a_inputs_missing`. This tool sends no trio. Decisions: `allow` (recorded against the head; the only thing that licenses a merge), `refuse` (the story is escalated before this returns), `already_merged`, `head_moved`, `unevaluated` (503 with `Retry-After`). **Thread mode** (the current claim was placed under a source with `mode: thread` — the mode is bound to the dispatch at placement): no pull request; the gate judges the latest checkpoint recorded under the story's CURRENT claim, on the branch that claim's dispatch ran on, and adds refusals `empty_change`, `checkpoint_tree_mismatch`, `no_checkpoint_recorded` and `claim_ended` (the claim was released after an earlier one recorded checkpoints); the source's runners must be at contract 1.20.0+ sending checkpoint messages, or every story is refused `no_checkpoint_recorded`; the branch is judged first, and a missing branch in a readable repository (`branch_missing`), a head nobody recorded (`branch_head_unrecorded`), an earlier checkpoint (`branch_head_regressed`), or a head the stage row did not record is `head_moved` back to implementing while the claim is LIVE, and a refusal naming `claim_not_live` (escalated) when it is not; an unreadable repository refuses `pull_request_unavailable`; the diff judged is the three-dot diff against the merge base, so a base that moved on is not a refusal, and a checkpoint the base already contains (its branch deleted or not) is `already_merged` only under a recorded allow naming it, otherwise `checkpoint_on_base_without_allow`; a thread loopctl could not read refuses `thread_unreadable`; CI is read by the checkpoint's exact SHA from both the check-runs and the commit-status APIs (only a GitHub Actions check run satisfies a required check; statuses are recorded, never trusted) against the source's `required_checks` — a failed one refuses `required_check_failed`, one still running or not yet reported is `unevaluated` (`required_check_pending` / `required_check_missing`, retry after 300s; neither counts toward the unevaluated bound, and 24 hours after the story entered `ci` both are refused `required_check_timed_out`; per name the latest run of each check suite counts and every suite must pass; a checkpoint changing `.github/workflows/` or `.github/actions/` is refused `ci_definition_changed`), a source requiring none refuses `required_checks_unset`, a failed read is `ci_evidence_unavailable`, and `local-gate` is recorded but never counted; the answer's `ci_evidence` is what was read and is copied onto the checkpoint; the answer carries `mode` (bound at placement, `pr` when the claim has no accepted dispatch whatever the source says now, null only on ledger contention) and `base_sha` (that merge base), an allow records it, and the merge executor merges only while the base head still equals it. Requires an orchestrator- or user-ROLE key (`exact_role: [:orchestrator, :user]`, so an agent key is 403'd); `LOOPCTL_ORCH_KEY` is sent when set, else `LOOPCTL_API_KEY`. 403 `custody_tier_required` without a human anchor, 404 for an unknown story, 422 when the story is not at `ci`. Required: `story_id` (refused locally if not a UUID) and `claim_epoch` (a non-negative integer, refused locally otherwise); optional `effect_proof`. | +| `merge_precondition` | **Run the merge gate over a story's real pull request** (`POST /api/v1/stories/:id/merge-precondition`). Both delivery gates run again over the diff that exists, plus custody, the 12-file / 1000-line hard bound, the head-has-not-moved check and the self-deploy exclusion; the story must be at stage `ci`. **Gate A reads what triage persisted, never the caller**: the lens verdicts recorded with the story's most recent triage, or a human's re-queue of a Gate A escalation — `gate_a_inputs` on the answer says which, and `missing` refuses with `gate_a_inputs_missing`. This tool sends no trio. Decisions: `allow` (recorded against the head; the only thing that licenses a merge), `refuse` (the story is escalated before this returns), `already_merged`, `head_moved`, `unevaluated` (503 with `Retry-After`). **Thread mode** (the current claim was placed under a source with `mode: thread` — the mode is bound to the dispatch at placement): no pull request; the gate judges the latest checkpoint recorded under the story's CURRENT claim, on the branch that claim's dispatch ran on, and adds refusals `empty_change`, `checkpoint_tree_mismatch`, `no_checkpoint_recorded` and `claim_ended` (the claim was released after an earlier one recorded checkpoints); the source's runners must be at contract 1.20.0+ sending checkpoint messages, or every story is refused `no_checkpoint_recorded`; the branch is judged first, and a missing branch in a readable repository (`branch_missing`), a head nobody recorded (`branch_head_unrecorded`), an earlier checkpoint (`branch_head_regressed`), or a head the stage row did not record is `head_moved` back to implementing while the claim is LIVE, and a refusal naming `claim_not_live` (escalated) when it is not; an unreadable repository refuses `pull_request_unavailable`; the diff judged is the three-dot diff against the merge base, so a base that moved on is not a refusal, and a checkpoint the base already contains (its branch deleted or not) is `already_merged` only under a recorded allow naming it, otherwise `checkpoint_on_base_without_allow`; a thread loopctl could not read refuses `thread_unreadable`; CI is read by the checkpoint's exact SHA from both the check-runs and the commit-status APIs (only a job of a GitHub Actions workflow run that a push of the thread branch at that commit triggered satisfies a required check, and only by concluding `success` — a skipped job fails; statuses and other check runs are recorded or ignored, never trusted) against the source's `required_checks` — a failed one refuses `required_check_failed`, one still running or not yet reported is `unevaluated` (`required_check_pending` / `required_check_missing`, retry after the answer's `Retry-After`; neither counts toward the unevaluated bound, and past the gate's CI wait limit from the story's entry into `ci` both are refused `required_check_timed_out`; per name the newest run of each workflow counts and every workflow must pass; a checkpoint changing `.github/workflows/` or `.github/actions/` is refused `ci_definition_changed`), a source requiring none refuses `required_checks_unset`, a failed read is `ci_evidence_unavailable`, and `local-gate` is recorded but never counted; the answer's `ci_evidence` is what was read and is copied onto the checkpoint; the answer carries `mode` (bound at placement, `pr` when the claim has no accepted dispatch whatever the source says now, null only on ledger contention) and `base_sha` (that merge base), an allow records it, and the merge executor merges only while the base head still equals it. Requires an orchestrator- or user-ROLE key (`exact_role: [:orchestrator, :user]`, so an agent key is 403'd); `LOOPCTL_ORCH_KEY` is sent when set, else `LOOPCTL_API_KEY`. 403 `custody_tier_required` without a human anchor, 404 for an unknown story, 422 when the story is not at `ci`. Required: `story_id` (refused locally if not a UUID) and `claim_epoch` (a non-negative integer, refused locally otherwise); optional `effect_proof`. | | `intake_source_enroll` | **Bind a GitHub repository to a work project and mint its webhook secret** (`POST /api/v1/intake/sources`) — the step that gives the delivery loop an input, and the row `place_dispatch` reads `repo` and `base_branch` from (its 409 `no_intake_source` means this has not been done; 409 `ambiguous_intake_source` means it has been done twice). **The secret is returned ONCE and can never be read again** — the column is encrypted at rest and redacted on the schema, so no other tool carries it and losing it costs a revoke, a re-enrolment and a reconfigured GitHub webhook. **This tool does not return it either:** a tool result lands in the transcript and the audit log, so it is written to the required `secret_file` with mode 0600 (the handling `runner_enroll` gives a runner credential) and the result carries the source row, the `webhook_url` and that path. The path is reserved before the request, so an existing file or an unwritable directory is refused with nothing enrolled, and any outcome that is not a clean creation withholds the response body — an unparsed 2xx body IS the secret. A 2xx carrying the source and the secret but no webhook path is RECOVERED rather than discarded (the path is derived from the source id, flagged `webhook_url_derived`), and an outcome that proves a source id but no usable secret REVOKES that source rather than leaving it holding the repository's unique slot. Then configure GitHub: Payload URL = the returned `webhook_url`, content type `application/json`, Secret = the contents of the file, and the **Issues** event only — `gh api repos/OWNER/REPO/hooks -f name=web -f config[url]= -f config[content_type]=json -f config[secret]="$(cat )" -f 'events[]=issues'`. Every later delivery failure is the SAME 401 `invalid_signature` — unknown or revoked source, suspended tenant, missing or wrong signature, or a payload whose `repository.full_name` does not match — so the webhook's Recent Deliveries tab cannot tell you which. `base_branch` is set HERE: it defaults to `master` when the body does not name it, so name `main` for a repository created on GitHub since 2020 or every dispatch is cut from a trunk that does not exist. It is not nullable (a dispatch must name a branch), so null or blank is refused rather than falling back to the default, and `intake_source_update` changes it afterwards. Refusals: 403 `api_key_mint_forbidden` if your key was minted by a dispatch (this mints a credential belonging to no lineage, so only an unlineaged operator key may), 403 `custody_tier_required` on an agent-rooted tenant, 422 for a repository that is not `owner/name`, an ACTIVE source already binding it, a project that is missing / archived / not a work project, or a `target_epic_id` outside that project. `mode` picks the merge route: `pr` (the default when omitted) or `thread`, where the merge gate evaluates the story's latest recorded checkpoint instead of a pull request — which needs the source's runners at contract 1.20.0+ sending checkpoint messages, or every story is refused `no_checkpoint_recorded`; any other value, null included, is refused. The mode is bound to each implement dispatch when it is placed, so it decides only what stories placed afterwards get; a change is always allowed. A `thread` source must also name `required_checks` (422 otherwise): the CI checks the merge gate requires on a checkpoint's exact commit, GitHub Actions check-run names, bounded in number and length by the server; `local-gate` is refused because whoever pushed posts it. Required: `repo_full_name`, `project_id`, `secret_file`. Optional: `base_branch`, `mode`, `required_checks`, and `target_epic_id` — **omitting the epic is not a neutral default**: with no epic every triaged report is escalated to a human and every record stays `pending_triage` until `intake_source_update` names one. Requires `LOOPCTL_USER_KEY`. | | `intake_source_list` | List the tenant's intake sources (`GET /api/v1/intake/sources`): `id` (the webhook URL is `/api/v1/intake/github/`), `project_id`, `repo_full_name`, `base_branch`, `mode` (`pr` or `thread`), `target_epic_id`, `revoked_at`, timestamps. **The webhook secret is not here and is not anywhere** — it is returned once by `intake_source_enroll`, so this is not how to recover one. Use it to find the source behind a 409 `no_intake_source` or `ambiguous_intake_source`, to check a repository's `base_branch` before placing work, and to identify a source left behind by an enrolment whose outcome was unknown. Optional: `include_revoked`. Requires `LOOPCTL_USER_KEY`; the only one of the four that does not also need a human-anchored tenant. | | `intake_source_update` | **Set where a source's work lands** (`PATCH /api/v1/intake/sources/:id`): `target_epic_id`, the epic triaged stories are created in, `base_branch`, the branch every dispatch for this repository is cut from, `mode` (`pr` or `thread`), whether the merge gate reads a pull request or the story's latest recorded thread checkpoint, and `required_checks`, the CI checks a thread checkpoint must pass on its exact commit (a thread source must name at least one, judged over the source as it will be, so switching to `thread` without them or clearing them on a thread source is 422; `local-gate` is refused). Presence decides — a field you do not name is left exactly as it was, and naming none of them is 422 `nothing_to_update`. `mode` has no cleared state either, and a null or unknown value is refused locally; A change is always allowed and affects only stories placed afterwards: each implement dispatch records the mode, and the base branch, it was placed under, and the merge gate reads those. **Do not send `target_epic_id: null` to mean "not changing this":** an explicit null is the only way to CLEAR the epic, which returns the source to escalating every report instead of filing a story. `base_branch` has no cleared state and a null there is refused locally. This is how a source already pointed at the wrong trunk is corrected — `intake_source_enroll` takes `base_branch` itself, so a `main` repository no longer needs a second call — and the remedy for a source with no epic — every record from it stays `pending_triage` and is retried until one is named, then they promote on the next run with nothing lost. Revoked sources are 404, not a no-op: revoking clears the target so the epic can be deleted, and repointing one would put that block back. 422 when the epic is not in this source's project. Required: `source_id` (refused locally if it is not a UUID — the server answers the same 404 for malformed and unknown). Requires `LOOPCTL_USER_KEY` and a human-anchored tenant. | diff --git a/mcp-server/index.js b/mcp-server/index.js index aa4ef09d..8e29d40d 100755 --- a/mcp-server/index.js +++ b/mcp-server/index.js @@ -8503,15 +8503,18 @@ const TOOLS = [ "`thread_unreadable`. It adds refusals `empty_change` (the " + "checkpoint's tree equals the base's, or no file changed) and " + "`checkpoint_tree_mismatch` (the forge's tree is not the one the claimant recorded). " + - "CI IS READ BY THE CHECKPOINT'S EXACT SHA, from both the check-runs and the " + - "commit-status APIs, against the source's `required_checks`; only a GitHub Actions " + - "check run satisfies one (a status is recorded, never trusted): a failed one refuses " + - "`required_check_failed`, one still running or not yet reported answers `unevaluated` " + - "(`required_check_pending` / `required_check_missing`, retry after 300s; neither counts " + - "toward the unevaluated bound, and 24 hours after the story entered ci both are " + - "refused `required_check_timed_out`; per name the latest run of each check suite " + - "counts and every suite must pass), a checkpoint changing `.github/workflows/` or " + - "`.github/actions/` is refused `ci_definition_changed`, a source requiring none refuses " + + "CI IS READ BY THE CHECKPOINT'S EXACT SHA against the source's `required_checks`: only " + + "a job of a GitHub Actions workflow run that a PUSH of the thread branch at that commit " + + "triggered satisfies one, and only by concluding `success` (a skipped job fails; " + + "statuses and other check runs are never trusted). A failed one refuses " + + "`required_check_failed`; one still running or not yet reported answers `unevaluated` " + + "(`required_check_pending` / `required_check_missing`, retry after the answer's " + + "Retry-After; neither counts toward the unevaluated bound, and past the gate's CI wait " + + "limit from the story's entry into ci both are refused `required_check_timed_out`; per " + + "name the newest run of each workflow counts and every workflow must pass). A " + + "checkpoint changing `.github/workflows/` or `.github/actions/` is refused " + + "`ci_definition_changed` (`ci_definition_unknown` when its diff could not be listed), " + + "a source requiring none refuses " + "`required_checks_unset`, and an evidence read that failed is `ci_evidence_unavailable`. " + "`local-gate` is recorded, never counted. The answer's `ci_evidence` is what was read, " + "and it is copied onto the checkpoint. " + @@ -8726,7 +8729,7 @@ const TOOLS = [ items: { type: "string" }, description: "The CI checks a THREAD-mode checkpoint must pass on its exact commit before the " + - "merge gate allows it: GitHub Actions check-run names as they appear " + + "merge gate allows it: GitHub Actions job names as they appear " + "on the commit. A `thread` source must name at least one (422 otherwise); `pr` " + "mode never reads it. Distinct, non-blank names, bounded in number and length by the server (422 past them). " + "`local-gate` is refused: whoever pushed posts it, so it is only recorded.", @@ -8836,7 +8839,7 @@ const TOOLS = [ items: { type: "string" }, description: "The CI checks a THREAD-mode checkpoint must pass on its exact commit before the " + - "merge gate allows it: GitHub Actions check-run names as they appear " + + "merge gate allows it: GitHub Actions job names as they appear " + "on the commit. A `thread` source must name at least one (422 otherwise); `pr` " + "mode never reads it. Distinct, non-blank names, bounded in number and length by the server (422 past them). " + "`local-gate` is refused. Omit to leave the current list alone.", diff --git a/mcp-server/lib/intake-sources.js b/mcp-server/lib/intake-sources.js index 6c60fb8f..dc1a0f32 100644 --- a/mcp-server/lib/intake-sources.js +++ b/mcp-server/lib/intake-sources.js @@ -174,7 +174,7 @@ export function requiredChecksRefusal(checks) { if (!ok) { return refuse( - "`required_checks` must be a list of GitHub Actions check-run names, as they appear " + + "`required_checks` must be a list of GitHub Actions job names, as they appear " + "on the commit (for example [\"test\", \"lint\"]).", ); } diff --git a/test/loopctl/delivery/ci_evidence_test.exs b/test/loopctl/delivery/ci_evidence_test.exs index 9a35b1ef..e17deb15 100644 --- a/test/loopctl/delivery/ci_evidence_test.exs +++ b/test/loopctl/delivery/ci_evidence_test.exs @@ -1,116 +1,113 @@ defmodule Loopctl.Delivery.CiEvidenceTest do @moduledoc """ - `Loopctl.Delivery.CiEvidence` (US-45.6): what CI said about one commit, judged against the - required checks. Pure, so every case is a table of evidence and an expected judgement. + `Loopctl.Delivery.CiEvidence` (US-45.6): what CI said about one commit of a thread, judged + against the required checks. Pure, so every case is evidence in and a judgement out. The + jobs here are the ones the adapter already filtered to a push of the thread branch at the + commit (`GitHubPullRequestSourceTest` pins that filter). """ use ExUnit.Case, async: true alias Loopctl.Delivery.CiEvidence - defp run(name, status, conclusion \\ nil), - do: %{name: name, status: status, conclusion: conclusion, app: "github-actions"} + defp job(name, status, conclusion \\ nil, extra \\ %{}) do + Map.merge( + %{ + id: 1, + name: name, + status: status, + conclusion: conclusion, + run_id: 10, + workflow: "ci.yml" + }, + extra + ) + end defp status(context, state), do: %{context: context, state: state} - defp judge(required, runs, statuses \\ []), - do: CiEvidence.judge(required, %{check_runs: runs, statuses: statuses}) + defp judge(required, jobs, statuses \\ []), + do: CiEvidence.judge(required, %{jobs: jobs, statuses: statuses}) - test "a completed run concluding success, neutral or skipped passes" do - for conclusion <- ["success", "neutral", "skipped"] do - assert %{passed: ["test"], failed: [], pending: [], missing: []} = - judge(["test"], [run("test", "completed", conclusion)]), - conclusion - end + test "a completed job passes only by concluding success" do + assert %{passed: ["test"], failed: []} = + judge(["test"], [job("test", "completed", "success")]) end - test "any other conclusion fails, naming it" do - for conclusion <- ["failure", "cancelled", "timed_out", "action_required", "stale"] do + # Review round 1 of #910, finding 1: a job GitHub skipped did not run — a required job + # skipped because a job it `needs:` failed must never read as green. + test "any other conclusion fails, skipped and neutral included" do + for conclusion <- ["skipped", "neutral", "failure", "cancelled", "timed_out"] do assert %{failed: [{"test", ^conclusion}], passed: []} = - judge(["test"], [run("test", "completed", conclusion)]) + judge(["test"], [job("test", "completed", conclusion)]) end end - test "a run that has not completed is pending, whatever it concluded so far" do + test "a job that has not completed is pending" do for state <- ["queued", "in_progress", "waiting"] do - assert %{pending: ["test"], passed: [], failed: []} = judge(["test"], [run("test", state)]) + assert %{pending: ["test"], passed: []} = judge(["test"], [job("test", state)]) end end - # Review round 2, finding 1: a status can be posted by the implementer, so it never - # satisfies a required check; nor does a run some other App created. - test "a commit status never satisfies a required check, however green or new" do - statuses = [status("test", "success")] - assert %{missing: ["test"], passed: []} = judge(["test"], [], statuses) - - failing = Map.put(run("test", "completed", "failure"), :id, 1) - assert %{failed: [{"test", "failure"}]} = judge(["test"], [failing], statuses) + test "a required name no trusted job carries is missing, whatever statuses say" do + assert %{missing: ["test"], passed: []} = + judge(["test"], [job("lint", "completed", "success")], [status("test", "success")]) end - # Round 3: runs are judged latest-per-SUITE, and across suites a green run never hides a red - # one under the same name. - test "a passing run in one suite never hides a failing run in another" do - ci_fail = %{run("test", "completed", "failure") | app: "github-actions"} - ci_fail = Map.merge(ci_fail, %{id: 100, check_suite: 1}) - other_pass = Map.merge(run("test", "completed", "success"), %{id: 105, check_suite: 2}) - - assert %{failed: [{"test", "failure"}], passed: []} = judge(["test"], [ci_fail, other_pass]) + # Per workflow, only its newest run counts; across workflows nothing hides anything. + test "a newer run of the same workflow supersedes an older one" do + old = job("test", "completed", "failure", %{id: 1, run_id: 10}) + new = job("test", "completed", "success", %{id: 2, run_id: 11}) - # Within ONE suite, a later re-run supersedes the failure it re-ran. - rerun = Map.merge(run("test", "completed", "success"), %{id: 101, check_suite: 1}) - assert %{passed: ["test"]} = judge(["test"], [ci_fail, rerun]) - - # And a suite still running keeps the name pending when none failed. - running = Map.merge(run("test", "in_progress"), %{id: 106, check_suite: 3}) - assert %{pending: ["test"]} = judge(["test"], [rerun, running]) + assert %{passed: ["test"]} = judge(["test"], [old, new]) end - test "only a GitHub Actions check run counts" do - other_app = %{run("test", "completed", "success") | app: "some-other-app"} - assert %{missing: ["test"], passed: []} = judge(["test"], [other_app]) - end + test "a passing job in one workflow never hides a failing one in another" do + ci = job("test", "completed", "failure", %{id: 1, run_id: 10, workflow: "ci.yml"}) + lint = job("test", "completed", "success", %{id: 2, run_id: 20, workflow: "lint.yml"}) + + assert %{failed: [{"test", "failure"}], passed: []} = judge(["test"], [ci, lint]) - test "a required name nothing reported is missing" do - assert %{missing: ["test"], passed: [], pending: [], failed: []} = - judge(["test"], [run("lint", "completed", "success")]) + running = job("test", "in_progress", nil, %{id: 3, run_id: 30, workflow: "e2e.yml"}) + green = job("test", "completed", "success", %{id: 4, run_id: 11, workflow: "ci.yml"}) + assert %{pending: ["test"]} = judge(["test"], [green, running]) end - # Review round 1, finding 5: only the LATEST result under a name counts, as GitHub's own - # required-check rule judges it. - test "among runs of one name the highest id decides, whatever it concluded" do - old_fail = Map.put(run("test", "completed", "failure"), :id, 1) - new_pass = Map.put(run("test", "completed", "success"), :id, 2) - assert %{passed: ["test"], failed: []} = judge(["test"], [new_pass, old_fail]) + test "within one run, the job's latest attempt (highest id) decides" do + first = job("test", "completed", "failure", %{id: 1}) + retry = job("test", "completed", "success", %{id: 2}) - rerun = Map.put(run("test", "queued"), :id, 3) - assert %{pending: ["test"], passed: []} = judge(["test"], [new_pass, rerun]) + assert %{passed: ["test"]} = judge(["test"], [first, retry]) end test "each required name is judged on its own" do - runs = [run("test", "completed", "success"), run("lint", "completed", "failure")] + jobs = [ + job("test", "completed", "success", %{id: 1}), + job("lint", "completed", "failure", %{id: 2}) + ] assert %{passed: ["test"], failed: [{"lint", "failure"}], missing: ["dialyzer"]} = - judge(["test", "lint", "dialyzer"], runs) + judge(["test", "lint", "dialyzer"], jobs) end - # AC-45.6.2: the local gate is recorded and never satisfies a required check, even when a - # caller lists it. + # AC-45.6.2: the local gate is recorded and never satisfies a required check. test "local-gate is reported, never looked up as a required check" do statuses = [status("local-gate", "success")] - assert %{local_gate: "success", missing: ["test"], passed: []} = - judge(["test"], [], statuses) - - assert %{local_gate: "success", passed: [], missing: []} = - judge(["local-gate"], [], statuses) - + assert %{local_gate: "success", missing: ["test"], passed: []} = judge(["test"], [], statuses) + assert %{local_gate: "success", passed: [], missing: []} = judge(["local-gate"], [], statuses) assert CiEvidence.lookup_names(["local-gate", "test"]) == ["test"] end - test "to_record/5 keeps the evidence and the judgement with string keys" do + test "statuses that could not be read record local-gate as unread and decide nothing" do + assert %{local_gate: "unread", passed: ["test"]} = + judge(["test"], [job("test", "completed", "success")], {:unread, :forbidden}) + end + + test "to_record/5 keeps only what the judgement read, with string keys" do evidence = %{ - check_runs: [run("test", "completed", "failure")], - statuses: [status("local-gate", "success")] + jobs: [job("test", "completed", "failure"), job("codeql", "completed", "success")], + statuses: [status("local-gate", "success"), status("other", "success")] } result = CiEvidence.judge(["test"], evidence) @@ -121,18 +118,8 @@ defmodule Loopctl.Delivery.CiEvidenceTest do assert record["read_at"] == "2026-09-27T10:00:00.000000Z" assert record["local_gate"] == "success" assert record["failed"] == [%{"name" => "test", "why" => "failure"}] - assert [%{"name" => "test", "conclusion" => "failure"}] = record["check_runs"] - - # Round 3: only what the judgement read is kept — required runs and local-gate. - noisy = %{ - evidence - | check_runs: [run("codeql", "completed", "success") | evidence.check_runs], - statuses: [status("other", "success") | evidence.statuses] - } - slim = CiEvidence.to_record("abc", ["test"], noisy, result, ~U[2026-09-27 10:00:00Z]) - assert Enum.map(slim["check_runs"], & &1["name"]) == ["test"] - assert Enum.map(slim["statuses"], & &1["context"]) == ["local-gate"] - assert [%{"context" => "local-gate", "state" => "success"}] = record["statuses"] + assert [%{"name" => "test", "workflow" => "ci.yml", "conclusion" => "failure"}] = + record["jobs"] 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 1473efcc..5a8aa627 100644 --- a/test/loopctl/delivery/github_pull_request_source_test.exs +++ b/test/loopctl/delivery/github_pull_request_source_test.exs @@ -754,108 +754,134 @@ defmodule Loopctl.Delivery.GitHubPullRequestSourceTest do # -- helpers --------------------------------------------------------------------------- describe "check_evidence/3 (US-45.6)" do - defp gh_run(id, name, conclusion), - do: %{ - "id" => id, - "name" => name, - "status" => "completed", - "conclusion" => conclusion, - "app" => %{"slug" => "github-actions"}, - "check_suite" => %{"id" => id * 10} - } - - # Review round 1, finding 7: ONE paged read of every run on the commit, never one per name. - test "reads every latest run on the exact SHA in one paged call, and every status" do + @branch "loop/story-7-abcd1234" + + defp gh_run(id, extra \\ %{}) do + Map.merge( + %{ + "id" => id, + "path" => ".github/workflows/ci.yml", + "head_sha" => @head, + "head_branch" => @branch, + "event" => "push" + }, + extra + ) + end + + defp gh_job(id, name, conclusion), + do: %{"id" => id, "name" => name, "status" => "completed", "conclusion" => conclusion} + + # The three reads, each answered by `answers`, keyed by what it asks for. + defp stub_ci(answers) do stub(fn conn -> case String.split(conn.request_path, "/") do - [_, "repos", "acme", "widgets", "commits", @head, "check-runs"] -> - query = URI.decode_query(conn.query_string) - assert query["filter"] == "latest" - assert query["per_page"] == "100" - refute Map.has_key?(query, "check_name") - assert query["page"] == "1" + [_, "repos", "acme", "widgets", "actions", "runs"] -> + send(self(), {:runs_query, URI.decode_query(conn.query_string)}) + json(conn, answers.runs) - json(conn, %{ - "total_count" => 2, - "check_runs" => [ - Map.put(gh_run(7, "test", "success"), "completed_at", "2026-09-27T10:00:00Z"), - gh_run(8, "lint / credo", "failure") - ] - }) + [_, "repos", "acme", "widgets", "actions", "runs", id, "jobs"] -> + assert URI.decode_query(conn.query_string)["filter"] == "latest" + json(conn, Map.fetch!(answers.jobs, String.to_integer(id))) [_, "repos", "acme", "widgets", "commits", @head, "status"] -> - assert conn.query_string == "per_page=100" - - json(conn, %{ - "total_count" => 1, - "statuses" => [ - %{"context" => "local-gate", "state" => "success", "updated_at" => "t"} - ] - }) + answer_statuses(conn, answers.statuses) end end) + end - assert {:ok, %{check_runs: runs, statuses: statuses}} = - Source.check_evidence(@repo, @head) + defp answer_statuses(conn, {:status, code}), + do: conn |> Plug.Conn.put_status(code) |> json(%{}) - assert [ - %{ - id: 7, - name: "test", - app: "github-actions", - check_suite: 70, - completed_at: "2026-09-27T10:00:00Z" - }, - %{id: 8} - ] = runs + defp answer_statuses(conn, body), do: json(conn, body) - assert [%{context: "local-gate", state: "success", at: "t"}] = statuses + test "reads the jobs of the push runs of THIS branch at THIS commit, tagged with the run" do + stub_ci(%{ + runs: %{"total_count" => 1, "workflow_runs" => [gh_run(5)]}, + jobs: %{5 => %{"total_count" => 1, "jobs" => [gh_job(50, "test", "success")]}}, + statuses: %{ + "total_count" => 1, + "statuses" => [%{"context" => "local-gate", "state" => "success"}] + } + }) + + assert {:ok, %{jobs: [job], statuses: [%{context: "local-gate"}]}} = + Source.check_evidence(@repo, @head, @branch) + + assert %{id: 50, name: "test", run_id: 5, workflow: ".github/workflows/ci.yml"} = job + + assert_received {:runs_query, query} + assert query["head_sha"] == @head + assert query["branch"] == @branch + assert query["event"] == "push" end - test "further pages are read until the total is reached" do - stub(fn conn -> - if String.ends_with?(conn.request_path, "/check-runs") do - page = URI.decode_query(conn.query_string)["page"] - run = gh_run(String.to_integer(page), "job-#{page}", "success") - json(conn, %{"total_count" => 2, "check_runs" => [run]}) - else - json(conn, %{"total_count" => 0, "statuses" => []}) - end - end) + # Round 1 of #910, finding 2: a run the API filter let through for another branch, event or + # commit is dropped here too — only the thread's own push runs are trusted. + test "a run for another branch, event or commit is never read" do + stub_ci(%{ + runs: %{ + "total_count" => 3, + "workflow_runs" => [ + gh_run(6, %{"head_branch" => "evil"}), + gh_run(7, %{"event" => "workflow_dispatch"}), + gh_run(8, %{"head_sha" => String.duplicate("9", 40)}) + ] + }, + jobs: %{}, + statuses: %{"total_count" => 0, "statuses" => []} + }) - assert {:ok, %{check_runs: [%{name: "job-1"}, %{name: "job-2"}]}} = - Source.check_evidence(@repo, @head) + assert {:ok, %{jobs: []}} = Source.check_evidence(@repo, @head, @branch) end - test "a list the forge truncated is an error, never a partial answer" do - stub(fn conn -> - if String.ends_with?(conn.request_path, "/check-runs") do - json(conn, %{"total_count" => 1000, "check_runs" => [gh_run(1, "test", "success")]}) - else - json(conn, %{"total_count" => 0, "statuses" => []}) - end - end) + # Round 1 of #910, finding 3: a job repeated across a moved page boundary is counted once, + # and a list short of its total is refused. + test "jobs are de-duplicated by id and a short list is refused as truncated" do + stub_ci(%{ + runs: %{"total_count" => 1, "workflow_runs" => [gh_run(5)]}, + jobs: %{ + 5 => %{ + "total_count" => 2, + "jobs" => [gh_job(50, "test", "success"), gh_job(50, "test", "success")] + } + }, + statuses: %{"total_count" => 0, "statuses" => []} + }) - assert {:error, {:check_runs_truncated, 1000, 3}} = - Source.check_evidence(@repo, @head) + assert {:error, {:jobs_truncated, 2, 1}} = Source.check_evidence(@repo, @head, @branch) + end - stub(fn conn -> - if String.ends_with?(conn.request_path, "/check-runs") do - json(conn, %{"total_count" => 0, "check_runs" => []}) - else - json(conn, %{"total_count" => 101, "statuses" => []}) - end - end) + test "more workflow runs than the bound is refused rather than read" do + runs = for id <- 1..11, do: gh_run(id) + + stub_ci(%{ + runs: %{"total_count" => 11, "workflow_runs" => runs}, + jobs: %{}, + statuses: %{"total_count" => 0, "statuses" => []} + }) + + assert {:error, {:too_many_workflow_runs, 11}} = + Source.check_evidence(@repo, @head, @branch) + end + + # Round 1 of #910, finding 4: the statuses feed only the recorded local-gate state. + test "statuses that cannot be read are recorded as unread, never a failed read" do + stub_ci(%{ + runs: %{"total_count" => 1, "workflow_runs" => [gh_run(5)]}, + jobs: %{5 => %{"total_count" => 1, "jobs" => [gh_job(50, "test", "success")]}}, + statuses: {:status, 403} + }) - assert {:error, {:statuses_truncated, 101, 0}} = - Source.check_evidence(@repo, @head) + assert {:ok, %{jobs: [_], statuses: {:unread, _reason}}} = + Source.check_evidence(@repo, @head, @branch) end - test "an unreadable body is an error naming its shape" do + test "an unreadable runs body is an error naming its shape" do stub(fn conn -> json(conn, %{"message" => "nope"}) end) - assert {:error, {:unreadable_check_runs, {:map, ["message"]}}} = - Source.check_evidence(@repo, @head) + assert {:error, {:unreadable_workflow_runs, {:map, ["message"]}}} = + Source.check_evidence(@repo, @head, @branch) end end diff --git a/test/loopctl/delivery/merge_precondition_integration_test.exs b/test/loopctl/delivery/merge_precondition_integration_test.exs index b235bc3f..445e5f50 100644 --- a/test/loopctl/delivery/merge_precondition_integration_test.exs +++ b/test/loopctl/delivery/merge_precondition_integration_test.exs @@ -524,10 +524,10 @@ defmodule Loopctl.Delivery.MergePreconditionIntegrationTest do stub_thread(ctx) parent = String.duplicate("b", 40) - Mox.stub(MockPullRequestSource, :check_evidence, fn @repo, sha -> + Mox.stub(MockPullRequestSource, :check_evidence, fn @repo, sha, _branch -> cond do - sha == @head -> {:ok, %{check_runs: [ci_run("test", nil, "in_progress")], statuses: []}} - sha == parent -> {:ok, %{check_runs: [ci_run("test", "success")], statuses: []}} + sha == @head -> {:ok, %{jobs: [ci_run("test", nil, "in_progress")], statuses: []}} + sha == parent -> {:ok, %{jobs: [ci_run("test", "success")], statuses: []}} end end) @@ -545,7 +545,7 @@ defmodule Loopctl.Delivery.MergePreconditionIntegrationTest do assert %{"ci" => %{"sha" => @head, "passed" => ["test"], "failed" => []}} = checkpoint_evidence(ctx) - stub_thread(ctx, ci: %{check_runs: [ci_run("test", "failure")], statuses: []}) + stub_thread(ctx, ci: %{jobs: [ci_run("test", "failure")], statuses: []}) reset_allow(ctx) assert {:ok, %Verdict{decision: :refuse} = refused} = enforce(ctx) @@ -559,8 +559,8 @@ defmodule Loopctl.Delivery.MergePreconditionIntegrationTest do # never by polls, so a slow pipeline never escalates and a stuck one still does. test "a CI wait never reaches the unevaluated bound; past the time limit it escalates", ctx do for ci <- [ - %{check_runs: [ci_run("test", nil, "queued")], statuses: []}, - %{check_runs: [], statuses: []} + %{jobs: [ci_run("test", nil, "queued")], statuses: []}, + %{jobs: [], statuses: []} ] do stub_thread(ctx, ci: ci) @@ -585,7 +585,7 @@ defmodule Loopctl.Delivery.MergePreconditionIntegrationTest do # Round 3: the required checks are the source's CURRENT list, so correcting the list # (a renamed job) reaches a story already in flight. test "the required checks are the source's current list, read live", ctx do - stub_thread(ctx, ci: %{check_runs: [ci_run("unit", "success")], statuses: []}) + stub_thread(ctx, ci: %{jobs: [ci_run("unit", "success")], statuses: []}) assert {:ok, %Verdict{decision: :unevaluated} = waiting} = enforce(ctx) assert {:required_check_missing, "test"} in waiting.reasons @@ -603,7 +603,7 @@ defmodule Loopctl.Delivery.MergePreconditionIntegrationTest do blips = MergePrecondition.max_consecutive_unevaluated() - 1 fault = fn -> - Mox.stub(MockPullRequestSource, :check_evidence, fn @repo, _sha -> + Mox.stub(MockPullRequestSource, :check_evidence, fn @repo, _sha, _branch -> {:error, {:github_unreachable, :timeout}} end) @@ -612,8 +612,8 @@ defmodule Loopctl.Delivery.MergePreconditionIntegrationTest do fault.() - Mox.stub(MockPullRequestSource, :check_evidence, fn @repo, _sha -> - {:ok, %{check_runs: [ci_run("test", nil, "queued")], statuses: []}} + Mox.stub(MockPullRequestSource, :check_evidence, fn @repo, _sha, _branch -> + {:ok, %{jobs: [ci_run("test", nil, "queued")], statuses: []}} end) assert {:ok, %Verdict{decision: :unevaluated}} = enforce(ctx) @@ -622,6 +622,22 @@ defmodule Loopctl.Delivery.MergePreconditionIntegrationTest do assert Stages.get(ctx.tenant_id, ctx.story_id).stage == :ci end + # Round 1 of #910, finding 8: with no recorded entry into ci, the wait is measured from the + # checkpoint's recording, so it is bounded for every story. + test "a story with no recorded ci entry is bounded from its checkpoint instead", ctx do + stub_thread(ctx, ci: %{jobs: [], statuses: []}) + + recorded = + DateTime.add(DateTime.utc_now(), -(MergePrecondition.ci_wait_limit_seconds() + 60)) + + {1, _} = + from(c in Loopctl.Threads.Checkpoint, where: c.id == ^ctx.checkpoint.id) + |> AdminRepo.update_all(set: [inserted_at: recorded]) + + assert {:ok, %Verdict{decision: :refuse} = verdict} = enforce(ctx) + assert {:required_check_timed_out, "test", :missing} in verdict.reasons + end + test "TC-45.4.3 a branch head nobody reported goes back to implementing, unescalated", ctx do pushed = String.duplicate("9", 40) make_claim_live(ctx) @@ -1094,8 +1110,9 @@ defmodule Loopctl.Delivery.MergePreconditionIntegrationTest do Mox.stub(MockPullRequestSource, :repo_files, fn @repo, _ref -> {:ok, @repo_files} end) # US-45.6: CI on the CHECKPOINT's commit, green unless the test says otherwise. - ci = Keyword.get(opts, :ci, %{check_runs: [ci_run("test", "success")], statuses: []}) - Mox.stub(MockPullRequestSource, :check_evidence, fn @repo, ^head -> {:ok, ci} end) + ci = Keyword.get(opts, :ci, %{jobs: [ci_run("test", "success")], statuses: []}) + # The read names the thread branch: only its push runs are trusted. + Mox.stub(MockPullRequestSource, :check_evidence, fn @repo, ^head, ^branch -> {:ok, ci} end) end # Records the story's transition into `ci` at `at` (the CI wait's origin). diff --git a/test/loopctl/delivery/merge_precondition_judge_test.exs b/test/loopctl/delivery/merge_precondition_judge_test.exs index b9d7edc1..aec3eea9 100644 --- a/test/loopctl/delivery/merge_precondition_judge_test.exs +++ b/test/loopctl/delivery/merge_precondition_judge_test.exs @@ -1203,14 +1203,14 @@ defmodule Loopctl.Delivery.MergePreconditionJudgeTest do describe "CI evidence on the checkpoint's exact commit (US-45.6)" do test "a failed required check refuses, naming it and its conclusion" do verdict = - judge_thread(ci: %{check_runs: [run("test", "completed", "failure")], statuses: []}) + judge_thread(ci: %{jobs: [run("test", "completed", "failure")], statuses: []}) assert verdict.decision == :refuse assert {:required_check_failed, "test", "failure"} in verdict.reasons end test "a pending required check is unevaluated, retried after five minutes, never allowed" do - verdict = judge_thread(ci: %{check_runs: [run("test", "in_progress", nil)], statuses: []}) + verdict = judge_thread(ci: %{jobs: [run("test", "in_progress", nil)], statuses: []}) assert verdict.decision == :unevaluated assert {:required_check_pending, "test"} in verdict.reasons @@ -1218,7 +1218,7 @@ defmodule Loopctl.Delivery.MergePreconditionJudgeTest do end test "a required check missing from the commit is unevaluated, never allowed" do - verdict = judge_thread(ci: %{check_runs: [], statuses: []}) + verdict = judge_thread(ci: %{jobs: [], statuses: []}) assert verdict.decision == :unevaluated assert {:required_check_missing, "test"} in verdict.reasons @@ -1226,7 +1226,7 @@ defmodule Loopctl.Delivery.MergePreconditionJudgeTest do test "a failure decides even while another required check is still running" do ci = %{ - check_runs: [run("test", "completed", "failure"), run("lint", "queued", nil)], + jobs: [run("test", "completed", "failure"), run("lint", "queued", nil)], statuses: [] } @@ -1239,7 +1239,7 @@ defmodule Loopctl.Delivery.MergePreconditionJudgeTest do # TC-45.6.2: the local gate alone never satisfies a required check. test "only a local-gate success is not an allow" do - ci = %{check_runs: [], statuses: [%{context: "local-gate", state: "success"}]} + ci = %{jobs: [], statuses: [%{context: "local-gate", state: "success"}]} verdict = judge_thread(ci: ci) assert verdict.decision == :unevaluated @@ -1290,9 +1290,25 @@ defmodule Loopctl.Delivery.MergePreconditionJudgeTest do end end + # Round 1 of #910, finding 7: a diff that could not be listed fails CLOSED here. + test "a checkpoint whose diff could not be listed is refused ci_definition_unknown" do + verdict = judge_thread(diff_override: {:error, {:file_list_truncated, 300}}) + + assert verdict.decision == :refuse + assert {:ci_definition_unknown, {:file_list_truncated, 300}} in verdict.reasons + end + + # Round 1 of #910, finding 5: contention reading the ci entry is a retry, not a crash. + test "a ci-entry read that met contention is unevaluated" do + verdict = judge_thread([], ci_entered_at: {:error, :busy}) + + assert verdict.decision == :unevaluated + assert {:ci_entry_unreadable, :busy} in verdict.reasons + end + # Review round 2, finding 3: a CI wait holds back only an allow. test "a refusal is decided now, not held back while CI is still running" do - running = %{check_runs: [run("test", "queued", nil)], statuses: []} + running = %{jobs: [run("test", "queued", nil)], statuses: []} verdict = judge_thread(head_tree_sha: String.duplicate("f", 40), ci: running) assert verdict.decision == :refuse @@ -1343,10 +1359,10 @@ defmodule Loopctl.Delivery.MergePreconditionJudgeTest do # Review round 1, findings 1 and 2: a CI wait is bounded in TIME, never by polls. test "a CI wait, running or missing, is left out of the unevaluated bound" do - pending = judge_thread(ci: %{check_runs: [run("test", "queued", nil)], statuses: []}) + pending = judge_thread(ci: %{jobs: [run("test", "queued", nil)], statuses: []}) refute MergePrecondition.counts_toward_unevaluated_bound?(pending) - missing = judge_thread(ci: %{check_runs: [], statuses: []}) + missing = judge_thread(ci: %{jobs: [], statuses: []}) refute MergePrecondition.counts_toward_unevaluated_bound?(missing) forge = judge_thread([], ci_evidence: {:error, {:github_unreachable, :timeout}}) @@ -1358,18 +1374,18 @@ defmodule Loopctl.Delivery.MergePreconditionJudgeTest do late = DateTime.add(entered, MergePrecondition.ci_wait_limit_seconds() + 1) on_time = DateTime.add(entered, MergePrecondition.ci_wait_limit_seconds()) - stuck = %{check_runs: [run("test", "in_progress", nil)], statuses: []} + stuck = %{jobs: [run("test", "in_progress", nil)], statuses: []} - refused = judge_thread([ci: stuck], ci_entered_at: entered, now: late) + refused = judge_thread([ci: stuck], ci_entered_at: {:ok, entered}, now: late) assert refused.decision == :refuse assert {:required_check_timed_out, "test", :pending} in refused.reasons never = - judge_thread([ci: %{check_runs: [], statuses: []}], ci_entered_at: entered, now: late) + judge_thread([ci: %{jobs: [], statuses: []}], ci_entered_at: {:ok, entered}, now: late) assert {:required_check_timed_out, "test", :missing} in never.reasons - waiting = judge_thread([ci: stuck], ci_entered_at: entered, now: on_time) + waiting = judge_thread([ci: stuck], ci_entered_at: {:ok, entered}, now: on_time) assert waiting.decision == :unevaluated end end @@ -1414,6 +1430,7 @@ defmodule Loopctl.Delivery.MergePreconditionJudgeTest do checkpoint: {:ok, checkpoint}, claim_live?: Keyword.get(overrides, :claim_live?, true), required_checks: Keyword.get(overrides, :required_checks, ["test"]), + ci_entered_at: {:ok, nil}, ci_evidence: {:ok, ci_read(Keyword.get(overrides, :ci, green_ci()))} }) |> Map.merge(Map.new(fact_overrides)) @@ -1421,7 +1438,7 @@ defmodule Loopctl.Delivery.MergePreconditionJudgeTest do end # US-45.6: the checkpoint commit's CI as `gather/3` reads it. - defp green_ci, do: %{check_runs: [run("test", "completed", "success")], statuses: []} + defp green_ci, do: %{jobs: [run("test", "completed", "success")], statuses: []} defp run(name, status, conclusion), do: %{name: name, status: status, conclusion: conclusion, app: "github-actions"} diff --git a/test/loopctl/delivery/stages_test.exs b/test/loopctl/delivery/stages_test.exs index ed2312b1..0584d230 100644 --- a/test/loopctl/delivery/stages_test.exs +++ b/test/loopctl/delivery/stages_test.exs @@ -438,10 +438,10 @@ defmodule Loopctl.Delivery.StagesTest do {story, _row} = at_stage(:pr_open) opts = [claim_epoch: story.claim_epoch] - assert Stages.entered_at(story.tenant_id, story.id, :ci) == nil + assert Stages.entered_at(story.tenant_id, story.id, :ci) == {:ok, nil} {:ok, _} = Stages.advance(story.tenant_id, story.id, {:pr_open, :ci}, opts) - first = Stages.entered_at(story.tenant_id, story.id, :ci) + {:ok, first} = Stages.entered_at(story.tenant_id, story.id, :ci) assert %DateTime{} = first {:ok, _} = Stages.advance(story.tenant_id, story.id, {:ci, :implementing, :ci_red}, opts) @@ -452,15 +452,15 @@ defmodule Loopctl.Delivery.StagesTest do {:ok, _} = Stages.advance(story.tenant_id, story.id, {from, to}, opts) end) - second = Stages.entered_at(story.tenant_id, story.id, :ci) + {:ok, second} = Stages.entered_at(story.tenant_id, story.id, :ci) assert DateTime.compare(second, first) == :gt # A later transition OUT of the stage does not move when it was entered. {:ok, _} = Stages.advance(story.tenant_id, story.id, {:ci, :implementing, :ci_red}, opts) - assert Stages.entered_at(story.tenant_id, story.id, :ci) == second + assert Stages.entered_at(story.tenant_id, story.id, :ci) == {:ok, second} # Another tenant's story is never read. - assert Stages.entered_at(fixture(:tenant).id, story.id, :ci) == nil + assert Stages.entered_at(fixture(:tenant).id, story.id, :ci) == {:ok, nil} end end diff --git a/test/loopctl/intake_test.exs b/test/loopctl/intake_test.exs index 3a53d0ec..23d68672 100644 --- a/test/loopctl/intake_test.exs +++ b/test/loopctl/intake_test.exs @@ -338,7 +338,16 @@ defmodule Loopctl.IntakeTest do test "blank, duplicate and oversized names are refused", ctx do # A JSON null element casts to nil: it must be a 422, never a raise (round 1, finding 3). - for bad <- [[""], [" "], ["test", "test"], [String.duplicate("x", 201)], [nil], ["a", nil]] do + for bad <- [ + [""], + [" "], + ["test", "test"], + [String.duplicate("x", 201)], + [nil], + ["a", nil], + ["test "], + [" lint"] + ] do assert {:error, _changeset} = enrol(ctx, %{mode: :thread, required_checks: bad}), inspect(bad) end diff --git a/test/support/data_case.ex b/test/support/data_case.ex index 8b5f30af..39119bdf 100644 --- a/test/support/data_case.ex +++ b/test/support/data_case.ex @@ -226,7 +226,7 @@ defmodule Loopctl.DataCase do end) # US-45.6: CI evidence for a checkpoint's commit, on the same fail-closed default. - Mox.stub(Loopctl.MockPullRequestSource, :check_evidence, fn _repo, _sha -> + Mox.stub(Loopctl.MockPullRequestSource, :check_evidence, fn _repo, _sha, _branch -> {:error, :not_stubbed} end) From 24c70efcb7c9b9eb16147cda708e000d72c48a72 Mon Sep 17 00:00:00 2001 From: Mark Kreyman Date: Sun, 27 Sep 2026 01:10:09 -0600 Subject: [PATCH 3/4] US-45.6 v2 review round 2: every job in the newest run counts 1. Per workflow, only its newest run counts, and in it EVERY job carrying a required name must pass: matrix legs sharing a name are separate jobs (round 1 wrongly removed the newest-run step and judged the highest job id alone). 2. The ci entry time is read on exactly the path that reads CI, so a lock wait on it can no longer mask a merged or moved head's decision. 3. An identical evidence read that is later advances the stored read_at, so a slower read from in between is :superseded; an identical earlier read is :ok. 4. OpenAPI ci_evidence, the delivery-loop doc and the Source field comment describe the jobs design, not v1's check runs and statuses. 5. A composite action's action.yml anywhere in the diff is a CI definition change. 6. The jobs list's shape is judged before de-duplicating, so a malformed entry is unreadable_jobs, never a crash. 7. Per-run jobs reads run concurrently (4 at a time); the moduledoc restates the ceiling. 8. The migration names the manual step for thread sources enrolled before it; CHANGELOG gives the real column type. 9. ci_result/1 uses Map.fetch!: CI is judged once, in judge/1. Mutations re-run: A01-A47 and B01-B15 as retargeted, plus C01, C02, C04-C10 (C10 and A40 re-run as C10b/A40b after a test for the identical-earlier case). All exit 0. Finding 2's gating has no falsifiable test: which path reads the entry time is not observable from a test. --- CHANGELOG.md | 2 +- docs/agent-delivery-loop.md | 2 +- lib/loopctl/delivery/ci_evidence.ex | 32 +++++++---- .../delivery/github_pull_request_source.ex | 54 ++++++++++++------- lib/loopctl/delivery/merge_precondition.ex | 29 ++++++---- lib/loopctl/intake/source.ex | 5 +- lib/loopctl/threads.ex | 33 ++++++++++-- .../merge_precondition_controller.ex | 10 ++-- ...000_add_intake_sources_required_checks.exs | 13 +++-- test/loopctl/delivery/ci_evidence_test.exs | 13 +++-- .../github_pull_request_source_test.exs | 12 +++++ .../merge_precondition_judge_test.exs | 3 ++ test/loopctl/threads_test.exs | 13 ++++- 13 files changed, 160 insertions(+), 61 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 87681f5e..b21ad616 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,7 +10,7 @@ All notable changes to loopctl are documented here. migration `20260927100000`). A THREAD-mode source must now name its `required_checks`, and the gate's `GITHUB_TOKEN` needs `actions: read` (and, best effort, `commit statuses: read`) for it.** The - migration adds `intake_sources.required_checks` (text array, NOT NULL, default empty; no + migration adds `intake_sources.required_checks` (`varchar(255)[]`, NOT NULL, default empty; no backfill). `POST`/`PATCH /api/v1/intake/sources` and the `intake_source_enroll` / `intake_source_update` MCP tools take it; a `thread` source naming none is 422, judged over the source as it will be, `local-gate` is refused, and a change is recorded as diff --git a/docs/agent-delivery-loop.md b/docs/agent-delivery-loop.md index fa352bf9..ef6a1485 100644 --- a/docs/agent-delivery-loop.md +++ b/docs/agent-delivery-loop.md @@ -122,7 +122,7 @@ Before a merge, an orchestrator or operator calls `merge_precondition` (`POST /a | `head_moved` | The pull request's head changed since CI ran. The story goes back to `implementing` over `base_moved`, and the recorded head is cleared, so the gate is not called again until the story is back at `ci`. | | `unevaluated` | 503 with `Retry-After`. After repeated `unevaluated` answers the story escalates. | -**Thread mode (epic 45, US-45.4).** A story whose claim was PLACED under an intake source with `mode: thread` (`intake_source_update`) has no pull request. The mode, and the base branch, are recorded on the implement dispatch at placement, so changing the source affects only stories placed afterwards; a change is always allowed. The gate judges the latest checkpoint the story's CURRENT claim recorded, on the branch that claim's dispatch ran on and against the base branch it was placed on, needs no `pr_number`, and also refuses `empty_change` (the checkpoint's tree equals the base's, or no file changed), `checkpoint_tree_mismatch`, `no_checkpoint_recorded`, `claim_ended` (the current claim recorded nothing but an earlier, released one did), and `thread_unreadable` (loopctl could not read the thread). The branch is judged first: a branch missing from a readable repository (`branch_missing`), one naming a commit nobody reported (`branch_head_unrecorded`), one naming an earlier checkpoint of the claim (`branch_head_regressed`), or a checkpoint that is not the recorded head means the head moved. The diff judged is the checkpoint's three-dot diff against its merge base, so a base that moved on is not a refusal, and a checkpoint the base already contains, its branch deleted or not, is `already_merged` only under a recorded allow naming it, and otherwise refused `checkpoint_on_base_without_allow`. A claim with no accepted dispatch (a session that claimed the story itself) is judged as a pull request against the source's current base branch, whatever the source's mode. **While the claim is live** that is `head_moved`, back to `implementing` like any moved head. **When it is not live** (reported, review requested, lease expired) the claimant cannot record a fix, so the gate refuses naming `claim_not_live` and the story escalates instead of looping. A repository the token cannot read refuses `pull_request_unavailable`. An allow is recorded naming the checkpoint id and sha and `base_sha`, the merge base the judged diff is relative to. **Thread mode needs the source's runners at runner contract 1.20.0 or later sending `checkpoint` messages; otherwise every story is refused `no_checkpoint_recorded`.** **CI is read by the checkpoint's exact SHA (US-45.6)**, from both the check-runs and the commit-status APIs (only a job of a GitHub Actions workflow run that a push of the thread branch at that commit triggered satisfies a required check, and only by concluding `success` — a skipped job fails; commit statuses and check runs created any other way are never trusted, because the implementer can create them), against the source's `required_checks` (a thread source must name at least one; set with `intake_source_update`): a failed one refuses `required_check_failed`; one still running or not yet reported answers `unevaluated` with a `Retry-After` and never counts toward the unevaluated bound; past the gate's CI wait limit from the story's entry into `ci` both are refused `required_check_timed_out`, so a slow pipeline waits and a stuck or missing check still reaches a human. Per name the newest run of each workflow counts and every workflow must pass; the required checks are the source's current list, so each required job must run on every push to the thread branches; and a checkpoint changing `.github/workflows/` or `.github/actions/` is refused `ci_definition_changed` for a human, because Actions runs the workflow files of the commit under test; a source requiring none refuses `required_checks_unset`. A `local-gate` status is recorded on the checkpoint and never satisfies a required check, because whoever pushed posts it. What was read is copied onto the checkpoint's `gate_evidence` under `ci`. The merge executor (US-45.5) merges only while the base head still equals that `base_sha`, and otherwise takes its base-update path (US-45.5). This gate judges claimant checkpoints only; reading the executor's `base_update` checkpoints is US-45.5's (AC-45.5.9). +**Thread mode (epic 45, US-45.4).** A story whose claim was PLACED under an intake source with `mode: thread` (`intake_source_update`) has no pull request. The mode, and the base branch, are recorded on the implement dispatch at placement, so changing the source affects only stories placed afterwards; a change is always allowed. The gate judges the latest checkpoint the story's CURRENT claim recorded, on the branch that claim's dispatch ran on and against the base branch it was placed on, needs no `pr_number`, and also refuses `empty_change` (the checkpoint's tree equals the base's, or no file changed), `checkpoint_tree_mismatch`, `no_checkpoint_recorded`, `claim_ended` (the current claim recorded nothing but an earlier, released one did), and `thread_unreadable` (loopctl could not read the thread). The branch is judged first: a branch missing from a readable repository (`branch_missing`), one naming a commit nobody reported (`branch_head_unrecorded`), one naming an earlier checkpoint of the claim (`branch_head_regressed`), or a checkpoint that is not the recorded head means the head moved. The diff judged is the checkpoint's three-dot diff against its merge base, so a base that moved on is not a refusal, and a checkpoint the base already contains, its branch deleted or not, is `already_merged` only under a recorded allow naming it, and otherwise refused `checkpoint_on_base_without_allow`. A claim with no accepted dispatch (a session that claimed the story itself) is judged as a pull request against the source's current base branch, whatever the source's mode. **While the claim is live** that is `head_moved`, back to `implementing` like any moved head. **When it is not live** (reported, review requested, lease expired) the claimant cannot record a fix, so the gate refuses naming `claim_not_live` and the story escalates instead of looping. A repository the token cannot read refuses `pull_request_unavailable`. An allow is recorded naming the checkpoint id and sha and `base_sha`, the merge base the judged diff is relative to. **Thread mode needs the source's runners at runner contract 1.20.0 or later sending `checkpoint` messages; otherwise every story is refused `no_checkpoint_recorded`.** **CI is read by the checkpoint's exact SHA (US-45.6)** (only a job of a GitHub Actions workflow run that a push of the thread branch at that commit triggered satisfies a required check, and only by concluding `success` — a skipped job fails; commit statuses and check runs created any other way are never trusted, because the implementer can create them), against the source's `required_checks` (a thread source must name at least one; set with `intake_source_update`): a failed one refuses `required_check_failed`; one still running or not yet reported answers `unevaluated` with a `Retry-After` and never counts toward the unevaluated bound; past the gate's CI wait limit from the story's entry into `ci` both are refused `required_check_timed_out`, so a slow pipeline waits and a stuck or missing check still reaches a human. Per name the newest run of each workflow counts and every workflow must pass; the required checks are the source's current list, so each required job must run on every push to the thread branches; and a checkpoint changing `.github/workflows/` or `.github/actions/` is refused `ci_definition_changed` for a human, because Actions runs the workflow files of the commit under test; a source requiring none refuses `required_checks_unset`. A `local-gate` status is recorded on the checkpoint and never satisfies a required check, because whoever pushed posts it. What was read is copied onto the checkpoint's `gate_evidence` under `ci`. The merge executor (US-45.5) merges only while the base head still equals that `base_sha`, and otherwise takes its base-update path (US-45.5). This gate judges claimant checkpoints only; reading the executor's `base_update` checkpoints is US-45.5's (AC-45.5.9). ## 7. After the merge diff --git a/lib/loopctl/delivery/ci_evidence.ex b/lib/loopctl/delivery/ci_evidence.ex index 7ba1302b..63c20c76 100644 --- a/lib/loopctl/delivery/ci_evidence.ex +++ b/lib/loopctl/delivery/ci_evidence.ex @@ -33,11 +33,12 @@ defmodule Loopctl.Delivery.CiEvidence do ## How one required check is judged - Per workflow file, only the job with the name and the highest id counts: job ids only grow, - so that is the newest attempt of the newest run, and a re-run that went green supersedes - the failure it re-ran (the jobs read is `filter=latest` as well). ACROSS workflows nothing supersedes anything: two workflows - with a job `test` are two checks under one name, and a green one must never hide a red one. - So a name is `:failed` when any workflow's job failed, `:pending` when none failed and any + Per workflow file, only its NEWEST run counts (a later push run of the same commit + supersedes an earlier one), and in it EVERY job carrying the name — matrix legs are separate + jobs, and one green leg must never hide a red one. Each job is its latest attempt (the jobs + read is `filter=latest`), so a re-run that went green supersedes the failure it re-ran. + ACROSS workflows nothing supersedes anything either: two workflows with a job `test` are two + checks under one name. So a name is `:failed` when any counted job failed, `:pending` when none failed and any is still running, `:passed` only when every one passed, and `:missing` when no trusted job carries it. A job: @@ -102,11 +103,9 @@ defmodule Loopctl.Delivery.CiEvidence do defp check_state(name, jobs) do states = jobs + |> newest_run_per_workflow() |> Enum.filter(&(&1.name == name)) - |> Enum.group_by(&Map.get(&1, :workflow)) - |> Enum.map(fn {_workflow, named} -> - named |> Enum.max_by(&(Map.get(&1, :id) || 0)) |> job_state() - end) + |> Enum.map(&job_state/1) cond do states == [] -> :missing @@ -116,6 +115,21 @@ defmodule Loopctl.Delivery.CiEvidence do end end + # Per workflow file, the jobs of its NEWEST run only: a later push run of the commit + # supersedes an earlier one. Within that run EVERY job counts — matrix legs and any jobs + # that share a name are separate jobs, and one green leg must never hide a red one (#910 + # round 2, finding 1). The jobs read is `filter=latest`, so each is its latest attempt. + defp newest_run_per_workflow(jobs) do + newest = + jobs + |> Enum.group_by(&Map.get(&1, :workflow), &(Map.get(&1, :run_id) || 0)) + |> Map.new(fn {workflow, run_ids} -> {workflow, Enum.max(run_ids)} end) + + Enum.filter(jobs, fn job -> + (Map.get(job, :run_id) || 0) == Map.fetch!(newest, Map.get(job, :workflow)) + end) + end + defp job_state(%{status: "completed", conclusion: "success"}), do: :passed defp job_state(%{status: "completed", conclusion: conclusion}), diff --git a/lib/loopctl/delivery/github_pull_request_source.ex b/lib/loopctl/delivery/github_pull_request_source.ex index a806872c..472f1ce1 100644 --- a/lib/loopctl/delivery/github_pull_request_source.ex +++ b/lib/loopctl/delivery/github_pull_request_source.ex @@ -70,8 +70,12 @@ defmodule Loopctl.Delivery.GitHubPullRequestSource do all five (a pull request plus two refs) waits at most 35 seconds before it has an answer, and the answer to a timeout is an ESCALATION, never a pass. A THREAD-mode evaluation makes more: the branch ref, the commit, the comparison, two trees, the workflow runs, one jobs - read per workflow run and the combined status — about eight calls for a repository with a - couple of workflows, plus one repository read after a 404 on the branch. Nothing here runs inside a + read per workflow run (at most `@max_workflow_runs`, read `@job_read_concurrency` at a time) + and the combined status. Their ceiling is the sum of the sequential reads' timeouts plus + the jobs reads' in batches: with ten workflows, seven reads plus three batches at 7s each, + 70 seconds before an answer, and a + timeout is a transient `:unevaluated`, never a pass. Plus one repository read after a 404 on + the branch. Nothing here runs inside a database transaction: the caller gathers every fact before it opens one, so a slow forge never holds a pooled connection. @@ -126,6 +130,8 @@ defmodule Loopctl.Delivery.GitHubPullRequestSource do @workflow_run_page 100 @job_page 100 @max_workflow_runs 10 + # How many of those jobs reads run at once; each is bounded by `req_options/0`'s timeouts. + @job_read_concurrency 4 # GitHub's primary rate-limit window is an hour. Anything beyond that plus slack is not a # window rolling over, so it is not turned into a delay a caller would sleep on. @@ -274,32 +280,40 @@ defmodule Loopctl.Delivery.GitHubPullRequestSource do # Each run's jobs, latest attempt only (`filter=latest`), tagged with the run and its # workflow file. De-duplicated by job id and checked against the total, so a page boundary # that moved between reads can neither repeat a job nor hide one. + # + # CONCURRENT, bounded (#910 round 2, finding 7): the reads are independent, and done one after + # another a push that triggered many workflows multiplied the wait by their number. The first + # failure in run order is the answer, so the outcome does not depend on which read is slower. defp jobs_of(repo, runs) do - Enum.reduce_while(runs, {:ok, []}, fn run, {:ok, acc} -> - query = URI.encode_query(%{"filter" => "latest", "per_page" => @job_page}) + query = URI.encode_query(%{"filter" => "latest", "per_page" => @job_page}) - with {:ok, body} <- get(repo, "/actions/runs/#{run["id"]}/jobs?" <> query), - {:ok, jobs} <- run_jobs(body, run) do - {:cont, {:ok, acc ++ jobs}} - else - {:error, _reason} = error -> {:halt, error} - end + runs + |> Task.async_stream( + fn run -> + with {:ok, body} <- get(repo, "/actions/runs/#{run["id"]}/jobs?" <> query) do + run_jobs(body, run) + end + end, + max_concurrency: @job_read_concurrency, + timeout: :infinity + ) + |> Enum.reduce_while({:ok, []}, fn + {:ok, {:ok, jobs}}, {:ok, acc} -> {:cont, {:ok, acc ++ jobs}} + {:ok, {:error, _reason} = error}, _acc -> {:halt, error} end) end defp run_jobs(%{"total_count" => total, "jobs" => jobs}, run) when is_integer(total) and is_list(jobs) do - jobs = Enum.uniq_by(jobs, & &1["id"]) + # Shape FIRST: de-duplicating reads `"id"` off each entry, which raises on a non-map. + if Enum.all?(jobs, &job?/1) do + jobs = Enum.uniq_by(jobs, & &1["id"]) - cond do - not Enum.all?(jobs, &job?/1) -> - {:error, {:unreadable_jobs, shape(jobs)}} - - total > length(jobs) -> - {:error, {:jobs_truncated, total, length(jobs)}} - - true -> - {:ok, Enum.map(jobs, &job_fact(&1, run))} + if total > length(jobs), + do: {:error, {:jobs_truncated, total, length(jobs)}}, + else: {:ok, Enum.map(jobs, &job_fact(&1, run))} + else + {:error, {:unreadable_jobs, shape(jobs)}} end end diff --git a/lib/loopctl/delivery/merge_precondition.ex b/lib/loopctl/delivery/merge_precondition.ex index 6bae0843..4d800b72 100644 --- a/lib/loopctl/delivery/merge_precondition.ex +++ b/lib/loopctl/delivery/merge_precondition.ex @@ -766,7 +766,8 @@ defmodule Loopctl.Delivery.MergePrecondition do end) end - defp ci_result(facts), do: Map.get_lazy(facts, :ci_result, fn -> judge_ci(facts) end) + # Put by `judge/1` before anything reads it: CI is judged once per evaluation. + defp ci_result(facts), do: Map.fetch!(facts, :ci_result) defp judge_ci(facts) do case value(facts, :ci_evidence) do @@ -1003,7 +1004,8 @@ defmodule Loopctl.Delivery.MergePrecondition do # edits `.github/workflows/` — or a composite action they call — can make its own required # checks report green: the implementer attesting its own work through a check run it never # needed an App to create. A human merges such a change. Matched on every name the diff - # carries, renames' old and new names included, so moving a workflow file is caught too. + # carries, renames' old and new names included, so moving a workflow file is caught too, + # and on any composite action's `action.yml`, wherever it sits. @ci_definition_prefixes [".github/workflows/", ".github/actions/"] defp ci_definition_reasons({:ok, %{files: files} = diff}) do @@ -1021,8 +1023,13 @@ defmodule Loopctl.Delivery.MergePrecondition do defp ci_definition_reasons({:error, reason}), do: [{:ci_definition_unknown, reason}] defp ci_definition_reasons(_no_diff), do: [] - defp ci_definition?(name) when is_binary(name), - do: Enum.any?(@ci_definition_prefixes, &String.starts_with?(name, &1)) + # A composite action can live anywhere a workflow's `uses: ./path` points, so its definition + # file is matched by NAME wherever it sits (#910 round 2, finding 5); reusable workflows can + # only live under `.github/workflows/`, which the prefix covers. + defp ci_definition?(name) when is_binary(name) do + Enum.any?(@ci_definition_prefixes, &String.starts_with?(name, &1)) or + Path.basename(name) in ["action.yml", "action.yaml"] + end defp ci_definition?(_name), do: false @@ -1402,8 +1409,6 @@ defmodule Loopctl.Delivery.MergePrecondition do # with its list cleared makes a thread already placed refuse `required_checks_unset`, # which names the fix — setting the list again — and is not a dead end. required_checks: source_required_checks(source), - # When the story entered `ci`: what a CI wait is measured from. - ci_entered_at: ci_entered_at(mode, story, checkpoint), now: DateTime.utc_now() } @@ -1418,7 +1423,11 @@ defmodule Loopctl.Delivery.MergePrecondition do Map.merge(facts, %{ head_files: repo_files(repo, pull_request, :head_sha, skip?), base_files: repo_files(repo, pull_request, :merge_base_sha, skip?), - ci_evidence: ci_evidence(facts, pull_request, skip?) + ci_evidence: ci_evidence(facts, pull_request, skip?), + # When the story entered `ci`, what a CI wait is measured from — read on exactly the path + # that reads CI, so a lock wait on it can never mask a merged or moved head's decision + # (#910 round 2, finding 2). + ci_entered_at: ci_entered_at(facts, pull_request, skip?, story) }) end @@ -1426,15 +1435,15 @@ defmodule Loopctl.Delivery.MergePrecondition do # (contention is `{:error, :busy}`, a retry). A story that reached `ci` with no recorded # transition — a backfill, a repair — falls back to its checkpoint's recording, which is # always known, so the wait is bounded for every story (round 1 of #910, finding 8). - defp ci_entered_at(:thread, story, checkpoint) do + defp ci_entered_at(%{mode: :thread} = facts, {:ok, %{merged?: false}}, false = _skip?, story) do case Stages.entered_at(story.tenant_id, story.id, :ci) do {:ok, %DateTime{} = entered} -> {:ok, entered} - {:ok, nil} -> {:ok, checkpoint_recorded_at(checkpoint)} + {:ok, nil} -> {:ok, checkpoint_recorded_at(facts.checkpoint)} {:error, _reason} = error -> error end end - defp ci_entered_at(_mode, _story, _checkpoint), do: {:ok, nil} + defp ci_entered_at(_facts, _pull_request, _skip?, _story), do: {:ok, nil} defp checkpoint_recorded_at({:ok, %{recorded_at: %DateTime{} = at}}), do: at defp checkpoint_recorded_at(_checkpoint), do: nil diff --git a/lib/loopctl/intake/source.ex b/lib/loopctl/intake/source.ex index 7bc521b9..af7f5318 100644 --- a/lib/loopctl/intake/source.ex +++ b/lib/loopctl/intake/source.ex @@ -77,8 +77,9 @@ defmodule Loopctl.Intake.Source do # THE CHECKS A THREAD-MODE CHECKPOINT MUST PASS ON ITS EXACT COMMIT (US-45.6). A pull # request's required checks are its base branch's protection, enforced by GitHub at merge # time; a thread has no pull request and the loopctl App pushes the squash itself, so the - # merge gate reads the checks by SHA and this names which ones it requires. Check-run - # names or commit-status contexts, as they appear on the commit. `pr` mode never reads it. + # merge gate reads the checks by SHA and this names which ones it requires: GitHub Actions + # JOB names, as they appear on the commit (a commit-status context can never satisfy one; + # see `Loopctl.Delivery.CiEvidence`). `pr` mode never reads it. field :required_checks, {:array, :string}, default: [] field :webhook_secret, Loopctl.Vault.Binary, redact: true field :revoked_at, :utc_datetime_usec diff --git a/lib/loopctl/threads.ex b/lib/loopctl/threads.ex index 7dd24048..4819763f 100644 --- a/lib/loopctl/threads.ex +++ b/lib/loopctl/threads.ex @@ -529,6 +529,26 @@ defmodule Loopctl.Threads do :ok + # Same judgement, read later: only the stored `read_at` moves, so a slower evaluation + # that read in between can no longer pass for the newer one (#910 round 2, finding 3). + :advance_read_at -> + {1, _} = + from(c in Checkpoint, where: c.id == ^checkpoint_id) + |> update([c], + set: [ + gate_evidence: + fragment( + "jsonb_set(?, ARRAY[?, 'read_at'], to_jsonb(?::text))", + c.gate_evidence, + ^key, + ^Map.get(record, "read_at") + ) + ] + ) + |> Repo.update_all([]) + + :ok + answer -> answer end @@ -537,15 +557,20 @@ defmodule Loopctl.Threads do # The decision, taken under the row lock so two evaluations cannot both think they are the # newer one. A record read no LATER than the stored one is `:superseded`, never `:ok`: the # allow path must not record an allow on evidence that did not land (round 2, finding 4). - # One that says exactly what is stored already, read_at aside, is `:ok` with no write, so a - # CI wait polled for hours rewrites the row only when the judgement moves (finding 6). + # One that says exactly what is stored already, read_at aside, rewrites nothing but the + # stored `read_at` (when it is later), so a CI wait polled for hours rewrites the judgement + # only when it moves (finding 6) and the ordering guard stays current. defp evidence_write(nil, _record), do: {:error, :not_found} defp evidence_write(%{record: nil}, _record), do: :write defp evidence_write(%{record: stored}, record) do + later? = later?(Map.get(record, "read_at"), Map.get(stored, "read_at")) + same? = Map.delete(stored, "read_at") == Map.delete(record, "read_at") + cond do - Map.delete(stored, "read_at") == Map.delete(record, "read_at") -> :ok - not later?(Map.get(record, "read_at"), Map.get(stored, "read_at")) -> :superseded + same? and later? and is_binary(Map.get(record, "read_at")) -> :advance_read_at + same? -> :ok + not later? -> :superseded true -> :write end end diff --git a/lib/loopctl_web/controllers/merge_precondition_controller.ex b/lib/loopctl_web/controllers/merge_precondition_controller.ex index 32bb65d8..725bc210 100644 --- a/lib/loopctl_web/controllers/merge_precondition_controller.ex +++ b/lib/loopctl_web/controllers/merge_precondition_controller.ex @@ -159,10 +159,12 @@ defmodule LoopctlWeb.MergePreconditionController do type: :object, nullable: true, description: - "Thread mode (US-45.6): what CI said about the checkpoint's EXACT commit, read " <> - "from both the check-runs and the commit-status APIs — `sha`, `read_at`, " <> - "`required`, the raw `check_runs` and `statuses`, `local_gate` (recorded, " <> - "never counted), and the judgement `passed` / `pending` / `missing` / " <> + "Thread mode (US-45.6): what CI said about the checkpoint's EXACT commit — `sha`, " <> + "`read_at`, `required`, `jobs` (the jobs under a required name from the " <> + "GitHub Actions workflow runs a push of the thread branch triggered: `name`, " <> + "`workflow`, `run_id`, `status`, `conclusion`, `url`), `local_gate` (from the " <> + "commit statuses, best effort; recorded, never counted), and the judgement " <> + "`passed` / `pending` / `missing` / " <> "`failed`. The same object is copied onto the checkpoint's `gate_evidence` " <> "under `ci`. null when it was not read (pr mode, a moved or merged head)." }, diff --git a/priv/repo/migrations/20260927100000_add_intake_sources_required_checks.exs b/priv/repo/migrations/20260927100000_add_intake_sources_required_checks.exs index b93ab82a..733f0688 100644 --- a/priv/repo/migrations/20260927100000_add_intake_sources_required_checks.exs +++ b/priv/repo/migrations/20260927100000_add_intake_sources_required_checks.exs @@ -9,12 +9,15 @@ defmodule Loopctl.Repo.Migrations.AddIntakeSourcesRequiredChecks do gate reads the checks by SHA and needs to know which ones are required. They are named here, per repository. - NOT NULL, default the empty list, so every existing source is unchanged: a `pr`-mode source - never reads the column. A `thread`-mode source must name at least one check, which the - changeset enforces on every write (`Loopctl.Intake.Source.validate_required_checks/1`) and - the gate backstops by refusing `required_checks_unset`. + NOT NULL, default the empty list. A `pr`-mode source never reads the column and is + unchanged. A `thread`-mode source must name at least one check, which the changeset enforces + on every write (`Loopctl.Intake.Source.validate_required_checks/1`) and the gate backstops + by refusing `required_checks_unset`. - No backfill and no manual step. + MANUAL STEP for any `thread`-mode source enrolled BEFORE this migration: it has no required + checks, so every one of its stories is refused `required_checks_unset` until an operator + names them (`intake_source_update` with `required_checks`). No backfill: loopctl cannot know + which CI jobs a repository requires. """ use Ecto.Migration diff --git a/test/loopctl/delivery/ci_evidence_test.exs b/test/loopctl/delivery/ci_evidence_test.exs index e17deb15..7bc06f9c 100644 --- a/test/loopctl/delivery/ci_evidence_test.exs +++ b/test/loopctl/delivery/ci_evidence_test.exs @@ -73,11 +73,16 @@ defmodule Loopctl.Delivery.CiEvidenceTest do assert %{pending: ["test"]} = judge(["test"], [green, running]) end - test "within one run, the job's latest attempt (highest id) decides" do - first = job("test", "completed", "failure", %{id: 1}) - retry = job("test", "completed", "success", %{id: 2}) + # #910 round 2, finding 1: attempts are already collapsed by the `filter=latest` read, so two + # jobs of one run sharing a name are separate jobs (matrix legs), and every one must pass. + test "within one run, every job carrying the name must pass" do + leg_a = job("test", "completed", "failure", %{id: 1}) + leg_b = job("test", "completed", "success", %{id: 2}) - assert %{passed: ["test"]} = judge(["test"], [first, retry]) + assert %{failed: [{"test", "failure"}], passed: []} = judge(["test"], [leg_a, leg_b]) + + assert %{passed: ["test"]} = + judge(["test"], [%{leg_a | conclusion: "success"}, leg_b]) end test "each required name is judged on its own" do diff --git a/test/loopctl/delivery/github_pull_request_source_test.exs b/test/loopctl/delivery/github_pull_request_source_test.exs index 5a8aa627..6219b300 100644 --- a/test/loopctl/delivery/github_pull_request_source_test.exs +++ b/test/loopctl/delivery/github_pull_request_source_test.exs @@ -852,6 +852,18 @@ defmodule Loopctl.Delivery.GitHubPullRequestSourceTest do assert {:error, {:jobs_truncated, 2, 1}} = Source.check_evidence(@repo, @head, @branch) end + # #910 round 2, finding 6: shape is judged before anything reads an entry's fields. + test "a jobs list with a malformed entry is unreadable, never a crash" do + stub_ci(%{ + runs: %{"total_count" => 1, "workflow_runs" => [gh_run(5)]}, + jobs: %{5 => %{"total_count" => 2, "jobs" => [gh_job(50, "test", "success"), "junk"]}}, + statuses: %{"total_count" => 0, "statuses" => []} + }) + + assert {:error, {:unreadable_jobs, {:list, 2}}} = + Source.check_evidence(@repo, @head, @branch) + end + test "more workflow runs than the bound is refused rather than read" do runs = for id <- 1..11, do: gh_run(id) diff --git a/test/loopctl/delivery/merge_precondition_judge_test.exs b/test/loopctl/delivery/merge_precondition_judge_test.exs index aec3eea9..24bee72a 100644 --- a/test/loopctl/delivery/merge_precondition_judge_test.exs +++ b/test/loopctl/delivery/merge_precondition_judge_test.exs @@ -1280,6 +1280,9 @@ defmodule Loopctl.Delivery.MergePreconditionJudgeTest do for {files, renames} <- [ {[".github/workflows/ci.yml"], []}, {[".github/actions/setup/action.yml"], []}, + # #910 round 2, finding 5: a composite action a workflow calls from anywhere. + {["ci/run-tests/action.yml"], []}, + {["action.yaml"], []}, {["ci/new.yml"], [{".github/workflows/old.yml", "ci/new.yml"}]} ] do pr_diff = {:ok, %{files: files, renames: renames}} diff --git a/test/loopctl/threads_test.exs b/test/loopctl/threads_test.exs index dd1c3302..cd6166ce 100644 --- a/test/loopctl/threads_test.exs +++ b/test/loopctl/threads_test.exs @@ -298,7 +298,18 @@ defmodule Loopctl.ThreadsTest do assert :ok = Threads.record_gate_evidence(ctx.tenant_id, ctx.story.id, cp.id, "ci", same_later) - assert %{"ci" => ^newer} = stored_evidence(ctx, cp.id) + # #910 round 2, finding 3: the same judgement read later moves the stored read_at, so a + # slower evaluation that read in between (10:00:03) can no longer pass for the newer one. + assert %{"ci" => ^same_later} = stored_evidence(ctx, cp.id) + + # The same judgement read EARLIER than what is stored is :ok, never :superseded: the + # stored record already says the same thing, so an allow resting on it may stand. + assert :ok = Threads.record_gate_evidence(ctx.tenant_id, ctx.story.id, cp.id, "ci", newer) + assert %{"ci" => ^same_later} = stored_evidence(ctx, cp.id) + in_between = %{"read_at" => "2026-09-27T10:00:03.000000Z", "pending" => ["test"]} + + assert :superseded = + Threads.record_gate_evidence(ctx.tenant_id, ctx.story.id, cp.id, "ci", in_between) newest = %{"read_at" => "2026-09-27T10:00:10.000000Z", "failed" => ["test"]} assert :ok = Threads.record_gate_evidence(ctx.tenant_id, ctx.story.id, cp.id, "ci", newest) From 11c8cd6c4c59ec41182ebee5c9b81b160eb29d29 Mon Sep 17 00:00:00 2001 From: Mark Kreyman Date: Sun, 27 Sep 2026 01:38:36 -0600 Subject: [PATCH 4/4] US-45.6 v2 review round 3 (the ceiling): runs judged from runs, evidence on decisions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixed in place (no round 4; every fix carries mutation proof), following #902's round 3: none of the findings touched the trust model the rewrite settled. - Runs (findings 2, 3, 4, 7): the adapter reduces the push runs to each workflow's NEWEST run before bounding and before any jobs read, and returns those runs. CiEvidence takes the newest run per workflow from the runs, once per judgement: a newest run with no jobs yet holds a name pending; one that ended with no jobs (startup_failure) fails a name no job carries (run_). - Evidence (findings 1, 5): read_at is stamped when the read STARTS; evidence is copied onto the checkpoint only for a decision (allow, refuse), so CI-wait polls write nothing. - Latency (finding 6): the gate's three forge reads (two trees, CI evidence) run concurrently. - MCP (finding 9): required_checks refuses surrounding whitespace and duplicates locally, matching the server. - Counter (finding 8): kept clearing on a pure CI wait (round 2's decision); documented why the wait is still bounded — every answered poll is judged against the CI wait limit. Mutations: the cited set re-run (A06b, A40b, C10b retargeted) and D01-D05, D07-D09 (D03 re-proved as D03b after a complexity split), all 60 exit 0. --- lib/loopctl/delivery/ci_evidence.ex | 61 +++++++++++++------ .../delivery/github_pull_request_source.ex | 28 ++++++++- lib/loopctl/delivery/merge_precondition.ex | 44 ++++++++++--- mcp-server/lib/intake-sources.js | 11 +++- mcp-server/test/intake_source_tools.test.js | 2 + test/loopctl/delivery/ci_evidence_test.exs | 33 ++++++++++ .../github_pull_request_source_test.exs | 30 ++++++++- .../merge_precondition_integration_test.exs | 25 ++++++++ 8 files changed, 199 insertions(+), 35 deletions(-) diff --git a/lib/loopctl/delivery/ci_evidence.ex b/lib/loopctl/delivery/ci_evidence.ex index 63c20c76..2882eef9 100644 --- a/lib/loopctl/delivery/ci_evidence.ex +++ b/lib/loopctl/delivery/ci_evidence.ex @@ -83,14 +83,20 @@ defmodule Loopctl.Delivery.CiEvidence do @doc "Judges `evidence` for one commit against `required`. See the moduledoc." @spec judge([String.t()], evidence()) :: result() - def judge(required, %{jobs: jobs, statuses: statuses}) do + def judge(required, %{jobs: jobs, statuses: statuses} = evidence) do + # The newest run of each workflow, and its jobs, computed ONCE per judgement. + runs = newest_runs(Map.get(evidence, :runs), jobs) + newest_ids = MapSet.new(runs, & &1.id) + job_run_ids = MapSet.new(jobs, &run_id/1) + counted = Enum.filter(jobs, &(run_id(&1) in newest_ids)) + jobless = Enum.reject(runs, &(&1.id in job_run_ids)) acc = %{passed: [], pending: [], missing: [], failed: []} judged = required |> lookup_names() |> Enum.reduce(acc, fn name, acc -> - case check_state(name, jobs) do + case check_state(name, counted, jobless) do {:failed, conclusion} -> Map.update!(acc, :failed, &[{name, conclusion} | &1]) state -> Map.update!(acc, state, &[name | &1]) end @@ -100,36 +106,51 @@ defmodule Loopctl.Delivery.CiEvidence do Map.put(judged, :local_gate, local_gate_state(statuses)) end - defp check_state(name, jobs) do + # Every job carrying the name, in each workflow's newest run. A newest run with NO jobs yet + # that is still running may be about to create one, so it holds the name pending (#910 round + # 3, finding 3). One that ENDED with no jobs at all (`startup_failure`, an invalid workflow) + # fails the name when no job carries it — otherwise it read as missing and waited out the CI + # limit before anyone was told CI never ran (finding 4). + defp check_state(name, jobs, jobless) do states = - jobs - |> newest_run_per_workflow() - |> Enum.filter(&(&1.name == name)) - |> Enum.map(&job_state/1) + for(%{name: ^name} = job <- jobs, do: job_state(job)) ++ + for %{status: status} <- jobless, status != "completed", do: :pending + dead = for %{status: "completed", conclusion: conclusion} <- jobless, do: conclusion || "none" + + combine(states, dead) + end + + defp combine([], [conclusion | _]), do: {:failed, "run_" <> conclusion} + defp combine([], []), do: :missing + + defp combine(states, _dead) do cond do - states == [] -> :missing failed = Enum.find(states, &match?({:failed, _}, &1)) -> failed :pending in states -> :pending true -> :passed end end - # Per workflow file, the jobs of its NEWEST run only: a later push run of the commit - # supersedes an earlier one. Within that run EVERY job counts — matrix legs and any jobs - # that share a name are separate jobs, and one green leg must never hide a red one (#910 - # round 2, finding 1). The jobs read is `filter=latest`, so each is its latest attempt. - defp newest_run_per_workflow(jobs) do - newest = - jobs - |> Enum.group_by(&Map.get(&1, :workflow), &(Map.get(&1, :run_id) || 0)) - |> Map.new(fn {workflow, run_ids} -> {workflow, Enum.max(run_ids)} end) - - Enum.filter(jobs, fn job -> - (Map.get(job, :run_id) || 0) == Map.fetch!(newest, Map.get(job, :workflow)) + # The newest run of each workflow file. The runs come from the adapter already reduced to + # that (`GitHubPullRequestSource`); a caller that names no runs gets them derived from the + # jobs, which is all an older caller carried. + defp newest_runs(runs, _jobs) when is_list(runs) do + runs + |> Enum.group_by(&Map.get(&1, :workflow)) + |> Enum.map(fn {_workflow, same} -> Enum.max_by(same, & &1.id) end) + end + + defp newest_runs(nil, jobs) do + jobs + |> Enum.group_by(&Map.get(&1, :workflow), &run_id/1) + |> Enum.map(fn {workflow, ids} -> + %{id: Enum.max(ids), workflow: workflow, status: "completed", conclusion: nil} end) end + defp run_id(job), do: Map.get(job, :run_id) || 0 + defp job_state(%{status: "completed", conclusion: "success"}), do: :passed defp job_state(%{status: "completed", conclusion: conclusion}), diff --git a/lib/loopctl/delivery/github_pull_request_source.ex b/lib/loopctl/delivery/github_pull_request_source.ex index 472f1ce1..bdc4d382 100644 --- a/lib/loopctl/delivery/github_pull_request_source.ex +++ b/lib/loopctl/delivery/github_pull_request_source.ex @@ -225,7 +225,8 @@ defmodule Loopctl.Delivery.GitHubPullRequestSource do {:ok, branch} <- ref(branch), {:ok, runs} <- push_runs(repo, sha, branch), {:ok, jobs} <- jobs_of(repo, runs) do - {:ok, %{jobs: jobs, statuses: commit_statuses(repo, sha)}} + {:ok, + %{runs: Enum.map(runs, &run_fact/1), jobs: jobs, statuses: commit_statuses(repo, sha)}} end end @@ -244,13 +245,34 @@ defmodule Loopctl.Delivery.GitHubPullRequestSource do with {:ok, body} <- get(repo, "/actions/runs?" <> query), {:ok, runs} <- workflow_runs(body) do {:ok, - Enum.filter(runs, fn run -> + runs + |> Enum.filter(fn run -> run["head_sha"] == sha and run["head_branch"] == branch and run["event"] == "push" - end)} + end) + |> newest_per_workflow()} |> bounded_runs() end end + # A commit pushed more than once has a run per push per workflow; only each workflow's + # NEWEST run is judged (`Loopctl.Delivery.CiEvidence`), so only it is read, and the bound + # counts workflows rather than pushes (#910 round 3, finding 2). + defp newest_per_workflow(runs) do + runs + |> Enum.group_by(& &1["path"]) + |> Enum.map(fn {_path, same} -> Enum.max_by(same, & &1["id"]) end) + |> Enum.sort_by(& &1["id"]) + end + + defp run_fact(run) do + %{ + id: run["id"], + workflow: run["path"], + status: run["status"], + conclusion: run["conclusion"] + } + end + # One jobs read per run: a push that triggered more workflows than this is refused rather # than costing an unbounded number of calls on every poll. defp bounded_runs({:ok, runs}) when length(runs) > @max_workflow_runs, diff --git a/lib/loopctl/delivery/merge_precondition.ex b/lib/loopctl/delivery/merge_precondition.ex index 4d800b72..342a5f27 100644 --- a/lib/loopctl/delivery/merge_precondition.ex +++ b/lib/loopctl/delivery/merge_precondition.ex @@ -572,14 +572,21 @@ defmodule Loopctl.Delivery.MergePrecondition do end end - # AC-45.6.1: the CI evidence the verdict read is copied onto the checkpoint it was read - # for, whatever the decision, so the thread shows why a checkpoint did or did not merge. - # An allow has ALREADY copied it (`record_allow/4`, where a copy that does not land is a - # refusal); for any other decision a copy that does not land is logged and changes nothing, + # AC-45.6.1: the CI evidence a DECISION rested on is copied onto the checkpoint it was read + # for, so the thread shows why a checkpoint did or did not merge. An allow has ALREADY + # copied it (`record_allow/4`, where a copy that does not land is not an allow); for a + # refusal a copy that does not land is logged and changes nothing, # because the decision it would explain was not an authorisation. defp note_ci_evidence(%Verdict{decision: :allow} = verdict, _tenant_id, _story_id), do: verdict + # Only a DECISION is copied (#910 round 3, finding 5): a CI wait, a moved head or an adopted + # merge explains nothing about why a checkpoint did or did not merge, and copying every wait + # rewrote the checkpoint on every poll for up to a day. + defp note_ci_evidence(%Verdict{decision: decision} = verdict, _tenant_id, _story_id) + when decision != :refuse, + do: verdict + defp note_ci_evidence(%Verdict{} = verdict, tenant_id, story_id) do case copy_ci_evidence(tenant_id, story_id, verdict) do # A newer read already stands, which is the record the thread should show. @@ -756,6 +763,11 @@ defmodule Loopctl.Delivery.MergePrecondition do (`ci_wait_limit_seconds/0`), because polls are the wrong unit for it: a pipeline waiting on `needs:` outlasts a few polls legitimately. A wait that ALSO carries a transient forge fault counts, as that fault always has. + + A pure CI wait also CLEARS the count, because the forge answered. Faults that alternate with + answered waits therefore never reach the bound — and do not need to: every answered poll is + judged against `ci_wait_limit_seconds/0`, so the wait still ends at that limit on the first + poll the forge answers after it. """ @spec counts_toward_unevaluated_bound?(Verdict.t()) :: boolean() def counts_toward_unevaluated_bound?(%Verdict{reasons: reasons}) do @@ -1420,10 +1432,22 @@ defmodule Loopctl.Delivery.MergePrecondition do # the same facts. skip? = moved_reasons(facts) != [] + # The three forge reads are independent, so they run CONCURRENTLY: the wait is the slowest + # read's, not their sum (#910 round 3, finding 6). Each is bounded by the adapter's own + # timeouts; the order of the results is fixed. + [head_files, base_files, ci_evidence] = + [ + fn -> repo_files(repo, pull_request, :head_sha, skip?) end, + fn -> repo_files(repo, pull_request, :merge_base_sha, skip?) end, + fn -> ci_evidence(facts, pull_request, skip?) end + ] + |> Task.async_stream(& &1.(), timeout: :infinity) + |> Enum.map(fn {:ok, fact} -> fact end) + Map.merge(facts, %{ - head_files: repo_files(repo, pull_request, :head_sha, skip?), - base_files: repo_files(repo, pull_request, :merge_base_sha, skip?), - ci_evidence: ci_evidence(facts, pull_request, skip?), + head_files: head_files, + base_files: base_files, + ci_evidence: ci_evidence, # When the story entered `ci`, what a CI wait is measured from — read on exactly the path # that reads CI, so a lock wait on it can never mask a merged or moved head's decision # (#910 round 2, finding 2). @@ -1467,8 +1491,12 @@ defmodule Loopctl.Delivery.MergePrecondition do with {:ok, repo} <- facts.repo, {:ok, %{commit_sha: sha}} <- facts.checkpoint, [_ | _] <- names do + # Stamped BEFORE the read: evidence is ordered by when it was read FROM, so a slow read + # that began earlier can never pass for the newer one (#910 round 3, finding 1). + started = DateTime.utc_now() + case source().check_evidence(repo, sha, branch) do - {:ok, evidence} -> {:ok, %{evidence: evidence, sha: sha, read_at: DateTime.utc_now()}} + {:ok, evidence} -> {:ok, %{evidence: evidence, sha: sha, read_at: started}} {:error, _reason} = error -> error end else diff --git a/mcp-server/lib/intake-sources.js b/mcp-server/lib/intake-sources.js index dc1a0f32..dfbb9a0a 100644 --- a/mcp-server/lib/intake-sources.js +++ b/mcp-server/lib/intake-sources.js @@ -168,14 +168,19 @@ export function modeRefusal(mode) { * stored mode this client cannot see, so only the shape is judged here. */ export function requiredChecksRefusal(checks) { + // The same shape the server holds (`Source.validate_required_checks/1`): non-blank, no + // surrounding whitespace (a job name never carries it, so "test " could never match), and + // distinct. Bounds on count and length stay the server's alone. const ok = Array.isArray(checks) && - checks.every((name) => typeof name === "string" && name.trim() !== ""); + checks.every((name) => typeof name === "string" && name !== "" && name.trim() === name) && + new Set(checks).size === checks.length; if (!ok) { return refuse( - "`required_checks` must be a list of GitHub Actions job names, as they appear " + - "on the commit (for example [\"test\", \"lint\"]).", + "`required_checks` must be a list of distinct GitHub Actions job names, as they " + + "appear on the commit, with no surrounding whitespace (for example " + + "[\"test\", \"lint\"]).", ); } diff --git a/mcp-server/test/intake_source_tools.test.js b/mcp-server/test/intake_source_tools.test.js index 88fc4fc2..03ed75f6 100644 --- a/mcp-server/test/intake_source_tools.test.js +++ b/mcp-server/test/intake_source_tools.test.js @@ -376,6 +376,8 @@ describe("intake_source_enroll", () => { ["test", /must be a list/], [[""], /must be a list/], [[7], /must be a list/], + [["test "], /must be a list/], + [["test", "test"], /must be a list/], [["test", "local-gate"], /local-gate` can never be a required check/], ].entries()) { const { calls, apiCall } = fakeApi(created()); diff --git a/test/loopctl/delivery/ci_evidence_test.exs b/test/loopctl/delivery/ci_evidence_test.exs index 7bc06f9c..94e9d7df 100644 --- a/test/loopctl/delivery/ci_evidence_test.exs +++ b/test/loopctl/delivery/ci_evidence_test.exs @@ -85,6 +85,39 @@ defmodule Loopctl.Delivery.CiEvidenceTest do judge(["test"], [%{leg_a | conclusion: "success"}, leg_b]) end + # #910 round 3, findings 3 and 4: the newest run of a workflow is taken from the RUNS, so a + # re-run with no jobs yet holds the name pending, and a run that died before creating any + # job fails a name no job carries. + test "a newest run with no jobs yet holds the name pending, over an older run's failure" do + old_fail = job("test", "completed", "failure", %{run_id: 10}) + + runs = [ + %{id: 10, workflow: "ci.yml", status: "completed", conclusion: "failure"}, + %{id: 11, workflow: "ci.yml", status: "queued", conclusion: nil} + ] + + assert %{pending: ["test"], failed: []} = + CiEvidence.judge(["test"], %{runs: runs, jobs: [old_fail], statuses: []}) + end + + test "a newest run that ended with no jobs fails a name no job carries" do + runs = [%{id: 12, workflow: "ci.yml", status: "completed", conclusion: "startup_failure"}] + + assert %{failed: [{"test", "run_startup_failure"}]} = + CiEvidence.judge(["test"], %{runs: runs, jobs: [], statuses: []}) + + # A completed jobless run of ANOTHER workflow does not fail a name some job carries. + green = job("test", "completed", "success", %{run_id: 13, workflow: "ci.yml"}) + + runs = [ + %{id: 13, workflow: "ci.yml", status: "completed", conclusion: "success"}, + %{id: 14, workflow: "lint.yml", status: "completed", conclusion: "startup_failure"} + ] + + assert %{passed: ["test"]} = + CiEvidence.judge(["test"], %{runs: runs, jobs: [green], statuses: []}) + end + test "each required name is judged on its own" do jobs = [ job("test", "completed", "success", %{id: 1}), diff --git a/test/loopctl/delivery/github_pull_request_source_test.exs b/test/loopctl/delivery/github_pull_request_source_test.exs index 6219b300..55af89aa 100644 --- a/test/loopctl/delivery/github_pull_request_source_test.exs +++ b/test/loopctl/delivery/github_pull_request_source_test.exs @@ -864,8 +864,36 @@ defmodule Loopctl.Delivery.GitHubPullRequestSourceTest do Source.check_evidence(@repo, @head, @branch) end + # #910 round 3, finding 2: a commit pushed more than once has a run per push per workflow; + # only each workflow's newest is read, and the bound counts workflows, not pushes. + test "only each workflow's newest run is read, and the bound counts workflows" do + runs = + for push <- 0..3, {wf, n} <- [{"a", 1}, {"b", 2}, {"c", 3}] do + gh_run(push * 10 + n, %{"path" => ".github/workflows/#{wf}.yml"}) + end + + newest = + runs + |> Enum.group_by(& &1["path"]) + |> Enum.map(fn {_, rs} -> Enum.max_by(rs, & &1["id"])["id"] end) + + stub_ci(%{ + runs: %{"total_count" => length(runs), "workflow_runs" => runs}, + jobs: + Map.new( + newest, + &{&1, %{"total_count" => 1, "jobs" => [gh_job(&1 * 100, "test", "success")]}} + ), + statuses: %{"total_count" => 0, "statuses" => []} + }) + + assert {:ok, %{runs: read_runs, jobs: jobs}} = Source.check_evidence(@repo, @head, @branch) + assert Enum.sort(Enum.map(read_runs, & &1.id)) == Enum.sort(newest) + assert length(jobs) == 3 + end + test "more workflow runs than the bound is refused rather than read" do - runs = for id <- 1..11, do: gh_run(id) + runs = for id <- 1..11, do: gh_run(id, %{"path" => ".github/workflows/w#{id}.yml"}) stub_ci(%{ runs: %{"total_count" => 11, "workflow_runs" => runs}, diff --git a/test/loopctl/delivery/merge_precondition_integration_test.exs b/test/loopctl/delivery/merge_precondition_integration_test.exs index 445e5f50..47152018 100644 --- a/test/loopctl/delivery/merge_precondition_integration_test.exs +++ b/test/loopctl/delivery/merge_precondition_integration_test.exs @@ -536,6 +536,31 @@ defmodule Loopctl.Delivery.MergePreconditionIntegrationTest do assert Stages.get(ctx.tenant_id, ctx.story_id).merge_gate_allowed_sha == nil end + # #910 round 3, finding 1: read_at is stamped when the read STARTS. + test "the evidence's read_at is when the read began, not when it returned", ctx do + stub_thread(ctx) + test_pid = self() + + Mox.stub(MockPullRequestSource, :check_evidence, fn @repo, _sha, _branch -> + send(test_pid, {:reading_at, DateTime.utc_now()}) + Process.sleep(20) + {:ok, %{jobs: [ci_run("test", "success")], statuses: []}} + end) + + assert {:ok, %Verdict{decision: :allow} = verdict} = enforce(ctx) + assert_received {:reading_at, reading_at} + {:ok, read_at, _} = DateTime.from_iso8601(verdict.ci_evidence["read_at"]) + assert DateTime.compare(read_at, reading_at) != :gt + end + + # #910 round 3, finding 5: a CI wait decides nothing and copies nothing. + test "a CI wait copies no evidence onto the checkpoint", ctx do + stub_thread(ctx, ci: %{jobs: [ci_run("test", nil, "queued")], statuses: []}) + + assert {:ok, %Verdict{decision: :unevaluated}} = enforce(ctx) + refute Map.has_key?(checkpoint_evidence(ctx), "ci") + end + # AC-45.6.1: whatever the decision, the evidence read is copied onto the checkpoint. test "the evidence is copied onto the checkpoint on an allow and on a refusal", ctx do stub_thread(ctx)