From 8822a5aad02ea1d72f9d6835bf28b1c9de7c75c6 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Fri, 23 Oct 2026 11:25:14 -0400 Subject: [PATCH 1/5] feat(pr-pipeline): let address-feedback edit any file the PR already changes A finding anchored on one of a pull request's files whose fix lives in another of its files stalled the round as feedback-uncertain. The select job now records the PR's files (rename sources included) as pr_paths in the thread file and run.json, and thread-scope.mjs admits them in every kept thread's scope for an existing file. The model's tool, apply-patch's kept-threads rule and the reply job enforce the same set. Workflows, instruction paths, the HIGH floor and every apply-patch refusal still apply; a file the fix creates gets no scope from pr_paths. Co-Authored-By: Claude Opus 5.5 --- ...feat-address-feedback-pr-paths-7c41e2a9.md | 7 +++ ...feat-address-feedback-pr-paths-3e9b0d51.md | 7 +++ .github/scripts/apply-patch/apply-patch.mjs | 17 ++++-- .../scripts/apply-patch/apply-patch.test.mjs | 41 ++++++++++++++- .github/scripts/apply-patch/predicates.mjs | 9 +++- .../scripts/pr-unblock/address-feedback.mjs | 8 +-- .../pr-unblock/address-feedback.test.mjs | 32 +++++++++++- .../scripts/pr-unblock/publish-feedback.mjs | 38 ++++++++++---- .../pr-unblock/publish-feedback.test.mjs | 24 +++++++++ .github/scripts/pr-unblock/thread-scope.mjs | 42 ++++++++++++--- .../scripts/pr-unblock/thread-scope.test.mjs | 52 ++++++++++++++++++- docs/conventions/pr-pipeline/README.md | 2 +- .../pr-pipeline/pr-run-activity.md | 16 ++++-- .../pull-request/reference/comments-ci.md | 5 +- .../pull-request/scripts/feedback-thread.sh | 16 ++++-- .../scripts/feedback-thread.test.sh | 25 +++++++++ 16 files changed, 301 insertions(+), 40 deletions(-) create mode 100644 .changes/pr-pipeline/feat-address-feedback-pr-paths-7c41e2a9.md create mode 100644 .changes/source-control/feat-address-feedback-pr-paths-3e9b0d51.md 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..744a337848 --- /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, rename sources included, not only in the thread's own scope. The select job writes those files as `pr_paths` in the thread file and `run.json`, less `.github/workflows/`, `.gitmodules`, `CODEOWNERS`, `settings*.json`, package manifests, `requirements*.txt` and the HIGH floor's trusted-actor list, trusted-trigger check and `resolve-config`. `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 any `pr_paths` file. 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..96fe5acc2e --- /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. An instruction path and the trusted-actor list, trusted-trigger check and `resolve-config` stay refused. `reference/comments-ci.md` says so. diff --git a/.github/scripts/apply-patch/apply-patch.mjs b/.github/scripts/apply-patch/apply-patch.mjs index 80c64fb021..9bb0063fed 100755 --- a/.github/scripts/apply-patch/apply-patch.mjs +++ b/.github/scripts/apply-patch/apply-patch.mjs @@ -569,7 +569,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}`); } @@ -684,23 +684,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 8a69124a3f..bf9ea58546 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 d77251ad2c..6cf956a07e 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..15b77c12be 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 ?? "") && @@ -200,8 +206,13 @@ export function renderReply({ disposition, evidence, fixSha, note }) { const SKILL_FILE = /^((?:.*\/)?skills\/[^/]+\/)SKILL\.md$/; // 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 (`prPaths`), 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, prPaths) { + if (changed.some((file) => prPaths.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 +237,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, 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 +252,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,7 +260,12 @@ 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) { @@ -255,7 +273,7 @@ async function fixesLanded({ api, run, headSha, replies, appId }) { } if (fixSha) { const changed = ((await api.getCommit(fixSha)).files ?? []).map((file) => file.filename); - if (!valid.every((reply) => touchesThread(changed, threadPath.get(reply.thread)))) { + if (!valid.every((reply) => touchesThread(changed, threadPath.get(reply.thread), prPaths))) { return null; } } diff --git a/.github/scripts/pr-unblock/publish-feedback.test.mjs b/.github/scripts/pr-unblock/publish-feedback.test.mjs index c928e13f0e..e0dc3c24d1 100644 --- a/.github/scripts/pr-unblock/publish-feedback.test.mjs +++ b/.github/scripts/pr-unblock/publish-feedback.test.mjs @@ -485,6 +485,30 @@ test("a VALID fix that changes a file its thread's named_paths holds lands; one assert.equal(malformed.escalationReason, "feedback-invalid"); }); +// 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 lands; one the PR does not change, a new one, or a protected one does not", async () => { + const thread = { ...RUN.threads[0], path: "plugins/p/skills/s/converge.md" }; + const script = "plugins/p/skills/s/scripts/converge-plan.sh"; + for (const [label, prPaths, file, landed] of [ + ["in the PR", [thread.path, script], { filename: script, status: "modified" }, true], + ["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] } }, + }); + const result = await run(api, lines(reply({ disposition: "VALID" })), 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", async () => { const api = fakeApi({ compare: { status: "ahead", commits: [fixCommit()], files: [{ filename: "docs/b.md" }] }, diff --git a/.github/scripts/pr-unblock/thread-scope.mjs b/.github/scripts/pr-unblock/thread-scope.mjs index 5e56188103..2c4014fe88 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,28 @@ const BAD_SEGMENT = /(^|\/)\.{0,2}(\/|$)/; // space, control or non-ASCII character. const PLAIN_PATH = /^[A-Za-z0-9_@+.-]+(\/[A-Za-z0-9_@+.-]+)*$/; +// The pr-merge HIGH floor's paths (babysit_risk.py HIGH_FLOOR) besides +// CODEOWNERS and .gitmodules, which isInstructionPath holds: the trusted-actor +// list, the trusted-trigger check and resolve-config. +const HIGH_FLOOR_PATH = /^\.github\/(standards\/trusted-actors|actions\/check-trusted-trigger|actions\/resolve-config)\//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 on the +// HIGH floor. +const prPathAllowed = (path) => + typeof path === "string" && + PLAIN_PATH.test(path) && + !BAD_SEGMENT.test(path) && + !isProtectedPath(path) && + !isInstructionPath(path) && + !HIGH_FLOOR_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 +71,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..cb487e0e1d 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,53 @@ 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 or HIGH floor path the PR changes is still 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", + "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. + for (const path of refused) { + assert.equal(inThreadScope(path, new Set([DOC]), [], { prPaths: new Set(refused) }), false, path); + } +}); diff --git a/docs/conventions/pr-pipeline/README.md b/docs/conventions/pr-pipeline/README.md index 932dcfc383..d4e0a281f0 100644 --- a/docs/conventions/pr-pipeline/README.md +++ b/docs/conventions/pr-pipeline/README.md @@ -525,7 +525,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` (the blob the base merge leaves there, or a conflicted path left with no conflict markers). | +| `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` (the blob the base merge leaves there, or a conflicted path left with no conflict markers). | | `max-files` | The most paths one patch may change, or `null`. | | `requires` | What the producer's output must say before anything applies: `dependency-verdict: fix`, the scanned verdict's field. | | `content-rule` | Optional; what each changed file's bytes must also meet, on the local blobs before any commit is built, escalating as `protected-path`: `dependency-version-change` (`dependency-review`: modified in place, a regular file, changed only by a version, `fixRefusal` in `dependency-classify.mjs`). | diff --git a/docs/conventions/pr-pipeline/pr-run-activity.md b/docs/conventions/pr-pipeline/pr-run-activity.md index 18b9b0ea19..45d35522bd 100644 --- a/docs/conventions/pr-pipeline/pr-run-activity.md +++ b/docs/conventions/pr-pipeline/pr-run-activity.md @@ -852,7 +852,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-`. Its outputs are `proceed`, the head it read, the escalation, `fix-requested`, both files' SHA-256 and that attempt; every later job takes its gate, head and reply inputs from them, never from a model call's run job, whose runner head @@ -885,7 +885,14 @@ 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 under `.github/standards/trusted-actors/`, + `.github/actions/check-trusted-trigger/` or `.github/actions/resolve-config/` (the pr-merge + HIGH floor). 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 @@ -899,7 +906,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) @@ -937,7 +945,7 @@ 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`, 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..7c4c31cf24 100644 --- a/plugins/source-control/skills/pull-request/reference/comments-ci.md +++ b/plugins/source-control/skills/pull-request/reference/comments-ci.md @@ -33,7 +33,10 @@ The preamble names the tool's path. It is the only shell command this mode runs: 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`). + `monitors` or `bin`). Every kept thread's scope also holds each file in the thread file's + `pr_paths`, the files the pull request already changes, while the gated head holds it: 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 ` reads a disposition file from the scratch directory, checks it against the template, and records it. Exit 1 means the tool refused and recorded nothing: fix the file and record again. One record per thread. diff --git a/plugins/source-control/skills/pull-request/scripts/feedback-thread.sh b/plugins/source-control/skills/pull-request/scripts/feedback-thread.sh index ad2bea21f5..572d0dc0ef 100755 --- a/plugins/source-control/skills/pull-request/scripts/feedback-thread.sh +++ b/plugins/source-control/skills/pull-request/scripts/feedback-thread.sh @@ -50,12 +50,17 @@ scratch_dir=${FEEDBACK_SCRATCH:-} workspace=${FEEDBACK_WORKSPACE:-} # CODEOWNERS, a settings*.json, a package manifest or lockfile) and no script # or code file, a test file in its directory, or a path in its named_paths # (the files a write-access fix request named) in that same plugins/

