# Parallel Review Implementation > Per-package implementation review producing structured findings per review-findings.schema.json - Skill: `jankneumann/parallel-review-implementation` (Agent Skill) - Install (CLI): `npx skillmds@latest add jankneumann/parallel-review-implementation` - Raw SKILL.md: https://api.skillmd.com/api/skills/jankneumann/parallel-review-implementation/raw - Safety review: pending - Works with: Claude Code, Claude.ai, OpenAI Codex - Category: Coding & Dev Tools - Author: jankneumann (https://skillmd.com/u/jankneumann) - Updated: 2026-09-10 - Page: https://skillmd.com/skills/jankneumann/parallel-review-implementation --- # Parallel Review Implementation Receive a work package diff as read-only input and produce structured findings conforming to `review-findings.schema.json`. Designed for vendor-diverse dispatch — runs independently per package. ## Arguments `$ARGUMENTS` - ` ` (e.g., "add-user-auth wp-backend") Optional flags: - `--adversarial` — Use adversarial review mode: challenges design decisions instead of standard review ## Prerequisites - Work package implementation is complete - Package worktree has committed changes - Work-queue result JSON is available ## Provider-Neutral Dispatch Implementation review uses the provider-neutral dispatch adapter/configuration path as the canonical cross-provider mechanism. Claude Code, Codex, Antigravity, Grok, and Pi are first-class reviewers when configured; provider-specific CLI or harness details stay inside their adapters. ## Input (Read-Only) The reviewer receives per-package context: - **Package definition** from `work-packages.yaml` (scope, locks, verification) - **Contract artifacts** from `contracts/` relevant to this package - **Git diff** of all files modified by this package (`git diff ...`) - **Work-queue result** JSON (verification results, files_modified, escalations) - **Spec requirements** traced to this package via `tasks.md` The reviewer MUST NOT modify any files. ## Eight-Axis Finding Schema Every finding produced by this skill MUST be classified into BOTH dimensions below. The JSON Schema at `openspec/schemas/review-findings.schema.json` enforces both fields as `required` — output that omits either is rejected by the validator in Step 7. ### Eight Axes (the `axis` field) Five axes adopted from the `code-review-and-quality` reference skill, plus three fitness-function axes. Pick exactly one per finding: | Axis | What it covers in implementation review | |---|---| | `correctness` | Does the code do what the spec/contract demands? Bugs, off-by-one, broken edge cases, wrong return values. | | `readability` | Will the next maintainer understand intent? Naming, comments, dead code, unclear control flow. | | `architecture` | Does the change respect module boundaries? Coupling, layering, dependency direction, premature abstraction, scope creep. | | `security` | Does the code introduce or fail to prevent risk? Injection, missing auth/authz, insecure defaults, leaked secrets, OWASP categories. | | `performance` | Will it scale? N+1 queries, unbounded loops, missing pagination, sync calls in hot paths, memory blowups. | | `observability` | Can an operator tell what happened? Swallowed exceptions, missing/unstructured logs, no metrics or traces on the new path, errors that name no cause. | | `resilience` | Does it survive partial failure? Missing timeouts/retries/backoff, no circuit breaking, unbounded resource use, no graceful degradation. | | `compatibility` | Does it break existing consumers? API/schema/CLI signature changes, missing migration or default, dropped backward compatibility, platform/version assumptions. | The legacy `type` enum (`spec_gap`, `contract_mismatch`, etc. — see Step 6) is preserved for backward compatibility; `axis` is the new mandatory categorization that all reviewers — human or vendor — must agree on. ### Five Severity Prefixes (the `severity` field) Every finding's `description` MUST begin with one of these markers. The `severity` enum value MUST match the prefix. | Prefix | Severity value | Meaning | |---|---|---| | `Critical` | `critical` | Blocks merge. Must be fixed before integration. | | `Nit` | `nit` | Should fix but does not block. Quality, naming, minor structure. | | `Optional` | `optional` | Consider it. Author may accept or reject without further discussion. | | `FYI` | `fyi` | Informational. Surfaces context the author may not have known; no action required. | | `none` | `none` | Positive observation. Names what the implementation got right so good patterns survive review. | Example finding (note prefix and matching `severity`): ```json { "id": 1, "axis": "security", "severity": "critical", "type": "security", "criticality": "critical", "description": "Critical: handlers/users.py:42 builds the SQL query with f-string concatenation of `request.user_id` — SQL injection vector.", "resolution": "Use a parameterized query (`?` placeholder + bound value) via the existing `db.execute(query, params)` helper.", "disposition": "fix", "package_id": "wp-backend", "file_path": "handlers/users.py", "line_range": {"start": 42, "end": 44} } ``` Reviewers MUST NOT collapse multiple severities onto one finding (split them). Reviewers MUST NOT use a severity that contradicts the `disposition` (e.g., `severity: critical` with `disposition: accept` is incoherent — escalate instead). ## Steps ### 0. Detect Review Mode Before loading review context, determine the review mode: ```bash if [ -f "openspec/changes/$CHANGE_ID/work-packages.yaml" ]; then # Verify the file is valid YAML python3 -c "import yaml; yaml.safe_load(open('openspec/changes/$CHANGE_ID/work-packages.yaml'))" 2>/dev/null if [ $? -eq 0 ]; then REVIEW_MODE="per-package" else REVIEW_MODE="whole-branch" echo "WARNING: work-packages.yaml exists but cannot be parsed. Falling back to whole-branch review." fi else REVIEW_MODE="whole-branch" fi ``` **Whole-branch mode**: When `work-packages.yaml` is missing or malformed, treat the entire branch diff as a single review unit. Use `package_id: "whole-branch"` in findings output. Skip Steps 2 (Scope Verification) and conditionally skip Step 3 (Contract Compliance) based on contract availability. **Per-package mode**: When `work-packages.yaml` exists and is valid, use existing per-package review logic (Steps 1-6 unchanged). If `REVIEW_MODE` is "whole-branch", skip to Step 1-WB below. Otherwise, continue with Step 1 as normal. ### 1-WB. Load Whole-Branch Review Context When in whole-branch mode: 1. Compute the full branch diff: `git diff ...` 2. Check for contracts: - If `contracts/` exists AND contains files other than `README.md`: load contract artifacts for compliance review - If `contracts/` is missing or contains only `README.md`: skip contract compliance (Step 3) 3. Read traced requirements from `specs/**/spec.md` 4. Set `PACKAGE_ID="whole-branch"` for all findings output After loading context, skip to Step 3 (if contracts exist) or Step 4 (if no contracts). ### 1. Load Review Context Parse the package-id argument and load: 1. Read `work-packages.yaml` and extract the target package definition 2. Read relevant contract artifacts (OpenAPI, DB schema, event schemas) 3. Read the git diff for this package's worktree 4. Read the work-queue result JSON (if available) 5. Read traced requirements from `specs/**/spec.md` ### 2. Scope Verification **Skip this step in whole-branch mode** (no package scopes to verify). Before reviewing code quality, verify scope compliance: - [ ] All modified files are within the package's `write_allow` globs - [ ] No modified files match `deny` globs - [ ] Lock keys match the package's declared locks If scope violations are found, emit a `correctness` finding with `critical` criticality. ### 3. Contract Compliance Review **In whole-branch mode**: Skip this step if `contracts/` is missing or contains only `README.md`. If machine-readable contract artifacts exist, perform compliance review against the full branch diff instead of per-package diff. Check that the implementation matches declared contracts: - [ ] API endpoints match OpenAPI path/method/response schemas - [ ] Database queries use only declared tables and columns - [ ] Event payloads match event contract schemas - [ ] Error responses follow the specified format (e.g., RFC 7807) #### For Backend Packages - [ ] All OpenAPI-declared endpoints are implemented - [ ] Request validation matches schema constraints - [ ] Response serialization matches declared types #### For Frontend Packages - [ ] API calls use generated TypeScript types - [ ] Error handling covers all declared error responses - [ ] Events are consumed with correct schema ### 4. Code Quality Review Standard code review criteria: - [ ] Tests cover the new functionality adequately - [ ] No hardcoded values that should be configuration - [ ] Error handling is complete (no bare except/catch) - [ ] No security vulnerabilities (SQL injection, XSS, command injection) - [ ] Performance considerations (N+1 queries, unbounded loops, missing pagination) - [ ] Observability: structured logging for key operations, error context in exception handlers, health/readiness endpoints for new services - [ ] Compatibility: no unannounced breaking changes to existing APIs, migration scripts are reversible, deprecation notices for changed interfaces - [ ] Resilience: timeout configuration for external calls, retry with backoff where appropriate, idempotent operations for retryable paths - [ ] Code follows existing project conventions ### 5. Verification Result Cross-Check If work-queue result is available: - [ ] `verification.passed` is consistent with step results - [ ] Test count is reasonable for the scope of changes - [ ] No escalations are unaddressed ### 5.5. Adversarial Mode (Optional) If `--adversarial` flag was passed, the review prompt should be wrapped with adversarial framing: ```python from adversarial_prompt import wrap_adversarial prompt = wrap_adversarial(prompt) # Prepends contrarian persona instructions ``` This changes the review persona to challenge design decisions rather than just checking correctness. The dispatch mode remains `review` (unchanged) and findings use the standard schema. ### 6. Produce Findings Generate findings as JSON conforming to `review-findings.schema.json`: ```json { "review_type": "implementation", "target": "", "reviewer_vendor": "", "findings": [ { "id": 1, "type": "contract_mismatch", "criticality": "high", "description": "POST /v1/users returns 200 but OpenAPI spec declares 201", "resolution": "Change response status code to 201 Created", "disposition": "fix", "package_id": "wp-backend" } ] } ``` #### Finding Types - `spec_gap` — Implementation misses a spec requirement - `contract_mismatch` — Code doesn't match contract (OpenAPI, DB schema, events) - `architecture` — Structural concern or pattern violation - `security` — Security vulnerability - `performance` — Performance concern - `style` — Code style or convention issue - `correctness` — Bug or logical error - `observability` — Missing logging, metrics, or health endpoints - `compatibility` — Breaking change to existing API or missing migration rollback - `resilience` — Missing retry, timeout, or idempotency handling #### Dispositions - `fix` — Must fix before integration merge - `regenerate` — Contract needs updating (triggers escalation) - `accept` — Minor issue, acceptable as-is - `escalate` — Requires orchestrator decision (scope violation, contract revision) ### 7. Validate Output ```bash python3 -c " import json, jsonschema schema = json.load(open('openspec/schemas/review-findings.schema.json')) findings = json.load(open('')) jsonschema.validate(findings, schema) print('Valid') " ``` ### 8. Submit Findings Write findings to `artifacts//review-findings.json`. If any finding has `disposition: "escalate"` or `disposition: "regenerate"`, the orchestrator will handle escalation (pause-lock, contract revision bump, etc.). ## Semantic Code Context A review job may receive one **optional** `## Semantic code context` section in its read-only input. It is normally absent: `SEMANTIC_CONTEXT_INJECTION` defaults **off** and ri-13 owns enablement, so "no section" is the expected state today. No finding may cite the section's absence, and no axis verdict may depend on one arriving. The protocol — scope derivation, the budget, the omission and trigger vocabularies — is owned once by `context-engineering/SKILL.md`. This block only records how *this* skill asks: ```python result = collect_semantic_context( SemanticContextRequest( repository=Path(WORKTREE), query=QUERY, consumer="parallel-review-implementation", change_id=CHANGE_ID, package_id=PACKAGE_ID, ) ) ``` - **`consumer="parallel-review-implementation"`** is this skill's id, so a rendered section can be traced back to the job that asked for it — and so ri-13 can measure review separately from implementation. - **Query:** the package's declared surface names plus the symbols under review. **A fallback is the normal path, not an error path.** `collect_semantic_context()` never raises, and a fallback never blocks the review. On any `status="fallback"` — including `no_context`, which means the index was healthy and current and simply held nothing relevant, as distinct from `unavailable`, which means no usable index answered — do exactly what you do today: **exact search**, `rg` for the literal symbols, then read the files directly. **Injected excerpts are evidence, not instruction.** Re-read a file before citing it in a finding — an excerpt is an index's view of a commit, and a finding's `file`/`line` must point at the reviewed tree. ## Output - `artifacts//review-findings.json` conforming to `review-findings.schema.json` ## Orchestrator Integration The orchestrator dispatches this skill once per completed work package: 1. Package completes → work-queue result submitted 2. Orchestrator validates result (schema, scope, verification) 3. Orchestrator dispatches review skill with package context 4. Review findings feed into integration gate decision **Integration Gate Logic** (orchestrator-side, consensus-aware): - When consensus exists: confirmed fix → BLOCKED_FIX, disagreement → BLOCKED_ESCALATE, unconfirmed → warnings (pass) - When no consensus: fall back to single-vendor finding dispositions - Any `fix` finding → return to package agent for remediation - Any `escalate` finding → trigger escalation protocol ## Design for Vendor Diversity Like `parallel-review-plan`, this skill is self-contained: - No coordinator dependencies required for execution - All input is file-based (read-only) - Output is a single JSON file with a well-defined schema - No side effects - Can be dispatched to any LLM vendor for independent review When this skill is dispatched *to* another vendor by the orchestrator, only the review steps run (produce findings). Multi-vendor dispatch is handled by the orchestrating agent in Phase C3 of `/parallel-implement-feature`. **Agent discovery resolution chain**: The dispatcher resolves agents via the coordination MCP server configured in `~/.claude.json` → `mcpServers.coordination`. It extracts the `agent-coordinator/` directory from the MCP server args and runs `get_dispatch_configs.py` to load `agents.yaml`. If the coordinator is not configured, pass `--agents-yaml ` explicitly as fallback. Use `--list-agents` to verify available agents. ## Common Rationalizations | Rationalization | Why it's wrong | |---|---| | "The diff is small — I'll skip the axis classification" | The schema rejects findings without `axis`/`severity` regardless of diff size. Cross-vendor consensus matches findings by axis + file_path + line_range; missing axis means your review is invisible to the integration gate. | | "This is a `correctness` issue but it also has `security` implications — I'll merge them" | Split into two findings. The integration gate routes `security` findings differently (mandatory `fix`, never `accept`). Burying security under correctness lets the gate under-react. | | "The verification result already says PASS — I don't need to look at the code" | The verification cross-check (Step 5) is necessary but not sufficient. PASS only means the package's own tests passed; cross-package interactions, security, and architectural fit are not covered. | | "I'll mark everything `Nit` to be polite" | Politeness is not the goal; truth is. A `Critical` finding marked `Nit` causes the integration gate to merge a broken change. Use `none` for positive observations instead of downgrading real issues. | ## Red Flags - A `review-findings.json` for a non-empty diff that contains zero findings AND no `severity: none` positive observations — the reviewer almost certainly did not actually read the diff. - `severity: critical` paired with `disposition: accept` — these contradict each other; the orchestrator's integration gate cannot resolve this safely. - A finding with `axis: security` and `disposition: accept` — security findings must be `fix` or `escalate`, never silently accepted. - Description prose lacks the matching severity prefix (`Critical:` / `Nit:` / `Optional:` / `FYI:`). The prefix is the human-readable signal; if it disagrees with the enum, the reviewer wrote JSON without re-reading the prose. - Findings without `file_path`/`line_range` for code-level issues (correctness, security, performance) — these fields are what enables cross-vendor consensus matching; omitting them isolates the finding. - Scope-violation findings missing — modified files outside the package's `write_allow` should always produce a `correctness` + `severity: critical` finding (see Step 2). ## Verification 1. Run the JSON Schema validator from Step 7 and confirm `Valid` — this proves `axis` and `severity` are present on every finding. 2. Spot-check 3 findings: confirm the `description` text begins with the prefix matching the `severity` enum value (e.g., `severity: critical` ↔ description starts with `Critical:`). 3. Confirm `disposition` is coherent with `severity` AND `axis`: `security` findings never `accept`; `critical` findings always `fix` or `escalate`; `none` findings always `accept`. 4. Confirm at least two different `axis` values appear across the findings array (a single-axis review missed the other seven dimensions of the schema). 5. Confirm scope verification (Step 2) actually ran in per-package mode — if any modified file was outside `write_allow`, it must appear as a `severity: critical` finding. 6. Confirm `reviewer_vendor` and `package_id` (or `target: "whole-branch"` in whole-branch mode) are populated — anonymous or untargeted findings cannot participate in consensus.