Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
---
bump: patch
---

### Fixed

- A review thread the address-feedback lane answered `UNCERTAIN`, `VALID-defer` or `INCORRECT`
and left open no longer stalls the pull request with no queue. `feedback-untaken-threads` and
`feedback-unadjudicated` now add `needs-session` beside their failing status, so a session
builder takes the thread. While that stop stands on the head, the select job skips the new
`awaiting-session` reason instead of escalating again; it still clears the stop
(`stop-resolved`) once no thread is unresolved, and a new head escalates afresh.
9 changes: 9 additions & 0 deletions .github/actions/report-check-run/report-check-run.test.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -357,6 +357,15 @@ test("a stop-resolved skip POSTs neutral under the schema's enum", async () => {
assert.equal(calls[0].body.output.title, "Skipped: stop-resolved");
});

test("an awaiting-session skip POSTs neutral under the schema's enum", async () => {
const verdict = { ...JSON.parse(readFileSync(fixture("verdict-skipped.json"), "utf8")), "skip-reason": "awaiting-session", outcome: "skipped" };
const verdictPath = path.join(dir, "verdict.json");
writeFileSync(verdictPath, JSON.stringify(verdict));
const { calls } = await run({ VERDICT_PATH: verdictPath, ACT_OUTCOME: "skipped" });
assert.equal(calls[0].body.conclusion, "neutral");
assert.equal(calls[0].body.output.title, "Skipped: awaiting-session");
});

test("the enum is read from base-path: a base schema without the reason POSTs failure", async () => {
const schemaPath = path.join(basePath, SCHEMA_REL);
const schema = JSON.parse(readFileSync(schemaPath, "utf8"));
Expand Down
29 changes: 27 additions & 2 deletions .github/scripts/pr-unblock/address-feedback.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,9 @@
// the label on and no review lane holds the PR (gate-source.mjs),
// and on an adopted owner PR only as prHold lets fix-ci work it.
// On an adopted owner PR a body line starting `Hold:` skips
// awaiting-human.
// awaiting-human. An untaken thread whose stop on this head is
// already in the session-builder queue (sessionQueued) skips
// awaiting-session instead of escalating again.
//
// Every comment body is copied into the thread file and never logged or
// interpolated into a command. Only a trusted person's reply to the lane's
Expand All @@ -45,7 +47,7 @@ import { fileURLToPath, pathToFileURL } from "node:url";
import { commentItems, FAILING, gateSource, laneHold, latestRuns, listLabelEvents, prHold, ROUND_MARKER } from "./gate-source.mjs";
import { retryFetch } from "./retry-fetch.mjs";
import { isProtectedPath, namedInScope, prPathsOf } from "./thread-scope.mjs";
import { retryRung, stopSkip } from "./escalation-routes.mjs";
import { retryRung, ROUTES, stopSkip } from "./escalation-routes.mjs";

// The round cap is the config's `loop.review-rounds`, passed in as
// REVIEW_ROUNDS; the resolver's default is the fallback.
Expand Down Expand Up @@ -945,6 +947,24 @@ export function keepThreads({ threads, reviewComments, reviews, heads, security
return kept;
}

// Whether this head's address-feedback stop sits in the session-builder
// queue: the lanes App's lane-stopped comment for the activity (one, edited in
// place, report-escalation.sh) names `headSha` and a reason whose
// escalation-routes.json entry names a `label` the PR carries (needs-session).
const LANE_STOPPED = `<!-- pr-unblock:lane-stopped activity=${ACTIVITY} -->`;
export function sessionQueued({ labels = [], issueComments, headSha, isLanesApp }) {
const names = new Set(labels.map((label) => label.name));
return issueComments.some((comment) => {
const body = String(comment.body ?? "").replaceAll("\r\n", "\n");
if (!isLanesApp(comment.user) || firstLine(body) !== LANE_STOPPED) {
return false;
}
const reason = /^- Reason: `([^`]*)`$/m.exec(body)?.[1] ?? "";
const label = Object.hasOwn(ROUTES, reason) ? ROUTES[reason].label : undefined;
return /^- Head: `([0-9a-f]{40})`$/m.exec(body)?.[1] === headSha && typeof label === "string" && names.has(label);
});
}

export async function select({
api,
pr,
Expand Down Expand Up @@ -1039,6 +1059,11 @@ export async function select({
if (untaken && owner) {
return { decision: "skip", skipReason: "not-applicable", threads: kept };
}
// A stop already queued for a session builder on this head stands down:
// escalating again would only re-post it. A new head escalates afresh.
if (untaken && sessionQueued({ labels: pull.labels, issueComments, headSha, isLanesApp })) {
return { decision: "skip", skipReason: "awaiting-session", threads: kept };
}
return untaken
? { decision: "escalate", escalationReason: "feedback-untaken-threads", threads: kept }
: { decision: "skip", skipReason: "stop-resolved", resolved: true, threads: kept };
Expand Down
49 changes: 48 additions & 1 deletion .github/scripts/pr-unblock/address-feedback.test.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import { fileURLToPath, pathToFileURL } from "node:url";

import { ciSnapshot, claimsSecurity, coveringLanes, createGitHubApi, currentThreads, main, maxRoundsFrom, needsPerson, planLanes, runRecord, scanRefusal, select, trustedHumansFrom, VERIFY_NOTE_MARKER } from "./address-feedback.mjs";
import { failingChecks } from "./gate-source.mjs";
import { escalationRoute } from "./escalation-routes.mjs";
import { escalationRoute, ROUTES } from "./escalation-routes.mjs";
import { tempDir } from "../../../lib/test-tmp.mjs";

// Ids from .github/standards/trusted-actors/trusted-actors.json and the
Expand Down Expand Up @@ -1989,6 +1989,53 @@ test("an unresolved thread the lane does not take escalates feedback-untaken-thr
assert.deepEqual([held.decision, held.skipReason], ["skip", "awaiting-human"], "a person holds the PR");
});

// The census of 2026-10-25..11-01 (11 PRs, 19 failing statuses): a thread the
// lane answered UNCERTAIN, VALID-defer or INCORRECT stayed open, the next
// round dropped it, and feedback-untaken-threads failed the head with no
// queue label, so the PR waited on the owner. Both stops now queue the PR
// for a session builder (needs-session), and while that stop stands on the
// head the select job stands down without escalating again.
test("feedback-untaken-threads and feedback-unadjudicated queue the PR for a session builder", () => {
for (const reason of ["feedback-untaken-threads", "feedback-unadjudicated"]) {
assert.deepEqual([ROUTES[reason].route, ROUTES[reason].label], ["fail", "needs-session"], reason);
}
});

const laneStopped = (reason, { head = HEAD, user = APP_USER, activity = "address-feedback" } = {}) => ({
id: 5,
user,
body: `<!-- pr-unblock:lane-stopped activity=${activity} -->\r\nThe pr-unblock lane stopped \`${activity}\` on this pull request: why.\r\n\r\n- Reason: \`${reason}\`\r\n- Head: \`${head}\`\r\n`,
});
const answered = { reviewComments: [comment(100, REVIEWER), comment(101, HUMAN)], threads: [thread("PRRT_1", [100, 101])] };

test("an untaken thread whose stop on this head is in the session-builder queue skips awaiting-session", async () => {
for (const reason of ["feedback-untaken-threads", "feedback-unadjudicated"]) {
const result = await run(fakeApi({ ...answered, labels: ["needs-session"], issueComments: [reviewedHead, laneStopped(reason)] }));
assert.deepEqual([result.decision, result.skipReason, result.escalationReason, result.resolved], ["skip", "awaiting-session", undefined, undefined], reason);
}
});

test("an untaken thread escalates again unless this head's own address-feedback stop is queued", async () => {
for (const [label, overrides] of [
["no needs-session label", { issueComments: [reviewedHead, laneStopped("feedback-untaken-threads")] }],
["a stop on an older head", { labels: ["needs-session"], issueComments: [reviewedHead, laneStopped("feedback-untaken-threads", { head: OLD_HEAD })] }],
["a stop comment not by the lanes App", { labels: ["needs-session"], issueComments: [reviewedHead, laneStopped("feedback-untaken-threads", { user: { id: STRANGER, type: "User" } })] }],
["a stop whose reason names no queue label", { labels: ["needs-session"], issueComments: [reviewedHead, laneStopped("feedback-stood-open")] }],
["another activity's queued stop", { labels: ["needs-session"], issueComments: [reviewedHead, laneStopped("ci-fix-escalated", { activity: "fix-ci" })] }],
]) {
const result = await run(fakeApi({ ...answered, ...overrides }));
assert.deepEqual([result.decision, result.escalationReason], ["escalate", "feedback-untaken-threads"], label);
}
});

test("a queued stop still clears once no thread is unresolved, and a person's hold still wins", async () => {
const queued = { labels: ["needs-session"], issueComments: [reviewedHead, laneStopped("feedback-untaken-threads")] };
const resolved = await run(fakeApi({ ...queued, threads: [thread("PRRT_1", [100, 101], { isResolved: true })] }));
assert.deepEqual([resolved.decision, resolved.skipReason, resolved.resolved], ["skip", "stop-resolved", true]);
const held = await run(fakeApi({ ...answered, ...queued, labels: ["needs-session", "needs-human"] }));
assert.deepEqual([held.decision, held.skipReason], ["skip", "awaiting-human"]);
});

// pr-feedback-dispatch.yml sends verify-pass-head after a pr-verify pass at
// the PR's head; the select job then lists only the stale fail-note threads.
test("main select with a pass head lists the stale fail-note threads for the reply job and keeps none for a model", async (t) => {
Expand Down
4 changes: 2 additions & 2 deletions .github/scripts/pr-unblock/escalation-routes.json
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,12 @@
"feedback-not-landed": { "activity": "address-feedback", "route": "fail", "why": "a fix the lane recorded did not reach the branch, so none of its review replies was posted" },
"feedback-invalid": { "activity": "address-feedback", "route": "fail", "why": "the review replies the lane recorded did not pass validation, so none was posted" },
"feedback-partial": { "activity": "address-feedback", "route": "fail", "why": "posting the review replies stopped partway, or a fixed thread could not be resolved" },
"feedback-untaken-threads": { "activity": "address-feedback", "route": "fail", "why": "the pull request has an unresolved review thread the lane does not take (a reviewer it does not act on, a review of an older head, or a thread it already answered), so nothing will resolve it" },
"feedback-untaken-threads": { "activity": "address-feedback", "route": "fail", "label": "needs-session", "why": "the pull request has an unresolved review thread the lane does not take (a reviewer it does not act on, a review of an older head, or a thread it already answered), so no lane resolves it and a session builder takes it" },
"feedback-publish-failed": { "activity": "address-feedback", "route": "fail", "why": "the reply job failed before it decided anything (a lane token it could not get, or a script error), so no reply was posted" },
"feedback-incomplete": { "activity": "address-feedback", "route": "fail", "why": "the lane did not record a disposition for every review thread it kept, so none was posted" },
"feedback-disputed": { "activity": "address-feedback", "route": "hold", "why": "a disputed review finding is one only a person decides (a CRITICAL finding, a security-review finding with no rating the lane reads, or a thread a person opened or directed)" },
"feedback-stood-open": { "activity": "address-feedback", "route": "fail", "why": "an adjudicator found a review finding stands, and the next round left it open again" },
"feedback-unadjudicated": { "activity": "address-feedback", "route": "fail", "why": "the lane left a review finding open and no adjudicator verdict settled it: none was recorded, it found the finding does not hold where the lane had not disputed it (an UNCERTAIN or VALID-defer reply), which only an INCORRECT reply lets a verdict resolve, it found the finding asks only for tests whose covering check runs had not all passed when the round began, or it raised a security question on a finding the security-review lane rated below CRITICAL" },
"feedback-unadjudicated": { "activity": "address-feedback", "route": "fail", "label": "needs-session", "why": "the lane left a review finding open and no adjudicator verdict settled it: none was recorded, it found the finding does not hold where the lane had not disputed it (an UNCERTAIN or VALID-defer reply), which only an INCORRECT reply lets a verdict resolve, it found the finding asks only for tests whose covering check runs had not all passed when the round began, or it raised a security question on a finding the security-review lane rated below CRITICAL; so a session builder takes it" },
"feedback-question-intent": { "activity": "address-feedback", "route": "hold", "why": "an adjudicator found that a disputed review finding turns on what the change is meant to do, which a person decides" },
"feedback-question-design": { "activity": "address-feedback", "route": "hold", "why": "an adjudicator found that a disputed review finding turns on a design choice, which a person decides" },
"feedback-question-security": { "activity": "address-feedback", "route": "hold", "why": "an adjudicator found that a disputed review finding, other than a security-review finding rated below CRITICAL, turns on a security judgment, which a person decides" },
Expand Down
8 changes: 5 additions & 3 deletions .github/scripts/pr-unblock/report-escalation.test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -148,18 +148,20 @@ context_of() {
# status, and SESSION_REASONS, a change the lane cannot make (a protected
# path, a weakened required check after its fresh fixer round, a check the
# rung-up retry could not fix, a dependency update the review could not
# finish), also put the PR in the session-builder
# finish, a review thread the lane left open with no verdict to settle it),
# also put the PR in the session-builder
# queue with needs-session instead of in front of a person.
HOLD_REASONS=(feedback-round-cap feedback-uncertain feedback-disputed
feedback-question-intent feedback-question-design feedback-question-security
dependency-security-close dependency-escalate dependency-fix-refused)
SESSION_REASONS=(protected-path-conflict conflict-patch-protected-path
feedback-protected-path ci-fix-protected-path ci-fix-weaken-required ci-fix-escalated
dependency-round-cap dependency-fix-not-landed dependency-config-unreadable
dependency-files-truncated dependency-control-uncited dependency-workflow-unpinned)
dependency-files-truncated dependency-control-uncited dependency-workflow-unpinned
feedback-untaken-threads feedback-unadjudicated)
FAIL_REASONS=(workflow-staged unexpected-path-staged repeat-cap unresolved-conflict conflict-patch-invalid conflict-patch-not-applied
feedback-not-converging feedback-verifier-open feedback-not-landed feedback-invalid feedback-partial
feedback-incomplete feedback-stood-open feedback-unadjudicated feedback-untaken-threads feedback-publish-failed
feedback-incomplete feedback-stood-open feedback-publish-failed
dependency-invalid
ci-fix-round-cap ci-fix-empty ci-fix-invalid ci-fix-not-applied ci-fix-weaken-retry)
ROUTES="$SCRIPT_DIR/escalation-routes.json"
Expand Down
3 changes: 2 additions & 1 deletion docs/contracts/loop-lane/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -402,7 +402,8 @@ Three labels take work off the unattended path, and each means one thing (owner

`needs-session` is the session-builder queue, not an owner stop: a pr-unblock stop on a change the
lane cannot make (a protected path, a weakened required check, a check the `fix-ci` rung-up could
not fix, an unfinished dependency review)
not fix, an unfinished dependency review, a review thread the lane left open with no verdict to
settle it)
goes to an agent session, which removes the label once it has made the change; pr-merge's
`clear-escalations` sweep removes it when a same-head retry cleared the stop
([routing table](../../conventions/pr-pipeline/README.md#activities)).
Expand Down
9 changes: 7 additions & 2 deletions docs/conventions/pr-pipeline/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -713,7 +713,10 @@ entry that names `"label": "needs-session"` is a change the lane cannot make: a
not push, a fix that weakens a required check after its fresh fixer round, a failing check the
`fix-ci` rung-up retry could not fix, or a dependency update
the review could not finish (its cap, an unlanded fix, an unreadable config, a cut-short listing,
an uncited control, a workflow change beyond a pin swap). `review-dependency` and
an uncited control, a workflow change beyond a pin swap), or a review thread the lane left open
that no lane or verdict will settle (`feedback-untaken-threads`, `feedback-unadjudicated`; while
that stop stands on the head, `address-feedback`'s select job skips `awaiting-session` instead of
escalating again). `review-dependency` and
`apply-dependency-patch` do not apply while `needs-session` is on (their `labels-absent` in
`pr-pipeline.yaml`), so such a stop gets no same-head model re-run and no fresh round budget on a
rebased head until a session removes the label, or `clear-escalations.mjs` does: once a same-head
Expand Down Expand Up @@ -948,7 +951,9 @@ job's activity check it ends the head; on a push, reply or verdict job's check t
retries the head under its per-head cap), `pass-only` (a pass-only address-feedback round's reply
job, which ran no model and only resolved fail-note threads a pr-verify pass superseded; the
dispatcher's release reads skip it), `stop-resolved` (an address-feedback or fix-ci select job found
no stop standing on its head, whether or not one ever did), or, for an issue run under
no stop standing on its head, whether or not one ever did), `awaiting-session` (an address-feedback
select job whose head's stop already sits in the `needs-session` queue, so it does not escalate
again), or, for an issue run under
`models.issue-dimensions`, `research-first` or `probe-first` (its Uncertainty), or
`split-required` (its Size is `split`). Issue mode posts no check, so work-resolve-issue.yml reports those three dimension skips
on the issue instead, to no person: `research-first` and `probe-first` lose `agent-ready` and
Expand Down
3 changes: 2 additions & 1 deletion docs/conventions/pr-pipeline/pr-pipeline.schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -510,7 +510,8 @@
"clean-merge",
"lane-stopped",
"pass-only",
"stop-resolved"
"stop-resolved",
"awaiting-session"
],
"description": "The one reason a skipped activity's neutral check carries. Not used by the config file; the runner and check-run reporter validate reasons against it. Meaning: README.md, Outputs and skips."
},
Expand Down
7 changes: 5 additions & 2 deletions docs/conventions/pr-pipeline/pr-run-activity.md
Original file line number Diff line number Diff line change
Expand Up @@ -960,8 +960,11 @@ the base's `.github/scripts/pr-unblock/address-feedback.mjs`, `apply-patch.mjs`
those replies is such a fix request by a write-access human, and
reports `human-gate=fix-request`, which lifts only the human-gated hold of the push job. No kept
thread skips `awaiting-human` on such a PR; elsewhere it escalates `feedback-untaken-threads`
(routed to a failing status) while any review thread is unresolved, and skips `not-applicable`
only when none is. The dispatcher (`feedback-dispatch.mjs`) dispatches an ungated head for any
(routed to a failing status and the `needs-session` queue) while any review thread is
unresolved, and skips `not-applicable` only when none is. While the PR carries `needs-session`
and the lanes App's address-feedback lane-stopped comment names this head with a reason routed
to it, an unresolved thread skips `awaiting-session` instead, so the stop stands without a
second escalation. The dispatcher (`feedback-dispatch.mjs`) dispatches an ungated head for any
unresolved thread, so such a head reaches this step once: its lane-stopped check then ends the
head, which a reply job's skip does not release. A failed reply job with no escalation reason
(its lane token, a script error) stops as `feedback-publish-failed`, which `pr-unblock.yml`
Expand Down
Loading