docs(skills): add read/write-profiler (dual-walk) discipline to the add-engine-variant walker checklist - #5086
Conversation
…r checklist step PR-6.75 adds a third compiler-exhaustive classification surface — game/ability_rw.rs, the CR 603.3b ordering-conflict profiler. Unlike the walker's fail-closed axes, an elided read here is fail-OPEN (a missed conflict silently auto-orders an order-dependent trigger group), so new variants/fields must be classified in BOTH walks. STAGED pre-merge: do not push/PR until PR-6.75 lands on main; re-verify the game::triggers consumer clause against the merged commit-2 wiring. Assisted-by: ClaudeCode:claude-fable-5
There was a problem hiding this comment.
Code Review
This pull request updates the engine variant addition guide (SKILL.md) to document the read/write conflict profiler (ability_rw.rs) as a third discipline when adding new enum variants or fields. The review feedback suggests expanding the field-addition classification guidance to cover cases where either the walker or the profiler classifies a variant precisely, ensuring both are updated to maintain soundness.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
|
||
| The same discipline applies to the walker's **resolution-time choice classifier** (`effect_resolution_choice_freedom` / `ability_resolution_choice_freedom` in `crates/engine/src/game/ability_scan.rs`, consumed by `analysis::resource::loop_states_cover_modulo_growth` item 6): a NEW `Effect` variant fails to compile there until classified. The default classification is `MayPrompt` (fail-closed — an unproven claim only costs a false-negative cover rejection). Classifying a variant as choice-free (`FreeUnlessLifeReplacements`, or any future `Free`-class verdict) is a SOUNDNESS claim — "resolving can never enter a non-priority `WaitingFor`, for ANY state" — and requires (a) a resolver trace cited in the arm's comment (file:line of the handler in `game/effects/` proving no `WaitingFor` raise on any path, including replacement-pipeline calls such as `replace_event`'s `NeedsChoice`), (b) destructuring the arm without `..` so a future field forces re-audit, and (c) updating the pinned guard test (`resolution_choice_verdicts_are_exactly_pinned`). A new field on a `MayPrompt`-classified variant needs no action (`{ .. }` arms are already fail-closed). | ||
|
|
||
| The same discipline applies a THIRD time to the **read/write conflict profiler** (`crates/engine/src/game/ability_rw.rs` — `ability_rw_profile` / `trigger_condition_rw_profile`, consumed by `game::triggers` legacy-path ordering, CR 603.3b): a NEW variant on any traversed enum fails to compile there until classified (exhaustive, no `_` fallback), and a NEW read/write-bearing field on an existing variant is caught only where the arm destructures without `..` — precise arms bind ALL payload fields (the module's binding mandate), but maximal-conservative arms keep `{ .. }`. If you add a read- or write-bearing field to a variant the profiler classifies precisely, classify it in BOTH `ability_scan.rs` (per-axis) AND `ability_rw.rs` (kind+scope read/write profile). An elided read in the profiler is fail-OPEN for the same-event ordering gate (a missed conflict auto-orders an order-dependent group — CR 603.3b unsoundness), so this is a soundness obligation, not bookkeeping. |
There was a problem hiding this comment.
[MEDIUM] Expand field-addition classification guidance to cover precision in either walker.\n\nWhy it matters: If a new read/write-bearing field is added to a variant that is classified precisely in ability_scan.rs but conservatively in ability_rw.rs, the compiler will only force an update in ability_scan.rs, potentially leading to a missed precision opportunity in ability_rw.rs (or vice versa).\n\nSuggested fix: Change 'the profiler classifies precisely' to 'either the walker or the profiler classifies precisely'.
| The same discipline applies a THIRD time to the **read/write conflict profiler** (`crates/engine/src/game/ability_rw.rs` — `ability_rw_profile` / `trigger_condition_rw_profile`, consumed by `game::triggers` legacy-path ordering, CR 603.3b): a NEW variant on any traversed enum fails to compile there until classified (exhaustive, no `_` fallback), and a NEW read/write-bearing field on an existing variant is caught only where the arm destructures without `..` — precise arms bind ALL payload fields (the module's binding mandate), but maximal-conservative arms keep `{ .. }`. If you add a read- or write-bearing field to a variant the profiler classifies precisely, classify it in BOTH `ability_scan.rs` (per-axis) AND `ability_rw.rs` (kind+scope read/write profile). An elided read in the profiler is fail-OPEN for the same-event ordering gate (a missed conflict auto-orders an order-dependent group — CR 603.3b unsoundness), so this is a soundness obligation, not bookkeeping. | |
| The same discipline applies a THIRD time to the **read/write conflict profiler** (crates/engine/src/game/ability_rw.rs — ability_rw_profile / trigger_condition_rw_profile, consumed by game::triggers legacy-path ordering, CR 603.3b): a NEW variant on any traversed enum fails to compile there until classified (exhaustive, no '_' fallback), and a NEW read/write-bearing field on an existing variant is caught only where the arm destructures without '..' — precise arms bind ALL payload fields (the module's binding mandate), but maximal-conservative arms keep { .. }. If you add a read- or write-bearing field to a variant that *either* the walker or the profiler classifies precisely, classify it in BOTH ability_scan.rs (per-axis) AND ability_rw.rs (kind+scope read/write profile). An elided read in the profiler is fail-OPEN for the same-event ordering gate (a missed conflict auto-orders an order-dependent group — CR 603.3b unsoundness), so this is a soundness obligation, not bookkeeping. |
matthewevans
left a comment
There was a problem hiding this comment.
This PR changes .claude/skills/add-engine-variant/SKILL.md, which is an agent/skill instruction file. The automated PR sweep treats changes under .claude/skills/** as direct-maintainer-review only because they affect future maintainer/agent behavior, so I’m not approving, enqueueing, or accepting this from the sweep.
Please leave this for Matt/direct maintainer handling. No judgment on the content here; this is a scope/authority gate for instruction-file changes.
|
🤖 AI text below 🤖 Dependency cleared: #5072 (PR-6.75) merged as 🤖 Generated with Claude Code |
…#5072 R4/R5) (#5100) The #5086 dual-walk paragraph predates #5072's review rounds 4-5. Adds the three decision-bearing boolean axes (reads_member_bound, reads_event_live, writes_event_object) that drive profiles_conflict's same-event discriminators, and the post-classification full-DB sweep step with the predicate-class vs genuine-exact-set distinction (auto-absorb vs completeness-asserted explicit add). Symbols grep-verified against main at 8f47155. Assisted-by: ClaudeCode:claude-fable-5
🤖 AI text below 🤖
Summary
Documents the read/write-profiler (dual-walk) discipline in the
add-engine-variantskill's walker-checklist step: every new enum variant/field on walker-traversed ability/effect types must be explicitly classified per-axis inability_rw.rs's read/write walks (no_bindings, no wildcard arms), with the fail-closed rationale (CR 603.3b: an unclassified read/write span silently under-prompts trigger ordering).Depends on #5072 (PR-6.75), which introduces
ability_rw_profile/trigger_condition_rw_profileand the dual-walk itself — merge this after it. Docs-only companion in the #4905 pattern (skill-file changes ship separately from feature PRs).Files changed
.claude/skills/add-engine-variant/SKILL.mdCR references
CR 603.3b (context in the documented rationale; no engine code touched).
Track
Developer
LLM
Model: claude-opus-4-8
Thinking: high
Tier: Frontier
Verification
Docs-only change — no engine/parser code touched:
ability_rw_profile×34 uses,trigger_condition_rw_profile×5, both consumed intriggers.rs).cargo fmt --all— no-op (no Rust files).Scope Expansion
None.
Validation Failures
None.
CI Failures
None.
Related: #5072 (PR-6.75 series), #4905 (prior skill-file companion PR precedent).
🤖 Generated with Claude Code