---
phase: 06.7
reviewers: [codex]
reviewed_at: 2026-05-17T11:18:25Z
plans_reviewed: [06.7-05-PLAN.md, 06.7-06-PLAN.md, 06.7-07-PLAN.md]
model: gpt-5.5
---

# Cross-AI Plan Review — Phase 06.7 Gap-Closure (Plans 05/06/07)

## Codex Review

## Plan 05

**Summary**  
Good plan, and it targets the three real blocker root causes. The core approach is right: keep `monotonic_at_ms` integer at the dispatcher boundary, source wire `vx/vy` from live input intent, and consume `anim_state.running` as the remote animation authority. I would amend one behavior before execution: preserve stale-remote safety so old `anim_state.running=true` does not animate a frozen remote forever.

**Strengths**
- Fixes all three diagnosed blocker causes directly.
- Keeps protocol schema strict instead of weakening `.int()`.
- Correctly avoids changing `packages/game-logic/src/step.ts`; BNO instant-set `vx/vy=0` remains valid simulation behavior.
- Adds focused regression tests around the exact failures.
- Uses `anim_state` as the explicit wire source of truth, which matches Phase 06.7’s client-authoritative movement carve-out.

**Concerns**
- **MEDIUM:** `onSimulationTickRemote(..., isRunning=true)` can bypass existing stale-snapshot velocity zeroing. If a remote stops sending updates, stale `anim_state.running=true` may keep animating in place.
- **LOW:** The proposed `axisX as -1|0|1` / `axisY as -1|0|1` cast hides future drift if `inputDispatcher.axisX()` ever returns non-discrete values.
- **LOW:** RC #1 coverage is client-unit only. That is probably enough, but the original failure happened at server zod parse, so one server/client-path acceptance test would be stronger.

**Suggestions**
- In `PlayerRenderer.onSimulationTickRemote`, compute running as `staleSnapshot ? false : (isRunning ?? Math.hypot(renderVx, renderVy) > VELOCITY_THRESHOLD)`.
- Add a regression test: stale snapshot + `isRunning=true` still renders Stand.
- Replace raw casts with a tiny helper like `toAxis(value): -1 | 0 | 1`, or at least type `InputDispatcher.axisX/Y()` as discrete axes.
- Keep Plan 05 as three commits only if each step stays green; otherwise make one commit after the whole contract is coherent.

**Risk Assessment: MEDIUM**  
The fix is conceptually correct, but the stale-snapshot interaction is a real animation regression risk.

## Plan 06

**Summary**  
The collision diagnosis is solid and the fix direction is right: normalize walkable-grid right/bottom sampling to the same exclusive-boundary convention as the polys path. The plan needs cleanup before execution because the named test files do not match the current repo layout, and a few proposed test coordinates do not actually represent the stated Navi feet bbox.

**Strengths**
- Correctly identifies the right/bottom inclusive-vs-exclusive mismatch.
- Chooses the right abstraction point: `isFeetBboxWalkable` owns grid semantics.
- Tests both direct helper behavior and end-to-end `step()` parity.
- Preserves left/top behavior explicitly.
- This also reduces future reconcile churn once server-side anti-cheat returns.

**Concerns**
- **HIGH:** `packages/game-logic/test/collision.test.ts` and `packages/game-logic/test/step.test.ts` do not exist. Current likely targets are `walkable-edge.test.ts`, `collision-axis-slide.test.ts`, `wall-slide.test.ts`, or new dedicated files.
- **MEDIUM:** The right-edge direct test coordinate is wrong as written. For `pos.x = N*44 + 35`, `left` should be `pos.x + NAVI_MASK.left = N*44 + 26`, not `N*44 - 9`.
- **LOW:** The fix changes room-edge behavior too: `right === room.w` / `bottom === room.h` becomes flush-walkable rather than out-of-bounds. That is probably correct under exclusive bounds, but it should be pinned.

**Suggestions**
- Update file targets to actual paths, preferably `packages/game-logic/test/walkable-edge.test.ts` plus a new or existing `step-*` test file.
- Build test samples from `pos + NAVI_MASK.*` rather than hand-coded `left/right/top/bottom`.
- Add explicit room-boundary tests: flush at max room edge allowed, one pixel past blocked.
- Keep the helper comment precise: parameters are bbox boundaries, internal right/bottom samples are converted to last interior pixel.

**Risk Assessment: MEDIUM**  
The production fix is low risk, but the plan’s current test-file and coordinate errors will waste execution time or produce weak tests.

## Plan 07

**Summary**  
The DX goal is good, but the must-have claim is currently overstated. `pretest:integration` and `pretest:full` help only when operators run package scripts. They do not affect the exact failure command shown in UAT: `pnpm -C apps/server exec vitest run test/protocol-v4-handshake.integ.test.ts`.

**Strengths**
- Addresses a real repeated false-positive class.
- Keeps unit-test inner loop unchanged.
- Uses a simple npm lifecycle hook instead of adding a custom runner.
- Documents the operator doctrine where the failure was observed.

**Concerns**
- **HIGH:** Direct `pnpm -C apps/server exec vitest run ...` bypasses `pretest:*`. So the plan does not satisfy its first must-have as written.
- **LOW:** The proposed grep checks for `stale-protocol-build`, but the sample runbook text does not include that exact hyphenated token.
- **LOW:** `pretest:full` covers all tests, but `test:full` may become slower. Acceptable, just note it.

