From 33feab3570363f8bf0d24ed4295495dc05fa3abf Mon Sep 17 00:00:00 2001 From: Scott Haug Date: Sun, 19 Jul 2026 20:23:04 -0700 Subject: [PATCH] feat: add goal-first correctness review ## Summary - Add a standalone read-only correctness review skill with a focused rubric and shared-contract output - Add raw authorization, missing-test, and repository-convention fixtures - Add contract tests and validate the skill through clean-context forward tests ## Why - Provide repository-owned correctness, security, compatibility, and validation review without a third-party runtime dependency - Ground findings in ticket intent and local repository evidence before generic advice --- CHANGELOG.md | 2 + README.md | 2 + .../fixtures/auth-regression/expected.json | 21 +++++ .../fixtures/auth-regression/packet.json | 29 ++++++ review-suite/fixtures/manifest.json | 3 + .../fixtures/missing-test/expected.json | 21 +++++ .../fixtures/missing-test/packet.json | 28 ++++++ .../repository-convention-clean/expected.json | 9 ++ .../repository-convention-clean/packet.json | 28 ++++++ skills/review-correctness/SKILL.md | 93 +++++++++++++++++++ skills/review-correctness/agents/openai.yaml | 4 + .../candidate.diff | 14 +++ .../standalone-ticket-regression/prompt.md | 6 ++ .../repository-evidence.md | 11 +++ .../standalone-ticket-regression/result.json | 42 +++++++++ .../standalone-ticket-regression/ticket.md | 9 ++ .../validation.md | 7 ++ .../references/correctness-rubric.md | 84 +++++++++++++++++ .../scripts/tests/test_skill_contract.py | 75 +++++++++++++++ 19 files changed, 488 insertions(+) create mode 100644 review-suite/fixtures/auth-regression/expected.json create mode 100644 review-suite/fixtures/auth-regression/packet.json create mode 100644 review-suite/fixtures/missing-test/expected.json create mode 100644 review-suite/fixtures/missing-test/packet.json create mode 100644 review-suite/fixtures/repository-convention-clean/expected.json create mode 100644 review-suite/fixtures/repository-convention-clean/packet.json create mode 100644 skills/review-correctness/SKILL.md create mode 100644 skills/review-correctness/agents/openai.yaml create mode 100644 skills/review-correctness/evals/standalone-ticket-regression/candidate.diff create mode 100644 skills/review-correctness/evals/standalone-ticket-regression/prompt.md create mode 100644 skills/review-correctness/evals/standalone-ticket-regression/repository-evidence.md create mode 100644 skills/review-correctness/evals/standalone-ticket-regression/result.json create mode 100644 skills/review-correctness/evals/standalone-ticket-regression/ticket.md create mode 100644 skills/review-correctness/evals/standalone-ticket-regression/validation.md create mode 100644 skills/review-correctness/references/correctness-rubric.md create mode 100644 skills/review-correctness/scripts/tests/test_skill_contract.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 36921ef..4429a68 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: add goal-first correctness review - feat: define shared code review contracts + (5600132585c502b21434a938e0319ba58521ee67) - feat: add epic sequence implementation skill (06bd81f4293a24e12cde1f0e466596b41095e8f4) - revert: remove modular code review contract diff --git a/README.md b/README.md index 9b58c4b..48f208f 100644 --- a/README.md +++ b/README.md @@ -15,6 +15,8 @@ Current skills: dependency-aware implementation, review, merge, cleanup, and closeout - `skills/prepare-changesets` — decompose a large, review-ready branch into a deterministic chain of smaller, reviewable changesets and GitHub PRs +- `skills/review-correctness` — find material behavioral, security, + compatibility, data-integrity, and validation failures in a code change ## Quick Start diff --git a/review-suite/fixtures/auth-regression/expected.json b/review-suite/fixtures/auth-regression/expected.json new file mode 100644 index 0000000..cc9584b --- /dev/null +++ b/review-suite/fixtures/auth-regression/expected.json @@ -0,0 +1,21 @@ +{ + "schema_version": "1.0", + "lens": "correctness", + "candidate": {"head_sha": "8181818181818181818181818181818181818181", "comparison_base_sha": "9191919191919191919191919191919191919191"}, + "verdict": "changes_required", + "findings": [{ + "id": "correctness.export-authorization", + "lens": "correctness", + "severity": "blocking", + "confidence": "high", + "rule": "Non-administrators remain forbidden from exporting customer records.", + "evidence": [{"location": "export.py:2", "detail": "The candidate checks only that an actor exists, so any authenticated role can export the record."}], + "concern": "The change removes the role check at the data-export trust boundary.", + "impact": "Ordinary users can access tenant-private customer exports.", + "proposed_change": "Restore the administrator role check and cover a non-administrator denial.", + "expected_effect": "Preserve the required export authorization boundary.", + "location": "export.py:2" + }], + "blocking_reasons": [], + "next_action": "Restore role-based authorization and add the missing denial test." +} diff --git a/review-suite/fixtures/auth-regression/packet.json b/review-suite/fixtures/auth-regression/packet.json new file mode 100644 index 0000000..031707b --- /dev/null +++ b/review-suite/fixtures/auth-regression/packet.json @@ -0,0 +1,29 @@ +{ + "schema_version": "1.0", + "repository": {"identity": "example/admin", "base_branch": "main"}, + "candidate": { + "head_sha": "8181818181818181818181818181818181818181", + "comparison_base_sha": "9191919191919191919191919191919191919191", + "diff": { + "format": "unified_diff", + "complete": true, + "content": "diff --git a/export.py b/export.py\n--- a/export.py\n+++ b/export.py\n@@ -1,4 +1,4 @@\n def export_customer_data(actor, customer):\n- if actor.role != 'admin':\n+ if actor is None:\n raise Forbidden()\n return serialize(customer)\n" + } + }, + "change_contract": { + "goal": "Allow administrators to export customer records.", + "acceptance_criteria": ["Administrators may export records.", "Non-administrators remain forbidden."], + "non_goals": ["Expand export access to ordinary users."], + "preserved_behaviors": ["The admin role check remains the export authorization boundary."] + }, + "sources": { + "repository_instructions": [], + "named_documents": [{"label": "Export authorization policy", "location": "SECURITY.md", "summary": "Only administrators may export customer records."}], + "nearby_patterns": [] + }, + "validation": [ + {"name": "export tests", "command": "pytest tests/test_export.py", "scope": "focused", "status": "passed", "result": "2 passed"}, + {"name": "full tests", "command": "pytest", "scope": "full", "status": "passed", "result": "30 passed"} + ], + "context": {"authorization": ["Customer exports contain tenant-private data."]} +} diff --git a/review-suite/fixtures/manifest.json b/review-suite/fixtures/manifest.json index 34ffdd4..a0fca44 100644 --- a/review-suite/fixtures/manifest.json +++ b/review-suite/fixtures/manifest.json @@ -1,5 +1,8 @@ [ {"name": "behavior-bug", "packet_valid": true}, + {"name": "auth-regression", "packet_valid": true}, + {"name": "missing-test", "packet_valid": true}, + {"name": "repository-convention-clean", "packet_valid": true}, {"name": "duplicated-policy", "packet_valid": true}, {"name": "imagined-machinery", "packet_valid": true}, {"name": "necessary-complexity", "packet_valid": true}, diff --git a/review-suite/fixtures/missing-test/expected.json b/review-suite/fixtures/missing-test/expected.json new file mode 100644 index 0000000..6ca1053 --- /dev/null +++ b/review-suite/fixtures/missing-test/expected.json @@ -0,0 +1,21 @@ +{ + "schema_version": "1.0", + "lens": "correctness", + "candidate": {"head_sha": "8282828282828282828282828282828282828282", "comparison_base_sha": "9292929292929292929292929292929292929292"}, + "verdict": "changes_required", + "findings": [{ + "id": "correctness.invalid-count-test", + "lens": "correctness", + "severity": "blocking", + "confidence": "high", + "rule": "Non-numeric input raises InvalidCount.", + "evidence": [{"location": "test_parser.py:1", "detail": "Both tests cover numeric success; neither executes the ticket's required failure behavior."}], + "concern": "The only new acceptance behavior is not exercised by validation.", + "impact": "The domain-error contract can regress while all supplied tests remain green.", + "proposed_change": "Replace the redundant larger-count test with a non-numeric input assertion for InvalidCount and its input.", + "expected_effect": "Make the required error conversion executable and regression-protected.", + "location": "test_parser.py:4" + }], + "blocking_reasons": [], + "next_action": "Add the acceptance-path failure test and rerun focused and full validation." +} diff --git a/review-suite/fixtures/missing-test/packet.json b/review-suite/fixtures/missing-test/packet.json new file mode 100644 index 0000000..fde5bbd --- /dev/null +++ b/review-suite/fixtures/missing-test/packet.json @@ -0,0 +1,28 @@ +{ + "schema_version": "1.0", + "repository": {"identity": "example/parser", "base_branch": "main"}, + "candidate": { + "head_sha": "8282828282828282828282828282828282828282", + "comparison_base_sha": "9292929292929292929292929292929292929292", + "diff": { + "format": "unified_diff", + "complete": true, + "content": "diff --git a/parser.py b/parser.py\n--- a/parser.py\n+++ b/parser.py\n@@ -1,2 +1,4 @@\n def parse_count(value):\n+ if not value.isdigit():\n+ raise InvalidCount(value)\n return int(value)\ndiff --git a/test_parser.py b/test_parser.py\n--- a/test_parser.py\n+++ b/test_parser.py\n@@ -1,2 +1,5 @@\n def test_count():\n assert parse_count('2') == 2\n+\n+def test_larger_count():\n+ assert parse_count('20') == 20\n" + } + }, + "change_contract": { + "goal": "Return the domain InvalidCount error for non-numeric input.", + "acceptance_criteria": ["Non-numeric input raises InvalidCount.", "Numeric parsing remains unchanged."], + "non_goals": ["Accept signed or decimal counts."], + "preserved_behaviors": ["Numeric strings return their integer value."] + }, + "sources": { + "repository_instructions": [], + "named_documents": [], + "nearby_patterns": [{"label": "Domain error tests", "location": "test_amount_parser.py", "summary": "Error conversions are asserted by exception type and input."}] + }, + "validation": [ + {"name": "parser tests", "command": "pytest test_parser.py", "scope": "focused", "status": "passed", "result": "2 passed"}, + {"name": "full tests", "command": "pytest", "scope": "full", "status": "passed", "result": "18 passed"} + ] +} diff --git a/review-suite/fixtures/repository-convention-clean/expected.json b/review-suite/fixtures/repository-convention-clean/expected.json new file mode 100644 index 0000000..fc6adfe --- /dev/null +++ b/review-suite/fixtures/repository-convention-clean/expected.json @@ -0,0 +1,9 @@ +{ + "schema_version": "1.0", + "lens": "correctness", + "candidate": {"head_sha": "8383838383838383838383838383838383838383", "comparison_base_sha": "9393939393939393939393939393939393939393"}, + "verdict": "clean", + "findings": [], + "blocking_reasons": [], + "next_action": "No material correctness action is required." +} diff --git a/review-suite/fixtures/repository-convention-clean/packet.json b/review-suite/fixtures/repository-convention-clean/packet.json new file mode 100644 index 0000000..2189b95 --- /dev/null +++ b/review-suite/fixtures/repository-convention-clean/packet.json @@ -0,0 +1,28 @@ +{ + "schema_version": "1.0", + "repository": {"identity": "example/catalog", "base_branch": "main"}, + "candidate": { + "head_sha": "8383838383838383838383838383838383838383", + "comparison_base_sha": "9393939393939393939393939393939393939393", + "diff": { + "format": "unified_diff", + "complete": true, + "content": "diff --git a/catalog.py b/catalog.py\n--- a/catalog.py\n+++ b/catalog.py\n@@ -1,2 +1,5 @@\n def optional_product(store, product_id):\n- return store.get(product_id)\n+ try:\n+ return store.get(product_id)\n+ except ProductNotFound:\n+ return None\n" + } + }, + "change_contract": { + "goal": "Return None when the optional catalog lookup has no product.", + "acceptance_criteria": ["Missing products return None.", "Present products are returned unchanged."], + "non_goals": ["Suppress storage or transport failures."], + "preserved_behaviors": ["Only ProductNotFound represents an absent optional value."] + }, + "sources": { + "repository_instructions": [{"label": "Optional lookup convention", "location": "CONTRIBUTING.md", "summary": "Optional repository readers translate the domain not-found exception to None at the reader boundary."}], + "named_documents": [], + "nearby_patterns": [{"label": "Optional customer lookup", "location": "customers.py:optional_customer", "summary": "Catches only CustomerNotFound and returns None."}] + }, + "validation": [ + {"name": "catalog tests", "command": "pytest tests/test_catalog.py", "scope": "focused", "status": "passed", "result": "3 passed including present, missing, and transport failure"}, + {"name": "full tests", "command": "pytest", "scope": "full", "status": "passed", "result": "25 passed"} + ] +} diff --git a/skills/review-correctness/SKILL.md b/skills/review-correctness/SKILL.md new file mode 100644 index 0000000..4c4ea55 --- /dev/null +++ b/skills/review-correctness/SKILL.md @@ -0,0 +1,93 @@ +--- +name: review-correctness +description: Review a code change against its stated goal for material behavioral, security, authorization, compatibility, data-integrity, performance, and validation failures. Use for a correctness-focused PR, branch, or patch review, either from raw repository and ticket evidence or from the repository-owned shared review packet. Return only the shared finding and verdict shape and never modify the reviewed candidate. +--- + +# Review Correctness + +Determine whether the candidate satisfies its observable contract without +introducing a material failure. Review only; leave fixes and workflow mutations +to the caller. + +## Load the contracts + +1. Read the canonical review contract at `../../review-suite/CONTRACT.md` and + its packet and result schemas. +2. Read [the correctness rubric](references/correctness-rubric.md). +3. Treat the canonical contract as authoritative for required evidence, finding + fields, severity, confidence, verdicts, candidate identity, and base drift. +4. Return `blocked` with the missing dependency when the canonical contract is + unavailable. Do not invent a local replacement. + +## Establish the candidate + +- When given a shared review packet, validate it before reviewing. Reject + malformed structure. Convert missing essential evidence into a conforming + `blocked` result. +- When invoked from raw evidence, construct the packet conceptually before + inspecting implementation details. Establish repository, base, captured head, + complete diff, observable goal, acceptance criteria, explicit non-goals, + preserved behavior, source documents, and focused and full validation. +- Do not infer missing intent from the implementation. Return `blocked` when the + goal, acceptance criteria, candidate identity, complete diff, or required + 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. + +## Review in priority order + +1. Compare the observable goal, acceptance criteria, non-goals, and preserved + behavior with the complete candidate diff. +2. Read applicable repository instructions, named architecture or contract + documents, representative nearby implementation, and nearby tests. +3. Inspect security and authorization boundaries before lower-risk behavior. +4. Inspect applicable behavioral, failure, concurrency, data-integrity, + compatibility, validation, and material performance dimensions from the + rubric. +5. Prefer explicit repository rules and demonstrated local idioms over generic + advice. +6. Check that tests and exact validation evidence prove success, failure, + regression, and preserved behavior required by the change contract. + +Do not mechanically emit every category. Follow evidence into the dimensions +that can materially affect this candidate. + +## Apply the finding threshold + +Raise a finding only when concrete ticket, code, test, repository, or runtime +evidence demonstrates a material current concern and supports a smallest +sufficient correction. + +- Mark a demonstrated correctness, security, authorization, acceptance, + architecture, compatibility, or validation failure `blocking`. +- Use `strong_recommendation` only for a material, tractable, ticket-scoped + safety improvement with demonstrated current risk. +- Use `defer` only for a real, evidenced concern intentionally outside the + active ticket or dependent on a missing decision. +- Omit style, praise, broad modernization, numerical quality rules, speculative + hardening, imagined compatibility, and generic best-practice advice. + +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. + +## Return the shared result + +Return only JSON conforming to +`../../review-suite/contracts/review-result.schema.json` with lens +`correctness`. + +- Return `clean` when no blocking or strong-recommendation finding remains. +- Return `changes_required` when at least one actionable gating finding remains. +- Return `blocked` when essential evidence or a product or architecture decision + prevents a trustworthy verdict. +- Keep deferred findings non-gating. +- Do not add praise, a scorecard, generic resources, or prose outside the shared + result. + +## Preserve read-only integrity + +Do not edit or format files, create repository artifacts, commit, push, resolve +threads, post reviews, or update tickets. Run only safe read-only inspection and +validation commands. When the caller supplies pre-review candidate state, +preserve it exactly and report any unexpected mutation as an integrity failure. diff --git a/skills/review-correctness/agents/openai.yaml b/skills/review-correctness/agents/openai.yaml new file mode 100644 index 0000000..f039e31 --- /dev/null +++ b/skills/review-correctness/agents/openai.yaml @@ -0,0 +1,4 @@ +interface: + display_name: "Review Correctness" + short_description: "Find material correctness and security failures" + default_prompt: "Use $review-correctness to review this code change for material correctness, security, compatibility, and validation failures." diff --git a/skills/review-correctness/evals/standalone-ticket-regression/candidate.diff b/skills/review-correctness/evals/standalone-ticket-regression/candidate.diff new file mode 100644 index 0000000..59ac5ae --- /dev/null +++ b/skills/review-correctness/evals/standalone-ticket-regression/candidate.diff @@ -0,0 +1,14 @@ +diff --git a/webhooks.py b/webhooks.py +--- a/webhooks.py ++++ b/webhooks.py +@@ -1,9 +1,6 @@ + def handle(event, repository, mailer, clock): +- existing = repository.find(event.id) +- if existing is not None: +- return existing.result +- ++ repository.record_receipt(event.id, clock.now()) + result = apply_event(event) + repository.save_result(event.id, result) + mailer.send(result.notification) + return result diff --git a/skills/review-correctness/evals/standalone-ticket-regression/prompt.md b/skills/review-correctness/evals/standalone-ticket-regression/prompt.md new file mode 100644 index 0000000..f4fba34 --- /dev/null +++ b/skills/review-correctness/evals/standalone-ticket-regression/prompt.md @@ -0,0 +1,6 @@ +Use the `review-correctness` 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` or use any +prior review conclusion. Reconstruct the observable change contract from those +sources before inspecting the implementation. Return only the shared review +result JSON and do not modify any files or repository state. diff --git a/skills/review-correctness/evals/standalone-ticket-regression/repository-evidence.md b/skills/review-correctness/evals/standalone-ticket-regression/repository-evidence.md new file mode 100644 index 0000000..f57ef6b --- /dev/null +++ b/skills/review-correctness/evals/standalone-ticket-regression/repository-evidence.md @@ -0,0 +1,11 @@ +# Repository evidence + +Repository: `example/webhooks` Base branch: `main` Candidate head: +`8484848484848484848484848484848484848484` Comparison base: +`9494949494949494949494949494949494949494` + +`AGENTS.md` requires webhook handlers to remain idempotent by provider event ID. +`webhooks.py` is the only handler for these events. Before this candidate it +looked up an existing delivery before applying the event. Nearby handler tests +assert that a duplicate event returns the stored result and does not call the +mailer a second time. diff --git a/skills/review-correctness/evals/standalone-ticket-regression/result.json b/skills/review-correctness/evals/standalone-ticket-regression/result.json new file mode 100644 index 0000000..113c73c --- /dev/null +++ b/skills/review-correctness/evals/standalone-ticket-regression/result.json @@ -0,0 +1,42 @@ +{ + "schema_version": "1.0", + "lens": "correctness", + "candidate": { + "head_sha": "8484848484848484848484848484848484848484", + "comparison_base_sha": "9494949494949494949494949494949494949494" + }, + "verdict": "changes_required", + "findings": [ + { + "id": "correctness.duplicate-idempotency-regression", + "lens": "correctness", + "severity": "blocking", + "confidence": "high", + "rule": "Duplicate deliveries with the same provider event ID must return the stored result without applying the event or sending another notification.", + "evidence": [ + { + "location": "ticket.md", + "detail": "The ticket explicitly preserves provider-event-ID idempotency and requires retries to return the stored result without reapplying the event or resending its notification." + }, + { + "location": "candidate.diff:webhooks.py", + "detail": "The candidate removes repository.find(event.id) and its early return, so every delivery now calls apply_event, save_result, and mailer.send." + }, + { + "location": "repository-evidence.md", + "detail": "The repository requires webhook handlers to remain idempotent by provider event ID, and the prior handler performed the existing-delivery lookup before applying the event." + } + ], + "concern": "A retry of an already stored provider event no longer takes the idempotent return path.", + "impact": "Duplicate deliveries reapply the event and send duplicate notifications, violating an explicit preserved behavior and repository rule.", + "proposed_change": "Restore the existing-delivery lookup and early return before record_receipt, then record received_at only for a provider event ID that is newly received.", + "expected_effect": "First deliveries record received_at before processing, while duplicate deliveries continue returning the stored result without repeated side effects.", + "location": "webhooks.py" + } + ], + "blocking_reasons": [], + "validation_limitations": [ + "The reported passing focused and full test runs conflict with the complete diff and the repository evidence that nearby tests assert duplicate-delivery idempotency; the supplied validation summary does not explain this contradiction." + ], + "next_action": "Restore the provider-event-ID duplicate guard ahead of receipt recording and rerun the duplicate-delivery and full suites." +} diff --git a/skills/review-correctness/evals/standalone-ticket-regression/ticket.md b/skills/review-correctness/evals/standalone-ticket-regression/ticket.md new file mode 100644 index 0000000..bdf816a --- /dev/null +++ b/skills/review-correctness/evals/standalone-ticket-regression/ticket.md @@ -0,0 +1,9 @@ +# Record webhook receipt time + +Our webhook audit needs to show when an event first reached the service. Record +`received_at` for a newly received event before processing it. + +Retries are normal. Delivering the same provider event ID more than once must +continue to return the stored result without applying the event or sending its +notification again. Preserve the existing provider event ID as the idempotency +key. This ticket does not change retry policy or notification content. diff --git a/skills/review-correctness/evals/standalone-ticket-regression/validation.md b/skills/review-correctness/evals/standalone-ticket-regression/validation.md new file mode 100644 index 0000000..826a32a --- /dev/null +++ b/skills/review-correctness/evals/standalone-ticket-regression/validation.md @@ -0,0 +1,7 @@ +# Validation evidence + +- Focused: `pytest tests/test_webhooks.py` passed, 4 tests. The added test + checks that a first delivery records `received_at`. +- Full: `pytest` passed, 37 tests. + +The candidate diff is complete. The test commands did not modify tracked files. diff --git a/skills/review-correctness/references/correctness-rubric.md b/skills/review-correctness/references/correctness-rubric.md new file mode 100644 index 0000000..b878c62 --- /dev/null +++ b/skills/review-correctness/references/correctness-rubric.md @@ -0,0 +1,84 @@ +# Correctness review rubric + +Use only the dimensions applicable to the candidate. Begin with its stated goal +and repository evidence; never turn this reference into a generic checklist +report. + +## Goal and behavior + +- Trace each acceptance criterion to observable implementation and evidence. +- Check logic, state transitions, boundary values, empty states, ordering, + pagination, precision, timezones, and encoding where relevant. +- Verify named preserved behavior and detect regressions outside the happy path. +- Distinguish a real requirement from behavior imagined by the implementation. + +## Security and authorization + +- Verify authentication and authorization at the actual trust boundary. +- Check input validation, injection paths, path traversal, unsafe serialization, + secret handling, and unintended data exposure. +- Check tenant, organization, role, ownership, and field-level access rules. +- Treat a demonstrated privilege expansion or missing trust-boundary check as + blocking. + +Do not request generalized hardening without a reachable current risk. + +## Failure and data integrity + +- Follow error propagation, cleanup, partial failure, and recovery behavior. +- Check transactions, consistency boundaries, retries, idempotency, duplicate + delivery, cancellation, timeouts, and races when the change participates in + them. +- Verify atomicity and fencing requirements before proposing simpler control + flow. +- Check resource ownership and cleanup for files, connections, locks, streams, + and background work. + +## Repository architecture and compatibility + +- Read applicable `AGENTS.md`, contributor guidance, named designs, and public + contracts before judging architecture or idioms. +- Inspect representative nearby code and tests for actual extension points, + module boundaries, error handling, naming, and test conventions. +- Check public APIs, schemas, migrations, serialization, compatibility, and + rollout behavior explicitly required by the ticket or repository. +- Prefer local evidence over generic SOLID, design-pattern, function-length, or + cyclomatic-complexity rules. + +Do not invent legacy consumers, migration requirements, or compatibility +promises. + +## Tests and validation + +- Require evidence for success, relevant failure paths, regressions, and named + preserved behavior. +- Detect assertions that do not exercise the changed behavior, mocks that bypass + the risk, and passing suites that omit a required boundary. +- Treat missing required validation, unexplained unavailable commands, and + contradictory test evidence according to the shared blocked and finding + semantics. +- Do not demand redundant tests when existing coverage or repository convention + already proves the behavior. + +## Performance and operations + +- Report only plausible material regressions such as obvious N+1 work, unbounded + processing, blocking operations on a critical path, leaked resources, or a + documented capacity violation. +- Tie operational findings to current ordering, latency, memory, retry, or + availability requirements. +- Do not recommend caching, parallelism, indexing, batching, or new dependencies + speculatively. + +## False-positive controls + +Omit a concern when: + +- repository instructions or a named contract explicitly establish the candidate + behavior; +- representative nearby code uses the same pattern for the same boundary; +- existing tests already prove the claimed missing behavior; +- the impact is aesthetic, hypothetical, or outside any current requirement; +- the proposed correction merely substitutes a preferred pattern; or +- the concern belongs exclusively to solution-simplicity or code-simplicity + review without a demonstrated correctness consequence. diff --git a/skills/review-correctness/scripts/tests/test_skill_contract.py b/skills/review-correctness/scripts/tests/test_skill_contract.py new file mode 100644 index 0000000..862de41 --- /dev/null +++ b/skills/review-correctness/scripts/tests/test_skill_contract.py @@ -0,0 +1,75 @@ +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 SkillContractTests(unittest.TestCase): + 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.assertNotIn("code-review-pro", skill) + + def test_correctness_fixture_results_conform(self): + expectations = { + "behavior-bug": "changes_required", + "auth-regression": "changes_required", + "missing-test": "changes_required", + "repository-convention-clean": "clean", + } + for fixture_name, verdict in expectations.items(): + with self.subTest(fixture=fixture_name): + fixture = REVIEW_SUITE / "fixtures" / fixture_name + packet = load(fixture / "packet.json") + result = load(fixture / "expected.json") + self.assertEqual([], VALIDATOR.validate_pair(packet, result)) + self.assertEqual("correctness", result["lens"]) + self.assertEqual(verdict, result["verdict"]) + + def test_standalone_eval_uses_raw_evidence(self): + evaluation = SKILL_ROOT / "evals" / "standalone-ticket-regression" + 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", + ) + ) + result = load(evaluation / "result.json") + + self.assertNotIn("expected", prompt.lower()) + self.assertNotIn("change_contract", evidence) + self.assertEqual([], VALIDATOR.validate_result(result)) + self.assertEqual("correctness", result["lens"]) + self.assertEqual("changes_required", result["verdict"]) + self.assertEqual( + "8484848484848484848484848484848484848484", + result["candidate"]["head_sha"], + ) + self.assertTrue( + any("idempotency" in finding["id"] for finding in result["findings"]) + ) + + +if __name__ == "__main__": + unittest.main()