ci(engine): gate against costs on carrier-nested ability definitions - #7114
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThe change adds a recursive census script for cost-bearing ability definitions under inline carriers. CI runs the census against generated card data and fails on invalid, incomplete, or violating data. ChangesCost carrier census
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CIWorkflow
participant CensusScript
participant GeneratedCardData
CIWorkflow->>CensusScript: Run --check
CensusScript->>GeneratedCardData: Read generated card data
GeneratedCardData-->>CensusScript: Return card definitions
CensusScript-->>CIWorkflow: Return scan status and counts
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
matthewevans
force-pushed
the
cost-walk-carrier-guard
branch
from
August 9, 2026 00:45
d5429d1 to
e6dac9c
Compare
matthewevans
enabled auto-merge
August 9, 2026 00:46
`visit_ability_def_costs_scoped` follows only the chain-link axis (`cost`,
`unless_pay.cost`, `sub_ability`, `else_ability`, `mode_abilities`). It does
not descend the eight inline branch carriers that `visit_effect_scoped` does
via `visit_nested_ability_def_scoped` — `Vote`, `SeparateIntoPiles`,
`RevealFromHand`, `FlipCoin`, `FlipCoins`, `FlipCoinUntilLose`, `RollDie`,
`ChooseOneOf`. That divergence is documented as a KNOWN GAP on the walker and
chartered for a real fix (drive both walks from one shared carrier list, after
a census of the 8 `ResolutionScope::IncludeRegisteredLater` entry points).
Consequence if the shape appears: CR 605.1a's cost criterion ("its cost and
effect don't move any card to or from a library") is never applied to a cost
on a carrier-nested definition, so an ability whose branch pays `Mill`,
`Exile { Library }`, `ExileWithAggregate { Library }` or
`ReturnToHand { Library }` wrongly keeps mana-ability status. Those four are
the ones that matter: they carry no nested `Effect`, so they are structurally
invisible to any effect-shaped visitor.
Zero such costs exist in the corpus today, so this is a drift guard, not a
fix. The trigger is a *parser* production starting to emit one — a change that
compiles cleanly, fails no existing test, and whose author is working in
`parser/oracle_effect/` where nothing points at `ability_visit.rs`.
Why a CI gate and not a `#[test]`. The full export is gitignored and absent
from the `Rust tests` job, so a test reading it via
`support::shared_card_export_json()` self-skips there — see the header of
`scripts/check-test-card-data-load.sh`, which says so outright. Since the
triggering change arrives through CI, a CI-invisible guard is green for
exactly the person it exists to stop. Wired instead into `card-data-gate`,
which has the generated export, alongside `draw_replacement_census.py
--corpus` — same reasoning, same job, same unconditional placement (the
gates-cache does not hash `scripts/**`, so a cache hit must not skip it).
Missing input is an error, never a skip, and two reach guards keep a zero-hit
result honest: a minimum corpus size, and a requirement that the scan actually
encounter carrier nodes (a rename of the serialized tags would otherwise pass
vacuously).
Verified by falsification against the live 35,657-card export rather than by
observing a green:
- clean run: 0 hits, 437 carrier nodes reached, 3.4s
- seeding the walk as if the root were under a carrier: 13,511 hits, with
card names and JSON paths rendered — the same count the Rust prototype
produced under the same seed, confirming the port is equivalent
- renaming all eight carrier tags: reach guard fires
- truncating the export to 100 cards: reach guard fires
- absent card-data: exit 2 with a generate-it message, not a pass
matthewevans
force-pushed
the
cost-walk-carrier-guard
branch
from
August 9, 2026 01:03
e6dac9c to
6c7e5d1
Compare
matthewevans
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/cost_walk_carrier_census.py`:
- Around line 132-145: The input-loading flow before the MIN_CARDS guard must
handle invalid data separately from census violations: catch file read and JSON
parsing errors, print an input error to stderr, and return status 2; also
validate that the parsed export is an object before len() and export.items() are
used, returning 2 for invalid types. Preserve the existing status for detected
rule gaps in the later census logic.
- Around line 97-109: Update the census walker and corpus gate to track the set
of encountered carrier tags, not just whether any carrier was found. Require
INLINE_BRANCH_CARRIERS - encountered_tags to be empty before passing validation,
while retaining a separate carrier-node count for success reporting; keep the
gate’s existing general input and minimum corpus/reachability checks.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 83c8a950-4acb-422f-82f2-5f78e19c100e
📒 Files selected for processing (2)
.github/workflows/ci.ymlscripts/cost_walk_carrier_census.py
Comment on lines
+97
to
+109
| inner = carrier_tag(node) | ||
| if inner is not None: | ||
| seen[0] += 1 | ||
| if under_carrier is not None and is_ability_def_shaped(node) and node.get("cost") is not None: | ||
| hits.append(f"{trail} (under `{under_carrier}`)") | ||
| # A node can be BOTH nested under an outer carrier and a carrier itself; | ||
| # the innermost tag is the more useful one to report. | ||
| nxt = inner if inner is not None else under_carrier | ||
| for key, value in node.items(): | ||
| walk(value, nxt, f"{trail}/{key}", hits, seen) | ||
| elif isinstance(node, list): | ||
| for index, value in enumerate(node): | ||
| walk(value, under_carrier, f"{trail}[{index}]", hits, seen) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Require reachability for each supported carrier tag.
seen[0] > 0 only proves that one supported carrier exists. If one serialized tag changes while another seven remain present, this guard passes and the walker no longer scans definitions below the renamed carrier.
Record the encountered tags and fail when INLINE_BRANCH_CARRIERS - encountered_tags is nonempty. Keep a separate carrier-node count for the success output.
As per path instructions, the corpus gate must remain a general detector and “validate required input data and minimum corpus/carrier reachability.”
Also applies to: 147-159
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/cost_walk_carrier_census.py` around lines 97 - 109, Update the census
walker and corpus gate to track the set of encountered carrier tags, not just
whether any carrier was found. Require INLINE_BRANCH_CARRIERS - encountered_tags
to be empty before passing validation, while retaining a separate carrier-node
count for success reporting; keep the gate’s existing general input and minimum
corpus/reachability checks.
Source: Path instructions
Comment on lines
+132
to
+145
| export = json.loads(args.card_data.read_text(encoding="utf-8")) | ||
|
|
||
| if len(export) < MIN_CARDS: | ||
| print( | ||
| f"ERROR: reach guard failed -- export has {len(export)} entries, expected >= {MIN_CARDS}.\n" | ||
| "A truncated export makes the zero-hit result below meaningless.", | ||
| file=sys.stderr, | ||
| ) | ||
| return 2 | ||
|
|
||
| hits: list[str] = [] | ||
| seen = [0] | ||
| for name, card in export.items(): | ||
| walk(card, None, name, hits, seen) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Return the input-data failure status for invalid JSON.
At Line 132, malformed or physically truncated JSON raises JSONDecodeError and exits with status 1. Status 1 also means that the census detected a rules gap at Lines 161-180. A valid JSON value that is not an object can also fail at Line 134 or Line 144 with the same status.
Catch read and JSON parse errors, validate that the export is an object, print an input error, and return 2.
As per path instructions, the gate must “use distinct nonzero failures for missing/truncated data versus detected violations.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/cost_walk_carrier_census.py` around lines 132 - 145, The
input-loading flow before the MIN_CARDS guard must handle invalid data
separately from census violations: catch file read and JSON parsing errors,
print an input error to stderr, and return status 2; also validate that the
parsed export is an object before len() and export.items() are used, returning 2
for invalid types. Preserve the existing status for detected rule gaps in the
later census logic.
Source: Path instructions
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #7113, closing the CodeRabbit thread on
visit_ability_def_costs_scoped.The gap
visit_ability_def_costs_scopedfollows only the chain-link axis (cost,unless_pay.cost,sub_ability,else_ability,mode_abilities). It does not descend the eight inline branch carriers thatvisit_effect_scopedreaches viavisit_nested_ability_def_scoped:Vote,SeparateIntoPiles,RevealFromHand,FlipCoin,FlipCoins,FlipCoinUntilLose,RollDie,ChooseOneOf.So a cost on a carrier-nested
AbilityDefinitionescapes CR 605.1a's cost criterion — "its cost and effect don't move any card to or from a library" — and an ability whose branch paysMill,Exile { Library },ExileWithAggregate { Library }orReturnToHand { Library }keeps mana-ability status it should have lost. Those four are the ones that matter: they carry no nestedEffect, so they are structurally invisible to any effect-shaped visitor.Zero such costs exist in the corpus today. This is a drift guard, not a fix. The real fix — driving both walks from one shared carrier→nested-def list — widens cost coverage for every
ResolutionScope::IncludeRegisteredLatercaller (8 public entry points) and stays chartered separately behind its own census.Why a CI gate and not a
#[test]This started as a Rust integration test. That was wrong, and the repo says so in its own words.
The trigger for this gap is a parser production starting to emit a cost on a branch-carried definition — a change that compiles cleanly, fails no existing test, and whose author is working in
parser/oracle_effect/where nothing points atability_visit.rs. That author's change arrives through CI.But the full export is gitignored and absent from the
Rust testsjob. The header ofscripts/check-test-card-data-load.shstates it directly: tests reading it "silently self-skip there: they are invisible in CI yet bloat every local and Tilttest-enginerun." A guard that self-skips in CI is green for exactly the person it exists to stop, while costing ~5s on every local run.So it is wired into
card-data-gate, which has the generated export, immediately afterdraw_replacement_census.py --corpus— same reasoning, same job, same unconditional placement. Unconditional matters: the gates-cache does not hashscripts/**, so a cache hit must not skip it. That precedent script states the principle this one follows: "a gate that quietly passes when its input is missing is not a gate."Validation
Verified by falsification against the live 35,657-card export, not by observing a green:
The 13,511 figure is the same count the Rust prototype produced under the same seeding, which is what confirms the port is semantically equivalent to the walk it replaces.
Two reach guards keep the zero-hit result honest: a minimum corpus size, and a requirement that the scan actually encounter carrier nodes — a rename of the serialized tags would otherwise pass vacuously.
Diff
Two files, 209 insertions, 0 deletions: the census script, and the
card-data-gatestep that runs it. No engine or test-binary changes.Summary by CodeRabbit