Refactor review-pr to use dedicated agents and server-owned execution #4451

Context

review-pr/SKILL.md has grown to roughly 1,735 lines and 80 KB. About 769 lines are inside fenced shell, Python, or JSON blocks. The embedded standard and deletion-review prompt templates occupy another roughly 90 lines.

The skill was about 930 lines before the deterministically gated overengineering-auditor work. That change added two proper custom auditors, but it also expanded the parent skill substantially with snapshot validation, candidate validation and adjudication, aggregation, publication, and effect-handling instructions.

The result is that one skill is currently serving several different purposes:

• high-level review orchestration;
• static role prompts for the arch, tests, defense, bugs, cohesion, slop, and deletion-regression reviewers;
• an executable-looking specification for diff and snapshot validation;
• finding validation, suppression, deduplication, and verdict logic;
• artifact publication and GitHub request/fallback handling.

The reachability and abstraction-surface reviewers already use dedicated registered agent definitions. The standard six dimensions and deletion-regression reviewer still use generic agents whose standing instructions are assembled inside review-pr.

There is also already real package code for parts of the deterministic workflow, including experimental-auditor validation, aggregation, verdict selection, and publication. Despite that, the skill continues to describe much of the behavior as code-like prompt content.

Direction

Refactor the remaining review dimensions into dedicated bundled agent definitions, following the pattern used by the newer specialized auditors. Static role guidance, tool restrictions, output shape, severity guidance, and dimension-specific review behavior should live with those agents. The parent skill should provide only the context that changes for a particular review, such as the diff authority, prior findings, snapshot identity, and other invocation data.

Move deterministic processing out of SKILL.md. If the workflow needs code to validate snapshots or artifacts, select dispatches, validate agent output, aggregate and suppress findings, calculate a verdict, publish artifacts, or construct and account for GitHub mutations, that behavior should live in server/package code exposed through supported callable or tool boundaries. The skill should call those boundaries rather than asking a model to interpret shell or Python pseudocode.

After the refactor, review-pr/SKILL.md should primarily explain the review sequence, the important safety and authority boundaries, and how to invoke the dedicated agents and server-owned operations. It should not duplicate static agent instructions or carry implementation blocks that are better expressed as tested code.

Tests that currently pin literal prose and pseudocode in the skill should move toward testing the agent definitions, callable interfaces, and important workflow behavior. This should preserve the useful review modes, diff scoping, stale-snapshot protections, verdict routing, and GitHub safety behavior without requiring an exact reproduction of individual model findings.

The implementation should have room to choose sensible agent granularity and server API boundaries. This issue is about establishing clear ownership and removing prompt-embedded implementation, not prescribing a particular line count or forcing a specific internal decomposition.

Related Work

• Migrate make-plan adversarial agents from MCP resource reads to native subagent_type invocation #2601 established the native custom-agent invocation pattern for another workflow.
• resolve-review SKILL.md inline Python blocks cause /tmp scratch writes #3375 addressed inline Python implementation blocks in a review-related skill.
• Eliminate Inline Diff Embedding in review_pr — Pass File Path to Subagents #3636 addressed diff delivery to review subagents.
• Add deterministically gated overengineering auditors to review-pr #4421 introduced the deterministically gated overengineering auditors and much of the current orchestration surface.

Review-quality evidence: PR #4564 (2026-08-12)

The iteration-0 local review handoff for PR #4564 produced 83 aggregated findings. Independent validation against the identity-matching HEAD classified 8 as actionable, 73 as rejected/stale, and 2 as DISCUSS (duplicate presentations of one design decision). The accepted dimensional-bound decision was subsequently documented in ADR 0005 by commit f5bd402a7485e90ca8d06495936c2c6740003fe9.

Durable artifact bundle: https://gist.github.com/Trecek/82a8391c869e6a2fad4572da802db4c1

The Gist preserves the complete local_findings and paired diff_context, plus pre-change analysis, final resolve report, rejected-pattern ledger, deferred DISCUSS entries, and the ReviewPR summary.

Original absolute artifact paths

• /home/talon/projects/worktrees/impl-rectify-recipe-delivery-budget-reconciliation-20260811-100109/.autoskillit/temp/review-pr/local_findings_4564.json
• /home/talon/projects/worktrees/impl-rectify-recipe-delivery-budget-reconciliation-20260811-100109/.autoskillit/temp/review-pr/diff_context_4564.json
• /home/talon/projects/worktrees/impl-rectify-recipe-delivery-budget-reconciliation-20260811-100109/.autoskillit/temp/review-pr/summary_4564_20260812T153709Z.md
• /home/talon/projects/worktrees/impl-rectify-recipe-delivery-budget-reconciliation-20260811-100109/.autoskillit/temp/resolve-review/analysis_4564_20260812-155436.md
• /home/talon/projects/worktrees/impl-rectify-recipe-delivery-budget-reconciliation-20260811-100109/.autoskillit/temp/resolve-review/report_4564_20260812-161533.md
• /home/talon/projects/worktrees/impl-rectify-recipe-delivery-budget-reconciliation-20260811-100109/.autoskillit/temp/resolve-review/reject_patterns_4564.json
• /home/talon/projects/worktrees/impl-rectify-recipe-delivery-budget-reconciliation-20260811-100109/.autoskillit/temp/resolve-review/deferred_observations_4564.json

Rejected-finding patterns

1. Design intent missed: deliberate multipage policy, attestation headroom, capability probing, and static dimensional typing were reported as defects without reconciling current history or ADRs.
2. Contract ledgers mistaken for brittle duplication: exact schema, mode, provenance, and allowlist tests were treated as redundant even though they intentionally pin complete contracts.
3. Registration/façade imports mistaken for dead code: reviewers did not account for import-time MCP registration, package façades, or consumers outside the immediate file.
4. Existing isolation mechanisms overlooked: findings alleged global-state leakage where fixtures, monkeypatch, or try/finally already restored state.
5. Stale or factually incorrect claims: several findings targeted moved code, the wrong dependency, or behavior absent at the handoff HEAD.
6. Style elevated without a reachable impact: naming, docstring, private-helper, test-placement, and narrowing observations were promoted without demonstrating a correctness or material maintenance failure.
7. Duplicates survived aggregation: duplicate findings are expected from independent auditors, but materially identical claims reaching the final handoff indicate the aggregation key is too anchor/message-sensitive. The dimensional-bound pair and flow-record-normalization pair are concrete examples in the bundle.

Recommended implications for this refactor

• Require bounded history/intent evidence before escalating a deliberate semantic change.
• Teach dedicated agents to recognize contract-ledger, registration-side-effect, façade, and test-isolation patterns.
• Require a reachable counterexample for bug/critical classifications.
• Re-anchor every finding to the exact handoff HEAD before aggregation.
• Deduplicate by affected symbol plus normalized claim, not only path/line/message identity; preserve auditor provenance on the winning record.
• Treat uncertainty as a reason to gather evidence, not automatically as requires_decision=true (24 of the 73 rejected findings carried that flag).

Review-quality evidence: PR #4590 (2026-08-15)

The iteration-0 local handoff for PR #4590 produced 31 findings: 1 critical, 23 warning, and 7 info. Identity-matched validation against handoff HEAD e77cd48977f3b35bfe33369713e2b7c6245ccb83 classified 2 warning findings as ACCEPT, 22 findings as REJECT, and no findings as DISCUSS. Three additional INFO findings were independently validated and addressed. The final branch HEAD e1152517ca68f4fd91324326fabb61b0110a5154 passed 37,188 tests and full pre-commit.

Original absolute artifact paths

