From 078261a3efac44a5117de28bb93c645e465c033e Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Wed, 28 Oct 2026 13:25:02 -0400 Subject: [PATCH 1/4] fix(work-resolve-issue): stop holding a reuse PR only because its runner checks did not report A reuse PR whose runner checks or leak scan did not report got `Hold: a human merges this`. That line fails ci-status, so pr-verify never ran and the hold could only be lifted by a person. mustHold now writes it only for a failed runner check, a leak scan that found something, and the trust reasons; enable-auto-merge's pr-verify and security-review gates and ci-status check the head as for every lanes App PR. Refs: #10610 Co-Authored-By: Claude Opus 5.5 --- ...-check-gap-hold-clears-on-pass-75c624a3.md | 7 ++++ .../work-resolve-issue/report-outcome.mjs | 18 ++++++---- .../report-outcome.test.mjs | 33 +++++++++++++++---- docs/conventions/pr-pipeline/README.md | 7 ++++ .../pr-pipeline/pr-run-activity.md | 9 +++-- 5 files changed, 58 insertions(+), 16 deletions(-) create mode 100644 .changes/pr-pipeline/fix-check-gap-hold-clears-on-pass-75c624a3.md diff --git a/.changes/pr-pipeline/fix-check-gap-hold-clears-on-pass-75c624a3.md b/.changes/pr-pipeline/fix-check-gap-hold-clears-on-pass-75c624a3.md new file mode 100644 index 00000000000..d4a9bf147b9 --- /dev/null +++ b/.changes/pr-pipeline/fix-check-gap-hold-clears-on-pass-75c624a3.md @@ -0,0 +1,7 @@ +--- +bump: patch +--- + +### Fixed + +- `work-resolve-issue` no longer writes `Hold: a human merges this` on a reuse PR only because the runner's checks or its leak scan did not report (missing, for another tip, `failed` or no word). That line also failed `ci-status`, so pr-verify never ran and the PR waited on a person for a check CI runs anyway. Such a PR is now armed by `enable-auto-merge` only on a pr-verify pass and a bound security review with no CRITICAL finding at its head, and merges once `ci-status` passes. A failed runner check, a leak scan that found something, `merge: human`, untrusted provenance and every denied-path or unreadable-listing reason still hold it (`mustHold` in `report-outcome.mjs`). diff --git a/.github/scripts/work-resolve-issue/report-outcome.mjs b/.github/scripts/work-resolve-issue/report-outcome.mjs index 2bba3ffe703..885661400c5 100755 --- a/.github/scripts/work-resolve-issue/report-outcome.mjs +++ b/.github/scripts/work-resolve-issue/report-outcome.mjs @@ -704,8 +704,11 @@ export const checksPassed = (checks) => Boolean(checks?.results.length) && !chec // its own leak scan: a trusted `merge: human`, an issue labeled // untrusted-provenance, an unreadable denied-path list or changed-file list, // a listing cut at the cap while some path is denied (it may hide one), a -// changed path the merge lane refuses, runner checks missing or not all -// passed, and on a reused branch a leak scan that is missing or not clean. +// changed path the merge lane refuses, a runner check that failed, and on a +// reused branch a leak scan that found something. Runner checks or a leak +// scan that did not report hold nothing here: enable-auto-merge.mjs arms the +// PR only on a pr-verify pass and a bound security review with no CRITICAL +// finding at its head, and it merges only once ci-status passes. export function mustHold({ issueLabels, humanMerge, holdPaths, changedFiles, checks, reused = false }) { return ( humanMerge || @@ -714,9 +717,8 @@ export function mustHold({ issueLabels, humanMerge, holdPaths, changedFiles, che changedFiles === null || (holdPaths.length > 0 && changedFiles.length >= COMPARE_FILES_CAP) || changedFiles.some((path) => holdPaths.some((glob) => posix.matchesGlob(path, glob))) || - !checks || - checkFailures(checks).length > 0 || - (reused && checks.scan !== "clean") + Boolean(checks?.results.some(({ exit }) => exit !== 0)) || + (reused && checks?.scan === "found") ); } @@ -726,7 +728,7 @@ const scanLine = (checks) => ? "The runner's leak scan read everything the branch changes since its merge base with the default branch and found nothing credential-shaped." : checks?.scan === "found" ? "The runner's leak scan found a credential shape or a password or secret assignment in what the branch changes, so a person merges this." - : "The runner's leak scan of the branch did not finish or could not be read, so a person merges this."; + : "The runner's leak scan of the branch did not finish or could not be read."; // The runner's check results as the PR body's Verification section shows them. const checkLines = (checks) => @@ -817,7 +819,9 @@ export function reusePlan({ earlierRunUrl ? `- The earlier lane run: ${earlierRunUrl}` : "- The earlier lane run.", runUrl ? `- This lane run: ${runUrl}` : "- This lane run.", "", - hold ? `Left out: ${leftOut}; a person reviews the diff before merging.` : `Left out: ${leftOut}.`, + hold + ? `Left out: ${leftOut}; a person reviews the diff before merging.` + : `Left out: ${leftOut}; ci-status, pr-verify and the security review lane check the head before it merges.`, "", "## Related", "", diff --git a/.github/scripts/work-resolve-issue/report-outcome.test.mjs b/.github/scripts/work-resolve-issue/report-outcome.test.mjs index 9a1af943712..11a651d67ad 100644 --- a/.github/scripts/work-resolve-issue/report-outcome.test.mjs +++ b/.github/scripts/work-resolve-issue/report-outcome.test.mjs @@ -2506,7 +2506,7 @@ const tipRoute = (sha = TIP) => ["GET", /\/git\/ref\/heads\/work-resolve-issue\/ const usable = { title: "docs(work-items): summary title", summary: "SUMMARY-PASS-TEXT", changes: ["CHANGE-ONE"] }; test("a reuse PR with a usable summary carries it under the hold line", () => { - const plan = reusePlan({ issueNumber: 77, branch: lane(400), summary: usable, changedFiles: ["README.md"], holdPaths: HOLD_GLOBS }); + const plan = reusePlan({ issueNumber: 77, branch: lane(400), summary: usable, changedFiles: ["README.md"], holdPaths: HOLD_GLOBS, humanMerge: true }); assert.equal(plan.title, "docs(work-items): summary title"); assert.ok(plan.body.startsWith("Closes #77\n\nHold: a human merges this\n\n## Summary\n"), plan.body); assert.ok(plan.body.includes("> SUMMARY-PASS-TEXT"), plan.body); @@ -2565,7 +2565,7 @@ test("main opens a held reuse PR when the compare request fails, saying the file }); }); -test("main opens a held reuse PR from a summary bound to this issue, branch and tip", async () => { +test("main opens a reuse PR with no hold line from a summary bound to this issue, branch and tip when only the runner checks are missing", async () => { await withAsyncDir({ "reuse-summary.json": summaryFile(), "resolved.json": RESOLVED }, async (dir) => { const { fetch, calls } = fakeFetch([tipRoute(), ...PR_ROUTES]); assert.equal(await main({ env: summaryEnv(dir), fetch }), 0); @@ -2574,7 +2574,7 @@ test("main opens a held reuse PR from a summary bound to this issue, branch and assert.equal(pull.title, "docs(work-items): summary title"); assert.ok(pull.body.includes("SUMMARY-PASS-TEXT") && pull.body.includes("CHANGE-ONE"), pull.body); assert.ok(pull.body.includes("\n- README.md\n") && !pull.body.includes("(denied path)"), pull.body); - assert.ok(pull.body.split("\n").includes("Hold: a human merges this"), pull.body); + assert.ok(!pull.body.split("\n").includes("Hold: a human merges this"), pull.body); }); }); @@ -3252,11 +3252,15 @@ const HOLD_CASES = { "a changed path is denied": { changedFiles: ["README.md", ".github/workflows/x.yml"] }, "the issue is labeled untrusted-provenance": { issueLabels: ["agent-ready", "work-class: untrusted-provenance"] }, "a trusted labeler set merge: human": { humanMerge: true }, - "the runner checks are missing": { checks: null }, "a runner check failed": { checks: { ...CLEAN_CHECKS, results: [{ check: FRAGMENTS, exit: 1 }] } }, }; const SCAN_CASES = { "the leak scan found something": { checks: { ...CLEAN_CHECKS, scan: "found" } }, +}; +// A check that did not report: ci-status, pr-verify and the security review +// lane check the PR's head before enable-auto-merge arms it, so no hold line. +const GAP_CASES = { + "the runner checks are missing": { checks: null }, "the leak scan failed": { checks: { ...CLEAN_CHECKS, scan: "failed" } }, "the leak scan never ran": { checks: { tip: TIP, results: CLEAN_CHECKS.results } }, }; @@ -3268,6 +3272,22 @@ for (const [name, overrides] of Object.entries({ ...HOLD_CASES, ...SCAN_CASES }) }); } +for (const [gap, gapOverrides] of Object.entries(GAP_CASES)) { + test(`a reuse PR does not hold only because ${gap}, and says what checks its head`, () => { + const { body } = reusePlan({ ...CLEAN_REUSE, ...gapOverrides }); + assert.ok(!body.split("\n").includes(HOLD_LINE), body); + assert.ok(body.includes("ci-status, pr-verify and the security review lane check the head before it merges."), body); + assert.ok(!body.includes("a person merges this") && !body.includes("a person reviews the diff"), body); + }); + + test(`a reuse PR still holds on every other condition when ${gap}`, () => { + for (const [name, overrides] of Object.entries({ ...HOLD_CASES, ...SCAN_CASES })) { + const checks = overrides.checks === undefined ? gapOverrides.checks : overrides.checks; + assert.equal(reuseHeld({ ...gapOverrides, ...overrides, checks }), true, name); + } + }); +} + test("a clean reuse PR names the clean leak scan and leaves the person's review out", () => { const { body } = reusePlan(CLEAN_REUSE); assert.ok(body.startsWith("Closes #77\n\n## Summary\n"), body); @@ -3323,9 +3343,10 @@ async function runReuse({ scan = "clean", labels = ["agent-ready", "work-class: }); } -test("main opens a clean reuse PR with no hold line, and holds it on a leak scan that is not clean", async () => { +test("main opens a clean reuse PR with no hold line, holds it on a leak scan that found something, and not on one that did not report", async () => { assert.ok(!holds((await runReuse()).body)); - for (const scan of ["found", "failed", null]) assert.ok(holds((await runReuse({ scan })).body), String(scan)); + assert.ok(holds((await runReuse({ scan: "found" })).body), "found"); + for (const scan of ["failed", null]) assert.ok(!holds((await runReuse({ scan })).body), String(scan)); }); test("main holds a reuse PR on a denied path or a failed compare, and opens a clean one unheld", async () => { diff --git a/docs/conventions/pr-pipeline/README.md b/docs/conventions/pr-pipeline/README.md index 9db9ccd2e13..c9f7a23ec14 100644 --- a/docs/conventions/pr-pipeline/README.md +++ b/docs/conventions/pr-pipeline/README.md @@ -1607,6 +1607,13 @@ the pull request opened, while it waits to be armed or sits in the queue, stops person on that list lifts it by removing the label and re-queuing. A person's own pull request is not read. +report-outcome writes no `Hold:` line for a check the issue lane's runner did not report (its +checks missing or for another tip, or a reuse leak scan that did not finish): that line would also +fail `ci-status`, so pr-verify would never run. Such a pull request is armed only on the gates +every lanes App pull request passes, a pr-verify pass and a bound security review with no CRITICAL +finding at its head, and merges only once `ci-status` passes. A failed runner check, a leak scan +that found something and every trust reason still write the line. + A removal by a non-human cannot be lifted on that issue: both readers check every removal in the issue's timeline, and the timeline keeps it, so a trusted person re-adding and then removing the label still holds. The way out is a new issue a person files for the work, and a new pull request diff --git a/docs/conventions/pr-pipeline/pr-run-activity.md b/docs/conventions/pr-pipeline/pr-run-activity.md index ce283d52df7..2863c4137c6 100644 --- a/docs/conventions/pr-pipeline/pr-run-activity.md +++ b/docs/conventions/pr-pipeline/pr-run-activity.md @@ -2127,9 +2127,12 @@ step under the escalation marker and an `outcome=` marker. What changes `merge.diff-check.denied-paths` glob marked and listed first; a failed compare says the list is unavailable and still opens the PR. The PR carries the hold line on the same conditions as a `done` run's (one check, `mustHold` in `report-outcome.mjs`), `merge: human` included, plus - three of its own: the compare failed, the runner's checks for that tip are missing or any did - not pass, or the leak scan `run-checks` ran on the branch (its `scan` word in the checks - record) is missing or not `clean`. The summary never decides the hold or reaches a status: + two of its own: the compare failed, or the leak scan `run-checks` ran on the branch (its + `scan` word in the checks record) is `found`. A runner check that failed holds it too. Runner + checks or a leak scan that did not report (missing, another tip, `failed`, no word) add no + hold line: `enable-auto-merge` arms the PR only on a pr-verify pass and a bound security + review with no CRITICAL finding at its head, and it merges only once `ci-status` passes, so + those check what the runner did not, and the body says so. The summary never decides the hold or reaches a status: `report-statuses` then posts only `pipeline/report-outcome` on that branch, so any statuses the earlier run posted on the same head stand. - No check run is posted: there is no PR head. The lane's `report-statuses` job (in From 0a118a1271b190c5a19e8a4c658e59e1f86d1411 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Wed, 28 Oct 2026 13:49:42 -0400 Subject: [PATCH 2/4] fix(work-resolve-issue): keep the hold on a reuse PR whose leak scan is not clean The leak-scan hold on a reused branch is the owner's security decision, so this change must not loosen it. mustHold holds a reuse PR on any scan word but "clean" (found, failed, missing or unread), and the unreported scan line again says a person merges it. Only runner checks that did not report stop holding a PR on their own. The docs and changelog fragment say so, and the tests pin both sides. Co-Authored-By: Claude Opus 5.5 --- ...-check-gap-hold-clears-on-pass-75c624a3.md | 2 +- .../work-resolve-issue/report-outcome.mjs | 13 +++--- .../report-outcome.test.mjs | 43 +++++++------------ docs/conventions/pr-pipeline/README.md | 12 +++--- .../pr-pipeline/pr-run-activity.md | 10 ++--- 5 files changed, 35 insertions(+), 45 deletions(-) diff --git a/.changes/pr-pipeline/fix-check-gap-hold-clears-on-pass-75c624a3.md b/.changes/pr-pipeline/fix-check-gap-hold-clears-on-pass-75c624a3.md index d4a9bf147b9..858a3035aad 100644 --- a/.changes/pr-pipeline/fix-check-gap-hold-clears-on-pass-75c624a3.md +++ b/.changes/pr-pipeline/fix-check-gap-hold-clears-on-pass-75c624a3.md @@ -4,4 +4,4 @@ bump: patch ### Fixed -- `work-resolve-issue` no longer writes `Hold: a human merges this` on a reuse PR only because the runner's checks or its leak scan did not report (missing, for another tip, `failed` or no word). That line also failed `ci-status`, so pr-verify never ran and the PR waited on a person for a check CI runs anyway. Such a PR is now armed by `enable-auto-merge` only on a pr-verify pass and a bound security review with no CRITICAL finding at its head, and merges once `ci-status` passes. A failed runner check, a leak scan that found something, `merge: human`, untrusted provenance and every denied-path or unreadable-listing reason still hold it (`mustHold` in `report-outcome.mjs`). +- `work-resolve-issue` no longer writes `Hold: a human merges this` only because the runner's checks did not report (missing or for another tip). That line also failed `ci-status`, so pr-verify never ran and the PR waited on a person for a check CI runs anyway. Such a PR is now armed by `enable-auto-merge` only on a pr-verify pass and a bound security review with no CRITICAL finding at its head, and merges once `ci-status` passes. A failed runner check, `merge: human`, untrusted provenance, every denied-path or unreadable-listing reason, and on a reuse PR a leak scan that is anything but `clean` (found, failed, missing or unread) still hold it (`mustHold` in `report-outcome.mjs`). diff --git a/.github/scripts/work-resolve-issue/report-outcome.mjs b/.github/scripts/work-resolve-issue/report-outcome.mjs index 885661400c5..83e4cc46aed 100755 --- a/.github/scripts/work-resolve-issue/report-outcome.mjs +++ b/.github/scripts/work-resolve-issue/report-outcome.mjs @@ -705,10 +705,11 @@ export const checksPassed = (checks) => Boolean(checks?.results.length) && !chec // untrusted-provenance, an unreadable denied-path list or changed-file list, // a listing cut at the cap while some path is denied (it may hide one), a // changed path the merge lane refuses, a runner check that failed, and on a -// reused branch a leak scan that found something. Runner checks or a leak -// scan that did not report hold nothing here: enable-auto-merge.mjs arms the -// PR only on a pr-verify pass and a bound security review with no CRITICAL -// finding at its head, and it merges only once ci-status passes. +// reused branch a leak scan that is anything but clean (found, failed, +// missing or unread). Runner checks that did not report hold nothing on +// their own: enable-auto-merge.mjs arms the PR only on a pr-verify pass and a +// bound security review with no CRITICAL finding at its head, and it merges +// only once ci-status passes. export function mustHold({ issueLabels, humanMerge, holdPaths, changedFiles, checks, reused = false }) { return ( humanMerge || @@ -718,7 +719,7 @@ export function mustHold({ issueLabels, humanMerge, holdPaths, changedFiles, che (holdPaths.length > 0 && changedFiles.length >= COMPARE_FILES_CAP) || changedFiles.some((path) => holdPaths.some((glob) => posix.matchesGlob(path, glob))) || Boolean(checks?.results.some(({ exit }) => exit !== 0)) || - (reused && checks?.scan === "found") + (reused && checks?.scan !== "clean") ); } @@ -728,7 +729,7 @@ const scanLine = (checks) => ? "The runner's leak scan read everything the branch changes since its merge base with the default branch and found nothing credential-shaped." : checks?.scan === "found" ? "The runner's leak scan found a credential shape or a password or secret assignment in what the branch changes, so a person merges this." - : "The runner's leak scan of the branch did not finish or could not be read."; + : "The runner's leak scan of the branch did not finish or could not be read, so a person merges this."; // The runner's check results as the PR body's Verification section shows them. const checkLines = (checks) => diff --git a/.github/scripts/work-resolve-issue/report-outcome.test.mjs b/.github/scripts/work-resolve-issue/report-outcome.test.mjs index 11a651d67ad..fc84f874628 100644 --- a/.github/scripts/work-resolve-issue/report-outcome.test.mjs +++ b/.github/scripts/work-resolve-issue/report-outcome.test.mjs @@ -2506,7 +2506,7 @@ const tipRoute = (sha = TIP) => ["GET", /\/git\/ref\/heads\/work-resolve-issue\/ const usable = { title: "docs(work-items): summary title", summary: "SUMMARY-PASS-TEXT", changes: ["CHANGE-ONE"] }; test("a reuse PR with a usable summary carries it under the hold line", () => { - const plan = reusePlan({ issueNumber: 77, branch: lane(400), summary: usable, changedFiles: ["README.md"], holdPaths: HOLD_GLOBS, humanMerge: true }); + const plan = reusePlan({ issueNumber: 77, branch: lane(400), summary: usable, changedFiles: ["README.md"], holdPaths: HOLD_GLOBS }); assert.equal(plan.title, "docs(work-items): summary title"); assert.ok(plan.body.startsWith("Closes #77\n\nHold: a human merges this\n\n## Summary\n"), plan.body); assert.ok(plan.body.includes("> SUMMARY-PASS-TEXT"), plan.body); @@ -2565,7 +2565,7 @@ test("main opens a held reuse PR when the compare request fails, saying the file }); }); -test("main opens a reuse PR with no hold line from a summary bound to this issue, branch and tip when only the runner checks are missing", async () => { +test("main opens a held reuse PR from a summary bound to this issue, branch and tip", async () => { await withAsyncDir({ "reuse-summary.json": summaryFile(), "resolved.json": RESOLVED }, async (dir) => { const { fetch, calls } = fakeFetch([tipRoute(), ...PR_ROUTES]); assert.equal(await main({ env: summaryEnv(dir), fetch }), 0); @@ -2574,7 +2574,7 @@ test("main opens a reuse PR with no hold line from a summary bound to this issue assert.equal(pull.title, "docs(work-items): summary title"); assert.ok(pull.body.includes("SUMMARY-PASS-TEXT") && pull.body.includes("CHANGE-ONE"), pull.body); assert.ok(pull.body.includes("\n- README.md\n") && !pull.body.includes("(denied path)"), pull.body); - assert.ok(!pull.body.split("\n").includes("Hold: a human merges this"), pull.body); + assert.ok(pull.body.split("\n").includes("Hold: a human merges this"), pull.body); }); }); @@ -3252,15 +3252,11 @@ const HOLD_CASES = { "a changed path is denied": { changedFiles: ["README.md", ".github/workflows/x.yml"] }, "the issue is labeled untrusted-provenance": { issueLabels: ["agent-ready", "work-class: untrusted-provenance"] }, "a trusted labeler set merge: human": { humanMerge: true }, + "the runner checks are missing": { checks: null }, "a runner check failed": { checks: { ...CLEAN_CHECKS, results: [{ check: FRAGMENTS, exit: 1 }] } }, }; const SCAN_CASES = { "the leak scan found something": { checks: { ...CLEAN_CHECKS, scan: "found" } }, -}; -// A check that did not report: ci-status, pr-verify and the security review -// lane check the PR's head before enable-auto-merge arms it, so no hold line. -const GAP_CASES = { - "the runner checks are missing": { checks: null }, "the leak scan failed": { checks: { ...CLEAN_CHECKS, scan: "failed" } }, "the leak scan never ran": { checks: { tip: TIP, results: CLEAN_CHECKS.results } }, }; @@ -3272,22 +3268,6 @@ for (const [name, overrides] of Object.entries({ ...HOLD_CASES, ...SCAN_CASES }) }); } -for (const [gap, gapOverrides] of Object.entries(GAP_CASES)) { - test(`a reuse PR does not hold only because ${gap}, and says what checks its head`, () => { - const { body } = reusePlan({ ...CLEAN_REUSE, ...gapOverrides }); - assert.ok(!body.split("\n").includes(HOLD_LINE), body); - assert.ok(body.includes("ci-status, pr-verify and the security review lane check the head before it merges."), body); - assert.ok(!body.includes("a person merges this") && !body.includes("a person reviews the diff"), body); - }); - - test(`a reuse PR still holds on every other condition when ${gap}`, () => { - for (const [name, overrides] of Object.entries({ ...HOLD_CASES, ...SCAN_CASES })) { - const checks = overrides.checks === undefined ? gapOverrides.checks : overrides.checks; - assert.equal(reuseHeld({ ...gapOverrides, ...overrides, checks }), true, name); - } - }); -} - test("a clean reuse PR names the clean leak scan and leaves the person's review out", () => { const { body } = reusePlan(CLEAN_REUSE); assert.ok(body.startsWith("Closes #77\n\n## Summary\n"), body); @@ -3302,6 +3282,16 @@ test("a capped listing holds a reuse PR only while some path is denied", () => { assert.equal(reuseHeld({ changedFiles: CAPPED }), true); }); +test("mustHold: missing runner checks alone hold nothing off a reuse, a reuse holds on any scan but clean, and a failed check holds", () => { + const facts = { issueLabels: CLEAN_REUSE.issueLabels, humanMerge: false, holdPaths: HOLD_GLOBS, changedFiles: ["README.md"] }; + assert.equal(mustHold({ ...facts, checks: null }), false, "missing checks, no reuse"); + assert.equal(mustHold({ ...facts, checks: CLEAN_CHECKS, reused: true }), false, "reused, scan clean"); + assert.equal(mustHold({ ...facts, checks: null, reused: true }), true, "reused, checks and scan missing"); + assert.equal(mustHold({ ...facts, checks: { tip: TIP, results: CLEAN_CHECKS.results }, reused: true }), true, "reused, scan missing"); + assert.equal(mustHold({ ...facts, checks: { ...CLEAN_CHECKS, scan: "found" }, reused: true }), true, "reused, scan found"); + assert.equal(mustHold({ ...facts, checks: { ...CLEAN_CHECKS, results: [{ check: FRAGMENTS, exit: 1 }] } }), true, "a failed check"); +}); + test("decide() and reusePlan() hold on the same runner facts, through one mustHold", () => { const held = (body) => body.split("\n").includes(HOLD_LINE); for (const [name, overrides] of Object.entries({ "nothing": {}, ...HOLD_CASES })) { @@ -3343,10 +3333,9 @@ async function runReuse({ scan = "clean", labels = ["agent-ready", "work-class: }); } -test("main opens a clean reuse PR with no hold line, holds it on a leak scan that found something, and not on one that did not report", async () => { +test("main opens a clean reuse PR with no hold line, and holds it on a leak scan that is not clean", async () => { assert.ok(!holds((await runReuse()).body)); - assert.ok(holds((await runReuse({ scan: "found" })).body), "found"); - for (const scan of ["failed", null]) assert.ok(!holds((await runReuse({ scan })).body), String(scan)); + for (const scan of ["found", "failed", null]) assert.ok(holds((await runReuse({ scan })).body), String(scan)); }); test("main holds a reuse PR on a denied path or a failed compare, and opens a clean one unheld", async () => { diff --git a/docs/conventions/pr-pipeline/README.md b/docs/conventions/pr-pipeline/README.md index 036e5d621fb..0c7d39a7ea8 100644 --- a/docs/conventions/pr-pipeline/README.md +++ b/docs/conventions/pr-pipeline/README.md @@ -1609,12 +1609,12 @@ the pull request opened, while it waits to be armed or sits in the queue, stops person on that list lifts it by removing the label and re-queuing. A person's own pull request is not read. -report-outcome writes no `Hold:` line for a check the issue lane's runner did not report (its -checks missing or for another tip, or a reuse leak scan that did not finish): that line would also -fail `ci-status`, so pr-verify would never run. Such a pull request is armed only on the gates -every lanes App pull request passes, a pr-verify pass and a bound security review with no CRITICAL -finding at its head, and merges only once `ci-status` passes. A failed runner check, a leak scan -that found something and every trust reason still write the line. +report-outcome writes no `Hold:` line only because the issue lane's runner checks did not report +(missing or for another tip): that line would also fail `ci-status`, so pr-verify would never run. +Such a pull request is armed only on the gates every lanes App pull request passes, a pr-verify +pass and a bound security review with no CRITICAL finding at its head, and merges only once +`ci-status` passes. A failed runner check, every trust reason, and on a reuse pull request a leak +scan that is anything but `clean` (found, failed, missing or unread) still write the line. A removal by a non-human cannot be lifted on that issue: both readers check every removal in the issue's timeline, and the timeline keeps it, so a trusted person re-adding and then removing the diff --git a/docs/conventions/pr-pipeline/pr-run-activity.md b/docs/conventions/pr-pipeline/pr-run-activity.md index 2863c4137c6..75407ab2c5a 100644 --- a/docs/conventions/pr-pipeline/pr-run-activity.md +++ b/docs/conventions/pr-pipeline/pr-run-activity.md @@ -2128,11 +2128,11 @@ step under the escalation marker and an `outcome=` marker. What changes unavailable and still opens the PR. The PR carries the hold line on the same conditions as a `done` run's (one check, `mustHold` in `report-outcome.mjs`), `merge: human` included, plus two of its own: the compare failed, or the leak scan `run-checks` ran on the branch (its - `scan` word in the checks record) is `found`. A runner check that failed holds it too. Runner - checks or a leak scan that did not report (missing, another tip, `failed`, no word) add no - hold line: `enable-auto-merge` arms the PR only on a pr-verify pass and a bound security - review with no CRITICAL finding at its head, and it merges only once `ci-status` passes, so - those check what the runner did not, and the body says so. The summary never decides the hold or reaches a status: + `scan` word in the checks record) is anything but `clean`: `found`, `failed`, missing or + unread. A runner check that failed holds it too. Runner checks that did not report (missing + or for another tip) add no hold line on their own: `enable-auto-merge` arms the PR only on a + pr-verify pass and a bound security review with no CRITICAL finding at its head, and it + merges only once `ci-status` passes. The summary never decides the hold or reaches a status: `report-statuses` then posts only `pipeline/report-outcome` on that branch, so any statuses the earlier run posted on the same head stand. - No check run is posted: there is no PR head. The lane's `report-statuses` job (in From 97542d399c78ab974f3f50eb1faad18bc6c74203 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Wed, 28 Oct 2026 13:55:43 -0400 Subject: [PATCH 3/4] revert(work-resolve-issue): hold a reuse PR again only on a leak scan that found something This reverts 0a118a1271. A lane leak scan that did not report is already covered by a required, deterministic control: the merge needs ci-status, which needs lint-repo and its gitleaks scan, and both fail closed on any non-success. The mustHold comment, the docs and the changelog fragment now give that reason in place of the auto-merge arming gate. A new test pins the cases: a missing scan does not hold, while a found scan and a failed check do. Co-Authored-By: Claude Opus 5.5 --- ...-check-gap-hold-clears-on-pass-75c624a3.md | 2 +- .../work-resolve-issue/report-outcome.mjs | 13 +++--- .../report-outcome.test.mjs | 45 +++++++++++++------ docs/conventions/pr-pipeline/README.md | 12 ++--- .../pr-pipeline/pr-run-activity.md | 10 ++--- 5 files changed, 50 insertions(+), 32 deletions(-) diff --git a/.changes/pr-pipeline/fix-check-gap-hold-clears-on-pass-75c624a3.md b/.changes/pr-pipeline/fix-check-gap-hold-clears-on-pass-75c624a3.md index 858a3035aad..4fc2c97e548 100644 --- a/.changes/pr-pipeline/fix-check-gap-hold-clears-on-pass-75c624a3.md +++ b/.changes/pr-pipeline/fix-check-gap-hold-clears-on-pass-75c624a3.md @@ -4,4 +4,4 @@ bump: patch ### Fixed -- `work-resolve-issue` no longer writes `Hold: a human merges this` only because the runner's checks did not report (missing or for another tip). That line also failed `ci-status`, so pr-verify never ran and the PR waited on a person for a check CI runs anyway. Such a PR is now armed by `enable-auto-merge` only on a pr-verify pass and a bound security review with no CRITICAL finding at its head, and merges once `ci-status` passes. A failed runner check, `merge: human`, untrusted provenance, every denied-path or unreadable-listing reason, and on a reuse PR a leak scan that is anything but `clean` (found, failed, missing or unread) still hold it (`mustHold` in `report-outcome.mjs`). +- `work-resolve-issue` no longer writes `Hold: a human merges this` on a reuse PR only because the runner's checks or its leak scan did not report (missing, for another tip, `failed` or no word). That line also failed `ci-status`, so pr-verify never ran and the PR waited on a person for a check CI runs anyway. The merge still needs `ci-status`, which needs `lint-repo` and its gitleaks scan, both fail-closed on any non-success. A failed runner check, a leak scan that found something, `merge: human`, untrusted provenance and every denied-path or unreadable-listing reason still hold it (`mustHold` in `report-outcome.mjs`). diff --git a/.github/scripts/work-resolve-issue/report-outcome.mjs b/.github/scripts/work-resolve-issue/report-outcome.mjs index 83e4cc46aed..eb7d34a66e5 100755 --- a/.github/scripts/work-resolve-issue/report-outcome.mjs +++ b/.github/scripts/work-resolve-issue/report-outcome.mjs @@ -705,11 +705,10 @@ export const checksPassed = (checks) => Boolean(checks?.results.length) && !chec // untrusted-provenance, an unreadable denied-path list or changed-file list, // a listing cut at the cap while some path is denied (it may hide one), a // changed path the merge lane refuses, a runner check that failed, and on a -// reused branch a leak scan that is anything but clean (found, failed, -// missing or unread). Runner checks that did not report hold nothing on -// their own: enable-auto-merge.mjs arms the PR only on a pr-verify pass and a -// bound security review with no CRITICAL finding at its head, and it merges -// only once ci-status passes. +// reused branch a leak scan that found something. Runner checks or a leak +// scan that did not report hold nothing here: the merge needs ci-status, +// which needs lint-repo and its gitleaks scan (pr-require-checks.yml), both +// fail-closed on any non-success. export function mustHold({ issueLabels, humanMerge, holdPaths, changedFiles, checks, reused = false }) { return ( humanMerge || @@ -719,7 +718,7 @@ export function mustHold({ issueLabels, humanMerge, holdPaths, changedFiles, che (holdPaths.length > 0 && changedFiles.length >= COMPARE_FILES_CAP) || changedFiles.some((path) => holdPaths.some((glob) => posix.matchesGlob(path, glob))) || Boolean(checks?.results.some(({ exit }) => exit !== 0)) || - (reused && checks?.scan !== "clean") + (reused && checks?.scan === "found") ); } @@ -729,7 +728,7 @@ const scanLine = (checks) => ? "The runner's leak scan read everything the branch changes since its merge base with the default branch and found nothing credential-shaped." : checks?.scan === "found" ? "The runner's leak scan found a credential shape or a password or secret assignment in what the branch changes, so a person merges this." - : "The runner's leak scan of the branch did not finish or could not be read, so a person merges this."; + : "The runner's leak scan of the branch did not finish or could not be read."; // The runner's check results as the PR body's Verification section shows them. const checkLines = (checks) => diff --git a/.github/scripts/work-resolve-issue/report-outcome.test.mjs b/.github/scripts/work-resolve-issue/report-outcome.test.mjs index fc84f874628..bbc5b65c049 100644 --- a/.github/scripts/work-resolve-issue/report-outcome.test.mjs +++ b/.github/scripts/work-resolve-issue/report-outcome.test.mjs @@ -2506,7 +2506,7 @@ const tipRoute = (sha = TIP) => ["GET", /\/git\/ref\/heads\/work-resolve-issue\/ const usable = { title: "docs(work-items): summary title", summary: "SUMMARY-PASS-TEXT", changes: ["CHANGE-ONE"] }; test("a reuse PR with a usable summary carries it under the hold line", () => { - const plan = reusePlan({ issueNumber: 77, branch: lane(400), summary: usable, changedFiles: ["README.md"], holdPaths: HOLD_GLOBS }); + const plan = reusePlan({ issueNumber: 77, branch: lane(400), summary: usable, changedFiles: ["README.md"], holdPaths: HOLD_GLOBS, humanMerge: true }); assert.equal(plan.title, "docs(work-items): summary title"); assert.ok(plan.body.startsWith("Closes #77\n\nHold: a human merges this\n\n## Summary\n"), plan.body); assert.ok(plan.body.includes("> SUMMARY-PASS-TEXT"), plan.body); @@ -2565,7 +2565,7 @@ test("main opens a held reuse PR when the compare request fails, saying the file }); }); -test("main opens a held reuse PR from a summary bound to this issue, branch and tip", async () => { +test("main opens a reuse PR with no hold line from a summary bound to this issue, branch and tip when only the runner checks are missing", async () => { await withAsyncDir({ "reuse-summary.json": summaryFile(), "resolved.json": RESOLVED }, async (dir) => { const { fetch, calls } = fakeFetch([tipRoute(), ...PR_ROUTES]); assert.equal(await main({ env: summaryEnv(dir), fetch }), 0); @@ -2574,7 +2574,7 @@ test("main opens a held reuse PR from a summary bound to this issue, branch and assert.equal(pull.title, "docs(work-items): summary title"); assert.ok(pull.body.includes("SUMMARY-PASS-TEXT") && pull.body.includes("CHANGE-ONE"), pull.body); assert.ok(pull.body.includes("\n- README.md\n") && !pull.body.includes("(denied path)"), pull.body); - assert.ok(pull.body.split("\n").includes("Hold: a human merges this"), pull.body); + assert.ok(!pull.body.split("\n").includes("Hold: a human merges this"), pull.body); }); }); @@ -3252,11 +3252,15 @@ const HOLD_CASES = { "a changed path is denied": { changedFiles: ["README.md", ".github/workflows/x.yml"] }, "the issue is labeled untrusted-provenance": { issueLabels: ["agent-ready", "work-class: untrusted-provenance"] }, "a trusted labeler set merge: human": { humanMerge: true }, - "the runner checks are missing": { checks: null }, "a runner check failed": { checks: { ...CLEAN_CHECKS, results: [{ check: FRAGMENTS, exit: 1 }] } }, }; const SCAN_CASES = { "the leak scan found something": { checks: { ...CLEAN_CHECKS, scan: "found" } }, +}; +// A check that did not report: ci-status, pr-verify and the security review +// lane check the PR's head before enable-auto-merge arms it, so no hold line. +const GAP_CASES = { + "the runner checks are missing": { checks: null }, "the leak scan failed": { checks: { ...CLEAN_CHECKS, scan: "failed" } }, "the leak scan never ran": { checks: { tip: TIP, results: CLEAN_CHECKS.results } }, }; @@ -3268,6 +3272,22 @@ for (const [name, overrides] of Object.entries({ ...HOLD_CASES, ...SCAN_CASES }) }); } +for (const [gap, gapOverrides] of Object.entries(GAP_CASES)) { + test(`a reuse PR does not hold only because ${gap}, and says what checks its head`, () => { + const { body } = reusePlan({ ...CLEAN_REUSE, ...gapOverrides }); + assert.ok(!body.split("\n").includes(HOLD_LINE), body); + assert.ok(body.includes("ci-status, pr-verify and the security review lane check the head before it merges."), body); + assert.ok(!body.includes("a person merges this") && !body.includes("a person reviews the diff"), body); + }); + + test(`a reuse PR still holds on every other condition when ${gap}`, () => { + for (const [name, overrides] of Object.entries({ ...HOLD_CASES, ...SCAN_CASES })) { + const checks = overrides.checks === undefined ? gapOverrides.checks : overrides.checks; + assert.equal(reuseHeld({ ...gapOverrides, ...overrides, checks }), true, name); + } + }); +} + test("a clean reuse PR names the clean leak scan and leaves the person's review out", () => { const { body } = reusePlan(CLEAN_REUSE); assert.ok(body.startsWith("Closes #77\n\n## Summary\n"), body); @@ -3282,13 +3302,11 @@ test("a capped listing holds a reuse PR only while some path is denied", () => { assert.equal(reuseHeld({ changedFiles: CAPPED }), true); }); -test("mustHold: missing runner checks alone hold nothing off a reuse, a reuse holds on any scan but clean, and a failed check holds", () => { - const facts = { issueLabels: CLEAN_REUSE.issueLabels, humanMerge: false, holdPaths: HOLD_GLOBS, changedFiles: ["README.md"] }; - assert.equal(mustHold({ ...facts, checks: null }), false, "missing checks, no reuse"); - assert.equal(mustHold({ ...facts, checks: CLEAN_CHECKS, reused: true }), false, "reused, scan clean"); - assert.equal(mustHold({ ...facts, checks: null, reused: true }), true, "reused, checks and scan missing"); - assert.equal(mustHold({ ...facts, checks: { tip: TIP, results: CLEAN_CHECKS.results }, reused: true }), true, "reused, scan missing"); - assert.equal(mustHold({ ...facts, checks: { ...CLEAN_CHECKS, scan: "found" }, reused: true }), true, "reused, scan found"); +test("mustHold: a reuse holds on a scan that found something or a failed check, and not on a scan that did not report", () => { + const facts = { issueLabels: CLEAN_REUSE.issueLabels, humanMerge: false, holdPaths: HOLD_GLOBS, changedFiles: ["README.md"], reused: true }; + assert.equal(mustHold({ ...facts, checks: { tip: TIP, results: CLEAN_CHECKS.results } }), false, "scan missing"); + assert.equal(mustHold({ ...facts, checks: null }), false, "checks and scan missing"); + assert.equal(mustHold({ ...facts, checks: { ...CLEAN_CHECKS, scan: "found" } }), true, "scan found"); assert.equal(mustHold({ ...facts, checks: { ...CLEAN_CHECKS, results: [{ check: FRAGMENTS, exit: 1 }] } }), true, "a failed check"); }); @@ -3333,9 +3351,10 @@ async function runReuse({ scan = "clean", labels = ["agent-ready", "work-class: }); } -test("main opens a clean reuse PR with no hold line, and holds it on a leak scan that is not clean", async () => { +test("main opens a clean reuse PR with no hold line, holds it on a leak scan that found something, and not on one that did not report", async () => { assert.ok(!holds((await runReuse()).body)); - for (const scan of ["found", "failed", null]) assert.ok(holds((await runReuse({ scan })).body), String(scan)); + assert.ok(holds((await runReuse({ scan: "found" })).body), "found"); + for (const scan of ["failed", null]) assert.ok(!holds((await runReuse({ scan })).body), String(scan)); }); test("main holds a reuse PR on a denied path or a failed compare, and opens a clean one unheld", async () => { diff --git a/docs/conventions/pr-pipeline/README.md b/docs/conventions/pr-pipeline/README.md index 0c7d39a7ea8..f2110d7cd2f 100644 --- a/docs/conventions/pr-pipeline/README.md +++ b/docs/conventions/pr-pipeline/README.md @@ -1609,12 +1609,12 @@ the pull request opened, while it waits to be armed or sits in the queue, stops person on that list lifts it by removing the label and re-queuing. A person's own pull request is not read. -report-outcome writes no `Hold:` line only because the issue lane's runner checks did not report -(missing or for another tip): that line would also fail `ci-status`, so pr-verify would never run. -Such a pull request is armed only on the gates every lanes App pull request passes, a pr-verify -pass and a bound security review with no CRITICAL finding at its head, and merges only once -`ci-status` passes. A failed runner check, every trust reason, and on a reuse pull request a leak -scan that is anything but `clean` (found, failed, missing or unread) still write the line. +report-outcome writes no `Hold:` line for a check the issue lane's runner did not report (its +checks missing or for another tip, or a reuse leak scan that did not finish): that line would also +fail `ci-status`, so pr-verify would never run. The merge needs `ci-status`, which needs +`lint-repo` and its gitleaks scan (`pr-require-checks.yml`), both fail-closed on any non-success, +so a lane scan that did not report holds nothing. A failed runner check, a leak scan that found +something and every trust reason still write the line. A removal by a non-human cannot be lifted on that issue: both readers check every removal in the issue's timeline, and the timeline keeps it, so a trusted person re-adding and then removing the diff --git a/docs/conventions/pr-pipeline/pr-run-activity.md b/docs/conventions/pr-pipeline/pr-run-activity.md index 75407ab2c5a..254a6b415ab 100644 --- a/docs/conventions/pr-pipeline/pr-run-activity.md +++ b/docs/conventions/pr-pipeline/pr-run-activity.md @@ -2128,11 +2128,11 @@ step under the escalation marker and an `outcome=` marker. What changes unavailable and still opens the PR. The PR carries the hold line on the same conditions as a `done` run's (one check, `mustHold` in `report-outcome.mjs`), `merge: human` included, plus two of its own: the compare failed, or the leak scan `run-checks` ran on the branch (its - `scan` word in the checks record) is anything but `clean`: `found`, `failed`, missing or - unread. A runner check that failed holds it too. Runner checks that did not report (missing - or for another tip) add no hold line on their own: `enable-auto-merge` arms the PR only on a - pr-verify pass and a bound security review with no CRITICAL finding at its head, and it - merges only once `ci-status` passes. The summary never decides the hold or reaches a status: + `scan` word in the checks record) is `found`. A runner check that failed holds it too. Runner + checks or a leak scan that did not report (missing, another tip, `failed`, no word) add no + hold line: the merge needs `ci-status`, which needs `lint-repo` and its gitleaks scan + (`pr-require-checks.yml`), both fail-closed on any non-success. The summary never decides the + hold or reaches a status: `report-statuses` then posts only `pipeline/report-outcome` on that branch, so any statuses the earlier run posted on the same head stand. - No check run is posted: there is no PR head. The lane's `report-statuses` job (in From 464a7e6f594cc6548d6067b1752b3b9c6d9373fe Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Wed, 28 Oct 2026 15:13:35 -0400 Subject: [PATCH 4/4] fix(pr-pipeline): keep holding a reuse PR whose leak scan is not clean lint-repo's gitleaks scan does not cover what the lane's reuse scan (scanLeak) flags: it honors a gitleaks:allow comment or .gitleaksignore the branch itself can add, its generic-api-key rule needs 10+ word-class characters at entropy 3.5 where scanLeak flags 8+ of any non-quote character, and it reads only the commits' patches, not every changed file's bytes. So a reuse PR holds again on any scan word but clean. Runner checks that did not report still hold nothing. Co-Authored-By: Claude Opus 5.5 --- ...-check-gap-hold-clears-on-pass-75c624a3.md | 2 +- .../work-resolve-issue/report-outcome.mjs | 13 ++--- .../report-outcome.test.mjs | 51 ++++++++----------- docs/conventions/pr-pipeline/README.md | 13 ++--- .../pr-pipeline/pr-run-activity.md | 9 ++-- 5 files changed, 41 insertions(+), 47 deletions(-) diff --git a/.changes/pr-pipeline/fix-check-gap-hold-clears-on-pass-75c624a3.md b/.changes/pr-pipeline/fix-check-gap-hold-clears-on-pass-75c624a3.md index 4fc2c97e548..c36709f190f 100644 --- a/.changes/pr-pipeline/fix-check-gap-hold-clears-on-pass-75c624a3.md +++ b/.changes/pr-pipeline/fix-check-gap-hold-clears-on-pass-75c624a3.md @@ -4,4 +4,4 @@ bump: patch ### Fixed -- `work-resolve-issue` no longer writes `Hold: a human merges this` on a reuse PR only because the runner's checks or its leak scan did not report (missing, for another tip, `failed` or no word). That line also failed `ci-status`, so pr-verify never ran and the PR waited on a person for a check CI runs anyway. The merge still needs `ci-status`, which needs `lint-repo` and its gitleaks scan, both fail-closed on any non-success. A failed runner check, a leak scan that found something, `merge: human`, untrusted provenance and every denied-path or unreadable-listing reason still hold it (`mustHold` in `report-outcome.mjs`). +- `work-resolve-issue` no longer writes `Hold: a human merges this` only because the runner's checks did not report (missing or for another tip). That line also failed `ci-status`, so pr-verify never ran and the PR waited on a person for checks `ci-status` reruns anyway. On a reused branch, a leak scan that is anything but `clean` (found, failed or missing) still holds it, since `lint-repo`'s gitleaks scan does not cover what the lane scan flags. A failed runner check, `merge: human`, untrusted provenance and every denied-path or unreadable-listing reason still hold it too (`mustHold` in `report-outcome.mjs`). diff --git a/.github/scripts/work-resolve-issue/report-outcome.mjs b/.github/scripts/work-resolve-issue/report-outcome.mjs index eb7d34a66e5..ff3e8612c00 100755 --- a/.github/scripts/work-resolve-issue/report-outcome.mjs +++ b/.github/scripts/work-resolve-issue/report-outcome.mjs @@ -705,10 +705,11 @@ export const checksPassed = (checks) => Boolean(checks?.results.length) && !chec // untrusted-provenance, an unreadable denied-path list or changed-file list, // a listing cut at the cap while some path is denied (it may hide one), a // changed path the merge lane refuses, a runner check that failed, and on a -// reused branch a leak scan that found something. Runner checks or a leak -// scan that did not report hold nothing here: the merge needs ci-status, -// which needs lint-repo and its gitleaks scan (pr-require-checks.yml), both -// fail-closed on any non-success. +// reused branch a leak scan that is missing or not clean. Runner checks that +// did not report hold nothing: ci-status reruns them on the head. The scan is +// not covered that way: lint-repo's gitleaks honors a gitleaks:allow comment +// or .gitleaksignore the branch can add, its password rule misses short or +// symbol-bearing values scanLeak flags, and it reads only the commits' patches. export function mustHold({ issueLabels, humanMerge, holdPaths, changedFiles, checks, reused = false }) { return ( humanMerge || @@ -718,7 +719,7 @@ export function mustHold({ issueLabels, humanMerge, holdPaths, changedFiles, che (holdPaths.length > 0 && changedFiles.length >= COMPARE_FILES_CAP) || changedFiles.some((path) => holdPaths.some((glob) => posix.matchesGlob(path, glob))) || Boolean(checks?.results.some(({ exit }) => exit !== 0)) || - (reused && checks?.scan === "found") + (reused && checks?.scan !== "clean") ); } @@ -728,7 +729,7 @@ const scanLine = (checks) => ? "The runner's leak scan read everything the branch changes since its merge base with the default branch and found nothing credential-shaped." : checks?.scan === "found" ? "The runner's leak scan found a credential shape or a password or secret assignment in what the branch changes, so a person merges this." - : "The runner's leak scan of the branch did not finish or could not be read."; + : "The runner's leak scan of the branch did not finish or could not be read, so a person merges this."; // The runner's check results as the PR body's Verification section shows them. const checkLines = (checks) => diff --git a/.github/scripts/work-resolve-issue/report-outcome.test.mjs b/.github/scripts/work-resolve-issue/report-outcome.test.mjs index bbc5b65c049..90a8f0831cf 100644 --- a/.github/scripts/work-resolve-issue/report-outcome.test.mjs +++ b/.github/scripts/work-resolve-issue/report-outcome.test.mjs @@ -2506,7 +2506,7 @@ const tipRoute = (sha = TIP) => ["GET", /\/git\/ref\/heads\/work-resolve-issue\/ const usable = { title: "docs(work-items): summary title", summary: "SUMMARY-PASS-TEXT", changes: ["CHANGE-ONE"] }; test("a reuse PR with a usable summary carries it under the hold line", () => { - const plan = reusePlan({ issueNumber: 77, branch: lane(400), summary: usable, changedFiles: ["README.md"], holdPaths: HOLD_GLOBS, humanMerge: true }); + const plan = reusePlan({ issueNumber: 77, branch: lane(400), summary: usable, changedFiles: ["README.md"], holdPaths: HOLD_GLOBS }); assert.equal(plan.title, "docs(work-items): summary title"); assert.ok(plan.body.startsWith("Closes #77\n\nHold: a human merges this\n\n## Summary\n"), plan.body); assert.ok(plan.body.includes("> SUMMARY-PASS-TEXT"), plan.body); @@ -2565,7 +2565,7 @@ test("main opens a held reuse PR when the compare request fails, saying the file }); }); -test("main opens a reuse PR with no hold line from a summary bound to this issue, branch and tip when only the runner checks are missing", async () => { +test("main opens a held reuse PR from a summary bound to this issue, branch and tip", async () => { await withAsyncDir({ "reuse-summary.json": summaryFile(), "resolved.json": RESOLVED }, async (dir) => { const { fetch, calls } = fakeFetch([tipRoute(), ...PR_ROUTES]); assert.equal(await main({ env: summaryEnv(dir), fetch }), 0); @@ -2574,7 +2574,7 @@ test("main opens a reuse PR with no hold line from a summary bound to this issue assert.equal(pull.title, "docs(work-items): summary title"); assert.ok(pull.body.includes("SUMMARY-PASS-TEXT") && pull.body.includes("CHANGE-ONE"), pull.body); assert.ok(pull.body.includes("\n- README.md\n") && !pull.body.includes("(denied path)"), pull.body); - assert.ok(!pull.body.split("\n").includes("Hold: a human merges this"), pull.body); + assert.ok(pull.body.split("\n").includes("Hold: a human merges this"), pull.body); }); }); @@ -3254,15 +3254,13 @@ const HOLD_CASES = { "a trusted labeler set merge: human": { humanMerge: true }, "a runner check failed": { checks: { ...CLEAN_CHECKS, results: [{ check: FRAGMENTS, exit: 1 }] } }, }; +// A reuse's scan word, when it is anything but clean: lint-repo's gitleaks +// does not cover what scanLeak flags, so a missing scan holds like a finding. const SCAN_CASES = { "the leak scan found something": { checks: { ...CLEAN_CHECKS, scan: "found" } }, -}; -// A check that did not report: ci-status, pr-verify and the security review -// lane check the PR's head before enable-auto-merge arms it, so no hold line. -const GAP_CASES = { - "the runner checks are missing": { checks: null }, "the leak scan failed": { checks: { ...CLEAN_CHECKS, scan: "failed" } }, "the leak scan never ran": { checks: { tip: TIP, results: CLEAN_CHECKS.results } }, + "the runner checks and their scan are missing": { checks: null }, }; for (const [name, overrides] of Object.entries({ ...HOLD_CASES, ...SCAN_CASES })) { @@ -3272,21 +3270,12 @@ for (const [name, overrides] of Object.entries({ ...HOLD_CASES, ...SCAN_CASES }) }); } -for (const [gap, gapOverrides] of Object.entries(GAP_CASES)) { - test(`a reuse PR does not hold only because ${gap}, and says what checks its head`, () => { - const { body } = reusePlan({ ...CLEAN_REUSE, ...gapOverrides }); - assert.ok(!body.split("\n").includes(HOLD_LINE), body); - assert.ok(body.includes("ci-status, pr-verify and the security review lane check the head before it merges."), body); - assert.ok(!body.includes("a person merges this") && !body.includes("a person reviews the diff"), body); - }); - - test(`a reuse PR still holds on every other condition when ${gap}`, () => { - for (const [name, overrides] of Object.entries({ ...HOLD_CASES, ...SCAN_CASES })) { - const checks = overrides.checks === undefined ? gapOverrides.checks : overrides.checks; - assert.equal(reuseHeld({ ...gapOverrides, ...overrides, checks }), true, name); - } - }); -} +test("a reuse PR with a clean scan does not hold only because no runner check reported, and says what checks its head", () => { + const { body } = reusePlan({ ...CLEAN_REUSE, checks: { ...CLEAN_CHECKS, results: [] } }); + assert.ok(!body.split("\n").includes(HOLD_LINE), body); + assert.ok(body.includes("ci-status, pr-verify and the security review lane check the head before it merges."), body); + assert.ok(!body.includes("a person reviews the diff"), body); +}); test("a clean reuse PR names the clean leak scan and leaves the person's review out", () => { const { body } = reusePlan(CLEAN_REUSE); @@ -3302,12 +3291,17 @@ test("a capped listing holds a reuse PR only while some path is denied", () => { assert.equal(reuseHeld({ changedFiles: CAPPED }), true); }); -test("mustHold: a reuse holds on a scan that found something or a failed check, and not on a scan that did not report", () => { +test("mustHold: a reuse holds on any scan word but clean and on a failed check; runner checks that did not report hold nothing", () => { const facts = { issueLabels: CLEAN_REUSE.issueLabels, humanMerge: false, holdPaths: HOLD_GLOBS, changedFiles: ["README.md"], reused: true }; - assert.equal(mustHold({ ...facts, checks: { tip: TIP, results: CLEAN_CHECKS.results } }), false, "scan missing"); - assert.equal(mustHold({ ...facts, checks: null }), false, "checks and scan missing"); + assert.equal(mustHold({ ...facts, checks: CLEAN_CHECKS }), false, "clean reuse"); + assert.equal(mustHold({ ...facts, checks: { tip: TIP, results: CLEAN_CHECKS.results } }), true, "scan missing"); + assert.equal(mustHold({ ...facts, checks: null }), true, "checks and scan missing"); + assert.equal(mustHold({ ...facts, checks: { ...CLEAN_CHECKS, scan: "failed" } }), true, "scan failed"); assert.equal(mustHold({ ...facts, checks: { ...CLEAN_CHECKS, scan: "found" } }), true, "scan found"); assert.equal(mustHold({ ...facts, checks: { ...CLEAN_CHECKS, results: [{ check: FRAGMENTS, exit: 1 }] } }), true, "a failed check"); + assert.equal(mustHold({ ...facts, checks: { ...CLEAN_CHECKS, results: [] } }), false, "reuse, clean scan, no runner check reported"); + assert.equal(mustHold({ ...facts, reused: false, checks: null }), false, "no reuse, runner checks missing"); + assert.equal(mustHold({ ...facts, reused: false, checks: { tip: TIP, results: [{ check: FRAGMENTS, exit: 1 }] } }), true, "no reuse, a failed check"); }); test("decide() and reusePlan() hold on the same runner facts, through one mustHold", () => { @@ -3351,10 +3345,9 @@ async function runReuse({ scan = "clean", labels = ["agent-ready", "work-class: }); } -test("main opens a clean reuse PR with no hold line, holds it on a leak scan that found something, and not on one that did not report", async () => { +test("main opens a clean reuse PR with no hold line, and holds it on a leak scan that is not clean", async () => { assert.ok(!holds((await runReuse()).body)); - assert.ok(holds((await runReuse({ scan: "found" })).body), "found"); - for (const scan of ["failed", null]) assert.ok(!holds((await runReuse({ scan })).body), String(scan)); + for (const scan of ["found", "failed", null]) assert.ok(holds((await runReuse({ scan })).body), String(scan)); }); test("main holds a reuse PR on a denied path or a failed compare, and opens a clean one unheld", async () => { diff --git a/docs/conventions/pr-pipeline/README.md b/docs/conventions/pr-pipeline/README.md index 285ef07fc97..084a7615df3 100644 --- a/docs/conventions/pr-pipeline/README.md +++ b/docs/conventions/pr-pipeline/README.md @@ -1611,12 +1611,13 @@ the pull request opened, while it waits to be armed or sits in the queue, stops person on that list lifts it by removing the label and re-queuing. A person's own pull request is not read. -report-outcome writes no `Hold:` line for a check the issue lane's runner did not report (its -checks missing or for another tip, or a reuse leak scan that did not finish): that line would also -fail `ci-status`, so pr-verify would never run. The merge needs `ci-status`, which needs -`lint-repo` and its gitleaks scan (`pr-require-checks.yml`), both fail-closed on any non-success, -so a lane scan that did not report holds nothing. A failed runner check, a leak scan that found -something and every trust reason still write the line. +report-outcome writes no `Hold:` line only because the issue lane's runner checks did not report +(missing or for another tip): that line would also fail `ci-status`, so pr-verify would never run, +and `ci-status` reruns those checks on the head. A reuse leak scan is not covered that way, so on a +reused branch any scan word but `clean` still holds: `lint-repo`'s gitleaks scan honors a +`gitleaks:allow` comment or `.gitleaksignore` the branch can add, its password rule misses short +or symbol-bearing values the lane scan flags, and it reads only the commits' patches. A failed +runner check and every trust reason still write the line. A removal by a non-human cannot be lifted on that issue: both readers check every removal in the issue's timeline, and the timeline keeps it, so a trusted person re-adding and then removing the diff --git a/docs/conventions/pr-pipeline/pr-run-activity.md b/docs/conventions/pr-pipeline/pr-run-activity.md index d15beeba1c0..5bd35ecaf0e 100644 --- a/docs/conventions/pr-pipeline/pr-run-activity.md +++ b/docs/conventions/pr-pipeline/pr-run-activity.md @@ -2130,11 +2130,10 @@ step under the escalation marker and an `outcome=` marker. What changes unavailable and still opens the PR. The PR carries the hold line on the same conditions as a `done` run's (one check, `mustHold` in `report-outcome.mjs`), `merge: human` included, plus two of its own: the compare failed, or the leak scan `run-checks` ran on the branch (its - `scan` word in the checks record) is `found`. A runner check that failed holds it too. Runner - checks or a leak scan that did not report (missing, another tip, `failed`, no word) add no - hold line: the merge needs `ci-status`, which needs `lint-repo` and its gitleaks scan - (`pr-require-checks.yml`), both fail-closed on any non-success. The summary never decides the - hold or reaches a status: + `scan` word in the checks record) is missing or not `clean`, since `lint-repo`'s gitleaks scan + does not cover what that scan flags. A runner check that failed holds it too. Runner checks + that did not report (missing or another tip) add no hold line of their own: `ci-status` reruns + them on the head. The summary never decides the hold or reaches a status: `report-statuses` then posts only `pipeline/report-outcome` on that branch, so any statuses the earlier run posted on the same head stand. - No check run is posted: there is no PR head. The lane's `report-statuses` job (in