Fixes#121
Merged
Merged
Conversation
…ions [codex] Add Lorcana triage regression coverage
The engine→UI converter had a heuristic in `countAutoResolvedSlots` that assumed the leading slot was always auto-bound to the source card when `maxSelections < totalSlots`. That assumption only holds for `from: SELF, to: CHOSEN` effects (Nero). For `from: CHOSEN, to: SELF` effects (Luisa Madrigal "I Can Take It", Luisa No Pressure, Isabela "Feel Better") the direction is inverted: the chooser fills FROM and the engine auto-binds TO to the source. The heuristic flipped the slots, producing a slotted submission whose FROM matched the source card — which the FROM-DSL filtered out via `excludeSelf: true` — so the engine silently no-op'd and the pending effect remained on the stack, looping the UI. Three open-bug clusters trace back to this seam (~30+ Luisa reports alone in the latest week). The fix surfaces the auto-resolved slot keys explicitly from the engine on `TargetResolutionSelectionContext.autoResolvedSlots`, and rewrites the converter's slot/payload builders to read those keys instead of guessing. The fallback (for snapshots that predate the field) now picks the trailing slots — matching the dominant Lorcana descriptor shape — rather than the leading slots. - engine/types/resolution-selection: add `autoResolvedSlots?` field - engine/runtime-moves/.../selection-context: derive `["from"]` or `["to"]` by inspecting `effect.from` / `effect.to` for the literal `"SELF"` and emit the field on `move-damage` contexts - interaction/build-player-interaction-view: replace `countAutoResolvedSlots`'s leading-slot heuristic with `getAutoResolvedSlotIndices`; update `buildSlots`, `buildTargetPayload`, `buildTargetSubmissionPayload`, `computeActiveSlotIndex`, `filterCardCandidatesForActiveSlot` to index by chooser-slot position instead of by "after auto-resolved" - tests: replace the misleading "marks slot 0 as auto-resolved when maxSelections < totalSlots" test with two explicit cases (`['from']` and `['to']`); add Luisa regression covering the slotted submission payload; add engine-side coverage that `deriveAutoResolvedSlots` correctly emits `["to"]` for `from: CHOSEN, to: SELF` and `["from"]` for the inverse Verified: lorcana-engine (1007 pass), lorcana-interaction (52 pass), lorcana-simulator (651 pass), lorcana-cards Luisa+Isabela suites (13 pass), and check-types across the four affected packages clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Classify all 1551 open bug reports into 19 engineering states and emit a per-cluster worksheet so humans can drive the report queue to zero. - `bug_reports.md`: raw export (1551 rows, 2026-03-24 .. 2026-05-16). - `bug_reports_triage.md`: read-only state per report. - `bug_reports_pending.md`: 240 card clusters + 6 UI clusters + 558 singletons with checkbox blanks (Reproduced / Not reproducible / WAI / Already fixed / Duplicate / Out of engine / User error) plus notes and resolution fields. - `.triage/`: deterministic Python pipeline (parse -> cluster -> classify -> render/pending) plus the intermediate JSONL artifacts and a README. State distribution: 227 likely fixed-in-engine, 1 fixed-in-card, 4 covered- by-test, 6 no-bug-confirmed, 14 non-bug comments, 3 vague, 52 out-of-engine- scope (network/replay UI/matchmaking/undo/skip/timer/chat/client-crash), and 1247 pending across 240 card clusters and 558 singletons. Cross-checked against the human-written daily triage doc at packages/lorcana/lorcana-engine/docs/daily-feedback-replay-triage-2026-05-15.md (22 reports): all 21 in-dataset reports landed in compatible states. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Hand-test fixture for PR #114. Loads at `/tests/triage-2026-05-16-luisa-and-isabela-feel-better` and seats both affected cards on the same board with damaged ally sources, so a reviewer can confirm both prompts now resolve: - Luisa Madrigal — Confident Climber (activated `I CAN TAKE IT`, `from: CHOSEN_CHARACTER_OF_YOURS, to: SELF`) - Isabela Madrigal — Perfectly in Control (triggered `FEEL BETTER` on play and on quest, same direction) Pre-fix, the engine→UI converter assumed the auto-bound slot was always FROM and silently dropped both prompts. With the fix the prompt asks for a damaged character of yours and the damage moves onto the chooser. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…e games - `createLiveEntries` now returns one row per engine log (not one per dispatch), so triggered abilities and auto-resolved sub-effects each get their own entry - Sub-effect rows use `log.playerId` for actor attribution instead of the dispatching player's id, fixing the "wrong player" label on opponent triggers - `createSpectatorHistoryEntries` switches from `find` to `filter` so all matching engine logs per stateVersion are expanded in the history path too - `game-mode-message-router` simplifies actorId extraction to fall back to the first engine log's playerId rather than the potentially-wrong moveType-matched log (which could point to a different player) - Both `HvHPlayerOrchestrator` and `SpectatorMatchOrchestrator` updated to use `createLiveEntries` directly; `createLiveEntry` kept as a deprecated wrapper Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Introduce --open and --fork flags that bridge CLI-based triage to the simulator's replay watcher and fork views, so agents can move from trace output to interactive in-browser verification without manually constructing URLs. - Add open-replay module: buildReplayUrl, getOpenCommand, openInBrowser - Extend CLI arg parser with --open, --fork, --side, --base-url flags - Add 9 unit tests for URL building and platform-specific open commands - Update replay-debugging skill docs with browser-mode usage and examples - Add Step 3.5 to triage-player-report: when to move from CLI to browser - Add README for replay-cli package
…arden Add browser mode to tcg-replay CLI for in-browser replay triage
fix: update vite-plus to 0.1.21
…-17-fixes [codex] Fix daily feedback triage issues
…-2026-05-16 Add bug-report triage worksheet for 1551 open reports
…irection Fix move-damage slot direction for to: SELF prompts (Luisa, Isabela)
…8c972 Fix event log: show sub-effects and correct player attribution in live games
…erges Fix main CI after merged PRs
Contributor
|
Mention Blocks like a regular teammate with your question or request: @blocks review this pull request Run |
|
Claude finished @eduardomoroni's task in 2m 10s —— View job Code Review: PR #121 - Fixes
📋 Review SummaryThis PR delivers critical bug fixes for the Lorcana simulator, addressing a high-impact UI/engine routing bug affecting ~30+ user reports for cards like Luisa Madrigal and Isabela. Additionally, it includes comprehensive triage infrastructure, daily feedback fixes, and regression coverage. The changes are well-tested and thoughtfully designed. Overall Assessment: The core fixes are sound and well-tested. The code quality is high with good documentation. This PR successfully addresses several player-reported issues. 🔍 General FeedbackPositive Aspects
Areas for Consideration
🎯 Detailed Code Review1. Move-Damage Slot Direction Fix (Critical)Files: The fix correctly addresses the root cause: the previous heuristic assumed leading slots were auto-resolved, which failed for Strengths:
Observation: The fix is complete and well-tested. The change from 2. Sacrifice Cost FixFile: case "sacrifice": {
// Banish the sacrifice target (item) as the cost of playing this card
const sacrificeTargetId = (
"sacrificeTarget" in params ? params.sacrificeTarget : undefined
)!;
banishAsAbilityCost(ctx, {
cardId: sacrificeTargetId,
sourceId: cardId,
playerId: currentPlayer,
});Strengths:
Test Coverage: The Belle - Apprentice Inventor test (lines 50-76) properly validates that Ingenious Device's TIME GROWS SHORT triggers when sacrificed. 3. Event Log Attribution FixFiles: The fix ensures sub-effects get their own log entries with correct player attribution by using Observation: This is a correct fix for the "wrong player" label issue on opponent triggers. 4. Triage InfrastructureDirectory: The Python pipeline (parse → cluster → classify → render) is comprehensive and deterministic. Strengths:
Consideration: This is production infrastructure that will need maintenance as card sets and bug patterns evolve. 5. Daily Feedback Fixes (2026-05-17)File: Hiram Flaversham Fix (lines 40-63):
Black Cauldron Fixes (lines 65-100+):
🔵 Low Severity Observations
✅ No Issues FoundAfter thorough analysis of the diff:
The PR successfully addresses its stated goals with high-quality implementations. |
There was a problem hiding this comment.
Pull request overview
This PR bundles together a heterogeneous batch of fixes and tooling for the Lorcana simulator. It pins the vite/vitest/vite-plus catalog versions, ships an --open/--fork browser-launch mode for the replay CLI, refactors slotted move-damage handling so engine-provided autoResolvedSlots drives the UI (fixing Luisa-style to: SELF effects), broadens spectator log handling so auto-resolved sub-effects produce their own entries, validates illegal choice indices on playCard, lets put-under feed the discard-exit metric (Black Cauldron → Horned King), and adds a large set of triage docs, fixture-driven test routes, and a Python triage pipeline under .triage/.
Changes:
- Engine: slot-aware auto-resolve via
autoResolvedSlots, illegal-choice validation inplayCard, discard-exit recording in put-under, play-from-under availability fixes, Hiram Flaversham "if you do" gating. - Simulator: spectator/HvH orchestrators emit one snapshot per engineLog; new
createLiveEntriesAPI; new browser-mode replay CLI; new triage fixtures and routes. - Docs/tooling: triage reports for 2026-05-15 and 2026-05-17, Python triage pipeline under
.triage/, updated replay-debugging skill and triage command.
Reviewed changes
Copilot reviewed 47 out of 55 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-workspace.yaml | Pin vite/vitest catalog versions to 0.1.21 |
| packages/tools/replay-cli/src/open-replay.ts | New URL builder + OS-specific browser launcher |
| packages/tools/replay-cli/src/cli.ts | Add --open/--fork/--side/--base-url flags |
| packages/tools/replay-cli/README.md | Document trace vs browser mode |
| packages/tools/replay-cli/tests/open-replay.test.ts | Unit tests for URL builder and OS commands |
| .../routes/tests/triage-2026-05-16.../+page.ts/+page.svelte | New harness route for Luisa/Isabela fixture |
| .../game-mode-message-router.ts | Simplify actorId fallback to first engineLog playerId |
| .../spectator-match-orchestrator.svelte.ts | Emit per-engineLog snapshots; new createLiveEntries |
| .../simulator-devtools/routes/test-routes.test.ts | Cover new 2026-05-17 triage fixture routes |
| .../fixtures/triage-2026-05-17-remaining.ts | 12 new visual-validation fixtures |
| .../fixtures/triage-2026-05-16-luisa-and-isabela-feel-better.ts | New Luisa/Isabela fixture |
| .../fixtures/index.ts | Register the new fixtures |
| .../hvh/hvh-player-orchestrator.svelte.ts | Switch to createLiveEntries |
| .../build-player-interaction-view.ts | Use engine-supplied autoResolvedSlots; trailing-slot fallback |
| .../build-player-interaction-view.test.ts | New tests for to: SELF and from: SELF slot routing |
| .../types/resolution-selection.ts | Add autoResolvedSlots field |
| .../action-effects/selection-context.ts | Derive autoResolvedSlots; conditional unwrap in choice legality |
| .../action-effects/selection-context.test.ts | Tests for derived autoResolvedSlots |
| .../action-effects/put-under-effect.ts | Record discard-exit when moving cards under |
| .../action-effects/composed-effect-resolver.ts | Add getLegalChoiceOptionIndices; defensive check |
| .../moves/core/play-card.ts | Validate choiceIndex against legal options |
| .../lorcana-engine-base.ts | Consider play-from-under cards in available moves |
| docs/daily-feedback-* and fix-session docs | Triage reports for 2026-05-15 and 2026-05-17 |
| Card tests (firefly-swarm, mickey-snowboard-ace, black-cauldron, horned-king, rapunzel, minnie, hiram-flaversham) | New regression coverage |
| 149-hiram-flaversham-toymaker.ts | Gate draw 2 behind "if you do" conditional |
| .triage/*.py, .triage/README.md | Python triage pipeline (hardcoded paths) |
| .ai_memory/*, .agents/skills/replay-debugging/SKILL.md, .claude/commands/triage-player-report.md | Workflow docs |
Comments suppressed due to low confidence (1)
packages/lorcana/lorcana-simulator/src/routes/matches/[matchId]/games/[gameId]/modes/game-mode-message-router.ts:208
- The removed fallback used to look up the engineLog whose
typematched the WSmoveType(e.g. matching"inkCard"engine logs to aputCardIntoInkwellWS message). The new code simply picks the first engineLog with aplayerId. If state_update broadcasts include sub-effect logs from a different actor before the primary log, or if the engine log order is not guaranteed to put the dispatcher's log first, this can mis-attribute theactorId. Worth confirming the engine guarantees the first log inengineLogsis always the dispatcher's action — the new comment asserts this, but the previous code went to lengths to avoid relying on that assumption.
| url: string, | ||
| ): { command: string; args: string[] } { | ||
| if (platform === "darwin") return { command: "open", args: [url] }; | ||
| if (platform === "win32") return { command: "cmd", args: ["/c", "start", "", url] }; |
| from pathlib import Path | ||
| from datetime import datetime | ||
|
|
||
| ROOT = Path('/Users/wazar/projects/lorcana-simulator') |
Comment on lines
+292
to
+311
| export function createLiveEntry( | ||
| args: Parameters<typeof createLiveEntries>[0], | ||
| ): MoveLogEntrySnapshot { | ||
| const entries = createLiveEntries(args); | ||
| if (entries[0]) return entries[0]; | ||
| // Absolute fallback: blank entry so callers expecting a non-null value don't crash. | ||
| const resolveCard = buildCardReferenceResolver(args.engine, args.cardsMaps); | ||
| const moveId = isLorcanaSimulatorMoveId(args.acceptedMove.moveId) | ||
| ? args.acceptedMove.moveId | ||
| : "passTurn"; // safe default — will render as an empty/unknown row | ||
| const entry: MoveLogEntrySnapshot = { | ||
| // stateVersion is stable across move_accepted (unicast to issuer) and | ||
| // state_update (broadcast to everyone) for the same move; using it for | ||
| // the dedup id prevents duplicate entries on the dispatcher's screen. | ||
| // Manual moves emit no engine logs, so buildAcceptedMove falls back to | ||
| // Date.now() per packet — that wall-clock differs between the two | ||
| // arrivals and would otherwise sneak past pushEntries' Set-based dedup. | ||
| id: `spectator-live-${args.acceptedMove.stateVersion}-${moveId}`, | ||
| timestamp: args.acceptedMove.timestamp, | ||
| turnNumber: args.acceptedMove.turnNumber, | ||
| moveId, | ||
| actorSide: args.resolveActorSide(args.acceptedMove.actorId), | ||
| title: "", | ||
| playerId: args.acceptedMove.actorId, | ||
| params, | ||
| typedLogEntry: strippedLog as MoveLogEntrySnapshot["typedLogEntry"], | ||
| params: undefined, | ||
| typedLogEntry: undefined, |
Comment on lines
+411
to
+419
| function countChooserSlotsBefore(autoResolvedIndices: readonly number[], slotIndex: number): number { | ||
| let count = 0; | ||
| for (let i = 0; i < slotIndex; i += 1) { | ||
| if (!autoResolvedIndices.includes(i)) { | ||
| count += 1; | ||
| } | ||
| } | ||
| return count; | ||
| } |
Comment on lines
+557
to
+571
| let effectiveOption = option; | ||
| if (optionRecord?.type === "conditional") { | ||
| const conditionMet = evaluateActionCondition( | ||
| optionRecord.condition as never, | ||
| args.ctx as never, | ||
| args.cardPlayed, | ||
| args.resolutionInput, | ||
| ); | ||
| effectiveOption = conditionMet | ||
| ? (optionRecord.then ?? optionRecord.effect ?? optionRecord.ifTrue) | ||
| : (optionRecord.else ?? optionRecord.ifFalse); | ||
| if (!effectiveOption) { | ||
| return []; | ||
| } | ||
| } |
Comment on lines
+1592
to
+1618
| for (const ability of actionEffects) { | ||
| const effectRecord = ability.effect as unknown as Record<string, unknown> | undefined; | ||
| if (!effectRecord || effectRecord.type !== "choice") { | ||
| continue; | ||
| } | ||
| const options = Array.isArray(effectRecord.options) | ||
| ? effectRecord.options | ||
| : Array.isArray(effectRecord.choices) | ||
| ? effectRecord.choices | ||
| : []; | ||
| if (params.choiceIndex >= options.length) { | ||
| return fail("choiceIndex is out of range", "INVALID_CHOICE_INDEX", cardDef); | ||
| } | ||
| const legalChoiceIndices = getLegalChoiceOptionIndices( | ||
| ctx, | ||
| cardPlayedPayload, | ||
| effectRecord as never, | ||
| actionResolutionInput, | ||
| ); | ||
| if (!legalChoiceIndices.includes(params.choiceIndex)) { | ||
| return fail( | ||
| "Selected choice option is not currently legal", | ||
| "ILLEGAL_CHOICE_OPTION", | ||
| cardDef, | ||
| ); | ||
| } | ||
| } |
| ); | ||
| if (moved) { | ||
| movedCardId = selectedCardId; | ||
| recordDiscardExitThisTurn(ctx); |
Comment on lines
+4473
to
+4495
| const hasPlayablePlayFromUnderCard = (() => { | ||
| const currentTurn = this.getState().ctx.status.turn ?? 1; | ||
| const permissions = getActivePlayFromUnderPermissions( | ||
| this.getState().G.playFromUnderPermissions, | ||
| clientPlayerId as PlayerId, | ||
| currentTurn, | ||
| ); | ||
| for (const permission of permissions) { | ||
| const sourceItemMeta = this.getState().ctx.zones.private.cardMeta[permission.sourceItemId]; | ||
| const underCardIds = Array.isArray(sourceItemMeta?.cardsUnder) | ||
| ? (sourceItemMeta.cardsUnder as CardInstanceId[]) | ||
| : []; | ||
| if (underCardIds.some((cardId) => this.canPlayCard(cardId))) { | ||
| return true; | ||
| } | ||
| } | ||
| return false; | ||
| })(); | ||
|
|
||
| const shouldAnalyzePlayCards = | ||
| legalMoveIds.includes("playCard") || | ||
| playerBoard.hand.some((cardId) => this.canPlayCard(cardId as CardInstanceId)); | ||
| playerBoard.hand.some((cardId) => this.canPlayCard(cardId as CardInstanceId)) || | ||
| hasPlayablePlayFromUnderCard; |
| import { buildReplayUrl, openInBrowser, type ForkSide, type OpenMode } from "./open-replay"; | ||
|
|
||
| const DEFAULT_API_ORIGIN = "https://api.tcg.online"; | ||
| const DEFAULT_BASE_URL = "http://localhost:5173"; |
Code Review Roast 🔥Verdict: No New Issues Found | Recommendation: Merge This PR is primarily a dependency bump with some card fixes. All the substantive issues I found were already caught by previous reviews (as shown in the existing comments table). 🏆 Best part: The 💀 Worst part: The daily feedback doc claims "replay was not available, but the interaction was rules-backed" — either get the replay or call it a speculative improvement, but don't wave your hands at missing evidence. 📊 Overall: Like a well-tuned engine — the parts that matter are in order, and the rough edges have already been sanded down. Files Reviewed (24 files)
Reviewed by laguna-m.1-20260312:free · 2,119,999 tokens |
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
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.
No description provided.