Breadway/omniastra
1
0
Fork 0
Code Issues 1 Pull requests Projects Releases Packages Wiki Activity Actions

Fix code review findings (OMNI-1 to OMNI-24) #25

Merged
Breadway merged 17 commits from fix/review-findings into main 2026-10-08 19:54:27 +08:00
Conversation 2 Commits 17 Files changed 41 +1276 -246
Breadway commented 2026-10-06 16:07:04 +08:00
Owner
Copy link

Fixes the findings from the code review, one commit per issue or small group. Most fixes come with a regression test; for most of them I confirmed the test fails on the old code.

Closes OMNI-1, Closes OMNI-2, Closes OMNI-3, Closes OMNI-4, Closes OMNI-6, Closes OMNI-7, Closes OMNI-8, Closes OMNI-9, Closes OMNI-10, Closes OMNI-11, Closes OMNI-12, Closes OMNI-13, Closes OMNI-14, Closes OMNI-15, Closes OMNI-16, Closes OMNI-17, Closes OMNI-18, Closes OMNI-19, Closes OMNI-20, Closes OMNI-21, Closes OMNI-22, Closes OMNI-23, Closes OMNI-24

OMNI-5 (docs/RESULTS.md missing) could not be reproduced: the file exists in this tree. It was closed without a code change.

Grouped by area

  • Engine and DSL: Relabel remaps EventSlot constants (1), validator holes (6), unowned shared objects (7), determinize keeps zone constraints (8), on_cancel for countered stack items (11), simultaneous wins are a draw (16), cumulative costs and overflow-safe arithmetic (17), step accounting and docs (18, 19), full state_hash (19)
  • Search: one evaluation per MCTS expansion (9), HeuristicAgent no longer sees hidden draws (10)
  • Observation and model: stack tokens (12), target category ids clear of template ids (13), backbone transplant and freeze cover the bias tables (3)
  • Transfer and eval: budget grouping (2), disjoint select/report split (4), eval_checkpoint uses the trained config (14)
  • CLI, generator, training: seat alternation, strict variant parsing, run-log and generate_set errors (15), generator and relabel no-ops (20), empty batch and replay index panics (21), seat-relative tug (22)
  • Tests and hygiene: stronger tests (23), dead code and stale references (24)

Things to know before merging

  • The model input layout changed (7 token classes, one new relation id, new category ids, owner tokens for shared objects). Existing checkpoints will not load.
  • ActionDef.on_cancel changes the hash of every game definition, and generated tug games differ in representation.
  • Results produced before these fixes (relabelled tempo games, transfer summaries, backbone arms, reported CE) should not be trusted. docs/RESULTS.md has a section on it.

Decisions taken

  • OMNI-18: no new event kinds. Transform, SetController, Hide, Detach and variable effects stay silent (documented), and now spend steps. New kinds would shift the fixed 32-slot event vocabulary and change every game's history, and no current game needs triggers on them. Revisit if a game wants them.
  • OMNI-20: the inverted tempo burn pairs cost and damage inversely (damage + cost = 6). Normal games are unchanged because the random draws are identical in every variant.
  • OMNI-22: tug now uses a per-player zero-sum lead resource. Checked against the pre-change generator on 80 seeds: identical acceptance (Normal 57/80, Inverted 63/80) with identical rejection reasons, so the rewrite does not change which games pass.

cargo test --workspace passes and the build has no warnings.

