fix(installer): ensure hooks/lib is copied with correct permissions (fixes #2185)#2188
Open
Yeachan-Heo wants to merge 1 commit intodevfrom
Open
fix(installer): ensure hooks/lib is copied with correct permissions (fixes #2185)#2188Yeachan-Heo wants to merge 1 commit intodevfrom
Yeachan-Heo wants to merge 1 commit intodevfrom
Conversation
…issions The ensureStandaloneHookScripts function had two redundant loops that copied files from templates/hooks/lib to ~/.claude/hooks/lib. The first loop did not set executable permissions (0o755), which caused ERR_MODULE_NOT_FOUND errors when hooks tried to import stdin.mjs and atomic-write.mjs after omc update. Removed the redundant first loop, keeping only the second loop that properly sets permissions with chmodSync(targetPath, 0o755). Fixes #2185 Constraint: Both loops targeted the same source/destination directories Constraint: Second loop already handled all required lib files with permissions Rejected: Merge the loops - simpler to remove the redundant one entirely Confidence: high Scope-risk: narrow Directive: Any future lib file copy operations must set executable permissions
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Owner
Author
|
Review note: diff scope looks narrow and targeted (installer loop cleanup + regression tests). Current CI failure is still the same repo-wide base red we saw on other PRs, not an installer-specific regression:
Keeping this open while the base test failures are handled separately. — |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #2185:
omc updatewas not copyingtemplates/hooks/libfiles with executable permissions, causingERR_MODULE_NOT_FOUNDerrors when hooks tried to importstdin.mjsandatomic-write.mjs.Root Cause
The
ensureStandaloneHookScriptsfunction insrc/installer/index.tshad two redundant copy loops:Both loops targeted the same directories, so the first loop's unpermissioned copies would be overwritten by the second loop. However, if the first loop partially failed or if there were race conditions, files could end up without proper permissions.
Fix
Removed the redundant first loop that didn't set permissions. The second loop already handles all lib files correctly with proper permissions.
Changes
src/installer/index.ts: Removed redundant copy loop (12 lines)src/__tests__/installer.test.ts: Added regression tests for Bug: omc update does not copy hooks/lib/ directory, causing ERR_MODULE_NOT_FOUND #2185Testing
Regression Tests Added
should have all required lib files in templates/hooks/lib- Verifies stdin.mjs, atomic-write.mjs, config-dir.mjs existshould have all standalone hook template files present- Verifies all 7 main hook files existCloses #2185