/` directory a kept thread's path sits in, except one under `hooks/`, `.claude-plugin/` or a `.claude/` directory, a `CLAUDE.md` or `AGENTS.md`, or a `.sh`, `.py`, `.mjs`, `.js` or `.ts` file, and a test file (`test_*.py`, `*_test.py`, `*.test.*`, `*.spec.*`) in the same directory as a kept thread's path. `.github/` stays refused. diff --git a/.github/scripts/pr-unblock/address-feedback.mjs b/.github/scripts/pr-unblock/address-feedback.mjs index b3aed64fa29..b8f3fc7da3e 100644 --- a/.github/scripts/pr-unblock/address-feedback.mjs +++ b/.github/scripts/pr-unblock/address-feedback.mjs @@ -22,6 +22,8 @@ import { appendFileSync, readFileSync, writeFileSync } from "node:fs"; import { pathToFileURL } from "node:url"; +import { isProtectedPath } from "./thread-scope.mjs"; + // The round cap is the config's `loop.review-rounds`, passed in as // REVIEW_ROUNDS; the resolver's default is the fallback. export const DEFAULT_MAX_ROUNDS = 2; @@ -39,10 +41,6 @@ const FILES_CAP = 3000; const firstLine = (body) => String(body ?? "").split("\n")[0].replace(/\r$/, ""); -export function isProtectedPath(path) { - return /^\.github(\/|$)/i.test(String(path)); -} - // Round markers the lanes App posted; any other author's copy is ignored. export const isRoundMarker = (comment, appId) => comment.user?.id === appId && firstLine(comment.body) === ROUND_MARKER; diff --git a/.github/scripts/pr-unblock/promote-scope.test.mjs b/.github/scripts/pr-unblock/promote-scope.test.mjs new file mode 100644 index 00000000000..ab6a1ad957d --- /dev/null +++ b/.github/scripts/pr-unblock/promote-scope.test.mjs @@ -0,0 +1,112 @@ +// Runs the review-threads path check of pr-run-activity-write.yml's promote +// step (the `script:` of the step with `id: promote`) against fake GitHub +// clients. A changed path in scope reaches the next gate, the hold-label read, +// which fails here because the case passes no hold labels. +import assert from "node:assert/strict"; +import { createHash } from "node:crypto"; +import { mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import { createRequire } from "node:module"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { after, test } from "node:test"; + +const WORKFLOW = new URL("../../workflows/pr-run-activity-write.yml", import.meta.url); +const SCOPE = new URL("./thread-scope.mjs", import.meta.url); +const HEAD = "a".repeat(40); +const LANE = "b".repeat(40); + +function promoteScript() { + const lines = readFileSync(WORKFLOW, "utf8").split("\n"); + const id = lines.findIndex((line) => /^\s+id: promote$/.test(line)); + assert.notEqual(id, -1, "the workflow has the promote step"); + const start = lines.findIndex((line, i) => i > id && /^\s+script: \|$/.test(line)); + const keyIndent = lines[start].search(/\S/); + const body = []; + for (const line of lines.slice(start + 1)) { + if (line.trim() !== "" && line.search(/\S/) <= keyIndent) break; + body.push(line); + } + const indent = Math.min(...body.filter((l) => l.trim()).map((l) => l.search(/\S/))); + return body.map((l) => l.slice(indent)).join("\n"); +} + +const dir = mkdtempSync(join(tmpdir(), "promote-scope-")); +after(() => rmSync(dir, { recursive: true, force: true })); + +async function promote(files, { scope = readFileSync(SCOPE), sha256 } = {}) { + const threadFile = join(dir, "threads.json"); + writeFileSync(threadFile, JSON.stringify({ threads: [{ path: "plugins/p/evals/c/case.yaml" }, { path: "src/round.py" }] })); + const scopeScript = join(dir, "thread-scope.mjs"); + writeFileSync(scopeScript, scope); + const env = { + HEAD_SHA: HEAD, + HEAD_REF: "feature", + LANE_BRANCH: "lane-commits/x", + MERGE_BASE: "false", + THREAD_FILE: threadFile, + SCOPE_SCRIPT: scopeScript, + SCOPE_SHA256: sha256 ?? createHash("sha256").update(readFileSync(SCOPE)).digest("hex"), + DEPENDENCY_REVIEW: "false", + HOLD_LABELS: "", + }; + const saved = { ...process.env }; + Object.assign(process.env, env); + const failed = []; + const core = { setOutput() {}, info() {}, warning() {}, setFailed: (message) => failed.push(message) }; + const github = { + request: async () => ({ data: { object: { sha: LANE } } }), + rest: { + repos: { compareCommitsWithBasehead: async () => ({ data: { files } }) }, + git: { deleteRef: async () => {} }, + }, + }; + const context = { repo: { owner: "o", repo: "r" } }; + try { + const AsyncFunction = (async () => {}).constructor; + await new AsyncFunction("github", "context", "core", "require", promoteScript())( + github, + context, + core, + createRequire(import.meta.url), + ); + } finally { + process.env = saved; + } + return failed; +} + +const HOLD = /^No hold labels were read/; +const OUT = /^The lane branch changes a path outside the kept threads' scope or under \.github\//; + +test("in-scope changes pass the path check: the thread's path, its plugin's files, a sibling test", async () => { + const failed = await promote([ + { filename: "plugins/p/evals/c/case.yaml" }, + { filename: "plugins/p/evals/c/samples/deep/new.json" }, + { filename: "plugins/p/evals/other/samples/new.json" }, + { filename: "plugins/p/README.md" }, + { filename: "src/test_round.py" }, + ]); + assert.equal(failed.length, 1); + assert.match(failed[0], HOLD); +}); + +for (const file of [ + { filename: "plugins/q/evals/c/samples/new.json" }, + { filename: "src/lib/test_round.py" }, + { filename: ".github/workflows/x.yml" }, + { filename: "plugins/p/evals/c/x.json", previous_filename: ".github/x.yml" }, +]) { + test(`an out-of-scope change stops the move: ${file.previous_filename ?? file.filename}`, async () => { + const failed = await promote([{ filename: "plugins/p/evals/c/case.yaml" }, file]); + assert.equal(failed.length, 1); + assert.match(failed[0], OUT); + }); +} + +test("a rewritten scope copy, or no recorded hash, stops the move", async () => { + const permissive = "export const inThreadScope = () => true;\n"; + for (const options of [{ scope: permissive }, { sha256: "" }]) { + const failed = await promote([{ filename: ".github/workflows/x.yml" }], options); + assert.deepEqual(failed, ["thread-scope.mjs does not match the base's copy; the PR branch is not moved"]); + } +}); diff --git a/.github/scripts/pr-unblock/publish-feedback.mjs b/.github/scripts/pr-unblock/publish-feedback.mjs index a2eed539121..9825930de76 100755 --- a/.github/scripts/pr-unblock/publish-feedback.mjs +++ b/.github/scripts/pr-unblock/publish-feedback.mjs @@ -10,7 +10,8 @@ // fields, within the limits below. Any failure posts no reply. // 2. For VALID replies, confirms every fix commit reached the PR branch // after the model job's gated head and that the branch changed no path -// but the kept threads' and none under .github/. +// outside the kept threads' scope (thread-scope.mjs) and none under +// .github/. // 3. Re-reads each thread and skips one that is resolved, moved, or has a // comment the model job did not see; posts the templated reply on its // root comment and resolves it only when it is VALID. The PR head is @@ -46,7 +47,6 @@ import { countRounds, createGitHubApi, currentThreads, - isProtectedPath, isRoundMarker, DEFAULT_MAX_ROUNDS, maxRoundsFrom, @@ -57,6 +57,7 @@ import { SHA, trustedHumansFrom, } from "./address-feedback.mjs"; +import { inThreadScope } from "./thread-scope.mjs"; export const LIMITS = { bytes: 262144, note: 2000, evidence: 10, files: 300 }; const THREAD_ID = /^PRRT_[A-Za-z0-9_-]{1,100}$/; @@ -168,9 +169,10 @@ export function renderReply({ disposition, evidence, fixSha, note }) { } // Whenever the PR branch moved since the model job's gated head, or a reply is -// VALID, that range may change only the kept threads' paths (run.json, whose -// SHA-256 the model job's runner recorded), and nothing under .github/. Every VALID -// fix must be a commit in that range that changes its own thread's path. +// VALID, that range may change only what inThreadScope allows around the kept +// threads' paths (run.json, whose SHA-256 the model job's runner recorded), and +// nothing under .github/. Every VALID fix must be a commit in that range that +// changes its own thread's path. async function fixesLanded({ api, run, headSha, replies }) { const valid = replies.filter((reply) => reply.disposition === "VALID"); if (valid.length === 0 && run.head_sha === headSha) { @@ -187,7 +189,7 @@ async function fixesLanded({ api, run, headSha, replies }) { files.length < LIMITS.files && files.every((file) => [file.filename, file.previous_filename].every( - (path) => path === undefined || (paths.has(path) && !isProtectedPath(path)), + (path) => path === undefined || inThreadScope(path, paths), ), ); if (!inRange) { diff --git a/.github/scripts/pr-unblock/publish-feedback.test.mjs b/.github/scripts/pr-unblock/publish-feedback.test.mjs index d799f61770a..c8a0255e125 100644 --- a/.github/scripts/pr-unblock/publish-feedback.test.mjs +++ b/.github/scripts/pr-unblock/publish-feedback.test.mjs @@ -422,6 +422,23 @@ for (const [label, compare] of [ }); } +test("a VALID fix that also adds a file in its thread's plugin lands; another plugin's file does not", async () => { + const caseRun = JSON.stringify({ ...RUN, threads: [{ ...RUN.threads[0], path: "plugins/p/evals/c/case.yaml" }] }); + for (const [sample, landed] of [ + ["plugins/p/evals/c/samples/new.json", true], + ["plugins/p/evals/other/samples/new.json", true], + ["plugins/q/evals/c/samples/new.json", false], + ]) { + const api = fakeApi({ + compare: { status: "ahead", commits: [{ sha: FIX }], files: [{ filename: "plugins/p/evals/c/case.yaml" }, { filename: sample }] }, + commits: { [FIX]: { files: [{ filename: "plugins/p/evals/c/case.yaml" }, { filename: sample }] } }, + }); + const result = await run(api, lines(reply({ disposition: "VALID", fix_sha: FIX })), caseRun); + assert.equal(result.escalationReason, landed ? "" : "feedback-not-landed", sample); + assert.equal(api.posted().length, landed ? 1 : 0, sample); + } +}); + test("feedback-not-landed, nothing posted: a VALID fix commit that does not change its own thread's path", async () => { const api = fakeApi({ compare: { status: "ahead", commits: [{ sha: FIX }], files: [{ filename: "docs/b.md" }] }, diff --git a/.github/scripts/pr-unblock/thread-scope.mjs b/.github/scripts/pr-unblock/thread-scope.mjs new file mode 100644 index 00000000000..2aab10f4da2 --- /dev/null +++ b/.github/scripts/pr-unblock/thread-scope.mjs @@ -0,0 +1,41 @@ +// Which paths an address-feedback fix may change, shared by the promote step +// of .github/workflows/pr-run-activity-write.yml (which imports a SHA-256 +// pinned copy, so it imports nothing itself) and publish-feedback.mjs. + +export function isProtectedPath(path) { + return /^\.github(\/|$)/i.test(String(path)); +} + +const PLUGIN_DIR = /^plugins\/[^/]+\//; +// In the plugin directory but run or loaded as instructions by the harness or +// a shell, so out of the plugin-wide part: hooks, manifests, script or code +// files, and agent rules (CLAUDE.md, AGENTS.md, anything under .claude/). +const PLUGIN_CODE = + /^plugins\/[^/]+\/(hooks|\.claude-plugin)\/|\.(sh|py|mjs|js|ts)$|(^|\/)(claude|agents)\.md$|(^|\/)\.claude\//i; +const TEST_FILE = /^(test_[^/]*\.py|[^/]*_test\.py|[^/]*\.(test|spec)\.[^/]+)$/; +const dirOf = (path) => path.slice(0, path.lastIndexOf("/") + 1); + +// An empty, . or .. segment, so a leading or doubled / too. +const BAD_SEGMENT = /(^|\/)\.{0,2}(\/|$)/; + +// A kept thread's own path, a file in the plugins/
/ directory a kept +// thread's path sits in that is not under hooks/, .claude-plugin/ or a +// .claude/ directory, not a CLAUDE.md or AGENTS.md, and not a .sh, .py, .mjs, +// .js or .ts file, or a test file in the same directory as a +// kept thread's path; never anything under .github/, and never a path that is +// not in normal form. +export function inThreadScope(path, threadPaths) { + if (isProtectedPath(path) || BAD_SEGMENT.test(path)) { + return false; + } + if (threadPaths.has(path)) { + return true; + } + const kept = [...threadPaths]; + const plugin = PLUGIN_DIR.exec(path)?.[0]; + if (plugin !== undefined && !PLUGIN_CODE.test(path) && kept.some((p) => p.startsWith(plugin))) { + return true; + } + const dir = dirOf(path); + return TEST_FILE.test(path.slice(dir.length)) && kept.some((p) => dirOf(p) === dir); +} diff --git a/.github/scripts/pr-unblock/thread-scope.test.mjs b/.github/scripts/pr-unblock/thread-scope.test.mjs new file mode 100644 index 00000000000..87582f7cd3a --- /dev/null +++ b/.github/scripts/pr-unblock/thread-scope.test.mjs @@ -0,0 +1,131 @@ +import assert from "node:assert/strict"; +import { test } from "node:test"; + +import { inThreadScope, isProtectedPath } from "./thread-scope.mjs"; + +const kept = new Set(["plugins/p/evals/c/graders/x.md", "src/a.js"]); + +test("a kept thread's own path is in scope", () => { + assert.equal(inThreadScope("src/a.js", kept), true); + assert.equal(inThreadScope("plugins/p/evals/c/graders/x.md", kept), true); +}); + +// plugins/planning/README.md is the file #8178's run 39117146913 refused. +test("any file in the same plugins/
/ directory as a kept thread's path is in scope, at any depth", () => { + for (const path of [ + "plugins/p/evals/c/samples/y.json", + "plugins/p/evals/other/samples/deep/z.json", + "plugins/p/evals/evals.json", + "plugins/p/skills/s/SKILL.md", + "plugins/p/README.md", + ]) { + assert.equal(inThreadScope(path, kept), true, path); + } + assert.equal(inThreadScope("plugins/planning/README.md", new Set(["plugins/planning/surface/round.py"])), true); +}); + +// Each refused path and the kept thread paths of the four recorded trips. +test("the four recorded trips are in scope", () => { + const trips = [ + // #8043, run 39100052790: an eval sample beside the thread's case. + ["plugins/session-flow/evals/rr-skill-fit-names-owner/samples/names-skill-and-plugin.json", ["plugins/session-flow/evals/rr-skill-fit-names-owner/case.yaml"]], + // #8103, run 39107762307: the thread is on case.yaml:18. + ["plugins/discipline/evals/ror-second-error-way/samples/second-way-named.json", ["plugins/discipline/evals/ror-second-error-way/case.yaml"]], + // #8142, run 39113747960: a .py file, in scope only as a sibling test. + ["plugins/planning/surface/test_round.py", ["plugins/planning/surface/round.py"]], + // #8178, run 39117146913: the PR's kept thread paths. + [ + "plugins/planning/README.md", + ["plugins/session-flow/skills/retro/SKILL.md", "plugins/planning/skills/draft-goal-condition/SKILL.md", "plugins/planning/.claude-plugin/plugin.json"], + ], + ]; + for (const [path, threads] of trips) { + assert.equal(inThreadScope(path, new Set(threads)), true, path); + } +}); + +test("hooks, manifests, agent rules and non-test code in the same plugin are refused", () => { + const named = new Set(["plugins/p/skills/s/SKILL.md"]); + for (const path of [ + "plugins/p/hooks/hooks.json", + "plugins/p/hooks/guard.md", + "plugins/p/.claude-plugin/plugin.json", + "plugins/p/scripts/run.sh", + "plugins/p/skills/s/scripts/helper.py", + "plugins/p/lib/x.mjs", + "plugins/p/lib/x.js", + "plugins/p/lib/x.ts", + "plugins/p/lib/X.SH", + "plugins/p/AGENTS.md", + "plugins/p/skills/s/CLAUDE.md", + "plugins/p/.claude/rules/r.md", + "plugins/p/.claude/settings.json", + ]) { + assert.equal(inThreadScope(path, named), false, path); + } +}); + +test("a thread's own path stays in scope whatever its type, and a sibling test of code stays in scope", () => { + for (const path of ["plugins/p/hooks/guard.sh", "plugins/p/.claude-plugin/plugin.json", "plugins/p/AGENTS.md"]) { + assert.equal(inThreadScope(path, new Set([path])), true, path); + } + assert.equal(inThreadScope("plugins/p/hooks/guard.test.mjs", new Set(["plugins/p/hooks/guard.mjs"])), true); + assert.equal(inThreadScope("plugins/p/scripts/test_run.py", new Set(["plugins/p/README.md"])), false); +}); + +test("another plugin's file and an unnamed file outside plugins/ stay out of scope", () => { + assert.equal(inThreadScope("plugins/q/evals/c/samples/y.json", kept), false); + assert.equal(inThreadScope("plugins/pp/README.md", kept), false); + assert.equal(inThreadScope("src/b.js", kept), false); + assert.equal(inThreadScope("plugins/README.md", kept), false); +}); + +test("with no kept thread under the plugin, its files are refused", () => { + assert.equal(inThreadScope("plugins/p/evals/c/samples/y.json", new Set(["src/a.js"])), false); +}); + +// Names from the coordinator's addition; plugins/planning/surface/test_round.py +// is the file #8142's run 39113747960 refused. +test("a test file in the same directory as a kept thread's path is in scope", () => { + const surface = new Set(["plugins/planning/surface/round.py", "src/a.js", "README.md"]); + for (const path of [ + "plugins/planning/surface/test_round.py", + "plugins/planning/surface/round_test.py", + "src/a.test.js", + "src/a.spec.ts", + "test_root.py", + ]) { + assert.equal(inThreadScope(path, surface), true, path); + } +}); + +test("outside plugins/, a test file in another directory, or a same-directory file that is no test, stays out of scope", () => { + const surface = new Set(["src/a.js"]); + for (const path of ["src/lib/a.test.js", "src/helper.js", "src/testing.js", "src/test_a.sh", "test_root.py"]) { + assert.equal(inThreadScope(path, surface), false, path); + } +}); + +test("a test file under .github/ is refused even beside a kept thread's path there", () => { + assert.equal(inThreadScope(".github/scripts/x.test.mjs", new Set([".github/scripts/x.mjs"])), false); +}); + +test("a path with an empty, . or .. segment, or a leading /, is refused before any other rule", () => { + for (const path of [ + "plugins/p/evals/c/../../../../.github/workflows/x.yml", + "plugins/p/evals/c/./samples/y.json", + "plugins/p/evals/c//y.json", + "/src/a.js", + "./src/a.js", + ]) { + assert.equal(inThreadScope(path, new Set([...kept, path])), false, path); + } +}); + +test(".github/ is refused even when a kept thread names a file there", () => { + const github = new Set([".github/workflows/x.yml", ".GitHub/evals/c/a.md"]); + assert.equal(inThreadScope(".github/workflows/x.yml", github), false); + assert.equal(inThreadScope(".GitHub/evals/c/b.md", github), false); + assert.equal(isProtectedPath(".github"), true); + assert.equal(isProtectedPath(".githubx/a"), false); +}); diff --git a/.github/workflows/pr-run-activity-write.yml b/.github/workflows/pr-run-activity-write.yml index 0ac7bb7ab5a..8ca64b3e402 100644 --- a/.github/workflows/pr-run-activity-write.yml +++ b/.github/workflows/pr-run-activity-write.yml @@ -507,7 +507,9 @@ jobs: # on a head a review lane's marker confirms, left by it alone or answered # by a trusted person after the lane escalated them, then # skips (none), escalates (a .github/** diff, or the round cap) or - # writes the thread file the skill's review-thread tool reads. + # writes the thread file the skill's review-thread tool reads. It also + # copies out the base's thread-scope.mjs with its SHA-256, which the + # promote step checks before it imports the copy. - name: Select the review threads id: feedback-select if: steps.config.outputs.applies == 'true' && steps.config.outputs.prepare == 'review-threads' @@ -518,11 +520,15 @@ jobs: APP_SENDER_ID: ${{ vars.AUTOMATION_LANES_APP_SENDER_ID }} LANE_BRANCH: ${{ format('lane-commits/{0}/{1}/{2}-{3}', inputs.lane, inputs.activity, github.run_id, github.run_attempt) }} FEEDBACK_DIR: ${{ runner.temp }}/lane-feedback + SCOPE_BIN: ${{ runner.temp }}/lane-feedback-bin TRUSTED_ACTORS: .base/.github/standards/trusted-actors/trusted-actors.json TRACKER_CONFIG: .base/.work-item-tracker.json RESOLVED: ${{ runner.temp }}/resolved-config.json run: | - mkdir -p "$FEEDBACK_DIR" + mkdir -p "$FEEDBACK_DIR" "$SCOPE_BIN" + cp .base/.github/scripts/pr-unblock/thread-scope.mjs "$SCOPE_BIN/thread-scope.mjs" + sum=$(sha256sum <"$SCOPE_BIN/thread-scope.mjs") + echo "scope-sha256=${sum%% *}" >>"$GITHUB_OUTPUT" REVIEW_ROUNDS=$(jq -r '.loop["review-rounds"]' "$RESOLVED") export REVIEW_ROUNDS THREAD_FILE="$FEEDBACK_DIR/threads.json" node .base/.github/scripts/pr-unblock/address-feedback.mjs select @@ -1484,11 +1490,14 @@ jobs: MERGE_BASE: ${{ steps.config.outputs.prepare == 'merge-base' }} COMMIT_OUTCOME: ${{ steps.commit.outcome }} COMMIT_SHA: ${{ steps.commit.outputs.commit }} - # review-threads mode moves the PR branch only when the lane branch - # changes nothing but the kept threads' paths, none under .github/. + # review-threads mode moves the PR branch only when every path the + # lane branch changes is in the kept threads' scope, as the base's + # thread-scope.mjs decides, and none is under .github/. THREAD_FILE: >- ${{ steps.config.outputs.prepare == 'review-threads' && format('{0}/lane-feedback/threads.json', runner.temp) || '' }} + SCOPE_SCRIPT: ${{ runner.temp }}/lane-feedback-bin/thread-scope.mjs + SCOPE_SHA256: ${{ steps.feedback-select.outputs.scope-sha256 }} # dependency-review mode moves the PR branch only when the lane # branch changes nothing under .github/ and every commit's message # carries [dependabot skip], so Dependabot may still rebase the PR. @@ -1505,6 +1514,7 @@ jobs: script: | const { HEAD_SHA, HEAD_REF, LANE_BRANCH, MERGE_BASE, COMMIT_OUTCOME, COMMIT_SHA, THREAD_FILE } = process.env; const { DEPENDENCY_REVIEW, DEPENDENCY_OUTPUT, PR_NUMBER, HOLD_LABELS, READ_TOKEN } = process.env; + const { SCOPE_SCRIPT, SCOPE_SHA256 } = process.env; core.setOutput('skip-reason', ''); const tip = async (branch) => (await github.request('GET /repos/{owner}/{repo}/git/ref/{ref}', { ...context.repo, ref: `heads/${branch}` })) @@ -1520,13 +1530,22 @@ jobs: return; } if (THREAD_FILE) { - const paths = new Set(JSON.parse(require('fs').readFileSync(THREAD_FILE, 'utf8')).threads.map((t) => t.path)); + const fs = require('fs'); + const paths = new Set(JSON.parse(fs.readFileSync(THREAD_FILE, 'utf8')).threads.map((t) => t.path)); + // Imported from the bytes just hashed, so a rewrite after the + // check cannot reach the import. + const scope = fs.readFileSync(SCOPE_SCRIPT); + if (!SCOPE_SHA256 || require('crypto').createHash('sha256').update(scope).digest('hex') !== SCOPE_SHA256) { + core.setFailed("thread-scope.mjs does not match the base's copy; the PR branch is not moved"); + return; + } + const { inThreadScope } = await import(`data:text/javascript;base64,${scope.toString('base64')}`); const { data } = await github.rest.repos.compareCommitsWithBasehead({ ...context.repo, basehead: `${HEAD_SHA}...${lane}` }); const bad = data.files.length >= 300 ? ['(300 or more files)'] : data.files .flatMap((f) => [f.filename, f.previous_filename].filter(Boolean)) - .filter((p) => !paths.has(p) || /^\.github(\/|$)/i.test(p)); + .filter((p) => !inThreadScope(p, paths)); if (bad.length > 0) { - core.setFailed(`The lane branch changes a path no kept thread names or under .github/ (${bad.slice(0, 5).join(', ')}); the PR branch is not moved`); + core.setFailed(`The lane branch changes a path outside the kept threads' scope or under .github/ (${bad.slice(0, 5).join(', ')}); the PR branch is not moved`); return; } } diff --git a/docs/conventions/pr-pipeline/CHANGELOG.md b/docs/conventions/pr-pipeline/CHANGELOG.md index 0ca3e572d3b..41e0e463886 100644 --- a/docs/conventions/pr-pipeline/CHANGELOG.md +++ b/docs/conventions/pr-pipeline/CHANGELOG.md @@ -1,5 +1,17 @@ # Changelog for the PR pipeline convention +## 2.12.3 - 2026-10-18 + +`pr-unblock` `address-feedback` widens a kept thread's scope from its path alone to also a file in +the `plugins/
/` directory that path sits in, and a test file (`test_*.py`, `*_test.py`, +`*.test.*`, `*.spec.*`) in that path's directory +([#8109](https://github.com/melodic-software/claude-code-plugins/issues/8109)). The plugin-wide +part leaves out files the harness or a shell runs or loads as instructions: anything under +`hooks/`, `.claude-plugin/` or a `.claude/` directory, a `CLAUDE.md` or `AGENTS.md`, and any +`.sh`, `.py`, `.mjs`, `.js` or `.ts` file. The review-thread tool, the promote step and the reply +job apply the same rule; the promote step imports the base's `thread-scope.mjs` from a copy taken +before the head checkout, checked against its SHA-256. `.github/` stays refused. `version` stays 2. + ## 2.12.2 - 2026-10-18 On a pull request labeled `needs-human` (the human-gated role label), `pr-unblock` diff --git a/docs/conventions/pr-pipeline/pr-run-activity.md b/docs/conventions/pr-pipeline/pr-run-activity.md index 07debb3b560..11dde0b9dfb 100644 --- a/docs/conventions/pr-pipeline/pr-run-activity.md +++ b/docs/conventions/pr-pipeline/pr-run-activity.md @@ -612,9 +612,14 @@ the base's `.github/scripts/pr-unblock/address-feedback.mjs` and `publish-feedba and Write are denied on the directory and on the tool's replies directory. The tool lists the kept threads, checks paths, and records a reply: it refuses a disposition file outside the scratch directory or off the template, and a `VALID` reply whose fix is not on the lane branch - or whose lane branch changes a file no kept thread names or any `.github/` path. It posts - nothing. The promote step then moves the PR branch only when the lane branch changes nothing - but kept-thread paths, none under `.github/`, in fewer than 300 files. + or whose lane branch changes a file outside every kept thread's scope or any `.github/` path. + A thread's scope is its path; a file in the `plugins/
/` directory its path sits in, except
+ one under `hooks/`, `.claude-plugin/` or a `.claude/` directory, a `CLAUDE.md` or `AGENTS.md`,
+ or a `.sh`, `.py`, `.mjs`, `.js` or `.ts` file; and a test file (`test_*.py`, `*_test.py`, `*.test.*`, `*.spec.*`) in its path's
+ directory. It posts nothing. The promote step then moves the PR branch only when every path the
+ lane branch changes is in a kept thread's scope, none under `.github/`, in fewer than 300
+ files, judged by a copy of the base's `thread-scope.mjs` taken before the head checkout and
+ imported only when its SHA-256 matches.
3. After the promote step, and after a skill that failed, was cancelled or timed out, a pinned
`github-script` step writes `run.json` (the gated head and the kept threads' ids, paths and
comment ids, from the thread file) beside a copy of the tool's `replies.jsonl`, and the job
@@ -641,7 +646,7 @@ the base's `.github/scripts/pr-unblock/address-feedback.mjs` and `publish-feedba
and escalates `feedback-incomplete`;
- when a reply is `VALID` or the PR branch moved, compares the model job's gated head with the
PR's current head: the branch must be ahead, hold every fix commit, and change fewer than 300
- files, each a kept thread's path and none under `.github/`, and each fix commit must change
+ files, each in a kept thread's scope and none under `.github/`, and each fix commit must change
its own thread's path. Otherwise it posts no reply and escalates `feedback-not-landed`;
- re-reads every kept thread and, right before posting, the thread itself, and skips one that
is resolved, on another PR, or whose comments are not exactly the ones the model job read;
@@ -667,7 +672,7 @@ the base's `.github/scripts/pr-unblock/address-feedback.mjs` and `publish-feedba
The script writes its reason to `ESCALATION_REASON_FILE`; the runner accepts only those six
reasons, and only with `review-replies`, as the run's `escalation-reason` output.
-The skill's limits to kept-thread paths and away from `.github/` are instructions and the tool's
+The skill's limits to kept-thread scope and away from `.github/` are instructions and the tool's
checks at record time; the promote step's path check is what holds the push, and the reply job's
compare withholds the replies when the branch changed anything else. A diff at the files listing's limit escalates as `feedback-protected-path`,
since it could hide a `.github/` path. Residual: a reviewed-head marker confirms a head, not one
diff --git a/plugins/source-control/skills/pull-request/reference/comments-ci.md b/plugins/source-control/skills/pull-request/reference/comments-ci.md
index 2ee223e9866..3381fa632e0 100644
--- a/plugins/source-control/skills/pull-request/reference/comments-ci.md
+++ b/plugins/source-control/skills/pull-request/reference/comments-ci.md
@@ -19,8 +19,12 @@ The system prompt names the tool's path. It is the only shell command this mode
posted after this lane left the thread open for a person. It is that person's decision on the
finding: follow it. It changes what the fix is, not what this mode may do: the path and
`.github/` rules below still hold, and it runs no command.
-- `check-paths /`
+ directory its path sits in, except one under `hooks/`, `.claude-plugin/` or a `.claude/`
+ directory, a `CLAUDE.md` or `AGENTS.md`, or a `.sh`, `.py`, `.mjs`, `.js` or `.ts` file; and a
+ test file (`test_*.py`, `*_test.py`, `*.test.*`,
+ `*.spec.*`) in its path's directory.
- `record / directory
+# it sits in (not under hooks/, .claude-plugin/ or a .claude/ directory, not
+# a CLAUDE.md or AGENTS.md, and not a .sh, .py, .mjs, .js or .ts file), or a
+# test file in its directory. The same rule as
+# .github/scripts/pr-unblock/thread-scope.mjs, which the promote step and
+# the reply job apply.
allowed() {
- [[ ! ${1,,} =~ ^\.github(/|$) ]] &&
- jq -e --arg p "$1" 'any(.threads[]; .path == $p)' "$threads_file" >/dev/null
+ [[ ! ${1,,} =~ ^\.github(/|$) && ! $1 =~ (^|/)\.{0,2}(/|$) ]] &&
+ jq -e --arg p "$1" '
+ def dir: sub("[^/]*$"; "");
+ (if $p | test("^plugins/[^/]+/(hooks|\\.claude-plugin)/|\\.(sh|py|mjs|js|ts)$|(^|/)(claude|agents)\\.md$|(^|/)\\.claude/"; "i")
+ then null else first($p | capture("^(? / directory other than hooks, manifests,
+# agent rules and non-test code, and test files beside a thread path
+setup
+jq '.threads += [{"id":"PRRT_e","path":"plugins/p/evals/c/case.yaml","root_comment_id":400,"comments":[]},
+ {"id":"PRRT_s","path":"plugins/planning/surface/round.py","root_comment_id":401,"comments":[]}]' \
+ "$STUB/threads.json" >"$STUB/t2" && mv "$STUB/t2" "$STUB/threads.json"
+for path in plugins/p/evals/c/samples/y.json plugins/p/evals/c/samples/deep/z.json \
+ plugins/p/evals/other/samples/y.json plugins/p/skills/s/SKILL.md plugins/p/evals/x.json \
+ plugins/planning/README.md plugins/planning/surface/test_round.py plugins/planning/surface/round_test.py src/a.test.js src/a.spec.ts; do
+ run check-paths "$path" >/dev/null 2>&1
+ assert_exit "check-paths allows $path" 0 "$?"
+done
+for path in plugins/q/evals/c/samples/y.json plugins/pp/README.md plugins/README.md \
+ plugins/p/hooks/hooks.json plugins/p/.claude-plugin/plugin.json plugins/p/scripts/run.sh \
+ plugins/p/AGENTS.md plugins/p/skills/s/CLAUDE.md plugins/p/.claude/rules/r.md plugins/p/lib/X.TS \
+ plugins/planning/surface/helper.py plugins/planning/test_round.py \
+ src/lib/a.test.js src/helper.js src/test_a.sh test_root.py \
+ plugins/p/evals/c/../../../../.github/workflows/x.yml plugins/p/evals/c/./y.json plugins/p/evals/c//y.json \
+ /src/a.js ./src/a.js; do
+ run check-paths "$path" >/dev/null 2>&1
+ assert_exit "check-paths refuses $path" 1 "$?"
+done
# record VALID: checked against the lane branch and appended, nothing posted
setup