From ce2ac939ce4d3b47a57bddc402814233a4d4a167 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Thu, 29 Oct 2026 18:41:34 -0400 Subject: [PATCH] perf(pr-pipeline): record main-review stops and cut merge-job lane reads pr-review-main now records a designed stop (binary, too-large, long-line) as a neutral `verdict=none reason=` check run instead of failing the run. The binder reads it as no CRITICAL and no binding, naming the lane's unbound reason `main-review-stop:` inside the existing MED evidence-unbound cause. No person hold is added. For a lane-source pull request the merge job lists only the security lane's pull_request runs and reads the code lane's pr-review-main check runs on the head only. Refs: #10961 Co-Authored-By: Claude Opus 5.5 --- ...read-cuts-and-main-review-stop-5c2e91a7.md | 12 ++ ...read-cuts-and-main-review-stop-9d41b3e6.md | 17 ++ .../scripts/pr-review-main/workflow.test.mjs | 118 +++++++++++++- .github/workflows/pr-review-main.yml | 147 +++++++++++++----- docs/conventions/pr-pipeline/README.md | 8 +- .../skills/babysit-prs/reference/safety.md | 4 +- .../skills/babysit-prs/scripts/babysit_gh.py | 69 +++++--- .../babysit-prs/scripts/babysit_merge.py | 50 +++++- .../scripts/babysit_review_evidence.py | 24 ++- .../scripts/tests/test_babysit_gh.py | 90 ++++++++++- .../scripts/tests/test_babysit_merge.py | 71 ++++++++- .../tests/test_babysit_review_evidence.py | 85 ++++++++++ 12 files changed, 605 insertions(+), 90 deletions(-) create mode 100644 .changes/pr-pipeline/perf-merge-job-read-cuts-and-main-review-stop-5c2e91a7.md create mode 100644 .changes/source-control/perf-merge-job-read-cuts-and-main-review-stop-9d41b3e6.md diff --git a/.changes/pr-pipeline/perf-merge-job-read-cuts-and-main-review-stop-5c2e91a7.md b/.changes/pr-pipeline/perf-merge-job-read-cuts-and-main-review-stop-5c2e91a7.md new file mode 100644 index 0000000000..702a365c8a --- /dev/null +++ b/.changes/pr-pipeline/perf-merge-job-read-cuts-and-main-review-stop-5c2e91a7.md @@ -0,0 +1,12 @@ +--- +bump: minor +--- + +### Changed + +- pr-review-main records a designed stop instead of failing. A diff that holds a binary file, is + over the 300000-byte cap, or has a line over 1800 bytes leaves the leg green with no verdict. + The record job then posts the check run `pr-review-main: ` on the head, titled + `verdict=none reason=` with conclusion neutral, and logs a + `pr-review-main stop` notice. Each stop is one countable record, and a real failure (a failed + fetch or diff write, an empty diff) still fails the run. diff --git a/.changes/source-control/perf-merge-job-read-cuts-and-main-review-stop-9d41b3e6.md b/.changes/source-control/perf-merge-job-read-cuts-and-main-review-stop-9d41b3e6.md new file mode 100644 index 0000000000..6bbc639cde --- /dev/null +++ b/.changes/source-control/perf-merge-job-read-cuts-and-main-review-stop-9d41b3e6.md @@ -0,0 +1,17 @@ +--- +bump: minor +--- + +### Changed + +- The babysit merge gate reads pr-review-main's `verdict=none reason=` check run as a + designed stop. The stop counts no CRITICAL finding and binds no lane. With no verdict on the + head, the lane's unbound reason is `main-review-stop:` instead of + `lane-source-changed`, so the gate report names why. It stays the same MED + `evidence-unbound:` lane cause, with no new person hold. Any other title that does not + read still counts one CRITICAL. +- The merge job reads less for a pull request that changes a lane source. It lists only the + security lane's pull_request runs, because the code lane binds there on a main-run review + alone, and reads the code lane's `pr-review-main` check runs on the head only. The security + lane still reads every head of the pull request, so a CRITICAL finding on any head still + carries. diff --git a/.github/scripts/pr-review-main/workflow.test.mjs b/.github/scripts/pr-review-main/workflow.test.mjs index fdffccc0db..28b973d6fe 100644 --- a/.github/scripts/pr-review-main/workflow.test.mjs +++ b/.github/scripts/pr-review-main/workflow.test.mjs @@ -129,6 +129,117 @@ test("a diff with a line past the review's cap gets no verdict", async () => { } }); +// The "Write the diff" script run under bash as Actions runs it, with git and +// python3 stubbed: python3 (write-diff.py) writes the given patch, or fails. +function runDiff(patch, { pythonFails = false, job = KEY, maxBytes = "300" } = {}) { + const run = /^ {8}run: \|\n((?:(?: {10}.*)?\n)+)/m.exec(`${step("Write the diff", job)}\n`)[1].replace(/^ {10}/gm, ""); + const dir = mkdtempSync(path.join(tmpdir(), "pr-review-main-stop-")); + try { + const bin = path.join(dir, "bin"); + mkdirSync(bin); + writeFileSync(path.join(bin, "git"), `#!/bin/sh\ncase "$*" in *rev-parse*) echo ${"e".repeat(40)} ;; esac\n`); + writeFileSync(path.join(dir, "fixture.patch"), patch); + writeFileSync(path.join(bin, "python3"), pythonFails ? "#!/bin/sh\nexit 1\n" : `#!/bin/sh\ncp "${path.join(dir, "fixture.patch")}" "$6"\n`); + chmodSync(path.join(bin, "git"), 0o755); + chmodSync(path.join(bin, "python3"), 0o755); + const out = path.join(dir, "out"); + writeFileSync(out, ""); + const data = path.join(dir, "data"); + const env = { ...process.env, PATH: `${bin}:${process.env.PATH}`, GITHUB_OUTPUT: out, LANE: "claude-security-review", HEAD_SHA: SHA_A, BASE_SHA: SHA_B, PR_NUMBER: "7", DATA: data, SRC: path.join(dir, "src"), MAX_DIFF_BYTES: maxBytes, PART_BYTES: "15000", PART_LINES: "1000", LONG_LINE_BYTES: "1800" }; + const result = spawnSync("bash", ["--noprofile", "--norc", "-e", "-o", "pipefail", "-c", run], { encoding: "utf8", env }); + const parts = (() => { + try { + return readdirSync(path.join(data, "diff")); + } catch { + return []; + } + })(); + return { status: result.status, output: readFileSync(out, "utf8"), stdout: result.stdout, parts }; + } finally { + rmSync(dir, { recursive: true, force: true }); + } +} + +test("a diff the review cannot read whole stops green and names its reason for the record job", () => { + const small = "diff --git a/x b/x\n@@ -1 +1 @@\n+short\n"; + for (const job of LEGS) { + const tooLarge = runDiff(`diff --git a/x b/x\n@@ -1 +1 @@\n${"+line\n".repeat(100)}`, { job }); + assert.equal(tooLarge.status, 0, job); + assert.equal(tooLarge.output, `stop=too-large\nclaude-security-review-stop=too-large\nclaude-security-review-head=${SHA_A}\n`, job); + assert.deepEqual(tooLarge.parts, [], job); + assert.match(tooLarge.stdout, /^::notice title=pr-review-main stop::reason=too-large lane=claude-security-review pr=7; /m, job); + const longLine = runDiff(`diff --git a/x b/x\n@@ -1 +1 @@\n+${"x".repeat(1801)}\n`, { job, maxBytes: "300000" }); + assert.equal(longLine.status, 0, job); + assert.match(longLine.output, /^stop=long-line$/m, job); + // A failed write-diff.py is no designed stop: the step fails, unnamed. + const failed = runDiff(small, { job, pythonFails: true }); + assert.notEqual(failed.status, 0, job); + assert.doesNotMatch(failed.output, /stop=/, job); + // A diff it reads whole writes its parts and no stop. + const whole = runDiff(small, { job }); + assert.equal(whole.status, 0, job); + assert.doesNotMatch(whole.output, /stop=/, job); + assert.deepEqual(whole.parts, ["part-001.patch"], job); + } +}); + +test("after a stop no later step of the leg runs, and each lane's stop leaves the leg", () => { + for (const job of LEGS) { + const gated = STEPS[job].filter((s) => /steps\.diff\.outcome == 'success'/.test(s.block)); + assert.ok(gated.length >= 5, job); + for (const s of gated) assert.match(s.block, /steps\.diff\.outcome == 'success' && steps\.diff\.outputs\.stop == ''/, `${job}: ${s.name}`); + for (const lane of ["claude-review", "claude-security-review"]) { + for (const kind of ["stop", "head"]) { + assert.match(jobText(job), new RegExp(`^ {6}${lane}-${kind}: \\$\\{\\{ steps\\.diff\\.outputs\\.${lane}-${kind} \\}\\}$`, "m"), `${job} ${lane}-${kind}`); + } + } + } +}); + +// The record job's "Record the stops" script, with gh stubbed to log its args. +function recordStops(env) { + const block = stepsOf("record").find((s) => s.name === "Record the stops").block; + const run = /^ {8}run: \|\n((?:(?: {10}.*)?\n)+)/m.exec(`${block}\n`)[1].replace(/^ {10}/gm, ""); + const dir = mkdtempSync(path.join(tmpdir(), "pr-review-main-record-")); + try { + const log = path.join(dir, "gh.log"); + writeFileSync(path.join(dir, "gh"), `#!/bin/sh\nprintf '%s\\n' "$@" >>"${log}"\n`); + chmodSync(path.join(dir, "gh"), 0o755); + writeFileSync(log, ""); + const blank = { CODE_STOP: "", CODE_HEAD: "", SECURITY_STOP: "", SECURITY_HEAD: "" }; + const result = spawnSync("bash", ["--noprofile", "--norc", "-e", "-o", "pipefail", "-c", run], { encoding: "utf8", env: { ...process.env, PATH: `${dir}:${process.env.PATH}`, GITHUB_REPOSITORY: "o/r", ...blank, ...env } }); + return { status: result.status, calls: readFileSync(log, "utf8") }; + } finally { + rmSync(dir, { recursive: true, force: true }); + } +} + +test("the record job posts each stop as a neutral verdict=none record, from a closed reason", () => { + const block = stepsOf("record").find((s) => s.name === "Record the stops").block; + // A stop run uploads no artifact, so the step never waits on the download. + assert.doesNotMatch(block, /steps\.download/); + assert.match(block, /\^\(too-large\|binary\|long-line\)\$/); + for (const [lane, prefix] of [["claude-review", "CODE"], ["claude-security-review", "SECURITY"]]) { + for (const kind of ["stop", "head"]) { + assert.equal(input(block, `${prefix}_${kind.toUpperCase()}`), `\${{ needs.review.outputs.${lane}-${kind} || needs.review-subscription.outputs.${lane}-${kind} || needs.review-fallback.outputs.${lane}-${kind} }}`); + } + } + const posted = recordStops({ SECURITY_STOP: "too-large", SECURITY_HEAD: SHA_B }); + assert.equal(posted.status, 0); + const args = posted.calls.split("\n"); + for (const arg of ["repos/o/r/check-runs", "name=pr-review-main: claude-security-review", `head_sha=${SHA_B}`, "conclusion=neutral", "output[title]=verdict=none reason=too-large"]) { + assert.ok(args.includes(arg), arg); + } + assert.doesNotMatch(posted.calls, /claude-review\n/); + // No stop, nothing posted; a reason or head outside the closed set fails. + assert.deepEqual(recordStops({}), { status: 0, calls: "" }); + for (const bad of [{ CODE_STOP: "other", CODE_HEAD: SHA_A }, { CODE_STOP: "too-large", CODE_HEAD: "main" }, { CODE_STOP: "too-large\nx", CODE_HEAD: SHA_A }]) { + const result = recordStops(bad); + assert.notEqual(result.status, 0, JSON.stringify(bad)); + assert.equal(result.calls, ""); + } +}); + // write-diff.py run on a fixture repository: each case is a commit on top of // a base, and the patch it prints for base...head. // A cap of 0 makes every patch over it, so moves may collapse. @@ -572,7 +683,12 @@ test("a later leg runs only for the lanes the previous leg handed on", () => { } // The fallback leg is the last: it hands nothing on. assert.equal(index("Hand the lane on", FB), -1); - assert.doesNotMatch(jobText(FB), /^ {4}outputs:$/m); + // Its outputs are the designed stops alone, for the record job. + const fbOutputs = /^ {4}outputs:\n((?: {6}.*\n)+)/m.exec(jobText(FB))[1]; + assert.deepEqual( + [...fbOutputs.matchAll(/^ {6}([a-z-]+):/gm)].map(([, name]) => name), + ["claude-review-stop", "claude-security-review-stop", "claude-review-head", "claude-security-review-head"], + ); assert.doesNotMatch(step("Review the pull request", KEY), /fallback-ready/); assert.equal(input(step("Review the pull request", SUB), "fallback-ready"), "${{ steps.credential.outputs.fallback-ready }}"); assert.doesNotMatch(step("Review the pull request", FB), /fallback-ready/); diff --git a/.github/workflows/pr-review-main.yml b/.github/workflows/pr-review-main.yml index 4c9e9a509c..3ef6250689 100644 --- a/.github/workflows/pr-review-main.yml +++ b/.github/workflows/pr-review-main.yml @@ -97,6 +97,10 @@ jobs: claude-security-review: ${{ steps.handoff.outputs.claude-security-review }} claude-review-base-sha: ${{ steps.base.outputs.claude-review-base-sha }} claude-security-review-base-sha: ${{ steps.base.outputs.claude-security-review-base-sha }} + claude-review-stop: ${{ steps.diff.outputs.claude-review-stop }} + claude-security-review-stop: ${{ steps.diff.outputs.claude-security-review-stop }} + claude-review-head: ${{ steps.diff.outputs.claude-review-head }} + claude-security-review-head: ${{ steps.diff.outputs.claude-security-review-head }} steps: - name: Check out the default branch tip uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 @@ -191,13 +195,17 @@ jobs: # a move that may run or load (an executable, a skills, agents, # commands or hooks path, a file a tool reads by name), and a move an # added line names still print whole. A patch that still - # shows a binary file, or is larger than one review can read whole, - # gets no verdict: the step fails and the lanes stay unbound. The tree - # the verdict is keyed on comes from git, never from the model. + # shows a binary file, is larger than one review can read whole, or + # holds a line longer than Read returns whole gets no verdict: the step + # stops green with the reason as an output, the record job posts it as + # `verdict=none reason=` on the head, and the lanes stay unbound + # under that reason. Any other failure fails the step. The tree the + # verdict is keyed on comes from git, never from the model. - name: Write the diff id: diff if: steps.select.outputs.review == 'true' env: + LANE: ${{ matrix.lane }} HEAD_SHA: ${{ steps.gate.outputs.head-sha }} BASE_SHA: ${{ steps.gate.outputs.base-sha }} PR_NUMBER: ${{ steps.gate.outputs.pr-number }} @@ -217,6 +225,13 @@ jobs: # (babysit_risk LONG_LINE_BYTES, `long-line-review:`). LONG_LINE_BYTES: "1800" run: | + # A designed stop: the reason, from this script's own literals, and + # the head, as the lane's outputs for the record job. + stop() { + echo "::notice title=pr-review-main stop::reason=$1 lane=$LANE pr=$PR_NUMBER; $2; no verdict, so the lanes stay unbound" + { echo "stop=$1"; echo "$LANE-stop=$1"; echo "$LANE-head=$HEAD_SHA"; } >>"$GITHUB_OUTPUT" + exit 0 + } export GIT_CONFIG_NOSYSTEM=1 GIT_CONFIG_GLOBAL=/dev/null mkdir -p "$DATA" git init -q "$SRC" @@ -224,17 +239,14 @@ jobs: git -C "$SRC" fetch -q --no-tags --filter=blob:none origin "$BASE_SHA" "$HEAD_SHA" python3 -I .base/.github/scripts/pr-review-main/write-diff.py "$SRC" "$BASE_SHA" "$HEAD_SHA" "$DATA/diff.patch" "$MAX_DIFF_BYTES" if grep -aqE '^(Binary files .* differ|GIT binary patch)$' "$DATA/diff.patch"; then - echo "::error::the diff holds a binary file the review cannot read; no verdict, so the lanes stay unbound" - exit 1 + stop binary "the diff holds a binary file the review cannot read" fi if [ "$(wc -c <"$DATA/diff.patch")" -gt "$MAX_DIFF_BYTES" ]; then - echo "::error::the diff is over $MAX_DIFF_BYTES bytes, more than one review reads whole; no verdict, so the lanes stay unbound" - exit 1 + stop too-large "the diff is $(wc -c <"$DATA/diff.patch") bytes, over the $MAX_DIFF_BYTES one review reads whole" fi # A diff line's text, past its +, - or space, longer than the cap. if ! LC_ALL=C awk -v max="$LONG_LINE_BYTES" 'length($0) - 1 > max { exit 3 }' "$DATA/diff.patch"; then - echo "::error::a diff line is over $LONG_LINE_BYTES bytes, more than the review reads whole; no verdict, so the lanes stay unbound and the merge gate holds it for a person" - exit 1 + stop long-line "a diff line is over $LONG_LINE_BYTES bytes, more than the review reads whole" fi # Parts the reviewer reads whole, each named in the prompt; the # verdict counts only when every part was read whole @@ -261,7 +273,7 @@ jobs: echo "diff: $(cat "$DATA"/diff/part-*.patch | wc -l) lines in $(find "$DATA/diff" -type f | wc -l) parts" - name: Select the trusted PR text - if: steps.diff.outcome == 'success' + if: steps.diff.outcome == 'success' && steps.diff.outputs.stop == '' env: API_BUDGET_TOKEN: github-token uses: ./.base/.github/actions/select-trusted-text # zizmor: ignore[self-repository] @@ -276,7 +288,7 @@ jobs: # in it (`../../reference/...`) resolves inside the data directory; the # rules the code lane names go beside the standards. - name: Copy the review criteria - if: steps.diff.outcome == 'success' + if: steps.diff.outcome == 'success' && steps.diff.outputs.stop == '' env: SKILL: ${{ matrix.skill }} DATA: ${{ github.workspace }}/pr-review-main-data @@ -291,7 +303,7 @@ jobs: - name: Resolve the review's model id: models - if: steps.diff.outcome == 'success' + if: steps.diff.outcome == 'success' && steps.diff.outputs.stop == '' uses: ./.base/.github/actions/resolve-config # zizmor: ignore[self-repository] with: model-route: ${{ matrix.route }} @@ -302,7 +314,7 @@ jobs: # set, nothing reviews and the lane stays unbound. - name: Check the credential id: credential - if: steps.diff.outcome == 'success' + if: steps.diff.outcome == 'success' && steps.diff.outputs.stop == '' env: PRESENT: ${{ secrets.REVIEWS_CLAUDE_API_KEY != '' }} SECRET_NAME: REVIEWS_CLAUDE_API_KEY @@ -504,7 +516,7 @@ jobs: # A reviewed pull request that left no verdict and handed nothing on # fails here, so the run shows it; the lane stays unbound either way. - name: Fail without a verdict - if: always() && steps.diff.outcome == 'success' && steps.credential.outputs.present == 'true' && steps.verdict.outcome != 'success' && steps.handoff.outcome != 'success' + if: always() && steps.diff.outcome == 'success' && steps.diff.outputs.stop == '' && steps.credential.outputs.present == 'true' && steps.verdict.outcome != 'success' && steps.handoff.outcome != 'success' run: | echo "::error::the review left no verdict for this head" exit 1 @@ -576,6 +588,10 @@ jobs: claude-security-review: ${{ steps.handoff.outputs.claude-security-review }} claude-review-base-sha: ${{ steps.base.outputs.claude-review-base-sha }} claude-security-review-base-sha: ${{ steps.base.outputs.claude-security-review-base-sha }} + claude-review-stop: ${{ steps.diff.outputs.claude-review-stop }} + claude-security-review-stop: ${{ steps.diff.outputs.claude-security-review-stop }} + claude-review-head: ${{ steps.diff.outputs.claude-review-head }} + claude-security-review-head: ${{ steps.diff.outputs.claude-security-review-head }} steps: - name: Check out the default branch tip uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 @@ -669,13 +685,17 @@ jobs: # a move that may run or load (an executable, a skills, agents, # commands or hooks path, a file a tool reads by name), and a move an # added line names still print whole. A patch that still - # shows a binary file, or is larger than one review can read whole, - # gets no verdict: the step fails and the lanes stay unbound. The tree - # the verdict is keyed on comes from git, never from the model. + # shows a binary file, is larger than one review can read whole, or + # holds a line longer than Read returns whole gets no verdict: the step + # stops green with the reason as an output, the record job posts it as + # `verdict=none reason=` on the head, and the lanes stay unbound + # under that reason. Any other failure fails the step. The tree the + # verdict is keyed on comes from git, never from the model. - name: Write the diff id: diff if: steps.select.outputs.review == 'true' env: + LANE: ${{ matrix.lane }} HEAD_SHA: ${{ steps.gate.outputs.head-sha }} BASE_SHA: ${{ steps.gate.outputs.base-sha }} PR_NUMBER: ${{ steps.gate.outputs.pr-number }} @@ -695,6 +715,13 @@ jobs: # (babysit_risk LONG_LINE_BYTES, `long-line-review:`). LONG_LINE_BYTES: "1800" run: | + # A designed stop: the reason, from this script's own literals, and + # the head, as the lane's outputs for the record job. + stop() { + echo "::notice title=pr-review-main stop::reason=$1 lane=$LANE pr=$PR_NUMBER; $2; no verdict, so the lanes stay unbound" + { echo "stop=$1"; echo "$LANE-stop=$1"; echo "$LANE-head=$HEAD_SHA"; } >>"$GITHUB_OUTPUT" + exit 0 + } export GIT_CONFIG_NOSYSTEM=1 GIT_CONFIG_GLOBAL=/dev/null mkdir -p "$DATA" git init -q "$SRC" @@ -702,17 +729,14 @@ jobs: git -C "$SRC" fetch -q --no-tags --filter=blob:none origin "$BASE_SHA" "$HEAD_SHA" python3 -I .base/.github/scripts/pr-review-main/write-diff.py "$SRC" "$BASE_SHA" "$HEAD_SHA" "$DATA/diff.patch" "$MAX_DIFF_BYTES" if grep -aqE '^(Binary files .* differ|GIT binary patch)$' "$DATA/diff.patch"; then - echo "::error::the diff holds a binary file the review cannot read; no verdict, so the lanes stay unbound" - exit 1 + stop binary "the diff holds a binary file the review cannot read" fi if [ "$(wc -c <"$DATA/diff.patch")" -gt "$MAX_DIFF_BYTES" ]; then - echo "::error::the diff is over $MAX_DIFF_BYTES bytes, more than one review reads whole; no verdict, so the lanes stay unbound" - exit 1 + stop too-large "the diff is $(wc -c <"$DATA/diff.patch") bytes, over the $MAX_DIFF_BYTES one review reads whole" fi # A diff line's text, past its +, - or space, longer than the cap. if ! LC_ALL=C awk -v max="$LONG_LINE_BYTES" 'length($0) - 1 > max { exit 3 }' "$DATA/diff.patch"; then - echo "::error::a diff line is over $LONG_LINE_BYTES bytes, more than the review reads whole; no verdict, so the lanes stay unbound and the merge gate holds it for a person" - exit 1 + stop long-line "a diff line is over $LONG_LINE_BYTES bytes, more than the review reads whole" fi # Parts the reviewer reads whole, each named in the prompt; the # verdict counts only when every part was read whole @@ -739,7 +763,7 @@ jobs: echo "diff: $(cat "$DATA"/diff/part-*.patch | wc -l) lines in $(find "$DATA/diff" -type f | wc -l) parts" - name: Select the trusted PR text - if: steps.diff.outcome == 'success' + if: steps.diff.outcome == 'success' && steps.diff.outputs.stop == '' env: API_BUDGET_TOKEN: github-token uses: ./.base/.github/actions/select-trusted-text # zizmor: ignore[self-repository] @@ -754,7 +778,7 @@ jobs: # in it (`../../reference/...`) resolves inside the data directory; the # rules the code lane names go beside the standards. - name: Copy the review criteria - if: steps.diff.outcome == 'success' + if: steps.diff.outcome == 'success' && steps.diff.outputs.stop == '' env: SKILL: ${{ matrix.skill }} DATA: ${{ github.workspace }}/pr-review-main-data @@ -769,7 +793,7 @@ jobs: - name: Resolve the review's model id: models - if: steps.diff.outcome == 'success' + if: steps.diff.outcome == 'success' && steps.diff.outputs.stop == '' uses: ./.base/.github/actions/resolve-config # zizmor: ignore[self-repository] with: model-route: ${{ matrix.route }} @@ -780,7 +804,7 @@ jobs: # set, nothing reviews and the lane stays unbound. - name: Check the credential id: credential - if: steps.diff.outcome == 'success' + if: steps.diff.outcome == 'success' && steps.diff.outputs.stop == '' env: PRESENT: ${{ secrets.REVIEWS_CLAUDE_CODE_OAUTH_TOKEN != '' }} SECRET_NAME: REVIEWS_CLAUDE_CODE_OAUTH_TOKEN @@ -985,7 +1009,7 @@ jobs: # A reviewed pull request that left no verdict and handed nothing on # fails here, so the run shows it; the lane stays unbound either way. - name: Fail without a verdict - if: always() && steps.diff.outcome == 'success' && steps.credential.outputs.present == 'true' && steps.verdict.outcome != 'success' && steps.handoff.outcome != 'success' + if: always() && steps.diff.outcome == 'success' && steps.diff.outputs.stop == '' && steps.credential.outputs.present == 'true' && steps.verdict.outcome != 'success' && steps.handoff.outcome != 'success' run: | echo "::error::the review left no verdict for this head" exit 1 @@ -1046,6 +1070,13 @@ jobs: contents: read # check out main; read the PR's commits for the diff pull-requests: read # the PR, its files, comments and reviews actions: read # the upstream run the trigger gate reads + # One output per lane, since a matrix job's outputs merge: the designed + # stop that left the lane without a verdict, and the head it names. + outputs: + claude-review-stop: ${{ steps.diff.outputs.claude-review-stop }} + claude-security-review-stop: ${{ steps.diff.outputs.claude-security-review-stop }} + claude-review-head: ${{ steps.diff.outputs.claude-review-head }} + claude-security-review-head: ${{ steps.diff.outputs.claude-security-review-head }} steps: - name: Check out the default branch tip uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 @@ -1139,13 +1170,17 @@ jobs: # a move that may run or load (an executable, a skills, agents, # commands or hooks path, a file a tool reads by name), and a move an # added line names still print whole. A patch that still - # shows a binary file, or is larger than one review can read whole, - # gets no verdict: the step fails and the lanes stay unbound. The tree - # the verdict is keyed on comes from git, never from the model. + # shows a binary file, is larger than one review can read whole, or + # holds a line longer than Read returns whole gets no verdict: the step + # stops green with the reason as an output, the record job posts it as + # `verdict=none reason=` on the head, and the lanes stay unbound + # under that reason. Any other failure fails the step. The tree the + # verdict is keyed on comes from git, never from the model. - name: Write the diff id: diff if: steps.select.outputs.review == 'true' env: + LANE: ${{ matrix.lane }} HEAD_SHA: ${{ steps.gate.outputs.head-sha }} BASE_SHA: ${{ steps.gate.outputs.base-sha }} PR_NUMBER: ${{ steps.gate.outputs.pr-number }} @@ -1165,6 +1200,13 @@ jobs: # (babysit_risk LONG_LINE_BYTES, `long-line-review:`). LONG_LINE_BYTES: "1800" run: | + # A designed stop: the reason, from this script's own literals, and + # the head, as the lane's outputs for the record job. + stop() { + echo "::notice title=pr-review-main stop::reason=$1 lane=$LANE pr=$PR_NUMBER; $2; no verdict, so the lanes stay unbound" + { echo "stop=$1"; echo "$LANE-stop=$1"; echo "$LANE-head=$HEAD_SHA"; } >>"$GITHUB_OUTPUT" + exit 0 + } export GIT_CONFIG_NOSYSTEM=1 GIT_CONFIG_GLOBAL=/dev/null mkdir -p "$DATA" git init -q "$SRC" @@ -1172,17 +1214,14 @@ jobs: git -C "$SRC" fetch -q --no-tags --filter=blob:none origin "$BASE_SHA" "$HEAD_SHA" python3 -I .base/.github/scripts/pr-review-main/write-diff.py "$SRC" "$BASE_SHA" "$HEAD_SHA" "$DATA/diff.patch" "$MAX_DIFF_BYTES" if grep -aqE '^(Binary files .* differ|GIT binary patch)$' "$DATA/diff.patch"; then - echo "::error::the diff holds a binary file the review cannot read; no verdict, so the lanes stay unbound" - exit 1 + stop binary "the diff holds a binary file the review cannot read" fi if [ "$(wc -c <"$DATA/diff.patch")" -gt "$MAX_DIFF_BYTES" ]; then - echo "::error::the diff is over $MAX_DIFF_BYTES bytes, more than one review reads whole; no verdict, so the lanes stay unbound" - exit 1 + stop too-large "the diff is $(wc -c <"$DATA/diff.patch") bytes, over the $MAX_DIFF_BYTES one review reads whole" fi # A diff line's text, past its +, - or space, longer than the cap. if ! LC_ALL=C awk -v max="$LONG_LINE_BYTES" 'length($0) - 1 > max { exit 3 }' "$DATA/diff.patch"; then - echo "::error::a diff line is over $LONG_LINE_BYTES bytes, more than the review reads whole; no verdict, so the lanes stay unbound and the merge gate holds it for a person" - exit 1 + stop long-line "a diff line is over $LONG_LINE_BYTES bytes, more than the review reads whole" fi # Parts the reviewer reads whole, each named in the prompt; the # verdict counts only when every part was read whole @@ -1209,7 +1248,7 @@ jobs: echo "diff: $(cat "$DATA"/diff/part-*.patch | wc -l) lines in $(find "$DATA/diff" -type f | wc -l) parts" - name: Select the trusted PR text - if: steps.diff.outcome == 'success' + if: steps.diff.outcome == 'success' && steps.diff.outputs.stop == '' env: API_BUDGET_TOKEN: github-token uses: ./.base/.github/actions/select-trusted-text # zizmor: ignore[self-repository] @@ -1224,7 +1263,7 @@ jobs: # in it (`../../reference/...`) resolves inside the data directory; the # rules the code lane names go beside the standards. - name: Copy the review criteria - if: steps.diff.outcome == 'success' + if: steps.diff.outcome == 'success' && steps.diff.outputs.stop == '' env: SKILL: ${{ matrix.skill }} DATA: ${{ github.workspace }}/pr-review-main-data @@ -1239,7 +1278,7 @@ jobs: - name: Resolve the review's model id: models - if: steps.diff.outcome == 'success' + if: steps.diff.outcome == 'success' && steps.diff.outputs.stop == '' uses: ./.base/.github/actions/resolve-config # zizmor: ignore[self-repository] with: model-route: ${{ matrix.route }} @@ -1250,7 +1289,7 @@ jobs: # set, nothing reviews and the lane stays unbound. - name: Check the credential id: credential - if: steps.diff.outcome == 'success' + if: steps.diff.outcome == 'success' && steps.diff.outputs.stop == '' env: PRESENT: ${{ secrets.FALLBACK_CLAUDE_CODE_OAUTH_TOKEN != '' }} SECRET_NAME: FALLBACK_CLAUDE_CODE_OAUTH_TOKEN @@ -1437,7 +1476,7 @@ jobs: # A reviewed pull request that left no verdict fails here, so the run shows it; the lane stays unbound either way. - name: Fail without a verdict - if: always() && steps.diff.outcome == 'success' && steps.credential.outputs.present == 'true' && steps.verdict.outcome != 'success' + if: always() && steps.diff.outcome == 'success' && steps.diff.outputs.stop == '' && steps.credential.outputs.present == 'true' && steps.verdict.outcome != 'success' run: | echo "::error::the review left no verdict for this head" exit 1 @@ -1457,6 +1496,30 @@ jobs: permissions: checks: write # the per-lane count check run on the reviewed head steps: + # A designed stop (a diff the review cannot read whole) leaves no + # verdict and no artifact; its record names the reason on the head, + # neutral like the counts. Only the leg that stopped sets a lane's pair. + - name: Record the stops + env: + GH_TOKEN: ${{ github.token }} + CODE_STOP: ${{ needs.review.outputs.claude-review-stop || needs.review-subscription.outputs.claude-review-stop || needs.review-fallback.outputs.claude-review-stop }} + CODE_HEAD: ${{ needs.review.outputs.claude-review-head || needs.review-subscription.outputs.claude-review-head || needs.review-fallback.outputs.claude-review-head }} + SECURITY_STOP: ${{ needs.review.outputs.claude-security-review-stop || needs.review-subscription.outputs.claude-security-review-stop || needs.review-fallback.outputs.claude-security-review-stop }} + SECURITY_HEAD: ${{ needs.review.outputs.claude-security-review-head || needs.review-subscription.outputs.claude-security-review-head || needs.review-fallback.outputs.claude-security-review-head }} + run: | + for lane in claude-review claude-security-review; do + if [ "$lane" = claude-review ]; then reason=$CODE_STOP head=$CODE_HEAD; else reason=$SECURITY_STOP head=$SECURITY_HEAD; fi + [ -z "$reason" ] && continue + [[ $reason =~ ^(too-large|binary|long-line)$ && $head =~ ^[0-9a-f]{40}$ ]] || { + echo "::error::a stop output is malformed for $lane"; exit 1; } + gh api -X POST "repos/$GITHUB_REPOSITORY/check-runs" \ + -f name="pr-review-main: $lane" -f head_sha="$head" -f status=completed \ + -f conclusion=neutral \ + -f "output[title]=verdict=none reason=$reason" \ + -f "output[summary]=pr-review-main did not review $head from main: the diff stopped the review ($reason), so it left no verdict." >/dev/null + echo "recorded $lane $head stop=$reason" + done + - name: Download this run's verdicts id: download continue-on-error: true diff --git a/docs/conventions/pr-pipeline/README.md b/docs/conventions/pr-pipeline/README.md index 5d522bad11..6cf86eb9c6 100644 --- a/docs/conventions/pr-pipeline/README.md +++ b/docs/conventions/pr-pipeline/README.md @@ -1101,7 +1101,13 @@ the rules that only tighten in lane uploads `verdict.json` as `pr-review-main---`, the tree read by git, never the model, and a separate job with no model posts its counts as the check run `pr-review-main: ` (titled - `findings= critical=`, conclusion neutral) on the reviewed head; it posts nothing else. + `findings= critical=`, conclusion neutral) on the reviewed head. A designed stop (a + binary file, a patch over the size cap or a line over 1800 bytes) leaves the leg green with no + verdict, and the record job posts it on the head as the same check run titled + `verdict=none reason=`, so each stop is a countable record; it + posts nothing else. A stop counts no CRITICAL finding and never binds a lane: with no verdict + the lanes stay unbound as `main-review-stop:`, a lane's reason. A forged stop only + renames that reason. The gate (`babysit_review_evidence._main_bound`) counts a verdict only when its run is that file, event `workflow_run`, on the default branch of this repository with a head the branch holds, and the verdict names that lane, pull request and tree. As with pr-verify's FAIL, a diff --git a/plugins/source-control/skills/babysit-prs/reference/safety.md b/plugins/source-control/skills/babysit-prs/reference/safety.md index 899b206b5c..d3c5eed858 100644 --- a/plugins/source-control/skills/babysit-prs/reference/safety.md +++ b/plugins/source-control/skills/babysit-prs/reference/safety.md @@ -1137,7 +1137,9 @@ any refusal is the blocker `unattended merge refused: `: `permission-widening-review`, and an added `pull_request_target` or `workflow_run` trigger as `privileged-trigger-review`, all of which a person clears. A pull request that changes a lane source binds its review lanes only on `pr-review-main.yml`'s verdict from main - (`babysit_review_evidence._main_bound`). + (`babysit_review_evidence._main_bound`); a designed stop that review recorded on the head + (`verdict=none reason=`) binds nothing and names the unbound reason + `main-review-stop:`. Which changes count as which is in that docstring. Check-only (no `--merge`) writes nothing and reports the whole decision under `unattended`. With diff --git a/plugins/source-control/skills/babysit-prs/scripts/babysit_gh.py b/plugins/source-control/skills/babysit-prs/scripts/babysit_gh.py index 4eccf7c26c..08d1a433b1 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_gh.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_gh.py @@ -2475,28 +2475,40 @@ def read(path: str, binary: bool = False) -> Any: # The check run pr-review-main.yml's record job posts per lane on each head it -# reviewed, titled with its counts. +# reviewed, titled with its counts, or with the designed stop that left that +# head without a verdict (a diff the review cannot read whole). MAIN_REVIEW_CHECK_TITLE = re.compile(r"findings=(\d{1,6}) critical=(\d{1,6})") +MAIN_REVIEW_STOP_TITLE = re.compile(r"verdict=none reason=(too-large|binary|long-line)") + +# One record: (findings, CRITICAL), ("stop", reason), or None when unread. +MainReviewRecord = tuple[int, int] | tuple[str, str] | None + + +def main_review_stop(record: MainReviewRecord) -> str | None: + """The stop reason a record carries, or None for any other record.""" + if isinstance(record, tuple) and len(record) == 2 and record[0] == "stop": + return str(record[1]) + return None def collect_main_review_counts( repo: str, - shas: Iterable[str], + shas: Mapping[str, Iterable[str]], names: Mapping[str, str], env_overrides: Mapping[str, str] | None = None, -) -> dict[str, dict[str, list[tuple[int, int] | None]]]: - """Per head SHA and lane, the (findings, CRITICAL) counts of every check - run named `names[lane]` on that head, None for a title that does not - read, counting only runs the github-actions app posted. `filter=all` - reads every run of the name, since the default - returns only the latest, which a re-run or a forged clean record would - replace. A failed read raises `RuntimeError`.""" - found: dict[str, dict[str, list[tuple[int, int] | None]]] = {} - for sha in dict.fromkeys(shas): - if not re.fullmatch(r"[0-9a-f]{40}", str(sha)): - raise RuntimeError(f"a pull request head is not a SHA: {sha!r}") - found[sha] = {} - for lane, name in names.items(): +) -> dict[str, dict[str, list[MainReviewRecord]]]: + """Per head SHA and lane, the records of every check run named + `names[lane]` on that head, read only on the heads `shas[lane]` lists: + (findings, CRITICAL) for a counts title, ("stop", reason) for a designed + stop's title, None for a title that does not read, counting only runs the + github-actions app posted. `filter=all` reads every run of the name, since + the default returns only the latest, which a re-run or a forged clean + record would replace. A failed read raises `RuntimeError`.""" + found: dict[str, dict[str, list[MainReviewRecord]]] = {} + for lane, name in names.items(): + for sha in dict.fromkeys(shas.get(lane, ())): + if not re.fullmatch(r"[0-9a-f]{40}", str(sha)): + raise RuntimeError(f"a pull request head is not a SHA: {sha!r}") runs = fetch_paginated_api( f"repos/{repo}/commits/{sha}/check-runs" f"?check_name={quote(name, safe='')}&filter=all&per_page=100", @@ -2504,31 +2516,38 @@ def collect_main_review_counts( items_key="check_runs", env_overrides=env_overrides, ) - counts: list[tuple[int, int] | None] = [] + counts: list[MainReviewRecord] = [] for check in runs: # Only the record job's own token posts as github-actions. app = json_object(check.get("app")).get("slug") if check.get("name") != name or app != "github-actions": continue title = str(json_object(check.get("output")).get("title") or "") - match = MAIN_REVIEW_CHECK_TITLE.fullmatch(title) - counts.append((int(match[1]), int(match[2])) if match else None) - found[sha][lane] = counts + if match := MAIN_REVIEW_CHECK_TITLE.fullmatch(title): + counts.append((int(match[1]), int(match[2]))) + elif match := MAIN_REVIEW_STOP_TITLE.fullmatch(title): + counts.append(("stop", match[1])) + else: + counts.append(None) + found.setdefault(sha, {})[lane] = counts return found def main_review_critical( - counts: Mapping[str, Mapping[str, list[tuple[int, int] | None]]], + counts: Mapping[str, Mapping[str, list[MainReviewRecord]]], names: Iterable[str], ) -> dict[str, int]: """Per lane, the most CRITICAL findings any head's record carries; a record whose title did not read counts one, so a doubtful record holds - rather than clears.""" + rather than clears. A designed stop carries no verdict and counts none.""" found = dict.fromkeys(names, 0) for by_lane in counts.values(): for lane in found: for each in by_lane.get(lane, ()): - found[lane] = max(found[lane], 1 if each is None else each[1]) + if each is None: + found[lane] = max(found[lane], 1) + elif main_review_stop(each) is None: + found[lane] = max(found[lane], int(each[1])) return found @@ -2543,8 +2562,12 @@ def collect_main_review_critical( has no delete, so a count recorded once stays readable after its verdict artifact expires or is deleted (`collect_main_review_counts`). A failed read raises `RuntimeError`.""" + heads = list(shas) return main_review_critical( - collect_main_review_counts(repo, shas, names, env_overrides), names + collect_main_review_counts( + repo, {lane: heads for lane in names}, names, env_overrides + ), + names, ) 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 dde4da3aa3..a775327b6a 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py @@ -187,6 +187,7 @@ collect_lane_runs, collect_main_review_counts, main_review_critical, + main_review_stop, collect_main_reviews, compare_shows_behind, fetch_base_compare, @@ -3174,11 +3175,20 @@ def _lane_evidence( ) except GraphQLUnavailableError: threads = None + head_sha = str(head.get("sha")) + head_file = _head_file(repo, head_sha) + base_sha = json_object(pr.get("base")).get("sha") + # A lane-source pull request binds the code lane on a main-run review + # alone (`bind()`), so only the security lane's runs, which its carried + # blocking count reads, are listed. + main = review_evidence.needs_main_review(changed_paths, head_file, base_sha) runs = collect_lane_runs( repo, number, str(head.get("ref")), - review_evidence.LANES.values(), + [review_evidence.LANES[review_evidence.SEVERITY_LANE]] + if main + else review_evidence.LANES.values(), str(pr.get("created_at") or ""), read_env, ) @@ -3192,9 +3202,6 @@ def human(actor: Any) -> bool: permission=permission, ) - head_sha = str(head.get("sha")) - head_file = _head_file(repo, head_sha) - base_sha = json_object(pr.get("base")).get("sha") main_reviews, main_context = _main_reviews( repo, number, @@ -3204,6 +3211,7 @@ def human(actor: Any) -> bool: base_sha, read_env, [str(item.get("sha")) for item in commits], + main=main, ) lanes = review_evidence.bind( head_sha, @@ -3230,14 +3238,19 @@ def _main_reviews( base_sha: Any, read_env: Mapping[str, str] | None, pull_request_heads: Sequence[str] = (), + *, + main: bool | None = None, ) -> tuple[list[dict[str, Any]] | None, dict[str, Any] | None]: """The main-run reviews `bind()` reads for a pull request that changes a lane source, with the context it checks them against (the head's tree, read from the commit, the default branch, and per lane the most CRITICAL findings the record job's check runs carry on any head of the pull request: its commits and the heads force pushes replaced), or - (None, None) when the pull request changes none, so no read is spent.""" - if not review_evidence.needs_main_review(changed_paths, head_file, base_sha): + (None, None) when the pull request changes none, so no read is spent. + `main` is the caller's `needs_main_review` answer, computed when None.""" + if main is None: + main = review_evidence.needs_main_review(changed_paths, head_file, base_sha) + if not main: return None, None branch = repository_default_branch(repo) if not branch: @@ -3260,12 +3273,21 @@ def _main_reviews( checks = { lane: review_evidence.main_review_check(lane) for lane in review_evidence.LANES } + # The security lane's CRITICAL carries from every head of the pull + # request. The code lane binds only beside its record on the head, and + # `_main_lane` never reads its carried count but for None, so its other + # heads cost no read. + every = [head_sha, *pull_request_heads, *_force_pushed_heads(repo, number)] counts = collect_main_review_counts( repo, - [head_sha, *pull_request_heads, *_force_pushed_heads(repo, number)], + { + lane: every if lane == review_evidence.SEVERITY_LANE else [head_sha] + for lane in checks + }, checks, read_env, ) + on_head = counts.get(head_sha, {}) return reviews, { "repo": repo, "pr": number, @@ -3275,7 +3297,19 @@ def _main_reviews( # The record job's counts on this head: a verdict binds only beside # one that matches it, so a run cancelled before `record` binds none. "head_records": { - lane: [list(c) for c in counts.get(head_sha, {}).get(lane, ()) if c] + lane: [ + list(c) + for c in on_head.get(lane, ()) + if c is not None and main_review_stop(c) is None + ] + for lane in checks + }, + # The designed stops the record job posted on this head (a diff the + # review cannot read whole): they name why no verdict binds. + "head_stops": { + lane: sorted( + {s for c in on_head.get(lane, ()) if (s := main_review_stop(c))} + ) for lane in checks }, } 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 50910860ff..d5e16e21a8 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 @@ -412,7 +412,8 @@ def _main_lane( """The lane's evidence for a pull request that changes a lane source: bound when a counted main-run review names the pinned head (`_main_bound`) and the recorded counts were read, else unbound as - `lane-source-changed`. The security lane's blocking count is the most of + `lane-source-changed`, or as `main-review-stop:` when no verdict + binds and the record job posted a designed stop on the head. The security lane's blocking count is the most of the main-run reviews' CRITICAL findings at this tree, any its pull_request review left open (`_carried_blocking`), and the most any main-run review recorded on another head of this pull request @@ -423,22 +424,35 @@ def _main_lane( except Exception: # noqa: BLE001 - any doubt is unbound found = None recorded = _recorded_critical(lane, context) + reason = "lane-source-changed" + if found is None and (stop := _head_stop(lane, context)): + reason = f"main-review-stop:{stop}" if lane != SEVERITY_LANE: if found is None or recorded is None: - return _unbound(lane, "lane-source-changed", head, *carried) + return _unbound(lane, reason, head, *carried) return LaneEvidence(lane, True, None, found[0], False, None) runs, comments, threads, is_human, head_file = carried left = _carried_blocking(head, runs, comments, threads, is_human, head_file) if found is None or recorded is None: held = max(left or 0, recorded or 0) - return LaneEvidence( - lane, False, "lane-source-changed", None, False, held or None - ) + return LaneEvidence(lane, False, reason, None, False, held or None) findings, critical = found blocking = max(critical, left or 0, recorded) return LaneEvidence(lane, True, None, max(findings, blocking), False, blocking) +def _head_stop(lane: str, context: Mapping[str, Any] | None) -> str | None: + """The first designed stop pr-review-main's record job posted for `lane` + on the pinned head (`main_context["head_stops"]`, read from the closed + set `babysit_gh.MAIN_REVIEW_STOP_TITLE` matches), or None. It only names + why no verdict binds: a stop never binds a lane.""" + found = context.get("head_stops") if isinstance(context, Mapping) else None + stops = found.get(lane) if isinstance(found, Mapping) else None + if not isinstance(stops, list | tuple): + return None + return next((each for each in stops if isinstance(each, str) and each), None) + + def _recorded_critical(lane: str, context: Mapping[str, Any] | None) -> int | None: """The most CRITICAL findings a main-run review recorded for `lane` on any head of this pull request (`main_context["carried_critical"]`), so a diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_gh.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_gh.py index 2b4d1f110e..ae68b3ece4 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_gh.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_gh.py @@ -1324,13 +1324,97 @@ def test_another_apps_run_of_the_name_is_no_record(self) -> None: ) self.assertEqual(counts["a" * 40]["claude-security-review"], [(1, 1)]) - def counts(self, by_sha: dict[str, list[dict[str, Any]]]) -> Any: + def counts( + self, + by_sha: dict[str, list[dict[str, Any]]], + shas: dict[str, list[str]] | None = None, + calls: list[str] | None = None, + ) -> Any: def fake(args: list[str], env: Any = None) -> Any: + if calls is not None: + calls.append(args[1]) sha = args[1].split("/commits/", 1)[1].split("/", 1)[0] - return [{"check_runs": by_sha.get(sha, [])}] + name = args[1].split("check_name=", 1)[1].split("&", 1)[0] + return [ + { + "check_runs": [ + run + for run in by_sha.get(sha, []) + if gh.quote(run["name"], safe="") == name + ] + } + ] + if shas is None: + shas = {lane: list(by_sha) for lane in self.NAMES} with mock.patch.object(gh, "gh_json", side_effect=fake): - return gh.collect_main_review_counts("owner/repo", list(by_sha), self.NAMES) + return gh.collect_main_review_counts("owner/repo", shas, self.NAMES) + + def test_a_stop_title_reads_as_the_stop_and_its_reason(self) -> None: + counts = self.counts( + { + "a" * 40: [ + self.check( + "claude-security-review", "verdict=none reason=too-large" + ), + self.check("claude-review", "verdict=none reason=other"), + ] + } + ) + self.assertEqual( + counts["a" * 40], + { + "claude-security-review": [("stop", "too-large")], + "claude-review": [None], + }, + ) + + def test_a_stop_counts_no_critical_and_an_unknown_reason_counts_one(self) -> None: + found = self.collect( + { + "a" * 40: [ + self.check("claude-security-review", "verdict=none reason=binary"), + self.check("claude-review", "verdict=none reason=other"), + ] + } + ) + self.assertEqual(found, {"claude-review": 1, "claude-security-review": 0}) + + def test_a_stop_on_one_head_leaves_another_heads_critical(self) -> None: + found = self.collect( + { + "a" * 40: [ + self.check( + "claude-security-review", "verdict=none reason=long-line" + ) + ], + "b" * 40: [ + self.check("claude-security-review", "findings=1 critical=1") + ], + } + ) + self.assertEqual(found["claude-security-review"], 1) + + def test_reads_each_lane_only_on_the_heads_listed_for_it(self) -> None: + calls: list[str] = [] + head, old = "a" * 40, "b" * 40 + counts = self.counts( + { + head: [self.check("claude-review", "findings=1 critical=0")], + old: [self.check("claude-security-review", "findings=2 critical=2")], + }, + {"claude-review": [head], "claude-security-review": [head, old, head]}, + calls, + ) + # One read per (head, lane) listed: 1 + 2, the repeated head once. + self.assertEqual(len(calls), 3) + self.assertEqual( + counts, + { + head: {"claude-review": [(1, 0)], "claude-security-review": []}, + old: {"claude-security-review": [(2, 2)]}, + }, + ) def test_takes_the_most_critical_findings_on_any_head(self) -> None: found = self.collect( diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge.py index 341f88d956..e345e148e9 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge.py @@ -2488,15 +2488,14 @@ def collect(repo, names, workflow, branch, env): merge, "collect_main_review_counts", side_effect=lambda repo, shas, names, env: ( - calls.append(("critical-counts", list(shas), names)) + calls.append(("critical-counts", dict(shas), names)) or { HEAD: { - "claude-review": [(2, 0)], + "claude-review": [(2, 0), ("stop", "too-large")], "claude-security-review": [(1, 0), None], }, "b" * 40: { - "claude-review": [], - "claude-security-review": [(3, 1)], + "claude-security-review": [(3, 1), ("stop", "binary")], }, } ), @@ -2529,14 +2528,24 @@ def test_a_lane_source_collects_by_the_heads_tree(self) -> None: "claude-review": [[2, 0]], "claude-security-review": [[1, 0]], }, + # A designed stop on this head, never one on another head. + "head_stops": { + "claude-review": ["too-large"], + "claude-security-review": [], + }, }, ) - # Every head of the pull request, force-pushed ones included. + # The security lane on every head of the pull request, force-pushed + # ones included; the code lane on the head alone, the only records + # its binding reads. self.assertEqual( calls[1], ( "critical-counts", - [HEAD, HEAD, "b" * 40], + { + "claude-review": [HEAD], + "claude-security-review": [HEAD, HEAD, "b" * 40], + }, { "claude-review": "pr-review-main: claude-review", "claude-security-review": "pr-review-main: claude-security-review", @@ -2557,6 +2566,56 @@ def test_a_lane_source_collects_by_the_heads_tree(self) -> None: ) +class LaneRunsReadTests(unittest.TestCase): + """`_lane_evidence` lists a lane's pull_request runs only when `bind()` + can read them: a lane-source pull request binds the code lane on a + main-run review alone, so only the security lane's runs are listed (its + carried blocking count reads them).""" + + def lanes_listed(self, changed: list[str]) -> list[Any]: + listed: list[Any] = [] + pr = { + "head": {"ref": "feat", "sha": HEAD}, + "base": {"sha": "b" * 40}, + "user": {"login": "someone"}, + "commits": 0, + } + with ( + mock.patch.object(merge, "fetch_paginated_api", return_value=[]), + mock.patch.object(merge, "fetch_review_threads", return_value=[]), + mock.patch.object( + merge, + "collect_lane_runs", + side_effect=lambda repo, number, ref, lanes, *rest: ( + listed.extend(lanes) or [] + ), + ), + mock.patch.object(merge, "_main_reviews", return_value=(None, None)), + ): + merge._lane_evidence( + "owner/repo", + 5, + pr, + "agent", + frozenset(), + lambda login: "write", + changed, + ) + return listed + + def test_a_lane_source_lists_only_the_security_lanes_runs(self) -> None: + self.assertEqual( + self.lanes_listed(["app.py", "AGENTS.md"]), + [merge.review_evidence.LANES["claude-security-review"]], + ) + + def test_any_other_pull_request_lists_every_lane(self) -> None: + self.assertEqual( + self.lanes_listed(["app.py"]), + list(merge.review_evidence.LANES.values()), + ) + + class ForcePushedHeadsTests(unittest.TestCase): """The heads force pushes replaced, whose recorded review counts still hold; a listing that is not whole raises.""" 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 ec8139806e..3243316903 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 @@ -1022,6 +1022,91 @@ def test_a_verdict_with_no_matching_record_on_the_head_does_not_bind( for lane in LANES: self.assertTrue(matched[lane].bound, lane) + def test_a_designed_stop_on_the_head_names_the_unbound_reason(self) -> None: + # pr-review-main stopped on a diff too large to read whole and posted + # `verdict=none reason=too-large`: no verdict, so nothing binds, and + # the reason says why; a CRITICAL another head carried still holds. + context = dict( + CONTEXT, + carried_critical={"claude-review": 0, "claude-security-review": 2}, + head_records={lane: [] for lane in LANES}, + head_stops={lane: ["too-large"] for lane in LANES}, + ) + result = self.main_bind(None, context=context) + for lane in LANES: + self.assertUnbound(result[lane], "main-review-stop:too-large") + self.assertEqual(result["claude-security-review"].blocking_findings, 2) + # The scorer reports the stop by name as a lane cause, the same MED + # unbound evidence as before; no person code is added for it. + scored = risk.score( + risk.RiskInput( + files=(), + files_complete=True, + max_changed_lines=None, + high_paths=(), + lanes=tuple(result.values()), + ) + ) + unbound = { + r.code: (r.severity, r.detail) + for r in scored.reasons + if r.code.startswith("evidence-unbound:") + } + self.assertEqual( + unbound, + { + f"evidence-unbound:{lane}": ( + risk.Risk.MED, + "not bound: main-review-stop:too-large", + ) + for lane in LANES + }, + ) + # The carried CRITICAL is the only reason a person reads. + self.assertEqual( + [r.code for r in risk.person_reasons(scored.reasons)], + ["security-finding"], + ) + + def test_a_verdict_on_the_head_binds_over_a_stop_beside_it(self) -> None: + # A re-run reviewed the head whole after an earlier run stopped on it. + reviews = [main_review(lane, findings=1, critical=0) for lane in LANES] + result = self.main_bind( + reviews, + context=dict( + CONTEXT, + head_records={lane: [[1, 0]] for lane in LANES}, + head_stops={lane: ["binary"] for lane in LANES}, + ), + ) + for lane in LANES: + self.assertTrue(result[lane].bound, lane) + self.assertIsNone(result[lane].unbound_reason) + + def test_the_code_lanes_pull_request_runs_change_nothing(self) -> None: + # Pin for the merge job's read cut: a lane-source pull request's + # evidence is the same whether or not the code lane's runs were + # listed, so the caller lists only the security lane's. + security = [whole("claude-security-review", S)] + both = [whole(lane, S) for lane in LANES] + for reviews in (None, [main_review(lane) for lane in LANES]): + with self.subTest(reviews=bool(reviews)): + results = [ + ev.bind( + S, + runs, + both_markers(), + [], + is_human=is_human, + changed_paths=("app.py", self.SOURCE), + head_file=fixed_head, + main_reviews=reviews, + main_context=recorded(reviews, CONTEXT), + ) + for runs in (security, both) + ] + self.assertEqual(results[0], results[1]) + def test_a_clean_main_run_review_of_the_head_binds_both_lanes(self) -> None: result = self.main_bind([main_review(lane) for lane in LANES]) for lane in LANES: