Skip to content

feat(pr-pipeline): file the split for a lane-unrunnable done-when at refine - #9439

Merged
kyle-sexton merged 11 commits into
mainfrom
feat/refine-files-unrunnable-split
Oct 24, 2026
Merged

kyle-sexton merged 11 commits into
mainfrom
feat/refine-files-unrunnable-split

Conversation

@kyle-sexton

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

Copy link
Copy Markdown
Member

Closes #9396

Summary

Intake refinement now files the split that admission recommends, the owner's option 1 on #9396. Admission holds an issue at status: needs-decision when some of its done-when items ask for things an unattended lane run cannot do (the check from #9367 and #9376). The refine-issue model run from #9310 now also takes such an issue and judges whether the rest of the done-when stands on its own. When it does, apply-refinement makes these changes as the lanes App:

  • It files one child issue labeled needs-human that holds only the held items, copied as written. The child links back with Split from #N, gets Type Task, and ends with the born-triaged marker, so intake skips it.
  • It rewrites the parent's body. The held task-list lines become one plain line, - Done-when item 2 needs a person, so it moved to #M. The lane-checkable items stay as they were.
  • It adds needs-triage, removes the App's status: needs-decision, posts the marker comment, and lets redispatch-refined triage the parent once more, so admission runs on what is left.

This reuses #9310's code paths with no second mechanism. It uses the same refine-select → refine-issue → apply-refinement → redispatch-refined jobs, the same REFINE_MARKER comment as the once-per-issue key, the same lane-token activity (apply-refinement, effect mutate-tracker), the same context hash, and the same check that drops a refinement when the title or body changed while the model ran.

Fix

  • refine.mjs --select picks a split candidate deterministically with splitOf. The issue must not be thin, the verdict's Flags: line must be exactly none, it must be at a status: needs-decision the lanes App set (read from label events by account id), admission's own unrunnableCriteria must find at least one held item but not all of them, and the lanes App's admission comment must be exactly the refusal for those items. So an issue held for another reason as well (Size, type, class) waits for a person. The context carries split: {count, items: [{n, kind, text}]}. An issue that is thin goes down the existing thin path unchanged.
  • In the model's structured output, a new required split field is the item numbers to move. The model writes no text that ends up in the child or the rewritten body. validateOutput (no model) accepts a split only when all of these hold: exact is true, split names exactly the held numbers, at least one item stays, criteria and questions are empty, and outcome is set. A malformed or wider proposal throws before anything is written. If the model is not exact, it asks questions through the existing path and the issue stays held.
  • applyRefinement re-checks that the App still holds the issue. It finds a child it already filed (an App-authored issue starting with <!-- intake-triage split --> on the parent's timeline) or files one. It then sets the labels, PATCHes the body, and posts the marker comment last. A failure before the body edit leaves the held items in place, and a retry reuses the same child. The split comment carries no - [ ] line, so admission's criterion 7 and its unrunnable check do not read it.
  • The child's Type is set through the same GraphQL updateIssue(issueTypeId) call that intake's reusable uses. The REST type field on create is dropped without push access, and this token has contents: read. The type is best effort: if it fails, the run logs a warning and intake triages the child once, which is harmless.
  • An item's deeper-indented lines (a sub-list or wrapped text) move to the child with it and leave the parent with it.
  • The issue is read again before its labels and body change. If a person edited it meanwhile, the child stays filed, the issue stays held, and nothing else is written. A retry reuses a child only when it holds the same items.
  • admit.mjs gains a taskItems helper (items with their indented lines), which unrunnableCriteria now uses, and a refusalText helper that admission's refusal comment and refine's sole-reason check share.
  • intake-triage.yml: the prompt adds step 6 for a held issue, the schema gains split, select and apply get BASE_PATH, and the comments are updated. pr-pipeline.yaml: the apply-refinement comment now states the wider scope.

Verification

  • node --test .github/scripts/intake-triage/*.test.mjs .github/actions/resolve-config/callers.test.mjs after merging main: all passed (intake-triage suites 221). The new split tests in refine.test.mjs cover these cases:
    • An unrunnable done-when files exactly one needs-human child with only that item, the parent body loses it and links the child, and the marker comment goes last.
    • A second run does nothing: apply writes nothing and select skips because of the marker.
    • An issue with no unrunnable item is not selected and nothing is written.
    • A malformed proposal is rejected and nothing is written. The bad cases are: a non-array, non-integer items, a missing key, a runnable item added, the wrong item, no items, drafted criteria, no outcome, questions alongside a split, and a not-exact output that moves items.
    • Edge cases: a retry reuses the child it filed and ignores an outsider's look-alike, and a needs-decision that a person set is left alone.
  • actionlint and zizmor --offline on intake-triage.yml: clean.
  • scripts/check-affected.sh --tests: the lint checks and node suites passed. The Python suites it selected failed there only because this machine has no pytest; run with uv run --with pytest python -m pytest, babysit-prs passed 2599 and contract-clause-coverage passed 24.
  • Ready-time reviews ran locally in fresh-context subagents, because review-venue reported both CI lanes absent. Security: no findings. Code: four findings. Three were fixed in fix(pr-pipeline): split only an issue held for its unrunnable items alone: multi-line items, the sole-reason check, and re-reading the body before writing. The fourth asked whether the timeline's source.issue carries body. It does: checked live on fix(planning): brief TLDR prints "1 questions" for a single question #9378's timeline. No live run on a real issue yet: canary fix(planning): brief TLDR prints "1 questions" for a single question #9378 is the natural first case once this merges.

Related

🤖 Generated with Claude Code

kyle-sexton and others added 6 commits October 23, 2026 11:21
…refine

When admission holds an issue at the lanes App's needs-decision because
some, not all, of its done-when items ask for what a lane run cannot do,
the refine run judges the split. A proposal naming exactly the held items
makes apply-refinement file one needs-human child holding them, rewrite
the issue's done-when to the rest plus a link, lift needs-decision and
triage once more. The refinement marker keeps it to once per issue.

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

REST drops `type` on create without push access, so the child gets its
Task type through the same updateIssue call intake's reusable uses, best
effort. Select reads the admission rules and web hosts only for an issue
that is not thin, and comments that said refinement never edits a body or
adds needs-human now say what a split does.

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

Ready-time review fixes. The split now requires the lanes App's admission
comment to be exactly the refusal for those items, so an issue also held
for another reason waits for a person. An item's deeper-indented lines
move with it to the child and leave the parent with it. The issue is read
again before its labels and body change, so an edit made meanwhile is
kept, and a retry reuses only a child holding the same items.

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

chatgpt-codex-connector Bot commented Oct 23, 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-23T16:01:28.460167Z 3a7982a 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 23, 2026 •

Copy link
Copy Markdown
Contributor

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

Security review: PR #9439 (head ceff254)

No security findings. critical-findings: 0

I read the full diff (all 8 files) and the PR context. The PR context is data, and it holds no embedded instructions.

What I checked

  • Model output as a trust boundary. The model returns only integers in split. validateOutput requires them to equal the held item numbers from the select-time context exactly. They must leave at least one item behind, with no drafted criteria and no questions. The child body and the rewritten parent body come only from the issue's own text, via movedTexts and splitBody. A prompt-injected issue or page therefore cannot write text into the child or the parent. It can only trigger a split that the deterministic splitOf rules already qualify.
  • Eligibility is deterministic and bound to the lanes App.
    • Select-time gates (splitOf and selectRefinement): the label must be status: needs-decision, and a verdict with Flags: none.
    • Select-time gate: the App's admission comment must exactly equal ADMIT_MARKER plus refusalText(unrunnableReason(...)), so only that sole refusal reason qualifies. That comment is matched by the App's account id.
    • decisionSetter requires the App's account id as the last actor to set the label, and applyRefinement re-checks it.
    • applyRefinement re-checks the label before writing.
    • The existing context-hash and once-per-issue marker checks are unchanged.
    • Rules and web hosts are read from the default-branch checkout (BASE_PATH=.base), not from the PR head.
  • Retry reuse of the child. childOf matches a timeline cross-referenced event only when the source issue is not a PR, was authored by the App's id, is in this repository, and has a byte-identical body. An outsider's look-alike issue cannot be adopted. The test covers this.
  • Injection into the child. The copied items come from an issue authored by a listed human or the lanes App. @ is broken so the child does not re-ping. The child carries the born-triaged marker last and the needs-human label, so no lane works it.
  • TOCTOU. The issue is re-read before the labels and body edit, and a changed body leaves the issue held. The window is small and fails safe.
  • Token scope. The activity now creates an issue and edits the refined issue's body, as the PR notes. The grant is unchanged (issues: write, effect mutate-tracker). It runs from the default-branch checkout, and its inputs are validated by closed-shape code with no model text reaching the writes. The docs/conventions/pr-pipeline.yaml scope comment is updated to match, so the declaration is not weakened. The change is intended (owner decision on feat(pr-pipeline): decide whether refinement files the split for a lane-unrunnable done-when #9396) and disclosed.
  • Actions hardening. The intake-triage.yml changes only add BASE_PATH, extend the prompt text and widen the schema. They add no expression injection, no trigger change and no permission change, so I left that to zizmor.

Minor observations (not findings)

  • applyRefinement compares decisionSetter(...) !== appId. If appId were undefined, an issue with no labeled event would pass. selectRefinement can't produce a split context without a defined appId, because the admission comment match requires it. This is not reachable today.
  • Plugin manifests: none changed, so there are no manifest <field> lines.
  • Trust-gate paths (trusted-actors, check-trusted-trigger, CODEOWNERS, .gitmodules): none touched, so no hold.
  • I did not read docs/conventions/instruction-exception-register/README.md. No standing guardrail was deleted or narrowed beyond the scope-comment update above, which a person reviews per the PR's trust note.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3a7982aaf6

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

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@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 3a7982aaf6399d63d7449be38d42c706b53c81cc; 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 ceff254914cbfb0a98f27d5cbce0118916079634: 0 finding(s), 0 as inline comments.

Code review: PR #9439 (head ceff254)

No findings. I read all 8 changed files in .claude-lane/pr.diff, and the PR context. I also read admit.mjs around unrunnableCriteria, unrunnableReason and sayWhy, to check how the new code uses them. I did not run the tests or any code, so the passing-test claims in the PR description are author-claimed and unverified by me.

What I checked

  • Contract with admit.mjs.
    • splitOf destructures {found} from unrunnableCriteria, which returns {items, found} (admit.mjs:195).
    • splitOf rebuilds the admission comment as ADMIT_MARKER + "\n" + refusalText(unrunnableReason(...)). That matches what sayWhy posts (admit.mjs:319), because the refusal now goes through the shared refusalText.
    • So the sole-reason check compares like with like.
  • Output validation.
    • validateOutput accepts a split only when exact is true and criteria and questions are empty.
    • It also requires the moved numbers to equal the held numbers and at least one item to stay.
    • A split from a model that is not exact, or for an issue that was not held, is rejected before any write.
    • The new split key is required in the schema, in validateOutput, and in the callers.test.mjs assertion.
  • Write order and retry.
    • applyRefinement re-checks the App's hold, then files or finds the child, then re-reads the body.
    • It then sets the labels, edits the body, and posts the marker comment last.
    • A failure before the body edit leaves the issue held. The next triage hits the same split and reuses the child, matched on App author, repository and identical body.
  • Nested items.
    • ownedLines, movedTexts and splitBody move a moved item's deeper-indented lines with it, and each nested line moves once.
    • splitOf refuses a split that would carry a nested task-list line the lane can run.
  • Closing issue feat(pr-pipeline): decide whether refinement files the split for a lane-unrunnable done-when #9396, option 1 as decided by the owner. Each part is implemented and has a test.
    • It files one needs-human child holding only the unrunnable items.
    • It rewrites the parent's done-when to the remaining items plus a link to the child.
    • It clears status: needs-decision so admission re-runs.
    • It runs at most once per issue, keyed on REFINE_MARKER.
  • Embedded instructions. None found in the PR body or the issue sections.
  • Manifests. The diff changes no plugin.json or marketplace.json, so there are no manifest <field> lines. The changelog fragment's bump: minor fits a new capability.

Hold. None.

  • Cost: the existing refine-issue model run gets a longer prompt and one more schema field. There is no new model call, trigger, model, or limit.
  • Trust-gate: none of the trusted-actors files, check-trusted-trigger, CODEOWNERS or .gitmodules are touched.
  • Critical-security: nothing at that level. The PR says it widens the apply-refinement activity's scope (it now creates an issue and edits a body). The workflow's token grant is unchanged.

Not reviewed. No live run exists. The PR says canary #9378 is the first real case after merge.

@github-actions github-actions Bot added the needs-human Human-in-the-loop required; autonomous sessions must not resolve items carrying this. label Oct 23, 2026
@github-actions

Copy link
Copy Markdown
Contributor

claude-review holds this pull request for a person (cost) at 3a7982aaf6399d63d7449be38d42c706b53c81cc.

.github/scripts/intake-triage/refine.mjs (selectRefinement/splitOf) and .github/workflows/intake-triage.yml (refine-select now passes BASE_PATH) widen which issues the existing refine-issue model job runs on. It now also starts for any issue held at the lanes App's status: needs-decision for some, not all, unrunnable done-when items. Each such issue gets one extra model run, so model spend rises. Per the PR, apply-refinement also now creates an issue and edits an issue body, so the owner should confirm the cost and scope change.

claude-lane: claude-review

kyle-sexton and others added 3 commits October 23, 2026 12:02
splitBody let a nested `- [ ]` line's own item number overwrite its
moved parent's claim, so the parent kept the nested checks while the
child also got them. Every line a moved item owns now leaves the parent.

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

claude-lane: claude-review

@kyle-sexton

Copy link
Copy Markdown
Member Author

Decision recorded. Cost: approve the extra model run per held issue.

@kyle-sexton kyle-sexton removed the needs-human Human-in-the-loop required; autonomous sessions must not resolve items carrying this. label Oct 24, 2026
kyle-sexton and others added 2 commits October 24, 2026 00:09
…le nested line

taskItems numbers a nested task-list line as its own item, while splitBody
and movedTexts move it with its parent. splitOf now returns null when a
moved item owns a nested task-list line the lane can perform, so no item
moves unasked and an item stays exactly when one is not moved. movedTexts
skips an item nested under another moved item, so the child body holds
each line once.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kyle-sexton
kyle-sexton added this pull request to the merge queue Oct 24, 2026
Merged via the queue into main with commit bf85a3b Oct 24, 2026
35 checks passed
@kyle-sexton
kyle-sexton deleted the feat/refine-files-unrunnable-split branch October 24, 2026 04:18
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-pipeline): decide whether refinement files the split for a lane-unrunnable done-when

1 participant