diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 08242de..09dfe42 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -57,3 +57,6 @@ jobs: - name: Run the unit tests working-directory: review run: uv run --locked pytest tests/unit + - name: Run the release tests + working-directory: review + run: uv run --locked pytest tests/unit -m release diff --git a/docs/build-plan.md b/docs/build-plan.md index 983f2c8..817536b 100644 --- a/docs/build-plan.md +++ b/docs/build-plan.md @@ -55,7 +55,7 @@ Tasks: 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. +- [x] 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). The read-back is not recorded yet; do it before V26 b. - [ ] 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. - [x] Owner: a test workspace with a very small spend limit for V9. @@ -92,7 +92,7 @@ Depends on: none for most tasks; V18 for `collect.py`, V16 for the push epoch in ### 1b. Analyze and publish -- [x] `analyze.py`: one Messages API call per pass, no tools, `claude-opus-5-5`, explicit `effort` and `max_tokens`, structured output with the stripped schema and a full schema check in code (V5), fallback, at most 2 retries, a request deadline, stale check before the calls. The spend limit error (HTTP 400 `invalid_request_error`, "workspace API usage limits") is an outage with its own clear reason, not a request bug (V9). Always writes exactly three pass records, and exits 0 when it wrote them. The pass record has the optional `meta` field (reason, model, stop reason, schema digest, token usage). Federation goes through the SDK's `WorkloadIdentityCredentials`; the IDs are placeholders until the owner creates the production rule, and the `release`-marked test (`pytest -m release`) fails until phase 3 sets them. Tests use a fake client. +- [x] `analyze.py`: one Messages API call per pass, no tools, `claude-opus-5-5`, explicit `effort` and `max_tokens`, structured output with the stripped schema and a full schema check in code (V5), fallback, at most 2 retries, a request deadline, stale check before the calls. The spend limit error (HTTP 400 `invalid_request_error`, "workspace API usage limits") is an outage with its own clear reason, not a request bug (V9). Always writes exactly three pass records, and exits 0 when it wrote them. The pass record has the optional `meta` field (reason, model, stop reason, schema digest, token usage). Federation goes through the SDK's `WorkloadIdentityCredentials`; placeholder IDs make it refuse to call, and the `release`-marked test (`pytest -m release`) fails on them; phase 3 set the production IDs. Tests use a fake client. - [x] `publish.py`: escaping, inline `COMMENT` review, sticky summary, final check, exit code (0 only for `pass`). It never approves, requests changes, merges, closes, labels or edits the PR (spec decision K-1). A summary over the comment limit is cut in the spec's order, or fails closed with the full record in the gate job log. The summary shows token counts, no currency. Depends on: V3, V5, V9 (analyze); V6, V16, V24 (publish). @@ -104,7 +104,7 @@ Depends on: V3, V5, V9 (analyze); V6, V16, V24 (publish). - [ ] The dependency-review job exports the step outcome as the job output named `outcome` (`publish.py` reads `needs.dependency-review.outputs.outcome`, and `DEPENDENCY_REVIEW_OUTCOME` if the workflow sets it; the two must agree). - [ ] Guard every artifact download in the gate job with an explicit non-empty id check (D10). An empty `artifact-ids` makes `actions/download-artifact` fetch every artifact of the run. When analyze failed, its ids are empty, and `publish.py` must find a missing directory and fail with the `artifact` reason while it still posts the summary. - [ ] A unit test parses the workflow: no `continue-on-error`, the dependency-review job exports the step `outcome`, every `uses:` is a full SHA. -- [ ] `.github/workflows/test.yml`: unit tests on PRs to this repo (`pull_request`, no path filter) and on pushes to `main`. The job is `unit-tests`, with `contents: read`, no `id-token`, no environment and no secrets, so it runs a PR's code with a read-only token (the no-execute rule covers the review workflow). +- [ ] `.github/workflows/test.yml`: unit tests on PRs to this repo (`pull_request`, no path filter) and on pushes to `main`. The job is `unit-tests`, with `contents: read`, no `id-token`, no environment and no secrets, so it runs a PR's code with a read-only token (the no-execute rule covers the review workflow). From phase 3 the job has a second step, `pytest tests/unit -m release`, so the check name does not change. - [ ] Every workflow job starts with harden-runner and has `timeout-minutes`. Actions are pinned to the newest release at least 7 days old, except `setup-uv` v10.3.2 (spec K-15). Python is the runner image's 3.12, and `setup-uv` has an exact version and checksum, with its cache and the Astral mirror off. Artifact names carry the run attempt. - [ ] `.github/workflows/eval.yml`: manual fixture evals. The job uses `environment: eval` and has `contents: read` and `id-token: write` only. A workflow unit test reads every workflow file and fails if a workflow other than `eval.yml` uses `environment: eval`, or if a job that uses `eval` sets no explicit job-level `permissions` or has any `write` permission other than `id-token: write`. Job-level `permissions` is required even if the workflow level sets it. `write-all` at either level fails the test, and so do `read-all` and a missing job-level block, because the job must state `contents: read` and `id-token: write` itself. - [ ] `review/src/oc_review/evals.py`: the runner that `eval.yml` calls (`python -m oc_review.evals --fixtures-dir ... --out-dir ...`), with placeholder eval federation IDs and one smoke fixture. The phase 2 fixtures and IDs come later. `analyze` takes an optional federation IDs parameter for it. @@ -129,7 +129,7 @@ Exit criteria: The fixture eval calls the live API and costs money. The owner runs it on request only. It is the one exception to the no-execute rule: it may run a PR's code, only after an owner approves the `eval` environment run (K-4). -- [x] V27, before the first eval (passed 2026-10-27, throwaway branch `v27-probe`, never merged; Console deny reasons and the P1 scope are pending owner confirmation): test the eval federation rule (an approved `eval.yml` run gets a token; the deny cases in the spec get none, including the other-repo case, spec P2-2; it ran as the substitute N4, `push` in claude-review-test with no environment, 401, so repository isolation is not tested separately). +- [x] V27, before the first eval (passed 2026-10-27, throwaway branch `v27-probe`, never merged; Console deny reasons read by the owner on 2026-10-27: `match_claim_absent` for N1 and the no-environment job of the rejected-approval run, `match_claim_value_mismatch` for the no-environment job of the push run, N3 and N4; claim name and P1 scope not shown in the list view, so P1 scope not confirmed): test the eval federation rule (an approved `eval.yml` run gets a token; the deny cases in the spec get none, including the other-repo case, spec P2-2; it ran as the substitute N4, `push` in claude-review-test with no environment, 401, so repository isolation is not tested separately). - [x] Real eval federation IDs in `evals.py`, checked by a unit test against the production placeholders (spec P2-1). - [x] Eval runner: expectation format v2 and the rendered summary per case (spec "Testing", P2-5). - [x] Fixture builder `review/tests/eval_fixtures.py` and the 13 cases of the spec's minimum set in `review/tests/fixtures`, with expected outcomes. Unit tests: each bundle equals a rebuild, and each hard-block case fails the gate whatever Claude returns. @@ -142,7 +142,7 @@ Depends on: phase 1; V27. Exit criteria: -- [x] V27 passes. +- [x] V27 passes, with caveats: the "another environment" case was not run, and the repository check was not tested separately (N4 ran as `push` with no environment); P1 scope `workspace:developer` not confirmed (the Console list view does not show it). - [x] Every fixture in the minimum set gives its expected outcome: one full run with all 13 cases matched (spec P2-4; run 40445885087). - [x] The hard-block fixtures (bidi, git URL lockfile) block whatever Claude returns. The unit tests prove that both fail the gate whatever Claude returns, and run 1 confirms it. @@ -150,6 +150,9 @@ Exit criteria: Run the spec's workflow acceptance tests under a test org ruleset that requires the central workflow from `OperationCode/.github` at the candidate release SHA and targets only claude-review-test. Only one ruleset requires the workflow there at a time. +- [x] Production IDs in `analyze.py` and the release tests in CI (the candidate S1). R1 `claude-review-prod-r1` (`fdrl_01N2ZktTiVKqUvvXwywBFfys`) and the production service account are set; `test.yml` runs `pytest tests/unit -m release`. +- [x] V26 a: R1 created at the placeholder SHA and accepted by the Console (2026-10-27); R2 `claude-review-p3-r2` created the same way, inert until the release simulation. +- [x] P3-2: an over-long string no longer fails a pass. Release eval run 40517202830 at S1 matched 12 of 13: one supply-chain quote went over 300 characters, which the API cannot enforce. Analyze now cuts every string over its schema `maxLength` (summary, file, quote, title, explanation) to that limit and keeps the finding (spec P3-2). Fix round 1: a cut file path ends in "/", so it names no changed file; when the file, title or quote is at its limit, the inline comment key covers every shown field, so a cut never hides a distinct explanation. Fix round 2: a redaction can make two titles equal far below the limit, so the key of every finding covers every shown field, at any length. Fix round 3: each inline comment carries the D14 key and the full key, both of the redacted shown text; one run dedupes by full key, a later run skips a D14 key already on the PR and shows the finding in full in the summary when its full key is new. Fix round 4: an earlier comment counts only when its text checks out (the D14 skip needs the inline layout, path, side and title; "see the earlier comment" needs the exact body at the same line), and an unverified high or critical finding is never cut from the summary. A candidate with this change needs its own eval. - [ ] V26 b, before the acceptance tests: after the owner sets the production rule to the candidate SHA, a run at that SHA gets a token and a run at another SHA is denied. - [ ] V26 c, before V22 b: release steps 1 to 6 with a scratch rule R2 on the test workspace, R1 unchanged. Include the second exchange at the release SHA, and check that the workflows rule accepts a scratch ruleset pinned to a SHA that is only on a non-default branch of `.github`. - [ ] A fork PR from an account outside the org, including a draft that later becomes ready. The review starts with no approval click (V8 b). @@ -159,7 +162,7 @@ Run the spec's workflow acceptance tests under a test org ruleset that requires - [ ] V22 a and b: pin behind main, then a pin move with PRs open. - [ ] V23: a same-path workflow file in the target repo. - [ ] V25: the pilot bypass. Maintain and Admin can merge past a failed run, Write cannot, and GitHub records each bypass. Record the role `actor_id` values, and whether a direct push to the default branch is blocked. -- [ ] The rest of the acceptance list in the spec's "Testing" section, including the outage process (V9). +- [ ] The rest of the acceptance list in the spec's "Testing" section, including the outage process. Design change (spec P3-1): the invalid federation rule case runs live with a denied exchange; the exhausted budget case is covered by V9 (phase 0) and the unit tests, with no Console limit edit. Owner: @@ -187,6 +190,7 @@ Exit criteria: - [ ] Start a run on each open PR in the pilot repos (push, or close and reopen). - [ ] Each week, record false positives, overrides, bypasses, failed runs by reason, egress blocks and cost per run. Keep cost numbers (any currency amount) in the owner's private notes and the Anthropic Console, never in this public repo. Public text may give ratios and timing only. - [ ] Fix false positives in prompts or code. Each fix goes through phase 2 again, then ships as in "Each release" below. +- [ ] Plan publication before writing: bounded continuation comments, pointed to from the sticky summary, hold every summary-only finding (changed keys, unverified, outside the diff) when one comment cannot. Then no explanation is cut from the PR comments and summary capacity no longer fails a run whose computed result is `pass` (spec "Publishing (code)", size limit residual; review r-p3-cut-final Important 1). Keep the fail-closed check until this path is complete. Ships as in "Each release". Depends on: phase 3. @@ -208,7 +212,7 @@ Exit criteria: - [ ] Owner: CODEOWNERS in each supported repo. - [ ] 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. +- [ ] 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). - [ ] 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. diff --git a/docs/claude-review-design.md b/docs/claude-review-design.md index 6675b3b..38592d9 100644 --- a/docs/claude-review-design.md +++ b/docs/claude-review-design.md @@ -197,7 +197,7 @@ Only the gate job writes to GitHub, and it writes only comments. It has no Anthr Why each host: `github.com` (checkout, setup-uv), `raw.githubusercontent.com` and `release-assets.githubusercontent.com` (uv version manifest and uv download), `files.pythonhosted.org` (`uv sync` from `uv.lock`), `api.github.com` (REST reads and writes), `pypi.org` and `registry.npmjs.org` (collect's registry lookups), `results-receiver.actions.githubusercontent.com` and `*.blob.core.windows.net` (artifact upload and download), `api.anthropic.com` and the Actions OIDC token host (analyze). The blob host and the OIDC host (`run-actions-N-azure-REGION.actions.githubusercontent.com`) change per run, so they need wildcards; the analyze wildcard also covers `results-receiver`. The `test.yml` and `eval.yml` lists are not measured: they hold the hosts that the same checkout, setup-uv and `uv sync` steps (and, for `eval.yml`, the token exchange and the API) reached in V10, and `test.yml` adds `results-receiver.actions.githubusercontent.com`, which the runner itself reached. Audit mode records egress and blocks nothing. Block mode is `egress-policy: block` plus `disable-telemetry: true`; the action accepts `disable-telemetry: true` only in block mode, so in audit mode harden-runner sends telemetry to StepSecurity. Block mode is revisited before org-wide enforcement (K-14, build plan phase 5). The allowlist is defense in depth. It does not prove that no data can leave, because GitHub itself is on the list. -- **Other workflows:** `test.yml` runs the unit tests of a PR to this repo (`pull_request`, no path filter, and `push` to `main`) with `contents: read` only, no `id-token`, no environment and no secrets. Its job `unit-tests` is the required check name. It runs the PR's own code with that read-only token, so it is not the review workflow: the no-execute rule (and `CLAUDE.md`'s wording of it) covers the review workflow, which never runs PR content. `eval.yml` is `workflow_dispatch` only (never `workflow_call`), has `permissions: {}` at the top, and one job with the `eval` environment (see "Fixture eval credential"). +- **Other workflows:** `test.yml` runs the unit tests of a PR to this repo (`pull_request`, no path filter, and `push` to `main`) with `contents: read` only, no `id-token`, no environment and no secrets. Its job `unit-tests` is the required check name, and it has two test steps: the default unit tests, then `pytest tests/unit -m release` (the `release`-marked tests, see "Testing"), so the check name does not change. It runs the PR's own code with that read-only token, so it is not the review workflow: the no-execute rule (and `CLAUDE.md`'s wording of it) covers the review workflow, which never runs PR content. `eval.yml` is `workflow_dispatch` only (never `workflow_call`), has `permissions: {}` at the top, and one job with the `eval` environment (see "Fixture eval credential"). - **Timeouts:** 10 minutes per job (`eval.yml`: 30 minutes). Each Messages API call also has a request deadline (see "Request shape"). A job that times out or loses its runner fails, so the run fails. - **Merge queue:** not supported in v1. Repos that enable a merge queue stay out of the target list until a `merge_group` path is designed and tested. @@ -350,7 +350,7 @@ Three passes, each one call to the Messages API with no tools. Do not use `claud - **Model:** `claude-opus-5-5`. Adaptive thinking is always on for this model ([what's new](https://platform.claude.com/docs/en/models/opus-5-5/whats-new-opus-5-5)). Set `output_config.effort` explicitly per pass. - **Output limit:** set `max_tokens` explicitly per pass, in code. Thinking tokens count toward `max_tokens` ([Messages API](https://platform.claude.com/docs/en/api/messages)), so the value bounds thinking plus output. The values are 16,000 for `review` and `security` and 32,000 for `supply-chain` (tuned in the phase 2 evals). A `max_tokens` stop makes the pass `failed`. -- **Structured output:** `output_config.format` with the findings JSON schema below. Structured outputs do not support some JSON Schema keywords: `minLength`, `maxLength`, `minimum`, `maximum`, `exclusiveMinimum`, `exclusiveMaximum`, `multipleOf`, `minItems`, `maxItems` and `uniqueItems`. So analyze sends a stripped copy of the schema without these keywords, removed only where they are schema keywords (a property named `maximum` stays), and records the digest of the schema it sent in `meta.request_schema_sha256` on every pass record. The full schema stays the contract. Analyze checks every response against it, and a response that fails it makes the pass `failed`. The publisher checks it again when it reads the pass envelope (see "Job handoff and provenance"). Code never truncates findings to fit. The prompts state the limits, so the model knows them (V5). A pass with more findings than its limit (10, 10, 15) that is valid against the full schema stays `ok`; the gate fails it in one place, not waivable (see "Reasons"), and its findings still show. +- **Structured output:** `output_config.format` with the findings JSON schema below. Structured outputs do not support some JSON Schema keywords: `minLength`, `maxLength`, `minimum`, `maximum`, `exclusiveMinimum`, `exclusiveMaximum`, `multipleOf`, `minItems`, `maxItems` and `uniqueItems`. So analyze sends a stripped copy of the schema without these keywords, removed only where they are schema keywords (a property named `maximum` stays), and records the digest of the schema it sent in `meta.request_schema_sha256` on every pass record. The full schema stays the contract. Analyze checks every response against it, and a response that fails it makes the pass `failed`. The publisher checks it again when it reads the pass envelope (see "Job handoff and provenance"). Code never drops a finding to fit, and it cuts text in one case only: before the check, analyze cuts every string over its schema `maxLength` (`summary`, `file`, `quote`, `title`, `explanation`) to that limit, and the finding stays (P3-2). A cut file path ends in "/" so that it names no file (P3-2). The cut is driven by the full schema, not by a list of fields. When the string holds a credential-like string, found as the publisher finds it, the cut is taken from the redacted string, so no part of a secret survives the cut (the publisher redacts before it cuts, see "Publishing (code)"). The cut never changes category, severity, confidence, side or line, and blocking never reads the cut strings, so it cannot lower risk. Every other schema error (type, `minLength`, enum, a missing or extra key) still fails the pass. The prompts state the limits, so the model knows them (V5). A pass with more findings than its limit (10, 10, 15) that is valid against the full schema stays `ok`; the gate fails it in one place, not waivable (see "Reasons"), and its findings still show. - **Refusals:** security content can trip safety classifiers. Enable the server-side fallback (`fallbacks: "default"` with beta `server-side-fallback-2026-07-01`). Every attempt that produced output is billed separately ([refusals and fallback](https://platform.claude.com/docs/en/build-with-claude/refusals-and-fallback)), so the cost bound counts each fallback attempt. Record the top-level `model` field. Always check `stop_reason` before reading content: only `end_turn` is accepted, and `max_tokens`, `refusal`, `pause_turn` or any other value makes the pass `failed`, even when the text looks like valid JSON. Analyze reads the last `text` block, so a fallback block before it is ignored. Any API error also makes the pass `failed`, which makes the gate fail. Each failed pass records a clear reason in `meta.reason`: "workspace spend limit reached" (see "Auth and cost"), "request rejected" (any other 4xx, none retried), "authentication failed" (401 or 403, or a token that cannot be read), "API unavailable" (408, 409, 429, 5xx, a connection error or a timeout, after the retries), "deadline exceeded", "render failed", "PR read failed", "federation not configured", "OIDC token unavailable", "OIDC claim mismatch", "static credential refused", "API host not allowed", "missing environment variable: ", a response with no text block, that is not valid JSON or that does not match the findings schema (the reason names the JSON path and the keyword, never the value), and "internal error". - **Retries and deadline:** at most 2 SDK retries per call, and a request deadline per call that fits inside the job timeout. Each pass has an absolute deadline of 420 seconds from the start of analyze. Every HTTP request, each retry and the token exchange included, starts before that deadline and gets at most the time left. A retry whose wait (`retry-after`, or the SDK backoff) would reach the deadline does not start, so a retry never begins after the deadline. A pass with no result at the deadline is `failed` with "deadline exceeded", and analyze writes its records and exits 0 without waiting for it. With the 10-minute job timeout, that leaves time to write the records. - **Redaction:** credential-like strings in the bundle are replaced with `[REDACTED:]` before rendering. The security pass sees the marker and its location, never the value. The citation check uses this redacted text (see "Citation check"). Credential-like strings are known token prefixes, private keys, JWTs, `user:password@` in URLs, and values after a credential name (`password = ...`, `api_key: ...`) or a `Bearer` or `Basic` scheme that pass a fixed secret test. Redaction keeps the line count, so line numbers do not move (R-LINES). @@ -439,6 +439,8 @@ Two separate questions: is the evidence real, and can the finding be an inline c 2. `line` exists in that side's blob, or in the rendered title or body. 3. `quote`, with whitespace normalized, appears in the redacted text of that side's blob (see "Redacted text"), or in the rendered title or body, within 3 lines of `line`. +A quote or file that analyze cut to its limit (see "Request shape") is checked as cut. The cut keeps a prefix, so a quote that is at that place stays verified after the cut, unless the prefix is only whitespace or a credential-like string was redacted before the cut; then the finding is unverified and still blocks. A file path is an identifier, not prose: a plain prefix of a long path can be the path of a different changed file. So a cut path keeps the first 399 characters and ends in "/". No file path ends in "/", and the file lookup never matches such a path, so that finding is unverified, goes to the summary and still blocks. + **PR title and body.** A finding in the title or body cites `/.pr/title` or `/.pr/body` with `side: "head"` (K-6). It never cites a code file. Code checks it against the rendered title or body text. It is never inline-eligible, because it is not in the diff, so it goes to the summary only. The publisher escapes it like all untrusted text. **Redacted text.** Code compares each quote with the same redacted text that the model saw. A quote may be exactly one `[REDACTED:]` marker only when the line has no other text. Such a citation is verified only when that marker is on the cited line. A quote never rebuilds a credential. A finding on a redacted line always stays, verified or not (K-6). @@ -565,9 +567,9 @@ There is one build of the central workflow. Its gate job always applies every ru - **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. - **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. -- **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. Skip a finding that was already posted on an earlier run. The key is the SHA-256 of file, side, title and quote, kept in a hidden `` in each inline comment and read back only from review comments by `github-actions[bot]`. 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. +- **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"). It contains the run ID and attempt, the gate result, every reason with its waivable flag, the head SHA, merge base SHA and base ref it was computed for, the override (if any), prefilter results, coverage, the dependency delta, findings per pass, unverified findings, the token usage of the run per pass, and a link to the workflow run. Usage is token counts only (input, output, cache write, cache read, iterations); the code holds no price and the repo shows no currency. Findings that were posted inline show one line that points to the inline comment. 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. -- **Size limit and cut order:** a comment body is at most 65,536 characters, and the code aims at 64,000. If the body is too long, it cuts only these blocks, in this order, each from its end: unverified findings, low and medium findings, dependency delta, prefilter signals. Each cut says "truncated, see the run log". The result, the reasons, the SHAs, the override data, the high and critical findings, the model summaries and the hard block and coverage details are never cut. If those alone do not fit, the run fails closed: the comment becomes a short notice of fixed size with result `fail`, the run and attempt, the SHAs and base ref, the computed gate result, a count per reason code 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. +- **Size limit and cut order:** a comment body is at most 65,536 characters, and the code aims at 64,000. 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 delta, prefilter signals. Each cut says "truncated, see the run log". The result, the reasons, the SHAs, the override data, the high and critical findings (verified or not, so also one whose file path analyze cut), the model summaries and the hard block and coverage details are never cut. If those alone do not fit, the run fails closed: the comment becomes a short notice of fixed size with result `fail`, the run and attempt, the SHAs and base ref, the computed gate result, a count per reason code 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. @@ -577,7 +579,7 @@ There is one build of the central workflow. Its gate job always applies every ru ## Auth and cost -- **Anthropic credential:** workload identity federation only. The analyze job gets a GitHub OIDC token with audience `https://api.anthropic.com`, decodes its claims without verifying them and requires `job_workflow_sha` to equal `job.workflow_sha` (and `aud` to hold the audience), then writes the token to a file with mode 0600 under `RUNNER_TEMP`. The file is deleted after the calls, and the token is never printed or put in argv. The `anthropic` SDK (1.17.0) supports federation natively, and does the token exchange and refresh itself. Analyze does not set the SDK's `ANTHROPIC_*` environment variables. It passes the three IDs and the token file to `WorkloadIdentityCredentials` in code, because with an explicit credential the SDK reads no credential from the environment, so an `ANTHROPIC_API_KEY`, `ANTHROPIC_AUTH_TOKEN` or profile on the runner cannot stand in for federation. The client also pins the base URL to `https://api.anthropic.com` (so `ANTHROPIC_BASE_URL` cannot move the calls or the exchange), refuses a static credential, or a credential header such as `X-Api-Key` or `Authorization` from `ANTHROPIC_CUSTOM_HEADERS`, with the pass reason "static credential refused", and sends every request through a guard that refuses another host, a scheme other than https and a request after the deadline. The IDs are not secret. They are constants in `analyze.py`, in the pinned central code, not in repo variables or secrets, so a repo cannot change them. Until the owner creates the production rule, they are placeholders, and analyze makes no call: each pass is `failed` with "federation not configured". A unit test marked `release` (skipped by default) fails on a placeholder; phase 3 turns it on. So each release names its own federation rule ID (see "Each release" below). +- **Anthropic credential:** workload identity federation only. The analyze job gets a GitHub OIDC token with audience `https://api.anthropic.com`, decodes its claims without verifying them and requires `job_workflow_sha` to equal `job.workflow_sha` (and `aud` to hold the audience), then writes the token to a file with mode 0600 under `RUNNER_TEMP`. The file is deleted after the calls, and the token is never printed or put in argv. The `anthropic` SDK (1.17.0) supports federation natively, and does the token exchange and refresh itself. Analyze does not set the SDK's `ANTHROPIC_*` environment variables. It passes the three IDs and the token file to `WorkloadIdentityCredentials` in code, because with an explicit credential the SDK reads no credential from the environment, so an `ANTHROPIC_API_KEY`, `ANTHROPIC_AUTH_TOKEN` or profile on the runner cannot stand in for federation. The client also pins the base URL to `https://api.anthropic.com` (so `ANTHROPIC_BASE_URL` cannot move the calls or the exchange), refuses a static credential, or a credential header such as `X-Api-Key` or `Authorization` from `ANTHROPIC_CUSTOM_HEADERS`, with the pass reason "static credential refused", and sends every request through a guard that refuses another host, a scheme other than https and a request after the deadline. The IDs are not secret. They are constants in `analyze.py`, in the pinned central code, not in repo variables or secrets, so a repo cannot change them. They are set (2026-10-27): rule R1 `claude-review-prod-r1` (`fdrl_01N2ZktTiVKqUvvXwywBFfys`), organization `2bc920f9-977b-43b2-870a-f840e4ce809c`, and service account `claude-review` (`svac_01PiMgfb6qJgNpLZUV8hprCC`), in the Console workspace `claude-review`. Placeholder (all-zero) IDs make analyze refuse to call: each pass is `failed` with "federation not configured". The unit test `test_release_federation_ids_are_not_placeholders`, marked `release` (skipped in the default run), fails on a placeholder, and CI runs it on every PR since phase 3. So each release names its own federation rule ID (see "Each release" below). - **Federation rule** ([WIF reference](https://platform.claude.com/docs/en/manage-claude/wif-reference)). The production rule, in the shape that passed V3 (V3 used the scratch workflow path and SHA, and also matched `repository_owner`): ``` @@ -624,7 +626,7 @@ token lifetime: 10 minutes A queued run does not count for step 8; only a completed run at the release SHA does (`job.workflow_sha`). Never use a re-run of a run at S1 to migrate a PR: after R1 is deleted, it fails auth. - **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 deny reasons and the P1 scope (`workspace:developer` in `claude-review-p0-test`) are pending owner confirmation in the Console authentication history, by request id. +- **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. @@ -633,7 +635,7 @@ token lifetime: 10 minutes - **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. - **No per-PR quota in v1.** A real per-PR cap needs an atomic, trusted counter that reserves each attempt before the calls start, including reopens, retries, concurrent runs and runs that fail before publishing. GitHub Actions has no such store, and a count kept in a bot comment is neither atomic nor trusted. v1 relies on the workspace limit. This is an outage boundary, not a quota: one abusive PR can use it up and stop reviews for every PR. Two cheap controls reduce waste without claiming a cap: analyze skips the Claude calls when its run is stale, and the size gate bounds one run. The response to abuse is the outage process plus closing or locking the PR, or blocking the user. A per-PR quota needs a central service and is a later design. - **Worst case per run:** three passes, each with all allowed retries and fallback attempts, each with a cold cache write of the bundle and a full `max_tokens` output. Compute this from current list prices before the pilot ruleset is enabled, and keep the numbers out of this public repo. -- **Outage process:** when the API, auth or budget fails, every gate fails and merges block. The owner announces it, then either waits, raises the limit, or for urgent merges uses the ruleset bypass (org admins; on a pilot repo also maintainers and repo admins; logged by GitHub). V9 tested the spend limit case with a scratch workflow. The acceptance tests repeat it with the built workflow before the pilot ruleset is enabled. +- **Outage process:** when the API, auth or budget fails, every gate fails and merges block. The owner announces it, then either waits, raises the limit, or for urgent merges uses the ruleset bypass (org admins; on a pilot repo also maintainers and repo admins; logged by GitHub). V9 tested the spend limit case with a scratch workflow. Design change (phase 3, P3-1): the acceptance tests do not lower the spend limit of a workspace in the Console. The evidence for the exhausted-budget case is V9 (a real spend limit on a scratch workflow) plus the unit tests that classify the limit error as an outage with its own reason and fail the gate. The "invalid federation rule" outage case is still run live, with a denied exchange (V26 b). - **Cost reporting:** the summary comment shows the token usage of the run from `meta.usage`, per pass, with no currency (the code holds no price list, and this repo records no currency amounts). The owner converts tokens to money in private notes. The publisher also appends one JSON line per run to the workflow summary (run ID, attempt, result, reason codes, and status, model and token counts per pass), so monthly use can be added up from Actions logs. ## Repo layout @@ -673,7 +675,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), 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`. One test, marked `release` and skipped by default, 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, 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`. @@ -717,7 +719,7 @@ token lifetime: 10 minutes - PR that was open before the repo joined the ruleset - pin moved to a new release while PRs are open (V22) - egress in audit mode with the measured lists (block mode is revisited before org-wide enforcement, K-14) - - invalid federation rule and an exhausted test budget (outage process) + - invalid federation rule (outage process): a denied exchange, run live. An exhausted budget is covered by V9 and the unit tests, with no Console limit edit (phase 3 design change, P3-1) - **Live smoke test:** the scratch repo above, before any real repo. ## Rollout @@ -738,7 +740,8 @@ The tasks, owner actions and exit criteria for each step are in [docs/build-plan - [ ] Create the public repo `OperationCode/.github` and push this local repo. - [ ] 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). 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. +- [x] 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); no read-back is recorded yet. +- [ ] 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. - [ ] 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/)). @@ -791,8 +794,8 @@ These are assumptions I could not confirm from docs. Each has a concrete test. I 23. **Same-path workflow in the target repo (blocking).** T6 was inconclusive: the PR's copy never ran, and the scratch repo was both source and target. Use a source repo separate from the target. Test: a fork PR adds `.github/workflows/claude-review.yml` to the target repo with a passing job `oc-claude-review`, while the central run fails. Pass when the box blocks. 24. **Answered: run list for the final check (build).** 2026-10-25, pass. The job's `GITHUB_TOKEN` with `actions: read` listed runs by head SHA on same-repo `pull_request` runs and on `pull_request_target` runs of fork and same-repo PRs, including ruleset-started runs, with the correct `path`. A new run first appeared in the list within 5 s of its creation in every measurement (poll step about 3 s). The final check stays defense in depth. Evidence: [40227607202](https://github.com/OperationCode/claude-review-test/actions/runs/40227607202) and [40227659578](https://github.com/OperationCode/claude-review-test/actions/runs/40227659578) (same-repo); [40230848821](https://github.com/OperationCode/claude-review-test/actions/runs/40230848821) (ruleset-started runs, PR [17](https://github.com/OperationCode/claude-review-test/pull/17)); b1 run A [40236352621](https://github.com/OperationCode/claude-review-test/actions/runs/40236352621) listed run B (fork PR [15](https://github.com/OperationCode/claude-review-test/pull/15)). 25. **Pilot bypass (blocking).** The docs list the `RepositoryRole` actor type but not the `actor_id` values of the built-in roles. Test: in the scratch repo, an org ruleset with only the workflows rule and a bypass for the Maintain and Admin repository roles in `pull_request` mode. Read it back and record the `actor_id` values. Pass when, on a failed run, a user with Maintain and a user with Admin can merge with the bypass, a user with Write cannot, and GitHub records each bypass. Also record whether a direct push to the default branch is blocked; before a repo joins the pilot, check it for people or bots that push to the default branch directly. -26. **Placeholder SHA in a federation rule (verify in phase 3; part a at setup).** The setup and every release create a rule at the placeholder SHA `0000000000000000000000000000000000000000` and later edit it to a real SHA ("Each release" in "Auth and cost"). V5 and V9 edited a rule from one real SHA to another; no test created a rule with a SHA that matches no commit. Test: (a) at setup, create the production rule at the placeholder and read it back. If the Console rejects it, stop, and resolve the order of rule ID and SHA before the production setup goes on. (b) In phase 3, before the acceptance tests, edit the rule to the candidate SHA and read it back. Pass when a run under the scratch test ruleset at the candidate SHA gets a token, and a run at another SHA is denied. (c) Before the first release (V22 b), do steps 1 to 6 of "Each release" with a scratch rule R2 on the test workspace, including the re-edit of R2 to the merge SHA and the second exchange. Do not change R1 during this check. Pass when R2 reads back at the placeholder, then at the candidate SHA, then at the release SHA, and the exchange succeeds at both real SHAs. The scratch ruleset is pinned to a SHA that is on a non-default branch of `.github` at step 4 (a fork branch counts if the fork is the test path); pass only if the workflows rule accepts and runs it. If it does not, validate after the merge, at the main SHA, and the order of the steps changes. -27. **Eval federation rule (verify in phase 2, before the first eval).** The rule cannot name `eval.yml`, because a dispatch run's `job_workflow_ref` ends in the branch ref (V3). It binds `repository_id`, `event_name` and `environment: eval` instead ("Fixture eval credential"). Passed 2026-10-27 (UTC) with one substitute and one case not run. The other-repo case ran as `push` in claude-review-test with no environment (N4, 401), so `repository_id` is not tested separately. The "another environment" case was not run; only the no-environment case ran (N1). The Console reason for each deny is not recorded yet, and the Console deny reasons and the P1 scope are pending owner confirmation. See "Fixture eval credential". Test on the test workspace `claude-review-p0-test`, and record the Console reason for each deny. Pass when an approved `eval.yml` dispatch run in `.github` gets a token with scope `workspace:developer` for that workspace only, and each of these gets no token: a run whose approval is rejected (the job never starts, and the Console shows no exchange); a dispatch run in `.github` with no environment or another environment; a dispatch run with an `eval` environment in another repo (claude-review-test, with its own `eval` environment; this case is required, P2-2); a run in `.github` with the `eval` environment on another event (for example `push`). Also pass when the workflow unit test (see "Testing") fails on a fixture workflow other than `eval.yml` that uses `eval`, and on a job that uses `eval` with a GitHub write permission. The residual stays: an approver who approves without reading. +26. **Placeholder SHA in a federation rule (verify in phase 3; part a done 2026-10-27).** The setup and every release create a rule at the placeholder SHA `0000000000000000000000000000000000000000` and later edit it to a real SHA ("Each release" in "Auth and cost"). V5 and V9 edited a rule from one real SHA to another; no test created a rule with a SHA that matches no commit. Test: (a) at setup, create the production rule at the placeholder and read it back. If the Console rejects it, stop, and resolve the order of rule ID and SHA before the production setup goes on. Result of part a (2026-10-27): the owner created R1 and the Console accepted it; no read-back output is recorded, so read R1 back before part b. R1 `claude-review-prod-r1` (`fdrl_01N2ZktTiVKqUvvXwywBFfys`) was created with `job_workflow_ref` at the placeholder SHA. The scratch rule R2 `claude-review-p3-r2` (`fdrl_01AfACzupW3U4Bx361ZUbsyR`; service account `claude-review-p0`, workspace `claude-review-p0-test`) was created the same way and stays at the placeholder until the release simulation of part c. (b) In phase 3, before the acceptance tests, edit the rule to the candidate SHA and read it back. Pass when a run under the scratch test ruleset at the candidate SHA gets a token, and a run at another SHA is denied. (c) Before the first release (V22 b), do steps 1 to 6 of "Each release" with a scratch rule R2 on the test workspace, including the re-edit of R2 to the merge SHA and the second exchange. Do not change R1 during this check. Pass when R2 reads back at the placeholder, then at the candidate SHA, then at the release SHA, and the exchange succeeds at both real SHAs. The scratch ruleset is pinned to a SHA that is on a non-default branch of `.github` at step 4 (a fork branch counts if the fork is the test path); pass only if the workflows rule accepts and runs it. If it does not, validate after the merge, at the main SHA, and the order of the steps changes. +27. **Eval federation rule (verify in phase 2, before the first eval).** The rule cannot name `eval.yml`, because a dispatch run's `job_workflow_ref` ends in the branch ref (V3). It binds `repository_id`, `event_name` and `environment: eval` instead ("Fixture eval credential"). Passed 2026-10-27 (UTC) with one substitute, one case not run, and the P1 scope not confirmed. The other-repo case ran as `push` in claude-review-test with no environment (N4, 401), so `repository_id` is not tested separately. The "another environment" case was not run; only the no-environment case ran (N1). The Console reason for each deny is recorded in "Fixture eval credential" (`match_claim_absent` for N1 and the no-environment job of the rejected-approval run; `match_claim_value_mismatch` for the no-environment job of the push run, N3 and N4); claim name and P1 scope not shown in the list view, so the P1 scope `workspace:developer` is not confirmed. See "Fixture eval credential". Test on the test workspace `claude-review-p0-test`, and record the Console reason for each deny. Pass when an approved `eval.yml` dispatch run in `.github` gets a token with scope `workspace:developer` for that workspace only, and each of these gets no token: a run whose approval is rejected (the job never starts, and the Console shows no exchange); a dispatch run in `.github` with no environment or another environment; a dispatch run with an `eval` environment in another repo (claude-review-test, with its own `eval` environment; this case is required, P2-2); a run in `.github` with the `eval` environment on another event (for example `push`). Also pass when the workflow unit test (see "Testing") fails on a fixture workflow other than `eval.yml` that uses `eval`, and on a job that uses `eval` with a GitHub write permission. The residual stays: an approver who approves without reading. Verify later (phase 3): @@ -894,8 +897,10 @@ V10: decisions from the V10 egress run, 2026-10-27 (UTC). Where a line below cha P2: phase 2 rulings, 2026-10-27 (UTC). Where a P2- line changes an earlier line, it wins. -- P2-1. Eval workspace. Evals reuse the test workspace `claude-review-p0-test` (not production, with its own spend limit) instead of a new eval workspace. They keep their own federation rule, `fdrl_01BL4n1gQzkrroCmDZoXUePm`, whose IDs are in `evals.py`; a unit test checks that they are valid, are not placeholders and differ from the production IDs. The production IDs in `analyze.py` stay placeholders until phase 3. The phase 3 scratch rules (V26 c) are on the same workspace, so they and evals share its spend limit, and a spent limit stops only them, never production reviews. -- P2-2. V27 includes the other-repo deny case: a dispatch run with its own `eval` environment in claude-review-test gets no token. Result: the case ran as a substitute, N4: `push` in claude-review-test, no environment, 401. The missing environment or the `push` event alone explains the 401, so the `repository_id` check is not tested separately; the dispatch case with its own `eval` environment was not run (see V27 in "Auth and cost"). +- P2-1. Eval workspace. Evals reuse the test workspace `claude-review-p0-test` (not production, with its own spend limit) instead of a new eval workspace. They keep their own federation rule, `fdrl_01BL4n1gQzkrroCmDZoXUePm`, whose IDs are in `evals.py`; a unit test checks that they are valid, are not placeholders and differ from the production IDs. The production IDs in `analyze.py` were placeholders until phase 3 and are set to R1 and the production service account (P3-1). The phase 3 scratch rules (V26 c) are on the same workspace, so they and evals share its spend limit, and a spent limit stops only them, never production reviews. +- P2-2. V27 includes the other-repo deny case: a dispatch run with its own `eval` environment in claude-review-test gets no token. Result: the case ran as a substitute, N4: `push` in claude-review-test, no environment, 401. The missing environment or the `push` event alone explains the 401, so the `repository_id` check is not tested separately; the dispatch case with its own `eval` environment was not run (see V27 in "Auth and cost"). Console reason for N4: `match_claim_value_mismatch` (claim name not shown in the list view). - 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-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). diff --git a/review/prompts/system.md b/review/prompts/system.md index 1f6b817..1fce68d 100644 --- a/review/prompts/system.md +++ b/review/prompts/system.md @@ -32,7 +32,7 @@ Code checks every citation against the same text that you see. Each finding cite - `file`: the path exactly as the bundle shows it, at most 400 characters. For `side: "base"` on a renamed file, the old path is allowed. - `side`: `head` for a line in the PR version. `base` for a line in the merge base version, for example a removed check. - `line`: the line number on that side, as the bundle shows it. An integer, 1 or more. Never count lines yourself from the patch. -- `quote`: text copied exactly from that line, 1 to 300 characters. Do not paraphrase, shorten with "...", or join lines. +- `quote`: text copied exactly from that line, 1 to 300 characters. When the line is longer, copy the part of it that shows the problem. Do not paraphrase, shorten with "...", or join lines. Cite the line that shows the problem most directly. When a problem spans several lines, cite the first line that is wrong. diff --git a/review/src/oc_review/analyze.py b/review/src/oc_review/analyze.py index 841ecee..5c92b6c 100644 --- a/review/src/oc_review/analyze.py +++ b/review/src/oc_review/analyze.py @@ -13,8 +13,8 @@ cannot be written. Federation: the IDs below are not secret. They live in the pinned code, so a -repo cannot change them, and each release names its own rule. Until the owner -creates the production rule they are placeholders, and analyze refuses to call. +repo cannot change them, and each release names its own rule. Placeholder +(all-zero) IDs make analyze refuse to call. """ from __future__ import annotations @@ -24,6 +24,7 @@ import binascii import copy import email.utils +import functools import hashlib import json import os @@ -42,7 +43,7 @@ import anthropic import httpx2 -from . import collect, envelope, render, snapshot +from . import collect, envelope, publish, render, snapshot PASS_NAMES = ("review", "security", "supply-chain") # Started first; the other passes start when its response begins to stream, @@ -73,10 +74,11 @@ CREDENTIAL_HEADERS = frozenset({"x-api-key", "authorization"}) # Production federation rule (spec "Auth and cost", "Each release"). -# Placeholders until the owner creates the rule: analyze refuses to call. -ANTHROPIC_FEDERATION_RULE_ID = "fdrl_000000000000000000000000" -ANTHROPIC_ORGANIZATION_ID = "00000000-0000-0000-0000-000000000000" -ANTHROPIC_SERVICE_ACCOUNT_ID = "svac_000000000000000000000000" +# Rule R1 `claude-review-prod-r1` and service account `claude-review`, in +# workspace `claude-review`. Each release names its own rule here. +ANTHROPIC_FEDERATION_RULE_ID = "fdrl_01N2ZktTiVKqUvvXwywBFfys" +ANTHROPIC_ORGANIZATION_ID = "2bc920f9-977b-43b2-870a-f840e4ce809c" +ANTHROPIC_SERVICE_ACCOUNT_ID = "svac_01PiMgfb6qJgNpLZUV8hprCC" FEDERATION_RULE_ID_RE = re.compile(r"^fdrl_[A-Za-z0-9]{24}$") ORGANIZATION_ID_RE = re.compile(r"^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$") @@ -212,9 +214,14 @@ def schema_digest(schema: Any) -> str: return hashlib.sha256(canonical_json(schema)).hexdigest() +@functools.cache +def findings_schema() -> Mapping[str, Any]: + """The full findings schema, the contract. Callers must not change it.""" + return envelope.parse_json((envelope.SCHEMA_DIR / "findings.schema.json").read_bytes()) + + def request_schema() -> dict[str, Any]: - full = envelope.parse_json((envelope.SCHEMA_DIR / "findings.schema.json").read_bytes()) - return strip_schema(full) + return strip_schema(findings_schema()) def build_request(pass_name: str, *, system_text: str, user_text: str, pass_text: str, @@ -271,9 +278,61 @@ def usage_of(message: Any) -> dict[str, int] | None: return out +def cut_string(text: str, limit: int) -> str: + """The first `limit` characters of `text`, with no part of a secret. + + The publisher redacts before it cuts, so no part of a secret survives a + cut. A cut here comes first, so when the text holds a credential-like + string (found the way the publisher finds it: after it removes control + and invisible characters), the cut is taken from that redacted text.""" + cleaned = publish.strip_controls(text) + redacted = render.redact_text(cleaned) + return (text if redacted == cleaned else redacted)[:limit] + + +# Properties that name a file. A path is an identifier, not prose. +PATH_FIELDS = frozenset({"file"}) + + +def cut_path(text: str, limit: int) -> str: + """A cut file path that names no file: `cut_string` to `limit - 1`, then "/". + + A plain prefix of a path can be the path of another changed file, and the + citation check would then verify the finding on that file. Git and GitHub + never name a file with a final "/", so the cut path names no changed file + and the finding stays unverified (and still blocks, P3-2).""" + return cut_string(text, limit - 1) + "/" + + +def cut_to_limits(doc: Any, schema: Mapping[str, Any], name: str | None = None) -> Any: + """Cut each string over its schema `maxLength` to that limit, in place + (spec "Request shape", P3-2). The finding stays, and nothing else changes. + + The API cannot enforce `maxLength`, so one long string would fail the + whole pass. The walk follows `properties` and `items`, the only structure + keywords of the findings schema (a unit test checks that it reaches every + `maxLength`). Every other keyword is left to the schema check. `name` is + the property that holds `doc`; a path is cut with `cut_path`.""" + if isinstance(doc, dict): + props = schema.get("properties") + for key, sub in props.items() if isinstance(props, Mapping) else (): + if key in doc and isinstance(sub, Mapping): + doc[key] = cut_to_limits(doc[key], sub, key) + elif isinstance(doc, list): + items = schema.get("items") + if isinstance(items, Mapping): + doc[:] = [cut_to_limits(v, items) for v in doc] + elif isinstance(doc, str): + limit = schema.get("maxLength") + if isinstance(limit, int) and len(doc) > limit: + return (cut_path if name in PATH_FIELDS else cut_string)(doc, limit) + return doc + + def parse_response(message: Any) -> PassResult: """Check `stop_reason` first, then parse the final text block and check it - against the full findings schema. Never truncates.""" + against the full findings schema. Only a string over its `maxLength` is + cut (`cut_to_limits`); nothing else is truncated.""" model = _clip(getattr(message, "model", None), META_MODEL_MAX) stop_reason = _clip(getattr(message, "stop_reason", None), META_STOP_REASON_MAX) usage = usage_of(message) @@ -289,6 +348,7 @@ def parse_response(message: Any) -> PassResult: doc = envelope.parse_json(texts[-1].text.encode("utf-8")) except (envelope.EnvelopeError, UnicodeEncodeError) as exc: raise PassFailure(R_BAD_JSON, **info) from exc + cut_to_limits(doc, findings_schema()) try: envelope.validate_findings(doc) except envelope.EnvelopeError as exc: diff --git a/review/src/oc_review/publish.py b/review/src/oc_review/publish.py index 4f738c1..64d09bc 100644 --- a/review/src/oc_review/publish.py +++ b/review/src/oc_review/publish.py @@ -50,7 +50,7 @@ import unicodedata import urllib.parse from collections.abc import Mapping, Sequence -from dataclasses import dataclass +from dataclasses import dataclass, replace from pathlib import Path from typing import Any @@ -115,7 +115,7 @@ def _invisible(ch: str) -> bool: ) -def _strip_controls(text: str) -> str: +def strip_controls(text: str) -> str: text = _LINE_BREAKS.sub("\n", text).replace("\t", " ") out: list[str] = [] marks = 0 @@ -158,15 +158,28 @@ def escape_text(text: str, limit: int, *, multiline: bool = False) -> str: raise TypeError("escape_text takes a str") if not isinstance(limit, int) or limit < 1: raise ValueError("limit must be a positive int") - text = redact_text(_strip_controls(text)) + joined, cut = _clean_text(text, limit, multiline) + escaped = "\\\n".join(_escape_line(line) for line in joined.split("\n")) + return escaped + TRUNCATED if cut else escaped + + +def _clean_text(text: str, limit: int, multiline: bool) -> tuple[str, bool]: + """The redacted, cut text that `escape_text` escapes, and whether it was cut.""" + text = redact_text(strip_controls(text)) lines = [" ".join(line.split()) for line in text.split("\n")] lines = [line for line in lines if line] joined = "\n".join(lines) if multiline else " ".join(lines) cut = len(joined) > limit if cut: joined = joined[:limit].rstrip() - escaped = "\\\n".join(_escape_line(line) for line in joined.split("\n")) - return escaped + TRUNCATED if cut else escaped + return joined, cut + + +def shown_field(kind: str, text: str) -> str: + """The text of a field as the reader sees it, before Markdown escaping: + redacted and cut to the field limit, with a mark when cut.""" + joined, cut = _clean_text(text, LIMITS[kind], kind in MULTILINE) + return joined + "… [truncated]" if cut else joined def escape_field(kind: str, text: str) -> str: @@ -644,32 +657,83 @@ def read_runs(api: GitHubApi, ctx: PublishContext, head_sha: str) -> verdict.Run # Inline review # --------------------------------------------------------------------------- -_FINDING_KEY_RE = re.compile(r"") +_FINDING_KEY_RE = re.compile(r"") UNTRUSTED_NOTE = "Model output from untrusted input. Check it before you act on it." VERIFIED_NOTE = "Verified means only that the quoted text is at that place. It does not mean the claim is true." +@dataclass(frozen=True) +class FindingKeys: + """The two keys of a finding, both from the shown (redacted, cut) text. + + `d14` is the SHA-256 of `file\\0side\\0title\\0quote`. It does not change + when the line moves or the explanation is worded again, so it is the + across-run key. `full` is the SHA-256 of the JSON list of every shown + field. Two findings share it only when they show the same text, so it is + the in-run key (P3-2).""" + + d14: str + full: str + + +def finding_keys(f: verdict.CitedFinding) -> FindingKeys: + file, title, quote = (shown_field(k, getattr(f, k)) for k in ("file", "title", "quote")) + explanation = shown_field("explanation", f.explanation) + d14 = "\0".join((file, f.side, title, quote)) + # `json.dumps` with its defaults gives the same ASCII text for the same values. + full = json.dumps([f.pass_name, f.category, f.severity, f.confidence, file, f.side, f.line, + title, quote, explanation]) + return FindingKeys(hashlib.sha256(d14.encode("utf-8")).hexdigest(), + hashlib.sha256(full.encode("ascii")).hexdigest()) + + def finding_key(f: verdict.CitedFinding) -> str: - """SHA-256 of `file\\0side\\0title\\0quote` (D14).""" - raw = "\0".join((f.file, f.side, f.title, f.quote)) - return hashlib.sha256(raw.encode("utf-8", "surrogatepass")).hexdigest() + """The full key, which names a finding in `InlineOutcome`.""" + return finding_keys(f).full + + +@dataclass(frozen=True) +class PostedComment: + """A review comment of `github-actions[bot]` whose body starts with the + one marker it holds. `lines` holds its `line` and `original_line`.""" + + d14: str + full: str + body: str + path: str + side: str + lines: frozenset[int] -def read_posted_keys(api: GitHubApi, ctx: PublishContext) -> frozenset[str]: - """Dedupe keys in the review comments of `github-actions[bot]` on the PR. - A read error gives no keys, so findings are posted again, never hidden.""" +def _int(value: Any) -> int | None: + return value if isinstance(value, int) and not isinstance(value, bool) else None + + +def read_posted_comments(api: GitHubApi, ctx: PublishContext) -> dict[str, tuple[PostedComment, ...]]: + """The review comments of `github-actions[bot]` on the PR that start with + their only marker, by D14 key. The login proves only that some Actions + workflow in the repo wrote a comment, so `build_review` also checks its + text. A read error gives no comments, so findings are posted again, never + hidden.""" try: items = api.get_pages(f"{ctx.repo_path}/pulls/{ctx.pr_number}/comments") except collect.CollectError: - return frozenset() - keys: set[str] = set() + return {} + out: dict[str, list[PostedComment]] = {} for item in items: if not isinstance(item, Mapping): continue - user, body = item.get("user"), item.get("body") - if isinstance(user, Mapping) and user.get("login") == verdict.BOT_LOGIN and isinstance(body, str): - keys.update(_FINDING_KEY_RE.findall(body)) - return frozenset(keys) + user, body, path, side = (item.get(k) for k in ("user", "body", "path", "side")) + if not (isinstance(user, Mapping) and user.get("login") == verdict.BOT_LOGIN): + continue + if not (isinstance(body, str) and isinstance(path, str) and isinstance(side, str)): + continue + m = _FINDING_KEY_RE.match(body) + if m is None or len(_FINDING_KEY_RE.findall(body)) != 1: + continue + lines = frozenset(n for n in (_int(item.get("line")), _int(item.get("original_line"))) if n is not None) + out.setdefault(m.group(1), []).append(PostedComment(m.group(1), m.group(2), body, path, side, lines)) + return {d14: tuple(cs) for d14, cs in out.items()} def _diff_path(f: verdict.CitedFinding, files: Sequence[verdict.FileSides]) -> str | None: @@ -689,10 +753,10 @@ def _finding_line(f: verdict.CitedFinding) -> str: return f"**{f.severity} {f.category}**, confidence {f.confidence}, pass {f.pass_name}" -def inline_comment_body(f: verdict.CitedFinding, key: str) -> str: +def inline_comment_body(f: verdict.CitedFinding, keys: FindingKeys) -> str: return "\n\n".join( ( - f"", + f"", _finding_line(f), _esc("title", f.title), _esc("explanation", f.explanation), @@ -701,13 +765,39 @@ def inline_comment_body(f: verdict.CitedFinding, key: str) -> str: ) +# The finding line of an inline comment, from the schema enums and our pass names. +_FINDING_LINE_RE = re.compile( + r"\*\*(?:low|medium|high|critical) (?:bug|security|supply-chain|malicious|prompt-injection)\*\*, " + r"confidence (?:low|medium|high), pass (?:" + "|".join(map(re.escape, verdict.PASS_NAMES)) + ")" +) + + +def _shows_same_place(c: PostedComment, f: verdict.CitedFinding, path: str, side: str) -> bool: + """`c` has the layout of `inline_comment_body` (marker, finding line, + title, explanation, note), at `path` and `side`, with the title of `f`. + Its explanation and line can differ: the D14 key covers neither.""" + parts = c.body.split("\n\n") + return ( + c.path == path + and c.side == side + and len(parts) == 5 + and _FINDING_LINE_RE.fullmatch(parts[1]) is not None + and parts[2] == _esc("title", f.title) + and parts[3] != "" + and parts[4] == f"_{UNTRUSTED_NOTE}_" + ) + + @dataclass(frozen=True) class InlineOutcome: - """Keys posted by this run, keys already on the PR, and keys that this - run could not post (they show in full in the summary, D7).""" + """Full keys posted by this run, full keys already on the PR, full keys + whose D14 key is on the PR under other full keys (they show in full in + the summary), and full keys that this run could not post (they show in + full in the summary, D7).""" posted: frozenset[str] = frozenset() earlier: frozenset[str] = frozenset() + changed: frozenset[str] = frozenset() not_posted: frozenset[str] = frozenset() note: str | None = None @@ -715,41 +805,48 @@ class InlineOutcome: def build_review( result: verdict.GateResult, files: Sequence[verdict.FileSides], - posted: frozenset[str], + posted: Mapping[str, Sequence[PostedComment]], head_sha: str, -) -> tuple[dict[str, Any] | None, frozenset[str], frozenset[str]]: - """The review request for verified, inline-eligible findings that are not - on the PR yet. Returns (request or None, new keys, keys already posted).""" +) -> tuple[dict[str, Any] | None, InlineOutcome]: + """The review request for verified, inline-eligible findings that no + earlier comment shows, deduped by full key. Returns (request or None, the + outcome if the request is posted). + + An earlier comment with the D14 key of a finding counts only when it has + the inline layout, the same path, side and title (`_shows_same_place`); + then the finding is not posted again. It is `earlier` only when that + comment is, in full, the body this run would write, at the same line; + else it is `changed` and the summary shows it in full. So a forged marker + can at most move a finding from inline to its full summary block.""" comments = [] new: set[str] = set() earlier: set[str] = set() + changed: set[str] = set() for f in result.findings: if not (f.verified and f.inline): continue - key = finding_key(f) - if key in posted: - earlier.add(key) - continue + keys = finding_keys(f) path = _diff_path(f, files) - if key in new or path is None: + side = "RIGHT" if f.side == "head" else "LEFT" + body = inline_comment_body(f, keys) + same = [c for c in posted.get(keys.d14, ()) if path is not None and _shows_same_place(c, f, path, side)] + if same: + exact = any(c.full == keys.full and c.body == body and f.line in c.lines for c in same) + (earlier if exact else changed).add(keys.full) continue - new.add(key) - comments.append( - { - "path": path, - "line": f.line, - "side": "RIGHT" if f.side == "head" else "LEFT", - "body": inline_comment_body(f, key), - } - ) + if keys.full in new or path is None: + continue + new.add(keys.full) + comments.append({"path": path, "line": f.line, "side": side, "body": body}) + outcome = InlineOutcome(posted=frozenset(new), earlier=frozenset(earlier), changed=frozenset(changed)) if not comments: - return None, frozenset(), frozenset(earlier) + return None, outcome body = ( f"OC Claude review, run {result.run_id}: {len(comments)} inline finding(s). " f"{UNTRUSTED_NOTE} The gate result is in the summary comment." ) request = {"commit_id": head_sha, "event": REVIEW_EVENT, "body": body, "comments": comments} - return request, frozenset(new), frozenset(earlier) + return request, outcome # --------------------------------------------------------------------------- @@ -768,10 +865,11 @@ def build_review( "Reasons, SHAs and override data are never cut, so this run fails closed." ) -# Cut order (D8): unverified findings, low and medium findings, dependency -# delta, signals. Every other block has no rank and is never cut: reasons, -# result, SHAs, override data, and also the blocks D8 does not list (high and -# critical findings, model summaries, hard block and coverage details). +# Cut order (D8): unverified low and medium findings, low and medium findings, +# dependency delta, signals. Every other block has no rank and is never cut: +# reasons, result, SHAs, override data, and also the blocks D8 does not list +# (high and critical findings, verified or not, model summaries, hard block +# and coverage details). CUT_UNVERIFIED = 0 CUT_LOW_MEDIUM = 1 CUT_DEPENDENCY = 2 @@ -857,7 +955,10 @@ def _finding_where(f: verdict.CitedFinding) -> str: def _finding_block(f: verdict.CitedFinding, inline: InlineOutcome) -> _Block: key = finding_key(f) head = f"- {_finding_line(f)}, at {_finding_where(f)}: {_esc('title', f.title)}" - rank = None if f.severity in verdict.BLOCKING_SEVERITIES else CUT_LOW_MEDIUM + # A high or critical finding is never cut, also when it is unverified + # (a cut file path makes it so). + protected = f.severity in verdict.BLOCKING_SEVERITIES + rank = None if protected else CUT_LOW_MEDIUM group = f"findings-{f.pass_name}" if not f.verified: return _Block( @@ -869,14 +970,16 @@ def _finding_block(f: verdict.CitedFinding, inline: InlineOutcome) -> _Block: f"Quote: {_esc('quote', f.quote)}", ) ), - CUT_UNVERIFIED, + None if protected else CUT_UNVERIFIED, "unverified", ) if key in inline.posted: return _Block(f"{head}. See the inline comment.", rank, group) if key in inline.earlier: return _Block(f"{head}. See the inline comment from an earlier run.", rank, group) - if key in inline.not_posted: + if key in inline.changed: + where = "Differs from the inline comment of an earlier run, shown here." + elif key in inline.not_posted: where = "Inline comment not posted, shown here." elif f.file in verdict.META_PATHS: where = "PR title or body, summary only." @@ -1323,9 +1426,9 @@ def post_inline_review( when the fresh snapshot check before the write shows the run stale.""" if not any(f.verified and f.inline for f in result.findings): return InlineOutcome() - request, new, earlier = build_review(result, files, read_posted_keys(api, ctx), head_sha) + request, outcome = build_review(result, files, read_posted_comments(api, ctx), head_sha) if request is None: - return InlineOutcome(earlier=earlier) + return outcome check = check_snapshot(api, ctx, snap) if check.blocks_writes: print(f"publish: {check.describe()} before the inline review; no more writes", file=sys.stderr) @@ -1341,8 +1444,10 @@ def post_inline_review( else: why = "the inline review could not be posted" print(f"publish: {why}: {exc}", file=sys.stderr) - return InlineOutcome(earlier=earlier, not_posted=new, note=f"{why}; its findings are shown in full here.") - return InlineOutcome(posted=new, earlier=earlier) + return replace( + outcome, posted=frozenset(), not_posted=outcome.posted, note=f"{why}; its findings are shown in full here." + ) + return outcome def _artifact_stale(result: verdict.GateResult) -> bool: diff --git a/review/src/oc_review/verdict.py b/review/src/oc_review/verdict.py index 2fad852..863ec61 100644 --- a/review/src/oc_review/verdict.py +++ b/review/src/oc_review/verdict.py @@ -582,6 +582,9 @@ def __init__(self, files: Iterable[FileSides]) -> None: self.by_old_path[f.old_path] = f def lookup(self, path: str, side: str) -> FileSides | None: + if path.endswith("/"): + # No file path ends in "/". A cut path does (analyze `cut_path`). + return None if side == "base": # The old path of a rename names its base blob, also when an added # file reuses that path. diff --git a/review/tests/unit/test_analyze.py b/review/tests/unit/test_analyze.py index 25479ce..2a6aa74 100644 --- a/review/tests/unit/test_analyze.py +++ b/review/tests/unit/test_analyze.py @@ -15,7 +15,7 @@ import httpx2 import pytest -from oc_review import analyze, collect, envelope, render +from oc_review import analyze, collect, envelope, publish, render, verdict from oc_review.envelope import sha256_hex FIX = Path(__file__).parent / "fixtures" / "analyze" @@ -51,6 +51,19 @@ def configured(monkeypatch): monkeypatch.setattr(analyze, name, value) +PLACEHOLDER_IDS = { + "ANTHROPIC_FEDERATION_RULE_ID": "fdrl_000000000000000000000000", + "ANTHROPIC_ORGANIZATION_ID": "00000000-0000-0000-0000-000000000000", + "ANTHROPIC_SERVICE_ACCOUNT_ID": "svac_000000000000000000000000", +} + + +@pytest.fixture +def placeholders(monkeypatch): + for name, value in PLACEHOLDER_IDS.items(): + monkeypatch.setattr(analyze, name, value) + + # --- fake Anthropic client -------------------------------------------------- @@ -420,7 +433,7 @@ def test_stop_reason_other_than_end_turn_fails(tmp_path, configured, stop): ('{"summary": "a", "summary": "b", "findings": []}', analyze.R_BAD_JSON), ('{"summary": "a", "findings": [], "x": NaN}', analyze.R_BAD_JSON), ("", analyze.R_BAD_JSON), - (findings_text(title="t" * 121), f"{analyze.R_SCHEMA}: 'findings'/0/'title': [maxLength]"), + (findings_text(title=["t" * 121]), f"{analyze.R_SCHEMA}: 'findings'/0/'title': [type]"), (findings_text(16), f"{analyze.R_SCHEMA}: 'findings': [maxItems]"), ('{"summary": "a"}', f"{analyze.R_SCHEMA}: : [required]"), ], @@ -447,6 +460,202 @@ def test_more_findings_than_the_pass_limit_stay_ok(tmp_path, configured): assert rec["status"] == "ok" and len(rec["payload"]["findings"]) == 11 +# --- over-long strings (P3-2) ---------------------------------------------------- + + +def schema_limits(schema, path=()): + """Every `maxLength` in a schema, by the keyword path to it.""" + out = {} + if isinstance(schema, dict): + for key, value in schema.items(): + if key == "maxLength": + out[path] = value + else: + out.update(schema_limits(value, (*path, key))) + elif isinstance(schema, list): + for i, value in enumerate(schema): + out.update(schema_limits(value, (*path, i))) + return out + + +LIMITS = {path[-1]: limit for path, limit in schema_limits(analyze.findings_schema()).items()} +# One obfuscated line, longer than the quote limit (eval case obfuscated-test-payload). +LONG_LINE = "const _0x1a = [" + ", ".join(["'\\x65\\x76\\x61\\x6c'"] * 30) + "]; global[_0x1a[0]](payload);" +LONG_FILE = f"const payload = require('./p');\n{LONG_LINE}\nmodule.exports = {{}};\n" +GH_TOKEN = "ghp_" + "A1b2C3d4E5f6G7h8I9j0K1l2M3n4O5p6Q7r8" + + +def long_text(limit, extra=40): + return ("plain words " * (limit // 4))[: limit + extra] + + +def long_doc(**override): + """A critical malicious finding, with fields set by `override`; `summary` sets the summary.""" + doc = json.loads(findings_text(category="malicious", severity="critical", file="test/setup.js", line=2)) + summary = override.pop("summary", None) + if summary is not None: + doc["summary"] = summary + doc["findings"][0].update(override) + return doc + + +def cut(doc): + return analyze.cut_to_limits(doc, analyze.findings_schema()) + + +def cut_value(field, value): + """A plain prefix; a path ends in "/" so that it names no file.""" + if field == "file": + return value[: LIMITS[field] - 1] + "/" + return value[: LIMITS[field]] + + +def test_the_cut_reaches_every_max_length_in_the_schema(): + paths = schema_limits(analyze.findings_schema()) + assert set(LIMITS) == {"summary", "file", "quote", "title", "explanation"} + for path in paths: # only `properties` / `items` steps, the ones the cut walks + steps = list(path) + while steps: + key = steps.pop(0) + if key == "properties": + steps.pop(0) + else: + assert key == "items", path + + +@pytest.mark.parametrize("field", sorted(LIMITS)) +def test_over_long_string_is_cut_and_the_finding_stays(tmp_path, configured, field): + value = long_text(LIMITS[field]) + sent = long_doc(**{field: value}) + client = FakeClient({"supply-chain": Behavior(message=message(json.dumps(sent)))}) + run = run_main(tmp_path, client=client) + assert statuses(run) == {"review": "ok", "security": "ok", "supply-chain": "ok"} + rec = run.records["supply-chain"] + assert rec["meta"]["reason"] is None + want = long_doc(**{field: cut_value(field, value)}) + assert rec["payload"] == want # only that string changed: category, severity, flags stay + envelope.validate_findings(rec["payload"]) # the record holds the full-schema-valid payload + + +def test_every_string_over_its_limit_at_once_is_cut(tmp_path, configured): + sent = long_doc(**{field: long_text(limit) for field, limit in LIMITS.items()}) + client = FakeClient({"security": Behavior(message=message(json.dumps(sent)))}) + rec = run_main(tmp_path, client=client).records["security"] + assert rec["status"] == "ok" + payload = rec["payload"] + [f] = payload["findings"] + assert len(payload["summary"]) == LIMITS["summary"] + assert all(len(f[name]) == LIMITS[name] for name in LIMITS if name != "summary") + original = sent["findings"][0] + assert {k: f[k] for k in original if k not in LIMITS} == {k: original[k] for k in original if k not in LIMITS} + assert (f["category"], f["severity"]) == ("malicious", "critical") + + +@pytest.mark.parametrize("field", sorted(LIMITS)) +def test_string_at_the_limit_is_unchanged(field): + value = "q" * LIMITS[field] + doc = cut(long_doc(**{field: value})) + got = doc["summary"] if field == "summary" else doc["findings"][0][field] + assert got == value + + +def test_cut_keeps_no_part_of_a_secret(): + limit = LIMITS["explanation"] + # A token that straddles the limit, with a zero-width space that hides it + # from redaction until the publisher removes invisible characters. + hidden = GH_TOKEN[:12] + "​" + GH_TOKEN[12:] + text = "e" * (limit - 20) + " " + hidden + " tail" + assert GH_TOKEN[:18] in publish.strip_controls(text[:limit]) # a plain prefix would leak it + got = cut(long_doc(explanation=text))["findings"][0]["explanation"] + assert len(got) <= limit and "[REDACTED" in got + shown = publish.escape_field("explanation", got) + assert not any(GH_TOKEN[i : i + 8] in publish.strip_controls(got) for i in range(len(GH_TOKEN) - 8)) + assert not any(GH_TOKEN[i : i + 8] in re.sub(r"\\(.)", r"\1", shown) for i in range(len(GH_TOKEN) - 8)) + + +def test_cut_without_a_secret_is_a_plain_prefix(): + text = "zero​width " + long_text(LIMITS["title"]) + assert cut(long_doc(title=text))["findings"][0]["title"] == text[: LIMITS["title"]] + + +def test_cut_quote_still_passes_the_citation_check(): + doc = cut(long_doc(quote=LONG_LINE)) + files = (verdict.FileSides("test/setup.js", head_text=LONG_FILE, head_hunks=((1, 3),)),) + c = verdict.cite_finding("supply-chain", doc["findings"][0], files) + assert len(c.quote) == LIMITS["quote"] + assert c.verified and c.inline and c.blocking + + +def test_cut_inside_a_whitespace_run_still_verifies(): + line = "x" * (LIMITS["quote"] - 2) + " \t " + "y" * 50 + doc = cut(long_doc(quote=line)) + files = (verdict.FileSides("test/setup.js", head_text=f"a\n{line}\n", head_hunks=((1, 2),)),) + assert verdict.cite_finding("supply-chain", doc["findings"][0], files).verified + + +def test_cut_quote_that_is_not_on_the_line_stays_unverified_and_blocking(): + doc = cut(long_doc(quote="z" * 400)) + files = (verdict.FileSides("test/setup.js", head_text=LONG_FILE, head_hunks=((1, 3),)),) + c = verdict.cite_finding("supply-chain", doc["findings"][0], files) + assert not c.verified and c.blocking + + +def test_cut_file_path_stays_unverified_and_blocking(): + path = "abcde/" * 75 + "setup.js" + assert path[LIMITS["file"] - 1] != "/" # so the plain cut does not end in "/" + doc = cut(long_doc(file=path, quote="global[_0x1a[0]](payload);")) + f = doc["findings"][0] + assert f["file"] == path[: LIMITS["file"] - 1] + "/" + files = (verdict.FileSides(path, head_text=LONG_FILE, head_hunks=((1, 3),)),) + c = verdict.cite_finding("supply-chain", f, files) + assert not c.verified and not c.inline and c.blocking + + +def test_cut_file_path_never_names_another_changed_file(): + short = "a" * 200 + "/" + "b" * 199 # a changed file whose path is the plain cut of `long` + long = short + "X" + assert len(short) == LIMITS["file"] + files = tuple(verdict.FileSides(p, head_text="danger();\n", head_hunks=((1, 1),)) for p in (short, long)) + doc = cut(long_doc(file=long, line=1, quote="danger();")) + f = doc["findings"][0] + assert len(f["file"]) == LIMITS["file"] and f["file"].endswith("/") + envelope.validate_findings(doc) + c = verdict.cite_finding("supply-chain", f, files) + assert (c.verified, c.inline, c.blocking) == (False, False, True) + assert c.unverified_reason == "file is not in the changed files" + + +def test_a_path_with_a_final_slash_names_no_file(): + files = (verdict.FileSides("dir/", head_text="danger();\n", head_hunks=((1, 1),)),) + f = json.loads(findings_text(file="dir/", line=1, quote="danger();"))["findings"][0] + assert not verdict.cite_finding("review", f, files).verified + + +@pytest.mark.parametrize( + "override, reason", + [ + ({"quote": 5}, "'findings'/0/'quote': [type]"), + ({"quote": ""}, "'findings'/0/'quote': [minLength]"), + ({"file": ""}, "'findings'/0/'file': [minLength]"), + ({"title": 121}, "'findings'/0/'title': [type]"), + ({"summary": ["s" * 2000]}, "'summary': [type]"), + ({"severity": "urgent", "title": "t" * 500}, "'findings'/0/'severity': [enum]"), + ({"extra": "x", "explanation": "e" * 2000}, "'findings'/0: [additionalProperties]"), + ], +) +def test_other_schema_errors_still_fail_the_pass(tmp_path, configured, override, reason): + run = run_main(tmp_path, client=FakeClient({"review": Behavior(message=message(json.dumps(long_doc(**override))))})) + assert run.records["review"]["status"] == "failed" + assert reasons(run)["review"] == f"{analyze.R_SCHEMA}: {reason}" + + +def test_missing_field_still_fails_the_pass(tmp_path, configured): + doc = long_doc(explanation="e" * 2000) + del doc["findings"][0]["title"] + run = run_main(tmp_path, client=FakeClient({"review": Behavior(message=message(json.dumps(doc)))})) + assert reasons(run)["review"] == f"{analyze.R_SCHEMA}: 'findings'/0: [required]" + + def test_final_text_block_after_a_fallback_block_is_parsed(tmp_path, configured): fallback = SimpleNamespace(type="fallback", **{"from": {"model": "claude-opus-5-5"}}, to={"model": "x"}) msg = message( @@ -994,7 +1203,7 @@ def boom(*a, **k): assert run.client.calls == {} and run.oidc.calls == [] -def test_placeholder_federation_refuses_to_call(tmp_path): +def test_placeholder_federation_refuses_to_call(tmp_path, placeholders): run = run_main(tmp_path) assert_three_records(run) assert set(statuses(run).values()) == {"failed"} @@ -1152,7 +1361,7 @@ def test_federation_constants_have_the_id_format(): @pytest.mark.release def test_release_federation_ids_are_not_placeholders(): - """Phase 3 turns this on for every release (`pytest -m release`).""" + """CI runs this on every PR (`pytest -m release`).""" ids = analyze.federation_ids() assert analyze.ids_valid(ids) assert not analyze.ids_are_placeholders(ids), "federation IDs are still placeholders" @@ -1195,7 +1404,7 @@ def factory(token_file, *, deadline, clock, federation): return FakeClient() results = _analyze_direct(tmp_path, factory, federation=other) - assert seen == [other] # module constants are still placeholders: only the parameter can pass + assert seen == [other] # the parameter, not the module constants, names the IDs here assert {r.status for r in results.values()} == {"ok"} diff --git a/review/tests/unit/test_evals.py b/review/tests/unit/test_evals.py index 0232340..cc5388c 100644 --- a/review/tests/unit/test_evals.py +++ b/review/tests/unit/test_evals.py @@ -174,12 +174,30 @@ def test_eval_ids_are_the_real_eval_rule(): assert analyze.ids_valid(ids) and not analyze.ids_are_placeholders(ids) +TEST_RULE_ID = "fdrl_01DrmQ8SP1yTeAi8icEtQbUD" +SIMULATION_RULE_ID = "fdrl_01AfACzupW3U4Bx361ZUbsyR" +TEST_SERVICE_ACCOUNT_ID = "svac_017eR3ohB6rP5MEAsda6U7X6" + + +def test_production_ids_are_the_real_production_rule(): + assert analyze.federation_ids() == analyze.FederationIds( + "fdrl_01N2ZktTiVKqUvvXwywBFfys", "2bc920f9-977b-43b2-870a-f840e4ce809c", "svac_01PiMgfb6qJgNpLZUV8hprCC" + ) + + def test_eval_ids_differ_from_production_ids(): ids, prod = evals.eval_federation(), analyze.federation_ids() + assert analyze.ids_valid(prod) and not analyze.ids_are_placeholders(prod) assert ids.rule_id != prod.rule_id assert ids.service_account_id != prod.service_account_id - # production stays a placeholder until phase 3 - assert analyze.ids_are_placeholders(prod) + # one organization, so the workspace and service account separate the rules + assert ids.organization_id == prod.organization_id + + +def test_production_ids_differ_from_every_test_id(): + prod = analyze.federation_ids() + assert prod.rule_id not in {evals.eval_federation().rule_id, TEST_RULE_ID, SIMULATION_RULE_ID} + assert prod.service_account_id not in {evals.eval_federation().service_account_id, TEST_SERVICE_ACCOUNT_ID} def test_main_uses_the_eval_ids_by_default(tmp_path): diff --git a/review/tests/unit/test_prompts.py b/review/tests/unit/test_prompts.py index fb7db31..bd117ca 100644 --- a/review/tests/unit/test_prompts.py +++ b/review/tests/unit/test_prompts.py @@ -127,6 +127,7 @@ def drop_lines(text: str, phrase: str) -> str: ("Citation rules", "Report a finding even when you are not sure that its citation will check out"), ("Citation rules", "Never count lines yourself"), ("Citation rules", "Do not paraphrase"), + ("Citation rules", "When the line is longer, copy the part of it that shows the problem"), ] diff --git a/review/tests/unit/test_publish.py b/review/tests/unit/test_publish.py index 05f9847..b76949c 100644 --- a/review/tests/unit/test_publish.py +++ b/review/tests/unit/test_publish.py @@ -3,6 +3,8 @@ from __future__ import annotations +import base64 +import hashlib import io import json import re @@ -14,7 +16,7 @@ import pytest -from oc_review import collect, envelope, linediff, publish, verdict +from oc_review import analyze, collect, envelope, linediff, publish, verdict from oc_review.collect import HttpRequest, HttpResponse from oc_review.publish import GitHubApi, PublishContext @@ -175,7 +177,8 @@ def _route(self, req: HttpRequest, path: str, query: dict) -> HttpResponse: return HttpResponse(self.review_status, b"", req.url) body = json.loads(req.body) for c in body["comments"]: - self.review_comments.append({"user": {"login": BOT}, "body": c["body"]}) + self.review_comments.append({"user": {"login": BOT}, "body": c["body"], "path": c["path"], + "side": c["side"], "line": c["line"], "original_line": c["line"]}) return self._json(req, {"id": 1}) if m == "POST" and path == f"{API}/issues/{PR}/comments": self.next_id += 1 @@ -505,7 +508,7 @@ def test_inline_review_only_verified_and_eligible(tmp_path): got = sorted((c["path"], c["line"], c["side"]) for c in rv["comments"]) assert got == [("src/app.py", 10, "LEFT"), ("src/app.py", 10, "RIGHT")] for c in rv["comments"]: - assert re.match(r"\n\n", c["body"]) + assert re.match(r"\n\n", c["body"]) assert publish.UNTRUSTED_NOTE in c["body"] s = r.summary assert "in diff head. See the inline comment." in s @@ -523,25 +526,287 @@ def test_inline_rename_base_side_uses_new_path(tmp_path): assert [(c["path"], c["side"]) for c in rv["comments"]] == [("src/new.py", "LEFT")] +def marker(f: dict) -> str: + keys = publish.finding_keys(verdict.cite_finding("review", f, ())) + return f"" + + +def posted_by(r) -> list[dict]: + """The inline comments of run `r`, as the next run reads them back.""" + return [{"user": {"login": BOT}, "body": c["body"], "path": c["path"], "side": c["side"], "line": c["line"], + "original_line": c["line"]} for rv in r.gh.bodies("POST", "/reviews") for c in rv["comments"]] + + +def earlier(f: dict, login: str = BOT, pass_name: str = "review", **over) -> dict: + """The review comment that an earlier run posted for `f`, as GitHub returns it.""" + cited = verdict.cite_finding(pass_name, f, ()) + doc = {"user": {"login": login}, "body": publish.inline_comment_body(cited, publish.finding_keys(cited)), + "path": f["file"], "side": "RIGHT" if f["side"] == "head" else "LEFT", "line": f["line"], + "original_line": f["line"]} + return {**doc, **over} + + def test_dedupe_by_bot_key_only(tmp_path): f1 = finding(title="first") f2 = finding(title="second", line=11, quote="line 11") - key1 = publish.finding_key(verdict.cite_finding("review", f1, ())) - key2 = publish.finding_key(verdict.cite_finding("review", f2, ())) gh = FakeGitHub(review_comments=[ - {"user": {"login": BOT}, "body": f"\n\nold"}, - {"user": {"login": "attacker"}, "body": f""}, + earlier(f1), + earlier(f2, login="attacker"), ]) r = run(tmp_path, gh, passes=clean_passes(review=[f1, f2, dict(f2)])) (rv,) = r.gh.bodies("POST", "/reviews") - assert [c["line"] for c in rv["comments"]] == [11] # key1 skipped; the spoofed key2 is not + assert [c["line"] for c in rv["comments"]] == [11] # f1 skipped; the spoofed f2 is not assert "first. See the inline comment from an earlier run." in r.summary +SPOOFED = finding("malicious", "critical", title="Steals the token", explanation="Critical evidence not in the forged comment.") + + +def _forged(kind: str) -> dict: + """A review comment of `github-actions[bot]` with the keys of `SPOOFED` + that is not the comment the publisher writes for it now.""" + good = earlier(SPOOFED) + head, finding_line, title, explanation, note = good["body"].split("\n\n") + bodies = { + "marker only": marker(SPOOFED), + "unrelated reply": "Unrelated workflow reply\n" + marker(SPOOFED), + "edited explanation": "\n\n".join((head, finding_line, title, "Nothing to see here\\.", note)), + "other title": "\n\n".join((head, finding_line, "Harmless", explanation, note)), + "bad finding line": "\n\n".join((head, "**fine**", title, explanation, note)), + "no note": "\n\n".join((head, finding_line, title, explanation)), + "second marker": good["body"] + "\n\n" + marker(finding(title="other")), + "marker not first": "Note\n\n" + good["body"], + } + if kind in bodies: + return {**good, "body": bodies[kind]} + return {**good, **{"other path": {"path": "src/other.py"}, "other side": {"side": "LEFT"}, + "other line": {"line": 12, "original_line": 12}}[kind]} + + +# Review r-p3-cut-final Important 2: the bot login proves only that some +# Actions workflow wrote the comment, not that it shows this finding. +@pytest.mark.parametrize("kind", ["marker only", "unrelated reply", "other title", "bad finding line", "no note", + "second marker", "marker not first", "other path", "other side"]) +def test_forged_bot_comment_does_not_stop_the_inline_comment(tmp_path, kind): + r = run(tmp_path, FakeGitHub(review_comments=[_forged(kind)]), passes=clean_passes(review=[SPOOFED])) + (rv,) = r.gh.bodies("POST", "/reviews") + assert [plain(c["body"]).count(SPOOFED["explanation"]) for c in rv["comments"]] == [1] + assert "earlier run" not in r.summary + assert r.code == 1 + + +@pytest.mark.parametrize("kind", ["edited explanation", "other line"]) +def test_bot_comment_with_other_text_or_line_gives_the_full_summary_block(tmp_path, kind): + # The comment has the layout, place and title of the finding, so the D14 + # skip holds; it is not the comment of this finding, so the summary shows + # the finding in full. + r = run(tmp_path, FakeGitHub(review_comments=[_forged(kind)]), passes=clean_passes(review=[SPOOFED])) + assert r.gh.bodies("POST", "/reviews") == [] + s = plain(r.summary) + assert "Differs from the inline comment of an earlier run, shown here." in s + assert SPOOFED["explanation"] in s and "See the inline comment from an earlier run" not in s + assert r.code == 1 + + +def test_exact_bot_comment_is_the_earlier_comment(tmp_path): + r = run(tmp_path, FakeGitHub(review_comments=[earlier(SPOOFED)]), passes=clean_passes(review=[SPOOFED])) + assert r.gh.bodies("POST", "/reviews") == [] + assert "See the inline comment from an earlier run." in r.summary + assert SPOOFED["explanation"] not in plain(r.summary) + # GitHub sets `line` to null on an outdated comment; `original_line` stays. + outdated = earlier(SPOOFED, line=None) + r = run(tmp_path / "outdated", FakeGitHub(review_comments=[outdated]), passes=clean_passes(review=[SPOOFED])) + assert "See the inline comment from an earlier run." in r.summary + + +def test_finding_line_pattern_has_the_schema_enums(): + props = analyze.findings_schema()["properties"]["findings"]["items"]["properties"] + for pass_name in verdict.PASS_NAMES: + for sev in props["severity"]["enum"]: + for cat in props["category"]["enum"]: + for conf in props["confidence"]["enum"]: + f = verdict.cite_finding(pass_name, finding(cat, sev, confidence=conf), ()) + assert publish._FINDING_LINE_RE.fullmatch(publish._finding_line(f)) + assert not publish._FINDING_LINE_RE.fullmatch("**high bug**, confidence high, pass other") + + +def test_one_key_marker_is_not_read(tmp_path): + # A marker with one key (the format before the two-key marker) is not + # read, so its finding is posted again, never hidden. + f1 = finding(title="first") + d14 = publish.finding_keys(verdict.cite_finding("review", f1, ())).d14 + gh = FakeGitHub(review_comments=[{"user": {"login": BOT}, "body": f""}]) + r = run(tmp_path, gh, passes=clean_passes(review=[f1])) + (rv,) = r.gh.bodies("POST", "/reviews") + assert len(rv["comments"]) == 1 + + +def test_finding_keys_are_fixed(): + keys = publish.finding_keys(verdict.cite_finding("review", finding(), ())) + # D14: the format before P3-2, so comments posted then still match. + assert keys.d14 == hashlib.sha256(b"src/app.py\0head\0t\0shell=True").hexdigest() + raw = b'["review", "bug", "low", "high", "src/app.py", "head", 10, "t", "shell=True", "e"]' + assert keys.full == hashlib.sha256(raw).hexdigest() + # Fixed values: a change to the key format re-posts every earlier comment. + assert keys.d14 == "397e93ee9b12a9cb82eb2bcfb2fba39ce085c18618932418e458293ad4e6c29e" + assert keys.full == "cd2365850b5f3a1b4a9cc356d224471b66f23a5353b3110a0ed072271c55ad52" + assert publish.finding_key(verdict.cite_finding("review", finding(), ())) == keys.full + + +def test_marker_has_a_fixed_size_and_comment_limits_do_not_change(tmp_path): + long = finding(explanation="word " * 240) # at the explanation limit + r = run(tmp_path, passes=clean_passes(review=[long])) + (rv,) = r.gh.bodies("POST", "/reviews") + (c,) = rv["comments"] + head, rest = c["body"].split("\n\n", 1) + assert len(head) == len("") + 64 + 1 + 64 + assert rest == publish.inline_comment_body( + verdict.cite_finding("review", long, ()), publish.FindingKeys("0" * 64, "0" * 64)).split("\n\n", 1)[1] + assert "word " * 239 + "word" in plain(rest) and publish.TRUNCATED not in rest + assert publish.LIMITS["explanation"] == 1200 and publish.SUMMARY_LIMIT == 64_000 + + +@pytest.mark.parametrize("field", ["title", "quote", "explanation", "file"]) +def test_keys_are_of_the_redacted_text(field): + # Review rr-p3-cut-fix2 Minor 1: a published key of the raw text is a + # guess check for the redacted part. + other = "ghp_" + "Z" * 36 + def keys(token): + return publish.finding_keys(verdict.cite_finding("review", finding(**{field: f"Uses {token} here"}), ())) + assert publish.shown_field(field, f"Uses {GH_TOKEN} here") == publish.shown_field(field, f"Uses {other} here") + assert keys(GH_TOKEN) == keys(other) + assert keys(GH_TOKEN) != keys("nothing") + + +def test_rerun_with_the_same_findings_posts_nothing_new(tmp_path): + passes = clean_passes(review=[finding(title="bug", explanation="Same words.")]) + first = run(tmp_path / "1", passes=passes) + assert len(posted_by(first)) == 1 + second = run(tmp_path / "2", FakeGitHub(review_comments=posted_by(first)), passes=passes) + assert second.gh.bodies("POST", "/reviews") == [] + assert "bug. See the inline comment from an earlier run." in second.summary + assert "Same words." not in second.summary + + +def moved_bundle(): + """The head of `bundle_doc` with one line added at the top: the finding moves from line 10 to 11.""" + head = "added line\n" + HEAD_TEXT + return bundle_doc(files=[{"path": "src/app.py", "old_path": None, "status": "modified", + "base_content": BASE_TEXT, "head_content": head}]) + + +def test_rerun_with_new_words_and_a_moved_line_shows_the_finding_in_the_summary(tmp_path): + first = run(tmp_path / "1", passes=clean_passes(review=[finding(title="bug", explanation="Old words.")])) + assert len(posted_by(first)) == 1 + moved = finding(title="bug", line=11, explanation="New words: a different claim.") + second = run(tmp_path / "2", FakeGitHub(review_comments=posted_by(first)), bundle=moved_bundle(), + passes=clean_passes(review=[moved])) + assert second.gh.bodies("POST", "/reviews") == [] # no second inline comment + s = plain(second.summary) + assert "Differs from the inline comment of an earlier run, shown here." in s + assert "New words: a different claim." in s and "**low bug**" in s + # The next run with the same words matches the comment of the first run + # by D14 key only, so it still shows the new words in full. + third = run(tmp_path / "3", FakeGitHub(review_comments=posted_by(first)), bundle=moved_bundle(), + passes=clean_passes(review=[moved])) + assert "New words: a different claim." in plain(third.summary) + + +def test_moved_line_alone_shows_the_finding_in_the_summary(tmp_path): + # The line is a shown field, so the full key differs, and the summary + # shows the new place. + first = run(tmp_path / "1", passes=clean_passes(review=[finding(title="bug", explanation="Same words.")])) + 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 "Differs from the inline comment of an earlier run" in plain(second.summary) + + +def test_same_d14_key_in_one_run_after_an_earlier_comment(tmp_path): + old = finding("security", "high", title="same", explanation="Old words.") + new = finding("malicious", "critical", title="same", explanation="Critical new words.") + gh = FakeGitHub(review_comments=[earlier(old)]) + r = run(tmp_path, gh, passes=clean_passes(review=[old, new])) + assert r.gh.bodies("POST", "/reviews") == [] + s = plain(r.summary) + assert "same. See the inline comment from an earlier run." in s + assert "Differs from the inline comment of an earlier run, shown here." in s + assert "Critical new words." in s and "Old words." not in s + + +def test_findings_below_the_limits_that_differ_in_explanation_are_both_posted(tmp_path): + first = finding("security", "high", title="same", explanation="First distinct explanation.") + second = finding("malicious", "critical", title="same", explanation="Second distinct explanation.") + r = run(tmp_path, passes=clean_passes(review=[first, second])) + (rv,) = r.gh.bodies("POST", "/reviews") + bodies = [plain(c["body"]) for c in rv["comments"]] + assert len(bodies) == 2 + assert any("First distinct explanation." in b for b in bodies) + assert any("Second distinct explanation." in b for b in bodies) + + +def jwt(sub: str, length: int) -> str: + """A JWT that the redaction finds, padded to `length` with its signature.""" + def b64(doc: dict[str, str]) -> str: + return base64.urlsafe_b64encode(json.dumps(doc).encode()).decode().rstrip("=") + token = b64({"alg": "HS256"}) + "." + b64({"sub": sub}) + "." + return token + "s" * (length - len(token)) + + +def test_titles_that_redaction_makes_equal_keep_both_explanations(tmp_path): + # Review rr-p3-cut-fix1 Important A: the cut is taken from the redacted + # text, so it can be far below the limit and still make two titles equal. + titles = ["Token " + jwt("user-a", 155), "Token " + jwt("user-b", 155)] + assert [len(t) for t in titles] == [161, 161] + first = finding("security", "high", title=titles[0], explanation="First distinct explanation.") + second = finding("malicious", "critical", title=titles[1], explanation="Second distinct explanation: secret theft.") + doc = analyze.cut_to_limits({"summary": "s", "findings": [first, second]}, analyze.findings_schema()) + assert [f["title"] for f in doc["findings"]] == ["Token [REDACTED:jwt]"] * 2 + r = run(tmp_path, passes=clean_passes(review=doc["findings"])) + (rv,) = r.gh.bodies("POST", "/reviews") + bodies = [plain(c["body"]) for c in rv["comments"]] + assert len(bodies) == 2 + assert any("First distinct explanation." in b for b in bodies) + assert any("Second distinct explanation: secret theft." in b for b in bodies) + + +CUT_PREFIX = "Same long title " + "x" * 104 # exactly the title limit + + +def cut_pair(): + """Two findings whose titles differ only past the title limit, after the analyze cut.""" + first = finding("security", "high", title=CUT_PREFIX + " SQL injection", explanation="First distinct explanation.") + second = finding("malicious", "critical", title=CUT_PREFIX + " secret theft", + explanation="Second distinct explanation: secret theft.") + doc = analyze.cut_to_limits({"summary": "s", "findings": [first, second]}, analyze.findings_schema()) + assert doc["findings"][0]["title"] == doc["findings"][1]["title"] == CUT_PREFIX + return doc["findings"] + + +def test_findings_that_differ_past_the_cut_are_both_posted(tmp_path): + r = run(tmp_path, passes=clean_passes(review=cut_pair())) + (rv,) = r.gh.bodies("POST", "/reviews") + bodies = [plain(c["body"]) for c in rv["comments"]] + assert len(bodies) == 2 + assert any("First distinct explanation." in b for b in bodies) + assert any("Second distinct explanation: secret theft." in b for b in bodies) + + +def test_finding_that_differs_past_the_cut_from_an_earlier_one_is_shown_in_full(tmp_path): + first, second = cut_pair() + gh = FakeGitHub(review_comments=[earlier(first)]) + r = run(tmp_path, gh, passes=clean_passes(review=[first, second])) + assert r.gh.bodies("POST", "/reviews") == [] + s = plain(r.summary) + assert "See the inline comment from an earlier run." in s + assert "Differs from the inline comment of an earlier run, shown here." in s + assert "Second distinct explanation: secret theft." in s + + def test_no_review_when_nothing_new(tmp_path): f1 = finding(title="first") - key = publish.finding_key(verdict.cite_finding("review", f1, ())) - gh = FakeGitHub(review_comments=[{"user": {"login": BOT}, "body": f""}]) + gh = FakeGitHub(review_comments=[earlier(f1)]) r = run(tmp_path, gh, passes=clean_passes(review=[f1])) assert r.gh.bodies("POST", "/reviews") == [] assert r.code == 0 @@ -549,9 +814,8 @@ def test_no_review_when_nothing_new(tmp_path): def test_review_comments_read_with_paging(tmp_path): findings = [finding(title=f"t{i}", line=10) for i in range(3)] - keys = [publish.finding_key(verdict.cite_finding("review", f, ())) for f in findings] filler = [{"user": {"login": BOT}, "body": "x"} for _ in range(150)] - gh = FakeGitHub(review_comments=filler + [{"user": {"login": BOT}, "body": f""} for k in keys]) + gh = FakeGitHub(review_comments=filler + [earlier(f) for f in findings]) r = run(tmp_path, gh, passes=clean_passes(review=findings)) assert r.gh.bodies("POST", "/reviews") == [] assert sum(p.endswith(f"/pulls/{PR}/comments") for p in r.gh.paths()) == 2 @@ -629,6 +893,45 @@ def test_untrusted_text_is_escaped_in_summary(tmp_path): assert not re.search(r"(?more" + + def end(kind): + return "/" if kind == "file" else "" + + payload = {"summary": over("summary"), "findings": [finding( + "malicious", "high", file=over("file"), quote=over("quote"), title=over("title"), + explanation=over("explanation"))]} + analyze.cut_to_limits(payload, analyze.findings_schema()) + envelope.validate_findings(payload) + [f] = payload["findings"] + cut = {"summary": payload["summary"], **{k: f[k] for k in ("file", "quote", "title", "explanation")}} + assert all(v.endswith(tail + end(k)) and len(v) == publish.LIMITS[k] for k, v in cut.items()) + + passes = clean_passes() + passes["review"] = pass_doc("review", findings=payload["findings"], summary=payload["summary"]) + r = run(tmp_path, passes=passes) + 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) + for kind, value in cut.items(): + 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 + body = s.replace(verdict.STICKY_MARKER, "") + assert not re.search(r"(?" not in body and "**bold**" not in body + + def test_summary_under_limit_and_cut_in_order(tmp_path): long = "word " * 200 def many(cat, sev, n, quote="shell=True", line=10): @@ -726,6 +1029,67 @@ def test_reasons_kept_when_cuttable_data_is_cut(tmp_path, capsys): assert s.rstrip().endswith("The result that counts is the required workflow in the merge box.") +CUT_PATH = "abcde/" * 75 + "setup.js" # over the file limit: analyze cuts it to a final "/" + + +def cut_path_passes(label: str, counts=(("review", 10), ("security", 10), ("supply-chain", 15)), pad=1100): + """Critical findings with a cut path (so unverified), at the per-pass limits.""" + out = {} + for p, n in counts: + doc = {"summary": "ok", "findings": [ + finding("malicious", "critical", file=CUT_PATH, title=f"{p} issue {i}", + explanation=f"{label} claim {p} {i} " + "!" * pad) for i in range(n)]} + analyze.cut_to_limits(doc, analyze.findings_schema()) + assert all(f["file"].endswith("/") for f in doc["findings"]) + out[p] = pass_doc(p, **doc) + return out + + +def claims(passes: dict) -> list[str]: + return [f["explanation"].split("!")[0].strip() for p in passes.values() for f in p["payload"]["findings"]] + + +def test_critical_findings_with_a_cut_path_are_never_cut_from_the_summary(tmp_path, capsys): + # Review r-p3-cut-final Important 1, probe 3: every critical explanation + # is in the summary, or the run fails closed with the size notice. + passes = cut_path_passes("CRITICAL") + r = run(tmp_path, passes=passes) + assert r.code == 1 and r.gh.bodies("POST", "/reviews") == [] + s = plain(r.summary) + assert publish.OVERFLOW_NOTE in s and "### Summary too large" in s + assert not any(c in s for c in claims(passes)) # no partial list + assert r.record["result"] == "fail" and r.record["publish_error"] + rec = full_record_from_log(capsys.readouterr().out) + assert len([f for f in rec["findings"] if f["severity"] == "critical"]) == 35 + + +def test_unverified_high_and_critical_findings_stay_when_others_are_cut(tmp_path): + passes = cut_path_passes("CRITICAL", counts=(("supply-chain", 3),)) + passes["supply-chain"]["payload"]["findings"].append( + finding("security", "high", title="unverified high", quote="not there", explanation="HIGH claim kept")) + passes["review"] = pass_doc("review", findings=[ + finding(title=f"low {i}", quote="not there", explanation=f"LOW claim {i} " + "!" * 1100) for i in range(10)]) + passes["security"] = pass_doc("security", findings=[ + finding(severity="medium", title=f"medium {i}", line=25, quote="line 25", + explanation=f"MEDIUM claim {i} " + "!" * 1100) for i in range(10)]) + r = run(tmp_path, bundle=bundle_doc(delta=dep_delta(250)), passes=passes) + s = plain(r.summary) + assert r.code == 1 and publish.OVERFLOW_NOTE not in s + assert "pass-records" not in s + assert publish.TRUNCATED_NOTE in s and "LOW claim 9" not in s + for c in claims({"supply-chain": passes["supply-chain"]}): + assert c in s + assert "HIGH claim kept" in s + + +def test_unverified_finding_ranks(): + def rank(sev): + f = verdict.cite_finding("review", finding(severity=sev, quote="not there"), ()) + assert not f.verified + return publish._finding_block(f, publish.InlineOutcome()).rank + assert [rank(s) for s in ("low", "medium", "high", "critical")] == [publish.CUT_UNVERIFIED] * 2 + [None] * 2 + + def test_reasons_that_do_not_fit_fail_closed(tmp_path, capsys): r = run(tmp_path, bundle=bundle_doc(hard_blocks=hard_blocks(400, pad=200))) s = r.summary diff --git a/review/tests/unit/test_workflows.py b/review/tests/unit/test_workflows.py index bf1e355..609f477 100644 --- a/review/tests/unit/test_workflows.py +++ b/review/tests/unit/test_workflows.py @@ -756,6 +756,22 @@ def test_harden_runner_egress_lists_are_the_v10_lists(): assert "disable-sudo-and-containers" not in inputs(step), f"{path.name} {name}" +def test_test_workflow_runs_the_release_tests_in_the_unit_tests_job(): + """Spec "Testing": `pytest -m release` runs on every PR, in the required + `unit-tests` job, so the required check name does not change.""" + wf = load_workflow(WORKFLOWS / TEST) + job = jobs(wf.doc)["unit-tests"] + runs = [step.get("run", "") for step in steps(job)] + assert "uv run --locked pytest tests/unit" in runs + release = [r for r in runs if "-m release" in r] + assert release == ["uv run --locked pytest tests/unit -m release"] + assert runs.index(release[0]) > runs.index("uv run --locked pytest tests/unit") + step = next(s for s in steps(job) if s.get("run") == release[0]) + assert step.get("working-directory") == "review" + assert "if" not in step and "continue-on-error" not in step + assert "if" not in job and "continue-on-error" not in job + + # --------------------------------------------------------------------------- # Tests: fixtures prove that each check fails on its violation # ---------------------------------------------------------------------------