[kyle-sexton]

No related issue: claude-code-plugins runs 40054659344 (security) and 40054659337 (code) on PR #9545 finished the model leg (8 and 9 turns), then the runner annotated the key job with "Skip output 'review' since it may contain secret." The status job got an empty review and published failure-class=incomplete: the model returned no usable review. Which secret matched is not verified, and no secret value was read.

Summary

The runner drops a job output whose value contains a registered secret's value anywhere in it, so any review that quotes one fails this way. The review now moves from the model jobs to the status job as an artifact, and only the file's sha256 travels as a job output.

Fix

• Each model job (review-key, review-subscription, identical steps) gains two steps after extract. A script step with no model writes { review, last-summary } to ${{ runner.temp }}/review-handoff/<job>.json and outputs its sha256. upload-artifact (the pinned v7.0.2) uploads it as pr-review-cli-handoff-<lane>-<job>-<run_id>-<head sha>, with overwrite: true and retention-days: 1. The name carries no run_attempt: re-running only the failed status job keeps the key job's attempt-1 outputs, so it has to find the attempt-1 file. The write step runs always(), so a skip still carries its summary.
• review and last-summary leave the job outputs and handoff-sha256 joins them. last-summary is the previous review's summary, read back from the marker, so it is model text as well.
• The status job downloads pr-review-cli-handoff-<lane>-*-<run_id>-<head sha> with merge-multiple and no github-token or run-id, so it reads this run only. A check step recomputes each leg's sha256. On a match it sets the key-review, sub-review and last-summary step outputs. On a mismatch, or with no file, it sets nothing and warns, and publish falls into its existing incomplete path. Step outputs are not filtered for secrets; only job outputs are (sources below).
• Unchanged: publish.cjs, hold.cjs, the env names they read (KEY_REVIEW, SUB_REVIEW, LAST_SUMMARY), the marker, the review-complete notice, check names, the lane-record format, and the trust rules. The model still has no shell or write tool, and the file is written after the model step from the extract step's checked output.
• pr-files stays a job output. It holds the head SHA, changed_files, each file name and a rename's old name, and the hunk ranges as numbers (writePrFiles, pr-review-cli.yml scope step). That is file names and numbers from the PR author, not model text. If a file name happened to contain a registered secret's value, the output would drop, and publish and the hold would fall back to their own listFiles read: a slower run, not a failed one.
• Timeouts: the model jobs go from 40/42 to 42/44 minutes and the status job from 17 to 19, keeping the tested 5-minute headroom over the steps' limits.
• SECURITY MODEL header updated: the handoff, its one-day retention, and why it exposes nothing new. The model reads only the checkout and the files the lane writes for it, and run-claude-cli writes the structured output only after its credential scan clears it.

Sources

