From 1c49fa3649ce4c0fa1b5d7bb6a2fe0930334e738 Mon Sep 17 00:00:00 2001 From: Dmitry Kropachev Date: Tue, 20 Oct 2026 22:41:02 -0400 Subject: [PATCH] Stage Codex inspection before defect assessment Ask native reviewers to describe source behavior before the driver asks about problems. Keep observations tied to frozen Git pins and collect cited findings for the existing parent verification. Delay assessment briefs until after the factual pass. Preserve read-only permissions, native reviewer roles, and host-controlled model settings. Mark the Codex path experimental and synchronize version 1.0.5. --- .claude-plugin/marketplace.json | 2 +- .claude-plugin/plugin.json | 2 +- .codex-plugin/plugin.json | 2 +- PROVENANCE.json | 10 ++ README.md | 4 +- codex/skills/code-review-ad/SKILL.md | 2 + common/codex/review.md | 62 +++++++------ common/codex/scripts/aspect-driver.py | 128 ++++++++++++++++++++++++++ tests/test_aspect_driver.py | 85 +++++++++++++++++ 9 files changed, 266 insertions(+), 31 deletions(-) create mode 100644 common/codex/scripts/aspect-driver.py create mode 100644 tests/test_aspect_driver.py 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