feat(parser): +X/+Y dynamic pump with per-axis quantities (Aspect of Wolf)#5743
Conversation
matthewevans
left a comment
There was a problem hiding this comment.
Request changes — the claimed supported-card behavior still lacks a discriminating runtime witness.
🔴 Blocker
crates/engine/src/parser/oracle_static/tests.rs:28788-28827verifies only the parser output. It never installs Aspect of Wolf on an enchanted creature, supplies an odd Forest count, or observes the layer result. A regression in static-definition attachment, recipient rebinding, or dynamic quantity resolution would still pass while a supported card gets the wrong P/T. Add an integration test through the game pipeline that shows three Forests yield+1/+2, and that fails if the per-axis binding is reverted.
🟡 Non-blocking
- The parser test correctly distinguishes the down/up
QuantityExprs; keep it as the narrow grammar guard alongside the runtime test.
✅ Clean
- The production parser reaches the existing dynamic-P/T authority and uses the existing rounded quantity expression rather than adding new engine surface. The fetched Oracle text matches the comma-separated
X/Yform handled by this diff.
Recommendation: request changes — add the end-to-end odd-count runtime regression before marking Aspect of Wolf supported.
There was a problem hiding this comment.
Code Review
This pull request introduces support for parsing and handling compound "+X/+Y" pump binding clauses (such as in Aspect of Wolf) where the X and Y axes bind to different quantities. It updates the parser to split these clauses on "and Y is", resolves each axis independently, and allows 'y' as a dynamic variable axis in the grammar. Feedback focuses on ensuring strict adherence to the project's architectural rules (R1), specifically pointing out that the manual string splitting and trimming operations bypass the nom parser framework. Additionally, a minor documentation inconsistency regarding the casing of the input string was noted.
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.
| if let Some((x_part, y_part)) = clause.split_around(", and y is ") { | ||
| if parse_where_x_quantity_expression(x_part.original).is_some() | ||
| && parse_where_x_quantity_expression(y_part.original).is_some() | ||
| { | ||
| return clause; | ||
| } | ||
| } |
There was a problem hiding this comment.
This new logic uses clause.split_around(", and y is ") for parsing dispatch, which appears to violate style guide rule R1. R1 mandates the use of nom combinators for all new parser dispatch logic. Using string-splitting methods for dispatching logic is explicitly discouraged.
A more idiomatic approach would be to use a nom parser here. For example, you could attempt to parse the clause with nom::sequence::separated_pair and nom::bytes::complete::tag_no_case(", and y is "). If it succeeds, you know you have the two-part quantity expression. This would align better with the architectural rules of the project.
References
- R1. Nom combinators on the first pass — no exceptions. Every new parser dispatch under
crates/engine/src/parser/must use nom 8.0 combinators. Findings include any new.contains("..."),.starts_with("..."),.ends_with("..."),.find("..."), or.split_once("...")used for parsing dispatch in non-test parser code. (link) - Avoid verbatim string equality for parsing Oracle phrases as it bypasses the robust nom-based parser and creates fragile matches. Instead, decompose compound phrases into modular, reusable parsers for constituent parts and compose them using idiomatic combinator aggregates to prevent combinatorial explosion and improve maintainability.
| let (_, (x_expr, y_expr)) = nom_primitives::split_once_on(&lower, " and y is ").ok()?; | ||
| Some(( | ||
| x_expr.trim().trim_end_matches(',').to_string(), | ||
| y_expr.trim().trim_end_matches('.').trim().to_string(), | ||
| )) |
There was a problem hiding this comment.
The use of .trim().trim_end_matches(',') and .trim().trim_end_matches('.').trim() in split_x_and_y_where_clause violates style guide rule R1. This rule mandates using nom combinators for parsing logic, and manual string manipulation for cleaning up parser inputs is an anti-pattern in this codebase.
Instead of manually trimming, this logic should be incorporated into the consuming parsers using nom combinators. For example, parse_cda_quantity could be wrapped to handle optional leading/trailing whitespace and punctuation. If modifying parse_cda_quantity is not feasible, a local nom parser could be defined within split_x_and_y_where_clause to handle this cleaning idiomatically.
References
- R1. Nom combinators on the first pass — no exceptions. Every new parser dispatch under
crates/engine/src/parser/must use nom 8.0 combinators. Findings include any new.contains("..."),.starts_with("..."),.ends_with("..."),.find("..."), or.split_once("...")used for parsing dispatch in non-test parser code. (link) - Avoid redundant string prefix checks or manual trimming before calling parsing functions that already perform leading character validation and whitespace trimming, and avoid verbatim string equality or manual string manipulation for parsing Oracle phrases by using robust nom-based parsers.
| /// rounded up"). Returns `None` for the common single-quantity clause. `wx` is | ||
| /// already lowercased, so the " and y is " boundary matches the printed |
There was a problem hiding this comment.
The comment on line 1161 states that "wx is already lowercased", but the implementation on line 1168 does wx.to_lowercase(). The comment on line 1165 correctly states that wx arrives in original case. This inconsistency can be confusing. Please update the comment to reflect the implementation.
| /// rounded up"). Returns `None` for the common single-quantity clause. `wx` is | |
| /// already lowercased, so the " and y is " boundary matches the printed | |
| /// rounded up"). Returns `None` for the common single-quantity clause. The | |
| /// " and y is " boundary is matched case-insensitively. The trailing comma left |
Parse changes introduced by this PR · 1 card(s), 2 signature(s) (baseline: main
|
|
Thanks both — addressed. @matthewevans (blocker — runtime witness): added @gemini (R1):
Local: unit + new integration test pass; |
…m cleanup
- Blocker (matthewevans): add tests/integration/aspect_of_wolf_per_axis_xy.rs
— installs Aspect of Wolf on a 2/2, controls 3 Forests, and asserts the layer
pipeline yields 3/4 (X = half rounded down = 1 power, Y = half rounded up = 2
toughness). Fails if the per-axis binding is reverted.
- R1 (gemini): replace the lower.rs split_around with the nom split_once_on
combinator; split the anthem clause on ', and y is ' so the joining comma is
consumed and the manual trim_end_matches(',' / '.') go away (period is already
stripped upstream). Fix the stale 'already lowercased' doc line.
|
Thanks for the detailed breakdown and for adding the integration test. The approach of splitting the binding clause and resolving each axis independently is a robust way to handle the Aspect of Wolf logic without introducing new engine variants. Everything looks clean and well-verified. |
matthewevans
left a comment
There was a problem hiding this comment.
Request changes — the new per-axis grammar incorrectly treats unbound -X/-Y text as a cost-X pump.
🔴 Blocker
parse_variable_pt_patternnow acceptsyas a dynamic axis unconditionally, whileparse_dynamic_pt_in_textstill defaults every missing binding toQuantityRef::CostXPaid. The current parse-diff confirms the resulting regression: Snowblind gained a staticContinuousparse. Its Oracle text isEnchanted creature gets -X/-Y. If ... X is ... Y is ...; it has no{X}cost and its X/Y values are defined by later conditional sentences. The new static parser therefore emits-CostXPaid/-CostXPaid, which resolves to0for the Aura rather than its rules-defined reduction. KeepYdistinct fromXand accept the second dynamic axis only when the samewhere X is <A>, and Y is <B>binding was structurally parsed; otherwise preserve the existing unsupported result. Add a regression guard for Snowblind (no synthetic cost-X static), then regenerate/inspect the parse diff.
🟡 Non-blocking
- The current parse-diff also adds Bioplasm, although the PR claims only Aspect of Wolf. Once the blocker is fixed, either include Bioplasm in the stated scope with an appropriate behavioral witness or narrow the change so the documented parse impact matches the actual card set.
✅ Clean
- The new Aspect integration test is a real layer-pipeline witness: three Forests produce
3/4, so it distinguishes the down/up axes from a shared quantity. Thenom_primitives::split_once_onreplacement addresses Gemini's stale R1 review finding.
Recommendation: request changes — guard Y-axis parsing on the structured paired binding and prove Snowblind remains honest before re-review.
…Wolf) A dynamic P/T pump whose two axes bind to DIFFERENT quantities — "gets +X/+Y, where X is <A> and Y is <B>" — dropped: parse_variable_pt_pattern rejected the 'y' axis, and parse_dynamic_pt_in_text applied one quantity to both axes; the where-clause bounding also truncated the 'and Y is <B>' half. - grammar.rs: parse_variable_pt_pattern accepts 'y' as a dynamic axis. - anthem.rs: parse_dynamic_pt_in_text splits the binding clause on ' and y is ' and resolves each half via parse_cda_quantity, assigning X to power and Y to toughness (single-quantity clauses unchanged). - lower.rs: structurally_bound_where_x_clause keeps a '<X qty>, and Y is <Y qty>' continuation (a binding continuation, not a new instruction) when both halves parse as where-X quantities. Aspect of Wolf now grants +X/+Y where X = half the number of Forests you control rounded down (power) and Y = the same rounded up (toughness). No new engine variant; both quantities (DivideRounded over ObjectCount) already exist.
…m cleanup
- Blocker (matthewevans): add tests/integration/aspect_of_wolf_per_axis_xy.rs
— installs Aspect of Wolf on a 2/2, controls 3 Forests, and asserts the layer
pipeline yields 3/4 (X = half rounded down = 1 power, Y = half rounded up = 2
toughness). Fails if the per-axis binding is reverted.
- R1 (gemini): replace the lower.rs split_around with the nom split_once_on
combinator; split the anthem clause on ', and y is ' so the joining comma is
consumed and the manual trim_end_matches(',' / '.') go away (period is already
stripped upstream). Fix the stale 'already lowercased' doc line.
…ng (Snowblind)
matthewevans [HIGH]: accepting 'y' unconditionally regressed Snowblind
("gets -X/-Y", X/Y defined by later sentences, no {X} cost) into a bogus
-CostXPaid/-CostXPaid static.
parse_variable_pt_pattern now returns a typed PtAxisMag { Fixed | VarX |
VarY } per axis. parse_dynamic_pt_in_text accepts a distinct '+X/+Y'
(VarX on power, VarY on toughness) ONLY when the paired
'where X is <A>, and Y is <B>' binding is structurally present; otherwise
the pattern is left unsupported (no synthesized cost-X). Same-letter and
fixed axes are unchanged.
- Snowblind '-X/-Y' now drops (regression guard added).
- Bioplasm ('...power and Y is its toughness', no joining comma) no
longer matches the ', and y is ' split, so it also stays unsupported —
the parse impact narrows back to Aspect of Wolf only.
|
Good catch — fixed the regression. Blocker (Snowblind synthetic cost-X):
Non-blocking (Bioplasm): its binding is Local: full |
f81bcda to
ecbe3ea
Compare
matthewevans
left a comment
There was a problem hiding this comment.
Approved — the per-axis parser is now constrained to the supported paired +X/+Y form and has a discriminating layer-pipeline regression.
🔴 Blocker
- None.
🟡 Non-blocking
- None.
✅ Clean
- Aspect of Wolf’s odd three-Forest runtime test distinguishes the rounded-down power axis from the rounded-up toughness axis through attachment and layer evaluation.
- The structured paired binding is the only accepted
Yform; Snowblind and the unsupported+Y/+X/+Y/+Yshapes cannot synthesize cost-X semantics. - The fresh parse-diff contains only Aspect of Wolf, matching the claimed one-card scope.
Recommendation: approve and enqueue.
Summary
A dynamic P/T pump whose two axes bind to different quantities —
gets +X/+Y, where X is <A> and Y is <B>— dropped entirely. Aspect of Wolf now grants+X/+Ywhere X = half the number of Forests you control, rounded down (power) and Y = the same, rounded up (toughness).Implementation method
Hand-authored, grammar-only — no new engine variant. Both quantities (
QuantityExpr::DivideRoundedoverObjectCount) already exist and are runtime-evaluated; the parser just couldn't reach the second axis.Three seams:
grammar.rs—parse_variable_pt_patternacceptsyas a dynamic axis (wasx/digit only).anthem.rs—parse_dynamic_pt_in_textsplits the binding clause onand y is(case-insensitively) and resolves each half viaparse_cda_quantity, assigning X→power, Y→toughness. Single-quantity clauses (+X/+X, +X/+0, …) are unchanged.lower.rs—structurally_bound_where_x_clausekeeps a<X qty>, and Y is <Y qty>continuation (a binding continuation, not a new instruction) only when both halves independently parse as where-X quantities, so a genuine…, and <verb>next-instruction still bounds off.CR references
CR 613.4c (Layer 7c additive P/T) + CR 107.1a (rounding). Touched, not invented.
Files changed
parser/oracle_static/grammar.rs,parser/oracle_static/anthem.rs(+ helpersplit_x_and_y_where_clause)parser/oracle_effect/lower.rs(where-X bounding)parser/oracle_static/mod.rs(prelude:RoundingMode),tests.rsVerification (local, committed head
f900c7dfdd42690dd03d4bd52b5c0c703b197145)cargo test -p engine --lib dynamic_pt_pump_binds_x_and_y_axes_to_distinct_quantities— ok (power = DivideRounded/2/Down, toughness = DivideRounded/2/Up)cargo test -p engine --lib parser::oracle— ok (7904 passed, 0 failed) — full sweep, since the changed functions are core to every dynamic pump and where-X cardcargo fmt -p engine -- --check,cargo clippy -p engine --lib(-D warnings) — cleanscripts/check-parser-combinators.sh <base>,scripts/check-engine-authorities.sh <base>— passTransparency: did not run
scripts/ai-gate.sh(Gate A) — fresh--releasebuild, local disk tight; not fabricating a PASS line. CI covers this head.Anchored on
crates/engine/src/parser/oracle_static/anthem.rs— the existing single-quantitywhere Xresolution inparse_dynamic_pt_in_textthis generalizes.crates/engine/src/parser/oracle_effect/lower.rs— the existing comma-boundedstructurally_bound_where_x_clause(theand Y iscase is added ahead of it).Claimed parse impact
+X/+Yper-axis pump now parses (was dropped). (Phyrexian Ingester shares this+X/+Yshape but additionally needs an exiled-card toughness quantity — a separate follow-up.)