From 1fdb23ef211b53116dc2cc0b09626bd18ef21799 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Wed, 11 Nov 2026 10:41:25 -0500 Subject: [PATCH 1/4] fix(pr-pipeline): reject skill activities whose effect is not read A skill runs only in pr-run-activity-read.yml, which holds no write token. The schema and resolve-config (rejection 17, skill-not-read) now refuse a skill whose effect is not read at config load, in any lane. Every skill in pr-pipeline.yaml already has effect read. With no config able to reach it, the write runner's skill path is removed: the claude-code-action step, the lane commit branch and promote step, the turn-cap, model-turn and verdict checks, the reply upload, the lane-outcome record and the unused OAuth secret. The write runner now fails red before the mint for any skill. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01VQb9Q2ceZLXhTR4d1vkTCe --- .../check-signed-commits.test.mjs | 7 +- .../actions/collect-base-activity/action.yml | 2 +- .github/actions/resolve-config/README.md | 8 +- .../actions/resolve-config/callers.test.mjs | 115 +--- .../resolve-config/fixtures/valid.yaml | 10 + .../resolve-config/live-config.test.mjs | 15 +- .../resolve-config/resolve-config.test.mjs | 27 +- .github/actions/resolve-config/resolve.mjs | 13 +- .../actions/resolve-config/resolve.test.mjs | 73 ++- .../resolve-config/self-checks.test.mjs | 34 +- .../resolve-config/verify-scripts.test.mjs | 29 +- .../resolve-config/web-access.test.mjs | 19 +- .github/workflows/pr-run-activity-write.yml | 566 +----------------- docs/conventions/pr-pipeline/CHANGELOG.md | 12 + docs/conventions/pr-pipeline/README.md | 40 +- .../examples/claude-code-plugins.yaml | 14 +- .../pr-pipeline/pr-pipeline.schema.json | 42 +- .../pr-pipeline/pr-run-activity.md | 131 +--- scripts/act-turns-denied.test.sh | 22 +- scripts/write-runner-hold-labels.test.sh | 90 +++ scripts/write-runner-never-rules.test.sh | 49 +- scripts/write-runner-over-cap-success.test.sh | 102 ---- scripts/write-runner-promote-holds.test.sh | 213 ------- 23 files changed, 405 insertions(+), 1228 deletions(-) create mode 100755 scripts/write-runner-hold-labels.test.sh delete mode 100755 scripts/write-runner-over-cap-success.test.sh delete mode 100755 scripts/write-runner-promote-holds.test.sh diff --git a/.github/actions/check-signed-commits/check-signed-commits.test.mjs b/.github/actions/check-signed-commits/check-signed-commits.test.mjs index 6fd97642b82..770a5ce5d05 100644 --- a/.github/actions/check-signed-commits/check-signed-commits.test.mjs +++ b/.github/actions/check-signed-commits/check-signed-commits.test.mjs @@ -469,10 +469,15 @@ function workflowJobs(name) { return jobs; } +// The write runner runs no model. +test("pr-run-activity-write.yml: no job runs claude-code-action or run-claude-cli", () => { + assert.deepEqual([...workflowJobs("pr-run-activity-write.yml")].filter(([, job]) => job.model).map(([name]) => name), []); +}); + // claude-code-action passes the job's GITHUB_TOKEN to the model, and a // run-claude-cli job's other steps hold it, so a job that runs either holds // contents and pull-requests read and no issues permission. -for (const file of ["pr-run-activity-write.yml", "pr-run-activity-read.yml"]) { +for (const file of ["pr-run-activity-read.yml"]) { test(`${file}: no model job holds issues or a contents or pull-requests write`, () => { const jobs = [...workflowJobs(file)]; const models = jobs.filter(([, job]) => job.model); diff --git a/.github/actions/collect-base-activity/action.yml b/.github/actions/collect-base-activity/action.yml index 02d1e1d5138..3c7f5f2f64b 100644 --- a/.github/actions/collect-base-activity/action.yml +++ b/.github/actions/collect-base-activity/action.yml @@ -254,7 +254,7 @@ runs: # naming a path outside it (a script may print what it reads), unsets # GH_TOKEN and GITHUB_TOKEN in the script's own environment (no Bash # process above it holds them either: run-claude-cli's steps set - # neither, and the write runner's skill settings clear both), and + # neither), and # keeps git from loading system, global or hook and fsmonitor # programs. As for the # checks, is where the copy is written and where it runs. diff --git a/.github/actions/resolve-config/README.md b/.github/actions/resolve-config/README.md index eddf257cb18..09a19aa025b 100644 --- a/.github/actions/resolve-config/README.md +++ b/.github/actions/resolve-config/README.md @@ -73,7 +73,7 @@ The step fails and writes no file when: 10. A composite contains itself, directly or through another (`activity-cycle`), or holds a composite, which would put a leaf more than two layers below its lane (`activity-depth`). 11. A leaf sets `prepare` with an effect its mode does not take: `ci-context` needs `read`, - `apply-patch` `mutate-branch`, the others an effect other than `read` (`invalid-prepare`). + `apply-patch` `mutate-branch`; the others need `applied-by.patch` (`invalid-prepare`). 12. The selected leaf applies and sets `prepare`, and `prepare-supported` is not `true`: the runner would run the skill without its prepare step. The action checks this; `resolveGrant` does not. @@ -83,7 +83,7 @@ The step fails and writes no file when: 14. The patch rules or their schema are missing, fail the schema, or a row does not refuse the universal set, commits with the base as a parent with an `allow` other than exactly `[merge-result]`, or a patch-producing activity's prepare mode has no row (`patch-rules`). The - action checks this right after `loadConfig` (rejections 1-11, 13 and 16) and before it gathers + action checks this right after `loadConfig` (rejections 1-11, 13, 16 and 17) and before it gathers any fact or resolves a slot, for every lane; rejections 12 and 15 come after the slot resolves. `loadConfig` and `resolveGrant` do not check the patch rules, so the token broker never reads these files. @@ -92,6 +92,10 @@ The step fails and writes no file when: 16. `models.default-profile` or a route names a profile `models.profiles` does not define (`undefined-profile`), or a route key is neither a skill activity nor a run route (`verify-pr`, `review-code`, `review-security`, `triage-issue`, `summarize-reuse`, `adjudicate-feedback`; `undefined-route`), so a mistyped route cannot quietly take the default. +17. A skill leaf's `effect` is not `read` (`skill-not-read`), in any lane or none: a skill runs + only in the read runner, which holds no write token, and a script activity applies or publishes + what it hands on. The schema refuses it first; the reader repeats the check so it holds whatever + schema it is given. It also fails when a predicate it decides cannot be decided: `pulls/{n}/files` lists 3000 files or fewer than the PR's `changed_files`, a predicate needs the PR and there is no `pr-number`, or a diff --git a/.github/actions/resolve-config/callers.test.mjs b/.github/actions/resolve-config/callers.test.mjs index c06d7fe2f73..58a1ae0b73b 100644 --- a/.github/actions/resolve-config/callers.test.mjs +++ b/.github/actions/resolve-config/callers.test.mjs @@ -1938,66 +1938,38 @@ test("the write runner keeps a lane-hold marker only for a held script activity } }); -test("no job using pr-run-activity-write.yml runs the resolve-issue model step, and the write runner refuses an issue skill", () => { +test("no job using pr-run-activity-write.yml runs a skill activity, and the write runner refuses a skill before the mint", () => { const dir = new URL("../../workflows/", import.meta.url); + const config = parse(read("../../../docs/conventions/pr-pipeline.yaml")); for (const name of readdirSync(dir).filter((file) => /\.ya?ml$/.test(file))) { const { jobs = {} } = parse(readFileSync(new URL(name, dir), "utf8")) ?? {}; for (const [id, job] of Object.entries(jobs)) { if (String(job?.uses ?? "").endsWith("pr-run-activity-write.yml")) { - assert.notEqual(job.with?.activity, "resolve-issue", `${name} ${id}`); + assert.equal(typeof config.activities[job.with?.activity]?.skill, "undefined", `${name} ${id}`); } } } const { jobs } = workflow("pr-run-activity-write.yml"); - const refuse = jobs.run.steps.find((s) => s.name === "Assert an issue skill runs in the read runner"); + const steps = jobs.run.steps; + const refuse = steps.find((s) => s.name === "Assert a skill runs in the read runner"); assert.ok(refuse); + assert.ok(steps.indexOf(refuse) < steps.findIndex((s) => s.id === "lane-token")); const context = (issue, kind) => ({ "inputs.issue-number": issue, "steps.config.outcome": "success", "steps.config.outputs.kind": kind }); assert.equal(evaluate(refuse.if, context("7", "skill")), true); + assert.equal(evaluate(refuse.if, context("", "skill")), true); assert.equal(evaluate(refuse.if, context("7", "script")), false); - assert.equal(evaluate(refuse.if, context("", "skill")), false); assert.equal(spawnSync("bash", ["-c", refuse.run]).status, 1); + assert.ok(!steps.some((s) => /claude-code-action/.test(s.uses ?? "")), "the write runner runs no model"); }); // The write runner's run job holds the lane token and id-token: write, so it -// prepares no runner for head test files: the schema refuses run-test-file.sh -// and check-skill.sh on a write-effect skill (verify-scripts.test.mjs), and a -// read skill runs them in pr-run-activity-read.yml, whose run job holds neither -// (pinned below with its read-only permissions). +// prepares no runner for head test files: a skill runs them in +// pr-run-activity-read.yml, whose run job holds neither (pinned below with its +// read-only permissions). test("pr-run-activity-write.yml prepares no head test-file runner", () => { assert.doesNotMatch(read("../../workflows/pr-run-activity-write.yml"), /run-test-file|check-skill/); }); -// claude-code-action puts the lane token in GH_TOKEN and GITHUB_TOKEN of the -// environment it hands Claude, and the environment scrub keeps both for Bash, -// so the skill's settings set both to empty: no Bash process, the wrappers' -// ancestors included, holds the token in its /proc//environ. The action's -// MCP servers get the token from their own config env. -test("pr-run-activity-write.yml clears the GitHub token from the skill's Bash", () => { - const steps = Object.values(workflow("pr-run-activity-write.yml").jobs).flatMap((job) => job.steps ?? []); - const skill = steps.find((step) => step.id === "act-skill"); - const clear = steps.find((step) => step.id === "skill-settings"); - assert.ok(clear, "no step clears the token from the skill's settings"); - assert.ok(steps.indexOf(clear) < steps.indexOf(skill)); - assert.equal(clear.if, skill.if); - assert.equal(clear.env.SETTINGS_JSON, "${{ steps.config.outputs.settings-json }}"); - assert.equal(skill.with.settings, "${{ steps.skill-settings.outputs.json }}"); - const dir = tempDir("skill-settings-"); - const output = join(dir, "output"); - const run = (settingsJson) => { - writeFileSync(output, ""); - const result = spawnSync("bash", ["-c", clear.run], { - env: { ...process.env, SETTINGS_JSON: settingsJson, GITHUB_OUTPUT: output }, - encoding: "utf8", - }); - assert.equal(result.status, 0, result.stderr); - return JSON.parse(/^json=(.*)$/m.exec(readFileSync(output, "utf8"))[1]); - }; - const cleared = { GH_TOKEN: "", GITHUB_TOKEN: "" }; - assert.deepEqual(run(""), { env: cleared }); - assert.deepEqual(run(SUBAGENT), { env: { CLAUDE_CODE_SUBAGENT_MODEL: "haiku", ...cleared } }); - assert.deepEqual(run('{"env":{"GH_TOKEN":"x"},"model":"m"}'), { env: cleared, model: "m" }); -}); - // The admission gate: on an issue anyone can open, only a listed human's // agent-ready label (or the lanes App's own) may start a job that gets the // issue lane's Claude credential or runs the read runner. @@ -3100,21 +3072,19 @@ test("pr-run-activity-read.yml: the fallback call's lane record names why it ran assert.equal(credential({}), ""); }); -// The same action, at the same pin, as the write workflow. -test("pr-run-activity-read.yml: the lane record uses the write workflow's report-lane-outcome", () => { - const { jobs } = workflow("pr-run-activity-read.yml"); - const writeRecord = workflow("pr-run-activity-write.yml").jobs.run.steps.find((s) => s.id === "lane-record"); - assert.equal(jobs.run.steps.find((s) => s.id === "lane-record").uses, writeRecord.uses); -}); - -// Both lane check runs carry the lane-outcome record itself, so limit-stop.mjs -// reads one JSON shape from read and write activities alike. -test("pr-run-activity read and write: the posted check carries the lane-outcome record", () => { +// The read runner's check run carries the lane-outcome record, the JSON shape +// limit-stop.mjs reads; the write runner runs no model, so it has none. +test("pr-run-activity read: the posted check carries the lane-outcome record; write: none", () => { for (const file of ["pr-run-activity-read.yml", "pr-run-activity-write.yml"]) { const { jobs } = workflow(file); - assert.equal(jobs.run.outputs.record, "${{ steps.lane-record.outputs.record }}", file); const post = jobs.report.steps.find((s) => s.uses === "./.base/.github/actions/report-check-run"); - assert.equal(post.with["lane-outcome"], "${{ needs.run.outputs.record }}", file); + if (file === "pr-run-activity-read.yml") { + assert.equal(jobs.run.outputs.record, "${{ steps.lane-record.outputs.record }}", file); + assert.equal(post.with["lane-outcome"], "${{ needs.run.outputs.record }}", file); + } else { + assert.equal(jobs.run.outputs.record, undefined, file); + assert.equal(post.with["lane-outcome"], undefined, file); + } // The retired key=value summary line's inputs (ADR 0087 slice 3). assert.deepEqual(Object.keys(post.with).filter((key) => /^lane-(class|resets|turns|max-turns|models|profile)$/.test(key)), [], file); } @@ -4660,43 +4630,6 @@ test("pr-run-activity-read.yml: after the collector, no checkout's post step fin assert.equal(runStep(remove, state.temp, {}).status, 1, "an unset workspace removes nothing"); }); -test("pr-run-activity-write.yml: after the skill, no checkout's post step finds a .git/config to run git in", async (t) => { - const steps = workflow("pr-run-activity-write.yml").jobs.run.steps; - const remove = steps.find((s) => s.name === "Remove the checkouts' .git directories"); - const at = (predicate) => steps.findIndex(predicate); - assert.equal(at((s) => s === remove), at((s) => s.id === "act-result") + 1, "right after the turn-cap check, the skill's own next step"); - // Pinned, as the revoke step is: the skill could have changed PATH or BASH_ENV. - assert.equal(remove.run, undefined); - assert.match(remove.uses, /^actions\/github-script@[0-9a-f]{40}$/); - // A failed or cancelled skill still runs it, since the post steps run after a - // cancel; a script activity, which needs its checkout, never does. - assert.equal(evaluate(remove.if, { "steps.act-skill.outcome": "success" }), true); - assert.equal(evaluate(remove.if, { "steps.act-skill.outcome": "failure" }), true); - assert.equal(evaluate(remove.if, { "steps.act-skill.outcome": "cancelled" }), true); - assert.equal(evaluate(remove.if, { "steps.act-skill.outcome": "skipped" }), false); - // No step a skill run reaches after it runs git or checks out. - const gitAfter = steps.slice(at((s) => s === remove) + 1).filter((s) => /\bgit\b/.test(String(s.run ?? "")) || String(s.uses ?? "").startsWith("actions/checkout@")); - assert.deepEqual(gitAfter.map((s) => s.name), []); - const { createRequire } = await import("node:module"); - const run = async (env) => { - let failed = null; - const core = { setFailed: (message) => (failed = message) }; - await new AsyncFunction("require", "core", "process", remove.with.script)(createRequire(import.meta.url), core, { env }); - return failed; - }; - const state = plantedCheckout(t); - state.plant(); - mkdirSync(join(state.work, ".base", ".git"), { recursive: true }); - writeFileSync(join(state.work, ".base", ".git", "config"), "[core]\n\tfsmonitor = /bin/false\n"); - assert.notEqual(await run({ WORKSPACE: "" }), null, "an empty workspace fails"); - assert.equal(existsSync(join(state.work, ".git", "config")), true, "and removes nothing"); - assert.equal(await run({ WORKSPACE: state.work }), null); - assert.equal(existsSync(join(state.work, ".git", "config")), false); - assert.equal(existsSync(join(state.work, ".base", ".git", "config")), false); - assert.equal(readFileSync(join(state.work, "tracked.txt"), "utf8"), "unchanged\n", "the work tree stays"); - assert.equal(await run({ WORKSPACE: state.work }), null, "nothing to remove is no failure"); -}); - // --- the model job's payloads to the scan job, sealed --- // name=value lines from a GITHUB_OUTPUT file; the carry and seal steps write no heredoc. @@ -9540,7 +9473,7 @@ test("pr-unblock.yml: resolve-feedback-threads is its own base-only script job a assert.equal(grants["publish-feedback"], "publish-artifact"); // The write runner: checkout base skips the head checkout and its assert; - // the base checkout persists no credentials; a script runs no skill step. + // the base checkout persists no credentials; the runner has no skill step. const steps = workflow("pr-run-activity-write.yml").jobs.run.steps; const step = (name) => steps.find((s) => s.name === name); assert.equal(step("Check out the default branch tip").with["persist-credentials"], false); @@ -9549,9 +9482,9 @@ test("pr-unblock.yml: resolve-feedback-threads is its own base-only script job a for (const name of ["Check out the PR head", "Assert the head is the gated SHA"]) { assert.equal(on(name, "base"), false, `${name} skips on checkout: base`); assert.equal(on(name, "head"), true, `${name} runs on checkout: head`); - assert.equal(on(name, ""), true, `${name} runs for a skill`); + assert.equal(on(name, ""), true, `${name} runs when checkout is unset`); } - assert.match(String(steps.find((s) => s.id === "act-skill").if), /kind == 'skill'/); + assert.equal(steps.find((s) => s.id === "act-skill"), undefined); assert.match(String(steps.find((s) => s.id === "act-script").if), /kind == 'script'/); }); diff --git a/.github/actions/resolve-config/fixtures/valid.yaml b/.github/actions/resolve-config/fixtures/valid.yaml index 39f77937034..e29c38af47b 100644 --- a/.github/actions/resolve-config/fixtures/valid.yaml +++ b/.github/actions/resolve-config/fixtures/valid.yaml @@ -15,6 +15,14 @@ activities: paths: ["src/**"] fix-prose: skill: ai-slop:audit + applied-by: + patch: apply-prose + effect: read + gating: advisory + reads-untrusted: false + apply-prose: + script: .github/scripts/apply-patch/apply-patch.mjs + prepare: apply-patch effect: mutate-branch gating: advisory reads-untrusted: false @@ -29,3 +37,5 @@ lanes: stage: refine slots: - activity: fix-prose + - activity: apply-prose + needs: [fix-prose] diff --git a/.github/actions/resolve-config/live-config.test.mjs b/.github/actions/resolve-config/live-config.test.mjs index 200a3155d76..e41751d5268 100644 --- a/.github/actions/resolve-config/live-config.test.mjs +++ b/.github/actions/resolve-config/live-config.test.mjs @@ -592,10 +592,9 @@ test("live config: resolve-issue holds no write token; apply-issue-patch may onl }); }); -// activity-rules.json is trust-root data: a config that gives resolve-issue a -// write effect again is refused. The schema takes tidy only on a read skill -// that hands on a patch, so the write-effect config drops it too, and the -// refusal comes from the activity rules rather than the schema. +// A config that gives resolve-issue a write effect again is refused: a skill's +// effect is read. The write-effect config drops tidy, which the schema takes +// only on a read skill that hands on a patch. test("live config: resolve-issue with a write effect is refused", () => { const writes = FILES.config .replace( @@ -606,7 +605,7 @@ test("live config: resolve-issue with a write effect is refused", () => { assert.notEqual(writes, FILES.config); assert.throws( () => resolve({ ...FILES, config: writes }, { lane: "work-resolve-issue", activity: "resolve-issue", configPath: CONFIG_PATH, basePath: ".base", facts: { ...FACTS, ...ISSUE } }), - (error) => error.code === "forbidden-by-activity", + (error) => error.code === "invalid-config", ); }); @@ -823,14 +822,14 @@ test("live config: fix-docs is not asked to fix an instruction surface", () => { assert.deepEqual(slot["changed-paths"], ["docs/guide.md"]); }); -// activity-rules.json is trust-root data: a config that gives fix-docs a -// write effect again is refused. +// A config that gives fix-docs a write effect again is refused: a skill's +// effect is read. test("live config: fix-docs with a write effect is refused", () => { const writes = FILES.config.replace("effect: read\n applied-by:\n patch: apply-patch\n", "effect: mutate-branch\n"); assert.notEqual(writes, FILES.config); assert.throws( () => resolve({ ...FILES, config: writes }, { lane: "pr-refine", activity: "fix-docs", configPath: CONFIG_PATH, basePath: ".base", facts: FACTS }), - (error) => error.code === "forbidden-by-activity", + (error) => error.code === "invalid-config", ); }); diff --git a/.github/actions/resolve-config/resolve-config.test.mjs b/.github/actions/resolve-config/resolve-config.test.mjs index ae6ece704b9..dadc5fe54ce 100644 --- a/.github/actions/resolve-config/resolve-config.test.mjs +++ b/.github/actions/resolve-config/resolve-config.test.mjs @@ -211,12 +211,13 @@ test("a valid config writes ResolvedConfig and the selected activity's outputs", test("a mutate-branch activity can commit; args stay in the file only", async () => { const { env } = workspace(); env.LANE = "pr-refine"; - env.ACTIVITY = "fix-prose"; + env.ACTIVITY = "apply-prose"; + env.PREPARE_SUPPORTED = "true"; assert.equal(await main({ env, github: noApi }), 0); const outputs = readOutputs(env.GITHUB_OUTPUT); assert.deepEqual( - [outputs.contents, outputs["can-commit"], outputs.skill, outputs.gating], - ["write", "true", "ai-slop:audit", "advisory"], + [outputs.contents, outputs["can-commit"], outputs.script, outputs.gating], + ["write", "true", ".github/scripts/apply-patch/apply-patch.mjs", "advisory"], ); assert.equal(Object.hasOwn(outputs, "args"), false); }); @@ -227,10 +228,11 @@ const withPermissionMode = (mode) => { const { env } = workspace(); env.LANE = "pr-refine"; env.ACTIVITY = "fix-prose"; + env.APPLIED_BY_SUPPORTED = "patch"; const path = join(env.BASE_PATH, CONFIG_PATH); if (mode !== undefined) { const config = readFileSync(path, "utf8"); - const set = config.replace(" reads-untrusted: false\nlanes:", ` reads-untrusted: false\n permission-mode: ${mode}\nlanes:`); + const set = config.replace(" reads-untrusted: false\n apply-prose:", ` reads-untrusted: false\n permission-mode: ${mode}\n apply-prose:`); assert.notEqual(set, config); writeFileSync(path, set); } @@ -279,7 +281,7 @@ test("a script's checkout output is head unless the leaf sets base; a skill has test("checkout fails the schema on a skill or with a value other than head or base", async (t) => { await assertFailsRed(t, withCheckout("pr"), /checkout/); - await assertFailsRed(t, withCheckout("base", { activity: "fix-prose", after: / reads-untrusted: false\n(?=lanes:)/ }), /checkout|script/); + await assertFailsRed(t, withCheckout("base", { activity: "fix-prose", after: / reads-untrusted: false\n(?= apply-prose:)/ }), /checkout|script/); }); test("without an activity only enabled and slots are output", async () => { @@ -524,8 +526,8 @@ test("changed-paths holds the head's changed files minus removed ones and the ba writeFileSync( config, readFileSync(config, "utf8").replace( - " effect: mutate-branch\n", - " effect: mutate-branch\n inputs: [changed-paths]\n", + " patch: apply-prose\n", + " patch: apply-prose\n inputs: [changed-paths]\n", ), ); mkdirSync(join(env.BASE_PATH, ".github"), { recursive: true }); @@ -535,6 +537,7 @@ test("changed-paths holds the head's changed files minus removed ones and the ba ); env.LANE = "pr-refine"; env.ACTIVITY = "fix-prose"; + env.APPLIED_BY_SUPPORTED = "patch"; const files = [ { filename: "docs/kept.md", status: "modified" }, { filename: "docs/gone.md", status: "removed" }, @@ -631,12 +634,13 @@ function withPredicate(env, predicate) { writeFileSync( config, readFileSync(config, "utf8").replace( - " effect: mutate-branch\n", - ` effect: mutate-branch\n applies-when: ${JSON.stringify(predicate)}\n`, + " patch: apply-prose\n", + ` patch: apply-prose\n applies-when: ${JSON.stringify(predicate)}\n`, ), ); env.LANE = "pr-refine"; env.ACTIVITY = "fix-prose"; + env.APPLIED_BY_SUPPORTED = "patch"; } test("labels come from the pull_request payload without an API call", async () => { @@ -748,12 +752,13 @@ function withPrepare(env, extra = "") { writeFileSync( config, readFileSync(config, "utf8").replace( - " effect: mutate-branch\n", - ` effect: mutate-branch\n prepare: merge-base\n${extra}`, + " patch: apply-prose\n", + ` patch: apply-prose\n prepare: merge-base\n${extra}`, ), ); env.LANE = "pr-refine"; env.ACTIVITY = "fix-prose"; + env.APPLIED_BY_SUPPORTED = "patch"; } test("a leaf's prepare mode is a step output, set only when the leaf declares one", async () => { diff --git a/.github/actions/resolve-config/resolve.mjs b/.github/actions/resolve-config/resolve.mjs index 1f1ff583639..ea5364ad84b 100644 --- a/.github/actions/resolve-config/resolve.mjs +++ b/.github/actions/resolve-config/resolve.mjs @@ -211,9 +211,16 @@ function checkLeaves(activities, activityRules) { if (isComposite(activity)) { continue; } + // A skill runs only in the read runner, which holds no write credential; + // a script applies or publishes what it hands on. + if (typeof activity.skill === "string" && activity.effect !== "read") { + throw new Rejection( + "skill-not-read", + `activity \`${name}\` is a skill with effect ${activity.effect}; a skill has effect read and hands its change to a script activity`, + ); + } // ci-context is the read runner's one mode, apply-patch a branch-writing - // script's; every other mode needs a skill that can write, or a read - // skill whose patch a script applies. + // script's; every other mode needs a read skill whose patch a script applies. const needs = { "ci-context": "read", "apply-patch": "mutate-branch" }[activity.prepare]; const handsOnPatch = isObject(activity["applied-by"]) && typeof activity["applied-by"].patch === "string"; if ( @@ -224,7 +231,7 @@ function checkLeaves(activities, activityRules) { "invalid-prepare", needs ? `activity \`${name}\` sets prepare: ${activity.prepare}, which needs effect ${needs}` - : `activity \`${name}\` sets prepare with effect read and no applied-by.patch; prepare needs a skill that can write or hands on a patch`, + : `activity \`${name}\` sets prepare with no applied-by.patch; a skill's prepare mode hands its change on as a patch`, ); } const rule = own(activityRules, name); diff --git a/.github/actions/resolve-config/resolve.test.mjs b/.github/actions/resolve-config/resolve.test.mjs index 5dfca6b1567..2c9d5ce3251 100644 --- a/.github/actions/resolve-config/resolve.test.mjs +++ b/.github/actions/resolve-config/resolve.test.mjs @@ -143,11 +143,32 @@ test("rejection 3: a gate activity in pr-explain is rejected", () => { test("rejection 3: a merge effect outside pr-merge is rejected in a lane the run did not ask for", () => { const config = edited((doc) => { - doc.activities["fix-prose"].effect = "merge"; + doc.activities["apply-prose"].effect = "merge"; + delete doc.activities["apply-prose"].prepare; }); assertRejected(() => run(config), "forbidden-by-lane"); }); +// A skill runs only in the read runner: the schema and the reader each refuse +// a skill whose effect is not read, in any lane. +test("a skill with an effect other than read fails the schema and the reader", () => { + const schemaWithoutRule = structuredClone(SCHEMA); + schemaWithoutRule.$defs.leaf.allOf = schemaWithoutRule.$defs.leaf.allOf.filter( + (rule) => JSON.stringify(rule.if) !== JSON.stringify({ required: ["skill"] }), + ); + assert.equal(schemaWithoutRule.$defs.leaf.allOf.length, SCHEMA.$defs.leaf.allOf.length - 1); + for (const effect of ["mutate-branch", "publish-artifact", "mutate-tracker"]) { + const config = edited((doc) => { + doc.activities["measure-coverage"].effect = effect; + }); + assertRejected(() => run(config, { lane: "pr-refine", activity: "fix-prose" }), "invalid-config"); + assertRejected( + () => run(config, { lane: "pr-refine", activity: "fix-prose" }, { schema: schemaWithoutRule }), + "skill-not-read", + ); + } +}); + // Rejection 4 test("rejection 4: a lane whose stage differs from lane-rules.json is rejected", () => { @@ -803,7 +824,7 @@ test("resolveGrant returns the selected slot's effect and grant", () => { grant: READ_GRANT, }); assert.deepEqual( - grantOf(fixture("valid.yaml"), { lane: "pr-refine", activity: "fix-prose" }), + grantOf(fixture("valid.yaml"), { lane: "pr-refine", activity: "apply-prose" }), { effect: "mutate-branch", grant: { contents: "write", "pull-requests": "read", issues: "read" }, @@ -1108,7 +1129,7 @@ test("changed-paths with nothing left skips the activity as not-applicable-paths test("a read activity's changed-paths keeps instruction surfaces and owned paths", () => { const config = withChangedPaths((doc) => { - doc.activities["fix-prose"].effect = "read"; + delete doc.activities["fix-prose"]["applied-by"]; }); assert.deepEqual( targetsOf(["CLAUDE.md", "plugins/ai-slop/README.md"], config)[ @@ -1138,7 +1159,7 @@ const COMPOSITE = `version: 2 activities: simplify: skill: code-tidying:batch-simplify - effect: mutate-branch + effect: read gating: advisory reads-untrusted: false fix-lint: @@ -1448,13 +1469,23 @@ test("selecting a composite key is rejected as undefined-activity", () => { // prepare +// simplify hands its change on as a patch to apply-simplify, which needs it. const withPrepare = (edit) => composite((doc) => { doc.activities.simplify.prepare = "merge-base"; + doc.activities.simplify["applied-by"] = { patch: "apply-simplify" }; + doc.activities["apply-simplify"] = { + script: ".github/scripts/apply-patch/apply-patch.mjs", + prepare: "apply-patch", + effect: "mutate-branch", + gating: "advisory", + "reads-untrusted": false, + }; + doc.lanes["pr-refine"].slots.push({ activity: "apply-simplify", needs: ["simplify"] }); edit?.(doc); }); -test("prepare on a non-read skill leaf resolves and is carried on the slot", () => { +test("prepare on a skill leaf that hands on a patch resolves and is carried on the slot", () => { const selected = runRefine(withPrepare(), { activity: "simplify", facts: LINT_FACTS, @@ -1470,9 +1501,10 @@ test("prepare on a non-read skill leaf resolves and is carried on the slot", () ); }); -test("prepare: review-threads on a non-read skill leaf resolves and is carried on the slot", () => { +test("prepare: review-threads on a skill leaf that hands on a patch and replies resolves and is carried on the slot", () => { const config = withPrepare((doc) => { doc.activities.simplify.prepare = "review-threads"; + doc.activities.simplify["applied-by"]["feedback-replies"] = "apply-simplify"; }); assert.equal(runRefine(config, { activity: "simplify", facts: LINT_FACTS }).selected.prepare, "review-threads"); }); @@ -1481,7 +1513,7 @@ test("prepare: review-threads on a non-read skill leaf resolves and is carried o // requires applied-by.patch of it. test("prepare on a read skill leaf with no applied-by.patch is rejected", () => { const config = withPrepare((doc) => { - doc.activities.simplify.effect = "read"; + delete doc.activities.simplify["applied-by"]; }); assertRejected(() => runRefine(config), "invalid-config"); }); @@ -1499,16 +1531,16 @@ test("prepare on a script leaf, or a value other than merge-base, fails the sche // A read ci-context skill that hands on its patch resolves: the live fix-ci // (live-config.test.mjs). Without applied-by.patch its edits would leave nowhere. -test("prepare: ci-context needs effect read, and a read one needs applied-by.patch", () => { - const read = withPrepare((doc) => { +test("prepare: ci-context resolves on a skill that hands on a patch, and fails the schema without one", () => { + const handsOn = withPrepare((doc) => { doc.activities.simplify.prepare = "ci-context"; - doc.activities.simplify.effect = "read"; }); - assertRejected(() => runRefine(read), "invalid-config"); - const write = withPrepare((doc) => { + assert.equal(runRefine(handsOn, { activity: "simplify", facts: LINT_FACTS }).selected.prepare, "ci-context"); + const none = withPrepare((doc) => { doc.activities.simplify.prepare = "ci-context"; + delete doc.activities.simplify["applied-by"]; }); - assertRejected(() => runRefine(write), "invalid-prepare"); + assertRejected(() => runRefine(none), "invalid-config"); }); test("prepare: apply-patch is the one mode a script may set, and it needs effect mutate-branch", () => { @@ -1629,17 +1661,18 @@ lanes: - activity: ${activity} `; -// address-feedback runs on the read runner: it neither pushes nor posts. -for (const [activity, allowed, forbidden] of [ - ["address-feedback", "read", ["mutate-branch", "publish-artifact", "mutate-tracker", "merge"]], - ["publish-feedback", "publish-artifact", ["mutate-branch", "mutate-tracker", "merge"]], +// address-feedback runs on the read runner: it neither pushes nor posts. As a +// skill, the schema refuses any other effect before its activity rule does. +for (const [activity, allowed, forbidden, code] of [ + ["address-feedback", "read", ["mutate-branch", "publish-artifact", "mutate-tracker", "merge"], "invalid-config"], + ["publish-feedback", "publish-artifact", ["mutate-branch", "mutate-tracker", "merge"], "forbidden-by-activity"], ]) { test(`the shipped activity rules give ${activity} ${allowed} and nothing that ${allowed === "read" ? "pushes or posts" : "pushes"}`, () => { // The live vocabulary: the fixture lacks the verb "publish". const live = { vocabulary: JSON.parse(readFileSync(LIVE_VOCABULARY, "utf8")) }; assert.equal(run(feedbackLeaf(activity, allowed), { lane: "pr-unblock", activity }, live).selected.effect, allowed); for (const effect of forbidden) { - assertRejected(() => run(feedbackLeaf(activity, effect), { lane: "pr-unblock", activity }, live), "forbidden-by-activity"); + assertRejected(() => run(feedbackLeaf(activity, effect), { lane: "pr-unblock", activity }, live), code); } }); } @@ -1697,7 +1730,7 @@ function resolveEveryExampleSlot(vocabulary) { test("the README example validates and resolves for each of its lanes", () => { const results = resolveEveryExampleSlot(VOCABULARY); - assert.equal(Object.keys(results).length, 16); + assert.equal(Object.keys(results).length, 17); // The example's models block routes claude to review and leaves the rest // on default-profile standard. assert.deepEqual(results["pr-review / claude"].models, { @@ -1747,7 +1780,7 @@ test("integration: the README example resolves against the synced standards voca const results = resolveEveryExampleSlot( JSON.parse(readFileSync(LIVE_VOCABULARY, "utf8")), ); - assert.equal(Object.keys(results).length, 16); + assert.equal(Object.keys(results).length, 17); }); // applied-by: a read skill names the script activity that applies each thing diff --git a/.github/actions/resolve-config/self-checks.test.mjs b/.github/actions/resolve-config/self-checks.test.mjs index df09990dbc3..c51593797db 100644 --- a/.github/actions/resolve-config/self-checks.test.mjs +++ b/.github/actions/resolve-config/self-checks.test.mjs @@ -25,9 +25,12 @@ const resolveDoc = (doc, request) => { ...FILES, config: stringify(doc) }, { configPath: "docs/conventions/pr-pipeline.yaml", basePath: ".base", ...request }, ); -const run = (fixDocs) => { +const run = (fixDocs, { handsOnPatch = true } = {}) => { const doc = parse(read("./fixtures/valid.yaml")); Object.assign(doc.activities["fix-prose"], fixDocs); + if (!handsOnPatch) { + delete doc.activities["fix-prose"]["applied-by"]; + } return resolveDoc(doc, { lane: "pr-refine", activity: "fix-prose", @@ -60,31 +63,22 @@ for (const entry of ["scripts/x.sh; rm -rf /", "../scripts/x.sh", "scripts/x.sh }); } -// A skill with an effect other than read runs in the write runner's run job, -// which holds the lane token and id-token: write, so neither list may name a -// script that runs the checkout's test files there. -for (const entry of [ - "scripts/affected-tests.sh --run", - "scripts/run-outside-node-suites.sh", - "scripts/selection-audit.sh", - "scripts/check-guardrails-ps-differential.sh --harvest", - "scripts/run-test-file.sh", -]) { - for (const list of ["checks", "self-checks"]) { - test(`${list} naming ${JSON.stringify(entry)} on a write-effect skill fail the schema`, () => { - rejected(() => run({ [list]: [entry] })); - }); - } -} +// The read runner, which holds no write credential, runs a skill's checks, so +// a check may run the checkout's test files. +test("checks and self-checks may run the checkout's test files", () => { + const { selected } = run({ checks: ["scripts/run-test-file.sh"], "self-checks": ["scripts/affected-tests.sh --run"] }); + assert.deepEqual(selected.checks, ["scripts/run-test-file.sh"]); + assert.deepEqual(selected["self-checks"], ["scripts/affected-tests.sh --run"]); +}); -test("check-affected.sh --tests stays a check: from the base copy it skips the branch's tests", () => { +test("check-affected.sh --tests stays a check", () => { assert.deepEqual(run({ checks: ["scripts/check-affected.sh --tests"] }).selected.checks, ["scripts/check-affected.sh --tests"]); }); // Only the runner that stages edit tools stages them, so a read skill that // hands on no patch would list them for nothing. -test("self-checks with effect read and no applied-by.patch fail the schema", () => { - rejected(() => run({ effect: "read", "self-checks": ["scripts/check-x.sh"] })); +test("self-checks with no applied-by.patch fail the schema", () => { + rejected(() => run({ "self-checks": ["scripts/check-x.sh"] }, { handsOnPatch: false })); }); test("an empty or repeating self-checks list fails the schema", () => { diff --git a/.github/actions/resolve-config/verify-scripts.test.mjs b/.github/actions/resolve-config/verify-scripts.test.mjs index 72dae99ccb6..97b87428b7c 100644 --- a/.github/actions/resolve-config/verify-scripts.test.mjs +++ b/.github/actions/resolve-config/verify-scripts.test.mjs @@ -20,9 +20,12 @@ const FILES = { activityRules: json("./activity-rules.json"), }; -const run = (fixDocs) => { +const run = (fixDocs, { handsOnPatch = true } = {}) => { const doc = parse(read("./fixtures/valid.yaml")); Object.assign(doc.activities["fix-prose"], fixDocs); + if (!handsOnPatch) { + delete doc.activities["fix-prose"]["applied-by"]; + } return resolve( { ...FILES, config: stringify(doc) }, { @@ -34,9 +37,9 @@ const run = (fixDocs) => { }, ); }; -const rejected = (fixDocs) => +const rejected = (fixDocs, options) => assert.throws( - () => run(fixDocs), + () => run(fixDocs, options), (error) => { assert.ok(error instanceof Rejection, `expected a Rejection, got ${error}`); assert.equal(error.code, "invalid-config", error.message); @@ -78,8 +81,8 @@ for (const path of [ // Only the runner that stages edit tools grants them, so a read skill that // hands on no patch would list them for nothing. -test("verify-scripts with effect read and no applied-by.patch fails the schema", () => { - rejected({ effect: "read", "verify-scripts": ["scripts/check-x.sh"] }); +test("verify-scripts with no applied-by.patch fails the schema", () => { + rejected({ "verify-scripts": ["scripts/check-x.sh"] }, { handsOnPatch: false }); }); // The read runner stages them under prepare: ci-context too, as base copies the @@ -89,16 +92,12 @@ test("checks on a prepare: ci-context skill fail the schema", () => { rejected({ effect: "read", prepare: "ci-context", "applied-by": { patch: "apply-patch" }, checks: ["scripts/check-x.sh"] }); }); -// A skill with an effect other than read runs in the write runner's run job, -// which holds the lane token and id-token: write, and claude-code-action writes -// that token into the checkout's .git/config, so the schema refuses the scripts -// that run checkout code there (pr-run-activity.md, Skill activities). -for (const script of ["scripts/run-test-file.sh", "plugins/skill-quality/scripts/check-skill.sh"]) { - test(`verify-scripts listing ${script} on a write-effect skill fails the schema`, () => { - rejected({ "verify-scripts": [script] }); - rejected({ "verify-scripts": ["scripts/check-x.sh", script] }); - }); -} +// The read runner, which holds no write credential, may run scripts that run +// checkout code. +test("verify-scripts may list scripts that run checkout code", () => { + const list = ["scripts/run-test-file.sh", "plugins/skill-quality/scripts/check-skill.sh"]; + assert.deepEqual(run({ "verify-scripts": list }).selected["verify-scripts"], list); +}); test("an empty or repeating verify-scripts list fails the schema", () => { rejected({ "verify-scripts": [] }); diff --git a/.github/actions/resolve-config/web-access.test.mjs b/.github/actions/resolve-config/web-access.test.mjs index cde7297d61d..79c9cdd6cb9 100644 --- a/.github/actions/resolve-config/web-access.test.mjs +++ b/.github/actions/resolve-config/web-access.test.mjs @@ -12,9 +12,12 @@ import { Rejection, resolve } from "./resolve.mjs"; const read = (relative) => readFileSync(new URL(relative, import.meta.url), "utf8"); const json = (relative) => JSON.parse(read(relative)); -const run = (fixDocs) => { +const run = (fixDocs, { handsOnPatch = true } = {}) => { const doc = parse(read("./fixtures/valid.yaml")); Object.assign(doc.activities["fix-prose"], fixDocs); + if (!handsOnPatch) { + delete doc.activities["fix-prose"]["applied-by"]; + } return resolve( { config: stringify(doc), @@ -33,10 +36,10 @@ const run = (fixDocs) => { }, ); }; -const selected = (fixDocs) => run(fixDocs).selected; -const rejected = (fixDocs) => +const selected = (fixDocs, options) => run(fixDocs, options).selected; +const rejected = (fixDocs, options) => assert.throws( - () => run(fixDocs), + () => run(fixDocs, options), (error) => { assert.ok(error instanceof Rejection, `expected a Rejection, got ${error}`); assert.equal(error.code, "invalid-config", error.message); @@ -80,12 +83,12 @@ for (const domains of ["all", [], ["*"], ["any"], ["code.claude.com,x"], ["Code. // Only the runner that stages edit tools grants the web, so a read skill that // hands on no patch cannot ask for it (README.md, Activities, web-access). -test("web-access on with effect read and no applied-by.patch fails the schema", () => { - rejected({ effect: "read", "reads-untrusted": true, "web-access": { enabled: true, domains: "any" } }); +test("web-access on with no applied-by.patch fails the schema", () => { + rejected({ "reads-untrusted": true, "web-access": { enabled: true, domains: "any" } }, { handsOnPatch: false }); }); -test("web-access off with effect read validates", () => { - assert.doesNotThrow(() => selected({ effect: "read", "web-access": { enabled: false } })); +test("web-access off with no applied-by.patch validates", () => { + assert.doesNotThrow(() => selected({ "web-access": { enabled: false } }, { handsOnPatch: false })); }); test("web-access with an unknown key fails the schema", () => { diff --git a/.github/workflows/pr-run-activity-write.yml b/.github/workflows/pr-run-activity-write.yml index 4d228f60d9b..df1e2973f20 100644 --- a/.github/workflows/pr-run-activity-write.yml +++ b/.github/workflows/pr-run-activity-write.yml @@ -4,10 +4,9 @@ name: PR Run Activity Write # reports it as the check run ` / `. The run job gates, # resolves the base-SHA config, fails red on a `read` effect, gets the App # token the effect grants from the lanes token broker with its OIDC token -# before any head checkout, runs a skill or a script, and revokes the token; -# the report job writes the check from the run job's runner-computed outputs. -# In issue mode it runs scripts only (apply-issue-patch, report-outcome); an -# issue's skill runs in pr-run-activity-read.yml with no write token. +# before any head checkout, runs a script, and revokes the token; the report +# job writes the check from the run job's runner-computed outputs. It runs no +# skill: every skill runs in pr-run-activity-read.yml with no write token. # Neither file references the App key. Read activities run in # pr-run-activity-read.yml, whose head-code job holds no OIDC token. # Job contract: docs/conventions/pr-pipeline/pr-run-activity.md. @@ -41,7 +40,7 @@ on: issue-number: description: >- Issue mode: the issue an issues lane works, on an issues event only. - There is no PR: the trusted base is the default branch tip, a skill + There is no PR: the trusted base is the default branch tip, a script commits to a new branch, and no check run is posted. required: false type: string @@ -479,10 +478,6 @@ on: docs/conventions/pr-pipeline.yaml `funding:`. required: true type: string - secrets: - claude-code-oauth-token: - description: Claude Code OAuth token for skill activities. - required: false outputs: escalation-reason: description: >- @@ -556,23 +551,18 @@ jobs: cache-mode: read # id-token: write is only for the broker request. Any step of this job can # request an OIDC token for another audience: an accepted residual (ADR - # 0058), bounded by the broker's one mint per job. So no step runs a - # PR-head test file: the schema refuses the verify-scripts, checks and - # self-checks that run checkout tests on a write-effect skill. + # 0058), bounded by the broker's one mint per job. So no step runs PR-head + # code: the job runs only base scripts, never a skill. permissions: contents: read pull-requests: read id-token: write # The report job trusts only these, all computed by runner steps. Every # one but act-outcome, escalation-reason and api-failures is a step - # outcome or an output of a step that ran before any PR head code. act-outcome reads - # steps that run after the skill: for a gate skill the verdict step, so a - # gate skill must not execute head code; its promote and turn terms only - # turn a success into a failure. The act-result step turns a failed skill - # step into a success only when the execution file's result message is a - # success over the turn cap, a shape the CLI writes and a finished run - # could report anyway. escalation-reason comes from the base's reply, - # verdict and apply-patch scripts, which run no model and no head code. + # outcome or an output of a step that ran before any PR head code. + # act-outcome is the script step's outcome, and escalation-reason comes + # from the base's reply, verdict and apply-patch scripts, which run no + # model and no head code. outputs: base-sha: ${{ steps.trusted-base.outputs.commit }} head-sha: ${{ steps.gate.outputs.head-sha }} @@ -595,32 +585,15 @@ jobs: || steps.gate.outputs.proceed != 'true' && steps.gate.outputs.reason || steps.config.outputs.skip-reason || steps.head-recheck.outputs.skip-reason || steps.lane-token.outputs.skip-reason - || steps.prepare.outputs.skip-reason || steps.promote.outputs.skip-reason + || steps.prepare.outputs.skip-reason || steps.act-script.outputs.skip-reason || steps.patch-select.outputs.skip-reason }} # The base action's own failure text, so the report names why the run # job failed before its act step. run-failure: ${{ steps.lane-token.outputs.failure-reason }} - # The lane-outcome JSON, from steps that read the execution file after - # the skill, empty when the step wrote none; the report-check-run action - # validates it before the check run carries it. A forged limit stop can - # only restart the lane after a reset limit-stop.mjs clamps to at least - # `limits.reset-clamp-min` hours away, at most - # `limits.limit-stops-per-head` times on one head. - record: ${{ steps.lane-record.outputs.record }} - # Set after the model ran, so forgeable: post.mjs checks every row. + # Set after the script ran, so forgeable: post.mjs checks every row. api-failures: ${{ steps.api-failures.outputs.api-failures }} - # A gate skill succeeds only on a `pass` verdict, a skill that took no - # model turn fails, and a skill whose commits could not be moved onto - # the PR branch fails. A skill step that failed only on the turn cap - # counts as succeeded (act-result). The verdict step's ACT_OUTCOME repeats this - # expression: runner-policy refuses YAML aliases. - act-outcome: >- - ${{ steps.act-result.outputs.outcome == 'success' - && (steps.config.outputs.gating == 'gate' && steps.skill-verdict.outcome != 'success' - || steps.promote.outcome == 'failure' || steps.act-turns.outcome != 'success') && 'failure' - || steps.act-skill.outcome != 'skipped' && (steps.act-result.outputs.outcome || steps.act-skill.outcome) - || steps.act-script.outcome }} + act-outcome: ${{ steps.act-script.outcome }} steps: # The job if: above cannot match an unset variable, so the guard would # silently let the App's own events through. An empty default branch @@ -865,13 +838,14 @@ jobs: # review-threads and dependency-review. prepare-supported: "true" - # An issue's skill runs model-chosen commands, so it runs in - # pr-run-activity-read.yml with no write token; issue mode here runs - # only the scripts that apply and report its work. - - name: Assert an issue skill runs in the read runner - if: inputs.issue-number != '' && steps.config.outcome == 'success' && steps.config.outputs.kind == 'skill' + # A skill runs model-chosen commands, so it runs in + # pr-run-activity-read.yml with no write token (resolve-config refuses a + # skill whose effect is not read); this file runs only the scripts that + # apply, publish and report its work. + - name: Assert a skill runs in the read runner + if: steps.config.outcome == 'success' && steps.config.outputs.kind == 'skill' run: | - echo "::error::issue-skill-in-write-runner: an issue skill runs in pr-run-activity-read.yml, which holds no write token" + echo "::error::skill-in-write-runner: a skill runs in pr-run-activity-read.yml, which holds no write token" exit 1 # review-threads runs in pr-run-activity-read.yml, after its caller's @@ -1366,8 +1340,7 @@ jobs: fi echo "no-progress=$no_progress" >>"$GITHUB_OUTPUT" - # The labels the commit, promote and apply-patch steps treat as a hold, - # as a JSON array: do-not-merge, the human-gated role label resolved from the base + # The labels the apply-patch step treats as a hold, as a JSON array: do-not-merge, the human-gated role label resolved from the base # the way report-escalation.sh does, and the activity's labels-absent # list, all read before the head checkout replaces .base. The # human-gated label is left out only for apply-patch when the caller's @@ -1383,7 +1356,7 @@ jobs: id: hold-label if: >- steps.head-recheck.outputs.proceed == 'true' && steps.lane-token.outputs.token != '' - && (steps.config.outputs.kind == 'skill' || steps.config.outputs.prepare == 'apply-patch') + && steps.config.outputs.prepare == 'apply-patch' env: RESOLVED: ${{ runner.temp }}/resolved-config.json HUMAN_GATE: ${{ steps.config.outputs.prepare == 'apply-patch' && inputs.patch-human-gate || '' }} @@ -1485,54 +1458,13 @@ jobs: - name: Isolate the runner if: steps.lane-token.outputs.token != '' uses: ./.base/.github/actions/setup-runner-isolation # zizmor: ignore[self-repository] - with: - install-subprocess-isolation: ${{ steps.config.outputs.kind == 'skill' }} - - # The act-turns step's script, copied before the head checkout replaces - # .base, with its SHA-256, which that step checks its copy against - # before it loads it. The same step runs in pr-run-activity-read.yml. - - name: Copy the act-turns script from the base - id: act-turns-bin - if: steps.lane-token.outputs.token != '' && steps.config.outputs.kind == 'skill' - env: - DEST: ${{ runner.temp }}/act-turns-bin - run: | - mkdir -p "$DEST" - cp .base/.github/scripts/lib/act-turns.mjs "$DEST/" - sum=$(sha256sum <"$DEST/act-turns.mjs") - echo "sha256=${sum%% *}" >>"$GITHUB_OUTPUT" - - # Issue mode checks out the trusted base, the default branch tip, or for - # apply-patch the issue patch's start commit, never the model job's - # workspace. - # The head checkout replaces .base, so the base's scripts/setup-env.sh - # and the files it installs from are copied out first; a base that - # predates the script skips the install. - - name: Copy the dependency setup from the base - id: setup-env - if: steps.lane-token.outputs.token != '' && steps.config.outputs.kind == 'skill' - env: - DEST: ${{ runner.temp }}/setup-env - run: | - if [ ! -f .base/scripts/setup-env.sh ]; then - echo "::notice::the base has no scripts/setup-env.sh; the skill runs without its dependencies" - exit 0 - fi - policy=.github/standards/runner-policy - mkdir -p "$DEST/scripts" "$DEST/$policy" - for file in scripts/setup-env.sh .python-version .github/requirements-ci.txt \ - package.json package-lock.json "$policy/package.json" "$policy/package-lock.json"; do - cp ".base/$file" "$DEST/$file" - done - echo "ready=true" >>"$GITHUB_OUTPUT" # Logged here, before the head checkout replaces .base: no step after # head code loads an action from a checkout. The job-token calls of the # branch re-read (github-script), the broker step's compare and the # trusted base's dispatch read (gh in composite actions) go through no - # counted client, and so do the promote step's pull and label reads, - # which come later; each is named, so the count reads as a floor. The - # lanes App token's calls, most of them after head code, are not logged. + # counted client; each is named, so the count reads as a floor. The + # lanes App token's calls are not logged. - name: Log the API budget before the head checkout id: api-budget if: always() @@ -1545,10 +1477,11 @@ jobs: ${{ steps.trusted-base.outcome != 'skipped' && !contains(fromJSON('["issues","pull_request"]'), github.event_name) && 'trusted-base:pulls' || '' }} ${{ steps.head-recheck.outcome != 'skipped' && steps.config.outputs.effect == 'mutate-branch' && inputs.issue-number == '' && 'head-recheck:git/ref' || '' }} ${{ steps.lane-token.outcome != 'skipped' && inputs.issue-number == '' && github.event_name == 'pull_request' && 'lane-token:compare' || '' }} - ${{ steps.lane-token.outcome == 'success' && inputs.issue-number == '' && 'promote:pulls promote:issues/labels' || '' }} - # A `checkout: base` script activity skips it: the job holds only the - # trusted base checkout, and .base stays in place. + # Issue mode checks out the trusted base, the default branch tip, or for + # apply-patch the issue patch's start commit, never the model job's + # workspace. A `checkout: base` script activity skips it: the job holds + # only the trusted base checkout, and .base stays in place. - name: Check out the PR head if: steps.lane-token.outputs.token != '' && steps.config.outputs.checkout != 'base' uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 @@ -1619,439 +1552,6 @@ jobs: fi bash "$BIN/run-prepare.sh" - # claude-code-action's commit_files builds each commit on the live tip of - # CLAUDE_BRANCH with file bytes from this checkout - # (src/mcp/github-file-ops-server.ts:226-367 at ed670b4), so a push to - # the PR branch during the run would have its edits to the same files - # overwritten. The skill commits to a branch made at the gated SHA - # instead, and a later step moves the PR branch to it only while the PR - # branch is still at the gated SHA. - - name: Make the lane's commit branch at the gated SHA - id: lane-branch - if: >- - steps.lane-token.outputs.token != '' && steps.config.outputs.kind == 'skill' - && steps.config.outputs.can-commit == 'true' - uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 - env: - HEAD_SHA: ${{ steps.gate.outputs.head-sha }} - LANE_BRANCH: ${{ format('lane-commits/{0}/{1}/{2}-{3}', inputs.lane, inputs.activity, github.run_id, github.run_attempt) }} - with: - github-token: ${{ steps.lane-token.outputs.token }} - retries: 0 - script: | - const { HEAD_SHA, LANE_BRANCH } = process.env; - await github.rest.git.createRef({ ...context.repo, ref: `refs/heads/${LANE_BRANCH}`, sha: HEAD_SHA }); - core.setOutput('branch', LANE_BRANCH); - - - name: Set up Python for the skill - if: >- - steps.setup-env.outputs.ready == 'true' - uses: actions/setup-python@6d4fd22b20465ccc0f7354476588e8df7c9ca0e0 # v7.1.0 - with: - python-version-file: ${{ runner.temp }}/setup-env/.python-version - - # The install reads only the base copies, so no head code runs: pytest - # and the Node tools land on PATH for the base wrappers. - - name: Install the skill's dependencies - if: >- - steps.setup-env.outputs.ready == 'true' - env: - SETUP_ENV_VENV: ${{ runner.temp }}/setup-env-venv - run: | - bash "$RUNNER_TEMP/setup-env/scripts/setup-env.sh" --root "$RUNNER_TEMP/setup-env" - # Node resolves a package from the importing file's directory, so the - # base-built trees are copied into the checkout, where node_modules - # is ignored. - for dir in . .github/standards/runner-policy; do - [ -d "$GITHUB_WORKSPACE/$dir" ] || continue - rm -rf "${GITHUB_WORKSPACE:?}/$dir/node_modules" - cp -R "$RUNNER_TEMP/setup-env/$dir/node_modules" "$GITHUB_WORKSPACE/$dir/" - done - - # claude-code-action sets GH_TOKEN and GITHUB_TOKEN to the lane token in - # its own environment and hands that whole environment to Claude - # (src/entrypoints/run.ts:189-191, base-action/src/parse-sdk-options.ts:279 - # at the pinned SHA). - # The environment scrub keeps both for Bash, so checkout code a wrapper - # runs could read the token from the Bash shell's or the sandbox's pid 1 - # /proc//environ. The settings' env sets both to empty for Claude's - # own subprocesses; the action's MCP servers, commit signing among them, - # still get the token from their own config env - # (src/mcp/install-mcp-server.ts:116-148). Bash here runs only what the - # allow rules name, the wrappers among them, which unset both anyway. - - name: Keep the GitHub token out of Bash - id: skill-settings - if: >- - steps.lane-token.outputs.token != '' && steps.config.outputs.kind == 'skill' - env: - SETTINGS_JSON: ${{ steps.config.outputs.settings-json }} - run: | - settings=$SETTINGS_JSON - [ -n "$settings" ] || settings='{}' - json=$(jq -c '.env = (.env // {}) + {GH_TOKEN: "", GITHUB_TOKEN: ""}' <<<"$settings") - echo "json=$json" >>"$GITHUB_OUTPUT" - - # A failure does not stop the job here: the next step fails it unless the failure - # is only the turn cap on a successful result. - - name: Run the skill activity - id: act-skill - if: >- - steps.lane-token.outputs.token != '' && steps.config.outputs.kind == 'skill' - continue-on-error: true - uses: anthropics/claude-code-action@4fa93a88de49bbef68b10eb15aabedf28ca0e1ed # v1.0.271 - env: - # Strips the OAuth token and other credentials from Bash, hook and - # MCP subprocesses and gives Bash its own PID namespace, so /proc - # shows no ancestor's env. It keeps GH_TOKEN and GITHUB_TOKEN - # (code.claude.com/docs/en/env-vars, as of 2026-10-04; recheck when - # the CLAUDE_CODE_SUBPROCESS_ENV_SCRUB row stops listing GH_TOKEN - # and GITHUB_TOKEN as kept), which the settings below clear. - CLAUDE_CODE_SUBPROCESS_ENV_SCRUB: "1" - # The action stops reading at Claude's first result message - # (base-action/src/run-claude-sdk.ts:198-209), so background work - # that reports in a later turn is lost. This runs every subagent in - # the foreground and turns off run_in_background - # (code.claude.com/docs/en/env-vars, CLAUDE_CODE_DISABLE_BACKGROUND_TASKS - # row, as of 2026-10-15; recheck on a claude-code-action bump that - # drops that break, or when the row stops naming run_in_background). - CLAUDE_CODE_DISABLE_BACKGROUND_TASKS: "1" - CLAUDE_BRANCH: ${{ steps.lane-branch.outputs.branch || steps.gate.outputs.head-ref }} - TRUSTED_CONTEXT_FILE: >- - ${{ steps.config.outputs.reads-untrusted == 'true' && inputs.issue-number == '' - && format('{0}/trusted-context.json', runner.temp) || '' }} - with: - claude_code_oauth_token: ${{ secrets.claude-code-oauth-token }} - github_token: ${{ steps.lane-token.outputs.token }} - use_commit_signing: true - # The action refuses a bot actor unless it is listed. The trigger - # gate has already decided both bot cases: the lanes App passes it - # only by setting the app field in issue mode, and github-actions - # only on a dispatcher run it verified, whose id this names. - allowed_bots: >- - ${{ inputs.dispatcher-run-id != '' && 'melodic-automation-lanes[bot],github-actions' - || 'melodic-automation-lanes[bot]' }} - plugin_marketplaces: ${{ runner.temp }}/base-marketplace - plugins: ${{ steps.stage.outputs.plugins }} - prompt: ${{ steps.stage.outputs.prompt }} - # The model, turns and effort come from the base config's models - # profile (resolve-config), never from text the run reads, with the - # GitHub token cleared for Bash. - settings: ${{ steps.skill-settings.outputs.json }} - claude_args: >- - --setting-sources user - --permission-mode dontAsk - ${{ steps.config.outputs.claude-args }} - ${{ steps.stage.outputs.claude-args }} - - # claude-code-action fails the step on a `success` result whose - # num_turns is over --max-turns, after the CLI has finished the work - # (base-action/src/run-claude-sdk.ts:241-250 at the pinned SHA). Such a - # run counts as a success: the last result message in the execution - # file must say subtype `success`, is_error false, and num_turns over - # the cap. Any other failure, or a missing or unreadable file, fails - # the job here, as the skill step itself did before. - - name: Check the skill's failure is only the turn cap - id: act-result - if: always() && (steps.act-skill.outcome == 'success' || steps.act-skill.outcome == 'failure') - uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 - env: - ACT_OUTCOME: ${{ steps.act-skill.outcome }} - EXECUTION_FILE: >- - ${{ steps.act-skill.outputs.execution_file - || format('{0}/claude-execution-output.json', runner.temp) }} - MAX_TURNS: ${{ steps.config.outputs.max-turns }} - with: - retries: 0 - script: | - const fs = require('fs'); - const { ACT_OUTCOME, EXECUTION_FILE, MAX_TURNS } = process.env; - if (ACT_OUTCOME === 'success') { - core.setOutput('outcome', 'success'); - return; - } - let result = null; - try { - const messages = JSON.parse(fs.readFileSync(EXECUTION_FILE, 'utf8')); - result = Array.isArray(messages) ? messages.filter((m) => m?.type === 'result').at(-1) ?? null : null; - } catch (error) { - core.setFailed(`the skill step failed and its execution file is not readable: ${error.code ?? error.name}`); - return; - } - const cap = /^[1-9][0-9]*$/.test(MAX_TURNS ?? '') ? Number(MAX_TURNS) : null; - const turns = result?.num_turns; - if ( - result?.subtype === 'success' && result.is_error === false && cap !== null - && Number.isSafeInteger(turns) && turns > cap - ) { - core.notice(`the skill finished with a successful result after ${turns} turns, over the ${cap}-turn cap; counted as a success`); - core.setOutput('outcome', 'success'); - return; - } - core.setFailed( - `the skill step failed: result subtype=${JSON.stringify(result?.subtype ?? null)} ` + - `is_error=${JSON.stringify(result?.is_error ?? null)} num_turns=${JSON.stringify(turns ?? null)} max-turns=${cap ?? 'unset'}`, - ); - - # Each actions/checkout post step runs git in its checkout's path - # whenever /.git/config is there, as a node step that holds the - # job's runtime token. The skill can write the head checkout's .git, and - # the head checkout removed .base, so it could make one there too; with - # both gone, both post steps return before any git runs. No later step of - # a skill run needs git, and a script activity skips this step and keeps - # its checkout. A pinned action rather than a shell step, like the revoke - # step, since the skill could have changed PATH or BASH_ENV. A cancelled - # skill runs it too: the post steps still run after a timeout or a cancel. - - name: Remove the checkouts' .git directories - if: always() && steps.act-skill.outcome != 'skipped' - uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 - env: - WORKSPACE: ${{ github.workspace }} - with: - retries: 0 - script: | - const { rmSync } = require('fs'); - const { WORKSPACE } = process.env; - if (!WORKSPACE) { - core.setFailed('the workspace path is empty, so the checkouts keep their .git directories'); - return; - } - for (const dir of ['.git', '.base/.git']) { - rmSync(`${WORKSPACE}/${dir}`, { recursive: true, force: true }); - } - - # The reply is untrusted model text: it is read with jq and printed as - # data, never interpolated into a command. The file path falls back to - # the action's fixed location (base-action/src/execution-file.ts:6-13). - # It is also uploaded below as audit evidence, since the REST API cannot - # read step summaries; nothing in this workflow reads that artifact. - - name: Show the skill's final reply - if: always() && steps.lane-token.outputs.token != '' && (steps.act-skill.outcome == 'success' || steps.act-skill.outcome == 'failure') - env: - EXECUTION_FILE: >- - ${{ steps.act-skill.outputs.execution_file - || format('{0}/claude-execution-output.json', runner.temp) }} - REPLY_DIR: ${{ runner.temp }}/skill-reply - # The model can read the lane token, so its reply could quote it; - # the token is cut out before the reply is shown or uploaded. - LANE_TOKEN: ${{ steps.lane-token.outputs.token }} - run: | - mkdir -p "$REPLY_DIR" - if [ ! -f "$EXECUTION_FILE" ]; then - echo "No execution file to summarize" | tee "$REPLY_DIR/reply.txt" - exit 0 - fi - jq -r --arg token "$LANE_TOKEN" '[.[] | select(.type == "result")] | last | (.result // "") - | if $token == "" then . else split($token) | join("[REDACTED]") end - | .[:4000] | gsub("`"; "'"'"'")' \ - "$EXECUTION_FILE" >"$REPLY_DIR/reply.txt" - { - echo "### Skill reply" - echo - echo '```text' - cat "$REPLY_DIR/reply.txt" - echo '```' - } >>"$GITHUB_STEP_SUMMARY" - - # A slash command whose expansion fails, such as a skill pre-compute line - # the permission mode denies, ends with a `success` result and no model - # turn, so the act did nothing. This fails it and records why. The same - # step runs in pr-run-activity-read.yml, which holds no lane token. - - name: Check the skill took a model turn - id: act-turns - if: always() && steps.lane-token.outputs.token != '' && (steps.act-skill.outcome == 'success' || steps.act-skill.outcome == 'failure') - uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 - env: - EXECUTION_FILE: >- - ${{ steps.act-skill.outputs.execution_file - || format('{0}/claude-execution-output.json', runner.temp) }} - REPLY_DIR: ${{ runner.temp }}/skill-reply - LANE_TOKEN: ${{ steps.lane-token.outputs.token }} - ACT_TURNS_MODULE: ${{ runner.temp }}/act-turns-bin/act-turns.mjs - ACT_TURNS_SHA256: ${{ steps.act-turns-bin.outputs.sha256 }} - with: - retries: 0 - # .github/scripts/lib/act-turns.mjs, loaded from the bytes checked - # against the base copy's hash, with the lane token cut out of the - # recorded text and the run fields in the step summary. - script: | - const { createHash } = require('crypto'); - const { readFileSync } = require('fs'); - const { ACT_TURNS_MODULE, ACT_TURNS_SHA256, EXECUTION_FILE, REPLY_DIR, LANE_TOKEN } = process.env; - const source = readFileSync(ACT_TURNS_MODULE); - if (!ACT_TURNS_SHA256 || createHash('sha256').update(source).digest('hex') !== ACT_TURNS_SHA256) { - core.setFailed('act-turns: the staged script does not match the copy taken from the base'); - return; - } - const redact = (text) => (LANE_TOKEN ? text.split(LANE_TOKEN).join('[REDACTED]') : text); - const { runStep } = await import(`data:text/javascript;base64,${source.toString('base64')}`); - await runStep({ core, executionFile: EXECUTION_FILE, replyDir: REPLY_DIR, redact, summary: true }); - - # The lane record: the run's failure class and, on a usage limit, when it - # resets, read from the execution file's structured fields by - # ci-workflows' classifier, never from model text. The report job copies - # it onto the posted check run, and issue mode writes it into lane.json, - # where the limit-stop restarts read it. - # The secret the caller passed, named from its funding role: - # `issue-lane` reports ISSUE_LANE_CLAUDE_CODE_OAUTH_TOKEN. - - name: Name the run's credential - id: credential - if: always() && (steps.act-skill.outcome == 'success' || steps.act-skill.outcome == 'failure') - env: - ROLE: ${{ inputs.role }} - run: | - if [[ ! "$ROLE" =~ ^[a-z][a-z0-9-]*$ ]]; then - echo "::error::role must be lowercase letters, digits and hyphens" - exit 1 - fi - role=${ROLE^^} - echo "name=${role//-/_}_CLAUDE_CODE_OAUTH_TOKEN" >>"$GITHUB_OUTPUT" - - - name: Record the lane outcome - id: lane-record - if: always() && (steps.act-skill.outcome == 'success' || steps.act-skill.outcome == 'failure') - uses: melodic-software/ci-workflows/.github/actions/report-lane-outcome@6b9c640bf209c0c7e76f1111c5d18c96707348a2 # 6b9c640 2026-11-02 - with: - outcome: ${{ steps.act-result.outputs.outcome || steps.act-skill.outcome }} - execution-file: >- - ${{ steps.act-skill.outputs.execution_file - || format('{0}/claude-execution-output.json', runner.temp) }} - lane: ${{ inputs.lane }} - lane-id: ${{ inputs.lane }} - credential: ${{ steps.credential.outputs.name }} - max-turns: ${{ steps.config.outputs.max-turns }} - - - name: Scan the upload for credentials - id: scan-upload - if: >- - always() && (steps.act-skill.outcome == 'success' || steps.act-skill.outcome == 'failure') - env: - SCAN_PATHS: | - ${{ runner.temp }}/skill-reply/reply.txt - ${{ runner.temp }}/skill-reply/act-turns.json - run: node "$RUNNER_TEMP/scan-gate/.github/actions/scan-upload/scan-upload.mjs" - - - name: Upload the skill's final reply - if: >- - (always() && (steps.act-skill.outcome == 'success' || steps.act-skill.outcome == 'failure')) && steps.scan-upload.outputs.clean == 'true' - uses: actions/upload-artifact@cf430e030ddbb5b0abf93d22962f4752f3646cd9 # v7.0.2 - with: - name: skill-reply-${{ inputs.lane }}-${{ inputs.activity }}-${{ github.run_attempt }} - # Named files only: head code can write into this directory. - path: | - ${{ runner.temp }}/skill-reply/reply.txt - ${{ runner.temp }}/skill-reply/act-turns.json - retention-days: 3 - if-no-files-found: error - - # The verdict comes from the action's structured_output, read through env - # and parsed by jq. Anything but exactly `pass` fails the activity; the - # summary is model text, printed as data. - - name: Check the gate skill's verdict - id: skill-verdict - if: always() && steps.act-result.outputs.outcome == 'success' && steps.config.outputs.gating == 'gate' - env: - STRUCTURED_OUTPUT: ${{ steps.act-skill.outputs.structured_output }} - run: | - verdict=$(jq -r 'if type == "object" and (.verdict | type) == "string" then .verdict else "" end' <<<"$STRUCTURED_OUTPUT") || verdict="" - summary=$(jq -r 'if type == "object" then (.summary // "" | tostring)[:4000] | gsub("`"; "'"'"'") else "" end' <<<"$STRUCTURED_OUTPUT") || summary="" - { - echo "### Gate verdict" - echo - echo '```text' - printf 'verdict: %s\n\n%s\n' "$verdict" "$summary" - echo '```' - } >>"$GITHUB_STEP_SUMMARY" - if [ "$verdict" != pass ]; then - echo "::error::the gate skill's verdict is not pass" - exit 1 - fi - - # Fast-forwards the PR branch to the lane's commits only while it is - # still at the gated SHA; otherwise the commits are dropped and the run - # the new head started redoes the activity. A do-not-merge or human-gated - # label, or a draft, set during the run drops them (awaiting-human). A - # pinned action, like the revoke step, since the activity could have - # changed PATH. The lane branch is deleted either way. - - name: Move the PR branch to the lane's commits - id: promote - if: always() && steps.lane-token.outputs.token != '' && steps.lane-branch.outputs.branch != '' && inputs.issue-number == '' - uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 - env: - HEAD_SHA: ${{ steps.gate.outputs.head-sha }} - HEAD_REF: ${{ steps.gate.outputs.head-ref }} - LANE_BRANCH: ${{ steps.lane-branch.outputs.branch }} - PR_NUMBER: ${{ steps.gate.outputs.pr-number }} - HOLD_LABELS: ${{ steps.hold-label.outputs.labels }} - READ_TOKEN: ${{ github.token }} - with: - github-token: ${{ steps.lane-token.outputs.token }} - retries: 0 - script: | - const { HEAD_SHA, HEAD_REF, LANE_BRANCH } = process.env; - const { PR_NUMBER, HOLD_LABELS, READ_TOKEN } = process.env; - core.setOutput('skip-reason', ''); - const tip = async (branch) => - (await github.request('GET /repos/{owner}/{repo}/git/ref/{ref}', { ...context.repo, ref: `heads/${branch}` })) - .data.object.sha; - try { - const lane = await tip(LANE_BRANCH); - if (lane === HEAD_SHA) { - core.info('The activity made no commits'); - return; - } - // A hold or a draft set while the model worked stops the push: - // the config decided the hold - // predicates only at the start of the job. Read fresh with the - // job's read-only token through fetch, since github-script gives - // the script no second client. - if (!HOLD_LABELS) { - core.setFailed('No hold labels were read from the base; the PR branch is not moved'); - return; - } - const holds = new Set(['do-not-merge', ...JSON.parse(HOLD_LABELS)]); - const read = async (path) => { - const response = await fetch(`${process.env.GITHUB_API_URL}/repos/${context.repo.owner}/${context.repo.repo}/${path}`, { - headers: { Accept: 'application/vnd.github+json', Authorization: `Bearer ${READ_TOKEN}`, 'X-GitHub-Api-Version': '2022-11-28' }, - }); - if (!response.ok) { - throw new Error(`Reading ${path} answered ${response.status}; the PR branch is not moved`); - } - // More labels than one page holds could hide a hold. - if (/rel="next"/.test(response.headers.get('link') ?? '')) { - throw new Error(`${path} has more than one page; the PR branch is not moved`); - } - return response.json(); - }; - const pull = await read(`pulls/${Number(PR_NUMBER)}`); - const labels = await read(`issues/${Number(PR_NUMBER)}/labels?per_page=100`); - const held = labels.map((label) => label.name).filter((name) => holds.has(name)); - if (held.length || pull.draft !== false) { - core.info(`The PR is ${held.length ? `held (${held.join(', ')})` : 'a draft'}; the lane's commits (${lane}) are dropped`); - core.setOutput('skip-reason', 'awaiting-human'); - return; - } - // The last read before the move, to keep its window short. - const head = await tip(HEAD_REF); - if (head !== HEAD_SHA) { - core.setFailed(`The PR branch moved from ${HEAD_SHA} to ${head} during the run, so the lane's commits (${lane}) are not applied; the run for the new head redoes the activity`); - return; - } - // force: false refuses anything but a fast-forward. - await github.rest.git.updateRef({ ...context.repo, ref: `heads/${HEAD_REF}`, sha: lane, force: false }); - // The push is confirmed from git/ref, which GET /pulls can lag. - const moved = await tip(HEAD_REF); - if (moved !== lane) { - core.setFailed(`The PR branch reads ${moved} after the update to ${lane}`); - return; - } - core.info(`Moved ${HEAD_REF} from ${HEAD_SHA} to ${lane}`); - } finally { - // A failed cleanup must not fail an activity whose commits landed. - await github.rest.git - .deleteRef({ ...context.repo, ref: `heads/${LANE_BRANCH}` }) - .catch((error) => core.warning(`Could not delete ${LANE_BRANCH}: ${error.message}`)); - } - # Issue mode, script activity: what the resolve-issue run left. A # missing artifact leaves the directory empty, which reads as stopped. - name: Download the issue outcome @@ -2330,15 +1830,10 @@ jobs: SKIP_REASON: >- ${{ steps.config.outputs.skip-reason || steps.head-recheck.outputs.skip-reason || steps.lane-token.outputs.skip-reason - || steps.prepare.outputs.skip-reason || steps.promote.outputs.skip-reason + || steps.prepare.outputs.skip-reason || steps.act-script.outputs.skip-reason || steps.patch-select.outputs.skip-reason }} - ACT_OUTCOME: >- - ${{ steps.act-result.outputs.outcome == 'success' - && (steps.config.outputs.gating == 'gate' && steps.skill-verdict.outcome != 'success' - || steps.promote.outcome == 'failure' || steps.act-turns.outcome != 'success') && 'failure' - || steps.act-skill.outcome != 'skipped' && (steps.act-result.outputs.outcome || steps.act-skill.outcome) - || steps.act-script.outcome }} + ACT_OUTCOME: ${{ steps.act-script.outcome }} VERDICT_DIR: ${{ runner.temp }}/verdict run: | mkdir -p "$VERDICT_DIR" @@ -2651,7 +2146,6 @@ jobs: can-commit: ${{ needs.run.outputs.can-commit }} all-verified: ${{ steps.signed.outputs.all-verified }} unverified: ${{ steps.signed.outputs.unverified }} - lane-outcome: ${{ needs.run.outputs.record }} verdict-path: ${{ runner.temp }}/verdict/verdict.json base-path: .base head-sha-fallback: ${{ github.event.pull_request.head.sha || github.sha }} diff --git a/docs/conventions/pr-pipeline/CHANGELOG.md b/docs/conventions/pr-pipeline/CHANGELOG.md index c40a4ac069c..ab9092b1bcd 100644 --- a/docs/conventions/pr-pipeline/CHANGELOG.md +++ b/docs/conventions/pr-pipeline/CHANGELOG.md @@ -1,5 +1,17 @@ # Changelog for the PR pipeline convention +## [9.21.0] - 2026-11-11 + +### Changed + +- A skill activity's `effect` must be `read`. The schema and resolve-config (rejection 17, `skill-not-read`) refuse any other effect at config load, in any lane, so every skill runs in `pr-run-activity-read.yml` and hands its change to a script activity. Every skill in `pr-pipeline.yaml` already has effect `read`. +- `pr-run-activity-write.yml` runs scripts only and fails red with `skill-in-write-runner` before the mint for any skill, not just an issue skill. + +### Removed + +- The write runner's skill path, which no config could reach: the claude-code-action step, its lane commit branch and promote step, the turn-cap, model-turn and gate-verdict checks, the skill reply upload, the lane-outcome record, and the unused `claude-code-oauth-token` secret. The write runner's report no longer carries a `lane-outcome`. +- The schema's head-test-check limits on a skill whose effect is not `read`, which no skill can now have. + ## [9.20.0] - 2026-11-11 ### Added diff --git a/docs/conventions/pr-pipeline/README.md b/docs/conventions/pr-pipeline/README.md index c2ab5c7468d..9c9f7b3ba9b 100644 --- a/docs/conventions/pr-pipeline/README.md +++ b/docs/conventions/pr-pipeline/README.md @@ -676,10 +676,11 @@ Every lane: runs per item: each comment, review reply and linked issue is checked by its own author, not by the PR's. Web pages and CI logs are untrusted wherever they came from and stay data. The trusted-actor list is one central standards component that every lane reads. -- Runs Claude Code with `--permission-mode dontAsk` and the tools its skill needs to do its job: a - write-effect skill can change, commit and push any file in the repository with its token - ([`pr-run-activity.md`](pr-run-activity.md#skill-activities)). It gets the write access its job - needs through a short-lived, job-scoped App token, revoked when the job ends. Local lanes keep +- Runs Claude Code with `--permission-mode dontAsk` and the tools its skill needs to do its job, + always in the read runner with no GitHub token: a skill's effect is `read`, and the config + reader refuses any other. Its change reaches the branch only through a script activity + ([`pr-run-activity.md`](pr-run-activity.md#skill-activities)), which gets the write access its + job needs through a short-lived, job-scoped App token, revoked when the job ends. Local lanes keep auto mode (AGENTS.md). Capability is never removed for safety: a lane's scope comes from how granular its activity is, the PR is the review step, and safety comes from who can trigger a lane, token lifetime and scope, workflow execution protections, the kill switch, and a @@ -756,12 +757,12 @@ Each leaf declares: `read`, or `applied-by.patch`"; the effect half goes once the last skill leaves `pr-run-activity-write.yml`. - `checks` (optional, `skill` only): repository check scripts, each `scripts/.sh` with its - flags fixed, that the skill may run. The runner that gives the skill its edit tools (the write - runner, or the read runner for a skill that hands on a patch) runs each from the base copy + flags fixed, that the skill may run. The read runner gives a skill that hands on a patch its edit + tools and runs each from the base copy against the checkout, so no head code runs, and tells the skill to fix what a check reports before it reports done ([`pr-run-activity.md`](pr-run-activity.md#skill-activities)). The schema refuses `checks`, - `self-checks`, `verify-scripts` and `web-access` turned on for any other skill (effect `read` + `self-checks`, `verify-scripts` and `web-access` turned on for any other skill (one without `applied-by.patch`, or with `prepare: ci-context`), where they would do nothing, except `verify-scripts` under `prepare: ci-context`, which the read runner stages as a base copy the skill is told to run in place of the checkout's; its Bash, Edit and Write stay open, so review @@ -769,25 +770,20 @@ Each leaf declares: - `self-checks` (optional, `skill` only): the same shape as `checks`, run and fixed by the skill through the same wrapper before it commits or hands its change on. Advisory: unlike `checks`, no runner job reruns them, so a failure the skill cannot fix still opens the PR and is named in its - report. A script in both lists fails red. On a skill whose effect is not `read`, whose run job - holds the lane token and `id-token: write`, neither list may name a script that runs the - checkout's test files (`affected-tests.sh`, `run-outside-node-suites.sh`, `selection-audit.sh`, - `check-guardrails-ps-differential.sh`, `run-test-file.sh`); `check-affected.sh --tests` stays - allowed, since from the base copy it skips the branch's tests. + report. A script in both lists fails red. - `verify-scripts` (optional, `skill` only): exact repository-relative paths of scripts, each `scripts/.sh`, `plugins//scripts/.sh` or `plugins//skills//scripts/.sh`, that the skill may run with any arguments but a path outside the checkout. The runner that gives the skill its edit tools runs each from a read-only base copy of its plugin (or of `scripts/`) against the checkout, with `GH_TOKEN` and `GITHUB_TOKEN` unset. No Bash process above it holds - the token either: the write runner's settings clear it, and the read runner's run-claude-cli steps set - neither token ([`pr-run-activity.md`](pr-run-activity.md#skill-activities)). The arguments are the model's, so + the token either: the read runner's run-claude-cli steps set neither token ([`pr-run-activity.md`](pr-run-activity.md#skill-activities)). The arguments are the model's, so a script that changes files, runs code from the checkout or reaches the network hands the run that power: list one only when it limits itself, as `scripts/run-test-file.sh` (one test file inside the checkout), `scripts/git-mv-rm.sh` (`git mv` and `git rm` inside the checkout) and `scripts/new-changelog-fragment.sh` (new files under the checkout's `.changes/` only) do, or when the run may hold that power, as `plugins/skill-quality/scripts/check-skill.sh` (each checked skill's `scripts/*.test.sh` and the checkout's markdownlint) does. Those two run checkout - code, so the schema refuses them on a skill whose effect is not `read`: its run job holds the - lane token and `id-token: write`. + code, which the read runner's run job may, since it holds neither the lane token nor + `id-token: write`. - `web-access` (optional, `skill` only, off when absent): `enabled` and `domains`, either `any` or a list of host names (`*.` allowed as a prefix). When on, the skill may search the web, fetch those domains, and run the research skills, wherever the runner gives it its edit tools @@ -814,15 +810,15 @@ Each leaf declares: token broker grants to work-unblock's `publish-lane-level` alone (owner decision Q113 (a)). The runner derives ordering from it, takes the token grant from [`effect-grants.json`](../../../.github/actions/resolve-config/effect-grants.json), and fails a - `read` activity that leaves the working tree dirty unless it hands its change on as a patch. An - activity that runs head code (tests, linters, builds) takes effect `read`, because a - write-effect activity must not execute head code. To change the branch it hands its change on + `read` activity that leaves the working tree dirty unless it hands its change on as a patch. A + skill's effect is always `read`: the schema and the config reader refuse any other, so no model + runs beside the lane token. An activity that runs head code (tests, linters, builds) takes effect + `read`, because a write-effect activity must not execute head code. To change the branch it hands its change on to a `prepare: apply-patch` script with no model and no head code, named in its `applied-by.patch`, as fix-docs and fix-ci do with `apply-patch`. - `gating`: `gate` feeds `ci-status`; `advisory` is reported only. `ci-status` stays the only - required check. A `gate` skill activity must not execute head code, since its verdict is read - in the same job after the skill ran; tests, linters and builds that gate run as a `script` - activity or under a separate design. + required check. Tests, linters and builds that gate run as a `script` activity or under a + separate design. - `reads-untrusted`: whether it reads issue, PR, comment, web or CI-log text, or PR head files, which can quote such text. Ingested text is data, never instructions ([`untrusted-content`](../../contracts/untrusted-content/README.md)). diff --git a/docs/conventions/pr-pipeline/examples/claude-code-plugins.yaml b/docs/conventions/pr-pipeline/examples/claude-code-plugins.yaml index f776f018cd6..0d1ec3312d5 100644 --- a/docs/conventions/pr-pipeline/examples/claude-code-plugins.yaml +++ b/docs/conventions/pr-pipeline/examples/claude-code-plugins.yaml @@ -5,7 +5,9 @@ activities: simplify: skill: code-tidying:batch-simplify args: --unattended # a lane run has nobody to answer an ask - effect: mutate-branch + effect: read # a skill never holds a write token + applied-by: + patch: apply-patch gating: advisory reads-untrusted: false inputs: [base-sha, head-sha, changed-paths] @@ -41,13 +43,13 @@ activities: inputs: [head-sha, changed-paths] claude: skill: review:code-review - effect: publish-artifact + effect: read gating: gate reads-untrusted: true inputs: [pr, base-sha, head-sha] review-security: skill: review:security-review - effect: publish-artifact + effect: read gating: gate reads-untrusted: true inputs: [pr, base-sha, head-sha] @@ -60,7 +62,7 @@ activities: inputs: [pr, issue, head-sha] explain: skill: review:explain-change - effect: publish-artifact + effect: read gating: advisory reads-untrusted: true inputs: [pr, base-sha, head-sha] @@ -115,7 +117,7 @@ activities: inputs: [head-sha] sweep-comments: skill: review:fanout - effect: mutate-tracker + effect: read gating: advisory reads-untrusted: true inputs: [pr] @@ -125,6 +127,8 @@ lanes: stage: refine slots: - activity: simplify + - activity: apply-patch + needs: [simplify] pr-run-checks: stage: verify slots: diff --git a/docs/conventions/pr-pipeline/pr-pipeline.schema.json b/docs/conventions/pr-pipeline/pr-pipeline.schema.json index 781c60b7be1..2106888913e 100644 --- a/docs/conventions/pr-pipeline/pr-pipeline.schema.json +++ b/docs/conventions/pr-pipeline/pr-pipeline.schema.json @@ -709,17 +709,12 @@ } }, "edit-runner": { - "anyOf": [ - { "properties": { "effect": { "not": { "const": "read" } } } }, - { - "required": ["applied-by"], - "properties": { - "applied-by": { "required": ["patch"] }, - "prepare": { "not": { "const": "ci-context" } } - } - } - ], - "description": "Only the runner that stages a skill's edit tools grants checks, self-checks, verify-scripts and web-access: the write runner, or the read runner for a skill with applied-by.patch and no prepare: ci-context. Anywhere else they would do nothing, so they are refused." + "required": ["applied-by"], + "properties": { + "applied-by": { "required": ["patch"] }, + "prepare": { "not": { "const": "ci-context" } } + }, + "description": "Only the read runner stages a skill's edit tools, for a skill with applied-by.patch and no prepare: ci-context, so only that skill takes checks, self-checks, verify-scripts and web-access. Anywhere else they would do nothing, so they are refused." }, "verify-runner": { "anyOf": [ @@ -728,10 +723,6 @@ ], "description": "verify-scripts are staged where checks are, and under prepare: ci-context too, where the skill is told to run a listed script only from its base copy." }, - "no-head-test-check": { - "not": { "pattern": "^scripts/(affected-tests|run-outside-node-suites|selection-audit|check-guardrails-ps-differential|run-test-file)\\.sh( |$)" }, - "description": "A check or self-check of a skill whose effect is not read runs in the write runner's run job, which holds the lane token and id-token: write, so it may not be a script that runs the checkout's test files. check-affected.sh --tests is allowed: run from the base copy against the checkout, it skips the branch's tests." - }, "leaf": { "type": "object", "additionalProperties": false, @@ -782,16 +773,9 @@ }, "allOf": [ { - "if": { "required": ["effect"], "properties": { "effect": { "not": { "const": "read" } } } }, - "then": { - "properties": { - "checks": { "items": { "$ref": "#/$defs/no-head-test-check" } }, - "self-checks": { "items": { "$ref": "#/$defs/no-head-test-check" } }, - "verify-scripts": { - "items": { "not": { "enum": ["scripts/run-test-file.sh", "plugins/skill-quality/scripts/check-skill.sh"] } } - } - } - } + "if": { "required": ["skill"] }, + "then": { "properties": { "effect": { "const": "read" } } }, + "description": "A skill runs only in the read runner, which holds no write credential: its effect is read, and a script activity applies or publishes what it hands on." }, { "if": { "required": ["effect", "prepare"], "properties": { "effect": { "const": "read" }, "prepare": { "enum": ["ci-context", "review-threads", "merge-base"] } } }, @@ -824,7 +808,7 @@ "skill": { "$ref": "#/$defs/skill" }, "prepare": { "enum": ["merge-base", "review-threads", "dependency-review", "ci-context", "apply-patch"], - "description": "What the runner does around the activity (README.md, Activities; pr-run-activity.md, the prepare sections). apply-patch is the one mode for a script, a mutate-branch one; the others run around a skill. ci-context needs effect read and hands its change on as a patch; merge-base, review-threads and dependency-review need an effect other than read unless their activity hands its change on as a patch, review-threads with effect read also hands on its replies (applied-by.feedback-replies), and dependency-review with effect read its verdict (applied-by.dependency-verdict). ci-context: download the failing-check context file a base job wrote, check its SHA-256 and head, make it read-only and name it to the skill, which also gets Bash, Edit and Write. apply-patch: check the patch artifact a producer left (one patch.diff with its SHA-256, on the head it worked on), apply it to the gated head, and push it as one commit without force; patch-rules.json registers the rules each producer's patch is to be held to (README.md, Patch rules). merge-base: merge the base tip without committing before the skill, then commit with the gated head and the base tip as parents (with effect read, a base job of the caller merges first and decides, the read runner merges the same base again and hands the skill's resolutions on as a patch measured from the conflict tree, and a prepare: apply-patch script commits the merge). review-threads: before any model turn, keep the PR's unresolved reviewer-bot threads on a reviewed head and escalate on a .github/** diff or the round cap (on the read runner, a base job of the caller does this and the runner checks its thread file); give the skill the review-thread tool; after it, upload the replies it recorded for a script job to validate and post. dependency-review: before any model turn, skip a PR Dependabot did not open or one sorted auto, and escalate on the round cap (on the read runner, a base job of the caller does this and the runner checks its context file); give the skill a read-only context file and a verdict schema for its structured output; after it, upload the verdict for a script job to validate and act on, and the fix as a patch a prepare: apply-patch script pushes only on a fix verdict." + "description": "What the runner does around the activity (README.md, Activities; pr-run-activity.md, the prepare sections). apply-patch is the one mode for a script, a mutate-branch one; the others run around a skill. A skill's mode hands its change on as a patch; review-threads also hands on its replies (applied-by.feedback-replies), and dependency-review its verdict (applied-by.dependency-verdict). ci-context: download the failing-check context file a base job wrote, check its SHA-256 and head, make it read-only and name it to the skill, which also gets Bash, Edit and Write. apply-patch: check the patch artifact a producer left (one patch.diff with its SHA-256, on the head it worked on), apply it to the gated head, and push it as one commit without force; patch-rules.json registers the rules each producer's patch is to be held to (README.md, Patch rules). merge-base: a base job of the caller merges the base tip first and decides, the read runner merges the same base again without committing and hands the skill's resolutions on as a patch measured from the conflict tree, and a prepare: apply-patch script commits the merge with the gated head and the base tip as parents. review-threads: before any model turn, keep the PR's unresolved reviewer-bot threads on a reviewed head and escalate on a .github/** diff or the round cap (on the read runner, a base job of the caller does this and the runner checks its thread file); give the skill the review-thread tool; after it, upload the replies it recorded for a script job to validate and post. dependency-review: before any model turn, skip a PR Dependabot did not open or one sorted auto, and escalate on the round cap (on the read runner, a base job of the caller does this and the runner checks its context file); give the skill a read-only context file and a verdict schema for its structured output; after it, upload the verdict for a script job to validate and act on, and the fix as a patch a prepare: apply-patch script pushes only on a fix verdict." }, "applied-by": { "type": "object", @@ -846,7 +830,7 @@ "type": "string", "pattern": "^scripts/[A-Za-z0-9_-]+\\.sh( --[a-z][a-z-]*)*$" }, - "description": "Repository check scripts the skill may run, each with its fixed flags. The runner that stages the skill's edit tools (the write runner, or the read runner for a skill with applied-by.patch) runs each from the base copy against the checkout, through a wrapper the skill can call with no arguments." + "description": "Repository check scripts the skill may run, each with its fixed flags. The runner that stages the skill's edit tools (the read runner, for a skill with applied-by.patch) runs each from the base copy against the checkout, through a wrapper the skill can call with no arguments." }, "self-checks": { "type": "array", @@ -866,7 +850,7 @@ "type": "string", "pattern": "^(scripts|plugins/[a-z0-9-]+/(skills/[a-z0-9-]+/)?scripts)/[A-Za-z0-9_-]+\\.sh$" }, - "description": "Exact repository-relative paths of scripts the skill may run with any arguments. The runner that stages the skill's edit tools copies each script's plugin (or the base scripts/) to a read-only directory, denies Edit and Write on it, and allows a wrapper that runs the script from the checkout, with GH_TOKEN and GITHUB_TOKEN unset (no ancestor process holds them either, though the write runner's checkout keeps the token in .git/config), with any arguments but a path outside it. A script that changes files, runs code from the checkout, or reaches the network hands the run that power: list one only when it limits itself, as scripts/run-test-file.sh, scripts/git-mv-rm.sh and scripts/new-changelog-fragment.sh do, or when the run may hold that power, as plugins/skill-quality/scripts/check-skill.sh (checkout test scripts and markdownlint) does. Those two run checkout code, so a skill whose effect is not read, whose run job holds the lane token and id-token: write, may list neither." + "description": "Exact repository-relative paths of scripts the skill may run with any arguments. The runner that stages the skill's edit tools copies each script's plugin (or the base scripts/) to a read-only directory, denies Edit and Write on it, and allows a wrapper that runs the script from the checkout, with GH_TOKEN and GITHUB_TOKEN unset (no ancestor process holds them either), with any arguments but a path outside it. A script that changes files, runs code from the checkout, or reaches the network hands the run that power: list one only when it limits itself, as scripts/run-test-file.sh, scripts/git-mv-rm.sh and scripts/new-changelog-fragment.sh do, or when the run may hold that power, as plugins/skill-quality/scripts/check-skill.sh (checkout test scripts and markdownlint) does." }, "web-access": { "type": "object", @@ -959,7 +943,7 @@ "publish-artifact", "merge" ], - "description": "What the activity may change. The runner derives ordering and the token grant from it and checks that a read activity left the tree clean, unless the activity hands its change on as a patch. mutate-variable reads everything and writes only what its lane's activity grant adds: repository Variables, for work-unblock's publish-lane-level alone." + "description": "What the activity may change; a skill's is read. The runner derives ordering and the token grant from it and checks that a read activity left the tree clean, unless the activity hands its change on as a patch. mutate-variable reads everything and writes only what its lane's activity grant adds: repository Variables, for work-unblock's publish-lane-level alone." }, "gating": { "enum": ["gate", "advisory"], diff --git a/docs/conventions/pr-pipeline/pr-run-activity.md b/docs/conventions/pr-pipeline/pr-run-activity.md index 09fbaed2e70..2813c8bc87b 100644 --- a/docs/conventions/pr-pipeline/pr-run-activity.md +++ b/docs/conventions/pr-pipeline/pr-run-activity.md @@ -10,8 +10,10 @@ Two reusable workflows run one activity of one lane each and end in one check ru before the head checkout for any other effect. - [`pr-run-activity-write.yml`](../../../.github/workflows/pr-run-activity-write.yml) runs every other effect. It declares no App key either: its run job holds `id-token: write` and gets the - effect's token from the lanes token broker before any head checkout. It never runs for the lanes - App bot, and fails red with `effect-read` for a `read` activity. + effect's token from the lanes token broker before any head checkout. It runs scripts only: a + skill's effect is always `read` (the config reader refuses any other), and the file fails red + with `skill-in-write-runner` before the mint for a skill. It never runs for the lanes App bot, + and fails red with `effect-read` for a `read` activity. Every lane workflow calls one of them once per activity; the activity's effect decides which. Which activity runs, with what model, turn budget and token grant, comes from the base SHA's @@ -133,8 +135,8 @@ retries under `limits.stop-retries`, and `scan-refused-output` holds for a person ahead of any funding, limit or turn-cap record. A failed download there names no refusal. -Secrets are passed by name, never `inherit`: `claude-code-oauth-token` (skill activities, both -files) and, in the read file, `claude-api-key` (a PR-mode skill's first call), one of the two +Secrets are passed by name, never `inherit`: `claude-code-oauth-token` (skill activities, the read +file only) and, in the read file, `claude-api-key` (a PR-mode skill's first call), one of the two per call. Variables: `CLAUDE_LANES_DISABLED` and `CLAUDE_LANES_REPO_DISABLED` (the org and repository kill switches), `AUTOMATION_LANES_APP_SENDER_ID`, the App bot's numeric user id, the one place it is written, and @@ -378,13 +380,8 @@ empty; `head-sha` and `head-ref` are the trigger gate's; `base-sha` is set only is always on the default branch. `escalation-reason` comes from the base's reply, verdict and apply-patch scripts, which run no model and no head code. `act-outcome` is the activity step's outcome. In the read file a skill runs on run-claude-cli (`act-key` or `act-token`), and one -whose step succeeded is a failure when it took no model turn. In the write file a skill step -claude-code-action failed only because a `success` result went over `--max-turns` counts as -succeeded. That holds only when the execution file's last result message has subtype `success`, -`is_error` false and `num_turns` over the cap; any other failure, or a missing or unreadable -execution file, still fails the job. There a skill whose step succeeded is a failure when it took -no model turn, when it is a gate skill and the verdict check (below) fails, or when its commits -could not be moved onto the PR branch. These checks are steps that run after the skill. +whose step succeeded is a failure when it took no model turn. The write file runs no skill, so its +`act-outcome` is the script step's outcome. Both files also output the lane record of a skill run: `record`, the lane-outcome JSON (ci-workflows `docs/lane-outcome.md`) that ci-workflows' `report-lane-outcome` builds from the @@ -422,8 +419,8 @@ mints nothing. `collect-base-activity` fails red on a path outside that name set the slash command reaches the skill's arguments. A `reads-untrusted` skill must read PR text (title, body, comments, reviews, linked issues) only from that file, never through the API. Existing skills are not yet adapted to this and still read PR text themselves; until each is, its untrusted-text exposure is a -known residual. The skill gets the App token as `github_token` in the write file; the read file -runs the skill on run-claude-cli, whose steps pass it no GitHub token. `claude_args` passes `--setting-sources user`, so no +known residual. Every skill runs in the read file, on run-claude-cli, whose steps pass it no +GitHub token; the write file runs no skill. `claude_args` passes `--setting-sources user`, so no project or local settings, hooks, `CLAUDE.md`, `AGENTS.md` or `.mcp.json` from the PR head load; `--permission-mode dontAsk`; `--allowedTools "Skill(:)"`, so the skill's own `allowed-tools` decide what else it may use; and `resolve-config`'s `claude-args` for the @@ -471,9 +468,8 @@ read-only, denies `Edit` and `Write` on it, allows `Bash(/bin/ *)`, wrapper in `--append-system-prompt`. The wrapper changes to the checkout, refuses any argument (or the value after an `=`) naming an existing path whose real path lies outside the checkout, since a script may print lines of a file it reads, unsets `GH_TOKEN` and `GITHUB_TOKEN` in the script's -own environment (no Bash process above it holds the token either: in the write runner the skill's -settings clear it, and the read runner's run-claude-cli steps set neither `GH_TOKEN` nor -`GITHUB_TOKEN`), sets `GIT_CONFIG_NOSYSTEM`, `GIT_CONFIG_GLOBAL=/dev/null` +own environment (no Bash process above it holds the token either: the read runner's +run-claude-cli steps set neither `GH_TOKEN` nor `GITHUB_TOKEN`), sets `GIT_CONFIG_NOSYSTEM`, `GIT_CONFIG_GLOBAL=/dev/null` and command-scope `core.fsmonitor=false` and `core.hooksPath=/dev/null`, for skill-quality's `check-skill.sh` alone sets `HOME` to a fresh `mktemp -d` directory and `CLAUDE_CONFIG_DIR` to its `.claude`, since the skill tests it runs write there and the runner's `~/.claude` is read-only, @@ -503,12 +499,9 @@ running `npm ci --ignore-scripts` there once when it has a `package-lock.json` a without `GH_TOKEN` or `GITHUB_TOKEN`, with git reading a fixed test config in place of the host's (`scripts/lib/test-git-env.sh`). It refuses any other file and any name outside `[A-Za-z0-9._/-]`, so pytest reads no node id. When it is listed, the system prompt names its -wrapper as the way to run a test file and says a compound `cd ; ` is refused. Its tests are checkout code with the network open, so the schema -refuses `scripts/run-test-file.sh` and `check-skill.sh` in the `verify-scripts` of a skill whose -effect is not `read`: the write runner's run job holds the lane token and `id-token: write`, and -only a read skill with `applied-by.patch`, in the read runner's run job that holds neither, may -list them. For the same reason the schema refuses, in such a skill's `checks` and `self-checks`, -the scripts that run the checkout's test files ([README.md](README.md#activities)). `scripts/git-mv-rm.sh` runs +wrapper as the way to run a test file and says a compound `cd ; ` is refused. Its tests are checkout code with the network open, so +only a skill in the read runner's run job, which holds neither the lane token nor `id-token: write`, +may list them; every skill runs there ([README.md](README.md#activities)). `scripts/git-mv-rm.sh` runs `git mv ` or `git rm -r -f ...`, first recording an untracked path as intent-to-add so a file the run created can be renamed or removed, with `GIT_LITERAL_PATHSPECS=1` so git reads each path literally, never as a glob or pathspec magic. The index is local: the @@ -556,61 +549,10 @@ environment and gives Bash its own PID namespace; it does not sandbox the networ ([env vars: What the subprocess environment scrub removes](https://code.claude.com/docs/en/env-vars#what-the-subprocess-environment-scrub-removes); as of 2026-10-15). Without `web-access` the skill has no web tool and no `Bash` rule that fetches, so `dontAsk` refuses every fetch. Fetched pages are -untrusted text, so the activity must set `reads-untrusted: true`. No rule limits which files it changes: with its -token it can change, commit and push any file in the repository, and claude-code-action writes -the token into the origin URL under `use_commit_signing` -(`src/github/operations/git-config.ts:129-133` at `ed670b4`), so a `git push` works too. The PR is -the review step: the review lanes, `ci-status`, review-thread resolution and the merge gate see -every lane commit. The hardening is the broker's scoped, hour-long token revoked when the job -ends, base-SHA config and runner, the trusted-actor filter, the kill switch, -`--setting-sources user`, an explicit `--permission-mode` and never `bypassPermissions`. The -residuals are in [Trust-root paths](README.md#trust-root-paths): the model can read its token -(`GH_TOKEN`, `GITHUB_TOKEN`, `.git/config`) until it is revoked, and under `web-access` send it to -any domain the activity allows, which is -[ADR 0055](../../adr/0055-load-nothing-head-controlled-into-a-pipeline-skill-activity.md)'s -accepted residual, a skill's own `Bash` grant can exceed its job (`Bash(git:*)` in -`ai-slop:audit`), and a lane commit can change instruction files the PR author's local session -later loads. The skill's final reply is uploaded as an artifact with the token string cut out. -`commit_files` builds each commit on the live tip of `CLAUDE_BRANCH` with file bytes from the -gated checkout (`src/mcp/github-file-ops-server.ts:226-367` at `ed670b4`), so committing straight -to the PR branch would overwrite a push made during the run. After the skill, a pinned -`github-script` step fast-forwards the PR branch to the lane branch only while the PR branch is -still at the gated SHA, and deletes the lane branch either way. When the PR branch has moved, the -lane's commits are dropped and the activity fails; a later run (a reopen or a dispatch) gates the -new head and redoes the activity. Right before the move it -re-reads the PR's labels and draft flag with the job's `GITHUB_TOKEN`, then reads the PR branch's -tip last: `do-not-merge`, the base's human-gated role label, a label in the activity's -`labels-absent` list (both read from the base before the head checkout) or a draft drops the lane's -commits and skips `awaiting-human`, and a read that fails or a second page of labels moves nothing -and fails the step. A raw `git push` from the -skill skips this check. Why a skill activity loads nothing from -the PR head: -[ADR 0055](../../adr/0055-load-nothing-head-controlled-into-a-pipeline-skill-activity.md). - -In the write file, on PR events claude-code-action adds a second head-isolation layer beside -`--setting-sources user`: when it treats the PR head as untrusted, it replaces `.claude`, -`.mcp.json`, `CLAUDE.md` and its other listed config paths with the PR base branch's copies -before Claude starts. The read file runs no claude-code-action step. - -In the write file, a skill whose `gating` is `gate` must end with a verdict. `claude_args` adds `--json-schema` with a -schema requiring `{"verdict": "pass" | "fail", "summary": string}`; claude-code-action fails its -step when no structured output returns. The next step reads the action's `structured_output` -output through `env`, parses it with `jq`, writes the summary to the step summary as data, and fails -red unless the verdict is exactly `pass`. A `fail` verdict therefore makes `act-outcome` `failure` -and the check a failure. A `gating` value other than `gate` or `advisory` fails the job red before -the head checkout. - -A `gating: gate` skill must not execute head code: no test, linter or build of the PR runs through -its Bash. The verdict check runs after the skill in the same job, so head code that ran first could -forge it, for example by writing a `$RUNNER_TEMP` file-command file such as one `BASH_ENV` points -at, or by leaving a process running. Tests, linters and builds that gate run as a `script` activity, -or under a separate design that judges their result in a job head code never touched. - -After a write-file skill, a pinned `actions/github-script` step removes the head checkout's -and `.base`'s `.git` directories, so neither `actions/checkout` post step runs git under config the -skill wrote while it holds the job's runtime token. A script activity skips that step. A process -the skill left running can make them again: no step stops such processes yet -([#8302](https://github.com/melodic-software/claude-code-plugins/issues/8302)). +untrusted text, so the activity must set `reads-untrusted: true`. The skill holds no GitHub token and pushes nothing: its change leaves the run only as an artifact +(a patch, replies, a verdict or a work outcome) that a script activity in the write file checks and +applies, and the PR is the review step for what lands. Why a skill activity loads nothing from the +PR head: [ADR 0055](../../adr/0055-load-nothing-head-controlled-into-a-pipeline-skill-activity.md). The step summary (in a PR-mode read run, the scan job's) shows the skill's final reply (at most 4000 characters, backticks neutralized) for audit. It is model output, printed as data. The same text is uploaded as the @@ -631,8 +573,7 @@ none), cut to 300 characters, and `denied_total`, the count of all of them. Unti the 32768-byte cap a read run's scan job carries it under, the oldest stderr messages drop first, then denials from the end. It writes them to `act-turns.json`, which goes into the same artifact and onto the step summary, where each denial shows as `tool: input` followed by -`(+N more)` when some were cut; in the write file the lane token is cut out -of both. In a PR-mode read run, `act-turns.json` is sealed with the reply for the scan job, which +`(+N more)` when some were cut. In a PR-mode read run, `act-turns.json` is sealed with the reply for the scan job, which shows and uploads it only after a clean leak scan. The full execution file is not uploaded: it holds head text and tool output. @@ -1895,8 +1836,9 @@ What it trusts: - Lane and activity from the reusable workflow's own inputs. - `needs.run.result`, and the run job's `act-outcome`, `can-commit`, `gate-reason`, `head-sha`, - `base-sha` and, in the write file, `run-failure` outputs, computed by the runner. All but a gate skill's `act-outcome` come from steps - that ran before any head code; that one rests on the rule that a gate skill runs no head code. + `base-sha` and, in the write file, `run-failure` outputs, computed by the runner. All but + `act-outcome` come from steps that ran before any head code; in the write file that one is a base + script's outcome, and the write file runs no skill. - Its own signed-commit result. On an unverified commit, `check-signed-commits` fails closed: the check is a failure, and the report job adds `needs-human` when the repository has that label and posts a comment whose first line is ``, which holds the lane @@ -1909,13 +1851,10 @@ The activity ran on exactly the commit the check is posted on: the run job check cannot make the activity judge a different commit than the one its check marks, so a later force-push back to the checked SHA never carries a result for code the activity did not see. -`persist-credentials: false` does not hold for a skill activity. claude-code-action writes the App -token into the origin URL in `.git/config` and exports it to Claude as `GH_TOKEN` and -`GITHUB_TOKEN`. A skill activity whose effect is not `read` therefore holds a contents-write App -token inside the Claude process and the git config. Such a skill must not execute head code: no -test or build of the PR runs through Bash. A lane that needs to run head code with a write effect -is outside this contract until a split design exists (a read-only run, then a separate step that -makes the signed commit). +No skill runs beside an App token: a skill's effect is always `read`, so it runs in the read file, +whose run job holds `contents: read` and `pull-requests: read` and no `id-token`, and hands its +change on as a patch whose SHA-256 the write file checks before it requests the token. The write +file's head checkout sets `persist-credentials: false`, and the scripts it runs come from the base. Secrets in the job that runs head code: neither file references the App key, and a run holds it only if a caller passes it ([below](#secrets-and-the-two-files)). Every skill job references the @@ -1924,15 +1863,9 @@ Claude OAuth token. Two routes reach it and the activity's GitHub token: - Environment inheritance, no root needed. claude-code-action puts `CLAUDE_CODE_OAUTH_TOKEN` and the token it was given (as `GH_TOKEN` and `GITHUB_TOKEN`) in Claude's environment, so every Bash child of Claude inherits them and can read them from its own environment or from - `/proc//environ`. The write file's skill step sets `CLAUDE_CODE_SUBPROCESS_ENV_SCRUB=1`, which - strips the OAuth token and other credentials from Bash, hook and MCP subprocesses and gives - Bash its own PID namespace, so an ancestor's `environ` is out of reach. The scrub keeps - `GH_TOKEN` and `GITHUB_TOKEN`. The read file runs its skill on run-claude-cli, not this action, - and its run-claude-cli steps set neither token. The write file's skill step sets both to empty - in its settings' `env`, so no Bash process, the sandbox's pid 1 included, holds the effect-scoped App token; the action's MCP - servers, commit signing among them, get it from their own config `env`. The action installs - bubblewrap only for `allowed_non_write_users`, so run job step 9 installs it and fails the job - when it is missing. + `/proc//environ`. Every skill runs in the read file, on run-claude-cli, not this + action, and its run-claude-cli steps set neither token; the write file runs no skill and + references no Claude credential. - Runner memory, with root. A hosted runner holds referenced secrets in the runner's memory. Removing sudo and docker access before the head checkout blocks the documented memory-dump route. @@ -2221,8 +2154,8 @@ The `issue-number` input of both files runs a lane on an issue instead of a PR. [`work-resolve-issue.yml`](../../../.github/workflows/work-resolve-issue.yml): the `agent-ready` label on an issue runs `resolve-issue` (`/work-items:work --issue --ci`) in the read file in [patch output mode](#patch-output-mode), so the model job holds no write token, then `apply-issue-patch` and -`report-outcome` (scripts) in the write file. The write file refuses an issue skill red -(`issue-skill-in-write-runner`). The model job's run job holds `contents: read` and +`report-outcome` (scripts) in the write file. The write file refuses any skill red +(`skill-in-write-runner`). The model job's run job holds `contents: read` and `pull-requests: read` only, and the skill runs on `run-claude-cli` like a PR-mode skill ([Two calls on one credential each](#two-calls-on-one-credential-each)), whose leg environment holds its one Claude credential and no GitHub token. Before any job that diff --git a/scripts/act-turns-denied.test.sh b/scripts/act-turns-denied.test.sh index 30f0ac50b20..4b8caba5a27 100755 --- a/scripts/act-turns-denied.test.sh +++ b/scripts/act-turns-denied.test.sh @@ -1,19 +1,18 @@ #!/usr/bin/env bash -# Behavior test: the "Check the skill took a model turn" step in both -# pr-run-activity files records each permission_denials entry as +# Behavior test: the "Check the skill took a model turn" step in +# pr-run-activity-read.yml records each permission_denials entry as # {tool_name, input} in act-turns.json, keeping the first 20 with denied_total # counting all, and the step summary shows each as `tool: input`, then # `(+N more)` for any cut. Tool names are cut to 100 characters, at most 4 # stderr messages are kept with local_command_stderr_total counting all, and # denied entries drop from the end until denied serializes to at most 8192 # bytes. A worst-case file, every field at its limit in 6-byte-escaped -# characters, fits the 32768-byte carry cap in both lanes, with the oldest -# stderr entries dropped first. The write file's step writes the summary itself; the read -# file's writes none, and its scan job's summary step, after the leak scan -# and in both modes, renders the act-turns.json it wrote. Runs each step's +# characters, fits the 32768-byte carry cap, with the oldest stderr entries +# dropped first. The step writes no summary; the scan job's summary step, +# after the leak scan and in both modes, renders the act-turns.json it wrote. Runs each step's # github-script body against a canned execution file with a stub `core` whose # summary records its code blocks. -# test-scope: .github/workflows/pr-run-activity-write.yml .github/workflows/pr-run-activity-read.yml .github/scripts/lib/act-turns.mjs .github/standards/runner-policy/package.json +# test-scope: .github/workflows/pr-run-activity-read.yml .github/scripts/lib/act-turns.mjs .github/standards/runner-policy/package.json set -uo pipefail ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" @@ -93,10 +92,9 @@ const runActTurns = async (lane, wf, messages) => { } catch {} return { dir, blocks, error, turns }; }; -// The summary views of a run: the write step's own, else the read file's scan -// job summary step rendering the act-turns.json in `dir`; its run job shows none. -const views = async (lane, wf, dir, blocks) => { - if (lane === "write") return [["the act-turns step", blocks.join("\n")]]; +// The summary views of a run: the scan job summary step rendering the +// act-turns.json in `dir`; the run job shows none. +const views = async (lane, wf, dir) => { const out = []; say(!wf.jobs.run.steps.some((s) => s.name === "Show the skill's run in the step summary"), "read: the run job shows no skill run in its summary"); for (const [job, steps] of [["scan", wf.jobs.scan.steps]]) { @@ -114,7 +112,7 @@ const views = async (lane, wf, dir, blocks) => { const result = (extra) => ({ type: "result", subtype: "success", is_error: false, num_turns: 3, duration_ms: 1, modelUsage: { "claude-x": {} }, ...extra }); -for (const lane of ["write", "read"]) { +for (const lane of ["read"]) { const wf = YAML.parse(readFileSync(`.github/workflows/pr-run-activity-${lane}.yml`, "utf8")); if (!wf.jobs.run.steps.find((s) => s.id === "act-turns")?.with?.script) { say(false, `${lane}: the run job has an act-turns step with a script`); diff --git a/scripts/write-runner-hold-labels.test.sh b/scripts/write-runner-hold-labels.test.sh new file mode 100755 index 00000000000..92b235f8288 --- /dev/null +++ b/scripts/write-runner-hold-labels.test.sh @@ -0,0 +1,90 @@ +#!/usr/bin/env bash +# Behavior test: the "Read the hold labels from the base" step in +# pr-run-activity-write.yml, which gives apply-patch its hold labels: do-not-merge, +# the human-gated role label unless the caller's base-code verdict lifts it, +# and the activity's labels-absent list. Runs the step's bash with a stub +# gate-recheck.mjs. +# test-scope: .github/workflows/pr-run-activity-write.yml .github/standards/runner-policy/package.json +set -uo pipefail + +ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" +# shellcheck source=lib/test-harness.sh +. "$ROOT/scripts/lib/test-harness.sh" + +results=$( + cd "$ROOT" && node --input-type=module - <<'EOF' +import { readFileSync } from "node:fs"; +import { createRequire } from "node:module"; +const require = createRequire(`${process.cwd()}/.github/standards/runner-policy/package.json`); +const YAML = require("yaml"); + +const say = (good, text) => console.log(`${good ? "ok" : "fail"}\t${text}`); +const wf = YAML.parse(readFileSync(".github/workflows/pr-run-activity-write.yml", "utf8")); +// The hold-label step: its bash run with a resolved config naming the +// activity's labels-absent, from a directory with no .base tracker file, so +// the human-gated label is the needs-human default. +const { execFileSync } = await import("node:child_process"); +const { existsSync, mkdirSync, mkdtempSync, writeFileSync, rmSync } = await import("node:fs"); +const { tmpdir } = await import("node:os"); +const { join } = await import("node:path"); +const holdStep = wf.jobs.run.steps.find((s) => s.id === "hold-label"); +// gate: the caller's patch-human-gate. On lane-escalation the step runs the +// base's gate-recheck.mjs; a stub in its place answers `recheck` and records +// the environment it was given. +const holdLabelsFor = (gate, labelsAbsent, recheck = "true") => { + const dir = mkdtempSync(join(tmpdir(), "hold-label-")); + try { + const resolved = join(dir, "resolved.json"); + const out = join(dir, "out"); + const seen = join(dir, "seen.json"); + writeFileSync(resolved, JSON.stringify({ selected: { "applies-when": { "labels-absent": labelsAbsent } } })); + writeFileSync(out, ""); + const stub = join(dir, ".base/.github/scripts/pr-unblock"); + mkdirSync(stub, { recursive: true }); + writeFileSync(join(stub, "gate-recheck.mjs"), `import { appendFileSync, writeFileSync } from "node:fs"; +writeFileSync(${JSON.stringify(seen)}, JSON.stringify({ label: process.env.LABEL, pr: process.env.PR_NUMBER, head: process.env.HEAD_SHA, activity: process.env.ACTIVITY, out: process.env.GITHUB_OUTPUT === ${JSON.stringify(out)} })); +${recheck === "fail" ? "process.exit(1);" : `appendFileSync(process.env.GITHUB_OUTPUT, "lane-escalation=${recheck}\\n");`} +`); + try { + execFileSync("bash", ["-e", "-c", holdStep.run], { cwd: dir, stdio: "pipe", env: { PATH: process.env.PATH, TMPDIR: dir, RESOLVED: resolved, HUMAN_GATE: gate, ACTIVITY: "fix-ci", PR_NUMBER: "7", HEAD_SHA: "a".repeat(40), GITHUB_OUTPUT: out } }); + } catch { + return { failed: true, seen: existsSync(seen) ? JSON.parse(readFileSync(seen, "utf8")) : null }; + } + return { holds: JSON.parse(readFileSync(out, "utf8").replace(/^labels=/, "")), seen: existsSync(seen) ? JSON.parse(readFileSync(seen, "utf8")) : null }; + } finally { + rmSync(dir, { recursive: true, force: true }); + } +}; +say(String(holdStep?.env?.ACTIVITY) === "${{ inputs.patch-activity }}", "the hold-label step hands gate-recheck.mjs the caller's patch-activity"); +say(String(holdStep?.env?.HUMAN_GATE) === "${{ steps.config.outputs.prepare == 'apply-patch' && inputs.patch-human-gate || '' }}", + "the hold-label step reads the caller's base-code verdict (patch-human-gate) only for an apply-patch run"); +let r_ = holdLabelsFor("fix-request", ["do-not-merge"]); +say(JSON.stringify(r_.holds) === '["do-not-merge"]' && r_.seen === null, + `a write-access fix request on a needs-human PR lifts only the human-gated hold, with no re-read (${JSON.stringify(r_)})`); +r_ = holdLabelsFor("fix-request", ["needs-human"]); +say(r_.holds.includes("do-not-merge") && r_.holds.includes("needs-human"), + "do-not-merge stays a hold beside a fix request, and a listed labels-absent label still holds"); +r_ = holdLabelsFor("lane-escalation", ["do-not-merge"], "true"); +say(JSON.stringify(r_.holds) === '["do-not-merge"]' && r_.seen?.label === "needs-human" && r_.seen.pr === "7" && r_.seen.head === "a".repeat(40) && r_.seen.activity === "fix-ci" && r_.seen.out === false, + `a lane escalation the re-read still finds lifts only the human-gated hold (${JSON.stringify(r_)})`); +r_ = holdLabelsFor("lane-escalation", ["do-not-merge"], "false"); +say(r_.holds?.includes("needs-human") && r_.holds.includes("do-not-merge") && r_.seen !== null, + `a lane escalation the re-read no longer finds keeps the human-gated hold (${JSON.stringify(r_)})`); +r_ = holdLabelsFor("lane-escalation", ["do-not-merge"], "fail"); +say(r_.failed === true, `a re-read that fails fails the step (${JSON.stringify(r_)})`); +for (const gate of ["false", "", "true", "FIX-REQUEST", "lane-escalation-x"]) { + r_ = holdLabelsFor(gate, ["do-not-merge"]); + say(r_.holds?.includes("needs-human") && r_.holds.includes("do-not-merge") && r_.seen === null, + `with patch-human-gate ${JSON.stringify(gate)} the human-gated label stays a hold (${JSON.stringify(r_)})`); +} +EOF +) +if [[ -z "$results" ]]; then + fail "the check printed nothing; is .github/standards/runner-policy installed?" +fi +while IFS=$'\t' read -r verdict text; do + [[ -n "$verdict" ]] || continue + if [[ "$verdict" == ok ]]; then ok "$text"; else fail "$text"; fi +done <<<"$results" + +test_harness::report diff --git a/scripts/write-runner-never-rules.test.sh b/scripts/write-runner-never-rules.test.sh index e33d750dd52..854d172c438 100755 --- a/scripts/write-runner-never-rules.test.sh +++ b/scripts/write-runner-never-rules.test.sh @@ -8,10 +8,6 @@ # Reads the workflow as YAML (the runner-policy `yaml` package, which CI # installs for check-read-caller-keys.test.sh) and asserts on step order, # conditions and step bodies. What it cannot assert from this file: -# - The skill's own allowed-tools Bash grant (its SKILL.md) and its token: -# with a contents-write token the model could push or run head code -# itself. The backstops are the App's missing `workflows` permission and -# ADR 0055's accepted residual, not this file. # - That apply-patch.mjs refuses every workflow change a merge pushes; # apply-patch.test.mjs owns that. # test-scope: .github/workflows/pr-run-activity-write.yml .github/actions/resolve-config/activity-rules.json .github/actions/collect-base-activity/action.yml .github/scripts/merge-base/run-prepare.sh .github/scripts/apply-patch/apply-patch.mjs .github/actions/resolve-config/patch-rules.json .github/standards/runner-policy/package.json @@ -47,14 +43,13 @@ const mint = at((s) => s.id === "lane-token"); const body = (s) => `${s.run ?? ""}${s.with?.script ?? ""}`; const after = steps.slice(checkout + 1); -// Clause 1: no force-push. The promote step moves a skill's lane branch; -// apply-patch.mjs moves the PR branch for a patch, a merge's included. +// Clause 1: no force-push. apply-patch.mjs moves the PR branch for a patch, a +// merge's included; no step of the workflow moves a ref itself. const applyPatch = readFileSync(".github/scripts/apply-patch/apply-patch.mjs", "utf8"); -const updates = steps.flatMap((s) => body(s).match(/updateRef\([^)]*\)/g) ?? []); -say(updates.length >= 1, `the run job updates refs through updateRef (${updates.length} calls)`); +const updates = steps.flatMap((s) => body(s).match(/(update|create)Ref\(/g) ?? []); +say(updates.length === 0, `no step of the run job creates or updates a ref itself (${updates.length} calls)`); say(/write: \(api, ref, commit\) => api\.updateRef\(`heads\/\$\{ref\}`, commit, false\)/.test(applyPatch) && !/updateRef\([^)]*true\)/.test(applyPatch), "apply-patch.mjs moves the PR branch with force: false"); -say(updates.every((u) => /force: false/.test(u)), "every updateRef call passes force: false"); say(!steps.some((s) => /git\s+push|--force|\+refs\//.test(body(s))), "no step runs git push or a forced refspec"); // portability-ok: embedded node JavaScript regex, not a shell tool pattern say(!steps.some((s) => /(update|create)Ref\(/.test(body(s)) && /Ref\([^)]*force:\s*true/.test(body(s))), // portability-ok: embedded node JavaScript regex, not a shell tool pattern "no ref update sets force: true"); @@ -118,15 +113,14 @@ const offenders = after.filter((s) => s.run && headRun.test(s.run)).map((s) => s say(offenders.length === 0, `no run step after the head checkout runs head code (${offenders.join(", ") || "none"})`); const script = byId("act-script").run ?? ""; say(/"\$RUNNER_TEMP\/base-script\/\$\(basename "\$SCRIPT"\)"/.test(script), "a script activity runs from its base copy"); -const skill = byId("act-skill"); -const args = String(skill.with?.claude_args ?? ""); -// The action admits the lanes App, which the trigger gate passes only on its -// app field write, and github-actions only on a dispatcher-sent run the gate -// verified; never '*'. cant-fail-ok: the allowed_bots value is the contract. -say(skill.with?.allowed_bots === "${{ inputs.dispatcher-run-id != '' && 'melodic-automation-lanes[bot],github-actions' || 'melodic-automation-lanes[bot]' }}", - `the skill admits the lanes App, and github-actions only with a dispatcher-run-id (got ${JSON.stringify(skill.with?.allowed_bots)})`); -say(/--setting-sources user/.test(args) && /--permission-mode dontAsk/.test(args) && !/bypass/i.test(args), - "the skill loads user settings only, under dontAsk"); +// No model runs beside the lane token: no step runs claude-code-action or +// takes a Claude credential, and a skill fails red before the mint. +say(!steps.some((s) => /claude-code-action/.test(s.uses ?? "")) && !/secrets\./.test(JSON.stringify(wf.jobs.run)) && !wf.on.workflow_call.secrets, + "the write runner runs no model and takes no secret"); +const refuseSkill = at((s) => s.name === "Assert a skill runs in the read runner"); +say(refuseSkill >= 0 && refuseSkill < mint && String(steps[refuseSkill].if ?? "") === "steps.config.outcome == 'success' && steps.config.outputs.kind == 'skill'" + && /exit 1/.test(steps[refuseSkill].run ?? ""), + "a skill fails red before the mint"); const stage = readFileSync(".github/actions/collect-base-activity/action.yml", "utf8"); // The five exceptions: an activity's `checks`, each a read-only base-copy // wrapper the skill may run with no arguments; its `verify-scripts`, each a @@ -152,13 +146,12 @@ say(/chmod -R a-w "\$VERIFY_DIR"/.test(stage) && /disallowed="\$\{disallowed:\+\ const refuse = at((s) => s.name === "Assert review-threads runs in the read runner"); say(refuse >= 0 && refuse < mint && /steps\.config\.outputs\.prepare == 'review-threads'/.test(steps[refuse].if ?? "") && /exit 1/.test(steps[refuse].run ?? ""), "a review-threads activity fails red before the mint"); -say(!steps.some((s) => ["feedback-select", "feedback-collect"].includes(s.id)) && !("THREAD_FILE" in (byId("promote").env ?? {})), +say(!steps.some((s) => ["feedback-select", "feedback-collect"].includes(s.id)), "the write runner keeps no review threads and collects no replies"); say(/proceed != 'false'/.test(byId("head-recheck").if ?? ""), "a select step that does not proceed skips the mint"); const runSteps = JSON.stringify(steps); say(!/resolveReviewThread|\/replies['"`]|record-round/.test(runSteps) && !/feedback-round/.test(JSON.stringify(steps.map((s) => s.id))), "no step of the write runner replies on or resolves a review thread"); -const promoteScript = String(byId("promote").with?.script ?? ""); say(byId("act-script").env?.RUN_SHA256 === "${{ inputs.review-run-sha256 }}", "the reply script gets the SHA-256 of the run.json the caller's select job wrote"); // "Re-run failed jobs" on the reply job alone bumps github.run_attempt but @@ -187,13 +180,13 @@ say(/:\?\*:dependency-invalid \| :\?\*:dependency-fix-not-landed\)/.test(byId("a && /:\?\*:dependency-fix-refused\)/.test(byId("act-script").run ?? ""), "a script escalates with the verdict script's reasons only in dependency-verdict mode"); // dependency-review runs on the read runner, after its caller's select job: -// this file refuses it before the mint, selects nothing, promotes no fix and -// collects no verdict; apply-patch pushes the fix under the dependency-review row. +// this file refuses it before the mint, selects nothing and collects no +// verdict; apply-patch pushes the fix under the dependency-review row. const refuseDependency = at((s) => s.name === "Assert dependency-review runs in the read runner"); say(refuseDependency >= 0 && refuseDependency < mint && /steps\.config\.outputs\.prepare == 'dependency-review'/.test(steps[refuseDependency].if ?? "") && /exit 1/.test(steps[refuseDependency].run ?? ""), "a dependency-review activity fails red before the mint"); -say(!steps.some((s) => ["dependency-select", "dependency-collect"].includes(s.id)) && !/DEPENDENCY_REVIEW|CLASSIFY_SHA256|inFixScope|fixRefusal/.test(promoteScript), - "the write runner selects no dependency pull request, promotes no dependency fix and collects no verdict"); +say(!steps.some((s) => ["dependency-select", "dependency-collect"].includes(s.id)), + "the write runner selects no dependency pull request and collects no verdict"); // apply-patch runs from a base copy, so each sibling-directory module it or // its predicates import must be copied beside it. const siblingImports = ["apply-patch.mjs", "predicates.mjs"].flatMap((file) => @@ -246,13 +239,9 @@ say(octokitCallers.length === 0, `no github-script step calls getOctokit (${octo say(applyPatch.indexOf("await target.held(api, number,", applyPatch.indexOf("async function pushMerge")) < applyPatch.indexOf("await api.createCommit(message, tree, [headSha, merge.baseSha])"), "apply-patch.mjs re-reads the PR's holds before creating a merge commit"); const holdLabel = byId("hold-label").run ?? ""; -const promoteEnv = byId("promote").env ?? {}; say(/\.base\/docs\/conventions\/work-items\.yaml/.test(holdLabel) && /labels-absent/.test(holdLabel) && /echo "labels=\$labels"/.test(holdLabel) - && /^\$\{\{ steps\.hold-label\.outputs\.labels \}\}$/.test(String(promoteEnv.HOLD_LABELS)) - && String(promoteEnv.READ_TOKEN) === "${{ github.token }}" - && /^\$\{\{ steps\.hold-label\.outputs\.labels \}\}$/.test(String(byId("act-script").env?.HOLD_LABELS)) - && /^\$\{\{ steps\.gate\.outputs\.pr-number \}\}$/.test(String(promoteEnv.PR_NUMBER)), - "the hold labels are resolved from the base before the head checkout and reach the promote step with the read-only token and the apply-patch script"); + && /^\$\{\{ steps\.hold-label\.outputs\.labels \}\}$/.test(String(byId("act-script").env?.HOLD_LABELS)), + "the hold labels are resolved from the base before the head checkout and reach the apply-patch script"); // Issue mode runs its skill in the read runner, which holds no lane token: // these checks read that file. Only the skill run collects the outcome, and diff --git a/scripts/write-runner-over-cap-success.test.sh b/scripts/write-runner-over-cap-success.test.sh deleted file mode 100755 index 3fb89b118fd..00000000000 --- a/scripts/write-runner-over-cap-success.test.sh +++ /dev/null @@ -1,102 +0,0 @@ -#!/usr/bin/env bash -# Behavior test: the "Check the skill's failure is only the turn cap" step in -# pr-run-activity-write.yml counts a failed skill step as a success only when -# the execution file's last result message is subtype success, is_error false, -# with num_turns over --max-turns. Every other failure, and a missing or -# unreadable execution file, fails the step. Runs the step's github-script -# body with a stub `core` against temp execution files. -# test-scope: .github/workflows/pr-run-activity-write.yml .github/standards/runner-policy/package.json -set -uo pipefail - -ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" -# shellcheck source=lib/test-harness.sh -. "$ROOT/scripts/lib/test-harness.sh" - -results=$( - cd "$ROOT" && node --input-type=module - <<'EOF' -import { mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; -import { createRequire } from "node:module"; -import { tmpdir } from "node:os"; -import { join } from "node:path"; -const require = createRequire(`${process.cwd()}/.github/standards/runner-policy/package.json`); -const YAML = require("yaml"); - -const say = (good, text) => console.log(`${good ? "ok" : "fail"}\t${text}`); -const wf = YAML.parse(readFileSync(".github/workflows/pr-run-activity-write.yml", "utf8")); -const steps = wf.jobs.run.steps; -const at = steps.findIndex((s) => s.id === "act-skill"); -const step = steps[at + 1]; -say(step?.id === "act-result" && Boolean(step?.with?.script), "the turn-cap check is the step right after the skill step"); -say(steps[at]?.["continue-on-error"] === true, "the skill step's failure is left to the turn-cap check"); -const record = steps.find((s) => s.id === "lane-record"); -say(/steps\.act-result\.outputs\.outcome/.test(String(record?.with?.outcome)), "the lane record reads the checked outcome"); - -const dir = mkdtempSync(join(tmpdir(), "over-cap-")); -const AsyncFunction = (async () => {}).constructor; - -// messages: the execution file's JSON, or a raw string, or null for no file. -async function run({ outcome = "failure", messages, maxTurns = "75" } = {}) { - const file = join(dir, `exec-${Math.random().toString(36).slice(2)}.json`); - if (messages !== null) { - writeFileSync(file, typeof messages === "string" ? messages : JSON.stringify(messages)); - } - const calls = { failed: [], outputs: {} }; - const core = { - notice: () => {}, - setFailed: (m) => calls.failed.push(m), - setOutput: (k, v) => { - calls.outputs[k] = v; - }, - }; - Object.assign(process.env, { ACT_OUTCOME: outcome, EXECUTION_FILE: file, MAX_TURNS: maxTurns }); - await new AsyncFunction("core", "require", step.with.script)(core, require); - return calls; -} - -const result = (fields) => [{ type: "system" }, { type: "assistant" }, { type: "result", ...fields }]; -const succeeded = (r) => r.outputs.outcome === "success" && r.failed.length === 0; -const failed = (r) => r.outputs.outcome === undefined && r.failed.length === 1; - -let r = await run({ messages: result({ subtype: "success", is_error: false, num_turns: 140 }) }); -say(succeeded(r), "a failed step whose result is a success over the cap counts as a success"); - -r = await run({ messages: result({ subtype: "error_max_turns", is_error: true, num_turns: 76 }) }); -say(failed(r), "an over-cap error result stays a failure"); - -r = await run({ messages: result({ subtype: "success", is_error: true, num_turns: 140 }) }); -say(failed(r), "an over-cap success with is_error true stays a failure"); - -r = await run({ messages: result({ subtype: "success", is_error: false, num_turns: 140 }), maxTurns: "" }); -say(failed(r), "an over-cap success with no cap configured stays a failure"); - -r = await run({ messages: result({ subtype: "success", is_error: false, num_turns: 75 }) }); -say(failed(r), "a failed step whose success result is within the cap stays a failure"); - -r = await run({ messages: null }); -say(failed(r), "a failed step with no execution file stays a failure"); - -r = await run({ messages: "{not json" }); -say(failed(r), "a failed step with an unparsable execution file stays a failure"); - -r = await run({ messages: [{ type: "assistant" }] }); -say(failed(r), "a failed step with no result message stays a failure"); - -r = await run({ - messages: [...result({ subtype: "success", is_error: false, num_turns: 140 }), { type: "result", subtype: "error_during_execution", is_error: true, num_turns: 141 }], -}); -say(failed(r), "only the last result message counts"); - -r = await run({ outcome: "success", messages: result({ subtype: "success", is_error: false, num_turns: 20 }) }); -say(succeeded(r), "a skill step that succeeded within the cap is unchanged"); -rmSync(dir, { recursive: true, force: true }); -EOF -) -if [[ -z "$results" ]]; then - fail "the check printed nothing; is .github/standards/runner-policy installed?" -fi -while IFS=$'\t' read -r verdict text; do - [[ -n "$verdict" ]] || continue - if [[ "$verdict" == ok ]]; then ok "$text"; else fail "$text"; fi -done <<<"$results" - -test_harness::report diff --git a/scripts/write-runner-promote-holds.test.sh b/scripts/write-runner-promote-holds.test.sh deleted file mode 100755 index d551ea5681c..00000000000 --- a/scripts/write-runner-promote-holds.test.sh +++ /dev/null @@ -1,213 +0,0 @@ -#!/usr/bin/env bash -# Behavior test: the "Move the PR branch to the lane's commits" step in -# pr-run-activity-write.yml re-reads the PR's labels and draft flag right -# before it moves the PR branch, and drops the lane's commits (awaiting-human) -# when a do-not-merge or human-gated label, or a draft, was set during the run. -# A read it cannot complete moves nothing. Runs the step's github-script body -# with a stub `github`, `core` and `fetch`. -# test-scope: .github/workflows/pr-run-activity-write.yml .github/standards/runner-policy/package.json -set -uo pipefail - -ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" -# shellcheck source=lib/test-harness.sh -. "$ROOT/scripts/lib/test-harness.sh" - -results=$( - cd "$ROOT" && node --input-type=module - <<'EOF' -import { readFileSync } from "node:fs"; -import { createRequire } from "node:module"; -const require = createRequire(`${process.cwd()}/.github/standards/runner-policy/package.json`); -const YAML = require("yaml"); - -const say = (good, text) => console.log(`${good ? "ok" : "fail"}\t${text}`); -const wf = YAML.parse(readFileSync(".github/workflows/pr-run-activity-write.yml", "utf8")); -const step = wf.jobs.run.steps.find((s) => s.name === "Move the PR branch to the lane's commits"); -say(Boolean(step?.with?.script), "the run job has a promote step with a script"); - -const GATED = "a".repeat(40); -const LANE = "b".repeat(40); -const AsyncFunction = (async () => {}).constructor; - -// pull: the GET pulls/ body; labels: label names; status and next shape -// the labels answer. -// holdLabels: the hold-label step's output, a JSON array. -async function run({ pull = { draft: false }, labels = [], labelsStatus = 200, labelsNext = false, holdLabels = '["do-not-merge","needs-human"]', headMovesOnRead = false } = {}) { - const calls = { failed: [], info: [], outputs: {}, updated: [], deleted: [], fetched: [] }; - let head = GATED; - const github = { - request: async (_route, { ref }) => ({ data: { object: { sha: ref === "heads/lane" ? LANE : headMovesOnRead && head === GATED ? "c".repeat(40) : head } } }), - rest: { - git: { - updateRef: async ({ ref, sha }) => { - calls.updated.push(ref); - head = sha; - }, - deleteRef: async ({ ref }) => { - calls.deleted.push(ref); - }, - }, - }, - }; - const core = { - info: (m) => calls.info.push(m), - warning: () => {}, - setFailed: (m) => calls.failed.push(m), - setOutput: (k, v) => { - calls.outputs[k] = v; - }, - }; - const fetch = async (url, { headers }) => { - calls.fetched.push([url, headers.Authorization]); - const isLabels = url.includes("/labels"); - const status = isLabels ? labelsStatus : 200; - return { - ok: status < 300, - status, - headers: { get: () => (isLabels && labelsNext ? '; rel="next"' : "") }, - json: async () => (isLabels ? labels.map((name) => ({ name })) : pull), - }; - }; - Object.assign(process.env, { - HEAD_SHA: GATED, - HEAD_REF: "feature", - LANE_BRANCH: "lane", - MERGE_BASE: "false", - COMMIT_OUTCOME: "", - COMMIT_SHA: "", - THREAD_FILE: "", - PR_NUMBER: "42", - HOLD_LABELS: holdLabels, - READ_TOKEN: "read-only", - GITHUB_API_URL: "https://api.github.com", - }); - const context = { repo: { owner: "o", repo: "r" } }; - let threw = null; - try { - await new AsyncFunction("github", "core", "context", "fetch", "require", step.with.script)(github, core, context, fetch, require); - } catch (e) { - threw = e; - } - return { ...calls, threw }; -} - -let r = await run(); -say(r.updated.length === 1 && r.failed.length === 0 && !r.threw && r.outputs["skip-reason"] === "", - "no hold and not a draft: the PR branch moves to the lane's commits"); -say(r.fetched.length === 2 && r.fetched.every(([, auth]) => auth === "Bearer read-only"), - "the labels and the draft flag are read with the job's read-only token"); - -for (const label of ["do-not-merge", "needs-human"]) { - r = await run({ labels: ["bug", label] }); - say(r.updated.length === 0 && r.outputs["skip-reason"] === "awaiting-human" && r.deleted.includes("heads/lane"), - `a ${label} label added during the run: nothing moves, awaiting-human, the lane branch is deleted`); -} - -r = await run({ labels: ["status: blocked-human"], holdLabels: '["do-not-merge","status: blocked-human"]' }); -say(r.updated.length === 0 && r.outputs["skip-reason"] === "awaiting-human", - "the base's own human-gated label is a hold"); - -r = await run({ labels: ["needs-human"], holdLabels: '["do-not-merge","status: blocked-human","needs-human"]' }); -say(r.updated.length === 0 && r.outputs["skip-reason"] === "awaiting-human", - "a label from the activity's labels-absent list is a hold beside a custom human-gated label"); - -r = await run({ labels: ["needs-human"], holdLabels: '["do-not-merge"]' }); -say(r.updated.length === 1 && r.failed.length === 0 && r.outputs["skip-reason"] === "", - "a needs-human label outside the hold list (address-feedback's) does not stop the move"); - -r = await run({ labels: ["do-not-merge"], holdLabels: "[]" }); -say(r.updated.length === 0 && r.outputs["skip-reason"] === "awaiting-human", - "do-not-merge is a hold even when the list omits it"); - -r = await run({ headMovesOnRead: true }); -say(r.updated.length === 0 && r.failed.length === 1 && r.fetched.length === 2, - "the PR head is re-read after the hold reads, and a moved head moves nothing"); - -r = await run({ pull: { draft: true } }); -say(r.updated.length === 0 && r.outputs["skip-reason"] === "awaiting-human" && r.deleted.includes("heads/lane"), - "a PR converted to a draft during the run: nothing moves, awaiting-human"); - -r = await run({ pull: {} }); -say(r.updated.length === 0, "a PR whose draft flag does not read false moves nothing"); - -r = await run({ labelsStatus: 502 }); -say(r.updated.length === 0 && (r.threw !== null || r.failed.length > 0) && r.deleted.includes("heads/lane"), - "a labels read that fails moves nothing and fails the step"); - -r = await run({ labelsNext: true }); -say(r.updated.length === 0 && (r.threw !== null || r.failed.length > 0), - "more labels than one page moves nothing and fails the step"); - -r = await run({ holdLabels: "" }); -say(r.updated.length === 0 && r.failed.length === 1, "no hold labels from the base moves nothing and fails the step"); - -r = await run({ holdLabels: "not json" }); -say(r.updated.length === 0 && (r.threw !== null || r.failed.length > 0), "a malformed hold list moves nothing and fails the step"); - -// The hold-label step: its bash run with a resolved config naming the -// activity's labels-absent, from a directory with no .base tracker file, so -// the human-gated label is the needs-human default. -const { execFileSync } = await import("node:child_process"); -const { existsSync, mkdirSync, mkdtempSync, writeFileSync, rmSync } = await import("node:fs"); -const { tmpdir } = await import("node:os"); -const { join } = await import("node:path"); -const holdStep = wf.jobs.run.steps.find((s) => s.id === "hold-label"); -// gate: the caller's patch-human-gate. On lane-escalation the step runs the -// base's gate-recheck.mjs; a stub in its place answers `recheck` and records -// the environment it was given. -const holdLabelsFor = (gate, labelsAbsent, recheck = "true") => { - const dir = mkdtempSync(join(tmpdir(), "hold-label-")); - try { - const resolved = join(dir, "resolved.json"); - const out = join(dir, "out"); - const seen = join(dir, "seen.json"); - writeFileSync(resolved, JSON.stringify({ selected: { "applies-when": { "labels-absent": labelsAbsent } } })); - writeFileSync(out, ""); - const stub = join(dir, ".base/.github/scripts/pr-unblock"); - mkdirSync(stub, { recursive: true }); - writeFileSync(join(stub, "gate-recheck.mjs"), `import { appendFileSync, writeFileSync } from "node:fs"; -writeFileSync(${JSON.stringify(seen)}, JSON.stringify({ label: process.env.LABEL, pr: process.env.PR_NUMBER, head: process.env.HEAD_SHA, activity: process.env.ACTIVITY, out: process.env.GITHUB_OUTPUT === ${JSON.stringify(out)} })); -${recheck === "fail" ? "process.exit(1);" : `appendFileSync(process.env.GITHUB_OUTPUT, "lane-escalation=${recheck}\\n");`} -`); - try { - execFileSync("bash", ["-e", "-c", holdStep.run], { cwd: dir, stdio: "pipe", env: { PATH: process.env.PATH, TMPDIR: dir, RESOLVED: resolved, HUMAN_GATE: gate, ACTIVITY: "fix-ci", PR_NUMBER: "7", HEAD_SHA: "a".repeat(40), GITHUB_OUTPUT: out } }); - } catch { - return { failed: true, seen: existsSync(seen) ? JSON.parse(readFileSync(seen, "utf8")) : null }; - } - return { holds: JSON.parse(readFileSync(out, "utf8").replace(/^labels=/, "")), seen: existsSync(seen) ? JSON.parse(readFileSync(seen, "utf8")) : null }; - } finally { - rmSync(dir, { recursive: true, force: true }); - } -}; -say(String(holdStep?.env?.ACTIVITY) === "${{ inputs.patch-activity }}", "the hold-label step hands gate-recheck.mjs the caller's patch-activity"); -say(String(holdStep?.env?.HUMAN_GATE) === "${{ steps.config.outputs.prepare == 'apply-patch' && inputs.patch-human-gate || '' }}", - "the hold-label step reads the caller's base-code verdict (patch-human-gate) only for an apply-patch run"); -let r_ = holdLabelsFor("fix-request", ["do-not-merge"]); -say(JSON.stringify(r_.holds) === '["do-not-merge"]' && r_.seen === null, - `a write-access fix request on a needs-human PR lifts only the human-gated hold, with no re-read (${JSON.stringify(r_)})`); -r_ = holdLabelsFor("fix-request", ["needs-human"]); -say(r_.holds.includes("do-not-merge") && r_.holds.includes("needs-human"), - "do-not-merge stays a hold beside a fix request, and a listed labels-absent label still holds"); -r_ = holdLabelsFor("lane-escalation", ["do-not-merge"], "true"); -say(JSON.stringify(r_.holds) === '["do-not-merge"]' && r_.seen?.label === "needs-human" && r_.seen.pr === "7" && r_.seen.head === "a".repeat(40) && r_.seen.activity === "fix-ci" && r_.seen.out === false, - `a lane escalation the re-read still finds lifts only the human-gated hold (${JSON.stringify(r_)})`); -r_ = holdLabelsFor("lane-escalation", ["do-not-merge"], "false"); -say(r_.holds?.includes("needs-human") && r_.holds.includes("do-not-merge") && r_.seen !== null, - `a lane escalation the re-read no longer finds keeps the human-gated hold (${JSON.stringify(r_)})`); -r_ = holdLabelsFor("lane-escalation", ["do-not-merge"], "fail"); -say(r_.failed === true, `a re-read that fails fails the step (${JSON.stringify(r_)})`); -for (const gate of ["false", "", "true", "FIX-REQUEST", "lane-escalation-x"]) { - r_ = holdLabelsFor(gate, ["do-not-merge"]); - say(r_.holds?.includes("needs-human") && r_.holds.includes("do-not-merge") && r_.seen === null, - `with patch-human-gate ${JSON.stringify(gate)} the human-gated label stays a hold (${JSON.stringify(r_)})`); -} -EOF -) -if [[ -z "$results" ]]; then - fail "the check printed nothing; is .github/standards/runner-policy installed?" -fi -while IFS=$'\t' read -r verdict text; do - [[ -n "$verdict" ]] || continue - if [[ "$verdict" == ok ]]; then ok "$text"; else fail "$text"; fi -done <<<"$results" - -test_harness::report From ba3991685bb8d636b9a76edbdf8dff8aadab4da4 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Wed, 11 Nov 2026 10:42:22 -0500 Subject: [PATCH 2/4] fix(pr-pipeline): record the change as a changelog fragment pr-pipeline releases from fragments, so the change is described under .changes/pr-pipeline/ and the hand CHANGELOG entry is reverted. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01VQb9Q2ceZLXhTR4d1vkTCe --- .../fix-pr-pipeline-skill-effect-read-5c1e9a27.md | 13 +++++++++++++ docs/conventions/pr-pipeline/CHANGELOG.md | 12 ------------ 2 files changed, 13 insertions(+), 12 deletions(-) create mode 100644 .changes/pr-pipeline/fix-pr-pipeline-skill-effect-read-5c1e9a27.md diff --git a/.changes/pr-pipeline/fix-pr-pipeline-skill-effect-read-5c1e9a27.md b/.changes/pr-pipeline/fix-pr-pipeline-skill-effect-read-5c1e9a27.md new file mode 100644 index 00000000000..bf62357fed1 --- /dev/null +++ b/.changes/pr-pipeline/fix-pr-pipeline-skill-effect-read-5c1e9a27.md @@ -0,0 +1,13 @@ +--- +bump: minor +--- + +### Changed + +- A skill activity's `effect` must be `read`. The schema and resolve-config (rejection 17, `skill-not-read`) refuse any other effect at config load, in any lane, so every skill runs in `pr-run-activity-read.yml` and hands its change to a script activity. +- `pr-run-activity-write.yml` runs scripts only and fails red with `skill-in-write-runner` before the mint for any skill, not just an issue skill. + +### Removed + +- The write runner's skill path, which no config can now reach: the claude-code-action step, its lane commit branch and promote step, the turn-cap, model-turn and gate-verdict checks, the skill reply upload, the lane-outcome record, and the unused `claude-code-oauth-token` secret. The write runner's report no longer carries a `lane-outcome`. +- The schema's head-test-check limits on a skill whose effect is not `read`, which no skill can now have. diff --git a/docs/conventions/pr-pipeline/CHANGELOG.md b/docs/conventions/pr-pipeline/CHANGELOG.md index ab9092b1bcd..c40a4ac069c 100644 --- a/docs/conventions/pr-pipeline/CHANGELOG.md +++ b/docs/conventions/pr-pipeline/CHANGELOG.md @@ -1,17 +1,5 @@ # Changelog for the PR pipeline convention -## [9.21.0] - 2026-11-11 - -### Changed - -- A skill activity's `effect` must be `read`. The schema and resolve-config (rejection 17, `skill-not-read`) refuse any other effect at config load, in any lane, so every skill runs in `pr-run-activity-read.yml` and hands its change to a script activity. Every skill in `pr-pipeline.yaml` already has effect `read`. -- `pr-run-activity-write.yml` runs scripts only and fails red with `skill-in-write-runner` before the mint for any skill, not just an issue skill. - -### Removed - -- The write runner's skill path, which no config could reach: the claude-code-action step, its lane commit branch and promote step, the turn-cap, model-turn and gate-verdict checks, the skill reply upload, the lane-outcome record, and the unused `claude-code-oauth-token` secret. The write runner's report no longer carries a `lane-outcome`. -- The schema's head-test-check limits on a skill whose effect is not `read`, which no skill can now have. - ## [9.20.0] - 2026-11-11 ### Added From 52a50321ca2f9bce244c68b35b7b1fb90a0faff6 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Wed, 11 Nov 2026 10:48:58 -0500 Subject: [PATCH 3/4] fix(pr-pipeline): match the sandbox's pr-refine grants to its read skills pr-pipeline-sandbox#326 makes fix-marker a read skill and removes check-write-effect and audit-env-write; the central grant row follows. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01VQb9Q2ceZLXhTR4d1vkTCe --- .github/actions/resolve-config/lane-grants.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/actions/resolve-config/lane-grants.json b/.github/actions/resolve-config/lane-grants.json index e801aab9ed0..cb08ae8303a 100644 --- a/.github/actions/resolve-config/lane-grants.json +++ b/.github/actions/resolve-config/lane-grants.json @@ -22,7 +22,7 @@ "work-resolve-issue": { "resolve-issue": "read", "apply-issue-patch": "mutate-branch", "report-outcome": "publish-artifact" }, "pr-unblock": { "resolve-conflicts": "read", "apply-conflict-patch": "mutate-branch", "address-feedback": "read", "apply-feedback-patch": "mutate-branch", "publish-feedback": "publish-artifact", "review-dependency": "read", "apply-dependency-patch": "mutate-branch", "apply-dependency-verdict": "publish-artifact", "fix-ci": "read", "apply-patch": "mutate-branch", "report-escalation": "publish-artifact" }, "pr-run-checks": { "check-sandbox": "read", "check-token": "read", "check-hardening": "read", "check-trusted-text": "read", "check-read-token": "read" }, - "pr-refine": { "fix-marker": "mutate-branch", "check-write-effect": "mutate-branch", "audit-env-write": "mutate-tracker", "fix-ci": "read", "fix-docs": "read", "apply-patch": "mutate-branch" }, + "pr-refine": { "fix-marker": "read", "fix-ci": "read", "fix-docs": "read", "apply-patch": "mutate-branch" }, "pr-explain": { "explain-marker": "read" }, "pr-review": { "review-gate": "read", "audit-env": "read" } }, From 079aa51ce839bb022f423f6a449ee24c1da855ba Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Wed, 11 Nov 2026 15:16:54 -0500 Subject: [PATCH 4/4] docs(pr-pipeline): scope the lane record and skill setup to the read runner The write runner runs no skill, so it outputs no lane record and runs no subprocess isolation or setup-env install for one. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01VQb9Q2ceZLXhTR4d1vkTCe --- docs/conventions/pr-pipeline/pr-run-activity.md | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/docs/conventions/pr-pipeline/pr-run-activity.md b/docs/conventions/pr-pipeline/pr-run-activity.md index 4736e9698de..b41dba326ff 100644 --- a/docs/conventions/pr-pipeline/pr-run-activity.md +++ b/docs/conventions/pr-pipeline/pr-run-activity.md @@ -334,7 +334,7 @@ the broker request, and no other permission; it never holds `checks: write` or ` `plugins/source-control/scripts/resolve-version-bump-conflict.sh` to `$RUNNER_TEMP/merge-base-bin`, recording each copy's SHA-256 as a step output, and read `loop.no-progress` from the resolved config. -9. `setup-runner-isolation`: for a skill, installs bubblewrap and socat and, where +9. `setup-runner-isolation`: for a skill, which runs in the read file only, installs bubblewrap and socat and, where `/proc/sys/kernel/apparmor_restrict_unprivileged_userns` exists, sets it to `0`. It installs with `apt-get` from the image's own package lists, else after `apt-get update` (three tries), all within 540 s, under the image's own mirror failover settings. It uses no Actions cache: the @@ -343,17 +343,17 @@ the broker request, and no other permission; it never holds `checks: write` or ` 10. Removes sudo and docker access for the rest of the job: `/var/run/docker.sock` becomes root-only and the runner user's `/etc/sudoers.d/runner` entry is deleted. It fails red if `sudo -n true` still succeeds or the socket is still open to the runner user. -11. For a skill, copies the base's [`scripts/setup-env.sh`](../../../scripts/setup-env.sh) and the +11. For a skill (the read file only), copies the base's [`scripts/setup-env.sh`](../../../scripts/setup-env.sh) and the files it installs from (`.python-version`, `.github/requirements-ci.txt`, the root and `.github/standards/runner-policy` `package.json` and `package-lock.json`) to `$RUNNER_TEMP/setup-env`; a base without the script skips this and step 12's install with a notice. Then checks out the trigger gate's `head-sha` with `persist-credentials: false` (full history for a merge, else depth 1), then fails red unless `git rev-parse HEAD` equals that SHA. This replaces `.base`. -12. For a skill, sets up Python from the copied `.python-version` and runs the copied +12. For a skill (the read file only), sets up Python from the copied `.python-version` and runs the copied `setup-env.sh`, which installs the hash-pinned wheels in `.github/requirements-ci.txt` into a venv under `$RUNNER_TEMP` and the locked Node packages, puts both on `PATH`, and copies the - `node_modules` trees into the checkout. Both files install from the base copies in + `node_modules` trees into the checkout. It installs from the base copies in `$RUNNER_TEMP/setup-env`, so no head code runs before the skill's own Bash; a read skill that needs the head's own packages runs `npm ci` there. Then runs the activity (below) and, in the read file, records whether the tree is dirty, or @@ -394,7 +394,7 @@ outcome. In the read file a skill runs on run-claude-cli (`act-key` or `act-toke whose step succeeded is a failure when it took no model turn. The write file runs no skill, so its `act-outcome` is the script step's outcome. -Both files also output the lane record of a skill run: `record`, the lane-outcome JSON +The read file also outputs the lane record of a skill run (the write file runs no skill and outputs none): `record`, the lane-outcome JSON (ci-workflows `docs/lane-outcome.md`) that ci-workflows' `report-lane-outcome` builds from the execution file after the skill, naming the credential the leg ran on, or for a read leg that failed before the launcher named it (a CLI install), the secret the call was given. The report job