feat(ahbg): extract validated presentation-only board - #17
erinepshovel-code wants to merge 21 commits into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b2f1da218b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| x: RADIUS * (q + r / 2), | ||
| y: RADIUS * (Math.sqrt(3) / 2) * r, |
There was a problem hiding this comment.
For observations using any UCNS orientation, nesting, or future non-default construction, this formula independently reconstructs center positions from AHBG axial coordinates instead of rendering UCNS's exact centers. This conflicts with the authority contract in ahbg/README.md lines 13-21 and will silently flatten those valid geometries into this locally defined grid; the snapshot should carry UCNS-derived display positions and their source identity rather than recomputing them here.
AGENTS.md reference: AGENTS.md:L13-L15
Useful? React with 👍 / 👎.
| <section class="board-wrap"> | ||
| <h1>AHBG plane</h1> | ||
| <p class="note">Presentation only. Not mechanics. Each tile is a centerpoint; circles are Seed of Life geometry. Dashed traces show already-resolved motion. Click a center, or focus it and press Enter/Space, to inspect.</p> | ||
| <svg id="board" role="img" aria-label="Seed of Life tiles as centerpoints with resolved unit motion traces"></svg> |
There was a problem hiding this comment.
For screen-reader users, role="img" makes the SVG's descendants presentational, so the dynamically created role="button" tile hit targets are not exposed as individual controls even though they remain keyboard-focusable. Use a container role that permits interactive descendants, or move the controls outside the image semantics, so the advertised tile inspection is accessible.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| const motionUnits = new Set(); | ||
| for (const motion of snapshot.motions || []) { |
There was a problem hiding this comment.
When a fetched snapshot contains motions: false, 0, or "", this fallback treats the malformed field as an empty motion list and accepts the document, whereas the Python contract rejects every present non-list value. That breaks the package's fail-closed validation boundary for browser consumers; distinguish an omitted field from a present value and require Array.isArray(snapshot.motions) before iterating.
Useful? React with 👍 / 👎.
| @@ -0,0 +1,95 @@ | |||
| """Project a sanitized observation into an AHBG presentation snapshot. | |||
There was a problem hiding this comment.
These new behavior-bearing Python and JavaScript modules start directly with implementation and omit the MODULE_BUILD declarations required by the pinned skill-lib/meta-module-build/SKILL.md doctrine. Consequently the module-build runner reports them as coverage gaps and cannot audit their declared owner, public surface, boundaries, dependencies, rollout, or rollback; add the module declarations before treating this package as a bounded build.
AGENTS.md reference: AGENTS.md:L47-L49
Useful? React with 👍 / 👎.
| const angle = (Math.PI * 2 * index) / group.length - Math.PI / 2; | ||
| const spread = UNIT_RADIUS * 1.35; | ||
| return { x: Math.cos(angle) * spread, y: Math.sin(angle) * spread }; |
There was a problem hiding this comment.
When four or more units share a tile, placing every center on a fixed 1.35 * UNIT_RADIUS ring no longer separates the markers: with four units adjacent centers are about 21 pixels apart while each marker is 22 pixels wide, and the overlap rapidly worsens as more units are added. Since snapshots impose no occupancy cap and the README promises separation for multiple occupants, size the spread by group length or use additional rings.
Useful? React with 👍 / 👎.
| "tiles": tiles, | ||
| "units": units, | ||
| "selected_tile": selected_tile, | ||
| "feed": [dict(item) for item in feed], |
There was a problem hiding this comment.
When a feed item contains internal metadata alongside its public turn and text—for example a private prompt, DM state, or certification field—dict(item) copies all of it into the returned presentation snapshot, and the validator permits those unknown keys. Any caller that serializes this supposedly visual-only snapshot to the browser therefore leaks fields that are never rendered; explicitly project only the declared feed fields just as the tile, unit, and motion loops do.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a4464f7293
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| raise PresentationSnapshotError(f"tile {tile_id} y must be numeric and nonboolean") | ||
| position = (float(x), float(y)) | ||
| if position in positions: | ||
| raise PresentationSnapshotError(f"tile source position repeats: {position}") |
There was a problem hiding this comment.
When load_snapshot reads Python-supported NaN or Infinity constants, or the projector receives non-finite floats directly, these type checks accept them and the snapshot is reported as validated. Such coordinates produce invalid display geometry and are rejected by the browser validator, while sufficiently large integers can instead raise an uncaught OverflowError during float(x). Convert safely and require math.isfinite so the Python and browser contracts agree.
Useful? React with 👍 / 👎.
| const motion = motionByUnit[unit.id]; | ||
| const fromCenter = motion ? sourceToPixel(byId[motion.from]) : center; | ||
| const origin = { x: fromCenter.x + offset.x, y: fromCenter.y + offset.y }; |
There was a problem hiding this comment.
When a moving unit and a stationary unit initially share a tile but end on different tiles, offset is calculated from the moving unit's destination group and then reused at its origin. Both markers therefore start at the same center even though the multiple-occupant layout is intended to keep them separated; build a separate initial grouping from motion from tiles plus stationary units and use its offsets for origin.
Useful? React with 👍 / 👎.
| const minX = Math.min(...pixels.map((p) => p.x)) - RADIUS * 1.2; | ||
| const minY = Math.min(...pixels.map((p) => p.y)) - RADIUS * 1.2; | ||
| const maxX = Math.max(...pixels.map((p) => p.x)) + RADIUS * 1.2; | ||
| const maxY = Math.max(...pixels.map((p) => p.y)) + RADIUS * 1.2; | ||
| svg.setAttribute("viewBox", `${minX} ${minY} ${maxX - minX} ${maxY - minY}`); |
There was a problem hiding this comment.
With the new count-scaled spread, a tile containing roughly 16 or more units places marker edges beyond the fixed 1.2 * RADIUS padding, especially on an outermost tile, so SVG viewport clipping hides part of the otherwise separated markers. The snapshot contract has no occupancy cap; compute the bounds after unit offsets or enlarge them to cover the maximum displaced marker extent.
Useful? React with 👍 / 👎.
| if label is not None and (not isinstance(label, str) or not label): | ||
| raise PresentationSnapshotError(f"unit {unit_id} label must be exact non-empty text when present") |
There was a problem hiding this comment.
When a JSON snapshot explicitly contains label: null on a tile or unit, or turn: null on a feed item, the use of .get() treats the field as absent and accepts the document even though these fields must be non-empty text or a non-negative integer when present. The browser validator rejects the same snapshot, so a Python-validated document can fail at its consumer; distinguish key absence from an explicitly null value while retaining the separately documented nullable selected_tile behavior.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b4ca55333
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| function plainInteger(value) { | ||
| return Number.isInteger(value); | ||
| } |
There was a problem hiding this comment.
When a Python-validated snapshot contains a turn above Number.MAX_SAFE_INTEGER (for example 9007199254740993), JSON parsing rounds it to 9007199254740992, and this predicate still accepts the rounded value. The board then displays a different root or feed turn than the producer supplied; enforce the same safe-integer range in both validators rather than using Number.isInteger alone.
Useful? React with 👍 / 👎.
| raise ValueError("y must be numeric and nonboolean") | ||
| if isinstance(scale, bool) or not isinstance(scale, (int, float)) or scale <= 0: |
There was a problem hiding this comment.
When callers pass NaN or infinity as the scale, this guard accepts it because comparisons with NaN are false and positive infinity is greater than zero, causing source_to_display to return non-finite coordinates. The same remains true for non-finite x and y; since this is declared as a public transform and already validates numeric inputs, require math.isfinite for all three values before producing display geometry.
Useful? React with 👍 / 👎.
| try: | ||
| payload = json.loads(target.read_text(encoding="utf-8")) | ||
| except (OSError, json.JSONDecodeError) as exc: | ||
| raise PresentationSnapshotError(f"cannot load snapshot: {exc}") from exc |
There was a problem hiding this comment.
When the snapshot file contains invalid UTF-8, Path.read_text raises UnicodeDecodeError, which is not covered by this exception handler. Any caller that handles the package's advertised PresentationSnapshotError for malformed snapshots will instead receive an unexpected exception and can crash; wrap decoding errors alongside I/O and JSON parse failures.
Useful? React with 👍 / 👎.
| @@ -0,0 +1,164 @@ | |||
| """Regression tests for the AHBG presentation-only boundary. | |||
There was a problem hiding this comment.
Although the repair added MODULE_BUILD declarations, fresh inspection shows that the behavior modules still declare no CONTRACTS and this executable test module owns no CHECKS entries, so the required test-build audit cannot link any of these nine tests to source obligations or detect orphaned behavior. Add source-owned contracts and test-owned checks rather than relying only on unittest discovery.
AGENTS.md reference: AGENTS.md:L47-L49
Useful? React with 👍 / 👎.
…bounds (#20) Recovered the presentation-only board from closed #17 (built from current main). Repairs the two remaining renderer defects: - initial vs final multi-unit occupancy offsets: a unit now uses the initial occupancy group of its motion origin for the start marker and the final group of its presented tile for the end marker, instead of applying the final-tile offset to both endpoints. - SVG viewBox bounds: viewBox now derives from seed circles plus every displaced unit marker extent at both motion endpoints, so offset markers cannot clip at the board edge. Keeps UCNS as geometry authority (presentation only scales supplied centers). Regression tests mirror the frozen offset math and assert the corrected source structure. Co-authored-by: erinepshovel-code <250928284+erinepshovel-code@users.noreply.github.com>
1 participant
Replacement for presentation concept in #3
Built from current
main@eaec7fd6ee4e829b6fae10a2c6d520b35857137d. This PR contains only the AHBG presentation surface plus its dedicated CI; it does not carry the engine, DeepSeek, ora0minscope that accumulated in #3.Preserved concept
Review repairs
node --checkusing current Actions v6.Boundary
Presentation geometry is explicitly not game adjacency authority. No engine, War, turn, RNG, permissions, or agent-harness behavior is defined here.
Usage
hmmm
The live engine-to-observation adapter remains owned by the eventual engine integration; construction animation awaits an owning mechanics event contract.