test(parser): reword assert messages that leaked planning coordinates (#9268)

[luckenbach]

Closes #9268. Follow-up from #8785 / PR #9201. Test-message-only; no behaviour
change and no assertion semantics changed.

The leak

Four assert!/expect messages carried identifiers from the #8785
implementation run — C3.1, C1.5, and C3.9 twice. They surface in CI failure
output, where someone debugging an unrelated break has nothing to look them up
in: the process artifact they indexed no longer exists. #9201's comment-only
cleanup could not reach them because they sit inside string literals.

What changed

Each message now states the property being asserted. The message a debugger
actually sees goes from

C3.9: NEVER an unconditioned CanAttackWithDefender on an interposed line

to

an interposed line must NEVER yield an unconditioned CanAttackWithDefender — the interposed class is the permission's gate, so dropping it grants the permission unconditionally

File	Test	Was
oracle_static/tests.rs	weathered_sentinels_line_is_consumed_by_the_non_attached_static_production	C3.1:
oracle_static/tests.rs	unrecognized_interposed_class_is_permanently_inert_and_leaves_the_card_red	Phase 1's C1.5
oracle_effect/tests.rs	walking_bulwark_comma_compound_carries_the_anchored_condition	C3.9:
oracle_effect/tests.rs	adjacent_defender_grammars_keep_their_own_parse_on_the_effect_side	C3.9:

Reachability verified, not assumed

A reworded message is worthless if the assertion can't fire. Each of the four was
sabotaged in turn and the run confirmed to print the new text.

Worth reporting: my first attempt at that check was itself faulty. I sabotaged
the first test by perturbing the second argument to
parse_can_attack_despite_defender, which is display-only — the parse still
succeeded, the expect never fired, and the test simply passed. Had I read
"no panic" as "fine", I'd have shipped an unverified message. Redone by feeding
production (b) a line it genuinely declines, which produced the expected panic.

Both files restored byte-identical afterwards (md5 verified against pre-sabotage
copies).

Scope, and two things that are NOT hits

assert!/expect message strings only — that is where the reported harm is. A
coordinate in a doc comment reaches a reader who already has the surrounding
text; the issue's complaint is specifically about context-free CI output. Those
files do still contain planning coordinates in comments (U2.x, Phase N,
M-1), left alone deliberately.

Two near-misses I checked and did not change:

• "L3" / "L4" in oracle_static/tests.rs assert messages — these are
  Fblthp's printed Oracle lines 3 and 4, domain vocabulary the surrounding
  doc comments define ("Fblthp L4 — 'You may plot nonland cards…'"). Correct
  usage that a regex sweep would have destroyed.
• "P6 U6a Probe" in oracle_effect/tests.rs — a synthetic card name
  passed to parse_oracle_text, not a failure message. It is a process
  identifier, but it is input data and does not reach CI output. Flagging it
  rather than silently widening scope; happy to rename it if you'd prefer.

Verification

Full battery running against the committed candidate 061e15875; I'll post the
figures when they land rather than assert them now.

🤖 Generated with Claude Code

https://claude.ai/code/session_01GVgJDj9cX1g1Csfqz7Cp2U

Summary by CodeRabbit

• Tests
  • Improved failure messages for tests covering conditional class handling and anchored-class parsing, clarifying the expected behavior when checks fail.
  • Test assertions and expected results remain unchanged.

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository: phase-rs/phase/.coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 68b93609-f0e5-4ca3-ac4d-776ff9c44946

📥 Commits

Reviewing files that changed from the base of the PR and between 061e158 and d242715.

📒 Files selected for processing (1)

• crates/engine/src/parser/oracle_static/tests.rs

🚧 Files skipped from review as they are similar to previous changes (1)

• crates/engine/src/parser/oracle_static/tests.rs

Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review.

---

📝 Walkthrough

Walkthrough

Four assertion failure messages in two parser test files now describe the properties under test. The assertions and expected values are unchanged.

Changes

Parser test failure messages

Layer / File(s)	Summary
Assertion failure-message wording crates/engine/src/parser/oracle_effect/tests.rs, crates/engine/src/parser/oracle_static/tests.rs	Messages now describe the expected interposed-class condition, parser branch, and coverage result. Assertions and expected values are unchanged.

Priority: ⬇️ Low

Estimated code review effort: 1 (Trivial) | ~3 minutes

Change: Other · Severity of issue fixed: Low

Merge Risk: ⚪ Minimal · up to d2427

The reported changes are limited to test failure messages, with no identified merge-blocking issue. Confirm the final wording and run the normal checks before merging.

Architecture Summary

Architecture risk: 🔵 Low · up to 061e1

The change affects 1 system.

Changed systems: crates

Architecture concerns
No architecture-level concerns identified.

Review details

Systems and components

• observed — crates (service) was modified; 2 changed files map to changed impact.

Before / after behavior

• observed — Modified behavior in crates/engine/src/parser/oracle_effect/tests.rs: The assertion message now states that an interposed line must not produce an unconditioned CanAttackWithDefender, and describes the interposed class as the permission gate.
• observed — Modified behavior in crates/engine/src/parser/oracle_effect/tests.rs: The assertion message now states that a re-attached conjunct must retain the interposed class’s condition, and describes the result of rejoining it without that gate.
• observed — Modified behavior in crates/engine/src/parser/oracle_static/tests.rs: The parser test’s failure message now identifies production (b) as the branch expected to consume the line and describes reaching the assertion as evidence that a shadowing branch claimed it first.
• observed — Modified behavior in crates/engine/src/parser/oracle_static/tests.rs: The coverage test’s failure message now states that a recognized anchored class should have no coverage gap, attributing gap reporting to the unenforceable marker rather than unsupported CanAttackWithDefender registry support.

🚥 Pre-merge checks | ✅ 5

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: rewording parser assertion messages that exposed planning coordinates.
Linked Issues check	✅ Passed	The whole-PR diff changes the four assertion messages named in #9268: two in oracle_static/tests.rs and two in oracle_effect/tests.rs. The messages now describe parser, coverage, and condition-pre…
Out of Scope Changes check	✅ Passed	The reviewed changes are limited to the four test message strings required by #9268. The diff shows no production-code, public-API, or assertion-semantics changes. The changes remain within issue scop…
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0…

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

Generated for head d242715c9a4a2c84ae3ec0fdd4e27c5466e48708.

Parse changes introduced by this PR · 0 card(s), 0 signature(s) (baseline: main f6b156d2d8de)

59 card(s) had Oracle-text changes (errata/reprint) — excluded as non-parser.
Cards only in baseline: 229.

[luckenbach]

Battery green on the committed candidate 061e15875:

• cargo fmt --check exit 0
• Gate P PASS
• lib 22063 passed, 0 failed
• integration 7673 passed, 0 failed
• clippy, CI's exact line: 0 errors, exit 0

One note on Gate A, so the line isn't read as more than it is. It reports
SKIPPED (no files under crates/engine/src/parser changed) on a non-degenerate
range. That is correct and by design rather than a miss: SCOPE_EXCLUDES drops
**/tests.rs and **/*_tests.rs, and both files this PR touches are outlined
test files. So Gate A had nothing in scope — that is not evidence the change is
combinator-clean, it's evidence the gate doesn't apply. It happens not to matter
here, since the diff adds no code at all, only string-literal text.

🤖 Generated with Claude Code

https://claude.ai/code/session_01GVgJDj9cX1g1Csfqz7Cp2U

[matthewevans]

Held pending verification of a maintainer fix. On head 061e15875f656362b2ca9c822f83639191d2dc01, crates/engine/src/parser/oracle_static/tests.rs:36769 says a failure means a shadowing branch claimed the line first. This test calls parse_can_attack_despite_defender directly; its expect can fail only when that parser returns None. I prepared a one-string correction locally as commit 712d725e41, preserving the test's assertion and input.

The issue's other three message changes are scoped and useful; the current-head required CI is green and the parse-diff receipt reports no card-parse changes. Tilt is unavailable for this checkout, so the local fix has not been verified or pushed. Next step: verify the corrected candidate with the project Tilt resources, push the fix, then review the new head before approval and merge-queue entry.

[luckenbach]

Your diagnosis is right and the message is mine to own. The test calls
parse_can_attack_despite_defender directly, so no dispatcher is involved and
there is no shadowing branch that could claim the line — the expect fires only
when that parser itself declines. I replaced a meaningless coordinate with an
actively wrong explanation, which is worse than what it replaced.

My own reachability check corroborates you: to make that expect fire I had to
feed the parser a line it genuinely declines ("Flying"). Nothing about
shadowing was involved in producing the failure, and I didn't notice the message
was describing a mechanism the test cannot exercise.

712d725e41 as you describe it — one string, assertion and input preserved — is
the right shape. Standing by; no need to route it through me.

[matthewevans]

Reviewed head d242715: the four failure messages now describe the tested properties, including the direct parser call. Assertions and inputs are unchanged. Local cargo fmt and diff checks passed; required GitHub checks will gate merge.

[matthewevans]

Approved on head d242715c9a4a2c84ae3ec0fdd4e27c5466e48708; merge held for the current-head parse-diff receipt. The test-message correction is reviewed, and the new-head card-data check has passed. The sticky parse-diff comment still identifies the prior head 061e15875f656362b2ca9c822f83639191d2dc01; its publishing workflow runs after CI completes. I disabled auto-merge until that receipt confirms no card-parse changes. Next step: inspect the refreshed receipt, then re-enable auto-merge if it is clean.