Skip to content

feat(skills): adversarial review before every PR a skill opens - #1372

Merged
potiuk merged 3 commits into
mainfrom
feat/adversarial-review-pre-pr
Sep 24, 2026
Merged

potiuk merged 3 commits into
mainfrom
feat/adversarial-review-pre-pr

Conversation

@potiuk

@potiuk potiuk commented Sep 24, 2026

Copy link
Copy Markdown
Member

PR 3 of 4, stacked on #1371. This PR's base is feat/adversarial-review-setup, so the diff shows only PR 3. Merge #1371 first; this PR then retargets to main.

Summary

  • A shared pre-PR block (tools/dev/blocks/pre-pr-adversarial-review.md) in every skill that opens a PR, placed where the PR's title and body are final and before the push where the flow allows. Eleven skills carry it, and release-prepare carries it at each of its three PRs. security-issue-import-from-pr, which verifies a patch someone else proposed, reviews that PR.
  • Reviewers see only what the PR will publish: the diff, plus the title and body as posted, after the skill's own public-surface checks.
    • The tool refuses a --repo-dir that is the project's private tracker, since reviewers can read every file there. --allow-tracker-checkout opts out when the tracker's own code is under review.
    • A skill without an upstream checkout reviews --target pr:<N> from an empty temp directory.
    • A change that isn't a local branch is reviewed as a diff file.
  • When it runs. Security-family skills run whenever at least one reviewer is configured, whatever the mode. Other skills run only under mode: on-pr-create. The review is advisory: unavailable reviewers never stop the flow, a finding the human wants fixed sends the flow back, and findings are untrusted data.
  • Validator rule. The skill validator fails a PR-opening skill that lacks the block. It scans Markdown and scripts, including the Python argv form, and keeps explicit, reasoned exemption and delegation lists.

Type of change

  • Skill change (.claude/skills/<name>/) — eval fixtures updated below
  • Python package (tools/*/ with pyproject.toml)
  • CI / dev loop (prek, workflows, validators)

Test plan

  • prek run --all-files passes
  • For Python packages touched: tools/adversarial-review (143 tests) and tools/skill-and-tool-validator (478) pass pytest, ruff check and mypy
  • For skill behaviour changes, new eval suites:
    • security-issue-fix/step-7-adversarial-review (6 cases): runs under mode: off with the posted title and body file, never the tracker's title or a draft that quotes the tracker; a silent skip without config; a one-line skip without the plugin; an instruction inside a finding is not followed; all reviewers unavailable still continues; a finding the user wants fixed stops the flow.
    • setup-upstream-fix/step-5-adversarial-review (4 cases): on-pr-create runs; on-demand, off and an empty reviewer list skip silently.
  • Other:
    • validate_pre_pr_review_block reports 0 violations on the tree. Unit tests cover prose, sibling files, scripts, the exemption and the delegation.
    • A test pins the block's invocation to the sandbox exclusion pattern.
    • The tool tests cover the tracker-checkout refusal and its opt-out.

RFC-AI-0004 compliance

  • HITL: advisory only. The human decides what to fix, the PR flow is never blocked, and each run keeps its harness prompt.
  • Privacy LLM: no private content reaches another model. The input is only what the PR publishes, the tracker checkout is refused, and the host skills run their public-surface checks first.
  • Write-access discipline: the block creates nothing; it runs a read-only review before the host skill's own confirmed PR step.

Linked issues

Stacked on #1371; follows #1368.

Notes for reviewers (optional)

  • Existing evals: several existing step evals now include the block's text in their extracted step, among them release-announce-draft/step-3-site-bump, security-issue-import-from-pr/step-6-confirm and setup-override-upstream/step-6-pr-confirm. Their fixtures carry no reviewer config, so the block is a silent skip there. They are worth a live re-run.
  • Release-management steps: the block says the review adds nothing to the JSON these steps return.

🤖 Generated with Claude Code

@potiuk potiuk added family:tools tools/* capability:review Deep per-item code review or contributor mentoring labels Sep 24, 2026
@potiuk
potiuk added this pull request to stack #1374 September 24, 2026 16:52
@potiuk
potiuk force-pushed the feat/adversarial-review-pre-pr branch from f396aa4 to b34496d Compare September 24, 2026 17:05
Base automatically changed from feat/adversarial-review-setup to main September 24, 2026 17:14
@potiuk
potiuk force-pushed the feat/adversarial-review-pre-pr branch from b34496d to 84b18db Compare September 24, 2026 17:14
A shared block (tools/dev/blocks/pre-pr-adversarial-review.md) in each of the
ten skills that run `gh pr create`, and in security-issue-import-from-pr,
which verifies a patch someone else proposed. It runs the configured
reviewers over the diff and the PR title and body exactly as they will be
posted — never tracker, CVE, reporter or mail content — shows the findings
as untrusted, advisory data, and never blocks the flow. The security family
runs it whenever a reviewer is configured, whatever the mode.

The skill validator now fails a PR-opening skill that lacks the block, with
an explicit, reasoned exemption list. Eval suites:
security-issue-fix/step-7-adversarial-review (5 cases) and
setup-upstream-fix/step-5-adversarial-review (2).

Generated-by: Claude Opus 5
… matches

The block now uses the unquoted, literal-~ plugin path with <version> — the
form PR 2 settled on — and says the body file must sit in the checkout or a
temporary directory. A test pins the block's invocation to the sandbox
exclusion pattern.

Generated-by: Claude Opus 5
- the tool refuses a --repo-dir that is the project's tracker (reviewers
  can read every file there) unless --allow-tracker-checkout says the
  tracker's own code is under review
- the block: pass the title and body as posted, after the skill's own
  public-surface checks — allowed identifiers stay, private content never
  goes; --repo-dir is never the tracker (an empty temp dir for pr: with no
  checkout); a change that is not a local branch is reviewed as a diff
  file; the review adds nothing to a step's JSON; a finding the human
  wants fixed sends the flow back
- hosts: issue-fix runs its 5c check first; model-verify and model-prepare
  review the helper's dry-run diff; import-from-pr uses an empty temp dir;
  import-from-scan reviews the scrubbed report inside the report-back
  bullet; release-prepare gets the block at 2f and 14b too
- validator: also scans skill scripts (argv form included) and lists
  skills that open PRs through another skill's helper
- evals check the body file passed and cover the stop, off and empty cases

Generated-by: Claude Opus 5
@potiuk
potiuk force-pushed the feat/adversarial-review-pre-pr branch from 84b18db to 623a192 Compare September 24, 2026 21:44
@potiuk
potiuk merged commit 88339a0 into main Sep 24, 2026
12 checks passed
@potiuk
potiuk deleted the feat/adversarial-review-pre-pr branch September 24, 2026 21:50
@potiuk potiuk added substrate:framework-dev Tool substrate: build / validate / eval the framework itself substrate:review Tool substrate: second-model read of a change before publishing (read-only CLIs, merged findings) labels Sep 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

capability:review Deep per-item code review or contributor mentoring family:tools tools/* substrate:framework-dev Tool substrate: build / validate / eval the framework itself substrate:review Tool substrate: second-model read of a change before publishing (read-only CLIs, merged findings)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant