From 996c02cf6c9be72c0ffc8233c64238db176c69fc Mon Sep 17 00:00:00 2001 From: mathrb Date: Thu, 25 Jun 2026 12:57:27 +0000 Subject: [PATCH 1/3] docs: design for refactoring CLAUDE.md Key Rules into a tiered rules index Tiered approach (Option C): inline Workflow essentials + Rules Index table + tripwire headlines in CLAUDE.md, with rule bodies extracted to 10 referenced .claude/rules/*.md files loaded on demand. Includes the lossless 62-rule mapping and migration plan. Co-Authored-By: Claude Opus 4.8 --- ...-25-claude-md-key-rules-refactor-design.md | 238 ++++++++++++++++++ 1 file changed, 238 insertions(+) create mode 100644 docs/plans/2026-06-25-claude-md-key-rules-refactor-design.md diff --git a/docs/plans/2026-06-25-claude-md-key-rules-refactor-design.md b/docs/plans/2026-06-25-claude-md-key-rules-refactor-design.md new file mode 100644 index 00000000..47c5adae --- /dev/null +++ b/docs/plans/2026-06-25-claude-md-key-rules-refactor-design.md @@ -0,0 +1,238 @@ +# Design: Refactor CLAUDE.md "Key Rules" into a tiered rules index + +**Date:** 2026-06-25 +**Status:** Approved design (not yet implemented) +**Scope:** Documentation only — no code changes. + +--- + +## Problem + +`CLAUDE.md` is 363 lines / ~42 KB. The `## Key Rules` section alone is **62 dense +prose rules, ~29.5 KB — roughly 70% of the whole file** — and all of it loads into +*every* session's context. + +This hurts on four axes at once (all four flagged as goals): + +- **Token cost** — ~7K tokens of always-on text taxes every conversation. +- **Readability** — 62 unsorted paragraphs are hard to scan; the relevant rule is + hard to find. +- **Reliability** — past the ~150–200 instruction ceiling that frontier models + reliably follow, rules dilute ("context rot") and adherence quietly drops. +- **Maintainability** — rules accrete with no stated home, format, or pruning rule. + +External guidance (Anthropic Agent Skills; 2026 CLAUDE.md best-practice writeups) +converges on: **keep the always-loaded file a "map, not the territory"** — answer +*what / why / how*, push detail into *referenced* docs (progressive disclosure), +aim for ≤ ~200 lines. + +Key nuance: `@import`-ed files are **eagerly loaded** into every session, so imports +help modularity but **not token cost**. Only *referenced* docs (read on demand, like +this repo's existing *Spec Document Index*) actually shrink the context tax. + +A second observation that shapes the design: the genuine cross-cutting **invariants +already live elsewhere** in CLAUDE.md — `## Architecture Constraints` +(events-as-stream, projections-never-stored, immutable state, dependency direction), +`## Things You Must Not Do`, and `## Segment Format Convention`. So `## Key Rules` is +**not** the invariant layer — it is almost entirely the *situational / domain-gotcha* +layer. The refactor must not duplicate the invariants already stated above. + +--- + +## Chosen approach (Option C — tiered: tripwires inline, bodies extracted) + +Considered three options: + +- **A. Group in place** — cluster the 62 rules under domain subheads. Readability + only; zero token savings; stays over the line ceiling. Rejected. +- **B. Full extraction** — move every rule to `.claude/rules/*.md`, leave only an + index. Max token savings, but gotchas become *unknown unknowns* — the agent won't + open `cricket.md` unless something cues it. Reliability dip. Rejected. +- **C. Tiered (chosen)** — keep a short inline *Workflow essentials* block + a + *Rules Index* table + one-line *tripwire headlines*; move the bulky explanations to + referenced domain files. The **warning still fires in-context** (reliability), the + **explanation is on-demand** (token cut), it's **grouped** (readable), and there's + a **clear home** for growth (maintainable). Hits all four goals. + +### Defaults + +- **Location:** `.claude/rules/` (idiomatic agent-rules home; keeps these out of the + published `docs/` tree). Referenced — **not** `@import`-ed — so they load on demand. +- **Granularity:** 10 domain files (below). X01 keeps its own file (flagship game, + room to grow) rather than folding into `game-engine.md`. + +--- + +## New shape of the `## Key Rules` section + +Three parts replace the 62-rule wall: + +### 1. Workflow essentials (stays fully inline, ~11 short lines) + +Rules that apply to *every* change and are cheap to keep hot — on-demand loading +would just cost a round-trip every session: + +- Branch-first; `/`, type ∈ {feat, fix, docs, chore, hotfix}. +- PR titles = soft Conventional Commits. +- Squash-merge only. +- Every PR reviewed via the `code-review:code-review` skill before merge; CI green. +- CI does **not** run `build_runner` — commit generated `.g/.freezed/.mocks` files. +- Run `flutter analyze --no-fatal-infos` as the **last** step before push. +- Never commit `pubspec.lock` / `.flutter-plugins-dependencies` unless deps changed. +- Stage explicit paths — never `git add -A` / `git add .`. +- Route all stat numbers through `StatFormatter` — CI greps `lib/features/*/presentation/` + for `toStringAsFixed` and fails on any match. +- Spec edits touch only the spec — never code, unless explicitly asked. +- After any UI refactor, update the test expectations in the same session. + +### 2. Rules Index (the new core — mirrors the Spec Document Index format) + +| Before you touch… | Read first | +|---|---| +| Cricket scoring / variants / labels | `.claude/rules/cricket.md` | +| X01 scoring / strategy / turn_score | `.claude/rules/x01.md` | +| Game events / rounds / payloads / RNG | `.claude/rules/game-engine.md` | +| Stats / projections / formatters | `.claude/rules/statistics.md` | +| Drift schema / DB / test fixtures | `.claude/rules/database.md` | +| Auto-scorer / camera / capture | `.claude/rules/auto-scorer.md` | +| UI / design tokens / navigation | `.claude/rules/ui-design.md` | +| Notifier / widget tests | `.claude/rules/testing.md` | +| Releases / version / CI / build tooling | `.claude/rules/git-ci-release.md` | +| E2E / Playwright | `.claude/rules/e2e.md` | + +### 3. Tripwire headlines (1–3 per domain, bare one-liners) + +The *surprising* fact stays in-context so the warning fires even before the file is +opened; the *explanation* is on-demand. Examples: + +- ⚠️ **Cricket variant labels** live in 3 aligned places — see cricket.md +- ⚠️ **Adding a cricket variant** = 4 edits incl. a registry test — see cricket.md +- ⚠️ **Cricket scoring × targetMode are orthogonal** — never hardcode `[15..20]` +- ⚠️ **`context.go()` wipes the back stack** — use `push()` for poppable nav +- ⚠️ **Completed games are read-only** — create incomplete, insert, then `completeGame()` +- ⚠️ **After a schema change** — bump `databaseVersion`, add `onUpgrade`, regen snapshots + +--- + +## Domain-file format + +`.claude/rules/.md` — flat list, one `###` heading per rule (greppable), +consistent `**Rule:** / **Why:**` body (mirrors the repo's memory-file convention): + +```markdown +# Cricket rules + +> Loaded on demand. CLAUDE.md's Rules Index points here before any cricket work. + +### Variant labels live in three aligned places +**Rule:** picker (`variant_selection_page.dart`, Title Case), in-game header +(`cricket_board_page.dart`, Title Case), history list +(`game_summary_card_widget.dart`, lowercase). Change one formula → audit the others. +**Formula:** fixed → `scoring` alone; random/crazy + standard → mode only; +random/crazy + non-standard → `mode · scoring`. + +### Adding a variant = four coordinated edits +**Rule:** (1) `_cricketVariants()` in `variant_selection_page.dart`, +(2) `cricketXxxRules` in `rules/content/cricket_rules.dart`, +(3) slug→rules in `kGameRules` (`rules_registry.dart`), +(4) slug in `expectedSlugs` in `rules_registry_test.dart`. +**Why:** the registry test fails CI if (4) is missed; the info-icon silently shows +"Rules unavailable." if (3) is missing — only the test enforces coverage. +``` + +--- + +## Rule → file mapping (lossless move checklist) + +All 62 current Key Rules, by destination. (Numbered in current top-to-bottom order.) + +**Inline — Workflow essentials (11):** GameConfig-edited-in-bottom-sheet stays UI; +these are the process rules → Branch naming, PR titles, Squash-merge, PR reviews, +CI-no-build_runner, Analyze-in-CI, plugins/lock-not-committed, Stage-explicit-paths, +Number-formatting (StatFormatter gate), Spec-edits, UI-refactors. + +**`cricket.md` (4):** Adding a cricket variant · Right-after-TurnStarted cricket +event · Cricket scoring × target mode orthogonal · Cricket variant labels in 3 places. + +**`x01.md` (2):** X01 strategy values lowercase · X01 `TurnEnded` carries `turn_score`. + +**`game-engine.md` (8):** GameConfig dispatch (`maybeMap`) · DartThrown payload keys · +`local_sequence` per-game · RNG in use cases · Round semantics (`totalRounds`) · +Per-leg round cap · `DartCorrected` payload key (`original_event_id`) · +`endGame()/endDrill()` don't mutate `isComplete`. + +**`statistics.md` (7):** Statistics scope resets · Computing stats over an event slice +(`PlayerStatsAssembler`) · `GameStats.gameType` load-bearing · Statistics loader vs +computation · Cricket mark-bucket field overload · Statistics scope required +(`gameType`) · Projection snapshots two-level. + +**`database.md` (8):** Repository exceptions · Contract tests · Database +(versions/migrations) · Drift foreign keys · Test database setup · Test game setup +ordering · Watchable queries · Repository contract tests (`runHybridTests`). + +**`auto-scorer.md` (8):** Camera preview · YOLOView path · Capture sidecar contract · +Device-session regression fixtures · Capture writes respect opt-in · `DartInputSink` +(submitDart/advanceTurn) · Camera/device-only not widget-testable · Camera-first +board layout + tests. + +**`ui-design.md` (5):** Game config bottom sheet · Colors · DESIGN_SYSTEM specifics · +Stats breakdown tables reuse `PostGameStatsBreakdown` · Navigation `go()` vs `push()`. + +**`testing.md` (2):** Notifier tests · Widget test finders. + +**`git-ci-release.md` (5):** Releases tag-driven · Version bumps · `flutter create` +stray `widget_test.dart` · "Unused" in `lib/` may be wiring · Sentry error handlers. + +**`e2e.md` (2):** E2E regression reminders · Driving Playwright against local builds. + +Total: 11 inline + 51 in files = **62** (lossless). + +--- + +## Maintenance convention (added to CLAUDE.md so the structure self-perpetuates) + +- **New rule → domain file, not CLAUDE.md.** Add a `###` entry to the matching + `.claude/rules/*.md`. Add to the inline blocks only if truly cross-cutting (applies + to every change) *and* short. +- **Tripwire only when surprising/destructive.** Routine rules get an index row, not + a headline — over-adding headlines is what causes re-bloat. +- **Prune when gated.** When a rule becomes enforced by a test/lint/CI gate, shrink + the prose to a pointer at the gate ("enforced by `rules_registry_test.dart`"). The + gate is the source of truth. +- **Revise like code; be specific.** "Use X" not "prefer X when possible." If an + agent gets something wrong twice, that's a missing/weak rule. +- **Index ↔ files stay in sync** — every `.claude/rules/*.md` has exactly one Rules + Index row. + +--- + +## Migration plan + +1. Branch `docs/refactor-claude-md-key-rules` off `main`. *(done — this design lives here.)* +2. Create the 10 `.claude/rules/*.md` files, moving each rule's **full current text + verbatim** (lossless move first — no rewriting). +3. Rewrite the `## Key Rules` section to the new shape (Workflow essentials + Rules + Index + tripwires), and add the Maintenance convention block. +4. Verify nothing dropped against the 62-rule mapping table above (reproduce it in the + PR description with a ✓ per rule). +5. *(Optional second pass)* tighten verbose entries to the `Rule:/Why:` format. +6. Open PR; review via the `code-review:code-review` skill; report before/after + line + KB counts in the PR body. + +Doc-only change — no code tests at risk. The only "test" is the lossless-move checklist. + +### Expected outcome + +`## Key Rules` ~29.5 KB → roughly **6–8 KB inline** (essentials + index + tripwires); +CLAUDE.md back under the ~200-line / ~200-instruction guidance; the ~21 KB of detail +moves to 10 on-demand files that load only when their domain is in play. + +--- + +## References + +- [Anthropic — Equipping agents with Agent Skills](https://www.anthropic.com/engineering/equipping-agents-for-the-real-world-with-agent-skills) +- [Skill authoring best practices](https://platform.claude.com/docs/en/agents-and-tools/agent-skills/best-practices) +- [Progressive Disclosure as a System Design Pattern](https://www.newsletter.swirlai.com/p/agent-skills-progressive-disclosure) +- [CLAUDE.md Best Practices: The Complete 2026 Guide](https://maketocreate.com/claude-md-best-practices-the-complete-2026-guide/) +- [CLAUDE.md Best Practices: 9 Rules for 2026 (TECHSY)](https://techsy.io/en/blog/claude-md-best-practices) From 87e5905c61b776f85398fa83548fdb26db97b4b4 Mon Sep 17 00:00:00 2001 From: mathrb Date: Thu, 25 Jun 2026 13:06:18 +0000 Subject: [PATCH 2/3] docs: refactor CLAUDE.md Key Rules into a tiered rules index MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the 60-rule / ~29.5 KB Key Rules wall with three lean parts: - Workflow essentials (inline) — the rules that apply to every change - a Rules Index table (mirrors the Spec Document Index) pointing to 10 on-demand docs/rules/*.md domain files - one-line tripwire headlines for the most surprising/destructive gotchas All 60 rules moved losslessly into docs/rules/{cricket,x01,game-engine, statistics,database,auto-scorer,ui-design,testing,git-ci-release,e2e}.md. Adds a maintenance convention so the section stays lean. CLAUDE.md 42 KB -> 18 KB; Key Rules section 29.5 KB -> 5.4 KB inline, back under the ~200-line always-loaded guidance. Co-Authored-By: Claude Opus 4.8 --- CLAUDE.md | 157 +++++------------- ...-25-claude-md-key-rules-refactor-design.md | 68 ++++---- docs/rules/auto-scorer.md | 27 +++ docs/rules/cricket.md | 15 ++ docs/rules/database.md | 27 +++ docs/rules/e2e.md | 6 + docs/rules/game-engine.md | 27 +++ docs/rules/git-ci-release.md | 25 +++ docs/rules/statistics.md | 30 ++++ docs/rules/testing.md | 7 + docs/rules/ui-design.md | 15 ++ docs/rules/x01.md | 9 + 12 files changed, 264 insertions(+), 149 deletions(-) create mode 100644 docs/rules/auto-scorer.md create mode 100644 docs/rules/cricket.md create mode 100644 docs/rules/database.md create mode 100644 docs/rules/e2e.md create mode 100644 docs/rules/game-engine.md create mode 100644 docs/rules/git-ci-release.md create mode 100644 docs/rules/statistics.md create mode 100644 docs/rules/testing.md create mode 100644 docs/rules/ui-design.md create mode 100644 docs/rules/x01.md diff --git a/CLAUDE.md b/CLAUDE.md index 2ed0b5f4..c192239f 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -198,125 +198,44 @@ Used in `dart_throws.segment`, `DartThrown` event payloads, and all engine logic ## Key Rules -**GameConfig dispatch:** Use `maybeMap` (not `maybeWhen`) — callbacks receive typed subclass instances: `config.maybeMap(x01: (c) => c.startingScore, orElse: () => '')`. Requires explicit `import 'game_config.dart'`; not available via transitive import. - -**Game config is edited in a bottom sheet, not a page:** X01/Cricket in/out strategy, legs-to-win, and starting score are set via the config-summary chip → `GameConfigPanel` bottom sheet on the player-selection screen (`APPLY` commits). `GameConfigPage` exists but is unwired (and there is no "Custom" variant tile — it was removed pre-1.0, #684). The `LEGS TO WIN` `ConfigStepperWidget` +/- icon buttons carry localized `semanticLabel`s (`setupLegsIncrement`/`setupLegsDecrement`, #666 fixed); the X01 config-summary chip surfaces `legsToWin` as a localized `setupLegsCount` segment, matching Cricket (#667 fixed). (E2E driving notes: `docs/E2E_REGRESSION.md` § Authoring specs.) - -**Repository exceptions:** All exceptions extend `RepositoryException` (`lib/core/error/repository_exception.dart`). Never throw raw `Exception` from a repository implementation. - -**Contract tests:** Every repository implementation must pass the shared contract tests in `test/contracts/`. Never skip or comment out tests to make CI pass. - -**Database:** drift on every platform (`NativeDatabase.createInBackground` on mobile/desktop, `WasmDatabase` over IndexedDB on web). Schema lives in `lib/core/persistence/drift/database.dart`; `databaseVersion = 2` (v1→v2 added `unlocked_achievements`, #522 — the first migration, applied via `MigrationStrategy.onUpgrade` `m.createTable`). `PRAGMA foreign_keys = ON` is set in `MigrationStrategy.beforeOpen`, which runs on every connection (including immediately after `onUpgrade`), so migrations don't need to toggle it themselves — and adding a brand-new table via `m.createTable` needs no FK-disable dance (CREATE TABLE never triggers FK checks). Completed games are read-only — enforced in application logic, not triggers. The canonical SQL DDL reference is `docs/DATABASE_DDL.md`. After editing drift table classes, run `dart run build_runner build`. **After any schema change, also bump `databaseVersion`, add an `onUpgrade` step, and regenerate the schema snapshots: `dart run drift_dev schema dump lib/core/persistence/drift/database.dart drift_schemas/` then `dart run drift_dev schema generate drift_schemas/ test/drift_schemas/` (committed; `test/core/persistence/drift/migration_test.dart` verifies upgrades with drift's `SchemaVerifier`).** - -**Drift foreign keys:** Plain `text()()` emits NO foreign key clause — you must call `.references(Type, #col, onDelete: KeyAction.{cascade|restrict|setNull})` explicitly. `PRAGMA foreign_keys = ON` is a no-op without `.references()`. When two columns in a table reference the same parent (e.g. `game_sessions.host_player_id` and `current_turn_player_id` both → `players`), add `@ReferenceName('xxx')` annotations to disambiguate manager-API helpers, or build_runner warns. - -**Test database setup:** Drift tests use `AppDatabase(NativeDatabase.memory())` directly — see `test/drift_test_base.dart`. With FK enforcement active, fixtures must respect FK order: insert players before competitors, games before competitors/dart_throws/game_events, and use `playerRepo.createPlayer()` to seed referenced player IDs before any `createGame` call. - -**Test game setup ordering:** Drift enforces read-only on completed games. In tests: create game with `isComplete: false` → insert darts/events → call `gameRepo.completeGame()`. Never set `isComplete: true` at creation if you need to insert data afterward. - -**Statistics scope resets:** Turn resets on `TurnStarted`, Leg resets on `LegCompleted`, Match resets on `GameCompleted`. No other reset points. - -**DartThrown payload keys:** `competitor_id`, `player_id`, `segment`, `multiplier`, `score`, `input_method`, plus optional `x`/`y` (see `buildDartThrownEvent` in `lib/features/game/domain/usecases/game_use_case_helpers.dart`). `x`/`y` are the auto-scorer's normalised canonical impact position (#571, canonical scoring frame: origin = centre, radius 1.0 at the double ring, **5/20 calibration wire at top** so segment 20's centre is ~9° clockwise of vertical; the heatmap widget rotates only the *display* via `kHeatmapDisplayRotation` to show a standard "20 at top" board, #697 — stored values stay in the wire-at-top frame) — **omitted when null** (manual entry, noise, pre-#571 events) so old events parse unchanged. The same values are written to `dart_throws.x`/`.y`. The use cases read them off the `DartThrow` they already receive (the notifier sets them); a correction/undo deletes the dart and re-entry is manual → position stays null by design. No `turn_number`, no `dart_number` — reconstruct turn grouping via `TurnStarted`/`TurnEnded` event boundaries if needed. - -**Computing stats over an event slice:** All projection wiring lives in `PlayerStatsAssembler` (`lib/features/statistics/domain/assemblers/`). Use the method that matches the scope: `fromEvents` (career), `gameStatsFromEvents` (per-game), `playerStatsForGameFromEvents` (per-player-per-game), `legCompetitorStatsFromEvents` (per-leg). Repos and use cases load events and delegate. If you need a new projection bundle, add it to the assembler — do not re-wire `ProjectionRunner` directly in repos or use cases. Snapshot keys: `x01_average`, `x01_checkout`, `x01_highest_checkout`, `x01.highScoreBuckets`, `cricket.mpt`, `cricket.markBuckets`, `cricket.firstNineMpr`. First-nine projections (`cricket.firstNineMpr`, X01 first-nine PPR) only count when `TurnStarted` events are present — fixtures emitting just `DartThrown`/`TurnEnded` silently produce null first-nine stats. - -**`local_sequence` is per-game, not global:** every new game restarts `local_sequence` at 1, so multiple games' events share the same sequence range. Any query that loads events across multiple games MUST sort by `(game_id, local_sequence)` — sorting by `local_sequence` alone interleaves games and corrupts projection state across game boundaries. `ProjectionRunner.run()` enforces this internally; SQL queries feeding it should match. - -**`GameStats.gameType` is load-bearing:** the post-game summary branches on `gameStats.gameType == GameType.cricket.name` to choose MPR vs PPR labels and rows. Every return path of `getGameStats` (including the empty-darts early return) must set it, in both repository implementations. - -**Statistics loader vs computation:** Statistics computation lives in `PlayerStatsAssembler` (shared). The loader queries live in `lib/core/persistence/drift/repositories/statistics_repository_drift.dart` — load events + dart_throws, then delegate to the assembler. When changing how stats are computed, update only the assembler. - -**Watchable queries:** drift's per-query reactivity is automatic — `select(...).watch()` re-fires whenever drift sees a write to one of the referenced tables. No notify-after-write protocol to remember. - -**Repository contract tests:** `runHybridTests` (`test/hybrid_test_runner.dart`) spins up a fresh in-memory `AppDatabase` per test against the shared `*_contract.dart` suites. The "hybrid" name is vestigial from the dual-backend era (issue #112) — there is now a single backend. - -**Adding a cricket variant:** four coordinated edits — (1) `_cricketVariants()` entry in `variant_selection_page.dart`, (2) a `cricketXxxRules` content block in `rules/content/cricket_rules.dart`, (3) the slug → rules entry in `kGameRules` (`rules_registry.dart`), (4) the slug in `expectedSlugs` in `rules_registry_test.dart` (the registry test fails CI if you miss this). The info-icon shows "Rules unavailable." silently if (3) is missing — only the test enforces coverage. - -**Adding a "right-after-TurnStarted" cricket event:** every site that emits the event (`CreateGameUseCase`, `ProcessCricketDartUseCase`, the three TurnStarted emission sites in `active_cricket_game_provider.dart`) must emit it; AND `UndoLastDartUseCase` must add the event type to BOTH its supersession-collection loop and its replay-skip branch — otherwise a turn-boundary undo replays the cancelled turn's event and corrupts state. Same applies to projections: add to `consumedEventTypes` on every cricket projection (or extend the shared `CricketTargetsTracker` mixin) AND the `legHistoryFromEvents` inline tracker in `PlayerStatsAssembler`. - -**RNG in use cases:** event-emitting use cases that need randomness (e.g. `CreateGameUseCase`, `ProcessCricketDartUseCase` for `CrazyTargetsRolled`) accept an optional `math.Random?` constructor parameter that defaults to `math.Random()` in production. Tests inject a seeded `math.Random(seed)` for determinism. RNG runs **once at emission**, the value lands in the event payload, and `engine.apply()` is pure — replay never re-rolls. - -**Cricket scoring × target mode are orthogonal axes:** `CricketGameConfig` exposes `scoring` ∈ {`standard`, `cut-throat`, `no-score`} and `targetMode` ∈ {`fixed`, `random`, `crazy`}; `GameState` mirrors them as `cricketScoring`/`cricketTargetMode` plus a dynamic `cricketTargets: List` (+ implicit Bull) and `cricketLockedTargets: Set`. The engine reads the target set from state — never from a hardcoded `[15..20]` constant — so any target mode is accepted by the same code path. Legacy configs carrying a single `variant` string deserialise to `{scoring: , targetMode: 'fixed'}` via a `readValue` mapping at JSON read time; **no event migration**, historical replay stays correct. Stats loader buckets cricket games by `targetMode` (today only `fixed` is wired; `random`/`crazy` cohorts arrive with PRs #237/#238). See `docs/plans/2026-05-19-cricket-target-modes-design.md`. - -**Cricket mark-bucket field overload:** `CompetitorStats.{five..nine}MarkTurns` are populated as **exact-N** counts by `getGameStats` and `ComputeLegStatsUseCase` (read from the `*Exact` snapshot keys) but as **≥-N** counts by `getPlayerStats` (read from the `*MarkTurns` keys). Same field, different cohorts by call path. - -**Statistics scope is required:** `getPlayerStats` and `watchPlayerStats` take `required GameType gameType`. PPR-shaped fields are X01-only and cricket fields are cricket-only — a single call cannot mix types coherently. The player-picker AVG badge consumes `playerStatsProvider`, which passes `GameType.x01`. - -**Notifier tests:** Use `ProviderContainer` with `overrides`. Never instantiate notifiers directly. Use `ProviderScope` with `overrides` for widget tests. - -**Widget test finders:** `StatFormatter.fmtDouble` strips trailing zeros — e.g. `170.0` → `'170'`, which collides with raw `int` values rendered the same way. Prefer `findsNWidgets(n)` or more specific finders (`find.descendant`) over `findsOneWidget` when stat rows may share literals. - -**Colors:** Always use themed color tokens from `docs/design/DESIGN_SYSTEM.md`. Never hardcode color values directly in widgets. - -**DESIGN_SYSTEM specifics the review repeatedly catches:** inactive/opponent score numerals use `cs.onSurfaceVariant` (active = `onSurface`, practice target = `primary`); `label-sm` over-line above a hero numeral uses `primaryFixed` (not `onSurfaceVariant`); game-board player names = `labelMedium` ALL-CAPS `letterSpacing: 1.2`; score numerals never scale/wrap — constrain the container, never wrap a score in `FittedBox(scaleDown)`. - -**Number formatting:** Use `StatFormatter` (`lib/core/utils/stat_formatter.dart`) for all statistics display — `fmtDouble`, `fmtPct`, `fmtPerLeg`. Never use inline `toStringAsFixed()` in statistics UI. `test.yml`'s "Stats UI formatter gate" greps `lib/features/*/presentation/` for `toStringAsFixed` and fails CI on ANY match (not just stats) — route every number through `StatFormatter` (it wraps `toStringAsFixed` in `core/utils`, outside the gate), even non-stat labels like a `2.5×` zoom readout. - -**Auto-scorer camera preview:** `CameraPreview` handles device orientation itself (its own internal `AspectRatio` + `RotatedBox`; `controller.value.aspectRatio` is ALWAYS the landscape sensor ratio). Do NOT wrap it in your own `Center > AspectRatio(controller.value.aspectRatio)` — on a portrait screen that shrinks the preview to a letterboxed band (this is what #408 did). Detection runs on `takePicture()`'s raw sensor frame (landscape buffer), **served as-is — the app does not rotate it**. The model needs the board roughly upright, so a portrait-held phone (board sideways in the landscape buffer) detects poorly until the model is trained for that orientation (#393); the app-side fix is **not** rotation (a rotation auto-detect was tried and dropped — it can't tell upright from upside-down). Preview-display orientation and detection-input orientation are independent — don't conflate them. - -**Auto-scorer camera (YOLOView path):** the in-game preview + aim view run a live `YOLOView` with native streaming inference (`onResult` ~3 Hz, `inferenceFrequency: 3`) — NOT a `takePicture`/`_busy` polling loop (`_detectTick`/`_tick`/`_busy` are gone). Frame stills come from `YOLOViewController.capturePhoto(withOverlays: false)`. The auto/fire-and-forget callers (`_captureEmitted`/`_captureCorrected`) run in a `try/catch` (a dropped capture must never disrupt scoring) with no busy guard. The user-triggered manual buttons (`_manualCapture`, the aim view's `_capture`) instead `tapToFocus(0.5,0.5)` + await `kAutoScorerFocusSettle` before the shot (capturePhoto does NOT autofocus, so a manual still is otherwise blurry) and hold a `_capturing` flag that disables the button while in flight (debounces double-taps); that flag does NOT gate the auto callers, which can still interleave. Every manual path also gates on `dataCollectionEnabledProvider` before persisting (like the auto paths). The legacy predict path (`AutoScorerSession.onFrame`/`detectOnly` → `DartDetector`) still exists but isn't the in-game path. - -**Auto-scorer capture sidecar is a probe contract:** the `CaptureRecord` JSON shape (`capture_record.dart`) is the `ddp-preprocess` ingest contract. Adding a key = coordinated edit: `toJson` + `fromJson` (with a backward-compat default for old sidecars) + `withCorrection` (preserve it) + the `unorderedEquals` key-set test in `capture_record_test.dart` (fails CI otherwise). Probe-side consumption needs a reciprocal change in the probe repo. - -**Device-session regression fixtures (#488):** `SessionReplayer` rebuilds the tracker from the fixture's EMBEDDED `TrackerSegment` config, so a raw capture replays with its original (possibly buggy) config — its recorded outcomes ARE the bug. To use a capture as a regression test for a fix, hand-edit the embedded config in the fixture to the fixed value and assert `isFaithful == false` (the fixed replay must diverge from the buggy recording) + the corrected invariants on `frame.replayed` — never assert against the recorded outcomes. - -**Capture writes respect the opt-in:** any path that persists a training capture (emission, manual button, or correction-driven) MUST gate on `dataCollectionEnabledProvider` — the store is non-null even when the toggle is off, so an ungated write silently hoards frames the user opted out of. - -**`DartInputSink` carries `submitDart` AND `advanceTurn`:** the auto-scorer→game port has two methods. `advanceTurn()` backs the opt-in "auto-advance when board is cleared" feature (`autoAdvanceOnClearEnabledProvider`, default off): the YOLOView preview calls it on a board-clear (`TrackerPhase.rebaselined`) **guarded by `_sawDartsThisTurn`** — `rebaselined` also fires on a transform-only empty board at turn start, so without that guard it skips players who haven't thrown. Each board's sink impl must **no-op when a modal/celebration is pending or the game is complete** (X01's `advanceTurn` dismisses the bust/leg modals, so without the guard an auto-advance would blow past an unacknowledged win; cricket's `nextPlayer` doesn't dismiss them — the guard is the sole safety net) and must bump `activeTurnSignal` like the manual NEXT button. Pure decision lives in `shouldAutoAdvance` (`domain/tracking/auto_advance.dart`); the widget trigger is device-only. The board-clear itself is a **confirm-before-clear** gate: the board must read empty for `DartTrackerConfig.emptyFramesToRebaseline` consecutive *calibrated* frames before `_rebaseline()` fires (confirmed darts are retained meanwhile, so a dart reappearing before the threshold resets the count and re-matches without re-emitting). The default is **9 frames ≈ 3s at `kAutoScorerInferenceHz`=3** — deliberately long so a transient dart-detection flicker or brief darts-only occlusion (~1s) is NOT mistaken for a pull (premature advance + double-counted dart, #499); a genuine pull leaves the board empty for several seconds and still clears. - -**Round semantics:** A "round" is one full rotation where ALL competitors throw. `totalRounds` is the correct field name. Do not use `maxRounds` or count per-competitor turns or individual dart throws as rounds. - -**Per-leg round cap:** X01 and Cricket enforce a round cap per leg (see `GameConfigurationConstants` and engine logic). When the cap is hit with no winner, the leg is decided by current standing — do not extend rounds silently. Both engines and any UI showing round progress must respect this. - -**Spec edits:** When asked to update a spec or document, only edit that document — do not modify code files unless explicitly asked. - -**UI refactors:** After any widget redesign or UI refactor, update the corresponding test expectations in the same session before committing. - -**E2E regression reminders:** The committed Playwright suite (`e2e/*.spec.ts`) is tag-sliced and **run manually by choice** (no CI gate — though it does run green headless via Playwright's bundled Chromium / software GL; gating is a future option, not a blocker). After changing a game engine, scoring/stats projection, correction/undo flow, localized strings, or the auto-scorer sink, consult the coverage map in `docs/E2E_REGRESSION.md` and remind the user which `npx playwright test --grep @tag` slice to run before merging. Never assume the suite ran. When adding a game/feature, add a tagged spec (and a coverage-map row) in the same PR. - -**Navigation — `context.go()` vs `context.push()`:** `context.go()` REPLACES the entire route stack — Android's physical back button then has nothing to pop and exits the app. Use `context.push()` for any forward navigation that should be back-poppable (Home → Stats/History/Players/Settings, list → detail, etc.). Reserve `context.go()` for intentional stack resets: game completion → home, post-deletion redirects, deep-link landing pages. If a screen MUST be reached via `go()` (e.g. the variant selection flow), wrap its body in `PopScope(canPop: false, onPopInvokedWithResult: (didPop, _) { if (!didPop) context.go(GameRoutes.home); })` like `variant_selection_page.dart` does, so the Android back button still works. - -**Branch naming:** All work goes on a branch off `main` named `/` where type ∈ {`feat`, `fix`, `docs`, `chore`, `hotfix`}. Slugs are short and dash-separated (`feat/cricket-stats-export`). Never commit directly to `main`. - -**PR titles:** Soft Conventional Commits — `feat(cricket): ...`, `fix(x01): ...`, `docs: ...`, `chore(deps): ...`. PR titles become squash-merge commit messages and feed GitHub's auto-generated release notes. - -**Squash-merge only:** PRs are always squash-merged. Don't rebase-merge or merge-commit. Branches auto-delete after merge. - -**PR reviews:** every PR — including small or "obvious" ones — gets reviewed via the `code-review:code-review` skill before merge. Self-review via `gh pr diff` is not sufficient. The skill runs an 8-step pipeline (eligibility → CLAUDE.md fetch → summary → 5 parallel Sonnet reviews covering CLAUDE.md compliance / bugs / git history / prior PR comments / in-code comments → confidence scoring at 0/25/50/75/100 → filter ≥80 → post). Issues that score below 80 should still be fixed by the author if real (just not posted as inline comments). CI must be green before merging. - -**Releases are tag-driven:** Pushing a tag `vX.Y.Z` (or `vX.Y.Z-rcN` for pre-release) triggers `release.yml`, which builds and publishes the signed APK to GitHub Releases. Every merge to `main` also auto-tags `v-rc` (N = next-available rc number) via `auto-rc.yml` and publishes a pre-release — devs do not push RC tags manually. Never manually upload an APK to a release. Tags must point to a commit that's reachable from `main` (`release.yml` enforces this). Full process in `docs/RELEASES.md`. - -**Version bumps:** When asked to bump the version, edit only `pubspec.yaml`'s `version:` field (e.g. `1.0.0+0` → `1.1.0+0`) in a `chore: bump version to X.Y.Z` PR. The `+N` suffix is a placeholder; CI overrides `versionCode` from `github.run_number` on tag builds. - -**CI does not run `build_runner`:** Generated `.g.dart` / `.freezed.dart` / `.mocks.dart` files are committed. After editing any `@freezed`, `@riverpod`, or `@GenerateMocks` annotation, regenerate locally and commit the result in the same PR — CI will fail otherwise. - -**Analyze in CI:** `test.yml` runs `flutter analyze --no-fatal-infos`. Warnings block CI; infos are advisory. ~190 info-level lints are tolerated (deprecated `overrideWith`, `curly_braces_in_flow_control_structures`, `avoid_print` in test infra). Cleaning them is optional polish — never tighten this flag without raising it. **Always run project-wide `flutter analyze --no-fatal-infos` before pushing — `flutter analyze ` may not surface unused-import / unused-variable warnings that the project-wide variant catches.** For a fast pre-push check that filters out the info noise, grep: `flutter analyze --no-fatal-infos 2>&1 | grep -E '^\s*(warning|error) •'` — empty output means CI-clean. Run this **after** your last file change, not just once early in the session: warnings introduced by later test/import edits will otherwise slip through. - -**`flutter create` drops a stray `test/widget_test.dart`:** scaffolding `android/`/`web/`/`ios/` per machine re-creates a default `test/widget_test.dart` that references a non-existent `MyApp`. It's untracked so CI never sees it, but it fails local `flutter test` and trips `flutter analyze` with a phantom `MyApp isn't a class` error — don't chase it as a real regression. `rm test/widget_test.dart` after scaffolding. - -**"Unused" in `lib/` may be forgotten wiring:** When `flutter analyze` flags an unused field, parameter, or import in `lib/`, check whether it represents incomplete wiring (a setter that updates a field nothing reads, a constructor param never used in the body) before deleting. If unsure, ask — silent deletion can lock in a no-op user-facing control as the intended behavior. - -**Camera/device-only changes aren't widget-testable** (no `CameraPreview` in `flutter test`, no web camera) — verify on a real device. PR builds upload the debug APK as a 7-day artifact (`build-apk.yml`); download it from the PR's *Build APK* Actions run (or `gh run download `) to sideload without merging. Different signing key than local/release builds → uninstall the existing app first. - -**Camera-first board layout + tests:** boards branch on `cameraFirst = autoScoringOn && cameraPreview != null` — the camera-first column lives behind `if (cameraFirst)` (manual layout in `else`), `GameStatusBarWidget(showDarts: false)` moves the darts to `ProminentDartBandWidget`, and the primary state uses `HeroMetricWidget` / per-game strips (`X01OtherPlayersStripWidget`, `CricketMarksStripWidget`, `PracticePlayersStripWidget`; `PracticeTargetDisplayWidget(heroSize: true)`). Inside `if (cameraFirst)`, Dart promotes `cameraPreview` non-null — call it WITHOUT `!` (else `unnecessary_non_null_assertion`). Unlike the live `CameraPreview`, the camera-first **chrome IS widget-testable**: override `autoScoringEnabledProvider` with a fake whose `build()` returns true AND `boardCameraPreviewBuilderProvider.overrideWithValue((c, id) => const SizedBox(key: ValueKey('camera-stub')))`. Default board tests run in manual mode (neither overridden → `cameraFirst` false). - -**X01 strategy values are lowercase short forms:** `'straight'` / `'double'` / `'master'` for both `inStrategy` and `outStrategy` (`GameState` defaults). Engines and projections compare against these literals; UI labels (e.g. "Double Out") live in a display mapper only. Never store the friendly labels. - -**`DartCorrected` payload key is `original_event_id`:** a string referencing the corrected `DartThrown.eventId`. Any replay-aware code path (`UndoLastDartUseCase`, `PlayerStatsAssembler.fromEvents`) must collect these and skip the originals. - -**Projection snapshots are two-level:** top level keyed by `engine.descriptor.id` (e.g. `'x01.doubleOut'`), inner map keyed by field name (e.g. `'doubleOutSuccessRate'`). Wiring a new engine into `PlayerStatsAssembler.fromEvents` means reading at both levels — running an engine without reading its snapshot is a silent no-op. - -**`.flutter-plugins-dependencies` and `pubspec.lock`** regenerate on every `flutter pub get` / `flutter run` / `build_runner build`; never commit either unless the dep set actually changed. Both commonly show `M` in `git status` — `git checkout pubspec.lock .flutter-plugins-dependencies` before staging to keep PR diffs clean. - -**Stage explicit paths — never `git add -A` / `git add .`:** the working tree carries many untracked scratch files (`*.png` / `*.yml` / exported capture `*.jpg` at the repo root, `e2e/node_modules`, `.playwright-*`). A blanket add stages hundreds of junk files — always `git add `. - -**Sentry error handlers:** `SentryFlutter.init` auto-installs `FlutterError.onError` and `PlatformDispatcher.instance.onError` via `FlutterErrorIntegration` and `OnErrorIntegration` (sentry_flutter ≥ ~7.x; current pin `^9.16.1`). Do NOT add manual handlers in `main.dart` — they would override Sentry's wiring and silence the crash pipeline. See the `lib/main.dart` header comment. **Crash reporting is opt-out (default on):** `main.dart` reads `kCrashReportingPrefKey` from SharedPreferences *before* `SentryFlutter.init` and skips init entirely when disabled (so the handlers are installed only when enabled), surfacing the toggle via the `CrashReportingEnabled` provider in Settings → Feedback. A clean runtime re-init isn't supported and native crashes bypass `beforeSend`, so the toggle takes effect on the next launch — do NOT "fix" the conditional wrapper into an unconditional call. The `Report a Bug` action gates on `Sentry.isEnabled` (a safe `NoOpHub` no-op when off) so feedback is never silently dropped. - -**`endGame()` / `endDrill()` write `is_complete=true` to the DB but do NOT mutate `state.value.gameState.isComplete`** — the post-game-navigation listener (`practice_board_page` / `x01_board_page` etc.) watches that flag and would otherwise route every menu-driven exit through post-game instead of home. When you need an authoritative "is this game complete?" signal outside the active-game provider (e.g. from the router's `onExit`), read it from `GameRepository.getGame(id)`, not the notifier state. See `app_router.dart`'s `_gameIsComplete` helper for the pattern. - -**Cricket variant labels live in three places and must stay aligned:** `variant_selection_page.dart` (picker — Title Case), `cricket_board_page.dart` (in-game header — Title Case), `game_summary_card_widget.dart` (history list — lowercase by design). When you change one site's `scoring × targetMode` label formula, audit the other two. The shared rule: fixed → `scoring` alone; random/crazy + standard → just the mode; random/crazy + non-standard → `mode · scoring`. - -**X01 `TurnEnded` payload carries `turn_score` (`turn_start_score - turn_end_score` per `docs/statistics/x01.projections.md` §5.2).** `ProcessDartUseCase` computes it; `buildTurnEndedEvent` accepts an optional `turnScore` int. `X01AverageProjection` prefers this delta over per-dart sum (so bust + Double-In not-in turns contribute 0 to PPR), and falls back to dart-sum when absent for backward compatibility with pre-#318 events. Any new X01 dart-emission path MUST pass `turnScore` or new games will use the legacy convention. +> Hard-won project rules. The everyday **Workflow essentials** below apply to every change and stay inline. Everything else is grouped by domain in the **Rules Index** — open the matching `docs/rules/*.md` file *before* you start work in that area. The one-line ⚠️ tripwires flag the most surprising/destructive gotchas so the warning fires even before you open the file. + +### Workflow essentials (apply to every change) + +- **Branch first.** All work goes on a branch off `main` named `/`, type ∈ {`feat`, `fix`, `docs`, `chore`, `hotfix`}, slug short and dash-separated (`feat/cricket-stats-export`). Never commit directly to `main`. +- **PR titles** = soft Conventional Commits (`feat(cricket): …`, `fix(x01): …`, `docs: …`, `chore(deps): …`) — they become the squash-merge message and feed release notes. +- **Squash-merge only.** Never rebase-merge or merge-commit. Branches auto-delete after merge. +- **Every PR is reviewed** via the `code-review:code-review` skill before merge (not `gh pr diff`); CI must be green. Full pipeline → `docs/rules/git-ci-release.md`. +- **CI does not run `build_runner`.** Generated `.g.dart` / `.freezed.dart` / `.mocks.dart` are committed — after editing any `@freezed` / `@riverpod` / `@GenerateMocks`, regenerate locally and commit in the same PR. +- **Run `flutter analyze --no-fatal-infos` as the LAST step before push** (project-wide, not per-path). Quick CI-clean check: `flutter analyze --no-fatal-infos 2>&1 | grep -E '^\s*(warning|error) •'` → empty = clean. Detail → `docs/rules/git-ci-release.md`. +- **Stage explicit paths — never `git add -A` / `git add .`.** The tree carries many untracked scratch files (`*.png`/`*.jpg`/`*.yml` at the repo root, `e2e/node_modules`, `.playwright-*`); a blanket add stages junk. +- **Never commit `pubspec.lock` / `.flutter-plugins-dependencies`** unless the dep set actually changed (`git checkout` them before staging). +- **Route every displayed number through `StatFormatter`** (`lib/core/utils/stat_formatter.dart`) — `test.yml` greps `lib/features/*/presentation/` for `toStringAsFixed` and fails CI on ANY match. Detail → `docs/rules/statistics.md`. +- **Spec edits touch only the spec** — never code, unless explicitly asked. +- **After any UI refactor, update the test expectations in the same session** before committing. + +### Rules Index — open the matching file before working in that area + +| Before you touch… | Read first | ⚠️ Top tripwires | +|---|---|---| +| Cricket scoring / variants / labels | `docs/rules/cricket.md` | Variant labels live in **3 aligned places**; adding a variant = **4 edits incl. a registry test**; scoring × targetMode are orthogonal — **never hardcode `[15..20]`** | +| X01 scoring / strategy / turn_score | `docs/rules/x01.md` | Strategy values are lowercase (`'straight'`/`'double'`/`'master'`); `TurnEnded` must carry `turn_score` | +| Game events / rounds / payloads / RNG | `docs/rules/game-engine.md` | `local_sequence` is **per-game** — sort by `(game_id, local_sequence)`; a "round" = full rotation (`totalRounds`); `DartCorrected` key is `original_event_id`; `endGame/endDrill` don't mutate `gameState.isComplete` | +| Stats / projections / formatters | `docs/rules/statistics.md` | All projection wiring lives in `PlayerStatsAssembler`; `GameStats.gameType` is load-bearing; snapshots are two-level | +| Drift schema / DB / test fixtures | `docs/rules/database.md` | **Completed games are read-only** — create incomplete → insert → `completeGame()`; after any schema change bump `databaseVersion` + `onUpgrade` + regen snapshots; FKs need explicit `.references()` | +| Auto-scorer / camera / capture | `docs/rules/auto-scorer.md` | Don't wrap `CameraPreview` in your own `AspectRatio`; **every capture write gates on `dataCollectionEnabledProvider`**; camera changes aren't widget-testable | +| UI / design tokens / navigation | `docs/rules/ui-design.md` | Always use DESIGN_SYSTEM tokens — never hardcode colors; **`context.go()` wipes the back stack** — use `push()` for poppable nav | +| Notifier / widget tests | `docs/rules/testing.md` | Use `ProviderContainer`/`ProviderScope` overrides — never instantiate notifiers directly | +| Releases / version / CI / build tooling | `docs/rules/git-ci-release.md` | Releases are **tag-driven** — never upload an APK manually; don't add manual Sentry handlers in `main.dart` | +| E2E / Playwright | `docs/rules/e2e.md` | Suite is tag-sliced + run manually — remind which `--grep @tag` slice after engine/projection/i18n/sink changes | + +### Adding or changing a rule (keep this section lean) + +- **New rule → the matching `docs/rules/*.md` file, not here.** Add to *Workflow essentials* only if it's truly cross-cutting (applies to every change) *and* short. +- **Add a ⚠️ tripwire only when the rule is surprising or destructive.** Routine rules get a home in the file, not a headline — over-adding headlines is what re-bloats this section. +- **Prune when gated.** Once a rule is enforced by a test/lint/CI gate, shrink it to a pointer at the gate (the gate is the source of truth). +- **Be specific, revise like code.** "Use X", not "prefer X when possible". If an agent gets something wrong twice, that's a missing or weak rule. +- **Keep the index in sync** — every `docs/rules/*.md` file has exactly one Rules Index row. --- diff --git a/docs/plans/2026-06-25-claude-md-key-rules-refactor-design.md b/docs/plans/2026-06-25-claude-md-key-rules-refactor-design.md index 47c5adae..96b7a777 100644 --- a/docs/plans/2026-06-25-claude-md-key-rules-refactor-design.md +++ b/docs/plans/2026-06-25-claude-md-key-rules-refactor-design.md @@ -1,21 +1,27 @@ # Design: Refactor CLAUDE.md "Key Rules" into a tiered rules index **Date:** 2026-06-25 -**Status:** Approved design (not yet implemented) +**Status:** Implemented (CLAUDE.md 42 KB → 18 KB; Key Rules 29.5 KB → 5.4 KB inline) **Scope:** Documentation only — no code changes. +> **Build note (as-built):** the live file had **60** rules, not 62 — the original +> count came from a stale snapshot that included two rules ("Stats breakdown tables" +> and "Driving Playwright locally") not present on `main`. Counts below are corrected +> to 60. `Widget test finders` landed in `statistics.md` (stat-literal collisions), +> so `testing.md` holds one rule. + --- ## Problem -`CLAUDE.md` is 363 lines / ~42 KB. The `## Key Rules` section alone is **62 dense +`CLAUDE.md` is 363 lines / ~42 KB. The `## Key Rules` section alone is **60 dense prose rules, ~29.5 KB — roughly 70% of the whole file** — and all of it loads into *every* session's context. This hurts on four axes at once (all four flagged as goals): - **Token cost** — ~7K tokens of always-on text taxes every conversation. -- **Readability** — 62 unsorted paragraphs are hard to scan; the relevant rule is +- **Readability** — 60 unsorted paragraphs are hard to scan; the relevant rule is hard to find. - **Reliability** — past the ~150–200 instruction ceiling that frontier models reliably follow, rules dilute ("context rot") and adherence quietly drops. @@ -43,9 +49,9 @@ layer. The refactor must not duplicate the invariants already stated above. Considered three options: -- **A. Group in place** — cluster the 62 rules under domain subheads. Readability +- **A. Group in place** — cluster the 60 rules under domain subheads. Readability only; zero token savings; stays over the line ceiling. Rejected. -- **B. Full extraction** — move every rule to `.claude/rules/*.md`, leave only an +- **B. Full extraction** — move every rule to `docs/rules/*.md`, leave only an index. Max token savings, but gotchas become *unknown unknowns* — the agent won't open `cricket.md` unless something cues it. Reliability dip. Rejected. - **C. Tiered (chosen)** — keep a short inline *Workflow essentials* block + a @@ -56,8 +62,10 @@ Considered three options: ### Defaults -- **Location:** `.claude/rules/` (idiomatic agent-rules home; keeps these out of the - published `docs/` tree). Referenced — **not** `@import`-ed — so they load on demand. +- **Location:** `docs/rules/` — a **tracked** home alongside the existing spec docs + the Spec Document Index already points into. (`.claude/rules/` was the first choice + but `.claude/` is gitignored here, so those files would never reach the repo or CI.) + Referenced — **not** `@import`-ed — so they load on demand. - **Granularity:** 10 domain files (below). X01 keeps its own file (flagship game, room to grow) rather than folding into `game-engine.md`. @@ -65,7 +73,7 @@ Considered three options: ## New shape of the `## Key Rules` section -Three parts replace the 62-rule wall: +Three parts replace the 60-rule wall: ### 1. Workflow essentials (stays fully inline, ~11 short lines) @@ -89,16 +97,16 @@ would just cost a round-trip every session: | Before you touch… | Read first | |---|---| -| Cricket scoring / variants / labels | `.claude/rules/cricket.md` | -| X01 scoring / strategy / turn_score | `.claude/rules/x01.md` | -| Game events / rounds / payloads / RNG | `.claude/rules/game-engine.md` | -| Stats / projections / formatters | `.claude/rules/statistics.md` | -| Drift schema / DB / test fixtures | `.claude/rules/database.md` | -| Auto-scorer / camera / capture | `.claude/rules/auto-scorer.md` | -| UI / design tokens / navigation | `.claude/rules/ui-design.md` | -| Notifier / widget tests | `.claude/rules/testing.md` | -| Releases / version / CI / build tooling | `.claude/rules/git-ci-release.md` | -| E2E / Playwright | `.claude/rules/e2e.md` | +| Cricket scoring / variants / labels | `docs/rules/cricket.md` | +| X01 scoring / strategy / turn_score | `docs/rules/x01.md` | +| Game events / rounds / payloads / RNG | `docs/rules/game-engine.md` | +| Stats / projections / formatters | `docs/rules/statistics.md` | +| Drift schema / DB / test fixtures | `docs/rules/database.md` | +| Auto-scorer / camera / capture | `docs/rules/auto-scorer.md` | +| UI / design tokens / navigation | `docs/rules/ui-design.md` | +| Notifier / widget tests | `docs/rules/testing.md` | +| Releases / version / CI / build tooling | `docs/rules/git-ci-release.md` | +| E2E / Playwright | `docs/rules/e2e.md` | ### 3. Tripwire headlines (1–3 per domain, bare one-liners) @@ -116,7 +124,7 @@ opened; the *explanation* is on-demand. Examples: ## Domain-file format -`.claude/rules/.md` — flat list, one `###` heading per rule (greppable), +`docs/rules/.md` — flat list, one `###` heading per rule (greppable), consistent `**Rule:** / **Why:**` body (mirrors the repo's memory-file convention): ```markdown @@ -144,7 +152,7 @@ random/crazy + non-standard → `mode · scoring`. ## Rule → file mapping (lossless move checklist) -All 62 current Key Rules, by destination. (Numbered in current top-to-bottom order.) +All 60 current Key Rules, by destination. (Numbered in current top-to-bottom order.) **Inline — Workflow essentials (11):** GameConfig-edited-in-bottom-sheet stays UI; these are the process rules → Branch naming, PR titles, Squash-merge, PR reviews, @@ -161,7 +169,7 @@ event · Cricket scoring × target mode orthogonal · Cricket variant labels in Per-leg round cap · `DartCorrected` payload key (`original_event_id`) · `endGame()/endDrill()` don't mutate `isComplete`. -**`statistics.md` (7):** Statistics scope resets · Computing stats over an event slice +**`statistics.md` (8):** Statistics scope resets · Computing stats over an event slice (`PlayerStatsAssembler`) · `GameStats.gameType` load-bearing · Statistics loader vs computation · Cricket mark-bucket field overload · Statistics scope required (`gameType`) · Projection snapshots two-level. @@ -175,24 +183,24 @@ Device-session regression fixtures · Capture writes respect opt-in · `DartInpu (submitDart/advanceTurn) · Camera/device-only not widget-testable · Camera-first board layout + tests. -**`ui-design.md` (5):** Game config bottom sheet · Colors · DESIGN_SYSTEM specifics · -Stats breakdown tables reuse `PostGameStatsBreakdown` · Navigation `go()` vs `push()`. +**`ui-design.md` (4):** Game config bottom sheet · Colors · DESIGN_SYSTEM specifics · +Navigation `go()` vs `push()`. -**`testing.md` (2):** Notifier tests · Widget test finders. +**`testing.md` (1):** Notifier tests. *(Widget test finders → `statistics.md`.)* **`git-ci-release.md` (5):** Releases tag-driven · Version bumps · `flutter create` stray `widget_test.dart` · "Unused" in `lib/` may be wiring · Sentry error handlers. -**`e2e.md` (2):** E2E regression reminders · Driving Playwright against local builds. +**`e2e.md` (1):** E2E regression reminders. -Total: 11 inline + 51 in files = **62** (lossless). +Total: 11 inline + 49 in files = **60** (lossless). --- ## Maintenance convention (added to CLAUDE.md so the structure self-perpetuates) - **New rule → domain file, not CLAUDE.md.** Add a `###` entry to the matching - `.claude/rules/*.md`. Add to the inline blocks only if truly cross-cutting (applies + `docs/rules/*.md`. Add to the inline blocks only if truly cross-cutting (applies to every change) *and* short. - **Tripwire only when surprising/destructive.** Routine rules get an index row, not a headline — over-adding headlines is what causes re-bloat. @@ -201,7 +209,7 @@ Total: 11 inline + 51 in files = **62** (lossless). gate is the source of truth. - **Revise like code; be specific.** "Use X" not "prefer X when possible." If an agent gets something wrong twice, that's a missing/weak rule. -- **Index ↔ files stay in sync** — every `.claude/rules/*.md` has exactly one Rules +- **Index ↔ files stay in sync** — every `docs/rules/*.md` has exactly one Rules Index row. --- @@ -209,11 +217,11 @@ Total: 11 inline + 51 in files = **62** (lossless). ## Migration plan 1. Branch `docs/refactor-claude-md-key-rules` off `main`. *(done — this design lives here.)* -2. Create the 10 `.claude/rules/*.md` files, moving each rule's **full current text +2. Create the 10 `docs/rules/*.md` files, moving each rule's **full current text verbatim** (lossless move first — no rewriting). 3. Rewrite the `## Key Rules` section to the new shape (Workflow essentials + Rules Index + tripwires), and add the Maintenance convention block. -4. Verify nothing dropped against the 62-rule mapping table above (reproduce it in the +4. Verify nothing dropped against the 60-rule mapping table above (reproduce it in the PR description with a ✓ per rule). 5. *(Optional second pass)* tighten verbose entries to the `Rule:/Why:` format. 6. Open PR; review via the `code-review:code-review` skill; report before/after diff --git a/docs/rules/auto-scorer.md b/docs/rules/auto-scorer.md new file mode 100644 index 00000000..fed6276e --- /dev/null +++ b/docs/rules/auto-scorer.md @@ -0,0 +1,27 @@ +# Auto-scorer / camera / capture rules + +> Loaded on demand. CLAUDE.md's Rules Index points here before touching the auto-scorer, camera preview, capture pipeline, or camera-first board layout. + +### Auto-scorer camera preview +`CameraPreview` handles device orientation itself (its own internal `AspectRatio` + `RotatedBox`; `controller.value.aspectRatio` is ALWAYS the landscape sensor ratio). Do NOT wrap it in your own `Center > AspectRatio(controller.value.aspectRatio)` — on a portrait screen that shrinks the preview to a letterboxed band (this is what #408 did). Detection runs on `takePicture()`'s raw sensor frame (landscape buffer), **served as-is — the app does not rotate it**. The model needs the board roughly upright, so a portrait-held phone (board sideways in the landscape buffer) detects poorly until the model is trained for that orientation (#393); the app-side fix is **not** rotation (a rotation auto-detect was tried and dropped — it can't tell upright from upside-down). Preview-display orientation and detection-input orientation are independent — don't conflate them. + +### Auto-scorer camera (YOLOView path) +The in-game preview + aim view run a live `YOLOView` with native streaming inference (`onResult` ~3 Hz, `inferenceFrequency: 3`) — NOT a `takePicture`/`_busy` polling loop (`_detectTick`/`_tick`/`_busy` are gone). Frame stills come from `YOLOViewController.capturePhoto(withOverlays: false)`. The auto/fire-and-forget callers (`_captureEmitted`/`_captureCorrected`) run in a `try/catch` (a dropped capture must never disrupt scoring) with no busy guard. The user-triggered manual buttons (`_manualCapture`, the aim view's `_capture`) instead `tapToFocus(0.5,0.5)` + await `kAutoScorerFocusSettle` before the shot (capturePhoto does NOT autofocus, so a manual still is otherwise blurry) and hold a `_capturing` flag that disables the button while in flight (debounces double-taps); that flag does NOT gate the auto callers, which can still interleave. Every manual path also gates on `dataCollectionEnabledProvider` before persisting (like the auto paths). The legacy predict path (`AutoScorerSession.onFrame`/`detectOnly` → `DartDetector`) still exists but isn't the in-game path. + +### Auto-scorer capture sidecar is a probe contract +The `CaptureRecord` JSON shape (`capture_record.dart`) is the `ddp-preprocess` ingest contract. Adding a key = coordinated edit: `toJson` + `fromJson` (with a backward-compat default for old sidecars) + `withCorrection` (preserve it) + the `unorderedEquals` key-set test in `capture_record_test.dart` (fails CI otherwise). Probe-side consumption needs a reciprocal change in the probe repo. + +### Device-session regression fixtures (#488) +`SessionReplayer` rebuilds the tracker from the fixture's EMBEDDED `TrackerSegment` config, so a raw capture replays with its original (possibly buggy) config — its recorded outcomes ARE the bug. To use a capture as a regression test for a fix, hand-edit the embedded config in the fixture to the fixed value and assert `isFaithful == false` (the fixed replay must diverge from the buggy recording) + the corrected invariants on `frame.replayed` — never assert against the recorded outcomes. + +### Capture writes respect the opt-in +Any path that persists a training capture (emission, manual button, or correction-driven) MUST gate on `dataCollectionEnabledProvider` — the store is non-null even when the toggle is off, so an ungated write silently hoards frames the user opted out of. + +### `DartInputSink` carries `submitDart` AND `advanceTurn` +The auto-scorer→game port has two methods. `advanceTurn()` backs the opt-in "auto-advance when board is cleared" feature (`autoAdvanceOnClearEnabledProvider`, default off): the YOLOView preview calls it on a board-clear (`TrackerPhase.rebaselined`) **guarded by `_sawDartsThisTurn`** — `rebaselined` also fires on a transform-only empty board at turn start, so without that guard it skips players who haven't thrown. Each board's sink impl must **no-op when a modal/celebration is pending or the game is complete** (X01's `advanceTurn` dismisses the bust/leg modals, so without the guard an auto-advance would blow past an unacknowledged win; cricket's `nextPlayer` doesn't dismiss them — the guard is the sole safety net) and must bump `activeTurnSignal` like the manual NEXT button. Pure decision lives in `shouldAutoAdvance` (`domain/tracking/auto_advance.dart`); the widget trigger is device-only. The board-clear itself is a **confirm-before-clear** gate: the board must read empty for `DartTrackerConfig.emptyFramesToRebaseline` consecutive *calibrated* frames before `_rebaseline()` fires (confirmed darts are retained meanwhile, so a dart reappearing before the threshold resets the count and re-matches without re-emitting). The default is **9 frames ≈ 3s at `kAutoScorerInferenceHz`=3** — deliberately long so a transient dart-detection flicker or brief darts-only occlusion (~1s) is NOT mistaken for a pull (premature advance + double-counted dart, #499); a genuine pull leaves the board empty for several seconds and still clears. + +### Camera/device-only changes aren't widget-testable +(no `CameraPreview` in `flutter test`, no web camera) — verify on a real device. PR builds upload the debug APK as a 7-day artifact (`build-apk.yml`); download it from the PR's *Build APK* Actions run (or `gh run download `) to sideload without merging. Different signing key than local/release builds → uninstall the existing app first. + +### Camera-first board layout + tests +Boards branch on `cameraFirst = autoScoringOn && cameraPreview != null` — the camera-first column lives behind `if (cameraFirst)` (manual layout in `else`), `GameStatusBarWidget(showDarts: false)` moves the darts to `ProminentDartBandWidget`, and the primary state uses `HeroMetricWidget` / per-game strips (`X01OtherPlayersStripWidget`, `CricketMarksStripWidget`, `PracticePlayersStripWidget`; `PracticeTargetDisplayWidget(heroSize: true)`). Inside `if (cameraFirst)`, Dart promotes `cameraPreview` non-null — call it WITHOUT `!` (else `unnecessary_non_null_assertion`). Unlike the live `CameraPreview`, the camera-first **chrome IS widget-testable**: override `autoScoringEnabledProvider` with a fake whose `build()` returns true AND `boardCameraPreviewBuilderProvider.overrideWithValue((c, id) => const SizedBox(key: ValueKey('camera-stub')))`. Default board tests run in manual mode (neither overridden → `cameraFirst` false). diff --git a/docs/rules/cricket.md b/docs/rules/cricket.md new file mode 100644 index 00000000..1568bc27 --- /dev/null +++ b/docs/rules/cricket.md @@ -0,0 +1,15 @@ +# Cricket rules + +> Loaded on demand. CLAUDE.md's Rules Index points here before any cricket work. + +### Adding a cricket variant +Four coordinated edits — (1) `_cricketVariants()` entry in `variant_selection_page.dart`, (2) a `cricketXxxRules` content block in `rules/content/cricket_rules.dart`, (3) the slug → rules entry in `kGameRules` (`rules_registry.dart`), (4) the slug in `expectedSlugs` in `rules_registry_test.dart` (the registry test fails CI if you miss this). The info-icon shows "Rules unavailable." silently if (3) is missing — only the test enforces coverage. + +### Adding a "right-after-TurnStarted" cricket event +Every site that emits the event (`CreateGameUseCase`, `ProcessCricketDartUseCase`, the three TurnStarted emission sites in `active_cricket_game_provider.dart`) must emit it; AND `UndoLastDartUseCase` must add the event type to BOTH its supersession-collection loop and its replay-skip branch — otherwise a turn-boundary undo replays the cancelled turn's event and corrupts state. Same applies to projections: add to `consumedEventTypes` on every cricket projection (or extend the shared `CricketTargetsTracker` mixin) AND the `legHistoryFromEvents` inline tracker in `PlayerStatsAssembler`. + +### Cricket scoring × target mode are orthogonal axes +`CricketGameConfig` exposes `scoring` ∈ {`standard`, `cut-throat`, `no-score`} and `targetMode` ∈ {`fixed`, `random`, `crazy`}; `GameState` mirrors them as `cricketScoring`/`cricketTargetMode` plus a dynamic `cricketTargets: List` (+ implicit Bull) and `cricketLockedTargets: Set`. The engine reads the target set from state — never from a hardcoded `[15..20]` constant — so any target mode is accepted by the same code path. Legacy configs carrying a single `variant` string deserialise to `{scoring: , targetMode: 'fixed'}` via a `readValue` mapping at JSON read time; **no event migration**, historical replay stays correct. Stats loader buckets cricket games by `targetMode` (today only `fixed` is wired; `random`/`crazy` cohorts arrive with PRs #237/#238). See `docs/plans/2026-05-19-cricket-target-modes-design.md`. + +### Cricket variant labels live in three places and must stay aligned +`variant_selection_page.dart` (picker — Title Case), `cricket_board_page.dart` (in-game header — Title Case), `game_summary_card_widget.dart` (history list — lowercase by design). When you change one site's `scoring × targetMode` label formula, audit the other two. The shared rule: fixed → `scoring` alone; random/crazy + standard → just the mode; random/crazy + non-standard → `mode · scoring`. diff --git a/docs/rules/database.md b/docs/rules/database.md new file mode 100644 index 00000000..b1f1c4b5 --- /dev/null +++ b/docs/rules/database.md @@ -0,0 +1,27 @@ +# Database, drift & repository rules + +> Loaded on demand. CLAUDE.md's Rules Index points here before touching the drift schema, DB, repositories, or test fixtures. + +### Repository exceptions +All exceptions extend `RepositoryException` (`lib/core/error/repository_exception.dart`). Never throw raw `Exception` from a repository implementation. + +### Contract tests +Every repository implementation must pass the shared contract tests in `test/contracts/`. Never skip or comment out tests to make CI pass. + +### Database +drift on every platform (`NativeDatabase.createInBackground` on mobile/desktop, `WasmDatabase` over IndexedDB on web). Schema lives in `lib/core/persistence/drift/database.dart`; `databaseVersion = 2` (v1→v2 added `unlocked_achievements`, #522 — the first migration, applied via `MigrationStrategy.onUpgrade` `m.createTable`). `PRAGMA foreign_keys = ON` is set in `MigrationStrategy.beforeOpen`, which runs on every connection (including immediately after `onUpgrade`), so migrations don't need to toggle it themselves — and adding a brand-new table via `m.createTable` needs no FK-disable dance (CREATE TABLE never triggers FK checks). Completed games are read-only — enforced in application logic, not triggers. The canonical SQL DDL reference is `docs/DATABASE_DDL.md`. After editing drift table classes, run `dart run build_runner build`. **After any schema change, also bump `databaseVersion`, add an `onUpgrade` step, and regenerate the schema snapshots: `dart run drift_dev schema dump lib/core/persistence/drift/database.dart drift_schemas/` then `dart run drift_dev schema generate drift_schemas/ test/drift_schemas/` (committed; `test/core/persistence/drift/migration_test.dart` verifies upgrades with drift's `SchemaVerifier`).** + +### Drift foreign keys +Plain `text()()` emits NO foreign key clause — you must call `.references(Type, #col, onDelete: KeyAction.{cascade|restrict|setNull})` explicitly. `PRAGMA foreign_keys = ON` is a no-op without `.references()`. When two columns in a table reference the same parent (e.g. `game_sessions.host_player_id` and `current_turn_player_id` both → `players`), add `@ReferenceName('xxx')` annotations to disambiguate manager-API helpers, or build_runner warns. + +### Test database setup +Drift tests use `AppDatabase(NativeDatabase.memory())` directly — see `test/drift_test_base.dart`. With FK enforcement active, fixtures must respect FK order: insert players before competitors, games before competitors/dart_throws/game_events, and use `playerRepo.createPlayer()` to seed referenced player IDs before any `createGame` call. + +### Test game setup ordering +Drift enforces read-only on completed games. In tests: create game with `isComplete: false` → insert darts/events → call `gameRepo.completeGame()`. Never set `isComplete: true` at creation if you need to insert data afterward. + +### Watchable queries +drift's per-query reactivity is automatic — `select(...).watch()` re-fires whenever drift sees a write to one of the referenced tables. No notify-after-write protocol to remember. + +### Repository contract tests +`runHybridTests` (`test/hybrid_test_runner.dart`) spins up a fresh in-memory `AppDatabase` per test against the shared `*_contract.dart` suites. The "hybrid" name is vestigial from the dual-backend era (issue #112) — there is now a single backend. diff --git a/docs/rules/e2e.md b/docs/rules/e2e.md new file mode 100644 index 00000000..ba0b395f --- /dev/null +++ b/docs/rules/e2e.md @@ -0,0 +1,6 @@ +# E2E / Playwright rules + +> Loaded on demand. CLAUDE.md's Rules Index points here before touching E2E specs or the Playwright suite. + +### E2E regression reminders +The committed Playwright suite (`e2e/*.spec.ts`) is tag-sliced and **run manually by choice** (no CI gate — though it does run green headless via Playwright's bundled Chromium / software GL; gating is a future option, not a blocker). After changing a game engine, scoring/stats projection, correction/undo flow, localized strings, or the auto-scorer sink, consult the coverage map in `docs/E2E_REGRESSION.md` and remind the user which `npx playwright test --grep @tag` slice to run before merging. Never assume the suite ran. When adding a game/feature, add a tagged spec (and a coverage-map row) in the same PR. diff --git a/docs/rules/game-engine.md b/docs/rules/game-engine.md new file mode 100644 index 00000000..69f2f97f --- /dev/null +++ b/docs/rules/game-engine.md @@ -0,0 +1,27 @@ +# Game engine rules (events, rounds, payloads, RNG) + +> Loaded on demand. CLAUDE.md's Rules Index points here before touching game events, rounds, dart payloads, RNG, or game lifecycle. + +### GameConfig dispatch +Use `maybeMap` (not `maybeWhen`) — callbacks receive typed subclass instances: `config.maybeMap(x01: (c) => c.startingScore, orElse: () => '')`. Requires explicit `import 'game_config.dart'`; not available via transitive import. + +### DartThrown payload keys +`competitor_id`, `player_id`, `segment`, `multiplier`, `score`, `input_method`, plus optional `x`/`y` (see `buildDartThrownEvent` in `lib/features/game/domain/usecases/game_use_case_helpers.dart`). `x`/`y` are the auto-scorer's normalised canonical impact position (#571, canonical scoring frame: origin = centre, radius 1.0 at the double ring, **5/20 calibration wire at top** so segment 20's centre is ~9° clockwise of vertical; the heatmap widget rotates only the *display* via `kHeatmapDisplayRotation` to show a standard "20 at top" board, #697 — stored values stay in the wire-at-top frame) — **omitted when null** (manual entry, noise, pre-#571 events) so old events parse unchanged. The same values are written to `dart_throws.x`/`.y`. The use cases read them off the `DartThrow` they already receive (the notifier sets them); a correction/undo deletes the dart and re-entry is manual → position stays null by design. No `turn_number`, no `dart_number` — reconstruct turn grouping via `TurnStarted`/`TurnEnded` event boundaries if needed. + +### `local_sequence` is per-game, not global +Every new game restarts `local_sequence` at 1, so multiple games' events share the same sequence range. Any query that loads events across multiple games MUST sort by `(game_id, local_sequence)` — sorting by `local_sequence` alone interleaves games and corrupts projection state across game boundaries. `ProjectionRunner.run()` enforces this internally; SQL queries feeding it should match. + +### RNG in use cases +Event-emitting use cases that need randomness (e.g. `CreateGameUseCase`, `ProcessCricketDartUseCase` for `CrazyTargetsRolled`) accept an optional `math.Random?` constructor parameter that defaults to `math.Random()` in production. Tests inject a seeded `math.Random(seed)` for determinism. RNG runs **once at emission**, the value lands in the event payload, and `engine.apply()` is pure — replay never re-rolls. + +### Round semantics +A "round" is one full rotation where ALL competitors throw. `totalRounds` is the correct field name. Do not use `maxRounds` or count per-competitor turns or individual dart throws as rounds. + +### Per-leg round cap +X01 and Cricket enforce a round cap per leg (see `GameConfigurationConstants` and engine logic). When the cap is hit with no winner, the leg is decided by current standing — do not extend rounds silently. Both engines and any UI showing round progress must respect this. + +### `DartCorrected` payload key is `original_event_id` +A string referencing the corrected `DartThrown.eventId`. Any replay-aware code path (`UndoLastDartUseCase`, `PlayerStatsAssembler.fromEvents`) must collect these and skip the originals. + +### `endGame()` / `endDrill()` write `is_complete=true` to the DB but do NOT mutate `state.value.gameState.isComplete` +The post-game-navigation listener (`practice_board_page` / `x01_board_page` etc.) watches that flag and would otherwise route every menu-driven exit through post-game instead of home. When you need an authoritative "is this game complete?" signal outside the active-game provider (e.g. from the router's `onExit`), read it from `GameRepository.getGame(id)`, not the notifier state. See `app_router.dart`'s `_gameIsComplete` helper for the pattern. diff --git a/docs/rules/git-ci-release.md b/docs/rules/git-ci-release.md new file mode 100644 index 00000000..fc4fb42c --- /dev/null +++ b/docs/rules/git-ci-release.md @@ -0,0 +1,25 @@ +# Git, CI, release & build-tooling rules + +> Loaded on demand. CLAUDE.md's Rules Index points here for release/version/CI/build-tooling detail. +> The everyday workflow rules (branch-first, PR titles, squash-merge, stage explicit paths, commit generated files, analyze-before-push) are inline in CLAUDE.md's *Workflow essentials* — this file holds the longer-form detail and the rarer gotchas. + +### PR reviews (full pipeline) +Every PR — including small or "obvious" ones — gets reviewed via the `code-review:code-review` skill before merge. Self-review via `gh pr diff` is not sufficient. The skill runs an 8-step pipeline (eligibility → CLAUDE.md fetch → summary → 5 parallel Sonnet reviews covering CLAUDE.md compliance / bugs / git history / prior PR comments / in-code comments → confidence scoring at 0/25/50/75/100 → filter ≥80 → post). Issues that score below 80 should still be fixed by the author if real (just not posted as inline comments). CI must be green before merging. + +### Analyze in CI (full detail) +`test.yml` runs `flutter analyze --no-fatal-infos`. Warnings block CI; infos are advisory. ~190 info-level lints are tolerated (deprecated `overrideWith`, `curly_braces_in_flow_control_structures`, `avoid_print` in test infra). Cleaning them is optional polish — never tighten this flag without raising it. **Always run project-wide `flutter analyze --no-fatal-infos` before pushing — `flutter analyze ` may not surface unused-import / unused-variable warnings that the project-wide variant catches.** For a fast pre-push check that filters out the info noise, grep: `flutter analyze --no-fatal-infos 2>&1 | grep -E '^\s*(warning|error) •'` — empty output means CI-clean. Run this **after** your last file change, not just once early in the session: warnings introduced by later test/import edits will otherwise slip through. + +### Releases are tag-driven +Pushing a tag `vX.Y.Z` (or `vX.Y.Z-rcN` for pre-release) triggers `release.yml`, which builds and publishes the signed APK to GitHub Releases. Every merge to `main` also auto-tags `v-rc` (N = next-available rc number) via `auto-rc.yml` and publishes a pre-release — devs do not push RC tags manually. Never manually upload an APK to a release. Tags must point to a commit that's reachable from `main` (`release.yml` enforces this). Full process in `docs/RELEASES.md`. + +### Version bumps +When asked to bump the version, edit only `pubspec.yaml`'s `version:` field (e.g. `1.0.0+0` → `1.1.0+0`) in a `chore: bump version to X.Y.Z` PR. The `+N` suffix is a placeholder; CI overrides `versionCode` from `github.run_number` on tag builds. + +### `flutter create` drops a stray `test/widget_test.dart` +Scaffolding `android/`/`web/`/`ios/` per machine re-creates a default `test/widget_test.dart` that references a non-existent `MyApp`. It's untracked so CI never sees it, but it fails local `flutter test` and trips `flutter analyze` with a phantom `MyApp isn't a class` error — don't chase it as a real regression. `rm test/widget_test.dart` after scaffolding. + +### "Unused" in `lib/` may be forgotten wiring +When `flutter analyze` flags an unused field, parameter, or import in `lib/`, check whether it represents incomplete wiring (a setter that updates a field nothing reads, a constructor param never used in the body) before deleting. If unsure, ask — silent deletion can lock in a no-op user-facing control as the intended behavior. + +### Sentry error handlers +`SentryFlutter.init` auto-installs `FlutterError.onError` and `PlatformDispatcher.instance.onError` via `FlutterErrorIntegration` and `OnErrorIntegration` (sentry_flutter ≥ ~7.x; current pin `^9.16.1`). Do NOT add manual handlers in `main.dart` — they would override Sentry's wiring and silence the crash pipeline. See the `lib/main.dart` header comment. **Crash reporting is opt-out (default on):** `main.dart` reads `kCrashReportingPrefKey` from SharedPreferences *before* `SentryFlutter.init` and skips init entirely when disabled (so the handlers are installed only when enabled), surfacing the toggle via the `CrashReportingEnabled` provider in Settings → Feedback. A clean runtime re-init isn't supported and native crashes bypass `beforeSend`, so the toggle takes effect on the next launch — do NOT "fix" the conditional wrapper into an unconditional call. The `Report a Bug` action gates on `Sentry.isEnabled` (a safe `NoOpHub` no-op when off) so feedback is never silently dropped. diff --git a/docs/rules/statistics.md b/docs/rules/statistics.md new file mode 100644 index 00000000..81780562 --- /dev/null +++ b/docs/rules/statistics.md @@ -0,0 +1,30 @@ +# Statistics & projections rules + +> Loaded on demand. CLAUDE.md's Rules Index points here before touching stats, projections, or formatters. + +### Statistics scope resets +Turn resets on `TurnStarted`, Leg resets on `LegCompleted`, Match resets on `GameCompleted`. No other reset points. + +### Computing stats over an event slice +All projection wiring lives in `PlayerStatsAssembler` (`lib/features/statistics/domain/assemblers/`). Use the method that matches the scope: `fromEvents` (career), `gameStatsFromEvents` (per-game), `playerStatsForGameFromEvents` (per-player-per-game), `legCompetitorStatsFromEvents` (per-leg). Repos and use cases load events and delegate. If you need a new projection bundle, add it to the assembler — do not re-wire `ProjectionRunner` directly in repos or use cases. Snapshot keys: `x01_average`, `x01_checkout`, `x01_highest_checkout`, `x01.highScoreBuckets`, `cricket.mpt`, `cricket.markBuckets`, `cricket.firstNineMpr`. First-nine projections (`cricket.firstNineMpr`, X01 first-nine PPR) only count when `TurnStarted` events are present — fixtures emitting just `DartThrown`/`TurnEnded` silently produce null first-nine stats. + +### `GameStats.gameType` is load-bearing +The post-game summary branches on `gameStats.gameType == GameType.cricket.name` to choose MPR vs PPR labels and rows. Every return path of `getGameStats` (including the empty-darts early return) must set it, in both repository implementations. + +### Statistics loader vs computation +Statistics computation lives in `PlayerStatsAssembler` (shared). The loader queries live in `lib/core/persistence/drift/repositories/statistics_repository_drift.dart` — load events + dart_throws, then delegate to the assembler. When changing how stats are computed, update only the assembler. + +### Cricket mark-bucket field overload +`CompetitorStats.{five..nine}MarkTurns` are populated as **exact-N** counts by `getGameStats` and `ComputeLegStatsUseCase` (read from the `*Exact` snapshot keys) but as **≥-N** counts by `getPlayerStats` (read from the `*MarkTurns` keys). Same field, different cohorts by call path. + +### Statistics scope is required +`getPlayerStats` and `watchPlayerStats` take `required GameType gameType`. PPR-shaped fields are X01-only and cricket fields are cricket-only — a single call cannot mix types coherently. The player-picker AVG badge consumes `playerStatsProvider`, which passes `GameType.x01`. + +### Projection snapshots are two-level +Top level keyed by `engine.descriptor.id` (e.g. `'x01.doubleOut'`), inner map keyed by field name (e.g. `'doubleOutSuccessRate'`). Wiring a new engine into `PlayerStatsAssembler.fromEvents` means reading at both levels — running an engine without reading its snapshot is a silent no-op. + +### Number formatting +Use `StatFormatter` (`lib/core/utils/stat_formatter.dart`) for all statistics display — `fmtDouble`, `fmtPct`, `fmtPerLeg`. Never use inline `toStringAsFixed()` in statistics UI. `test.yml`'s "Stats UI formatter gate" greps `lib/features/*/presentation/` for `toStringAsFixed` and fails CI on ANY match (not just stats) — route every number through `StatFormatter` (it wraps `toStringAsFixed` in `core/utils`, outside the gate), even non-stat labels like a `2.5×` zoom readout. + +### Widget test finders (stat literals) +`StatFormatter.fmtDouble` strips trailing zeros — e.g. `170.0` → `'170'`, which collides with raw `int` values rendered the same way. Prefer `findsNWidgets(n)` or more specific finders (`find.descendant`) over `findsOneWidget` when stat rows may share literals. diff --git a/docs/rules/testing.md b/docs/rules/testing.md new file mode 100644 index 00000000..292e3c35 --- /dev/null +++ b/docs/rules/testing.md @@ -0,0 +1,7 @@ +# Testing rules + +> Loaded on demand. CLAUDE.md's Rules Index points here before writing notifier or widget tests. +> Repository contract-test rules live in `database.md`; stat-literal finder pitfalls live in `statistics.md`. + +### Notifier tests +Use `ProviderContainer` with `overrides`. Never instantiate notifiers directly. Use `ProviderScope` with `overrides` for widget tests. diff --git a/docs/rules/ui-design.md b/docs/rules/ui-design.md new file mode 100644 index 00000000..a0736503 --- /dev/null +++ b/docs/rules/ui-design.md @@ -0,0 +1,15 @@ +# UI, design tokens & navigation rules + +> Loaded on demand. CLAUDE.md's Rules Index points here before touching UI, design tokens, game-config UI, or navigation. + +### Game config is edited in a bottom sheet, not a page +X01/Cricket in/out strategy, legs-to-win, and starting score are set via the config-summary chip → `GameConfigPanel` bottom sheet on the player-selection screen (`APPLY` commits). `GameConfigPage` exists but is unwired (and there is no "Custom" variant tile — it was removed pre-1.0, #684). The `LEGS TO WIN` `ConfigStepperWidget` +/- icon buttons carry localized `semanticLabel`s (`setupLegsIncrement`/`setupLegsDecrement`, #666 fixed); the X01 config-summary chip surfaces `legsToWin` as a localized `setupLegsCount` segment, matching Cricket (#667 fixed). (E2E driving notes: `docs/E2E_REGRESSION.md` § Authoring specs.) + +### Colors +Always use themed color tokens from `docs/design/DESIGN_SYSTEM.md`. Never hardcode color values directly in widgets. + +### DESIGN_SYSTEM specifics the review repeatedly catches +inactive/opponent score numerals use `cs.onSurfaceVariant` (active = `onSurface`, practice target = `primary`); `label-sm` over-line above a hero numeral uses `primaryFixed` (not `onSurfaceVariant`); game-board player names = `labelMedium` ALL-CAPS `letterSpacing: 1.2`; score numerals never scale/wrap — constrain the container, never wrap a score in `FittedBox(scaleDown)`. + +### Navigation — `context.go()` vs `context.push()` +`context.go()` REPLACES the entire route stack — Android's physical back button then has nothing to pop and exits the app. Use `context.push()` for any forward navigation that should be back-poppable (Home → Stats/History/Players/Settings, list → detail, etc.). Reserve `context.go()` for intentional stack resets: game completion → home, post-deletion redirects, deep-link landing pages. If a screen MUST be reached via `go()` (e.g. the variant selection flow), wrap its body in `PopScope(canPop: false, onPopInvokedWithResult: (didPop, _) { if (!didPop) context.go(GameRoutes.home); })` like `variant_selection_page.dart` does, so the Android back button still works. diff --git a/docs/rules/x01.md b/docs/rules/x01.md new file mode 100644 index 00000000..4e7a34d8 --- /dev/null +++ b/docs/rules/x01.md @@ -0,0 +1,9 @@ +# X01 rules + +> Loaded on demand. CLAUDE.md's Rules Index points here before any X01 work. + +### X01 strategy values are lowercase short forms +`'straight'` / `'double'` / `'master'` for both `inStrategy` and `outStrategy` (`GameState` defaults). Engines and projections compare against these literals; UI labels (e.g. "Double Out") live in a display mapper only. Never store the friendly labels. + +### X01 `TurnEnded` payload carries `turn_score` +`turn_score` (`turn_start_score - turn_end_score` per `docs/statistics/x01.projections.md` §5.2). `ProcessDartUseCase` computes it; `buildTurnEndedEvent` accepts an optional `turnScore` int. `X01AverageProjection` prefers this delta over per-dart sum (so bust + Double-In not-in turns contribute 0 to PPR), and falls back to dart-sum when absent for backward compatibility with pre-#318 events. Any new X01 dart-emission path MUST pass `turnScore` or new games will use the legacy convention. From 18c1589a96b1a1f9cd888e5a324467f4fafd3952 Mon Sep 17 00:00:00 2001 From: mathrb Date: Thu, 25 Jun 2026 13:20:39 +0000 Subject: [PATCH 3/3] docs: address code-review findings on Key Rules refactor - Fix dangling "see Sentry note below" cross-reference in the Technology Decisions table (the Sentry rule moved to docs/rules/git-ci-release.md). - Restore the exact `git checkout pubspec.lock .flutter-plugins-dependencies` command + "regenerate on every pub get/run/build_runner" rationale that the inline condensation had dropped. - Add a GameConfig-dispatch tripwire (`maybeMap` not `maybeWhen`) and widen the game-engine index label so the rule is discoverable. - Correct the design-doc rule-count mapping (statistics.md 9, git-ci-release.md 7; clarify the 3 double-placed rules; 60 distinct rules, lossless). Co-Authored-By: Claude Opus 4.8 --- CLAUDE.md | 6 +++--- ...-06-25-claude-md-key-rules-refactor-design.md | 16 +++++++++++----- 2 files changed, 14 insertions(+), 8 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index c192239f..842ec806 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -146,7 +146,7 @@ All state classes use `freezed`. Never mutate state in place. Always use `copyWi | Navigation | `go_router` | | UUID generation | `uuid` | | Code generation runner | `build_runner` | -| Crash reporting | `sentry_flutter` (conditionally initialized in `lib/main.dart` — opt-out, see Sentry note below; do not remove the `SentryFlutter.init` call) | +| Crash reporting | `sentry_flutter` (conditionally initialized in `lib/main.dart` — opt-out, see the Sentry rule in `docs/rules/git-ci-release.md`; do not remove the `SentryFlutter.init` call) | Platform selection (native SQLite vs WASM) happens once in the Drift factory. Everywhere else sees only the repository interface. @@ -209,7 +209,7 @@ Used in `dart_throws.segment`, `DartThrown` event payloads, and all engine logic - **CI does not run `build_runner`.** Generated `.g.dart` / `.freezed.dart` / `.mocks.dart` are committed — after editing any `@freezed` / `@riverpod` / `@GenerateMocks`, regenerate locally and commit in the same PR. - **Run `flutter analyze --no-fatal-infos` as the LAST step before push** (project-wide, not per-path). Quick CI-clean check: `flutter analyze --no-fatal-infos 2>&1 | grep -E '^\s*(warning|error) •'` → empty = clean. Detail → `docs/rules/git-ci-release.md`. - **Stage explicit paths — never `git add -A` / `git add .`.** The tree carries many untracked scratch files (`*.png`/`*.jpg`/`*.yml` at the repo root, `e2e/node_modules`, `.playwright-*`); a blanket add stages junk. -- **Never commit `pubspec.lock` / `.flutter-plugins-dependencies`** unless the dep set actually changed (`git checkout` them before staging). +- **Never commit `pubspec.lock` / `.flutter-plugins-dependencies`** unless the dep set actually changed — they regenerate on every `flutter pub get` / `flutter run` / `build_runner build` and commonly show `M`. Run `git checkout pubspec.lock .flutter-plugins-dependencies` before staging to keep PR diffs clean. - **Route every displayed number through `StatFormatter`** (`lib/core/utils/stat_formatter.dart`) — `test.yml` greps `lib/features/*/presentation/` for `toStringAsFixed` and fails CI on ANY match. Detail → `docs/rules/statistics.md`. - **Spec edits touch only the spec** — never code, unless explicitly asked. - **After any UI refactor, update the test expectations in the same session** before committing. @@ -220,7 +220,7 @@ Used in `dart_throws.segment`, `DartThrown` event payloads, and all engine logic |---|---|---| | Cricket scoring / variants / labels | `docs/rules/cricket.md` | Variant labels live in **3 aligned places**; adding a variant = **4 edits incl. a registry test**; scoring × targetMode are orthogonal — **never hardcode `[15..20]`** | | X01 scoring / strategy / turn_score | `docs/rules/x01.md` | Strategy values are lowercase (`'straight'`/`'double'`/`'master'`); `TurnEnded` must carry `turn_score` | -| Game events / rounds / payloads / RNG | `docs/rules/game-engine.md` | `local_sequence` is **per-game** — sort by `(game_id, local_sequence)`; a "round" = full rotation (`totalRounds`); `DartCorrected` key is `original_event_id`; `endGame/endDrill` don't mutate `gameState.isComplete` | +| Game events / config dispatch / rounds / payloads / RNG | `docs/rules/game-engine.md` | `GameConfig` dispatch uses `maybeMap`, **not** `maybeWhen`; `local_sequence` is **per-game** — sort by `(game_id, local_sequence)`; a "round" = full rotation (`totalRounds`); `DartCorrected` key is `original_event_id`; `endGame/endDrill` don't mutate `gameState.isComplete` | | Stats / projections / formatters | `docs/rules/statistics.md` | All projection wiring lives in `PlayerStatsAssembler`; `GameStats.gameType` is load-bearing; snapshots are two-level | | Drift schema / DB / test fixtures | `docs/rules/database.md` | **Completed games are read-only** — create incomplete → insert → `completeGame()`; after any schema change bump `databaseVersion` + `onUpgrade` + regen snapshots; FKs need explicit `.references()` | | Auto-scorer / camera / capture | `docs/rules/auto-scorer.md` | Don't wrap `CameraPreview` in your own `AspectRatio`; **every capture write gates on `dataCollectionEnabledProvider`**; camera changes aren't widget-testable | diff --git a/docs/plans/2026-06-25-claude-md-key-rules-refactor-design.md b/docs/plans/2026-06-25-claude-md-key-rules-refactor-design.md index 96b7a777..4dcb2735 100644 --- a/docs/plans/2026-06-25-claude-md-key-rules-refactor-design.md +++ b/docs/plans/2026-06-25-claude-md-key-rules-refactor-design.md @@ -169,10 +169,11 @@ event · Cricket scoring × target mode orthogonal · Cricket variant labels in Per-leg round cap · `DartCorrected` payload key (`original_event_id`) · `endGame()/endDrill()` don't mutate `isComplete`. -**`statistics.md` (8):** Statistics scope resets · Computing stats over an event slice +**`statistics.md` (9):** Statistics scope resets · Computing stats over an event slice (`PlayerStatsAssembler`) · `GameStats.gameType` load-bearing · Statistics loader vs computation · Cricket mark-bucket field overload · Statistics scope required -(`gameType`) · Projection snapshots two-level. +(`gameType`) · Projection snapshots two-level · Widget test finders · Number +formatting (also a short inline pointer in Workflow essentials). **`database.md` (8):** Repository exceptions · Contract tests · Database (versions/migrations) · Drift foreign keys · Test database setup · Test game setup @@ -188,12 +189,17 @@ Navigation `go()` vs `push()`. **`testing.md` (1):** Notifier tests. *(Widget test finders → `statistics.md`.)* -**`git-ci-release.md` (5):** Releases tag-driven · Version bumps · `flutter create` -stray `widget_test.dart` · "Unused" in `lib/` may be wiring · Sentry error handlers. +**`git-ci-release.md` (7):** Releases tag-driven · Version bumps · `flutter create` +stray `widget_test.dart` · "Unused" in `lib/` may be wiring · Sentry error handlers · +PR reviews (full pipeline) · Analyze in CI (full detail) — the last two also have +short inline pointers in Workflow essentials. **`e2e.md` (1):** E2E regression reminders. -Total: 11 inline + 49 in files = **60** (lossless). +Total: **60 distinct rules** (lossless) = 8 inline-only + 49 file-only + 3 +double-placed. The 3 double-placed (Number formatting, PR reviews, Analyze in CI) +each have a short inline pointer in Workflow essentials *and* a full entry in their +domain file, so file entries sum to 52 (49 + 3). ---