diff --git a/CHANGELOG.md b/CHANGELOG.md index 503a572..ea68b7c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,7 +6,9 @@ summary: Chronological history of repository and skill changes. ## 2026-07-19 — Epic workflow and review contract cleanup +- feat: compose repository-owned code review - feat: add local code simplicity review + (d6ed890f6924a2ae7ae4b04fa95072ee853c9b97) - feat: add whole-solution simplicity review (8459402e95888047587cf423454f9f8ac42f6881) - feat: add goal-first correctness review diff --git a/README.md b/README.md index 7e67d1a..884f2d2 100644 --- a/README.md +++ b/README.md @@ -11,6 +11,8 @@ A personal monorepo for agent skills and supporting scripts. Current skills: +- `skills/review-code-change` — orchestrate the repository-owned review lenses + into one evidence-bound, deduplicated verdict - `skills/implement-epic-sequence` — execute GitHub or Linear epics through dependency-aware implementation, review, merge, cleanup, and closeout - `skills/prepare-changesets` — decompose a large, review-ready branch into a diff --git a/review-suite/CONTRACT.md b/review-suite/CONTRACT.md index 6a74613..f613af6 100644 --- a/review-suite/CONTRACT.md +++ b/review-suite/CONTRACT.md @@ -88,6 +88,24 @@ fields that the caller could not establish and may preserve already-demonstrated findings, but those findings do not convert the blocked review into a merge verdict. +## Simplification proposal dispositions + +When an orchestrator asks correctness to assess a validated simplification +result, supply that result beside the unchanged review packet. Do not add review +conclusions to the packet itself. Correctness returns one +`proposal_dispositions` item for every supplied gating proposal: + +- `compatible` means the proposal preserves demonstrated correctness and remains + actionable; and +- `unsafe` means concrete correctness or repository evidence invalidates the + proposal even though the current candidate may already be correct. + +Each disposition identifies the source finding and lens and cites concrete +evidence. It does not describe a candidate defect and therefore does not change +the correctness verdict by itself. If correctness cannot assess a supplied +proposal trustworthily, return `blocked`. Only `correctness` and `aggregate` +results may contain proposal dispositions. + ## Candidate identity and base drift Bind every result to the packet's captured head and comparison base. Any edit, diff --git a/review-suite/contracts/review-result.schema.json b/review-suite/contracts/review-result.schema.json index 410d729..7dab07c 100644 --- a/review-suite/contracts/review-result.schema.json +++ b/review-suite/contracts/review-result.schema.json @@ -87,6 +87,44 @@ "type": "array", "items": {"type": "string", "minLength": 1} }, + "proposal_dispositions": { + "type": "array", + "items": { + "type": "object", + "additionalProperties": false, + "required": [ + "finding_id", + "source_lens", + "disposition", + "reason", + "evidence" + ], + "properties": { + "finding_id": { + "type": "string", + "pattern": "^[a-z0-9][a-z0-9._-]*$" + }, + "source_lens": { + "enum": ["solution_simplicity", "code_simplicity"] + }, + "disposition": {"enum": ["compatible", "unsafe"]}, + "reason": {"type": "string", "minLength": 1}, + "evidence": { + "type": "array", + "minItems": 1, + "items": { + "type": "object", + "additionalProperties": false, + "required": ["location", "detail"], + "properties": { + "location": {"type": "string", "minLength": 1}, + "detail": {"type": "string", "minLength": 1} + } + } + } + } + } + }, "next_action": {"type": "string", "minLength": 1} } } diff --git a/review-suite/scripts/tests/test_contracts.py b/review-suite/scripts/tests/test_contracts.py index 2b5c91e..958381a 100644 --- a/review-suite/scripts/tests/test_contracts.py +++ b/review-suite/scripts/tests/test_contracts.py @@ -159,6 +159,44 @@ def test_unknown_finding_enum_is_rejected(self): result["findings"][0]["confidence"] = "certain" self.assertTrue(VALIDATOR.validate_result(result)) + def test_clean_correctness_result_can_reject_an_unsafe_proposal(self): + result = copy.deepcopy(self.clean) + result["lens"] = "correctness" + result["proposal_dispositions"] = [ + { + "finding_id": "solution-simplicity.remove-claim-fence", + "source_lens": "solution_simplicity", + "disposition": "unsafe", + "reason": "The predicates implement the required claim fence.", + "evidence": [ + { + "location": "worker.py:complete", + "detail": "The token predicate prevents stale completion.", + } + ], + } + ] + self.assertEqual([], VALIDATOR.validate_result(result)) + + def test_simplification_lens_cannot_disposition_its_own_proposal(self): + result = copy.deepcopy(self.clean) + result["lens"] = "solution_simplicity" + result["proposal_dispositions"] = [ + { + "finding_id": "solution-simplicity.remove-claim-fence", + "source_lens": "solution_simplicity", + "disposition": "unsafe", + "reason": "The predicates implement the required claim fence.", + "evidence": [ + { + "location": "worker.py:complete", + "detail": "The token predicate prevents stale completion.", + } + ], + } + ] + self.assertTrue(VALIDATOR.validate_result(result)) + def test_result_must_match_packet_candidate(self): packet = load(ROOT / "fixtures" / "clean-change" / "packet.json") result = copy.deepcopy(self.clean) diff --git a/review-suite/scripts/validate.py b/review-suite/scripts/validate.py index ac6171b..76ec236 100644 --- a/review-suite/scripts/validate.py +++ b/review-suite/scripts/validate.py @@ -181,6 +181,22 @@ def validate_result(result: dict[str, Any]) -> list[str]: ] if foreign: errors.append("$.findings: lens mismatch for " + ", ".join(foreign)) + + dispositions = result.get("proposal_dispositions", []) + if dispositions and result.get("lens") not in {"correctness", "aggregate"}: + errors.append( + "$.proposal_dispositions: only correctness or aggregate results may " + "disposition simplification proposals" + ) + disposition_ids = [item.get("finding_id") for item in dispositions] + duplicate_dispositions = sorted( + {item for item in disposition_ids if disposition_ids.count(item) > 1} + ) + if duplicate_dispositions: + errors.append( + "$.proposal_dispositions: duplicate finding id(s): " + + ", ".join(duplicate_dispositions) + ) return errors diff --git a/skills/review-code-change/SKILL.md b/skills/review-code-change/SKILL.md new file mode 100644 index 0000000..5107b6c --- /dev/null +++ b/skills/review-code-change/SKILL.md @@ -0,0 +1,101 @@ +--- +name: review-code-change +description: Run the complete repository-owned review of a PR, branch, patch, or change set. Use when one entrypoint should build a trustworthy evidence packet, invoke solution simplicity, correctness, and code simplicity in order, reconcile their results, and return one bounded aggregate verdict. Fail closed when evidence or local lens skills are missing, remain read-only, and never depend on a third-party review skill. +--- + +# Review Code Change + +Produce one trustworthy, bounded verdict for one captured candidate. Orchestrate +the repository-owned lenses; do not reproduce their rubrics. + +## Load the contracts and dependencies + +1. Read `../../review-suite/CONTRACT.md` and both shared schemas. +2. Read [the orchestration protocol](references/orchestration-protocol.md). +3. Verify that `review-solution-simplicity`, `review-correctness`, and + `review-code-simplicity` are available and readable. +4. Return a conforming aggregate `blocked` result naming every missing skill. Do + not fall back to another skill or generic self-review. + +## Build one evidence packet + +Build the shared packet once from raw ticket, PR, repository, candidate, and +validation artifacts. Capture: + +- observable goal, acceptance criteria, non-goals, and preserved behavior; +- repository instructions, named specifications, and representative nearby code + and tests; +- exact head and relevant base or merge-base identity plus the complete diff; +- focused and full validation commands with exact results; and +- worktree state required to prove read-only integrity. + +Exclude implementation transcripts, intended answers, prior conclusions, +suspected findings, and fixture expected outputs. Validate the packet before +invoking any lens. Return `blocked` when required evidence cannot be recovered +safely. + +## Run the deliberate sequence + +For a full review, invoke the skills sequentially from the same validated +packet: + +1. `review-solution-simplicity` +2. `review-correctness` +3. `review-code-simplicity` + +Validate each result before continuing. Stop on a `blocked` result. Stop after a +solution-simplicity result that requires replacing the implementation strategy; +the caller must redesign and start a full review on a new head. For a tractable +in-strategy proposal, invoke correctness with the unchanged packet plus the +validated solution result as separate proposal context. Require one proposal +disposition for each gating simplification finding so correctness can reject an +unsafe proposal without inventing a defect in already-correct candidate code. + +Stop after an actionable correctness result; do not spend tokens on local +simplification before the correctness fix. A clean pass through all applicable +lenses ends the review. + +## Handle fixes and cycles + +The orchestrator is read-only. Return the required fix and next lens; the caller +applies changes and supplies a new packet bound to the new head. + +- After solution redesign, restart the full sequence. +- After a correctness fix, rerun correctness and every downstream lens whose + assumptions changed. +- After a code-simplicity fix, rerun code simplicity and then targeted + correctness. +- Use at most three full fix/re-review cycles by default. On the final cycle, + return unresolved material findings without requesting another automatic + cycle. +- Ignore style, praise, speculative hardening, and deferred scope when counting + cycles. + +## Aggregate one result + +Follow the protocol to validate, deduplicate, and reconcile results. Correctness +and explicit repository constraints override unsafe simplification. Preserve +deferred findings as non-gating. Preserve proposal dispositions in the aggregate +so every accepted or rejected simplification remains auditable. + +Return only JSON conforming to the shared result schema with lens `aggregate`. +Include candidate identity, material findings, blocking reasons, validation +limitations, and the next required action. + +- `changes_required`: a blocking or strong-recommendation finding remains. +- `blocked`: evidence, a required dependency, or a lens verdict is + untrustworthy. +- `clean`: every required lens completed and no gating finding remains. + +Never count an unavailable or skipped required lens as clean. + +## Preserve candidate integrity + +Bind every packet and result to the captured head. Build a new packet after any +code change, rebase, conflict resolution, or update. For base-only drift, apply +the shared risk-based merge-candidate rules. + +Do not edit or format reviewed files, apply fixes, create candidate artifacts, +commit, push, post reviews, resolve threads, approve, merge, or update tickets. +Run only safe read-only inspection and validation commands. Verify that the +candidate state is unchanged before returning. diff --git a/skills/review-code-change/agents/openai.yaml b/skills/review-code-change/agents/openai.yaml new file mode 100644 index 0000000..f613cba --- /dev/null +++ b/skills/review-code-change/agents/openai.yaml @@ -0,0 +1,4 @@ +interface: + display_name: "Review Code Change" + short_description: "Run the repository-owned code review suite" + default_prompt: "Use $review-code-change to build one evidence packet and run the complete repository-owned review sequence for this change." diff --git a/skills/review-code-change/evals/cases.json b/skills/review-code-change/evals/cases.json new file mode 100644 index 0000000..00786b3 --- /dev/null +++ b/skills/review-code-change/evals/cases.json @@ -0,0 +1,102 @@ +[ + { + "id": "ordered-clean", + "candidate": {"head_sha": "1010101010101010101010101010101010101010", "comparison_base_sha": "a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0"}, + "available_dependencies": ["review-solution-simplicity", "review-correctness", "review-code-simplicity"], + "evidence": {"complete": true}, "cycle": 1, "phase": "full", + "harness_outcomes": { + "solution_simplicity": {"schema_version": "1.0", "lens": "solution_simplicity", "candidate": {"head_sha": "1010101010101010101010101010101010101010", "comparison_base_sha": "a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0"}, "verdict": "clean", "findings": [], "blocking_reasons": []}, + "correctness": {"schema_version": "1.0", "lens": "correctness", "candidate": {"head_sha": "1010101010101010101010101010101010101010", "comparison_base_sha": "a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0"}, "verdict": "clean", "findings": [], "blocking_reasons": []}, + "code_simplicity": {"schema_version": "1.0", "lens": "code_simplicity", "candidate": {"head_sha": "1010101010101010101010101010101010101010", "comparison_base_sha": "a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0"}, "verdict": "clean", "findings": [], "blocking_reasons": []} + } + }, + { + "id": "early-solution-redesign", + "candidate": {"head_sha": "2020202020202020202020202020202020202020", "comparison_base_sha": "b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0"}, + "available_dependencies": ["review-solution-simplicity", "review-correctness", "review-code-simplicity"], + "evidence": {"complete": true}, "cycle": 1, "phase": "full", + "harness_outcomes": { + "solution_simplicity": {"schema_version": "1.0", "lens": "solution_simplicity", "candidate": {"head_sha": "2020202020202020202020202020202020202020", "comparison_base_sha": "b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0"}, "verdict": "changes_required", "findings": [{"id": "solution-simplicity.remove-provider-platform", "lens": "solution_simplicity", "severity": "blocking", "confidence": "high", "rule": "Remote providers are out of scope.", "evidence": [{"location": "candidate diff", "detail": "A registry, provider API, configuration, and dispatch exist for one local write."}], "concern": "The candidate implements an out-of-scope provider platform.", "impact": "It adds configuration, dispatch, and provider failure modes.", "proposed_change": "Replace the provider platform with one direct local write.", "expected_effect": "Remove the platform, configuration, and provider failure modes."}], "blocking_reasons": [], "next_action": "Redesign the implementation and restart the full review on a new head."}, + "correctness": {"schema_version": "1.0", "lens": "correctness", "candidate": {"head_sha": "2020202020202020202020202020202020202020", "comparison_base_sha": "b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0"}, "verdict": "clean", "findings": [], "blocking_reasons": []}, + "code_simplicity": {"schema_version": "1.0", "lens": "code_simplicity", "candidate": {"head_sha": "2020202020202020202020202020202020202020", "comparison_base_sha": "b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0"}, "verdict": "clean", "findings": [], "blocking_reasons": []} + } + }, + { + "id": "correctness-overrides-simplification", + "candidate": {"head_sha": "3030303030303030303030303030303030303030", "comparison_base_sha": "c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0"}, + "available_dependencies": ["review-solution-simplicity", "review-correctness", "review-code-simplicity"], + "evidence": {"complete": true}, "cycle": 1, "phase": "full", + "harness_outcomes": { + "solution_simplicity": {"schema_version": "1.0", "lens": "solution_simplicity", "candidate": {"head_sha": "3030303030303030303030303030303030303030", "comparison_base_sha": "c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0"}, "verdict": "changes_required", "findings": [{"id": "solution-simplicity.remove-conditional-write", "lens": "solution_simplicity", "severity": "strong_recommendation", "confidence": "medium", "rule": "Prefer a direct status update when extra predicates are unnecessary.", "evidence": [{"location": "candidate update", "detail": "The conditional update has more predicates than a direct update."}], "concern": "The predicates may be unnecessary local machinery.", "impact": "They add conditions and a failure path.", "proposed_change": "Remove claim-token and running-state predicates.", "expected_effect": "Fewer predicates and one less failure path."}], "blocking_reasons": [], "next_action": "Continue to correctness before accepting this in-strategy simplification."}, + "correctness": {"schema_version": "1.0", "lens": "correctness", "candidate": {"head_sha": "3030303030303030303030303030303030303030", "comparison_base_sha": "c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0"}, "verdict": "clean", "findings": [], "blocking_reasons": [], "proposal_dispositions": [{"finding_id": "solution-simplicity.remove-conditional-write", "source_lens": "solution_simplicity", "disposition": "unsafe", "reason": "The proposed removal would discard the required atomic claim fence; the current candidate already preserves it.", "evidence": [{"location": "candidate conditional update", "detail": "The claim-token and running-state predicates prevent an expired worker from completing another worker's claim."}]}], "next_action": "Reject the unsafe simplification and continue reviewing the unchanged candidate."}, + "code_simplicity": {"schema_version": "1.0", "lens": "code_simplicity", "candidate": {"head_sha": "3030303030303030303030303030303030303030", "comparison_base_sha": "c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0"}, "verdict": "clean", "findings": [], "blocking_reasons": []} + } + }, + { + "id": "deduplicate-overlap", + "candidate": {"head_sha": "4040404040404040404040404040404040404040", "comparison_base_sha": "d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0"}, + "available_dependencies": ["review-solution-simplicity", "review-correctness", "review-code-simplicity"], + "evidence": {"complete": true}, "cycle": 1, "phase": "full", + "harness_outcomes": { + "solution_simplicity": {"schema_version": "1.0", "lens": "solution_simplicity", "candidate": {"head_sha": "4040404040404040404040404040404040404040", "comparison_base_sha": "d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0"}, "verdict": "changes_required", "findings": [{"id": "solution-simplicity.remove-local-policy-service", "lens": "solution_simplicity", "severity": "strong_recommendation", "confidence": "high", "rule": "Use the existing authorization boundary.", "evidence": [{"location": "policy service", "detail": "A new service duplicates the active-admin rule."}], "concern": "The service duplicates repository policy.", "impact": "It adds a service and another policy owner.", "proposed_change": "Call require_active_admin directly.", "expected_effect": "Remove one service and one duplicate policy."}], "blocking_reasons": [], "next_action": "Continue through correctness because this is a bounded in-strategy removal."}, + "correctness": {"schema_version": "1.0", "lens": "correctness", "candidate": {"head_sha": "4040404040404040404040404040404040404040", "comparison_base_sha": "d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0"}, "verdict": "clean", "findings": [], "blocking_reasons": [], "proposal_dispositions": [{"finding_id": "solution-simplicity.remove-local-policy-service", "source_lens": "solution_simplicity", "disposition": "compatible", "reason": "Reusing the existing active-admin helper preserves the repository's authorization boundary.", "evidence": [{"location": "repository policy helper", "detail": "require_active_admin implements the same required authorization check."}]}]}, + "code_simplicity": {"schema_version": "1.0", "lens": "code_simplicity", "candidate": {"head_sha": "4040404040404040404040404040404040404040", "comparison_base_sha": "d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0"}, "verdict": "changes_required", "findings": [{"id": "code-simplicity.reuse-active-admin", "lens": "code_simplicity", "severity": "strong_recommendation", "confidence": "high", "rule": "Reuse repository policy helpers.", "evidence": [{"location": "new actions", "detail": "Two actions repeat the active-admin condition."}], "concern": "The actions duplicate the same repository policy.", "impact": "Three policy definitions can drift.", "proposed_change": "Call require_active_admin directly.", "expected_effect": "Reduce three policy definitions to one."}], "blocking_reasons": []} + } + }, + { + "id": "targeted-correctness-recheck", + "candidate": {"head_sha": "5050505050505050505050505050505050505050", "comparison_base_sha": "e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0"}, + "available_dependencies": ["review-solution-simplicity", "review-correctness", "review-code-simplicity"], + "evidence": {"complete": true, "new_packet_after_code_simplification": true}, "cycle": 2, "phase": "post_code_simplification", + "harness_outcomes": { + "code_simplicity": {"schema_version": "1.0", "lens": "code_simplicity", "candidate": {"head_sha": "5050505050505050505050505050505050505050", "comparison_base_sha": "e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0"}, "verdict": "clean", "findings": [], "blocking_reasons": []}, + "correctness": {"schema_version": "1.0", "lens": "correctness", "candidate": {"head_sha": "5050505050505050505050505050505050505050", "comparison_base_sha": "e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0"}, "verdict": "clean", "findings": [], "blocking_reasons": []} + } + }, + { + "id": "missing-dependency", + "candidate": {"head_sha": "6060606060606060606060606060606060606060", "comparison_base_sha": "f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0"}, + "available_dependencies": ["review-solution-simplicity", "review-correctness"], + "evidence": {"complete": true}, "cycle": 1, "phase": "full", "harness_outcomes": {} + }, + { + "id": "missing-evidence", + "candidate": {"head_sha": "7070707070707070707070707070707070707070", "comparison_base_sha": "a1a1a1a1a1a1a1a1a1a1a1a1a1a1a1a1a1a1a1a1"}, + "available_dependencies": ["review-solution-simplicity", "review-correctness", "review-code-simplicity"], + "evidence": {"complete": false, "blocking_reasons": ["Acceptance criteria are missing.", "Full validation has no result or unavailable reason."]}, + "cycle": 1, "phase": "full", "harness_outcomes": {} + }, + { + "id": "deferred-only-clean", + "candidate": {"head_sha": "8080808080808080808080808080808080808080", "comparison_base_sha": "b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1"}, + "available_dependencies": ["review-solution-simplicity", "review-correctness", "review-code-simplicity"], + "evidence": {"complete": true}, "cycle": 1, "phase": "full", + "harness_outcomes": { + "solution_simplicity": {"schema_version": "1.0", "lens": "solution_simplicity", "candidate": {"head_sha": "8080808080808080808080808080808080808080", "comparison_base_sha": "b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1"}, "verdict": "clean", "findings": [{"id": "solution-simplicity.future-cleanup", "lens": "solution_simplicity", "severity": "defer", "confidence": "high", "rule": "A real legacy path remains outside this ticket.", "evidence": [{"location": "ticket", "detail": "Migration cleanup is explicitly deferred to issue #90."}], "concern": "A compatibility path remains outside active scope.", "impact": "The branch remains until callers migrate.", "proposed_change": "Remove the path in issue #90.", "expected_effect": "Retire one compatibility branch after migration."}], "blocking_reasons": []}, + "correctness": {"schema_version": "1.0", "lens": "correctness", "candidate": {"head_sha": "8080808080808080808080808080808080808080", "comparison_base_sha": "b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1"}, "verdict": "clean", "findings": [], "blocking_reasons": []}, + "code_simplicity": {"schema_version": "1.0", "lens": "code_simplicity", "candidate": {"head_sha": "8080808080808080808080808080808080808080", "comparison_base_sha": "b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1"}, "verdict": "clean", "findings": [], "blocking_reasons": []} + } + }, + { + "id": "cycle-budget-exhausted", + "candidate": {"head_sha": "9090909090909090909090909090909090909090", "comparison_base_sha": "c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1"}, + "available_dependencies": ["review-solution-simplicity", "review-correctness", "review-code-simplicity"], + "evidence": {"complete": true}, "cycle": 3, "phase": "full", + "harness_outcomes": { + "solution_simplicity": {"schema_version": "1.0", "lens": "solution_simplicity", "candidate": {"head_sha": "9090909090909090909090909090909090909090", "comparison_base_sha": "c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1"}, "verdict": "clean", "findings": [], "blocking_reasons": []}, + "correctness": {"schema_version": "1.0", "lens": "correctness", "candidate": {"head_sha": "9090909090909090909090909090909090909090", "comparison_base_sha": "c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1"}, "verdict": "changes_required", "findings": [{"id": "correctness.retry-still-unbounded", "lens": "correctness", "severity": "blocking", "confidence": "high", "rule": "Retries must stop at the configured maximum.", "evidence": [{"location": "candidate diff", "detail": "The third candidate still schedules after the maximum."}], "concern": "Retry scheduling remains unbounded.", "impact": "The required retry limit is violated.", "proposed_change": "Stop scheduling at max_retries.", "expected_effect": "Bound repeated work at the required limit."}], "blocking_reasons": []}, + "code_simplicity": {"schema_version": "1.0", "lens": "code_simplicity", "candidate": {"head_sha": "9090909090909090909090909090909090909090", "comparison_base_sha": "c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1"}, "verdict": "clean", "findings": [], "blocking_reasons": []} + } + }, + { + "id": "mismatched-lens-result", + "candidate": {"head_sha": "a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2", "comparison_base_sha": "b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2"}, + "available_dependencies": ["review-solution-simplicity", "review-correctness", "review-code-simplicity"], + "evidence": {"complete": true}, "cycle": 1, "phase": "full", + "harness_outcomes": { + "solution_simplicity": {"schema_version": "1.0", "lens": "correctness", "candidate": {"head_sha": "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef", "comparison_base_sha": "b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2"}, "verdict": "clean", "findings": [], "blocking_reasons": []}, + "correctness": {"schema_version": "1.0", "lens": "correctness", "candidate": {"head_sha": "a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2", "comparison_base_sha": "b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2"}, "verdict": "clean", "findings": [], "blocking_reasons": []}, + "code_simplicity": {"schema_version": "1.0", "lens": "code_simplicity", "candidate": {"head_sha": "a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2", "comparison_base_sha": "b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2"}, "verdict": "clean", "findings": [], "blocking_reasons": []} + } + } +] diff --git a/skills/review-code-change/evals/results.json b/skills/review-code-change/evals/results.json new file mode 100644 index 0000000..8da0af4 --- /dev/null +++ b/skills/review-code-change/evals/results.json @@ -0,0 +1,177 @@ +[ + { + "case_id": "ordered-clean", + "observed_sequence": ["solution_simplicity", "correctness", "code_simplicity"], + "stopped_reason": "all_required_lenses_clean", + "result": { + "schema_version": "1.0", "lens": "aggregate", + "candidate": {"head_sha": "1010101010101010101010101010101010101010", "comparison_base_sha": "a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0"}, + "verdict": "clean", "findings": [], "blocking_reasons": [], + "next_action": "No further review action is required." + } + }, + { + "case_id": "early-solution-redesign", + "observed_sequence": ["solution_simplicity"], + "stopped_reason": "solution_strategy_replacement_required", + "result": { + "schema_version": "1.0", "lens": "aggregate", + "candidate": {"head_sha": "2020202020202020202020202020202020202020", "comparison_base_sha": "b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0"}, + "verdict": "changes_required", + "findings": [{ + "id": "solution-simplicity.remove-provider-platform", "lens": "solution_simplicity", "severity": "blocking", "confidence": "high", + "rule": "Remote providers are out of scope.", + "evidence": [{"location": "candidate diff", "detail": "A registry, provider API, configuration, and dispatch exist for one local write."}], + "concern": "The implementation introduces a provider platform even though the scoped behavior requires only one local write.", + "impact": "The unnecessary strategy adds configuration, dispatch, and provider failure modes outside the change contract.", + "proposed_change": "Replace the provider platform with one direct local write.", + "expected_effect": "Remove the platform, configuration, and provider failure modes." + }], + "blocking_reasons": [], + "next_action": "Redesign the implementation, capture a new head-bound evidence packet, and restart the full review sequence." + } + }, + { + "case_id": "correctness-overrides-simplification", + "observed_sequence": ["solution_simplicity", "correctness", "code_simplicity"], + "stopped_reason": "unsafe_simplification_rejected_and_remaining_lenses_clean", + "result": { + "schema_version": "1.0", "lens": "aggregate", + "candidate": {"head_sha": "3030303030303030303030303030303030303030", "comparison_base_sha": "c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0"}, + "verdict": "clean", "findings": [], + "blocking_reasons": [], + "proposal_dispositions": [{ + "finding_id": "solution-simplicity.remove-conditional-write", + "source_lens": "solution_simplicity", "disposition": "unsafe", + "reason": "The proposed removal would discard the required atomic claim fence; the current candidate already preserves it.", + "evidence": [{"location": "candidate conditional update", "detail": "The claim-token and running-state predicates prevent an expired worker from completing another worker's claim."}] + }], + "next_action": "Keep the existing claim fencing; no candidate change or further review action is required." + } + }, + { + "case_id": "deduplicate-overlap", + "observed_sequence": ["solution_simplicity", "correctness", "code_simplicity"], + "stopped_reason": "code_simplicity_gating_finding", + "result": { + "schema_version": "1.0", "lens": "aggregate", + "candidate": {"head_sha": "4040404040404040404040404040404040404040", "comparison_base_sha": "d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0"}, + "verdict": "changes_required", + "findings": [{ + "id": "solution-simplicity.remove-local-policy-service", "lens": "solution_simplicity", "severity": "strong_recommendation", "confidence": "high", + "rule": "Use the existing authorization boundary.", + "evidence": [ + {"location": "solution_simplicity result", "detail": "A new service duplicates the active-admin rule."}, + {"location": "code_simplicity result", "detail": "Two new actions repeat the same active-admin condition."} + ], + "concern": "The new service and actions duplicate the existing active-admin authorization policy.", + "impact": "The candidate maintains three policy definitions and adds an unnecessary service.", + "proposed_change": "Call require_active_admin directly and remove the duplicate local policy service and conditions.", + "expected_effect": "Reduce three policy definitions to one and remove the duplicate service.", + "related_finding_ids": ["code-simplicity.reuse-active-admin"] + }], + "blocking_reasons": [], + "proposal_dispositions": [{ + "finding_id": "solution-simplicity.remove-local-policy-service", + "source_lens": "solution_simplicity", "disposition": "compatible", + "reason": "Reusing the existing active-admin helper preserves the repository's authorization boundary.", + "evidence": [{"location": "repository policy helper", "detail": "require_active_admin implements the same required authorization check."}] + }], + "next_action": "Apply the deduplicated policy reuse change, build a new packet, rerun code simplicity, and then run targeted correctness." + } + }, + { + "case_id": "targeted-correctness-recheck", + "observed_sequence": ["code_simplicity", "correctness"], + "stopped_reason": "all_required_results_clean", + "result": { + "schema_version": "1.0", "lens": "aggregate", + "candidate": {"head_sha": "5050505050505050505050505050505050505050", "comparison_base_sha": "e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0e0"}, + "verdict": "clean", "findings": [], "blocking_reasons": [], + "next_action": "No further review action is required." + } + }, + { + "case_id": "missing-dependency", + "observed_sequence": [], + "stopped_reason": "missing_required_dependency", + "result": { + "schema_version": "1.0", "lens": "aggregate", + "candidate": {"head_sha": "6060606060606060606060606060606060606060", "comparison_base_sha": "f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0"}, + "verdict": "blocked", "findings": [], + "blocking_reasons": ["Required review skill review-code-simplicity is unavailable."], + "next_action": "Make review-code-simplicity available and readable, then restart the full review." + } + }, + { + "case_id": "missing-evidence", + "observed_sequence": [], + "stopped_reason": "evidence_packet_incomplete", + "result": { + "schema_version": "1.0", "lens": "aggregate", + "candidate": {"head_sha": "7070707070707070707070707070707070707070", "comparison_base_sha": "a1a1a1a1a1a1a1a1a1a1a1a1a1a1a1a1a1a1a1a1"}, + "verdict": "blocked", "findings": [], + "blocking_reasons": ["Acceptance criteria are missing.", "Full validation has no result or unavailable reason."], + "validation_limitations": ["Full validation has no result or unavailable reason."], + "next_action": "Recover the missing acceptance criteria and full-validation result or unavailable reason, rebuild and validate the evidence packet, then restart the full review sequence." + } + }, + { + "case_id": "deferred-only-clean", + "observed_sequence": ["solution_simplicity", "correctness", "code_simplicity"], + "stopped_reason": "all_required_lenses_clean_with_deferred_finding", + "result": { + "schema_version": "1.0", "lens": "aggregate", + "candidate": {"head_sha": "8080808080808080808080808080808080808080", "comparison_base_sha": "b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1"}, + "verdict": "clean", + "findings": [{ + "id": "solution-simplicity.future-cleanup", "lens": "solution_simplicity", "severity": "defer", "confidence": "high", + "rule": "A real legacy path remains outside this ticket.", + "evidence": [{"location": "ticket", "detail": "The ticket explicitly defers migration cleanup to issue #90."}], + "concern": "A legacy compatibility path remains, but its removal is explicitly outside the active ticket.", + "impact": "The compatibility branch remains until its callers migrate.", + "proposed_change": "Remove the legacy path in issue #90.", + "expected_effect": "Retire one compatibility branch after its callers migrate." + }], + "blocking_reasons": [], + "next_action": "No candidate change is required; track the deferred legacy-path cleanup in issue #90." + } + }, + { + "case_id": "cycle-budget-exhausted", + "observed_sequence": ["solution_simplicity", "correctness"], + "stopped_reason": "cycle_budget_exhausted_with_correctness_finding", + "result": { + "schema_version": "1.0", "lens": "aggregate", + "candidate": {"head_sha": "9090909090909090909090909090909090909090", "comparison_base_sha": "c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1c1"}, + "verdict": "changes_required", + "findings": [{ + "id": "correctness.retry-still-unbounded", "lens": "correctness", "severity": "blocking", "confidence": "high", + "rule": "Retries must stop at the configured maximum.", + "evidence": [{"location": "candidate diff", "detail": "The third candidate still increments and schedules after the maximum."}], + "concern": "The retry path continues scheduling work after reaching the configured maximum.", + "impact": "Repeated work remains unbounded and violates the required retry limit.", + "proposed_change": "Stop scheduling when retry_count reaches max_retries.", + "expected_effect": "Bound repeated work at the required limit." + }], + "blocking_reasons": [], + "next_action": "The caller must correct the retry bound before merge; the three-cycle automatic review budget is exhausted, so do not request another automatic cycle." + } + }, + { + "case_id": "mismatched-lens-result", + "observed_sequence": ["solution_simplicity"], + "stopped_reason": "untrustworthy_lens_result", + "result": { + "schema_version": "1.0", "lens": "aggregate", + "candidate": {"head_sha": "a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2", "comparison_base_sha": "b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2"}, + "verdict": "blocked", "findings": [], + "blocking_reasons": [ + "The invoked solution-simplicity lens returned lens correctness instead of solution_simplicity.", + "The invoked solution-simplicity lens returned head deadbeefdeadbeefdeadbeefdeadbeefdeadbeef instead of the packet head a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2." + ], + "validation_limitations": [], + "next_action": "Rerun review-solution-simplicity with the validated packet for the exact captured candidate before continuing." + } + } +] diff --git a/skills/review-code-change/evals/standalone-clean/candidate.diff b/skills/review-code-change/evals/standalone-clean/candidate.diff new file mode 100644 index 0000000..6a510e0 --- /dev/null +++ b/skills/review-code-change/evals/standalone-clean/candidate.diff @@ -0,0 +1,9 @@ +diff --git a/profile.py b/profile.py +--- a/profile.py ++++ b/profile.py +@@ -1,2 +1,4 @@ + def display_name(user): +- return user.name ++ if user.preferred_name: ++ return user.preferred_name ++ return user.name diff --git a/skills/review-code-change/evals/standalone-clean/prompt.md b/skills/review-code-change/evals/standalone-clean/prompt.md new file mode 100644 index 0000000..ab2e650 --- /dev/null +++ b/skills/review-code-change/evals/standalone-clean/prompt.md @@ -0,0 +1,10 @@ +Use the `review-code-change` skill to review the candidate described by the raw +evidence in this directory. Read `ticket.md`, `repository-evidence.md`, +`candidate.diff`, and `validation.md`; do not inspect `result.json`, answer +keys, or prior conclusions. Build one shared packet, then apply the three +repository-owned lens skills in their required order. Do not modify files or +repository state. + +For evaluation recording only, return an object with `observed_sequence` and +`result`; `result` must be the production aggregate JSON conforming to the +shared result schema. diff --git a/skills/review-code-change/evals/standalone-clean/repository-evidence.md b/skills/review-code-change/evals/standalone-clean/repository-evidence.md new file mode 100644 index 0000000..b602c03 --- /dev/null +++ b/skills/review-code-change/evals/standalone-clean/repository-evidence.md @@ -0,0 +1,9 @@ +# Repository evidence + +Repository: `example/profile` Base branch: `main` Candidate head: +`1212121212121212121212121212121212121212` Comparison base: +`abababababababababababababababababababab` + +`AGENTS.md` requires direct code for a two-value domain fallback unless more +states are specified. `contact.py` uses the same explicit preferred-value then +fallback branch. No shared display-name helper or applicable dependency exists. diff --git a/skills/review-code-change/evals/standalone-clean/result.json b/skills/review-code-change/evals/standalone-clean/result.json new file mode 100644 index 0000000..7211721 --- /dev/null +++ b/skills/review-code-change/evals/standalone-clean/result.json @@ -0,0 +1,20 @@ +{ + "observed_sequence": [ + "review-solution-simplicity", + "review-correctness", + "review-code-simplicity" + ], + "result": { + "schema_version": "1.0", + "lens": "aggregate", + "candidate": { + "head_sha": "1212121212121212121212121212121212121212", + "comparison_base_sha": "abababababababababababababababababababab" + }, + "verdict": "clean", + "findings": [], + "blocking_reasons": [], + "validation_limitations": [], + "next_action": "No changes are required." + } +} diff --git a/skills/review-code-change/evals/standalone-clean/ticket.md b/skills/review-code-change/evals/standalone-clean/ticket.md new file mode 100644 index 0000000..f260f3c --- /dev/null +++ b/skills/review-code-change/evals/standalone-clean/ticket.md @@ -0,0 +1,5 @@ +# Prefer a user's chosen display name + +Display `preferred_name` when the user has supplied one. Continue displaying the +existing name as the fallback. This ticket does not change name storage, +validation, or public profile contracts. diff --git a/skills/review-code-change/evals/standalone-clean/validation.md b/skills/review-code-change/evals/standalone-clean/validation.md new file mode 100644 index 0000000..caef9d3 --- /dev/null +++ b/skills/review-code-change/evals/standalone-clean/validation.md @@ -0,0 +1,8 @@ +# Validation evidence + +- Focused: `pytest tests/test_profile.py` passed, 4 tests covering preferred and + fallback names. +- Full: `pytest` passed, 20 tests. + +The candidate diff is complete. The worktree had no staged, unstaged, or +untracked changes before or after validation. diff --git a/skills/review-code-change/references/orchestration-protocol.md b/skills/review-code-change/references/orchestration-protocol.md new file mode 100644 index 0000000..caae537 --- /dev/null +++ b/skills/review-code-change/references/orchestration-protocol.md @@ -0,0 +1,125 @@ +# Orchestration protocol + +This protocol owns sequencing and aggregation only. The shared contract owns +packet and result semantics; each lens skill owns its review rubric. + +## Prepare and validate evidence + +Resolve the live ticket or PR before reading the implementation. Separate +explicit requirements from inferred context and record non-goals and invariants. +Read applicable repository instructions and named specifications, then capture +the complete diff, exact candidate identity, representative nearby code and +tests, validation evidence, and worktree state. + +Run the shared packet validator. Do not invoke a lens with a malformed packet. +Do not repair missing intent by reading the implementation transcript. + +## Sequence decisions + +Use this decision table after validating each lens result. + +| Current result | Next action | +| --------------------------------------------- | -------------------------------------------------------------------- | +| solution `blocked` | aggregate `blocked`; stop | +| solution requires strategy replacement | aggregate findings; caller redesigns; stop | +| solution has a tractable in-strategy proposal | run correctness | +| solution `clean` | run correctness | +| correctness `blocked` | aggregate `blocked`; stop | +| correctness has a gating finding | aggregate findings; caller fixes; stop | +| correctness `clean` | run code simplicity | +| code simplicity `blocked` | aggregate `blocked`; stop | +| code simplicity has a gating finding | aggregate findings; caller fixes, then rechecks code and correctness | +| all required results `clean` | aggregate `clean`; stop | + +A solution result requires strategy replacement when its proposed change +replaces major mechanisms, ownership boundaries, storage, state, or operational +design and its next action calls for redesign or a full restart. Do not infer +early exit merely from finding count. A bounded local removal can proceed to +correctness. + +For a tractable gating proposal, pass the validated simplification result beside +the unchanged packet when invoking correctness. Do not merge it into the packet. +Require correctness to return one shared proposal disposition for every supplied +gating finding. A missing or mismatched disposition makes the lens result +untrustworthy and therefore `blocked`. + +## Validate lens results + +For every result: + +- validate it against the shared result schema and cross-field semantics; +- require the expected lens name; +- require the packet's exact head and comparison-base identity for clean and + changes-required results; +- reject stale, malformed, or mismatched results as `blocked`; and +- retain explicit validation limitations. +- when simplification proposals were supplied to correctness, require exactly + one disposition for each supplied gating finding and no unknown finding IDs. + +## Deduplicate root causes + +Cluster findings only when concrete evidence shows the same root cause and +smallest sufficient correction. Similar location or wording alone is not enough. +Use stable IDs and `related_finding_ids` to retain the contributing identities. + +For a true duplicate: + +1. Keep one finding owned by the lens with the controlling concern: correctness, + then solution simplicity, then code simplicity. +2. Retain unique evidence from every contributing lens. +3. Preserve the highest supported severity and confidence. +4. State the smallest correction that satisfies the controlling constraint. +5. List the other stable IDs in `related_finding_ids`. + +Keep separate findings when their corrections or material impacts differ. + +## Resolve conflicts + +When a simplification proposal conflicts with demonstrated correctness, +security, compatibility, migration, concurrency, or repository constraints: + +- require correctness to mark the proposal `unsafe` with concrete evidence; +- omit the rejected simplification finding from the aggregate findings; +- preserve the disposition in the aggregate; and +- do not create a correctness finding unless the current candidate itself has a + demonstrated defect. + +When correctness marks a proposal `compatible`, keep the simplification finding +actionable in the aggregate. A proposal disposition does not erase an unrelated +correctness finding. + +Do not downgrade a correctness finding because a different design might avoid +it. A caller may choose a redesign, but that begins a new full review on a new +head. + +## Aggregate verdicts + +- `blocked` wins when evidence, dependencies, candidate identity, or any + required lens is untrustworthy. +- Otherwise, `changes_required` wins when a blocking or strong-recommendation + finding remains. +- Otherwise, return `clean`, including any real deferred findings. + +Deferred-only results are clean because deferred findings do not gate the +candidate. Never erase their evidence or silently promote them into the active +scope. + +## Re-review matrix + +| Change applied by caller | New-head sequence | +| ------------------------ | ----------------------------------------------------------- | +| solution redesign | solution, correctness, code | +| correctness fix | correctness, then affected downstream lenses | +| code-simplicity fix | code, then targeted correctness | +| base-only drift | apply shared drift rules, then reset only affected evidence | + +Count a full cycle when a candidate reaches a material finding and the caller +supplies a new head for another full or required downstream pass. At cycle +three, return unresolved findings and tell the caller that the automatic cycle +budget is exhausted. + +## Preserve read-only operation + +Record candidate state before delegation and verify it afterward. Treat an +unexpected modification by any delegated lens as an integrity failure and return +`blocked`. The caller owns all changes and external workflow actions. diff --git a/skills/review-code-change/scripts/tests/test_orchestration_contract.py b/skills/review-code-change/scripts/tests/test_orchestration_contract.py new file mode 100644 index 0000000..0bbca4c --- /dev/null +++ b/skills/review-code-change/scripts/tests/test_orchestration_contract.py @@ -0,0 +1,206 @@ +from __future__ import annotations + +import importlib.util +import json +import unittest +from pathlib import Path + +SKILL_ROOT = Path(__file__).resolve().parents[2] +REPOSITORY_ROOT = SKILL_ROOT.parents[1] +REVIEW_SUITE = REPOSITORY_ROOT / "review-suite" +SPEC = importlib.util.spec_from_file_location( + "review_contract_validator", REVIEW_SUITE / "scripts" / "validate.py" +) +assert SPEC and SPEC.loader +VALIDATOR = importlib.util.module_from_spec(SPEC) +SPEC.loader.exec_module(VALIDATOR) + + +def load(path: Path): + return json.loads(path.read_text()) + + +class OrchestrationContractTests(unittest.TestCase): + @classmethod + def setUpClass(cls): + cls.skill = (SKILL_ROOT / "SKILL.md").read_text() + cls.protocol = ( + SKILL_ROOT / "references" / "orchestration-protocol.md" + ).read_text() + cls.cases = { + case["id"]: case for case in load(SKILL_ROOT / "evals" / "cases.json") + } + cls.results = { + record["case_id"]: record + for record in load(SKILL_ROOT / "evals" / "results.json") + } + + def test_skill_uses_only_local_lenses_in_default_order(self): + names = [ + "review-solution-simplicity", + "review-correctness", + "review-code-simplicity", + ] + positions = [ + self.skill.index( + f"`{name}`", self.skill.index("Run the deliberate sequence") + ) + for name in names + ] + self.assertEqual(sorted(positions), positions) + self.assertNotIn("code-review-pro", self.skill + self.protocol) + self.assertIn("Build the shared packet once", self.skill) + self.assertIn("implementation transcripts", self.skill) + self.assertIn("at most three", self.skill) + self.assertIn("Do not edit", self.skill) + + def test_forward_evaluations_cover_every_case_and_conform(self): + self.assertEqual(set(self.cases), set(self.results)) + for case_id, record in self.results.items(): + with self.subTest(case=case_id): + result = record["result"] + self.assertEqual([], VALIDATOR.validate_result(result)) + self.assertEqual("aggregate", result["lens"]) + self.assertEqual(self.cases[case_id]["candidate"], result["candidate"]) + + def test_harness_lens_results_are_valid_and_candidate_bound(self): + for case_id, case in self.cases.items(): + for expected_lens, result in case["harness_outcomes"].items(): + with self.subTest(case=case_id, lens=expected_lens): + self.assertEqual([], VALIDATOR.validate_result(result)) + if ( + case_id == "mismatched-lens-result" + and expected_lens == "solution_simplicity" + ): + self.assertNotEqual(expected_lens, result["lens"]) + self.assertNotEqual(case["candidate"], result["candidate"]) + else: + self.assertEqual(expected_lens, result["lens"]) + self.assertEqual(case["candidate"], result["candidate"]) + + def test_sequence_and_verdict_boundaries(self): + expected = { + "ordered-clean": ( + ["solution_simplicity", "correctness", "code_simplicity"], + "clean", + ), + "early-solution-redesign": (["solution_simplicity"], "changes_required"), + "correctness-overrides-simplification": ( + ["solution_simplicity", "correctness", "code_simplicity"], + "clean", + ), + "deduplicate-overlap": ( + ["solution_simplicity", "correctness", "code_simplicity"], + "changes_required", + ), + "targeted-correctness-recheck": ( + ["code_simplicity", "correctness"], + "clean", + ), + "missing-dependency": ([], "blocked"), + "missing-evidence": ([], "blocked"), + "deferred-only-clean": ( + ["solution_simplicity", "correctness", "code_simplicity"], + "clean", + ), + "cycle-budget-exhausted": ( + ["solution_simplicity", "correctness"], + "changes_required", + ), + "mismatched-lens-result": (["solution_simplicity"], "blocked"), + } + for case_id, (sequence, verdict) in expected.items(): + with self.subTest(case=case_id): + record = self.results[case_id] + self.assertEqual(sequence, record["observed_sequence"]) + self.assertEqual(verdict, record["result"]["verdict"]) + + def test_correctness_dispositions_cover_supplied_solution_proposals(self): + for case_id, case in self.cases.items(): + observed = self.results[case_id]["observed_sequence"] + solution = case["harness_outcomes"].get("solution_simplicity") + correctness = case["harness_outcomes"].get("correctness") + if not solution or "correctness" not in observed: + continue + gating_ids = { + finding["id"] + for finding in solution["findings"] + if finding["severity"] in {"blocking", "strong_recommendation"} + } + if not gating_ids: + continue + with self.subTest(case=case_id): + disposition_ids = { + item["finding_id"] + for item in correctness.get("proposal_dispositions", []) + } + self.assertEqual(gating_ids, disposition_ids) + aggregate_ids = { + item["finding_id"] + for item in self.results[case_id]["result"].get( + "proposal_dispositions", [] + ) + } + self.assertEqual(gating_ids, aggregate_ids) + + def test_conflict_dedup_deferred_and_budget_semantics(self): + conflict = self.results["correctness-overrides-simplification"]["result"] + self.assertEqual([], conflict["findings"]) + self.assertEqual( + "unsafe", + conflict["proposal_dispositions"][0]["disposition"], + ) + self.assertEqual( + "solution-simplicity.remove-conditional-write", + conflict["proposal_dispositions"][0]["finding_id"], + ) + + deduplicated = self.results["deduplicate-overlap"]["result"] + self.assertEqual(1, len(deduplicated["findings"])) + self.assertIn( + "code-simplicity.reuse-active-admin", + deduplicated["findings"][0]["related_finding_ids"], + ) + + deferred = self.results["deferred-only-clean"]["result"] + self.assertTrue( + all(item["severity"] == "defer" for item in deferred["findings"]) + ) + + exhausted = self.results["cycle-budget-exhausted"]["result"] + self.assertIn("budget is exhausted", exhausted["next_action"]) + + def test_standalone_raw_evidence_builds_one_clean_ordered_review(self): + evaluation = SKILL_ROOT / "evals" / "standalone-clean" + prompt = (evaluation / "prompt.md").read_text() + evidence = "\n".join( + (evaluation / name).read_text() + for name in ( + "ticket.md", + "repository-evidence.md", + "candidate.diff", + "validation.md", + ) + ) + record = load(evaluation / "result.json") + + self.assertNotIn("expected", prompt.lower()) + self.assertNotIn("change_contract", evidence) + self.assertEqual( + [ + "review-solution-simplicity", + "review-correctness", + "review-code-simplicity", + ], + record["observed_sequence"], + ) + self.assertEqual([], VALIDATOR.validate_result(record["result"])) + self.assertEqual("clean", record["result"]["verdict"]) + self.assertEqual( + "1212121212121212121212121212121212121212", + record["result"]["candidate"]["head_sha"], + ) + + +if __name__ == "__main__": + unittest.main() diff --git a/skills/review-correctness/SKILL.md b/skills/review-correctness/SKILL.md index 4c4ea55..a3a6e8e 100644 --- a/skills/review-correctness/SKILL.md +++ b/skills/review-correctness/SKILL.md @@ -33,6 +33,9 @@ to the caller. validation evidence cannot be established. - Bind the result to the packet's candidate identity. Never reuse evidence after a head change. Apply the shared base-drift rules when only the base advances. +- When an orchestrator supplies a validated simplification result beside the + packet, assess each gating proposal against the packet's requirements and + preserved behavior. Keep the packet itself unchanged. ## Review in priority order @@ -71,6 +74,19 @@ Do not perform the dedicated whole-solution or local code-simplicity lenses. Report correctness consequences of complexity only when they create a concrete failure or make required behavior unprovable. +## Disposition supplied simplification proposals + +For every gating proposal supplied by the orchestrator, return one shared +`proposal_dispositions` item: + +- use `compatible` when the proposal preserves demonstrated correctness; or +- use `unsafe` when concrete correctness or repository evidence invalidates the + proposal even though the current candidate may already be correct. + +Do not turn a rejected hypothetical edit into a candidate correctness finding. A +proposal disposition does not change the correctness verdict by itself. If the +proposal cannot be assessed from trustworthy evidence, return `blocked`. + ## Return the shared result Return only JSON conforming to @@ -82,6 +98,8 @@ Return only JSON conforming to - Return `blocked` when essential evidence or a product or architecture decision prevents a trustworthy verdict. - Keep deferred findings non-gating. +- Include proposal dispositions when the orchestrator supplied simplification + proposals, using the shared contract shape. - Do not add praise, a scorecard, generic resources, or prose outside the shared result. diff --git a/skills/review-correctness/scripts/tests/test_skill_contract.py b/skills/review-correctness/scripts/tests/test_skill_contract.py index 862de41..2e2e8a1 100644 --- a/skills/review-correctness/scripts/tests/test_skill_contract.py +++ b/skills/review-correctness/scripts/tests/test_skill_contract.py @@ -25,6 +25,8 @@ def test_skill_uses_shared_contract_and_is_read_only(self): skill = (SKILL_ROOT / "SKILL.md").read_text() self.assertIn("../../review-suite/CONTRACT.md", skill) self.assertIn("Preserve read-only integrity", skill) + self.assertIn("proposal_dispositions", skill) + self.assertIn("Do not turn a rejected hypothetical edit", skill) self.assertNotIn("code-review-pro", skill) def test_correctness_fixture_results_conform(self):