diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json index 49a38d9..d310896 100644 --- a/.claude-plugin/marketplace.json +++ b/.claude-plugin/marketplace.json @@ -8,7 +8,7 @@ "name": "code-review-ad", "source": "./", "description": "code-review-ad", - "version": "1.0.4" + "version": "1.0.5" } ] } diff --git a/.claude-plugin/plugin.json b/.claude-plugin/plugin.json index 9ecb944..9f3273f 100644 --- a/.claude-plugin/plugin.json +++ b/.claude-plugin/plugin.json @@ -1,6 +1,6 @@ { "name": "code-review-ad", - "version": "1.0.4", + "version": "1.0.5", "description": "Independent Toolkit, Standards, Spec, and conditional Performance review.", "author": { "name": "Dmitry Kropachev" diff --git a/.codex-plugin/plugin.json b/.codex-plugin/plugin.json index c46878a..438a3a5 100644 --- a/.codex-plugin/plugin.json +++ b/.codex-plugin/plugin.json @@ -1,6 +1,6 @@ { "name": "code-review-ad", - "version": "1.0.4", + "version": "1.0.5", "description": "Independent Toolkit, Standards, Spec, and conditional Performance review.", "author": { "name": "Dmitry Kropachev" diff --git a/PROVENANCE.json b/PROVENANCE.json index bfb512a..c791556 100644 --- a/PROVENANCE.json +++ b/PROVENANCE.json @@ -22,6 +22,16 @@ "claude/skills/code-review-ad/SKILL.md" ], "description": "Shared evidence criteria, broad inspection, truncation recovery and explicit candidate reconciliation; native host methods retained. Model and reasoning-effort configuration belongs to the host or runner for both skills." + }, + { + "version": "1.0.5", + "paths": [ + "common/codex/scripts/aspect-driver.py", + "common/codex/review.md", + "codex/skills/code-review-ad/SKILL.md", + "tests/test_aspect_driver.py" + ], + "description": "Experimental Codex aspect-first inspection followed by driver-issued assessment and structured finding collection; read-only source validation and existing parent reconciliation retained." } ] } diff --git a/README.md b/README.md index 7ba93e1..33f671c 100644 --- a/README.md +++ b/README.md @@ -1,6 +1,6 @@ # code-review-ad -Independent MIT plugin for Claude Code and Codex. Version 1.0.4. +Independent MIT plugin for Claude Code and Codex. Version 1.0.5. ## Install @@ -59,4 +59,6 @@ Invoke `code-review-ad` with a PR number or URL. Review stays in the current hos Both hosts use shared evidence criteria for runtime/security, test/build, diagnostic, documentation, rule, maintenance, and explicit acceptance findings. Reviews include concrete maintenance risks supported by named duplicated sites, state coupling, API obligations, or a supported failure mode. Reviewers inspect changed files broadly before deep verification and keep omitted or truncated evidence visible as coverage gaps. Ordinary reports remain concise; candidate ledgers are available for requested diagnostics. Directly related pre-existing issues carry explicit scope labels. +The experimental Codex path starts native reviewers with factual code-aspect inspection. Its read-only `common/codex/scripts/aspect-driver.py` then asks whether those observations reveal problems and collects cited findings before the existing parent verification. Claude retains its current method. + Model and reasoning-effort settings are controlled by the host or runner. Neither host skill sets reviewer overrides. diff --git a/codex/skills/code-review-ad/SKILL.md b/codex/skills/code-review-ad/SKILL.md index 2d18249..de81ad1 100644 --- a/codex/skills/code-review-ad/SKILL.md +++ b/codex/skills/code-review-ad/SKILL.md @@ -9,4 +9,6 @@ Model and reasoning-effort settings belong to the host or runner. Do not set rev Before work, read [the lifecycle procedure](../../../common/lifecycle.md), then run its entry command with `--host codex`. Retain its returned token and follow its structured action. Always run its completion command with that token in a finalization step, including on failure or interruption. +Native reviewers first describe code aspects, then run the bundled driver to assess those observations and collect any supported findings. The shared procedure defines the staged handoff. + Read [the shared procedure](../../../common/codex/procedure.md) and perform the requested workflow. Resolve paths beginning with `PLUGIN_ROOT/` against this plugin's root. Preserve the shared procedure's validation and publication guards. diff --git a/common/codex/review.md b/common/codex/review.md index b89897a..31e4c8b 100644 --- a/common/codex/review.md +++ b/common/codex/review.md @@ -89,34 +89,42 @@ reference is not evidence that its checks passed. Verify each applicable asserti Launch the three core native Codex agents, named `toolkit`, `standards`, and `spec`, in parallel. Launch a fourth, independent Performance subagent only when the user explicitly requests performance review, or both the PR changes a hot path and repository guidance or existing performance tests/benchmarks show that performance matters here. Assess that condition from the frozen diff and repository evidence before launching agents; do not infer it from a generic possibility of slowdown. If launched, name it `performance` and give it the same constraints, pins, rules and PR metadata, with only performance-relevant source facts. -Start each reviewer with a fresh context (`fork_turns: "none"`). Give every pass the same frozen +Start each reviewer with a fresh context (`fork_turns: "none"`). Give each the same frozen PR description/spec, pins, changed-file list, commit metadata, claim inventory, applicable -repository/user rules, and exact diff command. Supply absolute paths to this procedure, its -pass brief, and the selected check references. Do not copy this procedure or the references -again into the handoff; the agent reads the applicable files in focused instruction reads, -separate from patch output. Include the shared reporting and inspection paths in every handoff. -Do not pass parent hypotheses, other reviewers' findings, or the parent conversation. - -- Toolkit reads [its brief](passes/toolkit.md). -- Standards reads [its brief](passes/standards.md). -- Spec reads [its brief](passes/spec.md). -- When selected, Performance reads [its brief](passes/performance.md). - -Each prompt must explicitly forbid edits, formatters, posting, model subprocesses, further -agents, and Git checkout/switch/reset/stash/worktree. Keep those prohibitions in prompt text, -not shell commands. Read the assigned briefs/checks separately from the pinned diff. -Use focused file or hunk reads, checking outer and nested output for omitted or truncated -ranges and retrieving them before marking coverage complete. Inspect every changed file -and relevant metadata broadly before deep candidate verification, without repeating a full scan. Unfinished checks -are named coverage gaps, not grounds to claim no findings. Do not run -tests, builds, linters, or formatters. Return findings promptly. - -For each candidate, return its issue category, exact claim, changed line, source-backed trigger and consequence, -base/head distinction, and relevant contrary evidence considered. Include the required state, -transition, interleaving, and numerical tables from selected checks. Add a compact coverage -note for each applicable requested check: source evidence plus finding reference, supported -no-finding conclusion, or unexamined gap. Include test assertions/failure cleanup and commit -requirements. Unexamined means a gap, never no finding. Do not drop low-severity verified facts. +repository/user rules, and exact diff command. Do not pass parent hypotheses, other agents' +findings, or the parent conversation. + +Use the bundled [aspect driver](scripts/aspect-driver.py) in each native agent's own session. +The initial task is to look at the assigned aspects and describe the implementation. Give the +agent the driver command and the inspection boundary, without asking it to find bugs or sending +this review procedure, reporting criteria, pass brief, or check contents in that initial handoff. +Supply selected check paths as driver arguments for later use. The driver is read-only and +prints its next instruction as JSON; no scratch or state files are needed. + +1. Run `python3 PLUGIN_ROOT/common/codex/scripts/aspect-driver.py --phase inspect + --pass ROLE --base BASE --head HEAD`, replacing ROLE with `toolkit`, `standards`, `spec`, + or the selected `performance` role. Follow the printed aspect prompts and inspect every + changed file broadly, then trace the relevant behavior. Record factual observations with + pinned file/line citations and explicit gaps. Use focused reads and recover needed truncated + ranges. Do not read the pass brief or check contents yet. +2. In that same agent session, run the driver with `--phase assess`, the same role and pins, + `--observations` containing JSON `{summary,evidence:[{path,line,observation}],gaps:[]}`, + and one `--check PATH` per selected reference. Quote JSON as literal shell arguments. + The driver's output asks whether the observed behavior has any problems. Read its `readNow` + and applicable `checks` resources at this point, then follow its assessment instructions. + Apply the same substantive checks, evidence criteria, and coverage obligations as before. +3. Run `--phase collect` with the same role, pins and observations and `--findings` containing + the driver's requested finding array (or `[]`). Return the collected findings, required + evidence tables and coverage gaps to the parent. The driver validates citations and + structure; the parent's existing verification still decides which assertions are supported. + If Spec has no meaningful specification, return `No spec available` with source observations + and an empty findings array. An unfinished required check remains a coverage gap. + +Each handoff must explicitly forbid edits, formatters, posting, model subprocesses, further +agents, and Git checkout/switch/reset/stash/worktree. Do not run tests, builds, or linters. +Keep those prohibitions in prompt text, not shell commands. The driver requires Python 3.11+ +from the plugin requirements. If it cannot run, disclose the incomplete staged inspection; +do not silently substitute a direct bug-finding prompt or claim that the protocol completed. Do not share interim findings or read more source in the parent while reviewers run. Wait on their native handles, using a 60000 ms observation timeout where supported so empty polls diff --git a/common/codex/scripts/aspect-driver.py b/common/codex/scripts/aspect-driver.py new file mode 100644 index 0000000..83776f6 --- /dev/null +++ b/common/codex/scripts/aspect-driver.py @@ -0,0 +1,128 @@ +#!/usr/bin/env python3 +"""Read-only, two-stage source inspection and finding collection for native agents.""" +import argparse +import hashlib +import json +from pathlib import Path, PurePosixPath +import re +import subprocess +import sys + + +ASPECTS = { + "toolkit": [ + "Describe inputs, outputs, error propagation, and public type/API behavior.", + "Trace ownership, state transitions, callbacks, waits, retries, and shutdown.", + "Describe test stimuli, assertions, deadlines, and cleanup paths.", + "Relate factual comments and documentation to the implementation.", + "Map changed branches and repeated logic to their callers and tests.", + ], + "standards": [ + "Identify documented repository rules and their artifact-specific scope.", + "Compare changed setup, teardown, helpers, and declarations with nearby conventions.", + "Describe duplicated sites and state or API obligations that require coordination.", + "Map commit contents, messages, attribution, and validation claims to the changes.", + ], + "spec": [ + "Identify explicit promises in the PR description and supplied specification.", + "Map those promises to base/head behavior, including completion and failure ordering.", + "Describe how changed tests stimulate the promised behavior.", + ], + "performance": [ + "Describe work, allocation, I/O, and contention in changed hot paths.", + "Compare base/head costs for concrete inputs and concurrency levels.", + ], +} +ROOT = Path(__file__).resolve().parents[3] + + +def git(*args): + return subprocess.check_output(["git", *args], text=True, stderr=subprocess.PIPE).strip() + + +def citation(row, head): + path, line = row.get("path"), row.get("line") + if (not isinstance(path, str) or not path or "\\" in path + or PurePosixPath(path).is_absolute() or ".." in path.split("/")): + raise ValueError("citation needs a repository-relative path") + if type(line) is not int or line < 1: + raise ValueError("citation needs a positive integer line") + if line > len(git("show", f"{head}:{path}").splitlines()): + raise ValueError("citation line is outside the pinned head file") + + +def observations(value, head): + if not isinstance(value, dict) or not isinstance(value.get("summary"), str) or not value["summary"].strip(): + raise ValueError("observations need a nonempty factual summary") + evidence, gaps = value.get("evidence"), value.get("gaps") + if not isinstance(evidence, list) or not isinstance(gaps, list) or any(not isinstance(g, str) or not g.strip() for g in gaps): + raise ValueError("observations need evidence and explicit gap lists") + if not evidence and not gaps: + raise ValueError("supply source evidence or disclose the unexamined scope") + for row in evidence: + citation(row, head) + if not isinstance(row.get("observation"), str) or not row["observation"].strip(): + raise ValueError("each source citation needs a factual observation") + return value + + +def drive(args): + if not all(re.fullmatch(r"[a-f0-9]{40}", pin) for pin in (args.base, args.head)): + raise ValueError("full base/head OIDs are required") + if git("rev-parse", "HEAD") != args.head: + raise ValueError("checkout HEAD differs from the pinned head") + if git("merge-base", args.base, args.head) != args.base: + raise ValueError("base must be the pinned merge-base") + common = {"schemaVersion": 1, "pass": args.role, "base": args.base, "head": args.head} + if args.phase == "inspect": + return {**common, "phase": "inspect", "aspects": ASPECTS[args.role], + "instruction": "Look at these aspects across every changed file and relevant metadata. Describe what the code does, with source citations, and record unexamined scope. Then run this driver with --phase assess and --observations JSON containing summary, evidence [{path,line,observation}], and gaps. Stay in this same native agent session."} + seen = observations(json.loads(args.observations or "null"), args.head) + digest = hashlib.sha256(json.dumps(seen, sort_keys=True).encode()).hexdigest() + common.update(observations=seen, observationSha256=digest) + if args.phase == "assess": + return {**common, "phase": "assess", + "question": "Having looked at those aspects, do you see any problems there? No problems is a valid answer. Assess the observed behavior against the applicable contracts and consider contrary evidence; investigate a concern further only as needed to support its actual assertion.", + "readNow": [str(ROOT / "common" / "reporting.md"), + str(ROOT / "common" / "codex" / "passes" / f"{args.role}.md")], + "checks": args.check, + "instruction": "Apply the supplied pass brief and applicable check references now. Collect supported findings with --phase collect, the same --observations, and --findings JSON. Each finding needs id, category, claim, path, line, trigger, consequence, baseHead, and contraryEvidence. An empty array is valid. Preserve material coverage gaps; do not create files, run tests/builds, delegate, or publish."} + findings = json.loads(args.findings or "null") + if not isinstance(findings, list): + raise ValueError("findings must be a JSON array; use [] for no findings") + ids = set() + for row in findings: + if not isinstance(row, dict): + raise ValueError("each finding must be an object") + for field in ("id", "category", "claim", "trigger", "consequence", "baseHead", "contraryEvidence"): + if not isinstance(row.get(field), str) or not row[field].strip(): + raise ValueError(f"finding needs nonempty {field}") + if row["id"] in ids: + raise ValueError("finding IDs must be unique within a pass") + ids.add(row["id"]) + citation(row, args.head) + return {**common, "phase": "collected", "findings": findings, + "coverageGaps": seen["gaps"], + "instruction": "Return these findings and coverage gaps to the parent for its existing independent verification. Collection validates structure and citations, not semantic truth."} + + +def main(): + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("--phase", choices=("inspect", "assess", "collect"), required=True) + parser.add_argument("--pass", dest="role", choices=ASPECTS, required=True) + parser.add_argument("--base", required=True) + parser.add_argument("--head", required=True) + parser.add_argument("--observations") + parser.add_argument("--findings") + parser.add_argument("--check", action="append", default=[]) + args = parser.parse_args() + try: + print(json.dumps(drive(args), ensure_ascii=False)) + except (ValueError, TypeError, AttributeError, subprocess.CalledProcessError) as exc: + print(f"aspect-driver: {exc}", file=sys.stderr) + return 1 + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/tests/test_aspect_driver.py b/tests/test_aspect_driver.py new file mode 100644 index 0000000..6b3b1ee --- /dev/null +++ b/tests/test_aspect_driver.py @@ -0,0 +1,85 @@ +"""Exercise staged collection against real pinned Git objects without source edits.""" +import json +from pathlib import Path +import subprocess +import sys + +import pytest + + +DRIVER = Path(__file__).resolve().parents[1] / "common/codex/scripts/aspect-driver.py" + + +@pytest.fixture +def repo(tmp_path): + def git(*args): + return subprocess.check_output(["git", "-C", str(tmp_path), *args], text=True).strip() + git("init", "-q") + git("config", "user.email", "test@example.invalid") + git("config", "user.name", "Test") + (tmp_path / "code.py").write_text("value = 1\n") + git("add", ".") + git("commit", "-qm", "base") + base = git("rev-parse", "HEAD") + (tmp_path / "code.py").write_text("value = 2\n") + git("commit", "-qam", "head") + return tmp_path, base, git("rev-parse", "HEAD") + + +def run(repo, phase, **inputs): + directory, base, head = repo + args = [sys.executable, str(DRIVER), "--phase", phase, "--pass", "toolkit", + "--base", base, "--head", head] + for key, value in inputs.items(): + args.extend(["--" + key, json.dumps(value)]) + return subprocess.run(args, cwd=directory, capture_output=True, text=True) + + +def seen(**updates): + return {"summary": "The assigned value changes.", "evidence": [ + {"path": "code.py", "line": 1, "observation": "Value is now two."}], + "gaps": [], **updates} + + +def finding(**updates): + return {"id": "t1", "category": "runtime", "claim": "Example claim", + "path": "code.py", "line": 1, "trigger": "Example input", + "consequence": "Example consequence", "baseHead": "One becomes two", + "contraryEvidence": "No alternate assignment", **updates} + + +def test_stages_collect_without_editing_checkout(repo): + assert run(repo, "inspect").returncode == 0 + assessment = run(repo, "assess", observations=seen()) + assert assessment.returncode == 0, assessment.stderr + assessed = json.loads(assessment.stdout) + assert assessed["observations"] == seen() + collected = run(repo, "collect", observations=seen(), findings=[finding()]) + assert collected.returncode == 0, collected.stderr + value = json.loads(collected.stdout) + assert value["findings"] == [finding()] + assert value["observationSha256"] == assessed["observationSha256"] + assert subprocess.check_output(["git", "status", "--porcelain"], cwd=repo[0]) == b"" + + +def test_no_findings_preserves_unexamined_scope(repo): + result = run(repo, "collect", observations=seen(gaps=["Shutdown not inspected"]), findings=[]) + assert result.returncode == 0, result.stderr + assert json.loads(result.stdout)["coverageGaps"] == ["Shutdown not inspected"] + + +@pytest.mark.parametrize("update", [{"path": "../code.py"}, {"path": "missing.py"}, + {"line": 200}, {"line": True}, {"claim": ""}]) +def test_rejects_invalid_claims_and_citations(repo, update): + assert run(repo, "collect", observations=seen(), findings=[finding(**update)]).returncode == 1 + + +def test_does_not_assess_without_observations(repo): + assert run(repo, "assess").returncode == 1 + assert run(repo, "assess", observations=seen(evidence=[])).returncode == 1 + + +def test_rejects_duplicate_ids_and_moved_checkout(repo): + assert run(repo, "collect", observations=seen(), findings=[finding(), finding()]).returncode == 1 + directory, base, head = repo + assert run((directory, base, base), "inspect").returncode == 1