diff --git a/.claude/hooks/README.md b/.claude/hooks/README.md index 9a54e4a..6e4069c 100644 --- a/.claude/hooks/README.md +++ b/.claude/hooks/README.md @@ -12,7 +12,7 @@ All behavior is controlled by **`config.json`** — no project logic is hardcode |---|---| | `config.json` | Shared defaults for the config-aware hooks; two hooks ignore it (see "Hooks that ignore config") (per-project overrides: `/.claude/hooks.config.json`, see "Project-local overrides") | | `lib/hook-config.cjs` | Shared loader: reads `config.json` from the hooks directory and merges the optional project-local override over it | -| `lib/git-command.cjs` | Shared command parser for the git/gh guards (merge, branch-delete, push-audit, CI status): quote-aware segment splitting, env/sudo/`bash -c` unwrapping, path-qualified binaries, git/gh global options such as `git -C ""`, `$(...)`/backtick substitutions and `( ... )` subshells parsed as commands of their own, and the working directory each command runs in, tracked through `cd`/`pushd` and `git -C` (unknown for `cd "$VAR"`, `cd -` and the like) | +| `lib/git-command.cjs` | Shared command parser for the git/gh guards (merge, branch-delete, push-audit, CI status): quote-aware segment splitting, env/sudo/`xargs` prefix stripping, `bash -c`/`eval` unwrapping, path-qualified binaries, git/gh global options such as `git -C ""`, `$(...)`/backtick substitutions and `( ... )` subshells parsed as commands of their own, and the working directory each command runs in, tracked through `cd`/`pushd` and `git -C` (unknown for `cd "$VAR"`, `cd -` and the like) | | `check-arch-boundary.mjs` | PreToolUse: block file writes that violate package rules | | `check-dep-guard.js` | PreToolUse: warn on dependency installs | | `check-script-exists.mjs` | PreToolUse: warn/block when an npm run script doesn't exist; detect no-op commands | @@ -20,17 +20,19 @@ All behavior is controlled by **`config.json`** — no project logic is hardcode | `check-bash-description.js` | PreToolUse: block Bash calls without a description | | `check-commit-message.js` | PreToolUse: warn/block each `git commit` (chained, `git -C`, path-qualified, `bash -c`, heredoc-fed) whose message lacks the required markers (default WHY/EXPECTED) | | `check-merge-guard.js` | PreToolUse: unconditionally block `gh pr merge`, a merge-REST `PUT`, a local `git merge` on master/main, or a push that lands commits directly on master/main | -| `check-git-push-audit.js` | PreToolUse: audit a `git push` diff for secrets (blocks), debug statements, commented-out code, lockfile drift, missing tests | +| `check-git-push-audit.js` | PreToolUse: audit a `git push` diff for secrets (blocks), debug statements, commented-out code, lockfile drift, missing tests. A push that only deletes remote branches sends no commits and is skipped | | `check-branch-delete-guard.js` | PreToolUse: block `git push --delete`/`:branch` when an open PR still bases off that branch | | `check-search-command.js` | PreToolUse: nudge (or block) raw shell `grep`/`find`/`cat`/`head`/`tail` in favor of dedicated search/read tools | | `validate-content.mjs` | PostToolUse: run validation after watched files change | | `check-empty-tests.mjs` | PostToolUse: fail when test files have empty blocks or no assertions (Write content, or the file on disk after Edit/MultiEdit) | | `check-todo-format.mjs` | PostToolUse: warn/block on non-conforming TODO comment format (Write content, or the file on disk after Edit/MultiEdit) | | `check-ci-status-after-push.js` | PostToolUse: after a push, block until the matching GitHub Actions run completes and report the real result | +| `check-agent-routing.js` | UserPromptSubmit, opt-in (not registered by default): add a workflow reminder to the conversation when a prompt matches a route's keywords; see "Optional: agent routing reminders" | | `session-start.sh` | Optional manual wrapper that invokes session-start.cjs (no longer registered in hooks.json) | | `session-start.cjs` | SessionStart: loads config through `lib/hook-config.cjs` and prints the project orientation banner (project name defaults to the project directory basename) | | `hooks.json` | Human-readable reference copy of the settings.json hooks block | -| `check-branch-delete-guard.test.js`, `check-merge-guard.test.js`, `check-git-push-audit.test.js`, `check-ci-status-after-push.test.js`, `check-commit-message.test.js`, `check-search-command.test.js`, `lib/git-command.test.cjs` | Behavioral tests for each guard's detection/extraction logic, plus lifecycle tests that run the hook on stdin JSON (the push audit against throwaway git repos) | +| `check-branch-delete-guard.test.js`, `check-merge-guard.test.js`, `check-git-push-audit.test.js`, `check-ci-status-after-push.test.js`, `check-commit-message.test.js`, `check-env-guard.test.js`, `check-search-command.test.js`, `check-script-exists.test.js`, `check-agent-routing.test.js`, `lib/git-command.test.cjs`, `lib/git-command-wrappers.test.cjs` | Behavioral tests for each guard's detection/extraction logic, plus lifecycle tests that run the hook on stdin JSON (the push audit against throwaway git repos) | +| `check-merge-guard.hook.test.js` | Runtime tests: run `check-merge-guard.js` with stdin JSON and assert the exit code for `eval`/`xargs`-wrapped merges and pushes, so a parser gap shows up as a hook that lets the command run | | `hook-config-integration.test.cjs`, `lib/hook-config.test.cjs` | Tests for the config loader, project-local overrides and hook registration | Run every test from the hooks directory with `node --test *.test.js *.test.cjs lib/*.test.cjs`. A bare @@ -63,7 +65,7 @@ keys you want to change (see "Project-local overrides" below). Available keys: - `script_guard` — set `enabled: true` to validate npm run scripts before they run - `bash_description_guard` — requires terminal command explanations; always on when registered (the hook does not read this setting yet) - `env_guard` — blocks `.env` reads; always on when registered (the hook does not read this setting) -- `commit_guard` — set `enabled: true` to require WHY/EXPECTED (or your `required_markers`) in git commit messages; `on_violation` picks warn or block +- `commit_guard` — enabled by default in block mode, requires WHY/EXPECTED (or your `required_markers`) in git commit messages; `on_violation` picks warn or block - `merge_guard` — enabled by default, unconditionally blocks any command that merges a PR or lands commits on master/main - `push_audit` — enabled by default, blocks pushes whose diff contains hardcoded secrets - `branch_delete_guard` — enabled by default, blocks deleting a branch a stacked PR still bases off (requires `gh`) @@ -125,6 +127,19 @@ Merge the `hooks` block from `hooks.json` into your project's `.claude/settings. } ``` +#### Optional: agent routing reminders + +`check-agent-routing.js` isn't in the block above: its default keywords (`plan`, `error`, `build`, ...) match most prompts. To use +it, tailor the keywords and reminders in its `ROUTES` table to the project, then add: + +```json +"UserPromptSubmit": [ + { + "hooks": [{ "type": "command", "command": "node .claude/hooks/check-agent-routing.js" }] + } +] +``` + ### 4. Verify Run the session banner manually to confirm config loads correctly: @@ -229,8 +244,8 @@ Block reads of `.env` files: ``` No fields beyond `enabled`, and `check-env-guard.js` does not read config at all, so the block is -unconditional today and the `enabled` flag has no effect. Blocks (exit 2) any `Read` call whose file path matches `.env` or -`.env.*`. Environment files carry secrets and must never be read into context. +unconditional today and the `enabled` flag has no effect. Blocks (exit 2) any `Read` call whose file name matches `.env` or +`.env.*`, compared case-insensitively (`.ENV` opens the same file on Windows and macOS). Environment files carry secrets and must never be read into context. --- @@ -451,8 +466,8 @@ Validate npm run scripts before they execute: } ``` -- `package_json_paths` — list of package.json files to check for script names (extend for monorepos) -- `on_missing` — `"warn"` prints to stderr and exits 0; `"block"` exits 1 and stops the command +- `package_json_paths` — list of package.json files to check for script names, relative to the project directory (`CLAUDE_PROJECT_DIR`); extend for monorepos +- `on_missing` — `"warn"` prints to stderr and exits 0; `"block"` exits 2 and stops the command - Also detects trivial no-op commands (`true`, `exit 0`, empty echo) regardless of `enabled` --- @@ -463,9 +478,9 @@ Require WHY/EXPECTED sections in every commit message: ```json "commit_guard": { - "enabled": false, + "enabled": true, "required_markers": ["WHY:", "EXPECTED:"], - "on_violation": "warn" + "on_violation": "block" } ``` @@ -475,7 +490,8 @@ Require WHY/EXPECTED sections in every commit message: Every `git commit` in a Bash call is checked on its own — chained commands, `git -C ` and other git global options, a path-qualified `git`, and `bash -c "..."` wrappers — and a heredoc message body (`git commit -F - <<'EOF'`) counts as part of its commit. Markers in a different -command of the chain don't count. +command of the chain don't count. If the shared config can't be read, the defaults apply and the +guard blocks. --- @@ -635,7 +651,8 @@ Claude Code invokes each registered command when the matching tool is used. - `check-arch-boundary.mjs` checks the file **as it will be after the write**: Write content, or for Edit/MultiEdit the current file with the replacements applied. Any edit to a file that already has a violation is therefore blocked until the violation is fixed, while files that aren't touched are never checked. - `forbidden_content_pattern` is a simple regex heuristic — it can produce false positives in test files or comments. - Hooks run in the **project root** working directory. Relative paths in `validate_command` are resolved from there. -- **Mixed module system**: `check-branch-delete-guard.js`, `check-ci-status-after-push.js`, `check-env-guard.js`, `check-git-push-audit.js`, and `check-merge-guard.js` are CommonJS (`require`); `check-dep-guard.js` and `check-search-command.js` were converted to CommonJS too (see changelog below). `check-arch-boundary.mjs`, `check-empty-tests.mjs`, `check-script-exists.mjs`, `check-todo-format.mjs`, and `validate-content.mjs` use ESM `import`, so they carry the `.mjs` extension: Node then loads them as ESM whatever the nearest `package.json` `"type"` says. As `.js` files they would crash in a project whose `package.json` has `"type": "commonjs"`. +- **Mixed module system**: `check-branch-delete-guard.js`, `check-ci-status-after-push.js`, `check-env-guard.js`, `check-git-push-audit.js`, and `check-merge-guard.js` are CommonJS (`require`); `check-dep-guard.js` and `check-search-command.js` were converted to CommonJS too (see changelog below). `check-arch-boundary.mjs`, `check-empty-tests.mjs`, `check-script-exists.mjs`, `check-todo-format.mjs`, and `validate-content.mjs` use ESM `import`, so they carry the `.mjs` extension: Node then loads them as ESM whatever the nearest `package.json` `"type"` says. As `.js` files they would crash in a project whose `package.json` has `"type": "commonjs"`, which turns off Node's module-syntax detection. +- **Shell indirection the guards cannot see**: `eval` and `xargs` are unwrapped, but a command built at run time (`eval "$CMD"`, `$CMD`), a script that pushes internally, and `xargs` long options that take a separate value beyond `--max-args`, `--max-procs`, `--max-lines`, `--max-chars`, `--delimiter`, `--eof` and `--arg-file` are not detected. Also undetected: an `xargs` option value that is itself quoted and contains a space (`xargs -E"a b" git push`), and a quoted command word that contains a space other than a path to `git`/`gh`. Refs piped in on stdin are not known to the audit, so `git branch --list | xargs git push origin` audits `HEAD~1..HEAD`; only the `{}` placeholder form (`xargs -I{} git push origin {}`) is detected and blocked, because its ref cannot be resolved. `eval` runs in the current shell, so a `cd` inside it carries to later commands, unlike `bash -c`, whose `cd` does not. --- @@ -652,6 +669,17 @@ Claude Code invokes each registered command when the matching tool is used. ## Changelog +- Fixes ported from a project's adoption of this template: + - The five ESM hooks are renamed to `.mjs`, since as `.js` they crash under `"type": "commonjs"`. + - `check-commit-message.js` reads `commit_guard`: `enabled`, `required_markers` and `on_violation`, with block on by default. It also checks every `git commit` in a chain, wrapper or heredoc. + - `check-git-push-audit.js` honors `push_audit.enabled`. Before trusting `git diff`, it also checks that every blob the diff needs exists, because `git diff` can exit 0 with a blob missing. + - `check-ci-status-after-push.js` kills `gh run watch` at `completion_timeout_seconds`. + - `check-arch-boundary.mjs` applies Edit/MultiEdit replacements to the current file before checking it. + - `check-empty-tests.mjs` and `check-todo-format.mjs` read the file from disk after an Edit/MultiEdit, and `assert.(...)` now counts as an assertion. + - `check-env-guard.js` compares file names case-insensitively. + - The adversarial-coder, constitutional-fix and test-driven-development skills no longer commit without the user's approval. `test-driven-development/Skill.md` is renamed to `SKILL.md`. + +- `eval` and `xargs` wrappers are now seen through by the git/gh guards. `eval "git push origin main"` is analyzed like `bash -c`, and `xargs [options] git push ...` (including clustered short options such as `-rt`, `-rn1` and `-0I{}`, and attached long options) exposes the inner command, so neither form is a silent bypass of the merge, branch-delete, push-audit or CI-status guards. `eval` runs all of its arguments after quote removal (`eval "git push" origin +main` keeps the refspec), and a quoted or backslash-escaped command word or wrapper option (`"eval"`, `'sudo'`, `g\it`, `xargs '-r'`) is matched by its unquoted spelling. An unquoted Windows path such as `C:\Git\bin\git.exe` keeps its backslashes as separators. ANSI-C quoting (`eval $'git push origin main'`) and backslash-newline line continuations are unquoted the way the shell does it, `xargs.exe` is recognised, and xargs `-R`, `-J` and `-S` take a separate value. - Command parsing for the git/gh guards moved into `lib/git-command.cjs`. `check-branch-delete-guard.js`, `check-ci-status-after-push.js`, and `check-git-push-audit.js` previously recognized only a subset of the push forms `check-merge-guard.js` handled, so `sudo git push`, `/usr/bin/git push`, and `git -C push` skipped them. All four now share one parser. Option values are matched as whole shell words, so `git -C "C:\Work Tree" push origin main` is no longer cut off at the space, and prefixes are stripped before shell wrappers are unwrapped, so `env bash -c "gh pr merge 27"` is caught. - `check-git-push-audit.js` generic secret floors restored to 8 characters (credentials) and 4 (passwords), from 20 and 12. Test fixtures that trip them use the existing trailing `// nosecret` marker. diff --git a/.claude/hooks/check-agent-routing.js b/.claude/hooks/check-agent-routing.js new file mode 100644 index 0000000..7a68a8a --- /dev/null +++ b/.claude/hooks/check-agent-routing.js @@ -0,0 +1,196 @@ +#!/usr/bin/env node +/** + * check-agent-routing.js + * Hook type : UserPromptSubmit + * Purpose : Remind Claude Code to follow the correct workflow or invoke + * the right skill/agent before starting implementation work. + * + * Inputs (via stdin JSON): + * { prompt: string } + * + * Opt-in: not registered in hooks.json by default, since its generic + * keywords ('plan', 'error', 'build', ...) match most prompts until a project + * tailors them. See README.md to register it. + * + * Always exits 0 (never blocks). A matching prompt prints its reminder to + * stdout, which Claude Code adds to the conversation as context. + * + * Routing table (prompt keywords → recommended workflow): + * schema / data model / type / interface → model-first, plan before code + * test / tdd / failing test → /test-driven-development + * component / ui / layout / screen / panel → UI best practices + * content / copy / document / write → keep content out of code + * refactor → scope check before changes + * bug / fix / broken / error → diagnose before changing + * plan / architect / design / structure → plan before implement + * release / ship / deploy → pre-ship checklist + * implement / build / add feature / scaffold → agent routing checklist + */ + +// ─── HOW TO ADAPT THIS FILE FOR A NEW PROJECT ──────────────────────────────── +// Each route has two parts: +// keywords — lowercase phrases matched against the user's prompt (substring match). +// Add project-specific vocabulary here: domain nouns, file/package names, +// framework names, feature-area jargon, custom skill/agent names, etc. +// reminder — lines printed to stdout (added to the conversation) when a keyword +// matches. Informational only (exit 0, never blocking). Point to your project's actual skills, +// agents, package paths, commands, and constraints. +// +// Tips: +// • Keep keywords lowercase; the prompt is .toLowerCase()'d before matching. +// • Use leading/trailing spaces in keywords (' fix ') to avoid false positives. +// • Add a new route object for any domain that doesn't fit the defaults below. +// • Remove routes that have no equivalent in your project. +// ───────────────────────────────────────────────────────────────────────────── + +const ROUTES = [ + { + // Add project-specific schema vocabulary here: + // e.g. your ORM model names, proto/GraphQL type keywords, domain aggregate names, + // validation library keywords ('joi', 'yup', 'zod', 'pydantic', …), or the names + // of foundational shared packages that own your type contracts. + keywords: ['schema', 'data model', 'domain type', 'domain logic', 'zod', 'type definition', 'interface definition'], + reminder: [ + '📐 SCHEMA / DATA MODEL WORK DETECTED', + ' 1. Define types/schemas before writing any implementation or UI.', + // Replace with your project's review gate, e.g.: + // ' 2. Get architect review before touching packages/core-types.' + ' 2. Get architect review before changing shared or foundational models.', + ' 3. Follow TDD: write a failing test against the new shape first.', + ], + }, + { + // Add your project's test framework keywords if relevant: + // e.g. 'jest', 'vitest', 'pytest', 'rspec', 'go test', 'cypress', 'playwright' + keywords: ['write test', 'failing test', 'tdd', 'red green', 'unit test', 'add test', 'test coverage'], + reminder: [ + '🧪 TDD WORK DETECTED', + // Replace /test-driven-development with your project's TDD skill name if different. + ' Invoke /test-driven-development skill.', + ' Follow RED → GREEN → COMMIT → REFACTOR.', + ' Do NOT write implementation before the test exists and fails.', + ], + }, + { + // Add UI-layer keywords specific to your stack: + // e.g. 'tailwind', 'shadcn', 'mui', 'chakra', 'swiftui', 'jetpack compose', + // or area names like 'dashboard', 'drawer', 'toast', 'wizard', 'page' + keywords: ['component', 'ui ', ' ui', 'layout', 'screen', 'panel', 'sidebar', 'modal', 'styling', 'styles'], + reminder: [ + '🎨 UI WORK DETECTED', + // Replace with your project's UI constraints, e.g.: + // ' No direct store/service imports in presentational components — pass props.' + // ' UI agent: ui-builder. Domain types must exist before UI is scaffolded.' + ' Presentational components should receive props only — avoid tight coupling to stores/services.', + ' Ensure domain types and data contracts are defined before building UI on top of them.', + ], + }, + { + // Add your project's content/copy vocabulary: + // e.g. CMS entry types, localisation keys, content package names, markdown file paths, + // or domain-specific content nouns ('blog post', 'email template', 'help article') + keywords: ['write content', 'add copy', 'document content', 'static content', 'copy change'], + reminder: [ + '📜 CONTENT / COPY WORK DETECTED', + // Replace with your project's content location and validation command, e.g.: + // ' All content lives in packages/content — never hardcoded in source files.' + // ' Run `npm run validate:content` after editing JSON/YAML content files.' + ' Keep content out of application logic — store in dedicated content files or a CMS.', + ' Do not hardcode user-facing strings directly in component or logic files.', + ], + }, + { + // 'refactor' is usually broad enough — add synonyms if your team uses specific terms, + // e.g. 'extract service', 'move to module', 'split package', 'rename module' + keywords: ['refactor'], + reminder: [ + '♻️ REFACTOR DETECTED', + // Replace with your project's refactor skill if you have one, e.g.: + // ' Invoke /refactor-boundary-check before moving code across packages.' + ' Scope the change before starting: what moves, what stays, what breaks?', + ' Confirm tests pass before and after. Do not mix refactor with feature work.', + ], + }, + { + // Adjust ' fix ' spacing / add synonyms as needed for your team's vocabulary. + keywords: ['bug', ' fix ', 'broken', 'not working', 'error', 'failing', 'crash'], + reminder: [ + '🐛 BUG / FIX DETECTED', + // Replace with your project's QA agent name if you have one, e.g.: + // ' Use the qa-reviewer agent. Reproduce before patching.' + ' Diagnose root cause before changing code — read the error, check assumptions.', + ' Do not retry the identical fix blindly. Escalate to the user if stuck after investigation.', + ], + }, + { + // Add your project's planning/design vocabulary if different. + keywords: ['plan', 'architect', 'design', 'structure', 'how should', 'approach'], + reminder: [ + '🏛️ PLANNING / ARCHITECTURE DETECTED', + // Replace with your project's architect agent/skill name if you have one. + ' Use the Plan (architect) subagent for design decisions.', + ' Propose a short written plan before any implementation begins.', + ' Get explicit approval before proceeding to code.', + ], + }, + { + // Add your project's deploy/release vocabulary: + // e.g. 'merge to main', 'cut release', 'tag version', 'push to prod', CI pipeline names + keywords: ['release', 'ship', 'deploy', 'publish'], + reminder: [ + '🚀 RELEASE / DEPLOY DETECTED', + // Replace with your project's release skill and checklist, e.g.: + // ' Invoke /release-readiness. Ensure CHANGELOG, version bump, and tag are ready.' + // ' Confirm deployment target (staging vs prod) and env-specific secrets.' + ' Run the pre-ship checklist: tests green, changelog updated, no debug artifacts.', + ' Confirm deployment target and any environment-specific config before pushing.', + ], + }, + { + // Add your project's feature-work vocabulary: + // e.g. names of custom scaffold skills, multi-package workspace commands, + // or area-specific nouns that signal new feature work ('add route', 'new endpoint') + keywords: ['implement', 'build', 'add feature', 'add support', 'scaffold'], + reminder: [ + '⚙️ IMPLEMENTATION WORK DETECTED', + ' Before writing code:', + // Replace with your project's agent roster and scaffold skill, e.g.: + // ' 1. Identify the correct agent (domain-engineer / ui-builder / content-editor).' + // ' 2. Invoke /feature-scaffold if this touches multiple packages.' + ' 1. Identify the right subagent type (Plan / Explore / general-purpose).', + ' 2. Define or confirm data contracts/types first.', + ' 3. Start with a failing test via /test-driven-development.', + ], + }, +]; + +const chunks = []; +process.stdin.on('data', c => chunks.push(c)); +process.stdin.on('end', () => { + let data; + try { + data = JSON.parse(Buffer.concat(chunks).toString('utf8')); + } catch { + process.exit(0); + } + + const prompt = (data?.prompt ?? '').toLowerCase(); + if (!prompt) process.exit(0); + + const matched = ROUTES.filter(route => route.keywords.some(kw => prompt.includes(kw))); + + if (matched.length === 0) process.exit(0); + + // Stdout on exit 0 is what Claude Code adds to the conversation for a + // UserPromptSubmit hook; stderr or a non-zero exit only reaches the terminal. + process.stdout.write('\n┌─ AGENT ROUTING REMINDER ────────────────────────────────────────\n'); + for (const route of matched) { + route.reminder.forEach(line => process.stdout.write(`│ ${line}\n`)); + if (matched.indexOf(route) < matched.length - 1) { + process.stdout.write('│\n'); + } + } + process.stdout.write('└──────────────────────────────────────────────────────────────────\n\n'); + + process.exit(0); +}); diff --git a/.claude/hooks/check-agent-routing.test.js b/.claude/hooks/check-agent-routing.test.js new file mode 100644 index 0000000..ec15f55 --- /dev/null +++ b/.claude/hooks/check-agent-routing.test.js @@ -0,0 +1,36 @@ +// check-agent-routing.test.js +// Lifecycle tests for check-agent-routing.js — run with +// `node --test *.test.js *.test.cjs lib/*.test.cjs` from the hooks directory. +// +// A UserPromptSubmit hook's stdout on exit 0 is what Claude Code adds to +// the conversation; stderr or a non-zero exit only reaches the terminal. + +'use strict'; + +const test = require('node:test'); +const assert = require('node:assert/strict'); +const path = require('node:path'); +const { spawnSync } = require('node:child_process'); + +function runHook(prompt) { + const r = spawnSync(process.execPath, [path.join(__dirname, 'check-agent-routing.js')], { + input: JSON.stringify({ hook_event_name: 'UserPromptSubmit', prompt }), + encoding: 'utf8', + timeout: 30000, + }); + return { status: r.status, stdout: r.stdout || '' }; +} + +test('hook lifecycle', async (t) => { + await t.test('a matching prompt adds its reminder as context (stdout, exit 0)', () => { + const r = runHook('Please implement the new checkout page'); + assert.equal(r.status, 0); + assert.match(r.stdout, /IMPLEMENTATION WORK DETECTED/); + }); + + await t.test('an unmatched prompt adds nothing', () => { + const r = runHook('hello'); + assert.equal(r.status, 0); + assert.equal(r.stdout, ''); + }); +}); diff --git a/.claude/hooks/check-branch-delete-guard.js b/.claude/hooks/check-branch-delete-guard.js index b23baca..c6ae357 100644 --- a/.claude/hooks/check-branch-delete-guard.js +++ b/.claude/hooks/check-branch-delete-guard.js @@ -39,7 +39,8 @@ const { execSync, execFileSync } = require('child_process'); const { loadHookConfig } = require('./lib/hook-config.cjs'); -const { commandSegments, pushTargetDir, stripLeadingEnvPrefix, stripSurroundingQuotes } = require('./lib/git-command.cjs'); +const fs = require('fs'); +const { commandSegments, pushArgs, splitRefspec, stripLeadingEnvPrefix, stripSurroundingQuotes } = require('./lib/git-command.cjs'); function isEnabled() { try { @@ -82,13 +83,9 @@ function tryRun(cmd, cwd) { // // Handles `--delete`/`-d` in any position relative to the remote (e.g. // both `git push origin --delete branch` and `git push --delete origin -// branch` are valid git syntax and equally common) by tokenizing each -// push segment and treating every non-flag token after the remote as a -// branch to delete — rather than assuming a fixed "remote, then -// --delete, then branches" shape, which silently matched zero branches -// (and so silently allowed the delete) for the flag-before-remote form. -// Also handles the older `:branch` refspec shorthand independently, since -// that doesn't need a --delete/-d flag at all. +// branch` are valid git syntax and equally common), and the older +// `:branch` refspec shorthand, which doesn't need a --delete/-d flag at +// all. See branchesDeletedBy(). // // Segment splitting and normalization (env/sudo prefixes, `bash -c` // wrappers, a path-qualified git binary, `git -C ` and other global @@ -114,41 +111,49 @@ function normalizeBranchName(name) { } function extractDeletedBranches(fullCmd) { - const branches = new Set(); - - for (const { normalized: segment } of commandSegments(fullCmd)) { - // Anchored at the start of the segment — "contains git push" isn't - // enough, since that also matches inside an unrelated quoted string - // that merely mentions it. - if (!/^git\s+push\b/.test(segment)) continue; - - const tokens = segment.split(/\s+/); // tokens[0] === 'git', tokens[1] === 'push' - // Unquoted first: the shell strips the quotes from `"--delete"` before - // git sees it, so it's the same flag. - const rest = tokens.slice(2).map(stripSurroundingQuotes); - const hasDeleteFlag = rest.some((t) => t === '--delete' || t === '-d'); - if (hasDeleteFlag) { - const nonFlags = rest.filter((t) => !t.startsWith('-')); - for (const b of nonFlags.slice(1)) branches.add(normalizeBranchName(b)); // [0] is the remote - } + return [...new Set(deletesByRepo(fullCmd).flatMap((d) => d.branches))]; +} - // `git push origin :branch-name` — a leading bare colon means - // "delete", scoped to this same segment. Captures any run of - // non-whitespace after the colon rather than an allow-listed character - // class — git ref names permit a wide range of characters (`@`, `+`, - // etc.; see git-check-ref-format), and a narrower class here would - // silently fail to extract a real, valid branch name, defeating the - // guard for exactly the kind of ref it should be catching. The colon - // may also be immediately preceded by an opening quote (a quoted - // refspec token, e.g. `":branch"`) rather than only whitespace/start - // of segment — without allowing for that, the colon itself is never - // found and the whole delete goes undetected, not just misparsed. - for (const m of segment.matchAll(/(?:^|\s)["']?:(\S+?)["']?(?=\s|$)/g)) { - branches.add(normalizeBranchName(m[1])); +// The branches each repository's pushes delete: `[{ dir, branches }]`, one +// entry per repository, where `dir` is the directory those pushes run in +// (null when it can't be determined or doesn't exist). A chain can delete +// branches in several repositories, and each one's PRs are its own. +function deletesByRepo(fullCmd) { + const byDir = new Map(); + for (const { normalized: segment, cwd } of commandSegments(fullCmd)) { + const dir = cwd !== null && fs.existsSync(cwd) ? cwd : null; + for (const branch of branchesDeletedBy(segment)) { + if (!byDir.has(dir)) byDir.set(dir, new Set()); + byDir.get(dir).add(branch); } } + return [...byDir].map(([dir, branches]) => ({ dir, branches: [...branches] })); +} + +// The branches one normalized segment deletes, if it's a `git push`. +// Arguments come from the shared pushArgs(): quotes removed, flags in any +// position, the remote from a positional or `--repo`/`--repo=`. With +// --delete/-d every refspec names a branch to delete; without it, a +// refspec with an empty source (`:branch`, or `+:branch` with the force +// prefix) does. A colon refspec in the remote position (`git push :branch`) +// counts too, erring toward checking. +function branchesDeletedBy(segment) { + // Anchored at the start of the segment — "contains git push" isn't + // enough, since that also matches inside an unrelated quoted string + // that merely mentions it. + if (!/^git\s+push\b/.test(segment)) return []; - return [...branches]; + const { remote, refspecs, deletes } = pushArgs(segment); + // A remote URL (`git@host:owner/repo`, `ssh://...`) has a colon too, but + // isn't a refspec. + const remoteIsRefspec = remote && remote.includes(':') && !/@|:\/\//.test(remote); + const candidates = remoteIsRefspec ? [remote, ...refspecs] : refspecs; + const branches = new Set(); + for (const refspec of candidates) { + const { src, dest } = splitRefspec(refspec); + if (deletes || !src) branches.add(normalizeBranchName(dest)); + } + return [...branches].filter(Boolean); } // The open PRs basing off each branch: `{ affected, warnings }`, where @@ -212,24 +217,31 @@ function runHook() { } const cmd = (input?.tool_input?.command || '').trim(); - const branches = extractDeletedBranches(cmd); - if (branches.length === 0) process.exit(0); // not a delete push — allow + const deletes = deletesByRepo(cmd); + if (deletes.length === 0) process.exit(0); // not a delete push — allow - // Open PRs of the repo being pushed to (after any `cd`/`git -C`), not - // this hook's own working directory, whose PRs say nothing about it. - const repoDir = pushTargetDir(cmd); - if (!repoDir) { - console.error('[branch-delete-guard] Could not determine which repository this delete targets (e.g. `cd "$VAR"`) — cannot check for stacked PRs depending on this branch. Allowing, but verify manually with `gh pr list --state open` in that repo first.'); - process.exit(0); + // Each delete is checked against the open PRs of the repo it pushes to + // (after any `cd`/`git -C`), not this hook's own working directory or + // another push's repo, whose PRs say nothing about it. Every repo is + // checked before deciding, so an unchecked one can't hide a dependent + // found in another. + const affected = []; + const warnings = []; + for (const { dir, branches } of deletes) { + const named = branches.map((b) => `"${b}"`).join(', '); + if (!dir) { + warnings.push(`[branch-delete-guard] Could not determine which repository the delete of ${named} targets (e.g. \`cd "$VAR"\`) — cannot check for stacked PRs depending on it. Allowing, but verify manually with \`gh pr list --state open\` in that repo first.`); + continue; + } + const ghAuth = tryRun('gh auth status', dir); + if (!ghAuth.ok) { + warnings.push(`[branch-delete-guard] gh CLI not available/authenticated — cannot check for stacked PRs depending on ${named}. Allowing, but verify manually with \`gh pr list --state open\` in ${dir} first.`); + continue; + } + const found = findDependents(branches, dir); + affected.push(...found.affected); + warnings.push(...found.warnings); } - - const ghAuth = tryRun('gh auth status', repoDir); - if (!ghAuth.ok) { - console.error('[branch-delete-guard] gh CLI not available/authenticated — cannot check for stacked PRs depending on this branch. Allowing, but verify manually with `gh pr list --state open` first.'); - process.exit(0); - } - - const { affected, warnings } = findDependents(branches, repoDir); if (affected.length === 0) { for (const warning of warnings) console.error(warning); process.exit(0); diff --git a/.claude/hooks/check-branch-delete-guard.test.js b/.claude/hooks/check-branch-delete-guard.test.js index 99e82f2..43ed5cd 100644 --- a/.claude/hooks/check-branch-delete-guard.test.js +++ b/.claude/hooks/check-branch-delete-guard.test.js @@ -38,6 +38,20 @@ test('extractDeletedBranches', async (t) => { assert.deepEqual(branches('git push origin :branch4'), ['branch4']); }); + await t.test('--repo= instead of a positional remote', () => { + assert.deepEqual(branches('git push --repo=origin --delete main'), ['main']); + assert.deepEqual(branches('git push --repo origin --delete topic other'), ['other', 'topic']); + }); + + await t.test('an SSH remote is not read as a delete refspec', () => { + assert.deepEqual(branches('git push git@github.com:owner/repo --delete topic'), ['topic']); + assert.deepEqual(branches('git push ssh://git@github.com:22/owner/repo :topic'), ['topic']); + }); + + await t.test('a force-prefixed delete refspec: +:branch', () => { + assert.deepEqual(branches('git push origin +:feature-base'), ['feature-base']); + }); + await t.test('fully-qualified :refs/heads/branch shorthand is normalized to the bare name', () => { assert.deepEqual(branches('git push origin :refs/heads/branch5'), ['branch5']); }); @@ -211,6 +225,18 @@ test('hook lifecycle', async (t) => { assert.match(r.stderr, /gh CLI not available/); }); + await t.test('each delete is checked in its own repo, not the first push\'s', () => { + const workTree = fs.mkdtempSync(path.join(os.tmpdir(), 'work-tree-')); + try { + const r = runHookWithoutGh(`git -C "${workTree}" push origin --delete b; cd "$REPO" && git push origin --delete a`); + assert.equal(r.status, 0); + assert.match(r.stderr, /Could not determine which repository/); + assert.match(r.stderr, /gh CLI not available/); + } finally { + fs.rmSync(workTree, { recursive: true, force: true }); + } + }); + await t.test('a non-delete push exits without reaching the PR check', () => { const r = runHookWithoutGh('git push origin topic'); assert.equal(r.status, 0); diff --git a/.claude/hooks/check-ci-status-after-push.js b/.claude/hooks/check-ci-status-after-push.js index 5f6439a..bca0195 100644 --- a/.claude/hooks/check-ci-status-after-push.js +++ b/.claude/hooks/check-ci-status-after-push.js @@ -24,7 +24,7 @@ const { execFileSync } = require('child_process'); const { loadHookConfig } = require('./lib/hook-config.cjs'); -const { firstPushSegment, isGitPush, pushArgs, pushTargetDir, splitRefspec } = require('./lib/git-command.cjs'); +const { isGitPush, pushArgs, pushSegments, splitRefspec } = require('./lib/git-command.cjs'); function readConfig() { try { @@ -134,97 +134,109 @@ function runHook() { process.exit(0); } - // The branch, SHA, and runs all come from the repo that was pushed, not - // this hook's own working directory. - const repoDir = pushTargetDir(cmd); - if (!repoDir) { - console.error('[ci-status-check] Could not determine which repository this push targeted (e.g. `cd "$VAR"`) — skipping CI verification. Check manually with `gh run list` in that repo.'); - process.exit(0); + // Every push in the command is checked in turn: a chain that pushes two + // repositories starts a run in each. + let status = 0; + for (const push of pushSegments(cmd)) { + status = Math.max(status, verifyPush(push, config)); } + process.exit(status); + }); +} - // The commit and remote branch named by the push's refspec, falling - // back to HEAD and the current branch when none is given. - const ref = pushedRef(firstPushSegment(cmd).normalized, (key) => tryRun('git', ['config', '--get-all', key], repoDir).out.trim()); - if (ref?.skip) { - console.error(`[ci-status-check] Skipping CI verification: ${ref.skip}.`); - process.exit(0); - } +// Waits for and reports the CI run of one pushSegments() entry. Returns the +// exit code it calls for: 0 = passed or skipped, 1 = failed or not found. +function verifyPush(push, config) { + // The branch, SHA, and runs all come from the repo that was pushed, not + // this hook's own working directory. + const repoDir = push.dir; + if (!repoDir) { + console.error('[ci-status-check] Could not determine which repository this push targeted (e.g. `cd "$VAR"`) — skipping CI verification. Check manually with `gh run list` in that repo.'); + return 0; + } - const ghCheck = tryRun('gh', ['auth', 'status'], repoDir); - if (!ghCheck.ok) { - console.error('[ci-status-check] gh CLI not available/authenticated — skipping CI verification. Check manually with `gh run list`.'); - process.exit(0); - } + // The commit and remote branch named by the push's refspec, falling + // back to HEAD and the current branch when none is given. + const ref = pushedRef(push.normalized, (key) => tryRun('git', ['config', '--get-all', key], repoDir).out.trim()); + if (ref?.skip) { + console.error(`[ci-status-check] Skipping CI verification: ${ref.skip}.`); + return 0; + } - const src = ref?.src || 'HEAD'; - const branch = ref && ref.dest !== 'HEAD' - ? ref.dest - : run('git', ['branch', '--show-current'], repoDir).trim(); - const sha = run('git', ['rev-parse', '--verify', '--quiet', `${src}^{commit}`], repoDir).trim(); - if (!branch || !sha) { - if (ref) { - console.error(`[ci-status-check] Could not resolve the pushed ref "${src}" to a commit — skipping CI verification. Check manually with \`gh run list\` in that repo.`); - } - process.exit(0); // not in a git repo, or detached HEAD — nothing to verify - } + const ghCheck = tryRun('gh', ['auth', 'status'], repoDir); + if (!ghCheck.ok) { + console.error('[ci-status-check] gh CLI not available/authenticated — skipping CI verification. Check manually with `gh run list`.'); + return 0; + } - const newRunTimeoutMs = (config.new_run_timeout_seconds ?? 60) * 1000; - const pollIntervalMs = (config.poll_interval_seconds ?? 5) * 1000; - const completionTimeoutMs = (config.completion_timeout_seconds ?? 180) * 1000; - - console.error(`[ci-status-check] Waiting for GitHub Actions run on ${branch}@${sha.slice(0, 7)}...`); - - // Phase 1: wait for a run matching this exact commit to appear at all. - let matchedRun = null; - const phase1Deadline = Date.now() + newRunTimeoutMs; - while (Date.now() < phase1Deadline && !matchedRun) { - const listResult = tryRun( - 'gh', - ['run', 'list', '--branch', branch, '--limit', '5', '--json', 'databaseId,status,conclusion,headSha'], - repoDir - ); - if (listResult.ok) { - try { - const runs = JSON.parse(listResult.out); - matchedRun = runs.find((r) => r.headSha === sha) || null; - } catch { - // malformed JSON — treat as not-found-yet, keep polling - } - } - if (!matchedRun) sleepSync(pollIntervalMs); + const src = ref?.src || 'HEAD'; + const branch = ref && ref.dest !== 'HEAD' + ? ref.dest + : run('git', ['branch', '--show-current'], repoDir).trim(); + const sha = run('git', ['rev-parse', '--verify', '--quiet', `${src}^{commit}`], repoDir).trim(); + if (!branch || !sha) { + if (ref) { + console.error(`[ci-status-check] Could not resolve the pushed ref "${src}" to a commit — skipping CI verification. Check manually with \`gh run list\` in that repo.`); } + return 0; // not in a git repo, or detached HEAD — nothing to verify + } - if (!matchedRun) { - console.error( - `[ci-status-check] No GitHub Actions run found for ${sha.slice(0, 7)} within ${newRunTimeoutMs / 1000}s. This may mean the repo has no workflow watching this branch, or GitHub is slow to register it. You must manually verify with \`gh run list --branch ${branch}\` before reporting success to the user.` - ); - process.exit(1); - } + const newRunTimeoutMs = (config.new_run_timeout_seconds ?? 60) * 1000; + const pollIntervalMs = (config.poll_interval_seconds ?? 5) * 1000; + const completionTimeoutMs = (config.completion_timeout_seconds ?? 180) * 1000; - // Phase 2: `gh run watch` does its own polling internally and exits - // non-zero on a failed conclusion with --exit-status — no need to - // hand-roll a second poll loop. It has no timeout flag of its own, so a - // queued or stuck run would block the session indefinitely; the child is - // killed at completion_timeout_seconds instead. - const watchResult = tryRun('gh', ['run', 'watch', String(matchedRun.databaseId), '--exit-status'], repoDir, { - timeoutMs: completionTimeoutMs, - }); - - if (watchResult.timedOut) { - console.error( - `[ci-status-check] ⏱ CI run ${matchedRun.databaseId} for ${sha.slice(0, 7)} did not finish within ${completionTimeoutMs / 1000}s. Its result is unknown — check \`gh run view ${matchedRun.databaseId}\` before reporting success to the user.` - ); - process.exit(1); - } + console.error(`[ci-status-check] Waiting for GitHub Actions run on ${branch}@${sha.slice(0, 7)}...`); - if (watchResult.ok) { - console.error(`[ci-status-check] ✅ CI PASSED for ${sha.slice(0, 7)} (run ${matchedRun.databaseId}).`); - process.exit(0); + // Phase 1: wait for a run matching this exact commit to appear at all. + let matchedRun = null; + const phase1Deadline = Date.now() + newRunTimeoutMs; + while (Date.now() < phase1Deadline && !matchedRun) { + const listResult = tryRun( + 'gh', + ['run', 'list', '--branch', branch, '--limit', '5', '--json', 'databaseId,status,conclusion,headSha'], + repoDir + ); + if (listResult.ok) { + try { + const runs = JSON.parse(listResult.out); + matchedRun = runs.find((r) => r.headSha === sha) || null; + } catch { + // malformed JSON — treat as not-found-yet, keep polling + } } + if (!matchedRun) sleepSync(pollIntervalMs); + } + if (!matchedRun) { console.error( - `[ci-status-check] ❌ CI FAILED for ${sha.slice(0, 7)} (run ${matchedRun.databaseId}). Run \`gh run view ${matchedRun.databaseId} --log-failed\` to see why before telling the user this succeeded.` + `[ci-status-check] No GitHub Actions run found for ${sha.slice(0, 7)} within ${newRunTimeoutMs / 1000}s. This may mean the repo has no workflow watching this branch, or GitHub is slow to register it. You must manually verify with \`gh run list --branch ${branch}\` before reporting success to the user.` ); - process.exit(1); + return 1; + } + + // Phase 2: `gh run watch` does its own polling internally and exits + // non-zero on a failed conclusion with --exit-status — no need to + // hand-roll a second poll loop. It has no timeout flag of its own, so a + // queued or stuck run would block the session indefinitely; the child is + // killed at completion_timeout_seconds instead. + const watchResult = tryRun('gh', ['run', 'watch', String(matchedRun.databaseId), '--exit-status'], repoDir, { + timeoutMs: completionTimeoutMs, }); + + if (watchResult.timedOut) { + console.error( + `[ci-status-check] ⏱ CI run ${matchedRun.databaseId} for ${sha.slice(0, 7)} did not finish within ${completionTimeoutMs / 1000}s. Its result is unknown — check \`gh run view ${matchedRun.databaseId}\` before reporting success to the user.` + ); + return 1; + } + + if (watchResult.ok) { + console.error(`[ci-status-check] ✅ CI PASSED for ${sha.slice(0, 7)} (run ${matchedRun.databaseId}).`); + return 0; + } + + console.error( + `[ci-status-check] ❌ CI FAILED for ${sha.slice(0, 7)} (run ${matchedRun.databaseId}). Run \`gh run view ${matchedRun.databaseId} --log-failed\` to see why before telling the user this succeeded.` + ); + return 1; } diff --git a/.claude/hooks/check-ci-status-after-push.test.js b/.claude/hooks/check-ci-status-after-push.test.js index e5430b1..4749439 100644 --- a/.claude/hooks/check-ci-status-after-push.test.js +++ b/.claude/hooks/check-ci-status-after-push.test.js @@ -153,4 +153,11 @@ test('hook lifecycle', async (t) => { assert.equal(r.status, 0); assert.match(r.stderr, /Skipping CI verification: the push deletes a remote branch/); }); + + await t.test('every push in a chain is checked, not just the first', () => { + const r = runHook(`git -C "${__dirname}" push origin --delete some-branch; cd "$REPO" && git push`); + assert.equal(r.status, 0); + assert.match(r.stderr, /Could not determine which repository/); + assert.match(r.stderr, /Skipping CI verification: the push deletes a remote branch/); + }); }); diff --git a/.claude/hooks/check-commit-message.test.js b/.claude/hooks/check-commit-message.test.js index 77f3327..fcc1615 100644 --- a/.claude/hooks/check-commit-message.test.js +++ b/.claude/hooks/check-commit-message.test.js @@ -1,6 +1,6 @@ // check-commit-message.test.js // Behavioral tests for the commit-message guard's detection logic — run -// with `node --test .claude/hooks/*.test.js`. +// with `node --test *.test.js *.test.cjs lib/*.test.cjs` from the hooks directory. // // check-commit-message.js only runs its stdin-driven hook lifecycle when // executed directly (`require.main === module`), so requiring it here is diff --git a/.claude/hooks/check-env-guard.js b/.claude/hooks/check-env-guard.js index 0ea9118..d87facd 100644 --- a/.claude/hooks/check-env-guard.js +++ b/.claude/hooks/check-env-guard.js @@ -21,7 +21,9 @@ process.stdin.on('end', () => { } const filePath = input?.tool_input?.file_path || ''; - const basename = path.basename(filePath); + // Lowercased: on case-insensitive file systems (Windows, macOS default) + // `.ENV` opens the same file as `.env`. + const basename = path.basename(filePath).toLowerCase(); if (basename === '.env' || basename.startsWith('.env.')) { const msg = JSON.stringify({ diff --git a/.claude/hooks/check-env-guard.test.js b/.claude/hooks/check-env-guard.test.js new file mode 100644 index 0000000..b9705df --- /dev/null +++ b/.claude/hooks/check-env-guard.test.js @@ -0,0 +1,35 @@ +// check-env-guard.test.js +// Lifecycle tests for check-env-guard.js — run with +// `node --test *.test.js *.test.cjs lib/*.test.cjs` from the hooks directory. + +'use strict'; + +const test = require('node:test'); +const assert = require('node:assert/strict'); +const path = require('node:path'); +const { spawnSync } = require('node:child_process'); + +function readOf(filePath) { + return spawnSync(process.execPath, [path.join(__dirname, 'check-env-guard.js')], { + input: JSON.stringify({ tool_name: 'Read', tool_input: { file_path: filePath } }), + encoding: 'utf8', + }); +} + +test('check-env-guard', async (t) => { + for (const name of ['.env', '.env.production', '.ENV', '.Env.Local']) { + await t.test(`blocks reading ${name}`, () => { + const r = readOf(path.join('project', name)); + assert.equal(r.status, 2, `${name}: ${r.stdout}`); + assert.match(r.stdout, /off-limits/); + }); + } + + for (const name of ['env.js', '.envrc', 'my.env', 'README.md']) { + await t.test(`allows reading ${name}`, () => { + const r = readOf(path.join('project', name)); + assert.equal(r.status, 0, `${name}: ${r.stdout}`); + assert.equal(r.stdout, ''); + }); + } +}); diff --git a/.claude/hooks/check-git-push-audit.js b/.claude/hooks/check-git-push-audit.js index 2a02149..d99aa00 100644 --- a/.claude/hooks/check-git-push-audit.js +++ b/.claude/hooks/check-git-push-audit.js @@ -28,7 +28,7 @@ 'use strict'; const { execSync, execFileSync } = require('child_process'); -const { firstPushSegment, isGitPush, pushArgs, pushTargetDir, splitRefspec } = require('./lib/git-command.cjs'); +const { isGitPush, pushArgs, pushSegments, splitRefspec } = require('./lib/git-command.cjs'); const { loadHookConfig } = require('./lib/hook-config.cjs'); // ── Secret patterns ──────────────────────────────────────────────────────────── @@ -107,7 +107,12 @@ function outgoingRange(segment, repoDir, run = (args) => git(args, repoDir)) { else if (sets.includes('--all') || sets.includes('--branches')) revs.push('--branches'); if (sets.includes('--tags')) revs.push('--tags'); for (const refspec of refspecs) { - const { src } = splitRefspec(refspec); + const { src, dest } = splitRefspec(refspec); + // A bare `:`/`+:` pushes every branch the remote also has. + if (!src && !dest) { + revs.push('--branches'); + continue; + } if (!src) continue; // `:branch` deletes if (!run(['rev-parse', '--verify', '--quiet', `${src}^{commit}`]).ok) return { error: `\`${src}\`` }; revs.push(src); @@ -134,6 +139,19 @@ function outgoingRange(segment, repoDir, run = (args) => git(args, repoDir)) { return { revs, exclude, label: [...revs, ...(named ? ['--not', ...exclude] : [])].join(' ') }; } +// Whether a normalized `git push` segment only deletes remote branches +// (`--delete`/`-d`, or every refspec a `:branch`/`+:branch`) and so sends +// no commits to audit. A bare `:` or `+:` is git's matching-branches +// refspec, not a delete. +function isDeleteOnly(segment) { + const { refspecs, sets, deletes } = pushArgs(segment); + if (deletes) return true; + return sets.length === 0 && refspecs.length > 0 && refspecs.every((r) => { + const { src, dest } = splitRefspec(r); + return !src && dest; + }); +} + // Existence check by exit code only — git's "ambiguous argument" error for a // missing ref (e.g. HEAD~1 on a root commit) writes the ref name itself to // stdout as part of its usage hint, which would make a truthy-stdout check @@ -266,13 +284,18 @@ function runHook() { // rooted elsewhere would otherwise audit the wrong repository. An // unknown or missing target blocks rather than auditing this hook's // directory, which would pass secrets in the real target unseen. - const repoDir = pushTargetDir(cmd); - if (!repoDir) { + // Every push in the command is audited, each in its own repository: a + // chain like `git -C a push && git -C b push` sends both. + // A delete-only push sends no commits, so there's nothing of it to audit. + const pushes = pushSegments(cmd).filter((p) => !isDeleteOnly(p.normalized)); + if (pushes.length === 0) process.exit(0); + if (pushes.some((p) => !p.dir)) { const report = 'PRE-PUSH AUDIT: Could not determine which repository this push targets (e.g. `cd "$VAR"`, or a `git -C` path that does not exist), so its changes cannot be audited. Push with a literal path instead.'; process.stderr.write(report + '\n'); process.stdout.write(JSON.stringify({ continue: false, stopReason: report })); process.exit(2); } + const repoDirs = [...new Set(pushes.map((p) => p.dir))]; // Staged changes and the committed range are combined, not either/or: // the commits are what the push sends, so a benign staged file must not // stop them from being audited. @@ -280,7 +303,7 @@ function runHook() { // A failed git read (an error, or output past maxBuffer) leaves only // partial stdout; auditing that as complete would pass whatever was // cut off, so it blocks instead. - function readGit(args, what) { + function readGit(args, what, repoDir) { const r = git(args, repoDir); if (r.ok) return r.out; const report = `PRE-PUSH AUDIT: Could not read ${what} (\`git ${args.join(' ')}\` failed in ${repoDir}), so they cannot be audited.`; @@ -288,13 +311,15 @@ function runHook() { process.stdout.write(JSON.stringify({ continue: false, stopReason: report })); process.exit(2); } + // Labels name the repository only when the command pushes more than one. + const labelIn = (label, repoDir) => (repoDirs.length > 1 ? `${label} in ${repoDir}` : label); // `git diff` can exit 0 with a blob it needs missing from the object // store (seen with git 2.56 on Linux, where 2.51 on Windows fails), so // the exit code alone can't prove the patch is complete. Every blob the // diff reads is checked first. Submodule entries (mode 160000) name // commits in another repository, so they are skipped. - function blobsMissing(range) { - const raw = readGit(['diff', ...range, '--raw', '--no-abbrev'], 'the changed objects'); + function blobsMissing(range, repoDir) { + const raw = readGit(['diff', ...range, '--raw', '--no-abbrev'], 'the changed objects', repoDir); const oids = []; for (const line of raw.split('\n')) { const m = line.match(/^:(\d+) (\d+) ([0-9a-f]+) ([0-9a-f]+) /); @@ -310,44 +335,47 @@ function runHook() { return true; } } - function addDiff(label, range) { + function addDiff(label, range, repoDir) { const what = `the ${label} changes`; - const files = readGit(['diff', ...range, '--name-only'], what).trim(); + const files = readGit(['diff', ...range, '--name-only'], what, repoDir).trim(); if (!files) return; - if (blobsMissing(range)) { + if (blobsMissing(range, repoDir)) { const report = `PRE-PUSH AUDIT: Could not read ${what} (an object \`git diff ${range.join(' ')}\` needs is missing in ${repoDir}), so they cannot be audited.`; process.stderr.write(report + '\n'); process.stdout.write(JSON.stringify({ continue: false, stopReason: report })); process.exit(2); } - parts.push({ label, files, text: readGit(['diff', ...range], what) }); + parts.push({ label: labelIn(label, repoDir), files, text: readGit(['diff', ...range], what, repoDir) }); } - addDiff('staged', ['--staged']); - // Every commit the push would send, each audited by its own patch rather - // than one net diff: a secret added in one commit and deleted in a later - // one is still sent in history. See outgoingRange() for which commits - // those are. Merges show their changes against the first parent, so a - // conflict resolution can't add a line unseen. - const range = outgoingRange(firstPushSegment(cmd).normalized, repoDir); - if (range.error) { - const report = `PRE-PUSH AUDIT: Could not resolve ${range.error} to a commit in ${repoDir}, so the commits this push sends cannot be audited. Push a literal, existing branch or ref instead.`; - process.stderr.write(report + '\n'); - process.stdout.write(JSON.stringify({ continue: false, stopReason: report })); - process.exit(2); - } - const logArgs = ['log', '--root', '--diff-merges=first-parent', '--format=', ...range.revs, ...(range.exclude.length > 0 ? ['--not', ...range.exclude] : [])]; - const readLog = (flag) => readGit(['log', flag, ...logArgs.slice(1)], 'the commits this push sends'); - const outgoingFiles = range.revs.length > 0 ? readLog('--name-only').trim() : ''; - if (outgoingFiles) { - parts.push({ label: `unpushed commits (${range.label})`, files: outgoingFiles, text: readLog('-p') }); - } else { - // Nothing outgoing (e.g. pushing an already-pushed commit to a new - // branch name): the last commit, as a best effort. HEAD~1 doesn't - // exist for a root commit, so diff that against git's empty tree. - const EMPTY_TREE = '4b825dc642cb6eb9a060e54bf8d69288fbee4904'; - const hasParent = refExists('HEAD~1', repoDir); - addDiff(hasParent ? 'HEAD~1..HEAD' : 'root commit (vs. empty tree)', [hasParent ? 'HEAD~1' : EMPTY_TREE, 'HEAD']); + for (const repoDir of repoDirs) addDiff('staged', ['--staged'], repoDir); + for (const push of pushes) { + const repoDir = push.dir; + // Every commit the push would send, each audited by its own patch + // rather than one net diff: a secret added in one commit and deleted + // in a later one is still sent in history. See outgoingRange() for + // which commits those are. Merges show their changes against the + // first parent, so a conflict resolution can't add a line unseen. + const range = outgoingRange(push.normalized, repoDir); + if (range.error) { + const report = `PRE-PUSH AUDIT: Could not resolve ${range.error} to a commit in ${repoDir}, so the commits this push sends cannot be audited. Push a literal, existing branch or ref instead.`; + process.stderr.write(report + '\n'); + process.stdout.write(JSON.stringify({ continue: false, stopReason: report })); + process.exit(2); + } + const logArgs = ['--root', '--diff-merges=first-parent', '--format=', ...range.revs, ...(range.exclude.length > 0 ? ['--not', ...range.exclude] : [])]; + const readLog = (flag) => readGit(['log', flag, ...logArgs], 'the commits this push sends', repoDir); + const outgoingFiles = range.revs.length > 0 ? readLog('--name-only').trim() : ''; + if (outgoingFiles) { + parts.push({ label: labelIn(`unpushed commits (${range.label})`, repoDir), files: outgoingFiles, text: readLog('-p') }); + } else { + // Nothing outgoing (e.g. pushing an already-pushed commit to a new + // branch name): the last commit, as a best effort. HEAD~1 doesn't + // exist for a root commit, so diff that against git's empty tree. + const EMPTY_TREE = '4b825dc642cb6eb9a060e54bf8d69288fbee4904'; + const hasParent = refExists('HEAD~1', repoDir); + addDiff(hasParent ? 'HEAD~1..HEAD' : 'root commit (vs. empty tree)', [hasParent ? 'HEAD~1' : EMPTY_TREE, 'HEAD'], repoDir); + } } const diffText = parts.map((p) => p.text).join('\n'); @@ -447,7 +475,7 @@ function runHook() { const report = [ `PRE-PUSH AUDIT — ${verdict}`, - `Audited: ${diffSource} in ${repoDir} (${fileList.length} file(s) changed)`, + `Audited: ${diffSource} in ${repoDirs.join(', ')} (${fileList.length} file(s) changed)`, '', findings.length > 0 ? rows : ' No findings.', '', diff --git a/.claude/hooks/check-git-push-audit.test.js b/.claude/hooks/check-git-push-audit.test.js index a506da6..abb2133 100644 --- a/.claude/hooks/check-git-push-audit.test.js +++ b/.claude/hooks/check-git-push-audit.test.js @@ -126,6 +126,61 @@ test('hook lifecycle', async (t) => { assert.match(r.stderr, /Could not determine which repository/); }); + await t.test('every push in a chain is audited, not just the first', () => { + const clean = fs.mkdtempSync(path.join(os.tmpdir(), 'push-audit-clean-')); + try { + const cleanGit = (...args) => execFileSync('git', args, { cwd: clean, stdio: 'pipe' }); + cleanGit('init', '-q', '-b', 'main'); + cleanGit('config', 'user.email', 'test@example.com'); + cleanGit('config', 'user.name', 'Test'); + fs.writeFileSync(path.join(clean, 'a.txt'), 'benign\n'); + cleanGit('add', '.'); + cleanGit('commit', '-q', '-m', 'benign'); + const r = run(`git -C "${clean}" push origin main && git -C "${repo}" push origin main`); + assert.equal(r.status, 2, r.stderr); + assert.match(r.stderr, /hardcoded credential/); + assert.ok(r.stderr.includes(clean) && r.stderr.includes(repo), r.stderr); + } finally { + fs.rmSync(clean, { recursive: true, force: true }); + } + }); + + await t.test('an undeterminable later push in a chain is blocked', () => { + const r = run(`git -C "${repo}" push origin main && cd "$REPO" && git push origin main`); + assert.equal(r.status, 2); + assert.match(r.stderr, /Could not determine which repository/); + }); + + await t.test('a delete-only push sends no commits, so nothing is audited', () => { + for (const command of [ + `git -C "${repo}" push origin --delete feature`, + `git -C "${repo}" push origin :feature`, + `git -C "${repo}" push origin +:feature`, + ]) { + const r = run(command); + assert.equal(r.status, 0, `${command}\n${r.stderr}`); + } + }); + + await t.test('a matching-branches refspec (: or +:) is audited, not treated as a delete', () => { + for (const command of [`git -C "${repo}" push origin :`, `git -C "${repo}" push origin +:`]) { + const r = run(command); + assert.equal(r.status, 2, `${command}\n${r.stderr}`); + assert.match(r.stderr, /hardcoded credential/); + } + }); + + await t.test('a delete-only push in an unknown repo is not blocked', () => { + const r = run('cd "$REPO" && git push origin --delete feature'); + assert.equal(r.status, 0, r.stderr); + }); + + await t.test('a push that sends commits is still audited when chained with a delete', () => { + const r = run(`git -C "${repo}" push origin --delete feature && git -C "${repo}" push origin main`); + assert.equal(r.status, 2); + assert.match(r.stderr, /hardcoded credential/); + }); + // Last: it adds an upstream and a staged file to the shared repo. await t.test('a staged file does not stop the outgoing commits from being audited', () => { git('remote', 'add', 'origin', remote); diff --git a/.claude/hooks/check-merge-guard.hook.test.js b/.claude/hooks/check-merge-guard.hook.test.js new file mode 100644 index 0000000..00be5c5 --- /dev/null +++ b/.claude/hooks/check-merge-guard.hook.test.js @@ -0,0 +1,83 @@ +// check-merge-guard.hook.test.js +// Runtime tests: run check-merge-guard.js as Claude Code does (JSON on stdin) +// and assert on the exit code, so a wrapper form the parser fails to see +// through shows up as a hook that lets the command run. Exit 2 blocks. +// +// Run with `node --test *.test.js *.test.cjs lib/*.test.cjs` from the hooks directory. + +'use strict'; + +const test = require('node:test'); +const assert = require('node:assert/strict'); +const os = require('node:os'); +const path = require('node:path'); +const { spawnSync } = require('node:child_process'); + +const HOOK = path.join(__dirname, 'check-merge-guard.js'); + +function runHook(command) { + const cwd = os.tmpdir(); + const result = spawnSync(process.execPath, [HOOK], { + input: JSON.stringify({ tool_name: 'Bash', tool_input: { command }, cwd }), + cwd, + encoding: 'utf8', + timeout: 15000, + }); + return result.status; +} + +function assertBlocked(command, why) { + assert.equal( + runHook(command), + 2, + `the hook should exit 2 (block) for ${JSON.stringify(command)}. ${why} ` + + 'Any other exit code lets the command run unguarded.' + ); +} + +function assertAllowed(command, why) { + assert.equal( + runHook(command), + 0, + `the hook should exit 0 (allow) for ${JSON.stringify(command)}. ${why}` + ); +} + +test('check-merge-guard hook, eval and xargs wrappers', async (t) => { + await t.test('xargs -- (end of options) before a guarded command', () => { + assertBlocked('xargs -- gh pr merge 27', 'after -- the next word is the command.'); + assertBlocked('xargs -r -- gh pr merge 27', 'options before -- must not hide the command.'); + assertBlocked('xargs -- git push origin main', 'a push to main must be blocked through xargs --.'); + assertBlocked('echo 27 | xargs -- gh pr merge', 'the piped form must be blocked too.'); + }); + + await t.test('xargs with option clusters and placeholders', () => { + assertBlocked('echo 27 | xargs -rt gh pr merge', 'a clustered -rt must not hide the command.'); + assertBlocked('echo 27 | xargs -I{} gh pr merge {}', 'an attached -I{} must not hide the command.'); + assertBlocked('echo 27 | xargs -rn1 gh pr merge', 'a -rn1 cluster must not hide the command.'); + assertBlocked('xargs -rt git push origin main', 'a clustered -rt must not hide a push to main.'); + }); + + await t.test('eval around a guarded command', () => { + assertBlocked('eval "gh pr merge 27"', 'eval runs its argument as a command line.'); + assertBlocked('eval -- "gh pr merge 27"', 'eval accepts -- before its argument.'); + assertBlocked('eval gh pr merge 27', 'unquoted eval arguments are joined and run.'); + assertBlocked('eval "git push origin main"', 'a push to main must be blocked through eval.'); + }); + + await t.test('wrappers nested in other wrappers', () => { + assertBlocked('bash -c "echo 27 | xargs -- gh pr merge"', 'bash -c around xargs.'); + assertBlocked('eval "echo 27 | xargs -rt gh pr merge"', 'eval around xargs.'); + }); + + await t.test('a very long eval chain still blocks instead of crashing', () => { + assertBlocked(`${'eval '.repeat(20000)}gh pr merge 27`, + 'a stack overflow in the parser would exit 1 and fail open.'); + }); + + await t.test('wrappers around harmless commands are allowed', () => { + assertAllowed('xargs -- echo gh pr merge 27', 'the inner command is echo.'); + assertAllowed('eval "echo gh pr merge 27"', 'the inner command is echo.'); + assertAllowed('echo 27 | xargs -rt echo done', 'nothing guarded runs.'); + }); +}); diff --git a/.claude/hooks/check-merge-guard.js b/.claude/hooks/check-merge-guard.js index bba4a66..91ed1e5 100644 --- a/.claude/hooks/check-merge-guard.js +++ b/.claude/hooks/check-merge-guard.js @@ -40,7 +40,7 @@ const { execSync } = require('child_process'); const { loadHookConfig } = require('./lib/hook-config.cjs'); -const { commandSegments, stripSurroundingQuotes } = require('./lib/git-command.cjs'); +const { commandSegments, pushArgs, splitRefspec, stripSurroundingQuotes } = require('./lib/git-command.cjs'); const PROTECTED_BRANCHES = ['master', 'main']; @@ -94,73 +94,38 @@ function checkoutTarget(segment) { // chain. return raw ? stripSurroundingQuotes(raw) : null; } + // `git checkout [] -- ` and `git checkout .` restore files + // and leave HEAD where it was, so they aren't a move to another branch. + if (tokens.includes('--')) return null; const raw = tokens.find((t) => !t.startsWith('-')); - return raw ? stripSurroundingQuotes(raw) : null; + const target = raw ? stripSurroundingQuotes(raw) : null; + return target === '.' ? null : target; } -// A refspec token is `:` with no spaces, and a bare branch-name -// token (no colon) pushes a local branch of that name to a remote branch -// of the same name. Once flags are filtered out, every remaining non-flag -// token is checked as a candidate — not just tokens assumed to follow a -// remote — for two reasons found in review: -// 1. A rigid `git push ` shape (nothing else) missed -// `git push --force origin feature:main` and `git push origin -// --force feature:main` entirely, since a flag anywhere breaks a -// fixed-position match. Tokenizing and filtering flags, the same -// approach check-branch-delete-guard.js uses for --delete, closes -// that gap regardless of flag position. -// 2. Requiring at least a remote + one more token missed both -// `git push feature:main` (remote omitted — git still resolves this -// via config in some setups) and, more importantly, a *plain* -// `git push origin master` with no colon at all, which lands commits -// on master just as directly as any refspec does. Checking every -// non-flag token — including a lone one — for either a colon-refspec -// or an exact match against a protected branch name closes both, -// erring toward blocking rather than trying to perfectly replicate -// git's own remote-vs-refspec argument resolution. +// The push's arguments come from the shared pushArgs() (lib/git-command.cjs): +// flags in any position and the values of flags such as `-o` are skipped, +// quotes are removed, and a `#` comment or a redirection (`>/dev/null`, +// `2>&1`) is not an argument. Every refspec is checked, not just one in a +// fixed position. splitRefspec() drops a leading `+` (a per-refspec force) +// and gives an empty dest (`branch:`) the src's name, as git does. A bare +// branch name (`git push origin master`) pushes to the remote branch of the +// same name, and `refs/heads/main` is the same branch as `main`. No +// `src !== dest` requirement: `main:main` lands on main like any other. +// +// A lone colon refspec in the remote position (`git push feature:main`) +// is checked too, erring toward blocking rather than replicating git's +// own remote-vs-refspec resolution. A lone bare name (`git push master`) +// is the repository argument, not a branch, so it isn't. function protectedRefspecPush(segment) { if (!/^git\s+push\b/.test(segment)) return null; - const tokens = segment.split(/\s+/); // tokens[0] === 'git', tokens[1] === 'push' - const nonFlags = tokens.slice(2).filter((t) => !t.startsWith('-')); - - for (const rawToken of nonFlags) { - // This hook sees the raw command string, quotes and all — the shell - // hasn't stripped them yet the way it would before argv reaches git - // itself. `git push origin "main"` or `git push origin "feature:main"` - // would otherwise leave a trailing quote character on the token - // (`main"`), which never matches PROTECTED_BRANCHES even though the - // quoting changes nothing about what actually gets pushed. - const token = stripSurroundingQuotes(rawToken); - const colonIdx = token.indexOf(':'); - if (colonIdx !== -1) { - let src = token.slice(0, colonIdx); - // An empty dest (`git push origin branch:`) means "push to a remote - // ref with the same name as src" — git's own default-dest behavior - // for a trailing-colon refspec. - let dest = token.slice(colonIdx + 1) || src; - src = src.replace(/^refs\/heads\//, ''); - dest = dest.replace(/^refs\/heads\//, ''); - // No `src !== dest` requirement — `git push origin main:main` (or - // the trailing-colon form `main:`) lands commits on main exactly as - // directly as any other refspec targeting it, so there's no case - // where src and dest being equal makes this any less a direct push - // to a protected branch. - if (PROTECTED_BRANCHES.includes(dest)) { - return { src, dest }; - } - continue; - } - - // A bare protected-branch-name token only counts once at least a - // remote/repository has also been given (`git push origin master`) — - // a single lone token (`git push master`) is what git itself treats - // as the repository argument, not a branch, so there's no push of - // anything named master/main to react to yet. A fully-qualified - // `refs/heads/main` is the same branch. - const branch = token.replace(/^refs\/heads\//, ''); - if (nonFlags.length >= 2 && PROTECTED_BRANCHES.includes(branch)) { - return { src: branch, dest: branch }; + const { remote, refspecs } = pushArgs(segment); + const candidates = remote && remote.includes(':') ? [remote, ...refspecs] : refspecs; + for (const token of candidates) { + const { src, dest } = splitRefspec(token); + const target = dest.replace(/^refs\/heads\//, ''); + if (PROTECTED_BRANCHES.includes(target)) { + return { src: src.replace(/^refs\/heads\//, ''), dest: target }; } } return null; @@ -180,19 +145,36 @@ function findMergeReason(fullCmd, getCurrentBranch = currentBranch) { const segments = parsed.map((s) => s.raw); const normalized = parsed.map((s) => s.normalized); - // Resolved once per command and reused by both the push and merge - // checks below: a branch this command chain explicitly checks out to + // Reused by both the push and merge checks below: the branch this command + // chain last checked out in a given directory before segment i // (textually — the actual HEAD hasn't moved yet when this hook runs), - // falling back to whatever branch is actually currently checked out. - // Lazy since a real `git rev-parse` call is only worth paying for when - // nothing in the command already answers the question textually. - const checkedOutTo = normalized - .map(checkoutTarget) - .find((target) => target && PROTECTED_BRANCHES.includes(target)); - // Cached per directory: `git -C a push && git -C b merge x` asks two repos. + // falling back to whatever branch is actually checked out there. Keyed + // by directory, so `git -C a checkout main && git -C b push` judges b by + // its own branch, and in order, so `checkout main && checkout feature && + // push` pushes feature. A protected checkout in a directory that can't + // be known (null) might be any repository, so it counts for every + // directory from then on. + const lastCheckout = new Map(); + let unknownProtected = null; + const checkoutsBefore = parsed.map((s, i) => { + const before = { byDir: new Map(lastCheckout), unknownProtected }; + const target = checkoutTarget(normalized[i]); + if (target && s.cwd !== null) lastCheckout.set(s.cwd, target); + if (target && s.cwd === null && PROTECTED_BRANCHES.includes(target)) unknownProtected = target; + return before; + }); + function checkedOutTo(i, dir) { + const { byDir, unknownProtected: unknown } = checkoutsBefore[i]; + const own = dir === null ? undefined : byDir.get(dir); + return own && PROTECTED_BRANCHES.includes(own) ? own : unknown ?? own; + } + // Lazy and cached per directory: a real `git rev-parse` call is only + // worth paying for when nothing in the command already answers the + // question textually, and `git -C a push && git -C b merge x` asks two repos. const branchByDir = new Map(); - function effectiveBranch(dir) { - if (checkedOutTo) return checkedOutTo; + function effectiveBranch(i, dir) { + const target = checkedOutTo(i, dir); + if (target) return target; if (!branchByDir.has(dir)) branchByDir.set(dir, getCurrentBranch(dir)); return branchByDir.get(dir); } @@ -231,31 +213,30 @@ function findMergeReason(fullCmd, getCurrentBranch = currentBranch) { // `git switch main && git push origin`. protectedRefspecPush() only // catches an explicit mention of a protected branch, so this implicit // form landed commits on master/main completely undetected. Only - // applies when nothing explicit was given (nonFlags.length <= 1): + // applies when nothing explicit was given (no refspecs): // an explicit branch/refspec naming a *different* branch (e.g. // `git push origin some-other-branch` while sitting on master) is a // deliberate, legitimate push of something else and shouldn't be // swept in just because HEAD happens to be on a protected branch. if (/^git\s+push\b/.test(segment)) { - const tokens = segment.split(/\s+/); - const rest = tokens.slice(2); - // --all/--mirror push every local branch (or the whole repo, - // including local master/main if either exists) to the remote, + const { refspecs, sets } = pushArgs(segment); + // --all/--branches/--mirror push every local branch (or the whole + // repo, including local master/main if either exists) to the remote, // regardless of which branch is currently checked out — unlike an // ordinary implicit push, this isn't gated on the current branch // being protected at all, since it can land commits on master/main // even when run from an unrelated feature branch. Blocked - // unconditionally whenever either flag is present, matching this + // unconditionally whenever any of them is present, matching this // hook's stated "err toward blocking" direction. - if (rest.includes('--all') || rest.includes('--mirror')) { - return `push includes --all/--mirror, which can update protected branches regardless of the current branch — matched: ${segments[i]}`; + if (sets.some((flag) => flag !== '--tags')) { + return `push includes ${sets.filter((flag) => flag !== '--tags').join('/')}, which can update protected branches regardless of the current branch — matched: ${segments[i]}`; } - const nonFlags = rest.filter((t) => !t.startsWith('-')); - if (nonFlags.length <= 1) { - if (parsed[i].cwd === null && !checkedOutTo) { + // `--tags` with no refspec pushes only tags, not the current branch. + if (refspecs.length === 0 && sets.length === 0) { + if (parsed[i].cwd === null && !checkedOutTo(i, null)) { return `implicit push (no explicit branch/refspec given) in a repository this guard can't determine (e.g. \`cd "$VAR"\`), so its current branch can't be checked — name the branch or use a literal path — matched: ${segments[i]}`; } - const branch = effectiveBranch(parsed[i].cwd); + const branch = effectiveBranch(i, parsed[i].cwd); if (branch && PROTECTED_BRANCHES.includes(branch)) { return `implicit push (no explicit branch/refspec given) while on protected branch "${branch}" — matched: ${segments[i]}`; } @@ -267,10 +248,10 @@ function findMergeReason(fullCmd, getCurrentBranch = currentBranch) { // protected branch. for (let i = 0; i < normalized.length; i++) { if (!/^git\s+merge\b/.test(normalized[i])) continue; - if (parsed[i].cwd === null && !checkedOutTo) { + if (parsed[i].cwd === null && !checkedOutTo(i, null)) { return `local \`git merge\` in a repository this guard can't determine (e.g. \`cd "$VAR"\`), so its current branch can't be checked — use a literal path`; } - const branch = effectiveBranch(parsed[i].cwd); + const branch = effectiveBranch(i, parsed[i].cwd); if (branch && PROTECTED_BRANCHES.includes(branch)) { return `local \`git merge\` on protected branch "${branch}"`; } diff --git a/.claude/hooks/check-merge-guard.test.js b/.claude/hooks/check-merge-guard.test.js index 7f19db4..8571084 100644 --- a/.claude/hooks/check-merge-guard.test.js +++ b/.claude/hooks/check-merge-guard.test.js @@ -428,3 +428,67 @@ test('findMergeReason', async (t) => { assert.equal(blocked('(cd repo && git push origin main)', onFeatureBranch), true); }); }); + +test('PR #72 review findings', async (t) => { + await t.test('a force-prefixed refspec to a protected branch is blocked', () => { + for (const cmd of [ + 'git push origin +master', + 'git push origin +refs/heads/master', + 'git push origin +feature:main', + 'git push origin +feature:refs/heads/main', + ]) { + assert.equal(blocked(cmd, onFeatureBranch), true, + `${cmd} force-pushes to a protected branch; the leading + only adds --force for that refspec.`); + } + }); + + await t.test('a comment or redirection is not a refspec', () => { + for (const cmd of [ + 'git push origin # pushes the current branch', + 'git push origin >/dev/null', + 'git push origin > out.log', + 'git push origin 2>&1', + 'git push origin 2>/dev/null', + ]) { + assert.equal(blocked(cmd, onMaster), true, + `${cmd} is an implicit push of the current branch (master); the comment/redirection is not a branch name.`); + } + assert.equal(blocked('git push origin feature # not main', onMaster), false, + 'an explicit non-protected branch is still allowed when the comment mentions main.'); + }); + + await t.test('a checkout in one repo does not decide the branch in another', () => { + const branchIn = (dir) => (dir && dir.endsWith('b') ? 'fix/unrelated-feature' : 'master'); + assert.equal(blocked('git -C a checkout main && git -C b push', branchIn), false, + 'repo b is on a feature branch; the checkout of main happened in repo a.'); + assert.equal(blocked('git -C a checkout main && git -C a push', onFeatureBranch), true, + 'repo a is checked out to main before its implicit push.'); + assert.equal(blocked('git -C a checkout main && git -C b merge x', branchIn), false, + 'the merge runs in repo b, which is on a feature branch.'); + }); +}); + +test('PR #17 review findings', async (t) => { + await t.test('the latest checkout in a repo decides its branch, not the first', () => { + assert.equal(blocked('git -C a checkout main && git -C a checkout feature && git -C a push', onFeatureBranch), false, + 'repo a ends on feature before its implicit push.'); + assert.equal(blocked('git -C a checkout feature && git -C a checkout main && git -C a push', onFeatureBranch), true, + 'repo a ends on main before its implicit push.'); + assert.equal(blocked('git checkout main && git push && git checkout feature', onFeatureBranch), true, + 'the push runs while on main; a later checkout does not change that.'); + }); + + await t.test('a tag-only push from a protected branch is allowed', () => { + assert.equal(blocked('git push origin --tags', onMaster), false, + '--tags with no refspec pushes only tags, not the current branch.'); + assert.equal(blocked('git push origin --tags main', onFeatureBranch), true, + 'an explicit protected refspec alongside --tags is still blocked.'); + }); + + await t.test('a file checkout does not move off a protected branch', () => { + assert.equal(blocked('git checkout main && git checkout -- app.js && git push', onFeatureBranch), true, + '`git checkout -- ` restores a file and stays on main.'); + assert.equal(blocked('git checkout main && git checkout . && git push', onFeatureBranch), true, + '`git checkout .` restores files and stays on main.'); + }); +}); diff --git a/.claude/hooks/check-script-exists.mjs b/.claude/hooks/check-script-exists.mjs index a681429..ff755f9 100644 --- a/.claude/hooks/check-script-exists.mjs +++ b/.claude/hooks/check-script-exists.mjs @@ -27,9 +27,11 @@ import { fileURLToPath } from 'url'; import { createRequire } from 'module'; const SCRIPT_DIR = dirname(fileURLToPath(import.meta.url)); -const { loadHookConfig } = createRequire(import.meta.url)('./lib/hook-config.cjs'); +const { loadHookConfig, resolveProjectDir } = createRequire(import.meta.url)('./lib/hook-config.cjs'); const CONFIG_PATH = resolve(SCRIPT_DIR, 'config.json'); -const CWD = process.cwd(); +// package_json_paths are relative to the project (CLAUDE_PROJECT_DIR), not +// to whatever directory the hook happens to be started in. +const PROJECT_DIR = resolveProjectDir(); function log(msg) { process.stderr.write(`[script-guard] ${msg}\n`); @@ -56,7 +58,7 @@ const { // ── Load all package.json scripts from configured paths ─────────────────── const availableScripts = new Set(); for (const pkgPath of package_json_paths) { - const fullPath = resolve(CWD, pkgPath); + const fullPath = resolve(PROJECT_DIR, pkgPath); if (!existsSync(fullPath)) continue; try { const pkg = JSON.parse(readFileSync(fullPath, 'utf8')); @@ -134,8 +136,9 @@ process.stdin.on('end', () => { process.stderr.write(` Available: ${[...availableScripts].sort().join(', ') || '(none found)'}\n`); process.stderr.write(` Check for a typo or add the script to package.json before running.\n\n`); + // Exit 2 is what blocks a PreToolUse call; exit 1 only reports. if (on_missing === 'block') { - process.exit(1); + process.exit(2); } } diff --git a/.claude/hooks/check-script-exists.test.js b/.claude/hooks/check-script-exists.test.js new file mode 100644 index 0000000..a7460a3 --- /dev/null +++ b/.claude/hooks/check-script-exists.test.js @@ -0,0 +1,61 @@ +// check-script-exists.test.js +// Lifecycle tests for check-script-exists.mjs — run with +// `node --test *.test.js *.test.cjs lib/*.test.cjs` from the hooks directory. +// +// The hook runs on stdin JSON as Claude Code invokes it, from a working +// directory outside the project, with CLAUDE_PROJECT_DIR naming the project. + +'use strict'; + +const test = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const os = require('node:os'); +const path = require('node:path'); +const { spawnSync } = require('node:child_process'); + +test('hook lifecycle', async (t) => { + const projectDir = fs.mkdtempSync(path.join(os.tmpdir(), 'script-guard-proj-')); + const elsewhere = fs.mkdtempSync(path.join(os.tmpdir(), 'script-guard-cwd-')); + fs.mkdirSync(path.join(projectDir, '.claude')); + fs.writeFileSync(path.join(projectDir, 'package.json'), JSON.stringify({ scripts: { build: 'echo build' } })); + + const run = (command, scriptGuard = {}) => { + fs.writeFileSync( + path.join(projectDir, '.claude', 'hooks.config.json'), + JSON.stringify({ script_guard: { enabled: true, ...scriptGuard } }) + ); + const r = spawnSync(process.execPath, [path.join(__dirname, 'check-script-exists.mjs')], { + input: JSON.stringify({ tool_name: 'Bash', tool_input: { command } }), + cwd: elsewhere, + env: { ...process.env, CLAUDE_PROJECT_DIR: projectDir }, + encoding: 'utf8', + timeout: 30000, + }); + return { status: r.status, stderr: r.stderr || '' }; + }; + + try { + await t.test('package_json_paths resolve against the project dir, not the working directory', () => { + const r = run('npm run nope'); + assert.equal(r.status, 0); + assert.match(r.stderr, /Script "nope" not found/); + assert.match(r.stderr, /Available: build/); + }); + + await t.test('a script that exists in the project passes silently', () => { + const r = run('npm run build'); + assert.equal(r.status, 0); + assert.doesNotMatch(r.stderr, /not found/); + }); + + await t.test('on_missing "block" blocks the command (exit 2)', () => { + const r = run('npm run nope', { on_missing: 'block' }); + assert.equal(r.status, 2); + assert.match(r.stderr, /Script "nope" not found/); + }); + } finally { + fs.rmSync(projectDir, { recursive: true, force: true }); + fs.rmSync(elsewhere, { recursive: true, force: true }); + } +}); diff --git a/.claude/hooks/check-search-command.js b/.claude/hooks/check-search-command.js index fd51cda..077c5a8 100644 --- a/.claude/hooks/check-search-command.js +++ b/.claude/hooks/check-search-command.js @@ -23,7 +23,7 @@ */ const { loadHookConfig } = require('./lib/hook-config.cjs'); -const { commandSegments, stripSurroundingQuotes } = require('./lib/git-command.cjs'); +const { commandSegments, unquotedWords } = require('./lib/git-command.cjs'); const { resolve } = require('path'); const CONFIG_PATH = resolve(__dirname, 'config.json'); @@ -56,7 +56,11 @@ function rawSearchCommands(command, commands = DEFAULT_COMMANDS) { const hits = []; for (const { normalized } of commandSegments(command)) { - const head = stripSurroundingQuotes(normalized.split(/\s+/)[0]); + // A quoted executable path may contain spaces, so it's split as the + // shell would; an unquoted one keeps its backslashes (a Windows path). + const head = /^["']/.test(normalized) + ? (unquotedWords(normalized) || [])[0] + : normalized.split(/\s+/)[0]; if (!head) continue; const basename = head.split(/[\\/]/).pop().toLowerCase().replace(/\.exe$/, ''); diff --git a/.claude/hooks/check-search-command.test.js b/.claude/hooks/check-search-command.test.js index e3738d7..9f3c4e2 100644 --- a/.claude/hooks/check-search-command.test.js +++ b/.claude/hooks/check-search-command.test.js @@ -44,6 +44,12 @@ test('rawSearchCommands', async (t) => { assert.deepEqual(rawSearchCommands("echo '$(tail -1 f)'"), []); }); + await t.test('a quoted executable path containing spaces is detected', () => { + assert.deepEqual(rawSearchCommands('"C:\\Program Files\\Git\\usr\\bin\\grep.exe" -rn foo src'), ['grep']); + assert.deepEqual(rawSearchCommands("'/opt/my tools/bin/cat' file.txt"), ['cat']); + assert.deepEqual(rawSearchCommands('C:\\Git\\usr\\bin\\tail.exe -5 f'), ['tail']); + }); + await t.test('unwatched commands are not reported', () => { assert.deepEqual(rawSearchCommands('npm run build'), []); }); diff --git a/.claude/hooks/config.json b/.claude/hooks/config.json index 64a9f08..6aa3832 100644 --- a/.claude/hooks/config.json +++ b/.claude/hooks/config.json @@ -121,14 +121,14 @@ "package_json_paths": ["package.json"], "_package_json_paths_note": "List of package.json files to check for script existence. Extend for monorepos.", "on_missing": "warn", - "_on_missing_note": "'warn' prints to stderr (exit 0) | 'block' exits 1 and prevents the command" + "_on_missing_note": "'warn' prints to stderr (exit 0) | 'block' exits 2 and prevents the command" }, "commit_guard": { - "enabled": false, + "enabled": true, "_comment": "Warn or block git commit commands that do not include WHY and EXPECTED sections in the message.", "required_markers": ["WHY:", "EXPECTED:"], - "on_violation": "warn", + "on_violation": "block", "_on_violation_note": "warn prints to stderr (exit 0) | block exits 2 and rejects the tool call" }, diff --git a/.claude/hooks/hook-config-integration.test.cjs b/.claude/hooks/hook-config-integration.test.cjs index f8ea2e6..e41386b 100644 --- a/.claude/hooks/hook-config-integration.test.cjs +++ b/.claude/hooks/hook-config-integration.test.cjs @@ -455,6 +455,13 @@ test('check-git-push-audit.js reads the project override (README: push_audit)', test('check-commit-message.js reads the project override (README: commit_guard)', async (t) => { const BAD = bash('git commit -m "no sections"'); + await t.test('no override: the shared default blocks', () => { + withProject(undefined, (p, c) => { + const r = runHook('check-commit-message.js', p, c, BAD); + assert.equal(r.status, 2, msg('commit guard default', 'commit_guard defaults to enabled + block', 'exit 2', `${r.status} ${r.stderr}`, 'A registered guard must keep blocking unless a project opts out.')); + }); + }); + await t.test('override enabled + on_violation=block: blocks', () => { withProject({ commit_guard: { enabled: true, on_violation: 'block' } }, (p, c) => { const r = runHook('check-commit-message.js', p, c, BAD); diff --git a/.claude/hooks/lib/git-command-wrappers.test.cjs b/.claude/hooks/lib/git-command-wrappers.test.cjs new file mode 100644 index 0000000..7ea1364 --- /dev/null +++ b/.claude/hooks/lib/git-command-wrappers.test.cjs @@ -0,0 +1,541 @@ +// git-command-wrappers.test.cjs +// Behavioral tests for the `eval` and `xargs` wrapper handling in +// git-command.cjs — run with `node --test lib/*.test.cjs` from the hooks +// directory. +// +// These are security guards (they block `git push`, `gh pr merge`, branch +// deletes). A wrapper form that is not unwrapped is a silent bypass, so every +// assertion message names WHAT was expected, the INPUT, and WHY it matters. + +'use strict'; + +const test = require('node:test'); +const assert = require('node:assert/strict'); +const os = require('node:os'); +const path = require('node:path'); +const { commandSegments, firstPushSegment, isGitPush } = require('./git-command.cjs'); + +const START = path.join(os.tmpdir(), 'wrapper-tests-start'); + +function normalizedList(cmd) { + return commandSegments(cmd, START).map((s) => s.normalized); +} + +function expectPush(cmd, expected, why) { + assert.equal( + isGitPush(cmd), + true, + `isGitPush should be true for input ${JSON.stringify(cmd)}. ${why} ` + + 'Missing it lets a push run unguarded.' + ); + const seg = firstPushSegment(cmd, START); + assert.ok( + seg, + `firstPushSegment should return a segment for input ${JSON.stringify(cmd)} ` + + `but returned ${seg}. segments=${JSON.stringify(normalizedList(cmd))}` + ); + if (expected !== undefined) { + assert.equal( + seg.normalized, + expected, + `firstPushSegment(${JSON.stringify(cmd)}).normalized should be ` + + `${JSON.stringify(expected)}: wrapper and its own options must be dropped, ` + + 'the inner command and its fixed args kept verbatim, so downstream guards ' + + 'can inspect the real refspec.' + ); + } +} + +function expectNoPush(cmd, why) { + assert.equal( + isGitPush(cmd), + false, + `isGitPush should be false for input ${JSON.stringify(cmd)}. ${why} ` + + `segments=${JSON.stringify(normalizedList(cmd))}` + ); + assert.equal( + firstPushSegment(cmd, START), + undefined, + `firstPushSegment should be undefined for input ${JSON.stringify(cmd)}. ${why}` + ); +} + +function expectSegment(cmd, expected, why) { + const list = normalizedList(cmd); + assert.ok( + list.includes(expected), + `commandSegments(${JSON.stringify(cmd)}) should include a segment normalized ` + + `${JSON.stringify(expected)}. ${why} Got ${JSON.stringify(list)}.` + ); +} + +function expectNoSegment(cmd, notExpected, why) { + const list = normalizedList(cmd); + assert.ok( + !list.includes(notExpected), + `commandSegments(${JSON.stringify(cmd)}) must NOT include a segment normalized ` + + `${JSON.stringify(notExpected)}. ${why} Got ${JSON.stringify(list)}.` + ); +} + +test('eval and xargs wrappers', async (t) => { + await t.test('eval: quoted and unquoted forms are analysed like bash -c', () => { + for (const cmd of [ + 'eval "git push origin main"', + "eval 'git push origin main'", + 'eval git push origin main', + ]) { + expectPush(cmd, 'git push origin main', 'eval runs its args as a command line.'); + } + }); + + await t.test('eval: chain inside tracks cd into the push cwd', () => { + const cmd = 'eval "cd repo && git push origin main"'; + expectPush(cmd, 'git push origin main', 'eval body is a command chain.'); + const seg = firstPushSegment(cmd, START); + assert.equal( + seg.cwd, + path.resolve(START, 'repo'), + `push segment cwd for ${JSON.stringify(cmd)} should be path.resolve(startDir,'repo') ` + + 'because the cd inside eval precedes the push; a wrong cwd makes branch/repo ' + + `lookups inspect the wrong repository. Got ${seg.cwd}.` + ); + }); + + await t.test('eval: nested wrapper inside eval', () => { + expectPush( + 'eval "bash -c \'git push origin main\'"', + 'git push origin main', + 'eval containing bash -c must be unwrapped twice.' + ); + }); + + await t.test('eval: env prefix and position in a chain', () => { + expectPush('FOO=1 eval "git push origin main"', 'git push origin main', + 'an env assignment prefix must not hide eval.'); + expectPush('echo hi && eval "git push origin main"', 'git push origin main', + 'eval after && is a separate command.'); + expectPush('eval "git push origin main" && echo done', 'git push origin main', + 'eval before && is a separate command.'); + }); + + await t.test('eval: other guarded commands are exposed', () => { + expectSegment('eval "gh pr merge 5"', 'gh pr merge 5', + 'the merge guard relies on seeing gh pr merge through eval.'); + }); + + await t.test('eval: negatives are not pushes', () => { + expectNoPush('eval "echo git push origin main"', + 'the pushed text is only an argument to echo.'); + expectNoPush('echo eval "git push origin main"', + 'eval is merely an argument of echo here, nothing is executed.'); + expectNoPush('eval "ls -la"', 'ls is not a push.'); + expectNoPush('eval', 'eval with no args runs nothing.'); + expectNoPush('eval ""', 'eval of an empty string runs nothing.'); + }); + + await t.test('eval: robustness on unterminated quote and variable expansion', () => { + let result; + assert.doesNotThrow( + () => { result = isGitPush('eval "git push origin main'); }, + 'isGitPush must not throw on an unterminated quote: eval "git push origin main' + ); + assert.equal(typeof result, 'boolean', + 'isGitPush must return a boolean for an unterminated-quote eval input.'); + let result2; + assert.doesNotThrow( + () => { result2 = isGitPush('eval "$CMD"'); }, + 'isGitPush must not throw on eval "$CMD"' + ); + assert.equal(result2, false, + 'isGitPush(\'eval "$CMD"\') should be false: an unexpanded variable is not a detectable push.'); + }); + + await t.test('xargs: plain and piped', () => { + expectPush('xargs git push origin main', 'git push origin main', + 'xargs runs its COMMAND; the xargs name must be dropped.'); + expectPush('echo main | xargs git push origin', 'git push origin', + 'xargs after a pipe still runs git push.'); + expectPush('/usr/bin/xargs git push origin main', 'git push origin main', + 'a path-qualified xargs must be recognised.'); + }); + + await t.test('xargs: options without values', () => { + for (const cmd of [ + 'xargs -r git push', + 'xargs -0 git push', + 'xargs -t git push', + 'xargs --no-run-if-empty git push', + 'xargs -p git push', + ]) { + expectPush(cmd, 'git push', 'valueless xargs options must be skipped.'); + } + }); + + await t.test('xargs: options with a separate value', () => { + for (const cmd of [ + 'xargs -n 1 git push origin', + 'xargs -P 4 git push', + 'xargs -L 1 git push', + "xargs -d '\\n' git push", + 'xargs -E EOF git push', + 'xargs -s 100 git push', + 'xargs -a list.txt git push', + ]) { + const exp = cmd.includes('origin') ? 'git push origin' : 'git push'; + expectPush(cmd, exp, + 'the option AND its separate value must both be skipped, otherwise the ' + + 'value is mistaken for the command.'); + } + }); + + await t.test('xargs: options with an attached value', () => { + expectPush('xargs -n1 git push', 'git push', 'attached -n1 is one token.'); + expectPush('xargs -I{} git push origin {}', 'git push origin {}', + 'placeholder text must be kept verbatim.'); + expectPush('xargs -I {} git push origin {}', 'git push origin {}', + '-I with a separate value must skip both tokens.'); + expectPush('xargs --replace={} git push origin {}', 'git push origin {}', + 'long option with =value is a single token.'); + expectPush('xargs --max-args=1 git push', 'git push', + 'long option with =value is a single token.'); + expectPush('xargs -i git push origin {}', 'git push origin {}', + '-i takes only an attached optional value, so the next token is the command.'); + }); + + await t.test('xargs: -- ends options', () => { + expectPush('xargs -- git push origin main', 'git push origin main', + 'after -- the next token is the command.'); + expectSegment('xargs -- gh pr merge 27', 'gh pr merge 27', + 'after -- the next token is the command; the merge guard must still see gh pr merge.'); + expectSegment('xargs -r -- gh pr merge 27', 'gh pr merge 27', + 'options before -- must not stop the command from being exposed.'); + }); + + await t.test('xargs: other guarded commands are exposed', () => { + expectSegment('xargs gh pr merge', 'gh pr merge', 'merge guard must see through xargs.'); + expectSegment('xargs -I{} gh pr merge {}', 'gh pr merge {}', + 'merge guard must see through xargs -I.'); + expectSegment('xargs git branch -D', 'git branch -D', + 'branch-delete guard must see through xargs.'); + expectSegment('xargs git -C repo push', 'git push', + 'git global options are stripped as for plain git.'); + expectPush('xargs git -C repo push', 'git push', 'xargs git -C repo push is a push.'); + }); + + await t.test('xargs: negatives are not pushes', () => { + expectNoPush('xargs echo git push origin main', 'the inner command is echo.'); + expectNoPush('xargs rm -f', 'rm is not a push.'); + expectNoPush('xargs', 'xargs alone has no command (defaults to echo).'); + expectNoPush('git log --oneline | xargs echo', 'no push anywhere.'); + expectNoPush('xargs -I{} echo git push {}', 'the inner command is echo.'); + }); + + await t.test('xargs inside other wrappers', () => { + expectPush('bash -c "echo main | xargs git push origin"', 'git push origin', + 'bash -c body containing xargs.'); + expectPush('eval "echo main | xargs git push origin"', 'git push origin', + 'eval body containing xargs.'); + expectPush('(echo main | xargs git push origin)', 'git push origin', + 'subshell containing xargs.'); + }); + + await t.test('xargs: robustness on options missing values', () => { + for (const cmd of ['xargs -n', 'xargs -I']) { + assert.doesNotThrow( + () => commandSegments(cmd, START), + `commandSegments must not throw for malformed input ${JSON.stringify(cmd)}` + ); + assert.doesNotThrow( + () => isGitPush(cmd), + `isGitPush must not throw for malformed input ${JSON.stringify(cmd)}` + ); + } + }); + + await t.test('regression: pre-existing behavior still holds', () => { + expectPush('git push origin main', 'git push origin main', 'plain push.'); + expectPush('cd repo && git push', 'git push', 'chain push.'); + expectPush('bash -c "git push origin main"', 'git push origin main', 'bash -c push.'); + expectPush('echo $(git push origin main)', 'git push origin main', 'command substitution.'); + expectNoPush('git status', 'git status is not a push.'); + expectNoPush('git commit -m "xargs git push"', + 'a commit message mentioning xargs git push is not a push.'); + expectNoPush("git commit -m 'eval \"git push\"'", + 'a commit message mentioning eval is not a push.'); + expectNoSegment('git commit -m "xargs git push"', 'git push', + 'message text must not become a segment.'); + }); +}); + +test('xargs short-option clustering (POSIX/GNU getopt)', async (t) => { + await t.test('valueless letters clustered', () => { + expectPush('xargs -rt git push', 'git push', + 'cluster -rt is two valueless options; treating it as the command hides the push.'); + expectPush('xargs -0r git push origin main', 'git push origin main', + 'cluster -0r is two valueless options.'); + expectPush('xargs -trp git push', 'git push', + 'cluster -trp is three valueless options.'); + expectPush('echo main | xargs -rt git push origin', 'git push origin', + 'clustered options after a pipe.'); + }); + + await t.test('value letter with attached remainder', () => { + expectPush('xargs -rn1 git push', 'git push', + 'in -rn1 the value 1 is attached to n; the next token is the command.'); + expectPush('xargs -0I{} git push origin {}', 'git push origin {}', + 'in -0I{} the replace string {} is attached to I; placeholder kept verbatim.'); + }); + + await t.test('value letter ending the cluster takes the next token', () => { + expectPush('xargs -rn 1 git push origin', 'git push origin', + 'in -rn 1 the value is the NEXT token; it must be skipped, not taken as the command.'); + expectPush('xargs -rI {} git push origin {}', 'git push origin {}', + 'in -rI {} the replace string is the NEXT token and must be skipped.'); + }); + + await t.test('mixed cluster then separate option', () => { + expectPush('xargs -rt -n 1 git push origin', 'git push origin', + 'cluster followed by a separate -n 1 option: both must be consumed.'); + }); + + await t.test('other guarded commands behind clusters', () => { + expectSegment('xargs -rt gh pr merge', 'gh pr merge', + 'the merge guard must see through clustered xargs options.'); + expectSegment('xargs -rt git branch -D', 'git branch -D', + 'the branch-delete guard must see through clustered xargs options.'); + }); + + await t.test('clustered xargs inside other wrappers', () => { + expectPush('bash -c "echo main | xargs -rt git push origin"', 'git push origin', + 'bash -c body containing clustered xargs.'); + expectPush('eval "echo main | xargs -rt git push origin"', 'git push origin', + 'eval body containing clustered xargs.'); + }); + + await t.test('negatives: inner command is not git push', () => { + expectNoPush('xargs -rt echo git push', 'the inner command is echo.'); + expectNoPush('xargs -rn1 echo git push', 'the inner command is echo; 1 is the n value.'); + expectNoPush('xargs -0r rm -f', 'rm is not a push.'); + }); + + await t.test('robustness: clusters missing a value', () => { + for (const cmd of ['xargs -rn', 'xargs -rt', 'xargs -0I']) { + assert.doesNotThrow( + () => { commandSegments(cmd, START); isGitPush(cmd); }, + `commandSegments/isGitPush must not throw for malformed input ${JSON.stringify(cmd)}` + ); + } + }); +}); + +test('xargs long options with an attached value', async (t) => { + await t.test('any --name=value is one token, not the command', () => { + expectPush('xargs --process-slot-var=SLOT git push origin', 'git push origin', + 'a long option with an attached value outside the common list must still be skipped.'); + expectPush('xargs --max-procs=4 --process-slot-var=X -rt git push', 'git push', + 'attached long options mixed with a short cluster must all be skipped.'); + expectNoPush('xargs --process-slot-var=X echo git push', 'the inner command is echo.'); + }); +}); + +function segmentFor(cmd, normalized) { + return commandSegments(cmd, START).find((s) => s.normalized === normalized); +} + +test('eval edge cases', async (t) => { + await t.test('deep eval chains do not throw, stay fast, and expose the inner command', () => { + const deepMerge = 'eval '.repeat(20000) + 'gh pr merge 5'; + const deepPush = 'eval '.repeat(20000) + 'git push origin main'; + const started = Date.now(); + let segs; + assert.doesNotThrow( + () => { segs = commandSegments(deepMerge, START); }, + 'commandSegments must not throw (e.g. stack overflow) for "eval " x20000 + "gh pr merge 5"; ' + + 'a crash in a security hook can fail open.' + ); + assert.ok( + segs.some((s) => s.normalized === 'gh pr merge 5'), + 'commandSegments of "eval " x20000 + "gh pr merge 5" should include a segment normalized ' + + `'gh pr merge 5'; deep eval nesting must not hide the merge. Got ${segs.length} segments.` + ); + let pushed; + assert.doesNotThrow( + () => { pushed = isGitPush(deepPush); }, + 'isGitPush must not throw for "eval " x20000 + "git push origin main".' + ); + assert.equal(pushed, true, + 'isGitPush should be true for "eval " x20000 + "git push origin main"; ' + + 'padding with evals must not bypass the push guard.'); + const elapsed = Date.now() - started; + assert.ok( + elapsed < 2000, + `20000-deep eval inputs took ${elapsed} ms (limit 2000 ms). A quadratic parser is a ` + + 'DoS/timeout risk for a security hook that runs on every command.' + ); + }); + + await t.test('two-level quoted eval is detected', () => { + expectPush('eval "eval \\"git push origin main\\""', 'git push origin main', + 'eval nested inside a quoted eval must be unwrapped twice.'); + }); + + await t.test('cd inside eval persists to later commands (eval runs in the current shell)', () => { + for (const cmd of ['eval "cd repo"; git push origin main', 'eval cd repo; git push origin main']) { + const seg = segmentFor(cmd, 'git push origin main'); + assert.ok(seg, `commandSegments(${JSON.stringify(cmd)}) should include 'git push origin main'.`); + assert.equal( + seg.cwd, + path.resolve(START, 'repo'), + `push cwd for ${JSON.stringify(cmd)} should be path.resolve(startDir,'repo'): eval runs in ` + + 'the current shell so its cd persists; a wrong cwd makes the guard inspect the wrong ' + + `repository/branch. Got ${seg.cwd}.` + ); + } + const cmd = 'eval "cd repo && cd .." ; git push'; + const seg = segmentFor(cmd, 'git push'); + assert.ok(seg, `commandSegments(${JSON.stringify(cmd)}) should include 'git push'.`); + assert.equal( + seg.cwd, + path.resolve(START), + `push cwd for ${JSON.stringify(cmd)} should be the start dir (cd repo then cd ..). Got ${seg.cwd}.` + ); + }); + + await t.test('regression: cd inside bash -c does not leak (subprocess)', () => { + const cmd = 'bash -c "cd repo"; git push origin main'; + const seg = segmentFor(cmd, 'git push origin main'); + assert.ok(seg, `commandSegments(${JSON.stringify(cmd)}) should include 'git push origin main'.`); + assert.equal( + seg.cwd, + path.resolve(START), + `push cwd for ${JSON.stringify(cmd)} should stay the start dir: a bash -c subprocess's cd ` + + `does not affect the parent shell. Got ${seg.cwd}.` + ); + }); + + await t.test('eval -- is accepted', () => { + expectPush('eval -- "git push origin main"', 'git push origin main', + 'eval accepts -- to end options; it must not hide the push.'); + expectPush('eval -- git push origin main', 'git push origin main', + 'eval accepts -- to end options; unquoted form.'); + expectNoPush('eval -- "echo git push"', 'the pushed text is only an argument to echo.'); + }); +}); + +test('quoted command words', async (t) => { + await t.test('a quoted eval is still eval', () => { + for (const cmd of [ + '"eval" "git push origin main"', + "'eval' 'git push origin main'", + 'e"va"l git push origin main', + '\\eval git push origin main', + ]) { + expectPush(cmd, 'git push origin main', + 'quote removal runs before the shell looks up the command word, so this is the eval builtin.'); + } + }); + + await t.test('other quoted wrapper words are still wrappers', () => { + expectPush('"sudo" git push origin main', 'git push origin main', + 'a quoted sudo still runs its command.'); + expectPush("'xargs' -0 git push origin main", 'git push origin main', + 'a quoted xargs still runs its command.'); + expectPush('"env" FOO=1 git push origin main', 'git push origin main', + 'a quoted env still runs its command.'); + expectPush('"bash" -c "git push origin main"', 'git push origin main', + 'a quoted bash still runs its -c string.'); + expectPush('xargs "git" push origin main', 'git push origin main', + 'a quoted command word after xargs is still the command xargs runs.'); + }); + + await t.test('a backslash anywhere in a command word is removed', () => { + expectPush('g\\it push origin main', 'git push origin main', + 'quote removal turns g\\it into git.'); + expectPush('sud\\o git push origin main', 'git push origin main', + 'quote removal turns sud\\o into sudo.'); + expectPush('xarg\\s git push origin main', 'git push origin main', + 'quote removal turns xarg\\s into xargs.'); + expectPush('C:\\Git\\bin\\git.exe push origin main', 'git push origin main', + 'an unquoted Windows path to git is still recognised as git.'); + }); + + await t.test('quoted wrapper options are still options', () => { + expectPush("xargs '-r' git push origin main", 'git push origin main', + 'quote removal makes -r an xargs option.'); + expectPush('xargs "--no-run-if-empty" git push origin main', 'git push origin main', + 'quote removal makes --no-run-if-empty an xargs option.'); + expectPush("xargs '-n' 1 git push origin main", 'git push origin main', + 'a quoted value-taking option still takes the next word.'); + expectPush("env '-i' git push origin main", 'git push origin main', + 'quote removal makes -i an env option.'); + expectPush("sudo '-n' git push origin main", 'git push origin main', + 'quote removal makes -n a sudo option.'); + }); + + await t.test('a quoted git or gh is still git or gh', () => { + expectPush('"git" push origin main', 'git push origin main', + 'a quoted git is the same binary.'); + expectSegment("'gh' pr merge 5", 'gh pr merge 5', + 'the merge guard relies on seeing gh pr merge.'); + }); + + await t.test('eval joins all of its arguments', () => { + expectPush('eval "git push" origin main', 'git push origin main', + 'eval runs every argument, joined by spaces; dropping the rest loses the refspec.'); + expectPush('eval "git" "push" "origin" "+main"', 'git push origin +main', + 'eval runs every argument, joined by spaces; the force refspec must survive.'); + expectPush('eval git push origin "main"', 'git push origin main', + 'eval re-parses its joined arguments after quote removal.'); + }); + + await t.test('escaped quotes inside an eval string stay quoted', () => { + expectPush('eval "git push origin \\"my branch\\""', 'git push origin "my branch"', + 'an escaped quote is literal text, so the inner command keeps it.'); + }); + + await t.test('negatives: quoted words that are only arguments', () => { + expectNoPush('echo "eval" "git push origin main"', + 'eval is an argument of echo here, nothing is executed.'); + expectNoPush('"eval" "echo git push origin main"', + 'the pushed text is only an argument to echo.'); + expectNoPush('"git push" origin main', + 'a single quoted word "git push" names a command that does not exist.'); + }); +}); + +test('shell quoting forms inside wrappers', async (t) => { + await t.test('ANSI-C quoting is unquoted', () => { + expectPush("eval $'git push origin main'", 'git push origin main', + "$'...' is one argument to eval, which runs it."); + expectPush("eval $'git push origin \\'main\\''", "git push origin 'main'", + "an escaped single quote inside $'...' is literal text."); + expectPush("$'git' push origin main", 'git push origin main', + "an ANSI-C quoted command word is still git."); + expectPush("eval $'git\\tpush origin main'", 'git push origin main', + 'a \\t escape becomes a tab, which separates words like a space.'); + }); + + await t.test('backslash-newline is a line continuation, not text', () => { + expectPush('eval "g\\\nit push origin main"', 'git push origin main', + 'inside double quotes, backslash-newline is removed before eval sees it.'); + expectPush('eval g\\\nit push origin main', 'git push origin main', + 'unquoted, backslash-newline is removed before eval sees it.'); + }); + + await t.test('xargs.exe and path-qualified Windows xargs', () => { + expectPush('xargs.exe git push origin main', 'git push origin main', + 'xargs.exe is the same program on Windows.'); + expectPush('C:\\Git\\usr\\bin\\xargs.exe git push origin main', 'git push origin main', + 'a Windows path to xargs.exe is the same program.'); + }); + + await t.test('xargs short options that take a separate value', () => { + expectPush('xargs -R 1 git push origin main', 'git push origin main', + '-R (max replacements) takes the next word as its value.'); + expectPush('xargs -J {} git push origin main', 'git push origin main', + '-J (BSD replacement string) takes the next word as its value.'); + expectPush('xargs -S 1024 git push origin main', 'git push origin main', + '-S (BSD replacement size) takes the next word as its value.'); + }); +}); diff --git a/.claude/hooks/lib/git-command.cjs b/.claude/hooks/lib/git-command.cjs index 684c3b6..a22bc28 100644 --- a/.claude/hooks/lib/git-command.cjs +++ b/.claude/hooks/lib/git-command.cjs @@ -82,6 +82,21 @@ function substitutionEnd(str, i) { return str.length; } +// Index just past the closing `'` of an ANSI-C quoted string (`$'...'`) +// opening at `i`, or the end of `str` if unterminated. Unlike plain single +// quotes, a backslash escapes the next character, so `\'` doesn't end it. +function ansiCQuoteEnd(str, i) { + for (let j = i + 2; j < str.length; j++) { + if (str[j] === '\\') j++; + else if (str[j] === "'") return j + 1; + } + return str.length; +} + +// `$'...'` escapes and what they stand for. Others (`\xHH`, octal) are kept +// as written, which can only make a word harder to recognise, not easier. +const ANSI_C_ESCAPES = { n: '\n', t: '\t', r: '\r', a: '\x07', b: '\b', e: '\x1b', E: '\x1b', f: '\f', v: '\v', '\\': '\\', "'": "'", '"': '"', '?': '?' }; + // Index just past a quoted string, an escaped character, or a command // substitution opening at `i` — text that goes into a segment whole and is // never split — or -1 if `i` opens none of those. Single quotes are fully @@ -89,6 +104,7 @@ function substitutionEnd(str, i) { function unsplittableEnd(str, i) { const ch = str[i]; if (ch === '\\') return Math.min(i + 2, str.length); + if (ch === '$' && str[i + 1] === "'") return ansiCQuoteEnd(str, i); if (ch === "'") { const close = str.indexOf("'", i + 1); return close === -1 ? str.length : close + 1; @@ -194,6 +210,111 @@ function unwrapShellDashC(segment) { return quoteChar === '"' ? content.replace(/\\(["\\$`])/g, '$1') : content; } +// `str` split into shell words with quote removal applied, as the shell +// does before running a command: `e"va"l` → `eval`, `\eval` → `eval`, +// `"a \"b\""` → `a "b"`, `$'a\tb'` → `ab`, and a backslash-newline +// line continuation is removed. `$(...)` and backtick substitutions are +// copied through unchanged, to be parsed later. Null if a quote is +// unterminated. +function unquotedWords(str) { + const words = []; + let current = ''; + let inWord = false; + for (let i = 0; i < str.length; i++) { + const ch = str[i]; + if (ch === '\\' && str[i + 1] === '\n') { + i++; + continue; + } + if (/\s/.test(ch)) { + if (inWord) words.push(current); + current = ''; + inWord = false; + continue; + } + inWord = true; + if (ch === '\\') { + if (i + 1 < str.length) current += str[++i]; + } else if (ch === '$' && str[i + 1] === "'") { + let j = i + 2; + for (; j < str.length && str[j] !== "'"; j++) { + if (str[j] === '\\' && j + 1 < str.length) { + const esc = str[++j]; + current += esc in ANSI_C_ESCAPES ? ANSI_C_ESCAPES[esc] : `\\${esc}`; + } else { + current += str[j]; + } + } + if (j >= str.length) return null; + i = j; + } else if (ch === "'") { + const close = str.indexOf("'", i + 1); + if (close === -1) return null; + current += str.slice(i + 1, close); + i = close; + } else if (ch === '"') { + let j = i + 1; + for (; j < str.length && str[j] !== '"'; j++) { + if (str[j] === '\\' && str[j + 1] === '\n') { + j++; + } else if (str[j] === '\\' && /["\\$`]/.test(str[j + 1] || '')) { + current += str[++j]; + } else if (str[j] === '`' || (str[j] === '$' && str[j + 1] === '(')) { + const end = substitutionEnd(str, j); + current += str.slice(j, end); + j = end - 1; + } else { + current += str[j]; + } + } + if (j >= str.length) return null; + i = j; + } else if (ch === '`' || (ch === '$' && str[i + 1] === '(')) { + const end = substitutionEnd(str, i); + current += str.slice(i, end); + i = end - 1; + } else { + current += ch; + } + } + if (inWord) words.push(current); + return words; +} + +// `eval ARGS...` runs its arguments, after quote removal and joined by +// spaces, as a command line. Returns that inner string, or null if +// `segment` isn't an eval or a quote is unterminated. Leading `eval` words +// (and `--`) are skipped in a loop, not by recursion, so a very long +// `eval eval eval ...` chain can't overflow the stack. +function unwrapEval(segment) { + const words = unquotedWords(segment); + if (words === null || words[0] !== 'eval') return null; + let i = 0; + while (words[i] === 'eval') { + i++; + if (words[i] === '--') i++; + } + return words.slice(i).join(' '); +} + +// A quoted or backslash-escaped leading word (`"eval"`, `'sudo'`, `g\it`, +// `'-r'`) → its unquoted spelling, since that's what the shell looks up or +// passes on. A word that unquotes to something containing whitespace is left +// alone: no wrapper or option is spelled that way, and a quoted path with +// spaces is handled by stripCommandPathPrefix(). So is an unquoted Windows +// path (`C:\Git\bin\git.exe`, `.\gh.exe`), whose backslashes are separators +// as written, not escapes. +const LEADING_WORD = new RegExp(`^${WORD}`); +const UNQUOTED_WINDOWS_PATH = /^(?:[A-Za-z]:|\.{1,2})\\[^"']*$/; + +function unquoteLeadingWord(segment) { + const m = segment.match(LEADING_WORD); + if (!m || !/[\\"']/.test(m[0]) || UNQUOTED_WINDOWS_PATH.test(m[0])) return segment; + const words = unquotedWords(m[0]); + if (words === null || words.length !== 1 || /\s/.test(words[0])) return segment; + return words[0] + segment.slice(m[0].length); +} + // A leading `env` (bare or path-qualified, with its own flags) and any // number of `VAR=value` assignments — `GIT_SSH_COMMAND="ssh -vv" git push` // is the same push. @@ -209,7 +330,7 @@ function stripLeadingEnvPrefix(segment) { let prev; do { prev = s; - s = s + s = unquoteLeadingWord(s) .replace(ENV_END_OPTIONS, '') .replace(ENV_ARG_FLAG_COMBINED, '') .replace(ENV_ARG_FLAG_SEPARATE, '') @@ -235,14 +356,63 @@ function stripLeadingWrapperPrefix(segment) { do { prev = s; if (m[1] === 'sudo') { - s = s.replace(SUDO_ARG_FLAG, '').replace(SUDO_NO_ARG_FLAG, ''); + s = unquoteLeadingWord(s).replace(SUDO_ARG_FLAG, '').replace(SUDO_NO_ARG_FLAG, ''); } else if (m[1] === 'command') { - s = s.replace(COMMAND_NO_ARG_FLAG, ''); + s = unquoteLeadingWord(s).replace(COMMAND_NO_ARG_FLAG, ''); } } while (s !== prev); return s; } +// `xargs [options] COMMAND ...` runs COMMAND, so the word and its own +// options are dropped and COMMAND (placeholders and all) is what's checked. +// Short options may be clustered getopt-style (`-rn1`, `-0I {}`): letters +// are walked in turn, and a value-taking letter (n P L d E s a I, and R J S +// from GNU/BSD) takes the rest of the token, or the next token if none +// remains; i/e/l take only an attached rest. Only the common options are +// handled. +const XARGS_COMMAND = /^(?:\S*[\\/])?xargs(?:\.exe)?(?=\s|$)\s*/; +const XARGS_SHORT_CLUSTER = /^-([^-\s]\S*)\s*/; +const XARGS_LONG_ARG_FLAG_COMBINED = /^--[a-z][a-z-]*=\S*\s*/; +const XARGS_LONG_ARG_FLAG_SEPARATE = new RegExp(`^--(?:max-args|max-procs|max-lines|max-chars|delimiter|eof|arg-file)\\s+${WORD}\\s*`); +const XARGS_LONG_NO_ARG_FLAG = /^--[a-z][a-z-]*(?=\s|$)\s*/; +const XARGS_SHORT_VALUE_LETTER = /[nPLdEsaIRJS]/; +const XARGS_SHORT_OPTIONAL_LETTER = /[iel]/; +const NEXT_WORD = new RegExp(`^${WORD}\\s*`); + +// `s` minus one leading short-option cluster (and the next word, if the +// cluster ends in a value-taking letter), or unchanged if there is none. +function stripXargsShortCluster(s) { + const m = s.match(XARGS_SHORT_CLUSTER); + if (!m) return s; + const rest = s.slice(m[0].length); + const letters = m[1]; + for (let i = 0; i < letters.length; i++) { + if (XARGS_SHORT_OPTIONAL_LETTER.test(letters[i])) return rest; + if (XARGS_SHORT_VALUE_LETTER.test(letters[i])) { + return i < letters.length - 1 ? rest : rest.replace(NEXT_WORD, ''); + } + } + return rest; +} + +function stripLeadingXargs(segment) { + if (!XARGS_COMMAND.test(segment)) return segment; + let s = segment.replace(XARGS_COMMAND, ''); + let prev; + do { + prev = s; + s = stripXargsShortCluster( + unquoteLeadingWord(s) + .replace(ENV_END_OPTIONS, '') + .replace(XARGS_LONG_ARG_FLAG_COMBINED, '') + .replace(XARGS_LONG_ARG_FLAG_SEPARATE, '') + .replace(XARGS_LONG_NO_ARG_FLAG, ''), + ); + } while (s !== prev); + return s; +} + // Shell control-flow words that put the next word in command position: // once `if true; then git push; fi` is split on `;`, the push segment // starts with `then`. Same for `do`/`else`/`elif`/`if`/`while`/`until`, @@ -254,13 +424,13 @@ function stripLeadingControlFlow(segment) { } // Env prefixes, sudo/command/exec wrappers, and control-flow words, in any -// mix and order. +// mix and order, each command word unquoted first. function stripPrefixes(segment) { let s = segment; let prev; do { prev = s; - s = stripLeadingWrapperPrefix(stripLeadingEnvPrefix(stripLeadingControlFlow(s))); + s = stripLeadingXargs(stripLeadingWrapperPrefix(stripLeadingEnvPrefix(stripLeadingControlFlow(unquoteLeadingWord(s))))); } while (s !== prev); return s; } @@ -388,11 +558,25 @@ function gitDir(segment, base) { // A `cd` inside a shell wrapper doesn't carry out of it, as in a real shell. function commandSegments(fullCmd, startDir = process.cwd()) { const out = []; + walkSegments(fullCmd, startDir, out); + return out; +} + +// Appends fullCmd's segments to `out`; returns the directory it ends in. A +// `bash -c` runs in a subprocess, so its `cd` stays inside it; `eval` runs in +// the current shell, so its `cd` carries on to what follows. +function walkSegments(fullCmd, startDir, out) { let dir = startDir; for (const raw of segmentsOf(fullCmd)) { - const inner = unwrapShellDashC(stripPrefixes(raw)); - if (inner !== null) { - out.push(...commandSegments(inner, dir)); + const stripped = stripPrefixes(raw); + const dashC = unwrapShellDashC(stripped); + if (dashC !== null) { + walkSegments(dashC, dir, out); + continue; + } + const evalInner = unwrapEval(stripped); + if (evalInner !== null) { + dir = walkSegments(evalInner, dir, out); continue; } for (const body of substitutionsOf(raw)) { @@ -407,21 +591,23 @@ function commandSegments(fullCmd, startDir = process.cwd()) { } out.push({ raw, normalized, cwd: gitDir(raw, dir) }); } - return out; + return dir; } -// The first `git push` in the command, as a commandSegments() entry, or -// undefined if there is none. -function firstPushSegment(fullCmd, startDir = process.cwd()) { - return commandSegments(fullCmd, startDir).find((s) => /^git\s+push(?:\s|$)/.test(s.normalized)); +// Every `git push` in the command, in order, as commandSegments() entries +// plus `dir`: the directory that push runs in if it can be determined and +// exists, otherwise null. A chain can push several repos, so a hook that +// checks a push checks each one here, not only the first. +function pushSegments(fullCmd, startDir = process.cwd()) { + return commandSegments(fullCmd, startDir) + .filter((s) => /^git\s+push(?:\s|$)/.test(s.normalized)) + .map((s) => ({ ...s, dir: s.cwd !== null && fs.existsSync(s.cwd) ? s.cwd : null })); } -// The directory the first `git push` in the command runs in, if it can be -// determined and exists; otherwise null. -function pushTargetDir(fullCmd, startDir = process.cwd()) { - const push = firstPushSegment(fullCmd, startDir); - if (!push || push.cwd === null) return null; - return fs.existsSync(push.cwd) ? push.cwd : null; +// The first `git push` in the command, as a pushSegments() entry, or +// undefined if there is none. +function firstPushSegment(fullCmd, startDir = process.cwd()) { + return pushSegments(fullCmd, startDir)[0]; } // True if any command in the string is an actual `git push`. @@ -442,13 +628,30 @@ function stripSurroundingQuotes(token) { return token; } +// A redirection word: `>`, `>>`, `2>`, `&>`, `<`, with or without its +// target attached (`>/dev/null`, `2>&1`). segmentsOf() splits `2>&1` at +// the `&`, leaving `2>` at the end of the segment. +const REDIRECTION = /^\d*(?:&>>?|>>?&?|>\||<&?)/; + // A segment's shell words with their quotes removed: `"/tmp/remote repo"` // is one word, and `feat"ure"` is `feature`. Backslash escapes and -// expansions are left as written. +// expansions are left as written. A `#` comment ends the words, and +// redirections (and a detached target, as in `> out.log`) are dropped, so +// neither is mistaken for an argument. function shellWords(segment) { - return (segment.match(new RegExp(WORD, 'g')) || []).map((word) => - word.replace(/"([^"]*)"|'([^']*)'/g, (_, dq, sq) => dq ?? sq) - ); + const raw = segment.match(new RegExp(WORD, 'g')) || []; + const words = []; + for (let i = 0; i < raw.length; i++) { + const word = raw[i]; + if (word.startsWith('#')) break; + const redirect = word.match(REDIRECTION); + if (redirect) { + if (redirect[0] === word) i++; + continue; + } + words.push(word.replace(/"([^"]*)"|'([^']*)'/g, (_, dq, sq) => dq ?? sq)); + } + return words; } // `git push` options whose value is the next word, so it isn't mistaken @@ -504,9 +707,10 @@ module.exports = { isGitPush, normalizeSegment, pushArgs, - pushTargetDir, + pushSegments, segmentsOf, splitRefspec, stripLeadingEnvPrefix, stripSurroundingQuotes, + unquotedWords, }; diff --git a/.claude/hooks/lib/git-command.test.cjs b/.claude/hooks/lib/git-command.test.cjs index b26f6dc..8537d39 100644 --- a/.claude/hooks/lib/git-command.test.cjs +++ b/.claude/hooks/lib/git-command.test.cjs @@ -8,7 +8,7 @@ const test = require('node:test'); const assert = require('node:assert/strict'); const os = require('node:os'); const path = require('node:path'); -const { commandSegments, isGitPush, pushArgs, pushTargetDir, splitRefspec } = require('./git-command.cjs'); +const { commandSegments, isGitPush, pushArgs, pushSegments, splitRefspec } = require('./git-command.cjs'); const BASE = path.resolve(os.tmpdir(), 'base'); @@ -171,16 +171,28 @@ test('commandSegments cwd', async (t) => { } }); -test('pushTargetDir', async (t) => { +test('pushSegments', async (t) => { + const pushDirs = (cmd) => pushSegments(cmd, BASE).map((s) => s.dir); + await t.test('the directory of the push segment when it exists', () => { - assert.equal(pushTargetDir(`cd "${__dirname}" && git push`, BASE), __dirname); - assert.equal(pushTargetDir(`git -C "${__dirname}" push`, BASE), __dirname); + assert.deepEqual(pushDirs(`cd "${__dirname}" && git push`), [__dirname]); + assert.deepEqual(pushDirs(`git -C "${__dirname}" push`), [__dirname]); + }); + + await t.test('null for a missing directory or an unknown one', () => { + assert.deepEqual(pushDirs('cd does-not-exist && git push'), [null]); + assert.deepEqual(pushDirs('cd "$REPO" && git push'), [null]); + }); + + await t.test('no entries when nothing is pushed', () => { + assert.deepEqual(pushSegments(`cd "${__dirname}" && git status`, BASE), []); }); - await t.test('null for a missing directory, an unknown one, or no push', () => { - assert.equal(pushTargetDir('cd does-not-exist && git push', BASE), null); - assert.equal(pushTargetDir('cd "$REPO" && git push', BASE), null); - assert.equal(pushTargetDir(`cd "${__dirname}" && git status`, BASE), null); + await t.test('every push in a chain, each with its own directory', () => { + const parent = path.dirname(__dirname); + const pushes = pushSegments(`git -C "${__dirname}" push origin a && git status && git -C "${parent}" push origin b`, BASE); + assert.deepEqual(pushes.map((s) => s.dir), [__dirname, parent]); + assert.deepEqual(pushes.map((s) => s.normalized), ['git push origin a', 'git push origin b']); }); });