Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
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 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`).
17 changes: 11 additions & 6 deletions .github/scripts/work-resolve-issue/report-outcome.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -704,8 +704,12 @@ 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 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 ||
Expand All @@ -714,9 +718,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 !== "clean")
Comment thread
kyle-sexton marked this conversation as resolved.
Resolved
);
}

Expand Down Expand Up @@ -817,7 +820,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",
"",
Expand Down
24 changes: 23 additions & 1 deletion .github/scripts/work-resolve-issue/report-outcome.test.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -3252,13 +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 }] } },
};
// 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" } },
"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 })) {
Expand All @@ -3268,6 +3270,13 @@ for (const [name, overrides] of Object.entries({ ...HOLD_CASES, ...SCAN_CASES })
});
}

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);
assert.ok(body.startsWith("Closes #77\n\n## Summary\n"), body);
Expand All @@ -3282,6 +3291,19 @@ 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 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: 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", () => {
const held = (body) => body.split("\n").includes(HOLD_LINE);
for (const [name, overrides] of Object.entries({ "nothing": {}, ...HOLD_CASES })) {
Expand Down
8 changes: 8 additions & 0 deletions docs/conventions/pr-pipeline/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -1614,6 +1614,14 @@ 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,
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
label still holds. The way out is a new issue a person files for the work, and a new pull request
Expand Down
8 changes: 5 additions & 3 deletions docs/conventions/pr-pipeline/pr-run-activity.md
Original file line number Diff line number Diff line change
Expand Up @@ -2133,9 +2133,11 @@ step under the escalation marker and an `outcome=<reason>` 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 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
Expand Down
Loading