Verification judges a commit's Actions workflow runs by outcome (#913) by mkreyman · Pull Request #915 · mkreyman/loopctl

mkreyman · 2026-09-28T00:19:23Z

Closes #913. Supersedes #914, which reached its third review round still finding material defects and is rewritten here per the review-round rule, the same way #909 became #910.

Why

Story verification read commits/:sha/check-runs, which needs the Checks permission. GitHub grants Checks to Apps only, so the fine-grained GITHUB_TOKEN on production cannot hold it. Every verification CI lookup on a private repository returned 403. This was measured on infra and home_care_billing.

What changed from #914, and why

#914's rounds kept adding machinery to decide which of a commit's workflows are CI: tag-push detection, waiting and cancelled precedence, and a ref allowlist on pushed branch names. Each round found the edges of the previous one. Nothing in a workflow run says whether it is CI, so this version drops the classification and judges by outcome, in this order:

A failure, timed_out or startup_failure run fails the commit, and the failing run's URL is recorded. This holds even when another run is still queued.
Otherwise, a queued, in-progress, requested or pending run means wait.
Otherwise, a successful run passes the commit.
Anything else is no CI evidence, and the worker falls back to local re-execution. That covers no runs at all, and runs that are cancelled, skipped, neutral, action_required, stale or waiting.

A run that reached no result never outweighs one that did.

The pieces

Shared reader. GitHubPullRequestSource.commit_ci_runs/2 reads actions/runs?head_sha=&event= once per event (push and pull_request). It uses the merge gate's own shape check and truncation refusal, and keeps the newest uncancelled run per workflow, event and branch. newest_per_workflow and this function share one newest_per rule. auth_headers/1 moves into the source, so the two modules no longer call each other.
Verdict. GitHubActions keeps only the verdict. It no longer makes an outbound call, so its egress allowlist entry and its row in docs/egress-guard.md are removed. The repo URL parser now keeps dots in repository names.
Worker. Three changes:
- It casts the story and tenant ids in its schemaless query. Before this, every verification run that had a commit crashed before reaching CI.
- It snoozes through a rate limit, using the forge's delay or a 60 s floor.
- It records url, plus ci_unavailable_reason as a short code that never contains the repo URL.
Docs. In FLY_SECRETS.md the GITHUB_TOKEN row now asks for actions read, not checks. There is a CHANGELOG entry.

Decisions a reviewer may question

