Skip to content

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

Merged
kyle-sexton merged 6 commits into
mainfrom
fix/pr-pipeline-skill-effect-read
Nov 11, 2026
Merged

kyle-sexton merged 6 commits into
mainfrom
fix/pr-pipeline-skill-effect-read

Conversation

@kyle-sexton

@kyle-sexton kyle-sexton commented Nov 11, 2026 •

Copy link
Copy Markdown
Member

No related issue: owner-requested change to the PR pipeline config contract.

Summary

Every skill activity in docs/conventions/pr-pipeline.yaml has effect read and runs in pr-run-activity-read.yml. This makes that a rule: the schema and resolve-config refuse a skill whose effect is not read, and the write runner's skill path, which no config can then reach, is removed.

Fix

  • resolve.mjs checkLeaves: new rejection skill-not-read for a skill leaf whose effect is not read (rejection 17 in the resolve-config README).
  • pr-pipeline.schema.json: a leaf with skill must have effect: read; the edit-runner def and descriptions drop the non-read skill branch; no-head-test-check is removed.
  • pr-run-activity-write.yml: one pre-mint assert refuses any skill (skill-in-write-runner); removed the claude-code-action step and its lane branch, promote, turn-cap, model-turn, verdict, reply-upload, lane-record and .git removal steps, and the unused claude-code-oauth-token secret. act-outcome is the script step's outcome.
  • Fixtures, the example config and tests now model a skill as read plus applied-by.patch to an apply-patch script; tests that covered only the removed steps are removed or narrowed.
  • lane-grants.json: the sandbox's pr-refine row matches pr-pipeline-sandbox#326.
  • Docs: README, pr-run-activity.md and a .changes/pr-pipeline/ fragment (minor).

