Skip to content

feat(pr-review-cli): pass each lane its prior findings - #955

Merged
kyle-sexton merged 1 commit into
mainfrom
feat/950-prior-findings-step
Nov 1, 2026
Merged

kyle-sexton merged 1 commit into
mainfrom
feat/950-prior-findings-step

Conversation

@kyle-sexton

Copy link
Copy Markdown
Member

Closes #950
Refs: melodic-software/claude-code-plugins#7777

Summary

claude-code-plugins#12011 added plugins/review/lib/prior-findings.mjs and taught /review:code-review and /review:security-review to act on a PRIOR FINDINGS: prompt line. Nothing produced that line. This runs the script on every model leg of pr-review-cli.yml, for both lanes, and passes its note to the compose prompt the same way the settled-threads note is passed.

Fix

  • New composite .github/actions/collect-prior-findings (one bash step, no model, no github-script, so no context shadowing as in fix(report-lane-outcome): stop shadowing github-script's context parameter #952). It:
    • removes any head-shipped .claude-lane/prior-findings.md;
    • reads the round cap file with gh api -X GET repos/$REPOSITORY/contents/docs/conventions/pr-pipeline.yaml -f ref="$DEFAULT_BRANCH" (raw), where DEFAULT_BRANCH is github.event.repository.default_branch, never the head or base SHA. Any failed read (404 included) leaves CAP_CONFIG unset, so the script counts no cap;
    • runs node "$SOURCE/plugins/review/lib/prior-findings.mjs" from the claude-code-plugins default-branch clone the plugin step already staged, with the job token (contents: read, pull-requests: read, unchanged);
    • on a script failure removes the output file and fails the step; the script writes note= only after a successful write, so the compose prompt gets no line and the lane reviews as before.
  • pr-review-cli.yml: in review-key, review-subscription and review-fallback, a prior step after settled with the settled step's if:, continue-on-error: true and timeout-minutes: 2; lane maps code/security to claude-review/claude-security-review, the footer names the script matches. Compose gets PRIOR_NOTE: ${{ steps.prior.outputs.note }} and env.PRIOR_NOTE right after env.SETTLED_NOTE.
  • Each leg job's limit rises from 93/101 to 95/103 minutes (code/security), so the job still exceeds its steps' sum by 5 as pr-review-cli.test.cjs requires.
  • SECURITY MODEL header and README list the new file and action.

Verification

  • node --test .github/scripts/*.test.cjs: 793 pass, 0 fail (4 of them new in collect-prior-findings.test.cjs: cap read from the default branch with the exact gh arguments; unread cap or empty default branch leaves CAP_CONFIG unset and still writes the file; a failed script leaves no file and no note, including a head-planted one; every leg wires the step before compose with read-only permissions).
  • The script's GraphQL query, sent live with gh api graphql against claude-code-plugins#12011, returns no errors and github-actions with databaseId 41898282 on reviews and comments (the fix(collect-settled-threads): read resolvedBy as the User it is #953 failure class).
  • zizmor on the action and workflow: no findings. shellcheck on the composite's run block: clean. Local actionlint 1.7.12 reports only the classes it already reports on main ($/ local action refs, cache-mode); CI's pinned actionlint is the authority.
  • After merge: the next lane run's log should show prior-findings: N prior post(s), R round(s), cap 3 on a claude-code-plugins PR.

Related

🤖 Generated with Claude Code

Every model leg (key, subscription, fallback) runs the new
collect-prior-findings composite after the plugin clone and before
compose, on both lanes. It runs claude-code-plugins'
plugins/review/lib/prior-findings.mjs with the job token, writes
.claude-lane/prior-findings.md and hands the PRIOR FINDINGS note to the
compose prompt beside the settled-threads note. The round cap
(loop.review-rounds) is read from docs/conventions/pr-pipeline.yaml on
the repository's default branch, never the head; an unread cap means no
cap. A failed script leaves no file and no note, so the lane reviews as
before. Each leg job's limit rises by the step's 2 minutes.

Closes #950

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kyle-sexton
kyle-sexton marked this pull request as ready for review November 1, 2026 18:41
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Nov 1, 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-11-01T18:42:36.029972Z 7495c6c 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

github-actions Bot commented Nov 1, 2026

Copy link
Copy Markdown

claude-review reviewed 7495c6c3adea7ad9354177fd424f5baadadda3db: 0 finding(s), 0 as inline comments.

No findings.

I read the whole diff (4 files), the PR context, and the existing collect-settled-threads step and the matching settled steps in pr-review-cli.yml for comparison.

  • Wiring: The prior step is added to all three legs (review-key, review-subscription, review-fallback) with the same if:, continue-on-error and timeout-minutes: 2 as the settled step. Each leg passes PRIOR_NOTE into compose and adds env.PRIOR_NOTE right after env.SETTLED_NOTE. The job limits rise from 93/101 to 95/103 minutes, which matches the 2-minute step.
  • Round cap: action.yml reads the cap from DEFAULT_BRANCH, never the head. Any failed read leaves CAP_CONFIG unset, so no cap applies.
  • Planted file: The step removes a head-planted prior-findings.md before running. It removes the file again, and fails the step, when the script fails.
  • Tests: The new tests cover those three behaviours and the wiring on every leg.
  • Local action ref: uses: $/.github/actions/... is the same form the existing settled step uses, so it is not a new problem.
  • Not verified: I could not read plugins/review/lib/prior-findings.mjs, because it is not in this checkout. I could not confirm that the env names the action passes (OUTPUT_PATH, LANE, CAP_CONFIG, GITHUB_TOKEN) are the ones the script reads. The settled action uses OUTPUT_FILE, a different script. The PR says the script was run live, but that is the author's claim and I did not confirm it. I also did not run the tests; the "793 pass" count is likewise the author's claim.
  • Closing issue feat(pr-review-cli): run prior-findings.mjs and pass its note to the review lanes #950: Its body was omitted from the context because the author is not on the allowed list. I treated it as unread, so I could not check the change against any acceptance criteria.

@github-actions

github-actions Bot commented Nov 1, 2026

Copy link
Copy Markdown

claude-security-review reviewed 7495c6c3adea7ad9354177fd424f5baadadda3db: 0 finding(s), 0 as inline comments.

No security findings. critical-findings: 0

What the PR does: it adds a collect-prior-findings composite action and wires it into all three model legs of pr-review-cli.yml (review-key, review-subscription, review-fallback). It also adds a test, a README entry and a 2-minute timeout bump. The action runs prior-findings.mjs from the plugin clone and adds a PRIOR FINDINGS: note to the compose prompt.

Trust-control and trust-boundary checks:

  • Round cap: pr-pipeline.yaml is read through the contents API with ref=$DEFAULT_BRANCH, never the head. A failed or empty read leaves CAP_CONFIG unset, which means no cap and the review still runs. This does not weaken a merge hold: it only feeds the review prompt and no gate script consumes it.
  • Script source: SOURCE is ${{ runner.temp }}/claude-code-plugins, the clone of the default-branch tip that the plugin step already stages. The step is gated on steps.plugin.outcome == 'success'. The head cannot supply the script.
  • Planted output file: rm -f -- "$OUTPUT_PATH" runs before the script and again after a failure. A file the head ships at .claude-lane/prior-findings.md never reaches the model, and OUTPUT_PATH is a fixed literal from the workflow.
  • Token scope: the action gets github.token as GH_TOKEN and GITHUB_TOKEN. The jobs' permissions are contents: read and pull-requests: read, which the new test asserts, so no scope grows and no secret or App token is added.
  • Injection: the run: block interpolates nothing from the github context directly. Every value goes through env:, so there is no expression injection.
  • Prompt text: the new PRIOR_NOTE path is a fixed file name. The script writes the note only on success, and the file is labelled DATA in the skill. The posts it lists are limited, per the workflow header, to bot-authored posts carrying the lane footer. I could not read prior-findings.mjs here (it lives in claude-code-plugins), so that filter rests on the header comment and the skill text, not on code I read.
  • Failure behaviour: continue-on-error: true fails open, but only to the previous behaviour (a review with no prior findings).
  • Timeouts and charges: the timeouts go from 93/101 to 95/103 minutes, and the step timeout is 2 minutes. This starts no billed resource.
  • Manifests, .github/actions/* trust controls and instruction surfaces: the diff touches no plugin manifest, trusted-actor list, kill switch, token broker, merge-hold script or standing instruction.

Not reported: uses: $/.github/actions/collect-prior-findings matches the existing collect-settled-threads step's form, so I treated it as the repository's convention. Action pinning and workflow hardening are zizmor's lane.

@kyle-sexton
kyle-sexton added this pull request to the merge queue Nov 1, 2026
Merged via the queue into main with commit 821aee1 Nov 1, 2026
53 of 56 checks passed
@kyle-sexton
kyle-sexton deleted the feat/950-prior-findings-step branch November 1, 2026 19:11
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.

feat(pr-review-cli): run prior-findings.mjs and pass its note to the review lanes

1 participant