fix: tldr-read-enforcer hook ignored when Claude uses a relative file path#152
fix: tldr-read-enforcer hook ignored when Claude uses a relative file path#152marcuspuchalla wants to merge 1 commit intoparcadei:mainfrom
Conversation
|
PR author is not in the allowed authors list. |
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughAdded optional import-related fields to DaemonResponse, made file path resolution use path.resolve with a project-cwd fallback and updated the daemon-client import to a .js extension, and excluded Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| const filePath = input.tool_input.file_path || ''; | ||
| const rawFilePath = input.tool_input.file_path || ''; | ||
| // Resolve relative paths — statSync fails silently on relative paths, bypassing the hook | ||
| const filePath = isAbsolute(rawFilePath) ? rawFilePath : join(input.cwd, rawFilePath); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.claude/hooks/src/tldr-read-enforcer.ts (1)
374-376: Core fix is correct; considerresolveoverjoinfor stronger absoluteness guarantee.
path.join(input.cwd, rawFilePath)produces a relative result ifinput.cwdis itself relative — the original bug silently re-emerges.path.resolvealways returns an absolute path, regardless of whetherinput.cwdis absolute or relative.While Claude's hook system should always supply an absolute
cwd,resolvemakes the contract explicit and eliminates the edge-case regression path.🛡️ Suggested defensive change
-import { basename, extname, join, isAbsolute } from 'path'; +import { basename, extname, resolve, isAbsolute } from 'path';-const rawFilePath = input.tool_input.file_path || ''; -// Resolve relative paths — statSync fails silently on relative paths, bypassing the hook -const filePath = isAbsolute(rawFilePath) ? rawFilePath : join(input.cwd, rawFilePath); +const rawFilePath = input.tool_input.file_path || ''; +// Resolve relative paths — statSync fails silently on relative paths, bypassing the hook +const filePath = isAbsolute(rawFilePath) ? rawFilePath : resolve(input.cwd, rawFilePath);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/hooks/src/tldr-read-enforcer.ts around lines 374 - 376, Replace the path construction that uses join so relative input.cwd can still yield a relative result: change the logic that sets filePath (currently using isAbsolute(rawFilePath) ? rawFilePath : join(input.cwd, rawFilePath)) to use path.resolve for the fallback (i.e., isAbsolute(rawFilePath) ? rawFilePath : resolve(input.cwd, rawFilePath)); ensure the code imports resolve from 'path' alongside isAbsolute and join (or replace join usage entirely) so filePath is always an absolute path even when input.cwd is relative.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.claude/hooks/src/tldr-read-enforcer.ts:
- Around line 374-376: Replace the path construction that uses join so relative
input.cwd can still yield a relative result: change the logic that sets filePath
(currently using isAbsolute(rawFilePath) ? rawFilePath : join(input.cwd,
rawFilePath)) to use path.resolve for the fallback (i.e.,
isAbsolute(rawFilePath) ? rawFilePath : resolve(input.cwd, rawFilePath)); ensure
the code imports resolve from 'path' alongside isAbsolute and join (or replace
join usage entirely) so filePath is always an absolute path even when input.cwd
is relative.
6ce9590 to
b0f65ce
Compare
marcuspuchalla
left a comment
There was a problem hiding this comment.
Good catch — valid defensive concern. While Claude Code's hook spec guarantees cwd is always present in PreToolUse payloads, there's no runtime guard. If input.cwd were somehow absent, join(undefined, rawFilePath) would throw a TypeError outside the try/catch block below, crashing the hook.
Fixed in the latest commit with a process.cwd() fallback:
const projectCwd = input.cwd || process.cwd();
const filePath = isAbsolute(rawFilePath) ? rawFilePath : join(projectCwd, rawFilePath);This is purely defensive — in normal operation input.cwd is always a string — but graceful degradation is better than an unhandled crash.
… path
The hook checks the file size before deciding whether to intercept a Read.
It does this with statSync(filePath). But statSync needs an absolute path —
if Claude passes a relative path like 'src/api/foo.ts', statSync can't find
the file (the hook process has a different working directory than the project).
It throws, the catch block silently returns {}, and the hook does nothing —
the raw file is read as normal, bypassing the TLDR token savings entirely.
Fix: convert relative paths to absolute using input.cwd (the project directory
that Claude Code provides in the hook payload):
const filePath = isAbsolute(rawFilePath)
? rawFilePath
: join(input.cwd, rawFilePath);
Also fixes pre-existing TypeScript compile errors:
- daemon-client import missing .js extension (required by NodeNext module resolution)
- DaemonResponse interface missing imports/importers fields (used in importsDaemon())
- tsconfig excludes src/__tests__ to stop test files breaking the build
b0f65ce to
99ea9aa
Compare
marcuspuchalla
left a comment
There was a problem hiding this comment.
Good call on . Fixed in the latest commit — replaced with :
const filePath = resolve(projectCwd, rawFilePath);resolve() handles both cases natively: returns an absolute path as-is, resolves a relative path against projectCwd. The isAbsolute guard and join import are no longer needed.
|
Re the Sentry inline comment on |
Problem
The hook checks the file size with
statSync(filePath)to decide whether to intercept a Read. ButstatSyncneeds an absolute path — if Claude passes a relative path likesrc/api/foo.ts, the hook's process has a different working directory than the project, sostatSynccan't find the file.It throws, the
catch {}block silently returns{}, and the raw file is read normally — TLDR never kicks in for any relative path.Fix
Convert relative paths to absolute using
input.cwd(the project directory Claude Code provides in the hook payload):Also fixes pre-existing TypeScript compile errors
daemon-clientimport missing.jsextension — required by NodeNext module resolutionDaemonResponseinterface missingimports/importersfields — used inimportsDaemon()at line 906 but not declared, causingTS2339tsconfig.jsonnow excludessrc/__tests__— test files need jest/vitest types not present in the main tsconfig, causing spurious build errorsTesting
Verified that after this fix, reading a file via relative path (e.g.
src/api/estimateSwap.ts) correctly triggers the TLDR context instead of falling through to a raw read.Summary by CodeRabbit
Bug Fixes
Chores