From 101bd67390b6a92908a7ebf0786a4fc0e9fc34bf Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Wed, 28 Oct 2026 22:52:09 -0400 Subject: [PATCH 1/5] feat(pr-pipeline): answer lane review threads on owner-login PRs from the lanes App address-feedback now also serves an adopted owner PR (a kind: human trusted actor's, under the adoption section, idle for adoption.idle-hours with no Hold: line), so its replies post as the lanes App and the babysit worker tier can resolve the outdated threads a lane answered. On such a PR publish-feedback leaves answered threads open, and the select job leaves a person's thread with no stop. Refs: #10763 Co-Authored-By: Claude Opus 5.5 --- .../feat-owner-pr-feedback-7d3a91c2.md | 8 + .../scripts/pr-unblock/address-feedback.mjs | 9 +- .../pr-unblock/address-feedback.test.mjs | 23 ++- .../scripts/pr-unblock/feedback-dispatch.mjs | 123 ++++++++++++-- .../pr-unblock/feedback-dispatch.test.mjs | 152 +++++++++++++++++- .../scripts/pr-unblock/fix-ci-dispatch.mjs | 2 +- .../scripts/pr-unblock/publish-feedback.mjs | 21 ++- .../pr-unblock/publish-feedback.test.mjs | 35 ++++ docs/conventions/pr-pipeline/README.md | 45 +++++- 9 files changed, 392 insertions(+), 26 deletions(-) create mode 100644 .changes/pr-pipeline/feat-owner-pr-feedback-7d3a91c2.md diff --git a/.changes/pr-pipeline/feat-owner-pr-feedback-7d3a91c2.md b/.changes/pr-pipeline/feat-owner-pr-feedback-7d3a91c2.md new file mode 100644 index 0000000000..486f29b88a --- /dev/null +++ b/.changes/pr-pipeline/feat-owner-pr-feedback-7d3a91c2.md @@ -0,0 +1,8 @@ +--- +bump: minor +--- + +### Added + +- `address-feedback` also serves an adopted owner PR: one a `kind: human` account on the base's trusted-actors list opened, under an `adoption` section, with no `Hold:` body line, once the PR and its head commit are `adoption.idle-hours` old, the rule fix-ci adopts by. Its replies post as the lanes App, so the owner's babysit worker tier can tell a lane answer from the person's and resolve the outdated threads the lane answered ([#10763](https://github.com/melodic-software/claude-code-plugins/issues/10763)). The half-hourly sweep sends one such idle PR per tick whose reviewed head has no round yet, since its review usually finishes while the session is still active. Lanes App PRs, and every other author's PRs, are dispatched as before. +- On an owner PR, `publish-feedback` posts each answer and leaves the thread open, since the lanes App cannot resolve a thread on a PR it did not open, and the select job skips a thread the person wrote or answered with no stop instead of escalating `feedback-untaken-threads`. diff --git a/.github/scripts/pr-unblock/address-feedback.mjs b/.github/scripts/pr-unblock/address-feedback.mjs index 9c453ce909..52c24f5a57 100644 --- a/.github/scripts/pr-unblock/address-feedback.mjs +++ b/.github/scripts/pr-unblock/address-feedback.mjs @@ -588,7 +588,14 @@ export async function select({ return { decision: "skip", skipReason: "awaiting-human", threads: kept }; } const stale = new Set(staleVerifyNotes(threads, headSha)); - return threads.some((thread) => thread.isResolved === false && !stale.has(thread.id)) + const untaken = threads.some((thread) => thread.isResolved === false && !stale.has(thread.id)); + // On a PR a person opened, a thread the lane does not take is theirs (one + // they wrote, answered, or a lane answer the lanes App cannot resolve + // there), and so no stop: it still holds the merge until it is resolved. + if (untaken && pull.user?.id !== appId) { + return { decision: "skip", skipReason: "not-applicable", threads: kept }; + } + return untaken ? { decision: "escalate", escalationReason: "feedback-untaken-threads", threads: kept } : { decision: "skip", skipReason: "not-applicable", 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 87fbd1ce02..67429dd5a1 100644 --- a/.github/scripts/pr-unblock/address-feedback.test.mjs +++ b/.github/scripts/pr-unblock/address-feedback.test.mjs @@ -45,7 +45,8 @@ function fakeApi(overrides = {}) { return { posted, permissionReads, - getPull: async () => ({ number: 42, labels: (overrides.labels ?? []).map((name) => ({ name })) }), + // The lanes App opened the PR unless the case names another opener. + getPull: async () => ({ number: 42, user: { id: overrides.opener ?? APP }, labels: (overrides.labels ?? []).map((name) => ({ name })) }), async getPermission(login) { permissionReads.push(login); return (overrides.permissions ?? { "kyle-sexton": "write" })[login] ?? "none"; @@ -1601,3 +1602,23 @@ test("scanRefusal reads a patch and an output marker together as output, in eith } } }); + +// On a PR a person opened, an unresolved thread the lane does not take is the +// person's: one they wrote or answered, or a lane answer the lanes App cannot +// resolve there. It is no stop, and it does not read as resolved. +test("on an owner PR an untaken thread skips with no stop; on a lanes App PR it still escalates", async () => { + const threads = { reviewComments: [comment(100, REVIEWER), comment(101, HUMAN)], threads: [thread("PRRT_1", [100, 101])] }; + const owner = await run(fakeApi({ ...threads, opener: HUMAN })); + assert.equal(owner.decision, "skip"); + assert.equal(owner.skipReason, "not-applicable"); + assert.notEqual(owner.resolved, true); + assert.deepEqual(owner.threads, []); + const app = await run(fakeApi(threads)); + assert.equal(app.escalationReason, "feedback-untaken-threads"); +}); + +test("on an owner PR a thread a person opened is never kept", async () => { + const result = await run(fakeApi({ opener: HUMAN, reviewComments: [comment(100, HUMAN)], threads: [thread("PRRT_1", [100])] })); + assert.deepEqual(result.threads, []); + assert.notEqual(result.decision, "proceed"); +}); diff --git a/.github/scripts/pr-unblock/feedback-dispatch.mjs b/.github/scripts/pr-unblock/feedback-dispatch.mjs index 1bea6534c0..6135fcdbc0 100644 --- a/.github/scripts/pr-unblock/feedback-dispatch.mjs +++ b/.github/scripts/pr-unblock/feedback-dispatch.mjs @@ -1,4 +1,5 @@ -// Starts pr-unblock's address-feedback activity on a lanes App PR once both +// Starts pr-unblock's address-feedback activity on a lanes App PR, or an +// adopted owner PR once idle (ownerAuthored, ownerSkip), once both // review lanes have reviewed its current head and left an unresolved thread. // Run by .github/workflows/pr-feedback-dispatch.yml on workflow_run, after the // kill switch. From the event file it takes only the upstream run's id, path, @@ -14,7 +15,10 @@ // - the upstream run is a completed pull_request run of a review workflow, // by exact path, in this repository and on a head of this repository; // - exactly one open, non-draft PR into main has that run's head SHA as its -// current head, and the lanes App opened it; +// current head, and the lanes App opened it, or a person on the base's +// trusted-actors list did under an `adoption` section, with no `Hold:` body +// line and the PR and its head commit idle-hours old; on such an owner PR +// only a thread the select job keeps dispatches; // - it carries no do-not-merge label; // - both review lanes' newest reviewed-head markers, written by // github-actions[bot], name that head; @@ -58,9 +62,9 @@ // the human gate (a needs-human PR dispatches here whoever applied the label) // nor the head's check runs decide it; the hold labels still skip it. It only // resolves notes a later pr-verify pass superseded, and the needs-human label -// it leaves on still blocks the merge. Every round goes only to lanes App PRs: -// on a PR a person opened, verify-notes.mjs holds a fail with a label it -// removes itself on the next pass, so there is no thread to resolve. +// it leaves on still blocks the merge. A pass-only round goes only to lanes +// App PRs: on a PR a person opened, verify-notes.mjs holds a fail with a label +// it removes itself on the next pass, so there is no thread to resolve. // // A round whose reply job skipped no-model-turn, or finding-stands (an // adjudicator let a disputed finding stand and the head did not move, so no @@ -71,7 +75,10 @@ // main whose head's newest reply check took one of those skips, fewer than // SWEEP_HEAD_CAP times at that head, is decided as above with the // newest completed review run at that head as the upstream (reviewRunAt), -// which the record names. A sweep dispatches at most once, since the +// which the record names. With no such head, and no stale stop, it sends the +// first idle adopted owner PR whose reviewed head has no round yet +// (sweepIdleOwner), since that PR's own review completion found it active. +// A sweep dispatches at most once, since the // dispatched-run record names one upstream and one run, and keeps no record. // // A run of this file an outage lost dispatched nothing the lanes trust when @@ -117,6 +124,7 @@ import { LOOKBACK_HOURS, LOST } from "../maintenance-reconcile-lost-runs/infra-f import { fixRequesters, humanGatedLabelFrom, onReviewedHead, reviewedHeads, reviewerIdsFrom, 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 { HOLD_LINE } from "./fix-ci-dispatch.mjs"; import { anyHandles, LANE_STOP, laneStop, limitResetsAt, permanentlyHandles } from "./limit-stop.mjs"; import { budgetedFetch } from "./request-budget.mjs"; @@ -152,6 +160,32 @@ export const DEFAULTS = Object.freeze({ const SHA = /^[0-9a-f]{40}$/; const RUN_ID = /^[1-9][0-9]{0,14}$/; +// No owner is adopted unless the base config's `adoption` section sets +// idle-hours (pr-pipeline.yaml). +export const NO_ADOPTION = Object.freeze({ ownerIds: Object.freeze(new Set()), idleHours: undefined }); + +// An owner PR: one a person on the base's trusted-actors list opened (by id, +// kind human), under an `adoption` section. Its rounds post as the lanes App +// like a lanes App PR's, so a lane answer is told apart from the person's. +export const ownerAuthored = (pull, adoption = NO_ADOPTION) => + adoption.idleHours !== undefined && pull.user?.type === "User" && adoption.ownerIds.has(pull.user?.id); + +// Why an owner PR is not adopted for a round now, or "": as fix-ci adopts one +// (fix-ci-dispatch.mjs skipReason), no body line starting `Hold:`, and both +// the PR's last update and its head commit at least idle-hours old, so the +// lane does not push onto a branch a session is still working on. An +// unreadable date is not idle. +export async function ownerSkip({ api, pull, adoption, now }) { + if (HOLD_LINE.test(String(pull.body ?? ""))) { + return "held by a Hold: line"; + } + const since = now - adoption.idleHours * 3_600_000; + if (!(Date.parse(pull.updated_at ?? "") <= since) || !(Date.parse(await api.headCommittedAt(pull.head.sha)) <= since)) { + return `owner PR or its head commit active in the last ${adoption.idleHours} hour(s)`; + } + return ""; +} + // The tick whose completion starts the sweep. export const SWEEP_WORKFLOW = ".github/workflows/pr-unblock-dispatch.yml"; // Releasing rounds at one head after which the sweep stops sending it @@ -266,6 +300,7 @@ export async function dispatchFeedback({ record = emptyRecord(), notesSince = null, passOnly = false, + adoption = NO_ADOPTION, }) { const skip = (why) => { log(`skip: ${why}`); @@ -313,8 +348,17 @@ export async function dispatchFeedback({ if (pull.draft !== false) { return skip(`#${number} is a draft`); } - if (pull.user?.id !== appSenderId) { - return skip(`#${number} was not opened by the lanes App`); + // A lanes App PR, or an adopted owner PR once idle. A pass-only round + // resolves threads as the lanes App, which only its own PRs allow. + const owner = pull.user?.id !== appSenderId; + if (owner && (passOnly || !ownerAuthored(pull, adoption))) { + return skip(`#${number} was not opened by the lanes App${passOnly ? "" : " or an adopted owner"}`); + } + if (owner) { + const why = await ownerSkip({ api, pull, adoption, now }); + if (why) { + return skip(`#${number}: ${why}`); + } } const sent = record.dispatched.get(head); if (sent?.number === number) { @@ -374,6 +418,11 @@ export async function dispatchFeedback({ if (!workable && !threads.some((thread) => thread.isResolved === false)) { return skip(`#${number} has no unresolved review thread`); } + // On an owner PR an unresolved thread the lane does not take (one the + // person wrote or answered) stays the person's, so it starts no round. + if (owner && !workable) { + return skip(`#${number} has no thread the lane works`); + } } // Both review lanes finish on one head, and their dispatchers queue in one // concurrency group. The first one's run posts no check until it ends, so @@ -470,16 +519,41 @@ export async function sweepReleased({ api, repository, appSenderId, log, setting } log("sweep: no head whose last round left it for another round to dispatch"); const stale = await sweepPulls(context, (pull) => sweepStale({ ...context, pull }), pulls); - if (stale === null) { - log("sweep: no head with a failing address-feedback stop and nothing left to fix"); + if (stale !== null) { + return stale; + } + log("sweep: no head with a failing address-feedback stop and nothing left to fix"); + const idle = await sweepPulls(context, (pull) => sweepIdleOwner({ ...context, pull }), pulls); + if (idle === null) { + log("sweep: no idle owner PR whose reviewed head the lane has not answered"); + } + return idle; +} + +// An owner PR's review completes while the person's session is still on it, +// so its review-run dispatch skips the idle test; once idle, the tick sends +// it. dispatchFeedback decides the rest (both markers on the head, a thread +// the lane works, no address-feedback check run there yet). The listing's +// last update is checked first, so a PR still active spends no reads. +async function sweepIdleOwner(context) { + const { pull, appSenderId, rest } = context; + const since = (rest.now ?? Date.now()) - (rest.adoption?.idleHours ?? 0) * 3_600_000; + if (pull.user?.id === appSenderId || !(Date.parse(pull.updated_at ?? "") <= since)) { + return null; } - return stale; + const { capped } = await releaseState(context); + return capped ? null : dispatchAtHead(context); } -// The selection every sweep uses: each open lanes App PR into the base. -async function openAppPulls({ api, repository, appSenderId, settings }) { +// The selection every sweep uses: each open lanes App PR into the base, and +// each adopted owner PR (dispatchFeedback applies its idle test). +async function openAppPulls({ api, repository, appSenderId, settings, rest }) { return (await api.listOpenPulls(settings.base)).filter( - (pull) => pull.user?.id === appSenderId && pull.draft === false && pull.head?.repo?.full_name === repository && SHA.test(pull.head?.sha ?? ""), + (pull) => + (pull.user?.id === appSenderId || ownerAuthored(pull, rest?.adoption)) && + pull.draft === false && + pull.head?.repo?.full_name === repository && + SHA.test(pull.head?.sha ?? ""), ); } @@ -770,6 +844,8 @@ export function createGitHubApi({ token, repository, apiUrl, graphqlUrl, fetch: return (await request(`${repo}/actions/workflows/${workflow}/runs?event=workflow_dispatch&per_page=100`)).workflow_runs ?? []; }, getWorkflowRun: (id) => request(`${repo}/actions/runs/${encodeURIComponent(id)}`), + // A commit's committer date, the time it was made. + headCommittedAt: async (sha) => (await request(`${repo}/commits/${sha}`)).commit?.committer?.date ?? "", async listOpenPulls(base) { const pulls = []; for (let page = 1; page <= 10; page += 1) { @@ -857,6 +933,23 @@ export async function resolveFeedbackSlot({ number, head, env, github, settings }); } +// The owners whose PRs a round serves: the base trusted-actors list's people, +// under the base config's `adoption` section. An unreadable section adopts no +// owner, and lanes App PRs proceed. +export async function ownerAdoption(env, log = () => {}) { + try { + const { adoptionFromBase } = await import(new URL("../../actions/resolve-config/resolve-config.mjs", import.meta.url).href); + const idleHours = adoptionFromBase({ BASE_PATH: env.BASE_PATH || ".base" })["idle-hours"]; + if (idleHours === undefined) { + return NO_ADOPTION; + } + return { ownerIds: trustedHumansFrom(env.TRUSTED_ACTORS || `${env.BASE_PATH || ".base"}/.github/standards/trusted-actors/trusted-actors.json`), idleHours }; + } catch (error) { + log(`::warning::cannot read the base adoption section or trusted-actors list (${error.message}); no owner PR is served this run`); + return NO_ADOPTION; + } +} + // Writes run-id, the dispatched run, as a step output; the workflow records // it with the upstream run id as an artifact on this run. export async function main(env, { api, github, resolveSlot, log = (line) => console.log(line), now = Date.now() } = {}) { @@ -888,6 +981,8 @@ export async function main(env, { api, github, resolveSlot, log = (line) => cons trustedHumans: trustedHumansFrom(env.TRUSTED_ACTORS || `${env.BASE_PATH || ".base"}/.github/standards/trusted-actors/trusted-actors.json`), humanGatedLabel: humanGatedLabelFrom(env.TRACKER_CONFIG || `${env.BASE_PATH || ".base"}/.work-item-tracker.json`), resolveSlot: resolveSlot ?? (({ number, head }) => resolveFeedbackSlot({ number, head, env, github })), + adoption: await ownerAdoption(env, log), + now, log, }; const summary = () => { diff --git a/.github/scripts/pr-unblock/feedback-dispatch.test.mjs b/.github/scripts/pr-unblock/feedback-dispatch.test.mjs index ebddfc8b6c..046cc18ba5 100644 --- a/.github/scripts/pr-unblock/feedback-dispatch.test.mjs +++ b/.github/scripts/pr-unblock/feedback-dispatch.test.mjs @@ -5,7 +5,7 @@ import { test } from "node:test"; import { LIMIT_DEFAULTS } from "../work-resolve-issue/limits.mjs"; import { emptyRecord, parseRecord, serializeRecord } from "./dispatch-record.mjs"; -import { createGitHubApi, dispatchFeedback, main, SWEEP_HEAD_CAP, sweepReleased } from "./feedback-dispatch.mjs"; +import { createGitHubApi, dispatchFeedback, main, NO_ADOPTION, ownerAdoption, SWEEP_HEAD_CAP, sweepReleased } from "./feedback-dispatch.mjs"; import { tempDir } from "../../../lib/test-tmp.mjs"; // The title GitHub gives a pr-unblock.yml run, read from the workflow file: @@ -1864,3 +1864,153 @@ test("a pr-merge run on a head still gated, of another event, or with no review assert.match((await mergeMain(dir, unreviewed)).lines.at(-1), /no completed review run yet/); assert.deepEqual([...scheduled.calls.dispatch, ...unreviewed.calls.dispatch], []); }); + +// Owner PRs: one a person on the trusted-actors list opened, adopted under the +// base config's `adoption` section once its last update and head commit are +// idle-hours old (fix-ci's adoption rule). Its round posts as the lanes App. +const NOW_MS = Date.parse("2026-10-28T12:00:00Z"); +const ADOPTION = { ownerIds: new Set([KYLE.databaseId]), idleHours: 2 }; +const ownerPull = (overrides = {}) => + pull({ user: { id: KYLE.databaseId, type: "User" }, updated_at: "2026-10-28T09:00:00Z", body: "Closes #1", ...overrides }); +const ownerApi = (options = {}, committedAt = "2026-10-28T08:00:00Z") => { + const api = fakeApi({ pulls: ownerPull(), ...options }); + api.calls.commits = []; + api.headCommittedAt = async (sha) => { + api.calls.commits.push(sha); + return committedAt; + }; + return api; +}; +async function decideOwner(api, { adoption = ADOPTION, passOnly = false } = {}) { + const lines = []; + const result = await dispatchFeedback({ + api, + repository: REPOSITORY, + runId: RUN_ID, + upstream: api.upstream, + appSenderId: APP_ID, + trustedHumans: new Set([KYLE.databaseId]), + resolveSlot: async () => resolvedLane(), + adoption, + passOnly, + now: NOW_MS, + log: (line) => lines.push(line), + }); + return { result, lines }; +} + +test("an idle owner PR's lane-review thread dispatches address-feedback, so the lanes App answers it", async () => { + for (const threads of [[thread([CLAUDE])], [LANE_FINDING]]) { + const api = ownerApi({ threads }); + const { result } = await decideOwner(api); + assert.deepEqual(result, { number: 42, workflowRunId: "38779200001" }); + assert.deepEqual(api.calls.dispatch[0].inputs, { "pr-number": "42", activity: "address-feedback", "dispatcher-run-id": RUN_ID }); + assert.deepEqual(api.calls.commits, [HEAD]); + } +}); + +test("an owner PR or its head commit active within idle-hours is not dispatched", async () => { + for (const [label, api] of [ + ["a recent update", ownerApi({ pulls: ownerPull({ updated_at: "2026-10-28T11:00:00Z" }) })], + ["a recent head commit", ownerApi({}, "2026-10-28T11:30:00Z")], + ["an unreadable update", ownerApi({ pulls: ownerPull({ updated_at: undefined }) })], + ["an unreadable commit date", ownerApi({}, "")], + ]) { + const { result, lines } = await decideOwner(api); + assert.equal(result, null, label); + assert.match(lines.at(-1), /owner PR or its head commit active in the last 2 hour\(s\)/, label); + assert.deepEqual(api.calls.dispatch, [], label); + } +}); + +test("an owner PR with a Hold: body line is not dispatched", async () => { + const api = ownerApi({ pulls: ownerPull({ body: "Summary\n hold: I am still on this" }) }); + const { result, lines } = await decideOwner(api); + assert.equal(result, null); + assert.match(lines.at(-1), /held by a Hold: line/); +}); + +test("a person's PR is not served without an adoption section, or when the person is not on the trusted list", async () => { + for (const [label, api, adoption] of [ + ["no adoption section", ownerApi(), NO_ADOPTION], + ["an untrusted person", ownerApi({ pulls: ownerPull({ user: { id: STRANGER.databaseId, type: "User" } }) }), ADOPTION], + ["a trusted id typed as a Bot", ownerApi({ pulls: ownerPull({ user: { id: KYLE.databaseId, type: "Bot" } }) }), ADOPTION], + ]) { + const { result, lines } = await decideOwner(api, { adoption }); + assert.equal(result, null, label); + assert.match(lines.at(-1), /was not opened by the lanes App or an adopted owner/, label); + assert.deepEqual(api.calls.commits, [], label); + } +}); + +test("on an owner PR a thread a person wrote or answered stays the person's and starts no round", async () => { + for (const [label, threads] of [ + ["a thread the owner opened", [thread([KYLE])]], + ["a reviewer thread the owner answered", [{ isResolved: false, comments: { nodes: [{ author: CLAUDE, body: "finding", createdAt: "2026-10-28T08:00:00Z", ...onReview(CLAUDE) }, { author: KYLE, body: "Fixed in abc.", createdAt: "2026-10-28T08:01:00Z" }], pageInfo: { hasNextPage: false } } }]], + ]) { + const api = ownerApi({ threads }); + const { result, lines } = await decideOwner(api); + assert.equal(result, null, label); + assert.match(lines.at(-1), /has no thread the lane works/, label); + assert.deepEqual(api.calls.dispatch, [], label); + } +}); + +test("a lanes App PR is decided as before under an adoption section: no idle read", async () => { + const api = ownerApi({ pulls: pull({ updated_at: "2026-10-28T11:59:00Z" }) }, "2026-10-28T11:59:00Z"); + const { result } = await decideOwner(api); + assert.equal(result?.number, 42); + assert.deepEqual(api.calls.commits, []); +}); + +test("a pass-only round never goes to an owner PR", async () => { + const api = ownerApi(); + const { result, lines } = await decideOwner(api, { passOnly: true }); + assert.equal(result, null); + assert.match(lines.at(-1), /was not opened by the lanes App$/); +}); + +test("the sweep dispatches an idle owner PR whose reviewed head the lane has not answered", async () => { + const sweepApi = (pulls) => { + const api = ownerApi({ pulls }); + Object.assign(api, { + listOpenPulls: async () => pulls, + listStatuses: async () => [], + listCompletedRuns: async (path) => (path.endsWith("pr-review-hosted.yml") ? [upstreamRun({ conclusion: "success", created_at: "2026-10-28T08:30:00Z" })] : []), + }); + return api; + }; + const sweep = (api) => + sweepReleased({ + api, + repository: REPOSITORY, + runId: RUN_ID, + appSenderId: APP_ID, + trustedHumans: new Set([KYLE.databaseId]), + resolveSlot: async () => resolvedLane(), + adoption: ADOPTION, + now: NOW_MS, + log: () => {}, + }); + const idle = sweepApi(ownerPull()); + assert.equal((await sweep(idle))?.number, 42); + assert.equal(idle.calls.dispatch.length, 1); + // Still active: the listing's update time skips it before any read. + const active = sweepApi(ownerPull({ updated_at: "2026-10-28T11:00:00Z" })); + assert.equal(await sweep(active), null); + assert.deepEqual(active.calls.commits, []); + // An untrusted person's PR is never listed. + const stranger = sweepApi(ownerPull({ user: { id: STRANGER.databaseId, type: "User" } })); + assert.equal(await sweep(stranger), null); + assert.deepEqual(stranger.calls.dispatch, []); +}); + +test("ownerAdoption reads the base adoption section and the trusted list's people; an unreadable base adopts no owner", async () => { + const root = new URL("../../../", import.meta.url).pathname; + const adoption = await ownerAdoption({ BASE_PATH: root }); + assert.ok(Number.isInteger(adoption.idleHours)); + assert.ok(adoption.ownerIds.has(KYLE.databaseId)); + const lines = []; + assert.equal(await ownerAdoption({ BASE_PATH: "/nonexistent-base" }, (line) => lines.push(line)), NO_ADOPTION); + assert.match(lines[0], /no owner PR is served this run/); +}); diff --git a/.github/scripts/pr-unblock/fix-ci-dispatch.mjs b/.github/scripts/pr-unblock/fix-ci-dispatch.mjs index 1df8ef2ddd..979dcf2205 100644 --- a/.github/scripts/pr-unblock/fix-ci-dispatch.mjs +++ b/.github/scripts/pr-unblock/fix-ci-dispatch.mjs @@ -94,7 +94,7 @@ const MAX_PAGES = 20; // A body line starting "Hold:", as babysit's BODY_MERGE_HOLD_RE reads it // (babysit_merge.py): any case, after any non-newline whitespace, NBSP // included, with a bare CR as a line break too. -const HOLD_LINE = /(?:^|(?<=[\r\n]))[^\S\r\n]*hold:/i; +export const HOLD_LINE = /(?:^|(?<=[\r\n]))[^\S\r\n]*hold:/i; // The tracker's human-gated label from .work-item-tracker.json's text, as // pr-merge.yml resolves it: config.role_labels["human-gated"] trimmed, or diff --git a/.github/scripts/pr-unblock/publish-feedback.mjs b/.github/scripts/pr-unblock/publish-feedback.mjs index 2823086a56..d5d6982d23 100755 --- a/.github/scripts/pr-unblock/publish-feedback.mjs +++ b/.github/scripts/pr-unblock/publish-feedback.mjs @@ -26,7 +26,9 @@ // feedback-not-landed. // 3. Re-reads each thread and skips one that is resolved, moved, or has a // comment the model job did not see; posts the templated reply on its -// root comment and resolves it only when it is VALID. The PR head is +// root comment and resolves it only when it is VALID, and only on a +// lanes App PR: on a PR a person opened the lanes App cannot resolve, so +// the thread is left open for the person or their babysit worker tier. The PR head is // re-read before each post and each resolve: once it is not the head // this job gated, nothing more is posted or resolved. // 4. Posts the round marker, as the lanes App, once per model run: a @@ -675,6 +677,17 @@ async function postReplies({ api, pr, headSha, headRef, appId, patchState = "", return { escalationReason, posted: [] }; } replies = replies.map((reply) => (reply.disposition === "VALID" ? { ...reply, fixSha: landed.get(reply.thread) } : reply)); + // GitHub lets only a PR's author or a writer resolve its threads, and the + // lanes App is neither on a PR a person opened: there each answered thread + // is left open with the lanes App's reply, for the person or their + // babysit worker tier to resolve. + const appPr = (await api.getPull(pr))?.user?.id === appId; + const resolve = appPr + ? resolveAfterReply + : async (_api, id) => { + log(`${id}: left open; the lanes App cannot resolve a thread on a PR it did not open`); + return true; + }; const posted = []; for (const reply of replies) { const ids = shown.get(reply.thread).comment_ids; @@ -706,7 +719,7 @@ async function postReplies({ api, pr, headSha, headRef, appId, patchState = "", throw new Error(`the reply on ${reply.thread} did not land on its thread`); } posted.push({ thread: reply.thread, disposition: reply.disposition }); - if (reply.disposition === "VALID" && !(await resolveAfterReply(api, reply.thread, [...ids, answer.id], headMoved))) { + if (reply.disposition === "VALID" && !(await resolve(api, reply.thread, [...ids, answer.id], headMoved))) { log(`replied on ${reply.thread} but left it open: it or the PR head changed while the reply was posted`); escalationReason = "feedback-partial"; continue; @@ -716,7 +729,7 @@ async function postReplies({ api, pr, headSha, headRef, appId, patchState = "", escalationReason = "feedback-partial"; break; } - log(`replied on ${reply.thread} (${reply.disposition})${reply.disposition === "VALID" ? "; resolved" : ""}`); + log(`replied on ${reply.thread} (${reply.disposition})${reply.disposition === "VALID" && appPr ? "; resolved" : ""}`); } // A thread left open (INCORRECT, UNCERTAIN or VALID-defer) goes to the // adjudicator's verdict, not straight to a person (owner decision): it is @@ -739,7 +752,7 @@ async function postReplies({ api, pr, headSha, headRef, appId, patchState = "", verdicts: posted.some((entry) => DISPUTED.includes(entry.disposition)) ? parseVerdicts(readAdjudication(), run.head_sha) : new Map(), pushed: run.head_sha !== headSha, headMoved, - resolveAfterReply, + resolveAfterReply: resolve, escalationReason, log, }); diff --git a/.github/scripts/pr-unblock/publish-feedback.test.mjs b/.github/scripts/pr-unblock/publish-feedback.test.mjs index 371577d374..14e0f52a03 100644 --- a/.github/scripts/pr-unblock/publish-feedback.test.mjs +++ b/.github/scripts/pr-unblock/publish-feedback.test.mjs @@ -89,6 +89,8 @@ function fakeApi(overrides = {}) { return calls.filter((call) => call[0] === "readBranch").length > (overrides.movesAfter ?? Infinity) ? PUSHED : NOW; }, getCommit: async (sha) => overrides.commits?.[sha] ?? { files: [{ filename: "src/a.js" }] }, + // The lanes App opened the PR unless the case names another opener. + getPull: async () => ({ user: { id: overrides.opener ?? APP } }), // The PR's commits, oldest first; none earlier than the range by default. listPullCommits: async () => overrides.pullCommits ?? [], compare: async (base, head) => { @@ -1635,3 +1637,36 @@ test("a pass-only round skips pass-only, so the dispatcher can tell its reply ch const result = await run(staleApi([noteThread("PRRT_old")]), null, staleRun(["PRRT_old"])); assert.equal(result.skipReason, "pass-only"); }); + +// On a PR a person opened GitHub lets the lanes App reply but not resolve +// (#10763): each answer posts as the lanes App and the thread stays open for +// the person or their babysit worker tier, with no escalation. +test("on an owner PR a VALID reply and a lane verdict post as the lanes App and leave their threads open", async () => { + const api = fakeApi({ opener: HUMAN }); + const logged = []; + const result = await publish({ + api, + pr: 42, + headSha: NOW, + headRef: "feature", + appId: APP, + modelRun: "5000-1", + reviewerId: REVIEWER, + trustedHumans: new Set([HUMAN]), + readArtifact: () => ({ runText: JSON.stringify(RUN), repliesText: lines(reply({ disposition: "VALID" }), reply({ thread: "PRRT_b", evidence: ["docs/b.md:1"] })) }), + readAdjudication: () => verdicts(judged("PRRT_b", "lane")), + fileExists: exists, + log: (line) => logged.push(line), + }); + assert.equal(result.escalationReason, ""); + assert.deepEqual(api.posted().map((call) => call[2]), [100, 200, 200], "both lane replies and the adjudication reply"); + assert.deepEqual(api.resolved(), []); + assert.ok(logged.some((line) => line === "PRRT_a: left open; the lanes App cannot resolve a thread on a PR it did not open"), logged.join("\n")); + assert.ok(logged.includes("replied on PRRT_a (VALID)"), logged.join("\n")); +}); + +test("on a lanes App PR the same round still resolves both threads", async () => { + const api = fakeApi(); + await run(api, lines(reply({ disposition: "VALID" }), reply({ thread: "PRRT_b", evidence: ["docs/b.md:1"] })), JSON.stringify(RUN), verdicts(judged("PRRT_b", "lane"))); + assert.deepEqual(api.resolved(), ["PRRT_a", "PRRT_b"]); +}); diff --git a/docs/conventions/pr-pipeline/README.md b/docs/conventions/pr-pipeline/README.md index dcac699309..424557142f 100644 --- a/docs/conventions/pr-pipeline/README.md +++ b/docs/conventions/pr-pipeline/README.md @@ -174,8 +174,8 @@ dispatches it. on `workflow_run` `completed` of `pr-review-hosted` and `pr-review-security-hosted`. It takes only the upstream run id from the event and reads the rest back. It dispatches `pr-unblock.yml` with `activity: address-feedback` when the upstream is a `pull_request` run of one of those two paths in -this repository, exactly one open non-draft PR into `main` has its head, the lanes App opened that -PR, it carries no `do-not-merge`, both lanes' newest reviewed-head markers +this repository, exactly one open non-draft PR into `main` has its head, the lanes App or an +[adopted owner](#owner-pull-requests) opened that 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 @@ -229,8 +229,9 @@ sends a pass-only round (input `verify-pass-head`), needing no workable thread, check-run room: `address-feedback-select` lists those threads in `run.json`'s `stale_notes`, no model job or neutral check runs, and `publish-feedback` resolves each one still open and stale while the PR head is the passing one, as the lanes App, with no reply and no round marker, and skips -`pass-only`, which the dispatcher's release reads pass over. Every address-feedback round, -pass-only included, goes only to lanes App PRs. Resolving a thread +`pass-only`, which the dispatcher's release reads pass over. A pass-only round goes only to lanes +App PRs; other address-feedback rounds also go to adopted owner PRs +([Owner pull requests](#owner-pull-requests)). Resolving a thread takes more than the `post-verify-notes` token holds, so that job never resolves one. In `pr-unblock.yml`, `address-feedback-select` (a base job with no model and a read-only token) keeps the threads, `address-feedback` (the model job, `read`, on the read runner with a @@ -241,6 +242,39 @@ pushed commit ([`pr-run-activity.md`](pr-run-activity.md#prepare-review-threads) thread against the reviewed heads the select job recorded in `run.json`, since a review lane edits its one marker in place when it reviews the pushed fix. +### Owner pull requests + +Agent sessions answer review threads under the owner's login, so on a PR the owner opened a lane +answer cannot be told apart from the person's, and the babysit worker tier, which resolves only +threads a lane login answered, resolves none of them (#10763). `address-feedback` therefore also +serves an adopted owner PR, posting its replies as the lanes App: + +- Who: a PR whose author is a `kind: human` id on the base's trusted-actors list, typed `User`, + under an [`adoption`](#adoption) section (`feedback-dispatch.mjs` `ownerAuthored`). Without that + section, or for anyone else, nothing changes: the dispatch skips the PR. +- When: as fix-ci adopts one, no body line starts `Hold:`, and both the PR's last update and its head + commit's committer date are at least `adoption.idle-hours` old (`ownerSkip`), so the lane never + pushes onto a branch a session is still working on. A review usually finishes while the session + is active, so that dispatch skips; each scheduled tick's sweep then sends one idle owner PR whose + reviewed head has no `pr-unblock / address-feedback` check run (`sweepIdleOwner`), deciding it + as for its newest completed review run. +- What: the round is the App PR round, unchanged: the same threads (only the reviewer bot's, a + listed review bot's, or a lane's, under the same trusted-person rules), the same CRITICAL and + security holds, the same `apply-feedback-patch` push under the same patch rules, and no merge. + A thread a person wrote, answered without a fix request, or directed still goes to that person; + on an owner PR such a thread starts no round, and the select job skips it with no stop instead of + escalating `feedback-untaken-threads`. +- Resolve: GitHub lets only a PR's author or a writer resolve its threads, so `publish-feedback` + posts each answer and leaves the thread open, logging it, instead of resolving it. A VALID fix + makes its thread outdated, and the owner's babysit worker tier resolves an outdated thread a lane + login answered (`--own-pr-lane-logins`, from `babysit_lane_logins`). A thread the adjudicator + closed for the lane whose lines did not move stays open for the person. +- No pass-only round goes to an owner PR: a person-opened PR gets no fail-note threads + (`verify-notes.mjs`). + +The push is the power fix-ci already holds on an adopted PR, under the same idle rule; the lane +gains no new grant. + The reviewer bot is `claude[bot]`, a review bot listed in [`review-threads`](#review-threads), or `github-actions[bot]` on a comment whose last non-blank line is a review lane's `claude-lane: ` footer, as ci-workflows' `pr-review-cli.yml` posts its findings; the review @@ -2852,6 +2886,9 @@ adds for that line, since its pr-merge escalation comment is not a pr-unblock la without one the tracker file and the trusted-actor list are not read, nothing is adopted, and only `do-not-merge` and `needs-human` hold. +A person's adopted PR also gets `address-feedback` rounds, under the same idle rule, whose replies +post as the lanes App ([Owner pull requests](#owner-pull-requests)). + | Key | Default | Meaning | |---|---|---| | `idle-hours` | none | Hours both a person's PR (its activity, as the list above reads it) and its head commit must be unchanged; 1 to 720. Absent: nothing is adopted. | From ff18491fde98793b30562099d2f85583eeff5cde Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Wed, 28 Oct 2026 22:58:09 -0400 Subject: [PATCH 2/5] perf(pr-pipeline): spend no sweep read on an active or answered owner PR Owner PRs join only the sweep's idle pass, which reads the head's address-feedback check runs before any other read. The select job's no-stop skip is limited to PRs a User opened. Comments in pr-pipeline.yaml and pr-unblock.yml name owner PRs. Refs: #10763 Co-Authored-By: Claude Opus 5.5 --- .../scripts/pr-unblock/address-feedback.mjs | 2 +- .../pr-unblock/address-feedback.test.mjs | 2 +- .../scripts/pr-unblock/feedback-dispatch.mjs | 34 +++++++++++-------- .../pr-unblock/feedback-dispatch.test.mjs | 14 ++++++++ .github/workflows/pr-unblock.yml | 5 +-- docs/conventions/pr-pipeline.yaml | 12 ++++--- 6 files changed, 46 insertions(+), 23 deletions(-) diff --git a/.github/scripts/pr-unblock/address-feedback.mjs b/.github/scripts/pr-unblock/address-feedback.mjs index 52c24f5a57..9fe9dbbc99 100644 --- a/.github/scripts/pr-unblock/address-feedback.mjs +++ b/.github/scripts/pr-unblock/address-feedback.mjs @@ -592,7 +592,7 @@ export async function select({ // On a PR a person opened, a thread the lane does not take is theirs (one // they wrote, answered, or a lane answer the lanes App cannot resolve // there), and so no stop: it still holds the merge until it is resolved. - if (untaken && pull.user?.id !== appId) { + if (untaken && pull.user?.type === "User") { return { decision: "skip", skipReason: "not-applicable", threads: kept }; } return untaken diff --git a/.github/scripts/pr-unblock/address-feedback.test.mjs b/.github/scripts/pr-unblock/address-feedback.test.mjs index 67429dd5a1..e1617921e6 100644 --- a/.github/scripts/pr-unblock/address-feedback.test.mjs +++ b/.github/scripts/pr-unblock/address-feedback.test.mjs @@ -46,7 +46,7 @@ function fakeApi(overrides = {}) { posted, permissionReads, // The lanes App opened the PR unless the case names another opener. - getPull: async () => ({ number: 42, user: { id: overrides.opener ?? APP }, labels: (overrides.labels ?? []).map((name) => ({ name })) }), + getPull: async () => ({ number: 42, user: overrides.opener ? { id: overrides.opener, type: "User" } : { id: APP, type: "Bot" }, labels: (overrides.labels ?? []).map((name) => ({ name })) }), async getPermission(login) { permissionReads.push(login); return (overrides.permissions ?? { "kyle-sexton": "write" })[login] ?? "none"; diff --git a/.github/scripts/pr-unblock/feedback-dispatch.mjs b/.github/scripts/pr-unblock/feedback-dispatch.mjs index 6135fcdbc0..6dd12cae44 100644 --- a/.github/scripts/pr-unblock/feedback-dispatch.mjs +++ b/.github/scripts/pr-unblock/feedback-dispatch.mjs @@ -512,7 +512,8 @@ export async function dispatchFeedback({ // Returns dispatchFeedback's result with that run's id, or null. export async function sweepReleased({ api, repository, appSenderId, log, settings = DEFAULTS, ...rest }) { const context = { api, repository, appSenderId, log, settings, rest }; - const pulls = await openAppPulls(context); + const listed = await openPulls(context); + const pulls = listed.filter((pull) => pull.user?.id === appSenderId); const dispatched = await sweepPulls(context, (pull) => sweepOne({ ...context, pull }), pulls); if (dispatched !== null) { return dispatched; @@ -523,7 +524,8 @@ export async function sweepReleased({ api, repository, appSenderId, log, setting return stale; } log("sweep: no head with a failing address-feedback stop and nothing left to fix"); - const idle = await sweepPulls(context, (pull) => sweepIdleOwner({ ...context, pull }), pulls); + const owners = listed.filter((pull) => pull.user?.id !== appSenderId); + const idle = owners.length === 0 ? null : await sweepPulls(context, (pull) => sweepIdleOwner({ ...context, pull }), owners); if (idle === null) { log("sweep: no idle owner PR whose reviewed head the lane has not answered"); } @@ -532,22 +534,24 @@ export async function sweepReleased({ api, repository, appSenderId, log, setting // An owner PR's review completes while the person's session is still on it, // so its review-run dispatch skips the idle test; once idle, the tick sends -// it. dispatchFeedback decides the rest (both markers on the head, a thread -// the lane works, no address-feedback check run there yet). The listing's -// last update is checked first, so a PR still active spends no reads. +// it. A PR still active by the listing's last update spends no read, and a +// head that already has a round spends two check-run reads, unless that round's reply +// released it, as sweepOne decides for a lanes App PR. dispatchFeedback +// decides the rest (both markers on the head, a thread the lane works). async function sweepIdleOwner(context) { - const { pull, appSenderId, rest } = context; + const { api, pull, rest, settings } = context; const since = (rest.now ?? Date.now()) - (rest.adoption?.idleHours ?? 0) * 3_600_000; - if (pull.user?.id === appSenderId || !(Date.parse(pull.updated_at ?? "") <= since)) { + if (!(Date.parse(pull.updated_at ?? "") <= since)) { return null; } - const { capped } = await releaseState(context); - return capped ? null : dispatchAtHead(context); + const rounds = await api.listCheckRuns({ sha: pull.head.sha, checkName: settings.checkName, appId: settings.appId }); + return rounds.length === 0 ? dispatchAtHead(context) : sweepOne(context); } -// The selection every sweep uses: each open lanes App PR into the base, and -// each adopted owner PR (dispatchFeedback applies its idle test). -async function openAppPulls({ api, repository, appSenderId, settings, rest }) { +// Each open lanes App PR into the base, and each adopted owner PR +// (dispatchFeedback applies its idle test); only the idle pass takes the +// owner PRs. +async function openPulls({ api, repository, appSenderId, settings, rest }) { return (await api.listOpenPulls(settings.base)).filter( (pull) => (pull.user?.id === appSenderId || ownerAuthored(pull, rest?.adoption)) && @@ -557,11 +561,11 @@ async function openAppPulls({ api, repository, appSenderId, settings, rest }) { ); } -// Each of `pulls` (default: openAppPulls), decided by `decide` in listing -// order until one dispatches. +// Each of `pulls` (default: the open lanes App PRs), decided by `decide` in +// listing order until one dispatches. async function sweepPulls(context, decide, pulls = null) { const { log } = context; - for (const pull of pulls ?? (await openAppPulls(context))) { + for (const pull of pulls ?? (await openPulls(context)).filter((row) => row.user?.id === context.appSenderId)) { let dispatched = null; try { dispatched = await decide(pull); diff --git a/.github/scripts/pr-unblock/feedback-dispatch.test.mjs b/.github/scripts/pr-unblock/feedback-dispatch.test.mjs index 046cc18ba5..4e1ac1c598 100644 --- a/.github/scripts/pr-unblock/feedback-dispatch.test.mjs +++ b/.github/scripts/pr-unblock/feedback-dispatch.test.mjs @@ -1999,6 +1999,20 @@ test("the sweep dispatches an idle owner PR whose reviewed head the lane has not const active = sweepApi(ownerPull({ updated_at: "2026-10-28T11:00:00Z" })); assert.equal(await sweep(active), null); assert.deepEqual(active.calls.commits, []); + // A head the lane already answered costs two check-run reads a tick: no + // review run, status, commit or thread read. + const answered = sweepApi(ownerPull()); + const done = { name: "pr-unblock / address-feedback", status: "completed", conclusion: "success", started_at: "2026-10-28T09:00:00Z", output: { title: "Answered" } }; + const reads = []; + for (const name of ["listCheckRuns", "listCompletedRuns", "listStatuses", "listReviewThreads", "listIssueComments", "pullsForCommit", "headCommittedAt"]) { + const inner = answered[name]; + answered[name] = async (...args) => { + reads.push(args[0]?.checkName ? `${name}:${args[0].checkName}` : name); + return name === "listCheckRuns" ? (args[0].checkName === "pr-unblock / address-feedback" ? [done] : []) : inner(...args); + }; + } + assert.equal(await sweep(answered), null); + assert.deepEqual(reads, ["listCheckRuns:pr-unblock / address-feedback", "listCheckRuns:pr-unblock / publish-feedback"]); // An untrusted person's PR is never listed. const stranger = sweepApi(ownerPull({ user: { id: STRANGER.databaseId, type: "User" } })); assert.equal(await sweep(stranger), null); diff --git a/.github/workflows/pr-unblock.yml b/.github/workflows/pr-unblock.yml index d32d497903..7f127824ea 100644 --- a/.github/workflows/pr-unblock.yml +++ b/.github/workflows/pr-unblock.yml @@ -12,8 +12,9 @@ name: pr-unblock # conflicts and hands them on as a patch, and a script job commits the merge. # address-feedback: # pr-feedback-dispatch.yml sends one after a review lane finishes on a lanes App -# PR with unresolved reviewer-bot threads; a base job keeps the threads, a read -# model job answers them, and script jobs push its patch and post its replies. +# PR, or an idle adopted owner PR, with unresolved reviewer-bot threads; a base +# job keeps the threads, a read model job answers them, and script jobs push +# its patch and post its replies. # After a pr-verify pass at a PR's head that left fail-note threads of earlier # heads open, it sends one with verify-pass-head: a pass-only round, where the # base job lists those threads, no model job runs, and the reply job resolves diff --git a/docs/conventions/pr-pipeline.yaml b/docs/conventions/pr-pipeline.yaml index f3582159c3..35f3500fc1 100644 --- a/docs/conventions/pr-pipeline.yaml +++ b/docs/conventions/pr-pipeline.yaml @@ -80,8 +80,10 @@ activities: # reply for each one. It posts nothing and holds no write token: the read # runner turns its uncommitted fixes into a patch that # apply-feedback-patch pushes, and hands its replies to publish-feedback. - # pr-feedback-dispatch.yml starts it only on pull requests the lanes App - # opened; a trusted person may also dispatch it by hand. + # pr-feedback-dispatch.yml starts it on pull requests the lanes App opened + # and on adopted owner ones once idle (`adoption` below; pr-pipeline + # README, Owner pull requests); a trusted person may also dispatch it by + # hand. skill: source-control:pull-request # The skill's CI mode for review comments: edits only files a finding # names, never under .github/workflows/, and writes replies to a file. @@ -889,8 +891,10 @@ red-main: # id, a person or an App), with no hold label and no `Hold:` body line, and, # for a person's PR, whose activity and head commit are both idle-hours old # (pr-merge counts only trusted-actor activity and pushes; fix-ci, the last -# update). fix-ci fixes its failed ci-status; pr-merge merges it under its -# usual gates. Without this section nothing is adopted. +# update). fix-ci fixes its failed ci-status; address-feedback answers a +# person's PR's lane review threads as the lanes App, under fix-ci's idle +# rule; pr-merge merges it under its usual gates. Without this section nothing +# is adopted. adoption: # 2: a session waiting in the GITHUB_TOKEN gate can be idle up to 2 h during a # breaker trip, so 2 h with no trusted-actor push, comment or label change From 86b97d3f85f8474724bcd05f62582165f886a0bb Mon Sep 17 00:00:00 2001 From: "melodic-automation-lanes[bot]" <337595936+melodic-automation-lanes[bot]@users.noreply.github.com> Date: Thu, 29 Oct 2026 05:24:21 +0000 Subject: [PATCH 3/5] Fix failing CI checks on feat/owner-pr-feedback Lane-Activity: pr-unblock/fix-ci --- .github/scripts/pr-unblock/feedback-dispatch.mjs | 6 +++++- .github/scripts/pr-unblock/fix-ci-dispatch.mjs | 6 +----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/.github/scripts/pr-unblock/feedback-dispatch.mjs b/.github/scripts/pr-unblock/feedback-dispatch.mjs index 6dd12cae44..d6566f21a3 100644 --- a/.github/scripts/pr-unblock/feedback-dispatch.mjs +++ b/.github/scripts/pr-unblock/feedback-dispatch.mjs @@ -124,7 +124,6 @@ import { LOOKBACK_HOURS, LOST } from "../maintenance-reconcile-lost-runs/infra-f import { fixRequesters, humanGatedLabelFrom, onReviewedHead, reviewedHeads, reviewerIdsFrom, 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 { HOLD_LINE } from "./fix-ci-dispatch.mjs"; import { anyHandles, LANE_STOP, laneStop, limitResetsAt, permanentlyHandles } from "./limit-stop.mjs"; import { budgetedFetch } from "./request-budget.mjs"; @@ -159,6 +158,11 @@ export const DEFAULTS = Object.freeze({ const SHA = /^[0-9a-f]{40}$/; const RUN_ID = /^[1-9][0-9]{0,14}$/; +// A body line starting "Hold:", as babysit's BODY_MERGE_HOLD_RE reads it +// (babysit_merge.py): any case, after any non-newline whitespace, NBSP +// included, with a bare CR as a line break too. fix-ci-dispatch.mjs imports +// it from here, so this floored module loads no unfloored one. +export const HOLD_LINE = /(?:^|(?<=[\r\n]))[^\S\r\n]*hold:/i; // No owner is adopted unless the base config's `adoption` section sets // idle-hours (pr-pipeline.yaml). diff --git a/.github/scripts/pr-unblock/fix-ci-dispatch.mjs b/.github/scripts/pr-unblock/fix-ci-dispatch.mjs index 979dcf2205..bc8a0a6ecf 100644 --- a/.github/scripts/pr-unblock/fix-ci-dispatch.mjs +++ b/.github/scripts/pr-unblock/fix-ci-dispatch.mjs @@ -53,7 +53,7 @@ import { } from "../maintenance-reconcile-lost-runs/infra-failure.mjs"; import { postingAttempt } from "../../actions/report-check-run/posting-run.mjs"; import { emptyRecord, isHandled, markHandled } from "./dispatch-record.mjs"; -import { runTitle } from "./feedback-dispatch.mjs"; +import { HOLD_LINE, runTitle } from "./feedback-dispatch.mjs"; import { failingChecks } from "./fix-ci-context.mjs"; import { prHold, reviewReader } from "./gate-source.mjs"; import { laneStop } from "./limit-stop.mjs"; @@ -91,10 +91,6 @@ const HELD_SKIP = "Skipped: awaiting-human"; const RUN_ID = /^[1-9][0-9]{0,14}$/; const MAX_PAGES = 20; -// A body line starting "Hold:", as babysit's BODY_MERGE_HOLD_RE reads it -// (babysit_merge.py): any case, after any non-newline whitespace, NBSP -// included, with a bare CR as a line break too. -export const HOLD_LINE = /(?:^|(?<=[\r\n]))[^\S\r\n]*hold:/i; // The tracker's human-gated label from .work-item-tracker.json's text, as // pr-merge.yml resolves it: config.role_labels["human-gated"] trimmed, or From 3499772bd68a5501eba17ce114f3beb895cc46aa Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Thu, 29 Oct 2026 02:29:40 -0400 Subject: [PATCH 4/5] fix(pr-pipeline): skip untaken threads only on adopted owner PRs and report owner-PR adjudications as left open The select job's no-stop skip read any User-opened PR as an owner PR, so a hand-dispatched round on another person's PR lost its feedback-untaken-threads stop. It now uses ownerAuthored with the base adoption section, moved into address-feedback.mjs so dispatch and select share one predicate. On a PR the lanes App did not open, the resolve stand-in returns LEFT_OPEN, and settleDisputes logs an adjudicator-closed thread as left open for the person instead of resolved. Co-Authored-By: Claude Opus 5.5 --- .../scripts/pr-unblock/address-feedback.mjs | 40 ++++++++++++++++--- .../pr-unblock/address-feedback.test.mjs | 18 +++++---- .../pr-unblock/adjudicate-feedback.mjs | 11 +++-- .../scripts/pr-unblock/feedback-dispatch.mjs | 31 ++------------ .../scripts/pr-unblock/publish-feedback.mjs | 7 ++-- .../pr-unblock/publish-feedback.test.mjs | 2 + 6 files changed, 63 insertions(+), 46 deletions(-) diff --git a/.github/scripts/pr-unblock/address-feedback.mjs b/.github/scripts/pr-unblock/address-feedback.mjs index 9fe9dbbc99..552726ad1c 100644 --- a/.github/scripts/pr-unblock/address-feedback.mjs +++ b/.github/scripts/pr-unblock/address-feedback.mjs @@ -537,6 +537,7 @@ export async function select({ trustedHumans, humanGatedLabel = "needs-human", maxRounds = DEFAULT_MAX_ROUNDS, + adoption = NO_ADOPTION, }) { const [pull, files, issueComments, reviews, reviewComments, threads] = await Promise.all([ api.getPull(pr), @@ -589,10 +590,11 @@ export async function select({ } const stale = new Set(staleVerifyNotes(threads, headSha)); const untaken = threads.some((thread) => thread.isResolved === false && !stale.has(thread.id)); - // On a PR a person opened, a thread the lane does not take is theirs (one - // they wrote, answered, or a lane answer the lanes App cannot resolve - // there), and so no stop: it still holds the merge until it is resolved. - if (untaken && pull.user?.type === "User") { + // On an adopted owner PR (ownerAuthored), a thread the lane does not take + // is the owner's (one they wrote, answered, or a lane answer the lanes App + // cannot resolve there), and so no stop: it still holds the merge until it + // is resolved. Any other person's PR still escalates. + if (untaken && ownerAuthored(pull, adoption)) { return { decision: "skip", skipReason: "not-applicable", threads: kept }; } return untaken @@ -1032,6 +1034,33 @@ export function trustedHumansFrom(trustedActorsPath) { ); } +// No owner is adopted unless the base config's `adoption` section sets +// idle-hours (pr-pipeline.yaml). +export const NO_ADOPTION = Object.freeze({ ownerIds: Object.freeze(new Set()), idleHours: undefined }); + +// An owner PR: one a person on the base's trusted-actors list opened (by id, +// kind human), under an `adoption` section. Its rounds post as the lanes App +// like a lanes App PR's, so a lane answer is told apart from the person's. +export const ownerAuthored = (pull, adoption = NO_ADOPTION) => + adoption.idleHours !== undefined && pull.user?.type === "User" && adoption.ownerIds.has(pull.user?.id); + +// The owners whose PRs a round serves: the base trusted-actors list's people, +// under the base config's `adoption` section. An unreadable section adopts no +// owner, and lanes App PRs proceed. +export async function ownerAdoption(env, log = () => {}) { + try { + const { adoptionFromBase } = await import(new URL("../../actions/resolve-config/resolve-config.mjs", import.meta.url).href); + const idleHours = adoptionFromBase({ BASE_PATH: env.BASE_PATH || ".base" })["idle-hours"]; + if (idleHours === undefined) { + return NO_ADOPTION; + } + return { ownerIds: trustedHumansFrom(env.TRUSTED_ACTORS || `${env.BASE_PATH || ".base"}/.github/standards/trusted-actors/trusted-actors.json`), idleHours }; + } catch (error) { + log(`::warning::cannot read the base adoption section or trusted-actors list (${error.message}); no owner PR is served this run`); + return NO_ADOPTION; + } +} + // The reviewer bot's id comes from the base's trusted-actors list, by login. export function reviewerIdFrom(trustedActorsPath, login = "claude[bot]") { const actor = readActors(trustedActorsPath).find((row) => row.login === login && row.kind === "bot"); @@ -1065,7 +1094,7 @@ export async function main(argv, env, { api, log = (line) => console.log(line) } fetch: await countedFetch(), }); const output = (key, value) => appendFileSync(env.GITHUB_OUTPUT, `${key}=${value}\n`); - const selectThreads = () => { + const selectThreads = async () => { need(env, ["TRUSTED_ACTORS"]); return select({ api: client, @@ -1078,6 +1107,7 @@ export async function main(argv, env, { api, log = (line) => console.log(line) } trustedHumans: trustedHumansFrom(env.TRUSTED_ACTORS), humanGatedLabel: humanGatedLabelFrom(env.TRACKER_CONFIG || ".base/.work-item-tracker.json"), maxRounds: maxRoundsFrom(env.REVIEW_ROUNDS), + adoption: await ownerAdoption(env, log), }); }; if (command === "select" && env.VERIFY_PASS_HEAD) { diff --git a/.github/scripts/pr-unblock/address-feedback.test.mjs b/.github/scripts/pr-unblock/address-feedback.test.mjs index e1617921e6..b0f7651b54 100644 --- a/.github/scripts/pr-unblock/address-feedback.test.mjs +++ b/.github/scripts/pr-unblock/address-feedback.test.mjs @@ -1603,18 +1603,22 @@ test("scanRefusal reads a patch and an output marker together as output, in eith } }); -// On a PR a person opened, an unresolved thread the lane does not take is the -// person's: one they wrote or answered, or a lane answer the lanes App cannot -// resolve there. It is no stop, and it does not read as resolved. -test("on an owner PR an untaken thread skips with no stop; on a lanes App PR it still escalates", async () => { +// On an adopted owner PR, an unresolved thread the lane does not take is the +// owner's: one they wrote or answered, or a lane answer the lanes App cannot +// resolve there. It is no stop, and it does not read as resolved. Any other +// person's PR, or an owner's with no adoption section, still escalates. +test("on an adopted owner PR an untaken thread skips with no stop; elsewhere it still escalates", async () => { const threads = { reviewComments: [comment(100, REVIEWER), comment(101, HUMAN)], threads: [thread("PRRT_1", [100, 101])] }; - const owner = await run(fakeApi({ ...threads, opener: HUMAN })); + const adoption = { ownerIds: new Set([HUMAN]), idleHours: 4 }; + const adopted = (api, set = adoption) => select({ api, pr: 42, headSha: HEAD, repository: "o/r", appId: APP, reviewerId: REVIEWER, trustedHumans: new Set([HUMAN]), adoption: set }); + const owner = await adopted(fakeApi({ ...threads, opener: HUMAN })); assert.equal(owner.decision, "skip"); assert.equal(owner.skipReason, "not-applicable"); assert.notEqual(owner.resolved, true); assert.deepEqual(owner.threads, []); - const app = await run(fakeApi(threads)); - assert.equal(app.escalationReason, "feedback-untaken-threads"); + assert.equal((await adopted(fakeApi(threads))).escalationReason, "feedback-untaken-threads", "a lanes App PR"); + assert.equal((await adopted(fakeApi({ ...threads, opener: STRANGER }))).escalationReason, "feedback-untaken-threads", "a stranger's PR"); + assert.equal((await run(fakeApi({ ...threads, opener: HUMAN }))).escalationReason, "feedback-untaken-threads", "an owner PR with no adoption section"); }); test("on an owner PR a thread a person opened is never kept", async () => { diff --git a/.github/scripts/pr-unblock/adjudicate-feedback.mjs b/.github/scripts/pr-unblock/adjudicate-feedback.mjs index 1d8561c43e..816f90ae79 100755 --- a/.github/scripts/pr-unblock/adjudicate-feedback.mjs +++ b/.github/scripts/pr-unblock/adjudicate-feedback.mjs @@ -47,6 +47,9 @@ export const MAX_THREADS = 3; // human verdict settles it, so a model never closes a thread the lane did not // dispute (settleDisputes). export const DISPUTED = Object.freeze(["INCORRECT", "UNCERTAIN", "VALID-defer"]); +// What a resolveAfterReply stand-in returns when the lanes App answered a +// thread it cannot resolve (a PR it did not open): handled, and still open. +export const LEFT_OPEN = "left-open"; export const VERDICTS = ["lane", "reviewer", "human"]; export const QUESTIONS = ["intent", "design", "security"]; // The first line of each adjudication reply the lanes App posts: this, or @@ -333,7 +336,8 @@ export const FINDING_STANDS_SKIP = "finding-stands"; // - human: no reply, feedback-question-; a question // only the owner answers stays the owner's; // - lane: on an INCORRECT reply, the fixed reply, then the thread is -// resolved; on an UNCERTAIN or VALID-defer reply (the lane could not tell, +// resolved (left open, as LEFT_OPEN, where the lanes App cannot resolve +// it); on an UNCERTAIN or VALID-defer reply (the lane could not tell, // or agreed the finding holds) no reply, the thread stays open (the // ruleset's thread resolution still holds the merge), and // feedback-unadjudicated; @@ -429,12 +433,13 @@ export async function settleDisputes({ api, pr, adjudicatedSha, posted, replies, throw new Error(`the adjudication reply on ${thread} did not land on its thread`); } if (verdict.verdict === "lane") { - if (!(await resolveAfterReply(api, thread, [...before.commentIds, answer.id], headMoved))) { + const resolved = await resolveAfterReply(api, thread, [...before.commentIds, answer.id], headMoved); + if (!resolved) { log(`${thread}: the finding does not hold, but the thread was left open: it or the PR head changed`); raise("feedback-partial"); continue; } - log(`${thread}: the finding does not hold; resolved`); + log(`${thread}: the finding does not hold; ${resolved === LEFT_OPEN ? "left open for the person" : "resolved"}`); } else { log(`${thread}: the finding stands`); stands = true; diff --git a/.github/scripts/pr-unblock/feedback-dispatch.mjs b/.github/scripts/pr-unblock/feedback-dispatch.mjs index d6566f21a3..e6cc2794d6 100644 --- a/.github/scripts/pr-unblock/feedback-dispatch.mjs +++ b/.github/scripts/pr-unblock/feedback-dispatch.mjs @@ -121,7 +121,9 @@ import { pathToFileURL } from "node:url"; import { commitPulls } from "../../actions/check-trusted-trigger/check-trusted-trigger.mjs"; import { countRequests } from "../../actions/log-api-budget/rate-guard.mjs"; import { LOOKBACK_HOURS, LOST } from "../maintenance-reconcile-lost-runs/infra-failure.mjs"; -import { fixRequesters, humanGatedLabelFrom, onReviewedHead, reviewedHeads, reviewerIdsFrom, threadAuthors, trustedHumansFrom } from "./address-feedback.mjs"; +import { fixRequesters, humanGatedLabelFrom, NO_ADOPTION, onReviewedHead, ownerAdoption, ownerAuthored, reviewedHeads, reviewerIdsFrom, threadAuthors, trustedHumansFrom } from "./address-feedback.mjs"; + +export { NO_ADOPTION, ownerAdoption, ownerAuthored }; import { commentItems, gateSource, listLabelEvents } from "./gate-source.mjs"; import { emptyRecord, isHandled, markHandled, readRecord, writeRecord } from "./dispatch-record.mjs"; import { anyHandles, LANE_STOP, laneStop, limitResetsAt, permanentlyHandles } from "./limit-stop.mjs"; @@ -164,16 +166,6 @@ const RUN_ID = /^[1-9][0-9]{0,14}$/; // it from here, so this floored module loads no unfloored one. export const HOLD_LINE = /(?:^|(?<=[\r\n]))[^\S\r\n]*hold:/i; -// No owner is adopted unless the base config's `adoption` section sets -// idle-hours (pr-pipeline.yaml). -export const NO_ADOPTION = Object.freeze({ ownerIds: Object.freeze(new Set()), idleHours: undefined }); - -// An owner PR: one a person on the base's trusted-actors list opened (by id, -// kind human), under an `adoption` section. Its rounds post as the lanes App -// like a lanes App PR's, so a lane answer is told apart from the person's. -export const ownerAuthored = (pull, adoption = NO_ADOPTION) => - adoption.idleHours !== undefined && pull.user?.type === "User" && adoption.ownerIds.has(pull.user?.id); - // Why an owner PR is not adopted for a round now, or "": as fix-ci adopts one // (fix-ci-dispatch.mjs skipReason), no body line starting `Hold:`, and both // the PR's last update and its head commit at least idle-hours old, so the @@ -941,23 +933,6 @@ export async function resolveFeedbackSlot({ number, head, env, github, settings }); } -// The owners whose PRs a round serves: the base trusted-actors list's people, -// under the base config's `adoption` section. An unreadable section adopts no -// owner, and lanes App PRs proceed. -export async function ownerAdoption(env, log = () => {}) { - try { - const { adoptionFromBase } = await import(new URL("../../actions/resolve-config/resolve-config.mjs", import.meta.url).href); - const idleHours = adoptionFromBase({ BASE_PATH: env.BASE_PATH || ".base" })["idle-hours"]; - if (idleHours === undefined) { - return NO_ADOPTION; - } - return { ownerIds: trustedHumansFrom(env.TRUSTED_ACTORS || `${env.BASE_PATH || ".base"}/.github/standards/trusted-actors/trusted-actors.json`), idleHours }; - } catch (error) { - log(`::warning::cannot read the base adoption section or trusted-actors list (${error.message}); no owner PR is served this run`); - return NO_ADOPTION; - } -} - // Writes run-id, the dispatched run, as a step output; the workflow records // it with the upstream run id as an artifact on this run. export async function main(env, { api, github, resolveSlot, log = (line) => console.log(line), now = Date.now() } = {}) { diff --git a/.github/scripts/pr-unblock/publish-feedback.mjs b/.github/scripts/pr-unblock/publish-feedback.mjs index d5d6982d23..0cdf5e5aa8 100755 --- a/.github/scripts/pr-unblock/publish-feedback.mjs +++ b/.github/scripts/pr-unblock/publish-feedback.mjs @@ -108,7 +108,7 @@ import { staleVerifyNotes, trustedHumansFrom, } from "./address-feedback.mjs"; -import { DISPUTED, parseVerdicts, settleDisputes } from "./adjudicate-feedback.mjs"; +import { DISPUTED, LEFT_OPEN, parseVerdicts, settleDisputes } from "./adjudicate-feedback.mjs"; import { inThreadScope, THREAD_ID, threadsNamed } from "./thread-scope.mjs"; import { stopSkip } from "./escalation-routes.mjs"; @@ -680,13 +680,14 @@ async function postReplies({ api, pr, headSha, headRef, appId, patchState = "", // GitHub lets only a PR's author or a writer resolve its threads, and the // lanes App is neither on a PR a person opened: there each answered thread // is left open with the lanes App's reply, for the person or their - // babysit worker tier to resolve. + // babysit worker tier to resolve. The stand-in returns LEFT_OPEN, so + // settleDisputes reports such a thread as left open, not resolved. const appPr = (await api.getPull(pr))?.user?.id === appId; const resolve = appPr ? resolveAfterReply : async (_api, id) => { log(`${id}: left open; the lanes App cannot resolve a thread on a PR it did not open`); - return true; + return LEFT_OPEN; }; const posted = []; for (const reply of replies) { diff --git a/.github/scripts/pr-unblock/publish-feedback.test.mjs b/.github/scripts/pr-unblock/publish-feedback.test.mjs index 14e0f52a03..95181f5137 100644 --- a/.github/scripts/pr-unblock/publish-feedback.test.mjs +++ b/.github/scripts/pr-unblock/publish-feedback.test.mjs @@ -1663,6 +1663,8 @@ test("on an owner PR a VALID reply and a lane verdict post as the lanes App and assert.deepEqual(api.resolved(), []); assert.ok(logged.some((line) => line === "PRRT_a: left open; the lanes App cannot resolve a thread on a PR it did not open"), logged.join("\n")); assert.ok(logged.includes("replied on PRRT_a (VALID)"), logged.join("\n")); + assert.ok(logged.includes("PRRT_b: the finding does not hold; left open for the person"), logged.join("\n")); + assert.ok(!logged.some((line) => line.endsWith("; resolved")), logged.join("\n")); }); test("on a lanes App PR the same round still resolves both threads", async () => { From fdec2b145ca32d8ec9c66725b20da2b5424c15b4 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Thu, 29 Oct 2026 08:31:55 -0400 Subject: [PATCH 5/5] fix(pr-pipeline): clear owner PR stale stops and bound the idle owner sweep An adopted owner PR whose feedback round escalated kept its failing pr-unblock/address-feedback status after the owner resolved every thread without pushing: owner PRs were left out of the stale-stop recovery, and the idle pass ignored a non-releasing round. sweepIdleOwner now runs that recovery for a head with a round, and sweepStale applies ownerSkip's Hold: and idle gates to an owner PR. An idle owner head with no round re-paid about six reads every tick when the dispatch skipped. The idle pass now decides such a head only within IDLE_OWNER_WINDOW_HOURS (two hours, four ticks) of the PR turning idle; past it the head costs one check-run read, and any new update opens the window again. The stale recovery takes no window, since resolving a thread may not move the update time. Co-Authored-By: Claude Opus 5.5 --- .../feat-owner-pr-feedback-7d3a91c2.md | 2 +- .../scripts/pr-unblock/feedback-dispatch.mjs | 44 +++++++--- .../pr-unblock/feedback-dispatch.test.mjs | 87 ++++++++++++++++++- docs/conventions/pr-pipeline/README.md | 6 +- 4 files changed, 123 insertions(+), 16 deletions(-) diff --git a/.changes/pr-pipeline/feat-owner-pr-feedback-7d3a91c2.md b/.changes/pr-pipeline/feat-owner-pr-feedback-7d3a91c2.md index 486f29b88a..6dbd8cbb40 100644 --- a/.changes/pr-pipeline/feat-owner-pr-feedback-7d3a91c2.md +++ b/.changes/pr-pipeline/feat-owner-pr-feedback-7d3a91c2.md @@ -4,5 +4,5 @@ bump: minor ### Added -- `address-feedback` also serves an adopted owner PR: one a `kind: human` account on the base's trusted-actors list opened, under an `adoption` section, with no `Hold:` body line, once the PR and its head commit are `adoption.idle-hours` old, the rule fix-ci adopts by. Its replies post as the lanes App, so the owner's babysit worker tier can tell a lane answer from the person's and resolve the outdated threads the lane answered ([#10763](https://github.com/melodic-software/claude-code-plugins/issues/10763)). The half-hourly sweep sends one such idle PR per tick whose reviewed head has no round yet, since its review usually finishes while the session is still active. Lanes App PRs, and every other author's PRs, are dispatched as before. +- `address-feedback` also serves an adopted owner PR: one a `kind: human` account on the base's trusted-actors list opened, under an `adoption` section, with no `Hold:` body line, once the PR and its head commit are `adoption.idle-hours` old, the rule fix-ci adopts by. Its replies post as the lanes App, so the owner's babysit worker tier can tell a lane answer from the person's and resolve the outdated threads the lane answered ([#10763](https://github.com/melodic-software/claude-code-plugins/issues/10763)). The half-hourly sweep sends one such idle PR per tick whose reviewed head has no round yet, since its review usually finishes while the session is still active; it decides such a head only within two hours of the PR turning idle, and clears a failing address-feedback stop once the owner has resolved every thread. Lanes App PRs, and every other author's PRs, are dispatched as before. - On an owner PR, `publish-feedback` posts each answer and leaves the thread open, since the lanes App cannot resolve a thread on a PR it did not open, and the select job skips a thread the person wrote or answered with no stop instead of escalating `feedback-untaken-threads`. diff --git a/.github/scripts/pr-unblock/feedback-dispatch.mjs b/.github/scripts/pr-unblock/feedback-dispatch.mjs index e6cc2794d6..86925fb954 100644 --- a/.github/scripts/pr-unblock/feedback-dispatch.mjs +++ b/.github/scripts/pr-unblock/feedback-dispatch.mjs @@ -76,8 +76,9 @@ // SWEEP_HEAD_CAP times at that head, is decided as above with the // newest completed review run at that head as the upstream (reviewRunAt), // which the record names. With no such head, and no stale stop, it sends the -// first idle adopted owner PR whose reviewed head has no round yet -// (sweepIdleOwner), since that PR's own review completion found it active. +// first idle adopted owner PR whose reviewed head has no round yet, or whose +// round's stop no thread holds any more (sweepIdleOwner), since that PR's own +// review completion found it active. // A sweep dispatches at most once, since the // dispatched-run record names one upstream and one run, and keeps no record. // @@ -528,20 +529,35 @@ export async function sweepReleased({ api, repository, appSenderId, log, setting return idle; } +// Hours after an owner PR turns idle during which the tick still sends a head +// with no round (judgment: four of pr-unblock-dispatch.yml's half-hourly ticks, +// so a late or dropped tick, or a transient skip, does not lose the PR). +export const IDLE_OWNER_WINDOW_HOURS = 2; + // An owner PR's review completes while the person's session is still on it, // so its review-run dispatch skips the idle test; once idle, the tick sends -// it. A PR still active by the listing's last update spends no read, and a -// head that already has a round spends two check-run reads, unless that round's reply -// released it, as sweepOne decides for a lanes App PR. dispatchFeedback -// decides the rest (both markers on the head, a thread the lane works). +// it. A PR still active by the listing's last update spends no read; an idle +// one spends one check-run read. A head with no round is decided by +// dispatchFeedback (both markers on the head, a thread the lane works, about +// six reads) only within IDLE_OWNER_WINDOW_HOURS of turning idle, so each +// update buys at most that window's ticks of those reads; a new thread, marker +// or fix request moves the update time and opens the window again. A head +// with a round spends a reply check-run read, and a status read unless that +// round's reply released it (sweepOne, as for a lanes App PR); a failing stop +// with no thread left is then cleared as sweepStale clears one on a lanes App +// PR, with no window, since resolving a thread may not move the update time. async function sweepIdleOwner(context) { const { api, pull, rest, settings } = context; const since = (rest.now ?? Date.now()) - (rest.adoption?.idleHours ?? 0) * 3_600_000; - if (!(Date.parse(pull.updated_at ?? "") <= since)) { + const updated = Date.parse(pull.updated_at ?? ""); + if (!(updated <= since)) { return null; } const rounds = await api.listCheckRuns({ sha: pull.head.sha, checkName: settings.checkName, appId: settings.appId }); - return rounds.length === 0 ? dispatchAtHead(context) : sweepOne(context); + if (rounds.length > 0) { + return (await sweepOne(context)) ?? sweepStale(context); + } + return updated > since - IDLE_OWNER_WINDOW_HOURS * 3_600_000 ? dispatchAtHead(context) : null; } // Each open lanes App PR into the base, and each adopted owner PR @@ -589,14 +605,22 @@ export function newestStatus(statuses, context) { // the activity then finds nothing to fix (address-feedback.mjs `resolved`), // and report-stop-status clears the status. Once per stop: a pr-unblock run // for the PR created after the failing status holds the head. The upstream is -// the head's newest completed review run, as for any other dispatch. -async function sweepStale({ api, pull, repository, log, settings, rest }) { +// the head's newest completed review run, as for any other dispatch. An +// adopted owner PR (sweepIdleOwner) keeps its Hold: line and idle gates. +async function sweepStale({ api, pull, repository, appSenderId, log, settings, rest }) { const head = pull.head.sha; const number = pull.number; const status = newestStatus(await api.listStatuses(head), `pr-unblock/${settings.activity}`); if (status?.state !== "failure" && status?.state !== "error") { return null; } + if (pull.user?.id !== appSenderId) { + const why = await ownerSkip({ api, pull, adoption: rest.adoption, now: rest.now ?? Date.now() }); + if (why) { + log(`skip: #${number}: ${why}`); + return null; + } + } const held = (pull.labels ?? []).map((label) => label.name).filter((name) => settings.holdLabels.includes(name)); if (held.length > 0) { log(`skip: #${number} is labeled ${held.join(", ")}`); diff --git a/.github/scripts/pr-unblock/feedback-dispatch.test.mjs b/.github/scripts/pr-unblock/feedback-dispatch.test.mjs index 4e1ac1c598..d77b70bdf7 100644 --- a/.github/scripts/pr-unblock/feedback-dispatch.test.mjs +++ b/.github/scripts/pr-unblock/feedback-dispatch.test.mjs @@ -5,7 +5,7 @@ import { test } from "node:test"; import { LIMIT_DEFAULTS } from "../work-resolve-issue/limits.mjs"; import { emptyRecord, parseRecord, serializeRecord } from "./dispatch-record.mjs"; -import { createGitHubApi, dispatchFeedback, main, NO_ADOPTION, ownerAdoption, SWEEP_HEAD_CAP, sweepReleased } from "./feedback-dispatch.mjs"; +import { createGitHubApi, dispatchFeedback, IDLE_OWNER_WINDOW_HOURS, main, NO_ADOPTION, ownerAdoption, SWEEP_HEAD_CAP, sweepReleased } from "./feedback-dispatch.mjs"; import { tempDir } from "../../../lib/test-tmp.mjs"; // The title GitHub gives a pr-unblock.yml run, read from the workflow file: @@ -1999,8 +1999,8 @@ test("the sweep dispatches an idle owner PR whose reviewed head the lane has not const active = sweepApi(ownerPull({ updated_at: "2026-10-28T11:00:00Z" })); assert.equal(await sweep(active), null); assert.deepEqual(active.calls.commits, []); - // A head the lane already answered costs two check-run reads a tick: no - // review run, status, commit or thread read. + // A head the lane already answered costs two check-run reads and a status + // read a tick: no review run, commit or thread read. const answered = sweepApi(ownerPull()); const done = { name: "pr-unblock / address-feedback", status: "completed", conclusion: "success", started_at: "2026-10-28T09:00:00Z", output: { title: "Answered" } }; const reads = []; @@ -2012,13 +2012,92 @@ test("the sweep dispatches an idle owner PR whose reviewed head the lane has not }; } assert.equal(await sweep(answered), null); - assert.deepEqual(reads, ["listCheckRuns:pr-unblock / address-feedback", "listCheckRuns:pr-unblock / publish-feedback"]); + assert.deepEqual(reads, ["listCheckRuns:pr-unblock / address-feedback", "listCheckRuns:pr-unblock / publish-feedback", "listStatuses"]); // An untrusted person's PR is never listed. const stranger = sweepApi(ownerPull({ user: { id: STRANGER.databaseId, type: "User" } })); assert.equal(await sweep(stranger), null); assert.deepEqual(stranger.calls.dispatch, []); }); +const ownerSweepApi = (pulls) => { + const api = ownerApi({ pulls }); + Object.assign(api, { + listOpenPulls: async () => pulls, + listStatuses: async () => [], + listCompletedRuns: async (path) => (path.endsWith("pr-review-hosted.yml") ? [upstreamRun({ conclusion: "success", created_at: "2026-10-28T08:30:00Z" })] : []), + }); + return api; +}; +const ownerSweep = (api) => + sweepReleased({ + api, + repository: REPOSITORY, + runId: RUN_ID, + appSenderId: APP_ID, + trustedHumans: new Set([KYLE.databaseId]), + resolveSlot: async () => resolvedLane(), + adoption: ADOPTION, + now: NOW_MS, + log: () => {}, + }); + +test("the sweep decides an idle owner head with no round only within the idle window, so a skip is not re-read every tick", async () => { + // Idle since 09:00 + 2 h = 11:00: inside the window, the full decision runs. + const fresh = ownerSweepApi(ownerPull({ updated_at: "2026-10-28T09:00:00Z" })); + fresh.listReviewThreads = async () => [thread([KYLE])]; + assert.equal(await ownerSweep(fresh), null); + assert.deepEqual(fresh.calls.commits, [HEAD]); + // Idle since 07:00 + 2 h = 09:00, more than IDLE_OWNER_WINDOW_HOURS ago: the + // same skip costs one check-run read, no review run, PR, commit or thread read. + const reads = []; + const settled = ownerSweepApi(ownerPull({ updated_at: "2026-10-28T07:00:00Z" })); + for (const name of ["listCheckRuns", "listCompletedRuns", "listStatuses", "listReviewThreads", "listIssueComments", "pullsForCommit", "headCommittedAt"]) { + const inner = settled[name]; + settled[name] = async (...args) => { + reads.push(name); + return inner(...args); + }; + } + assert.equal(await ownerSweep(settled), null); + assert.deepEqual(reads, ["listCheckRuns"]); + assert.equal(IDLE_OWNER_WINDOW_HOURS, 2); + // A new update opens the window again. + const reopened = ownerSweepApi(ownerPull({ updated_at: "2026-10-28T09:59:00Z" })); + assert.equal((await ownerSweep(reopened))?.number, 42); +}); + +test("the sweep clears an idle owner PR's failing stop once no thread is left, past the idle window, under its gates", async () => { + const stopped = (pullOverrides = {}, committedAt = "2026-10-28T08:00:00Z") => { + const api = ownerApi({ pulls: ownerPull({ updated_at: "2026-10-28T07:00:00Z", ...pullOverrides }) }, committedAt); + const round = { name: "pr-unblock / address-feedback", status: "completed", conclusion: "failure", started_at: "2026-10-28T06:00:00Z", output: { title: "Escalated" } }; + Object.assign(api, { + listOpenPulls: async () => ownerPull({ updated_at: "2026-10-28T07:00:00Z", ...pullOverrides }), + listCheckRuns: async ({ checkName }) => (checkName === "pr-unblock / address-feedback" ? [round] : []), + listStatuses: async () => [{ ...STOP, created_at: "2026-10-28T06:30:00Z" }], + listReviewThreads: async () => [{ isResolved: true, comments: { nodes: [] } }], + listWorkflowRuns: async () => [], + listCompletedRuns: async (path) => (path.endsWith("pr-review-hosted.yml") ? [upstreamRun({ conclusion: "success", created_at: "2026-10-28T05:30:00Z" })] : []), + }); + return api; + }; + const api = stopped(); + const result = await ownerSweep(api); + assert.equal(result?.number, 42); + assert.deepEqual(api.calls.dispatch[0].inputs, { "pr-number": "42", activity: "address-feedback", "dispatcher-run-id": RUN_ID }); + // A Hold: line or a recent head commit still keeps the lane off. + for (const [label, held] of [ + ["a Hold: line", stopped({ body: "Hold: mine" })], + ["a recent head commit", stopped({}, "2026-10-28T11:30:00Z")], + ]) { + assert.equal(await ownerSweep(held), null, label); + assert.deepEqual(held.calls.dispatch, [], label); + } + // An open thread leaves the stop to the person. + const open = stopped(); + open.listReviewThreads = async () => [{ isResolved: false, comments: { nodes: [] } }]; + assert.equal(await ownerSweep(open), null); +}); + test("ownerAdoption reads the base adoption section and the trusted list's people; an unreadable base adopts no owner", async () => { const root = new URL("../../../", import.meta.url).pathname; const adoption = await ownerAdoption({ BASE_PATH: root }); diff --git a/docs/conventions/pr-pipeline/README.md b/docs/conventions/pr-pipeline/README.md index 1eb1957453..53cb9a372c 100644 --- a/docs/conventions/pr-pipeline/README.md +++ b/docs/conventions/pr-pipeline/README.md @@ -257,7 +257,11 @@ serves an adopted owner PR, posting its replies as the lanes App: pushes onto a branch a session is still working on. A review usually finishes while the session is active, so that dispatch skips; each scheduled tick's sweep then sends one idle owner PR whose reviewed head has no `pr-unblock / address-feedback` check run (`sweepIdleOwner`), deciding it - as for its newest completed review run. + as for its newest completed review run. It decides such a head only within + `IDLE_OWNER_WINDOW_HOURS` of the PR turning idle, so a head with nothing for the lane is not + re-read every tick; any new update opens the window again. A head whose round stopped with a + failing `pr-unblock/address-feedback` status, and no thread left, is dispatched to clear it, as + for a lanes App PR, under the same `Hold:` and idle gates. - What: the round is the App PR round, unchanged: the same threads (only the reviewer bot's, a listed review bot's, or a lane's, under the same trusted-person rules), the same CRITICAL and security holds, the same `apply-feedback-patch` push under the same patch rules, and no merge.