Phase 4: upload fixture eval results; show the never-approves line once

.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

docs/build-plan.md

@@ -196,7 +196,7 @@ Exit criteria:
 - [ ] Start a run on each open PR in the pilot repos (push, or close and reopen).
 - [ ] Each week, record false positives, overrides, bypasses, failed runs by reason, egress blocks and cost per run. Keep cost numbers (any currency amount) in the owner's private notes and the Anthropic Console, never in this public repo. Public text may give ratios and timing only.
 - [ ] Fix false positives in prompts or code. Each fix goes through phase 2 again, then ships as in "Each release" below.
-- [ ] 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".
+- [x] Keep the fixture eval results as an artifact: `eval.yml` uploads `results.jsonl` and each case's `summary.md` as `eval-results-a<N>` (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".
 
 Depends on: phase 3.

docs/claude-review-design.md

@@ -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<N>` (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.
 
@@ -571,7 +571,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 `<!-- oc-finding:<d14 key> <full key> -->` 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 `<!-- oc-claude-review -->` 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 `<!-- oc-claude-review -->` 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:
 
   ```
    <!-- oc-claude-review -->                                  hidden marker, first line

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"\n<sub>Run [{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.</sub>")
     return "\n".join(lines)

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)]

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."""