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..6dbd8cbb40 --- /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; 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/address-feedback.mjs b/.github/scripts/pr-unblock/address-feedback.mjs index cbc5cfb802..9520d4442c 100644 --- a/.github/scripts/pr-unblock/address-feedback.mjs +++ b/.github/scripts/pr-unblock/address-feedback.mjs @@ -600,6 +600,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), @@ -651,7 +652,15 @@ 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 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 ? { decision: "escalate", escalationReason: "feedback-untaken-threads", threads: kept } : { decision: "skip", skipReason: "not-applicable", resolved: true, threads: kept }; } @@ -1088,6 +1097,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"); @@ -1121,7 +1157,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, @@ -1134,6 +1170,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 fe45c1b3b4..abd69505ef 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: 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"; @@ -1619,3 +1620,27 @@ test("scanRefusal reads a patch and an output marker together as output, in eith } } }); + +// 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 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, []); + 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 () => { + 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/adjudicate-feedback.mjs b/.github/scripts/pr-unblock/adjudicate-feedback.mjs index bee0347342..b8771ad39b 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 @@ -341,7 +344,8 @@ export const FINDING_STANDS_SKIP = "finding-stands"; // a security-review finding rated below SECURITY_PERSON_SEVERITY, which is // feedback-unadjudicated; // - 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; @@ -445,12 +449,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 1bea6534c0..86925fb954 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,11 @@ // 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, 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. // // A run of this file an outage lost dispatched nothing the lanes trust when @@ -114,7 +122,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"; @@ -151,6 +161,27 @@ 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; + +// 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"; @@ -266,6 +297,7 @@ export async function dispatchFeedback({ record = emptyRecord(), notesSince = null, passOnly = false, + adoption = NO_ADOPTION, }) { const skip = (why) => { log(`skip: ${why}`); @@ -313,8 +345,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 +415,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 @@ -463,31 +509,75 @@ 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; } 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; } - return stale; + log("sweep: no head with a failing address-feedback stop and nothing left to fix"); + 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"); + } + return idle; } -// The selection every sweep uses: each open lanes App PR into the base. -async function openAppPulls({ api, repository, appSenderId, settings }) { +// 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; 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; + 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 }); + 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 +// (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 && 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 ?? ""), ); } -// 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); @@ -515,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(", ")}`); @@ -770,6 +868,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) { @@ -888,6 +988,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..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, 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: @@ -1864,3 +1864,246 @@ 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, []); + // 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 = []; + 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", "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 }); + 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..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. -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..0cdf5e5aa8 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 @@ -106,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"; @@ -675,6 +677,18 @@ 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. 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 LEFT_OPEN; + }; const posted = []; for (const reply of replies) { const ids = shown.get(reply.thread).comment_ids; @@ -706,7 +720,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 +730,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 +753,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..95181f5137 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,38 @@ 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")); + 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 () => { + 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/.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 3c1bffce97..f296eba678 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 diff --git a/docs/conventions/pr-pipeline/README.md b/docs/conventions/pr-pipeline/README.md index c44d6083e9..53cb9a372c 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,43 @@ 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. 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. + 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 @@ -2878,6 +2916,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. |