diff --git a/.changes/pr-pipeline/fix-uncertain-thread-needs-session-5d2b8e61.md b/.changes/pr-pipeline/fix-uncertain-thread-needs-session-5d2b8e61.md new file mode 100644 index 00000000000..001dd694e9d --- /dev/null +++ b/.changes/pr-pipeline/fix-uncertain-thread-needs-session-5d2b8e61.md @@ -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. diff --git a/.github/actions/report-check-run/report-check-run.test.mjs b/.github/actions/report-check-run/report-check-run.test.mjs index 0be36d756fa..ad171f6009d 100644 --- a/.github/actions/report-check-run/report-check-run.test.mjs +++ b/.github/actions/report-check-run/report-check-run.test.mjs @@ -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")); diff --git a/.github/scripts/pr-unblock/address-feedback.mjs b/.github/scripts/pr-unblock/address-feedback.mjs index 04cba0bc582..38716862a8d 100644 --- a/.github/scripts/pr-unblock/address-feedback.mjs +++ b/.github/scripts/pr-unblock/address-feedback.mjs @@ -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 @@ -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. @@ -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 = ``; +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, @@ -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 }; diff --git a/.github/scripts/pr-unblock/address-feedback.test.mjs b/.github/scripts/pr-unblock/address-feedback.test.mjs index 7f6eef3380e..1d79cb4bf39 100644 --- a/.github/scripts/pr-unblock/address-feedback.test.mjs +++ b/.github/scripts/pr-unblock/address-feedback.test.mjs @@ -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 @@ -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: `\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) => { diff --git a/.github/scripts/pr-unblock/escalation-routes.json b/.github/scripts/pr-unblock/escalation-routes.json index 48ed6042da7..7111156cd4c 100644 --- a/.github/scripts/pr-unblock/escalation-routes.json +++ b/.github/scripts/pr-unblock/escalation-routes.json @@ -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" }, diff --git a/.github/scripts/pr-unblock/report-escalation.test.sh b/.github/scripts/pr-unblock/report-escalation.test.sh index ba7459eb10c..726e07710e2 100755 --- a/.github/scripts/pr-unblock/report-escalation.test.sh +++ b/.github/scripts/pr-unblock/report-escalation.test.sh @@ -148,7 +148,8 @@ 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 @@ -156,10 +157,11 @@ HOLD_REASONS=(feedback-round-cap feedback-uncertain feedback-disputed 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" diff --git a/docs/contracts/loop-lane/README.md b/docs/contracts/loop-lane/README.md index 6d0cd1b3654..f0e6167b33c 100644 --- a/docs/contracts/loop-lane/README.md +++ b/docs/contracts/loop-lane/README.md @@ -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)). diff --git a/docs/conventions/pr-pipeline/README.md b/docs/conventions/pr-pipeline/README.md index f0ac7b6ed20..f6b83aa2252 100644 --- a/docs/conventions/pr-pipeline/README.md +++ b/docs/conventions/pr-pipeline/README.md @@ -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 @@ -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 diff --git a/docs/conventions/pr-pipeline/pr-pipeline.schema.json b/docs/conventions/pr-pipeline/pr-pipeline.schema.json index 3c7eeb19051..bd98eebeb92 100644 --- a/docs/conventions/pr-pipeline/pr-pipeline.schema.json +++ b/docs/conventions/pr-pipeline/pr-pipeline.schema.json @@ -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." }, diff --git a/docs/conventions/pr-pipeline/pr-run-activity.md b/docs/conventions/pr-pipeline/pr-run-activity.md index e719aeea5d1..e1390c5fcab 100644 --- a/docs/conventions/pr-pipeline/pr-run-activity.md +++ b/docs/conventions/pr-pipeline/pr-run-activity.md @@ -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`