• Job outputs: "Outputs containing secrets are redacted on the runner and not sent to GitHub Actions. If an output is skipped because it may contain a secret, you will see the following warning message: "Skip output {output.Key} since it may contain secret."" (workflow syntax, jobs.<job_id>.outputs).
• Substring match: the runner skips an output when !string.Equals(output.Value, HostContext.SecretMasker.MaskSecrets(output.Value)) (actions/runner JobExtension.cs#L821-L825). MaskSecrets replaces every occurrence inside the value, so a secret embedded in a larger text drops the whole output. Step outputs (SetOutputFileCommand) carry no such check (FileCommandManager.cs#L328-L350).
• Artifacts are not masked or scanned. The docs say nothing about masking for artifacts: Store and share data with workflow artifacts describes only digest validation. Redaction is described only for logs: "Redacting of secrets is performed by your workflow runners. This means a secret will only be redacted if it was used within a job and is accessible by the runner. If an unredacted secret is sent to a workflow run log…" (Secure use reference). So nothing documented strips or masks secrets in an upload; the bytes go up as written.
• No REST request: with no github-token, download-artifact leaves findBy unset (download-artifact.ts at v8.0.2). The toolkit's internal client then calls the results service with ACTIONS_RUNTIME_TOKEN (artifact-twirp-client.ts, config.ts). Upload always takes that path (CreateArtifact, FinalizeArtifact in upload-artifact.ts).

API calls per run

• GITHUB_TOKEN REST: +0. The pinned shapes in pr-review-cli-api-calls.test.cjs stay at 8 / 6 / 10 / 8 / 11, now run through the handoff path.
• Results service (runtime token, a separate budget): per uploading leg, CreateArtifact and FinalizeArtifact plus one blob PUT. For the download, one ListArtifacts, then per file a ListArtifacts and a GetSignedArtifactURL plus one blob GET. A key-only run adds 5 results-service calls and 2 blob transfers; a handoff run adds 9 and 4.

Verification

• New: "a review quoting a registered secret's value still reaches publish through the handoff". It runs the key job's shipped handoff step, then drops every job output that contains the sentinel, as the runner does, then runs the status check step and publish. The sentinel arrives in the posted inline comment and the marker, and the job-token requests match the run without it.
• New: "the status job refuses a handoff file whose sha256 does not match the key job's". It rewrites the file between the jobs, and publish fails incomplete.
• New structural test: the artifact name binds lane, job, run id and head SHA; the download has no token or run id; publish and the hold read the checked step outputs; no needs.*.outputs.review or last-summary remains.
• Local, one suite at a time under nice: node --test .github/scripts/*.test.cjs 647 pass, 0 fail; .github/actions/report-lane-outcome/*.test.cjs 77 pass; .github/actions/run-claude-cli/*.test.cjs 72 pass. actionlint (1.7.12) clean on the workflow. zizmor 1.30.0 reports no findings (2 suppressed, as before). biome ci --error-on-warnings .github/scripts .github/actions clean.

Related

• claude-code-plugins pins this workflow at v0.65.0 in pr-review-hosted.yml and pr-review-security-hosted.yml; a follow-up PR repins both after the release.

🤖 Generated with Claude Code

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-10-24T02:53:01.282252Z	c18f059	PR opened

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[claude]

Claude finished @kyle-sexton's task in 4m 7s —— View job

---

Security review complete

• Gather context (PR body, changed files)
• Read diff / changed files
• Apply security review criteria (logic, trust-boundary, Actions security)
• Check instruction-surface deletions / manifest changes (N/A — no instruction files or plugin manifests touched)
• Adversarial validation of candidate findings (no candidates survived initial review, so none needed)
• Post final review

Scope: .github/workflows/pr-review-cli.yml (+150/-15), .github/scripts/pr-review-cli.test.cjs (+41), .github/scripts/pr-review-cli-api-calls.test.cjs (+96/-6).

What changed: the review text and the previous marker's summary move from the model jobs to the status job via an actions/upload-artifact/download-artifact handoff file instead of as job outputs, because the runner silently drops a job output whose value contains a registered secret's value anywhere in it. Only a sha256 of the handoff file crosses as a job output, and the status job refuses the downloaded file unless its hash matches.

Trust-boundary reasoning specific to this change (not zizmor's lane):

• Artifacts are not secret-masked or scanned, so this deliberately routes content that may match a registered secret around the runner's masking. I checked whether this creates a new exfiltration path. It doesn't meaningfully widen exposure: the model has no shell/write/web tools and reads only the checkout and the files the lane writes for it (pr-review-cli.yml#L35), so any value it "quotes" that happens to match a registered secret is a value the pull request's own diff already shows to the same audience (anyone with read access to the repo/run) that could read the artifact. The new test confirms the matched content still reaches the same public pulls.createReview/issues.createComment calls once the handoff round-trips successfully (pr-review-cli-api-calls.test.cjs#L636-L661), so the artifact isn't creating a disclosure that publishing wouldn't also create.
• Artifact naming (pr-review-cli-handoff-${{ inputs.lane }}-${{ github.job }}-${{ github.run_id }}-${{ github.event.pull_request.head.sha }}) is bound to run_id and head.sha, both of which are fixed by the triggering event and not forgeable within the run, so there's no cross-run or cross-PR collision path for the download pattern to pick up another run's artifact. inputs.lane is supplied by the trusted caller workflow (pr-review.yml / pr-review-security.yml), not by the pull request, so it isn't an attacker-controlled input into the artifact name/pattern.
• download-artifact is called with no github-token/run-id, which (per the toolkit's artifact-twirp-client/config.ts, cited in the PR body) scopes the lookup to the current run via ACTIONS_RUNTIME_TOKEN — consistent with the "this run only" claim and not exploitable to pull another run's artifact.
• The status job's handoffs step validates the expected sha256's shape (/^[0-9a-f]{64}$/) and compares it against a hash it recomputes itself from the downloaded bytes, with the expected value coming only from a trusted job output (never from the artifact's own content), so a tampered or substituted artifact can't forge a match — matches the new "refuses a handoff file whose sha256 does not match" test.
• All new/changed uses: references (actions/github-script, actions/upload-artifact@…v7.0.2, actions/download-artifact@…v8.0.2) remain pinned to full commit SHAs with version comments; no permission widening in the review-key/review-subscription/status jobs' permissions: blocks (artifact upload/download rides the runtime token, not GITHUB_TOKEN).

No instruction-surface deletions or plugin-manifest changes are present in this diff, so those criteria don't apply.

No security issues found.

critical-findings: 0

claude-lane-summary: lane=claude-security-review sha=c18f059c8a16b3774d452c4dcead08fef90ede7c run=40055922799
(branch)

[claude]

Claude finished @kyle-sexton's task in 6m 20s —— View job

---

Code review — pr-review-cli.yml artifact handoff

• Gather context (diff, PR body, rules, REVIEW.md)
• Review .github/workflows/pr-review-cli.yml changes
• Review test file changes
• Check pr-body-contract.md rule compliance
• Adversarially validate candidate findings
• Post summary

No findings. I traced the full data path for both legs: extract step output → handoff-file write (sha256 computed over the exact bytes written) → upload-artifact (gated on a non-empty sha256) → download-artifact in the status job (scoped to this run, no token) → the handoffs check step (regex-validates the sha256, re-hashes the downloaded bytes, and only then sets key-review/sub-review/last-summary) → publish.cjs/hold.cjs, which still read KEY_REVIEW/SUB_REVIEW/LAST_SUMMARY unchanged. Everything lines up, including the parts most likely to hide a bug:

• The timeout-budget claim (model jobs 40/42→42/44, status 17→19) is correct and tight against the repo's own enforced invariant (pr-review-cli.test.cjs's total + 5 <= job timeout-minutes check, both lanes) — I summed the step timeouts by hand for both lanes and both land exactly at the 5-minute floor.
• publish.cjs:161 (if (!review) return { ok: false, leg, failureClass: "incomplete" }) is exactly what makes the new "tampered handoff" test's expectation (failure-class=incomplete: the model returned no usable review) true, confirming the sha256 mismatch path degrades the way the test and PR body say it does.
• The two model jobs still each carry their own scope step (lines 359 and 1377), so steps.scope.outputs.last-summary is in scope for both jobs' handoff-write steps as written.
• Artifact naming/pattern binding (lane, job, run id, head SHA) is consistent between the upload name: and download pattern:, and the duplicated step blocks across review-key/review-subscription follow this file's existing (pre-existing, not introduced here) convention of mirroring every step between the two jobs rather than a composite action — not a new issue, so not flagged.
• pr-body-contract.md: title is Conventional Commits, no do-not-merge label, body opens with No related issue: <reason> and includes ## Summary/## Fix/## Verification/## Related — compliant.

Not independently verified (tool-restricted in this lane): I could not execute node --test, actionlint, or zizmor myself, so the PR body's reported local results (647/77/72 passing, clean actionlint/zizmor/biome) are author-claimed and unverified by me — I confirmed the logic statically instead (including hand-tracing the two new tests in pr-review-cli-api-calls.test.cjs against the actual workflow YAML).
·

claude-lane-summary: lane=claude-review sha=c18f059c8a16b3774d452c4dcead08fef90ede7c run=40055922799

[github-actions]

claude-security-review has reviewed this pull request through c18f059; a later push is reviewed from there.

[github-actions]

claude-review has reviewed this pull request through c18f059; a later push is reviewed from there.