diff --git a/.github/workflows/eval.yml b/.github/workflows/eval.yml index de48533..9258557 100644 --- a/.github/workflows/eval.yml +++ b/.github/workflows/eval.yml @@ -43,6 +43,7 @@ jobs: files.pythonhosted.org:443 api.anthropic.com:443 *.actions.githubusercontent.com:443 + *.blob.core.windows.net:443 - name: Check out the dispatched branch uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1, released 2026-07-20 with: @@ -64,3 +65,18 @@ jobs: uv run --no-sync --project review python -m oc_review.evals --fixtures-dir review/tests/fixtures --out-dir "$RUNNER_TEMP/eval" + # results.jsonl and each case's summary.md only. The per-case data/ dirs + # (bundle and pass records) stay out. No file holds a credential: the + # Anthropic token lives in memory only (spec "Fixture eval credential"). + - name: Upload the eval results + if: always() + uses: actions/upload-artifact@cf430e030ddbb5b0abf93d22962f4752f3646cd9 # v7.0.2, released 2026-10-07 + with: + name: eval-results-a${{ github.run_attempt }} + path: | + ${{ runner.temp }}/eval/results.jsonl + ${{ runner.temp }}/eval/*/summary.md + # warn, not error: if an earlier step failed before any result exists, + # a second red step would bury the real cause. + if-no-files-found: warn + retention-days: 14 diff --git a/docs/build-plan.md b/docs/build-plan.md index 399c62b..53c6ce1 100644 --- a/docs/build-plan.md +++ b/docs/build-plan.md @@ -185,19 +185,21 @@ Exit criteria: ## Phase 4: pilot -- [ ] Owner: before a repo joins the pilot, give it its own approval protection on the default branch: a repo ruleset or branch protection with at least 1 approval, code owner review and dismiss stale approvals on push. `back-end` has none today. -- [ ] Owner: before a repo joins the pilot, check it for people or bots that push to the default branch directly (V25). +- [x] Owner: record the pilot scope. The plan named only `back-end`; the owner added `front-end` (135777271), `operationcode-pybot` (146210805, default branch `master`) and `operationcode_infra` (107068150), with `back-end` (182584277). Four repos (spec P4-1). +- [x] Owner: before a repo joins the pilot, give it its own approval protection on the default branch: a repo ruleset or branch protection with at least 1 approval, code owner review and dismiss stale approvals on push. Done 2026-10-28: "Pilot approval protection" on `back-end` (26057791), `operationcode-pybot` (26057793) and `operationcode_infra` (26057794), with the Maintain, Admin and org admin bypass because pybot and infra have one active committer; `front-end`: the existing "Protect main" (25634150), only `require_code_owner_review` changed to true (spec P4-3). +- [x] Owner: before a repo joins the pilot, check it for people or bots that push to the default branch directly (V25). Done: none in 12 months on `back-end`, `operationcode-pybot` and `operationcode_infra`; `front-end` had a force push on 2026-10-28 that rewrote three merged PRs (cleanup of a malicious dependency payload), not new unreviewed code (spec P4-4). - [ ] Owner: accept that during the pilot a maintainer or repo admin can merge past any failed run, also on their own PR (spec "Pilot and enforcement"). -- [ ] Owner: confirm that the federation rule named in the release code has `job_workflow_ref` at the release SHA. -- [ ] Owner: create the pilot ruleset. Only the rule "Require workflows to pass before merging" with the central workflow `repository_id`, path and release SHA. Targets: the pilot repos by name (first `back-end`), default branch. Bypass in `pull_request` mode: the Maintain and Admin repository roles, and org admins. -- [ ] Owner: read the pilot ruleset back through the [org rules API](https://docs.github.com/en/rest/orgs/rules#get-an-organization-repository-ruleset). Check `repository_id`, path, `sha`, no `ref`, and the role `actor_id` values from V25. Then enable it. -- [ ] Owner: require 2FA for all org members. -- [ ] Owner: CODEOWNERS in each pilot repo for `.github/`, manifests, lockfiles, `pyproject.toml`, `Dockerfile`. -- [ ] Start a run on each open PR in the pilot repos (push, or close and reopen). +- [x] Owner: confirm that the federation rule named in the release code has `job_workflow_ref` at the release SHA. Read back by the owner at M4 `9e2795b` (2026-10-29, spec P4-5). +- [x] Owner: every pilot repo sends the immutable OIDC subject (spec "Immutable subject", P4-6). Found 2026-10-29: repos made before the GitHub default sent `repo:OperationCode/:...`, the rule rejected them (`match_subject_prefix`), and the first pilot runs failed closed as designed. Fix per repo: `PUT repos/OperationCode//actions/oidc/customization/sub` with `use_default` true and `use_immutable_subject` true. Before it, check the other `id-token` users in the repo and their relying-party trust: `back-end` and `operationcode-pybot` use a cloud OIDC trust policy that gained the immutable subjects (owner; old values kept). After it, all four test PRs passed (all four jobs `success`). +- [x] Owner: create the pilot ruleset. Only the rule "Require workflows to pass before merging" with the central workflow `repository_id`, path and release SHA. Targets: the pilot repos by name (`back-end`, `front-end`, `operationcode-pybot`, `operationcode_infra`), default branch. Bypass in `pull_request` mode: the Maintain and Admin repository roles, and org admins. Created disabled as `claude-review-pilot` (26132750): four repo ids, `~DEFAULT_BRANCH`, `repository_id` 1432342572, M4 `9e2795b`, bypass `RepositoryRole` 2 and 5 plus `OrganizationAdmin` (spec P4-2). +- [x] Owner: read the pilot ruleset back through the [org rules API](https://docs.github.com/en/rest/orgs/rules#get-an-organization-repository-ruleset). Check `repository_id`, path, `sha`, no `ref`, and the role `actor_id` values from V25. Then enable it. Read back (`do_not_enforce_on_create` false, no `ref`), then enabled 2026-10-29 (created and enabled the same morning). Rollback: `PUT` with `enforcement` set to `disabled`. +- [x] Owner: require 2FA for all org members. Done in the web UI; all 7 members already had 2FA (spec P4-5). +- [x] Owner: CODEOWNERS in each pilot repo for `.github/`, manifests, lockfiles, `pyproject.toml`, `Dockerfile`. Added on each default branch (`back-end`: @irvingpop @kylemh; `front-end`: @kylemh @irvingpop; `operationcode-pybot` and `operationcode_infra`: @irvingpop; spec P4-3). +- [ ] Right after enabling the pilot ruleset, list the open PRs in the pilot repos again (do not reuse a list from earlier recon) and start a run on each (close and reopen, or a new push). A required workflow starts only on a new PR event, so a PR opened between the recon and the enable gets no review run: front-end PR 2027 (opened 2026-10-29T08:06Z) was missed this way. - [ ] 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. -- [ ] Keep the fixture eval results as an artifact: `eval.yml` uploads none today, so a release eval's per-case results exist only in the job log (phase 2 run 40445885087 and phase 3 run 40528717338 were read from logs). Ships as in "Each release". -- [ ] 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". +- [x] Keep the fixture eval results as an artifact: `eval.yml` uploads `results.jsonl` and each case's `summary.md` as `eval-results-a` (14 days, `if: always()`). Before this, a release eval's per-case results existed only in the job log (phase 2 run 40445885087 and phase 3 run 40528717338 were read from logs). Ships as in "Each release". +- [ ] 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". Plan (planned, not built): [docs/continuation-comments-plan.md](continuation-comments-plan.md). Depends on: phase 3. @@ -216,6 +218,7 @@ Exit criteria: - [ ] targets: never `.github`, and none of the repos still in the pilot ruleset - [ ] Owner: read the org ruleset back and check the workflow entry (`repository_id`, path, `sha`, no `ref`). - [ ] Build the supported repo inventory (spec "Supported repos") as a list in this repo. +- [ ] Owner: every repo to be added to the org ruleset first sends the immutable OIDC subject (spec "Immutable subject"). Check `GET repos/OperationCode//actions/oidc/customization/sub`, check the other `id-token` users in the repo and their relying-party trust, set it per repo, and run a test PR (all jobs `success`) before the repo is a target. - [ ] 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. @@ -232,7 +235,7 @@ Exit criteria: ## Phase 6: org-wide enforcement -- [ ] Owner: enable the org ruleset with the supported repos that are not in the pilot. Add them in small groups. +- [ ] Owner: enable the org ruleset with the supported repos that are not in the pilot. Add them in small groups. Each repo in a group already sends the immutable OIDC subject (phase 5); without it the run fails closed. - [ ] Owner: move each pilot repo: remove it from the pilot ruleset targets, then add it to the org ruleset targets at once. Never let both rulesets require the workflow on one repo. - [ ] After each group or move, start a run on each open PR, and check that each shows the required workflow passing or failing on its own result. - [ ] Owner: delete the pilot ruleset when no repo is left in it. diff --git a/docs/claude-review-design.md b/docs/claude-review-design.md index 763e672..ee3cda7 100644 --- a/docs/claude-review-design.md +++ b/docs/claude-review-design.md @@ -2,7 +2,7 @@ Status: draft, revised after several adversarial design reviews and platform test rounds (see "Decision log"). Code that does not depend on the gate may be built in parallel with the "build" items in "Verify before building"; it merges to main only after they pass. Build plan: [docs/build-plan.md](build-plan.md). Owner: Irving Popovetsky -Last updated: 2026-10-26 +Last updated: 2026-10-29 ## Summary @@ -194,10 +194,10 @@ Only the gate job writes to GitHub, and it writes only comments. It has no Anthr - analyze: `api.github.com`, `github.com`, `raw.githubusercontent.com`, `release-assets.githubusercontent.com`, `files.pythonhosted.org`, `api.anthropic.com`, `*.actions.githubusercontent.com`, `*.blob.core.windows.net` - dependency-review: `api.github.com` only, because `show-openssf-scorecard` is `false` - oc-claude-review: `api.github.com`, `github.com`, `raw.githubusercontent.com`, `release-assets.githubusercontent.com`, `files.pythonhosted.org`, `results-receiver.actions.githubusercontent.com`, `*.blob.core.windows.net` - 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. + Why each host: `github.com` (checkout, setup-uv), `raw.githubusercontent.com` and `release-assets.githubusercontent.com` (uv version manifest and uv download), `files.pythonhosted.org` (`uv sync` from `uv.lock`), `api.github.com` (REST reads and writes), `pypi.org` and `registry.npmjs.org` (collect's registry lookups), `results-receiver.actions.githubusercontent.com` and `*.blob.core.windows.net` (artifact upload and download), `api.anthropic.com` and the Actions OIDC token host (analyze). The blob host and the OIDC host (`run-actions-N-azure-REGION.actions.githubusercontent.com`) change per run, so they need wildcards; the analyze wildcard also covers `results-receiver`. The `test.yml` and `eval.yml` lists are not measured: they hold the hosts that the same checkout, setup-uv and `uv sync` steps (and, for `eval.yml`, the token exchange and the API) reached in V10, and `test.yml` adds `results-receiver.actions.githubusercontent.com`, which the runner itself reached. `eval.yml` also holds `*.blob.core.windows.net:443`, added and not measured in V10, for the eval results artifact upload. Audit mode records egress and blocks nothing. Block mode is `egress-policy: block` plus `disable-telemetry: true`; the action accepts `disable-telemetry: true` only in block mode, so in audit mode harden-runner sends telemetry to StepSecurity. Block mode is revisited before org-wide enforcement (K-14, build plan phase 5). The allowlist is defense in depth. It does not prove that no data can leave, because GitHub itself is on the list. -- **Other workflows:** `test.yml` runs the unit tests of a PR to this repo (`pull_request`, no path filter, and `push` to `main`) with `contents: read` only, no `id-token`, no environment and no secrets. Its job `unit-tests` is the required check name, and it has two test steps: the default unit tests, then `pytest tests/unit -m release` (the `release`-marked tests, see "Testing"), so the check name does not change. It runs the PR's own code with that read-only token, so it is not the review workflow: the no-execute rule (and `CLAUDE.md`'s wording of it) covers the review workflow, which never runs PR content. `eval.yml` is `workflow_dispatch` only (never `workflow_call`), has `permissions: {}` at the top, and one job with the `eval` environment (see "Fixture eval credential"). +- **Other workflows:** `test.yml` runs the unit tests of a PR to this repo (`pull_request`, no path filter, and `push` to `main`) with `contents: read` only, no `id-token`, no environment and no secrets. Its job `unit-tests` is the required check name, and it has two test steps: the default unit tests, then `pytest tests/unit -m release` (the `release`-marked tests, see "Testing"), so the check name does not change. It runs the PR's own code with that read-only token, so it is not the review workflow: the no-execute rule (and `CLAUDE.md`'s wording of it) covers the review workflow, which never runs PR content. `eval.yml` is `workflow_dispatch` only (never `workflow_call`), has `permissions: {}` at the top, and one job with the `eval` environment (see "Fixture eval credential"). Its last step, with `if: always()`, uploads `results.jsonl` and each case's `summary.md` as the artifact `eval-results-a` (14 days; the per-case `data/` records stay out, and no uploaded file holds a credential). - **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. @@ -239,7 +239,7 @@ Jobs pass data only through artifacts from the same run and the same run attempt - Dependency delta (see "Dependency delta"). - Prefilter results: `signals[]` and `hard_blocks[]`. - Coverage record: files in the comparison, files collected, whether both trees were complete, per-file coverage (`full`, `binary`, `unavailable`), and `complete`. The gate treats a record whose `complete` is not `true` as a hard block, even when the prefilter list has no "incomplete coverage" entry. -- Size: estimated token count (characters of the final rendered bundle, its size field included, divided by 4). The size gate is 150k input tokens. Above that, skip the LLM passes. A bundle over the 16 MiB envelope cap is skipped the same way. The gate fails with the reason "too large for automatic review". +- Size: estimated token count (characters of the final rendered bundle, its size field included, divided by 4). The size gate is 150k input tokens. The estimate is of exactly what Claude sees, so no file type counts at less than its rendered size; a long file changed near both ends, such as a lockfile, stays small because its patch is anchored (K-17, "Coverage limits"). Above that, skip the LLM passes. A bundle over the 16 MiB envelope cap is skipped the same way. The gate fails with the reason "too large for automatic review". - The size gate skips only the Claude calls. All code checks still run. A `skipped-size` bundle keeps the prefilter results, the coverage record and the dependency delta in its payload. It leaves out only the content for Claude. The gate reads the hard blocks from this payload and from the separate prefilter result, and uses both. If the prefilter result is missing, incomplete or for another snapshot, the gate fails, not waivable (K-7). ### Consistent snapshot @@ -271,18 +271,19 @@ The review covers one immutable comparison pair: the merge base commit and the h - A truncated tree, an `unavailable` text file or a decode error is the hard block "incomplete coverage". An override cannot waive it, because the code did not see the content. - If GitHub rejects an inline comment position (its PR view can differ from the reviewed comparison), that finding goes to the summary. This never changes the gate. - Every API response and every parser has a byte cap that applies before parsing and before the token gate. -- **Line diff limits** (K-13): collect (patches) and the publisher (inline hunks) use the same bounded line diff (`linediff.py`). It trims the common prefix and suffix, then runs a difflib line diff of the middle only within three limits: +- **Line diff limits** (K-13): collect (patches) and the publisher (inline hunks) use the same bounded line diff (`linediff.py`); only collect adds the anchored step below (K-17). It trims the common prefix and suffix, then runs a difflib line diff of the middle only within three limits: - `DIFF_BUDGET = 25,000,000`: the product of the two middle lengths. - `FILE_WORK_BUDGET = 5,000,000`: counted steps for the diff of one file. - `RUN_WORK_BUDGET = 100,000,000`: counted steps for all files of one collect run, and separately for all files of one publisher run. Every file takes its steps from this one budget. A step is one line of the index or of a searched range, or one pass of difflib's inner loop, so the count is at least the work that difflib does. The count depends only on the input, never on the clock, and it is checked before each search runs. On low-entropy input (few distinct lines, each repeated many times) difflib's work grows about with the cube of the line count, so the product limit alone does not bound it. Measured on an Apple M-series core: about 1 s per file and 20 s per run at the limits. Long lines raise the cost of one step (hashing and comparing a line), so the times are approximate. A GitHub runner three times slower uses about 60 s of the 10-minute job timeout. Past a limit, the middle is one hunk that removes every base line and adds every head line (a larger patch than git's, never a wrong one), and in the publisher the file has no hunks (see "Inline eligibility"). A file past a step limit is charged all the steps it was allowed. The job log names the file and the limit. A middle with only added or only removed lines needs no diff, so no limit applies to it. A coarse diff never clears a block and never lowers risk. +- **Anchored patch past the size budget** (K-17): in collect only, a middle past `DIFF_BUDGET` is first split at anchors: the lines that occur exactly once in the base middle and once in the head middle, in the longest run that keeps their order on both sides (the "patience" anchors). Each gap between two anchors gets the bounded diff above, with the same three limits; a gap past a limit is one hunk that removes its base lines and adds its head lines. Anchoring costs one step per line of both middles plus one per line of the shorter middle, from the same file and run work budgets; when the file cannot pay that, the middle is one hunk as before and nothing is charged. A gap past a work limit charges the file its whole allowance. The result is a correct edit script (every line outside an equal run is shown as removed or added), so Claude still sees every changed line. It reads only the line text, never the path, so a file named like a lockfile, in any directory, gets the same patch as any other file with the same content. A middle with no shared unique line (for example a full rewrite) is still one hunk, so a truly large change still trips the size gate. The publisher does not anchor: past the size budget a file still has no inline hunks. Measured on PR 2027 in `OperationCode/front-end` (`pnpm-lock.yaml` of about 16,000 lines, changed near both ends): the patch went from +16,069/-16,091 lines to +1,127/-1,149 (GitHub shows +1,131/-1,153), in about 250,000 steps and 0.02 s on an Apple M-series core. - **Prefilter line diff:** the prefilter does not use `linediff.py`. It has its own line diff (a Myers shortest edit script) to find the "changed lines" (see the notes below the prefilter table). It has its own budgets in steps, `DIFF_WORK_BUDGET = 2,000,000` per file and `RUN_DIFF_WORK_BUDGET = 40,000,000` for all files of one prefilter run, which is one collect run. Every file takes `min(per file, left)` from the run budget, and a file past a limit is charged all it was allowed. The run budget is about 7 s on an Apple M-series core and about 20 s on a GitHub runner, approximate for the same reason. A file's work is bounded by its allowance plus one run of matching lines (at most the file's line count), not exactly by the allowance. Past either limit every head line between the common prefix and suffix counts as changed, and the log names the file and the limit (the run limit when both apply). This changes no gate rule. ## Dependency delta -- **Supported files:** `package.json`, `package-lock.json` (lockfile v2 and v3), `yarn.lock` (v1 and berry), `pnpm-lock.yaml` (v6 and later), `pyproject.toml`, `poetry.lock`, `uv.lock`, `requirements*.txt`. The parser for each file family has fixtures for each supported version. -- **Supported versions:** `package-lock.json` lockfile v2 and v3; `yarn.lock` v1 and berry metadata versions 4 to 8; `pnpm-lock.yaml` 6.0, 6.1 and 9.0; `poetry.lock` 1.1, 2.0 and 2.1; `uv.lock` version 1. Any other version is unknown. Pip `-r` and `-c` includes are not followed, and an unknown pip option is a parse failure. `npm-shrinkwrap.json` is not parsed; it is an unsupported dependency file. +- **Supported files:** `package.json`, `package-lock.json` (lockfile v2 and v3), `yarn.lock` (v1 and berry), `pnpm-lock.yaml` (v6 and later, one YAML document or the pnpm 12 pair of env and main documents, K-16), `pyproject.toml`, `poetry.lock`, `uv.lock`, `requirements*.txt`. The parser for each file family has fixtures for each supported version. +- **Supported versions:** `package-lock.json` lockfile v2 and v3; `yarn.lock` v1 and berry metadata versions 4 to 8; `pnpm-lock.yaml` 6.0, 6.1 and 9.0, in each document of a two-document file; `poetry.lock` 1.1, 2.0 and 2.1; `uv.lock` version 1. Any other version is unknown. Pip `-r` and `-c` includes are not followed, and an unknown pip option is a parse failure. `npm-shrinkwrap.json` is not parsed; it is an unsupported dependency file. - **Delta:** parse each changed supported file at the merge base and at the head (both from blobs by SHA) and compare. For each added or changed package: name, old and new version, resolved source (normalized), registry publish time of the new version, `hasInstallScript` (npm lockfile v2 and v3), and for npm direct deps the install lifecycle scripts from the registry manifest (`preinstall`, `install`, `postinstall`, `prepare`, `preprepare`, `postprepare`, `prepublish`). - **Alternatives:** a `pyproject.toml` dependency can list alternatives, split by markers: a list under `[tool.uv.sources]`, or a list of Poetry constraints. The code reads every alternative, whatever its marker, as its own package with its own version and source, so each one gets the source check and the 72-hour rule. A uv alternative with no known source key, or an empty Poetry list, is a parse failure. - **Registry lookups:** only fixed routes built from validated names: `https://registry.npmjs.org/` and `https://pypi.org/pypi//json`. No redirects to other hosts, a response byte cap, and a timeout. Never fetch a URL from the PR, including a lockfile `resolved` field. The code refuses every redirect. Caps: 16 MiB for one dependency file, 32 MiB for an npm response (the full packument, for per-version publish times), 16 MiB for a PyPI response. Each lookup has a 20-second timeout and at most 2 retries, for 429, 5xx and network errors only. @@ -560,6 +561,7 @@ There is one build of the central workflow. Its gate job always applies every ru - **What the pilot does not protect.** A maintainer or repo admin can merge past any failed run, also on their own PR. The override rule "author is not the PR author" does not apply to a bypass. An author can also call a real finding a false positive and ask a maintainer to bypass. The repo's own approval rule is the only other control. The owner accepts this for the pilot repos. - **Why a bypass list and not Evaluate mode.** Ruleset Evaluate mode is on GitHub Enterprise only ([org rules API](https://docs.github.com/en/rest/orgs/rules#create-an-organization-repository-ruleset)). On Team the workflows rule is either enforced or off. - **Bypass actors.** The [org rules API](https://docs.github.com/en/rest/orgs/rules#create-an-organization-repository-ruleset) lists the actor types `OrganizationAdmin` and `RepositoryRole`, and says `actor_id` is required for `RepositoryRole`. "If actor_type is OrganizationAdmin, actor_id is ignored", so the read-back shows `null` there. The `pull_request` bypass mode "means that an actor can only bypass rules on pull requests" ([repo rules API](https://docs.github.com/en/rest/repos/rules)). The docs do not list the `actor_id` values of the built-in Maintain and Admin roles. So an org owner picks the two roles in the ruleset UI, then reads the ruleset back through the API and records the `actor_id` values. Do not assume them. +- **Immutable subject (prerequisite for every target repo).** A repo created before GitHub made the immutable OIDC subject the default sends `sub` as `repo:OperationCode/:`. The federation rule's prefix `repo:OperationCode@10479364/*` rejects it (Console reason `match_subject_prefix`), and all three passes fail closed with "authentication failed" and the gate fails. This happened on the first pilot runs. Before a repo joins any ruleset that requires the workflow, an org owner checks `GET repos/OperationCode//actions/oidc/customization/sub`, and if `use_immutable_subject` is not `true` (the `sub_claim_prefix` must contain `@`, not the repo name), sets it per repo with `PUT repos/OperationCode//actions/oidc/customization/sub` with `use_default` true and `use_immutable_subject` true (no org template). The change applies to every OIDC workflow in that repo, not only the review. So first list the repo's other `id-token: write` users and check the trust policy of each relying party (for example a cloud OIDC trust policy): each must accept the immutable subject before the change. Add the new subject and keep the old one. Read the setting back, then start a run on a test PR and check that all jobs succeed. - **One ruleset per repo.** No two rulesets may require the workflow on the same repo at the same time. It is not tested whether that starts one run or two; two runs would make the older one `superseded`, so a PR could never pass. When the org ruleset is enabled, its targets leave out the repos still in the pilot. - **Org-wide enforcement.** Enable the org ruleset with the supported repos that are not in the pilot. Then move each pilot repo: remove it from the pilot ruleset targets, then add it to the org ruleset targets at once. When no repo is left, delete the pilot ruleset. Only org admins can then bypass, through a pull request. An override waives false positives, and the bypass covers outages. - **Dependency review.** The dependency-review step has no `continue-on-error`. The gate does not rely on that alone: the job exports the step `outcome`, and the gate fails, not waivable, when it is missing or not `success`, even if `needs.dependency-review.result` is `success`. A unit test fails if any step in the workflow has `continue-on-error` (see "Testing"). @@ -571,7 +573,7 @@ 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. The note stands at the top of the finding details and above the model summaries, both in the collapsed record. A finding title in the visible findings table is model text without the note: its "Found by" cell names the model pass that reported it. - **Inline comments:** one review per run, with `event: COMMENT` and `commit_id` set to the bundle's head SHA, for verified and inline-eligible findings only. Each finding has two keys, both computed from the shown values: each string redacted and cut to its field limit as the publisher shows it, never the raw value, so a published key is not a guess check for a redacted secret. The D14 key is the SHA-256 of file, side, title and quote, joined by NUL. The full key is the SHA-256 of the JSON list of every field that the reader sees: pass, category, severity, confidence, file, side, line, title, quote and explanation (Python `json.dumps` with its default settings, which gives the same ASCII text for the same values). Two findings share a full key only when they show the same text in every one of these fields. Both keys are kept in a hidden `` at the start of each inline comment and read back only from review comments by `github-actions[bot]` whose body starts with that marker and holds no other; a marker with one key is not read, so its finding is posted again. The login proves only that some Actions workflow in the repo wrote the comment (as for the summary), and the keys are public hashes of shown text, so the marker alone proves nothing: the code also checks the text of the comment (review r-p3-cut-final Important 2). In one run, findings are deduped by full key, so each distinct finding gets its own inline comment, even when a cut or a redaction makes two findings equal in file, title and quote (P3-2). Across runs, a finding is not posted again when an earlier comment has its D14 key, the same path and side, and the layout of an inline comment (the marker, a finding line of the schema enums and a pass name, the finding's escaped title, an explanation and the fixed note); the line and the explanation can differ, because the D14 key covers neither. The model words an explanation again on each call and a push moves lines, so the full key of the same issue is rarely the same on two runs, and the D14 key is what keeps a re-run or a push from posting every finding again. The summary points to the earlier comment only when that comment is, in full, the body this run would write for the finding (so its full key matches too), at the same path, side and line (`line`, or `original_line` when GitHub marks the comment outdated). Otherwise the summary shows the finding in full (title, severity, place, explanation and quote), marked as different from the inline comment of an earlier run. So no explanation is hidden behind an earlier comment that says something else, and a forged comment with the D14 key can at most move a finding from its inline comment to its full summary block. That block is subject to the summary size limit below: a high or critical finding is never cut, but a low or medium one can be cut at the largest sizes (residual, see below). If nothing is new, no review is posted. If GitHub rejects the review (HTTP 422, or any other failure), the code does not retry or split it: no inline comment is posted for this run, and the summary shows all of its findings in full. The gate does not change. -- **Summary comment:** one sticky comment, found by the hidden marker `` and an author check (the `github-actions[bot]` identity). A comment with the marker from any other author is ignored. The author check proves only that some Actions workflow in the repo wrote it, so the summary is never a trust source for the gate (see "Summary writes"). The comment is built for a fast human judgment: a reader gets the result and the next step in about three seconds, and everything else is one click away. It never says "approved" and never implies approval; the provenance line says "This bot never approves a PR." The override command and the links to override comments are built from trusted values (server URL, repository, PR number, comment ID), never from a field the API returned. The layout, from top to bottom: +- **Summary comment:** one sticky comment, found by the hidden marker `` and an author check (the `github-actions[bot]` identity). A comment with the marker from any other author is ignored. The author check proves only that some Actions workflow in the repo wrote it, so the summary is never a trust source for the gate (see "Summary writes"). The comment is built for a fast human judgment: a reader gets the result and the next step in about three seconds, and everything else is one click away. It never says "approved" and never implies approval; the provenance line says "This bot never approves a PR." and nothing else in the comment repeats it. The override command and the links to override comments are built from trusted values (server URL, repository, PR number, comment ID), never from a field the API returned. The layout, from top to bottom: ``` hidden marker, first line @@ -618,7 +620,7 @@ scope: workspace:developer token lifetime: 10 minutes ``` - - **Subject in ID form.** GitHub's `sub` uses the immutable ID form `repo:OperationCode@10479364/@:`. The Console's hint `repo:YOUR_ORG/*` does not match it: a rule with the prefix `repo:OperationCode/*` denied every token (reason `match_subject_prefix`). Every rule for this org uses the prefix `repo:OperationCode@10479364/*` (V3). + - **Subject in ID form.** GitHub's `sub` uses the immutable ID form `repo:OperationCode@10479364/@:`. The Console's hint `repo:YOUR_ORG/*` does not match it: a rule with the prefix `repo:OperationCode/*` denied every token (reason `match_subject_prefix`). Every rule for this org uses the prefix `repo:OperationCode@10479364/*` (V3). This holds only for a repo that sends the immutable form: see "Immutable subject" in "Pilot and enforcement". - **Pattern mode, not CEL.** The Console rule has two match modes, and a rule uses only one. CEL mode refuses a compound condition without a subject prefix (a guard for multi-tenant issuers such as GitHub). So the rule uses pattern mode: the subject prefix plus exact claims. There is no `startsWith` and no list of SHAs. - **Claims in a ruleset-required run.** `job_workflow_ref` ends in `@` (V2, V3). A `workflow_dispatch` run of the same file uses the ref form `@refs/heads/`, so it does not match. `ref` and `sha` name the target repo's default branch, so the rule does not match them. The subject prefix binds the org only; the rule does not match the rest of `sub` (it ends in `:pull_request` and does not name the actor, V2, V4). - **One SHA per rule.** An exact claim holds one value, so one rule allows one release SHA. A release makes a new rule for the new SHA and deletes the old rule later (see "Each release" below). @@ -806,7 +808,7 @@ The tasks, owner actions and exit criteria for each step are in [docs/build-plan 2. **Admin setup** (Irving, needs org owner): see the checklist below. 3. **Build** in this repo, with unit tests passing. There is one build; its gate job fails for real on `fail` (see "Pilot and enforcement"). 4. **Fixture evals and acceptance:** the fixture eval passes, and the workflow acceptance tests pass in the scratch repo, including a fork PR from an account outside the org. -5. **Pilot:** the "blocking" items must pass first. Check that the federation rule named in the release code has the release SHA. An org owner gives each pilot repo its own approval protection, then creates the pilot ruleset (see "Pilot and enforcement") with the release SHA and the pilot repos (first `back-end`), reads it back, and enables it. Then list open PRs and start a run on each (push, or close and reopen), because the rule does not start on PRs that were already open. Watch the real results on real PRs: false positives, overrides, bypasses, egress and cost per run. The owner decides when the pilot is done. +5. **Pilot:** the "blocking" items must pass first. Check that the federation rule named in the release code has the release SHA. An org owner gives each pilot repo its own approval protection and checks that it sends the immutable OIDC subject (see "Immutable subject" in "Pilot and enforcement"), then creates the pilot ruleset (see "Pilot and enforcement") with the release SHA and the pilot repos, reads it back, and enables it. Then, right after the enable, list the open PRs again (not from earlier recon) and start a run on each (close and reopen, or a new push). A required workflow starts only on a new PR event, so a PR opened between the recon and the enable gets no run (front-end PR 2027, opened 2026-10-29T08:06Z, was missed this way). Watch the real results on real PRs: false positives, overrides, bypasses, egress and cost per run. The owner decides when the pilot is done. 6. **Each release** (from the pilot on): follow "Each release" in "Auth and cost". Create a new federation rule at the placeholder SHA, put its ID in the candidate code, and set the rule to the candidate SHA. Validate the candidate before any pin moves: unit tests, the owner-approved fixture eval and a token exchange under the scratch test ruleset. If the merge makes a new SHA, its tree must equal the tested candidate. Move the pin of every ruleset that requires the workflow and read each back. Start a new run on each open PR, and delete the old rule only when each has completed at the release SHA (V22). 7. **Before org-wide enforcement:** the owner accepts in writing the residuals in "Which run counts" (an earlier pass on the same SHA, the cross-PR effect of one head SHA, a pin rollback, and run deletion, which V6 f confirmed for a Write user). An org owner fixes the org ruleset (see "Admin checklist"). Build the supported repo inventory (see "Supported repos"), and add CODEOWNERS to each supported repo. Revisit harden-runner block mode (K-14). Re-run the key acceptance tests. 8. **Org-wide enforcement:** enable the org ruleset with the supported repos that are not in the pilot, in small groups. Then move each pilot repo from the pilot ruleset to the org ruleset (see "Pilot and enforcement"), and delete the pilot ruleset. After each group, start a run on each open PR, and check that each shows the required workflow passing or failing on its own result. @@ -824,7 +826,8 @@ The tasks, owner actions and exit criteria for each step are in [docs/build-plan - [ ] Add CODEOWNERS in each active repo for `.github/`, `package.json`, lockfiles, `pyproject.toml`, `Dockerfile`. - [ ] Keep the supported repo inventory current (see "Supported repos"). - [ ] Each pilot repo, before it joins the pilot: its own approval protection on the default branch (a repo ruleset or branch protection with at least 1 approval, code owner review, dismiss stale approvals on push). -- [ ] Pilot ruleset: only the workflows rule, the release SHA, the pilot repos, bypass for the Maintain and Admin repository roles and org admins in `pull_request` mode. Read it back (`repository_id`, path, `sha`, no `ref`, and the role `actor_id` values), enable it, and start runs on open PRs (rollout step 5). +- [ ] Each target repo, before it joins a ruleset that requires the workflow: the immutable OIDC subject is set and read back, and the trust policy of every other OIDC user in that repo accepts it (see "Pilot and enforcement"). +- [ ] Pilot ruleset: only the workflows rule, the release SHA, the pilot repos, bypass for the Maintain and Admin repository roles and org admins in `pull_request` mode. Read it back (`repository_id`, path, `sha`, no `ref`, and the role `actor_id` values), enable it, then list the open PRs again and start a run on each (rollout step 5). - [ ] Org ruleset, before it is enabled: dismiss stale approvals on push on; `OrganizationAdmin` bypass mode `pull_request`, not `always`; remove the empty required status checks rule; fill the workflows rule with the `repository_id` of `OperationCode/.github`, the path and the pinned SHA; targets leave out the repos still in the pilot ruleset. Read it back, then enable it (rollout steps 7 and 8). - [ ] Before org-wide enforcement: record the owner's acceptance of the residuals in "Which run counts", including the cross-PR effect of one head SHA. Repo writers and admins can cancel, re-run and delete runs; no setting in this design limits that. @@ -913,7 +916,7 @@ T2: decisions from the workflows-rule tests T6 and T7 in [claude-review-test](ht - T2-5. "Any newer Actions job" claim (review Important 4): **moot**. The gate no longer depends on check-name selection. V14 and V19 are moot; V20 and V21 merge into V6. - T2-6. Summary ordering (review Minor 5): **accept**. The sticky summary is best effort; run ordering, duplicate deletion and their tests are removed. - T2-7. Fail closed: the required run is `success` only when every job succeeds, so a failed, cancelled or timed-out job blocks whatever the gate job does. No job has an `if:` except `always()` on the gate job. The gate job adds `actions: read` (read only) for the final check. -- T2-8. Rollout (changed by K-2 and P0-10): no advisory mode in target repos, because the review runs there only when required. The pilot on `back-end` runs blocking, with override and bypass as relief. The ruleset is created disabled and an org owner enables it. Each release adds the new SHA to the federation rule before the pin moves (V22). +- T2-8. Rollout (changed by K-2 and P0-10): no advisory mode in target repos, because the review runs there only when required. The pilot (four repos, P4-1; was `back-end` only) runs blocking, with override and bypass as relief. The ruleset is created disabled and an org owner enables it. Each release adds the new SHA to the federation rule before the pin moves (V22). - T2-9. Verify list: V1 answered; V2 to V13 relabeled "build" or "blocking"; V8 narrowed to a fake that starts first and non-member accounts; V22 (pin after main moves), V23 (same-path workflow in the target repo) and V24 (run list) added. T3: decisions on review J2 of the T2 revision, 2026-10-25. Where a T3- line changes a T2- line, the T3- line wins. @@ -985,3 +988,14 @@ P2: phase 2 rulings, 2026-10-27 (UTC). Where a P2- line changes an earlier line, - P3-5. Summary comment redesigned for fast human judgment (owner and coordinator, 2026-10-27). The comment now opens with a result heading and one colored callout, then a findings table, a checks strip and a provenance line, with the waiver and the full record collapsed ("Summary comment" under "Publishing (code)"). The owner chose the Fable design with these rulings: every fail uses the red `[!CAUTION]` callout and the red heading, waivable or not (a waivable fail on a fake check was misread as a mild warning under `[!WARNING]`, so `[!WARNING]` is not used for a fail); the waiver line stays in the collapsed Waiver block; model summaries and token counts stay in the collapsed record; duplicate findings merge only at the same file, side, line and gate effect; a medium finding in a blocking category reads "Does not block"; the visible line shows the 7-character head SHA and the record shows the full SHAs; emoji are allowed, each next to its word. Machine markers, the override line format, the size limit and the overflow path are unchanged. - P3-6. Account X results and Sonnet release (owner and coordinator, 2026-10-28). Account X is `irvingpop2`, a separate GitHub account outside the org that the owner made as a test identity; it became an outside collaborator when it got repo access. Steps ran from an isolated container. V8 b pass (PR 38): the central run started on a first-time-contributor fork PR with no approval click, and marking the draft ready started no new run. V8 a pass (PR 39): from a first-time fork, GitHub holds the fake `pull_request` job (`action_required`), so it cannot start before the central run; after a maintainer approved only that run it passed, and the box stayed BLOCKED and followed the central run; the fake-first case stays covered by PR 36. V23 pass (PR 40): a same-path `claude-review.yml` in the fork PR did not replace the central run, which failed on the bidi hard block; the fork run stayed `action_required` and never ran, so the running case is covered by PR 35. V6 f as a Write user: CONFIRMED residual (PR 41); deleting the newer run reopened the box on an older pass, and it stays in the residuals the owner must accept before org-wide enforcement. A Write user's plain merge of a failed PR was refused; this does not test the bypass, and the `--admin` attempt as Write was not run. V25 pass for Maintain and Admin, Write bypass NOT RUN: with the TR bypass list, a Maintain user (PR 42) and an Admin user (PR 43) merged a failed PR; GitHub records rule suite result `bypass` with the user ID as actor and no role ID, and the org rule-suites endpoint returns 403 on this plan, so use the repo endpoint; a direct push to main by Maintain is refused (GH013). The TR bypass list is a test-only setting and goes back to empty after phase 3 (owner's choice pending). Process: the owner ran the bypass merges and the direct push by hand, because the automation's safety classifier blocks bypass merges; the evidence comes from the GitHub API. Sonnet release: release evals 13 of 13 at M3 ([run 40592861233](https://github.com/OperationCode/.github/actions/runs/40592861233)); the live run [40593729924](https://github.com/OperationCode/claude-review-test/actions/runs/40593729924) passed with `claude-sonnet-5-5`. Console read-back: the owner read back R1 at 590a387 and R2 at 1cefe42 on 2026-10-27, before the M3 edit; R1 was then moved to M3; the service account and the workspace were not read back. V26 a is done for `job_workflow_ref`. NOT RUN, stays open: the Write `--admin` bypass attempt and V26 c deny at M2 before the R2 edit. Web UI "Re-run all jobs": pass (run 40628870689, PR 43, 2026-10-28): the click made attempt 2 of the same run ID, which re-read live state and updated the one summary comment; the merge box was not observed because the PR was already merged. - P3-7. Release M4, the comment redesign (owner and coordinator, 2026-10-28). M4 is `9e2795b97b22869b9e5c7a777e71a2afa8366eeb` on main (code from PR 12 `a410ddd`, docs PR 13). There is no separate candidate branch: the merge SHA was validated directly. Unit tests: `test.yml` at M4, [run 40710532524](https://github.com/OperationCode/.github/actions/runs/40710532524), success. Release evals: `eval.yml` at M4, [run 40713029193](https://github.com/OperationCode/.github/actions/runs/40713029193), 13 of 13 matched, success; the coordinator approved the run under the owner's delegation through phase 4. Console: the owner edited R1 `claude-review-prod-r1` (`fdrl_01N2ZktTiVKqUvvXwywBFfys`) so that `job_workflow_ref` is `OperationCode/.github/.github/workflows/claude-review.yml@9e2795b97b22869b9e5c7a777e71a2afa8366eeb`. The owner reported "rule updated" and shared a screenshot of the rules list; the rule detail value was not read back. TR (org ruleset 25835887) moved from M3 `2c63f49b461feac74c2c70c4f86ca90040df6a0a` to M4; read back: `repository_id` 1432342572, path `.github/workflows/claude-review.yml`, `sha` `9e2795b`, no `ref`, bypass list empty, enforcement active. Live checks in claude-review-test: PR 28 reopened, [run 40713506554](https://github.com/OperationCode/claude-review-test/actions/runs/40713506554) at M4, success, all four jobs `ok`, summary comment in the new layout (green PASS heading, TIP callout, checks strip). PR 40 reopened, [run 40713856249](https://github.com/OperationCode/claude-review-test/actions/runs/40713856249), failure, red FAIL heading, CAUTION callout, the hard block row (bidi) first, merged duplicate rows with "Found by" listing both passes. Both PRs were closed again. Console rules in the owner's screenshot: `claude-review-p3-r2`, `claude-review-prod-r1`, `eval-federation-rule`, `claude-reviewer-test`; the coordinator advised archiving `claude-review-p3-r2`. Deviation 1: no new federation rule for this release; R1 was edited in place, because the merged code names R1 (same as the phase 3 release, where R1 moved from S1 to M3). Cost: M3 can no longer get a token, so a rollback to M3 needs another Console edit. Deviation 2: the denied exchange at M4 before the Console edit (release step 5a) was NOT RUN, because the owner made the edit before the check. M4 is the current production SHA and the current pin. +- K-16. pnpm 12 lockfiles with two YAML documents (ruling on front-end PR 2027, 2026-10-29). pnpm 12 writes `pnpm-lock.yaml` as two YAML documents: first the env lockfile (one importer `.` with `configDependencies` and `packageManagerDependencies`, and the `packages` and `snapshots` of pnpm itself), then the main lockfile. The parser read one document only, so every front-end PR that changed the lockfile got the false hard block "incomplete dependency data" (does not parse), at the head and at the merge base. The parser now reads every document with the same safe loader that refuses duplicate keys. The loader expands YAML merge keys before that check, so a merged key that repeats another key in the same mapping is a parse failure too, in every YAML lockfile. It accepts one document, as before, or exactly two in that order. Each document must have a supported `lockfileVersion`. The first may hold only `lockfileVersion`, `importers`, `packages` and `snapshots`, and its only importer is `.` with only the two env keys. An env `packages` entry with its own `name` or `version` must match its key, and each env dependency must be a package that the parser emits from that document, so no env dependency can skip a check under another identity. The second may hold only the top-level keys of pnpm 12's main lockfile: `lockfileVersion`, `settings`, `catalogs`, `overrides`, `packageExtensionsChecksum`, `pnpmfileChecksum`, `ignoredOptionalDependencies`, `patchedDependencies`, `importers`, `packages`, `snapshots` and `time`. A one-document file keeps the old check, which ignores other keys. Packages of both documents go through every dependency check; pnpm and its platform binaries are dependencies too, and an env dependency counts as direct. Any other document count, an empty document, another env or main shape, or a bare document after a `...` end marker with no `---` is still the hard block. A malformed two-document merge-base file counts as empty, as in K-10. The size gate is unchanged. +- K-17. Anchored patch for long files changed near both ends (pilot false skip, 2026-10-29). PR 2027 in `OperationCode/front-end` (2 files, `package.json` and a `pnpm-lock.yaml` of about 16,000 lines, GitHub +1,131/-1,153) got "too large for automatic review", so a routine dependency update got no model review. Cause: the changes span both ends of the lockfile, so its middle is about 16,000 by 16,000 lines, past `DIFF_BUDGET` (25,000,000). The patch fell back to one hunk with every base line removed and every head line added (+16,069/-16,091). Rendered with line numbers, the bundle estimated 451,544 tokens against the 150,000 limit. A plain difflib diff of that middle needs about 82 million counted steps, past the 5 million per-file budget, so raising `DIFF_BUDGET` alone does not help. Now collect splits such a middle at lines that occur once on each side and diffs only the gaps, within the same budgets ("Coverage limits"). Result for PR 2027 at the same blobs: +1,127/-1,149, estimate 93,786 tokens with the main-branch parser (the head lockfile does not parse there, so the delta is small), 115,961 with the K-16 parser and registry data for all 229 changed packages. Rejected: (b) counting lockfiles at a fine-diff or capped size for the estimate, because the gate must measure what Claude is sent: Claude would still get the 450k-token rendering, and a path rule would let a PR shrink the estimate of any content by its file name. (c) Sending a code-built package delta instead of the lockfile patch: it needs a path or parse rule to pick the files, so content in a lockfile-named file could be hidden from the model passes, and the supply-chain pass would lose the raw lines it reads today. The anchored patch reads no path, never drops a changed line, and leaves the size gate fail-closed: a full rewrite has no anchors and is still one hunk. It changes no gate rule and lowers no risk. + +P4: phase 4 pilot start, 2026-10-28 and 2026-10-29 (owner and coordinator). The pilot ruleset was created and enabled on 2026-10-29. Where a P4- line changes an earlier line, it wins. + +- P4-1. Pilot scope (owner decision). The build plan named only `back-end`. The owner added three repos, so the pilot has four: `back-end` (182584277, `main`), `front-end` (135777271, `main`), `operationcode-pybot` (146210805, `master`) and `operationcode_infra` (107068150, `main`). +- P4-2. Pilot ruleset. Org ruleset `claude-review-pilot` (id 26132750): active; targets the four repo ids and `~DEFAULT_BRANCH`; one workflows rule with `repository_id` 1432342572, path `.github/workflows/claude-review.yml`, `sha` `9e2795b97b22869b9e5c7a777e71a2afa8366eeb` (M4), no `ref`, `do_not_enforce_on_create` false; bypass `RepositoryRole` 2 (Maintain), `RepositoryRole` 5 (Admin) and `OrganizationAdmin`, all in `pull_request` mode. It was created disabled, read back, then enabled. Rollback: `PUT` with `enforcement` set to `disabled`. +- P4-3. Approval protection and CODEOWNERS. Repo rulesets "Pilot approval protection" on `back-end` (26057791), `operationcode-pybot` (26057793) and `operationcode_infra` (26057794): 1 approval, code owner review, dismiss stale reviews, and the same bypass as the pilot ruleset. The reason for the bypass: `operationcode-pybot` and `operationcode_infra` have one active committer, so a rule without a bypass would block every merge. On `front-end` the existing ruleset "Protect main" (25634150) changed in one field only: `require_code_owner_review` false to true. CODEOWNERS was added on each default branch (`back-end`: @irvingpop @kylemh; `front-end`: @kylemh @irvingpop; `operationcode-pybot` and `operationcode_infra`: @irvingpop). +- P4-4. Direct-push check (V25), 12 months: none on `back-end`, `operationcode-pybot` and `operationcode_infra`. `front-end` had a force push on 2026-10-28 that rewrote three merged PRs, a cleanup of a malicious dependency payload; it was not new unreviewed code. +- P4-5. Org 2FA. The owner turned on the org 2FA requirement in the web UI. All 7 members already had 2FA. The federation rule R1 `job_workflow_ref` was read back by the owner at M4 `9e2795b` (this closes the read-back gap in P3-7; the P3-7 text below still says "not read back"). +- P4-6. Spec gap: the immutable subject (design change). Repos created before GitHub's immutable OIDC subject default send `sub` as `repo:OperationCode/:...`, and the federation rule's prefix `repo:OperationCode@10479364/` rejects it (Console reason `match_subject_prefix`). The first pilot runs failed closed as designed: all three passes "authentication failed", gate FAIL, exit 1. Fix, per repo: `PUT repos/OperationCode//actions/oidc/customization/sub` with `use_default` true and `use_immutable_subject` true (no org template). This changes `sub` for every OIDC workflow in the repo. `back-end` and `operationcode-pybot` push images through a cloud role whose trust policy had to gain the immutable subjects; the owner did that and kept the old values. After the change all four test PRs passed, with all four jobs `success`. The spec now lists the subject as a prerequisite for every target repo ("Pilot and enforcement"), and phases 4, 5 and 6 of the build plan have the step. diff --git a/docs/continuation-comments-plan.md b/docs/continuation-comments-plan.md new file mode 100644 index 0000000..9dcbdf4 --- /dev/null +++ b/docs/continuation-comments-plan.md @@ -0,0 +1,332 @@ +# Continuation comments for summary-only findings: plan + +Status: **Planned, not built.** This plan is not part of the spec yet. The spec ([docs/claude-review-design.md](claude-review-design.md)) stays the source of truth until the build PR moves the parts below into it (see "Rollout"). Build plan item: [docs/build-plan.md](build-plan.md), phase 4. +Owner: Irving Popovetsky +Last updated: 2026-10-28 (revised after plan review B: 3 Important, 4 Minor; owner decisions on Q1 to Q7) + +## Goal + +When the sticky summary cannot hold every summary-only finding in full, the publisher posts the findings in a bounded set of continuation comments ("parts"). Then: + +- no explanation of a finding is cut from the PR comments that the current sticky links, also when two runs publish at the same time, and +- summary capacity no longer fails a run whose computed gate result is `pass`, when the overflow comes from findings. + +The fail-closed check stays for every other case (see "Past the bound"). + +A "summary-only finding" is a finding with no inline comment that this run can point to: unverified, outside the diff, on `/.pr/title` or `/.pr/body`, changed from an earlier inline comment, or not posted because GitHub rejected the review. These are the posting states `unverified`, `outside`, `meta`, `changed` and `not-posted` in [publish.py:1085-1097](../review/src/oc_review/publish.py). + +## Current behavior + +All line numbers are at main `03e1b8d`. + +**Where the findings are shown.** The sticky summary shows one table row per place for every finding ([publish.py:1146-1194](../review/src/oc_review/publish.py)). The collapsed record has a detail row per finding ([publish.py:1723-1744](../review/src/oc_review/publish.py)) and, for each summary-only finding, a bullet with the explanation and the quote under "Findings with no inline comment, in full" ([publish.py:1668-1701](../review/src/oc_review/publish.py), [publish.py:1745-1748](../review/src/oc_review/publish.py)). Table rows and detail rows of low and medium findings have a cut rank, so at the largest sizes they can be cut too ([publish.py:1177-1180](../review/src/oc_review/publish.py), [publish.py:1306-1307](../review/src/oc_review/publish.py), [publish.py:1727-1743](../review/src/oc_review/publish.py)). + +**The size limit.** GitHub rejects a comment body over 65,536 characters. The code aims at 64,000 ([publish.py:864-865](../review/src/oc_review/publish.py)). + +**The cut.** `_fit` joins the blocks and cuts by rank, each rank from its end: unverified low and medium findings, low and medium findings, dependency changes, signals ([publish.py:879-883](../review/src/oc_review/publish.py), [publish.py:919-951](../review/src/oc_review/publish.py)). A high or critical finding has no rank, so it is never cut, verified or not ([publish.py:1108-1111](../review/src/oc_review/publish.py), [publish.py:1676-1677](../review/src/oc_review/publish.py)). A cut group gets the note "N item(s) truncated, see the run log" ([publish.py:913-916](../review/src/oc_review/publish.py)), and the full redacted record goes to the gate job log ([publish.py:2208-2210](../review/src/oc_review/publish.py), [publish.py:2036-2044](../review/src/oc_review/publish.py)). + +**The fail-closed check.** When the never-cut blocks alone are over the limit, `_fit` returns `(None, True)` ([publish.py:943-944](../review/src/oc_review/publish.py)). `render_summary` then returns the fixed-size notice with `overflow=True` ([publish.py:1930-1931](../review/src/oc_review/publish.py)). `publish` sets `publish_error = "the summary is over the comment limit"` ([publish.py:2218-2219](../review/src/oc_review/publish.py)) and returns 1 ([publish.py:2256-2258](../review/src/oc_review/publish.py)). The spec states this rule and its residual in "Publishing (code)", "Size limit and cut order" ([claude-review-design.md:594](claude-review-design.md)). + +**What triggers it.** The never-cut data is over 64,000 characters: the heading, callout, checks strip, provenance, snapshot, reasons, waiver, override data, hard block evidence, coverage rows, model summaries, token usage, and the table rows, detail rows and bullets of high and critical findings. Within the per-pass caps (10, 10 and 15 findings, [verdict.py:63-64](../review/src/oc_review/verdict.py)), the bullets are the large part. A measurement at `03e1b8d` with 35 unverified critical findings, every field at its schema limit and every character one that escapes to 3 characters, gave 300,164 characters of blocks: 267,730 in the 35 bullets (about 7,650 each) and 32,434 in everything else. + +**The caps do not bound the input.** A pass over its cap adds a not-waivable `pass-records` reason and still keeps every finding ([verdict.py:950-957](../review/src/oc_review/verdict.py), [verdict.py:50](../review/src/oc_review/verdict.py)). So 35 is the maximum for passes within their caps, not the maximum input of the publisher. Such a run fails at the gate in any case. + +**What the user sees.** The sticky comment becomes the notice `## 🔴 FAIL: OC Claude review could not publish` with a `[!CAUTION]` callout, the computed gate result, the snapshot, a count per reason and the number of valid overrides ([publish.py:1854-1884](../review/src/oc_review/publish.py)). The required run fails, so the merge box blocks, also when the computed result was `pass`. A low or medium bullet that was cut shows only the truncation note; its explanation is in the run log only. + +**Writes today.** The gate job may write only a `COMMENT` review, a new issue comment, and a PATCH of an issue comment ([publish.py:216-220](../review/src/oc_review/publish.py), [publish.py:270-277](../review/src/oc_review/publish.py)). Writes are never retried ([publish.py:229-230](../review/src/oc_review/publish.py)), and a write sends no precondition header, so a PATCH cannot be made conditional on the current body ([publish.py:278-284](../review/src/oc_review/publish.py)). A write can raise after GitHub accepted it, for example a timeout while the response is read ([publish.py:288-304](../review/src/oc_review/publish.py)). The order is inline review, summary, final check, exit code, with a snapshot check before each write ([publish.py:2194-2259](../review/src/oc_review/publish.py)). A snapshot check is three reads with a valid bundle (PR, base tip, compare) and one read without ([publish.py:550-567](../review/src/oc_review/publish.py)). Each read has a 30 second timeout and at most 2 retries on a 429, a 5xx or a network error ([collect.py:76-77](../review/src/oc_review/collect.py), [collect.py:240-268](../review/src/oc_review/collect.py)). The gate job has `contents: read`, `pull-requests: write`, `issues: write` and `actions: read`, no Anthropic credential and a 10-minute timeout ([claude-review.yml:228-237](../.github/workflows/claude-review.yml)). Runs on one PR are not serialized: the workflow has no `concurrency` ([claude-review.yml:8](../.github/workflows/claude-review.yml)). + +**The comment read.** `read_override_inputs` reads every issue comment once and returns an empty tuple when the read fails ([publish.py:625-629](../review/src/oc_review/publish.py)). The caller cannot tell a failed read from a PR with no comments. Then `write_summary` finds no sticky and POSTs a new one ([publish.py:2096-2103](../review/src/oc_review/publish.py)). + +## Design + +### When continuation is used + +The publisher renders the sticky as today. If the result fits with no cut of a finding bullet (no block of rank `CUT_UNVERIFIED` or `CUT_LOW_MEDIUM` is cut, and `_fit` does not return `None`), it posts no part. This is the normal case, and the sticky does not change. + +Otherwise it moves **every** summary-only bullet out of the sticky into parts (decision Q3). All-or-nothing keeps the order of findings in one place and gives the same layout on each re-run. + +What stays in the sticky: the findings table rows, the detail rows, the model summaries and all other never-cut data, each with its cut rank of today. At the largest sizes a low or medium table row or detail row can still be cut, with its truncation note. Its finding is still in full in a part. Dependency changes and signals keep their cut ranks and their run log note. + +### Ownership: each part belongs to one run attempt + +This is the rule that keeps a published set whole when runs overlap (review B, Important 1). + +- A run writes its parts only with POST. Each part carries the marker of its owner: run ID, run attempt and part number. A run never PATCHes a part of another run that can still be linked. +- A part is **linked** when a sticky comment by the bot lists its ID in a hidden links marker. Each sticky lists the IDs of the parts of its own run. +- A part is **retired** (PATCHed to a fixed short body) only when both of these are true: + 1. A run status read shows its owner run attempt is done: the run is `completed`, or its `run_attempt` is higher than the part's attempt. + 2. A comment read made **after** that status read shows no bot sticky that links the part. +- Both conditions stay true once they are true. A done run attempt never writes again, and only that run attempt writes a sticky that links its parts. So a part that a current or future sticky links is never retired. +- There is no reuse of a comment of another run, also not of a retired one. A PATCH cannot be made conditional on the body that the run read ([publish.py:278-284](../review/src/oc_review/publish.py)), so two runs could PATCH one retired comment at the same time and one would lose its part. Snapshot checks do not prevent this: they guard the PR head and base, not comment contents. + +With this rule, the race of review B Important 1 cannot happen: + +``` + run A (attempt 1) run B (attempt 1) + POST A.1, A.2 POST B.1 + sticky := B, links B.1 + cleanup: A is in progress -> A.1, A.2 kept + sticky := A, links A.1, A.2 (last writer wins) + final check: A.1, A.2 read back intact + later run C: + A done, B done; the sticky links A.1, A.2 + -> retire B.1 only +``` + +On an empty PR, two runs each POST their own set. There is no shared slot, so there is no duplicate of a slot. The set that the last sticky does not link is complete text of a run that does not count, and its header says so. A later run retires it. + +### Publish flow + +``` + first comment read (existing, now with a complete flag) + -> override inputs, sticky ID for the backlink, retire candidates (other runs' parts) + | + v + compute_gate (unchanged) + | + v + run status reads for the owners of the candidates (new, capped) + | + v + inline review (unchanged) + | + v + render sticky as today ------------------- fits, no finding bullet cut -----+ + | | + | a finding bullet would be cut, or _fit returns None | + v | + move every summary-only bullet out; sort; pack into parts | + | | + +-- parts > MAX_CONTINUATION, or the sticky still does not fit ---------+ | + | | | + v | | + for part 1..K: deadline? -> snapshot check -> POST own part | | + | stale -> write nothing more, exit 1 | | + | refused, no usable ID, or deadline passed -> stop, publish_error -+ | | + v | | | + snapshot check -> sticky write (normal layout that links the K parts, v v v + or the fail-closed notice; publication result recorded) + | + v + final check (existing reads); the comment read also verifies every linked part + | a linked part missing or changed -> publish_error + v + sticky update, at most one (snapshot check first): + pass -> fail at the final check, or verification failed + publish_error set -> the notice with the final result; else the normal layout + | + v + cleanup: retire candidates that are done and not linked + (snapshot check before each PATCH; capped; errors never fail the run) + | + v + exit code: 1 if publish_error, else verdict.exit_code(final) +``` + +Parts are written before the sticky (decision Q2), so the sticky links only parts that exist and states only what was written. On the first run of a PR that needs continuation, the parts appear above the sticky in the timeline. + +### Publication result + +The publisher records one value for the whole publication, so no later step can lose a failure (review B, Important 3): + +``` + Publication + part_ids: IDs of the parts this run wrote, in order + complete: every part was written with a usable ID and verified + error: None, or one fixed case label (see "Past the bound") +``` + +- `publish_error` is set from `Publication.error` and is never cleared. +- Every sticky render after the first gets the `Publication`. When `error` is set, the render is the fail-closed notice with the current gate result (initial or final) and the case label. It is never the normal layout with part links. So a waiver revoked at the final check after a refused part write updates the notice, not a summary that links missing parts. +- The step summary line gets `publish_error` and the counts `parts_written`, `parts_verified` and `stale_parts_left`. + +### Limits + +| Value | Proposed | Why | +|---|---|---| +| Body limit per part | 64,000 characters, the same `SUMMARY_LIMIT` as the sticky | GitHub rejects bodies over 65,536 characters. The same margin as the sticky. The test measures each final rendered part, with markers, header and footer. | +| Header and footer reserve | 1,000 characters | The header line, the run and sticky links and the untrusted-text note are fixed text plus integers. | +| Bullets per part | No count limit. Greedy packing by size, a bullet is never split | At the per-pass caps, a bullet with its `oc-continued:` marker is about 7,800 characters, so 8 fit in 63,000. | +| `MAX_CONTINUATION` (active parts per run) | 6, provisional (decision Q6) | Within the per-pass caps: 35 bullets at 8 per part is 5 parts, plus one spare. Past the caps the run already fails at the gate, and too many parts fail closed (case 1). | +| Part writes per run | At most `MAX_CONTINUATION` POSTs | One write per part. No retry. | +| Retire writes per run | At most 12 PATCHes, for parts of at most 6 owner run attempts | Bounds the cleanup. The rest waits for a later run (see "Read and write outcomes"). | +| Run status reads per run | At most 6, one per owner run attempt | `GET .../actions/runs/{id}`, oldest owner first. | +| Sticky writes | Unchanged: one write and at most one update | The verification failure uses the same single update as a pass to fail change. | +| Continuation time budget | 180 seconds for the part writes, then what is left of 60 seconds for cleanup | No request or retry starts after the deadline, and each request's timeout is the smaller of 30 seconds and the time left, as in the registry lookup budget ([collect.py:88-90](../review/src/oc_review/collect.py), [collect.py:656-671](../review/src/oc_review/collect.py)). Together 4 minutes, well under the 10-minute job timeout. | + +**Request count.** With a valid bundle, each snapshot check is 3 reads. The new writes are at most 6 part POSTs and 12 retire PATCHes, so at most 18 writes and 54 snapshot reads, plus at most 6 run status reads: 78 new requests per run before transport retries. At 30 seconds per request and up to 3 attempts per read, that worst case is far over the job timeout. The time budget is what bounds it: past the deadline a missing part fails the run (case 4) and the cleanup stops. A failed snapshot read stays fail-closed: the run writes nothing more and exits 1. + +**Historical comments.** Six is a bound on the active parts of one run, not on all part comments of a PR. Each run attempt that uses continuation POSTs up to 6 new comments. Retired comments stay on the PR with a short body (no DELETE, decision Q1). In sequential runs, at most two sets have full text at one time: the set that the sticky links and the set of the run before it, which the next cleanup retires. Overlapping runs, cleanup exhaustion and failed reads can leave more full sets for a time, each labeled with its run. + +**Order.** Parts hold one bullet per summary-only finding, in this order (review B, Minor 1): + +1. severity: critical, high, medium, low; +2. verified before unverified (the finding's own flag); +3. shown file, then line, then side (head before base); +4. pass order: review, security, supply-chain; +5. full key, so the order is total. + +Severity has priority over the table's row order. The table sorts merged rows by their highest severity and whether every member is unverified ([publish.py:1182-1193](../review/src/oc_review/publish.py)), and orders members by pass before severity ([publish.py:1114-1118](../review/src/oc_review/publish.py)). So the members of one merged row can be in different parts: a critical member in part 1 and a low member in part 3. Part 1 always holds the worst findings. + +### Past the bound + +The publisher never drops a finding silently. It fails closed, as today, in each of these cases. Each case has a fixed label in the notice and in the step summary. + +1. **Too many parts.** The bullets need more than `MAX_CONTINUATION` parts. No part is written. +2. **Sticky still too long.** The sticky is over the limit after every bullet moved out (for example, a very long list of coverage rows or hard block evidence, which this plan does not move, decision Q4). No part is written. +3. **Part write refused.** A part write raised `WriteError`, or its response had no usable comment ID. This includes a POST that GitHub accepted but whose response was lost ([publish.py:288-304](../review/src/oc_review/publish.py)). That comment is owned by this run attempt and no sticky links it, so a later run retires it. No more part writes. +4. **Time budget.** The deadline passed before every part was written. No more part writes. +5. **Part changed during the run.** The final-check comment read shows a linked part missing, not by the bot, or with a body other than the body written (line endings compared as `\n`). + +In cases 1 to 4 the sticky write is the notice of [publish.py:1854-1884](../review/src/oc_review/publish.py), with the case label. In case 5 the sticky is already written, so the one sticky update writes the notice (best effort; the exit code is 1 whatever that write does). In every case the full redacted record goes to the gate job log, `publish_error` is set and the job exits 1. A gate reason is never cleared by any of this. + +So the existing `overflow` path in `render_summary` and `publish` stays. What changes is only that findings overflow goes to continuation first. + +### Read and write outcomes + +Each outcome is defined, so no failure reads as an empty PR (review B, Important 2). The first comment read gets a `complete` flag, as `Timeline` and `RunList` have today ([verdict.py:291-292](../review/src/oc_review/verdict.py), [publish.py:642-659](../review/src/oc_review/publish.py)); the override inputs keep using the same comments. + +| Event | What the publisher does | Fails publication? | +|---|---|---| +| First comment read complete, no bot part or sticky | POST every part. Backlink to the PR page. No retire candidate. | No | +| First comment read failed or incomplete (page error, bad Link header, 429 after retries) | POST every part (a POST only creates own comments). Retire nothing in this run. Backlink to the PR page. The sticky write POSTs a new sticky, as today ([publish.py:2096-2103](../review/src/oc_review/publish.py)); a part linked by any bot sticky is kept, so a second sticky protects its parts too. | No | +| Run status read failed (any error after retries) | That owner run counts as not done. None of its parts is retired. | No | +| Final-check comment read failed or incomplete | Verification cannot run. The parts were written with confirmed IDs, so publication stays complete; the step summary records `parts_verified: unknown`. Retire nothing. | No | +| A linked part missing or changed at the final check | Case 5. | Yes | +| Part write refused, 429 or 5xx (writes are not retried), no usable ID, or response lost | Case 3. | Yes | +| More retire candidates than 12 writes or 6 owner runs (cleanup exhaustion) | Retire oldest first up to the caps. Leave the rest for a later run. | No | +| Retire write refused, or its response lost | Stop the cleanup for this run. A lost response leaves the comment retired or not; a later run sees which. | No | +| Deadline passes during the cleanup | Stop the cleanup. | No | + +**Stale parts that stay.** The sticky does not list parts that the cleanup left: the sticky is written before the cleanup, and a sticky update only for cleanup would be one more write per run. Each part is self-describing instead. Its header names its run and attempt and says that the summary comment holds the current review and that a part of another run is out of date. The step summary line has `stale_parts_left`. A stale part never affects the gate. + +### Comment layout + +``` + owner marker, first line + ### OC Claude review: findings in full, part of , run attempt + Run [, attempt ](run URL). The current result is in the [summary comment](link). + A part of another run than the one the summary names is out of date. + The required workflow in the merge box is the result that counts. This bot never approves a PR. + > **Model output from untrusted input. Check it before you act on it.** Verified means ... + one per bullet + - + ... +``` + +Retired body: + +``` + + This part is no longer used. The summary comment holds the current review. +``` + +In the sticky, one new hidden line after the sticky marker: + +``` + the parts this sticky links +``` + +- The owner and parts markers use fixed prefixes and integers only. The regex accepts only that form. Neither contains the sticky marker `` as a substring, because `find_sticky_comment` matches by substring ([verdict.py:756-763](../review/src/oc_review/verdict.py)). A unit test holds this. +- Each bullet has the two keys of `finding_keys` ([publish.py:685-693](../review/src/oc_review/publish.py)) in the inline-marker shape, under the prefix `oc-continued:` and not `oc-finding:`. So `_FINDING_KEY_RE` ([publish.py:666](../review/src/oc_review/publish.py)) never matches it. That regex reads only review comments ([publish.py:718-742](../review/src/oc_review/publish.py)), and parts are issue comments, so there are two separate guards. +- The bullet text has no gate-dependent words: no Effect column, no waiver text, no result. So a final check that turns `pass` into `fail` ([publish.py:2232-2246](../review/src/oc_review/publish.py)) needs no part update. +- **Backlink (review B, Minor 4).** The part body is fixed before the sticky write and is never PATCHed later. When the first comment read found a sticky (`find_sticky_comment`), the link is that comment's `#issuecomment-` anchor. When it found none (the first run, or a failed read), the link is the PR conversation page. The sticky is PATCHed in place on later runs, so from the second run on the anchor is stable. This costs no write. +- The run link, the backlink and the part links in the sticky are built from trusted values only (server URL, repository, PR number, run ID, comment ID), as for the override links today. + +In the sticky: + +- The findings table pointer of a row with summary-only members reads "Details are in part i." with a link, in place of "Details are in the record." A merged row whose summary-only members are in more than one part lists each part once, in part order: "Details are in parts 1 and 3." (decision Q3). +- In the record, "Findings with no inline comment, in full:" becomes one fixed line: "N findings with no inline comment are shown in full in K continuation comments:" and the K links. + +### Re-runs and duplicates + +- **Re-run.** A new run or a new attempt POSTs a new set and its sticky links only that set. The older set is retired by the cleanup of this run or a later one, when it is done and not linked. A re-run makes no second **linked** set: one sticky links one set. +- **No delete.** The plan adds no DELETE route. The write allowlist stays at three routes ([publish.py:216-220](../review/src/oc_review/publish.py)). See decision Q1. +- **No content trust.** The publisher reads a part only for its owner marker, its ID, its author and (for own parts at the final check) its body. It never reads findings, keys or text from a part for a decision. Any Actions workflow in the repo can write as `github-actions[bot]`, so a forged part is at most retired. A forged sticky that links a part only keeps that part from retirement. This is the same rule as for the sticky. +- **Limits.** A person who can edit other people's comments, or another Actions workflow, can edit or delete a part. During the run, the final-check verification catches this (case 5). After the gate job ends, nothing reconciles it: the sticky can link a part with other text or no part. The gate does not change, the full redacted record of the run is in the gate job log, and a re-run publishes a new set. The spec states this residual next to "Summary writes" ([claude-review-design.md:540](claude-review-design.md)). + +### Security + +- **Same escaping and redaction.** A part holds only bullets from `_finding_block`, which uses `_esc` and `escape_text` for every untrusted string ([publish.py:157-205](../review/src/oc_review/publish.py)): control characters removed, credentials redacted before any cut, pings, links and commit references broken, Markdown punctuation escaped. The fixed header text and the markers hold integers and fixed text only. No new escaping code. +- **Only the write job posts.** The parts are written by the gate job `oc-claude-review`, which has no Anthropic credential and no `id-token` ([claude-review.yml:228-237](../.github/workflows/claude-review.yml)). Analyze does not change. +- **Permissions unchanged.** `POST .../issues/{n}/comments` and `PATCH .../issues/comments/{id}` are already allowed. The run status read is a GET under the existing `actions: read`. No new write route, no new scope, no workflow change. +- **PATCH only own comments.** The PATCH route accepts any comment ID. A token with `issues: write` may be able to edit comments by other people, for example an override comment. So the build adds a code guard: a PATCH target must be the sticky that this run read as a bot comment with the sticky marker or wrote itself, or a part that passed the retire rule in this run. Any other ID is refused before the call. A unit test holds this. +- **Override parsing is not affected.** Override comments from bots are ignored, and a part holds only escaped text, so it cannot hold a valid override line. +- **Never a review verdict.** Parts are issue comments. The bot still never approves, requests changes, merges, closes, labels or edits the PR. + +### Gate interaction + +- Continuation runs after `compute_gate` and reads the frozen `GateResult` only. It never changes a reason, a finding, an override or the gate status. +- The exit code stays `1` when `publish_error` is set, else `verdict.exit_code(final)` ([publish.py:2253-2259](../review/src/oc_review/publish.py)). The only change: findings overflow no longer sets `publish_error` when every summary-only finding is published in full in the sticky or in verified parts. That is not a gate reason and no override applies to it, so no gate verdict changes. A failed run caused by a gate reason stays failed. +- A missing, refused or changed part can only make the run fail. Nothing in a part is read back for a decision. Cleanup outcomes never change the exit code. +- The rule "only code lowers risk" holds: the decision to post parts is code, and Claude output cannot clear anything through it. + +## Test plan + +Unit tests, no network, with the fake GitHub of `test_publish.py`. The fake gains run status, comment IDs in creation order and a fake clock. + +- **Normal case unchanged.** Every existing summary test renders the same body. No part is written when nothing would be cut. +- **Bound within the caps.** 35 findings at every schema limit, with characters that escape to 3 characters, need at most `MAX_CONTINUATION - 1` parts. Each **final rendered** part, with its owner marker, header, `oc-continued:` markers and footer, is at most 64,000 characters. The sticky with the parts marker and K links fits too. The test reads the caps from `PASS_MAX_FINDINGS` and `LIMITS`, so a cap change that breaks the bound fails it. +- **Over-limit pass.** A schema-valid review pass with 11 findings at every limit: the gate fails for `pass-records` (not waivable), the run exits 1, and the full redacted record is in the job log, whether the parts fit or case 1 applies. +- **Completeness.** For many sizes and mixes, the set of full keys of summary-only findings equals the set of `oc-continued:` keys across the parts, each once. +- **Order.** The order across parts follows the five keys in "Order". A merged row with a critical and a low summary-only member: the critical bullet is in part 1, the low one in a later part, and the row pointer names both parts. +- **Never split.** No bullet spans two parts. +- **Past the bound.** Cases 1 and 2 with a test-only lower limit: no part is written, the sticky is the notice with the case label, the exit is 1. +- **Refused part write.** A 4xx, a 5xx and a 429 on part 2: no more part writes, the sticky is the notice, `publish_error` is set, exit 1. +- **Lost response.** The fake stores part 2, then the transport raises: case 3, exit 1. A later run (owner done, not linked) retires that part. +- **Time budget.** The fake clock passes the deadline before part 3: case 4. In the cleanup: the cleanup stops, the exit code is the gate's. No request starts after a deadline. +- **Stale.** A snapshot change before part 2: nothing more is written, exit 1, step summary has `stale`. +- **Two publishers interleaved.** Two runs A and B with K = 2 each (test-only limit) on one fake. The test runs every interleaving of their steps (reads, POSTs, sticky writes, cleanup), on an empty PR and on a PR with an older done set. Invariant at the end of every interleaving: every part that a bot sticky links has the body its run wrote, and no part of a run that is not done was retired. A third run C then retires every set that no sticky links. +- **Changed part.** A human edit and a delete of a linked part before the final check: case 5, the sticky update is the notice, exit 1. +- **Combined final update.** A refused part write plus a waiver revoked at the final check: the sticky update is the notice with the final result, not a body with part links. Case 1 plus a newer run at the final check: the notice with the final result. Case 5 plus a pass to fail change: one sticky update, the notice. +- **Read outcomes.** A failed first comment read and a pagination failure: every part is POSTed, nothing is retired, the backlink is the PR page. A failed run status read: that owner's parts are kept. A failed final-check comment read: no case 5, nothing retired, `parts_verified: unknown`. +- **Cleanup bounds.** 13 retire candidates: 12 retired oldest first, `stale_parts_left: 1`, no failure. Candidates of 7 owner runs: 6 status reads. A refused retire write: the cleanup stops, no failure. +- **Retire rule.** A part of an in-progress run is never retired. A part that any bot sticky links is never retired. A part of attempt 1 is retired when attempt 2 is running and no sticky links it. +- **First-run backlink.** No bot comment at the start: each part links the PR page, and no part is PATCHed later. +- **PATCH guard.** A PATCH to an ID that is not the own sticky or a part that passed the retire rule is refused before the call. +- **Markers.** No owner, retired or parts marker contains the sticky marker. `_FINDING_KEY_RE` never matches a part. `find_sticky_comment` never picks a part. An untrusted field that holds a marker string (in every field) is escaped, so it cannot make a marker. A part marker by another author is ignored. A bot-authored forged part is at most retired. +- **Escaping.** The existing escaping tests run on a part body: a finding that quotes a credential, an `@` mention, a link, a commit SHA, HTML and an override line. +- **Gate truth.** For every case of the gate matrix, the gate status and reasons are the same with continuation used and not used. The exit code differs only in the findings-overflow case, from 1 to `verdict.exit_code(final)`. +- **Request count.** No run makes more than 6 part writes, 12 retire writes, 2 sticky writes and 6 run status reads, and each write follows a snapshot check. + +**Eval impact.** Continuation is code only. No model behavior changes, so no new live fixture case is needed. Two changes to the eval runner: + +- It renders the parts too, to `//continuation-.md`, when a case needs them. +- `summary_excludes` checks the sticky and every part, so a credential cannot leave through a part without a failed case. + +Add one offline case under `review/tests/unit/fixtures/` (like `eval-smoke`) with fake pass records that force continuation. It runs in the unit tests, not in the live set. The release eval of the candidate still runs as in "Each release", because the candidate changes code. + +**Live check before release.** The model does not return enough long findings on demand, and the pilot may not show a natural continuation before the release. So the release adds one scripted publisher check on claude-review-test, run by the owner as part of the owner-approved release steps: the candidate gate job publishes the offline case's pass records with a test-only `MAX_CONTINUATION` of 2, (a) twice in a row, (b) twice at the same time, (c) once with case 1. The records show the parts, the links, the retirement of the older set and the notice. One normal run must also show the same sticky as before (no regression). + +## Rollout + +Two PRs, then one release. + +1. **PR 1, pure functions.** Pack, sort, render a part body, the markers and their parsers, the retire rule as a pure function of (parts, run states, stickies), the `Publication` value, and their tests (bound, over-limit, order, completeness, retire rule). Not wired into `publish`. The fail-closed path is unchanged and is the only path. +2. **PR 2, wire it.** The `complete` flag on the comment read, run status reads, part writes with a snapshot check before each, the time budget, verification, the single sticky update with `Publication`, the cleanup, the PATCH guard, the sticky pointers, the eval runner changes and the offline case. Update the spec in the same PR (list below). In this PR, findings overflow stops failing the run, and only after the interleaving, combined final update and read outcome tests pass. The fail-closed check is not removed: it stays for the five cases in "Past the bound". +3. **Release.** The first release after M4. It ships as in "Each release" in the build plan: new federation rule, unit tests, the owner-approved release eval, the live publisher check above, the scratch test ruleset run, pin moves and read-backs. + +The findings-overflow use of the fail-closed check goes away only in PR 2, after PR 1 and PR 2 tests pass, and in production only when the pin moves to that release. The check itself stays. + +Spec sections to change in PR 2: + +- "Architecture" diagram: "post inline comments (event: COMMENT), then the sticky summary" adds the parts between them and the cleanup after the final check. +- "Threat model", the exfiltration row: the publisher escapes text in the summary and in parts. +- "Publishing (code)": "Summary comment" (the parts marker, the part pointers in the table and the record), "Size limit and cut order" (continuation first, `MAX_CONTINUATION`, the five fail-closed cases; remove the residual that a low or medium explanation can be cut and that capacity fails a `pass`), "Full record" (also printed when parts are used), "Order of writes" (inline, parts, summary, final check with verification, at most one summary update, cleanup; PATCH only own comments; the time budget), "Refused summary write" (also a refused, lost, late or changed part). +- "Which run counts", "Summary writes": parts belong to one run attempt, are never a trust source, and are retired only when done and not linked; the residual of edits after the run. +- "Testing": the new unit tests, the eval runner changes and the live publisher check. +- "Decision log": a new line for this change and decisions Q1 to Q7. +- [docs/build-plan.md](build-plan.md) phase 4: mark the item done. +- The `publish.py` module docstring: the write order. + +## Decisions + +The owner accepted the recommended answer to each open question (2026-10-28). Review B's corrections are folded in. + +- **Q1. PATCH stale parts to a retired body, no DELETE.** A DELETE route could delete an override comment if a bug picks the wrong ID; the allowlist stays at three routes, and the PATCH guard is required. Correction: retired comments are not bounded by six (see "Historical comments"), and cleanup exhaustion is defined. In-place reuse of another run's comment is dropped, because a PATCH cannot check the body it replaces (Important 1); retiring stays. +- **Q2. Parts before the sticky.** The sticky can then link parts that exist, by IDs from the POST responses. The first-run backlink is the PR page, and the complete-publication rule is the ownership rule plus verification. +- **Q3. Move every summary-only bullet.** Completeness and a stable order are easier to check. A merged row whose members are in more than one part names each part. +- **Q4. Other never-cut data stays in the sticky.** The build plan item is about findings. Overflow of that data stays fail-closed (case 2), and the full redacted record stays in the job log. +- **Q5. A snapshot check before each write.** The spec requires it ([claude-review-design.md:597](claude-review-design.md)). Correction: it is three reads per check with a valid bundle, and it does not protect comment contents from another writer; the ownership rule does that. +- **Q6. `MAX_CONTINUATION` = 6, provisional.** Within the per-pass caps the worst case is 5 parts. The test measures final rendered parts; an over-limit pass falls to case 1 and fails at the gate anyway. Six bounds active parts per run, not historical comments. +- **Q7. A refused required part fails the run.** Missing full findings must never count as a complete publication. `Publication` keeps the failure through the final update. Retirement errors never fail the run (see "Read and write outcomes"). diff --git a/review/src/oc_review/collect.py b/review/src/oc_review/collect.py index 5cbcc8f..25f7c09 100644 --- a/review/src/oc_review/collect.py +++ b/review/src/oc_review/collect.py @@ -110,7 +110,10 @@ # Past this product of middle lengths, or past the work budget of the file or # of the run (linediff), the patch is one hunk that removes every base line and # adds every head line between the common prefix and suffix: a larger patch -# than git's, never a wrong one. +# than git's, never a wrong one. Past this product the middle is first split at +# lines that occur once on each side (linediff `anchored`, K-17), so a long +# file changed near both ends (a lockfile) keeps a small patch; only the gaps +# that are still past a limit are one hunk each. PATCH_DIFF_BUDGET = linediff.DIFF_BUDGET NO_NEWLINE = "\\ No newline at end of file" @@ -413,7 +416,7 @@ def _git_lines(text: str | None) -> tuple[list[str], bool]: def _opcodes(a: Sequence[Any], b: Sequence[Any], work: linediff.WorkBudget | None) -> linediff.Diff: - return linediff.diff(a, b, PATCH_DIFF_BUDGET, work) + return linediff.diff(a, b, PATCH_DIFF_BUDGET, work, anchored=True) def _range(start: int, stop: int) -> str: @@ -947,7 +950,8 @@ def scan(sha: str | None, mode: str | None, path: str) -> prefilter.BlobScan | N patch, adds, dels, coarse = _patch(base_text, head_text, work) if coarse is not None: # The job log says why (repr: a path is one log line). - print(f"collect: {change.path!r}: {coarse}; the patch is one hunk for the changed middle", file=sys.stderr) + shape = "parts past a limit are one hunk" if coarse.startswith(linediff.ANCHORED) else "the patch is one hunk for the changed middle" + print(f"collect: {change.path!r}: {coarse}; {shape}", file=sys.stderr) try: entry["patch"] = render.redact_patch(patch, base_text, head_text) except render.RenderError as exc: diff --git a/review/src/oc_review/deps.py b/review/src/oc_review/deps.py index e3326d7..2e0944f 100644 --- a/review/src/oc_review/deps.py +++ b/review/src/oc_review/deps.py @@ -762,13 +762,17 @@ def _parse_yarn_berry(text: str) -> _Result: class _UniqueKeyLoader(_SafeLoader): # type: ignore[misc, valid-type] - """Safe loader that refuses a duplicate mapping key (parser differential).""" + """Safe loader that refuses a duplicate mapping key (parser differential). + + Merge keys are expanded first, so a merged key that repeats another key + is refused too: no parser has to choose which value wins. + """ def construct_mapping(self, node: Any, deep: bool = False) -> dict[Any, Any]: + if isinstance(node, yaml.MappingNode): + self.flatten_mapping(node) seen: set[Any] = set() for key_node, _value in node.value: - if key_node.tag == "tag:yaml.org,2002:merge": - continue key = self.construct_object(key_node, deep=True) if key in seen: raise DepsError(f"duplicate YAML key {str(key)[:60]!r}") @@ -801,11 +805,79 @@ def _pnpm_source(resolution: dict[str, Any], key: str, name: str, version: str) return Source(kind="unknown", normalized="") +# pnpm 12 writes the env lockfile (the package manager and config +# dependencies) as a first YAML document before the main lockfile. +_PNPM_ENV_KEYS = frozenset({"lockfileVersion", "importers", "packages", "snapshots"}) +_PNPM_ENV_IMPORTER_KEYS = frozenset({"configDependencies", "packageManagerDependencies"}) +# The top-level fields of pnpm 12's main lockfile (`Lockfile` in its +# lockfile crate). The single-document form keeps the old, open check. +_PNPM_MAIN_KEYS = frozenset({ + "lockfileVersion", "settings", "catalogs", "overrides", "packageExtensionsChecksum", "pnpmfileChecksum", + "ignoredOptionalDependencies", "patchedDependencies", "importers", "packages", "snapshots", "time", +}) + + def _parse_pnpm_lock(text: str) -> _Result: - doc = _as_dict(yaml.load(text, Loader=_UniqueKeyLoader), "pnpm-lock.yaml") + """One document, or two: the env lockfile, then the main lockfile. + + Any other document count, an empty document or a document of an + unexpected shape is a parse failure. Packages of both documents are + returned, so the env document's entries (pnpm itself) get every check. + """ + docs: list[Any] = [] + for doc in yaml.load_all(text, Loader=_UniqueKeyLoader): + docs.append(doc) + if len(docs) > 2: + raise DepsError("pnpm-lock.yaml has more than two YAML documents") + if len(docs) == 1: + return _parse_pnpm_doc(_as_dict(docs[0], "pnpm-lock.yaml"), "pnpm-lock.yaml") + if len(docs) != 2: + raise DepsError("pnpm-lock.yaml has no YAML document") + env = _as_dict(docs[0], "pnpm-lock.yaml env document") + root = _check_pnpm_env_doc(env) + _env_version, env_packages, _ = _parse_pnpm_doc(env, "pnpm-lock.yaml env document") + main = _as_dict(docs[1], "pnpm-lock.yaml main document") + if set(main) - _PNPM_MAIN_KEYS: + raise DepsError(f"pnpm-lock.yaml main document has an unexpected shape (keys {sorted(map(str, main))})") + version, packages, _ = _parse_pnpm_doc(main, "pnpm-lock.yaml") + # Every env dependency must be a package the walker emits, so none + # escapes a check under another identity. + emitted = {(p.name, p.version) for p in env_packages} + for section in _PNPM_ENV_IMPORTER_KEYS: + for name, dep in _as_dict({} if root.get(section) is None else root[section], section).items(): + dep_version = _as_str(_as_dict(dep, f"{section}.{name}").get("version"), f"{section}.{name} version") + if (name, dep_version.split("(", 1)[0]) not in emitted: + raise DepsError(f"pnpm-lock.yaml env dependency {name}@{dep_version} has no packages entry") + return version, env_packages + packages, [] + + +def _check_pnpm_env_doc(env: dict[str, Any]) -> dict[str, Any]: + """The env document holds one importer `.` with only env dependencies, + and its package keys name the identity: an entry's own `name` or + `version` must not differ from its key. Returns the importer `.`. + """ + extra = set(env) - _PNPM_ENV_KEYS + if extra or "lockfileVersion" not in env or "importers" not in env: + raise DepsError(f"pnpm-lock.yaml env document has an unexpected shape (keys {sorted(map(str, env))})") + importers = _as_dict(env["importers"], "env importers") + if list(importers) != ["."]: + raise DepsError("pnpm-lock.yaml env document must have exactly the importer '.'") + root = _as_dict(importers["."], "env importer '.'") + if set(root) - _PNPM_ENV_IMPORTER_KEYS: + raise DepsError("pnpm-lock.yaml env importer '.' has an unexpected key") + entries = _as_dict({} if env.get("packages") is None else env["packages"], "env packages") + for key, entry in entries.items(): + name, version = _pnpm_split_key(_as_str(key, "env package key")) + entry = _as_dict(entry, f"env packages[{key!r}]") + if entry.get("name", name) != name or str(entry.get("version", version)) != version: + raise DepsError(f"pnpm-lock.yaml env package {str(key)[:60]!r} names another package") + return root + + +def _parse_pnpm_doc(doc: dict[str, Any], what: str) -> _Result: version = str(doc.get("lockfileVersion")) if version not in SUPPORTED_PNPM_LOCK_VERSIONS: - raise UnsupportedVersionError(f"pnpm-lock.yaml lockfileVersion {version!r} is not supported") + raise UnsupportedVersionError(f"{what} lockfileVersion {version!r} is not supported") entries = _as_dict({} if doc.get("packages") is None else doc["packages"], "packages") snapshots = _as_dict({} if doc.get("snapshots") is None else doc["snapshots"], "snapshots") build_names: set[tuple[str, str]] = set() @@ -846,7 +918,7 @@ def _pnpm_direct_names(doc: dict[str, Any]) -> set[str]: sections.extend(_as_dict(v, "importer") for v in _as_dict(importers, "importers").values()) names: set[str] = set() for section in sections: - for dep_key in ("dependencies", "devDependencies", "optionalDependencies"): + for dep_key in ("dependencies", "devDependencies", "optionalDependencies", *_PNPM_ENV_IMPORTER_KEYS): deps = section.get(dep_key) if deps is not None: names.update(_as_dict(deps, dep_key)) diff --git a/review/src/oc_review/linediff.py b/review/src/oc_review/linediff.py index 78e7ced..5da1ecf 100644 --- a/review/src/oc_review/linediff.py +++ b/review/src/oc_review/linediff.py @@ -15,6 +15,14 @@ clock). Past a limit the middle is one `replace`: every base line there is removed and every head line added. The result then is not exact, and its `reason` names the limit. + +With `anchored=True` (collect's patches only, K-17), a middle past the size +budget is first split at anchors: lines that occur exactly once in the base +middle and once in the head middle, in the longest run that keeps their +order on both sides. Each gap between anchors gets the bounded diff above, +and a gap past a limit is one `replace`. Anchoring takes its steps from the +same work budget. The result is a correct edit script, never git's, so it is +not exact either. """ from __future__ import annotations @@ -38,6 +46,7 @@ PAST_SIZE_BUDGET = "line diff past the size budget" PAST_FILE_WORK_BUDGET = "line diff past the per-file work budget" PAST_RUN_WORK_BUDGET = "line diff past the run work budget" +ANCHORED = "line diff past the size budget, anchored on lines that occur once on each side" class WorkBudget: @@ -97,9 +106,17 @@ def find_longest_match(self, alo: int = 0, ahi: int | None = None, blo: int = 0, return super().find_longest_match(alo, ahi, blo, bhi) -def diff(a: Sequence[Any], b: Sequence[Any], budget: int = DIFF_BUDGET, work: WorkBudget | None = None) -> Diff: +def diff( + a: Sequence[Any], + b: Sequence[Any], + budget: int = DIFF_BUDGET, + work: WorkBudget | None = None, + *, + anchored: bool = False, +) -> Diff: """difflib-style opcodes of `a` to `b`, bounded by the size budget and by - `work` (a fresh `WorkBudget` when None).""" + `work` (a fresh `WorkBudget` when None). `anchored` splits a middle past + the size budget at unique lines first (`_anchored`).""" if work is None: work = WorkBudget() pre = 0 @@ -114,13 +131,15 @@ def diff(a: Sequence[Any], b: Sequence[Any], budget: int = DIFF_BUDGET, work: Wo if pre: codes.append(("equal", 0, pre, 0, pre)) if am or bm: - middle: list[Opcode] + middle: list[Opcode] | None = None if not am: middle = [("insert", 0, 0, 0, len(bm))] elif not bm: middle = [("delete", 0, len(am), 0, 0)] elif len(am) * len(bm) > budget: reason = PAST_SIZE_BUDGET + if anchored: + middle, reason = _anchored(am, bm, budget, work) else: allowance = max(0, min(work.per_file, work.left)) try: @@ -132,7 +151,7 @@ def diff(a: Sequence[Any], b: Sequence[Any], budget: int = DIFF_BUDGET, work: Wo reason = PAST_FILE_WORK_BUDGET if allowance == work.per_file else PAST_RUN_WORK_BUDGET else: work.left -= allowance - matcher.left - if reason is not None: + if middle is None: middle = [("replace", 0, len(am), 0, len(bm))] codes.extend((tag, i1 + pre, i2 + pre, j1 + pre, j2 + pre) for tag, i1, i2, j1, j2 in middle) if suf: @@ -140,6 +159,95 @@ def diff(a: Sequence[Any], b: Sequence[Any], budget: int = DIFF_BUDGET, work: Wo return Diff(codes, reason) +def _unique_pairs(a: Sequence[Any], b: Sequence[Any]) -> list[tuple[int, int]]: + """(i, j) of each line that occurs once in `a` and once in `b`, by i.""" + seen_a: dict[Any, int] = {} + for i, line in enumerate(a): + seen_a[line] = -1 if line in seen_a else i + seen_b: dict[Any, int] = {} + for j, line in enumerate(b): + seen_b[line] = -1 if line in seen_b else j + return [ + (i, j) for i, line in enumerate(a) if seen_a[line] == i and (j := seen_b.get(line, -1)) >= 0 + ] + + +def _longest_ordered(pairs: list[tuple[int, int]]) -> list[tuple[int, int]]: + """The longest subsequence of `pairs` (sorted by i) whose j also rises.""" + tails: list[int] = [] # smallest last j of a run of each length + tail_at: list[int] = [] # index in `pairs` of that last element + prev = [-1] * len(pairs) + for k, (_, j) in enumerate(pairs): + n = bisect_left(tails, j) + if n == len(tails): + tails.append(j) + tail_at.append(k) + else: + tails[n] = j + tail_at[n] = k + prev[k] = tail_at[n - 1] if n else -1 + out: list[tuple[int, int]] = [] + k = tail_at[-1] if tail_at else -1 + while k >= 0: + out.append(pairs[k]) + k = prev[k] + out.reverse() + return out + + +def _anchored( + a: Sequence[Any], b: Sequence[Any], budget: int, work: WorkBudget +) -> tuple[list[Opcode] | None, str]: + """Opcodes of a middle past the size budget, split at anchors, and the + reason. None (one `replace`) when the file's allowance cannot pay for + the anchoring, which is then not run and costs nothing. + + Anchoring costs one step per line of both sides plus one per line of the + shorter side (the pair list and its longest ordered run). Every gap + takes its steps from what is left of the file's allowance, so the file + and the run limits hold. A file past a limit in a gap is charged its + whole allowance, and the reason names that limit. + """ + allowance = max(0, min(work.per_file, work.left)) + cost = len(a) + len(b) + min(len(a), len(b)) + if cost > allowance: + return None, PAST_SIZE_BUDGET + gaps = WorkBudget(total=allowance - cost, per_file=allowance - cost) + anchors = _longest_ordered(_unique_pairs(a, b)) + codes: list[Opcode] = [] + past: set[str] = set() + i0 = j0 = 0 + for i, j in [*anchors, (len(a), len(b))]: + part = diff(a[i0:i], b[j0:j], budget, gaps) + if part.reason is not None: + past.add(part.reason) + codes.extend((tag, i1 + i0, i2 + i0, j1 + j0, j2 + j0) for tag, i1, i2, j1, j2 in part.codes) + if i < len(a): + codes.append(("equal", i, i + 1, j, j + 1)) + i0, j0 = i + 1, j + 1 + work.left -= allowance - gaps.left + # A gap's own work reason is relative to `gaps`; name the file's limit. + worst = None + if past - {PAST_SIZE_BUDGET}: + worst = PAST_FILE_WORK_BUDGET if allowance == work.per_file else PAST_RUN_WORK_BUDGET + elif past: + worst = PAST_SIZE_BUDGET + reason = ANCHORED if worst is None else f"{ANCHORED}; part {worst.removeprefix('line diff ')}" + return _merge_equal(codes), reason + + +def _merge_equal(codes: list[Opcode]) -> list[Opcode]: + """Join adjacent `equal` opcodes, so `hunks` sees each equal run whole.""" + out: list[Opcode] = [] + for code in codes: + if out and code[0] == "equal" and out[-1][0] == "equal": + tag, i1, _, j1, _ = out[-1] + out[-1] = (tag, i1, code[2], j1, code[4]) + else: + out.append(code) + return out + + def hunks(codes: list[Opcode], n: int) -> Iterator[list[Opcode]]: """Group opcodes into hunks with `n` lines of context (difflib's rule).""" if not any(tag != "equal" for tag, *_ in codes): diff --git a/review/src/oc_review/publish.py b/review/src/oc_review/publish.py index 2e27339..af2dd15 100644 --- a/review/src/oc_review/publish.py +++ b/review/src/oc_review/publish.py @@ -1362,7 +1362,7 @@ def _callout(result: verdict.GateResult, rows: list[_Row], hard: list[_Row], has return "\n".join(f"> {x}" for x in ("[!NOTE]", *lines)) else: lines = ["**No findings. This review found no reason to block.** Nothing to do for this review."] - lines.append("The merge box shows whether the other required checks pass. This bot never approves a PR.") + lines.append("The merge box shows whether the other required checks pass.") return "\n".join(f"> {x}" for x in ("[!TIP]", *lines)) blocking = [r for r in model if r.effect != EFFECT_NONE] @@ -1879,6 +1879,8 @@ def _overflow_body(ctx: PublishContext, result: verdict.GateResult) -> str: flag = "✅ yes" if verdict.WAIVABLE[code] else "🚫 no" lines.append(_row(_reason_label(code), flag, f"{counts[code]} reason(s)")) lines.append(f"\n{len(result.valid_overrides)} valid override(s).") + # The full record body is a separate surface from the pass comment, so its + # "never approves" sentence is intended. lines.append(f"\nRun [{ctx.run_id}, attempt {ctx.run_attempt}]({ctx.run_url}). " "The required workflow in the merge box is the result that counts. This bot never approves a PR.") return "\n".join(lines) diff --git a/review/tests/unit/fixtures/deps/pnpm-lock.v9-env.yaml b/review/tests/unit/fixtures/deps/pnpm-lock.v9-env.yaml new file mode 100644 index 0000000..2bc9e34 --- /dev/null +++ b/review/tests/unit/fixtures/deps/pnpm-lock.v9-env.yaml @@ -0,0 +1,87 @@ +--- +lockfileVersion: '9.0' + +importers: + + .: + configDependencies: {} + packageManagerDependencies: + pnpm: + specifier: 12.15.0 + version: 12.15.0 + +packages: + + '@pnpm/exe.darwin-arm64@12.15.0': + resolution: {integrity: sha512-N0ZykiDWCvqSBJvt71j7cSoBR993FqvYZFtmpCtqvW3pguiI+NAgraLPA5OTwu5mV+/d8hYpVcPktJA3xFDLtA==} + cpu: [arm64] + os: [darwin] + + '@pnpm/exe.linux-x64@12.15.0': + resolution: {integrity: sha512-yc+QlZR+9+nr29J4fRzhMoapgxTBDndq/+peyoq1XUufnQFnfbXUMFudDIKzHi545linGqmOmXfn0NoufNgcOg==} + cpu: [x64] + os: [linux] + libc: [glibc] + + pnpm@12.15.0: + resolution: {integrity: sha512-s5UdbW/0qfE9x89WzSjSuOs328ycaOUuHhGRhz6p+CoBcoerULqtxczElPA2xnwOR+sTYIwvo29HcRth6hKOYQ==} + engines: {node: '>=18'} + hasBin: true + +snapshots: + + '@pnpm/exe.darwin-arm64@12.15.0': + optional: true + + '@pnpm/exe.linux-x64@12.15.0': + optional: true + + pnpm@12.15.0: + optionalDependencies: + '@pnpm/exe.darwin-arm64': 12.15.0 + '@pnpm/exe.linux-x64': 12.15.0 + +--- +lockfileVersion: '9.0' + +settings: + autoInstallPeers: true + excludeLinksFromLockfile: false + +overrides: + postcss: ^8.5.18 + +importers: + + .: + dependencies: + '@next/bundle-analyzer': + specifier: ^16.4.3 + version: 16.4.3 + devDependencies: + esbuild: + specifier: ^0.28.2 + version: 0.28.2 + +packages: + + '@next/bundle-analyzer@16.4.3': + resolution: {integrity: sha512-FGcLziT2rqvGZTxZUjnCzwjCANnigN9vrbCm+1hQtzXbbtc1bXBTRmkXohVqyB/ReF5bOjpM2bNJQhz0PfLk9Q==} + + esbuild@0.28.2: + resolution: {integrity: sha512-HKVLS8dvII+xoKW9kmqxbRKrnWEXfJJr/FZhhJmiqIB0e053QNYFqOBouTMO/k5sID4MvCiUCvv8b9M4h32wIA==} + engines: {node: '>=18'} + hasBin: true + + ms@2.1.3: + resolution: {integrity: sha512-6FlzubTLZG3J2a/NVCAleEhjzq5oxgHyaCU9yYXvcLsvoVaHJq/s5xXI6/XXP6tz7R9xAOtHnSO/tXtF3WRTlA==} + +snapshots: + + '@next/bundle-analyzer@16.4.3': + dependencies: + ms: 2.1.3 + + esbuild@0.28.2: {} + + ms@2.1.3: {} diff --git a/review/tests/unit/test_collect.py b/review/tests/unit/test_collect.py index fcad189..ca85f9b 100644 --- a/review/tests/unit/test_collect.py +++ b/review/tests/unit/test_collect.py @@ -3,6 +3,7 @@ from __future__ import annotations import base64 +import hashlib import json import random import re @@ -434,6 +435,88 @@ def test_size_gate_boundary_on_the_final_document(): assert over.envelope["payload"]["size"]["estimated_tokens"] == tokens +# ---------------------------------------------------------------- lockfile churn and the size gate (K-17) + + +def _pnpm_lock(n: int, bumped: frozenset[int] = frozenset()) -> str: + """A pnpm 9 lockfile of `n` packages, 9 lines each; `bumped` get a new version.""" + + def v(k: int) -> str: + return f"2.0.{k}" if k in bumped else f"1.0.{k}" + + def integrity(k: int) -> str: + return base64.b64encode(hashlib.sha512(f"pkg-{k}@{v(k)}".encode()).digest()).decode() + + lines = ["lockfileVersion: '9.0'", "", "settings:", " autoInstallPeers: true", "", "importers:", "", " .:", " dependencies:"] + for k in range(n): + lines += [f" pkg-{k}:", f" specifier: ^{v(k)}", f" version: {v(k)}"] + lines += ["", "packages:", ""] + for k in range(n): + lines += [f" pkg-{k}@{v(k)}:", f" resolution: {{integrity: sha512-{integrity(k)}}}", " engines: {node: '>=18'}", ""] + lines += ["snapshots:", ""] + for k in range(n): + lines += [f" pkg-{k}@{v(k)}: {{}}", ""] + return "".join(f"{line}\n" for line in lines) + + +def test_lockfile_churn_past_the_diff_size_budget_stays_under_the_size_gate(capsys): + # PR 2027 shape: version bumps near both ends of a 16,000-line lockfile. + n = 1_800 + bumped = frozenset({0, n - 1, *range(7, n, 9)}) + base, head = _pnpm_lock(n), _pnpm_lock(n, bumped) + assert base.count("\n") * head.count("\n") > linediff.DIFF_BUDGET + # One hunk for the whole middle holds every line of both sides: past the gate by itself. + assert (len(base) + len(head)) // c.CHARS_PER_TOKEN > c.SIZE_GATE_TOKENS + col, _, _ = run(_snapshot({"pnpm-lock.yaml": ("100644", base)}, {"pnpm-lock.yaml": ("100644", head)})) + doc = col.envelope + assert doc["status"] == "ok" + assert doc["payload"]["size"]["estimated_tokens"] < c.SIZE_GATE_TOKENS + (f,) = doc["payload"]["files"] + # Each bump changes 5 lines (specifier, version, package key, integrity, snapshot key). + assert (f["additions"], f["deletions"]) == (5 * len(bumped), 5 * len(bumped)) + assert apply_patch(base, f["patch"]) == head + assert linediff.ANCHORED in capsys.readouterr().err + + +@pytest.mark.parametrize("path", ["src/app.js", "pnpm-lock.yaml"]) +def test_large_rewrite_is_still_too_large(path): + # Every line changes, so no line anchors the diff: one hunk, over the gate, + # whatever the file is called (a lockfile name with code in it too). + n = 16_000 + base = "".join(f"export const value{i} = compute({i});\n" for i in range(n)) + head = "".join(f"export const value{i} = compute({i}) + 1;\n" for i in range(n)) + col, _, _ = run(_snapshot({path: ("100644", base)}, {path: ("100644", head)})) + doc = col.envelope + assert doc["status"] == "skipped-size" + assert doc["payload"]["size"]["reason"].startswith(c.SIZE_GATE_REASON) + assert doc["payload"]["size"]["estimated_tokens"] > c.SIZE_GATE_TOKENS + + +INJECTED = " evilScript: node -e fetchAndRunPayload" + + +def test_lockfile_name_or_path_gets_no_other_treatment(): + # The anchored diff never reads the path: code placed in a file named like + # a lockfile, at the root or in an unexpected directory, gets the same + # patch as any other file, and every changed line renders for Claude. + n = 700 + base = _pnpm_lock(n) + head_lines = _pnpm_lock(n, frozenset({0, n // 2, n - 1})).split("\n") + for at in (40, 3_000, 6_000): + head_lines.insert(at, INJECTED) + head = "\n".join(head_lines) + assert base.count("\n") * head.count("\n") > linediff.DIFF_BUDGET + paths = ["pnpm-lock.yaml", "packages/app/node_modules/pnpm-lock.yaml", "src/pnpm-lock.yaml.js"] + col, _, _ = run(_snapshot({p: ("100644", base) for p in paths}, {p: ("100644", head) for p in paths})) + assert col.envelope["status"] == "ok" + files = files_by_path(col) + assert len({files[p]["patch"] for p in paths}) == 1 + patch = files[paths[0]]["patch"] + assert apply_patch(base, patch) == head + assert patch.count(f"+{INJECTED}\n") == 3 + assert render.render_bundle(col.envelope).text.count(INJECTED) == 3 * len(paths) + + # ---------------------------------------------------------------- envelope file @@ -696,7 +779,13 @@ def test_unified_diff_budget_fallback_is_still_correct(monkeypatch): head = base.replace("5\n", "five\n").replace("25\n", "twenty-five\n") patch, adds, dels = c.unified_diff(base, head) assert apply_patch(base, patch) == head - assert (adds, dels) == (21, 21) + # Past the size budget the middle is split at unique lines (K-17). + assert (adds, dels) == (3, 3) # lines 5, 15 and 25 + # With no line that occurs once on each side, it is one hunk. + base, head = "s\n" + "x\n" * 28 + "e\n", "t\n" + "x\n" * 28 + "f\n" + patch, adds, dels = c.unified_diff(base, head) + assert apply_patch(base, patch) == head + assert (adds, dels) == (30, 30) # Low entropy ("a"/"b" lines): about 2.5 million counted diff steps. @@ -947,6 +1036,36 @@ def test_lockfile_migration_gets_every_check(): assert _signals(col, prefilter.BASE_DEP_UNPARSED) +PNPM_TWO_DOCS = ( + "---\nlockfileVersion: '9.0'\nimporters:\n .:\n configDependencies: {}\n" + " packageManagerDependencies:\n pnpm: {specifier: 12.15.0, version: 12.15.0}\n" + "packages:\n pnpm@12.15.0:\n resolution: {integrity: sha512-abc}\nsnapshots:\n pnpm@12.15.0: {}\n" + "---\nlockfileVersion: '9.0'\nimporters:\n .:\n dependencies:\n" + " left-pad: {specifier: ^1.3.0, version: 1.3.0}\n" + "packages:\n left-pad@1.3.0:\n resolution: {integrity: sha512-def}\nsnapshots:\n left-pad@1.3.0: {}\n" +) + + +def test_pnpm_two_document_base_that_does_not_parse_is_a_signal(): + # K-10 holds for the two-document form: a malformed base (an unknown main + # document key) counts as empty, and every head entry gets every check. + base = PNPM_TWO_DOCS + "unrecognizedPackageTable: {}\n" + fx = _snapshot({"pnpm-lock.yaml": ("100644", base)}, {"pnpm-lock.yaml": ("100644", PNPM_TWO_DOCS)}) + fx["registry"] = { + "https://registry.npmjs.org/left-pad": _packument("left-pad", "1.3.0", "2018-04-09T01:51:42.000Z"), + "https://registry.npmjs.org/pnpm": _packument("pnpm", "12.15.0", "2026-09-01T00:00:00.000Z"), + } + col, _, registry = run(fx) + payload = col.envelope["payload"] + assert payload["prefilter"]["hard_blocks"] == [] + assert _signals(col, prefilter.BASE_DEP_UNPARSED) == { + ("pnpm-lock.yaml", "base side counts as empty: does not parse at the merge base") + } + changes = {(ch["name"], tuple(ch["old_versions"]), ch["new_version"]) for ch in payload["dependency_delta"]["changes"]} + assert changes == {("pnpm", (), "12.15.0"), ("left-pad", (), "1.3.0")} + assert sorted(registry.urls) == ["https://registry.npmjs.org/left-pad", "https://registry.npmjs.org/pnpm"] + + def test_lockfile_migration_over_a_lookup_cap_is_a_hard_block(monkeypatch): big = dict(OLD_PACKUMENTS) big["https://registry.npmjs.org/is-odd"] = { diff --git a/review/tests/unit/test_deps.py b/review/tests/unit/test_deps.py index c165c2a..f9750cd 100644 --- a/review/tests/unit/test_deps.py +++ b/review/tests/unit/test_deps.py @@ -277,6 +277,239 @@ def test_pnpm_v5_is_unknown_version(): load("pnpm-lock.v5.yaml", "pnpm-lock.yaml") +# pnpm 12: the env lockfile document, then the main lockfile document. +PNPM_ENV = ( + "lockfileVersion: '9.0'\n" + "importers:\n" + " .:\n" + " configDependencies: {}\n" + " packageManagerDependencies:\n" + " pnpm: {specifier: 12.15.0, version: 12.15.0}\n" + "packages:\n" + " pnpm@12.15.0:\n" + " resolution: {integrity: sha512-abc}\n" + "snapshots:\n" + " pnpm@12.15.0: {}\n" +) +PNPM_MAIN = ( + "lockfileVersion: '9.0'\n" + "importers:\n" + " .:\n" + " dependencies:\n" + " left-pad: {specifier: ^1.3.0, version: 1.3.0}\n" + "packages:\n" + " left-pad@1.3.0:\n" + " resolution: {integrity: sha512-def}\n" + "snapshots:\n" + " left-pad@1.3.0: {}\n" +) + + +def pnpm_two(env: str = PNPM_ENV, main: str = PNPM_MAIN) -> str: + return f"---\n{env}\n---\n{main}" + + +def test_pnpm_two_documents(): + parsed = load("pnpm-lock.v9-env.yaml", "pnpm-lock.yaml") + assert parsed.format_version == "9.0" + names = set(by_name(parsed)) + assert names == {"pnpm", "@pnpm/exe.darwin-arm64", "@pnpm/exe.linux-x64", "@next/bundle-analyzer", "esbuild", "ms"} + pnpm = one(parsed, "pnpm") + assert pnpm.version == "12.15.0" and pnpm.direct is True and pnpm.source.official + assert one(parsed, "@pnpm/exe.linux-x64").direct is False + assert one(parsed, "@next/bundle-analyzer").direct is True + assert one(parsed, "esbuild").direct is True + assert one(parsed, "ms").direct is False + assert all(p.source.official for p in parsed.packages) + + +def test_pnpm_env_document_packages_get_every_check(): + env = PNPM_ENV.replace( + "resolution: {integrity: sha512-abc}", + "resolution: {tarball: 'https://evil.example.com/pnpm-12.15.0.tgz'}\n requiresBuild: true", + ) + parsed = parse("pnpm-lock.yaml", pnpm_two(env=env)) + assert foreign_names(parsed) == {"pnpm"} + assert one(parsed, "pnpm").has_install_script is True + # A new env document adds pnpm to the delta like any other package. + base = parse("pnpm-lock.yaml", PNPM_MAIN) + head = parse("pnpm-lock.yaml", pnpm_two()) + assert [c.name for c in deps.compute_delta(base, head)] == ["pnpm"] + + +def test_pnpm_single_document_still_parses_with_explicit_start(): + assert {p.name for p in parse("pnpm-lock.yaml", f"---\n{PNPM_MAIN}").packages} == {"left-pad"} + + +@pytest.mark.parametrize( + "text", + [ + pytest.param("", id="no-document"), + pytest.param(pnpm_two() + "\n---\n" + PNPM_MAIN, id="three-documents"), + pytest.param(f"---\n---\n{PNPM_MAIN}", id="empty-env-document"), + pytest.param(f"---\n{PNPM_ENV}\n---\n", id="empty-main-document"), + pytest.param(pnpm_two(main="- a\n"), id="main-not-a-mapping"), + pytest.param(pnpm_two(env="- a\n"), id="env-not-a-mapping"), + pytest.param(pnpm_two(env=PNPM_MAIN, main=PNPM_ENV), id="documents-swapped"), + pytest.param(pnpm_two(env=PNPM_ENV + "settings: {autoInstallPeers: true}\n"), id="env-extra-top-level-key"), + pytest.param(pnpm_two(env=PNPM_ENV.replace("lockfileVersion: '9.0'\n", "")), id="env-no-version"), + pytest.param(pnpm_two(env=PNPM_ENV.replace(" .:\n", " web:\n")), id="env-other-importer"), + pytest.param( + pnpm_two(env=PNPM_ENV.replace(" configDependencies: {}\n", " dependencies: {}\n")), + id="env-importer-unknown-key", + ), + pytest.param( + pnpm_two(env=PNPM_ENV.replace("version: 12.15.0}", "version: 12.16.0}")), + id="env-dependency-without-packages-entry", + ), + pytest.param( + pnpm_two(env=PNPM_ENV.replace("packages:\n", "packages:\n pnpm@12.15.0: {}\n")), + id="env-duplicate-key", + ), + pytest.param( + pnpm_two(main=PNPM_MAIN.replace("packages:\n", "packages:\n left-pad@1.3.0: {}\n")), + id="main-duplicate-key", + ), + ], +) +def test_pnpm_multi_document_fails_closed(text): + with pytest.raises(DepsError): + parse("pnpm-lock.yaml", text) + + +@pytest.mark.parametrize("side", ["env", "main"]) +def test_pnpm_multi_document_unknown_version(side): + env, main = PNPM_ENV, PNPM_MAIN + if side == "env": + env = env.replace("'9.0'", "'10.0'") + else: + main = main.replace("'9.0'", "'10.0'") + with pytest.raises(UnsupportedVersionError): + parse("pnpm-lock.yaml", pnpm_two(env=env, main=main)) + + +# An env package whose own name and version are another package's: the env +# check saw pnpm@12.15.0, but the walker emitted left-pad@1.3.0. +PNPM_ENV_RENAMED = PNPM_ENV.replace( + " pnpm@12.15.0:\n resolution:", " pnpm@12.15.0:\n name: left-pad\n version: 1.3.0\n resolution:" +) + + +def test_pnpm_env_package_cannot_take_another_identity(): + base = parse("pnpm-lock.yaml", PNPM_MAIN) + with pytest.raises(DepsError, match="names another package"): + parse("pnpm-lock.yaml", pnpm_two(env=PNPM_ENV_RENAMED)) + # Only the version differs from the key. + with pytest.raises(DepsError, match="names another package"): + parse("pnpm-lock.yaml", pnpm_two(env=PNPM_ENV.replace(" resolution:", " version: 1.0.0\n resolution:", 1))) + # Fields that repeat the key are not a conflict; pnpm still reaches the delta. + same = PNPM_ENV.replace(" resolution:", " name: pnpm\n version: 12.15.0\n resolution:", 1) + assert [c.name for c in deps.compute_delta(base, parse("pnpm-lock.yaml", pnpm_two(env=same)))] == ["pnpm"] + + +@pytest.mark.parametrize( + "extra", + [ + "unrecognizedPackageTable:\n malware@1.0.0:\n resolution: {tarball: 'https://evil.example/malware.tgz'}\n", + "dependencies:\n left-pad: 1.3.0\n", + "onlyBuiltDependencies: [esbuild]\n", + ], + ids=["unknown-table", "v6-root-dependencies", "pnpm-10-field"], +) +def test_pnpm_main_document_extra_key_fails_closed(extra): + with pytest.raises(DepsError, match="main document has an unexpected shape"): + parse("pnpm-lock.yaml", pnpm_two(main=PNPM_MAIN + extra)) + # The single-document form keeps its open check. + assert {p.name for p in parse("pnpm-lock.yaml", PNPM_MAIN + extra).packages} >= {"left-pad"} + + +def test_pnpm_main_document_known_keys_parse(): + main = ( + PNPM_MAIN + + "settings: {autoInstallPeers: true}\ncatalogs: {}\noverrides: {ms: 2.1.3}\n" + + "packageExtensionsChecksum: sha256-x\npnpmfileChecksum: sha256-y\nignoredOptionalDependencies: [fsevents]\n" + + "patchedDependencies: {}\ntime: {}\n" + ) + assert {p.name for p in parse("pnpm-lock.yaml", pnpm_two(main=main)).packages} == {"pnpm", "left-pad"} + + +@pytest.mark.parametrize( + "text", + [ + # Anchors are local to a document: an alias to the env document's anchor. + pytest.param( + pnpm_two(env=PNPM_ENV.replace("{integrity: sha512-abc}", "&res {integrity: sha512-abc}"), + main=PNPM_MAIN.replace("{integrity: sha512-def}", "*res")), + id="cross-document-alias", + ), + pytest.param( + pnpm_two(env=PNPM_ENV.replace("lockfileVersion: '9.0'", "lockfileVersion: !!python/object/apply:os.getcwd []")), + id="unsafe-tag-env", + ), + pytest.param( + pnpm_two(main=PNPM_MAIN.replace("lockfileVersion: '9.0'", "lockfileVersion: !!python/object/apply:os.getcwd []")), + id="unsafe-tag-main", + ), + pytest.param( + pnpm_two(main=PNPM_MAIN.replace("{integrity: sha512-def}", "!!python/name:os.system ''")), + id="unsafe-tag-main-package", + ), + # A merged key that repeats an explicit key: parsers can differ on the winner. + pytest.param( + pnpm_two(main=PNPM_MAIN.replace( + " left-pad@1.3.0:\n resolution: {integrity: sha512-def}\n", + " left-pad@1.3.0:\n <<: {resolution: {tarball: 'https://evil.example/x.tgz'}}\n" + " resolution: {integrity: sha512-def}\n", + )), + id="merge-key-collides-with-explicit-key", + ), + pytest.param( + pnpm_two(main=PNPM_MAIN.replace( + " left-pad@1.3.0:\n resolution: {integrity: sha512-def}\n", + " left-pad@1.3.0:\n <<: [{resolution: {integrity: sha512-def}}, " + "{resolution: {tarball: 'https://evil.example/x.tgz'}}]\n", + )), + id="merge-keys-collide", + ), + pytest.param("---\n" + PNPM_ENV + "...\n---\n" + PNPM_MAIN + "...\n---\n" + PNPM_MAIN, id="three-documents-with-end-markers"), + # YAML 1.2 allows a bare document after `...`; the YAML 1.1 parser does not. + pytest.param(PNPM_ENV + "...\n" + PNPM_MAIN, id="end-marker-without-start-marker"), + ], +) +def test_pnpm_document_boundaries_fail_closed(text): + with pytest.raises(DepsError): + parse("pnpm-lock.yaml", text) + + +def test_pnpm_merge_key_without_collision_parses(): + main = PNPM_MAIN.replace( + " left-pad@1.3.0:\n resolution: {integrity: sha512-def}\n", + " left-pad@1.3.0:\n <<: {resolution: {integrity: sha512-def}}\n deprecated: old\n", + ) + assert one(parse("pnpm-lock.yaml", pnpm_two(main=main)), "left-pad").source.official + + +@pytest.mark.parametrize( + "text", + [ + pytest.param("---\n" + PNPM_ENV + "...\n---\n" + PNPM_MAIN + "...\n", id="explicit-end-markers"), + pytest.param("" + pnpm_two(), id="bom"), + # A `---` line inside a block scalar is text, not a document start. + pytest.param( + pnpm_two(main=PNPM_MAIN.replace( + " resolution: {integrity: sha512-def}\n", + " resolution: {integrity: sha512-def}\n deprecated: |\n use x\n ---\n not a document\n", + )), + id="marker-in-block-scalar", + ), + ], +) +def test_pnpm_document_boundaries_parse(text): + parsed = parse("pnpm-lock.yaml", text) + assert {p.name for p in parsed.packages} == {"pnpm", "left-pad"} + assert one(parsed, "pnpm").direct is True + + # --- pyproject.toml ------------------------------------------------------------ diff --git a/review/tests/unit/test_linediff.py b/review/tests/unit/test_linediff.py index 932e9e2..e297b03 100644 --- a/review/tests/unit/test_linediff.py +++ b/review/tests/unit/test_linediff.py @@ -169,3 +169,95 @@ def test_per_file_and_run_reasons(): assert linediff.diff(a, b, work=linediff.WorkBudget(total=1_000, per_file=10**9)).reason == ( linediff.PAST_RUN_WORK_BUDGET ) + + +# ---------------------------------------------------------------- anchored (collect, K-17) + + +def _check_script(a, b, codes) -> int: + """`codes` is a correct edit script of `a` to `b`; returns the changed line count.""" + i = j = changed = 0 + for tag, i1, i2, j1, j2 in codes: + assert (i1, j1) == (i, j) + if tag == "equal": + assert a[i1:i2] == b[j1:j2] + else: + changed += (i2 - i1) + (j2 - j1) + i, j = i2, j2 + assert (i, j) == (len(a), len(b)) + return changed + + +def _lockfile(n: int, bumped: set[int]) -> list[str]: + """pnpm-like lines: unique name and integrity lines, repeated filler lines.""" + out = [] + for k in range(n): + v = f"1.0.{k + (k in bumped)}" + out += [f" pkg-{k}@{v}:", f" resolution: {{integrity: sha512-{k}-{v}}}", " engines: {node: '>=18'}", ""] + return out + + +def test_anchored_lockfile_churn_is_a_small_diff_within_the_file_budget(): + n = 4_000 # 16,000 lines per side, changed near both ends + bumped = {0, n - 1, *range(1, n, 97)} + a, b = _lockfile(n, set()), _lockfile(n, bumped) + assert len(a) * len(b) > linediff.DIFF_BUDGET + work = linediff.WorkBudget() + d = linediff.diff(a, b, work=work, anchored=True) + assert d.reason == linediff.ANCHORED and not d.exact + assert _check_script(a, b, d.codes) == 4 * len(bumped) # name and integrity line, each side + assert 0 < linediff.RUN_WORK_BUDGET - work.left < linediff.FILE_WORK_BUDGET + # Without `anchored` (the publisher) the middle is one replace, as before. + plain = linediff.diff(a, b, work=linediff.WorkBudget()) + assert plain.reason == linediff.PAST_SIZE_BUDGET + assert _check_script(a, b, plain.codes) > len(a) + len(b) - 10 + + +def test_anchored_diff_is_a_correct_edit_script(): + rng = random.Random(17) + for _ in range(300): + pool = [f"u{k}" for k in range(40)] + ["x", "y", ""] * 10 + a = [rng.choice(pool) for _ in range(rng.randint(0, 50))] + b = [rng.choice(pool) for _ in range(rng.randint(0, 50))] + work = linediff.WorkBudget() + d = linediff.diff(a, b, budget=rng.choice([0, 1, 20, 400]), work=work, anchored=True) + _check_script(a, b, d.codes) + assert all(x[0] != "equal" or y[0] != "equal" for x, y in zip(d.codes, d.codes[1:])) + assert work.left <= linediff.RUN_WORK_BUDGET + + +def test_anchored_with_no_unique_line_is_one_replace(): + a, b = ["s", *_alt(10)], ["t", *_alt(10, "b", "a")] + d = linediff.diff(a, b, budget=1, anchored=True) + assert d.codes == _coarse(a, b) + assert d.reason == linediff.ANCHORED + "; part past the size budget" + + +def test_anchoring_is_charged_and_skipped_when_the_file_cannot_pay(): + a, b = ["a", "x", "b"], ["c", "x", "d"] + cost = len(a) + len(b) + 3 + work = linediff.WorkBudget(total=cost - 1) + d = linediff.diff(a, b, budget=0, work=work, anchored=True) + assert (d.reason, d.codes, work.left) == (linediff.PAST_SIZE_BUDGET, _coarse(a, b), cost - 1) + work = linediff.WorkBudget(total=cost) + d = linediff.diff(a, b, budget=0, work=work, anchored=True) + assert d.codes == [("replace", 0, 1, 0, 1), ("equal", 1, 2, 1, 2), ("replace", 2, 3, 2, 3)] + assert d.reason == linediff.ANCHORED + "; part past the size budget" + assert work.left == 0 + + +def test_anchored_gap_past_the_work_budget_charges_the_whole_allowance(): + low_a, low_b = LOW_ENTROPY["a-only-vs-alternating"] + a, b = ["head", *low_a, "tail"], ["head2", "head", *low_b, "tail"] + # Only the gap after the anchor "head" fits the size budget. + budget = len(low_a) * len(low_b) + work = linediff.WorkBudget() + d = linediff.diff(a, b, budget=budget, work=work, anchored=True) + assert d.reason == linediff.ANCHORED + "; part past the per-file work budget" + assert work.left == linediff.RUN_WORK_BUDGET - linediff.FILE_WORK_BUDGET + _check_script(a, b, d.codes) + run = linediff.WorkBudget(total=1_000_000) + assert linediff.diff(a, b, budget=budget, work=run, anchored=True).reason == ( + linediff.ANCHORED + "; part past the run work budget" + ) + assert run.left == 0 diff --git a/review/tests/unit/test_summary_comment.py b/review/tests/unit/test_summary_comment.py index 6a48ffa..a2e5a1b 100644 --- a/review/tests/unit/test_summary_comment.py +++ b/review/tests/unit/test_summary_comment.py @@ -229,6 +229,7 @@ def test_the_comment_never_implies_approval(rendered): _, r = rendered s = r.summary.lower() assert "approved" not in s and "this bot never approves a pr" in s + assert s.count("never approves") == 1 def test_marker_first_and_details_balanced(rendered): @@ -357,6 +358,7 @@ def test_a_waiver_applied_is_a_green_pass_and_the_findings_stay_listed(tmp_path) s = r.summary assert r.code == 0 and "## 🟢 PASS: OC Claude review\n" in s and callout_kind(s) == "TIP" assert "A maintainer waiver was applied by maintainer. Findings stay listed." in s + assert s.lower().count("never approves") == 1 [row] = table_rows(s, "| Severity |") assert row[1] == "Blocks, waived" assert ("blocking-finding", True) in [(c, w) for c, w, _ in reason_rows(s)] diff --git a/review/tests/unit/test_workflows.py b/review/tests/unit/test_workflows.py index 609f477..df14501 100644 --- a/review/tests/unit/test_workflows.py +++ b/review/tests/unit/test_workflows.py @@ -685,6 +685,14 @@ def test_only_eval_yml_uses_the_eval_environment(): assert users == [(EVAL, "fixture-evals")] +def test_eval_upload_step_always_runs_and_keeps_data_out(): + [step] = [s for j in jobs(load_workflow(WORKFLOWS / EVAL).doc).values() for s in steps(j) + if UPLOAD in str(s.get("uses", ""))] + assert step["if"] == "always()" + assert int(step["with"]["retention-days"]) > 0 + assert "data/" not in step["with"]["path"] + + def run_publish_script(script: str, tmp_path: Path, publish_exit: int) -> tuple[int, str]: """Run a publish script as the runner does (`bash -e`), with a stub `uv` that records its arguments and exits with `publish_exit`. No network."""