Skip to content

fix: identify hooks by parse time index instead of memory address - #14

Closed
usefulish wants to merge 1 commit into
desbma:masterfrom
usefulish:pr/stable-hook-identity
Closed

fix: identify hooks by parse time index instead of memory address#14
usefulish wants to merge 1 commit into
desbma:masterfrom
usefulish:pr/stable-hook-identity

Conversation

@usefulish

Copy link
Copy Markdown
Contributor

FolderHookId is the hook's memory address, so identity only works while the hook map is built once and never moved or mutated, and clones of the same hook get different identities. Any future map rebuild (config reload) or restructuring would silently break the allow_concurrent bookkeeping.

Assign each hook a unique index during FolderConfig deserialization (so no parsing path can forget it) and use that as identity. FolderHookId gets a test-only Debug derive for the assertions.

Red-green test: hook_id_is_stable_and_unique fails on master (a clone gets a different id than the original), passes with the fix.

🤖 Generated with Claude Code

FolderHookId was the hook's memory address, which only works while the
hook map is built once and never moved or mutated, and gives clones of
the same hook different identities. Assign each hook a unique index
during deserialization and use that instead.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
desbma added a commit that referenced this pull request Jul 15, 2026
@desbma

desbma commented Jul 15, 2026

Copy link
Copy Markdown
Owner

Good catch on the fragility, although it is latent rather than a live bug: ids are only ever computed from references into the hook map, which is built once and never mutated. I addressed it differently in d92fa3a: FolderHook is no longer Clone and the map stores references into the parsed config, so both assumptions (one allocation per hook, no moves) are compiler-enforced, without an index field that silently defaults to 0 when construction bypasses deserialization. Closing in favor of that.

@desbma desbma closed this Jul 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants