.coderabbit.yaml

# 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-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/mtgish-cards.json"           # generated import data
    - "!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 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 404.2). 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/engine/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 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. `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