Skip to content

fix(pr-pipeline): drop non-security review and verify stops - #9798

Merged
kyle-sexton merged 23 commits into
mainfrom
fix/review-verify-stops
Oct 25, 2026
Merged

kyle-sexton merged 23 commits into
mainfrom
fix/review-verify-stops

Conversation

@kyle-sexton

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

Copy link
Copy Markdown
Member

No related issue: removes non-intent, non-security stops (owner goal 2026-10-24; rows D:S1, D:S3, D:S6/N2, D:S7, D:S8, D:S10, D:S12, D:STALL 1, D:STALL 3, D:STALL 4, D:STALL 5, D:STALL 6, D:STALL 7, E:E19)

Trust path. This pull request changes the review lanes' hold classes, the review callers, ci-status's cost-only clear, the pr-verify prompt and workflow, and the lost-run reconciler's pr-verify handling. It needs the trust-path reviewer.

Summary

Only an intent mismatch or a security issue should stop an item. This removes the review, verify and ci-status stops in this batch that are neither, and keeps every security gate it touches:

  • The review callers no longer hold on a changed path: both keep hold-paths, which in ci-workflows v0.65.8 only flags the four trust-gate paths for extra security scrutiny and never holds by itself. The review skills' hold text is fix(review): point the hold classes at the live ci-workflows prompt #9857's (pointer to pr-review-cli.yml@v0.65.8); this pull request does not change either review skill.
  • The verifier judges the takeover set on what a change does, not on the path; counts only real-charge cost holds as owner holds; and passes an oversize diff with notes.
  • ci-status's cost-only clear is no longer blocked by any lanes App comment, only by a lanes App escalation comment.
  • pr-verify runs that failed, were lost after the judge started, or were stopped by the kill switch are re-run once by the lost-run reconciler instead of leaving the head with no verdict.

Fix

