diff --git a/CHANGELOG.md b/CHANGELOG.md index d567b4d..36921ef 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: define shared code review contracts - feat: add epic sequence implementation skill + (06bd81f4293a24e12cde1f0e466596b41095e8f4) - revert: remove modular code review contract (b889fe4dc313dc50320dcb20f98980b993062c9a) diff --git a/README.md b/README.md index c64796e..9b58c4b 100644 --- a/README.md +++ b/README.md @@ -5,6 +5,8 @@ A personal monorepo for agent skills and supporting scripts. ## Repository Layout - `skills/` — skill folders, each containing a `SKILL.md` and bundled resources +- `review-suite/` — canonical code-review contracts, validators, and raw + evaluation fixtures shared by repository-owned review skills - `justfile` — common tasks for testing, validation, and formatting Current skills: @@ -26,6 +28,13 @@ Run skill-specific tests: ```bash just test-prepare-changesets +just test-review-suite +``` + +Validate a review packet and result together: + +```bash +python3 review-suite/scripts/validate.py pair packet.json result.json ``` Run deterministic local evals (no Codex required): diff --git a/justfile b/justfile index ab43cc5..94d89be 100644 --- a/justfile +++ b/justfile @@ -19,8 +19,15 @@ test: done; \ if [ "$found" -eq 0 ]; then \ echo "No skill tests found under {{skills_dir}}/*/scripts/tests"; \ + fi; \ + if [ -d review-suite/scripts/tests ]; then \ + echo "Running tests in review-suite/scripts/tests"; \ + python3 -m unittest discover -s review-suite/scripts/tests -p 'test_*.py'; \ fi +test-review-suite: + python3 -m unittest discover -s review-suite/scripts/tests -p 'test_*.py' + test-prepare-changesets: python3 -m unittest discover -s {{skills_dir}}/prepare-changesets/scripts/tests -p 'test_*.py' diff --git a/review-suite/CONTRACT.md b/review-suite/CONTRACT.md new file mode 100644 index 0000000..6a74613 --- /dev/null +++ b/review-suite/CONTRACT.md @@ -0,0 +1,114 @@ +# Code review suite contract + +This directory is the canonical, non-skill foundation for the repository-owned +code review suite. Review skills consume these contracts; they must not copy or +silently redefine them. + +## Contract ownership + +- `contracts/review-packet.schema.json` owns the packet shape. +- `contracts/review-result.schema.json` owns the finding and result shapes. +- This document owns the cross-field semantics that JSON Schema cannot express + clearly. +- `scripts/validate.py` enforces both schemas and these semantic rules without + third-party dependencies. +- `fixtures/` contains raw review inputs and separate expected material outcomes + for deterministic tests and independent forward tests. + +## Review packet + +A review packet binds the review to one candidate and states what that candidate +must accomplish. Required evidence is deliberately distinct from optional +context. + +Required packet sections are: + +1. `repository`: repository identity and base branch. +2. `candidate`: the captured head, the comparison base or merge base, and the + complete candidate diff. +3. `change_contract`: observable goal, non-empty acceptance criteria, explicit + non-goals, and behavior or invariants to preserve. +4. `sources`: applicable repository instructions, named design or contract + documents, and representative nearby patterns. Arrays may be empty only when + the caller has established that no such source applies. +5. `validation`: at least one `focused` and one `full` validation entry, with + every required command's exact result or an explicit reason that the command + is unavailable. + +Optional `context` records public API, data, authorization, compatibility, and +operational concerns when applicable. Optional `worktree` records tracked, +staged, unstaged, untracked, and ignored state when candidate integrity depends +on it. Optional `base_drift` records why evidence was retained or reset after +the base advanced. + +Do not infer missing intent. Missing repository identity, goal, acceptance +criteria, candidate identity, a complete diff, or required validation evidence +prevents a trustworthy review and must yield a `blocked` result. + +## Finding semantics + +Every material finding contains: + +- a stable identifier; +- its owning lens; +- severity and confidence; +- the requirement, non-goal, invariant, or repository rule involved; +- concrete evidence; +- the concern and material impact; +- the smallest sufficient proposed change; and +- the expected behavioral or complexity effect. + +Use these severities: + +- `blocking`: a demonstrated correctness, security, authorization, acceptance, + architecture, compatibility, or validation failure that prevents merge. +- `strong_recommendation`: a material, tractable, ticket-scoped improvement + supported by concrete evidence and a sufficiently specified correction. +- `defer`: a real concern intentionally outside the active ticket, dependent on + an unresolved decision, or not justified strongly enough to change the + candidate. + +Do not emit aesthetic preferences, praise, generic resources, numerical quality +scores, imagined compatibility needs, speculative hardening, or abstractions +that merely move complexity behind another name. + +## Verdict semantics + +- `clean`: no `blocking` or `strong_recommendation` finding remains. Deferred + findings may be retained without failing the gate. +- `changes_required`: at least one actionable `blocking` or + `strong_recommendation` finding remains. +- `blocked`: essential evidence or a product or architecture decision is + missing, so no trustworthy merge verdict is possible. Include at least one + concrete `blocking_reason`. + +`clean` and `changes_required` results must include complete candidate identity +and must not include `blocking_reasons`. A `blocked` result may omit candidate +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. + +## Candidate identity and base drift + +Bind every result to the packet's captured head and comparison base. Any edit, +rebase, conflict resolution, or update operation that changes the head +invalidates head-bound evidence and requires a new packet. + +When only the base advances, inspect the effective merge candidate. Retain prior +head-bound evidence only when all of these are true: + +- the effective diff is unchanged; +- the resulting tree is unchanged; +- no conflict exists; +- no relevant base code overlaps the candidate; and +- repository policy does not require a complete reset. + +Record the decision and reason in `base_drift`. Otherwise reset affected or all +evidence as repository policy and the changed candidate require. + +## Fixture use + +Each fixture keeps `expected.json` separate from `prompt.md` and `packet.json`. +Give a forward-testing reviewer only the prompt and raw packet. Do not expose +the expected outcome, implementation transcript, prior conclusions, or suspected +finding. diff --git a/review-suite/contracts/review-packet.schema.json b/review-suite/contracts/review-packet.schema.json new file mode 100644 index 0000000..9dd0af7 --- /dev/null +++ b/review-suite/contracts/review-packet.schema.json @@ -0,0 +1,166 @@ +{ + "$schema": "https://json-schema.org/draft/2020-12/schema", + "$id": "https://github.com/shaug/agent-scripts/review-suite/contracts/review-packet.schema.json", + "title": "Review packet", + "type": "object", + "additionalProperties": false, + "required": [ + "schema_version", + "repository", + "candidate", + "change_contract", + "sources", + "validation" + ], + "properties": { + "schema_version": { + "const": "1.0" + }, + "repository": { + "type": "object", + "additionalProperties": false, + "required": ["identity", "base_branch"], + "properties": { + "identity": {"type": "string", "minLength": 1}, + "base_branch": {"type": "string", "minLength": 1} + } + }, + "candidate": { + "type": "object", + "additionalProperties": false, + "required": ["head_sha", "comparison_base_sha", "diff"], + "properties": { + "head_sha": {"type": "string", "pattern": "^[0-9a-f]{40}$"}, + "comparison_base_sha": { + "type": "string", + "pattern": "^[0-9a-f]{40}$" + }, + "diff": { + "type": "object", + "additionalProperties": false, + "required": ["format", "complete", "content"], + "properties": { + "format": {"enum": ["unified_diff"]}, + "complete": {"const": true}, + "content": {"type": "string", "minLength": 1} + } + } + } + }, + "change_contract": { + "type": "object", + "additionalProperties": false, + "required": [ + "goal", + "acceptance_criteria", + "non_goals", + "preserved_behaviors" + ], + "properties": { + "goal": {"type": "string", "minLength": 1}, + "acceptance_criteria": { + "type": "array", + "minItems": 1, + "items": {"type": "string", "minLength": 1} + }, + "non_goals": { + "type": "array", + "items": {"type": "string", "minLength": 1} + }, + "preserved_behaviors": { + "type": "array", + "items": {"type": "string", "minLength": 1} + } + } + }, + "sources": { + "type": "object", + "additionalProperties": false, + "required": [ + "repository_instructions", + "named_documents", + "nearby_patterns" + ], + "properties": { + "repository_instructions": { + "type": "array", + "items": {"type": "object", "required": ["label", "location"], "properties": {"label": {"type": "string", "minLength": 1}, "location": {"type": "string", "minLength": 1}, "summary": {"type": "string"}}, "additionalProperties": false} + }, + "named_documents": { + "type": "array", + "items": {"type": "object", "required": ["label", "location"], "properties": {"label": {"type": "string", "minLength": 1}, "location": {"type": "string", "minLength": 1}, "summary": {"type": "string"}}, "additionalProperties": false} + }, + "nearby_patterns": { + "type": "array", + "items": {"type": "object", "required": ["label", "location"], "properties": {"label": {"type": "string", "minLength": 1}, "location": {"type": "string", "minLength": 1}, "summary": {"type": "string"}}, "additionalProperties": false} + } + } + }, + "validation": { + "type": "array", + "minItems": 1, + "items": { + "type": "object", + "additionalProperties": false, + "required": ["name", "command", "scope", "status"], + "properties": { + "name": {"type": "string", "minLength": 1}, + "command": {"type": "string", "minLength": 1}, + "scope": {"enum": ["focused", "full"]}, + "status": {"enum": ["passed", "failed", "unavailable"]}, + "result": {"type": "string", "minLength": 1}, + "reason": {"type": "string", "minLength": 1} + } + } + }, + "context": { + "type": "object", + "additionalProperties": false, + "properties": { + "public_api": {"type": "array", "items": {"type": "string", "minLength": 1}}, + "data": {"type": "array", "items": {"type": "string", "minLength": 1}}, + "authorization": {"type": "array", "items": {"type": "string", "minLength": 1}}, + "compatibility": {"type": "array", "items": {"type": "string", "minLength": 1}}, + "operational": {"type": "array", "items": {"type": "string", "minLength": 1}} + } + }, + "worktree": { + "type": "object", + "additionalProperties": false, + "required": ["tracked", "staged", "unstaged", "untracked", "ignored"], + "properties": { + "tracked": {"type": "array", "items": {"type": "string"}}, + "staged": {"type": "array", "items": {"type": "string"}}, + "unstaged": {"type": "array", "items": {"type": "string"}}, + "untracked": {"type": "array", "items": {"type": "string"}}, + "ignored": {"type": "array", "items": {"type": "string"}} + } + }, + "base_drift": { + "type": "object", + "additionalProperties": false, + "required": [ + "captured_base_sha", + "current_base_sha", + "effective_diff_changed", + "resulting_tree_changed", + "conflict", + "relevant_overlap", + "repository_requires_reset", + "decision", + "reason" + ], + "properties": { + "captured_base_sha": {"type": "string", "pattern": "^[0-9a-f]{40}$"}, + "current_base_sha": {"type": "string", "pattern": "^[0-9a-f]{40}$"}, + "effective_diff_changed": {"type": "boolean"}, + "resulting_tree_changed": {"type": "boolean"}, + "conflict": {"type": "boolean"}, + "relevant_overlap": {"type": "boolean"}, + "repository_requires_reset": {"type": "boolean"}, + "decision": {"enum": ["retain", "reset_affected", "reset_all"]}, + "reason": {"type": "string", "minLength": 1} + } + } + } +} diff --git a/review-suite/contracts/review-result.schema.json b/review-suite/contracts/review-result.schema.json new file mode 100644 index 0000000..410d729 --- /dev/null +++ b/review-suite/contracts/review-result.schema.json @@ -0,0 +1,92 @@ +{ + "$schema": "https://json-schema.org/draft/2020-12/schema", + "$id": "https://github.com/shaug/agent-scripts/review-suite/contracts/review-result.schema.json", + "title": "Review result", + "type": "object", + "additionalProperties": false, + "required": [ + "schema_version", + "lens", + "candidate", + "verdict", + "findings", + "blocking_reasons" + ], + "properties": { + "schema_version": {"const": "1.0"}, + "lens": { + "enum": [ + "correctness", + "solution_simplicity", + "code_simplicity", + "aggregate" + ] + }, + "candidate": { + "type": "object", + "additionalProperties": false, + "properties": { + "head_sha": {"type": "string", "pattern": "^[0-9a-f]{40}$"}, + "comparison_base_sha": {"type": "string", "pattern": "^[0-9a-f]{40}$"} + } + }, + "verdict": {"enum": ["clean", "changes_required", "blocked"]}, + "findings": { + "type": "array", + "items": { + "type": "object", + "additionalProperties": false, + "required": [ + "id", + "lens", + "severity", + "confidence", + "rule", + "evidence", + "concern", + "impact", + "proposed_change", + "expected_effect" + ], + "properties": { + "id": {"type": "string", "pattern": "^[a-z0-9][a-z0-9._-]*$"}, + "lens": {"enum": ["correctness", "solution_simplicity", "code_simplicity"]}, + "severity": {"enum": ["blocking", "strong_recommendation", "defer"]}, + "confidence": {"enum": ["high", "medium", "low"]}, + "rule": {"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} + } + } + }, + "concern": {"type": "string", "minLength": 1}, + "impact": {"type": "string", "minLength": 1}, + "proposed_change": {"type": "string", "minLength": 1}, + "expected_effect": {"type": "string", "minLength": 1}, + "location": {"type": "string", "minLength": 1}, + "related_finding_ids": { + "type": "array", + "items": {"type": "string", "pattern": "^[a-z0-9][a-z0-9._-]*$"} + } + } + } + }, + "blocking_reasons": { + "type": "array", + "items": {"type": "string", "minLength": 1} + }, + "validation_limitations": { + "type": "array", + "items": {"type": "string", "minLength": 1} + }, + "next_action": {"type": "string", "minLength": 1} + } +} diff --git a/review-suite/fixtures/PROMPT.md b/review-suite/fixtures/PROMPT.md new file mode 100644 index 0000000..6bc9b99 --- /dev/null +++ b/review-suite/fixtures/PROMPT.md @@ -0,0 +1,4 @@ +Review the supplied `packet.json` as a read-only reviewer. Use the repository's +canonical review packet, finding, and verdict contract. Return only a conforming +review result. Do not inspect `expected.json` or infer expectations from fixture +names. diff --git a/review-suite/fixtures/behavior-bug/expected.json b/review-suite/fixtures/behavior-bug/expected.json new file mode 100644 index 0000000..f706168 --- /dev/null +++ b/review-suite/fixtures/behavior-bug/expected.json @@ -0,0 +1,21 @@ +{ + "schema_version": "1.0", + "lens": "correctness", + "candidate": {"head_sha": "1111111111111111111111111111111111111111", "comparison_base_sha": "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"}, + "verdict": "changes_required", + "findings": [{ + "id": "correctness.equal-balance", + "lens": "correctness", + "severity": "blocking", + "confidence": "high", + "rule": "A charge equal to the available balance succeeds.", + "evidence": [{"location": "limits.py:2", "detail": "The strict comparison rejects amount equal to balance."}], + "concern": "The new boundary condition contradicts the acceptance criterion.", + "impact": "Customers cannot spend their full available balance.", + "proposed_change": "Use a less-than-or-equal balance comparison and add the equality regression test.", + "expected_effect": "Restore the required boundary behavior without changing overdraft policy.", + "location": "limits.py:2" + }], + "blocking_reasons": [], + "next_action": "Correct the comparison and add the missed boundary test." +} diff --git a/review-suite/fixtures/behavior-bug/packet.json b/review-suite/fixtures/behavior-bug/packet.json new file mode 100644 index 0000000..04bbb2d --- /dev/null +++ b/review-suite/fixtures/behavior-bug/packet.json @@ -0,0 +1,29 @@ +{ + "schema_version": "1.0", + "repository": {"identity": "example/payments", "base_branch": "main"}, + "candidate": { + "head_sha": "1111111111111111111111111111111111111111", + "comparison_base_sha": "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + "diff": { + "format": "unified_diff", + "complete": true, + "content": "diff --git a/limits.py b/limits.py\n--- a/limits.py\n+++ b/limits.py\n@@ -1,2 +1,2 @@\n def can_charge(balance, amount):\n- return amount <= balance\n+ return amount < balance\n" + } + }, + "change_contract": { + "goal": "Allow a charge whenever sufficient balance is available.", + "acceptance_criteria": ["A charge equal to the available balance succeeds."], + "non_goals": ["Change overdraft policy."], + "preserved_behaviors": ["Charges above the available balance fail."] + }, + "sources": { + "repository_instructions": [], + "named_documents": [], + "nearby_patterns": [{"label": "Limit boundary tests", "location": "tests/test_limits.py"}] + }, + "validation": [ + {"name": "limit tests", "command": "pytest tests/test_limits.py", "scope": "focused", "status": "passed", "result": "4 passed"}, + {"name": "full tests", "command": "pytest", "scope": "full", "status": "passed", "result": "12 passed"} + ], + "context": {"data": ["Balances and charge amounts use exact integer cents."]} +} diff --git a/review-suite/fixtures/clean-change/expected.json b/review-suite/fixtures/clean-change/expected.json new file mode 100644 index 0000000..3491568 --- /dev/null +++ b/review-suite/fixtures/clean-change/expected.json @@ -0,0 +1,9 @@ +{ + "schema_version": "1.0", + "lens": "aggregate", + "candidate": {"head_sha": "5555555555555555555555555555555555555555", "comparison_base_sha": "eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee"}, + "verdict": "clean", + "findings": [], + "blocking_reasons": [], + "next_action": "No material review action is required." +} diff --git a/review-suite/fixtures/clean-change/packet.json b/review-suite/fixtures/clean-change/packet.json new file mode 100644 index 0000000..a520471 --- /dev/null +++ b/review-suite/fixtures/clean-change/packet.json @@ -0,0 +1,24 @@ +{ + "schema_version": "1.0", + "repository": {"identity": "example/profile", "base_branch": "main"}, + "candidate": { + "head_sha": "5555555555555555555555555555555555555555", + "comparison_base_sha": "eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee", + "diff": { + "format": "unified_diff", + "complete": true, + "content": "diff --git a/profile.py b/profile.py\n--- a/profile.py\n+++ b/profile.py\n@@ -1,2 +1,4 @@\n def display_name(user):\n- return user.name\n+ if user.preferred_name:\n+ return user.preferred_name\n+ return user.name\n" + } + }, + "change_contract": { + "goal": "Display a preferred name when one is present.", + "acceptance_criteria": ["Preferred names take precedence.", "The existing name remains the fallback."], + "non_goals": ["Change name storage or validation."], + "preserved_behaviors": ["Users without a preferred name display their existing name."] + }, + "sources": {"repository_instructions": [], "named_documents": [], "nearby_patterns": [{"label": "Simple fallback convention", "location": "contact.py"}]}, + "validation": [ + {"name": "profile tests", "command": "pytest tests/test_profile.py", "scope": "focused", "status": "passed", "result": "4 passed including preferred and fallback names"}, + {"name": "full tests", "command": "pytest", "scope": "full", "status": "passed", "result": "20 passed"} + ] +} diff --git a/review-suite/fixtures/duplicated-policy/expected.json b/review-suite/fixtures/duplicated-policy/expected.json new file mode 100644 index 0000000..8d57565 --- /dev/null +++ b/review-suite/fixtures/duplicated-policy/expected.json @@ -0,0 +1,21 @@ +{ + "schema_version": "1.0", + "lens": "code_simplicity", + "candidate": {"head_sha": "2222222222222222222222222222222222222222", "comparison_base_sha": "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb"}, + "verdict": "changes_required", + "findings": [{ + "id": "code-simplicity.reuse-auth-policy", + "lens": "code_simplicity", + "severity": "strong_recommendation", + "confidence": "high", + "rule": "Use the repository's existing active-administrator policy helper.", + "evidence": [{"location": "actions.py:2", "detail": "Both actions repeat the same policy."}, {"location": "auth.py:require_active_admin", "detail": "The repository already centralizes this exact rule."}], + "concern": "The change creates three independently editable definitions of one authorization policy.", + "impact": "Future policy changes can leave these actions inconsistent and increase reviewer cognitive load.", + "proposed_change": "Call require_active_admin from both actions.", + "expected_effect": "Remove duplicated policy while preserving behavior.", + "location": "actions.py:2" + }], + "blocking_reasons": [], + "next_action": "Reuse the existing authorization helper and rerun focused tests." +} diff --git a/review-suite/fixtures/duplicated-policy/packet.json b/review-suite/fixtures/duplicated-policy/packet.json new file mode 100644 index 0000000..3450282 --- /dev/null +++ b/review-suite/fixtures/duplicated-policy/packet.json @@ -0,0 +1,28 @@ +{ + "schema_version": "1.0", + "repository": {"identity": "example/admin", "base_branch": "main"}, + "candidate": { + "head_sha": "2222222222222222222222222222222222222222", + "comparison_base_sha": "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb", + "diff": { + "format": "unified_diff", + "complete": true, + "content": "diff --git a/actions.py b/actions.py\nnew file mode 100644\n--- /dev/null\n+++ b/actions.py\n@@ -0,0 +1,8 @@\n+def delete_user(actor, user):\n+ if actor.role != 'admin' or actor.suspended:\n+ raise Forbidden()\n+ store.delete(user)\n+def restore_user(actor, user):\n+ if actor.role != 'admin' or actor.suspended:\n+ raise Forbidden()\n+ store.restore(user)\n" + } + }, + "change_contract": { + "goal": "Add administrator delete and restore actions.", + "acceptance_criteria": ["Active administrators may delete and restore users.", "Other actors are rejected."], + "non_goals": ["Redesign role storage."], + "preserved_behaviors": ["Suspended administrators remain unauthorized."] + }, + "sources": { + "repository_instructions": [], + "named_documents": [], + "nearby_patterns": [{"label": "Shared authorization helper", "location": "auth.py:require_active_admin", "summary": "Existing helper implements the same policy."}] + }, + "validation": [ + {"name": "action tests", "command": "pytest tests/test_actions.py", "scope": "focused", "status": "passed", "result": "6 passed"}, + {"name": "full tests", "command": "pytest", "scope": "full", "status": "passed", "result": "24 passed"} + ] +} diff --git a/review-suite/fixtures/imagined-machinery/expected.json b/review-suite/fixtures/imagined-machinery/expected.json new file mode 100644 index 0000000..a722409 --- /dev/null +++ b/review-suite/fixtures/imagined-machinery/expected.json @@ -0,0 +1,21 @@ +{ + "schema_version": "1.0", + "lens": "solution_simplicity", + "candidate": {"head_sha": "3333333333333333333333333333333333333333", "comparison_base_sha": "cccccccccccccccccccccccccccccccccccccccc"}, + "verdict": "changes_required", + "findings": [{ + "id": "solution-simplicity.remove-provider-framework", + "lens": "solution_simplicity", + "severity": "strong_recommendation", + "confidence": "high", + "rule": "Remote providers and an extension API are explicit non-goals.", + "evidence": [{"location": "export.py:1", "detail": "A registry, factory, provider interface, and selection parameter exist only for unspecified providers."}], + "concern": "The overall design solves imagined provider requirements rather than the requested local-file outcome.", + "impact": "The ticket carries avoidable concepts, configuration, and failure modes.", + "proposed_change": "Replace the provider framework with one direct local-file write at the existing export boundary.", + "expected_effect": "Meet the complete requirement with fewer concepts and no lost capability.", + "location": "export.py:1" + }], + "blocking_reasons": [], + "next_action": "Redesign around the required local-file operation before detailed review continues." +} diff --git a/review-suite/fixtures/imagined-machinery/packet.json b/review-suite/fixtures/imagined-machinery/packet.json new file mode 100644 index 0000000..e721a5e --- /dev/null +++ b/review-suite/fixtures/imagined-machinery/packet.json @@ -0,0 +1,24 @@ +{ + "schema_version": "1.0", + "repository": {"identity": "example/exporter", "base_branch": "main"}, + "candidate": { + "head_sha": "3333333333333333333333333333333333333333", + "comparison_base_sha": "cccccccccccccccccccccccccccccccccccccccc", + "diff": { + "format": "unified_diff", + "complete": true, + "content": "diff --git a/export.py b/export.py\nnew file mode 100644\n--- /dev/null\n+++ b/export.py\n@@ -0,0 +1,24 @@\n+from pathlib import Path\n+\n+class ExportProviderRegistry:\n+ def __init__(self):\n+ self.providers = {}\n+\n+ def register(self, name, factory):\n+ self.providers[name] = factory\n+\n+ def create(self, name):\n+ return self.providers[name]()\n+\n+class LocalFileProvider:\n+ def write(self, path, value):\n+ Path(path).write_text(value)\n+\n+def build_registry():\n+ registry = ExportProviderRegistry()\n+ registry.register('local', LocalFileProvider)\n+ return registry\n+\n+def export(path, value, provider='local'):\n+ registry = build_registry()\n+ registry.create(provider).write(path, value)\n" + } + }, + "change_contract": { + "goal": "Write the generated report to a caller-provided local path.", + "acceptance_criteria": ["The report is written to the requested local file."], + "non_goals": ["Support remote storage providers.", "Create a provider extension API."], + "preserved_behaviors": ["Report generation remains unchanged."] + }, + "sources": {"repository_instructions": [], "named_documents": [], "nearby_patterns": []}, + "validation": [ + {"name": "export tests", "command": "pytest tests/test_export.py", "scope": "focused", "status": "passed", "result": "3 passed"}, + {"name": "full tests", "command": "pytest", "scope": "full", "status": "passed", "result": "8 passed"} + ] +} diff --git a/review-suite/fixtures/manifest.json b/review-suite/fixtures/manifest.json new file mode 100644 index 0000000..34ffdd4 --- /dev/null +++ b/review-suite/fixtures/manifest.json @@ -0,0 +1,9 @@ +[ + {"name": "behavior-bug", "packet_valid": true}, + {"name": "duplicated-policy", "packet_valid": true}, + {"name": "imagined-machinery", "packet_valid": true}, + {"name": "necessary-complexity", "packet_valid": true}, + {"name": "clean-change", "packet_valid": true}, + {"name": "missing-evidence", "packet_valid": false}, + {"name": "unrelated-base-drift", "packet_valid": true} +] diff --git a/review-suite/fixtures/missing-evidence/expected.json b/review-suite/fixtures/missing-evidence/expected.json new file mode 100644 index 0000000..255fc93 --- /dev/null +++ b/review-suite/fixtures/missing-evidence/expected.json @@ -0,0 +1,10 @@ +{ + "schema_version": "1.0", + "lens": "aggregate", + "candidate": {"head_sha": "6666666666666666666666666666666666666666", "comparison_base_sha": "ffffffffffffffffffffffffffffffffffffffff"}, + "verdict": "blocked", + "findings": [], + "blocking_reasons": ["Acceptance criteria are missing.", "Required validation is unavailable without a reason."], + "validation_limitations": ["The required test result cannot be evaluated."], + "next_action": "Supply acceptance criteria and the validation result or unavailable reason." +} diff --git a/review-suite/fixtures/missing-evidence/packet.json b/review-suite/fixtures/missing-evidence/packet.json new file mode 100644 index 0000000..b9cd060 --- /dev/null +++ b/review-suite/fixtures/missing-evidence/packet.json @@ -0,0 +1,19 @@ +{ + "schema_version": "1.0", + "repository": {"identity": "example/unknown", "base_branch": "main"}, + "candidate": { + "head_sha": "6666666666666666666666666666666666666666", + "comparison_base_sha": "ffffffffffffffffffffffffffffffffffffffff", + "diff": {"format": "unified_diff", "complete": true, "content": "diff --git a/a.py b/a.py\n--- a/a.py\n+++ b/a.py\n@@ -1 +1 @@\n-old\n+new\n"} + }, + "change_contract": { + "goal": "Change the behavior.", + "non_goals": [], + "preserved_behaviors": [] + }, + "sources": {"repository_instructions": [], "named_documents": [], "nearby_patterns": []}, + "validation": [ + {"name": "focused tests", "command": "pytest tests/test_change.py", "scope": "focused", "status": "passed", "result": "2 passed"}, + {"name": "required tests", "command": "pytest", "scope": "full", "status": "unavailable"} + ] +} diff --git a/review-suite/fixtures/necessary-complexity/expected.json b/review-suite/fixtures/necessary-complexity/expected.json new file mode 100644 index 0000000..8c9e752 --- /dev/null +++ b/review-suite/fixtures/necessary-complexity/expected.json @@ -0,0 +1,9 @@ +{ + "schema_version": "1.0", + "lens": "solution_simplicity", + "candidate": {"head_sha": "4444444444444444444444444444444444444444", "comparison_base_sha": "dddddddddddddddddddddddddddddddddddddddd"}, + "verdict": "clean", + "findings": [], + "blocking_reasons": [], + "next_action": "Continue to correctness review; the apparent complexity preserves a required concurrency invariant." +} diff --git a/review-suite/fixtures/necessary-complexity/packet.json b/review-suite/fixtures/necessary-complexity/packet.json new file mode 100644 index 0000000..a5df902 --- /dev/null +++ b/review-suite/fixtures/necessary-complexity/packet.json @@ -0,0 +1,29 @@ +{ + "schema_version": "1.0", + "repository": {"identity": "example/worker", "base_branch": "main"}, + "candidate": { + "head_sha": "4444444444444444444444444444444444444444", + "comparison_base_sha": "dddddddddddddddddddddddddddddddddddddddd", + "diff": { + "format": "unified_diff", + "complete": true, + "content": "diff --git a/claims.py b/claims.py\nnew file mode 100644\n--- /dev/null\n+++ b/claims.py\n@@ -0,0 +1,7 @@\n+def complete(job_id, claim_token):\n+ updated = db.update_where(\n+ id=job_id, claim_token=claim_token, status='running',\n+ values={'status': 'complete', 'claim_token': None},\n+ )\n+ if updated != 1:\n+ raise LostClaim(job_id)\n" + } + }, + "change_contract": { + "goal": "Prevent an expired worker from completing a job claimed by another worker.", + "acceptance_criteria": ["Completion succeeds only for the current claim token.", "Lost claims fail without changing the job."], + "non_goals": ["Replace the database claim protocol."], + "preserved_behaviors": ["Completion is atomic with claim verification.", "Concurrent workers cannot both complete one job."] + }, + "sources": { + "repository_instructions": [], + "named_documents": [{"label": "Claim protocol", "location": "DESIGN.md", "summary": "Requires one conditional write to preserve fencing."}], + "nearby_patterns": [] + }, + "validation": [ + {"name": "concurrency tests", "command": "pytest tests/test_claims.py", "scope": "focused", "status": "passed", "result": "6 passed including stale-token race"}, + {"name": "full tests", "command": "pytest", "scope": "full", "status": "passed", "result": "18 passed"} + ], + "context": {"data": ["The database conditional update is the concurrency boundary."]} +} diff --git a/review-suite/fixtures/unrelated-base-drift/expected.json b/review-suite/fixtures/unrelated-base-drift/expected.json new file mode 100644 index 0000000..93b9959 --- /dev/null +++ b/review-suite/fixtures/unrelated-base-drift/expected.json @@ -0,0 +1,9 @@ +{ + "schema_version": "1.0", + "lens": "aggregate", + "candidate": {"head_sha": "7777777777777777777777777777777777777777", "comparison_base_sha": "1212121212121212121212121212121212121212"}, + "verdict": "clean", + "findings": [], + "blocking_reasons": [], + "next_action": "Retain the head-bound evidence and record the unrelated-base-drift justification." +} diff --git a/review-suite/fixtures/unrelated-base-drift/packet.json b/review-suite/fixtures/unrelated-base-drift/packet.json new file mode 100644 index 0000000..d105d5c --- /dev/null +++ b/review-suite/fixtures/unrelated-base-drift/packet.json @@ -0,0 +1,31 @@ +{ + "schema_version": "1.0", + "repository": {"identity": "example/profile", "base_branch": "main"}, + "candidate": { + "head_sha": "7777777777777777777777777777777777777777", + "comparison_base_sha": "1212121212121212121212121212121212121212", + "diff": {"format": "unified_diff", "complete": true, "content": "diff --git a/profile.py b/profile.py\n--- a/profile.py\n+++ b/profile.py\n@@ -1 +1 @@\n-return user.name\n+return user.preferred_name or user.name\n"} + }, + "change_contract": { + "goal": "Display a preferred name when one is present.", + "acceptance_criteria": ["Preferred names take precedence and existing names remain the fallback."], + "non_goals": [], + "preserved_behaviors": ["Existing names remain the fallback."] + }, + "sources": {"repository_instructions": [], "named_documents": [], "nearby_patterns": []}, + "validation": [ + {"name": "profile tests", "command": "pytest tests/test_profile.py", "scope": "focused", "status": "passed", "result": "4 passed"}, + {"name": "full tests", "command": "pytest", "scope": "full", "status": "passed", "result": "20 passed"} + ], + "base_drift": { + "captured_base_sha": "3434343434343434343434343434343434343434", + "current_base_sha": "1212121212121212121212121212121212121212", + "effective_diff_changed": false, + "resulting_tree_changed": false, + "conflict": false, + "relevant_overlap": false, + "repository_requires_reset": false, + "decision": "retain", + "reason": "The base changed only in unrelated documentation; the effective diff and resulting tree are unchanged." + } +} diff --git a/review-suite/scripts/tests/test_contracts.py b/review-suite/scripts/tests/test_contracts.py new file mode 100644 index 0000000..2b5c91e --- /dev/null +++ b/review-suite/scripts/tests/test_contracts.py @@ -0,0 +1,224 @@ +from __future__ import annotations + +import copy +import importlib.util +import json +import subprocess +import unittest +from pathlib import Path + +ROOT = Path(__file__).resolve().parents[2] +SPEC = importlib.util.spec_from_file_location( + "review_contract_validator", ROOT / "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()) + + +def candidate_identity(packet): + return { + field: packet["candidate"][field] + for field in ("head_sha", "comparison_base_sha") + if field in packet["candidate"] + } + + +class FixtureTests(unittest.TestCase): + def test_fixture_packets_and_expected_results(self): + manifest = load(ROOT / "fixtures" / "manifest.json") + self.assertGreaterEqual(len(manifest), 6) + for entry in manifest: + with self.subTest(entry=entry["name"]): + fixture = ROOT / "fixtures" / entry["name"] + packet = load(fixture / "packet.json") + result = load(fixture / "expected.json") + packet_errors = VALIDATOR.validate_packet(packet) + if entry["packet_valid"]: + self.assertEqual([], packet_errors) + else: + self.assertTrue(packet_errors) + self.assertEqual("blocked", result["verdict"]) + self.assertEqual([], VALIDATOR.validate_result(result)) + + def test_expected_outcomes_are_not_in_forward_test_prompt(self): + prompt = (ROOT / "fixtures" / "PROMPT.md").read_text().lower() + self.assertNotIn("behavior-bug", prompt) + self.assertNotIn("changes_required", prompt) + self.assertNotIn("strong_recommendation", prompt) + + def test_unrelated_base_drift_is_accepted(self): + packet = load(ROOT / "fixtures" / "unrelated-base-drift" / "packet.json") + self.assertEqual([], VALIDATOR.validate_packet(packet)) + + def test_every_fixture_diff_is_a_parseable_patch(self): + manifest = load(ROOT / "fixtures" / "manifest.json") + for entry in manifest: + with self.subTest(entry=entry["name"]): + packet = load(ROOT / "fixtures" / entry["name"] / "packet.json") + completed = subprocess.run( + ["git", "apply", "--numstat"], + input=packet["candidate"]["diff"]["content"], + capture_output=True, + check=False, + text=True, + ) + self.assertEqual(0, completed.returncode, completed.stderr) + + +class PacketValidationTests(unittest.TestCase): + def setUp(self): + self.packet = load(ROOT / "fixtures" / "clean-change" / "packet.json") + + def test_missing_required_field_is_rejected(self): + del self.packet["change_contract"]["acceptance_criteria"] + self.assertTrue(VALIDATOR.validate_packet(self.packet)) + + def test_unknown_enum_is_rejected(self): + self.packet["validation"][0]["status"] = "skipped" + self.assertTrue(VALIDATOR.validate_packet(self.packet)) + + def test_unavailable_validation_requires_reason(self): + self.packet["validation"][0] = { + "name": "tests", + "command": "pytest", + "scope": "full", + "status": "unavailable", + } + self.assertIn( + "$.validation[0]: unavailable requires reason", + VALIDATOR.validate_packet(self.packet), + ) + + def test_retain_rejects_active_base_drift_invalidator(self): + drift_packet = load(ROOT / "fixtures" / "unrelated-base-drift" / "packet.json") + drift_packet["base_drift"]["relevant_overlap"] = True + self.assertTrue(VALIDATOR.validate_packet(drift_packet)) + + def test_focused_and_full_validation_are_required(self): + self.packet["validation"] = [self.packet["validation"][0]] + self.assertIn( + "$.validation: missing full validation", + VALIDATOR.validate_packet(self.packet), + ) + + def test_malformed_validation_item_returns_errors(self): + self.packet["validation"] = ["pytest"] + self.assertTrue(VALIDATOR.validate_packet(self.packet)) + + +class ResultValidationTests(unittest.TestCase): + def setUp(self): + self.clean = load(ROOT / "fixtures" / "clean-change" / "expected.json") + self.gating = load(ROOT / "fixtures" / "behavior-bug" / "expected.json") + + def test_clean_with_gating_finding_is_rejected(self): + result = copy.deepcopy(self.gating) + result["verdict"] = "clean" + self.assertTrue(VALIDATOR.validate_result(result)) + + def test_changes_required_without_gating_finding_is_rejected(self): + result = copy.deepcopy(self.clean) + result["verdict"] = "changes_required" + self.assertTrue(VALIDATOR.validate_result(result)) + + def test_blocked_without_reason_is_rejected(self): + result = copy.deepcopy(self.clean) + result["verdict"] = "blocked" + self.assertTrue(VALIDATOR.validate_result(result)) + + def test_deferred_only_clean_result_is_accepted(self): + result = copy.deepcopy(self.clean) + result["findings"] = [ + { + "id": "code-simplicity.existing-duplication", + "lens": "code_simplicity", + "severity": "defer", + "confidence": "high", + "rule": "The active ticket does not own the existing parser duplication.", + "evidence": [ + { + "location": "legacy_parser.py:10", + "detail": "The duplicated parser predates and is untouched by the candidate.", + } + ], + "concern": "An existing parser is duplicated outside the changed code.", + "impact": "The duplication is real but not caused by this ticket.", + "proposed_change": "Address the existing duplication separately.", + "expected_effect": "Preserve active scope while recording evidenced follow-up work.", + } + ] + self.assertEqual([], VALIDATOR.validate_result(result)) + + def test_unknown_finding_enum_is_rejected(self): + result = copy.deepcopy(self.gating) + result["findings"][0]["confidence"] = "certain" + 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) + result["candidate"]["head_sha"] = "9999999999999999999999999999999999999999" + errors = VALIDATOR.validate_pair(packet, result) + self.assertIn("candidate.head_sha: result does not match packet", errors) + + def test_blocked_result_can_omit_missing_candidate_identity(self): + packet = load(ROOT / "fixtures" / "missing-evidence" / "packet.json") + del packet["candidate"]["head_sha"] + result = load(ROOT / "fixtures" / "missing-evidence" / "expected.json") + del result["candidate"]["head_sha"] + self.assertEqual([], VALIDATOR.validate_pair(packet, result)) + + def test_blocked_pair_rejects_unknown_packet_enum(self): + packet = load(ROOT / "fixtures" / "clean-change" / "packet.json") + packet["validation"][0]["status"] = "skipped" + result = load(ROOT / "fixtures" / "missing-evidence" / "expected.json") + result["candidate"] = candidate_identity(packet) + self.assertTrue(VALIDATOR.validate_pair(packet, result)) + + def test_blocked_pair_rejects_unknown_packet_property(self): + packet = load(ROOT / "fixtures" / "clean-change" / "packet.json") + packet["unexpected"] = True + result = load(ROOT / "fixtures" / "missing-evidence" / "expected.json") + result["candidate"] = candidate_identity(packet) + self.assertTrue(VALIDATOR.validate_pair(packet, result)) + + def test_blocked_pair_accepts_missing_exact_validation_result(self): + packet = load(ROOT / "fixtures" / "clean-change" / "packet.json") + del packet["validation"][0]["result"] + result = load(ROOT / "fixtures" / "missing-evidence" / "expected.json") + result["candidate"] = candidate_identity(packet) + self.assertEqual([], VALIDATOR.validate_pair(packet, result)) + + def test_blocked_result_cannot_invent_missing_identity(self): + packet = load(ROOT / "fixtures" / "missing-evidence" / "packet.json") + del packet["candidate"]["head_sha"] + result = load(ROOT / "fixtures" / "missing-evidence" / "expected.json") + errors = VALIDATOR.validate_pair(packet, result) + self.assertIn( + "candidate.head_sha: result invents identity absent from packet", errors + ) + + def test_merge_verdict_requires_complete_candidate_identity(self): + result = copy.deepcopy(self.clean) + del result["candidate"]["head_sha"] + self.assertTrue(VALIDATOR.validate_result(result)) + + def test_malformed_finding_returns_errors(self): + result = copy.deepcopy(self.clean) + result["findings"] = ["not-a-finding"] + self.assertTrue(VALIDATOR.validate_result(result)) + + def test_malformed_pair_candidate_returns_errors(self): + packet = load(ROOT / "fixtures" / "clean-change" / "packet.json") + result = copy.deepcopy(self.clean) + result["candidate"] = None + self.assertTrue(VALIDATOR.validate_pair(packet, result)) + + +if __name__ == "__main__": + unittest.main() diff --git a/review-suite/scripts/validate.py b/review-suite/scripts/validate.py new file mode 100644 index 0000000..ac6171b --- /dev/null +++ b/review-suite/scripts/validate.py @@ -0,0 +1,269 @@ +#!/usr/bin/env python3 +"""Validate repository-owned review packets and results.""" + +from __future__ import annotations + +import argparse +import json +import re +import sys +from pathlib import Path +from typing import Any + +ROOT = Path(__file__).resolve().parents[1] +SCHEMAS = { + "packet": ROOT / "contracts" / "review-packet.schema.json", + "result": ROOT / "contracts" / "review-result.schema.json", +} + +BLOCKABLE_PACKET_ERROR_PATTERNS = ( + re.compile( + r"^\$: missing required property " + r"'(repository|candidate|change_contract|sources|validation)'$" + ), + re.compile(r"^\$\.repository: missing required property '(identity|base_branch)'$"), + re.compile( + r"^\$\.candidate: missing required property " + r"'(head_sha|comparison_base_sha|diff)'$" + ), + re.compile( + r"^\$\.candidate\.diff: missing required property " + r"'(format|complete|content)'$" + ), + re.compile(r"^\$\.candidate\.diff\.complete: expected constant True$"), + re.compile(r"^\$\.candidate\.diff\.content: string is too short$"), + re.compile( + r"^\$\.change_contract: missing required property " + r"'(goal|acceptance_criteria|non_goals|preserved_behaviors)'$" + ), + re.compile(r"^\$\.change_contract\.goal: string is too short$"), + re.compile( + r"^\$\.change_contract\.acceptance_criteria: " + r"expected at least 1 item\(s\)$" + ), + re.compile(r"^\$\.sources: missing required property "), + re.compile(r"^\$\.validation: expected at least 1 item\(s\)$"), + re.compile(r"^\$\.validation\[\d+\]: (passed|failed) requires result$"), + re.compile(r"^\$\.validation\[\d+\]: unavailable requires reason$"), + re.compile(r"^\$\.validation: missing (focused|full) validation$"), +) + + +def _path(parent: str, key: object) -> str: + if isinstance(key, int): + return f"{parent}[{key}]" + return f"{parent}.{key}" if parent else str(key) + + +def _is_type(value: Any, expected: str) -> bool: + if expected == "object": + return isinstance(value, dict) + if expected == "array": + return isinstance(value, list) + if expected == "string": + return isinstance(value, str) + if expected == "boolean": + return isinstance(value, bool) + return False + + +def validate_schema(value: Any, schema: dict[str, Any], at: str = "$") -> list[str]: + """Validate the JSON Schema subset used by this repository.""" + errors: list[str] = [] + expected_type = schema.get("type") + if expected_type and not _is_type(value, expected_type): + return [f"{at}: expected {expected_type}"] + + if "const" in schema and value != schema["const"]: + errors.append(f"{at}: expected constant {schema['const']!r}") + if "enum" in schema and value not in schema["enum"]: + errors.append(f"{at}: expected one of {schema['enum']!r}") + if isinstance(value, str): + if len(value) < schema.get("minLength", 0): + errors.append(f"{at}: string is too short") + if pattern := schema.get("pattern"): + if re.fullmatch(pattern, value) is None: + errors.append(f"{at}: does not match {pattern!r}") + if isinstance(value, list): + if len(value) < schema.get("minItems", 0): + errors.append(f"{at}: expected at least {schema['minItems']} item(s)") + if item_schema := schema.get("items"): + for index, item in enumerate(value): + errors.extend(validate_schema(item, item_schema, _path(at, index))) + if isinstance(value, dict): + properties = schema.get("properties", {}) + for key in schema.get("required", []): + if key not in value: + errors.append(f"{at}: missing required property {key!r}") + if schema.get("additionalProperties") is False: + for key in value.keys() - properties.keys(): + errors.append(f"{_path(at, key)}: unknown property") + for key, child in value.items(): + if key in properties: + errors.extend(validate_schema(child, properties[key], _path(at, key))) + return errors + + +def validate_packet(packet: dict[str, Any]) -> list[str]: + schema = json.loads(SCHEMAS["packet"].read_text()) + errors = validate_schema(packet, schema) + if errors: + return errors + + for index, validation in enumerate(packet.get("validation", [])): + status = validation.get("status") + if status in {"passed", "failed"} and not validation.get("result"): + errors.append(f"$.validation[{index}]: {status} requires result") + if status == "unavailable" and not validation.get("reason"): + errors.append(f"$.validation[{index}]: unavailable requires reason") + + scopes = {validation["scope"] for validation in packet["validation"]} + for required_scope in ("focused", "full"): + if required_scope not in scopes: + errors.append(f"$.validation: missing {required_scope} validation") + + drift = packet.get("base_drift") + if drift and drift.get("decision") == "retain": + invalidators = ( + "effective_diff_changed", + "resulting_tree_changed", + "conflict", + "relevant_overlap", + "repository_requires_reset", + ) + active = [name for name in invalidators if drift.get(name) is True] + if active: + errors.append( + "$.base_drift: retain contradicts active invalidator(s): " + + ", ".join(active) + ) + return errors + + +def validate_result(result: dict[str, Any]) -> list[str]: + schema = json.loads(SCHEMAS["result"].read_text()) + errors = validate_schema(result, schema) + if errors: + return errors + verdict = result.get("verdict") + findings = result.get("findings", []) + reasons = result.get("blocking_reasons", []) + gating = [ + finding + for finding in findings + if finding.get("severity") in {"blocking", "strong_recommendation"} + ] + + if verdict == "clean" and gating: + errors.append("$.verdict: clean contradicts gating findings") + if verdict == "changes_required" and not gating: + errors.append("$.verdict: changes_required requires a gating finding") + if verdict == "blocked" and not reasons: + errors.append("$.verdict: blocked requires at least one blocking reason") + if verdict in {"clean", "changes_required"} and reasons: + errors.append(f"$.blocking_reasons: must be empty for {verdict}") + if verdict in {"clean", "changes_required"}: + candidate = result["candidate"] + for field in ("head_sha", "comparison_base_sha"): + if field not in candidate: + errors.append(f"$.candidate: {verdict} requires {field}") + + identifiers = [finding.get("id") for finding in findings] + duplicates = sorted({item for item in identifiers if identifiers.count(item) > 1}) + if duplicates: + errors.append("$.findings: duplicate finding id(s): " + ", ".join(duplicates)) + + if result.get("lens") != "aggregate": + foreign = [ + finding.get("id", "") + for finding in findings + if finding.get("lens") != result.get("lens") + ] + if foreign: + errors.append("$.findings: lens mismatch for " + ", ".join(foreign)) + return errors + + +def is_blockable_packet_error(error: str) -> bool: + """Return whether a packet error represents absent review evidence.""" + return any(pattern.search(error) for pattern in BLOCKABLE_PACKET_ERROR_PATTERNS) + + +def validate_document(kind: str, document: dict[str, Any]) -> list[str]: + if kind == "packet": + return validate_packet(document) + return validate_result(document) + + +def validate_pair(packet: dict[str, Any], result: dict[str, Any]) -> list[str]: + packet_errors = validate_packet(packet) + result_errors = validate_result(result) + errors = [f"result: {error}" for error in result_errors] + for error in packet_errors: + if result.get("verdict") != "blocked" or not is_blockable_packet_error(error): + errors.append(f"packet: {error}") + packet_candidate = packet.get("candidate", {}) + result_candidate = result.get("candidate", {}) + if not isinstance(packet_candidate, dict) or not isinstance(result_candidate, dict): + return errors + for field in ("head_sha", "comparison_base_sha"): + packet_has_field = field in packet_candidate + result_has_field = field in result_candidate + if result_has_field and not packet_has_field: + errors.append( + f"candidate.{field}: result invents identity absent from packet" + ) + elif packet_has_field and not result_has_field: + errors.append(f"candidate.{field}: result omits identity present in packet") + elif packet_has_field and packet_candidate[field] != result_candidate[field]: + errors.append(f"candidate.{field}: result does not match packet") + return errors + + +def main() -> int: + parser = argparse.ArgumentParser() + parser.add_argument("kind", choices=["packet", "result", "pair"]) + parser.add_argument("document", type=Path) + parser.add_argument("result_document", type=Path, nargs="?") + args = parser.parse_args() + + try: + document = json.loads(args.document.read_text()) + except (OSError, json.JSONDecodeError) as error: + print(f"{args.document}: {error}", file=sys.stderr) + return 2 + if not isinstance(document, dict): + print( + f"{args.document}: top-level JSON value must be an object", file=sys.stderr + ) + return 2 + + if args.kind == "pair": + if args.result_document is None: + parser.error("pair requires a packet and result document") + try: + result_document = json.loads(args.result_document.read_text()) + except (OSError, json.JSONDecodeError) as error: + print(f"{args.result_document}: {error}", file=sys.stderr) + return 2 + if not isinstance(result_document, dict): + print( + f"{args.result_document}: top-level JSON value must be an object", + file=sys.stderr, + ) + return 2 + errors = validate_pair(document, result_document) + else: + if args.result_document is not None: + parser.error(f"{args.kind} accepts exactly one document") + errors = validate_document(args.kind, document) + if errors: + for error in errors: + print(error, file=sys.stderr) + return 1 + print(f"valid {args.kind}: {args.document}") + return 0 + + +if __name__ == "__main__": + raise SystemExit(main())