fix(coverage): stop flagging 'perpetually gets +N/+M' as a semantic-audit false positive

[minion1227]

Summary

Fixes the single largest false-positive class in the cargo semantic-audit dev tool (crates/engine/src/game/coverage.rs). The pump-parameter check pump_matches_oracle recognized Effect::Pump, Effect::PumpAll, and static AddPower/AddToughness modifications as satisfying a "+N/+M" oracle line, but not Effect::ApplyPerpetual { modification: PerpetualModification::ModifyPowerToughness { .. } }.

Digital-only Alchemy cards that read "[object] perpetually gets +N/+M" lower to ApplyPerpetual, not a top-level Pump, so each one was reported as a spurious WrongParameter: no matching pump effect finding. The parser was always correct — only the audit heuristic was blind to this lowering.

Impact (measured)

cargo semantic-audit on the current card database, before vs. after:

Category	Before	After
WrongParameter	39	11
SilentDrop	201	201
DroppedCondition	66	66
DroppedDuration	24	24
Cards with findings	317	291

−28 false positives, every other category unchanged — the fix is surgical to the pump check. The 28 eliminated are all perpetually gets +N/+M cards (Heir to Dragonfire, Perennial Gravewarden, Tomakul Phoenix, Scion of Shiv, Shattering Finale, Davriel's Withering, …); I confirmed by live-parse that each emits ApplyPerpetual{ModifyPowerToughness} with correct deltas.

Anchored on

• crates/engine/src/game/coverage.rs:7531 — the existing pump_matches_oracle match arms for Effect::Pump / Effect::PumpAll / Effect::GenericEffect; the new arm mirrors their if <fields> == expected => return true guard-and-return shape on a typed enum variant.
• crates/engine/src/game/coverage.rs — the existing test_audit_counter_parameter_accepts_choose_one_of_counter_branches test; the new test mirrors its make_face() → push ability → audit_card_lines() → assert-no-WrongParameter structure exactly.

Gate A

./scripts/check-parser-combinators.sh → exit 0 (silent success; no violations). This change adds no parser dispatch code — it extends an audit heuristic with a typed match arm on an existing enum variant. No find/contains/split_once/starts_with/string-literal match arms.

CR

• CR 613.4c — layer 7c: effects that modify (not set) power/toughness. Verified against docs/MagicCompRules.txt. ("perpetually" itself is digital-only Alchemy with no CR entry, matching the existing annotation on PerpetualModification in types/ability.rs.)

Verification

• cargo test -p engine --lib audit → 16/16 pass, including the new test_audit_pump_parameter_accepts_perpetual_modify_power_toughness; no regressions.
• cargo fmt -p engine clean.
• cargo semantic-audit re-run: WrongParameter 39 → 11 (table above); all other categories byte-identical.

Scope / risk

Single file, audit-tooling only. No game-logic, parser, or serialization change — this does not alter how any card parses or resolves; it only fixes how the semantic-audit dev tool classifies an already-correct parse. Zero runtime/gameplay impact. No CI-relevant surface beyond the added unit test.

Model: claude-opus-4-8
Tier: Frontier
Thinking: High

[gemini-code-assist]

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

[github-actions]

Parse changes introduced by this PR · 16 card(s), 6 signature(s) (baseline: main 02bb03df2ba4)

14 card(s) · ability/PreventDamage · field target: any target → parent target

Examples: Delirium, Djeru's Resolve, Ebony Horse (+11 more)

7 card(s) · ability/PreventDamage · added: PreventDamage (amount=All, scope=CombatDamage, target=any target)

Examples: Delirium, Ebony Horse, Elvish Scout (+4 more)

2 card(s) · ability/effect_structure · removed: effect_structure

Examples: Diligent Farmhand, Pardic Firecat

1 card(s) · static/CountsAsNamed(Flame Burst) · added: CountsAsNamed(Flame Burst)

Examples: Pardic Firecat

1 card(s) · static/CountsAsNamed(Muscle Burst) · added: CountsAsNamed(Muscle Burst)

Examples: Diligent Farmhand

1 card(s) · ability/PreventDamage · added: PreventDamage (amount=All, scope=AllDamage, target=any target)

Examples: Kiora, the Tide's Fury

1 card(s) had Oracle-text changes (errata/reprint) — excluded as non-parser.

[matthewevans]

Reviewed at head e496e63e. All required checks green, parse-diff sticky clean (no_changes, correct — this is audit tooling, not parser code). The CR 613.4c citation checks out: docs/MagicCompRules.txt:2990 reads "Layer 7c: Effects and counters that modify power and/or toughness (but don't set power and/or toughness to a specific number or value) are applied." Accurate, and the note that "perpetually" has no CR entry of its own is honest.

The underlying observation is right. Effect::ApplyPerpetual { modification: PerpetualModification::ModifyPowerToughness { .. } } is a real lowering of "perpetually gets +N/+M", and every Alchemy perpetual-pump card currently produces a spurious WrongParameter: no matching pump effect. Worth fixing.

Two things before this lands.

---

MED — the new arm can't tell "perpetually gets +1/+1" from "gets +1/+1"

crates/engine/src/game/coverage.rs:7515

fn pump_matches_oracle(
    def: &AbilityDefinition,
    expected_power: i32,
    expected_toughness: i32,
) -> bool {

The function never receives the Oracle line. It only sees the expected deltas the caller extracted. So the new arm accepts ApplyPerpetual { ModifyPowerToughness { .. } } as satisfying any "+N/+M" text — including a plain, temporary "gets +1/+1 until end of turn".

That inverts what the audit is for. If a card whose Oracle text says "gets +1/+1 until end of turn" ever mis-lowers to ApplyPerpetual — a permanent modification instead of a until-end-of-turn one, which is a serious rules bug — the semantic audit used to catch it as WrongParameter. After this change it passes silently. You've widened the acceptance set past the class you meant to admit.

The caller has what's needed. has_pump (coverage.rs:7913) is reached from audit_card_lines(oracle, &face), which knows the line text. Thread that down — a PerpetualMatch enum, or the line itself — and admit ApplyPerpetual only when the line actually says "perpetually". That keeps the audit discriminating for the temporary case while removing the false positives you're targeting.

MED — the new test is a vacuous negative

coverage.rs:11713

let findings = audit_card_lines(oracle, &face);
assert!(
    !findings.iter().any(
        |f| matches!(f, SemanticFinding::WrongParameter { field, .. } if field == "pump")
    ),
    ...
);

This asserts the absence of a finding. It passes if the new arm works — and equally if audit_card_lines produced no findings at all for some unrelated reason (the line failing to classify as a pump line, make_face() shaping the input so the pump check is never reached, an early return upstream). Nothing proves the input reached the code you added.

This is the highest-frequency defect we see in contributor PRs, and it's exactly why: a negative assertion is only meaningful with a paired positive reach-guard. Please add both:

1. Reach-guard. In the same test, assert the audit did run the pump check — e.g. construct the identical face with a deliberately mismatched delta (power_delta: 2 against +1/+1 text) and assert a WrongParameter { field: "pump" } is produced. That proves the code path is live and that your passing case passes for the right reason.
2. Discrimination for the MED above. Once the arm is guarded on "perpetually", add the case that makes the guard matter: Oracle "This creature gets +1/+1 until end of turn." lowered to ApplyPerpetual{ModifyPowerToughness{1,1}} must still be flagged.

Both should fail on the current code — that's the point.

---

To be explicit about severity: this is audit tooling, not game logic, so the blast radius is false negatives in cargo semantic-audit rather than wrong game behavior. But an audit that can't distinguish a permanent modification from a temporary one is quietly less useful than one that reports a few false positives, and the fix is small. The comment naming Heir to Dragonfire, Perennial Gravewarden, and Tomakul Phoenix is good practice — it makes the class legible.

[minion1227]

Thanks for the careful review — both addressed in 513e7b2e.

MED 1 (over-broad acceptance). Threaded a typed PerpetualPump {Allowed, Disallowed} signal, derived once from the oracle line at the call site, down through has_pump → pump_matches_oracle. The ApplyPerpetual{ModifyPowerToughness} arm now returns true only when the line actually says "perpetual", so a temporary "+N/+M until end of turn" that mis-lowered to a permanent modification is no longer silently admitted.

MED 2 (vacuous negative). Rewrote the test to prove the fix non-vacuously:

• Function level (the gating authority, directly): pump_matches_oracle(ApplyPerpetual{+1/+1}, 1, 1, Allowed) == true; …, Disallowed == false; and a mismatched delta (ApplyPerpetual{+2/+2} vs 1,1, Allowed) == false. This exercises the arm you asked to be gated and proves the +1/+1 acceptance passes for the right reason.
• End-to-end accept: a perpetual line + ApplyPerpetual → 0 pump findings.
• End-to-end discrimination: your suggested "Target creature gets +1/+1 until end of turn." case, lowered to a permanent ApplyPerpetual, is still flagged.

One nuance worth surfacing on the discrimination case: it comes out as a SilentDrop, not a WrongParameter. The audit's line↔element matcher won't associate a permanent ApplyPerpetual with a temporary "+N/+M" line (the permanent effect doesn't match the pump-line shape, so the delta-based match fails), so the mislowering is caught upstream of the parameter check rather than by it. The test asserts the line is flagged (non-empty findings) and documents this. If you'd rather the assertion be pinned to a specific finding type, I can reshape it — but the observable guarantee (a temporary-worded line that lowered permanent does not pass silently) holds either way.

cargo test -p engine --lib audit: 16/16 green.

[matthewevans]

APPROVE — supersedes my earlier CHANGES_REQUESTED (review 4668788478). Both MEDs from that round are resolved by 513e7b2e.

MED #1 (arm over-accepted) — resolved. pump_matches_oracle now takes a fourth argument, a typed PerpetualPump { Allowed, Disallowed } enum, and the new Effect::ApplyPerpetual { ModifyPowerToughness } arm is gated on it:

Effect::ApplyPerpetual {
    modification: PerpetualModification::ModifyPowerToughness { power_delta, toughness_delta },
    ..
} if perpetual == PerpetualPump::Allowed
    && *power_delta == expected_power
    && *toughness_delta == expected_toughness => { return true; }

The value is derived once per Oracle line at the call site (PerpetualPump::from_lower_line(&lower_for_pt), keyed on contains("perpetual")) and threaded through has_pump → pump_matches_oracle. A temporary "gets +N/+M until end of turn" line lowers to Disallowed, the guard fails, and the arm no longer silently accepts a permanent ApplyPerpetual — restoring the audit's ability to catch a duration-mislowering bug. The guard lives inside the ApplyPerpetual match arm, so Disallowed genuinely reaches and rejects that arm rather than short-circuiting before it. Note the author chose a typed enum over a bool unprompted, matching the no-bool-flags rule.

MED #2 (vacuous negative) — resolved by a four-layer discrimination ladder in test_audit_pump_parameter_perpetual_gated_by_oracle_line:

1. pump_matches_oracle(perpetual_pump(1,1), 1, 1, Allowed) → accepts.
2. !pump_matches_oracle(perpetual_pump(1,1), 1, 1, Disallowed) → rejects (the fix itself; would fail if reverted).
3. !pump_matches_oracle(perpetual_pump(2,2), 1, 1, Allowed) → rejects on delta mismatch, proving the arm compares deltas rather than blanket-accepting ApplyPerpetual.
4. Reach-guard: a temporary "Target creature gets +1/+1 until end of turn." line whose effect is ApplyPerpetual{1,1} asserts !temporary_findings.is_empty() — proving audit_card_lines actually emits findings on this input (it flags both a DroppedDuration and a WrongParameter{field:"pump"}), which is what makes the earlier "zero pump findings" assertion on the perpetual line meaningful rather than vacuous.

I hand-traced both cases end to end: the perpetual line produces no WrongParameter{pump}, and the temporary-mislowered line is still flagged. Change is at the right seam (the audit acceptance predicate, not the parser) and this is audit-only tooling, so the nom-combinator mandate does not apply to the contains("perpetual") gate. CR 613.4c (Layer 7c P/T modification) verified. All 12 required checks green.