Repository navigation
fix(pr-pipeline): let address-feedback edit any file in its thread's plugin #8184
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
c5ff5f2
fix(pr-pipeline): let address-feedback edit its thread's eval case an…
kyle-sexton 95261dd
fix(pr-pipeline): read the scope copy's env in promote and refuse non…
kyle-sexton e91f012
Merge remote-tracking branch 'origin/main' into fix/pr-unblock-eval-c…
kyle-sexton 07d2739
fix(pr-pipeline): widen a kept thread's scope to its whole plugin dir…
kyle-sexton 8d71414
chore: merge origin/main into fix/pr-unblock-eval-case-scope
kyle-sexton 970ba31
fix(pr-pipeline): keep hooks, manifests, agent rules and code out of …
kyle-sexton 185553e
Merge origin/main into fix/pr-unblock-eval-case-scope
kyle-sexton 69d4029
Merge origin/main into fix/pr-unblock-eval-case-scope
kyle-sexton c19cd50
Merge remote-tracking branch 'origin/main' into fix/pr-unblock-eval-c…
kyle-sexton d52de51
fix(pr-pipeline): drop tracker back-reference from thread-scope test
kyle-sexton dead527
Merge origin/main into fix/pr-unblock-eval-case-scope
kyle-sexton 0e43863
Merge origin/main into fix/pr-unblock-eval-case-scope
kyle-sexton File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
7 changes: 7 additions & 0 deletions
7
.changes/source-control/fix-pr-unblock-eval-case-scope-ed5fddbc.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| --- | ||
| bump: patch | ||
| --- | ||
|
|
||
| ### Fixed | ||
|
|
||
| - The `comments --ci` review-thread tool's `check-paths` and `record` now also allow a file in the `plugins/<p>/` directory a kept thread's path sits in, except one under `hooks/`, `.claude-plugin/` or a `.claude/` directory, a `CLAUDE.md` or `AGENTS.md`, or a `.sh`, `.py`, `.mjs`, `.js` or `.ts` file, and a test file (`test_*.py`, `*_test.py`, `*.test.*`, `*.spec.*`) in the same directory as a kept thread's path. `.github/` stays refused. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,112 @@ | ||
| // Runs the review-threads path check of pr-run-activity-write.yml's promote | ||
| // step (the `script:` of the step with `id: promote`) against fake GitHub | ||
| // clients. A changed path in scope reaches the next gate, the hold-label read, | ||
| // which fails here because the case passes no hold labels. | ||
| import assert from "node:assert/strict"; | ||
| import { createHash } from "node:crypto"; | ||
| import { mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; | ||
| import { createRequire } from "node:module"; | ||
| import { tmpdir } from "node:os"; | ||
| import { join } from "node:path"; | ||
| import { after, test } from "node:test"; | ||
|
|
||
| const WORKFLOW = new URL("../../workflows/pr-run-activity-write.yml", import.meta.url); | ||
| const SCOPE = new URL("./thread-scope.mjs", import.meta.url); | ||
| const HEAD = "a".repeat(40); | ||
| const LANE = "b".repeat(40); | ||
|
|
||
| function promoteScript() { | ||
| const lines = readFileSync(WORKFLOW, "utf8").split("\n"); | ||
| const id = lines.findIndex((line) => /^\s+id: promote$/.test(line)); | ||
| assert.notEqual(id, -1, "the workflow has the promote step"); | ||
| const start = lines.findIndex((line, i) => i > id && /^\s+script: \|$/.test(line)); | ||
| const keyIndent = lines[start].search(/\S/); | ||
| const body = []; | ||
| for (const line of lines.slice(start + 1)) { | ||
| if (line.trim() !== "" && line.search(/\S/) <= keyIndent) break; | ||
| body.push(line); | ||
| } | ||
| const indent = Math.min(...body.filter((l) => l.trim()).map((l) => l.search(/\S/))); | ||
| return body.map((l) => l.slice(indent)).join("\n"); | ||
| } | ||
|
|
||
| const dir = mkdtempSync(join(tmpdir(), "promote-scope-")); | ||
| after(() => rmSync(dir, { recursive: true, force: true })); | ||
|
|
||
| async function promote(files, { scope = readFileSync(SCOPE), sha256 } = {}) { | ||
| const threadFile = join(dir, "threads.json"); | ||
| writeFileSync(threadFile, JSON.stringify({ threads: [{ path: "plugins/p/evals/c/case.yaml" }, { path: "src/round.py" }] })); | ||
| const scopeScript = join(dir, "thread-scope.mjs"); | ||
| writeFileSync(scopeScript, scope); | ||
| const env = { | ||
| HEAD_SHA: HEAD, | ||
| HEAD_REF: "feature", | ||
| LANE_BRANCH: "lane-commits/x", | ||
| MERGE_BASE: "false", | ||
| THREAD_FILE: threadFile, | ||
| SCOPE_SCRIPT: scopeScript, | ||
| SCOPE_SHA256: sha256 ?? createHash("sha256").update(readFileSync(SCOPE)).digest("hex"), | ||
| DEPENDENCY_REVIEW: "false", | ||
| HOLD_LABELS: "", | ||
| }; | ||
| const saved = { ...process.env }; | ||
| Object.assign(process.env, env); | ||
| const failed = []; | ||
| const core = { setOutput() {}, info() {}, warning() {}, setFailed: (message) => failed.push(message) }; | ||
| const github = { | ||
| request: async () => ({ data: { object: { sha: LANE } } }), | ||
| rest: { | ||
| repos: { compareCommitsWithBasehead: async () => ({ data: { files } }) }, | ||
| git: { deleteRef: async () => {} }, | ||
| }, | ||
| }; | ||
| const context = { repo: { owner: "o", repo: "r" } }; | ||
| try { | ||
| const AsyncFunction = (async () => {}).constructor; | ||
| await new AsyncFunction("github", "context", "core", "require", promoteScript())( | ||
| github, | ||
| context, | ||
| core, | ||
| createRequire(import.meta.url), | ||
| ); | ||
| } finally { | ||
| process.env = saved; | ||
| } | ||
| return failed; | ||
| } | ||
|
|
||
| const HOLD = /^No hold labels were read/; | ||
| const OUT = /^The lane branch changes a path outside the kept threads' scope or under \.github\//; | ||
|
|
||
| test("in-scope changes pass the path check: the thread's path, its plugin's files, a sibling test", async () => { | ||
| const failed = await promote([ | ||
| { filename: "plugins/p/evals/c/case.yaml" }, | ||
| { filename: "plugins/p/evals/c/samples/deep/new.json" }, | ||
| { filename: "plugins/p/evals/other/samples/new.json" }, | ||
| { filename: "plugins/p/README.md" }, | ||
| { filename: "src/test_round.py" }, | ||
| ]); | ||
| assert.equal(failed.length, 1); | ||
| assert.match(failed[0], HOLD); | ||
| }); | ||
|
|
||
| for (const file of [ | ||
| { filename: "plugins/q/evals/c/samples/new.json" }, | ||
| { filename: "src/lib/test_round.py" }, | ||
| { filename: ".github/workflows/x.yml" }, | ||
| { filename: "plugins/p/evals/c/x.json", previous_filename: ".github/x.yml" }, | ||
| ]) { | ||
| test(`an out-of-scope change stops the move: ${file.previous_filename ?? file.filename}`, async () => { | ||
| const failed = await promote([{ filename: "plugins/p/evals/c/case.yaml" }, file]); | ||
| assert.equal(failed.length, 1); | ||
| assert.match(failed[0], OUT); | ||
| }); | ||
| } | ||
|
|
||
| test("a rewritten scope copy, or no recorded hash, stops the move", async () => { | ||
| const permissive = "export const inThreadScope = () => true;\n"; | ||
| for (const options of [{ scope: permissive }, { sha256: "" }]) { | ||
| const failed = await promote([{ filename: ".github/workflows/x.yml" }], options); | ||
| assert.deepEqual(failed, ["thread-scope.mjs does not match the base's copy; the PR branch is not moved"]); | ||
| } | ||
| }); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
| // Which paths an address-feedback fix may change, shared by the promote step | ||
| // of .github/workflows/pr-run-activity-write.yml (which imports a SHA-256 | ||
| // pinned copy, so it imports nothing itself) and publish-feedback.mjs. | ||
|
|
||
| export function isProtectedPath(path) { | ||
| return /^\.github(\/|$)/i.test(String(path)); | ||
| } | ||
|
|
||
| const PLUGIN_DIR = /^plugins\/[^/]+\//; | ||
| // In the plugin directory but run or loaded as instructions by the harness or | ||
| // a shell, so out of the plugin-wide part: hooks, manifests, script or code | ||
| // files, and agent rules (CLAUDE.md, AGENTS.md, anything under .claude/). | ||
| const PLUGIN_CODE = | ||
| /^plugins\/[^/]+\/(hooks|\.claude-plugin)\/|\.(sh|py|mjs|js|ts)$|(^|\/)(claude|agents)\.md$|(^|\/)\.claude\//i; | ||
|
kyle-sexton marked this conversation as resolved.
Resolved
kyle-sexton marked this conversation as resolved.
Resolved
kyle-sexton marked this conversation as resolved.
Resolved
kyle-sexton marked this conversation as resolved.
Resolved
kyle-sexton marked this conversation as resolved.
Resolved
kyle-sexton marked this conversation as resolved.
Resolved
|
||
| const TEST_FILE = /^(test_[^/]*\.py|[^/]*_test\.py|[^/]*\.(test|spec)\.[^/]+)$/; | ||
| const dirOf = (path) => path.slice(0, path.lastIndexOf("/") + 1); | ||
|
|
||
| // An empty, . or .. segment, so a leading or doubled / too. | ||
| const BAD_SEGMENT = /(^|\/)\.{0,2}(\/|$)/; | ||
|
|
||
| // A kept thread's own path, a file in the plugins/<p>/ directory a kept | ||
| // thread's path sits in that is not under hooks/, .claude-plugin/ or a | ||
| // .claude/ directory, not a CLAUDE.md or AGENTS.md, and not a .sh, .py, .mjs, | ||
| // .js or .ts file, or a test file in the same directory as a | ||
| // kept thread's path; never anything under .github/, and never a path that is | ||
| // not in normal form. | ||
| export function inThreadScope(path, threadPaths) { | ||
| if (isProtectedPath(path) || BAD_SEGMENT.test(path)) { | ||
| return false; | ||
| } | ||
| if (threadPaths.has(path)) { | ||
| return true; | ||
| } | ||
| const kept = [...threadPaths]; | ||
| const plugin = PLUGIN_DIR.exec(path)?.[0]; | ||
| if (plugin !== undefined && !PLUGIN_CODE.test(path) && kept.some((p) => p.startsWith(plugin))) { | ||
| return true; | ||
| } | ||
| const dir = dirOf(path); | ||
| return TEST_FILE.test(path.slice(dir.length)) && kept.some((p) => dirOf(p) === dir); | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,131 @@ | ||
| import assert from "node:assert/strict"; | ||
| import { test } from "node:test"; | ||
|
|
||
| import { inThreadScope, isProtectedPath } from "./thread-scope.mjs"; | ||
|
|
||
| const kept = new Set(["plugins/p/evals/c/graders/x.md", "src/a.js"]); | ||
|
|
||
| test("a kept thread's own path is in scope", () => { | ||
| assert.equal(inThreadScope("src/a.js", kept), true); | ||
| assert.equal(inThreadScope("plugins/p/evals/c/graders/x.md", kept), true); | ||
| }); | ||
|
|
||
| // plugins/planning/README.md is the file #8178's run 39117146913 refused. | ||
| test("any file in the same plugins/<p>/ directory as a kept thread's path is in scope, at any depth", () => { | ||
| for (const path of [ | ||
| "plugins/p/evals/c/samples/y.json", | ||
| "plugins/p/evals/other/samples/deep/z.json", | ||
| "plugins/p/evals/evals.json", | ||
| "plugins/p/skills/s/SKILL.md", | ||
| "plugins/p/README.md", | ||
| ]) { | ||
| assert.equal(inThreadScope(path, kept), true, path); | ||
| } | ||
| assert.equal(inThreadScope("plugins/planning/README.md", new Set(["plugins/planning/surface/round.py"])), true); | ||
| }); | ||
|
|
||
| // Each refused path and the kept thread paths of the four recorded trips. | ||
| test("the four recorded trips are in scope", () => { | ||
| const trips = [ | ||
| // #8043, run 39100052790: an eval sample beside the thread's case. | ||
| ["plugins/session-flow/evals/rr-skill-fit-names-owner/samples/names-skill-and-plugin.json", ["plugins/session-flow/evals/rr-skill-fit-names-owner/case.yaml"]], | ||
| // #8103, run 39107762307: the thread is on case.yaml:18. | ||
| ["plugins/discipline/evals/ror-second-error-way/samples/second-way-named.json", ["plugins/discipline/evals/ror-second-error-way/case.yaml"]], | ||
| // #8142, run 39113747960: a .py file, in scope only as a sibling test. | ||
| ["plugins/planning/surface/test_round.py", ["plugins/planning/surface/round.py"]], | ||
| // #8178, run 39117146913: the PR's kept thread paths. | ||
| [ | ||
| "plugins/planning/README.md", | ||
| ["plugins/session-flow/skills/retro/SKILL.md", "plugins/planning/skills/draft-goal-condition/SKILL.md", "plugins/planning/.claude-plugin/plugin.json"], | ||
| ], | ||
| ]; | ||
| for (const [path, threads] of trips) { | ||
| assert.equal(inThreadScope(path, new Set(threads)), true, path); | ||
| } | ||
| }); | ||
|
|
||
| test("hooks, manifests, agent rules and non-test code in the same plugin are refused", () => { | ||
| const named = new Set(["plugins/p/skills/s/SKILL.md"]); | ||
| for (const path of [ | ||
| "plugins/p/hooks/hooks.json", | ||
| "plugins/p/hooks/guard.md", | ||
| "plugins/p/.claude-plugin/plugin.json", | ||
| "plugins/p/scripts/run.sh", | ||
| "plugins/p/skills/s/scripts/helper.py", | ||
| "plugins/p/lib/x.mjs", | ||
| "plugins/p/lib/x.js", | ||
| "plugins/p/lib/x.ts", | ||
| "plugins/p/lib/X.SH", | ||
| "plugins/p/AGENTS.md", | ||
| "plugins/p/skills/s/CLAUDE.md", | ||
| "plugins/p/.claude/rules/r.md", | ||
| "plugins/p/.claude/settings.json", | ||
| ]) { | ||
| assert.equal(inThreadScope(path, named), false, path); | ||
| } | ||
| }); | ||
|
|
||
| test("a thread's own path stays in scope whatever its type, and a sibling test of code stays in scope", () => { | ||
| for (const path of ["plugins/p/hooks/guard.sh", "plugins/p/.claude-plugin/plugin.json", "plugins/p/AGENTS.md"]) { | ||
| assert.equal(inThreadScope(path, new Set([path])), true, path); | ||
| } | ||
| assert.equal(inThreadScope("plugins/p/hooks/guard.test.mjs", new Set(["plugins/p/hooks/guard.mjs"])), true); | ||
| assert.equal(inThreadScope("plugins/p/scripts/test_run.py", new Set(["plugins/p/README.md"])), false); | ||
| }); | ||
|
|
||
| test("another plugin's file and an unnamed file outside plugins/ stay out of scope", () => { | ||
| assert.equal(inThreadScope("plugins/q/evals/c/samples/y.json", kept), false); | ||
| assert.equal(inThreadScope("plugins/pp/README.md", kept), false); | ||
| assert.equal(inThreadScope("src/b.js", kept), false); | ||
| assert.equal(inThreadScope("plugins/README.md", kept), false); | ||
| }); | ||
|
|
||
| test("with no kept thread under the plugin, its files are refused", () => { | ||
| assert.equal(inThreadScope("plugins/p/evals/c/samples/y.json", new Set(["src/a.js"])), false); | ||
| }); | ||
|
|
||
| // Names from the coordinator's addition; plugins/planning/surface/test_round.py | ||
| // is the file #8142's run 39113747960 refused. | ||
| test("a test file in the same directory as a kept thread's path is in scope", () => { | ||
| const surface = new Set(["plugins/planning/surface/round.py", "src/a.js", "README.md"]); | ||
| for (const path of [ | ||
| "plugins/planning/surface/test_round.py", | ||
| "plugins/planning/surface/round_test.py", | ||
| "src/a.test.js", | ||
| "src/a.spec.ts", | ||
| "test_root.py", | ||
| ]) { | ||
| assert.equal(inThreadScope(path, surface), true, path); | ||
| } | ||
| }); | ||
|
|
||
| test("outside plugins/, a test file in another directory, or a same-directory file that is no test, stays out of scope", () => { | ||
| const surface = new Set(["src/a.js"]); | ||
| for (const path of ["src/lib/a.test.js", "src/helper.js", "src/testing.js", "src/test_a.sh", "test_root.py"]) { | ||
| assert.equal(inThreadScope(path, surface), false, path); | ||
| } | ||
| }); | ||
|
|
||
| test("a test file under .github/ is refused even beside a kept thread's path there", () => { | ||
| assert.equal(inThreadScope(".github/scripts/x.test.mjs", new Set([".github/scripts/x.mjs"])), false); | ||
| }); | ||
|
|
||
| test("a path with an empty, . or .. segment, or a leading /, is refused before any other rule", () => { | ||
| for (const path of [ | ||
| "plugins/p/evals/c/../../../../.github/workflows/x.yml", | ||
| "plugins/p/evals/c/./samples/y.json", | ||
| "plugins/p/evals/c//y.json", | ||
| "/src/a.js", | ||
| "./src/a.js", | ||
| ]) { | ||
| assert.equal(inThreadScope(path, new Set([...kept, path])), false, path); | ||
| } | ||
| }); | ||
|
|
||
| test(".github/ is refused even when a kept thread names a file there", () => { | ||
| const github = new Set([".github/workflows/x.yml", ".GitHub/evals/c/a.md"]); | ||
| assert.equal(inThreadScope(".github/workflows/x.yml", github), false); | ||
| assert.equal(inThreadScope(".GitHub/evals/c/b.md", github), false); | ||
| assert.equal(isProtectedPath(".github"), true); | ||
| assert.equal(isProtectedPath(".githubx/a"), false); | ||
| }); |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.