The worker's reads carry the delivery reader's user agent, because the reader is shared on purpose (Verification reads a commit's Actions workflow runs, not its check runs #914 round 1, finding 7). A separate agent string would need a second request path.
A failed release or deploy workflow on the verified commit now fails verification. The alternative was the workflow classification that Verification reads a commit's Actions workflow runs, not its check runs #914 could not make sound. Story commits are pre-squash branch commits, and releases run on master's squash commits, so in this fleet the two rarely share a SHA.

Round 1 (`410c2fa`)

Confused deputy, fixed. CI is now read from the project's intake source repository only, never from its tenant-editable repo_url, because the read carries the operator's token. This path was dormant before this PR, since every run crashed on the uncast UUID first. Fixing that crash is what would have made the path live.
A waiting run no longer hides an older failure. A newer run that reached no result (for example one waiting for approval) no longer hides an older failure of the same workflow.
Waits are bounded. A run still waiting on CI 24 hours after it was created ends with ci_wait_exhausted.
Local errors are recorded as codes rather than raw error terms.
Reads run concurrently, and push_runs reuses commit_runs.
Still a decision, not a defect: a failed run of any workflow fails the commit (round 1 finding 4). A verification run is observational and never sets verified_status, so a false fail costs a look, while a false pass is what misleads. This is stated in the code where the rule is decided.

Round 2 (`00c98ee`)

Short SHAs: an abbreviated commit SHA is resolved to the full one before reading runs. Without this, its runs were never found.
Grace period: a commit with no workflow runs yet gets ten minutes for GitHub to create them.
One clock, with backoff: the wait budget is now the configurable run-age window. Polling backs off with the run's age, from 60 s up to 15 min.
Docs: the moduledoc and CHANGELOG are corrected, including the real signal a missing actions: read produces.

Needs Mark: the operator token reads tenant-named repositories

Round 2 finding 4 is real, and it is wider than this PR. An intake source's repo_full_name is tenant-supplied too: enrollment never proves the tenant controls the repository. The merge gate already reads diffs, file trees, CI and deployments of any enrolled repository with the operator's single GITHUB_TOKEN, and verification now does the same, no more. So any tenant that can enroll an intake source can read, through loopctl, whatever that token can see.

What fixes it: per-tenant GitHub credentials. That means each tenant installs the loopctl GitHub App on its own repositories. loopctl then reads with THAT installation's token, and enrollment is refused for a repository the tenant's installation does not cover. This is a separate epic: the credential model, enrollment, the merge gate, verification and the executor all move together.
What it blocks: safely admitting any tenant other than you. With a single tenant there is no one to disclose to.
Recommendation: merge this PR as it stands, since it does not widen the exposure, and file the per-tenant credential epic before a second tenant signs up.

Checks

Commit gate: 11966 tests, 0 failures.
Mutations with bin/mutate.sh, every one exit 0: 23 on the rewrite, 28 on round 1, 35 on round 2. Each commit message lists its set.

The review gate is running.

…ns (#913) GitHub offers the Checks permission to Apps only, so the fine-grained GITHUB_TOKEN now on production cannot hold it, and every story-verification CI lookup on a private repository 403'd (measured on infra and home_care_billing). get_status now reads actions/runs?head_sha=, which needs actions: read. The summary had two latent defects, fixed with it: - an empty run list read as SUCCESS (Enum.all? of nothing), so a commit whose CI had not started was verified; - a cancelled, timed-out or skipped run read as in_progress for ever. Only the newest run of each workflow per event counts, so a green re-run supersedes a failure. A Req.Test seam (config :verification_github_req_plug) lets the endpoint, the head_sha parameter and the response key be tested. Mutations, each run with bin/mutate.sh against github_actions_test.exs, all exit 0: empty-list guard, not-completed guard, newest-run dedupe, per-event dedupe, skipped-as-pass, check-runs endpoint, dropped head_sha, check_runs body key. FLY_SECRETS.md: GITHUB_TOKEN needs actions (not checks) read.

…-run reader Most findings had one cause: verification got its own, weaker reader of a commit's workflow runs instead of the merge gate's hardened one. GitHubPullRequestSource.commit_ci_runs/2 now reads them with the same request, shape check and truncation refusal as check_evidence/3, keeps only push and pull_request runs of the exact commit, and returns the newest run per workflow, event and branch (a failure on any branch the commit was pushed to counts). GitHubActions keeps only the verdict, and its request plumbing, Req.Test seam and egress allowlist entry are gone. Verdict: no runs, or only skipped/neutral runs, is no evidence (the worker falls back to local re-execution) instead of an unbounded wait; a cancelled run is no evidence rather than a failure; any other non-success fails and carries that run's URL. Docs: the FLY_SECRETS GITHUB_TOKEN row no longer says actions read is thread-only; a CHANGELOG entry tells operators the token needs actions read; egress-guard.md and the allowlist describe the shared reader. Mutations, each with bin/mutate.sh, all exit 0: empty guard, not-completed guard, skipped and neutral exclusion, cancelled as no evidence, cancelled not counted as failure, failure URL, event filter, sha filter, per-branch grouping, newest-by-id, truncation refusal, and the sha passed through get_status.

…d the worker keeps the reason Round 1's shared reader asked for every run of the commit and filtered by event only after the truncation check, so a busy schedule history could make a commit unverifiable; it also counted tag pushes (release workflows) as CI and let a newer cancelled run hide an older success. commit_ci_runs now reads push and pull_request separately, drops runs of a pushed TAG (one ref read per distinct pushed name, bounded), and keeps per workflow, event and branch the newest run that was not cancelled. newest_per_workflow and the verification path share one newest_per rule. The verdict treats a run waiting on an environment approval as no evidence rather than an endless wait. The worker snoozes out a rate limit instead of cloning, records why CI gave no verdict (ci_unavailable_reason) and the failing run's url. It also casts the story and tenant ids in its schemaless query: the uncast string crashed every run that had a commit before it reached CI. The repo URL parser keeps dots in a repository name. Docs: the module's call list and ceiling, the allowlist justification's attribution, and the CHANGELOG entry. Mutations, each with bin/mutate.sh, all exit 0 (23): empty guard, not-completed guard, waiting not in progress, waiting as no evidence, skipped/neutral exclusion, cancelled as no evidence, cancelled not a failure, failure URL, dotted repo name, sha passed through, sha filter, local event filter, event query parameter, per-branch grouping, newest by id, prefer uncancelled, tag drop, tag-drop wiring, truncation refusal, worker URL, rate-limit snooze, recorded reason, UUID cast.

…w classification #914's three review rounds each added machinery to decide which of a commit's workflows are CI (tag-push detection, waiting and cancelled precedence, a ref-name allowlist on pushed branches), and each round found the edges of the last. Nothing in a workflow run says whether it is CI, so this drops the classification and judges by outcome alone: 1. a failed, timed-out or unstartable run fails the commit (with its URL), even beside a run still queued; 2. otherwise a queued or running run is a wait; 3. otherwise a successful run passes; 4. anything else is no CI evidence: no runs, or only cancelled, skipped, neutral, action_required, stale or waiting runs. A run that reached no result never outweighs one that did, so a deploy waiting on an approval or cancelled by a newer merge leaves a green CI run green. commit_ci_runs loses the tag-ref reads (and with them the invalid_ref halt and the tag-exists-now judgement): two reads, one per event. auth_headers moves into GitHubPullRequestSource, which ends the two modules calling each other. The worker snoozes a rate limit for the forge's delay or a 60s floor when none parses, and records the no-evidence reason as a short code (github_api_error:403, no_ci_evidence) instead of inspect(reason), whose terms can carry a credentialed repo_url. The CHANGELOG entry states the rule as it is. Mutations, each with bin/mutate.sh, all exit 0 (23): failed conclusion set, failure before wait, running set, waiting not a wait, empty not a pass, success clause, failure URL, dotted repo name, sha passed through, sha filter, local event filter, event query parameter, per-branch grouping, newest by id, prefer uncancelled, truncation refusal, blank token, worker URL, rate-limit snooze, rate-limit floor, reason code, recorded reason, UUID cast.

…udget, and results before non-results The worker read CI from the project's repo_url, which a tenant can set to any repository, with the operator's GITHUB_TOKEN: a confused-deputy read of another repository's CI. It now resolves the repository from the project's intake source (Intake.source_for_project/2, the binding the merge gate uses); no source means no CI read, recorded as no_intake_source. get_status takes owner/name, so the URL parser (and its port, case and fragment misreads) is gone. A newer run that reached no result (waiting, action_required, skipped) hid an older failure of the same workflow. commit_ci_runs takes the preference from the caller, and verification prefers runs with a result or still running. Snoozes had no bound, since start_run resets started_at on every perform: a run still waiting a day after it was created now ends as error, ci_wait_exhausted. The local fallback records its error as a code too. The two event reads run concurrently, and push_runs reuses commit_runs instead of repeating it. The rule's bias toward failure is stated where it is decided: a verification run never sets verified_status, so a false fail costs a look and a false pass misleads. FLY_SECRETS no longer names github_api_error as the rate-limit symptom. Mutations, each with bin/mutate.sh, all exit 0 (28): failed set, failure before wait, running set, waiting not a wait, empty not a pass, success clause, failure URL, sha passed through, result preference, sha filter, local event filter, event query parameter, both events read, per-branch grouping, newest by id, truncation refusal, blank token, worker URL, rate-limit snooze, rate-limit floor, reason code, recorded reason, UUID cast, intake repository used, intake source required, wait budget, local error as a code, no repo_url.

…eated, and one bounded, backed-off wait - An abbreviated commit SHA (VerificationRun allows 7-64 hex) never matched the full head_sha GitHub filters and returns, so its runs were never found. commit_ci_runs now resolves it through GET /commits/:sha first. - An empty run list was final at once, though GitHub creates a push's runs seconds after the push. It now has its own code, no_workflow_runs, and the worker waits ten minutes from the run's creation before treating it as a repository without Actions CI. - The wait budget was a second hard-coded 24h clock on inserted_at beside the configurable run-age window. It is now that window. Polls back off with age (a tenth of it, 60s to 15 minutes), since every poll spends reads on the token the merge gate uses. - The moduledoc's claims that a waiting run is never ended and that CI-unavailable marks manual review were false; both are rewritten. The CHANGELOG names the real signal of a missing actions: read (github_api_error:403/404) and says plainly that a project with no intake source gets no CI read. Mutations, each with bin/mutate.sh, all exit 0 (35): the round-1 set plus empty-list code, full-SHA resolution and its length guard, wait budget on the run-age window, backoff, the 15-minute cap, the grace, and the grace bound.

mkreyman · 2026-09-28T00:55:36Z

Held after round 3. Not merging.

Round 3, the ceiling, still found material defects, and so did every round before it:

A transient 5xx or timeout ends a run for good, while a rate limit is waited out.
One small successful workflow passes a commit whose real CI has not run.
An abbreviated SHA not yet on GitHub skips the grace.
The local fallback still clones the tenant-editable repo_url.
The worker matches GitHub-specific error terms through a generic behaviour.
The SHA is re-resolved on every poll.
The grace is measured from creation rather than from the start.
Counts render like HTTP statuses.
The docstring names the wrong arity.
The auth_headers tests and the user agent sit in the wrong module.

That is evidence about the source, not a reason for round 4. The source problem: #913 asked for one endpoint change, but fixing the UUID crash (every verification run with a commit crashed before reaching CI) woke up a verification subsystem that has never produced a verdict in production. Its semantics were never designed, so each round has been designing them one finding at a time:

which runs count as the commit's CI;
which repository is read, and with whose credential;
when a missing, pending or unreachable answer means wait, and when it means give up.

Proposed shape for the rewrite, as a spec'd story rather than this PR:

Reuse the merge gate's judgement. Judge verification with the merge gate's CiEvidence against the intake source's required_checks: named jobs, not "any workflow". That settles which runs count.
One transient-versus-final policy. Adopt the merge gate's unevaluated handling for transient forge failures.
Credentials. Read with the credential the per-tenant decision below settles. The local fallback clones the same repository CI is read from.

It is blocked on one decision only Mark can make: the credential model in "Needs Mark" above. Verification has produced no verdicts all along, so holding this changes nothing in production.

…ent whole The first draft fed CiEvidence.judge/2 a branch-free read, which drops the premise judge/2 is sound under (push runs of one branch at one SHA) and skipped the ci_definition_changed refusal it depends on. The story now names the story's own branch and check_evidence/3, the refusal, the :missing and jobless-run outcomes, required_checks as a pr-mode opt-in, the merge gate's transient/permanent split and consecutive-fault bound, when the local fallback may run and how it clones, one-read SHA resolution through /commits/:ref, an operator-token allowlist until the per-tenant credential decision, ambiguous_intake_source as its own code, separate runs per test outcome, and the #915 edge list as an acceptance criterion rather than a note.

…e rules (#916) * US-26.4.6: story verification judges CI with the merge gate's evidence rules Specs the rewrite #915 was held for after its third review round. Story verification has never produced a CI verdict in production, and its rules (which runs count, which repository and credential, when to wait) were being designed one review finding at a time. This story settles them by reuse: the repository from the intake source only, the verdict from CiEvidence over the source's required_checks, transient failures waited out as the merge gate does, and one credential seam so the open per-tenant credential decision changes nothing else. * Review round 1 on #916: the story reuses the merge gate's trust argument whole The first draft fed CiEvidence.judge/2 a branch-free read, which drops the premise judge/2 is sound under (push runs of one branch at one SHA) and skipped the ci_definition_changed refusal it depends on. The story now names the story's own branch and check_evidence/3, the refusal, the :missing and jobless-run outcomes, required_checks as a pr-mode opt-in, the merge gate's transient/permanent split and consecutive-fault bound, when the local fallback may run and how it clones, one-read SHA resolution through /commits/:ref, an operator-token allowlist until the per-tenant credential decision, ambiguous_intake_source as its own code, separate runs per test outcome, and the #915 edge list as an acceptance criterion rather than a note.

mkreyman · 2026-09-28T16:55:25Z

Superseded by #931. Round 3 here, the ceiling, still found correctness bugs, all in machinery this PR added around the fix (polling, grace, repository switch). #931 makes only the change #913 asked for, in the adapter.

…thout evidence, bound the wait - Only runs the commit's own push or pull_request triggered count; a deploy, schedule or dispatch run on the same commit neither passes nor fails it. - Only success passes. skipped, neutral and cancelled are no evidence either way (a skipped run ran no tests; cancel-in-progress cancels a superseded commit's run). No counting run is {:error, :no_workflow_runs}, not in progress. - The newest run per workflow is the highest id. A body of another shape is unreadable_workflow_runs, not a crash. Req's transient retry is back. - The worker logs the reason a lookup gave no verdict (a token without actions: read would otherwise be a silent local run), and ends a run whose CI is still unfinished past verification_max_run_age_seconds as error ci_unfinished instead of snoozing for ever. The clock is inserted_at: every snooze restarts the run and resets started_at. - Found while testing that: the worker's project lookup passed string UUIDs to a schemaless query, which Postgrex refused, so no started run ever reached the CI read. The ids are now cast. - The egress allowlist justification, docs/egress-guard.md and the GITHUB_TOKEN row describe the Actions read; the row no longer implies a pr-mode deployment can skip actions: read. Declined: reusing Loopctl.Delivery.CiEvidence. It judges named required checks against a thread's intake source and branch; verification holds only a repo URL and a sha, and wiring it to the intake source is what grew #914/#915. Mutations (bin/mutate.sh, all exit 0): endpoint back to check-runs; event filter removed; no-evidence drop removed; cancelled counted; empty read as in progress; oldest run chosen; truncation never refusing; shape check removed; the UUID cast removed (all three worker CI tests fail as on master); the wait bound removed; the reason log removed.

mkreyman added 4 commits September 27, 2026 17:26

mkreyman mentioned this pull request Sep 28, 2026

Verification reads a commit's Actions workflow runs, not its check runs #914

Closed

mkreyman added 2 commits September 27, 2026 18:32

mkreyman mentioned this pull request Sep 28, 2026

US-26.4.6: story verification judges CI with the merge gate's evidence rules #916

Merged

mkreyman mentioned this pull request Sep 28, 2026

Story verification reads CI from Actions workflow runs, not check runs (#913) #931

Closed

mkreyman closed this Sep 28, 2026

mkreyman deleted the fix/913-verification-ci-verdict branch September 28, 2026 17:47

mkreyman mentioned this pull request Sep 28, 2026

US-26.4.6: story verification reads CI from the intake source, per-tenant credential (fixes #913) #933

Merged

Navigation Menu

Verification judges a commit's Actions workflow runs by outcome (#913) - #915

Verification judges a commit's Actions workflow runs by outcome (#913)#915
mkreyman wants to merge 6 commits into
masterfrom
fix/913-verification-ci-verdict

mkreyman commented Sep 28, 2026 •

edited

Loading

Uh oh!

mkreyman commented Sep 28, 2026

Uh oh!

mkreyman commented Sep 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

mkreyman commented Sep 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why

What changed from #914, and why

The pieces

Decisions a reviewer may question

Round 1 (410c2fa)

Round 2 (00c98ee)

Needs Mark: the operator token reads tenant-named repositories

Checks

Uh oh!

mkreyman commented Sep 28, 2026

Uh oh!

mkreyman commented Sep 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

mkreyman commented Sep 28, 2026 •

edited

Loading

Round 1 (`410c2fa`)

Round 2 (`00c98ee`)