row done / skipped why
D:S1 moved Landed in #9857: both review skills' hold blocks point at pr-review-cli.yml@v0.65.8. This pull request leaves both skills as main has them.
D:S3 done The review skills' trust-gate class went in #9857. Both callers keep main's hold-paths list: the input "asks the review to give a changed file matching one extra security scrutiny. A match never holds the pull request by itself" (pr-review-cli.yml@v0.65.8#L243-L247). An earlier draft of this branch added a takeover-path paragraph to security-review/SKILL.md (extra paths: kill switch, lane token, resolve-config; a takeover rated CRITICAL). It is dropped: #9857's Trusted paths paragraph covers the same place, and two paragraphs on one topic would disagree. The pr-pipeline README's "Review-lane holds" now says a hold-paths match only flags a file for scrutiny and never holds by itself.
D:S6 / N2 done pr-verify/prompt.md: a change to the takeover set is read with extra care and fails only when it lets someone outside the organization gain trust, push, merge or mint a token, or weakens one of those checks beyond the spec.
D:S7 done prompt.md: the owner-kept cost hold is a hold on a real charge; model, effort, turn, timeout or schedule changes get a note.
D:S8 done prompt.md: a diff too large to judge whole returns pass-notes naming the unjudged parts and a split, unless what was read blocks.
D:S10 skipped The missing-severity-line rule is read by babysit_review_evidence.py (poteto's file, off limits), and no mechanism re-runs the security lane from there. Per the batch instruction, the CRITICAL default stays.
D:S11 skipped Held by the batch instruction (cross-repo broker deploy).
D:S12 skipped Security: the check covers Dependabot, the release App and the standards-sync App, whose pull requests no review lane reads. Removing it, or passing the over-3000-file case, would merge an unreviewed change to the trusted-actor list or CODEOWNERS. Only its comments changed: they name the trust-gate paths directly instead of the callers' hold-paths.
D:STALL 1 skipped Already fixed on main by #9568: rerun-ci-status uploads its rerun-ci-status-<run>-<attempt>-<sha> record, which check-trusted-trigger accepts.
D:STALL 2 skipped Held by the batch instruction (resolves with poteto's rows).
D:STALL 3 done pr-verify.yml: both "Report the kill switch" steps now fail the job, and the reconciler re-runs a failed first attempt, so a run stopped by the kill switch is judged after the switch clears. The reconciler idles while the switch is on.
D:STALL 4 done pr-require-checks.yml cost-only clear: only a lanes App comment carrying a <!-- work-items:escalation marker, created or edited in the window, keeps the hold. Review replies and fix notes no longer do. A lanes escalation still blocks the clear.
D:STALL 5 done reconcile.mjs: failure joins the lost conclusions for recorded lanes (LANE_LOST), so a judgment that broke after it started is re-run once (attempt 2 is never re-run).
D:STALL 6 skipped Not a stall. With hold-label set and publish: comments, the reusable's Apply the hold step runs always(), including a no-change skip. uncounted is reported only for a run in summary mode or without a hold label, a caller change that would otherwise dodge the holds.
D:STALL 7 skipped Lives in resolve-pr.mjs, which belongs to the Q78 builder.
E:E19 done reconcile.mjs: the judge-step hold is removed. A run lost after its judge started is re-run once. It was there to stop a cancel-to-rejudge; the lanes App token has no actions: write (pr-merge.yml: "actions: write, which the lane App token does not have"), a FAIL uploaded before the loss still counts, and attempt 2 is never re-run. A run none of whose jobs started is still left alone, because a newer run judges the head. Composed with #9871: a job whose steps did not list in full, and whose cut list reaches the judge step or its predecessor, stays held, because a runner-loss annotation there can be echoed judge output. Only a run whose steps listed in full is re-run after its judge step started.

Trust-path reviewer, please check:

  1. Is it safe to drop the judge-step hold (E:E19 / STALL 5)? The argument is in the E:E19 row.
  2. Should the code lane also name the takeover paths as extra-scrutiny paths where no security lane runs? Today only its critical-security class covers them.

Verification

  • node --test .github/scripts/maintenance-reconcile-lost-runs/*.test.mjs after merging main (fix(ci): count a review lane by its head-bound evidence when the jobs API lists no steps #9813's steps-unknown handling kept; its two judge-step-hold tests now expect the once-only re-run E:E19 makes; fix(maintenance): count a runner-loss annotation only on a failed job cut before the judge step #9871's cut-list holds kept): 89 pass. The new tests cover a judge-started run re-run once, a failed run re-run once, a kill-switched run re-run, a failed attempt 2 never re-run, and pr-verify failing on the kill switch. They failed before the change.
  • bash scripts/ci-hold-cost-decision.test.sh: PASS=140 FAIL=0. New cases: an edited escalation keeps the hold, and a non-escalation App comment clears. They failed before the change.
  • bash scripts/ci-review-lanes.test.sh: PASS=83 FAIL=0. Both callers send exactly the four trust-gate paths as hold-paths, and the exempt-author patterns stay pinned to the same four paths.
  • node --test in .github/scripts/pr-verify: 101 pass.
  • scripts/check-affected.sh --tests on the merged head: 12 passed, 1 skipped. The one failure was pytest missing on this machine. The 14 selected Python suites pass under python3 -m unittest.
  • actionlint on the five changed workflows: clean. markdownlint on the changed markdown: 0 issues.
  • UNVERIFIED: how the lanes behave live after the hold change. That needs a pull request through both review lanes.

Related

  • Inventory: stops-vs-global-intent-2026-10-24.md (owner session), section "Automation Lanes".
  • ci-workflows pr-review-cli.yml v0.65.8: the pin both review callers use; hold-paths is scrutiny-only there.

🤖 Generated with Claude Code

kyle-sexton and others added 8 commits October 24, 2026 12:35
The review lanes hold only for a real charge or a CRITICAL security
finding: code-review's cost class covers non-free runners and IaC, and
the trust-gate path hold is gone; the security lane reads the takeover
paths with extra scrutiny and rates a takeover CRITICAL. The verifier
judges the takeover set on what a change does, treats only real-charge
holds as owner holds, and passes an oversize diff with notes. ci-status's
cost-only clear ignores lanes App comments that are not escalations. The
lost-run reconciler re-runs a failed or judge-started pr-verify run once,
and pr-verify fails a kill-switched run so it is re-run after the switch.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DUS7XB8hUjcDnuSYLqJRzd
…ep spend out of the count

The cost class listed only runners and infrastructure as code, narrower than
the wrapper prompt's paid-plan, Azure and GitHub billing list, and called
subscription spend a counted finding, which holds the merge. Point at
pr-review-cli.yml@v0.65.7 for both the schema and the rule, and name such
spend in one summary line outside the findings count.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
# Conflicts:
#	.github/workflows/pr-review-hosted.yml
#	.github/workflows/pr-review-security-hosted.yml
# Conflicts:
#	.github/scripts/maintenance-reconcile-lost-runs/reconcile.mjs
Lane Funding's own pull request replaces the hold-class text in both review
skills with a pointer to pr-review-cli.yml, so this branch drops its edits
to those sections and keeps only the security lane's takeover-path scrutiny.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kyle-sexton
kyle-sexton marked this pull request as ready for review October 25, 2026 01:38
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 25, 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-25T01:40:46.457143Z dae4450 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 Oct 25, 2026 •

Copy link
Copy Markdown
Contributor

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

Incremental review of the four files changed since 158d751: no findings.

The change moves the reconciler's pr-verify hold from the judge step's listed conclusion to whether the "Write the verdict" step started (WRITE_STEP). The judge step is continue-on-error: true, so the jobs API lists it as success even when it broke. The old hold could therefore keep a run whose judgment failed from ever being re-run.

What I checked by reading the code. I ran no tests.

  • .github/workflows/pr-verify.yml: both jobs, check-intent and the subscription job, have a "Write the verdict" step gated on if: steps.judge.outcome == 'success'. That is the true outcome, not the continue-on-error masked one. The reconciler's premise holds.
  • holdReason in reconcile.mjs counts only steps with a non-null conclusion other than skipped. A skipped write (judge broke) does not hold, so the run is re-run once. A write that succeeded, failed or was cancelled does hold. A cut step list that reaches the judge step or its predecessor is still held before this check, so the runner-loss path is unchanged.
  • The new test regex in reconcile.test.mjs finds both write steps in the workflow and asserts the if: gate. The new skipped-write test covers the re-run path. The renamed hold test covers the held path and the new summary text.
  • The changelog fragment and the comments in the workflow and reconcile.mjs match the new behavior.

Cosmetic only, not counted as findings: the rewrapped comments in reconcile.mjs (around "A FAIL" / "the first attempt uploaded") and in maintenance-reconcile-lost-runs.yml have uneven line lengths.

No manifest items apply, since no plugin manifest changed. No cost hold: the change adds no billed resource and no paid-plan setting. It can only trigger one more pr-verify run per head, which spends prepaid or subscription usage. The prompt names no closing issue ("No related issue"), so there are no acceptance criteria to check.

@github-actions

github-actions Bot commented Oct 25, 2026 •

Copy link
Copy Markdown
Contributor

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

Incremental security review of 158d751..f688926. I found no security issues.

critical-findings: 0

The change moves the reconciler's hold test for a pr-verify run from "the judge step succeeded" to "the Write the verdict step started". That is safe:

  • .github/workflows/pr-verify.yml marks the judge step continue-on-error: true, so the jobs API lists it as success even when it broke. The old test would have held, and so left without a verdict, every broken judgment.
  • Write the verdict has if: steps.judge.outcome == 'success' in both jobs (lines 342 and 646 of pr-verify.yml). outcome is the true result before continue-on-error, so the write step starts only when the judgment finished.
  • The new test in reconcile.test.mjs pins that if: on both write steps, so the hold keeps working if someone edits the workflow.
  • A run that lost its runner during or after the write step still lists the judge step, or the step before it, in its cut step list. holdReason holds that case (reconcile.mjs:273), so a possibly finished judgment is not judged twice.
  • started ignores skipped steps, so a skipped write step does not hold the run. The new test covers that case.
  • Re-run trust is unchanged. The re-run still goes through the recorded rerunRecordName artifact that check-trusted-trigger reads. The change adds no new trigger, permission, input or token use.

Manifests: this diff touches no plugin manifest.

Trusted paths: none of the four changed files matches the trusted-path globs.

The rest of the change is a changelog entry and comments. It spends no billed resources, so there is no hold.

kyle-sexton and others added 3 commits October 25, 2026 02:49
…ne re-run comment

The verifier passes a part of the diff it could not judge with notes only
when that part is prose or data; an unjudged workflow, action, script, hook
or binary still fails. A probe fixture with an unreadable binary pins it.

The reconciler's comment now says why a lane re-run grants nothing: an
uploaded FAIL counts whatever the run's conclusion, and the ci-status job
does hold actions: write.

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

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

claude-review: 1 inline finding(s) on 907d1f07b8fa6a4f0f1e3e1a8ea13fe3ff7e0188; the summary is in this lane's pull request comment.

claude-lane: claude-review

Comment thread .github/scripts/pr-verify/prompt.md Resolved
The trusted-actor list, CODEOWNERS and .gitmodules are data, so an
oversize diff that left one of them unjudged passed with notes.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@melodic-automation-lanes melodic-automation-lanes Bot added the needs-human Human-in-the-loop required; autonomous sessions must not resolve items carrying this. label Oct 25, 2026
@melodic-automation-lanes

Copy link
Copy Markdown
Contributor

The pr-unblock lane stopped on this pull request and needs a person: the merge stages a change under .github/workflows/, which the lane does not push.

  • Reason: workflow-staged
  • Resume key: 907d1f07b8fa6a4f0f1e3e1a8ea13fe3ff7e0188 (the head the lane stopped on)

To resume, deal with the reason above, remove the needs-human label (and do-not-merge, if set), then dispatch the pr-unblock workflow with pr-number=9798 and activity=resolve-conflicts:

gh workflow run pr-unblock.yml -R melodic-software/claude-code-plugins -f pr-number=9798 -f activity=resolve-conflicts

# Conflicts:
#	.github/scripts/maintenance-reconcile-lost-runs/reconcile.mjs

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

claude-review: 1 inline finding(s) on b8f4f07bf81a7814cf41755740885a21acc147d4; the summary is in this lane's pull request comment.

claude-lane: claude-review

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

claude-security-review: 1 inline finding(s) on b8f4f07bf81a7814cf41755740885a21acc147d4; the summary is in this lane's pull request comment.

claude-lane: claude-security-review

Comment thread .github/scripts/pr-verify/prompt.md Resolved
@kyle-sexton kyle-sexton removed the needs-human Human-in-the-loop required; autonomous sessions must not resolve items carrying this. label Oct 25, 2026
kyle-sexton and others added 2 commits October 25, 2026 07:20
The lost-run reconcile log claimed a newer run judges the head for both
hold reasons; only the not-started reason implies one. The cut-list hold
now logs its reason with no newer-run claim.

The verifier's oversize-diff rule now also fails when an unjudged part is
a package or plugin manifest, a lockfile, or agent or skill instructions
that steer the review or verify lanes. Other docs and fixtures keep
pass-notes.

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

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

claude-security-review: 2 inline finding(s) on afde7b32bc773018aaba75ae272c32e3417dfb45; the summary is in this lane's pull request comment.

claude-lane: claude-security-review

Comment thread .github/scripts/pr-verify/prompt.md Resolved
kyle-sexton and others added 2 commits October 25, 2026 09:09
A FAIL judgment followed by a write or upload error ended the run failed
with no FAIL uploaded, and the reconciler re-ran it, giving the head a
second model sample. The reconciler now holds a fully listed pr-verify
run whose judge step concluded success and names the hold in the run
summary; runs lost before or during the judgment are still re-run once.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…e judge step

The judge step is continue-on-error, so its listed conclusion is success
even when the judgment broke, and holding on it kept broken judgments
from their one re-run. The reconciler now holds a fully listed pr-verify
run only when its "Write the verdict" step started; that step runs only
on the judge step's true success. Every other lost run is re-run once.

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