diff --git a/.changes/pr-pipeline/feat-address-feedback-pr-paths-7c41e2a9.md b/.changes/pr-pipeline/feat-address-feedback-pr-paths-7c41e2a9.md new file mode 100644 index 0000000000..7ec01cfbde --- /dev/null +++ b/.changes/pr-pipeline/feat-address-feedback-pr-paths-7c41e2a9.md @@ -0,0 +1,7 @@ +--- +bump: minor +--- + +### Changed + +- `address-feedback` may fix a finding in any file the pull request already changes that the gated head holds, not only in the thread's own scope. The select job writes the PR's files, rename sources included, as `pr_paths` in the thread file and `run.json`, less `.github/workflows/`, `.gitmodules`, `CODEOWNERS`, `settings*.json`, package manifests, `requirements*.txt`, anything under `.github/`, `CLAUDE.md`, `AGENTS.md`, `.mcp.json`, `.lsp.json`, a `.claude` or `.claude-plugin` directory and a plugin's `hooks/`, `monitors/` and `bin/`. `apply-patch.mjs`'s `kept-threads` rule and the reply job admit them, and only as changes to an existing file: a file the fix creates keeps the old scope. A `VALID` fix commit lands when it changes the thread's path or a `pr_paths` file at a line the reply's evidence cites inside one of its hunks. Every apply-patch refusal still applies. diff --git a/.changes/source-control/feat-address-feedback-pr-paths-3e9b0d51.md b/.changes/source-control/feat-address-feedback-pr-paths-3e9b0d51.md new file mode 100644 index 0000000000..47dec92987 --- /dev/null +++ b/.changes/source-control/feat-address-feedback-pr-paths-3e9b0d51.md @@ -0,0 +1,7 @@ +--- +bump: minor +--- + +### Changed + +- The review-thread tool (`feedback-thread.sh`) of `/source-control:pull-request comments --ci` admits a file in the thread file's `pr_paths`, the files the pull request already changes, in every kept thread's scope while the gated head holds it. `reference/comments-ci.md` tells the model to run `check-paths` before calling a fix out of scope, and to cite a changed line in such a file as evidence. diff --git a/.github/scripts/apply-patch/apply-patch.mjs b/.github/scripts/apply-patch/apply-patch.mjs index e3ce65a5f9..11ae628029 100755 --- a/.github/scripts/apply-patch/apply-patch.mjs +++ b/.github/scripts/apply-patch/apply-patch.mjs @@ -637,7 +637,7 @@ async function pushMerge({ api, cwd, gitBin, row, context, number, target, headS return escalate("invalid", `the patch changes ${entries.length} paths; the row allows ${row["max-files"]}`); } const allow = row.allow.map((name) => PREDICATES[name](context)); - const outside = entries.find((entry) => !allow.every((admits) => admits(entry.path))); + const outside = entries.find((entry) => !allow.every((admits) => admits(entry.path, entry))); if (outside) { return reason("unexpected-path-staged", `not a conflicted path: ${outside.path}`); } @@ -840,7 +840,7 @@ export async function push({ if (row["max-files"] !== null && entries.length > row["max-files"]) { return escalate("invalid", `the patch changes ${entries.length} paths; the row allows ${row["max-files"]}`); } - const outside = entries.find((entry) => !allow.every((admits) => admits(entry.path))); + const outside = entries.find((entry) => !allow.every((admits) => admits(entry.path, entry))); if (outside) { return escalate("protected-path", `not allowed by ${row.allow.join(", ")}: ${outside.path}`); } @@ -955,23 +955,30 @@ function need(env, keys) { } // The kept threads of a threads.json: each a path and, when a fix request -// named files, its named_paths. +// named files, its named_paths; and the files the PR already changes, its +// pr_paths. export function keptThreadsFrom(file) { if (!file) { return undefined; } - const threads = JSON.parse(readFileSync(file, "utf8"))?.threads; + const { threads, pr_paths: prPaths = [] } = JSON.parse(readFileSync(file, "utf8")) ?? {}; const plain = (value) => typeof value === "string" && value !== ""; if ( !Array.isArray(threads) || + !Array.isArray(prPaths) || + !prPaths.every(plain) || !threads.every( (thread) => plain(thread?.path) && (thread.named_paths === undefined || (Array.isArray(thread.named_paths) && thread.named_paths.every(plain))), ) ) { - throw new Error("KEPT_THREADS_FILE is not a thread file of paths and named_paths"); + throw new Error("KEPT_THREADS_FILE is not a thread file of paths, named_paths and pr_paths"); } - return { paths: new Set(threads.map((thread) => thread.path)), threads: threads.map(({ path, named_paths }) => ({ path, named_paths })) }; + return { + paths: new Set(threads.map((thread) => thread.path)), + threads: threads.map(({ path, named_paths }) => ({ path, named_paths })), + prPaths: new Set(prPaths), + }; } // The base dependabot.yml's [ecosystem, directory] pairs, or null when the diff --git a/.github/scripts/apply-patch/apply-patch.test.mjs b/.github/scripts/apply-patch/apply-patch.test.mjs index bc797424a8..6e1ddb2d61 100644 --- a/.github/scripts/apply-patch/apply-patch.test.mjs +++ b/.github/scripts/apply-patch/apply-patch.test.mjs @@ -703,6 +703,42 @@ test("review-threads: a file a kept thread's named_paths holds pushes; the same }); }); +// A finding on a doc fixed in a script the pull request also changes: the +// thread file's pr_paths carry the PR's files into every thread's scope. +const docThread = (prPaths) => ({ ...keptOf([{ path: "README.md" }]), prPaths: new Set(prPaths) }); +const docPush = (fix, prPaths) => pushWith(fix, fakeApi(fix.repo, fix.head), { row: row("review-threads"), context: { keptThreads: docThread(prPaths) } }); + +test("review-threads: a thread on one PR file fixed in another PR file pushes", async (t) => { + const fix = repoWithPatch(t, (repo) => writeFileSync(join(repo, "scripts/x.sh"), "#!/bin/sh\nfoo=2\n")); + assert.equal((await docPush(fix, ["README.md", "scripts/x.sh"])).outcome, "pushed"); +}); + +test("review-threads: a fix in a file the PR does not change is refused", async (t) => { + const fix = repoWithPatch(t, (repo) => writeFileSync(join(repo, "scripts/x.sh"), "#!/bin/sh\nfoo=2\n")); + assert.deepEqual(await docPush(fix, ["README.md"]), { outcome: "escalate", escalationReason: "patch-protected-path" }); +}); + +test("review-threads: a new file stays refused even when pr_paths lists it", async (t) => { + const fix = repoWithPatch(t, (repo) => writeFileSync(join(repo, "scripts/new.sh"), "#!/bin/sh\n")); + assert.deepEqual(await docPush(fix, ["README.md", "scripts/new.sh"]), { outcome: "escalate", escalationReason: "patch-protected-path" }); +}); + +test("review-threads: a protected path the PR changes is still refused", async (t) => { + for (const path of [".github/workflows/ci.yml", "CODEOWNERS", ".github/actions/resolve-config/resolve.mjs"]) { + const fix = repoWithPatch( + t, + (repo) => writeFileSync(join(repo, path), "changed\n"), + { + head: (repo) => { + mkdirSync(join(repo, dirname(path)), { recursive: true }); + writeFileSync(join(repo, path), "head\n"); + }, + }, + ); + assert.deepEqual(await docPush(fix, ["README.md", path]), { outcome: "escalate", escalationReason: "patch-protected-path" }, path); + } +}); + test("review-threads: a symbolic link at an in-scope path is refused patch-invalid", async (t) => { const fix = repoWithPatch(t, (repo) => { mkdirSync(join(repo, "plugins/p"), { recursive: true }); @@ -731,9 +767,12 @@ test("keptThreadsFrom reads a threads.json's paths and named_paths, and refuses { path: "plugins/p/a.md", named_paths: ["plugins/p/b.js"] }, { path: "src/c.py", named_paths: undefined }, ], + prPaths: new Set(), }); + writeFileSync(file, JSON.stringify({ threads: [{ path: "plugins/p/a.md" }], pr_paths: ["plugins/p/a.md", "src/d.sh"] })); + assert.deepEqual(keptThreadsFrom(file).prPaths, new Set(["plugins/p/a.md", "src/d.sh"])); assert.equal(keptThreadsFrom(""), undefined); - for (const text of [JSON.stringify(["plugins/p/a.md"]), JSON.stringify({ threads: [{ path: 7 }] }), JSON.stringify({ threads: [{ path: "a", named_paths: "b" }] })]) { + for (const text of [JSON.stringify(["plugins/p/a.md"]), JSON.stringify({ threads: [{ path: 7 }] }), JSON.stringify({ threads: [{ path: "a", named_paths: "b" }] }), JSON.stringify({ threads: [{ path: "a" }], pr_paths: "b" }), JSON.stringify({ threads: [{ path: "a" }], pr_paths: [7] })]) { writeFileSync(file, text); assert.throws(() => keptThreadsFrom(file), /KEPT_THREADS_FILE/, text); } diff --git a/.github/scripts/apply-patch/predicates.mjs b/.github/scripts/apply-patch/predicates.mjs index c58db67914..020c5fde5e 100644 --- a/.github/scripts/apply-patch/predicates.mjs +++ b/.github/scripts/apply-patch/predicates.mjs @@ -9,12 +9,17 @@ import { inThreadScope } from "../pr-unblock/thread-scope.mjs"; export const PREDICATES = { any: () => () => true, // A kept thread's path, or a file thread-scope.mjs places in its scope, - // its named_paths included: `keptThreads` is { paths, threads }. + // its named_paths and the PR's own files the patch does not add included: + // `keptThreads` is { paths, threads, prPaths }. "kept-threads": ({ keptThreads }) => { if (!(keptThreads?.paths instanceof Set) || keptThreads.paths.size === 0 || !Array.isArray(keptThreads.threads)) { throw new Error("allow kept-threads needs the run's kept thread paths"); } - return (path) => inThreadScope(path, keptThreads.paths, keptThreads.threads); + return (path, entry) => + inThreadScope(path, keptThreads.paths, keptThreads.threads, { + prPaths: keptThreads.prPaths ?? new Set(), + added: entry === undefined || entry.status?.[0] === "A", + }); }, // An npm or pip manifest or lock file outside .github/, in a directory of // its ecosystem among the base dependabot.yml's [ecosystem, directory] diff --git a/.github/scripts/pr-unblock/address-feedback.mjs b/.github/scripts/pr-unblock/address-feedback.mjs index e952c912cd..15968c1900 100644 --- a/.github/scripts/pr-unblock/address-feedback.mjs +++ b/.github/scripts/pr-unblock/address-feedback.mjs @@ -13,7 +13,8 @@ // the configured number of rounds already ran and no write-access // fix request came after the newest one) or proceed, writing the thread file // the skill's review-thread tool reads, with the same-plugin files -// a write-access fix request names (namedPaths), and run.json, the +// a write-access fix request names (namedPaths) and the files the +// PR already changes (pr_paths, prPathsOf), and run.json, the // record the reply job checks the replies against (runRecord). On a PR carrying the // human-gated label it keeps only threads a trusted person with // write access asked it to fix (isFixRequest), and skips @@ -31,7 +32,7 @@ import { join } from "node:path"; import { pathToFileURL } from "node:url"; import { commentItems, gateSource, listLabelEvents, ROUND_MARKER } from "./gate-source.mjs"; -import { isProtectedPath, namedInScope } from "./thread-scope.mjs"; +import { isProtectedPath, namedInScope, prPathsOf } 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. @@ -498,7 +499,7 @@ export async function select({ // put the label on. fixRequested: labeled, threads: worked, - file: { version: 1, repository, pr, head_sha: headSha, threads: worked }, + file: { version: 1, repository, pr, head_sha: headSha, pr_paths: prPathsOf(files), threads: worked }, }; } @@ -509,6 +510,7 @@ export function runRecord(file) { const run = { version: 1, head_sha: file.head_sha, + pr_paths: file.pr_paths, threads: file.threads.map((t) => ({ id: t.id, path: t.path, diff --git a/.github/scripts/pr-unblock/address-feedback.test.mjs b/.github/scripts/pr-unblock/address-feedback.test.mjs index 177b2ff6aa..a6c6c46941 100644 --- a/.github/scripts/pr-unblock/address-feedback.test.mjs +++ b/.github/scripts/pr-unblock/address-feedback.test.mjs @@ -82,6 +82,7 @@ test("an unresolved reviewer-bot thread on a marker-confirmed head proceeds and repository: "o/r", pr: 42, head_sha: HEAD, + pr_paths: ["src/a.js"], threads: [ { id: "PRRT_1", @@ -95,6 +96,33 @@ test("an unresolved reviewer-bot thread on a marker-confirmed head proceeds and }); }); +// A finding on a doc whose fix lives in a script the PR also changes: the +// thread file lists every file the PR changes, rename sources included, but +// none a fix may never change. +test("the thread file's pr_paths hold the PR's files, rename sources included, less the refused ones", async () => { + const files = [ + { filename: "plugins/p/skills/s/converge.md" }, + { filename: "plugins/p/skills/s/scripts/converge-plan.sh" }, + { filename: "docs/new.md", previous_filename: "docs/old.md", status: "renamed" }, + { filename: "src/a.js" }, + { filename: ".github/actions/resolve-config/resolve.mjs" }, + { filename: ".github/standards/trusted-actors/trusted-actors.json" }, + { filename: ".github/actions/check-trusted-trigger/action.yml" }, + { filename: "CODEOWNERS" }, + { filename: "plugins/p/.claude/settings.json" }, + { filename: "a\\b.md" }, + ]; + const result = await run(fakeApi({ files })); + assert.equal(result.decision, "proceed"); + assert.deepEqual(result.file.pr_paths, [ + "docs/new.md", + "docs/old.md", + "plugins/p/skills/s/converge.md", + "plugins/p/skills/s/scripts/converge-plan.sh", + "src/a.js", + ]); +}); + test("the security lane's marker also confirms a head", async () => { const result = await run( fakeApi({ issueComments: [{ id: 1, user: { id: ACTIONS_BOT }, body: marker("claude-security-review", HEAD) }] }), @@ -699,10 +727,12 @@ test("main select writes outputs and the thread file; escalation writes no threa assert.equal(okFile.threads[0].id, "PRRT_1"); // No lane branch: the model job commits nothing. assert.equal(Object.hasOwn(okFile, "lane_branch"), false); + // The files the PR changes, for every kept thread's scope. + assert.deepEqual(okFile.pr_paths, ["src/a.js"]); // run.json's bytes, in the shape the write runner's collect step wrote. assert.equal( readFileSync(join(dir, "ok.run.json"), "utf8"), - `{"version":1,"head_sha":"${HEAD}","threads":[{"id":"PRRT_1","path":"src/a.js","comment_ids":[100]}]}\n`, + `{"version":1,"head_sha":"${HEAD}","pr_paths":["src/a.js"],"threads":[{"id":"PRRT_1","path":"src/a.js","comment_ids":[100]}]}\n`, ); // The people on the list come from TRUSTED_ACTORS. diff --git a/.github/scripts/pr-unblock/publish-feedback.mjs b/.github/scripts/pr-unblock/publish-feedback.mjs index 0c1b4bcffc..b9f3555621 100755 --- a/.github/scripts/pr-unblock/publish-feedback.mjs +++ b/.github/scripts/pr-unblock/publish-feedback.mjs @@ -123,7 +123,13 @@ export function parseRun(text) { } catch { throw new Invalid("run.json does not parse"); } - check(run?.version === 1 && SHA.test(run.head_sha ?? "") && Array.isArray(run.threads), "run.json is malformed"); + check( + run?.version === 1 && + SHA.test(run.head_sha ?? "") && + Array.isArray(run.threads) && + (run.pr_paths === undefined || (Array.isArray(run.pr_paths) && run.pr_paths.every((path) => typeof path === "string"))), + "run.json is malformed", + ); for (const thread of run.threads) { check( THREAD_ID.test(thread?.id ?? "") && @@ -199,9 +205,27 @@ export function renderReply({ disposition, evidence, fixSha, note }) { const SKILL_FILE = /^((?:.*\/)?skills\/[^/]+\/)SKILL\.md$/; +// Whether a unified diff's new side (a commit file's `patch`) holds `line` in +// one of its hunks; false with no patch, as for a binary or very large file. +export function lineChanged(patch, line) { + for (const [, start, count] of String(patch ?? "").matchAll(/^@@ -\d+(?:,\d+)? \+(\d+)(?:,(\d+))? @@/gm)) { + const first = Number(start); + const size = count === undefined ? 1 : Number(count); + if (line >= first && line < first + size) { + return true; + } + } + return false; +} + // Whether a fix commit's changed files touch a thread's path: the path itself, -// or for a skill's SKILL.md any file under that skill's directory. -function touchesThread(changed, threadPath) { +// a file the pull request already changes whose line the reply's evidence +// cites inside a hunk of the fix (`cited`), so a finding on one file fixed in another of the PR's files +// lands, or for a skill's SKILL.md any file under that skill's directory. +function touchesThread(changed, threadPath, cited) { + if (changed.some((file) => cited.has(file))) { + return true; + } const skillDir = SKILL_FILE.exec(threadPath)?.[1]; return skillDir === undefined ? changed.includes(threadPath) : changed.some((file) => file.startsWith(skillDir)); } @@ -226,11 +250,12 @@ export function fixCommitOf(commits, appId) { // Whenever the PR branch moved since the select job's head, or a reply is // VALID, that range may change only what inThreadScope allows around the kept -// threads' paths (run.json, whose SHA-256 the select job recorded), and -// nothing under .github/workflows/. With a VALID reply the range must hold -// exactly one fix commit (fixCommitOf), and it must change every VALID thread's path, or -// for a thread on a skill's SKILL.md any file in that skill's directory -// (touchesThread). Returns the fix commit, "" when no reply is VALID, or null. +// threads' paths and the PR's own files (run.json, whose SHA-256 the select +// job recorded), adding none of those files, and nothing under +// .github/workflows/. With a VALID reply the range must hold +// exactly one fix commit (fixCommitOf), and it must change every VALID thread's path, a +// file the PR already changes at a line the reply's evidence cites, or for a +// thread on a skill's SKILL.md any file in that skill's directory (touchesThread). Returns the fix commit, "" when no reply is VALID, or null. async function fixesLanded({ api, run, headSha, replies, appId }) { const valid = replies.filter((reply) => reply.disposition === "VALID"); if (valid.length === 0 && run.head_sha === headSha) { @@ -240,6 +265,7 @@ async function fixesLanded({ api, run, headSha, replies, appId }) { const fixSha = valid.length === 0 ? "" : fixCommitOf(comparison.commits ?? [], appId); const threadPath = new Map(run.threads.map((thread) => [thread.id, thread.path])); const paths = new Set(threadPath.values()); + const prPaths = new Set(run.pr_paths ?? []); const files = comparison.files ?? []; const inRange = ["ahead", "identical"].includes(comparison.status) && @@ -247,15 +273,30 @@ async function fixesLanded({ api, run, headSha, replies, appId }) { files.length < LIMITS.files && files.every((file) => [file.filename, file.previous_filename].every( - (path) => path === undefined || inThreadScope(path, paths, run.threads), + (path) => + path === undefined || + inThreadScope(path, paths, run.threads, { + prPaths, + added: path === file.filename && ["added", "renamed", "copied"].includes(file.status), + }), ), ); if (!inRange) { return null; } if (fixSha) { - const changed = ((await api.getCommit(fixSha)).files ?? []).map((file) => file.filename); - if (!valid.every((reply) => touchesThread(changed, threadPath.get(reply.thread)))) { + const commitFiles = (await api.getCommit(fixSha)).files ?? []; + const changed = commitFiles.map((file) => file.filename); + const patches = new Map(commitFiles.map((file) => [file.filename, file.patch])); + // The pr_paths files whose cited line the fix commit changed. + const cited = (reply) => + new Set( + reply.evidence + .map((ev) => [ev.slice(0, ev.lastIndexOf(":")), Number(ev.slice(ev.lastIndexOf(":") + 1))]) + .filter(([path, line]) => prPaths.has(path) && lineChanged(patches.get(path), line)) + .map(([path]) => path), + ); + if (!valid.every((reply) => touchesThread(changed, threadPath.get(reply.thread), cited(reply)))) { return null; } } diff --git a/.github/scripts/pr-unblock/publish-feedback.test.mjs b/.github/scripts/pr-unblock/publish-feedback.test.mjs index c928e13f0e..57e77c650d 100644 --- a/.github/scripts/pr-unblock/publish-feedback.test.mjs +++ b/.github/scripts/pr-unblock/publish-feedback.test.mjs @@ -485,12 +485,39 @@ test("a VALID fix that changes a file its thread's named_paths holds lands; one assert.equal(malformed.escalationReason, "feedback-invalid"); }); -test("feedback-not-landed, nothing posted: a VALID fix commit that does not change its own thread's path", async () => { +// A finding on one file of the PR fixed in another: run.json's pr_paths +// carry the PR's files into the thread's scope. +test("a VALID fix in another file the PR changes, citing a line it changed, lands; uncited, not in the PR, new or protected, it does not", async () => { + // The doc and the script, as files the checkout holds. + const thread = { ...RUN.threads[0], path: "docs/b.md" }; + const script = "src/a.js"; + for (const [label, prPaths, file, landed, evidence = [`${script}:1`]] of [ + ["in the PR", [thread.path, script], { filename: script, status: "modified" }, true], + ["in the PR, not cited", [thread.path, script], { filename: script, status: "modified" }, false, [`${thread.path}:1`]], + ["in the PR, cited outside the fix's hunks", [thread.path, script], { filename: script, status: "modified" }, false, [`${script}:9`]], + ["not in the PR", [thread.path], { filename: script, status: "modified" }, false], + ["added by the fix", [thread.path, script], { filename: script, status: "added" }, false], + ["under .github/workflows/", [thread.path, ".github/workflows/x.yml"], { filename: ".github/workflows/x.yml", status: "modified" }, false], + ["CODEOWNERS", [thread.path, "CODEOWNERS"], { filename: "CODEOWNERS", status: "modified" }, false], + ]) { + const api = fakeApi({ + compare: { status: "ahead", commits: [fixCommit()], files: [file] }, + commits: { [FIX]: { files: [{ ...file, patch: "@@ -1,2 +1,3 @@\n x\n+y\n z" }] } }, + }); + const result = await run(api, lines(reply({ disposition: "VALID", evidence })), JSON.stringify({ ...RUN, pr_paths: prPaths, threads: [thread] })); + assert.equal(result.escalationReason, landed ? "" : "feedback-not-landed", label); + assert.equal(api.posted().length, landed ? 1 : 0, label); + } + const malformed = await run(fakeApi(), lines(reply({ disposition: "VALID" })), JSON.stringify({ ...RUN, pr_paths: [7] })); + assert.equal(malformed.escalationReason, "feedback-invalid"); +}); + +test("feedback-not-landed, nothing posted: a VALID fix commit that does not change its own thread's path or a PR file its evidence cites", async () => { const api = fakeApi({ compare: { status: "ahead", commits: [fixCommit()], files: [{ filename: "docs/b.md" }] }, commits: { [FIX]: { files: [{ filename: "docs/b.md" }] } }, }); - const result = await run(api, lines(reply({ disposition: "VALID" }))); + const result = await run(api, lines(reply({ disposition: "VALID" })), JSON.stringify({ ...RUN, pr_paths: ["docs/b.md", "src/a.js"], threads: [RUN.threads[0]] })); assert.equal(result.escalationReason, "feedback-not-landed"); assert.deepEqual(api.posted(), []); }); @@ -889,3 +916,12 @@ test("no-model-turn is a schema skip reason, so its check posts neutral, not red const schema = JSON.parse(readFileSync(new URL("../../../docs/conventions/pr-pipeline/pr-pipeline.schema.json", import.meta.url), "utf8")); assert.ok(schema.$defs["skip-reason"].enum.includes("no-model-turn")); }); + +test("lineChanged reads a patch's new-side hunks", async () => { + const { lineChanged } = await import("./publish-feedback.mjs"); + const patch = "@@ -1,2 +1,3 @@\n x\n+y\n z\n@@ -10 +11 @@\n-a\n+b\n@@ -20,2 +21,0 @@\n-c\n-d"; + for (const [line, want] of [[1, true], [3, true], [4, false], [11, true], [12, false], [21, false]]) { + assert.equal(lineChanged(patch, line), want, String(line)); + } + assert.equal(lineChanged(undefined, 1), false); +}); diff --git a/.github/scripts/pr-unblock/thread-scope.mjs b/.github/scripts/pr-unblock/thread-scope.mjs index 5e56188103..c0d3d8fc10 100644 --- a/.github/scripts/pr-unblock/thread-scope.mjs +++ b/.github/scripts/pr-unblock/thread-scope.mjs @@ -1,8 +1,9 @@ // Which paths an address-feedback fix may change, shared by // address-feedback.mjs, publish-feedback.mjs and apply-patch.mjs's // kept-threads rule, and mirrored by the review-thread tool -// (feedback-thread.sh). It imports nothing, so the write runner's copy of it -// beside apply-patch.mjs's needs no other file. +// (feedback-thread.sh), which reads the pr_paths prPathsOf wrote. It imports +// nothing, so the write runner's copy of it beside apply-patch.mjs's needs no +// other file. // Under .github/workflows/, which the lane token has no permission to push. export function isProtectedPath(path) { @@ -29,6 +30,32 @@ const BAD_SEGMENT = /(^|\/)\.{0,2}(\/|$)/; // space, control or non-ASCII character. const PLAIN_PATH = /^[A-Za-z0-9_@+.-]+(\/[A-Za-z0-9_@+.-]+)*$/; +// Control-plane and agent-instruction surfaces: anything under .github/ +// (which holds the pr-merge HIGH floor's trusted-actor list, trusted-trigger +// check and resolve-config, and this pipeline's own scripts and actions), +// CLAUDE.md, AGENTS.md, .mcp.json, .lsp.json, a .claude or .claude-plugin +// directory, and a plugin's hooks/, monitors/ and bin/. +const CONTROL_PATH = + /^\.github\/|(^|\/)(CLAUDE\.md|AGENTS\.md|\.mcp\.json|\.lsp\.json)$|(^|\/)\.claude(-plugin)?\/|^plugins\/[^/]+\/(hooks|monitors|bin)\//i; + +// A path a pull request changes that a fix may also change: plain, in normal +// form, and neither under .github/workflows/, an instruction path nor a +// control-plane or agent-instruction surface. A thread on such a path still +// reaches it through the thread's own scope. +const prPathAllowed = (path) => + typeof path === "string" && + PLAIN_PATH.test(path) && + !BAD_SEGMENT.test(path) && + !isProtectedPath(path) && + !isInstructionPath(path) && + !CONTROL_PATH.test(path); + +// The paths a pull request's file listing (each { filename, +// previous_filename }) changes, rename sources included, that prPathAllowed +// allows; sorted, each once. The select job writes them as pr_paths. +export const prPathsOf = (files) => + [...new Set(files.flatMap((file) => [file?.filename, file?.previous_filename]).filter(prPathAllowed))].sort(); + // A path a trusted write-access fix request on a kept thread named may join // that thread's scope: a plain path in normal form, in the same plugins/
/ // directory as the thread's own path, and neither under .github/workflows/ nor @@ -48,17 +75,22 @@ export function namedInScope(path, threadPath) { // A kept thread's own path, a file in the plugins/
/ directory a kept
// thread's path sits in that is no instruction path and no script or code
// file, a test file in the same directory as a kept thread's path,
-// or a path in a kept thread's named_paths (in `threads`, each { path,
-// named_paths }) that namedInScope allows for that thread; never anything
-// under .github/workflows/, and never a path that is not plain and in normal
-// form.
-export function inThreadScope(path, threadPaths, threads = []) {
+// a path in a kept thread's named_paths (in `threads`, each { path,
+// named_paths }) that namedInScope allows for that thread, or a path the pull
+// request already changes (`prPaths`, prPathsOf's) that the fix does not add
+// (`added`: the fix creates the path, as a new file or a rename's target);
+// never anything under .github/workflows/, and never a path that is not plain
+// and in normal form.
+export function inThreadScope(path, threadPaths, threads = [], { prPaths = new Set(), added = false } = {}) {
if (isProtectedPath(path) || BAD_SEGMENT.test(path) || !PLAIN_PATH.test(path)) {
return false;
}
if (threadPaths.has(path)) {
return true;
}
+ if (!added && prPaths.has(path) && prPathAllowed(path)) {
+ return true;
+ }
if (threads.some((t) => Array.isArray(t?.named_paths) && t.named_paths.includes(path) && namedInScope(path, t.path))) {
return true;
}
diff --git a/.github/scripts/pr-unblock/thread-scope.test.mjs b/.github/scripts/pr-unblock/thread-scope.test.mjs
index c79bc585d8..01c0eecbd2 100644
--- a/.github/scripts/pr-unblock/thread-scope.test.mjs
+++ b/.github/scripts/pr-unblock/thread-scope.test.mjs
@@ -1,7 +1,7 @@
import assert from "node:assert/strict";
import { test } from "node:test";
-import { inThreadScope, isProtectedPath } from "./thread-scope.mjs";
+import { inThreadScope, isProtectedPath, prPathsOf } from "./thread-scope.mjs";
const kept = new Set(["plugins/p/evals/c/graders/x.md", "src/a.js"]);
@@ -224,3 +224,66 @@ test("a path with a non-ASCII letter is refused, even as a kept thread's own pat
assert.equal(inThreadScope(path, new Set([...kept, path])), false, path);
}
});
+
+// A finding on a doc whose fix lives in a script the pull request also
+// changes: thread on A, fix in B.
+const DOC = "plugins/p/skills/s/converge.md";
+const SCRIPT = "plugins/p/skills/s/scripts/converge-plan.sh";
+
+test("a file the PR already changes is in every kept thread's scope", () => {
+ const prPaths = new Set(prPathsOf([{ filename: DOC }, { filename: SCRIPT }, { filename: "src/b.js", previous_filename: "src/old.js" }]));
+ for (const path of [SCRIPT, "src/b.js", "src/old.js"]) {
+ assert.equal(inThreadScope(path, new Set([DOC]), [], { prPaths }), true, path);
+ }
+});
+
+test("a file the PR does not change stays outside a thread's scope", () => {
+ const prPaths = new Set([DOC]);
+ assert.equal(inThreadScope(SCRIPT, new Set([DOC]), [], { prPaths }), false);
+ assert.equal(inThreadScope("src/b.js", new Set([DOC]), [], { prPaths }), false);
+});
+
+test("a fix that adds a file the PR lists gets no pr_paths scope", () => {
+ const prPaths = new Set([DOC, SCRIPT, "src/b.js"]);
+ assert.equal(inThreadScope(SCRIPT, new Set([DOC]), [], { prPaths, added: true }), false);
+ assert.equal(inThreadScope("src/b.js", new Set([DOC]), [], { prPaths, added: true }), false);
+ // A new file stays in the thread's own scope as before.
+ assert.equal(inThreadScope("plugins/p/skills/s/new.md", new Set([DOC]), [], { prPaths, added: true }), true);
+});
+
+test("a protected, instruction, HIGH floor or control-plane path the PR changes is refused", () => {
+ const refused = [
+ ".github/workflows/x.yml",
+ ".GitHub/Workflows/x.yml",
+ "CODEOWNERS",
+ ".github/CODEOWNERS",
+ ".gitmodules",
+ "plugins/p/.claude/settings.json",
+ "plugins/p/package.json",
+ ".github/standards/trusted-actors/trusted-actors.json",
+ ".github/actions/check-trusted-trigger/action.yml",
+ ".github/actions/resolve-config/resolve.mjs",
+ ".github/scripts/pr-unblock/thread-scope.mjs",
+ ".github/actions/setup-runner-isolation/action.yml",
+ ".github/standards/runner-policy/x.json",
+ "CLAUDE.md",
+ "plugins/p/AGENTS.md",
+ ".mcp.json",
+ "plugins/p/.lsp.json",
+ ".claude/rules/r.md",
+ "plugins/p/.claude-plugin/plugin.json",
+ "plugins/p/hooks/guard.sh",
+ "plugins/p/monitors/monitors.json",
+ "plugins/p/bin/tool",
+ "a\\b.md",
+ "a b.md",
+ "src/../x.md",
+ ];
+ const listed = refused.map((filename) => ({ filename }));
+ assert.deepEqual(prPathsOf(listed), []);
+ // Even when a thread file lists them, inThreadScope refuses each beside a
+ // thread outside their plugin.
+ for (const path of refused) {
+ assert.equal(inThreadScope(path, new Set(["docs/x.md"]), [], { prPaths: new Set(refused) }), false, path);
+ }
+});
diff --git a/docs/conventions/pr-pipeline/README.md b/docs/conventions/pr-pipeline/README.md
index be45636549..9c8923fc10 100644
--- a/docs/conventions/pr-pipeline/README.md
+++ b/docs/conventions/pr-pipeline/README.md
@@ -536,7 +536,7 @@ each row to these keys:
| Key | Holds |
|---|---|
| `refuse` | Named refusals; any match refuses the whole patch. |
-| `allow` | What every other changed path must also meet: `any`, `kept-threads` (a path in the scope of a thread the select job kept, its `named_paths` included, read from that job's thread file), `dependency-manifests` (an npm or pip manifest or lock file outside `.github/` in a directory the base `.github/dependabot.yml` lists for its ecosystem) or `merge-result` (a path the base merge left conflicted; under a merge row only the patch's own changes, measured from the conflict tree, face the rules, every conflicted path left in the tree must hold no conflict markers, and the merge's base-side changes pass through). |
+| `allow` | What every other changed path must also meet: `any`, `kept-threads` (a path in the scope of a thread the select job kept, its `named_paths` included, or a file the pull request already changes that the patch does not add, its `pr_paths`, read from that job's thread file), `dependency-manifests` (an npm or pip manifest or lock file outside `.github/` in a directory the base `.github/dependabot.yml` lists for its ecosystem) or `merge-result` (a path the base merge left conflicted; under a merge row only the patch's own changes, measured from the conflict tree, face the rules, every conflicted path left in the tree must hold no conflict markers, and the merge's base-side changes pass through). |
| `max-files` | The most paths one patch may change, or `null`. |
| `max-blobs` | Merge rows only: the most blobs one merge commit may upload, each one neither the head nor the base holds at its path; over it nothing is written. |
| `requires` | What the producer's output must say before anything applies: `dependency-verdict: fix`, the scanned verdict's field. |
diff --git a/docs/conventions/pr-pipeline/pr-run-activity.md b/docs/conventions/pr-pipeline/pr-run-activity.md
index e32bfabedd..d2b1fe6cb3 100644
--- a/docs/conventions/pr-pipeline/pr-run-activity.md
+++ b/docs/conventions/pr-pipeline/pr-run-activity.md
@@ -870,7 +870,7 @@ the base's `.github/scripts/pr-unblock/address-feedback.mjs`, `apply-patch.mjs`
`cancelled` or `timed_out`) still runs one more round, on those threads only; a
round with no readable time grants none. Both escalations skip `awaiting-human`, and no model
job runs. Otherwise it writes `threads.json` and `run.json` (the gated head and the kept
- threads' ids, paths, comment ids and named paths, from the thread file) and uploads both as
+ threads' ids, paths, comment ids and named paths, and `pr_paths`, from the thread file) and uploads both as
`feedback-context- /` directory its path sits in, except an
- instruction path (`CLAUDE.md`, `AGENTS.md`, `.mcp.json`, `.lsp.json`, `.gitmodules` or
- `CODEOWNERS` at any depth, a `.claude` or `.claude-plugin` directory, a plugin's `hooks/`,
- `monitors/` or `bin/`) or a `.sh`, `.bash`, `.zsh`, `.ps1`,
+ instruction path
+ (`.gitmodules`, `CODEOWNERS`, a `settings*.json`, `package.json`, `package-lock.json` or `requirements*.txt`, at any depth) or a `.sh`, `.bash`, `.zsh`, `.ps1`,
`.py`, `.mjs`, `.cjs`, `.js`, `.ts`, `.mts` or `.cts` file; a test file (`test_*.py`, `*_test.py`, `*.test.*`, `*.spec.*`) in its path's
directory; and its `named_paths`. The select job sets those from the thread's fix requests by a
trusted person with write access: each file-shaped word outside fenced code and quoted lines,
@@ -903,7 +902,15 @@ the base's `.github/scripts/pr-unblock/address-feedback.mjs`, `apply-patch.mjs`
does not, kept when it is in the thread path's `plugins/ /` directory, in normal form, not
under `.github/workflows/` and not an instruction path (the same list), and
a regular file, not a symbolic link or gitlink, at the gated head (ten per thread
- at most).
+ at most). Every kept thread's scope also holds the thread file's `pr_paths`: each file the
+ pull request's file listing changes, rename sources included, in normal form, not under
+ `.github/workflows/`, not a `.gitmodules`, `CODEOWNERS`, `settings*.json`, package manifest or
+ `requirements*.txt`, and not a control-plane or agent-instruction surface: anything under
+ `.github/` (the pr-merge HIGH floor's trusted-actor list, trusted-trigger check and
+ `resolve-config` among it), `CLAUDE.md`, `AGENTS.md`, `.mcp.json` or `.lsp.json`, a `.claude`
+ or `.claude-plugin` directory, or a plugin's `hooks/`, `monitors/` or `bin/`. A path the fix creates gets no scope from `pr_paths`: the tool checks it exists at
+ the gated head, `apply-patch.mjs` refuses an added entry, and the reply job an `added`,
+ `renamed` or `copied` file.
3. After the skill, the run job carries the tool's `replies.jsonl` beside the patch, at most
49152 bytes, sealed for the scan job ([Patch output mode](#patch-output-mode)), and only for a
`review-threads` skill. The scan job scans it with the reply and the patch and, on a clean
@@ -917,7 +924,8 @@ the base's `.github/scripts/pr-unblock/address-feedback.mjs`, `apply-patch.mjs`
`scan-*` outputs, never a run job's, and only when that call's scan also uploaded replies.
Before the mint it downloads the select
job's thread file and fails red unless its SHA-256 is the select job's; `apply-patch.mjs` then
- admits only paths in a kept thread's scope (the `kept-threads` rule, `named_paths` included),
+ admits only paths in a kept thread's scope (the `kept-threads` rule, `named_paths` and
+ `pr_paths` included),
skips `superseded-sha` when the head moved since the select job read it, keeps the
human-gated hold unless the select job reported `fix-requested`, and commits with the
`Lane-Activity: pr-unblock/address-feedback` trailer. An empty patch (every reply needs no fix)
@@ -955,7 +963,8 @@ the base's `.github/scripts/pr-unblock/address-feedback.mjs`, `apply-patch.mjs`
current head: the branch must be ahead and change fewer than 300 files, each in a kept
thread's scope and none under `.github/workflows/`, and with a `VALID` reply hold exactly one commit the
lanes App authored with the push job's trailer, committed by `web-flow` with a `valid`
- signature, which must change each `VALID` thread's path,
+ signature, which must change each `VALID` thread's path, a file in `run.json`'s `pr_paths` at a
+ line the reply's evidence cites inside one of the fix commit's hunks,
or for a thread on a skill's `SKILL.md` any file in that skill's directory. Otherwise it
posts no reply and escalates `feedback-not-landed`: a push that was superseded, refused,
escalated or held made no such commit;
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 cb2c9b65d8..1a15380e62 100644
--- a/plugins/source-control/skills/pull-request/reference/comments-ci.md
+++ b/plugins/source-control/skills/pull-request/reference/comments-ci.md
@@ -31,9 +31,12 @@ The preamble names the tool's path. It is the only shell command this mode runs:
test file (`test_*.py`, `*_test.py`, `*.test.*`,
`*.spec.*`) in its path's directory; and each file in its `named_paths`: a file a trusted person
with write access named in a fix request, in the same `plugins/ /` directory, existing at the
- gated head, and not an instruction path (`CLAUDE.md`, `AGENTS.md`, `.mcp.json`, `.lsp.json`,
- `.gitmodules`, `CODEOWNERS`, a `.claude` or `.claude-plugin` directory, a plugin's `hooks`,
- `monitors` or `bin`).
+ gated head, and not an instruction path (`.gitmodules`, `CODEOWNERS`, a `settings*.json`,
+ `package.json`, `package-lock.json` or `requirements*.txt`, at any depth). Every kept thread's
+ scope also holds each file in the thread file's `pr_paths`, the files the pull request already
+ changes outside `.github/` and the agent-instruction surfaces, while the gated head holds it as
+ a regular file: a finding on one of the PR's files may be fixed in another. A file the fix would
+ create is never in `pr_paths`' part of the scope.
- `record /
-# directory that is no instruction path. The same rule as
+# directory that is no instruction path; or, when the gated head holds it as a
+# regular file, a file in the thread file's pr_paths (the files the pull request already
+# changes, which the select job already held to thread-scope.mjs's
+# prPathsOf). The same rule as
# .github/scripts/pr-unblock/thread-scope.mjs, which the promote step and
# the reply job apply.
allowed() {
+ local tracked=false mode
+ # A regular or executable file at the gated head: no new file, symbolic link or gitlink.
+ mode=$(git_tree ls-tree HEAD -- "$1" 2>/dev/null | cut -d' ' -f1) || mode=""
+ [[ $mode == 100644 || $mode == 100755 ]] && tracked=true
[[ ! ${1,,} =~ ^\.github/workflows(/|$) && ! $1 =~ (^|/)\.{0,2}(/|$) ]] &&
- jq -e --arg p "$1" '
+ jq -e --arg p "$1" --argjson tracked "$tracked" '
def dir: sub("[^/]*$"; "");
def plugin: first(capture("^(? / directory,
# never under .github/, never an instruction path, and never off normal form. run-transcript.js is
# the file #8116's thread PRRT_kwDOTCGFQM6tojDu was refused.