Fixes the findings from the code review, one commit per issue or small group. Most fixes come with a regression test; for most of them I confirmed the test fails on the old code. Closes OMNI-1, Closes OMNI-2, Closes OMNI-3, Closes OMNI-4, Closes OMNI-6, Closes OMNI-7, Closes OMNI-8, Closes OMNI-9, Closes OMNI-10, Closes OMNI-11, Closes OMNI-12, Closes OMNI-13, Closes OMNI-14, Closes OMNI-15, Closes OMNI-16, Closes OMNI-17, Closes OMNI-18, Closes OMNI-19, Closes OMNI-20, Closes OMNI-21, Closes OMNI-22, Closes OMNI-23, Closes OMNI-24 OMNI-5 (docs/RESULTS.md missing) could not be reproduced: the file exists in this tree. It was closed without a code change. ## Grouped by area - Engine and DSL: Relabel remaps EventSlot constants (1), validator holes (6), unowned shared objects (7), determinize keeps zone constraints (8), `on_cancel` for countered stack items (11), simultaneous wins are a draw (16), cumulative costs and overflow-safe arithmetic (17), step accounting and docs (18, 19), full `state_hash` (19) - Search: one evaluation per MCTS expansion (9), HeuristicAgent no longer sees hidden draws (10) - Observation and model: stack tokens (12), target category ids clear of template ids (13), backbone transplant and freeze cover the bias tables (3) - Transfer and eval: budget grouping (2), disjoint select/report split (4), `eval_checkpoint` uses the trained config (14) - CLI, generator, training: seat alternation, strict variant parsing, run-log and `generate_set` errors (15), generator and relabel no-ops (20), empty batch and replay index panics (21), seat-relative tug (22) - Tests and hygiene: stronger tests (23), dead code and stale references (24) ## Things to know before merging - The model input layout changed (7 token classes, one new relation id, new category ids, owner tokens for shared objects). Existing checkpoints will not load. - `ActionDef.on_cancel` changes the hash of every game definition, and generated tug games differ in representation. - Results produced before these fixes (relabelled tempo games, transfer summaries, backbone arms, reported CE) should not be trusted. `docs/RESULTS.md` has a section on it. ## Decisions taken - OMNI-18: no new event kinds. Transform, SetController, Hide, Detach and variable effects stay silent (documented), and now spend steps. New kinds would shift the fixed 32-slot event vocabulary and change every game's history, and no current game needs triggers on them. Revisit if a game wants them. - OMNI-20: the inverted tempo burn pairs cost and damage inversely (damage + cost = 6). Normal games are unchanged because the random draws are identical in every variant. - OMNI-22: tug now uses a per-player zero-sum `lead` resource. Checked against the pre-change generator on 80 seeds: identical acceptance (Normal 57/80, Inverted 63/80) with identical rejection reasons, so the rewrite does not change which games pass. `cargo test --workspace` passes and the build has no warnings.
Death triggers compared EventSlot against a raw attribute id, so relabelled
tempo/skirmish games watched the wrong attribute. The equivalence test now
sweeps several relabel seeds and a new test checks the slot constant.
budget_matches accepted smaller runs, so larger budget rows averaged in the
smaller ones. The reported CE was the min over checkpoints (including step 0)
on the selection set; it is now measured on a disjoint fresh-episode split.
transplant_backbone left rel_tab/tb_tab/rel_as_tab at their fresh init and
freeze_backbone kept training them. Tests now fingerprint all three tables.
- Response actions can no longer target/source the active player's private zone
- source zones get the same visibility check as targets
- OwnerOf zone references are scope-checked against the real scope
- modifiers are validated without action targets
- Delay bodies are validated without targets, iteration or event bindings
Each hole has a negative test.
Shared-zone setup objects were owned by seat 0, so views showed the market as
seat 0's and determinize pooled shared hidden cards with seat 0's. They now
have no owner (reported as NO_REL, 0 in tokens) until they enter a player zone.

determinize fell back to the true hidden identities when 24 shuffles failed
allowed_kinds. It now random-walks from the (valid) truth with constraint-safe
swaps, so it always returns a valid sample.
MCTS evaluated the expanded node for priors and discarded its value, then
evaluated the child again (2 evaluations per expansion, measured: 16 for 8
sims). The expansion value is now backed up.
HeuristicAgent looked ahead on the true state; it now looks ahead in a
determinized world with a resampled RNG stream.
Adds ActionDef.on_cancel (default no-op), run when a stacked action is
countered or its source vanished. gambit.ron and the stack generator use it to
discard the card. Tests check limbo is empty whenever the stack is.
Stack items were only visible as a size and source edges. Each now has a
Stack token (action def, class, source template, actor, depth) with
source/actor relations. Player/number/hidden target ids moved from
100+r/200/201 to a range above MAX_TEMPLATES so they cannot collide with
template ids. This changes the input layout (NUM_CLASSES 6 -> 7, one new
relation id), so checkpoints trained earlier are not loadable.
- eval_checkpoint reads model_cfg and max_history from the checkpoint's
  meta.json (max_history is now recorded by train, selfplay and transfer)
- omnia play alternates seats so first-player advantage is not counted as wins
- Variant::parse is strict and shared by the CLI and the transfer runner
- generate_set returns Result with a list of dropped games instead of
  panicking on I/O errors and silently dropping failures
- RunLog::create/write return io::Result and callers propagate it
- simultaneous wins are shared (0 each) instead of +1 each, so two-player
  payoffs stay zero-sum; dead arithmetic in finish removed
- action costs are summed per resource before the affordability check
- Neg, Div, range_i32, the cost subtraction and two abs() calls no longer
  overflow/panic on i32::MIN / full-range inputs
- Transform, SetController, Hide, Detach, Reveal and Attach now spend a step
  per object; the effects that stay silent (no event) are documented
- MAX_REPEAT constant and docs match the clamp of 64
- state_hash includes the stack, delayed effects, win/lose flags and outcome
- inverted tempo variant now pairs burn cost and damage inversely (the
  multiply-by-1 was a no-op); rng draws are unchanged across variants
- measure() really reports fault_rate and rejects above max_fault_rate
  (the old 'fa > 0' return made the stat always 0)
