[kyle-sexton]

No related issue: follow-up to melodic-software/ci-workflows#973

Summary

pr-review-main.yml moves to the check-key-credit gate merged in melodic-software/ci-workflows#973 (bc62beb95a989641f30552a932a132ebb8ecd801). That gate reads one artifact name, key-credit-stop-<workflow>-<key-job>, and no longer reads run history for a Record the key's credit stop in run N step. Repinning alone would leave the gate with no marker to find, so it would never skip the key: it fails open, so nothing would turn red.

Fix

• review-handoffs-credit: repin check-key-credit to bc62beb9 and expose the gate's marker output. The step that carried origin-run forward is removed.
• review-handoffs: the record step is replaced by Write the key's credit-stop marker (this run's id) and Upload the key's credit-stop marker (upload-artifact cf430e03 v7.0.2, retention-days: 1, if-no-files-found: error). The marker is written only when a key leg that ran handed on with credit-stop, never for a lane the gate itself handed on.
• .github/artifact-retention.json: new key-credit-stop-* family with scan: none. This differs from ci-workflows' pr-review-cli.yml, which scans its marker. Here the marker holds only $GITHUB_RUN_ID, and scanning it would need a .base checkout in review-handoffs, which has permissions: {}. This follows the existing scan: none rows for workflow-written ids.
• The window stays ${{ vars.KEY_CREDIT_WINDOW_HOURS || '1' }}. No permission changes: review-handoffs-credit keeps actions: read, review-handoffs keeps permissions: {}, and upload-artifact uses the runtime token.
• workflow.test.mjs now fails on any of these: the gate pinned to a SHA other than bc62beb9, a missing marker output, missing write or upload steps, an upload whose name is not needs.review-handoffs-credit.outputs.marker, retention other than 1, or any remaining Record the key's credit stop step.

Verification

• node --test .github/scripts/pr-review-main/workflow.test.mjs: 50/50 pass.
• bash scripts/check-artifact-retention.sh --check: 81 upload steps match 56 families.
• bash scripts/ci-review-lanes.test.sh: PASS=239 FAIL=0.
• actionlint (local 1.7.12): its only findings are the cache-mode job keys already on main, which this version does not know.
• bash scripts/check-affected.sh --base origin/main --no-fetch: 10 passed, 5 skipped over 3 changed files.

Related

• melodic-software/ci-workflows#973, the marker gate.
• melodic-software/ci-workflows#976 sets the hosted review lanes' window default to 1 h (it replaces the closed ci(review): run the hosted review lanes on a 1-hour key-credit window #12834).
• pr-verify.yml moves in perf(pr-verify): read the key's credit history only for a head it judges #12799.

Operations

Marker name: key-credit-stop-pr-review-main.yml-review-handoffs (gate.cjs markerName("pr-review-main.yml", "review-handoffs")).

To clear a credit stop by hand, for example after topping up or rotating the key inside the window:

gh api 'repos/melodic-software/claude-code-plugins/actions/artifacts?name=key-credit-stop-pr-review-main.yml-review-handoffs'
gh api -X DELETE repos/melodic-software/claude-code-plugins/actions/artifacts/<id>

Hardening residuals

• The gate (gate.cjs judgeMarkers at bc62beb9) accepts any unexpired marker artifact from a same-repository run, with no filter on workflow, event or branch. So any same-repository run can upload one and move the key leg to the subscription account for the window. That affects availability only: no verdict changes, nothing bills, and the leg fails closed. It was raised in poteto's independent security review of perf(pr-verify): read the key's credit history only for a head it judges #12799 and is deferred to the hardening phase under the owner's threat-model rule. The fix then is to require that the uploading run's workflow path is the caller workflow.

🤖 Generated with Claude Code

[chatgpt-codex-connector]

Codex Review Summary

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

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-11-03T15:06:19.177047Z	6a051b3	Draft marked ready

ℹ️ 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.

[github-actions]

claude-security-review reviewed 6a051b3fad0d2b1e76c7f336d39bb2e2f719671a: read 3 of 3 changed file(s); 1 finding(s), 0 as inline comments.

Security review: PR #12838 (pr-review-main moves to the credit-stop marker gate)

