Skip to content

fix(pr-pipeline): keep the unrunnable-criterion split out of the needs_human path - #9376

Merged
kyle-sexton merged 2 commits into
mainfrom
fix/9375-unrunnable-split-not-needs-human
Oct 23, 2026
Merged

kyle-sexton merged 2 commits into
mainfrom
fix/9375-unrunnable-split-not-needs-human

Conversation

@kyle-sexton

Copy link
Copy Markdown
Member

Closes #9375

Summary

Canary #9372 had a Windows 11 host item in its done-when. It ended with status: needs-decision and needs-human, and the only comment was the reusable's fixed note: no split and work class none. The cause: #9367's rubric clause routed these items through the intent flag, and the model escalated with needs_human. The pinned reusable (ci-workflows intake-triage.yml@2fe38c0, lines 1524 and 1552-1556) drops the model's labels and comment on escalation. A flagged or escalated verdict also never reaches admit.mjs, which runs only on status: ready. So the split comment could not be written on either path.

Fix

  • Deterministic: apply-verdict.mjs now calls a new noteUnrunnable in admit.mjs whenever it holds an issue (escalated or flagged verdict, or a hold label). When a done-when item is one the lane cannot perform, it posts the same admission comment decide() would, carrying ADMIT_MARKER. The comment names each item and recommends keeping the lane-checkable items as the admissible issue and moving the rest to a needs-human child issue. It changes no labels: the held path already sets status: needs-decision. Errors are recorded the same way admission errors are.
  • Rubric (intake-triage.yml): I removed fix(pr-pipeline): hold an issue whose done-when the lane cannot perform #9367's clause from the intent flag and added an UNRUNNABLE DONE-WHEN section. It says such an item alone is no reason for needs_human, the intent flag or a none class: the model triages the lane-checkable items with their class and fields, and admission holds the issue and posts the split. On that path apply-verdict sets status: ready and admit.mjs refuses with the split, as fix(pr-pipeline): hold an issue whose done-when the lane cannot perform #9367 intended.
  • Changelog fragment for pr-pipeline (patch).

Verification

  • node --test admit.test.mjs apply-verdict.test.mjs: 142 pass, 0 fail. New tests:
    • noteUnrunnable on docs(machine-health): README says 19 Windows checks; the catalog has 27 #9287's Windows 11 host done-when posts one comment with the marker and the split recommendation, and writes no labels. A runnable done-when writes nothing.
    • run() on an escalated verdict calls the split note: the outcome is needs-decision and the summary shows Admission: noted. A ready verdict does not call it.
  • callers, reenter, triage-sweep and pr-verify/workflow tests: 198 pass. actionlint is clean.

Related

🤖 Generated with Claude Code

…s_human path

apply-verdict now posts admit.mjs's split note on an issue it holds
(an escalated or flagged verdict, or a hold label) when a done-when item
is one the lane cannot perform, so the split survives an escalation that
drops the lane comment. The intake rubric stops routing these items
through the intent flag and asks the model to triage the lane-checkable
items with their work class.

Closes #9375

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

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 🔄 Running since 2026-10-23T08:15:05.851821Z 86dab5f 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.

@github-actions

github-actions Bot commented Oct 23, 2026 •

Copy link
Copy Markdown
Contributor

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

No security issues found in the incremental change (admit.mjs, admit.test.mjs, apply-verdict.mjs, since 86dab5f).

critical-findings: 0

What changed and how I read it:

  • admissionSettled pulls the "already agent-ready" and "agent-ready was removed before" checks out of decide(). noteUnrunnable now calls it too, so a held issue a person has already admitted, or has had admission revoked on, gets no split note. The check is the same as before, and it leaves decide()'s skip behaviour unchanged. It adds no label write and no new authority.
  • noteUnrunnable now takes the issue object that apply-verdict already fetched. It no longer takes number and body separately, and run() passes the same fetched issue (apply-verdict.mjs:396). That fetch sits behind the existing open-issue and trusted-author checks (apply-verdict.mjs:312-317). The change adds no new input source.
  • The one new read is GET {base}/events, paginated. It is scoped to the same repository and issue number. The events are only checked for an unlabeled event naming agent-ready, and they are never rendered or executed.
  • sayWhy now returns early when the existing App-authored marker comment already holds the same body. This removes a redundant PATCH. The comment-ownership check (App login plus ADMIT_MARKER prefix) is unchanged, so no one else's comment can be edited.
  • The test changes are fixtures only.

manifest fields: the incremental diff touches no plugin manifest.

@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 86dab5f516323544cfe4e0f4d17f5ca7ba0a9f18; the summary is in this lane's pull request comment.

claude-lane: claude-review

@github-actions

github-actions Bot commented Oct 23, 2026 •

Copy link
Copy Markdown
Contributor

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

Incremental review of admit.mjs, admit.test.mjs and apply-verdict.mjs, covering the change since 86dab5f. I found nothing to flag.

  • admissionSettled extraction: decide() now calls the new helper. The skip reasons and their order are unchanged.
  • noteUnrunnable signature: it now takes issue, and apply-verdict.mjs passes the issue it already fetched through noteSplit(issue). The only call site matches the new signature.
  • Skip behaviour: noteUnrunnable skips an issue that is already agent-ready or had the label removed before. It returns {outcome: "skipped", reasons}, which the existing Admission: summary line in run() renders without changes.
  • sayWhy no-op: it returns early when the existing note's body is identical, and still edits a changed note in place with PATCH.
  • Tests: the new noteUnrunnable tests cover both skip cases and the repeated-run case: unchanged note, no write; changed note, PATCH. The fake GitHub client already supports the events and comments options they use. I did not run the tests; I only read them.
  • Closing issue fix(pr-pipeline): the unrunnable-criterion hold loses its split recommendation behind needs_human #9375: the three criteria were reviewed in the earlier pass, and nothing in this change weakens them. The split comment is still written on the held path, and a test still covers the Windows-host item.
  • Hold: no hold class applies. The change adds no model call or spend and touches no trusted-actors, CODEOWNERS or .gitmodules files.

…e an unchanged note alone

noteUnrunnable now applies decide()'s early exits through a shared
admissionSettled (already agent-ready, or agent-ready removed before),
and sayWhy writes nothing when the App's admission comment already
holds the same text.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kyle-sexton
kyle-sexton added this pull request to the merge queue Oct 23, 2026
Merged via the queue into main with commit 98e329a Oct 23, 2026
32 checks passed
@kyle-sexton
kyle-sexton deleted the fix/9375-unrunnable-split-not-needs-human branch October 23, 2026 08:28
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.

fix(pr-pipeline): the unrunnable-criterion hold loses its split recommendation behind needs_human

1 participant