diff --git a/.changes/source-control/fix-lane-source-review-plugin-c60dcf4d.md b/.changes/source-control/fix-lane-source-review-plugin-c60dcf4d.md new file mode 100644 index 00000000000..e792fae4ffd --- /dev/null +++ b/.changes/source-control/fix-lane-source-review-plugin-c60dcf4d.md @@ -0,0 +1,7 @@ +--- +bump: patch +--- + +### Fixed + +- Babysit review evidence no longer unbinds both review lanes (`lane-source-changed`) for a pull request that changes `plugins/review/`. The reusable `pr-review-cli.yml` stages the review plugin from claude-code-plugins' default-branch tip, never the head, so a pull request's copy feeds no review. `scripts/ci-review-plugin-source.test.sh` fails if either review caller stops calling that reusable pinned to a commit SHA or passes it a plugin path. Every other lane-source path still unbinds. 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 c725ca9af9b..7f69f70efcd 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 @@ -51,9 +51,7 @@ # What a pull request's own checkout can feed or forge for its own review: # its workflows (a pull_request event runs the head's copy of the callers, and # any workflow it adds can post a github-actions[bot] reviewed-head marker), -# the review plugin path (the reusable stages it from main, so this is -# caution against a caller that loads it from the checkout), Claude Code project config and instruction files at -# any depth, the criteria the lane skills read from the checkout (REVIEW.md, +# Claude Code project config and instruction files at any depth, the criteria the lane skills read from the checkout (REVIEW.md, # the standards index, the instruction exception register, the # untrusted-content contract), the lane's input directory .claude-lane/ (the # code lane falls back to a context file there), and .gitattributes, which can @@ -61,10 +59,13 @@ # from a checkout of main and the reusable runs its composites from its own # repository, so the rest of .github/ feeds no review. A pull request that # changes any listed path may have changed its own review, so its records -# prove no review by the default branch's lane. +# prove no review by the default branch's lane. plugins/review/ is not listed: +# the reusable pr-review-cli.yml stages the review plugin from +# claude-code-plugins' default-branch tip, never the head, and +# scripts/ci-review-plugin-source.test.sh fails if a caller stops calling it +# pinned to a SHA or passes it a plugin path. LANE_SOURCE_PREFIXES = ( ".github/workflows/", - "plugins/review/", "docs/standards/", "docs/conventions/instruction-exception-register/", "docs/conventions/untrusted-content/", 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 485ab34b734..6a354d116ac 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 @@ -330,7 +330,6 @@ def test_a_change_to_a_file_the_review_reads_from_the_checkout(self) -> None: for path in ( ".github/workflows/pr-review-hosted.yml", ".github/workflows/pr-review-security-hosted.yml", - "plugins/review/skills/security-review/SKILL.md", ".claude/settings.json", "plugins/foo/.claude/rules/x.md", ".mcp.json", @@ -361,6 +360,35 @@ def test_a_change_to_a_file_the_review_reads_from_the_checkout(self) -> None: for lane in LANES: self.assertUnbound(result[lane], "lane-source-changed") + def test_a_review_plugin_change_binds_both_lanes(self) -> None: + # The reusable stages the review plugin from the default branch's + # tip, never the head (scripts/ci-review-plugin-source.test.sh pins + # the callers to that), so a pull request's copy feeds no review. + for path in ( + "plugins/review/skills/security-review/SKILL.md", + "plugins/review/skills/code-review/SKILL.md", + "plugins/review/agents/lane-verifier.md", + "plugins/review/.claude-plugin/plugin.json", + "plugins/review", + ): + with self.subTest(path): + result = self.bound("app.py", path) + for lane in LANES: + self.assertTrue(result[lane].bound, lane) + + def test_a_review_plugin_change_beside_lane_source_still_unbinds(self) -> None: + for other in ( + ".github/workflows/pr-review-hosted.yml", + "docs/standards/README.md", + ".claude-lane/pr-context.md", + "plugins/review/CLAUDE.md", + "plugins/review/.claude/settings.json", + ): + with self.subTest(other): + result = self.bound("plugins/review/skills/code-review/SKILL.md", other) + for lane in LANES: + self.assertUnbound(result[lane], "lane-source-changed") + def test_a_path_that_is_not_a_string_binds_no_lane(self) -> None: result = self.bound("app.py", None) # type: ignore[arg-type] for lane in LANES: diff --git a/scripts/ci-review-plugin-source.test.sh b/scripts/ci-review-plugin-source.test.sh new file mode 100755 index 00000000000..0fdae1758c1 --- /dev/null +++ b/scripts/ci-review-plugin-source.test.sh @@ -0,0 +1,79 @@ +#!/usr/bin/env bash +# Structure test: each review caller runs melodic-software/ci-workflows' +# pr-review-cli.yml pinned to a commit SHA and passes it no input naming a +# plugin path. That reusable stages the review plugin from claude-code-plugins' +# default-branch tip, never the pull request's head, so the review-evidence +# binder (babysit_review_evidence.py) does not list plugins/review/ as lane +# source. A caller that loaded the plugin from its checkout would let a pull +# request change its own review, and this test fails first. +# test-scope: .github/workflows/pr-review-hosted.yml .github/workflows/pr-review-security-hosted.yml plugins/source-control/skills/babysit-prs/scripts/babysit_review_evidence.py .github/standards/runner-policy/package.json +set -uo pipefail + +ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" +# shellcheck source=lib/test-harness.sh +. "$ROOT/scripts/lib/test-harness.sh" + +results=$( + cd "$ROOT" && node --input-type=module - <<'EOF' +import { readFileSync } from "node:fs"; +import { createRequire } from "node:module"; +const require = createRequire(`${process.cwd()}/.github/standards/runner-policy/package.json`); +const YAML = require("yaml"); + +const say = (good, text) => console.log(`${good ? "ok" : "fail"}\t${text}`); +const REUSABLE = "melodic-software/ci-workflows/.github/workflows/pr-review-cli.yml@"; + +// The problems with one caller's jobs, or [] when it is sound. +function problems(jobs) { + const calls = Object.entries(jobs ?? {}).filter( + ([, job]) => typeof job?.uses === "string" && job.uses.includes("pr-review-cli.yml"), + ); + if (calls.length !== 1) return [`${calls.length} jobs call pr-review-cli.yml, want 1`]; + const [id, job] = calls[0]; + const found = []; + if (!job.uses.startsWith(REUSABLE) || !/^[0-9a-f]{40}$/.test(job.uses.slice(REUSABLE.length))) { + found.push(`job ${id} uses '${job.uses}', not ${REUSABLE}<40-hex SHA>`); + } + for (const [key, value] of Object.entries(job.with ?? {})) { + if (/plugin/i.test(key) || /plugins?\//i.test(String(value))) { + found.push(`job ${id} passes ${key}: '${value}', a plugin path`); + } + } + return found; +} + +for (const file of ["pr-review-hosted.yml", "pr-review-security-hosted.yml"]) { + const wf = YAML.parse(readFileSync(`.github/workflows/${file}`, "utf8")); + const found = problems(wf.jobs); + say(found.length === 0, + `${file} calls pr-review-cli.yml at a commit SHA with no plugin path${found.length ? `: ${found.join("; ")}` : ""}`); +} + +// The check itself rejects each shape it guards against. +const sha = "4fbceb4b75ffd568dfa5db31de0d0e753c0c64e6"; +const call = (uses, inputs) => ({ review: { uses, with: inputs } }); +for (const [name, jobs] of [ + ["a tag ref", call(`${REUSABLE}v0.65.9`, {})], + ["a short SHA", call(`${REUSABLE}${sha.slice(0, 12)}`, {})], + ["another repository's copy", call(`someone/ci-workflows/.github/workflows/pr-review-cli.yml@${sha}`, {})], + ["a plugin-dirs input", call(`${REUSABLE}${sha}`, { "plugin-dirs": "plugins/review" })], + ["a plugin path in another input", call(`${REUSABLE}${sha}`, { extra: "./plugins/review" })], + ["no call to the reusable", { review: { "runs-on": "ubuntu-latest" } }], + ["two calls to the reusable", { ...call(`${REUSABLE}${sha}`, {}), again: { uses: `${REUSABLE}${sha}` } }], +]) { + say(problems(jobs).length > 0, `the check rejects ${name}`); +} +say(problems(call(`${REUSABLE}${sha}`, { lane: "code", "hold-label": "needs-human" })).length === 0, + "the check accepts a SHA-pinned call with no plugin path"); +EOF +) +status=$? +if [[ $status -ne 0 || -z "$results" ]]; then + fail "the node check did not run (exit $status)" +fi +while IFS=$'\t' read -r verdict text; do + [[ -n "$verdict" ]] || continue + if [[ "$verdict" == ok ]]; then ok "$text"; else fail "$text"; fi +done <<<"$results" + +test_harness::report