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

fix(builder): stop when damaged resources have no replacement #2096

Merged
Arkex-ai merged 5 commits from fix/unresolved-damaged-files into main 2026-11-11 19:29:28 +00:00
Conversation 6 Commits 5 Files changed 4 +143 -23
Arkex-ai commented 2026-11-11 02:27:49 +00:00 (Migrated from github.com)

Summary

The Builder could compile damaged files with no replacement, then mark the check successful and skip the damage on retry. It now stops before lift, leaves a persistent list of the affected files, and retries the check until the dump is repaired. Addresses the unresolved-resource part of #2005.

Technical changes

  • Write the damage report before failing, preserving the report added by merged #2254 and the UTF-8/legacy stamp handling from #2253.
  • Write resolved-v1 only after every detected damaged file has a replacement. Preserve unresolved extraction heads for retries; recheck older success markers once.
  • Cover fresh, cached and update entry points, mixed repairs, Unicode stamps and a successful retry after adding a counterpart. Keep the dump-size guidance and distinguish a replacement from a build-stopping missing replacement.

Validation

Tested head c215f012, rebased onto main 6df80a8d. Focused cache-entry suite passes on Windows and Linux (17 methods; Windows skips one link-privilege control), plus eight NUP head/stamp tests and the stand-in checks on both systems. The cached rebuild/update regression verifies the unresolved report exists before the failure, while the successful marker remains absent and retry heads remain intact. Hosted checks will run on this head.

Existing installations rescan their romfs once when replacing a legacy checked marker: about 100,000 file opens, potentially minutes. The common 23.3 GB dump remains accepted when all damaged files have replacements. The worklist's 1% reach is a provisional planning estimate, not measured prevalence.

Earlier full-suite evidence and limits

At 1f96189b on exact base 7759a6f3, full Linux candidate and baseline each passed 373 / 0 failed / 23 skipped, using the same filesystem and toolchain. Full Windows candidate passed 363 / 2 failed / 31 skipped, with the same two failures as its exact base: the recorder destructor negative control and NUP link privileges. This is earlier-head evidence; the subsequent rebase and diagnostic wording changes have focused validation above, with fresh hosted checks pending.

These are authored, game-free fixtures. No complete real-dump build or gameplay validation is claimed for this pre-build gate. Donor integrity is handled separately by #2019, already merged. #2391's author is coordinating a later combined resolved-v2 marker so wrapped-file detection still invalidates older checks.

Dev-work claim.

Current-head workflow Result
Game-free source regressions Needs attention
Linux AppImage Passed
PR title Passed
Vulkan back end with NVIDIA DLSS Passed
Windows builder Passed
## Summary The Builder could compile damaged files with no replacement, then mark the check successful and skip the damage on retry. It now stops before lift, leaves a persistent list of the affected files, and retries the check until the dump is repaired. Addresses the unresolved-resource part of #2005. ## Technical changes - Write the damage report before failing, preserving the report added by merged #2254 and the UTF-8/legacy stamp handling from #2253. - Write `resolved-v1` only after every detected damaged file has a replacement. Preserve unresolved extraction heads for retries; recheck older success markers once. - Cover fresh, cached and update entry points, mixed repairs, Unicode stamps and a successful retry after adding a counterpart. Keep the dump-size guidance and distinguish a replacement from a build-stopping missing replacement. ## Validation Tested head **`c215f012`**, rebased onto main **`6df80a8d`**. Focused cache-entry suite passes on Windows and Linux (17 methods; Windows skips one link-privilege control), plus eight NUP head/stamp tests and the stand-in checks on both systems. The cached rebuild/update regression verifies the unresolved report exists before the failure, while the successful marker remains absent and retry heads remain intact. Hosted [checks](https://git.magicbean.dev/MagicBeanDev/Hookshot/pulls/2096/checks) will run on this head. Existing installations rescan their romfs once when replacing a legacy checked marker: about 100,000 file opens, potentially minutes. The common 23.3 GB dump remains accepted when all damaged files have replacements. The worklist's 1% reach is a provisional planning estimate, not measured prevalence. <details> <summary>Earlier full-suite evidence and limits</summary> At **`1f96189b`** on exact base **`7759a6f3`**, full Linux candidate and baseline each passed **373 / 0 failed / 23 skipped**, using the same filesystem and toolchain. Full Windows candidate passed **363 / 2 failed / 31 skipped**, with the same two failures as its exact base: the recorder destructor negative control and NUP link privileges. This is earlier-head evidence; the subsequent rebase and diagnostic wording changes have focused validation above, with fresh hosted checks pending. These are authored, game-free fixtures. No complete real-dump build or gameplay validation is claimed for this pre-build gate. Donor integrity is handled separately by #2019, already merged. #2391's author is coordinating a later combined `resolved-v2` marker so wrapped-file detection still invalidates older checks. </details> [Dev-work claim](https://discord.com/channels/1568810484818378804/1569019786766188665/1569897037862805726). <!-- pr-ci-status --> | Current-head workflow | Result | |---|---| | [Game-free source regressions](https://github.com/NotRexed/Hookshot/actions/runs/43074702738) | Needs attention | | [Linux AppImage](https://github.com/NotRexed/Hookshot/actions/runs/43074702672) | Passed | | [PR title](https://github.com/NotRexed/Hookshot/actions/runs/43074698986) | Passed | | [Vulkan back end with NVIDIA DLSS](https://github.com/NotRexed/Hookshot/actions/runs/43074702730) | Passed | | [Windows builder](https://github.com/NotRexed/Hookshot/actions/runs/43074702704) | Passed | <!-- /pr-ci-status -->
Arkex-ai commented 2026-11-11 03:28:16 +00:00 (Migrated from github.com)

