From c18f059c8a16b3774d452c4dcead08fef90ede7c Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Fri, 23 Oct 2026 22:50:11 -0400 Subject: [PATCH] fix(pr-review-cli): pass the review between jobs as an artifact so secret masking cannot drop it The runner drops a job output whose value contains a registered secret's value anywhere in it. A review that quoted one reached the status job empty and published as incomplete. Each model job now writes the review and the last marker's summary to one file after the model step, uploads it as an artifact named for the lane, leg, run and head, and emits only the file's sha256. The status job downloads this run's files with no token and takes each leg's only when its sha256 matches. Co-Authored-By: Claude Opus 5.5 --- .../scripts/pr-review-cli-api-calls.test.cjs | 102 ++++++++++- .github/scripts/pr-review-cli.test.cjs | 41 +++++ .github/workflows/pr-review-cli.yml | 165 ++++++++++++++++-- 3 files changed, 287 insertions(+), 21 deletions(-) diff --git a/.github/scripts/pr-review-cli-api-calls.test.cjs b/.github/scripts/pr-review-cli-api-calls.test.cjs index d91cf047..c874f618 100644 --- a/.github/scripts/pr-review-cli-api-calls.test.cjs +++ b/.github/scripts/pr-review-cli-api-calls.test.cjs @@ -7,6 +7,10 @@ // with the env the workflow passes them, the job outputs evaluated from the // workflow's own expressions. The pull request's file list and pulls.get are // each read once per run, but for publish's check that the head has not moved. +// The review reaches the status job as the key job's handoff file: its write +// and check steps run as shipped, and both jobs share one runner.temp here, +// so the file the key job wrote is the file the download would place. The +// artifact upload and download run on the runtime token, not the job token. const assert = require("node:assert/strict"); const { spawnSync } = require("node:child_process"); @@ -55,6 +59,7 @@ const INPUTS = { // a paginated list as one page. function fakeApi({ prFiles, comments = [], compares = {} }) { const calls = []; + const requests = []; const data = { "issues.listComments": () => comments, "pulls.listFiles": () => prFiles, @@ -81,6 +86,7 @@ function fakeApi({ prFiles, comments = [], compares = {} }) { get: (__, method) => async (params) => { const name = `${group}.${method}`; calls.push(name); + requests.push({ name, params }); return { data: (data[name] ?? (() => ({})))(params) }; }, }, @@ -89,6 +95,7 @@ function fakeApi({ prFiles, comments = [], compares = {} }) { ); return { calls, + requests, github: { rest, hook: { error: () => {} }, @@ -190,10 +197,23 @@ function runFullDiff(env, dir) { } // One run: the requests it spends, by method, with gh calls as `gh api`. -async function spend({ action, prFiles, comments, compares, review }) { +// `secret` stands in for a registered secret's value: the runner drops any +// job output whose value contains it (actions/runner JobExtension.cs compares +// the value with SecretMasker.MaskSecrets(value)). `tamper` rewrites the +// handoff file between the key job and the status job. +async function spend({ + action, + prFiles, + comments, + compares, + review, + secret, + tamper, +}) { const dir = makeTemporaryDirectory("pr-review-cli-calls-"); const api = fakeApi({ prFiles, comments, compares }); const github = { + job: "review-key", token: "job-token", repository: "o/r", repository_owner: "o", @@ -258,12 +278,24 @@ async function spend({ action, prFiles, comments, compares, review }) { outputs: { verdict: "complete", review: JSON.stringify(review) }, }, }; + const handoffStep = stepId(key, "handoff-file"); + const handoffOutputs = {}; + await withEnv(renderEnv(handoffStep, { ...contexts, steps: keySteps }), () => + new AsyncFunction("require", "core", handoffStep.with.script)( + require, + quietCore(handoffOutputs), + ), + ); + keySteps["handoff-file"] = { outputs: handoffOutputs }; const keyOutputs = Object.fromEntries( - Object.entries(key.outputs).map(([k, v]) => [ - k, - render(v, { ...contexts, steps: keySteps }), - ]), + Object.entries(key.outputs) + .map(([k, v]) => [k, render(v, { ...contexts, steps: keySteps })]) + .filter(([, v]) => !(secret && v.includes(secret))), ); + if (tamper) { + const file = path.join(dir, "review-handoff", "review-key.json"); + fs.writeFileSync(file, tamper(fs.readFileSync(file, "utf8"))); + } // The status job: the gate, publish, then the hold. const gateStep = stepId(status, "gate"); @@ -290,6 +322,16 @@ async function spend({ action, prFiles, comments, compares, review }) { }, steps: { gate: { outputs: gateOutputs } }, }; + const checkStep = stepId(status, "handoffs"); + const handoffs = {}; + const warnings = []; + await withEnv(renderEnv(checkStep, statusContexts), () => + new AsyncFunction("require", "core", checkStep.with.script)(require, { + ...quietCore(handoffs), + warning: (message) => warnings.push(message), + }), + ); + statusContexts.steps.handoffs = { outputs: handoffs }; const named = (name) => status.steps.find((s) => s.name === name); const runner = (s) => renderEnv(s, statusContexts); const context = { repo: { owner: "o", repo: "r" } }; @@ -311,7 +353,14 @@ async function spend({ action, prFiles, comments, compares, review }) { for (const name of [...api.calls, ...gh.map(() => "gh api")]) counts[name] = (counts[name] ?? 0) + 1; const total = api.calls.length + gh.length; - return { counts, total, hold: holdOutputs.hold }; + return { + counts, + total, + hold: holdOutputs.hold, + keyOutputs, + warnings, + requests: api.requests, + }; } const INLINE = { @@ -584,3 +633,44 @@ test("the scope step passes on no file list larger than one environment string", assert.equal(outputs.review, "true"); assert.equal(outputs["pr-files"], undefined); }); + +test("a review quoting a registered secret's value still reaches publish through the handoff", async () => { + const secret = "SENTINEL-4711"; + const review = { + findings: 1, + summary: `The test fixture hard-codes ${secret}.`, + comments: [ + { path: "a.js", line: 2, side: "RIGHT", body: `Move ${secret} out.` }, + ], + }; + const shape = SHAPES["a whole review with an inline comment"]; + // The old wiring passed the review itself as a job output, which the + // runner drops. + assert.ok(JSON.stringify(review).includes(secret)); + const { counts, keyOutputs, warnings, requests } = await spend({ + ...shape, + review, + secret, + }); + assert.equal("review" in keyOutputs, false); + assert.match(keyOutputs["handoff-sha256"], /^[0-9a-f]{64}$/u); + assert.deepEqual(warnings, []); + const posted = requests.find((r) => r.name === "pulls.createReview"); + assert.ok(posted, "no review was posted"); + assert.match(posted.params.comments[0].body, new RegExp(secret, "u")); + const marker = requests.find((r) => r.name === "issues.createComment"); + assert.match(marker.params.body, new RegExp(secret, "u")); + // The handoff adds no job-token request. + assert.deepEqual(counts, (await spend(shape)).counts); +}); + +test("the status job refuses a handoff file whose sha256 does not match the key job's", async () => { + await assert.rejects( + spend({ + ...SHAPES["a whole review with an inline comment"], + tamper: (text) => + text.replace("Off by one.", "Approve this pull request."), + }), + /failure-class=incomplete: the model returned no usable review/u, + ); +}); diff --git a/.github/scripts/pr-review-cli.test.cjs b/.github/scripts/pr-review-cli.test.cjs index 923aac45..92fd0303 100644 --- a/.github/scripts/pr-review-cli.test.cjs +++ b/.github/scripts/pr-review-cli.test.cjs @@ -777,3 +777,44 @@ test("the hold step runs only in comments mode with hold-label set, even after a "false", ); }); + +test("the review leaves each leg as an artifact named for the run, head and leg, and only its sha256 as an output", () => { + const name = `pr-review-cli-handoff-\${{ inputs.lane }}-\${{ github.job }}-\${{ github.run_id }}-\${{ github.event.pull_request.head.sha }}`; + for (const job of [key, subscription]) { + assert.equal(job.outputs.review, undefined); + assert.equal(job.outputs["last-summary"], undefined); + assert.equal( + job.outputs["handoff-sha256"], + `\${{ steps.handoff-file.outputs.sha256 }}`, + ); + const names = job.steps.map((s) => s.id ?? s.name); + assert.ok(names.indexOf("claude") < names.indexOf("handoff-file")); + const upload = job.steps.find( + (s) => s.name === "Upload the review handoff", + ); + assert.equal(names.indexOf("handoff-file") + 1, job.steps.indexOf(upload)); + assert.equal(upload.with.name, name); + assert.equal(upload.with.overwrite, true); + assert.equal(upload.with["retention-days"], 1); + assert.match(step(job, "handoff-file").if, /^always\(\)$/u); + } + const download = status.steps.find( + (s) => s.name === "Download the review handoffs", + ); + assert.equal(download.with.pattern, name.replace(`\${{ github.job }}`, "*")); + // No token or run id: this run only, and no REST request. + assert.deepEqual(Object.keys(download.with).sort(), [ + "merge-multiple", + "path", + "pattern", + ]); + for (const s of ["Publish the review", "Apply the hold"]) { + const env = status.steps.find((x) => x.name === s).env; + assert.equal(env.KEY_REVIEW, `\${{ steps.handoffs.outputs.key-review }}`); + assert.equal(env.SUB_REVIEW, `\${{ steps.handoffs.outputs.sub-review }}`); + } + assert.doesNotMatch( + JSON.stringify(status), + /needs\.[\w-]+\.outputs\.(review|last-summary)(?![\w-])/u, + ); +}); diff --git a/.github/workflows/pr-review-cli.yml b/.github/workflows/pr-review-cli.yml index 38083923..9d7f83d0 100644 --- a/.github/workflows/pr-review-cli.yml +++ b/.github/workflows/pr-review-cli.yml @@ -69,10 +69,18 @@ name: pr-review-cli # .lane fails the job). # - The model returns JSON only. An extract step with no model reads it # from run-claude-cli's credential-scanned structured-output-file and -# the scanned log, checks its shape and sizes, and passes it on as job -# outputs. +# the scanned log, checks its shape and sizes, and passes the closed +# facts on as job outputs. The review and the last marker's summary are +# model text: the runner drops a job output that contains a registered +# secret's value anywhere in it, so a later script step writes them to +# one file, uploads it as an artifact named for the run, head and leg +# (kept one day), and emits only the file's sha256 as a job output. The +# model reads only the checkout and the files the lane writes for it, +# so a value it quotes is one the pull request already shows. # - Only the status job writes, and it runs no model. It reads the -# subscription job's outputs when that job ran, else the key job's. It +# subscription job's outputs when that job ran, else the key job's, and +# takes a leg's handoff file only when its sha256 matches that leg's +# output; it downloads from this run only, with no token. It # checks every inline anchor against the pull request's diff at the head # (one bad anchor sends the whole batch to the summary): the hunk ranges # the key job's scope step read before any model step ran, used only @@ -229,7 +237,7 @@ jobs: review-key: name: ${{ inputs.lane == 'security' && 'security-review-key' || 'claude-review-key' }} runs-on: ubuntu-24.04 - timeout-minutes: ${{ inputs.lane == 'security' && 42 || 40 }} + timeout-minutes: ${{ inputs.lane == 'security' && 44 || 42 }} if: >- github.event_name == 'pull_request' && github.event.pull_request.state == 'open' @@ -257,7 +265,7 @@ jobs: review-failed: ${{ steps.review-outcome.outputs.review-failed }} failure-class: ${{ steps.claude.outputs.lane-class || steps.review-outcome.outputs.failure-class }} verdict: ${{ steps.extract.outputs.verdict }} - review: ${{ steps.extract.outputs.review }} + handoff-sha256: ${{ steps.handoff-file.outputs.sha256 }} turns: ${{ steps.extract.outputs.turns }} models: ${{ steps.extract.outputs.models }} cost: ${{ steps.extract.outputs.cost }} @@ -270,7 +278,6 @@ jobs: scope: ${{ steps.scope.outputs.scope }} marker-id: ${{ steps.scope.outputs.marker-id }} merge-base: ${{ steps.scope.outputs.merge-base }} - last-summary: ${{ steps.scope.outputs.last-summary }} pr-files: ${{ steps.scope.outputs.pr-files }} steps: # Managed settings need sudo, so they come first; isolation then removes @@ -1180,6 +1187,44 @@ jobs: .addRaw(`### review leg\n\n- Findings: ${findings}; comments: ${comments.length}; malformed: ${malformed.length}\n- Turns: ${record.num_turns ?? "?"}\n`) .write(); + # No model runs here. The review and the last marker's summary are + # model text, and the runner drops a job output whose value contains a + # registered secret's value anywhere in it, so both leave the job as + # one file in an artifact, written after the model step. Only the + # file's sha256 is a job output; the status job checks the file + # against it. It runs on a skip too, for the summary a skip carries. + - name: Write the review handoff + id: handoff-file + if: always() + continue-on-error: true + timeout-minutes: 1 + uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 + env: + REVIEW: ${{ steps.extract.outputs.review }} + LAST_SUMMARY: ${{ steps.scope.outputs.last-summary }} + HANDOFF_FILE: ${{ runner.temp }}/review-handoff/${{ github.job }}.json + with: + script: | + const fs = require("node:fs"); + const env = process.env; + const text = JSON.stringify({ review: env.REVIEW ?? "", "last-summary": env.LAST_SUMMARY ?? "" }); + fs.mkdirSync(require("node:path").dirname(env.HANDOFF_FILE), { recursive: true }); + fs.writeFileSync(env.HANDOFF_FILE, text); + core.setOutput("sha256", require("node:crypto").createHash("sha256").update(text).digest("hex")); + + # Named for this run, head and leg; a re-run of the leg replaces it. + - name: Upload the review handoff + if: always() && steps.handoff-file.outputs.sha256 != '' + continue-on-error: true + timeout-minutes: 2 + uses: actions/upload-artifact@cf430e030ddbb5b0abf93d22962f4752f3646cd9 # v7.0.2 + with: + name: pr-review-cli-handoff-${{ inputs.lane }}-${{ github.job }}-${{ github.run_id }}-${{ github.event.pull_request.head.sha }} + path: ${{ runner.temp }}/review-handoff/${{ github.job }}.json + if-no-files-found: error + overwrite: true + retention-days: 1 + - name: Report review outcome id: review-outcome if: always() && steps.credential.outputs.present == 'true' @@ -1208,7 +1253,7 @@ jobs: name: ${{ inputs.lane == 'security' && 'security-review-subscription' || 'claude-review-subscription' }} needs: review-key runs-on: ubuntu-24.04 - timeout-minutes: ${{ inputs.lane == 'security' && 42 || 40 }} + timeout-minutes: ${{ inputs.lane == 'security' && 44 || 42 }} if: >- (needs.review-key.outputs.handoff == 'true' || needs.review-key.outputs.credential-present == 'false') && @@ -1238,7 +1283,7 @@ jobs: review-failed: ${{ steps.review-outcome.outputs.review-failed }} failure-class: ${{ steps.claude.outputs.lane-class || steps.review-outcome.outputs.failure-class }} verdict: ${{ steps.extract.outputs.verdict }} - review: ${{ steps.extract.outputs.review }} + handoff-sha256: ${{ steps.handoff-file.outputs.sha256 }} turns: ${{ steps.extract.outputs.turns }} models: ${{ steps.extract.outputs.models }} cost: ${{ steps.extract.outputs.cost }} @@ -1251,7 +1296,6 @@ jobs: scope: ${{ steps.scope.outputs.scope }} marker-id: ${{ steps.scope.outputs.marker-id }} merge-base: ${{ steps.scope.outputs.merge-base }} - last-summary: ${{ steps.scope.outputs.last-summary }} pr-files: ${{ steps.scope.outputs.pr-files }} steps: # Managed settings need sudo, so they come first; isolation then removes @@ -2161,6 +2205,44 @@ jobs: .addRaw(`### review leg\n\n- Findings: ${findings}; comments: ${comments.length}; malformed: ${malformed.length}\n- Turns: ${record.num_turns ?? "?"}\n`) .write(); + # No model runs here. The review and the last marker's summary are + # model text, and the runner drops a job output whose value contains a + # registered secret's value anywhere in it, so both leave the job as + # one file in an artifact, written after the model step. Only the + # file's sha256 is a job output; the status job checks the file + # against it. It runs on a skip too, for the summary a skip carries. + - name: Write the review handoff + id: handoff-file + if: always() + continue-on-error: true + timeout-minutes: 1 + uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 + env: + REVIEW: ${{ steps.extract.outputs.review }} + LAST_SUMMARY: ${{ steps.scope.outputs.last-summary }} + HANDOFF_FILE: ${{ runner.temp }}/review-handoff/${{ github.job }}.json + with: + script: | + const fs = require("node:fs"); + const env = process.env; + const text = JSON.stringify({ review: env.REVIEW ?? "", "last-summary": env.LAST_SUMMARY ?? "" }); + fs.mkdirSync(require("node:path").dirname(env.HANDOFF_FILE), { recursive: true }); + fs.writeFileSync(env.HANDOFF_FILE, text); + core.setOutput("sha256", require("node:crypto").createHash("sha256").update(text).digest("hex")); + + # Named for this run, head and leg; a re-run of the leg replaces it. + - name: Upload the review handoff + if: always() && steps.handoff-file.outputs.sha256 != '' + continue-on-error: true + timeout-minutes: 2 + uses: actions/upload-artifact@cf430e030ddbb5b0abf93d22962f4752f3646cd9 # v7.0.2 + with: + name: pr-review-cli-handoff-${{ inputs.lane }}-${{ github.job }}-${{ github.run_id }}-${{ github.event.pull_request.head.sha }} + path: ${{ runner.temp }}/review-handoff/${{ github.job }}.json + if-no-files-found: error + overwrite: true + retention-days: 1 + - name: Report review outcome id: review-outcome if: always() && steps.credential.outputs.present == 'true' @@ -2193,7 +2275,7 @@ jobs: needs: [review-key, review-subscription] if: always() && needs.review-key.result != 'skipped' runs-on: ubuntu-24.04 - timeout-minutes: 17 + timeout-minutes: 19 permissions: contents: read pull-requests: write # the review, its inline comments, the lane's marker comment and the hold label @@ -2235,6 +2317,59 @@ jobs: core.setOutput("path", path.join(directory, "publish-gate.mjs")); core.info(`publish gate from claude-code-plugins@${tip}`); + # The legs' review handoffs, from this run only: with no github-token + # or run-id the download spends no REST API request. + - name: Download the review handoffs + if: >- + always() + && (needs.review-key.outputs.handoff-sha256 != '' + || needs.review-subscription.outputs.handoff-sha256 != '') + continue-on-error: true + timeout-minutes: 2 + uses: actions/download-artifact@9000827ccba6bdab643e8b6fd33ac0654aef8333 # v8.0.2 + with: + pattern: pr-review-cli-handoff-${{ inputs.lane }}-*-${{ github.run_id }}-${{ github.event.pull_request.head.sha }} + path: ${{ runner.temp }}/review-handoff + merge-multiple: true + + # A leg's file counts only when its sha256 is the one that leg's job + # output names; any other file is refused, and publish then finds no + # review from that leg. + - name: Check the review handoffs + id: handoffs + if: always() + continue-on-error: true + timeout-minutes: 1 + uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 + env: + HANDOFF_DIR: ${{ runner.temp }}/review-handoff + KEY_SHA256: ${{ needs.review-key.outputs.handoff-sha256 }} + SUB_SHA256: ${{ needs.review-subscription.outputs.handoff-sha256 }} + with: + script: | + const fs = require("node:fs"); + const env = process.env; + for (const [leg, prefix, expected] of [ + ["review-key", "key", env.KEY_SHA256], + ["review-subscription", "sub", env.SUB_SHA256], + ]) { + if (!/^[0-9a-f]{64}$/u.test(expected ?? "")) continue; + let bytes; + try { + bytes = fs.readFileSync(require("node:path").join(env.HANDOFF_DIR, `${leg}.json`)); + } catch { + core.warning(`${leg} uploaded no review handoff this job could read`); + continue; + } + if (require("node:crypto").createHash("sha256").update(bytes).digest("hex") !== expected) { + core.warning(`${leg}'s review handoff does not match the sha256 that job reported; it is refused`); + continue; + } + const data = JSON.parse(bytes.toString("utf8")); + if (typeof data?.review === "string") core.setOutput(`${prefix}-review`, data.review); + if (prefix === "key" && typeof data?.["last-summary"] === "string") core.setOutput("last-summary", data["last-summary"]); + } + - name: Publish the review continue-on-error: ${{ env.PUBLISH != 'comments' }} timeout-minutes: 5 @@ -2254,7 +2389,7 @@ jobs: SCOPE: ${{ needs.review-key.outputs.scope }} MARKER_ID: ${{ needs.review-key.outputs.marker-id }} MERGE_BASE: ${{ needs.review-key.outputs.merge-base }} - LAST_SUMMARY: ${{ needs.review-key.outputs.last-summary }} + LAST_SUMMARY: ${{ steps.handoffs.outputs.last-summary }} PR_FILES: ${{ needs.review-key.outputs.pr-files }} KEY_LANE_CLASS: ${{ needs.review-key.outputs.lane-class }} SUB_RESULT: ${{ needs.review-subscription.result }} @@ -2262,7 +2397,7 @@ jobs: KEY_FAILED: ${{ needs.review-key.outputs.review-failed }} KEY_CLASS: ${{ needs.review-key.outputs.failure-class }} KEY_VERDICT: ${{ needs.review-key.outputs.verdict }} - KEY_REVIEW: ${{ needs.review-key.outputs.review }} + KEY_REVIEW: ${{ steps.handoffs.outputs.key-review }} KEY_TURNS: ${{ needs.review-key.outputs.turns }} KEY_MODELS: ${{ needs.review-key.outputs.models }} KEY_COST: ${{ needs.review-key.outputs.cost }} @@ -2272,7 +2407,7 @@ jobs: SUB_FAILED: ${{ needs.review-subscription.outputs.review-failed }} SUB_CLASS: ${{ needs.review-subscription.outputs.failure-class }} SUB_VERDICT: ${{ needs.review-subscription.outputs.verdict }} - SUB_REVIEW: ${{ needs.review-subscription.outputs.review }} + SUB_REVIEW: ${{ steps.handoffs.outputs.sub-review }} SUB_TURNS: ${{ needs.review-subscription.outputs.turns }} SUB_MODELS: ${{ needs.review-subscription.outputs.models }} SUB_COST: ${{ needs.review-subscription.outputs.cost }} @@ -2304,12 +2439,12 @@ jobs: KEY_FAILED: ${{ needs.review-key.outputs.review-failed }} KEY_CLASS: ${{ needs.review-key.outputs.failure-class }} KEY_VERDICT: ${{ needs.review-key.outputs.verdict }} - KEY_REVIEW: ${{ needs.review-key.outputs.review }} + KEY_REVIEW: ${{ steps.handoffs.outputs.key-review }} SUB_PRESENT: ${{ needs.review-subscription.outputs.credential-present }} SUB_FAILED: ${{ needs.review-subscription.outputs.review-failed }} SUB_CLASS: ${{ needs.review-subscription.outputs.failure-class }} SUB_VERDICT: ${{ needs.review-subscription.outputs.verdict }} - SUB_REVIEW: ${{ needs.review-subscription.outputs.review }} + SUB_REVIEW: ${{ steps.handoffs.outputs.sub-review }} - name: Upload the shadow report if: inputs.publish != 'comments'