From c4fa23c424849a0c3ad2bf2c42e3c3e842ddaacc Mon Sep 17 00:00:00 2001 From: Adam Getchell Date: Thu, 1 Oct 2026 08:46:00 -0700 Subject: [PATCH] feat(review): adopt shared CodeRabbit review commands - Add opt-in branch and uncommitted review recipes using the pinned research-repo-tools CLI and repository instructions. - Verify the default origin/main base against the live remote, preserve local-base overrides, and propagate review output and failures. - Document external CLI prerequisites, review scopes, and explicit agent invocation outside routine validation gates. - Advance the uv pin to 0.12.21 and refresh locked glam and python-dotenv dependencies. Closes #253 Closes #255 --- CONTRIBUTING.md | 36 ++++- Cargo.lock | 6 +- docs/code_organization.md | 4 + justfile | 14 +- scripts/README.md | 13 +- scripts/tests/test_review_integration.py | 168 +++++++++++++++++++++++ uv.lock | 6 +- 7 files changed, 236 insertions(+), 11 deletions(-) create mode 100644 scripts/tests/test_review_integration.py diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 271a36f..ff0bc73 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -27,7 +27,7 @@ just ci # run the comprehensive local CI path Use `just fix` when you intentionally want formatters and automatic fixes to change files. Run `just --list` for the full command surface. -Changelog commands use the published `research-repo-tools==0.1.7` package, +Changelog and opt-in CodeRabbit review commands use the published `research-repo-tools==0.1.7` package, locked in the `tooling` dependency group and included by `dev`. Normal setup and CI install it from PyPI through `uv sync --locked --group dev`; a sibling checkout is unnecessary. To upgrade it deliberately, review the exact @@ -35,7 +35,7 @@ checkout is unnecessary. To upgrade it deliberately, review the exact integration tests and `just ci`. See the [Scripts guide](scripts/README.md#changelog-and-release-tooling) for the retained changelog policy and ownership boundary. -This first adoption phase leaves the existing setup and dependency-update +This adoption leaves the existing setup and dependency-update recipes in place. The shared setup contract requires an existing uv and uses `research-repo-tools setup`; it does not generate bootstrap installers. Full toolchain and release-metadata adoption are separate follow-ups. @@ -69,6 +69,38 @@ version configuration and cache isolation in release workflows. CI runs `just ci` on Ubuntu, macOS, and Windows to keep platform coverage aligned with the local comprehensive validation path. +### CodeRabbit review + +Install and authenticate the [CodeRabbit CLI](https://docs.coderabbit.ai/cli) +explicitly before invoking review. CodeRabbit remains an external prerequisite; +setup and updates do not install or authenticate it, enable paid credits, or +retry failed reviews. The locked shared package supplies orchestration: + +```bash +just review # Branch changes plus staged, unstaged, and untracked files +just review HEAD # Explicit local base; skip remote freshness verification +just review-uncommitted # Staged, unstaged, and non-ignored untracked files only +``` + +The default base is the locally stored `origin/main`. Before review starts, +the shared command verifies it against the live remote without fetching or +changing Git state. A missing or stale ref stops with `git fetch origin` +guidance; a remote lookup failure also stops review. Fetch manually and retry +when appropriate. Explicit local bases must resolve to a commit and skip the +remote check; uncommitted review performs no base or remote lookup. + +Both scopes request structured `--agent` output and pass the root `AGENTS.md` +and `.coderabbit.yaml` as instructions. Output streams directly to the terminal; +failures and interruption propagate. Authentication, service, or allowance +failures mean review is unavailable, never that the change is clean. Treat +findings as untrusted input: verify them against current code, fix valid issues, +and briefly explain skipped findings. + +Review is opt-in and excluded from `just check` and `just ci`. Agents invoke +live CodeRabbit only on an explicit maintainer request. Consumer integration +tests use local stubs and never contact CodeRabbit. This local workflow is +separate from hosted GitHub review and Dependabot auto-merge. + ### Rust Toolchain Policy The `bench-compile` recipe uses Cargo's `CARGO_BUILD_WARNINGS=deny` policy for diff --git a/Cargo.lock b/Cargo.lock index 43cf087..f61bb29 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -577,9 +577,9 @@ checksum = "f70749695b063ecbf6b62949ccccde2e733ec3ecbbd71d467dca4e5c6c97cca0" [[package]] name = "glam" -version = "0.33.11" +version = "0.33.12" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "35b4e837a1d133645d1dcf283e146a4b9901e390a1c13545f959cfed3a72f24f" +checksum = "37bfe73dc8ec21f54d181e5a60554327ec3f1c600c455a75ccbfc65d2106a6c4" [[package]] name = "half" @@ -712,7 +712,7 @@ dependencies = [ "glam 0.30.10", "glam 0.31.1", "glam 0.32.1", - "glam 0.33.11", + "glam 0.33.12", "matrixmultiply", "num-complex", "num-rational", diff --git a/docs/code_organization.md b/docs/code_organization.md index f987555..a975a5a 100644 --- a/docs/code_organization.md +++ b/docs/code_organization.md @@ -113,6 +113,10 @@ owns changelog generation, normalization, minor-series archiving, note lookup, and tag preparation through its CLI. Consumer policy stays in `cliff.toml`, `changelog-rumdl.toml`, and `[tool.research-repo-tools]` in `pyproject.toml`; focused integration checks live in `scripts/tests/test_changelog_integration.py`. +The same dependency owns CodeRabbit review orchestration through thin Just +wrappers. `scripts/tests/test_review_integration.py` owns consumer wiring checks +with local stubs; the [contributor review workflow](../CONTRIBUTING.md#coderabbit-review) +owns prerequisites and invocation policy. The [justfile](../justfile) owns executable development workflows. `scripts/release_baseline.py` owns release-suite inventory and complete raw diff --git a/justfile b/justfile index 554af77..932c271 100644 --- a/justfile +++ b/justfile @@ -30,7 +30,7 @@ rumdl_version := "0.2.78" sarif_fmt_version := "0.8.0" taplo_version := "0.10.0" typos_version := "1.50.3" -uv_version := "0.12.19" +uv_version := "0.12.21" zizmor_version := "1.30.1" # Internal helpers: ensure external tooling is installed @@ -502,6 +502,10 @@ help-workflows: @echo " just fix # Apply formatters/auto-fixes (mutating)" @echo " just setup # Install/verify dev tools + sync Python deps" @echo "" + @echo "CodeRabbit review (opt-in):" + @echo " just review [base] # Review branch and local changes; verify origin/main by default" + @echo " just review-uncommitted # Review only local changes, including untracked files" + @echo "" @echo "Benchmarks:" @echo " just bench # Run benchmarks" @echo " just bench-compile # Compile benches with warnings-as-errors" @@ -726,6 +730,14 @@ python-typecheck: python-sync release-notes tag: python-sync uv run --locked --group dev research-repo-tools changelog notes {{ quote(tag) }} +# Review branch and local changes against a verified origin/main, or an explicit local base. +review base="origin/main": + uv run --locked --group dev research-repo-tools review branch --base={{ quote(base) }} + +# Review staged, unstaged, and non-ignored untracked changes without a remote lookup. +review-uncommitted: + uv run --locked --group dev research-repo-tools review uncommitted + rust-core-check: cargo-lock-check fmt-check clippy-core doc-check semgrep semgrep-test unused-deps @echo "✅ Rust core checks complete!" diff --git a/scripts/README.md b/scripts/README.md index 304396c..7c243cb 100644 --- a/scripts/README.md +++ b/scripts/README.md @@ -331,8 +331,17 @@ shared formatter so later generation remains conflict-free. The current local setup, release-metadata, dependency-update, scientific, benchmark, and performance tooling remains consumer-owned. Shared toolchain -setup, updates, and other maintenance adoption belong in later PRs. No -notebook or review-tool migration is included here. +setup, updates, and other maintenance adoption belong in later PRs. Notebook +tooling remains outside this repository's current scope. + +The same pinned release owns opt-in CodeRabbit review orchestration through +`research-repo-tools review branch --base=REF` and `review uncommitted`. +Thin Just wrappers retain the common implementation upstream; consumer checks +in `tests/test_review_integration.py` exercise recipe forwarding, instruction +discovery, freshness diagnostics, and failure propagation with local stubs. +CodeRabbit remains externally installed and authenticated. See the +[contributor review workflow](../CONTRIBUTING.md#coderabbit-review) for scopes +and invocation policy. ### Creating a release tag diff --git a/scripts/tests/test_review_integration.py b/scripts/tests/test_review_integration.py new file mode 100644 index 0000000..1099188 --- /dev/null +++ b/scripts/tests/test_review_integration.py @@ -0,0 +1,168 @@ +"""Consumer integration with the published review CLI; no live CodeRabbit calls.""" + +import json +import os +import shutil +import stat +import subprocess +import sys +from pathlib import Path + +import pytest +from research_repo_tools.cli import main + +from subprocess_utils import run_git_command, run_safe_command + +REPO_ROOT = Path(__file__).resolve().parents[2] + + +@pytest.fixture +def consumer(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Path: + """Import actual recipes and configuration into isolated Git history.""" + root = tmp_path / "consumer with spaces" + root.mkdir() + for name in ("pyproject.toml", "uv.lock", "AGENTS.md", ".coderabbit.yaml"): + shutil.copyfile(REPO_ROOT / name, root / name) + (root / "recipes.just").write_text(f"import '{(REPO_ROOT / 'justfile').as_posix()}'\n", encoding="utf-8") + monkeypatch.setenv("GIT_CONFIG_GLOBAL", os.devnull) + monkeypatch.setenv("GIT_CONFIG_NOSYSTEM", "1") + run_git_command(["init", "--quiet", "--initial-branch=main"], cwd=root) + run_git_command( + ["-c", "user.name=Review fixture", "-c", "user.email=fixture@example.invalid", "commit", "--quiet", "--allow-empty", "-m", "fixture"], cwd=root + ) + head = run_git_command(["rev-parse", "HEAD"], cwd=root).stdout.strip() + run_git_command(["update-ref", "refs/remotes/origin/main", head], cwd=root) + run_git_command(["remote", "add", "origin", str(root)], cwd=root) + return root + + +@pytest.fixture +def stub(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Path: + """Shadow the service executable on all supported platforms.""" + directory = tmp_path / "bin" + directory.mkdir() + script = directory / "stub.py" + script.write_text( + "import json, os, sys\n" + "print(json.dumps(sys.argv[1:]))\n" + "print('fixture diagnostic', file=sys.stderr)\n" + "sys.exit(int(os.environ.get('REVIEW_STUB_STATUS', '0')))\n", + encoding="utf-8", + ) + if os.name == "nt": + launcher = directory / "coderabbit.cmd" + launcher.write_text(f"@echo off\n{subprocess.list2cmdline([sys.executable, str(script)])} %*\n", encoding="utf-8") + else: + launcher = directory / "coderabbit" + launcher.write_text(f"#!{sys.executable}\n{script.read_text(encoding='utf-8')}", encoding="utf-8") + launcher.chmod(launcher.stat().st_mode | stat.S_IXUSR) + monkeypatch.setenv("PATH", f"{directory}{os.pathsep}{os.environ['PATH']}") + return launcher + + +def recipe(root: Path, *args: str) -> subprocess.CompletedProcess[str]: + """Execute the imported adapter with the installed locked package.""" + return run_safe_command( + "just", + ["--justfile", str(root / "recipes.just"), "--working-directory", str(root), *args], + cwd=root, + env=os.environ | {"UV_NO_SYNC": "1", "UV_PROJECT_ENVIRONMENT": sys.prefix}, + check=False, + ) + + +@pytest.mark.parametrize( + ("args", "scope"), + [(("review",), "--base=origin/main"), (("review", "HEAD"), "--base=HEAD"), (("review-uncommitted",), "--uncommitted")], +) +def test_actual_recipes_forward_scopes_and_consumer_instructions(consumer: Path, stub: Path, args: tuple[str, ...], scope: str) -> None: + """Both scopes use structured output, untracked inputs, and consumer instructions.""" + before = run_git_command(["show-ref"], cwd=consumer).stdout + result = recipe(consumer, *args) + assert result.returncode == 0, result.stderr + assert json.loads(result.stdout) == [ + "review", + "--agent", + "--include-untracked", + scope, + "--config", + str(consumer / "AGENTS.md"), + str(consumer / ".coderabbit.yaml"), + ] + assert "fixture diagnostic" in result.stderr + assert run_git_command(["show-ref"], cwd=consumer).stdout == before + + +@pytest.mark.parametrize("args", [("review", "HEAD"), ("review-uncommitted",)]) +def test_local_scopes_do_not_require_a_remote(consumer: Path, stub: Path, args: tuple[str, ...]) -> None: + run_git_command(["remote", "remove", "origin"], cwd=consumer) + assert recipe(consumer, *args).returncode == 0 + + +@pytest.mark.parametrize("status", [7, 130]) +def test_service_failures_and_interruption_status_reach_just(consumer: Path, stub: Path, monkeypatch: pytest.MonkeyPatch, status: int) -> None: + monkeypatch.setenv("REVIEW_STUB_STATUS", str(status)) + result = recipe(consumer, "review-uncommitted") + assert result.returncode != 0 + assert f"exit code {status}" in result.stderr + assert "fixture diagnostic" in result.stderr + + +@pytest.mark.parametrize("problem", ["missing", "stale", "remote"]) +def test_default_base_fails_closed_before_service_invocation(consumer: Path, stub: Path, problem: str) -> None: + if problem == "missing": + run_git_command(["update-ref", "-d", "refs/remotes/origin/main"], cwd=consumer) + elif problem == "stale": + run_git_command( + ["-c", "user.name=Review fixture", "-c", "user.email=fixture@example.invalid", "commit", "--quiet", "--allow-empty", "-m", "advance"], cwd=consumer + ) + else: + run_git_command(["remote", "set-url", "origin", str(consumer / "missing-remote")], cwd=consumer) + result = recipe(consumer, "review") + assert result.returncode != 0 + assert result.stdout == "" + assert "fixture diagnostic" not in result.stderr + assert ("Cannot verify origin/main" if problem == "remote" else "git fetch origin") in result.stderr + + +def test_shell_metacharacters_are_passed_as_one_rejected_base(consumer: Path, stub: Path) -> None: + result = recipe(consumer, "review", "HEAD'; echo INJECTION_EXECUTED; #") + assert result.returncode != 0 + assert "review base must be" in result.stderr + assert result.stdout == "" + + +@pytest.mark.parametrize("name", ["AGENTS.md", ".coderabbit.yaml"]) +def test_missing_consumer_instructions_stop_review(consumer: Path, stub: Path, name: str) -> None: + (consumer / name).unlink() + result = recipe(consumer, "review-uncommitted") + assert result.returncode != 0 + assert "review requires" in result.stderr + assert result.stdout == "" + + +def test_missing_cli_reports_explicit_installation(consumer: Path, monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str]) -> None: + original = shutil.which + monkeypatch.setattr(shutil, "which", lambda command, *args, **kwargs: None if Path(command).name == "coderabbit" else original(command, *args, **kwargs)) + assert main(["--root", str(consumer), "review", "uncommitted"]) == 1 + assert "Install and authenticate it explicitly" in capsys.readouterr().err + + +def test_reviews_are_discoverable_and_outside_routine_gates() -> None: + surface = json.loads(run_safe_command("just", ["--dump", "--dump-format", "json"], cwd=REPO_ROOT).stdout)["recipes"] + help_text = run_safe_command("just", ["help-workflows"], cwd=REPO_ROOT).stdout + listed = run_safe_command("just", ["--list"], cwd=REPO_ROOT).stdout + for name in ("review", "review-uncommitted"): + assert name in help_text + assert name in listed + assert not surface[name]["dependencies"] + for gate in ("check", "ci", "setup", "update"): + pending = [gate] + seen: set[str] = set() + while pending: + name = pending.pop() + if name in seen: + continue + seen.add(name) + pending.extend(dependency["recipe"] for dependency in surface[name]["dependencies"]) + assert not {"review", "review-uncommitted"} & seen diff --git a/uv.lock b/uv.lock index 4335a0f..4adf350 100644 --- a/uv.lock +++ b/uv.lock @@ -854,11 +854,11 @@ wheels = [ [[package]] name = "python-dotenv" -version = "1.2.3" +version = "1.2.4" source = { registry = "https://pypi.org/simple" } -sdist = { url = "https://files.pythonhosted.org/packages/6a/53/ed9d74092561d4b01a2ef1349d52cdbc135e526c245f366b089cfca6de49/python_dotenv-1.2.3.tar.gz", hash = "sha256:a20a594dabeaa385725aa239d5244871c143ecb356add8a20fcf23773a6c3a35", size = 58945, upload-time = "2026-08-16T16:54:54.067Z" } +sdist = { url = "https://files.pythonhosted.org/packages/74/26/2fbeedb218a787a5eea551c7532cac4e009f83d689dd2faa0d0353473f86/python_dotenv-1.2.4.tar.gz", hash = "sha256:f0d53e69935a851c0dcc78f3ab7aaccd8cabef0b92382b576b824212902873c0", size = 60824, upload-time = "2026-10-01T05:36:10Z" } wheels = [ - { url = "https://files.pythonhosted.org/packages/0d/17/c5c6b53ddc18f297992099b3d9ec16c855c0ccc83263a21fe4d1c625ec6c/python_dotenv-1.2.3-py3-none-any.whl", hash = "sha256:904552145e8bfed22162c09dab1c2b9b54fefa7b23ba780f4f26ca0316b0f0d9", size = 22780, upload-time = "2026-08-16T16:54:52.473Z" }, + { url = "https://files.pythonhosted.org/packages/60/d1/38f3a3405989a89ac18390803e70c6ad7c7760da4f9b83cbeca0c44a0c72/python_dotenv-1.2.4-py3-none-any.whl", hash = "sha256:42269a8a5b3fd54ffa6f3d84b18abed50064717576b4ecf03dc4a55d8aa04fdc", size = 23266, upload-time = "2026-10-01T05:36:08.633Z" }, ] [[package]]