fix(pr-pipeline): reject skill activities whose effect is not read

.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.

.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);

.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, <dir> is where the copy is written and <at> where it runs.

.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

.github/actions/resolve-config/callers.test.mjs

@@ -1951,66 +1951,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/<pid>/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.
@@ -3113,21 +3085,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);
   }
@@ -4673,43 +4643,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.
@@ -9571,7 +9504,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);
@@ -9580,9 +9513,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'/);
 });
 

.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]

.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" }
     },

.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",
   );
 });