Skip to content
This repository was archived by the owner on Jun 11, 2026. It is now read-only.

Fix class availability watcher timing#45

Merged
steipete merged 2 commits into
masterfrom
fix/class-load-watcher
Jun 11, 2026
Merged

Fix class availability watcher timing#45
steipete merged 2 commits into
masterfrom
fix/class-load-watcher

Conversation

@steipete

Copy link
Copy Markdown
Owner

Summary

  • observe newly loaded Objective-C classes only after runtime registration is complete
  • replay image state for callbacks registered after the global hook
  • retain a bounded, race-safe legacy fallback for older supported Apple platforms
  • add real-runtime normal and forced-legacy regression probes, CocoaPods source coverage, and CI coverage

Root cause

_dyld_register_func_for_add_image runs before Objective-C has registered classes from the image. The previous watcher queried immediately in that callback, missed the class, and did not retry until an unrelated later image load.

A macOS arm64 probe reproduced the ordering as callback=missing after_dlopen=visible. The new implementation uses objc_addLoadImageFunc where available and a bounded Objective-C registration check for older supported systems.

Verification

  • bash Tests/verify_class_load_watcher.sh
  • IKT_FORCE_DYLD_IMAGE_CALLBACK=1 bash Tests/verify_class_load_watcher.sh
  • bash Tests/verify_release_linking.sh
  • swift test and swift test -c release: 16 tests passed in each configuration
  • macOS arm64 Xcode coverage tests with UTF-16 and UTF-8
  • iOS 26.5 simulator Xcode coverage tests
  • watchOS 26.5 simulator build
  • autoreview clean with no accepted/actionable findings

Local tvOS verification was unavailable because this Xcode installation has no tvOS runtime; GitHub Actions retains the tvOS job.

Closes #26

@clawsweeper

clawsweeper Bot commented Jun 11, 2026

Copy link
Copy Markdown

Codex review: found issues before merge. Reviewed June 11, 2026, 6:32 AM ET / 10:32 UTC.

Summary
The branch moves class availability waiting from immediate dyld callbacks to Objective-C image-load callbacks with a bounded dyld fallback, exports the callback bridge, and adds packaging, CI, and runtime probe coverage.

Reproducibility: yes. at source/probe level: current master installs the watcher from _dyld_register_func_for_add_image, and the PR body reports a macOS arm64 ordering probe as callback=missing after_dlopen=visible. I did not execute the Xcode probe in this read-only review.

Review metrics: 2 noteworthy metrics.

  • Changed surface: 11 files, 344 additions, 25 deletions. The PR spans runtime code, public headers, CocoaPods packaging, CI, and a new probe script, so source review alone is not enough.
  • Public callback bridge: 1 exported C function added. IKTRegisterImageDidLoadCallback becomes public header surface and should be reviewed as an ABI/API addition, not only an internal Swift refactor.

Merge readiness
Overall: 🦐 gold shrimp
Proof: 🦞 diamond lobster
Patch quality: 🦐 gold shrimp
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • [P2] Fix the legacy fallback to choose the 32-bit or 64-bit section scanner from header->magic, then rerun the forced-legacy class watcher probe.

Risk before merge

  • [P1] The forced/legacy dyld path may still miss Objective-C class sections on arm64_32 watchOS 5 until class-list scanning uses the loaded Mach-O header magic rather than __LP64__.
  • [P1] I did not run Xcode builds or the new shell probe because this review had to keep the checkout read-only; the PR body provides runtime verification and CI should remain the merge gate.

Maintainer options:

  1. Decide the mitigation before merge
    Land the watcher timing fix after correcting legacy Mach-O section detection and letting the normal and forced-legacy runtime probes pass on the declared Apple platform matrix.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

  • [P2] Owner-authored PR should remain in maintainer review; the remaining blocker is a narrow watchOS legacy fallback correctness fix plus completion of the platform verification gate.

Security
Cleared: The diff adds local runtime probes and narrows a Homebrew install command; I found no concrete security or supply-chain regression in the patch.

Review findings

  • [P2] Detect Mach-O width from the loaded header — Sources/SuperBuilder/src/ITKSuperBuilder.m:72
Review details

Best possible solution:

Land the watcher timing fix after correcting legacy Mach-O section detection and letting the normal and forced-legacy runtime probes pass on the declared Apple platform matrix.

Do we have a high-confidence way to reproduce the issue?

Yes, at source/probe level: current master installs the watcher from _dyld_register_func_for_add_image, and the PR body reports a macOS arm64 ordering probe as callback=missing after_dlopen=visible. I did not execute the Xcode probe in this read-only review.

Is this the best way to solve the issue?

No, not quite yet. Moving the primary path to objc_addLoadImageFunc is the right direction, but the legacy fallback should detect Mach-O width from the loaded header before this is the narrowest maintainable fix for all declared platforms.

Full review comments:

  • [P2] Detect Mach-O width from the loaded header — Sources/SuperBuilder/src/ITKSuperBuilder.m:72
    On watchOS 5 arm64_32, __LP64__ is false even though loaded images use 64-bit Mach-O headers. That sends the legacy dyld fallback through the 32-bit getsectiondata path, so it can miss __objc_classlist, skip the Objective-C registration wait, and fire before NSClassFromString can see the class; branch on header->magic/MH_MAGIC_64 instead.
    Confidence: 0.86

Overall correctness: patch is incorrect
Overall confidence: 0.86

AGENTS.md: not found in the target repository.

Codex review notes: model internal, reasoning high; reviewed against fdea299f53da.

Label changes

Label changes:

  • add P2: This is a normal-priority runtime bug fix with limited blast radius, plus one blocking platform-compatibility finding before merge.
  • add rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦞 diamond lobster and patch quality is 🦐 gold shrimp.
  • add status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Not applicable: The external-contributor proof gate does not apply to this owner-authored PR, though the PR body includes substantial local runtime verification commands and outcomes.
  • remove status: 👀 ready for maintainer look: Current PR status label is status: ⏳ waiting on author.
  • remove rating: 🐚 platinum hermit: Current PR rating is rating: 🦐 gold shrimp, so this older rating label is no longer current.

Label justifications:

  • P2: This is a normal-priority runtime bug fix with limited blast radius, plus one blocking platform-compatibility finding before merge.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦞 diamond lobster and patch quality is 🦐 gold shrimp.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Not applicable: The external-contributor proof gate does not apply to this owner-authored PR, though the PR body includes substantial local runtime verification commands and outcomes.
Evidence reviewed

What I checked:

  • Target policy check: No AGENTS.md was present inside the target InterposeKit repository; a parent ClawSweeper AGENTS.md was read but did not apply as target-repo policy.
  • Current behavior being fixed: Current master registers the class watcher directly from the dyld add-image callback, which matches the reported timing problem where class lookup can run before Objective-C registration completes. (Sources/InterposeKit/Watcher.swift:87, fdea299f53da)
  • PR implementation path: The PR head registers IKTRegisterImageDidLoadCallback, uses objc_addLoadImageFunc on newer Apple OS versions, and retains a dyld fallback for older supported systems. (Sources/SuperBuilder/src/ITKSuperBuilder.m:195, 7b4a268b67d2)
  • Actionable legacy fallback issue: The PR chooses the getsectiondata overload with #if __LP64__, while the repository still declares watchOS 5 support and existing comments identify watchOS arm64_32 as an ILP32 target, so header magic is the safer source of truth. (Sources/SuperBuilder/src/ITKSuperBuilder.m:72, 7b4a268b67d2)
  • Declared supported platform floor: The package manifest declares .watchOS(.v5), so the pre-watchOS 6 legacy callback path remains relevant to the PR’s stated supported-platform coverage. (Package.swift:10, fdea299f53da)
  • Feature history: The watcher and original dyld registration path appear to date to Peter Steinberger’s Hook individual objects commit, and the PR commits are also authored by Peter Steinberger. (Sources/InterposeKit/Watcher.swift:87, 9e387e1e8936)

Likely related people:

  • steipete: Git history shows Peter Steinberger introduced the watcher/dyld path in Hook individual objects, authored the PR commits, and has recent adjacent SuperBuilder fixes in the same native helper area. (role: feature owner and current PR author; confidence: high; commits: 9e387e1e8936, ebe65e8385b4, 7b4a268b67d2; files: Sources/InterposeKit/Watcher.swift, Sources/SuperBuilder/src/ITKSuperBuilder.m, Sources/SuperBuilder/include/ITKSuperBuilder.h)
  • ishutinvv: Recent history shows adjacent work in ITKSuperBuilder.m, though not specifically the class-load watcher fallback. (role: recent adjacent contributor; confidence: medium; commits: c3a2ef45274f; files: Sources/SuperBuilder/src/ITKSuperBuilder.m)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ebe65e8385

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +72 to +73
#if __LP64__
const struct mach_header_64 *header64 = (const struct mach_header_64 *)header;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Detect Mach-O width from the loaded header

On watchOS 5 arm64_32, __LP64__ is false even though the loaded images use 64-bit Mach-O headers. That sends the legacy dyld fallback through the 32-bit getsectiondata path, so it can miss __objc_classlist, skip the Objective-C registration wait, and fire the watcher before NSClassFromString can see the class. Since watchOS 5 is still a declared deployment target and this fallback is specifically for pre-watchOS 6, branch on header->magic/MH_MAGIC_64 rather than the compile-time pointer model.

Useful? React with 👍 / 👎.

@clawsweeper clawsweeper Bot added rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Jun 11, 2026
@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. P2 Normal priority bug or improvement with limited blast radius. and removed rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Jun 11, 2026
@steipete steipete merged commit 20945a4 into master Jun 11, 2026
7 checks passed
@steipete steipete deleted the fix/class-load-watcher branch June 11, 2026 10:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

P2 Normal priority bug or improvement with limited blast radius. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

_dyld_register_func_for_add_image callback is run before Objective-C classes are loaded

1 participant