feat: add ISO-backed framedata runtime mapping #23

Open
JayKobe6k wants to merge 50 commits from feat/iso-framedata-runtime-mapping into master
Owner

Summary

  • read fighter DAT and animation members directly from a user-supplied NTSC 1.02 ISO without extracting game data
  • interpret subaction timelines with one-based public frames and immutable action/frame records
  • map runtime Character/Action states through audited main.dol MotionState.anim_id and move_id tables with validated callback metadata
  • evaluate deterministic neutral-costume FigaTree/JOBJ poses for fighter-owned hitboxes, including cross-fighter part remapping and Nana animation fallback
  • expose nominal unit-rate TransN/TransN2 animation-root samples, adjacent deltas, scale-selection flags, and facing projection without presenting them as runtime locomotion
  • recover opcode-20 forward-roll facing flips so standard forward/backward FrameData.roll_end_position() works from ISO root curves while tech and ledge rolls fail explicitly
  • let omitted FrameData.iso_path and high-level get_framedata() queries select the ISO through MELEE_ISO_PATH, with source-keyed caches and validated disc provenance
  • expose script-proven ISO hitbox timing and size while representing unavailable runtime geometry, locomotion, facing, and projectile fields as None
  • derive per-hitbox active ranges from the common segment sequence for both CSV and ISO results
  • keep get_raw_framedata_csv() explicitly historical and fail closed for article/projectile-dependent ISO timing instead of returning partial grouped-special results
  • deprecate FrameData while explicitly rejecting unsupported historical runtime geometry and ambiguous article hitboxes

Validation

  • python test.py (389 tests)
  • python -m compileall -q melee test.py
  • focused Ruff and Pyright checks for the ISO pose, subaction, and framedata extraction modules (0 diagnostics)
  • strict Sphinx HTML build with warnings as errors
  • canonical NTSC 1.02 Fox jab pose matches checked-in runtime coordinates within 1e-6
  • canonical high-level query sweep: 8,828 playable action states, 8,334 complete results, 494 explicit article/projectile uncertainty errors, 0 unexpected failures
  • canonical animation-root sweep: 7,932 animation-bearing states, 0 extraction errors
  • canonical standard-roll sweep: 3,458 per-frame comparisons across all 52 forward/backward rolls, maximum historical CSV difference 4.58e-05
  • retail NTSC 1.02 audit: 33 fighters, 10,658 DAT actions, 11,477 runtime motion states, and 55,056 executable callback pointers

Current Limits

  • article/projectile timelines, secondary dynamics, inverse kinematics, model-part changes, incoming-pose blending, and callback-specific runtime semantics remain future work
  • compatibility range, collision, tech/ledge-roll displacement, and other runtime-only geometry methods remain unavailable until those dependencies have an ISO-native model
  • get_raw_framedata_csv() and remaining direct .framedata dictionary consumers still depend on the historical CSV during incremental migration
## Summary - read fighter DAT and animation members directly from a user-supplied NTSC 1.02 ISO without extracting game data - interpret subaction timelines with one-based public frames and immutable action/frame records - map runtime `Character`/`Action` states through audited `main.dol` `MotionState.anim_id` and `move_id` tables with validated callback metadata - evaluate deterministic neutral-costume FigaTree/JOBJ poses for fighter-owned hitboxes, including cross-fighter part remapping and Nana animation fallback - expose nominal unit-rate `TransN`/`TransN2` animation-root samples, adjacent deltas, scale-selection flags, and facing projection without presenting them as runtime locomotion - recover opcode-20 forward-roll facing flips so standard forward/backward `FrameData.roll_end_position()` works from ISO root curves while tech and ledge rolls fail explicitly - let omitted `FrameData.iso_path` and high-level `get_framedata()` queries select the ISO through `MELEE_ISO_PATH`, with source-keyed caches and validated disc provenance - expose script-proven ISO hitbox timing and size while representing unavailable runtime geometry, locomotion, facing, and projectile fields as `None` - derive per-hitbox active ranges from the common segment sequence for both CSV and ISO results - keep `get_raw_framedata_csv()` explicitly historical and fail closed for article/projectile-dependent ISO timing instead of returning partial grouped-special results - deprecate `FrameData` while explicitly rejecting unsupported historical runtime geometry and ambiguous article hitboxes ## Validation - `python test.py` (389 tests) - `python -m compileall -q melee test.py` - focused Ruff and Pyright checks for the ISO pose, subaction, and framedata extraction modules (0 diagnostics) - strict Sphinx HTML build with warnings as errors - canonical NTSC 1.02 Fox jab pose matches checked-in runtime coordinates within `1e-6` - canonical high-level query sweep: 8,828 playable action states, 8,334 complete results, 494 explicit article/projectile uncertainty errors, 0 unexpected failures - canonical animation-root sweep: 7,932 animation-bearing states, 0 extraction errors - canonical standard-roll sweep: 3,458 per-frame comparisons across all 52 forward/backward rolls, maximum historical CSV difference `4.58e-05` - retail NTSC 1.02 audit: 33 fighters, 10,658 DAT actions, 11,477 runtime motion states, and 55,056 executable callback pointers ## Current Limits - article/projectile timelines, secondary dynamics, inverse kinematics, model-part changes, incoming-pose blending, and callback-specific runtime semantics remain future work - compatibility range, collision, tech/ledge-roll displacement, and other runtime-only geometry methods remain unavailable until those dependencies have an ISO-native model - `get_raw_framedata_csv()` and remaining direct `.framedata` dictionary consumers still depend on the historical CSV during incremental migration
feat: add ISO-backed framedata parsing
All checks were successful
Linux tests / Python 3.12 (push) Successful in 33s
Linux tests / Python 3.13 (push) Successful in 33s
Linux tests / Python 3.11 (push) Successful in 35s
da93b77b3e
fix: handle retail framedata script layouts
All checks were successful
Linux tests / Python 3.12 (push) Successful in 34s
Linux tests / Python 3.13 (push) Successful in 35s
Linux tests / Python 3.11 (push) Successful in 35s
d67b032627
feat: map runtime actions from Melee DOL
All checks were successful
Linux tests / Python 3.13 (push) Successful in 35s
Linux tests / Python 3.12 (push) Successful in 35s
Linux tests / Python 3.11 (push) Successful in 37s
Linux tests / Python 3.11 (pull_request) Successful in 36s
Linux tests / Python 3.13 (pull_request) Successful in 34s
Linux tests / Python 3.12 (pull_request) Successful in 35s
ba5200f395
JayKobe6k left a comment

Found four actionable correctness issues in the ISO-backed runtime mapping and subaction interpretation.

Found four actionable correctness issues in the ISO-backed runtime mapping and subaction interpretation.
@ -0,0 +350,4 @@
values.update(_fields(words, specs))
if opcode == 11:
values["size"] = values["size_raw"] / 256.0
values["bone_local_x"] = values["bone_local_x_raw"] / 256.0
Author
Owner

[P2] These effective axes are reversed relative to the runtime collision vector: ftAction_8007121C assigns encoded z_offset to HitCapsule.b_offset.x and encoded x_offset to b_offset.z, while this maps source X to public X and source Z to public Z. For example, retail Fox jab is reported as (x=0, z=4.65625) although the runtime vector is (x=4.65625, z=0). Please swap the effective X/Z assignments (retaining separately named raw fields if needed) and correct the synthetic assertion.

[P2] These effective axes are reversed relative to the runtime collision vector: `ftAction_8007121C` assigns encoded `z_offset` to `HitCapsule.b_offset.x` and encoded `x_offset` to `b_offset.z`, while this maps source X to public X and source Z to public Z. For example, retail Fox jab is reported as `(x=0, z=4.65625)` although the runtime vector is `(x=4.65625, z=0)`. Please swap the effective X/Z assignments (retaining separately named raw fields if needed) and correct the synthetic assertion.
JayKobe6k marked this conversation as resolved
@ -0,0 +505,4 @@
target = int(command.parameter("target"))
if target % 4 or target < 0 or target >= len(data):
raise SubactionParseError(f"{context}: opcode {opcode} targets invalid data offset 0x{target:X}")
if opcode == 7 and target <= pc:
Author
Owner

[P1] Treating every backward goto as a loop skips valid tail jumps into earlier shared routines. On the retail ISO, Fox FTILT_HIGH consists of a backward jump to the terminating FTILT_MID script, but this returns only opcode 7 and no hitboxes, making ISO-backed is_attack() false instead of exposing the frame-5 hitboxes. Captain Falcon's FSMASH_MID similarly skips the shared cleanup and leaves its hitboxes active through frame 64 instead of frame 21. Please follow backward targets and classify a loop only after detecting actual repeated control flow, with a terminating backward-jump regression test.

[P1] Treating every backward `goto` as a loop skips valid tail jumps into earlier shared routines. On the retail ISO, Fox `FTILT_HIGH` consists of a backward jump to the terminating `FTILT_MID` script, but this returns only opcode 7 and no hitboxes, making ISO-backed `is_attack()` false instead of exposing the frame-5 hitboxes. Captain Falcon's `FSMASH_MID` similarly skips the shared cleanup and leaves its hitboxes active through frame 64 instead of frame 21. Please follow backward targets and classify a loop only after detecting actual repeated control flow, with a terminating backward-jump regression test.
JayKobe6k marked this conversation as resolved
@ -0,0 +322,4 @@
dat_index = self.dat_action_index(character, action)
if dat_index is None:
return None
return self.action(FIGHTER_KINDS[character.value].code, dat_index)
Author
Owner

[P2] This direct PlNn.dat lookup skips Nana's runtime fallback. ftData_80085FD4 selects Popo's corresponding action record for non-demo Nana whenever Nana's x14 animation pointer is null; on the retail ISO, Nana jab therefore returns a 16-frame script-only timeline here instead of the inherited 30-frame endpoint/29 snapshots, with the same problem affecting grab and rolls. Please apply the Nana-to-Popo record fallback used by the engine and cover an inherited common action.

[P2] This direct `PlNn.dat` lookup skips Nana's runtime fallback. `ftData_80085FD4` selects Popo's corresponding action record for non-demo Nana whenever Nana's `x14` animation pointer is null; on the retail ISO, Nana jab therefore returns a 16-frame script-only timeline here instead of the inherited 30-frame endpoint/29 snapshots, with the same problem affecting grab and rolls. Please apply the Nana-to-Popo record fallback used by the engine and cover an inherited common action.
Author
Owner

[P3] A mapped DAT index can still denote an absent animation, but this always returns its empty record despite the documented None contract. Retail Fox NEUTRAL_ATTACK_3 maps to index 48, whose animation size, runtime animation pointer, and script are all zero, yet action_for_state() returns an empty ActionRecord. After applying fighter-specific fallback rules, please return None for mapped-but-empty actions and test this common-state case.

[P3] A mapped DAT index can still denote an absent animation, but this always returns its empty record despite the documented `None` contract. Retail Fox `NEUTRAL_ATTACK_3` maps to index 48, whose animation size, runtime animation pointer, and script are all zero, yet `action_for_state()` returns an empty `ActionRecord`. After applying fighter-specific fallback rules, please return `None` for mapped-but-empty actions and test this common-state case.
JayKobe6k marked this conversation as resolved
test: model animated article actions
Some checks failed
Linux tests / Python 3.12 (push) Successful in 36s
Linux tests / Python 3.13 (push) Successful in 36s
Linux tests / Python 3.11 (push) Successful in 38s
Linux tests / Python 3.12 (pull_request) Failing after 8s
Linux tests / Python 3.13 (pull_request) Successful in 34s
Linux tests / Python 3.11 (pull_request) Successful in 6m47s
76286764d9
Author
Owner

Review comment #651: Fixed in 2202f97. Backward gotos now execute until they actually re-enter previously seen control flow, so terminating shared routines are preserved. Added a higher-address-to-lower-address terminating tail-jump regression. Focused tests: python -m unittest test.DiscFrameDataTests.test_control_flow_and_lossless_command_lengths test.DiscFrameDataTests.test_public_api_combat_timeline_and_local_geometry (2 passed). Retail verification now reports Fox FTILT_HIGH active on frames 5-8 and Captain Falcon FSMASH_MID active on frames 18-21. Full branch: python test.py (335 passed).

Review comment #651: Fixed in 2202f97. Backward gotos now execute until they actually re-enter previously seen control flow, so terminating shared routines are preserved. Added a higher-address-to-lower-address terminating tail-jump regression. Focused tests: `python -m unittest test.DiscFrameDataTests.test_control_flow_and_lossless_command_lengths test.DiscFrameDataTests.test_public_api_combat_timeline_and_local_geometry` (2 passed). Retail verification now reports Fox FTILT_HIGH active on frames 5-8 and Captain Falcon FSMASH_MID active on frames 18-21. Full branch: `python test.py` (335 passed).
Author
Owner

Review comment #652: Fixed in 2202f97. Effective hitbox X now uses the encoded z_offset, and effective Z uses encoded x_offset, matching ftAction_8007121C; the synthetic geometry assertions were corrected. Focused tests: python -m unittest test.DiscFrameDataTests.test_control_flow_and_lossless_command_lengths test.DiscFrameDataTests.test_public_api_combat_timeline_and_local_geometry (2 passed). Retail Fox jab now reports (x=4.65625, z=0). Full branch: python test.py (335 passed); uvx ruff check melee/_subaction.py melee/disc_framedata.py passed.

Review comment #652: Fixed in 2202f97. Effective hitbox X now uses the encoded `z_offset`, and effective Z uses encoded `x_offset`, matching `ftAction_8007121C`; the synthetic geometry assertions were corrected. Focused tests: `python -m unittest test.DiscFrameDataTests.test_control_flow_and_lossless_command_lengths test.DiscFrameDataTests.test_public_api_combat_timeline_and_local_geometry` (2 passed). Retail Fox jab now reports `(x=4.65625, z=0)`. Full branch: `python test.py` (335 passed); `uvx ruff check melee/_subaction.py melee/disc_framedata.py` passed.
Author
Owner

Review comment #653: Fixed in 3f97918, with the article-action fixture correction in 7628676. action_for_state() now resolves a Nana entry with no local animation payload through the corresponding Popo record, while low-level action("Nn", index) remains unchanged. Added synthetic Nana/Popo fallback coverage. Focused tests: python -m unittest test.DiscFrameDataTests.test_public_api_combat_timeline_and_local_geometry test.DiscFrameDataTests.test_nana_runtime_actions_inherit_missing_popo_animations (2 passed). Retail verification now gives Nana jab/grab/forward-roll timelines of 29/29/31 snapshots. Full branch: python test.py (335 passed).

Review comment #653: Fixed in 3f97918, with the article-action fixture correction in 7628676. `action_for_state()` now resolves a Nana entry with no local animation payload through the corresponding Popo record, while low-level `action("Nn", index)` remains unchanged. Added synthetic Nana/Popo fallback coverage. Focused tests: `python -m unittest test.DiscFrameDataTests.test_public_api_combat_timeline_and_local_geometry test.DiscFrameDataTests.test_nana_runtime_actions_inherit_missing_popo_animations` (2 passed). Retail verification now gives Nana jab/grab/forward-roll timelines of 29/29/31 snapshots. Full branch: `python test.py` (335 passed).
Author
Owner

Review comment #654: Fixed in 3f97918, with the animated-article fixture correction in 7628676. After fighter-specific fallback, action_for_state() now returns None when the resolved DAT entry has no animation payload; animated article-producing states retain their explicit unsupported-hitbox error. Added mapped-but-empty Fox and article-state coverage. Focused tests: python -m unittest test.DiscFrameDataTests.test_deprecated_framedata_iso_timing_facade test.DiscFrameDataTests.test_public_api_combat_timeline_and_local_geometry test.DiscFrameDataTests.test_nana_runtime_actions_inherit_missing_popo_animations (3 passed). Retail Fox NEUTRAL_ATTACK_3 now returns None; full python test.py passed 335 tests.

Review comment #654: Fixed in 3f97918, with the animated-article fixture correction in 7628676. After fighter-specific fallback, `action_for_state()` now returns `None` when the resolved DAT entry has no animation payload; animated article-producing states retain their explicit unsupported-hitbox error. Added mapped-but-empty Fox and article-state coverage. Focused tests: `python -m unittest test.DiscFrameDataTests.test_deprecated_framedata_iso_timing_facade test.DiscFrameDataTests.test_public_api_combat_timeline_and_local_geometry test.DiscFrameDataTests.test_nana_runtime_actions_inherit_missing_popo_animations` (3 passed). Retail Fox NEUTRAL_ATTACK_3 now returns `None`; full `python test.py` passed 335 tests.
JayKobe6k left a comment

Found four actionable correctness issues in the ISO-backed runtime mapping and compatibility facade.

