diff --git a/.github/workflows/claude-review.yml b/.github/workflows/claude-review.yml index 6381f9a..164fd7d 100644 --- a/.github/workflows/claude-review.yml +++ b/.github/workflows/claude-review.yml @@ -18,9 +18,9 @@ # - no ${{ }} inside run: scripts; values reach scripts through env: # # 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; -# 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 +# 24 jobs in "Complete job" (symptoms match harden-runner issue 708, which +# v2.22.4 fixes upstream; audit mode stays until the .github issue 24 soak and +# block-mode runs pass). 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 @@ -52,9 +52,9 @@ jobs: merge-base-sha: ${{ steps.collect.outputs.merge-base-sha }} steps: - name: Harden the runner - uses: step-security/harden-runner@46b06cea07f9e64621fdf066ba77ac6915163d43 # v2.22.3, released 2026-10-27 + uses: step-security/harden-runner@1d88384fd173af6ecb012dfd0264a306b711241a # v2.22.4, released 2026-11-06 with: - # Audit until https://github.com/step-security/harden-runner/issues/708 is fixed (spec K-14). + # Audit until the issue 24 soak and block-mode runs pass; upstream issue 708 is fixed in v2.22.4 (spec K-14). egress-policy: audit allowed-endpoints: > api.github.com:443 @@ -119,9 +119,9 @@ jobs: supply-chain-digest: ${{ steps.analyze.outputs.supply-chain-digest }} steps: - name: Harden the runner - uses: step-security/harden-runner@46b06cea07f9e64621fdf066ba77ac6915163d43 # v2.22.3, released 2026-10-27 + uses: step-security/harden-runner@1d88384fd173af6ecb012dfd0264a306b711241a # v2.22.4, released 2026-11-06 with: - # Audit until https://github.com/step-security/harden-runner/issues/708 is fixed (spec K-14). + # Audit until the issue 24 soak and block-mode runs pass; upstream issue 708 is fixed in v2.22.4 (spec K-14). egress-policy: audit allowed-endpoints: > api.github.com:443 @@ -216,9 +216,9 @@ jobs: outcome: ${{ steps.dr.outcome }} steps: - name: Harden the runner - uses: step-security/harden-runner@46b06cea07f9e64621fdf066ba77ac6915163d43 # v2.22.3, released 2026-10-27 + uses: step-security/harden-runner@1d88384fd173af6ecb012dfd0264a306b711241a # v2.22.4, released 2026-11-06 with: - # Audit until https://github.com/step-security/harden-runner/issues/708 is fixed (spec K-14). + # Audit until the issue 24 soak and block-mode runs pass; upstream issue 708 is fixed in v2.22.4 (spec K-14). egress-policy: audit allowed-endpoints: > api.github.com:443 @@ -249,9 +249,9 @@ jobs: UV_PYTHON: python3.12 steps: - name: Harden the runner - uses: step-security/harden-runner@46b06cea07f9e64621fdf066ba77ac6915163d43 # v2.22.3, released 2026-10-27 + uses: step-security/harden-runner@1d88384fd173af6ecb012dfd0264a306b711241a # v2.22.4, released 2026-11-06 with: - # Audit until https://github.com/step-security/harden-runner/issues/708 is fixed (spec K-14). + # Audit until the issue 24 soak and block-mode runs pass; upstream issue 708 is fixed in v2.22.4 (spec K-14). egress-policy: audit allowed-endpoints: > api.github.com:443 diff --git a/.github/workflows/eval.yml b/.github/workflows/eval.yml index a6baa34..9763906 100644 --- a/.github/workflows/eval.yml +++ b/.github/workflows/eval.yml @@ -41,7 +41,7 @@ jobs: environment: ${{ steps.select.outputs.environment }} steps: - name: Harden the runner - uses: step-security/harden-runner@46b06cea07f9e64621fdf066ba77ac6915163d43 # v2.22.3, released 2026-10-27 + uses: step-security/harden-runner@1d88384fd173af6ecb012dfd0264a306b711241a # v2.22.4, released 2026-11-06 with: egress-policy: audit allowed-endpoints: > @@ -70,7 +70,7 @@ jobs: EVAL_ENVIRONMENT: ${{ needs.select-environment.outputs.environment }} steps: - name: Harden the runner - uses: step-security/harden-runner@46b06cea07f9e64621fdf066ba77ac6915163d43 # v2.22.3, released 2026-10-27 + uses: step-security/harden-runner@1d88384fd173af6ecb012dfd0264a306b711241a # v2.22.4, released 2026-11-06 with: egress-policy: audit allowed-endpoints: > diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index b75bda5..c50a3f0 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -31,7 +31,7 @@ jobs: UV_PYTHON: python3.12 steps: - name: Harden the runner - uses: step-security/harden-runner@46b06cea07f9e64621fdf066ba77ac6915163d43 # v2.22.3, released 2026-10-27 + uses: step-security/harden-runner@1d88384fd173af6ecb012dfd0264a306b711241a # v2.22.4, released 2026-11-06 with: egress-policy: audit allowed-endpoints: > diff --git a/docs/build-plan.md b/docs/build-plan.md index 9ff621e..b55afde 100644 --- a/docs/build-plan.md +++ b/docs/build-plan.md @@ -106,7 +106,7 @@ 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; 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. +- [ ] 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-06 every workflow pins harden-runner v2.22.4, a second age exception approved by the owner (spec K-16), and since 2026-11-05 setup-uv v10.3.3, 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"). @@ -222,11 +222,11 @@ 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. 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: "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" off (2026-11-06: the sole reviewer is the run actor, so it would block every approval; read back: reviewer irvingpop, no branch policy, admin bypass 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). -- [ ] Revisit harden-runner block mode (spec K-14): switch to block when harden-runner [issue 708](https://github.com/step-security/harden-runner/issues/708) is fixed in a harden-runner release. Pin that release, measure the `test.yml` and `eval.yml` lists in audit mode, and re-test block mode. If no job hangs, set `egress-policy: block` and `disable-telemetry: true` in every job, and update the unit tests and the spec. +- [ ] Revisit harden-runner block mode (spec K-14): switch to block after the soak. Upstream [issue 708](https://github.com/step-security/harden-runner/issues/708) is fixed in v2.22.4, which is pinned since 2026-11-06 (spec K-16; .github issue 24 stays open). Measure the `test.yml` and `eval.yml` lists in audit mode, and re-test block mode. If no job hangs, set `egress-policy: block` and `disable-telemetry: true` in every job, and update the unit tests and the spec. Depends on: phase 4; V6, V8, V22, V23. diff --git a/docs/claude-review-design.md b/docs/claude-review-design.md index 55b8e1d..587b019 100644 --- a/docs/claude-review-design.md +++ b/docs/claude-review-design.md @@ -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. 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"). | +| 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 ("prevent self-review" is off: the sole reviewer is also the run actor, so it would block every approval); 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. | @@ -196,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 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. +- **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-06): harden-runner v2.22.4 (`1d88384fd173af6ecb012dfd0264a306b711241a`, released 2026-11-06; a second age exception, K-16) and setup-uv v10.3.3 (`68559e99cc807c1eb7e9238fd43a15bba460eec2`, released 2026-10-28, 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: @@ -505,6 +505,7 @@ An override is a PR comment from an authorized person: - the author has write or admin permission now, from `GET /repos/{owner}/{repo}/collaborators/{user}/permission` (its legacy `permission` field reports maintain as write) - the reason is not empty - If several comments are valid, any one is enough, and the summary lists all of them. +- A rejected override is not silent. For each `/oc-review` comment that code rejects (any rejection except a comment from the bot itself), the summary shows one line under the callout, `A /oc-review override by (comment ) was ignored: `, with the reason in plain words per rejection code (`verdict.OV_REJECTION_TEXT`), and the job log has the same line. At most 3 lines show in the summary, then a count of the rest, so comment spam cannot grow it. The line is display only: it never changes the verdict or the risk. - The publisher parses comments with a fixed pattern. No LLM reads them. The whole comment body must be the command line and nothing else. Comments from bots are ignored. Only `write` and `admin` in the legacy `permission` field count. - **To apply:** post the override, then re-run the newest run of the required workflow with the REST call `POST /repos/{owner}/{repo}/actions/runs/{run_id}/rerun`. The `gh` CLI rerun fails on ruleset-started runs (platform finding P0-3). Do not close and reopen the PR to apply an override: a reopen is a push epoch event, so it makes an override posted before it not count. This does not depend on `labeled` or `ready_for_review`. A re-run of an older run exits `superseded`, so it can only block. A re-run uses the same `GITHUB_SHA` and ref ([re-run workflows](https://docs.github.com/en/actions/how-tos/manage-workflow-runs/re-run-workflows-and-jobs)), and the publisher reads the live PR state anyway. A re-run also repeats the Claude calls. The override still applies, because it covers the code at that SHA pair in that push epoch, not one set of findings. - **Push invalidates:** a new commit has a new head SHA, and a force push or branch restore moves the epoch, so no earlier grant matches in a new run. Nothing needs cleanup. A head that returns to an earlier SHA is covered in "Which run counts". @@ -611,7 +612,7 @@ There is one build of the central workflow. Its gate job always applies every ru - **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 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. + - **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, and with no conditional or negating word before the phrase in its sentence ("If", "when", "unless", "whether", "would", "should", "not", "never" and the like). So "is not disabled", "is neither disabled nor turned off" "is enabled but X is disabled" and "If the dependency graph 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. @@ -704,12 +705,13 @@ token lifetime: 10 minutes | +-- any other ref -----------> environment `eval-test` required reviewer: the owner (irvingpop) - prevent self-review on + prevent self-review off (the owner is + also the run actor) (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. + 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. Live `eval-test` (read 2026-11-06, `GET /repos/OperationCode/.github/environments/eval-test`): one protection rule, `required_reviewers`, with the owner (irvingpop) as the only reviewer and "prevent self-review" **off**; no branch policy; admin bypass on. "Prevent self-review" is off because the sole reviewer is also the run actor, so it would block every approval. The approval is the control (see above). 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 ("prevent self-review" is off, see above); 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. @@ -875,7 +877,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). 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. [x] The `eval-test` federation rule `eval-test-federation-rule` (2026-11-06), like the eval rule, with the exact claim `environment: "eval-test"`, and its ID in `evals.py`. [ ] Test it as V27. 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" off (sole reviewer = run actor), for every other ref. [x] The `eval-test` federation rule `eval-test-federation-rule` (2026-11-06), like the eval rule, with the exact claim `environment: "eval-test"`, and its ID in `evals.py`. [ ] Test it as V27. 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/)). @@ -1028,8 +1030,9 @@ 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 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-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) fixed part of it: its revert of GitHub's DNS experiment depends on a feature flag fetched at run time. v2.22.4 (2026-11-06, pinned 2026-11-06, K-16) carries the safe-by-default fix (agent v0.18.1, one DNS-flag default change), so upstream issue 708 is fixed. Audit mode stays until the soak and block-mode runs of [.github issue 24](https://github.com/OperationCode/.github/issues/24) pass; issue 24 stays open, and the next steps are the soak and the block switch. 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: with the fixed harden-runner release (v2.22.4) pinned, 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). +- K-16. `harden-runner` age exception (owner). On 2026-11-06 the owner (irvingpop) approved pinning harden-runner v2.22.4 (`1d88384fd173af6ecb012dfd0264a306b711241a`, released 2026-11-06) before the 2-day age of P5-5, because the update is well scoped (agent v0.18.1, one DNS-flag default change) and the owner is in contact with the upstream developer. Every workflow pins it; egress stays in audit mode with the allowlists unchanged. Issue 24 stays open: the next steps are the soak and the block switch (K-14). P2: phase 2 rulings, 2026-10-27 (UTC). Where a P2- line changes an earlier line, it wins. diff --git a/review/src/oc_review/evals.py b/review/src/oc_review/evals.py index 27b51ba..c02ab1c 100644 --- a/review/src/oc_review/evals.py +++ b/review/src/oc_review/evals.py @@ -81,7 +81,8 @@ class EvalRule: 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. + # `eval-test`: any other ref; the owner is the required reviewer and approves + # each run (prevent self-review is off: the owner is also the run actor). # Rule `eval-test-federation-rule` (created 2026-11-06): a copy of the # `eval` rule with the claim `environment: eval-test`. It uses the # `eval` service account. diff --git a/review/src/oc_review/publish.py b/review/src/oc_review/publish.py index cc91e13..73f6537 100644 --- a/review/src/oc_review/publish.py +++ b/review/src/oc_review/publish.py @@ -737,18 +737,37 @@ def read_commit_signing(api: GitHubApi, ctx: PublishContext) -> CommitSigning | # 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. +# neutral. The same holds for text before the phrase in its sentence or +# clause: a conditional or hypothetical lead ("If the dependency graph is +# disabled, ...", "when", "unless", "whether", "would", "should", ...) or a +# negation ("not", "never", "no") gives no evidence (`_GRAPH_OFF_LEAD_RE`). _GRAPH_OFF_RE = re.compile( 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_LEAD_RE = re.compile( + r"\b(?:if|when|whenever|unless|whether|once|until|while|though|although|" + r"would|could|should|might|may|can|will|cannot|can't|not|no|never|neither|nor|" + r"isn't|wasn't|don't|doesn't|didn't|but|however|" + r"suppose|assuming|ensure|make\s+sure|check|verify|confirm)\b|[?\u2019']t\b", + re.IGNORECASE, +) +_CLAUSE_BREAK_RE = re.compile(r"[.!?;:\n]") _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)) + """True when the message has the affirmative phrase `_GRAPH_OFF_RE` and + the text before it, back to the start of its sentence, has no conditional + or negating word (`_GRAPH_OFF_LEAD_RE`).""" + for m in _GRAPH_OFF_RE.finditer(message): + lead = message[: m.start()] + breaks = list(_CLAUSE_BREAK_RE.finditer(lead)) + sentence = lead[breaks[-1].end():] if breaks else lead + if not _GRAPH_OFF_LEAD_RE.search(sentence): + return True + return False def dependency_graph_off_evidence(resp: collect.HttpResponse | None) -> bool: @@ -1778,6 +1797,32 @@ def _waiver_blocks(result: verdict.GateResult) -> list[_Block]: return _details("🔑 Waiver: how a maintainer clears this", [_Block(text)]) +IGNORED_OVERRIDE_SHOWN = 3 + + +def ignored_override_lines(result: verdict.GateResult) -> list[str]: + """One plain line per ignored `/oc-review` comment: the author, a link-free + comment id and why (`verdict.OV_REJECTION_TEXT`). Display only: the gate + never reads it. The author login is untrusted text, so it is escaped.""" + return [ + f"A `/oc-review` override by {_esc('login', c.author_login)} (comment {c.comment_id}) was ignored: " + f"{verdict.OV_REJECTION_TEXT[c.rejection or '']}." + for c in result.rejected_overrides + ] + + +def _ignored_override_blocks(result: verdict.GateResult) -> list[_Block]: + """Right under the callout, so a rejected override is not silent. At most + `IGNORED_OVERRIDE_SHOWN` lines, so comment spam cannot grow the summary.""" + lines = ignored_override_lines(result) + if not lines: + return [] + shown = lines[:IGNORED_OVERRIDE_SHOWN] + if len(lines) > len(shown): + shown.append(f"{len(lines) - len(shown)} more ignored override comment(s) are not listed.") + return [_Block("\n" + "\n\n".join(f"> {x}" for x in shown).replace("\n\n> ", "\n>\n> "))] + + def _title(text: str) -> _Block: return _Block(f"\n**{text}**\n") @@ -2210,6 +2255,7 @@ def _summary_blocks( _Block(verdict.STICKY_MARKER), _Block(_status_heading(result, model, rows) + "\n" + MISSION_LINE), _Block("\n" + _callout(result, rows, hard, payload is not None, facts)), + *_ignored_override_blocks(result), ] # The signing row is informational: it is not in the heading count or the callout. b += _findings_table([*rows, *_signing_row(facts)]) @@ -2607,6 +2653,8 @@ def publish( # d) exit code append_step_summary(step_summary, ctx, final, full=full, publish_error=publish_error) print(f"publish: result {final.status}, reasons {', '.join(sorted(final.codes)) or 'none'}") + for line in ignored_override_lines(final): + print(f"publish: {line}") if publish_error is not None: print(f"publish: {publish_error}; the run fails", file=sys.stderr) return 1 diff --git a/review/src/oc_review/verdict.py b/review/src/oc_review/verdict.py index 863ec61..c1b1148 100644 --- a/review/src/oc_review/verdict.py +++ b/review/src/oc_review/verdict.py @@ -131,6 +131,24 @@ OV_PR_AUTHOR = "pr-author" OV_NO_PERMISSION = "no-write-permission" +# Why a rejected `/oc-review` comment was ignored, in words for the summary and +# the job log. One entry per rejection code: a rule that changes later (who may +# override) changes its `_override_rejection` line and this text, nothing else. +# A bot-authored comment is the bot's own text, not an attempt, so it has no +# entry and is never shown. +OV_REJECTION_TEXT: Mapping[str, str] = MappingProxyType({ + OV_MALFORMED: "the comment is not in the form `/oc-review override `", + OV_EMPTY_REASON: "the reason is empty", + OV_NO_SNAPSHOT: "there is no valid reviewed snapshot to waive", + OV_WRONG_HEAD: "the head SHA is not the reviewed head", + OV_WRONG_MERGE_BASE: "the merge base SHA is not the reviewed merge base", + OV_TIMELINE_INCOMPLETE: "the PR timeline could not be read completely", + OV_BEFORE_EPOCH: "the comment is older than the last force push, reopen or base change", + OV_EDITED: "the comment was edited after it was posted", + OV_PR_AUTHOR: "the PR author cannot waive their own PR", + OV_NO_PERMISSION: "the author does not have write or admin access", +}) + # ---------------------------------------------------------------- inputs @@ -499,6 +517,12 @@ def waivable_only(self) -> bool: def valid_overrides(self) -> tuple[OverrideCandidate, ...]: return tuple(c for c in self.overrides if c.valid) + @property + def rejected_overrides(self) -> tuple[OverrideCandidate, ...]: + """Display only: `/oc-review` comments that were ignored, with the + reason in `OV_REJECTION_TEXT`. Never changes the verdict or the risk.""" + return tuple(c for c in self.overrides if c.rejection in OV_REJECTION_TEXT) + @property def waived(self) -> tuple[Reason, ...]: return self.reasons if self.override_applied else () diff --git a/review/tests/unit/fixtures/workflows/edits.yml b/review/tests/unit/fixtures/workflows/edits.yml index 76490bc..a6c89dc 100644 --- a/review/tests/unit/fixtures/workflows/edits.yml +++ b/review/tests/unit/fixtures/workflows/edits.yml @@ -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@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" + old: " steps:\n - name: Harden the runner\n uses: step-security/harden-runner@1d88384fd173af6ecb012dfd0264a306b711241a # v2.22.4, released 2026-11-06\n with:\n # Audit until the issue 24 soak and block-mode runs pass; upstream issue 708 is fixed in v2.22.4 (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 @@ -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@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" + 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@1d88384fd173af6ecb012dfd0264a306b711241a # v2.22.4, released 2026-11-06\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@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" + 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@1d88384fd173af6ecb012dfd0264a306b711241a # v2.22.4, released 2026-11-06\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@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" + 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@1d88384fd173af6ecb012dfd0264a306b711241a # v2.22.4, released 2026-11-06\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 @@ -648,6 +648,21 @@ old: " timeout-minutes: 5\n" new: " timeout-minutes: 30\n" expect: timeout +- id: eval-workflow-default-shell + file: eval.yml + old: "jobs:\n" + new: "defaults:\n run:\n shell: sh\n\njobs:\n" + expect: eval-environment +- id: eval-job-default-shell + file: eval.yml + old: " needs: select-environment\n" + new: " needs: select-environment\n defaults:\n run:\n shell: sh\n" + expect: eval-environment +- id: eval-step-shell + file: eval.yml + old: " run: uv sync --locked --no-dev --project review\n" + new: " shell: sh\n run: uv sync --locked --no-dev --project review\n" + expect: eval-environment - id: eval-needs-removed file: eval.yml old: " needs: select-environment\n" diff --git a/review/tests/unit/test_comment_format.py b/review/tests/unit/test_comment_format.py index ce6be4f..ca26d51 100644 --- a/review/tests/unit/test_comment_format.py +++ b/review/tests/unit/test_comment_format.py @@ -186,6 +186,8 @@ def test_without_positive_evidence_the_row_stays_neutral(tmp_path, case): "The dependency graph for this repository is disabled.", "Dependency graph has been turned off for this repository.", "Dependency graph is currently disabled.", + "Dependency graph is not enabled. Contact your admin.", + "Review failed. Make sure it is set up: the dependency graph is disabled for this repository.", ]) def test_evidence_messages(message): resp = HttpResponse(403, error_body(message), "https://api.github.com/x", {}) @@ -209,6 +211,14 @@ def test_evidence_messages(message): "Dependency graph was not disabled.", "Dependency graph is reportedly disabled.", "Dependency graph is hardly ever turned off.", + # Text before the phrase that makes it conditional or hypothetical. + "If the dependency graph is disabled, enable it in the settings.", + "Check whether the dependency graph is disabled for this repository.", + "Unless the dependency graph is disabled, this call returns the diff.", + "When the dependency graph is not enabled, the API would answer 403.", + "Your admin should check that the dependency graph is disabled.", + "This does not mean the dependency graph is disabled for this repository.", + "It is not true that the dependency graph is disabled.", ]) def test_negated_or_joined_messages_are_not_evidence(message): """Display only: a negated state, or a state from another clause, keeps diff --git a/review/tests/unit/test_publish.py b/review/tests/unit/test_publish.py index f2a8e61..5b590b1 100644 --- a/review/tests/unit/test_publish.py +++ b/review/tests/unit/test_publish.py @@ -408,6 +408,41 @@ def test_clean_run_passes_and_posts_summary(tmp_path): assert r.record["result"] == "pass" +def test_rejected_override_is_shown_in_summary_and_log(tmp_path, capsys): + """A rejected override is not silent: one line in the summary and one in + the log. The gate result does not change.""" + passes = clean_passes(security=[finding("security", "high", title="Shell call")]) + gh = FakeGitHub(issue_comments=[override(7, AUTHOR)]) # the PR author + r = run(tmp_path, gh, passes=passes) + assert r.code == 1 # still fails: the override was not valid + line = "A `/oc-review` override by contributor (comment 7) was ignored: the PR author cannot waive their own PR." + assert line in plain(r.summary) + assert f"publish: {line}" in capsys.readouterr().out + + +def test_override_without_permission_is_ignored_with_reason(tmp_path): + gh = FakeGitHub(issue_comments=[override(8, "drive-by")], permissions={"drive-by": "read"}) + r = run(tmp_path, gh, passes=clean_passes(security=[finding("security", "high")])) + assert r.code == 1 + assert "(comment 8) was ignored: the author does not have write or admin access." in plain(r.summary) + + +def test_valid_override_shows_no_ignored_line(tmp_path, capsys): + r = run(tmp_path, FakeGitHub(issue_comments=[override(1)]), + passes=clean_passes(security=[finding("security", "high")])) + assert r.code == 0 + assert "was ignored" not in r.summary + assert "was ignored" not in capsys.readouterr().out + + +def test_ignored_override_lines_are_capped_in_the_summary(tmp_path): + comments = [override(i, AUTHOR) for i in range(10, 16)] + r = run(tmp_path, FakeGitHub(issue_comments=comments), passes=clean_passes(security=[finding("security", "high")])) + s = plain(r.summary) + assert s.count("was ignored") == publish.IGNORED_OVERRIDE_SHOWN + assert "3 more ignored override comment(s) are not listed." in s + + def test_summary_has_every_required_field(tmp_path): passes = clean_passes(security=[finding("security", "high", title="Shell call")]) gh = FakeGitHub(issue_comments=[override(1)]) diff --git a/review/tests/unit/test_workflows.py b/review/tests/unit/test_workflows.py index cf998cb..0637423 100644 --- a/review/tests/unit/test_workflows.py +++ b/review/tests/unit/test_workflows.py @@ -608,6 +608,25 @@ def environment_name(value: Any) -> str | None: return name +def eval_shell_violations(wf: Workflow, r: "Report") -> None: + """The select script means what it says only under the default shell. A + shell set at the workflow level (`defaults.run.shell`), the job level + (`defaults.run.shell`) or in a step (`shell:`) can change that, so eval.yml + sets none of them.""" + def set_shell(node: Any) -> bool: + run = node.get("defaults", {}).get("run") if isinstance(node.get("defaults"), dict) else None + return isinstance(run, dict) and "shell" in run + + if "defaults" in wf.doc: + r.add("workflow", "eval-environment", "no `defaults:` in eval.yml (a workflow default shell changes the select script)") + for name, job in jobs(wf.doc).items(): + if "defaults" in job or set_shell(job): + r.add(name, "eval-environment", "no `defaults:` in an eval.yml job") + for st in steps(job): + if "shell" in st: + r.add(name, "eval-environment", "no step `shell:` in eval.yml") + + 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 @@ -658,6 +677,7 @@ def eval_boundary_violations(directory: Path) -> list[str]: if wf.doc.get("permissions") == "write-all": r.add("workflow", "eval-permissions", "write-all at workflow level") eval_select_violations(wf, r) + eval_shell_violations(wf, r) for name, job in jobs(wf.doc).items(): if "environment" not in job: continue @@ -887,6 +907,20 @@ def test_publish_stub_check_fails_on_hidden_exit(case, tmp_path): assert code == 0 +HARDEN_RUNNER_PIN = "step-security/harden-runner@1d88384fd173af6ecb012dfd0264a306b711241a" # v2.22.4 (spec K-16) + + +def test_every_harden_runner_step_pins_v2_22_4(): + """Issue 24 / K-16: one harden-runner pin everywhere, with its tag in the comment.""" + count = 0 + for path in workflow_files(WORKFLOWS): + for line in path.read_text(encoding="utf-8").splitlines(): + if "step-security/harden-runner@" in line: + count += 1 + assert line.split("uses:")[1].strip().startswith(HARDEN_RUNNER_PIN + " # v2.22.4"), f"{path.name}: {line}" + assert count == 7 + + @pytest.mark.release def test_harden_runner_egress_lists_are_the_v10_lists(): """V10 and K-14: audit mode, and each central job keeps the list that V10 diff --git a/review/tools/release.py b/review/tools/release.py index c6eff79..9a82f0f 100644 --- a/review/tools/release.py +++ b/review/tools/release.py @@ -78,8 +78,9 @@ # name-bound stand-in for the review, which a same-name job can satisfy. REVIEW_CHECK_NAMES = ("oc-claude-review", "claude of duty") # The run each release needs at the SHA: (file, event, branch). test.yml runs -# on the push to main; the release eval is an owner-approved dispatch on main -# (as for cc710fb, cf19216 and 96f4966). A pull_request run at the same head +# on the push to main; the release eval is a dispatch on main, which needs no +# approval (the `eval` environment allows only main; earlier releases such as +# cc710fb, cf19216 and 96f4966 were approved by the owner). A pull_request run at the same head # SHA tested a merge ref, so it does not count. RELEASE_WORKFLOWS: tuple[tuple[str, str, str], ...] = (("test.yml", "push", "main"), ("eval.yml", "workflow_dispatch", "main"))