diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e949c3d..a7452f0 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -55,7 +55,7 @@ jobs: - name: Run unit tests run: | found=0 - for tests in skills/*/scripts/tests; do + for tests in skills/*/scripts/tests review-suite/scripts/tests; do if [ -d "$tests" ]; then found=1 echo "Running tests in $tests" @@ -63,5 +63,5 @@ jobs: fi done if [ "$found" -eq 0 ]; then - echo "No skill tests found under skills/*/scripts/tests" + echo "No tests found under skills/*/scripts/tests or review-suite/scripts/tests" fi diff --git a/CHANGELOG.md b/CHANGELOG.md index a2630bc..ffd9c77 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,9 +4,65 @@ summary: Chronological history of repository and skill changes. # Changelog +## 2026-07-20 — Portability, watcher resilience, and Claude adaptation + +- refactor: route the clear predicate through `has_failed_pr_checks` — the + code-simplicity lens on PR #27 flagged the last inline copy of the + failed-PR-check policy inside `is_github_candidate_clear`; all three agreement + sites now structurally share one predicate +- fix: share one failed-PR-check predicate across the watcher — the initial + `review-code-change` pass on PR #27 found the retry gate refusing retries that + `recommend_actions` recommends for failed-runs-only states; extract + `has_failed_pr_checks` and use it in both sites, and match repository case + insensitively in state-target validation + (`625dae641a9652368f03b6be825f48d9addab056`) +- fix: close the final low-severity review findings — mirror the clear predicate + in `has_failed_pr_checks` so a PR-check-backed failed run never reads as + `idle`, case-normalize repositories before deriving state files and locks, + match fragment run links, reject `--repo` without an explicit `--pr`, make + boolean schema constants reject numeric one, and document `--poll-seconds` and + `--max-flaky-retries` (`f79266a390e970cd25cf8af1bed6b9bd9cf154ee`) +- fix: align retry gating and delegation tooling with review round four — accept + cancelled-only check failures in the retry gate so a recommended retry is + never refused, grant the review orchestrator the subagent and skill tools its + Claude adapter requires, reject `--once` with `--retry-failed-now`, match + query-string run links, add a repo digest to default state filenames, keep + `diagnose_ci_failure` visible after retry exhaustion, and document zero-check + `--stop-when-clear` pairing (`ddb29d0ce0409554cec61ed54b2c6e7ed6d84c6a`) +- fix: close adversarial-review findings — resolve bundled-validator schemas in + both layouts and execute every bundled copy in place, scope failed workflow + runs to the PR's own checks so push/schedule failures cannot wedge the + watcher, emit `resolve_draft_state`/`resolve_merge_conflict` instead of + `idle`, complete `forbidden_actions` on all forward expectations with a + vocabulary-spam canary, stop backfilling `target_skill` in the Claude + executor, reject `--once --watch`, handle `OSError` cleanly, import bundled + validators in review-skill tests, and document eval flag pre-classification, + the gh 2.37 floor, and state-file durability + (`48b6f614d15d50dae4ba5c63d7b3e3471647dd1a`) +- fix: close independent-review findings — count cancelled checks and failed + runs/jobs in the watcher's clear predicate, run review-suite tests in CI, + bundle the dependency-free packet validator into each review skill, make + `--stop-when-clear` imply `ready_to_merge` and test every documented CLI + invocation, fail closed on empty `gh pr checks` payloads, surface ghost-author + comments, move watcher state into a per-user 0700 directory, add + forbidden-action forward grading, unify `observed_sequence` tokens, and rename + `agents/claude.md` to `agents/claude-code.md` to avoid the case-insensitive + CLAUDE.md memory-file collision (`b5bf81b81a6dd521edcdfc561988ca621a566d39`) +- fix: make skills self-contained and adapt the suite to Claude runtimes — + bundle the review-suite contract into each review skill with a + `just sync-contracts` target and drift test, use skill-root-relative watcher + paths, survive transient watcher failures with bounded backoff, add + `--max-polls`/`--stop-when-clear` bounded watch modes and a + `confirm_feedback_disposition` action, move eval answer keys out of + reviewer-visible input directories, add a Claude headless forward-eval + executor, add `agents/claude.md` adapters, `allowed-tools` on review skills, + and trigger-oriented skill descriptions, and trim contract tests to + load-bearing invariants (`474756bea51237376b81ad7d593eef2d8de273f1`) + ## 2026-07-20 — Composed ticket and PR execution - fix: execute result-blind forward evaluations in fresh contexts + (`f452db4cf47e56b3f8fea560977a3ce98ca26caa`) - feat: delegate the `implement-ticket` PR lifecycle to `babysit-pr` (`d5838d49587ab34a00973441a870cd525cfcd773`) diff --git a/CLAUDE.md b/CLAUDE.md new file mode 100644 index 0000000..af7e95f --- /dev/null +++ b/CLAUDE.md @@ -0,0 +1,5 @@ +# Claude Code Guidelines + +See @AGENTS.md for the repository working model, required checks, skill +conventions, safety rules, and git workflow. Both files apply equally; keep +repository guidance in `AGENTS.md` rather than duplicating it here. diff --git a/README.md b/README.md index 3622369..929a1b0 100644 --- a/README.md +++ b/README.md @@ -44,8 +44,19 @@ implement-epic ``` Compatible runtimes may provide named subagents or equivalent isolated -implementation and review contexts. OpenAI-facing files under `agents/` are -optional discovery metadata, not part of the skills' portable contracts. +implementation and review contexts. Files under each skill's `agents/` directory +(`openai.yaml` for OpenAI runtimes, `claude-code.md` for Claude Code) are +optional discovery and adapter metadata, not part of the skills' portable +contracts. + +Each review skill bundles a verbatim copy of the canonical `review-suite` +contract and schemas under its own `references/review-suite/` directory so the +skill remains self-contained when installed elsewhere. Edit only the canonical +files and refresh the copies with: + +```bash +just sync-contracts +``` ## Quick Start @@ -79,18 +90,26 @@ just eval-prepare-changesets just eval-implement-ticket ``` -The ticket-composition evaluator starts a fresh process for each raw-artifact -case, with fixture identity and grader expectations withheld. Its bundled -reference executor validates the portable skill contract deterministically. To -forward-evaluate another compatible agent runtime, pass its stdin/stdout JSON +The ticket-composition evaluator starts a fresh process for each case, with +fixture identity and grader expectations withheld. Case artifacts carry +pre-classified scenario flags (for example, a CI failure already labeled +branch-caused or infrastructure), so the harness grades obligation mapping and +terminal-state selection — not evidence classification itself. Its bundled +reference executor is a deterministic simulation of a compliant runtime, not a +model. To forward-evaluate a real agent runtime, pass its stdin/stdout JSON adapter through `scripts/evals/run_forward.py --executor` and retain captured -observations with `--output-dir`. +observations with `--output-dir`. A Claude Code headless adapter is bundled: + +```bash +just eval-implement-ticket-claude +``` ## Prerequisites - Python 3.11+ - Git -- GitHub CLI (`gh`) for PR workflows +- GitHub CLI (`gh`) 2.37 or newer for PR workflows (the babysit-pr watcher + requires `gh pr checks --json`) - `skills-ref` on `PATH` for skill validation (optional but recommended) ## Notes diff --git a/justfile b/justfile index 1818e2e..5bab059 100644 --- a/justfile +++ b/justfile @@ -8,6 +8,19 @@ md_targets := "." list-skills: @find {{skills_dir}} -mindepth 1 -maxdepth 1 -type d -print +# Refresh the review-suite contract copies bundled into each review skill so +# the skills stay self-contained when installed outside this repository. +sync-contracts: + @for skill in review-code-change review-correctness review-code-simplicity review-solution-simplicity; do \ + dest="{{skills_dir}}/$skill/references/review-suite"; \ + mkdir -p "$dest"; \ + cp review-suite/CONTRACT.md "$dest/CONTRACT.md"; \ + cp review-suite/contracts/review-packet.schema.json "$dest/review-packet.schema.json"; \ + cp review-suite/contracts/review-result.schema.json "$dest/review-result.schema.json"; \ + cp review-suite/scripts/validate.py "$dest/validate.py"; \ + echo "Synced $dest"; \ + done + test: @found=0; \ for tests in {{skills_dir}}/*/scripts/tests; do \ @@ -37,6 +50,11 @@ test-implement-ticket: eval-implement-ticket: python3 {{skills_dir}}/implement-ticket/scripts/evals/run_forward.py +# Real-runtime forward evaluation; requires the `claude` CLI on PATH. +eval-implement-ticket-claude: + python3 {{skills_dir}}/implement-ticket/scripts/evals/run_forward.py \ + --executor "python3 {{skills_dir}}/implement-ticket/scripts/evals/claude_executor.py" + test-implement-epic: python3 -m unittest discover -s {{skills_dir}}/implement-epic/scripts/tests -p 'test_*.py' @@ -97,7 +115,7 @@ lint-skills: AGENTSKILLS_DIR=".tools/agentskills"; \ SKILLS_REF_BIN=""; \ install_skills_ref() { \ - echo "Installing skills-ref into .venv from local cache..."; \ + echo "Installing skills-ref: recreating .venv and cloning agentskills from GitHub (network required)..."; \ rm -rf .venv; \ python -m venv .venv; \ mkdir -p .tools; \ diff --git a/review-suite/CONTRACT.md b/review-suite/CONTRACT.md index f613af6..412a672 100644 --- a/review-suite/CONTRACT.md +++ b/review-suite/CONTRACT.md @@ -15,6 +15,17 @@ silently redefine them. - `fixtures/` contains raw review inputs and separate expected material outcomes for deterministic tests and independent forward tests. +Because a skill folder is the unit of distribution, each review skill bundles a +verbatim copy of this document, both schemas, and the dependency-free +`validate.py` under its own `references/review-suite/` directory so the skill +works — including packet and result validation — when installed outside this +repository. The copies are mechanical mirrors, not forks: edit only the +canonical files here, refresh the copies with `just sync-contracts`, and rely on +the bundled-contract test to fail on any drift. References in this document to +`scripts/validate.py` and `fixtures/` describe this canonical directory; in an +installed skill, use the bundled `validate.py` beside this file, and expect no +fixtures. + ## Review packet A review packet binds the review to one candidate and states what that candidate diff --git a/review-suite/scripts/tests/test_bundled_contracts.py b/review-suite/scripts/tests/test_bundled_contracts.py new file mode 100644 index 0000000..4d6555c --- /dev/null +++ b/review-suite/scripts/tests/test_bundled_contracts.py @@ -0,0 +1,92 @@ +"""Verify skill-bundled review-suite contract copies match the canonical source. + +Each review skill bundles the canonical contract and schemas under +`references/review-suite/` so the skill remains self-contained when installed +outside this repository. `just sync-contracts` refreshes the copies; this test +fails when any copy drifts from the canonical file. +""" + +from __future__ import annotations + +import subprocess +import sys +import unittest +from pathlib import Path + +REPOSITORY_ROOT = Path(__file__).resolve().parents[3] +REVIEW_SUITE = REPOSITORY_ROOT / "review-suite" +BUNDLING_SKILLS = ( + "review-code-change", + "review-correctness", + "review-code-simplicity", + "review-solution-simplicity", +) +CANONICAL_FILES = { + "CONTRACT.md": REVIEW_SUITE / "CONTRACT.md", + "review-packet.schema.json": REVIEW_SUITE + / "contracts" + / "review-packet.schema.json", + "review-result.schema.json": REVIEW_SUITE + / "contracts" + / "review-result.schema.json", + "validate.py": REVIEW_SUITE / "scripts" / "validate.py", +} + + +class BundledContractTests(unittest.TestCase): + def test_every_review_skill_bundles_identical_contract_copies(self): + for skill in BUNDLING_SKILLS: + bundle = REPOSITORY_ROOT / "skills" / skill / "references" / "review-suite" + for name, canonical in CANONICAL_FILES.items(): + bundled = bundle / name + with self.subTest(skill=skill, file=name): + self.assertTrue( + bundled.exists(), + f"{bundled} is missing; run `just sync-contracts`", + ) + self.assertEqual( + canonical.read_bytes(), + bundled.read_bytes(), + f"{bundled} drifted from {canonical}; " + "run `just sync-contracts`", + ) + + def test_every_bundled_validator_executes_in_its_installed_layout(self): + """The bundled validate.py must run from references/review-suite/. + + Byte-identity alone once shipped a copy that resolved its schemas + against the canonical directory layout and crashed everywhere else; + execute each copy in place against a known-good fixture pair. + """ + fixture = REVIEW_SUITE / "fixtures" / "repository-convention-clean" + for skill in BUNDLING_SKILLS: + bundled = ( + REPOSITORY_ROOT + / "skills" + / skill + / "references" + / "review-suite" + / "validate.py" + ) + with self.subTest(skill=skill): + completed = subprocess.run( + [ + sys.executable, + str(bundled), + "pair", + str(fixture / "packet.json"), + str(fixture / "expected.json"), + ], + capture_output=True, + text=True, + check=False, + ) + self.assertEqual( + 0, + completed.returncode, + f"{bundled} failed: {completed.stderr.strip()}", + ) + + +if __name__ == "__main__": + unittest.main() diff --git a/review-suite/scripts/tests/test_contracts.py b/review-suite/scripts/tests/test_contracts.py index 958381a..77198bf 100644 --- a/review-suite/scripts/tests/test_contracts.py +++ b/review-suite/scripts/tests/test_contracts.py @@ -82,6 +82,15 @@ def test_unknown_enum_is_rejected(self): self.packet["validation"][0]["status"] = "skipped" self.assertTrue(VALIDATOR.validate_packet(self.packet)) + def test_boolean_const_rejects_numeric_one(self): + # Python's `1 == True` must not let numeric values satisfy + # `"const": true`. + self.packet["candidate"]["diff"]["complete"] = 1 + self.assertIn( + "$.candidate.diff.complete: expected constant True", + VALIDATOR.validate_packet(self.packet), + ) + def test_unavailable_validation_requires_reason(self): self.packet["validation"][0] = { "name": "tests", diff --git a/review-suite/scripts/validate.py b/review-suite/scripts/validate.py index 76ec236..50e0b12 100644 --- a/review-suite/scripts/validate.py +++ b/review-suite/scripts/validate.py @@ -10,10 +10,27 @@ from pathlib import Path from typing import Any -ROOT = Path(__file__).resolve().parents[1] +HERE = Path(__file__).resolve().parent + + +def _schema_file(name: str) -> Path: + """Locate a schema in either supported layout. + + Canonical layout: review-suite/scripts/validate.py with schemas under + review-suite/contracts/. Bundled layout (installed review skills): + references/review-suite/validate.py with the schemas beside it. + """ + for candidate in (HERE / name, HERE.parent / "contracts" / name): + if candidate.is_file(): + return candidate + raise FileNotFoundError( + f"Cannot locate {name} beside {HERE} or in {HERE.parent / 'contracts'}" + ) + + SCHEMAS = { - "packet": ROOT / "contracts" / "review-packet.schema.json", - "result": ROOT / "contracts" / "review-result.schema.json", + "packet": _schema_file("review-packet.schema.json"), + "result": _schema_file("review-result.schema.json"), } BLOCKABLE_PACKET_ERROR_PATTERNS = ( @@ -74,8 +91,11 @@ def validate_schema(value: Any, schema: dict[str, Any], at: str = "$") -> list[s 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 "const" in schema: + const = schema["const"] + # `1 == True` in Python; a boolean constant must reject numeric 1/1.0. + if value != const or isinstance(const, bool) != isinstance(value, bool): + errors.append(f"{at}: expected constant {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): diff --git a/skills/babysit-pr/SKILL.md b/skills/babysit-pr/SKILL.md index d3c7084..d63d3d5 100644 --- a/skills/babysit-pr/SKILL.md +++ b/skills/babysit-pr/SKILL.md @@ -1,6 +1,6 @@ --- name: babysit-pr -description: Monitor an existing GitHub pull request through current-head CI, review feedback, mergeability, and optional merge. Use when an agent must watch a PR until it is ready to merge, merge it when explicitly authorized, or keep watching until it closes; diagnose failures, make only authorized ticket-scoped fixes, rerun validation and repository-owned review after head changes, and return a candidate-bound terminal handoff. +description: Watch, monitor, or babysit an existing GitHub pull request through current-head CI, review feedback, mergeability, and optional merge. Use when asked to watch a PR until it is ready to merge, merge it when explicitly authorized, shepherd it through failing checks and review comments, or keep watching until it closes; diagnoses failures, makes only authorized ticket-scoped fixes, reruns validation and repository-owned review after head changes, and returns a candidate-bound terminal handoff. --- # Babysit PR @@ -23,7 +23,9 @@ tracker, close a parent, deploy, or delete branches and worktrees. watcher or evaluating a new upstream version. Use `scripts/gh_pr_watch.py` for deterministic snapshots, JSONL monitoring, and -bounded failed-run retries. Treat its actions as recommendations, not proof that +bounded failed-run retries. All watcher paths below are relative to this skill's +root directory; resolve them against the installed skill location, not the +repository checkout. Treat its actions as recommendations, not proof that repository-specific gates passed. ## Require compatible capabilities @@ -109,25 +111,43 @@ than continuing from cached state. ## Start and own the watcher -Run a snapshot first: +Run a snapshot first (paths are relative to this skill's root): ```bash -python3 skills/babysit-pr/scripts/gh_pr_watch.py --pr --once +python3 scripts/gh_pr_watch.py --pr --once ``` For persistent monitoring, run: ```bash -python3 skills/babysit-pr/scripts/gh_pr_watch.py \ +python3 scripts/gh_pr_watch.py \ --pr \ --completion-policy \ --watch ``` -Keep consuming JSONL output in the controlling task. Do not detach the watcher -and claim monitoring is complete. Run only one continuous watcher for one -repository/PR state file. After pausing to change or push code, restart the -watcher on the new live candidate without waiting for another user request. +Always pass an explicit PR number or URL; do not rely on `auto` resolution in a +repository with multiple open PRs. + +Keep consuming JSONL output in the controlling task, using whichever execution +mode the runtime supports: + +- A runtime that can hold a long-running foreground process may stream `--watch` + output directly. +- A runtime with bounded foreground command windows (for example, a shell tool + with a timeout) must either run `--watch` as a managed background task and + read its incremental JSONL output between other work, or run bounded + foreground windows with `--watch --max-polls ` or + `--watch --stop-when-clear` and re-invoke until a terminal condition is + reached. `--stop-when-clear` asserts only GitHub-native gates; every + repository-specific gate and feedback disposition must still be verified. + +Do not detach the watcher and claim monitoring is complete. Run only one +continuous watcher for one repository/PR state file; every mode, including +`--once` and `--retry-failed-now`, takes the same exclusive state lock, so stop +the running watcher before a snapshot or retry and restart it afterward. After +pausing to change or push code, restart the watcher on the new live candidate +without waiting for another user request. The watcher reports all published feedback sources, unresolved threads, candidate changes, CI state, failed-job log endpoints, retry usage, @@ -174,10 +194,11 @@ Follow [CI and feedback decisions](references/ci-and-feedback.md). - Never change tests, CI, dependencies, or infrastructure merely to hide a flaky or unrelated failure. - Use the retry command only after log-based classification and only within the - configured budget: + configured budget. Stop the continuous watcher first; retry shares its + exclusive state lock: ```bash -python3 skills/babysit-pr/scripts/gh_pr_watch.py \ +python3 scripts/gh_pr_watch.py \ --pr \ --retry-failed-now \ --eligible-run-id @@ -266,6 +287,23 @@ Include repository, PR, head, base, branch/worktree, policy, authority used, validation, repository-owned review, CI, retry, human/connector/comment/review/ thread state, fixes and pushed heads, mergeability, merged/closed identity, deferred findings, mutation ownership, caller-owned follow-up, and one next -action or blocker. +action or blocker. For example: + +```text +terminal_state: ready_to_merge +repository: example/project pr: #482 +head: 4f2c…9a1d (branch fix/issue-77, worktree ../wt-issue-77) +base: main @ 7be0…44c2 +completion_policy: ready_to_merge authority_used: read + ticket-scoped fix + push +validation: `just test` pass @ head; full gate pass @ head +repository_review: review-code-change clean @ head 4f2c…9a1d vs base 7be0…44c2 +ci: 6/6 checks pass @ head retries_used: 1/3 (run 8123, infrastructure) +feedback: 3 comments dispositioned (2 fixed, 1 rejected with evidence); + 0 unresolved threads; human review approved @ head; connector clean @ head +fixes_pushed: 1 (commit 4f2c…9a1d, review-requested null check) +mergeability: MERGEABLE / CLEAN mutation_ownership: this task, released +caller_owned_follow_up: merge, tracker transition, branch/worktree cleanup +next_action: caller may merge via repository-approved squash method +``` Under `watch_until_closed`, a ready snapshot is progress rather than terminal. diff --git a/skills/babysit-pr/agents/claude-code.md b/skills/babysit-pr/agents/claude-code.md new file mode 100644 index 0000000..7f1b1f3 --- /dev/null +++ b/skills/babysit-pr/agents/claude-code.md @@ -0,0 +1,19 @@ +# Claude Code adapter + +Optional discovery metadata for Claude Code and Claude Agent SDK runtimes. It +does not constrain the skill's portable contract. + +- Display name: Babysit PR. +- Suggested prompt: "Use the babysit-pr skill to monitor this pull request until + it reaches the requested completion policy." +- Long waits: Claude Code bounds foreground shell commands, so run + `scripts/gh_pr_watch.py --watch` as a background shell task and read its + incremental JSONL output between other work, or run bounded foreground windows + with `--watch --max-polls ` or `--watch --stop-when-clear` and re-invoke + until terminal. Retain ownership of the monitoring task either way; never + detach and declare monitoring complete. +- Locking: stop the background watcher before `--once` or `--retry-failed-now`; + all modes share one exclusive state lock per repository/PR. +- Fresh read-only review context: invoke repository-owned `review-code-change` + in a subagent (Agent tool) restricted to read-only tools, giving it only raw + candidate evidence. diff --git a/skills/babysit-pr/evals/README.md b/skills/babysit-pr/evals/README.md new file mode 100644 index 0000000..6f5dbb4 --- /dev/null +++ b/skills/babysit-pr/evals/README.md @@ -0,0 +1,8 @@ +# Babysit-pr evaluations + +`cases.json` describes scenario inputs and `expectations.json` records the +required terminal state and actions for each case. There is no automated runner +yet; the pair is consumed by `scripts/tests/test_skill_contract.py` as contract +data and can be replayed manually or through a compatible headless agent +harness. Give an evaluated agent only `cases.json` entries; never show it +`expectations.json`. diff --git a/skills/babysit-pr/evals/results.json b/skills/babysit-pr/evals/expectations.json similarity index 100% rename from skills/babysit-pr/evals/results.json rename to skills/babysit-pr/evals/expectations.json diff --git a/skills/babysit-pr/references/github.md b/skills/babysit-pr/references/github.md index 6450de6..53ed0c9 100644 --- a/skills/babysit-pr/references/github.md +++ b/skills/babysit-pr/references/github.md @@ -16,6 +16,8 @@ owns the ticket. - Confirm `gh` authentication and repository access. - Resolve the PR from an explicit number/URL or an unambiguous current branch. + Prefer passing the explicit number or URL to every watcher invocation; `auto` + resolution is a convenience for single-PR checkouts only. - Capture repository, number, URL, state, head repository/branch/SHA, base branch/SHA, mergeability, merge-state status, and review decision. - Inspect open/merged PRs and plausible branches for a superseding candidate. @@ -29,43 +31,74 @@ dependency, and close mutations to the caller. ## Watcher commands -Use a one-shot snapshot for diagnosis: +Paths are relative to this skill's root directory. Use a one-shot snapshot for +diagnosis: ```bash -python3 skills/babysit-pr/scripts/gh_pr_watch.py \ +python3 scripts/gh_pr_watch.py \ --repo OWNER/REPO --pr NUMBER --once ``` Use JSONL monitoring for a persistent task: ```bash -python3 skills/babysit-pr/scripts/gh_pr_watch.py \ +python3 scripts/gh_pr_watch.py \ --repo OWNER/REPO --pr NUMBER \ --completion-policy ready_to_merge --watch ``` -Use `--state-file` only when the caller needs a controlled durable location. The -default state is isolated by repository and PR in the operating system's -temporary directory. A state file whose stored repository/PR differs from the -live target fails closed. +For runtimes with bounded foreground command windows, either run `--watch` as a +managed background task and read its incremental JSONL output, or bound each +foreground window with `--watch --max-polls ` or `--watch --stop-when-clear` +and re-invoke until a terminal condition is reached. `--stop-when-clear` exits +when GitHub-native gates are clear; it never asserts repository-specific gates +or feedback disposition. When no completion policy is passed it implies +`ready_to_merge`; combining it with an explicit `watch_until_closed` is +rejected. On a repository with zero configured checks the candidate is never +GitHub-clear (the watcher emits `verify_required_check_policy` instead), so pair +`--stop-when-clear` with `--max-polls` to keep the window bounded. + +Watch mode polls every `--poll-seconds` (default 30) and tolerates a bounded +number of consecutive transient GitHub CLI failures (`--max-transient-failures`, +default 5), emitting a `transient_error` event with backoff before retrying; it +still exits nonzero when the budget is exhausted and immediately on identity +failures. The per-head flaky retry budget is `--max-flaky-retries` (default 3). + +Use `--state-file` when the caller needs a controlled durable location. The +default state is isolated by repository and PR in a per-user mode-0700 directory +under the operating system's temporary directory; a reboot or temp-file cleaner +may reset it, which re-surfaces already-seen feedback (safe) and resets retry +budgets (permits extra reruns). Pass `--state-file` on a durable path when retry +budgets must survive reboots. A state file whose stored repository/PR differs +from the live target fails closed. All modes share a nonblocking lock on their repository/PR state file, including one-shot snapshots, continuous watch, and retry mutation. Do not run a second -controller for the same state. The controlling task must consume watch output -and terminate the process when interrupted; never leave a detached watcher. +controller for the same state; stop the continuous watcher before running +`--once` or `--retry-failed-now`, then restart it. The controlling task must +consume watch output and terminate the process when interrupted; never leave a +detached watcher. The watcher emits: - exact PR/head/base identity and candidate-change flags; -- check counts and per-check metadata; -- workflow failures and direct failed-job log endpoints; +- check counts (including cancelled checks) and per-check metadata; +- failed workflow runs and direct failed-job log endpoints, scoped to the runs + backing the PR's own checks; failed head-SHA workflows that are not PR checks + (push- or schedule-triggered) appear separately under + `non_pr_check_failed_runs` and never gate readiness or retries; +- explicit `resolve_draft_state` and `resolve_merge_conflict` actions for draft + and conflicting PRs; - all published feedback, new feedback, and unresolved threads; - retry count and remaining budget; - mergeability/review state; and - ordered recommended actions. Recommendations never establish repository-specific review, connector, or -local-validation success. +local-validation success. In particular, `verify_external_gates` asserts only +GitHub-native gates; when any published feedback exists the watcher also emits +`confirm_feedback_disposition` because it can deduplicate conversation comments +but cannot verify that the controller dispositioned them. ## State and candidate changes diff --git a/skills/babysit-pr/scripts/gh_pr_watch.py b/skills/babysit-pr/scripts/gh_pr_watch.py index aebe187..f14217b 100644 --- a/skills/babysit-pr/scripts/gh_pr_watch.py +++ b/skills/babysit-pr/scripts/gh_pr_watch.py @@ -7,6 +7,7 @@ """ import argparse +import hashlib import json import os import re @@ -69,11 +70,45 @@ def parse_args(): "trigger flaky reruns." ) ) - parser.add_argument("--pr", default="auto", help="auto, PR number, or PR URL") + parser.add_argument( + "--pr", + default="auto", + help=( + "PR number or PR URL. `auto` resolves from the current branch; " + "prefer an explicit number or URL in multi-PR repositories." + ), + ) parser.add_argument("--repo", help="Optional OWNER/REPO override") parser.add_argument( "--poll-seconds", type=int, default=30, help="Watch poll interval" ) + parser.add_argument( + "--max-transient-failures", + type=int, + default=5, + help=( + "Consecutive transient GitHub CLI failures tolerated in --watch " + "mode before exiting nonzero" + ), + ) + parser.add_argument( + "--max-polls", + type=int, + default=0, + help=( + "Exit --watch after this many successful snapshots (0 = unlimited). " + "Use for bounded foreground execution windows." + ), + ) + parser.add_argument( + "--stop-when-clear", + action="store_true", + help=( + "Exit --watch once GitHub-native gates are clear " + "(the `verify_external_gates` action). The controller must still " + "verify repository-specific gates and feedback disposition." + ), + ) parser.add_argument( "--max-flaky-retries", type=int, @@ -84,8 +119,12 @@ def parse_args(): parser.add_argument( "--completion-policy", choices=sorted(COMPLETION_POLICIES), - default="watch_until_closed", - help="Controller-selected terminal policy (reported but not enforced alone)", + default=None, + help=( + "Controller-selected terminal policy (reported but not enforced " + "alone). Defaults to watch_until_closed, or ready_to_merge when " + "--stop-when-clear is given." + ), ) parser.add_argument( "--once", action="store_true", help="Emit one snapshot and exit" @@ -111,8 +150,29 @@ def parse_args(): parser.error("--poll-seconds must be > 0") if args.max_flaky_retries < 0: parser.error("--max-flaky-retries must be >= 0") + if args.max_transient_failures < 0: + parser.error("--max-transient-failures must be >= 0") + if args.max_polls < 0: + parser.error("--max-polls must be >= 0") + if (args.max_polls or args.stop_when_clear) and not args.watch: + parser.error("--max-polls and --stop-when-clear require --watch") + if args.stop_when_clear and args.completion_policy == "watch_until_closed": + parser.error( + "--stop-when-clear conflicts with an explicit watch_until_closed; " + "a ready snapshot is progress, not terminal, under that policy" + ) + if args.completion_policy is None: + args.completion_policy = ( + "ready_to_merge" if args.stop_when_clear else "watch_until_closed" + ) if args.watch and args.retry_failed_now: parser.error("--watch cannot be combined with --retry-failed-now") + if args.once and args.watch: + parser.error("--once cannot be combined with --watch") + if args.once and args.retry_failed_now: + parser.error("--once cannot be combined with --retry-failed-now") + if args.repo and args.pr == "auto": + parser.error("--repo requires an explicit --pr number or URL") if args.eligible_run_id and not args.retry_failed_now: parser.error("--eligible-run-id requires --retry-failed-now") if args.retry_failed_now and not args.eligible_run_id: @@ -133,7 +193,8 @@ def _format_gh_error(cmd, err): return "\n".join(parts) -def gh_text(args, repo=None, allowed_returncodes=()): +def gh_capture(args, repo=None, allowed_returncodes=()): + """Run gh and return (stdout, stderr), tolerating allowed exit codes.""" cmd = ["gh"] # `gh api` does not accept `-R/--repo` on all gh versions. The watcher's # API calls use explicit endpoints (e.g. repos/{owner}/{repo}/...), so the @@ -147,9 +208,14 @@ def gh_text(args, repo=None, allowed_returncodes=()): raise GhCommandError("`gh` command not found") from err except subprocess.CalledProcessError as err: if err.returncode in allowed_returncodes: - return err.stdout or "" + return err.stdout or "", err.stderr or "" raise GhCommandError(_format_gh_error(cmd, err)) from err - return proc.stdout + return proc.stdout, proc.stderr or "" + + +def gh_text(args, repo=None, allowed_returncodes=()): + stdout, _ = gh_capture(args, repo=repo, allowed_returncodes=allowed_returncodes) + return stdout def gh_json(args, repo=None, allowed_returncodes=()): @@ -304,10 +370,32 @@ def save_state(path, state): raise +def default_state_directory(): + """Per-user, mode-0700 state directory under the system temp directory. + + State content is trusted (seen-feedback IDs, retry budgets), so on shared + multi-user hosts it must not live at a predictable world-writable path. + chmod fails closed when another user pre-created the directory. + """ + getuid = getattr(os, "getuid", None) + owner = str(getuid()) if getuid else "user" + directory = Path(tempfile.gettempdir()) / f"agent-babysit-pr-{owner}" + directory.mkdir(mode=0o700, exist_ok=True) + os.chmod(directory, 0o700) + return directory + + def default_state_file_for(pr): - repo_slug = re.sub(r"[^A-Za-z0-9_.-]+", "-", pr["repo"]).strip("-") - return Path(tempfile.gettempdir()) / ( - f"agent-babysit-pr-{repo_slug}-pr{pr['number']}.json" + # The slug keeps the filename readable; the digest of the exact + # repository string guarantees distinct repositories can never collide + # on one state file (e.g. `a-b/c` vs `a/b-c`). GitHub slugs are + # case-insensitive, so normalize case first: `--repo Owner/Repo` and a + # URL-derived `owner/repo` must share one state file and one lock. + repo = pr["repo"].lower() + repo_slug = re.sub(r"[^a-z0-9_.-]+", "-", repo).strip("-") + digest = hashlib.sha256(repo.encode("utf-8")).hexdigest()[:8] + return default_state_directory() / ( + f"agent-babysit-pr-{repo_slug}-{digest}-pr{pr['number']}.json" ) @@ -321,7 +409,9 @@ def validate_state_target(state, pr, state_path): number_matches = int(stored_number) == int(pr["number"]) except (TypeError, ValueError): number_matches = False - if stored_repo != pr["repo"] or not number_matches: + # GitHub slugs are case-insensitive; match the normalization used by + # default_state_file_for so mixed-case invocations share one state file. + if stored_repo.lower() != pr["repo"].lower() or not number_matches: raise RuntimeError( "State file target does not match live PR: " f"{state_path} stores {stored_repo}#{stored_number}, " @@ -332,7 +422,10 @@ def validate_state_target(state, pr, state_path): @contextmanager def watcher_lock(state_path): if fcntl is None: - raise RuntimeError("Continuous watch requires file-lock support") + raise RuntimeError( + "This watcher requires POSIX file-lock support (fcntl); every " + "mode, including --once, shares the repository/PR state lock" + ) lock_path = state_path.with_suffix(state_path.suffix + ".lock") lock_path.parent.mkdir(parents=True, exist_ok=True) with lock_path.open("a+", encoding="utf-8") as lock_file: @@ -354,9 +447,23 @@ def get_pr_checks(pr_spec, repo): if parsed["value"] is not None: cmd.append(parsed["value"]) cmd.extend(["--json", checks_fields()]) - data = gh_json(cmd, repo=repo, allowed_returncodes=(1, 8)) - if data is None: - return [] + # gh exits 1 when checks failed and 8 when checks are pending, still + # emitting JSON. An empty payload on those codes is a real error unless + # gh explicitly reports that the PR has no checks; otherwise treating it + # as zero checks would mask a failure as a benign policy state. + stdout, stderr = gh_capture(cmd, repo=repo, allowed_returncodes=(1, 8)) + raw = stdout.strip() + if not raw: + if "no checks reported" in stderr.lower(): + return [] + raise GhCommandError( + "`gh pr checks` returned no JSON payload; check state is unknown" + + (f"\nstderr: {stderr.strip()}" if stderr.strip() else "") + ) + try: + data = json.loads(raw) + except json.JSONDecodeError as err: + raise GhCommandError("Failed to parse JSON from `gh pr checks`") from err if not isinstance(data, list): raise GhCommandError("Unexpected payload from `gh pr checks`") return data @@ -372,6 +479,7 @@ def summarize_checks(checks): pending_count = 0 failed_count = 0 passed_count = 0 + cancelled_count = 0 for check in checks: bucket = str(check.get("bucket") or "").lower() if is_pending_check(check): @@ -380,11 +488,16 @@ def summarize_checks(checks): failed_count += 1 if bucket == "pass": passed_count += 1 + # gh buckets cancelled checks as `cancel`, not `fail`; a cancelled + # required check must never read as clean. + if bucket == "cancel": + cancelled_count += 1 return { "total_count": len(checks), "pending_count": pending_count, "failed_count": failed_count, "passed_count": passed_count, + "cancelled_count": cancelled_count, "all_terminal": pending_count == 0, "items": checks, } @@ -395,7 +508,11 @@ def workflow_run_ids_from_checks(checks): for check in checks: if not isinstance(check, dict): continue - match = re.search(r"/actions/runs/(\d+)(?:/|$)", str(check.get("link") or "")) + # Details links take several shapes: .../runs/123, .../runs/123/job/456, + # the legacy .../runs/123?check_suite_focus=true, and fragments. + match = re.search( + r"/actions/runs/(\d+)(?:[/?#]|$)", str(check.get("link") or "") + ) if match: run_ids.add(int(match.group(1))) return run_ids @@ -439,6 +556,18 @@ def failed_runs_from_workflow_runs(runs, head_sha): return failed_runs +def partition_runs_by_pr_checks(failed_runs, pr_check_run_ids): + """Split failed runs into PR-check-backed runs and other head workflows.""" + pr_check_runs = [] + other_runs = [] + for run in failed_runs: + if run.get("run_id") in pr_check_run_ids: + pr_check_runs.append(run) + else: + other_runs.append(run) + return pr_check_runs, other_runs + + def get_jobs_for_run(repo, run_id): endpoint = f"repos/{repo}/actions/runs/{run_id}/jobs" return gh_api_object_list_paginated(endpoint, "jobs", repo=repo) @@ -496,13 +625,20 @@ def failed_jobs_from_workflow_runs(repo, runs, head_sha): return failed_jobs +_authenticated_login_cache = None + + def get_authenticated_login(): + global _authenticated_login_cache + if _authenticated_login_cache is not None: + return _authenticated_login_cache data = gh_json(["api", "user"]) if not isinstance(data, dict) or not data.get("login"): raise GhCommandError( "Unable to determine authenticated GitHub login from `gh api user`" ) - return str(data["login"]) + _authenticated_login_cache = str(data["login"]) + return _authenticated_login_cache def comment_endpoints(repo, pr_number): @@ -836,7 +972,9 @@ def fetch_review_state(pr, state, authenticated_login=None): new_items = [] for item in all_items: item_id = item.get("id") - if not item_id or not item.get("author"): + # Keep items with a deleted ("ghost") author: they still carry + # published feedback and must surface as new exactly once. + if not item_id: continue kind = item["kind"] seen = { @@ -891,9 +1029,26 @@ def unique_actions(actions): return out +def has_failed_pr_checks(checks_summary, failed_runs, failed_jobs): + """One shared failed-PR-check predicate. + + recommend_actions, is_github_candidate_clear, and the retry gate must + all agree on what counts as a failed PR check; two diverging copies of + this test have twice produced recommend/refuse contradictions. + """ + return ( + checks_summary["failed_count"] > 0 + or int(checks_summary.get("cancelled_count") or 0) > 0 + or bool(failed_runs) + or bool(failed_jobs) + ) + + def is_github_candidate_clear( pr, checks_summary, + failed_runs, + failed_jobs, new_review_items, unresolved_threads, ): @@ -907,7 +1062,9 @@ def is_github_candidate_clear( return False if not checks_summary["all_terminal"]: return False - if checks_summary["failed_count"] > 0 or checks_summary["pending_count"] > 0: + if checks_summary["pending_count"] > 0: + return False + if has_failed_pr_checks(checks_summary, failed_runs, failed_jobs): return False if new_review_items: return False @@ -932,10 +1089,11 @@ def recommend_actions( candidate_changed, retries_used, max_retries, + has_published_feedback=False, ): actions = [] if pr["closed"] or pr["merged"]: - if new_review_items: + if new_review_items or unresolved_threads: actions.append("process_review_feedback") actions.append("stop_pr_closed") return unique_actions(actions) @@ -943,12 +1101,24 @@ def recommend_actions( if candidate_changed: actions.append("rebuild_candidate_evidence") + # Draft and conflicting states are the ones that most need controller + # action; never let them degenerate to a bare `idle`. + if pr.get("draft"): + actions.append("resolve_draft_state") + if ( + str(pr.get("mergeable") or "") == "CONFLICTING" + or str(pr.get("merge_state_status") or "") == "DIRTY" + ): + actions.append("resolve_merge_conflict") + if new_review_items or unresolved_threads: actions.append("process_review_feedback") - has_failed_pr_checks = checks_summary["failed_count"] > 0 or bool(failed_jobs) - if has_failed_pr_checks: + if has_failed_pr_checks(checks_summary, failed_runs, failed_jobs): if checks_summary["all_terminal"] and retries_used >= max_retries: + # The exhausted budget only ends flaky retries; a new fixable + # branch-caused failure at this head still deserves diagnosis. + actions.append("diagnose_ci_failure") actions.append("stop_exhausted_retries") else: actions.append("diagnose_ci_failure") @@ -967,10 +1137,19 @@ def recommend_actions( if is_github_candidate_clear( pr, checks_summary, + failed_runs, + failed_jobs, new_review_items, unresolved_threads, ): actions.append("verify_external_gates") + # The watcher can dedupe conversation comments as "seen" but cannot + # verify that the controller dispositioned them; issue comments have + # no resolvable thread. `verify_external_gates` therefore asserts only + # GitHub-native gates. Remind the controller whenever any published + # feedback exists for this PR. + if has_published_feedback: + actions.append("confirm_feedback_disposition") elif str(pr.get("mergeable") or "") not in {"MERGEABLE", "CONFLICTING"}: actions.append("wait_for_mergeability") @@ -1024,11 +1203,25 @@ def collect_snapshot(args, locked_state_path, locked_pr_identity): # After resolving `--pr auto`, reuse the concrete PR number. checks = get_pr_checks(str(pr["number"]), repo=pr["repo"]) checks_summary = summarize_checks(checks) + pr_check_run_ids = workflow_run_ids_from_checks(checks) workflow_runs = get_workflow_runs_for_sha(pr["repo"], pr["head_sha"]) - failed_runs = failed_runs_from_workflow_runs(workflow_runs, pr["head_sha"]) + all_failed_runs = failed_runs_from_workflow_runs(workflow_runs, pr["head_sha"]) + # Only workflow runs backing the PR's own checks gate readiness, diagnosis, + # and retries. Other failed head-SHA workflows (push- or schedule-triggered) + # are reported informationally so they can never wedge the watcher in an + # unretryable, unclearable state. + failed_runs, non_pr_check_failed_runs = partition_runs_by_pr_checks( + all_failed_runs, + pr_check_run_ids, + ) + pr_check_workflow_runs = [ + run + for run in workflow_runs + if isinstance(run, dict) and run.get("id") in pr_check_run_ids + ] failed_jobs = failed_jobs_from_workflow_runs( pr["repo"], - workflow_runs, + pr_check_workflow_runs, pr["head_sha"], ) @@ -1043,6 +1236,7 @@ def collect_snapshot(args, locked_state_path, locked_pr_identity): candidate_changed, retries_used, args.max_flaky_retries, + has_published_feedback=bool(all_review_items or review_threads), ) state["version"] = STATE_VERSION @@ -1056,6 +1250,7 @@ def collect_snapshot(args, locked_state_path, locked_pr_identity): "pr": pr, "checks": checks_summary, "failed_runs": failed_runs, + "non_pr_check_failed_runs": non_pr_check_failed_runs, "failed_jobs": failed_jobs, "review_items": all_review_items, "new_review_items": new_review_items, @@ -1125,7 +1320,9 @@ def _retry_failed_now_locked(args, state_path, locked_pr_identity): if pr["closed"] or pr["merged"]: result["reason"] = "pr_closed" return result - if checks_summary["failed_count"] <= 0 and not snapshot["failed_jobs"]: + # Use the exact predicate recommend_actions uses, so a recommended retry + # is never refused with no_failed_pr_checks. + if not has_failed_pr_checks(checks_summary, failed_runs, snapshot["failed_jobs"]): result["reason"] = "no_failed_pr_checks" return result if not failed_runs: @@ -1201,15 +1398,46 @@ def print_event(event, payload): print_json({"event": event, "payload": payload}) +def transient_retry_delay(poll_seconds, consecutive_failures): + return min(poll_seconds * (2 ** (consecutive_failures - 1)), 300) + + def run_watch(args): state_path, locked_pr_identity = resolve_locked_target(args) + max_transient_failures = getattr(args, "max_transient_failures", 5) + max_polls = getattr(args, "max_polls", 0) + stop_when_clear = getattr(args, "stop_when_clear", False) + consecutive_failures = 0 + polls = 0 with watcher_lock(state_path): while True: - snapshot, _ = collect_snapshot( - args, - locked_state_path=state_path, - locked_pr_identity=locked_pr_identity, - ) + try: + snapshot, _ = collect_snapshot( + args, + locked_state_path=state_path, + locked_pr_identity=locked_pr_identity, + ) + except GhCommandError as err: + # A persistent watcher must survive transient GitHub CLI and + # network failures. Identity failures (RuntimeError) still + # fail closed immediately. + consecutive_failures += 1 + if consecutive_failures > max_transient_failures: + raise + delay = transient_retry_delay(args.poll_seconds, consecutive_failures) + print_event( + "transient_error", + { + "error": str(err), + "consecutive_failures": consecutive_failures, + "max_transient_failures": max_transient_failures, + "retry_in_seconds": delay, + }, + ) + time.sleep(delay) + continue + consecutive_failures = 0 + polls += 1 print_event( "snapshot", { @@ -1223,6 +1451,27 @@ def run_watch(args): print_event( "stop", { + "reason": "terminal_actions", + "actions": snapshot.get("actions"), + "pr": snapshot.get("pr"), + }, + ) + return 0 + if stop_when_clear and "verify_external_gates" in actions: + print_event( + "stop", + { + "reason": "github_candidate_clear", + "actions": snapshot.get("actions"), + "pr": snapshot.get("pr"), + }, + ) + return 0 + if max_polls and polls >= max_polls: + print_event( + "stop", + { + "reason": "max_polls_reached", "actions": snapshot.get("actions"), "pr": snapshot.get("pr"), }, @@ -1254,7 +1503,7 @@ def main(): snapshot["state_file"] = str(state_path) print_json(snapshot) return 0 - except (GhCommandError, RuntimeError, ValueError) as err: + except (GhCommandError, OSError, RuntimeError, ValueError) as err: sys.stderr.write(f"PR watcher error: {err}\n") return 1 except KeyboardInterrupt: diff --git a/skills/babysit-pr/scripts/tests/test_gh_pr_watch.py b/skills/babysit-pr/scripts/tests/test_gh_pr_watch.py index 0779bde..321715d 100644 --- a/skills/babysit-pr/scripts/tests/test_gh_pr_watch.py +++ b/skills/babysit-pr/scripts/tests/test_gh_pr_watch.py @@ -43,6 +43,7 @@ def sample_checks(**overrides): "pending_count": 0, "failed_count": 0, "passed_count": 4, + "cancelled_count": 0, "all_terminal": True, "items": [], } @@ -50,8 +51,8 @@ def sample_checks(**overrides): return value -def sample_args(state_file): - return argparse.Namespace( +def sample_args(state_file, **overrides): + args = argparse.Namespace( pr="123", repo="example/project", state_file=str(state_file), @@ -59,7 +60,13 @@ def sample_args(state_file): completion_policy="ready_to_merge", poll_seconds=1, eligible_run_id=[99], + max_transient_failures=2, + max_polls=0, + stop_when_clear=False, ) + for key, value in overrides.items(): + setattr(args, key, value) + return args class ReviewStateTests(unittest.TestCase): @@ -155,6 +162,44 @@ def test_all_published_authors_are_emitted_as_untrusted_evidence(self): self.assertEqual(all_items, new_items) self.assertEqual("$(unsafe command)", all_items[0]["body"]) + def test_ghost_author_comment_still_surfaces_as_new(self): + comments = [ + { + "id": 7, + "user": None, + "author_association": "NONE", + "body": "Comment from a deleted account.", + "created_at": "2026-01-01T00:00:00Z", + "html_url": "https://example.test/7", + } + ] + with mock.patch.object( + WATCHER, + "_review_payloads", + return_value=(comments, [], []), + ): + all_items, new_items = WATCHER.fetch_review_state( + sample_pr(), {}, "operator" + ) + self.assertEqual(1, len(all_items)) + self.assertEqual(1, len(new_items)) + self.assertEqual("external", new_items[0]["source_class"]) + + def test_pr_checks_empty_payload_fails_closed_unless_no_checks(self): + with mock.patch.object( + WATCHER, "gh_capture", return_value=("", "some transient error") + ): + with self.assertRaisesRegex( + WATCHER.GhCommandError, "check state is unknown" + ): + WATCHER.get_pr_checks("123", "example/project") + with mock.patch.object( + WATCHER, + "gh_capture", + return_value=("", "no checks reported on the 'feature' branch"), + ): + self.assertEqual([], WATCHER.get_pr_checks("123", "example/project")) + def test_seen_items_remain_in_complete_feedback(self): state = {"seen_issue_comment_ids": ["1"]} comments = [ @@ -179,6 +224,78 @@ def test_seen_items_remain_in_complete_feedback(self): self.assertEqual([], new_items) +class ParseArgsTests(unittest.TestCase): + """The documented command lines must parse; doc drift fails here.""" + + def parse(self, *argv): + with mock.patch.object(WATCHER.sys, "argv", ["gh_pr_watch.py", *argv]): + return WATCHER.parse_args() + + def test_documented_once_snapshot_parses(self): + args = self.parse("--pr", "123", "--once") + self.assertTrue(args.once) + self.assertEqual("watch_until_closed", args.completion_policy) + + def test_documented_watch_with_policy_parses(self): + args = self.parse( + "--pr", "123", "--completion-policy", "ready_to_merge", "--watch" + ) + self.assertTrue(args.watch) + self.assertEqual("ready_to_merge", args.completion_policy) + + def test_documented_stop_when_clear_implies_ready_to_merge(self): + args = self.parse("--pr", "123", "--watch", "--stop-when-clear") + self.assertTrue(args.stop_when_clear) + self.assertEqual("ready_to_merge", args.completion_policy) + + def test_documented_max_polls_parses(self): + args = self.parse("--pr", "123", "--watch", "--max-polls", "3") + self.assertEqual(3, args.max_polls) + + def test_stop_when_clear_rejects_explicit_watch_until_closed(self): + with self.assertRaises(SystemExit): + self.parse( + "--pr", + "123", + "--watch", + "--stop-when-clear", + "--completion-policy", + "watch_until_closed", + ) + + def test_bounded_flags_require_watch(self): + with self.assertRaises(SystemExit): + self.parse("--pr", "123", "--stop-when-clear") + with self.assertRaises(SystemExit): + self.parse("--pr", "123", "--max-polls", "2") + + def test_documented_retry_invocation_parses(self): + args = self.parse( + "--pr", "123", "--retry-failed-now", "--eligible-run-id", "99" + ) + self.assertTrue(args.retry_failed_now) + self.assertEqual([99], args.eligible_run_id) + + def test_once_and_watch_conflict(self): + with self.assertRaises(SystemExit): + self.parse("--pr", "123", "--once", "--watch") + + def test_repo_override_requires_explicit_pr(self): + with self.assertRaises(SystemExit): + self.parse("--repo", "example/project", "--once") + + def test_once_and_retry_conflict(self): + with self.assertRaises(SystemExit): + self.parse( + "--pr", + "123", + "--once", + "--retry-failed-now", + "--eligible-run-id", + "99", + ) + + class PaginationAndThreadTests(unittest.TestCase): def test_check_json_accepts_gh_failure_and_pending_exit_codes(self): error = WATCHER.subprocess.CalledProcessError( @@ -425,6 +542,160 @@ def test_native_clear_only_recommends_external_gate_verification(self): ) self.assertEqual(["verify_external_gates"], actions) + def test_cancelled_check_is_never_clear(self): + summary = WATCHER.summarize_checks([{"bucket": "cancel", "state": "CANCELLED"}]) + self.assertEqual(1, summary["cancelled_count"]) + self.assertEqual(0, summary["failed_count"]) + self.assertTrue(summary["all_terminal"]) + actions = WATCHER.recommend_actions( + sample_pr(), + summary, + [{"run_id": 99}], + [], + [], + [], + False, + 0, + 3, + ) + self.assertNotIn("verify_external_gates", actions) + self.assertIn("diagnose_ci_failure", actions) + + def test_run_ids_extracted_from_all_details_link_shapes(self): + checks = [ + {"link": "https://github.com/e/p/actions/runs/1"}, + {"link": "https://github.com/e/p/actions/runs/2/job/9"}, + {"link": "https://github.com/e/p/actions/runs/3?check_suite_focus=true"}, + {"link": "https://github.com/e/p/actions/runs/4#summary"}, + {"link": "https://external-ci.example.test/build/5"}, + ] + self.assertEqual({1, 2, 3, 4}, WATCHER.workflow_run_ids_from_checks(checks)) + + def test_non_pr_check_run_failures_do_not_block_or_wedge(self): + failed_runs = [ + {"run_id": 99, "workflow_name": "ci"}, + {"run_id": 500, "workflow_name": "nightly-push"}, + ] + pr_runs, other_runs = WATCHER.partition_runs_by_pr_checks(failed_runs, {99}) + self.assertEqual([failed_runs[0]], pr_runs) + self.assertEqual([failed_runs[1]], other_runs) + # A failed push/schedule workflow on the head SHA must not block a + # clear candidate or produce an unretryable retry recommendation. + self.assertTrue( + WATCHER.is_github_candidate_clear( + sample_pr(), sample_checks(), [], [], [], [] + ) + ) + actions = WATCHER.recommend_actions( + sample_pr(), sample_checks(), [], [], [], [], False, 0, 3 + ) + self.assertEqual(["verify_external_gates"], actions) + + def test_pr_check_failed_run_alone_still_recommends_diagnosis(self): + # Diagnosis must mirror the clear predicate: a PR-check-backed failed + # run with green buckets and no failed jobs is not clear, so it must + # never degenerate to `idle`. + actions = WATCHER.recommend_actions( + sample_pr(), + sample_checks(), + [{"run_id": 99}], + [], + [], + [], + False, + 0, + 3, + ) + self.assertIn("diagnose_ci_failure", actions) + self.assertNotIn("idle", actions) + self.assertNotIn("verify_external_gates", actions) + + def test_draft_pr_recommends_draft_resolution_not_idle(self): + actions = WATCHER.recommend_actions( + sample_pr(draft=True), + sample_checks(), + [], + [], + [], + [], + False, + 0, + 3, + ) + self.assertIn("resolve_draft_state", actions) + self.assertNotIn("idle", actions) + self.assertNotIn("verify_external_gates", actions) + + def test_conflicting_pr_recommends_conflict_resolution_not_idle(self): + actions = WATCHER.recommend_actions( + sample_pr(mergeable="CONFLICTING", merge_state_status="DIRTY"), + sample_checks(), + [], + [], + [], + [], + False, + 0, + 3, + ) + self.assertIn("resolve_merge_conflict", actions) + self.assertNotIn("idle", actions) + self.assertNotIn("verify_external_gates", actions) + + def test_failed_runs_block_clear_even_with_green_check_buckets(self): + self.assertFalse( + WATCHER.is_github_candidate_clear( + sample_pr(), + sample_checks(), + [{"run_id": 99, "conclusion": "cancelled"}], + [], + [], + [], + ) + ) + self.assertFalse( + WATCHER.is_github_candidate_clear( + sample_pr(), + sample_checks(), + [], + [{"job_id": 8}], + [], + [], + ) + ) + + def test_native_clear_with_prior_feedback_requires_disposition_check(self): + actions = WATCHER.recommend_actions( + sample_pr(), + sample_checks(), + [], + [], + [], + [], + False, + 0, + 3, + has_published_feedback=True, + ) + self.assertEqual( + ["verify_external_gates", "confirm_feedback_disposition"], + actions, + ) + + def test_closed_pr_with_unresolved_threads_requires_feedback_processing(self): + actions = WATCHER.recommend_actions( + sample_pr(closed=True, state="CLOSED"), + sample_checks(), + [], + [], + [], + [{"id": "thread-1"}], + False, + 0, + 3, + ) + self.assertEqual(["process_review_feedback", "stop_pr_closed"], actions) + def test_unresolved_thread_blocks_native_clear(self): actions = WATCHER.recommend_actions( sample_pr(), @@ -518,6 +789,15 @@ def test_state_target_mismatch_fails_closed(self): Path("state.json"), ) + def test_state_target_accepts_repo_case_variants(self): + # Mixed-case invocations share one state file; validation must use + # the same case-insensitive match as default_state_file_for. + WATCHER.validate_state_target( + {"pr": {"repo": "Example/Project", "number": 123}}, + sample_pr(), + Path("state.json"), + ) + def test_state_is_atomic_and_round_trips(self): with tempfile.TemporaryDirectory() as directory: state_path = Path(directory) / "state.json" @@ -528,9 +808,24 @@ def test_state_is_atomic_and_round_trips(self): def test_default_state_file_is_product_neutral(self): state_path = WATCHER.default_state_file_for(sample_pr()) - self.assertIn("agent-babysit-pr-example-project-pr123", str(state_path)) + self.assertIn("agent-babysit-pr-example-project-", str(state_path)) + self.assertTrue(state_path.name.endswith("-pr123.json")) self.assertNotIn("codex", str(state_path).lower()) + def test_distinct_repositories_never_share_a_state_file(self): + first = WATCHER.default_state_file_for(sample_pr(repo="owner/re-po")) + second = WATCHER.default_state_file_for(sample_pr(repo="owner-re/po")) + third = WATCHER.default_state_file_for(sample_pr(repo="owner/re_po")) + self.assertEqual(3, len({first, second, third})) + + def test_repo_case_variants_share_one_state_file(self): + # GitHub slugs are case-insensitive; `--repo Owner/Repo` and a + # URL-derived `owner/repo` must share one state file and one lock. + self.assertEqual( + WATCHER.default_state_file_for(sample_pr(repo="Example/Project")), + WATCHER.default_state_file_for(sample_pr(repo="example/project")), + ) + def test_failed_jobs_include_direct_log_endpoint(self): with mock.patch.object( WATCHER, @@ -598,6 +893,137 @@ def test_watch_keeps_polling_a_ready_open_pr(self): ["snapshot", "snapshot", "stop"], [event[0] for event in events] ) + def test_watch_survives_transient_errors_then_recovers(self): + outcomes = [ + WATCHER.GhCommandError("rate limited"), + WATCHER.GhCommandError("bad gateway"), + ( + { + "pr": sample_pr(closed=True, state="CLOSED"), + "checks": sample_checks(), + "actions": ["stop_pr_closed"], + }, + Path("/tmp/state.json"), + ), + ] + events = [] + args = sample_args(Path("/tmp/state.json")) + with ( + mock.patch.object(WATCHER, "resolve_pr", return_value=sample_pr()), + mock.patch.object(WATCHER, "watcher_lock", return_value=nullcontext()), + mock.patch.object(WATCHER, "collect_snapshot", side_effect=outcomes), + mock.patch.object( + WATCHER, "print_event", side_effect=lambda *event: events.append(event) + ), + mock.patch.object(WATCHER.time, "sleep") as sleep, + ): + self.assertEqual(0, WATCHER.run_watch(args)) + self.assertEqual( + ["transient_error", "transient_error", "snapshot", "stop"], + [event[0] for event in events], + ) + self.assertEqual([1, 2], [call.args[0] for call in sleep.call_args_list]) + + def test_watch_fails_after_transient_failure_budget(self): + args = sample_args(Path("/tmp/state.json"), max_transient_failures=1) + with ( + mock.patch.object(WATCHER, "resolve_pr", return_value=sample_pr()), + mock.patch.object(WATCHER, "watcher_lock", return_value=nullcontext()), + mock.patch.object( + WATCHER, + "collect_snapshot", + side_effect=WATCHER.GhCommandError("persistent outage"), + ), + mock.patch.object(WATCHER, "print_event"), + mock.patch.object(WATCHER.time, "sleep"), + ): + with self.assertRaisesRegex(WATCHER.GhCommandError, "persistent outage"): + WATCHER.run_watch(args) + + def test_watch_does_not_retry_identity_failures(self): + args = sample_args(Path("/tmp/state.json")) + with ( + mock.patch.object(WATCHER, "resolve_pr", return_value=sample_pr()), + mock.patch.object(WATCHER, "watcher_lock", return_value=nullcontext()), + mock.patch.object( + WATCHER, + "collect_snapshot", + side_effect=RuntimeError( + "Snapshot target changed repository/PR identity" + ), + ), + mock.patch.object(WATCHER, "print_event"), + mock.patch.object(WATCHER.time, "sleep") as sleep, + ): + with self.assertRaisesRegex(RuntimeError, "identity"): + WATCHER.run_watch(args) + sleep.assert_not_called() + + def test_watch_stop_when_clear_emits_reason_and_exits(self): + snapshot = ( + { + "pr": sample_pr(), + "checks": sample_checks(), + "actions": [ + "verify_external_gates", + "confirm_feedback_disposition", + ], + }, + Path("/tmp/state.json"), + ) + events = [] + args = sample_args(Path("/tmp/state.json"), stop_when_clear=True) + with ( + mock.patch.object(WATCHER, "resolve_pr", return_value=sample_pr()), + mock.patch.object(WATCHER, "watcher_lock", return_value=nullcontext()), + mock.patch.object(WATCHER, "collect_snapshot", return_value=snapshot), + mock.patch.object( + WATCHER, "print_event", side_effect=lambda *event: events.append(event) + ), + mock.patch.object(WATCHER.time, "sleep"), + ): + self.assertEqual(0, WATCHER.run_watch(args)) + self.assertEqual(["snapshot", "stop"], [event[0] for event in events]) + self.assertEqual("github_candidate_clear", events[1][1]["reason"]) + + def test_watch_max_polls_bounds_foreground_execution(self): + snapshot = ( + { + "pr": sample_pr(), + "checks": sample_checks(pending_count=1, all_terminal=False), + "actions": ["wait_for_checks"], + }, + Path("/tmp/state.json"), + ) + events = [] + args = sample_args(Path("/tmp/state.json"), max_polls=2) + with ( + mock.patch.object(WATCHER, "resolve_pr", return_value=sample_pr()), + mock.patch.object(WATCHER, "watcher_lock", return_value=nullcontext()), + mock.patch.object(WATCHER, "collect_snapshot", return_value=snapshot), + mock.patch.object( + WATCHER, "print_event", side_effect=lambda *event: events.append(event) + ), + mock.patch.object(WATCHER.time, "sleep"), + ): + self.assertEqual(0, WATCHER.run_watch(args)) + self.assertEqual( + ["snapshot", "snapshot", "stop"], [event[0] for event in events] + ) + self.assertEqual("max_polls_reached", events[2][1]["reason"]) + + def test_authenticated_login_is_cached_per_process(self): + WATCHER._authenticated_login_cache = None + try: + with mock.patch.object( + WATCHER, "gh_json", return_value={"login": "operator"} + ) as gh_json: + self.assertEqual("operator", WATCHER.get_authenticated_login()) + self.assertEqual("operator", WATCHER.get_authenticated_login()) + self.assertEqual(1, gh_json.call_count) + finally: + WATCHER._authenticated_login_cache = None + class RetryTests(unittest.TestCase): def test_retry_uses_failed_runs_and_increments_head_budget(self): @@ -642,6 +1068,90 @@ def test_retry_uses_failed_runs_and_increments_head_budget(self): ) self.assertEqual(2, save_state.call_args.args[1]["retries_by_sha"]["head-1"]) + def test_retry_accepts_cancelled_only_check_failures(self): + # recommend_actions counts cancelled checks as failed PR checks; the + # retry gate must mirror that so a recommended retry is never refused. + state = { + "pr": {"repo": "example/project", "number": 123}, + "retries_by_sha": {}, + } + snapshot = { + "pr": sample_pr(), + "checks": sample_checks( + cancelled_count=1, + items=[ + { + "bucket": "cancel", + "link": "https://github.com/example/project/actions/runs/99/job/8", + } + ], + ), + "failed_runs": [{"run_id": 99}], + "failed_jobs": [], + "retry_state": { + "current_sha_retries_used": 0, + "max_flaky_retries": 3, + }, + } + with ( + mock.patch.object( + WATCHER, "collect_snapshot", return_value=(snapshot, Path("state.json")) + ), + mock.patch.object(WATCHER, "load_state", return_value=(state, False)), + mock.patch.object(WATCHER, "save_state"), + mock.patch.object(WATCHER, "gh_text") as gh_text, + ): + result = WATCHER._retry_failed_now_locked( + sample_args(Path("state.json")), + Path("state.json"), + ("example/project", 123), + ) + self.assertTrue(result["rerun_attempted"]) + self.assertEqual("rerun_triggered", result["reason"]) + gh_text.assert_called_once() + + def test_retry_accepts_failed_runs_only_state(self): + # Green buckets + a PR-check-backed failed run is the same state that + # blocks clear and recommends diagnosis/retry; the retry gate must + # share recommend_actions' predicate rather than refuse it. + state = { + "pr": {"repo": "example/project", "number": 123}, + "retries_by_sha": {}, + } + snapshot = { + "pr": sample_pr(), + "checks": sample_checks( + items=[ + { + "bucket": "pass", + "link": "https://github.com/example/project/actions/runs/99/job/8", + } + ], + ), + "failed_runs": [{"run_id": 99}], + "failed_jobs": [], + "retry_state": { + "current_sha_retries_used": 0, + "max_flaky_retries": 3, + }, + } + with ( + mock.patch.object( + WATCHER, "collect_snapshot", return_value=(snapshot, Path("state.json")) + ), + mock.patch.object(WATCHER, "load_state", return_value=(state, False)), + mock.patch.object(WATCHER, "save_state"), + mock.patch.object(WATCHER, "gh_text") as gh_text, + ): + result = WATCHER._retry_failed_now_locked( + sample_args(Path("state.json")), + Path("state.json"), + ("example/project", 123), + ) + self.assertTrue(result["rerun_attempted"]) + self.assertEqual("rerun_triggered", result["reason"]) + gh_text.assert_called_once() + def test_retry_rejects_mixed_current_and_unverified_run_ids(self): snapshot = { "pr": sample_pr(), diff --git a/skills/babysit-pr/scripts/tests/test_skill_contract.py b/skills/babysit-pr/scripts/tests/test_skill_contract.py index 12b95b0..aeebeaf 100644 --- a/skills/babysit-pr/scripts/tests/test_skill_contract.py +++ b/skills/babysit-pr/scripts/tests/test_skill_contract.py @@ -1,3 +1,11 @@ +"""Load-bearing contract invariants for the babysit-pr skill. + +These tests intentionally check only stable identifiers — skill names, +terminal states, policy tokens, dependency names, file layout, and +neutrality — not prose phrasing. Behavior is covered by +test_gh_pr_watch.py and the evaluation data under evals/. +""" + from __future__ import annotations import json @@ -25,23 +33,20 @@ def setUpClass(cls): cls.decisions = read("references/ci-and-feedback.md") cls.upstream = read("references/upstream.md") cls.watcher = read("scripts/gh_pr_watch.py") - cls.skill_compact = compact(cls.skill) - cls.decisions_compact = compact(cls.decisions) cls.contract = compact(cls.skill + cls.github + cls.decisions) cls.cases = {item["id"]: item for item in json.loads(read("evals/cases.json"))} - cls.results = { - item["case_id"]: item for item in json.loads(read("evals/results.json")) + cls.expectations = { + item["case_id"]: item + for item in json.loads(read("evals/expectations.json")) } def test_frontmatter_and_runtime_neutral_contract(self): self.assertTrue(self.skill.startswith("---\nname: babysit-pr\n")) self.assertNotIn("Codex", self.contract) - self.assertIn("compatible capabilities", self.skill) - self.assertIn("does not constrain the core contract", self.skill) self.assertNotIn("REVIEW_BOT_LOGIN_KEYWORDS", self.watcher) self.assertNotIn("/tmp/codex", self.watcher) - def test_completion_policies_and_results_are_distinct(self): + def test_completion_policies_and_terminal_states_are_stable(self): for policy in ( "ready_to_merge", "merge_when_ready", @@ -51,59 +56,14 @@ def test_completion_policies_and_results_are_distinct(self): self.assertIn(policy, self.watcher) for state in ("ready_to_merge", "merged", "closed", "blocked"): self.assertIn(state, self.skill) - self.assertIn( - "a ready snapshot is progress rather than terminal", self.contract - ) - def test_candidate_and_remote_gates_are_current(self): - for phrase in ( - "exact head and base SHAs", - "effective diff", - "resulting tree", - "base-only drift", - "zero undispositioned actionable", - "current human and connector review", - "superseding PR", - ): - self.assertIn(phrase, self.contract) - - def test_review_dependency_and_mutation_ownership_are_explicit(self): + def test_review_dependency_is_repository_owned(self): self.assertIn("review-code-change", self.contract) - self.assertIn("fresh read-only context", self.contract) - self.assertIn("exclusive mutation ownership", self.contract) - self.assertIn("standalone `ready_to_merge`", self.skill) - self.assertIn("prevents a standalone watcher", self.skill) - self.assertIn("Never create a competing branch", self.skill_compact) - - def test_tracker_and_cleanup_stay_outside(self): - self.assertIn( - "does not select or implement the original ticket", - self.skill_compact, - ) - self.assertIn("Do not request or use tracker-transition", self.skill_compact) - self.assertIn("branch-deletion", self.skill_compact) - self.assertIn("Leave tracker transition", self.skill_compact) - - def test_ci_feedback_security_and_published_state(self): - self.assertIn("direct failed-job log endpoint", self.decisions_compact) - self.assertIn( - "pending reviews and their inline comments", self.decisions_compact - ) - self.assertIn("untrusted content", self.decisions_compact) - self.assertIn("never makes embedded commands safe", self.decisions_compact) - self.assertIn("bounded", self.skill) - - def test_watcher_contract_is_deterministic(self): - for phrase in ( - "one-shot snapshot", - "jsonl monitoring", - "pagination", - "pending review", - "reviewThreads", - "nonblocking lock", - "atomic", - ): - self.assertIn(phrase.lower(), (self.github + self.watcher).lower()) + + def test_watcher_paths_are_skill_relative(self): + self.assertNotIn("skills/babysit-pr/scripts", self.skill) + self.assertNotIn("skills/babysit-pr/scripts", self.github) + self.assertIn("scripts/gh_pr_watch.py", self.skill) def test_upstream_is_pinned_and_licensed(self): self.assertIn("a770e5b8470d3320eb53a56a286ea4a0a70a1f59", self.upstream) @@ -111,60 +71,28 @@ def test_upstream_is_pinned_and_licensed(self): self.assertTrue((SKILL_ROOT / "LICENSE.apache-2.0").is_file()) self.assertNotIn("raw.githubusercontent.com", self.watcher) - def test_eval_surface_covers_required_boundaries(self): - required = { - "ready-without-merge", - "authorized-merge", - "watch-ready-until-closed", - "feedback-before-retry", - "pending-review-publication", - "branch-caused-ci-fix", - "infrastructure-retry", - "retry-budget-exhausted", - "external-head-change", - "stale-approval", - "connector-current-head", - "authorized-reply-resolution", - "unauthorized-human-response", - "other-worker-owns-candidate", - "unrelated-base-drift", - "relevant-base-drift", - "closed-without-merge", - "superseding-and-partial-api", - "missing-capability", - "untrusted-content", - "documented-absent-gates", - } - self.assertEqual(required, set(self.cases)) - self.assertEqual(required, set(self.results)) + def test_eval_cases_and_expectations_stay_paired(self): + self.assertTrue(self.cases) + self.assertEqual(set(self.cases), set(self.expectations)) - def test_eval_results_preserve_authority_and_review(self): + def test_eval_expectations_preserve_authority_boundaries(self): self.assertEqual( "ready_to_merge", - self.results["ready-without-merge"]["terminal_state"], + self.expectations["ready-without-merge"]["terminal_state"], ) - self.assertEqual("merged", self.results["authorized-merge"]["terminal_state"]) self.assertEqual( - "closed", self.results["closed-without-merge"]["terminal_state"] + "merged", self.expectations["authorized-merge"]["terminal_state"] ) self.assertEqual( - "blocked", self.results["missing-capability"]["terminal_state"] - ) - self.assertIn( - "review-code-change", - " ".join(self.results["branch-caused-ci-fix"]["required_actions"]), + "closed", self.expectations["closed-without-merge"]["terminal_state"] ) - self.assertIn( - "do not reply", - self.results["unauthorized-human-response"]["required_actions"], + self.assertEqual( + "blocked", self.expectations["missing-capability"]["terminal_state"] ) - def test_ui_and_repository_docs_are_updated(self): - metadata = read("agents/openai.yaml") - self.assertIn('display_name: "Babysit PR"', metadata) - self.assertIn("$babysit-pr", metadata) - self.assertIn("skills/babysit-pr", (REPOSITORY_ROOT / "README.md").read_text()) - self.assertIn("babysit-pr", (REPOSITORY_ROOT / "CHANGELOG.md").read_text()) + def test_runtime_adapters_exist_for_both_products(self): + self.assertIn('display_name: "Babysit PR"', read("agents/openai.yaml")) + self.assertIn("Claude Code adapter", read("agents/claude-code.md")) if __name__ == "__main__": diff --git a/skills/implement-epic/SKILL.md b/skills/implement-epic/SKILL.md index e96f6e7..e1a5735 100644 --- a/skills/implement-epic/SKILL.md +++ b/skills/implement-epic/SKILL.md @@ -1,6 +1,6 @@ --- name: implement-epic -description: Implement one or more GitHub or Linear epics through their live dependency graphs by selecting ready PR-sized children and invoking the repository-owned implement-ticket skill exactly once per child. Use when an agent should sequence an entire epic, one named child, or a named subset; refresh graph state after merges; preserve authority and isolation boundaries; and verify epic-wide acceptance and closeout without duplicating single-ticket implementation, review, PR, merge, or cleanup mechanics. +description: Implement, work through, or finish one or more GitHub or Linear epics through their live dependency graphs by selecting ready PR-sized children and invoking the repository-owned implement-ticket skill exactly once per child. Use when asked to sequence an entire epic, a parent issue with sub-issues, one named child, or a named subset; refreshes graph state after merges, preserves authority and isolation boundaries, and verifies epic-wide acceptance and closeout without duplicating single-ticket implementation, review, PR, merge, or cleanup mechanics. --- # Implement Epic diff --git a/skills/implement-epic/agents/claude-code.md b/skills/implement-epic/agents/claude-code.md new file mode 100644 index 0000000..129cd85 --- /dev/null +++ b/skills/implement-epic/agents/claude-code.md @@ -0,0 +1,18 @@ +# Claude Code adapter + +Optional discovery metadata for Claude Code and Claude Agent SDK runtimes. It +does not constrain the skill's portable contract. + +- Display name: Implement Epic. +- Suggested prompt: "Use the implement-epic skill to implement this epic through + its live dependency graph and the repository-owned ticket workflow." +- Delegated children: run each `implement-ticket` execution either inline in the + primary context or in one exclusively owned subagent per child; never two + mutating contexts on one candidate. Parallel children require the explicit + authorization and non-overlap proof from the skill contract. +- Graph reads: use `gh` GraphQL (or the Linear MCP connector when Linear owns + the graph) for native parent/sub-issue/blocker relationships; do not derive + the graph from Markdown task lists. +- Long waits: child CI and review waiting belongs to `implement-ticket` and + `babysit-pr`; keep this orchestrating task alive until the requested scope + reaches its completion policy. diff --git a/skills/implement-epic/evals/README.md b/skills/implement-epic/evals/README.md new file mode 100644 index 0000000..93d03ce --- /dev/null +++ b/skills/implement-epic/evals/README.md @@ -0,0 +1,8 @@ +# Implement-epic evaluations + +`cases.json` describes scenario inputs and `expectations.json` records the +required terminal state and actions for each case. There is no automated runner +yet; the pair is consumed by `scripts/tests/test_orchestration_contract.py` as +contract data and can be replayed manually or through a compatible headless +agent harness. Give an evaluated agent only `cases.json` entries; never show it +`expectations.json`. diff --git a/skills/implement-epic/evals/results.json b/skills/implement-epic/evals/expectations.json similarity index 100% rename from skills/implement-epic/evals/results.json rename to skills/implement-epic/evals/expectations.json diff --git a/skills/implement-epic/scripts/tests/test_orchestration_contract.py b/skills/implement-epic/scripts/tests/test_orchestration_contract.py index d85b873..a51a591 100644 --- a/skills/implement-epic/scripts/tests/test_orchestration_contract.py +++ b/skills/implement-epic/scripts/tests/test_orchestration_contract.py @@ -1,3 +1,10 @@ +"""Load-bearing contract invariants for the implement-epic skill. + +These tests intentionally check only stable identifiers — skill names, +terminal states, dependency names, file layout, and neutrality — not prose +phrasing. Scenario coverage lives in the evaluation data under evals/. +""" + from __future__ import annotations import json @@ -27,190 +34,79 @@ def setUpClass(cls): cls.contract = compact(cls.skill + cls.github + cls.linear + cls.closeout) cls.eval_contract = compact( read(SKILL_ROOT / "evals" / "cases.json") - + read(SKILL_ROOT / "evals" / "results.json") + + read(SKILL_ROOT / "evals" / "expectations.json") ) cls.cases = { item["id"]: item for item in json.loads(read(SKILL_ROOT / "evals" / "cases.json")) } - cls.results = { + cls.expectations = { item["case_id"]: item - for item in json.loads(read(SKILL_ROOT / "evals" / "results.json")) + for item in json.loads(read(SKILL_ROOT / "evals" / "expectations.json")) } def test_canonical_name_and_metadata(self): self.assertTrue(self.skill.startswith("---\nname: implement-epic\n")) - self.assertNotIn("implement-epic-sequence", self.contract) - self.assertFalse((REPOSITORY_ROOT / "skills/implement-epic-sequence").exists()) metadata = read(SKILL_ROOT / "agents" / "openai.yaml") self.assertIn('display_name: "Implement Epic"', metadata) - self.assertIn("$implement-epic", metadata) - self.assertNotIn("$implement-epic-sequence", metadata) + self.assertIn( + "Claude Code adapter", read(SKILL_ROOT / "agents" / "claude-code.md") + ) def test_product_neutral_runtime_contract(self): self.assertNotIn("Codex", self.contract) self.assertNotIn("OpenAI", self.contract) self.assertNotIn("Codex", self.eval_contract) self.assertNotIn("OpenAI", self.eval_contract) - self.assertIn("compatible agentic runtime", self.contract) - self.assertIn( - "`implement-epic` and repository-owned `implement-ticket` by stable skill name", - self.contract, - ) - self.assertIn( - "`implement-epic` → `implement-ticket` → (`review-code-change`, `babysit-pr`)", - self.contract, - ) - self.assertIn("retain task state", self.contract) - self.assertIn("poll or wait for asynchronous", self.contract) - self.assertIn( - "worker and subagent describe possible isolated execution roles", - self.contract, - ) - self.assertIn("does not constrain the operating contract", self.contract) - def test_implement_ticket_is_required_and_owns_one_ticket(self): - self.assertIn("verify that `implement-ticket` is available", self.contract) + def test_dependency_chain_is_stable_and_acyclic(self): self.assertIn( - "supports `ready_pr`, `merged`, `blocked`, and `requires_epic`", + "`implement-epic` → `implement-ticket` → " + "(`review-code-change`, `babysit-pr`)", self.contract, ) - self.assertIn("Return `blocked` before mutation", self.contract) - self.assertIn("Invoke `implement-ticket` once", self.skill) - self.assertIn("never reproduce its one-ticket workflow", self.skill) - - def test_epic_does_not_own_review_or_ticket_mechanics(self): - self.assertIn("Do not invoke individual review lenses", self.contract) - self.assertIn( - "Do not invoke individual review lenses or `review-code-change` directly", - self.contract, - ) - self.assertNotIn("review-solution-simplicity", self.contract) - self.assertNotIn("review-correctness", self.contract) - self.assertNotIn("review-code-simplicity", self.contract) - self.assertNotIn("fix/re-review cycles", self.contract) - self.assertNotIn("per-PR cleanup", self.closeout) self.assertIn( "Do not make this skill invoke `review-code-change` or `babysit-pr` itself", self.contract, ) + self.assertIn("never recursively invoke this skill", self.contract) - def test_graph_selection_and_refresh_use_live_native_state(self): - self.assertIn( - "native parent, sub-issue, `blockedBy`, and `blocking`", self.contract - ) - self.assertIn("After every verified merge", self.contract) - self.assertIn("reread the native graph", self.contract) - self.assertIn("Do not reuse an earlier ready set", self.contract) - self.assertIn( - "A `ready_pr` result does not satisfy a dependency", self.contract - ) - - def test_result_handling_is_fail_closed(self): + def test_child_terminal_states_are_stable(self): for state in ("ready_pr", "merged", "blocked", "requires_epic"): self.assertIn(f"`{state}`", self.contract) - self.assertIn("Do not count the child complete", self.contract) - self.assertIn("Never count it as complete", self.contract) - self.assertIn("never recursively invoke this skill", self.contract) - def test_scope_and_authority_do_not_expand(self): - self.assertIn( - "Pass authority into `implement-ticket` without expansion", self.contract - ) - self.assertIn( - "Child merge authority does not imply parent closeout", self.contract - ) - self.assertIn("For one named child, stop", self.contract) - self.assertIn("Do not implement unnamed siblings", self.contract) - self.assertIn("Parent-close authority is separate", self.contract) - - def test_delegated_mutation_is_exclusive(self): - self.assertIn( - "exclusive ownership of one verified ticket worktree", self.contract - ) - self.assertIn( - "Never run two mutating contexts against the same candidate", self.contract - ) - self.assertIn( - "reject parallel mutation", - self.results["parallel-nonoverlap-required"]["required_actions"], - ) - self.assertEqual( - "serial_execution_required", - self.results["parallel-nonoverlap-required"]["workflow_state"], - ) + def test_epic_does_not_own_lens_mechanics(self): + self.assertNotIn("review-solution-simplicity", self.contract) + self.assertNotIn("review-correctness", self.contract) + self.assertNotIn("review-code-simplicity", self.contract) + self.assertNotIn("fix/re-review cycles", self.contract) - def test_tracker_and_pr_host_ownership_are_separate(self): - self.assertIn("Resolve issue-tracker ownership independently", self.contract) - self.assertIn("`implement-ticket` owns GitHub PR-host", self.contract) - self.assertIn("same-numbered GitHub issues", self.contract) - self.assertIn("same-numbered GitHub issue", self.contract) - - def test_closeout_is_epic_wide_and_conservative(self): - self.assertIn("Verify parent acceptance criteria", self.contract) - self.assertIn("Sweep late feedback", self.contract) - self.assertIn("keep the epic open", self.contract) - self.assertIn("invoke `implement-ticket`", self.contract) - self.assertIn("Close each epic separately", self.contract) - - def test_forward_cases_cover_composed_contract(self): - required = { - "two-child-refresh-chain", - "named-child-boundary", - "ready-pr-does-not-unblock", - "blocked-missing-sibling-outcome", - "blocked-then-independent-ready", - "missing-implement-ticket", - "exclusive-delegated-worktree", - "ticket-evidence-pass-through", - "late-feedback-blocks-closeout", - "authorized-full-epic-closeout", - "umbrella-closeout-separately", - "unexpected-requires-epic-child-result", - "parallel-nonoverlap-required", - "equivalent-isolated-context-profile", - "missing-review-dependency-through-ticket", - "missing-isolation-capability", - "missing-asynchronous-wait", - "transitive-babysit-results", - } - self.assertEqual(required, set(self.cases)) - self.assertEqual(required, set(self.results)) + def test_eval_cases_and_expectations_stay_paired(self): + self.assertTrue(self.cases) + self.assertEqual(set(self.cases), set(self.expectations)) - def test_forward_results_preserve_critical_boundaries(self): + def test_eval_expectations_preserve_critical_boundaries(self): self.assertEqual( "waiting_for_child_merge", - self.results["ready-pr-does-not-unblock"]["workflow_state"], + self.expectations["ready-pr-does-not-unblock"]["workflow_state"], ) self.assertEqual( - "blocked", self.results["missing-implement-ticket"]["workflow_state"] + "blocked", self.expectations["missing-implement-ticket"]["workflow_state"] ) self.assertEqual( "closeout_blocked", - self.results["late-feedback-blocks-closeout"]["workflow_state"], + self.expectations["late-feedback-blocks-closeout"]["workflow_state"], ) self.assertEqual( - "blocked", - self.results["unexpected-requires-epic-child-result"]["workflow_state"], - ) - self.assertEqual( - "epic_children_merged", - self.results["equivalent-isolated-context-profile"]["workflow_state"], + "serial_execution_required", + self.expectations["parallel-nonoverlap-required"]["workflow_state"], ) for case_id in ( "missing-review-dependency-through-ticket", "missing-isolation-capability", "missing-asynchronous-wait", ): - self.assertEqual("blocked", self.results[case_id]["workflow_state"]) - self.assertEqual( - "mixed_ticket_results", - self.results["transitive-babysit-results"]["workflow_state"], - ) - self.assertIn( - "do not invoke babysit-pr directly", - self.results["transitive-babysit-results"]["required_actions"], - ) + self.assertEqual("blocked", self.expectations[case_id]["workflow_state"]) if __name__ == "__main__": diff --git a/skills/implement-ticket/SKILL.md b/skills/implement-ticket/SKILL.md index 712bd57..b188913 100644 --- a/skills/implement-ticket/SKILL.md +++ b/skills/implement-ticket/SKILL.md @@ -1,6 +1,6 @@ --- name: implement-ticket -description: Implement exactly one standalone GitHub or Linear ticket, or one named child of a larger epic, through an isolated branch and pull request. Use when an agent should resolve live ticket and dependency context, enforce readiness and authority boundaries, implement and validate one coherent change, run an initial repository-owned review, delegate the published PR lifecycle to babysit-pr, and verify tracker, mainline, and cleanup outcomes. Detect whole-epic requests before mutation and route them toward implement-epic without creating a circular skill dependency. +description: Implement, build, or fix exactly one standalone GitHub or Linear ticket or issue, or one named child of a larger epic, through an isolated branch and pull request. Use when asked to implement a ticket end to end; resolves live ticket and dependency context, enforces readiness and authority boundaries, implements and validates one coherent change, runs an initial repository-owned review, delegates the published PR lifecycle to babysit-pr, and verifies tracker, mainline, and cleanup outcomes. Detects whole-epic requests before mutation and routes them toward implement-epic without creating a circular skill dependency. --- # Implement Ticket @@ -127,7 +127,7 @@ Use this order: 1. Current user instructions. 2. Live ticket, relationship, branch, PR, and review state. -3. Repository `AGENTS.md` and equivalent local instructions. +3. Repository agent instructions (`AGENTS.md`, `CLAUDE.md`, or equivalent). 4. Named architecture, design, contract, migration, and rollout documents. 5. Current code and tests. 6. Prior summaries or memory. diff --git a/skills/implement-ticket/agents/claude-code.md b/skills/implement-ticket/agents/claude-code.md new file mode 100644 index 0000000..d42f073 --- /dev/null +++ b/skills/implement-ticket/agents/claude-code.md @@ -0,0 +1,18 @@ +# Claude Code adapter + +Optional discovery metadata for Claude Code and Claude Agent SDK runtimes. It +does not constrain the skill's portable contract. + +- Display name: Implement Ticket. +- Suggested prompt: "Use the implement-ticket skill to implement this ticket, + run its initial review, delegate the published PR to babysit-pr, and verify + the authorized result." +- Isolated implementation state: create the ticket branch in a dedicated git + worktree (for example via Claude Code's worktree support) owned exclusively by + one mutating context. +- Fresh read-only review context: invoke repository-owned `review-code-change` + in a subagent (Agent tool) restricted to read-only tools, giving it only raw + candidate evidence — never the implementation transcript. +- Long waits: follow babysit-pr's Claude adapter for CI and review monitoring; + ordinary pending CI is not a blocker, so keep the task alive with background + monitoring rather than returning early. diff --git a/skills/implement-ticket/evals/README.md b/skills/implement-ticket/evals/README.md new file mode 100644 index 0000000..38dbc71 --- /dev/null +++ b/skills/implement-ticket/evals/README.md @@ -0,0 +1,16 @@ +# Implement-ticket evaluations + +- `cases.json` + `expectations.json`: scenario contract data consumed by + `scripts/tests/test_implement_ticket_contract.py`. +- `forward_cases.json` + `forward_expectations.json`: result-blind forward + evaluations executed by `scripts/evals/run_forward.py`. Case artifacts carry + pre-classified scenario flags (`whole_epic`, CI `classification`, + `base_drift`, and similar), so grading covers obligation mapping and + terminal-state selection rather than evidence classification. The default + executor (`scripts/evals/fixture_executor.py`) is a deterministic simulation + of a compliant runtime, not a model; pass `--executor` to grade a real agent + runtime such as `scripts/evals/claude_executor.py`. Every expectation defines + `forbidden_actions`, so emitting the whole action vocabulary fails. + +Give an evaluated agent only the case artifacts; never show it any expectations +file. diff --git a/skills/implement-ticket/evals/results.json b/skills/implement-ticket/evals/expectations.json similarity index 100% rename from skills/implement-ticket/evals/results.json rename to skills/implement-ticket/evals/expectations.json diff --git a/skills/implement-ticket/evals/forward_expectations.json b/skills/implement-ticket/evals/forward_expectations.json index ebc1bd9..d6106d1 100644 --- a/skills/implement-ticket/evals/forward_expectations.json +++ b/skills/implement-ticket/evals/forward_expectations.json @@ -1,20 +1,240 @@ [ - {"case_id": "whole-epic-before-ticket-dependencies", "target_skill": "implement-ticket", "terminal_state": "requires_epic", "required_actions": ["route_before_ticket_dependencies", "perform_no_mutation"], "private_grader_marker": "never-send-expectations-to-executor"}, - {"case_id": "missing-babysit-pr", "target_skill": "implement-ticket", "terminal_state": "blocked", "required_actions": ["fail_before_mutation", "name_missing_babysit_pr"]}, - {"case_id": "standalone-ready-pr", "target_skill": "implement-ticket", "terminal_state": "ready_pr", "required_actions": ["invoke_ready_to_merge", "verify_non_merge_gates"]}, - {"case_id": "authorized-merge-closeout", "target_skill": "implement-ticket", "terminal_state": "merged", "required_actions": ["invoke_merge_when_ready", "verify_merge_live", "caller_verifies_mainline_tracker_cleanup"]}, - {"case_id": "linear-ticket-github-pr", "target_skill": "implement-ticket", "terminal_state": "merged", "required_actions": ["preserve_tracker_pr_host_separation", "invoke_merge_when_ready", "caller_verifies_mainline_tracker_cleanup"]}, - {"case_id": "published-feedback-fix", "target_skill": "implement-ticket", "terminal_state": "ready_pr", "required_actions": ["ticket_scoped_fix", "revalidate_commit_push", "fresh_review_code_change", "rebuild_remote_gates"]}, - {"case_id": "branch-caused-ci-fix", "target_skill": "implement-ticket", "terminal_state": "merged", "required_actions": ["ticket_scoped_fix", "revalidate_commit_push", "fresh_review_code_change", "rebuild_remote_gates"]}, - {"case_id": "infrastructure-retry", "target_skill": "implement-ticket", "terminal_state": "ready_pr", "required_actions": ["retry_diagnosed_run_only", "make_no_code_mutation"]}, - {"case_id": "unauthorized-human-response", "target_skill": "implement-ticket", "terminal_state": "blocked", "required_actions": ["do_not_reply_or_resolve", "preserve_feedback_gate"]}, - {"case_id": "stale-connector-verdict", "target_skill": "implement-ticket", "terminal_state": "blocked", "required_actions": ["reject_stale_connector_verdict"]}, - {"case_id": "unrelated-base-drift", "target_skill": "implement-ticket", "terminal_state": "merged", "required_actions": ["retain_only_proven_unaffected_evidence"]}, - {"case_id": "relevant-base-drift", "target_skill": "implement-ticket", "terminal_state": "merged", "required_actions": ["invalidate_drift_affected_evidence", "revalidate_commit_push", "fresh_review_code_change", "rebuild_remote_gates"]}, - {"case_id": "external-head-change", "target_skill": "implement-ticket", "terminal_state": "ready_pr", "required_actions": ["revalidate_candidate_identity", "invalidate_head_bound_evidence", "rebuild_remote_gates"]}, - {"case_id": "closed-without-merge", "target_skill": "implement-ticket", "terminal_state": "blocked", "required_actions": ["preserve_artifacts", "report_closed_without_merge"]}, - {"case_id": "malformed-babysitter-result", "target_skill": "implement-ticket", "terminal_state": "blocked", "required_actions": ["reject_stale_or_malformed_result", "reread_live_pr"]}, - {"case_id": "delegated-mutation-ownership", "target_skill": "implement-ticket", "terminal_state": "ready_pr", "required_actions": ["transfer_exclusive_mutation_ownership"]}, - {"case_id": "resumed-pr-deduplication", "target_skill": "implement-ticket", "terminal_state": "merged", "required_actions": ["adopt_verified_canonical_pr", "deduplicate_prior_actions"]}, - {"case_id": "implement-epic-consumes-ticket-results", "target_skill": "implement-epic", "terminal_state": "mixed_ticket_results", "required_actions": ["consume_ticket_states_unchanged", "do_not_invoke_babysit_pr_directly", "refresh_graph_after_merged_only"]} + { + "case_id": "whole-epic-before-ticket-dependencies", + "forbidden_actions": [ + "ticket_scoped_fix", + "invoke_ready_to_merge", + "invoke_merge_when_ready" + ], + "private_grader_marker": "never-send-expectations-to-executor", + "required_actions": [ + "route_before_ticket_dependencies", + "perform_no_mutation" + ], + "target_skill": "implement-ticket", + "terminal_state": "requires_epic" + }, + { + "case_id": "missing-babysit-pr", + "forbidden_actions": [ + "invoke_ready_to_merge", + "invoke_merge_when_ready" + ], + "required_actions": [ + "fail_before_mutation", + "name_missing_babysit_pr" + ], + "target_skill": "implement-ticket", + "terminal_state": "blocked" + }, + { + "case_id": "standalone-ready-pr", + "forbidden_actions": [ + "invoke_merge_when_ready" + ], + "required_actions": [ + "invoke_ready_to_merge", + "verify_non_merge_gates" + ], + "target_skill": "implement-ticket", + "terminal_state": "ready_pr" + }, + { + "case_id": "authorized-merge-closeout", + "forbidden_actions": [ + "fail_before_mutation", + "invoke_ready_to_merge", + "perform_no_mutation" + ], + "required_actions": [ + "invoke_merge_when_ready", + "verify_merge_live", + "caller_verifies_mainline_tracker_cleanup" + ], + "target_skill": "implement-ticket", + "terminal_state": "merged" + }, + { + "case_id": "linear-ticket-github-pr", + "forbidden_actions": [ + "invoke_ready_to_merge", + "perform_no_mutation" + ], + "required_actions": [ + "preserve_tracker_pr_host_separation", + "invoke_merge_when_ready", + "caller_verifies_mainline_tracker_cleanup" + ], + "target_skill": "implement-ticket", + "terminal_state": "merged" + }, + { + "case_id": "published-feedback-fix", + "forbidden_actions": [ + "invoke_merge_when_ready" + ], + "required_actions": [ + "ticket_scoped_fix", + "revalidate_commit_push", + "fresh_review_code_change", + "rebuild_remote_gates" + ], + "target_skill": "implement-ticket", + "terminal_state": "ready_pr" + }, + { + "case_id": "branch-caused-ci-fix", + "forbidden_actions": [ + "make_no_code_mutation", + "retry_diagnosed_run_only" + ], + "required_actions": [ + "ticket_scoped_fix", + "revalidate_commit_push", + "fresh_review_code_change", + "rebuild_remote_gates" + ], + "target_skill": "implement-ticket", + "terminal_state": "merged" + }, + { + "case_id": "infrastructure-retry", + "forbidden_actions": [ + "ticket_scoped_fix" + ], + "required_actions": [ + "retry_diagnosed_run_only", + "make_no_code_mutation" + ], + "target_skill": "implement-ticket", + "terminal_state": "ready_pr" + }, + { + "case_id": "unauthorized-human-response", + "forbidden_actions": [ + "invoke_ready_to_merge", + "invoke_merge_when_ready" + ], + "required_actions": [ + "do_not_reply_or_resolve", + "preserve_feedback_gate" + ], + "target_skill": "implement-ticket", + "terminal_state": "blocked" + }, + { + "case_id": "stale-connector-verdict", + "forbidden_actions": [ + "invoke_merge_when_ready", + "invoke_ready_to_merge" + ], + "required_actions": [ + "reject_stale_connector_verdict" + ], + "target_skill": "implement-ticket", + "terminal_state": "blocked" + }, + { + "case_id": "unrelated-base-drift", + "forbidden_actions": [ + "invalidate_drift_affected_evidence" + ], + "required_actions": [ + "retain_only_proven_unaffected_evidence" + ], + "target_skill": "implement-ticket", + "terminal_state": "merged" + }, + { + "case_id": "relevant-base-drift", + "forbidden_actions": [ + "retain_only_proven_unaffected_evidence" + ], + "required_actions": [ + "invalidate_drift_affected_evidence", + "revalidate_commit_push", + "fresh_review_code_change", + "rebuild_remote_gates" + ], + "target_skill": "implement-ticket", + "terminal_state": "merged" + }, + { + "case_id": "external-head-change", + "forbidden_actions": [ + "invoke_merge_when_ready" + ], + "required_actions": [ + "revalidate_candidate_identity", + "invalidate_head_bound_evidence", + "rebuild_remote_gates" + ], + "target_skill": "implement-ticket", + "terminal_state": "ready_pr" + }, + { + "case_id": "closed-without-merge", + "forbidden_actions": [ + "invoke_merge_when_ready", + "ticket_scoped_fix" + ], + "required_actions": [ + "preserve_artifacts", + "report_closed_without_merge" + ], + "target_skill": "implement-ticket", + "terminal_state": "blocked" + }, + { + "case_id": "malformed-babysitter-result", + "forbidden_actions": [ + "invoke_merge_when_ready", + "invoke_ready_to_merge" + ], + "required_actions": [ + "reject_stale_or_malformed_result", + "reread_live_pr" + ], + "target_skill": "implement-ticket", + "terminal_state": "blocked" + }, + { + "case_id": "delegated-mutation-ownership", + "forbidden_actions": [ + "invoke_merge_when_ready", + "reject_concurrent_mutation" + ], + "required_actions": [ + "transfer_exclusive_mutation_ownership" + ], + "target_skill": "implement-ticket", + "terminal_state": "ready_pr" + }, + { + "case_id": "resumed-pr-deduplication", + "forbidden_actions": [ + "fail_before_mutation", + "perform_no_mutation" + ], + "required_actions": [ + "adopt_verified_canonical_pr", + "deduplicate_prior_actions" + ], + "target_skill": "implement-ticket", + "terminal_state": "merged" + }, + { + "case_id": "implement-epic-consumes-ticket-results", + "forbidden_actions": [ + "invoke_merge_when_ready", + "invoke_ready_to_merge" + ], + "required_actions": [ + "consume_ticket_states_unchanged", + "do_not_invoke_babysit_pr_directly", + "refresh_graph_after_merged_only" + ], + "target_skill": "implement-epic", + "terminal_state": "mixed_ticket_results" + } ] diff --git a/skills/implement-ticket/references/cleanup-and-result.md b/skills/implement-ticket/references/cleanup-and-result.md index 55ebaad..700d71c 100644 --- a/skills/implement-ticket/references/cleanup-and-result.md +++ b/skills/implement-ticket/references/cleanup-and-result.md @@ -63,6 +63,24 @@ unless the caller has one. Include every applicable field: - deferred findings and intentionally unperformed work; and - one concrete next action or blocking reason. +For example: + +```text +terminal_state: ready_pr +ticket: LIN-482 (Linear) repository: example/project (GitHub PRs) +pr: #91 open, mergeable base: main @ 7be0…44c2 +branch: scott/lin-482-rate-limits worktree: ../wt-lin-482 +head: 4f2c…9a1d +completion_policy: ready PR only authority_used: implement + push + PR create +validation: `just test` pass @ head; `just check` full gate pass @ head +initial_review: review-code-change clean @ head 4f2c…9a1d vs base 7be0…44c2 +babysit_pr: ready_to_merge @ head, verified against live GitHub state; + CI 6/6 pass, human review approved, 0 unresolved threads +merge: withheld (not authorized) tracker: LIN-482 still In Progress +cleanup: none performed (PR open) deferred: one defer-severity naming finding +next_action: caller may merge; merging will move LIN-482 to Done via PR link +``` + For `ready_pr`, require a verified `babysit-pr: ready_to_merge` result for the still-current open and mergeable PR. Every applicable non-merge gate must pass; the only withheld action is merge. Do not list ordinary pending CI or review as diff --git a/skills/implement-ticket/scripts/evals/claude_executor.py b/skills/implement-ticket/scripts/evals/claude_executor.py new file mode 100644 index 0000000..855a4e2 --- /dev/null +++ b/skills/implement-ticket/scripts/evals/claude_executor.py @@ -0,0 +1,178 @@ +#!/usr/bin/env python3 +"""Real-runtime forward-evaluation executor backed by Claude Code headless mode. + +Reads one result-blind evaluation packet as JSON on stdin (the shape built by +`run_forward.py`), asks a fresh `claude -p` process to act as the target +skill's runtime, and prints one JSON result to stdout: + + {"target_skill": ..., "terminal_state": ..., "actions": [...]} + +The evaluated model receives the skill prompt, the request, and raw scenario +artifacts, plus the closed action vocabulary below so its choices are gradable +against `forward_expectations.json`. It never sees fixture identity or any +expectations. Requires the `claude` CLI on PATH (override with --claude-bin). + +Usage: + python3 run_forward.py --executor "python3 claude_executor.py" +""" + +from __future__ import annotations + +import argparse +import json +import re +import subprocess +import sys + +TERMINAL_STATES = ( + "ready_pr", + "merged", + "blocked", + "requires_epic", + "mixed_ticket_results", +) + +# Closed vocabulary shared with fixture_executor.py and +# forward_expectations.json. Grading is multiple-choice by design: the model +# must decide which obligations apply, not invent matching strings. +ACTION_VOCABULARY = ( + "adopt_verified_canonical_pr", + "caller_verifies_mainline_tracker_cleanup", + "consume_ticket_states_unchanged", + "deduplicate_prior_actions", + "do_not_invoke_babysit_pr_directly", + "do_not_reply_or_resolve", + "fail_before_mutation", + "fresh_review_code_change", + "invalidate_drift_affected_evidence", + "invalidate_head_bound_evidence", + "invoke_merge_when_ready", + "invoke_ready_to_merge", + "make_no_code_mutation", + "name_missing_babysit_pr", + "perform_no_mutation", + "preserve_artifacts", + "preserve_feedback_gate", + "preserve_tracker_pr_host_separation", + "rebuild_remote_gates", + "refresh_graph_after_merged_only", + "reject_concurrent_mutation", + "reject_stale_connector_verdict", + "reject_stale_or_malformed_result", + "report_closed_without_merge", + "reread_live_pr", + "retain_only_proven_unaffected_evidence", + "retry_diagnosed_run_only", + "revalidate_candidate_identity", + "revalidate_commit_push", + "route_before_ticket_dependencies", + "ticket_scoped_fix", + "transfer_exclusive_mutation_ownership", + "verify_merge_live", + "verify_non_merge_gates", +) + + +def build_prompt(payload: dict) -> str: + return "\n".join( + [ + "You are the runtime executing the agent skill below for one", + "scenario. Decide how a fully compliant runtime must terminate", + "and which obligations apply. Do not perform any real tool", + "actions; reason from the artifacts alone.", + "", + "## Skill", + payload["skill_prompt"], + "", + "## Request", + payload["request"], + "", + "## Granted authority (JSON)", + json.dumps(payload.get("authority") or {}, sort_keys=True), + "", + "## Available capabilities (JSON)", + json.dumps(payload.get("capabilities") or {}, sort_keys=True), + "", + "## Scenario artifacts (JSON)", + json.dumps(payload["artifacts"], indent=2, sort_keys=True), + "", + "## Answer format", + "Return ONLY one JSON object, no prose and no code fence:", + '{"target_skill": "' + payload["target_skill"] + '",', + ' "terminal_state": ,", + ' "actions": }', + json.dumps(list(ACTION_VOCABULARY), indent=2), + ] + ) + + +def extract_json_object(text: str) -> dict: + text = text.strip() + fenced = re.search(r"```(?:json)?\s*(\{.*?\})\s*```", text, re.DOTALL) + candidate = fenced.group(1) if fenced else text + start = candidate.find("{") + end = candidate.rfind("}") + if start < 0 or end <= start: + raise ValueError("executor model returned no JSON object") + return json.loads(candidate[start : end + 1]) + + +def run_claude(prompt: str, claude_bin: str, model: str | None) -> dict: + command = [claude_bin, "-p", "--output-format", "json"] + if model: + command.extend(["--model", model]) + completed = subprocess.run( + command, + input=prompt, + text=True, + capture_output=True, + check=False, + ) + if completed.returncode: + raise RuntimeError( + f"claude exited {completed.returncode}: {completed.stderr.strip()}" + ) + envelope = json.loads(completed.stdout) + result_text = envelope.get("result") + if not isinstance(result_text, str): + raise RuntimeError("claude --output-format json returned no result text") + return extract_json_object(result_text) + + +def normalize(payload: dict, observed: dict) -> dict: + actions = observed.get("actions") + if not isinstance(actions, list): + actions = [] + return { + # Report exactly what the model claimed; backfilling from the payload + # would make the grader's target_skill check vacuous. + "target_skill": observed.get("target_skill"), + "terminal_state": observed.get("terminal_state"), + "actions": sorted( + {str(action) for action in actions if str(action) in ACTION_VOCABULARY} + ), + } + + +def parse_args() -> argparse.Namespace: + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("--claude-bin", default="claude") + parser.add_argument( + "--model", + default=None, + help="Optional model override passed to `claude --model`", + ) + return parser.parse_args() + + +def main() -> int: + args = parse_args() + payload = json.load(sys.stdin) + observed = run_claude(build_prompt(payload), args.claude_bin, args.model) + json.dump(normalize(payload, observed), sys.stdout, sort_keys=True) + sys.stdout.write("\n") + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/skills/implement-ticket/scripts/evals/fixture_executor.py b/skills/implement-ticket/scripts/evals/fixture_executor.py index 3529425..72e91ac 100644 --- a/skills/implement-ticket/scripts/evals/fixture_executor.py +++ b/skills/implement-ticket/scripts/evals/fixture_executor.py @@ -1,16 +1,29 @@ #!/usr/bin/env python3 -"""Deterministic fresh-process stand-in for a compatible agent runtime.""" +"""Deterministic fresh-process stand-in for a compatible agent runtime. + +This is a simulation, not a model evaluation: it hand-codes the decisions a +compliant runtime must make so the forward harness and grading stay +deterministic. It cannot detect a model misreading the skill; use +`claude_executor.py` (or another real-runtime executor) via +`run_forward.py --executor` for behavioral coverage. +""" from __future__ import annotations import json import os +import re import sys +def compact(text: str) -> str: + """Normalize whitespace so Markdown reflows do not break matching.""" + return re.sub(r"\s+", " ", text) + + def action_result(payload: dict) -> dict: target = payload["target_skill"] - prompt = payload["skill_prompt"] + prompt = compact(payload["skill_prompt"]) required_contract = { "implement-ticket": ( "`review-code-change` and `babysit-pr` are available", @@ -23,7 +36,7 @@ def action_result(payload: dict) -> dict: "`ready_pr`", ), }[target] - if not all(fragment in prompt for fragment in required_contract): + if not all(compact(fragment) in prompt for fragment in required_contract): return { "target_skill": target, "terminal_state": "blocked", diff --git a/skills/implement-ticket/scripts/evals/run_forward.py b/skills/implement-ticket/scripts/evals/run_forward.py index 2426b09..572b580 100644 --- a/skills/implement-ticket/scripts/evals/run_forward.py +++ b/skills/implement-ticket/scripts/evals/run_forward.py @@ -8,7 +8,6 @@ import shlex import subprocess import sys -import tempfile from pathlib import Path SKILL_ROOT = Path(__file__).resolve().parents[2] @@ -70,6 +69,13 @@ def grade(case_id: str, observed: dict, expected: dict) -> list[str]: ) if missing_actions: failures.append(f"missing actions: {', '.join(missing_actions)}") + # Forbidden actions keep an executor from passing by emitting the whole + # action vocabulary on every case. + forbidden_actions = sorted( + set(expected.get("forbidden_actions") or []) & observed_actions + ) + if forbidden_actions: + failures.append(f"forbidden actions: {', '.join(forbidden_actions)}") if observed.get("target_skill") != expected.get("target_skill"): failures.append( f"target_skill: expected {expected.get('target_skill')!r}, " @@ -118,13 +124,6 @@ def main() -> int: (args.output_dir / f"{case_id}.json").write_text( json.dumps(result, indent=2, sort_keys=True) + "\n" ) - else: - with tempfile.TemporaryDirectory( - prefix="implement-ticket-forward-" - ) as directory: - output_dir = Path(directory) - for case_id, result in observations.items(): - (output_dir / f"{case_id}.json").write_text(json.dumps(result)) summary = { "total": len(observations), diff --git a/skills/implement-ticket/scripts/tests/test_forward_evals.py b/skills/implement-ticket/scripts/tests/test_forward_evals.py index 03c8f6b..b437b9a 100644 --- a/skills/implement-ticket/scripts/tests/test_forward_evals.py +++ b/skills/implement-ticket/scripts/tests/test_forward_evals.py @@ -15,6 +15,14 @@ assert SPEC and SPEC.loader SPEC.loader.exec_module(RUNNER) +CLAUDE_EXECUTOR_PATH = SKILL_ROOT / "scripts" / "evals" / "claude_executor.py" +CLAUDE_SPEC = importlib.util.spec_from_file_location( + "implement_ticket_claude_executor", CLAUDE_EXECUTOR_PATH +) +CLAUDE_EXECUTOR = importlib.util.module_from_spec(CLAUDE_SPEC) +assert CLAUDE_SPEC and CLAUDE_SPEC.loader +CLAUDE_SPEC.loader.exec_module(CLAUDE_EXECUTOR) + class ForwardEvaluationTests(unittest.TestCase): @classmethod @@ -73,6 +81,36 @@ def test_reference_executor_evaluates_the_supplied_skill_prompt(self): self.assertEqual("blocked", observed["terminal_state"]) self.assertIn("skill_contract_incomplete", observed["actions"]) + def test_vocabulary_spam_fails_every_case(self): + """An executor emitting the whole action vocabulary must never pass. + + This forces every expectation record to keep at least one + forbidden action, so the anti-gaming defense stays complete as + cases are added. + """ + expectations = json.loads(self.expectations_text) + vocabulary = sorted(CLAUDE_EXECUTOR.ACTION_VOCABULARY) + for expected in expectations: + spam = { + "target_skill": expected["target_skill"], + "terminal_state": expected["terminal_state"], + "actions": vocabulary, + } + with self.subTest(case=expected["case_id"]): + failures = RUNNER.grade(expected["case_id"], spam, expected) + self.assertTrue( + any("forbidden actions" in failure for failure in failures), + f"{expected['case_id']} has no forbidden_actions teeth", + ) + + def test_claude_executor_reports_model_claims_verbatim(self): + normalized = CLAUDE_EXECUTOR.normalize( + {"target_skill": "implement-ticket"}, + {"terminal_state": "ready_pr", "actions": ["invoke_ready_to_merge"]}, + ) + # No backfill: a model that omits target_skill must fail grading. + self.assertIsNone(normalized["target_skill"]) + def test_required_composition_cases_are_executable(self): observations, failures = RUNNER.evaluate( RUNNER.DEFAULT_CASES, diff --git a/skills/implement-ticket/scripts/tests/test_implement_ticket_contract.py b/skills/implement-ticket/scripts/tests/test_implement_ticket_contract.py index 449992f..a9859b8 100644 --- a/skills/implement-ticket/scripts/tests/test_implement_ticket_contract.py +++ b/skills/implement-ticket/scripts/tests/test_implement_ticket_contract.py @@ -1,3 +1,11 @@ +"""Load-bearing contract invariants for the implement-ticket skill. + +These tests intentionally check only stable identifiers — skill names, +terminal states, policy tokens, routing markers, dependency names, file +layout, and neutrality — not prose phrasing. Behavior is covered by the +forward evaluations under scripts/evals/. +""" + from __future__ import annotations import json @@ -26,23 +34,12 @@ def setUpClass(cls): cls.gates = read(SKILL_ROOT / "references" / "review-and-merge-gates.md") cls.handoff = read(SKILL_ROOT / "references" / "babysit-pr-handoff.md") cls.result = read(SKILL_ROOT / "references" / "cleanup-and-result.md") - cls.babysit_skill = read(REPOSITORY_ROOT / "skills" / "babysit-pr" / "SKILL.md") - cls.babysit_ci = read( - REPOSITORY_ROOT - / "skills" - / "babysit-pr" - / "references" - / "ci-and-feedback.md" - ) cls.skill_compact = compact(cls.skill) - cls.github_compact = compact(cls.github) - cls.linear_compact = compact(cls.linear) - cls.gates_compact = compact(cls.gates) cls.handoff_compact = compact(cls.handoff) cls.result_compact = compact(cls.result) cls.eval_contract = compact( read(SKILL_ROOT / "evals" / "cases.json") - + read(SKILL_ROOT / "evals" / "results.json") + + read(SKILL_ROOT / "evals" / "expectations.json") ) cls.all_contract = compact( cls.skill + cls.github + cls.linear + cls.gates + cls.handoff + cls.result @@ -51,9 +48,9 @@ def setUpClass(cls): item["id"]: item for item in json.loads(read(SKILL_ROOT / "evals" / "cases.json")) } - cls.results = { + cls.expectations = { item["case_id"]: item - for item in json.loads(read(SKILL_ROOT / "evals" / "results.json")) + for item in json.loads(read(SKILL_ROOT / "evals" / "expectations.json")) } def test_frontmatter_and_product_neutral_contract(self): @@ -62,130 +59,19 @@ def test_frontmatter_and_product_neutral_contract(self): self.assertNotIn("OpenAI", self.all_contract) self.assertNotIn("Codex", self.eval_contract) self.assertNotIn("OpenAI", self.eval_contract) - self.assertNotIn("code-review-pro", self.all_contract) - self.assertIn("compatible agentic runtime", self.skill) - self.assertIn( - "`implement-ticket`, repository-owned `review-code-change`, and repository-owned `babysit-pr` by stable skill name", - self.skill_compact, - ) - self.assertIn( - "worker and subagent describe possible isolated execution roles", - self.skill_compact, - ) - self.assertIn( - "does not constrain the operating contract", - self.skill_compact, - ) - - def test_scope_is_exactly_one_ticket(self): - self.assertIn("one ticket per branch, worktree, and PR", self.skill_compact) - self.assertIn("do not select or implement it", self.skill_compact) - self.assertIn("Never claim whole-epic acceptance", self.skill_compact) - self.assertIn("parent and sibling context as evidence", self.skill_compact) - self.assertIn( - "canonical owner of generic single-ticket readiness", - self.skill_compact, - ) - self.assertIn( - "`implement-epic` consumes this contract", - self.skill_compact, - ) - self.assertNotIn("implement-epic-sequence", self.skill_compact) - - def test_epic_routing_is_pre_mutation_and_acyclic(self): - self.assertIn("Guard whole-epic scope before mutation", self.skill_compact) - self.assertIn("Treat a named child of an epic", self.skill_compact) - self.assertIn("Do not invoke or require `implement-epic`", self.skill_compact) - self.assertIn( - "implement-ticket:requires-epic::", - self.all_contract, - ) - self.assertIn("routing cycle detected", self.all_contract) - - def test_readiness_fails_closed_without_widening_scope(self): - self.assertIn("no unresolved native blocker", self.skill_compact) - self.assertIn( - "closed, canceled, or not-planned prerequisite", self.skill_compact - ) - self.assertIn("never absorb that sibling", self.skill_compact) - self.assertIn("require explicit ownership transfer", self.skill_compact) - self.assertIn( - "never claim its candidate as this run's `ready_pr`", self.github_compact - ) - - def test_authority_does_not_expand(self): - self.assertIn("ready PR only", self.skill_compact) - self.assertIn("merge after gates", self.skill_compact) - self.assertIn("merge plus manual transition", self.skill_compact) - self.assertIn( - "parent closure always require separate explicit authority", - self.all_contract, - ) - self.assertIn( - "When merge authority is unclear, stop at a ready PR", self.skill_compact - ) - self.assertIn( - "automatic ticket transition caused by the selected closing syntax", - self.skill_compact, - ) - self.assertIn( - "disclose the consequence in the resolved completion policy", - self.github_compact, - ) - - def test_review_and_babysit_dependencies_fail_before_mutation(self): - self.assertIn("both `review-code-change` and `babysit-pr`", self.skill_compact) - self.assertIn("before creating a branch, worktree", self.skill_compact) - self.assertIn("Return `blocked` before mutation", self.skill_compact) - self.assertIn("private PR loop", self.skill_compact) - self.assertIn("Do not substitute another skill", self.gates_compact) - self.assertIn( - "fresh or minimally inherited read-only context", self.gates_compact - ) - self.assertIn("Exclude the implementation transcript", self.gates_compact) - self.assertIn("at most three full fix/re-review cycles", self.gates_compact) - self.assertIn("Treat any mutation as an integrity failure", self.gates_compact) - - def test_initial_review_and_candidate_integrity_are_preserved(self): - self.assertIn( - "fresh or minimally inherited read-only context", self.gates_compact - ) - self.assertIn( - "initial review is clean for the exact live head", self.gates_compact - ) - self.assertIn("current-candidate non-merge gate", self.handoff_compact) - self.assertIn("Never pass expected findings", self.handoff_compact) - def test_post_publication_lifecycle_has_one_canonical_owner(self): - self.assertIn("sole canonical owner", self.handoff_compact) - self.assertIn("After handoff, `babysit-pr` owns", self.handoff_compact) - self.assertIn("Do not reproduce those mechanics", self.handoff_compact) - self.assertIn("Diagnose CI and feedback", self.babysit_skill) - self.assertIn("Classify CI failures", self.babysit_ci) - self.assertNotIn("gh run rerun", self.all_contract) - self.assertNotIn("failed-job log endpoint", self.all_contract) - self.assertNotIn("connector feedback passes", self.all_contract) + def test_terminal_states_are_stable(self): + for state in ("ready_pr", "merged", "blocked", "requires_epic"): + self.assertIn(state, self.skill) + self.assertIn(state, self.result_compact) - def test_handoff_policy_authority_and_results_are_explicit(self): - for field in ( - "ticket identity", - "worktree", - "exact head SHA", - "exact base SHA", - "validation commands", - "review-cycle budget", - "exclusive mutation ownership", + def test_completion_policies_and_mapping_are_stable(self): + for policy in ( + "ready PR only", + "merge after gates", + "merge plus manual transition", ): - self.assertIn(field, self.handoff_compact) - self.assertIn( - "`ready PR only` invokes `babysit-pr` with `ready_to_merge`", - self.handoff_compact, - ) - self.assertIn( - "`merge after gates` invokes it with `merge_when_ready`", - self.handoff_compact, - ) - self.assertIn("never uses `watch_until_closed`", self.handoff_compact) + self.assertIn(policy, self.skill_compact) for source, target in ( ("ready_to_merge", "ready_pr"), ("merged", "merged"), @@ -193,116 +79,59 @@ def test_handoff_policy_authority_and_results_are_explicit(self): ("blocked", "blocked"), ): self.assertIn(f"`{source}` maps to `{target}`", self.handoff_compact) - self.assertIn("PR closed without merge", self.handoff_compact) + self.assertIn("watch_until_closed", self.handoff_compact) - def test_dependency_graph_is_acyclic_and_epic_is_transitive(self): - self.assertIn("dependency graph is deliberately acyclic", self.skill_compact) + def test_epic_routing_marker_and_cycle_guard_are_stable(self): self.assertIn( - "`babysit-pr` must never invoke `implement-ticket`", self.skill_compact + "implement-ticket:requires-epic::", + self.all_contract, ) - self.assertIn("Do not re-enter this skill", self.skill_compact) - - def test_forward_evaluation_context_is_raw_and_uncontaminated(self): - for artifact in ( - "ticket", - "repository-instruction", - "PR", - "diff", - "resulting-tree", - "check", - "review", - "comment", - "thread", - "worktree", - ): - self.assertIn(artifact, self.handoff) - self.assertIn("Exclude implementation transcripts", self.handoff) - self.assertIn("Treat contaminated evidence as invalid", self.handoff_compact) - - def test_tracker_and_pr_host_ownership_are_separate(self): - self.assertIn("same-numbered GitHub issue", self.github_compact) - self.assertIn("Linear owns the ticket", self.github_compact) - self.assertIn("Resolve repository, PR host", self.linear_compact) - self.assertIn("Do not close or verify the parent epic", self.linear_compact) + self.assertIn("routing cycle detected", self.all_contract) + self.assertIn("implement-epic", self.skill_compact) - def test_cleanup_and_result_contract_are_complete(self): - for state in ("ready_pr", "merged", "blocked", "requires_epic"): - self.assertIn(state, self.result_compact) + def test_dependency_names_are_repository_owned_and_acyclic(self): + self.assertIn("review-code-change", self.skill_compact) + self.assertIn("babysit-pr", self.skill_compact) self.assertIn( - "tracked, staged, unstaged, untracked, and ignored", self.result_compact - ) - self.assertIn("Never force removal", self.result_compact) - self.assertIn("Do not close a parent epic", self.result_compact) - self.assertIn( - "affected native dependency relationships were reread after the transition", - self.result_compact, + "`babysit-pr` must never invoke `implement-ticket`", self.skill_compact ) - self.assertIn("report newly unblocked work", self.github_compact) - self.assertIn("report newly unblocked work", self.linear_compact) - def test_forward_cases_cover_required_boundaries(self): - required = { - "standalone-ready-pr", - "authorized-merge-cleanup", - "named-epic-child-only", - "open-native-blocker", - "closed-prerequisite-outcome-missing", - "sibling-outcome-missing", - "canonical-pr-owned-elsewhere", - "correctness-fix-and-rereview", - "clean-local-review-remote-pending", - "unrelated-base-drift-retained", - "linear-ticket-github-pr", - "epic-with-children", - "undecomposed-epic", - "explicit-child-not-redirected", - "missing-implement-epic", - "repeated-epic-handoff", - "equivalent-isolated-context-profile", - "missing-review-code-change", - "missing-isolation-capability", - "missing-asynchronous-wait", - } - self.assertEqual(required, set(self.cases)) - self.assertEqual(required, set(self.results)) + def test_instruction_file_naming_is_host_neutral(self): + self.assertIn("CLAUDE.md", self.skill_compact) + self.assertIn("AGENTS.md", self.skill_compact) - def test_forward_results_enforce_routing_and_authority(self): + def test_eval_cases_and_expectations_stay_paired(self): + self.assertTrue(self.cases) + self.assertEqual(set(self.cases), set(self.expectations)) + + def test_eval_expectations_enforce_routing_and_authority(self): self.assertEqual( - "ready_pr", self.results["standalone-ready-pr"]["terminal_state"] + "ready_pr", self.expectations["standalone-ready-pr"]["terminal_state"] ) self.assertEqual( "blocked", - self.results["canonical-pr-owned-elsewhere"]["terminal_state"], - ) - self.assertEqual( - "blocked", self.results["sibling-outcome-missing"]["terminal_state"] + self.expectations["canonical-pr-owned-elsewhere"]["terminal_state"], ) self.assertEqual( "requires_epic", - self.results["missing-implement-epic"]["terminal_state"], - ) - self.assertEqual( - "blocked", self.results["repeated-epic-handoff"]["terminal_state"] + self.expectations["missing-implement-epic"]["terminal_state"], ) self.assertEqual( - "ready_pr", - self.results["equivalent-isolated-context-profile"]["terminal_state"], + "blocked", self.expectations["repeated-epic-handoff"]["terminal_state"] ) for case_id in ( "missing-review-code-change", "missing-isolation-capability", "missing-asynchronous-wait", ): - self.assertEqual("blocked", self.results[case_id]["terminal_state"]) - self.assertIn( - "do not merge", - self.results["clean-local-review-remote-pending"]["required_actions"], - ) + self.assertEqual("blocked", self.expectations[case_id]["terminal_state"]) - def test_ui_metadata_matches_skill(self): + def test_runtime_adapters_exist_for_both_products(self): metadata = read(SKILL_ROOT / "agents" / "openai.yaml") self.assertIn('display_name: "Implement Ticket"', metadata) - self.assertIn("$implement-ticket", metadata) + self.assertIn( + "Claude Code adapter", read(SKILL_ROOT / "agents" / "claude-code.md") + ) if __name__ == "__main__": diff --git a/skills/review-code-change/SKILL.md b/skills/review-code-change/SKILL.md index 5107b6c..b9401c7 100644 --- a/skills/review-code-change/SKILL.md +++ b/skills/review-code-change/SKILL.md @@ -1,6 +1,7 @@ --- name: review-code-change -description: Run the complete repository-owned review of a PR, branch, patch, or change set. Use when one entrypoint should build a trustworthy evidence packet, invoke solution simplicity, correctness, and code simplicity in order, reconcile their results, and return one bounded aggregate verdict. Fail closed when evidence or local lens skills are missing, remain read-only, and never depend on a third-party review skill. +description: Review a code change, diff, PR, branch, or patch with the complete repository-owned review suite. Use when asked to review a change or run code review; builds a trustworthy evidence packet, invokes solution simplicity, correctness, and code simplicity in order, reconciles their results, and returns one bounded aggregate verdict. Fails closed when evidence or local lens skills are missing, remains read-only, and never depends on a third-party review skill. +allowed-tools: Read, Grep, Glob, Bash, Agent, Task, Skill --- # Review Code Change @@ -10,7 +11,11 @@ the repository-owned lenses; do not reproduce their rubrics. ## Load the contracts and dependencies -1. Read `../../review-suite/CONTRACT.md` and both shared schemas. +1. Read the bundled canonical review contract at + [references/review-suite/CONTRACT.md](references/review-suite/CONTRACT.md) + and both shared schemas beside it. Inside this skill's source monorepo, the + repository-root `review-suite/` directory is the canonical origin and the + bundled copies are kept byte-identical to it. 2. Read [the orchestration protocol](references/orchestration-protocol.md). 3. Verify that `review-solution-simplicity`, `review-correctness`, and `review-code-simplicity` are available and readable. @@ -97,5 +102,9 @@ the shared risk-based merge-candidate rules. Do not edit or format reviewed files, apply fixes, create candidate artifacts, commit, push, post reviews, resolve threads, approve, merge, or update tickets. -Run only safe read-only inspection and validation commands. Verify that the -candidate state is unchanged before returning. +Run only safe read-only inspection and validation commands. Runtimes that +support tool restriction should enforce the `allowed-tools` frontmatter, which +excludes file-editing tools. The shell remains necessary for validation commands +and can still mutate files, so prefer a sandboxed or deny-write shell where +available; the recorded before/after candidate state is the authoritative +integrity check. Verify that the candidate state is unchanged before returning. diff --git a/skills/review-code-change/agents/claude-code.md b/skills/review-code-change/agents/claude-code.md new file mode 100644 index 0000000..58bbaaa --- /dev/null +++ b/skills/review-code-change/agents/claude-code.md @@ -0,0 +1,17 @@ +# Claude Code adapter + +Optional discovery metadata for Claude Code and Claude Agent SDK runtimes. It +does not constrain the skill's portable contract. + +- Display name: Review Code Change. +- Suggested prompt: "Use the review-code-change skill to build one evidence + packet and run the complete repository-owned review sequence for this change." +- Read-only enforcement: this orchestrator declares + `allowed-tools: Read, Grep, Glob, Bash, Agent, Task, Skill` — no file-editing + tools, plus the subagent and skill tools it needs to delegate. Each lens skill + declares `allowed-tools: Read, Grep, Glob, Bash`; run every lens in a context + that honors that restriction, such as a subagent without file-editing tools. +- Lens invocation: run each lens skill sequentially in a fresh subagent seeded + only with the validated packet; never share the orchestrator's working notes + or a prior lens's raw reasoning. When the runtime cannot spawn subagents, the + caller must provision one fresh read-only context per lens instead. diff --git a/skills/review-code-change/evals/README.md b/skills/review-code-change/evals/README.md new file mode 100644 index 0000000..39f4170 --- /dev/null +++ b/skills/review-code-change/evals/README.md @@ -0,0 +1,8 @@ +# Review-code-change evaluations + +- `cases.json` + `expectations.json`: orchestration scenario contract data + consumed by `scripts/tests/test_orchestration_contract.py`. +- `standalone-clean/`: raw forward-evaluation inputs for a full suite run. The + expected outcome lives in `expected/standalone-clean.result.json`, outside the + input directory, so a forward-testing reviewer pointed at the input directory + cannot read the answer key. diff --git a/skills/review-code-change/evals/results.json b/skills/review-code-change/evals/expectations.json similarity index 100% rename from skills/review-code-change/evals/results.json rename to skills/review-code-change/evals/expectations.json diff --git a/skills/review-code-change/evals/standalone-clean/result.json b/skills/review-code-change/evals/expected/standalone-clean.result.json similarity index 82% rename from skills/review-code-change/evals/standalone-clean/result.json rename to skills/review-code-change/evals/expected/standalone-clean.result.json index 7211721..ebca30f 100644 --- a/skills/review-code-change/evals/standalone-clean/result.json +++ b/skills/review-code-change/evals/expected/standalone-clean.result.json @@ -1,8 +1,8 @@ { "observed_sequence": [ - "review-solution-simplicity", - "review-correctness", - "review-code-simplicity" + "solution_simplicity", + "correctness", + "code_simplicity" ], "result": { "schema_version": "1.0", diff --git a/skills/review-code-change/evals/standalone-clean/prompt.md b/skills/review-code-change/evals/standalone-clean/prompt.md index ab2e650..7507d27 100644 --- a/skills/review-code-change/evals/standalone-clean/prompt.md +++ b/skills/review-code-change/evals/standalone-clean/prompt.md @@ -1,10 +1,11 @@ Use the `review-code-change` skill to review the candidate described by the raw evidence in this directory. Read `ticket.md`, `repository-evidence.md`, -`candidate.diff`, and `validation.md`; do not inspect `result.json`, answer -keys, or prior conclusions. Build one shared packet, then apply the three -repository-owned lens skills in their required order. Do not modify files or -repository state. +`candidate.diff`, and `validation.md`; do not inspect answer keys (stored +outside this directory) or prior conclusions. Build one shared packet, then +apply the three repository-owned lens skills in their required order. Do not +modify files or repository state. -For evaluation recording only, return an object with `observed_sequence` and +For evaluation recording only, return an object with `observed_sequence` (the +lens tokens actually invoked, in order, e.g. `solution_simplicity`) and `result`; `result` must be the production aggregate JSON conforming to the shared result schema. diff --git a/skills/review-code-change/references/orchestration-protocol.md b/skills/review-code-change/references/orchestration-protocol.md index caae537..ac4615f 100644 --- a/skills/review-code-change/references/orchestration-protocol.md +++ b/skills/review-code-change/references/orchestration-protocol.md @@ -11,8 +11,11 @@ Read applicable repository instructions and named specifications, then capture the complete diff, exact candidate identity, representative nearby code and tests, validation evidence, and worktree state. -Run the shared packet validator. Do not invoke a lens with a malformed packet. -Do not repair missing intent by reading the implementation transcript. +Run the shared packet validator — the bundled dependency-free copy at +[references/review-suite/validate.py](review-suite/validate.py), or the +canonical `review-suite/scripts/validate.py` inside the source monorepo. Do not +invoke a lens with a malformed packet. Do not repair missing intent by reading +the implementation transcript. ## Sequence decisions diff --git a/skills/review-code-change/references/review-suite/CONTRACT.md b/skills/review-code-change/references/review-suite/CONTRACT.md new file mode 100644 index 0000000..412a672 --- /dev/null +++ b/skills/review-code-change/references/review-suite/CONTRACT.md @@ -0,0 +1,143 @@ +# 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. + +Because a skill folder is the unit of distribution, each review skill bundles a +verbatim copy of this document, both schemas, and the dependency-free +`validate.py` under its own `references/review-suite/` directory so the skill +works — including packet and result validation — when installed outside this +repository. The copies are mechanical mirrors, not forks: edit only the +canonical files here, refresh the copies with `just sync-contracts`, and rely on +the bundled-contract test to fail on any drift. References in this document to +`scripts/validate.py` and `fixtures/` describe this canonical directory; in an +installed skill, use the bundled `validate.py` beside this file, and expect no +fixtures. + +## 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. + +## Simplification proposal dispositions + +When an orchestrator asks correctness to assess a validated simplification +result, supply that result beside the unchanged review packet. Do not add review +conclusions to the packet itself. Correctness returns one +`proposal_dispositions` item for every supplied gating proposal: + +- `compatible` means the proposal preserves demonstrated correctness and remains + actionable; and +- `unsafe` means concrete correctness or repository evidence invalidates the + proposal even though the current candidate may already be correct. + +Each disposition identifies the source finding and lens and cites concrete +evidence. It does not describe a candidate defect and therefore does not change +the correctness verdict by itself. If correctness cannot assess a supplied +proposal trustworthily, return `blocked`. Only `correctness` and `aggregate` +results may contain proposal dispositions. + +## Candidate identity and base drift + +Bind every result to the packet's captured head and comparison base. Any edit, +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/skills/review-code-change/references/review-suite/review-packet.schema.json b/skills/review-code-change/references/review-suite/review-packet.schema.json new file mode 100644 index 0000000..9dd0af7 --- /dev/null +++ b/skills/review-code-change/references/review-suite/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/skills/review-code-change/references/review-suite/review-result.schema.json b/skills/review-code-change/references/review-suite/review-result.schema.json new file mode 100644 index 0000000..7dab07c --- /dev/null +++ b/skills/review-code-change/references/review-suite/review-result.schema.json @@ -0,0 +1,130 @@ +{ + "$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} + }, + "proposal_dispositions": { + "type": "array", + "items": { + "type": "object", + "additionalProperties": false, + "required": [ + "finding_id", + "source_lens", + "disposition", + "reason", + "evidence" + ], + "properties": { + "finding_id": { + "type": "string", + "pattern": "^[a-z0-9][a-z0-9._-]*$" + }, + "source_lens": { + "enum": ["solution_simplicity", "code_simplicity"] + }, + "disposition": {"enum": ["compatible", "unsafe"]}, + "reason": {"type": "string", "minLength": 1}, + "evidence": { + "type": "array", + "minItems": 1, + "items": { + "type": "object", + "additionalProperties": false, + "required": ["location", "detail"], + "properties": { + "location": {"type": "string", "minLength": 1}, + "detail": {"type": "string", "minLength": 1} + } + } + } + } + } + }, + "next_action": {"type": "string", "minLength": 1} + } +} diff --git a/skills/review-code-change/references/review-suite/validate.py b/skills/review-code-change/references/review-suite/validate.py new file mode 100644 index 0000000..50e0b12 --- /dev/null +++ b/skills/review-code-change/references/review-suite/validate.py @@ -0,0 +1,305 @@ +#!/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 + +HERE = Path(__file__).resolve().parent + + +def _schema_file(name: str) -> Path: + """Locate a schema in either supported layout. + + Canonical layout: review-suite/scripts/validate.py with schemas under + review-suite/contracts/. Bundled layout (installed review skills): + references/review-suite/validate.py with the schemas beside it. + """ + for candidate in (HERE / name, HERE.parent / "contracts" / name): + if candidate.is_file(): + return candidate + raise FileNotFoundError( + f"Cannot locate {name} beside {HERE} or in {HERE.parent / 'contracts'}" + ) + + +SCHEMAS = { + "packet": _schema_file("review-packet.schema.json"), + "result": _schema_file("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: + const = schema["const"] + # `1 == True` in Python; a boolean constant must reject numeric 1/1.0. + if value != const or isinstance(const, bool) != isinstance(value, bool): + errors.append(f"{at}: expected constant {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)) + + dispositions = result.get("proposal_dispositions", []) + if dispositions and result.get("lens") not in {"correctness", "aggregate"}: + errors.append( + "$.proposal_dispositions: only correctness or aggregate results may " + "disposition simplification proposals" + ) + disposition_ids = [item.get("finding_id") for item in dispositions] + duplicate_dispositions = sorted( + {item for item in disposition_ids if disposition_ids.count(item) > 1} + ) + if duplicate_dispositions: + errors.append( + "$.proposal_dispositions: duplicate finding id(s): " + + ", ".join(duplicate_dispositions) + ) + return errors + + +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()) diff --git a/skills/review-code-change/scripts/tests/test_orchestration_contract.py b/skills/review-code-change/scripts/tests/test_orchestration_contract.py index 0bbca4c..bd06a56 100644 --- a/skills/review-code-change/scripts/tests/test_orchestration_contract.py +++ b/skills/review-code-change/scripts/tests/test_orchestration_contract.py @@ -8,8 +8,11 @@ SKILL_ROOT = Path(__file__).resolve().parents[2] REPOSITORY_ROOT = SKILL_ROOT.parents[1] REVIEW_SUITE = REPOSITORY_ROOT / "review-suite" +# Import the skill's own bundled validator so these tests exercise the +# installed layout, not only the canonical monorepo copy. SPEC = importlib.util.spec_from_file_location( - "review_contract_validator", REVIEW_SUITE / "scripts" / "validate.py" + "review_contract_validator", + SKILL_ROOT / "references" / "review-suite" / "validate.py", ) assert SPEC and SPEC.loader VALIDATOR = importlib.util.module_from_spec(SPEC) @@ -32,7 +35,7 @@ def setUpClass(cls): } cls.results = { record["case_id"]: record - for record in load(SKILL_ROOT / "evals" / "results.json") + for record in load(SKILL_ROOT / "evals" / "expectations.json") } def test_skill_uses_only_local_lenses_in_default_order(self): @@ -49,10 +52,25 @@ def test_skill_uses_only_local_lenses_in_default_order(self): ] self.assertEqual(sorted(positions), positions) self.assertNotIn("code-review-pro", self.skill + self.protocol) - self.assertIn("Build the shared packet once", self.skill) - self.assertIn("implementation transcripts", self.skill) - self.assertIn("at most three", self.skill) - self.assertIn("Do not edit", self.skill) + + def test_orchestrator_allowed_tools_can_delegate_but_not_edit(self): + # The orchestrator needs subagent/skill tools to run each lens in a + # fresh context, but must never hold file-editing tools. + self.assertIn( + "allowed-tools: Read, Grep, Glob, Bash, Agent, Task, Skill", + self.skill, + ) + + def test_skill_bundles_the_shared_contract(self): + self.assertIn("references/review-suite/CONTRACT.md", self.skill) + bundle = SKILL_ROOT / "references" / "review-suite" + for name in ( + "CONTRACT.md", + "review-packet.schema.json", + "review-result.schema.json", + "validate.py", + ): + self.assertTrue((bundle / name).is_file(), name) def test_forward_evaluations_cover_every_case_and_conform(self): self.assertEqual(set(self.cases), set(self.results)) @@ -182,16 +200,18 @@ def test_standalone_raw_evidence_builds_one_clean_ordered_review(self): "validation.md", ) ) - record = load(evaluation / "result.json") + record = load( + SKILL_ROOT / "evals" / "expected" / "standalone-clean.result.json" + ) + # The reviewer-visible input directory must not contain the answer key. + self.assertEqual( + [], [path for path in evaluation.glob("*result*") if path.is_file()] + ) self.assertNotIn("expected", prompt.lower()) self.assertNotIn("change_contract", evidence) self.assertEqual( - [ - "review-solution-simplicity", - "review-correctness", - "review-code-simplicity", - ], + ["solution_simplicity", "correctness", "code_simplicity"], record["observed_sequence"], ) self.assertEqual([], VALIDATOR.validate_result(record["result"])) diff --git a/skills/review-code-simplicity/SKILL.md b/skills/review-code-simplicity/SKILL.md index 0273460..f7ae33c 100644 --- a/skills/review-code-simplicity/SKILL.md +++ b/skills/review-code-simplicity/SKILL.md @@ -1,6 +1,7 @@ --- name: review-code-simplicity -description: Review a code change for local implementation complexity, meaningful DRY and reuse opportunities, unnecessary control flow, and bespoke code replaceable by established repository modules or justified dependencies. Use for implementation-level simplicity review of a PR, branch, patch, or tests, either from raw evidence or the shared review packet. Preserve the chosen architecture and behavior, return only the shared result shape, and never modify the candidate. +description: Review a code change, diff, PR, branch, patch, or tests for local implementation complexity, meaningful DRY and reuse opportunities, unnecessary control flow, and bespoke code replaceable by established repository modules or justified dependencies. Use when asked to simplify code or run an implementation-level simplicity review, either from raw evidence or the shared review packet. Preserves the chosen architecture and behavior, returns only the shared result shape, and never modifies the candidate. +allowed-tools: Read, Grep, Glob, Bash --- # Review Code Simplicity @@ -11,8 +12,11 @@ caller. ## Load the contracts -1. Read the canonical review contract at `../../review-suite/CONTRACT.md` and - its packet and result schemas. +1. Read the bundled canonical review contract at + [references/review-suite/CONTRACT.md](references/review-suite/CONTRACT.md) + and its packet and result schemas beside it. Inside this skill's source + monorepo, the repository-root `review-suite/` directory is the canonical + origin and the bundled copies are kept byte-identical to it. 2. Read [the code-simplicity rubric](references/code-simplicity-rubric.md). 3. Treat the canonical contract as authoritative for evidence, finding fields, severity, confidence, verdicts, candidate identity, and base drift. @@ -78,9 +82,9 @@ licensing costs. Do not add a dependency for trivial behavior or future use. ## Return the shared result -Return only JSON conforming to -`../../review-suite/contracts/review-result.schema.json` with lens -`code_simplicity`. +Return only JSON conforming to the bundled +[review-result schema](references/review-suite/review-result.schema.json) with +lens `code_simplicity`. - Return `clean` when no material local simplification remains. - Return `changes_required` when a blocking or strong-recommendation finding @@ -92,5 +96,10 @@ Return only JSON conforming to Do not edit or format files, apply refactors, create repository artifacts, commit, push, resolve threads, post reviews, or update tickets. Run only safe -read-only inspection and validation commands. Preserve supplied pre-review -candidate state exactly and report unexpected mutation as an integrity failure. +read-only inspection and validation commands. Runtimes that support tool +restriction should enforce the `allowed-tools` frontmatter, which excludes +file-editing tools. The shell remains necessary for validation commands and can +still mutate files, so prefer a sandboxed or deny-write shell where available; +the recorded before/after candidate state is the authoritative integrity check. +Preserve supplied pre-review candidate state exactly and report unexpected +mutation as an integrity failure. diff --git a/skills/review-code-simplicity/agents/claude-code.md b/skills/review-code-simplicity/agents/claude-code.md new file mode 100644 index 0000000..e34d90a --- /dev/null +++ b/skills/review-code-simplicity/agents/claude-code.md @@ -0,0 +1,11 @@ +# Claude Code adapter + +Optional discovery metadata for Claude Code and Claude Agent SDK runtimes. It +does not constrain the skill's portable contract. + +- Display name: Review Code Simplicity. +- Suggested prompt: "Use the review-code-simplicity skill to find concrete + behavior-preserving reuse and DRY improvements in this change." +- Read-only enforcement: this skill declares + `allowed-tools: Read, Grep, Glob, Bash`; run it in a context that honors that + restriction, such as a subagent without file-editing tools. diff --git a/skills/review-code-simplicity/evals/README.md b/skills/review-code-simplicity/evals/README.md new file mode 100644 index 0000000..2d85f58 --- /dev/null +++ b/skills/review-code-simplicity/evals/README.md @@ -0,0 +1,6 @@ +# review-code-simplicity evaluations + +`standalone-duplicated-policy/` holds raw forward-evaluation inputs. The +expected outcome lives in `expected/standalone-duplicated-policy.result.json`, +outside the input directory, so a forward-testing reviewer pointed at the input +directory cannot read the answer key. diff --git a/skills/review-code-simplicity/evals/standalone-duplicated-policy/result.json b/skills/review-code-simplicity/evals/expected/standalone-duplicated-policy.result.json similarity index 100% rename from skills/review-code-simplicity/evals/standalone-duplicated-policy/result.json rename to skills/review-code-simplicity/evals/expected/standalone-duplicated-policy.result.json diff --git a/skills/review-code-simplicity/evals/standalone-duplicated-policy/prompt.md b/skills/review-code-simplicity/evals/standalone-duplicated-policy/prompt.md index a13bdab..3a30f8e 100644 --- a/skills/review-code-simplicity/evals/standalone-duplicated-policy/prompt.md +++ b/skills/review-code-simplicity/evals/standalone-duplicated-policy/prompt.md @@ -1,6 +1,6 @@ Use the `review-code-simplicity` 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 a -prior conclusion. Reconstruct the change contract before inspecting local reuse. -Return only the shared review result JSON and do not modify files or repository -state. +`candidate.diff`, and `validation.md`; do not inspect answer keys (stored +outside this directory) or use a prior conclusion. Reconstruct the change +contract before inspecting local reuse. Return only the shared review result +JSON and do not modify files or repository state. diff --git a/skills/review-code-simplicity/references/review-suite/CONTRACT.md b/skills/review-code-simplicity/references/review-suite/CONTRACT.md new file mode 100644 index 0000000..412a672 --- /dev/null +++ b/skills/review-code-simplicity/references/review-suite/CONTRACT.md @@ -0,0 +1,143 @@ +# 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. + +Because a skill folder is the unit of distribution, each review skill bundles a +verbatim copy of this document, both schemas, and the dependency-free +`validate.py` under its own `references/review-suite/` directory so the skill +works — including packet and result validation — when installed outside this +repository. The copies are mechanical mirrors, not forks: edit only the +canonical files here, refresh the copies with `just sync-contracts`, and rely on +the bundled-contract test to fail on any drift. References in this document to +`scripts/validate.py` and `fixtures/` describe this canonical directory; in an +installed skill, use the bundled `validate.py` beside this file, and expect no +fixtures. + +## 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. + +## Simplification proposal dispositions + +When an orchestrator asks correctness to assess a validated simplification +result, supply that result beside the unchanged review packet. Do not add review +conclusions to the packet itself. Correctness returns one +`proposal_dispositions` item for every supplied gating proposal: + +- `compatible` means the proposal preserves demonstrated correctness and remains + actionable; and +- `unsafe` means concrete correctness or repository evidence invalidates the + proposal even though the current candidate may already be correct. + +Each disposition identifies the source finding and lens and cites concrete +evidence. It does not describe a candidate defect and therefore does not change +the correctness verdict by itself. If correctness cannot assess a supplied +proposal trustworthily, return `blocked`. Only `correctness` and `aggregate` +results may contain proposal dispositions. + +## Candidate identity and base drift + +Bind every result to the packet's captured head and comparison base. Any edit, +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/skills/review-code-simplicity/references/review-suite/review-packet.schema.json b/skills/review-code-simplicity/references/review-suite/review-packet.schema.json new file mode 100644 index 0000000..9dd0af7 --- /dev/null +++ b/skills/review-code-simplicity/references/review-suite/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/skills/review-code-simplicity/references/review-suite/review-result.schema.json b/skills/review-code-simplicity/references/review-suite/review-result.schema.json new file mode 100644 index 0000000..7dab07c --- /dev/null +++ b/skills/review-code-simplicity/references/review-suite/review-result.schema.json @@ -0,0 +1,130 @@ +{ + "$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} + }, + "proposal_dispositions": { + "type": "array", + "items": { + "type": "object", + "additionalProperties": false, + "required": [ + "finding_id", + "source_lens", + "disposition", + "reason", + "evidence" + ], + "properties": { + "finding_id": { + "type": "string", + "pattern": "^[a-z0-9][a-z0-9._-]*$" + }, + "source_lens": { + "enum": ["solution_simplicity", "code_simplicity"] + }, + "disposition": {"enum": ["compatible", "unsafe"]}, + "reason": {"type": "string", "minLength": 1}, + "evidence": { + "type": "array", + "minItems": 1, + "items": { + "type": "object", + "additionalProperties": false, + "required": ["location", "detail"], + "properties": { + "location": {"type": "string", "minLength": 1}, + "detail": {"type": "string", "minLength": 1} + } + } + } + } + } + }, + "next_action": {"type": "string", "minLength": 1} + } +} diff --git a/skills/review-code-simplicity/references/review-suite/validate.py b/skills/review-code-simplicity/references/review-suite/validate.py new file mode 100644 index 0000000..50e0b12 --- /dev/null +++ b/skills/review-code-simplicity/references/review-suite/validate.py @@ -0,0 +1,305 @@ +#!/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 + +HERE = Path(__file__).resolve().parent + + +def _schema_file(name: str) -> Path: + """Locate a schema in either supported layout. + + Canonical layout: review-suite/scripts/validate.py with schemas under + review-suite/contracts/. Bundled layout (installed review skills): + references/review-suite/validate.py with the schemas beside it. + """ + for candidate in (HERE / name, HERE.parent / "contracts" / name): + if candidate.is_file(): + return candidate + raise FileNotFoundError( + f"Cannot locate {name} beside {HERE} or in {HERE.parent / 'contracts'}" + ) + + +SCHEMAS = { + "packet": _schema_file("review-packet.schema.json"), + "result": _schema_file("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: + const = schema["const"] + # `1 == True` in Python; a boolean constant must reject numeric 1/1.0. + if value != const or isinstance(const, bool) != isinstance(value, bool): + errors.append(f"{at}: expected constant {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)) + + dispositions = result.get("proposal_dispositions", []) + if dispositions and result.get("lens") not in {"correctness", "aggregate"}: + errors.append( + "$.proposal_dispositions: only correctness or aggregate results may " + "disposition simplification proposals" + ) + disposition_ids = [item.get("finding_id") for item in dispositions] + duplicate_dispositions = sorted( + {item for item in disposition_ids if disposition_ids.count(item) > 1} + ) + if duplicate_dispositions: + errors.append( + "$.proposal_dispositions: duplicate finding id(s): " + + ", ".join(duplicate_dispositions) + ) + return errors + + +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()) diff --git a/skills/review-code-simplicity/scripts/tests/test_skill_contract.py b/skills/review-code-simplicity/scripts/tests/test_skill_contract.py index 8cfe364..25dbead 100644 --- a/skills/review-code-simplicity/scripts/tests/test_skill_contract.py +++ b/skills/review-code-simplicity/scripts/tests/test_skill_contract.py @@ -8,8 +8,11 @@ SKILL_ROOT = Path(__file__).resolve().parents[2] REPOSITORY_ROOT = SKILL_ROOT.parents[1] REVIEW_SUITE = REPOSITORY_ROOT / "review-suite" +# Import the skill's own bundled validator so these tests exercise the +# installed layout, not only the canonical monorepo copy. SPEC = importlib.util.spec_from_file_location( - "review_contract_validator", REVIEW_SUITE / "scripts" / "validate.py" + "review_contract_validator", + SKILL_ROOT / "references" / "review-suite" / "validate.py", ) assert SPEC and SPEC.loader VALIDATOR = importlib.util.module_from_spec(SPEC) @@ -23,7 +26,16 @@ def load(path: Path): 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("references/review-suite/CONTRACT.md", skill) + self.assertIn("allowed-tools: Read, Grep, Glob, Bash", skill) + bundle = SKILL_ROOT / "references" / "review-suite" + for name in ( + "CONTRACT.md", + "review-packet.schema.json", + "review-result.schema.json", + "validate.py", + ): + self.assertTrue((bundle / name).is_file(), name) self.assertIn("Preserve read-only integrity", skill) self.assertIn("From raw evidence", skill) self.assertIn("already installed", skill) @@ -60,8 +72,17 @@ def test_standalone_eval_finds_repository_reuse_from_raw_evidence(self): "validation.md", ) ) - result = load(evaluation / "result.json") + result = load( + SKILL_ROOT + / "evals" + / "expected" + / "standalone-duplicated-policy.result.json" + ) + # The reviewer-visible input directory must not contain the answer key. + self.assertEqual( + [], [path for path in evaluation.glob("*result*") if path.is_file()] + ) self.assertNotIn("expected", prompt.lower()) self.assertNotIn("change_contract", evidence) self.assertEqual([], VALIDATOR.validate_result(result)) diff --git a/skills/review-correctness/SKILL.md b/skills/review-correctness/SKILL.md index a3a6e8e..2f21984 100644 --- a/skills/review-correctness/SKILL.md +++ b/skills/review-correctness/SKILL.md @@ -1,6 +1,7 @@ --- 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. +description: Review a code change, diff, PR, branch, or patch for bugs and material behavioral, security, authorization, compatibility, data-integrity, performance, and validation failures against its stated goal. Use when asked to find bugs or run a correctness-focused review, either from raw repository and ticket evidence or from the repository-owned shared review packet. Returns only the shared finding and verdict shape and never modifies the reviewed candidate. +allowed-tools: Read, Grep, Glob, Bash --- # Review Correctness @@ -11,8 +12,11 @@ to the caller. ## Load the contracts -1. Read the canonical review contract at `../../review-suite/CONTRACT.md` and - its packet and result schemas. +1. Read the bundled canonical review contract at + [references/review-suite/CONTRACT.md](references/review-suite/CONTRACT.md) + and its packet and result schemas beside it. Inside this skill's source + monorepo, the repository-root `review-suite/` directory is the canonical + origin and the bundled copies are kept byte-identical to it. 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. @@ -89,9 +93,9 @@ proposal cannot be assessed from trustworthy evidence, return `blocked`. ## Return the shared result -Return only JSON conforming to -`../../review-suite/contracts/review-result.schema.json` with lens -`correctness`. +Return only JSON conforming to the bundled +[review-result schema](references/review-suite/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. @@ -107,5 +111,10 @@ Return only JSON conforming to 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. +validation commands. Runtimes that support tool restriction should enforce the +`allowed-tools` frontmatter, which excludes file-editing tools. The shell +remains necessary for validation commands and can still mutate files, so prefer +a sandboxed or deny-write shell where available; the recorded before/after +candidate state is the authoritative integrity check. 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/claude-code.md b/skills/review-correctness/agents/claude-code.md new file mode 100644 index 0000000..9fce09d --- /dev/null +++ b/skills/review-correctness/agents/claude-code.md @@ -0,0 +1,11 @@ +# Claude Code adapter + +Optional discovery metadata for Claude Code and Claude Agent SDK runtimes. It +does not constrain the skill's portable contract. + +- Display name: Review Correctness. +- Suggested prompt: "Use the review-correctness skill to review this code change + for material correctness, security, compatibility, and validation failures." +- Read-only enforcement: this skill declares + `allowed-tools: Read, Grep, Glob, Bash`; run it in a context that honors that + restriction, such as a subagent without file-editing tools. diff --git a/skills/review-correctness/evals/README.md b/skills/review-correctness/evals/README.md new file mode 100644 index 0000000..7ceed8f --- /dev/null +++ b/skills/review-correctness/evals/README.md @@ -0,0 +1,6 @@ +# review-correctness evaluations + +`standalone-ticket-regression/` holds raw forward-evaluation inputs. The +expected outcome lives in `expected/standalone-ticket-regression.result.json`, +outside the input directory, so a forward-testing reviewer pointed at the input +directory cannot read the answer key. diff --git a/skills/review-correctness/evals/standalone-ticket-regression/result.json b/skills/review-correctness/evals/expected/standalone-ticket-regression.result.json similarity index 100% rename from skills/review-correctness/evals/standalone-ticket-regression/result.json rename to skills/review-correctness/evals/expected/standalone-ticket-regression.result.json diff --git a/skills/review-correctness/evals/standalone-ticket-regression/prompt.md b/skills/review-correctness/evals/standalone-ticket-regression/prompt.md index f4fba34..e89a37f 100644 --- a/skills/review-correctness/evals/standalone-ticket-regression/prompt.md +++ b/skills/review-correctness/evals/standalone-ticket-regression/prompt.md @@ -1,6 +1,7 @@ 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. +`candidate.diff`, and `validation.md`; do not inspect answer keys (stored +outside this directory) 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/references/correctness-rubric.md b/skills/review-correctness/references/correctness-rubric.md index b878c62..3aa2db7 100644 --- a/skills/review-correctness/references/correctness-rubric.md +++ b/skills/review-correctness/references/correctness-rubric.md @@ -36,8 +36,9 @@ Do not request generalized hardening without a reachable current risk. ## Repository architecture and compatibility -- Read applicable `AGENTS.md`, contributor guidance, named designs, and public - contracts before judging architecture or idioms. +- Read applicable agent instructions (`AGENTS.md`, `CLAUDE.md`, or equivalent), + 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 diff --git a/skills/review-correctness/references/review-suite/CONTRACT.md b/skills/review-correctness/references/review-suite/CONTRACT.md new file mode 100644 index 0000000..412a672 --- /dev/null +++ b/skills/review-correctness/references/review-suite/CONTRACT.md @@ -0,0 +1,143 @@ +# 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. + +Because a skill folder is the unit of distribution, each review skill bundles a +verbatim copy of this document, both schemas, and the dependency-free +`validate.py` under its own `references/review-suite/` directory so the skill +works — including packet and result validation — when installed outside this +repository. The copies are mechanical mirrors, not forks: edit only the +canonical files here, refresh the copies with `just sync-contracts`, and rely on +the bundled-contract test to fail on any drift. References in this document to +`scripts/validate.py` and `fixtures/` describe this canonical directory; in an +installed skill, use the bundled `validate.py` beside this file, and expect no +fixtures. + +## 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. + +## Simplification proposal dispositions + +When an orchestrator asks correctness to assess a validated simplification +result, supply that result beside the unchanged review packet. Do not add review +conclusions to the packet itself. Correctness returns one +`proposal_dispositions` item for every supplied gating proposal: + +- `compatible` means the proposal preserves demonstrated correctness and remains + actionable; and +- `unsafe` means concrete correctness or repository evidence invalidates the + proposal even though the current candidate may already be correct. + +Each disposition identifies the source finding and lens and cites concrete +evidence. It does not describe a candidate defect and therefore does not change +the correctness verdict by itself. If correctness cannot assess a supplied +proposal trustworthily, return `blocked`. Only `correctness` and `aggregate` +results may contain proposal dispositions. + +## Candidate identity and base drift + +Bind every result to the packet's captured head and comparison base. Any edit, +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/skills/review-correctness/references/review-suite/review-packet.schema.json b/skills/review-correctness/references/review-suite/review-packet.schema.json new file mode 100644 index 0000000..9dd0af7 --- /dev/null +++ b/skills/review-correctness/references/review-suite/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/skills/review-correctness/references/review-suite/review-result.schema.json b/skills/review-correctness/references/review-suite/review-result.schema.json new file mode 100644 index 0000000..7dab07c --- /dev/null +++ b/skills/review-correctness/references/review-suite/review-result.schema.json @@ -0,0 +1,130 @@ +{ + "$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} + }, + "proposal_dispositions": { + "type": "array", + "items": { + "type": "object", + "additionalProperties": false, + "required": [ + "finding_id", + "source_lens", + "disposition", + "reason", + "evidence" + ], + "properties": { + "finding_id": { + "type": "string", + "pattern": "^[a-z0-9][a-z0-9._-]*$" + }, + "source_lens": { + "enum": ["solution_simplicity", "code_simplicity"] + }, + "disposition": {"enum": ["compatible", "unsafe"]}, + "reason": {"type": "string", "minLength": 1}, + "evidence": { + "type": "array", + "minItems": 1, + "items": { + "type": "object", + "additionalProperties": false, + "required": ["location", "detail"], + "properties": { + "location": {"type": "string", "minLength": 1}, + "detail": {"type": "string", "minLength": 1} + } + } + } + } + } + }, + "next_action": {"type": "string", "minLength": 1} + } +} diff --git a/skills/review-correctness/references/review-suite/validate.py b/skills/review-correctness/references/review-suite/validate.py new file mode 100644 index 0000000..50e0b12 --- /dev/null +++ b/skills/review-correctness/references/review-suite/validate.py @@ -0,0 +1,305 @@ +#!/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 + +HERE = Path(__file__).resolve().parent + + +def _schema_file(name: str) -> Path: + """Locate a schema in either supported layout. + + Canonical layout: review-suite/scripts/validate.py with schemas under + review-suite/contracts/. Bundled layout (installed review skills): + references/review-suite/validate.py with the schemas beside it. + """ + for candidate in (HERE / name, HERE.parent / "contracts" / name): + if candidate.is_file(): + return candidate + raise FileNotFoundError( + f"Cannot locate {name} beside {HERE} or in {HERE.parent / 'contracts'}" + ) + + +SCHEMAS = { + "packet": _schema_file("review-packet.schema.json"), + "result": _schema_file("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: + const = schema["const"] + # `1 == True` in Python; a boolean constant must reject numeric 1/1.0. + if value != const or isinstance(const, bool) != isinstance(value, bool): + errors.append(f"{at}: expected constant {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)) + + dispositions = result.get("proposal_dispositions", []) + if dispositions and result.get("lens") not in {"correctness", "aggregate"}: + errors.append( + "$.proposal_dispositions: only correctness or aggregate results may " + "disposition simplification proposals" + ) + disposition_ids = [item.get("finding_id") for item in dispositions] + duplicate_dispositions = sorted( + {item for item in disposition_ids if disposition_ids.count(item) > 1} + ) + if duplicate_dispositions: + errors.append( + "$.proposal_dispositions: duplicate finding id(s): " + + ", ".join(duplicate_dispositions) + ) + return errors + + +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()) diff --git a/skills/review-correctness/scripts/tests/test_skill_contract.py b/skills/review-correctness/scripts/tests/test_skill_contract.py index 2e2e8a1..a13b28f 100644 --- a/skills/review-correctness/scripts/tests/test_skill_contract.py +++ b/skills/review-correctness/scripts/tests/test_skill_contract.py @@ -8,8 +8,11 @@ SKILL_ROOT = Path(__file__).resolve().parents[2] REPOSITORY_ROOT = SKILL_ROOT.parents[1] REVIEW_SUITE = REPOSITORY_ROOT / "review-suite" +# Import the skill's own bundled validator so these tests exercise the +# installed layout, not only the canonical monorepo copy. SPEC = importlib.util.spec_from_file_location( - "review_contract_validator", REVIEW_SUITE / "scripts" / "validate.py" + "review_contract_validator", + SKILL_ROOT / "references" / "review-suite" / "validate.py", ) assert SPEC and SPEC.loader VALIDATOR = importlib.util.module_from_spec(SPEC) @@ -23,7 +26,16 @@ def load(path: Path): 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("references/review-suite/CONTRACT.md", skill) + self.assertIn("allowed-tools: Read, Grep, Glob, Bash", skill) + bundle = SKILL_ROOT / "references" / "review-suite" + for name in ( + "CONTRACT.md", + "review-packet.schema.json", + "review-result.schema.json", + "validate.py", + ): + self.assertTrue((bundle / name).is_file(), name) self.assertIn("Preserve read-only integrity", skill) self.assertIn("proposal_dispositions", skill) self.assertIn("Do not turn a rejected hypothetical edit", skill) @@ -57,8 +69,17 @@ def test_standalone_eval_uses_raw_evidence(self): "validation.md", ) ) - result = load(evaluation / "result.json") + result = load( + SKILL_ROOT + / "evals" + / "expected" + / "standalone-ticket-regression.result.json" + ) + # The reviewer-visible input directory must not contain the answer key. + self.assertEqual( + [], [path for path in evaluation.glob("*result*") if path.is_file()] + ) self.assertNotIn("expected", prompt.lower()) self.assertNotIn("change_contract", evidence) self.assertEqual([], VALIDATOR.validate_result(result)) diff --git a/skills/review-solution-simplicity/SKILL.md b/skills/review-solution-simplicity/SKILL.md index 22e0282..040447e 100644 --- a/skills/review-solution-simplicity/SKILL.md +++ b/skills/review-solution-simplicity/SKILL.md @@ -1,6 +1,7 @@ --- name: review-solution-simplicity -description: Review a code change for whole-solution over-engineering by mapping its major mechanisms to stated requirements and proposing a materially smaller requirement-complete design. Use for architecture-level simplicity review of a PR, branch, or patch, either from raw ticket and repository evidence or from the shared review packet. Preserve justified safety and operational complexity, return only the shared result shape, and never modify the candidate. +description: Review a code change, diff, PR, branch, or patch for whole-solution over-engineering by mapping its major mechanisms to stated requirements and proposing a materially smaller requirement-complete design. Use when asked whether a change is over-engineered or for an architecture-level simplicity review, either from raw ticket and repository evidence or from the shared review packet. Preserves justified safety and operational complexity, returns only the shared result shape, and never modifies the candidate. +allowed-tools: Read, Grep, Glob, Bash --- # Review Solution Simplicity @@ -11,8 +12,11 @@ caller. ## Load the contracts -1. Read the canonical review contract at `../../review-suite/CONTRACT.md` and - its packet and result schemas. +1. Read the bundled canonical review contract at + [references/review-suite/CONTRACT.md](references/review-suite/CONTRACT.md) + and its packet and result schemas beside it. Inside this skill's source + monorepo, the repository-root `review-suite/` directory is the canonical + origin and the bundled copies are kept byte-identical to it. 2. Read [the solution-simplicity rubric](references/solution-simplicity-rubric.md). 3. Treat the canonical contract as authoritative for evidence, finding fields, @@ -77,9 +81,9 @@ change. ## Return the shared result -Return only JSON conforming to -`../../review-suite/contracts/review-result.schema.json` with lens -`solution_simplicity`. +Return only JSON conforming to the bundled +[review-result schema](references/review-suite/review-result.schema.json) with +lens `solution_simplicity`. - Return `clean` when every major mechanism is justified and no gating finding remains. @@ -93,5 +97,10 @@ Return only JSON conforming to Do not edit or format files, apply the alternative, create repository artifacts, commit, push, resolve threads, post reviews, or update tickets. Run only safe -read-only inspection and validation commands. Preserve supplied pre-review -candidate state exactly and report unexpected mutation as an integrity failure. +read-only inspection and validation commands. Runtimes that support tool +restriction should enforce the `allowed-tools` frontmatter, which excludes +file-editing tools. The shell remains necessary for validation commands and can +still mutate files, so prefer a sandboxed or deny-write shell where available; +the recorded before/after candidate state is the authoritative integrity check. +Preserve supplied pre-review candidate state exactly and report unexpected +mutation as an integrity failure. diff --git a/skills/review-solution-simplicity/agents/claude-code.md b/skills/review-solution-simplicity/agents/claude-code.md new file mode 100644 index 0000000..a3783af --- /dev/null +++ b/skills/review-solution-simplicity/agents/claude-code.md @@ -0,0 +1,11 @@ +# Claude Code adapter + +Optional discovery metadata for Claude Code and Claude Agent SDK runtimes. It +does not constrain the skill's portable contract. + +- Display name: Review Solution Simplicity. +- Suggested prompt: "Use the review-solution-simplicity skill to find a + materially smaller requirement-complete design for this change." +- Read-only enforcement: this skill declares + `allowed-tools: Read, Grep, Glob, Bash`; run it in a context that honors that + restriction, such as a subagent without file-editing tools. diff --git a/skills/review-solution-simplicity/evals/README.md b/skills/review-solution-simplicity/evals/README.md new file mode 100644 index 0000000..d1f9284 --- /dev/null +++ b/skills/review-solution-simplicity/evals/README.md @@ -0,0 +1,6 @@ +# review-solution-simplicity evaluations + +`standalone-provider-framework/` holds raw forward-evaluation inputs. The +expected outcome lives in `expected/standalone-provider-framework.result.json`, +outside the input directory, so a forward-testing reviewer pointed at the input +directory cannot read the answer key. diff --git a/skills/review-solution-simplicity/evals/standalone-provider-framework/result.json b/skills/review-solution-simplicity/evals/expected/standalone-provider-framework.result.json similarity index 100% rename from skills/review-solution-simplicity/evals/standalone-provider-framework/result.json rename to skills/review-solution-simplicity/evals/expected/standalone-provider-framework.result.json diff --git a/skills/review-solution-simplicity/evals/standalone-provider-framework/prompt.md b/skills/review-solution-simplicity/evals/standalone-provider-framework/prompt.md index 8a19db3..f886018 100644 --- a/skills/review-solution-simplicity/evals/standalone-provider-framework/prompt.md +++ b/skills/review-solution-simplicity/evals/standalone-provider-framework/prompt.md @@ -1,6 +1,6 @@ Use the `review-solution-simplicity` 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 a -prior review conclusion. Reconstruct the observable change contract before -comparing the whole solution. Return only the shared review result JSON and do -not modify files or repository state. +`candidate.diff`, and `validation.md`; do not inspect answer keys (stored +outside this directory) or use a prior review conclusion. Reconstruct the +observable change contract before comparing the whole solution. Return only the +shared review result JSON and do not modify files or repository state. diff --git a/skills/review-solution-simplicity/references/review-suite/CONTRACT.md b/skills/review-solution-simplicity/references/review-suite/CONTRACT.md new file mode 100644 index 0000000..412a672 --- /dev/null +++ b/skills/review-solution-simplicity/references/review-suite/CONTRACT.md @@ -0,0 +1,143 @@ +# 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. + +Because a skill folder is the unit of distribution, each review skill bundles a +verbatim copy of this document, both schemas, and the dependency-free +`validate.py` under its own `references/review-suite/` directory so the skill +works — including packet and result validation — when installed outside this +repository. The copies are mechanical mirrors, not forks: edit only the +canonical files here, refresh the copies with `just sync-contracts`, and rely on +the bundled-contract test to fail on any drift. References in this document to +`scripts/validate.py` and `fixtures/` describe this canonical directory; in an +installed skill, use the bundled `validate.py` beside this file, and expect no +fixtures. + +## 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. + +## Simplification proposal dispositions + +When an orchestrator asks correctness to assess a validated simplification +result, supply that result beside the unchanged review packet. Do not add review +conclusions to the packet itself. Correctness returns one +`proposal_dispositions` item for every supplied gating proposal: + +- `compatible` means the proposal preserves demonstrated correctness and remains + actionable; and +- `unsafe` means concrete correctness or repository evidence invalidates the + proposal even though the current candidate may already be correct. + +Each disposition identifies the source finding and lens and cites concrete +evidence. It does not describe a candidate defect and therefore does not change +the correctness verdict by itself. If correctness cannot assess a supplied +proposal trustworthily, return `blocked`. Only `correctness` and `aggregate` +results may contain proposal dispositions. + +## Candidate identity and base drift + +Bind every result to the packet's captured head and comparison base. Any edit, +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/skills/review-solution-simplicity/references/review-suite/review-packet.schema.json b/skills/review-solution-simplicity/references/review-suite/review-packet.schema.json new file mode 100644 index 0000000..9dd0af7 --- /dev/null +++ b/skills/review-solution-simplicity/references/review-suite/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/skills/review-solution-simplicity/references/review-suite/review-result.schema.json b/skills/review-solution-simplicity/references/review-suite/review-result.schema.json new file mode 100644 index 0000000..7dab07c --- /dev/null +++ b/skills/review-solution-simplicity/references/review-suite/review-result.schema.json @@ -0,0 +1,130 @@ +{ + "$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} + }, + "proposal_dispositions": { + "type": "array", + "items": { + "type": "object", + "additionalProperties": false, + "required": [ + "finding_id", + "source_lens", + "disposition", + "reason", + "evidence" + ], + "properties": { + "finding_id": { + "type": "string", + "pattern": "^[a-z0-9][a-z0-9._-]*$" + }, + "source_lens": { + "enum": ["solution_simplicity", "code_simplicity"] + }, + "disposition": {"enum": ["compatible", "unsafe"]}, + "reason": {"type": "string", "minLength": 1}, + "evidence": { + "type": "array", + "minItems": 1, + "items": { + "type": "object", + "additionalProperties": false, + "required": ["location", "detail"], + "properties": { + "location": {"type": "string", "minLength": 1}, + "detail": {"type": "string", "minLength": 1} + } + } + } + } + } + }, + "next_action": {"type": "string", "minLength": 1} + } +} diff --git a/skills/review-solution-simplicity/references/review-suite/validate.py b/skills/review-solution-simplicity/references/review-suite/validate.py new file mode 100644 index 0000000..50e0b12 --- /dev/null +++ b/skills/review-solution-simplicity/references/review-suite/validate.py @@ -0,0 +1,305 @@ +#!/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 + +HERE = Path(__file__).resolve().parent + + +def _schema_file(name: str) -> Path: + """Locate a schema in either supported layout. + + Canonical layout: review-suite/scripts/validate.py with schemas under + review-suite/contracts/. Bundled layout (installed review skills): + references/review-suite/validate.py with the schemas beside it. + """ + for candidate in (HERE / name, HERE.parent / "contracts" / name): + if candidate.is_file(): + return candidate + raise FileNotFoundError( + f"Cannot locate {name} beside {HERE} or in {HERE.parent / 'contracts'}" + ) + + +SCHEMAS = { + "packet": _schema_file("review-packet.schema.json"), + "result": _schema_file("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: + const = schema["const"] + # `1 == True` in Python; a boolean constant must reject numeric 1/1.0. + if value != const or isinstance(const, bool) != isinstance(value, bool): + errors.append(f"{at}: expected constant {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)) + + dispositions = result.get("proposal_dispositions", []) + if dispositions and result.get("lens") not in {"correctness", "aggregate"}: + errors.append( + "$.proposal_dispositions: only correctness or aggregate results may " + "disposition simplification proposals" + ) + disposition_ids = [item.get("finding_id") for item in dispositions] + duplicate_dispositions = sorted( + {item for item in disposition_ids if disposition_ids.count(item) > 1} + ) + if duplicate_dispositions: + errors.append( + "$.proposal_dispositions: duplicate finding id(s): " + + ", ".join(duplicate_dispositions) + ) + return errors + + +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()) diff --git a/skills/review-solution-simplicity/scripts/tests/test_skill_contract.py b/skills/review-solution-simplicity/scripts/tests/test_skill_contract.py index a455de2..c8b3717 100644 --- a/skills/review-solution-simplicity/scripts/tests/test_skill_contract.py +++ b/skills/review-solution-simplicity/scripts/tests/test_skill_contract.py @@ -8,8 +8,11 @@ SKILL_ROOT = Path(__file__).resolve().parents[2] REPOSITORY_ROOT = SKILL_ROOT.parents[1] REVIEW_SUITE = REPOSITORY_ROOT / "review-suite" +# Import the skill's own bundled validator so these tests exercise the +# installed layout, not only the canonical monorepo copy. SPEC = importlib.util.spec_from_file_location( - "review_contract_validator", REVIEW_SUITE / "scripts" / "validate.py" + "review_contract_validator", + SKILL_ROOT / "references" / "review-suite" / "validate.py", ) assert SPEC and SPEC.loader VALIDATOR = importlib.util.module_from_spec(SPEC) @@ -23,7 +26,16 @@ def load(path: Path): 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("references/review-suite/CONTRACT.md", skill) + self.assertIn("allowed-tools: Read, Grep, Glob, Bash", skill) + bundle = SKILL_ROOT / "references" / "review-suite" + for name in ( + "CONTRACT.md", + "review-packet.schema.json", + "review-result.schema.json", + "validate.py", + ): + self.assertTrue((bundle / name).is_file(), name) self.assertIn("Preserve read-only integrity", skill) self.assertIn("From raw evidence", skill) self.assertNotIn("code-review-pro", skill) @@ -56,8 +68,17 @@ def test_standalone_eval_reconstructs_contract_from_raw_evidence(self): "validation.md", ) ) - result = load(evaluation / "result.json") + result = load( + SKILL_ROOT + / "evals" + / "expected" + / "standalone-provider-framework.result.json" + ) + # The reviewer-visible input directory must not contain the answer key. + self.assertEqual( + [], [path for path in evaluation.glob("*result*") if path.is_file()] + ) self.assertNotIn("expected", prompt.lower()) self.assertNotIn("change_contract", evidence) self.assertEqual([], VALIDATOR.validate_result(result))