Found four actionable correctness issues in the ISO-backed runtime mapping and compatibility facade.
@ -0,0 +470,4 @@
pc += 4
continue
if opcode == 2:
next_time = max(time, float(command.parameter("frame")))
Author
Owner

[P2] max(time, target) discards the runtime timer's negative remainder when an asynchronous timer targets an earlier frame, so later synchronous waits are added to elapsed time instead of the remaining timer. Retail Yoshi DAIR consequently reports IASA on frame 56 instead of 53, and Samus BAT_SWING_3 shifts its hitboxes from frames 13-15 to 16-18. Please track the command timer separately from elapsed animation time and cover a past absolute timer followed by relative timers.

[P2] `max(time, target)` discards the runtime timer's negative remainder when an asynchronous timer targets an earlier frame, so later synchronous waits are added to elapsed time instead of the remaining timer. Retail Yoshi `DAIR` consequently reports IASA on frame 56 instead of 53, and Samus `BAT_SWING_3` shifts its hitboxes from frames 13-15 to 16-18. Please track the command timer separately from elapsed animation time and cover a past absolute timer followed by relative timers.
JayKobe6k marked this conversation as resolved
@ -0,0 +325,4 @@
record = self.action(FIGHTER_KINDS[character.value].code, dat_index)
if character is Character.NANA and not record.animation_size:
record = self.action(FIGHTER_KINDS[Character.POPO.value].code, dat_index)
return record if record.animation_size else None
Author
Owner

[P2] Filtering solely on animation_size drops executable script-only records. Retail Popo POPO_SPECIAL_S_1 maps to DAT index 314, whose animation size is zero but whose relocated script has 75 commands and 25 hitbox events; action_for_state() therefore returns None during Squall Hammer and the facade reports no attack or frame timing. Please reject only records lacking both animation and script after inheritance resolution, and add a script-only runtime-state regression test.

