Add goal-first review-correctness skill

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

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
 

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."
+}

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."]}
+}

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},

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."
+}

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"}
+  ]
+}

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."
+}

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"}
+  ]
+}

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.

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."

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

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.

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.

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."
+}

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.

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.