Skip to content

feat(pr-review-cli): skip the key job while the key's credit stop is recent - #882

Merged
kyle-sexton merged 5 commits into
mainfrom
feat/skip-burned-key-job
Oct 24, 2026
Merged

kyle-sexton merged 5 commits into
mainfrom
feat/skip-burned-key-job

Conversation

@kyle-sexton

@kyle-sexton kyle-sexton commented Oct 24, 2026 •

Copy link
Copy Markdown
Member

No related issue: owner decision Q57 (2026-10-24)

Summary

A keyed review lane whose API key is out of credits still starts the key job on every run: apt installs bubblewrap, checks out, preps, runs the launcher, gets a credit stop, and only then hands off to the subscription job. That is a wasted hosted job and extra API calls and delay on every review. Owner decision Q57, design (a): read this workflow's own recent runs and skip the key job while a credit stop is inside a 6-hour window.

Fix

  • New composite action .github/actions/check-key-credit (gate.cjs), run by a new check-key-credit job (claude-review-key-credit / security-review-key-credit) before review-key. It holds no secret and only actions: read, and uses the job token, not the App token. It makes one list call (GET .../actions/workflows/<caller file>/runs?status=completed&created=>=<now-window>&per_page=100). Then, newest first, it makes at most 3 job-list calls (GET .../runs/<id>/jobs?filter=latest) and stops at the first run that settles the question.
  • The credit-stop signal: the key job gets a step named Record the key's credit stop in run ${{ github.run_id }}. It runs only when steps.claude.outputs.lane-class == 'credit-stop', the launcher's class from classify.cjs isCreditStop, which only the key leg can end in. An auth class (401, or 402 without the credit text) never counts. When the gate skips, its own job runs a step with the same name pattern that carries the origin run id. The next run therefore finds the original credit stop in its newest run, and the window counts from that credit stop, not from the last skip. The first run after the window runs the key again, which picks up a new cycle or a replaced key.
  • When the gate skips, review-key is skipped and review-subscription starts (needs: [check-key-credit, review-key], !cancelled()). The existing handoff path is unchanged and still requires review-key to succeed. The status job runs when either leg ran. It reads the scope outputs (scope-review, skip, since, scope, marker-id, merge-base, pr-files) from the subscription job when the key was skipped, and takes the last summary from the subscription leg when the key leg left none. The subscription's fallback-reason is credit-stop, so the lane records keep their existing value set. Check names, marker text, the review-complete notice and the lane-record format are unchanged.
  • Fail open: review-key's if starts with !cancelled() and skips only on an exact skip-key == 'true'. A failed gate job, a refused read, an unreadable ref, a window of 0 or no settling run within the cap all run the key as before.
  • New input key-credit-window-hours (number, default 6). The gate writes one log line per run. A skip is a ::notice title=check-key-credit:: naming the origin run and its URL.
  • Caller change required: callers must grant actions: read on the job that uses this workflow. Without it the run fails at startup, because a called workflow can only lower the caller's grant. The header's canonical caller and the README say so.

