From 51a5fd3924ecd77496276164f72d6302d479f13f Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Tue, 28 Jul 2026 23:09:25 -0400 Subject: [PATCH 1/4] docs: claude lane contract close-out (WIP, unverified) Work-in-progress output from an in-flight worker that died with its session before reporting or being verified. Committed as-is to make it durable and diffable for a fresh-context verifier; not yet reviewed. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_013pLW2dybov9xvTFtx48Ueb --- CLAUDE.md | 8 ++-- README.md | 116 ++++++++++++++++++++++++++++++++++++++++-------------- 2 files changed, 91 insertions(+), 33 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 5379a12..d92438a 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -4,10 +4,12 @@ Guidance for working in this repo. The [README](README.md) covers what each building block does and the consumer contract; this file captures the rules a change must not violate. -## Security ground rules — `claude-review` reusable workflow +## Security ground rules — the Claude lane reusable workflows -The PR-review workflow runs an AI agent with an org credential on a **public** -repo. These rules are load-bearing; changing any of them needs explicit review. +`claude-review.yml`, `claude-security-review.yml`, and `claude-e2e-verify.yml` +each run an AI agent with an org credential on a **public** repo, and each +workflow's own header cites this section for the rationale. These rules are +load-bearing; changing any of them needs explicit review. - **`pull_request` only; never `pull_request_target` or `workflow_run` with secrets.** Those triggers run in a privileged context (base-repo secrets + diff --git a/README.md b/README.md index 0c3ee60..5579cf8 100644 --- a/README.md +++ b/README.md @@ -636,27 +636,33 @@ GitHub continues the normal weekly patching of each hosted image generation. repo, so the lane scopes itself to security-sensitive surfaces — but the caller must NOT express that scope with a workflow-level `on.pull_request.paths` filter, because a path miss leaves a required check Pending forever and wedges - every prose PR. Instead the caller triggers on all PR events and passes the - `paths` input (security-sensitive surfaces in Actions `paths:` syntax: - workflow files, permission/settings configs, hook and shell scripts, - auth/token-touching code, network-call sites); the workflow's `changes` job - evaluates it and a not-applicable PR yields a name-stable skipped - `security-review` check. A consumer's ruleset may make that EXECUTION check - required (check context ` / security-review`); the VERDICT stays - advisory. All inputs have public-safe defaults documented inline in the - workflow header (the authoritative list). Consume it per the [Claude + every prose PR. Instead the caller triggers on all PR events and supplies that + scope as a pattern list in Actions `paths:` syntax (workflow files, + permission/settings configs, hook and shell scripts, auth/token-touching code, + network-call sites); the workflow's `changes` job evaluates it and a + not-applicable PR yields a name-stable skipped `security-review` check. A + consumer's ruleset may make that EXECUTION check required (check context + ` / security-review`); the VERDICT stays advisory. + + **Where that pattern list lives** is the caller's choice between two inputs. + The conventional shape is `paths-file`, pointing at a repo-owned file + (`.github/claude-security-paths`) so each repo keeps its own + security-sensitive-surface list in its own tree; the inline `paths` input + takes the same content directly and, when non-empty, wins over the file. The + file is read from the PR's **base** branch, never the head, so a PR cannot + edit its content to skip its own security review — repointing the input is + still possible, but only as a visible caller diff, which is the pre-existing + trust boundary. An absent or unreadable file **fails open** (every PR + reviewed, with a warning), matching the `changes` job's fail-open discipline + throughout; both inputs empty means no filtering, so a consumer that passes + nothing is unaffected. All inputs have public-safe defaults documented inline + in the workflow header (the authoritative list). Consume it per the [Claude lanes — shared consumption contract](#claude-lanes--shared-consumption-contract) - below, triggering on all PR events (no workflow-level `paths:`) and passing - the `paths` input in the job: + below, triggering on all PR events (no workflow-level `paths:`): ```yaml with: - paths: | - .github/workflows/** - .github/actions/** - **/*.sh - **/*.ps1 - # plus the caller's own auth/token and network-call source paths + paths-file: .github/claude-security-paths ``` - `.github/workflows/claude-e2e-verify.yml` — Claude-powered end-to-end @@ -878,9 +884,9 @@ GitHub continues the normal weekly patching of each hosted image generation. `claude-review.yml`, `claude-security-review.yml`, and `claude-e2e-verify.yml` share one consumption shape. Each is **advisory**: it posts PR comments and never gates `ci-status`. (The advisory verdict is separate from execution -evidence: `claude-security-review.yml` additionally takes a `paths` input and -its name-stable `security-review` check may be made a required status check — -see its entry above.) Each is a whole-job concern (job `permissions:` plus a +evidence: `claude-security-review.yml` scopes itself to security-sensitive +paths, and its name-stable `security-review` check may be made a required +status check — see its entry above.) Each is a whole-job concern (job `permissions:` plus a `secrets:` interface), which is why each is a reusable workflow rather than a composite action — the caller owns the triggers and the permission grant, and the workflow owns the SHA-pinned `anthropics/claude-code-action` and the safe @@ -889,15 +895,7 @@ handling. Security rules live in [CLAUDE.md](CLAUDE.md). ```yaml on: pull_request: - # Trigger types are PER LANE — copy the canonical caller from the lane's - # own workflow header, not this line: - # claude-review: [opened, ready_for_review, reopened] - # (no `synchronize` — pushes do not re-trigger the code review; - # re-run the job for a fresh pass) - # claude-security-review: [opened, synchronize, ready_for_review, reopened] - # (its check certifies execution at the merge head) - # claude-e2e-verify: [opened, synchronize, ready_for_review, reopened] - types: [opened, synchronize, ready_for_review, reopened] + types: [] jobs: : permissions: @@ -913,7 +911,48 @@ The caller's job must grant those three permissions (a called workflow can only downgrade, not elevate); the `CLAUDE_CODE_OAUTH_TOKEN` org secret has visibility "all repositories", so every org repo receives it. Pass that one named secret explicitly rather than `secrets: inherit`, which forwards every -parent secret. Fork PRs receive no secrets by design and are not reviewed. +parent secret. Fork PRs receive no secrets by design and are not reviewed. On +both review lanes the caller can name actors whose comments are withheld from +the agent's context — prompt-injection hygiene, not a trigger gate. + +**Trigger cadence is per lane, deliberately.** `claude-review` runs on +`opened` / `ready_for_review` / `reopened` and **not** on `synchronize`: a push +does not re-trigger the code review, so re-run the job for a fresh pass. That +caps per-PR spend on active branches, and it is safe precisely because the +lane's verdict gates nothing. `claude-security-review` keeps `synchronize`, +because its check certifies that a security pass ran at the head being merged — +a review of an earlier head is not that evidence. `claude-e2e-verify` keeps it +too. Take each lane's canonical caller from its own workflow header. + +**Bounded retry.** Every lane makes at most **two** agent attempts — one +automatic retry, never a loop. The retry is deliberately narrow, because a +second attempt after the agent has already spoken duplicates its comments: it +fires only on **zero assistant turns** in the first attempt's execution file. +Nor does an **auth-class** failure retry — HTTP 401/402/403, or an +`authentication_error` / `billing_error` / `permission_error` in the error +payload — because the credential needs an operator and no retry can clear it. +The gate also honors the same guards the first attempt does, so a superseded +run (and, on the code-review lane, a capped one) never spends a retry. Between +the attempts the lane backs off `retry-delay-seconds` plus a 0–29 second +jitter, so lanes retrying against the same contended seat do not re-collide in +lockstep. What this buys is availability against the sporadic-429 class without +weakening the security lane's fail-closed execution claim. + +One divergence is worth knowing. The two review lanes demand **proof** of zero +turns: a missing or unparsable execution file is not proof — a hard kill can +lose the file after turns were already spent — so they do not retry on one. +`claude-e2e-verify` reads an unreadable file as recording no assistant turn and +does retry. It also sets no `track_progress` tracking comment, so it has no +orphan comment to clean up between attempts, which the review lanes do. + +**Review-count cap (code-review lane only).** `claude-review.yml` stops +reviewing a PR after `max-reviews-per-pr` successful reviews, capping spend on +long-lived PRs. The counter is a **visible** per-PR status comment upserted +after each review, which doubles as the human "was this reviewed" signal; +deleting it resets the count, which is fail-open by design. A capped run is a +name-stable skip, not a red check. Treat it as a soft cap: concurrent runs for +different heads read the counter before either writes it, so a burst can +briefly exceed it by the number of concurrent heads. **Kill-switches.** Every lane honors two Actions variables at job level: `CLAUDE_LANES_DISABLED` (all lanes) and a per-lane switch @@ -928,6 +967,23 @@ disabled NO lane reports security findings — REVIEW.md's code-review exclusion keys on the security workflow file existing, and the file remains — so re-enable promptly and treat the outage window as security-unreviewed. +All four organization variables carry **all-repositories** visibility. That is +a deliberate deviation from the org's selected-visibility convention for +Actions variables, not an oversight: a switch scoped to a selection is invisible +to every repo outside it, so flipping it during an incident would silently +no-op exactly where nobody is looking. A kill-switch is only worth having if it +reaches the whole fleet. Do not "correct" the visibility to selected. + +**Adoption.** Wiring these callers by hand is the fallback. The intended path +is the sync-managed caller components in +[`melodic-software/standards`](https://github.com/melodic-software/standards), +which distribute a canonical per-lane caller to every target repo and keep it +current through the ordinary sync PR. That work is in flight in +[standards#286](https://github.com/melodic-software/standards/pull/286) and not +yet landed — it is gated on the sync App gaining the `workflows` permission +that writing files under `.github/workflows/` requires. Until it lands, copy +the canonical caller from the lane's own workflow header. + ## Standalone gate checks — shared adoption contract `semantic-pr.yml`, `pr-issue-linkage.yml`, and `do-not-merge-gate.yml` are From 021c06cf92e5fd92ede65ea9bcb47bcfae5b4082 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Tue, 28 Jul 2026 23:21:36 -0400 Subject: [PATCH 2/4] docs: correct and complete the Claude lane contract close-out Reviews the unverified WIP commit against the shipped workflow source at c136b27 (v0.9.1) and fixes what the prose got wrong or left out. CLAUDE.md's security section heading was widened from claude-review alone to all three lanes, which left two bullets asserting rules a covered lane violates by design: - "No untrusted checkout before the action" is false for claude-e2e-verify, whose whole purpose is to build, serve, and browser-drive the PR head. Scoped the rule to the review lanes and made the e2e exception explicit, pointing at the header that records the persisted-credential residual and its bound. - "the review degrades to a warning" on fork PRs is true for claude-review and claude-e2e-verify but false for claude-security-review, which skips the job at job level so the fail-closed mapping cannot pin every fork PR red. Split the mechanism per lane and recorded that the resulting skip reads as success. Also swept the remaining bullets for singular-voice leftovers. README gains the fail-closed execution contract the new retry prose forward-referenced but never established: the security lane's check is a separate required context, its claim is that a pass RAN, and a legitimate non-run (fork, out-of-scope, skip-actor, kill-switch) is a name-stable skip a ruleset reads as success. Adds the draft-PR skip to the cadence paragraph, ties the review counter to SUCCESSFUL reviews, and rewrites the adoption pointer so it stops carrying in-flight PR state that drifts. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_013pLW2dybov9xvTFtx48Ueb --- CLAUDE.md | 35 ++++++++++++++++++++++--------- README.md | 62 +++++++++++++++++++++++++++++++++++++++---------------- 2 files changed, 69 insertions(+), 28 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index d92438a..d905e97 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -14,14 +14,19 @@ load-bearing; changing any of them needs explicit review. - **`pull_request` only; never `pull_request_target` or `workflow_run` with secrets.** Those triggers run in a privileged context (base-repo secrets + write token) over potentially untrusted fork code — the "pwn request" class, - i.e. token exfiltration. The workflow keeps a tripwire step that hard-fails on - those two events. Do not whitelist a single event (that would block a + i.e. token exfiltration. Each workflow keeps a tripwire step that hard-fails + on those two events. Do not whitelist a single event (that would block a consumer's legitimate `workflow_dispatch` / `schedule`); reject only the dangerous two. - **Fork PRs are intentionally not reviewed.** GitHub passes no secrets and a - read-only token to fork-triggered `pull_request` runs, so the review degrades - to a warning. That is the safety guarantee, not a bug — never "fix" it by - reaching for `pull_request_target`. + read-only token to fork-triggered `pull_request` runs, so the agent cannot + run. `claude-review` and `claude-e2e-verify` let it degrade to a warning; + `claude-security-review` skips the job at job level instead, because its + fail-closed mapping would otherwise pin every fork PR red for a cause no push + can fix — and because that skip reads as success, its required check does not + prove a fork PR was reviewed, so fork changes to security-sensitive surfaces + need a human. The no-secrets guarantee is the safety property, not a bug — + never "fix" it by reaching for `pull_request_target`. - **SHA-pin both layers.** Consumers pin this reusable workflow `@<40-char-sha>`; this workflow pins `anthropics/claude-code-action@` (mutable tags were the tj-actions/CVE-2025-30066 vector). Dependabot bumps the inner pin. @@ -29,7 +34,7 @@ load-bearing; changing any of them needs explicit review. the caller's `GITHUB_TOKEN` grant, so the consumer's caller job must grant exactly `contents: read` + `pull-requests: write` + `id-token: write` (`id-token` mints the Claude GitHub App token and is required even with an - OAuth/API-key credential). The review job re-declares that minimal set so an + OAuth/API-key credential). Each lane's job re-declares that minimal set so an over-granting caller is still narrowed here. - **Never touch the org secret.** `CLAUDE_CODE_OAUTH_TOKEN` is an org secret with visibility *all repositories* — the live, deliberate value. Never @@ -40,12 +45,22 @@ load-bearing; changing any of them needs explicit review. (defaults) — both can surface model-authored content or secrets in publicly visible logs. Never enable Actions debug (`ACTIONS_STEP_DEBUG`) here. Never echo the token. -- **No untrusted checkout before the action.** Do not check out a PR head ref - into the workspace root ahead of the action step. +- **No untrusted checkout before the action — on the two review lanes.** Do not + check out a PR head ref into the workspace root ahead of the action step, and + never execute PR-authored code in a review job. `claude-e2e-verify` is the + deliberate exception, because building, serving, and browser-driving the PR + head *is* that lane: it runs PR-authored code on the runner alongside the + persisted `GITHUB_TOKEN` and the agent's credential. That residual is + accepted, not mitigated in-job; the reasoning, its bound (the + `pull_request`-only tripwire plus the fork-PR no-secrets guarantee), and the + hardening direction if the lane's scope widens are recorded in that + workflow's own header. Do not widen it, and do not carry the pattern into a + review lane. The `secrets:`/`vars:` context is unavailable in composite actions; that, plus -the need for job-level `permissions` and the `secrets:` interface, is why review -is a reusable workflow and the tool-runner lanes are composite actions. +the need for job-level `permissions` and the `secrets:` interface, is why each +Claude lane is a reusable workflow and the tool-runner lanes are composite +actions. ## Pin everything by SHA diff --git a/README.md b/README.md index 5579cf8..170f343 100644 --- a/README.md +++ b/README.md @@ -886,11 +886,12 @@ share one consumption shape. Each is **advisory**: it posts PR comments and never gates `ci-status`. (The advisory verdict is separate from execution evidence: `claude-security-review.yml` scopes itself to security-sensitive paths, and its name-stable `security-review` check may be made a required -status check — see its entry above.) Each is a whole-job concern (job `permissions:` plus a -`secrets:` interface), which is why each is a reusable workflow rather than a -composite action — the caller owns the triggers and the permission grant, and -the workflow owns the SHA-pinned `anthropics/claude-code-action` and the safe -handling. Security rules live in [CLAUDE.md](CLAUDE.md). +status check — see its entry above.) Each is a whole-job concern (job +`permissions:` plus a `secrets:` interface), which is why each is a reusable +workflow rather than a composite action — the caller owns the triggers and the +permission grant, and the workflow owns the SHA-pinned +`anthropics/claude-code-action` and the safe handling. Security rules live in +[CLAUDE.md](CLAUDE.md). ```yaml on: @@ -915,14 +916,39 @@ parent secret. Fork PRs receive no secrets by design and are not reviewed. On both review lanes the caller can name actors whose comments are withheld from the agent's context — prompt-injection hygiene, not a trigger gate. +**What the security lane's required check proves — and does not.** Only +`claude-security-review` reports a check a ruleset can require, under its own +context (` / security-review`), never through `ci-status`. Its +claim is narrow: a security pass RAN at this head, or the PR was judged not +applicable. The verdict stays advisory — findings never fail the job. Execution +is what **fails closed**: when the PR is in scope and the review could not run +at all (usage limit, dead credential, SDK crash), the check reports FAILURE, +because `success`, `neutral` and `skipped` all satisfy a required check, so +failure is the only conclusion that does not silently authorize a merge on +absent evidence. That mapping is scoped to `pull_request` runs; fork PRs and +non-PR events cannot run the review at all, so they keep the pass-through and +their green is not execution evidence. + +The corollary is what the check does not prove. Every legitimate non-run is a +name-stable job-level skip that a ruleset reads as success — a fork PR, an +out-of-scope PR, a skip-listed actor, a kill-switched lane. So a green required +check does not establish that a fork PR was reviewed; review fork changes to +security-sensitive surfaces by hand. Availability is bought back by the bounded +retry below, and a sustained outage is meant to be handled by an explicit, +logged, attributable break-glass on the consumer's ruleset — never by weakening +the check. + **Trigger cadence is per lane, deliberately.** `claude-review` runs on `opened` / `ready_for_review` / `reopened` and **not** on `synchronize`: a push does not re-trigger the code review, so re-run the job for a fresh pass. That caps per-PR spend on active branches, and it is safe precisely because the -lane's verdict gates nothing. `claude-security-review` keeps `synchronize`, -because its check certifies that a security pass ran at the head being merged — -a review of an earlier head is not that evidence. `claude-e2e-verify` keeps it -too. Take each lane's canonical caller from its own workflow header. +lane's verdict gates nothing. It also skips draft PRs at job level, so an +`opened` event on a draft costs nothing and `ready_for_review` is what buys the +review. `claude-security-review` keeps `synchronize`, because its check +certifies that a security pass ran at the head being merged — a review of an +earlier head is not that evidence, and it reviews drafts. `claude-e2e-verify` +keeps `synchronize` too. Take each lane's canonical caller from its own +workflow header. **Bounded retry.** Every lane makes at most **two** agent attempts — one automatic retry, never a loop. The retry is deliberately narrow, because a @@ -948,7 +974,8 @@ orphan comment to clean up between attempts, which the review lanes do. **Review-count cap (code-review lane only).** `claude-review.yml` stops reviewing a PR after `max-reviews-per-pr` successful reviews, capping spend on long-lived PRs. The counter is a **visible** per-PR status comment upserted -after each review, which doubles as the human "was this reviewed" signal; +after each successful review — failed and skipped runs never inflate it — which +doubles as the human "was this reviewed" signal; deleting it resets the count, which is fail-open by design. A capped run is a name-stable skip, not a red check. Treat it as a soft cap: concurrent runs for different heads read the counter before either writes it, so a burst can @@ -974,15 +1001,14 @@ to every repo outside it, so flipping it during an incident would silently no-op exactly where nobody is looking. A kill-switch is only worth having if it reaches the whole fleet. Do not "correct" the visibility to selected. -**Adoption.** Wiring these callers by hand is the fallback. The intended path -is the sync-managed caller components in +**Adoption.** Each lane's own workflow header carries its canonical caller; +copy it from there. Callers are additionally being brought under the org's +sync-managed component distribution in [`melodic-software/standards`](https://github.com/melodic-software/standards), -which distribute a canonical per-lane caller to every target repo and keep it -current through the ordinary sync PR. That work is in flight in -[standards#286](https://github.com/melodic-software/standards/pull/286) and not -yet landed — it is gated on the sync App gaining the `workflows` permission -that writing files under `.github/workflows/` requires. Until it lands, copy -the canonical caller from the lane's own workflow header. +which materializes a canonical per-lane caller into every target repo and keeps +it current through the ordinary sync PR. Where a repo's caller is +sync-managed, change it at the component source and let the sync carry it to +every target — never edit the materialized caller in the target repo. ## Standalone gate checks — shared adoption contract From 455a752aab5b91e35b087f11f04c3ee5bc80135c Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Tue, 28 Jul 2026 23:32:12 -0400 Subject: [PATCH 3/4] docs: close the green-without-review gap the verifier found MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Independent verification against c136b27 found the corollary paragraph asserting something false: it enumerated the security lane's legitimate non-runs as four name-stable job-level skips. There is a fifth path, and it is not a skip. A run whose head was superseded while it queued gates every step off the freshness guard (claude-security-review.yml:569, 576, 651, 800, 830), which leaves review-failed unset so the fail-closed step skips too (:860) — the job reports GREEN having reviewed nothing. It is safe in practice because the newer run for the current head reports the same context, but a paragraph whose entire job is telling a consumer what a green required check does not prove must not omit it. Also corrected, all from the same verification pass: - Two bullets still asserted of three lanes what holds for one. display_report is hardcoded false on the security lane, unset on e2e, and a caller-flippable input on claude-review; show_full_output is set by no lane. And "no untrusted checkout before the action" was never the real invariant — all three lanes check out before the action step and the default pull_request checkout is the merge ref, so PR-authored content is always in the workspace. Presence is not the hazard; execution is. Restated as the invariant that actually holds. - The heading claimed each workflow header cites this section; all three cite the file. Corrected rather than left to imply a heading-level cite that reference-integrity does not protect. - Narrowed "only the security lane reports a check a ruleset can require" to what is true: only its check is designed to be. - Fork PRs skip the job rather than taking the pass-through. - Recorded that claude-e2e-verify gates on nothing but its kill-switches — no draft skip and no skip-actors input — so the most expensive lane has the loosest trigger. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_013pLW2dybov9xvTFtx48Ueb --- CLAUDE.md | 24 ++++++++++++++++-------- README.md | 39 ++++++++++++++++++++++----------------- 2 files changed, 38 insertions(+), 25 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index d905e97..23969c0 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -8,8 +8,8 @@ change must not violate. `claude-review.yml`, `claude-security-review.yml`, and `claude-e2e-verify.yml` each run an AI agent with an org credential on a **public** repo, and each -workflow's own header cites this section for the rationale. These rules are -load-bearing; changing any of them needs explicit review. +workflow's `SECURITY MODEL` header block cites this file for the rationale. +These rules are load-bearing; changing any of them needs explicit review. - **`pull_request` only; never `pull_request_target` or `workflow_run` with secrets.** Those triggers run in a privileged context (base-repo secrets + @@ -42,12 +42,20 @@ load-bearing; changing any of them needs explicit review. selected-repositories. Forced-failure testing uses a repo-level same-name override in a sandbox repo, never a change to the org secret. - **Public-repo log hygiene.** Keep `display_report` and `show_full_output` off - (defaults) — both can surface model-authored content or secrets in publicly - visible logs. Never enable Actions debug (`ACTIONS_STEP_DEBUG`) here. Never - echo the token. -- **No untrusted checkout before the action — on the two review lanes.** Do not - check out a PR head ref into the workspace root ahead of the action step, and - never execute PR-authored code in a review job. `claude-e2e-verify` is the + — both can surface model-authored content or secrets in publicly visible + logs. No lane sets `show_full_output`, so it stays at the action's off + default. `display_report` is enforced unevenly, and deliberately so: + `claude-security-review` hardcodes it `false` so a consumer cannot turn it + on, `claude-e2e-verify` never sets it, and `claude-review` exposes it as an + input for private consumers — **a public caller must never set + `display-report: true`**. Never enable Actions debug (`ACTIONS_STEP_DEBUG`) + here. Never echo the token. +- **Never execute PR-authored code in a review job.** All three lanes check out + before the action step, and on `pull_request` the default checkout resolves to + the PR merge ref, so PR-authored file content is already in the workspace root + — presence is not the hazard, execution is. Never point a checkout at the PR + head ref, and never run a build, install script, or test command against that + tree in a review lane. `claude-e2e-verify` is the deliberate exception, because building, serving, and browser-driving the PR head *is* that lane: it runs PR-authored code on the runner alongside the persisted `GITHUB_TOKEN` and the agent's credential. That residual is diff --git a/README.md b/README.md index 170f343..0f6dfbe 100644 --- a/README.md +++ b/README.md @@ -917,26 +917,29 @@ both review lanes the caller can name actors whose comments are withheld from the agent's context — prompt-injection hygiene, not a trigger gate. **What the security lane's required check proves — and does not.** Only -`claude-security-review` reports a check a ruleset can require, under its own -context (` / security-review`), never through `ci-status`. Its -claim is narrow: a security pass RAN at this head, or the PR was judged not +`claude-security-review`'s check is designed to be required: it reports under +its own context (` / security-review`), never through `ci-status`. +Its claim is narrow: a security pass RAN at this head, or the PR was judged not applicable. The verdict stays advisory — findings never fail the job. Execution is what **fails closed**: when the PR is in scope and the review could not run at all (usage limit, dead credential, SDK crash), the check reports FAILURE, because `success`, `neutral` and `skipped` all satisfy a required check, so failure is the only conclusion that does not silently authorize a merge on -absent evidence. That mapping is scoped to `pull_request` runs; fork PRs and -non-PR events cannot run the review at all, so they keep the pass-through and -their green is not execution evidence. - -The corollary is what the check does not prove. Every legitimate non-run is a -name-stable job-level skip that a ruleset reads as success — a fork PR, an -out-of-scope PR, a skip-listed actor, a kill-switched lane. So a green required -check does not establish that a fork PR was reviewed; review fork changes to -security-sensitive surfaces by hand. Availability is bought back by the bounded -retry below, and a sustained outage is meant to be handled by an explicit, -logged, attributable break-glass on the consumer's ruleset — never by weakening -the check. +absent evidence. That mapping is scoped to `pull_request` runs; a non-PR event +cannot run the review at all and keeps the historical pass-through, and a fork +PR skips the job outright — neither green is execution evidence. + +The corollary is what the check does not prove. Every legitimate non-run reads +as success to a ruleset. Four are name-stable job-level skips: a fork PR, an +out-of-scope PR, a skip-listed actor, a kill-switched lane. A fifth is not a +skip at all — a run whose head was superseded while it queued retires itself +step by step and reports a **green** job having reviewed nothing, on the +premise that the newer run for the current head reports the same context. So a +green required check is not by itself proof that this head was reviewed, and it +never establishes that a fork PR was: review fork changes to security-sensitive +surfaces by hand. Availability is bought back by the bounded retry below, and a +sustained outage is meant to be handled by an explicit, logged, attributable +break-glass on the consumer's ruleset — never by weakening the check. **Trigger cadence is per lane, deliberately.** `claude-review` runs on `opened` / `ready_for_review` / `reopened` and **not** on `synchronize`: a push @@ -947,8 +950,10 @@ lane's verdict gates nothing. It also skips draft PRs at job level, so an review. `claude-security-review` keeps `synchronize`, because its check certifies that a security pass ran at the head being merged — a review of an earlier head is not that evidence, and it reviews drafts. `claude-e2e-verify` -keeps `synchronize` too. Take each lane's canonical caller from its own -workflow header. +keeps `synchronize` too, and gates on nothing but its kill-switches — no draft +skip, no `skip-actors` input — so the most expensive lane has the loosest gate. +Scope it with the caller's own trigger types. Take each lane's canonical caller +from its own workflow header. **Bounded retry.** Every lane makes at most **two** agent attempts — one automatic retry, never a loop. The retry is deliberately narrow, because a From 2004b998335784fd830cd2e866a6eb5b28ccc38c Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Wed, 29 Jul 2026 00:28:43 -0400 Subject: [PATCH 4/4] docs: describe the security lane's real path matcher The `paths` inputs are matched as root-anchored gitignore patterns via `git check-ignore`, not as Actions `paths:` patterns. Identical for ordinary globs; `!` negation cannot be honored as Actions defines it and is now rejected by the workflow (ci-workflows#289). --- README.md | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 0f6dfbe..37b88c7 100644 --- a/README.md +++ b/README.md @@ -637,7 +637,7 @@ GitHub continues the normal weekly patching of each hosted image generation. caller must NOT express that scope with a workflow-level `on.pull_request.paths` filter, because a path miss leaves a required check Pending forever and wedges every prose PR. Instead the caller triggers on all PR events and supplies that - scope as a pattern list in Actions `paths:` syntax (workflow files, + scope as a pattern list of root-anchored globs (workflow files, permission/settings configs, hook and shell scripts, auth/token-touching code, network-call sites); the workflow's `changes` job evaluates it and a not-applicable PR yields a name-stable skipped `security-review` check. A @@ -655,7 +655,12 @@ GitHub continues the normal weekly patching of each hosted image generation. trust boundary. An absent or unreadable file **fails open** (every PR reviewed, with a warning), matching the `changes` job's fail-open discipline throughout; both inputs empty means no filtering, so a consumer that passes - nothing is unaffected. All inputs have public-safe defaults documented inline + nothing is unaffected. Patterns are matched as root-anchored **gitignore** + patterns rather than Actions `paths:` patterns — identical for the ordinary + `*` / `**` globs worth writing here, but `!` negation cannot be honored as + Actions defines it and is rejected outright, so express an exclusion by + narrowing the positive patterns. All inputs have public-safe defaults + documented inline in the workflow header (the authoritative list). Consume it per the [Claude lanes — shared consumption contract](#claude-lanes--shared-consumption-contract) below, triggering on all PR events (no workflow-level `paths:`):