Skip to content

fix(parser): Dig 'reveal ... on top of your library, rest on bottom' kept-to-top destination (#2349) - #3155

Merged
matthewevans merged 1 commit into
mainfrom
fix/issue-2349-fertile-thicket-reveal-top
Jun 13, 2026
Merged

fix(parser): Dig 'reveal ... on top of your library, rest on bottom' kept-to-top destination (#2349)#3155
matthewevans merged 1 commit into
mainfrom
fix/issue-2349-fertile-thicket-reveal-top

Conversation

@matthewevans

Copy link
Copy Markdown
Member

Summary

Closes #2349.

Fertile Thicket ("reveal up to one basic land card from among them, then put that card on top of your library and the rest on the bottom") did not keep the revealed land on top — the parser's parse_dig_destination_tail (oracle_effect/sequence.rs) recognized only "onto the battlefield"/"into your hand" as kept destinations, not "on top of your library", so the kept destination resolved to None and the reveal became unimplemented. The DigChoice resolver already routes kept→top / rest→bottom correctly when destination == Some(Library) — only the parser couldn't reach it.

Class fix (parser-only):

  • Add an "on top of your library" arm to parse_dig_destination_tailSome((Some(Zone::Library), false)) (+ leading-comma strip so ", then put that card on top..." is reached).
  • Promote reveal: true for reveal-from-among clauses via a new parser-internal reveal_verb field on ContinuationAst::DigFromAmong (look-only digs keep reveal_verb: false — no regression).

Covers the reveal-to-top peek class; resolver/types unchanged.

Tests

  • Runtime fertile_thicket_reveal_to_top_2349.rs: revealed basic land ends on library top (index 0) and is publicly revealed; the other 4 go to the bottom. Both asserts fail on revert.
  • Parser unit test: the clause → DigFromAmong{ Up(1), destination: Some(Library), rest_destination: Some(Library), reveal_verb: true }.

CR 401.4, 701.20a, 701.20e (grep-verified). Verified locally via the parser combinator gate; full verification deferred to CI.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request resolves Issue #2349 by enabling the parser to recognize library-top destinations (such as 'on top of your library') and correctly tracking whether a 'reveal' verb was used to promote the action to a public reveal. However, the introduction of the reveal_verb boolean field on the ContinuationAst::DigFromAmong struct violates Style Guide Rule R2 (No bool fields). It is recommended to replace this boolean field with a typed enum, such as DigVisibility, to better represent the design space.

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.

Comment on lines +309 to +314
/// CR 701.20a vs 701.20e: True when the from-among clause's stripped verb
/// was "reveal" (a public action) rather than "put"/"choose" (a private
/// look). Promotes the patched Dig to `reveal: true` even when the kept
/// cards route to a fixed library position (Fertile Thicket).
#[serde(default)]
reveal_verb: bool,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

[HIGH] Violation of Style Guide Rule R2 (No bool fields). Using a bool field (reveal_verb) to represent the visibility/action type of the dig clause does not express the design space well and violates the repository's strict rule against boolean fields on structs or variant payloads.

Suggested fix: Replace reveal_verb: bool with a typed enum DigVisibility defined at the module level:

#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Default)]
pub(crate) enum DigVisibility {
    #[default]
    Private,
    PublicReveal,
}
Suggested change
/// CR 701.20a vs 701.20e: True when the from-among clause's stripped verb
/// was "reveal" (a public action) rather than "put"/"choose" (a private
/// look). Promotes the patched Dig to `reveal: true` even when the kept
/// cards route to a fixed library position (Fertile Thicket).
#[serde(default)]
reveal_verb: bool,
/// CR 701.20a vs 701.20e: The visibility of the from-among clause's action.
/// Promotes the patched Dig to reveal: true when set to PublicReveal.
#[serde(default)]
visibility: DigVisibility,
References
  1. Rule R2: No bool fields — parameterize with existing typed enums. A bool field never expresses the design space; the project uses typed enums instead. Any new bool field on a struct or bool variant payload where an existing enum (or a small new enum) would carry the same information with more meaning is a finding. (link)

@matthewevans

Copy link
Copy Markdown
Member Author

Deferring this one. The parser fix is real (the clause splitter bisects 'reveal … from among them, then put that card on top …' so the from-among continuation never absorbs into the look-Dig), but suppressing the , then put split for from-among clauses regresses Zimone's Experiment (zimones_experiment_full_parse_tree: keep_count Some(2) → None). Getting the splitter guard precise enough to fix Fertile Thicket without disturbing the other from-among/put-rest cards needs more careful surgery than is worth blocking the queue on right now. Will revisit.

@matthewevans
matthewevans force-pushed the fix/issue-2349-fertile-thicket-reveal-top branch from b8be9ba to 0d3850a Compare June 13, 2026 18:42
@matthewevans
matthewevans force-pushed the fix/issue-2349-fertile-thicket-reveal-top branch from 0d3850a to 42433d8 Compare June 13, 2026 19:00
@matthewevans
matthewevans added this pull request to the merge queue Jun 13, 2026
Merged via the queue into main with commit c3db233 Jun 13, 2026
10 checks passed
@matthewevans
matthewevans deleted the fix/issue-2349-fertile-thicket-reveal-top branch June 13, 2026 19:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fertile Thicket: land is sent to bottom of library when player selects 'yes' to look at top 5

1 participant