diff --git a/.agents/skills/autoreview/BUILD.bazel b/.agents/skills/autoreview/BUILD.bazel new file mode 100644 index 000000000..90b30f620 --- /dev/null +++ b/.agents/skills/autoreview/BUILD.bazel @@ -0,0 +1,8 @@ +filegroup( + name = "autoreview", + srcs = [ + "SKILL.md", + "lenses.md", + ], + visibility = ["//visibility:public"], +) diff --git a/.agents/skills/autoreview/SKILL.md b/.agents/skills/autoreview/SKILL.md new file mode 100644 index 000000000..6d27c85cc --- /dev/null +++ b/.agents/skills/autoreview/SKILL.md @@ -0,0 +1,132 @@ +--- +name: autoreview +description: >- + Review a SubmitQueue change from independent lenses after mechanical checks. + Use when the user asks for an autoreview, a review of the current changes or + diff, a pull request review, or a review against AGENTS.md. +--- + +# Autoreview + +Review the change. Do not edit it, do not commit, and do not post to the pull request. Findings are advice. A clean verdict covers only the target below, and a review that did not finish is not clean. + +Run the four stages in order. `AGENTS.md` is the source of truth; `CLAUDE.md` is a copy, so do not read it separately. + +## 1. Scope and intent + +Run `bazel run //.agents/skills/autoreview/scripts:review-scope --` from the repository, followed by any arguments below. The tool is read-only and does not fetch. Its source is [`scripts/review-scope.py`](scripts/review-scope.py). + +- By default it reviews committed changes since the merge-base with `origin/main`, plus staged, unstaged, and untracked files. +- `--base REF` when the user names a different base. +- `--range REF` when the target is committed work only; dirty work is excluded. +- `--commit REV` when the user names one commit. Dirty work is excluded. +- `--uncommitted` when the user wants only uncommitted work. +- When the user names a pull request, read it with `gh pr view N --json baseRefName,baseRefOid,headRefOid,title,body`. Verify local `HEAD` equals `headRefOid` and `baseRefOid` exists locally, then pass `--range ` so unrelated dirty work is excluded. Do not fetch. If either check fails, stop and report the review as incomplete rather than reviewing a different revision. + +Read the full commit messages (`git log ..HEAD`) or the pull request body, not just the subjects the script prints. Write a one-line intent and record whether it came from the PR body, commit messages, or the user. Use `unspecified` when none states an intent; do not invent one. Hold the change to that intent, including changes that do not belong in it. + +The script's `## smells` lines are leads. A hit is not a finding until a lens confirms it in the code. `## hints` means an interface declaration or mock-generation source changed. + +Read context from the reviewed snapshot. Branch and uncommitted targets use the working tree. A range target uses `git show HEAD:` and its merge-base for before-state; a commit target uses `git show :` and its parent. Do not open the working-tree copy for a committed-only target when dirty work is excluded or the named commit is not `HEAD`. If required context cannot be read from the Git objects, mark coverage incomplete. + +## 2. Mechanical checks + +Run only the checks that match the file list. + +- Run checks only when the filesystem materializes the reviewed target. A branch or uncommitted review uses the current tree. For `--range`, any dirty work means all checks are skipped. For `--commit`, all checks are skipped unless the named commit equals `HEAD` and the tree is clean. Record `skipped: reviewed target is not materialized` and mark verification limited; never report results from a different tree. +- `make check-gazelle` when `go` or `build` is not `none`. +- `make check-mocks` when `## hints` is not `none`, or a file under `mock/` changed. +- `make check-tidy` when `MODULE.bazel`, `go.mod`, or `go.sum` changed. +- `make proto` when a `.proto`, `protopb/`, `*.pb.go`, or `*.pb.yarpc.go` file changed. +- `make lint` when anything other than `docs` changed. +- `bazel build` (or `./tool/bazel build` when `bazel` is not on `PATH`) on each package under `## packages`: append `:all`, so `//` becomes `//:all` and `//pkg` becomes `//pkg:all`. +- For each affected package, query `tests(:all)`. Run `bazel test` only on the returned labels. An empty query is `not applicable`, not a failure. Exclude `integration` and `e2e` tags unless a changed file is an integration or e2e test. Do not test `//...`. + +`make lint`, `make check-gazelle`, `make check-mocks`, `make check-tidy`, and `make proto` rewrite files and then require a clean tree. Skip them when `dirty: true`, record `skipped: dirty tree`, and mark verification limited. When the tree is clean, record `git status --porcelain` first and compare it after every command. A successful command that leaves changes is a failed stale-output check. Restore the tracked paths with `git restore` and remove only untracked paths that were not in the earlier status, whether the command failed or merely left edits. Do not leave the rewrite in the worktree. + +`bazel test` does not edit source. Run it on a dirty tree too. + +A failed check is a fact. Report the target and the first error. Do not turn a smell into a failed check. + +## 3. Independent lenses + +Read [`lenses.md`](lenses.md). Run a lens only when its files changed: + +- **Architecture and boundaries** — `go`, `proto`, `sql`, `build`, or `config` is not `none`. +- **Correctness and failure modes** — `go`, `tests`, `python`, `shell`, `proto`, `sql`, or `config` is not `none`. +- **Simplicity and clarity** — `go`, `tests`, `python`, `shell`, `proto`, `sql`, `build`, `config`, or `other` is not `none`. +- **Tests and hermeticness** — `go`, `tests`, `python`, `shell`, `build`, `config`, `generated`, or `other` is not `none`. +- **Operability** — the diff changes logging, metrics, or an error returned to a caller. +- **Docs and RFCs** — `docs` is not `none`. + +Generated files (`protopb/`, `mock/`, `*_mock.go`, `*.pb.go`) are not a lens of their own. Each lens checks whether they were regenerated, and does not review them line by line. + +When you can start subagents, start one per applicable lens in parallel. Give each the lens brief, the normalized intent and its source, the `review-scope` output, the diff, and nothing from the conversation that wrote the change. When you cannot, finish one lens and set it aside before reading the next brief. + +Pass a lens the smells that match it: `secondary-index` and `extension-factory` to architecture; `narration-comment` to simplicity; `sleep`, `error-string-assert`, `external-test-package`, `repo-root`, and `shell-script` to tests and hermeticness; `formatted-log` to operability. The lens confirms or drops each one. + +If the diff is too large to pass, give the reviewer the file list and have it read the files. Anything unread is listed in the report, and the verdict is incomplete. + +## 4. Verify and consolidate + +- Admit a finding only when the changed code creates a reachable material impact. It must state the impact, the shortest reproducing scenario or static caller trace, supporting evidence, the smallest safe fix in this change, and a verification step. If impact or reachability is not established, turn it into a question or drop it. +- Open every cited location and enough surrounding code to verify the causal claim. Ordering findings need the relevant producer and consumer; concurrency findings need a concrete interleaving; rollout findings need the incompatible old/new combination; missing-test findings need the uncovered observable behavior. +- Consolidate by root cause, not wording. Merge multiple symptoms and their evidence into one finding. Resolve contradictory recommendations against the code, intent, and `AGENTS.md`; if the evidence does not decide, keep one question. +- Severity is based on consequence: **Blocker** means reproducible merge-unsafe behavior such as data loss, incorrect state, outage, security exposure, or incompatible rollout. **Should-fix** means a reachable material defect with a bounded fix appropriate to this change. **Nit** means style, preference, or process consistency without behavioral impact. Confidence is high, medium, or low. +- A question is a missing fact, not a severity. A material unresolved question makes coverage incomplete. +- Nits stay out unless the user asked for a wider pass. +- Sweep `AGENTS.md` for rules no lens covered: layout and import paths, committed proto generation, entity field comments, string enums, int64 milliseconds, Makefile target order, and commit shape when the range has commits (conventional, subject under 70 characters, one line per paragraph, no `Co-Authored-By` / `Co-authored-by`). +- Commit-shape findings must quote the offending subject or trailer from `git log ..HEAD` (or the named commit). Drop any claim that contradicts the message body you just read. A `Co-authored-by` trailer is a **Nit** (process/policy), never Blocker or Should-fix, and belongs under Nits only when the user asked for nits or for a commit-message pass. +- List files in scope that were not read, checks that were skipped, and lenses that did not finish. + +## Report + +Lead with three separate statuses. **Verdict** is `clean` or `findings`; a blocker, should-fix, or failed check makes it `findings`. **Coverage** is `complete` or `incomplete`; unread files, unfinished lenses, or material unresolved questions make it `incomplete`. **Verification** is `complete` or `limited`; any applicable skipped check makes it `limited`. Call the review clean without qualification only when all three are clean/complete/complete. + +Within each priority, group findings by the concern they affect, not by which reviewer found them. Use these dimensions when applicable: + +- **Architecture and contracts** — design, boundaries, separation of concerns, and interface contracts. +- **Correctness and failure modes** — behavior, state transitions, ordering, retries, and concrete bugs. +- **Simplicity and clarity** — complexity, naming, comments, terminology, and references. +- **Tests and hermeticness** — behavioral coverage, isolation, deterministic inputs, and build metadata. +- **Operability** — errors, logs, metrics, and incident diagnosis. +- **Documentation and RFCs** — decisions, alternatives, status, links, and prose conventions that do not fit a more specific dimension. + +Use only dimensions with findings. Documentation findings about architecture or pipeline behavior stay under the corresponding substantive dimension; do not put every documentation defect under **Documentation and RFCs**. + +```markdown +# Autoreview + +Scope: | Intent: () +Verdict: clean | findings | Coverage: complete | incomplete | Verification: complete | limited + +## Merge blockers +### +- () `path:line` + - Impact: + - Evidence: + - Fix: + - Verify: + +## Should fix +### +- () `path:line` + - Impact: + - Evidence: + - Fix: + - Verify: + +## Questions requiring a decision +## Verification +- Passed: +- Failed: +- Skipped: +## Not reviewed + +--- +_Generated with the repository's `.agents/skills/autoreview` workflow; findings were rechecked against at [ plus working tree]._ +``` + +Always end the report with the attribution footer shown above. Include a **Nits** section only when requested. Omit empty priority and dimension sections. Write "None" for an empty required section. + +When the user asks for a pull-request comment, prepare concise Markdown in the same priority-first, dimension-grouped format. It may omit Scope, Intent, statuses, questions, and not-reviewed sections when they add no useful information. Summarize passing checks on one line; retain details for failed or skipped checks and use the same attribution footer. Do not post with `gh` or the GitHub API unless the user explicitly says to post or publish the comment. diff --git a/.agents/skills/autoreview/lenses.md b/.agents/skills/autoreview/lenses.md new file mode 100644 index 000000000..c03a59155 --- /dev/null +++ b/.agents/skills/autoreview/lenses.md @@ -0,0 +1,75 @@ +# Lenses + +Paste one brief to one reviewer. A brief is the whole assignment: the reviewer has not seen the conversation that wrote the change, and it does not edit files. + +Generated files (`protopb/`, `mock/`, `*_mock.go`, `*.pb.go`, `*.pb.yarpc.go`) are not reviewed line by line. Say only whether they look stale relative to their source. + +Report a finding only when the changed code creates a reachable material impact. For each item give severity (`blocker`, `should-fix`, or `nit` when requested), confidence (`high`, `medium`, or `low`), changed `path:line`, observable impact, the shortest reproducing scenario or static caller trace, supporting locations, the smallest safe fix in this change, and a specific verification step. Cite the `AGENTS.md` section when it explains the engineering constraint, but a rule violation without behavioral, contract, verification, or maintenance impact is a nit and is omitted unless requested. + +When impact or reachability cannot be established, report a question with the exact missing fact. A material question makes review coverage incomplete. Inspect the surrounding implementation and tests needed to support the claim; the changed line alone is not evidence of a system-level defect. Do not restate a mechanical failure (formatting, gazelle, mocks, lint, or a failed test). If nothing is wrong, answer `No findings`. + +Confirm or drop every candidate smell the prompt forwards. A smell is not a finding until the code supports it. + +## Architecture and boundaries + +Covers Design and Separation of Concern. Read `AGENTS.md` on versioned state, controllers, queue payloads, and extensions. + +- Versioned snapshots. Version arithmetic stays in the controller: `newVersion = oldVersion + 1`, and `Version` is assigned only after the store write succeeds. +- Persist before publish. The exceptions are a status log of a transition, and an idempotent nudge whose consumer re-derives everything from current state. See `submitqueue/orchestrator/controller/speculate/finalize.go` and `submitqueue/orchestrator/controller/buildsignal/buildsignal.go`. +- An id on the queue when the consumer shares the producer's store. The full payload when the queue crosses a service boundary, with the client owning the correlation id. +- An extension contract a key-value store could implement. Report a new `KEY idx_` when it exists to answer a query the caller should already have the key for and therefore forces an avoidable backend capability. Batch atomicity, multi-key queries, and cross-entity transactions do not belong on the contract. +- A decision or action extension takes the thin reference entity (`entity.Request`, `entity.Batch`) and resolves changes, diffs, and targets itself. `conflict.Analyzer` is the reference shape. +- Factories and per-queue routing live in service wiring. `NewFactory` under an `extension/` package violates that ownership when it couples the extension to topology or implementation selection. A service-scoped aggregate, implementation, mocks, and schema live with the service that resolves them; the behavioral contracts stay shared. +- `platform/` does not import a domain. A domain `core/` does not import a service package. An implementation lives in a subpackage of its interface. + +Trace both sides of a boundary before reporting it: producer and consumer for queue contracts, caller and implementation for interfaces, controller and store for versioned writes. State the concrete coupling, backend incompatibility, race, or rollout failure the boundary violation creates. + +## Correctness and failure modes + +Covers Correctness and Bugs. Read `AGENTS.md` on eventual consistency, idempotency, and `platform/errs`. + +- The consumer is safe under at-least-once delivery. A redelivery converges; it does not apply a side effect twice. A stale read is tolerated. +- Extensions return plain errors. A classifier decides retry. A failed publish is not wrapped as retryable just so the handler runs again. `storage.ErrNotFound` is a user error only at a call site that knows the user asked for a missing resource. +- Races, lost updates, a wrong version guard, and a publish that can run before its write. Give the concrete interleaving. Context cancellation is material only when the call can block or leak work after cancellation. +- A concrete defect in the changed lines: nil or empty identity, a mishandled `""` enum sentinel, an off-by-one, an error that is dropped. A design disagreement is not a bug. +- Rolling compatibility. Proto field numbers stay stable and evolution stays additive. For schema or message findings, name the incompatible old/new binary combination and deployment order; do not infer a rollout failure from `NOT NULL` alone. + +## Simplicity and clarity + +Covers Simplicity. Read `AGENTS.md` on naming, comments, and code style. + +- A name says what it acts on, at the call site, without opening the definition. Naming is a nit unless ambiguity can cause misuse or makes the changed behavior materially harder to verify. +- A rename left the old term in code, comments, or docs. Watch `merging` against `landing`, and `land` against `merge` in git-specific code. +- A false comment that changes a reader's understanding of behavior is should-fix. Narration and comment-budget violations are nits. Prefer a clearer name over a comment. +- Value types. `(T, bool)` for absence, not a pointer. An error is a failure, not a branch of normal control flow. +- Dead code that expands the maintenance or behavior surface. Treat an intent mismatch as a question unless it creates review risk or changes behavior outside the stated scope. + +## Tests and hermeticness + +Covers Testing and Hermeticness. Read `AGENTS.md` on testing and `doc/howto/TESTING.md`. + +- Report missing coverage only when the change adds observable behavior or a material failure path that existing tests do not exercise. Name the exact scenario, why current tests miss it, the regression it could permit, and the smallest test that would prove it. +- Same-package tests, table-driven structure, testify, and generated mocks are repository conventions. Report them as nits unless the deviation causes a concrete coverage, maintenance, or generation failure. +- No `assert` or `require` on `err.Error()`. No `time.Sleep`. No timeout of the test's own. +- Integration and e2e inputs come from runfiles: `testutil.Runfile`, `testutil.WithBuildContext`, and Bazel `data`. Resolving the repo root (`show-toplevel`, `FindRepoRoot`) is a finding. A compose context is `{category}-{domain}-{name}`. Docker or network access has the `integration` and `requires-network` tags. +- Repository automation and validation run through Bazel with pinned toolchains and declared `srcs`/`data`; Make targets are thin entry points, not a second build graph. Report host-installed tools, undeclared files, runtime downloads, or direct source-tree assumptions when they make local, CI, or remote execution diverge. +- Nontrivial scripting is a Python `py_binary`/`py_test` or a Go binary like the repository linters, not Bash. A shell file is limited to an unavoidable `exec`-style launcher with no parsing, branching workflow, or domain logic. For a finding, identify the portability, quoting, dependency, or hermetic-execution failure and name the Python or Go target that should own the logic. + +## Operability + +Run when the change logs, records a metric, or returns an error to a caller. Read `AGENTS.md` on structured logging. + +- A metric tag has a bounded value set. Estimate the expected cardinality before reporting it; names are not inherently unbounded. +- For a log-volume finding, identify the log level and estimate the event rate or loop amplification. Per-message debug logging is not automatically a defect. +- Logging uses `Debugw`, `Infow`, `Errorw`, or typed zap fields. `Infof`, `Errorf`, and the other formatted zap methods are a finding. `fmt.Errorf` is not. +- The log or error carries the identifiers needed to diagnose the demonstrated failure. Missing context is a finding only when the surrounding operation cannot otherwise be identified. + +## Docs and RFCs + +Run only when documentation changed. Read `AGENTS.md` on README files and markdown prose. + +- A decision names material alternatives and why one was chosen when that context is needed to evaluate or safely change it. Missing alternatives are otherwise a question or nit. +- Terms match the code. A doc that still says `merging` where the code says `landing` is a finding. +- A README describes behavior in prose. It does not paste an interface or a type definition. A short example is fine only where the doc already says to include one. +- Prose is one line per paragraph and one line per list item. Hard wrapping is a nit unless it breaks rendering or generated processing. Code blocks, tables, and diagrams keep their own line breaks. +- A new or renamed RFC is linked from `doc/rfc/index.md`, and links to the old path are updated. diff --git a/.agents/skills/autoreview/scripts/BUILD.bazel b/.agents/skills/autoreview/scripts/BUILD.bazel new file mode 100644 index 000000000..298797dc1 --- /dev/null +++ b/.agents/skills/autoreview/scripts/BUILD.bazel @@ -0,0 +1,20 @@ +load("@rules_python//python:defs.bzl", "py_binary", "py_test") + +py_binary( + name = "review-scope", + srcs = ["review-scope.py"], + legacy_create_init = 0, + main = "review-scope.py", + visibility = ["//visibility:public"], +) + +py_test( + name = "review-scope-test", + size = "small", + srcs = [ + "review-scope.py", + "review_scope_test.py", + ], + legacy_create_init = 0, + main = "review_scope_test.py", +) diff --git a/.agents/skills/autoreview/scripts/review-scope.py b/.agents/skills/autoreview/scripts/review-scope.py new file mode 100644 index 000000000..c6372c389 --- /dev/null +++ b/.agents/skills/autoreview/scripts/review-scope.py @@ -0,0 +1,562 @@ +#!/usr/bin/env python3 +"""Print the read-only review scope, affected packages, and candidate smells.""" + +from __future__ import annotations + +import argparse +import ast +from dataclasses import dataclass +import os +from pathlib import Path, PurePosixPath +import re +import subprocess +import sys + + +KINDS = ( + "go", + "tests", + "python", + "proto", + "sql", + "build", + "shell", + "config", + "generated", + "docs", + "other", +) +GENERATED_SUFFIXES = ("_mock.go", ".pb.go", ".pb.yarpc.go") +BUILD_FILES = { + "BUILD", + "BUILD.bazel", + "Dockerfile", + "Makefile", + "MODULE.bazel", + "go.mod", + "go.sum", +} +CONFIG_SUFFIXES = (".json", ".toml", ".yaml", ".yml") + + +@dataclass(frozen=True) +class Change: + status: str + path: str + old_path: str | None = None + + def display(self) -> str: + if self.old_path: + return f"{escape_path(self.old_path)} -> {escape_path(self.path)}" + return escape_path(self.path) + + def package_paths(self) -> tuple[str, ...]: + if self.old_path: + return (self.old_path, self.path) + return (self.path,) + + +def die(message: str) -> None: + print(f"review-scope: {message}", file=sys.stderr) + raise SystemExit(2) + + +def run_git(*args: str, check: bool = True) -> str: + process = subprocess.run( + ("git", *args), + check=False, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + text=True, + env={**os.environ, "GIT_OPTIONAL_LOCKS": "0"}, + ) + if check and process.returncode != 0: + detail = process.stderr.strip() or "git command failed" + die(detail) + return process.stdout + + +def run_git_bytes(*args: str, check: bool = True) -> bytes: + process = subprocess.run( + ("git", *args), + check=False, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + env={**os.environ, "GIT_OPTIONAL_LOCKS": "0"}, + ) + if check and process.returncode != 0: + detail = process.stderr.decode(errors="replace").strip() or "git command failed" + die(detail) + return process.stdout + + +def parser() -> argparse.ArgumentParser: + result = argparse.ArgumentParser( + prog="review-scope", + description=( + "Print the target, stated intent, changed files by kind, affected " + "Bazel packages, and candidate smells in added lines." + ), + ) + modes = result.add_mutually_exclusive_group() + modes.add_argument( + "--base", + metavar="REF", + help="review HEAD and dirty work against the merge-base with REF", + ) + modes.add_argument( + "--range", + metavar="REF", + help="review committed changes from the merge-base with REF through HEAD", + ) + modes.add_argument("--commit", metavar="REV", help="review one commit only") + modes.add_argument( + "--uncommitted", + action="store_true", + help="review staged, unstaged, and untracked files only", + ) + return result + + +def is_generated(path: str) -> bool: + parts = PurePosixPath(path).parts + return ( + path.endswith(".go") and ("protopb" in parts or "mock" in parts) + or path.endswith(GENERATED_SUFFIXES) + ) + + +def kind_of( + path: str, repo: Path | None = None, treeish: str | None = None +) -> str: + name = PurePosixPath(path).name + if ( + name in BUILD_FILES + or name.startswith(("Dockerfile.", "Makefile.")) + or path.endswith((".bzl", ".bazelrc", ".bazelignore")) + ): + return "build" + if path.endswith(".sql"): + return "sql" + if path.endswith(".proto"): + return "proto" + if path.endswith(("_test.go", "_test.py")) or name.startswith("test_") and path.endswith(".py"): + return "tests" + if is_generated(path): + return "generated" + if path.endswith(".go"): + return "go" + if path.endswith(".py"): + return "python" + if path.endswith((".sh", ".bash", ".zsh")): + return "shell" + if path.endswith(".md") or path.startswith("doc/"): + return "docs" + if path.endswith(CONFIG_SUFFIXES) or name.startswith("."): + return "config" + if treeish: + content = run_git("show", f"{treeish}:{path}", check=False).encode() + first_line = content.splitlines()[:1] + if first_line and first_line[0].startswith(b"#!"): + if b"python" in first_line[0]: + return "python" + return "shell" + elif repo: + candidate = repo / path + if not candidate.is_symlink() and candidate.is_file(): + first_line = candidate.read_bytes().splitlines()[:1] + if first_line and first_line[0].startswith(b"#!"): + if b"python" in first_line[0]: + return "python" + return "shell" + return "other" + + +def parse_name_status(output: bytes) -> list[Change]: + fields = output.split(b"\0") + changes: list[Change] = [] + index = 0 + while index < len(fields) and fields[index]: + status = fields[index].decode("ascii") + index += 1 + if status.startswith(("R", "C")): + if index + 1 >= len(fields): + die("malformed rename or copy record from git") + old_path = os.fsdecode(fields[index]) + path = os.fsdecode(fields[index + 1]) + changes.append(Change(status[0], path, old_path)) + index += 2 + else: + if index >= len(fields): + die("malformed name-status record from git") + changes.append(Change(status, os.fsdecode(fields[index]))) + index += 1 + return changes + + +def git_name_status(*args: str) -> list[Change]: + return parse_name_status( + run_git_bytes("diff", "--name-status", "--find-renames", "-z", *args) + ) + + +def classify_changes( + changes: list[Change], + repo: Path | None = None, + treeish: str | None = None, +) -> list[tuple[str, Change]]: + latest_changes: dict[str, Change] = {} + for change in changes: + latest_changes[change.path] = change + + classified: list[tuple[str, Change]] = [] + for change in latest_changes.values(): + changed_kinds = { + kind_of(path, repo, treeish) for path in change.package_paths() + } + classified.extend( + (kind, change) for kind in KINDS if kind in changed_kinds + ) + return classified + + +def escape_path(path: str) -> str: + return path.encode("unicode_escape", errors="backslashreplace").decode("ascii") + + +def git_diff(*args: str) -> str: + return run_git( + "-c", + "core.safecrlf=false", + "diff", + "--no-ext-diff", + "--no-textconv", + "--unified=0", + *args, + ) + + +def bazel_package(repo: Path, path: str) -> str | None: + directory = PurePosixPath(path).parent + while True: + relative = "" if str(directory) == "." else str(directory) + candidate = repo / relative + if (candidate / "BUILD.bazel").is_file() or (candidate / "BUILD").is_file(): + return f"//{relative}" if relative else "//" + if not relative: + return None + directory = directory.parent + + +def narration_comment(line: str) -> bool: + match = re.match(r"^\s*//\s*(.*)$", line) + if not match: + return False + text = match.group(1) + if re.match(r"^(Check if|Loop over)\s", text): + return True + opener = re.match( + r"^(Fetch|Get|Read|Deserialize|Parse|Publish|Send|Persist|Store|Save|" + r"Create|Build|Return)\s+(\S+)", + text, + ) + return bool(opener and opener.group(2) != "returns") + + +def formatted_log(line: str) -> bool: + if re.search(r"\.(Infof|Debugf|Warnf|Fatalf|Panicf|DPanicf)\s*\(", line): + return True + return bool(re.search(r"\.Errorf\s*\(", re.sub(r"fmt\.Errorf\s*\(", "", line))) + + +def analyze_added_line(path: str, number: int, text: str) -> list[tuple[str, str]]: + results: list[tuple[str, str]] = [] + generated = is_generated(path) + is_test = path.endswith("_test.go") and not generated + is_go = path.endswith(".go") and not generated + + if is_test and re.search(r"time\.Sleep\s*\(", text): + results.append(("SMELL", "sleep")) + if ( + is_test + and ("assert." in text or "require." in text) + and ".Error()" in text + ): + results.append(("SMELL", "error-string-assert")) + if is_test and re.match(r"^\s*package\s+[A-Za-z0-9_]+_test\s*$", text): + results.append(("SMELL", "external-test-package")) + if is_test and ("show-toplevel" in text or re.search(r"[Ff]indRepoRoot", text)): + results.append(("SMELL", "repo-root")) + if is_go and formatted_log(text): + results.append(("SMELL", "formatted-log")) + if path.endswith(".sql") and re.search(r"key\s+`?idx_", text, re.IGNORECASE): + results.append(("SMELL", "secondary-index")) + if is_go and "extension" in PurePosixPath(path).parts and "NewFactory" in text: + results.append(("SMELL", "extension-factory")) + if is_go and narration_comment(text): + results.append(("SMELL", "narration-comment")) + if ( + is_go + and not is_test + and re.match(r"^\s*type\s+[A-Za-z_][A-Za-z0-9_]*\s+interface\s*\{", text) + ): + results.append(("HINT", "interface")) + + clean_text = text.replace("\t", " ")[:200] + display_path = escape_path(path) + return [ + (kind, f"{display_path}:{number}: {identifier}: {clean_text}") + for kind, identifier in results + ] + + +def analyze_diff(diff: str) -> list[tuple[str, str]]: + hits: list[tuple[str, str]] = [] + path = "" + new_line = 0 + for line in diff.splitlines(): + if line.startswith("diff --git "): + path = "" + elif line.startswith("+++ "): + path = line[4:] + if path.startswith('"'): + try: + quoted_path = ast.literal_eval(path) + path = os.fsdecode(quoted_path.encode("latin1")) + except (SyntaxError, UnicodeEncodeError, ValueError): + path = path[1:-1] + if path.startswith("b/"): + path = path[2:] + elif line.startswith("@@ "): + match = re.search(r"\+(\d+)", line) + if match: + new_line = int(match.group(1)) + elif line.startswith("+") and not line.startswith("+++"): + if path and path != "/dev/null": + hits.extend(analyze_added_line(path, new_line, line[1:])) + new_line += 1 + elif line.startswith(" "): + new_line += 1 + return hits + + +def read_untracked(repo: Path, path: str) -> list[tuple[str, str]]: + candidate = repo / path + if candidate.is_symlink() or not candidate.is_file(): + return [] + content = candidate.read_bytes() + if b"\0" in content: + return [] + hits: list[tuple[str, str]] = [] + for number, line in enumerate(content.decode(errors="replace").splitlines(), start=1): + hits.extend(analyze_added_line(path, number, line)) + return hits + + +def mock_source_hints( + repo: Path, changes: list[Change], treeish: str | None = None +) -> list[tuple[str, str]]: + hints: list[tuple[str, str]] = [] + for path in {change.path for change in changes}: + candidate = repo / path + if ( + not path.endswith(".go") + or path.endswith("_test.go") + or is_generated(path) + ): + continue + if treeish: + content = run_git("show", f"{treeish}:{path}", check=False) + elif candidate.is_symlink() or not candidate.is_file(): + continue + else: + content = candidate.read_text(errors="replace") + for number, line in enumerate( + content.splitlines(), start=1 + ): + if "//go:generate" in line and "mockgen" in line: + hints.append( + ( + "HINT", + f"{escape_path(path)}:{number}: mock-source: {line[:200]}", + ) + ) + break + return hints + + +def shell_script_hints( + repo: Path, changes: list[Change], treeish: str | None = None +) -> list[tuple[str, str]]: + return [ + ( + "SMELL", + f"{escape_path(path)}:1: shell-script: " + "confirm this is only a thin launcher and move logic to a Bazel Python or Go target", + ) + for path in sorted({change.path for change in changes}) + if kind_of(path, repo, treeish) == "shell" + ] + + +def print_section(name: str, values: list[str]) -> None: + print(f"\n## {name}") + if values: + print("\n".join(values)) + else: + print("none") + + +def main() -> None: + args = parser().parse_args() + workspace = os.environ.get("BUILD_WORKSPACE_DIRECTORY") + if workspace: + os.chdir(workspace) + repo_output = run_git("rev-parse", "--show-toplevel", check=False).strip() + if not repo_output: + die("not inside a git repository") + repo = Path(repo_output) + os.chdir(repo) + + head = run_git("rev-parse", "--short=12", "HEAD").strip() + dirty = bool(run_git("status", "--porcelain")) + changes: list[Change] = [] + diff = "" + base_ref = args.base or args.range or "origin/main" + base_sha = "" + commit = "" + + if args.commit: + mode = "commit" + verified = run_git( + "rev-parse", "--verify", "--quiet", f"{args.commit}^{{commit}}", check=False + ).strip() + if not verified: + die(f"unknown commit: {args.commit}") + commit = run_git("rev-parse", "--short=12", verified).strip() + changes.extend( + parse_name_status( + run_git_bytes( + "diff-tree", + "--root", + "--no-commit-id", + "--name-status", + "--find-renames", + "-z", + "-r", + commit, + ) + ) + ) + diff = run_git( + "show", + "--format=", + "--unified=0", + "--no-ext-diff", + "--no-textconv", + commit, + ) + elif args.uncommitted: + mode = "uncommitted" + changes.extend(git_name_status("HEAD")) + diff = git_diff("HEAD") + else: + mode = "range" if args.range else "branch" + verified = run_git( + "rev-parse", "--verify", "--quiet", f"{base_ref}^{{commit}}", check=False + ).strip() + if not verified: + die( + f"base {base_ref} is not available locally; pass --base. " + "This script does not fetch." + ) + base_sha = run_git("merge-base", "HEAD", base_ref).strip() + if not base_sha: + die(f"no merge-base between HEAD and {base_ref}") + if mode == "range": + changes.extend(git_name_status(base_sha, "HEAD")) + diff = git_diff(base_sha, "HEAD") + else: + changes.extend(git_name_status(base_sha)) + diff = git_diff(base_sha) + + hits = analyze_diff(diff) + if mode not in {"commit", "range"}: + untracked = [ + path + for path in run_git( + "ls-files", "--others", "--exclude-standard", "-z" + ).split("\0") + if path + ] + for path in untracked: + changes.append(Change("?", path)) + hits.extend(read_untracked(repo, path)) + + target_tree = commit if mode == "commit" else "HEAD" if mode == "range" else None + hits.extend(mock_source_hints(repo, changes, target_tree)) + hits.extend(shell_script_hints(repo, changes, target_tree)) + classified = classify_changes(changes, repo, target_tree) + + print(f"target: {mode}") + if mode in {"branch", "range"}: + base = run_git("rev-parse", "--short=12", base_sha).strip() + print(f"base: {base} ({base_ref})") + print(f"head: {head}") + if mode == "range": + print(f"range: {base}..HEAD; dirty work excluded") + else: + print(f"range: {base}..HEAD plus staged, unstaged, and untracked") + elif mode == "commit": + print(f"commit: {commit}") + print(f"head: {head}") + print(f"range: commit {commit} only") + else: + print(f"head: {head}") + print("range: staged, unstaged, and untracked") + print(f"dirty: {str(dirty).lower()}") + if mode in {"commit", "range"} and dirty: + print("note: dirty work is excluded from this review") + + if mode in {"branch", "range"}: + subjects = run_git("log", "--format=- %s", f"{base_sha}..HEAD").splitlines() + base = run_git("rev-parse", "--short=12", base_sha).strip() + print_section("intent", subjects or [f"- (no commits since {base})"]) + elif mode == "commit": + print_section("intent", [run_git("log", "-1", "--format=- %s", commit).strip()]) + else: + print_section("intent", ["- uncommitted work"]) + + print("\n## files") + for kind in KINDS: + print(f"{kind}:") + entries = sorted( + f" {change.status} {change.display()}" + for item_kind, change in classified + if item_kind == kind + ) + print("\n".join(entries) if entries else " none") + + packages = sorted( + { + package + for kind, change in classified + if kind != "docs" + for path in change.package_paths() + for package in [bazel_package(repo, path)] + if package + } + ) + print_section("packages", packages) + + smells = sorted({message for kind, message in hits if kind == "SMELL"}) + print("\n## smells") + print("note: a hit is a candidate, not a finding") + print("\n".join(smells) if smells else "none") + + hints = sorted({message for kind, message in hits if kind == "HINT"}) + print_section("hints", hints) + + +if __name__ == "__main__": + main() diff --git a/.agents/skills/autoreview/scripts/review_scope_test.py b/.agents/skills/autoreview/scripts/review_scope_test.py new file mode 100644 index 000000000..358142c2b --- /dev/null +++ b/.agents/skills/autoreview/scripts/review_scope_test.py @@ -0,0 +1,196 @@ +from __future__ import annotations + +import importlib.util +import os +from pathlib import Path +import subprocess +import sys +import tempfile +import unittest + + +SCRIPT = Path(__file__).with_name("review-scope.py") +SPEC = importlib.util.spec_from_file_location("review_scope", SCRIPT) +assert SPEC is not None +assert SPEC.loader is not None +review_scope = importlib.util.module_from_spec(SPEC) +sys.modules[SPEC.name] = review_scope +SPEC.loader.exec_module(review_scope) + + +class ReviewScopeTest(unittest.TestCase): + def test_kind_of_classifies_tooling_and_build_files(self) -> None: + cases = { + "mock/BUILD.bazel": "build", + "tool.py": "python", + "test_tool.py": "tests", + "run.sh": "shell", + "config.yaml": "config", + "service/Dockerfile.debug": "build", + "generated/mock/example.go": "generated", + "generated/mock/README.md": "docs", + } + + for path, expected in cases.items(): + with self.subTest(path=path): + self.assertEqual(expected, review_scope.kind_of(path)) + + def test_kind_of_reads_an_extensionless_script_shebang(self) -> None: + with tempfile.TemporaryDirectory() as directory: + repo = Path(directory) + (repo / "tool").write_text("#!/usr/bin/env bash\n") + + self.assertEqual("shell", review_scope.kind_of("tool", repo)) + + def test_parse_name_status_preserves_rename_copy_and_paths(self) -> None: + changes = review_scope.parse_name_status( + b"R100\0old\tname\0new\nname\0C075\0source\0copy\0M\0plain\0" + ) + + self.assertEqual( + [ + review_scope.Change("R", "new\nname", "old\tname"), + review_scope.Change("C", "copy", "source"), + review_scope.Change("M", "plain"), + ], + changes, + ) + + def test_cross_kind_rename_is_classified_at_both_endpoints(self) -> None: + change = review_scope.Change("R", "guide.md", "tool.py") + + self.assertEqual( + [("python", change), ("docs", change)], + review_scope.classify_changes([change]), + ) + + def test_bazel_package_formats_the_root_package(self) -> None: + with tempfile.TemporaryDirectory() as directory: + repo = Path(directory) + (repo / "BUILD.bazel").write_text("") + + self.assertEqual("//", review_scope.bazel_package(repo, "MODULE.bazel")) + + def test_analyze_diff_escapes_control_characters_in_paths(self) -> None: + hits = review_scope.analyze_diff( + 'diff --git "a/name\\t.go" "b/name\\t.go"\n' + '+++ "b/name\\t.go"\n' + "@@ -0,0 +1 @@\n" + "+// Get value\n" + ) + + self.assertEqual( + [("SMELL", r"name\t.go:1: narration-comment: // Get value")], + hits, + ) + + def test_untracked_symlink_contents_are_not_scanned(self) -> None: + with tempfile.TemporaryDirectory() as directory: + repo = Path(directory) + target = repo / "target.go" + target.write_text("// Get value\n") + (repo / "linked.go").symlink_to(target) + + self.assertEqual([], review_scope.read_untracked(repo, "linked.go")) + + def test_changed_mock_source_emits_a_hint(self) -> None: + with tempfile.TemporaryDirectory() as directory: + repo = Path(directory) + source = repo / "store.go" + source.write_text( + "//go:generate mockgen -source=store.go -destination=mock/store.go\n" + "package store\n" + ) + + self.assertEqual( + [ + ( + "HINT", + "store.go:1: mock-source: " + "//go:generate mockgen -source=store.go " + "-destination=mock/store.go", + ) + ], + review_scope.mock_source_hints( + repo, [review_scope.Change("M", "store.go")] + ), + ) + + def test_changed_shell_script_emits_a_hermeticness_smell(self) -> None: + with tempfile.TemporaryDirectory() as directory: + repo = Path(directory) + + self.assertEqual( + [ + ( + "SMELL", + "tool/run.sh:1: shell-script: " + "confirm this is only a thin launcher and move logic " + "to a Bazel Python or Go target", + ) + ], + review_scope.shell_script_hints( + repo, [review_scope.Change("A", "tool/run.sh")] + ), + ) + + def test_branch_mode_uses_the_net_worktree_diff(self) -> None: + with tempfile.TemporaryDirectory() as directory: + repo = Path(directory) + self.run_git(repo, "init") + self.run_git(repo, "config", "user.email", "review@example.com") + self.run_git(repo, "config", "user.name", "Review Test") + (repo / "BUILD.bazel").write_text("") + source = repo / "review.go" + source.write_text("package review\n") + self.run_git(repo, "add", ".") + self.run_git(repo, "commit", "-m", "initial") + base = self.run_git(repo, "rev-parse", "HEAD").stdout.strip() + + source.write_text("package review\n// Get value\n") + self.run_git(repo, "add", "review.go") + self.run_git(repo, "commit", "-m", "add narration") + source.write_text("package review\n") + + env = os.environ.copy() + env.pop("BUILD_WORKSPACE_DIRECTORY", None) + result = subprocess.run( + (sys.executable, str(SCRIPT), "--base", base), + cwd=repo, + check=True, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + text=True, + env=env, + ) + + self.assertNotIn("narration-comment", result.stdout) + self.assertIn("go:\n none", result.stdout) + + range_result = subprocess.run( + (sys.executable, str(SCRIPT), "--range", base), + cwd=repo, + check=True, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + text=True, + env=env, + ) + + self.assertIn("narration-comment", range_result.stdout) + self.assertIn("dirty work is excluded from this review", range_result.stdout) + + @staticmethod + def run_git(repo: Path, *args: str) -> subprocess.CompletedProcess[str]: + return subprocess.run( + ("git", *args), + cwd=repo, + check=True, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + text=True, + ) + + +if __name__ == "__main__": + unittest.main() diff --git a/.claude/skills/autoreview b/.claude/skills/autoreview new file mode 120000 index 000000000..ec585a8be --- /dev/null +++ b/.claude/skills/autoreview @@ -0,0 +1 @@ +../../.agents/skills/autoreview \ No newline at end of file diff --git a/.gitignore b/.gitignore index 08d5b9cc1..68a0bf17d 100644 --- a/.gitignore +++ b/.gitignore @@ -5,7 +5,10 @@ MODULE.bazel.lock .vscode/* !.vscode/launch.json .idea/ -.claude/ +# Local Claude Code state stays ignored. The autoreview skill symlink is shared. +.claude/* +!.claude/skills/ +!.claude/skills/autoreview .mcp.json .mcp.json.bak .ijwb/ diff --git a/MODULE.bazel b/MODULE.bazel index ed5039830..34b2067ca 100644 --- a/MODULE.bazel +++ b/MODULE.bazel @@ -5,6 +5,7 @@ bazel_dep(name = "git", version = "2.55.0") bazel_dep(name = "rules_go", version = "0.57.0") bazel_dep(name = "gazelle", version = "0.45.0") bazel_dep(name = "rules_proto", version = "7.1.0") +bazel_dep(name = "rules_python", version = "2.2.0") single_version_override( module_name = "git",