From 1067a59a043edd7418e413cd82a2f02d400b0bb6 Mon Sep 17 00:00:00 2001 From: Richard Levasseur Date: Sun, 25 Oct 2026 17:04:06 -0700 Subject: [PATCH] agents: expand code review rules and review-code sub-agents Code reviews previously lacked dedicated checks for cross-language architecture, shell script idioms, and justification auditing (such as catching copy-pasted or redundant changes), as well as several Starlark, Python, `.bazelrc`, and PR review standards. Add `.agents/rules/code-style.md` and expand existing rules in `.agents/rules/` (`shell.md`, `starlark.md`, `bazelrc.md`, `python.md`, and `pr.md`). Add three new review sub-agent prompts (`Code Style & Architecture Auditor`, `Shell Script Auditor`, and `Justification Auditor`) and update the existing `review-code` sub-agent prompts and `SKILL.md` workflow. --- .agents/rules/bazelrc.md | 3 + .agents/rules/code-style.md | 49 +++++++ .agents/rules/pr.md | 9 ++ .agents/rules/python.md | 36 +++++ .agents/rules/shell.md | 17 +++ .agents/rules/starlark.md | 36 ++++- .agents/skills/review-code/SKILL.md | 130 ++++++++++++++++-- .../review-code/review-code-style-prompt.md | 21 +++ .../skills/review-code/review-docs-prompt.md | 5 + .../review-justification-auditor-prompt.md | 32 +++++ .../review-code/review-pr-standards-prompt.md | 15 +- .../review-code/review-python-prompt.md | 22 ++- .../skills/review-code/review-shell-prompt.md | 13 ++ .../review-code/review-starlark-prompt.md | 21 ++- 14 files changed, 390 insertions(+), 19 deletions(-) create mode 100644 .agents/rules/code-style.md create mode 100644 .agents/skills/review-code/review-code-style-prompt.md create mode 100644 .agents/skills/review-code/review-justification-auditor-prompt.md create mode 100644 .agents/skills/review-code/review-shell-prompt.md diff --git a/.agents/rules/bazelrc.md b/.agents/rules/bazelrc.md index 87135ce788..dbacd29cb4 100644 --- a/.agents/rules/bazelrc.md +++ b/.agents/rules/bazelrc.md @@ -16,3 +16,6 @@ globs: "*.bazelrc,.bazelrc" * **Invocation Privacy**: Keep local invocations private. Never add `--build_metadata=VISIBILITY=PUBLIC` to repository-level or user bazelrc files; configure public visibility and CI metadata only in CI. +* Use `--disk_cache=` (or `--nodisk_cache` in Bazel 10+) to disable disk + caching; `--disk_cache=false` sets the cache directory to `./false` prior to + Bazel 10. diff --git a/.agents/rules/code-style.md b/.agents/rules/code-style.md new file mode 100644 index 0000000000..bc2df1aa52 --- /dev/null +++ b/.agents/rules/code-style.md @@ -0,0 +1,49 @@ +--- +trigger: glob +description: Cross-language code style, control flow, and CLI conventions +globs: "*.py,*.sh,*.cc,*.h,*.go,*.bzl,*.bazel" +--- + +# Code Style Rules + +## File Organization & Definition Order +Order definitions within a file as: +1. Global variables / constants +2. Flags / CLI arguments +3. Functions +4. Classes +5. Main + +## Control Flow & API Design +* Use early-return guard clauses over deeply nested conditionals. +* When branching on validation modes, make the fall-off `else` the error case + (fail-closed). +* Accept one form per input (e.g., a file path, not "path or inline content"). + Do not add unrequested modes, filters, knobs, or flag aliases. Create test + fixture files instead of adding test-only code paths. +* Use module constants directly instead of adding function parameters for values + that never vary. +* When exposing a private constant (`_NAME`) as public (`NAME`), update all + internal callers to `NAME` and remove `_NAME = NAME` aliases. +* Omit speculative precondition checks when the underlying operation fails + clearly on its own. +* Do not accept `None` or add `if x is not None` guards unless `None` is a valid + domain state. +* Do not write nested comprehensions or comprehensions with multiple `for` + clauses; use explicit loops. + +## CLI & Flags +* Comment non-obvious flags (`--force`), non-default schemes, subtle arguments + (such as significant trailing slashes), and timeouts/thresholds/retries. +* Restrict constrained flag values strictly to the expected tokens (e.g., + `true|false`) rather than permissive truthy/falsy sets. +* Include `(AGENTS: requires user confirmation to use)` in `--help` text and + error messages for CLI flags that bypass safety checks. + +## Sorting +* Add `keep-sorted start` and `keep-sorted end` markers on list and dict config + or registry tables, even when they have a single entry. + +## Bulk Logging +* In bulk operations, log a single summary line on success; on failure, list + each failing item sorted, one per line, with its location and reason. diff --git a/.agents/rules/pr.md b/.agents/rules/pr.md index fa578823c8..a271befae7 100644 --- a/.agents/rules/pr.md +++ b/.agents/rules/pr.md @@ -54,3 +54,12 @@ Before drafting any pull request description, strictly adhere to the rules in Evaluate each item against the diff and project rules, flag unresolved items with `BLOCKER`, `QUESTION`, or `HUMAN-SIGN-OFF`, and verify the `"do not merge"` label is applied if open questions remain. + +## PR Review Comments & Thread Resolution +* End every posted PR comment or review reply with an italicized disclosure line + identifying it as an agent-generated response and indicating whether it was + automatically generated or user-reviewed/approved. +* After fixing an inline PR review thread, reply with the commit hash and + explanation (`addPullRequestReviewThreadReply`) and resolve the thread + (`resolveReviewThread`) via `gh api graphql`, batching at most 3-4 threads per + request to avoid `502 Bad Gateway` timeouts. diff --git a/.agents/rules/python.md b/.agents/rules/python.md index 3bacd7bda8..40049b9734 100644 --- a/.agents/rules/python.md +++ b/.agents/rules/python.md @@ -23,6 +23,21 @@ globs: "*.py,*.sh" * **Directly Executable Scripts**: Scripts executed directly (outside a `py_binary` rule) must have `chmod +x` (mode `100755`) and a `#!/usr/bin/env python3` shebang. +* Do not repeat default values in CLI flag `help` strings when the parser + displays defaults automatically. +* Include a `Usage:` example and input file format description in CLI module + docstrings. + +## API Minimalism & Module State +* Pass dependencies explicitly; avoid implicit optionals that auto-load state or + magical global state. Do not return mutable internal state. +* Assume the supported execution environment. Do not add `ImportError` + fallbacks or `sys.path` probing unless direct invocation outside Bazel is + required by an external caller constraint and justified in a comment or PR + description. +* Prefix private module-level constants and tables with `_`. +* Use direct attribute syntax (`obj.attr`) instead of `getattr`/`setattr` for + static attribute names. ## Interface & ABC Docstrings * In abstract base class / interface methods, always document `Args:`, @@ -30,6 +45,27 @@ globs: "*.py,*.sh" ## Exception Handling * Inherit custom exceptions from `Exception`, not `RuntimeError`. +* Let errors bubble up from utility functions instead of printing and returning + failure sentinels or catching and re-wrapping into caller-specific exception + classes. +* Use `e.add_note(...)` to attach context to an exception and plain `raise` when + re-raising. +* Do not place redundant `return` statements after `sys.exit(...)`. + +## Unit Testing +* Use real temporary files (`tmp_path` or `tempfile`) instead of mocking file IO + or `pathlib.Path`. +* Use `unittest.mock.patch` (and `mock.create_autospec(..., instance=True)` over + bare `Mock()`/`MagicMock()`) instead of manual state save/restore, and avoid + over-mocking the unit under test. +* Call `main(args)` directly in tests instead of mutating `sys.argv`, and test + `sys.exit` with `pytest.raises(SystemExit)` / `assertRaises(SystemExit)` + instead of mocking `sys.exit`. +* Do not prefix test method names with redundant target names (e.g., use + `test_success_directory` instead of `test_main_success_directory`). +* Factor repeated assertion blocks into `_assert_*` helpers, and assert against + literal expected strings in formatting/transformation tests rather than + duplicating production logic. ## Subprocess * Demarcate captured stdout/stderr in exceptions with 20 `=` characters: diff --git a/.agents/rules/shell.md b/.agents/rules/shell.md index faa2022490..1a5cc94bde 100644 --- a/.agents/rules/shell.md +++ b/.agents/rules/shell.md @@ -13,3 +13,20 @@ globs: "*.sh" `SC2046` assuming paths lack spaces. * Do not create companion `.sh` forwarders that merely pass `"$@"` to another executable or script. +* Prefer long option names (e.g., `--force`) over short flags in script and + subprocess calls. When a short flag has no long form (e.g., `git clean -d -x`) + or is used for cross-platform compatibility (e.g., BSD/macOS vs. GNU), add an + inline comment stating the corresponding long flag name and what it does. +* Invoke executable scripts with shebangs directly (`./foo.sh`), not via an + explicit interpreter (`bash ./foo.sh` or `python3 ./foo.py`). +* For option parsing in Bash scripts, use GNU `getopt` with `--name` and + long-only option names without custom pre-parsing normalization loops. +* Display exact command lines for key steps using an isolated sub-shell + (`(set -x; cmd ...)`), keeping high-level `echo` messages outside. +* Include a `Modifies:` section in function doc comments when a function + mutates global variables, state, or arrays. +* Comment non-obvious Bash parameter expansions (e.g., `${key^^}`). +* Do not append `|| true` to network, Git, or build commands unless failure is + an expected state documented with an inline comment. +* Under `set -e`, do not check `$?` after a simple command; test failing + commands directly via `if ! cmd; then` or `cmd || ...`. diff --git a/.agents/rules/starlark.md b/.agents/rules/starlark.md index 8a1b5017f0..be11bf03c8 100644 --- a/.agents/rules/starlark.md +++ b/.agents/rules/starlark.md @@ -18,9 +18,10 @@ globs: "*.bzl,BUILD,BUILD.bazel,*.bazel" ## Manual Tag on Internal Macro Helper Targets * When macros instantiate internal helper targets (such as private rule targets - for artifact extraction or linking support), always include `tags = ["manual"]`. -* **Why**: This prevents internal helper targets from being implicitly built when - wildcard target patterns (e.g., `//...`) are expanded. + for artifact extraction or linking support), always include + `tags = ["manual"]`. +* **Why**: This prevents internal helper targets from being implicitly built + when wildcard target patterns (e.g., `//...`) are expanded. ## Private Alias Pattern when Appending to User `deps` * When macros append internal helper targets to user-provided dependency lists @@ -79,7 +80,27 @@ globs: "*.bzl,BUILD,BUILD.bazel,*.bazel" * **`repository_ctx.repo_metadata`**: `repository_ctx.repo_metadata` was introduced in Bazel 8.3.0. +## BUILD & Target Hygiene +* Do not use deprecated `$(location)` or `$(locations)`. In execution-root + contexts (such as `genrule.cmd`), use `$(execpath ...)` and + `$(execpaths ...)`. +* Do not define overlapping `filegroup` targets or `exports_files` globs in the + same `BUILD` package; reconcile or compose them. +* Move complex loops and branching out of `BUILD` files into `.bzl` macros. +* When exposing data files across packages, define a named `filegroup` in the + owning package rather than referencing raw source files in caller `data`. + ## Code Style & Conventions +* Forward unused wrapper macro arguments via `**kwargs` instead of duplicating + the underlying rule or macro signature. +* Silence unused Starlark variables with `_ = a, b # @unused`. +* Prefer analysis-time validation over loading-time checks so defined but + unbuilt targets do not cause loading-time failures. +* Prefer `ctx.actions.write` over spawning a subprocess for small content known + at analysis time. +* In `genrule.cmd` or multi-line command strings, place closing triple quotes + immediately after the last argument (`"""\n... --flag"""`) when trailing + newlines would break chained commands. * **Do Not Export `BUILD` Files Unnecessarily**: Do not add `BUILD` or `BUILD.bazel` to `exports_files(...)` unless strictly required. * **No Nested List Comprehensions**: Do not write nested list comprehensions @@ -124,3 +145,12 @@ globs: "*.bzl,BUILD,BUILD.bazel,*.bazel" * **Test Suite Registration**: Collect test setup functions in a private `_tests` list and register them cleanly via `test_suite(name = name, tests = _tests)`. +* Use `rules_testing`'s built-in `TestingAspectInfo` and `target.attr()` instead + of custom aspect providers to inspect rule attributes. +* For single-target `analysis_test`, use singular `target = ...` and top-level + `config_settings = ...` instead of plural `targets = {"subject": ...}`. +* In multi-target tests, name target variables after their roles rather than + `subject`. +* Use a single `contains_at_least([...])` or `contains_exactly([...])` call + instead of chaining multiple `.contains(...)` assertions on the same + collection. diff --git a/.agents/skills/review-code/SKILL.md b/.agents/skills/review-code/SKILL.md index 5cf6e12e30..1847d52816 100644 --- a/.agents/skills/review-code/SKILL.md +++ b/.agents/skills/review-code/SKILL.md @@ -77,6 +77,37 @@ file from `.agents/skills/review-code/`: against `.agents/rules/news.md` and `.agents/skills/create-news/SKILL.md`. +10. Code Style & Architecture Sub-Agent + (`Role: "Code Style & Architecture Auditor"`): + - Prompt file: + `.agents/skills/review-code/review-code-style-prompt.md` + - Focus: Audits cross-language code style across all code files (`*.py`, + `*.sh`, `*.cc`, `*.h`, `*.go`, `*.bzl`, `*.bazel`): file definition order, + guard clauses, fail-closed `else`, single input form, no unrequested + knobs/aliases, no speculative validation or defensive `None` checks, no + nested comprehensions, naming precision, simplification, `keep-sorted` + markers, and CLI safety annotations. + +11. Shell Script Sub-Agent (`Role: "Shell Script Auditor"`): + - Prompt file: + `.agents/skills/review-code/review-shell-prompt.md` + - Focus: Audits `*.sh`, git hooks, and inline shell scripts against + `.agents/rules/shell.md` and `.agents/rules/code-style.md` (long flags + and commented short flags, direct shebang invocation, GNU `getopt --name`, + `(set -x; ...)`, `Modifies:` doc sections, no blind `|| true`, and no `$?` + checks under `set -e`). + +12. Justification & Exception Sub-Agent + (`Role: "Justification & Exception Auditor"`): + - Prompt file: + `.agents/skills/review-code/review-justification-auditor-prompt.md` + - Focus: Evaluates all stated or missing justifications for rule exceptions + (such as `sys.path`/`ImportError` probing, lint/type/buildifier + suppressions, custom timeouts/retries, or review replies claiming a rule + does not apply), rejecting missing, circular, or inaccurate excuses as + `SHOULD-FIX` and flagging plausible technical constraints as + `HUMAN-SIGN-OFF`. + ### Action Instructions - Immediately create a user-facing review artifact named `code-review-results.md` in the conversation artifact directory so progress @@ -85,10 +116,11 @@ file from `.agents/skills/review-code/`: committed and uncommitted branch changes without newer `upstream/main` commits. - Launch all sub-agents concurrently using `invoke_subagent`. -- Update `code-review-results.md` as sub-agent responses come in, omitting all - passing-check prose. Do not use bold for the introduction or lead-in of a - bulleted list or bullet item; use **bold** for any questions the user needs - to answer. Structure `code-review-results.md` as follows: +- Update `code-review-results.md` as sub-agent responses come in, applying the + Synthesis & Anti-Noise Gate (below) and omitting all passing-check prose. Do + not use bold for the introduction or lead-in of a bulleted list or bullet + item; use **bold** for any questions the user needs to answer. Structure + `code-review-results.md` as follows: ````markdown # Code Review: @@ -118,11 +150,91 @@ file from `.agents/skills/review-code/`: ## Passing Domains ```` +- Whenever an open issue in `code-review-results.md` is resolved (via local + edit, a new commit, or reviewer decision), move it from `## Open Issues` to + `## Resolved Issues` (never delete it) and record how it was resolved. +- If the PR author posts a question or reply requiring reviewer input on a + comment thread, record their response (`* Author Response: ...`) and a + proposed `* Draft Reply` under the corresponding issue in + `code-review-results.md`, then prompt the user via `ask_question` (including a + clickable `[code-review-results.md](file:///...#L-L)` link in the + `question` prompt text). +- Never auto-resolve a rule exception solely because the author added a + justification comment or reply. Whenever a justification is present or added, + use the `Justification & Exception Auditor` to evaluate it, rejecting weak or + inaccurate justifications as `SHOULD-FIX` and keeping plausible justifications + open as `[HUMAN-SIGN-OFF]` until the user explicitly signs off. - Once all sub-agents finish, finalize `code-review-results.md` and provide a concise summary with the overall verdict and a clickable markdown link to `code-review-results.md` in the response. -- If a PR is being reviewed, ask the user if they want to post comments to the - PR. -- When posting comments to the PR, add a note in italics at the end of each - comment identifying it as an agent-generated comment and referring to yourself - using a magnanimous title. +- If reviewing a PR belonging to another user and at least one net-new candidate + comment remains after the Anti-Noise Gate, prompt the user via `ask_question` + (including a clickable `[code-review-results.md](file:///...)` link in the + `question` prompt text) to choose whether to: + 1. Post the feedback as non-draft comments + 2. Approve PR and post non-draft comments + 3. Post the feedback as draft comments + 4. Review the feedback + 5. Do not post comments +- If reviewing or re-reviewing a PR belonging to another user and the verdict is + `PASS` (0 open issues and 0 net-new candidate comments) while the PR is not + yet approved by the user, prompt the user via `ask_question` (including a + clickable `[code-review-results.md](file:///...)` link in the `question` + prompt text) to choose whether to: + 1. Approve PR + 2. Do nothing +- Immediately before posting comments (after `ask_question` returns), re-fetch + the latest PR description, commits, and review comments (`gh pr view`), move + any issues addressed in the interim to `## Resolved Issues` in + `code-review-results.md`, and drop their candidate comments. +- When posting comments or replies to a PR, append an italicized disclosure line + at the end of each comment identifying it as an agent-generated response and + stating whether it was automatically generated or user-reviewed/approved. + +### Synthesis & Anti-Noise Gate (Guardrails) + +When aggregating sub-agent findings into `code-review-results.md` and before +posting comments, filter out noise: + +1. Starlark is not Python: Do not leak Python complaints into Starlark (`.bzl`, + `BUILD`, `*.bazel`). Starlark has no `is` operator (`== None` is correct), + freezes default arguments (mutable defaults are safe), accepts `# @unused` or + `# buildifier: disable=...` suppressions, and prefers explicit `len()` + checks. Do not suggest Python stdlib functions. +2. Third-Party, Vendored, & Generated Code: Never nitpick style or structure in + third-party imported, vendored, lockfile, or generated files (such as + `*requirements*.txt`, `gazelle_python_manifest.yaml`, or patches). Only flag + build breakages or license issues. +3. Formatter-Handled Output: Never ask to revert a formatter's output (such as + `buildifier` or `ruff` quote/comma/order changes, trailing newlines, or + markdown hard-break backslashes). +4. `TODO` Syntax: Do not complain about `TODO(...)` formatting or demand a + username or issue number. Drop `TODO` syntax nits entirely. +5. Rule Over-Application: + - Only flag a redundant comment when it literally restates the code. Never + remove a "why" rationale or Arrange/Act/Assert test headers. + - Naming nits must be objective (redundant qualifier, misleading term, or + cryptic abbreviation), not subjective taste. + - Do not require docstrings on wrapper macros where the signature is + forwarded via `**kwargs`, or enforce explicit types when duck-typing is the + established pattern. +6. Verify Before Claiming: A finding that asserts a behavior must quote the + exact line(s). If uncertain about external tool or API semantics, phrase it + as a `QUESTION` ("Does X do Y here?") rather than a factual assertion. +7. No Persona Flourishes in Posted Comments: Review comments posted to GitHub + must contain only technical review content and the trailing italicized + agent-generated disclosure line. Never include conversational chat persona + flourishes (song lyrics, jokes, sassy remarks) in posted review comments. +8. Follow-Up Etiquette: Do not block on explicit "postponed to a follow-up" + notes or minor out-of-scope follow-ups. +9. No Speculative Defensive Checks: Do not invent hypothetical edge cases (such + as "if list is empty, indexing raises"). Assume callers provide valid domain + inputs unless there is concrete evidence otherwise. +10. Specific Test Requests: Do not demand tests vaguely ("please add tests"); + always specify the exact behavior, edge case, or branch that needs testing. +11. Consolidation (>= 15 Findings): If sub-agents produce 15 or more findings + total, consolidate repeated nits into a single comment ("same here and N + other places") or omit the lowest-priority formatting nits first without + suppressing naming precision or simplification findings. +12. Deduplicate Against Existing Comments: Inspect existing PR review comments + and suppress candidate comments that duplicate feedback already posted. diff --git a/.agents/skills/review-code/review-code-style-prompt.md b/.agents/skills/review-code/review-code-style-prompt.md new file mode 100644 index 0000000000..6ec8c2f2cb --- /dev/null +++ b/.agents/skills/review-code/review-code-style-prompt.md @@ -0,0 +1,21 @@ +You are a specialized Code Style & Architecture Auditor sub-agent. +Your sole task is to audit all code and configuration files in `git diff` +(`*.py`, `*.sh`, `*.cc`, `*.h`, `*.go`, `*.bzl`, `*.bazel`) for cross-language +code style, control flow, refactoring, and CLI conventions: + +1. Read and strictly enforce `.agents/rules/code-style.md`, `AGENTS.md`, and + `CONTRIBUTING.md`. +2. Naming Precision: Flag redundant qualifiers (e.g., `_repo` on `@repo`), + cryptic abbreviations (e.g., `lbl`), or misleading terms (e.g., `wheel_name` + vs. `distribution_name`). +3. Simplification: Suggest collapsing symmetric branches, using early returns + or continues, passing lists directly instead of joining strings when + supported, and questioning dead or unreachable `else` branches. +4. Guardrails: Never nitpick third-party, vendored, lockfile, or generated + files. Ignore formatter-handled output and `TODO(...)` syntax. Do not invent + speculative defensive checks; assume callers provide valid domain inputs. +5. Rule-Exception Justifications: If the author provides a justification for an + environment fallback, suppression, or style exception, ensure the + `Justification & Exception Auditor` evaluates it for human sign-off. + +@.agents/skills/review-code/review-report-format.md diff --git a/.agents/skills/review-code/review-docs-prompt.md b/.agents/skills/review-code/review-docs-prompt.md index b215a1a7a3..78442555af 100644 --- a/.agents/skills/review-code/review-docs-prompt.md +++ b/.agents/skills/review-code/review-docs-prompt.md @@ -18,5 +18,10 @@ in `git diff` against the project's documentation rules: APIs are included in `//docs:docs` or relevant docs build targets. 8. Ensure docs describe user-visible behavior, not internal workarounds or implementation details. +9. Flag tautological comments or headings that merely restate the code or + heading, and verify docstrings and associated docs are updated when behavior + changes. +10. Guardrails: Do not enforce the 80-column wrap limit on Markdown tables, and + do not bikeshed clear prose wording. @.agents/skills/review-code/review-report-format.md diff --git a/.agents/skills/review-code/review-justification-auditor-prompt.md b/.agents/skills/review-code/review-justification-auditor-prompt.md new file mode 100644 index 0000000000..04eae5afed --- /dev/null +++ b/.agents/skills/review-code/review-justification-auditor-prompt.md @@ -0,0 +1,32 @@ +You are a specialized Justification & Exception Auditor sub-agent. +Your sole task is to audit all stated or missing justifications for rule +exceptions, convention overrides, lint/type/buildifier suppressions, and custom +thresholds across `git diff`, comments, PR descriptions, and review replies: + +1. Read and strictly enforce `.agents/rules/code-style.md`, + `.agents/rules/python.md`, `.agents/rules/starlark.md`, and `AGENTS.md`. +2. Inspect the diff, comments, PR description, and review threads for: + - Direct-execution environment probing (`ImportError` fallbacks or + `sys.path` manipulation to run a script via raw `python3` outside Bazel + when a binary target exists). + - Lint, type, or formatter suppressions (`# buildifier: disable=...` outside + test `/private/` loads, `# pyrefly: ignore[...]`, `# ruff: noqa`, + `# type: ignore`). + - Custom timeouts or retry counts. + - Review thread replies claiming a rule or convention does not apply. +3. Classify every exception into one of three outcomes: + - Missing Justification (`Severity: SHOULD-FIX`): A rule permits justified + exceptions or requires an explanatory comment, but none is given. Ask the + author to either conform to the standard convention or document a concrete + technical justification. + - Weak, Circular, or Inaccurate Justification (`Severity: SHOULD-FIX`): The + stated reason is circular (restates what the code does), convenience-only + ("simpler", "easier"), or already solved by standard project tooling. + Reject the justification and explain why it does not hold up. + - Plausible Technical Constraint (`Severity: HUMAN-SIGN-OFF`): The + justification cites a concrete, verifiable external constraint. Never + silently pass a justification; report it as `HUMAN-SIGN-OFF`, quote the + justification, and evaluate why it appears plausible so the human reviewer + explicitly signs off. + +@.agents/skills/review-code/review-report-format.md diff --git a/.agents/skills/review-code/review-pr-standards-prompt.md b/.agents/skills/review-code/review-pr-standards-prompt.md index 89a4d90b11..4b18618a47 100644 --- a/.agents/skills/review-code/review-pr-standards-prompt.md +++ b/.agents/skills/review-code/review-pr-standards-prompt.md @@ -8,11 +8,20 @@ Your sole task is to audit PR titles and PR descriptions against `feat(cc): ...`, `docs(python): ...`). For agent rules/skills, use `agents:` prefix. 3. Ensure PR descriptions explain *why* a change is made and provide a - high-level overview of *how*, following advice in `CONTRIBUTING.md`. -4. If a PR has already been created, enforce PR update rules: do NOT amend or + high-level overview of *how*, following advice in `CONTRIBUTING.md`. Flag + descriptions that merely restate the diff ("diff-slop") without stating the + problem or motivation; if an agent-authored PR generates diff-slop, instruct + the author to update their agent prompt. +4. Verify the description matches what the code actually does, cites performance + benefits for optimization PRs, and mentions any dropped patches or + intentional behavioral changes. +5. Check scope: flag unrelated directory changes, but allow minor follow-ups or + larger refactors to be deferred to follow-up PRs without blocking. Ignore + pre-existing unrelated test/build failures. +6. If a PR has already been created, enforce PR update rules: do NOT amend or rebase existing commits (create new commits and merges instead, to preserve code review comment threads). -5. Inspect the PR description and comments for open questions, `TODO` items, +7. Inspect the PR description and comments for open questions, `TODO` items, or "Things we could do / follow-ups" sections. Evaluate each item against the diff and project rules, flag items requiring resolution or user input with `BLOCKER`, `QUESTION`, or `HUMAN-SIGN-OFF`, and verify the PR has the diff --git a/.agents/skills/review-code/review-python-prompt.md b/.agents/skills/review-code/review-python-prompt.md index 0e43eafaf8..3efe3263cc 100644 --- a/.agents/skills/review-code/review-python-prompt.md +++ b/.agents/skills/review-code/review-python-prompt.md @@ -3,8 +3,8 @@ Your sole task is to audit all Python changes (including `.py` files, tests, and inline Python in shell scripts or workflows) in `git diff` against the project's Python conventions: -1. Read and strictly enforce `.agents/rules/python.md`, `AGENTS.md`, and - `CONTRIBUTING.md`. +1. Read and strictly enforce `.agents/rules/python.md`, + `.agents/rules/code-style.md`, `AGENTS.md`, and `CONTRIBUTING.md`. 2. Check Python pytest conventions: when registering pytest fixtures from helper modules in test files, use `pytest_plugins = [""]`. 3. Name fixture functions with a `fixture_` prefix (e.g. `def fixture_foo():`) @@ -14,5 +14,23 @@ project's Python conventions: 5. Verify non-trivial scripts (such as multi-line Python or shell logic) are factored into standalone files (e.g. `.py` or `.sh` files) rather than inlined via heredocs (`<< 'EOF'`) or `-c` flags (`python3 -c`, `bash -c`). +6. API Minimalism & No Magic: Flag implicit optionals that auto-load state or + magical global state; require callers to pass dependencies explicitly. Flag + returning mutable internal state and unprefixed private module constants. +7. Exceptions: Flag utility functions that print errors or swallow exceptions + instead of raising; prefer `e.add_note(...)` for extra context, plain `raise` + when re-raising, and ask what error is being handled on broad `except` + blocks. +8. Testing Rigor: Require real temporary files (`tmp_path` or `tempfile`) over + mocked file IO or `pathlib.Path`, `unittest.mock.patch` and + `mock.create_autospec(..., instance=True)` over bare `Mock()`/`MagicMock()` + or manual state save/restore, direct `main(args)` calls over mutating + `sys.argv`, and `pytest.raises(SystemExit)` / `assertRaises(SystemExit)` over + mocking `sys.exit`. Question over-mocked tests that mock out the unit under + test. +9. Execution Environment: When a script includes `sys.path` or `ImportError` + probing for direct `python3` execution outside Bazel, flag it as `SHOULD-FIX` + if unjustified, or ensure the `Justification & Exception Auditor` evaluates + any stated justification for human sign-off. @.agents/skills/review-code/review-report-format.md diff --git a/.agents/skills/review-code/review-shell-prompt.md b/.agents/skills/review-code/review-shell-prompt.md new file mode 100644 index 0000000000..1bdb0a1f25 --- /dev/null +++ b/.agents/skills/review-code/review-shell-prompt.md @@ -0,0 +1,13 @@ +You are a specialized Shell Script Auditor sub-agent. +Your sole task is to audit all shell and Bash scripts (`*.sh`, git hooks, and +inline shell scripts) in `git diff`: + +1. Read and strictly enforce `.agents/rules/shell.md`, + `.agents/rules/code-style.md`, `AGENTS.md`, and `CONTRIBUTING.md`. +2. Ensure script filenames reflect when and where they execute. +3. Guardrails: Do not flag `|| true` when consistently copied from nearby lines + in the same file for a known-flaky command, and do not demand unit tests for + simple Bash glue scripts. Do not ask to move function definitions above + top-level CLI argument checks or global variable assignments. + +@.agents/skills/review-code/review-report-format.md diff --git a/.agents/skills/review-code/review-starlark-prompt.md b/.agents/skills/review-code/review-starlark-prompt.md index f97c93bda6..5dfc5dd820 100644 --- a/.agents/skills/review-code/review-starlark-prompt.md +++ b/.agents/skills/review-code/review-starlark-prompt.md @@ -3,7 +3,8 @@ Your sole task is to audit all Starlark (`.bzl`, `BUILD`, `*.bazel`) changes in `git diff` against the project's Starlark coding rules and conventions: 1. Read and strictly enforce `.agents/rules/starlark.md`, - `.agents/rules/bzl.md`, `AGENTS.md`, and `CONTRIBUTING.md`. + `.agents/rules/bzl.md`, `.agents/rules/bazelrc.md`, `AGENTS.md`, and + `CONTRIBUTING.md`. 2. Verify iterative algorithms are used (no recursion, no `while` loops). 3. Ensure every `.bzl` file outside `tests/` has a corresponding `bzl_library` target in its `BUILD` file with proper dependencies. @@ -11,7 +12,11 @@ in `git diff` against the project's Starlark coding rules and conventions: `# buildifier: disable=bzl-visibility`. 5. Check multi-line rule/macro doc arguments: use triple-quoted strings (`"""`), and do NOT use trailing backslashes (`\`) on opening triple-quotes. -6. Verify analysis tests use `rules_testing`, not `bazel_skylib`. +6. Verify analysis tests use `rules_testing` (not `bazel_skylib`), use + `TestingAspectInfo` and `target.attr()`, use singular `target = ...` for + single-target tests, avoid naming multi-target variables `subject`, and use + `contains_at_least([...])` or `contains_exactly([...])` instead of chained + `.contains(...)` calls. 7. Ensure shared labels use `labels.` from `python/private/common_labels.bzl` rather than redefining `Label("//...")`. 8. Prefer `types.is_*` helpers from `@bazel_skylib//lib:types.bzl` over @@ -21,5 +26,17 @@ in `git diff` against the project's Starlark coding rules and conventions: direct approach fails, and why the workaround works. 11. Verify no compatibility logic or feature guards target Bazel versions older than `MINIMUM_BAZEL_VERSION` in `//:version.bzl`. +12. Inspect the full `BUILD` file (not just the diff hunk) to flag overlapping + or redundant `filegroup`, `exports_files`, or `glob(...)` definitions. +13. Check macro and rule hygiene: forward unused wrapper arguments via + `**kwargs`, silence unused variables with `_ = a, b # @unused`, prefer + analysis-time validation over loading-time checks, prefer + `ctx.actions.write` over subprocesses for small static strings, use + `$(execpath ...)` / `$(execpaths ...)` instead of `$(location)` / + `$(locations)`, and check public `.bzl` symbol removals/renames for breaking + users. +14. Guardrails: Starlark is not Python (`== None` is correct, mutable defaults + are frozen, no Python stdlib). Ignore `buildifier` formatting output and + unowned wrapper macro docstrings. @.agents/skills/review-code/review-report-format.md