- CLASS_LABELS documents that only labels 0..16 are permuted
The track was one absolute variable that favoured seat 0 upwards and seat 1
downwards, so the model had to infer its seat from turn parity. Each player
now has a lead resource (summing to zero), so pushing always raises my own
lead. generate() asserts Family::supports instead of silently producing a
normal game for an unsupported variant.
- generator test now requires a minimum acceptance rate and sane stats
- search test checks each agent against random separately
- clone/replay test actually replays the played prefix and the cloned step
- remove unused bincode (observation, training) and blake3 (training)
- remove dead helpers/timers and the unused-variable warning in the CLI
- the 'attn qk' benchmark now times only its own loop
- drop README/DESIGN lines about the Astra film project and astra/ code
Breadway commented 2026-10-08 19:53:22 +08:00
Author
Owner
Copy link

Review (board cleanup 2026-10-08)

Verified at head efd6d26, with #27 stacked on top (62b4d98): cargo build --release --locked --workspace is clean and cargo test --workspace --release --locked passes in full. cargo clippy -D warnings fails on four lints in omnia-dsl (analysis.rs:209, two derivable impls in ir.rs, &mut Vec in relabel.rs), but main has the same four, so this PR did not introduce them. The repo has no CI, so these local runs are the only check.

I read the full diff against OMNI-1 to OMNI-24. No blocking issues. Points worth checking:

  • MCTS expansion (OMNI-9): one evaluation now gives both the priors and the leaf value that gets backed up. No child is stepped into on expansion. This is standard AlphaZero-style expansion, and the new test bounds evals to <= sims.
  • Stack cancel (OMNI-11): cancel_item runs on_cancel through run_top, which still drains triggers. The behaviour matches the old emit + drain, plus the new effect. One minor difference: run_top resets the step budget for the cancel effect.
  • determinize (OMNI-8): the constrained case random-walks from the true assignment for 8*n swaps, so samples are somewhat correlated with the truth when constraints are tight. That is still much better than the old fallback to the truth, and the test asserts at least 10 of 20 samples differ. Non-blocking.
  • Format break: NUM_CLASSES 6->7, rel::NUM_BASE 19->20 and the cat-id moves change embedding shapes. Checkpoints and cached observation datasets from before this PR will not load. RESULTS.md already marks earlier runs as unreliable, so this is expected, but it should be stated explicitly there.

Ready to merge. Merge #25 first, then retarget #27 to main.

## Review (board cleanup 2026-10-08) Verified at head `efd6d26`, with #27 stacked on top (`62b4d98`): `cargo build --release --locked --workspace` is clean and `cargo test --workspace --release --locked` passes in full. `cargo clippy -D warnings` fails on four lints in `omnia-dsl` (analysis.rs:209, two derivable impls in ir.rs, `&mut Vec` in relabel.rs), but main has the same four, so this PR did not introduce them. The repo has no CI, so these local runs are the only check. I read the full diff against OMNI-1 to OMNI-24. No blocking issues. Points worth checking: - **MCTS expansion (OMNI-9):** one evaluation now gives both the priors and the leaf value that gets backed up. No child is stepped into on expansion. This is standard AlphaZero-style expansion, and the new test bounds evals to <= sims. - **Stack cancel (OMNI-11):** `cancel_item` runs `on_cancel` through `run_top`, which still drains triggers. The behaviour matches the old emit + drain, plus the new effect. One minor difference: `run_top` resets the step budget for the cancel effect. - **determinize (OMNI-8):** the constrained case random-walks from the true assignment for 8*n swaps, so samples are somewhat correlated with the truth when constraints are tight. That is still much better than the old fallback to the truth, and the test asserts at least 10 of 20 samples differ. Non-blocking. - **Format break:** `NUM_CLASSES` 6->7, `rel::NUM_BASE` 19->20 and the cat-id moves change embedding shapes. Checkpoints and cached observation datasets from before this PR will not load. RESULTS.md already marks earlier runs as unreliable, so this is expected, but it should be stated explicitly there. Ready to merge. Merge #25 first, then retarget #27 to main.
Breadway commented 2026-10-08 19:54:51 +08:00
Author
Owner
Copy link

Merged 2026-10-08 after review (see the review comment on PR #25): the release build is clean and all workspace tests pass. Closing OMNI-1 to OMNI-4 and OMNI-6 to OMNI-24. Note: observation shapes changed (NUM_CLASSES, relation vocab, cat ids), so checkpoints and datasets from before this merge will not load.

Merged 2026-10-08 after review (see the review comment on PR #25): the release build is clean and all workspace tests pass. Closing OMNI-1 to OMNI-4 and OMNI-6 to OMNI-24. Note: observation shapes changed (NUM_CLASSES, relation vocab, cat ids), so checkpoints and datasets from before this merge will not load.
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
bug
No labels
bug
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
Breadway/omniastra!25
No description provided.