Skip to content

refactor(reviewing-prs): make skill the worker protocol - #14

Merged
SSFSKIM merged 2 commits into
mainfrom
refactor/reviewing-prs-skill-entrypoint
Jul 15, 2026
Merged

refactor(reviewing-prs): make skill the worker protocol#14
SSFSKIM merged 2 commits into
mainfrom
refactor/reviewing-prs-skill-entrypoint

Conversation

@SSFSKIM

@SSFSKIM SSFSKIM commented Jul 14, 2026

Copy link
Copy Markdown
Owner

Who is submitting this PR? (required)

Field Value
Your model + version Claude Opus 4.8 (claude-opus-4-8)
Harness + version Claude Code 2.1.203
All plugins installed See complete inventory below
Human partner who reviewed this diff SSFSKIM originated the restructuring and requested this PR. Complete diff review is pending; this is intentionally a draft PR.
Installed plugin inventory
  • agent-harness-workspace@agent-harness 0.1.0 (project, disabled)
  • agent-harness@agent-harness 0.2.0 (user, disabled)
  • agent-sdk-dev@claude-plugins-official unknown (user, enabled)
  • chrome-devtools-mcp@claude-plugins-official 1.5.0 (user, disabled)
  • claude-code-setup@claude-plugins-official 1.0.0 (user, enabled)
  • claude-md-management@claude-plugins-official 1.0.0 (user, enabled)
  • cloud-sql-postgresql@claude-plugins-official 0.4.0 (project, disabled)
  • cloudflare@claude-plugins-official 1.0.0 (user, disabled)
  • code-modernization@claude-plugins-official unknown (local, disabled)
  • code-review@claude-plugins-official unknown (user, disabled)
  • code-simplifier@claude-plugins-official 1.0.0 (user, enabled)
  • codex@openai-codex 1.0.6 (user, enabled)
  • commit-commands@claude-plugins-official unknown (project, disabled)
  • context7@claude-plugins-official unknown (local, enabled)
  • context7@claude-plugins-official unknown (user, enabled)
  • desktop-commander@claude-plugins-official 0.2.0 (user, disabled)
  • discord@claude-plugins-official 0.0.4 (local, disabled)
  • doperpowers@doperpowers 7.17.0 (user, enabled)
  • explanatory-output-style@claude-plugins-official 1.0.0 (user, enabled)
  • feature-dev@claude-plugins-official unknown (local, disabled)
  • firecrawl@claude-plugins-official 1.0.9 (user, disabled)
  • frontend-design@claude-plugins-official unknown (user, enabled)
  • learning-output-style@claude-plugins-official 1.0.0 (user, disabled)
  • linear@claude-plugins-official unknown (project, disabled)
  • mcp-server-dev@claude-plugins-official unknown (user, enabled)
  • microsoft-docs@claude-plugins-official 0.3.1 (user, disabled)
  • playground@claude-plugins-official unknown (project, disabled)
  • plugin-dev@claude-plugins-official unknown (user, enabled)
  • pr-review-toolkit@claude-plugins-official unknown (user, disabled)
  • pyright-lsp@claude-plugins-official 1.0.0 (user, enabled)
  • remember@claude-plugins-official 0.8.3 (user, enabled)
  • rust-analyzer-lsp@claude-plugins-official 1.0.0 (user, enabled)
  • security-guidance@claude-plugins-official 2.0.6 (user, enabled)
  • skill-creator@claude-plugins-official unknown (user, enabled)
  • sourcegraph@claude-plugins-official 0.1.0 (user, disabled)
  • supabase@claude-plugins-official 0.1.12 (user, enabled)
  • typescript-lsp@claude-plugins-official 1.0.0 (user, enabled)

What problem are you trying to solve?

The reviewing-prs skill entrypoint served the wrong primary audience. SKILL.md contained the setup and operations manual, while dispatched review workers bypassed the skill and received a separate references/review-worker-protocol.md template. That inverted the intended skill boundary: invoking doperpowers:reviewing-prs did not load the review worker's operative protocol.

This was identified while tracing where finding routing, tech-debt parking, decision-discipline review, self-merge, and confident-ready escalation actually lived. The protocol content itself was correct; its placement and loading path were not.

What does this PR change?

