Skip to content

[agents docs] review-rules: add Tripwires section #13775

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
yiyixuxu wants to merge 1 commit into
base: main
Choose a base branch
from
+26 −0
Open

[agents docs] review-rules: add Tripwires section #13775

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions .ai/review-rules.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,32 @@ Before reviewing, read and apply the guidelines in:
- [skills/parity-testing/SKILL.md](skills/parity-testing/SKILL.md) — testing rules, comparison utilities
- [skills/parity-testing/pitfalls.md](skills/parity-testing/pitfalls.md) — known pitfalls (dtype mismatches, config assumptions, etc.)

## Tripwires

A short, non-exhaustive list of mistakes that block merge on their own — the
clearest signal a contributor hasn't read [AGENTS.md](AGENTS.md) and the docs
it links to. Failing one of these is enough to send a PR back; passing them
all doesn't make the PR good, it just means it's worth a proper review.

1. **New mandatory dependency.** Any new import outside the existing
Copy link
Copy Markdown
Collaborator Author

@yiyixuxu yiyixuxu May 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@claude is there anything else you want to add it here? can you open a PR with suggested changes
COMMIT THIS

Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot May 21, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude finished @yiyixuxu's task in 3m 7s —— View job

Added 6 additional tripwires to review-rules.md

  • Read current .ai/review-rules.md and PR diff
  • Explore the agents/tripwires codebase for context (read AGENTS.md, models.md, pipelines.md, modular.md, parity-testing docs)
  • Draft additional tripwires based on patterns from the docs
  • Edit .ai/review-rules.md with new tripwires

After reading through all the .ai/ documentation files, I identified 6 additional merge-blocking patterns that are well-documented as gotchas but weren't yet in the Tripwires section. Here's what I added (tripwires 4–9):

# Tripwire Why it blocks Source
4 Unconditional torch.float64 / .double() Breaks MPS and NPU entirely models.md gotcha #5
5 Missing @torch.no_grad() on pipeline __call__ GPU OOM from gradient accumulation pipelines.md gotcha #2
6 Missing import registration in __init__.py ImportError for users models.md gotcha #1
7 Graph-breaking ops in model forward Breaks torch.compile with fullgraph=True models.md coding style
8 Modular blocks importing from diffusers.pipelines.* Core architecture violation modular.md gotcha #1
9 Hardcoded dtype in model forward Breaks GGUF / quantized loading models.md gotcha #4

Each entry follows the same format as the existing tripwires: what to look for, why it matters, and a link back to the detailed documentation.

Patterns I considered but left out (better as review feedback than hard tripwires):

  • Subclassing existing pipelines for variants (pipelines.md gotcha Convert glide weights #4) — a convention violation but not always a contributor-ignorance signal
  • Capability flags without implementation (models.md gotcha make from hub import work #3) — important but more subtle, covered well in the Common mistakes path
  • Test model paths in contributor repos (modular.md gotcha add unet ldm in init #6) — easy to fix, not a sign of deeper misunderstanding

dependency set (e.g. `einops`, `rotary_embedding_torch`) — either rewrite
in native PyTorch or guard as optional with `is_X_available()` plus a
dummy in `utils/dummy_*.py`. See [models.md §Coding style](models.md#coding-style)
and gotcha #2.

2. **Attention bypassing the dispatch pattern.** Calls to
`F.scaled_dot_product_attention` directly, or imports of third-party
attention libraries inside the model file. New attention must define an
`Attention` class + processor in the model file and route through
`dispatch_attention_fn`. See
[models.md §Attention pattern](models.md#attention-pattern).

3. **Custom `from_pretrained` (or `save_pretrained`) on a model or pipeline
class.** `ModelMixin` and `DiffusionPipeline` already provide these and
handle sharding, dtype, device map, single-file, etc. An override is
almost always a sign the contributor missed an existing mixin or kwarg —
raise the question before reviewing the rest.

## Common mistakes

Common mistakes are covered in the common-mistakes / gotcha sections in [AGENTS.md](AGENTS.md), [models.md](models.md), [pipelines.md](pipelines.md), and [modular.md](modular.md). Additionally, watch for below patterns that aren't covered there:
Expand Down
Loading