• /home/talon/projects/worktrees/impl-rectify_audit_parent_authority_2026-08-14_144010-20260814-161228/.autoskillit/temp/review-pr/local_findings_4590.json
• /home/talon/projects/worktrees/impl-rectify_audit_parent_authority_2026-08-14_144010-20260814-161228/.autoskillit/temp/review-pr/diff_context_4590.json
• /home/talon/projects/worktrees/impl-rectify_audit_parent_authority_2026-08-14_144010-20260814-161228/.autoskillit/temp/review-pr/raw_findings_4590.json
• /home/talon/projects/worktrees/impl-rectify_audit_parent_authority_2026-08-14_144010-20260814-161228/.autoskillit/temp/review-pr/consolidated_findings_4590.json
• /home/talon/projects/worktrees/impl-rectify_audit_parent_authority_2026-08-14_144010-20260814-161228/.autoskillit/temp/review-pr/summary_4590_20260815T030650Z.md
• /home/talon/projects/worktrees/impl-rectify_audit_parent_authority_2026-08-14_144010-20260814-161228/.autoskillit/temp/resolve-review/analysis_4590_20260815-032749.md
• /home/talon/projects/worktrees/impl-rectify_audit_parent_authority_2026-08-14_144010-20260814-161228/.autoskillit/temp/resolve-review/report_4590_20260815-032749.md
• /home/talon/projects/worktrees/impl-rectify_audit_parent_authority_2026-08-14_144010-20260814-161228/.autoskillit/temp/resolve-review/reject_patterns_4590.json
• /home/talon/projects/worktrees/impl-rectify_audit_parent_authority_2026-08-14_144010-20260814-161228/.autoskillit/temp/resolve-review/deferred_observations_4590.json
• /home/talon/projects/worktrees/impl-rectify_audit_parent_authority_2026-08-14_144010-20260814-161228/.autoskillit/temp/resolve-review/precommit_4590.log
• /home/talon/projects/worktrees/impl-rectify_audit_parent_authority_2026-08-14_144010-20260814-161228/.autoskillit/temp/resolve-review/push_4590.log

Rejected-finding category distribution

Fifteen of 22 rejects (68.2%) came from intentional contract/design semantics being read as defects. Five more (22.7%) came from architecture or API-direction misunderstandings. Only two (9.1%) were uncategorized factual/speculative misses.

Rejected-finding patterns

1. Capability-declared backend asymmetry mistaken for inconsistency: Codex must materialize declared MCP-forward variables, while Claude has no corresponding config-forward capability. Two substantially identical findings proposed removing the required Codex empty sentinel.
2. Never-raises transport translation mistaken for layer coupling: three findings targeted the same authority-mismatch branch, overlooking that the exception is an IL-0 core contract deliberately translated into a sanitized IL-3 MCP response.
3. Worker isolation and pytest semantics overlooked: findings alleged xdist leakage from PATH/chdir/class monkeypatches even though workers are separate processes and monkeypatch restores state; another incorrectly claimed dotted monkeypatch.setattr silently no-ops despite raising=True being the default.
4. Fail-closed authority semantics misread: exact supplied path/owner identity and immutable expected ownership were reported as security gaps even though access-time checks reject symlink and ownership drift.
5. Distinct layer tests mistaken for duplication: the same stale-handle scenarios intentionally validate ledger behavior at IL-1 and sanitized response behavior at IL-3.
6. Maintenance/performance claims lacked reachable impact: suggested caching, timeout increases, test splitting, and redundant annotations did not demonstrate a hot path, observed failure, or simpler maintained result.
7. Duplicate claims survived aggregation: the Codex sentinel pair and the three authority-mismatch findings reached the final handoff separately despite sharing the same affected symbol and normalized claim.

Recommended implications for this refactor

• Put backend capability contracts and Never-raises transport-boundary guidance directly in the dedicated auditor definitions.
• Require auditors to test xdist/process-isolation and monkeypatch semantics before escalating shared-state findings.
• Require an observed or reachable performance/failure case before recommending caching, timeout expansion, or test decomposition.
• Teach security reviewers to distinguish immutable expected authority from access-time observed identity; both are required for fail-closed drift detection.
• Deduplicate by affected symbol plus normalized behavioral claim while preserving every auditor's provenance.
• Promote this 22-reject corpus into agent-eval canaries: backend asymmetry, transport translation, xdist isolation, cross-layer test intent, and fail-closed identity are concrete contrastive examples.

Review-quality evidence: PR #5050 (2026-09-15)

The iteration-0 local handoff for PR #5050 produced 23 findings: 1 critical, 9 warning, and 13 info. Identity-matched validation against handoff HEAD 91e68c68432e3ba2d1a1cc7c9b71ab68e83abd1d classified 1 warning finding as ACCEPT, 22 findings as REJECT, and no findings as DISCUSS. All 13 INFO findings were independently considered for correctness and maintainability value; none warranted a change. The accepted coverage fix was committed as 12b93271583ed99af0b38ccd7ddafd8d9e26028c, and the final branch passed 36,328 tests.

Durable artifact bundle: https://gist.github.com/Trecek/a5aff1a0bba3c80f6ac475edf46ad620

The Gist preserves the complete local_findings, paired diff_context, raw findings, ReviewPR summary, pre-change analysis, final resolve report, rejected-pattern ledger, classification map, and deferred-observation file.

Original absolute artifact paths

• /home/talon/projects/worktrees/impl-issue-5028-path-b-codex-discovery-attestation-20260915-081356/.autoskillit/temp/review-pr/local_findings_5050.json
• /home/talon/projects/worktrees/impl-issue-5028-path-b-codex-discovery-attestation-20260915-081356/.autoskillit/temp/review-pr/diff_context_5050.json
• /home/talon/projects/worktrees/impl-issue-5028-path-b-codex-discovery-attestation-20260915-081356/.autoskillit/temp/review-pr/raw_findings_5050.json
• /home/talon/projects/worktrees/impl-issue-5028-path-b-codex-discovery-attestation-20260915-081356/.autoskillit/temp/review-pr/summary_5050_20260915T175451Z.md
• /home/talon/projects/worktrees/impl-issue-5028-path-b-codex-discovery-attestation-20260915-081356/.autoskillit/temp/resolve-review/analysis_5050_20260915-181023.md
• /home/talon/projects/worktrees/impl-issue-5028-path-b-codex-discovery-attestation-20260915-081356/.autoskillit/temp/resolve-review/report_5050_20260915-181904.md
• /home/talon/projects/worktrees/impl-issue-5028-path-b-codex-discovery-attestation-20260915-081356/.autoskillit/temp/resolve-review/reject_patterns_5050.json
• /home/talon/projects/worktrees/impl-issue-5028-path-b-codex-discovery-attestation-20260915-081356/.autoskillit/temp/resolve-review/classification_5050.json
• /home/talon/projects/worktrees/impl-issue-5028-path-b-codex-discovery-attestation-20260915-081356/.autoskillit/temp/resolve-review/deferred_observations_5050.json

Rejected-finding category distribution

Fifteen of 22 rejects (68.2%) came from intentional contract or design semantics being read as defects—the same share observed in PR #4590. Most of the corpus therefore reinforces the existing evidence above rather than adding a new broad trend.

New rejected-finding patterns

1. Surface similarity mistaken for semantic equivalence: several findings recommended unifying names, field types, or return shapes without tracing that the values belong to different ownership or lifecycle stages. Examples include the pinned upstream legacy_root_relpath versus an invocation-specific expected_discovery_root, binding-level skill_entries versus command-level projected_skill_entries, and origin-extraction results versus downstream validation errors.
2. Validation locality mistaken for validation absence: two findings described passive transport dataclasses as defenseless even though canonical entries are validated at authoritative construction and revalidated at the execution trust boundary. Neither finding demonstrated a path by which malformed entries could bypass both boundaries and reach Codex.
3. Terminology-only concern escalated to critical: the round's sole critical finding alleged a partial naming migration but identified no behavioral contradiction or reachable maintenance failure. This reinforces the existing requirement for bug/critical findings to provide a reachable counterexample rather than severity derived from wording alone.

Recommended implications for this refactor

• Before recommending that names, field types, or return shapes be unified, require a short producer/owner/consumer trace showing that the values have the same semantic role across lifecycle stages.
• Before requesting validation inside another record type, require a validation-custody trace and a concrete bypass showing malformed data can reach a consumer without crossing an existing authoritative boundary.
• Treat terminology consistency as critical only when the mismatch creates a reachable behavioral, contract, or maintenance failure; otherwise classify it as warning or info.
• Add contrastive agent-eval canaries for upstream-layout versus invocation-expectation naming, binding evidence versus command evidence, and authoritative-boundary validation versus passive-container validation.

Review-quality evidence: PR #5076 (2026-09-17)

The iteration-3 local handoff for PR #5076 produced 161 findings: 2 critical, 56 warning, and 103 info. Identity-matched validation against handoff HEAD 90c01eb22310c3ae7ef3bcfd0dfe54ad3f38e2ee classified 1 info finding as ACCEPT and 160 findings as REJECT (148 false_positive_intentional_pattern, 12 design_intent_misread); no findings were DISCUSS. All 161 round-3 candidates were unpostable because the new strict DiffAnchorAuthority line-admission gate rejects any (path, line) tuple outside the diff's right-side admitted set; this is a structural property of the round-3 reviewer, not an indicator of invalid concern. The accepted fix was committed as a87a7f004 and the final branch HEAD passed 48,106 tests and full pre-commit.

Durable artifact bundle: https://gist.github.com/Trecek/5c513ff55082c9a6b5ec708ca55a89b9

The Gist preserves the complete local_findings, paired diff_context, raw findings, round-3 summary, classification map, reject-pattern ledger, pre-change analysis, final resolve report, and the empty deferred-observation file.

Original absolute artifact paths

• /home/talon/projects/worktrees/impl-rectify_review_anchor_publication_immunity_2026-09-16_182232-20260916-221440/.autoskillit/temp/review-pr/round-3/local_findings_5076.json
• /home/talon/projects/worktrees/impl-rectify_review_anchor_publication_immunity_2026-09-16_182232-20260916-221440/.autoskillit/temp/review-pr/round-3/diff_context_5076.json
• /home/talon/projects/worktrees/impl-rectify_review_anchor_publication_immunity_2026-09-16_182232-20260916-221440/.autoskillit/temp/review-pr/round-3/raw_findings_5076.json
• /home/talon/projects/worktrees/impl-rectify_review_anchor_publication_immunity_2026-09-16_182232-20260916-221440/.autoskillit/temp/review-pr/round-3/summary_5076_round3.md
• /home/talon/projects/worktrees/impl-rectify_review_anchor_publication_immunity_2026-09-16_182232-20260916-221440/.autoskillit/temp/review-pr/round-3/final_verdict.json
• /home/talon/projects/worktrees/impl-rectify_review_anchor_publication_immunity_2026-09-16_182232-20260916-221440/.autoskillit/temp/resolve-review/analysis_5076_20260917T173000Z.md
• /home/talon/projects/worktrees/impl-rectify_review_anchor_publication_immunity_2026-09-16_182232-20260916-221440/.autoskillit/temp/resolve-review/report_5076_20260917T163000Z.md
• /home/talon/projects/worktrees/impl-rectify_review_anchor_publication_immunity_2026-09-16_182232-20260916-221440/.autoskillit/temp/resolve-review/reject_patterns_5076.json
• /home/talon/projects/worktrees/impl-rectify_review_anchor_publication_immunity_2026-09-16_182232-20260916-221440/.autoskillit/temp/resolve-review/classification_5076.json
• /home/talon/projects/worktrees/impl-rectify_review_anchor_publication_immunity_2026-09-16_182232-20260916-221440/.autoskillit/temp/resolve-review/deferred_observations_5076.json

Rejected-finding category distribution

148 of 160 rejects (92.5%) came from audit false positives on intentional code. This is the highest false-positive share in the four-corpus sample (#4564, #4590, #5050, #5076), driven by the new strict anchor-admission gate and its interaction with audit prompts.

Rejected-finding patterns

1. Stdlib-semantic misunderstanding cluster (NEW PATTERN): five duplicate findings on _aggregation.py:65 claimed isinstance(error, str) and not isinstance(error, AnchorAdmission) reduces to isinstance(error, str) because "AnchorAdmission and str are mutually exclusive." They are NOT — AnchorAdmission is a StrEnum, so its instances ARE str instances. The audit repeatedly asserted a type-system claim that is provably false at runtime. Two further critical findings made the same kind of stdlib-semantic mistake: _aggregation.py:240 claimed positional-binding failure on a function declared with * (keyword-only-forced — positional binding is unreachable); _review.py:327 claimed "tuple unpacking assumes 2-tuple" on a code site that already correctly unpacks the 2-tuple returned by the new extract_valid_lines signature. All 7 callers were updated to the new tuple shape (verified via grep).

2. Removed-but-callers-already-updated: the FilterResult and filter_findings removal findings claim orphaned-import risk; grep -rn confirms zero remaining importers, so the symbols can be removed without breakage. Same pattern as the "registration/façade imports mistaken for dead code" pattern from PR Rectify: Recipe Delivery Budget Reconciliation (#4557) #4564 evidence.

3. Architectural-guard relaxation with documented rationale: the subset-allowlist assertion in test_contract_module_imports_only_stdlib_and_anchor_sibling carries an explicit comment (L172-176) documenting the trade-off; the audit treated the relaxation as a silent regression. This is the same "contract ledgers mistaken for brittle duplication" pattern observed across PR Rectify: Recipe Delivery Budget Reconciliation (#4557) #4564 and PR feat: attest projected Codex skill discovery #5050.

4. Type-narrowing asserts flagged as production invariants: two findings on poster.py:377, 423 flagged assert response is not None and slot is not None as python -O-stripping risks. These asserts are inside an exhaustive match whose default arm is assert_never, so they are pure type narrowing — the underlying guarantee comes from the match arms, not the asserts.

5. Hardcoded "RIGHT" side flagged as side-channel bug: two findings on _validation.py:248, 296 flagged the hardcoded "RIGHT" argument to anchor_authority.classify. LEFT-side admission is reached through a separate deletion-finding path (deletion_only=True flows through validated_deletion); the standard-finding path is intentionally RIGHT-side only.

6. Recipe head_sha source change flagged as identity-drift risk: eight findings on implementation.json, merge-prs.json, research.json, research-review.json, remediation.json flagged the change from context.review_head_sha / context.resolve_review_head_sha to context.pr_head_sha. The change is a deliberate contract migration documented in code comments and tested by tests/recipe/test_review_publication_receipt_contracts.py:226.

7. Unpostable confusion vs. invalid concern: all 161 round-3 candidates were unpostable. The audit surfaced real concerns in some cases (e.g., the redundant-field() cleanup) and fabricated concerns in others (e.g., the StrEnum-vs-str confusion). Unpostable status alone is not a signal of validity; the auditor must separately verify the cited code. Future reviewers should treat "unpostable" as orthogonal to "valid concern" rather than as evidence of invalidity.

Recommended implications for this refactor

• Add a stdlib-type-semantics-vs-audit-claim contrastive canary to the dedicated auditor definitions. Pair the false claim with the correct statement: "AnchorAdmission (StrEnum) is a str — isinstance(x, AnchorAdmission) implies isinstance(x, str)." Counter-pair: "function declared with * after positional args forces keyword-only — positional binding is unreachable." Counter-pair: "if all callers were updated to the new return shape, the unpacking failure mode does not materialize."
• Teach dedicated agents to disambiguate "unpostable" from "invalid": the strict anchor-admission gate will reject any candidate whose (path, line) is not in the diff. Treat this as a separate signal from "the cited code matches the description" — the auditor must verify the latter independently.
• Treat StrEnum-vs-str and MappingProxyType-vs-dict as a recurring audit-failure mode worth a dedicated prompt subsection. The PR-5076 corpus shows at least five findings produced by this single misunderstanding.
• Promote the PR-5076 148-reject corpus into agent-eval canaries alongside the PR Rectify: Recipe Delivery Budget Reconciliation (#4557) #4564, Rectify: Parent-Owned Audit Admission Across Projected-Plugin Children #4590, and feat: attest projected Codex skill discovery #5050 corpora. The stdlib-semantic and removed-but-callers-updated canaries are net-new; the architectural-guard-relaxation, type-narrowing-assert, hardcoded-side, and head_sha-migration canaries reinforce prior evidence.

Review-quality evidence: PR #5248 (2026-10-05)

• Complete PR #5248 corpus: all 74 rejections, pattern analysis, source/provenance, and contrastive positive controls
• All 15 original accepted findings with exact fix and repair commit links; 0 critical, 9 warning, 6 info
• Full commit manifest: 20 pushed commits, including the three agreed discussion follow-ups
• Tested and pushed PR #5248 changes