[P2] Filtering solely on `animation_size` drops executable script-only records. Retail Popo `POPO_SPECIAL_S_1` maps to DAT index 314, whose animation size is zero but whose relocated script has 75 commands and 25 hitbox events; `action_for_state()` therefore returns `None` during Squall Hammer and the facade reports no attack or frame timing. Please reject only records lacking both animation and script after inheritance resolution, and add a script-only runtime-state regression test.
JayKobe6k marked this conversation as resolved
@ -41,2 +43,3 @@
"""
def __init__(self, write=False):
def __init__(self, write=False, *, iso_path=None):
warnings.warn(
Author
Owner

[P2] This unconditional warning is also reached through unchanged public APIs that internally construct FrameData, including get_framedata(), CharacterState, and SimpleControls. With the common DeprecationWarning-as-error policy, those APIs now raise before doing any work even though their callers never selected the deprecated class and cannot substitute DiscFrameData there. Please migrate or suppress the internal construction paths while retaining the warning for direct deprecated use.

[P2] This unconditional warning is also reached through unchanged public APIs that internally construct `FrameData`, including `get_framedata()`, `CharacterState`, and `SimpleControls`. With the common `DeprecationWarning`-as-error policy, those APIs now raise before doing any work even though their callers never selected the deprecated class and cannot substitute `DiscFrameData` there. Please migrate or suppress the internal construction paths while retaining the warning for direct deprecated use.
JayKobe6k marked this conversation as resolved
@ -109,0 +134,4 @@
"ISO-backed hitbox timing is unavailable for special states without fighter hitboxes; "
"the move may create an unparsed article or projectile"
)
return tuple(frame.local_frame for frame in record.timeline.frames if frame.active_hitboxes)
Author
Owner

[P2] These snapshots include script-declared hitboxes whose creation is runtime-conditional. Retail Fox DAMAGE_FLY_HIGH has only a requires_thrown_hitbox_owner generation, which the engine skips after an ordinary hit, but the facade reports an attack active on frames 1-28. Because this query has no owner context, please exclude conditional generations or report the timing as unavailable, and cover this state.

[P2] These snapshots include script-declared hitboxes whose creation is runtime-conditional. Retail Fox `DAMAGE_FLY_HIGH` has only a `requires_thrown_hitbox_owner` generation, which the engine skips after an ordinary hit, but the facade reports an attack active on frames 1-28. Because this query has no owner context, please exclude conditional generations or report the timing as unavailable, and cover this state.
JayKobe6k marked this conversation as resolved
fix: limit framedata deprecation warnings
All checks were successful
Linux tests / Python 3.12 (push) Successful in 35s
Linux tests / Python 3.13 (push) Successful in 36s
Linux tests / Python 3.11 (pull_request) Successful in 37s
Linux tests / Python 3.11 (push) Successful in 38s
Linux tests / Python 3.12 (pull_request) Successful in 33s
Linux tests / Python 3.13 (pull_request) Successful in 34s
6eff0a4437
Author
Owner

Review comment #807: Fixed in 1bd7274. The compatibility facade now omits hitboxes requiring a runtime thrown-hitbox owner, while the lower-level DiscFrameData timeline remains lossless and retains the conditional candidate. Added explicit low-level-retention and facade-filtering coverage. Focused tests: python -m unittest test.DiscFrameDataTests.test_deprecated_framedata_iso_timing_facade test.DiscFrameDataTests.test_public_api_combat_timeline_and_local_geometry test.DiscFrameDataTests.test_nana_runtime_actions_inherit_missing_popo_animations (3 passed). Retail Fox DAMAGE_FLY_HIGH remains present in the low-level timeline but the facade now reports no attack and preserves its 28-frame duration. Full suite: python test.py (336 passed); py_compile passed.

Review comment #807: Fixed in 1bd7274. The compatibility facade now omits hitboxes requiring a runtime thrown-hitbox owner, while the lower-level `DiscFrameData` timeline remains lossless and retains the conditional candidate. Added explicit low-level-retention and facade-filtering coverage. Focused tests: `python -m unittest test.DiscFrameDataTests.test_deprecated_framedata_iso_timing_facade test.DiscFrameDataTests.test_public_api_combat_timeline_and_local_geometry test.DiscFrameDataTests.test_nana_runtime_actions_inherit_missing_popo_animations` (3 passed). Retail Fox DAMAGE_FLY_HIGH remains present in the low-level timeline but the facade now reports no attack and preserves its 28-frame duration. Full suite: `python test.py` (336 passed); `py_compile` passed.
Author
Owner

Review comment #806: Fixed in 3a6eb11. After Nana inheritance resolution, action_for_state() now treats a record as empty only when it has neither animation data nor a relocated subaction script. Added synthetic script-only runtime mapping coverage and corrected the documented contract. Focused tests: python -m unittest test.DiscFrameDataTests.test_public_api_combat_timeline_and_local_geometry test.DiscFrameDataTests.test_nana_runtime_actions_inherit_missing_popo_animations (2 passed). Retail Popo POPO_SPECIAL_S_1 now returns DAT 314 with 75 commands, 66 frames, and 25 hitbox events, while Fox NEUTRAL_ATTACK_3 remains None. Full suite: python test.py (336 passed); PR Ruff target set and py_compile passed.

Review comment #806: Fixed in 3a6eb11. After Nana inheritance resolution, `action_for_state()` now treats a record as empty only when it has neither animation data nor a relocated subaction script. Added synthetic script-only runtime mapping coverage and corrected the documented contract. Focused tests: `python -m unittest test.DiscFrameDataTests.test_public_api_combat_timeline_and_local_geometry test.DiscFrameDataTests.test_nana_runtime_actions_inherit_missing_popo_animations` (2 passed). Retail Popo POPO_SPECIAL_S_1 now returns DAT 314 with 75 commands, 66 frames, and 25 hitbox events, while Fox NEUTRAL_ATTACK_3 remains `None`. Full suite: `python test.py` (336 passed); PR Ruff target set and `py_compile` passed.
Author
Owner

Review comment #805: Fixed in 25c04f7. The interpreter now keeps the signed command timer separate from elapsed animation time and preserves negative carry across later synchronous waits; the cycle state includes that timer. Added an overdue-absolute-then-relative timer regression. Focused tests: python -m unittest test.DiscFrameDataTests.test_malformed_iso_dat_and_subactions_fail_with_context test.DiscFrameDataTests.test_control_flow_and_lossless_command_lengths (2 passed). Retail verification now reports Yoshi DAIR IASA at frame 53 and Samus BAT_SWING_3 hitboxes on frames 13-15. Full suite: python test.py (336 passed); PR Ruff target set and py_compile passed.

Review comment #805: Fixed in 25c04f7. The interpreter now keeps the signed command timer separate from elapsed animation time and preserves negative carry across later synchronous waits; the cycle state includes that timer. Added an overdue-absolute-then-relative timer regression. Focused tests: `python -m unittest test.DiscFrameDataTests.test_malformed_iso_dat_and_subactions_fail_with_context test.DiscFrameDataTests.test_control_flow_and_lossless_command_lengths` (2 passed). Retail verification now reports Yoshi DAIR IASA at frame 53 and Samus BAT_SWING_3 hitboxes on frames 13-15. Full suite: `python test.py` (336 passed); PR Ruff target set and `py_compile` passed.
Author
Owner

Review comment #808: Fixed in 6eff0a4. Internal CSV compatibility construction in get_framedata(), CharacterState, and SimpleControls now suppresses only the FrameData constructor warning through a private switch; direct deprecated construction still warns. Added warnings-as-errors coverage for all three public paths. Focused tests: python -m unittest test.SLPFile.test_internal_framedata_construction_does_not_warn test.DiscFrameDataTests.test_deprecated_framedata_iso_timing_facade (2 passed), plus a fresh-process python -W error::DeprecationWarning probe covering all three APIs. Full suite: python test.py (336 passed); py_compile passed.

Review comment #808: Fixed in 6eff0a4. Internal CSV compatibility construction in `get_framedata()`, `CharacterState`, and `SimpleControls` now suppresses only the `FrameData` constructor warning through a private switch; direct deprecated construction still warns. Added warnings-as-errors coverage for all three public paths. Focused tests: `python -m unittest test.SLPFile.test_internal_framedata_construction_does_not_warn test.DiscFrameDataTests.test_deprecated_framedata_iso_timing_facade` (2 passed), plus a fresh-process `python -W error::DeprecationWarning` probe covering all three APIs. Full suite: `python test.py` (336 passed); `py_compile` passed.
JayKobe6k left a comment

Found four actionable correctness and contract issues in the ISO-backed framedata implementation.

Found four actionable correctness and contract issues in the ISO-backed framedata implementation.
@ -7,0 +62,4 @@
retains those script-declared candidates and their condition flag.
As with ``PlayerState.action_frame``, every exported ``local_frame`` and
``FrameSnapshot.local_frame`` is one-indexed. ``ActionRecord.frame(1)`` always
Author
Owner

[P3] This always guarantee is false for public empty DAT records: DiscFrameData.action("Fx", 48) returns an ActionRecord whose timeline has no snapshots, so frame(1) raises IndexError. Please either give empty records an initial snapshot or qualify the contract to records with a non-empty timeline and cover that behavior.

[P3] This `always` guarantee is false for public empty DAT records: `DiscFrameData.action("Fx", 48)` returns an `ActionRecord` whose timeline has no snapshots, so `frame(1)` raises `IndexError`. Please either give empty records an initial snapshot or qualify the contract to records with a non-empty timeline and cover that behavior.
JayKobe6k marked this conversation as resolved
@ -0,0 +137,4 @@
header = stream.read(0x430)
if len(header) != 0x430:
raise DiscImageError("could not read the complete GameCube disc header")
if header[0:6] != b"GALE01":
Author
Owner

[P2] This accepts any image that retains the GALE01 revision-2 header as canonical NTSC 1.02, after which DiscFrameData reports the pinned doldecomp provenance and reads fixed retail addresses. The PR's synthetic DOL, and realistic modded images, pass these checks and can return non-retail mappings despite the documented requirement not to apply NTSC 1.02 knowledge to modded files. Please verify main.dol against the pinned retail fingerprint before exposing build metadata or runtime mappings, and test rejection of a same-header modified DOL.

[P2] This accepts any image that retains the `GALE01` revision-2 header as canonical NTSC 1.02, after which `DiscFrameData` reports the pinned doldecomp provenance and reads fixed retail addresses. The PR's synthetic DOL, and realistic modded images, pass these checks and can return non-retail mappings despite the documented requirement not to apply NTSC 1.02 knowledge to modded files. Please verify `main.dol` against the pinned retail fingerprint before exposing build metadata or runtime mappings, and test rejection of a same-header modified DOL.
JayKobe6k marked this conversation as resolved
@ -0,0 +280,4 @@
def _animation_snapshot_count(frame_count: float) -> int:
if frame_count <= 0:
return 0
return max(1, math.ceil(frame_count) - 1)
Author
Owner

[P2] This always removes the FigaTree endpoint even when a reached state-changing command executes there. Retail Bowser action 362 has animation_frame_count == 32 and creates its hitbox at source/public frame 32; the event is retained but snapshots stop at 31, so the ISO facade reports is_attack() == False and no hitbox instead of the frame-32 hitbox present in the checked-in capture. Please preserve an endpoint snapshot when a reached event occurs there, or report the timing as unavailable, and add this action as a regression test.

[P2] This always removes the FigaTree endpoint even when a reached state-changing command executes there. Retail Bowser action 362 has `animation_frame_count == 32` and creates its hitbox at source/public frame 32; the event is retained but snapshots stop at 31, so the ISO facade reports `is_attack() == False` and no hitbox instead of the frame-32 hitbox present in the checked-in capture. Please preserve an endpoint snapshot when a reached event occurs there, or report the timing as unavailable, and add this action as a regression test.
JayKobe6k marked this conversation as resolved
@ -109,0 +131,4 @@
if record is None:
return ()
if not record.timeline.hitbox_generations and self.is_bmove(character, action):
raise DiscFrameDataError(
Author
Owner

[P2] This intentional exception also escapes through SimpleControls before its existing special-action fallback. Continuing Fox NEUTRAL_B when PlayerState.action is LASER_GUN_PULL with FrameData(iso_path=...) calls is_attack() here and raises instead of returning AttackFrameData, so the new facade cannot be used with the public controls API for article specials. Please classify known specials before this hitbox query, or normalize unavailable timing inside SimpleControls, and add an end-to-end continuation test.

[P2] This intentional exception also escapes through `SimpleControls` before its existing special-action fallback. Continuing Fox `NEUTRAL_B` when `PlayerState.action` is `LASER_GUN_PULL` with `FrameData(iso_path=...)` calls `is_attack()` here and raises instead of returning `AttackFrameData`, so the new facade cannot be used with the public controls API for article specials. Please classify known specials before this hitbox query, or normalize unavailable timing inside `SimpleControls`, and add an end-to-end continuation test.
JayKobe6k marked this conversation as resolved
fix: recognize ISO-backed article specials
All checks were successful
Linux tests / Python 3.13 (push) Successful in 36s
Linux tests / Python 3.12 (push) Successful in 36s
Linux tests / Python 3.11 (push) Successful in 37s
Linux tests / Python 3.11 (pull_request) Successful in 38s
Linux tests / Python 3.12 (pull_request) Successful in 33s
Linux tests / Python 3.13 (pull_request) Successful in 34s
c1b12bfbbf
Author
Owner

Review comment #846: Fixed in d01b477. GameCubeDisc.read_dol() now verifies the canonical NTSC 1.02 main.dol SHA-1, and DiscFrameData forces that check before exposing build provenance or fixed-address mappings. Added same-header modified-DOL rejection coverage plus private synthetic-fixture digest overrides. Tests: python -m unittest test.DiscFrameDataTests (10 passed on the final branch), python test.py (339 passed), PR Ruff target set and py_compile passed.

Review comment #846: Fixed in d01b477. `GameCubeDisc.read_dol()` now verifies the canonical NTSC 1.02 `main.dol` SHA-1, and `DiscFrameData` forces that check before exposing build provenance or fixed-address mappings. Added same-header modified-DOL rejection coverage plus private synthetic-fixture digest overrides. Tests: `python -m unittest test.DiscFrameDataTests` (10 passed on the final branch), `python test.py` (339 passed), PR Ruff target set and `py_compile` passed.
Author
Owner

Review comment #847: Fixed in 243071b. Exclusive FigaTree endpoints remain omitted for ordinary scripts, but reached hitbox or IASA events now extend snapshots through their actual frame. Added a regression proving an ordinary 8.0 endpoint stays at 7 snapshots while a frame-8 hitbox/IASA event produces the required eighth snapshot. Tests: focused DiscFrameDataTests checks passed, python test.py (339 passed), PR Ruff target set and py_compile passed.

Review comment #847: Fixed in 243071b. Exclusive FigaTree endpoints remain omitted for ordinary scripts, but reached hitbox or IASA events now extend snapshots through their actual frame. Added a regression proving an ordinary 8.0 endpoint stays at 7 snapshots while a frame-8 hitbox/IASA event produces the required eighth snapshot. Tests: focused `DiscFrameDataTests` checks passed, `python test.py` (339 passed), PR Ruff target set and `py_compile` passed.
Author
Owner

Review comment #848: Fixed in c1b12bf. SimpleControls now recognizes a requested known special action before consulting hitbox timing, while direct ISO-backed FrameData.is_attack() calls still raise for unavailable article/projectile timing. Added an end-to-end Fox neutral-B continuation regression that returns AttackFrameData for LASER_GUN_PULL. Tests: focused controls run (64 passed), DiscFrameDataTests (10 passed), python test.py (339 passed), and py_compile passed.

Review comment #848: Fixed in c1b12bf. `SimpleControls` now recognizes a requested known special action before consulting hitbox timing, while direct ISO-backed `FrameData.is_attack()` calls still raise for unavailable article/projectile timing. Added an end-to-end Fox neutral-B continuation regression that returns `AttackFrameData` for `LASER_GUN_PULL`. Tests: focused controls run (64 passed), `DiscFrameDataTests` (10 passed), `python test.py` (339 passed), and `py_compile` passed.
Author
Owner

Review comment #849: Fixed in 243071b. Empty low-level DAT records remain empty rather than fabricating a frame, and the documentation now limits frame(1) to non-empty timelines and explicitly documents frame_count == 0 / IndexError behavior. Added assertions for empty Fox DAT index 48. Tests: DiscFrameDataTests (10 passed), python test.py (339 passed), strict Sphinx HTML build with warnings as errors passed.

Review comment #849: Fixed in 243071b. Empty low-level DAT records remain empty rather than fabricating a frame, and the documentation now limits `frame(1)` to non-empty timelines and explicitly documents `frame_count == 0` / `IndexError` behavior. Added assertions for empty Fox DAT index 48. Tests: `DiscFrameDataTests` (10 passed), `python test.py` (339 passed), strict Sphinx HTML build with warnings as errors passed.
JayKobe6k left a comment

Found five actionable parser and framedata correctness issues.

Found five actionable parser and framedata correctness issues.
@ -0,0 +155,4 @@
f"FST size 0x{self.fst_size:X} exceeds the header maximum 0x{fst_max_size:X}"
)
stream.seek(self.fst_offset)
fst = stream.read(self.fst_size)
Author
Owner

[P2] This read, the analogous DOL read at line 286, and read_member() materialize sizes controlled by unauthenticated image metadata. The current checks bound them only to the supplied file length, while fst_max_size comes from the same header, so a standard 1.46 GB sparse ISO can request almost 1.46 GB in one allocation before structural parsing or DOL fingerprinting and terminate the process instead of raising DiscImageError. Please enforce audited upper bounds for FST, DOL, and member sizes before reading them and add oversized-but-in-bounds sparse-image tests.

[P2] This read, the analogous DOL read at line 286, and `read_member()` materialize sizes controlled by unauthenticated image metadata. The current checks bound them only to the supplied file length, while `fst_max_size` comes from the same header, so a standard 1.46 GB sparse ISO can request almost 1.46 GB in one allocation before structural parsing or DOL fingerprinting and terminate the process instead of raising `DiscImageError`. Please enforce audited upper bounds for FST, DOL, and member sizes before reading them and add oversized-but-in-bounds sparse-image tests.
JayKobe6k marked this conversation as resolved
@ -0,0 +91,4 @@
def _validate_reference_chain(self, offset: int, index: int) -> None:
seen = set()
while offset != 0xFFFFFFFF:
Author
Owner

[P3] HSD external-reference links may terminate with either 0 or 0xFFFFFFFF; the pinned HSDRaw implementation explicitly stops on both. This loop follows a zero terminator into data offset zero, so a valid zero-terminated reference chain is rejected as cyclic. Please stop after reading either terminal link value and add a minimal zero-terminated reference regression test.

[P3] HSD external-reference links may terminate with either `0` or `0xFFFFFFFF`; the pinned HSDRaw implementation explicitly stops on both. This loop follows a zero terminator into data offset zero, so a valid zero-terminated reference chain is rejected as cyclic. Please stop after reading either terminal link value and add a minimal zero-terminated reference regression test.
JayKobe6k marked this conversation as resolved
@ -0,0 +738,4 @@
frame_count = max(
frame_count,
max((event.local_frame for event in events), default=0),
_frame(iasa_time) if iasa_time is not None else 0,
Author
Owner

[P2] This lets an IASA marker after a finite animation endpoint extend the action with unreachable snapshots. In the retail ISO, Fox f-smash ends at source time 40 but its script's stale IASA is at 46, so this now makes timeline.frame_count and the public FrameData.frame_count()/iasa() return 46 and exposes frames 41-46. The pinned runtime dispatches commands before anim_cb, which permits the endpoint fix, but ftCo_AttackS4_Anim transitions as soon as the animation has no frames remaining, so time 46 is never reached (DK fair is similarly inflated from endpoint 60 to 85). Please only let IASA extend a FigaTree-bounded timeline when it occurs at the reachable endpoint, while retaining later raw script timing, and add a post-endpoint IASA regression test.

[P2] This lets an IASA marker after a finite animation endpoint extend the action with unreachable snapshots. In the retail ISO, Fox f-smash ends at source time 40 but its script's stale IASA is at 46, so this now makes `timeline.frame_count` and the public `FrameData.frame_count()`/`iasa()` return 46 and exposes frames 41-46. The pinned runtime dispatches commands before `anim_cb`, which permits the endpoint fix, but `ftCo_AttackS4_Anim` transitions as soon as the animation has no frames remaining, so time 46 is never reached (DK fair is similarly inflated from endpoint 60 to 85). Please only let IASA extend a FigaTree-bounded timeline when it occurs at the reachable endpoint, while retaining later raw script timing, and add a post-endpoint IASA regression test.
JayKobe6k marked this conversation as resolved
@ -109,0 +130,4 @@
record = self._disc_action(character, action)
if record is None:
return ()
if not record.timeline.hitbox_generations and self.is_bmove(character, action):
Author
Owner

[P2] This uses legacy is_bmove(), which treats nearly every action ID at or above 341 as a special even when the character-relative runtime state is not. Retail DK action 341 is HeavyWait with move ID ThrowF and no fighter hitboxes, yet FrameData.is_attack() and CharacterState.get_state() raise the article-timing error instead of reporting a non-attack state. Please gate article uncertainty using character-relative MotionState special metadata rather than the numeric heuristic and cover DK HeavyWait through the public state classifier.

[P2] This uses legacy `is_bmove()`, which treats nearly every action ID at or above 341 as a special even when the character-relative runtime state is not. Retail DK action 341 is `HeavyWait` with move ID `ThrowF` and no fighter hitboxes, yet `FrameData.is_attack()` and `CharacterState.get_state()` raise the article-timing error instead of reporting a non-attack state. Please gate article uncertainty using character-relative MotionState special metadata rather than the numeric heuristic and cover DK `HeavyWait` through the public state classifier.
JayKobe6k marked this conversation as resolved
@ -629,0 +683,4 @@
count = 0
previous = None
for frame in frames:
if previous is None or frame != previous + 1:
Author
Owner

[P2] This counts gaps in active frames, but parsed generations can replace hitboxes without an inactive frame. Retail Pichu FSMASH_MID creates three hitbox IDs at each of six start frames (16, 19, 22, 25, 28, 31) and stays continuously active through frame 33, so FrameData.hitbox_count() returns 1 instead of 6. Please count distinct unconditional generation start frames, grouping IDs created on the same frame, and add this forward-smash regression test.

[P2] This counts gaps in active frames, but parsed generations can replace hitboxes without an inactive frame. Retail Pichu `FSMASH_MID` creates three hitbox IDs at each of six start frames (`16, 19, 22, 25, 28, 31`) and stays continuously active through frame 33, so `FrameData.hitbox_count()` returns `1` instead of `6`. Please count distinct unconditional generation start frames, grouping IDs created on the same frame, and add this forward-smash regression test.
JayKobe6k marked this conversation as resolved
fix: count contiguous hitbox generations
All checks were successful
Linux tests / Python 3.12 (push) Successful in 35s
Linux tests / Python 3.13 (push) Successful in 36s
Linux tests / Python 3.11 (push) Successful in 38s
Linux tests / Python 3.11 (pull_request) Successful in 40s
Linux tests / Python 3.12 (pull_request) Successful in 33s
Linux tests / Python 3.13 (pull_request) Successful in 34s
3247b449a1
Author
Owner

Review comment #910: Fixed in 37c0254. Added canonical NTSC 1.02 ceilings for FST, DOL, and supported fighter DAT/AJ member reads, checked after range validation and before allocation. Added oversized-but-in-bounds sparse image coverage for all three reads. Tests: python -m unittest test.DiscFrameDataTests.test_iso_validation_fst_bounded_read_and_exact_pairing test.DiscFrameDataTests.test_modified_dol_is_rejected_before_build_provenance (2 passed); uvx ruff check melee/_gamecube.py passed; python test.py (341 passed).

Review comment #910: Fixed in 37c0254. Added canonical NTSC 1.02 ceilings for FST, DOL, and supported fighter DAT/AJ member reads, checked after range validation and before allocation. Added oversized-but-in-bounds sparse image coverage for all three reads. Tests: `python -m unittest test.DiscFrameDataTests.test_iso_validation_fst_bounded_read_and_exact_pairing test.DiscFrameDataTests.test_modified_dol_is_rejected_before_build_provenance` (2 passed); `uvx ruff check melee/_gamecube.py` passed; `python test.py` (341 passed).
Author
Owner

Review comment #911: Fixed in e8ca05f. Reference validation now accepts a zero link after validating the current node, while retaining separate two-node cycle detection so data offset zero remains a valid chain head. Tests: python -m unittest test.DiscFrameDataTests.test_malformed_iso_dat_and_subactions_fail_with_context (1 passed); uvx ruff check melee/_hsd_dat.py passed; python test.py (341 passed).

Review comment #911: Fixed in e8ca05f. Reference validation now accepts a zero link after validating the current node, while retaining separate two-node cycle detection so data offset zero remains a valid chain head. Tests: `python -m unittest test.DiscFrameDataTests.test_malformed_iso_dat_and_subactions_fail_with_context` (1 passed); `uvx ruff check melee/_hsd_dat.py` passed; `python test.py` (341 passed).
Author
Owner

Review comment #912: Fixed in dd05276. Finite FigaTree timelines now extend for IASA only when the marker is reachable at or before the endpoint; raw post-endpoint IASA commands and timing remain retained. The endpoint hitbox/IASA regression still reaches frame 8, while a raw frame-14 IASA leaves an 8.0 endpoint at 7 snapshots. Tests: python -m unittest test.DiscFrameDataTests.test_reached_state_changes_extend_exclusive_animation_endpoint (1 passed); retail ISO verification reports Fox f-smash frame count/IASA 39/39 with raw IASA 46 and DK fair 59/59 with raw IASA 85; uvx ruff check melee/_subaction.py passed; python test.py (341 passed).

Review comment #912: Fixed in dd05276. Finite FigaTree timelines now extend for IASA only when the marker is reachable at or before the endpoint; raw post-endpoint IASA commands and timing remain retained. The endpoint hitbox/IASA regression still reaches frame 8, while a raw frame-14 IASA leaves an 8.0 endpoint at 7 snapshots. Tests: `python -m unittest test.DiscFrameDataTests.test_reached_state_changes_extend_exclusive_animation_endpoint` (1 passed); retail ISO verification reports Fox f-smash frame count/IASA 39/39 with raw IASA 46 and DK fair 59/59 with raw IASA 85; `uvx ruff check melee/_subaction.py` passed; `python test.py` (341 passed).
Author
Owner

Review comment #913: Fixed in 7591d76. ISO article uncertainty now uses the audited MotionState move ID range instead of legacy numeric is_bmove() classification; the legacy public heuristic remains unchanged. Added synthetic DK HeavyWait coverage through CharacterState. Tests: python -m unittest test.DiscFrameDataTests.test_iso_article_uncertainty_uses_motion_move_id test.DiscFrameDataTests.test_deprecated_framedata_iso_timing_facade test.DiscFrameDataTests.test_iso_framedata_simple_controls_continues_article_special (3 passed); retail DK action 341 now reports move ID 53, is_attack() == False, and Standing; python test.py (341 passed).

Review comment #913: Fixed in 7591d76. ISO article uncertainty now uses the audited MotionState move ID range instead of legacy numeric `is_bmove()` classification; the legacy public heuristic remains unchanged. Added synthetic DK HeavyWait coverage through `CharacterState`. Tests: `python -m unittest test.DiscFrameDataTests.test_iso_article_uncertainty_uses_motion_move_id test.DiscFrameDataTests.test_deprecated_framedata_iso_timing_facade test.DiscFrameDataTests.test_iso_framedata_simple_controls_continues_article_special` (3 passed); retail DK action 341 now reports move ID 53, `is_attack() == False`, and `Standing`; `python test.py` (341 passed).
Author
Owner

Review comment #914: Fixed in 3247b44. ISO hitbox counts now use distinct unconditional generation start frames, grouping IDs created on the same frame and excluding thrown-owner-conditional generations, instead of counting gaps in active snapshots. Added a continuously active six-generation, three-ID regression. Tests: python -m unittest test.DiscFrameDataTests.test_iso_hitbox_count_groups_generation_start_frames test.DiscFrameDataTests.test_deprecated_framedata_iso_timing_facade (2 passed); python -m unittest test.DiscFrameDataTests (12 passed); retail Pichu f-smash now reports starts 16/19/22/25/28/31 and count 6; python test.py (341 passed).

Review comment #914: Fixed in 3247b44. ISO hitbox counts now use distinct unconditional generation start frames, grouping IDs created on the same frame and excluding thrown-owner-conditional generations, instead of counting gaps in active snapshots. Added a continuously active six-generation, three-ID regression. Tests: `python -m unittest test.DiscFrameDataTests.test_iso_hitbox_count_groups_generation_start_frames test.DiscFrameDataTests.test_deprecated_framedata_iso_timing_facade` (2 passed); `python -m unittest test.DiscFrameDataTests` (12 passed); retail Pichu f-smash now reports starts 16/19/22/25/28/31 and count 6; `python test.py` (341 passed).
JayKobe6k left a comment

Found three actionable resource-safety and timeline correctness issues.

Found three actionable resource-safety and timeline correctness issues.
@ -0,0 +58,4 @@
location = struct.unpack_from(">I", self._file, reloc_start + index * 4)[0]
if location % 4 or location > self.data_size - 4:
self._fail(f"relocation {index} has invalid data offset 0x{location:X}")
if location in locations:
Author
Owner

[P2] This duplicate check linearly searches every prior relocation, making DAT validation quadratic in attacker-controlled reloc_count. A 320,032-byte input with 40,000 unique relocations takes about 3.9 seconds here, while a member within the new 0x214D00 cap can encode roughly 272,000 entries and stall parsing for minutes. Please track seen locations in a set (which can also become pointer_locations) and add a large unique-relocation regression.

[P2] This duplicate check linearly searches every prior relocation, making DAT validation quadratic in attacker-controlled `reloc_count`. A 320,032-byte input with 40,000 unique relocations takes about 3.9 seconds here, while a member within the new 0x214D00 cap can encode roughly 272,000 entries and stall parsing for minutes. Please track seen locations in a set (which can also become `pointer_locations`) and add a large unique-relocation regression.
JayKobe6k marked this conversation as resolved
@ -0,0 +74,4 @@
target, string_offset = struct.unpack_from(">II", self._file, roots_start + index * 8)
if target >= self.data_size:
self._fail(f"root {index} targets data offset 0x{target:X}")
name = self._file_c_string(strings_start + string_offset, strings_start, f"root {index}")
Author
Owner

[P2] This independently decodes and retains every root name, so overlapping string-table offsets can amplify a bounded DAT far beyond its input size; the analogous reference-name path has the same issue. A 73,573-byte file with 1,000 offsets into successive suffixes of one 65,536-byte string peaks at about 65.2 MB, so a crafted member below the read cap can still exhaust process memory. Please bound aggregate root/reference name materialization relative to the member size, reuse identical decodes where possible, and cover overlapping offsets.

[P2] This independently decodes and retains every root name, so overlapping string-table offsets can amplify a bounded DAT far beyond its input size; the analogous reference-name path has the same issue. A 73,573-byte file with 1,000 offsets into successive suffixes of one 65,536-byte string peaks at about 65.2 MB, so a crafted member below the read cap can still exhaust process memory. Please bound aggregate root/reference name materialization relative to the member size, reuse identical decodes where possible, and cover overlapping offsets.
JayKobe6k marked this conversation as resolved
@ -0,0 +737,4 @@
if animation_frame_count is not None:
frame_count = max(
frame_count,
max((event.local_frame for event in events), default=0),
Author
Owner

[P2] This unconditional event maximum exports runtime frames past a non-looping animation endpoint. Retail Bowser DOWN_B_GROUND maps to DAT action 313 with endpoint 40 and non-looping flags 0x82000005, but its stale 21-damage create event at time 41 makes the facade report a fabricated frame-41 attack even though the animation callback transitions when frame 40 ends. Please retain post-endpoint events as raw script evidence but exclude them from snapshots and facade-visible generation counts for non-looping records, while passing the 0x40000000 loop flag so genuinely wrapping actions such as Game & Watch rapid jab remain unrestricted.

[P2] This unconditional event maximum exports runtime frames past a non-looping animation endpoint. Retail Bowser `DOWN_B_GROUND` maps to DAT action 313 with endpoint 40 and non-looping flags `0x82000005`, but its stale 21-damage create event at time 41 makes the facade report a fabricated frame-41 attack even though the animation callback transitions when frame 40 ends. Please retain post-endpoint events as raw script evidence but exclude them from snapshots and facade-visible generation counts for non-looping records, while passing the `0x40000000` loop flag so genuinely wrapping actions such as Game & Watch rapid jab remain unrestricted.
JayKobe6k marked this conversation as resolved
fix: exclude unreachable timeline events
All checks were successful
Linux tests / Python 3.11 (pull_request) Successful in 38s
Linux tests / Python 3.11 (push) Successful in 39s
Linux tests / Python 3.12 (push) Successful in 36s
Linux tests / Python 3.13 (push) Successful in 36s
Linux tests / Python 3.12 (pull_request) Successful in 33s
Linux tests / Python 3.13 (pull_request) Successful in 34s
a1ab986f7a
Author
Owner

Review comment #1003: Fixed in 4d6935c. Relocation locations are now accumulated in a set, making duplicate detection and the final pointer-location representation O(1) per entry. Added a regression that parses 40,000 unique relocations. Focused tests: python -m unittest test.DiscFrameDataTests.test_dat_metadata_resource_guards test.DiscFrameDataTests.test_dat_roots_actions_and_figatree_frame_count test.DiscFrameDataTests.test_malformed_iso_dat_and_subactions_fail_with_context (3 passed in 0.023s). Full suite: python test.py (342 passed); uvx ruff check melee/_hsd_dat.py and py_compile passed.

Review comment #1003: Fixed in 4d6935c. Relocation locations are now accumulated in a set, making duplicate detection and the final pointer-location representation O(1) per entry. Added a regression that parses 40,000 unique relocations. Focused tests: `python -m unittest test.DiscFrameDataTests.test_dat_metadata_resource_guards test.DiscFrameDataTests.test_dat_roots_actions_and_figatree_frame_count test.DiscFrameDataTests.test_malformed_iso_dat_and_subactions_fail_with_context` (3 passed in 0.023s). Full suite: `python test.py` (342 passed); `uvx ruff check melee/_hsd_dat.py` and `py_compile` passed.
Author
Owner

Review comment #1004: Fixed in 4d6935c. Root/reference names are cached by string-table offset, so repeated entries share one decoded string, and newly decoded name bytes have an aggregate budget capped at the DAT size. Added regressions for 1,000 overlapping suffix offsets and 1,000 repeated offsets. Focused tests: python -m unittest test.DiscFrameDataTests.test_dat_metadata_resource_guards test.DiscFrameDataTests.test_dat_roots_actions_and_figatree_frame_count test.DiscFrameDataTests.test_malformed_iso_dat_and_subactions_fail_with_context (3 passed in 0.023s). Full suite: python test.py (342 passed); uvx ruff check melee/_hsd_dat.py and py_compile passed.

Review comment #1004: Fixed in 4d6935c. Root/reference names are cached by string-table offset, so repeated entries share one decoded string, and newly decoded name bytes have an aggregate budget capped at the DAT size. Added regressions for 1,000 overlapping suffix offsets and 1,000 repeated offsets. Focused tests: `python -m unittest test.DiscFrameDataTests.test_dat_metadata_resource_guards test.DiscFrameDataTests.test_dat_roots_actions_and_figatree_frame_count test.DiscFrameDataTests.test_malformed_iso_dat_and_subactions_fail_with_context` (3 passed in 0.023s). Full suite: `python test.py` (342 passed); `uvx ruff check melee/_hsd_dat.py` and `py_compile` passed.
Author
Owner

Review comment #1005: Fixed in a1ab986. Non-looping records now retain post-endpoint commands, hitbox events, and raw generations while excluding those events from snapshots and facade generation counts. The DAT 0x40000000 loop flag is passed through and exposed by ActionRecord.animation_loops, so looping records remain unrestricted. Focused tests: python -m unittest test.DiscFrameDataTests.test_reached_state_changes_extend_exclusive_animation_endpoint test.DiscFrameDataTests.test_iso_hitbox_count_groups_generation_start_frames test.DiscFrameDataTests.test_public_api_combat_timeline_and_local_geometry test.DiscFrameDataTests.test_deprecated_framedata_iso_timing_facade (4 passed). Retail verification: Bowser DOWN_B_GROUND retains raw frame-41 damage 21 but reports (False, -1, -1, 0, 39) through the facade; Game & Watch LOOPING_ATTACK_MIDDLE retains frames through 63 and reports seven generations. Full suite: python test.py (342 passed); targeted Ruff and py_compile passed.

Review comment #1005: Fixed in a1ab986. Non-looping records now retain post-endpoint commands, hitbox events, and raw generations while excluding those events from snapshots and facade generation counts. The DAT `0x40000000` loop flag is passed through and exposed by `ActionRecord.animation_loops`, so looping records remain unrestricted. Focused tests: `python -m unittest test.DiscFrameDataTests.test_reached_state_changes_extend_exclusive_animation_endpoint test.DiscFrameDataTests.test_iso_hitbox_count_groups_generation_start_frames test.DiscFrameDataTests.test_public_api_combat_timeline_and_local_geometry test.DiscFrameDataTests.test_deprecated_framedata_iso_timing_facade` (4 passed). Retail verification: Bowser `DOWN_B_GROUND` retains raw frame-41 damage 21 but reports `(False, -1, -1, 0, 39)` through the facade; Game & Watch `LOOPING_ATTACK_MIDDLE` retains frames through 63 and reports seven generations. Full suite: `python test.py` (342 passed); targeted Ruff and `py_compile` passed.
JayKobe6k left a comment

Found five actionable parser and framedata correctness issues.

Found five actionable parser and framedata correctness issues.
@ -0,0 +93,4 @@
def _validate_reference_chain(self, offset: int, index: int) -> None:
seen = set()
while offset != 0xFFFFFFFF:
Author
Owner

[P2] This starts a fresh complete external-reference chain walk for every reference, while the per-call seen set neither reuses validated suffixes nor rejects nodes already owned by another reference. A 70,922-byte DAT with 4,000 unique references to one 4,000-link chain passes validation after 16,000,000 u32 reads and takes about 4 seconds; the 0x214D00 member cap still permits roughly 37.2 billion visits. Shared chain nodes are invalid because each slot is resolved in place, but malformed DAT validation must remain bounded. Please track chain-location ownership globally, reject overlaps, and add a many-reference regression.

[P2] This starts a fresh complete external-reference chain walk for every reference, while the per-call `seen` set neither reuses validated suffixes nor rejects nodes already owned by another reference. A 70,922-byte DAT with 4,000 unique references to one 4,000-link chain passes validation after 16,000,000 `u32` reads and takes about 4 seconds; the 0x214D00 member cap still permits roughly 37.2 billion visits. Shared chain nodes are invalid because each slot is resolved in place, but malformed DAT validation must remain bounded. Please track chain-location ownership globally, reject overlaps, and add a many-reference regression.
JayKobe6k marked this conversation as resolved
@ -0,0 +207,4 @@
result = []
for index, offset in enumerate(range(actions, boundary, 0x18)):
symbol_pointer = self.pointer(offset, description=f"action {index} symbol")
symbol = self.c_string(symbol_pointer, f"action {index} symbol") if symbol_pointer is not None else None
Author
Owner

[P2] Action symbols use uncached c_string(), so the root/reference-name cache does not cover them. A fully validated ftDataKirby DAT using the pinned 479-action count can point every action at one 128 KiB symbol: the 145,604-byte input creates 479 distinct retained strings and peaks at about 63.2 MB. At the 0x214D00 member cap, this layout retains about 1.04 GB of decoded symbol payload alone. Please cache data-block strings by offset, enforce an aggregate decoded-symbol budget, and add a shared long-symbol regression.

[P2] Action symbols use uncached `c_string()`, so the root/reference-name cache does not cover them. A fully validated `ftDataKirby` DAT using the pinned 479-action count can point every action at one 128 KiB symbol: the 145,604-byte input creates 479 distinct retained strings and peaks at about 63.2 MB. At the 0x214D00 member cap, this layout retains about 1.04 GB of decoded symbol payload alone. Please cache data-block strings by offset, enforce an aggregate decoded-symbol budget, and add a shared long-symbol regression.
JayKobe6k marked this conversation as resolved
@ -0,0 +462,4 @@
break
if opcode in (1, 2):
frame = float(command.parameter("frame"))
timer = timer + frame if opcode == 1 else frame - time
Author
Owner

[P3] Opcode 2 subtracts cumulative script time, but the runtime subtracts CommandInfo.frame_count (cur_anim_frame + x898_unk), which differs after a wrap when action flag 0x20000000 is clear. Retail Female Wireframe RUNNING (Gl action 13, endpoint 20, flags 0x4000001e) reaches AsyncTimer(33) at elapsed time 21; this timestamps the following opcodes 54/43/goto at 33, while the wrapped animation phase is 1 and the runtime schedules them at 53. Ordinary goto-loop timing remains part of the documented effective-timestamp contract; only Set Timer Animation is explicitly truncated. Please track runtime frame count separately, pass the accumulation flag, and add a wrap-before-async regression.

[P3] Opcode 2 subtracts cumulative script time, but the runtime subtracts `CommandInfo.frame_count` (`cur_anim_frame + x898_unk`), which differs after a wrap when action flag 0x20000000 is clear. Retail Female Wireframe `RUNNING` (Gl action 13, endpoint 20, flags 0x4000001e) reaches `AsyncTimer(33)` at elapsed time 21; this timestamps the following opcodes 54/43/goto at 33, while the wrapped animation phase is 1 and the runtime schedules them at 53. Ordinary goto-loop timing remains part of the documented effective-timestamp contract; only `Set Timer Animation` is explicitly truncated. Please track runtime frame count separately, pass the accumulation flag, and add a wrap-before-async regression.
JayKobe6k marked this conversation as resolved
@ -109,0 +144,4 @@
"the move may create an unparsed article or projectile"
)
return tuple(
frame.local_frame
Author
Owner

[P2] This exposes script-local frames directly as normalized PlayerState.action_frame values, but that alignment is action-specific. Peach action 45 is marked zeroindex=True, so Console increments its live frame; the checked-in capture has hitboxes on frames 3-4, while the retail ISO facade reports 2-3, frame_count() 19 instead of 20, iasa() 16 instead of 17, and frame 2 as ATTACKING. Exhaustive comparison finds eight retail fighter-hitbox actions whose complete ISO window is exactly one frame early, including Peach/Zelda grabs and Zelda neutral-B. Please carry runtime frame alignment through action_for_state() into timeline/facade frames and cover these action-specific offsets.

[P2] This exposes script-local frames directly as normalized `PlayerState.action_frame` values, but that alignment is action-specific. Peach action 45 is marked `zeroindex=True`, so `Console` increments its live frame; the checked-in capture has hitboxes on frames 3-4, while the retail ISO facade reports 2-3, `frame_count()` 19 instead of 20, `iasa()` 16 instead of 17, and frame 2 as ATTACKING. Exhaustive comparison finds eight retail fighter-hitbox actions whose complete ISO window is exactly one frame early, including Peach/Zelda grabs and Zelda neutral-B. Please carry runtime frame alignment through `action_for_state()` into timeline/facade frames and cover these action-specific offsets.
JayKobe6k marked this conversation as resolved
@ -629,0 +695,4 @@
return 0
return len(
{
generation.start_frame
Author
Owner

[P2] Counting every generation start as another hit treats property redefinitions as rehit windows. Retail Mario NAir creates IDs 0/1 in group 0 at time 3, recreates the same enabled IDs/group at time 7, and clears at 33; this returns 2 while the checked-in CSV facade returns 1. Runtime preserves history for enabled same-ID/same-group CREATEs, while disabled or regrouped slots copy history from any other active same-group slot and clear it only when none exists. Please count group-scoped victim-history epochs across all four slots, so Mario remains one hit while Pichu f-smash's clear-separated waves remain six, and cover both cases.

[P2] Counting every generation start as another hit treats property redefinitions as rehit windows. Retail Mario NAir creates IDs 0/1 in group 0 at time 3, recreates the same enabled IDs/group at time 7, and clears at 33; this returns 2 while the checked-in CSV facade returns 1. Runtime preserves history for enabled same-ID/same-group CREATEs, while disabled or regrouped slots copy history from any other active same-group slot and clear it only when none exists. Please count group-scoped victim-history epochs across all four slots, so Mario remains one hit while Pichu f-smash's clear-separated waves remain six, and cover both cases.
JayKobe6k marked this conversation as resolved
fix: model wrapped animation timers
All checks were successful
Linux tests / Python 3.12 (push) Successful in 35s
Linux tests / Python 3.13 (push) Successful in 36s
Linux tests / Python 3.11 (push) Successful in 37s
Linux tests / Python 3.11 (pull_request) Successful in 39s
Linux tests / Python 3.12 (pull_request) Successful in 33s
Linux tests / Python 3.13 (pull_request) Successful in 33s
cce6dbec38
Author
Owner

Review comment #1028: Fixed in d424d1a. External-reference chain locations are now tracked across the whole DAT; a second reference that shares a resolved slot is rejected immediately, bounding aggregate traversal to the available chain locations. Added a 4,000-reference/4,000-link shared-chain regression. Focused tests: python -m unittest test.DiscFrameDataTests.test_dat_metadata_resource_guards test.DiscFrameDataTests.test_malformed_iso_dat_and_subactions_fail_with_context test.DiscFrameDataTests.test_dat_roots_actions_and_figatree_frame_count (3 passed in 0.026s). Full suite: python test.py (343 passed); targeted Ruff and py_compile passed.

Review comment #1028: Fixed in d424d1a. External-reference chain locations are now tracked across the whole DAT; a second reference that shares a resolved slot is rejected immediately, bounding aggregate traversal to the available chain locations. Added a 4,000-reference/4,000-link shared-chain regression. Focused tests: `python -m unittest test.DiscFrameDataTests.test_dat_metadata_resource_guards test.DiscFrameDataTests.test_malformed_iso_dat_and_subactions_fail_with_context test.DiscFrameDataTests.test_dat_roots_actions_and_figatree_frame_count` (3 passed in 0.026s). Full suite: `python test.py` (343 passed); targeted Ruff and `py_compile` passed.
Author
Owner

Review comment #1029: Fixed in d424d1a. DAT data-block strings are now cached by offset and charged to an aggregate budget capped at the DAT data size. Added a fully validated 479-action Kirby layout with one shared 128 KiB symbol and asserted every action reuses the same decoded object. Focused tests: python -m unittest test.DiscFrameDataTests.test_dat_metadata_resource_guards test.DiscFrameDataTests.test_malformed_iso_dat_and_subactions_fail_with_context test.DiscFrameDataTests.test_dat_roots_actions_and_figatree_frame_count (3 passed in 0.026s). Full suite: python test.py (343 passed); targeted Ruff and py_compile passed.

Review comment #1029: Fixed in d424d1a. DAT data-block strings are now cached by offset and charged to an aggregate budget capped at the DAT data size. Added a fully validated 479-action Kirby layout with one shared 128 KiB symbol and asserted every action reuses the same decoded object. Focused tests: `python -m unittest test.DiscFrameDataTests.test_dat_metadata_resource_guards test.DiscFrameDataTests.test_malformed_iso_dat_and_subactions_fail_with_context test.DiscFrameDataTests.test_dat_roots_actions_and_figatree_frame_count` (3 passed in 0.026s). Full suite: `python test.py` (343 passed); targeted Ruff and `py_compile` passed.
Author
Owner

Review comment #1031: Fixed in e34cfd7. hitbox_count() now replays reachable unconditional create/remove/clear events in command order and counts only when a group has no active slot history; raw parser generations remain lossless. Added same-group redefinition, clear-separated wave, cross-slot group-copy, and endpoint regressions. Focused tests: python -m unittest test.DiscFrameDataTests.test_iso_hitbox_count_tracks_group_victim_history_epochs test.DiscFrameDataTests.test_deprecated_framedata_iso_timing_facade (2 passed in 0.017s). Retail counts are Mario NAir 1, Pichu f-smash 6, Fox fair 5, Zelda up-smash 12, and Game & Watch rapid jab 7. Full suite: python test.py (343 passed); py_compile passed.

Review comment #1031: Fixed in e34cfd7. `hitbox_count()` now replays reachable unconditional create/remove/clear events in command order and counts only when a group has no active slot history; raw parser generations remain lossless. Added same-group redefinition, clear-separated wave, cross-slot group-copy, and endpoint regressions. Focused tests: `python -m unittest test.DiscFrameDataTests.test_iso_hitbox_count_tracks_group_victim_history_epochs test.DiscFrameDataTests.test_deprecated_framedata_iso_timing_facade` (2 passed in 0.017s). Retail counts are Mario NAir 1, Pichu f-smash 6, Fox fair 5, Zelda up-smash 12, and Game & Watch rapid jab 7. Full suite: `python test.py` (343 passed); `py_compile` passed.
Author
Owner

Review comment #1030: Fixed in 56cdfeb. The deprecated ISO facade now applies the eight audited action-specific compatibility offsets while low-level ActionTimeline records remain one-indexed script time, and the distinction is documented. Focused tests: python -m unittest test.DiscFrameDataTests.test_deprecated_framedata_iso_timing_facade test.DiscFrameDataTests.test_public_api_combat_timeline_and_local_geometry (2 passed). Retail verification matches all eight shipped windows: Peach jab 2 is (3, 4, 20, 17) and Zelda neutral-B is (13, 28, 60, 60) for first/last/count/IASA. Full suite: python test.py (343 passed); strict Sphinx, targeted Ruff, and py_compile passed.

Review comment #1030: Fixed in 56cdfeb. The deprecated ISO facade now applies the eight audited action-specific compatibility offsets while low-level `ActionTimeline` records remain one-indexed script time, and the distinction is documented. Focused tests: `python -m unittest test.DiscFrameDataTests.test_deprecated_framedata_iso_timing_facade test.DiscFrameDataTests.test_public_api_combat_timeline_and_local_geometry` (2 passed). Retail verification matches all eight shipped windows: Peach jab 2 is `(3, 4, 20, 17)` and Zelda neutral-B is `(13, 28, 60, 60)` for first/last/count/IASA. Full suite: `python test.py` (343 passed); strict Sphinx, targeted Ruff, and `py_compile` passed.
Author
Owner

Review comment #1032: Fixed in cce6dbe. The interpreter now tracks cumulative elapsed time separately from wrapped animation phase and the 0x20000000 accumulated-frame carry used by asynchronous timers; DiscFrameData forwards that flag. Added wrapped and accumulating timer regressions plus parser-wiring coverage. Focused tests: python -m unittest test.DiscFrameDataTests.test_wrapped_animation_frame_drives_async_timers test.DiscFrameDataTests.test_malformed_iso_dat_and_subactions_fail_with_context test.DiscFrameDataTests.test_reached_state_changes_extend_exclusive_animation_endpoint test.DiscFrameDataTests.test_control_flow_and_lossless_command_lengths test.DiscFrameDataTests.test_iso_hitbox_count_tracks_group_victim_history_epochs (5 passed in 0.018s). Retail Female Wireframe running now timestamps opcodes 54/43/goto at 53.0, while Game & Watch rapid jab remains seven waves through frame 63. Full suite: python test.py (343 passed); targeted Ruff and py_compile passed.

Review comment #1032: Fixed in cce6dbe. The interpreter now tracks cumulative elapsed time separately from wrapped animation phase and the `0x20000000` accumulated-frame carry used by asynchronous timers; `DiscFrameData` forwards that flag. Added wrapped and accumulating timer regressions plus parser-wiring coverage. Focused tests: `python -m unittest test.DiscFrameDataTests.test_wrapped_animation_frame_drives_async_timers test.DiscFrameDataTests.test_malformed_iso_dat_and_subactions_fail_with_context test.DiscFrameDataTests.test_reached_state_changes_extend_exclusive_animation_endpoint test.DiscFrameDataTests.test_control_flow_and_lossless_command_lengths test.DiscFrameDataTests.test_iso_hitbox_count_tracks_group_victim_history_epochs` (5 passed in 0.018s). Retail Female Wireframe running now timestamps opcodes 54/43/goto at 53.0, while Game & Watch rapid jab remains seven waves through frame 63. Full suite: `python test.py` (343 passed); targeted Ruff and `py_compile` passed.
JayKobe6k left a comment

Found five actionable correctness and resource-safety issues.

Found five actionable correctness and resource-safety issues.
@ -0,0 +200,4 @@
if not name or not name.isprintable() or "/" in name or "\\" in name or name in (".", ".."):
raise DiscImageError(f"FST entry {index} has unsafe name {name!r}")
parent_index, parent_end, parent_path = stack[-1]
path = f"{parent_path}/{name}" if parent_path else name
Author
Owner

[P2] The FST-size cap does not bound retained paths: each nested entry stores the complete parent path, while entries may reuse one long name-table string. A valid 6,213-byte FST with 100 nested 5,000-byte names retains 25,254,950 path characters (about 25.8 MB traced) before DOL authentication, and layouts below the 0x7529 cap can exhaust memory. Please cap directory depth or aggregate path materialization, cache decoded names, and reject over-budget FSTs.

[P2] The FST-size cap does not bound retained paths: each nested entry stores the complete parent path, while entries may reuse one long name-table string. A valid 6,213-byte FST with 100 nested 5,000-byte names retains 25,254,950 path characters (about 25.8 MB traced) before DOL authentication, and layouts below the 0x7529 cap can exhaust memory. Please cap directory depth or aggregate path materialization, cache decoded names, and reject over-budget FSTs.
@ -0,0 +438,4 @@
while True:
if len(executed) >= max_commands:
raise SubactionParseError(f"{context}: command guard exceeded {max_commands} executions")
state = (pc, time, timer, tuple(calls), tuple((loop[0], loop[1]) for loop in loops))
Author
Owner

[P2] The command guard does not bound interpreter memory because each execution retains complete copies of the call and loop stacks in the visited-state sets. A 516-byte script with 64 nested loops and 5,000 inner iterations executes 5,128 commands but peaks at 50.7 MB traced; the allowed 100,000 executions can exhaust the process. Please use compact state identities or enforce an aggregate control-flow complexity budget, and cover deeply nested finite loops.

[P2] The command guard does not bound interpreter memory because each execution retains complete copies of the call and loop stacks in the visited-state sets. A 516-byte script with 64 nested loops and 5,000 inner iterations executes 5,128 commands but peaks at 50.7 MB traced; the allowed 100,000 executions can exhaust the process. Please use compact state identities or enforce an aggregate control-flow complexity budget, and cover deeply nested finite loops.
@ -0,0 +375,4 @@
)
animation_frame_counts[animation_key] = frame_count
timeline = (
interpret_subaction(
Author
Owner

[P2] The frame guard applies independently to each action, but this loop rebuilds and retains a timeline for every record even when records share identical script and animation metadata. Replaying 100 identical 1,000-frame scripts creates 100 distinct timelines and 100,000 retained snapshots; a valid 479-action fighter table can request about 4.8 million snapshots on its first query. Please cache timelines by script/frame/flag identity and enforce a per-fighter aggregate snapshot or command budget.

[P2] The frame guard applies independently to each action, but this loop rebuilds and retains a timeline for every record even when records share identical script and animation metadata. Replaying 100 identical 1,000-frame scripts creates 100 distinct timelines and 100,000 retained snapshots; a valid 479-action fighter table can request about 4.8 million snapshots on its first query. Please cache timelines by script/frame/flag identity and enforce a per-fighter aggregate snapshot or command budget.
@ -109,0 +152,4 @@
if (
not record.timeline.hitbox_generations
and state is not None
and state.move_id in SPECIAL_MOVE_IDS
Author
Owner

[P2] Restricting unsupported-article detection to SPECIAL_MOVE_IDS silently misclassifies article attacks outside the four canonical special slots. On the retail ISO, Peach PARASOL_FALLING has move ID 87 and no fighter generations, so the ISO facade returns is_attack() == False, hitbox frames -1/-1, and CharacterState.InAir, while the checked-in CSV reports an active attack on frames 1-100. Please include known character-owned article attack states in uncertainty classification and raise DiscFrameDataError until their timelines are parsed.

[P2] Restricting unsupported-article detection to `SPECIAL_MOVE_IDS` silently misclassifies article attacks outside the four canonical special slots. On the retail ISO, Peach `PARASOL_FALLING` has move ID 87 and no fighter generations, so the ISO facade returns `is_attack() == False`, hitbox frames `-1/-1`, and `CharacterState.InAir`, while the checked-in CSV reports an active attack on frames 1-100. Please include known character-owned article attack states in uncertainty classification and raise `DiscFrameDataError` until their timelines are parsed.
@ -629,0 +722,4 @@
if event.hitbox.requires_thrown_hitbox_owner:
continue
group = event.hitbox.hit_group
if group not in active_groups:
Author
Owner

[P2] This increments before considering subsequent events at the same timestamp, so zero-duration generations count as hits even though they never appear in a collision snapshot. Retail Yoshi DAIR creates IDs 0/1 and then clears all at time 46; frame 46 has no active hitboxes and the checked-in CSV reports 14 hits on frames 18 through 44, but the ISO facade returns 15. Please coalesce same-time events and count only victim-history epochs that survive into a collision-visible snapshot.

[P2] This increments before considering subsequent events at the same timestamp, so zero-duration generations count as hits even though they never appear in a collision snapshot. Retail Yoshi `DAIR` creates IDs 0/1 and then clears all at time 46; frame 46 has no active hitboxes and the checked-in CSV reports 14 hits on frames 18 through 44, but the ISO facade returns 15. Please coalesce same-time events and count only victim-history epochs that survive into a collision-visible snapshot.
fix: ignore zero-duration hitbox epochs
All checks were successful
Linux tests / Python 3.12 (push) Successful in 35s
Linux tests / Python 3.13 (push) Successful in 36s
Linux tests / Python 3.11 (push) Successful in 37s
Linux tests / Python 3.11 (pull_request) Successful in 38s
Linux tests / Python 3.12 (pull_request) Successful in 33s
Linux tests / Python 3.13 (pull_request) Successful in 33s
ceda0468be
Author
Owner

Review comment #1059: Fixed in ad33734. FST names are cached by string-table offset, and every candidate path is charged against the canonical image aggregate of 0x3D38 characters before construction. Added a structurally valid 6,213-byte, 100-level shared-name FST regression that verifies rejection and one name decode. Focused tests: python -m unittest test.DiscFrameDataTests.test_fst_name_and_path_materialization_is_bounded test.DiscFrameDataTests.test_iso_validation_fst_bounded_read_and_exact_pairing (2 passed in 0.004s). Full suite: python test.py (345 passed); targeted Ruff, py_compile, and strict Sphinx passed.

Review comment #1059: Fixed in ad33734. FST names are cached by string-table offset, and every candidate path is charged against the canonical image aggregate of 0x3D38 characters before construction. Added a structurally valid 6,213-byte, 100-level shared-name FST regression that verifies rejection and one name decode. Focused tests: `python -m unittest test.DiscFrameDataTests.test_fst_name_and_path_materialization_is_bounded test.DiscFrameDataTests.test_iso_validation_fst_bounded_read_and_exact_pairing` (2 passed in 0.004s). Full suite: `python test.py` (345 passed); targeted Ruff, `py_compile`, and strict Sphinx passed.
Author
Owner

Review comment #1060: Fixed in 635efa8. Call and loop snapshots are now created once per execution, shared by both visited-state sets, and charged against a 100,000 retained-stack-value aggregate guard while preserving cycle precedence and backward tail jumps. Added the 64-level, 5,000-iteration finite-loop regression. Focused tests: python -m unittest test.DiscFrameDataTests.test_malformed_iso_dat_and_subactions_fail_with_context test.DiscFrameDataTests.test_control_flow_and_lossless_command_lengths test.DiscFrameDataTests.test_wrapped_animation_frame_drives_async_timers (3 passed in 0.019s). Full suite: python test.py (345 passed); targeted Ruff and py_compile passed.

Review comment #1060: Fixed in 635efa8. Call and loop snapshots are now created once per execution, shared by both visited-state sets, and charged against a 100,000 retained-stack-value aggregate guard while preserving cycle precedence and backward tail jumps. Added the 64-level, 5,000-iteration finite-loop regression. Focused tests: `python -m unittest test.DiscFrameDataTests.test_malformed_iso_dat_and_subactions_fail_with_context test.DiscFrameDataTests.test_control_flow_and_lossless_command_lengths test.DiscFrameDataTests.test_wrapped_animation_frame_drives_async_timers` (3 passed in 0.019s). Full suite: `python test.py` (345 passed); targeted Ruff and `py_compile` passed.
Author
Owner

Review comment #1061: Fixed in 8d4d3e5. _parse_fighter() now shares immutable timelines by script offset, frame count, loop flag, and accumulation flag, and enforces a 110,000-item aggregate command-plus-snapshot budget across unique timelines. Tests cover shared timelines, behaviorally distinct keys, and aggregate rejection. Focused tests: python -m unittest test.DiscFrameDataTests.test_public_api_combat_timeline_and_local_geometry test.DiscFrameDataTests.test_nana_runtime_actions_inherit_missing_popo_animations test.DiscFrameDataTests.test_wrapped_animation_frame_drives_async_timers test.DiscFrameDataTests.test_malformed_iso_dat_and_subactions_fail_with_context (4 passed in 0.027s). Retail audit parsed 33 fighters and 10,658 actions; Kirby is the maximum at 23,601 retained items. Full suite: python test.py (345 passed); targeted Ruff and py_compile passed.

Review comment #1061: Fixed in 8d4d3e5. `_parse_fighter()` now shares immutable timelines by script offset, frame count, loop flag, and accumulation flag, and enforces a 110,000-item aggregate command-plus-snapshot budget across unique timelines. Tests cover shared timelines, behaviorally distinct keys, and aggregate rejection. Focused tests: `python -m unittest test.DiscFrameDataTests.test_public_api_combat_timeline_and_local_geometry test.DiscFrameDataTests.test_nana_runtime_actions_inherit_missing_popo_animations test.DiscFrameDataTests.test_wrapped_animation_frame_drives_async_timers test.DiscFrameDataTests.test_malformed_iso_dat_and_subactions_fail_with_context` (4 passed in 0.027s). Retail audit parsed 33 fighters and 10,658 actions; Kirby is the maximum at 23,601 retained items. Full suite: `python test.py` (345 passed); targeted Ruff and `py_compile` passed.
Author
Owner

Review comment #1062: Fixed in e509ed9. Added the audited (PEACH, PARASOL_FALLING) article-attack exception without broadening move ID 87 or reintroducing the DK HeavyWait false positive. ISO hitbox queries and CharacterState now raise the documented unavailable-article error while frame_count() remains available. Focused tests: python -m unittest test.DiscFrameDataTests.test_iso_article_uncertainty_includes_character_owned_attack_states test.DiscFrameDataTests.test_iso_article_uncertainty_uses_motion_move_id test.DiscFrameDataTests.test_deprecated_framedata_iso_timing_facade (3 passed in 0.013s). Retail verification reports DiscFrameDataError for all four hitbox queries and state classification. Full suite: python test.py (345 passed); py_compile and strict Sphinx passed.

Review comment #1062: Fixed in e509ed9. Added the audited `(PEACH, PARASOL_FALLING)` article-attack exception without broadening move ID 87 or reintroducing the DK HeavyWait false positive. ISO hitbox queries and `CharacterState` now raise the documented unavailable-article error while `frame_count()` remains available. Focused tests: `python -m unittest test.DiscFrameDataTests.test_iso_article_uncertainty_includes_character_owned_attack_states test.DiscFrameDataTests.test_iso_article_uncertainty_uses_motion_move_id test.DiscFrameDataTests.test_deprecated_framedata_iso_timing_facade` (3 passed in 0.013s). Retail verification reports `DiscFrameDataError` for all four hitbox queries and state classification. Full suite: `python test.py` (345 passed); `py_compile` and strict Sphinx passed.
Author
Owner

Review comment #1063: Fixed in ceda046. hitbox_count() now replays exact-animation-time batches and counts only newly started victim-history groups still active after the batch. Added a same-time create/create/clear regression and retained separated cross-slot, Mario, Pichu, endpoint, looping, and conditional coverage. Focused tests: python -m unittest test.DiscFrameDataTests.test_iso_hitbox_count_tracks_group_victim_history_epochs test.DiscFrameDataTests.test_deprecated_framedata_iso_timing_facade test.DiscFrameDataTests.test_public_api_combat_timeline_and_local_geometry (3 passed in 0.021s). Retail counts are Yoshi DAIR 14, Mario NAir 1, Pichu f-smash 6, Fox fair 5, Zelda up-smash 12, and Game & Watch rapid jab 7. Full suite: python test.py (345 passed); targeted static checks passed.

Review comment #1063: Fixed in ceda046. `hitbox_count()` now replays exact-animation-time batches and counts only newly started victim-history groups still active after the batch. Added a same-time create/create/clear regression and retained separated cross-slot, Mario, Pichu, endpoint, looping, and conditional coverage. Focused tests: `python -m unittest test.DiscFrameDataTests.test_iso_hitbox_count_tracks_group_victim_history_epochs test.DiscFrameDataTests.test_deprecated_framedata_iso_timing_facade test.DiscFrameDataTests.test_public_api_combat_timeline_and_local_geometry` (3 passed in 0.021s). Retail counts are Yoshi DAIR 14, Mario NAir 1, Pichu f-smash 6, Fox fair 5, Zelda up-smash 12, and Game & Watch rapid jab 7. Full suite: `python test.py` (345 passed); targeted static checks passed.
JayKobe6k left a comment

Found five actionable correctness and parser-integrity issues.

Found five actionable correctness and parser-integrity issues.
@ -0,0 +278,4 @@
f"disc member {entry.path!r} size 0x{entry.size:X} exceeds the supported maximum "
f"0x{self._MAX_MEMBER_SIZE:X}"
)
with self.path.open("rb") as stream:
Author
Owner

[P2] DiscFrameData authenticates the DOL during construction, but lazy fighter reads reopen this path without verifying that it is still the same image. Replacing the ISO after construction let a probe retain the cached canonical DOL/build metadata while action("Fx", 0) read replacement DAT flags 0xB0000042. Please bind all reads to one immutable image snapshot or detect mutation and reauthenticate before lazy member reads, and cover replacement between construction and first fighter query.

[P2] `DiscFrameData` authenticates the DOL during construction, but lazy fighter reads reopen this path without verifying that it is still the same image. Replacing the ISO after construction let a probe retain the cached canonical DOL/build metadata while `action("Fx", 0)` read replacement DAT flags `0xB0000042`. Please bind all reads to one immutable image snapshot or detect mutation and reauthenticate before lazy member reads, and cover replacement between construction and first fighter query.
@ -0,0 +92,4 @@
references.append(DatRoot(name, target, True))
self.roots = tuple(roots)
self.references = tuple(references)
self.object_offsets = tuple(sorted(targets | {self.data_size}))
Author
Owner

[P2] These structure boundaries omit every validated external-reference chain location, so a reference can begin inside the fighter root, action table, dynamic table, or a string without triggering the overlap checks. A synthetic DAT with a valid reference at offset 0x20 inside the 0x60 fighter root still returned all 327 actions, and 0x20 was absent from object_offsets. Please merge reference_locations into the object boundaries and reject references overlapping required fighter structures.

[P2] These structure boundaries omit every validated external-reference chain location, so a reference can begin inside the fighter root, action table, dynamic table, or a string without triggering the overlap checks. A synthetic DAT with a valid reference at offset `0x20` inside the `0x60` fighter root still returned all 327 actions, and `0x20` was absent from `object_offsets`. Please merge `reference_locations` into the object boundaries and reject references overlapping required fighter structures.
@ -0,0 +539,4 @@
if opcode == 6:
if not calls:
raise SubactionParseError(f"{context}: return at DAT offset 0x{pc + 0x20:X} has no caller")
pc = calls.pop()
Author
Owner

[P2] Separate call and loop stacks let a return cross a loop frame pushed after the call. A call -> SetLoop -> return stream currently returns to the caller and exposes its IASA command ([5, 3, 6, 23, 0]), but the runtime stores loop and call entries on one LIFO return stack, so that return consumes the intervening loop entry instead. Please model a unified typed control stack or reject operations whose top entry has the wrong type, and test both crossed call/loop forms.

[P2] Separate call and loop stacks let a return cross a loop frame pushed after the call. A `call -> SetLoop -> return` stream currently returns to the caller and exposes its IASA command (`[5, 3, 6, 23, 0]`), but the runtime stores loop and call entries on one LIFO return stack, so that return consumes the intervening loop entry instead. Please model a unified typed control stack or reject operations whose top entry has the wrong type, and test both crossed call/loop forms.
@ -109,0 +145,4 @@
return self._disc_framedata.action_for_state(character, action)
def _disc_frame_offset(self, character, action):
return int((character, action) in _ISO_COMPATIBILITY_FRAME_OFFSETS)
Author
Owner

[P2] This boolean offset can express only zero or +1, but two retail actions need a hitbox-only -1 alignment. ISO-backed Samus FAIR reports active frames 5-6 through 31-32 instead of CSV frames 4-5 through 30-31, and Mewtwo LOOPING_ATTACK_MIDDLE reports 6,13,...,48 instead of 5,12,...,47; both frame counts already correctly match at 55 and 50. Please add signed hitbox-specific alignment without subtracting from frame count, and cover the complete active-frame sequences.

[P2] This boolean offset can express only zero or `+1`, but two retail actions need a hitbox-only `-1` alignment. ISO-backed Samus `FAIR` reports active frames 5-6 through 31-32 instead of CSV frames 4-5 through 30-31, and Mewtwo `LOOPING_ATTACK_MIDDLE` reports 6,13,...,48 instead of 5,12,...,47; both frame counts already correctly match at 55 and 50. Please add signed hitbox-specific alignment without subtracting from frame count, and cover the complete active-frame sequences.
@ -629,0 +721,4 @@
count = 0
for animation_time, events in groupby(
record.timeline.hitbox_events,
key=lambda event: event.animation_time,
Author
Owner

[P2] Batching by raw animation time still counts epochs that disappear within one exported collision frame. A create at time 0 followed by Wait(1) and clear at time 1 maps both events to local frame 1, whose snapshot has no active hitbox, yet hitbox_count() returns 1. Retail Kirby NAIR_LANDING and UAIR_LANDING consequently report is_attack() == False and hitbox frames -1/-1 but count 1. Please coalesce reachable events by local_frame as snapshot construction does and test this boundary.

[P2] Batching by raw animation time still counts epochs that disappear within one exported collision frame. A create at time 0 followed by `Wait(1)` and clear at time 1 maps both events to local frame 1, whose snapshot has no active hitbox, yet `hitbox_count()` returns 1. Retail Kirby `NAIR_LANDING` and `UAIR_LANDING` consequently report `is_attack() == False` and hitbox frames `-1/-1` but count 1. Please coalesce reachable events by `local_frame` as snapshot construction does and test this boundary.
fix: coalesce local-frame hitbox epochs
All checks were successful
Linux tests / Python 3.13 (push) Successful in 35s
Linux tests / Python 3.11 (pull_request) Successful in 37s
Linux tests / Python 3.11 (push) Successful in 39s
Linux tests / Python 3.12 (push) Successful in 35s
Linux tests / Python 3.13 (pull_request) Successful in 33s
Linux tests / Python 3.12 (pull_request) Successful in 34s
59c30b9a00
Author
Owner

Review comment #1090: Fixed in 78622a5. GameCubeDisc now captures the opened image identity from fstat() while validating the header/FST and checks that identity before every lazy member or DOL read, so replacing the path cannot combine cached canonical metadata with another image. Added an atomic-replacement regression before the first fighter query. Tests: python -m unittest test.DiscFrameDataTests.test_disc_framedata_rejects_replaced_image_before_lazy_read test.DiscFrameDataTests.test_iso_validation_fst_bounded_read_and_exact_pairing test.DiscFrameDataTests.test_modified_dol_is_rejected_before_build_provenance (3 passed); full python test.py (346 passed); targeted Ruff and py_compile passed.

Review comment #1090: Fixed in 78622a5. `GameCubeDisc` now captures the opened image identity from `fstat()` while validating the header/FST and checks that identity before every lazy member or DOL read, so replacing the path cannot combine cached canonical metadata with another image. Added an atomic-replacement regression before the first fighter query. Tests: `python -m unittest test.DiscFrameDataTests.test_disc_framedata_rejects_replaced_image_before_lazy_read test.DiscFrameDataTests.test_iso_validation_fst_bounded_read_and_exact_pairing test.DiscFrameDataTests.test_modified_dol_is_rejected_before_build_provenance` (3 passed); full `python test.py` (346 passed); targeted Ruff and `py_compile` passed.
Author
Owner

Review comment #1091: Fixed in d81a78f. Validated external-reference chain locations now participate in HsdDat.object_offsets without being treated as ordinary relocation pointers, so references embedded in the fighter root, action table, dynamic table, or strings are rejected by the existing overlap checks. Added an embedded fighter-root reference regression and updated the zero-terminated chain boundary assertion. Tests: python -m unittest test.DiscFrameDataTests.test_malformed_iso_dat_and_subactions_fail_with_context test.DiscFrameDataTests.test_dat_roots_actions_and_figatree_frame_count test.DiscFrameDataTests.test_dat_metadata_resource_guards (3 passed); full python test.py (346 passed); targeted Ruff and py_compile passed.

Review comment #1091: Fixed in d81a78f. Validated external-reference chain locations now participate in `HsdDat.object_offsets` without being treated as ordinary relocation pointers, so references embedded in the fighter root, action table, dynamic table, or strings are rejected by the existing overlap checks. Added an embedded fighter-root reference regression and updated the zero-terminated chain boundary assertion. Tests: `python -m unittest test.DiscFrameDataTests.test_malformed_iso_dat_and_subactions_fail_with_context test.DiscFrameDataTests.test_dat_roots_actions_and_figatree_frame_count test.DiscFrameDataTests.test_dat_metadata_resource_guards` (3 passed); full `python test.py` (346 passed); targeted Ruff and `py_compile` passed.
Author
Owner

Review comment #1092: Fixed in 2e8f747. Calls and loops now share one typed LIFO control stack, preserving separate depth/resource guards while rejecting return across an active loop and execute-loop across an active call instead of fabricating a valid timeline. Added both crossed-nesting regressions. Tests: python -m unittest test.DiscFrameDataTests.test_malformed_iso_dat_and_subactions_fail_with_context test.DiscFrameDataTests.test_control_flow_and_lossless_command_lengths test.DiscFrameDataTests.test_wrapped_animation_frame_drives_async_timers (3 passed); full python test.py (346 passed); targeted Ruff and py_compile passed.

Review comment #1092: Fixed in 2e8f747. Calls and loops now share one typed LIFO control stack, preserving separate depth/resource guards while rejecting `return` across an active loop and `execute-loop` across an active call instead of fabricating a valid timeline. Added both crossed-nesting regressions. Tests: `python -m unittest test.DiscFrameDataTests.test_malformed_iso_dat_and_subactions_fail_with_context test.DiscFrameDataTests.test_control_flow_and_lossless_command_lengths test.DiscFrameDataTests.test_wrapped_animation_frame_drives_async_timers` (3 passed); full `python test.py` (346 passed); targeted Ruff and `py_compile` passed.
Author
Owner

Review comment #1093: Fixed in f858acf. Added signed hitbox-only compatibility offsets for Samus FAIR and Mewtwo LOOPING_ATTACK_MIDDLE, leaving their already-correct frame counts and IASA values unchanged. Synthetic coverage asserts each complete corrected active-frame sequence. Tests: python -m unittest test.DiscFrameDataTests.test_deprecated_framedata_iso_timing_facade passed; full python test.py (346 passed); py_compile and strict Sphinx passed. Canonical retail ISO probe now reports Samus (4,5,11,12,18,19,25,26,30,31) with frame count 55/IASA 50 and Mewtwo (5,12,19,26,33,40,47) with frame count 50/IASA 50.

Review comment #1093: Fixed in f858acf. Added signed hitbox-only compatibility offsets for Samus FAIR and Mewtwo LOOPING_ATTACK_MIDDLE, leaving their already-correct frame counts and IASA values unchanged. Synthetic coverage asserts each complete corrected active-frame sequence. Tests: `python -m unittest test.DiscFrameDataTests.test_deprecated_framedata_iso_timing_facade` passed; full `python test.py` (346 passed); `py_compile` and strict Sphinx passed. Canonical retail ISO probe now reports Samus `(4,5,11,12,18,19,25,26,30,31)` with frame count 55/IASA 50 and Mewtwo `(5,12,19,26,33,40,47)` with frame count 50/IASA 50.
Author
Owner

Review comment #1094: Fixed in 59c30b9. Hitbox victim-history epochs are now coalesced by exported local_frame, matching snapshot construction, while each event retains its raw animation-time endpoint check. The regression uses create events at time 0 and a clear at time 1 that all map to local frame 1 and verifies a zero count. Tests: python -m unittest test.DiscFrameDataTests.test_iso_hitbox_count_tracks_group_victim_history_epochs test.DiscFrameDataTests.test_deprecated_framedata_iso_timing_facade test.DiscFrameDataTests.test_public_api_combat_timeline_and_local_geometry (3 passed); full python test.py (346 passed); py_compile and strict Sphinx passed. Canonical retail ISO now reports both Kirby NAIR_LANDING and UAIR_LANDING as non-attacks with -1/-1 hitbox frames and count 0.

Review comment #1094: Fixed in 59c30b9. Hitbox victim-history epochs are now coalesced by exported `local_frame`, matching snapshot construction, while each event retains its raw animation-time endpoint check. The regression uses create events at time 0 and a clear at time 1 that all map to local frame 1 and verifies a zero count. Tests: `python -m unittest test.DiscFrameDataTests.test_iso_hitbox_count_tracks_group_victim_history_epochs test.DiscFrameDataTests.test_deprecated_framedata_iso_timing_facade test.DiscFrameDataTests.test_public_api_combat_timeline_and_local_geometry` (3 passed); full `python test.py` (346 passed); `py_compile` and strict Sphinx passed. Canonical retail ISO now reports both Kirby NAIR_LANDING and UAIR_LANDING as non-attacks with `-1/-1` hitbox frames and count 0.
JayKobe6k left a comment

Found five actionable issues in the ISO-backed framedata implementation.

Found five actionable issues in the ISO-backed framedata implementation.
@ -0,0 +130,4 @@
_MAX_MEMBER_SIZE = 0x214D00
def __init__(self, path: str | os.PathLike[str]):
self.path = Path(path)
Author
Owner

[P3] This retains a relative path even though fighter members are reopened lazily. Constructing DiscFrameData("game.iso") in the ISO directory succeeds, but after changing the process working directory its first fighter query raises DiscImageError because the reopen resolves against the new directory, despite build.iso_path identifying the original absolute file. Please bind the stored path to its absolute construction-time location and cover a cwd change before the first lazy read.

[P3] This retains a relative path even though fighter members are reopened lazily. Constructing `DiscFrameData("game.iso")` in the ISO directory succeeds, but after changing the process working directory its first fighter query raises `DiscImageError` because the reopen resolves against the new directory, despite `build.iso_path` identifying the original absolute file. Please bind the stored path to its absolute construction-time location and cover a cwd change before the first lazy read.
@ -0,0 +203,4 @@
except OSError as exc:
stream.close()
raise DiscImageError(f"cannot stat disc image {self.path!s}: {exc}") from exc
if self._identity(metadata) != self._image_identity:
Author
Owner

[P3] This identity comparison occurs only before the subsequent seek/read, so an in-place writer can change the same inode immediately afterward and the altered member is accepted and cached under the original canonical build provenance. A synthetic probe changed PlFxAJ.dat after this fstat() and the lazy query returned the modified frame count without raising. Please compare descriptor identity again after each bounded read, before parsing or returning the bytes, and cover mutation between the pre-read check and read.

[P3] This identity comparison occurs only before the subsequent seek/read, so an in-place writer can change the same inode immediately afterward and the altered member is accepted and cached under the original canonical build provenance. A synthetic probe changed `PlFxAJ.dat` after this `fstat()` and the lazy query returned the modified frame count without raising. Please compare descriptor identity again after each bounded read, before parsing or returning the bytes, and cover mutation between the pre-read check and read.
@ -0,0 +199,4 @@
self._fail("fighter action table is null")
if actions % 4:
self._fail(f"fighter action table has unaligned data offset 0x{actions:X}")
if self.next_object_offset(fighter) < fighter + 0x60:
Author
Owner

[P2] Equal starts let incompatible required structures alias without tripping this greater-offset check. A DAT whose ftData root and action-table pointer both target offset 0 passes the 0x60 root check and, with Fox's expected count, returns 327 fabricated actions; the action and dynamic tables can likewise share a start and pass their extent checks. Please explicitly reject equal or overlapping ranges among the typed fighter descriptor, expected action table, and dynamic table, while retaining valid general-purpose pointer aliases, and cover both equal-start cases.

[P2] Equal starts let incompatible required structures alias without tripping this greater-offset check. A DAT whose `ftData` root and action-table pointer both target offset 0 passes the 0x60 root check and, with Fox's expected count, returns 327 fabricated actions; the action and dynamic tables can likewise share a start and pass their extent checks. Please explicitly reject equal or overlapping ranges among the typed fighter descriptor, expected action table, and dynamic table, while retaining valid general-purpose pointer aliases, and cover both equal-start cases.
@ -0,0 +505,4 @@
raise SubactionParseError(f"{context}: loop at DAT offset 0x{pc + 0x20:X} has zero count")
if loop_depth >= max_loop_depth:
raise SubactionParseError(f"{context}: loop depth guard exceeded {max_loop_depth}")
control_stack.append((pc + 4, count))
Author
Owner

[P2] This push is guarded only by the independent loop-depth limit, so aggregate call/loop occupancy can exceed the five words available between the fighter CommandInfo return stack and the following ColorOverlay. The interpreter accepts three nested loops ([3, 3, 3, 4, 4, 4, 0]); the third runtime loop push writes indices 4 and 5, with index 5 landing at Fighter+0x408 in ColorOverlay, yet parsing continues. Please bound each loop and call push so call_depth + 2 * loop_depth + pushed_words <= 5, and cover loop-only, call-only, and mixed overflow.

[P2] This push is guarded only by the independent loop-depth limit, so aggregate call/loop occupancy can exceed the five words available between the fighter `CommandInfo` return stack and the following `ColorOverlay`. The interpreter accepts three nested loops (`[3, 3, 3, 4, 4, 4, 0]`); the third runtime loop push writes indices 4 and 5, with index 5 landing at `Fighter+0x408` in `ColorOverlay`, yet parsing continues. Please bound each loop and call push so `call_depth + 2 * loop_depth + pushed_words <= 5`, and cover loop-only, call-only, and mixed overflow.
@ -109,0 +160,4 @@
assert disc_framedata is not None
state = disc_framedata.motion_state(character, action)
if (
not record.timeline.hitbox_generations
Author
Owner

[P2] This guard skips article uncertainty whenever an action also has fighter hitboxes, so mixed tether grabs silently return incomplete compatibility timing. On the canonical NTSC 1.02 ISO, Samus GRAB reports its first ISO-backed hitbox at frame 17 and WINDUP at frame 7, while the checked-in runtime capture includes its Grapple Beam marker at frame 7 and reports ATTACKING; Link and Young Link grabs have the same gap. Please raise for mixed article actions until their timelines can be merged, or include the article timing, and cover a tether grab.

[P2] This guard skips article uncertainty whenever an action also has fighter hitboxes, so mixed tether grabs silently return incomplete compatibility timing. On the canonical NTSC 1.02 ISO, Samus `GRAB` reports its first ISO-backed hitbox at frame 17 and `WINDUP` at frame 7, while the checked-in runtime capture includes its Grapple Beam marker at frame 7 and reports `ATTACKING`; Link and Young Link grabs have the same gap. Please raise for mixed article actions until their timelines can be merged, or include the article timing, and cover a tether grab.
fix: reject incomplete tether hitbox timing
All checks were successful
Linux tests / Python 3.12 (push) Successful in 34s
Linux tests / Python 3.13 (push) Successful in 35s
Linux tests / Python 3.11 (push) Successful in 37s
Linux tests / Python 3.11 (pull_request) Successful in 38s
Linux tests / Python 3.13 (pull_request) Successful in 33s
Linux tests / Python 3.12 (pull_request) Successful in 33s
5cdbbf2939
Author
Owner

Review comment #1219: Fixed in 5cdbbf2. ISO-backed hitbox queries now fail closed for the six Link, Young Link, and Samus standing/running tether-grab states whose scripts contain both fighter and article hitboxes; hitbox_count() uses the same uncertainty guard, while non-hitbox timing remains available. Added focused synthetic coverage, updated the framedata documentation and AGENTS guidance, and verified all six pairs against the canonical NTSC 1.02 ISO. Verification: 5 focused tests passed, full python test.py passed (353 tests), targeted Ruff and py_compile passed, and strict Sphinx completed with warnings as errors.

Review comment #1219: Fixed in `5cdbbf2`. ISO-backed hitbox queries now fail closed for the six Link, Young Link, and Samus standing/running tether-grab states whose scripts contain both fighter and article hitboxes; `hitbox_count()` uses the same uncertainty guard, while non-hitbox timing remains available. Added focused synthetic coverage, updated the framedata documentation and AGENTS guidance, and verified all six pairs against the canonical NTSC 1.02 ISO. Verification: 5 focused tests passed, full `python test.py` passed (353 tests), targeted Ruff and `py_compile` passed, and strict Sphinx completed with warnings as errors.
Author
Owner

Review comment #1220: Fixed in 5796edb. Fighter DAT validation now rejects pairwise-equal starts for the typed fighter root, action table, and dynamic behavior table before any range is decoded, while preserving valid duplicate targets in the generic HSD pointer table. Added regressions for each typed alias pair plus a control proving generic duplicate pointers remain accepted. Verification: 4 focused tests passed; the final full python test.py run passed (353 tests), targeted Ruff and py_compile passed.

Review comment #1220: Fixed in `5796edb`. Fighter DAT validation now rejects pairwise-equal starts for the typed fighter root, action table, and dynamic behavior table before any range is decoded, while preserving valid duplicate targets in the generic HSD pointer table. Added regressions for each typed alias pair plus a control proving generic duplicate pointers remain accepted. Verification: 4 focused tests passed; the final full `python test.py` run passed (353 tests), targeted Ruff and `py_compile` passed.
Author
Owner

Review comment #1221: Fixed in 8eabee6. Runtime traversal now accounts for the actual five-word event_return storage: calls consume one word and loop frames consume two, so call-only, loop-only, and mixed scripts fail before a physical return-stack overflow. The existing configurable parser depth remains an additional resource bound. Added boundary and overflow regressions for all three shapes. Verification: 4 focused tests passed; the final full python test.py run passed (353 tests), targeted Ruff and py_compile passed.

Review comment #1221: Fixed in `8eabee6`. Runtime traversal now accounts for the actual five-word `event_return` storage: calls consume one word and loop frames consume two, so call-only, loop-only, and mixed scripts fail before a physical return-stack overflow. The existing configurable parser depth remains an additional resource bound. Added boundary and overflow regressions for all three shapes. Verification: 4 focused tests passed; the final full `python test.py` run passed (353 tests), targeted Ruff and `py_compile` passed.
Author
Owner

Review comment #1222: Fixed in c1e14bc. GameCubeDisc now resolves the supplied image path at construction, so lazy reads remain bound to the originally validated file even if the process changes working directory. Added a regression that constructs from a relative path, changes cwd, and successfully reads a member from the original image. Verification: 7 focused disc-read tests passed; the final full python test.py run passed (353 tests), targeted Ruff and py_compile passed.

Review comment #1222: Fixed in `c1e14bc`. `GameCubeDisc` now resolves the supplied image path at construction, so lazy reads remain bound to the originally validated file even if the process changes working directory. Added a regression that constructs from a relative path, changes cwd, and successfully reads a member from the original image. Verification: 7 focused disc-read tests passed; the final full `python test.py` run passed (353 tests), targeted Ruff and `py_compile` passed.
Author
Owner

Review comment #1223: Fixed in c1e14bc. Every lazy FST, DOL, and member read now revalidates image identity after consuming the bytes, closing the same-inode replacement window that existed after the pre-read check. Deterministic regressions mutate the image through an injected read callback and assert that each read path raises DiscImageChangedError. Verification: 7 focused disc-read tests passed; the final full python test.py run passed (353 tests), targeted Ruff and py_compile passed.

Review comment #1223: Fixed in `c1e14bc`. Every lazy FST, DOL, and member read now revalidates image identity after consuming the bytes, closing the same-inode replacement window that existed after the pre-read check. Deterministic regressions mutate the image through an injected read callback and assert that each read path raises `DiscImageChangedError`. Verification: 7 focused disc-read tests passed; the final full `python test.py` run passed (353 tests), targeted Ruff and `py_compile` passed.
JayKobe6k left a comment

Found two actionable correctness and parser-integrity issues.

Found two actionable correctness and parser-integrity issues.
@ -0,0 +96,4 @@
def _validate_reference_chain(self, offset: int, index: int, locations: set[int]) -> None:
seen = set()
while offset != 0xFFFFFFFF:
Author
Owner

[P3] This loop treats an initial 0xFFFFFFFF reference-table target as an already-terminated chain, so no bounds check runs and HsdDat accepts a 4-byte DAT while exposing DatRoot(data_offset=4294967295). The sentinel terminates a link read from a valid in-data chain node; it is not itself a valid chain head. Please validate the table target as an aligned in-data offset before recognizing terminators and cover a sentinel-head input.

[P3] This loop treats an initial `0xFFFFFFFF` reference-table target as an already-terminated chain, so no bounds check runs and `HsdDat` accepts a 4-byte DAT while exposing `DatRoot(data_offset=4294967295)`. The sentinel terminates a link read from a valid in-data chain node; it is not itself a valid chain head. Please validate the table target as an aligned in-data offset before recognizing terminators and cover a sentinel-head input.
@ -0,0 +547,4 @@
target_state = (target, control_snapshot)
if opcode == 7 and target_state in seen_control_flow:
script_loop = True
break
Author
Owner

[P2] Breaking at the first repeated goto silently omits iterations that still execute within the current animation cycle. On the canonical ISO, Mewtwo NEUTRAL_B_CHARGING creates and clears its hitbox at times 0 and 3, jumps back at 5 in its 8-frame cycle, and therefore creates again before the cycle ends; the ISO facade reports last_hitbox_frame() == 2 and hitbox_count() == 1, while the checked-in runtime capture reports frame 6 and two hits. Please expand reachable loop iterations through the cycle boundary, or reject facade timing when script_loop_encountered makes it incomplete, and add this retail regression.

[P2] Breaking at the first repeated `goto` silently omits iterations that still execute within the current animation cycle. On the canonical ISO, Mewtwo `NEUTRAL_B_CHARGING` creates and clears its hitbox at times 0 and 3, jumps back at 5 in its 8-frame cycle, and therefore creates again before the cycle ends; the ISO facade reports `last_hitbox_frame() == 2` and `hitbox_count() == 1`, while the checked-in runtime capture reports frame 6 and two hits. Please expand reachable loop iterations through the cycle boundary, or reject facade timing when `script_loop_encountered` makes it incomplete, and add this retail regression.
fix: expand bounded subaction goto loops
All checks were successful
Linux tests / Python 3.13 (push) Successful in 35s
Linux tests / Python 3.12 (push) Successful in 36s
Linux tests / Python 3.11 (pull_request) Successful in 38s
Linux tests / Python 3.11 (push) Successful in 39s
Linux tests / Python 3.12 (pull_request) Successful in 33s
Linux tests / Python 3.13 (pull_request) Successful in 32s
315f93e2e8
Author
Owner

Review comment #1244: Fixed in 315f93e. Repeated goto control flow now expands only when it advances time toward a known finite animation endpoint, then stops at that initial-cycle boundary; zero-progress and unknown-duration loops keep the safe first-pass behavior, and counted SetLoop scripts remain unchanged. Added parser/facade coverage including Mewtwo alignment. Verification: 5 focused tests passed, full python test.py passed (354 tests), targeted Ruff and py_compile passed, strict Sphinx passed, and the canonical ISO audit parsed 33 fighters/10,658 actions with Mewtwo reporting first/last/count 1/6/2 and Game & Watch rapid jab still reporting 7 hits.

Review comment #1244: Fixed in `315f93e`. Repeated `goto` control flow now expands only when it advances time toward a known finite animation endpoint, then stops at that initial-cycle boundary; zero-progress and unknown-duration loops keep the safe first-pass behavior, and counted `SetLoop` scripts remain unchanged. Added parser/facade coverage including Mewtwo alignment. Verification: 5 focused tests passed, full `python test.py` passed (354 tests), targeted Ruff and `py_compile` passed, strict Sphinx passed, and the canonical ISO audit parsed 33 fighters/10,658 actions with Mewtwo reporting first/last/count `1/6/2` and Game & Watch rapid jab still reporting 7 hits.
Author
Owner

Review comment #1245: Fixed in afbccc4. External-reference validation now requires the table head itself to be an aligned in-data node before accepting either 0 or 0xFFFFFFFF read from that node as the chain terminator. Added a malformed sentinel-head regression while preserving both valid terminator forms. Verification: 2 focused DAT parser tests passed; final full python test.py passed (354 tests), targeted Ruff and py_compile passed.

Review comment #1245: Fixed in `afbccc4`. External-reference validation now requires the table head itself to be an aligned in-data node before accepting either `0` or `0xFFFFFFFF` read from that node as the chain terminator. Added a malformed sentinel-head regression while preserving both valid terminator forms. Verification: 2 focused DAT parser tests passed; final full `python test.py` passed (354 tests), targeted Ruff and `py_compile` passed.
Merge remote-tracking branch 'origin/master' into feat/iso-framedata-runtime-mapping
All checks were successful
Linux tests / Python 3.12 (push) Successful in 35s
Linux tests / Python 3.13 (push) Successful in 36s
Linux tests / Python 3.11 (push) Successful in 37s
Linux tests / Python 3.11 (pull_request) Successful in 38s
Linux tests / Python 3.13 (pull_request) Successful in 34s
Linux tests / Python 3.12 (pull_request) Successful in 34s
4fb86765b2
# Conflicts:
#	test.py
JayKobe6k left a comment

Found two actionable correctness and availability defects.

Found two actionable correctness and availability defects.
@ -0,0 +132,4 @@
def __init__(self, path: str | os.PathLike[str]):
self.path = Path(path).resolve()
try:
stream = self.path.open("rb")
Author
Owner

[P2] This opens the user-supplied path before checking S_ISREG, so on POSIX a FIFO with no writer blocks forever instead of raising DiscImageError; the same ordering in _open_validated_image() lets a FIFO swapped in before a lazy read hang before identity validation. An os.mkfifo() probe left GameCubeDisc(path) blocked until externally terminated. Please open nonblocking, fstat() the descriptor before wrapping it as a stream, and add POSIX-gated tests for initial and replacement FIFOs.

[P2] This opens the user-supplied path before checking `S_ISREG`, so on POSIX a FIFO with no writer blocks forever instead of raising `DiscImageError`; the same ordering in `_open_validated_image()` lets a FIFO swapped in before a lazy read hang before identity validation. An `os.mkfifo()` probe left `GameCubeDisc(path)` blocked until externally terminated. Please open nonblocking, `fstat()` the descriptor before wrapping it as a stream, and add POSIX-gated tests for initial and replacement FIFOs.
@ -0,0 +490,4 @@
if (
expanding_script_loop
and animation_frame_count is not None
and next_time >= animation_frame_count
Author
Owner

[P2] This new loop-expansion cutoff also fires when a wait lands exactly on the finite endpoint, before the following command can run. The runtime dispatches commands whose timer reaches zero before the animation callback; retail Mewtwo DAT actions 296/297/301/302 and Kirby actions 436/437/441/442 therefore execute Clear Hitboxes at time 8, but these timelines omit that event and expose only seven snapshots. Please retain commands at equality, stop only when the wait overshoots the endpoint, and cover exact-endpoint command execution in a loop-expanded script.

[P2] This new loop-expansion cutoff also fires when a wait lands exactly on the finite endpoint, before the following command can run. The runtime dispatches commands whose timer reaches zero before the animation callback; retail Mewtwo DAT actions 296/297/301/302 and Kirby actions 436/437/441/442 therefore execute `Clear Hitboxes` at time 8, but these timelines omit that event and expose only seven snapshots. Please retain commands at equality, stop only when the wait overshoots the endpoint, and cover exact-endpoint command execution in a loop-expanded script.
fix: retain exact-endpoint loop commands
All checks were successful
Linux tests / Python 3.13 (push) Successful in 37s
Linux tests / Python 3.12 (push) Successful in 37s
Linux tests / Python 3.11 (pull_request) Successful in 39s
Linux tests / Python 3.11 (push) Successful in 40s
Linux tests / Python 3.13 (pull_request) Successful in 34s
Linux tests / Python 3.12 (pull_request) Successful in 34s
42665d5bdd
Author
Owner

Review comment #1279: Fixed in b1cd6d4. Initial and lazy disc-image opens now use O_NONBLOCK until fstat() confirms the descriptor is a regular file, so a user-supplied or replacement FIFO raises DiscImageError without waiting for a writer. Added POSIX-gated, timeout-protected subprocess regressions for both paths. Verification: 5 focused disc identity/FIFO tests passed, targeted Ruff and py_compile passed, and final python test.py passed all 368 tests.

Review comment #1279: Fixed in `b1cd6d4`. Initial and lazy disc-image opens now use `O_NONBLOCK` until `fstat()` confirms the descriptor is a regular file, so a user-supplied or replacement FIFO raises `DiscImageError` without waiting for a writer. Added POSIX-gated, timeout-protected subprocess regressions for both paths. Verification: 5 focused disc identity/FIFO tests passed, targeted Ruff and `py_compile` passed, and final `python test.py` passed all 368 tests.
Author
Owner

Review comment #1280: Fixed in 42665d5. Loop expansion now stops only when a wait overshoots the finite endpoint, allowing commands due exactly at equality to execute. The regression now retains the endpoint clear and eighth snapshot while preserving the two Mewtwo hit windows. Verification: 4 focused interpreter tests passed; the canonical ISO audit parsed 33 fighters/10,658 actions and all eight affected Mewtwo/Kirby records now end with Clear Hitboxes at time 8; targeted Ruff, py_compile, strict Sphinx, and the full 368-test suite passed.

Review comment #1280: Fixed in `42665d5`. Loop expansion now stops only when a wait overshoots the finite endpoint, allowing commands due exactly at equality to execute. The regression now retains the endpoint clear and eighth snapshot while preserving the two Mewtwo hit windows. Verification: 4 focused interpreter tests passed; the canonical ISO audit parsed 33 fighters/10,658 actions and all eight affected Mewtwo/Kirby records now end with `Clear Hitboxes` at time 8; targeted Ruff, `py_compile`, strict Sphinx, and the full 368-test suite passed.
JayKobe6k left a comment

Found two actionable correctness and portability issues.

Found two actionable correctness and portability issues.
@ -0,0 +181,4 @@
file_descriptor = os.open(self.path, flags)
except IsADirectoryError as exc:
raise DiscImageError(f"disc image is not a regular file: {self.path!s}") from exc
except OSError as exc:
Author
Owner

[P2] On Windows, opening a directory with os.open raises PermissionError/WinError 5 rather than IsADirectoryError, so the supported Windows path falls into this generic branch and the new unconditional directory test fails because it receives cannot open instead of regular file. Please classify directories in this error path while retaining descriptor-level fstat validation for replacement races, and cover or emulate Windows' error mapping.

[P2] On Windows, opening a directory with `os.open` raises `PermissionError`/WinError 5 rather than `IsADirectoryError`, so the supported Windows path falls into this generic branch and the new unconditional directory test fails because it receives `cannot open` instead of `regular file`. Please classify directories in this error path while retaining descriptor-level `fstat` validation for replacement races, and cover or emulate Windows' error mapping.
@ -0,0 +557,4 @@
if opcode == 7 and target_state in seen_control_flow:
script_loop = True
repeated_state = (target, time, timer, control_snapshot)
if not animation_frame_count or time >= animation_frame_count or repeated_state in seen:
Author
Owner

[P2] This still stops before following a repeated goto when that jump itself is reached exactly at the finite endpoint, so zero-time commands at its target are omitted even though command dispatch precedes the animation callback. On the canonical ISO, Kirby actions 412/420 and Jigglypuff actions 302/310 all reach an endpoint goto to Clear Hitboxes; their 60-frame timelines stop at 59 and omit the clear at time 60. Please follow the endpoint jump and stop at the first subsequent overshooting wait or repeated full state, and cover a target command reached by an endpoint goto.

[P2] This still stops before following a repeated `goto` when that jump itself is reached exactly at the finite endpoint, so zero-time commands at its target are omitted even though command dispatch precedes the animation callback. On the canonical ISO, Kirby actions 412/420 and Jigglypuff actions 302/310 all reach an endpoint `goto` to `Clear Hitboxes`; their 60-frame timelines stop at 59 and omit the clear at time 60. Please follow the endpoint jump and stop at the first subsequent overshooting wait or repeated full state, and cover a target command reached by an endpoint `goto`.
fix: classify Windows disc directories
All checks were successful
Linux tests / Python 3.13 (push) Successful in 37s
Linux tests / Python 3.12 (push) Successful in 37s
Linux tests / Python 3.11 (pull_request) Successful in 39s
Linux tests / Python 3.11 (push) Successful in 39s
Linux tests / Python 3.12 (pull_request) Successful in 34s
Linux tests / Python 3.13 (pull_request) Successful in 34s
fb7cb7ef98
Author
Owner

Review comment #1293: Fixed in 7da6e70. Repeated gotos reached exactly at the finite endpoint now follow their target before the existing overshooting-wait or repeated-state guards stop expansion. Added a regression whose endpoint goto reaches a zero-time hitbox command. Verification: 4 focused endpoint/control-flow tests passed; the canonical NTSC 1.02 audit parsed 33 fighters/10,658 actions and Kirby 412/420 plus Jigglypuff 302/310 now retain Clear Hitboxes at time 60 with 60 snapshots; affected-source Ruff and py_compile, strict Sphinx, and full python test.py (368 tests) passed.

Review comment #1293: Fixed in `7da6e70`. Repeated gotos reached exactly at the finite endpoint now follow their target before the existing overshooting-wait or repeated-state guards stop expansion. Added a regression whose endpoint goto reaches a zero-time hitbox command. Verification: 4 focused endpoint/control-flow tests passed; the canonical NTSC 1.02 audit parsed 33 fighters/10,658 actions and Kirby 412/420 plus Jigglypuff 302/310 now retain `Clear Hitboxes` at time 60 with 60 snapshots; affected-source Ruff and `py_compile`, strict Sphinx, and full `python test.py` (368 tests) passed.
Author
Owner

Review comment #1294: Fixed in fb7cb7e. Windows-style directory PermissionError is now normalized to the regular-file diagnostic after confirming the path is a directory, while permission failures for regular files retain the generic open error and descriptor-level fstat validation remains unchanged. Added portable regressions for both classifications. Verification: 4 focused disc/FIFO/identity tests passed; affected-source Ruff and py_compile, strict Sphinx, canonical 33-fighter/10,658-action ISO audit, and full python test.py (368 tests) passed.

Review comment #1294: Fixed in `fb7cb7e`. Windows-style directory `PermissionError` is now normalized to the regular-file diagnostic after confirming the path is a directory, while permission failures for regular files retain the generic open error and descriptor-level `fstat` validation remains unchanged. Added portable regressions for both classifications. Verification: 4 focused disc/FIFO/identity tests passed; affected-source Ruff and `py_compile`, strict Sphinx, canonical 33-fighter/10,658-action ISO audit, and full `python test.py` (368 tests) passed.
JayKobe6k left a comment

Found two actionable portability and deployment issues.

Found two actionable portability and deployment issues.
@ -0,0 +130,4 @@
_MAX_MEMBER_SIZE = 0x214D00
def __init__(self, path: str | os.PathLike[str]):
self.path = Path(path).resolve()
Author
Owner

[P3] Resolving the path before the guarded open leaks RuntimeError for a circular symlink on supported Python 3.10-3.12 (Path.resolve() only stopped raising for non-strict symlink loops in 3.13), so DiscFrameData(path) bypasses the DiscImageError normalization used for other malformed or unopenable image paths. This is reproducible on 3.11 and 3.12. Please normalize resolution failures, or bind the absolute path without eagerly resolving symlinks, and add a version-compatible circular-symlink regression.

[P3] Resolving the path before the guarded open leaks `RuntimeError` for a circular symlink on supported Python 3.10-3.12 (`Path.resolve()` only stopped raising for non-strict symlink loops in 3.13), so `DiscFrameData(path)` bypasses the `DiscImageError` normalization used for other malformed or unopenable image paths. This is reproducible on 3.11 and 3.12. Please normalize resolution failures, or bind the absolute path without eagerly resolving symlinks, and add a version-compatible circular-symlink regression.
JayKobe6k marked this conversation as resolved
@ -0,0 +367,4 @@
if len(data) != dol_size:
raise DiscImageError("could not read the complete DOL")
self._assert_image_unchanged(stream)
digest = hashlib.sha1(data).hexdigest()
Author
Owner

[P2] This unconditional SHA-1 constructor is blocked on real FIPS-enforced deployments such as RHEL 9, where hashlib.sha1(data) raises UnsupportedDigestmodError/ValueError; because DiscFrameData.__init__() always reads the DOL, even the canonical ISO becomes unusable before any query. Please authenticate the DOL with a FIPS-supported digest such as SHA-256, update the pinned fingerprint and diagnostic, and cover a blocked-SHA-1 runtime.

[P2] This unconditional SHA-1 constructor is blocked on real FIPS-enforced deployments such as RHEL 9, where `hashlib.sha1(data)` raises `UnsupportedDigestmodError`/`ValueError`; because `DiscFrameData.__init__()` always reads the DOL, even the canonical ISO becomes unusable before any query. Please authenticate the DOL with a FIPS-supported digest such as SHA-256, update the pinned fingerprint and diagnostic, and cover a blocked-SHA-1 runtime.
JayKobe6k marked this conversation as resolved
Author
Owner

Review comment #1293: Reverified at current head fb7cb7e; the fix remains in pushed commit 7da6e70, so no additional code change was needed. The pre-fix parent omits the endpoint target, while current code follows the repeated goto at equality and retains the target command at time 60 for Kirby 412/420 and Jigglypuff 302/310. Tests: python -m unittest test.DiscFrameDataTests.test_repeated_goto_expands_to_animation_endpoint test.DiscFrameDataTests.test_reached_state_changes_extend_exclusive_animation_endpoint test.DiscFrameDataTests.test_control_flow_and_lossless_command_lengths test.DiscFrameDataTests.test_wrapped_animation_frame_drives_async_timers (4 passed); canonical ISO audit parsed the affected records correctly; python test.py (368 passed); affected-source Ruff, py_compile, and strict Sphinx passed.

Review comment #1293: Reverified at current head `fb7cb7e`; the fix remains in pushed commit `7da6e70`, so no additional code change was needed. The pre-fix parent omits the endpoint target, while current code follows the repeated goto at equality and retains the target command at time 60 for Kirby 412/420 and Jigglypuff 302/310. Tests: `python -m unittest test.DiscFrameDataTests.test_repeated_goto_expands_to_animation_endpoint test.DiscFrameDataTests.test_reached_state_changes_extend_exclusive_animation_endpoint test.DiscFrameDataTests.test_control_flow_and_lossless_command_lengths test.DiscFrameDataTests.test_wrapped_animation_frame_drives_async_timers` (4 passed); canonical ISO audit parsed the affected records correctly; `python test.py` (368 passed); affected-source Ruff, `py_compile`, and strict Sphinx passed.
Author
Owner

Review comment #1294: Reverified at current head fb7cb7e; the fix remains in pushed commit fb7cb7e, so no additional code change was needed. Deterministic Windows-style PermissionError emulation now classifies directories as non-regular files while preserving cannot open for denied regular files, and descriptor-level fstat validation remains intact. Tests: python -m unittest test.DiscFrameDataTests.test_iso_validation_fst_bounded_read_and_exact_pairing test.DiscFrameDataTests.test_disc_image_rejects_fifos_without_blocking test.DiscFrameDataTests.test_disc_construction_rejects_in_place_change_after_identity_check test.DiscFrameDataTests.test_disc_member_read_rejects_in_place_change_after_identity_check test.DiscFrameDataTests.test_disc_dol_read_rejects_in_place_change_after_identity_check (5 passed); python test.py (368 passed); affected-source Ruff, py_compile, and strict Sphinx passed.

Review comment #1294: Reverified at current head `fb7cb7e`; the fix remains in pushed commit `fb7cb7e`, so no additional code change was needed. Deterministic Windows-style `PermissionError` emulation now classifies directories as non-regular files while preserving `cannot open` for denied regular files, and descriptor-level `fstat` validation remains intact. Tests: `python -m unittest test.DiscFrameDataTests.test_iso_validation_fst_bounded_read_and_exact_pairing test.DiscFrameDataTests.test_disc_image_rejects_fifos_without_blocking test.DiscFrameDataTests.test_disc_construction_rejects_in_place_change_after_identity_check test.DiscFrameDataTests.test_disc_member_read_rejects_in_place_change_after_identity_check test.DiscFrameDataTests.test_disc_dol_read_rejects_in_place_change_after_identity_check` (5 passed); `python test.py` (368 passed); affected-source Ruff, `py_compile`, and strict Sphinx passed.
feat: evaluate static fighter hitbox poses
All checks were successful
Linux tests / Python 3.12 (push) Successful in 37s
Linux tests / Python 3.13 (push) Successful in 38s
Linux tests / Python 3.11 (pull_request) Successful in 40s
Linux tests / Python 3.11 (push) Successful in 41s
Linux tests / Python 3.12 (pull_request) Successful in 34s
Linux tests / Python 3.13 (pull_request) Successful in 34s
540f8d58b6
feat: select ISO framedata from environment
All checks were successful
Linux tests / Python 3.12 (push) Successful in 36s
Linux tests / Python 3.13 (push) Successful in 37s
Linux tests / Python 3.11 (pull_request) Successful in 38s
Linux tests / Python 3.11 (push) Successful in 39s
Linux tests / Python 3.12 (pull_request) Successful in 35s
Linux tests / Python 3.13 (pull_request) Successful in 35s
3ccc3ca3f5
feat: derive framedata queries from ISO
All checks were successful
Linux tests / Python 3.12 (push) Successful in 37s
Linux tests / Python 3.13 (push) Successful in 38s
Linux tests / Python 3.11 (push) Successful in 40s
Linux tests / Python 3.11 (pull_request) Successful in 40s
Linux tests / Python 3.13 (pull_request) Successful in 35s
Linux tests / Python 3.12 (pull_request) Successful in 35s
0aab04c96b
refactor: derive hitbox ranges from segments
All checks were successful
Linux tests / Python 3.12 (push) Successful in 37s
Linux tests / Python 3.13 (push) Successful in 37s
Linux tests / Python 3.11 (push) Successful in 40s
Linux tests / Python 3.11 (pull_request) Successful in 40s
Linux tests / Python 3.13 (pull_request) Successful in 35s
Linux tests / Python 3.12 (pull_request) Successful in 36s
593aa1f706
feat: expose animation root motion
All checks were successful
Linux tests / Python 3.12 (push) Successful in 38s
Linux tests / Python 3.13 (push) Successful in 39s
Linux tests / Python 3.11 (pull_request) Successful in 41s
Linux tests / Python 3.11 (push) Successful in 43s
Linux tests / Python 3.12 (pull_request) Successful in 35s
Linux tests / Python 3.13 (pull_request) Successful in 35s
d255abefd1
All checks were successful
Linux tests / Python 3.12 (push) Successful in 38s
Linux tests / Python 3.13 (push) Successful in 39s
Linux tests / Python 3.11 (pull_request) Successful in 41s
Linux tests / Python 3.11 (push) Successful in 43s
Linux tests / Python 3.12 (pull_request) Successful in 35s
Linux tests / Python 3.13 (pull_request) Successful in 35s
This pull request has changes conflicting with the target branch.
  • melee/bot/character_state.py
  • melee/bot/framedata_query.py
  • melee/framedata.py
  • test.py
View command line instructions

Manual merge helper

Use this merge commit message when completing the merge manually.

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin feat/iso-framedata-runtime-mapping:feat/iso-framedata-runtime-mapping
git switch feat/iso-framedata-runtime-mapping

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch master
git merge --no-ff feat/iso-framedata-runtime-mapping
git switch feat/iso-framedata-runtime-mapping
git rebase master
git switch master
git merge --ff-only feat/iso-framedata-runtime-mapping
git switch feat/iso-framedata-runtime-mapping
git rebase master
git switch master
git merge --no-ff feat/iso-framedata-runtime-mapping
git switch master
git merge --squash feat/iso-framedata-runtime-mapping
git switch master
git merge --ff-only feat/iso-framedata-runtime-mapping
git switch master
git merge feat/iso-framedata-runtime-mapping
git push origin master
Sign in to join this conversation.
No description provided.