feat(sim,render): #128 underground-embedding guards (PR 6-sim V30) + sprite containment (PR 6-render)#208
feat(sim,render): #128 underground-embedding guards (PR 6-sim V30) + sprite containment (PR 6-render)#208LightAxe wants to merge 15 commits into
Conversation
…28) — flurry PR 4 PR 4 of the #127/#128 fix series (plan/flurry/PR2-FIX-SPECS.md). Freezes surface terrain so it never mutates as food piles spawn/deplete or entrances are designated — the static-terrain bug class Phase 0 measured (477/900 piles revealed a HardBlock on removal). Scope is STRICTLY static terrain; no #127/#128 movement fixes (those are PR 5 / PR 6). What changed: - surface-features.ts: deleted the dynamic entrance/food-pile suppression from the shared feature selector (movement AND render). Split procedural vs frozen: surfaceFeatureProcedural (terrainSeed only) + a baked 128x128 movement-effect grid that is the source of truth for surfaceMovementAt (O(1)). surfaceFeatureAt consults the carve override so render never paints a sprite over a carved-passable tile (R4-3). Added bakeStaticTerrain (deterministic root-clearance reservation + corridor carve guaranteeing ONE connected component; no terrainSeed retry), computeSurfaceComponentMask, isSurfaceTileInComponent, validateSurfaceConnectivity. Boot-asserts no registry feature is Cosmetic (carve-detection precondition). - types.ts: SIM_VERSION_V28_STATIC_TERRAIN (LATEST=28); new stored Uint8Array bakedSurfaceEffect + derived surfaceComponentMask; procedural bake in createWorldState; threaded copyWorldState. - save.ts: bakedSurfaceEffect serialized packed 2-bit + base64 (~5.3 KB vs ~48 KB raw). Deserialize validates dims / enum range / full connectivity (every saved pile + entrance), rejecting corrupt or pre-V28 maps. MIN_ACCEPTED raised to V28 (posture 2 — old saves reject cleanly). - scenario.ts: bake BEFORE placing piles; reachable-spawn pile gate; world-gen connectivity assertion. - tick.ts: DesignateEntrance rejects unless candidate + clearance are already passable on the frozen grid AND in the connected component (can no longer carve). - food-system.ts: runtime pile spawn gated on the connected component. - constants.ts: SURFACE_ROOT_CLEARANCE_RADIUS. Oracle/tests: featureFieldHash extended to the FULL SurfaceFeatureSlice (kind+variant+anchor); committed oracle (0 mutate across pile spawn/deplete, entrance designate/open, save/load), connectivity, and simVersion rejection-boundary tests; determinism serializer threads the baked grid; suppression/render tests updated to the static-terrain contract. Measurements: save-size delta +0.49%; tick-time ~0.24 ms/tick; acceptance sweep (10 seeds x 3 difficulties) 0 connectivity failures, 0 hash mutations. verify green (79 files / 2336 tests). Does NOT close #127/#128 (PR 5/6). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…view) Addresses the PR #206 CI failure + Codex/ship-review findings. - Perf (CI timeout fix): createWorldState + createScenario ran a full 16,384-tile procedural bake on every call (twice per scenario), tipping many-iteration tests over the 5s timeout on the CI runner. Memoize bakeSurfaceEffectGrid by terrainSeed (pure function of it) and return a copy; the suite is now faster than the pre-PR baseline (tests ~31s vs ~60s). - save.ts base64 (Codex P2): reject `=` padding outside the final quartet (and the never-valid "x=y" form) instead of treating it as zero — malformed encodings now fail the load boundary as intended. - SAVE_FORMAT_VERSION 3 → 4 (ship-review): the new required on-disk bakedSurfaceEffect field is a shape break; bump it (and SAVE_KEY :v3→:v4 + purge old key) so save-shape correctness isn't silently coupled to the simVersion floor. - DesignateEntrance halo (ship-review, bugs dim): check component membership per clearance tile instead of a bare HardBlock test, so it no longer relies on an unenforced "globally one walkable component" assumption (isolated empty pockets are harmless). Softened the over-claiming doc comments to match what the bake actually guarantees (all roots/piles/entrances share one component). - computeSurfaceComponentMask: throw on a wrong-sized grid (fail loud instead of silently flooding the map into one component). verify green (79 files / 2336 tests). Still PR 4 scope only; does NOT close #127/#128. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…rio (codex P1) Codex P1: the procedural-bake `Map` was mutable module-level state in src/sim — an AGENTS.md ECS-rule blocker (state outside the world snapshot). Removed it. Restore the performance without a cache: - bakeStaticTerrain now carves a COPY of the procedural grid createWorldState already baked, instead of re-baking from scratch — one full procedural pass per createScenario (was two). Measured: 200x createScenario ~2.0s (was ~4s). - vitest.config: raise the default testTimeout to 15s. Static terrain made world construction bake the 128x128 grid (~8 ms each), so construction-heavy sweeps (200-seed spider-lair, 500-iteration pause-cadence) sat near the 5s default and tipped over it under CI-runner contention — the PR #206 CI timeout. Assertions and iteration counts are unchanged; genuinely-hung tests still fail. verify green (79 files / 2336 tests, ~57s). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ut fix The per-tile procedural bake called the recursive overlap-suppression check up to 16,384× per grid (~8 ms), so construction-heavy tests (200-seed spider-lair, 500-iteration pause-cadence) blew even a 15 s timeout on the CI runner. Raising the timeout only masked it; this fixes the root cause. bakeSurfaceEffectGrid now iterates ANCHORS in lex (ay,ax) order instead of tiles: the first surviving anchor covering a tile claims it (matching the per-tile lex-smallest-winner tie-break), with a per-anchor "first covering feature per offset" scratch to reproduce the selector's first-covering-feature + break-on- suppress semantics exactly, including negative (off-edge) anchors. Verified byte-identical to the per-tile surfaceFeatureProcedural across 80 seeds × all 16,384 tiles (0 mismatches), so surfaceFeatureAt's carve detection and the feature-field oracle stay consistent. Measured: ~0.55 ms/bake (was ~8 ms); 200× createScenario ~0.45 s (was ~2-4 s). Also: - Reverted the vitest testTimeout bump — no longer needed; the bake is fast. - createScenario resets surfaceComponentMask after initColony so the persisted/ validated mask is rooted at the real canonical root (first colony entrance) rather than relying on the pre-colony fallback root happening to equal it (ship-review advisory). verify green (79 files / 2336 tests). PR 4 scope only; does NOT close #127/#128. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…(codex P2) The DesignateEntrance sim gate (PR 4) now rejects tiles outside the connected walkable component or with a blocked clearance halo, but surface-input still previewed any empty tile — so a player saw a valid-looking entrance target and confirmation did nothing. Add isValidEntranceTarget (empty tile AND candidate + SURFACE_ROOT_CLEARANCE_RADIUS halo all in-component) mirroring the gate, and use it for the right-click preview. Smoke tests updated: the mock world carries a fully-walkable baked grid, plus new cases asserting no preview when the target or a halo tile is HardBlock. verify green (79 files / 2338 tests). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ia simVersion The e2e CI job failed (5 save/load specs): bumping SAVE_KEY v3→v4 orphaned every spec that addresses the literal 'subterrans:save:v3', and the bump itself was only a low-severity ship-review advisory, not a spec requirement. Revert SAVE_FORMAT_VERSION (4→3) and SAVE_KEY (:v4→:v3). The new required bakedSurfaceEffect lives in the SNAPSHOT (SerializedWorldState), and this codebase versions snapshot-content shape changes by simVersion — not the envelope's SAVE_FORMAT_VERSION (which versions envelope structure). deserializeWorldState validates simVersion FIRST, so raising MIN_ACCEPTED_SIM_VERSION to V28 already rejects every pre-static-terrain save (which also lacks the field) before unpack is reached — the required behaviour, achieved without an envelope bump and matching the v2/v3 precedent. The legacy-purge list reverts accordingly. All required PR-4 save behaviour is retained: MIN_ACCEPTED=V28, the required bakedSurfaceEffect field, and load-time dims/enum/connectivity validation. Verified locally: npm run verify green (2338); the 5 previously-failing e2e specs (menu-and-dialog #115/#196, phase-09 save-prompt) now pass (19/19 chromium). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ailing-bit guard Two low-severity ship-review advisories on the latest diff: - surface-features.ts: the @param world JSDoc on isAnchorSuppressedByOverlap still described the deleted isAnchorGameplaySuppressed call. Correct it — world is now only threaded to the recursive calls; no field is read (PR 4 removed dynamic suppression). Doc-only. - save.ts: unpackBakedSurfaceEffect now rejects non-zero unused high bits in the final packed byte when expectedLen isn't a multiple of 4, so a tampered payload can't decode to a grid serialize would never emit. No-op for the production 16384-tile grid (a multiple of 4), but keeps the exported, length-general validator honest. (Left as-is: the preview/sim entrance gates still diverge on column-uniqueness / entrance-cap — that divergence predates PR 4; this PR only added and mirrored the new component/clearance gate. Out of scope.) verify green (79 files / 2338 tests). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…lence/serialization tests, hardening Fable (adversarial review standing in for Codex) found no runtime bugs but flagged spec-mandated validation/test gaps + low-severity hardening. All addressed: P2 — load-validation + load-bearing test coverage: - validateSurfaceConnectivity now also checks each entrance's full radius-3 clearance halo is in-component (spec R3-8), mirroring the DesignateEntrance gate — so a corrupt/tampered save the sim gate could never produce is rejected. - Committed the bake↔selector equivalence sweep (bakeSurfaceEffectGrid byte- identical to per-tile surfaceFeatureProcedural across 6 seeds × all 16,384 tiles), plus direct computeSurfaceComponentMask and corridor-carve (carveCorridor merges a HardBlock-split row) regression tests. - §6.5 field-specific serialization tests for bakedSurfaceEffect: create→copy round-trip (determinism.test), save→load mutate-and-survive (catches a re-derive-from-seed deserializer), every unpackBakedSurfaceEffect rejection path, and load-time connectivity rejection. P3 — robustness: - Oracle entrance-designation step asserts ≥1 entrance was actually accepted (no vacuous rejection-only pass). - surfaceComponentMask materialized eagerly at world-gen, load, AND tick entry, so input/render queries (and mid-tick spawn checks) are pure reads, never a lazy world mutation. - isValidEntranceTarget halo bounds use the grid CONSTANTS (matches the sim gate). - packBakedSurfaceEffect throws on out-of-range effect values (fail at write, not a later read); unpackBakedSurfaceEffect rejects by base64 length BEFORE decoding (DoS guard, matching the issue #99 load-cap posture). - validateSurfaceConnectivity rejects the degenerate all-HardBlock-root case (zero entrances + zero piles no longer passes on a garbage all-zero mask). verify green (79 files / 2345 tests, +9 new); internal ship-review passed:true. Still PR 4 scope (static terrain); does NOT close #127/#128. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
🔍 Adversarial Review (Claude, standing in for Codex)Reviewed commit: What I verified before the findings: the tightening-write enumeration is complete — I walked every P2 — The queen has a second, unguarded descent path (class-ii guard incomplete) The class-ii landing guard was added to the worker descent ( P3 — Class-ii structural coverage is worker-only The committed class-ii test descends a CarryingFood forager onto a non-enterable landing. Neither queen descent path has a structural test — add one alongside the 3686 fix (a queen on an open entrance with a non-enterable column top must stay on the surface), so both queen sites are pinned against regression. VERDICT: REVISE — everything else is clean: the guard set is provably complete, the transactional semantics are right, the dead-digger lifecycle can't orphan, and the render invariant is both proven and non-vacuously probed. The single P2 is a four-line fix plus one test. |
🔍 Adversarial Re-Review (Claude, standing in for Codex) — round 2Reviewed commit: Both round-1 findings are addressed — verified against the diff:
Also noted: For the post-rebase ledger, the only item still open anywhere in the series is the non-blocking PR 4 nit (length pre-check shadows the decoder rejection-test labels in VERDICT: APPROVED — all three PRs in the series are now approved at their current heads (#206 |
…te (Fable P3) The malformed-input cases in the unpackBakedSurfaceEffect rejection test were all shorter than the expected base64 length for a full grid, so the DoS length pre-check rejected every one of them before the decoder branch each label named could run. Rebuild each case at the exact full-grid length (5464 chars) with a single targeted corruption — bad char, mid-string padding, x=y final-quartet padding — so each exercises the base64ToBytes branch it claims, and keep an explicit wrong-length case for the length gate itself. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… (C-both) — flurry PR 5 (V29) PR 5 of the #127/#128 fix series, built on PR 4 static terrain. Eliminates the dominant #127 mechanism (scent/priority-vs-wall, 86% of episodes) and the residual wander short-tail. Fix-A — passability-aware stepping (surface-routing.ts): - New stepTowardReachable(world, from, target): first step of a COMPLETE BFS goal field (distance-to-target over PR 4's single connected component), cached per target tile on the world (derived, not serialized). Replaces the naive pickCardinalStep in BOTH forager target branches (priority + scent). Target identity unchanged — only the step is path-aware. Three-valued: AtGoal (0,0) / Step / InvariantViolation. Scent ranks Manhattan-15-eligible piles by reachable path distance, lowest-foodPileId tie-break. No-revisit bypassed for targeted steps. Zone-guarded to the surface (underground falls back to cardinal step). Diagonals chosen only when a shared orthogonal is itself passable+descending (no corner-squeeze that the movement guard would reject). C-both — recent-tiles deepening: - RECENT_TILES_LEN 4→12 (chosen N: largest of {12,10,8,6,5} meeting all caps). - Compact canonical save encoding (records sorted by antId; head + non-sentinel (slot,x,y) in slot order); load validates + rejects malformed streams. Far smaller than the old flat arrays — total save size −19% vs baseline. simVersion V29 (posture 2): bump LATEST + raise MIN_ACCEPTED. Pocket-escape not needed — deepening alone drives confinement to zero. Acceptance (committed harness tests, ACCEPTANCE hold-out): aimedIntoWall=0, worst confinement=0tk (≤60), episodes>300=0; 4-vs-N sweep N=12 meets every throughput cap; field-specific copy/save-load + V29 boundary tests; save −18.95% (≤+5%); tick-time 0.26 ms/tick (≤0.5). verify green (80 files / 2380 tests); ship-review passed:true. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… metric, honest zone comments, ring-load hardening Fable (adversarial review standing in for Codex) flagged that the acceptance story had drifted. All findings addressed: P2 — acceptance integrity: - harness stepLandsOnWall reverted to the NAIVE pickCardinalStep direction (the #127 wall-aim signature), measured INDEPENDENTLY of the router. Calling stepTowardReachable made the cap tautologically 0 (the router never returns a wall step). Fix-A satisfies the cap by eliminating the confinement episodes the flag is tallied within (episodeAimedWall is only set during a confined run), not by redefining the flag — now falsifiable again. - The underground scent pull is now DELIBERATELY dropped in V29 with an honest comment (it fed SURFACE pile coordinates to an underground walker — meaningless; underground searchers use pheromone/wander). The prior "(pre-PR-5 behaviour)" comment was wrong. - The unreachable-surface priority case is documented as the benign transient stale-target handling it is (PR 4 guarantees current priority piles are reachable, so an unreachable target is a leftover from a prior Fighting/zone stint). Investigated alternatives: asserting crashes on these (S3 determinism scenarios); falling through to wander REGRESSES worst confinement to 108 tk. The cardinal step + surface-detour guard keeps the ant moving (harness confirms worst=0, aimedIntoWall=0), so it is kept — stepTowardReachable's throw remains a defensive guard. P3: - packRecentTiles comment corrected (antId<nextEntityId skips NEVER-allocated ids; dead-but-allocated ARE included). - unpackRecentTiles now rejects antId outside [0, min(nextEntityId, capacity)) — nextEntityId is the canonical (serializer) bound, capacity the memory-safety bound; a tampered count/nextEntityId mismatch is rejected, not silently dropped. - measureForagingThroughput doc: underground-carrier behaviour is subsumed by the cross-zone delivery latency/path/completion counters. - Added a goal-field cache-eviction (clear-all-on-overflow) regression test. verify green (80 files / 2381 tests); ship-review passed:true. Still PR 5 scope (V29 path-aware routing + deepened recent-tiles). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
…review round 1) Entity ids are never reused and dead rings are never read (all isRecentTile/detour consumers run only for alive ants), but an ant killed mid-Searching/CarryingFood keeps its populated ring in memory forever — serializing those records grew every subsequent save monotonically. Skip non-alive ids at pack time; old saves containing dead-ant records still load (unpack doesn't check alive) and self-heal on the next save. Tests now seed rings on live worker ids instead of raw ids 0/1/3, plus a regression test that a dead ant's populated ring is not serialized. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…sprite containment (PR 6-render) PR 6 of the #127/#128 fix series, built on PR 5. Closes the latent #128 underground-embedding invariant via two artifacts. PR 6-sim (V30, posture 2 — bump LATEST + raise MIN_ACCEPTED): - (ii) Descent landing-tile guard (worker + queen): an ant lands underground ONLY on a tile it canEnterUndergroundTile for its task; else it holds on the surface (re-checked next tick) — never seeks a nearest legal landing. Fails CLOSED if the grid is absent. - (iv) One task-aware occupancy guard on EVERY passability-tightening underground mutation, in the neutral underground-occupancy.ts module (findEmbeddedByTightening, keyed by currentGridColonyId — sees foreign occupants — placed neutrally to avoid the colony↔ant-system import cycle): - CancelDigMark Marked→Solid: single-tile guard; chamber-cancel guarded TRANSACTIONALLY (preflight the whole footprint, block-all-or-revert-all). - tickDeadDiggerCleanup BeingDug→Marked: retain-the-dead-claim-and-retry under a non-digger occupant (never orphans the BeingDug tile); liveness bound documented. - PlaceChamber Solid→Marked is loosening — not gated. PR 6-render (no simVersion bump — render-only): - sprite-containment.ts: containedScale() uniformly scales an underground ant sprite down just enough that its rotated AABB never crosses its Open anchor tile's boundary toward a Solid/off-grid neighbour (overflow into passable neighbours stays allowed). Wired into draw-underground.ts. The queen (20×14) and rotated fighters overflow the 16px tile pre-fix; workers at natural size do not. Acceptance (committed harness/probe tests, all printed): - #128 structural (ii descent + iv CancelDigMark/chamber-cancel/dead-digger incl. orphan-prevention) = 0 embed; adversarial dense-cancel stress = 0; AI record+replay (runAIController) = 0 natural embeddings. - Render containment probe: 525 configs (kinds × rotation extrema × max scale × interp-alpha sub-tile centers) — 0 dirt overlaps; non-vacuous (unclamped queen overflows) and does not over-clamp on passable neighbours. - V30 rejection-boundary; determinism + neutrality green; save −18.95% (≤+5%); tick-time 0.25 ms/tick (≤0.5). verify green (81 files / 2390 tests); ship-review passed:true (2 low advisories proactively addressed: fail-closed descent, dead-digger liveness bound). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…lass-ii P2) Fable found the V30 class-ii landing-tile guard was on 2 of 3 descent sites: the worker descent and the queen's step-onto-entrance descent were guarded, but the queen's PRE-MOVE descent short-circuit (already standing on her open entrance, ant-system.ts ~3686) still set zone=Underground/posY=0 with no enterability check — the same class-ii embed, reachable via the documented starter case or a corrupt save (isOpen true + non-enterable column top). - Add the same fail-closed landing-tile guard to the pre-move queen descent. - Add a structural test: a queen on an open entrance with a non-enterable column top stays on the surface (pins all queen descent paths against regression). - Minor: un-export rotatedAabbHalfExtents (internal-only; review surface nit). verify green (81 files / 2391 tests); ship-review passed:true (2 low advisories: documented diagonal-clamp conservatism; aabbOverlapsDirt kept as the committed containment-invariant predicate the probe asserts). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ntainedScale (ship-review round 1) Two over-clamps drove the contained sprite scale to 0 in normal play: - isDirt treated every off-grid tile as dirt, so a freshly-descended ant (posY=0, sprite center on the grid's top edge, edgeN=0) clamped to scale 0 and popped out of existence on every descent. Nothing renders above row 0 in the underground viewport, so the above-grid row is now exempt. - A Solid diagonal clamped BOTH axes to that corner's edge distances unconditionally, collapsing the factor toward 0 whenever the center neared a tile edge with dirt diagonally ahead — every tile crossing in a 1-wide tunnel. A corner only obstructs when the AABB crosses both of its edges, and shrinking along one axis suffices, so it now clamps only in that case and only along the less-restrictive axis. Both keep the containment invariant (footprint never paints dirt); regression tests fail against the previous clamping. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2187609 to
16d17fe
Compare
🔍 Adversarial Re-Review (Claude, standing in for Codex) — round 3 (post-rebase)Reviewed commit: The two containment over-clamp fixes are correct, and I checked the geometry adversarially rather than taking the commit message's word:
Both fixes ship with regression tests that fail against the previous clamping, and the original probe suite (vacuity control, no-over-clamp, worker-never-clamped) still passes around them. No findings. VERDICT: APPROVED (head |
PR 6 of the #127/#128 fix series — closes the latent #128 underground-embedding invariant. Built on PR 5 (#207) → stacked on #207, which is stacked on #206; review/merge #206 → #207 → #206 first. simVersion V30 (PR 6-sim); PR 6-render is render-only (no bump). Do not merge until Rob OKs.
PR 6-sim (
src/sim, V30, posture 2)canEnterUndergroundTilefor its task, else holds on the surface — never seeks a "nearest legal landing". Fails closed if the grid is absent.underground-occupancy.ts(findEmbeddedByTightening, keyed bycurrentGridColonyIdso foreign occupants are seen; placed neutrally to avoid the colony↔ant-system cycle):CancelDigMarkMarked→Solid — single-tile guard; chamber-cancel guarded transactionally (preflight whole footprint, block-all-or-revert-all).tickDeadDiggerCleanupBeingDug→Marked — retain-claim-and-retry under a non-digger occupant (never orphans the BeingDug tile); liveness bound documented.PR 6-render (
src/render, no bump)sprite-containment.ts—containedScale()uniformly scales an underground ant sprite down just enough that its rotated AABB never crosses its Open anchor tile's boundary toward a Solid/off-grid neighbour (overflow into passable neighbours stays allowed). The queen (20×14) and rotated fighters overflow the 16px tile pre-fix; workers don't. Wired intodraw-underground.ts.Acceptance (committed tests, all printed)
Verification
npm run verifygreen — 81 files / 2390 tests; internalship-reviewpassed:true(2 low advisories proactively addressed). Codex review pending (account out of credits); reviewed internally + by Fable.🤖 Generated with Claude Code