diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index f4ff95d..32558f2 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -13,6 +13,7 @@ jobs: - name: Run AI tooling tests run: | python3 ai/skills/go/scripts/tests/test_run_review.py + python3 ai/skills/go/scripts/tests/test_review_tier.py ai/tests/test-skill-spec.sh ai/tests/test-canonical-skills.sh ai/bin/sync-portable-skills.sh --check diff --git a/ai/README.md b/ai/README.md index a4c6103..26b0d88 100644 --- a/ai/README.md +++ b/ai/README.md @@ -52,7 +52,9 @@ When `implementation-planner` marks a plan with two or more stages `**Execution* ## Fresh review processes -`go` supports Claude Code and Codex. It passes its current harness explicitly to `skills/go/scripts/run-review.py`, which starts a fresh review in the same checkout. Claude uses print mode; Codex uses `exec`. Both save the review result under the checkout's `.notes/` directory so the parent can resume at validation after a context clear. The runner requires Python 3.11 or later and the selected CLI on `PATH`; it never substitutes the other harness. The installed `review-code` skill must support `REVIEW_CODE_REVIEW_DIR`; update it from its source repository with `bin/setup` if the runner reports an older installation. Review sessions, reports, temporary checkouts, and hook files stay under each attempt in `.notes/go-reviews/`. The Codex child receives no write grants for shared review caches. After success, the parent copies the report into the shared review history, where it survives removal of the implementation worktree. A Claude review runs on the accounts that docket's `config.toml` names in `claude_config_dir` and `claude_fallback_config_dirs`, in the order docket's rotation picks them. The runner skips each account that a usage limit in docket's `limits.jsonl` still applies to. A limit applies to every config directory signed in to the login (one account in one organization) that docket recorded it with. `run-review.py accounts` prints that rotation, the limits docket recorded, and the order a review would try the accounts in now, as JSON, and `usage-all` shows it below each account's usage. Without a docket config, the review runs on the current account. The runner refuses an account that is not signed in or has no `review-code` skill. When an account hits its usage limit before the review edits any files, the runner reviews again on the next account that is signed in to a different login, and then on the current account if the list leaves it out. A run with `--since-run ` copies that earlier run's report into its own attempt and asks `review-code` for an `--append` review of the changes since that report. +`go` supports Claude Code and Codex. It passes its current harness explicitly to `skills/go/scripts/run-review.py`, which starts a fresh review in the same checkout. Claude uses print mode; Codex uses `exec`. Both save the review result under the checkout's `.notes/` directory so the parent can resume at validation after a context clear. The runner requires Python 3.11 or later and the selected CLI on `PATH`; it never substitutes the other harness. The installed `review-code` skill must support `REVIEW_CODE_REVIEW_DIR`; update it from its source repository with `bin/setup` if the runner reports an older installation. Review sessions, reports, temporary checkouts, and hook files stay under each attempt in `.notes/go-reviews/`. The Codex child receives no write grants for shared review caches. After success, the parent copies the report into the shared review history, where it survives removal of the implementation worktree. A Claude review runs on the accounts that docket's `config.toml` names in `claude_config_dir` and `claude_fallback_config_dirs`, in the order docket's rotation picks them. The runner skips each account that a usage limit in docket's `limits.jsonl` still applies to. A limit applies to every config directory signed in to the login (one account in one organization) that docket recorded it with. `run-review.py accounts` prints that rotation, the limits docket recorded, and the order a review would try the accounts in now, as JSON, and `usage-all` shows it below each account's usage. Without a docket config, the review runs on the current account. The runner refuses an account that is not signed in or has no `review-code` skill. When an account hits its usage limit before the review edits any files, the runner reviews again on the next account that is signed in to a different login, and then on the current account if the list leaves it out. A run with `--since-run ` copies that earlier run's report into its own attempt and asks `review-code` for an `--append` review of the changes since that report. After a successful review, the runner copies the review's token-usage rows into the shared `token-usage.jsonl` with the `go` run IDs added, because the copy under `.notes/go-reviews/` is deleted with the worktree. + +`skills/go/scripts/review-tier.py` decides whether two late steps run. Review fix commits skips its review only for a small change to docs or tests that touches no risky path (such as a migration, a public API, auth, or infra) and makes no uncertain change (such as a rename or a deleted file). Final simplify skips its pass when the review fixes change a few lines or only docs. Prompt files, such as a `SKILL.md` or any file under `skills/`, count as code, and so does any file the script does not recognize as docs or tests. The line limits are constants at the top of the script. The Report step lists every skipped step and the commits no reviewer read. ## Model tiers @@ -73,6 +75,7 @@ Run the installer and portability tests with: ```sh python3 ai/skills/go/scripts/tests/test_run_review.py +python3 ai/skills/go/scripts/tests/test_review_tier.py ai/tests/test-skill-spec.sh ai/tests/test-canonical-skills.sh ai/bin/sync-portable-skills.sh --check diff --git a/ai/skills/go/SKILL.md b/ai/skills/go/SKILL.md index 8d29d62..4f87d7e 100644 --- a/ai/skills/go/SKILL.md +++ b/ai/skills/go/SKILL.md @@ -47,6 +47,7 @@ active-stage: review-code next-action: validate-review-fixes review-state: .notes/go-review-state.json review-run: 0b6e4c2a-5d1f-4e8b-9a3c-7f2d1e6b8a40 +fix-review-tier: skip 9c1d2e3..f4a5b6c (14 lines) - implement: done - simplify-commit: a1b2c3d @@ -436,6 +437,15 @@ Record `- reviews-addressed: `. The commits from Address reviews reached the branch after `review-code` read it, and Final simplify only simplifies them. This step runs `review-code` over them once. It runs before Final simplify so that simplify also reads this step's fixes. Save `active-stage: fix-review`. A state file without `review-run` predates this step. In that case, record the `run_id` that `run-review.py status` reports as `review-run` in the same write, because that run is then the Review step's. If HEAD still equals the `review-code` sha, Address reviews committed nothing: go to the bookkeeping below. +Decide whether the fix commits need a review. Run the tier script from the commit the Review step's review read, so the range covers the Review step's own fixes too. The script gives the same answer for the same range, so a resumed run makes the same decision: + +```bash +REVIEW_INPUT_SHA=$(jq -r .input_sha ".notes/go-reviews/$REVIEW_RUN/state.json") +python3 "$GO_SKILL_DIR/scripts/review-tier.py" --purpose fix-review --base "$REVIEW_INPUT_SHA" +``` + +It prints JSON with `tier`, `reasons`, and `stats`. Record `fix-review-tier: .. ()` in the header, replacing an earlier entry, so the Report step can name the decision. `skip`, which the script returns only for small changes to docs or tests, goes to the bookkeeping below without a review. `full` runs the review as below. A small code change still gets the review, because a small change can carry a large bug. + Push any unpushed commits first, and resolve a failed push before going on. `review-code` reads the PR's head from GitHub, so it does not review a commit that is only local. Save `next-action: run-review`, then run the runner as the Review step does, naming the review that step saved as `review-run`: ```bash @@ -456,6 +466,14 @@ Then handle the result as the Review step does, from waiting for `phase: reviewe The Review, Address reviews, and Review fix commits steps commit review fixes that Quality passes never read. Save `active-stage: final-simplify`, then list the branch's own commits since the later of `simplify-commit` and a `final-simplify` entry from an earlier pass, using the `git log --first-parent --no-merges` command from Quality passes, so a later pass reads only the commits added since the last simplify pass. If `git merge-base --is-ancestor HEAD` fails, a rebase has rewritten the branch since that pass and the fixes can no longer be told apart, so list from the branch's base as Quality passes resolves it. An empty list means the reviews changed nothing: go to the bookkeeping below. +Otherwise decide whether the commits need a simplify pass. Run the tier script from the start sha the list used: + +```bash +python3 "$GO_SKILL_DIR/scripts/review-tier.py" --purpose final-simplify --base +``` + +Record `final-simplify-tier: .. ()` in the header, replacing an earlier entry. `skip`, which the script returns for a few changed lines or a docs-only change, goes to the bookkeeping below. + Otherwise run `simplify` over those commits, recording declined findings and held comments as Quality passes does: - Pass the list as the scope, because the fixes are usually pushed by now: `Skill("simplify", args: "Review only the changes these commits made: . They are review fixes. The rest of the branch already had a simplify pass.")` @@ -568,7 +586,8 @@ explain-open translates each open or skipped item into plain English, weighs bot Then report the rest: - Commits added during the run — `git log ..HEAD --oneline` when the branch has a stage 1 commit, using the loop base that Implement's stage loop defines, otherwise `git log ^..HEAD --oneline` using the state file's first recorded sha (everything is pushed by now, so `@{u}..HEAD` comes back empty) -- The commits that landed after the last review read the branch, listed separately with their files: `git log --name-only --format='%h %s' ..HEAD`, where `` is the value `run-review.py status` reports, or the `simplify-commit` sha when it reports none. No reviewer read those changes, so the user may want to look at them. +- The commits that landed after the last review read the branch, listed separately with their files: `git log --name-only --format='%h %s' ..HEAD`, where `` is the value `run-review.py status` reports, or the `simplify-commit` sha when it reports none. No reviewer read those changes, so the user may want to look at them. When Review fix commits skipped its review, this list holds the fix commits. +- Each step that a tier skipped, from `fix-review-tier` and `final-simplify-tier` entries that read `skip`: the step's name, its commit range, and the reason - The PR URL (`gh pr view --json url -q .url`) and CI status - Gap entries logged this run — misses and false positives, with the `reviewhog-gaps.md` path — or that ReviewHog was skipped/timed out - Any drafted replies to human reviewers awaiting approval — these are never posted automatically diff --git a/ai/skills/go/scripts/review-tier.py b/ai/skills/go/scripts/review-tier.py new file mode 100644 index 0000000..db5ee1e --- /dev/null +++ b/ai/skills/go/scripts/review-tier.py @@ -0,0 +1,295 @@ +#!/usr/bin/env python3 +"""Decide whether go's fix-commit review or Final simplify runs over a commit range.""" + +import argparse +import fnmatch +import json +import re +import subprocess +import sys + + +FIX_REVIEW_MAX_LINES = 100 +FINAL_SIMPLIFY_MAX_LINES = 20 + +PROMPT_NAMES = { + "skill.md", + "agents.md", + "claude.md", + "gemini.md", + "copilot-instructions.md", + ".cursorrules", +} +PROMPT_SEGMENTS = { + "skills", + "agents", + "commands", + "prompts", + "handlers", + "contexts", + "rules", + "evals", + ".claude", + ".cursor", +} +TEST_SEGMENTS = {"test", "tests", "__tests__", "spec", "testdata"} +TEST_NAME = re.compile( + r"test_.*\.py|conftest\.py|.*_test\.go|.*_spec\.rb|.*\.(test|spec)\.(js|jsx|ts|tsx|mjs|cjs)" +) +DOCS_EXTENSIONS = (".md", ".mdx", ".rst", ".adoc") +DOCS_PREFIXES = ("README", "CHANGELOG", "LICENSE") + +API_SEGMENTS = {"api", "openapi", "serializers", "sdk"} +WIRE_EXTENSIONS = (".proto", ".avsc", ".capnp", ".thrift", ".fbs") +AUTH_WORDS = ( + "auth", + "permission", + "access_control", + "access-control", + "rbac", + "csrf", + "cors", + "secret", + "credential", + "crypt", + "password", + "security", + "saml", + "sso", +) +INFRA_SEGMENTS = { + "charts", + "helm", + "terraform", + "k8s", + "kubernetes", + "argocd", + ".github", + "deploy", + "infra", +} +DEPENDENCY_NAMES = { + "package.json", + "package-lock.json", + "pnpm-lock.yaml", + "yarn.lock", + "pyproject.toml", + "poetry.lock", + "uv.lock", + "pipfile", + "pipfile.lock", + "cargo.toml", + "cargo.lock", + "go.mod", + "go.sum", + "gemfile", + "gemfile.lock", +} +DATA_DELETION = re.compile( + r"\bDELETE\s+FROM\b|\bDROP\s+(TABLE|COLUMN|DATABASE|INDEX)\b|\bTRUNCATE\s+TABLE\b|\bALTER\s+TABLE\b|\brm\s+-rf\b", + re.IGNORECASE, +) +STATUS_REASONS = { + "R": "uncertain-rename", + "C": "uncertain-copy", + "D": "uncertain-deleted-file", + "T": "uncertain-type-change", +} +SUBMODULE_MODE = "160000" +SYMLINK_MODE = "120000" + + +def git(*args): + return subprocess.run( + ["git", *args], check=True, capture_output=True + ).stdout.decode(errors="replace") + + +def git_diff(*args): + # A user's external diff driver or textconv filter would change the output this script parses. + return git("diff", "--no-ext-diff", "--no-textconv", *args) + + +def split_path(path): + parts = path.split("/") + return parts[:-1], parts[-1] + + +def file_class(path): + # A file counts as docs or tests only when its name says so. + # Any file the rules do not recognize counts as code, so the review runs. + segments, name = split_path(path) + lowered = {segment.lower() for segment in segments} + lower_name = name.lower() + if lower_name in PROMPT_NAMES: + return "code" + if lowered & TEST_SEGMENTS or TEST_NAME.fullmatch(name): + return "test" + if lowered & PROMPT_SEGMENTS: + return "code" + suffix = lower_name.rpartition(".")[2] if "." in lower_name else "" + if lower_name.endswith(DOCS_EXTENSIONS) or ( + name.startswith(DOCS_PREFIXES) and suffix in {"", "txt"} + ): + return "docs" + return "code" + + +def path_risks(path): + segments, name = split_path(path) + lowered = {segment.lower() for segment in segments} + lower_name = name.lower() + lower_path = path.lower() + risks = [] + if ( + any("migration" in segment for segment in lowered) + or "alembic" in lowered + or lower_name.endswith(".sql") + ): + risks.append("risky-migration") + if lowered & API_SEGMENTS or lower_name.endswith(".graphql"): + risks.append("risky-api") + if lower_name.endswith(WIRE_EXTENSIONS): + risks.append("risky-wire-format") + if any(word in lower_path for word in AUTH_WORDS): + risks.append("risky-auth") + if ( + lowered & INFRA_SEGMENTS + or lower_name.endswith((".tf", ".tfvars")) + or lower_name.startswith(("dockerfile", "docker-compose")) + or (lower_name.startswith("values") and lower_name.endswith((".yaml", ".yml"))) + ): + risks.append("risky-infra") + if lower_name in DEPENDENCY_NAMES or fnmatch.fnmatch( + lower_name, "requirements*.txt" + ): + risks.append("risky-dependency") + return [f"{risk}: {path}" for risk in risks] + + +def raw_entries(base, head): + fields = git_diff("--raw", "-z", "-M", "-C", base, head).split("\0") + entries = [] + index = 0 + while index < len(fields) and fields[index]: + old_mode, new_mode, _, _, status = fields[index][1:].split(" ") + paths = 2 if status[0] in "RC" else 1 + entries.append( + { + "old_mode": old_mode, + "new_mode": new_mode, + "status": status[0], + "path": fields[index + paths], + } + ) + index += 1 + paths + return entries + + +def line_counts(base, head): + fields = git_diff("--numstat", "-z", "-M", "-C", base, head).split("\0") + counts = {} + index = 0 + while index < len(fields) and fields[index]: + added, removed, path = fields[index].split("\t", 2) + if path: + index += 1 + else: + path = fields[index + 2] + index += 3 + counts[path] = ( + None if added == "-" else (int(added), int(removed)) + ) + return counts + + +def added_lines(base, head): + in_hunk = False + for line in git_diff("--unified=0", "--no-color", base, head).splitlines(): + if line.startswith("diff --git "): + in_hunk = False + elif line.startswith("@@"): + in_hunk = True + elif in_hunk and line.startswith("+"): + yield line[1:] + + +def uncertain_reasons(entry, counts): + path = entry["path"] + modes = {entry["old_mode"], entry["new_mode"]} + reasons = [STATUS_REASONS[entry["status"]]] if entry["status"] in STATUS_REASONS else [] + if entry["status"] == "M" and entry["old_mode"] != entry["new_mode"]: + reasons.append("uncertain-mode-change") + if SUBMODULE_MODE in modes: + reasons.append("uncertain-submodule") + if SYMLINK_MODE in modes: + reasons.append("uncertain-symlink") + if counts.get(path, (0, 0)) is None: + reasons.append("uncertain-binary") + return [f"{reason}: {path}" for reason in reasons] + + +def decide(purpose, base, head): + result = {"purpose": purpose, "base": base, "head": head} + try: + entries = raw_entries(base, head) + counts = line_counts(base, head) + deletions = purpose == "fix-review" and any( + DATA_DELETION.search(line) for line in added_lines(base, head) + ) + except (subprocess.CalledProcessError, ValueError, IndexError) as error: + message = getattr(error, "stderr", None) or str(error).encode() + first_line = message.decode(errors="replace").strip().partition("\n")[0] + return result | { + "tier": "full", + "reasons": [f"uncertain-diff-failed: {first_line}"], + "stats": {"lines": 0, "files": 0}, + } + lines = sum(sum(count) for count in counts.values() if count) + result["stats"] = {"lines": lines, "files": len(entries)} + + uncertain = [] + risky = [] + shape = [] + classes = set() + for entry in entries: + path = entry["path"] + uncertain += uncertain_reasons(entry, counts) + if purpose == "fix-review": + risky += path_risks(path) + kind = file_class(path) + classes.add(kind) + if kind == "code": + shape.append(f"code-file: {path}") + count = counts.get(path) + if kind == "test" and count and count[1] > count[0]: + shape.append(f"test-lines-removed: {path}") + if deletions: + risky.append("risky-data-deletion") + + if purpose == "fix-review": + reasons = risky + uncertain + shape + limit = FIX_REVIEW_MAX_LINES + else: + reasons = uncertain + # A docs-only change skips Final simplify at any size. + limit = None if classes == {"docs"} else FINAL_SIMPLIFY_MAX_LINES + if limit is not None and lines > limit: + reasons.append(f"too-large: {lines} lines, over {limit}") + return result | {"tier": "full" if reasons else "skip", "reasons": reasons} + + +def main(): + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument( + "--purpose", choices=["fix-review", "final-simplify"], required=True + ) + parser.add_argument("--base", required=True) + parser.add_argument("--head", default="HEAD") + args = parser.parse_args() + print(json.dumps(decide(args.purpose, args.base, args.head))) + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/ai/skills/go/scripts/run-review.py b/ai/skills/go/scripts/run-review.py index da683b9..62d8540 100644 --- a/ai/skills/go/scripts/run-review.py +++ b/ai/skills/go/scripts/run-review.py @@ -759,6 +759,50 @@ def archive_review(source, pr_url, skill_dir): return archive_root / org / repo / filename +def share_token_rows(review_root_descriptor, skill_dir, run_id, since_run_id): + # review-code writes its usage row three directories above the report, which is this attempt's review root. + # Removing the worktree deletes that row. This function copies it to the log at the shared review root. + try: + source = os.open( + "token-usage.jsonl", + os.O_RDONLY | os.O_NOFOLLOW, + dir_fd=review_root_descriptor, + ) + except FileNotFoundError: + return + with open(source, "rb") as log: + lines = log.read().splitlines() + rows = [] + for line in lines: + try: + row = json.loads(line) + except ValueError: + continue + if isinstance(row, dict): + row.update(go_run_id=run_id, since_run_id=since_run_id) + rows.append(json.dumps(row) + "\n") + if not rows: + return + directory = open_directory(skill_dir / ".reviews") + try: + descriptor = os.open( + "token-usage.jsonl", + os.O_WRONLY | os.O_APPEND | os.O_CREAT | os.O_NOFOLLOW, + 0o600, + dir_fd=directory, + ) + finally: + os.close(directory) + try: + # The rows go out in one os.write call, so O_APPEND keeps them together when another review appends at the same time. + # The loop only finishes a short write. + data = "".join(rows).encode() + while data: + data = data[os.write(descriptor, data) :] + finally: + os.close(descriptor) + + def stop_process(process): if process is None: return @@ -938,6 +982,10 @@ def review_attempt( saved_review = attempt / "review.md" review_sha256 = copy_review(source, attempt_descriptor, saved_review.name) archived_review = archive_review(saved_review, args.pr_url, skill_dir) + try: + share_token_rows(review_root_descriptor, skill_dir, run_id, args.since_run) + except OSError as error: + state["token_log_error"] = str(error) state.update( phase="reviewed", next_action="validate-review-fixes", diff --git a/ai/skills/go/scripts/tests/test_review_tier.py b/ai/skills/go/scripts/tests/test_review_tier.py new file mode 100644 index 0000000..477984a --- /dev/null +++ b/ai/skills/go/scripts/tests/test_review_tier.py @@ -0,0 +1,979 @@ +import importlib.util +import json +import os +from pathlib import Path +import re +import subprocess +import sys +import tempfile +import unittest + + +SCRIPT = Path(__file__).resolve().parents[1] / "review-tier.py" +RISKY_KINDS = ( + "risky-migration", + "risky-api", + "risky-wire-format", + "risky-auth", + "risky-infra", + "risky-dependency", + "risky-data-deletion", +) +UNCERTAIN_KINDS = ( + "uncertain-rename", + "uncertain-copy", + "uncertain-deleted-file", + "uncertain-binary", + "uncertain-mode-change", + "uncertain-type-change", + "uncertain-submodule", + "uncertain-symlink", + "uncertain-diff-failed", +) +REASON_KINDS = RISKY_KINDS + UNCERTAIN_KINDS + ( + "code-file", + "test-lines-removed", + "too-large", +) +REASON_PATTERN = re.compile( + r"^(?:%s)(?:: .+)?$" % "|".join(re.escape(kind) for kind in REASON_KINDS) +) +PURPOSES = ("fix-review", "final-simplify") +SPEC = importlib.util.spec_from_file_location("review_tier", SCRIPT) +review_tier = importlib.util.module_from_spec(SPEC) +SPEC.loader.exec_module(review_tier) + +TEST_PATHS = ( + "test/helpers.py", + "tests/helpers.py", + "src/__tests__/widget.js", + "spec/widget_helper.rb", + "testdata/sample.json", + "src/test_widget.py", + "src/widget_test.go", + "src/widget.test.ts", + "src/widget.spec.ts", + "src/widget_spec.rb", + "src/conftest.py", + "ai/skills/go/scripts/tests/test_x.py", + "tests/requirements.txt", +) +DOCS_PATHS = ( + "notes.md", + "guide.mdx", + "guide.rst", + "guide.adoc", + "README", + "README.txt", + "CHANGELOG", + "LICENSE", + "LICENSE-APACHE", +) +PROMPT_PATHS = ( + "ai/skills/x/SKILL.md", + "ai/skills/x/references/notes.md", + "AGENTS.md", + "docs/CLAUDE.md", + "ai/agents/reviewer.md", + "commands/ship.md", + "prompts/summary.md", + "src/handlers/hook.md", + "docs/skills/overview.md", + "GEMINI.md", + ".github/copilot-instructions.md", + "ai/contexts/python.md", + "ai/evals/smallest-change/rule.md", + "ai/skills/foo/prompt.txt", + "ai/skills/support/templates/weekly-log.txt", + "prompts/summary.rst", + "ai/skills/x/tests/SKILL.md", + "tests/AGENTS.md", +) +CODE_PATHS = ( + "src/widget.py", + "Makefile", + "ai/skills/go/scripts/run.py", + "src/testing.py", + "src/tests.py", + "docs_site/page.py", + "docs/example.py", + "docs/conf.py", + "CMakeLists.txt", + "notes.txt", + "src/allowed_hosts.txt", + "src/ab_test.py", + "src/openapi_spec.yaml", + "README.py", +) +RISKY_PATHS = ( + ("risky-migration", "docs/migrations/overview.md"), + ("risky-migration", "tests/data_migrations/test_backfill.py"), + ("risky-migration", "tests/Migrations/test_upper.py"), + ("risky-migration", "tests/alembic/test_env.py"), + ("risky-migration", "tests/fixtures/schema.sql"), + ("risky-migration", "app/migrations/tests/test_backfill.py"), + ("risky-api", "docs/api/usage.md"), + ("risky-api", "guides/API/reference.md"), + ("risky-api", "tests/openapi/test_schema.py"), + ("risky-api", "tests/serializers/test_user.py"), + ("risky-api", "docs/sdk/intro.md"), + ("risky-api", "tests/fixtures/schema.graphql"), + ("risky-wire-format", "tests/fixture.proto"), + ("risky-wire-format", "tests/upper.PROTO"), + ("risky-wire-format", "tests/fixture.avsc"), + ("risky-wire-format", "tests/fixture.capnp"), + ("risky-wire-format", "tests/fixture.thrift"), + ("risky-wire-format", "tests/fixture.fbs"), + ("risky-auth", "tests/auth/test_login.py"), + ("risky-auth", "docs/permissions.md"), + ("risky-auth", "tests/test_access_control.py"), + ("risky-auth", "docs/access-control.md"), + ("risky-auth", "tests/test_rbac.py"), + ("risky-auth", "docs/csrf.md"), + ("risky-auth", "docs/cors.md"), + ("risky-auth", "tests/test_secrets.py"), + ("risky-auth", "docs/credentials.md"), + ("risky-auth", "tests/test_encrypt.py"), + ("risky-auth", "docs/password-reset.md"), + ("risky-auth", "docs/SECURITY.md"), + ("risky-auth", "docs/saml.md"), + ("risky-auth", "docs/sso.md"), + ("risky-auth", "docs/OAuth.md"), + ("risky-infra", "charts/README.md"), + ("risky-infra", "helm/README.md"), + ("risky-infra", "docs/terraform/setup.md"), + ("risky-infra", "k8s/README.md"), + ("risky-infra", "kubernetes/notes.md"), + ("risky-infra", "argocd/README.md"), + ("risky-infra", ".github/CONTRIBUTING.md"), + ("risky-infra", "deploy/README.md"), + ("risky-infra", "infra/README.md"), + ("risky-infra", "tests/main.tf"), + ("risky-infra", "tests/prod.tfvars"), + ("risky-infra", "tests/Dockerfile.ci"), + ("risky-infra", "tests/docker-compose.yml"), + ("risky-infra", "tests/values.yaml"), + ("risky-infra", "tests/values-prod.yml"), + ("risky-dependency", "tests/requirements-dev.txt"), + ("risky-dependency", "tests/requirements.txt"), + ("risky-dependency", "tests/package.json"), + ("risky-dependency", "tests/package-lock.json"), + ("risky-dependency", "tests/pnpm-lock.yaml"), + ("risky-dependency", "tests/yarn.lock"), + ("risky-dependency", "tests/pyproject.toml"), + ("risky-dependency", "tests/poetry.lock"), + ("risky-dependency", "tests/uv.lock"), + ("risky-dependency", "tests/Pipfile"), + ("risky-dependency", "tests/Pipfile.lock"), + ("risky-dependency", "tests/Cargo.toml"), + ("risky-dependency", "tests/Cargo.lock"), + ("risky-dependency", "tests/go.mod"), + ("risky-dependency", "tests/go.sum"), + ("risky-dependency", "tests/Gemfile"), + ("risky-dependency", "tests/Gemfile.lock"), +) +NON_RISKY_LOOKALIKE_PATHS = ( + "docs/rapid/notes.md", + "docs/api.md", + "docs/migration-guide.md", + "docs/charting/notes.md", + "docs/deployment/notes.md", + "docs/values.md", + "docs/sql-style.md", + "docs/protocol.md", + "tests/test_dockerfile_parser.py", + "tests/test_requirements.py", + "tests/test_values.py", +) +DATA_DELETION_STATEMENTS = ( + "DELETE FROM users", + "delete from users", + "DROP TABLE users", + "drop column email", + "DROP DATABASE app", + "DROP INDEX idx_users", + "TRUNCATE TABLE users", + "ALTER TABLE users ADD COLUMN age int", + "rm -rf build", +) + + +def numbered_lines(count, label="line"): + return "".join(f"{label} {number}\n" for number in range(1, count + 1)) + + +class ReviewTierTests(unittest.TestCase): + def setUp(self): + self.temp = tempfile.TemporaryDirectory(prefix="go-review-tier-tests-") + self.addCleanup(self.temp.cleanup) + self.root = Path(self.temp.name).resolve() + self.repo = self.root / "checkout with spaces" + self.repo.mkdir() + self.env = { + key: value + for key, value in os.environ.items() + if not key.startswith("GIT_") + } + self.env.update( + { + "GIT_CONFIG_GLOBAL": os.devnull, + "GIT_CONFIG_SYSTEM": os.devnull, + "GIT_CONFIG_COUNT": "3", + "GIT_CONFIG_KEY_0": "user.name", + "GIT_CONFIG_VALUE_0": "Fixture", + "GIT_CONFIG_KEY_1": "user.email", + "GIT_CONFIG_VALUE_1": "fixture@example.com", + "GIT_CONFIG_KEY_2": "commit.gpgsign", + "GIT_CONFIG_VALUE_2": "false", + } + ) + self.git("init", "-q", "-b", "feature") + self.write("seed.txt", "seed\n") + self.commit("Initial fixture") + + def git(self, *args): + return subprocess.run( + ["git", "-C", str(self.repo), *args], + env=self.env, + check=True, + capture_output=True, + text=True, + ).stdout.strip() + + def write(self, path, content): + target = self.repo / path + target.parent.mkdir(parents=True, exist_ok=True) + if isinstance(content, bytes): + target.write_bytes(content) + else: + target.write_text(content) + + def commit(self, message="Change"): + self.git("add", "-A") + self.git("commit", "-qm", message) + return self.head() + + def head(self): + return self.git("rev-parse", "HEAD") + + def change(self, files=None, base_files=None, removed=()): + if base_files: + for path, content in base_files.items(): + self.write(path, content) + self.commit("Base") + base = self.head() + for path, content in (files or {}).items(): + self.write(path, content) + for path in removed: + (self.repo / path).unlink() + self.commit("Change under review") + return base + + def invoke(self, *args): + return subprocess.run( + [sys.executable, str(SCRIPT), *args], + cwd=self.repo, + env=self.env, + capture_output=True, + text=True, + timeout=30, + ) + + def tier(self, purpose, base, head=None): + args = ["--purpose", purpose, "--base", base] + if head is not None: + args += ["--head", head] + completed = self.invoke(*args) + self.assertEqual(completed.returncode, 0, completed.stderr) + result = json.loads(completed.stdout) + self.assertValidSchema(result) + return result + + def assertValidSchema(self, result): + self.assertEqual( + set(result), {"purpose", "base", "head", "tier", "reasons", "stats"} + ) + self.assertIn(result["tier"], {"skip", "full"}) + self.assertIsInstance(result["reasons"], list) + for reason in result["reasons"]: + self.assertIsInstance(reason, str) + self.assertRegex(reason, REASON_PATTERN) + self.assertEqual(set(result["stats"]), {"lines", "files"}) + self.assertIsInstance(result["stats"]["lines"], int) + self.assertIsInstance(result["stats"]["files"], int) + + def assertHasReason(self, result, kind, detail=None): + matches = [ + reason + for reason in result["reasons"] + if reason.startswith(kind) and (detail is None or detail in reason) + ] + self.assertTrue(matches, f"no {kind} reason with {detail!r} in {result}") + + def assertLacksReason(self, result, kind): + self.assertFalse( + [reason for reason in result["reasons"] if reason.startswith(kind)], + f"unexpected {kind} reason in {result}", + ) + + def assertTier(self, result, tier): + self.assertEqual(result["tier"], tier, result) + + def make_executable(self, path, message): + os.chmod(self.repo / path, 0o755) + self.git("add", "-A") + self.git("update-index", "--chmod=+x", path) + self.git("commit", "-qm", message) + + def add_one(self, path, content=None, purpose="fix-review"): + base = self.change({path: numbered_lines(3) if content is None else content}) + return self.tier(purpose, base) + + def test_small_migration_change_requires_full_fix_review(self): + base = self.change({"app/migrations/0002_add_column.py": numbered_lines(3)}) + + result = self.tier("fix-review", base) + + self.assertTier(result, "full") + self.assertHasReason(result, "risky-migration") + + def test_new_test_under_migrations_requires_full_fix_review_without_code_file(self): + base = self.change({"app/migrations/tests/test_backfill.py": numbered_lines(3)}) + + result = self.tier("fix-review", base) + + self.assertTier(result, "full") + self.assertHasReason(result, "risky-migration") + self.assertLacksReason(result, "code-file") + + def test_rename_of_twelve_unchanged_files_requires_full_for_both_purposes(self): + originals = { + f"src/module_{number}.py": numbered_lines(5, f"module {number}") + for number in range(12) + } + for path, content in originals.items(): + self.write(path, content) + self.commit("Base") + base = self.head() + (self.repo / "lib").mkdir() + for number in range(12): + self.git("mv", f"src/module_{number}.py", f"lib/module_{number}.py") + self.commit("Move modules") + + for purpose in PURPOSES: + with self.subTest(purpose=purpose): + result = self.tier(purpose, base) + + self.assertTier(result, "full") + self.assertHasReason(result, "uncertain-rename") + self.assertEqual(result["stats"], {"lines": 0, "files": 12}) + + def test_rename_into_risky_directory_reports_the_risk(self): + base = self.change( + {"docs/migrations/notes.md": numbered_lines(5, "note")}, + base_files={"docs/notes.md": numbered_lines(5, "note")}, + removed=("docs/notes.md",), + ) + + result = self.tier("fix-review", base) + + self.assertHasReason(result, "uncertain-rename") + self.assertHasReason(result, "risky-migration") + + def test_copy_of_modified_file_requires_full_for_both_purposes(self): + original = numbered_lines(30, "original") + base = self.change( + {"src/orig.py": original + "extra\n", "lib/copy.py": original}, + base_files={"src/orig.py": original}, + ) + + for purpose in PURPOSES: + with self.subTest(purpose=purpose): + result = self.tier(purpose, base) + + self.assertTier(result, "full") + self.assertHasReason(result, "uncertain-copy") + + def test_deleted_lines_in_code_file_require_full_fix_review(self): + base = self.change( + {"src/app.py": numbered_lines(7)}, + base_files={"src/app.py": numbered_lines(10)}, + ) + + result = self.tier("fix-review", base) + + self.assertTier(result, "full") + self.assertHasReason(result, "code-file", "src/app.py") + + def test_deleted_lines_in_test_file_require_full_fix_review(self): + base = self.change( + {"tests/test_app.py": numbered_lines(7)}, + base_files={"tests/test_app.py": numbered_lines(10)}, + ) + + result = self.tier("fix-review", base) + + self.assertTier(result, "full") + self.assertHasReason(result, "test-lines-removed") + + def test_deleted_lines_in_docs_file_skip_fix_review(self): + base = self.change( + {"docs/guide.md": numbered_lines(7)}, + base_files={"docs/guide.md": numbered_lines(10)}, + ) + + result = self.tier("fix-review", base) + + self.assertTier(result, "skip") + + def test_five_deleted_lines_in_code_file_skip_final_simplify(self): + base = self.change( + {"src/app.py": numbered_lines(5)}, + base_files={"src/app.py": numbered_lines(10)}, + ) + + result = self.tier("final-simplify", base) + + self.assertTier(result, "skip") + + def test_test_file_with_equal_added_and_removed_lines_skips_fix_review(self): + base = self.change( + {"tests/test_app.py": numbered_lines(3, "new") + numbered_lines(7, "old")}, + base_files={"tests/test_app.py": numbered_lines(10, "old")}, + ) + + result = self.tier("fix-review", base) + + self.assertTier(result, "skip") + self.assertLacksReason(result, "test-lines-removed") + + def test_test_lines_removed_is_judged_per_file_not_over_the_whole_diff(self): + base = self.change( + { + "tests/test_shrunk.py": numbered_lines(7), + "tests/test_grown.py": numbered_lines(20), + }, + base_files={"tests/test_shrunk.py": numbered_lines(10)}, + ) + + result = self.tier("fix-review", base) + + self.assertTier(result, "full") + self.assertHasReason(result, "test-lines-removed") + + def test_removed_test_lines_do_not_affect_final_simplify(self): + base = self.change( + {"tests/test_app.py": numbered_lines(5)}, + base_files={"tests/test_app.py": numbered_lines(10)}, + ) + + result = self.tier("final-simplify", base) + + self.assertTier(result, "skip") + + def test_modified_line_counts_as_one_added_and_one_removed(self): + base = self.change( + {"src/app.py": "changed\n" + numbered_lines(4)}, + base_files={"src/app.py": "original\n" + numbered_lines(4)}, + ) + + result = self.tier("fix-review", base) + + self.assertEqual(result["stats"], {"lines": 2, "files": 1}) + + def test_deleted_file_requires_full_for_both_purposes(self): + base = self.change( + base_files={"docs/old.md": numbered_lines(3)}, removed=("docs/old.md",) + ) + + for purpose in PURPOSES: + with self.subTest(purpose=purpose): + result = self.tier(purpose, base) + + self.assertTier(result, "full") + self.assertHasReason(result, "uncertain-deleted-file") + + def test_deleted_migration_file_reports_the_risk(self): + base = self.change( + base_files={"app/migrations/0001_initial.py": numbered_lines(3)}, + removed=("app/migrations/0001_initial.py",), + ) + + result = self.tier("fix-review", base) + + self.assertHasReason(result, "risky-migration") + + def test_binary_file_requires_full_for_both_purposes(self): + base = self.change({"docs/logo.png": b"\x89PNG\r\n\x1a\n\x00\x00\x00\x0dIHDR"}) + + for purpose in PURPOSES: + with self.subTest(purpose=purpose): + result = self.tier(purpose, base) + + self.assertTier(result, "full") + self.assertHasReason(result, "uncertain-binary") + + def test_binary_file_contributes_no_lines(self): + base = self.change( + { + "docs/logo.png": b"\x89PNG\r\n\x1a\n\x00\x00\x00\x0dIHDR", + "tests/test_logo.py": numbered_lines(4), + } + ) + + result = self.tier("fix-review", base) + + self.assertEqual(result["stats"], {"lines": 4, "files": 2}) + + def test_mode_change_only_requires_full_for_both_purposes(self): + self.write("docs/example.sh", numbered_lines(3)) + self.commit("Base") + base = self.head() + self.make_executable("docs/example.sh", "Make example executable") + + for purpose in PURPOSES: + with self.subTest(purpose=purpose): + result = self.tier(purpose, base) + + self.assertTier(result, "full") + self.assertHasReason(result, "uncertain-mode-change") + + def test_mode_change_with_content_change_requires_full(self): + self.write("docs/example.sh", numbered_lines(3)) + self.commit("Base") + base = self.head() + self.write("docs/example.sh", numbered_lines(4)) + self.make_executable("docs/example.sh", "Edit example and make it executable") + + result = self.tier("fix-review", base) + + self.assertTier(result, "full") + self.assertHasReason(result, "uncertain-mode-change") + + def test_new_executable_file_is_not_a_mode_change(self): + base = self.head() + self.write("tests/run_fixture.sh", numbered_lines(3)) + self.make_executable("tests/run_fixture.sh", "Add executable fixture") + + result = self.tier("fix-review", base) + + self.assertTier(result, "skip") + self.assertLacksReason(result, "uncertain-mode-change") + + def test_added_symlink_requires_full_for_both_purposes(self): + self.write("docs/guide.md", numbered_lines(3)) + self.commit("Base") + base = self.head() + os.symlink("guide.md", self.repo / "docs/link.md") + self.commit("Add symlink") + + for purpose in PURPOSES: + with self.subTest(purpose=purpose): + result = self.tier(purpose, base) + + self.assertTier(result, "full") + self.assertHasReason(result, "uncertain-symlink") + + def test_regular_file_replaced_by_symlink_requires_full_with_type_change(self): + self.write("docs/guide.md", numbered_lines(3)) + self.write("docs/target.md", numbered_lines(3, "target")) + self.commit("Base") + base = self.head() + (self.repo / "docs/target.md").unlink() + os.symlink("guide.md", self.repo / "docs/target.md") + self.commit("Replace file with symlink") + + for purpose in PURPOSES: + with self.subTest(purpose=purpose): + result = self.tier(purpose, base) + + self.assertTier(result, "full") + self.assertHasReason(result, "uncertain-type-change") + + def test_added_submodule_requires_full_for_both_purposes(self): + base = self.head() + self.git("update-index", "--add", "--cacheinfo", f"160000,{base},vendor/lib") + self.git("commit", "-qm", "Add submodule") + + for purpose in PURPOSES: + with self.subTest(purpose=purpose): + result = self.tier(purpose, base) + + self.assertTier(result, "full") + self.assertHasReason(result, "uncertain-submodule") + + def test_small_readme_change_skips_fix_review(self): + base = self.change({"README.md": numbered_lines(3)}) + + result = self.tier("fix-review", base) + + self.assertTier(result, "skip") + + def test_new_test_file_of_100_lines_skips_fix_review(self): + base = self.change({"tests/test_big.py": numbered_lines(100)}) + + result = self.tier("fix-review", base) + + self.assertTier(result, "skip") + + def test_new_test_file_of_101_lines_requires_full_fix_review(self): + base = self.change({"tests/test_big.py": numbered_lines(101)}) + + result = self.tier("fix-review", base) + + self.assertTier(result, "full") + self.assertHasReason(result, "too-large") + + def test_fix_review_size_threshold_sums_lines_across_files(self): + base = self.change( + { + "tests/test_one.py": numbered_lines(50), + "docs/guide.md": numbered_lines(51), + } + ) + + result = self.tier("fix-review", base) + + self.assertTier(result, "full") + self.assertHasReason(result, "too-large") + + def test_test_file_adding_delete_from_requires_full_fix_review(self): + base = self.change({"tests/test_cleanup.py": 'sql = "DELETE FROM users"\n'}) + + result = self.tier("fix-review", base) + + self.assertTier(result, "full") + self.assertHasReason(result, "risky-data-deletion") + + def test_added_data_deletion_line_that_starts_with_plus_plus_is_risky(self): + base = self.change({"tests/test_cleanup.py": "++ DROP TABLE users;\n"}) + + result = self.tier("fix-review", base) + + self.assertTier(result, "full") + self.assertHasReason(result, "risky-data-deletion") + + def test_external_diff_driver_does_not_hide_added_data_deletion(self): + base = self.change({"tests/test_cleanup.py": 'sql = "DELETE FROM users"\n'}) + self.env["GIT_EXTERNAL_DIFF"] = "true" + + result = self.tier("fix-review", base) + + self.assertTier(result, "full") + self.assertHasReason(result, "risky-data-deletion") + + def test_removed_data_deletion_statement_is_not_risky(self): + base = self.change( + {"tests/test_cleanup.py": "SELECT 1\nkeep\n"}, + base_files={"tests/test_cleanup.py": "DELETE FROM users\nkeep\n"}, + ) + + result = self.tier("fix-review", base) + + self.assertTier(result, "skip") + self.assertLacksReason(result, "risky-data-deletion") + + def test_data_deletion_words_in_a_file_name_are_not_risky(self): + result = self.add_one("tests/test_drop table notes.py") + + self.assertTier(result, "skip") + self.assertLacksReason(result, "risky-data-deletion") + + def test_final_simplify_skips_20_changed_lines_of_code(self): + base = self.change({"src/app.py": numbered_lines(20)}) + + result = self.tier("final-simplify", base) + + self.assertTier(result, "skip") + + def test_final_simplify_requires_full_for_21_changed_lines_of_code(self): + base = self.change({"src/app.py": numbered_lines(21)}) + + result = self.tier("final-simplify", base) + + self.assertTier(result, "full") + self.assertHasReason(result, "too-large") + + def test_final_simplify_size_threshold_counts_docs_lines_next_to_code(self): + base = self.change( + {"src/app.py": numbered_lines(1), "docs/guide.md": numbered_lines(20)} + ) + + result = self.tier("final-simplify", base) + + self.assertTier(result, "full") + self.assertHasReason(result, "too-large") + + def test_final_simplify_skips_large_docs_only_change(self): + base = self.change( + {"docs/guide.md": numbered_lines(150), "README.md": numbered_lines(150)} + ) + + result = self.tier("final-simplify", base) + + self.assertTier(result, "skip") + + def test_final_simplify_does_not_treat_large_test_change_as_docs(self): + base = self.change({"tests/test_big.py": numbered_lines(30)}) + + result = self.tier("final-simplify", base) + + self.assertTier(result, "full") + self.assertHasReason(result, "too-large") + + def test_final_simplify_does_not_treat_large_prompt_change_as_docs(self): + base = self.change({"ai/skills/x/SKILL.md": numbered_lines(30)}) + + result = self.tier("final-simplify", base) + + self.assertTier(result, "full") + self.assertHasReason(result, "too-large") + + def test_final_simplify_skips_small_migration_change(self): + base = self.change({"app/migrations/0002_add_column.py": numbered_lines(3)}) + + result = self.tier("final-simplify", base) + + self.assertTier(result, "skip") + + def test_final_simplify_skips_small_data_deletion_change(self): + base = self.change({"tests/test_cleanup.py": 'sql = "DROP TABLE users"\n'}) + + result = self.tier("final-simplify", base) + + self.assertTier(result, "skip") + + def test_final_simplify_omits_risky_reasons_from_full_result(self): + base = self.change({"app/migrations/0003_big.py": numbered_lines(25)}) + + result = self.tier("final-simplify", base) + + self.assertTier(result, "full") + for kind in RISKY_KINDS: + self.assertLacksReason(result, kind) + + def test_final_simplify_requires_full_for_uncertain_docs_change(self): + base = self.change( + {"docs/guide.md": numbered_lines(150)}, + base_files={"docs/old.md": numbered_lines(3)}, + removed=("docs/old.md",), + ) + + result = self.tier("final-simplify", base) + + self.assertTier(result, "full") + self.assertHasReason(result, "uncertain-deleted-file") + + def test_fix_review_lists_every_reason_that_applies(self): + base = self.change( + { + "app/migrations/0003_big.py": numbered_lines(101), + "tests/test_old.py": numbered_lines(1), + }, + base_files={ + "docs/old.md": numbered_lines(3), + "tests/test_old.py": numbered_lines(5), + }, + removed=("docs/old.md",), + ) + + result = self.tier("fix-review", base) + + self.assertTier(result, "full") + for kind in ( + "risky-migration", + "uncertain-deleted-file", + "code-file", + "test-lines-removed", + "too-large", + ): + with self.subTest(kind=kind): + self.assertHasReason(result, kind) + + def test_base_equal_to_head_skips_both_purposes(self): + for purpose in PURPOSES: + with self.subTest(purpose=purpose): + result = self.tier(purpose, "HEAD") + + self.assertTier(result, "skip") + self.assertEqual(result["reasons"], []) + self.assertEqual(result["stats"], {"lines": 0, "files": 0}) + + def test_empty_commit_range_skips(self): + base = self.head() + self.git("commit", "--allow-empty", "-qm", "Empty") + + result = self.tier("fix-review", base) + + self.assertTier(result, "skip") + + def test_unresolvable_base_exits_zero_with_full_and_diff_failed(self): + for purpose in PURPOSES: + with self.subTest(purpose=purpose): + result = self.tier(purpose, "deadbeef") + + self.assertTier(result, "full") + self.assertHasReason(result, "uncertain-diff-failed") + self.assertEqual( + (result["purpose"], result["base"], result["head"]), + (purpose, "deadbeef", "HEAD"), + ) + + def test_unresolvable_head_exits_zero_with_full_and_diff_failed(self): + result = self.tier("fix-review", "HEAD", "deadbeef") + + self.assertTier(result, "full") + self.assertHasReason(result, "uncertain-diff-failed") + + def test_multi_commit_range_counts_changes_from_every_commit(self): + base = self.head() + self.write("src/one.py", numbered_lines(2)) + self.commit("First") + self.write("tests/test_two.py", numbered_lines(3)) + self.commit("Second") + + result = self.tier("fix-review", base) + + self.assertEqual(result["stats"], {"lines": 5, "files": 2}) + self.assertHasReason(result, "code-file", "src/one.py") + + def test_explicit_head_excludes_later_commits(self): + base = self.head() + self.write("tests/test_a.py", numbered_lines(3)) + head = self.commit("Test") + self.write("src/late.py", numbered_lines(3)) + self.commit("Later code") + + result = self.tier("fix-review", base, head) + + self.assertTier(result, "skip") + self.assertEqual(result["stats"], {"lines": 3, "files": 1}) + + def test_diff_compares_trees_directly_rather_than_from_merge_base(self): + self.git("checkout", "-qb", "side") + self.write("docs/side.md", numbered_lines(3, "side")) + side = self.commit("Side change") + self.git("checkout", "-q", "feature") + self.write("docs/main.md", numbered_lines(3, "feature")) + self.commit("Feature change") + + result = self.tier("fix-review", side) + + self.assertTier(result, "full") + self.assertHasReason(result, "uncertain-deleted-file") + + def test_mixed_docs_and_tests_of_60_lines_skip_fix_review(self): + base = self.change( + {"docs/guide.md": numbered_lines(30), "tests/test_app.py": numbered_lines(30)} + ) + + result = self.tier("fix-review", base) + + self.assertTier(result, "skip") + + def test_mixed_change_with_one_code_file_names_only_that_file(self): + base = self.change( + { + "docs/guide.md": numbered_lines(30), + "tests/test_app.py": numbered_lines(29), + "src/app.py": numbered_lines(1), + } + ) + + result = self.tier("fix-review", base) + + self.assertTier(result, "full") + self.assertHasReason(result, "code-file", "src/app.py") + code_reasons = [r for r in result["reasons"] if r.startswith("code-file")] + self.assertFalse( + [r for r in code_reasons if "docs/guide.md" in r or "tests/test_app.py" in r], + result, + ) + + def test_output_echoes_purpose_base_and_given_head(self): + self.write("docs/guide.md", numbered_lines(3)) + self.commit("Docs") + + for purpose in PURPOSES: + with self.subTest(purpose=purpose): + result = self.tier(purpose, "HEAD~1", "HEAD") + + self.assertEqual( + (result["purpose"], result["base"], result["head"]), + (purpose, "HEAD~1", "HEAD"), + ) + + def test_output_head_defaults_to_head(self): + self.write("docs/guide.md", numbered_lines(3)) + self.commit("Docs") + + result = self.tier("fix-review", "HEAD~1") + + self.assertEqual(result["head"], "HEAD") + + def test_output_counts_a_rename_as_one_file(self): + base = self.change( + {"docs/new-name.md": numbered_lines(10, "same")}, + base_files={"docs/old-name.md": numbered_lines(10, "same")}, + removed=("docs/old-name.md",), + ) + + result = self.tier("fix-review", base) + + self.assertEqual(result["stats"]["files"], 1) + + def test_usage_errors_exit_2(self): + cases = { + "missing purpose": ("--base", "HEAD"), + "unknown purpose": ("--purpose", "review", "--base", "HEAD"), + "missing base": ("--purpose", "fix-review"), + } + for name, args in cases.items(): + with self.subTest(case=name): + completed = self.invoke(*args) + + self.assertEqual(completed.returncode, 2, completed.stderr) + self.assertIn("usage:", completed.stderr) + + +class PathRuleTests(unittest.TestCase): + def assert_class(self, paths, expected): + for path in paths: + with self.subTest(path=path): + self.assertEqual(review_tier.file_class(path), expected) + + def test_prompt_markdown_is_code(self): + self.assert_class(PROMPT_PATHS, "code") + + def test_test_paths_are_tests(self): + self.assert_class(TEST_PATHS, "test") + + def test_docs_paths_are_docs(self): + self.assert_class(DOCS_PATHS, "docs") + + def test_code_paths_are_code(self): + self.assert_class(CODE_PATHS, "code") + + def test_risky_docs_and_test_paths_report_their_risk(self): + for kind, path in RISKY_PATHS: + with self.subTest(kind=kind, path=path): + self.assertNotEqual(review_tier.file_class(path), "code") + risks = review_tier.path_risks(path) + self.assertTrue(any(risk.startswith(kind) for risk in risks), risks) + + def test_risky_lookalike_paths_report_no_risk(self): + for path in NON_RISKY_LOOKALIKE_PATHS: + with self.subTest(path=path): + self.assertNotEqual(review_tier.file_class(path), "code") + self.assertEqual(review_tier.path_risks(path), []) + + def test_data_deletion_statements_match(self): + for statement in DATA_DELETION_STATEMENTS: + with self.subTest(statement=statement): + self.assertTrue( + review_tier.DATA_DELETION.search(f'command = "{statement}"') + ) + + +if __name__ == "__main__": + unittest.main() diff --git a/ai/skills/go/scripts/tests/test_run_review.py b/ai/skills/go/scripts/tests/test_run_review.py index 67e213c..cf17922 100644 --- a/ai/skills/go/scripts/tests/test_run_review.py +++ b/ai/skills/go/scripts/tests/test_run_review.py @@ -133,6 +133,13 @@ review_root = Path(os.environ.get("REVIEW_CODE_REVIEW_DIR") or global_review_root) if mode == "ignores_review_override": review_root = global_review_root +if "REVIEW_TEST_TOKEN_LOG" in os.environ: + review_root.mkdir(parents=True, exist_ok=True) + child_log = review_root / "token-usage.jsonl" + if os.environ.get("REVIEW_TEST_TOKEN_LOG_LINK"): + child_log.symlink_to(os.environ["REVIEW_TEST_TOKEN_LOG_LINK"]) + else: + child_log.write_bytes(os.environb[b"REVIEW_TEST_TOKEN_LOG"]) review_file = review_root / "review.md" if mode == "misleading_artifact": review_file = review_root / "unrelated-org/other-repo/pr-999.md" @@ -282,10 +289,12 @@ def git(self, *args): text=True, ).stdout.strip() - def invoke(self, *args, mode="success", edit=False, cwd=None): + def invoke(self, *args, mode="success", edit=False, cwd=None, token_log=None): env = dict( self.env, REVIEW_TEST_MODE=mode, REVIEW_TEST_EDIT="yes" if edit else "no" ) + if token_log is not None: + env["REVIEW_TEST_TOKEN_LOG"] = token_log return subprocess.run( [sys.executable, str(RUNNER), *args], cwd=cwd or self.repo, @@ -1689,6 +1698,110 @@ def test_since_run_appends_to_the_earlier_review_and_archives_both(self): self.assertEqual(state["archive_review_file"], str(self.archive_file)) self.assertEqual(self.archive_file.read_text(), expected) + def shared_token_log(self): + return self.review_root / "token-usage.jsonl" + + def shared_token_rows(self): + return [ + json.loads(line) + for line in self.shared_token_log().read_text().splitlines() + ] + + def test_review_appends_its_token_rows_to_the_shared_log(self): + self.shared_token_log().write_text('{"identifier": "earlier"}\n') + rows = '{"identifier": "123", "total_tokens": 5}\n{"identifier": "123", "total_tokens": 7}\n' + + state = self.assert_success(self.run_review(token_log=rows)) + + self.assertEqual( + self.shared_token_rows(), + [ + {"identifier": "earlier"}, + { + "identifier": "123", + "total_tokens": 5, + "go_run_id": state["run_id"], + "since_run_id": None, + }, + { + "identifier": "123", + "total_tokens": 7, + "go_run_id": state["run_id"], + "since_run_id": None, + }, + ], + ) + self.assertNotIn("token_log_error", state) + + def test_since_run_token_rows_name_the_earlier_run(self): + earlier, result = self.review_since_earlier_run( + mode="append_seed", token_log='{"review_mode": "delta"}\n' + ) + state = self.assert_success(result) + + self.assertEqual( + self.shared_token_rows()[-1], + { + "review_mode": "delta", + "go_run_id": state["run_id"], + "since_run_id": earlier["run_id"], + }, + ) + + def test_review_without_a_child_token_log_leaves_the_shared_log_alone(self): + self.assert_success(self.run_review()) + + self.assertFalse(self.shared_token_log().exists()) + + def test_reused_review_does_not_append_its_rows_again(self): + self.assert_success(self.run_review(token_log='{"total_tokens": 1}\n')) + self.assert_success(self.run_review(token_log='{"total_tokens": 1}\n')) + + self.assertEqual(len(self.shared_token_rows()), 1) + + def test_failed_review_does_not_append_its_token_rows(self): + for mode in ("blocked", "change_head"): + with self.subTest(mode=mode): + result = self.run_review(mode=mode, token_log='{"total_tokens": 1}\n') + self.assertNotEqual(result.returncode, 0, result.stdout) + self.assertFalse(self.shared_token_log().exists()) + + def test_token_rows_that_are_not_json_objects_are_dropped(self): + rows = '{"total_tokens": 1}\nnot json\n[1, 2]\n\n{"total_tokens": 2}\n' + + self.assert_success(self.run_review(token_log=rows)) + + self.assertEqual( + [row["total_tokens"] for row in self.shared_token_rows()], [1, 2] + ) + + def test_undecodable_token_rows_are_dropped_and_the_review_still_succeeds(self): + rows = '{"total_tokens": 1}\n\udcff\udcfe{"total_tokens": 2}\n' + + state = self.assert_success(self.run_review(token_log=rows)) + + self.assertEqual( + [row["total_tokens"] for row in self.shared_token_rows()], [1] + ) + self.assertNotIn("token_log_error", state) + + def test_shared_log_symlink_is_not_followed_and_the_review_still_succeeds(self): + self.shared_token_log().symlink_to(self.protected) + + state = self.assert_success(self.run_review(token_log='{"total_tokens": 1}\n')) + + self.assertEqual(self.protected.read_text(), "Preserve this content.\n") + self.assertIn("token_log_error", state) + self.assertEqual(self.state()["phase"], "reviewed") + + def test_child_token_log_symlink_is_not_read(self): + self.env["REVIEW_TEST_TOKEN_LOG_LINK"] = str(self.protected) + + state = self.assert_success(self.run_review(token_log="")) + + self.assertFalse(self.shared_token_log().exists()) + self.assertIn("token_log_error", state) + def test_since_run_child_returning_the_untouched_seed_fails(self): earlier, result = self.review_since_earlier_run(mode="untouched_seed") self.assertNotEqual(result.returncode, 0, result.stdout)