diff --git a/.github/actions/check-key-credit/action.yml b/.github/actions/check-key-credit/action.yml new file mode 100644 index 00000000..6617325d --- /dev/null +++ b/.github/actions/check-key-credit/action.yml @@ -0,0 +1,46 @@ +name: Key credit gate +description: > + Before a keyed lane's key job: whether that job should skip its key because + the key ended in a credit stop in this workflow inside the window. Reads + the workflow's completed runs created inside the window (one list call) + and at most three of their job lists, with the job token, which needs + actions: read. Any failure or an unclear answer leaves skip-key `false`, + so the key job runs as before. Writes one log line either way; a skip is a + notice naming the run whose credit stop caused it. See gate.cjs. + +inputs: + key-job: + description: >- + The key job's display name, lower-case kebab (for example + claude-review-key). The record steps are looked up in this job and in + `-credit`, the job that runs this action. + required: true + window-hours: + description: How far back a credit stop counts, in hours (0 turns the gate off; at most 168). + required: false + default: "6" + github-token: + description: "The job token, with actions: read." + required: false + default: ${{ github.token }} + +outputs: + skip-key: + description: "`true` when the key job should skip its key, else `false` or empty." + value: ${{ steps.gate.outputs.skip-key }} + origin-run: + description: The run id whose credit stop caused the skip, or empty. + value: ${{ steps.gate.outputs.origin-run }} + +runs: + using: composite + steps: + - name: Read the key's credit history + id: gate + shell: bash + env: + KEY_JOB: ${{ inputs.key-job }} + WINDOW_HOURS: ${{ inputs.window-hours }} + WORKFLOW_REF: ${{ github.workflow_ref }} + GITHUB_TOKEN: ${{ inputs.github-token }} + run: node "$GITHUB_ACTION_PATH/gate.cjs" || true diff --git a/.github/actions/check-key-credit/gate.cjs b/.github/actions/check-key-credit/gate.cjs new file mode 100644 index 00000000..d6c42e9d --- /dev/null +++ b/.github/actions/check-key-credit/gate.cjs @@ -0,0 +1,241 @@ +// check-key-credit: whether a keyed lane's key job should skip the model +// because the key ended in a credit stop inside the window. +// +// It reads the calling workflow's completed runs created inside the window +// (one list call) and, newest first, at most MAX_JOB_READS of their job lists +// (one call each), and stops at the first run that settles the question: +// - the key job or this gate's job has a successful step whose name is +// RECORD_STEP and an origin run id that is one of the listed runs: skip, +// citing that origin; +// - that record names a run outside the list: the credit stop expired, run; +// - the key job's model step (MODEL_STEP) ran with no record: the key ran +// with no credit stop, run. +// A run that settles nothing (cancelled, no key job, no review needed) is +// passed over. Any failure, an unreadable answer or no settling run within +// the cap runs the key, as before this gate: it never skips the subscription +// path and never fails the step. +// +// The record step runs in the key job only after the launcher's lane-class +// `credit-stop` (run-claude-cli: classify.cjs isCreditStop), so an auth stop +// (401, or 402 without the credit text) never counts, and names its own run. +// When this gate skips the key, its job's record step names the origin run +// instead, so the window runs from the credit stop itself, not from the +// last skip. A same-repository pull request that edits the caller workflow +// could write a record; the worst it does is move reviews to the +// subscription for the window, and such writers already hold the secrets. +// More than 100 completed runs in the window can push the origin off the +// list, which runs the key. +"use strict"; + +const fs = require("node:fs"); +const path = require("node:path"); + +const RECORD_STEP = "Record the key's credit stop in run "; +const RECORD_PATTERN = /^Record the key's credit stop in run (\d{1,20})$/u; +const MODEL_STEP = "Claude review"; +const MAX_JOB_READS = 3; +const MAX_WINDOW_HOURS = 168; +const WORKFLOW_REF = /^[\w.-]+\/[\w.-]+\/\.github\/workflows\/([\w.-]+\.ya?ml)@/u; + +// The rate-limit reading report-api-budget reads, as the scope step writes +// it. A failed write only drops the reading. +function recordBudget(response, runnerTemp) { + if (!runnerTemp) return; + const values = ["limit", "remaining", "used", "reset", "resource"].map( + (key) => { + const value = String(response.headers?.get?.(`x-ratelimit-${key}`) ?? ""); + return (key === "resource" ? /^[a-z_]+$/u : /^\d+$/u).test(value) + ? value + : "na"; + }, + ); + try { + const dir = path.join(runnerTemp, "api-budget"); + fs.mkdirSync(dir, { recursive: true }); + fs.appendFileSync( + path.join(dir, "github-token"), + `${response.status} ${values.join(" ")}\n`, + ); + } catch { + // No reading: the notice is only missing one. + } +} + +// The job named `name` in one run's job list: exactly, or ` / +// name` when a caller's job runs the reusable. +function jobNamed(jobs, name) { + return jobs.find( + (job) => + typeof job?.name === "string" && + job.status === "completed" && + Array.isArray(job.steps) && + (job.name === name || job.name.endsWith(` / ${name}`)), + ); +} + +// What one run's job list settles: { skip, origin } or { skip: false, +// reason }, or null when it settles nothing. The record is the key job's +// own, or the gate job's (`-credit`) carried from an earlier run. +function judgeRun(jobs, keyJob, windowIds) { + const list = Array.isArray(jobs) ? jobs : []; + const key = jobNamed(list, keyJob); + const gate = jobNamed(list, `${keyJob}-credit`); + for (const step of [...(gate?.steps ?? []), ...(key?.steps ?? [])]) { + const match = + step?.conclusion === "success" && typeof step.name === "string" + ? step.name.match(RECORD_PATTERN) + : null; + if (!match) continue; + return windowIds.has(match[1]) + ? { skip: true, origin: match[1] } + : { + skip: false, + reason: `the credit stop in run ${match[1]} is outside the window`, + }; + } + const model = key?.steps.find((step) => step?.name === MODEL_STEP); + if (model?.conclusion === "success" || model?.conclusion === "failure") { + return { skip: false, reason: "the key ran with no credit stop" }; + } + return null; +} + +// The decision, from injected readers; never throws. +async function decide({ + listRuns, + listJobs, + currentRunId, + keyJob, + maxJobReads = MAX_JOB_READS, +}) { + let runs; + try { + runs = await listRuns(); + } catch (error) { + return { skip: false, reason: `the run list was not read (${error.message})` }; + } + if (!Array.isArray(runs)) { + return { skip: false, reason: "the run list was not a list" }; + } + const candidates = runs.filter( + (run) => Number.isSafeInteger(run?.id) && String(run.id) !== currentRunId, + ); + const windowIds = new Set(candidates.map((run) => String(run.id))); + for (const run of candidates.slice(0, maxJobReads)) { + let jobs; + try { + jobs = await listJobs(run.id); + } catch (error) { + return { + skip: false, + reason: `run ${run.id}'s jobs were not read (${error.message})`, + }; + } + const verdict = judgeRun(jobs, keyJob, windowIds); + if (verdict) return { ...verdict, run: String(run.id) }; + } + return { + skip: false, + reason: + candidates.length === 0 + ? "no completed run in the window" + : `no run of the newest ${Math.min(candidates.length, maxJobReads)} settles it`, + }; +} + +function readEnv(env) { + const keyJob = env.KEY_JOB ?? ""; + const hours = Number(env.WINDOW_HOURS); + const workflow = (env.WORKFLOW_REF ?? "").match(WORKFLOW_REF)?.[1] ?? null; + const repository = env.GITHUB_REPOSITORY ?? ""; + return { + keyJob, + hours, + workflow, + repository, + api: env.GITHUB_API_URL || "https://api.github.com", + server: env.GITHUB_SERVER_URL || "https://github.com", + runId: env.GITHUB_RUN_ID ?? "", + token: env.GITHUB_TOKEN ?? "", + runnerTemp: env.RUNNER_TEMP ?? "", + valid: + /^[a-z][a-z0-9-]{0,99}$/u.test(keyJob) && + /^[\w.-]+\/[\w.-]+$/u.test(repository) && + workflow !== null && + /^\d{1,20}$/u.test(env.GITHUB_RUN_ID ?? ""), + }; +} + +async function main(env = process.env, fetchImpl = globalThis.fetch, now = () => new Date()) { + const config = readEnv(env); + const lines = []; + let verdict; + if (!(config.hours > 0 && config.hours <= MAX_WINDOW_HOURS)) { + verdict = { skip: false, reason: "the window is off" }; + } else if (!config.valid || config.token === "") { + verdict = { skip: false, reason: "the run, workflow, job or token is unreadable" }; + } else { + const get = async (url) => { + const response = await fetchImpl(url, { + headers: { + Accept: "application/vnd.github+json", + Authorization: `Bearer ${config.token}`, + "X-GitHub-Api-Version": "2022-11-28", + }, + signal: AbortSignal.timeout(15000), + }); + recordBudget(response, config.runnerTemp); + if (!response.ok) throw new Error(`HTTP ${response.status}`); + return response.json(); + }; + const since = new Date(now().getTime() - config.hours * 3600 * 1000) + .toISOString() + .replace(/\.\d{3}Z$/u, "Z"); + const base = `${config.api}/repos/${config.repository}/actions`; + verdict = await decide({ + currentRunId: config.runId, + keyJob: config.keyJob, + listRuns: async () => + ( + await get( + `${base}/workflows/${config.workflow}/runs?status=completed&exclude_pull_requests=true&per_page=100&created=${encodeURIComponent(`>=${since}`)}`, + ) + )?.workflow_runs, + listJobs: async (id) => + (await get(`${base}/runs/${id}/jobs?filter=latest&per_page=100`))?.jobs, + }); + } + if (verdict.skip) { + lines.push( + `::notice title=check-key-credit::${config.keyJob} skips the key: the key's credit stop in run ${verdict.origin} (${config.server}/${config.repository}/actions/runs/${verdict.origin}) is inside the ${config.hours}-hour window; the subscription job reviews`, + ); + } else { + lines.push(`check-key-credit: ${config.keyJob || "the key job"} runs the key: ${verdict.reason}`); + } + if (env.GITHUB_OUTPUT) { + fs.appendFileSync( + env.GITHUB_OUTPUT, + `skip-key=${verdict.skip}\norigin-run=${verdict.skip ? verdict.origin : ""}\n`, + ); + } + return { verdict, lines }; +} + +if (require.main === module) { + main() + .then(({ lines }) => process.stdout.write(`${lines.join("\n")}\n`)) + .catch(() => { + // Fail open: no output leaves skip-key empty, so the key runs. + process.stdout.write("check-key-credit: the check failed; the key runs\n"); + }); +} + +module.exports = { + MAX_JOB_READS, + MODEL_STEP, + RECORD_PATTERN, + RECORD_STEP, + decide, + judgeRun, + main, +}; diff --git a/.github/scripts/check-key-credit.test.cjs b/.github/scripts/check-key-credit.test.cjs new file mode 100644 index 00000000..b06a2220 --- /dev/null +++ b/.github/scripts/check-key-credit.test.cjs @@ -0,0 +1,263 @@ +"use strict"; + +// check-key-credit reads a keyed lane's own recent runs and skips the key job +// only on a credit-stop record inside the window; every other answer, and +// every failure, runs the key. Its calls: one run list, then at most +// MAX_JOB_READS job lists. + +const assert = require("node:assert/strict"); +const fs = require("node:fs"); +const path = require("node:path"); +const test = require("node:test"); + +const { + MAX_JOB_READS, + decide, + judgeRun, + main, +} = require("../actions/check-key-credit/gate.cjs"); +const { makeTemporaryDirectory } = require("./temporary-directories.cjs"); + +const KEY = "claude-review-key"; +const step = (name, conclusion = "success") => ({ name, conclusion }); +const keyJob = (steps, name = `pr-review / ${KEY}`) => ({ + name, + status: "completed", + steps, +}); +const RAN = step("Claude review", "failure"); +const credit = (id) => step(`Record the key's credit stop in run ${id}`); + +test("a run settles on a record in the window, a record outside it, or a key that ran", () => { + const ids = new Set(["10", "11"]); + assert.deepEqual(judgeRun([keyJob([RAN, credit(10)])], KEY, ids), { + skip: true, + origin: "10", + }); + // The gate job's carried record counts the same. + assert.deepEqual( + judgeRun( + [keyJob([credit(11)], `pr-review / ${KEY}-credit`), keyJob([], "x")], + KEY, + ids, + ), + { skip: true, origin: "11" }, + ); + assert.equal(judgeRun([keyJob([credit(9)])], KEY, ids).skip, false); + assert.equal(judgeRun([keyJob([RAN])], KEY, ids).skip, false); + assert.equal( + judgeRun([keyJob([step("Claude review", "success")])], KEY, ids).skip, + false, + ); +}); + +test("a run settles nothing without a finished key job whose model or record ran", () => { + const ids = new Set(["10"]); + for (const jobs of [ + [], + null, + [keyJob([step("Claude review", "skipped")])], + [{ ...keyJob([RAN]), status: "in_progress" }], + // A record that did not succeed, and another lane's job. + [keyJob([credit(10).name].map((name) => step(name, "skipped")))], + [keyJob([RAN], "pr-review / security-review-key")], + [keyJob([RAN], `pr-review / not-${KEY}`)], + // The jobs API sometimes returns every job of a run with no steps. + [keyJob([]), keyJob([], `pr-review / ${KEY}-credit`)], + [{ name: `pr-review / ${KEY}`, status: "completed" }], + ]) { + assert.equal(judgeRun(jobs, KEY, ids), null, JSON.stringify(jobs)); + } +}); + +test("an auth stop or any other class never reads as a credit stop", () => { + // Only the record step's exact name counts, and the workflow runs it only + // after lane-class credit-stop (pr-review-cli.test.cjs holds that). + const ids = new Set(["10"]); + for (const name of [ + "Record the key's credit stop in run ", + "Record the key's credit stop in run 10 ", + "Record the key's auth stop in run 10", + `Record the key's credit stop in run \${{ github.run_id }}`, + ]) { + assert.equal( + judgeRun([keyJob([RAN, step(name)])], KEY, ids).skip, + false, + name, + ); + } +}); + +test("decide reads newest first, skips the current run and caps its job reads", async () => { + const runs = [ + { id: 7 }, + { id: 6 }, + { id: 5 }, + { id: 4 }, + { id: 3 }, + { id: 2 }, + ]; + const read = []; + const verdict = await decide({ + currentRunId: "7", + keyJob: KEY, + listRuns: async () => runs, + listJobs: async (id) => { + read.push(id); + return id === 2 ? [keyJob([credit(2)])] : []; + }, + }); + assert.deepEqual(read, [6, 5, 4]); + assert.equal(read.length, MAX_JOB_READS); + assert.equal(verdict.skip, false); + + const read2 = []; + const skip = await decide({ + currentRunId: "9", + keyJob: KEY, + listRuns: async () => runs, + listJobs: async (id) => { + read2.push(id); + return id === 6 ? [keyJob([credit(3)], `r / ${KEY}-credit`)] : []; + }, + }); + assert.deepEqual(skip, { skip: true, origin: "3", run: "6" }); + assert.deepEqual(read2, [7, 6]); +}); + +test("runs whose jobs all come back with empty steps run the key after the cap", async () => { + const read = []; + const verdict = await decide({ + currentRunId: "9", + keyJob: KEY, + listRuns: async () => [{ id: 8 }, { id: 7 }, { id: 6 }, { id: 5 }], + listJobs: async (id) => { + read.push(id); + return [keyJob([]), keyJob([], `r / ${KEY}-credit`)]; + }, + }); + assert.equal(verdict.skip, false); + assert.deepEqual(read, [8, 7, 6]); +}); + +test("decide fails open on every read failure", async () => { + const boom = async () => { + throw new Error("HTTP 403"); + }; + for (const args of [ + { listRuns: boom, listJobs: async () => [] }, + { listRuns: async () => ({}), listJobs: async () => [] }, + { listRuns: async () => [{ id: 1 }], listJobs: boom }, + { listRuns: async () => [], listJobs: async () => [] }, + ]) { + const verdict = await decide({ currentRunId: "9", keyJob: KEY, ...args }); + assert.equal(verdict.skip, false); + } +}); + +function env(dir, extra = {}) { + return { + KEY_JOB: KEY, + WINDOW_HOURS: "6", + WORKFLOW_REF: + "o/r/.github/workflows/pr-review-hosted.yml@refs/pull/1/merge", + GITHUB_REPOSITORY: "o/r", + GITHUB_RUN_ID: "100", + GITHUB_TOKEN: "t", + GITHUB_API_URL: "https://api.example", + GITHUB_SERVER_URL: "https://example", + GITHUB_OUTPUT: path.join(dir, "output"), + RUNNER_TEMP: dir, + ...extra, + }; +} + +function fakeFetch(routes) { + const urls = []; + const fetchImpl = async (url, init) => { + urls.push(url); + assert.equal(init.headers.Authorization, "Bearer t"); + const [, body] = routes.find(([pattern]) => pattern.test(url)) ?? []; + const headers = new Map([ + ["x-ratelimit-limit", "1000"], + ["x-ratelimit-remaining", String(1000 - urls.length)], + ["x-ratelimit-used", String(urls.length)], + ["x-ratelimit-reset", "1"], + ["x-ratelimit-resource", "core"], + ]); + return { + ok: body !== undefined, + status: body === undefined ? 404 : 200, + headers, + json: async () => body, + }; + }; + return { urls, fetchImpl }; +} + +test("main skips on a credit stop in the window with one list call and one jobs call, and says which run", async () => { + const dir = makeTemporaryDirectory("check-key-credit-"); + const { urls, fetchImpl } = fakeFetch([ + [ + /\/workflows\/pr-review-hosted\.yml\/runs\?/u, + { workflow_runs: [{ id: 99 }, { id: 98 }] }, + ], + [/\/runs\/99\/jobs\?/u, { jobs: [keyJob([RAN, credit(98)])] }], + ]); + const now = () => new Date("2026-10-24T12:00:00Z"); + const { verdict, lines } = await main(env(dir), fetchImpl, now); + assert.deepEqual(verdict, { skip: true, origin: "98", run: "99" }); + assert.equal(urls.length, 2); + assert.match(urls[0], /status=completed/u); + assert.match(urls[0], /created=%3E%3D2026-10-24T06%3A00%3A00Z/u); + assert.equal(lines.length, 1); + assert.match( + lines[0], + /^::notice title=check-key-credit::claude-review-key skips the key: the key's credit stop in run 98 \(https:\/\/example\/o\/r\/actions\/runs\/98\)/u, + ); + assert.equal( + fs.readFileSync(path.join(dir, "output"), "utf8"), + "skip-key=true\norigin-run=98\n", + ); + // Each response's rate-limit reading, for report-api-budget. + assert.equal( + fs.readFileSync(path.join(dir, "api-budget", "github-token"), "utf8"), + "200 1000 999 1 1 core\n200 1000 998 2 1 core\n", + ); +}); + +test("main runs the key on a refused read, an unreadable ref or a window of 0, spending nothing on the last two", async () => { + for (const [extra, calls] of [ + [{}, 1], + [{ WINDOW_HOURS: "0" }, 0], + [{ WINDOW_HOURS: "abc" }, 0], + [{ WORKFLOW_REF: "nonsense" }, 0], + [{ GITHUB_TOKEN: "" }, 0], + ]) { + const dir = makeTemporaryDirectory("check-key-credit-"); + const { urls, fetchImpl } = fakeFetch([]); + const { verdict, lines } = await main(env(dir, extra), fetchImpl); + assert.equal(verdict.skip, false, JSON.stringify(extra)); + assert.equal(urls.length, calls, JSON.stringify(extra)); + assert.match(lines[0], /^check-key-credit: .* runs the key: /u); + assert.equal( + fs.readFileSync(path.join(dir, "output"), "utf8"), + "skip-key=false\norigin-run=\n", + ); + } +}); + +test("the action passes the caller's workflow ref and the window to the gate", () => { + const { parseWorkflow } = require("./workflow-yaml.cjs"); + const action = parseWorkflow( + fs.readFileSync( + path.join(__dirname, "..", "actions", "check-key-credit", "action.yml"), + "utf8", + ), + ); + const run = action.runs.steps[0]; + assert.equal(run.env.WORKFLOW_REF, `\${{ github.workflow_ref }}`); + assert.equal(run.env.WINDOW_HOURS, `\${{ inputs.window-hours }}`); + assert.equal(action.inputs["window-hours"].default, "6"); + assert.match(run.run, /\|\| true$/u); +}); diff --git a/.github/scripts/pr-review-cli.test.cjs b/.github/scripts/pr-review-cli.test.cjs index 862ef032..cd8c56f8 100644 --- a/.github/scripts/pr-review-cli.test.cjs +++ b/.github/scripts/pr-review-cli.test.cjs @@ -119,7 +119,9 @@ test("each secret-bearing job turns away forks, drafts, Dependabot, unlisted bot github, inputs: { "allowed-bot-sender-ids": "", ...inputs }, needs: { + "check-key-credit": { result: "success", outputs: {} }, "review-key": { + result: "success", outputs: { handoff: "true", "credential-present": "true", @@ -129,7 +131,7 @@ test("each secret-bearing job turns away forks, drafts, Dependabot, unlisted bot }, }), ); - for (const job of [key, subscription]) { + for (const job of [workflow.jobs["check-key-credit"], key, subscription]) { assert.equal(passes(job, PR), true, job.name); const refused = [ withPr((g) => { @@ -176,15 +178,32 @@ test("each secret-bearing job turns away forks, drafts, Dependabot, unlisted bot } }); -test("the subscription job starts only on an exact handoff or a key job with no key", () => { - const starts = (outputs) => +test("the subscription job starts only on an exact handoff, a key job with no key or a credit skip", () => { + const starts = (outputs, result = "success", skipKey = "") => Boolean( evaluate(subscription.if, { github: PR, inputs: { "allowed-bot-sender-ids": "" }, - needs: { "review-key": { outputs } }, + needs: { + "check-key-credit": { outputs: { "skip-key": skipKey } }, + "review-key": { result, outputs }, + }, }), ); + // A key job skipped by check-key-credit's exact `true`, and no other skip. + assert.equal(starts({}, "skipped", "true"), true); + for (const skipKey of ["", "false", "True", "true ", "1"]) { + assert.equal( + starts({}, "skipped", skipKey), + false, + JSON.stringify(skipKey), + ); + } + // A key job that failed or was cancelled starts nothing, as before. + for (const result of ["failure", "cancelled"]) { + assert.equal(starts({ handoff: "true" }, result, "true"), false, result); + } + assert.deepEqual(subscription.needs, ["check-key-credit", "review-key"]); assert.equal(starts({ handoff: "true", "credential-present": "true" }), true); assert.equal(starts({ handoff: "", "credential-present": "false" }), true); for (const handoff of ["", "false", "True", "true ", "1"]) { @@ -196,10 +215,177 @@ test("the subscription job starts only on an exact handoff or a key job with no } // A review the scope step skipped sets no credential-present at all. assert.equal(starts({ handoff: "", "credential-present": "" }), false); - assert.equal(subscription.needs, "review-key"); assert.doesNotMatch(subscription.if, /always\(\)|failure\(\)/u); }); +test("the key job runs unless check-key-credit says exactly true, a failed check-key-credit included", () => { + const runs = (skipKey, result = "success") => + Boolean( + evaluate(key.if, { + github: PR, + inputs: { "allowed-bot-sender-ids": "" }, + needs: { + "check-key-credit": { result, outputs: { "skip-key": skipKey } }, + }, + }), + ); + assert.equal(runs("true"), false); + for (const skipKey of ["", "false", "True"]) { + assert.equal(runs(skipKey), true, JSON.stringify(skipKey)); + } + assert.equal(runs("", "failure"), true); + assert.equal(runs("", "skipped"), true); + assert.equal(key.needs, "check-key-credit"); + const gate = workflow.jobs["check-key-credit"]; + assert.deepEqual(gate.permissions, { actions: "read" }); + assert.equal(gate.steps[0].uses, "$/.github/actions/check-key-credit"); + assert.equal(gate.outputs["skip-key"], `\${{ steps.gate.outputs.skip-key }}`); + assert.doesNotMatch(JSON.stringify(gate), /secrets\./u); +}); + +test("the credit record steps carry the names and conditions check-key-credit reads", () => { + const { + MODEL_STEP, + RECORD_PATTERN, + } = require("../actions/check-key-credit/gate.cjs"); + const render = (name, context) => + name.replace(/\$\{\{\s*([^}]+?)\s*\}\}/gu, (_, expr) => + String(evaluate(expr, context)), + ); + for (const job of [key, subscription]) { + const record = job.steps.find((s) => s.name.startsWith("Record the key")); + assert.match( + render(record.name, { github: { run_id: "123" } }), + RECORD_PATTERN, + ); + assert.equal(step(job, "claude").name, MODEL_STEP); + const runsAfter = (laneClass) => + Boolean( + evaluate(record.if, { + steps: { claude: { outputs: { "lane-class": laneClass } } }, + }), + ); + assert.equal(runsAfter("credit-stop"), true); + for (const laneClass of ["auth", "rate-limit", "", "other"]) { + assert.equal(runsAfter(laneClass), false, laneClass); + } + } + const carry = workflow.jobs["check-key-credit"].steps[1]; + const context = (skip) => ({ + steps: { gate: { outputs: { "skip-key": skip, "origin-run": "99" } } }, + }); + assert.match(render(carry.name, context("true")), RECORD_PATTERN); + assert.equal( + render(carry.name, context("true")).match(RECORD_PATTERN)[1], + "99", + ); + assert.equal(Boolean(evaluate(carry.if, context("true"))), true); + assert.equal(Boolean(evaluate(carry.if, context(""))), false); +}); + +test("after a credit skip the status job runs and reads the subscription job's scope", () => { + const needs = (keyResult, subResult) => ({ + "review-key": { + result: keyResult, + outputs: + keyResult === "skipped" + ? {} + : { "scope-review": "key", "pr-files": "k" }, + }, + "review-subscription": { + result: subResult, + outputs: + subResult === "skipped" + ? {} + : { "scope-review": "sub", "pr-files": "s" }, + }, + }); + const runs = (k, s) => Boolean(evaluate(status.if, { needs: needs(k, s) })); + assert.equal(runs("skipped", "success"), true); + assert.equal(runs("success", "skipped"), true); + assert.equal(runs("skipped", "skipped"), false); + for (const name of ["Publish the review", "Apply the hold"]) { + const env = status.steps.find((s) => s.name === name).env; + const read = (k, s, v) => evaluate(env[v], { needs: needs(k, s) }); + assert.equal(read("skipped", "success", "PR_FILES"), "s", name); + assert.equal(read("success", "success", "PR_FILES"), "k", name); + assert.equal(read("success", "skipped", "PR_FILES"), "k", name); + } + const publish = status.steps.find((s) => s.name === "Publish the review").env; + for (const v of [ + "SCOPE_REVIEW", + "SKIP", + "SKIP_REASON", + "SINCE", + "SCOPE", + "MARKER_ID", + "MERGE_BASE", + ]) { + assert.match( + publish[v], + /needs\.review-key\.result == 'skipped' && needs\.review-subscription\.outputs\./u, + v, + ); + } + assert.equal( + evaluate(publish.SCOPE_REVIEW, { needs: needs("skipped", "success") }), + "sub", + ); + // The subscription leg records the skip as the credit stop it follows. + assert.equal( + evaluate(step(subscription, "review-outcome").with["fallback-reason"], { + needs: { "review-key": { result: "skipped", outputs: {} } }, + }), + "credit-stop", + ); +}); + +test("the status job takes the last summary from the subscription leg when the key leg left none", async () => { + const dir = makeTemporaryDirectory("pr-review-cli-handoffs-"); + const crypto = require("node:crypto"); + const write = (leg, data) => { + const bytes = Buffer.from(JSON.stringify(data)); + fs.writeFileSync(path.join(dir, `${leg}.json`), bytes); + return crypto.createHash("sha256").update(bytes).digest("hex"); + }; + const check = status.steps.find((s) => s.id === "handoffs"); + const run = async (env) => { + const outputs = {}; + const core = { + setOutput: (k, v) => { + outputs[k] = v; + }, + warning: () => {}, + }; + const saved = { ...process.env }; + Object.assign(process.env, { + HANDOFF_DIR: dir, + KEY_SHA256: "", + SUB_SHA256: "", + ...env, + }); + try { + await new AsyncFunction("require", "core", check.with.script)( + require, + core, + ); + } finally { + for (const k of ["HANDOFF_DIR", "KEY_SHA256", "SUB_SHA256"]) { + if (k in saved) process.env[k] = saved[k]; + else delete process.env[k]; + } + } + return outputs; + }; + const sub = write("review-subscription", { "last-summary": "from sub" }); + assert.equal((await run({ SUB_SHA256: sub }))["last-summary"], "from sub"); + const keyLeg = write("review-key", { "last-summary": "from key" }); + assert.equal( + (await run({ KEY_SHA256: keyLeg, SUB_SHA256: sub }))["last-summary"], + "from key", + ); +}); + test("the subscription job repeats the key job's steps but for the credential and the fallback reason", () => { const strip = (steps, secret, input) => steps.map((s) => { diff --git a/.github/workflows/pr-review-cli.yml b/.github/workflows/pr-review-cli.yml index 743df5f1..43adacb9 100644 --- a/.github/workflows/pr-review-cli.yml +++ b/.github/workflows/pr-review-cli.yml @@ -22,6 +22,7 @@ name: pr-review-cli # # request against this grant at startup, so summary mode needs it # # too, though it writes nothing. # permissions: +# actions: read # the check-key-credit job reads this workflow's runs # contents: read # pull-requests: write # uses: melodic-software/ci-workflows/.github/workflows/pr-review-cli.yml@ @@ -48,6 +49,14 @@ name: pr-review-cli # That job runs on a fresh ubuntu-24.04 runner, repeats every hardening # step and takes no artifact or cache from the key job. Either secret # may be empty. +# - The check-key-credit job runs first, holds no secret and no write, and reads +# only this workflow's own runs with the job token (actions: read, so +# the caller grants it). When the key job ended in a credit stop +# (lane-class `credit-stop`) in a run created inside +# key-credit-window-hours, the key job is skipped and the subscription +# job reviews, its scope read by the status job in the key job's place. +# A failed or unclear read runs the key job as before +# (.github/actions/check-key-credit). # - The model jobs cannot write: contents: read and pull-requests: read, # no id-token, and `cache-mode: none`. The launcher step's env holds the # one Claude credential and no GitHub token. Before the checkout the job @@ -83,7 +92,8 @@ name: pr-review-cli # 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 +# the key job's scope step (the subscription job's when check-key-credit +# skipped the key job) read before any model step ran, used only # when a fresh read shows the head has not moved, else a read of its # own. Each step writes its own outputs file, so the later model step # cannot rewrite the scope step's outputs; the status job already trusts @@ -237,6 +247,14 @@ on: scrutiny. A match never holds the pull request by itself. type: string default: "" + key-credit-window-hours: + description: >- + Hours a key's credit stop in this workflow keeps later runs off the + key: their key job is skipped and the subscription job reviews. + The first run after the window tries the key again, so a new cycle + or a replaced key is picked up. 0 always tries the key. + type: number + default: 6 outputs: hold: description: "`true` when this run held the pull request, else `false`." @@ -258,11 +276,70 @@ permissions: contents: read jobs: + # Whether the key is still out of credits: a key-job credit stop in this + # workflow inside the window skips the key job. Its record step carries the + # origin run forward in its name, which the next run's read finds, so the + # window counts from the credit stop, not the last skip. Every failure runs + # the key job: its `if` starts with !cancelled(). + check-key-credit: + name: ${{ inputs.lane == 'security' && 'security-review-key-credit' || 'claude-review-key-credit' }} + runs-on: ubuntu-24.04 + timeout-minutes: 10 + if: >- + github.event_name == 'pull_request' + && github.event.pull_request.state == 'open' + && github.event.pull_request.draft == false + && github.event.pull_request.head.repo.full_name == github.repository + && github.actor != 'dependabot[bot]' + && github.event.pull_request.user.login != 'dependabot[bot]' + && (!endsWith(github.actor, '[bot]') + || (inputs.allowed-bot-sender-ids != '' + && !startsWith(inputs.allowed-bot-sender-ids, ',') + && !endsWith(inputs.allowed-bot-sender-ids, ',') + && !contains(inputs.allowed-bot-sender-ids, ',,') + && github.event.sender.id + && contains(format(',{0},', inputs.allowed-bot-sender-ids), + format(',{0},', github.event.sender.id)))) + permissions: + actions: read + cache-mode: none + outputs: + skip-key: ${{ steps.gate.outputs.skip-key }} + steps: + - name: Read the key's credit history + id: gate + continue-on-error: true + timeout-minutes: 2 + uses: $/.github/actions/check-key-credit + with: + key-job: ${{ inputs.lane == 'security' && 'security-review-key' || 'claude-review-key' }} + window-hours: ${{ inputs.key-credit-window-hours }} + + - 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" + + # The job token's api-budget notice, from the readings the gate's + # requests left. It spends no request. + - name: Log the API budget + if: always() + continue-on-error: true + timeout-minutes: 1 + uses: $/.github/actions/report-api-budget + with: + tokens: github-token + review-key: name: ${{ inputs.lane == 'security' && 'security-review-key' || 'claude-review-key' }} + needs: check-key-credit runs-on: ubuntu-24.04 - timeout-minutes: ${{ inputs.lane == 'security' && 45 || 43 }} + timeout-minutes: ${{ inputs.lane == 'security' && 46 || 44 }} if: >- + needs.check-key-credit.outputs.skip-key != 'true' + && !cancelled() + && github.event_name == 'pull_request' && github.event.pull_request.state == 'open' && github.event.pull_request.draft == false @@ -1101,6 +1178,16 @@ jobs: cli-version: ${{ steps.compose.outputs.cli-version }} leg-timeout-minutes: ${{ inputs.lane == 'security' && '13' || '11' }} + # Read by the next runs' check-key-credit job: a step named for this run that + # succeeds only after the launcher's credit stop, which only the key + # leg can end in. 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: always() && steps.claude.outputs.lane-class == 'credit-stop' + continue-on-error: true + timeout-minutes: 1 + run: echo "the key ended in a credit stop" + # The CLI's init message must report the mode asked for; dontAsk may # read default, which run-claude-cli's `--permission-prompts none` # makes deny every call no rule allows, as dontAsk does. A mismatch, or @@ -1329,11 +1416,14 @@ jobs: # writes them into a script. review-subscription: name: ${{ inputs.lane == 'security' && 'security-review-subscription' || 'claude-review-subscription' }} - needs: review-key + needs: [check-key-credit, review-key] runs-on: ubuntu-24.04 - timeout-minutes: ${{ inputs.lane == 'security' && 45 || 43 }} + timeout-minutes: ${{ inputs.lane == 'security' && 46 || 44 }} if: >- - (needs.review-key.outputs.handoff == 'true' || needs.review-key.outputs.credential-present == 'false') + ((needs.review-key.result == 'success' + && (needs.review-key.outputs.handoff == 'true' || needs.review-key.outputs.credential-present == 'false')) + || (needs.review-key.result == 'skipped' && needs.check-key-credit.outputs.skip-key == 'true')) + && !cancelled() && github.event_name == 'pull_request' && github.event.pull_request.state == 'open' @@ -2173,6 +2263,16 @@ jobs: cli-version: ${{ steps.compose.outputs.cli-version }} leg-timeout-minutes: ${{ inputs.lane == 'security' && '13' || '11' }} + # Read by the next runs' check-key-credit job: a step named for this run that + # succeeds only after the launcher's credit stop, which only the key + # leg can end in. 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: always() && steps.claude.outputs.lane-class == 'credit-stop' + continue-on-error: true + timeout-minutes: 1 + run: echo "the key ended in a credit stop" + # The CLI's init message must report the mode asked for; dontAsk may # read default, which run-claude-cli's `--permission-prompts none` # makes deny every call no rule allows, as dontAsk does. A mismatch, or @@ -2381,7 +2481,7 @@ jobs: known-failure-class: ${{ steps.claude.outputs.lane-class }} handoff: ${{ steps.claude.outputs.handoff }} # Why this job ran, for this leg's lane record. - fallback-reason: ${{ needs.review-key.outputs.lane-class == 'credit-stop' && 'credit-stop' || needs.review-key.outputs.lane-class == 'auth' && 'auth' || needs.review-key.outputs.credential-present == 'false' && 'no-key' || '' }} + fallback-reason: ${{ (needs.review-key.result == 'skipped' || needs.review-key.outputs.lane-class == 'credit-stop') && 'credit-stop' || needs.review-key.outputs.lane-class == 'auth' && 'auth' || needs.review-key.outputs.credential-present == 'false' && 'no-key' || '' }} # The job token's api-budget notice, from the readings the scope # step's requests and the shell steps' `gh api -i` files left. It @@ -2405,7 +2505,7 @@ jobs: status: name: ${{ inputs.lane == 'security' && 'security-review' || 'claude-review-status' }} needs: [review-key, review-subscription] - if: always() && needs.review-key.result != 'skipped' + if: always() && (needs.review-key.result != 'skipped' || needs.review-subscription.result != 'skipped') runs-on: ubuntu-24.04 timeout-minutes: 20 permissions: @@ -2481,6 +2581,9 @@ jobs: script: | const fs = require("node:fs"); const env = process.env; + // The key leg's last summary, else the subscription leg's (both + // read the same marker; the key job is skipped after a credit stop). + let lastSummary = false; for (const [leg, prefix, expected] of [ ["review-key", "key", env.KEY_SHA256], ["review-subscription", "sub", env.SUB_SHA256], @@ -2499,7 +2602,10 @@ jobs: } 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"]); + if (!lastSummary && typeof data?.["last-summary"] === "string") { + core.setOutput("last-summary", data["last-summary"]); + lastSummary = true; + } } - name: Publish the review @@ -2514,15 +2620,15 @@ jobs: BASE_REF: ${{ github.event.pull_request.base.ref }} PUBLISH_GATE: ${{ steps.gate.outputs.path }} ARTIFACT_DIR: ${{ runner.temp }}/shadow-report - SCOPE_REVIEW: ${{ needs.review-key.outputs.scope-review }} - SKIP: ${{ needs.review-key.outputs.skip }} - SKIP_REASON: ${{ needs.review-key.outputs.skip-reason }} - SINCE: ${{ needs.review-key.outputs.since }} - SCOPE: ${{ needs.review-key.outputs.scope }} - MARKER_ID: ${{ needs.review-key.outputs.marker-id }} - MERGE_BASE: ${{ needs.review-key.outputs.merge-base }} + SCOPE_REVIEW: ${{ needs.review-key.result == 'skipped' && needs.review-subscription.outputs.scope-review || needs.review-key.outputs.scope-review }} + SKIP: ${{ needs.review-key.result == 'skipped' && needs.review-subscription.outputs.skip || needs.review-key.outputs.skip }} + SKIP_REASON: ${{ needs.review-key.result == 'skipped' && needs.review-subscription.outputs.skip-reason || needs.review-key.outputs.skip-reason }} + SINCE: ${{ needs.review-key.result == 'skipped' && needs.review-subscription.outputs.since || needs.review-key.outputs.since }} + SCOPE: ${{ needs.review-key.result == 'skipped' && needs.review-subscription.outputs.scope || needs.review-key.outputs.scope }} + MARKER_ID: ${{ needs.review-key.result == 'skipped' && needs.review-subscription.outputs.marker-id || needs.review-key.outputs.marker-id }} + MERGE_BASE: ${{ needs.review-key.result == 'skipped' && needs.review-subscription.outputs.merge-base || needs.review-key.outputs.merge-base }} LAST_SUMMARY: ${{ steps.handoffs.outputs.last-summary }} - PR_FILES: ${{ needs.review-key.outputs.pr-files }} + PR_FILES: ${{ needs.review-key.result == 'skipped' && needs.review-subscription.outputs.pr-files || needs.review-key.outputs.pr-files }} KEY_LANE_CLASS: ${{ needs.review-key.outputs.lane-class }} SUB_RESULT: ${{ needs.review-subscription.result }} KEY_PRESENT: ${{ needs.review-key.outputs.credential-present }} @@ -2565,7 +2671,7 @@ jobs: HEAD_SHA: ${{ github.event.pull_request.head.sha }} BASE_REF: ${{ github.event.pull_request.base.ref }} PUBLISH_GATE: ${{ steps.gate.outputs.path }} - PR_FILES: ${{ needs.review-key.outputs.pr-files }} + PR_FILES: ${{ needs.review-key.result == 'skipped' && needs.review-subscription.outputs.pr-files || needs.review-key.outputs.pr-files }} KEY_LANE_CLASS: ${{ needs.review-key.outputs.lane-class }} SUB_RESULT: ${{ needs.review-subscription.result }} KEY_PRESENT: ${{ needs.review-key.outputs.credential-present }} diff --git a/README.md b/README.md index aa82d55c..5760320c 100644 --- a/README.md +++ b/README.md @@ -717,6 +717,14 @@ consumer to audit it. step: the `api-budget` notice (`token=github-token ... cost=N`) the hub collector reads, built from the `x-ratelimit-*` headers of the job's own requests, so it spends none (its `action.yml` describes what it does). +- `.github/actions/check-key-credit` — `pr-review-cli.yml`'s `check-key-credit` + job: skips the key job while the key's credit stop in the same workflow is + inside `key-credit-window-hours` (default 6), reading the workflow's runs + with the job token (one list call, at most three job-list calls), and runs + the key on any failure. Another keyed lane adopts it with the same three + pieces: a gate job before its key job, a record step named for its run + after the launcher's `credit-stop`, and a subscription job that also starts + on the gate's skip (its `action.yml` describes the inputs). Each input's meaning and default is documented inline in the action's `inputs:` block. @@ -1131,7 +1139,9 @@ GitHub continues the normal weekly patching of each hosted image generation. credential-scanned JSON artifact instead, lane records under a `-shadow` lane id, and no job ever red. The caller grants `contents: read` and `pull-requests: write` in both modes and, in summary mode, does not cancel - in-progress runs. The workflow header carries the canonical caller and + in-progress runs, and grants `actions: read` for the `check-key-credit` job, + which skips the key job for `key-credit-window-hours` after the key's + credit stop. The workflow header carries the canonical caller and the security model. With `hold-label` set, comments mode also holds the pull request for a person: when the review returns a `hold` with criterion `cost` (only a change that can cause a real charge, as the compose step's