--- phase: 6 reviewers: [codex] reviewed_at: 2026-05-11T01:30:00Z scope: pending-only (gap-closure plans 06-10..06-17) plans_reviewed: - 06-10-PLAN.md - 06-11-PLAN.md - 06-12-PLAN.md - 06-13-PLAN.md - 06-14-PLAN.md - 06-15-PLAN.md - 06-16-PLAN.md - 06-17-PLAN.md skipped_reviewers: - claude (self — invoked from Claude Code; would not be independent) unavailable_reviewers: - gemini, coderabbit, opencode, qwen, cursor (CLI not installed) --- # Cross-AI Plan Review — Phase 6 Gap-Closure (06-10..06-17) ## Codex Review ## Summary The 8-plan set is directionally strong and mostly targets the real UAT blockers rather than papering over symptoms. The best parts are the explicit root-cause step for reconnect, the extraction-first movement/assets approach, and final human UAT as a hard gate. Main risk is execution size: 06-14 and 06-16 are large, cross-cutting plans with enough unknowns in extracted asset/room formats to threaten schedule and introduce regressions. I would approve the sequence with revisions before execution. ## Strengths - The sequencing is mostly sound: quick UX unblockers first, diagnosis before reconnect fix, numeric movement before sprite-state, assets before sprite-state, UAT last. - D-25 is handled correctly: 06-12 isolates root cause before 06-15 changes code. - The plans preserve the core architecture: server-authoritative, bearer WS auth unchanged, no client position trust. - Requirements traceability is considered throughout, including impl/unit/int/doc tags. - 06-17 correctly treats CLI-08 as a human acceptance milestone, not just automated green checks. - 06-13 and 06-16 correctly use `extracted/client-5-8/objects/0000-server/` as canonical movement/player behavior source. ## Concerns ### HIGH — 06-14 is too broad and likely under-researched 06-14 combines BMP bootstrap, atlas build, BNCentral room conversion, room signing, server registration, client room rendering, player rendering, and e2e changes. Any mismatch in extracted `meta.json`, `tiles.json`, background format, object-to-sprite resolution, or signing helper could block the whole plan. Concrete risk: the plan assumes `meta.json` fields like `frame_w`, `frame_h`, `origin_x`, `origin_y`, and room/layout fields without proving the extracted schema. If these names differ, the pasted implementation path fails. ### HIGH — 06-14 may violate "narrow gap closure" Replacing synthetic `mvp-lobby` with full BNCentral is product-correct, but it is not a small patch. It may pull in collision, invisible logic objects, background namespaces, tile dimensions, object sprite resolution, and spawn semantics. That is a lot of Phase 7-shaped work inside Phase 6. Suggestion: define an MVP BNCentral slice explicitly: render background + walkable floor + enough walls for visual identity, but defer non-player object rendering unless required for CLI-08. ### HIGH — 06-15 dependency metadata is incomplete 06-15 front matter depends only on `06-12`, but in the actual wave model it runs after 06-14. Its e2e tests will execute against the current GameScene/room path, which after Wave 10 should be `bn-central`. Encode that dependency explicitly as `depends_on: [06-12, 06-14]` or make the tests room-agnostic. ### MEDIUM — 06-10 contrast test may be brittle The test assumes selectors like `label[for="username"]`. The provided CSS/action text sometimes implies wrapping labels rather than `for` attributes. If the current HTML lacks `for`, the test fails for a test-shape reason. Also `#EF4444` on `#0A0E1A` should be verified against 4.5:1 before locking it. Red-on-dark often passes, but do not assume. ### MEDIUM — 06-11 "canvas click opens menu" may hurt gameplay Opening the Esc menu on any single canvas click is unusual and can interfere with future mouse interactions. The decision is user-locked, but the plan should guard harder: suppress while pointer-lock/drag/chat/force-reset/reconnect overlays are active, and consider only clicks on an inert game background if that can be detected. ### MEDIUM — logout cookie assertion may be wrong Playwright checking cookies after Better-Auth sign-out with `sessionCookies.every(c => !c.value)` can fail if expired cookies are simply absent. Better assertion: `expect(sessionCookies).toHaveLength(0)` or allow both absent and empty. Also Better-Auth may use `HttpOnly`, so `document.cookie` is not reliable; `page.context().cookies()` is the right source. ### MEDIUM — 06-13 constants may not be enough for "movement matches BNO" Movement feel may depend on collision resolution, diagonal normalization, room speed, `image_speed`, server tick cadence, keyboard repeat behavior, and GM built-ins like `move_contact_solid`, not just speed/friction/accel. The plan should explicitly include diagonal speed handling and collision-step behavior if found in `pcode_mover.gml`. ### MEDIUM — 06-16 animation timing is underspecified `deriveFrame()` increments one frame per render/update call in the draft. If Phaser runs at 60 FPS and BNO room speed is 30, animation will run 2x unless cycle advance is based on accumulated simulation ticks or `image_speed` from GML. Tie animation to simulation tick count, not renderer frame rate. ### MEDIUM — e2e canvas pixel assertions are weak Canvas sampling from Phaser WebGL is fragile. Prefer a deterministic test hook guarded by test/dev mode, e.g. `window.__rebno.localFrame`, `window.__rebno.loadedTextures`, and `data-room-id`, plus one screenshot artifact if needed. ### LOW — Trace tags may reference undeclared IDs Plans mention `[->REQ-CDOC-04]` and `[->REQ-CDOC-*]`. If those IDs are not declared in `traceable-reqs.toml`, `pnpm trace:check` will fail. Use only manifest-declared IDs. ### LOW — 06-17 assumes committing mp4 is acceptable The plan handles size, but committing video into `.planning` may bloat repo history. Prefer Git LFS by default for `.mp4`, even under 50 MB. ## Suggestions - Split 06-14 internally with explicit stop points: bootstrap sprites green, then BNCentral JSON green, then client render green. Do not proceed to the next subsection if extracted schema assumptions fail. - Add a schema-discovery pre-task to 06-14 that records actual extracted sprite/room JSON field names before implementation. - Add `06-14` as an explicit dependency for 06-15 unless tests are intentionally room-neutral. - In 06-13, add tests for diagonal movement normalization and collision behavior, or document absence from extracted GML. - In 06-16, drive animation phase from simulation ticks / BNO `image_speed`, not Phaser render frames. - In 06-11, make logout/reconnect cleanup include room listener teardown to avoid duplicate handlers after returning to LoginScene. - In 06-17, require `pnpm trace:check` and `pnpm verify:phase-6` after the UAT doc/video update, not only before. ## Risk Assessment **Risk: MEDIUM-HIGH for closing CLI-08.** The plan set targets the right blockers and the order is mostly correct, so the strategy is credible. The risk comes from 06-14's asset/room conversion scope and 06-16's fidelity expectations. If extracted schemas and room rendering are straightforward, this can close CLI-08. If BNCentral conversion exposes non-trivial room/tile/background semantics, the current plan may overrun and need a narrower "real-looking BNCentral MVP slice" to keep Phase 6 moving. --- ## Consensus Summary Only one independent reviewer (Codex) was available — Claude was self-skipped for independence, and Gemini/CodeRabbit/OpenCode/Qwen/Cursor CLIs are not installed on this machine. No multi-reviewer consensus is possible; treat findings below as single-source rather than agreed. ### Single-Reviewer Top Concerns (HIGH) 1. **06-14 scope creep / schema assumptions** — combines BMP bootstrap + atlas build + BNCentral conversion + signing + server registration + client renderers + e2e in one plan. Assumes `meta.json` / `tiles.json` field names without prior schema discovery. Risk of mid-plan blockage if extracted schema differs from assumed shape. 2. **06-14 violates "narrow gap closure"** — full BNCentral replacement pulls in Phase-7-shaped work (collision, logic objects, backgrounds, spawn semantics). Recommend explicit MVP-slice scoping inside the plan. 3. **06-15 dependency metadata missing 06-14** — `depends_on` lists only 06-12, but Wave 11 executes after Wave 10 (06-14). Encode explicitly or make tests room-agnostic. ### Single-Reviewer Notable Suggestions (MEDIUM) - 06-14: add schema-discovery pre-task before implementation. - 06-13: cover diagonal normalization + collision behavior, or document their absence from `pcode_mover.gml`. - 06-16: drive animation phase from simulation ticks (BNO `image_speed`), not Phaser render frames — otherwise 60 FPS Phaser runs 2× BNO's 30 FPS room speed. - 06-10: verify `#EF4444` on `#0A0E1A` actually meets 4.5:1 contrast; check label-`for` vs wrapping-label selector assumption. - Better-Auth logout cookie assertion: use `page.context().cookies()` not `document.cookie`; assert empty length, not falsy values. - e2e canvas pixel sampling is fragile — prefer deterministic test hooks (`window.__rebno.*`) over WebGL pixel reads. - 06-17: gate on `pnpm trace:check` + `pnpm verify:phase-6` AFTER artifact updates, not before. ### Divergent Views N/A — single reviewer. ## How to Use ```bash /gsd-plan-phase 6 --reviews ``` To get true cross-AI signal, install Gemini CLI (`gemini`) and/or others, then re-run `/gsd-review 6`.