agents: expand code review rules and review-code sub-agents

.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.

.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.

.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.

.agents/rules/python.md

@@ -23,13 +23,49 @@ 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:`,
   `Returns:`, and all custom exceptions raised under `Raises:`.
 
 ## 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:

.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 || ...`.

.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.

.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: <Target / PR>
@@ -118,11 +150,91 @@ file from `.agents/skills/review-code/`:
   ## Passing Domains
   <Comma-separated list of audited domains with 0 issues>
   ````
+- 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<start>-L<end>)` 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.