diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json index d310896..892a0e8 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.5" + "version": "1.0.6" } ] } diff --git a/.claude-plugin/plugin.json b/.claude-plugin/plugin.json index 9f3273f..a19da9c 100644 --- a/.claude-plugin/plugin.json +++ b/.claude-plugin/plugin.json @@ -1,6 +1,6 @@ { "name": "code-review-ad", - "version": "1.0.5", + "version": "1.0.6", "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 438a3a5..21eb4eb 100644 --- a/.codex-plugin/plugin.json +++ b/.codex-plugin/plugin.json @@ -1,6 +1,6 @@ { "name": "code-review-ad", - "version": "1.0.5", + "version": "1.0.6", "description": "Independent Toolkit, Standards, Spec, and conditional Performance review.", "author": { "name": "Dmitry Kropachev" diff --git a/PROVENANCE.json b/PROVENANCE.json index 8d32de5..56cad3c 100644 --- a/PROVENANCE.json +++ b/PROVENANCE.json @@ -29,6 +29,17 @@ "common/reporting.md" ], "description": "Require simple, direct and concise review language without filler words for both hosts and every native reviewer." + }, + { + "version": "1.0.6", + "paths": [ + "common/codex/scripts/aspect-driver.py", + "common/codex/review.md", + "common/codex/passes", + "codex/skills/code-review-ad/SKILL.md", + "tests/test_aspect_driver.py" + ], + "description": "Codex issue discovery from the first prompt and one role-specific deeper-look driver call in the same session, without intermediate observation or finding-collection payloads; read-only pins and parent verification retained." } ] } diff --git a/README.md b/README.md index 7e79f0a..e5be187 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.5. +Independent MIT plugin for Claude Code and Codex. Version 1.0.6. ## Install @@ -60,3 +60,5 @@ 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. Model and reasoning-effort settings are controlled by the host or runner. Neither host skill sets reviewer overrides. + +Codex reviewers find issues immediately, then call the read-only driver once for a role-specific deeper-look prompt before returning findings for parent verification. Both looks use the same native agent session and reuse inspected evidence; there are no intermediate observation or finding-collection payloads. Claude retains its current method. diff --git a/codex/skills/code-review-ad/SKILL.md b/codex/skills/code-review-ad/SKILL.md index 2d18249..fe303e9 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 find issues immediately, then use the bundled driver for one role-specific deeper look in the same session before returning supported findings. + 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/passes/performance.md b/common/codex/passes/performance.md index f9648d8..ebc14f9 100644 --- a/common/codex/passes/performance.md +++ b/common/codex/passes/performance.md @@ -1,3 +1,5 @@ # Performance pass +Find concrete performance regressions in the selected hot paths immediately. + Inspect changed code and relevant call sites for PR-introduced runtime, memory, I/O, or contention regressions. Prioritize paths exercised per request, item, or retry; derive the work or allocation from the changed operation and a realistic input or concurrency level. Compare base and head, and report only a concrete, consequential regression at a changed line. Do not run benchmarks or expand into general correctness, style, or spec review. Return `No findings` when no performance-sensitive change warrants a finding. diff --git a/common/codex/passes/spec.md b/common/codex/passes/spec.md index 74ede97..dd1ee20 100644 --- a/common/codex/passes/spec.md +++ b/common/codex/passes/spec.md @@ -1,3 +1,5 @@ # Spec pass +Find unmet explicit requirements and contradictions with the supplied specification immediately. + Compare the changed behavior against the PR description, linked issue, and explicit acceptance criteria. Compare the PR body's claimed scope and validation with every commit's actual changes, including tests and observable error/logging behavior. Trace important paths and tests, checking whether reordered early returns skip work formerly performed before them. For a new failure return before a moved state update, check what remains stale while failures continue; a later successful retry does not erase that interval. Compare the base and head for changes to when a public call completes, including new waits, cancellation points, and timeout behavior. If a new wait makes close or stop explicitly depend on earlier queued work, check whether the PR describes that completion order; report a documentation gap even if a previous flush could also block and no new hang is proved. If a wait can remain pending on a peer, I/O, or a promise, check whether the PR describes that observable behavior and whether a test covers it. If none provides a meaningful specification, return `No spec available` for this axis; do not invent one. diff --git a/common/codex/passes/standards.md b/common/codex/passes/standards.md index 945918d..37e8302 100644 --- a/common/codex/passes/standards.md +++ b/common/codex/passes/standards.md @@ -1,3 +1,5 @@ # Standards pass +Find applicable repository-rule violations and evidenced maintenance risks immediately. + Review against applicable, documented repository rules and established conventions supported by specific files. Compare every new test's setup and teardown with nearby tests and contributing guidance, including tracing or logging initialization. Compare a new error variant's display style with adjacent variants and every existing variant that wraps the same inner error; report a pattern mismatch only when it violates an applicable rule or causes a concrete issue. Inspect the PR body and each commit for concrete rule violations, unsupported attribution or validation claims, broken local issue references, a fix split from its test into a later commit, and later fixup commits. For each commit, compare its subject and body with all behavior, public types, and ordering it changes; a subject about one fix may conceal a moved check or another bundled change. If user-level guidance forbids attribution in work authored on the user's behalf, check the PR body and commits for that attribution and cite the guidance; existing older commits do not waive the user's explicit rule. Also inspect changed code for concrete maintenance risks: duplicated nearby explanations, identical adjacent `if` conditions, or scaffolding; loops that repeatedly call the same metric update when one counted operation would suffice; bounded polling sleeps that could use an existing or new notification, or drive the handler directly in a test; new test helpers that repeat existing builders; classifiers that must stay synchronized; unclear test-helper names; fragile references into containers that later edits could invalidate; and new state checks callers could forget. A timeout bounds a poll but does not make its observation deterministic, and lack of an existing signal does not itself make polling the best test structure. For each new lifecycle flag, enumerate every writer of the guarded state, including setup-only paths, and check whether each is guarded or relies only on a comment about call order; report a current failure, applicable rule violation, or maintenance risk meeting the shared evidence criteria. Check whether send-like paths or existing predicates omit the flag. Compare newly added or rewrapped documentation lines with the surrounding file's line-length convention, reporting a concrete violation of an applicable documented rule. When a new documentation block sits above an older comment on the same declaration, check for duplicated descriptions and whether the old comment separates the doc block from the declaration. For each new test wrapper, name the behavior it adds and check whether its class name communicates that behavior; report unclear names separately from duplicated forwarding methods. Report rule violations as findings. Currently safe code may still create a maintenance risk when exact duplicated sites, coupled state, API obligations, or a supported failure mode are demonstrated. Report at proportionate severity; omit matters of taste and generic future warnings. diff --git a/common/codex/passes/toolkit.md b/common/codex/passes/toolkit.md index a082fb5..6fdd7c6 100644 --- a/common/codex/passes/toolkit.md +++ b/common/codex/passes/toolkit.md @@ -1,3 +1,5 @@ # Toolkit pass +Find bugs and other supported issues immediately within this pass's scope. + In one pass, inspect comments and documentation for factual mismatch; test behavior and missing cases; error handling and swallowed failures; type and API design; correctness, maintainability, and security. For a new error path that discards a resource and schedules a retry, trace the stored state used by the next attempt: whether failure clears or changes it, whether repeated retries can succeed without external change, and what happens as existing resources expire. Scheduling another retry is not itself recovery, but retaining requested state can be intentional. Report the mechanism only when its consequence violates an applicable contract; do not invent a requirement to roll back a failed request or admit a resource with the wrong configuration. For a changed error log level, compare base and head and check whether repeated failures remain visible to operators or callers at default verbosity; report the visibility consequence separately from a commit-message mismatch. Compare a new error variant's display text and doc comment with every path that can produce it, not just the first call site. For changed tests, check that assertions enforce the claimed behavior, including whether they already hold before the changed action, every wait has a failure deadline, and cleanup still runs after an assertion or unexpected exception. A wait without a deadline and cleanup skipped by an exception are distinct hazards even when both can hang the test; cite both mechanisms. Map each changed production branch and client/server path to an actual test stimulus; do not count a test merely because it calls the surrounding method. When a test creates fixtures for both sides, check whether each captured hook is actually invoked and asserted. For a newly added override, compare the base declaration and nearby implementations under supported compile-time guards; a method that overrides only in the default configuration can leave another configuration abstract or fail to compile. For a new wait on queued work, trace every operation needed to settle that work, including peer I/O and shutdown or abort actions sequenced after the wait. Compare base and head ordering before concluding an older flush had the same liveness behavior. For changed docs and test comments, compare the stated consequence or platform coverage with the actual error, assertion, abort, or supported build path. A process-terminating assertion is materially more severe than an ordinary recoverable error; flag documentation that hides that consequence. For a comment claiming no other path waits on a future or state member, inspect later reads of that member. Inspect comments claiming coverage on a particular architecture against source constraints and build configuration; if read-only evidence is insufficient, mark the claim unproven. Check newly added comments against adjacent existing comments for duplication or contradiction. Combine overlapping checks into one finding. Report demonstrated defects, applicable rule violations, and maintenance risks meeting the shared evidence criteria. diff --git a/common/codex/review.md b/common/codex/review.md index b89897a..c33b8f9 100644 --- a/common/codex/review.md +++ b/common/codex/review.md @@ -89,34 +89,39 @@ 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. + +Ask each reviewer to find issues immediately within its assigned role. Supply paths to its +pass brief (`passes/ROLE.md`), the shared reporting and inspection criteria, and selected checks; +read them before inspecting source. Do not copy their contents into the handoff. + +After the initial broad inspection and candidate verification, each reviewer runs this command +exactly once in the same session, even if it found no issues: + +`python3 PLUGIN_ROOT/common/codex/scripts/aspect-driver.py --pass ROLE --base BASE --head HEAD` + +Use `toolkit`, `standards`, `spec`, or the selected `performance` role and the pinned merge-base +and head. Follow the printed role-specific deeper-look prompt. Reuse prior evidence; make +focused reads for new candidates or unresolved gaps without repeating the full scan or +reloading unchanged instructions. The driver only supplies this follow-up: no observation +serialization, finding-collection call, scratch files, or extra agent is needed. + +Return supported findings from both looks with issue category, exact claim, changed line, +source-backed trigger and consequence, base/head distinction, and contrary evidence. +Include required tables and a compact coverage note for each applicable check: evidence plus +a finding, supported no-finding conclusion, or unexamined gap. There is no finding quota. +If Spec has no meaningful specification, return `No spec available`. Unfinished checks remain +coverage gaps; do not drop low-severity verified facts. + +Each handoff must explicitly forbid edits, tests, builds, linters, formatters, posting, +model subprocesses, further agents, and Git checkout/switch/reset/stash/worktree. +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 driver workflow; +do not claim that the follow-up completed. Do not add another discovery round +in the parent; its remaining work is verification and reconciliation below. 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..d321e97 --- /dev/null +++ b/common/codex/scripts/aspect-driver.py @@ -0,0 +1,47 @@ +#!/usr/bin/env python3 +"""Print one role-specific deeper-look prompt for an ongoing native review.""" +import argparse +import re +import subprocess +import sys + + +PROMPTS = { + "toolkit": "Look deeper for additional bugs and other supported issues beyond your first candidates. Challenge boundary inputs, failure and cleanup paths, ownership, and async ordering; check whether tests can pass while the claimed behavior is broken.", + "standards": "Look deeper for additional repository-rule violations or evidenced maintenance risks. Check overlooked rule scope and exceptions, duplicated sites, and state or API obligations across callers. Support each concern with the exact applicable rule or demonstrated coordination invariant.", + "spec": "Look deeper for additional gaps in the explicit promises. Challenge failure, completion, and alternate input paths; check whether tests distinguish the promised behavior. Stay within the stated conditions and do not invent requirements. If no meaningful specification exists, retain No spec available.", + "performance": "Look deeper for additional performance regressions. Challenge scaling, repeated work, allocation, and contention under concrete input sizes or concurrency. Support the cost difference with source evidence and the repository's performance requirements.", +} + + +def git(*args): + return subprocess.check_output(["git", *args], text=True, stderr=subprocess.PIPE).strip() + + +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") + return (PROMPTS[args.role] + "\n\n" + "Take this deeper look once in the same session, whether or not you found issues initially. Reuse inspected evidence; make only focused reads for new candidates or unresolved gaps. Preserve earlier supported findings and consider contrary evidence. There is no finding quota; no additional issues is valid. Return findings and coverage gaps to the parent after this follow-up.") + + +def main(): + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("--pass", dest="role", choices=PROMPTS, required=True) + parser.add_argument("--base", required=True) + parser.add_argument("--head", required=True) + args = parser.parse_args() + try: + print(drive(args)) + except (ValueError, 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..2947c10 --- /dev/null +++ b/tests/test_aspect_driver.py @@ -0,0 +1,66 @@ +"""Exercise deeper-look prompts against real pinned Git objects.""" +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, role="toolkit"): + directory, base, head = repo + return subprocess.run([sys.executable, str(DRIVER), "--pass", role, + "--base", base, "--head", head], + cwd=directory, capture_output=True, text=True) + + +def test_returns_distinct_role_prompts_without_editing_checkout(repo): + prompts = [] + for role in ("toolkit", "standards", "spec", "performance"): + result = run(repo, role) + assert result.returncode == 0, result.stderr + assert result.stdout.strip() + prompts.append(result.stdout) + assert len(set(prompts)) == len(prompts) + assert subprocess.check_output(["git", "status", "--porcelain"], cwd=repo[0]) == b"" + + +@pytest.mark.parametrize("pin", ["HEAD", "abc", "f" * 40]) +def test_rejects_invalid_or_missing_base(repo, pin): + directory, base, head = repo + result = run((directory, pin, head)) + assert result.returncode == 1 + assert not result.stdout + + +def test_rejects_moved_checkout(repo): + directory, base, head = repo + assert run((directory, base, base)).returncode == 1 + + +def test_rejects_non_ancestor_base(repo): + directory, base, head = repo + other = subprocess.check_output( + ["git", "commit-tree", f"{base}^{{tree}}", "-p", base, "-m", "alternate"], + cwd=directory, text=True).strip() + result = run((directory, other, head)) + assert result.returncode == 1 + assert "base must be the pinned merge-base" in result.stderr