**Suggestions**
- Either change the must-have to “package-script integ tests auto-build protocol,” or add a dedicated script like `test:protocol-v4` with `pretest:protocol-v4`.
- Update HUMAN-UAT to tell operators not to use direct `pnpm exec vitest` unless they first run `pnpm -C packages/protocol build`.
- Include the literal token `stale-protocol-build` if verification greps for it.
- Prefer workspace filtering if you want less path fragility: `pnpm --dir ../.. --filter @rebno/protocol build`.

**Risk Assessment: LOW-MEDIUM**  
Low production risk, but medium process risk because it may not prevent the exact command path that caused the false positive.

## Cross-Plan

**Gap Closure**  
Plans 05 and 06 close the 4 real UAT gaps if amended: 3 blocker remote-sync gaps plus 1 minor collision gap. Plan 07 closes the Test 5 process trap, not a production gap.

**Wave Parallelism**  
Mostly safe. File ownership is disjoint: Plan 05 touches `apps/client`, Plan 06 touches `packages/game-logic`, Plan 07 touches `apps/server/package.json` and UAT docs. The only practical coupling is verification: Plan 05 imports `@rebno/game-logic`, so run final client tests after Plan 06 merges.

**Possible Plan 08**
- Not necessary if Plan 05 adds stale-snapshot animation coverage and Plan 07 fixes the direct-`vitest` bypass claim.
- If not folded in, Plan 08 should cover: stale `anim_state.running` does not animate frozen remotes, direct protocol-handshake test command cannot bypass protocol build, and Phase 06.8 self-player snapshot re-enable has a parity guard.

**Hard Rule 1 Compliance**  
Compliant. Movement fields, including `position`, `velocity`, `facing`, and `anim_state`, remain client-authoritative under the Phase 06.7 carve-out. Server still validates schema and keeps chat, identity, room transitions, inventory, and persistence outside the trust carve-out.

---

## Codex-Verified Concerns (orchestrator validation)

The orchestrator (Claude Opus 4.7) verified codex's high-severity findings against the live codebase:

1. **Plan 06 test-file paths — CONFIRMED WRONG.** `packages/game-logic/test/collision.test.ts` and `packages/game-logic/test/step.test.ts` do not exist. Actual relevant files: `walkable-edge.test.ts` (isFeetBboxWalkable home), `step-pass-through.test.ts`, `step-bno-fidelity.test.ts`, `wall-slide.test.ts`, `collision-axis-slide.test.ts`. Plan 06 must be amended before execution.

2. **Plan 06 coordinate math — CONFIRMED WRONG.** `NAVI_MASK = {left:-9, top:-9, right:9, bottom:0}` per `packages/game-logic/src/constants.ts:86-91`. For `pos.x = N*44 + 35`, the feet bbox is `left = N*44 + 26`, `right = N*44 + 44`. Plan 06 wrote `left = N*44 - 9` which is off by 35 px. Test setup math must be rebuilt from `pos + NAVI_MASK.*`.

3. **Plan 07 must-have overstated — CONFIRMED.** `pretest:integration` / `pretest:full` hooks only fire on the npm-lifecycle wrapped commands (`pnpm -C apps/server test:integration`). The UAT failure command was `pnpm -C apps/server exec vitest run test/protocol-v4-handshake.integ.test.ts` — direct `exec vitest`, which bypasses every npm hook. Plan 07's first must-have ("automatic" build before integ tests) is not satisfied by the hooks alone.

## Consensus Recommendations

### Amend Plan 05 (MEDIUM risk — fold in OR defer to Plan 08)
- Add stale-snapshot guard: `running = staleSnapshot ? false : (isRunning ?? hypot(vx,vy) > 0)`
- Add stale-snapshot regression test (broadcast running=true + frozen position → Stand)
- Replace `as -1|0|1` raw casts with `toAxis()` helper OR widen `InputDispatcher.axisX/Y()` signature to discrete union

### Amend Plan 06 (MEDIUM risk — must amend before execution)
- Fix file targets: `walkable-edge.test.ts` (isFeetBboxWalkable tests) + new or existing `step-*.test.ts` (parity tests)
- Rebuild test coordinates from `pos + NAVI_MASK.*` not hand-coded edges
- Add room-boundary regression test (flush at room.w / room.h allowed; +1 px past blocked)
- Add precise helper docstring clarifying internal exclusive-boundary semantics

### Amend Plan 07 (LOW-MEDIUM risk — fold in)
- Either: (a) tighten must-have wording to "package-script integ tests auto-build protocol" + add HUMAN-UAT operator doctrine warning against direct `pnpm exec vitest` without protocol pre-build, OR
- (b) Add dedicated `test:protocol-v4` script with `pretest:protocol-v4` and update UAT runbook + debug `.md` resolution lines to use it
- Add literal `stale-protocol-build` token to runbook (verification grep depends on it)
- Use `pnpm --filter @rebno/protocol build` to avoid `../../` path fragility

### Wave Parallelism Constraint (codex)
- Plan 06 should land BEFORE final apps/client vitest gate in Plan 05 (apps/client consumes @rebno/game-logic). Add explicit dependency: Plan 05 final `pnpm -C apps/client exec vitest run` gate runs AFTER Plan 06 commits.

### Possible Plan 08 (if not folded above)
- Stale `anim_state.running` does not animate frozen remote (Plan 05 amendment guard)
- Direct `pnpm exec vitest test/*.integ.test.ts` cannot bypass protocol build (Plan 07 amendment)
- Phase 06.8 self-player snapshot re-enable has 1-px parity guard (Plan 06 follow-up — pin the parity invariant)

### Hard Rule 1 Compliance — PASS
All three plans respect the narrow movement carve-out. Server still authoritative on chat, identity, room transitions, inventory, persistence. Schema validation preserved.