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

- 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.
Original file line number Diff line number Diff line change
Expand Up @@ -51,20 +51,21 @@
# 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
# hide a hunk from the lane's `git diff`. The callers run every local action
# 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/",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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:
Expand Down
79 changes: 79 additions & 0 deletions scripts/ci-review-plugin-source.test.sh
Original file line number Diff line number Diff line change
@@ -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
Loading