From e7e92beaa6a24a40fb0e4a1a3e811c21f2d95ebb Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Fri, 23 Oct 2026 10:06:21 -0400 Subject: [PATCH 1/4] feat(pr-pipeline): work lane-escalated needs-human PRs in address-feedback A needs-human label a lane escalation (pr-unblock or fix-ci) put on no longer stops address-feedback: the dispatcher and the select job admit the PR's reviewer-bot threads when the lanes App added the label, its escalation comment followed, and no review-lane hold names the current head or is open. A review-lane hold, or anything ambiguous, keeps the fix-request-only rule. The label stays on; clear-escalations owns it. The hold, escalation and label-window logic moves from clear-escalations.mjs into pr-unblock/gate-source.mjs, which both lanes now share. Co-Authored-By: Claude Opus 5.5 --- .../scripts/pr-merge/clear-escalations.mjs | 61 ++---- .../scripts/pr-unblock/address-feedback.mjs | 27 ++- .../pr-unblock/address-feedback.test.mjs | 50 +++++ .../scripts/pr-unblock/feedback-dispatch.mjs | 25 ++- .../pr-unblock/feedback-dispatch.test.mjs | 90 ++++++++- .github/scripts/pr-unblock/gate-source.mjs | 174 ++++++++++++++++++ .../scripts/pr-unblock/gate-source.test.mjs | 83 +++++++++ .github/workflows/pr-run-activity-write.yml | 6 +- .github/workflows/pr-unblock.yml | 3 +- docs/conventions/pr-pipeline/README.md | 7 +- 10 files changed, 468 insertions(+), 58 deletions(-) create mode 100644 .github/scripts/pr-unblock/gate-source.mjs create mode 100644 .github/scripts/pr-unblock/gate-source.test.mjs diff --git a/.github/scripts/pr-merge/clear-escalations.mjs b/.github/scripts/pr-merge/clear-escalations.mjs index b36a852b18..83dfc4b83a 100755 --- a/.github/scripts/pr-merge/clear-escalations.mjs +++ b/.github/scripts/pr-merge/clear-escalations.mjs @@ -64,14 +64,25 @@ import { pathToFileURL } from "node:url"; import { createGitHub, paginate } from "../../actions/check-trusted-trigger/check-trusted-trigger.mjs"; import { humanGatedLabelFrom } from "../pr-unblock/address-feedback.mjs"; +import { + CLEARED, + ESCALATION, + escalationsSince, + foreignMarker, + isApp, + labelHistory, + laneHold, + lines, + openReviewHold, + PAIRED_MS, + RESUME_KEY, + same, +} from "../pr-unblock/gate-source.mjs"; import { isProtectedPath } from "../pr-unblock/thread-scope.mjs"; const DEFAULT_BOT = "melodic-automation-lanes[bot]"; -const ESCALATION = ""; -const RESUME_KEY = /^$/; const REASON = /^- Reason: `([a-z-]+)`$/m; const clearedMarker = (reason, head) => ``; -const CLEARED = /^$/; // babysit_merge.py's verifier-refusal escalation, reasons comment and // shared-copy escalation: the marker on the first or second line, the next // line naming `at head ```. @@ -81,39 +92,18 @@ const PR_MERGE_MARKERS = Object.freeze([ "", ]); const AT_HEAD = / at head `([^`]*)`/g; -// A review lane's hold comment opens a line with this marker. The lanes post -// as claude[bot], or as github-actions[bot] with the lane's footer as the last -// non-blank line (address-feedback.mjs's lane comment rule). -const REVIEW_HOLD = "\n\nThe pr-unblock lane stopped.`, + created_at: "2026-10-18T10:00:05Z", + updated_at: "2026-10-18T10:00:05Z", +}; +const REVIEW_HOLD = { + id: 3, + user: { login: "github-actions[bot]", id: ACTIONS_BOT, type: "Bot" }, + body: `\nHeld.\n\nclaude-lane: claude-review`, + created_at: "2026-10-18T09:00:00Z", + updated_at: "2026-10-18T09:00:00Z", +}; +const APP_LABELED = [{ event: "labeled", actor: APP_USER, label: { name: "needs-human" }, created_at: "2026-10-18T10:00:00Z" }]; +const reviewedHead = { id: 1, user: { id: ACTIONS_BOT }, body: marker("claude-review", HEAD) }; + +test("on a needs-human PR a lane escalation gated, a reviewer-bot thread proceeds and releases the label hold", async () => { + const result = await run(fakeApi({ labels: ["needs-human"], issueComments: [reviewedHead, LANE_ESCALATION], labelEvents: APP_LABELED })); + assert.equal(result.decision, "proceed"); + assert.equal(result.fixRequested, true); + assert.equal(result.file.threads[0].root_comment_id, 100); +}); + +for (const [label, issueComments] of [ + ["a review-lane hold", [reviewedHead, REVIEW_HOLD]], + ["a lane escalation beside a review-lane hold", [reviewedHead, REVIEW_HOLD, LANE_ESCALATION]], + ["an escalation marker with no resume-key line", [reviewedHead, { ...LANE_ESCALATION, body: "\nStopped." }]], +]) { + test(`on a needs-human PR, a reviewer-bot thread under ${label} holds the PR (awaiting-human)`, async () => { + const result = await run(fakeApi({ labels: ["needs-human"], issueComments, labelEvents: APP_LABELED })); + assert.equal(result.decision, "skip"); + assert.equal(result.skipReason, "awaiting-human"); + }); +} + +test("without needs-human no label event is read", async () => { + const labelEventReads = []; + assert.equal((await run(fakeApi({ labelEventReads }))).decision, "proceed"); + assert.deepEqual(labelEventReads, []); +}); + test("without needs-human nothing changes and no permission is read", async () => { const api = fakeApi(escalated()); const result = await run(api); diff --git a/.github/scripts/pr-unblock/feedback-dispatch.mjs b/.github/scripts/pr-unblock/feedback-dispatch.mjs index b283682da4..4c8d4aafd3 100644 --- a/.github/scripts/pr-unblock/feedback-dispatch.mjs +++ b/.github/scripts/pr-unblock/feedback-dispatch.mjs @@ -22,8 +22,10 @@ // every comment is the reviewer bot's, a trusted person with write access // asked the lane to fix it (isFixRequest), or a trusted person answered the // lane's escalation (address-feedback.mjs threadAuthors); on a PR carrying -// the human-gated label, only a thread where a trusted person with write -// access asked the lane to fix it (isFixRequest); +// the human-gated label, the same when a lane escalation put the label on +// and no review lane holds the PR (gate-source.mjs gateSource), and +// otherwise only a thread where a trusted person with write access asked +// the lane to fix it (isFixRequest); // - that head has no pr-unblock / address-feedback check run yet (one the // usage limit stopped stops counting once its limit resets, and one that // started before the head's newest pr-unblock / publish-feedback check run @@ -60,6 +62,7 @@ import { pathToFileURL } from "node:url"; import { commitPulls } from "../../actions/check-trusted-trigger/check-trusted-trigger.mjs"; import { fixRequesters, humanGatedLabelFrom, threadAuthors, trustedHumansFrom } from "./address-feedback.mjs"; +import { commentItems, gateSource, listLabelEvents } from "./gate-source.mjs"; import { emptyRecord, isHandled, markHandled, readRecord, writeRecord } from "./dispatch-record.mjs"; import { anyHandles, limitResetsAt, permanentlyHandles } from "./limit-stop.mjs"; import { budgetedFetch } from "./request-budget.mjs"; @@ -227,8 +230,19 @@ export async function dispatchFeedback({ } const threads = await api.listReviewThreads(number); // A human-gated PR dispatches only for a thread a trusted person with - // write access asked the lane to fix. - const gated = (pull.labels ?? []).some((label) => label.name === humanGatedLabel); + // write access asked the lane to fix, unless a lane escalation put the + // label on and no review lane holds the PR. The label stays on either way: + // clear-escalations.mjs owns taking it off. + const labeled = (pull.labels ?? []).some((label) => label.name === humanGatedLabel); + const gate = labeled + ? gateSource({ + timeline: [...commentItems(comments), ...(await api.listLabelEvents(number))], + label: humanGatedLabel, + head, + isLanesApp: (user) => user?.id === appSenderId && user?.type === "Bot", + }) + : null; + const gated = labeled && gate.source !== "lane-escalation"; const fixers = await fixRequesters({ comments: threads.flatMap((thread) => thread.comments.nodes.map((node) => ({ authorId: node.author?.databaseId, login: node.author?.login, body: node.body })), @@ -240,7 +254,7 @@ export async function dispatchFeedback({ if (!hasWorkableThread(threads, { reviewerId: settings.reviewerId, appId: appSenderId, trustedHumans, requesters, fixers })) { return skip( gated - ? `#${number} is labeled ${humanGatedLabel} and has no thread a trusted person with write access asked the lane to fix` + ? `#${number} is labeled ${humanGatedLabel} (${gate.source}: ${gate.reason}) and has no thread a trusted person with write access asked the lane to fix` : `#${number} has no unresolved thread left by the reviewer bot alone, asked to be fixed, or answered by a trusted person`, ); } @@ -393,6 +407,7 @@ export function createGitHubApi({ token, repository, apiUrl, graphqlUrl, fetch = } throw new Error(`#${number} has more than 5000 comments`); }, + listLabelEvents: (number) => listLabelEvents({ graphql, repository, number }), async listReviewThreads(number) { const threads = []; let after = null; diff --git a/.github/scripts/pr-unblock/feedback-dispatch.test.mjs b/.github/scripts/pr-unblock/feedback-dispatch.test.mjs index d4943d19db..44eb9ebadc 100644 --- a/.github/scripts/pr-unblock/feedback-dispatch.test.mjs +++ b/.github/scripts/pr-unblock/feedback-dispatch.test.mjs @@ -87,8 +87,9 @@ function fakeApi({ workflowRuns = [], workflowRunId = 38779200001, permissions = { "kyle-sexton": "admin" }, + labelEvents = [], } = {}) { - const calls = { dispatch: [], permission: [] }; + const calls = { dispatch: [], permission: [], labelEvents: 0 }; return { calls, // The event's workflow_run, which the script takes in place of a read. @@ -110,6 +111,10 @@ function fakeApi({ }, listIssueComments: async () => comments, listReviewThreads: async () => threads, + async listLabelEvents() { + calls.labelEvents += 1; + return labelEvents; + }, async getPermission(login) { calls.permission.push(login); return permissions[login] ?? "none"; @@ -783,6 +788,89 @@ for (const [label, options] of [ }); } +// Where needs-human came from (gate-source.mjs). A lane escalation: +// report-escalation.sh's label, then its marked comment, by the lanes App. A +// review-lane hold: the review lane's label and hold comment. +const APP_USER = { login: "melodic-automation-lanes[bot]", id: APP_ID, type: "Bot" }; +const ACTIONS_USER = { ...ACTIONS_BOT, type: "Bot" }; +const RESUME = ``; +const escalationComment = ({ body = `\n${RESUME}\nThe pr-unblock lane stopped.\n\n- Reason: \`ci-fix-round-cap\``, user = APP_USER, at = "2026-10-18T10:00:05Z" } = {}) => ({ + user, + body, + created_at: at, + updated_at: at, +}); +const holdComment = ({ sha = HEAD, at = "2026-10-18T09:00:00Z" } = {}) => ({ + user: ACTIONS_USER, + body: `\nChanges a trust gate.\n\n\`claude-lane: claude-security-review\``, + created_at: at, + updated_at: at, +}); +const labelEvent = (event, actor, at) => ({ event, actor, label: { name: "needs-human" }, created_at: at }); +const appLabeled = labelEvent("labeled", APP_USER, "2026-10-18T10:00:00Z"); +const reviewed = [marker("claude-review"), marker("claude-security-review")]; + +test("a needs-human PR a lane escalation gated dispatches for a reviewer-bot thread", async () => { + const api = fakeApi({ pulls: humanGated(), comments: [...reviewed, escalationComment()], labelEvents: [appLabeled] }); + const { result, lines } = await decide(api); + assert.equal(result?.number, 42, lines.join("\n")); + assert.equal(api.calls.dispatch.length, 1); +}); + +for (const [label, options] of [ + ["a review-lane hold on the current head", { + comments: [...reviewed, holdComment()], + labelEvents: [labelEvent("labeled", ACTIONS_USER, "2026-10-18T09:00:00Z")], + }], + ["a lane escalation beside a review-lane hold on the current head", { + comments: [...reviewed, holdComment({ at: "2026-10-18T10:30:00Z" }), escalationComment()], + labelEvents: [appLabeled], + }], + ["a lane escalation beside an open review-lane hold on an older head", { + comments: [...reviewed, holdComment({ sha: OLD_HEAD }), escalationComment()], + labelEvents: [appLabeled], + }], + ["an escalation marker with no resume-key line", { + comments: [...reviewed, escalationComment({ body: "\nThe pr-unblock lane stopped." })], + labelEvents: [appLabeled], + }], + ["a lane escalation beside an escalation marker out of place", { + comments: [...reviewed, escalationComment(), escalationComment({ body: `Note.\n\n${RESUME}`, at: "2026-10-18T10:05:00Z" })], + labelEvents: [appLabeled], + }], + ["a lane escalation beside a hold marker whose head does not parse", { + comments: [...reviewed, holdComment({ sha: "bbbb" }), escalationComment()], + labelEvents: [appLabeled], + }], + ["an escalation marker someone other than the lanes App wrote", { + comments: [...reviewed, escalationComment({ user: { login: "kyle-sexton", id: KYLE.databaseId, type: "User" } })], + labelEvents: [appLabeled], + }], + ["a lane escalation under a label a person added", { + comments: [...reviewed, escalationComment()], + labelEvents: [labelEvent("labeled", { login: "kyle-sexton", id: KYLE.databaseId, type: "User" }, "2026-10-18T10:00:00Z")], + }], + ["a lane escalation older than the last label add", { + comments: [...reviewed, escalationComment({ at: "2026-10-18T09:00:00Z" })], + labelEvents: [appLabeled], + }], +]) { + test(`a needs-human PR dispatches nothing for a reviewer-bot thread under ${label}`, async () => { + const api = fakeApi({ pulls: humanGated(), ...options }); + const { result, lines } = await decide(api); + assert.equal(result, null, lines.join("\n")); + assert.deepEqual(api.calls.dispatch, []); + // The skip names why the label does not admit the PR. + assert.match(lines.at(-1), /is labeled needs-human \((?:review-hold|unknown): /); + }); +} + +test("a PR without needs-human reads no label events", async () => { + const api = fakeApi(); + assert.equal((await decide(api)).result.number, 42); + assert.equal(api.calls.labelEvents, 0); +}); + test("without needs-human a trusted reply dispatches with no permission read", async () => { const api = fakeApi({ threads: [decidedThread({ author: KYLE_LOGIN })] }); assert.equal((await decide(api)).result.number, 42); diff --git a/.github/scripts/pr-unblock/gate-source.mjs b/.github/scripts/pr-unblock/gate-source.mjs new file mode 100644 index 0000000000..4101580b79 --- /dev/null +++ b/.github/scripts/pr-unblock/gate-source.mjs @@ -0,0 +1,174 @@ +// gate-source.mjs: where a pull request's human-gated label came from, read +// from its timeline. clear-escalations.mjs (pr-merge) uses the pieces to decide +// whether the label may come off; feedback-dispatch.mjs and address-feedback.mjs +// (pr-unblock) use gateSource to decide whether address-feedback works the PR +// while the label stays on. +// +// Two writers put the label on: +// - a lane escalation: the lanes App's report-escalation.sh (pr-unblock, +// fix-ci) adds the label, then posts a comment whose first line is +// ESCALATION and whose second is the resume-key line; +// - a review-lane hold: a review lane's status job adds the label and posts a +// `` comment, +// as claude[bot], or as github-actions[bot] ending in the lane's footer. +// +// Timeline items take the REST timeline shape: `event` ("commented", +// "labeled", "unlabeled"), `user` or `actor` ({ login, id, type }), `body`, +// `label`, `created_at`, `updated_at`. Text written by anyone but those +// authors is never read. + +export const ESCALATION = ""; +export const RESUME_KEY = /^$/; +// Any escalation marker, of any lane: one that is not a whole ESCALATION +// comment is unparsable here. +const ANY_ESCALATION = "$/; +// A review lane's hold comment opens a line with this marker. +export const REVIEW_HOLD = "$/; +const LANE_FOOTER = /^`?claude-lane: (?:claude-review|claude-security-review)`?$/; +// report-escalation.sh writes the label, then the comment, retrying the +// comment for up to 15 seconds; a minute covers that (judgment). +export const PAIRED_MS = 60_000; + +export const lines = (body) => String(body ?? "").split("\n").map((line) => line.replace(/\r$/, "")); +export const same = (a, b) => String(a ?? "").toLowerCase() === String(b).toLowerCase(); +export const isApp = (user, bot) => user?.login === bot && user?.type === "Bot"; + +// The REVIEW_HOLD marker line of a review lane's hold comment, or undefined. +export function laneHold(item) { + const body = lines(item.body).map((line) => line.trim()); + const marker = body.find((line) => line.startsWith(REVIEW_HOLD)); + const lane = + isApp(item.user, "claude[bot]") || + (isApp(item.user, "github-actions[bot]") && LANE_FOOTER.test(body.findLast((line) => line !== "") ?? "")); + return item.event === "commented" && lane ? marker : undefined; +} + +// Issue comments (REST) as timeline items. +export const commentItems = (comments) => comments.map((comment) => ({ event: "commented", ...comment })); + +// The newest `labeled` event for `label`, when it was, and when the label was +// last removed at or before it (-Infinity when never). +export function labelHistory(timeline, label) { + const labeled = timeline.filter((item) => item.event === "labeled" && same(item.label?.name, label)).at(-1); + const since = Date.parse(labeled?.created_at ?? ""); + const lifted = Math.max( + -Infinity, + ...timeline + .filter((item) => item.event === "unlabeled" && same(item.label?.name, label)) + .map((item) => Date.parse(item.created_at)) + .filter((time) => time <= since), + ); + return { labeled, since, lifted }; +} + +// A review lane's hold comment written or edited since the label was last +// removed (ever, when it never was): a person removing the label lifts it. +export function openReviewHold(timeline, lifted) { + return timeline.find( + (item) => laneHold(item) !== undefined && !(Math.max(Date.parse(item.created_at), Date.parse(item.updated_at ?? item.created_at)) < lifted), + ); +} + +// The lanes App's escalation comments written at or after `since`. +export function escalationsSince(appComments, since) { + return appComments + .map((item) => ({ body: lines(item.body), at: Date.parse(item.created_at) })) + .filter(({ body, at }) => body[0] === ESCALATION && RESUME_KEY.test(body[1] ?? "") && at >= since); +} + +// The first marker line on a lanes App comment written or edited at or after +// `since` that `known` does not accept and is not clear-escalations' own. +export function foreignMarker(appComments, since, known) { + return appComments + .filter((item) => !(Date.parse(item.created_at) < since) || !(Date.parse(item.updated_at ?? item.created_at) < since)) + .flatMap((item) => lines(item.body).filter((line) => line.includes("\n\nStopped.`, at); +const hold = (sha, at) => + commented(ACTIONS, `\nHeld.\n\nclaude-lane: claude-review`, at); +const source = (timeline) => gateSource({ timeline, label: "needs-human", head: HEAD, isLanesApp }).source; + +test("a hold on an older head a person lifted by unlabeling leaves a later lane escalation workable", () => { + const timeline = [ + hold(OLD, "2026-10-18T08:00:00Z"), + label("unlabeled", OWNER, "2026-10-18T09:00:00Z"), + label("labeled", APP, "2026-10-18T10:00:00Z"), + escalation(), + ]; + assert.equal(source(timeline), "lane-escalation"); +}); + +test("a hold on the current head holds even after a person unlabeled it", () => { + const timeline = [ + hold(HEAD, "2026-10-18T08:00:00Z"), + label("unlabeled", OWNER, "2026-10-18T09:00:00Z"), + label("labeled", APP, "2026-10-18T10:00:00Z"), + escalation(), + ]; + assert.equal(source(timeline), "review-hold"); +}); + +test("a hold marker text a person wrote is not read", () => { + const timeline = [commented(OWNER, ``, "2026-10-18T10:01:00Z"), label("labeled", APP, "2026-10-18T10:00:00Z"), escalation()]; + assert.equal(source(timeline), "lane-escalation"); +}); + +test("another lane's marker on a lanes App comment since the label is unknown", () => { + const timeline = [label("labeled", APP, "2026-10-18T10:00:00Z"), escalation(), commented(APP, "\nRisk.", "2026-10-18T10:02:00Z")]; + assert.equal(source(timeline), "unknown"); +}); + +test("an escalation more than a minute after the label is unknown", () => { + assert.equal(source([label("labeled", APP, "2026-10-18T10:00:00Z"), escalation("2026-10-18T10:05:00Z")]), "unknown"); +}); + +test("listLabelEvents maps GraphQL label events to timeline items across pages", async () => { + const pages = [ + { + nodes: [{ __typename: "LabeledEvent", createdAt: "2026-10-18T10:00:00Z", label: { name: "needs-human" }, actor: { __typename: "Bot", login: APP.login, databaseId: APP.id } }], + pageInfo: { hasNextPage: true, endCursor: "c1" }, + }, + { + nodes: [{ __typename: "UnlabeledEvent", createdAt: "2026-10-18T11:00:00Z", label: { name: "needs-human" }, actor: { __typename: "User", login: OWNER.login, databaseId: OWNER.id } }], + pageInfo: { hasNextPage: false, endCursor: null }, + }, + ]; + const asked = []; + const graphql = async (query, variables) => { + asked.push(variables); + return { repository: { pullRequest: { timelineItems: pages[asked.length - 1] } } }; + }; + const events = await listLabelEvents({ graphql, repository: "o/r", number: 7 }); + assert.deepEqual(events, [ + label("labeled", APP, "2026-10-18T10:00:00Z"), + label("unlabeled", OWNER, "2026-10-18T11:00:00Z"), + ]); + assert.deepEqual(asked, [ + { owner: "o", name: "r", number: 7, after: null }, + { owner: "o", name: "r", number: 7, after: "c1" }, + ]); +}); diff --git a/.github/workflows/pr-run-activity-write.yml b/.github/workflows/pr-run-activity-write.yml index 1a1457095f..2b86999198 100644 --- a/.github/workflows/pr-run-activity-write.yml +++ b/.github/workflows/pr-run-activity-write.yml @@ -306,7 +306,8 @@ on: apply-patch with patch-prepare review-threads: the select job's fix-requested output. `true` leaves the human-gated label out of the holds, since that job kept only threads a trusted person with - write access asked the lane to fix. + write access asked the lane to fix, or a lane escalation put the + label on. required: false type: string default: "" @@ -1085,7 +1086,8 @@ jobs: # human-gated label is left out only for apply-patch under the # review-threads row, when the caller's select job kept nothing but # threads a trusted person with write access asked the lane to fix on a - # PR carrying it, unless labels-absent lists it. + # PR carrying it, or a lane escalation put it on, unless labels-absent + # lists it. - name: Read the hold labels from the base id: hold-label if: >- diff --git a/.github/workflows/pr-unblock.yml b/.github/workflows/pr-unblock.yml index a7f97468d1..4d76903091 100644 --- a/.github/workflows/pr-unblock.yml +++ b/.github/workflows/pr-unblock.yml @@ -413,7 +413,8 @@ jobs: patch-threads-attempt: ${{ needs.address-feedback-select.outputs.context-attempt }} patch-threads-sha256: ${{ needs.address-feedback-select.outputs.threads-sha256 }} # true drops the human-gated label from the holds: the select job kept - # only threads a trusted person with write access asked to fix. + # only threads a trusted person with write access asked to fix, or a + # lane escalation put the label on (gate-source.mjs). patch-fix-requested: ${{ needs.address-feedback-select.outputs.fix-requested }} reply-attempt: ${{ needs.address-feedback-subscription.result != 'skipped' && needs.address-feedback-subscription.outputs.scan-reply-attempt || needs.address-feedback-subscription.result == 'skipped' && needs.address-feedback.outputs.scan-reply-attempt || '' }} diff --git a/docs/conventions/pr-pipeline/README.md b/docs/conventions/pr-pipeline/README.md index 019f057889..54cb171582 100644 --- a/docs/conventions/pr-pipeline/README.md +++ b/docs/conventions/pr-pipeline/README.md @@ -155,7 +155,12 @@ PR, it carries no `do-not-merge`, both lanes' newest reviewed-head markers name the head, an unresolved thread exists that `address-feedback` would keep (left only by the reviewer bot, one the lane never answered where a fix request, below, follows the reviewer bot's finding, or answered by a trusted human after the lane escalated it; on a PR carrying the -human-gated label, only one whose answer is a fix request), the head has no +human-gated label, only one whose answer is a fix request, unless a lane escalation put the label +on: the lanes App added it last, its `work-items:escalation lane=pr-unblock` comment (pr-unblock +or fix-ci) followed within a minute, no other marker is on its comments since, and no review-lane +`claude-lane-hold` names the current head or is open, per +`.github/scripts/pr-unblock/gate-source.mjs`; anything ambiguous keeps the fix-request rule, and +the label stays on for `clear-escalations.mjs` to remove), the head has no `pr-unblock / address-feedback` check run (one that started before the head's newest `pr-unblock / publish-feedback` check run stops counting when that run skipped `no-model-turn`: the round handled nothing, and the round cap still bounds the retries; a limit stop keeps its From 3aba7860bccb95e1162fb838afe25dc88d8ec4be Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Fri, 23 Oct 2026 10:09:55 -0400 Subject: [PATCH 2/4] chore(pr-pipeline): add changelog fragment for lane-escalated address-feedback Co-Authored-By: Claude Opus 5.5 --- .../feat-address-feedback-lane-escalation-ffa0d57f.md | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 .changes/pr-pipeline/feat-address-feedback-lane-escalation-ffa0d57f.md diff --git a/.changes/pr-pipeline/feat-address-feedback-lane-escalation-ffa0d57f.md b/.changes/pr-pipeline/feat-address-feedback-lane-escalation-ffa0d57f.md new file mode 100644 index 0000000000..54ca185d5b --- /dev/null +++ b/.changes/pr-pipeline/feat-address-feedback-lane-escalation-ffa0d57f.md @@ -0,0 +1,7 @@ +--- +bump: minor +--- + +### Changed + +- `address-feedback` works a pull request carrying the human-gated label when a lane escalation (pr-unblock or fix-ci) put the label on and no review-lane hold names the current head or is open; a review-lane hold, or anything ambiguous, still limits it to threads a trusted person with write access asked it to fix. The label stays on for `clear-escalations` to remove. From f47b14f1101a621ce95b5445110248d045d5ab00 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Fri, 23 Oct 2026 10:17:18 -0400 Subject: [PATCH 3/4] fix(pr-pipeline): keep lane-escalated PRs workable after an address-feedback round gateSource treated address-feedback's own round record as a foreign marker, so the first round on an escalated PR gated every later one. It now accepts the round and model-run lines. The dispatcher's skip tests pin the gate source per case, and the docs say clear-escalations does not clear a label once a round is recorded. Co-Authored-By: Claude Opus 5.5 --- ...dress-feedback-lane-escalation-ffa0d57f.md | 2 +- .../scripts/pr-unblock/address-feedback.mjs | 4 ++-- .../pr-unblock/feedback-dispatch.test.mjs | 24 +++++++++---------- .github/scripts/pr-unblock/gate-source.mjs | 24 ++++++++++++------- .../scripts/pr-unblock/gate-source.test.mjs | 10 ++++++++ docs/conventions/pr-pipeline/README.md | 3 ++- 6 files changed, 43 insertions(+), 24 deletions(-) diff --git a/.changes/pr-pipeline/feat-address-feedback-lane-escalation-ffa0d57f.md b/.changes/pr-pipeline/feat-address-feedback-lane-escalation-ffa0d57f.md index 54ca185d5b..3e6a5819d0 100644 --- a/.changes/pr-pipeline/feat-address-feedback-lane-escalation-ffa0d57f.md +++ b/.changes/pr-pipeline/feat-address-feedback-lane-escalation-ffa0d57f.md @@ -4,4 +4,4 @@ bump: minor ### Changed -- `address-feedback` works a pull request carrying the human-gated label when a lane escalation (pr-unblock or fix-ci) put the label on and no review-lane hold names the current head or is open; a review-lane hold, or anything ambiguous, still limits it to threads a trusted person with write access asked it to fix. The label stays on for `clear-escalations` to remove. +- `address-feedback` works a pull request carrying the human-gated label when a lane escalation (pr-unblock or fix-ci) put the label on and no review-lane hold names the current head or is open; a review-lane hold, or anything ambiguous, still limits it to threads a trusted person with write access asked it to fix. The label stays on: `clear-escalations` removes it only while no address-feedback round has been recorded since it went on, so once a round runs a person removes it. diff --git a/.github/scripts/pr-unblock/address-feedback.mjs b/.github/scripts/pr-unblock/address-feedback.mjs index f167a1cfb8..e952c912cd 100644 --- a/.github/scripts/pr-unblock/address-feedback.mjs +++ b/.github/scripts/pr-unblock/address-feedback.mjs @@ -30,13 +30,13 @@ import { appendFileSync, lstatSync, readdirSync, readFileSync, writeFileSync } f import { join } from "node:path"; import { pathToFileURL } from "node:url"; -import { commentItems, gateSource, listLabelEvents } from "./gate-source.mjs"; +import { commentItems, gateSource, listLabelEvents, ROUND_MARKER } from "./gate-source.mjs"; import { isProtectedPath, namedInScope } from "./thread-scope.mjs"; // The round cap is the config's `loop.review-rounds`, passed in as // REVIEW_ROUNDS; the resolver's default is the fallback. export const DEFAULT_MAX_ROUNDS = 2; -export const ROUND_MARKER = ""; +export { ROUND_MARKER }; // github-actions[bot], the author of the review lanes' reviewed-head markers. export const MARKER_AUTHOR_ID = 41898282; const REVIEWED_HEAD = diff --git a/.github/scripts/pr-unblock/feedback-dispatch.test.mjs b/.github/scripts/pr-unblock/feedback-dispatch.test.mjs index 44eb9ebadc..b0b3242201 100644 --- a/.github/scripts/pr-unblock/feedback-dispatch.test.mjs +++ b/.github/scripts/pr-unblock/feedback-dispatch.test.mjs @@ -817,51 +817,51 @@ test("a needs-human PR a lane escalation gated dispatches for a reviewer-bot thr assert.equal(api.calls.dispatch.length, 1); }); -for (const [label, options] of [ - ["a review-lane hold on the current head", { +for (const [label, source, options] of [ + ["a review-lane hold on the current head", "review-hold", { comments: [...reviewed, holdComment()], labelEvents: [labelEvent("labeled", ACTIONS_USER, "2026-10-18T09:00:00Z")], }], - ["a lane escalation beside a review-lane hold on the current head", { + ["a lane escalation beside a review-lane hold on the current head", "review-hold", { comments: [...reviewed, holdComment({ at: "2026-10-18T10:30:00Z" }), escalationComment()], labelEvents: [appLabeled], }], - ["a lane escalation beside an open review-lane hold on an older head", { + ["a lane escalation beside an open review-lane hold on an older head", "review-hold", { comments: [...reviewed, holdComment({ sha: OLD_HEAD }), escalationComment()], labelEvents: [appLabeled], }], - ["an escalation marker with no resume-key line", { + ["an escalation marker with no resume-key line", "unknown", { comments: [...reviewed, escalationComment({ body: "\nThe pr-unblock lane stopped." })], labelEvents: [appLabeled], }], - ["a lane escalation beside an escalation marker out of place", { + ["a lane escalation beside an escalation marker out of place", "unknown", { comments: [...reviewed, escalationComment(), escalationComment({ body: `Note.\n\n${RESUME}`, at: "2026-10-18T10:05:00Z" })], labelEvents: [appLabeled], }], - ["a lane escalation beside a hold marker whose head does not parse", { + ["a lane escalation beside a hold marker whose head does not parse", "unknown", { comments: [...reviewed, holdComment({ sha: "bbbb" }), escalationComment()], labelEvents: [appLabeled], }], - ["an escalation marker someone other than the lanes App wrote", { + ["an escalation marker someone other than the lanes App wrote", "unknown", { comments: [...reviewed, escalationComment({ user: { login: "kyle-sexton", id: KYLE.databaseId, type: "User" } })], labelEvents: [appLabeled], }], - ["a lane escalation under a label a person added", { + ["a lane escalation under a label a person added", "unknown", { comments: [...reviewed, escalationComment()], labelEvents: [labelEvent("labeled", { login: "kyle-sexton", id: KYLE.databaseId, type: "User" }, "2026-10-18T10:00:00Z")], }], - ["a lane escalation older than the last label add", { + ["a lane escalation older than the last label add", "unknown", { comments: [...reviewed, escalationComment({ at: "2026-10-18T09:00:00Z" })], labelEvents: [appLabeled], }], ]) { - test(`a needs-human PR dispatches nothing for a reviewer-bot thread under ${label}`, async () => { + test(`a needs-human PR dispatches nothing for a reviewer-bot thread under ${label} (${source})`, async () => { const api = fakeApi({ pulls: humanGated(), ...options }); const { result, lines } = await decide(api); assert.equal(result, null, lines.join("\n")); assert.deepEqual(api.calls.dispatch, []); // The skip names why the label does not admit the PR. - assert.match(lines.at(-1), /is labeled needs-human \((?:review-hold|unknown): /); + assert.ok(lines.at(-1).includes(`is labeled needs-human (${source}: `), lines.at(-1)); }); } diff --git a/.github/scripts/pr-unblock/gate-source.mjs b/.github/scripts/pr-unblock/gate-source.mjs index 4101580b79..23a99e181e 100644 --- a/.github/scripts/pr-unblock/gate-source.mjs +++ b/.github/scripts/pr-unblock/gate-source.mjs @@ -22,9 +22,13 @@ export const RESUME_KEY = /^"; +const MODEL_RUN = /^$/; export const CLEARED = /^$/; // A review lane's hold comment opens a line with this marker. -export const REVIEW_HOLD = "$/; const LANE_FOOTER = /^`?claude-lane: (?:claude-review|claude-security-review)`?$/; // report-escalation.sh writes the label, then the comment, retrying the @@ -34,6 +38,9 @@ export const PAIRED_MS = 60_000; export const lines = (body) => String(body ?? "").split("\n").map((line) => line.replace(/\r$/, "")); export const same = (a, b) => String(a ?? "").toLowerCase() === String(b).toLowerCase(); export const isApp = (user, bot) => user?.login === bot && user?.type === "Bot"; +// Written or edited at or after `since`; an unparsable time counts as since. +const writtenSince = (item, since) => + !(Date.parse(item.created_at) < since) || !(Date.parse(item.updated_at ?? item.created_at) < since); // The REVIEW_HOLD marker line of a review lane's hold comment, or undefined. export function laneHold(item) { @@ -82,7 +89,7 @@ export function escalationsSince(appComments, since) { // `since` that `known` does not accept and is not clear-escalations' own. export function foreignMarker(appComments, since, known) { return appComments - .filter((item) => !(Date.parse(item.created_at) < since) || !(Date.parse(item.updated_at ?? item.created_at) < since)) + .filter((item) => writtenSince(item, since)) .flatMap((item) => lines(item.body).filter((line) => line.includes("\n\nRound 1.", "2026-10-18T10:30:00Z"), + commented(APP, "\nRound 2.", "2026-10-18T11:30:00Z"), + ]; + assert.equal(source(timeline), "lane-escalation"); +}); + test("an escalation more than a minute after the label is unknown", () => { assert.equal(source([label("labeled", APP, "2026-10-18T10:00:00Z"), escalation("2026-10-18T10:05:00Z")]), "unknown"); }); diff --git a/docs/conventions/pr-pipeline/README.md b/docs/conventions/pr-pipeline/README.md index 54cb171582..4cf7ce90d6 100644 --- a/docs/conventions/pr-pipeline/README.md +++ b/docs/conventions/pr-pipeline/README.md @@ -160,7 +160,8 @@ on: the lanes App added it last, its `work-items:escalation lane=pr-unblock` com or fix-ci) followed within a minute, no other marker is on its comments since, and no review-lane `claude-lane-hold` names the current head or is open, per `.github/scripts/pr-unblock/gate-source.mjs`; anything ambiguous keeps the fix-request rule, and -the label stays on for `clear-escalations.mjs` to remove), the head has no +the label stays on, and `clear-escalations.mjs` removes it only while no address-feedback round +record is on the lanes App's comments since it went on), the head has no `pr-unblock / address-feedback` check run (one that started before the head's newest `pr-unblock / publish-feedback` check run stops counting when that run skipped `no-model-turn`: the round handled nothing, and the round cap still bounds the retries; a limit stop keeps its From 7f9f16df892af1f95a935c4723e4f0ce715fe59f Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Fri, 23 Oct 2026 10:19:52 -0400 Subject: [PATCH 4/4] docs(pr-pipeline): name do-not-merge as the re-gate for a lane-escalated PR Co-Authored-By: Claude Opus 5.5 --- .../feat-address-feedback-lane-escalation-ffa0d57f.md | 2 +- docs/conventions/pr-pipeline/README.md | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/.changes/pr-pipeline/feat-address-feedback-lane-escalation-ffa0d57f.md b/.changes/pr-pipeline/feat-address-feedback-lane-escalation-ffa0d57f.md index 3e6a5819d0..d2a2a6b04a 100644 --- a/.changes/pr-pipeline/feat-address-feedback-lane-escalation-ffa0d57f.md +++ b/.changes/pr-pipeline/feat-address-feedback-lane-escalation-ffa0d57f.md @@ -4,4 +4,4 @@ bump: minor ### Changed -- `address-feedback` works a pull request carrying the human-gated label when a lane escalation (pr-unblock or fix-ci) put the label on and no review-lane hold names the current head or is open; a review-lane hold, or anything ambiguous, still limits it to threads a trusted person with write access asked it to fix. The label stays on: `clear-escalations` removes it only while no address-feedback round has been recorded since it went on, so once a round runs a person removes it. +- `address-feedback` works a pull request carrying the human-gated label when a lane escalation (pr-unblock or fix-ci) put the label on and no review-lane hold names the current head or is open; a review-lane hold, or anything ambiguous, still limits it to threads a trusted person with write access asked it to fix. The label stays on: `clear-escalations` removes it only while no address-feedback round has been recorded since it went on, so once a round runs a person removes it. To stop the lane working such a PR, add `do-not-merge`. diff --git a/docs/conventions/pr-pipeline/README.md b/docs/conventions/pr-pipeline/README.md index 4cf7ce90d6..f659ed195c 100644 --- a/docs/conventions/pr-pipeline/README.md +++ b/docs/conventions/pr-pipeline/README.md @@ -161,7 +161,8 @@ or fix-ci) followed within a minute, no other marker is on its comments since, a `claude-lane-hold` names the current head or is open, per `.github/scripts/pr-unblock/gate-source.mjs`; anything ambiguous keeps the fix-request rule, and the label stays on, and `clear-escalations.mjs` removes it only while no address-feedback round -record is on the lanes App's comments since it went on), the head has no +record is on the lanes App's comments since it went on; to stop the lane working such a PR, a +person adds `do-not-merge`, since re-adding a label already on records no event), the head has no `pr-unblock / address-feedback` check run (one that started before the head's newest `pr-unblock / publish-feedback` check run stops counting when that run skipped `no-model-turn`: the round handled nothing, and the round cap still bounds the retries; a limit stop keeps its