SKILL.md is now the canonical Review Worker Protocol, and the former operator-oriented skill body lives in references/operation-manual.md. review-dispatch.sh renders a thin per-PR bootstrap that explicitly invokes doperpowers:reviewing-prs and supplies every dynamic placeholder binding, while preserving the protocol text byte-for-byte.

Tests and living documentation now enforce and describe that boundary.

Is this change appropriate for the core library?

Yes. This restructures the core autonomous review loop's skill entrypoint and applies equally to every adopting repository and both supported worker species. It adds no project-specific configuration or dependency.

What alternatives did you consider?

  1. Leave the split unchanged. Rejected because the skill entrypoint would continue loading operator documentation while the actual worker protocol remained hidden in a supporting file.
  2. Render SKILL.md verbatim into every spawn prompt. Better file ownership, but the worker would still bypass skill invocation and static doctrine would still be copied into every prompt.
  3. Generate a per-PR dynamic skill copy. Rejected as unnecessary complexity. The static skill plus a thin runtime-binding bootstrap cleanly separates doctrine from instance context.

Does this PR contain multiple unrelated changes?

No. The dispatcher, tests, issue-tracker cross-reference, and living-spec notes all change only to support the single skill-entrypoint restructuring.

Existing PRs

PR #9 introduced the native review engine and current worker protocol behavior. It does not address the skill-entrypoint/operator-manual inversion; this PR preserves #9's engine and policy unchanged.

Environment tested

Harness Harness version Model Model version/ID
Claude Code 2.1.203 Claude Opus 4.8 claude-opus-4-8
Codex worker path (hermetic dispatcher/engine tests) repository test harness Stubbed worker + native-engine argument assertions N/A

New harness support (required if this PR adds a new harness)

Not applicable. No new harness is added.

Evaluation

  • Initial human prompt: "shouldn't skill itself be called by the worker ... swap protocol md <-> SKILL.md ... just restructure, no content change"
  • Baseline: the new structural test failed 19 assertions against the old layout, including that SKILL.md lacked the worker protocol and the dispatcher bypassed the skill entrypoint.
  • After the change: the structural test passes; the complete reviewing-prs dispatcher, review-engine, and land-dispatch suites pass; implementing-ticket protocol invariants and Codex sync regressions pass; all changed shell files pass shell lint.
  • Text-preservation check: the protocol portion of the new SKILL.md exactly equals origin/main's former review-worker-protocol.md; the operation manual exactly equals the former skill body except for the required protocol-location row.
  • Behavioral eval sessions after the change: 0. This is intentionally a structure-only move with byte-for-byte policy preservation. The executable boundary is covered by the RED/GREEN structural test and existing dispatcher tests.

Rigor

  • Used doperpowers:writing-skills and followed a RED/GREEN structure test before modifying the skill layout
  • Tested adversarial dispatcher cases already covered by the suite: stale/dead workers, sweep isolation, base-not-HEAD manifests, engine outage, ticketless PRs, engine switching, host/boot identity, and unresolved placeholders
  • Did not modify carefully tuned protocol content; an exact comparison against origin/main enforces this

Independent reviewer delegation was stopped after the native code-review path began abnormal recursive fan-out (43+ subagents). No result from that runaway review was relied upon; verification above is from direct commands and repository tests.

Human review

  • A human has reviewed the COMPLETE proposed diff before submission

This PR is opened as a draft specifically to provide the complete diff for that review. Do not mark ready or merge until this box can be checked truthfully.

… tail

Adversarial review of the entrypoint restructure found two defects the
file-based protocol introduced and one doc drift:

- The bootstrap told the worker to 'Use doperpowers:reviewing-prs' without
  pinning where the protocol resolves from. In the detached review worktree
  the workspace .agents/skills is PR-controlled, so a PR could ship a
  same-named skill replacing the merge rubric. The bootstrap now binds
  SKILL_FILE (dispatcher-owned absolute path), instructs the worker to open
  it unconditionally, and forbids workspace resolution.
- SKILL.md ended with unrendered '---- PR brief ----' template sections that
  nothing renders (dispatch renders the bootstrap, not SKILL.md). Removed;
  the protocol now points at the dispatch prompt for briefs and manifests.
- operation-manual.md still described the pre-#12 self-merge clause ('only
  low findings'); it now matches the protocol's non-blocker wording.

Regression asserts pin the SKILL_FILE binding, the spoofing rejection, the
tail's absence, and the manual wording.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant