diff --git a/.github/workflows/claude-review.yml b/.github/workflows/claude-review.yml index c289f74..6381f9a 100644 --- a/.github/workflows/claude-review.yml +++ b/.github/workflows/claude-review.yml @@ -19,9 +19,12 @@ # # Harden-runner runs in audit mode (spec K-14): in V10, block mode hung 2 of # 24 jobs in "Complete job" (symptoms match open harden-runner issue 708; -# no fix in v2.22.2). Each job keeps the allowlist that V10 measured, -# so block mode needs only egress-policy: block plus disable-telemetry: true -# (the action accepts that only in block mode). Block mode is revisited +# v2.22.3 has a partial fix only, the safe-by-default fix is not released, +# .github issue 24). Each job keeps the allowlist that V10 measured, plus +# releases.astral.sh:443 where setup-uv runs (v10.3.3 fetches the uv +# version manifest there first; not measured), so block mode needs only +# egress-policy: block plus disable-telemetry: true (the action accepts that +# only in block mode). Block mode is revisited # before org-wide enforcement (build plan, phase 5). name: Claude of Duty @@ -49,7 +52,7 @@ jobs: merge-base-sha: ${{ steps.collect.outputs.merge-base-sha }} steps: - name: Harden the runner - uses: step-security/harden-runner@dc2769abcc0fcfbadfc1df54081c1608579ef6b2 # v2.22.2, released 2026-10-19 + uses: step-security/harden-runner@46b06cea07f9e64621fdf066ba77ac6915163d43 # v2.22.3, released 2026-10-27 with: # Audit until https://github.com/step-security/harden-runner/issues/708 is fixed (spec K-14). egress-policy: audit @@ -57,6 +60,7 @@ jobs: api.github.com:443 github.com:443 raw.githubusercontent.com:443 + releases.astral.sh:443 release-assets.githubusercontent.com:443 files.pythonhosted.org:443 pypi.org:443 @@ -71,7 +75,7 @@ jobs: path: central persist-credentials: false - name: Set up uv - uses: astral-sh/setup-uv@07a047e608db84b3d8c06cd9a7c7dcc9c355c18e # v10.3.2, released 2026-10-22 + uses: astral-sh/setup-uv@68559e99cc807c1eb7e9238fd43a15bba460eec2 # v10.3.3, released 2026-10-28 with: version: 0.13.1 checksum: eadd9d27c134ab5ac1ffd4473b97a741407f3ea238288f03121fc4deddf90220 @@ -115,7 +119,7 @@ jobs: supply-chain-digest: ${{ steps.analyze.outputs.supply-chain-digest }} steps: - name: Harden the runner - uses: step-security/harden-runner@dc2769abcc0fcfbadfc1df54081c1608579ef6b2 # v2.22.2, released 2026-10-19 + uses: step-security/harden-runner@46b06cea07f9e64621fdf066ba77ac6915163d43 # v2.22.3, released 2026-10-27 with: # Audit until https://github.com/step-security/harden-runner/issues/708 is fixed (spec K-14). egress-policy: audit @@ -123,6 +127,7 @@ jobs: api.github.com:443 github.com:443 raw.githubusercontent.com:443 + releases.astral.sh:443 release-assets.githubusercontent.com:443 files.pythonhosted.org:443 api.anthropic.com:443 @@ -136,7 +141,7 @@ jobs: path: central persist-credentials: false - name: Set up uv - uses: astral-sh/setup-uv@07a047e608db84b3d8c06cd9a7c7dcc9c355c18e # v10.3.2, released 2026-10-22 + uses: astral-sh/setup-uv@68559e99cc807c1eb7e9238fd43a15bba460eec2 # v10.3.3, released 2026-10-28 with: version: 0.13.1 checksum: eadd9d27c134ab5ac1ffd4473b97a741407f3ea238288f03121fc4deddf90220 @@ -211,7 +216,7 @@ jobs: outcome: ${{ steps.dr.outcome }} steps: - name: Harden the runner - uses: step-security/harden-runner@dc2769abcc0fcfbadfc1df54081c1608579ef6b2 # v2.22.2, released 2026-10-19 + uses: step-security/harden-runner@46b06cea07f9e64621fdf066ba77ac6915163d43 # v2.22.3, released 2026-10-27 with: # Audit until https://github.com/step-security/harden-runner/issues/708 is fixed (spec K-14). egress-policy: audit @@ -244,7 +249,7 @@ jobs: UV_PYTHON: python3.12 steps: - name: Harden the runner - uses: step-security/harden-runner@dc2769abcc0fcfbadfc1df54081c1608579ef6b2 # v2.22.2, released 2026-10-19 + uses: step-security/harden-runner@46b06cea07f9e64621fdf066ba77ac6915163d43 # v2.22.3, released 2026-10-27 with: # Audit until https://github.com/step-security/harden-runner/issues/708 is fixed (spec K-14). egress-policy: audit @@ -252,6 +257,7 @@ jobs: api.github.com:443 github.com:443 raw.githubusercontent.com:443 + releases.astral.sh:443 release-assets.githubusercontent.com:443 files.pythonhosted.org:443 results-receiver.actions.githubusercontent.com:443 @@ -264,7 +270,7 @@ jobs: path: central persist-credentials: false - name: Set up uv - uses: astral-sh/setup-uv@07a047e608db84b3d8c06cd9a7c7dcc9c355c18e # v10.3.2, released 2026-10-22 + uses: astral-sh/setup-uv@68559e99cc807c1eb7e9238fd43a15bba460eec2 # v10.3.3, released 2026-10-28 with: version: 0.13.1 checksum: eadd9d27c134ab5ac1ffd4473b97a741407f3ea238288f03121fc4deddf90220 diff --git a/.github/workflows/eval.yml b/.github/workflows/eval.yml index 9258557..a6baa34 100644 --- a/.github/workflows/eval.yml +++ b/.github/workflows/eval.yml @@ -1,18 +1,30 @@ # Manual fixture evals against the live API (costs money; run on request only). # # workflow_dispatch only, on the branch to test: a dispatch run uses that -# branch's copy of this file and of the code. The job uses the `eval` -# environment, which needs the owner's approval for every run, and the eval -# federation rule binds the token to that environment. Only this file may use -# `eval`, and its job states contents: read and id-token: write and nothing -# else, so the job that holds the Anthropic credential has no GitHub write -# (spec "Fixture eval credential"; review/tests/unit/test_workflows.py). +# branch's copy of this file and of the code. The environment comes from the +# ref: a run on main uses `eval` (main-only branch policy, no approval: main is +# reviewed code); a run on any other ref uses `eval-test` (the owner approves +# every run, and prevent self-review is on: it can run unreviewed code with the +# eval credential). Each environment has its own eval federation rule, which +# binds the token to that environment. # -# Harden-runner runs in audit mode (spec K-14). The allowlist is not measured -# for this job: it holds the hosts that the same checkout, setup-uv, uv sync -# and Anthropic token exchange and API steps reached in the V10 run of the -# central workflow. Measure it in audit mode before block mode (build plan, -# phase 5). +# The choice is made in the shell of a job with no permissions, not in an +# expression: Actions expressions compare strings without case, so +# `github.ref == 'refs/heads/main'` is also true for a branch named MAIN or +# Main. The shell `[ "$GITHUB_REF" = refs/heads/main ]` is exact. The evals job +# takes that one output for its environment and for EVAL_ENVIRONMENT, so +# evals.py picks that environment's rule. If the select job fails, the evals +# job does not run. Only this file may use `eval` or `eval-test`, and the evals +# job states contents: read and id-token: write and nothing else, so the job +# that holds the Anthropic credential has no GitHub write (spec "Fixture eval +# credential"; review/tests/unit/test_workflows.py). +# +# Harden-runner runs in audit mode (spec K-14). The allowlists are not +# measured. The evals job holds the hosts that the same checkout, setup-uv, +# uv sync and Anthropic token exchange and API steps reached in the V10 run of +# the central workflow. The select job runs one shell test and calls nothing; +# its list holds only hosts the runner itself reaches. Measure them in audit +# mode before block mode (build plan, phase 5). name: eval @@ -21,24 +33,50 @@ on: workflow_dispatch permissions: {} jobs: + select-environment: + runs-on: ubuntu-24.04 + timeout-minutes: 5 + permissions: {} + outputs: + environment: ${{ steps.select.outputs.environment }} + steps: + - name: Harden the runner + uses: step-security/harden-runner@46b06cea07f9e64621fdf066ba77ac6915163d43 # v2.22.3, released 2026-10-27 + with: + egress-policy: audit + allowed-endpoints: > + github.com:443 + results-receiver.actions.githubusercontent.com:443 + - name: Select the eval environment from the ref (exact, case-sensitive) + id: select + run: | + if [ "$GITHUB_REF" = "refs/heads/main" ]; then + echo "environment=eval" >> "$GITHUB_OUTPUT" + else + echo "environment=eval-test" >> "$GITHUB_OUTPUT" + fi + fixture-evals: + needs: select-environment runs-on: ubuntu-24.04 timeout-minutes: 30 - environment: eval + environment: ${{ needs.select-environment.outputs.environment }} permissions: contents: read id-token: write env: UV_PYTHON_DOWNLOADS: never UV_PYTHON: python3.12 + EVAL_ENVIRONMENT: ${{ needs.select-environment.outputs.environment }} steps: - name: Harden the runner - uses: step-security/harden-runner@dc2769abcc0fcfbadfc1df54081c1608579ef6b2 # v2.22.2, released 2026-10-19 + uses: step-security/harden-runner@46b06cea07f9e64621fdf066ba77ac6915163d43 # v2.22.3, released 2026-10-27 with: egress-policy: audit allowed-endpoints: > github.com:443 raw.githubusercontent.com:443 + releases.astral.sh:443 release-assets.githubusercontent.com:443 files.pythonhosted.org:443 api.anthropic.com:443 @@ -49,7 +87,7 @@ jobs: with: persist-credentials: false - name: Set up uv - uses: astral-sh/setup-uv@07a047e608db84b3d8c06cd9a7c7dcc9c355c18e # v10.3.2, released 2026-10-22 + uses: astral-sh/setup-uv@68559e99cc807c1eb7e9238fd43a15bba460eec2 # v10.3.3, released 2026-10-28 with: version: 0.13.1 checksum: eadd9d27c134ab5ac1ffd4473b97a741407f3ea238288f03121fc4deddf90220 diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 09dfe42..b75bda5 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -31,12 +31,13 @@ jobs: UV_PYTHON: python3.12 steps: - name: Harden the runner - uses: step-security/harden-runner@dc2769abcc0fcfbadfc1df54081c1608579ef6b2 # v2.22.2, released 2026-10-19 + uses: step-security/harden-runner@46b06cea07f9e64621fdf066ba77ac6915163d43 # v2.22.3, released 2026-10-27 with: egress-policy: audit allowed-endpoints: > github.com:443 raw.githubusercontent.com:443 + releases.astral.sh:443 release-assets.githubusercontent.com:443 files.pythonhosted.org:443 results-receiver.actions.githubusercontent.com:443 @@ -45,7 +46,7 @@ jobs: with: persist-credentials: false - name: Set up uv - uses: astral-sh/setup-uv@07a047e608db84b3d8c06cd9a7c7dcc9c355c18e # v10.3.2, released 2026-10-22 + uses: astral-sh/setup-uv@68559e99cc807c1eb7e9238fd43a15bba460eec2 # v10.3.3, released 2026-10-28 with: version: 0.13.1 checksum: eadd9d27c134ab5ac1ffd4473b97a741407f3ea238288f03121fc4deddf90220 diff --git a/docs/build-plan.md b/docs/build-plan.md index 5336b49..deb888b 100644 --- a/docs/build-plan.md +++ b/docs/build-plan.md @@ -11,7 +11,7 @@ Every PR to a supported OperationCode repo gets the three-pass Claude review, an Done when: - [ ] The central workflow runs on every supported repo, from `OperationCode/.github` at a pinned SHA. -- [ ] A failed run blocks the merge. Only org admins can bypass, through a pull request. +- [ ] A failed run blocks the merge. Only org admins can bypass, through a pull request. (Follow-up work after phase 6: until someone asks the owner for a gate, maintainers keep the pilot bypass and the review is an advisory signal for them; spec P6-5.) - [ ] Every rule in the [CLAUDE.md](../CLAUDE.md) holds, and the unit tests check the ones that code can check. - [x] The owner has accepted the residuals in writing (spec "Which run counts"; P5-8, 2026-11-01). @@ -23,8 +23,9 @@ Done when: test, fork PR) bypass on) │ 6 org-wide enforcement <── 5 before org-wide <───────┘ - (org ruleset on, (ruleset fixes, - bypass: org admins) risk acceptance, re-tests) + (review ruleset on the (ruleset fixes, + supported repos, pilot risk acceptance, re-tests) + bypass stays, P6-5) Gate on each arrow: the exit criteria of the phase before it. Exception: 1a may start in parallel with phase 0 (see phase 1a). @@ -57,7 +58,7 @@ Owner: - [x] Owner: create a scratch source repo in the org for the V tests, separate from claude-review-test ([claude-review-p0-source](https://github.com/OperationCode/claude-review-p0-source)). - [ ] Owner: Anthropic Console: a dedicated workspace with a hard monthly spend limit and a service account. Create the production federation rule as in spec "Auth and cost": pattern mode, subject prefix `repo:OperationCode@10479364/*`, scope `workspace:developer`, the review workspace only, and the placeholder SHA `0000000000000000000000000000000000000000` in `job_workflow_ref`. Read it back; if the Console rejects the placeholder, stop and resolve the order of rule ID and SHA first (spec V26 a). Its ID goes into the central code; phase 3 sets the candidate SHA. No API key. Done 2026-10-27: workspace `claude-review`, service account `claude-review`, rule R1 `claude-review-prod-r1` created; the Console accepted the placeholder SHA (V26 a partial). The owner read R1 back at M1 and R2 at M2 on 2026-10-27 (spec P3-6). Still open: the service account and the workspace were not read back. - [ ] Owner: the test workspace (reused for evals, spec P2-1), and a federation rule for `workflow_dispatch` runs in `OperationCode/.github` whose job uses the environment `eval`: pattern mode, the same subject prefix, exact claims `repository_owner_id`, `repository_id`, `event_name` and `environment`, scope `workspace:developer`, that workspace only (spec "Auth and cost"). Its IDs are in `evals.py`. The rule cannot name `eval.yml`: any `.github` workflow that uses `eval` can match, so the owner's approval and the workflow unit test (1c) hold it to `eval.yml`. Test it as in spec V27 before phase 2. -- [ ] Owner: on `.github`, create the GitHub environment `eval`: the owner is a required reviewer for every run, and any branch may deploy to it. "Prevent self-review" was off during phases 2 to 4 (owner, 2026-10-26), so that the coordinator, which runs with the owner's token, could approve eval runs it started from reviewed branches. It is on since 2026-11-01. Admin bypass stays on. During phase 2 the coordinator's approval stands in for the owner's under the owner's delegation. Before approving a run, check the run that waits for approval: the workflow file and commit it runs, every job in it that uses `eval`, and their effective permissions (no GitHub write). Also check that the PR does not change `eval.yml`, any other workflow file, the eval code, the prompts, the fixtures or the federation setup in a harmful way, and that `test.yml` passed. The approval is the control; the residual risk is an approver who approves without reading. Exception to the no-execute rule (K-4): evals of PRs to this repo may run that PR's code, only after an owner approves the `eval` environment run. Needed before phase 2. +- [ ] Owner: on `.github`, create the GitHub environment `eval`: the owner is a required reviewer for every run, and any branch may deploy to it. "Prevent self-review" was off during phases 2 to 4 (owner, 2026-10-26), so that the coordinator, which runs with the owner's token, could approve eval runs it started from reviewed branches. It was on from 2026-11-01 to 2026-11-04. Since 2026-11-04 `eval` is for `main` only, with no reviewer, and other refs use `eval-test` (spec P6-4; phase 6 below). Admin bypass stays on. During phase 2 the coordinator's approval stands in for the owner's under the owner's delegation. Before approving a run, check the run that waits for approval: the workflow file and commit it runs, every job in it that uses `eval`, and their effective permissions (no GitHub write). Also check that the PR does not change `eval.yml`, any other workflow file, the eval code, the prompts, the fixtures or the federation setup in a harmful way, and that `test.yml` passed. The approval is the control; the residual risk is an approver who approves without reading. Exception to the no-execute rule (K-4): evals of PRs to this repo may run that PR's code, only after an owner approves the `eval` environment run. Needed before phase 2. - [x] Owner: a test workspace with a very small spend limit for V9. - [ ] Owner: the test org ruleset for V6 that targets only claude-review-test. - [ ] Owner: a Dependabot PR in claude-review-test for V4. @@ -105,8 +106,8 @@ Depends on: V3, V5, V9 (analyze); V6, V16, V24 (publish). - [ ] Guard every artifact download in the gate job with an explicit non-empty id check (D10). An empty `artifact-ids` makes `actions/download-artifact` fetch every artifact of the run. When analyze failed, its ids are empty, and `publish.py` must find a missing directory and fail with the `artifact` reason while it still posts the summary. - [ ] A unit test parses the workflow: no `continue-on-error`, the dependency-review job exports the step `outcome`, every `uses:` is a full SHA. - [ ] `.github/workflows/test.yml`: unit tests on PRs to this repo (`pull_request`, no path filter) and on pushes to `main`. The job is `unit-tests`, with `contents: read`, no `id-token`, no environment and no secrets, so it runs a PR's code with a read-only token (the no-execute rule covers the review workflow). From phase 3 the job has a second step, `pytest tests/unit -m release`, so the check name does not change. -- [ ] Every workflow job starts with harden-runner and has `timeout-minutes`. Actions are pinned to the newest release at least 2 days old (spec P5-5; it was 7 days), except `setup-uv` v10.3.2 (spec K-15). Python is the runner image's 3.12, and `setup-uv` has an exact version and checksum, with its cache and the Astral mirror off. Artifact names carry the run attempt. -- [ ] `.github/workflows/eval.yml`: manual fixture evals. The job uses `environment: eval` and has `contents: read` and `id-token: write` only. A workflow unit test reads every workflow file and fails if a workflow other than `eval.yml` uses `environment: eval`, or if a job that uses `eval` sets no explicit job-level `permissions` or has any `write` permission other than `id-token: write`. Job-level `permissions` is required even if the workflow level sets it. `write-all` at either level fails the test, and so do `read-all` and a missing job-level block, because the job must state `contents: read` and `id-token: write` itself. +- [ ] Every workflow job starts with harden-runner and has `timeout-minutes`. Actions are pinned to the newest release at least 2 days old (spec P5-5; it was 7 days), except `setup-uv` v10.3.2 (spec K-15; since 2026-11-05 every workflow pins harden-runner v2.22.3 and setup-uv v10.3.3, both under the 2-day rule). Python is the runner image's 3.12, and `setup-uv` has an exact version and checksum, with its cache and the Astral mirror off. Artifact names carry the run attempt. +- [ ] `.github/workflows/eval.yml`: manual fixture evals. The job uses `environment: eval` (since spec P6-4: `eval` on `main`, `eval-test` on any other ref, chosen by an exact shell test in a select job with no permissions) and has `contents: read` and `id-token: write` only. A workflow unit test reads every workflow file and fails if a workflow other than `eval.yml` uses `environment: eval` (or `eval-test`), or if a job that uses `eval` sets no explicit job-level `permissions` or has any `write` permission other than `id-token: write`. Job-level `permissions` is required even if the workflow level sets it. `write-all` at either level fails the test, and so do `read-all` and a missing job-level block, because the job must state `contents: read` and `id-token: write` itself. - [ ] `review/src/oc_review/evals.py`: the runner that `eval.yml` calls (`python -m oc_review.evals --fixtures-dir ... --out-dir ...`), with placeholder eval federation IDs and one smoke fixture. The phase 2 fixtures and IDs come later. `analyze` takes an optional federation IDs parameter for it. - [ ] Prefilter run budget: the prefilter line diff has a budget of 40,000,000 steps per run next to its 2,000,000 per file (spec "Coverage limits"). - [x] V10: harden-runner in audit mode for a full run, then block mode (`disable-telemetry: true`) with the measured allowlist. Done 2026-10-27 (spec V10): block mode passed, but 2 of 24 block-mode jobs hung in "Complete job" (the symptoms match open harden-runner issue 708). Ruling K-14: every job runs in audit mode with its measured list and no `disable-sudo-and-containers`; the `release`-marked test holds the central lists to the V10 lists. The `test.yml` and `eval.yml` lists are not measured (phase 5). @@ -127,7 +128,7 @@ Exit criteria: ## Phase 2: fixture evals -The fixture eval calls the live API and costs money. The owner runs it on request only. It is the one exception to the no-execute rule: it may run a PR's code, only after an owner approves the `eval` environment run (K-4). +The fixture eval calls the live API and costs money. The owner runs it on request only. It is the one exception to the no-execute rule: it may run a PR's code, only after an owner approves the eval environment run (K-4; since spec P6-4 a run on a PR branch uses `eval-test`, and a run on `main` uses `eval` with no approval). - [x] V27, before the first eval (passed 2026-10-27, throwaway branch `v27-probe`, never merged; Console deny reasons read by the owner on 2026-10-27: `match_claim_absent` for N1 and the no-environment job of the rejected-approval run, `match_claim_value_mismatch` for the no-environment job of the push run, N3 and N4; claim name and P1 scope not shown in the list view, so P1 scope not confirmed): test the eval federation rule (an approved `eval.yml` run gets a token; the deny cases in the spec get none, including the other-repo case, spec P2-2; it ran as the substitute N4, `push` in claude-review-test with no environment, 401, so repository isolation is not tested separately). - [x] Real eval federation IDs in `evals.py`, checked by a unit test against the production placeholders (spec P2-1). @@ -209,7 +210,10 @@ Depends on: phase 3. Exit criteria: -- [ ] Owner: the owner judges that the false positive rate and the run failures are acceptable on real PRs, after a period the owner picks. +- [ ] Owner: the owner judges that the false positive rate and the run failures are acceptable on real PRs, after a period the owner picks. Evidence (owner, 2026-11-05, spec P6-5): + - [ ] 5 real PRs reviewed across the pilot repos at the live release or later (owner and Dependabot PRs count). + - [ ] 2 weekly metric reports. + - [ ] 7 days in a row with no unexplained failure: no unexplained false-positive hard block, no unexplained run failure, no maintainer bypass without a reason. ## Phase 5: before org-wide enforcement @@ -218,7 +222,7 @@ Exit criteria: - [x] Supported repo inventory: [docs/supported-repos.md](supported-repos.md). - [ ] Owner: every repo to be added to the review ruleset first sends the immutable OIDC subject (spec "Immutable subject"). Check `GET repos/OperationCode//actions/oidc/customization/sub`, check the other `id-token` users in the repo and their relying-party trust, set it per repo and read it back before the repo is a target. Right after the repo becomes a target, run a test PR in that repo (all four jobs `success` at the pin); the repo is accepted only when it passes. - [x] CODEOWNERS in each supported repo (pilot repos P4-3; `START_HERE` P5-4). -- [x] Owner: "prevent self-review" is on for the `eval` environment (2026-11-01). Admin bypass stays on. +- [x] Owner: "prevent self-review" is on for the `eval` environment (2026-11-01). Admin bypass stays on. Changed 2026-11-04 (spec P6-4): `eval` now has a main-only branch policy and no required reviewer or "prevent self-review" (read back 2026-11-05: only the `branch_policy` rule, policy `main`); branch evals move to `eval-test`, which has the owner as reviewer and "prevent self-review" on. - [x] Owner: Dependabot alerts with npm malware detection on active repos. On for the active repos (owner, by 2026-11-04; spec P5-11). - [x] Re-run the key acceptance tests on the current release: V6 a, V8 a, V22 b, V23 and the outage process (the invalid federation rule case), each case on its own terms (V6 a on a same-repo and a fork PR, with the timed-out and slow runs from a scratch-only build; V8 a from an account outside the org). A case not run leaves phase 5 open until the owner accepts the gap in the decision log. Done 2026-11-02 at `88e055f`: all 8 rows pass, and the merge box read mergeable on a run that was not `success` only in the stated residuals (spec P5-9). Finding: the baseline blocks a PR with unsigned commits, also from a fork; the summary now names them (spec P6-1). - [x] Scratch bypass matrix and switch rehearsal on claude-review-test: under the scratch test ruleset and the live baseline, check who can merge which PR kind with the empty, the pilot-shaped and the org-admin-only bypass lists, then return to the pilot-shaped list (the rollback of the phase 6 bypass switch). The baseline is never changed for this test. Pass 2026-11-04: 27 merge cells and 6 direct pushes as expected, the return test passed, every ruleset read back as before (spec P5-10). @@ -235,21 +239,40 @@ Exit criteria: ## Phase 6: org-wide enforcement -- [ ] Owner: add each supported repo to the review ruleset 26132750 (immutable subject first, test PR right after). After each add, start a run on each open PR. After the last add, set its bypass list to org admins in `pull_request` mode (rehearsed on the scratch test ruleset first) and rename it `claude-review`. -- [ ] Keep the supported repo inventory current. A repo joins or leaves through a PR to this repo. -- [ ] Owner: Phase 6 also applies the org security configuration. Details are kept in the owner's records. -- [ ] Release item: summary comment format (spec P6-1): one row per cause, the repo setting label for a Dependency graph that is off, plain paths, no empty token table, and the informational unsigned-commit row with the signing guide link. Display only; the gate does not change. Ships as in "Each release". +Scope (owner, 2026-11-05, spec P6-5): the review targets are the 4 pilot repos; `START_HERE` is not a target (it will be archived, issue #27); the pilot bypass stays. + +- [ ] Keep the supported repo inventory current. A repo joins or leaves through a PR to this repo. A new target: immutable subject first, a test PR right after, then a run on each open PR. +- [x] Owner: Phase 6 also applies the org security configurations. Done and read back 2026-11-05: the configuration for public repos is enforced on the active public repos and is the default for new public repos; the dependency configuration is enforced and is the default for new private and internal repos. One planned change to the dependency configuration is skipped by owner decision (spec P6-5). Details are kept in the owner's records. +- [x] Eval environment split, code part (spec P6-4): `eval.yml` uses `eval` on `main` and `eval-test` on any other ref, chosen by an exact, case-sensitive shell test in a select job with no permissions, whose one output also sets `EVAL_ENVIRONMENT` (an Actions expression compares without case, so `MAIN` would have chosen `eval`); `evals.py` holds the federation IDs per environment and fails closed on another value and on an environment whose rule is not set; the workflow unit test allows the two names only in `eval.yml`. +- [ ] Owner: create the `eval-test` environment on `.github` (required reviewer the owner, "prevent self-review" on, admin bypass as `eval`), and its federation rule on the test workspace (as the eval rule, with the exact claim `environment: "eval-test"`). Then set the rule ID in `evals.py` in a PR, and test it as V27: an approved `eval-test` run on a branch gets a token; a run with no environment, or in another environment, gets none. Until then branch evals fail closed ("eval-test federation rule not configured"). +- [x] Release item: summary comment format (spec P6-1): one row per cause, the repo setting label for a Dependency graph that is off, plain paths, no empty token table, and the informational unsigned-commit row with the signing guide link. Display only; the gate does not change. Shipped in 364058d (PR #26), released 2026-11-04. +- [ ] Release item: comment fixes from the first live run (spec P6-3): the TIP callout names the signing step when the unsigned-commit row shows, "commit" for a one-commit PR, no setting label on a negated message, and an error body cut off mid-read is treated as empty. Display only; the gate does not change. The same release carries the harden-runner v2.22.3 and setup-uv v10.3.3 pins (audit mode stays; `releases.astral.sh` added to the setup-uv jobs' allowlists). Ships as in "Each release". Exit criteria: -- [ ] Every supported repo is a target of the review ruleset and gets the baseline rules, and only org admins can bypass either one, through a pull request. The org security configuration is attached and read back (final acceptance test, checks 1-8). +- [ ] Phase 4 exit is met, then the final acceptance test passes (read-only GETs; record the date and the output path here in a PR): + 1. The review ruleset 26132750 is active, targets exactly the `target` repos in [supported-repos.md](supported-repos.md) by `repository_id` (the 4 pilot repos), on `~DEFAULT_BRANCH`, and never `.github`. + 2. It has one rule, `workflows`, with one entry: the `repository_id` of `.github`, path `.github/workflows/claude-review.yml`, `sha` equal to the live release SHA, and no `ref`. The scratch test ruleset 25835887 has the same pin and targets only claude-review-test. + 3. Its bypass list is the pilot bypass (see spec "Pilot and enforcement"), the accepted state (spec P6-5). + 4. No other org ruleset has a `workflows` entry for the central workflow (the `make release-plan` drift check is `ok`). + 5. On each target, `GET /repos/OperationCode/{repo}/rules/branches/{default branch}` lists `workflows` from 26132750, and `pull_request` (1 approval, dismiss stale approvals, Code Owners review), `non_fast_forward`, `deletion` and `required_signatures` from the baseline 25787833, whose only bypass is org admins in `pull_request` mode. + 6. Each target passes its preflight: no merge queue, CODEOWNERS with no errors, the immutable OIDC subject, the Dependency graph on (SBOM 200), a recursive tree that is not truncated, and a passed run on record at the live release. + 7. The security join matches the read-back of 2026-11-05 (owner's records), feature by feature: + - each active public repo has the public configuration attached and enforced, and reads secret scanning on, push protection on, Dependabot alerts on (`GET .../vulnerability-alerts` 204), Dependabot security updates on, private vulnerability reporting on, and SBOM 200; + - every other repo in the org (the archived repos and the private repo) has the dependency configuration attached and enforced, with the Dependency graph, Dependabot alerts and Dependabot security updates enabled; together the two sets are every repo in the org, each with exactly one configuration; + - the defaults: public repos get the public configuration, private and internal repos get the dependency configuration; + - no feature that was on in that read-back is off now. The dependency configuration's other fields are not checked (the planned change to them is skipped by owner decision). + +Follow-up work (not a phase 6 exit item; spec P6-5): + +- [ ] When someone asks the owner for a gate: set the review ruleset's bypass list to org admins in `pull_request` mode (rehearsed on the scratch test ruleset, spec P5-10), rename it `claude-review`, read it back, and check live on one target that a Maintain user cannot bypass a failed run. Then update `RULESETS` in the release tool to the new name. ## Each release (from phase 4 on) Each release edits rule R1 in place, once, at the merge SHA, with the owner-run release tool (spec "Auth and cost", "Each release"; decision P4-9): - [ ] Merge the release PR. The merge SHA is the release SHA. -- [ ] Validate at the release SHA: `test.yml` (the push run on main) and the owner-approved fixture eval (`eval.yml`, a `workflow_dispatch` run on main) both conclude success. A pull_request run at the same head SHA does not count. +- [ ] Validate at the release SHA: `test.yml` (the push run on main) and the fixture eval on main (`eval.yml`, a `workflow_dispatch` run on main that the owner requests; it runs in `eval` with no approval step since spec P6-4) both conclude success. A pull_request run at the same head SHA does not count. - [ ] Owner: `make release-plan SHA=`. Read the plan: every precondition `ok`, the R1 `job_workflow_ref` change, and the pin change of each ruleset. - [ ] Owner: `make release SHA=`. There is no prompt: it prints the plan and applies at once, so read `make release-plan` first. The tool edits R1 first, then moves the pin of every ruleset that requires the workflow (every ruleset in `RULESETS`; the drift check refuses a release if another org ruleset requires the workflow), and reads all three back. A required run started between the rule edit and the last pin move fails closed. A non-zero exit after a write prints which object is at which SHA, the records path and the recovery (the same command again, or the rollback command it prints): follow it before anything else. - [ ] Start a new run on each open PR (push, or close and reopen). Do not re-run old runs: a run at the old SHA fails auth. Do this after every pin move, also a move back: the box counts only runs at the pinned SHA (spec "Pinned SHA only"). diff --git a/docs/claude-review-design.md b/docs/claude-review-design.md index 89e3956..91fed6e 100644 --- a/docs/claude-review-design.md +++ b/docs/claude-review-design.md @@ -2,7 +2,7 @@ Status: draft, revised after several adversarial design reviews and platform test rounds (see "Decision log"). Code that does not depend on the gate may be built in parallel with the "build" items in "Verify before building"; it merges to main only after they pass. Build plan: [docs/build-plan.md](build-plan.md). Owner: Irving Popovetsky -Last updated: 2026-11-01 +Last updated: 2026-11-05 ## Summary @@ -40,7 +40,7 @@ Out of scope for v1: auto-fix, a verifier pass that drops false positives, revie | A PR edits the review workflow or prompts | The ruleset runs the workflow file from `OperationCode/.github` at a pinned SHA. A PR to a target repo cannot change that file. A file at the same path in the target repo is a different workflow (V23). The `.github` repo itself is excluded from the ruleset and protected by CODEOWNERS plus 1 approval. Only org admins may bypass, and only through a pull request. The prefilter flags every change under `.github/workflows/`. | | Someone forges the gate, or a repo admin turns the review off | The ruleset binds the gate to the central workflow file (repository, path, pinned SHA), not to a check name. In tests with one scratch repo as both workflow source and target, a same-name job from a PR's own workflow neither passed nor blocked the rule (see "What each role can still do"). V8, V22 and V23 test a separate source repo, a pin that is not the tip of main, and a same-path file in the target. Only org owners can change the ruleset. Repo writers can delete runs (V6 f). During the pilot, maintainers and admins can merge past a failed run, still with the approval the baseline requires (see "Pilot and enforcement"). | | Injection tells Claude to run commands, push code, approve or merge | Claude has no tools and no GitHub token. The job that calls Claude has `contents: read`, `pull-requests: read` and `id-token: write` only. The job that writes to GitHub runs no LLM and has no Anthropic credential. The bot never submits `APPROVE` or `REQUEST_CHANGES` (K-1). | -| A PR to `.github` changes eval code, and the eval run executes it with the eval credential | The rule "no PR code is executed" covers the review workflow. One exception: evals of PRs to this repo may run that PR's code, only after an owner approves the `eval` environment run. The environment keeps the owner as required reviewer; "prevent self-review" is on (since 2026-11-01); during phases 2 to 4 it was off and the coordinator's approval stood in for the owner's under the owner's delegation (see "Auth and cost", Approval). The eval credential is its own federation rule on the test workspace `claude-review-p0-test`, not the review workspace, and that workspace has its own spend limit. The eval job has no GitHub write permission (see "Auth and cost"). | +| A PR to `.github` changes eval code, and the eval run executes it with the eval credential | The rule "no PR code is executed" covers the review workflow. One exception: evals of PRs to this repo may run that PR's code, only after an owner approves the eval environment run. A run on any ref other than `main` uses the `eval-test` environment, which has the owner as required reviewer with "prevent self-review" on; a run on `main` uses the `eval` environment, which only `main` can deploy to and which needs no approval, because `main` holds only reviewed code (P6-4; see "Auth and cost", Approval). During phases 2 to 4 one `eval` environment served every branch, with "prevent self-review" off, and the coordinator's approval stood in for the owner's under the owner's delegation. The eval credential is its own federation rule on the test workspace `claude-review-p0-test`, not the review workspace, and that workspace has its own spend limit. The eval job has no GitHub write permission (see "Auth and cost"). | | Injection steals the Anthropic credential | Claude cannot read the environment (no tools). Auth is workload identity federation only, with a short-lived token scoped to one workspace. There is no long-lived key to steal. | | Injection exfiltrates data through the review comment | The only output channel is schema-checked JSON. The publisher escapes every untrusted string (from Claude and from the PR), strips @mentions, links and images, and caps length. Credential-like strings in the PR are redacted before the model sees them and before anything is posted. | | Injection suppresses findings ("this code is safe") | Cannot be fully prevented. Deterministic prefilter checks run with no LLM and Claude cannot clear them. Claude also reports injection attempts as findings. A human approval is required. | @@ -67,7 +67,8 @@ Two org rulesets enforce the review on every supported repo: the review ruleset path .github/workflows/claude-review.yml sha targets: the supported repos by repository id (never .github), default branch - bypass: org admins only, pull_request mode, for emergencies + bypass: the pilot bypass, pull_request mode (see "Pilot and enforcement"); + org admins only is follow-up work (P6-5) baseline ruleset 25787833 org-baseline (enforced by GitHub, changed only by org owners) rule: block deletion @@ -82,7 +83,7 @@ Two org rulesets enforce the review on every supported repo: the review ruleset - The workflows rule binds the gate to the workflow file (repository, path and SHA), not to a check name. GitHub starts the required workflow on each PR event, and the merge waits for that run's result. - The GitHub Team plan offers this rule, and the rule accepts a SHA pin ([claude-review-test](https://github.com/OperationCode/claude-review-test), T6). - The central workflow triggers on `pull_request_target` only. Ruleset-required workflows support it ([changelog](https://github.blog/changelog/2023-10-26-requiring-workflows-with-rulesets-now-supports-the-pull_request_target-trigger)). Target repos hold no caller workflow and no repo ruleset for the review. -- The review ruleset is the active pilot ruleset. Org-wide enforcement adds the supported repos to its targets, then sets its bypass list to org admins only. The baseline ruleset was enabled on a canary repo (infra#192), then widened to all repos (decision P5-1). Each release moves the pin of the review ruleset (see "Rollout"). +- The review ruleset is the active pilot ruleset. Org-wide enforcement adds the supported repos to its targets and keeps the pilot bypass: for maintainers the review is an advisory signal, not a hard gate. Setting its bypass list to org admins only is follow-up work, done only when someone asks the owner for a gate (P6-5). The baseline ruleset was enabled on a canary repo (infra#192), then widened to all repos (decision P5-1). Each release moves the pin of the review ruleset (see "Rollout"). - Before each target change, bypass change and pin move, an org owner reads the review ruleset back through the [org rules API](https://docs.github.com/en/rest/orgs/rules#get-an-organization-repository-ruleset). The workflow entry must have the `repository_id` of `OperationCode/.github`, the path above and the release `sha`, and no `ref`, so no mutable ref can stand in for the pin. Before each review target change the baseline is read back too, and every review target must get the baseline rules from a ruleset whose only bypass is org admins in pull_request mode. - Ruleset Evaluate mode would report without blocking, but the [org rules API](https://docs.github.com/en/rest/orgs/rules#create-an-organization-repository-ruleset) offers it on GitHub Enterprise only ("evaluate is only available with GitHub Enterprise"). On Team the workflows rule is either enforced or off. So the pilot uses an enforced ruleset with a wide bypass list instead (see "Pilot and enforcement"). - Never target `.github`. A ruleset that requires a workflow from `.github` and also targets `.github` can deadlock ([example](https://github.com/ContextualWisdomLab/.github/issues/732)). The file also runs as a normal workflow on PRs to `.github` itself. Nothing requires it there, so that result does not gate merges. That normal run uses the file from the default branch of `.github`, not the PR's copy (see "Workflow details"). It also fails auth by design: the production federation rule binds `job_workflow_ref` to `@`, and a normal run has `@refs/heads/main`, so each pass is `failed` and the run shows a red result. That result does not gate a merge. The file has no event filter to hide it, because a ruleset ignores filters and no test shows that a filter would help. @@ -115,7 +116,7 @@ Facts about ruleset-required workflows ([troubleshooting rules](https://docs.git | Outside contributor (fork PR) | Nothing found. The rule ignores jobs from the PR's own workflows. A fork from a non-member account was tested: the review starts with no approval click and a fake or same-path workflow does not replace it (V8, V23). Cannot cancel, re-run or delete runs. | | Org member without write access to the repo | Same as an outside contributor. Not tested with a separate account. | | Repo writer | Cannot forge or remove the rule. Can approve another writer's PR, which is the 1 approval a merge needs. Can cancel, re-run and delete workflow runs ([workflow runs API](https://docs.github.com/en/rest/actions/workflow-runs#delete-a-workflow-run)). A cancel only blocks. A re-run reads the live PR state. Deleting a newer run on a head SHA can expose an older pass on that SHA. Confirmed as a Write user (V6 f, PR 41). | -| Repo maintainer | Same as a repo writer once the review ruleset has the org-admin-only bypass. On a pilot repo, can merge past a failed run with the pilot bypass, also on their own PR, which GitHub records; the baseline approval rule still applies. | +| Repo maintainer | Same as a repo writer once the review ruleset has the org-admin-only bypass (follow-up work, P6-5). On a pilot repo, and on every review target while the pilot bypass stays, can merge past a failed run with the pilot bypass, also on their own PR, which GitHub records; the baseline approval rule still applies. | | Repo admin | Cannot remove or change the rule. Same run powers as a writer. Can change repo-level Actions settings, which do not affect the required run (V12, phase 3: with Actions disabled, and with an allowed-actions list that excludes the actions the workflow uses, the required run still started and ran). On a pilot repo, can merge past a failed run with the pilot bypass; the baseline approval rule still applies. | | Org admin | Bypasses the ruleset through a pull request, and changes it. | @@ -195,7 +196,7 @@ Only the gate job writes to GitHub, and it writes only comments. It has no Anthr - **Concurrency:** no workflow-level group and no job-level group. Ruleset-required workflows must not use `cancel-in-progress`. Runs for one PR are not serialized; "Which run counts" covers several runs on one head. Every job checks the snapshot before it does costly or visible work (see "Consistent snapshot"). - **Getting the review code:** each job checks out `OperationCode/.github` at `${{ job.workflow_sha }}`, "the commit SHA of the workflow file that defines the current job" ([contexts](https://docs.github.com/en/actions/reference/workflows-and-actions/contexts)). Never use `github.sha` (in `pull_request_target` it is the target repo's default branch) or `github.workflow_sha`. The checkout names `repository: OperationCode/.github` explicitly. The analyze job also checks that its OIDC `job_workflow_sha` claim equals `job.workflow_sha`. - **Never** `actions/checkout` the PR head. Never run `npm install`, `pip install` or any build step on PR content. Never use `actions/cache` in this workflow. -- **Action pinning:** every `uses:` pinned to a full commit SHA, with a comment that names the tag and its release date. Pin the newest release that is at least 2 days old when pinned (decision P5-5). One named exception (K-15): `astral-sh/setup-uv` v10.3.2, released 2026-10-22, a security update that the owner pinned before it was 7 days old (the rule then). A unit test checks the SHA form and the comment. +- **Action pinning:** every `uses:` pinned to a full commit SHA, with a comment that names the tag and its release date. Pin the newest release that is at least 2 days old when pinned (decision P5-5). One named exception in the past (K-15): `astral-sh/setup-uv` v10.3.2, released 2026-10-22, a security update that the owner pinned before it was 7 days old (the rule then). Current pins (2026-11-05): harden-runner v2.22.3 (`46b06cea07f9e64621fdf066ba77ac6915163d43`, released 2026-10-27) and setup-uv v10.3.3 (`68559e99cc807c1eb7e9238fd43a15bba460eec2`, released 2026-10-28), both under the 2-day rule. A unit test checks the SHA form and the comment. - **Python and uv:** Python comes from the runner image (`ubuntu-24.04`, Python 3.12), with `UV_PYTHON_DOWNLOADS=never` and `UV_PYTHON=python3.12`, so no Python download and no `actions/setup-python`. `astral-sh/setup-uv` has an exact uv `version` and its `checksum`, `enable-cache: false` (its default caches on `pull_request` and `workflow_dispatch`) and `download-from-astral-mirror: false`. Deps install with `uv sync --locked --no-dev`. `oc_review` is not installed as a package: steps run `python -m oc_review.` with `PYTHONPATH` set to the checkout's `review/src`. - **No expressions in scripts:** no `${{ }}` appears inside a `run:` script. Every expression reaches a script through `env:`. The gate job's last step runs a fixed script with the default shell, with no `defaults:`, no `shell:` override and a fixed `env:` (the job and step env are exact lists in the unit test), so the exit code of the publish command is the exit code of the job. No workflow refers to the `secrets` context in any form. - **Runner hardening:** `step-security/harden-runner` as the first step of every job, with `egress-policy: audit`, a per-job `allowed-endpoints` list and no `disable-sudo-and-containers` (K-14). The list is measured, not guessed: V10 ran the full workflow in the scratch repo in audit mode, recorded every endpoint per job, then ran block mode with only those. The measured lists, all on port 443: @@ -203,10 +204,11 @@ Only the gate job writes to GitHub, and it writes only comments. It has no Anthr - analyze: `api.github.com`, `github.com`, `raw.githubusercontent.com`, `release-assets.githubusercontent.com`, `files.pythonhosted.org`, `api.anthropic.com`, `*.actions.githubusercontent.com`, `*.blob.core.windows.net` - dependency-review: `api.github.com` only, because `show-openssf-scorecard` is `false` - oc-claude-review: `api.github.com`, `github.com`, `raw.githubusercontent.com`, `release-assets.githubusercontent.com`, `files.pythonhosted.org`, `results-receiver.actions.githubusercontent.com`, `*.blob.core.windows.net` + - added, not measured (2026-11-05): `releases.astral.sh` in every job that runs setup-uv (collect, analyze, oc-claude-review, `test.yml`, `eval.yml`). setup-uv v10.3.3 fetches the uv version manifest from this Astral mirror first, and from `raw.githubusercontent.com` only when that fails (its `src/utils/constants.ts` and `src/download/manifest.ts`). The uv download itself stays on GitHub (`download-from-astral-mirror: false`). Why each host: `github.com` (checkout, setup-uv), `raw.githubusercontent.com` and `release-assets.githubusercontent.com` (uv version manifest and uv download), `files.pythonhosted.org` (`uv sync` from `uv.lock`), `api.github.com` (REST reads and writes), `pypi.org` and `registry.npmjs.org` (collect's registry lookups), `results-receiver.actions.githubusercontent.com` and `*.blob.core.windows.net` (artifact upload and download), `api.anthropic.com` and the Actions OIDC token host (analyze). The blob host and the OIDC host (`run-actions-N-azure-REGION.actions.githubusercontent.com`) change per run, so they need wildcards; the analyze wildcard also covers `results-receiver`. The `test.yml` and `eval.yml` lists are not measured: they hold the hosts that the same checkout, setup-uv and `uv sync` steps (and, for `eval.yml`, the token exchange and the API) reached in V10, and `test.yml` adds `results-receiver.actions.githubusercontent.com`, which the runner itself reached. `eval.yml` also holds `*.blob.core.windows.net:443`, added and not measured in V10, for the eval results artifact upload. Audit mode records egress and blocks nothing. Block mode is `egress-policy: block` plus `disable-telemetry: true`; the action accepts `disable-telemetry: true` only in block mode, so in audit mode harden-runner sends telemetry to StepSecurity. Block mode is revisited before org-wide enforcement (K-14, build plan phase 5). The allowlist is defense in depth. It does not prove that no data can leave, because GitHub itself is on the list. -- **Other workflows:** `test.yml` runs the unit tests of a PR to this repo (`pull_request`, no path filter, and `push` to `main`) with `contents: read` only, no `id-token`, no environment and no secrets. Its job `unit-tests` is the required check name, and it has two test steps: the default unit tests, then `pytest tests/unit -m release` (the `release`-marked tests, see "Testing"), so the check name does not change. It runs the PR's own code with that read-only token, so it is not the review workflow: the no-execute rule (and `CLAUDE.md`'s wording of it) covers the review workflow, which never runs PR content. `eval.yml` is `workflow_dispatch` only (never `workflow_call`), has `permissions: {}` at the top, and one job with the `eval` environment (see "Fixture eval credential"). Its last step, with `if: always()`, uploads `results.jsonl` and each case's `summary.md` as the artifact `eval-results-a` (14 days; the per-case `data/` records stay out, and no uploaded file holds a credential). +- **Other workflows:** `test.yml` runs the unit tests of a PR to this repo (`pull_request`, no path filter, and `push` to `main`) with `contents: read` only, no `id-token`, no environment and no secrets. Its job `unit-tests` is the required check name, and it has two test steps: the default unit tests, then `pytest tests/unit -m release` (the `release`-marked tests, see "Testing"), so the check name does not change. It runs the PR's own code with that read-only token, so it is not the review workflow: the no-execute rule (and `CLAUDE.md`'s wording of it) covers the review workflow, which never runs PR content. `eval.yml` is `workflow_dispatch` only (never `workflow_call`), has `permissions: {}` at the top, and two jobs: `select-environment` (no permissions, no environment) picks `eval` for `refs/heads/main` and `eval-test` for any other ref with an exact shell test, and `fixture-evals` takes that output as its environment (see "Fixture eval credential"). Its last step, with `if: always()`, uploads `results.jsonl` and each case's `summary.md` as the artifact `eval-results-a` (14 days; the per-case `data/` records stay out, and no uploaded file holds a credential). - **Timeouts:** 10 minutes per job (`eval.yml`: 30 minutes). Each Messages API call also has a request deadline (see "Request shape"). A job that times out or loses its runner fails, so the run fails. - **Merge queue:** not supported in v1. Repos that enable a merge queue stay out of the target list until a `merge_group` path is designed and tested. @@ -578,7 +580,8 @@ There is one build of the central workflow. Its gate job always applies every ru - **Bypass actors.** The [org rules API](https://docs.github.com/en/rest/orgs/rules#create-an-organization-repository-ruleset) lists the actor types `OrganizationAdmin` and `RepositoryRole`, and says `actor_id` is required for `RepositoryRole`. "If actor_type is OrganizationAdmin, actor_id is ignored", so the read-back shows `null` there. The `pull_request` bypass mode "means that an actor can only bypass rules on pull requests" ([repo rules API](https://docs.github.com/en/rest/repos/rules)). The docs do not list the `actor_id` values of the built-in Maintain and Admin roles. So an org owner picks the two roles in the ruleset UI, then reads the ruleset back through the API and records the `actor_id` values. Do not assume them. - **Immutable subject (prerequisite for every target repo).** A repo created before GitHub made the immutable OIDC subject the default sends `sub` as `repo:OperationCode/:`. The federation rule's prefix `repo:OperationCode@10479364/*` rejects it (Console reason `match_subject_prefix`), and all three passes fail closed with "authentication failed" and the gate fails. This happened on the first pilot runs. Before a repo joins any ruleset that requires the workflow, an org owner checks `GET repos/OperationCode//actions/oidc/customization/sub`, and if `use_immutable_subject` is not `true` (the `sub_claim_prefix` must contain `@`, not the repo name), sets it per repo with `PUT repos/OperationCode//actions/oidc/customization/sub` with `use_default` true and `use_immutable_subject` true (no org template). The change applies to every OIDC workflow in that repo, not only the review. So first list the repo's other `id-token: write` users and check the trust policy of each relying party (for example a cloud OIDC trust policy): each must accept the immutable subject before the change. Add the new subject and keep the old one. Read the setting back before the repo becomes a target. Right after it becomes a target, start a run on a test PR in that repo and check that all four jobs succeed; a repo whose test fails leaves the target list until it is fixed. - **One review ruleset.** Only the review ruleset, and the scratch test ruleset on claude-review-test, may contain a workflows entry for the central workflow. No two rulesets may require the workflow on the same repo at the same time: it is not tested whether that starts one run or two, and two runs would make the older one `superseded`, so a PR could never pass. The release tool's drift check refuses a release when another org ruleset has one. -- **Org-wide enforcement.** Add each supported repo to the review ruleset (each one already gets the baseline). Then set the review ruleset's bypass list to org admins only, in pull_request mode, and rename it `claude-review`. Only org admins can then bypass either ruleset, through a pull request. An override waives false positives, and the bypass covers outages. +- **Org-wide enforcement.** Add each supported repo to the review ruleset (each one already gets the baseline). The pilot bypass stays (owner decision, 2026-11-05, P6-5): the review is an advisory signal for maintainers, not a hard gate, and the baseline approval still applies to every merge. An override waives false positives, and the bypass covers outages. +- **Gate (follow-up work, P6-5).** When someone asks the owner for a gate: set the review ruleset's bypass list to org admins only, in pull_request mode, and rename it `claude-review` (the switch and its rollback were rehearsed on the scratch test ruleset, P5-10). Only org admins can then bypass either ruleset, through a pull request. - **Dependency review.** The dependency-review step has no `continue-on-error`. The gate does not rely on that alone: the job exports the step `outcome`, and the gate fails, not waivable, when it is missing or not `success`, even if `needs.dependency-review.result` is `success`. A unit test fails if any step in the workflow has `continue-on-error` (see "Testing"). - **Pin moves.** Only org owners can change the pin. Each pin move needs a federation rule for the new SHA first (see "Each release" in "Auth and cost"). Before each pin move an org owner reads the ruleset back (see "The org rulesets"). The review ruleset and the scratch test ruleset use the same pin. The baseline holds no pin. A release moves the pin of every ruleset that requires the workflow, and each is read back. - **The bot never submits a review** (no `APPROVE`, no `REQUEST_CHANGES`). The failing required run is the signal (see K-1 in "Decision log"). @@ -605,11 +608,11 @@ There is one build of the central workflow. Its gate job always applies every ru - **Result heading:** the heading keeps its colored dot and takes one of five fixed texts. `## 🟢 All clear: Claude of Duty review passed` for a pass with no findings (or a pass by an applied waiver). `## 🟡 Clear to proceed: Claude of Duty logged N item(s) to consider` for a pass with non-blocking findings. `## 🔴 Hold deployment: Claude of Duty identified N item(s) requiring attention` for a fail, where N is the number of rows in the findings table (hard blocks, passes or jobs that did not run, repo setting rows, and model finding rows, so it equals what the body lists before a size cut; the informational commit row does not count); a fail with no row reads `## 🔴 Hold deployment: Claude of Duty identified items requiring attention`, with no number. `## 🔴 Comms down: the Claude of Duty review did not complete` for a fail caused by a pass, job or data that did not complete. `## 🔴 Comms down: Claude of Duty could not post its full report` for the fixed overflow notice. N is "1 item" or "N items". The line `*Mission: Review the code. Protect the branch. Deploy the future.*` stands directly under every heading. The result comes from the gate (status, reasons, override), never from model text. Every emoji has its word next to it, so color never carries meaning alone. - **Branding is display text only.** "Claude of Duty" and the job names `Recon` (collect), `Analysis` (analyze), `Supply line check` (dependency-review) and `Mission report` (oc-claude-review) are display names. The workflow file name, the job ids, the `` and `` markers, `needs` references, the ruleset path and the federation `job_workflow_ref` do not change, and no code reads a display name. The check `test / unit-tests` keeps its name because a ruleset requires it by name. - **Size gate callout:** the oversize case reads `**Out of range.** This change is too large for the model review. File checks ran, but model checks did not. A maintainer can waive this.` - - **Callout rules:** `[!TIP]` (green) for a pass with no findings. `[!NOTE]` (blue) for a pass with non-blocking findings. `[!TIP]` for a pass by an applied waiver, with "A maintainer waiver was applied by LOGIN. Findings stay listed." **Every fail uses the red `[!CAUTION]` callout, waivable or not (owner ruling, decision P3-5).** `[!WARNING]` is never used for a fail result. Whether a waiver can clear the fail stays visible in the callout text ("A maintainer can waive it." or "No waiver can clear it.") and in the Effect column. The callout holds two bold sentences: how serious and who can clear it, then the next step (for the author, or for a maintainer when the author has nothing to change). The wording is a fixed table keyed by the worst reason, with fixed labels for reason codes and check IDs ("bidi-control" reads "Hidden text-direction control"); no internal ID shows. A repo setting problem (see "Repo setting rows") is never given to the author as a step: when it is the only part that did not complete, the next step is for a repo admin and says the author does not need to change anything; with a hard block or another failure, the author's step stays and one sentence adds that only a repo admin can fix the setting. + - **Callout rules:** `[!TIP]` (green) for a pass with no findings. `[!NOTE]` (blue) for a pass with non-blocking findings. `[!TIP]` for a pass by an applied waiver, with "A maintainer waiver was applied by LOGIN. Findings stay listed." A TIP ends its first line with "Nothing to do for this review.", except when the unsigned-commit row shows: then it says "One step is left: sign the unsigned commits (see the Commits row)." (P6-3). **Every fail uses the red `[!CAUTION]` callout, waivable or not (owner ruling, decision P3-5).** `[!WARNING]` is never used for a fail result. Whether a waiver can clear the fail stays visible in the callout text ("A maintainer can waive it." or "No waiver can clear it.") and in the Effect column. The callout holds two bold sentences: how serious and who can clear it, then the next step (for the author, or for a maintainer when the author has nothing to change). The wording is a fixed table keyed by the worst reason, with fixed labels for reason codes and check IDs ("bidi-control" reads "Hidden text-direction control"); no internal ID shows. A repo setting problem (see "Repo setting rows") is never given to the author as a step: when it is the only part that did not complete, the next step is for a repo admin and says the author does not need to change anything; with a hard block or another failure, the author's step stays and one sentence adds that only a repo admin can fix the setting. - **Findings table:** one row per place, worst first: hard blocks (🚫), passes or jobs that did not run (⚠️) and repo setting rows (⚙️), then model findings by severity (⛔ critical, 🔴 high, 🟠 medium, 🟡 low), verified before unverified within a severity. Columns: severity, effect, where (file and line in a code span), finding (the title, with a pointer to the inline comment when there is one), found by. The Effect column comes from gate code: "Blocks, no waiver", "Blocks, waivable", "Blocks, waived" (an applied waiver) or "Does not block". A medium finding in a blocking category reads "Does not block" (gate truth: a blocking category blocks only at high or critical severity). An unverified finding shows "❌ not checked" after its severity and still sorts by its severity. The informational commit row sorts last. The table is left out when there are no rows. - **One row per cause (P6-1):** reasons that report one root cause show once, in the findings table and in the record's reasons table. The job result reason of the dependency-review job (`dependency-review: failure`) and the two dependency review reasons (job result and step outcome) are one cause, so one row. Passes that did not complete for the same reason share one row ("Review, Security and Supply chain passes"); passes with different reasons keep their own rows. A merged reasons row lists every detail, so the record keeps every reason. The gate, the step summary and the overflow notice still count every reason. - - **Repo setting rows (P6-1):** a failure that comes from an owner or repo setting, not from the author's change, is labeled as such (⚙️ **Repo setting**, where "Repo settings", effect "Blocks, no waiver"). The case built: when the dependency-review job or step reports `failure`, the gate job reads `GET /repos/{owner}/{repo}/dependency-graph/compare/{merge base}...{head}` once. A 403 alone is not evidence (P6-2): GitHub also answers 403 for a private repo without Advanced Security, for a fork, for a missing permission and for a secondary rate limit, and `actions/dependency-review-action` prints the same "Please ensure that Dependency graph is enabled" advice for every 403. So the label needs positive evidence: HTTP 403, no rate limit headers (`x-ratelimit-remaining: 0`, `retry-after`), and a JSON error `message` from GitHub that says the Dependency graph is disabled, not enabled or turned off, and that does not name a rate limit or Advanced Security. The transport keeps an error body up to the byte cap for this. With that evidence the row says only a repo admin can fix it, and the author does not need to change anything; the checks strip shows "⚙️ repo setting" and the record adds the cause. Any other answer (200, another status, an empty, non-JSON or other message, a network error or any exception in the read) keeps the neutral dependency review row. The exact text GitHub sends for a disabled Dependency graph is not confirmed live; until a live run shows it, the label may never appear, which leaves the neutral row. The label is display only: the reason stays, not waivable, and the result does not change. - - **Unsigned commits row (P6-1):** informational. The gate job reads `GET /repos/{owner}/{repo}/pulls/{n}/commits` (every page; GitHub lists at most 250) and counts the commits whose `commit.verification.verified` is not `true`. With one or more, the findings table ends with a row: ℹ️ **Info**, effect "No effect on this review", where "Commits", "**Unsigned commits:** N of M commits in this PR are not verified-signed. The org requires signed commits, so the merge box blocks until every commit is signed." and a link to [GitHub's commit signing guide](https://docs.github.com/en/authentication/managing-commit-signature-verification), found by "Commit check (code, no model)". Zero unsigned commits, a list that cannot be read, or any error or exception in the read gives no row. The row has no room of its own (P6-2): the summary is rendered without it first, and the row is kept only when the body with it has exactly the same cuts and no overflow, so it can never cause a cut or the fail-closed overflow notice. Reason: the baseline's `required_signatures` blocks the merge of a PR with unsigned commits, also after a passing review and an approval (P5-9), and the author otherwise sees no reason in the comment. The row never changes the result, a reason, the heading, the callout, the waiver line or the exit code. The data never goes to Claude: it is read in the gate job, which has no Anthropic credential, after the passes ran. + - **Repo setting rows (P6-1):** a failure that comes from an owner or repo setting, not from the author's change, is labeled as such (⚙️ **Repo setting**, where "Repo settings", effect "Blocks, no waiver"). The case built: when the dependency-review job or step reports `failure`, the gate job reads `GET /repos/{owner}/{repo}/dependency-graph/compare/{merge base}...{head}` once. A 403 alone is not evidence (P6-2): GitHub also answers 403 for a private repo without Advanced Security, for a fork, for a missing permission and for a secondary rate limit, and `actions/dependency-review-action` prints the same "Please ensure that Dependency graph is enabled" advice for every 403. So the label needs positive evidence: HTTP 403, no rate limit headers (`x-ratelimit-remaining: 0`, `retry-after`), and a JSON error `message` from GitHub that says the Dependency graph is disabled, not enabled or turned off, and that does not name a rate limit or Advanced Security. The message must hold one narrow affirmative phrase (P6-3): "Dependency graph", an optional "for this repository", one verb ("is", "was", "has been"), an optional "currently", then "disabled", "not enabled" or "turned off", with nothing else between them. So "is not disabled", "is neither disabled nor turned off" and "is enabled but X is disabled" are not evidence. The transport keeps an error body up to the byte cap for this; an error body that cannot be read (also a cut-off body, `http.client.IncompleteRead`) is empty, and the status still fails the caller. With that evidence the row says only a repo admin can fix it, and the author does not need to change anything; the checks strip shows "⚙️ repo setting" and the record adds the cause. Any other answer (200, another status, an empty, non-JSON or other message, a network error or any exception in the read) keeps the neutral dependency review row. The exact text GitHub sends for a disabled Dependency graph is not confirmed live; until a live run shows it, the label may never appear, which leaves the neutral row. The label is display only: the reason stays, not waivable, and the result does not change. + - **Unsigned commits row (P6-1):** informational. The gate job reads `GET /repos/{owner}/{repo}/pulls/{n}/commits` (every page; GitHub lists at most 250) and counts the commits whose `commit.verification.verified` is not `true`. With one or more, the findings table ends with a row: ℹ️ **Info**, effect "No effect on this review", where "Commits", "**Unsigned commits:** N of M commits in this PR are not verified-signed. The org requires signed commits, so the merge box blocks until every commit is signed." ("commit" when M is 1, "is" when N is 1; P6-3) and a link to [GitHub's commit signing guide](https://docs.github.com/en/authentication/managing-commit-signature-verification), found by "Commit check (code, no model)". Zero unsigned commits, a list that cannot be read, or any error or exception in the read gives no row. The row has no room of its own (P6-2): the summary is rendered without it first, and the row is kept only when the body with it has exactly the same cuts and no overflow, so it can never cause a cut or the fail-closed overflow notice. Reason: the baseline's `required_signatures` blocks the merge of a PR with unsigned commits, also after a passing review and an approval (P5-9), and the author otherwise sees no reason in the comment. The row never changes the result, a reason, the heading, the waiver line or the exit code. It changes only one callout sentence: a TIP that shows with the row names the signing step instead of "Nothing to do for this review", so the two do not disagree; a NOTE or CAUTION callout does not change. When the row is left out, the TIP keeps "Nothing to do". The data never goes to Claude: it is read in the gate job, which has no Anthropic credential, after the passes ran. - **Merge rule:** model findings merge into one row only when the shown file, side, line and gate effect are all the same. Findings with different effects stay on separate rows. A merged row shows the highest severity, the title of the highest-severity member (ties go to the pass order: review, security, supply-chain), and every pass that reported it, once each, in pass order, with its highest severity. The inline pointer shows only when every member has an inline comment; otherwise the cell says the other findings are in the record only. A merged row can be cut only when every member can be cut (one protected member protects the row). A hard block never merges with a model finding: the hard block is code and the finding is model output. The record keeps every title, one row per finding. - **Checks strip:** one row with file checks (hard blocks and signals), each of the three passes (ran with its finding count, failed, skipped, missing), dependency review (ok, failed, or "⚙️ repo setting") and coverage. It is never cut. - **Collapsed Waiver block:** only when the gate fails for waivable reasons only. It holds the exact override line in a code block with the full SHAs, who may post it, the re-run method, the no-edit and no-reopen rules, and that a waiver covers only this head and merge base. A hard block, a failed pass and every other not-waivable reason show no waiver line. @@ -652,7 +655,8 @@ token lifetime: 10 minutes ``` 1. merge the release PR; the merge SHA is the release SHA 2. validate at the release SHA: test.yml (push to main) and the - owner-approved fixture eval (eval.yml) both conclude success + fixture eval (eval.yml, a workflow_dispatch run on main in the + `eval` environment, no approval step) both conclude success 3. make release-plan SHA= read-only, shows every change 4. make release SHA= no prompt (applies at once after the plan; `make release-plan` is the read-only dry run), then: a. preconditions (fail closed): 40-hex SHA on main; a test.yml @@ -686,10 +690,26 @@ token lifetime: 10 minutes History: phases 3 and 4 used a two-rule flow (a new rule R2 per release at the placeholder SHA, its ID in the candidate, two Console edits, then delete R1; V26, P0-15). Every phase 4 release instead edited R1 in place at the merge SHA (cc710fb, cf19216, 96f4966), and P4-9 makes that the spec. The trade-off: no overlap window in which runs at the old SHA still get tokens, and a rollback is one more rule edit instead of a switch back to the old rule. - **Only analyze can mint a token.** Collect and publish have no `id-token` permission. This is how "the GitHub-writing job has no Anthropic credential" holds, because the federation rule cannot tell jobs in the same workflow apart. -- **Fixture eval credential.** `eval.yml` runs on `workflow_dispatch`, so the review federation rule rejects its token. Evals use a second federation rule (its IDs are in `evals.py`, separate from the production IDs in `analyze.py`). It is bound to the test workspace `claude-review-p0-test`, which is not production and has its own spend limit (P2-1). It uses pattern mode, the subject prefix `repo:OperationCode@10479364/*`, the scope `workspace:developer` and the test workspace only. The scratch rules of phase 3 (V26 c) are on the same workspace, so evals and those tests share its spend limit: when it is spent, evals and those tests stop, and production reviews do not. Exact claims: `repository_owner_id`, `repository_id` of `.github`, `event_name: "workflow_dispatch"` and `environment: "eval"` ([OIDC](https://docs.github.com/en/actions/reference/security/oidc)). In a dispatch run `job_workflow_ref` ends in the branch ref (V3), and evals run on any branch, so an exact claim cannot name `eval.yml`. V27 passed on 2026-10-27 (UTC), on the throwaway branch `v27-probe`, which is never merged. P1 (approved `workflow_dispatch` run, job in environment `eval`): token issued (HTTP 200, req_011CgS2vx6h7nwH6wi3dAFNz, 03:30:02Z). N1 (no environment): 401 (req_011CgS2ticG9pbbKTffkfMeK, 03:29:32Z). N2 (approval rejected): the job never started and made no exchange. N3 (`push` event, approved): 401 (req_011CgS2r8BXRmeR8EPtPd9si, 03:28:57Z). N4 (`push` in claude-review-test, no environment): 401 (req_011CgS34fZVr9eZEVx8z8xZw, 03:31:47Z); the job has no environment and ran on `push`, so the missing environment or the `push` event alone explains the 401, and the `repository_id` check is not tested separately. The dispatch run with its own `eval` environment in claude-review-test was not run. U1 (unit tests, `test_workflows.py -k eval`, on a clean tree): pass. An independent check of the run logs agreed with these results and found no secret. Every deny returns the same 401 body, so the owner read the reasons in the Console federation authentication history on 2026-10-27 (all rows: issuer `github-actions`, rule `eval-federation-rule`, service account `claude-review-p0`; times in UTC). 03:27:59Z Failure `match_claim_value_mismatch` (the no-environment job in the push run). 03:28:57Z Failure `match_claim_value_mismatch` (N3, `push` event). 03:29:32Z Failure `match_claim_absent` (N1, no environment). 03:30:03Z Success (P1, approved dispatch; the probe log has 03:30:02Z, so the two clocks differ by one second). 03:30:34Z Failure `match_claim_absent` (the no-environment job in the rejected-approval run; the Console showed no row for N2 itself, the rejected job, as expected). 03:31:47Z Failure `match_claim_value_mismatch` (N4, other repo). The list view does not name the claim and does not show the scope of the P1 token: claim name and P1 scope not shown in the list view. The next sentence is inference from the probe token claims, not read from the Console: each failure code fits the expected cause, an absent `environment` claim or a mismatched `event_name` or `repository` claim, but the list view names no claim. The no-environment job in the push run got `match_claim_value_mismatch`, not `match_claim_absent`; this fits the `event_name` check failing first (also inference). - - **Claim boundary.** The rule accepts a dispatch run of any workflow file in `.github` whose job uses the `eval` environment, not only `eval.yml`. That only `eval.yml` uses `eval` is a repo convention; the rule cannot enforce it. Two controls hold the credential separation: the owner's approval of each run, and a workflow unit test. The unit test reads every workflow file in `.github` and fails if a workflow other than `eval.yml` uses the `eval` environment, or if a job that uses `eval` sets no explicit job-level `permissions` or has any `write` permission other than `id-token: write`. Environment names are not case sensitive, so the test treats `EVAL` as `eval`, and an environment that is not a literal string (an expression) counts as a use of `eval`. Job-level `permissions` is required on every job that uses `eval`, even when the workflow level sets it, and it holds exactly `contents: read` and `id-token: write`. `eval.yml` triggers on `workflow_dispatch` only. The test handles the string forms: `write-all` at either level fails the test, and `read-all` or a missing job-level block also fails it, because the job must state `contents: read` and `id-token: write` itself. The eval job has `contents: read` and `id-token: write` only, so the job that holds the Anthropic credential has no GitHub write permission. - - **Approval.** The `eval` environment allows deployments from any branch, and requires the owner's approval before every run. The federation rule stays bound to the environment claim, so no token is minted for a run that the owner did not approve. Before approving, the approver checks the run that waits for approval: the workflow file and commit it runs, every job in that workflow that uses `eval`, and their effective permissions (no GitHub write). The approver also checks that the PR does not change `eval.yml`, any other workflow file, the eval code, the prompts, the fixtures or the federation setup in a harmful way, and that `test.yml` passed. The approval is the control: only a few trusted people touch the `.github` repo, and the residual risk is an approver who approves without reading the diff. The `eval` environment keeps the owner as required reviewer. "Prevent self-review" is on (since 2026-11-01). It was off during phases 2 to 4 (owner decision, 2026-10-26), so that the coordinator could approve eval runs it started from reviewed branches; the owner had delegated the approval to the coordinator, which runs with the owner's token. Admin bypass stays on. - - Evals never use the review workspace or a static key. The rule "never check out or execute PR content" covers the review workflow. One exception: evals of PRs to `OperationCode/.github` may run that PR's code, only after an owner approves the `eval` environment run (K-4). Evals of unmerged PR code still need the owner's approval; "prevent self-review" is on (since 2026-11-01); during phases 2 to 4 the coordinator's approval stood in for it under the owner's delegation. +- **Fixture eval credential.** `eval.yml` runs on `workflow_dispatch`, so the review federation rule rejects its token. Evals use their own federation rules, one per eval environment (P6-4): the evals job uses the environment `eval` for a run on `refs/heads/main` and `eval-test` for any other ref, and sets `EVAL_ENVIRONMENT` to the same value. A select job with no permissions makes the choice with an exact shell test (`[ "$GITHUB_REF" = "refs/heads/main" ]`) and exports it as its one output; the evals job takes that output (`${{ needs.select-environment.outputs.environment }}`) for both. The choice is not an Actions expression: expressions compare strings without case, so `github.ref == 'refs/heads/main'` is also true for a branch named `MAIN` or `Main` (review AC, I1). If the select job fails, the evals job does not run. `evals.py` holds the IDs per environment (separate from the production IDs in `analyze.py`): the same organization; for each environment its own rule and a service account (`eval-test` uses the `eval` service account until the owner chooses another). The runner accepts only `eval` and `eval-test` in `EVAL_ENVIRONMENT`; any other value, or an environment whose rule ID is not set, fails closed before any token is minted ("eval-test federation rule not configured" until the owner creates that rule). The `eval` rule is described below; the `eval-test` rule is the same with the exact claim `environment: "eval-test"`. It is bound to the test workspace `claude-review-p0-test`, which is not production and has its own spend limit (P2-1). It uses pattern mode, the subject prefix `repo:OperationCode@10479364/*`, the scope `workspace:developer` and the test workspace only. The scratch rules of phase 3 (V26 c) are on the same workspace, so evals and those tests share its spend limit: when it is spent, evals and those tests stop, and production reviews do not. Exact claims: `repository_owner_id`, `repository_id` of `.github`, `event_name: "workflow_dispatch"` and `environment: "eval"` ([OIDC](https://docs.github.com/en/actions/reference/security/oidc)). In a dispatch run `job_workflow_ref` ends in the branch ref (V3), and evals run on any branch, so an exact claim cannot name `eval.yml`. V27 passed on 2026-10-27 (UTC), on the throwaway branch `v27-probe`, which is never merged. P1 (approved `workflow_dispatch` run, job in environment `eval`): token issued (HTTP 200, req_011CgS2vx6h7nwH6wi3dAFNz, 03:30:02Z). N1 (no environment): 401 (req_011CgS2ticG9pbbKTffkfMeK, 03:29:32Z). N2 (approval rejected): the job never started and made no exchange. N3 (`push` event, approved): 401 (req_011CgS2r8BXRmeR8EPtPd9si, 03:28:57Z). N4 (`push` in claude-review-test, no environment): 401 (req_011CgS34fZVr9eZEVx8z8xZw, 03:31:47Z); the job has no environment and ran on `push`, so the missing environment or the `push` event alone explains the 401, and the `repository_id` check is not tested separately. The dispatch run with its own `eval` environment in claude-review-test was not run. U1 (unit tests, `test_workflows.py -k eval`, on a clean tree): pass. An independent check of the run logs agreed with these results and found no secret. Every deny returns the same 401 body, so the owner read the reasons in the Console federation authentication history on 2026-10-27 (all rows: issuer `github-actions`, rule `eval-federation-rule`, service account `claude-review-p0`; times in UTC). 03:27:59Z Failure `match_claim_value_mismatch` (the no-environment job in the push run). 03:28:57Z Failure `match_claim_value_mismatch` (N3, `push` event). 03:29:32Z Failure `match_claim_absent` (N1, no environment). 03:30:03Z Success (P1, approved dispatch; the probe log has 03:30:02Z, so the two clocks differ by one second). 03:30:34Z Failure `match_claim_absent` (the no-environment job in the rejected-approval run; the Console showed no row for N2 itself, the rejected job, as expected). 03:31:47Z Failure `match_claim_value_mismatch` (N4, other repo). The list view does not name the claim and does not show the scope of the P1 token: claim name and P1 scope not shown in the list view. The next sentence is inference from the probe token claims, not read from the Console: each failure code fits the expected cause, an absent `environment` claim or a mismatched `event_name` or `repository` claim, but the list view names no claim. The no-environment job in the push run got `match_claim_value_mismatch`, not `match_claim_absent`; this fits the `event_name` check failing first (also inference). + - **Claim boundary.** Each eval rule accepts a dispatch run of any workflow file in `.github` whose job uses its environment, not only `eval.yml`. That only `eval.yml` uses `eval` and `eval-test` is a repo convention; the rules cannot enforce it. Three controls hold the credential separation: the `eval` branch policy (only `main`, which holds reviewed code, can deploy to it), the owner's approval of each `eval-test` run, and a workflow unit test. The unit test reads every workflow file in `.github` and fails if a workflow other than `eval.yml` uses `eval` or `eval-test`, if `eval.yml` lacks the select job (no permissions, no environment, the one output, the fixed exact shell test), if a job in `eval.yml` with an environment does not need that job, uses anything but its output, or sets `EVAL_ENVIRONMENT` to anything else, or if such a job sets no explicit job-level `permissions` or has any `write` permission other than `id-token: write`. Environment names are not case sensitive, so the test treats `EVAL` as `eval`, and in any other workflow an environment that is not a literal string (an expression) counts as a use. Job-level `permissions` is required on every job that uses `eval`, even when the workflow level sets it, and it holds exactly `contents: read` and `id-token: write`. `eval.yml` triggers on `workflow_dispatch` only. The test handles the string forms: `write-all` at either level fails the test, and `read-all` or a missing job-level block also fails it, because the job must state `contents: read` and `id-token: write` itself. The eval job has `contents: read` and `id-token: write` only, so the job that holds the Anthropic credential has no GitHub write permission. + - **Approval (P6-4).** Two environments, chosen by the ref: + + ``` + eval.yml run (workflow_dispatch) + | + +-- ref == refs/heads/main --> environment `eval` + | branch policy: main only + | no required reviewer (main is reviewed code) + | admin bypass on + | + +-- any other ref -----------> environment `eval-test` + required reviewer: the owner (irvingpop) + prevent self-review on + (can run unreviewed code with the eval credential) + ``` + + Live `eval` (read 2026-11-05, `GET /repos/OperationCode/.github/environments/eval`): one protection rule, `branch_policy`, with a custom branch policy `main`; no required reviewer; admin bypass on. The owner changed it on 2026-11-04 (reviewer and "prevent self-review" removed, main-only branch policy added), so release evals on `main` run without an approval. A run on `main` can deploy only to `eval` and a run on another ref only asks for `eval-test`; a branch that edits `eval.yml` to ask for `eval` is refused by the branch policy, so no token is minted. Each federation rule stays bound to its environment claim, so no `eval-test` token is minted for a run that the owner did not approve. Before approving an `eval-test` run, the approver checks the run that waits for approval: the workflow file and commit it runs, every job in that workflow that uses `eval`, and their effective permissions (no GitHub write). The approver also checks that the PR does not change `eval.yml`, any other workflow file, the eval code, the prompts, the fixtures or the federation setup in a harmful way, and that `test.yml` passed. The approval is the control: only a few trusted people touch the `.github` repo, and the residual risk is an approver who approves without reading the diff. Admin bypass stays on for both environments. History: until 2026-11-04 one `eval` environment allowed any branch with the owner as required reviewer. "Prevent self-review" was off during phases 2 to 4 (owner decision, 2026-10-26), so that the coordinator could approve eval runs it started from reviewed branches (the owner had delegated the approval to the coordinator, which runs with the owner's token), and on from 2026-11-01 to 2026-11-04. + - Evals never use the review workspace or a static key. The rule "never check out or execute PR content" covers the review workflow. One exception: evals of PRs to `OperationCode/.github` may run that PR's code, only after an owner approves the `eval-test` environment run (K-4, P6-4). Evals of unmerged PR code always run in `eval-test`, which needs the owner's approval with "prevent self-review" on; during phases 2 to 4 the coordinator's approval stood in for it under the owner's delegation. - **No static key fallback.** An org secret is readable by any workflow in every repo that can see it, so a repo writer could copy it or spend it. If federation cannot be made to work (V2, V3), building stops and the auth design is revisited. If a static key is ever used, record that every writer of every target repo can read it. - **Workspace:** a dedicated Anthropic workspace with a hard monthly spend limit. At the limit the Messages API returns HTTP 400 `invalid_request_error` with the message "You have reached your specified workspace API usage limits" (V9), not a 429. The SDK does not retry a 400, so the pass fails fast. Analyze classifies this error as an outage, not as a request bug: each pass that gets this error is `failed` with the clear reason "workspace spend limit reached", recorded in `meta.reason`, and the gate reason and the summary show it. Analyze matches all three: HTTP 400, error type `invalid_request_error`, and the text "workspace API usage limits" in the message. Any other 400 is "request rejected". A pass that completed before the limit keeps its result, and analyze still writes exactly three pass records. Any such failed pass makes the gate fail, and no override can waive it. The token exchange still succeeds at the limit, so the outage shows only at the Messages call. Access returns on the first day of the next month at 00:00 UTC, or when the owner raises the limit. - **Overshoot (residual).** The limit is not checked against live spend on each request. In V9, with sequential calls, enforcement lagged about 39 s after the spend passed the limit, so 3 more calls got through. The overshoot equals the cost of the calls that start during the lag. With a deliberately tiny test limit, spend reached 87 percent over the limit. Three passes per run and concurrent runs start more calls during the lag, so they can overshoot more. The limit bounds spend only approximately; size it with this lag in mind. @@ -739,7 +759,7 @@ token lifetime: 10 minutes ## Testing -- **Unit tests (CI on every PR to this repo, no API calls):** prefilter checks, coverage rules, lockfile parsing per supported version, citation check (head and base side, title and body paths, marker-only quotes, inline eligibility, empty quotes, a quote or file that analyze cut to its limit), gate reasons, override checks, envelope checks, sanitizer and escaping, redaction, the summary comment format (one row per cause, the repo setting label, plain paths, the token line, and the unsigned-commit row with the gate result unchanged, P6-1), nonce handling, "every prompt has ``", schema validation of good and bad fixtures, and a workflow file check. It parses every workflow with duplicate keys rejected. For the central workflow it fails on: another trigger than `pull_request_target`, a workflow-level permission, a `concurrency` key, any `continue-on-error`, job names or job scopes that differ from "Architecture", `id-token` outside analyze, a `checks` scope, a job `if:` other than `always()` on the gate job, a step `if:` other than the exact guard on a gate download, a `uses:` that is not a full SHA with a tag comment, a checkout of anything but `OperationCode/.github` at `job.workflow_sha`, a PR ref or `github.sha`, an `actions/cache` step, a `setup-uv` step without `enable-cache: false`, a `${{` in a `run:`, the `secrets` context, a download by name, pattern or run, an upload without `if-no-files-found: error` or without the attempt in its name, a harden-runner step that is not first, not in `audit` mode, without a non-empty `host:port` allowlist, with `disable-telemetry: true` (block mode only) or with `disable-sudo-and-containers`, an `ANTHROPIC_*` variable, a missing or wrong timeout, a publish step that is not the fixed script, and a dependency-review job that does not export the step `outcome`. Two tests are marked `release` and skipped in the default run: `test_release_federation_ids_are_not_placeholders` (the production IDs in `analyze.py` are valid and not placeholders) and `test_harden_runner_egress_lists_are_the_v10_lists`. CI runs them on every PR in a second step of the `unit-tests` job (`pytest tests/unit -m release`; `test_test_workflow_runs_the_release_tests_in_the_unit_tests_job` checks the step), so a candidate that names a placeholder cannot merge. The second test checks that each central job's allowlist equals the list that V10 measured, in `audit` mode (K-14). It also reads every workflow file in this repo, including `test.yml` (no environment, no `id-token`, no secrets), and fails if a workflow other than `eval.yml` uses the `eval` environment, or if a job that uses `eval` sets no explicit `permissions` or has any `write` permission other than `id-token: write` (see "Fixture eval credential"). +- **Unit tests (CI on every PR to this repo, no API calls):** prefilter checks, coverage rules, lockfile parsing per supported version, citation check (head and base side, title and body paths, marker-only quotes, inline eligibility, empty quotes, a quote or file that analyze cut to its limit), gate reasons, override checks, envelope checks, sanitizer and escaping, redaction, the summary comment format (one row per cause, the repo setting label, plain paths, the token line, and the unsigned-commit row with the gate result unchanged, P6-1), nonce handling, "every prompt has ``", schema validation of good and bad fixtures, and a workflow file check. It parses every workflow with duplicate keys rejected. For the central workflow it fails on: another trigger than `pull_request_target`, a workflow-level permission, a `concurrency` key, any `continue-on-error`, job names or job scopes that differ from "Architecture", `id-token` outside analyze, a `checks` scope, a job `if:` other than `always()` on the gate job, a step `if:` other than the exact guard on a gate download, a `uses:` that is not a full SHA with a tag comment, a checkout of anything but `OperationCode/.github` at `job.workflow_sha`, a PR ref or `github.sha`, an `actions/cache` step, a `setup-uv` step without `enable-cache: false`, a `${{` in a `run:`, the `secrets` context, a download by name, pattern or run, an upload without `if-no-files-found: error` or without the attempt in its name, a harden-runner step that is not first, not in `audit` mode, without a non-empty `host:port` allowlist, with `disable-telemetry: true` (block mode only) or with `disable-sudo-and-containers`, an `ANTHROPIC_*` variable, a missing or wrong timeout, a publish step that is not the fixed script, and a dependency-review job that does not export the step `outcome`. Two tests are marked `release` and skipped in the default run: `test_release_federation_ids_are_not_placeholders` (the production IDs in `analyze.py` are valid and not placeholders) and `test_harden_runner_egress_lists_are_the_v10_lists`. CI runs them on every PR in a second step of the `unit-tests` job (`pytest tests/unit -m release`; `test_test_workflow_runs_the_release_tests_in_the_unit_tests_job` checks the step), so a candidate that names a placeholder cannot merge. The second test checks that each central job's allowlist equals the list that V10 measured, in `audit` mode (K-14). It also reads every workflow file in this repo, including `test.yml` (no environment, no `id-token`, no secrets), and fails if a workflow other than `eval.yml` uses the `eval` or `eval-test` environment, if `eval.yml` picks its environment or sets `EVAL_ENVIRONMENT` other than from the select job's exact shell test (the test runs that script for `refs/heads/main`, `MAIN`, `Main`, tags, PR refs and others), or if a job that uses an eval environment sets no explicit `permissions` or has any `write` permission other than `id-token: write` (see "Fixture eval credential"). The runner tests check that `evals.py` accepts only `eval` and `eval-test` and fails closed on a missing value, another value, or an environment whose rule is not configured. - **Gate matrix (unit):** every row of the reasons table on its own and in combination, plus: missing pass record, duplicate pass record, extra file, wrong `run_attempt`, digest mismatch, envelope for another PR, each `needs..result` value (`failure`, `cancelled`, `skipped`), stale head, override with a wrong SHA, edited override, override by the PR author, override by a user with `read`, override against a non-waivable reason, override older than the push epoch, override older than a reopened event, override posted after a reopened event and then a re-run (accepted), unreadable timeline, several valid overrides, spoofed sticky marker from another author, base ref not the default branch, merge base changed, a newer run at the final check (`superseded`), a grant deleted before the final check, a dependency-review step `outcome` that is `failure` or missing while `needs.dependency-review.result` is `success` (not-waivable fail), and the exit code for every gate result (0 only for `pass`). - **Eval runner:** `python -m oc_review.evals --fixtures-dir --out-dir ` is the entry point of `eval.yml`. It reads `JOB_WORKFLOW_SHA` and the runner OIDC variables in the env. A case is a directory with `bundle.json` (an `ok` bundle envelope) and `expected.json`. It calls `analyze` with the eval federation IDs (P2-1), computes the gate with synthetic trusted inputs (every job `success`, dependency review `success`, a current snapshot, no overrides), renders the sticky summary with the publisher's code and writes it to `//summary.md` (nothing is posted inline, so every finding shows in full). It prints one JSON line per case with token counts only, and exits 0 only when at least one case ran and every case matched. A case matches only when all three passes are `ok`. `expected.json` (format v2) has these keys, each optional; another key, a duplicate key at any depth, a wrong type (`null` included) or an unknown value fails the case before any call: - `gate`: `pass` or `fail`. @@ -749,7 +769,7 @@ token lifetime: 10 minutes - `summary_excludes`: strings that must not appear in the rendered summary, as written or with the publisher's Markdown escapes removed. A mismatch names the index, never the string. - **Fixture builder:** a fixture's `bundle.json` is the trusted input for the gate's hard blocks, so it is never written by hand. `review/tests/eval_fixtures.py` (trusted code, offline) builds each bundle from the case's `source/` files: it synthesizes the GitHub and registry responses, runs the real `collect.collect()` with a fixed time and run ID, and writes `bundle.json`. A unit test rebuilds every case and fails when a committed bundle differs, so the hard blocks, signals and redaction in a fixture come from the real prefilter and collect. Unit tests also prove, with fake pass records and no API, that each hard-block case fails the gate whatever Claude returns, and that each case with `hard_blocks: []` has no hard block. The smoke case `review/tests/unit/fixtures/eval-smoke` is for the unit tests only and is not part of a live run (P2-3). - **Dependency stage reproduction (local, manual):** `cd review && uv run python tools/dep_repro.py --repo OWNER/NAME --head HEAD_SHA --base MERGE_BASE_SHA` reads the two trees and the changed blobs through the REST API as data (never a checkout, install or build), runs `collect.dependency_facts` and the prefilter with the production limits against the live registries, and prints the keys, unique names, decoded and wire bytes, the lookup wall time, each failed lookup and every hard block and signal with a count. It is not imported by production code and not run in CI (K-20). -- **Fixture evals (manual `eval.yml`, costs money):** the cases in `review/tests/fixtures`. Run on the PR branch after each prompt or model change, with owner approval of the `eval` environment. Passing evals gate the merge of a prompt or model change, and they also gate any move of a ruleset pin: a pin moves only to a SHA whose tree has passed the evals (a merge SHA with the same tree as the tested candidate counts). No required check enforces this gate, so the owner holds the merge until the evals pass. Record the results in that PR. The pass bar is one full run with every case matched (P2-4). A miss is fixed in the prompt or the code, then the full set runs again. An expectation changes only with a written reason in the PR. Minimum set, with the part that code decides (unit tests check it, and the live run again) and the part that the model decides (only the live run checks it): +- **Fixture evals (manual `eval.yml`, costs money):** the cases in `review/tests/fixtures`. Run on the PR branch after each prompt or model change, with owner approval of the `eval-test` environment (a run on `main` uses `eval` and needs no approval, P6-4). Passing evals gate the merge of a prompt or model change, and they also gate any move of a ruleset pin: a pin moves only to a SHA whose tree has passed the evals (a merge SHA with the same tree as the tested candidate counts). No required check enforces this gate, so the owner holds the merge until the evals pass. Record the results in that PR. The pass bar is one full run with every case matched (P2-4). A miss is fixed in the prompt or the code, then the full set runs again. An expectation changes only with a written reason in the PR. Minimum set, with the part that code decides (unit tests check it, and the live run again) and the part that the model decides (only the live run checks it): - clean small refactor -> `pass`, no high findings (`gate: pass`, `max_severity: medium`, no hard block). Code: no hard block. Model: the gate and the severities. - real bug (`if (user.role = "admin")`) -> verified `bug` finding (`bug` `low` or higher, `file`, `side: head`, `verified: true`). Code: no hard block, and `verified` comes from the citation check. Model: that the finding exists. - SQL injection -> verified `security` high (`gate: fail`). Code: no hard block; the gate follows from the finding. Model: the finding. @@ -845,9 +865,9 @@ The tasks, owner actions and exit criteria for each step are in [docs/build-plan 3. **Build** in this repo, with unit tests passing. There is one build; its gate job fails for real on `fail` (see "Pilot and enforcement"). 4. **Fixture evals and acceptance:** the fixture eval passes, and the workflow acceptance tests pass in the scratch repo, including a fork PR from an account outside the org. 5. **Pilot:** the "blocking" items must pass first. Check that the federation rule named in the release code has the release SHA. An org owner reads the baseline ruleset back and checks that each pilot repo gets its approval rule (1 approval, Code Owners review, dismiss stale approvals on push, bypass org admins only; at the pilot start on 2026-10-28, repo rulesets did this, P4-3), and checks that each pilot repo sends the immutable OIDC subject (see "Immutable subject" in "Pilot and enforcement"), then creates the pilot ruleset (see "Pilot and enforcement") with the release SHA and the pilot repos, reads it back, and enables it. Then, right after the enable, list the open PRs again (not from earlier recon) and start a run on each (close and reopen, or a new push). A required workflow starts only on a new PR event, so a PR opened between the recon and the enable gets no run (front-end PR 2027, opened 2026-10-29T08:06Z, was missed this way). Watch the real results on real PRs: false positives, overrides, bypasses, egress and cost per run. The owner decides when the pilot is done. -6. **Each release** (from the pilot on): follow "Each release" in "Auth and cost". Merge, validate at the merge SHA (unit tests and the owner-approved fixture eval), then the owner runs `make release-plan` and `make release` with that SHA: the tool edits R1 in place, moves the pin of every ruleset that requires the workflow, and reads all three back. Start a new run on each open PR, and check one token exchange under the scratch test ruleset. +6. **Each release** (from the pilot on): follow "Each release" in "Auth and cost". Merge, validate at the merge SHA (unit tests and the fixture eval on main, which the owner requests; it needs no approval since P6-4), then the owner runs `make release-plan` and `make release` with that SHA: the tool edits R1 in place, moves the pin of every ruleset that requires the workflow, and reads all three back. Start a new run on each open PR, and check one token exchange under the scratch test ruleset. 7. **Before org-wide enforcement:** the owner accepted in writing (P5-8, 2026-11-01) the residuals in "Which run counts" (an earlier pass on the same SHA, the cross-PR effect of one head SHA, a pin rollback, and run deletion, which V6 f confirmed for a Write user). The baseline ruleset 25787833 is live on every repo (P5-1). Build the supported repo inventory (see "Supported repos"), and add CODEOWNERS to each supported repo. Revisit harden-runner block mode (K-14). Re-run the key acceptance tests. -8. **Org-wide enforcement:** add the supported repos to the review ruleset, in small groups, then set its bypass list to org admins only (see "Pilot and enforcement"). After each group, start a run on each open PR, and check that each shows the required workflow passing or failing on its own result. +8. **Org-wide enforcement:** add the supported repos to the review ruleset, in small groups; the pilot bypass stays (see "Pilot and enforcement", P6-5). After each group, start a run on each open PR, and check that each shows the required workflow passing or failing on its own result. The switch to an org-admin-only bypass is follow-up work, done only when someone asks the owner for a gate. ## Admin checklist (Irving) @@ -855,7 +875,7 @@ The tasks, owner actions and exit criteria for each step are in [docs/build-plan - [ ] Protect `.github` main: CODEOWNERS review, 1 approval, no force push, no deletion. CODEOWNERS is the Contributors team, which needs write access to the repo. Org admins may bypass through a pull request only. - [ ] Also on `.github` main: dismiss stale approvals on push, and require the `test.yml` check (the job `unit-tests`). - [ ] Anthropic Console: dedicated workspace with a hard monthly spend limit, service account, federation rule as in "Auth and cost" (pattern mode, ID-form subject prefix, scope `workspace:developer`, the review workspace only, the placeholder SHA until phase 3; V26). Done 2026-10-27: workspace `claude-review`, service account `claude-review`, rule R1 `claude-review-prod-r1` created at the placeholder SHA and accepted by the Console (V26 a partial); no read-back is recorded, so this item stays open. -- [ ] Anthropic Console, eval part: the eval federation rule (V27) on the test workspace `claude-review-p0-test`, separate from the review workspace (P2-1). A GitHub environment `eval` on `.github` that requires the owner's approval for every run, allows any branch, and has "prevent self-review" on (off during phases 2 to 4 by owner decision of 2026-10-26; on since 2026-11-01). No API key. +- [ ] Anthropic Console, eval part: the eval federation rule (V27) on the test workspace `claude-review-p0-test`, separate from the review workspace (P2-1). GitHub environments on `.github` (P6-4): `eval` with a main-only branch policy and no required reviewer (since 2026-11-04); `eval-test` with the owner as required reviewer and "prevent self-review" on, for every other ref. [ ] An `eval-test` federation rule like the eval rule, with the exact claim `environment: "eval-test"`, and its ID in `evals.py`. No API key. - [ ] Org Actions setting: default `GITHUB_TOKEN` permissions read-only. - [ ] Org setting: require 2FA for all members. A stolen maintainer account bypasses everything in this design. - [ ] Enable Dependabot alerts with npm malware detection on active repos ([changelog](https://github.blog/changelog/2026-03-17-dependabot-now-detects-malware-in-npm-dependencies/)). @@ -865,7 +885,7 @@ The tasks, owner actions and exit criteria for each step are in [docs/build-plan - [ ] Each target repo, before it joins a ruleset that requires the workflow: the immutable OIDC subject is set and read back, and the trust policy of every other OIDC user in that repo accepts it (see "Pilot and enforcement"). - [ ] Pilot ruleset: only the workflows rule, the release SHA, the pilot repos, bypass for the Maintain and Admin repository roles and org admins in `pull_request` mode. Read it back (`repository_id`, path, `sha`, no `ref`, and the role `actor_id` values), enable it, then list the open PRs again and start a run on each (rollout step 5). - [x] Baseline ruleset 25787833: rules and bypass as in the diagram in "The org rulesets", `~ALL`, read back after each target change (done 2026-11-01, P5-1). -- [ ] Review ruleset 26132750: bypass list org admins only in `pull_request` mode, renamed `claude-review`, read back (rollout step 8). +- [ ] Follow-up, not a phase 6 item (P6-5): review ruleset 26132750 bypass list org admins only in `pull_request` mode, renamed `claude-review`, read back, when someone asks the owner for a gate. - [x] Before org-wide enforcement: record the owner's acceptance of the residuals in "Which run counts", including the cross-PR effect of one head SHA. Done 2026-11-01 (P5-8). Repo writers and admins can cancel, re-run and delete runs; no setting in this design limits that. ## Verify before building @@ -974,7 +994,7 @@ K: owner decisions, 2026-10-25. Where a K- line changes an earlier line, the K- - K-1. The bot does not submit reviews: no `APPROVE` and no `REQUEST_CHANGES`, only `COMMENT` reviews and the sticky summary. Reasons: CLAUDE.md forbids it. An LLM approval could count toward required approvals and lower risk, which breaks "only code lowers risk". A requested-changes review needs a dismissal to clear, and needs an org setting that lets Actions review PRs. The failing required check already gives the signal. - K-2. One build, real failure, bypass during the pilot. The advisory release, the enforcing release, the mode constant and V22 (c) are removed. There is one build of the central workflow, and its gate job fails for real when the result is `fail`. During the pilot, "advisory" means the failure is not enforced: a separate pilot ruleset with only the workflows rule targets the pilot repos, and its bypass list holds the Maintain and Admin repository roles and org admins in `pull_request` mode. Maintainers see the red X and can merge with the bypass, which GitHub records. Org-wide enforcement enables the org ruleset on the supported repos, then moves the pilot repos to it one at a time, then deletes the pilot ruleset. No repository role keeps a bypass. Ruleset Evaluate mode is Enterprise only, so on Team the workflows rule is either enforced or off. V25 tests the bypass and records the role `actor_id` values, which the docs do not list. The owner's existing disabled org ruleset is fixed before it is enabled (see "Admin checklist"). Fix round 1: each pilot repo gets its own approval protection first; no repo is in two rulesets that require the workflow; evals use their own federation rule and a workspace separate from production (the test workspace, P2-1). -- K-3. Evals run on any branch, with owner approval. The `eval` environment allows any branch and requires the owner as reviewer for every run, so evals can run on a PR before merge. The eval federation rule stays bound to the environment claim. Passing evals gate the merge of a prompt or model change, and any ruleset pin move (this restores the merge gate). The approver checks that the PR does not change `eval.yml`, the eval code, the prompts, the fixtures or the federation setup in a harmful way, and that `test.yml` passed. The approval is the control. Residual risk: an approver who approves without reading. +- K-3. Evals run on any branch, with owner approval (P6-4 splits this into `eval` for `main` and `eval-test` for other refs). The `eval` environment allows any branch and requires the owner as reviewer for every run, so evals can run on a PR before merge. The eval federation rule stays bound to the environment claim. Passing evals gate the merge of a prompt or model change, and any ruleset pin move (this restores the merge gate). The approver checks that the PR does not change `eval.yml`, the eval code, the prompts, the fixtures or the federation setup in a harmful way, and that `test.yml` passed. The approval is the control. Residual risk: an approver who approves without reading. - K-4. Eval exception to the no-execute rule. The rule "never check out, install, build or execute PR content" is for the review workflow. One exception, approved by the owner: evals of PRs to `OperationCode/.github` may run that PR's code, only after an owner approves the `eval` environment run. "Prevent self-review" is on (since 2026-11-01); during phases 2 to 4 it was off and the coordinator's approval stood in for the owner's, under the owner's delegation. `CLAUDE.md` states the same exception. The merge gate from K-3 is manual: no required check enforces it, so the owner holds the merge until the evals pass. The approver check now also covers the prompts and the fixtures, and that `test.yml` passed. - K-5. Artifact digest (review W1 Important 1). The producer computes the SHA-256 of the envelope file bytes in a trusted step and exports it next to `artifact-id`. The consumer checks the extracted file against it. The `artifact-digest` of `actions/upload-artifact` is the digest of the zip and cannot be checked from the extracted file, so it is not the gate check. The same entry sets the envelope size caps (16 MiB `bundle`, 1 MiB `pass`, one file per artifact) and the rule that `payload` is `null` unless `status` is `ok`. - K-6. Metadata citations and redacted quotes (review W7 Important 1 and 2). An injection only in the PR title or body had no file line to cite, and a line that holds only a credential had no allowed quote. The title and body are cited as the pseudo-files `/.pr/title` and `/.pr/body`, with `side: "head"` and line numbers in the rendered, redacted text. A leading `/` is never a git tree path, so no repo file can collide. These findings are checked against the rendered text, never go inline, and are escaped like all untrusted text. The citation check compares quotes with the same redacted text that the model saw. A quote may be exactly one redaction marker when the line has no other text, and it verifies when that marker is on that line. A quote never rebuilds a credential, and the finding stays. The supply-chain rule that an explanation says what the code does when it runs applies only to executable changes. @@ -1008,8 +1028,8 @@ P0: results of the phase 0 platform tests, 2026-10-25 and 2026-10-26 (UTC), in [ V10: decisions from the V10 egress run, 2026-10-27 (UTC). Where a line below changes an earlier line, it wins. -- K-14. Egress in audit mode (ruling on V10; the owner delegated the decision). Block mode passed V10, but 2 of 24 block-mode jobs hung in "Complete job" (2 of 6 runs). Each hang leaves the run in progress for about 15 minutes, then fails closed until someone re-runs it. V10 hit this in 2 of 6 block-mode runs. That is not a measured rate on real PRs. The symptoms match the open harden-runner [issue 708](https://github.com/step-security/harden-runner/issues/708); the runner region and the cause are not confirmed. No allowlist entry fixed it, and v2.22.2, the latest release, has no fix. Audit mode still records every endpoint. So every harden-runner step uses `egress-policy: audit`, keeps the list that V10 measured for its job, and has no `disable-sudo-and-containers`. Moving to block mode is `egress-policy: block` plus `disable-telemetry: true` in each job; the unit tests keep the lists in place, and the `release` test holds the central lists to the V10 lists. Residuals: egress is recorded, not blocked; harden-runner sends telemetry to StepSecurity from every run (the owner accepted this residual); the `test.yml` and `eval.yml` lists are not measured. Before org-wide enforcement (build plan, phase 5), revisit block mode: when a harden-runner release fixes [issue 708](https://github.com/step-security/harden-runner/issues/708), pin it, measure the `test.yml` and `eval.yml` lists, re-test block mode, then switch. -- K-15. `setup-uv` age exception (owner). The owner overrode the 7-day age rule for `astral-sh/setup-uv` v10.3.2 (`07a047e608db84b3d8c06cd9a7c7dcc9c355c18e`, released 2026-10-22), a security update with the same inputs as v10.3.0, so V10 ran with it and every workflow pins it. It is the only exception; later updates follow the rule. The age rule is now 2 days (P5-5). +- K-14. Egress in audit mode (ruling on V10; the owner delegated the decision). Block mode passed V10, but 2 of 24 block-mode jobs hung in "Complete job" (2 of 6 runs). Each hang leaves the run in progress for about 15 minutes, then fails closed until someone re-runs it. V10 hit this in 2 of 6 block-mode runs. That is not a measured rate on real PRs. The symptoms match the open harden-runner [issue 708](https://github.com/step-security/harden-runner/issues/708); the runner region and the cause are not confirmed. No allowlist entry fixed it, and v2.22.2, the latest release then, had no fix. v2.22.3 (2026-10-27, pinned 2026-11-05) fixes part of it: its revert of GitHub's DNS experiment depends on a feature flag fetched at run time, and the safe-by-default fix is promised for a later release ([.github issue 24](https://github.com/OperationCode/.github/issues/24) tracks the block-mode re-test). Audit mode stays. Audit mode still records every endpoint. So every harden-runner step uses `egress-policy: audit`, keeps the list that V10 measured for its job, and has no `disable-sudo-and-containers`. Moving to block mode is `egress-policy: block` plus `disable-telemetry: true` in each job; the unit tests keep the lists in place, and the `release` test holds the central lists to the V10 lists (plus `releases.astral.sh` for setup-uv v10.3.3, added 2026-11-05, not measured). Residuals: egress is recorded, not blocked; harden-runner sends telemetry to StepSecurity from every run (the owner accepted this residual); the `test.yml` and `eval.yml` lists are not measured. Before org-wide enforcement (build plan, phase 5), revisit block mode: when a harden-runner release fixes [issue 708](https://github.com/step-security/harden-runner/issues/708), pin it, measure the `test.yml` and `eval.yml` lists, re-test block mode, then switch. +- K-15. `setup-uv` age exception (owner). The owner overrode the 7-day age rule for `astral-sh/setup-uv` v10.3.2 (`07a047e608db84b3d8c06cd9a7c7dcc9c355c18e`, released 2026-10-22), a security update with the same inputs as v10.3.0, so V10 ran with it and every workflow pins it. It is the only exception; later updates follow the rule. The age rule is now 2 days (P5-5). On 2026-11-05 every workflow moved to v10.3.3 (`68559e99cc807c1eb7e9238fd43a15bba460eec2`, released 2026-10-28) under the 2-day rule; it fetches the uv version manifest from the Astral mirror `releases.astral.sh` first, so that host joined the allowlists of the jobs that run setup-uv (not measured). P2: phase 2 rulings, 2026-10-27 (UTC). Where a P2- line changes an earlier line, it wins. @@ -1062,3 +1082,6 @@ P6: phase 6, org-wide enforcement, from 2026-11-04 (owner and coordinator). Wher - P6-1. Summary comment format (from the PR #23 comment review and P5-9). Five display changes in the sticky summary; the gate does not change. (1) One row per cause: the dependency-review job result and step outcome reasons are one row, passes that failed for the same reason share one row, in the findings table and in the record's reasons table, which still lists every detail. (2) Repo setting rows: a dependency-review failure is labeled as a repo setting (Dependency graph off) that only a repo admin can fix only on positive evidence from GitHub's error message (P6-2), and the author is not told to fix it. (3) File paths and code-made reason details show as plain text in code spans, not as backslash-escaped prose, with the same cleaning, redaction and reference breaking. (4) The token usage table collapses to one line when no pass recorded any count. (5) An informational row counts the PR's commits that are not verified-signed, with a link to GitHub's commit signing guide; no row at zero. Details in "Summary comment". The commit list and the dependency probe are read by the gate job (no Anthropic credential) for display only: they are not gate inputs, never reach Claude, and cannot change the result, a reason, the waiver line or the exit code. Unit tests check each item and that the gate result is the same with and without them. - P6-2. Review of P6-1 (three Important findings, fixed before release). (1) The unsigned-commit row could push a summary near the 64,000-character limit into the fail-closed overflow notice, so unsigned commits could fail a passing run: the summary is now rendered without the row first, the row stays only when it changes no cut, and every read error gives no row. (2) A 403 on the dependency comparison alone does not prove the Dependency graph is off (private repo without Advanced Security, fork, permission, secondary rate limit): the setting label now needs GitHub's error message to say the graph is disabled; otherwise the row stays neutral. (3) A file name with a backslash before a pipe could end a table cell early in some parsers: a backslash run before a pipe is now split from it with a zero-width space. Unit tests cover the near-limit case, read errors, 14 probe answers that are not evidence, and hostile names (backslash, pipe, backtick, newline, bidi) under two table-splitting rules. +- P6-3. Comment fixes from the first live run at `364058d` (claude-review-test PR 4, 2026-11-04; display only, the gate does not change). (1) The TIP callout said "Nothing to do for this review" while the unsigned-commit row said the merge box blocks: with the row, the TIP now names the signing step. (2) The row said "1 of 1 commits ... is": it now says "commit" when the PR has one commit. (3) The Dependency graph setting match ignored negation and could join two clauses: it now needs one narrow affirmative phrase, "dependency graph", an optional "for this repository", one verb ("is", "was", "has been"), an optional "currently", then "disabled", "not enabled" or "turned off", with nothing else between them (review AC, I2: "is neither disabled nor turned off" passed the first fix). (4) The error-body read in `collect.default_transport` did not catch `http.client.IncompleteRead`: it now catches `http.client.HTTPException` like the other read errors; the body is empty and the status still fails the caller. (5) "Never approves" shows once in the pass comment: already true at `364058d` (a unit test checks it), no change. +- P6-4. Eval environment split (owner and coordinator, 2026-11-04). Release evals run on `main`, which holds only reviewed code, so they need no approval; evals of other refs can run unreviewed code with the eval credential, so they need the owner's approval. Step 1 (owner, 2026-11-04): the `eval` environment got a main-only branch policy and lost its required reviewer and "prevent self-review"; admin bypass stays on. Until step 2, branch evals are refused by that policy. Step 2 (this change): `eval.yml` picks `eval` on `refs/heads/main` and `eval-test` on any other ref, with an exact shell test in a select job with no permissions (an Actions expression compares without case, review AC I1), and passes the choice to `evals.py` in `EVAL_ENVIRONMENT`; `evals.py` holds the federation IDs per environment and fails closed on any other value and on an environment whose rule is not set. Open owner steps: create the `eval-test` environment (required reviewer the owner, "prevent self-review" on), create its federation rule on the test workspace with the exact claim `environment: "eval-test"`, then set the rule ID in `evals.py` (a PR) and test it as V27 (an approved run gets a token, a run in another environment or with no environment gets none). +- P6-5. Phase 6 scope (owner, 2026-11-05). (1) Phase 4 exit evidence: 5 real PRs reviewed (was 10; owner and Dependabot PRs count), 2 weekly metric reports, and 7 days with no unexplained false-positive hard block, run failure or maintainer bypass without a reason. (2) `START_HERE` is not a review target: it will be archived after its content moves into `.github` (issue #27 in this repo). The review targets stay the 4 pilot repos. (3) The bypass switch is deferred to follow-up work: the review ruleset keeps the pilot bypass, and the review stays an advisory signal for maintainers, not a hard gate, until someone asks the owner for a gate. The name change to `claude-review` waits for that switch. (4) Code security configurations: the configuration for public repos (enforced on the active public repos, default for new public repos) and the dependency configuration (enforced, default for new private and internal repos) are live and read back; one planned change to the dependency configuration is skipped by owner decision. Details are kept in the owner's records. (5) The phase 6 final acceptance test checks this shape (build plan, phase 6 exit); the gate-shape checks (org-admin-only bypass, the renamed ruleset, a live check that a Maintain user cannot bypass) move to the follow-up. diff --git a/docs/supported-repos.md b/docs/supported-repos.md index bcdd3fa..2d51368 100644 --- a/docs/supported-repos.md +++ b/docs/supported-repos.md @@ -10,7 +10,6 @@ Every active repo also gets the baseline ruleset 25787833 (`~ALL`), whether it i | `back-end` | 182584277 | `main` | target | 2026-10-29 | | `operationcode_infra` | 107068150 | `main` | target | 2026-10-29 | | `operationcode-pybot` | 146210805 | `master` | target | 2026-10-29 | -| `START_HERE` | 82613670 | `master` | candidate | | ## Excluded active repos @@ -19,6 +18,7 @@ Every active repo also gets the baseline ruleset 25787833 (`~ALL`), whether it i | `.github` | 1432342572 | Never a target: it holds the central workflow, and a ruleset that requires a workflow from `.github` and also targets `.github` can deadlock. | | `claude-review-test` | 1432457406 | Test repo. Only the scratch test ruleset 25835887 requires the workflow there. | | `claude-review-p0-source` | 1433591286 | Test source repo for the acceptance tests. No review ruleset. | +| `START_HERE` | 82613670 | Not a review target: it will be archived after its content moves into `.github` ([issue #27](https://github.com/OperationCode/.github/issues/27); spec P6-5). | Private repos are excluded (not supported): v1 supports public repos only. diff --git a/review/src/oc_review/collect.py b/review/src/oc_review/collect.py index 53dceb3..fd80fe3 100644 --- a/review/src/oc_review/collect.py +++ b/review/src/oc_review/collect.py @@ -30,6 +30,7 @@ from __future__ import annotations import argparse +import http.client import json import math import os @@ -234,10 +235,11 @@ def default_transport( headers = {k.lower(): v for k, v in (exc.headers or {}).items()} # The error body, cut at the byte cap. Only a display-only probe reads # it (publish.dependency_graph_off_evidence); every other caller fails - # on the status alone. + # on the status alone. A body that cannot be read (also a cut-off + # body, http.client.IncompleteRead) is empty: the status still fails. try: body = exc.read(request.max_bytes + 1) if clock() <= deadline else b"" - except (OSError, ValueError): + except (OSError, ValueError, http.client.HTTPException): body = b"" return HttpResponse(exc.code, body or b"", request.url, headers) diff --git a/review/src/oc_review/evals.py b/review/src/oc_review/evals.py index 583b284..a6bf12d 100644 --- a/review/src/oc_review/evals.py +++ b/review/src/oc_review/evals.py @@ -19,8 +19,13 @@ - `summary_excludes`: strings that must not appear in the rendered summary. Another key, or a wrong type or value, fails the case before any call. +The workflow names its GitHub environment in `EVAL_ENVIRONMENT` (`eval` on +main, `eval-test` on any other ref). The runner refuses to start, before any +OIDC token is minted, when that value is missing or another name, or when the +federation rule of that environment is not configured. + For each case the runner calls `analyze.analyze` with the eval federation IDs -and a GitHub client on an in-memory transport (no GitHub read), writes the +of that environment and a GitHub client on an in-memory transport (no GitHub read), writes the three pass records, and computes the gate with the same code the publisher uses and synthetic trusted inputs: every job `success`, dependency review `success`, a current snapshot, no overrides. It renders the sticky summary @@ -44,17 +49,44 @@ import time import urllib.parse from collections.abc import Callable, Mapping, Sequence +from dataclasses import dataclass from pathlib import Path +from types import MappingProxyType from typing import Any from oc_review import analyze, collect, envelope, publish, snapshot, verdict -# The eval federation rule, bound to the test workspace `claude-review-p0-test` -# (never the review workspace; spec "Fixture eval credential"). They are -# separate from the production IDs in `analyze`. -EVAL_FEDERATION_RULE_ID = "fdrl_01BL4n1gQzkrroCmDZoXUePm" +# The eval federation rules, one per GitHub environment, bound to the test +# workspace `claude-review-p0-test` (never the review workspace; spec "Fixture +# eval credential"). They are separate from the production IDs in `analyze`. +# eval.yml picks the environment from the ref (main -> `eval`, any other ref -> +# `eval-test`) and passes the same choice in EVAL_ENVIRONMENT_VAR. Each rule +# binds its token to its own environment claim. EVAL_ORGANIZATION_ID = "2bc920f9-977b-43b2-870a-f840e4ce809c" -EVAL_SERVICE_ACCOUNT_ID = "svac_017eR3ohB6rP5MEAsda6U7X6" +EVAL_ENVIRONMENT_VAR = "EVAL_ENVIRONMENT" +EVAL_ENVIRONMENT_MAIN = "eval" +EVAL_ENVIRONMENT_TEST = "eval-test" + + +@dataclass(frozen=True) +class EvalRule: + """The federation rule and service account of one eval environment. A + rule ID of None means the owner has not created the rule yet: the runner + refuses to run in that environment.""" + + rule_id: str | None + service_account_id: str + + +EVAL_FEDERATION: Mapping[str, EvalRule] = MappingProxyType({ + # `eval`: runs on main only (reviewed code), no approval. + EVAL_ENVIRONMENT_MAIN: EvalRule("fdrl_01BL4n1gQzkrroCmDZoXUePm", "svac_017eR3ohB6rP5MEAsda6U7X6"), + # `eval-test`: any other ref, owner approval with prevent self-review. The + # rule is not created yet (owner step); set its ID here when it exists. + # The service account is the `eval` one; the owner may give this rule + # its own service account and change it here. + EVAL_ENVIRONMENT_TEST: EvalRule(None, "svac_017eR3ohB6rP5MEAsda6U7X6"), +}) EVAL_REPOSITORY = "OperationCode/eval-fixtures" EVAL_PR_AUTHOR = "eval-author" @@ -70,8 +102,31 @@ class EvalError(Exception): """A case that cannot run (bad fixture). The case fails; the run goes on.""" -def eval_federation() -> analyze.FederationIds: - return analyze.FederationIds(EVAL_FEDERATION_RULE_ID, EVAL_ORGANIZATION_ID, EVAL_SERVICE_ACCOUNT_ID) +class EvalConfigError(Exception): + """The run cannot start: no allowed eval environment, or its federation + rule is not configured. Every case fails closed (no OIDC token is minted).""" + + +def eval_environment(env: Mapping[str, str]) -> str: + """The eval environment the workflow chose. Only `eval` and `eval-test` + are allowed; a missing or other value fails closed.""" + name = env.get(EVAL_ENVIRONMENT_VAR) + if name not in EVAL_FEDERATION: + raise EvalConfigError( + f"{EVAL_ENVIRONMENT_VAR} must be one of {sorted(EVAL_FEDERATION)}, got {str(name)[:40]!r}" + ) + return name + + +def eval_federation(environment: str = EVAL_ENVIRONMENT_MAIN) -> analyze.FederationIds: + """The federation IDs of one eval environment. Raises EvalConfigError for + another name or a rule that is not configured.""" + rule = EVAL_FEDERATION.get(environment) + if rule is None: + raise EvalConfigError(f"unknown eval environment {str(environment)[:40]!r}") + if rule.rule_id is None: + raise EvalConfigError(f"{environment} federation rule not configured") + return analyze.FederationIds(rule.rule_id, EVAL_ORGANIZATION_ID, rule.service_account_id) def pr_transport(bundle: Mapping[str, Any], repository: str = EVAL_REPOSITORY) -> collect.Transport: @@ -341,7 +396,12 @@ def main( parser.add_argument("--out-dir", required=True) args = parser.parse_args(argv) env = os.environ if environ is None else environ - federation = federation or eval_federation() + try: + environment = eval_environment(env) + federation = federation or eval_federation(environment) + except EvalConfigError as exc: + print(f"evals: {exc}", file=sys.stderr) + return 1 out_dir = Path(args.out_dir) out_dir.mkdir(parents=True, exist_ok=True) diff --git a/review/src/oc_review/publish.py b/review/src/oc_review/publish.py index 86af2af..cc91e13 100644 --- a/review/src/oc_review/publish.py +++ b/review/src/oc_review/publish.py @@ -731,12 +731,26 @@ def read_commit_signing(api: GitHubApi, ctx: PublishContext) -> CommitSigning | # alone is not evidence: GitHub also answers 403 for a private repo without # Advanced Security, for a fork, for a missing permission and for a secondary # rate limit (and the action prints the same advice for every 403). +# Positive evidence only: a narrow affirmative phrase, "dependency graph", +# an optional "for this repository", one verb ("is", "was", "has been"), an +# optional "currently", then the state ("disabled", "not enabled", "turned +# off"). Nothing else may stand between them, so a negation ("is not +# disabled", "is neither disabled nor ..."), another clause ("is enabled but X +# is disabled") or any other word gives no evidence, and the row stays +# neutral. _GRAPH_OFF_RE = re.compile( - r"\bdependency[ -]graph\b[^.]*\b(?:is\s+)?(?:disabled|not\s+enabled|turned\s+off)\b", re.IGNORECASE + r"\bdependency[ -]graph(?:\s+for\s+this\s+repository)?\s+(?:is|was|has\s+been)\s+(?:currently\s+)?" + r"(?:disabled|not\s+enabled|turned\s+off)\b", + re.IGNORECASE, ) _GRAPH_OFF_EXCLUDE_RE = re.compile(r"rate[ -]limit|advanced security|abuse", re.IGNORECASE) +def _says_graph_off(message: str) -> bool: + """True when the message has the affirmative phrase `_GRAPH_OFF_RE`.""" + return bool(_GRAPH_OFF_RE.search(message)) + + def dependency_graph_off_evidence(resp: collect.HttpResponse | None) -> bool: """True only on positive evidence: HTTP 403, no rate limit headers, and a JSON error `message` from GitHub that says the Dependency graph is @@ -754,7 +768,7 @@ def dependency_graph_off_evidence(resp: collect.HttpResponse | None) -> bool: message = doc.get("message") if isinstance(doc, Mapping) else None if not isinstance(message, str) or len(message) > 1000: return False - return bool(_GRAPH_OFF_RE.search(message)) and not _GRAPH_OFF_EXCLUDE_RE.search(message) + return _says_graph_off(message) and not _GRAPH_OFF_EXCLUDE_RE.search(message) def read_dependency_graph_off(api: GitHubApi, ctx: PublishContext, snap: snapshot.Snapshot | None) -> bool: @@ -1515,8 +1529,9 @@ def _signing_row(facts: SummaryFacts) -> list[_Row]: if c is None or c.unsigned == 0: return [] of = f"{c.unsigned} of the first {c.total}" if c.capped else f"{c.unsigned} of {c.total}" + noun = "commit" if c.total == 1 else "commits" claim = ( - f"**Unsigned commits:** {of} commits in this PR {'is' if c.unsigned == 1 else 'are'} not " + f"**Unsigned commits:** {of} {noun} in this PR {'is' if c.unsigned == 1 else 'are'} not " "verified-signed. The org requires signed commits, so the merge box blocks until every commit is " f"signed. [How to sign commits]({SIGNING_GUIDE_URL})" ) @@ -1554,6 +1569,11 @@ def _model_rows(rows: list[_Row]) -> list[_Row]: MISSION_LINE = "*Mission: Review the code. Protect the branch. Deploy the future.*" APPROVAL_NOTE = "*Claude of Duty never approves a PR. A human reviewer gives the final approval.*" +# The TIP callout's last step. With the unsigned-commit row, "nothing to do" +# would contradict the row (the merge box blocks until every commit is +# signed), so the callout names the signing step instead. Display only. +NOTHING_TO_DO = "Nothing to do for this review." +SIGNING_STEP = "One step is left: sign the unsigned commits (see the Commits row)." def _status_heading(result: verdict.GateResult, model_rows: list[_Row], rows: list[_Row]) -> str: @@ -1590,9 +1610,10 @@ def _callout( setting = facts.dependency_graph_off and _dependency_failed(result) setting_also = " A repo setting also blocks the dependency review; only a repo admin can fix it (see its row)." if result.status != verdict.FAIL: + step = SIGNING_STEP if _signing_row(facts) else NOTHING_TO_DO if result.override_applied: login = _esc("login", result.valid_overrides[0].author_login) if result.valid_overrides else "a maintainer" - lines = [f"**A maintainer waiver was applied by {login}. Findings stay listed.** Nothing to do for this review."] + lines = [f"**A maintainer waiver was applied by {login}. Findings stay listed.** {step}"] elif model: one = len(model) == 1 lines = [ @@ -1601,7 +1622,7 @@ def _callout( ] return "\n".join(f"> {x}" for x in ("[!NOTE]", *lines)) else: - lines = ["**No findings. This review found no reason to block.** Nothing to do for this review."] + lines = [f"**No findings. This review found no reason to block.** {step}"] lines.append("The merge box shows whether the other required checks pass.") return "\n".join(f"> {x}" for x in ("[!TIP]", *lines)) @@ -2233,10 +2254,13 @@ def render_summary( def _without_signing_row(body: str, facts: SummaryFacts) -> str: """`body` with the signing row (and a findings table header that only it - needed) taken out: equal to the body without the row when the row changed - nothing else.""" + needed) taken out, and the TIP callout's signing step put back to "nothing + to do": equal to the body without the row when the row changed nothing + else. The callout comes before any untrusted text, so the first + `SIGNING_STEP` is the callout's.""" [r] = _signing_row(facts) out = body.replace("\n" + _row(r.severity, r.effect, r.where, r.claim, r.found_by), "", 1) + out = out.replace(f"** {SIGNING_STEP}\n", f"** {NOTHING_TO_DO}\n", 1) if out.count(_FINDINGS_HEADER) == 1 and out.find(_FINDINGS_HEADER + "\n|") == -1: out = out.replace("\n" + _FINDINGS_HEADER, "", 1) return out diff --git a/review/tests/unit/fixtures/workflows/edits.yml b/review/tests/unit/fixtures/workflows/edits.yml index 8a2758f..76490bc 100644 --- a/review/tests/unit/fixtures/workflows/edits.yml +++ b/review/tests/unit/fixtures/workflows/edits.yml @@ -23,12 +23,12 @@ expect: [unpinned-uses, dependency-review] - id: uses-local-action-eval file: eval.yml - old: "uses: astral-sh/setup-uv@07a047e608db84b3d8c06cd9a7c7dcc9c355c18e" + old: "uses: astral-sh/setup-uv@68559e99cc807c1eb7e9238fd43a15bba460eec2" new: "uses: ./.github/actions/setup-uv" expect: unpinned-uses - id: uses-docker file: eval.yml - old: "uses: astral-sh/setup-uv@07a047e608db84b3d8c06cd9a7c7dcc9c355c18e" + old: "uses: astral-sh/setup-uv@68559e99cc807c1eb7e9238fd43a15bba460eec2" new: "uses: docker://ghcr.io/astral-sh/uv:0.13.1" expect: unpinned-uses - id: uses-no-version-comment @@ -55,7 +55,7 @@ # --- harden-runner, timeouts, scripts (every workflow) --- - id: harden-runner-not-first file: claude-review.yml - old: " steps:\n - name: Harden the runner\n uses: step-security/harden-runner@dc2769abcc0fcfbadfc1df54081c1608579ef6b2 # v2.22.2, released 2026-10-19\n with:\n # Audit until https://github.com/step-security/harden-runner/issues/708 is fixed (spec K-14).\n egress-policy: audit\n allowed-endpoints: >\n api.github.com:443\n - name: Dependency review" + old: " steps:\n - name: Harden the runner\n uses: step-security/harden-runner@46b06cea07f9e64621fdf066ba77ac6915163d43 # v2.22.3, released 2026-10-27\n with:\n # Audit until https://github.com/step-security/harden-runner/issues/708 is fixed (spec K-14).\n egress-policy: audit\n allowed-endpoints: >\n api.github.com:443\n - name: Dependency review" new: " steps:\n - name: Dependency review" expect: harden-runner - id: harden-runner-block-mode @@ -70,7 +70,7 @@ expect: harden-runner - id: harden-runner-list-missing file: test.yml - old: " allowed-endpoints: >\n github.com:443\n raw.githubusercontent.com:443\n release-assets.githubusercontent.com:443\n files.pythonhosted.org:443\n results-receiver.actions.githubusercontent.com:443\n" + old: " allowed-endpoints: >\n github.com:443\n raw.githubusercontent.com:443\n releases.astral.sh:443\n release-assets.githubusercontent.com:443\n files.pythonhosted.org:443\n results-receiver.actions.githubusercontent.com:443\n" new: "" expect: harden-runner - id: harden-runner-list-empty @@ -173,12 +173,12 @@ - id: extra-job-id-token-with-write file: eval.yml old: "jobs:\n" - new: "jobs:\n extra:\n runs-on: ubuntu-24.04\n timeout-minutes: 30\n permissions:\n contents: write\n id-token: write\n steps:\n - uses: step-security/harden-runner@dc2769abcc0fcfbadfc1df54081c1608579ef6b2 # v2.22.2, released 2026-10-19\n with:\n egress-policy: audit\n allowed-endpoints: api.github.com:443\n - run: echo hi\n" + new: "jobs:\n extra:\n runs-on: ubuntu-24.04\n timeout-minutes: 30\n permissions:\n contents: write\n id-token: write\n steps:\n - uses: step-security/harden-runner@46b06cea07f9e64621fdf066ba77ac6915163d43 # v2.22.3, released 2026-10-27\n with:\n egress-policy: audit\n allowed-endpoints: api.github.com:443\n - run: echo hi\n" expect: credential-separation - id: extra-job-write-all file: eval.yml old: "jobs:\n" - new: "jobs:\n extra:\n runs-on: ubuntu-24.04\n timeout-minutes: 30\n permissions: write-all\n steps:\n - uses: step-security/harden-runner@dc2769abcc0fcfbadfc1df54081c1608579ef6b2 # v2.22.2, released 2026-10-19\n with:\n egress-policy: audit\n allowed-endpoints: api.github.com:443\n - run: echo hi\n" + new: "jobs:\n extra:\n runs-on: ubuntu-24.04\n timeout-minutes: 30\n permissions: write-all\n steps:\n - uses: step-security/harden-runner@46b06cea07f9e64621fdf066ba77ac6915163d43 # v2.22.3, released 2026-10-27\n with:\n egress-policy: audit\n allowed-endpoints: api.github.com:443\n - run: echo hi\n" expect: credential-separation - id: collect-id-token file: claude-review.yml @@ -213,7 +213,7 @@ - id: job-added file: claude-review.yml old: " oc-claude-review:\n" - new: " extra:\n runs-on: ubuntu-24.04\n timeout-minutes: 10\n permissions:\n contents: read\n steps:\n - uses: step-security/harden-runner@dc2769abcc0fcfbadfc1df54081c1608579ef6b2 # v2.22.2, released 2026-10-19\n with:\n egress-policy: audit\n allowed-endpoints: api.github.com:443\n - run: echo hi\n oc-claude-review:\n" + new: " extra:\n runs-on: ubuntu-24.04\n timeout-minutes: 10\n permissions:\n contents: read\n steps:\n - uses: step-security/harden-runner@46b06cea07f9e64621fdf066ba77ac6915163d43 # v2.22.3, released 2026-10-27\n with:\n egress-policy: audit\n allowed-endpoints: api.github.com:443\n - run: echo hi\n oc-claude-review:\n" expect: jobs - id: gate-needs-short file: claude-review.yml @@ -610,6 +610,51 @@ expect: eval-permissions - id: eval-duplicate-key file: eval.yml - old: " environment: eval\n" - new: " environment: eval\n environment: other\n" + old: " environment: ${{ needs.select-environment.outputs.environment }}\n" + new: " environment: ${{ needs.select-environment.outputs.environment }}\n environment: other\n" expect: parse +- id: eval-environment-literal + file: eval.yml + old: " environment: ${{ needs.select-environment.outputs.environment }}\n" + new: " environment: eval\n" + expect: eval-environment +- id: eval-environment-case-insensitive-expression + file: eval.yml + old: " environment: ${{ needs.select-environment.outputs.environment }}\n" + new: " environment: ${{ github.ref == 'refs/heads/main' && 'eval' || 'eval-test' }}\n" + expect: eval-environment +- id: eval-environment-var-differs + file: eval.yml + old: " EVAL_ENVIRONMENT: ${{ needs.select-environment.outputs.environment }}\n" + new: " EVAL_ENVIRONMENT: eval\n" + expect: eval-environment +- id: eval-environment-var-removed + file: eval.yml + old: " EVAL_ENVIRONMENT: ${{ needs.select-environment.outputs.environment }}\n" + new: "" + expect: eval-environment +- id: eval-select-case-insensitive + file: eval.yml + old: "if [ \"$GITHUB_REF\" = \"refs/heads/main\" ]; then" + new: "if [ \"${GITHUB_REF,,}\" = \"refs/heads/main\" ]; then" + expect: eval-environment +- id: eval-select-id-token + file: eval.yml + old: " timeout-minutes: 5\n permissions: {}\n" + new: " timeout-minutes: 5\n permissions:\n id-token: write\n" + expect: eval-environment +- id: eval-select-timeout + file: eval.yml + old: " timeout-minutes: 5\n" + new: " timeout-minutes: 30\n" + expect: timeout +- id: eval-needs-removed + file: eval.yml + old: " needs: select-environment\n" + new: "" + expect: eval-environment +- id: eval-test-environment-in-test-yml + file: test.yml + old: " timeout-minutes: 10\n" + new: " timeout-minutes: 10\n environment: eval-test\n" + expect: [test-workflow, eval-environment, eval-permissions] diff --git a/review/tests/unit/fixtures/workflows/eval-boundary/contents-missing/eval.yml b/review/tests/unit/fixtures/workflows/eval-boundary/contents-missing/eval.yml index eab79e8..8570ff5 100644 --- a/review/tests/unit/fixtures/workflows/eval-boundary/contents-missing/eval.yml +++ b/review/tests/unit/fixtures/workflows/eval-boundary/contents-missing/eval.yml @@ -2,9 +2,26 @@ on: workflow_dispatch permissions: {} jobs: + select-environment: + runs-on: ubuntu-24.04 + timeout-minutes: 5 + permissions: {} + outputs: + environment: ${{ steps.select.outputs.environment }} + steps: + - id: select + run: | + if [ "$GITHUB_REF" = "refs/heads/main" ]; then + echo "environment=eval" >> "$GITHUB_OUTPUT" + else + echo "environment=eval-test" >> "$GITHUB_OUTPUT" + fi evals: + needs: select-environment runs-on: ubuntu-24.04 - environment: eval + environment: ${{ needs.select-environment.outputs.environment }} + env: + EVAL_ENVIRONMENT: ${{ needs.select-environment.outputs.environment }} permissions: id-token: write steps: diff --git a/review/tests/unit/fixtures/workflows/eval-boundary/eval-case-insensitive-expression/eval.yml b/review/tests/unit/fixtures/workflows/eval-boundary/eval-case-insensitive-expression/eval.yml new file mode 100644 index 0000000..cc8b6b8 --- /dev/null +++ b/review/tests/unit/fixtures/workflows/eval-boundary/eval-case-insensitive-expression/eval.yml @@ -0,0 +1,29 @@ +# Bad: an Actions expression compares without case, so refs/heads/MAIN would select eval. +on: workflow_dispatch +permissions: {} +jobs: + select-environment: + runs-on: ubuntu-24.04 + timeout-minutes: 5 + permissions: {} + outputs: + environment: ${{ steps.select.outputs.environment }} + steps: + - id: select + run: | + if [ "$GITHUB_REF" = "refs/heads/main" ]; then + echo "environment=eval" >> "$GITHUB_OUTPUT" + else + echo "environment=eval-test" >> "$GITHUB_OUTPUT" + fi + evals: + needs: select-environment + runs-on: ubuntu-24.04 + environment: ${{ github.ref == 'refs/heads/main' && 'eval' || 'eval-test' }} + env: + EVAL_ENVIRONMENT: ${{ github.ref == 'refs/heads/main' && 'eval' || 'eval-test' }} + permissions: + contents: read + id-token: write + steps: + - run: echo evals diff --git a/review/tests/unit/fixtures/workflows/eval-boundary/eval-env-var-literal/eval.yml b/review/tests/unit/fixtures/workflows/eval-boundary/eval-env-var-literal/eval.yml new file mode 100644 index 0000000..d09d1fc --- /dev/null +++ b/review/tests/unit/fixtures/workflows/eval-boundary/eval-env-var-literal/eval.yml @@ -0,0 +1,29 @@ +# Bad: EVAL_ENVIRONMENT must come from the same expression as the environment. +on: workflow_dispatch +permissions: {} +jobs: + select-environment: + runs-on: ubuntu-24.04 + timeout-minutes: 5 + permissions: {} + outputs: + environment: ${{ steps.select.outputs.environment }} + steps: + - id: select + run: | + if [ "$GITHUB_REF" = "refs/heads/main" ]; then + echo "environment=eval" >> "$GITHUB_OUTPUT" + else + echo "environment=eval-test" >> "$GITHUB_OUTPUT" + fi + evals: + needs: select-environment + runs-on: ubuntu-24.04 + environment: ${{ needs.select-environment.outputs.environment }} + env: + EVAL_ENVIRONMENT: eval + permissions: + contents: read + id-token: write + steps: + - run: echo evals diff --git a/review/tests/unit/fixtures/workflows/eval-boundary/eval-env-var-missing/eval.yml b/review/tests/unit/fixtures/workflows/eval-boundary/eval-env-var-missing/eval.yml new file mode 100644 index 0000000..260705e --- /dev/null +++ b/review/tests/unit/fixtures/workflows/eval-boundary/eval-env-var-missing/eval.yml @@ -0,0 +1,27 @@ +# Bad: no EVAL_ENVIRONMENT, so evals.py cannot pick the rule of the environment. +on: workflow_dispatch +permissions: {} +jobs: + select-environment: + runs-on: ubuntu-24.04 + timeout-minutes: 5 + permissions: {} + outputs: + environment: ${{ steps.select.outputs.environment }} + steps: + - id: select + run: | + if [ "$GITHUB_REF" = "refs/heads/main" ]; then + echo "environment=eval" >> "$GITHUB_OUTPUT" + else + echo "environment=eval-test" >> "$GITHUB_OUTPUT" + fi + evals: + needs: select-environment + runs-on: ubuntu-24.04 + environment: ${{ needs.select-environment.outputs.environment }} + permissions: + contents: read + id-token: write + steps: + - run: echo evals diff --git a/review/tests/unit/fixtures/workflows/eval-boundary/eval-input-expression/eval.yml b/review/tests/unit/fixtures/workflows/eval-boundary/eval-input-expression/eval.yml new file mode 100644 index 0000000..4f3690c --- /dev/null +++ b/review/tests/unit/fixtures/workflows/eval-boundary/eval-input-expression/eval.yml @@ -0,0 +1,33 @@ +# Bad: an environment from a dispatch input lets the caller pick it. +on: + workflow_dispatch: + inputs: + target: + type: string +permissions: {} +jobs: + select-environment: + runs-on: ubuntu-24.04 + timeout-minutes: 5 + permissions: {} + outputs: + environment: ${{ steps.select.outputs.environment }} + steps: + - id: select + run: | + if [ "$GITHUB_REF" = "refs/heads/main" ]; then + echo "environment=eval" >> "$GITHUB_OUTPUT" + else + echo "environment=eval-test" >> "$GITHUB_OUTPUT" + fi + evals: + needs: select-environment + runs-on: ubuntu-24.04 + environment: ${{ inputs.target }} + env: + EVAL_ENVIRONMENT: ${{ inputs.target }} + permissions: + contents: read + id-token: write + steps: + - run: echo evals diff --git a/review/tests/unit/fixtures/workflows/eval-boundary/eval-literal-eval-test/eval.yml b/review/tests/unit/fixtures/workflows/eval-boundary/eval-literal-eval-test/eval.yml new file mode 100644 index 0000000..5ad1fbb --- /dev/null +++ b/review/tests/unit/fixtures/workflows/eval-boundary/eval-literal-eval-test/eval.yml @@ -0,0 +1,29 @@ +# Bad: a literal eval-test; the environment must come from the fixed expression. +on: workflow_dispatch +permissions: {} +jobs: + select-environment: + runs-on: ubuntu-24.04 + timeout-minutes: 5 + permissions: {} + outputs: + environment: ${{ steps.select.outputs.environment }} + steps: + - id: select + run: | + if [ "$GITHUB_REF" = "refs/heads/main" ]; then + echo "environment=eval" >> "$GITHUB_OUTPUT" + else + echo "environment=eval-test" >> "$GITHUB_OUTPUT" + fi + evals: + needs: select-environment + runs-on: ubuntu-24.04 + environment: eval-test + env: + EVAL_ENVIRONMENT: ${{ needs.select-environment.outputs.environment }} + permissions: + contents: read + id-token: write + steps: + - run: echo evals diff --git a/review/tests/unit/fixtures/workflows/eval-boundary/eval-literal-eval/eval.yml b/review/tests/unit/fixtures/workflows/eval-boundary/eval-literal-eval/eval.yml new file mode 100644 index 0000000..d1d852c --- /dev/null +++ b/review/tests/unit/fixtures/workflows/eval-boundary/eval-literal-eval/eval.yml @@ -0,0 +1,29 @@ +# Bad: a literal eval; the environment must come from the fixed expression. +on: workflow_dispatch +permissions: {} +jobs: + select-environment: + runs-on: ubuntu-24.04 + timeout-minutes: 5 + permissions: {} + outputs: + environment: ${{ steps.select.outputs.environment }} + steps: + - id: select + run: | + if [ "$GITHUB_REF" = "refs/heads/main" ]; then + echo "environment=eval" >> "$GITHUB_OUTPUT" + else + echo "environment=eval-test" >> "$GITHUB_OUTPUT" + fi + evals: + needs: select-environment + runs-on: ubuntu-24.04 + environment: eval + env: + EVAL_ENVIRONMENT: ${{ needs.select-environment.outputs.environment }} + permissions: + contents: read + id-token: write + steps: + - run: echo evals diff --git a/review/tests/unit/fixtures/workflows/eval-boundary/eval-name-form/eval.yml b/review/tests/unit/fixtures/workflows/eval-boundary/eval-name-form/eval.yml new file mode 100644 index 0000000..e47ee35 --- /dev/null +++ b/review/tests/unit/fixtures/workflows/eval-boundary/eval-name-form/eval.yml @@ -0,0 +1,30 @@ +# Bad: the mapping form; the environment must be exactly the expression string. +on: workflow_dispatch +permissions: {} +jobs: + select-environment: + runs-on: ubuntu-24.04 + timeout-minutes: 5 + permissions: {} + outputs: + environment: ${{ steps.select.outputs.environment }} + steps: + - id: select + run: | + if [ "$GITHUB_REF" = "refs/heads/main" ]; then + echo "environment=eval" >> "$GITHUB_OUTPUT" + else + echo "environment=eval-test" >> "$GITHUB_OUTPUT" + fi + evals: + needs: select-environment + runs-on: ubuntu-24.04 + environment: + name: ${{ needs.select-environment.outputs.environment }} + env: + EVAL_ENVIRONMENT: ${{ needs.select-environment.outputs.environment }} + permissions: + contents: read + id-token: write + steps: + - run: echo evals diff --git a/review/tests/unit/fixtures/workflows/eval-boundary/evals-job-needs-missing/eval.yml b/review/tests/unit/fixtures/workflows/eval-boundary/evals-job-needs-missing/eval.yml new file mode 100644 index 0000000..c6f2223 --- /dev/null +++ b/review/tests/unit/fixtures/workflows/eval-boundary/evals-job-needs-missing/eval.yml @@ -0,0 +1,28 @@ +# Bad: the evals job must need the select job. +on: workflow_dispatch +permissions: {} +jobs: + select-environment: + runs-on: ubuntu-24.04 + timeout-minutes: 5 + permissions: {} + outputs: + environment: ${{ steps.select.outputs.environment }} + steps: + - id: select + run: | + if [ "$GITHUB_REF" = "refs/heads/main" ]; then + echo "environment=eval" >> "$GITHUB_OUTPUT" + else + echo "environment=eval-test" >> "$GITHUB_OUTPUT" + fi + evals: + runs-on: ubuntu-24.04 + environment: ${{ needs.select-environment.outputs.environment }} + env: + EVAL_ENVIRONMENT: ${{ needs.select-environment.outputs.environment }} + permissions: + contents: read + id-token: write + steps: + - run: echo evals diff --git a/review/tests/unit/fixtures/workflows/eval-boundary/good/eval.yml b/review/tests/unit/fixtures/workflows/eval-boundary/good/eval.yml index 37b0c23..b75a448 100644 --- a/review/tests/unit/fixtures/workflows/eval-boundary/good/eval.yml +++ b/review/tests/unit/fixtures/workflows/eval-boundary/good/eval.yml @@ -1,10 +1,29 @@ -# Good: eval.yml uses eval, and its job states exactly contents: read and id-token: write. +# Good: the select job picks eval or eval-test with the exact shell test; the evals job +# takes its output for the environment and EVAL_ENVIRONMENT and states exactly +# contents: read and id-token: write. on: workflow_dispatch permissions: {} jobs: + select-environment: + runs-on: ubuntu-24.04 + timeout-minutes: 5 + permissions: {} + outputs: + environment: ${{ steps.select.outputs.environment }} + steps: + - id: select + run: | + if [ "$GITHUB_REF" = "refs/heads/main" ]; then + echo "environment=eval" >> "$GITHUB_OUTPUT" + else + echo "environment=eval-test" >> "$GITHUB_OUTPUT" + fi evals: + needs: select-environment runs-on: ubuntu-24.04 - environment: eval + environment: ${{ needs.select-environment.outputs.environment }} + env: + EVAL_ENVIRONMENT: ${{ needs.select-environment.outputs.environment }} permissions: contents: read id-token: write diff --git a/review/tests/unit/fixtures/workflows/eval-boundary/job-block-missing/eval.yml b/review/tests/unit/fixtures/workflows/eval-boundary/job-block-missing/eval.yml index 852914c..6ead45d 100644 --- a/review/tests/unit/fixtures/workflows/eval-boundary/job-block-missing/eval.yml +++ b/review/tests/unit/fixtures/workflows/eval-boundary/job-block-missing/eval.yml @@ -4,8 +4,25 @@ permissions: contents: read id-token: write jobs: + select-environment: + runs-on: ubuntu-24.04 + timeout-minutes: 5 + permissions: {} + outputs: + environment: ${{ steps.select.outputs.environment }} + steps: + - id: select + run: | + if [ "$GITHUB_REF" = "refs/heads/main" ]; then + echo "environment=eval" >> "$GITHUB_OUTPUT" + else + echo "environment=eval-test" >> "$GITHUB_OUTPUT" + fi evals: + needs: select-environment runs-on: ubuntu-24.04 - environment: eval + environment: ${{ needs.select-environment.outputs.environment }} + env: + EVAL_ENVIRONMENT: ${{ needs.select-environment.outputs.environment }} steps: - run: echo evals diff --git a/review/tests/unit/fixtures/workflows/eval-boundary/job-read-all/eval.yml b/review/tests/unit/fixtures/workflows/eval-boundary/job-read-all/eval.yml index 8682b01..ff0a417 100644 --- a/review/tests/unit/fixtures/workflows/eval-boundary/job-read-all/eval.yml +++ b/review/tests/unit/fixtures/workflows/eval-boundary/job-read-all/eval.yml @@ -2,9 +2,26 @@ on: workflow_dispatch permissions: {} jobs: + select-environment: + runs-on: ubuntu-24.04 + timeout-minutes: 5 + permissions: {} + outputs: + environment: ${{ steps.select.outputs.environment }} + steps: + - id: select + run: | + if [ "$GITHUB_REF" = "refs/heads/main" ]; then + echo "environment=eval" >> "$GITHUB_OUTPUT" + else + echo "environment=eval-test" >> "$GITHUB_OUTPUT" + fi evals: + needs: select-environment runs-on: ubuntu-24.04 - environment: eval + environment: ${{ needs.select-environment.outputs.environment }} + env: + EVAL_ENVIRONMENT: ${{ needs.select-environment.outputs.environment }} permissions: read-all steps: - run: echo evals diff --git a/review/tests/unit/fixtures/workflows/eval-boundary/job-write-all/eval.yml b/review/tests/unit/fixtures/workflows/eval-boundary/job-write-all/eval.yml index fac53aa..6a89e7a 100644 --- a/review/tests/unit/fixtures/workflows/eval-boundary/job-write-all/eval.yml +++ b/review/tests/unit/fixtures/workflows/eval-boundary/job-write-all/eval.yml @@ -2,9 +2,26 @@ on: workflow_dispatch permissions: {} jobs: + select-environment: + runs-on: ubuntu-24.04 + timeout-minutes: 5 + permissions: {} + outputs: + environment: ${{ steps.select.outputs.environment }} + steps: + - id: select + run: | + if [ "$GITHUB_REF" = "refs/heads/main" ]; then + echo "environment=eval" >> "$GITHUB_OUTPUT" + else + echo "environment=eval-test" >> "$GITHUB_OUTPUT" + fi evals: + needs: select-environment runs-on: ubuntu-24.04 - environment: eval + environment: ${{ needs.select-environment.outputs.environment }} + env: + EVAL_ENVIRONMENT: ${{ needs.select-environment.outputs.environment }} permissions: write-all steps: - run: echo evals diff --git a/review/tests/unit/fixtures/workflows/eval-boundary/other-file-same-expression/deploy.yml b/review/tests/unit/fixtures/workflows/eval-boundary/other-file-same-expression/deploy.yml new file mode 100644 index 0000000..fe6de5d --- /dev/null +++ b/review/tests/unit/fixtures/workflows/eval-boundary/other-file-same-expression/deploy.yml @@ -0,0 +1,12 @@ +# Bad: only eval.yml may use the eval environment expression. +on: workflow_dispatch +permissions: {} +jobs: + deploy: + runs-on: ubuntu-24.04 + environment: ${{ needs.select-environment.outputs.environment }} + permissions: + contents: read + id-token: write + steps: + - run: echo deploy diff --git a/review/tests/unit/fixtures/workflows/eval-boundary/other-file-uses-eval-test-uppercase/deploy.yml b/review/tests/unit/fixtures/workflows/eval-boundary/other-file-uses-eval-test-uppercase/deploy.yml new file mode 100644 index 0000000..7438a6a --- /dev/null +++ b/review/tests/unit/fixtures/workflows/eval-boundary/other-file-uses-eval-test-uppercase/deploy.yml @@ -0,0 +1,12 @@ +# Bad: environment names are not case sensitive, so Eval-Test is eval-test (mapping form). +on: workflow_dispatch +permissions: {} +jobs: + deploy: + runs-on: ubuntu-24.04 + environment: + name: Eval-Test + permissions: + contents: read + steps: + - run: echo deploy diff --git a/review/tests/unit/fixtures/workflows/eval-boundary/other-file-uses-eval-test/deploy.yml b/review/tests/unit/fixtures/workflows/eval-boundary/other-file-uses-eval-test/deploy.yml new file mode 100644 index 0000000..38ec07d --- /dev/null +++ b/review/tests/unit/fixtures/workflows/eval-boundary/other-file-uses-eval-test/deploy.yml @@ -0,0 +1,12 @@ +# Bad: a workflow other than eval.yml uses eval-test. +on: workflow_dispatch +permissions: {} +jobs: + deploy: + runs-on: ubuntu-24.04 + environment: eval-test + permissions: + contents: read + id-token: write + steps: + - run: echo deploy diff --git a/review/tests/unit/fixtures/workflows/eval-boundary/pull-requests-write/eval.yml b/review/tests/unit/fixtures/workflows/eval-boundary/pull-requests-write/eval.yml index 6e010c8..429a99a 100644 --- a/review/tests/unit/fixtures/workflows/eval-boundary/pull-requests-write/eval.yml +++ b/review/tests/unit/fixtures/workflows/eval-boundary/pull-requests-write/eval.yml @@ -2,9 +2,26 @@ on: workflow_dispatch permissions: {} jobs: + select-environment: + runs-on: ubuntu-24.04 + timeout-minutes: 5 + permissions: {} + outputs: + environment: ${{ steps.select.outputs.environment }} + steps: + - id: select + run: | + if [ "$GITHUB_REF" = "refs/heads/main" ]; then + echo "environment=eval" >> "$GITHUB_OUTPUT" + else + echo "environment=eval-test" >> "$GITHUB_OUTPUT" + fi evals: + needs: select-environment runs-on: ubuntu-24.04 - environment: eval + environment: ${{ needs.select-environment.outputs.environment }} + env: + EVAL_ENVIRONMENT: ${{ needs.select-environment.outputs.environment }} permissions: contents: read id-token: write diff --git a/review/tests/unit/fixtures/workflows/eval-boundary/push-trigger/eval.yml b/review/tests/unit/fixtures/workflows/eval-boundary/push-trigger/eval.yml index 297582a..158497a 100644 --- a/review/tests/unit/fixtures/workflows/eval-boundary/push-trigger/eval.yml +++ b/review/tests/unit/fixtures/workflows/eval-boundary/push-trigger/eval.yml @@ -2,9 +2,26 @@ on: push permissions: {} jobs: + select-environment: + runs-on: ubuntu-24.04 + timeout-minutes: 5 + permissions: {} + outputs: + environment: ${{ steps.select.outputs.environment }} + steps: + - id: select + run: | + if [ "$GITHUB_REF" = "refs/heads/main" ]; then + echo "environment=eval" >> "$GITHUB_OUTPUT" + else + echo "environment=eval-test" >> "$GITHUB_OUTPUT" + fi evals: + needs: select-environment runs-on: ubuntu-24.04 - environment: eval + environment: ${{ needs.select-environment.outputs.environment }} + env: + EVAL_ENVIRONMENT: ${{ needs.select-environment.outputs.environment }} permissions: contents: read id-token: write diff --git a/review/tests/unit/fixtures/workflows/eval-boundary/select-job-id-token/eval.yml b/review/tests/unit/fixtures/workflows/eval-boundary/select-job-id-token/eval.yml new file mode 100644 index 0000000..18ad92e --- /dev/null +++ b/review/tests/unit/fixtures/workflows/eval-boundary/select-job-id-token/eval.yml @@ -0,0 +1,30 @@ +# Bad: the select job holds no permission at all. +on: workflow_dispatch +permissions: {} +jobs: + select-environment: + runs-on: ubuntu-24.04 + timeout-minutes: 5 + permissions: + id-token: write + outputs: + environment: ${{ steps.select.outputs.environment }} + steps: + - id: select + run: | + if [ "$GITHUB_REF" = "refs/heads/main" ]; then + echo "environment=eval" >> "$GITHUB_OUTPUT" + else + echo "environment=eval-test" >> "$GITHUB_OUTPUT" + fi + evals: + needs: select-environment + runs-on: ubuntu-24.04 + environment: ${{ needs.select-environment.outputs.environment }} + env: + EVAL_ENVIRONMENT: ${{ needs.select-environment.outputs.environment }} + permissions: + contents: read + id-token: write + steps: + - run: echo evals diff --git a/review/tests/unit/fixtures/workflows/eval-boundary/select-job-missing/eval.yml b/review/tests/unit/fixtures/workflows/eval-boundary/select-job-missing/eval.yml new file mode 100644 index 0000000..898212c --- /dev/null +++ b/review/tests/unit/fixtures/workflows/eval-boundary/select-job-missing/eval.yml @@ -0,0 +1,15 @@ +# Bad: no select job; the environment must come from its exact shell test. +on: workflow_dispatch +permissions: {} +jobs: + evals: + needs: select-environment + runs-on: ubuntu-24.04 + environment: ${{ needs.select-environment.outputs.environment }} + env: + EVAL_ENVIRONMENT: ${{ needs.select-environment.outputs.environment }} + permissions: + contents: read + id-token: write + steps: + - run: echo evals diff --git a/review/tests/unit/fixtures/workflows/eval-boundary/select-job-outputs/eval.yml b/review/tests/unit/fixtures/workflows/eval-boundary/select-job-outputs/eval.yml new file mode 100644 index 0000000..00c0c7c --- /dev/null +++ b/review/tests/unit/fixtures/workflows/eval-boundary/select-job-outputs/eval.yml @@ -0,0 +1,29 @@ +# Bad: the select job output must be the select step's output. +on: workflow_dispatch +permissions: {} +jobs: + select-environment: + runs-on: ubuntu-24.04 + timeout-minutes: 5 + permissions: {} + outputs: + environment: eval + steps: + - id: select + run: | + if [ "$GITHUB_REF" = "refs/heads/main" ]; then + echo "environment=eval" >> "$GITHUB_OUTPUT" + else + echo "environment=eval-test" >> "$GITHUB_OUTPUT" + fi + evals: + needs: select-environment + runs-on: ubuntu-24.04 + environment: ${{ needs.select-environment.outputs.environment }} + env: + EVAL_ENVIRONMENT: ${{ needs.select-environment.outputs.environment }} + permissions: + contents: read + id-token: write + steps: + - run: echo evals diff --git a/review/tests/unit/fixtures/workflows/eval-boundary/select-script-case-insensitive/eval.yml b/review/tests/unit/fixtures/workflows/eval-boundary/select-script-case-insensitive/eval.yml new file mode 100644 index 0000000..ac914b1 --- /dev/null +++ b/review/tests/unit/fixtures/workflows/eval-boundary/select-script-case-insensitive/eval.yml @@ -0,0 +1,29 @@ +# Bad: the select script must be the fixed exact comparison. +on: workflow_dispatch +permissions: {} +jobs: + select-environment: + runs-on: ubuntu-24.04 + timeout-minutes: 5 + permissions: {} + outputs: + environment: ${{ steps.select.outputs.environment }} + steps: + - id: select + run: | + if [ "${GITHUB_REF,,}" = "refs/heads/main" ]; then + echo "environment=eval" >> "$GITHUB_OUTPUT" + else + echo "environment=eval-test" >> "$GITHUB_OUTPUT" + fi + evals: + needs: select-environment + runs-on: ubuntu-24.04 + environment: ${{ needs.select-environment.outputs.environment }} + env: + EVAL_ENVIRONMENT: ${{ needs.select-environment.outputs.environment }} + permissions: + contents: read + id-token: write + steps: + - run: echo evals diff --git a/review/tests/unit/fixtures/workflows/eval-boundary/select-step-env/eval.yml b/review/tests/unit/fixtures/workflows/eval-boundary/select-step-env/eval.yml new file mode 100644 index 0000000..9a9abad --- /dev/null +++ b/review/tests/unit/fixtures/workflows/eval-boundary/select-step-env/eval.yml @@ -0,0 +1,31 @@ +# Bad: the select step takes no env (GITHUB_REF must be the runner's own value). +on: workflow_dispatch +permissions: {} +jobs: + select-environment: + runs-on: ubuntu-24.04 + timeout-minutes: 5 + permissions: {} + outputs: + environment: ${{ steps.select.outputs.environment }} + steps: + - id: select + env: + GITHUB_REF: refs/heads/main + run: | + if [ "$GITHUB_REF" = "refs/heads/main" ]; then + echo "environment=eval" >> "$GITHUB_OUTPUT" + else + echo "environment=eval-test" >> "$GITHUB_OUTPUT" + fi + evals: + needs: select-environment + runs-on: ubuntu-24.04 + environment: ${{ needs.select-environment.outputs.environment }} + env: + EVAL_ENVIRONMENT: ${{ needs.select-environment.outputs.environment }} + permissions: + contents: read + id-token: write + steps: + - run: echo evals diff --git a/review/tests/unit/fixtures/workflows/eval-boundary/workflow-call-trigger/eval.yml b/review/tests/unit/fixtures/workflows/eval-boundary/workflow-call-trigger/eval.yml index 7412183..9b8af1d 100644 --- a/review/tests/unit/fixtures/workflows/eval-boundary/workflow-call-trigger/eval.yml +++ b/review/tests/unit/fixtures/workflows/eval-boundary/workflow-call-trigger/eval.yml @@ -2,9 +2,26 @@ on: [workflow_dispatch, workflow_call] permissions: {} jobs: + select-environment: + runs-on: ubuntu-24.04 + timeout-minutes: 5 + permissions: {} + outputs: + environment: ${{ steps.select.outputs.environment }} + steps: + - id: select + run: | + if [ "$GITHUB_REF" = "refs/heads/main" ]; then + echo "environment=eval" >> "$GITHUB_OUTPUT" + else + echo "environment=eval-test" >> "$GITHUB_OUTPUT" + fi evals: + needs: select-environment runs-on: ubuntu-24.04 - environment: eval + environment: ${{ needs.select-environment.outputs.environment }} + env: + EVAL_ENVIRONMENT: ${{ needs.select-environment.outputs.environment }} permissions: contents: read id-token: write diff --git a/review/tests/unit/fixtures/workflows/eval-boundary/workflow-write-all/eval.yml b/review/tests/unit/fixtures/workflows/eval-boundary/workflow-write-all/eval.yml index d0ee16f..ca9e2e8 100644 --- a/review/tests/unit/fixtures/workflows/eval-boundary/workflow-write-all/eval.yml +++ b/review/tests/unit/fixtures/workflows/eval-boundary/workflow-write-all/eval.yml @@ -2,9 +2,26 @@ on: workflow_dispatch permissions: write-all jobs: + select-environment: + runs-on: ubuntu-24.04 + timeout-minutes: 5 + permissions: {} + outputs: + environment: ${{ steps.select.outputs.environment }} + steps: + - id: select + run: | + if [ "$GITHUB_REF" = "refs/heads/main" ]; then + echo "environment=eval" >> "$GITHUB_OUTPUT" + else + echo "environment=eval-test" >> "$GITHUB_OUTPUT" + fi evals: + needs: select-environment runs-on: ubuntu-24.04 - environment: eval + environment: ${{ needs.select-environment.outputs.environment }} + env: + EVAL_ENVIRONMENT: ${{ needs.select-environment.outputs.environment }} permissions: contents: read id-token: write diff --git a/review/tests/unit/fixtures/workflows/eval-boundary/yaml-extension/eval.yaml b/review/tests/unit/fixtures/workflows/eval-boundary/yaml-extension/eval.yaml index f42c1a6..034c1e2 100644 --- a/review/tests/unit/fixtures/workflows/eval-boundary/yaml-extension/eval.yaml +++ b/review/tests/unit/fixtures/workflows/eval-boundary/yaml-extension/eval.yaml @@ -2,9 +2,26 @@ on: workflow_dispatch permissions: {} jobs: + select-environment: + runs-on: ubuntu-24.04 + timeout-minutes: 5 + permissions: {} + outputs: + environment: ${{ steps.select.outputs.environment }} + steps: + - id: select + run: | + if [ "$GITHUB_REF" = "refs/heads/main" ]; then + echo "environment=eval" >> "$GITHUB_OUTPUT" + else + echo "environment=eval-test" >> "$GITHUB_OUTPUT" + fi evals: + needs: select-environment runs-on: ubuntu-24.04 - environment: eval + environment: ${{ needs.select-environment.outputs.environment }} + env: + EVAL_ENVIRONMENT: ${{ needs.select-environment.outputs.environment }} permissions: contents: read id-token: write diff --git a/review/tests/unit/test_comment_format.py b/review/tests/unit/test_comment_format.py index dcedb86..ce6be4f 100644 --- a/review/tests/unit/test_comment_format.py +++ b/review/tests/unit/test_comment_format.py @@ -6,10 +6,12 @@ from __future__ import annotations +import http.client import io import json import re import urllib.error +from dataclasses import replace import pytest @@ -180,12 +182,41 @@ def test_without_positive_evidence_the_row_stays_neutral(tmp_path, case): GRAPH_OFF_MESSAGE, "The dependency graph is not enabled for this repository.", "Dependency-graph is turned off for this repository", + "Review failed. Dependency graph is disabled for this repository", + "The dependency graph for this repository is disabled.", + "Dependency graph has been turned off for this repository.", + "Dependency graph is currently disabled.", ]) def test_evidence_messages(message): resp = HttpResponse(403, error_body(message), "https://api.github.com/x", {}) assert publish.dependency_graph_off_evidence(resp) +@pytest.mark.parametrize("message", [ + "Dependency graph is not disabled for this repository.", + "Dependency graph isn't disabled.", + "Dependency graph isn\u2019t turned off.", + "Dependency graph is no longer disabled.", + "Dependency graph is never turned off here.", + "The dependency graph is not not enabled.", + "Dependency graph is enabled, but the submission is disabled.", + "Dependency graph is enabled but the submission is disabled.", + "Dependency graph is on; the snapshot upload is disabled.", + "Dependency graph is on and the snapshot API is turned off.", + # Review AC I2: the state words before "nor" are still negated. + "Dependency graph is neither disabled nor turned off.", + "Dependency graph is either disabled or enabled.", + "Dependency graph was not disabled.", + "Dependency graph is reportedly disabled.", + "Dependency graph is hardly ever turned off.", +]) +def test_negated_or_joined_messages_are_not_evidence(message): + """Display only: a negated state, or a state from another clause, keeps + the neutral dependency review row.""" + resp = HttpResponse(403, error_body(message), "https://api.github.com/x", {}) + assert not publish.dependency_graph_off_evidence(resp) + + def test_probe_runs_only_when_dependency_review_failed(tmp_path): r = run(tmp_path) assert not [p for p in r.gh.paths() if "/dependency-graph/" in p] @@ -227,6 +258,34 @@ def open(self, req, timeout): assert len(resp.body) == 101 # cut at the byte cap + 1, like a 200 body +class _BadBody(io.BytesIO): + def __init__(self, error: Exception) -> None: + super().__init__(b"") + self.error = error + + def read(self, *args, **kwargs): + raise self.error + + +@pytest.mark.parametrize("error", [ + http.client.IncompleteRead(b"Dependency graph is dis", 40), + http.client.HTTPException("bad chunk"), + OSError("reset"), + ValueError("closed"), +]) +def test_default_transport_error_body_read_failure_keeps_the_status(error): + """A cut-off error body is empty: the status stays, so every caller still + fails on it (fail closed), and the display probe finds no evidence.""" + class Opener: + def open(self, req, timeout): + raise urllib.error.HTTPError(req.full_url, 403, "Forbidden", {}, _BadBody(error)) + + req = HttpRequest("GET", "https://api.github.com/x", {}, 100, 5.0) + resp = collect.default_transport(req, opener=Opener()) + assert resp.status == 403 and resp.body == b"" + assert not publish.dependency_graph_off_evidence(resp) + + # ---------------------------------------------------------------- 3. plain paths @@ -300,6 +359,53 @@ def test_unsigned_commits_row_has_count_and_link_and_leaves_the_pass(tmp_path): assert "**Unsigned commits:** 2 of 3 commits in this PR are not verified-signed." in row[3] assert f"[How to sign commits]({publish.SIGNING_GUIDE_URL})" in row[3] assert row[4] == publish.COMMIT_CHECK_ROW + # The TIP must not say "nothing to do" next to a row that says the merge box blocks. + assert publish.NOTHING_TO_DO not in s and f"** {publish.SIGNING_STEP}" in callout(s) + + +@pytest.mark.parametrize(("total", "unsigned", "text"), [ + (1, 1, "1 of 1 commit in this PR is not verified-signed."), + (2, 1, "1 of 2 commits in this PR is not verified-signed."), + (2, 2, "2 of 2 commits in this PR are not verified-signed."), + (3, 2, "2 of 3 commits in this PR are not verified-signed."), + (publish.COMMITS_MAX, 1, f"1 of the first {publish.COMMITS_MAX} commits in this PR is not verified-signed."), + (publish.COMMITS_MAX, 7, f"7 of the first {publish.COMMITS_MAX} commits in this PR are not verified-signed."), +]) +def test_signing_row_grammar(total, unsigned, text): + [row] = publish._signing_row(publish.SummaryFacts(commits=publish.CommitSigning(total, unsigned))) + assert row.claim.startswith(f"**Unsigned commits:** {text} ") + + +def callout(s: str) -> str: + """The callout lines (each starts with "> ").""" + return "\n".join(line for line in s.split("\n") if line.startswith("> ")) + + +def test_tip_says_nothing_to_do_only_without_unsigned_commits(tmp_path): + clean = run(tmp_path / "a", FakeGitHub(commits=[signed_commit(True)])).summary + assert callout_kind(clean) == "TIP" and f"** {publish.NOTHING_TO_DO}" in callout(clean) + assert publish.SIGNING_STEP not in clean + + +def test_waiver_tip_names_the_signing_step(): + from .test_publish import CTX + + r = replace(run_result_for_render(), override_applied=True) + facts = publish.SummaryFacts(commits=publish.CommitSigning(2, 1)) + body = publish.render_summary(CTX, r, bundle=None, inline=publish.InlineOutcome(), facts=facts).body + assert "Unsigned commits" in body and callout_kind(body) == "TIP" + assert "A maintainer waiver was applied by a maintainer" in callout(body) + assert publish.NOTHING_TO_DO not in body and f"** {publish.SIGNING_STEP}" in callout(body) + plain_body = publish.render_summary(CTX, r, bundle=None, inline=publish.InlineOutcome()).body + assert f"** {publish.NOTHING_TO_DO}" in callout(plain_body) and publish.SIGNING_STEP not in plain_body + + +def test_note_callout_is_unchanged_by_unsigned_commits(tmp_path): + passes = clean_passes(review=[finding("bug", "low", file="a.py", line=1, quote="x")]) + signed = run(tmp_path / "a", FakeGitHub(commits=[signed_commit(True)]), passes=passes).summary + unsigned = run(tmp_path / "b", FakeGitHub(commits=[signed_commit(False)]), passes=passes).summary + assert callout_kind(unsigned) == "NOTE" and callout(unsigned) == callout(signed) + assert publish.NOTHING_TO_DO not in unsigned and publish.SIGNING_STEP not in unsigned def test_unsigned_commits_never_change_a_fail(tmp_path): @@ -311,7 +417,7 @@ def test_unsigned_commits_never_change_a_fail(tmp_path): head = signed.summary.split("\n")[1] assert unsigned.summary.split("\n")[1] == head rows = findings_rows(unsigned.summary) - assert rows[-1][0] == publish.INFO_CELL and "1 of 1 commits in this PR is not" in rows[-1][3] + assert rows[-1][0] == publish.INFO_CELL and "1 of 1 commit in this PR is not" in rows[-1][3] assert "/oc-review override" in unsigned.summary # the waiver line is unchanged @@ -367,8 +473,11 @@ def test_signing_row_is_left_out_when_it_would_cause_a_cut(): tight = publish.render_summary(CTX, r, bundle=None, inline=publish.InlineOutcome(), limit=len(base.body) + 5, facts=facts) assert tight == base + # With the row left out, the TIP keeps "nothing to do": it never names a row that is not there. + assert f"** {publish.NOTHING_TO_DO}" in callout(tight.body) and publish.SIGNING_STEP not in tight.body roomy = publish.render_summary(CTX, r, bundle=None, inline=publish.InlineOutcome(), facts=facts) assert "Unsigned commits" in roomy.body and not roomy.cut and not roomy.overflow + assert f"** {publish.SIGNING_STEP}" in callout(roomy.body) def run_result_for_render() -> verdict.GateResult: diff --git a/review/tests/unit/test_evals.py b/review/tests/unit/test_evals.py index 87a18a9..3c2075b 100644 --- a/review/tests/unit/test_evals.py +++ b/review/tests/unit/test_evals.py @@ -104,7 +104,7 @@ def oidc_transport(url, request_token): env = {"JOB_WORKFLOW_SHA": JOB_SHA, "RUNNER_TEMP": str(tmp_path / "rt"), "ACTIONS_ID_TOKEN_REQUEST_URL": "https://pipelines.actions.githubusercontent.com/oidc?api-version=2.0", - "ACTIONS_ID_TOKEN_REQUEST_TOKEN": "t"} + "ACTIONS_ID_TOKEN_REQUEST_TOKEN": "t", "EVAL_ENVIRONMENT": "eval"} (tmp_path / "rt").mkdir(exist_ok=True) return evals.main( ["--fixtures-dir", str(tmp_path / "fixtures"), "--out-dir", str(tmp_path / "out")], @@ -196,8 +196,10 @@ def test_eval_ids_differ_from_production_ids(): def test_production_ids_differ_from_every_test_id(): prod = analyze.federation_ids() - assert prod.rule_id not in {evals.eval_federation().rule_id, TEST_RULE_ID, SIMULATION_RULE_ID} - assert prod.service_account_id not in {evals.eval_federation().service_account_id, TEST_SERVICE_ACCOUNT_ID} + eval_rules = {r.rule_id for r in evals.EVAL_FEDERATION.values()} + eval_accounts = {r.service_account_id for r in evals.EVAL_FEDERATION.values()} + assert prod.rule_id not in {*eval_rules, TEST_RULE_ID, SIMULATION_RULE_ID} + assert prod.service_account_id not in {*eval_accounts, TEST_SERVICE_ACCOUNT_ID} def test_main_uses_the_eval_ids_by_default(tmp_path): @@ -205,12 +207,92 @@ def test_main_uses_the_eval_ids_by_default(tmp_path): seen = [] env = {"JOB_WORKFLOW_SHA": JOB_SHA, "RUNNER_TEMP": str(tmp_path), "ACTIONS_ID_TOKEN_REQUEST_URL": "https://pipelines.actions.githubusercontent.com/oidc?api-version=2.0", - "ACTIONS_ID_TOKEN_REQUEST_TOKEN": "t"} + "ACTIONS_ID_TOKEN_REQUEST_TOKEN": "t", "EVAL_ENVIRONMENT": "eval"} evals.main(["--fixtures-dir", str(tmp_path / "fixtures"), "--out-dir", str(tmp_path / "out")], environ=env, oidc_transport=oidc, client_factory=factory_for(FakeClient(), seen), sleep=lambda s: None) assert seen == [evals.eval_federation()] +# ---------------------------------------------------------------- eval environments (eval, eval-test) + + +def main_in(tmp_path, environment, *, client=None, seen=None, oidc_calls=None): + """evals.main with no injected federation, so it picks the IDs itself. + `environment` None leaves EVAL_ENVIRONMENT out.""" + make_case(tmp_path / "fixtures") + env = {"JOB_WORKFLOW_SHA": JOB_SHA, "RUNNER_TEMP": str(tmp_path), + "ACTIONS_ID_TOKEN_REQUEST_URL": "https://pipelines.actions.githubusercontent.com/oidc?api-version=2.0", + "ACTIONS_ID_TOKEN_REQUEST_TOKEN": "t"} + if environment is not None: + env["EVAL_ENVIRONMENT"] = environment + + def oidc_transport(url, request_token): + if oidc_calls is not None: + oidc_calls.append(url) + return oidc(url, request_token) + + return evals.main(["--fixtures-dir", str(tmp_path / "fixtures"), "--out-dir", str(tmp_path / "out")], + environ=env, oidc_transport=oidc_transport, + client_factory=factory_for(client or FakeClient(), seen), sleep=lambda s: None) + + +def test_eval_environments_are_exactly_eval_and_eval_test(): + assert set(evals.EVAL_FEDERATION) == {"eval", "eval-test"} + assert evals.EVAL_ENVIRONMENT_VAR == "EVAL_ENVIRONMENT" + + +def test_eval_test_rule_is_unset_and_shares_the_eval_service_account(): + # The owner has not created the eval-test rule yet; the owner may change its service account. + assert evals.EVAL_FEDERATION["eval-test"].rule_id is None + assert evals.EVAL_FEDERATION["eval-test"].service_account_id == evals.EVAL_FEDERATION["eval"].service_account_id + + +def test_every_set_eval_rule_id_has_the_real_id_form(): + for name, rule in evals.EVAL_FEDERATION.items(): + if rule.rule_id is not None: + ids = analyze.FederationIds(rule.rule_id, evals.EVAL_ORGANIZATION_ID, rule.service_account_id) + assert analyze.ids_valid(ids) and not analyze.ids_are_placeholders(ids), name + + +def test_eval_test_without_a_rule_fails_closed(tmp_path, capsys): + client, oidc_calls = FakeClient(), [] + assert main_in(tmp_path, "eval-test", client=client, oidc_calls=oidc_calls) == 1 + assert "eval-test federation rule not configured" in capsys.readouterr().err + assert client.calls == 0 and oidc_calls == [] + with pytest.raises(evals.EvalConfigError, match="eval-test federation rule not configured"): + evals.eval_federation("eval-test") + + +@pytest.mark.parametrize("environment", [None, "", "EVAL", "Eval-Test", "production", "eval ", "eval-test2"]) +def test_other_environment_values_fail_closed(tmp_path, capsys, environment): + client, oidc_calls = FakeClient(), [] + assert main_in(tmp_path, environment, client=client, oidc_calls=oidc_calls) == 1 + assert "EVAL_ENVIRONMENT must be one of ['eval', 'eval-test']" in capsys.readouterr().err + assert client.calls == 0 and oidc_calls == [] + assert not (tmp_path / "out" / "results.jsonl").exists() + + +def test_unknown_environment_has_no_ids(): + with pytest.raises(evals.EvalConfigError): + evals.eval_federation("production") + + +def test_main_picks_the_ids_of_its_environment(tmp_path, monkeypatch): + configured = evals.EvalRule("fdrl_01ZyXwVuTsRqPoNmLkJiHgFe", "svac_01ZyXwVuTsRqPoNmLkJiHgFe") + monkeypatch.setattr(evals, "EVAL_FEDERATION", {**evals.EVAL_FEDERATION, "eval-test": configured}) + seen_test, seen_main = [], [] + main_in(tmp_path / "t", "eval-test", seen=seen_test) + main_in(tmp_path / "m", "eval", seen=seen_main) + assert seen_test == [analyze.FederationIds(configured.rule_id, evals.EVAL_ORGANIZATION_ID, + configured.service_account_id)] + assert seen_main == [evals.eval_federation("eval")] != seen_test + + +def test_eval_federation_map_is_read_only(): + with pytest.raises(TypeError): + evals.EVAL_FEDERATION["eval-test"] = evals.EvalRule("fdrl_x", "svac_x") # type: ignore[index] + + def test_failed_pass_is_not_a_match(tmp_path): make_case(tmp_path / "fixtures") client = FakeClient() diff --git a/review/tests/unit/test_supported_repos.py b/review/tests/unit/test_supported_repos.py index d794f11..80acbf2 100644 --- a/review/tests/unit/test_supported_repos.py +++ b/review/tests/unit/test_supported_repos.py @@ -59,8 +59,15 @@ def test_excluded_list_names_github_and_the_test_repo_and_overlaps_nothing(): assert not excluded & {r["id"] for r in supported()} -def test_ids_match_the_plan_at_phase_5(): - # Plan v2 section 2.1 (read-only GETs, 2026-11-01). Update with the list. +START_HERE_ID = 82613670 # to be archived (issue #27); not a review target (spec P6-5) + + +def test_ids_match_the_phase_6_scope(): + # The 4 pilot repos (read-only GETs, 2026-11-01); START_HERE dropped (spec P6-5). Update with the list. assert {r["id"]: r["status"] for r in supported()} == { - 135777271: "target", 182584277: "target", 107068150: "target", 146210805: "target", - 82613670: "candidate"} + 135777271: "target", 182584277: "target", 107068150: "target", 146210805: "target"} + + +def test_start_here_is_excluded_not_a_candidate(): + excluded = {int(cells[1]) for cells in table("Excluded active repos")} + assert START_HERE_ID in excluded diff --git a/review/tests/unit/test_workflows.py b/review/tests/unit/test_workflows.py index df14501..cf998cb 100644 --- a/review/tests/unit/test_workflows.py +++ b/review/tests/unit/test_workflows.py @@ -27,7 +27,26 @@ CENTRAL = "claude-review.yml" TEST = "test.yml" EVAL = "eval.yml" -EVAL_ENVIRONMENT = "eval" +# eval.yml picks its environment from the ref (spec "Fixture eval credential"): +# main runs reviewed code in `eval` (no approval); any other ref runs in +# `eval-test` (owner approval, prevent self-review). Actions expressions +# compare strings without case (`github.ref == 'refs/heads/main'` is true for +# refs/heads/MAIN), so the choice is made by an exact shell test in a select +# job with no permissions. The evals job takes its one output for the +# environment and for EVAL_ENVIRONMENT (read by evals.py). +EVAL_ENVIRONMENTS = frozenset({"eval", "eval-test"}) +EVAL_SELECT_JOB = "select-environment" +EVAL_SELECT_STEP_ID = "select" +EVAL_SELECT_RUN = ( + 'if [ "$GITHUB_REF" = "refs/heads/main" ]; then\n' + ' echo "environment=eval" >> "$GITHUB_OUTPUT"\n' + "else\n" + ' echo "environment=eval-test" >> "$GITHUB_OUTPUT"\n' + "fi\n" +) +EVAL_SELECT_OUTPUTS = {"environment": f"${{{{ steps.{EVAL_SELECT_STEP_ID}.outputs.environment }}}}"} +EVAL_ENVIRONMENT_EXPRESSION = f"${{{{ needs.{EVAL_SELECT_JOB}.outputs.environment }}}}" +EVAL_ENVIRONMENT_VAR = "EVAL_ENVIRONMENT" HARDEN = "step-security/harden-runner" CHECKOUT = "actions/checkout" @@ -48,6 +67,8 @@ # timeout-minutes per job, by workflow file (spec "Workflow details", plan 2.1). TIMEOUT_MINUTES = {"claude-review.yml": 10, "test.yml": 10, "eval.yml": 30} DEFAULT_TIMEOUT_MINUTES = 10 +# A job that differs from its file's value (the eval select job runs one shell test). +JOB_TIMEOUT_MINUTES = {("eval.yml", "select-environment"): 5} # Central workflow (spec "Architecture", "Workflow details"). COLLECT, ANALYZE, DEPENDENCY_REVIEW, GATE = "collect", "analyze", "dependency-review", "oc-claude-review" CENTRAL_PERMISSIONS = { @@ -111,21 +132,26 @@ # that only in block mode). No disable-sudo-and-containers (V10). EGRESS_POLICY = "audit" ENDPOINT_RE = re.compile(r"(?:\*\.)?[a-z0-9-]+(?:\.[a-z0-9-]+)+:[0-9]{1,5}") -# The allowlist per central job that V10 measured (kit K3, block mode). +# setup-uv v10.3.3 fetches the uv version manifest from the Astral mirror +# first (GitHub raw is the fallback; src/utils/constants.ts at its commit). Added +# to every job that runs setup-uv, not measured (spec K-14). +SETUP_UV_MANIFEST_HOST = "releases.astral.sh:443" +# The allowlist per central job that V10 measured (kit K3, block mode), plus +# SETUP_UV_MANIFEST_HOST where the job runs setup-uv. V10_ALLOWLISTS = { COLLECT: { - "api.github.com:443", "github.com:443", "raw.githubusercontent.com:443", + "api.github.com:443", "github.com:443", "raw.githubusercontent.com:443", SETUP_UV_MANIFEST_HOST, "release-assets.githubusercontent.com:443", "files.pythonhosted.org:443", "pypi.org:443", "registry.npmjs.org:443", "results-receiver.actions.githubusercontent.com:443", "*.blob.core.windows.net:443", }, ANALYZE: { - "api.github.com:443", "github.com:443", "raw.githubusercontent.com:443", + "api.github.com:443", "github.com:443", "raw.githubusercontent.com:443", SETUP_UV_MANIFEST_HOST, "release-assets.githubusercontent.com:443", "files.pythonhosted.org:443", "api.anthropic.com:443", "*.actions.githubusercontent.com:443", "*.blob.core.windows.net:443", }, DEPENDENCY_REVIEW: {"api.github.com:443"}, GATE: { - "api.github.com:443", "github.com:443", "raw.githubusercontent.com:443", + "api.github.com:443", "github.com:443", "raw.githubusercontent.com:443", SETUP_UV_MANIFEST_HOST, "release-assets.githubusercontent.com:443", "files.pythonhosted.org:443", "results-receiver.actions.githubusercontent.com:443", "*.blob.core.windows.net:443", }, @@ -310,9 +336,10 @@ def common_violations(wf: Workflow) -> list[str]: used = [s for s in strings(doc) if SECRETS_RE.search(s)] if used: r.add("workflow", "secrets", f"no secrets context in any form, got {used!r}") - want_timeout = TIMEOUT_MINUTES.get(wf.name, DEFAULT_TIMEOUT_MINUTES) + file_timeout = TIMEOUT_MINUTES.get(wf.name, DEFAULT_TIMEOUT_MINUTES) parsed_uses = 0 for name, job in jobs(doc).items(): + want_timeout = JOB_TIMEOUT_MINUTES.get((wf.name, name), file_timeout) timeout = job.get("timeout-minutes") if type(timeout) is not int or timeout != want_timeout: r.add(name, "timeout", f"timeout-minutes must be {want_timeout}, got {timeout!r}") @@ -581,10 +608,41 @@ def environment_name(value: Any) -> str | None: return name +def eval_select_violations(wf: Workflow, r: "Report") -> None: + """eval.yml has the select job: no environment, `permissions: {}`, the + one output, harden-runner steps and exactly one run step, the fixed + case-sensitive script with no if, env or shell.""" + job = jobs(wf.doc).get(EVAL_SELECT_JOB) + if not isinstance(job, dict): + r.add("workflow", "eval-environment", f"needs the job {EVAL_SELECT_JOB!r}") + return + where = EVAL_SELECT_JOB + if set(job) - {"runs-on", "timeout-minutes", "permissions", "outputs", "steps"}: + r.add(where, "eval-environment", f"unexpected keys {sorted(set(job) - {'runs-on', 'timeout-minutes', 'permissions', 'outputs', 'steps'})}") + if job.get("permissions") != {}: + r.add(where, "eval-environment", f"permissions must be {{}}, got {job.get('permissions')!r}") + if job.get("outputs") != EVAL_SELECT_OUTPUTS: + r.add(where, "eval-environment", f"outputs must be exactly {EVAL_SELECT_OUTPUTS!r}, got {job.get('outputs')!r}") + runs = [st for st in steps(job) if "uses" not in st] + others = [st for st in steps(job) if "uses" in st and action(st) != HARDEN] + if others: + r.add(where, "eval-environment", "only harden-runner and the select script may run here") + if len(runs) != 1: + r.add(where, "eval-environment", f"exactly one run step, got {len(runs)}") + return + [st] = runs + if st.get("id") != EVAL_SELECT_STEP_ID or st.get("run") != EVAL_SELECT_RUN or set(st) - {"name", "id", "run"}: + r.add(where, "eval-environment", "the select step must be the fixed script (id, run; no if, env or shell)") + + def eval_boundary_violations(directory: Path) -> list[str]: - """Only eval.yml may use `eval`, and each job that does states exactly - contents: read and id-token: write itself. Environment names are not case - sensitive, so `EVAL` counts; a name that is not a literal counts too.""" + """Only eval.yml may use `eval` or `eval-test`. eval.yml has the select + job (`eval_select_violations`); each eval.yml job with an environment + needs that job, uses exactly EVAL_ENVIRONMENT_EXPRESSION, sets + EVAL_ENVIRONMENT to the same expression, and states exactly contents: read + and id-token: write itself. In any other workflow, an environment that is + not a literal counts as a use; names are not case sensitive, so `EVAL` + counts too.""" out: list[str] = [] for path in workflow_files(directory): r = Report(path.name) @@ -599,17 +657,27 @@ def eval_boundary_violations(directory: Path) -> list[str]: r.add("workflow", "eval-trigger", f"workflow_dispatch only, got {sorted(triggers(wf.doc))}") if wf.doc.get("permissions") == "write-all": r.add("workflow", "eval-permissions", "write-all at workflow level") + eval_select_violations(wf, r) for name, job in jobs(wf.doc).items(): if "environment" not in job: continue - env_name = environment_name(job["environment"]) - if env_name is None: - r.add(name, "eval-environment", f"environment must be a literal string, got {job['environment']!r}") - uses_eval = env_name is None or env_name.strip().casefold() == EVAL_ENVIRONMENT - if not uses_eval: - continue - if wf.name != EVAL: - r.add(name, "eval-environment", "only eval.yml may use the eval environment") + if wf.name == EVAL: + if job.get("needs") not in (EVAL_SELECT_JOB, [EVAL_SELECT_JOB]): + r.add(name, "eval-environment", f"needs must be exactly {EVAL_SELECT_JOB!r}, got {job.get('needs')!r}") + if job["environment"] != EVAL_ENVIRONMENT_EXPRESSION: + r.add(name, "eval-environment", + f"environment must be exactly {EVAL_ENVIRONMENT_EXPRESSION!r}, got {job['environment']!r}") + if env(job).get(EVAL_ENVIRONMENT_VAR) != EVAL_ENVIRONMENT_EXPRESSION: + r.add(name, "eval-environment", + f"job env {EVAL_ENVIRONMENT_VAR} must be exactly {EVAL_ENVIRONMENT_EXPRESSION!r}") + else: + env_name = environment_name(job["environment"]) + if env_name is None: + r.add(name, "eval-environment", f"environment must be a literal string, got {job['environment']!r}") + elif env_name.strip().casefold() not in EVAL_ENVIRONMENTS: + continue + else: + r.add(name, "eval-environment", "only eval.yml may use the eval or eval-test environment") if "permissions" not in job: r.add(name, "eval-permissions", "a job that uses eval sets job-level permissions") elif job["permissions"] != EVAL_JOB_PERMISSIONS: @@ -675,14 +743,88 @@ def test_no_continue_on_error_anywhere(): assert all("continue-on-error" not in s for s in steps(job)), f"{path.name} {name}" -def test_only_eval_yml_uses_the_eval_environment(): +def test_only_eval_yml_uses_an_environment(): users = [ - (path.name, name) + (path.name, name, job["environment"]) for path in workflow_files(WORKFLOWS) for name, job in jobs(load_workflow(path).doc).items() - if "environment" in job and (environment_name(job["environment"]) or EVAL_ENVIRONMENT).strip().casefold() == EVAL_ENVIRONMENT + if "environment" in job ] - assert users == [(EVAL, "fixture-evals")] + assert users == [(EVAL, "fixture-evals", EVAL_ENVIRONMENT_EXPRESSION)] + + +def test_eval_environment_var_is_set_from_the_same_expression(): + job = jobs(load_workflow(WORKFLOWS / EVAL).doc)["fixture-evals"] + assert env(job)[EVAL_ENVIRONMENT_VAR] == job["environment"] == EVAL_ENVIRONMENT_EXPRESSION + + +def eval_select_script() -> str: + """The select step of the real eval.yml.""" + job = jobs(load_workflow(WORKFLOWS / EVAL).doc)[EVAL_SELECT_JOB] + [st] = [st for st in steps(job) if st.get("id") == EVAL_SELECT_STEP_ID] + return st["run"] + + +def eval_environment_for(ref: str, tmp_path: Path, script: str | None = None) -> str: + """Run the select script as the runner does (`bash -e`) with GITHUB_REF = + `ref`, and return the `environment` it writes to GITHUB_OUTPUT.""" + path = tmp_path / "select.sh" + path.write_text(eval_select_script() if script is None else script, encoding="utf-8") + output = tmp_path / "output" + output.write_text("", encoding="utf-8") + proc = subprocess.run([*DEFAULT_SHELL, str(path)], env={"PATH": "/usr/bin:/bin", "GITHUB_REF": ref, + "GITHUB_OUTPUT": str(output)}, + capture_output=True, text=True, timeout=30, check=False) + assert proc.returncode == 0, proc.stderr + lines = output.read_text(encoding="utf-8").splitlines() + assert len(lines) == 1 and lines[0].startswith("environment="), lines + return lines[0].removeprefix("environment=") + + +@pytest.mark.parametrize(("ref", "environment"), [ + ("refs/heads/main", "eval"), + ("refs/heads/MAIN", "eval-test"), # Actions `==` ignores case; the shell test does not + ("refs/heads/Main", "eval-test"), + ("refs/heads/main ", "eval-test"), + ("refs/heads/feature", "eval-test"), + ("refs/heads/main-test", "eval-test"), + ("refs/heads/x/refs/heads/main", "eval-test"), + ("refs/tags/main", "eval-test"), + ("refs/tags/v1.0.0", "eval-test"), + ("refs/pull/1/merge", "eval-test"), + ("main", "eval-test"), + ("", "eval-test"), +]) +def test_eval_environment_selection(ref, environment, tmp_path): + assert eval_select_script() == EVAL_SELECT_RUN + assert eval_environment_for(ref, tmp_path) == environment + assert environment in EVAL_ENVIRONMENTS + + +def test_case_insensitive_selection_would_pick_eval_for_main_in_capitals(tmp_path): + """The defect the select job closes: a case-insensitive compare (what an + Actions expression does) sends refs/heads/MAIN to `eval`.""" + loose = EVAL_SELECT_RUN.replace('[ "$GITHUB_REF" = "refs/heads/main" ]', + '[ "$(printf %s "$GITHUB_REF" | tr A-Z a-z)" = "refs/heads/main" ]') + assert loose != EVAL_SELECT_RUN + assert eval_environment_for("refs/heads/MAIN", tmp_path, loose) == "eval" + assert eval_environment_for("refs/heads/MAIN", tmp_path) == "eval-test" + + +def test_eval_select_job_has_no_permissions_and_no_environment(): + job = jobs(load_workflow(WORKFLOWS / EVAL).doc)[EVAL_SELECT_JOB] + assert job["permissions"] == {} and "environment" not in job + assert job["outputs"] == EVAL_SELECT_OUTPUTS + assert jobs(load_workflow(WORKFLOWS / EVAL).doc)["fixture-evals"]["needs"] == EVAL_SELECT_JOB + + +def test_eval_environment_names_match_the_runner(tmp_path): + from oc_review import evals + + assert set(evals.EVAL_FEDERATION) == EVAL_ENVIRONMENTS + assert evals.EVAL_ENVIRONMENT_VAR == EVAL_ENVIRONMENT_VAR + assert (evals.EVAL_ENVIRONMENT_MAIN, evals.EVAL_ENVIRONMENT_TEST) == ( + eval_environment_for("refs/heads/main", tmp_path), eval_environment_for("refs/heads/other", tmp_path)) def test_eval_upload_step_always_runs_and_keeps_data_out(): @@ -789,6 +931,22 @@ def test_test_workflow_runs_the_release_tests_in_the_unit_tests_job(): # Eval boundary fixture directory -> the tag its violation must carry. EVAL_CASES = { "other-file-uses-eval": "eval-environment", + "other-file-uses-eval-test": "eval-environment", + "other-file-uses-eval-test-uppercase": "eval-environment", + "other-file-same-expression": "eval-environment", + "eval-literal-eval": "eval-environment", + "eval-literal-eval-test": "eval-environment", + "eval-name-form": "eval-environment", + "eval-case-insensitive-expression": "eval-environment", + "select-job-missing": "eval-environment", + "select-job-id-token": "eval-environment", + "select-script-case-insensitive": "eval-environment", + "select-step-env": "eval-environment", + "select-job-outputs": "eval-environment", + "evals-job-needs-missing": "eval-environment", + "eval-input-expression": "eval-environment", + "eval-env-var-missing": "eval-environment", + "eval-env-var-literal": "eval-environment", "other-file-name-form": "eval-environment", "other-file-expression": "eval-environment", "other-file-uppercase": "eval-environment",