fix(pre-tool-use): don't intercept incidental memory-path mentions#251
Conversation
Tighten the Bash memory check so a path that only appears as inert text (echo/printf/claude prompt) passes through, while real interactions (file builtins, interpreters, redirects, substitutions) still route/guide. Closes activeloopai#87
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughReplaces substring memory-path checks with bash-aware tokenization and detection, wires ChangesBash memory path detection refinement
CI workflow condition tweak
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI command string
participant Parser as parseBashTokens
participant Detector as bashTouchesMemory
participant Hook as preToolUseHook
CLI->>Parser: raw bash command
Parser->>Detector: stages and tokens (heredoc bodies stripped)
Detector->>Hook: boolean: touchesMemory?
Hook->>Hook: decide rewrite / guidance / passthrough
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/claude-code/pre-tool-use.test.ts (1)
503-519: 💤 Low valueConsider strengthening boundary test assertions.
The boundary tests correctly verify that genuine memory interactions are intercepted (
empty === false), but they don't assert on the decision type or content. For consistency with other interception tests in this file (e.g., lines 148-152 which checkr.decisionandr.updatedCommand), consider adding assertions on the expected decision shape.Example strengthening
it("still intercepts `echo` redirecting INTO memory (documented write path)", () => { const r = runPreToolUse("Bash", { command: "echo 'hi' > ~/.deeplake/memory/note.md" }); expect(r.empty).toBe(false); + if (!r.empty) { + expect(r.decision).toBe("allow"); + expect(r.updatedCommand).toContain("RETRY REQUIRED"); + } });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/claude-code/pre-tool-use.test.ts` around lines 503 - 519, The three boundary tests using runPreToolUse ("still intercepts `echo` redirecting INTO memory (documented write path)", "still intercepts `echo` with a substitution touching memory", "intercepts a quoted reader path") only assert r.empty but should also assert the interception decision and updated command like other tests do; update each test to check r.decision (e.g., equals "intercept" or matches the existing decision shape used in earlier tests) and r.updatedCommand (or the expected sanitized/redirected command string) to ensure the decision shape and content are verified alongside r.empty, referencing runPreToolUse, r.decision, and r.updatedCommand to locate the change points.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/claude-code/pre-tool-use.test.ts`:
- Around line 503-519: The three boundary tests using runPreToolUse ("still
intercepts `echo` redirecting INTO memory (documented write path)", "still
intercepts `echo` with a substitution touching memory", "intercepts a quoted
reader path") only assert r.empty but should also assert the interception
decision and updated command like other tests do; update each test to check
r.decision (e.g., equals "intercept" or matches the existing decision shape used
in earlier tests) and r.updatedCommand (or the expected sanitized/redirected
command string) to ensure the decision shape and content are verified alongside
r.empty, referencing runPreToolUse, r.decision, and r.updatedCommand to locate
the change points.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3f5f08c1-2a08-4c2f-89f0-6d7433008af1
📒 Files selected for processing (3)
src/hooks/memory-path-utils.tssrc/hooks/pre-tool-use.tstests/claude-code/pre-tool-use.test.ts
The echo/printf/claude passthrough let three genuinely-interacting shapes reach the host shell: - Process substitution: the stage-level guard checked $( and backticks but not <(. The PR notes said isSafe() rejects <( upstream, but isSafe only runs AFTER bashTouchesMemory returns true. Moved the substitution guard to the top of bashTouchesMemory on the raw command, covering <(. - Input redirection: < was not tokenized, so feeding a memory file into `claude -p` via `< <memory-path>` passed through. < is now its own token (<</<<< lumped and ignored) and a redirect target check runs alongside >/>>. - Quote parsing: backslash was treated as an escape inside single quotes, where bash treats it as literal — `echo 'a\' ; cat <memory-path>` swallowed the cat stage into the echo passthrough. Escape handling now skips inside single quotes. All three cases added as regression tests; boundary tests also assert decision shape per review feedback. 4395 tests green.
|
Thanks for this — the approach is right: a narrow carve-out on top of broad detection is the correct call, and your justification for rejecting the "unrecognized command → pass through" idea from #87 is spot on. While reviewing, we probed the carve-out empirically and found three shapes that the old substring check intercepted but the new code passed through to the host shell. Rather than going back and forth, we pushed a follow-up commit (b19fb20) directly to this branch (maintainer edits are enabled) closing them:
All three are added as regression tests, the boundary tests now also assert the decision shape (per the CodeRabbit note), and the full suite is green (4395 tests). With these in, this looks good to us — thanks again for the contribution and the careful writeup. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/claude-code/pre-tool-use.test.ts`:
- Around line 508-511: The test uses a loose substring assertion on
r.updatedCommand (expect(r.updatedCommand).toContain("RETRY REQUIRED")) which
can mask regressions; update the assertions for the cases where !r.empty and
r.decision === "allow" to assert a stricter shape — either compare
r.updatedCommand to a stable expected constant (exact string) or use an anchored
regex to validate the exact guidance format (e.g., start/end or full message
structure) and keep the decision check as expect(r.decision).toBe("allow");
change the four occurrences referencing r.updatedCommand so they assert the
exact/anchored message instead of toContain.
- Around line 541-547: The test only checks r.empty and needs explicit
assertions about the hook decision and guidance; update this test (and the two
sibling cases) to assert the expected decision shape returned by runPreToolUse
(e.g., that r.decision === 'allow' and that the guidance/retry fields contain
the expected retry semantics or message text), and also assert r.empty is false
as before so regressions that change decision mode or guidance are caught; look
for the runPreToolUse call and the test blocks named "still intercepts..." to
add these assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 4549cfe0-3ac6-43dc-9e54-70445534ca83
📒 Files selected for processing (2)
src/hooks/memory-path-utils.tstests/claude-code/pre-tool-use.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/hooks/memory-path-utils.ts
| if (!r.empty) { | ||
| expect(r.decision).toBe("allow"); | ||
| expect(r.updatedCommand).toContain("RETRY REQUIRED"); | ||
| } |
There was a problem hiding this comment.
Strengthen guidance assertions to specific shapes instead of substring-only checks.
On Line 510, Line 519, Line 528, and Line 537, .toContain("RETRY REQUIRED") is broad and can mask wording/shape regressions. Prefer asserting a stricter structure (e.g., decision === "allow" + exact/anchored guidance format or a stable expected message constant).
As per coding guidelines, "tests/**: Prefer asserting on specific values (paths, messages) over generic substrings."
Also applies to: 517-520, 526-529, 535-538
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/claude-code/pre-tool-use.test.ts` around lines 508 - 511, The test uses
a loose substring assertion on r.updatedCommand
(expect(r.updatedCommand).toContain("RETRY REQUIRED")) which can mask
regressions; update the assertions for the cases where !r.empty and r.decision
=== "allow" to assert a stricter shape — either compare r.updatedCommand to a
stable expected constant (exact string) or use an anchored regex to validate the
exact guidance format (e.g., start/end or full message structure) and keep the
decision check as expect(r.decision).toBe("allow"); change the four occurrences
referencing r.updatedCommand so they assert the exact/anchored message instead
of toContain.
Source: Coding guidelines
The new helpers were only exercised through the bundled hook in a subprocess, which v8 coverage cannot see, so the per-file 90% gate failed (82.67% statements / 81.01% branches). Direct imports of the real module bring the file to 99.21% statements / 97.46% branches / 100% functions and lines.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/shared/memory-path-utils.test.ts (1)
126-128: 💤 Low valueConsider clarifying the test description.
The test correctly validates that string-based detection intercepts path traversal attempts (the path contains the memory substring even though it would resolve elsewhere). The description "on a memory path" could be more precise, e.g., "intercepts an interpreter with memory-path prefix even when traversing elsewhere" to make the security intent clearer.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/shared/memory-path-utils.test.ts` around lines 126 - 128, Update the test description to more clearly express the security intent: change the it() title for the case using bashTouchesMemory(`python3 ${MEM}/../../etc/passwd`) to something like "intercepts an interpreter with memory-path prefix even when traversal targets elsewhere" so it references the memory-path prefix and path traversal; locate the test using the bashTouchesMemory function and MEM constant in tests/shared/memory-path-utils.test.ts and replace the existing description string accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/shared/memory-path-utils.test.ts`:
- Around line 126-128: Update the test description to more clearly express the
security intent: change the it() title for the case using
bashTouchesMemory(`python3 ${MEM}/../../etc/passwd`) to something like
"intercepts an interpreter with memory-path prefix even when traversal targets
elsewhere" so it references the memory-path prefix and path traversal; locate
the test using the bashTouchesMemory function and MEM constant in
tests/shared/memory-path-utils.test.ts and replace the existing description
string accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 21bf65d5-2b22-40a8-8956-50b65a519915
📒 Files selected for processing (1)
tests/shared/memory-path-utils.test.ts
Fork PRs run with a read-only GITHUB_TOKEN, so the sticky-comment action fails with 'Resource not accessible by integration', marking the job red and skipping the bundle build/smoke steps after it. The coverage gate itself runs (and enforces) earlier regardless.
… name Addresses CodeRabbit review notes: the three new boundary tests now assert decision and retry-guidance content instead of only non-empty output, and the interpreter traversal test name states the security intent.
efenocchi
force-pushed
the
fix/pre-tool-use-incidental-memory-mentions
branch
from
cc37426 to
abbae04
Compare
2 participants
Closes #87
Problem
The Bash branch ran
touchesMemory(cmd)as a substring/regex test, so thememory path triggered interception even when it only appeared as inert text
in a prompt — e.g.
claude -p 'use ~/.deeplake/memory/'was rewritten to the[RETRY REQUIRED]echo instead of running.Fix
Added
bashTouchesMemory()(inmemory-path-utils.ts), which tokenizes thecommand (
parseBashTokens) and applies a narrow carve-out on top of broaddetection:
keeps file builtins routed and interpreters/fetchers/substitutions on the
retry-guidance path (preserves the host-access + path-traversal protections
the existing tests pin).
echo/printf/claudestages where the path is inert text passthrough — UNLESS it's a redirection target (a write, the documented
echo '<content>' > '<path>'path) or smuggles a$()/backticksubstitution, which fall through to be caught.
Swapped
touchesMemory→bashTouchesMemoryin the two Bash-command gates inpre-tool-use.ts; bare-path detection for Read/Grep/Glob/Write/Edit isunchanged.
Acceptance (#87)
cat ~/.deeplake/memory/index.md— still interceptedclaude -p 'use ~/.deeplake/memory/'— passes throughecho '~/.deeplake/memory/'— passes throughgrep foo ~/.deeplake/memory/summaries/— still interceptedWhy not the argv/command-name approach from the issue
Command-name alone can't satisfy the acceptance: it can't pass
echo '<path>'while keeping
rm/python3on a bare memory path routed/guided, since thoseare non-reader commands too. Worse, "unrecognized → pass through" would let
python3 ~/.deeplake/memory/../../../etc/passwdreach the host shell. Thecarve-out approach passes all four cases and keeps the existing security tests
green.
Tests
Added a
#87block covering the two passthrough cases plus boundaries (writeredirect,
$()substitution, quoted reader still intercept). Full existingsuite stays green.
Out of scope
parseBashTokenshandles; | || && \n, quotes, and>/>>; it does notunescape backslashes, fully model
2>/&>fd-redirects, or parse<((isSafe() rejects
<(,$(), backticks upstream).Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores