Skip to content

fix(pr-unblock): route a lane-answered open thread to the session queue - #11796

Merged
kyle-sexton merged 2 commits into
mainfrom
fix/pr-unblock-uncertain-needs-session
Nov 1, 2026
Merged

kyle-sexton merged 2 commits into
mainfrom
fix/pr-unblock-uncertain-needs-session

Conversation

@kyle-sexton

@kyle-sexton kyle-sexton commented Nov 1, 2026 •

Copy link
Copy Markdown
Member

No related issue: lane defect measured in a census of 1528 PRs (2026-10-25..11-01)

Summary

When address-feedback answers a review thread UNCERTAIN, VALID-defer or INCORRECT and
leaves it open, the next round drops that thread (threadAuthors: the lane's own reply is the
last comment) and keeps none. select then escalates feedback-untaken-threads. That reason, and
feedback-unadjudicated, routed fail with no label, so the stop landed in no queue and the PR
waited until the owner answered by hand.

Census (1528/1528 PRs updated 2026-10-25..11-01): 11 PRs carried 19 feedback-untaken-threads
statuses caused by an UNCERTAIN-last thread (#10583, #10802, #10868, #10900, #11072, #11457,
#11519, #11520, #11697, #11733, #11752). Across the window, 25 of 45 current-form UNCERTAIN
threads got an owner reply after the lane's. The count is a lower bound: a commit keeps only the
newest status per context.

Fix

  • escalation-routes.json: feedback-untaken-threads and feedback-unadjudicated now name
    "label": "needs-session", like feedback-protected-path and ci-fix-escalated. Each why
    says a session builder takes it. report-escalation.sh already adds that label before the
    lane-stopped comment, and clear-escalations.mjs already lifts it (SESSION_LABELS is derived
    from the table).
  • address-feedback.mjs select: an unresolved thread the lane does not take skips the new
    awaiting-session reason instead of escalating again, when sessionQueued holds. That needs
    the PR to carry the label and the lanes App's address-feedback lane-stopped comment to name
    this head with a reason routed to that label. The check reads pull.labels and
    issueComments, which select already fetches, so it adds no API call.
    • It runs after the gated (awaiting-human) and owner-PR (not-applicable) branches, and only
      when a thread is untaken. A head with no unresolved thread still skips stop-resolved with
      resolved: true, so a session that resolves the thread clears the failing status.
    • It requires this head's own address-feedback stop. A needs-session added by another
      activity (for example ci-fix-escalated) does not silence this stop. On a new head (no
      status there, so clear-escalations may lift the label), the lane escalates again and
      re-queues the PR.
  • pr-pipeline.schema.json $defs/skip-reason adds awaiting-session, since decide.mjs
    posts only enum members. The README's "Outputs and skips" section describes it.
  • Docs: the routing paragraph in docs/conventions/pr-pipeline/README.md#activities, the
    needs-session line in docs/contracts/loop-lane/README.md, pr-run-activity.md, and a
    changelog fragment.

Verification

  • Fail before. I restored the original address-feedback.mjs and escalation-routes.json
    and ran the new tests against them. The route test failed (actual: ['fail', undefined],
    expected ['fail', 'needs-session']). The stand-down test failed (actual: ['escalate', undefined, 'feedback-untaken-threads', undefined], expected ['skip', 'awaiting-session', ...]).
  • New tests in address-feedback.test.mjs:
    • The route and the stand-down, for both reasons.
    • Five controls that still escalate: no label, a stop on an older head, a stop comment not by
      the lanes App, a stop reason that names no queue label, and another activity's queued stop.
    • A queued stop with every thread resolved still skips stop-resolved with resolved: true.
    • needs-human still wins (awaiting-human).
  • Other test updates. report-check-run.test.mjs posts awaiting-session as neutral under
    the schema's enum. report-escalation.test.sh now has both reasons in its exact
    SESSION_REASONS set.
  • node --test runs:
    • address-feedback, adjudicate-feedback, publish-feedback, feedback-dispatch, gate-source,
      clear-escalations, clear-escalations-stale-hold, report-check-run, decide and
      maintenance-monitor-signals collect: 879 pass, 1 fail at first. The failure was
      feedback-dispatch's ownerAdoption reads the base adoption section, caused by a missing
      npm ci in .github/actions/resolve-config/ (ajv not found), not by main. At head
      a6d2263 with dependencies installed: feedback-dispatch and address-feedback 373 pass,
      0 fail; origin/main passes the same file 191/191.
    • resolve-config live-config and callers: 259 pass.
  • bash report-escalation.test.sh: 916 ok, exit 0.
  • scripts/check-affected.sh --tests: 12 passed, 4 skipped. It left the 77 selected suites
    to CI.
  • Nothing here surfaces the PR to the owner.
    • A session route adds only the entry's label, needs-session
      (.github/scripts/pr-unblock/report-escalation.sh:286-287). The human-gated label goes on
      only in the hold branch (report-escalation.sh:325-326), and neither reason routes hold
      (escalation-routes.json:18, :23).
    • The lane-stopped comment template mentions no account
      (report-escalation.sh:269-283).
    • The new skip posts only a neutral check (address-feedback.mjs:1064-1065) and writes no
      label or comment.
    • Nothing classes lane stops beyond this routing table. report-escalation.sh and
      clear-escalations.mjs read the route and label from it, and the morning brief counts queue
      labels and work-items:escalation markers, not lane-stopped reasons. So the label makes
      these session-builder stops everywhere they are read.

Related

  • awaiting-session leaves the monitoring lane's stalled lane stop signal unchanged
    (maintenance-monitor-signals/collect.mjs): a Skipped: check is not an attempt, so a
    queued stop older than two hours is still listed, as feedback-untaken-threads was before.
  • plugins/source-control/skills/** is unchanged.

🤖 Generated with Claude Code

kyle-sexton and others added 2 commits October 31, 2026 21:56
A thread address-feedback answered UNCERTAIN, VALID-defer or INCORRECT
stays open; the next round drops it and escalates
feedback-untaken-threads, a failing status with no queue label, so the
PR waits on the owner. feedback-untaken-threads and
feedback-unadjudicated now add needs-session, and the select job skips
awaiting-session instead of escalating again while this head's
address-feedback stop already sits in that queue.

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 02:54
@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-01T02:56:39.862767Z a6d2263 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
Contributor

claude-security-review reviewed a6d22636c707fe3d320de52eb2ce3470b96b14d9: read 10 of 10 changed file(s); 0 finding(s), 0 as inline comments.

Security review: no security issues found

I read every hunk of all 10 changed files. Two of them are on trusted paths: .github/scripts/pr-unblock/address-feedback.mjs and .github/scripts/pr-unblock/escalation-routes.json. Both are merge-hold controls, because clear-escalations.mjs imports the routing table. I read their whole change and the code they gate.

Merge holds: this change does not weaken any

  • Routing change (escalation-routes.json). feedback-untaken-threads and feedback-unadjudicated still route fail. The only change is the added "label": "needs-session". As a result, report-escalation.sh now also puts on a label that ci-status fails on. That makes the merge stricter, not looser.
  • When the label comes off. I checked the conditions in sessionVerdict:
    • The label comes off only if the lanes App added it last.
    • On the same head, the stop's pr-unblock/address-feedback status must read success and be newer than the stop.
    • On a new head, no failing lane status and no failing checks may remain.
    • Before this change these reasons put on no label at all. The failing commit status was the only thing holding the head, and that still applies.
  • New awaiting-session skip (address-feedback.mjs L1062-L1066).
    • It does not set resolved: true.
    • pr-unblock.yml only clears the failing status when stop-resolved == 'true' (L2818), so the head keeps its failing status.
    • The skip runs after the awaiting-human and owner-PR branches, so a needs-human hold still takes precedence.

Can the skip be triggered by spoofing?

No. sessionQueued (L955-L966) only counts a stop when all of these hold:

  • the comment passes isLanesApp(comment.user);
  • its first line is the exact lane-stopped marker for this activity;
  • it names the current head as a full 40-character SHA;
  • its reason is a key that Object.hasOwn(ROUTES, …) accepts, and that reason's route names a label the PR actually carries.

A comment from an outsider can't satisfy the author check. Adding labels requires triage access or higher. Even a forged match would only stop the lane from escalating again; the failing status from the earlier stop would still hold the merge.

Other checks

  • No change to token scope, the trusted-actor list, the kill switch or Actions permissions. Nothing reads or writes secrets.
  • The new code doesn't interpolate any comment text into a command, and doesn't log it.
  • The schema enum gains awaiting-session. The docs, tests and changelog changes have no security surface.
  • The PR context contains no instructions aimed at this review.

critical-findings: 0

@github-actions

github-actions Bot commented Nov 1, 2026

Copy link
Copy Markdown
Contributor

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

Code review: no findings

The PR adds "label": "needs-session" to the feedback-untaken-threads and feedback-unadjudicated routes. It also adds a sessionQueued check to select, so a head whose address-feedback stop is already queued skips the new awaiting-session reason instead of escalating again. That reason is also added to the schema's skip-reason enum. I found no correctness or maintainability problems. I read every hunk of all 10 changed files.

What I checked against the surrounding code (read only; I could not run anything):

  • Comment parsing. sessionQueued (address-feedback.mjs:955-966) parses the comment that report-escalation.sh:269-283 writes: the marker on the first line, then the - Reason: and - Head: lines. It accepts only comments from the lanes App (isLanesApp: matching id and type: "Bot"), the address-feedback activity, this exact head SHA, and a reason whose route names a label the PR carries. CRLF is normalized before the anchored /m regexes run. Comments from other activities (for example ci-fix-escalated) or older heads fall through and still escalate.
  • Order of checks. The new check runs after the owner Hold:, gated (awaiting-human) and owner-PR (not-applicable) branches, and only when untaken. A head with no unresolved thread still returns stop-resolved with resolved: true. In pr-unblock.yml:2818, only stop-resolved == 'true' clears the status, so the new skip leaves the failing pr-unblock/address-feedback status in place, as intended.
  • No re-dispatch loop. The new skip posts Skipped: awaiting-session on both the address-feedback and publish-feedback checks (pr-unblock.yml:901-927). That title is not in feedback-dispatch.mjs RELEASES (:217), and selectStopped (:565) still sees the earlier lane-stopped check. So the skip does not release the head for another round.
  • Label clearing. clear-escalations.mjs sessionVerdict (:1537-1610) decides from commit statuses and lane-stopped comments, not check titles, so the neutral skip does not change when needs-session is lifted. SESSION_LABELS is derived from the routes table, so the two reasons are picked up automatically.
  • Reporting. main passes skipReason straight to the skip-reason output (:1657). report-check-run validates the reason against the base schema enum, which this PR extends, and the new test covers it.
  • Tests. The new address-feedback tests cover the route, the stand-down for both reasons, the five cases that still escalate, the resolved case and the person-hold case. fakeApi maps the label strings to { name }, so the tests exercise the real pull.labels shape. report-escalation.test.sh moves both reasons from FAIL_REASONS to SESSION_REASONS, which matches the JSON.

Unverified: I did not run the test results in the PR description (879 pass / 1 fail, 916 ok, and so on). They are the author's claims. The one failing test the author reports (ownerAdoption reads the base adoption section) is outside this diff, and I did not check it.

Other checks:

  • No closing issue (the PR says "No related issue"), so there are no acceptance criteria to check.
  • No plugin manifest changes.
  • The changelog fragment is bump: patch, which fits a fix.
  • No embedded instructions in the PR context.
  • Spend: none beyond the current lane runs. The change only skips a repeat escalation on the same head.

@kyle-sexton
kyle-sexton added this pull request to the merge queue Nov 1, 2026
Merged via the queue into main with commit 48ad7c7 Nov 1, 2026
87 checks passed
@kyle-sexton
kyle-sexton deleted the fix/pr-unblock-uncertain-needs-session branch November 1, 2026 03:14
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