diff --git a/.changes/pr-pipeline/fix-needs-human-h38-routing-b0d69f43.md b/.changes/pr-pipeline/fix-needs-human-h38-routing-b0d69f43.md new file mode 100644 index 0000000000..6ca42007be --- /dev/null +++ b/.changes/pr-pipeline/fix-needs-human-h38-routing-b0d69f43.md @@ -0,0 +1,8 @@ +--- +bump: minor +--- + +### Changed + +- `enable-auto-merge.mjs` adds the human-gated label only for a cause a person must clear (owner rule 2026-10-26): a refusal the gate marks `needsHuman` (an untrusted spec, the repeated-FAIL cap, a pull request too old to verify), the trust hold, a risk hold the gate marks `reviewsPerson`, a hold path, a cost hold, a listing past 300 files (a fourth page that is not empty; no later read completes it), or the lane's own hold line. A changed spec and a changed-file page that did not read get the comment alone, and the next scheduled runs judge them again. `merge-holds.pathHoldKind` reports a path hold's kind, so the routing reads no prose. +- A refusal the lanes clear that has stood at the head for `merge.risk-escalation-minutes`, timed from the first lane refusal noted there (the comment's `pr-merge-unarmed-since` line), shows as a failing `pr-merge/unarmed` commit status on the head; an arm, the refusal becoming a person's, or a member's veto or a person turning auto-merge off standing in for it posts a passing one over it; a run that read nothing new (no time left, unread hold paths, a verdict that did not read) leaves the status alone. The new `unarmed-status` job in `pr-merge.yml` posts them with the job's own token (`contents: read`, `statuses: write`, no lane token) and trusts only a status the GitHub Actions bot posted; the status is informational, not `ci-status`. diff --git a/.changes/source-control/fix-needs-human-h38-routing-b0d69f43.md b/.changes/source-control/fix-needs-human-h38-routing-b0d69f43.md new file mode 100644 index 0000000000..dfea6d01ee --- /dev/null +++ b/.changes/source-control/fix-needs-human-h38-routing-b0d69f43.md @@ -0,0 +1,14 @@ +--- +bump: minor +--- + +### Changed + +- The merge gate pulls a person in only for a reason a person must clear (owner rule 2026-10-26): `babysit_risk.person_reasons` (`PERSON_CODES`: a CRITICAL security finding, a trusted-actor widening, a cost hold, every high path, and `files-capped`, a listing past the files API's 3000-file cap) is the one list the gate and `enable-auto-merge.mjs` read. A risk hold the lanes clear, such as review evidence not bound to the head or a listing a read did not finish, keeps its reasons comment and no longer gets the human-gated label however long it stands. +- A blocking security finding a bound review left open holds as `security-finding` after the lane unbinds (a new head, a newer marker, a retargeted base): the binder carries the count from the newest head of the pull request the security lane's evidence binds at, from the runs, comments and threads it already read. The gate's head-file reader sends no read again after a contents or comparison read fails, a rate limit included; a path it only cannot prove removed (`HeadFileUnproven`) stops nothing else. +- `babysit_verify_evidence.NEEDS_HUMAN` drops `verify:spec-changed`, which the lanes judge again; the terminal repeated-FAIL cap, `verify:too-old` and `verify:untrusted-spec` stay a person's. +- `evidence --lane-reviews` reports `reviewsPerson`, whether only a person clears the risk hold it names. + +### Fixed + +- The merge gate re-runs a rate-limited AI review only when the run's attempt 1 concluded `failure`, the condition under which the review binding takes attempt 2, so a re-run never spends the one allowed attempt on a hold. diff --git a/.github/scripts/pr-merge/enable-auto-merge.mjs b/.github/scripts/pr-merge/enable-auto-merge.mjs index ea8bb0bb65..42c16903d2 100755 --- a/.github/scripts/pr-merge/enable-auto-merge.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.mjs @@ -93,16 +93,26 @@ // edited in place while it is a lane's, posted anew when it becomes a // person's, and left alone while the reason and head are unchanged // (refusalWrites). The -// comment is the record; only a cause a person must clear (owner direction -// 2026-10-24: untrusted provenance, a security finding, a trusted-actor -// widening or a real charge, see verifyClears; a hold path, a configured -// denied path or the floor, which only a trusted approval lifts; and the -// lane's own hold line, owner decision Q-HOLDLINE) also gets the human-gated -// label, put back while the comment stands unless a person removed it -// since. clear-escalations takes the label off once the head moves, and -// this run labels the new head again only if its cause still holds. Holds a -// person already set (labels, their own body hold, a veto), the room and -// failed reads write nothing. +// comment is the record; only a cause a person must clear (owner rule +// 2026-10-26: an intent or design change or a CRITICAL security issue, with +// cost and trust holds and a hold no lane can clear kept: untrusted +// provenance or an untrusted spec, the repeated-FAIL cap, a pull request too +// old to verify, a security finding, a high path, a trusted-actor widening, +// a real charge or a listing too long to check, see verifyClears; a hold path, a configured denied path +// or the floor, which only a trusted approval lifts; and the lane's own hold +// line, owner decision Q-HOLDLINE) also gets the human-gated label, put back +// while the comment stands unless a person removed it since. Every other +// refusal, a listing read that failed included, is a lane's: the next +// scheduled runs judge it again, and once a lane's refusal has stood at the +// head for merge.risk-escalation-minutes (timed from the first one there, +// LANE_SINCE) the run hands a +// failing `pr-merge/unarmed` status for the head to the unarmed-status job +// (unarmedStatus, post-unarmed-status.mjs), and a passing one when it arms +// or the refusal becomes a person's. +// clear-escalations takes the label off at the latest once the head moves, +// and this run labels the new head again only if its cause still holds. +// Holds a person already set (labels, their own body hold, a veto), the room +// and a discussion that did not read write nothing. // Arming passes the head just read as expectedHeadOid, so a push in between // fails the arm rather than arming a head nobody read. Before it arms, the run // posts the gate's pin comment for that head (pinBody) unless the lanes App's @@ -132,6 +142,9 @@ // POSTMERGE_RESULT this run's babysit_postmerge.py report, for lane-room // RUNG_CAP vars.CLAUDE_LANES_MERGE_RUNG_CAP (empty is no cap; // unset holds the lane off, as the gate reads it) +// RISK_ESCALATION_MINUTES merge.risk-escalation-minutes (empty is +// RISK_ESCALATION_MINUTES) +// GITHUB_OUTPUT receives UNARMED_OUTPUT for the unarmed-status job // BABYSIT_VERIFY_*, BABYSIT_RUN_CACHE the reader's Actions token (the // job's GITHUB_TOKEN), saved listing, head-ref map and // run cache, as the gate's @@ -145,7 +158,7 @@ import { createGitHub, errorLabel, GitHubError } from "../../actions/check-trust import { rateLimitLatch } from "../../actions/log-api-budget/rate-guard.mjs"; import { humanGatedLabelFrom } from "../pr-unblock/address-feedback.mjs"; import { readHoldPaths } from "../work-resolve-issue/report-outcome.mjs"; -import { BODY_HOLD, costDecision, costHold, humansOf, LANE_HOLD, pathHold, personWrote, vetoers } from "./merge-holds.mjs"; +import { BODY_HOLD, costDecision, costHold, humansOf, LANE_HOLD, pathHoldKind, personWrote, vetoers } from "./merge-holds.mjs"; export { BODY_HOLD, HUMAN_VETO } from "./merge-holds.mjs"; @@ -408,23 +421,33 @@ function trustedHumans(env) { } } +// The head's changed paths, null when a page did not read, and `capped` +// when the listing has a page past MAX_FILE_PAGES, which no later read +// completes. Exactly MAX_FILE_PAGES full pages read one page more to tell. async function changedFiles(github, slug, number) { const paths = []; - for (let page = 1; page <= MAX_FILE_PAGES; page += 1) { + for (let page = 1; page <= MAX_FILE_PAGES + 1; page += 1) { const rows = await github("GET", `/repos/${slug}/pulls/${number}/files?per_page=100&page=${page}`); if (!Array.isArray(rows)) { - return null; + return { paths: null, capped: false }; + } + if (page > MAX_FILE_PAGES) { + return rows.length === 0 ? { paths, capped: false } : { paths: null, capped: true }; } for (const row of rows) { paths.push(String(row?.filename), ...(row?.previous_filename ? [String(row.previous_filename)] : [])); } if (rows.length < 100) { - return paths; + return { paths, capped: false }; } } - return null; + return { paths: null, capped: true }; } +// A listing past MAX_FILE_PAGES: no lane can ever judge its hold and cost +// paths, so a person clears it. +export const FILES_CAPPED_REASON = `it changes more than ${MAX_FILE_PAGES * 100} files, so its hold and cost paths cannot all be checked`; + // How verifyHold opens the reason for each answer that refuses: the issue // link, the verifier verdict and the risk score. export const READER_HOLDS = Object.freeze({ @@ -458,24 +481,21 @@ export function verifyHold(evidence, head) { return evidence.reviews === null ? null : "its risk score could not be read"; } -// The verifier refusals and risk reason codes a person must clear (owner -// direction 2026-10-24): untrusted provenance, the repeated-FAIL round cap -// (a kept cap no lane clears), a security finding (a CRITICAL one, or a -// path that could alias another, a symlink, submodule or mode change: -// `high-path:**`), a trusted-actor widening and a cost hold. A risk reason -// opens the reasons string or follows "; " -// (babysit_verify_evidence.lane_risk_refusal). -export const PERSON_REFUSALS = Object.freeze(["verify:untrusted-spec", "verify:repeated-fail"]); +// The trust hold's reasons (babysit_merge_policy.trust_refusal), each a +// person's: an issue label naming untrusted work, and labels that cannot be +// read whole, more than the 100 the read lists, which no later read changes. export const TRUST_REFUSALS = Object.freeze(["untrusted-provenance", "untrusted-provenance-removed-by-non-human", "issue-labels-unreadable"]); -const PERSON_RISK = /(?:^|; )(?:security-finding|trust-gate:[a-z-]+|cost-review-required|high-path:\*\*):/; -// Who clears the refusal verifyHold reads in `evidence` at `head`: `person` -// for a PERSON_REFUSALS refusal, an issue link the trust hold refuses -// (TRUST_REFUSALS, babysit_merge_policy.trust_refusal's reasons) or a -// risk hold carrying a PERSON_RISK reason, `lane` for every other refusal -// (any other issue link the lane does not take, a FAIL, a changed spec, another risk reason), which gets the comment -// and no label, and null for an unread answer, which the next run reads -// again. +// Who clears the refusal verifyHold reads in `evidence` at `head` (owner rule +// 2026-10-26: a person only for an intent or design change, or a CRITICAL +// security issue; cost and trust holds and a hold no lane can clear stay a +// person's): `person` +// for an issue link the trust hold refuses (TRUST_REFUSALS), a verifier +// refusal the gate marks `needsHuman` (babysit_verify_evidence NEEDS_HUMAN) +// or a risk hold it marks `reviewsPerson` (babysit_risk.person_reasons), +// `lane` for every other refusal, which gets the comment and no label, and +// null for an unread answer, which the next run reads again. An answer +// without its flag is a person's, as any doubt holds. export function verifyClears(evidence, head) { if (evidence === null || typeof evidence !== "object" || evidence.head !== head) { return null; @@ -484,10 +504,10 @@ export function verifyClears(evidence, head) { return TRUST_REFUSALS.includes(evidence.ineligible) ? "person" : "lane"; } if (typeof evidence.refusal === "string") { - return PERSON_REFUSALS.includes(evidence.refusal) ? "person" : "lane"; + return evidence.needsHuman === false ? "lane" : "person"; } if (evidence.ineligible === null && typeof evidence.reviews === "string") { - return PERSON_RISK.test(evidence.reviews) ? "person" : "lane"; + return evidence.reviewsPerson === false ? "lane" : "person"; } return null; } @@ -515,7 +535,19 @@ export function ejectionHold(evidence) { // refusal renders the same body. export const REFUSAL_MARKER = ""; export const ATTEND_QUEUE_MARKER = ""; -export function refusalBody(person, reason, head, label) { +// The lane comment's last line: when a lane's refusal was first noted at +// `head`, kept while the head is unchanged so a reason whose detail moves +// every run does not restart unarmedStatus's timer. +export const LANE_SINCE = /^$/; +const laneSince = (head, at) => ``; +// When the lane comment `body` says its refusal at `head` began; else +// undefined. +function sinceAt(body, head) { + const match = LANE_SINCE.exec(String(body ?? "").split("\n").at(-1).replace(/\r$/, "")); + return match && match[1] === head ? match[2] : undefined; +} + +export function refusalBody(person, reason, head, label, since = null) { const who = person ? `A person must clear this reason, so the pr-merge lane added the \`${label}\` label: fix the reason, then remove the label, and the next scheduled run judges it again.` : "The lanes clear this reason on their own; the next scheduled run judges it again."; @@ -527,6 +559,7 @@ export function refusalBody(person, reason, head, label) { "```text", String(reason).replaceAll(/[\u0000-\u0008\u000b-\u001f\u007f]/g, "?").replaceAll("```", "'''"), "```", + ...(person || since === null ? [] : [laneSince(head, since)]), ].join("\n"); } @@ -562,7 +595,7 @@ function priorRefusal(comments, bot, marker = REFUSAL_MARKER) { // when this write made the comment a person's, so a label a person removed // stays off until the reason or the head changes. Returns what it wrote, or // null when the comment already says this. -export function refusalWrites(pull, comments, { clears, reason, comment }, { bot, label }) { +export function refusalWrites(pull, comments, { clears, reason, comment }, { bot, label, now = Date.now() }) { // The gate's own marked comment (the queue-ejection one): edited in place, // with no label, as babysit_merge.upsert_marker_comment writes it. if (typeof comment === "string") { @@ -570,7 +603,8 @@ export function refusalWrites(pull, comments, { clears, reason, comment }, { bot return marked?.body === comment ? null : { body: comment, commentId: marked?.databaseId ?? null, label: null }; } const prior = priorRefusal(comments, bot); - const lane = refusalBody(false, reason, pull.headRefOid, label); + const since = sinceAt(prior?.body, pull.headRefOid) ?? new Date(now).toISOString(); + const lane = refusalBody(false, reason, pull.headRefOid, label, since); const person = refusalBody(true, reason, pull.headRefOid, label); const body = clears === "person" ? person : lane; // A person's comment already standing while the label is off (a pull @@ -585,6 +619,35 @@ export function refusalWrites(pull, comments, { clears, reason, comment }, { bot return body === person ? { body, commentId: null, label } : { body, commentId: prior?.databaseId ?? null, label: null }; } +// The commit status context the status job posts (pr-merge.yml +// unarmed-status), and the step output enable-auto-merge hands it. +export const UNARMED_CONTEXT = "pr-merge/unarmed"; +export const UNARMED_OUTPUT = "unarmed-statuses"; +// GitHub's limit on a status description. +const STATUS_DESCRIPTION_CHARS = 140; +// merge.risk-escalation-minutes' default (babysit_merge_policy.RISK_ESCALATION_MINUTES). +export const RISK_ESCALATION_MINUTES = 60; + +// The failing `pr-merge/unarmed` status for a refusal the lanes clear +// (`clears` lane) whose comment has stood unchanged at the head for +// `minutes`: the next scheduled runs judge it again until then, and past it +// the hold shows as a failing status on the head, never a label (owner rule +// 2026-10-26). Null while it has stood less, and for any other refusal. +export function unarmedStatus(pull, comments, { clears, reason, comment }, { bot, now, minutes }) { + if (clears !== "lane") { + return null; + } + // The gate's own marked comment is timed from its last change; the lane + // comment from the first lane refusal at the head (LANE_SINCE). + const marked = typeof comment === "string" ? priorRefusal(comments, bot, comment.split("\n", 1)[0]) : null; + const began = + typeof comment === "string" ? (marked?.body === comment ? (marked.updatedAt ?? marked.createdAt) : undefined) : sinceAt(priorRefusal(comments, bot)?.body, pull.headRefOid); + if (!(now - Date.parse(String(began)) >= minutes * 60_000)) { + return null; + } + return { sha: pull.headRefOid, state: "failure", description: Array.from(String(reason).replaceAll(/\s+/g, " ")).slice(0, STATUS_DESCRIPTION_CHARS).join("") }; +} + // The newest label removals on a pull request, for refusalWrites' lift check. export const UNLABELED_QUERY = `query($owner: String!, $name: String!, $number: Int!) { repository(owner: $owner, name: $name) { @@ -796,20 +859,27 @@ export function decide(pull, { bot, humanGated, holdPaths = null }, detail) { if (holdPaths === null) { return { action: armed ? "disarm" : "none", reason: "the hold paths could not be read" }; } - const pathHeld = pathHold(detail.files, holdPaths); + if (detail.filesCapped === true) { + return { action: armed ? "disarm" : "none", reason: FILES_CAPPED_REASON, clears: "person" }; + } + // A hold path a person clears; a listing or check that could not run is + // a lane's, which the next run reads again. + const pathHeld = pathHoldKind(detail.files, holdPaths); if (pathHeld !== null) { - return { action: armed ? "disarm" : "none", reason: pathHeld, clears: "person" }; + return { action: armed ? "disarm" : "none", reason: pathHeld.reason, clears: pathHeld.kind === "hold-path" ? "person" : "lane" }; } const costHeld = costHold(detail.files, detail.costDecision); if (costHeld !== null) { return { action: armed ? "disarm" : "none", reason: costHeld, clears: "person" }; } + // A hold a person set, read whole: `withdraw` marks a decided outcome, + // whose run may withdraw a lane's failing status. const vetoed = discussionHold(detail); if (vetoed !== null) { - return { action: armed ? "disarm" : "none", reason: vetoed }; + return { action: armed ? "disarm" : "none", reason: vetoed, withdraw: true }; } if (!armed && detail.timelineItems?.nodes?.at(-1)?.actor?.__typename === "User") { - return { action: "none", reason: "a person turned its auto-merge off" }; + return { action: "none", reason: "a person turned its auto-merge off", withdraw: true }; } if (!("verify" in detail)) { return { action: "verify", reason: "its verifier verdict is not read yet" }; @@ -846,7 +916,7 @@ async function listPulls(github, owner, name) { throw new Error(`more than ${MAX_PAGES * 100} open pull requests into main`); } -export async function main({ env = process.env, github, verify, room, sleep = (ms) => new Promise((resolve) => setTimeout(resolve, ms)), random, latch } = {}) { +export async function main({ env = process.env, github, verify, room, sleep = (ms) => new Promise((resolve) => setTimeout(resolve, ms)), random, latch, now = Date.now } = {}) { try { const repo = REPO.exec(env.GITHUB_REPOSITORY ?? ""); if (!repo) { @@ -861,6 +931,9 @@ export async function main({ env = process.env, github, verify, room, sleep = (m throw new Error("LANES_BOT_LOGIN is not an App bot login"); } const write = env.REPORT_ONLY === "false"; + const minutes = /^[0-9]+$/.test(env.RISK_ESCALATION_MINUTES ?? "") ? Number(env.RISK_ESCALATION_MINUTES) : RISK_ESCALATION_MINUTES; + // The pr-merge/unarmed statuses for the status job (UNARMED_OUTPUT). + const statuses = []; const humanGated = humanGatedLabelFrom(env.TRACKER_CONFIG || ".base/.work-item-tracker.json"); const holdPaths = readHoldPaths(env.RESOLVED_CONFIG ?? ""); // The newest answer's rate limit and body, for an arm GitHub refuses. @@ -897,7 +970,7 @@ export async function main({ env = process.env, github, verify, room, sleep = (m console.log(`enable-auto-merge: lane room: ${escapeCommand(JSON.stringify(lane))}`); } // After the room read, so the verdict reads keep their whole budget. - const deadline = Date.now() + VERIFY_BUDGET_MS; + const deadline = now() + VERIFY_BUDGET_MS; // Merges in flight: the gate's count, plus each armed lanes App pull // request the queue has not taken yet, which GitHub enqueues on its own. let inFlight = lane === null ? 0 : lane.count + pulls.filter((pull) => lanes(pull) && pull.autoMergeRequest != null && !lane.queued.includes(pull.number)).length; @@ -940,7 +1013,16 @@ export async function main({ env = process.env, github, verify, room, sleep = (m if (!reads || stopped !== null || refusal.clears === undefined) { return; } - const writes = refusalWrites(pull, detail?.comments?.nodes, refusal, { bot: slug, label: humanGated }); + const status = unarmedStatus(pull, detail?.comments?.nodes, refusal, { bot: slug, now: now(), minutes }); + if (status !== null && write) { + statuses.push(status); + } + const writes = refusalWrites(pull, detail?.comments?.nodes, refusal, { bot: slug, label: humanGated, now: now() }); + // A lane's failing status withdrawn once the refusal is a person's: + // the status job writes it only over a status already there. + if (write && writes?.label && writes.commentId === null && writes.body !== null) { + statuses.push({ sha: pull.headRefOid, state: "success", description: `a person clears this hold: ${humanGated}` }); + } if (writes === null) { return; } @@ -984,6 +1066,7 @@ export async function main({ env = process.env, github, verify, room, sleep = (m let { action, reason } = decide(pull, { bot: slug, humanGated, holdPaths }); let clears; let comment; + let withdraw; let detail = null; let decided = false; // Checked before the reads, so a held lane spends none. @@ -1003,23 +1086,23 @@ export async function main({ env = process.env, github, verify, room, sleep = (m detail = (await github("POST", "/graphql", { query: DETAIL_QUERY, variables }))?.data?.repository?.pullRequest ?? null; if (detail !== null) { detail = await wholeDiscussion(github, variables, detail); - const files = await changedFiles(github, `${repo[1]}/${repo[2]}`, pull.number); + const { paths: files, capped } = await changedFiles(github, `${repo[1]}/${repo[2]}`, pull.number); const decision = costHold(files, null) ? await costDecision(github, repo[0], pull.number, pull.body, trustedHumans(env), pull.headRefOid) : null; - detail = { ...detail, files, costDecision: decision }; + detail = { ...detail, files, filesCapped: capped, costDecision: decision }; } } catch (error) { // A read that fails decides nothing and writes nothing. detail = null; console.log(`enable-auto-merge: #${pull.number} discussion did not read: ${describe(error)}`); } - ({ action, reason, clears, comment } = decide(pull, { bot: slug, humanGated, holdPaths }, detail)); + ({ action, reason, clears, comment, withdraw } = decide(pull, { bot: slug, humanGated, holdPaths }, detail)); } if (action === "verify" && !reads) { ({ action, reason } = pull.autoMergeRequest != null ? { action: "none", reason: "already armed; the next scheduled run checks its verifier verdict" } : { action: "none", reason: "only a scheduled or hand-started run arms it, on the gate's verifier verdict" }); - } else if (action === "verify" && Date.now() > deadline) { + } else if (action === "verify" && now() > deadline) { ({ action, reason } = { action: "none", reason: "no time left in this run to read its verifier verdict" }); } else if (action === "verify") { let evidence = null; @@ -1045,7 +1128,7 @@ export async function main({ env = process.env, github, verify, room, sleep = (m console.log(`::warning title=enable-auto-merge::#${pull.number} its merge-queue ejections could not be read: ${escapeCommand(evidence.queue.readError)}`); } if (!decided) { - ({ action, reason, clears, comment } = decide(pull, { bot: slug, humanGated, holdPaths }, { ...detail, verify: evidence })); + ({ action, reason, clears, comment, withdraw } = decide(pull, { bot: slug, humanGated, holdPaths }, { ...detail, verify: evidence })); } } // A pull request armed earlier in this run counts toward the cap. @@ -1060,6 +1143,14 @@ export async function main({ env = process.env, github, verify, room, sleep = (m if (reason !== "not the lanes App's") { console.log(`enable-auto-merge: #${pull.number} left: ${escapeCommand(reason)}`); } + // A lane's refusal noted at this head that a decided hold a person + // set now stands in for (a veto, auto-merge turned off): its failing + // status is withdrawn, as an arm withdraws it. An outcome that read + // nothing (no time left, unread hold paths, a verdict that did not + // read) leaves the status as it is. + if (reads && write && withdraw === true && sinceAt(priorRefusal(detail?.comments?.nodes, slug)?.body, pull.headRefOid) !== undefined) { + statuses.push({ sha: pull.headRefOid, state: "success", description: "its lanes refusal no longer stands" }); + } await note(pull, detail, { clears, reason, comment }); continue; } @@ -1092,6 +1183,7 @@ export async function main({ env = process.env, github, verify, room, sleep = (m arms += 1; await github("POST", "/graphql", { query: ARM_MUTATION, variables: { id: pull.id, head: pull.headRefOid } }); console.log(`enable-auto-merge: #${pull.number} armed at ${pull.headRefOid}`); + statuses.push({ sha: pull.headRefOid, state: "success", description: "armed for native auto-merge" }); } else { await github("POST", "/graphql", { query: DISARM_MUTATION, variables: { id: pull.id } }); if (roomHeld.has(pull.number)) { @@ -1162,6 +1254,14 @@ export async function main({ env = process.env, github, verify, room, sleep = (m for (const [level, text] of lines) { console.log(`::${level} title=enable-auto-merge::${text}`); } + if (statuses.length > 0 && env.GITHUB_OUTPUT) { + try { + appendFileSync(env.GITHUB_OUTPUT, `${UNARMED_OUTPUT}=${JSON.stringify(statuses)}\n`); + } catch (error) { + failed += 1; + console.log(`::error title=enable-auto-merge::the ${UNARMED_CONTEXT} statuses were not handed on: ${describe(error)}`); + } + } if (stopped !== null) { failed += 1; console.log(`${GUARD_ERROR}enable-auto-merge: ${escapeCommand(stopped.message)}; no pull request is read, noted or armed after it this run, and each armed lanes App pull request is disarmed`); diff --git a/.github/scripts/pr-merge/enable-auto-merge.test.mjs b/.github/scripts/pr-merge/enable-auto-merge.test.mjs index 2e247a36df..240bcb1612 100644 --- a/.github/scripts/pr-merge/enable-auto-merge.test.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.test.mjs @@ -39,8 +39,11 @@ import { ghCalls, LANE_ROOM_TIMEOUT_MS, VERIFY_TIMEOUT_MS, + VERIFY_BUDGET_MS, readRoom, TRUST_REFUSALS, + FILES_CAPPED_REASON, + UNARMED_OUTPUT, verifyClears, verifyHold, } from "./enable-auto-merge.mjs"; @@ -236,11 +239,16 @@ async function run(pulls, env = {}, options = {}, lane = ROOM) { const { github, writes, verify, verified, calls, notes, pins } = world(pulls, options); const lines = []; const slept = []; + const output = join(mkdtempSync(join(tmpdir(), "enable-auto-merge-")), "output"); + writeFileSync(output, ""); const log = console.log; console.log = (line) => lines.push(line); try { - const code = await main({ env: { ...ENV, ...env }, github, verify, room: room(lane), sleep: async (ms) => slept.push(ms), random: options.random, now: options.now }); - return { code, writes: writes(), lines, verified, slept, calls, notes, pins }; + const code = await main({ env: { ...ENV, GITHUB_OUTPUT: output, ...env }, github, verify, room: room(lane), sleep: async (ms) => slept.push(ms), random: options.random, now: options.now ?? (() => Date.parse(NOW)) }); + // The `pr-merge/unarmed` statuses the run hands the status job. + const line = readFileSync(output, "utf8").split("\n").find((text) => text.startsWith(`${UNARMED_OUTPUT}=`)); + const statuses = line === undefined ? [] : JSON.parse(line.slice(UNARMED_OUTPUT.length + 1)); + return { code, writes: writes(), lines, verified, slept, calls, notes, pins, statuses }; } finally { console.log = log; } @@ -248,7 +256,9 @@ async function run(pulls, env = {}, options = {}, lane = ROOM) { // The refusal note's two bodies, written out by hand: the gate's reasons // marker on its own line, the head named on the next, the reason fenced. -const laneNote = (reason) => +// The run's clock, and so the time a lane comment first noted its head. +const NOW = "2026-10-23T10:00:00.000Z"; +const laneNote = (reason, since = NOW) => [ "", `Unattended merge: native auto-merge leaves this pull request unarmed at head \`${HEAD}\`. The lanes clear this reason on their own; the next scheduled run judges it again.`, @@ -256,6 +266,7 @@ const laneNote = (reason) => "```text", reason, "```", + ``, ].join("\n"); const personNote = (reason) => [ @@ -336,13 +347,19 @@ test("a floor path no configured glob names is commented on and labeled human-ga ); }); -test("an untrusted spec and the repeated-FAIL round cap are a person's verifier refusals; the rest are a lane's", async () => { +// Owner rule 2026-10-26: a person only for intent, design, a CRITICAL +// security issue or a hold no lane can clear. The gate's `needsHuman` +// (babysit_verify_evidence NEEDS_HUMAN: an untrusted spec, the terminal +// repeated-FAIL cap, a pull request too old to verify) decides, so a changed +// spec is a lane's. +test("only a verifier refusal the gate marks needsHuman is a person's; a changed spec is a lane's", async () => { const refused = (refusal, needsHuman) => ({ refusal, needsHuman, head: HEAD, ineligible: null, reviews: null }); const { notes } = await run([ pull(1, { verdict: refused("verify:untrusted-spec", true) }), - pull(2, { verdict: refused("verify:spec-changed", true) }), + pull(2, { verdict: refused("verify:spec-changed", false) }), pull(3, { verdict: refused("verify:repeated-fail", true) }), - pull(4, { verdict: { ...MISSING, refusal: "verify:ineligible", ineligible: "it closes 2 issues" } }), + pull(4, { verdict: refused("verify:too-old", true) }), + pull(5, { verdict: { ...MISSING, refusal: "verify:ineligible", ineligible: "it closes 2 issues" } }), ]); const refusing = (code) => `pr-merge's gate refuses its verifier verdict (${code})`; assert.deepEqual(notes, [ @@ -351,16 +368,107 @@ test("an untrusted spec and the repeated-FAIL round cap are a person's verifier { method: "POST", path: `${ISSUES}/2/comments`, body: { body: laneNote(refusing("verify:spec-changed")) } }, { method: "POST", path: `${ISSUES}/3/comments`, body: { body: personNote(refusing("verify:repeated-fail")) } }, { method: "POST", path: `${ISSUES}/3/labels`, body: { labels: ["needs-human"] } }, - { method: "POST", path: `${ISSUES}/4/comments`, body: { body: laneNote("pr-merge's lane does not take its issue link (it closes 2 issues)") } }, + { method: "POST", path: `${ISSUES}/4/comments`, body: { body: personNote(refusing("verify:too-old")) } }, + { method: "POST", path: `${ISSUES}/4/labels`, body: { labels: ["needs-human"] } }, + { method: "POST", path: `${ISSUES}/5/comments`, body: { body: laneNote("pr-merge's lane does not take its issue link (it closes 2 issues)") } }, ]); }); +test("verifyClears reads the gate's needsHuman and reviewsPerson, not the refusal text", () => { + const at = (fields) => verifyClears({ head: HEAD, ineligible: null, reviews: null, refusal: null, ...fields }, HEAD); + assert.equal(at({ refusal: "verify:repeated-fail", needsHuman: true }), "person"); + assert.equal(at({ refusal: "verify:untrusted-spec", needsHuman: false }), "lane"); + assert.equal(at({ reviews: "security-finding: 1 open", reviewsPerson: false }), "lane"); + assert.equal(at({ reviews: "evidence-unbound:claude-review: not bound", reviewsPerson: true }), "person"); + // An answer without the flag holds for a person, as an unknown does. + assert.equal(at({ refusal: "verify:fail" }), "person"); + assert.equal(at({ reviews: "evidence-unbound:claude-review: not bound" }), "person"); +}); + test("a refusal a lane clears is commented on, with no label", async () => { const { code, notes } = await run([pull(1, { verdict: MISSING })]); assert.equal(code, 0); assert.deepEqual(notes, [{ method: "POST", path: `${ISSUES}/1/comments`, body: { body: laneNote(MISSING_REASON) } }]); }); +// Owner rule 2026-10-26: a read that failed is never a person's; the next +// run reads it again, and nothing that failed to read arms. +// A listing past the page cap never reads whole, so its hold and cost paths +// are never checked: a person's. A page that did not read is read again. +test("a listing past the page cap is a person's refusal; one that did not read is a lane's; neither arms", async () => { + // Exactly 300 files is not past the cap: one more page reads empty, and + // it arms. + const exactly = Array.from({ length: 300 }, (_, index) => ({ filename: `docs/${index}.md` })); + const full = await run([pull(1, { files: exactly })]); + assert.deepEqual(full.writes, [{ mutation: "arm", id: "PR_1", head: HEAD }]); + assert.deepEqual(full.notes, []); + const files = Array.from({ length: 301 }, (_, index) => ({ filename: `docs/${index}.md` })); + const { code, notes, writes } = await run([pull(1, { files })]); + assert.equal(code, 0); + assert.deepEqual(writes, []); + assert.deepEqual(notes, [ + { method: "POST", path: `${ISSUES}/1/comments`, body: { body: personNote(FILES_CAPPED_REASON) } }, + { method: "POST", path: `${ISSUES}/1/labels`, body: { labels: ["needs-human"] } }, + ]); + const options = { bot: LANES.login, humanGated: "needs-human", holdPaths: [] }; + assert.deepEqual(decide(pull(1), options, { ...detail(), files: null, filesCapped: false, verify: PASSED }), { + action: "none", + reason: "its changed files could not all be read", + clears: "lane", + }); +}); + +test("a refusal a lane clears gets a failing pr-merge/unarmed status once it has stood merge.risk-escalation-minutes at the head, and an arm or a person's refusal a passing one", async () => { + // Timed from the first lane refusal at the head, so a reason whose detail + // changes every run does not restart it. + const standing = (since, said = MISSING_REASON) => pull(1, { verdict: MISSING, discussion: { comments: [lanesSaid(laneNote(said, since))] } }); + const now = () => Date.parse("2026-10-23T11:00:00Z"); + const failing = [{ sha: HEAD, state: "failure", description: MISSING_REASON }]; + assert.deepEqual((await run([standing("2026-10-23T10:00:00.000Z")], {}, { now })).statuses, failing); + assert.deepEqual((await run([standing("2026-10-23T10:00:00.000Z", "unreadable: RuntimeError: a detail that moves")], {}, { now })).statuses, failing); + assert.deepEqual((await run([standing("2026-10-23T10:01:00.000Z")], {}, { now })).statuses, []); + assert.deepEqual((await run([standing("2026-10-23T10:31:00.000Z")], { RISK_ESCALATION_MINUTES: "29" }, { now })).statuses, failing); + // A since line for another head starts the time again. + const moved = pull(1, { verdict: MISSING, discussion: { comments: [lanesSaid(laneNote(MISSING_REASON, "2026-01-01T00:00:00.000Z").replace(`head=${HEAD}`, `head=${"c".repeat(40)}`))] } }); + assert.deepEqual((await run([moved], {}, { now })).statuses, []); + // A lane's refusal that becomes a person's withdraws the failing status. + const escalated = pull(2, { files: COST_FILES, discussion: { comments: [lanesSaid(laneNote(MISSING_REASON, "2026-01-01T00:00:00.000Z"))] } }); + assert.deepEqual((await run([escalated], {}, { now })).statuses, [{ sha: HEAD, state: "success", description: "a person clears this hold: needs-human" }]); + const armed = await run([pull(3)], {}, { now }); + assert.deepEqual(armed.writes, [{ mutation: "arm", id: "PR_3", head: HEAD }]); + assert.deepEqual(armed.statuses, [{ sha: HEAD, state: "success", description: "armed for native auto-merge" }]); + // A lane's refusal that cleared while something else leaves it unarmed + // (here a member's veto) withdraws its failing status too. + const vetoed = pull(4, { + discussion: { comments: [lanesSaid(laneNote(MISSING_REASON, "2026-01-01T00:00:00.000Z")), said("kyle-sexton", "do not merge", "2026-10-23T10:30:00Z")] }, + }); + const held = await run([vetoed], {}, { now }); + assert.deepEqual(held.writes, []); + assert.deepEqual(held.statuses, [{ sha: HEAD, state: "success", description: "its lanes refusal no longer stands" }]); + // Report-only hands the status job nothing. + assert.deepEqual((await run([standing("2026-10-23T10:00:00.000Z")], { REPORT_ONLY: "true" }, { now })).statuses, []); +}); + +// A run that read nothing new about a standing lane refusal leaves its +// failing status alone: no success that the next reading run turns back. +test("an unread outcome withdraws no failing pr-merge/unarmed status", async () => { + const stood = (overrides = {}) => pull(1, { discussion: { comments: [lanesSaid(laneNote(MISSING_REASON, "2026-01-01T00:00:00.000Z"))] }, ...overrides }); + const at = Date.parse("2026-10-23T11:00:00Z"); + // No time left: the clock passes the verify budget after the room read. + let calls = 0; + const late = () => (calls++ === 0 ? at : at + VERIFY_BUDGET_MS + 1); + const cases = [ + ["no time left to read the verdict", await run([stood()], {}, { now: late })], + ["the hold paths did not read", await run([stood()], { RESOLVED_CONFIG: "/nonexistent/resolved-config.json" }, { now: () => at })], + ["the verdict read threw", await run([stood({ verdict: new Error("boom") })], {}, { now: () => at })], + ]; + for (const [name, result] of cases) { + assert.deepEqual(result.writes, [], name); + assert.deepEqual(result.statuses, [], name); + } +}); + + const unlabeled = (at, typename = "User", name = "needs-human") => ({ createdAt: at, actor: { __typename: typename, login: "someone" }, label: { name } }); test("an unchanged refusal writes nothing, and a label a person removed stays off", async () => { @@ -398,21 +506,40 @@ test("a changed refusal edits the one comment instead of posting another", async assert.deepEqual(notes, [{ method: "PATCH", path: `${ISSUES}/comments/77`, body: { body: laneNote(MISSING_REASON) } }]); }); -test("a risk hold is a person's only for a security finding, an aliasing path, a trusted-actor widening or a cost hold", async () => { - const risk = (reviews) => ({ refusal: null, needsHuman: false, head: HEAD, ineligible: null, reviews }); +test("a risk hold is a person's only for a security finding, a high path, a trusted-actor widening or a cost hold", async () => { + // reviewsPerson as babysit_risk.person_reasons answers for each code, + // read from the Python, the one list. + const codes = (reviews) => reviews.split("; ").map((part) => part.slice(0, part.indexOf(": "))); + const scripts = fileURLToPath(new URL("../../../plugins/source-control/skills/babysit-prs/scripts", import.meta.url)); + const answer = (reviews) => { + const python = spawnSync( + "python3", + ["-c", "import json,sys\nfrom babysit_risk import Reason, Risk, person_reasons\nprint(json.dumps(bool(person_reasons([Reason(c, Risk.HIGH, 'd') for c in json.load(sys.stdin)]))))"], + { cwd: scripts, input: JSON.stringify(codes(reviews)), encoding: "utf8" }, + ); + assert.equal(python.status, 0, python.stderr); + return JSON.parse(python.stdout); + }; + const risk = (reviews) => ({ refusal: null, needsHuman: false, head: HEAD, ineligible: null, reviews, reviewsPerson: answer(reviews) }); const holding = (reviews) => `pr-merge's gate holds it on its risk score (${reviews})`; - const lane = ["evidence-unbound:claude-review: no-marker", "manifest-review-required: plugins/a/.claude-plugin/plugin.json: hooks", "high-path:docs/**: docs/a.md"]; + const lane = ["evidence-unbound:claude-review: no-marker", "manifest-review-required: plugins/a/.claude-plugin/plugin.json: hooks", "unreadable: RuntimeError: boom"]; const person = [ "security-finding: 1 CRITICAL", "high-path:**: 'a' mode '120000' (symlink)", "files-incomplete: x; trust-gate:trusted-actors: entries added", "cost-review-required: infra/main.tf", + "high-path:docs/**: docs/a.md", ]; for (const reviews of lane) { - // A lane's stays a lane's however long its comment has stood. - const stood = pull(1, { verdict: risk(reviews), discussion: { comments: [lanesSaid(laneNote(holding(reviews)), "2026-01-01T00:00:00Z")] } }); - assert.deepEqual((await run([stood])).notes, [], reviews); - assert.deepEqual((await run([pull(1, { verdict: risk(reviews) })])).notes, [{ method: "POST", path: `${ISSUES}/1/comments`, body: { body: laneNote(holding(reviews)) } }], reviews); + // A lane's stays a lane's however long its comment has stood: past + // merge.risk-escalation-minutes it gets a failing status, never a label. + const stood = pull(1, { verdict: risk(reviews), discussion: { comments: [lanesSaid(laneNote(holding(reviews), "2026-01-01T00:00:00.000Z"))] } }); + const late = await run([stood]); + assert.deepEqual(late.notes, [], reviews); + assert.deepEqual(late.statuses, [{ sha: HEAD, state: "failure", description: holding(reviews).slice(0, 140) }], reviews); + const fresh = await run([pull(1, { verdict: risk(reviews) })]); + assert.deepEqual(fresh.notes, [{ method: "POST", path: `${ISSUES}/1/comments`, body: { body: laneNote(holding(reviews)) } }], reviews); + assert.deepEqual(fresh.statuses, [], reviews); } for (const reviews of person) { assert.deepEqual( @@ -466,7 +593,7 @@ test("a lane's refusal over a person's comment still edits it, with no label", ( { headRefOid: HEAD }, [lanesSaid(personNote(HOLD_PATH_REASON))], { clears: "lane", reason: MISSING_REASON }, - { bot: LANES.login, label: "needs-human" }, + { bot: LANES.login, label: "needs-human", now: Date.parse(NOW) }, ); assert.deepEqual(writes, { body: laneNote(MISSING_REASON), commentId: 77, label: null }); }); @@ -1030,7 +1157,8 @@ test("a pull request whose head now changes a hold path is not armed, or is disa test("a manifest change arms only when the gate's risk score allows it", () => { const options = { bot: LANES.login, humanGated: "needs-human", holdPaths: [] }; const files = ["docs/a.md", ".claude-plugin/marketplace.json", "plugins/a/.claude-plugin/plugin.json"]; - const review = { ...PASSED, reviews: "manifest-review-required: plugins/a/.claude-plugin/plugin.json: hooks" }; + // Owner Q-FLOOR (B): the security lane, not a person, reviews a manifest. + const review = { ...PASSED, reviews: "manifest-review-required: plugins/a/.claude-plugin/plugin.json: hooks", reviewsPerson: false }; assert.deepEqual(decide(pull(1), options, { ...detail(), files, verify: review }), { action: "none", reason: "pr-merge's gate holds it on its risk score (manifest-review-required: plugins/a/.claude-plugin/plugin.json: hooks)", diff --git a/.github/scripts/pr-merge/merge-holds.mjs b/.github/scripts/pr-merge/merge-holds.mjs index e7e6bb63f9..6991eab475 100755 --- a/.github/scripts/pr-merge/merge-holds.mjs +++ b/.github/scripts/pr-merge/merge-holds.mjs @@ -277,19 +277,23 @@ export const HOLD_PATH_FLOOR = Object.freeze([ // Why the head's changed `files` hold it, or null: one matching `holdPaths` // or HOLD_PATH_FLOOR. Null `holdPaths` or `files` (unreadable) holds it. -export function pathHold(files, holdPaths) { +export const pathHold = (files, holdPaths) => pathHoldKind(files, holdPaths)?.reason ?? null; + +// pathHold's answer with its kind: `hold-path` for a changed file a hold +// path matches, `unread` for a list that could not be read or checked. +export function pathHoldKind(files, holdPaths) { if (!Array.isArray(holdPaths)) { - return "the hold paths could not be read"; + return { kind: "unread", reason: "the hold paths could not be read" }; } if (!Array.isArray(files)) { - return "its changed files could not all be read"; + return { kind: "unread", reason: "its changed files could not all be read" }; } if (typeof posix.matchesGlob !== "function") { - return "this Node has no path.matchesGlob, so its hold paths cannot be checked"; + return { kind: "unread", reason: "this Node has no path.matchesGlob, so its hold paths cannot be checked" }; } const globs = [...holdPaths, ...HOLD_PATH_FLOOR]; const held = files.find((path) => globs.some((glob) => matchesHoldPath(path, glob))); - return held === undefined ? null : `it changes ${held}, a hold path`; + return held === undefined ? null : { kind: "hold-path", reason: `it changes ${held}, a hold path` }; } // Why the changed `files` hold it for the owner's cost decision, or null when diff --git a/.github/scripts/pr-merge/post-unarmed-status.mjs b/.github/scripts/pr-merge/post-unarmed-status.mjs new file mode 100755 index 0000000000..1b4107b7a0 --- /dev/null +++ b/.github/scripts/pr-merge/post-unarmed-status.mjs @@ -0,0 +1,108 @@ +#!/usr/bin/env node +// post-unarmed-status.mjs: posts the `pr-merge/unarmed` commit statuses that +// enable-auto-merge.mjs hands on (UNARMED_OUTPUT): `failure` on a lanes App +// pull request's head whose refusal the lanes clear has stood +// merge.risk-escalation-minutes unchanged, `success` on a head it armed. No +// model. +// +// The status shows a hold the lanes own on the pull request, in place of the +// human-gated label (owner rule 2026-10-26: a person only for an intent or +// design change or a CRITICAL security issue). It is informational: +// its context is not `ci-status`, the only check the main ruleset requires, +// so it neither holds nor passes a merge. Each head's newest status in the +// context is read first, trusted only when the GitHub Actions bot posted it +// (any token with statuses: write can post the context): one already in that +// state with that description gets no write, and a `success` is written only +// over a status already there, so an unchanged run writes nothing. +// +// Environment: +// GH_TOKEN the job's own token, with statuses: write +// GITHUB_REPOSITORY owner/repo +// STATUSES enable-auto-merge's UNARMED_OUTPUT, a JSON array +// GITHUB_RUN_ID, GITHUB_SERVER_URL, GITHUB_API_URL +import process from "node:process"; +import { pathToFileURL } from "node:url"; + +import { createGitHub } from "../../actions/check-trusted-trigger/check-trusted-trigger.mjs"; +import { UNARMED_CONTEXT } from "./enable-auto-merge.mjs"; + +const REPO = /^[A-Za-z0-9_.-]+\/[A-Za-z0-9_.-]+$/; +const SHA = /^[0-9a-f]{40}$/; +const STATES = new Set(["failure", "success"]); +// GitHub's limit on a status description. +const DESCRIPTION_CHARS = 140; +// Statuses read and written in one run at most (judgment: more open lane +// pull requests than this is a run to look at). +export const MAX_STATUSES = 100; +// The GitHub Actions bot's user id, which every GITHUB_TOKEN posts as +// (fix-ci-dispatch.mjs ACTIONS_BOT_ID). +const ACTIONS_BOT_ID = 41898282; + +const escapeCommand = (text) => String(text).replaceAll(/[\r\n%]/g, " "); + +// The statuses `text` hands on, well-formed only, one per head (its last). +export function parse(text) { + let list; + try { + list = JSON.parse(text); + } catch { + return []; + } + const byHead = new Map(); + for (const item of Array.isArray(list) ? list : []) { + if (SHA.test(String(item?.sha)) && STATES.has(item?.state) && typeof item.description === "string") { + byHead.set(item.sha, { sha: item.sha, state: item.state, description: Array.from(item.description).slice(0, DESCRIPTION_CHARS).join("") }); + } + } + return [...byHead.values()].slice(0, MAX_STATUSES); +} + +// Whether `status` changes what its head shows, given the head's newest +// status in the context (`current`, undefined for none). +export const changes = (status, current) => + current === undefined ? status.state === "failure" : current.state !== status.state || current.description !== status.description; + +export async function main({ env = process.env, github } = {}) { + try { + if (!REPO.test(env.GITHUB_REPOSITORY ?? "")) { + throw new Error("GITHUB_REPOSITORY is not owner/repo"); + } + const repo = env.GITHUB_REPOSITORY; + github ??= createGitHub({ token: env.GH_TOKEN, apiUrl: env.GITHUB_API_URL || undefined }); + const target = /^[1-9][0-9]*$/.test(env.GITHUB_RUN_ID ?? "") ? `${env.GITHUB_SERVER_URL || "https://github.com"}/${repo}/actions/runs/${env.GITHUB_RUN_ID}` : null; + let failed = 0; + let written = 0; + for (const status of parse(env.STATUSES ?? "")) { + try { + // Newest first, so the first in the context is the one shown. + const listed = await github("GET", `/repos/${repo}/commits/${status.sha}/statuses?per_page=100`); + const newest = (Array.isArray(listed) ? listed : []).find((item) => item?.context === UNARMED_CONTEXT); + // Another creator's status in the context is not this job's: never + // read it as already shown. + const current = newest?.creator?.id === ACTIONS_BOT_ID ? newest : undefined; + if (!changes(status, current)) { + continue; + } + await github("POST", `/repos/${repo}/statuses/${status.sha}`, { + state: status.state, + context: UNARMED_CONTEXT, + description: status.description, + ...(target ? { target_url: target } : {}), + }); + written += 1; + } catch (error) { + failed += 1; + console.log(`::error title=post-unarmed-status::${status.sha}: ${escapeCommand(error?.message ?? error)}`); + } + } + console.log(`post-unarmed-status: ${written} written, ${failed} failed`); + return failed === 0 ? 0 : 1; + } catch (error) { + console.log(`::error title=post-unarmed-status::${escapeCommand(error?.message ?? error)}`); + return 1; + } +} + +if (import.meta.url === pathToFileURL(process.argv[1] ?? "").href) { + process.exitCode = await main(); +} diff --git a/.github/scripts/pr-merge/post-unarmed-status.test.mjs b/.github/scripts/pr-merge/post-unarmed-status.test.mjs new file mode 100644 index 0000000000..620b441240 --- /dev/null +++ b/.github/scripts/pr-merge/post-unarmed-status.test.mjs @@ -0,0 +1,111 @@ +// Expected values come from the owner rule of 2026-10-26: a hold the lanes +// clear ends in a visible failing status, never the human-gated label, and a +// head that arms shows it passed. +import assert from "node:assert/strict"; +import { test } from "node:test"; + +import { UNARMED_CONTEXT } from "./enable-auto-merge.mjs"; +import { changes, MAX_STATUSES, main, parse } from "./post-unarmed-status.mjs"; + +const REPO = "melodic-software/claude-code-plugins"; +const A = "a".repeat(40); +const B = "b".repeat(40); +const ACTIONS = { id: 41898282, login: "github-actions[bot]" }; +const ENV = { GITHUB_REPOSITORY: REPO, GITHUB_RUN_ID: "9", GITHUB_SERVER_URL: "https://github.com" }; + +// `shown`: each head's statuses, newest first, as GitHub lists them. +async function run(statuses, shown = {}, { failRead = false } = {}) { + const posts = []; + const github = async (method, path, body) => { + const read = /^\/repos\/melodic-software\/claude-code-plugins\/commits\/([0-9a-f]{40})\/statuses\?per_page=100$/.exec(path); + if (method === "GET" && read) { + if (failRead) { + throw new Error("GET statuses failed with status 502"); + } + return shown[read[1]] ?? []; + } + assert.equal(method, "POST"); + posts.push({ path, body }); + return {}; + }; + const log = console.log; + console.log = () => {}; + try { + const code = await main({ env: { ...ENV, STATUSES: JSON.stringify(statuses) }, github }); + return { code, posts }; + } finally { + console.log = log; + } +} + +test("a failing status is posted on its head with the run as its target", async () => { + const { code, posts } = await run([{ sha: A, state: "failure", description: "pr-merge's gate refuses its verifier verdict (verify:missing)" }]); + assert.equal(code, 0); + assert.deepEqual(posts, [ + { + path: `/repos/${REPO}/statuses/${A}`, + body: { + state: "failure", + context: UNARMED_CONTEXT, + description: "pr-merge's gate refuses its verifier verdict (verify:missing)", + target_url: `https://github.com/${REPO}/actions/runs/9`, + }, + }, + ]); +}); + +test("an unchanged status writes nothing, and a success only replaces a status already shown", async () => { + const failing = { context: UNARMED_CONTEXT, state: "failure", description: "x", creator: ACTIONS }; + const { posts } = await run( + [ + { sha: A, state: "failure", description: "x" }, + { sha: B, state: "success", description: "armed for native auto-merge" }, + ], + { [A]: [failing] }, + ); + assert.deepEqual(posts, []); + const cleared = await run([{ sha: A, state: "success", description: "armed for native auto-merge" }], { [A]: [{ context: "other", state: "success", creator: ACTIONS }, failing] }); + assert.deepEqual( + cleared.posts.map(({ body }) => body.state), + ["success"], + ); +}); + +// Any token with statuses: write can post the context (fix-ci-dispatch.mjs +// reads the creator the same way). +test("a status in the context another creator posted is not read as this job's", async () => { + const forged = { context: UNARMED_CONTEXT, state: "failure", description: "x", creator: { id: 7, login: "someone" } }; + const { posts } = await run([{ sha: A, state: "failure", description: "x" }], { [A]: [forged] }); + assert.deepEqual( + posts.map(({ body }) => body.state), + ["failure"], + ); + const skipped = await run([{ sha: A, state: "success", description: "armed for native auto-merge" }], { [A]: [forged] }); + assert.deepEqual(skipped.posts, []); +}); + +test("only well-formed statuses are read, one per head and at most MAX_STATUSES", () => { + assert.deepEqual(parse("not json"), []); + assert.deepEqual(parse(JSON.stringify({ sha: A })), []); + assert.deepEqual( + parse( + JSON.stringify([ + { sha: "A".repeat(40), state: "failure", description: "x" }, + { sha: A, state: "pending", description: "x" }, + { sha: A, state: "failure", description: 7 }, + { sha: A, state: "failure", description: "first" }, + { sha: A, state: "success", description: "y".repeat(200) }, + ]), + ), + [{ sha: A, state: "success", description: "y".repeat(140) }], + ); + const many = Array.from({ length: MAX_STATUSES + 5 }, (_, index) => ({ sha: index.toString(16).padStart(40, "0"), state: "failure", description: "x" })); + assert.equal(parse(JSON.stringify(many)).length, MAX_STATUSES); + assert.equal(changes({ state: "success", description: "x" }, undefined), false); +}); + +test("a read that fails writes nothing for that head and fails the step", async () => { + const { code, posts } = await run([{ sha: A, state: "failure", description: "x" }], {}, { failRead: true }); + assert.equal(code, 1); + assert.deepEqual(posts, []); +}); diff --git a/.github/scripts/pr-merge/workflow.test.mjs b/.github/scripts/pr-merge/workflow.test.mjs index 20ce50910f..99cc1c4abb 100644 --- a/.github/scripts/pr-merge/workflow.test.mjs +++ b/.github/scripts/pr-merge/workflow.test.mjs @@ -268,7 +268,7 @@ test("the merge and enable-auto-merge jobs' kept-file readers share their reads" test("the last verifier read enable-auto-merge starts ends inside its step's timeout", () => { const arm = job("enable-auto-merge"); - const step = /- name: Arm and disarm auto-merge\n {8}if: .*\n {8}timeout-minutes: ([0-9]+)\n/.exec(arm); + const step = /- name: Arm and disarm auto-merge\n {8}id: arm\n {8}if: .*\n {8}timeout-minutes: ([0-9]+)\n/.exec(arm); assert.ok(step, "the arm step names its timeout"); // The room read, then the verdict budget and the last read it starts. assert.ok(LANE_ROOM_TIMEOUT_MS + VERIFY_BUDGET_MS + VERIFY_TIMEOUT_MS < Number(step[1]) * 60_000); @@ -360,6 +360,29 @@ test("enable-auto-merge's step timeouts fit inside its job's", () => { assert.ok(steps.reduce((sum, minutes) => sum + minutes, 0) < total, `${steps.join(" + ")} inside ${total}`); }); +// pr-unblock's report-conflict-status precedent: statuses: write only on a +// job that holds no lane token and runs only main's script. +test("the pr-merge/unarmed statuses are posted by a job of their own, with statuses: write and nothing it could run from a pull request", () => { + const arm = job("enable-auto-merge"); + assert.match(arm, /\n {4}outputs:\n {6}unarmed-statuses: \$\{\{ steps\.arm\.outputs\.unarmed-statuses \}\}\n/); + assert.match(arm, /\n {6}- name: Arm and disarm auto-merge\n {8}id: arm\n/); + assert.match(arm, /RISK_ESCALATION_MINUTES=\$\(yq '\.merge\."risk-escalation-minutes" \/\/ ""' "\$config"\)\n/); + const post = job("unarmed-status"); + assert.match(post, /^ {4}needs: \[enable-auto-merge\]\n/); + // It runs whenever enable-auto-merge handed it statuses, after a failed + // step too, and only then. + assert.match(post, /\n {4}if: \$\{\{ !cancelled\(\) && needs\.enable-auto-merge\.outputs\.unarmed-statuses != '' \}\}\n/); + const block = /\n {4}permissions:\n((?: {6}[a-z-]+: [a-z]+.*\n)+)/.exec(post)[1]; + assert.deepEqual( + [...block.matchAll(/ {6}([a-z-]+): ([a-z]+)/g)].map((m) => `${m[1]}: ${m[2]}`), + ["contents: read", "statuses: write"], + ); + assert.doesNotMatch(post, /lane-token|request-lane-token|id-token/); + assert.match(post, /\n {10}ref: refs\/heads\/main\n {10}path: \.base\n {10}persist-credentials: false\n/); + // The statuses reach the script through its environment, never the shell. + assert.match(post, /\n {10}STATUSES: \$\{\{ needs\.enable-auto-merge\.outputs\.unarmed-statuses \}\}\n {8}run: node \.base\/\.github\/scripts\/pr-merge\/post-unarmed-status\.mjs\n/); +}); + // The merge job's sizing comment: about a minute before the loop, then the // longer of the gate budget (10) plus a candidate's read limit (4) and a // deferred candidate's ceiling (20), plus one read's own timeout (4); the diff --git a/.github/workflows/pr-merge.yml b/.github/workflows/pr-merge.yml index d651fde5d3..4acc04f28e 100644 --- a/.github/workflows/pr-merge.yml +++ b/.github/workflows/pr-merge.yml @@ -1536,6 +1536,9 @@ jobs: checks: read # the review lanes' annotations, as the gate binds their evidence issues: read # the demotion records the lane's room re-checks id-token: write # prove to the token broker which workflow this is + # The pr-merge/unarmed statuses for the unarmed-status job to post. + outputs: + unarmed-statuses: ${{ steps.arm.outputs.unarmed-statuses }} steps: # As in the merge job: a rate limit post-merge-dispatch stopped on holds # this job's GITHUB_TOKEN reads too. @@ -1695,6 +1698,7 @@ jobs: # While merge.report-only is anything but false it reports each # decision and writes nothing. - name: Arm and disarm auto-merge + id: arm if: steps.lane-token.outputs.token != '' timeout-minutes: 18 env: @@ -1722,7 +1726,8 @@ jobs: config=.base/docs/conventions/pr-pipeline.yaml REPORT_ONLY=$(yq '.merge."report-only"' "$config") LANES_BOT_LOGIN=$(yq '.merge."agent-identity" // ""' "$config") - export REPORT_ONLY LANES_BOT_LOGIN + RISK_ESCALATION_MINUTES=$(yq '.merge."risk-escalation-minutes" // ""' "$config") + export REPORT_ONLY LANES_BOT_LOGIN RISK_ESCALATION_MINUTES node .base/.github/scripts/pr-merge/enable-auto-merge.mjs - name: Log the API budget at job end @@ -1775,6 +1780,43 @@ jobs: # cancelled: babysit_postmerge.py and post-merge-demotion-alert.yml both # count a cancelled full run as red. Every other event takes a group of its # own and does not wait. + # A lanes App pull request whose refusal the lanes clear shows it as a + # failing pr-merge/unarmed commit status on its head once the refusal has + # stood merge.risk-escalation-minutes, never the human-gated label, and a + # passing one once armed (owner rule 2026-10-26). A job of its own, as + # pr-unblock's report-conflict-status: the lanes App token carries no + # statuses permission, and only this job, which holds no lane token or + # OIDC token and runs nothing but main's script, gets statuses: write. + # Rules: .github/scripts/pr-merge/post-unarmed-status.mjs. + unarmed-status: + needs: [enable-auto-merge] + if: ${{ !cancelled() && needs.enable-auto-merge.outputs.unarmed-statuses != '' }} + runs-on: ubuntu-24.04 + timeout-minutes: 5 + permissions: + contents: read # check out main + statuses: write # post pr-merge/unarmed on lanes App pull request heads + steps: + - name: Check out the default branch tip + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + ref: refs/heads/main + path: .base + persist-credentials: false + + - name: Set up Node + uses: actions/setup-node@949feb2413d6458794dcd2491c4babbbce0c15c1 # v7.1.0 + with: + node-version-file: .base/.node-version + package-manager-cache: false + + - name: Post the pr-merge/unarmed statuses + timeout-minutes: 3 + env: + GH_TOKEN: ${{ github.token }} + STATUSES: ${{ needs.enable-auto-merge.outputs.unarmed-statuses }} + run: node .base/.github/scripts/pr-merge/post-unarmed-status.mjs + dispatch-full-run: needs: [merge] if: >- diff --git a/docs/conventions/pr-pipeline.yaml b/docs/conventions/pr-pipeline.yaml index 3151fc61ce..ce6cbcd458 100644 --- a/docs/conventions/pr-pipeline.yaml +++ b/docs/conventions/pr-pipeline.yaml @@ -925,10 +925,12 @@ merge: # 30-minute timeout with the minute before the loop and the closing steps # (judgment). gate-deferred-minutes: 20 - # Minutes a MED or HIGH reasons comment must stand unchanged at the head - # before the gate adds the human-gated label; 0 is no minimum. 60 is longer - # than a review lane's run (judgment), so a review still running at the - # head, which scores MED, finishes first. + # Minutes a reasons comment must stand unchanged at the head before the + # gate adds the human-gated label for a reason only a person clears, or + # enable-auto-merge posts a failing pr-merge/unarmed status for one the + # lanes clear; 0 is no minimum. 60 is longer than a review lane's run + # (judgment), so a review still running at the head, which scores MED, + # finishes first. risk-escalation-minutes: 60 # A candidate the last run refused on unchanged inputs is reported with that # refusal instead of gated again (babysit_merge_skip.py); it is gated again diff --git a/docs/conventions/pr-pipeline/README.md b/docs/conventions/pr-pipeline/README.md index 4ff65dd9e0..eed5df5d4c 100644 --- a/docs/conventions/pr-pipeline/README.md +++ b/docs/conventions/pr-pipeline/README.md @@ -1029,7 +1029,12 @@ An eligible pull request its score holds gets one `` naming the score, the reasons and the head. While `merge.report-only` is false, a run that finds the lane identity's reasons comment from an earlier run unchanged (same score, reasons and head) for at least `merge.risk-escalation-minutes` (default 60, timed from the comment's last change; 0 is no -minimum) escalates the refusal ([loop-lane ยง2](../loop-lane/README.md#2-escalation-contract)): it +minimum) escalates the refusal when a holding reason is one only a person clears +(`babysit_risk.person_reasons`: a CRITICAL security finding, bound or not, a high path, a +trusted-actor widening, a cost hold, or a listing past the files API's cap, owner rule 2026-10-26); +any other reason, such as review evidence not bound to the head, keeps its comment and is judged +again each run. An escalation +([loop-lane ยง2](../loop-lane/README.md#2-escalation-contract)) posts a new comment, that comment's text ending with a line saying the refusal persisted and needs a person, then adds the tracker's human-gated role label when no label matches it ignoring case, so the pull request leaves the WIP count and reaches the attended queue. A new comment notifies and is @@ -1184,11 +1189,19 @@ any of those, and does not turn it back on once a person has turned it off. A sc hand-started run that leaves a lanes App pull request unarmed on report-outcome's own `Hold: a human merges this` line, a hold path, a cost hold, the gate's verdict or its risk score says why in one `` comment, edited in place, and adds the human-gated -label only for a cause a person must clear: that hold line (owner decision Q-HOLDLINE), a -configured denied path or a [path floor](#merge-authority) path, a cost hold, an untrusted spec, -the repeated-FAIL round cap, a security finding or a trusted-actor widening. It puts the label -back while the comment stands unless a person removed it since; a person's own hold text gets -neither. GitHub then merges the pull request through the merge queue +label only for a cause a person must clear (owner rule 2026-10-26: an intent or design change or a +CRITICAL security issue, with cost and trust holds and a hold no lane can clear kept): that hold +line (owner decision Q-HOLDLINE), a configured denied path or a [path floor](#merge-authority) path, +a cost hold, untrusted provenance, an issue whose labels cannot be read whole, an untrusted spec, the +repeated-FAIL round cap, a pull request too old to verify, a security finding, a high path, a +trusted-actor widening or a listing past 300 files. It puts the label back while the comment stands +unless a person removed it since; a person's own hold text gets neither. Every other refusal, a +listing read that failed included, is the lanes': later scheduled runs judge it again, and once a +lanes' refusal has stood at the head for `merge.risk-escalation-minutes`, timed from the first one +noted there (the comment's `pr-merge-unarmed-since` line), the `unarmed-status` job posts a failing +`pr-merge/unarmed` commit status on the head; arming, or the refusal becoming a person's, posts a +passing one over it. The status is informational, not `ci-status`, and that job trusts only a status +the GitHub Actions bot posted. GitHub then merges the pull request through the merge queue once `ci-status` passes, which in the queue also holds a pass that no longer binds the live link: the body must still close, or with no closing issue name by a plain `Refs:` line, the issue the pass judged, linked the same way, alone or beside others (`merge-holds.mjs` `linkHold`), so a body @@ -1679,8 +1692,9 @@ exists (#7711). `verify:fail` and `verify:earlier-fail` only wait: the fail's notes go to an `address-feedback` round (`pr-feedback-dispatch.yml`, above), whose push the verifier judges again. -`verify:too-old`, `verify:repeated-fail`, `verify:spec-changed` and `verify:untrusted-spec` need a -human. While `merge.report-only` is false the lane escalates such a +`verify:too-old`, `verify:repeated-fail` and `verify:untrusted-spec` need a human; +`verify:spec-changed` holds for the lanes (owner rule 2026-10-26). While `merge.report-only` is +false the lane escalates such a refusal on the pull request ([loop-lane ยง2](../loop-lane/README.md#2-escalation-contract)): it keeps one `` comment naming the refusal and the head, and for `verify:repeated-fail` with a `fail` at the head quoting as data the first 1500 characters of diff --git a/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py b/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py index 3ab80bc469..5378af717d 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py @@ -85,9 +85,9 @@ report's `unattended` block says which. With `--merge`, a stop refusal dequeues the identity's queued entries, a held identity entry leaves the queue, an eligible MED or HIGH pull request gets one upserted - `` comment (and the human-gated role label once - an earlier run's identical comment has stood for - `merge.risk-escalation-minutes`), a verifier refusal that + `` comment (and, for a reason only a person + clears, the human-gated role label once an earlier run's identical comment + has stood for `merge.risk-escalation-minutes`), a verifier refusal that needs a human gets the tracker's human-gated role label and one upserted `` comment, a Dependabot update held under `merge.dependabot.majors: hold` gets the same label and one upserted @@ -405,6 +405,20 @@ def rerun_rate_limited_review( if not review_evidence.rate_limited(lane, {"annotations": annotations}): report["reason"] = "failure class is not rate-limit; never re-run" return report + # The binding takes attempt 2 only after an attempt 1 that concluded + # failure (review_evidence.rerun_binds), so the re-run must too. + try: + conclusion = json_object( + gh_json(["api", f"repos/{repo}/actions/runs/{run_id}/attempts/1"]) + ).get("conclusion") + except (RuntimeError, ValueError, json.JSONDecodeError) as exc: + report["reason"] = f"could not read the attempt: {type(exc).__name__}: {exc}" + return report + if conclusion != "failure": + report["reason"] = ( + f"attempt 1 concluded {conclusion!r}, so a re-run would not bind" + ) + return report proc = gh_capture( ["api", "-X", "POST", f"repos/{repo}/actions/runs/{run_id}/rerun"] ) @@ -3103,15 +3117,39 @@ def human(actor: Any) -> bool: return lanes, comments +class HeadFileUnproven(RuntimeError): + """A verdict about one path (`_head_file`): it 404s at the head and the + comparison does not prove it removed. Another path may still read.""" + + def _head_file(repo: str, head: str) -> Callable[[str, str], str | None]: """`bind()`'s `head_file`: a file's text at `head`, or None only when the comparison from the thread's commit lists it removed. A rename, a capped - file list or any other failure raises, so the thread keeps its human test.""" + file list or any other failure raises, so the thread keeps its human test. + After the first failed read (a refused or rate-limited contents or + comparison read) every later call raises without a read: it is never sent + again, however many threads or candidate heads + (review_evidence._carried_blocking) ask. `HeadFileUnproven` is a verdict + about its own path and latches nothing.""" cache: dict[tuple[str, str], str | None] = {} + failed: list[str] = [] def read(path: str, original: str) -> str | None: if (path, original) in cache: return cache[(path, original)] + if failed: + raise RuntimeError(f"head-file-not-read-after:{failed[0]}") + try: + text = _read_head_file(path, original) + except HeadFileUnproven: + raise + except Exception as exc: + failed.append(str(exc)) + raise + cache[(path, original)] = text + return text + + def _read_head_file(path: str, original: str) -> str | None: proc = gh_capture( [ "api", @@ -3139,9 +3177,8 @@ def read(path: str, original: str) -> str | None: for item in files ) ): - raise RuntimeError(f"head-file-unproven:{path}") + raise HeadFileUnproven(f"head-file-unproven:{path}") text = None - cache[(path, original)] = text return text return read @@ -3255,6 +3292,7 @@ def score_unattended( risk_input = babysit_risk.RiskInput( files=files, files_complete=complete, + files_capped=merge_policy.files_capped(pr), max_changed_lines=config.max_changed_lines, high_paths=config.denied_paths, lanes=lanes, @@ -3403,6 +3441,9 @@ def risk_comment_body(risk: babysit_risk.RiskResult, head: str) -> str: "A floor path holds it: a human merges it in person, never an agent or a lane." if _floor_held(risk) else "This pull request waits for one." + if babysit_risk.person_reasons(babysit_risk.holding(risk)) + else "The lanes clear its holding reasons (a size reason holds nothing); " + "pr-merge judges it again each run." ) lines = [ RISK_MARKER, @@ -3901,9 +3942,11 @@ def unattended_writes( this run refused it for itself (`own_refusal`) or its score holds it (`risk:`). Then the reasons comment for an eligible pull request its score holds, on a path not stopped, escalated (`escalate`) - when the comment read before this run already says the same and has for - `merge.risk-escalation-minutes` (`_stood_unchanged`), the comment first - rewritten to end with `RISK_ESCALATION_LINE` (once); once the score no longer holds it, a refusal + when a holding reason is one only a person clears + (`babysit_risk.person_reasons`) and the comment read before this run + already says the same and has for `merge.risk-escalation-minutes` + (`_stood_unchanged`), the comment first rewritten to end with + `RISK_ESCALATION_LINE` (once); once the score no longer holds it, a refusal comment naming the head becomes `risk_cleared_body`; and the escalation (`escalate_verify`) for a verifier refusal that needs a human, or for a Dependabot update held under `merge.dependabot.majors: hold`, keyed on the @@ -4021,13 +4064,21 @@ def judge(n: int) -> str | None: text = _normalized((prior or {}).get("body")) # An escalated comment at this head stays escalated when its # reasons change, so the label never stands beside a comment that - # no longer says why. - if prior is not None and ( - text == _normalized(escalated) - or ( - text.endswith(RISK_ESCALATION_LINE) and f" at head `{head}`" in text + # no longer says why. Only a reason a person clears escalates + # (babysit_risk.person_reasons, owner rule 2026-10-26); the lanes + # clear the rest, so their comment stays the record. + person = babysit_risk.person_reasons(babysit_risk.holding(risk)) + if ( + person + and prior is not None + and ( + text == _normalized(escalated) + or ( + text.endswith(RISK_ESCALATION_LINE) + and f" at head `{head}`" in text + ) + or _stood_unchanged(prior, body, minutes) ) - or _stood_unchanged(prior, body, minutes) ): escalation = escalate( repo, number, identity, escalated, RISK_MARKER, diff --git a/plugins/source-control/skills/babysit-prs/scripts/babysit_merge_policy.py b/plugins/source-control/skills/babysit-prs/scripts/babysit_merge_policy.py index 88c84ec661..dc7af5c61f 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_merge_policy.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_merge_policy.py @@ -576,10 +576,11 @@ class PipelineConfig: # Absent, or anything but a whole number from 1 to # `MAX_QUEUE_EJECTIONS_LIMIT`, is 2. max_queue_ejections: int = 2 - # `merge.risk-escalation-minutes`: how long a MED or HIGH reasons comment - # must have stood unchanged at the head before the gate adds the - # human-gated label; 0 is no minimum. Absent, or anything but a whole - # number, is `RISK_ESCALATION_MINUTES`. + # `merge.risk-escalation-minutes`: how long a reasons comment must have + # stood unchanged at the head before the gate adds the human-gated label + # for a reason only a person clears (`babysit_risk.person_reasons`); 0 is + # no minimum. Absent, or anything but a whole number, is + # `RISK_ESCALATION_MINUTES`. risk_escalation_minutes: int = RISK_ESCALATION_MINUTES # `merge.max-verify-fails`: counted pr-verify FAILs at distinct heads of # one pull request at which it holds for a human whatever a later pass @@ -4011,6 +4012,18 @@ def _changed_files( return tuple(files), complete +# The most files GitHub's pull request files listing returns +# (https://docs.github.com/en/rest/pulls/pulls#list-pull-requests-files). +FILES_API_CAP = 3000 + + +def files_capped(pr: Mapping[str, Any]) -> bool: + """Whether the REST pull request record changes more files than the + listing returns (`FILES_API_CAP`), so no read ever lists them all.""" + count = pr.get("changed_files") + return isinstance(count, int) and not isinstance(count, bool) and count > FILES_API_CAP + + def changed_files( repo: str, pr: Mapping[str, Any] ) -> tuple[tuple[ChangedFile, ...], bool]: @@ -4499,7 +4512,9 @@ def shared_refusal( or changed since (`_demotion_changed`), and judge every merge the post-merge check would judge now. The demotion comes last, so `demotion-changed` means nothing else refused (`lane_room` keeps such an - answer when the demotion reads the same). The binding comes before the + answer only when a fresh demotion read equals the shared one in every + field: the records, the newest record's creation and the newest clear + move the floor and the evidence window too). The binding comes before the age, so a candidate bound elsewhere reports `SHARED_BINDING_REFUSALS` and does not replace the run's answer. The floor is compared under the shared answer's demotion, which may be out of date; that changes only the diff --git a/plugins/source-control/skills/babysit-prs/scripts/babysit_review_evidence.py b/plugins/source-control/skills/babysit-prs/scripts/babysit_review_evidence.py index 7550c696b5..d171084502 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_review_evidence.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_review_evidence.py @@ -244,7 +244,12 @@ def bind( ) except Exception as exc: # noqa: BLE001 - any doubt is unbound reason = exc.reason if isinstance(exc, Unbound) else "evidence-error" - return tuple(LaneEvidence(lane, False, reason, None) for lane in LANES) + return tuple( + _unbound( + lane, reason, head_sha, runs, comments, threads, is_human, head_file + ) + for lane in LANES + ) def lane_source(path: Any) -> bool: @@ -288,11 +293,78 @@ def _bind_lane( [(None, marker), *_summary_comments(lane, comments)], ) except Unbound as exc: - return LaneEvidence(lane, False, exc.reason, None) + return _unbound( + lane, exc.reason, head, runs, comments, threads, is_human, head_file + ) blocking = critical if lane == SEVERITY_LANE else None return LaneEvidence(lane, True, None, count, clears, blocking) +def _unbound( + lane: str, + reason: str, + head: Any, + runs: Any, + comments: Any, + threads: Any, + is_human: Callable[[Any], bool], + head_file: Callable[[str, str], str | None] | None, +) -> LaneEvidence: + """The unbound evidence for `lane`, carrying for the security lane the + blocking findings its last bound review on this pull request still + leaves open (`_carried_blocking`), so an unbind never hides one.""" + carried = ( + _carried_blocking(head, runs, comments, threads, is_human, head_file) + if lane == SEVERITY_LANE + else None + ) + return LaneEvidence(lane, False, reason, None, False, carried or None) + + +def _carried_blocking( + head: Any, + runs: Any, + comments: Any, + threads: Any, + is_human: Callable[[Any], bool], + head_file: Callable[[str, str], str | None] | None, +) -> int | None: + """The security lane's blocking findings at the newest head of this pull + request its evidence binds at, the pinned head first, judged as at that + head against the threads and comments read now; None when none binds. + It reads only what the caller already read.""" + try: + lane_runs = _lane_runs(SEVERITY_LANE, runs) + ordered = sorted( + lane_runs, + key=lambda item: (str(item.get("created_at") or ""), item["id"]), + reverse=True, + ) + heads = [head, *dict.fromkeys(item["head_sha"] for item in ordered)] + except Exception: # noqa: BLE001 - nothing read, nothing carried + return None + for sha in dict.fromkeys(h for h in heads if isinstance(h, str)): + try: + reviewed, chain = _resolve(SEVERITY_LANE, sha, lane_runs) + marker = _require_marker(SEVERITY_LANE, sha, comments, newest_only=False) + _require_marked_runs(SEVERITY_LANE, lane_runs, comments) + _, _, critical = _open_findings( + SEVERITY_LANE, + lane_runs, + threads, + is_human, + set(), + reviewed, + chain, + head_file, + [(None, marker), *_summary_comments(SEVERITY_LANE, comments)], + ) + except Exception: # noqa: BLE001 - this head does not bind; try older + continue + return critical + return None + + def _lane_runs(lane: str, runs: Sequence[Mapping[str, Any]]) -> list[Mapping[str, Any]]: path = LANES[lane][0] if not isinstance(runs, list | tuple): @@ -543,9 +615,16 @@ def _record(lane: str, sha: str, job: Mapping[str, Any]) -> _Record | None: ) -def _require_marker(lane: str, head: str, comments: Sequence[Mapping[str, Any]]) -> str: +def _require_marker( + lane: str, + head: str, + comments: Sequence[Mapping[str, Any]], + newest_only: bool = True, +) -> str: """The newest reviewed-head marker the lane job wrote must name `head`; - its body, which carries the covering review's summary.""" + its body, which carries the covering review's summary. With + `newest_only` False, the newest marker naming `head` will do, for a + review a newer marker has passed.""" if not isinstance(comments, list | tuple): raise Unbound("evidence-error") newest: tuple[str, int, str, str] | None = None @@ -557,7 +636,7 @@ def _require_marker(lane: str, head: str, comments: Sequence[Mapping[str, Any]]) continue first_line = str(comment.get("body") or "").split("\n", 1)[0].rstrip("\r") match = MARKER_RE.match(first_line) - if not match or match[1] != lane: + if not match or match[1] != lane or not (newest_only or match[2] == head): continue key = ( str(comment.get("created_at") or ""), diff --git a/plugins/source-control/skills/babysit-prs/scripts/babysit_risk.py b/plugins/source-control/skills/babysit-prs/scripts/babysit_risk.py index 0134912476..a0a9583129 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_risk.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_risk.py @@ -89,6 +89,7 @@ import json import re from collections import Counter +from collections.abc import Iterable from dataclasses import dataclass from typing import Any @@ -166,6 +167,9 @@ class RiskInput: # Paths only a trusted author's own commits touched: the path floor and # the manifest review leave them out (owner decision 2026-10-22). floor_exempt: frozenset[str] = frozenset() + # The listing reached the files API's cap, so no read ever completes it + # and its paths can never all be judged. + files_capped: bool = False @dataclass(frozen=True) @@ -936,6 +940,9 @@ def _size_reasons(inp: RiskInput, files: list[_File] | None) -> list[Reason]: incomplete.extend(f"{f.path or '?'}: {p}" for f in files for p in f.problems) if incomplete: reasons.append(Reason("files-incomplete", Risk.MED, "; ".join(incomplete))) + if inp.files_capped is True: + detail = "the listing is past the files API's cap, so it never reads whole" + reasons.append(Reason(FILES_CAPPED_CODE, Risk.HIGH, detail)) lines = sum(f.lines for f in files) limit = inp.max_changed_lines if limit is None and inp.lines_optional is True: @@ -997,6 +1004,11 @@ def _lane_reasons(raw_lanes: Any) -> list[Reason]: reasons.append( Reason(f"evidence-unbound:{name}", Risk.MED, ": ".join(parts)) ) + # A blocking finding the lane did report holds whether or not its + # evidence binds to the head. + if name == _HOLDING_LANE and _is_count(count) and count > 0: + detail = f"{count} open, evidence unbound" + reasons.append(Reason(finding_code, finding_severity, detail)) continue total = getattr(lane, "open_findings", None) if name != _HOLDING_LANE: @@ -1056,6 +1068,29 @@ def holding(result: RiskResult) -> list[Reason]: ] +# The holding reasons only a person clears (owner rule 2026-10-26: a person +# only for an intent or design change or a CRITICAL security issue, with +# cost and trust holds kept, and for a hold no lane can clear): a CRITICAL +# security finding, a trusted-actor widening, a cost hold, every high path +# (the floor, a configured denied path, or an aliasing path, symlink, +# submodule or mode change) and a listing past the files API's cap. The lanes +# clear every other holding reason, such as review evidence not yet bound to +# the head or a listing a read did not finish, by judging again. The one list +# both the merge gate and enable-auto-merge.mjs read. +FILES_CAPPED_CODE = "files-capped" +PERSON_CODES = frozenset({LANES[_HOLDING_LANE][0], COST_CODE, FILES_CAPPED_CODE}) +PERSON_PREFIXES = (TRUST_GATE_PREFIX, "high-path:") + + +def person_reasons(reasons: Iterable[Reason]) -> list[Reason]: + """The reasons in `reasons` only a person clears (`PERSON_CODES`).""" + return [ + r + for r in reasons + if r.code in PERSON_CODES or r.code.startswith(PERSON_PREFIXES) + ] + + def holds(result: RiskResult) -> bool: """Whether the score refuses an unattended merge (`holding`).""" return bool(holding(result)) diff --git a/plugins/source-control/skills/babysit-prs/scripts/babysit_verify_evidence.py b/plugins/source-control/skills/babysit-prs/scripts/babysit_verify_evidence.py index 274dc1885d..40a3c42075 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_verify_evidence.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_verify_evidence.py @@ -73,13 +73,15 @@ head whose passes all predate the spec's last change, and the lost-run reconciler re-runs the head's checks to start that judgment. A changed closing set or a removed spec comment leaves no time to compare, and waits - for a person; + for a new head; - otherwise `verify:missing`. -`verify:too-old`, `verify:repeated-fail`, `verify:spec-changed` and -`verify:untrusted-spec` need a human, who merges in person; a -`verify:repeated-fail` escalation quotes the newest head FAIL's summary -(`Reader.fail_summary`). A `verify:fail` or `verify:earlier-fail` only waits: +`verify:too-old`, `verify:repeated-fail` and `verify:untrusted-spec` need a +human (`NEEDS_HUMAN`), who merges in person; a `verify:repeated-fail` +escalation quotes the newest head FAIL's summary (`Reader.fail_summary`). +`verify:spec-changed` holds for the lanes (owner rule 2026-10-26), and +enable-auto-merge.mjs shows one that stands as a failing `pr-merge/unarmed` +status. A `verify:fail` or `verify:earlier-fail` only waits: pr-feedback-dispatch.yml posts a FAIL's notes on the pull request as review comments for an address-feedback round, whose push gives a head the verifier judges again. Any read that fails refuses @@ -235,12 +237,14 @@ def verdict_name(name: str) -> re.Match[str] | None: # A verdict's `intent` when its pull request closed no issue in this # repository: the title and body were the spec (write-verdict.mjs). INTENT_PULL_REQUEST = "pull-request" -NEEDS_HUMAN = ( - REPEATED_FAIL, - TOO_OLD, - UNTRUSTED_SPEC, - SPEC_CHANGED, -) +# The refusals only a person clears (owner rule 2026-10-26: a person only +# for an intent or design change or a CRITICAL security issue, with trust +# holds kept): an untrusted spec (provenance), and the two no lane can clear, +# the repeated-FAIL cap (terminal: `merge.max-verify-fails`) and an old pull +# request (judged from its creation, so it never clears). A changed spec +# holds for the lanes, which judge it again. The one list the merge gate and +# enable-auto-merge.mjs (`needsHuman`) read. +NEEDS_HUMAN = (REPEATED_FAIL, TOO_OLD, UNTRUSTED_SPEC) ISSUE_FIELDS = ( "number lastEditedAt repository{nameWithOwner} " "labels(first:100){totalCount nodes{name}} " @@ -1868,8 +1872,9 @@ def lane_risk_refusal( pr: dict[str, Any], config: merge_policy.PipelineConfig, issue: int | None, -) -> str | None: - """Why pr-merge's gate would hold the pull request on its risk score, or +) -> tuple[str, bool] | None: + """Why pr-merge's gate would hold the pull request on its risk score, and + whether only a person clears it (`babysit_risk.person_reasons`), or None: `babysit_risk.score` and `holding` over the inputs the gate builds (`babysit_merge.score_unattended`) for a lane pull request, with `config` the base's tightened by the default branch head's. So the review lanes' evidence (an open CRITICAL @@ -1885,7 +1890,7 @@ def lane_risk_refusal( try: identity = config.agent_identity if not identity: - return "unreadable: no agent identity" + return "unreadable: no agent identity", False permission = merge_policy.permission_reader(repo) files, complete = merge_policy.changed_files(repo, pr) lanes, _ = babysit_merge._lane_evidence( @@ -1904,6 +1909,7 @@ def lane_risk_refusal( risk_input = babysit_risk.RiskInput( files=files, files_complete=complete, + files_capped=merge_policy.files_capped(pr), max_changed_lines=config.max_changed_lines, high_paths=config.denied_paths, lanes=lanes, @@ -1921,9 +1927,10 @@ def lane_risk_refusal( if decision: risk = babysit_risk.score(replace(risk_input, cost_decision=decision)) except Exception as exc: # noqa: BLE001 - any doubt holds, as the gate's - return f"unreadable: {type(exc).__name__}: {exc}" + return f"unreadable: {type(exc).__name__}: {exc}", False held = babysit_risk.holding(risk) - return "; ".join(f"{r.code}: {r.detail}" for r in held) or None + text = "; ".join(f"{r.code}: {r.detail}" for r in held) + return (text, bool(babysit_risk.person_reasons(held))) if text else None def lane_queue( @@ -1981,7 +1988,8 @@ def main(argv: list[str] | None = None, *, now: datetime | None = None) -> int: and no verdict or config read. With `--lane-reviews` (and `--base-sha` and `--rung`, the lane room's effective rung), a line whose refusal is null also carries `reviews`: null when pr-merge's gate would not hold - the pull request on its risk score (`lane_risk_refusal`), else why. With + the pull request on its risk score (`lane_risk_refusal`), else why, and + `reviewsPerson`: whether only a person clears that hold. With `--lane-queue` too, a line whose `reviews` is null also carries `queue` (`lane_queue`): the head's failed-check merge-queue removals, the `merge.max-queue-ejections` limit, and the gate's refusal and comment at @@ -2080,6 +2088,9 @@ def main(argv: list[str] | None = None, *, now: datetime | None = None) -> int: evidence = reader.evidence(args.number, args.head, pr) if args.lane_reviews and evidence.refusal is None: head = json_object(json_object(pr).get("head")).get("sha") + # Whether only a person clears `reviews`; an unread score the lanes + # read again. + link["reviewsPerson"] = False if head != args.head: link["reviews"] = f"unreadable: the head is {head}, not {args.head}" elif config is None: @@ -2092,8 +2103,8 @@ def main(argv: list[str] | None = None, *, now: datetime | None = None) -> int: cap=args.rung if args.rung in merge_policy.RUNG_RANK else "off", demoted=False, ).rung - link["reviews"] = ( - f"unreadable: the lane's rung is {args.rung}" + held = ( + (f"unreadable: the lane's rung is {args.rung}", False) if rung == "off" else lane_risk_refusal( args.repo, @@ -2103,6 +2114,7 @@ def main(argv: list[str] | None = None, *, now: datetime | None = None) -> int: reader.linked_issue.get(args.number), ) ) + link["reviews"], link["reviewsPerson"] = held or (None, False) if args.lane_queue and link["reviews"] is None and config is not None: link["queue"] = lane_queue( args.repo, diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_lane_reviews.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_lane_reviews.py index 50d863da3f..2bb74f7e4f 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_lane_reviews.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_lane_reviews.py @@ -93,6 +93,15 @@ def refusal( evidence: object = None, config: policy.PipelineConfig = CONFIG, ) -> str | None: + held = held_by(files, evidence, config) + return None if held is None else held[0] + + +def held_by( + files: tuple[ChangedFile, ...] = (DOC,), + evidence: object = None, + config: policy.PipelineConfig = CONFIG, +) -> tuple[str, bool] | None: evidence = lanes() if evidence is None else evidence effect = evidence if isinstance(evidence, Exception) else None with ( @@ -153,6 +162,24 @@ def test_a_failed_read_holds(self) -> None: ) self.assertTrue(str(held).startswith("unreadable: "), held) + def test_only_a_security_finding_or_a_path_hold_needs_a_person(self) -> None: + # Owner rule 2026-10-26: unbound review evidence, the manifest review + # and a failed read go back to the lanes; a CRITICAL finding and a + # symlink or submodule (an unsafe path) stay a person's. + after = PLUGIN_BASE.replace('"name": "foo",', '"name": "foo", "hooks": {},') + unproven = babysit_merge.HeadPusherUnproven("head-pusher-unproven:a") + cases = { + "critical finding": (held_by(evidence=lanes(critical=1)), True), + "symlink": (held_by((DOC, SYMLINK)), True), + "unbound review": (held_by(evidence=lanes(bound=False)), False), + "manifest": (held_by((DOC, manifest(after)), config=FLOOR_ON), False), + "failed read": (held_by(evidence=unproven), False), + } + for name, (held, person) in cases.items(): + with self.subTest(name): + self.assertIsNotNone(held) + self.assertIs(held[1], person, held) + class EvidenceCommandReadsTheRisk(unittest.TestCase): def cli( @@ -162,6 +189,7 @@ def cli( *, head: str = HEAD, rung: str | None = "C2", + person: bool = False, ) -> dict[str, object]: out = io.StringIO() argv = ["evidence", "--lane-reviews", "--repo", REPO, "--base-sha", BASE] @@ -175,7 +203,9 @@ def cli( mock.patch.object( ve.Reader, "evidence", lambda self, n, h, pr=None: evidence ), - mock.patch.object(ve, "lane_risk_refusal", return_value=risk) as read, + mock.patch.object( + ve, "lane_risk_refusal", return_value=risk and (risk, person) + ) as read, contextlib.redirect_stdout(out), ): ve.main([*argv, "41", HEAD]) @@ -189,6 +219,21 @@ def test_a_pass_carries_the_risk_hold_when_the_room_has_a_rung(self) -> None: ) self.assertIsNone(self.cli(ve.Evidence(None, HEAD, (), 0), None)["reviews"]) + def test_the_line_says_whether_only_a_person_clears_the_risk_hold(self) -> None: + passed = ve.Evidence(None, HEAD, (), 0) + for risk, person in ( + ("security-finding: 1 open", True), + ("evidence-unbound:claude-review: not bound", False), + (None, False), + ): + with self.subTest(risk): + report = self.cli(passed, risk, person=person) + self.assertEqual( + (report["reviews"], report["reviewsPerson"]), (risk, person) + ) + unread = self.cli(passed, None, head="d" * 40) + self.assertIs(unread["reviewsPerson"], False) + def test_no_rung_from_the_room_holds(self) -> None: for rung in (None, "off", "C9"): with self.subTest(rung=rung): @@ -244,7 +289,9 @@ def cli( "evidence", lambda self, n, h, pr=None: evidence or ve.Evidence(None, HEAD, (), 0), ), - mock.patch.object(ve, "lane_risk_refusal", return_value=risk), + mock.patch.object( + ve, "lane_risk_refusal", return_value=risk and (risk, False) + ), mock.patch.object( babysit_merge, "read_merge_queue", diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_lane_room.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_lane_room.py index 265200fa79..265a2902aa 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_lane_room.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_lane_room.py @@ -316,6 +316,16 @@ def test_a_demotion_that_moved_reads_the_answer_again(self) -> None: "rung-off:demotion", ), "another record": (policy.Demotion(False, None, (9,)), None), + # The same records with another clear or newest record: the + # keep binds every field, not the record numbers alone. + "another clear": ( + policy.Demotion(False, None, (), cleared_at=NEW_FLOOR), + None, + ), + "another newest record": ( + policy.Demotion(False, None, (), newest_created=NEW_FLOOR), + None, + ), } for name, (demotion, refusal) in cases.items(): with self.subTest(name): diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_rate_limit_rerun.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_rate_limit_rerun.py index 6da73db8c5..903f5fc209 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_rate_limit_rerun.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_rate_limit_rerun.py @@ -75,6 +75,7 @@ def _run( annotations: list[str | dict[str, Any]] = (RATE_LIMITED,), # type: ignore[assignment] now: str = "2026-10-09T15:00:00Z", check: dict[str, Any] = CHECK, + attempt_conclusion: str = "failure", ) -> tuple[dict[str, Any], list[list[str]]]: reads: list[list[str]] = [] posts: list[list[str]] = [] @@ -91,6 +92,8 @@ def gh_json(args: list[str]) -> Any: else {"annotation_level": "failure", "message": a} for a in annotations ] + if args[1] == f"repos/{REPO}/actions/runs/{RUN_ID}/attempts/1": + return {"run_attempt": 1, "conclusion": attempt_conclusion} raise AssertionError(f"unexpected gh_json call: {args}") def capture(cmd: list[str]) -> Any: @@ -114,6 +117,17 @@ def test_rate_limit_past_the_reset_reruns_the_whole_run(self) -> None: posts, [["api", "-X", "POST", f"repos/{REPO}/actions/runs/{RUN_ID}/rerun"]] ) + def test_a_failed_job_in_an_attempt_that_did_not_fail_is_not_rerun(self) -> None: + # The binding (review_evidence.rerun_binds) takes attempt 2 only + # after an attempt 1 that concluded failure, so a re-run of any other + # attempt would spend the one allowed attempt on a hold. + for conclusion in ("cancelled", "success"): + with self.subTest(conclusion): + report, posts = self._run(attempt_conclusion=conclusion) + self.assertFalse(report["rerun"]) + self.assertIn(conclusion, report["reason"]) + self.assertEqual(posts, []) + def test_before_the_reset_waits_without_reading_annotations(self) -> None: report, posts = self._run(now="2026-10-09T14:59:00Z") self.assertFalse(report["rerun"]) diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_unattended.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_unattended.py index b4496841bc..fe9e0361d5 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_unattended.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_unattended.py @@ -35,6 +35,7 @@ import babysit_dependabot as dependabot import babysit_gh as gh import babysit_merge as merge +import test_babysit_review_evidence as evidence_tests import babysit_merge_prefilter as prefilter import babysit_merge_policy as policy import babysit_postmerge as postmerge @@ -712,6 +713,8 @@ def test_a_policy_without_a_refusal_is_enabled(self) -> None: LaneEvidence(lane, False, "no-marker", None) for lane in ("claude-review", "claude-security-review") ) +# An open CRITICAL security finding: a hold only a person clears. +PERSON_HELD = (BOUND[0], LaneEvidence("claude-security-review", True, None, 1)) PROMOTED = policy.Promotion(dict.fromkeys(policy.PROMOTABLE_CELLS, "promoted"), None) PROCEED = policy.KillSwitch(True, "proceed") REAL_ASYNC_MERGE = merge.async_merge @@ -3373,7 +3376,7 @@ class ReasonsComment(Unattended): """MED/HIGH on an eligible pull request: one upserted comment (T14, D15).""" def body(self) -> str: - self.lanes = UNBOUND + self.lanes = PERSON_HELD self.run_main() posted = [ c @@ -3499,7 +3502,7 @@ def test_a_review_pending_med_ten_minutes_on_gets_no_label(self) -> None: self.setUp() self.earlier_sweep(body) self.now = self.WRITTEN + timedelta(minutes=10) - self.lanes = UNBOUND + self.lanes = PERSON_HELD _, out = self.run_main() self.assertEqual(self.log, []) self.assertEqual(out["unattended"]["writes"]["riskComment"], "unchanged") @@ -3510,7 +3513,7 @@ def test_the_same_reasons_sixty_one_minutes_on_get_the_label(self) -> None: self.setUp() self.earlier_sweep(body) self.now = self.WRITTEN + timedelta(minutes=61) - self.lanes = UNBOUND + self.lanes = PERSON_HELD _, out = self.run_main() self.assertEqual(self.log, ["risk-comment:POST", self.LABEL]) self.assertEqual( @@ -3545,7 +3548,7 @@ def test_escalating_posts_a_new_comment_before_the_label(self) -> None: body = self.body() self.setUp() self.earlier_sweep(body) - self.lanes = UNBOUND + self.lanes = PERSON_HELD self.run_main() self.assertEqual(self.log, ["risk-comment:POST", self.LABEL]) posted = self.posted_escalation() @@ -3559,7 +3562,7 @@ def test_an_escalated_comment_is_not_extended_again(self) -> None: body = self.body() self.setUp() self.earlier_sweep(f"{body}\n\n{merge.RISK_ESCALATION_LINE}") - self.lanes = UNBOUND + self.lanes = PERSON_HELD _, out = self.run_main() self.assertEqual(self.log, [self.LABEL]) self.assertEqual( @@ -3589,7 +3592,7 @@ def escalated_earlier(self) -> None: body = self.body() self.setUp() self.earlier_sweep(f"{body}\n\n{merge.RISK_ESCALATION_LINE}") - self.lanes = UNBOUND + self.lanes = PERSON_HELD def test_a_person_removing_the_label_after_the_escalation_keeps_it_off( self, @@ -3799,7 +3802,7 @@ def test_a_removal_before_a_new_escalation_does_not_lift_it(self) -> None: body = self.body() self.setUp() self.earlier_sweep(body) - self.lanes = UNBOUND + self.lanes = PERSON_HELD self.timeline = [ self.unlabeled("owner-person", "User", self.WRITTEN + timedelta(minutes=30)) ] @@ -3815,7 +3818,7 @@ def test_an_escalated_comment_at_a_new_head_drops_the_line(self) -> None: old = self.body().replace(HEAD, "d" * 40) self.setUp() self.earlier_sweep(f"{old}\n\n{merge.RISK_ESCALATION_LINE}") - self.lanes = UNBOUND + self.lanes = PERSON_HELD _, out = self.run_main() self.assertEqual(self.log, ["risk-comment:PATCH"]) self.assertNotIn(merge.RISK_ESCALATION_LINE, self.patched_body()) @@ -3826,7 +3829,7 @@ def test_a_recent_rewrite_of_the_reasons_resets_the_age(self) -> None: body = self.body() self.setUp() self.earlier_sweep(body, updated=self.now - timedelta(minutes=10)) - self.lanes = UNBOUND + self.lanes = PERSON_HELD _, out = self.run_main() self.assertEqual(self.log, []) self.assertNotIn("riskEscalation", out["unattended"]["writes"]) @@ -3837,7 +3840,7 @@ def test_no_minimum_labels_the_second_sweep(self) -> None: self.earlier_sweep(body) self.config = dataclasses.replace(self.config, risk_escalation_minutes=0) self.now = self.WRITTEN + timedelta(minutes=1) - self.lanes = UNBOUND + self.lanes = PERSON_HELD self.run_main() self.assertEqual(self.log, ["risk-comment:POST", self.LABEL]) @@ -3845,47 +3848,69 @@ def test_another_bot_accounts_marker_is_not_an_earlier_sweep(self) -> None: body = self.body() self.setUp() self.earlier_sweep(body, login="other-app[bot]", kind="Bot") - self.lanes = UNBOUND + self.lanes = PERSON_HELD self.run_main() self.assertEqual(self.log, ["risk-comment:POST"]) def test_the_first_sweep_comments_without_a_label(self) -> None: - self.lanes = UNBOUND + self.lanes = PERSON_HELD _, out = self.run_main() self.assertEqual(self.log, ["risk-comment:POST"]) self.assertNotIn("riskEscalation", out["unattended"]["writes"]) - def test_a_second_sweep_at_the_same_head_adds_the_human_gated_label(self) -> None: - # A MED or HIGH refusal on two sweeps at one head is - # labeled, the reasons comment first extended to say so. - for name, files in ( - ("MED", ((LOW_FILE,), True)), - ("HIGH", ((dataclasses.replace(LOW_FILE, path=".gitmodules"),), True)), + def test_a_second_sweep_at_the_same_head_labels_only_a_person_hold(self) -> None: + # A refusal a person clears (a high path) on two sweeps at one head + # is labeled, the reasons comment first extended to say so. One the + # lanes clear (review evidence not bound, MED) keeps its comment and + # gets no label however long it stands (owner rule 2026-10-26). + for name, files, lanes, person in ( + ("MED", ((LOW_FILE,), True), UNBOUND, False), + ( + "HIGH", + ((dataclasses.replace(LOW_FILE, path=".gitmodules"),), True), + BOUND, + True, + ), ): with self.subTest(name): self.setUp() self.config = dataclasses.replace(self.config, denied_paths=HELD) self.files = files - body = self.body() + self.lanes = lanes + self.run_main() + body = next( + c.args[0][-1].removeprefix("body=") + for c in self.capture.call_args_list + if "babysit-merge-risk" in " ".join(c.args[0]) + ) self.setUp() self.config = dataclasses.replace(self.config, denied_paths=HELD) self.files = files - self.lanes = UNBOUND + self.lanes = lanes self.earlier_sweep(body) + self.now = self.WRITTEN + timedelta(minutes=61) _, out = self.run_main() self.assertEqual(out["unattended"]["risk"]["score"], name) - self.assertEqual(self.log, ["risk-comment:POST", self.LABEL]) - self.assertEqual( - out["unattended"]["writes"]["riskEscalation"], - {"label": "added", "comment": "created"}, - ) + if person: + self.assertEqual(self.log, ["risk-comment:POST", self.LABEL]) + self.assertEqual( + out["unattended"]["writes"]["riskEscalation"], + {"label": "added", "comment": "created"}, + ) + else: + self.assertEqual(self.log, []) + self.assertEqual( + out["unattended"]["writes"]["riskComment"], "unchanged" + ) + self.assertNotIn("riskEscalation", out["unattended"]["writes"]) + self.assertIn("The lanes clear its holding reasons", body) def test_changed_reasons_at_the_same_head_rewrite_without_a_label( self, ) -> None: # The rewrite restarts the age the next sweep measures. self.earlier_sweep(f"{merge.RISK_MARKER}\nrisk **HIGH** at head `{HEAD}`.") - self.lanes = UNBOUND + self.lanes = PERSON_HELD _, out = self.run_main() self.assertEqual(self.log, ["risk-comment:PATCH"]) self.assertNotIn("riskEscalation", out["unattended"]["writes"]) @@ -3895,7 +3920,7 @@ def test_a_same_head_rerun_with_the_label_writes_nothing(self) -> None: self.setUp() self.earlier_sweep(f"{body}\n\n{merge.RISK_ESCALATION_LINE}") self.pr_labels = [{"name": "Needs-Human"}] - self.lanes = UNBOUND + self.lanes = PERSON_HELD _, out = self.run_main() self.assertEqual(self.log, []) self.assertEqual( @@ -3913,7 +3938,7 @@ def test_a_stored_escalation_differing_only_in_whitespace_writes_nothing( stored = f"{body}\n\n{merge.RISK_ESCALATION_LINE}".replace("\n", " \r\n") self.earlier_sweep(stored + " ") self.pr_labels = [{"name": "Needs-Human"}] - self.lanes = UNBOUND + self.lanes = PERSON_HELD _, out = self.run_main() self.assertEqual(self.log, []) self.assertEqual( @@ -3930,7 +3955,7 @@ def test_changed_reasons_on_an_escalated_comment_keep_the_escalation( f"\n\n{merge.RISK_ESCALATION_LINE}" ) self.pr_labels = [{"name": "Needs-Human"}] - self.lanes = UNBOUND + self.lanes = PERSON_HELD _, out = self.run_main() self.assertEqual(self.log, ["risk-comment:POST"]) posted = self.posted_escalation() @@ -3943,7 +3968,7 @@ def test_changed_reasons_on_an_escalated_comment_keep_the_escalation( def test_a_new_head_rewrites_the_comment_without_a_label(self) -> None: self.earlier_sweep(f"{merge.RISK_MARKER}\nrisk **MED** at head `{'d' * 40}`.") - self.lanes = UNBOUND + self.lanes = PERSON_HELD _, out = self.run_main() self.assertEqual(self.log, ["risk-comment:PATCH"]) self.assertNotIn("riskEscalation", out["unattended"]["writes"]) @@ -3954,7 +3979,7 @@ def test_another_accounts_comment_at_the_head_is_not_an_earlier_sweep( body = self.body() self.setUp() self.earlier_sweep(body, login="maintainer") - self.lanes = UNBOUND + self.lanes = PERSON_HELD self.run_main() self.assertEqual(self.log, ["risk-comment:POST"]) @@ -4012,7 +4037,7 @@ def test_a_refusal_after_a_cleared_comment_restarts_the_time(self) -> None: # MED, then LOW, then MED again with the same reasons: the cleared # comment has stood two hours, but the refusal is new. self.earlier_sweep(merge.risk_cleared_body(HEAD)) - self.lanes = UNBOUND + self.lanes = PERSON_HELD _, out = self.run_main() self.assertEqual(self.log, ["risk-comment:PATCH"]) self.assertNotIn("riskEscalation", out["unattended"]["writes"]) @@ -4022,7 +4047,7 @@ def test_report_only_writes_nothing_after_an_earlier_sweep(self) -> None: self.setUp() self.earlier_sweep(body) self.config = dataclasses.replace(self.config, report_only=True) - self.lanes = UNBOUND + self.lanes = PERSON_HELD _, out = self.run_main() self.assertIs(out["unattended"]["reportOnly"], True) self.capture.assert_not_called() @@ -4042,7 +4067,7 @@ def test_a_changed_assessment_rewrites_the_identitys_comment(self) -> None: "body": f"{merge.RISK_MARKER}\nquoted by a human", }, ] - self.lanes = UNBOUND + self.lanes = PERSON_HELD _, out = self.run_main() self.assertEqual(out["unattended"]["writes"]["riskComment"], "updated") self.assertEqual(self.log, ["risk-comment:PATCH"]) @@ -4297,6 +4322,138 @@ def test_renamed_or_unproven_file_raises(self) -> None: self.read(proc, files) +class HeadFileFailure(unittest.TestCase): + """A head-file read that fails, a rate limit included, is never sent + again in that bind, not for another thread and not for the carried + security pass (review_evidence._carried_blocking).""" + + RATE_LIMITED = subprocess.CompletedProcess( + [], 1, "", "gh: API rate limit exceeded (HTTP 403)" + ) + + def test_a_failed_read_is_not_sent_again(self) -> None: + with mock.patch.object( + merge, "gh_capture", return_value=self.RATE_LIMITED + ) as capture: + read = merge._head_file("owner/repo", HEAD) + for path in ("app.py", "lib.py", "app.py"): + with self.subTest(path), self.assertRaises(RuntimeError): + read(path, "b" * 40) + self.assertEqual(capture.call_count, 1) + + def test_a_path_the_comparison_cannot_prove_latches_nothing(self) -> None: + # old.py 404s and the comparison shows it renamed: a verdict about + # old.py alone. app.py, read next, still reads and its thread, whose + # flagged line is gone at the head, clears. + ev_tests = evidence_tests + bot = {"__typename": "Bot", "login": "someone-else"} + + def outdated(path: str) -> dict[str, Any]: + item = ev_tests.thread( + resolved=True, + resolved_by=bot, + outdated=True, + created="2025-12-31T00:00:00Z", + ) + item["comments"][0]["path"] = path + return item + + def capture(args: list[str], **_: Any) -> Any: + if args[-1].startswith("repos/owner/repo/contents/old.py"): + return subprocess.CompletedProcess( + args, 1, "", "gh: Not Found (HTTP 404)" + ) + return subprocess.CompletedProcess( + args, 0, "context\nrun(clean(user_input))\n", "" + ) + + renamed = {"files": [{"filename": "old.py", "status": "renamed"}]} + + def bind(threads: list[dict[str, Any]]) -> Any: + with ( + mock.patch.object(merge, "gh_capture", side_effect=capture) as read, + mock.patch.object(merge, "gh_json", return_value=renamed), + ): + lanes = ev_tests.by_lane( + merge.review_evidence.bind( + ev_tests.S, + [ + ev_tests.whole("claude-review", ev_tests.S, n=1, k=1), + ev_tests.whole("claude-security-review", ev_tests.S), + ], + ev_tests.both_markers(), + threads, + is_human=ev_tests.is_human, + changed_paths=(), + head_file=merge._head_file("owner/repo", ev_tests.S), + ) + ) + return lanes["claude-review"], [c.args[0][-1] for c in read.call_args_list] + + alone, _ = bind([outdated("old.py")]) + both, reads = bind([outdated("old.py"), outdated("app.py")]) + self.assertTrue(any("contents/app.py" in r for r in reads), reads) + # The app.py thread cleared: the count is old.py's alone. + self.assertEqual(both.open_findings, alone.open_findings) + + def test_the_bind_and_its_carried_pass_send_one_read_after_a_rate_limit( + self, + ) -> None: + # claude-review binds at S and reads the head for its outdated + # thread; the security lane's newest marker names a later head whose + # review failed, so it unbinds and the carried pass judges S again, + # which would read the head for its own outdated thread. + ev_tests = evidence_tests + sec = "claude-security-review" + later = "c2" * 20 + bot = {"__typename": "Bot", "login": "someone-else"} + + def outdated(body: str) -> dict[str, Any]: + return ev_tests.thread( + body, + resolved=True, + resolved_by=bot, + outdated=True, + created="2025-12-31T00:00:00Z", + ) + + failed = ev_tests.run( + sec, + later, + attempts=[ev_tests.attempt(sec, later, 1, [], "failure")], + created="2026-01-02T00:00:00Z", + ) + newer = {**ev_tests.marker(sec, later), "created_at": "2026-01-02T01:00:00Z"} + runs = [ + ev_tests.whole("claude-review", ev_tests.S, n=1, k=1), + ev_tests.whole(sec, ev_tests.S, n=1, k=1), + failed, + ] + threads = [ + outdated("a finding"), + outdated(f"finding\n\nseverity: CRITICAL\n`claude-lane: {sec}`"), + ] + with mock.patch.object( + merge, "gh_capture", return_value=self.RATE_LIMITED + ) as capture: + lanes = ev_tests.by_lane( + merge.review_evidence.bind( + ev_tests.S, + runs, + [*ev_tests.both_markers(), newer], + threads, + is_human=ev_tests.is_human, + changed_paths=(), + head_file=merge._head_file("owner/repo", ev_tests.S), + ) + ) + self.assertTrue(lanes["claude-review"].bound) + self.assertEqual(lanes[sec].unbound_reason, "no-marker") + # The carried pass still ran and carried a blocking count. + self.assertGreater(lanes[sec].blocking_findings or 0, 0) + self.assertEqual(capture.call_count, 1) + + class HeadPushers(unittest.TestCase): """#7339 item 3: who pushed a commit is known only through a linked login.""" diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_review_evidence.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_review_evidence.py index 06cc173c50..6d4c83e772 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_review_evidence.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_review_evidence.py @@ -2060,6 +2060,124 @@ def boom(actor: Any) -> bool: self.assertFalse(result[0].bound) +class CarriedBlockingTests(Base): + """A blocking security finding a bound review left open still holds after + the lane unbinds: the binder carries the count from the newest head of + the pull request the lane's evidence binds at, so the gate's person + routing (babysit_risk.person_reasons) still sees `security-finding`.""" + + SEC = "claude-security-review" + S2 = "c2" * 20 # a newer head + + def critical_at_s(self) -> tuple[list[dict[str, Any]], list[dict[str, Any]]]: + runs = [whole("claude-review", S), whole(self.SEC, S, n=1, k=1)] + body = f"finding\n\nseverity: CRITICAL\n`claude-lane: {self.SEC}`" + return runs, [thread(body)] + + def person(self, lanes: dict[str, Any]) -> list[str]: + result = risk.score( + risk.RiskInput( + files=( + risk.ChangedFile( + "docs/a.md", None, "modified", 1, 1, True, "100644", "100644" + ), + ), + files_complete=True, + max_changed_lines=None, + high_paths=(), + lanes=tuple(lanes[lane] for lane in LANES), + lines_optional=True, + ) + ) + return [r.code for r in risk.person_reasons(risk.holding(result))] + + def test_a_bound_critical_finding_holds_for_a_person(self) -> None: + runs, threads = self.critical_at_s() + bound = self.bind(runs, threads=threads) + self.assertEqual(bound[self.SEC].blocking_findings, 1) + self.assertEqual(self.person(bound), ["security-finding"]) + + def test_a_retargeted_base_carries_the_critical_finding(self) -> None: + runs, threads = self.critical_at_s() + lanes = by_lane( + ev.bind( + S, + runs, + both_markers(), + threads, + is_human=is_human, + head_file=fixed_head, + changed_paths=(), + base_changed=True, + ) + ) + self.assertUnbound(lanes[self.SEC], "base-retargeted") + self.assertEqual(lanes[self.SEC].blocking_findings, 1) + self.assertEqual(self.person(lanes), ["security-finding"]) + + def test_a_new_head_carries_the_critical_finding(self) -> None: + # A push to S2: no review there yet, or one that failed and wrote a + # newer marker. Either way the review at S still leaves it open. + runs, threads = self.critical_at_s() + failed = run( + self.SEC, + self.S2, + attempts=[attempt(self.SEC, self.S2, 1, [], "failure")], + created="2026-01-02T00:00:00Z", + ) + # Later than every marker at S, so it is the lane's newest. + newer = {**marker(self.SEC, self.S2), "created_at": "2026-01-02T01:00:00Z"} + for name, extra_runs, extra_markers in ( + ("no review yet", [], []), + ("a newer marker on a failed review", [failed], [newer]), + ): + with self.subTest(name): + lanes = by_lane( + ev.bind( + self.S2, + [*runs, *extra_runs], + [*both_markers(), *extra_markers], + threads, + is_human=is_human, + head_file=fixed_head, + changed_paths=(), + ) + ) + self.assertFalse(lanes[self.SEC].bound) + self.assertEqual(lanes[self.SEC].blocking_findings, 1) + self.assertEqual(self.person(lanes), ["security-finding"]) + + def test_an_older_marker_naming_a_head_is_read_only_off_the_newest( + self, + ) -> None: + older = marker(self.SEC, S) + newer = {**marker(self.SEC, self.S2), "created_at": "2026-01-02T01:00:00Z"} + comments = [newer, older] + with self.assertRaises(ev.Unbound): + ev._require_marker(self.SEC, S, comments) + self.assertEqual( + ev._require_marker(self.SEC, S, comments, newest_only=False), + older["body"], + ) + + def test_nothing_is_carried_without_a_blocking_finding(self) -> None: + runs = [whole("claude-review", S), whole(self.SEC, S, n=1, k=1)] + body = f"finding\n\nseverity: SUGGESTION\n`claude-lane: {self.SEC}`" + lanes = by_lane( + ev.bind( + self.S2, + runs, + both_markers(), + [thread(body)], + is_human=is_human, + head_file=fixed_head, + changed_paths=(), + ) + ) + self.assertIsNone(lanes[self.SEC].blocking_findings) + self.assertEqual(self.person(lanes), []) + + class SeverityTests(Base): """The security lane's blocking count holds only its CRITICAL findings; a finding of unreadable severity counts as CRITICAL.""" 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 74fb948ffa..a4ee86348e 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 @@ -22,8 +22,10 @@ from babysit_risk import ( ChangedFile, LaneEvidence, + Reason, Risk, RiskInput, + holding, holds, normalize_path, score, @@ -1458,6 +1460,91 @@ def test_an_unread_listing_holds(self): self.assertTrue(self._holds(files_complete=False)) +class PersonReasonsTests(unittest.TestCase): + """Owner rule 2026-10-26: a person clears only a security issue, a trust + or provenance hold, or a cost hold; the lanes clear every other holding + reason by judging again.""" + + def test_person_reasons_are_security_trust_cost_and_high_paths(self): + person = ( + "security-finding", + "trust-gate:trusted-actors", + "cost-review-required", + "high-path:**", + "high-path:**/.gitmodules", + "files-capped", + ) + lane = ( + "evidence-unbound:claude-review", + "evidence-unbound:unknown", + "files-incomplete", + "manifest-review-required", + "lines-over", + ) + reasons = [Reason(code, Risk.HIGH, "d") for code in person + lane] + self.assertEqual( + tuple(r.code for r in babysit_risk.person_reasons(reasons)), person + ) + + def test_an_unbound_review_holds_for_the_lanes_and_a_finding_for_a_person(self): + unbound = score( + _input(lanes=(_lane("claude-review", bound=False), BOTH_LANES_CLEAN[1])) + ) + self.assertTrue(holds(unbound)) + self.assertEqual(babysit_risk.person_reasons(holding(unbound)), []) + finding = score( + _input( + lanes=( + BOTH_LANES_CLEAN[0], + _lane("claude-security-review", open_findings=1), + ) + ) + ) + self.assertEqual( + [r.code for r in babysit_risk.person_reasons(holding(finding))], + ["security-finding"], + ) + + def test_a_carried_security_finding_on_an_unbound_lane_holds_for_a_person(self): + # The binder's unbound shape when an earlier bound review left a + # blocking finding open (babysit_review_evidence._carried_blocking; + # bound through the binder in CarriedBlockingTests there). + lane = LaneEvidence("claude-security-review", False, "no-run", None, False, 2) + result = score(_input(lanes=(BOTH_LANES_CLEAN[0], lane))) + codes = [r.code for r in babysit_risk.person_reasons(holding(result))] + self.assertEqual(codes, ["security-finding"]) + + def test_a_size_reason_holds_nothing_so_no_one_is_needed(self): + # lines-over, files-over and no-patch are MED reasons an unattended + # merge passes (MERGEABLE_MED): they never hold, so they never sit + # unarmed waiting for a lane or a person. + result = score( + _input( + max_changed_lines=10, + max_changed_files=1, + files=( + _file("plugins/foo/a.md", additions=50), + _file("plugins/foo/logo.png", has_patch=False), + ), + ) + ) + codes = {r.code for r in result.reasons} + self.assertTrue({"lines-over", "files-over"} <= codes, codes) + self.assertTrue(any(c.startswith("no-patch:") for c in codes), codes) + self.assertFalse(holds(result), result.reasons) + self.assertEqual(babysit_risk.person_reasons(holding(result)), []) + + def test_a_listing_past_the_files_api_cap_holds_for_a_person(self): + capped = score(_input(files_complete=False, files_capped=True)) + self.assertEqual( + [r.code for r in babysit_risk.person_reasons(holding(capped))], + ["files-capped"], + ) + unread = score(_input(files_complete=False)) + self.assertTrue(holds(unread)) + self.assertEqual(babysit_risk.person_reasons(holding(unread)), []) + + class SecurityBlockingTests(unittest.TestCase): """The security lane holds only on its CRITICAL findings (`blocking_findings`); the rest are reported and do not hold.""" diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_verify_evidence.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_verify_evidence.py index 4be48320d4..c84c87fa1d 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_verify_evidence.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_verify_evidence.py @@ -1456,6 +1456,30 @@ def test_fails_at_three_heads_refuse_despite_a_later_pass(self) -> None: self.assertTrue(evidence.needs_human) self.assertIn("3", evidence.detail or "") + def test_the_cap_fails_closed_when_its_fails_cannot_be_read(self) -> None: + # A listing that does not read refuses rather than letting the pass + # decide, so a short read never lifts the cap. + fake = self.failed_at(EARLIER, IN_PR, UNRELATED) + fake.fail = {"actions/artifacts"} + evidence = read(fake) + self.assertEqual(evidence.refusal, "verify:unreadable") + + def test_its_own_fails_cannot_expire_before_it_is_too_old(self) -> None: + # Artifacts last 90 days here; a pull request is refused for a person + # past TOO_OLD_DAYS, before any of its own FAILs can expire, so an + # expired FAIL never lifts the cap of a pull request still judged. + self.assertLess(ve.TOO_OLD_DAYS, 90) + expired = passing( + *( + artifact(named(sha, "fail"), i, expired=True) + for i, sha in zip((2, 3, 4), (EARLIER, IN_PR, UNRELATED)) + ), + runs={i: judged(i, OLDER_START) for i in (2, 3, 4)}, + ) + evidence = read(expired, pull(created_at="2026-07-27T23:00:00Z")) + self.assertEqual(evidence.refusal, "verify:too-old") + self.assertTrue(evidence.needs_human) + def test_fails_at_two_heads_leave_the_pass_to_decide(self) -> None: self.assertIsNone(read(self.failed_at(EARLIER, IN_PR)).refusal) @@ -1943,7 +1967,7 @@ def test_a_changed_set_refuses(self) -> None: fake.closing = closing evidence = read(fake) self.assertEqual(evidence.refusal, "verify:spec-changed") - self.assertTrue(evidence.needs_human) + self.assertFalse(evidence.needs_human) def refs(self, body: str | None, kind: str = REPO) -> FakeGitHub: """A pass recording issue 7, for a PR that closes none now.""" @@ -2926,13 +2950,13 @@ def test_the_label_is_the_trackers_human_gated_role(self) -> None: self.assertIn("labels[]=status: needs-decision", self.escalation_writes()[-1]) def test_another_label_does_not_stand_in_for_the_role_label(self) -> None: - self.verdicts[unattended.PR] = evidence("verify:spec-changed") + self.verdicts[unattended.PR] = evidence("verify:untrusted-spec") self.pr_labels = [{"name": "enhancement"}] _, out = self.lane() self.assertEqual(out["unattended"]["writes"]["escalation"]["label"], "added") def test_a_rerun_writes_no_second_label_or_comment(self) -> None: - self.verdicts[unattended.PR] = evidence("verify:spec-changed") + self.verdicts[unattended.PR] = evidence("verify:untrusted-spec") self.pr_labels = [{"name": "needs-human"}] self.comments = [ { @@ -2940,7 +2964,7 @@ def test_a_rerun_writes_no_second_label_or_comment(self) -> None: "user": {"login": unattended.IDENTITY, "type": "Bot"}, "created_at": "2026-10-12T10:00:00Z", "body": merge.verify_escalation_body( - "verify:spec-changed", unattended.HEAD + "verify:untrusted-spec", unattended.HEAD ), } ] @@ -2952,7 +2976,7 @@ def test_a_rerun_writes_no_second_label_or_comment(self) -> None: ) # The escalation's own label is a merge hold too; the reason still # names the verifier refusal. - self.assertEqual(out["unattended"]["reason"], "verify:spec-changed") + self.assertEqual(out["unattended"]["reason"], "verify:untrusted-spec") FAIL_RUN = (7, f"pr-verify-{unattended.PR}-{unattended.HEAD}-fail") @@ -4235,6 +4259,19 @@ def test_a_repeated_fail_prints_its_refusal(self) -> None: self.assertEqual(report["refusal"], ve.REPEATED_FAIL) self.assertTrue(report["needsHuman"]) + def test_a_changed_spec_alone_goes_back_to_the_lanes(self) -> None: + # Owner rule 2026-10-26: a person only where no lane can clear the + # refusal. An untrusted spec, the terminal repeated-FAIL cap and an + # old pull request stay a person's; a changed spec is judged again. + for refusal, person in ( + (ve.UNTRUSTED_SPEC, True), + (ve.REPEATED_FAIL, True), + (ve.TOO_OLD, True), + (ve.SPEC_CHANGED, False), + ): + with self.subTest(refusal): + self.assertIs(ve.Evidence(refusal, HEAD, (), 0).needs_human, person) + def test_a_short_head_or_a_zero_cap_is_refused(self) -> None: for argv in ( ["evidence", "--repo", REPO, str(PR), "abc"],