diff --git a/.changes/pr-pipeline/fix-resolve-only-job-9649f6d2.md b/.changes/pr-pipeline/fix-resolve-only-job-9649f6d2.md new file mode 100644 index 00000000000..b19af1e6f27 --- /dev/null +++ b/.changes/pr-pipeline/fix-resolve-only-job-9649f6d2.md @@ -0,0 +1,22 @@ +--- +bump: minor +--- + +### Added + +- pr-unblock's `resolve-feedback-threads` activity resolves review threads on a pull request a + person on the trusted-actors list opened, where `publish-feedback`'s `publish-artifact` token is + no writer and leaves its answers open. It resolves a thread whose comments are all a review + bot's or the lanes App's, whose finding no reviewer comment rates CRITICAL and an adjudicator + could judge, and that the lane answered (its VALID reply or a `finding-answered` adjudication is + the last comment) or GitHub reports outdated with no lane reply that left it open. A CRITICAL + finding, a person-decided security finding and any thread a person wrote or joined stay open. + It runs in its own job after `publish-feedback`: a script with no model, granted + `mutate-branch` (`contents: write`), reading the threads from GitHub itself + (`resolve-threads.mjs`, now on the hold floor as an enforcement path), and judging each thread + again on its re-read comments, edits included, before resolving it. It also runs after + `publish-feedback`'s own stops (an escalation, `finding-stands`, `no-model-turn`), never after + a kill-switch or gate skip. `publish-feedback`'s job and grant do not change. +- A script leaf's `checkout: base` has the write runner skip the PR head checkout and its SHA + assert, so the job holding the lane token holds only the trusted base checkout, credentials not + persisted. resolve-config outputs `checkout` (`head` by default) for every script activity. diff --git a/.changes/source-control/fix-resolve-only-job-9649f6d2.md b/.changes/source-control/fix-resolve-only-job-9649f6d2.md new file mode 100644 index 00000000000..ef6fc024954 --- /dev/null +++ b/.changes/source-control/fix-resolve-only-job-9649f6d2.md @@ -0,0 +1,5 @@ +--- +bump: none +--- + +Tests only: the hold-floor pins count the pr-unblock resolve script the floor now holds. diff --git a/.github/actions/resolve-config/action.yml b/.github/actions/resolve-config/action.yml index 507aa3e55b2..668d789005c 100644 --- a/.github/actions/resolve-config/action.yml +++ b/.github/actions/resolve-config/action.yml @@ -188,6 +188,9 @@ outputs: prepare: description: "The leaf's prepare mode (`merge-base`), empty when it sets none." value: ${{ steps.resolve.outputs.prepare }} + checkout: + description: "A script activity's checkout in the write runner: `head` (the default) or `base` (no head checkout); empty for a skill." + value: ${{ steps.resolve.outputs.checkout }} contents: description: The App grant's `contents` permission for the activity's effect. value: ${{ steps.resolve.outputs.contents }} diff --git a/.github/actions/resolve-config/activity-rules.json b/.github/actions/resolve-config/activity-rules.json index 9df17f112ce..75723c0a5e1 100644 --- a/.github/actions/resolve-config/activity-rules.json +++ b/.github/actions/resolve-config/activity-rules.json @@ -27,6 +27,10 @@ "forbidden-effects": ["mutate-branch", "mutate-tracker", "merge"], "never": "Writes to the branch, or posts anything but the recorded replies" }, + "resolve-feedback-threads": { + "forbidden-effects": ["publish-artifact", "mutate-tracker", "merge"], + "never": "Runs a model or head code, pushes, or posts: it only resolves review threads" + }, "review-dependency": { "forbidden-effects": ["publish-artifact", "mutate-tracker", "mutate-branch", "merge"], "never": "Holds a write token, posts to the pull request, merges or closes: its fix leaves only as a patch" diff --git a/.github/actions/resolve-config/callers.test.mjs b/.github/actions/resolve-config/callers.test.mjs index 095e56d0c98..01e3b95ee79 100644 --- a/.github/actions/resolve-config/callers.test.mjs +++ b/.github/actions/resolve-config/callers.test.mjs @@ -7738,8 +7738,9 @@ test("every caller of a reusable holding model legs passes the transcript key", // request-lane-token's kill-switch-ok ends a kill-switch refusal green with no // token, so nothing after it may run on success() alone: every later step is // gated, a step that reads the token runs only when it is set, and a job that -// needs the stopped job's success and reads none of its outputs also needs its -// skip output empty (an output only a token step sets already stops it). A +// needs the stopped job's success and reads none of its outputs also tests its +// skip output for empty (an output only a token step sets already stops it); +// it may also admit named skip reasons, never kill-switch. A // reusable workflow's caller job stands for its job, so the same holds for // the caller's dependents, through the skip output the workflow exposes. test("every kill-switch-ok caller runs no token step and no dependent job on a kill-switch stop", () => { @@ -7752,6 +7753,7 @@ test("every kill-switch-ok caller runs no token step and no dependent job on a k if (cond.replaceAll(`needs.${id}.outputs.${skip} `, "").includes(`needs.${id}.outputs.`)) continue; assert.ok(skip, `${name}:${id} exposes the token step's skip-reason for ${otherId}`); assert.ok(cond.includes(`needs.${id}.outputs.${skip} == ''`), `${name}:${otherId} skips on ${id}'s ${skip}`); + assert.ok(!cond.includes(`needs.${id}.outputs.${skip} == 'kill-switch'`), `${name}:${otherId} never runs on ${id}'s kill-switch`); } }; const names = readdirSync(new URL("../../workflows/", import.meta.url)).filter((f) => /\.ya?ml$/.test(f)); @@ -7826,6 +7828,7 @@ test("every kill-switch-ok caller runs no token step and no dependent job on a k "pr-unblock.yml:apply-patch-rung-up -> pr-run-activity-write.yml", "pr-unblock.yml:apply-shared-copies -> pr-run-activity-write.yml", "pr-unblock.yml:publish-feedback -> pr-run-activity-write.yml", + "pr-unblock.yml:resolve-feedback-threads -> pr-run-activity-write.yml", "pr-unblock.yml:report-escalation -> pr-run-activity-write.yml", "work-resolve-issue.yml:apply-issue-patch -> pr-run-activity-write.yml", "work-resolve-issue.yml:report-outcome -> pr-run-activity-write.yml", @@ -8013,11 +8016,12 @@ test(`${READ}: a broker kill-switch stop runs no model, lease, comment or label, // The caller jobs with a write grant that still start on a read call's stop // are gated only on their own select job, never on the read call's outputs: -// they started on the red broker-check before too. The two write-runner calls -// mint with kill-switch-ok, so the switch still on stops them green at the -// broker; fix-ci-unreported posts its check only on a call's broker allow. +// they started on the red broker-check before too. The write-runner calls +// (resolve-feedback-threads follows publish-feedback) mint with +// kill-switch-ok, so the switch still on stops them green at the broker; +// fix-ci-unreported posts its check only on a call's broker allow. const GATED_ON_SELECT = { - "pr-unblock.yml": ["publish-feedback", "apply-dependency-verdict", "fix-ci-unreported"], + "pr-unblock.yml": ["publish-feedback", "resolve-feedback-threads", "apply-dependency-verdict", "fix-ci-unreported"], }; test(`every caller of ${READ} runs no model, write token, push, comment or label off a read call's kill-switch stop`, () => { const { state } = readRun(STOP, "7"); @@ -8400,3 +8404,67 @@ test("work-unblock.yml: a finished work-resolve-issue run in this repository swe assert.equal(resolves("issues", "true"), true); assert.equal(resolves("workflow_run", "", "false"), false, "the kill switch stops it"); }); + +// #12634: the resolve of answered bot threads on an owner PR runs in its own +// write-runner job after publish-feedback, a script (no model step) whose +// leaf sets checkout: base, so the job holding its contents: write checks out +// only the trusted base, credentials not persisted, and never the PR head. It +// takes no thread ids from another job: the script reads them from GitHub. +// publish-feedback's job and grant stay as they were. +test("pr-unblock.yml: resolve-feedback-threads is its own base-only script job after publish-feedback, which is unchanged", () => { + const { jobs } = workflow("pr-unblock.yml"); + const job = jobs["resolve-feedback-threads"]; + const publish = jobs["publish-feedback"]; + const tokenPermissions = { contents: "read", "pull-requests": "read", checks: "write", issues: "write", "id-token": "write" }; + assert.deepEqual(publish.permissions, tokenPermissions, "publish-feedback's job permissions are unchanged"); + assert.equal(publish.with.activity, "publish-feedback"); + assert.deepEqual(job.permissions, tokenPermissions, "the job token holds no write beyond publish-feedback's"); + assert.equal(String(job.uses).split(" ")[0], "./.github/workflows/pr-run-activity-write.yml"); + assert.equal(job.with.activity, "resolve-feedback-threads"); + assert.equal(job.with["trusted-base"], "default-tip"); + assert.deepEqual(job.concurrency, publish.concurrency); + assert.deepEqual([job.needs].flat(), ["address-feedback-select", "apply-feedback-patch", "publish-feedback"]); + // Only the trigger gate's heads cross from other jobs; no read-runner output. + const crossed = [...JSON.stringify(job.with).matchAll(/needs\.([\w-]+)\.outputs\.([\w-]+)/g)].map((m) => `${m[1]}.${m[2]}`); + assert.deepEqual(crossed.sort(), ["address-feedback-select.head-sha", "apply-feedback-patch.pushed-commit"]); + // It runs on an acted round and on the reply script's own stops, which may + // still have answered other threads; never on a kill-switch or gate skip. + const runs = (result, skip) => + evaluate(job.if.replace("!cancelled() && ", ""), { "needs.publish-feedback.result": result, "needs.publish-feedback.outputs.skip-reason": skip }); + for (const skip of ["", "lane-stopped", "awaiting-human", "finding-stands", "no-model-turn"]) { + assert.equal(runs("success", skip), true, `runs on skip ${JSON.stringify(skip)}`); + assert.equal(runs("failure", skip), false, `not on a failed publish-feedback, skip ${JSON.stringify(skip)}`); + } + for (const skip of ["kill-switch", "superseded-sha", "not-applicable", "pass-only", "disabled-by-config"]) { + assert.equal(runs("success", skip), false, `not on skip ${skip}`); + } + + // The leaf: a script, mutate-branch, checkout base; publish-feedback keeps + // publish-artifact and the head checkout. + const config = parse(read("../../../docs/conventions/pr-pipeline.yaml")); + const leaf = config.activities["resolve-feedback-threads"]; + assert.equal(leaf.script, ".github/scripts/pr-unblock/resolve-threads.mjs"); + assert.equal(leaf.skill, undefined, "no model step"); + assert.equal(leaf.effect, "mutate-branch"); + assert.equal(leaf.checkout, "base"); + assert.equal(config.activities["publish-feedback"].effect, "publish-artifact"); + assert.equal(config.activities["publish-feedback"].checkout, undefined); + const grants = JSON.parse(read("./lane-grants.json"))["1277265216"].lanes["pr-unblock"]; + assert.equal(grants["resolve-feedback-threads"], "mutate-branch"); + assert.equal(grants["publish-feedback"], "publish-artifact"); + + // The write runner: checkout base skips the head checkout and its assert; + // the base checkout persists no credentials; a script runs no skill step. + const steps = workflow("pr-run-activity-write.yml").jobs.run.steps; + const step = (name) => steps.find((s) => s.name === name); + assert.equal(step("Check out the default branch tip").with["persist-credentials"], false); + const on = (name, checkout) => + evaluate(step(name).if, { "steps.lane-token.outputs.token": "t", "steps.config.outputs.checkout": checkout }); + for (const name of ["Check out the PR head", "Assert the head is the gated SHA"]) { + assert.equal(on(name, "base"), false, `${name} skips on checkout: base`); + assert.equal(on(name, "head"), true, `${name} runs on checkout: head`); + assert.equal(on(name, ""), true, `${name} runs for a skill`); + } + assert.match(String(steps.find((s) => s.id === "act-skill").if), /kind == 'skill'/); + assert.match(String(steps.find((s) => s.id === "act-script").if), /kind == 'script'/); +}); diff --git a/.github/actions/resolve-config/lane-grants.json b/.github/actions/resolve-config/lane-grants.json index 4fdb28d475b..5a800b69251 100644 --- a/.github/actions/resolve-config/lane-grants.json +++ b/.github/actions/resolve-config/lane-grants.json @@ -4,7 +4,7 @@ "lanes": { "work-resolve-issue": { "resolve-issue": "read", "apply-issue-patch": "mutate-branch", "report-outcome": "publish-artifact" }, "pr-refine": { "fix-docs": "read", "apply-patch": "mutate-branch" }, - "pr-unblock": { "resolve-conflicts": "read", "apply-conflict-patch": "mutate-branch", "address-feedback": "read", "apply-feedback-patch": "mutate-branch", "publish-feedback": "publish-artifact", "review-dependency": "read", "apply-dependency-patch": "mutate-branch", "apply-dependency-verdict": "publish-artifact", "fix-ci": "read", "apply-patch": "mutate-branch", "sync-shared-copies": "read", "apply-shared-copies": "mutate-branch", "report-escalation": "publish-artifact" }, + "pr-unblock": { "resolve-conflicts": "read", "apply-conflict-patch": "mutate-branch", "address-feedback": "read", "apply-feedback-patch": "mutate-branch", "publish-feedback": "publish-artifact", "resolve-feedback-threads": "mutate-branch", "review-dependency": "read", "apply-dependency-patch": "mutate-branch", "apply-dependency-verdict": "publish-artifact", "fix-ci": "read", "apply-patch": "mutate-branch", "sync-shared-copies": "read", "apply-shared-copies": "mutate-branch", "report-escalation": "publish-artifact" }, "pr-merge": { "merge-pr": "merge", "enable-auto-merge": "merge" }, "work-unblock": { "label-unblocked": "mutate-tracker" }, "intake-triage": { "apply-verdict": "mutate-tracker", "apply-refinement": "mutate-tracker" }, diff --git a/.github/actions/resolve-config/live-config.test.mjs b/.github/actions/resolve-config/live-config.test.mjs index 9e5b7467bba..9a59bf7b487 100644 --- a/.github/actions/resolve-config/live-config.test.mjs +++ b/.github/actions/resolve-config/live-config.test.mjs @@ -158,6 +158,7 @@ test("live config: pr-unblock runs its slots in order", () => { "address-feedback", "apply-feedback-patch", "publish-feedback", + "resolve-feedback-threads", "review-dependency", "apply-dependency-patch", "apply-dependency-verdict", @@ -345,6 +346,23 @@ test("live config: publish-feedback runs the reply script after address-feedback ); }); +// #12634: resolving a thread on an owner PR needs contents: write, so it +// runs in its own script slot after publish-feedback, which keeps +// publish-artifact, with no PR head checkout and on a PR head it never reads. +test("live config: resolve-feedback-threads resolves after publish-feedback with mutate-branch and no head checkout", () => { + const slot = selected("pr-unblock", "resolve-feedback-threads"); + assert.equal(slot.applies, true); + assert.equal(slot.kind, "script"); + assert.equal(slot.script, ".github/scripts/pr-unblock/resolve-threads.mjs"); + assert.equal(slot.checkout, "base"); + assert.equal(slot["reads-untrusted"], true); + assert.deepEqual(slot.needs, ["publish-feedback"]); + assert.deepEqual( + resolveGrant(FILES, { lane: "pr-unblock", activity: "resolve-feedback-threads", configPath: CONFIG_PATH }), + { effect: "mutate-branch", grant: { contents: "write", "pull-requests": "read", issues: "read" } }, + ); +}); + test("live config: report-escalation follows every pr-unblock activity", () => { assert.deepEqual(selected("pr-unblock", "report-escalation").needs, [ "resolve-conflicts", diff --git a/.github/actions/resolve-config/resolve-config.mjs b/.github/actions/resolve-config/resolve-config.mjs index 8847a62097d..9d3bb449874 100644 --- a/.github/actions/resolve-config/resolve-config.mjs +++ b/.github/actions/resolve-config/resolve-config.mjs @@ -278,6 +278,7 @@ function outputsFor({ lane, selected }) { ...(selected["applied-by"]?.patch === undefined ? {} : { "applied-by-patch": selected["applied-by"].patch }), ...(selected.prepare === undefined ? {} : { prepare: selected.prepare }), ...(selected.kind === "skill" ? { "permission-mode": selected["permission-mode"] ?? "dontAsk" } : {}), + ...(selected.kind === "script" ? { checkout: selected.checkout ?? "head" } : {}), ...(selected["issue-dimensions"] === undefined ? {} : { "issue-dimensions": selected["issue-dimensions"], "timeout-minutes": String(selected["timeout-minutes"]) }), diff --git a/.github/actions/resolve-config/resolve-config.test.mjs b/.github/actions/resolve-config/resolve-config.test.mjs index c29cde1106a..5f9c3adf4e4 100644 --- a/.github/actions/resolve-config/resolve-config.test.mjs +++ b/.github/actions/resolve-config/resolve-config.test.mjs @@ -203,6 +203,7 @@ test("a valid config writes ResolvedConfig and the selected activity's outputs", issues: "read", actions: "", "can-commit": "false", + checkout: "head", }); }); @@ -249,6 +250,37 @@ for (const mode of ["bypassPermissions", "acceptEdits", "default", "plan"]) { }); } +// A script's write-runner checkout: head unless the leaf sets base; a skill +// has none, and setting one on a skill or to another value fails the schema. +// The key is inserted after `after`, a line of the leaf. +const withCheckout = (checkout, { activity = "run-tests", after = " script: scripts/run-tests.sh\n" } = {}) => { + const { env } = workspace(); + env.ACTIVITY = activity; + if (activity === "fix-prose") { + env.LANE = "pr-refine"; + } + const path = join(env.BASE_PATH, CONFIG_PATH); + const config = readFileSync(path, "utf8"); + const set = config.replace(after, (line) => `${line} checkout: ${checkout}\n`); + assert.notEqual(set, config); + writeFileSync(path, set); + return env; +}; + +test("a script's checkout output is head unless the leaf sets base; a skill has none", async () => { + const env = withCheckout("base"); + assert.equal(await main({ env, github: srcChanged() }), 0); + assert.equal(readOutputs(env.GITHUB_OUTPUT).checkout, "base"); + const skill = withPermissionMode(undefined); + assert.equal(await main({ env: skill, github: noApi }), 0); + assert.equal(Object.hasOwn(readOutputs(skill.GITHUB_OUTPUT), "checkout"), false); +}); + +test("checkout fails the schema on a skill or with a value other than head or base", async (t) => { + await assertFailsRed(t, withCheckout("pr"), /checkout/); + await assertFailsRed(t, withCheckout("base", { activity: "fix-prose", after: / reads-untrusted: false\n(?=lanes:)/ }), /checkout|script/); +}); + test("without an activity only enabled and slots are output", async () => { const { env } = workspace(); env.ACTIVITY = ""; diff --git a/.github/actions/resolve-config/resolve.mjs b/.github/actions/resolve-config/resolve.mjs index 614b714c3d0..7f799d05553 100644 --- a/.github/actions/resolve-config/resolve.mjs +++ b/.github/actions/resolve-config/resolve.mjs @@ -903,6 +903,7 @@ function resolveSlot( ...(activity["web-access"] === undefined ? {} : { "web-access": activity["web-access"] }), ...(activity.tidy === undefined ? {} : { tidy: activity.tidy }), ...(activity["permission-mode"] === undefined ? {} : { "permission-mode": activity["permission-mode"] }), + ...(activity.checkout === undefined ? {} : { checkout: activity.checkout }), ...(activity["applied-by"] === undefined ? {} : { "applied-by": activity["applied-by"] }), effect: activity.effect, gating: activity.gating, diff --git a/.github/scripts/pr-merge/merge-holds.mjs b/.github/scripts/pr-merge/merge-holds.mjs index 1178d9493d2..2315fd9cc9c 100755 --- a/.github/scripts/pr-merge/merge-holds.mjs +++ b/.github/scripts/pr-merge/merge-holds.mjs @@ -351,6 +351,7 @@ export const HOLD_PATH_CLASSES = Object.freeze([ [".github/scripts/pr-unblock/report-escalation.sh", "enforcement"], [".github/scripts/pr-unblock/adjudicate-feedback.mjs", "enforcement"], [".github/scripts/pr-unblock/publish-feedback.mjs", "enforcement"], + [".github/scripts/pr-unblock/resolve-threads.mjs", "enforcement"], [".github/scripts/pr-unblock/request-budget.mjs", "enforcement"], [".github/scripts/pr-verify/resolve-pr.mjs", "enforcement"], [".github/scripts/pr-verify/write-verdict.mjs", "enforcement"], diff --git a/.github/scripts/pr-merge/refire-refused.mjs b/.github/scripts/pr-merge/refire-refused.mjs index e0db7f42afc..f7f2b92b663 100755 --- a/.github/scripts/pr-merge/refire-refused.mjs +++ b/.github/scripts/pr-merge/refire-refused.mjs @@ -97,6 +97,7 @@ const LANES = Object.freeze({ "apply-conflict-patch", "apply-feedback-patch", "publish-feedback", + "resolve-feedback-threads", "apply-dependency-patch", "apply-dependency-verdict", "apply-patch", diff --git a/.github/scripts/pr-unblock/address-feedback.mjs b/.github/scripts/pr-unblock/address-feedback.mjs index 04a6dde3f50..e4d9709222c 100644 --- a/.github/scripts/pr-unblock/address-feedback.mjs +++ b/.github/scripts/pr-unblock/address-feedback.mjs @@ -643,9 +643,15 @@ export function humanGatedLabelFrom(path) { // set of ids, given on a PR carrying the human-gated label), a thread is // worked only when a reply is a fix request by one of them, after the // escalation when there is one. +// A comment by a review bot: the reviewer bot, a listed review bot, or a +// review lane's github-actions[bot] comment (threadAuthors, resolve-threads.mjs). +export const isReviewerComment = (comment, { reviewerId, reviewerIds = [] }) => + comment.authorId === reviewerId || reviewerIds.includes(comment.authorId) || isLaneComment(comment); +// A pr-verify verifier note's root comment. +export const isVerifierRoot = (comment) => comment?.authorId === MARKER_AUTHOR_ID && firstLine(comment.body) === VERIFY_NOTE_MARKER; export function threadAuthors(comments, { reviewerId, reviewerIds = [], appId, trustedHumans = new Set(), requesters = null, fixers = requesters ?? new Set() }) { - const verifier = comments.length > 0 && comments[0].authorId === MARKER_AUTHOR_ID && firstLine(comments[0].body) === VERIFY_NOTE_MARKER; - const reviewer = (comment) => comment.authorId === reviewerId || reviewerIds.includes(comment.authorId) || isLaneComment(comment); + const verifier = comments.length > 0 && isVerifierRoot(comments[0]); + const reviewer = (comment) => isReviewerComment(comment, { reviewerId, reviewerIds }); const isHuman = (comment) => trustedHumans.has(comment.authorId) && (comment.lastEditedAt == null || trustedHumans.has(comment.editorId)); if (comments.length === 0 || (!reviewer(comments[0]) && !verifier && !isHuman(comments[0]))) { @@ -1277,7 +1283,11 @@ export async function currentThreads({ api, pr, reviewerId, reviewerIds = [], ap const THREAD_QUERY = `query($id: ID!) { node(id: $id) { ... on PullRequestReviewThread { - isResolved pullRequest { number } comments(first: 100) { totalCount nodes { databaseId } } + isResolved isOutdated pullRequest { number } + comments(first: 100) { + totalCount + nodes { databaseId body lastEditedAt editor { ... on User { databaseId } ... on Bot { databaseId } } } + } } } }`; @@ -1531,6 +1541,10 @@ export function createGitHubApi({ token, repository, apiUrl, graphqlUrl, fetch: pr: node?.pullRequest?.number, commentCount: node?.comments?.totalCount, commentIds: (node?.comments?.nodes ?? []).map((comment) => comment.databaseId), + // As listThreads' comment nodes, with each body: resolve-threads.mjs + // judges the thread again on them. + isOutdated: node?.isOutdated, + nodes: node?.comments?.nodes ?? [], }; }, // From git/ref, which GET /pulls can lag behind after a push. diff --git a/.github/scripts/pr-unblock/adjudicate-feedback.mjs b/.github/scripts/pr-unblock/adjudicate-feedback.mjs index 48381cccf5f..fc4cd76a391 100755 --- a/.github/scripts/pr-unblock/adjudicate-feedback.mjs +++ b/.github/scripts/pr-unblock/adjudicate-feedback.mjs @@ -88,6 +88,7 @@ const PROSE_LOCATION = /(? CRITICAL.test(String(body ?? "")); const firstLine = (body) => String(body ?? "").split("\n")[0].replace(/\r$/, ""); const safePath = (path) => typeof path === "string" && PATH.test(path) && !DOT_SEGMENT.test(path); diff --git a/.github/scripts/pr-unblock/publish-feedback.mjs b/.github/scripts/pr-unblock/publish-feedback.mjs index ef6cd90db49..c0995fdd25b 100755 --- a/.github/scripts/pr-unblock/publish-feedback.mjs +++ b/.github/scripts/pr-unblock/publish-feedback.mjs @@ -110,7 +110,7 @@ import { staleVerifyNotes, trustedHumansFrom, } from "./address-feedback.mjs"; -import { DISPUTED, LEFT_OPEN, parseVerdicts, settleDisputes } from "./adjudicate-feedback.mjs"; +import { DISPUTED, FINDING_ANSWERED, LEFT_OPEN, parseVerdicts, settleDisputes } from "./adjudicate-feedback.mjs"; import { inThreadScope, NOTE_ID, THREAD_ID, threadsNamed } from "./thread-scope.mjs"; import { stopSkip } from "./escalation-routes.mjs"; @@ -145,6 +145,13 @@ const LABELS = { "VALID-defer": "VALID (defer), left open", }; +// Whether a lane reply is one after which a lanes App PR's thread is resolved: +// the VALID reply (renderReply) or an adjudication that found the finding +// does not hold (FINDING_ANSWERED). On an owner PR such a thread is left open +// here, and resolve-threads.mjs resolves it. +const RESOLVING_FIRST_LINES = new Set([`**pr-unblock / address-feedback: ${LABELS.VALID}**`, FINDING_ANSWERED]); +export const isResolvingAnswer = (body) => RESOLVING_FIRST_LINES.has(String(body ?? "").split("\n")[0].replace(/\r$/, "")); + class Invalid extends Error {} function check(condition, why) { diff --git a/.github/scripts/pr-unblock/resolve-threads.mjs b/.github/scripts/pr-unblock/resolve-threads.mjs new file mode 100755 index 00000000000..e28d8482eea --- /dev/null +++ b/.github/scripts/pr-unblock/resolve-threads.mjs @@ -0,0 +1,200 @@ +#!/usr/bin/env node +// resolve-threads.mjs: the resolve job of the pr-unblock lane's +// address-feedback round, a script activity with no model and no head +// checkout (checkout: base). On a PR a trusted person opened, the lanes App is +// no writer to publish-feedback.mjs's publish-artifact token, so it replies +// and leaves each answered thread open; this job's mutate-branch token +// (contents: write) can resolve it. It reads the PR's threads from GitHub +// itself, never from another job's outputs, and resolves each one judgeThread +// rules `answered` or `outdated`: +// +// - every comment is a review bot's (isReviewerComment) or the lanes App's, +// and none was edited by another account: a thread a person wrote or +// joined stays theirs; +// - its root is a review bot's finding that no reviewer comment rates +// CRITICAL (isCritical) and that adjudicate-feedback.mjs adjudicable +// would let an adjudicator judge, so a security finding a person decides +// (CRITICAL or unrated, or a listed bot's on a head the security-review +// lane held or has not reviewed) stays open, as does a pr-verify note, +// which publish-feedback.mjs's pass-only round owns; +// - answered: its last comment is the lanes App's resolving answer +// (publish-feedback.mjs isResolvingAnswer); outdated: GitHub reports it +// outdated and no lanes App comment on it left it open (an INCORRECT, +// UNCERTAIN or VALID-defer reply, an escalation, or a finding that stood). +// +// Each thread is re-read just before its resolve and left alone unless it +// still holds the comments it was judged on and judgeThread, run again on +// their bodies and edits as they are then, still resolves it. Only a PR whose author +// is a person on the base's trusted-actors list is served; any other skips +// not-applicable. A failed resolve is logged, the rest are tried, and the job +// then fails so a re-run tries again. +// +// Environment: +// GH_TOKEN the lane token (contents: write, pull-requests: read) +// GITHUB_REPOSITORY owner/repo +// PR_NUMBER the gated pull request +// APP_SENDER_ID the lanes App bot's user id +// TRUSTED_ACTORS the base's trusted-actors.json: the owners and the reviewer bot +// REVIEWER_IDS the listed review bots' ids, one per line +// SKIP_REASON_FILE where the runner reads the skip reason + +import { writeFileSync } from "node:fs"; +import { pathToFileURL } from "node:url"; + +import { + createGitHubApi, + isLaneComment, + isReviewerComment, + isVerifierRoot, + laneOf, + need, + positive, + reviewerIdFrom, + reviewerIdsFrom, + securitySignals, + securityState, + trustedHumansFrom, +} from "./address-feedback.mjs"; +import { adjudicable, isCritical } from "./adjudicate-feedback.mjs"; +import { isResolvingAnswer } from "./publish-feedback.mjs"; + +// The most comments one thread read holds (address-feedback.mjs THREADS_QUERY). +const THREAD_COMMENT_CAP = 100; +export const RESOLVES = Object.freeze(["answered", "outdated"]); + +// An owner PR: opened by a person on the base's trusted-actors list. +export const ownerPull = (pull, trustedHumans) => pull?.user?.type === "User" && trustedHumans.has(pull.user.id); + +// One thread's verdict: `answered` or `outdated` (resolve it), else why it +// stays open. `thread` is a listThreads node, `rows` its comments' REST rows +// in thread order, `review` the root comment's review, `security` the PR's +// securitySignals. +export function judgeThread({ thread, rows, review, security, reviewerId, reviewerIds = [], appId }) { + if (thread.isResolved !== false) { + return "resolved"; + } + const nodes = thread.comments?.nodes ?? []; + if (thread.comments?.totalCount > THREAD_COMMENT_CAP || rows.length === 0 || rows.length !== nodes.length || rows.some((row) => row === undefined)) { + return "unread"; + } + const comments = rows.map((row) => ({ authorId: row.user?.id, body: row.body ?? "" })); + if (isVerifierRoot(comments[0])) { + return "verifier-note"; + } + const bot = (comment, i) => { + const edited = nodes[i].lastEditedAt != null && nodes[i].editor?.databaseId !== comment.authorId; + return !edited && (comment.authorId === appId || isReviewerComment(comment, { reviewerId, reviewerIds })); + }; + if (!comments.every(bot) || comments[0].authorId === appId) { + return "not-bot-only"; + } + // Tagged as keepThreads tags them for adjudicable. + const tagged = comments.map((comment, i) => { + const author = comment.authorId === appId ? "lane" : "reviewer"; + const lane = isLaneComment(comment) ? laneOf(comment.body) : null; + const listed = author === "reviewer" && reviewerIds.includes(comment.authorId); + const state = listed && i === 0 ? { security: review ? securityState(security, review) : "pending" } : {}; + return { author, ...(lane ? { lane } : {}), ...(listed ? { listed } : {}), ...state, body: comment.body }; + }); + if (tagged.some((comment) => comment.author === "reviewer" && isCritical(comment.body)) || !adjudicable(tagged)) { + return "person-decides"; + } + const lane = comments.filter((comment) => comment.authorId === appId); + if (comments.at(-1).authorId === appId && isResolvingAnswer(comments.at(-1).body)) { + return "answered"; + } + if (lane.some((comment) => !isResolvingAnswer(comment.body))) { + return "left-open"; + } + return thread.isOutdated === true ? "outdated" : "current"; +} + +const sameIds = (a, b) => a.length === b.length && a.every((id, i) => id === b[i]); + +export async function resolveThreads({ api, pr, appId, reviewerId, reviewerIds = [], trustedHumans, log = () => {} }) { + const pull = await api.getPull(pr); + if (!ownerPull(pull, trustedHumans)) { + log(`#${pr} was not opened by a person on the trusted-actors list; nothing is resolved`); + return { skipReason: "not-applicable", resolved: [] }; + } + const [threads, reviewComments, reviews, issueComments] = [await api.listThreads(pr), await api.listReviewComments(pr), await api.listReviews(pr), await api.listIssueComments(pr)]; + const byId = new Map(reviewComments.map((row) => [row.id, row])); + const reviewsById = new Map(reviews.map((row) => [row.id, row])); + const security = securitySignals(issueComments); + const resolved = []; + const failed = []; + for (const thread of threads) { + const rows = (thread.comments?.nodes ?? []).map((node) => byId.get(node.databaseId)); + const judge = (t, r) => judgeThread({ thread: t, rows: r, review: reviewsById.get(rows[0]?.pull_request_review_id), security, reviewerId, reviewerIds, appId }); + const verdict = judge(thread, rows); + if (!RESOLVES.includes(verdict)) { + if (verdict !== "resolved") { + log(`${thread.id}: left open (${verdict})`); + } + continue; + } + try { + // Judged again on the re-read bodies and edits: a comment edited since + // the first read (say, rated CRITICAL) leaves the thread open. + const now = await api.readThread(thread.id); + const nodes = now.nodes ?? []; + const fresh = + now.pr === pr && sameIds(now.commentIds, rows.map((row) => row.id)) && nodes.every((node) => typeof node.body === "string") + ? judge({ ...thread, isResolved: now.isResolved, isOutdated: now.isOutdated, comments: { totalCount: now.commentCount, nodes } }, rows.map((row, i) => ({ ...row, body: nodes[i].body }))) + : "changed"; + if (!RESOLVES.includes(fresh)) { + log(`${thread.id}: left open; it changed since it was read (${fresh})`); + continue; + } + if ((await api.resolveThread(thread.id)) !== true) { + throw new Error("it does not read resolved"); + } + resolved.push(thread.id); + log(`${thread.id}: resolved (${verdict})`); + } catch (error) { + failed.push(thread.id); + log(`could not resolve ${thread.id}: ${String(error?.message ?? error).slice(0, 300)}`); + } + } + log(`resolved ${resolved.length} review thread(s) on #${pr}`); + if (failed.length > 0) { + throw new Error(`could not resolve ${failed.join(", ")}`); + } + return { skipReason: "", resolved }; +} + +export async function main(env, { api, log = (line) => console.log(line) } = {}) { + need(env, ["GH_TOKEN", "GITHUB_REPOSITORY", "PR_NUMBER", "APP_SENDER_ID", "TRUSTED_ACTORS", "SKIP_REASON_FILE"]); + const { skipReason } = await resolveThreads({ + api: + api ?? + createGitHubApi({ + token: env.GH_TOKEN, + repository: env.GITHUB_REPOSITORY, + apiUrl: env.GITHUB_API_URL ?? "https://api.github.com", + graphqlUrl: env.GITHUB_GRAPHQL_URL ?? "https://api.github.com/graphql", + }), + pr: positive(env.PR_NUMBER, "PR_NUMBER"), + appId: positive(env.APP_SENDER_ID, "APP_SENDER_ID"), + reviewerId: reviewerIdFrom(env.TRUSTED_ACTORS), + reviewerIds: reviewerIdsFrom(env.REVIEWER_IDS), + trustedHumans: trustedHumansFrom(env.TRUSTED_ACTORS), + log, + }); + if (skipReason) { + writeFileSync(env.SKIP_REASON_FILE, `${skipReason}\n`); + } + return 0; +} + +if (process.argv[1] && import.meta.url === pathToFileURL(process.argv[1]).href) { + main(process.env).then( + (code) => { + process.exitCode = code; + }, + (error) => { + console.log(`::error::resolve-threads failed: ${error.message}`); + process.exitCode = 1; + }, + ); +} diff --git a/.github/scripts/pr-unblock/resolve-threads.test.mjs b/.github/scripts/pr-unblock/resolve-threads.test.mjs new file mode 100644 index 00000000000..3b91d65acb7 --- /dev/null +++ b/.github/scripts/pr-unblock/resolve-threads.test.mjs @@ -0,0 +1,178 @@ +import assert from "node:assert/strict"; +import { test } from "node:test"; + +import { FINDING_STANDS, MARKER_AUTHOR_ID, securitySignals, VERIFY_NOTE_MARKER } from "./address-feedback.mjs"; +import { FINDING_ANSWERED } from "./adjudicate-feedback.mjs"; +import { renderReply } from "./publish-feedback.mjs"; +import { judgeThread, resolveThreads } from "./resolve-threads.mjs"; + +const APP = 900; +const CLAUDE = 41; +const CODEX = 199175422; +const OWNER = 100; +const STRANGER = 555; +const HEAD = "a".repeat(40); + +const REVIEW = "This drops the error the caller needs."; +const CRITICAL_REVIEW = "severity: CRITICAL\n\nThis writes the token to the log."; +const laneFooter = (lane) => `\n\n\`claude-lane: ${lane}\``; +const reply = (disposition) => renderReply({ disposition, evidence: ["lib/a.mjs:3"], fixSha: disposition === "VALID" ? HEAD : "", note: "The lane's note." }); +const VALID = reply("VALID"); +const ANSWERED = `${FINDING_ANSWERED}\n**pr-unblock / adjudicate-feedback: the finding does not hold, resolved**`; + +let nextId = 1; +// A listThreads node and its REST rows; each comment is [author, body, editor?]. +function make(comments, { id = "PRRT_a", outdated = false, resolved = false } = {}) { + const rows = comments.map(([user, body]) => ({ id: nextId++, user: { id: user }, body, pull_request_review_id: 7 })); + const thread = { + id, + isResolved: resolved, + isOutdated: outdated, + comments: { + totalCount: rows.length, + nodes: rows.map((row, i) => ({ + databaseId: row.id, + lastEditedAt: comments[i][2] ? "2026-11-01T00:00:00Z" : null, + editor: comments[i][2] ? { databaseId: comments[i][2] } : null, + })), + }, + }; + return { thread, rows }; +} + +const NO_SECURITY = { reviewed: new Set(), holds: [] }; +const judge = (made, { review = { commit_id: HEAD, submitted_at: "2026-11-01T00:00:00Z" }, security = NO_SECURITY } = {}) => + judgeThread({ ...made, review, security, reviewerId: CLAUDE, reviewerIds: [CODEX], appId: APP }); + +test("a bot finding the lane answered VALID or an adjudication answered resolves, outdated or not", () => { + assert.equal(judge(make([[CLAUDE, REVIEW], [APP, VALID]])), "answered"); + assert.equal(judge(make([[CLAUDE, REVIEW], [APP, reply("INCORRECT")], [APP, ANSWERED]])), "answered"); + assert.equal(judge(make([[MARKER_AUTHOR_ID, `${REVIEW}${laneFooter("claude-review")}`], [APP, VALID]])), "answered"); +}); + +test("an outdated bot finding resolves; a current one with no answer stays", () => { + assert.equal(judge(make([[CLAUDE, REVIEW]], { outdated: true })), "outdated"); + assert.equal(judge(make([[CLAUDE, REVIEW]])), "current"); + // A reviewer re-raise after the answer: not answered, so only outdated resolves it. + assert.equal(judge(make([[CLAUDE, REVIEW], [APP, VALID], [CLAUDE, "Still wrong."]])), "current"); + assert.equal(judge(make([[CLAUDE, REVIEW], [APP, VALID], [CLAUDE, "Still wrong."]], { outdated: true })), "outdated"); +}); + +test("a CRITICAL bot finding stays open, answered or outdated", () => { + assert.equal(judge(make([[CLAUDE, CRITICAL_REVIEW], [APP, VALID]])), "person-decides"); + assert.equal(judge(make([[CLAUDE, CRITICAL_REVIEW]], { outdated: true })), "person-decides"); + assert.equal(judge(make([[MARKER_AUTHOR_ID, `${CRITICAL_REVIEW}${laneFooter("claude-review")}`], [APP, VALID]])), "person-decides"); + // A reviewer comment after the root that rates it CRITICAL holds it too. + assert.equal(judge(make([[CLAUDE, REVIEW], [CLAUDE, CRITICAL_REVIEW]], { outdated: true })), "person-decides"); +}); + +test("a security finding a person decides stays open: unrated security-review, a listed bot's on a held or unreviewed head", () => { + assert.equal(judge(make([[MARKER_AUTHOR_ID, `Unsafe eval.${laneFooter("claude-security-review")}`], [APP, VALID]])), "person-decides"); + assert.equal(judge(make([[MARKER_AUTHOR_ID, `severity: IMPORTANT\n\nUnsafe eval.${laneFooter("claude-security-review")}`], [APP, VALID]])), "answered"); + const codex = () => make([[CODEX, REVIEW]], { outdated: true }); + assert.equal(judge(codex()), "person-decides", "the security-review lane has not reviewed the bot's head"); + const reviewed = securitySignals([{ user: { id: MARKER_AUTHOR_ID }, body: `` }]); + assert.equal(judge(codex(), { security: reviewed }), "outdated"); + assert.equal(judge(codex(), { security: { ...reviewed, holds: [{ sha: HEAD, at: 0 }] } }), "person-decides"); +}); + +test("a human-authored or human-joined thread stays open", () => { + assert.equal(judge(make([[OWNER, REVIEW]], { outdated: true })), "not-bot-only"); + assert.equal(judge(make([[CLAUDE, REVIEW], [APP, VALID], [OWNER, "Thanks"]], { outdated: true })), "not-bot-only"); + assert.equal(judge(make([[CLAUDE, REVIEW], [STRANGER, "+1"], [APP, VALID]])), "not-bot-only"); + // A bot comment someone else edited is no longer the bot's alone. + assert.equal(judge(make([[CLAUDE, REVIEW, OWNER], [APP, VALID]])), "not-bot-only"); + assert.equal(judge(make([[CLAUDE, REVIEW, CLAUDE], [APP, VALID]])), "answered"); + // A thread the lanes App opened is no review finding. + assert.equal(judge(make([[APP, "note"]], { outdated: true })), "not-bot-only"); +}); + +test("a thread the lane left open or a pr-verify note stays open even when outdated", () => { + for (const body of [reply("INCORRECT"), reply("UNCERTAIN"), reply("VALID-defer"), `${FINDING_STANDS}\n**the finding stands**`]) { + assert.equal(judge(make([[CLAUDE, REVIEW], [APP, body]], { outdated: true })), "left-open", body.split("\n")[0]); + } + assert.equal(judge(make([[MARKER_AUTHOR_ID, `${VERIFY_NOTE_MARKER}\nA note.`]], { outdated: true })), "verifier-note"); + assert.equal(judge(make([[CLAUDE, REVIEW]], { outdated: true, resolved: true })), "resolved"); +}); + +// A fake API holding one PR's threads; resolveThread marks a thread resolved. +// `edits` maps a thread id to the [body, editor] its comments hold at the re-read. +function fakeApi({ author = { id: OWNER, type: "User" }, made = [], changed = new Set(), edits = {}, failing = new Set() } = {}) { + const calls = []; + const threads = made.map(({ thread }) => thread); + const rows = made.flatMap((m) => m.rows); + return { + calls, + getPull: async () => ({ user: author }), + listThreads: async () => threads, + listReviewComments: async () => rows, + listReviews: async () => [{ id: 7, commit_id: HEAD, submitted_at: "2026-11-01T00:00:00Z", user: { id: CLAUDE } }], + listIssueComments: async () => [], + readThread: async (id) => { + const thread = threads.find((t) => t.id === id); + const nodes = thread.comments.nodes.map((node, i) => { + const [body, editor] = edits[id]?.[i] ?? [rows.find((row) => row.id === node.databaseId).body]; + return editor ? { ...node, body, lastEditedAt: "2026-11-02T00:00:00Z", editor: { databaseId: editor } } : { ...node, body }; + }); + const ids = nodes.map((node) => node.databaseId); + const commentIds = changed.has(id) ? [...ids, 99999] : ids; + return { isResolved: thread.isResolved, isOutdated: thread.isOutdated, pr: 12, commentCount: commentIds.length, commentIds, nodes }; + }, + resolveThread: async (id) => { + calls.push(id); + if (failing.has(id)) { + throw new Error("HTTP 403"); + } + threads.find((t) => t.id === id).isResolved = true; + return true; + }, + }; +} + +const run = (api) => resolveThreads({ api, pr: 12, appId: APP, reviewerId: CLAUDE, reviewerIds: [CODEX], trustedHumans: new Set([OWNER]) }); + +test("on an owner PR it resolves only the answered or outdated non-CRITICAL bot threads", async () => { + const api = fakeApi({ + made: [ + make([[CLAUDE, REVIEW], [APP, VALID]], { id: "PRRT_answered" }), + make([[CLAUDE, REVIEW]], { id: "PRRT_outdated", outdated: true }), + make([[CLAUDE, CRITICAL_REVIEW], [APP, VALID]], { id: "PRRT_critical" }), + make([[OWNER, REVIEW]], { id: "PRRT_human", outdated: true }), + make([[CLAUDE, REVIEW]], { id: "PRRT_current" }), + ], + }); + assert.deepEqual(await run(api), { skipReason: "", resolved: ["PRRT_answered", "PRRT_outdated"] }); + assert.deepEqual(api.calls, ["PRRT_answered", "PRRT_outdated"]); +}); + +test("a PR the owner did not open is skipped with nothing resolved", async () => { + for (const author of [{ id: STRANGER, type: "User" }, { id: APP, type: "Bot" }, { id: OWNER, type: "Bot" }]) { + const api = fakeApi({ author, made: [make([[CLAUDE, REVIEW], [APP, VALID]])] }); + assert.deepEqual(await run(api), { skipReason: "not-applicable", resolved: [] }, JSON.stringify(author)); + assert.deepEqual(api.calls, []); + } +}); + +test("a thread that changed since it was read is left open; a failed resolve fails the job after the rest", async () => { + const changed = fakeApi({ made: [make([[CLAUDE, REVIEW], [APP, VALID]], { id: "PRRT_x" })], changed: new Set(["PRRT_x"]) }); + assert.deepEqual(await run(changed), { skipReason: "", resolved: [] }); + assert.deepEqual(changed.calls, []); + const failing = fakeApi({ + made: [make([[CLAUDE, REVIEW], [APP, VALID]], { id: "PRRT_f" }), make([[CLAUDE, REVIEW], [APP, VALID]], { id: "PRRT_g" })], + failing: new Set(["PRRT_f"]), + }); + await assert.rejects(run(failing), /could not resolve PRRT_f/); + assert.deepEqual(failing.calls, ["PRRT_f", "PRRT_g"]); +}); + +test("a comment edited between the reads is judged again: rated CRITICAL or edited by a person, the thread stays open", async () => { + const made = () => [make([[CLAUDE, REVIEW], [APP, VALID]], { id: "PRRT_e" }), make([[CLAUDE, REVIEW], [APP, VALID]], { id: "PRRT_kept" })]; + for (const edit of [[CRITICAL_REVIEW, CLAUDE], [REVIEW, OWNER]]) { + const api = fakeApi({ made: made(), edits: { PRRT_e: [edit] } }); + assert.deepEqual(await run(api), { skipReason: "", resolved: ["PRRT_kept"] }, JSON.stringify(edit)); + assert.deepEqual(api.calls, ["PRRT_kept"]); + } + // The bot's own edit that keeps the finding below CRITICAL still resolves. + const own = fakeApi({ made: made(), edits: { PRRT_e: [["This drops the error, as noted.", CLAUDE]] } }); + assert.deepEqual((await run(own)).resolved, ["PRRT_e", "PRRT_kept"]); +}); diff --git a/.github/workflows/pr-require-checks.yml b/.github/workflows/pr-require-checks.yml index 8f5f489803f..3fda83caceb 100644 --- a/.github/workflows/pr-require-checks.yml +++ b/.github/workflows/pr-require-checks.yml @@ -6753,7 +6753,7 @@ jobs: held_paths() { # The trust-gate paths, as bash patterns (`*` crosses `/`). # BEGIN hold-floor hold_patterns: generated by scripts/sync-hold-floor.py - hold_patterns=('.github/standards/trusted-actors/*' 'CODEOWNERS' '*/CODEOWNERS' 'docs/conventions/pr-pipeline.yaml' '.gitmodules' '*/.gitmodules' 'plugins/source-control/skills/babysit-prs/scripts/babysit_risk.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_check_inputs.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_checks.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_classify.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_dependabot.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_feedback.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_gh.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_merge_policy.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_postmerge.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_repo_config.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_review_evidence.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_review_trigger.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_run_cache.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_state.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_tracker_binding.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_util.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_verify_evidence.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_whole_review.py' '.claude/*' '*/.claude/*' 'docs/conventions/source-control.yaml' '.claude-lane/*' '*/.claude-lane/*' '.mcp.json' '*/.mcp.json' 'CLAUDE.md' '*/CLAUDE.md' 'CLAUDE.local.md' '*/CLAUDE.local.md' 'AGENTS.md' '*/AGENTS.md' 'REVIEW.md' '.github/actions/check-trusted-trigger/*' '.github/actions/resolve-config/*' '.github/actions/check-kill-switch/*' '.github/actions/request-lane-token/*' '.github/actions/log-api-budget/rate-guard.mjs' '.github/actions/log-api-budget/rate-limits.json' '.github/scripts/pr-merge/merge-holds.mjs' '.github/scripts/pr-merge/enable-auto-merge.mjs' '.github/scripts/pr-merge/superseded.mjs' '.github/scripts/pr-merge/clear-escalations.mjs' '.github/scripts/pr-merge/carry-run-cache.mjs' '.github/scripts/pr-merge/gate-check.mjs' '.github/scripts/pr-merge/head-ref-map.mjs' '.github/scripts/pr-merge/lane-records.mjs' '.github/scripts/pr-merge/last-report.mjs' '.github/scripts/pr-merge/rate-stop.bash' '.github/scripts/pr-merge/retry-call.bash' '.github/scripts/pr-merge/refire-refused.mjs' '.github/scripts/pr-merge/rotation.mjs' '.github/scripts/pr-merge/sweep-summary.mjs' '.github/scripts/pr-merge/who.mjs' '.github/scripts/pr-merge/post-unarmed-status.mjs' '.github/scripts/pr-merge/verify-answers.mjs' '.github/scripts/pr-merge/listing-carry.mjs' '.github/actions/report-check-run/posting-run.mjs' 'scripts/sync-hold-floor.py' '.github/scripts/pr-unblock/address-feedback.mjs' '.github/scripts/pr-unblock/escalation-routes.mjs' '.github/scripts/pr-unblock/escalation-routes.json' '.github/scripts/pr-unblock/gate-source.mjs' '.github/scripts/pr-unblock/thread-scope.mjs' '.github/scripts/pr-unblock/retry-fetch.mjs' '.github/scripts/work-resolve-issue/report-outcome.mjs' '.github/scripts/work-resolve-issue/limits.mjs' '.github/scripts/work-resolve-issue/retry-fetch.mjs' '.github/scripts/pr-unblock/report-escalation.sh' '.github/scripts/pr-unblock/adjudicate-feedback.mjs' '.github/scripts/pr-unblock/publish-feedback.mjs' '.github/scripts/pr-unblock/request-budget.mjs' '.github/scripts/pr-verify/resolve-pr.mjs' '.github/scripts/pr-verify/write-verdict.mjs' '.github/scripts/intake-triage/admit.mjs' '.github/actions/select-trusted-text/select-trusted-text.mjs' 'lib/publish-gate.mjs' '.github/recurring-schedule.json' 'plugins/work-items/scripts/charge-words.json' '.github/scripts/pr-unblock/dependency-classify.mjs' '.github/scripts/apply-patch/apply-patch.mjs' '.github/scripts/apply-patch/predicates.mjs' '.github/scripts/apply-patch/protected-paths.mjs' '.github/scripts/apply-patch/retry-fetch.mjs' '.github/scripts/pr-unblock/fix-ci-context.mjs' '.github/scripts/pr-unblock/feedback-dispatch.mjs' '.github/scripts/pr-unblock/verify-notes.mjs' '.github/scripts/pr-unblock/earlier-fail-notes.mjs' '.github/scripts/pr-unblock/limit-stop.mjs' '.github/scripts/lib/lane-outcome.mjs' '.github/scripts/pr-unblock/dispatch-record.mjs' '.github/scripts/pr-unblock/verify-cap.mjs' '.github/scripts/maintenance-reconcile-lost-runs/infra-failure.mjs' '.github/scripts/work-resolve-issue/stop-routes.json' '.github/scripts/work-resolve-issue/stop-routes.mjs' '.github/scripts/work-resolve-issue/known-names.json' '.github/scripts/lib/stop-reason.mjs' '.github/scripts/lib/admission-comment.mjs' '.github/scripts/lib/done-evidence.mjs' '.github/scripts/pr-review-main/*' 'docs/conventions/instruction-exception-register/*' 'docs/conventions/untrusted-content/*' 'docs/standards/*' 'docs/review/*' '.github/workflows/dependabot-fragments.yml' '.github/workflows/eval-browser-dispatch.yml' '.github/workflows/intake-reenter.yml' '.github/workflows/intake-triage.yml' '.github/workflows/maintenance-audit-lane-runs.yml' '.github/workflows/maintenance-reconcile-lost-runs.yml' '.github/workflows/maintenance-tidy-code-upkeep.yml' '.github/workflows/post-merge-demotion-alert.yml' '.github/workflows/post-merge-test-mutation.yml' '.github/workflows/pr-feedback-dispatch.yml' '.github/workflows/pr-merge.yml' '.github/workflows/pr-refine.yml' '.github/workflows/pr-require-checks.yml' '.github/workflows/pr-review-approved.yml' '.github/workflows/pr-review-comment-relay.yml' '.github/workflows/pr-review-hosted.yml' '.github/workflows/pr-review-main.yml' '.github/workflows/pr-review-security-hosted.yml' '.github/workflows/pr-run-activity-read.yml' '.github/workflows/pr-run-activity-write.yml' '.github/workflows/pr-unblock-dispatch.yml' '.github/workflows/pr-unblock.yml' '.github/workflows/pr-verify.yml' '.github/workflows/release-publish-plugins.yml' '.github/workflows/upstream-apply-changelog.yml' '.github/workflows/work-resolve-issue.yml' '.github/workflows/work-unblock.yml') + hold_patterns=('.github/standards/trusted-actors/*' 'CODEOWNERS' '*/CODEOWNERS' 'docs/conventions/pr-pipeline.yaml' '.gitmodules' '*/.gitmodules' 'plugins/source-control/skills/babysit-prs/scripts/babysit_risk.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_check_inputs.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_checks.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_classify.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_dependabot.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_feedback.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_gh.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_merge_policy.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_postmerge.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_repo_config.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_review_evidence.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_review_trigger.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_run_cache.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_state.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_tracker_binding.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_util.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_verify_evidence.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_whole_review.py' '.claude/*' '*/.claude/*' 'docs/conventions/source-control.yaml' '.claude-lane/*' '*/.claude-lane/*' '.mcp.json' '*/.mcp.json' 'CLAUDE.md' '*/CLAUDE.md' 'CLAUDE.local.md' '*/CLAUDE.local.md' 'AGENTS.md' '*/AGENTS.md' 'REVIEW.md' '.github/actions/check-trusted-trigger/*' '.github/actions/resolve-config/*' '.github/actions/check-kill-switch/*' '.github/actions/request-lane-token/*' '.github/actions/log-api-budget/rate-guard.mjs' '.github/actions/log-api-budget/rate-limits.json' '.github/scripts/pr-merge/merge-holds.mjs' '.github/scripts/pr-merge/enable-auto-merge.mjs' '.github/scripts/pr-merge/superseded.mjs' '.github/scripts/pr-merge/clear-escalations.mjs' '.github/scripts/pr-merge/carry-run-cache.mjs' '.github/scripts/pr-merge/gate-check.mjs' '.github/scripts/pr-merge/head-ref-map.mjs' '.github/scripts/pr-merge/lane-records.mjs' '.github/scripts/pr-merge/last-report.mjs' '.github/scripts/pr-merge/rate-stop.bash' '.github/scripts/pr-merge/retry-call.bash' '.github/scripts/pr-merge/refire-refused.mjs' '.github/scripts/pr-merge/rotation.mjs' '.github/scripts/pr-merge/sweep-summary.mjs' '.github/scripts/pr-merge/who.mjs' '.github/scripts/pr-merge/post-unarmed-status.mjs' '.github/scripts/pr-merge/verify-answers.mjs' '.github/scripts/pr-merge/listing-carry.mjs' '.github/actions/report-check-run/posting-run.mjs' 'scripts/sync-hold-floor.py' '.github/scripts/pr-unblock/address-feedback.mjs' '.github/scripts/pr-unblock/escalation-routes.mjs' '.github/scripts/pr-unblock/escalation-routes.json' '.github/scripts/pr-unblock/gate-source.mjs' '.github/scripts/pr-unblock/thread-scope.mjs' '.github/scripts/pr-unblock/retry-fetch.mjs' '.github/scripts/work-resolve-issue/report-outcome.mjs' '.github/scripts/work-resolve-issue/limits.mjs' '.github/scripts/work-resolve-issue/retry-fetch.mjs' '.github/scripts/pr-unblock/report-escalation.sh' '.github/scripts/pr-unblock/adjudicate-feedback.mjs' '.github/scripts/pr-unblock/publish-feedback.mjs' '.github/scripts/pr-unblock/resolve-threads.mjs' '.github/scripts/pr-unblock/request-budget.mjs' '.github/scripts/pr-verify/resolve-pr.mjs' '.github/scripts/pr-verify/write-verdict.mjs' '.github/scripts/intake-triage/admit.mjs' '.github/actions/select-trusted-text/select-trusted-text.mjs' 'lib/publish-gate.mjs' '.github/recurring-schedule.json' 'plugins/work-items/scripts/charge-words.json' '.github/scripts/pr-unblock/dependency-classify.mjs' '.github/scripts/apply-patch/apply-patch.mjs' '.github/scripts/apply-patch/predicates.mjs' '.github/scripts/apply-patch/protected-paths.mjs' '.github/scripts/apply-patch/retry-fetch.mjs' '.github/scripts/pr-unblock/fix-ci-context.mjs' '.github/scripts/pr-unblock/feedback-dispatch.mjs' '.github/scripts/pr-unblock/verify-notes.mjs' '.github/scripts/pr-unblock/earlier-fail-notes.mjs' '.github/scripts/pr-unblock/limit-stop.mjs' '.github/scripts/lib/lane-outcome.mjs' '.github/scripts/pr-unblock/dispatch-record.mjs' '.github/scripts/pr-unblock/verify-cap.mjs' '.github/scripts/maintenance-reconcile-lost-runs/infra-failure.mjs' '.github/scripts/work-resolve-issue/stop-routes.json' '.github/scripts/work-resolve-issue/stop-routes.mjs' '.github/scripts/work-resolve-issue/known-names.json' '.github/scripts/lib/stop-reason.mjs' '.github/scripts/lib/admission-comment.mjs' '.github/scripts/lib/done-evidence.mjs' '.github/scripts/pr-review-main/*' 'docs/conventions/instruction-exception-register/*' 'docs/conventions/untrusted-content/*' 'docs/standards/*' 'docs/review/*' '.github/workflows/dependabot-fragments.yml' '.github/workflows/eval-browser-dispatch.yml' '.github/workflows/intake-reenter.yml' '.github/workflows/intake-triage.yml' '.github/workflows/maintenance-audit-lane-runs.yml' '.github/workflows/maintenance-reconcile-lost-runs.yml' '.github/workflows/maintenance-tidy-code-upkeep.yml' '.github/workflows/post-merge-demotion-alert.yml' '.github/workflows/post-merge-test-mutation.yml' '.github/workflows/pr-feedback-dispatch.yml' '.github/workflows/pr-merge.yml' '.github/workflows/pr-refine.yml' '.github/workflows/pr-require-checks.yml' '.github/workflows/pr-review-approved.yml' '.github/workflows/pr-review-comment-relay.yml' '.github/workflows/pr-review-hosted.yml' '.github/workflows/pr-review-main.yml' '.github/workflows/pr-review-security-hosted.yml' '.github/workflows/pr-run-activity-read.yml' '.github/workflows/pr-run-activity-write.yml' '.github/workflows/pr-unblock-dispatch.yml' '.github/workflows/pr-unblock.yml' '.github/workflows/pr-verify.yml' '.github/workflows/release-publish-plugins.yml' '.github/workflows/upstream-apply-changelog.yml' '.github/workflows/work-resolve-issue.yml' '.github/workflows/work-unblock.yml') # merge.diff-check.path-holds is false: only Dependabot stays held. [ "$AUTHOR_ID" = 49699333 ] || hold_patterns=() # END hold-floor hold_patterns diff --git a/.github/workflows/pr-review-hosted.yml b/.github/workflows/pr-review-hosted.yml index 601d31eacd7..f3f34bc91c2 100644 --- a/.github/workflows/pr-review-hosted.yml +++ b/.github/workflows/pr-review-hosted.yml @@ -325,6 +325,7 @@ jobs: .github/scripts/pr-unblock/report-escalation.sh .github/scripts/pr-unblock/adjudicate-feedback.mjs .github/scripts/pr-unblock/publish-feedback.mjs + .github/scripts/pr-unblock/resolve-threads.mjs .github/scripts/pr-unblock/request-budget.mjs .github/scripts/pr-verify/resolve-pr.mjs .github/scripts/pr-verify/write-verdict.mjs diff --git a/.github/workflows/pr-review-security-hosted.yml b/.github/workflows/pr-review-security-hosted.yml index 38e917d1135..1bd6ce4e994 100644 --- a/.github/workflows/pr-review-security-hosted.yml +++ b/.github/workflows/pr-review-security-hosted.yml @@ -325,6 +325,7 @@ jobs: .github/scripts/pr-unblock/report-escalation.sh .github/scripts/pr-unblock/adjudicate-feedback.mjs .github/scripts/pr-unblock/publish-feedback.mjs + .github/scripts/pr-unblock/resolve-threads.mjs .github/scripts/pr-unblock/request-budget.mjs .github/scripts/pr-verify/resolve-pr.mjs .github/scripts/pr-verify/write-verdict.mjs diff --git a/.github/workflows/pr-run-activity-write.yml b/.github/workflows/pr-run-activity-write.yml index 7870086dbc7..e4bd94df933 100644 --- a/.github/workflows/pr-run-activity-write.yml +++ b/.github/workflows/pr-run-activity-write.yml @@ -1294,12 +1294,13 @@ jobs: merge-multiple: true # Also issue mode: report-outcome trusts a merge: human label only from - # a listed human or the lanes App. + # a listed human or the lanes App; and a checkout: base script, which + # reads the owners and the reviewer bot from it. - name: Copy the trusted actors for the reply script id: reply-actors if: >- steps.lane-token.outputs.token != '' - && (inputs.review-replies || inputs.issue-number != '') + && (inputs.review-replies || inputs.issue-number != '' || steps.config.outputs.checkout == 'base') && steps.config.outputs.kind == 'script' env: COPY: ${{ runner.temp }}/base-script/.trusted-actors.json @@ -1513,8 +1514,10 @@ jobs: ${{ steps.lane-token.outcome != 'skipped' && inputs.issue-number == '' && github.event_name == 'pull_request' && 'lane-token:compare' || '' }} ${{ steps.lane-token.outcome == 'success' && inputs.issue-number == '' && 'promote:pulls promote:issues/labels' || '' }} + # A `checkout: base` script activity skips it: the job holds only the + # trusted base checkout, and .base stays in place. - name: Check out the PR head - if: steps.lane-token.outputs.token != '' + if: steps.lane-token.outputs.token != '' && steps.config.outputs.checkout != 'base' uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: ref: >- @@ -1533,7 +1536,7 @@ jobs: # The activity judges exactly the commit the check is posted on. - name: Assert the head is the gated SHA - if: steps.lane-token.outputs.token != '' + if: steps.lane-token.outputs.token != '' && steps.config.outputs.checkout != 'base' env: HEAD_SHA: >- ${{ inputs.issue-number != '' @@ -2072,7 +2075,7 @@ jobs: # review-replies only; empty otherwise. APP_SENDER_ID: ${{ vars.AUTOMATION_LANES_APP_SENDER_ID }} FEEDBACK_REPLIES_DIR: ${{ inputs.review-replies && format('{0}/feedback-replies', runner.temp) || '' }} - TRUSTED_ACTORS: ${{ (inputs.review-replies || inputs.issue-number != '') && format('{0}/base-script/.trusted-actors.json', runner.temp) || '' }} + TRUSTED_ACTORS: ${{ (inputs.review-replies || inputs.issue-number != '' || steps.config.outputs.checkout == 'base') && format('{0}/base-script/.trusted-actors.json', runner.temp) || '' }} RUN_SHA256: ${{ inputs.review-run-sha256 }} REPLIES_SHA256: ${{ inputs.review-replies-sha256 }} REVIEW_PATCH_STATE: ${{ inputs.review-replies && inputs.review-patch-state || '' }} diff --git a/.github/workflows/pr-unblock.yml b/.github/workflows/pr-unblock.yml index 2545e01058c..1f7a5f634c7 100644 --- a/.github/workflows/pr-unblock.yml +++ b/.github/workflows/pr-unblock.yml @@ -2147,6 +2147,51 @@ jobs: pushed-commit: >- ${{ inputs.dispatcher-run-id != '' && needs.apply-feedback-patch.outputs.pushed-commit || '' }} + # The resolve job, a script with no model and no PR head checkout + # (checkout: base): on a PR a trusted person opened it resolves the threads + # publish-feedback answered or GitHub reports outdated, with no CRITICAL + # finding and no person's comment (resolve-threads.mjs). It reads the + # threads from GitHub, never from another job's outputs, and is the one job + # whose lane token holds the contents: write that resolving there needs; + # publish-feedback keeps publish-artifact. It escalates nothing. + resolve-feedback-threads: + needs: [address-feedback-select, apply-feedback-patch, publish-feedback] + # On an acted round, and on the reply script's own stops, after which + # other threads may still hold its answers: an escalation (lane-stopped, + # awaiting-human), finding-stands or no-model-turn. Never on any other + # skip: a kill-switch stop ends publish-feedback green with no token, and + # nothing may run after it. + if: >- + ${{ !cancelled() && needs.publish-feedback.result == 'success' + && (needs.publish-feedback.outputs.skip-reason == '' + || needs.publish-feedback.outputs.skip-reason == 'lane-stopped' + || needs.publish-feedback.outputs.skip-reason == 'awaiting-human' + || needs.publish-feedback.outputs.skip-reason == 'finding-stands' + || needs.publish-feedback.outputs.skip-reason == 'no-model-turn') }} + permissions: + contents: read + pull-requests: read + checks: write + issues: write + id-token: write + concurrency: + group: pr-pipeline-write-${{ inputs.pr-number }} + queue: max + uses: ./.github/workflows/pr-run-activity-write.yml # zizmor: ignore[self-repository] + with: + role: pr-unblock + lane: pr-unblock + activity: resolve-feedback-threads + pr-number: ${{ inputs.pr-number }} + dispatcher-run-id: ${{ inputs.dispatcher-run-id }} + trusted-base: default-tip + # As publish-feedback: the gate accepts the review of the head the + # select job read while the PR is at it or at the push job's commit. + expected-upstream-head: >- + ${{ inputs.dispatcher-run-id != '' && needs.address-feedback-select.outputs.head-sha || '' }} + pushed-commit: >- + ${{ inputs.dispatcher-run-id != '' && needs.apply-feedback-patch.outputs.pushed-commit || '' }} + # review-dependency is six jobs, so no model ever holds a write token. This # one runs only base code with a read-only token and checks out no PR head. # Before any model turn it reads the PR head, resolves the slot (draft and diff --git a/docs/conventions/pr-pipeline.yaml b/docs/conventions/pr-pipeline.yaml index 82e89a1fe3e..d4fe9090a73 100644 --- a/docs/conventions/pr-pipeline.yaml +++ b/docs/conventions/pr-pipeline.yaml @@ -157,6 +157,25 @@ activities: # Reads the replies the model wrote. reads-untrusted: true inputs: [pr, head-sha] + resolve-feedback-threads: + # On a pull request a trusted person opened, resolves the review-bot + # threads publish-feedback answered (or GitHub reports outdated) and could + # not resolve, with no CRITICAL or person-decided security finding and no + # comment by anyone but the review bots and the lanes App. A script, no + # model; it reads the threads from GitHub itself (resolve-threads.mjs). + script: .github/scripts/pr-unblock/resolve-threads.mjs + # Resolving a thread on a PR the lanes App did not open needs a writer, + # contents: write; it pushes nothing. + effect: mutate-branch + # No PR head checkout: the job holding the write holds only the trusted base. + checkout: base + gating: advisory + # Reads review comments, which quote the diff. + reads-untrusted: true + inputs: [pr] + applies-when: + labels-absent: [do-not-merge, "security: outside-actor"] + draft: [false] review-dependency: # Reviews one Dependabot pull request the classifier sorts `review` (a # major bump, an action publisher outside the allowlist, a group holding @@ -749,6 +768,9 @@ lanes: needs: [address-feedback] - activity: publish-feedback needs: [address-feedback, apply-feedback-patch] + # Runs after publish-feedback, in its own job with its own token. + - activity: resolve-feedback-threads + needs: [publish-feedback] # Started by pr-unblock-dispatch.yml for review-tier Dependabot PRs. - activity: review-dependency # Run after review-dependency, each in its own job with its own token: @@ -1498,6 +1520,8 @@ merge: class: enforcement - glob: ".github/scripts/pr-unblock/publish-feedback.mjs" class: enforcement + - glob: ".github/scripts/pr-unblock/resolve-threads.mjs" + class: enforcement - glob: ".github/scripts/pr-unblock/request-budget.mjs" class: enforcement - glob: ".github/scripts/pr-verify/resolve-pr.mjs" diff --git a/docs/conventions/pr-pipeline/README.md b/docs/conventions/pr-pipeline/README.md index aa9e93862fe..4590330e980 100644 --- a/docs/conventions/pr-pipeline/README.md +++ b/docs/conventions/pr-pipeline/README.md @@ -359,15 +359,23 @@ serves an adopted owner PR, posting its replies as the lanes App: 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. + (`publish-artifact`, no writer) posts each answer and leaves the thread open, logging it. The + `resolve-feedback-threads` job after it, a script with no model and `checkout: base` (no PR head + checkout), holds `mutate-branch` (`contents: write`, which makes it a writer) and resolves, on a + PR a person on the trusted-actors list opened, each thread whose comments are all a review bot's + or the lanes App's, whose finding no reviewer comment rates CRITICAL and an adjudicator could + judge (`adjudicable`: no person-decided security finding), and that is answered (the lane's + VALID reply or a `finding-answered` adjudication is its last comment) or outdated with no lane + reply that left it open (`resolve-threads.mjs`). It reads the threads from GitHub, never from + another job, judges each again on its re-read comments just before resolving, and escalates + nothing. It runs after an acted round and after the reply script's own stops (an escalation, + `finding-stands`, `no-model-turn`), never after a kill-switch or gate skip. A CRITICAL finding + and any thread a person wrote or joined stay 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 push is the power fix-ci already holds on an adopted PR, under the same idle rule; the one +grant the lane gains is `resolve-feedback-threads`' `mutate-branch`, used only to resolve. 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 diff --git a/docs/conventions/pr-pipeline/pr-pipeline.schema.json b/docs/conventions/pr-pipeline/pr-pipeline.schema.json index e449feb45cd..74c99150509 100644 --- a/docs/conventions/pr-pipeline/pr-pipeline.schema.json +++ b/docs/conventions/pr-pipeline/pr-pipeline.schema.json @@ -600,7 +600,8 @@ "web-access": ["skill"], "tidy": ["skill"], "applied-by": ["skill"], - "permission-mode": ["skill"] + "permission-mode": ["skill"], + "checkout": ["script"] }, "dependentSchemas": { "applied-by": { @@ -783,6 +784,11 @@ "pattern": "^(?!/)(?!(.*/)?\\.\\.(/|$))[^:]+$", "description": "Repository-relative path of a scripted step with no model in it." }, + "checkout": { + "enum": ["head", "base"], + "default": "head", + "description": "What the write runner checks out for a script activity: head (the gated PR head, replacing the base checkout) or base (no head checkout, so the job holding the lane token holds only the trusted base checkout, credentials not persisted). The read runner ignores it." + }, "effect": { "enum": [ "read", diff --git a/docs/conventions/pr-pipeline/pr-run-activity.md b/docs/conventions/pr-pipeline/pr-run-activity.md index 5a1e70aeca6..59967c9a0c5 100644 --- a/docs/conventions/pr-pipeline/pr-run-activity.md +++ b/docs/conventions/pr-pipeline/pr-run-activity.md @@ -645,7 +645,10 @@ root-level script path fails red rather than copying the whole base tree. Its en Exit 0 is success, non-zero is failure, and a reason file whose value is not a skip reason is a failure. A script that gets `GH_TOKEN` must not execute code from the PR head. The head checkout -persists no credentials, so a `read` script cannot push. The write file also exports its +persists no credentials, so a `read` script cannot push. A leaf that sets `checkout: base` +(resolve-config's `checkout` output) skips the head checkout and its SHA assert: the job keeps only +the trusted base checkout at `.base`, credentials not persisted, and gets `TRUSTED_ACTORS` as a +`review-replies` script does. The write file also exports its `escalation-reason` input as `ESCALATION_REASON` (empty unless a caller passes one), which the `report-escalation` leaf reads, and `TRACKER_CONFIG`, the base's `.work-item-tracker.json` copied to `$RUNNER_TEMP/base-script` before the head checkout and checked against its recorded SHA-256 diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_risk.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_risk.py index 3e6dc3b989c..6df15945856 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_risk.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_risk.py @@ -294,7 +294,7 @@ def test_floor_paths_are_high_whatever_the_switch(self): # generated copies hold; here the count of each class. self.assertEqual( Counter(c for _, c in FLOOR.classes), - {"trust": 19, "gate": 55, "enforcement": 54}, + {"trust": 19, "gate": 55, "enforcement": 55}, ) self.assertEqual( FLOOR.high, @@ -2298,7 +2298,7 @@ def _score(self, path, security): return score(_input(files=(_file(path),), lanes=lanes)) def test_a_clean_bound_security_review_clears_each_enforcement_path(self): - self.assertEqual(len(FLOOR.review), 54) + self.assertEqual(len(FLOOR.review), 55) for glob in FLOOR.review: with self.subTest(glob): result = self._score(glob, (_lane("claude-security-review"),)) diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_floor_paths.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_floor_paths.py index d3cfd5039fc..c20daf62159 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_floor_paths.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_floor_paths.py @@ -90,21 +90,21 @@ class TheEffectiveFloorDidNotMove(unittest.TestCase): change updates these pins in the pull request that makes it.""" def test_each_glob_keeps_its_class(self): - self.assertEqual(len(FLOOR.classes), 128) + self.assertEqual(len(FLOOR.classes), 129) self.assertEqual( digest(f"{g}\t{c}" for g, c in FLOOR.classes), - "dab09f9bfd53bd9b45b2c9e9927c74cb1cc2a2fe741402921f92ac9fe118b7d9", + "1af9bf4415ebcfe0874999ee0a961a398ee4005c9a27f6f7c1e5189c3fcee65c", ) def test_each_clearing_route_keeps_its_globs(self): pins = { FLOOR.high: "d02f625bb46fccfe784bcf606d5451ab1fb25db9ea21bffeaaefaaf1d5fab0c1", - FLOOR.review: "f993e2dd67a84a648ad19b70c3217050fb40a840ecf2b5b84f0478d405404ab0", + FLOOR.review: "5264e103bf8c812a6ca19de2ca6b605d5f02bd7f92ab7af15189b688eb89662e", FLOOR.author_review: "10af3daeaaeaaa01c77eafd040b831be3f147fdf3fa850b636b87a8e64094399", } self.assertEqual( (len(FLOOR.high), len(FLOOR.review), len(FLOOR.author_review)), - (74, 54, 15), + (74, 55, 15), ) for globs, pin in pins.items(): self.assertEqual(digest(globs), pin) @@ -170,7 +170,7 @@ def test_it_holds_only_what_every_consumer_shares(self): self.assertEqual(len(risk.FLOOR_CLASSES), 30) def test_every_other_row_sits_in_the_config(self): - self.assertEqual(len(ROWS), 128 - 30) + self.assertEqual(len(ROWS), 129 - 30) self.assertFalse(table(ROWS) & set(risk.FLOOR_CLASSES)) def test_the_config_every_consumer_reads_is_held_by_a_built_in_row(self): diff --git a/scripts/write-runner-never-rules.test.sh b/scripts/write-runner-never-rules.test.sh index d56654702c2..100ec340754 100755 --- a/scripts/write-runner-never-rules.test.sh +++ b/scripts/write-runner-never-rules.test.sh @@ -83,7 +83,7 @@ const keyed = steps.slice(mint + 1) .filter((s) => /steps\.head-recheck\.outputs\.proceed/.test(String(s.if ?? "")) && !/steps\.lane-token\.outputs\.token != ''/.test(String(s.if ?? ""))) .map((s) => s.name); say(keyed.length === 0, `no step after the mint keys on the head re-read instead of the token (${keyed.join(", ") || "none"})`); -say(String(steps[checkout]?.if ?? "") === "steps.lane-token.outputs.token != ''", "the head checkout runs only with a minted token"); +say(String(steps[checkout]?.if ?? "") === "steps.lane-token.outputs.token != '' && steps.config.outputs.checkout != 'base'", "the head checkout runs only with a minted token, and never for a checkout: base script"); const mintWith = byId("lane-token").with ?? {}; say(mintWith["pr-workflow-change-ok"] === "${{ inputs.issue-number == '' && github.event_name == 'pull_request' }}" && mintWith["github-token"] === "${{ github.token }}" && mintWith["head-sha"] === "${{ steps.gate.outputs.head-sha }}",