fix(hooks): allow tmux-wrapped dev server commands#321
fix(hooks): allow tmux-wrapped dev server commands#321affaan-m merged 2 commits intoaffaan-m:mainfrom
Conversation
|
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:
📝 WalkthroughWalkthroughThis change extracts a shared Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fa2785567e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
hooks/hooks.json
Outdated
| { | ||
| "type": "command", | ||
| "command": "node -e \"let d='';process.stdin.on('data',c=>d+=c);process.stdin.on('end',()=>{try{const i=JSON.parse(d);const cmd=i.tool_input?.command||'';if(process.platform!=='win32'&&/(npm run dev\\b|pnpm( run)? dev\\b|yarn dev\\b|bun run dev\\b)/.test(cmd)){console.error('[Hook] BLOCKED: Dev server must run in tmux for log access');console.error('[Hook] Use: tmux new-session -d -s dev \\\"npm run dev\\\"');console.error('[Hook] Then: tmux attach -t dev');process.exit(2)}}catch{}console.log(d)})\"" | ||
| "command": "node -e \"let d='';process.stdin.on('data',c=>d+=c);process.stdin.on('end',()=>{try{const i=JSON.parse(d);const cmd=i.tool_input?.command||'';if(process.platform!=='win32'&&!/^\\s*tmux\\b/.test(cmd)&&/(npm run dev\\b|pnpm( run)? dev\\b|yarn dev\\b|bun run dev\\b)/.test(cmd)){console.error('[Hook] BLOCKED: Dev server must run in tmux for log access');console.error('[Hook] Use: tmux new-session -d -s dev \\\"npm run dev\\\"');console.error('[Hook] Then: tmux attach -t dev');process.exit(2)}}catch{}console.log(d)})\"" |
There was a problem hiding this comment.
The new !/^\s*tmux\b/.test(cmd) guard exempts every command string that merely starts with tmux, so a command like tmux ls && npm run dev now passes even though the dev server still runs outside tmux. This weakens the hook’s stated enforcement (“Dev server must run in tmux for log access”) and introduces an easy false negative compared with the previous behavior.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hooks/hooks.json`:
- Line 10: The current prefix check in the "command" string (the long node -e
snippet in hooks.json) treats any command that starts with "tmux" as a safe
bypass and thus allows cases like "tmux ls && npm run dev"; change the check to
only allow tmux when it is used as a launcher that actually contains the dev
payload (e.g., tmux new, new-session, new-window, split-window variants that
include the dev command). Locate the "command" value in hooks.json and replace
the loose /^\\s*tmux\\b/ test with a stricter pattern that verifies both a
launcher verb (new, new-session, new-window, split-window) immediately after
tmux and that the rest of the tmux invocation includes one of the dev commands
(npm run dev | pnpm[ run]? dev | yarn dev | bun run dev), so only true
tmux-launched dev servers bypass the block.
There was a problem hiding this comment.
Pull request overview
Updates the Claude Code PreToolUse Bash hook to stop falsely blocking dev-server commands when they’re already wrapped by tmux, aligning the hook’s behavior with the intended “run dev server via tmux” workflow.
Changes:
- Add an early check to skip dev-server blocking when the command starts with
tmux. - Preserve existing dev-server blocking behavior for non-
tmuxcommands.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
hooks/hooks.json
Outdated
| { | ||
| "type": "command", | ||
| "command": "node -e \"let d='';process.stdin.on('data',c=>d+=c);process.stdin.on('end',()=>{try{const i=JSON.parse(d);const cmd=i.tool_input?.command||'';if(process.platform!=='win32'&&/(npm run dev\\b|pnpm( run)? dev\\b|yarn dev\\b|bun run dev\\b)/.test(cmd)){console.error('[Hook] BLOCKED: Dev server must run in tmux for log access');console.error('[Hook] Use: tmux new-session -d -s dev \\\"npm run dev\\\"');console.error('[Hook] Then: tmux attach -t dev');process.exit(2)}}catch{}console.log(d)})\"" | ||
| "command": "node -e \"let d='';process.stdin.on('data',c=>d+=c);process.stdin.on('end',()=>{try{const i=JSON.parse(d);const cmd=i.tool_input?.command||'';if(process.platform!=='win32'&&!/^\\s*tmux\\b/.test(cmd)&&/(npm run dev\\b|pnpm( run)? dev\\b|yarn dev\\b|bun run dev\\b)/.test(cmd)){console.error('[Hook] BLOCKED: Dev server must run in tmux for log access');console.error('[Hook] Use: tmux new-session -d -s dev \\\"npm run dev\\\"');console.error('[Hook] Then: tmux attach -t dev');process.exit(2)}}catch{}console.log(d)})\"" |
There was a problem hiding this comment.
This tmux-prefix exemption is added in the Claude PreToolUse hook, but the Cursor hook implementation still blocks tmux-wrapped dev server commands (see .cursor/hooks/before-shell-execution.js, used by .cursor/hooks.json). If both hook systems are used in this repo, consider applying the same tmux-detection change there as well to keep behavior consistent and avoid the false-positive persisting in Cursor.
| "command": "node -e \"let d='';process.stdin.on('data',c=>d+=c);process.stdin.on('end',()=>{try{const i=JSON.parse(d);const cmd=i.tool_input?.command||'';if(process.platform!=='win32'&&!/^\\s*tmux\\b/.test(cmd)&&/(npm run dev\\b|pnpm( run)? dev\\b|yarn dev\\b|bun run dev\\b)/.test(cmd)){console.error('[Hook] BLOCKED: Dev server must run in tmux for log access');console.error('[Hook] Use: tmux new-session -d -s dev \\\"npm run dev\\\"');console.error('[Hook] Then: tmux attach -t dev');process.exit(2)}}catch{}console.log(d)})\"" | |
| "command": "node -e \"let d='';process.stdin.on('data',c=>d+=c);process.stdin.on('end',()=>{try{const i=JSON.parse(d);const cmd=i.tool_input?.command||'';if(process.platform!=='win32'&&!process.env.TMUX&&!/^\\s*tmux\\b/.test(cmd)&&/(npm run dev\\b|pnpm( run)? dev\\b|yarn dev\\b|bun run dev\\b)/.test(cmd)){console.error('[Hook] BLOCKED: Dev server must run in tmux for log access');console.error('[Hook] Use: tmux new-session -d -s dev \\\"npm run dev\\\"');console.error('[Hook] Then: tmux attach -t dev');process.exit(2)}}catch{}console.log(d)})\"" |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="hooks/hooks.json">
<violation number="1" location="hooks/hooks.json:10">
P2: The tmux prefix check is too broad — any command starting with `tmux` bypasses the block, even when the dev server isn't actually wrapped in a tmux session (e.g., `tmux ls; npm run dev`). Consider matching only session-creation subcommands like `tmux new-session` or `tmux new` to ensure the dev server is genuinely running inside tmux.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".cursor/hooks/before-shell-execution.js">
<violation number="1" location=".cursor/hooks/before-shell-execution.js:10">
P2: Regex bypass: `.*` in `tmuxDevRe` matches across shell operators (`;`, `&&`, `||`), so a chained command like `tmux new-session -d 'echo'; npm run dev` is incorrectly allowed even though the dev server runs **outside** tmux.
Consider requiring the dev command to appear within a quoted argument to the tmux subcommand, e.g. by replacing the last `.*` with a pattern that expects a quote before the dev command:</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.cursor/hooks/before-shell-execution.js:
- Around line 10-11: The current check uses devRe and tmuxDevRe against the
whole cmd string so chained commands can bypass detection; update the logic in
before-shell-execution.js to split cmd into individual segments using a
separator regex (e.g. split on /\s*(?:&&|\|\||;)\s*/) and then for each segment
test if devRe.test(segment) and not tmuxDevRe.test(segment) — if any segment
matches that condition, treat it as a non-tmux dev invocation and block/handle
accordingly; refer to the existing tmuxDevRe, devRe, and cmd identifiers when
locating and replacing the single-string test with the per-segment loop.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.cursor/hooks/before-shell-execution.jshooks/hooks.json
🚧 Files skipped from review as they are similar to previous changes (1)
- hooks/hooks.json
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: checks are failing. Please fix failures before review.
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".cursor/hooks/before-shell-execution.js">
<violation number="1" location=".cursor/hooks/before-shell-execution.js:12">
P1: Naive splitting by `&&`/`||`/`;` doesn't respect shell quoting, causing a regression for common tmux patterns like `tmux new-session -d -s dev "cd /app && npm run dev"`. The `&&` inside the quoted tmux argument is treated as a shell operator, producing a bare `npm run dev"` segment that gets incorrectly blocked.
Add a full-command `tmuxDevRe` check as an escape hatch so that quoted compound inner commands are still recognized as tmux-wrapped:</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: checks are failing. Please fix failures before review.
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="hooks/hooks.json">
<violation number="1" location="hooks/hooks.json:10">
P2: The new quote-aware splitter mis-parses escaped quotes, which can wrongly block valid tmux-wrapped dev commands (false positive). Handle escaped quotes before toggling quote state.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.cursor/hooks/before-shell-execution.js:
- Around line 18-20: The parser only treats '&&' and '||' as segment separators
and misses single '&', allowing commands like '... & npm run dev' to be one
segment; update the splitting logic around variables s, i, c, sq, dq, segs, cur
so that a lone '&' (when not part of '&&') is treated as a separator too, and
adjust the increment logic so you only skip the next character when you matched
a two-character operator ('&&' or '||') rather than for single '&'; ensure the
segment push remains segs.push(cur.trim()); cur = '' and that you continue
parsing afterward.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.cursor/hooks/before-shell-execution.jshooks/hooks.jsonscripts/hooks/doc-file-warning.jsscripts/lib/project-detect.js
There was a problem hiding this comment.
🧹 Nitpick comments (1)
hooks/hooks.json (1)
10-10: Consider centralizing this parser/gating logic in one shared script.The same splitter/regex behavior now exists here and in
.cursor/hooks/before-shell-execution.js; reusing one implementation would reduce drift risk.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/hooks.json` at line 10, The inline node -e hook contains duplicated parsing/gating logic (the sq splitter function and regexes devRe and tmuxDevRe); extract that logic into a single reusable module that exports the splitter (sq) and a gating function (e.g., shouldBlockDevCommand(command):boolean) and replace the inline script in the "command" string (and the duplicate in .cursor/hooks/before-shell-execution.js) to require/execute the shared module so both places call the same implementation (use the same exported names like sq, devRe, tmuxDevRe or a single shouldBlockDevCommand helper to locate and reuse the exact behavior).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@hooks/hooks.json`:
- Line 10: The inline node -e hook contains duplicated parsing/gating logic (the
sq splitter function and regexes devRe and tmuxDevRe); extract that logic into a
single reusable module that exports the splitter (sq) and a gating function
(e.g., shouldBlockDevCommand(command):boolean) and replace the inline script in
the "command" string (and the duplicate in
.cursor/hooks/before-shell-execution.js) to require/execute the shared module so
both places call the same implementation (use the same exported names like sq,
devRe, tmuxDevRe or a single shouldBlockDevCommand helper to locate and reuse
the exact behavior).
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="hooks/hooks.json">
<violation number="1" location="hooks/hooks.json:10">
P2: The new single-`&` split rule incorrectly treats redirection operators (`2>&1`, `&>`, `|&`) as command separators, so command segmentation is wrong for valid shell commands.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
zzzhizhia
force-pushed
the
fix/tmux-dev-hook
branch
from
026af41 to
437e0b3
Compare
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
|
Hey @zzzhizhia! This PR has a merge conflict with main. Could you please rebase when you get a chance? Thanks! |
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
…d module - Fix single & incorrectly splitting redirection operators (&>, >&, 2>&1) - Fix escaped quotes (\", \') not being handled inside quoted strings - Extract splitShellSegments into shared scripts/lib/shell-split.js to eliminate duplication between hooks.json, before-shell-execution.js, and pre-bash-dev-server-block.js - Add comprehensive tests for shell splitting edge cases
zzzhizhia
force-pushed
the
fix/tmux-dev-hook
branch
from
437e0b3 to
d1238e0
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.cursor/hooks/before-shell-execution.js:
- Around line 35-41: The current catch blocks in before-shell-execution.js (the
inner catch after parsing/stdin and the final .catch(() => process.exit(0)))
silently swallow errors and exit 0; change them to fail fast and emit clear
errors instead: when the parser/stdin handling fails (the inner catch that
currently does noop) log a descriptive error to stderr (including the exception
message) and exit with a non-zero code, and replace the final .catch handler to
print a clear error message and exit non-zero rather than success. Target the
anonymous catch blocks surrounding the parse/stdin logic and the terminal
promise .catch and ensure they return a non-zero process.exit and include the
error details.
In `@scripts/lib/shell-split.js`:
- Around line 16-33: The splitter treats backslash-escaped operator characters
only when inside quotes, so sequences like \&& or \; outside quotes are still
parsed as separators; update shell-split logic to handle backslash escapes
outside quotes by adding the same escape-handling branch used inside the quoted
block (the if (ch === '\\' && i + 1 < command.length) { current += ch +
command[i + 1]; i++; continue; } pattern) before the separator/operator checks
so escaped separators are appended to current instead of splitting; modify the
code around variables command, ch, i, current and quote handling to run that
escape branch when quote is null, and add a unit/integration test that asserts a
command like "tmux new-session -d bash -lc cd /app \\&\\& npm run dev" does not
split on the escaped && outside quotes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f2827942-408c-40c7-b048-c7d300637180
📒 Files selected for processing (4)
.cursor/hooks/before-shell-execution.jsscripts/hooks/pre-bash-dev-server-block.jsscripts/lib/shell-split.jstests/lib/shell-split.test.js
Escaped operators like \&& and \; outside quotes were still being treated as separators. Add escape handling for unquoted context.
Summary
tmux new-session -d -s dev '...'!/^\s*tmux\b/.test(cmd)check to skip blocking when the command starts withtmuxTest plan
Summary by cubic
Allow tmux-wrapped dev servers while still blocking direct runs. We split commands with a quote- and escape-aware parser and exempt tmux launcher subcommands.
Written for commit ea3f66d. Summary will update on new commits.
Summary by CodeRabbit
Refactor
Tests