Verification

  • nice node --test .github/scripts/*.test.cjs: 701 pass, 0 fail.
  • New check-key-credit.test.cjs (9 tests) covers these cases:
    • A record inside the window skips the key. A record outside the window runs it. A key that ran with no record runs it.
    • An auth stop or a lookalike step name never counts.
    • The gate reads newest first, skips the current run and is capped at 3 job reads.
    • Every read failure fails open, and a jobs read that returns empty steps for every job (seen on claude-code-plugins run 40115785836) counts as no evidence, so the key runs after the cap.
    • main spends one list call and one jobs call on a skip, writes the notice and outputs, and records api-budget readings.
  • pr-review-cli.test.cjs: 6 new or changed tests fail against main's workflow and pass now:
    • The fork, draft and bot gate also covers check-key-credit.
    • The subscription starts on an exact credit skip only.
    • The key job runs unless the gate returns exactly true, including when the gate fails.
    • The record steps match the gate's pattern and run only on credit-stop.
    • The status job runs on a credit skip and reads the subscription's scope.
    • The last summary falls back to the subscription leg.
  • actionlint and biome check pass on the changed files.

Unverified until live

The carry step's name renders steps.gate.outputs.origin-run. The contexts table lists steps for jobs.<job_id>.steps.name, and matrix values render in step names in the jobs API, but a rendered step output has not been observed. The first credit stop on main after the re-pin is the probe: check that the next run's check-key-credit job skips with the notice, and that the skipping run's carry step shows a run id in its name. If it does not render, the gate still finds the key job's own record (its name uses github.run_id) for runs until that record falls out of the newest three, then runs the key again (fail open, smaller saving).

Related

  • Other keyed lanes adopt it with the same three pieces:

    • a gate job before the key job, with key-job set to the key job's name;
    • a record step named Record the key's credit stop in run ${{ github.run_id }}, gated on the launcher's lane-class == 'credit-stop';
    • a subscription job that also starts on review-key.result == 'skipped' && skip-key == 'true'.

    Wiring claude-code-plugins pr-unblock, pr-verify and the issue lane is out of scope here.

  • pr-pipeline-sandbox's callers need actions: read when they bump past this release.

🤖 Generated with Claude Code

…recent

A key out of credits still started the key job on every run: runner
isolation, checkout and prep, a credit stop, then the handoff. A new
key-credit job reads this workflow's completed runs from the last
key-credit-window-hours (default 6) with the job token (actions: read):
one list call and at most three job-list calls. When the key job ended
in the launcher's credit-stop class in that window, the key job is
skipped and the subscription job reviews; the status job then reads the
subscription job's scope, and the last summary falls back to that leg.

The key job records a credit stop as a step named for its run, run only
after lane-class credit-stop, so an auth stop never counts; a skipping
run carries the origin forward the same way, so the window counts from
the credit stop. Any read failure or unclear answer runs the key job as
before. Callers grant actions: read.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 24, 2026 •

Copy link
Copy Markdown

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-24T17:31:34.211669Z a8ff487 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 Bot commented Oct 24, 2026 •

Copy link
Copy Markdown

Review in progress

  • Gather context (diff, CLAUDE.md/AGENTS.md, REVIEW.md, path-scoped rules)
  • Review .github/actions/key-credit-gate/gate.cjs and action.yml
  • Review .github/workflows/pr-review-cli.yml changes
  • Review test coverage (key-credit-gate.test.cjs, pr-review-cli.test.cjs)
  • Review README.md changes
  • Check PR body contract compliance
  • Post findings

@claude

claude Bot commented Oct 24, 2026 •

Copy link
Copy Markdown

Security review in progress

  • Gather context (diff, PR description, changed files)
  • Review key-credit-gate/action.yml and gate.cjs for trust-boundary / injection issues
  • Review pr-review-cli.yml workflow changes (job wiring, permissions, actions: read grant)
  • Review test files for behavior consistency with claims
  • Check README changes
  • Apply skip gate / instruction-surface / manifest checks where relevant
  • Adversarially verify any candidate findings
  • Post final findings

View job run

…run the key

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@claude

claude Bot commented Oct 24, 2026 •

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 3m 50s —— View job


Security review complete

  • Invoke /review:security-review skill and gather inputs
  • Read diff (git diff origin/main...HEAD) and changed files
  • Review .github/actions/key-credit-gate/action.yml and gate.cjs (trust boundary, injection, token handling)
  • Review .github/workflows/pr-review-cli.yml changes (job wiring, permissions, fail-open logic)
  • Review test files for consistency with security claims
  • Check for instruction-surface deletions / manifest changes
  • Adversarially verify candidate findings
  • Post final findings

Scope: .github/actions/key-credit-gate/{action.yml,gate.cjs}, .github/scripts/{key-credit-gate.test.cjs,pr-review-cli.test.cjs}, .github/workflows/pr-review-cli.yml, README.md.

No security issues found. Notes from the review:

  • Permissions are correctly minimized. The new key-credit job declares permissions: actions: read only (pr-review-cli.yml#L303), which replaces (not adds to) the workflow-level contents: read default for that job, so it genuinely holds no secret and no write — confirmed by the test's assert.doesNotMatch(JSON.stringify(gate), /secrets\./u).
  • No injection surface. The only attacker-adjacent value is the run id embedded in the dynamic step name Record the key's credit stop in run ${{ ... }}. It is always a numeric GitHub run id (Number.isSafeInteger checked in decide(), \d{1,20} enforced by RECORD_PATTERN on read-back), never free text, so there's no path to expression/log injection via GITHUB_OUTPUT or the step name.
  • github.workflow_ref usage is correct, not a bug. gate.cjs extracts a bare filename from it and queries /repos/{GITHUB_REPOSITORY}/actions/workflows/{file}/runs. In a reusable-workflow context github.workflow_ref/github.repository resolve to the caller's own workflow and repo (that's the whole reason job_workflow_ref exists as a separate claim), so this correctly lists the consuming repo's own caller-workflow runs rather than something cross-tenant.
  • Credit-stop signal can't be cheaply forged. The record step only runs when classify.cjs's detectCreditStop: leg.leg === "api-key" produces lane-class credit-stop, which is unreachable from the subscription leg and distinct from auth (401 / non-credit 402). A same-repo contributor with push access could in principle edit the workflow to fabricate a record, but that's an existing trust boundary (such a contributor already has secret access) and is explicitly called out in the gate.cjs header comment — worst case is shifting reviews to the subscription job for the window, not a privilege escalation.
  • Fail-open is consistently enforced. Every failure mode (unreadable token/ref, HTTP error, non-array response, window <= 0) returns skip: false without throwing; review-key's if requires an exact == 'true' plus !cancelled(), so a failed/erroring gate always still runs the key job. Verified against key-credit-gate.test.cjs's fail-open cases and the pr-review-cli.test.cjs assertions (runs("", "failure"), runs("", "skipped") → true).
  • Scope stays same-repo. key-credit's if duplicates review-key's fork/draft/bot gate exactly (head.repo.full_name == github.repository, dependabot/bot-sender checks), so it never runs for fork PRs — consistent with the rest of the lane's trust model.
  • The duplicated Record the key's credit stop in run ... step on the review-subscription job is template-shared (same generator as the rest of that job's steps) and is inert there, since the subscription leg can never produce lane-class credit-stop. Not a finding, just worth noting for anyone reading the diff cold.
  • No .claude-plugin manifest, CLAUDE.md/AGENTS.md, or rules-file changes in this diff, so the instruction-surface and manifest checks are N/A here.

critical-findings: 0

claude-lane-summary: lane=claude-security-review sha=85f8f0c922750b5fd5f9206ecfb98b6417cc5717 run=40116561026

@claude

claude Bot commented Oct 24, 2026 •

Copy link
Copy Markdown

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


Review complete

  • Gather context (diff, CLAUDE.md/AGENTS.md, REVIEW.md, path-scoped rules, PR context)
  • Review .github/actions/key-credit-gate/gate.cjs and action.yml
  • Review .github/workflows/pr-review-cli.yml changes
  • Review test coverage (key-credit-gate.test.cjs, pr-review-cli.test.cjs)
  • Review README.md changes
  • Check PR body contract compliance
  • Post findings

Scope note: REVIEW.md confirms this repo runs a dedicated security lane (pr-review-security.yml), so this lane's scope is correctness/design/conventions/tests — security findings (trust boundaries, token scoping, etc.) are left to that lane.

PR body contract: compliant — opens with No related issue: owner decision Q57 (2026-10-24) and fills ## Summary/## Fix/## Verification/## Related per pr-body-contract.md.

What I checked closely:

  • judgeRun/decide in gate.cjs: the newest-first scan, the 3-call cap, the window-membership check, and the mutual exclusivity of the key job's own record vs. the gate job's carried-forward record within a single run — consistent and matches the 8 new tests.
  • The github.workflow_ref self-identification (action.yml#L44): since this reusable workflow only ever runs nested inside a caller's run (no standalone run history of its own), workflow_ref correctly resolves to the caller's workflow file — which is exactly where the claude-review-key/claude-review-key-credit job names actually land in the Jobs API, including the caller-job-prefixed jobNamed lookup. This checks out; it is not confused with job_workflow_ref (used elsewhere in this repo for OIDC trust, a different purpose).
  • The fail-open wiring: review-key's if (needs.key-credit.outputs.skip-key != 'true' && !cancelled() && ...), review-subscription's if, the status job's widened if, and the fallback-reason/SCOPE_REVIEW-family fallbacks to the subscription leg's outputs — all traced through and consistent with the PR description, including the edge case where a fork/draft/bot PR skips key-credit itself (no spurious subscription start).
  • The last-summary fallback logic in the handoffs script — confirmed the key leg is skipped correctly (empty KEY_SHA256 fails the sha256-shape check) so it falls through to the subscription leg.

No correctness or convention findings to report.

Not independently verified: I could not execute node --test, actionlint, or biome check myself in this lane (Bash execution of scripts/binaries is not available to me here) — the Verification section's pass counts are author-claimed, not independently confirmed by me.

claude-lane-summary: lane=claude-review sha=85f8f0c922750b5fd5f9206ecfb98b6417cc5717 run=40116561026

@github-actions

github-actions Bot commented Oct 24, 2026 •

Copy link
Copy Markdown

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

@github-actions

github-actions Bot commented Oct 24, 2026 •

Copy link
Copy Markdown

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

…ts token description

The naming lint requires a verb-first action directory, so the action and
its job become check-key-credit. An unquoted "actions: read" in the token
input's description broke the action's YAML for actionlint and zizmor.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@claude

claude Bot commented Oct 24, 2026 •

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 1m 57s —— View job


Review complete

  • Gather context (incremental diff, AGENTS.md, path-scoped rules, PR context)
  • Review .github/actions/check-key-credit/action.yml + gate.cjs changes
  • Review pr-review-cli.yml wiring changes
  • Review test file changes
  • Review README.md changes
  • Check PR body contract compliance
  • Post findings

Scope: incremental — only the diff since 85f8f0c (the rename of key-credit-gate → check-key-credit and quoting the github-token input description), per .claude-lane/incremental.diff.

PR body contract: no changes since the last pass; still compliant (No related issue: owner decision Q57 (2026-10-24), all four sections present).

What this commit does: a pure mechanical rename — action directory key-credit-gate → check-key-credit, the gate.cjs header comment/log strings, the test file and its require() paths, the workflow job id key-credit: → check-key-credit: and every needs:/needs.* reference to it, plus a YAML quoting fix on the github-token input's description. Verified repo-wide with grep -rn "key-credit-gate" that no stale reference to the old name survives outside this diff, and that every needs: key-credit / jobs["key-credit"] site in pr-review-cli.yml and pr-review-cli.test.cjs was updated consistently. The job's human-readable name: field (claude-review-key-credit / security-review-key-credit) is intentionally left unchanged — that's the external check name, not the job id, and wasn't touched by this diff.

Finding (posted inline):

  • README.md:720 — this line updates the action path to check-key-credit but still describes it as pr-review-cli.yml's key-credit job, which no longer exists under that id after this same commit renamed the job. The other README mention (line 1142) already says check-key-credit; this one is now the stale one.

Not independently verified: I could not run node --test in this lane (Bash execution of the test runner requires approval I don't have here), so the Verification section's pass counts remain author-claimed.

claude-lane-summary: lane=claude-review sha=38e9250d70942e26a9b36b6e4980e0b4c5082c99 run=40118174982

@claude

claude Bot commented Oct 24, 2026 •

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 50s —— View job


Security review complete

  • Invoke /review:security-review skill and gather inputs
  • Read .claude-lane/incremental.diff (change since 85f8f0c)
  • Assess security-relevant surface of the incremental change
  • Apply skip gate
  • Post findings

Scope: incremental review of 38e9250 vs the last-reviewed 85f8f0c, across .github/actions/key-credit-gate/* → .github/actions/check-key-credit/*, .github/scripts/key-credit-gate.test.cjs → check-key-credit.test.cjs, .github/scripts/pr-review-cli.test.cjs, .github/workflows/pr-review-cli.yml, README.md.

Skip gate applied (item 2 — no security-relevant surface): the entire incremental diff is a mechanical rename of the gate action and job id (key-credit-gate → check-key-credit, job key-credit → check-key-credit), carried consistently through the action, script, tests, workflow comments/strings, and README prose. No logic, condition, permission, trigger, or token-handling change accompanies it:

  • review-key's if still gates on needs.check-key-credit.outputs.skip-key != 'true' && !cancelled() — same condition, same job, new name.
  • review-subscription's needs/if updated to the new job id with the same result == 'skipped' && skip-key == 'true' logic.
  • The gate job's permissions: actions: read and its lack of any secret (assert.doesNotMatch(JSON.stringify(gate), /secrets\./u) in the updated test) are unchanged — see pr-review-cli.yml#L303.
  • The one non-rename line, check-key-credit/action.yml#L23, just quotes the github-token input's description string ("The job token, with actions: read.") — a YAML-scalar-safety fix for the embedded : , not a behavior or trust-boundary change.
  • No permissions:, trigger, token-handling, or .claude-plugin manifest change in this increment; not a trust-gate path.

This increment carries no exploitable change beyond what was already reviewed at 85f8f0c (see that review's findings above — none). No new security issues to report.

critical-findings: 0

claude-lane-summary: lane=claude-security-review sha=38e9250d70942e26a9b36b6e4980e0b4c5082c99 run=40118174982

Comment thread README.md Outdated
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@claude

claude Bot commented Oct 24, 2026 •

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 32s —— View job


Security review complete

  • Invoke /review:security-review skill and gather inputs
  • Read .claude-lane/incremental.diff (change since 38e9250)
  • Assess security-relevant surface of the incremental change
  • Apply skip gate
  • Post findings

Scope: incremental review of 8cacbe6 vs the last-reviewed 38e9250, limited to .github/scripts/pr-review-cli.test.cjs per the review scope note.

Skip gate applied (item 2 — no security-relevant surface): the entire incremental diff is a pure formatting change — reflowing one inline object literal in a test assertion onto multiple lines:

-        needs: { "check-key-credit": { result, outputs: { "skip-key": skipKey } } },
+        needs: {
+          "check-key-credit": { result, outputs: { "skip-key": skipKey } },
+        },

(.github/scripts/pr-review-cli.test.cjs around the "the key job runs unless check-key-credit says exactly true..." test, permalink.) The literal value, keys, and test assertions are byte-for-byte identical before and after — this is a biome/formatter-style line-wrap with no semantic change. No logic, permission, token-handling, trigger, or manifest change accompanies it.

No security-relevant surface in this increment. No new security issues to report; nothing beyond what was already reviewed at 38e9250 (see that review's findings — none).

critical-findings: 0

claude-lane-summary: lane=claude-security-review sha=8cacbe6b3267336f5e5f78e95f8976c46af5032c run=40118446767

@claude

claude Bot commented Oct 24, 2026 •

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 31s —— View job


Review complete

  • Gather context (incremental diff, AGENTS.md, path-scoped rules, PR context)
  • Review .github/scripts/pr-review-cli.test.cjs changes (since 38e9250)
  • Check PR body contract compliance
  • Post findings

Scope: incremental — only the diff since 38e9250 (this lane's last reviewed sha), limited to .github/scripts/pr-review-cli.test.cjs, per .claude-lane/incremental.diff.

Skip gate applied (item 3 — no review surface): the entire increment is a 3-line formatting reflow of one test's needs object literal (single-line → multi-line), matching the commit's own message (style(pr-review-cli): format the renamed gate test lines). No assertion, expectation, or test logic changed — confirmed by reading the diff and the surrounding test body (.github/scripts/pr-review-cli.test.cjs:220-240): the check-key-credit lookup key, the result/skip-key values, and every assert.* call are byte-identical to the prior pass, just re-wrapped for the 72/80-column formatter. No other file changed in this increment.

PR body contract: no changes since the last pass; still compliant (No related issue: owner decision Q57 (2026-10-24), all four sections present).

No findings to report for this increment.

claude-lane-summary: lane=claude-review sha=8cacbe6b3267336f5e5f78e95f8976c46af5032c run=40118446767

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kyle-sexton
kyle-sexton enabled auto-merge October 24, 2026 18:00
@claude

claude Bot commented Oct 24, 2026 •

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 26s —— View job


Review complete

  • Gather context (incremental diff, AGENTS.md, path-scoped rules, PR context)
  • Review README.md change since 8cacbe6
  • Check PR body contract compliance
  • Post findings

Scope: incremental — only the diff since 8cacbe6 (this lane's last reviewed sha), limited to README.md, per .claude-lane/incremental.diff.

What this commit does: fixes the stale cross-reference this lane flagged on the previous pass (inline comment on 38e9250) — README.md:720 now reads pr-review-cli.yml's check-key-credit job, matching the job id renamed in 38e9250 and matching the other mention at README.md:1142. No other content in this increment.

PR body contract: no changes since the last pass; still compliant (No related issue: owner decision Q57 (2026-10-24), all four sections present).

No findings to report for this increment.

claude-lane-summary: lane=claude-review sha=1d0d86a53d15b8d21ea7ac7faea0973e7695b13b run=40118654361

@kyle-sexton
kyle-sexton added this pull request to the merge queue Oct 24, 2026
Merged via the queue into main with commit 4f79cc9 Oct 24, 2026
30 checks passed
@kyle-sexton
kyle-sexton deleted the feat/skip-burned-key-job branch October 24, 2026 18:04
kyle-sexton added a commit to melodic-software/standards that referenced this pull request Oct 24, 2026
…contract (#877)

Refs: melodic-software/ci-workflows#882

## Summary

Registers the `pr-review-cli.yml` reusable-workflow contract at
ci-workflows
[v0.65.8](https://github.com/melodic-software/ci-workflows/releases/tag/v0.65.8)
(`4f79cc9244498ae856504cf74aeeb05dcc9c6b43`), so callers pinned to that
release pass runner-policy.

## Fix

Copies the v0.65.7 entry (`@4c531cea137f5cbb38d4c132dbfa6e38f76fba7d`)
forward in `components/runner-policy/policy.json` and adds the matching
copy-forward test in `runner-policy.test.mjs`, following #874 and #876.
v0.65.8
([ci-workflows#882](melodic-software/ci-workflows#882))
adds a `check-key-credit` job that skips the key job while the key's
recent credit stop is inside `key-credit-window-hours`. Callers pass no
new input. They grant `actions: read` on the calling job, which the
contract does not record. Inputs, secrets and runner are unchanged.

## Verification

- `node components/runner-policy/runner-policy.mjs --root <worktree>`:
Runner policy passed.
- `node --test components/runner-policy/runner-policy.test.mjs`: 350
pass, 0 fail.

## Related

-
[melodic-software/ci-workflows#882](melodic-software/ci-workflows#882)
(released as v0.65.8)
- #876 (the v0.65.7 contract)

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant