diff --git a/.github/artifact-retention.json b/.github/artifact-retention.json index c4489def028..f794bd585fa 100644 --- a/.github/artifact-retention.json +++ b/.github/artifact-retention.json @@ -358,6 +358,17 @@ ".github/workflows/maintenance-reconcile-lost-runs.yml#reconcile" ] }, + "key-credit-stop-*": { + "retention-days": 1, + "readers": [ + ".github/workflows/pr-review-main.yml review-handoffs-credit, through ci-workflows check-key-credit gate.cjs, in later runs inside vars.KEY_CREDIT_WINDOW_HOURS" + ], + "reason": "The gate counts a marker only inside its window, 1 hour unset, so 1 day covers it; a window over 24 hours ends at expiry. It holds only the run id the workflow writes, never model text, so it is not scanned.", + "scan": "none", + "uploads": [ + ".github/workflows/pr-review-main.yml#review-handoffs" + ] + }, "reconcile-settled-runs": { "retention-days": 1, "readers": [ diff --git a/.github/scripts/pr-review-main/workflow.test.mjs b/.github/scripts/pr-review-main/workflow.test.mjs index 49ef47c1d82..3294e0dbdb7 100644 --- a/.github/scripts/pr-review-main/workflow.test.mjs +++ b/.github/scripts/pr-review-main/workflow.test.mjs @@ -126,11 +126,14 @@ test("the model's jobs can only read, and the one writer runs no model", () => { assert.ok(review.length > 0, job); for (const [line, , level] of review) assert.equal(level, "read", `${job}: ${line}`); } - // The routes between legs hold no grant, no secret and no model. + // The routes between legs hold no grant, no secret and no model; the one + // action is review-handoffs' credit-stop marker upload, on the runtime token. for (const job of ["review-handoffs", "subscription-handoffs"]) { const route = jobText(job); assert.match(route, /^ {4}permissions: \{\}$/m, job); - assert.doesNotMatch(route, /run-claude-cli|secrets\.|actions\/checkout@|uses:/, job); + assert.doesNotMatch(route, /run-claude-cli|secrets\.|actions\/checkout@/, job); + const uses = route.match(/uses: .*/g) ?? []; + assert.deepEqual(uses.map((line) => line.replace(/@.*/, "")), job === "review-handoffs" ? ["uses: actions/upload-artifact"] : [], job); } // The trigger job reads with the key leg's grants and checks: read, for the // record job's check runs its judged-head read takes; it holds no secret @@ -852,7 +855,8 @@ test("the prompt names the per-concern review rules the criteria step copies", ( }); test("the verdict uploads once, after it is written, under the writer's name", () => { - assert.equal(text.match(/uses: actions\/upload-artifact@/g).length, LEGS.length); + // Each leg's verdict, plus review-handoffs' credit-stop marker. + assert.equal(text.match(/uses: actions\/upload-artifact@/g).length, LEGS.length + 1); for (const job of LEGS) { const uploads = STEPS[job].filter((s) => /uses: actions\/upload-artifact@/.test(s.block)); assert.equal(uploads.length, 1, job); @@ -1058,8 +1062,7 @@ test("the credit gate skips the key leg only on skip-key true, and a gate failur assert.ok(!runs({ result: "failure", outputs: {} }, { ...triggered, result: "failure" })); assert.ok(!runs({ result: "success", outputs: {} }, triggered, { cancelled: true })); // The gate runs on every run of this attempt's trigger job, a head with no - // lane source or one already judged included, so each completed run the - // gate reads next (only the newest few) carries the record forward. + // lane source or one already judged included. const probeRuns = (trigger) => admits(jobGate(PROBE), { github: upstreamRun, needs: { trigger } }); for (const outputs of [{}, { proceed: "" }, { review: "false" }, { judged: "true" }]) { assert.ok(probeRuns({ ...triggered, outputs: { ...triggered.outputs, ...outputs } }), JSON.stringify(outputs)); @@ -1068,19 +1071,19 @@ test("the credit gate skips the key leg only on skip-key true, and a gate failur assert.ok(!admits(jobGate(PROBE), { github: { ...upstreamRun, ref: "refs/heads/x" }, needs: { trigger: triggered } })); }); -test("the credit gate holds no secret, checks nothing out and reads only this workflow's runs", () => { +test("the credit gate holds no secret, checks nothing out and reads only the marker artifacts", () => { const job = jobText(PROBE); assert.equal(needsOf(PROBE), "trigger"); - assert.match(job, /^ {4}permissions:\n {6}actions: read #.*\n {4}outputs:\n {6}skip-key: \$\{\{ steps\.gate\.outputs\.skip-key \}\}\n/m); + assert.match(job, /^ {4}permissions:\n {6}actions: read #.*\n {4}outputs:\n {6}skip-key: \$\{\{ steps\.gate\.outputs\.skip-key \}\}\n {6}marker: \$\{\{ steps\.gate\.outputs\.marker \}\}\n/m); assert.doesNotMatch(job, /secrets\.|actions\/checkout|run-claude-cli|\.\/\.base/); const gate = step("Read the key's credit history", PROBE); assert.match(gate, /^ {8}id: gate$/m); assert.match(gate, /^ {8}continue-on-error: true$/m); - assert.match(gate, /^ {8}uses: melodic-software\/ci-workflows\/\.github\/actions\/check-key-credit@[0-9a-f]{40} # \S/m); - // The gate reads its record in `` and `-credit` by display - // name: review-handoffs and this job, neither renamed. + // The marker gate (ci-workflows#973); an older pin reads run history for a + // record step this workflow no longer writes, so it would never skip. + assert.match(gate, /^ {8}uses: melodic-software\/ci-workflows\/\.github\/actions\/check-key-credit@bc62beb95a989641f30552a932a132ebb8ecd801 # \S/m); + // The marker's name carries key-job: review-handoffs, the job that uploads it. assert.equal(input(gate, "key-job"), "review-handoffs"); - for (const id of [PROBE, "review-handoffs"]) assert.doesNotMatch(jobText(id), /^ {4}name:/m, id); assert.equal(input(gate, "window-hours"), "${{ vars.KEY_CREDIT_WINDOW_HOURS || '1' }}"); // Each skipped key leg leaves its lane record, so leg counts compare. for (const [lane, sw] of [["claude-review", "CLAUDE_REVIEW_DISABLED"], ["claude-security-review", "CLAUDE_SECURITY_REVIEW_DISABLED"]]) { @@ -1094,20 +1097,29 @@ test("the credit gate holds no secret, checks nothing out and reads only this wo } }); -// check-key-credit RECORD_PATTERN: /^Record the key's credit stop in run (\d{1,20})$/. -const RECORD_PATTERN = /^Record the key's credit stop in run (\d{1,20})$/u; -test("the record steps carry the pattern the gate reads, the key's only after a key leg's credit stop", () => { - const carried = "Record the key's credit stop in run ${{ steps.gate.outputs.origin-run }}"; - const own = "Record the key's credit stop in run ${{ github.run_id }}"; - assert.match(carried.replace("${{ steps.gate.outputs.origin-run }}", "12345"), RECORD_PATTERN); - assert.match(own.replace("${{ github.run_id }}", "12345"), RECORD_PATTERN); - assert.match(step(carried, PROBE), /^ {8}if: steps\.gate\.outputs\.skip-key == 'true'$/m); +// check-key-credit gate.cjs lists only artifacts named by its `marker` output, +// so review-handoffs must upload that artifact after a key leg's credit stop. +test("review-handoffs uploads the gate's marker only after a key leg's credit stop", () => { + assert.match(needsOf("review-handoffs"), new RegExp(`\\b${PROBE}\\b`)); + const write = step("Write the key's credit-stop marker", "review-handoffs"); + assert.match(write, /^ {8}id: credit-marker$/m); // The key leg's own handoff reason, never the route's: a lane the gate handed // on must not restart the window, or the key would never be tried again. - const record = step(own, "review-handoffs"); - assert.match(record, /^ {8}if: \$\{\{ !cancelled\(\) && \(needs\.review\.outputs\.claude-review == 'credit-stop' \|\| needs\.review\.outputs\.claude-security-review == 'credit-stop'\) \}\}$/m); - assert.match(record, /^ {8}continue-on-error: true$/m); - for (const job of LEGS) assert.equal(index(own, job), -1, job); + assert.match(write, /^ {8}if: \$\{\{ !cancelled\(\) && needs\.review-handoffs-credit\.outputs\.marker != '' && \(needs\.review\.outputs\.claude-review == 'credit-stop' \|\| needs\.review\.outputs\.claude-security-review == 'credit-stop'\) \}\}$/m); + assert.match(write, /^ {8}continue-on-error: true$/m); + assert.match(write, /echo "\$GITHUB_RUN_ID" >"\$RUNNER_TEMP\/key-credit-stop\/run-id"/); + const upload = step("Upload the key's credit-stop marker", "review-handoffs"); + assert.ok(index("Upload the key's credit-stop marker", "review-handoffs") > index("Write the key's credit-stop marker", "review-handoffs")); + assert.match(upload, /^ {8}if: \$\{\{ !cancelled\(\) && steps\.credit-marker\.outcome == 'success' \}\}$/m); + assert.match(upload, /^ {8}continue-on-error: true$/m); + assert.match(upload, /^ {8}uses: actions\/upload-artifact@[0-9a-f]{40} # \S/m); + assert.equal(input(upload, "name"), "${{ needs.review-handoffs-credit.outputs.marker }}"); + assert.equal(input(upload, "path"), "${{ runner.temp }}/key-credit-stop/run-id"); + assert.equal(input(upload, "if-no-files-found"), "error"); + assert.equal(input(upload, "retention-days"), "1"); + // The run-history record steps the old gate read are gone everywhere. + assert.doesNotMatch(text, /Record the key's credit stop/); + for (const job of LEGS) assert.equal(index("Upload the key's credit-stop marker", job), -1, job); }); test("with the key leg skipped by the credit gate, every enabled lane goes to the subscription leg as a credit stop", () => { diff --git a/.github/workflows/pr-review-main.yml b/.github/workflows/pr-review-main.yml index e0491574ee0..d43a4de849f 100644 --- a/.github/workflows/pr-review-main.yml +++ b/.github/workflows/pr-review-main.yml @@ -338,17 +338,14 @@ jobs: # out of credit: a key-leg credit stop in this workflow inside the window # (vars.KEY_CREDIT_WINDOW_HOURS, 1 hour unset) skips the review job, and # review-handoffs hands both lanes to review-subscription as credit-stop - # (ci-workflows check-key-credit, gate.cjs). The key's record is in - # review-handoffs, the job that routes a key leg's credit stop, so the - # gate reads that job and this one, `review-handoffs-credit`. Its own - # record carries the origin run forward in its name, so the window counts + # (ci-workflows check-key-credit, gate.cjs). review-handoffs, the job that + # routes a key leg's credit stop, uploads the artifact the gate's `marker` + # output names, and the gate lists that one name, so the window counts # from the credit stop, not the last skip, and a topped-up or rotated key - # is tried once it ends. It runs on every run of this attempt's trigger - # job, lane source or not, so each run the gate reads carries the record; - # only a run whose trigger job starts the key legs reports them skipped or - # hands them on. It holds no secret, checks nothing out and reads only this - # workflow's runs. Every failure runs the key: the review job's `if` starts - # with !cancelled(). + # is tried once it ends. Only a run whose trigger job starts the key legs + # reports them skipped or hands them on. It holds no secret, checks + # nothing out and reads only this repository's marker artifacts. Every + # failure runs the key: the review job's `if` starts with !cancelled(). review-handoffs-credit: needs: trigger if: >- @@ -361,27 +358,20 @@ jobs: timeout-minutes: 10 cache-mode: none permissions: - actions: read # this workflow's runs and their jobs + actions: read # this repository's credit-stop marker artifacts outputs: skip-key: ${{ steps.gate.outputs.skip-key }} + marker: ${{ steps.gate.outputs.marker }} steps: - name: Read the key's credit history id: gate continue-on-error: true timeout-minutes: 2 - uses: melodic-software/ci-workflows/.github/actions/check-key-credit@2476d5dde8979eab34374150b7e4b0a2b99f4e88 # 2476d5d 2026-10-29 + uses: melodic-software/ci-workflows/.github/actions/check-key-credit@bc62beb95a989641f30552a932a132ebb8ecd801 # bc62beb 2026-11-03 with: key-job: review-handoffs window-hours: ${{ vars.KEY_CREDIT_WINDOW_HOURS || '1' }} - # Read by the next runs' gate; its name is the record (check-key-credit - # RECORD_PATTERN). - - name: Record the key's credit stop in run ${{ steps.gate.outputs.origin-run }} - if: steps.gate.outputs.skip-key == 'true' - continue-on-error: true - timeout-minutes: 1 - run: echo "carried to the next run's check-key-credit read" - # Each skipped key leg's lane record: outcome skip, class credit-stop, # handed off, so leg counts before and after the gate compare. - name: Report the skipped claude-review key leg @@ -1272,15 +1262,31 @@ jobs: echo "matrix=$matrix" >>"$GITHUB_OUTPUT" echo "handed on: $(jq -r '[.include[].lane] | join(", ")' <<<"$matrix")" - # Read by the next runs' review-handoffs-credit gate: a step named for - # this run that succeeds only when a key leg that ran handed on with a - # credit stop, never on a lane the gate itself handed on. Its name is - # the record; keep it equal to check-key-credit's RECORD_PATTERN. - - name: Record the key's credit stop in run ${{ github.run_id }} - if: ${{ !cancelled() && (needs.review.outputs.claude-review == 'credit-stop' || needs.review.outputs.claude-security-review == 'credit-stop') }} + # Read by the next runs' review-handoffs-credit gate: the artifact its + # `marker` output names (gate.cjs markerName), written only when a key + # leg that ran handed on with a credit stop, never on a lane the gate + # itself handed on. Its upload time starts the window; one day's + # retention covers the 1-hour default. It holds only this run's id, so + # no leak scan runs over it (.github/artifact-retention.json). + - name: Write the key's credit-stop marker + id: credit-marker + if: ${{ !cancelled() && needs.review-handoffs-credit.outputs.marker != '' && (needs.review.outputs.claude-review == 'credit-stop' || needs.review.outputs.claude-security-review == 'credit-stop') }} + continue-on-error: true + timeout-minutes: 1 + run: | + mkdir -p "$RUNNER_TEMP/key-credit-stop" + echo "$GITHUB_RUN_ID" >"$RUNNER_TEMP/key-credit-stop/run-id" + + - name: Upload the key's credit-stop marker + if: ${{ !cancelled() && steps.credit-marker.outcome == 'success' }} continue-on-error: true timeout-minutes: 1 - run: echo "the key ended in a credit stop" + uses: actions/upload-artifact@cf430e030ddbb5b0abf93d22962f4752f3646cd9 # v7.0.2 + with: + name: ${{ needs.review-handoffs-credit.outputs.marker }} + path: ${{ runner.temp }}/key-credit-stop/run-id + if-no-files-found: error + retention-days: 1 # The subscription leg, one per lane the key leg handed on, on a new VM: # the key leg's processes, files and environment do not reach it. Its steps