feat(manabrew-compat): serve unmapped prompts from the interaction projection - #6700
Merged
Merged
Conversation
…ojection
The adapter named 42 of the engine's 127 `WaitingFor` variants; the other 85
fell through to `local.prompt-unsupported`. Closing them one at a time would
have meant 85 hand-written mappings, each wiring three sites and re-deriving
min/max and constraint logic the engine already computes — duplicated game
logic inside a serialization boundary.
The engine had already done the classification. `human_response_model`
(`game/interaction.rs`) matches all 127 variants with **no catch-all arm**, so
coverage is compiler-enforced rather than sampled, and collapses them onto 20
response models. `opportunity_for_slot` then renders each as one of the 12
`InteractionResponseSpec` variants — the whole enum, so the projector is total
over it. Reading it end to end also settles what looked like it needed runtime
observation: almost every spec field is a source constant (`confirm` is
literally `Explicit` at every site but `Select`), and what varies is numeric
bounds and candidate lists, which an adapter passes through rather than
reproduces.
So the wildcard arm now routes to the projection instead of refusing. Decisions
the engine renders as a finite `ExactChoices` list map generically onto
`ChooseFromSelection` — a pre-materialized candidate list is exactly that
family's shape, so the mapping is total and needs no per-variant judgement.
Measured over the production half of the file: **52 of the 85 map this way**,
taking prompt coverage from 42/127 to 94/127. The remaining 33 are
schema-valued (19 `Select`, 6 `TargetSequence`, 3 `AmountAssignments`, and six
singletons); their response space is unbounded, no single family carries their
bounds, and they still fail closed under a declared code rather than being
flattened into a selection that would lose them.
What makes this work without the adapter judging mechanics is that
`InteractionChoice.surfaces` carries payload shape as *data* —
`Object { name, zone, controller, power, tapped }`, `Player { seat }`,
`Value { .. }`. `choice_label` joins every naming surface rather than taking
the first, because choices can share an object and differ only in a `Value`:
the priority projection offers auto- and manual-payment casts of one spell that
way, and a first-surface label would render them identically.
Engine side, `resolve_interaction_response` is a non-mutating sibling of
`submit_interaction`, which now delegates to it — so the two cannot drift.
The adapter needs the materialization without the application, because it hands
a `GameAction` back to its caller. It must not re-derive that mapping:
`materialize_response` is exhaustive over `HumanResponseModel`, so an external
reimplementation would keep compiling while silently going stale. Authorization
is unchanged — `slot_for_submission` still authenticates the actor, so dropping
the mutation does not turn this into a way to read another seat's decision.
Two failure modes are deliberately closed rather than papered over:
* A viewer can be the authorized submitter for more than one interaction
slot — a decision both seats owe at once. The wire carries one prompt, so
serving `.first()` would answer one seat and silently drop the other. Both
the prompt and the response path require exactly one opportunity.
* The response gate cannot be a `WaitingFor` list for this family, and must
not be the projection either: `WaitingFor::Priority` also projects a finite
list, so gating on that alone would accept a `chooseFromSelection` answer
to a `chooseAction` prompt — a straight breach of the documented rule that
a response is valid only if its family matches the open prompt. It instead
rebuilds the prompt and checks the family, so it cannot drift from the
builder because it *is* the builder.
`PreparedManabrewSnapshot` gains the projection because it is derivable only
from raw state, which `build_prompt_input` does not have — `derive_viewer_-
interaction` reads authorization from the authoritative state and every
presentation surface from the filtered one.
Verified by running, not by inference: 77/77 `manabrew-compat` tests pass and
`cargo clippy -p manabrew-compat` exits 0, both in an isolated
`CARGO_TARGET_DIR` because **no Tilt resource runs this crate's tests** —
`test-engine` is `-p engine` and `test-ai` is `-p phase-ai`, so a green board
would have been silence, not evidence. The engine half is Tilt-green with
freshness confirmed against file mtimes.
The new test uses `TopOrBottomChoice` because it is genuinely unmapped and its
choices differ only by a `Value` surface, so it also pins the label logic. Its
non-vacuity guard is the same waiting state with no bound interaction
authority, which must still be unsupported — otherwise the first test could be
passing for an unrelated reason.
matthewevans
enabled auto-merge
July 27, 2026 18:31
Contributor
|
Warning Review limit reached
Next review available in: 8 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe engine adds non-mutating interaction response resolution. Manabrew compatibility snapshots now include viewer interaction projections, generate generic selection prompts, translate selections through the engine, and validate related prompt capabilities with expanded tests. ChangesInteraction resolution and prompting
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ManabrewCompat
participant PreparedManabrewSnapshot
participant interaction_prompt
participant resolve_interaction_response
participant GameState
ManabrewCompat->>PreparedManabrewSnapshot: prepare viewer interaction projection
PreparedManabrewSnapshot->>interaction_prompt: build generic selection prompt
interaction_prompt-->>ManabrewCompat: return ChooseFromSelection
ManabrewCompat->>resolve_interaction_response: translate chosen indices
resolve_interaction_response->>GameState: validate and materialize GameAction
GameState-->>ManabrewCompat: return resolved action
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
…t path The projection path handled one-of lists (`ExactChoices`). This adds the other half of the same idea: `Select`, a subset choice over the same kind of candidate list, differing only in how many entries may be taken. That difference is exactly what `ChooseFromSelection` already carries. `SelectionConstraint::Count` and `EngineValidatedCount` both bound a count, so they become `min_total`/`max_total` verbatim instead of the one-of path's hardcoded 1/1. `EngineValidatedCount` is treated identically on purpose: the extra legality the engine reserves to itself is rechecked on submit and is not expressible to a client either way, so advertising the count is the whole of what this family can honestly say. `SelectionConstraint::Aggregate` is not folded in. Its bound is a sum over a chosen attribute of the selected objects — "keep permanents with total power 4 or less" — not a number of objects, so no count is equivalent to it. Rendering it as an unbounded count would advertise illegal selections as legal, which is worse than refusing, so it fails closed under its own code. The suggested extension is cheap and worth recording: `SelectionOption` already carries `weight`, so the wire is one comparator and one amount away from expressing this without a new family. The response variant is not interchangeable with the spec — the engine rejects a `Choose` submitted against a `Select` schema as malformed, and vice versa — so the answer path now mirrors whichever shape the prompt rendered. Count bounds are deliberately not rechecked adapter-side: the engine owns them and rejects a violating submission, and a second check here would be a drifting authority on the same constraint. Measured over the production half of the file, prompt coverage goes from 94/127 `WaitingFor` variants to **113/127**. The 14 that remain are `TargetSequence` (6), `AmountAssignments` (3), and five singletons; each needs a family whose payload is not a count over a candidate list at all — an ordering, an amount distribution, a partition — so none of them belongs in this family. Note the census counts by response *model*: a `Select` whose runtime constraint is `Aggregate` still fails closed, so 113 is an upper bound. The new test uses `DiscardToHandSize` (CR 514.1) because it is the clearest subset choice — discard exactly `count` of the cards in hand — so it pins both things that distinguish this path from the one-of path: that the engine's bounds survive into the prompt, and that the answer goes back as a subset. 77 -> 78 tests, all passing, with `cargo clippy -p manabrew-compat` clean — run in an isolated `CARGO_TARGET_DIR`, since no Tilt resource executes this crate's tests.
Parse changes introduced by this PR✓ No card-parse changes detected. |
![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: 1
🤖 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 `@crates/manabrew-compat/src/lib.rs`:
- Around line 2470-2490: Update choice_label’s match over
InteractionPresentationSurface to remove the wildcard arm and make the match
exhaustive. Preserve the existing Object, Value, and Player label behavior, and
add explicit handling for every remaining current variant, choosing the
appropriate label behavior so future variants trigger a compile error until
handled.
🪄 Autofix (Beta)
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: b28f15bf-d8a7-4bdc-a9ab-b54702f3a30d
📒 Files selected for processing (3)
crates/engine/src/game/interaction.rscrates/engine/tests/integration/interaction_contract.rscrates/manabrew-compat/src/lib.rs
Comment on lines
+2470
to
+2490
| fn choice_label(choice: &InteractionChoice) -> String { | ||
| let parts = choice | ||
| .surfaces | ||
| .iter() | ||
| .filter_map(|surface| match surface { | ||
| InteractionPresentationSurface::Object { | ||
| name, reference, .. | ||
| } => Some(name.clone().unwrap_or_else(|| reference.clone())), | ||
| InteractionPresentationSurface::Value { value, .. } => Some(value.clone()), | ||
| InteractionPresentationSurface::Player { seat, .. } => Some(format!("Player {seat}")), | ||
| _ => None, | ||
| }) | ||
| .collect::<Vec<_>>(); | ||
| if parts.is_empty() { | ||
| // No naming surface at all. The opaque id is a poor label but a correct | ||
| // one; it is never empty, so the option stays distinguishable. | ||
| choice.id.as_str().to_string() | ||
| } else { | ||
| parts.join(" — ") | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -e rs . crates/engine/src/types | xargs rg -n -A 20 'enum InteractionPresentationSurface'Repository: phase-rs/phase
Length of output: 1642
🏁 Script executed:
#!/bin/bash
sed -n '688,760p' crates/engine/src/types/interaction.rsRepository: phase-rs/phase
Length of output: 1993
Remove the wildcard from choice_label
Object/Value/Player are handled explicitly, but _ => None will silently skip any new InteractionPresentationSurface variant; make this match exhaustive so new surfaces fail to compile until their label behavior is decided.
🤖 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 `@crates/manabrew-compat/src/lib.rs` around lines 2470 - 2490, Update
choice_label’s match over InteractionPresentationSurface to remove the wildcard
arm and make the match exhaustive. Preserve the existing Object, Value, and
Player label behavior, and add explicit handling for every remaining current
variant, choosing the appropriate label behavior so future variants trigger a
compile error until handled.
Source: Path instructions
The generic path so far emitted one family, because both shapes it handled
were a choice over candidates. `Number` is the first that is not: a range with
no candidate list at all, which `ChooseNumber` carries verbatim.
Handled before the candidate branches precisely because its candidate list is
empty by construction — the emptiness guard that protects the selection path
would otherwise reject it.
The answering action is the engine's to name, and this is where the bespoke arm
would have been wrong. `GameAction::ChooseX` is specific to X (CR 107.3); the
one unmapped numeric pause is `PayAmountChoice` (CR 107.14, pay any amount of
`{E}`), which the engine answers with `SubmitPayAmount`. So the arm now
dispatches `ChooseXValue` to `ChooseX` as before and routes everything else
through the projection, which resolves it to whatever action the engine
actually names. The test asserts `SubmitPayAmount { amount: 2 }` rather than
merely "not ChooseX", so a regression to the wrong action fails loudly.
Value bounds are deliberately not rechecked adapter-side, for the same reason
the selection path does not recheck counts: the engine owns the range and
rejects a violating submission, and a second check here would be a drifting
authority on the same constraint.
Two pieces of shared structure fall out and are factored rather than copied:
* `sole_open_opportunity` — every generic response path needs the lone open
interaction, and the "exactly one" rule is the same guard for all of them.
* `open_prompt` — the gate for every family the generic path can emit. Those
families have no fixed `WaitingFor` list, so the gate rebuilds the prompt
and reads its family; it cannot drift from the builder because it *is* the
builder. `ChooseNumber` now joins `ChooseFromSelection` in using it, with
the bespoke `ChooseXValue` check kept as the primary so shipped behaviour
for X is untouched.
Prompt coverage: 113/127 -> **114/127**. The 13 that remain are
`TargetSequence` (6), `AmountAssignments` (3), and singletons for sideboarding,
category choice, and the two shortcut families. None is a count over a list or
a scalar; each needs a family whose payload is an ordering, a distribution, or
a partition, so none belongs in the two shapes already served.
Also corrects an orphaned doc comment: an earlier edit left
`/// The output's family tag, for diagnostics.` stranded above the wrong item.
79 tests passing, `cargo clippy -p manabrew-compat` clean, both in an isolated
`CARGO_TARGET_DIR` since no Tilt resource runs this crate's tests.
`Sequence` is an ordered subset of the same candidate list `Select` draws from, and `ChooseFromSelection` already carries order: `chosen_indices` is a `Vec`, not a set, so the sequence the client sends survives to the engine, which fills its target slots in exactly that order. It answers with `InteractionResponse::Sequence`, not `Select` — the engine rejects the wrong variant as malformed — so the response path mirrors whichever shape the prompt rendered, as it already does for the one-of case. Fidelity gap recorded rather than hidden: this family cannot *tell* the client that order is significant; it renders as a plain selection. `Reorder` is not a substitute, because it orders the whole list while a target sequence is usually a proper subset. The test reverses the offered order on purpose. That is the entire assertion — an implementation that collected indices into a set, or sorted them, would hand the engine the targets the other way round and fail. It uses `ProliferateChoice` (CR 701.27), which also pins that a zero minimum reaches the prompt intact instead of being coerced to the one-of path's 1. Prompt coverage: 114/127 -> **120/127**. The 7 that remain are `AmountAssignments` (3), and singletons for sideboarding, category choice, and the two shortcut families. Each needs a payload that is not a count over a list, an order over a list, or a scalar — a per-candidate distribution, a grouped partition, or a shortcut reply — so none belongs in the shapes served here. The registry entry now names the two protocol shapes that would close most of them. 80 tests passing, `cargo clippy -p manabrew-compat` clean with zero warnings, both in an isolated `CARGO_TARGET_DIR` since no Tilt resource runs this crate's tests.
andriypolanski
pushed a commit
to andriypolanski/phase
that referenced
this pull request
Jul 27, 2026
…rs#6704) The guide claimed 78 declared / 65 emitted. Four codes were added when the generic interaction path shipped (phase-rs#6700) — simultaneous-decisions, schema-response, and aggregate-bound refusals, plus a narrowed prompt-unsupported — so the live figures are 82 and 69. The 13 documentary-only entries are unchanged, as is the invariant that matters: zero emitted codes are undeclared. Recomputed against this tree rather than carried over from the earlier count: declared 82 · emitted 69 · documentary-only 13 · undeclared emitted 0 which also agrees with the array's own type, `[UnsupportedCapability; 82]`. Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
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.
The adapter named 42 of the engine's 127
WaitingForvariants; the other 85fell through to
local.prompt-unsupported. Closing them one at a time wouldhave meant 85 hand-written mappings, each wiring three sites and re-deriving
min/max and constraint logic the engine already computes — duplicated game
logic inside a serialization boundary.
The engine had already done the classification.
human_response_model(
game/interaction.rs) matches all 127 variants with no catch-all arm, socoverage is compiler-enforced rather than sampled, and collapses them onto 20
response models.
opportunity_for_slotthen renders each as one of the 12InteractionResponseSpecvariants — the whole enum, so the projector is totalover it. Reading it end to end also settles what looked like it needed runtime
observation: almost every spec field is a source constant (
confirmisliterally
Explicitat every site butSelect), and what varies is numericbounds and candidate lists, which an adapter passes through rather than
reproduces.
So the wildcard arm now routes to the projection instead of refusing. Decisions
the engine renders as a finite
ExactChoiceslist map generically ontoChooseFromSelection— a pre-materialized candidate list is exactly thatfamily's shape, so the mapping is total and needs no per-variant judgement.
Measured over the production half of the file: 52 of the 85 map this way,
taking prompt coverage from 42/127 to 94/127. The remaining 33 are
schema-valued (19
Select, 6TargetSequence, 3AmountAssignments, and sixsingletons); their response space is unbounded, no single family carries their
bounds, and they still fail closed under a declared code rather than being
flattened into a selection that would lose them.
What makes this work without the adapter judging mechanics is that
InteractionChoice.surfacescarries payload shape as data —Object { name, zone, controller, power, tapped },Player { seat },Value { .. }.choice_labeljoins every naming surface rather than takingthe first, because choices can share an object and differ only in a
Value:the priority projection offers auto- and manual-payment casts of one spell that
way, and a first-surface label would render them identically.
Engine side,
resolve_interaction_responseis a non-mutating sibling ofsubmit_interaction, which now delegates to it — so the two cannot drift.The adapter needs the materialization without the application, because it hands
a
GameActionback to its caller. It must not re-derive that mapping:materialize_responseis exhaustive overHumanResponseModel, so an externalreimplementation would keep compiling while silently going stale. Authorization
is unchanged —
slot_for_submissionstill authenticates the actor, so droppingthe mutation does not turn this into a way to read another seat's decision.
Two failure modes are deliberately closed rather than papered over:
slot — a decision both seats owe at once. The wire carries one prompt, so
serving
.first()would answer one seat and silently drop the other. Boththe prompt and the response path require exactly one opportunity.
WaitingForlist for this family, and mustnot be the projection either:
WaitingFor::Priorityalso projects a finitelist, so gating on that alone would accept a
chooseFromSelectionanswerto a
chooseActionprompt — a straight breach of the documented rule thata response is valid only if its family matches the open prompt. It instead
rebuilds the prompt and checks the family, so it cannot drift from the
builder because it is the builder.
PreparedManabrewSnapshotgains the projection because it is derivable onlyfrom raw state, which
build_prompt_inputdoes not have —derive_viewer_- interactionreads authorization from the authoritative state and everypresentation surface from the filtered one.
Verified by running, not by inference: 77/77
manabrew-compattests pass andcargo clippy -p manabrew-compatexits 0, both in an isolatedCARGO_TARGET_DIRbecause no Tilt resource runs this crate's tests —test-engineis-p engineandtest-aiis-p phase-ai, so a green boardwould have been silence, not evidence. The engine half is Tilt-green with
freshness confirmed against file mtimes.
The new test uses
TopOrBottomChoicebecause it is genuinely unmapped and itschoices differ only by a
Valuesurface, so it also pins the label logic. Itsnon-vacuity guard is the same waiting state with no bound interaction
authority, which must still be unsupported — otherwise the first test could be
passing for an unrelated reason.
Summary by CodeRabbit
New Features
Bug Fixes
Tests