fix: segment-aware snip detection + quiet mode + shell keywords (closes #15, #16)#19
fix: segment-aware snip detection + quiet mode + shell keywords (closes #15, #16)#19EnRaiha wants to merge 3 commits into
Conversation
VincentHardouin#15, VincentHardouin#16) - Replace startsWith("snip ") with segment-aware alreadyWrapped() that scans compound commands (&&, ||, ;, &) for existing snip wrapper - Add SNIP_QUIET_NO_FILTER=1 env var prefix to eliminate "no filter" noise - Add shell loop/conditional keywords to UNPROXYABLE_COMMANDS
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Updates the Bash pre-execution hook to wrap eligible commands with snip more safely, while avoiding wrapping shell built-ins/control structures and running snip in a quiet/no-filter mode.
Changes:
- Prefix wrapped commands with
SNIP_QUIET_NO_FILTER=1 snip ...instead ofsnip ... - Expand the “do not wrap” command set to include shell keywords/control-flow tokens
- Add
alreadyWrapped()guard to skip rewriting commands that appear to already includesnip
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/index.ts | Alters bash command rewriting: new env prefix, broader unproxyable set, and new “already wrapped” detection |
| src/index.test.ts | Updates expectations to match the new SNIP_QUIET_NO_FILTER=1 prefix behavior |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| function alreadyWrapped(command: string): boolean { | ||
| const segments = command.split(OPERATOR_RE); | ||
| return segments.some((s) => { | ||
| const trimmed = s.trimStart(); | ||
| return ( | ||
| trimmed.startsWith("snip ") || | ||
| trimmed.replace(ENV_VAR_RE, "").startsWith("snip ") | ||
| ); | ||
| }); | ||
| } |
| const command = output.args.command; | ||
| if (!command || typeof command !== "string") return; | ||
| if (alreadyWrapped(command)) return; |
| if (findFirstPipe(command) !== -1) { | ||
| const pipeIdx = findFirstPipe(command) | ||
| const firstCmd = command.slice(0, pipeIdx).trimEnd() | ||
| const rest = command.slice(pipeIdx) | ||
| output.args.command = snipCommand(firstCmd) + ' ' + rest | ||
| return | ||
| const pipeIdx = findFirstPipe(command); | ||
| const firstCmd = command.slice(0, pipeIdx).trimEnd(); | ||
| const rest = command.slice(pipeIdx); | ||
| output.args.command = snipCommand(firstCmd) + " " + rest; | ||
| return; | ||
| } |
| if (command.startsWith("snip ")) return | ||
| const command = output.args.command; | ||
| if (!command || typeof command !== "string") return; | ||
| if (alreadyWrapped(command)) return; |
Greptile SummaryThis PR overhauls the snip-wrapping logic with three improvements: segment-aware already-wrapped detection (
|
| Filename | Overview |
|---|---|
| src/index.ts | Introduces alreadyWrapped() for segment-aware snowball prevention, prepends SNIP_QUIET_NO_FILTER=1 to all wrapped commands, and extends UNPROXYABLE_COMMANDS with shell keywords and snip itself as defense-in-depth. |
| src/index.test.ts | Updates all expected strings to the new SNIP_QUIET_NO_FILTER=1 snip format, adds a dedicated already-wrapped detection suite, and adds a one-liner loop test covering the new do keyword guard. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[toolExecuteBefore] --> B{tool === 'bash'?}
B -- No --> Z[return, no change]
B -- Yes --> C{alreadyWrapped?}
C -- Yes --> Z
C -- No --> D{findFirstPipe != -1?}
D -- Yes --> E[snipCommand firstCmd + pipe rest unchanged]
E --> Z
D -- No --> F[split by OPERATOR_RE]
F --> G{single segment?}
G -- Yes --> H[snipCommand whole command]
H --> Z
G -- No --> I[map segments]
I --> J{segment matches OPERATOR_RE?}
J -- Yes --> K[keep operator as-is]
J -- No --> L[snipCommand segment]
K & L --> M[join all segments]
M --> Z
Reviews (2): Last reviewed commit: "fix: add missing do keyword + tests for ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| function alreadyWrapped(command: string): boolean { | ||
| const segments = command.split(OPERATOR_RE); | ||
| return segments.some((s) => { | ||
| const trimmed = s.trimStart(); | ||
| return ( | ||
| trimmed.startsWith("snip ") || | ||
| trimmed.replace(ENV_VAR_RE, "").startsWith("snip ") | ||
| ); | ||
| }); | ||
| } |
There was a problem hiding this comment.
alreadyWrapped short-circuits on any wrapped segment, silently skipping others
segments.some() returns true the moment a single segment is found to contain a snip prefix, which bails out for the whole command. If a command arrives with only its first segment wrapped — e.g., SNIP_QUIET_NO_FILTER=1 snip cmd1 && cmd2 — alreadyWrapped returns true and cmd2 is never monitored. This is an intentional snowball-prevention trade-off, but it means partially-wrapped commands silently pass through unmonitored.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| const bareCmd = command.slice(envPrefix.length).trim(); | ||
| if (!bareCmd) return command; | ||
| if (UNPROXYABLE_COMMANDS.has(bareCmd.split(/\s+/)[0])) return command; | ||
| return `${envPrefix}SNIP_QUIET_NO_FILTER=1 snip ${bareCmd}`; |
There was a problem hiding this comment.
Thanks for the contribution, but I don't think it's a good idea to make that choice for everyone. I'd rather everyone be able to choose for themselves. Here, we're overriding the behavior for everyone.
|
Thanks for the feedback! You're right that forcing quiet mode on everyone isn't ideal. Here's my revised thinking:
Want me to update the PR with just (1) shell keywords fix + (3) quiet mode as opt-in via env var? Or would you prefer them as separate PRs? |
I think the smaller the PRs are, the easier they are to review. Your PR is trying to fix something that has already been addressed by snip via snip check and snip run (1) and PRs #13 and [#14] are trying to implement this in the code, but I think it's too complex to be merged as-is because it solves too many things. In my opinion, this plugin should do as little as possible. That is to say:
|
|
Fair point. You're right that Thanks for the detailed review — appreciate the explanation. |
Fixes #15 (snowball) + #16 (quiet mode) + shell keyword protection.
Prevention over Reaction
PR #17 deduplicates
snip snip snipafter it happens. This PR prevents it entirely with segment-aware detection.Changes
Fix #15 — Snowball bug
alreadyWrapped()scans compound command segments (&&, ||, ;, &) for existingsnipprefixFOO=bar snip cmddetected as wrappedstartsWith("snip ")single-segment checkFix #16 — Quiet mode
SNIP_QUIET_NO_FILTER=1to all wrapped commandsBonus: Shell keywords
for,while,if,case,done, etc. added toUNPROXYABLE_COMMANDSunproxyableReason)Test
30/30 pass | TypeScript typecheck ok | DeepSeek Pro reviewed