From 5a06ef4a7cfaee2b93973b72220a93cdba182d3f Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Fri, 23 Oct 2026 21:43:01 -0400 Subject: [PATCH 1/3] feat(pr-pipeline): cut the lane hold-path floor to the merge gate's own resolve-config no longer forces .github/** into merge.diff-check.denied-paths, and merge-holds.mjs drops its path-wide plugin.json and marketplace.json globs. Its floor is the forced paths plus babysit_risk.HIGH_FLOOR, and ci-status compares a changed manifest base against head as the gate's manifest review does (manifestChanges, kept equal to babysit_risk._manifest_changes by test_merge_holds_sync). enable-auto-merge already scores manifests through evidence --lane-reviews. Co-Authored-By: Claude Opus 5.5 --- .../agent-ac72c39ea4f0fbf44-4968e884.md | 8 + .../agent-ac72c39ea4f0fbf44-57679074.md | 5 + .github/actions/resolve-config/README.md | 2 +- .../resolve-config/merge-authority.test.mjs | 2 +- .github/actions/resolve-config/resolve.mjs | 1 - .../actions/resolve-config/resolve.test.mjs | 6 +- .../pr-merge/enable-auto-merge.test.mjs | 37 ++- .../fixtures/enable-auto-merge-resolved.json | 2 +- .github/scripts/pr-merge/merge-holds.mjs | 267 ++++++++++++++++-- .github/scripts/pr-merge/merge-holds.test.mjs | 118 +++++++- .github/workflows/pr-require-checks.yml | 13 +- docs/conventions/pr-pipeline/README.md | 18 +- .../tests/test_babysit_lane_reviews.py | 40 ++- .../scripts/tests/test_babysit_risk.py | 6 + .../scripts/tests/test_merge_holds_sync.py | 175 ++++++++++-- scripts/ci-lanes-pr-hold.test.sh | 52 +++- 16 files changed, 646 insertions(+), 106 deletions(-) create mode 100644 .changes/pr-pipeline/agent-ac72c39ea4f0fbf44-4968e884.md create mode 100644 .changes/source-control/agent-ac72c39ea4f0fbf44-57679074.md diff --git a/.changes/pr-pipeline/agent-ac72c39ea4f0fbf44-4968e884.md b/.changes/pr-pipeline/agent-ac72c39ea4f0fbf44-4968e884.md new file mode 100644 index 00000000000..515b0888c28 --- /dev/null +++ b/.changes/pr-pipeline/agent-ac72c39ea4f0fbf44-4968e884.md @@ -0,0 +1,8 @@ +--- +bump: minor +--- + +### Changed + +- resolve-config no longer adds `.github/**` to `merge.diff-check.denied-paths`; it still adds the config file's own path and the trusted-actor list. A lanes App pull request touching `.github/scripts`, `.github/actions` or another `.github` path off the merge gate's HIGH floor is no longer held by report-outcome's `Hold:` line, `enable-auto-merge` or `ci-status`, and merges on the two review lanes (owner direction 2026-10-23). The lanes App's token carries no `workflows` permission, so a lane cannot push a workflow change. +- `merge-holds.mjs` no longer holds every `plugin.json` and `marketplace.json` path-wide. Its hold-path floor is the forced paths plus the merge gate's HIGH floor (`babysit_risk.HIGH_FLOOR`), and `ci-status` compares a changed manifest base against head as the gate's manifest review does (`manifestChanges`, the rule of `babysit_risk._manifest_changes`): an executable `plugin.json` key or `experimental.monitors`, a marketplace plugin added, removed, renamed or re-sourced or a marketplace `name` change, a removed, renamed or copied manifest, and a side that does not read or parse or repeats a key hold; a `userConfig` default or version change merges. `enable-auto-merge` arms such a change once the gate's risk score (`evidence --lane-reviews`) allows it. ci-status now hands the module each file's status and the base commit. diff --git a/.changes/source-control/agent-ac72c39ea4f0fbf44-57679074.md b/.changes/source-control/agent-ac72c39ea4f0fbf44-57679074.md new file mode 100644 index 00000000000..7a3bbe165ed --- /dev/null +++ b/.changes/source-control/agent-ac72c39ea4f0fbf44-57679074.md @@ -0,0 +1,5 @@ +--- +bump: none +--- + +Only tests of the babysit-prs scripts change; no shipped behavior changes. diff --git a/.github/actions/resolve-config/README.md b/.github/actions/resolve-config/README.md index 32c538d7769..f79353b1be8 100644 --- a/.github/actions/resolve-config/README.md +++ b/.github/actions/resolve-config/README.md @@ -118,7 +118,7 @@ trusted-actor list or the lanes App, and a field with no counted value reads as known; an option no entry maps fails the step (`unmapped-dimension`); an applying slot becomes `research-first`, `probe-first` or `split-required` in that order ([README, Models](../../../docs/conventions/pr-pipeline/README.md#models)). A slot's `applies-when` replaces its activity's. `merge.diff-check.denied-paths` always -gains the config path, `.github/**` and the trusted-actor list. +gains the config path and the trusted-actor list. The lane's slots are its leaves: each composite is replaced by its children, depth first in list order. A leaf is enabled only when the lane, every slot on its path and its own slot are; it must diff --git a/.github/actions/resolve-config/merge-authority.test.mjs b/.github/actions/resolve-config/merge-authority.test.mjs index 78c46fe9472..1aae3821623 100644 --- a/.github/actions/resolve-config/merge-authority.test.mjs +++ b/.github/actions/resolve-config/merge-authority.test.mjs @@ -57,7 +57,7 @@ const rejectedFor = (config, pattern) => // forced denied paths. const VALID_MERGE = { rung: "off", - "diff-check": { "denied-paths": [CONFIG_PATH, ".github/**", TRUSTED_ACTORS] }, + "diff-check": { "denied-paths": [CONFIG_PATH, TRUSTED_ACTORS] }, "stack-landing": "manual", "lane-prs": "native", }; diff --git a/.github/actions/resolve-config/resolve.mjs b/.github/actions/resolve-config/resolve.mjs index 55a75e3e1b9..3bbf6785031 100644 --- a/.github/actions/resolve-config/resolve.mjs +++ b/.github/actions/resolve-config/resolve.mjs @@ -941,7 +941,6 @@ function resolveMerge(merge = {}, configPath) { ...new Set([ ...(diffCheck["denied-paths"] ?? []), configPath, - ".github/**", TRUSTED_ACTORS_PATH, ]), ]; diff --git a/.github/actions/resolve-config/resolve.test.mjs b/.github/actions/resolve-config/resolve.test.mjs index fb001f8467a..72f46484cce 100644 --- a/.github/actions/resolve-config/resolve.test.mjs +++ b/.github/actions/resolve-config/resolve.test.mjs @@ -302,7 +302,7 @@ const TRUSTED_ACTORS = ".github/standards/trusted-actors/trusted-actors.json"; test("forced denied paths are added when the config sets no merge section", () => { assert.deepEqual( run(fixture("valid.yaml")).merge["diff-check"]["denied-paths"], - [CONFIG_PATH, ".github/**", TRUSTED_ACTORS], + [CONFIG_PATH, TRUSTED_ACTORS], ); }); @@ -311,14 +311,14 @@ test("forced denied paths are added after the config's own, once each", () => { doc.merge = { rung: "C2", "diff-check": { - "denied-paths": [".claude/**", ".github/**"], + "denied-paths": [".claude/**", CONFIG_PATH], "max-changed-lines": 400, }, }; }); const resolved = run(config); assert.deepEqual(resolved.merge["diff-check"], { - "denied-paths": [".claude/**", ".github/**", CONFIG_PATH, TRUSTED_ACTORS], + "denied-paths": [".claude/**", CONFIG_PATH, TRUSTED_ACTORS], "max-changed-lines": 400, }); }); diff --git a/.github/scripts/pr-merge/enable-auto-merge.test.mjs b/.github/scripts/pr-merge/enable-auto-merge.test.mjs index bac7dbf3a8a..c7d7b656b64 100644 --- a/.github/scripts/pr-merge/enable-auto-merge.test.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.test.mjs @@ -376,28 +376,38 @@ test("the comment veto agrees with the gate's HUMAN_MERGE_VETO_RE", () => { // a later push can add a held path to a pull request that is already armed. test("a pull request whose head now changes a hold path is not armed, or is disarmed", async () => { const { writes } = await run([ - pull(1, { files: [{ filename: ".github/workflows/x.yml" }] }), - pull(2, { autoMergeRequest: {}, files: [{ filename: "plugins/a/README.md" }, { filename: "docs/a.md", previous_filename: ".github/a.md" }] }), + pull(1, { files: [{ filename: "docs/conventions/pr-pipeline.yaml" }] }), + pull(2, { + autoMergeRequest: {}, + files: [{ filename: "plugins/a/README.md" }, { filename: "docs/a.md", previous_filename: ".github/standards/trusted-actors/trusted-actors.json" }], + }), pull(3, { files: Array.from({ length: 301 }, (_, index) => ({ filename: `docs/${index}.md` })) }), pull(4, { files: [{ filename: "docs/held.md" }] }), pull(5), + // The rest of .github merges on the review lanes (owner direction 2026-10-23). + pull(6, { files: [{ filename: ".github/workflows/x.yml" }, { filename: ".github/scripts/pr-merge/merge-holds.mjs" }] }), ]); assert.deepEqual(writes, [ { mutation: "disarm", id: "PR_2" }, { mutation: "arm", id: "PR_5", head: HEAD }, + { mutation: "arm", id: "PR_6", head: HEAD }, ]); }); -// The gate scores the marketplace manifest HIGH (babysit_risk.is_manifest_path), -// so it is a hold path like every plugin manifest. -test("a pull request whose head changes the marketplace manifest is not armed, or is disarmed", () => { +// A manifest is no hold path: the gate's reader scores it (`evidence +// --lane-reviews`, babysit_risk manifest-review-required), so a manifest +// change the gate reviews is not armed, or is disarmed, and one it does not, +// such as a plugin.json userConfig default (#9483), arms. +test("a manifest change arms only when the gate's risk score allows it", () => { const options = { bot: LANES.login, humanGated: "needs-human", holdPaths: [] }; - const files = ["docs/a.md", ".claude-plugin/marketplace.json"]; - assert.deepEqual(decide(pull(1), options, { ...detail(), files, verify: PASSED }), { + const files = ["docs/a.md", ".claude-plugin/marketplace.json", "plugins/a/.claude-plugin/plugin.json"]; + const review = { ...PASSED, reviews: "manifest-review-required: plugins/a/.claude-plugin/plugin.json: hooks" }; + assert.deepEqual(decide(pull(1), options, { ...detail(), files, verify: review }), { action: "none", - reason: "it changes .claude-plugin/marketplace.json, a hold path", + reason: "pr-merge's gate holds it on its risk score (manifest-review-required: plugins/a/.claude-plugin/plugin.json: hooks)", }); - assert.equal(decide(pull(1, { autoMergeRequest: {} }), options, { ...detail(), files, verify: PASSED }).action, "disarm"); + assert.equal(decide(pull(1, { autoMergeRequest: {} }), options, { ...detail(), files, verify: review }).action, "disarm"); + assert.deepEqual(decide(pull(1), options, { ...detail(), files, verify: PASSED }), { action: "arm", reason: "no hold" }); }); // The gate holds a cost path (babysit_risk.COST_PATHS) until the body links @@ -446,12 +456,15 @@ test("merge-holds.mjs holds a changed or renamed-from hold path from the command return spawnSync(process.execPath, [fileURLToPath(new URL("./merge-holds.mjs", import.meta.url)), ...args], { encoding: "utf8" }); }; assert.equal(cli([["docs/a.md"]], ["docs/held.md"]).status, 0); - assert.match(cli([[".github/workflows/.x.yml"]], []).stdout, /it changes \.github\/workflows\/\.x\.yml, a hold path/); + assert.match(cli([[".github/actions/resolve-config/.x.mjs"]], []).stdout, /it changes \.github\/actions\/resolve-config\/\.x\.mjs, a hold path/); + assert.equal(cli([[".github/workflows/x.yml"]], []).status, 0); + // A manifest's sides are read at BASE_SHA and the head, none given here. + assert.match(cli([{ filename: "plugins/a/.claude-plugin/plugin.json", status: "modified" }], []).stdout, /plugin\.json \(contents unreadable\), a manifest change/); assert.match(cli([["plugins/a/.hidden"]], ["plugins/**"]).stdout, /it changes plugins\/a\/\.hidden, a hold path/); assert.equal(cli([["plugins/a/x.md"]], ["plugins/*.md"]).status, 0); - const renamed = cli([["docs/a.md", ".github/a.md"]], []); + const renamed = cli([["docs/a.md", ".github/actions/check-trusted-trigger/a.md"]], []); assert.equal(renamed.status, 1); - assert.match(renamed.stdout, /it changes \.github\/a\.md, a hold path/); + assert.match(renamed.stdout, /it changes \.github\/actions\/check-trusted-trigger\/a\.md, a hold path/); assert.match(cli([["docs/held.md"]], ["docs/held.md"]).stdout, /it changes docs\/held\.md, a hold path/); assert.match(cli([["docs/a.md"]], "docs/held.md").stdout, /the hold paths could not be read/); const approve = (id, commit) => ({ user: { login: `u${id}`, id, type: "User" }, state: "APPROVED", commit_id: commit, submitted_at: "2026-10-23T11:00:00Z" }); diff --git a/.github/scripts/pr-merge/fixtures/enable-auto-merge-resolved.json b/.github/scripts/pr-merge/fixtures/enable-auto-merge-resolved.json index 47a76fc4cdd..79931726c3b 100644 --- a/.github/scripts/pr-merge/fixtures/enable-auto-merge-resolved.json +++ b/.github/scripts/pr-merge/fixtures/enable-auto-merge-resolved.json @@ -1,7 +1,7 @@ { "merge": { "diff-check": { - "denied-paths": [".github/**", "docs/held.md"] + "denied-paths": ["docs/held.md", "docs/conventions/pr-pipeline.yaml", ".github/standards/trusted-actors/trusted-actors.json"] } } } diff --git a/.github/scripts/pr-merge/merge-holds.mjs b/.github/scripts/pr-merge/merge-holds.mjs index cdd147e8d02..7af97debe8c 100755 --- a/.github/scripts/pr-merge/merge-holds.mjs +++ b/.github/scripts/pr-merge/merge-holds.mjs @@ -16,6 +16,10 @@ // merge.diff-check.denied-paths plus HOLD_PATH_FLOOR. A person on the // trusted-actor list lifts it by approving the head: their newest review of // the head that approves, requests changes or was dismissed must approve. +// - A manifest change the merge gate reviews (manifestChanges, the rule of +// babysit_risk._manifest_changes): a plugin manifest or the marketplace +// manifest, read at the base commit (BASE_SHA) and the head and compared +// key by key. The same approval lifts it. // - A cost path (COST_PATHS): a changed file the gate holds for the owner's // cost decision. An approval does not lift it; only the decision the gate // reads does (costDecision), linked from the body. @@ -41,12 +45,13 @@ // per line, run as GET actions/runs/{runId} gives it, for each unexpired artifact // named a pass at the head; GITHUB_REPOSITORY names the repository. The // discussion file holds one REST issue comment, review or review comment per -// line; the files file one JSON array per changed file, its path then any -// renamed-from path; the denied-paths file the config's +// line; the files file one changed file per line, its path, renamed-from +// path and status (changeOf); the denied-paths file the config's // merge.diff-check.denied-paths as JSON; the trusted-actors file // trusted-actors.json. Each hold prints as an ::error:: and exits 1; no hold -// exits 0. A cost path with a decision link in the body reads the decision -// with GH_TOKEN. +// exits 0. A changed manifest, and a cost path with a decision link in the +// body, are read with GH_TOKEN. +import { Buffer } from "node:buffer"; import { readFileSync } from "node:fs"; import { posix } from "node:path"; import process from "node:process"; @@ -138,20 +143,194 @@ const MEMBERS = new Set(["OWNER", "MEMBER", "COLLABORATOR"]); // merge-authority.test.mjs checks the two agree. export const FORCED_DENIED_PATHS = Object.freeze([ "docs/conventions/pr-pipeline.yaml", - ".github/**", ".github/standards/trusted-actors/trusted-actors.json", ]); -// Every hold path check adds these: the forced paths, the rest of the merge -// gate's HIGH floor (babysit_risk.HIGH_FLOOR), and every plugin manifest and -// the marketplace manifest (babysit_risk.is_manifest_path), a path-wide -// stand-in for the gate's manifest checks. +// Every hold path check adds these: the forced paths and the merge gate's +// HIGH floor (babysit_risk.HIGH_FLOOR), which test_merge_holds_sync.py checks +// this covers. A manifest is compared base against head instead +// (manifestChanges). export const HOLD_PATH_FLOOR = Object.freeze([ ...FORCED_DENIED_PATHS, + ".github/standards/trusted-actors/**", + ".github/actions/check-trusted-trigger/**", "**/CODEOWNERS", "**/.gitmodules", - "**/.claude-plugin/plugin.json", - ".claude-plugin/marketplace.json", + ".github/actions/resolve-config/**", ]); + +// babysit_risk.is_manifest_path: a plugin manifest or the marketplace +// manifest, matched case-sensitively. +export function isManifestPath(path) { + const parts = String(path).split("/"); + return ( + (parts.length === 2 && parts[0] === ".claude-plugin" && parts[1] === "marketplace.json") || + (parts.length === 4 && parts[0] === "plugins" && parts[2] === ".claude-plugin" && parts[3] === "plugin.json") + ); +} + +// babysit_risk._manifest: the parsed object as a node tree, or null when the +// text is not a string, not JSON, not an object, or repeats a key. A node is +// { object: Map }, { array: [] } or { text }, a scalar's text telling values +// apart as Python's json.loads does: an integer and a float differ (1 and +// 1.0), so canonical (json.dumps with sort_keys) compares them as the gate. +function parseManifest(text) { + if (typeof text !== "string") { + return null; + } + try { + JSON.parse(text); + } catch { + return null; + } + // The text is valid JSON, so the walk below needs no error handling. + let at = 0; + const space = () => { + while (" \t\n\r".includes(text[at] ?? "x")) at += 1; + }; + const string = () => { + const start = at; + for (at += 1; text[at] !== '"'; at += 1) { + if (text[at] === "\\") at += 1; + } + at += 1; + return JSON.parse(text.slice(start, at)); + }; + const items = (close, read) => { + at += 1; + for (space(); text[at] !== close; space()) { + read(); + space(); + if (text[at] === ",") at += 1; + } + at += 1; + }; + const value = () => { + space(); + if (text[at] === "{") { + const object = new Map(); + items("}", () => { + const key = string(); + if (object.has(key)) throw new Error("duplicate key"); + space(); + at += 1; + object.set(key, value()); + }); + return { object }; + } + if (text[at] === "[") { + const array = []; + items("]", () => array.push(value())); + return { array }; + } + if (text[at] === '"') return { text: JSON.stringify(string()), string: true }; + const token = /^(?:true|false|null|-?\d+(?:\.\d+)?(?:[eE][+-]?\d+)?)/.exec(text.slice(at))[0]; + at += token.length; + if (!/^-?\d/.test(token)) return { text: token }; + if (!/[.eE]/.test(token)) return { text: `${BigInt(token)}` }; + const number = Number(token); + return { text: `${Object.is(number, -0) ? "-0" : String(number)}.f` }; + }; + try { + const root = value(); + return root.object ? root : null; + } catch { + return null; + } +} + +const canonical = (node) => + node.object + ? `{${[...node.object] + .sort(([a], [b]) => (a < b ? -1 : a > b ? 1 : 0)) + .map(([key, child]) => `${JSON.stringify(key)}:${canonical(child)}`) + .join(",")}}` + : node.array + ? `[${node.array.map(canonical).join(",")}]` + : node.text; +// babysit_risk._canonical: a key's canonical value, null when absent or +// `node` is no object. +const canonicalOf = (node, key) => (node?.object?.has(key) ? canonical(node.object.get(key)) : null); +// babysit_risk._EXECUTABLE_KEYS. +const EXECUTABLE_KEYS = Object.freeze(["hooks", "mcpServers", "lspServers", "monitors"]); + +function pluginChanges(base, head) { + const found = EXECUTABLE_KEYS.filter((key) => canonicalOf(base, key) !== canonicalOf(head, key)); + const monitors = (node) => canonicalOf(node.object.get("experimental"), "monitors"); + return monitors(base) === monitors(head) ? found : [...found, "experimental.monitors"]; +} + +// `plugins` by name, null unless a list of uniquely named objects. +function entries(node) { + const plugins = node.object.get("plugins") ?? { array: [] }; + if (!plugins.array) { + return null; + } + const named = new Map(); + for (const entry of plugins.array) { + const name = entry.object?.get("name"); + if (name?.string) { + named.set(JSON.parse(name.text), entry); + } + } + return named.size === plugins.array.length ? named : null; +} + +function marketplaceChanges(base, head) { + const found = canonicalOf(base, "name") === canonicalOf(head, "name") ? [] : ["name"]; + const old = entries(base); + const now = entries(head); + if (old === null || now === null) { + return [...found, "plugins is not a list of uniquely named entries"]; + } + for (const name of [...new Set([...old.keys(), ...now.keys()])].sort()) { + if (!now.has(name)) { + found.push(`plugins[${name}] removed`); + } else if (!old.has(name)) { + found.push(`plugins[${name}] added`); + } else if (canonicalOf(old.get(name), "source") !== canonicalOf(now.get(name), "source")) { + found.push(`plugins[${name}].source`); + } + } + return found; +} + +// babysit_risk._manifest_changes: what in one changed manifest needs a +// person's review, empty when nothing does; test_merge_holds_sync.py checks +// the two agree. `change` is { path, previous, status, contents }, contents +// [base text, head text] with null for an absent side. In plugin.json an +// executable key or experimental.monitors that differs, in marketplace.json +// a plugin added, removed or renamed, a changed entry source or marketplace +// name; a manifest removed, renamed or copied, of any other status, or with a +// side that does not read, parse or repeats a key holds. An added plugin.json +// is compared with {}. +export function manifestChanges({ path, previous = null, status, contents } = {}) { + if (!["modified", "added"].includes(status) || previous !== null) { + return [`${status ?? "unknown"} manifest`]; + } + if (!Array.isArray(contents) || contents.length !== 2) { + return ["contents unreadable"]; + } + const base = parseManifest(status === "added" ? "{}" : contents[0]); + const head = parseManifest(contents[1]); + if (base === null || head === null) { + return ["unreadable, malformed or a repeated key"]; + } + return path.endsWith("marketplace.json") ? marketplaceChanges(base, head) : pluginChanges(base, head); +} + +// Why the changed manifests in `changes` (manifestChanges's entries) hold +// the head, or null. A manifest renamed away holds too. +export function manifestHold(changes) { + if (!Array.isArray(changes)) { + return "its changed manifests could not be read"; + } + const held = changes + .filter(({ path, previous }) => isManifestPath(path) || (previous != null && isManifestPath(previous))) + .map((change) => [change.path, manifestChanges(change)]) + .filter(([, found]) => found.length); + return held.length ? `it changes ${held.map(([path, found]) => `${path} (${found.join(", ")})`).join("; ")}, a manifest change that needs review` : null; +} + // babysit_risk.COST_PATHS, matched case-folded as its glob_match does; // test_merge_holds_sync.py checks the two agree. export const COST_PATHS = Object.freeze([ @@ -337,12 +516,13 @@ export function pathLifters(items, humans, head) { } // Why the pull request's `body`, discussion `items` and changed `files` hold -// it; empty when nothing does. `lifters` approved the head, which lifts a hold -// path but not an unreadable file list or hold-path list, nor a cost path, -// which only the owner's `decision` lifts. -export function holds(body, items, files, holdPaths, lifters = [], decision = null) { +// it; empty when nothing does. `manifest` is manifestHold's reason. `lifters` +// approved the head, which lifts a hold path or a manifest change but not an +// unreadable file list or hold-path list, nor a cost path, which only the +// owner's `decision` lifts. +export function holds(body, items, files, holdPaths, lifters = [], decision = null, manifest = null) { const reasons = []; - const pathReason = pathHold(files, holdPaths); + const pathReason = pathHold(files, holdPaths) ?? manifest; const readable = Array.isArray(files) && Array.isArray(holdPaths); if (pathReason && !(readable && lifters.length)) { reasons.push( @@ -417,6 +597,53 @@ function linkOf(file, body, repo) { } } +// One files-file line as a manifestChanges entry: [filename, +// previous_filename or null, status] from the pull request files API, or an +// object of those keys. An older line of one path and any renamed-from path +// has no status, so a manifest on it holds. A module older than this one +// reads every entry of a line as a path, null and the status matching no +// hold path. +export function changeOf(line) { + if (Array.isArray(line)) { + const [path, previous = null, status = null] = line; + return { + path: String(path), + previous: previous === null ? null : String(previous), + status: line.length === 3 && typeof status === "string" ? status : null, + }; + } + if (line === null || typeof line !== "object" || typeof line.filename !== "string") { + throw new Error("a changed file does not read"); + } + const previous = line.previous_filename; + return { path: line.filename, previous: typeof previous === "string" && previous ? previous : null, status: typeof line.status === "string" ? line.status : null }; +} + +// `changes` with each modified or added manifest's [base text at `base`, +// head text at `head`], base null for an added one; a read that fails, or a +// base that is not a commit SHA, leaves its contents out, so it holds. +export async function withContents(github, repo, changes, base, head) { + const text = async (path, ref) => { + const file = await github("GET", `/repos/${repo}/contents/${path.split("/").map(encodeURIComponent).join("/")}?ref=${ref}`); + if (file?.type !== "file" || file.encoding !== "base64" || typeof file.content !== "string") { + throw new Error(`${path} at ${ref} did not read`); + } + return Buffer.from(file.content, "base64").toString("utf8"); + }; + return Promise.all( + changes.map(async (change) => { + if (!isManifestPath(change.path) || !["modified", "added"].includes(change.status) || change.previous !== null || !/^[0-9a-f]{40}$/.test(String(base))) { + return change; + } + try { + return { ...change, contents: [change.status === "added" ? null : await text(change.path, base), await text(change.path, head)] }; + } catch { + return change; + } + }), + ); +} + export async function main(argv = process.argv.slice(2), { env = process.env, github = fetchGitHub(env) } = {}) { const [number, bodyFile, discussionFile, filesFile, deniedFile, actorsFile, head, verdictsFile, linkFile] = argv; try { @@ -435,7 +662,8 @@ export async function main(argv = process.argv.slice(2), { env = process.env, gi } const lines = (file) => readFileSync(file, "utf8").split("\n").filter((line) => line.trim()); const items = lines(discussionFile).map((line) => fromRest(JSON.parse(line))); - const files = lines(filesFile).flatMap((line) => JSON.parse(line).map(String)); + const changes = lines(filesFile).map((line) => changeOf(JSON.parse(line))); + const files = changes.flatMap(({ path, previous }) => [path, ...(previous === null ? [] : [previous])]); const holdPaths = holdPathsOf(JSON.parse(readFileSync(deniedFile, "utf8"))); const actors = JSON.parse(readFileSync(actorsFile, "utf8"))?.actors; if (!Array.isArray(actors)) { @@ -450,8 +678,9 @@ export async function main(argv = process.argv.slice(2), { env = process.env, gi if (decision) { console.log(`#${number}: ${decision} records the owner's cost decision, which lifts its cost hold.`); } - const reasons = holds(body, items, files, holdPaths, lifters, decision); - const held = pathHold(files, holdPaths); + const manifest = manifestHold(await withContents(github, repo, changes, env.BASE_SHA, head)); + const reasons = holds(body, items, files, holdPaths, lifters, decision, manifest); + const held = pathHold(files, holdPaths) ?? manifest; if (held && lifters.length && Array.isArray(holdPaths)) { console.log(`#${number}: ${lifters.join(", ")} approved this head, which lifts its hold path (${escape(held)}).`); } diff --git a/.github/scripts/pr-merge/merge-holds.test.mjs b/.github/scripts/pr-merge/merge-holds.test.mjs index 336e1d47e3c..fd3d612338c 100644 --- a/.github/scripts/pr-merge/merge-holds.test.mjs +++ b/.github/scripts/pr-merge/merge-holds.test.mjs @@ -1,24 +1,30 @@ // Expected values come from the issue that added these holds and the gate's -// own rules: the merge gate scores a marketplace manifest change HIGH -// (babysit_risk.is_manifest_path) and holds a change to a cost path +// own rules: the merge gate holds a manifest change that installs or runs +// something new (babysit_risk manifest-review-required, the classes #9280 +// set: an executable plugin.json key or experimental.monitors; a marketplace +// plugin added, removed, renamed or re-sourced, or its name; a manifest +// removed, renamed or copied; a side that does not read or parse or repeats a +// key) and lets a userConfig default merge (#9483), holds a change to a cost path // (babysit_risk.COST_PATHS) until the owner's cost decision, which // babysit_merge_policy.cost_decision reads and test_babysit_merge_cost.py // pins case by case. ci-status names its owner by the trusted-actor list, as // its cost-only review hold step does, so a review approval lifts a hold path // but never a cost hold. import assert from "node:assert/strict"; +import { Buffer } from "node:buffer"; import { mkdtempSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { test } from "node:test"; -import { COMMENT_EDITED_QUERY, PULL_OPENED_QUERY, costDecision, costHold, linkHold, main } from "./merge-holds.mjs"; +import { COMMENT_EDITED_QUERY, PULL_OPENED_QUERY, changeOf, costDecision, costHold, linkHold, main } from "./merge-holds.mjs"; const REPO = "owner/repo"; const LINK = "https://github.com/owner/repo/issues/40#issuecomment-7"; const PR_CREATED = "2026-10-22T10:00:00Z"; const HUMANS = new Set([5]); const HEAD = "0123456789abcdef0123456789abcdef01234567"; +const BASE = "89abcdef0123456789abcdef0123456789abcdef"; const comment = (changes = {}) => ({ issue_url: "https://api.github.com/repos/owner/repo/issues/40", @@ -129,19 +135,32 @@ test("costHold holds a cost path, case-folded, until a decision", () => { // ci-status: merge-holds.mjs's command line on the live files, as the // workflow step runs it, with a GitHub for the decision reads. -function ci(files, { reviews = [], body = "Closes #40", options } = {}) { +// A `files` entry is a path, or a files API row; `texts` maps +// `@` to a manifest's text, which the contents API answers. +function ci(files, { reviews = [], body = "Closes #40", options, texts = {} } = {}) { const dir = mkdtempSync(join(tmpdir(), "merge-holds-")); writeFileSync(join(dir, "body"), body); writeFileSync(join(dir, "discussion"), reviews.map((row) => JSON.stringify(row)).join("\n")); writeFileSync(join(dir, "actors"), JSON.stringify({ actors: [{ kind: "human", login: "owner-person", id: 5 }] })); - writeFileSync(join(dir, "files"), files.map((path) => JSON.stringify([path])).join("\n")); + writeFileSync(join(dir, "files"), files.map((row) => JSON.stringify(typeof row === "string" ? [row] : row)).join("\n")); writeFileSync(join(dir, "denied"), "[]"); const argv = ["99", ...["body", "discussion", "files", "denied", "actors"].map((name) => join(dir, name)), HEAD]; - const { github } = world(options); + const { github: decisions } = world(options); + const github = async (method, path, data) => { + const read = /^\/repos\/owner\/repo\/contents\/(.+)\?ref=([0-9a-f]{40})$/.exec(path); + if (!read) { + return decisions(method, path, data); + } + const text = texts[`${decodeURIComponent(read[1])}@${read[2]}`]; + if (text === undefined) { + throw new Error("not found"); + } + return { type: "file", encoding: "base64", content: Buffer.from(text).toString("base64") }; + }; const lines = []; const log = console.log; console.log = (line) => lines.push(line); - return main(argv, { env: { GITHUB_REPOSITORY: REPO }, github }).then( + return main(argv, { env: { GITHUB_REPOSITORY: REPO, BASE_SHA: BASE }, github }).then( (code) => { console.log = log; return { code, out: lines.join("\n") }; @@ -154,11 +173,78 @@ function ci(files, { reviews = [], body = "Closes #40", options } = {}) { } const approve = { user: { login: "owner-person", id: 5, type: "User" }, state: "APPROVED", commit_id: HEAD, submitted_at: "2026-10-23T11:00:00Z" }; -test("ci-status holds the marketplace manifest, and a trusted approval of the head lifts it", async () => { - const held = await ci([".claude-plugin/marketplace.json"]); +const PLUGIN = "plugins/foo/.claude-plugin/plugin.json"; +const MARKET = ".claude-plugin/marketplace.json"; +const PLUGIN_BASE = '{"name": "foo", "version": "1.0.0", "userConfig": {"guard_enabled": {"type": "boolean", "default": true}}}'; +const MARKET_BASE = '{"name": "m", "plugins": [{"name": "foo", "source": "./plugins/foo", "category": "dev"}]}'; +const withKey = (key) => PLUGIN_BASE.replace('"name": "foo",', `"name": "foo", ${key},`); +// One manifest change through ci-status: its files API row and its sides. +const manifest = (path, before, after, row = {}) => + ci([{ filename: path, status: "modified", ...row }], { + texts: { ...(before === undefined ? {} : { [`${path}@${BASE}`]: before }), ...(after === undefined ? {} : { [`${path}@${HEAD}`]: after }) }, + }); + +test("ci-status holds every manifest change the merge gate reviews", async () => { + const cases = { + hooks: [PLUGIN, PLUGIN_BASE, withKey('"hooks": "./h.json"'), "hooks"], + mcpServers: [PLUGIN, PLUGIN_BASE, withKey('"mcpServers": {}'), "mcpServers"], + lspServers: [PLUGIN, PLUGIN_BASE, withKey('"lspServers": {}'), "lspServers"], + monitors: [PLUGIN, PLUGIN_BASE, withKey('"monitors": []'), "monitors"], + "experimental.monitors": [PLUGIN, PLUGIN_BASE, withKey('"experimental": {"monitors": []}'), "experimental.monitors"], + "a plugin added": [MARKET, MARKET_BASE, MARKET_BASE.replace("]}", ', {"name": "bar", "source": "./plugins/bar"}]}'), "plugins[bar] added"], + "a plugin removed": [MARKET, MARKET_BASE, '{"name": "m", "plugins": []}', "plugins[foo] removed"], + "a plugin renamed": [MARKET, MARKET_BASE, MARKET_BASE.replace('"name": "foo"', '"name": "fob"'), "plugins[fob] added, plugins[foo] removed"], + "a plugin source changed": [MARKET, MARKET_BASE, MARKET_BASE.replace("./plugins/foo", "https://example.com/foo.git"), "plugins[foo].source"], + "the marketplace name changed": [MARKET, MARKET_BASE, MARKET_BASE.replace('"name": "m"', '"name": "n"'), "name"], + "a parse error": [PLUGIN, PLUGIN_BASE, "{", "unreadable, malformed or a repeated key"], + "a duplicate key": [PLUGIN, PLUGIN_BASE, withKey('"name": "x"'), "unreadable, malformed or a repeated key"], + "an unreadable side": [PLUGIN, undefined, PLUGIN_BASE, "contents unreadable"], + }; + for (const [name, [path, before, after, found]] of Object.entries(cases)) { + const held = await manifest(path, before, after); + assert.equal(held.code, 1, name); + assert.ok(held.out.includes(`it changes ${path} (${found}), a manifest change that needs review`), `${name}: ${held.out}`); + } + for (const [name, row] of Object.entries({ + removed: { status: "removed" }, + renamed: { status: "renamed", previous_filename: "plugins/foo/plugin.json" }, + copied: { status: "copied", previous_filename: "plugins/bar/.claude-plugin/plugin.json" }, + })) { + const held = await manifest(PLUGIN, PLUGIN_BASE, PLUGIN_BASE, row); + assert.equal(held.code, 1, name); + assert.ok(held.out.includes(`(${name} manifest)`), `${name}: ${held.out}`); + } + // Renamed away from a manifest path holds too. + const away = await ci([{ filename: "plugins/foo/plugin.json", previous_filename: PLUGIN, status: "renamed" }]); + assert.ok(away.out.includes("(renamed manifest)"), away.out); +}); + +test("ci-status passes a manifest change the merge gate does not review, such as a userConfig default (#9483)", async () => { + for (const [path, before, after] of [ + [PLUGIN, PLUGIN_BASE, PLUGIN_BASE.replace('"default": true', '"default": false')], + [PLUGIN, PLUGIN_BASE, PLUGIN_BASE.replace("1.0.0", "1.1.0")], + [MARKET, MARKET_BASE, MARKET_BASE.replace('"dev"', '"tools"')], + ]) { + const passed = await manifest(path, before, after); + assert.equal(passed.code, 0, passed.out); + assert.match(passed.out, /no body hold, no member veto and no hold path\./); + } + const added = await ci([{ filename: PLUGIN, status: "added" }], { texts: { [`${PLUGIN}@${HEAD}`]: PLUGIN_BASE } }); + assert.equal(added.code, 0, added.out); +}); + +test("a trusted approval of the head lifts a manifest hold", async () => { + const hooked = withKey('"hooks": {}'); + const texts = { [`${PLUGIN}@${BASE}`]: PLUGIN_BASE, [`${PLUGIN}@${HEAD}`]: hooked }; + const lifted = await ci([{ filename: PLUGIN, status: "modified" }], { texts, reviews: [approve] }); + assert.equal(lifted.code, 0, lifted.out); + assert.match(lifted.out, /owner-person approved this head, which lifts its hold path/); +}); + +test("an old files line, with no status, holds a manifest", async () => { + const held = await ci([MARKET]); assert.equal(held.code, 1); - assert.match(held.out, /it changes \.claude-plugin\/marketplace\.json, a hold path/); - assert.equal((await ci([".claude-plugin/marketplace.json"], { reviews: [approve] })).code, 0); + assert.match(held.out, /\.claude-plugin\/marketplace\.json \(unknown manifest\)/); }); test("ci-status holds a cost path that an approval does not lift, and the owner's decision does", async () => { @@ -201,3 +287,13 @@ test("linkHold binds a pass to the issue and link kind it judged", () => { assert.match(linkHold({ linked_issues: [{ number: 5 }, { number: 6 }] }, "Closes #5", closes(5), "o/r"), /link changed/); assert.match(linkHold(record(5), "Closes #5", { totalCount: 11, nodes: [] }, "o/r"), /closing issues could not be read/); }); + +// ci-status writes [filename, previous_filename, status]; an older line has +// no status, which holds a manifest. +test("changeOf reads a files line with and without its status", () => { + assert.deepEqual(changeOf(["a.md", null, "modified"]), { path: "a.md", previous: null, status: "modified" }); + assert.deepEqual(changeOf(["b.md", "a.md", "renamed"]), { path: "b.md", previous: "a.md", status: "renamed" }); + assert.deepEqual(changeOf(["b.md", "a.md"]), { path: "b.md", previous: "a.md", status: null }); + assert.deepEqual(changeOf(["a.md"]), { path: "a.md", previous: null, status: null }); + assert.throws(() => changeOf("a.md"), /does not read/); +}); diff --git a/.github/workflows/pr-require-checks.yml b/.github/workflows/pr-require-checks.yml index 4cc446bcd6e..0635b84fcd4 100644 --- a/.github/workflows/pr-require-checks.yml +++ b/.github/workflows/pr-require-checks.yml @@ -4076,10 +4076,12 @@ jobs: # on the holds that gate honors: a body hold, a member's veto comment, # and a changed file or renamed-from path matching the merge lane's # denied paths (pr-pipeline.yaml merge.diff-check.denied-paths, as main - # has it, plus merge-holds.mjs's HOLD_PATH_FLOOR), so a push after arming is - # held on its own pull request event; a person on the trusted-actor list - # (as main has it) lifts a hold path by approving the head, then re-runs - # ci-status. A changed cost path (merge-holds.mjs COST_PATHS) also holds, + # has it, plus merge-holds.mjs's HOLD_PATH_FLOOR), or a plugin or + # marketplace manifest change the merge gate reviews (merge-holds.mjs + # manifestChanges, both sides read at BASE_SHA and the head), so a push + # after arming is held on its own pull request event; a person on the + # trusted-actor list (as main has it) lifts either by approving the head, + # then re-runs ci-status. A changed cost path (merge-holds.mjs COST_PATHS) also holds, # and only the owner's cost decision linked from the body lifts it; the # module reads that decision with this step's GH_TOKEN, only when the # body links one. All by the rules in @@ -4218,7 +4220,7 @@ jobs: exit 1 } read_api "the files of #$pr" "repos/$GITHUB_REPOSITORY/pulls/$pr/files?per_page=100" --paginate \ - --jq '.[] | [.filename, (.previous_filename // empty)] | @json' >"$RUNNER_TEMP/files.jsonl" || exit 1 + --jq '.[] | [.filename, .previous_filename, .status] | @json' >"$RUNNER_TEMP/files.jsonl" || exit 1 changed=$(jq -er '.changed_files | numbers' <<<"$pull") || { echo "::error::the changed-file count of #$pr could not be read."; exit 1; } if [ "$(grep -c . "$RUNNER_TEMP/files.jsonl")" -lt "$changed" ]; then echo "::error::GitHub listed fewer files than #$pr changes, so its hold paths cannot be checked; a person merges it." @@ -4287,6 +4289,7 @@ jobs: } verdicts=("$RUNNER_TEMP/verdicts.jsonl" "$RUNNER_TEMP/link.json") fi + export BASE_SHA out=$(node "$RUNNER_TEMP/merge-holds.mjs" "$pr" "$RUNNER_TEMP/pr-body.txt" "$RUNNER_TEMP/discussion.jsonl" \ "$RUNNER_TEMP/files.jsonl" "$RUNNER_TEMP/denied-paths.json" "$RUNNER_TEMP/trusted-actors.json" "$head" "${verdicts[@]}") || { printf '%s\n' "$out" diff --git a/docs/conventions/pr-pipeline/README.md b/docs/conventions/pr-pipeline/README.md index 9ecc12b0853..cde6b68f95d 100644 --- a/docs/conventions/pr-pipeline/README.md +++ b/docs/conventions/pr-pipeline/README.md @@ -601,7 +601,7 @@ The schema checks each file's shape. The config reader also rejects: - an activity named `run` or `report`, `pr-run-activity`'s own job names; - any `extends:` value ([Not settled yet](#not-settled-yet)). -The reader also adds the config file's own path, `.github/**` and the trusted-actor list to +The reader also adds the config file's own path and the trusted-actor list to `merge.diff-check.denied-paths`, whatever the config says. ## Concurrency @@ -1124,12 +1124,13 @@ hold paths, so one added after arming stops the merge without waiting for this j `Fail a lanes App pull request its body or a member holds` runs `.github/scripts/pr-merge/merge-holds.mjs` as main has it, the module `enable-auto-merge.mjs` arms by, over the live body, discussion and changed files (renamed-from paths included) against main's -`merge.diff-check.denied-paths` plus the paths resolve-config always adds and the rest of the merge -gate's HIGH floor (`CODEOWNERS`, `.gitmodules`, every `.claude-plugin/plugin.json` and the -root `.claude-plugin/marketplace.json`), on each pull request +`merge.diff-check.denied-paths` plus the paths resolve-config always adds and the merge gate's +HIGH floor (`babysit_risk.HIGH_FLOOR`), and holds a plugin manifest or marketplace manifest +change the gate's manifest review holds, both sides read and compared key by key as the gate +compares them (`manifestChanges`), on each pull request event and in the merge queue, where a red group removes the pull request. A push that adds a hold -path fails `ci-status` on its own head. A person on the trusted-actor list (as main has it) lifts a -hold path by approving that head: their newest review of it that approves, requests changes or was +path or such a manifest change fails `ci-status` on its own head. A person on the trusted-actor list +(as main has it) lifts either by approving that head: their newest review of it that approves, requests changes or was dismissed must approve. Then re-run `ci-status`, since a review does not. The approval lifts no body hold or veto, and arming still skips the pull request, so that person merges it. Both also hold a pull request that changes one of the merge gate's cost paths @@ -1139,8 +1140,9 @@ approval does not lift that hold. Only the owner's cost decision does, read as t line opens `**Decision recorded.** Cost:`, on the pull request or on its one closing issue after the pull request opened, linked from the body. That body edit re-runs `ci-status`. The module is read at the pull request's base commit, so a lanes App pull request whose base predates the module fails `ci-status` closed until it is updated with main. -Its own changes are held: `merge-holds.mjs` and this workflow sit under `.github/**`, a path -resolve-config always denies, so a lanes App pull request cannot weaken the rule and merge itself. A body edit re-runs +A pull request cannot weaken its own check, since the step runs the module as its base has it; a +change to `merge-holds.mjs` merges on the review lanes, and the lanes App's token, which carries no +`workflows` permission, cannot push a change to this workflow. A body edit re-runs `ci-status` at once. A comment does not, so a veto posted while armed takes effect when the queue builds the group, and after a person lifts one with a later comment, `ci-status` needs a re-run. diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_lane_reviews.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_lane_reviews.py index fc9e9dfa8b0..c2c39c70ef5 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_lane_reviews.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_lane_reviews.py @@ -6,12 +6,17 @@ pointer holds; the security lane's open CRITICAL findings hold (`security-finding`); a lane whose evidence is not bound to the head holds; claude-review's open findings are reported only; and a plain documentation -change by the lane merges. A read that fails holds. +change by the lane merges. A read that fails holds. With the HIGH path floor +on, as this repository's pr-pipeline.yaml sets it, a manifest change the +gate reviews holds (`manifest-review-required`, the classes #9280 set) and a +plugin.json userConfig default does not (#9483); native auto-merge has no +manifest hold path of its own, so this is the check that holds it. """ from __future__ import annotations import contextlib +import dataclasses import io import json import pathlib @@ -48,6 +53,21 @@ "100644", patch="@@ -1 +1 @@\n-old\n+new", ) +FLOOR_ON = dataclasses.replace(CONFIG, high_path_floor=True) +PLUGIN = "plugins/foo/.claude-plugin/plugin.json" +PLUGIN_BASE = ( + '{"name": "foo", "version": "1.0.0",' + ' "userConfig": {"guard_enabled": {"type": "boolean", "default": true}}}' +) + + +def manifest(after: str) -> ChangedFile: + return ChangedFile( + PLUGIN, None, "modified", 1, 1, True, "100644", "100644", + patch="@@ -1 +1 @@\n-a\n+b", contents=(PLUGIN_BASE, after), + ) + + SYMLINK = ChangedFile("docs/link", None, "added", 1, 0, True, "120000", "000000") SUBMODULE = ChangedFile("vendor/lib", None, "modified", 1, 1, False, "160000", "160000") @@ -69,7 +89,9 @@ def lanes( def refusal( - files: tuple[ChangedFile, ...] = (DOC,), evidence: object = None + files: tuple[ChangedFile, ...] = (DOC,), + evidence: object = None, + config: policy.PipelineConfig = CONFIG, ) -> str | None: evidence = lanes() if evidence is None else evidence effect = evidence if isinstance(evidence, Exception) else None @@ -86,7 +108,7 @@ def refusal( return_value=None if effect else (evidence, []), ), ): - return ve.lane_risk_refusal(REPO, 41, PR, CONFIG, "C2", 7) + return ve.lane_risk_refusal(REPO, 41, PR, config, "C2", 7) class LaneRiskRefusal(unittest.TestCase): @@ -113,6 +135,18 @@ def test_an_unbound_security_review_holds(self) -> None: str(refusal(evidence=lanes(bound=False))), ) + def test_a_manifest_change_the_gate_reviews_holds(self) -> None: + for key in ('"hooks": {}', '"mcpServers": {}', '"experimental": {"monitors": []}'): + after = PLUGIN_BASE.replace('"name": "foo",', f'"name": "foo", {key},') + with self.subTest(key): + held = refusal((DOC, manifest(after)), config=FLOOR_ON) + self.assertIn("manifest-review-required", str(held)) + self.assertIn(PLUGIN, str(held)) + + def test_a_user_config_default_merges(self) -> None: + after = PLUGIN_BASE.replace('"default": true', '"default": false') + self.assertIsNone(refusal((DOC, manifest(after)), config=FLOOR_ON)) + def test_a_failed_read_holds(self) -> None: held = refusal( evidence=babysit_merge.HeadPusherUnproven("head-pusher-unproven:abc") diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_risk.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_risk.py index 7e29536976a..6009442a9bf 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_risk.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_risk.py @@ -1418,6 +1418,12 @@ def test_a_removed_or_renamed_manifest_holds(self): self.assertIn( self.REVIEW, self._codes(MANIFEST, PLUGIN_BASE, PLUGIN_BASE, **renamed) ) + copied = dict( + status="copied", previous_path="plugins/bar/.claude-plugin/plugin.json" + ) + self.assertIn( + self.REVIEW, self._codes(MANIFEST, PLUGIN_BASE, PLUGIN_BASE, **copied) + ) class HoldsTests(unittest.TestCase): diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_merge_holds_sync.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_merge_holds_sync.py index ef1f78a3ab2..3feadaade36 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_merge_holds_sync.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_merge_holds_sync.py @@ -1,15 +1,18 @@ # test-scope: .github/scripts/pr-merge/merge-holds.mjs """The native auto-merge holds (`.github/scripts/pr-merge/merge-holds.mjs`, -run here under node) cover the merge gate's cost paths and manifests, so a -lane pull request native auto-merge merges cannot skip them. The gate's other -cost reasons (Pulumi project files, workflow and pipeline lines) are not -mirrored; workflows and the repository's pipeline config are hold paths -already. +run here under node) cover the merge gate's HIGH floor, cost paths and +manifest review, so a lane pull request native auto-merge merges cannot skip +them. The gate's other cost reasons (Pulumi project files, workflow and +pipeline lines) are not mirrored: the lanes App's token cannot push a +workflow file, and the repository's pipeline config is a hold path. +- The floor: every `babysit_risk.HIGH_FLOOR` glob is a hold path glob + (`HOLD_PATH_FLOOR`). - The cost paths: the same globs as `babysit_risk.COST_PATHS`, and the same paths matched (`glob_match` folds case and covers what sits under a glob). -- The manifests: every path `babysit_risk.is_manifest_path` takes is a hold - path (`pathHold`). +- The manifests: `manifestChanges` finds what `babysit_risk._manifest_changes` + finds for each change, so ci-status holds what the gate's manifest review + holds and passes what it passes. - The cost decision: `costDecision` accepts the same comments as `babysit_merge_policy.cost_decision`, with the owner a trusted person in place of an admin. @@ -36,14 +39,18 @@ / ".github/scripts/pr-merge/merge-holds.mjs" ) NODE = """ -import { COST_PATHS, costHold, pathHold } from %s; +import { COST_PATHS, HOLD_PATH_FLOOR, costHold } from %s; const paths = JSON.parse(process.argv[1]); console.log(JSON.stringify({ costPaths: COST_PATHS, + floor: HOLD_PATH_FLOOR, cost: paths.map((path) => costHold([path], null) !== null), - held: paths.map((path) => pathHold([path], []) !== null), })); """ +NODE_MANIFEST = """ +import { manifestChanges } from %s; +console.log(JSON.stringify(JSON.parse(process.argv[1]).map(manifestChanges))); +""" PATHS = [ "plugins/multi-agent/reference/defaults.yaml", "Plugins/Multi-Agent/Reference/Defaults.YAML", @@ -67,9 +74,6 @@ "deploy/main.bicepparam", "deploy/main.bicep.md", "docs/a.md", - ".claude-plugin/marketplace.json", - "plugins/foo/.claude-plugin/plugin.json", - "plugins/foo/.claude-plugin/other.json", ] @@ -97,20 +101,139 @@ def test_the_cost_paths_are_the_same(self) -> None: self.assertIn(True, answer["cost"]) self.assertIn(False, answer["cost"]) - def test_every_manifest_the_gate_reads_is_a_hold_path(self) -> None: - answer = module(PATHS) - manifests = [p for p in PATHS if risk.is_manifest_path(p)] - self.assertEqual( - manifests, - [ - ".claude-plugin/marketplace.json", - "plugins/foo/.claude-plugin/plugin.json", - ], - ) - for path, held in zip(PATHS, answer["held"], strict=True): - if path in manifests: - with self.subTest(path): - self.assertTrue(held) + def test_the_floor_covers_the_gate_s(self) -> None: + floor = module([])["floor"] + self.assertTrue(set(risk.HIGH_FLOOR) <= set(floor), floor) + + def test_the_manifest_review_is_the_gate_s(self) -> None: + js = manifest_changes([change for _, change, _ in MANIFEST_CASES]) + for (name, change, holds), found in zip(MANIFEST_CASES, js, strict=True): + with self.subTest(name): + self.assertEqual(found, py_manifest_changes(change)) + self.assertEqual(bool(found), holds) + + +PLUGIN = "plugins/foo/.claude-plugin/plugin.json" +MARKET = ".claude-plugin/marketplace.json" +PLUGIN_BASE = ( + '{"name": "foo", "version": "1.0.0",' + ' "userConfig": {"guard_enabled": {"type": "boolean", "default": true}}}' +) +MARKET_BASE = ( + '{"name": "m", "plugins": [{"name": "foo", "source": "./plugins/foo",' + ' "category": "dev"}]}' +) + + +def _key(text: str) -> str: + return PLUGIN_BASE.replace('"name": "foo",', f'"name": "foo", {text},') + + +def _case( + name: str, + after: str | None, + holds: bool, + *, + path: str = PLUGIN, + before: str | None = PLUGIN_BASE, + status: str = "modified", + previous: str | None = None, +) -> tuple[str, dict[str, Any], bool]: + return ( + name, + {"path": path, "previous": previous, "status": status, "contents": [before, after]}, + holds, + ) + + +# The classes #9280 set the gate's manifest review to hold, then changes it +# lets merge: a userConfig default (#9483), a version, a description, a +# marketplace category. +MANIFEST_CASES = [ + _case("hooks", _key('"hooks": "./h.json"'), True), + _case("mcpServers", _key('"mcpServers": {}'), True), + _case("lspServers", _key('"lspServers": {}'), True), + _case("monitors", _key('"monitors": []'), True), + _case("experimental.monitors", _key('"experimental": {"monitors": []}'), True), + _case("experimental.monitors removed", PLUGIN_BASE, True, before=_key('"experimental": {"monitors": []}')), + _case("hooks reordered", _key('"hooks": {"b": 1, "a": [1, 2]}'), False, before=_key('"hooks": {"a": [1, 2], "b": 1}')), + _case("hooks 1 to 1.0", _key('"hooks": {"t": 1.0}'), True, before=_key('"hooks": {"t": 1}')), + _case("hooks false to 0", _key('"hooks": 0'), True, before=_key('"hooks": false')), + _case( + "a plugin added", + MARKET_BASE.replace("]}", ', {"name": "bar", "source": "./plugins/bar"}]}'), + True, + path=MARKET, + before=MARKET_BASE, + ), + _case("a plugin removed", '{"name": "m", "plugins": []}', True, path=MARKET, before=MARKET_BASE), + _case("a plugin renamed", MARKET_BASE.replace('"name": "foo"', '"name": "fob"'), True, path=MARKET, before=MARKET_BASE), + _case( + "a plugin source changed", + MARKET_BASE.replace("./plugins/foo", "https://example.com/foo.git"), + True, + path=MARKET, + before=MARKET_BASE, + ), + _case("the marketplace name changed", MARKET_BASE.replace('"name": "m"', '"name": "n"'), True, path=MARKET, before=MARKET_BASE), + _case("plugins not a list", '{"name": "m", "plugins": {}}', True, path=MARKET, before=MARKET_BASE), + _case( + "two entries of one name", + MARKET_BASE.replace("]}", ', {"name": "foo", "source": "./plugins/foo"}]}'), + True, + path=MARKET, + before=MARKET_BASE, + ), + _case("removed", None, True, status="removed"), + _case("renamed", PLUGIN_BASE, True, status="renamed", previous="plugins/foo/plugin.json"), + _case("copied", PLUGIN_BASE, True, status="copied", previous="plugins/bar/.claude-plugin/plugin.json"), + _case("an unreadable head", None, True), + _case("a parse error", "{", True), + _case("not an object", "[]", True), + _case("a duplicate key", _key('"name": "x"'), True), + _case("a nested duplicate key", _key('"hooks": {"a": 1, "a": 2}'), True), + _case("an added plugin.json", PLUGIN_BASE, False, before=None, status="added"), + _case("an added plugin.json with hooks", _key('"hooks": {}'), True, before=None, status="added"), + _case("a userConfig default (#9483)", PLUGIN_BASE.replace('"default": true', '"default": false'), False), + _case("a version", PLUGIN_BASE.replace("1.0.0", "1.1.0"), False), + _case("a marketplace category", MARKET_BASE.replace('"dev"', '"tools"'), False, path=MARKET, before=MARKET_BASE), + _case( + "a marketplace metadata key", + MARKET_BASE.replace('"name": "m",', '"name": "m", "metadata": {"pluginRoot": "x"},'), + False, + path=MARKET, + before=MARKET_BASE, + ), +] + + +def manifest_changes(changes: list[dict[str, Any]]) -> list[list[str]]: + script = NODE_MANIFEST % json.dumps(MODULE.as_uri()) + proc = subprocess.run( + ["node", "--input-type=module", "-e", script, json.dumps(changes)], + capture_output=True, + text=True, + check=True, + ) + return json.loads(proc.stdout) + + +def py_manifest_changes(change: dict[str, Any]) -> list[str]: + status = change["status"] + two_sided = status in ("modified", "changed", "renamed", "copied") + before, after = change["contents"] + changed = risk.ChangedFile( + path=change["path"], + previous_path=change["previous"], + status=status, + additions=1, + deletions=1, + has_patch=True, + mode="100644", + previous_mode="100644" if two_sided else None, + contents=(None if status == "added" else before, after), + ) + return risk._manifest_changes(risk._check_file(changed), changed) REPO = "o/r" diff --git a/scripts/ci-lanes-pr-hold.test.sh b/scripts/ci-lanes-pr-hold.test.sh index a7e574bec34..6945c2cea81 100755 --- a/scripts/ci-lanes-pr-hold.test.sh +++ b/scripts/ci-lanes-pr-hold.test.sh @@ -90,8 +90,10 @@ fi # then prints each row of STUB_ISSUE, STUB_REVIEWS or STUB_INLINE (JSON # arrays) as one line, as `--jq '.[] | @json'` does. A contents read of # pr-pipeline.yaml prints STUB_CONFIG, or answers 404 when STUB_NO_CONFIG is -# set. A files read prints each row of STUB_FILES (a JSON array of -# [path, renamed-from?] arrays) as one line. A contents read of +# set. A files read answers STUB_FILES (a JSON array of [path, +# renamed-from or null, status] arrays, status renamed with a renamed-from +# path and modified otherwise) as the files API's rows, through the step's +# own --jq. A contents read of # trusted-actors.json prints the working tree's list, or answers 404 when # STUB_NO_ACTORS is set. An artifacts read by name prints the run ids # STUB_ARTIFACTS (a JSON object of name to run ids) gives that name, through @@ -137,7 +139,9 @@ sys.stdout.buffer.write(buffer.getvalue())' "$record" [[ -z "${STUB_NO_CONFIG:-}" ]] || { echo 'gh: Not Found (HTTP 404)' >&2; exit 1; } printf '%s\n' "$STUB_CONFIG" ;; -*/pulls/*/files\?*) rows "$STUB_FILES" ;; +*/pulls/*/files\?*) + jq -c '[.[] | {filename: .[0], previous_filename: .[1], status: (.[2] // (if .[1] then "renamed" else "modified" end))}]' <<<"$STUB_FILES" | jq -r "${*: -1}" + ;; */contents/.github/standards/trusted-actors/trusted-actors.json*) [[ -z "${STUB_NO_ACTORS:-}" ]] || { echo 'gh: Not Found (HTTP 404)' >&2; exit 1; } cat "$STUB_ACTORS" @@ -188,7 +192,7 @@ expect() { : >"$TMP_ROOT/log" out="$(PATH="$TMP_ROOT/bin:$PATH" STUB_LOG="$TMP_ROOT/log" STUB_MODULE="${STUB_MODULE:-$MODULE}" \ GITHUB_EVENT_NAME="${EVENT:-pull_request}" EVENT_ACTION="${ACTION-synchronize}" GITHUB_REF="${REF:-refs/pull/7/merge}" PR_NUMBER="${PR_NUMBER-7}" \ - GITHUB_REPOSITORY=o/r RUNNER_TEMP="$TMP_ROOT" GROUP_BASE_SHA="${GROUP_BASE_SHA-}" \ + GITHUB_REPOSITORY=o/r GITHUB_API_URL=http://127.0.0.1:9 RUNNER_TEMP="$TMP_ROOT" GROUP_BASE_SHA="${GROUP_BASE_SHA-}" \ LANES_DISABLED="${KILL-false}" LANES_REPO_DISABLED="${REPO_KILL-false}" bash -e "$TMP_ROOT/body.sh" 2>&1)" && rc=0 || rc=$? calls=$(grep -cE '/(comments|reviews)\?' "$TMP_ROOT/log") if [[ "$rc" -ne "$want_rc" ]]; then @@ -279,24 +283,42 @@ STUB_ISSUE='' STUB_REVIEWS='' STUB_INLINE='' STUB_PULL="$(pull 'Closes #1')" STUB_FILES='[["docs/held/a.md"]]' expect "a file under the config's denied paths fails" 1 3 "::error::#7: it changes docs/held/a.md, a hold path; an approving review of this head by a person on the trusted-actor list lifts it" -STUB_FILES='[["docs/guide.md"],["docs/moved.md",".github/workflows/x.yml"]]' STUB_PULL="$(STUB_CHANGED=2 pull 'Closes #1')" -expect "a rename away from a floor path fails on its old path" 1 3 "it changes .github/workflows/x.yml, a hold path" -STUB_FILES='[[".github/x\n::stop-commands::t"]]' STUB_PULL="$(pull 'Closes #1')" -expect "a newline in a held file name cannot start a workflow command" 1 3 "it changes .github/x%0A::stop-commands::t, a hold path" -STUB_FILES='[["docs/guide.md"],[".github/workflows/.x.yml"]]' STUB_PULL="$(STUB_CHANGED=2 pull 'Closes #1')" -expect "a dot-prefixed file under a floor tree fails" 1 3 "it changes .github/workflows/.x.yml, a hold path" -for held in CODEOWNERS docs/CODEOWNERS .gitmodules plugins/p/.claude-plugin/plugin.json; do +STUB_FILES='[["docs/guide.md"],["docs/moved.md",".github/actions/resolve-config/x.md"]]' STUB_PULL="$(STUB_CHANGED=2 pull 'Closes #1')" +expect "a rename away from a floor path fails on its old path" 1 3 "it changes .github/actions/resolve-config/x.md, a hold path" +STUB_FILES='[["docs/held/x\n::stop-commands::t"]]' STUB_PULL="$(pull 'Closes #1')" +expect "a newline in a held file name cannot start a workflow command" 1 3 "it changes docs/held/x%0A::stop-commands::t, a hold path" +STUB_FILES='[["docs/guide.md"],[".github/actions/check-trusted-trigger/.x.mjs"]]' STUB_PULL="$(STUB_CHANGED=2 pull 'Closes #1')" +expect "a dot-prefixed file under a floor tree fails" 1 3 "it changes .github/actions/check-trusted-trigger/.x.mjs, a hold path" +for held in CODEOWNERS docs/CODEOWNERS .gitmodules .github/standards/trusted-actors/README.md .github/actions/resolve-config/resolve.mjs; do STUB_FILES="[[\"$held\"]]" STUB_PULL="$(pull 'Closes #1')" expect "the merge gate's HIGH floor holds $held" 1 3 "it changes $held, a hold path" done +# The rest of .github merges on the review lanes (owner direction +# 2026-10-23); the lanes App's token cannot push a workflow file. +for path in .github/workflows/x.yml .github/scripts/pr-merge/merge-holds.mjs .github/actions/other/action.yml; do + STUB_FILES="[[\"$path\"]]" STUB_PULL="$(pull 'Closes #1')" + expect "$path is no hold path" 0 3 "no hold path" +done +# A manifest is compared base against head, read from the contents API, +# which this test does not answer, so a manifest change holds as unreadable; +# test_merge_holds_sync.py checks each comparison against the gate's. +STUB_FILES='[["plugins/p/.claude-plugin/plugin.json"]]' STUB_PULL="$(pull 'Closes #1')" +expect "a manifest whose sides do not read holds" 1 3 "it changes plugins/p/.claude-plugin/plugin.json (contents unreadable), a manifest change that needs review" +STUB_FILES='[[".claude-plugin/marketplace.json",null,"removed"]]' STUB_PULL="$(pull 'Closes #1')" +expect "a removed manifest holds" 1 3 "it changes .claude-plugin/marketplace.json (removed manifest)" +if grep -qF "[.filename, .previous_filename, .status]" <<<"$step" && grep -qxF " export BASE_SHA" <<<"$step"; then + ok "the step hands merge-holds.mjs each file's status and the base commit" +else + fail "the step does not hand merge-holds.mjs each file's status and the base commit" +fi STUB_FILES='[["docs/conventions/pr-pipeline.yaml"]]' STUB_PULL="$(pull 'Closes #1')" expect "the pipeline config is held by the floor" 1 3 "it changes docs/conventions/pr-pipeline.yaml, a hold path" -STUB_FILES='[[".github/workflows/x.yml"]]' +STUB_FILES='[["docs/held/x.md"]]' KYLE_OK="$(approval kyle-sexton 153232337 APPROVED 2026-10-23T11:00:00Z "$SHA")" STUB_REVIEWS="[$KYLE_OK]" expect "a trusted person's approval of the head lifts a hold path" 0 3 "kyle-sexton approved this head, which lifts its hold path" STUB_REVIEWS="[$(approval kyle-sexton 153232337 APPROVED 2026-10-23T11:00:00Z "$BASE")]" -expect "an approval of an earlier head does not lift it" 1 3 "it changes .github/workflows/x.yml, a hold path" +expect "an approval of an earlier head does not lift it" 1 3 "it changes docs/held/x.md, a hold path" STUB_REVIEWS="[$KYLE_OK,$(approval kyle-sexton 153232337 CHANGES_REQUESTED 2026-10-23T12:00:00Z "$SHA")]" expect "a later request for changes by the same person takes the lift back" 1 3 "a hold path" STUB_REVIEWS="[$KYLE_OK,$(approval kyle-sexton 153232337 COMMENTED 2026-10-23T12:00:00Z "$SHA")]" @@ -476,8 +498,8 @@ STUB_PULL="$(pull 'Closes #1')" STUB_ISSUE="[$KYLE_VETO]" expect "in the merge queue, a member's veto fails the group" 1 3 "kyle asked not to merge it" STUB_ISSUE="[$KYLE_VETO,$(comment kyle User MEMBER 2026-10-23T11:00:00Z 'ok, merge it')]" expect "in the merge queue, a lifted veto passes" 0 3 -STUB_ISSUE='' STUB_FILES='[[".github/workflows/x.yml"]]' -expect "in the merge queue, a hold path fails the group" 1 3 "it changes .github/workflows/x.yml, a hold path" +STUB_ISSUE='' STUB_FILES='[["docs/held/x.md"]]' +expect "in the merge queue, a hold path fails the group" 1 3 "it changes docs/held/x.md, a hold path" STUB_FILES='[["docs/guide.md"]]' export REF="refs/heads/main" expect "a merge-group ref with no pull request fails closed without a call" 1 0 "::error::cannot read a pull request number" From 5fb7023524910e8703d46a1d0f12c64a59711890 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Fri, 23 Oct 2026 22:06:33 -0400 Subject: [PATCH 2/3] fix(pr-pipeline): hold the floor's own imports and case variants Review fixes for the hold-path floor cut: - matchesHoldPath folds case, as glob_match and costHold do, so .github/codeowners or .github/actions/Resolve-Config/ holds. - The HIGH floor and HOLD_PATH_FLOOR hold .github/actions/check-kill-switch/**: check-trusted-trigger and resolve-config write their verdicts through its writeOutputs. A test walks both actions' imports and fails on any file the floor does not hold, and the floor test compares matched paths, case variants included, instead of glob strings. - Manifest contents decode as strict UTF-8 with the BOM kept, and an integer past int_max_str_digits holds, as the gate's json.loads refuses it. - A path hold and a manifest hold both reach the reason. - New manifest cases: hooks true to null and to false, a 20-digit integer, -0.0 to 0.0. - The workflows premise names what it covers: lanes App pushes only. Co-Authored-By: Claude Opus 5.5 --- .../agent-ac72c39ea4f0fbf44-4968e884.md | 2 +- .../agent-ac72c39ea4f0fbf44-57679074.md | 6 +- .../pr-merge/enable-auto-merge.test.mjs | 2 +- .github/scripts/pr-merge/merge-holds.mjs | 28 ++++-- docs/conventions/pr-pipeline/README.md | 4 +- .../babysit-prs/scripts/babysit_risk.py | 4 +- .../scripts/tests/test_merge_holds_sync.py | 89 +++++++++++++++++-- scripts/ci-lanes-pr-hold.test.sh | 4 +- 8 files changed, 112 insertions(+), 27 deletions(-) diff --git a/.changes/pr-pipeline/agent-ac72c39ea4f0fbf44-4968e884.md b/.changes/pr-pipeline/agent-ac72c39ea4f0fbf44-4968e884.md index 515b0888c28..28b935e1f22 100644 --- a/.changes/pr-pipeline/agent-ac72c39ea4f0fbf44-4968e884.md +++ b/.changes/pr-pipeline/agent-ac72c39ea4f0fbf44-4968e884.md @@ -4,5 +4,5 @@ bump: minor ### Changed -- resolve-config no longer adds `.github/**` to `merge.diff-check.denied-paths`; it still adds the config file's own path and the trusted-actor list. A lanes App pull request touching `.github/scripts`, `.github/actions` or another `.github` path off the merge gate's HIGH floor is no longer held by report-outcome's `Hold:` line, `enable-auto-merge` or `ci-status`, and merges on the two review lanes (owner direction 2026-10-23). The lanes App's token carries no `workflows` permission, so a lane cannot push a workflow change. +- resolve-config no longer adds `.github/**` to `merge.diff-check.denied-paths`; it still adds the config file's own path and the trusted-actor list. A lanes App pull request touching `.github/scripts`, `.github/actions` or another `.github` path off the merge gate's HIGH floor is no longer held by report-outcome's `Hold:` line, `enable-auto-merge` or `ci-status`, and merges on the two review lanes (owner direction 2026-10-23). The lanes App's token never carries the `workflows` permission, so no lanes App push changes a workflow file; another identity with write access can still push one after auto-merge is armed. Hold paths now match case-insensitively, as the merge gate's `glob_match` does, so `.github/codeowners` or `.github/actions/Resolve-Config/` holds. The floor also holds `.github/actions/check-kill-switch/**`, whose output writer check-trusted-trigger and resolve-config load; a test walks both actions' imports and fails on any file the floor does not hold. - `merge-holds.mjs` no longer holds every `plugin.json` and `marketplace.json` path-wide. Its hold-path floor is the forced paths plus the merge gate's HIGH floor (`babysit_risk.HIGH_FLOOR`), and `ci-status` compares a changed manifest base against head as the gate's manifest review does (`manifestChanges`, the rule of `babysit_risk._manifest_changes`): an executable `plugin.json` key or `experimental.monitors`, a marketplace plugin added, removed, renamed or re-sourced or a marketplace `name` change, a removed, renamed or copied manifest, and a side that does not read or parse or repeats a key hold; a `userConfig` default or version change merges. `enable-auto-merge` arms such a change once the gate's risk score (`evidence --lane-reviews`) allows it. ci-status now hands the module each file's status and the base commit. diff --git a/.changes/source-control/agent-ac72c39ea4f0fbf44-57679074.md b/.changes/source-control/agent-ac72c39ea4f0fbf44-57679074.md index 7a3bbe165ed..78420ef49d6 100644 --- a/.changes/source-control/agent-ac72c39ea4f0fbf44-57679074.md +++ b/.changes/source-control/agent-ac72c39ea4f0fbf44-57679074.md @@ -1,5 +1,7 @@ --- -bump: none +bump: patch --- -Only tests of the babysit-prs scripts change; no shipped behavior changes. +### Changed + +- The HIGH path floor (`babysit_risk.HIGH_FLOOR`) also holds `.github/actions/check-kill-switch/**`: check-trusted-trigger and resolve-config write their verdicts through its `writeOutputs`, so a change there could rewrite the trust check's result. diff --git a/.github/scripts/pr-merge/enable-auto-merge.test.mjs b/.github/scripts/pr-merge/enable-auto-merge.test.mjs index c7d7b656b64..d787a38f84c 100644 --- a/.github/scripts/pr-merge/enable-auto-merge.test.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.test.mjs @@ -384,7 +384,7 @@ test("a pull request whose head now changes a hold path is not armed, or is disa pull(3, { files: Array.from({ length: 301 }, (_, index) => ({ filename: `docs/${index}.md` })) }), pull(4, { files: [{ filename: "docs/held.md" }] }), pull(5), - // The rest of .github merges on the review lanes (owner direction 2026-10-23). + // The rest of .github merges on the review lanes. pull(6, { files: [{ filename: ".github/workflows/x.yml" }, { filename: ".github/scripts/pr-merge/merge-holds.mjs" }] }), ]); assert.deepEqual(writes, [ diff --git a/.github/scripts/pr-merge/merge-holds.mjs b/.github/scripts/pr-merge/merge-holds.mjs index 7af97debe8c..93e2576f05f 100755 --- a/.github/scripts/pr-merge/merge-holds.mjs +++ b/.github/scripts/pr-merge/merge-holds.mjs @@ -156,6 +156,7 @@ export const HOLD_PATH_FLOOR = Object.freeze([ "**/CODEOWNERS", "**/.gitmodules", ".github/actions/resolve-config/**", + ".github/actions/check-kill-switch/**", ]); // babysit_risk.is_manifest_path: a plugin manifest or the marketplace @@ -226,7 +227,12 @@ function parseManifest(text) { const token = /^(?:true|false|null|-?\d+(?:\.\d+)?(?:[eE][+-]?\d+)?)/.exec(text.slice(at))[0]; at += token.length; if (!/^-?\d/.test(token)) return { text: token }; - if (!/[.eE]/.test(token)) return { text: `${BigInt(token)}` }; + if (!/[.eE]/.test(token)) { + // json.loads refuses an integer past int_max_str_digits (4300), so the + // gate reads that side as malformed. + if (token.replace("-", "").length > 4300) throw new Error("integer too long"); + return { text: `${BigInt(token)}` }; + } const number = Number(token); return { text: `${Object.is(number, -0) ? "-0" : String(number)}.f` }; }; @@ -373,10 +379,12 @@ export const fromRest = (row) => ({ body: String(row?.body ?? ""), }); -// Whether `path` matches the hold-path `glob`. path.matchesGlob has no `dot` -// option, so `*` and `**` skip a segment opening with "."; both sides mark -// such a segment alike first, so `.github/**` holds `.github/a/.x` too. -const undot = (text) => String(text).replaceAll(/(^|\/)\./g, "$1\u0001"); +// Whether `path` matches the hold-path `glob`, case-folded as glob_match +// folds it: a lane host on a case-insensitive filesystem reads `.github/codeowners` +// as CODEOWNERS. path.matchesGlob has no `dot` option, so `*` and `**` skip a +// segment opening with "."; both sides mark such a segment alike first, so +// `.github/**` holds `.github/a/.x` too. +const undot = (text) => String(text).toLowerCase().replaceAll(/(^|\/)\./g, "$1\u0001"); export const matchesHoldPath = (path, glob) => posix.matchesGlob(undot(path), undot(glob)); // Why the head's changed `files` hold it, or null. Null `holdPaths` or @@ -522,7 +530,7 @@ export function pathLifters(items, humans, head) { // owner's `decision` lifts. export function holds(body, items, files, holdPaths, lifters = [], decision = null, manifest = null) { const reasons = []; - const pathReason = pathHold(files, holdPaths) ?? manifest; + const pathReason = [pathHold(files, holdPaths), manifest].filter(Boolean).join("; ") || null; const readable = Array.isArray(files) && Array.isArray(holdPaths); if (pathReason && !(readable && lifters.length)) { reasons.push( @@ -628,7 +636,9 @@ export async function withContents(github, repo, changes, base, head) { if (file?.type !== "file" || file.encoding !== "base64" || typeof file.content !== "string") { throw new Error(`${path} at ${ref} did not read`); } - return Buffer.from(file.content, "base64").toString("utf8"); + // Strict, as the gate's decode("utf-8"): invalid UTF-8 throws, so it + // holds, and a BOM stays, so JSON.parse refuses it as json.loads does. + return new TextDecoder("utf-8", { fatal: true, ignoreBOM: true }).decode(Buffer.from(file.content, "base64")); }; return Promise.all( changes.map(async (change) => { @@ -680,9 +690,9 @@ export async function main(argv = process.argv.slice(2), { env = process.env, gi } const manifest = manifestHold(await withContents(github, repo, changes, env.BASE_SHA, head)); const reasons = holds(body, items, files, holdPaths, lifters, decision, manifest); - const held = pathHold(files, holdPaths) ?? manifest; + const held = [pathHold(files, holdPaths), manifest].filter(Boolean).join("; ") || null; if (held && lifters.length && Array.isArray(holdPaths)) { - console.log(`#${number}: ${lifters.join(", ")} approved this head, which lifts its hold path (${escape(held)}).`); + console.log(`#${number}: ${lifters.join(", ")} approved this head, which lifts its hold path or manifest hold (${escape(held)}).`); } if (verdictsFile) { const unverified = verdictHold(lines(verdictsFile).map((line) => JSON.parse(line)), env.GITHUB_REPOSITORY); diff --git a/docs/conventions/pr-pipeline/README.md b/docs/conventions/pr-pipeline/README.md index a19dbeaf030..63013c886c1 100644 --- a/docs/conventions/pr-pipeline/README.md +++ b/docs/conventions/pr-pipeline/README.md @@ -753,9 +753,9 @@ and its queued entries leave; two different identities, or an unreadable head co - **HIGH path floor.** A fixed set of paths in the wrapper's code, joined with `merge.diff-check.denied-paths` and matched case-insensitively. It holds only the takeover set (owner direction 2026-10-23): `.github/standards/trusted-actors/**`, - `.github/actions/check-trusted-trigger/**`, `CODEOWNERS`, `.gitmodules`, and + `.github/actions/check-trusted-trigger/**`, `CODEOWNERS`, `.gitmodules`, `.github/actions/resolve-config/**`, whose code the token broker runs from main in its signer - process. Agent permission config, package manifests and workflows merge on the review lanes. A + process, and `.github/actions/check-kill-switch/**`, whose output writer both actions use. Agent permission config, package manifests and workflows merge on the review lanes. A diff that touches a floor path scores HIGH and is never merged unattended. A `plugins//.claude-plugin/plugin.json` or the top-level `.claude-plugin/marketplace.json` change scores HIGH as `manifest-review-required` when it diff --git a/plugins/source-control/skills/babysit-prs/scripts/babysit_risk.py b/plugins/source-control/skills/babysit-prs/scripts/babysit_risk.py index 30ef1d49f92..8e93b07ac06 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_risk.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_risk.py @@ -204,13 +204,15 @@ class RiskResult: # (the trusted-actor list and the check that enforces it), who owns review # (code owners), what code is pulled in from elsewhere (submodules), and the # code the token broker runs from main in its signer process (resolve-config, -# owner decision Q51). +# owner decision Q51), with check-kill-switch, whose writeOutputs both of those +# actions write their verdicts through. HIGH_FLOOR: tuple[str, ...] = ( ".github/standards/trusted-actors/**", ".github/actions/check-trusted-trigger/**", "**/CODEOWNERS", "**/.gitmodules", ".github/actions/resolve-config/**", + ".github/actions/check-kill-switch/**", ) # plugin.json keys whose values Claude Code runs or reads as run config. diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_merge_holds_sync.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_merge_holds_sync.py index 3feadaade36..8d664f9a633 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_merge_holds_sync.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_merge_holds_sync.py @@ -3,11 +3,16 @@ run here under node) cover the merge gate's HIGH floor, cost paths and manifest review, so a lane pull request native auto-merge merges cannot skip them. The gate's other cost reasons (Pulumi project files, workflow and -pipeline lines) are not mirrored: the lanes App's token cannot push a -workflow file, and the repository's pipeline config is a hold path. +pipeline lines) are not mirrored: the lanes App's token never carries the +`workflows` permission (request-lane-token checks the grant is contents, +pull-requests, issues and at most actions), so no lanes App push changes a +workflow file. That covers lanes App pushes only: another identity with +write access can still push a workflow change after auto-merge is armed. +The repository's pipeline config is a hold path. - The floor: every `babysit_risk.HIGH_FLOOR` glob is a hold path glob - (`HOLD_PATH_FLOOR`). + (`HOLD_PATH_FLOOR`), and `pathHold` holds the paths `glob_match` matches + over that floor, case variants included. - The cost paths: the same globs as `babysit_risk.COST_PATHS`, and the same paths matched (`glob_match` folds case and covers what sits under a glob). - The manifests: `manifestChanges` finds what `babysit_risk._manifest_changes` @@ -22,6 +27,7 @@ import json import pathlib +import re import shutil import subprocess import sys @@ -34,17 +40,37 @@ import babysit_merge_policy as policy import babysit_risk as risk -MODULE = ( - pathlib.Path(__file__).resolve().parents[6] - / ".github/scripts/pr-merge/merge-holds.mjs" +ROOT = pathlib.Path(__file__).resolve().parents[6] +MODULE = ROOT / ".github/scripts/pr-merge/merge-holds.mjs" +# The floor actions' entry points: every module they load is code main runs +# with their verdict, so the floor holds it too. +FLOOR_ENTRIES = ( + ".github/actions/check-trusted-trigger/check-trusted-trigger.mjs", + ".github/actions/resolve-config/resolve-config.mjs", ) +IMPORT = re.compile(r"""(?:from|import)\s*\(?\s*["'](\.{1,2}/[^"']+)["']""") + + +def import_closure(entries: tuple[str, ...]) -> list[str]: + seen: set[str] = set() + todo = [ROOT / entry for entry in entries] + while todo: + file = todo.pop().resolve() + name = file.relative_to(ROOT).as_posix() + if name in seen: + continue + seen.add(name) + for spec in IMPORT.findall(file.read_text(encoding="utf-8")): + todo.append(file.parent / spec) + return sorted(seen) NODE = """ -import { COST_PATHS, HOLD_PATH_FLOOR, costHold } from %s; +import { COST_PATHS, HOLD_PATH_FLOOR, costHold, pathHold } from %s; const paths = JSON.parse(process.argv[1]); console.log(JSON.stringify({ costPaths: COST_PATHS, floor: HOLD_PATH_FLOOR, cost: paths.map((path) => costHold([path], null) !== null), + held: paths.map((path) => pathHold([path], []) !== null), })); """ NODE_MANIFEST = """ @@ -75,6 +101,25 @@ "deploy/main.bicep.md", "docs/a.md", ] +FLOOR_PATHS = [ + ".github/actions/resolve-config/resolve.mjs", + ".github/actions/Resolve-Config/resolve.mjs", + ".github/CODEOWNERS", + ".github/codeowners", + "CODEOWNERS", + "docs/CodeOwners", + ".gitmodules", + "vendor/.GitModules", + ".github/standards/trusted-actors/trusted-actors.json", + ".github/Standards/trusted-actors/trusted-actors.json", + ".github/actions/check-trusted-trigger/action.yml", + ".github/actions/Check-Trusted-Trigger/action.yml", + "Docs/Conventions/PR-Pipeline.yaml", + ".github/actions/other/action.yml", + ".github/workflows/x.yml", + "docs/CODEOWNERS.md", + "plugins/foo/.claude-plugin/plugin.json", +] def module(paths: list[str]) -> dict[str, Any]: @@ -102,8 +147,24 @@ def test_the_cost_paths_are_the_same(self) -> None: self.assertIn(False, answer["cost"]) def test_the_floor_covers_the_gate_s(self) -> None: - floor = module([])["floor"] - self.assertTrue(set(risk.HIGH_FLOOR) <= set(floor), floor) + answer = module(FLOOR_PATHS) + self.assertTrue(set(risk.HIGH_FLOOR) <= set(answer["floor"]), answer["floor"]) + for path, held in zip(FLOOR_PATHS, answer["held"], strict=True): + with self.subTest(path): + self.assertEqual( + held, any(risk.glob_match(g, path) for g in answer["floor"]) + ) + self.assertIn(True, answer["held"]) + self.assertIn(False, answer["held"]) + + def test_the_floor_holds_what_its_actions_load(self) -> None: + files = import_closure(FLOOR_ENTRIES) + self.assertIn(".github/actions/check-kill-switch/check-kill-switch.mjs", files) + answer = module(files) + for path, held in zip(files, answer["held"], strict=True): + with self.subTest(path): + self.assertTrue(held) + self.assertTrue(any(risk.glob_match(g, path) for g in risk.HIGH_FLOOR)) def test_the_manifest_review_is_the_gate_s(self) -> None: js = manifest_changes([change for _, change, _ in MANIFEST_CASES]) @@ -158,6 +219,16 @@ def _case( _case("experimental.monitors removed", PLUGIN_BASE, True, before=_key('"experimental": {"monitors": []}')), _case("hooks reordered", _key('"hooks": {"b": 1, "a": [1, 2]}'), False, before=_key('"hooks": {"a": [1, 2], "b": 1}')), _case("hooks 1 to 1.0", _key('"hooks": {"t": 1.0}'), True, before=_key('"hooks": {"t": 1}')), + _case("an integer past int_max_str_digits", _key('"size": ' + "1" * 4301), True), + _case("hooks true to null", _key('"hooks": null'), True, before=_key('"hooks": true')), + _case("hooks true to false", _key('"hooks": false'), True, before=_key('"hooks": true')), + _case( + "hooks 20-digit integer, last digit", + _key('"hooks": {"t": 12345678901234567891}'), + True, + before=_key('"hooks": {"t": 12345678901234567890}'), + ), + _case("hooks -0.0 to 0.0", _key('"hooks": {"t": 0.0}'), True, before=_key('"hooks": {"t": -0.0}')), _case("hooks false to 0", _key('"hooks": 0'), True, before=_key('"hooks": false')), _case( "a plugin added", diff --git a/scripts/ci-lanes-pr-hold.test.sh b/scripts/ci-lanes-pr-hold.test.sh index 6945c2cea81..8fefe8312a7 100755 --- a/scripts/ci-lanes-pr-hold.test.sh +++ b/scripts/ci-lanes-pr-hold.test.sh @@ -293,8 +293,8 @@ for held in CODEOWNERS docs/CODEOWNERS .gitmodules .github/standards/trusted-act STUB_FILES="[[\"$held\"]]" STUB_PULL="$(pull 'Closes #1')" expect "the merge gate's HIGH floor holds $held" 1 3 "it changes $held, a hold path" done -# The rest of .github merges on the review lanes (owner direction -# 2026-10-23); the lanes App's token cannot push a workflow file. +# The rest of .github merges on the review lanes; the lanes App's token never +# carries the workflows permission, so no lanes App push changes a workflow. for path in .github/workflows/x.yml .github/scripts/pr-merge/merge-holds.mjs .github/actions/other/action.yml; do STUB_FILES="[[\"$path\"]]" STUB_PULL="$(pull 'Closes #1')" expect "$path is no hold path" 0 3 "no hold path" From 818f9140f098adc08ab0b49956dc775bc2249bbb Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Fri, 23 Oct 2026 22:28:54 -0400 Subject: [PATCH 3/3] fix(pr-pipeline): hold request-lane-token on the takeover floor Co-Authored-By: Claude Opus 5.5 --- .github/scripts/pr-merge/merge-holds.mjs | 1 + docs/conventions/pr-pipeline/README.md | 3 ++- .../source-control/skills/babysit-prs/scripts/babysit_risk.py | 4 +++- 3 files changed, 6 insertions(+), 2 deletions(-) diff --git a/.github/scripts/pr-merge/merge-holds.mjs b/.github/scripts/pr-merge/merge-holds.mjs index 93e2576f05f..6a725d8d28d 100755 --- a/.github/scripts/pr-merge/merge-holds.mjs +++ b/.github/scripts/pr-merge/merge-holds.mjs @@ -157,6 +157,7 @@ export const HOLD_PATH_FLOOR = Object.freeze([ "**/.gitmodules", ".github/actions/resolve-config/**", ".github/actions/check-kill-switch/**", + ".github/actions/request-lane-token/**", ]); // babysit_risk.is_manifest_path: a plugin manifest or the marketplace diff --git a/docs/conventions/pr-pipeline/README.md b/docs/conventions/pr-pipeline/README.md index 63013c886c1..d08500db7e0 100644 --- a/docs/conventions/pr-pipeline/README.md +++ b/docs/conventions/pr-pipeline/README.md @@ -755,7 +755,8 @@ and its queued entries leave; two different identities, or an unreadable head co (owner direction 2026-10-23): `.github/standards/trusted-actors/**`, `.github/actions/check-trusted-trigger/**`, `CODEOWNERS`, `.gitmodules`, `.github/actions/resolve-config/**`, whose code the token broker runs from main in its signer - process, and `.github/actions/check-kill-switch/**`, whose output writer both actions use. Agent permission config, package manifests and workflows merge on the review lanes. A + process, and `.github/actions/check-kill-switch/**`, whose output writer both actions use, and + `.github/actions/request-lane-token/**`, which mints the lanes App token. Agent permission config, package manifests and workflows merge on the review lanes. A diff that touches a floor path scores HIGH and is never merged unattended. A `plugins//.claude-plugin/plugin.json` or the top-level `.claude-plugin/marketplace.json` change scores HIGH as `manifest-review-required` when it diff --git a/plugins/source-control/skills/babysit-prs/scripts/babysit_risk.py b/plugins/source-control/skills/babysit-prs/scripts/babysit_risk.py index 8e93b07ac06..34cc58eb84f 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_risk.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_risk.py @@ -205,7 +205,8 @@ class RiskResult: # (code owners), what code is pulled in from elsewhere (submodules), and the # code the token broker runs from main in its signer process (resolve-config, # owner decision Q51), with check-kill-switch, whose writeOutputs both of those -# actions write their verdicts through. +# actions write their verdicts through, and request-lane-token, which mints and +# hands out the lanes App token in every privileged workflow. HIGH_FLOOR: tuple[str, ...] = ( ".github/standards/trusted-actors/**", ".github/actions/check-trusted-trigger/**", @@ -213,6 +214,7 @@ class RiskResult: "**/.gitmodules", ".github/actions/resolve-config/**", ".github/actions/check-kill-switch/**", + ".github/actions/request-lane-token/**", ) # plugin.json keys whose values Claude Code runs or reads as run config.