Verification

  • node --test --test-concurrency=4 over .github/actions/resolve-config/*.test.mjs, check-signed-commits, log-api-budget, lane-run-report post, label-unblocked, gate-source, publish-feedback, apply-patch, refire-refused and report-outcome tests: 2579 pass, 0 fail.
  • scripts/write-runner-never-rules.test.sh (75 pass), write-runner-hold-labels.test.sh (12), act-turns-denied.test.sh (34), write-runner-revoke-step, check-app-key-references, check-read-caller-keys, read-lane-broker-check, pr-unblock-trusted-base: all pass. shellcheck clean; actionlint reports no new findings.
  • Every skill in the live config is read: fix-docs, resolve-conflicts, address-feedback, review-dependency, fix-ci, resolve-issue, apply-changelog.
  • No caller of pr-run-activity-write.yml passes claude-code-oauth-token.

Follow-ups

  • Pre-existing (advisory): collect-base-activity's staging branch for a skill whose effect is not read (mutate-branch | mutate-tracker | publish-artifact | merge, with the commit tools and the can-commit prompt) is now unreachable. Removing it rewrites most of the 98 tests in collect-base-activity.test.mjs, whose default stage is a mutate-branch skill.
  • Pre-existing (advisory): the write runner's required role input is now unused. Removing it touches all 15 caller jobs here and the sandbox's synced callers at once, plus the role-consistency checks in callers.test.mjs.

Related

  • fix(pr-refine): run every refine skill with effect read pr-pipeline-sandbox#326 (merged) made the sandbox's refine skills read, so this change refuses no caller's config: the broker probe (loadConfig with this branch's resolve-config over the sandbox main's broker files) returns ok.
  • Callers of resolve-config: this repository and, through the lanes token broker (azure-iac ADR 0002), pr-pipeline-sandbox (the two repositories in lane-grants.json).

🤖 Generated with Claude Code

https://claude.ai/code/session_01VQb9Q2ceZLXhTR4d1vkTCe

kyle-sexton and others added 2 commits November 11, 2026 10:41
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VQb9Q2ceZLXhTR4d1vkTCe
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VQb9Q2ceZLXhTR4d1vkTCe
…ills

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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VQb9Q2ceZLXhTR4d1vkTCe
@kyle-sexton kyle-sexton removed the do-not-merge Hard merge gate: do not merge while applied. label Nov 11, 2026
@kyle-sexton
kyle-sexton marked this pull request as ready for review November 11, 2026 20:08
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Nov 11, 2026 •

Copy link
Copy Markdown
Codex Review Summary: ✅ Completed

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-11-11T20:10:00.352296Z 230ffbe Draft marked ready
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@github-actions

github-actions Bot commented Nov 11, 2026 •

Copy link
Copy Markdown
Contributor

claude-security-review reviewed a545d1aa50b9a3a5dad9328580fcb977b75bb622: read 22 of 22 changed file(s); 0 finding(s), 0 as inline comments.

Security review: PR #15393, no findings

I found no security issues. The change narrows what the automation trusts and widens nothing. This lane already reviewed an earlier head (230ffbef), so this pass was blocking only: it reports CRITICAL and IMPORTANT findings and drops SUGGESTIONs. It found neither.

critical-findings: 0

Trust controls checked

resolve-config (the token broker's signer inputs)

  • checkLeaves in resolve.mjs#L214-L222 adds the skill-not-read rejection. It refuses any skill leaf whose effect is not read. A new refusal can only narrow what the broker mints.
  • The invalid-prepare message changed. Its condition did not.
  • In lane-grants.json, the sandbox pr-refine row moves fix-marker from mutate-branch to read and drops check-write-effect (mutate-branch) and audit-env-write (mutate-tracker). That narrows the grants.

Schema (pr-pipeline.schema.json)

  • The new allOf rule (if required skill then effect const read) only adds a refusal.
  • edit-runner used to accept "effect not read, or applied-by.patch". It now requires applied-by.patch, which is stricter.
  • no-head-test-check and the verify-scripts enum ban are removed. Both applied only to a skill whose effect was not read. That case is now refused by the schema and again by the reader, so no write-token job can run head test files through them. Read skills could already list those scripts at base.

pr-run-activity-write.yml

  • The skill refusal now fires for every skill, not only in issue mode: if: steps.config.outcome == 'success' && steps.config.outputs.kind == 'skill'.

  • The refusal still runs before the lane-token mint, and the updated callers.test.mjs / write-runner-never-rules.test.sh assertions pin that order.

  • Removed from the write runner:

    • the claude-code-action step and the claude-code-oauth-token secret;
    • the lane-branch createRef and the promote updateRef;
    • the step that copied the lane token into the skill's settings;
    • the setup-env install and the reply upload.

    No model now runs beside the lane token, which leaves less attack surface.

  • act-outcome is now just steps.act-script.outcome, a base script's result.

  • The hold-label step now runs only for prepare == 'apply-patch'. The kind == 'skill' branch it dropped fed only the removed promote step. apply-patch still gets its hold labels through HOLD_LABELS, so no merge hold is weakened.

  • setup-runner-isolation no longer installs subprocess isolation in this file. Only the removed skill step needed it, and the sudo/docker removal and the rest of that action still run.

Untrusted text reaching a lane

  • No new path. Skills still run only in pr-run-activity-read.yml, which holds no write token.

The docs, tests, fixtures, example config and .changes fragment change no runtime behaviour, and they match the code above.

Settled and prior

  • The settled thread on pr-run-activity.md (lane-record wording) is marked lines changed since resolution: no and was not raised again.
  • The earlier review posted no findings, so none were repeated.

Embedded text

  • None. The PR context file contains no directives aimed at the review lane.

Coverage

  • I read every hunk of all 22 listed files, plus the two whole-file deletions: scripts/write-runner-over-cap-success.test.sh and scripts/write-runner-promote-holds.test.sh.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

claude-review: 1 inline finding(s) on 230ffbefd4279c4f7e181bc54236401d233768dd; the summary is in this lane's pull request comment.

claude-lane: claude-review

@github-actions

github-actions Bot commented Nov 11, 2026 •

Copy link
Copy Markdown
Contributor

claude-review reviewed a545d1aa50b9a3a5dad9328580fcb977b75bb622: read 22 of 22 changed file(s); 0 finding(s), 0 as inline comments.

Code review (blocking only): PR #15393

Scope. This lane already reviewed earlier heads of this PR, so this round looks for blocking findings only. I read every hunk of all 22 listed files in .claude-lane/pr.diff. I also read the hunks of the two test files the PR deletes (scripts/write-runner-over-cap-success.test.sh, scripts/write-runner-promote-holds.test.sh).

No blocking findings. I checked the four places the removal is most likely to break something:

  • No leftover references in the write runner. pr-run-activity-write.yml at head has no steps.* expression pointing at a step this PR removes: act-skill, promote, lane-branch, setup-env, act-turns-bin, act-turns, credential, lane-record, act-result, skill-verdict, skill-settings or scan-upload. The two scan steps that remain, scan-upload-2 and scan-upload-3, are still defined and still read $RUNNER_TEMP/scan-gate.
  • Removing the claude-code-oauth-token secret breaks no caller here. Every caller job in this repository that passes that secret calls pr-run-activity-read.yml; none of the write-runner calls in work-resolve-issue.yml, pr-refine.yml or pr-unblock.yml passes it. Callers in the sandbox repository are outside what I can read; the PR says pr-pipeline-sandbox#326 already moved them, and I have not checked that.
  • Dropping with: from "Isolate the runner" changes nothing. setup-runner-isolation defaults install-subprocess-isolation to "false", so the call behaves as before for a script activity.
  • The new rule and its tests line up. The new schema rule (if: required: [skill] then effect: const: read) and the new skill-not-read check in resolve.mjs checkLeaves match each other. The new test in resolve.test.mjs covers both: it runs once with the full schema and once with that one schema rule removed. The refusal step now matches any skill, issue mode or not, and it still sits before the token mint; callers.test.mjs and write-runner-never-rules.test.sh pin both.

Earlier finding. The thread at pr-run-activity.md line 397 is resolved, the owner replied with the fix, and its lines have not changed since, so I have not raised it again.

Embedded text. The PR context, settled-threads and prior-findings files contain no instructions aimed at this lane.

Verification. I ran nothing. The test pass counts and the sandbox broker probe in the PR description are the author's claims, and I have not checked them.

Manifest. No .claude-plugin/plugin.json or marketplace.json changes, so there are no manifest lines.

kyle-sexton and others added 2 commits November 11, 2026 15:16
…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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VQb9Q2ceZLXhTR4d1vkTCe
@kyle-sexton
kyle-sexton added this pull request to the merge queue Nov 11, 2026
Merged via the queue into main with commit 7b2631c Nov 11, 2026
53 checks passed
@kyle-sexton
kyle-sexton deleted the fix/pr-pipeline-skill-effect-read branch November 11, 2026 23:30
@github-actions github-actions Bot added the merge-path: owner-agent Post-merge accounting record (README.md, Merge paths) label Nov 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-path: owner-agent Post-merge accounting record (README.md, Merge paths)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant