Skip to content

refactor(reviewing-prs): separate correctness and protocol compliance - #15

Closed
SSFSKIM wants to merge 11 commits into
refactor/reviewing-prs-skill-entrypointfrom
refactor/reviewing-prs-split-review-responsibilities
Closed

refactor(reviewing-prs): separate correctness and protocol compliance#15
SSFSKIM wants to merge 11 commits into
refactor/reviewing-prs-skill-entrypointfrom
refactor/reviewing-prs-split-review-responsibilities

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 Fable 5 (claude-fable-5[1m])
Harness + version Claude Code 2.1.203
All plugins installed See complete inventory below
Human partner who reviewed this diff SSFSKIM identified the failure mode, approved the responsibility split and blocker rules, and requested this stacked PR. Complete line-by-line diff review is pending; this is intentionally a draft.
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 native Codex reviewer was being asked to perform two different jobs in one invocation: its built-in correctness review and ticket-specific spec/decision-discipline review injected through developer_instructions. In real use, the spec addendum did not behave as intended and could distract or weaken the otherwise robust native correctness review. Meanwhile the outer Review Worker waited for Codex instead of using its own ticket and process context to audit whether the Implement Worker actually passed the ready-for-agent gate and stopped for human-grade product decisions.

The intended split is actor-specific: native Codex should review code correctness without custom criteria, while the outer Review Worker should audit the Implement Worker's protocol compliance against the canonical ticket artifacts.

This PR is stacked on #14, which first makes SKILL.md the canonical Review Worker Protocol. The changes here are only the follow-up behavior and trust-boundary work enabled by that entrypoint restructure.

What does this PR change?

review-engine.sh now runs pure codex exec review --base with no criteria file or custom developer instructions. The outer Review Worker starts that engine in the background, independently records a ticket/spec/decision-discipline audit, then joins and routes both streams.

The audit uses the authorization-time issue body as primary specification, base-pinned issue-referenced documents and pre-resume human answers as authoritative supplements, and distinct PROTOCOL BLOCKER, SPEC FINDING, AUDIT NOTE, and EVIDENCE FINDING routes. The dispatcher supplies trusted absolute paths and current hashes for both sides of the audit contract; Implement Worker gate comments record the original issue-body and protocol hashes, and GitHub edit history resolves later ticket-body drift.

Is this change appropriate for the core library?

Yes. This is a general-purpose responsibility and trust-boundary correction for the core autonomous PR-review loop. It applies to every adopting repository and both Claude and Codex worker species, adds no dependency, and contains no project-specific policy.

What alternatives did you consider?

  1. Keep spec compliance in Codex and tune the wording. Rejected because the observed problem is responsibility coupling, not prompt length; even the minimal addendum can perturb native review.
  2. Run the worker audit after Codex completes. Rejected because it keeps the outer worker idle and lets Codex's findings anchor which product decisions the worker notices.
  3. Dispatch a second spec-review subagent. Rejected as unnecessary and unsafe here; reviewer delegation produced abnormal recursive fan-out of more than 43 subagents in this session.
  4. Embed the complete protocol in every spawn prompt. Rejected because it recreates duplicate doctrine and the entrypoint inversion fixed by refactor(reviewing-prs): make skill the worker protocol #14.
  5. Trust or compose workspace .agents/skills. Rejected because that path is PR-controlled in the detached review head. The dispatcher-owned canonical skill file is the only trusted protocol source.

Does this PR contain multiple unrelated changes?

No. The engine simplification, worker-owned audit, source hierarchy, evidence routing, canonical-skill trust boundary, tests, and living documentation are all required by the same responsibility split. PR #14 is the structural prerequisite and is therefore this PR's base rather than bundled into its diff.

Existing PRs

#14 is the direct stack base and only changes the skill entrypoint/file ownership. #9 introduced the native review engine and the criteria/developer-instruction carrier that this PR partially supersedes while preserving its nested environment recipe. #12 established native severity as the blocker bit for native findings; this PR preserves that rule and adds separate classes only for worker-produced protocol/spec/evidence findings.

Environment tested

Harness Harness version Model Model version/ID
Claude Code 2.1.203 Claude Fable 5 claude-fable-5[1m]
Direct native review codex-cli 0.144.1 GPT-5.6 Sol gpt-5.6-sol, high effort
Hermetic Codex worker/engine tests repository shell harness Stubbed Codex N/A

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

Not applicable. No new harness is added.

Evaluation

  • Initial human prompt: spec compliance in codex exec review developer instructions was not working as intended and could disrupt correctness review; move that responsibility into the outer Review Worker and run both tracks concurrently.
  • Live end-to-end worker eval sessions after the change: 0. This remains a draft until a consumer shakedown and complete human diff review.
  • RED baseline for the responsibility split: the engine exited 2 on the new no-criteria call; the skill contract reported 11 failures; rendered dispatch reported 6 failures.
  • Follow-up RED/GREEN cycles caught dispatch authorization timing, closing-artifact classification, native verdict derivation, canonical skill availability, resumed-ticket human answers, base-pinned referenced specs, ticketless evidence routing, and workspace same-name skill spoofing.
  • Direct Codex review rounds 1–7 found and verified eleven issues: eight P1s and three P2s. Each was fixed with a focused failing test before the implementation change. Round 8 returned no actionable regressions.
  • Deterministic verification passes: review-engine, skill-entrypoint, review-dispatch, land-dispatch, Implement Worker protocol invariants, Codex daemon substrate, Codex plugin sync regression, explicit shell lint, and stacked git diff --check.
  • tests/claude-code/run-skill-tests.sh has one unrelated model-output regex instability in test-subagent-driven-development.sh: two runs produced semantically compliant answers but missed different literal patterns. The affected skill and test have no diff in this branch.

Rigor

  • If this is a skills change: I used doperpowers:writing-skills and completed adversarial pressure testing
  • This change was tested adversarially, not just on the happy path
  • I did not modify Red Flags tables, rationalization lists, or "human partner" language

doperpowers:writing-skills and test-first RED/GREEN discipline were used. The first box remains unchecked because the branch has not yet run multiple live Review Worker behavior sessions; direct Codex branch review and hermetic protocol tests are not being mislabeled as that evaluation.

The direct reviewer path was the bounded CLI codex exec review --base ..., not the recursive native code-review/reviewer delegation path that previously spawned 43+ subagents.

Human review

  • A human has reviewed the COMPLETE proposed diff before submission

The human partner approved the design, blocker semantics, issue-source hierarchy, and stacked delivery. This PR is opened as a draft to provide the complete follow-up diff for line-by-line review; do not mark ready or merge until this box can be checked truthfully.

@SSFSKIM

SSFSKIM commented Jul 15, 2026

Copy link
Copy Markdown
Owner Author

Closing without merge, per adversarial review of the stack (three independent reviews). The verdict: the policies in this PR survived review intact — split responsibilities, anti-anchoring audit ordering, read-only concurrency, no-retroactive-spec — but the form did not: the provenance hash machinery is fragile at its weakest link (gate-side hashes are computed by an LLM following prose against a script-computed review side) and noisy in steady state (implement-protocol-sha256 mismatches after every plugin update), SPEC FINDING and EVIDENCE FINDING route identically, and the protocol doubled in size with prose the tests then froze.

This will be rebuilt from the policy record rather than patched: the execplan on this branch (docs/doperpowers/execplans/2026-07-14-split-review-responsibilities.md) is the source. The branch is kept for that record. Key changes for the rebuild: timestamp-anchored drift detection instead of hashes, one merged worker-finding class, evidence gate keyed on gate-comment presence, in-loop confident-ready expiry, and fixes for the letter-level dead-ends (ticketless TOO BIG, TECH_DEBT_ISSUE=none, engine-outage respawn cap, secondary linked issues).

The trust-boundary fix and dead-template-tail fix from this stack were pulled down into PR #14 directly (commit bf4552d).

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