From 5deee34c93395045b985e3baf91550e5f1ab7204 Mon Sep 17 00:00:00 2001 From: Thejesh Reddy <35212698+thejesh23@users.noreply.github.com> Date: Mon, 20 Jul 2026 13:21:03 -0700 Subject: [PATCH] fix(hooks): remove stray '?' that made every 'yarn ' fire tmux reminder (#2517) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(hooks): remove stray '?' that made every 'yarn ' trigger tmux reminder The tmux-reminder matcher uses one alternation per package manager. Each branch requires a subcommand (install|test) — except yarn, whose subcommand group carried a trailing `?`: yarn (install|test)? That made the subcommand optional, so the branch degraded to "yarn " plus anything: `yarn add foo`, `yarn build`, `yarn dev`, even `yarn --version` all matched and spammed the "Consider running in tmux" hint into the additional-context channel. Drop the `?` so yarn matches parity with npm/pnpm/bun. Verified locally against 14 cases (yarn install/test still fire; yarn add/build/dev/… no longer do; npm/pnpm/bun/pytest behavior unchanged). Fixes #2514 * test(hooks): add pre-bash-tmux-reminder regression tests Add coverage for the tmux-reminder matcher following the auto-tmux-dev.test.js structure — the regex-first hook now has direct regression tests for the yarn branch fix in this PR (and for the sibling package managers, other matched tools, TMUX bypass, and malformed input). 16 assertions total: - fires for: yarn install, yarn test, npm install, pnpm test, bun install, pytest tests/, cargo build - does NOT fire for: yarn add react, yarn build, yarn dev, yarn --version, bare `yarn`, npm run dev - respects TMUX env var - tolerates invalid JSON and missing command field Verified the tests actually catch the bug: reintroducing the buggy `yarn (install|test)?` fails 4 of the 5 yarn non-match cases (the fifth, bare `yarn`, stays passing because even the buggy branch requires a trailing space after yarn). Addresses CodeRabbit review on #2517. * test(hooks): fail loudly on spawn errors, use destructuring, split runTests Address three CodeRabbit review notes on tests/hooks/pre-bash-tmux-reminder.test.js: - Fail loudly on spawnSync errors: raise instead of coercing `result.status || 0`, which would mask spawn errors, timeouts, or signal termination as a successful exit 0 (masks legitimate test failures). - Use destructuring (`const { TMUX, ...env } = process.env`) instead of copy-then-`delete` so the base env is built immutably. - Split `runTests` (was 66 lines) into small per-group helpers (runYarnTests, runSiblingPackageManagerTests, runOtherToolTests, runTmuxBypassTests, runEdgeCaseTests). `runTests` is now 18 lines and purely orchestrates. 16 assertions still pass; no coverage changes. The 4th CodeRabbit note (avoid console.log in test files) is intentionally not adopted here — every sibling hook test in this repo (auto-tmux-dev.test.js, bash-hook-dispatcher.test.js, block-no-verify.test.js, etc.) writes to console.log because the project's own test runner (tests/run-all.js) is console-log based and there is no Jest/Mocha dependency. Diverging from the established convention in a bugfix PR is out of scope. * test(hooks): trim tmux reminder regression coverage --------- Co-authored-by: Haley Chen <2022hachen@gmail.com> --- scripts/hooks/pre-bash-tmux-reminder.js | 2 +- tests/hooks/pre-bash-tmux-reminder.test.js | 78 ++++++++++++++++++++++ 2 files changed, 79 insertions(+), 1 deletion(-) create mode 100644 tests/hooks/pre-bash-tmux-reminder.test.js diff --git a/scripts/hooks/pre-bash-tmux-reminder.js b/scripts/hooks/pre-bash-tmux-reminder.js index 2ad56ea3..e00c16df 100755 --- a/scripts/hooks/pre-bash-tmux-reminder.js +++ b/scripts/hooks/pre-bash-tmux-reminder.js @@ -13,7 +13,7 @@ function run(rawInput) { if ( process.platform !== 'win32' && !process.env.TMUX && - /(npm (install|test)|pnpm (install|test)|yarn (install|test)?|bun (install|test)|cargo build|make\b|docker\b|pytest|vitest|playwright)/.test(cmd) + /(npm (install|test)|pnpm (install|test)|yarn (install|test)|bun (install|test)|cargo build|make\b|docker\b|pytest|vitest|playwright)/.test(cmd) ) { return { additionalContext: [ diff --git a/tests/hooks/pre-bash-tmux-reminder.test.js b/tests/hooks/pre-bash-tmux-reminder.test.js new file mode 100644 index 00000000..a441f17e --- /dev/null +++ b/tests/hooks/pre-bash-tmux-reminder.test.js @@ -0,0 +1,78 @@ +const assert = require('assert'); +const path = require('path'); +const { spawnSync } = require('child_process'); + +const script = path.join(__dirname, '..', '..', 'scripts', 'hooks', 'pre-bash-tmux-reminder.js'); + +function run(command, extraEnv = {}) { + const { TMUX: _tmux, ...envWithoutTmux } = process.env; + const result = spawnSync(process.execPath, [script], { + encoding: 'utf8', + input: JSON.stringify({ tool_input: { command } }), + timeout: 10000, + env: { ...envWithoutTmux, ...extraEnv } + }); + + if (result.error) throw result.error; + if (result.signal) throw new Error(`hook terminated by ${result.signal}`); + + assert.strictEqual(result.status, 0, `unexpected exit for ${command}: ${result.stderr || ''}`); + return result.stdout || ''; +} + +function hasReminder(command, extraEnv) { + return run(command, extraEnv).includes('Consider running in tmux'); +} + +function runTests() { + console.log('\n=== Testing pre-bash-tmux-reminder.js ===\n'); + + if (process.platform === 'win32') { + console.log(' SKIP: hook is a no-op on win32'); + return true; + } + + const cases = [ + ['fires for yarn install and yarn test', () => { + assert.ok(hasReminder('yarn install')); + assert.ok(hasReminder('yarn test')); + }], + ['does not fire for ordinary yarn commands', () => { + assert.ok(!hasReminder('yarn add react')); + assert.ok(!hasReminder('yarn build')); + assert.ok(!hasReminder('yarn dev')); + assert.ok(!hasReminder('yarn --version')); + assert.ok(!hasReminder('yarn')); + }], + ['keeps sibling package-manager behavior', () => { + assert.ok(hasReminder('npm install')); + assert.ok(hasReminder('pnpm test')); + assert.ok(hasReminder('bun install')); + assert.ok(!hasReminder('npm run dev')); + }], + ['suppresses reminders inside tmux', () => { + assert.ok(!hasReminder('yarn install', { TMUX: '/tmp/tmux-1000/default,1,0' })); + }] + ]; + + let failed = 0; + for (const [name, fn] of cases) { + try { + fn(); + console.log(` PASS ${name}`); + } catch (error) { + failed++; + console.log(` FAIL ${name}`); + console.log(` ${error.message}`); + } + } + + console.log(`\nResults: ${cases.length - failed} passed, ${failed} failed\n`); + return failed === 0; +} + +if (require.main === module) { + process.exit(runTests() ? 0 : 1); +} + +module.exports = { runTests };