Rebased onto main ca082bfc at b2502067; the authored patch is unchanged. The focused cache/header checks pass. Full local Windows is 339 passed / 4 failed / 28 skipped, with all four failures reproduced on the exact base; Ubuntu is still running. Fresh upstream runs are linked in the description, and workflow lint is now green. The remaining finish-output fixture correction is tracked in #2102.

Rebased onto main `ca082bfc` at `b2502067`; the authored patch is unchanged. The focused cache/header checks pass. Full local Windows is 339 passed / 4 failed / 28 skipped, with all four failures reproduced on the exact base; Ubuntu is still running. Fresh upstream runs are linked in the description, and workflow lint is now green. The remaining finish-output fixture correction is tracked in #2102.
Arkex-ai commented 2026-11-11 11:53:55 +00:00 (Migrated from github.com)

The earlier AppImage run stopped before compilation on a Launchpad HTTP504 signing-key request.

Update: superseded by the rebase onto main 49f75dc8, head 085b1666. Fresh checks are running on the current head; no rerun of the old attempt is needed. The authored patch is unchanged by range-diff. Current CI links are in the PR description.

The earlier [AppImage run](https://github.com/NotRexed/Hookshot/actions/runs/43014889429) stopped before compilation on a Launchpad HTTP504 signing-key request. Update: superseded by the rebase onto main `49f75dc8`, head `085b1666`. Fresh checks are running on the current head; no rerun of the old attempt is needed. The authored patch is unchanged by range-diff. Current CI links are in the PR description.
hckrman101 (Migrated from github.com) approved these changes 2026-11-11 16:29:40 +00:00
hckrman101 (Migrated from github.com) left a comment

Looks right to me. Stopping on a damaged file with no stand-in is the right call for #2005 (77 all-zero files, the game can't reach the first frame), and writing the marker only after repair succeeds fixes the "checked once, skipped forever" hole. The tests cover fresh, cached, update, mixed and legacy-marker paths through the real entry point.

On false positives, I checked the case main lets through on purpose: the common 23.3 GB dump. Its three damaged textures all get a stand-in (#1308 lists the picks), so unresolved is 0 there and those builds go through as before. What gets blocked now is only a dump where some damaged file has no whole sibling of the same kind. Before this, main left those files as zeros and built anyway. There's no override for someone whose unresolved file is never loaded, but given #2005 I think stopping is the better default.

Costs and nits, none blocking:

  • Every existing install with a legacy .hookshot_romfs_checked rescans the whole romfs once on its next build or update (about 100k files opened, minutes with a virus scanner). That's unavoidable with this design, but it's worth one line in the description so nobody reads it as a regression.
  • The worklist row scores reach 1.0 as an "upper bound". The 23.3 GB dumps aren't affected (all resolved), so the real reach is the rare heavily damaged dump, and 5.0 puts it at the top of the list. A lower estimate would sort it more honestly.
  • Overlap with #2254: both rewrite the same print block in stand_in_damaged_files, so whichever lands second will conflict (bronze-ff has said they'll rebase). Semantically, once this is in, #2254 should write hookshot_damaged_files.txt before the fail(). Otherwise the unresolved case, the one most worth leaving a note for, never gets the file. Its premise that "a later build skips this check" also only holds for resolved dumps after this PR.

CI: everything passes except the AppImage job, which was cancelled at "System packages" after the 1 h timeout. That's infra, not this change, and main's AppImage runs at 49f75dc8 and 32693f02 fail too. Good to merge from my side.

Looks right to me. Stopping on a damaged file with no stand-in is the right call for #2005 (77 all-zero files, the game can't reach the first frame), and writing the marker only after repair succeeds fixes the "checked once, skipped forever" hole. The tests cover fresh, cached, update, mixed and legacy-marker paths through the real entry point. On false positives, I checked the case main lets through on purpose: the common 23.3 GB dump. Its three damaged textures all get a stand-in (#1308 lists the picks), so `unresolved` is 0 there and those builds go through as before. What gets blocked now is only a dump where some damaged file has no whole sibling of the same kind. Before this, main left those files as zeros and built anyway. There's no override for someone whose unresolved file is never loaded, but given #2005 I think stopping is the better default. Costs and nits, none blocking: - Every existing install with a legacy `.hookshot_romfs_checked` rescans the whole romfs once on its next build or update (about 100k files opened, minutes with a virus scanner). That's unavoidable with this design, but it's worth one line in the description so nobody reads it as a regression. - The worklist row scores reach 1.0 as an "upper bound". The 23.3 GB dumps aren't affected (all resolved), so the real reach is the rare heavily damaged dump, and 5.0 puts it at the top of the list. A lower estimate would sort it more honestly. - Overlap with #2254: both rewrite the same print block in `stand_in_damaged_files`, so whichever lands second will conflict (bronze-ff has said they'll rebase). Semantically, once this is in, #2254 should write `hookshot_damaged_files.txt` before the `fail()`. Otherwise the unresolved case, the one most worth leaving a note for, never gets the file. Its premise that "a later build skips this check" also only holds for resolved dumps after this PR. CI: everything passes except the AppImage job, which was cancelled at "System packages" after the 1 h timeout. That's infra, not this change, and main's AppImage runs at 49f75dc8 and 32693f02 fail too. Good to merge from my side.
docs/worklist.md
hckrman101 (Migrated from github.com) commented 2026-11-11 16:29:40 +00:00

Nit: 23.3 GB dumps resolve all three files, so reach 1.0 overstates this a lot. I'd score it on the heavily damaged dump subset (#2005-like) so it doesn't sit at the top of the list.

Nit: 23.3 GB dumps resolve all three files, so reach 1.0 overstates this a lot. I'd score it on the heavily damaged dump subset (#2005-like) so it doesn't sit at the top of the list.
nx2/make_game.py
hckrman101 (Migrated from github.com) commented 2026-11-11 16:29:40 +00:00

This makes every existing install do the full scan once on its next build or update, since nothing in the old marker tells resolved from unresolved. That's fine, but please note it in the PR description (the progress line does show it at least).

This makes every existing install do the full scan once on its next build or update, since nothing in the old marker tells resolved from unresolved. That's fine, but please note it in the PR description (the progress line does show it at least).
nx2/make_game.py
@ -460,8 +460,11 @@ def stand_in_damaged_files(out, romfs):
return
hckrman101 (Migrated from github.com) commented 2026-11-11 16:29:40 +00:00

Nit: the old text gave the sizes (complete about 23.8 GB, incomplete 23.3 GB). That's the quickest way for a player to tell which dump they have, and triage leans on it. Worth keeping.

Nit: the old text gave the sizes (complete about 23.8 GB, incomplete 23.3 GB). That's the quickest way for a player to tell which dump they have, and triage leans on it. Worth keeping.
hckrman101 (Migrated from github.com) commented 2026-11-11 16:29:40 +00:00

Nit: when the build then stops, "nothing similar found, left as it is" reads as if the build carried on with it. Something like "no replacement found" would match the error below.

Nit: when the build then stops, "nothing similar found, left as it is" reads as if the build carried on with it. Something like "no replacement found" would match the error below.
Arkex-ai commented 2026-11-11 16:41:14 +00:00 (Migrated from github.com)

Thanks for the review. Rebased at 35415b00; the #2253 conflict now preserves its stamp encoding fix and the resolved-only check. The new Unicode integration case and existing cache/stand-in tests pass on Windows and Linux. I added the one-time legacy rescan cost to the description and lowered the worklist reach to an explicitly provisional 1% estimate.

For #2254, agreed: the damaged-file report should be written before this failure, including unresolved files. Fresh CI is starting; earlier results remain labeled in the description.

Thanks for the review. Rebased at `35415b00`; the #2253 conflict now preserves its stamp encoding fix and the resolved-only check. The new Unicode integration case and existing cache/stand-in tests pass on Windows and Linux. I added the one-time legacy rescan cost to the description and lowered the worklist reach to an explicitly provisional 1% estimate. For #2254, agreed: the damaged-file report should be written before this failure, including unresolved files. Fresh CI is starting; earlier results remain labeled in the description.
hckrman101 (Migrated from github.com) approved these changes 2026-11-11 16:49:48 +00:00
hckrman101 (Migrated from github.com) left a comment

Re-checked at 35415b00 (rebased; the code change since my approval is the #2253 merge plus the Unicode-stamp test, and the resolved-only marker still reads right: only a fully resolved check writes resolved-v1, and a legacy marker is dropped and rescanned once).

Fixed: the one-time full rescan is now in the description; the worklist reach is lowered to a provisional 1%.

Still open, all nits: the dump-size hint is still gone from the message, and "left as it is" is still there (inline). The new worklist row sits after a blank line, so it falls outside the table and won't render as a row; "provisional1percent" and "common23.3GB" are also missing spaces.

The #2254 ordering (report written before fail()) is agreed in the thread, and this print block already comes before it. CI: AppImage failed on apt mirror Ign errors during system packages (not the change); the rest was still running when I looked.

Re-checked at 35415b00 (rebased; the code change since my approval is the #2253 merge plus the Unicode-stamp test, and the resolved-only marker still reads right: only a fully resolved check writes `resolved-v1`, and a legacy marker is dropped and rescanned once). Fixed: the one-time full rescan is now in the description; the worklist reach is lowered to a provisional 1%. Still open, all nits: the dump-size hint is still gone from the message, and "left as it is" is still there (inline). The new worklist row sits after a blank line, so it falls outside the table and won't render as a row; "provisional1percent" and "common23.3GB" are also missing spaces. The #2254 ordering (report written before `fail()`) is agreed in the thread, and this print block already comes before it. CI: AppImage failed on apt mirror `Ign` errors during system packages (not the change); the rest was still running when I looked.
docs/worklist.md
hckrman101 (Migrated from github.com) commented 2026-11-11 16:49:47 +00:00

Nit: the blank line above this row ends the table, so the row won't render as part of it. Move it up next to the other rows (and keep the score column numeric, plus the missing spaces in "provisional1percent" / "common23.3GB").

Nit: the blank line above this row ends the table, so the row won't render as part of it. Move it up next to the other rows (and keep the score column numeric, plus the missing spaces in "provisional1percent" / "common23.3GB").
nx2/make_game.py
hckrman101 (Migrated from github.com) commented 2026-11-11 16:49:47 +00:00

Nit (from before): the sizes (complete about 23.8 GB, incomplete 23.3 GB) are still gone. They are the quickest way for a player to tell which dump they have.

Nit (from before): the sizes (complete about 23.8 GB, incomplete 23.3 GB) are still gone. They are the quickest way for a player to tell which dump they have.
hckrman101 (Migrated from github.com) commented 2026-11-11 16:49:47 +00:00

Nit (from before): "left as it is" still reads as if the build carries on with it; "no replacement found" would match the error below.

Nit (from before): "left as it is" still reads as if the build carries on with it; "no replacement found" would match the error below.
Arkex-ai commented 2026-11-11 18:07:21 +00:00 (Migrated from github.com)

Updated at c215f012 on main 6df80a8d. The merged #2254 report is now written before the unresolved failure, with a regression covering cached rebuild and update paths. The review nits are addressed too: dump-size guidance, clear missing-replacement wording and the worklist table/spacing. Focused tests pass on Windows and Linux; earlier full-suite results remain labeled by SHA in the description. Fresh CI is starting. #2391 is coordinating the subsequent resolved-v2 stamp for wrapped-file detection.

Updated at c215f012 on main 6df80a8d. The merged #2254 report is now written before the unresolved failure, with a regression covering cached rebuild and update paths. The review nits are addressed too: dump-size guidance, clear missing-replacement wording and the worklist table/spacing. Focused tests pass on Windows and Linux; earlier full-suite results remain labeled by SHA in the description. Fresh CI is starting. #2391 is coordinating the subsequent resolved-v2 stamp for wrapped-file detection.
Commenting is not possible because the repository is archived.
Reviewers
No reviewers
hckrman101
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!2096
No description provided.