fix: check-dead-exports hook silently no-ops on ESM codebases#394
fix: check-dead-exports hook silently no-ops on ESM codebases#394carlos-alm wants to merge 8 commits intomainfrom
Conversation
|
Claude finished @carlos-alm's task —— View job PR Review Complete ✅Reviewing PR #394: fix: check-dead-exports hook silently no-ops on ESM codebases Todo List
Phase 1: Root Cause AnalysisProblem Being Solved: The git hook Why This Approach: Converting to ESM with Risk Assessment: Low risk. This is a targeted fix that addresses the exact root cause without changing functionality. Phase 2: Configuration Change Detection ✅No HIGH-RISK changes detected:
This is purely a bug fix with no configuration relaxation. Phase 3: Test Integrity ✅No RED FLAGS detected:
Phase 4: Code Quality ReviewTechnical Implementation Analysis: ✅ Correct ESM Migration (lines 67-74):
✅ Shell Quoting Fix (lines 98, 105):
✅ Error Handling Preserved:
✅ No Security Issues:
Minor Considerations:
Final AssessmentTest Plan ValidationThe provided test plan is appropriate:
This covers the critical functionality that was previously broken. |
Greptile SummaryThis PR fixes a long-standing silent no-op in the Key changes:
Confidence Score: 4/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A([PreToolUse: Bash tool invoked]) --> B[Parse tool_input.command via node -e CJS]
B --> C{COMMAND empty?}
C -- yes --> Z([exit 0 — pass])
C -- no --> D{matches git commit?}
D -- no --> Z
D -- yes --> E{graph.db exists?}
E -- no --> Z
E -- yes --> F{staged files?}
F -- no --> Z
F -- yes --> G{session-edits.log exists?}
G -- no --> Z
G -- yes --> H[Filter staged files to src/ JS/TS edited this session]
H --> I{FILES_TO_CHECK empty?}
I -- yes --> Z
I -- no --> J["node --input-type=module -e (ESM)
import fs, path, pathToFileURL
argv[2]=WORK_ROOT, argv[3]=FILES_TO_CHECK
pathToFileURL → await import(queries.js)
Build publicAPI from index.js exports
scanDynamic: collect dynamic-import names
exportsData per file → collect dead names"]
J --> K{DEAD_EXPORTS non-empty?}
K -- no --> Z
K -- yes --> L[Build REASON string]
L --> M["node -e (CJS) emit JSON deny response
argv[2]=REASON → permissionDecisionReason"]
M --> N([exit 0 — commit blocked])
Last reviewed commit: 5b2ef44 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
.claude/hooks/check-dead-exports.sh
Outdated
| const root = process.argv[1]; | ||
| const files = process.argv[2].split('\n').filter(Boolean); |
There was a problem hiding this comment.
process.argv off-by-one — hook is still a no-op
When Node.js is invoked with node -e "..." "$WORK_ROOT" "$FILES_TO_CHECK", process.argv[1] is not $WORK_ROOT — it is set to '' (empty string) or [stdin] for inline -e/--input-type=module scripts. The actual user-supplied arguments begin at index 2 and 3 respectively.
This means:
root = process.argv[1]→''(empty string)path.join('', 'src/queries.js')→'src/queries.js'(relative)fileUrlbecomes'file:///src/queries.js', which resolves to/src/queries.json the filesystem — a path that does not exist.
The await import(fileUrl) call will throw, the error is swallowed by 2>/dev/null and || true, and DEAD_EXPORTS remains empty — the hook is still a silent no-op even after the ESM fix.
The fix requires bumping both indices by one:
| const root = process.argv[1]; | |
| const files = process.argv[2].split('\n').filter(Boolean); | |
| const root = process.argv[2]; | |
| const files = process.argv[3].split('\n').filter(Boolean); |
(This is a pre-existing indexing bug that was masked while require() was in place, but it now becomes the primary blocker since the ESM import path is correctly reached.)
There was a problem hiding this comment.
Fixed in 062b404. Changed process.argv[1] to process.argv[2] and process.argv[2] to process.argv[3] to account for the -e flag consuming argv[1] when using node -e "...".
There was a problem hiding this comment.
Already addressed in commit 062b404 — argv indices were shifted to [2]/[3] to account for -e scripts.
.claude/hooks/check-dead-exports.sh
Outdated
| const files = process.argv[2].split('\n').filter(Boolean); | ||
|
|
||
| const { exportsData } = require(path.join(root, 'src/queries.js')); | ||
| const fileUrl = 'file:///' + path.join(root, 'src/queries.js').replace(/\\\\/g, '/'); |
There was a problem hiding this comment.
Double-slash in Unix file URL — use pathToFileURL instead
On Unix, path.join(root, 'src/queries.js') returns an absolute path like /home/user/project/src/queries.js. Prepending 'file:///' produces:
file:////home/user/project/src/queries.js // 4 slashes
The WHATWG URL parser sees an empty host and a pathname of //home/user/project/src/queries.js. Node's fileURLToPath then returns //home/user/project/src/queries.js, which POSIX treats like /path on most Linux distros — but the double-leading-slash is implementation-defined behaviour and not guaranteed.
On Windows the result (file:///C:/...) is correct, so this only surfaces on Unix.
The idiomatic Node.js solution is to let url.pathToFileURL handle both platforms:
| const fileUrl = 'file:///' + path.join(root, 'src/queries.js').replace(/\\\\/g, '/'); | |
| const { pathToFileURL } = await import('node:url'); | |
| const fileUrl = pathToFileURL(path.join(root, 'src/queries.js')).href; |
Alternatively, use 'file://' + path.join(root, 'src/queries.js') (two slashes, not three) on Unix, but pathToFileURL is the recommended portable API.
There was a problem hiding this comment.
Fixed in 062b404. Replaced the manual file:/// + path construction with pathToFileURL() from node:url, which correctly handles both Unix and Windows paths (no double-slash on Unix, proper drive-letter handling on Windows).
There was a problem hiding this comment.
Already addressed in commit 062b404 — switched to pathToFileURL for portable file URLs.
|
Claude finished @carlos-alm's task —— View job Reviewing PR #394: fix: check-dead-exports hook silently no-ops on ESM codebases Todo List
Phase 1: Root Cause AnalysisProblem Being Solved: The git hook Why This Approach: Converting to ESM with Risk Assessment: Low risk. This is a targeted fix that addresses the exact root cause without changing functionality. CRITICAL ISSUE CONFIRMED: Both Greptile and the previous reviewer are correct about the
Phase 2: Configuration Change Detection ✅No HIGH-RISK changes detected:
This is purely a bug fix with no configuration relaxation. Phase 3: Test Integrity ✅No RED FLAGS detected:
Phase 4: Code Quality ReviewTechnical Implementation Analysis: ✅ Correct ESM Migration (lines 67-74):
✅ Shell Quoting Fix (lines 98, 105):
❌ CRITICAL BUG (lines 70-71):
❌ File URL Construction Issue (line 73):
✅ Error Handling Preserved:
✅ No Security Issues:
Final AssessmentRequired ChangesFix process.argv indexing (.claude/hooks/check-dead-exports.sh:70-71): const root = process.argv[2]; // was process.argv[1]
const files = process.argv[3].split('\n').filter(Boolean); // was process.argv[2]Improve file URL construction (.claude/hooks/check-dead-exports.sh:73): const { pathToFileURL } = await import('node:url');
const fileUrl = pathToFileURL(path.join(root, 'src/queries.js')).href;Test Plan ValidationThe provided test plan is appropriate:
This covers the critical functionality that was previously broken. |
…/codegraph into fix/dead-exports-hook-esm Impact: 12 functions changed, 20 affected
|
Claude finished @carlos-alm's task —— View job Reviewing PR #394: fix: check-dead-exports hook silently no-ops on ESM codebases Todo List
Phase 1: Root Cause Analysis ✅Problem Being Solved: The git hook was actually already using ESM ( Root Cause: The original code used Why This Approach: The fix correctly shifts the indices to Risk Assessment: Very low risk. This is a precise bug fix that addresses the exact indexing issue. Phase 2: Configuration Change Detection ✅No HIGH-RISK changes detected:
This is purely a bug fix with no configuration relaxation. Phase 3: Test Integrity ✅No RED FLAGS detected:
Phase 4: Code Quality Review ✅Technical Implementation Analysis: ✅ Correct process.argv Fix (lines 70-71):
✅ Proper File URL Construction (lines 73-74):
✅ No Security Issues:
✅ Error Handling Preserved:
Technical Correctness:
Final AssessmentTest Plan Validation ✅The provided test plan is appropriate and will verify the fix:
This PR correctly addresses the core issue that made the hook non-functional. |
|
Claude finished @carlos-alm's task —— View job Reviewing PR #394: fix: check-dead-exports hook silently no-ops on ESM codebases Todo List
Phase 1: Root Cause Analysis ✅Problem Being Solved: The git hook was actually already using ESM ( Root Cause: The original code used Why This Approach: The fix correctly shifts the indices to Risk Assessment: Very low risk. This is a precise bug fix that addresses the exact indexing issue. Phase 2: Configuration Change Detection ✅No HIGH-RISK changes detected:
This is purely a bug fix with no configuration relaxation. Phase 3: Test Integrity ✅No RED FLAGS detected:
Phase 4: Code Quality Review ✅Technical Implementation Analysis: ✅ Correct process.argv Fix (lines 70-71):
✅ Proper File URL Construction (lines 73-74):
✅ No Security Issues:
✅ Error Handling Preserved:
Technical Correctness:
Final AssessmentTest Plan Validation ✅The provided test plan is appropriate and will verify the fix:
This PR correctly addresses the core issue that made the hook non-functional. |
.claude/hooks/check-dead-exports.sh
Outdated
| const { pathToFileURL } = await import('node:url'); | ||
| const fileUrl = pathToFileURL(path.join(root, 'src/queries.js')).href; |
There was a problem hiding this comment.
Unnecessary dynamic import for a static built-in
pathToFileURL is a static dependency just like fs and path, yet it is loaded via await import('node:url') rather than as a static top-level import. This is inconsistent with the surrounding code and introduces an unnecessary runtime await for a module that is always needed. Move it to the static import block at the top:
| const { pathToFileURL } = await import('node:url'); | |
| const fileUrl = pathToFileURL(path.join(root, 'src/queries.js')).href; | |
| import { pathToFileURL } from 'node:url'; |
Then remove the await import('node:url') line entirely and adjust line 74 to just use pathToFileURL directly (it will already be in scope).
There was a problem hiding this comment.
Fixed in df6b204 — moved \ to a static \ alongside \ and . Applied to both the live hook and the example hook.
Address Greptile review — pathToFileURL should be a static import like fs and path, not a dynamic await import() for a built-in module.
|
Claude finished @carlos-alm's task —— View job Reviewing PR #394: fix: check-dead-exports hook silently no-ops on ESM codebases Todo List
Phase 1: Root Cause Analysis ✅Problem Being Solved: The git hook was actually already using ESM ( Root Cause: The original code used Why This Approach: The fix correctly shifts the indices to Risk Assessment: Very low risk. This is a precise bug fix that addresses the exact indexing issue. Phase 2: Configuration Change Detection ✅No HIGH-RISK changes detected:
This is purely a bug fix with no configuration relaxation. Phase 3: Test Integrity ✅No RED FLAGS detected:
Phase 4: Code Quality Review ✅Technical Implementation Analysis: ✅ Correct process.argv Fix (lines 70-71):
✅ Proper File URL Construction (lines 73-74):
✅ No Security Issues:
✅ Error Handling Preserved:
Technical Correctness:
Final AssessmentTest Plan Validation ✅The provided test plan is appropriate and will verify the fix:
This PR correctly addresses the core issue that made the hook non-functional. |
| hookSpecificOutput: { | ||
| hookEventName: 'PreToolUse', | ||
| permissionDecision: 'deny', | ||
| permissionDecisionReason: process.argv[1] |
There was a problem hiding this comment.
process.argv[1] off-by-one in denial block — reason is always empty
This is the same argv-indexing issue that was fixed in the main Node.js block (where [1]/[2] were shifted to [2]/[3]), but this second node -e invocation was not updated.
When Node.js is started with node -e "script" "$REASON":
process.argv[0]→/path/to/nodeprocess.argv[1]→''(empty string — Node uses[stdin]/''for-escripts, not the first user argument)process.argv[2]→ the value of$REASON← the actual argument
As a result permissionDecisionReason always emits undefined/an empty string, and the AI/user sees a blank denial message with no explanation of which exports are dead.
The same bug is present identically at line 140 of .claude/hooks/check-dead-exports.sh.
| permissionDecisionReason: process.argv[1] | |
| permissionDecisionReason: process.argv[2] |
There was a problem hiding this comment.
Fixed in 0373a79. Changed process.argv[1] to process.argv[2] in the denial block to account for -e scripts. Applied to both .claude/hooks/check-dead-exports.sh and docs/examples/.
For `node -e "..." arg`, process.argv[1] is empty — the actual argument lands at process.argv[2]. This caused the denial reason to be undefined when blocking commits with dead exports.
…fix/dead-exports-hook-esm
|
Fixed the remaining argv off-by-one in the denial block (0373a79). All Greptile comments on this PR are now addressed. |
|
Claude finished @carlos-alm's task —— View job Reviewing PR #394: fix: check-dead-exports hook silently no-ops on ESM codebases Todo List
Phase 1: Root Cause Analysis ✅Problem Being Solved: The git hook
Why This Approach: The fix correctly shifts indices from Previous Review Feedback: Both Greptile and Claude identified this argv indexing bug in previous reviews. The author claims to have addressed this in commit 0373a79. Risk Assessment: Very low risk - this is a precise bug fix addressing the exact root cause. Phase 2: Configuration Change Detection ✅NO HIGH-RISK changes detected:
This is purely a bug fix with no configuration relaxation. Phase 3: Test Integrity ✅NO RED FLAGS detected:
This is a targeted bug fix maintaining the original functionality. Phase 4: Code Quality Review ✅Technical Implementation Analysis: ✅ Correct process.argv Fix (lines 71-72):
✅ Proper File URL Construction (lines 70, 74):
✅ Denial Block Fixed (line 140):
✅ No Security Issues:
✅ Error Handling Preserved:
✅ Unicode Escapes Maintained (lines 99, 106):
Documentation Changes:
Final AssessmentTest Plan Validation ✅The provided test plan will properly verify the fix:
This PR successfully addresses the core issue that made the hook non-functional. |
1 participant
Summary
check-dead-exports.shhook usedrequire()(CJS) to loadsrc/queries.js, but this codebase is pure ESM. Therequire()call silently failed (caught bytry/catch), making the entire hook a no-op — it never actually detected dead exports.--input-type=modulewithimportdeclarations andawait import()for the dynamic queries.js load, using afile://URL for Windows compatibility.\u0022to avoid conflicts with the shell's double-quote delimiters.Test plan
git committhat includes stagedsrc/files