diff --git a/docs/build-plan.md b/docs/build-plan.md index 0169ffc..5336b49 100644 --- a/docs/build-plan.md +++ b/docs/build-plan.md @@ -13,7 +13,7 @@ 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. - [ ] Every rule in the [CLAUDE.md](../CLAUDE.md) holds, and the unit tests check the ones that code can check. -- [ ] The owner has accepted the residuals in writing (spec "Which run counts"). +- [x] The owner has accepted the residuals in writing (spec "Which run counts"; P5-8, 2026-11-01). ## Phases @@ -57,7 +57,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" is off temporarily (owner, 2026-10-26), so that the coordinator, which runs with the owner's token, can approve eval runs it started from reviewed branches; the owner turns it on again in phase 5. 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 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. - [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. @@ -187,7 +187,7 @@ Exit criteria: - [x] Owner: record the pilot scope. The plan named only `back-end`; the owner added `front-end` (135777271), `operationcode-pybot` (146210805, default branch `master`) and `operationcode_infra` (107068150), with `back-end` (182584277). Four repos (spec P4-1). - [x] Owner: before a repo joins the pilot, give it its own approval protection on the default branch: a repo ruleset or branch protection with at least 1 approval, code owner review and dismiss stale approvals on push. Done 2026-10-28: "Pilot approval protection" on `back-end` (26057791), `operationcode-pybot` (26057793) and `operationcode_infra` (26057794), with the Maintain, Admin and org admin bypass because pybot and infra have one active committer; `front-end`: the existing "Protect main" (25634150), only `require_code_owner_review` changed to true (spec P4-3). -- [x] Owner: before a repo joins the pilot, check it for people or bots that push to the default branch directly (V25). Done: none in 12 months on `back-end`, `operationcode-pybot` and `operationcode_infra`; `front-end` had a force push on 2026-10-28 that rewrote three merged PRs (cleanup of a malicious dependency payload), not new unreviewed code (spec P4-4). +- [x] Owner: before a repo joins the pilot, check it for people or bots that push to the default branch directly (V25). Done: none in 12 months on `back-end`, `operationcode-pybot` and `operationcode_infra`; `front-end` had one history rewrite on 2026-10-28 that changed no reviewed content (spec P4-4). - [ ] Owner: accept that during the pilot a maintainer or repo admin can merge past any failed run, also on their own PR, with the approval that the baseline still requires (spec "Pilot and enforcement"). - [x] Owner: confirm that the federation rule named in the release code has `job_workflow_ref` at the release SHA. Read back by the owner at M4 `9e2795b` (2026-10-29, spec P4-5). - [x] Owner: every pilot repo sends the immutable OIDC subject (spec "Immutable subject", P4-6). Found 2026-10-29: repos made before the GitHub default sent `repo:OperationCode/:...`, the rule rejected them (`match_subject_prefix`), and the first pilot runs failed closed as designed. Fix per repo: `PUT repos/OperationCode//actions/oidc/customization/sub` with `use_default` true and `use_immutable_subject` true. Before it, check the other `id-token` users in the repo and their relying-party trust: `back-end` and `operationcode-pybot` use a cloud OIDC trust policy that gained the immutable subjects (owner; old values kept). After it, all four test PRs passed (all four jobs `success`). @@ -213,31 +213,32 @@ Exit criteria: ## Phase 5: before org-wide enforcement -- [ ] Owner: accept in writing the residuals in the spec's "Which run counts": an earlier pass on the same SHA, and run deletion (V6 f confirmed it as a Write user). +- [x] Owner: accept in writing the residuals in the spec's "Which run counts": an earlier pass on the same SHA, and run deletion (V6 f confirmed it as a Write user). Done 2026-11-01 (spec P5-8). - [x] Owner: baseline ruleset 25787833 (spec P5-1): `deletion`, `non_fast_forward`, `required_signatures`, `pull_request` (1 approval, dismiss stale approvals on push, Code Owners review, squash only); `~ALL`; `OrganizationAdmin` `pull_request` bypass only. Read back after each target change. - [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). -- [ ] Owner: turn "prevent self-review" back on for the `eval` environment (it is off during phase 2), and decide whether admin bypass stays on. -- [ ] Owner: Dependabot alerts with npm malware detection on active repos. -- [ ] 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. -- [ ] 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. +- [x] Owner: "prevent self-review" is on for the `eval` environment (2026-11-01). Admin bypass stays 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. Depends on: phase 4; V6, V8, V22, V23. Exit criteria: -- [ ] The written acceptance exists. -- [ ] The baseline read-back matches the P5-1 checklist, and the bypass matrix passed. -- [ ] The re-run tests pass. -- [ ] The owner's open security items are done or deferred, with the decision recorded. +- [x] The written acceptance exists (spec P5-8, 2026-11-01). +- [x] The baseline read-back matches the P5-1 checklist, and the bypass matrix passed (2026-11-04, spec P5-10, P5-11). +- [x] The re-run tests pass (2026-11-02, spec P5-9). +- [x] The owner's open security items are done or deferred, with the decision recorded: `eval` prevent self-review on, malware alerts on, harden-runner block mode deferred (K-14). Phase 5 exit met 2026-11-04 (spec P5-11). ## 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". Exit criteria: diff --git a/docs/claude-review-design.md b/docs/claude-review-design.md index 60fce5f..89e3956 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. The environment keeps the owner as required reviewer; "prevent self-review" is off during phase 2, and the coordinator's approval stands 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. 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"). | | 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. | @@ -170,6 +170,7 @@ Facts about ruleset-required workflows ([troubleshooting rules](https://docs.git | - re-validate the snapshot; stale -> fail, write nothing more | | - read override comments and push epoch, check authority now | | - compute one gate result (fail closed) | + | - display-only reads: PR commit signatures, Dependency graph probe (P6-1) | | - post inline comments (event: COMMENT), then the sticky summary | | - final check: snapshot, authority, no newer run on the head SHA | | - exit 0 only when the gate result is still pass | @@ -584,7 +585,7 @@ There is one build of the central workflow. Its gate job always applies every ru ## Publishing (code) -- **Escape all untrusted text** before posting: strings from Claude and strings from the PR (title, paths, package names, URLs, prefilter evidence). Remove control characters, replace `@` with `@​` (no pings), render as text with no Markdown links, images, HTML or autolinks, and truncate to fixed limits. Redact credential-like strings. Escaping also breaks issue references (`#123`, `GH-123`) and runs of 7 or more hex characters, so no text becomes a cross-reference or a commit link. +- **Escape all untrusted text** before posting: strings from Claude and strings from the PR (title, paths, package names, URLs, prefilter evidence). Remove control characters, replace `@` with `@​` (no pings), render as text with no Markdown links, images, HTML or autolinks, and truncate to fixed limits. Redact credential-like strings. Escaping also breaks issue references (`#123`, `GH-123`) and runs of 7 or more hex characters, so no text becomes a cross-reference or a commit link. File paths, and the code-made reason details, show as plain text in a code span, not as backslash-escaped prose (P6-1): the same cleaning (control characters removed, redacted, cut to the field limit), then references and hex runs broken, a backtick replaced with `'`, `<` replaced with `‹`, and `|` escaped so a table cell stays whole; a backslash run right before a pipe is split from it with a zero-width space, so every pipe has exactly one escaping backslash (table parsers differ on two). A code span renders no Markdown, link or HTML. - **Label model output:** each pass's text sits under a note that it is model output from untrusted input, to check before acting on it. The note stands at the top of the finding details and above the model summaries, both in the collapsed record. A finding title in the visible findings table is model text without the note: its "Found by" cell names the model pass that reported it. - **Inline comments:** one review per run, with `event: COMMENT` and `commit_id` set to the bundle's head SHA, for verified and inline-eligible findings only. Each finding has two keys, both computed from the shown values: each string redacted and cut to its field limit as the publisher shows it, never the raw value, so a published key is not a guess check for a redacted secret. The D14 key is the SHA-256 of file, side, title and quote, joined by NUL. The full key is the SHA-256 of the JSON list of every field that the reader sees: pass, category, severity, confidence, file, side, line, title, quote and explanation (Python `json.dumps` with its default settings, which gives the same ASCII text for the same values). Two findings share a full key only when they show the same text in every one of these fields. Both keys are kept in a hidden `` at the start of each inline comment and read back only from review comments by `github-actions[bot]` whose body starts with that marker and holds no other; a marker with one key is not read, so its finding is posted again. The login proves only that some Actions workflow in the repo wrote the comment (as for the summary), and the keys are public hashes of shown text, so the marker alone proves nothing: the code also checks the text of the comment (review r-p3-cut-final Important 2). In one run, findings are deduped by full key, so each distinct finding gets its own inline comment, even when a cut or a redaction makes two findings equal in file, title and quote (P3-2). Across runs, a finding is not posted again when an earlier comment has its D14 key, the same path and side, and the layout of an inline comment (the marker, a finding line of the schema enums and a pass name, the finding's escaped title, an explanation and the fixed note); the line and the explanation can differ, because the D14 key covers neither. The model words an explanation again on each call and a push moves lines, so the full key of the same issue is rarely the same on two runs, and the D14 key is what keeps a re-run or a push from posting every finding again. The summary points to the earlier comment only when that comment is, in full, the body this run would write for the finding (so its full key matches too), at the same path, side and line (`line`, or `original_line` when GitHub marks the comment outdated). Otherwise the summary shows the finding in full (title, severity, place, explanation and quote), marked as different from the inline comment of an earlier run. So no explanation is hidden behind an earlier comment that says something else, and a forged comment with the D14 key can at most move a finding from its inline comment to its full summary block. That block is subject to the summary size limit below: a high or critical finding is never cut, but a low or medium one can be cut at the largest sizes (residual, see below). If nothing is new, no review is posted. If GitHub rejects the review (HTTP 422, or any other failure), the code does not retry or split it: no inline comment is posted for this run, and the summary shows all of its findings in full. The gate does not change. - **Summary comment:** one sticky comment, found by the hidden marker `` and an author check (the `github-actions[bot]` identity). A comment with the marker from any other author is ignored. The author check proves only that some Actions workflow in the repo wrote it, so the summary is never a trust source for the gate (see "Summary writes"). The comment is built for a fast human judgment: a reader gets the result and the next step in about three seconds, and everything else is one click away. It never says "approved" and never implies approval; the footer line under the provenance says "Claude of Duty never approves a PR. A human reviewer gives the final approval." and nothing else in the comment repeats it. The override command and the links to override comments are built from trusted values (server URL, repository, PR number, comment ID), never from a field the API returned. The layout, from top to bottom: @@ -601,19 +602,22 @@ There is one build of the central workflow. Its gate job always applies every ru
Full review record
7. everything else, collapsed ``` - - **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, and model finding rows, so it equals what the body lists before a size cut); 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. + - **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. - - **Findings table:** one row per place, worst first: hard blocks (🚫), passes or jobs that did not run (⚠️), 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 table is left out when there are no rows. + - **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. + - **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. - **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 and coverage. It is never cut. + - **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. - - **Collapsed record:** the reviewed snapshot (full head SHA, merge base SHA, base ref, run), every reason with its waivable flag as a table with plain labels, the maintainer waivers (author, time, reason, comment link, applied, waived reasons), the code checks (hard block evidence, coverage with every unavailable file, dependency changes, dependency-review job), the signals (context only, never block; the first-time contributor signal keeps its caveat), the dependency changes, the finding details (a per-pass table with category, confidence, citation state, posting state and each pass's own title, plus the explanation and quote of every finding that has no inline comment and the posting state of each), the model summaries (one row per pass, with status, model and the escaped summary, or "No summary. Reason: ..."), and the token usage per pass. Usage is token counts only (input, output, cache write, cache read, attempts); a missing value reads "not recorded", never zero, and the total reads "Recorded total" when any value is missing. The code holds no price and the repo shows no currency. + - **Collapsed record:** the reviewed snapshot (full head SHA, merge base SHA, base ref, run), every reason with its waivable flag as a table with plain labels (one row per cause, every detail in a code span), the maintainer waivers (author, time, reason, comment link, applied, waived reasons), the code checks (hard block evidence, coverage with every unavailable file, dependency changes, dependency-review job), the signals (context only, never block; the first-time contributor signal keeps its caveat), the dependency changes, the finding details (a per-pass table with category, confidence, citation state, posting state and each pass's own title, plus the explanation and quote of every finding that has no inline comment and the posting state of each), the model summaries (one row per pass, with status, model and the escaped summary, or "No summary. Reason: ..."), and the token usage per pass. Usage is token counts only (input, output, cache write, cache read, attempts); a missing value reads "not recorded", never zero, and the total reads "Recorded total" when any value is missing. When no pass recorded any count (for example every pass failed before its call, or the size gate skipped them), the table collapses to one line, "No token usage was recorded for any pass." (P6-1). The code holds no price and the repo shows no currency. - **Size limit and cut order:** a comment body is at most 65,536 characters, and the code aims at 64,000. The collapsed blocks count toward the limit. If the body is too long, it cuts only these blocks, in this order, each from its end: unverified low and medium findings, low and medium findings, dependency changes, prefilter signals. A cut removes whole table rows, list items or detail bullets, never part of one, and a merged findings row is cut only when every member is cuttable. A group that lost items shows one line with the exact phrase "N item(s) truncated, see the run log", as a table row inside a table so that the table stays whole, and the provenance line adds "Some details were truncated, see the run log." (its size is reserved up front). The `
` open and close lines are never cut, so the tags stay balanced. The heading, the callout, the findings rows for hard blocks and for high and critical findings (verified or not, so also one whose file path analyze cut), the checks strip, the provenance line, the snapshot, the reasons, the waiver block, the override data, the hard block evidence, the coverage rows, the model summaries and the token usage are never cut. If those alone do not fit, the run fails closed: the comment becomes a short notice of fixed size (heading `## 🔴 Comms down: Claude of Duty could not post its full report`, mission line, a `[!CAUTION]` callout with the size note, the computed gate result, the snapshot, a table with a count per reason with its waivable flag, the number of valid overrides and the run link), and the job exits 1. This can turn a computed `pass` into a failed run. Residual (review r-p3-cut-final Important 1, coordinator ruling 2026-10-27): there is one summary comment and no continuation comment. So on a re-run whose findings changed keys at the largest allowed sizes, the cut can drop the explanation of a low or medium finding from the comment (it stays in the run log), and the never-cut high and critical blocks can exceed the limit, so the run fails closed even though the computed risk did not change. Bounded continuation comments that hold every summary-only finding are planned (build plan, phase 4). - **Full record:** whenever the comment is cut, the publisher prints the full redacted record (reasons, overrides, every finding, prefilter results, coverage, dependency delta) to the gate job log, between `::stop-commands::` and `::::` so that untrusted text cannot run workflow commands. The record also goes to the step summary when it is at most 512 KiB; over that, the step summary says that the record is in the job log. The step summary follows the comment as it stands at the end of the run: when the summary is updated after the final check and the update is not cut, the step summary has no full record; when that update fails, it keeps the record of the first, cut summary. "The run log" in each cut note means this job log. - **Gate:** the required run's result. No code calls the Checks API, and no job has `checks: write`. -- **Order of writes:** inline comments, then the summary, then the final check, then the exit code. Every write, including the update of the summary after the final check, is preceded by the snapshot check. If the check shows a stale run, or cannot run (a failed read), the code writes nothing more and exits 1. The step summary line of such a run has `result: fail` and the `stale` reason code, whatever the gate result computed before. Only the code's own write targets exist: `POST .../pulls/{n}/reviews` with `event: COMMENT`, `POST .../issues/{n}/comments` and `PATCH .../issues/comments/{id}`. Any other write is refused in code. +- **Order of writes:** inline comments, then the summary, then the final check, then the exit code. Every write, including the update of the summary after the final check, is preceded by the snapshot check. If the check shows a stale run, or cannot run (a failed read), the code writes nothing more and exits 1. The step summary line of such a run has `result: fail` and the `stale` reason code, whatever the gate result computed before. Only the code's own write targets exist: `POST .../pulls/{n}/reviews` with `event: COMMENT`, `POST .../issues/{n}/comments` and `PATCH .../issues/comments/{id}`. Any other write is refused in code. The display-only reads (the PR commit list and, after a dependency-review `failure`, the dependency comparison probe) run after the inline review and before the snapshot check that precedes the summary write; they are not inputs to `compute_gate` or the final check. - **No valid bundle:** if the bundle cannot be loaded, the publisher has no snapshot to re-validate. It then checks only that the PR head still equals the event's head. If it does, it posts the summary, and the gate fails for `artifact` and `stale` (not waivable). If the head moved, it writes nothing. - **Refused summary write:** if the first write of the summary fails, the publisher still runs the final reads, then exits 1. The step summary line has `result: fail` and a `publish_error`. This is not a gate reason, so no override applies. - **Never** approve, request changes, merge, close, label or edit the PR. @@ -684,8 +688,8 @@ token lifetime: 10 minutes - **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 off temporarily (owner decision, 2026-10-26), so that the coordinator can approve eval runs it started from reviewed branches. The owner turns it on again later (phase 5). Admin bypass stays on. During phase 2 the owner has delegated the approval to the coordinator, which runs with the owner's token: the coordinator's approval of an eval run it started from a reviewed branch stands in for the owner's approval. - - 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; during phase 2 the coordinator's approval stands in for it under the owner's delegation, and "prevent self-review" is off until phase 5. + - **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. - **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. @@ -735,7 +739,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, 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` 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"). - **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`. @@ -842,7 +846,7 @@ The tasks, owner actions and exit criteria for each step are in [docs/build-plan 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. -7. **Before org-wide enforcement:** the owner accepts in writing 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. +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. ## Admin checklist (Irving) @@ -851,7 +855,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" off during phase 2 (owner, 2026-10-26; back on in phase 5). 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). 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. - [ ] 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/)). @@ -862,7 +866,7 @@ The tasks, owner actions and exit criteria for each step are in [docs/build-plan - [ ] 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). -- [ ] 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. Repo writers and admins can cancel, re-run and delete runs; no setting in this design limits that. +- [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 @@ -971,7 +975,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-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. During phase 2 "prevent self-review" is off and the coordinator's approval stands 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-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. - K-7. Code checks on a size-gated run (review W6 Important 1). A `skipped-size` bundle had a `null` payload, so it could not carry the prefilter results, and the gate could not tell an empty hard-block list from missing evidence. Now the size gate skips only the Claude calls. A `skipped-size` bundle carries a payload with the prefilter results, the coverage record and the dependency delta, and leaves out only the content for Claude. The payload stays `null` for a `pass` record that is not `ok`, and for a `bundle` that is `failed` or `stale`. This changes the payload rule in K-5. The gate takes the prefilter result as a checked input with a state (complete or unavailable) bound to the head SHA and merge base SHA. Missing, incomplete or unbound evidence fails the gate, not waivable. The gate uses every hard block from the bundle payload and from that input, and never removes one. @@ -1014,7 +1018,7 @@ P2: phase 2 rulings, 2026-10-27 (UTC). Where a P2- line changes an earlier line, - P2-3. The smoke case moves out of the live fixtures to `review/tests/unit/fixtures/eval-smoke`, so a paid run holds only the minimum set. - P2-4. Pass bar for the model-judged cases: one full run with all 13 cases matched closes the phase 2 exit criterion. A miss is fixed in the prompt or the code, and the full set runs again. An expectation changes only with a written reason in the PR. Result: run 1 on 2026-10-27, [run 40445885087](https://github.com/OperationCode/.github/actions/runs/40445885087), commit 3a03ae8, matched 13 of 13 cases with every pass `ok` and no miss. Tokens: 207890 input, 20729 output. The coordinator started it under the owner's delegation and approved it in the `eval` environment. The run uploaded no artifact, so the per-case `summary.md` was not kept (phase 2 follows the spec: the runner only writes it to `--out-dir`, and no spec line requires an upload; see the build plan). - P2-5. Expectation format v2 and the fixture builder ("Testing"). An exact severity failed a "high" case on a `critical` answer, and there was no way to state "no high findings", a citation side, verification, alternative categories or a redacted summary. Now severity means "or higher", and `max_severity`, `file`, `side`, `verified`, category lists and `summary_excludes` exist. A hand-written bundle made "hard block regardless of Claude" true by construction, so every fixture bundle is built by the real `collect()` and a unit test checks it against a rebuild. -- P3-1. Phase 3 rulings (owner and coordinator, 2026-10-27). Q1: a new production service account `claude-review` (`svac_01PiMgfb6qJgNpLZUV8hprCC`), separate from the test account, so test teardown cannot break production. Q2: the coordinator's approval of `eval` runs extends through phase 4; "prevent self-review" stays off until phase 5. Q3: test work in claude-review-test and `p3-*` branches in `.github` is approved; the non-member fork author (account X) is pending, and such an account becomes an outside collaborator, not an org member, when given repo access. Q4: `test.yml` runs `pytest tests/unit -m release` on every PR, in the `unit-tests` job. Q5: no workflow change on main for V22 a; the evidence is `job.workflow_sha` against the pin. Q6: one eval, at the candidate commit only. Q7 (design change): no Console edit lowers the test workspace spend limit; the exhausted-budget outage case is covered by V9 (phase 0) and the unit tests; the invalid federation rule case still runs live (denied exchange, V26 b). V26 a: R1 was created at the placeholder SHA and the Console accepted it (2026-10-27); R2 was created the same way. +- P3-1. Phase 3 rulings (owner and coordinator, 2026-10-27). Q1: a new production service account `claude-review` (`svac_01PiMgfb6qJgNpLZUV8hprCC`), separate from the test account, so test teardown cannot break production. Q2: the coordinator's approval of `eval` runs extends through phase 4; "prevent self-review" stays off until phase 5 (it is on since 2026-11-01). Q3: test work in claude-review-test and `p3-*` branches in `.github` is approved; the non-member fork author (account X) is pending, and such an account becomes an outside collaborator, not an org member, when given repo access. Q4: `test.yml` runs `pytest tests/unit -m release` on every PR, in the `unit-tests` job. Q5: no workflow change on main for V22 a; the evidence is `job.workflow_sha` against the pin. Q6: one eval, at the candidate commit only. Q7 (design change): no Console edit lowers the test workspace spend limit; the exhausted-budget outage case is covered by V9 (phase 0) and the unit tests; the invalid federation rule case still runs live (denied exchange, V26 b). V26 a: R1 was created at the placeholder SHA and the Console accepted it (2026-10-27); R2 was created the same way. - P3-2. Over-long strings (design change). Release eval [run 40517202830](https://github.com/OperationCode/.github/actions/runs/40517202830) at the candidate S1 (ccbae51) matched 12 of 13 cases: in `obfuscated-test-payload` the supply-chain pass failed with "response does not match the findings schema: 'findings'/1/'quote': [maxLength]". Phase 2 run 40445885087 matched the same case with the same prompts, so this is model variance. Cause: structured outputs cannot enforce `maxLength` (P0-12), so the API accepts a string of any length, and one long string made the whole pass `failed` and the gate fail on a real PR. Fix (coordinator ruling, 2026-10-27): before the full schema check, analyze cuts every string over its schema `maxLength` to that limit and keeps the finding. This covers `summary` (1500), `file` (400), `quote` (300), `title` (120) and `explanation` (1200), and any limit added to the schema later; a unit test checks that the cut reaches every `maxLength`. A file path is an identifier, not prose, so its cut differs: the first 399 characters and a final "/" (fix round 1, review r-p3-cut Important 1). A plain prefix of a 401-character path can be the path of another changed file, which could then verify the finding and place its inline comment on the wrong file. No file path ends in "/", and the file lookup never matches one, so a cut path names no file. The finding stays and still counts toward the gate. The system prompt also tells the model to copy the part of a longer line that shows the problem. Why it does not lower risk: a finding is never dropped, and its category, severity, confidence, side and line never change. Blocking never depends on the text, the quote or verification ("Citation check"). Whitespace normalization keeps a prefix of a quote inside the normalized text wherever the whole quote is, so a quote that verified still verifies. A cut quote that is not at that place, a cut quote that is only whitespace or holds a redacted credential, or a cut file path, stays unverified and still blocks. A cut can make two findings that differed past the cut share file, title and quote (review r-p3-cut Important 2). A redaction before the cut can do the same far below the limit: two titles that differ only in a credential both become `Token [REDACTED:jwt]` (fix round 2, review rr-p3-cut-fix1 Important A). Two findings below every limit can also share file, side, title and quote and differ in explanation or severity. So each inline comment carries two keys ("Publishing (code)", fix round 3, review rr-p3-cut-fix2): the D14 key (file, side, title and quote) and a full key of every field that the reader sees, both of the shown, redacted text. In one run, findings are deduped by full key, so each distinct finding gets its own inline comment. Across runs, a finding whose D14 key is already on the PR is not posted again; when its full key matches none of the earlier comments with that D14 key, the summary shows it in full. So none hides the explanation of another, and a re-run or a push does not post every finding again (fix round 2 keyed only on every shown field, which re-posted almost every finding on each run, because the explanation and the line change). Fix round 4 (review r-p3-cut-final, coordinator rulings 2026-10-27): the marker alone is not proof that a comment shows a finding, because any Actions workflow in the repo writes as `github-actions[bot]` and the keys are public hashes. The D14 skip needs the inline layout, path, side and title, and "see the earlier comment" needs the exact body at the same line; a forged comment can at most move a finding to its full summary block. A cut file path makes a finding unverified, and an unverified high or critical finding now keeps its never-cut rank in the summary, so the summary shows every such explanation or the run fails closed. The residual of one summary comment (a low or medium explanation can be cut on a changed-key re-run at the largest sizes; capacity can fail a run closed) is stated in "Publishing (code)"; continuation comments are planned for phase 4. Findings that are equal in every shown field after the cut show one inline comment; the publisher cannot know the text past the cut, and that comment holds all the text of each. A credential-like string in an over-long string is redacted before the cut, so the cut cannot leave part of a secret that the publisher would then not recognize. The publisher escapes every character of a cut string, so a cut inside Markdown, HTML or an escape still shows as text (unit test). A cut title, explanation or summary has no truncation mark, because the publisher shows one only for its own cuts. A new candidate with this change needs its own eval (P0-15). - P3-3. Phase 3 results and spec corrections (2026-10-27). Four spec claims did not survive the live tests, and each is now corrected in place. (1) The merge box counts only runs at the pinned SHA, not the newest run on the head SHA; a pin move back revives older passes at that SHA (PR 28 on 2026-10-27 at 18:34:53Z; PRs 28 and 37 at 21:38:44Z). A forward move fails closed (the box blocks at once). The rollback is a residual, covered by the rule to start a new run on each open PR after any pin move; it joins the residuals in rollout step 7. (2) Repo Actions settings do not affect the required run (V12 pass); the role table no longer lists them as an admin power against the gate. (3) A normal cancel before the gate job starts does not stop the `always()` gate job (run 40565538379), which then writes a `pass` summary next to a `cancelled` run; a normal cancel while the gate job runs was not tested; the run conclusion still blocks the box, and a force-cancel stopped the gate job after the summary (run 40565910355). (4) The artifacts of earlier attempts are gone after a re-run; the gate reads only its own attempt, so it still fails closed, and the residual is that earlier pass records cannot be read back. Release procedure: two Console edits per release (candidate SHA, then merge SHA), a tree check of the merge SHA, and a denied exchange at the merge SHA before the second edit. Account X results came later (P3-6); the web UI "Re-run all jobs" case stays NOT RUN. The exhausted-budget outage case stays covered by V9 and the unit tests (Q7). - P3-4. Model per pass, Sonnet 5.5 (2026-10-27). The owner proposed `claude-sonnet-5-5` for general use. The model is now a per-pass setting, and every pass uses Sonnet 5.5. The switch needed a full match of the 13 cases (P2-4 bar); Opus 5.5 would stay on any pass where Sonnet missed. Result: two eval runs at 302882f, [40589761270](https://github.com/OperationCode/.github/actions/runs/40589761270) and [40590079545](https://github.com/OperationCode/.github/actions/runs/40590079545), each matched 13 of 13 cases with no failed pass (Opus reference [run 40528717338](https://github.com/OperationCode/.github/actions/runs/40528717338), also 13 of 13). Against Opus, total tokens were 0.97x and 0.98x, output tokens 0.71x and 0.76x, and eval wall time 84 s and 82 s against 138 s. No currency figures were recorded. The switch takes effect at the next release: the production rule pins a SHA, so merging does not change production. @@ -1033,14 +1037,14 @@ P4: phase 4 pilot start, 2026-10-28 and 2026-10-29 (owner and coordinator). The - P4-1. Pilot scope (owner decision). The build plan named only `back-end`. The owner added three repos, so the pilot has four: `back-end` (182584277, `main`), `front-end` (135777271, `main`), `operationcode-pybot` (146210805, `master`) and `operationcode_infra` (107068150, `main`). - P4-2. Pilot ruleset. Org ruleset `claude-review-pilot` (id 26132750): active; targets the four repo ids and `~DEFAULT_BRANCH`; one workflows rule with `repository_id` 1432342572, path `.github/workflows/claude-review.yml`, `sha` `9e2795b97b22869b9e5c7a777e71a2afa8366eeb` (M4), no `ref`, `do_not_enforce_on_create` false; bypass `RepositoryRole` 2 (Maintain), `RepositoryRole` 5 (Admin) and `OrganizationAdmin`, all in `pull_request` mode. It was created disabled, read back, then enabled. Rollback: `PUT` with `enforcement` set to `disabled`. - P4-3. Approval protection and CODEOWNERS. Repo rulesets "Pilot approval protection" on `back-end` (26057791), `operationcode-pybot` (26057793) and `operationcode_infra` (26057794): 1 approval, code owner review, dismiss stale reviews, and the same bypass as the pilot ruleset. The reason for the bypass: `operationcode-pybot` and `operationcode_infra` have one active committer, so a rule without a bypass would block every merge. On `front-end` the existing ruleset "Protect main" (25634150) changed in one field only: `require_code_owner_review` false to true. CODEOWNERS was added on each default branch (`back-end`: @irvingpop @kylemh; `front-end`: @kylemh @irvingpop; `operationcode-pybot` and `operationcode_infra`: @irvingpop). -- P4-4. Direct-push check (V25), 12 months: none on `back-end`, `operationcode-pybot` and `operationcode_infra`. `front-end` had a force push on 2026-10-28 that rewrote three merged PRs, a cleanup of a malicious dependency payload; it was not new unreviewed code. +- P4-4. Direct-push check (V25), 12 months: none on `back-end`, `operationcode-pybot` and `operationcode_infra`. `front-end` had one history rewrite on 2026-10-28 that changed no reviewed content. - P4-5. Org 2FA. The owner turned on the org 2FA requirement in the web UI. All 7 members already had 2FA. The federation rule R1 `job_workflow_ref` was read back by the owner at M4 `9e2795b` (this closes the read-back gap in P3-7; the P3-7 text below still says "not read back"). - P4-6. Spec gap: the immutable subject (design change). Repos created before GitHub's immutable OIDC subject default send `sub` as `repo:OperationCode/:...`, and the federation rule's prefix `repo:OperationCode@10479364/` rejects it (Console reason `match_subject_prefix`). The first pilot runs failed closed as designed: all three passes "authentication failed", gate FAIL, exit 1. Fix, per repo: `PUT repos/OperationCode//actions/oidc/customization/sub` with `use_default` true and `use_immutable_subject` true (no org template). This changes `sub` for every OIDC workflow in the repo. `back-end` and `operationcode-pybot` push images through a cloud role whose trust policy had to gain the immutable subjects; the owner did that and kept the old values. After the change all four test PRs passed, with all four jobs `success`. The spec now lists the subject as a prerequisite for every target repo ("Pilot and enforcement"), and phases 4, 5 and 6 of the build plan have the step. - P4-7. Pilot result and releases cf19216 and 96f4966 (owner and coordinator, 2026-10-31). Release cf19216 (PR #21) is K-18 and K-19; release 96f4966 (PR #22) is K-20 and K-21. Rule: dependency-check changes are reproduced and validated locally with `review/tools/dep_repro.py` before release. `OperationCode/front-end` PR 2027, a large dependency update, was falsely blocked three times at earlier releases (pnpm two-document lockfile parse, K-16; lockfile diff size, K-17; npm cap and byte-budget race, K-18 to K-21). At 96f4966 it passes with 0 hard blocks and 0 findings, the same as the local harness. `operationcode_infra` PR 191 passed with low, non-blocking findings. The test PRs were closed 2026-10-31. The pilot continues; the exit judgment is pending the owner. - P4-8. "Claude of Duty" branding (owner-approved text, release item pending). The sticky comment headings, the mission line, the oversize callout, the footer, the workflow name (`Claude of Duty`) and the job display names (`Recon`, `Analysis`, `Supply line check`, `Mission report`) change. Display text only: the markers, job ids, workflow file name, `test / unit-tests` and the inline finding format do not change. See "Publishing (code)". - P4-9. In-place release with an owner-run tool (owner decision, 2026-10-31). Each release edits rule R1 in place, once, at the merge SHA, as every phase 4 release already did; the R2-per-release flow of P0-15 and V26 is retired, and its validation moves to the merge SHA (unit tests, the owner-approved fixture eval, and one token exchange under the scratch test ruleset after the pins move). The exact SHA claim stays: a tag, a branch ref or a prefix match would save at most one edit and drop the Console as a gate outside GitHub. The owner runs `make release SHA=` (`review/tools/release.py`): fail-closed preconditions, no confirmation prompt (owner decision, 2026-10-31: the tool prints the plan, with current values and exact changes, and applies at once; `make release-plan` stays the read-only dry run and its header says so; `release-rule-test` likewise), the R1 edit through the Admin API, the ruleset pin moves, and a read-back of all three that must show only the SHA changed. The Console endpoints refuse Admin API keys, so the tool logs in with `ant auth login --scope org:admin` in a private temp config dir and logs out and deletes it on every exit; no admin token is stored between releases. The tool never runs in CI: it holds an Anthropic admin login and GitHub write at once. Live check on 2026-10-31: `make release-plan` at `96f4966` read R1 through `ant organization:federation:rules retrieve` (ant 2.4.0); the GET returns `audience` and `condition` as null, which the tool leaves out of the POST. The POST was tested live on 2026-10-31 17:21Z: the owner ran `make release-rule-test RULE=fdrl_01AfACzupW3U4Bx361ZUbsyR SHA=96f496603d968121609abcda0e10666871de2ddf`; scratch rule R2 (`claude-review-p3-r2`) moved from `1cefe4259bc26197ec4b063e8399ef856efdd4f2` to `96f4966`, the read-back showed only `job_workflow_ref` changed, and the logout and temp dir removal ran. The first production `make release` is still untested live. -P5: phase 5, Option B as built, 2026-10-31 to 2026-11-01 (owner and coordinator). Where a P5- line changes an earlier line, it wins. +P5: phase 5, Option B as built, 2026-10-31 to 2026-11-04 (owner and coordinator). Where a P5- line changes an earlier line, it wins. - P5-1. Baseline ruleset (owner, 2026-11-01). Org ruleset 25787833 is renamed `org-baseline`. Rules, bypass and targets as in "The org rulesets": `deletion`, `non_fast_forward`, `required_signatures` and `pull_request` (1 approval, dismiss stale approvals on push, Code Owners review, `allowed_merge_methods` squash only, `require_last_push_approval` false, `required_review_thread_resolution` false); `~DEFAULT_BRANCH` of `~ALL` repos, no excludes; bypass only `OrganizationAdmin` in `pull_request` mode. `require_extra_approval_for_unattributed_changes` is `true`: it came as an API default, and it stays on purpose. It adds one approval only to a PR that the Copilot agent opens with no human attribution, and it has no effect on Dependabot, squash or bypass (inferred from the GitHub docs). Every future PUT body of a ruleset with a `pull_request` rule sets it explicitly, so a changed API default cannot flip it. Rollout: canary `operationcode_infra` (infra#192, squash-merged by the owner through the org admin bypass; the squash commit is GitHub-signed and the rule suite shows `bypass`), then 4 repos, then 6, then `~ALL`, in steps. Each step was read back; rollback bodies are in the coordinator records. Lifting the baseline for one repo needs an org owner. - P5-2. Option B (owner, 2026-10-31). Layered rulesets: the review stays in its own workflows-only ruleset (26132750); the baseline (25787833) carries approvals. Reason: separate rollout, bypass and disable per concern; no gap while a pilot repo moves between rulesets; no second pinned ruleset in the release tool. This replaces the plan to fold the pilot into one org ruleset with the approval rules (rollout step 8, "Pilot and enforcement"). @@ -1049,4 +1053,12 @@ P5: phase 5, Option B as built, 2026-10-31 to 2026-11-01 (owner and coordinator) - P5-5. Action pin age 2 days (owner, 2026-10-31). Replaces the 7-day rule (see "Workflow details", K-15). - P5-6. Dependency graph enforced org-wide (owner, 2026-10-31), through a code security configuration with `enforcement: enforced` (phase 6, build plan). - P5-7. Release drift check. `RULESETS` in `review/tools/release.py` does not change: the baseline has no `workflows` rule, so a release has no pin to move there, and the tool requires exactly one workflow entry per listed ruleset. The plan step (also `make release-plan`) now lists every org ruleset (all pages) and reads each one. It fails closed when a ruleset not in `RULESETS` has a `workflows` entry with the `repository_id` of `OperationCode/.github` or the path `.github/workflows/claude-review.yml`, or when an id in `RULESETS` is not in the list. It also fails when any org ruleset requires a status check whose name contains `oc-claude-review` or `Claude of Duty`, because a same-name job can satisfy a name-bound check (see "What each role can still do"). A ruleset it cannot read fails it too. It prints the offending ids. Reason: the owner plans more org rulesets; one with a stale pin would fail every PR in its targets closed on auth, and two review rulesets on one repo can make every PR fail ("One review ruleset"). Repo-level rulesets are out of the tool's scope. -- P5-8. Reserved for the owner's written acceptance of the residuals in "Which run counts" and of the pilot bypass (rollout step 7). Not given yet; the entry, with the owner's name and the date, goes in a later PR. +- P5-8. Written acceptance (2026-11-01, owner Irving Popovetsky). The owner accepts in writing the residuals in "Which run counts" (an earlier pass on the same SHA, including revoke or role removal without a new run, H to B to H return pushes, and deleted newer runs; pin rollback; the cross-PR effect of one head SHA) and the pilot bypass until the switch to org-admin-only bypass. Practical note: after revoking an override, close and reopen the PR or push a new commit. +- P5-9. Acceptance re-run on the current release (2026-11-02, claude-review-test, pin `88e055f`). All 8 rows pass: V6 a part 1 on a fork PR and a same-repo PR, V6 a part 2 on both (the slow and the timed-out gate came from a scratch-only build that is never merged), V8 a (fork PR by account X `irvingpop2`, outside the org), V23, V22 b, and the outage process (invalid federation rule: all three passes `failed` with "authentication failed", not waivable, and the Console shows the denied exchanges). An observer read the merge box every 2 s (7,918 readings): it read mergeable while the newest central run was not `success` only in the two stated residuals, a reopen gap of about 4 s before the new run existed and the revival of older passes after a pin move back ("Which run counts"). The scratch test ruleset read back unchanged at the end. Findings: (1) the baseline's `required_signatures` blocks the merge of a PR with unsigned commits, also a fork PR that passed the review and has an approval; the same change made with a GitHub-signed commit merged. So outside contributors need signed commits, and the summary names unsigned commits (P6-1). (2) GitHub finds the signing key through the commit email: a signed commit whose email belongs to another account reads `unknown_key` and blocks. (3) A force push that keeps the same tree kept an approval although dismiss stale approvals is on (GitHub dismisses only when the diff changes); low risk, recorded. (4) `gh run cancel` cannot find a ruleset-started run, like `gh run rerun`; use `POST .../actions/runs/{run_id}/cancel`. +- P5-10. Bypass matrix and switch rehearsal (2026-11-04, claude-review-test; the scratch test ruleset at pin `88e055f` and the live baseline, which was never changed). Pass: 27 merge cells and 6 direct pushes match the expected table in four stages: (1) empty review bypass list; (2) the pilot-shaped list (the list in "Pilot and enforcement"); (3) org admins only, the phase 6 target; (4) the return to the pilot-shaped list by a PUT of the saved stage 2 read-back, which is the rollback of the phase 6 bypass switch. Each merge was made by the row's actor, and each bypass merge has a rule-suite `bypass` entry. With the pilot-shaped list, Maintain and Admin merged a PR that failed only the review, and never one without the approval: the review bypass does not reach the baseline. With org admins only, Maintain and Admin could not merge past a failed review, and an org admin could, through a pull request. Every direct push to main was refused (GH013). Between stages only `bypass_actors` changed, the stage 4 read-back equals the saved list, and at the end the scratch test ruleset, the baseline and the pilot ruleset read back as before. The passed stage 4 return test is the precondition for the phase 6 bypass switch. +- P5-11. Phase 5 exit (2026-11-04). The written acceptance exists (P5-8). The baseline read-back of 25787833 matches the P5-1 checklist: name `org-baseline`, enforcement active, target branch, `~DEFAULT_BRANCH` of `~ALL` with no excludes, exactly the rules `deletion`, `non_fast_forward`, `pull_request` and `required_signatures` (no `workflows` and no required status checks), the `pull_request` parameters of P5-1, and bypass only `OrganizationAdmin` in `pull_request` mode. The re-run tests pass (P5-9) and the bypass matrix passed (P5-10). `eval` prevent self-review is on, and Dependabot malware alerts are on for the active repos (owner). Harden-runner block mode stays deferred (K-14). + +P6: phase 6, org-wide enforcement, from 2026-11-04 (owner and coordinator). Where a P6- line changes an earlier line, it wins. + +- 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. diff --git a/review/src/oc_review/collect.py b/review/src/oc_review/collect.py index 954a7b4..53dceb3 100644 --- a/review/src/oc_review/collect.py +++ b/review/src/oc_review/collect.py @@ -232,7 +232,14 @@ def default_transport( return HttpResponse(resp.status, b"".join(chunks), resp.url, headers) except urllib.error.HTTPError as exc: headers = {k.lower(): v for k, v in (exc.headers or {}).items()} - return HttpResponse(exc.code, b"", request.url, headers) + # 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. + try: + body = exc.read(request.max_bytes + 1) if clock() <= deadline else b"" + except (OSError, ValueError): + body = b"" + return HttpResponse(exc.code, body or b"", request.url, headers) class GitHubClient: diff --git a/review/src/oc_review/publish.py b/review/src/oc_review/publish.py index c5c068f..86af2af 100644 --- a/review/src/oc_review/publish.py +++ b/review/src/oc_review/publish.py @@ -267,6 +267,25 @@ def get_wrapped_pages( url = nxt raise collect.HttpError(f"{path}: more than {max_pages} pages") + def get_status(self, path: str, *, max_bytes: int) -> collect.HttpResponse | None: + """One GET, no retry, any status: the response (body cut at `max_bytes`), + or None on a network error or a response from another host. For a + display-only probe whose status is the answer.""" + headers = { + "Accept": collect.ACCEPT_JSON, + "Authorization": f"Bearer {self._token}", + "User-Agent": "oc-claude-review", + "X-GitHub-Api-Version": collect.API_VERSION, + } + try: + resp = self._transport(collect.HttpRequest("GET", self._url(path, None), headers, max_bytes, self._timeout)) + except OSError: + return None + served = urllib.parse.urlsplit(resp.url) + if served.scheme != "https" or served.hostname != collect.API_HOST or served.port is not None: + return None + return resp + def write(self, method: str, path: str, body: Mapping[str, Any]) -> Mapping[str, Any]: """POST or PATCH one of `_WRITE_ROUTES`. Returns the response object.""" if not any(method == m and rx.fullmatch(path) for m, rx in _WRITE_ROUTES): @@ -659,6 +678,117 @@ def read_runs(api: GitHubApi, ctx: PublishContext, head_sha: str) -> verdict.Run return verdict.RunList(tuple(runs), complete) +# --------------------------------------------------------------------------- +# Display-only reads. The gate never reads these facts: they only label rows +# and add the informational commit signing row. They cannot change the +# result, a reason, the exit code or the waiver line. +# --------------------------------------------------------------------------- + +# GitHub lists at most 250 commits of a PR. +COMMITS_MAX = 250 +SIGNING_GUIDE_URL = "https://docs.github.com/en/authentication/managing-commit-signature-verification" +DEPENDENCY_PROBE_MAX_BYTES = 64 * 1024 + + +@dataclass(frozen=True) +class CommitSigning: + """Signature state of the PR's commits, as GitHub lists them.""" + + total: int + unsigned: int # commits whose `commit.verification.verified` is not true + + @property + def capped(self) -> bool: + return self.total >= COMMITS_MAX + + +@dataclass(frozen=True) +class SummaryFacts: + """Facts the publisher read from GitHub for the summary display only.""" + + commits: CommitSigning | None = None + dependency_graph_off: bool = False + + +def read_commit_signing(api: GitHubApi, ctx: PublishContext) -> CommitSigning | None: + """`GET /pulls/{n}/commits`, every page. None when the list cannot be read + (then the summary shows no signing row).""" + try: + items = api.get_pages(f"{ctx.repo_path}/pulls/{ctx.pr_number}/commits") + except collect.CollectError as exc: + print(f"publish: commit list not read ({str(exc)[:200]}); no signing row", file=sys.stderr) + return None + unsigned = 0 + for item in items: + commit = item.get("commit") if isinstance(item, Mapping) else None + ver = commit.get("verification") if isinstance(commit, Mapping) else None + if not (isinstance(ver, Mapping) and ver.get("verified") is True): + unsigned += 1 + return CommitSigning(len(items), unsigned) + + +# GitHub's own error message must say the Dependency graph is off. A 403 +# 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). +_GRAPH_OFF_RE = re.compile( + r"\bdependency[ -]graph\b[^.]*\b(?:is\s+)?(?: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 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 + disabled or not enabled. Anything else (no response, another status, an + empty, non-JSON or other message) is False, so the row keeps the neutral + dependency review wording.""" + if resp is None or resp.status != 403: + return False + if resp.headers.get("x-ratelimit-remaining") == "0" or "retry-after" in resp.headers: + return False + try: + doc = envelope.parse_json(resp.body) + except envelope.EnvelopeError: + return False + 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) + + +def read_dependency_graph_off(api: GitHubApi, ctx: PublishContext, snap: snapshot.Snapshot | None) -> bool: + """The dependency comparison of the reviewed pair, read once, judged by + `dependency_graph_off_evidence`.""" + if snap is None: + return False + resp = api.get_status( + f"{ctx.repo_path}/dependency-graph/compare/{snap.merge_base_sha}...{snap.head_sha}", + max_bytes=DEPENDENCY_PROBE_MAX_BYTES, + ) + return dependency_graph_off_evidence(resp) + + +def read_summary_facts( + api: GitHubApi, ctx: PublishContext, snap: snapshot.Snapshot | None, needs: Any, outcome: str | None +) -> SummaryFacts: + """The display-only reads. The dependency probe runs only when the + dependency-review job or step reported `failure`. Never raises: any error + in a read gives no fact (no signing row, the neutral dependency row).""" + dr_failed = job_results(needs).get(verdict.DEPENDENCY_REVIEW_JOB) == "failure" or outcome == "failure" + try: + commits = read_commit_signing(api, ctx) + except Exception as exc: # noqa: BLE001 - display only, never fails the run + print(f"publish: commit list not read ({type(exc).__name__}); no signing row", file=sys.stderr) + commits = None + try: + graph_off = dr_failed and read_dependency_graph_off(api, ctx, snap) + except Exception as exc: # noqa: BLE001 - display only, never fails the run + print(f"publish: dependency probe failed ({type(exc).__name__}); neutral row", file=sys.stderr) + graph_off = False + return SummaryFacts(commits=commits, dependency_graph_off=graph_off) + + # --------------------------------------------------------------------------- # Inline review # --------------------------------------------------------------------------- @@ -1023,6 +1153,18 @@ def _fit(blocks: Sequence[_Block], limit: int) -> tuple[str | None, bool]: HARD_BLOCK_CELL = "🚫 **Hard block**" NOT_RUN_CELL = "⚠️ **Not run**" BLOCKED_CELL = "🚫 **Blocked**" +SETTING_CELL = "⚙️ **Repo setting**" +INFO_CELL = "ℹ️ **Info**" +EFFECT_INFO = "No effect on this review" +COMMIT_CHECK_ROW = "Commit check (code, no model)" + +# An owner setting, not the author's change (item: label setting problems). +DEPENDENCY_GRAPH_CLAIM = ( + "Dependency review cannot run: the Dependency graph is off for this repo. Only a repo admin can fix " + "this: turn it on in the repo settings (Security, Advanced Security), then re-run the newest run. " + "The author does not need to change anything." +) +DEPENDENCY_GRAPH_DETAIL = "Cause: the Dependency graph is off (a repo setting that only a repo admin can change)." # Reasons that mean part of the review did not complete. _INCOMPLETE_CODES = frozenset( @@ -1045,6 +1187,37 @@ def _n(count: int, word: str) -> str: return f"{count} {word}" + ("" if count == 1 else "s") +def _join(names: Sequence[str]) -> str: + """"A", "A and B", "A, B and C".""" + return names[0] if len(names) == 1 else ", ".join(names[:-1]) + " and " + names[-1] + + +def _is_dependency_reason(r: verdict.Reason) -> bool: + """The dependency-review job result and the step outcome reasons. They + report one cause, so the summary shows them as one row.""" + return r.code == verdict.R_DEPENDENCY_REVIEW or ( + r.code == verdict.R_JOB_RESULT and r.detail.split(":", 1)[0] == verdict.DEPENDENCY_REVIEW_JOB + ) + + +_PASS_FAILED_RE = re.compile(r"pass (?:review|security|supply-chain) failed(?:: (.*))?", re.S) + + +def _cause_key(r: verdict.Reason) -> tuple[str, str]: + """Reasons with one key have one root cause and show once: the + dependency-review reasons, and passes that failed for the same reason.""" + if _is_dependency_reason(r): + return (verdict.R_DEPENDENCY_REVIEW, "") + m = _PASS_FAILED_RE.fullmatch(r.detail) if r.code == verdict.R_PASS_RECORDS else None + if m: + return (r.code, "failed: " + (m.group(1) or "")) + return (r.code, r.detail) + + +def _dependency_failed(result: verdict.GateResult) -> bool: + return any(_is_dependency_reason(r) for r in result.reasons) + + def _code_span(kind: str, text: str, suffix: str = "") -> str: """A code span that is safe in a table cell. `escape_field` backslash-escapes punctuation, which would show inside a code span, so a span is cleaned @@ -1054,10 +1227,29 @@ def _code_span(kind: str, text: str, suffix: str = "") -> str: def _span_of(joined: str, cut: bool, suffix: str = "") -> str: """The code span of text that `_clean_text` already cut: one marker only.""" - joined = _defang(joined).replace("`", "'").replace("<", "‹").replace("|", "\\|") + ("…" if cut else "") + joined = _defang(joined).replace("`", "'").replace("<", "‹") + joined = _escape_span_pipes(joined) + ("…" if cut else "") return f"`{joined or 'unknown'}{suffix}`" +_BACKSLASHES_BEFORE_PIPE = re.compile(r"(\\+)(?=\|)") + + +def _escape_span_pipes(text: str) -> str: + """Escape every `|` as `\\|` so a table cell stays whole. A backslash run + right before a pipe is first split from it with a ZWSP, so the pipe always + has exactly one escaping backslash before it. Table parsers differ on + `\\\\|` (cmark-gfm keeps it in the cell, others end the cell there); with + the ZWSP no parser sees two backslashes before a pipe.""" + return _BACKSLASHES_BEFORE_PIPE.sub(lambda m: m.group(1) + ZWSP, text).replace("|", "\\|") + + +def _path_span(value: Any, kind: str = "path") -> str: + """A file path as plain text in a code span (no backslash escapes): the + `_code_span` rules, so it stays safe. A non-string reads `unknown`.""" + return _code_span(kind, value if isinstance(value, str) else "") + + def _cell(text: str) -> str: return text.replace("\n", " ") @@ -1133,6 +1325,7 @@ class _Row: members: tuple[verdict.CitedFinding, ...] = () is_model: bool = False label: str = "" # a hard block row: its fixed label + info: bool = False # informational: not a reason, not counted in the heading def _found_by(pass_name: str, members: Sequence[verdict.CitedFinding]) -> str: @@ -1204,7 +1397,7 @@ def _check_evidence(item: Mapping[str, Any]) -> str: """The label, detail, file, subject and lines of a prefilter item, as one line.""" text = f"{_check_label(item.get('id'))}: {_esc('evidence', item.get('detail'))}" if item.get("file"): - text += f", in {_esc('path', item.get('file'))}" + text += f", in {_path_span(item.get('file'))}" if item.get("subject"): text += f", {_esc('evidence', item.get('subject'))}" lines = item.get("lines") @@ -1250,37 +1443,56 @@ def _hard_rows(result: verdict.GateResult, payload: Mapping[str, Any] | None) -> label = "Incomplete file coverage" if r.detail.startswith("incomplete-coverage") else "File check result" out.append( _Row((0, len(items) + n), HARD_BLOCK_CELL, EFFECT_NO_WAIVER, "Whole PR", - f"{label}: {_esc('reason', r.detail)}", FILE_CHECKS_ROW, label=label) + f"{label}: {_code_span('reason', r.detail)}", FILE_CHECKS_ROW, label=label) ) return out -def _not_run_rows(result: verdict.GateResult) -> list[_Row]: +def _not_run_rows(result: verdict.GateResult, facts: SummaryFacts) -> list[_Row]: """Rows for every pass or job that did not complete, and for every other - reason that is not a finding or a hard block.""" + reason that is not a finding or a hard block. Rows with one cause show + once: passes that failed for the same reason share a row, and the + dependency-review job result and step outcome are one row.""" out: list[_Row] = [] by_pass = {p.name: p for p in result.passes} size_gate = any(r.code == verdict.R_SIZE_GATE for r in result.reasons) - pass_rows = 0 + pass_claims: dict[str, list[str]] = {} for name in verdict.PASS_NAMES: info = by_pass.get(name) label = PASS_LABELS[name] if info is None: - claim = f"No valid record of the {label} pass. The pass returned no result." + claim = "No valid pass record. The pass returned no result." elif info.status == "ok" or (info.status == "skipped-size" and size_gate): continue else: reason = info.meta.get("reason") why = reason.strip().rstrip(".") if isinstance(reason, str) and reason.strip() else info.status claim = f"{_esc('reason', why)}. The pass returned no result." + pass_claims.setdefault(claim, []).append(label) + pass_rows = 0 + for claim, labels in pass_claims.items(): pass_rows += 1 - out.append(_Row((1, pass_rows), NOT_RUN_CELL, EFFECT_NO_WAIVER, f"{label} pass", claim, CODE_ROW)) + where = f"{_join(labels)} pass" + ("es" if len(labels) > 1 else "") + out.append(_Row((1, pass_rows), NOT_RUN_CELL, EFFECT_NO_WAIVER, where, claim, CODE_ROW)) + dependency = [r for r in result.reasons if _is_dependency_reason(r)] n = 0 for r in result.reasons: if r.code in (verdict.R_HARD_BLOCK, verdict.R_BLOCKING_FINDING): continue if r.code == verdict.R_PASS_RECORDS and pass_rows: continue + if _is_dependency_reason(r): + if r is not dependency[0]: + continue + n += 1 + if facts.dependency_graph_off: + out.append(_Row((1, 100 + n), SETTING_CELL, EFFECT_NO_WAIVER, "Repo settings", + DEPENDENCY_GRAPH_CLAIM, CODE_ROW)) + else: + details = ", ".join(_code_span("reason", x.detail) for x in dependency) + out.append(_Row((1, 100 + n), NOT_RUN_CELL, EFFECT_NO_WAIVER, "Dependency review", + f"{_reason_label(verdict.R_DEPENDENCY_REVIEW)}: {details}", CODE_ROW)) + continue n += 1 if r.code == verdict.R_SIZE_GATE: effect = EFFECT_WAIVED if result.override_applied else EFFECT_WAIVABLE @@ -1291,16 +1503,31 @@ def _not_run_rows(result: verdict.GateResult) -> list[_Row]: continue cell = BLOCKED_CELL if r.code in (verdict.R_STALE, verdict.R_SUPERSEDED, verdict.R_BASE_NOT_DEFAULT) else NOT_RUN_CELL out.append( - _Row((1, 100 + n), cell, EFFECT_NO_WAIVER, "Gate", f"{_reason_label(r.code)}: {_esc('reason', r.detail)}", CODE_ROW) + _Row((1, 100 + n), cell, EFFECT_NO_WAIVER, "Gate", f"{_reason_label(r.code)}: {_code_span('reason', r.detail)}", CODE_ROW) ) return out +def _signing_row(facts: SummaryFacts) -> list[_Row]: + """The informational unsigned-commit row; none when every commit is + verified-signed or the list could not be read. It never changes the gate.""" + c = facts.commits + 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}" + claim = ( + f"**Unsigned commits:** {of} commits 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})" + ) + return [_Row((3, 0), INFO_CELL, EFFECT_INFO, "Commits", claim, COMMIT_CHECK_ROW, info=True)] + + def _findings_table(rows: list[_Row]) -> list[_Block]: if not rows: return [] out = [ - _Block("\n| Severity | Effect | Where | Finding | Found by |"), + _Block("\n| Severity | Effect | Where | Finding | Found by |"), # = _FINDINGS_HEADER _Block("|---|---|---|---|---|"), ] for r in rows: @@ -1351,12 +1578,17 @@ def _blocking_phrase(model_rows: list[_Row]) -> str: return f"{len(blocking)} findings" -def _callout(result: verdict.GateResult, rows: list[_Row], hard: list[_Row], has_payload: bool) -> str: +def _callout( + result: verdict.GateResult, rows: list[_Row], hard: list[_Row], has_payload: bool, facts: SummaryFacts +) -> str: """The colored callout. Kind: TIP for a pass with no findings, NOTE for a pass with findings, CAUTION for every fail, waivable or not (ruling 1). - The text says whether a waiver can clear it.""" + The text says whether a waiver can clear it. A repo setting problem is + never given to the author as a step.""" model = _model_rows(rows) codes = result.codes + 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: if result.override_applied: login = _esc("login", result.valid_overrides[0].author_login) if result.valid_overrides else "a maintainer" @@ -1388,7 +1620,22 @@ def _callout(result: verdict.GateResult, rows: list[_Row], hard: list[_Row], has else: first = f"**{label} and {len(hard) - 1} other hard block(s) block this PR. No waiver can clear them.**{also}" nxt = "**Next step for the author:** fix the cause named in the row, then push. The review runs again on the new commit." + if setting: + nxt += setting_also elif codes & _INCOMPLETE_CODES: + other_incomplete = [r for r in result.reasons if r.code in _INCOMPLETE_CODES and not _is_dependency_reason(r)] + if setting and not other_incomplete: + first = f"**A repo setting stops the dependency review, so this PR could not be judged. No waiver can clear this.**{also}" + nxt = ( + "**Next step for a repo admin:** turn on the Dependency graph in the repo settings, then re-run " + "the newest run of the required workflow. " + ) + nxt += ( + f"The author also fixes or waives the finding{'' if len(blocking) == 1 else 's'}." + if blocking + else "The author does not need to change anything." + ) + return "\n".join(f"> {x}" for x in ("[!CAUTION]", first, nxt)) if failed and verdict.R_PASS_RECORDS in codes: who = " and ".join(failed) if len(failed) <= 2 else "Review, Security and Supply chain" first = f"**The {who} {'review' if len(failed) == 1 else 'reviews'} did not run, so this PR could not be judged. No waiver can clear this.**{also}" @@ -1400,6 +1647,8 @@ def _callout(result: verdict.GateResult, rows: list[_Row], hard: list[_Row], has if blocking else "The author does not need to change anything." ) + if setting: + nxt += setting_also elif verdict.R_BASE_NOT_DEFAULT in codes: ref = _esc("ref", result.snapshot.base_ref) if result.snapshot else "another branch" first = f"**This PR targets {ref}, not the default branch. No waiver can clear this.**{also}" @@ -1422,7 +1671,15 @@ def _callout(result: verdict.GateResult, rows: list[_Row], hard: list[_Row], has return "\n".join(f"> {x}" for x in ("[!CAUTION]", first, nxt)) -def _checks_strip(result: verdict.GateResult, payload: Mapping[str, Any] | None, hard_count: int) -> _Block: +def _dependency_cell(result: verdict.GateResult, facts: SummaryFacts, ok: str) -> str: + if not _dependency_failed(result): + return ok + return "⚙️ repo setting" if facts.dependency_graph_off else "❌ failed" + + +def _checks_strip( + result: verdict.GateResult, payload: Mapping[str, Any] | None, hard_count: int, facts: SummaryFacts +) -> _Block: """One-row table: what ran. Never cut.""" if payload is None: file_cell, coverage_cell = "❌ no result", "❌ unknown" @@ -1450,11 +1707,7 @@ def _checks_strip(result: verdict.GateResult, payload: Mapping[str, Any] | None, cells.append("⚠️ skipped, too large") else: cells.append(f"❌ {_esc('label', info.status)}") - dep_failed = any( - r.code == verdict.R_DEPENDENCY_REVIEW or (r.code == verdict.R_JOB_RESULT and verdict.DEPENDENCY_REVIEW_JOB in r.detail) - for r in result.reasons - ) - dep_cell = "❌ failed" if dep_failed else "✅ ok" + dep_cell = _dependency_cell(result, facts, "✅ ok") return _Block( "\n".join( ( @@ -1522,14 +1775,23 @@ def _snapshot_blocks(ctx: PublishContext, result: verdict.GateResult) -> list[_B ] -def _reason_blocks(result: verdict.GateResult) -> list[_Block]: +def _reason_blocks(result: verdict.GateResult, facts: SummaryFacts) -> list[_Block]: + """Every reason, one row per root cause (`_cause_key`): a row with several + reasons lists each detail.""" out = [_title("Reasons the gate failed")] if not result.reasons: return out + [_Block("None. No reason applies.")] out += [_Block("| Reason | Waivable? | Detail |"), _Block("|---|---|---|")] + causes: dict[tuple[str, str], list[verdict.Reason]] = {} for r in result.reasons: - flag = ("✅ yes, waived" if result.override_applied else "✅ yes") if r.waivable else "🚫 no" - out.append(_Block(_row(_reason_label(r.code), flag, _esc("reason", r.detail)))) + causes.setdefault(_cause_key(r), []).append(r) + for (code, _detail), members in causes.items(): + waivable = all(r.waivable for r in members) + flag = ("✅ yes, waived" if result.override_applied else "✅ yes") if waivable else "🚫 no" + detail = ", ".join(_code_span("reason", r.detail) for r in members) + if code == verdict.R_DEPENDENCY_REVIEW and facts.dependency_graph_off: + detail += " " + DEPENDENCY_GRAPH_DETAIL + out.append(_Block(_row(_reason_label(code), flag, detail))) if result.status == verdict.FAIL and result.override_command is None: out.append( _Block( @@ -1586,7 +1848,7 @@ def _delta_blocks(delta: Any) -> list[_Block]: f"- {_esc('label', ch.get('ecosystem'))} {_esc('package_name', ch.get('name'))}: " f"{olds} to {_esc('version', ch.get('new_version'))}, " f"source {_esc('label', source.get('kind'))} {_esc('url', source.get('normalized'))}, " - f"in {_esc('path', ch.get('path'))}" + f"in {_path_span(ch.get('path'))}" ) reg = ch.get("registry") if isinstance(reg, Mapping): @@ -1602,7 +1864,7 @@ def _delta_blocks(delta: Any) -> list[_Block]: src = s.get("source") if isinstance(s.get("source"), Mapping) else {} out.append( _Block( - f"- new file-wide source in {_esc('path', s.get('path'))}: {_esc('url', src.get('normalized'))}", + f"- new file-wide source in {_path_span(s.get('path'))}: {_esc('url', src.get('normalized'))}", CUT_DEPENDENCY, "dependency", ) @@ -1611,7 +1873,7 @@ def _delta_blocks(delta: Any) -> list[_Block]: if isinstance(fl, Mapping): out.append( _Block( - f"- failed: {_esc('path', fl.get('path'))}: {_esc('reason', fl.get('reason'))}", + f"- failed: {_path_span(fl.get('path'))}: {_esc('reason', fl.get('reason'))}", CUT_DEPENDENCY, "dependency", ) @@ -1619,7 +1881,9 @@ def _delta_blocks(delta: Any) -> list[_Block]: return out -def _check_table_blocks(result: verdict.GateResult, payload: Mapping[str, Any] | None) -> list[_Block]: +def _check_table_blocks( + result: verdict.GateResult, payload: Mapping[str, Any] | None, facts: SummaryFacts +) -> list[_Block]: """The "Checks (code, no model)" table: hard blocks, coverage, dependency changes and the dependency-review job. Hard block evidence and unavailable files are never cut.""" out = [_title("Checks (code, no model)"), _Block("| Check | Result | Detail |"), _Block("|---|---|---|")] @@ -1649,15 +1913,20 @@ def _check_table_blocks(result: verdict.GateResult, payload: Mapping[str, Any] | for f in files if isinstance(files, list) else (): if isinstance(f, Mapping) and f.get("coverage") == "unavailable": out.append( - _Block(_row("Unavailable file", "⚠️ unavailable", f"{_esc('path', f.get('path'))}: {_esc('reason', f.get('reason'))}")) + _Block(_row("Unavailable file", "⚠️ unavailable", f"{_path_span(f.get('path'))}: {_esc('reason', f.get('reason'))}")) ) has_delta = bool(_delta_blocks(payload.get("dependency_delta"))) out.append(_Block(_row("Dependency changes", "ℹ️ listed below" if has_delta else "✅ none", ""))) - dep_failed = any( - r.code == verdict.R_DEPENDENCY_REVIEW or (r.code == verdict.R_JOB_RESULT and verdict.DEPENDENCY_REVIEW_JOB in r.detail) - for r in result.reasons + setting = facts.dependency_graph_off and _dependency_failed(result) + out.append( + _Block( + _row( + "Dependency review job", + _dependency_cell(result, facts, "✅ success"), + DEPENDENCY_GRAPH_DETAIL if setting else "", + ) + ) ) - out.append(_Block(_row("Dependency review job", "❌ failed" if dep_failed else "✅ success", ""))) return out @@ -1679,7 +1948,7 @@ def _finding_block(f: verdict.CitedFinding, inline: InlineOutcome) -> _Block | N state = _posting_state(f, inline) if state in _INLINE_STATES: return None - where = f"{_esc('file', f.file)} line {f.line} ({f.side})" + where = f"{_path_span(f.file, 'file')} line {f.line} ({f.side})" head = f"- {_finding_line(f)}, at {where}: {_esc('title', f.title)}" # A high or critical finding is never cut, also when it is unverified. protected = not _cuttable(f) @@ -1797,13 +2066,20 @@ def _summary_table_blocks(result: verdict.GateResult) -> list[_Block]: return out +TOKENS_NONE = "No token usage was recorded for any pass." + + def _token_blocks(result: verdict.GateResult) -> list[_Block]: + """The token table, or one line when no pass recorded any count (an + all-"not recorded" table is noise).""" + by_pass = {p.name: p for p in result.passes} + if not any(_usage_numbers(p.meta) for p in by_pass.values()): + return [_title("Token usage"), _Block(TOKENS_NONE)] out = [ _title("Token usage"), _Block("| Pass | Input | Output | Cache write | Cache read | Attempts |"), _Block("|---|---:|---:|---:|---:|---:|"), ] - by_pass = {p.name: p for p in result.passes} totals: dict[str, int] = {} missing = False for name in verdict.PASS_NAMES: @@ -1830,12 +2106,12 @@ def _token_blocks(result: verdict.GateResult) -> list[_Block]: def _record_blocks(ctx: PublishContext, result: verdict.GateResult, payload: Mapping[str, Any] | None, - inline: InlineOutcome) -> list[_Block]: + inline: InlineOutcome, facts: SummaryFacts) -> list[_Block]: inner: list[_Block] = [] inner += _snapshot_blocks(ctx, result) - inner += _reason_blocks(result) + inner += _reason_blocks(result, facts) inner += _override_blocks(ctx, result) - inner += _check_table_blocks(result, payload) + inner += _check_table_blocks(result, payload, facts) inner += _signal_blocks(payload) inner.append(_title("Dependency changes")) delta = _delta_blocks(payload.get("dependency_delta")) if payload is not None else [] @@ -1901,23 +2177,25 @@ def _summary_blocks( payload: Mapping[str, Any] | None, inline: InlineOutcome, truncated: bool, + facts: SummaryFacts, ) -> list[_Block]: hard = _hard_rows(result, payload) rows = sorted( - [*hard, *_not_run_rows(result), *_merge_findings(result, inline)], + [*hard, *_not_run_rows(result, facts), *_merge_findings(result, inline)], key=lambda r: r.sort, ) model = _model_rows(rows) b = [ _Block(verdict.STICKY_MARKER), _Block(_status_heading(result, model, rows) + "\n" + MISSION_LINE), - _Block("\n" + _callout(result, rows, hard, payload is not None)), + _Block("\n" + _callout(result, rows, hard, payload is not None, facts)), ] - b += _findings_table(rows) - b.append(_checks_strip(result, payload, len(hard))) + # The signing row is informational: it is not in the heading count or the callout. + b += _findings_table([*rows, *_signing_row(facts)]) + b.append(_checks_strip(result, payload, len(hard), facts)) b.append(_provenance(ctx, result, truncated)) b += _waiver_blocks(result) - b += _record_blocks(ctx, result, payload, inline) + b += _record_blocks(ctx, result, payload, inline, facts) return b @@ -1928,16 +2206,55 @@ def render_summary( bundle: Mapping[str, Any] | None, inline: InlineOutcome, limit: int = SUMMARY_LIMIT, + facts: SummaryFacts | None = None, ) -> RenderedSummary: """The sticky summary body (D8). Every untrusted string goes through `escape_field` (or `_code_span`). Layout: result heading, callout, findings - table, checks strip, provenance, then the collapsed waiver and record.""" + table, checks strip, provenance, then the collapsed waiver and record. + `facts` are display-only (`read_summary_facts`); `result` alone decides + the gate.""" + facts = facts or SummaryFacts() payload = _payload_of(bundle) - body, cut = _fit(_summary_blocks(ctx, result, payload, inline, False), limit) + # The signing row is informational and must never change the comment + # otherwise: render without it, and keep it only when the body with it + # makes exactly the same cuts (and no overflow). + base = _render_fit(ctx, result, payload, inline, limit, replace(facts, commits=None)) + if not _signing_row(facts) or base.overflow: + return base + row = _render_fit(ctx, result, payload, inline, limit, facts) + if row.overflow or row.cut != base.cut or _without_signing_row(row.body, facts) != base.body: + print("publish: no room for the unsigned-commit row; left out", file=sys.stderr) + return base + return row + + +_FINDINGS_HEADER = "\n| Severity | Effect | Where | Finding | Found by |\n|---|---|---|---|---|" + + +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.""" + [r] = _signing_row(facts) + out = body.replace("\n" + _row(r.severity, r.effect, r.where, r.claim, r.found_by), "", 1) + if out.count(_FINDINGS_HEADER) == 1 and out.find(_FINDINGS_HEADER + "\n|") == -1: + out = out.replace("\n" + _FINDINGS_HEADER, "", 1) + return out + + +def _render_fit( + ctx: PublishContext, + result: verdict.GateResult, + payload: Mapping[str, Any] | None, + inline: InlineOutcome, + limit: int, + facts: SummaryFacts, +) -> RenderedSummary: + body, cut = _fit(_summary_blocks(ctx, result, payload, inline, False, facts), limit) if body is not None and cut: # The provenance line says so when a cut was made. Its longer text is # in the fit, so the body still fits. - body, cut = _fit(_summary_blocks(ctx, result, payload, inline, True), limit) + body, cut = _fit(_summary_blocks(ctx, result, payload, inline, True, facts), limit) if body is None: return RenderedSummary(_overflow_body(ctx, result), True, overflow=True) return RenderedSummary(body, cut) @@ -2208,14 +2525,16 @@ def publish( append_step_summary(step_summary, ctx, result, codes=[*result.codes, verdict.R_STALE]) return 1 - # b) sticky summary + # b) sticky summary. The display-only reads come first, so the snapshot + # check stays right before the write. + facts = read_summary_facts(api, ctx, snap, needs, dependency_review_outcome(needs, dependency_outcome_env)) check = check_snapshot(api, ctx, snap) if check.blocks_writes: print(f"publish: {check.describe()} before the summary; no more writes", file=sys.stderr) append_step_summary(step_summary, ctx, result, codes=[*result.codes, verdict.R_STALE]) return 1 bundle_doc = bundle_read.envelope if bundle_read.error is None else None - summary = render_summary(ctx, result, bundle=bundle_doc, inline=inline) + summary = render_summary(ctx, result, bundle=bundle_doc, inline=inline, facts=facts) full = full_record(ctx, result, bundle_doc) if summary.cut else None if full is not None: log_full_record(full) @@ -2243,7 +2562,7 @@ def publish( if result.passed and not final.passed: # Best effort: the exit code is non-zero whatever this write does. The # reads above can take many pages, so check the snapshot once more. - summary = render_summary(ctx, final, bundle=bundle_doc, inline=inline) + summary = render_summary(ctx, final, bundle=bundle_doc, inline=inline, facts=facts) check = check_snapshot(api, ctx, snap) if check.blocks_writes: print(f"publish: {check.describe()} before the summary update; no more writes", file=sys.stderr) diff --git a/review/tests/unit/test_comment_format.py b/review/tests/unit/test_comment_format.py new file mode 100644 index 0000000..dcedb86 --- /dev/null +++ b/review/tests/unit/test_comment_format.py @@ -0,0 +1,465 @@ +"""Summary comment format, phase 6 (spec "Summary comment"): one row per root +cause, owner setting problems labeled apart from the author's change, paths +as plain text, no empty token table, and the informational unsigned-commit +row. No network: the gate job runs against the fake GitHub of test_publish. +""" + +from __future__ import annotations + +import io +import json +import re +import urllib.error + +import pytest + +from oc_review import collect, publish, verdict +from oc_review.collect import HttpRequest, HttpResponse + +from .test_publish import ( + BLOCKING, + ZWSP, + FakeGitHub, + bundle_doc, + clean_passes, + finding, + needs_ok, + pass_doc, + plain, + reason_rows, + run, + signed_commit, +) +from .test_summary_comment import callout_kind, table_rows + + +def dr_failed_needs(): + needs = needs_ok(outcome="failure") + needs["dependency-review"]["result"] = "failure" + return needs + + +def findings_rows(s: str) -> list[list[str]]: + return table_rows(s, "| Severity |") if "| Severity |" in s else [] + + +def gate(r) -> tuple: + """What the gate decided: exit code, step summary result and reason codes.""" + rec = r.record + return r.code, rec["result"], tuple(sorted(rec.get("reasons") or rec.get("codes") or ())) + + +# ---------------------------------------------------------------- 1. one row per cause + + +def test_dependency_review_failure_is_one_row_and_one_reason_row(tmp_path): + r = run(tmp_path, needs=dr_failed_needs()) + s = r.summary + assert r.code == 1 + rows = findings_rows(s) + assert len(rows) == 1 # job, job result and step outcome: one cause + assert rows[0][:3] == ["⚠️ **Not run**", "Blocks, no waiver", "Dependency review"] + for detail in ("dependency-review: failure", "job result failure", "step outcome failure"): + assert detail in plain(rows[0][3]) + deps = [x for x in reason_rows(s) if x[0] == verdict.R_DEPENDENCY_REVIEW] + assert len(deps) == 1 and not deps[0][1] + assert "dependency-review: failure" in deps[0][2] and "step outcome failure" in deps[0][2] + assert not any(c == verdict.R_JOB_RESULT for c, _, _ in reason_rows(s)) + # One fail row: the heading counts it once. + assert "## 🔴 Comms down: the Claude of Duty review did not complete" in s + + +def test_dedupe_is_display_only(tmp_path): + """The gate still holds every reason: the step summary lists both codes.""" + r = run(tmp_path, needs=dr_failed_needs()) + codes = set(r.record.get("reasons") or r.record.get("codes") or ()) + assert {verdict.R_JOB_RESULT, verdict.R_DEPENDENCY_REVIEW} <= codes + + +def failed_pass(name, reason): + meta = {"reason": reason, "model": None, "stop_reason": None, "request_schema_sha256": "f" * 64, "usage": None} + return pass_doc(name, "failed", meta=meta) + + +def test_passes_that_failed_for_one_reason_share_a_row(tmp_path): + passes = {n: failed_pass(n, "authentication failed") for n in verdict.PASS_NAMES} + s = run(tmp_path, passes=passes).summary + rows = findings_rows(s) + assert [x[2] for x in rows] == ["Review, Security and Supply chain passes"] + assert "authentication failed" in plain(rows[0][3]) + [rec] = [x for x in reason_rows(s) if x[0] == verdict.R_PASS_RECORDS] # one record row, every detail + assert all(f"pass {n} failed: authentication failed" in rec[2] for n in verdict.PASS_NAMES) + + +def test_passes_that_failed_for_different_reasons_keep_their_rows(tmp_path): + passes = clean_passes() + passes["review"] = failed_pass("review", "authentication failed") + passes["security"] = failed_pass("security", "API unavailable after 3 attempts") + passes["supply-chain"] = failed_pass("supply-chain", "authentication failed") + s = run(tmp_path, passes=passes).summary + assert sorted(x[2] for x in findings_rows(s)) == ["Review and Supply chain passes", "Security pass"] + assert len([x for x in reason_rows(s) if x[0] == verdict.R_PASS_RECORDS]) == 2 + + +# ---------------------------------------------------------------- 2. owner setting problems + + +def graph_off_github(**kw) -> FakeGitHub: + """GitHub answers the dependency comparison with 403 and an error message + that says the Dependency graph is disabled: positive evidence.""" + return FakeGitHub(dependency_graph_status=403, dependency_graph_body=error_body(GRAPH_OFF_MESSAGE), **kw) + + +GRAPH_OFF_MESSAGE = "Dependency graph is disabled for this repository. Enable it in the repository settings." + + +def error_body(message: str) -> bytes: + return json.dumps({"message": message, "documentation_url": "https://docs.github.com/rest"}).encode() + + +def test_dependency_graph_off_is_a_repo_setting_not_the_authors_fault(tmp_path): + r = run(tmp_path, graph_off_github(), needs=dr_failed_needs()) + s = r.summary + [row] = findings_rows(s) + assert row[:3] == [publish.SETTING_CELL, "Blocks, no waiver", "Repo settings"] + assert row[3] == publish.DEPENDENCY_GRAPH_CLAIM + assert "Only a repo admin can fix this" in row[3] and "The author does not need to change anything." in row[3] + assert callout_kind(s) == "CAUTION" + assert "**A repo setting stops the dependency review" in s + assert "**Next step for a repo admin:** turn on the Dependency graph" in s + assert "Next step for the author" not in s + assert "| ⚙️ repo setting |" in s # checks strip + assert publish.DEPENDENCY_GRAPH_DETAIL in s # reasons and checks record + + +def test_setting_label_never_changes_the_gate(tmp_path): + plain_run = run(tmp_path / "a", needs=dr_failed_needs()) + setting_run = run(tmp_path / "b", graph_off_github(), needs=dr_failed_needs()) + assert publish.SETTING_CELL in setting_run.summary + assert gate(setting_run) == gate(plain_run) + assert setting_run.code == 1 + assert "/oc-review override" not in setting_run.summary # still not waivable + + +# A 403 alone is not evidence. Each case keeps the neutral dependency row. +NOT_EVIDENCE = { + "200": (200, {}, b""), + "404": (404, {}, b""), + "500": (500, {}, b""), + "403-empty-body": (403, {}, b""), + "403-fork-forbidden": (403, {}, error_body("Forbidden")), + "403-advanced-security-off": (403, {}, error_body( + "Dependency review is not supported on this repository: GitHub Advanced Security is not enabled.")), + "403-advanced-security-and-graph": (403, {}, error_body( + "Dependency graph is not enabled, or Advanced Security is disabled for this repository.")), + "403-permission": (403, {}, error_body("Resource not accessible by integration")), + "403-secondary-rate-limit-no-headers": (403, {}, error_body( + "You have exceeded a secondary rate limit. Please wait a few minutes before you try again.")), + "403-rate-limit-headers": (403, {"x-ratelimit-remaining": "0"}, error_body(GRAPH_OFF_MESSAGE)), + "403-retry-after": (403, {"retry-after": "60"}, error_body(GRAPH_OFF_MESSAGE)), + "403-not-json": (403, {}, b"Dependency graph is disabled"), + "403-message-not-a-string": (403, {}, json.dumps({"message": ["Dependency graph is disabled"]}).encode()), + "200-with-the-message": (200, {}, error_body(GRAPH_OFF_MESSAGE)), +} + + +@pytest.mark.parametrize("case", NOT_EVIDENCE) +def test_without_positive_evidence_the_row_stays_neutral(tmp_path, case): + status, headers, body = NOT_EVIDENCE[case] + gh = FakeGitHub(dependency_graph_status=status, dependency_graph_headers=headers, dependency_graph_body=body) + r = run(tmp_path, gh, needs=dr_failed_needs()) + s = r.summary + [row] = findings_rows(s) + assert row[2] == "Dependency review" and publish.SETTING_CELL not in s + assert "repo setting" not in s.lower() and "Next step for a repo admin" not in s + assert "| ❌ failed |" in s + assert r.code == 1 + + +@pytest.mark.parametrize("message", [ + GRAPH_OFF_MESSAGE, + "The dependency graph is not enabled for this repository.", + "Dependency-graph is turned off for this repository", +]) +def test_evidence_messages(message): + resp = HttpResponse(403, error_body(message), "https://api.github.com/x", {}) + assert 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] + r = run(tmp_path / "f", needs=dr_failed_needs()) + assert [p for p in r.gh.paths() if "/dependency-graph/" in p] + + +def test_probe_error_keeps_the_neutral_row(tmp_path): + gh = FakeGitHub() + + def boom(gh_, req): + if "/dependency-graph/" in req.url: + raise RuntimeError("unexpected") + + gh.after = boom + r = run(tmp_path, gh, needs=dr_failed_needs()) + assert r.code == 1 and publish.SETTING_CELL not in r.summary + assert findings_rows(r.summary)[0][2] == "Dependency review" + + +def test_hard_block_and_setting_tell_the_author_only_their_part(tmp_path): + item = {"id": "bidi-control", "level": "hard_block", "detail": "bidi control characters U+202E, line 1", + "file": "bidi.txt", "subject": None, "lines": [1], "count": 1} + s = run(tmp_path, graph_off_github(), bundle=bundle_doc(hard_blocks=[item]), needs=dr_failed_needs()).summary + assert "**Next step for the author:** fix the cause named in the row" in s + assert "only a repo admin can fix it" in s + assert [x[0] for x in findings_rows(s)] == ["🚫 **Hard block**", publish.SETTING_CELL] + + +def test_default_transport_keeps_a_bounded_error_body(): + class Opener: + def open(self, req, timeout): + raise urllib.error.HTTPError(req.full_url, 403, "Forbidden", {"X-Test": "1"}, + io.BytesIO(error_body(GRAPH_OFF_MESSAGE) + b" " * 500)) + + req = HttpRequest("GET", "https://api.github.com/x", {}, 100, 5.0) + resp = collect.default_transport(req, opener=Opener()) + assert resp.status == 403 and resp.headers == {"x-test": "1"} + assert len(resp.body) == 101 # cut at the byte cap + 1, like a 200 body + + +# ---------------------------------------------------------------- 3. plain paths + + +PATH = "review/tests/unit/test_publish-v2.py" + + +def test_paths_show_as_plain_text_not_backslash_escaped(tmp_path): + item = {"id": "bidi-control", "level": "hard_block", "detail": "bidi control characters U+202E, line 4", + "file": PATH, "subject": None, "lines": [4], "count": 1} + unsafe = finding("bug", "low", file="docs/a_b.md", line=3, quote="nope") + s = run(tmp_path, bundle=bundle_doc(hard_blocks=[item]), passes=clean_passes(review=[unsafe])).summary + assert f"`{PATH}`" in s and f"`{PATH}:4`" in s + assert "review\\/tests" not in s and "test\\_publish" not in s + # Reasons table detail: a code span, not escaped prose. + assert f"`bidi-control in {PATH}`" in s + # Finding bullet in the record. + assert "`docs/a_b.md` line 3 (head)" in s + + +def test_path_spans_stay_safe(tmp_path): + bad = "a`b@user/#12/" + "deadbeefcafe1234.py" + item = {"id": "binary-file", "level": "hard_block", "detail": "binary", "file": bad, + "subject": None, "lines": [], "count": 1} + s = run(tmp_path, bundle=bundle_doc(hard_blocks=[item])).summary + span = publish._path_span(bad) + assert span in s + assert span.count("`") == 2 and " 60_000 # the boundary case + assert gate(unsigned) == gate(signed) and unsigned.code == 0 + assert "publish_error" not in unsigned.record + # Exactly the same comment: the row did not fit, so it is left out. + assert unsigned.summary == signed.summary and "Unsigned commits" not in unsigned.summary + + +def test_signing_row_is_left_out_when_it_would_cause_a_cut(): + from .test_publish import CTX + + r = run_result_for_render() + base = publish.render_summary(CTX, r, bundle=None, inline=publish.InlineOutcome()) + facts = publish.SummaryFacts(commits=publish.CommitSigning(3, 2)) + # Room for the body, not for the body plus the row: same body, no cut. + tight = publish.render_summary(CTX, r, bundle=None, inline=publish.InlineOutcome(), + limit=len(base.body) + 5, facts=facts) + assert tight == base + 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 + + +def run_result_for_render() -> verdict.GateResult: + return verdict.GateResult(status=verdict.PASS, run_id=1000, reasons=(), findings=(), snapshot=None, + overrides=(), override_applied=False) + + +@pytest.mark.parametrize("error", [RuntimeError("boom"), TimeoutError("slow"), ValueError("bad")]) +def test_commit_read_errors_give_no_row_and_keep_the_pass(tmp_path, error): + gh = FakeGitHub(commits=[signed_commit(False)]) + + def fail(gh_, req): + if req.url.split("?")[0].endswith("/pulls/7/commits"): + raise error + + gh.after = fail + r = run(tmp_path, gh) + assert r.code == 0 and r.record["result"] == verdict.PASS + assert "Unsigned commits" not in r.summary + + +@pytest.mark.parametrize("items", [ + [{"sha": "x"}], # no commit object + [{"commit": {"verification": None}}], + ["not an object"], +]) +def test_commits_with_missing_data_never_change_the_pass(tmp_path, items): + r = run(tmp_path, FakeGitHub(commits=items)) + assert r.code == 0 and r.record["result"] == verdict.PASS + + +# ---------------------------------------------------------------- fix 3: hostile file names stay in one cell + + +def cells_cmark_gfm(line: str) -> list[str]: + """cmark-gfm: a cell ends at a `|` that has no backslash right before it.""" + return re.split(r"(? list[str]: + """Parsers such as marked: a `|` is escaped only after an odd number of + backslashes, so `\\\\|` ends the cell.""" + out, cur, i = [], "", 0 + while i < len(line): + ch = line[i] + if ch == "|": + n = len(cur) - len(cur.rstrip("\\")) + if n % 2 == 0: + out.append(cur) + cur = "" + i += 1 + continue + cur += ch + i += 1 + out.append(cur) + return out[1:-1] + + +HOSTILE = { + "pipe": "a|b.py", + "backslash-pipe": "a\\|b.py", + "two-backslashes-pipe": "a\\\\|b.py", + "three-backslashes-pipe": "a\\\\\\|b.py", + "trailing-backslash": "dir\\", + "backtick": "a`b`|c.py", + "newline": "a\nb|c.py", + "bidi": "a\u202eb|c.py", + "mix": "\\|`\\\\|\n\u2066||@u/#1\\", +} + + +@pytest.mark.parametrize("case", HOSTILE) +def test_hostile_path_stays_in_its_cell(tmp_path, case): + path = HOSTILE[case] + item = {"id": "binary-file", "level": "hard_block", "detail": "binary", "file": path, + "subject": None, "lines": [], "count": 1} + s = run(tmp_path, bundle=bundle_doc(hard_blocks=[item])).summary + rows = [line for line in s.split("\n") if line.startswith("| 🚫 **Hard block** |")] + assert len(rows) == 1 + for split in (cells_cmark_gfm, cells_even_backslashes): + cells = split(rows[0]) + assert len(cells) == 5, (case, split.__name__, cells) + assert cells[1].strip() == "Blocks, no waiver" and cells[4].strip() == "File checks (code, no model)" + assert cells[2].strip().startswith("`") and cells[2].strip().endswith("`") + span = publish._path_span(path) + assert span.count("`") == 2 and "\n" not in span and "\u202e" not in span and "\u2066" not in span + assert "<" not in span + # Every pipe has exactly one backslash before it. + assert not re.search(r"(? HTTP 500 after: Callable[[FakeGitHub, HttpRequest], None] | None = None requests: list = field(default_factory=list) @@ -157,10 +163,17 @@ def _route(self, req: HttpRequest, path: str, query: dict) -> HttpResponse: (f"/issues/{PR}/comments", self.issue_comments), (f"/issues/{PR}/timeline", self.timeline), (f"/pulls/{PR}/comments", self.review_comments), + (f"/pulls/{PR}/commits", self.commits), ): if m == "GET" and path == API + suffix: chunk, headers = _page(items, query, req.url) return self._json(req, chunk, headers=headers) + if m == "GET" and path.startswith(f"{API}/dependency-graph/compare/"): + assert path.rsplit("/", 1)[1] == f"{self.merge_base}...{self.head}" + if self.dependency_graph_status != 200: + return HttpResponse(self.dependency_graph_status, self.dependency_graph_body, req.url, + dict(self.dependency_graph_headers)) + return self._json(req, []) if m == "GET" and path.startswith(f"{API}/collaborators/"): login = urllib.parse.unquote(path.split("/")[-2]) if login not in self.permissions: @@ -204,6 +217,11 @@ def pr_doc(self) -> dict: } +def signed_commit(verified: bool, n: int = 0) -> dict: + """A `GET /pulls/{n}/commits` item with only the fields the publisher reads.""" + return {"sha": f"{n:040x}", "commit": {"verification": {"verified": verified, "reason": "valid" if verified else "unsigned"}}} + + def api_comment(cid, login, body, created=None, updated=None, type="User"): created = created or T0 + timedelta(hours=1) return { @@ -363,7 +381,9 @@ def reason_rows(text: str) -> list[tuple[str, bool, str]]: """The rows of the reasons table of the record, as (code, waivable, detail).""" m = re.search(r"\*\*Reasons the gate failed\*\*\n\n(.*?)(?:\n\n|\Z)", text, re.S) assert m, "no reasons section" - return [(LABEL_CODE[label], flag.startswith("✅"), plain(detail)) for label, flag, detail in _REASON_ROW.findall(m.group(1))] + # Details are code spans: drop the backticks (a span never holds one). + return [(LABEL_CODE[label], flag.startswith("✅"), plain(detail).replace("`", "")) + for label, flag, detail in _REASON_ROW.findall(m.group(1))] def reason_codes(text: str) -> set[str]: @@ -734,7 +754,7 @@ def test_moved_line_alone_shows_the_finding_in_the_summary(tmp_path): second = run(tmp_path / "2", FakeGitHub(review_comments=posted_by(first)), bundle=moved_bundle(), passes=clean_passes(review=[finding(title="bug", line=11, explanation="Same words.")])) assert second.gh.bodies("POST", "/reviews") == [] - assert "src/app.py line 11 (head)" in plain(second.summary) + assert "`src/app.py` line 11 (head)" in plain(second.summary) assert "Differs from the inline comment of an earlier run" in plain(second.summary) @@ -938,13 +958,21 @@ def end(kind): s = r.summary assert r.code == 1 and s.startswith(verdict.STICKY_MARKER) assert "Not verified: file is not in the changed files" in plain(s) + # A path shows as plain text in a code span: no backslash escapes, no + # backtick (it ends the span), no "<", and references still broken. + span = publish._path_span(cut["file"], "file") + assert span in s + assert span.endswith("\\'‹!--&@" + ZWSP + "user #" + ZWSP + "12 [/`") + assert span.count("`") == 2 and "<" not in span for kind, value in cut.items(): + if kind == "file": + continue shown = publish.escape_field(kind, value) assert shown in s, kind assert not shown.endswith(publish.TRUNCATED) assert plain(shown) == value, kind # every character shows as text escaped_end = r"\\\`\<\!\-\-\&\;\@" + ZWSP + r"user \#" + ZWSP + r"12 \[" - assert shown.endswith(escaped_end + "\\/" * (kind == "file")), kind + assert shown.endswith(escaped_end), kind body = s.replace(verdict.STICKY_MARKER, "") assert not re.search(r"(?" not in body and "**bold**" not in body