MagicBeanDev/Hookshot
Archived
Watch
6
0
Fork
You've already forked Hookshot
 0
Code Issues 125 Pull requests 205 Activity

fix(build): a new work folder's second build no longer translates the game again #2674

Closed
github-sync wants to merge 2 commits from github/pr/2674 into main
pull from: github/pr/2674
merge into: MagicBeanDev:main
Conversation 3 Commits 2 Files changed 3 +43 -5
github-sync commented 2026-11-12 06:22:34 +00:00
Owner

@jordanella opened this pull request on GitHub · 2026-11-12 05:48 UTC · original
Merged on GitHub in c544828ff2

In a new work folder (a fresh worktree, or a player's new install), the second build translates the whole game again even though nothing changed: 319 s for a no-op rebuild on Windows, 290 s of it the lift. For players, the first update after a new install doesn't get to keep the translation (#2324).

Cause

lift_key hashes image/hot_pieces.txt, split_hot's kept pieces, and the first lift in a work folder is what writes that file. The key was taken before that lift and stored after it. So the next build, which now finds hot_pieces.txt, computes another key and translates again. After that the key is stable, which is why the problem only shows once per folder.

Change

  • translate() takes the key again after the lift and split_hot, from the files the next build will read, and stores that one. keep_translation uses it too (nonlocal key).
  • A lift run from those inputs (the pieces this lift wrote, the boundaries it kept) writes the same C. If they ever differ, the build relifts, as it does today.

Testing

  • tests/test_lift_cache.py:
    • A new check fails without the change and passes with it: the key is taken again after split_hot, and a fresh folder's key from before its first lift isn't the next build's.
    • The test also read the lift's module list by whitespace, so it failed in any checkout whose path has a space in it (here E:\Hobby Development\...). It now reads by lines. CI's paths have no spaces, so CI was not affected.
  • tests/test_lift_cleanup_error.py runs the fresh-lift branch with stubs; it now stands in for lift_key and expects lift → key taken again → key written. Against main's make_game.py it fails, with the change it passes.
  • End to end on Windows (Ryzen 9 3900X, NVMe, main at 0ec31ff6 plus this change): in a work folder with no hot_pieces.txt and no key, the first build took 315 s (the lift is needed) and the second 14 s, skipping the lift. Without the change, the second took 319 s.
  • Full run_all.py on Windows: every check that reads make_game.py (52) passes, apart from test_cpp_preflight, test_linux_update and test_prepare_stall, which fail the same way on a clean main here.

Not tested

A player's update through the Builder end to end. It takes the same path in make_game.py (keep_translation, then kept_before with this key), but I haven't run it through the Builder.

Part of the dev-loop proposal's Phase 0 (#2636).

🤖 Generated with Claude Code

> [@jordanella](https://github.com/jordanella) opened this pull request on GitHub · 2026-11-12 05:48 UTC · [original](https://github.com/NotRexed/Hookshot/pull/2674) > **Merged on GitHub** in https://git.magicbean.dev/MagicBeanDev/Hookshot/commit/c544828ff2d545829505cd894bb7968c7c2c5fcb In a new work folder (a fresh worktree, or a player's new install), the second build translates the whole game again even though nothing changed: 319 s for a no-op rebuild on Windows, 290 s of it the lift. For players, the first update after a new install doesn't get to keep the translation (#2324). ## Cause `lift_key` hashes `image/hot_pieces.txt`, split_hot's kept pieces, and the first lift in a work folder is what writes that file. The key was taken *before* that lift and stored after it. So the next build, which now finds `hot_pieces.txt`, computes another key and translates again. After that the key is stable, which is why the problem only shows once per folder. ## Change - `translate()` takes the key again after the lift and split_hot, from the files the next build will read, and stores that one. `keep_translation` uses it too (`nonlocal key`). - A lift run from those inputs (the pieces this lift wrote, the boundaries it kept) writes the same C. If they ever differ, the build relifts, as it does today. ## Testing - **`tests/test_lift_cache.py`:** - A new check fails without the change and passes with it: the key is taken again after split_hot, and a fresh folder's key from before its first lift isn't the next build's. - The test also read the lift's module list by whitespace, so it failed in any checkout whose path has a space in it (here `E:\Hobby Development\...`). It now reads by lines. CI's paths have no spaces, so CI was not affected. - **`tests/test_lift_cleanup_error.py`** runs the fresh-lift branch with stubs; it now stands in for `lift_key` and expects lift → key taken again → key written. Against `main`'s `make_game.py` it fails, with the change it passes. - **End to end on Windows** (Ryzen 9 3900X, NVMe, `main` at 0ec31ff6 plus this change): in a work folder with no `hot_pieces.txt` and no key, the first build took 315 s (the lift is needed) and the second 14 s, skipping the lift. Without the change, the second took 319 s. - **Full `run_all.py` on Windows:** every check that reads `make_game.py` (52) passes, apart from `test_cpp_preflight`, `test_linux_update` and `test_prepare_stall`, which fail the same way on a clean `main` here. ## Not tested A player's update through the Builder end to end. It takes the same path in `make_game.py` (`keep_translation`, then `kept_before` with this key), but I haven't run it through the Builder. Part of the dev-loop proposal's Phase 0 (#2636). 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- github-sync:item:2674 -->
The lift key hashes image/hot_pieces.txt, split_hot's kept pieces, which the first lift in a work folder writes. The key
was taken before that lift and stored after it, so in a new work folder (a fresh worktree, a new install) the next build
never matched it and translated the whole game again for nothing: 319 s for a no-op rebuild on Windows, 290 s of it the
lift. For players, the first update after a new install did not keep the translation (#2324).

translate() now takes the key again after the lift and split_hot, from what the next build reads, and stores that one;
keep_translation uses it too. tests/test_lift_cache.py checks the order, and that the key before a first lift is not the
next build's. It also reads the lift's module list by lines, not by whitespace, so it passes in a checkout whose path has
a space in it.

Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
tests/test_lift_cleanup_error.py runs make_game's fresh-lift branch with stubs; it now stands in for lift_key too and
expects the order the previous commit makes: the lift, the key taken again, the key written.

Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
github-sync closed this pull request 2026-11-12 06:24:11 +00:00
github-sync commented 2026-11-12 06:24:12 +00:00
Author
Owner

@mooki000 commented on GitHub · 2026-11-12 06:00 UTC · original

One case to close before this merges; the fix itself does what it says (the second build in a new work folder no longer translates again).

The self-produced hot_pieces.txt fix is sound, but recomputing the whole key at 7a561e39:nx2/make_game.py:2181–2182 can certify output made from an older tracked input.
I reproduced this without a game: real production key/stamp/cache-predicate functions and the actual translate() AST, with tiny authored files and stand-ins only for the lift/split children (the fixture; it reads both commits from a clone).

  1. The child reads the old synthetic nx2/lift.py input and emits OLD output.
  2. While that child “runs”, the fixture changes that non-self input; the child also writes chunks.txt/hot_pieces.txt normally.
  3. The parent stamps its output, then a second build asks the real lifted_before predicate about the current inputs.
    Observed Linux/Python 3.12 fixture output (next_lift counts child calls in that second build):
base 87991bc8: old_output_certified=False, next_lift=1  PASS
head 7a561e39: old_output_certified=True,  next_lift=0  FAIL (exit 1)
guarded head: old_output_certified=False, next_lift=1  PASS (first lift refused)

The base stamped the pre-lift key, so the changed input safely forced the next lift; this PR stamps the changed-input key over OLD output.
An in-memory observation comparing non-self inputs before/after and refusing to stamp when they change fixes this case; the self-file-only control still skips the second lift, preserving your intended improvement.
Please refresh the self-produced planning inputs only after confirming the remaining inputs are unchanged, or use an immutable input snapshot.
This window requires a tracked input to change during a lift—for example an edit while its minutes of work run—not an ordinary unchanged build; no compiler, game or GPU was used.

> [@mooki000](https://github.com/mooki000) commented on GitHub · 2026-11-12 06:00 UTC · [original](https://github.com/NotRexed/Hookshot/pull/2674#issuecomment-6816001205) One case to close before this merges; the fix itself does what it says (the second build in a new work folder no longer translates again). The self-produced `hot_pieces.txt` fix is sound, but recomputing the **whole** key at `7a561e39:nx2/make_game.py:2181–2182` can certify output made from an older tracked input. I reproduced this without a game: real production key/stamp/cache-predicate functions and the actual `translate()` AST, with tiny authored files and stand-ins only for the lift/split children ([the fixture](https://gist.githubusercontent.com/mooki000/b86bd3f35969803fb032f71f035844f8/raw/efba25623bdaebc9ba2bd4101513a198d9b7e782/hookshot-2674-test_lift_key_input_race.py); it reads both commits from a clone). 1. The child reads the old synthetic `nx2/lift.py` input and emits OLD output. 2. While that child “runs”, the fixture changes that non-self input; the child also writes `chunks.txt`/`hot_pieces.txt` normally. 3. The parent stamps its output, then a second build asks the real `lifted_before` predicate about the current inputs. Observed Linux/Python 3.12 fixture output (`next_lift` counts child calls in that second build): ```text base 87991bc8: old_output_certified=False, next_lift=1 PASS head 7a561e39: old_output_certified=True, next_lift=0 FAIL (exit 1) guarded head: old_output_certified=False, next_lift=1 PASS (first lift refused) ``` The base stamped the **pre-lift** key, so the changed input safely forced the next lift; this PR stamps the changed-input key over OLD output. An in-memory observation comparing non-self inputs before/after and refusing to stamp when they change fixes this case; the self-file-only control still skips the second lift, preserving your intended improvement. Please refresh the self-produced planning inputs only after confirming the remaining inputs are unchanged, or use an immutable input snapshot. This window requires a tracked input to change **during a lift**—for example an edit while its minutes of work run—not an ordinary unchanged build; no compiler, game or GPU was used. <!-- github-sync:c:6816001205 -->
github-sync commented 2026-11-12 06:24:12 +00:00
Author
Owner

@hckrman101 approved on GitHub · 2026-11-12 06:06 UTC · original

Thanks, this is a clean fix and the cause is explained well.

I checked the parts that have to hold for it to be safe:

  • The new key matches what the next build computes. lift_key hashes image/hot_pieces.txt, and split_hot writes it during the lift. The cleanup step removes sa and probe but leaves image/, so the next build hashes the same file. build.py writes its kept list header from .lift_key (lift_key_of), which now holds the new key, and keep_translation checks against the same key through nonlocal key. So the player's kept_before path agrees with the dev path.
  • Skipping the lift on that key is correct. With prev equal to the pieces this lift just saved and the same bodies, sticky_split puts every function back in its own piece. Its largest cost then equals the fresh split's, so the sticky split wins, and each piece is written as sorted(names). The relift would write the same C, so the claim in the description holds. A stale hot_pieces.txt that split_hot dropped as out of balance is handled too: the key now picks up the rewritten file.
  • The second pass of the cut-short loop still relifts. The key file is removed first, so a key changed by the first pass can't match anything.
  • The tests. test_lift_cleanup_error compiles only the if lifted_before branch, so key = lift_key(...) rebinds the key in the test's namespace, and the stub sees exactly one rekey between lift and key. The splitlines() fix in test_lift_cache is a real fix for checkouts whose path has a space in it.

Re-hashing costs one extra lift_key call (an import walk and hashes of a few files) after a lift that takes minutes anyway, so that's fine.

CI: when I looked, the Python source checks on Windows (both parts) and Ubuntu part 2/2 were still running, along with the Builder package and DLSS/FSR link check jobs. Everything finished so far passes. Good to merge once those are green (Windows part 2/2 currently fails on main on test_psychov35/test_pcache_header_read; those failures aren't from this PR).

> [@hckrman101](https://github.com/hckrman101) approved on GitHub · 2026-11-12 06:06 UTC · [original](https://github.com/NotRexed/Hookshot/pull/2674#pullrequestreview-5869864640) Thanks, this is a clean fix and the cause is explained well. I checked the parts that have to hold for it to be safe: - **The new key matches what the next build computes.** `lift_key` hashes `image/hot_pieces.txt`, and `split_hot` writes it during the lift. The cleanup step removes `sa` and `probe` but leaves `image/`, so the next build hashes the same file. `build.py` writes its kept list header from `.lift_key` (`lift_key_of`), which now holds the new key, and `keep_translation` checks against the same key through `nonlocal key`. So the player's `kept_before` path agrees with the dev path. - **Skipping the lift on that key is correct.** With `prev` equal to the pieces this lift just saved and the same bodies, `sticky_split` puts every function back in its own piece. Its largest cost then equals the fresh split's, so the sticky split wins, and each piece is written as `sorted(names)`. The relift would write the same C, so the claim in the description holds. A stale `hot_pieces.txt` that split_hot dropped as out of balance is handled too: the key now picks up the rewritten file. - **The second pass of the cut-short loop still relifts.** The key file is removed first, so a key changed by the first pass can't match anything. - **The tests.** `test_lift_cleanup_error` compiles only the `if lifted_before` branch, so `key = lift_key(...)` rebinds the key in the test's namespace, and the stub sees exactly one `rekey` between `lift` and `key`. The `splitlines()` fix in `test_lift_cache` is a real fix for checkouts whose path has a space in it. Re-hashing costs one extra `lift_key` call (an import walk and hashes of a few files) after a lift that takes minutes anyway, so that's fine. CI: when I looked, the Python source checks on Windows (both parts) and Ubuntu part 2/2 were still running, along with the Builder package and DLSS/FSR link check jobs. Everything finished so far passes. Good to merge once those are green (Windows part 2/2 currently fails on main on test_psychov35/test_pcache_header_read; those failures aren't from this PR). <!-- github-sync:r:5869864640 -->
github-sync commented 2026-11-12 06:24:12 +00:00
Author
Owner

@jordanella commented on GitHub · 2026-11-12 06:12 UTC · original

Thanks, @mooki000: you're right, and I missed that window. Taking the whole key after the lift lets a translator file edited mid-lift vouch for C made from the old one. #2686 does what you suggested. It snapshots everything but the lift's own planning files (lift_key(..., planning=False)) as the lift starts, compares after it, and takes the key again only if nothing else changed; otherwise the pre-lift key stays and the next build relifts, as on the base. test_lift_cleanup_error.py has the mid-lift edit as a case, and it fails on main. I couldn't run your gist fixture here (this machine's tool permissions), so a run of it against #2686's head would be very welcome.

> [@jordanella](https://github.com/jordanella) commented on GitHub · 2026-11-12 06:12 UTC · [original](https://github.com/NotRexed/Hookshot/pull/2674#issuecomment-6816216668) Thanks, @mooki000: you're right, and I missed that window. Taking the whole key after the lift lets a translator file edited mid-lift vouch for C made from the old one. #2686 does what you suggested. It snapshots everything but the lift's own planning files (`lift_key(..., planning=False)`) as the lift starts, compares after it, and takes the key again only if nothing else changed; otherwise the pre-lift key stays and the next build relifts, as on the base. `test_lift_cleanup_error.py` has the mid-lift edit as a case, and it fails on `main`. I couldn't run your gist fixture here (this machine's tool permissions), so a run of it against #2686's head would be very welcome. <!-- github-sync:c:6816216668 -->
Commenting is not possible because the repository is archived.
Reviewers
No reviewers
Labels
Clear labels   
accessibility
Barrier affecting people with disabilities

  
autorelease: pending

   
autorelease: tagged

   
bug
Something isn't working

  
dependencies
Pull requests that update a dependency file

  
documentation
Improvements or additions to documentation

  
duplicate
This issue or pull request already exists

  
enhancement
New feature or request

  
github_actions
Pull requests that update GitHub Actions code

  
good first issue
Good for newcomers

  
help wanted
Extra attention is needed

  
invalid
This doesn't seem right

  
question
Further information is requested

  
testing wanted

   
wontfix
This will not be worked on

No labels
accessibility
autorelease: pending
autorelease: tagged
bug
dependencies
documentation
duplicate
enhancement
github_actions
good first issue
help wanted
invalid
question
testing wanted
wontfix
Milestone
Clear milestone
No items
No milestone
Assignees
Clear assignees
No assignees
1 participant
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
MagicBeanDev/Hookshot!2674
No description provided.