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

Closed
yiyixuxu wants to merge 1 commit into from
Closed

[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 numberDiff line numberDiff 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

@yiyixuxuyiyixuxuMay 21, 2026

Copy link
Copy Markdown
CollaboratorAuthor

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

@github-actionsgithub-actionsBotMay 21, 2026•
edited
Loading

Copy link
Copy Markdown
Contributor

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):

#TripwireWhy it blocksSource
4Unconditional torch.float64 / .double()Breaks MPS and NPU entirelymodels.md gotcha #5
5Missing @torch.no_grad() on pipeline __call__GPU OOM from gradient accumulationpipelines.md gotcha #2
6Missing import registration in __init__.pyImportError for usersmodels.md gotcha #1
7Graph-breaking ops in model forwardBreaks torch.compile with fullgraph=Truemodels.md coding style
8Modular blocks importing from diffusers.pipelines.*Core architecture violationmodular.md gotcha #1
9Hardcoded dtype in model forwardBreaks GGUF / quantized loadingmodels.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