fix: address PR review comments on block-no-verify hook

- Add `minimal` profile so the security hook runs in all profiles
- Scope -n/--no-verify flag check to the detected subcommand region,
  preventing false positives on chained commands (e.g. `git log -n 10`)
- Guard stdin listeners with `require.main === module` so require()
  from run-with-flags.js does not register unnecessary listeners
- Verify subcommand token is preceded only by flags/flag-args after
  "git", preventing misclassification of argument values as subcommands
- Add integration tests for block-no-verify hook

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

hooks/hooks.json

@@ -7,7 +7,7 @@
         "hooks": [
           {
             "type": "command",
-            "command": "node \"${CLAUDE_PLUGIN_ROOT}/scripts/hooks/run-with-flags.js\" \"pre:bash:block-no-verify\" \"scripts/hooks/block-no-verify.js\" \"standard,strict\""
+            "command": "node \"${CLAUDE_PLUGIN_ROOT}/scripts/hooks/run-with-flags.js\" \"pre:bash:block-no-verify\" \"scripts/hooks/block-no-verify.js\" \"minimal,standard,strict\""
           }
         ],
         "description": "Block git hook-bypass flag to protect pre-commit, commit-msg, and pre-push hooks from being skipped",

scripts/hooks/block-no-verify.js

@@ -76,6 +76,8 @@ function findGit(input, start) {
 
 /**
  * Detect which git subcommand (commit, push, etc.) is being invoked.
+ * Returns { command, offset } where offset is the position right after the
+ * subcommand keyword, so callers can scope flag checks to only that portion.
  */
 function detectGitCommand(input) {
   let start = 0;
@@ -88,18 +90,58 @@ function detectGitCommand(input) {
       continue;
     }
 
+    // Find the first matching subcommand token after "git".
+    // We pick the one closest to "git" so that argument values like
+    // "git push origin commit" don't misclassify "commit" as the subcommand.
+    let bestCmd = null;
+    let bestIdx = Infinity;
+
     for (const cmd of GIT_COMMANDS_WITH_NO_VERIFY) {
-      const cmdIdx = input.indexOf(cmd, git.idx + git.len);
-      if (cmdIdx === -1) continue;
+      let searchPos = git.idx + git.len;
+      while (searchPos < input.length) {
+        const cmdIdx = input.indexOf(cmd, searchPos);
+        if (cmdIdx === -1) break;
 
-      const before = cmdIdx > 0 ? input[cmdIdx - 1] : ' ';
-      const after = input[cmdIdx + cmd.length] || ' ';
-      if (!/\s/.test(before)) continue;
-      if (!/[\s;&#|>)\]}"']/.test(after) && after !== '') continue;
-      if (/[;|]/.test(input.slice(git.idx + git.len, cmdIdx))) continue;
-      if (isInComment(input, cmdIdx)) continue;
+        const before = cmdIdx > 0 ? input[cmdIdx - 1] : ' ';
+        const after = input[cmdIdx + cmd.length] || ' ';
+        if (!/\s/.test(before)) { searchPos = cmdIdx + 1; continue; }
+        if (!/[\s;&#|>)\]}"']/.test(after) && after !== '') { searchPos = cmdIdx + 1; continue; }
+        if (/[;|]/.test(input.slice(git.idx + git.len, cmdIdx))) break;
+        if (isInComment(input, cmdIdx)) { searchPos = cmdIdx + 1; continue; }
 
-      return cmd;
+        // Verify this token is the first non-flag word after "git" — i.e. the
+        // actual subcommand, not an argument value to a different subcommand.
+        const gap = input.slice(git.idx + git.len, cmdIdx);
+        const tokens = gap.trim().split(/\s+/).filter(Boolean);
+        // Every token before the candidate must be a flag or a flag argument.
+        // Git global flags like -c take a value argument (e.g. -c key=value).
+        let onlyFlagsAndArgs = true;
+        let expectFlagArg = false;
+        for (const t of tokens) {
+          if (expectFlagArg) { expectFlagArg = false; continue; }
+          if (t.startsWith('-')) {
+            // -c is a git global flag that takes the next token as its argument
+            if (t === '-c' || t === '-C' || t === '--work-tree' || t === '--git-dir' ||
+                t === '--namespace' || t === '--super-prefix') {
+              expectFlagArg = true;
+            }
+            continue;
+          }
+          onlyFlagsAndArgs = false;
+          break;
+        }
+        if (!onlyFlagsAndArgs) { searchPos = cmdIdx + 1; continue; }
+
+        if (cmdIdx < bestIdx) {
+          bestIdx = cmdIdx;
+          bestCmd = cmd;
+        }
+        break;
+      }
+    }
+
+    if (bestCmd) {
+      return { command: bestCmd, offset: bestIdx + bestCmd.length };
     }
 
     start = git.idx + git.len;
@@ -109,13 +151,17 @@ function detectGitCommand(input) {
 
 /**
  * Check if the input contains a --no-verify flag for a specific git command.
+ * Only inspects the portion of the input starting at `offset` (the position
+ * right after the detected subcommand keyword) so that flags belonging to
+ * earlier commands in a chain are not falsely matched.
  */
-function hasNoVerifyFlag(input, command) {
-  if (/--no-verify\b/.test(input)) return true;
+function hasNoVerifyFlag(input, command, offset) {
+  const region = input.slice(offset);
+  if (/--no-verify\b/.test(region)) return true;
 
   // For commit, -n is shorthand for --no-verify
   if (command === 'commit') {
-    if (/\s-n(?:\s|$)/.test(input) || /\s-n[a-zA-Z]/.test(input)) return true;
+    if (/\s-n(?:\s|$)/.test(region) || /\s-n[a-zA-Z]/.test(region)) return true;
   }
 
   return false;
@@ -132,10 +178,12 @@ function hasHooksPathOverride(input) {
  * Check a command string for git hook bypass attempts.
  */
 function checkCommand(input) {
-  const gitCommand = detectGitCommand(input);
-  if (!gitCommand) return { blocked: false };
+  const detected = detectGitCommand(input);
+  if (!detected) return { blocked: false };
 
-  if (hasNoVerifyFlag(input, gitCommand)) {
+  const { command: gitCommand, offset } = detected;
+
+  if (hasNoVerifyFlag(input, gitCommand, offset)) {
     return {
       blocked: true,
       reason: `BLOCKED: --no-verify flag is not allowed with git ${gitCommand}. Git hooks must not be bypassed.`,
@@ -197,23 +245,25 @@ function run(rawInput) {
 
 module.exports = { run };
 
-// Stdin fallback for spawnSync execution
-process.stdin.setEncoding('utf8');
-process.stdin.on('data', chunk => {
-  if (raw.length < MAX_STDIN) {
-    const remaining = MAX_STDIN - raw.length;
-    raw += chunk.substring(0, remaining);
-  }
-});
+// Stdin fallback for spawnSync execution — only when invoked directly, not via require()
+if (require.main === module) {
+  process.stdin.setEncoding('utf8');
+  process.stdin.on('data', chunk => {
+    if (raw.length < MAX_STDIN) {
+      const remaining = MAX_STDIN - raw.length;
+      raw += chunk.substring(0, remaining);
+    }
+  });
 
-process.stdin.on('end', () => {
-  const command = extractCommand(raw);
-  const result = checkCommand(command);
+  process.stdin.on('end', () => {
+    const command = extractCommand(raw);
+    const result = checkCommand(command);
 
-  if (result.blocked) {
-    process.stderr.write(result.reason + '\n');
-    process.exit(2);
-  }
+    if (result.blocked) {
+      process.stderr.write(result.reason + '\n');
+      process.exit(2);
+    }
 
-  process.stdout.write(raw);
-});
+    process.stdout.write(raw);
+  });
+}

tests/hooks/block-no-verify.test.js

@@ -0,0 +1,132 @@
+/**
+ * Tests for scripts/hooks/block-no-verify.js via run-with-flags.js
+ */
+
+const assert = require('assert');
+const path = require('path');
+const { spawnSync } = require('child_process');
+
+const runner = path.join(__dirname, '..', '..', 'scripts', 'hooks', 'run-with-flags.js');
+
+function test(name, fn) {
+  try {
+    fn();
+    console.log(`  \u2713 ${name}`);
+    return true;
+  } catch (error) {
+    console.log(`  \u2717 ${name}`);
+    console.log(`    Error: ${error.message}`);
+    return false;
+  }
+}
+
+function runHook(input, env = {}) {
+  const rawInput = typeof input === 'string' ? input : JSON.stringify(input);
+  const result = spawnSync('node', [runner, 'pre:bash:block-no-verify', 'scripts/hooks/block-no-verify.js', 'minimal,standard,strict'], {
+    input: rawInput,
+    encoding: 'utf8',
+    env: {
+      ...process.env,
+      ECC_HOOK_PROFILE: 'standard',
+      ...env
+    },
+    timeout: 15000,
+    stdio: ['pipe', 'pipe', 'pipe']
+  });
+
+  return {
+    code: Number.isInteger(result.status) ? result.status : 1,
+    stdout: result.stdout || '',
+    stderr: result.stderr || ''
+  };
+}
+
+let passed = 0;
+let failed = 0;
+
+console.log('\nblock-no-verify hook tests');
+console.log('─'.repeat(50));
+
+// --- Basic allow/block ---
+
+if (test('allows plain git commit', () => {
+  const r = runHook({ tool_input: { command: 'git commit -m "hello"' } });
+  assert.strictEqual(r.code, 0, `expected exit 0, got ${r.code}: ${r.stderr}`);
+})) passed++; else failed++;
+
+if (test('blocks --no-verify on git commit', () => {
+  const r = runHook({ tool_input: { command: 'git commit --no-verify -m "msg"' } });
+  assert.strictEqual(r.code, 2, `expected exit 2, got ${r.code}`);
+  assert.ok(r.stderr.includes('BLOCKED'), `stderr should contain BLOCKED: ${r.stderr}`);
+})) passed++; else failed++;
+
+if (test('blocks -n shorthand on git commit', () => {
+  const r = runHook({ tool_input: { command: 'git commit -n -m "msg"' } });
+  assert.strictEqual(r.code, 2, `expected exit 2, got ${r.code}`);
+  assert.ok(r.stderr.includes('BLOCKED'), `stderr should contain BLOCKED: ${r.stderr}`);
+})) passed++; else failed++;
+
+if (test('blocks core.hooksPath override', () => {
+  const r = runHook({ tool_input: { command: 'git -c core.hooksPath=/dev/null commit -m "msg"' } });
+  assert.strictEqual(r.code, 2, `expected exit 2, got ${r.code}`);
+  assert.ok(r.stderr.includes('core.hooksPath'), `stderr should mention core.hooksPath: ${r.stderr}`);
+})) passed++; else failed++;
+
+// --- Chained command false positive prevention (Comment 2) ---
+
+if (test('does not false-positive on -n belonging to git log in a chain', () => {
+  const r = runHook({ tool_input: { command: 'git log -n 10 && git commit -m "msg"' } });
+  assert.strictEqual(r.code, 0, `expected exit 0, got ${r.code}: ${r.stderr}`);
+})) passed++; else failed++;
+
+if (test('does not false-positive on --no-verify in a prior non-git command', () => {
+  const r = runHook({ tool_input: { command: 'echo --no-verify && git commit -m "msg"' } });
+  assert.strictEqual(r.code, 0, `expected exit 0, got ${r.code}: ${r.stderr}`);
+})) passed++; else failed++;
+
+if (test('still blocks --no-verify on the git commit part of a chain', () => {
+  const r = runHook({ tool_input: { command: 'git log -n 5 && git commit --no-verify -m "msg"' } });
+  assert.strictEqual(r.code, 2, `expected exit 2, got ${r.code}`);
+})) passed++; else failed++;
+
+// --- Subcommand detection (Comment 4) ---
+
+if (test('does not misclassify "commit" as subcommand when it is an argument to push', () => {
+  // "git push origin commit" — "commit" is a refspec arg, not the subcommand
+  const r = runHook({ tool_input: { command: 'git push origin commit' } });
+  // This should detect "push" as the subcommand, not "commit"
+  // Either way it should not block since there's no --no-verify
+  assert.strictEqual(r.code, 0, `expected exit 0, got ${r.code}: ${r.stderr}`);
+})) passed++; else failed++;
+
+// --- Blocks on push --no-verify ---
+
+if (test('blocks --no-verify on git push', () => {
+  const r = runHook({ tool_input: { command: 'git push --no-verify' } });
+  assert.strictEqual(r.code, 2, `expected exit 2, got ${r.code}`);
+  assert.ok(r.stderr.includes('git push'), `stderr should mention git push: ${r.stderr}`);
+})) passed++; else failed++;
+
+// --- Non-git commands pass through ---
+
+if (test('allows non-git commands', () => {
+  const r = runHook({ tool_input: { command: 'npm test' } });
+  assert.strictEqual(r.code, 0, `expected exit 0, got ${r.code}: ${r.stderr}`);
+})) passed++; else failed++;
+
+// --- Plain text input (not JSON) ---
+
+if (test('handles plain text input', () => {
+  const r = runHook('git commit -m "hello"');
+  assert.strictEqual(r.code, 0, `expected exit 0, got ${r.code}: ${r.stderr}`);
+})) passed++; else failed++;
+
+if (test('blocks plain text input with --no-verify', () => {
+  const r = runHook('git commit --no-verify -m "msg"');
+  assert.strictEqual(r.code, 2, `expected exit 2, got ${r.code}`);
+})) passed++; else failed++;
+
+console.log('─'.repeat(50));
+console.log(`Passed: ${passed}  Failed: ${failed}`);
+
+process.exit(failed > 0 ? 1 : 0);