Add repository-specific review rules to the Gemini style guide (#707)

## Summary

Adds a "Repository-specific review rules" section to the Gemini style guide, built from what human reviewers repeatedly asked for (or pushed back on) in this repo's PRs over the last 90 days (124 PRs, 216 human review comments, 122 bot inline comments). Also drops "import violations" from the `[MUST-FIX]` definition, since no import rules are defined anywhere.

- **Spec grounding:** check constants, defaults, optionality and version-specific behavior against the MCP spec; ask for spec links. *Medium; replay: helps.*
- **Configurable limits:** hard-coded limits that aren't from the spec should be configurable. *Medium; replay: helps.*
- **Documentation:** doc comments on new public API; high-level comments on complex private code; explain examples. *Medium; replay: helps.*
- **Extension types:** `as`/`is` on `dart_mcp` API types aren't safety checks. *Medium; replay: no effect (couldn't reproduce the bot's mistake).*
- **JSON typing:** `Map<String, Object?>`, `Object?` over `dynamic`, promotion over `as` + `!`. *Medium; replay: helps (weak).*
- **Copies and lookups:** avoid needless map copies and full scans; don't drop unknown fields. *Medium; replay: helps.*
- **LLM-facing text:** don't ask to pad tool descriptions or skill bodies. *Medium; replay: no effect (couldn't reproduce the bot's mistake).*
- **`TestEnvironment` teardown:** it already registers `addTearDown(shutdown)`. *Low; replay: helps.*

**Confidence: Medium overall** (evidence-based; replays on motivating PRs only, apart from one evaluation noted below) · Affects: bot (`.gemini/styleguide.md`) · Type: edit

## Change

Removes "import violations" from `[MUST-FIX]` and appends section 4 with the eight rules above. Rules are inlined (not linked) because the review bot most likely reads only its guidance files' text.

## Evidence

| Rule | Source | What it shows | Who (role) |
|---|---|---|---|
| Spec grounding | #580, #596, #629, #632, #635, #636, #679 (14 comments, 12 PRs) | asks for spec links; checks optionality/nullability against the schema. Bot also made wrong spec claims on #627, #629 | jakemac53 (maintainer), single-reviewer |
| Configurable limits | #632, #635, #655 | "make this configurable, keep the default" | jakemac53 (maintainer), single-reviewer |
| Documentation | #632, #638, #655, #676, #678, #679 (~11 comments) | asks for API docs and high-level comments | jakemac53 (maintainer), single-reviewer |
| Extension types | #699, #676, #654 (bot rebutted); #655 | casts to extension types never fail; `is` can't split unions; examples shouldn't use `is` checks | jakemac53 (maintainer), Yusufihsangorgel (PR author) |
| JSON typing | #528, #531, #655 (9 comments) | `Object?` not `dynamic`, `Map<String, Object?>`, promotion over casts | jakemac53 (maintainer), single-reviewer |
| Copies and lookups | #531, #568, #604, #655, #678, #679, #682 (9 comments, 2 reviewers) | needless copies and full scans; rebuilding drops fields | jakemac53, kenzieschmoll (maintainers) |
| LLM-facing text | #615; #578 (bot rebutted) | padding skill bodies/tool descriptions wastes tokens | jakemac53 (maintainer) |
| TestEnvironment | #637, #652 (5 bot comments rebutted) | bot asked for teardown `TestEnvironment` already does | Yusufihsangorgel (PR author) |

## Replay results

Simulated reviews of each PR diff with the current style guide and with the rule added (model: agent default, 1 run per version, motivating PRs plus 1-2 unrelated or near-miss PRs):

| Rule | Motivating PRs: current → with rule | Unrelated PRs (false flags) |
|---|---|---|
| Spec grounding | 0/2 → 2/2 | 0/2 → 0/2 |
| Documentation | 1/2 → 2/2 | 0/2 → 0/2 |
| Extension types | 0/2 → 0/2 (simulator didn't repeat the mistake) | 0/1 → 0/1 |
| JSON typing | 0/2 → 1/2 | 0/2 → 0/2 |
| Copies and lookups | 1/3 → 3/3 | 0/2 → 0/2 |
| Configurable limits | 0/3 → 3/3 (#571, whose spec-defined constants shouldn't be configurable, wasn't flagged) | 0/2 → 0/2 |
| LLM-facing text | 0/1 → 0/1 (simulator didn't repeat the mistake) | 0/1 → 0/1 |
| TestEnvironment | 1/2 → 0/2 wrong teardown requests | 0/1 → 0/1 |

A broader change narrowing `[MUST-FIX]` to "verified, concrete failures" was also evaluated (3 runs × 10 PRs, blind judge). It showed no effect, because the simulator never reproduced the bot's wrong MUST-FIX claims, so it isn't included. Only the undefined "import violations" wording is removed.

## Limitations

- Replays simulate the review bot with a fixed prompt; they aren't Gemini Code Assist itself.
- Small samples (1 run per version on 3-5 PRs per rule); motivating PRs were used to write the rules.
- Most evidence comes from one maintainer's reviews (labeled single-reviewer above).

---
*Prepared with `review-tuner` (agent-assisted evidence and replay workflow) and reviewed by @jakemac53.*
1 file changed
tree: 6b7e9073806c498d51139cb0063c21e262cdb167
  1. .agents/
  2. .gemini/
  3. .github/
  4. .vscode/
  5. mcp_examples/
  6. pkgs/
  7. .gitignore
  8. CONTRIBUTING.md
  9. GEMINI.md
  10. LICENSE
  11. pubspec.lock
  12. README.md
README.md

Overview

A general repository for AI / GenAI packages and explorations.

Packages

PackageDescriptionIssuesVersion
dart_mcpA package for making MCP servers and clients.issuespub package
dart_mcp_serverAn MCP server for Dart projects, exposing various developer tools to AI models.issuespub package
skillsA CLI for managing skills shipped in packages and from git reposissuespub package