I read every hunk of all 3 changed files: .github/workflows/pr-review-main.yml (a trusted path), .github/scripts/pr-review-main/workflow.test.mjs (trusted path, pr-review-main/**) and .github/artifact-retention.json. I also read the review-handoffs job around the change (pr-review-main.yml L1231-L1289) and the workflow_run trigger guards.

What I checked, with no finding:

• Token and permissions: nothing widens. review-handoffs-credit keeps actions: read (L360-L361). review-handoffs keeps permissions: {} (L1242). The upload uses only the runtime token, and no secret is added.
• Injection: the write step runs echo "$GITHUB_RUN_ID" and puts no expression inside run:. The artifact name comes from the gate's marker output, which the gate builds from fixed inputs (key-job: review-handoffs and the workflow name), so no attacker-controlled text reaches it.
• Pins: actions/upload-artifact@cf430e03… is the same pin as the verdict upload at L1188. check-key-credit is repinned to a full SHA, bc62beb9….
• scan: none on the marker family: the artifact holds only $GITHUB_RUN_ID and no model text, so skipping the leak scan exposes nothing.
• Kill switch and merge holds: both are unchanged. The CLAUDE_*_DISABLED switches still gate each skipped key leg's handoff (L1249-L1250).
• Fail-open direction: if the gate fails, the key leg runs, as before. The key-leg credit-stop condition still excludes lanes that the gate itself handed on.

Finding (SUGGESTION, inline on the repin at L176): the skip signal used to be a step name inside this workflow's own runs, which only the copy on main writes. It is now a repository artifact found by a fixed, public name. Any workflow run in the repository can upload an artifact with that name, including a pull_request run whose workflow file comes from the PR head. This file checks provenance for the verdict artifact (L239-L250), but whether the external gate.cjs checks the marker's producing run can't be confirmed from this repository. The impact is limited to routing: the key leg is skipped and reviews go to the subscription leg. Reviews still run, so nothing leaks, no account is compromised and nothing is billed. That is why this is not CRITICAL.

Spend: this only moves review runs between the API-key leg (prepaid credit) and the subscription leg. Neither is a billed charge, so there is no cost hold.

No embedded instructions in the PR context. The prior-findings file lists none (round 0 of 3, not past the cap).

critical-findings: 0

Findings not posted inline (their lines are not in the diff):

• .github/workflows/pr-review-main.yml:176 (RIGHT): Anyone who can run a workflow in this repository may be able to forge the credit-stop signal

Under the old gate, the skip signal was a step name inside this workflow's own runs. Only the workflow_run copy on main writes those, so a pull request could not fake it. With this repin, the gate instead lists repository artifacts by one fixed, public name: key-credit-stop-pr-review-main.yml-review-handoffs.

Any workflow run in the repository can upload an artifact under that name. That includes a pull_request run that executes a workflow file from the PR's own head. If gate.cjs accepts any unexpired artifact with the matching name, a PR author can upload one with a single actions/upload-artifact step. Doing that again each window keeps the API-key leg skipped, so every review goes to the subscription leg and spends its quota.

This file already guards against exactly this for the verdict artifact: L239-L250 check that the producing run's .path, .event == "workflow_run" and .head_branch == "main" are right before trusting it. Please confirm that the gate at bc62beb9 applies the same check to the marker's workflow_run (workflow path or id, workflow_run event, main branch). If it does not, add the check upstream.

The impact is limited to routing: reviews still run on the subscription leg. Nothing leaks, no account is taken over and nothing is billed, so this is not CRITICAL.

severity: SUGGESTION

[github-actions]

claude-review reviewed 6a051b3fad0d2b1e76c7f336d39bb2e2f719671a: read 3 of 3 changed file(s); 0 finding(s), 0 as inline comments.

Code review: PR #12838, move pr-review-main to the credit-stop marker gate

No findings. This is a complete review: I read every hunk of all 3 changed files in the full diff (.github/artifact-retention.json, .github/scripts/pr-review-main/workflow.test.mjs, .github/workflows/pr-review-main.yml). I also read the surrounding review-handoffs-credit, review and review-handoffs jobs at head. The workflow file is on the trusted-path list, so it got extra scrutiny.

What I checked

• Data flow. review-handoffs already has review-handoffs-credit in its needs (pr-review-main.yml#L1232), so it can read needs.review-handoffs-credit.outputs.marker. The new marker job output maps to steps.gate.outputs.marker (L364).
• Write and upload conditions. A marker is written only when a key leg that actually ran returned credit-stop. A lane the gate itself handed on is a skipped review job with empty outputs, so it never restarts the window. The write step also requires a non-empty marker, so if the gate step fails, no artifact is uploaded under an empty name and the key is still tried (fail-open). Both steps are continue-on-error, so a failure in either one never blocks the subscription matrix.
• Permissions. review-handoffs keeps permissions: {}. upload-artifact authenticates with the Actions runtime token, not GITHUB_TOKEN scopes, so the new upload needs no grant. The upload-artifact pin (cf430e03… v7.0.2) is the same SHA as the workflow's other three uploads.
• Retention. The key-credit-stop-* family has retention-days: 1 and scan: none, which matches the step's retention-days: 1. The reason text accepts that a window longer than 24 hours ends when the artifact expires. Since the gate fails open, that only means the key gets retried.
• Tests. The updated assertions match the head workflow: LEGS.length + 1 upload count, review-handoffs allowed exactly one uses: (actions/upload-artifact), the gate pinned to the exact bc62beb9 SHA, the marker output regex, the write and upload step ifs, inputs and order, and no Record the key's credit stop steps left anywhere. Dropping the display-name assertion is consistent with a gate that reads artifacts by name instead of job names.

Unverified

The behaviour of ci-workflows check-key-credit at bc62beb9 (the markerName format, the window computed from upload time, and how it filters the artifacts it lists) is out of this PR's scope. I did not read it, so those points are the author's claims. The author's Verification section (50/50 tests, artifact-retention check, actionlint) is also author-claimed. I ran none of it.

Security

I left security questions (for example, which artifacts the gate's repo-wide artifact listing would trust) to the separate security-review lane.

No closing issue (the PR is a follow-up to ci-workflows#973). No plugin manifests changed. No prior findings to reconcile, and the round cap was not reached.