/ -# directory that is no instruction path. The same rule as +# directory that is no instruction path; or, when the gated head holds it, a +# file in the thread file's pr_paths (the files the pull request already +# changes) that is no instruction path and not under the trusted-actor list, +# the trusted-trigger check or resolve-config. The same rule as # .github/scripts/pr-unblock/thread-scope.mjs, which the promote step and # the reply job apply. allowed() { + local tracked=false + git_tree cat-file -e "HEAD:$1" 2>/dev/null && 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("^(?plugins/[^/]+/)").c) // null; ($p | test("^[A-Za-z0-9_@+.-]+(/[A-Za-z0-9_@+.-]+)*$")) as $plain @@ -64,11 +69,14 @@ allowed() { then null else $p | plugin end) as $plugin | ($p | sub("^.*/"; "") | test("^(test_[^/]*\\.py|[^/]*_test\\.py|[^/]*\\.(test|spec)\\.[^/]+)$")) as $test | (if $instruction then null else $p | plugin end) as $named - | $plain and any(.threads[]; .path as $t + | ($tracked and ($instruction | not) + and ($p | test("^\\.github/(standards/trusted-actors|actions/check-trusted-trigger|actions/resolve-config)/"; "i") | not) + and any(.pr_paths[]?; . == $p)) as $pr + | $plain and ($pr or any(.threads[]; .path as $t | $t == $p or ($plugin != null and ($t | startswith($plugin))) or ($test and ($t | dir) == ($p | dir)) - or ($named != null and ($t | plugin) == $named and any(.named_paths[]?; . == $p)))' "$threads_file" >/dev/null + or ($named != null and ($t | plugin) == $named and any(.named_paths[]?; . == $p))))' "$threads_file" >/dev/null } # git in the checkout with no system, global, hook, fsmonitor or untracked diff --git a/plugins/source-control/skills/pull-request/scripts/feedback-thread.test.sh b/plugins/source-control/skills/pull-request/scripts/feedback-thread.test.sh index 992d9ec475..c6bb27b77f 100755 --- a/plugins/source-control/skills/pull-request/scripts/feedback-thread.test.sh +++ b/plugins/source-control/skills/pull-request/scripts/feedback-thread.test.sh @@ -126,6 +126,31 @@ for path in plugins/q/evals/c/samples/y.json plugins/pp/README.md plugins/README assert_exit "check-paths refuses $path" 1 "$?" done +# check-paths: a file the pull request already changes (pr_paths) joins every thread's scope while +# the gated head holds it; a new file, a .github/workflows/ path, an instruction path and the HIGH +# floor's paths stay out even when pr_paths lists them +setup +mkdir -p "$STUB/ws/.github/workflows" "$STUB/ws/.github/actions/resolve-config" +echo 'c' >"$STUB/ws/.github/workflows/ci.yml" +echo 'r' >"$STUB/ws/.github/actions/resolve-config/resolve.mjs" +echo 'o' >"$STUB/ws/CODEOWNERS" +fixture_git add -A +fixture_git commit -q -m more +run check-paths src/other.js >/dev/null 2>&1 +assert_exit "check-paths refuses a file the pull request changes when pr_paths does not list it" 1 "$?" +jq '.pr_paths = ["src/other.js", "src/new.js", ".github/workflows/ci.yml", ".github/actions/resolve-config/resolve.mjs", "CODEOWNERS"]' \ + "$STUB/threads.json" >"$STUB/t2" && mv "$STUB/t2" "$STUB/threads.json" +run check-paths src/other.js >/dev/null 2>&1 +assert_exit "check-paths allows a file pr_paths lists, outside every thread's own scope" 0 "$?" +for path in src/new.js .github/workflows/ci.yml .github/actions/resolve-config/resolve.mjs CODEOWNERS src/helper.js; do + run check-paths "$path" >/dev/null 2>&1 + assert_exit "check-paths refuses $path with pr_paths set" 1 "$?" +done +echo 'fixed' >"$STUB/ws/src/other.js" +disposition '{"disposition":"VALID","evidence":["src/a.js:1"],"fix_sha":"","note":"fixed in src/other.js"}' +run record PRRT_a "$STUB/scratch/d.json" >/dev/null 2>&1 +assert_exit "record takes a VALID fix made in a pr_paths file" 0 "$?" + # check-paths: a kept thread's named_paths join its scope only in its own plugins/

/ 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. From 226841cd2dec099f0971d19cf4287a14d4f854d3 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Fri, 23 Oct 2026 11:28:41 -0400 Subject: [PATCH 2/5] fix(pr-pipeline): land a PR-file fix only when its reply cites that file A VALID reply now lands from a fix in another of the pull request's files only when its evidence cites that file, so each thread keeps its anchor. comments-ci.md tells the model to run check-paths before calling a file out of scope and to cite the changed line. The tool's pr_paths check relies on the select job's filtering. Co-Authored-By: Claude Opus 5.5 --- .../feat-address-feedback-pr-paths-7c41e2a9.md | 2 +- .../feat-address-feedback-pr-paths-3e9b0d51.md | 2 +- .github/scripts/pr-unblock/publish-feedback.mjs | 17 +++++++++-------- .../pr-unblock/publish-feedback.test.mjs | 16 +++++++++------- docs/conventions/pr-pipeline/pr-run-activity.md | 3 ++- .../pull-request/reference/comments-ci.md | 11 +++++++---- .../pull-request/scripts/feedback-thread.sh | 8 +++----- .../scripts/feedback-thread.test.sh | 14 ++++++-------- 8 files changed, 38 insertions(+), 35 deletions(-) diff --git a/.changes/pr-pipeline/feat-address-feedback-pr-paths-7c41e2a9.md b/.changes/pr-pipeline/feat-address-feedback-pr-paths-7c41e2a9.md index 744a337848..4fbce6b5fb 100644 --- a/.changes/pr-pipeline/feat-address-feedback-pr-paths-7c41e2a9.md +++ b/.changes/pr-pipeline/feat-address-feedback-pr-paths-7c41e2a9.md @@ -4,4 +4,4 @@ bump: minor ### Changed -- `address-feedback` may fix a finding in any file the pull request already changes, rename sources included, not only in the thread's own scope. The select job writes those files as `pr_paths` in the thread file and `run.json`, less `.github/workflows/`, `.gitmodules`, `CODEOWNERS`, `settings*.json`, package manifests, `requirements*.txt` and the HIGH floor's trusted-actor list, trusted-trigger check and `resolve-config`. `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 any `pr_paths` file. Every apply-patch refusal still applies. +- `address-feedback` may fix a finding in any file the pull request already changes, rename sources included, not only in the thread's own scope. The select job writes those files as `pr_paths` in the thread file and `run.json`, less `.github/workflows/`, `.gitmodules`, `CODEOWNERS`, `settings*.json`, package manifests, `requirements*.txt` and the HIGH floor's trusted-actor list, trusted-trigger check and `resolve-config`. `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 the reply's evidence cites. 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 index 96fe5acc2e..47dec92987 100644 --- a/.changes/source-control/feat-address-feedback-pr-paths-3e9b0d51.md +++ b/.changes/source-control/feat-address-feedback-pr-paths-3e9b0d51.md @@ -4,4 +4,4 @@ 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. An instruction path and the trusted-actor list, trusted-trigger check and `resolve-config` stay refused. `reference/comments-ci.md` says so. +- 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/pr-unblock/publish-feedback.mjs b/.github/scripts/pr-unblock/publish-feedback.mjs index 15b77c12be..8a4238554c 100755 --- a/.github/scripts/pr-unblock/publish-feedback.mjs +++ b/.github/scripts/pr-unblock/publish-feedback.mjs @@ -206,11 +206,11 @@ export function renderReply({ disposition, evidence, fixSha, note }) { const SKILL_FILE = /^((?:.*\/)?skills\/[^/]+\/)SKILL\.md$/; // Whether a fix commit's changed files touch a thread's path: the path itself, -// a file the pull request already changes (`prPaths`), 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, prPaths) { - if (changed.some((file) => prPaths.has(file))) { +// a file the pull request already changes that the reply's evidence cites +// (`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]; @@ -241,8 +241,8 @@ export function fixCommitOf(commits, appId) { // 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, 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. +// file the PR already changes that 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) { @@ -273,7 +273,8 @@ async function fixesLanded({ api, run, headSha, replies, appId }) { } if (fixSha) { const changed = ((await api.getCommit(fixSha)).files ?? []).map((file) => file.filename); - if (!valid.every((reply) => touchesThread(changed, threadPath.get(reply.thread), prPaths))) { + const cited = (reply) => new Set(reply.evidence.map((ev) => ev.slice(0, ev.lastIndexOf(":"))).filter((path) => prPaths.has(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 e0dc3c24d1..8e4fa9271b 100644 --- a/.github/scripts/pr-unblock/publish-feedback.test.mjs +++ b/.github/scripts/pr-unblock/publish-feedback.test.mjs @@ -487,11 +487,13 @@ test("a VALID fix that changes a file its thread's named_paths holds lands; one // 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 lands; one the PR does not change, a new one, or a protected one does not", async () => { - const thread = { ...RUN.threads[0], path: "plugins/p/skills/s/converge.md" }; - const script = "plugins/p/skills/s/scripts/converge-plan.sh"; - for (const [label, prPaths, file, landed] of [ +test("a VALID fix in another file the PR changes, cited as evidence, 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`]], ["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], @@ -501,7 +503,7 @@ test("a VALID fix in another file the PR changes lands; one the PR does not chan compare: { status: "ahead", commits: [fixCommit()], files: [file] }, commits: { [FIX]: { files: [file] } }, }); - const result = await run(api, lines(reply({ disposition: "VALID" })), JSON.stringify({ ...RUN, pr_paths: prPaths, threads: [thread] })); + 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); } @@ -509,12 +511,12 @@ test("a VALID fix in another file the PR changes lands; one the PR does not chan 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 () => { +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(), []); }); diff --git a/docs/conventions/pr-pipeline/pr-run-activity.md b/docs/conventions/pr-pipeline/pr-run-activity.md index 45d35522bd..b57bd0d474 100644 --- a/docs/conventions/pr-pipeline/pr-run-activity.md +++ b/docs/conventions/pr-pipeline/pr-run-activity.md @@ -945,7 +945,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, a file in `run.json`'s `pr_paths`, + signature, which must change each `VALID` thread's path, a file in `run.json`'s `pr_paths` that + the reply's evidence cites, 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 7c4c31cf24..d66d77259d 100644 --- a/plugins/source-control/skills/pull-request/reference/comments-ci.md +++ b/plugins/source-control/skills/pull-request/reference/comments-ci.md @@ -47,13 +47,14 @@ The preamble names the tool's path. It is the only shell command this mode runs: reading code and docs only: run no command, test, build or script from the checkout. CI runs them after the push. 2. Classify. On a thread with a direction, apply the direction: record `VALID` once its fix is - in the working tree, or `UNCERTAIN` with the reason when it needs a file outside every kept thread's scope + in the working tree, or `UNCERTAIN` with the reason when it needs a file `check-paths` refuses or cannot be done in one scoped change. When the current code no longer shows the finding (fixed, moved or deleted since that review), make no edit: record `UNCERTAIN` with evidence of what the code shows now. Never record `INCORRECT` against a direction. Otherwise, a finding that touches security, shared infrastructure or another repository, and - that the code you read cannot settle, is UNCERTAIN. So is a fix that needs a file outside - every kept thread's scope, or is too large for one scoped change. + that the code you read cannot settle, is UNCERTAIN. So is a fix that needs a file + `check-paths` refuses, or is too large for one scoped change. A file outside the thread's own + path may still be in scope: run `check-paths` on it before deciding. 3. VALID: run `check-paths` on every file the fix changes before you edit it, and edit only when it prints `allowed`. Leave the fix uncommitted in the working tree before you record it: there is no commit tool, and the one commit the push job makes may fix several threads. @@ -68,7 +69,9 @@ The preamble names the tool's path. It is the only shell command this mode runs: } ``` - `VALID` and `INCORRECT` need at least one `path:line` in the checkout, at most 10. `fix_sha` + `VALID` and `INCORRECT` need at least one `path:line` in the checkout, at most 10. A `VALID` + fix that does not change the thread's own path must cite a line it changed in another of the + pull request's files, or its reply does not land. `fix_sha` stays empty, or is left out: the reply job finds the commit the push job made. `VALID-defer` is VALID (defer). The note is 1-2000 characters of plain prose: no control characters, no HTML comment (``), no token, no @-mention. It is posted in a code fence. diff --git a/plugins/source-control/skills/pull-request/scripts/feedback-thread.sh b/plugins/source-control/skills/pull-request/scripts/feedback-thread.sh index 572d0dc0ef..4adf924ac0 100755 --- a/plugins/source-control/skills/pull-request/scripts/feedback-thread.sh +++ b/plugins/source-control/skills/pull-request/scripts/feedback-thread.sh @@ -52,8 +52,8 @@ scratch_dir=${FEEDBACK_SCRATCH:-} workspace=${FEEDBACK_WORKSPACE:-} # (the files a write-access fix request named) in that same plugins/

/ # directory that is no instruction path; or, when the gated head holds it, a # file in the thread file's pr_paths (the files the pull request already -# changes) that is no instruction path and not under the trusted-actor list, -# the trusted-trigger check or resolve-config. The same rule as +# 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() { @@ -69,9 +69,7 @@ allowed() { then null else $p | plugin end) as $plugin | ($p | sub("^.*/"; "") | test("^(test_[^/]*\\.py|[^/]*_test\\.py|[^/]*\\.(test|spec)\\.[^/]+)$")) as $test | (if $instruction then null else $p | plugin end) as $named - | ($tracked and ($instruction | not) - and ($p | test("^\\.github/(standards/trusted-actors|actions/check-trusted-trigger|actions/resolve-config)/"; "i") | not) - and any(.pr_paths[]?; . == $p)) as $pr + | ($tracked and any(.pr_paths[]?; . == $p)) as $pr | $plain and ($pr or any(.threads[]; .path as $t | $t == $p or ($plugin != null and ($t | startswith($plugin))) diff --git a/plugins/source-control/skills/pull-request/scripts/feedback-thread.test.sh b/plugins/source-control/skills/pull-request/scripts/feedback-thread.test.sh index c6bb27b77f..f15e9b7e33 100755 --- a/plugins/source-control/skills/pull-request/scripts/feedback-thread.test.sh +++ b/plugins/source-control/skills/pull-request/scripts/feedback-thread.test.sh @@ -127,27 +127,25 @@ for path in plugins/q/evals/c/samples/y.json plugins/pp/README.md plugins/README done # check-paths: a file the pull request already changes (pr_paths) joins every thread's scope while -# the gated head holds it; a new file, a .github/workflows/ path, an instruction path and the HIGH -# floor's paths stay out even when pr_paths lists them +# the gated head holds it; a new file and a .github/workflows/ path stay out even when pr_paths +# lists them (the select job leaves instruction and HIGH floor paths out of pr_paths) setup -mkdir -p "$STUB/ws/.github/workflows" "$STUB/ws/.github/actions/resolve-config" +mkdir -p "$STUB/ws/.github/workflows" echo 'c' >"$STUB/ws/.github/workflows/ci.yml" -echo 'r' >"$STUB/ws/.github/actions/resolve-config/resolve.mjs" -echo 'o' >"$STUB/ws/CODEOWNERS" fixture_git add -A fixture_git commit -q -m more run check-paths src/other.js >/dev/null 2>&1 assert_exit "check-paths refuses a file the pull request changes when pr_paths does not list it" 1 "$?" -jq '.pr_paths = ["src/other.js", "src/new.js", ".github/workflows/ci.yml", ".github/actions/resolve-config/resolve.mjs", "CODEOWNERS"]' \ +jq '.pr_paths = ["src/other.js", "src/new.js", ".github/workflows/ci.yml"]' \ "$STUB/threads.json" >"$STUB/t2" && mv "$STUB/t2" "$STUB/threads.json" run check-paths src/other.js >/dev/null 2>&1 assert_exit "check-paths allows a file pr_paths lists, outside every thread's own scope" 0 "$?" -for path in src/new.js .github/workflows/ci.yml .github/actions/resolve-config/resolve.mjs CODEOWNERS src/helper.js; do +for path in src/new.js .github/workflows/ci.yml src/helper.js; do run check-paths "$path" >/dev/null 2>&1 assert_exit "check-paths refuses $path with pr_paths set" 1 "$?" done echo 'fixed' >"$STUB/ws/src/other.js" -disposition '{"disposition":"VALID","evidence":["src/a.js:1"],"fix_sha":"","note":"fixed in src/other.js"}' +disposition '{"disposition":"VALID","evidence":["src/other.js:1"],"fix_sha":"","note":"fixed in src/other.js"}' run record PRRT_a "$STUB/scratch/d.json" >/dev/null 2>&1 assert_exit "record takes a VALID fix made in a pr_paths file" 0 "$?" From 1f532f298720eb33f3b92652f88f4fcd9e3b6286 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Fri, 23 Oct 2026 11:31:22 -0400 Subject: [PATCH 3/5] refactor(pr-pipeline): hand the merge row's allow rules each entry too Co-Authored-By: Claude Opus 5.5 --- .github/scripts/apply-patch/apply-patch.mjs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/scripts/apply-patch/apply-patch.mjs b/.github/scripts/apply-patch/apply-patch.mjs index 38429ee20f..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}`); } From 6fa7d831f3563a502c301b304cf6b0565b809b9b Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Fri, 23 Oct 2026 11:49:52 -0400 Subject: [PATCH 4/5] fix(pr-pipeline): land a PR-file fix only at a cited changed line From the ready-phase reviews of this change: - the reply job counts a cited pr_paths file only when the cited line falls in a hunk of the fix commit, so one thread's fix cannot land another thread's reply; - the review-thread tool admits a pr_paths file only as a regular file at the gated head, never a symbolic link; - the docs name the instruction paths the code enforces. Co-Authored-By: Claude Opus 5.5 --- ...feat-address-feedback-pr-paths-7c41e2a9.md | 2 +- .../scripts/pr-unblock/publish-feedback.mjs | 34 +++++++++++++++---- .../pr-unblock/publish-feedback.test.mjs | 14 ++++++-- .../pr-pipeline/pr-run-activity.md | 9 +++-- .../pull-request/reference/comments-ci.md | 9 +++-- .../pull-request/scripts/feedback-thread.sh | 10 +++--- .../scripts/feedback-thread.test.sh | 6 +++- 7 files changed, 60 insertions(+), 24 deletions(-) diff --git a/.changes/pr-pipeline/feat-address-feedback-pr-paths-7c41e2a9.md b/.changes/pr-pipeline/feat-address-feedback-pr-paths-7c41e2a9.md index 4fbce6b5fb..4620bd84e4 100644 --- a/.changes/pr-pipeline/feat-address-feedback-pr-paths-7c41e2a9.md +++ b/.changes/pr-pipeline/feat-address-feedback-pr-paths-7c41e2a9.md @@ -4,4 +4,4 @@ bump: minor ### Changed -- `address-feedback` may fix a finding in any file the pull request already changes, rename sources included, not only in the thread's own scope. The select job writes those files as `pr_paths` in the thread file and `run.json`, less `.github/workflows/`, `.gitmodules`, `CODEOWNERS`, `settings*.json`, package manifests, `requirements*.txt` and the HIGH floor's trusted-actor list, trusted-trigger check and `resolve-config`. `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 the reply's evidence cites. Every apply-patch refusal still applies. +- `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` and the HIGH floor's trusted-actor list, trusted-trigger check and `resolve-config`. `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/.github/scripts/pr-unblock/publish-feedback.mjs b/.github/scripts/pr-unblock/publish-feedback.mjs index 8a4238554c..b9f3555621 100755 --- a/.github/scripts/pr-unblock/publish-feedback.mjs +++ b/.github/scripts/pr-unblock/publish-feedback.mjs @@ -205,9 +205,22 @@ 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, -// a file the pull request already changes that the reply's evidence cites -// (`cited`), so a finding on one file fixed in another of the PR's files +// 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))) { @@ -241,8 +254,8 @@ export function fixCommitOf(commits, appId) { // 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 that 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. +// 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) { @@ -272,8 +285,17 @@ async function fixesLanded({ api, run, headSha, replies, appId }) { return null; } if (fixSha) { - const changed = ((await api.getCommit(fixSha)).files ?? []).map((file) => file.filename); - const cited = (reply) => new Set(reply.evidence.map((ev) => ev.slice(0, ev.lastIndexOf(":"))).filter((path) => prPaths.has(path))); + 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 8e4fa9271b..57e77c650d 100644 --- a/.github/scripts/pr-unblock/publish-feedback.test.mjs +++ b/.github/scripts/pr-unblock/publish-feedback.test.mjs @@ -487,13 +487,14 @@ test("a VALID fix that changes a file its thread's named_paths holds lands; one // 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, cited as evidence, lands; uncited, not in the PR, new or protected, it does not", async () => { +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], @@ -501,7 +502,7 @@ test("a VALID fix in another file the PR changes, cited as evidence, lands; unci ]) { const api = fakeApi({ compare: { status: "ahead", commits: [fixCommit()], files: [file] }, - commits: { [FIX]: { 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); @@ -915,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/docs/conventions/pr-pipeline/pr-run-activity.md b/docs/conventions/pr-pipeline/pr-run-activity.md index 7760a7164d..218200ca55 100644 --- a/docs/conventions/pr-pipeline/pr-run-activity.md +++ b/docs/conventions/pr-pipeline/pr-run-activity.md @@ -886,9 +886,8 @@ the base's `.github/scripts/pr-unblock/address-feedback.mjs`, `apply-patch.mjs` thread's scope or any `.github/workflows/` path, read with no hook, fsmonitor or untracked-cache config. Every path must be plain ASCII letters, digits and `_@+.-` in `/`-separated segments. A thread's scope is its path; a file in the `plugins/

/` 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, @@ -956,8 +955,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, a file in `run.json`'s `pr_paths` that - the reply's evidence cites, + 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 d66d77259d..f367b697fb 100644 --- a/plugins/source-control/skills/pull-request/reference/comments-ci.md +++ b/plugins/source-control/skills/pull-request/reference/comments-ci.md @@ -31,10 +31,9 @@ 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`). Every kept thread's scope also holds each file in the thread file's - `pr_paths`, the files the pull request already changes, while the gated head holds it: a + 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, 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 ` reads a disposition file from the scratch directory, checks it @@ -71,7 +70,7 @@ The preamble names the tool's path. It is the only shell command this mode runs: `VALID` and `INCORRECT` need at least one `path:line` in the checkout, at most 10. A `VALID` fix that does not change the thread's own path must cite a line it changed in another of the - pull request's files, or its reply does not land. `fix_sha` + pull request's files, or its reply does not land, and nor does any other reply in the round. `fix_sha` stays empty, or is left out: the reply job finds the commit the push job made. `VALID-defer` is VALID (defer). The note is 1-2000 characters of plain prose: no control characters, no HTML comment (``), no token, no @-mention. It is posted in a code fence. diff --git a/plugins/source-control/skills/pull-request/scripts/feedback-thread.sh b/plugins/source-control/skills/pull-request/scripts/feedback-thread.sh index 4adf924ac0..28b510e868 100755 --- a/plugins/source-control/skills/pull-request/scripts/feedback-thread.sh +++ b/plugins/source-control/skills/pull-request/scripts/feedback-thread.sh @@ -50,15 +50,17 @@ scratch_dir=${FEEDBACK_SCRATCH:-} workspace=${FEEDBACK_WORKSPACE:-} # CODEOWNERS, a settings*.json, a package manifest or lockfile) and no script # or code file, a test file in its directory, or a path in its named_paths # (the files a write-access fix request named) in that same plugins/

/ -# directory that is no instruction path; or, when the gated head holds it, a -# file in the thread file's pr_paths (the files the pull request already +# 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 - git_tree cat-file -e "HEAD:$1" 2>/dev/null && tracked=true + 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" --argjson tracked "$tracked" ' def dir: sub("[^/]*$"; ""); diff --git a/plugins/source-control/skills/pull-request/scripts/feedback-thread.test.sh b/plugins/source-control/skills/pull-request/scripts/feedback-thread.test.sh index f15e9b7e33..53510e1cff 100755 --- a/plugins/source-control/skills/pull-request/scripts/feedback-thread.test.sh +++ b/plugins/source-control/skills/pull-request/scripts/feedback-thread.test.sh @@ -140,7 +140,11 @@ jq '.pr_paths = ["src/other.js", "src/new.js", ".github/workflows/ci.yml"]' \ "$STUB/threads.json" >"$STUB/t2" && mv "$STUB/t2" "$STUB/threads.json" run check-paths src/other.js >/dev/null 2>&1 assert_exit "check-paths allows a file pr_paths lists, outside every thread's own scope" 0 "$?" -for path in src/new.js .github/workflows/ci.yml src/helper.js; do +ln -s a.js "$STUB/ws/src/link.js" +fixture_git add -A +fixture_git commit -q -m link +jq '.pr_paths += ["src/link.js"]' "$STUB/threads.json" >"$STUB/t2" && mv "$STUB/t2" "$STUB/threads.json" +for path in src/new.js .github/workflows/ci.yml src/helper.js src/link.js; do run check-paths "$path" >/dev/null 2>&1 assert_exit "check-paths refuses $path with pr_paths set" 1 "$?" done From 4aca178bdeb8ec76f90c0d1100c00dd0314c1d29 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Fri, 23 Oct 2026 12:02:19 -0400 Subject: [PATCH 5/5] fix(pr-pipeline): keep control-plane and instruction surfaces out of pr_paths claude-security-review found that pr_paths put this pipeline's own scripts and actions, and agent-instruction files, in every thread's edit scope. pr_paths now leaves out 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/. A thread on such a file still reaches it through its own scope. Co-Authored-By: Claude Opus 5.5 --- ...feat-address-feedback-pr-paths-7c41e2a9.md | 2 +- .github/scripts/pr-unblock/thread-scope.mjs | 18 +++++++++++------- .../scripts/pr-unblock/thread-scope.test.mjs | 19 ++++++++++++++++--- .../pr-pipeline/pr-run-activity.md | 7 ++++--- .../pull-request/reference/comments-ci.md | 11 ++++++----- 5 files changed, 38 insertions(+), 19 deletions(-) diff --git a/.changes/pr-pipeline/feat-address-feedback-pr-paths-7c41e2a9.md b/.changes/pr-pipeline/feat-address-feedback-pr-paths-7c41e2a9.md index 4620bd84e4..7ec01cfbde 100644 --- a/.changes/pr-pipeline/feat-address-feedback-pr-paths-7c41e2a9.md +++ b/.changes/pr-pipeline/feat-address-feedback-pr-paths-7c41e2a9.md @@ -4,4 +4,4 @@ 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` and the HIGH floor's trusted-actor list, trusted-trigger check and `resolve-config`. `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. +- `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/.github/scripts/pr-unblock/thread-scope.mjs b/.github/scripts/pr-unblock/thread-scope.mjs index 2c4014fe88..c0d3d8fc10 100644 --- a/.github/scripts/pr-unblock/thread-scope.mjs +++ b/.github/scripts/pr-unblock/thread-scope.mjs @@ -30,21 +30,25 @@ const BAD_SEGMENT = /(^|\/)\.{0,2}(\/|$)/; // space, control or non-ASCII character. const PLAIN_PATH = /^[A-Za-z0-9_@+.-]+(\/[A-Za-z0-9_@+.-]+)*$/; -// The pr-merge HIGH floor's paths (babysit_risk.py HIGH_FLOOR) besides -// CODEOWNERS and .gitmodules, which isInstructionPath holds: the trusted-actor -// list, the trusted-trigger check and resolve-config. -const HIGH_FLOOR_PATH = /^\.github\/(standards\/trusted-actors|actions\/check-trusted-trigger|actions\/resolve-config)\//i; +// 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 on the -// HIGH floor. +// 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) && - !HIGH_FLOOR_PATH.test(path); + !CONTROL_PATH.test(path); // The paths a pull request's file listing (each { filename, // previous_filename }) changes, rename sources included, that prPathAllowed diff --git a/.github/scripts/pr-unblock/thread-scope.test.mjs b/.github/scripts/pr-unblock/thread-scope.test.mjs index cb487e0e1d..01c0eecbd2 100644 --- a/.github/scripts/pr-unblock/thread-scope.test.mjs +++ b/.github/scripts/pr-unblock/thread-scope.test.mjs @@ -251,7 +251,7 @@ test("a fix that adds a file the PR lists gets no pr_paths scope", () => { assert.equal(inThreadScope("plugins/p/skills/s/new.md", new Set([DOC]), [], { prPaths, added: true }), true); }); -test("a protected, instruction or HIGH floor path the PR changes is still refused", () => { +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", @@ -263,14 +263,27 @@ test("a protected, instruction or HIGH floor path the PR changes is still refuse ".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. + // 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([DOC]), [], { prPaths: new Set(refused) }), false, path); + assert.equal(inThreadScope(path, new Set(["docs/x.md"]), [], { prPaths: new Set(refused) }), false, path); } }); diff --git a/docs/conventions/pr-pipeline/pr-run-activity.md b/docs/conventions/pr-pipeline/pr-run-activity.md index 218200ca55..7ec70055ba 100644 --- a/docs/conventions/pr-pipeline/pr-run-activity.md +++ b/docs/conventions/pr-pipeline/pr-run-activity.md @@ -898,9 +898,10 @@ the base's `.github/scripts/pr-unblock/address-feedback.mjs`, `apply-patch.mjs` 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 under `.github/standards/trusted-actors/`, - `.github/actions/check-trusted-trigger/` or `.github/actions/resolve-config/` (the pr-merge - HIGH floor). A path the fix creates gets no scope from `pr_paths`: the tool checks it exists at + `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 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 f367b697fb..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,11 +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 (`.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, 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. + 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 ` reads a disposition file from the scratch directory, checks it against the template, and records it. Exit 1 means the tool refused and recorded nothing: fix the file and record again. One record per thread.