Repository navigation
Expand file tree
/
Copy path.coderabbit.yaml
More file actions
258 lines (258 loc) · 17.9 KB
/
Copy path.coderabbit.yaml
File metadata and controls
258 lines (258 loc) · 17.9 KB
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
187
188
189
190
191
192
193
194
195
196
197
198
199
200
201
202
203
204
205
206
207
208
209
210
211
212
213
214
215
216
217
218
219
220
221
222
223
224
225
226
227
228
229
230
231
232
233
234
235
236
237
238
239
240
241
242
243
244
245
246
247
248
249
250
251
252
253
254
255
256
257
258
# yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json
# CodeRabbit config for phase.rs — the free external review pass on this public repo.
# See .claude/skills/pr-contribution-handler/SKILL.md "Independent review pass".
#
# Rules distilled from CLAUDE.md, .gemini/styleguide.md, and the /review-engine-impl
# skill lenses. Path instructions are ADDITIVE: a file receives every instruction
# whose glob matches it, so these layer broad -> narrow, each adding only its
# increment. This repo's .coderabbit.yaml overrides the web UI, so keep the UI
# path-instruction section empty and edit this file instead.
language: en-US
reviews:
# Fewer, higher-signal comments — the maintainer loop confirms/refutes each
# against head, so noise costs reviewer time. Mirrors the old Gemini
# comment_severity_threshold: MEDIUM.
profile: chill
request_changes_workflow: false
high_level_summary: true
poem: false
review_status: true
auto_review:
enabled: true
drafts: false
# Generated / vendored / binary artifacts — reviewing these is pure noise.
# Ported from .gemini/config.yaml ignore_patterns for parity.
path_filters:
- "!client/public/card-data*.json" # generated by ./scripts/gen-card-data.sh
- "!client/public/coverage-*.json" # generated coverage report
- "!client/public/scryfall-*.json" # generated Scryfall data
- "!client/public/changelog*.json" # generated changelog
- "!client/public/**.png" # binary assets
- "!client/public/**.jpg" # binary assets
- "!client/src/wasm/**" # generated by ./scripts/build-wasm.sh
- "!**/*.d.ts" # committed wasm type artifact
- "!data/semantic-audit.json" # generated by `cargo semantic-audit`
- "!data/parser-gaps.json" # generated by `cargo parser-gaps`
- "!data/engine-inventory.json" # generated by `cargo engine-inventory`
- "!**/__snapshots__/**" # vitest snapshots
- "!**/snapshots/**" # insta snapshots
- "!docs/MagicCompRules.txt" # third-party rules text, not redistributed
- "!.planning/**" # gitignored planning docs
path_instructions:
- path: "**"
instructions: >-
Review phase.rs, an MTG (Magic: The Gathering) rules engine in Rust
(native + WASM) with a React/TypeScript frontend. The one question on every
PR: is this the most architecturally idiomatic approach for THIS codebase?
Judge against three co-equal, non-negotiable pillars — (1) idiomatic Rust:
typed enums over stringly/bool data, exhaustive `match` over wildcard
fallbacks; (2) strict fidelity to the MTG Comprehensive Rules (CR); (3)
composable building blocks that handle a CLASS of cards, not one special
case. `CLAUDE.md` at the repo root is the authoritative design document —
treat any deviation as a finding. Surface GAPS (missing or wrong behavior),
not style nits. Do NOT duplicate CI: `cargo fmt`, `clippy -D warnings`,
`pnpm lint`, and `pnpm type-check` already run — never comment on
formatting, whitespace, line length, or clippy-caught lints. Only surface
MEDIUM or higher severity. A concrete mandatory-architecture violation is
at least MEDIUM even when current outputs are correct: cite the violated
rule and the existing authority or typed alternative that fits. Do not
invent a runtime bug to justify it; a merely different implementation
preference without that evidence is not a finding.
A latent bug behind a guard or unreached branch
is still a finding — rate it by what happens when the form is reached or the
guard is removed, not by today's reachability. Check edge cases when relevant:
empty inputs (0 mana, 0 targets, empty filter), multi-target/modal/repeat-for
interactions, simultaneous events (dies + ETB in one SBA pass, copy-of-copy,
control change with summoning sickness), eliminated players still referenced,
and async races (state updates after unmount, two reconnects at once). Findings
only: no praise, no diff recap, no LGTM padding.
- path: "crates/**/*.rs"
instructions: >-
Idiomatic Rust is mandatory on first write. Findings: any new `bool` struct
field or `bool` variant payload where a typed enum (ControllerRef,
Comparator, PlayerScope, Option<T>, or a small new enum) would carry the same
information with more meaning; wildcard `_` match arms where the enum is known
and an exhaustive match would let the compiler catch missing variants;
hand-rolled `starts_with` + index-slicing that should be
`strip_prefix`/`strip_suffix`/`TextPair`; `as`-casts or unchecked conversions
at trust boundaries; and any new helper that duplicates an existing building
block. Prefer composing `std` primitives and reusing existing helpers over
re-implementation.
- path: "crates/engine/**"
instructions: >-
The engine owns ALL game logic; adapters (WASM, WebSocket, Tauri, P2P) are
thin serialization boundaries with zero rules. Findings: rules-touching code
with no verified `CR <number>: <description>` annotation, or a CR citation
whose rule body does not describe the code (CR 119 starting-life / 120 damage
/ 121 draw are adjacent and confused; 701.x keyword actions and 702.x keyword
abilities are arbitrary numbers and hallucination-prone). Do NOT flag compound
CR forms `CR X + CR Y` (interacting rules), `CR X / CR Y` (alternatives), or
range/subpart `CR 702.45a/b` — these are the documented convention, not
format violations. Player-scoped queries on NON-battlefield zones
(graveyard/library/hand/exile) must filter by `obj.owner`, not `controller`
(CR 108.4a). Zone changes must route through the replacement-aware pipeline
(`ProposedEvent::ZoneChange`), not a direct `zones::move_to_zone`, so
replacements can apply. Ability costs resolve through one authoritative
resolver — flag any call site that destructures or branches on an ability's
cost shape. New player-visible `GameState` must be threaded into
`filter_state_for_player` or opponents will see hidden information. Hot
`im::Vector` zones must use `im` methods (`push_back`/`pop_back`/`iter_mut`),
not materialize a std `Vec`; `truncate(n)` must be length-guarded (it panics
when n > len). Format/legality checks must use semantic identity (e.g. the
`Basic` supertype for singleton rules), not brittle card-name allowlists, and
banlists must reflect the actual format (Commander vs Duel Commander vs Pauper
Commander differ).
- path: "crates/phase-ast/**"
instructions: >-
The engine owns ALL game logic; adapters (WASM, WebSocket, Tauri, P2P) are
thin serialization boundaries with zero rules. Findings: rules-touching code
with no verified `CR <number>: <description>` annotation, or a CR citation
whose rule body does not describe the code (CR 119 starting-life / 120 damage
/ 121 draw are adjacent and confused; 701.x keyword actions and 702.x keyword
abilities are arbitrary numbers and hallucination-prone). Do NOT flag compound
CR forms `CR X + CR Y` (interacting rules), `CR X / CR Y` (alternatives), or
range/subpart `CR 702.45a/b` — these are the documented convention, not
format violations. Player-scoped queries on NON-battlefield zones
(graveyard/library/hand/exile) must filter by `obj.owner`, not `controller`
(CR 108.4a). Zone changes must route through the replacement-aware pipeline
(`ProposedEvent::ZoneChange`), not a direct `zones::move_to_zone`, so
replacements can apply. Ability costs resolve through one authoritative
resolver — flag any call site that destructures or branches on an ability's
cost shape. New player-visible `GameState` must be threaded into
`filter_state_for_player` or opponents will see hidden information. Hot
`im::Vector` zones must use `im` methods (`push_back`/`pop_back`/`iter_mut`),
not materialize a std `Vec`; `truncate(n)` must be length-guarded (it panics
when n > len). Format/legality checks must use semantic identity (e.g. the
`Basic` supertype for singleton rules), not brittle card-name allowlists, and
banlists must reflect the actual format (Commander vs Duel Commander vs Pauper
Commander differ).
- path: "crates/phase-parser/**"
instructions: >-
The engine owns ALL game logic; adapters (WASM, WebSocket, Tauri, P2P) are
thin serialization boundaries with zero rules. Findings: rules-touching code
with no verified `CR <number>: <description>` annotation, or a CR citation
whose rule body does not describe the code (CR 119 starting-life / 120 damage
/ 121 draw are adjacent and confused; 701.x keyword actions and 702.x keyword
abilities are arbitrary numbers and hallucination-prone). Do NOT flag compound
CR forms `CR X + CR Y` (interacting rules), `CR X / CR Y` (alternatives), or
range/subpart `CR 702.45a/b` — these are the documented convention, not
format violations. Player-scoped queries on NON-battlefield zones
(graveyard/library/hand/exile) must filter by `obj.owner`, not `controller`
(CR 108.4a). Zone changes must route through the replacement-aware pipeline
(`ProposedEvent::ZoneChange`), not a direct `zones::move_to_zone`, so
replacements can apply. Ability costs resolve through one authoritative
resolver — flag any call site that destructures or branches on an ability's
cost shape. New player-visible `GameState` must be threaded into
`filter_state_for_player` or opponents will see hidden information. Hot
`im::Vector` zones must use `im` methods (`push_back`/`pop_back`/`iter_mut`),
not materialize a std `Vec`; `truncate(n)` must be length-guarded (it panics
when n > len). Format/legality checks must use semantic identity (e.g. the
`Basic` supertype for singleton rules), not brittle card-name allowlists, and
banlists must reflect the actual format (Commander vs Duel Commander vs Pauper
Commander differ).
- path: "crates/engine/src/types/**"
instructions: >-
Parameterize, don't proliferate. Before accepting a new sibling enum variant,
check it is not a leaf-level parameterization of an existing variant's axis
(scope, target, comparator, aggregator, condition shape). Sibling-cluster
smell: three or more variants sharing a name root (X / OpponentX / TargetX /
AllX) or differing only by a comparator/aggregator/scope label — flag it and
recommend one parameterized variant. BUT the parameterization axis must stay
within a single CR rule section: do not unify across sections at the
leaf-reference layer (life is CR 119, player-only; power/toughness are
CR 208/209). A *reference* enum (HandSize, LifeTotal) must not carry a
Fixed/constant payload — that belongs one level up in an expression wrapper.
- path: "crates/phase-ast/src/types/**"
instructions: >-
Parameterize, don't proliferate. Before accepting a new sibling enum variant,
check it is not a leaf-level parameterization of an existing variant's axis
(scope, target, comparator, aggregator, condition shape). Sibling-cluster
smell: three or more variants sharing a name root (X / OpponentX / TargetX /
AllX) or differing only by a comparator/aggregator/scope label — flag it and
recommend one parameterized variant. BUT the parameterization axis must stay
within a single CR rule section: do not unify across sections at the
leaf-reference layer (life is CR 119, player-only; power/toughness are
CR 208/209). A *reference* enum (HandSize, LifeTotal) must not carry a
Fixed/constant payload — that belongs one level up in an expression wrapper.
- path: "crates/phase-parser/src/parser/**"
instructions: >-
Nom 8.0 combinators on the first pass — no exceptions. Findings: any new
`.contains`/`.starts_with`/`.ends_with`/`.find`/`.split_once` used for parsing
DISPATCH in non-test parser code (TextPair dual-string ops, justified
non-dispatch structural operations, and test code are exempt), and especially verbatim full-string Oracle equality
(`if lower == "..."`) — the single most prohibited pattern in the codebase,
which the CI combinator gate does NOT catch, so flag every occurrence. Compose
`alt()` per axis; never enumerate the cartesian product as separate
`tag("full string")` arms. For multi-position phrases, use the shared
`scan_at_word_boundaries` family in oracle_nom/primitives.rs rather than
copying caller-local `.find(' ')` loops. Structural cursor movement inside
that shared primitive is not parsing dispatch.
`parse_inner_condition` (oracle_nom/condition.rs)
is the single authority for game-state conditions — trigger and static parsers
must delegate, never re-implement recognition. For every new arm, verify the
plural / possessive / "an opponent's" / "your" / "their" / "non-X" / "another"
and article-word (a/an/one/two/N/X/each) variants are covered or explicitly out
of scope. When one arm defers to another by returning `None`, TRACE the
receiving arm and prove it accepts that form — a deferral to a path that does
not handle it silently drops the card to Unimplemented.
- path: "client/src/**"
instructions: >-
The frontend is a display layer, never a logic layer. Findings: any
computation, derivation, filtering, or inference of GAME data inside
`client/src/` — push it into the engine and expose the result (formatting
engine-provided values via string interpolation is fine; calculating or
inferring game state is not). New engine fields exposed to the UI must be wired
symmetrically through every adapter (WASM, WebSocket, Tauri, P2P) with a
round-trip test. Check `useEffect` dependencies carry the right identity for
back-to-back prompts, and that unmount cleanup handles animations, timers,
subscriptions, and observers. Touch targets >= 44pt; `:hover`-only state breaks
on mobile and needs a touch equivalent. i18n boundary: frontend-authored
user-facing text (titles, labels, buttons, tooltips, placeholders, log
templates) must route through `t()`; engine/card pass-through (card names,
Oracle text, interpolated enum strings) must NOT be wrapped in `t()`. Flag
hand-rolled `count === 1 ? …` pluralization (use `key_one`/`key_other`) and any
direct `i18n.changeLanguage` call (the preferences store owns language).
- path: "crates/phase-ai/**"
instructions: >-
Classifiers (polarity / threat / category) must cover the full enum, not just
the easy variants — untargeted board wipes (`Effect::DestroyAll`, `DamageAll`)
are real threats and easy to miss. Deadline-bail branches must score candidates
the SAME way as the no-bail branch (compose `tactical + penalty` once, not
weighting one side and not the other). Cache keys must reflect every input that
changes an AI decision — hashing `hand.len()` but not contents collides distinct
positions. Combination generators should short-circuit infeasible cases before
enumerating. New tactical policies must use the `PolicyVerdict` band helpers,
not raw sentinel scores.
- path: "crates/*server*/**"
instructions: >-
Transport layers (server-core, phase-server) carry zero game logic — flag any
rules/validation/derived-state computation that belongs in the engine. New
player-visible state must be filtered through `filter_state_for_player` so
opponents cannot see hidden information (hand, library, face-down). New wire
fields must be encoded and decoded symmetrically across every adapter (WASM,
WebSocket, Tauri, P2P) with a round-trip test. When reconnect or 3+ player
paths are touched, verify the disconnect grace period is honored and lobby
notifications fire for every joiner/leaver, not just the first.
- path: "crates/feed-scraper/**"
instructions: >-
Feed code must refuse to overwrite cached state with empty or zero-deck
responses (on both server and client sides) — a transient empty upstream
response must not clobber good cached data.
- path: "crates/engine/tests/**"
instructions: >-
Test adequacy is the highest-frequency contributor finding — scrutinize it.
A test must exercise the FAILURE path the fix prevents and drive the engine
through its production pipeline (`apply`/`WaitingFor`/`GameAction`/stack/casting/
combat declaration/replacement/scenario runner); a parser AST shape test does
NOT prove runtime semantics or a coverage-support claim. Verify integration
tests are registered with a `mod` line in `tests/integration/main.rs` — an
unregistered test compiles to nothing and shows false-green. For every negative
assertion (`!detector(...)`, "not applied", "does not parse to X"), require a
paired positive reach-guard proving the input actually reached the code under
test (parse succeeded, zero `Effect::Unimplemented`, expected positive shape);
an upstream short-circuit makes a negative pass for the wrong reason. Flag
constructor shortcuts (`create_object` setting `controller = owner`, builders
that bypass production wiring, `setUp` that pre-populates post-fix state) that
can silently mask the very bug a regression test claims to catch.
chat:
auto_reply: true