From 87cd1c851b4f78c28112af23b39c50c187642af5 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Sat, 7 Nov 2026 13:39:07 -0800 Subject: [PATCH 01/10] chore(quest): claim quest/m0/token-waste/rule-checks.md Quest-Claim: c04c502918737a36 From 7503f6dc3690884540cc3a03b3394e7253509919 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Sat, 7 Nov 2026 13:56:57 -0800 Subject: [PATCH 02/10] feat(hooks)!: enforce the mechanical rules with PreToolUse hooks quest setup installs a PreToolUse hook for Claude Code and Codex that denies tool calls breaking the rules a program can check: em dashes an agent adds, a git push without explicit refspecs, a force-push or rebase, a new GitHub post without its footer, and scratch writes under /tmp. Their prose leaves the hook's context and stays in `quest context` for agents without it. quest setup also writes Codex's SessionStart rules hook to hooks.json, with an additionalContextLimit that keeps the rules whole. Co-Authored-By: Claude Opus 5.5 --- README.md | 55 +++-- SETUP.md | 24 +- assets/CONTEXT.md | 8 +- quest/m0/token-waste/README.md | 4 +- quest/m0/token-waste/rule-checks.md | 46 ---- src/cli/hooks.ts | 136 ++++++++--- src/cli/main.ts | 58 +++-- src/cli/rules.ts | 334 ++++++++++++++++++++++++++++ src/cli/shell.ts | 147 ++++++++++++ src/runner/prompt.ts | 4 +- test/hooks.test.ts | 91 ++++++-- test/rules.test.ts | 175 +++++++++++++++ 12 files changed, 929 insertions(+), 153 deletions(-) delete mode 100644 quest/m0/token-waste/rule-checks.md create mode 100644 src/cli/rules.ts create mode 100644 src/cli/shell.ts create mode 100644 test/rules.test.ts diff --git a/README.md b/README.md index a4e69103..5a674ad1 100644 --- a/README.md +++ b/README.md @@ -1642,32 +1642,55 @@ The [getting started guide](docs/getting-started.md) covers what happens next. quest format and workflow they read before working on a quest outside a skill; `quest guide prompting` prints one section. -### Configure Claude Code +### Configure your agent -`quest setup` configures your own Claude Code settings for quest work, in every -repository: +`quest setup` configures your own Claude Code and Codex settings for quest +work, in every repository: ```sh quest setup ``` -It sets `env.BASH_MAX_TIMEOUT_MS` to 3300000, so a foreground command such as -`quest wait` can block for 55 minutes, just under the 1h prompt-cache TTL, -instead of the 10-minute default. +It sets Claude Code's `env.BASH_MAX_TIMEOUT_MS` to 3300000, so a foreground +command such as `quest wait` can block for 55 minutes, just under the 1h +prompt-cache TTL, instead of the 10-minute default. It also loads the rules once. The repository's `Quests: ` line has agents run `quest context` before any work, which costs a turn and is summarized away when the session compacts. Instead, `quest setup` installs a `SessionStart` -hook (startup, resume, clear, and compact) and a `SubagentStart` hook, each -running `quest hook`, which adds `quest context` to the session in a -repository whose root `AGENTS.md` or `CLAUDE.md` has the `Quests: ` line, and -nothing elsewhere. `quest` must be on the `PATH` Claude Code starts with, as -for the worktree hooks. - -It edits `$CLAUDE_CONFIG_DIR/settings.json`, or `~/.claude/settings.json`, -keeps your other settings, refuses a malformed file unchanged, writes once, -and prints each change. Rerun it after upgrading. `quest setup --remove` takes -the hooks out, and the timeout while it still holds Quest's value. +hook (startup, resume, clear, and compact), and for Claude Code a +`SubagentStart` hook, each running `quest hook`, which adds `quest context` to +the session in a repository whose root `AGENTS.md` or `CLAUDE.md` has the +`Quests: ` line, and nothing elsewhere. Codex holds the rules in full, rather +than spilling them to a file, through the hook's `additionalContextLimit`. + +The rules a program can check are enforced instead of read. A `PreToolUse` +hook on shell commands and file edits denies, with the rule as the reason: + +- an edit, commit message, or new GitHub post that adds an em dash outside a + `>` quote line; +- a `git push` whose refspecs lack a `:`; +- a force-push, `git rebase`, or `git pull --rebase`, unless the command + starts with `QUEST_USER_ASKED=1` because the user asked for it; +- a new GitHub post (`gh pr create`, `comment`, or `review`, `gh issue create` + or `comment`, or `gh api` with a `body` field) without a `(Written by ...)` + line; +- a scratch file written under `/tmp`, outside the checkout. + +A body it cannot read, such as one piped into `gh`, passes. `quest context` +still prints these rules, under a last `Checked` section that the hook leaves +out, for agents without it. + +`quest` must be on the `PATH` the agent starts with, as for the worktree +hooks. Codex asks you to trust the hooks once, and after each upgrade that +changes them, through `/hooks`; `quest review` runs Codex without your +config, so its reviewers run none of them. + +It edits `$CLAUDE_CONFIG_DIR/settings.json`, or `~/.claude/settings.json`, and +`$CODEX_HOME/hooks.json`, or `~/.codex/hooks.json`, when that directory exists. +It keeps your other settings, refuses a malformed file unchanged, writes each +once, and prints each change. Rerun it after upgrading. `quest setup --remove` +takes the hooks out, and the timeout while it still holds Quest's value. ## Where this is going diff --git a/SETUP.md b/SETUP.md index c56e8fd7..3e2daa7d 100644 --- a/SETUP.md +++ b/SETUP.md @@ -62,16 +62,20 @@ to proceed; never delete or rename their files to get past it. ## Configure the user's agent -`quest setup` configures the user's own Claude Code settings, not the -repository's, in `$CLAUDE_CONFIG_DIR/settings.json` (or -`~/.claude/settings.json`). It sets `env.BASH_MAX_TIMEOUT_MS` to 55 minutes, so -agents can block on a long wait in one call instead of polling. It also adds -the rules the `Quests: ` line points at to every session and subagent, and -again after compaction, which would otherwise summarize them away, through -hooks that need `quest` on the `PATH` Claude Code runs hooks with. Offer it, -explaining that it applies in every repository the user opens in Claude Code -(the hooks only in those with the `Quests: ` line), and run it with consent. It -keeps the file's other settings and prints what it changed. +`quest setup` configures the user's own agent settings, not the repository's: +Claude Code's in `$CLAUDE_CONFIG_DIR/settings.json` (or +`~/.claude/settings.json`), and Codex's in `$CODEX_HOME/hooks.json` (or +`~/.codex/hooks.json`) when that directory exists. It sets Claude Code's +`env.BASH_MAX_TIMEOUT_MS` to 55 minutes, so agents can block on a long wait in +one call instead of polling. It also adds the rules the `Quests: ` line points +at to every session, and again after compaction, which would otherwise +summarize them away, and blocks tool calls that break the rules a program can +check, such as a `git push` without an explicit refspec, through hooks that +need `quest` on the `PATH` the agent runs hooks with. Offer it, explaining that +it applies in every repository the user opens (the hooks act only in those with +the `Quests: ` line), and run it with consent. Codex then asks the user to +trust the hooks once through `/hooks`. It keeps the files' other settings and +prints what it changed. ## Opt into worktree hooks diff --git a/assets/CONTEXT.md b/assets/CONTEXT.md index 6ab89f6d..a05a2951 100644 --- a/assets/CONTEXT.md +++ b/assets/CONTEXT.md @@ -15,8 +15,6 @@ Engineering rules for every task, as `quest context` prints them. The repository - Every test runs in CI. The merge gate holds only fast, high-signal tests; slow ones run nightly. - Be adversarial: attack your own approach, reports, reviews, and other PRs as hard as anyone else's, and argue for the best approach over the first one that works. - Never add or change a rule in an `AGENTS.md`, `CONTRIBUTING.md`, or skill without being prompted. When you do, follow `quest guide prompting`. -- No em dashes. -- End every GitHub post you author with `(Written by with )`, naming the running model and its reasoning effort, or `(Written by )` when the harness doesn't report the effort; never guess it. Commit messages with a `Co-Authored-By:` trailer are exempt. - In a public repository, never mention customers, partners, or prospects, or link to private repositories. Justify work on its merits. ## Important @@ -56,7 +54,7 @@ Work in your own worktree, reusing a suitable one you already own. Read-only discovery needs no new checkout when your context or own checkout answers it. Put PR bodies, review drafts, logs, patches, helper scripts, and temporary clones in its `.tmp/`, so they disappear with the worktree. If the repository doesn't ignore `.tmp/`, add it to the file `git rev-parse --git-path info/exclude` names. -Never write them to `/tmp`, elsewhere in or beside the checkout, or in a directory other agents share, such as a session scratchpad. +Never write them elsewhere in or beside the checkout, or in a directory other agents share. Pass PR bodies to `gh` on stdin (`--body-file -`). Before ending a task, pausing for a decision, or handing off, commit a checkpoint and report what remains and which checks passed or did not run. @@ -87,9 +85,7 @@ A decision about an open PR never authorizes closing it. Confirm a close as its ## Pull requests -Push with an explicit refspec: `git push refs/heads/:refs/heads/`. -A bare branch name can push to the base instead, as under `push.default=upstream` when the branch tracks it. -Never rebase or force-push, even a branch only you hold; merge the base instead, only when the branch conflicts with it or a landing step, such as `quest merge` or a merge queue, refuses the head, and resolve the conflicts in that merge; a branch only behind needs none, since landing tests it on the current base. +Merge the base only when the branch conflicts with it or a landing step, such as `quest merge` or a merge queue, refuses the head, and resolve the conflicts in that merge; a branch only behind needs none, since landing tests it on the current base. A PR that another session pushed to in the last hour is theirs: report its events instead of pushing to it. Before each push to a PR's branch, confirm the PR is still open, since a push after it merges restores the deleted branch. Run the repository's checks locally while the PR is a draft; CI may run only on PRs ready for review. diff --git a/quest/m0/token-waste/README.md b/quest/m0/token-waste/README.md index a081e38a..b7696200 100644 --- a/quest/m0/token-waste/README.md +++ b/quest/m0/token-waste/README.md @@ -41,7 +41,8 @@ Decision prompts, chosen options marked ✅: (Theo's tstack, jdx's jouet, Matt Pocock's skills, HumanLayer, Mitchell Hashimoto), chosen ✅ and rejected listed: - ✅ Mechanical rules become hooks or lint, so their prose leaves the - always-loaded rules ([rule checks](/quest/m0/token-waste/rule-checks.md)). + always-loaded rules, done in + [moq-dev/quest#553](https://github.com/moq-dev/quest/pull/553). - ✅ A review round budget by severity, after Theo's `sol-loop` ([moq-dev/quest#514](https://github.com/moq-dev/quest/pull/514)). - ✅ `quest usage` moves from m1 into this epic, so its closing @@ -69,7 +70,6 @@ Decision prompts, chosen options marked ✅: ## Required - [Epics list no children](/quest/m0/token-waste/child-lists.md) - an epic's children are its directory, so finishing a quest edits no README -- [Mechanical rules become checks](/quest/m0/token-waste/rule-checks.md) - hooks enforce em dashes, push refspecs, no rebase or force-push, the footer, and `.tmp/`, and their prose leaves the always-loaded rules - [quest usage](/quest/m0/token-waste/usage.md) - per quest and PR: which models and efforts worked on it, their tokens and API-equal cost, and how the work went; the epic's re-measure runs it - [Point to token-max](/quest/m0/token-waste/token-max.md) - `SETUP.md` recommends the standalone settings doctor, and the re-measure runs it diff --git a/quest/m0/token-waste/rule-checks.md b/quest/m0/token-waste/rule-checks.md deleted file mode 100644 index 374d517f..00000000 --- a/quest/m0/token-waste/rule-checks.md +++ /dev/null @@ -1,46 +0,0 @@ -# [M] Mechanical rules become checks - -## Goal - -The rules in `assets/CONTEXT.md` that a program can check are enforced by -checks instead of prose: no em dashes in what agents write, `git push` with -an explicit refspec, never `git rebase` or a force-push, the -`(Written by ...)` footer on GitHub posts, and scratch files in the -checkout's `.tmp/` rather than `/tmp`. Each check blocks the action with a -message that says what to do instead, and the prose it replaces leaves the -always-loaded rules. - -## Plan - -Decided 2026-11-05 (user, adopting practices for the -[token waste](/quest/m0/token-waste/README.md) epic). Sources: Claude Code's -best practices ("if you keep adding emphasis, it belongs in a hook"), -HumanLayer ("never send an LLM to do a linter's job"), and t3code, which -enforces its prose rules with custom lint rules. - -Why: `quest context` and `quest guide` load about 260 instruction sentences -before every task, against the 150-200 published guidance says models follow -reliably. A rule that a check enforces no longer needs reading. - -Recommendations for the implementer: - -- Deliver Claude Code `PreToolUse` hooks (Bash for `git push`, `git rebase`, - `--force`, `/tmp` writes, and `gh` post bodies; Write and Edit for em - dashes) at user level through `quest setup`, as a row each in `HOOKS` and a - case each in `quest hook` (`src/cli/hooks.ts`), beside its `SessionStart` - and `SubagentStart` entries, since repositories write no hooks. Give Codex - the equivalent `hooks.json` entries where its hooks can match. -- Codex parity for the always-on rules too (coordinator, 2026-11-05, - follow-up of [moq-dev/quest#517](https://github.com/moq-dev/quest/pull/517), - which gave Claude Code the `SessionStart` rules hook and Codex none): - `quest setup` writes the same `SessionStart` entry (`compact` included) - to `~/.codex/hooks.json`, with an `additionalContextLimit` that fits the - rules. `quest review` runs Codex with `--ignore-user-config`; confirm in - a review transcript that this keeps the user hook out of reviews, and - keep it out if not. Codex asks to trust each hook once through `/hooks`. -- Keep a check narrow enough that it never blocks legitimate work: a force - push the user asked for, an em dash quoted from someone else, `/tmp` used - by a tool rather than for agent scratch. A check that cannot tell these - apart stays prose. -- Remove each enforced rule from `assets/CONTEXT.md` in the same PR, and say - in the PR body which rules stayed prose and why. diff --git a/src/cli/hooks.ts b/src/cli/hooks.ts index fbd099cf..9579503f 100644 --- a/src/cli/hooks.ts +++ b/src/cli/hooks.ts @@ -1,15 +1,16 @@ -// Quest's settings in the user's own Claude Code settings: what `quest setup` -// writes, and `quest hook`, which handles every hook it installs. Repositories -// write no hooks, since their settings would bind every contributor, so a user -// opts in once for all their repositories. Every hook runs the same command and -// the handler dispatches on the event, so a new hook is one more row in `HOOKS` -// and one more case in `hook`. +// Quest's settings in the user's own Claude Code and Codex settings: what +// `quest setup` writes, and `quest hook`, which handles every hook it installs. +// Repositories write no hooks, since their settings would bind every +// contributor, so a user opts in once for all their repositories. Every hook +// runs the same command and the handler dispatches on the event, so a new hook +// is one more row in an agent's hooks and one more case in `hook`. import { existsSync, mkdirSync, readFileSync, writeFileSync } from "node:fs"; import { homedir } from "node:os"; import { dirname, join } from "node:path"; import { REFERENCE_MARKER, lines } from "../core"; +import { broken } from "./rules"; import { CONTEXT } from "./skills"; /** The command every Quest entry runs; `quest setup` recognizes its own entries by it. */ @@ -18,23 +19,58 @@ export const HOOK_COMMAND = "quest hook"; /** Claude Code truncates longer hook context to a preview and a file path it never asks Claude to read. */ export const CONTEXT_LIMIT = 10_000; -/** One installed entry: the event, and the matcher narrowing it, if any. */ +/** One installed entry: the event, the matcher narrowing it, if any, and the handler's settings beyond its command. */ export interface HookEntry { event: string; matcher?: string; + settings?: Record; } +/** When a session starts, resumes, or clears, and after compaction, which summarizes earlier output away; a fork already holds the rules. */ +const SESSION = "startup|resume|clear|compact"; + /** - * The entries `quest setup` installs. SessionStart prints the rules when a - * session starts, resumes, or clears, and again after compaction, which - * summarizes earlier output away; a fork already holds them. SubagentStart - * gives each subagent the same rules, since it starts without the session's. + * The entries `quest setup` installs for Claude Code. SessionStart prints the + * rules; SubagentStart gives each subagent the same rules, since it starts + * without the session's. PreToolUse blocks the calls that break a rule a + * program can check. */ -export const HOOKS: readonly HookEntry[] = [ - { event: "SessionStart", matcher: "startup|resume|clear|compact" }, +export const CLAUDE_HOOKS: readonly HookEntry[] = [ + { event: "SessionStart", matcher: SESSION }, { event: "SubagentStart" }, + { event: "PreToolUse", matcher: "Bash|Write|Edit|MultiEdit" }, ]; +/** + * The same for Codex, whose shell commands match as Bash and whose file edits + * are `apply_patch`. Codex spills hook context beyond a token limit, about + * 2,500 by default, to a file; the rules' character cap bounds their tokens. + */ +export const CODEX_HOOKS: readonly HookEntry[] = [ + { event: "SessionStart", matcher: SESSION, settings: { additionalContextLimit: CONTEXT_LIMIT } }, + { event: "PreToolUse", matcher: "Bash|apply_patch" }, +]; + +/** A settings file `quest setup` writes: its path, its hooks, and whether it also holds Claude Code's foreground timeout cap. */ +export interface Target { + name: string; + path: string; + hooks: readonly HookEntry[]; + timeout: boolean; +} + +/** + * The settings `quest setup` configures: Claude Code's always, and Codex's + * `hooks.json` when its home directory exists, in `CODEX_HOME` when set, else + * `~/.codex`. + */ +export function targets(env: NodeJS.ProcessEnv = process.env): Target[] { + const claude = { name: "Claude Code", path: settingsPath(env), hooks: CLAUDE_HOOKS, timeout: true }; + const codexHome = env.CODEX_HOME || join(homedir(), ".codex"); + if (!existsSync(codexHome)) return [claude]; + return [claude, { name: "Codex", path: join(codexHome, "hooks.json"), hooks: CODEX_HOOKS, timeout: false }]; +} + /** * Claude Code's cap on a foreground command's timeout: 55 minutes, just under the * 1h prompt-cache TTL, so a blocking wait returns while the session's cache is warm. @@ -50,14 +86,15 @@ export function settingsPath(env: NodeJS.ProcessEnv = process.env): string { type Json = Record; /** - * Set Quest's settings in the Claude Code settings file at `path`: the - * foreground timeout cap and `entries` as Quest's hooks. With `remove`, take - * Quest's hooks out, and the cap while it still holds Quest's value. Keeps - * every other setting and hook, and writes once. Returns a line per change. A - * file that is not a JSON object, or whose `env` or `hooks` is malformed, - * throws before anything is written. + * Set Quest's settings in `target`'s file: its hooks, and the foreground + * timeout cap where it holds one. With `remove`, take Quest's hooks out, and + * the cap while it still holds Quest's value. Keeps every other setting and + * hook, and writes once. Returns a line per change. A file that is not a JSON + * object, or whose `env` or `hooks` is malformed, throws before anything is + * written. */ -export function setup(path: string, options: { remove?: boolean; entries?: readonly HookEntry[] } = {}): string[] { +export function setup(target: Target, remove = false): string[] { + const { path } = target; const original = existsSync(path) ? readFileSync(path, "utf8") : null; let settings: unknown = {}; if (original !== null) { @@ -70,13 +107,15 @@ export function setup(path: string, options: { remove?: boolean; entries?: reado if (!isObject(settings)) throw new Error(`${path} does not hold a JSON object`); const env = settings.env ?? {}; if (!isObject(env)) throw new Error(`${path}: env is not an object`); - const hooked = withHooks(settings, options.remove ? [] : (options.entries ?? HOOKS), path); + const hooked = withHooks(settings, remove ? [] : target.hooks, path); const changes: string[] = []; let updated = hooked; - if (hooked !== settings) changes.push(`${path}: ${options.remove ? "removed" : "installed"} Quest's hooks`); + if (hooked !== settings) changes.push(`${path}: ${remove ? "removed" : "installed"} Quest's hooks`); const cap = env.BASH_MAX_TIMEOUT_MS; - if (options.remove) { + if (!target.timeout) { + // Only Claude Code's settings hold the cap. + } else if (remove) { if (cap === BASH_MAX_TIMEOUT_MS) { const { BASH_MAX_TIMEOUT_MS: _, ...rest } = env; updated = { ...updated, env: rest }; @@ -116,18 +155,23 @@ function withHooks(settings: Json, entries: readonly HookEntry[], path: string): if (handlers.length === group.hooks.length) return [group]; const only = group.hooks.length === 1 && Object.keys(group).every((key) => ["matcher", "hooks"].includes(key)); - if (only && (group.matcher === undefined || typeof group.matcher === "string")) - installed.push({ event, ...(group.matcher === undefined ? {} : { matcher: group.matcher as string }) }); - else scattered = true; + if (only && (group.matcher === undefined || typeof group.matcher === "string")) { + const { type: _, command: __, ...settings } = group.hooks[0] as Json; + installed.push({ + event, + ...(group.matcher === undefined ? {} : { matcher: group.matcher as string }), + ...(Object.keys(settings).length === 0 ? {} : { settings }), + }); + } else scattered = true; return handlers.length > 0 ? [{ ...group, hooks: handlers }] : []; }); if (left.length > 0 || groups.length === 0) kept[event] = left; } if (!scattered && sameEntries(installed, entries)) return settings; - for (const { event, matcher } of entries) { + for (const { event, matcher, settings: extra } of entries) { const group = { ...(matcher === undefined ? {} : { matcher }), - hooks: [{ type: "command", command: HOOK_COMMAND }], + hooks: [{ type: "command", command: HOOK_COMMAND, ...extra }], }; kept[event] = [...((kept[event] as unknown[] | undefined) ?? []), group]; } @@ -143,7 +187,8 @@ function isOurs(handler: unknown): boolean { } function sameEntries(a: readonly HookEntry[], b: readonly HookEntry[]): boolean { - const key = (entry: HookEntry) => `${entry.event}\0${entry.matcher ?? ""}`; + const key = (entry: HookEntry) => + `${entry.event}\0${entry.matcher ?? ""}\0${JSON.stringify(Object.entries(entry.settings ?? {}).sort())}`; const left = a.map(key).sort(); const right = b.map(key).sort(); return left.length === right.length && left.every((value, i) => value === right[i]); @@ -153,17 +198,21 @@ function isObject(value: unknown): value is Json { return typeof value === "object" && value !== null && !Array.isArray(value); } -/** The fields of a Claude Code hook's JSON input that `quest hook` reads. */ +/** The fields of a Claude Code or Codex hook's JSON input that `quest hook` reads. */ interface HookInput { hook_event_name: string; cwd: string; + tool_name?: string; + tool_input?: Record; } /** - * What `quest hook` prints for the Claude Code hook event in `input`, or "" - * when it adds nothing. SessionStart and SubagentStart add `quest context` to - * the session's context in a repository whose root instructions carry Quest's - * reference line, so its agents never run the command themselves. + * What `quest hook` prints for the Claude Code or Codex hook event in `input`, + * or "" when it adds nothing, in a repository whose root instructions carry + * Quest's reference line. SessionStart and SubagentStart add `quest context` + * to the session's context, so its agents never run the command themselves. + * PreToolUse denies a tool call that breaks a rule `broken` checks, giving the + * rule as the reason. */ export function hook(input: unknown): string { const value = input as Partial | null; @@ -175,6 +224,25 @@ export function hook(input: unknown): string { case "SubagentStart": if (!usesQuest(value.cwd)) return ""; return JSON.stringify({ hookSpecificOutput: { hookEventName: event, additionalContext: CONTEXT } }); + case "PreToolUse": { + if ( + typeof value.tool_name !== "string" || + typeof value.tool_input !== "object" || + value.tool_input === null + ) + throw new Error("expected a PreToolUse hook's JSON input with tool_name and tool_input"); + if (!usesQuest(value.cwd)) return ""; + const rules = broken({ tool: value.tool_name, input: value.tool_input, cwd: value.cwd }); + if (rules.length === 0) return ""; + const reason = rules.join("\n"); + return JSON.stringify({ + hookSpecificOutput: { + hookEventName: event, + permissionDecision: "deny", + permissionDecisionReason: reason, + }, + }); + } default: throw new Error(`quest hook does not handle ${event}; run \`quest setup\` again to update your hooks`); } diff --git a/src/cli/main.ts b/src/cli/main.ts index 094e850d..565d3b21 100644 --- a/src/cli/main.ts +++ b/src/cli/main.ts @@ -64,7 +64,8 @@ import { roundLines, } from "./review"; import { type Recorded, pullModels, recordRound } from "./review-record"; -import { BASH_MAX_TIMEOUT_MS, HOOKS, hook, settingsPath, setup } from "./hooks"; +import { BASH_MAX_TIMEOUT_MS, CLAUDE_HOOKS, CODEX_HOOKS, type HookEntry, hook, setup, targets } from "./hooks"; +import { CHECKED_CONTEXT, USER_ASKED } from "./rules"; import { heal, init, uninstall } from "./setup"; import { Refusal, describe, notUpstream, releaseQuest, startQuest } from "./start"; import { CONTEXT, GUIDE, GUIDE_SECTIONS, SKILLS, guideSections, sectionHeading, skill } from "./skills"; @@ -1307,11 +1308,12 @@ export function main(argv: string[], out: Output): number | Promise { .command("context") .summary("Print the engineering rules agents follow in every task, before the repository's own") .description( - "Print the engineering rules agents follow in every task, before the repository's own. The hook " + - "`quest setup` installs adds them to each Claude Code session in a repository that uses Quest, so " + - "its agents need not run this.", + "Print the engineering rules agents follow in every task, before the repository's own. The hooks " + + "`quest setup` installs add them to each Claude Code and Codex session in a repository that uses " + + "Quest, so its agents need not run this, and check the rules under its last section, Checked, on " + + "each tool call, so the context they add leaves that section out.", ) - .action(() => out.stdout(CONTEXT)); + .action(() => out.stdout(`${CONTEXT}\n${CHECKED_CONTEXT}`)); program .command("skill") @@ -1334,36 +1336,52 @@ export function main(argv: string[], out: Output): number | Promise { out.stdout(found.instructions); }); + const listed = (hooks: readonly HookEntry[]) => + hooks.map((h) => (h.matcher ? `${h.event} (${h.matcher})` : h.event)).join(", "); program .command("setup") - .summary("Configure your own Claude Code settings for quest work") + .summary("Configure your own Claude Code and Codex settings for quest work") .description( "Configure your own Claude Code settings for quest work, in $CLAUDE_CONFIG_DIR/settings.json or else " + - "~/.claude/settings.json, keeping every other setting and hook. Touches no repository.\n\n" + - `Sets env.BASH_MAX_TIMEOUT_MS to ${BASH_MAX_TIMEOUT_MS}, so a foreground command, such as a blocking ` + + "~/.claude/settings.json, and Codex's in $CODEX_HOME/hooks.json or else ~/.codex/hooks.json when " + + "that directory exists, keeping every other setting and hook. Touches no repository.\n\n" + + `Sets Claude Code's env.BASH_MAX_TIMEOUT_MS to ${BASH_MAX_TIMEOUT_MS}, so a foreground command, such as a blocking ` + "wait, may run 55 minutes: just under the 1h prompt-cache TTL. " + - `Installs ${HOOKS.map((h) => (h.matcher ? `${h.event} (${h.matcher})` : h.event)).join(" and ")} hooks, ` + - "which add `quest context` to each session and subagent in a repository that uses Quest, and again " + - "after compaction, so agents never run it themselves. Each runs `quest hook`, so `quest` must be on " + - "the PATH Claude Code starts with.\n\n" + - "Validates the whole file, then writes it once and prints each change; settings that are not a JSON " + + `Installs Claude Code's ${listed(CLAUDE_HOOKS)} hooks and Codex's ${listed(CODEX_HOOKS)} hooks. ` + + "The start hooks add `quest context` to each session and Claude Code subagent in a repository that " + + "uses Quest, and again after compaction, so agents never run it themselves. PreToolUse blocks tool " + + "calls there that break a rule a program can check: em dashes an agent writes, a push without an " + + "explicit refspec, a force-push or rebase, a GitHub post without its `(Written by ...)` footer, and " + + "scratch files in /tmp. Each runs `quest hook`, so `quest` must be on the PATH the agent starts " + + "with, and Codex asks to trust its hooks once through /hooks.\n\n" + + "Validates each whole file, then writes it once and prints each change; settings that are not a JSON " + "object, or whose env or hooks are malformed, are an error and stay unchanged. Rerun it after " + "upgrading Quest; it replaces Quest's earlier hooks and changes nothing when all is set.", ) .option("--remove", "Remove Quest's hooks, and env.BASH_MAX_TIMEOUT_MS while it still holds Quest's value") .action((options: { remove?: boolean }) => { - for (const line of setup(settingsPath(), options)) print(line); + for (const target of targets()) { + const changes = setup(target, options.remove); + for (const line of changes) print(line); + if (target.name === "Codex" && changes.length > 0 && !options.remove) + print(`${target.path}: review and trust Quest's hooks with /hooks in Codex`); + } }); program .command("hook") - .summary("Handle a Claude Code hook event `quest setup` installed, read on stdin") + .summary("Handle a Claude Code or Codex hook event `quest setup` installed, read on stdin") .description( - "Handle the Claude Code hook event whose JSON input arrives on stdin, for the entries `quest setup` " + - "installs. SessionStart and SubagentStart print `quest context` as the session's or subagent's " + - "additional context, when the root AGENTS.md or CLAUDE.md of the checkout holding the input's cwd " + - "has a line starting with `Quests: `, as `quest init` writes, and nothing otherwise. Exits 1 on " + - "malformed input or an event it does not handle.", + "Handle the Claude Code or Codex hook event whose JSON input arrives on stdin, for the entries " + + "`quest setup` installs, in a repository whose root AGENTS.md or CLAUDE.md, in the checkout holding " + + "the input's cwd, has a line starting with `Quests: `, as `quest init` writes; elsewhere it prints " + + "nothing. SessionStart and SubagentStart print `quest context` as the session's or subagent's " + + "additional context. PreToolUse denies a call that breaks a checked rule, naming the rule: an edit " + + "or a commit message or new GitHub post that adds an em dash outside a `>` quote line, a `git push` " + + "whose refspecs lack a `:`, a force-push, a `git rebase` or `git pull --rebase`, a new GitHub post " + + "whose body lacks a `(Written by ...)` line, or a file written under /tmp. A force-push or rebase the " + + `user asked for passes with the \`${USER_ASKED}\` prefix. Exits 1 on malformed input or an event it ` + + "does not handle.", ) .action(() => { out.stdout(hook(JSON.parse(readFileSync(0, "utf8")))); diff --git a/src/cli/rules.ts b/src/cli/rules.ts new file mode 100644 index 00000000..099cf4bb --- /dev/null +++ b/src/cli/rules.ts @@ -0,0 +1,334 @@ +// The rules from `quest context` that a program can check, enforced on an +// agent's tool calls by `quest hook` instead of read as prose. Each check +// stays narrow enough to never block legitimate work: it judges only what the +// call itself shows, and lets through what it cannot tell apart. + +import { existsSync, readFileSync } from "node:fs"; +import { dirname, isAbsolute, join } from "node:path"; + +import { parse } from "./shell"; + +const EM_DASH = String.fromCharCode(0x2014); + +/** The prefix with which an agent attests that the user asked, in their own words, for a force-push or rebase. */ +export const USER_ASKED = "QUEST_USER_ASKED=1"; + +/** Each checked rule, as `quest hook` gives it when denying a call that breaks it. */ +export const RULES = { + emDash: "No em dashes in what you write: use a comma, colon, parentheses, or separate sentences. Text quoted from someone else keeps its em dashes on a `>` quote line.", + footer: "End every GitHub post you author with a `(Written by with )` line, naming the running model and its reasoning effort, or `(Written by )` when the harness doesn't report the effort; never guess it.", + refspec: + "Push with an explicit refspec, `git push refs/heads/:refs/heads/`: a bare branch name can push to the base instead, as under `push.default=upstream` when the branch tracks it.", + rewrite: + "Never rebase or force-push, even a branch only you hold; merge the base instead, when the branch conflicts with it or a landing step refuses the head.", + tmp: "Put scratch files, such as PR bodies, logs, patches, helper scripts, and temporary clones, in the checkout's `.tmp/`, never `/tmp`, so they disappear with the worktree.", +} as const; + +/** + * The checked rules as a section of `quest context`, for an agent no hook + * checks them for, such as one whose user never ran `quest setup`. The hook + * leaves it out of the context it adds. + */ +export const CHECKED_CONTEXT = `## Checked\n\n${Object.values(RULES) + .map((rule) => `- ${rule}`) + .join("\n")}\n`; + +/** What a denial adds to `rewrite`: the way through for a rebase or force-push the user asked for. */ +const ASKED = ` If the user asked for this in their own words, prefix the command with \`${USER_ASKED}\`.`; + +/** A tool call as Claude Code and Codex hand it to a `PreToolUse` hook. */ +export interface ToolCall { + tool: string; + input: Record; + cwd: string; +} + +/** The rules `call` breaks, each said once, in the order of `RULES`. */ +export function broken(call: ToolCall): string[] { + const state = check(call.cwd); + const { found, scratch } = state; + const text = (key: string) => (typeof call.input[key] === "string" ? (call.input[key] as string) : undefined); + const path = text("file_path"); + switch (call.tool) { + case "Bash": { + const command = text("command"); + if (command !== undefined) shell(command, state); + break; + } + case "Write": + if (path !== undefined && scratch(path)) found.add("tmp"); + if (path !== undefined && adds(read(call.cwd, path) ?? "", text("content") ?? "")) found.add("emDash"); + break; + case "Edit": + case "MultiEdit": { + if (path !== undefined && scratch(path)) found.add("tmp"); + const edits = Array.isArray(call.input.edits) + ? (call.input.edits as Record[]) + : [call.input]; + for (const edit of edits) + if (adds(String(edit.old_string ?? ""), String(edit.new_string ?? ""))) found.add("emDash"); + break; + } + case "apply_patch": { + const patch = text("command"); + if (patch !== undefined) applyPatch(patch, state); + break; + } + } + return (Object.keys(RULES) as Rule[]) + .filter((key) => found.has(key)) + .map((key) => (key === "rewrite" ? RULES[key] + ASKED : RULES[key])); +} + +/** Em dashes outside `>` quote lines. */ +function dashes(text: string): number { + let count = 0; + for (const line of text.split("\n")) if (!/^\s*>/.test(line)) count += line.split(EM_DASH).length - 1; + return count; +} + +/** Whether replacing `before` with `after` writes em dashes of its own, rather than keeping ones already there. */ +function adds(before: string, after: string): boolean { + return dashes(after) > dashes(before); +} + +type Rule = keyof typeof RULES; + +/** One call's check: the rules found broken so far, and where its relative paths resolve. */ +interface Check { + found: Set; + cwd: string; + /** Whether `path` is scratch under `/tmp`, written as is or through `$TMPDIR`, outside the checkout holding `cwd`. */ + scratch: (path: string) => boolean; +} + +function check(cwd: string): Check { + const root = checkoutRoot(cwd); + return { + found: new Set(), + cwd, + scratch: (path) => /^(\/tmp|\$TMPDIR|\$\{TMPDIR[^}]*\})(\/|$)/.test(path) && !`${path}/`.startsWith(`${root}/`), + }; +} + +/** The checkout holding `cwd`, found without running git, or `cwd` outside any. */ +function checkoutRoot(cwd: string): string { + for (let dir = cwd; ; dir = dirname(dir)) { + if (existsSync(join(dir, ".git"))) return dir; + if (dirname(dir) === dir) return cwd; + } +} + +function read(cwd: string, path: string): string | undefined { + try { + return readFileSync(isAbsolute(path) ? path : join(cwd, path), "utf8"); + } catch { + return undefined; + } +} + +/** Codex's `apply_patch` envelope: its file headers and its `+`/`-` lines. */ +function applyPatch(patch: string, check: Check) { + let added = ""; + let removed = ""; + for (const line of patch.split("\n")) { + const header = /^\*\*\* (?:Add File|Update File|Delete File|Move to): (.+)$/.exec(line); + if (header) { + if (check.scratch(header[1]!.trim())) check.found.add("tmp"); + } else if (line.startsWith("+")) added += `${line.slice(1)}\n`; + else if (line.startsWith("-")) removed += `${line.slice(1)}\n`; + } + if (adds(removed, added)) check.found.add("emDash"); +} + +/** + * The command `argv` runs once wrappers such as `env`, `timeout`, `direnv + * exec`, `nix develop --command`, and `quest gate` are peeled off, with the + * variable assignments in front of it added to `env`; `null` for none. + */ +function unwrap(argv: string[], env: Set): string[] | null { + let rest = argv; + for (;;) { + while (rest.length > 0 && /^[A-Za-z_][A-Za-z0-9_]*=/.test(rest[0]!)) env.add(rest.shift()!); + const [name, ...args] = rest; + if (name === undefined) return null; + if (["command", "exec", "nohup", "time", "builtin", "env", "nice"].includes(name)) rest = dropOptions(args); + else if (name === "timeout") rest = dropOptions(args).slice(1); + else if (name === "direnv" && args[0] === "exec") rest = args.slice(2); + else if (name === "quest" && args[0] === "gate") rest = args.slice(1); + else if (name === "nix" && ["develop", "shell"].includes(args[0] ?? "")) { + const at = args.findIndex((arg) => arg === "--command" || arg === "-c"); + if (at === -1) return rest; + rest = args.slice(at + 1); + } else return rest; + } +} + +/** `args` past its leading options, where `-n`, `-s`, `-k`, and `-u` take a value. */ +function dropOptions(args: string[]): string[] { + let i = 0; + while (i < args.length && args[i]!.startsWith("-")) i += /^-[nsku]$/.test(args[i]!) ? 2 : 1; + return args.slice(i); +} + +/** Check each simple command in a shell command line, following `sh -c` into its script. */ +function shell(line: string, check: Check) { + const { found, scratch, cwd } = check; + const written = new Map(); + for (const simple of parse(line)) { + const env = new Set(); + const argv = unwrap(simple.argv, env); + for (const { op, target } of simple.redirects) { + if ([">", ">>", ">|", "&>", "&>>"].includes(op)) { + if (scratch(target)) found.add("tmp"); + if (simple.stdin !== undefined) written.set(target, simple.stdin); + } + } + if (argv === null) continue; + const [name, ...args] = argv; + if (["bash", "sh", "zsh", "dash"].includes(name!)) { + const flag = args.findIndex((arg) => /^-[a-z]*c[a-z]*$/.test(arg)); + if (flag !== -1 && args[flag + 1] !== undefined) shell(args[flag + 1]!, check); + continue; + } + const asked = env.has(USER_ASKED); + const from = simple.redirects.find((redirect) => redirect.op === "<")?.target; + const stdin = simple.stdin ?? (from === undefined ? undefined : (written.get(from) ?? read(cwd, from))); + const input = (file: string) => (file === "-" ? stdin : (written.get(file) ?? read(cwd, file))); + if (name === "tee") { + if (args.some((arg) => !arg.startsWith("-") && scratch(arg))) found.add("tmp"); + for (const arg of args) + if (!arg.startsWith("-") && simple.stdin !== undefined) written.set(arg, simple.stdin); + } else if (name === "mkdir" || name === "touch") { + if (args.some(scratch)) found.add("tmp"); + } else if (["cp", "mv", "ln", "rsync"].includes(name!)) { + const last = args.filter((arg) => !arg.startsWith("-")).at(-1); + if (last !== undefined && scratch(last)) found.add("tmp"); + } else if (name === "mktemp") mktemp(args, check); + else if (name === "git") git(args, asked, input, check); + else if (name === "gh") gh(args, input, check); + } +} + +/** `mktemp` creates under `/tmp` unless given a directory or a template outside it. */ +function mktemp(args: string[], check: Check) { + let dir: string | undefined; + let tmpdir = false; + let template: string | undefined; + for (let i = 0; i < args.length; i++) { + const arg = args[i]!; + if (arg === "-p") dir = args[++i]; + else if (arg.startsWith("--tmpdir=")) dir = arg.slice("--tmpdir=".length); + else if (arg === "--tmpdir" || /^-[a-z]*t[a-z]*$/.test(arg)) tmpdir = true; + else if (!arg.startsWith("-")) template = arg; + } + if (dir !== undefined) { + if (check.scratch(dir)) check.found.add("tmp"); + } else if (template === undefined || tmpdir) check.found.add("tmp"); + else if (check.scratch(template)) check.found.add("tmp"); +} + +/** Options of `git` itself that take the next word. */ +const GIT_VALUE_OPTIONS = new Set(["-C", "-c", "--git-dir", "--work-tree", "--namespace", "--exec-path"]); + +function git(argv: string[], asked: boolean, input: (file: string) => string | undefined, check: Check) { + let i = 0; + while (i < argv.length && argv[i]!.startsWith("-")) i += GIT_VALUE_OPTIONS.has(argv[i]!) ? 2 : 1; + const [sub, ...args] = argv.slice(i); + if (sub === "push") push(args, asked, check); + else if (sub === "rebase") { + if (!asked && !args.some((arg) => ["--abort", "--quit", "--show-current-patch"].includes(arg))) + check.found.add("rewrite"); + } else if (sub === "pull") { + const rebase = args.some((arg) => /^(-r|--rebase(=(true|merges|interactive|i))?)$/.test(arg)); + if (rebase && !asked) check.found.add("rewrite"); + } else if (sub === "commit") { + const message = values(args, ["-m", "--message"], { attached: ["-m"] }).join("\n"); + const files = values(args, ["-F", "--file"], { attached: ["-F"] }).map(input); + if (adds("", [message, ...files].join("\n"))) check.found.add("emDash"); + } else if (sub === "clone" || (sub === "worktree" && args[0] === "add")) { + if (args.some((arg) => !arg.startsWith("-") && check.scratch(arg))) check.found.add("tmp"); + } +} + +/** `git push` options that take the next word. */ +const PUSH_VALUE_OPTIONS = new Set(["-o", "--push-option", "--repo", "--receive-pack", "--exec"]); + +function push(args: string[], asked: boolean, check: Check) { + const positionals: string[] = []; + const flags: string[] = []; + for (let i = 0; i < args.length; i++) { + const arg = args[i]!; + if (arg === "--") positionals.push(...args.slice(i + 1)); + if (arg === "--") break; + if (PUSH_VALUE_OPTIONS.has(arg)) i++; + else if (arg.startsWith("-")) flags.push(arg); + else positionals.push(arg); + } + const refspecs = positionals.slice(1); + const forced = + refspecs.some((spec) => spec.startsWith("+")) || + flags.some((flag) => /^(--force|--force-with-lease(=.*)?|--mirror|-[a-zA-Z]*f[a-zA-Z]*)$/.test(flag)); + if (forced && !asked) check.found.add("rewrite"); + const deleting = flags.some((flag) => flag === "--delete" || /^-[a-zA-Z]*d[a-zA-Z]*$/.test(flag)); + const whole = flags.some((flag) => ["--tags", "--all", "--branches", "--mirror"].includes(flag)); + if (whole || deleting) return; + if (refspecs.length === 0 || refspecs.some((spec) => !spec.includes(":"))) check.found.add("refspec"); +} + +/** + * The values `args` gives the options in `names`, as `--name value`, + * `--name=value`, or, for the short options in `attached`, `-nvalue`. + */ +function values(args: string[], names: string[], { attached = [] as string[] } = {}): string[] { + const found: string[] = []; + for (let i = 0; i < args.length; i++) { + const arg = args[i]!; + const long = names.find((name) => name.startsWith("--") && arg.startsWith(`${name}=`)); + if (names.includes(arg) && i + 1 < args.length) found.push(args[++i]!); + else if (long !== undefined) found.push(arg.slice(long.length + 1)); + else { + const short = attached.find((name) => arg.startsWith(name) && arg.length > name.length); + if (short !== undefined) found.push(arg.slice(short.length)); + } + } + return found; +} + +/** `gh` subcommands that post text, by group and command, and whether each authors a new post. */ +const POSTS: Record> = { + pr: { create: true, comment: true, review: true, edit: false }, + issue: { create: true, comment: true, edit: false }, +}; + +function gh(args: string[], input: (file: string) => string | undefined, check: Check) { + const [group, command, ...rest] = args; + let body: (string | undefined)[]; + let titles: string[] = []; + let authored: boolean; + if (group === "api") { + const fields = [command ?? "", ...rest]; + const raw = values(fields, ["-f", "--raw-field"]).filter((field) => field.startsWith("body=")); + const typed = values(fields, ["-F", "--field"]).filter((field) => field.startsWith("body=")); + body = [ + ...raw.map((field) => field.slice("body=".length)), + ...typed + .map((field) => field.slice("body=".length)) + .map((value) => (value.startsWith("@") ? input(value.slice(1)) : value)), + ]; + const method = values(fields, ["-X", "--method"]).at(-1)?.toUpperCase(); + authored = method !== "PATCH" && method !== "PUT"; + } else { + const post = POSTS[group ?? ""]?.[command ?? ""]; + if (post === undefined) return; + authored = post; + body = [...values(rest, ["-b", "--body"]), ...values(rest, ["-F", "--body-file"]).map((file) => input(file))]; + titles = values(rest, ["-t", "--title"]); + } + if (body.length === 0) return; + // A body that cannot be read here, such as one piped in, is left to the prose. + if (body.some((text) => text === undefined)) return; + const text = body.join("\n"); + if (authored && adds("", [...titles, text].join("\n"))) check.found.add("emDash"); + if (authored && !/^\(written by .+\)\s*$/im.test(text)) check.found.add("footer"); +} diff --git a/src/cli/shell.ts b/src/cli/shell.ts new file mode 100644 index 00000000..4176096e --- /dev/null +++ b/src/cli/shell.ts @@ -0,0 +1,147 @@ +// Splits a shell command line into its simple commands, closely enough for +// `quest hook` to judge what an agent's command does. It removes quoting, +// attaches redirections and here-documents to their command, and leaves +// expansions as written: nothing runs. + +/** A redirection: its operator and the word after it. */ +export interface Redirect { + op: string; + target: string; +} + +/** One simple command: its words, redirections, and the text a here-document or here-string feeds its stdin. */ +export interface Simple { + argv: string[]; + redirects: Redirect[]; + stdin?: string; +} + +const SEPARATORS = new Set([";", "&", "|", "(", ")", "\n"]); + +/** + * The simple commands in `line`, in order, across `;`, `&&`, `||`, pipes, + * subshells, and newlines. Command substitutions stay inside their word. + */ +export function parse(line: string): Simple[] { + const commands: Simple[] = []; + const heredocs: { command: Simple; delimiter: string; tabs: boolean }[] = []; + let command: Simple = { argv: [], redirects: [] }; + let word: string | null = null; + let redirect: string | null = null; + let i = 0; + + const endWord = () => { + if (word === null) return; + if (redirect === null) command.argv.push(word); + else if (redirect === "<<" || redirect === "<<-") + heredocs.push({ command, delimiter: word, tabs: redirect === "<<-" }); + else if (redirect === "<<<") command.stdin = `${word}\n`; + else command.redirects.push({ op: redirect, target: word }); + word = null; + redirect = null; + }; + const endCommand = () => { + endWord(); + if (command.argv.length > 0 || command.redirects.length > 0 || heredocs.some((h) => h.command === command)) + commands.push(command); + command = { argv: [], redirects: [] }; + }; + const append = (text: string) => { + word = (word ?? "") + text; + }; + /** The index just past the `)` closing a `$(` or `(` opened before `from`. */ + const closing = (from: number): number => { + let depth = 1; + let j = from; + while (j < line.length && depth > 0) { + const c = line[j]!; + if (c === "\\") j++; + else if (c === "'") j = line.indexOf("'", j + 1) === -1 ? line.length : line.indexOf("'", j + 1); + else if (c === "(") depth++; + else if (c === ")") depth--; + j++; + } + return j; + }; + + while (i < line.length) { + const c = line[i]!; + if (c === "\\") { + if (line[i + 1] === "\n") i += 2; + else { + append(line[i + 1] ?? ""); + i += 2; + } + } else if (c === "'") { + const end = line.indexOf("'", i + 1); + append(line.slice(i + 1, end === -1 ? line.length : end)); + i = end === -1 ? line.length : end + 1; + } else if (c === '"') { + let text = ""; + i++; + while (i < line.length && line[i] !== '"') { + if (line[i] === "\\" && '"\\$`\n'.includes(line[i + 1] ?? "")) { + if (line[i + 1] !== "\n") text += line[i + 1]; + i += 2; + } else if (line[i] === "$" && line[i + 1] === "(") { + const end = closing(i + 2); + text += line.slice(i, end); + i = end; + } else text += line[i++]; + } + append(text); + i++; + } else if (c === "$" && line[i + 1] === "'") { + const end = line.indexOf("'", i + 2); + append(line.slice(i + 2, end === -1 ? line.length : end).replace(/\\n/g, "\n")); + i = end === -1 ? line.length : end + 1; + } else if (c === "$" && line[i + 1] === "(") { + const end = closing(i + 2); + append(line.slice(i, end)); + i = end; + } else if (c === "`") { + const end = line.indexOf("`", i + 1); + append(line.slice(i, end === -1 ? line.length : end + 1)); + i = end === -1 ? line.length : end + 1; + } else if (c === "#" && word === null) { + while (i < line.length && line[i] !== "\n") i++; + } else if (c === " " || c === "\t") { + endWord(); + i++; + } else if (c === ">" || c === "<" || (c === "&" && line[i + 1] === ">")) { + // A word of digits right before the operator names the descriptor. + if (word !== null && /^\d+$/.test(word) && redirect === null) word = null; + endWord(); + const op = /^(&>>|&>|>>|>\||>&|>|<<<|<<-|<<|<&|<>|<)/.exec(line.slice(i))![1]!; + i += op.length; + redirect = op; + } else if (SEPARATORS.has(c)) { + endCommand(); + i += c !== "\n" && line[i + 1] === c ? 2 : 1; + if (c === "\n") i = readHeredocs(i); + } else { + append(c); + i++; + } + } + endCommand(); + return commands; + + /** Read the bodies of the here-documents opened on the line just ended, returning the index after them. */ + function readHeredocs(from: number): number { + let at = from; + for (const { command: owner, delimiter, tabs } of heredocs.splice(0)) { + const body: string[] = []; + while (at < line.length) { + const end = line.indexOf("\n", at); + const raw = line.slice(at, end === -1 ? line.length : end); + at = end === -1 ? line.length : end + 1; + const text = tabs ? raw.replace(/^\t+/, "") : raw; + if (text === delimiter) break; + body.push(text); + } + owner.stdin = body.length > 0 ? `${body.join("\n")}\n` : ""; + } + return at; + } +} diff --git a/src/runner/prompt.ts b/src/runner/prompt.ts index bacf90b0..d1efbd09 100644 --- a/src/runner/prompt.ts +++ b/src/runner/prompt.ts @@ -7,6 +7,7 @@ import { execFileSync } from "node:child_process"; import { lstatSync, readFileSync } from "node:fs"; import { join } from "node:path"; +import { CHECKED_CONTEXT } from "../cli/rules"; import { CONTEXT, compose, guideSections, skill } from "../cli/skills"; import type { Job, Prompt, Question } from "./protocol"; @@ -107,7 +108,8 @@ function task(job: Job): string { /** The workflow's own instructions, from the skills this binary ships, with what each builds on. */ function workflow(job: Job): string { const name = { start: "start", iterate: "iterate", merge: "merge", plan: "plan", bootstrap: null }[job.workflow]; - const parts = [`# Engineering context\n\n${CONTEXT.trim()}`]; + // The runner's agents run no `quest setup` hooks, so the checked rules stay prose for them. + const parts = [`# Engineering context\n\n${CONTEXT.trim()}\n\n${CHECKED_CONTEXT.trim()}`]; // Every workflow's work reaches the runner's review, whose rounds follow /quest-review. if (name) parts.push(`# The ${name} workflow\n\n${compose([skill(name)!, skill("review")!]).trim()}`); // Without `quest`, a merge checks the changed quest files against the guide itself. diff --git a/test/hooks.test.ts b/test/hooks.test.ts index 221a8a29..dd97127b 100644 --- a/test/hooks.test.ts +++ b/test/hooks.test.ts @@ -6,12 +6,24 @@ import { join } from "node:path"; import { expect, test } from "vitest"; import { REFERENCE_LINE } from "../src/core"; -import { BASH_MAX_TIMEOUT_MS, HOOKS, HOOK_COMMAND, hook, settingsPath, setup } from "../src/cli/hooks"; +import { + BASH_MAX_TIMEOUT_MS, + CLAUDE_HOOKS, + CODEX_HOOKS, + CONTEXT_LIMIT, + HOOK_COMMAND, + type Target, + hook, + settingsPath, + setup, + targets, +} from "../src/cli/hooks"; import { CONTEXT } from "../src/cli/skills"; import { tempDir } from "./fixture"; const ours = { type: "command", command: HOOK_COMMAND }; const read = (path: string) => JSON.parse(readFileSync(path, "utf8")); +const claude = (path: string): Target => ({ name: "Claude Code", path, hooks: CLAUDE_HOOKS, timeout: true }); test("settings live in CLAUDE_CONFIG_DIR when set, else ~/.claude", () => { expect(settingsPath({ CLAUDE_CONFIG_DIR: "/profiles/work" })).toBe(join("/profiles/work", "settings.json")); @@ -20,33 +32,33 @@ test("settings live in CLAUDE_CONFIG_DIR when set, else ~/.claude", () => { test("setup creates the settings file with the timeout and every hook, then changes nothing", () => { const path = join(tempDir(), "profile", "settings.json"); - expect(setup(path)).toEqual([ + expect(setup(claude(path))).toEqual([ `${path}: installed Quest's hooks`, `${path}: env.BASH_MAX_TIMEOUT_MS=${BASH_MAX_TIMEOUT_MS}`, ]); const settings = read(path); expect(settings.env).toEqual({ BASH_MAX_TIMEOUT_MS }); - for (const { event, matcher } of HOOKS) + for (const { event, matcher } of CLAUDE_HOOKS) expect(settings.hooks[event]).toEqual([{ ...(matcher ? { matcher } : {}), hooks: [ours] }]); const written = readFileSync(path, "utf8"); expect(written.endsWith("}\n")).toBe(true); - expect(setup(path)).toEqual([]); + expect(setup(claude(path))).toEqual([]); expect(readFileSync(path, "utf8")).toBe(written); }); test("setup replaces a different timeout, and removal leaves the user's own", () => { const path = join(tempDir(), "settings.json"); writeFileSync(path, JSON.stringify({ env: { FOO: "1", BASH_MAX_TIMEOUT_MS: "600000" } })); - expect(setup(path)).toContain(`${path}: env.BASH_MAX_TIMEOUT_MS=${BASH_MAX_TIMEOUT_MS} (was "600000")`); + expect(setup(claude(path))).toContain(`${path}: env.BASH_MAX_TIMEOUT_MS=${BASH_MAX_TIMEOUT_MS} (was "600000")`); expect(read(path).env).toEqual({ FOO: "1", BASH_MAX_TIMEOUT_MS }); - expect(setup(path, { remove: true })).toEqual([ + expect(setup(claude(path), true)).toEqual([ `${path}: removed Quest's hooks`, `${path}: removed env.BASH_MAX_TIMEOUT_MS`, ]); expect(read(path)).toEqual({ env: { FOO: "1" } }); writeFileSync(path, JSON.stringify({ env: { BASH_MAX_TIMEOUT_MS: "600000" } })); - expect(setup(path, { remove: true })).toEqual([]); + expect(setup(claude(path), true)).toEqual([]); expect(read(path)).toEqual({ env: { BASH_MAX_TIMEOUT_MS: "600000" } }); }); @@ -54,12 +66,12 @@ test("setup keeps other settings and hooks in place", () => { const path = join(tempDir(), "settings.json"); const theirs = { hooks: [{ type: "command", command: "dev-shell-hook claude", timeout: 600 }] }; writeFileSync(path, JSON.stringify({ theme: "dark", hooks: { SessionStart: [theirs] }, env: { A: "1" } })); - setup(path); + setup(claude(path)); const settings = read(path); expect(Object.keys(settings)).toEqual(["theme", "hooks", "env"]); expect(settings.hooks.SessionStart[0]).toEqual(theirs); expect(settings.hooks.SessionStart).toHaveLength(2); - expect(setup(path, { remove: true })).toHaveLength(2); + expect(setup(claude(path), true)).toHaveLength(2); expect(read(path)).toEqual({ theme: "dark", hooks: { SessionStart: [theirs] }, env: { A: "1" } }); }); @@ -75,21 +87,53 @@ test("setup replaces an earlier version's entries, including one sharing a group }, }), ); - expect(setup(path)).toContain(`${path}: installed Quest's hooks`); + expect(setup(claude(path))).toContain(`${path}: installed Quest's hooks`); const settings = read(path); - expect(settings.hooks.PreToolUse).toEqual([{ matcher: "Bash", hooks: [theirs] }]); - expect(settings.hooks.SessionStart).toEqual([{ matcher: HOOKS[0]!.matcher, hooks: [ours] }]); - expect(setup(path, { entries: [{ event: "Stop" }] })).toEqual([`${path}: installed Quest's hooks`]); + expect(settings.hooks.PreToolUse).toEqual([ + { matcher: "Bash", hooks: [theirs] }, + { matcher: CLAUDE_HOOKS[2]!.matcher, hooks: [ours] }, + ]); + expect(settings.hooks.SessionStart).toEqual([{ matcher: CLAUDE_HOOKS[0]!.matcher, hooks: [ours] }]); + expect(setup({ ...claude(path), hooks: [{ event: "Stop" }] })).toEqual([`${path}: installed Quest's hooks`]); expect(Object.keys(read(path).hooks)).toEqual(["PreToolUse", "Stop"]); }); +test("Codex's hooks file gets its hooks and their settings, but no timeout", () => { + const path = join(tempDir(), "hooks.json"); + const codex: Target = { name: "Codex", path, hooks: CODEX_HOOKS, timeout: false }; + expect(setup(codex)).toEqual([`${path}: installed Quest's hooks`]); + const settings = read(path); + expect(settings.env).toBeUndefined(); + expect(settings.hooks.SessionStart).toEqual([ + { matcher: CODEX_HOOKS[0]!.matcher, hooks: [{ ...ours, additionalContextLimit: CONTEXT_LIMIT }] }, + ]); + expect(setup(codex)).toEqual([]); + // A changed handler setting is an earlier version's entry, replaced. + expect(setup({ ...codex, hooks: [{ event: "SessionStart", settings: { additionalContextLimit: 1 } }] })).toEqual([ + `${path}: installed Quest's hooks`, + ]); + expect(setup(codex, true)).toEqual([`${path}: removed Quest's hooks`]); + expect(read(path)).toEqual({}); +}); + +test("setup configures Codex only where its home exists", () => { + const home = tempDir(); + const env = { CLAUDE_CONFIG_DIR: join(home, "claude"), CODEX_HOME: join(home, "codex") }; + expect(targets(env).map((target) => target.name)).toEqual(["Claude Code"]); + mkdirSync(env.CODEX_HOME); + expect(targets(env).map((target) => target.path)).toEqual([ + join(home, "claude", "settings.json"), + join(home, "codex", "hooks.json"), + ]); +}); + test("removing Quest's only settings drops the hooks and env keys", () => { const path = join(tempDir(), "settings.json"); writeFileSync(path, JSON.stringify({ theme: "dark" })); - setup(path); - setup(path, { remove: true }); + setup(claude(path)); + setup(claude(path), true); expect(read(path)).toEqual({ theme: "dark" }); - expect(setup(path, { remove: true })).toEqual([]); + expect(setup(claude(path), true)).toEqual([]); }); test.each([ @@ -104,8 +148,8 @@ test.each([ ])("setup refuses settings with %s and leaves them unchanged", (_, content) => { const path = join(tempDir(), "settings.json"); writeFileSync(path, content); - expect(() => setup(path)).toThrow(path); - expect(() => setup(path, { remove: true })).toThrow(path); + expect(() => setup(claude(path))).toThrow(path); + expect(() => setup(claude(path), true)).toThrow(path); expect(readFileSync(path, "utf8")).toBe(content); }); @@ -131,6 +175,17 @@ test("SessionStart adds nothing where the repository does not use Quest", () => expect(hook({ hook_event_name: "SessionStart", cwd: tempDir() })).toBe(""); }); +test("PreToolUse denies a call that breaks a checked rule, only where the repository uses Quest", () => { + const cwd = checkout(`${REFERENCE_LINE}\n`); + const call = { hook_event_name: "PreToolUse", cwd, tool_name: "Bash", tool_input: { command: "git push" } }; + const output = JSON.parse(hook(call)); + expect(output.hookSpecificOutput).toMatchObject({ hookEventName: "PreToolUse", permissionDecision: "deny" }); + expect(output.hookSpecificOutput.permissionDecisionReason).toContain("refspec"); + expect(hook({ ...call, tool_input: { command: "git push origin refs/heads/a:refs/heads/a" } })).toBe(""); + expect(hook({ ...call, cwd: checkout(null) })).toBe(""); + expect(() => hook({ hook_event_name: "PreToolUse", cwd })).toThrow("tool_name"); +}); + test("hook refuses malformed input and events it does not handle", () => { expect(() => hook({ cwd: "/" })).toThrow("hook_event_name"); expect(() => hook({ hook_event_name: "Stop", cwd: "/" })).toThrow("quest setup"); diff --git a/test/rules.test.ts b/test/rules.test.ts new file mode 100644 index 00000000..922c7b6b --- /dev/null +++ b/test/rules.test.ts @@ -0,0 +1,175 @@ +// The rules `quest hook` checks on a tool call, and the shell parsing they rest on. + +import { writeFileSync } from "node:fs"; +import { join } from "node:path"; + +import { beforeEach, expect, test } from "vitest"; + +import { RULES, USER_ASKED, broken } from "../src/cli/rules"; +import { parse } from "../src/cli/shell"; +import { tempDir } from "./fixture"; + +const DASH = String.fromCharCode(0x2014); +const DENIED_REWRITE = `${RULES.rewrite} If the user asked for this in their own words, prefix the command with \`${USER_ASKED}\`.`; +const FOOTER = "(Written by Opus 5.5 with high effort)"; +let cwd = ""; +beforeEach(() => { + cwd = tempDir(); +}); +const bash = (command: string) => broken({ tool: "Bash", input: { command }, cwd }); + +test("parse splits commands and attaches redirections and here-documents", () => { + expect( + parse( + `A=1 git push 'a b' "c\\"d" && echo x | tee out.log >/dev/null 2>&1; cat > .tmp/body.md <<'EOF'\nline $x\nEOF\ngh x`, + ), + ).toEqual([ + { argv: ["A=1", "git", "push", "a b", 'c"d'], redirects: [] }, + { argv: ["echo", "x"], redirects: [] }, + { + argv: ["tee", "out.log"], + redirects: [ + { op: ">", target: "/dev/null" }, + { op: ">&", target: "1" }, + ], + }, + { argv: ["cat"], redirects: [{ op: ">", target: ".tmp/body.md" }], stdin: "line $x\n" }, + { argv: ["gh", "x"], redirects: [] }, + ]); + expect(parse("echo $(git push) # git rebase")).toEqual([{ argv: ["echo", "$(git push)"], redirects: [] }]); + expect(parse("cat <<-END\n\tbody\n\tEND")).toEqual([{ argv: ["cat"], redirects: [], stdin: "body\n" }]); +}); + +test.each([ + "git push", + "git push origin", + "git push origin main", + "git push -u origin feature", + "git -C repo push origin refs/heads/a:refs/heads/a feature", + "direnv exec . git push origin main", + "bash -c 'git push origin main'", +])("a push without an explicit refspec is blocked: %s", (command) => { + expect(bash(command)).toEqual([RULES.refspec]); +}); + +test.each([ + "git push origin refs/heads/a:refs/heads/a", + "git push origin HEAD:refs/heads/a", + "git push -o ci.skip origin refs/heads/a:refs/heads/a", + "git push origin --delete feature", + "git push --tags origin", + "git log --grep 'git push origin main'", + "echo git push", +])("a push with explicit refspecs, or no push, passes: %s", (command) => { + expect(bash(command)).toEqual([]); +}); + +test.each([ + "git push --force origin refs/heads/a:refs/heads/a", + "git push -f origin refs/heads/a:refs/heads/a", + "git push -uf origin refs/heads/a:refs/heads/a", + "git push --force-with-lease=a origin refs/heads/a:refs/heads/a", + "git push origin +refs/heads/a:refs/heads/a", +])("a force-push is blocked unless the user asked: %s", (command) => { + expect(bash(command)).toEqual([DENIED_REWRITE]); + expect(bash(`${USER_ASKED} ${command}`)).toEqual([]); +}); + +test("a rebase is blocked unless the user asked, or it ends one", () => { + expect(bash("git rebase origin/main")).toEqual([DENIED_REWRITE]); + expect(bash("git pull --rebase origin main")).toEqual([DENIED_REWRITE]); + expect(bash(`${USER_ASKED} git rebase origin/main`)).toEqual([]); + expect(bash("git rebase --abort")).toEqual([]); + expect(bash("git pull --no-rebase origin main")).toEqual([]); + expect(bash("gh pr merge 5 --rebase")).toEqual([]); +}); + +test("scratch writes under /tmp are blocked, reads and checkout paths pass", () => { + for (const command of [ + "echo x > /tmp/log", + "just test > /tmp/test.log 2>&1", + "cmd | tee /tmp/out", + "mkdir -p /tmp/work", + "cp a.txt /tmp/", + "git clone https://example.com/r /tmp/r", + "mktemp -d", + "mktemp -t body.XXXX", + 'cat > "$TMPDIR/body.md" < .tmp/log 2>/dev/null", + "mktemp -p .tmp", + "mktemp .tmp/body.XXXX", + "cp /tmp/a .tmp/a", + ]) + expect(bash(command), command).toEqual([]); + expect(broken({ tool: "Write", input: { file_path: "/tmp/x.md", content: "x" }, cwd })).toEqual([RULES.tmp]); + expect( + broken({ + tool: "apply_patch", + input: { command: "*** Begin Patch\n*** Add File: /tmp/x\n+x\n*** End Patch" }, + cwd, + }), + ).toEqual([RULES.tmp]); +}); + +test("an edit that adds an em dash is blocked; one keeping or quoting it passes", () => { + const edit = (old_string: string, new_string: string) => + broken({ tool: "Edit", input: { file_path: "a.md", old_string, new_string }, cwd }); + expect(edit("a, b", `a ${DASH} b`)).toEqual([RULES.emDash]); + expect(edit(`a ${DASH} b`, `a ${DASH} c`)).toEqual([]); + expect(edit("", `> they said a ${DASH} b`)).toEqual([]); + expect( + broken({ + tool: "MultiEdit", + input: { file_path: "a.md", edits: [{ old_string: "a", new_string: DASH }] }, + cwd, + }), + ).toEqual([RULES.emDash]); + + const file = join(cwd, "kept.md"); + writeFileSync(file, `old ${DASH} text\n`); + const write = (content: string) => broken({ tool: "Write", input: { file_path: file, content }, cwd }); + expect(write(`old ${DASH} text\nmore\n`)).toEqual([]); + expect(write(`old ${DASH} text ${DASH} more\n`)).toEqual([RULES.emDash]); + + const patch = (lines: string) => + broken({ + tool: "apply_patch", + input: { command: `*** Begin Patch\n*** Update File: a.md\n${lines}\n*** End Patch` }, + cwd, + }); + expect(patch(`-a, b\n+a ${DASH} b`)).toEqual([RULES.emDash]); + expect(patch(`-a ${DASH} b\n+a ${DASH} c`)).toEqual([]); +}); + +test("commit messages may not add em dashes", () => { + expect(bash(`git commit -m "fix: a ${DASH} b"`)).toEqual([RULES.emDash]); + expect(bash(`git commit -F - <<'EOF'\nfix: a ${DASH} b\nEOF`)).toEqual([RULES.emDash]); + expect(bash('git commit -m "fix: a, b"')).toEqual([]); +}); + +test("a new GitHub post needs the footer and no em dashes; edits and unreadable bodies pass", () => { + expect(bash(`gh pr comment 5 --body "Done.\n\n${FOOTER}"`)).toEqual([]); + expect(bash('gh pr comment 5 --body "Done."')).toEqual([RULES.footer]); + expect(bash(`gh pr comment 5 --body "Done ${DASH} mostly.\n\n${FOOTER}"`)).toEqual([RULES.emDash]); + expect(bash(`gh pr create --title "a ${DASH} b" --body-file - <<'EOF'\nBody\n\n${FOOTER}\nEOF`)).toEqual([ + RULES.emDash, + ]); + expect(bash(`cat > .tmp/body.md <<'EOF'\nBody\nEOF\ngh issue create --title t --body-file .tmp/body.md`)).toEqual([ + RULES.footer, + ]); + writeFileSync(join(cwd, "body.md"), `Body\n\n${FOOTER}\n`); + expect(bash("gh pr create --title t --body-file body.md")).toEqual([]); + expect(bash("gh pr create --title t --body-file - < body.md")).toEqual([]); + expect(bash("gh api repos/o/r/issues/5/comments -f body=Done")).toEqual([RULES.footer]); + expect(bash("gh api repos/o/r/issues/5/comments -F body=@body.md")).toEqual([]); + expect(bash("gh pr edit 5 --body Done")).toEqual([]); + expect(bash("gh api -X PATCH repos/o/r/pulls/5 -f body=Done")).toEqual([]); + expect(bash("gh pr review 5 --approve")).toEqual([]); + expect(bash("cat .tmp/body.md | gh pr comment 5 --body-file -")).toEqual([]); + expect(bash("gh pr comment 5 --body-file missing.md")).toEqual([]); +}); From fd30a6694e70ee62e5c1410d799ee0e1872b479f Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Sat, 7 Nov 2026 13:59:11 -0800 Subject: [PATCH 03/10] fix(hooks): pass bodies with expansions, keep the user's ask in nested shells, check only clone destinations Co-Authored-By: Claude Opus 5.5 --- src/cli/rules.ts | 61 ++++++++++++++++++++++++++++++++++++++++------ test/rules.test.ts | 9 +++++++ 2 files changed, 63 insertions(+), 7 deletions(-) diff --git a/src/cli/rules.ts b/src/cli/rules.ts index 099cf4bb..e01bce38 100644 --- a/src/cli/rules.ts +++ b/src/cli/rules.ts @@ -171,12 +171,15 @@ function dropOptions(args: string[]): string[] { return args.slice(i); } -/** Check each simple command in a shell command line, following `sh -c` into its script. */ -function shell(line: string, check: Check) { +/** + * Check each simple command in a shell command line, following `sh -c` into + * its script, which inherits the assignments in `inherited`. + */ +function shell(line: string, check: Check, inherited: ReadonlySet = new Set()) { const { found, scratch, cwd } = check; const written = new Map(); for (const simple of parse(line)) { - const env = new Set(); + const env = new Set(inherited); const argv = unwrap(simple.argv, env); for (const { op, target } of simple.redirects) { if ([">", ">>", ">|", "&>", "&>>"].includes(op)) { @@ -188,7 +191,7 @@ function shell(line: string, check: Check) { const [name, ...args] = argv; if (["bash", "sh", "zsh", "dash"].includes(name!)) { const flag = args.findIndex((arg) => /^-[a-z]*c[a-z]*$/.test(arg)); - if (flag !== -1 && args[flag + 1] !== undefined) shell(args[flag + 1]!, check); + if (flag !== -1 && args[flag + 1] !== undefined) shell(args[flag + 1]!, check, env); continue; } const asked = env.has(USER_ASKED); @@ -246,11 +249,53 @@ function git(argv: string[], asked: boolean, input: (file: string) => string | u const message = values(args, ["-m", "--message"], { attached: ["-m"] }).join("\n"); const files = values(args, ["-F", "--file"], { attached: ["-F"] }).map(input); if (adds("", [message, ...files].join("\n"))) check.found.add("emDash"); - } else if (sub === "clone" || (sub === "worktree" && args[0] === "add")) { - if (args.some((arg) => !arg.startsWith("-") && check.scratch(arg))) check.found.add("tmp"); + } else if (sub === "clone") { + // `git clone [options] []`: only the directory is written. + const directory = positionals(args, CLONE_VALUE_OPTIONS)[1]; + if (directory !== undefined && check.scratch(directory)) check.found.add("tmp"); + } else if (sub === "worktree" && args[0] === "add") { + // `git worktree add [options] []`. + const path = positionals(args.slice(1), new Set(["-b", "-B", "--reason"]))[0]; + if (path !== undefined && check.scratch(path)) check.found.add("tmp"); } } +/** `git clone` options that take the next word. */ +const CLONE_VALUE_OPTIONS = new Set([ + "-b", + "--branch", + "-o", + "--origin", + "-c", + "--config", + "-u", + "--upload-pack", + "-j", + "--jobs", + "--depth", + "--reference", + "--reference-if-able", + "--template", + "--separate-git-dir", + "--filter", + "--shallow-since", + "--shallow-exclude", + "--server-option", + "--bundle-uri", + "--ref-format", +]); + +/** The words in `args` that are not options, nor the values of the options in `valued`. */ +function positionals(args: string[], valued: ReadonlySet): string[] { + const found: string[] = []; + for (let i = 0; i < args.length; i++) { + if (args[i] === "--") return [...found, ...args.slice(i + 1)]; + if (valued.has(args[i]!)) i++; + else if (!args[i]!.startsWith("-")) found.push(args[i]!); + } + return found; +} + /** `git push` options that take the next word. */ const PUSH_VALUE_OPTIONS = new Set(["-o", "--push-option", "--repo", "--receive-pack", "--exec"]); @@ -327,7 +372,9 @@ function gh(args: string[], input: (file: string) => string | undefined, check: } if (body.length === 0) return; // A body that cannot be read here, such as one piped in, is left to the prose. - if (body.some((text) => text === undefined)) return; + // So is one holding an expansion, such as `"$body"` or an unquoted here-document's `$(cat f)`, + // since the shell rather than the hook knows its text. + if (body.some((text) => text === undefined || /\$[\w{(]|`/.test(text))) return; const text = body.join("\n"); if (authored && adds("", [...titles, text].join("\n"))) check.found.add("emDash"); if (authored && !/^\(written by .+\)\s*$/im.test(text)) check.found.add("footer"); diff --git a/test/rules.test.ts b/test/rules.test.ts index 922c7b6b..107cff38 100644 --- a/test/rules.test.ts +++ b/test/rules.test.ts @@ -79,6 +79,8 @@ test("a rebase is blocked unless the user asked, or it ends one", () => { expect(bash("git rebase origin/main")).toEqual([DENIED_REWRITE]); expect(bash("git pull --rebase origin main")).toEqual([DENIED_REWRITE]); expect(bash(`${USER_ASKED} git rebase origin/main`)).toEqual([]); + expect(bash(`${USER_ASKED} bash -c 'git rebase origin/main'`)).toEqual([]); + expect(bash("bash -c 'git rebase origin/main'")).toEqual([DENIED_REWRITE]); expect(bash("git rebase --abort")).toEqual([]); expect(bash("git pull --no-rebase origin main")).toEqual([]); expect(bash("gh pr merge 5 --rebase")).toEqual([]); @@ -92,6 +94,8 @@ test("scratch writes under /tmp are blocked, reads and checkout paths pass", () "mkdir -p /tmp/work", "cp a.txt /tmp/", "git clone https://example.com/r /tmp/r", + "git clone -b main https://example.com/r /tmp/r", + "git worktree add -b x /tmp/x", "mktemp -d", "mktemp -t body.XXXX", 'cat > "$TMPDIR/body.md" < Date: Sat, 7 Nov 2026 14:00:55 -0800 Subject: [PATCH 04/10] fix(hooks): judge expansions by where they appear, and check clone's separate git dir Co-Authored-By: Claude Opus 5.5 --- src/cli/rules.ts | 62 ++++++++++++++++++++++++++++++---------------- src/cli/shell.ts | 39 +++++++++++++++++++++++------ test/rules.test.ts | 11 +++++++- 3 files changed, 81 insertions(+), 31 deletions(-) diff --git a/src/cli/rules.ts b/src/cli/rules.ts index e01bce38..b162977a 100644 --- a/src/cli/rules.ts +++ b/src/cli/rules.ts @@ -184,7 +184,7 @@ function shell(line: string, check: Check, inherited: ReadonlySet = new for (const { op, target } of simple.redirects) { if ([">", ">>", ">|", "&>", "&>>"].includes(op)) { if (scratch(target)) found.add("tmp"); - if (simple.stdin !== undefined) written.set(target, simple.stdin); + if (simple.stdin !== undefined && !simple.stdinExpanded) written.set(target, simple.stdin); } } if (argv === null) continue; @@ -196,12 +196,17 @@ function shell(line: string, check: Check, inherited: ReadonlySet = new } const asked = env.has(USER_ASKED); const from = simple.redirects.find((redirect) => redirect.op === "<")?.target; - const stdin = simple.stdin ?? (from === undefined ? undefined : (written.get(from) ?? read(cwd, from))); + // Text the shell expands is unknown here, so it reads as unreadable. + const stdin = simple.stdinExpanded + ? undefined + : (simple.stdin ?? (from === undefined ? undefined : (written.get(from) ?? read(cwd, from)))); const input = (file: string) => (file === "-" ? stdin : (written.get(file) ?? read(cwd, file))); + const literal = (value: string) => (simple.expanded?.some((word) => word.endsWith(value)) ? undefined : value); if (name === "tee") { if (args.some((arg) => !arg.startsWith("-") && scratch(arg))) found.add("tmp"); for (const arg of args) - if (!arg.startsWith("-") && simple.stdin !== undefined) written.set(arg, simple.stdin); + if (!arg.startsWith("-") && simple.stdin !== undefined && !simple.stdinExpanded) + written.set(arg, simple.stdin); } else if (name === "mkdir" || name === "touch") { if (args.some(scratch)) found.add("tmp"); } else if (["cp", "mv", "ln", "rsync"].includes(name!)) { @@ -209,7 +214,7 @@ function shell(line: string, check: Check, inherited: ReadonlySet = new if (last !== undefined && scratch(last)) found.add("tmp"); } else if (name === "mktemp") mktemp(args, check); else if (name === "git") git(args, asked, input, check); - else if (name === "gh") gh(args, input, check); + else if (name === "gh") gh(args, input, literal, check); } } @@ -250,9 +255,9 @@ function git(argv: string[], asked: boolean, input: (file: string) => string | u const files = values(args, ["-F", "--file"], { attached: ["-F"] }).map(input); if (adds("", [message, ...files].join("\n"))) check.found.add("emDash"); } else if (sub === "clone") { - // `git clone [options] []`: only the directory is written. - const directory = positionals(args, CLONE_VALUE_OPTIONS)[1]; - if (directory !== undefined && check.scratch(directory)) check.found.add("tmp"); + // `git clone [options] []` writes only the directory and a separate git directory. + const written = [positionals(args, CLONE_VALUE_OPTIONS)[1], ...values(args, ["--separate-git-dir"])]; + if (written.some((path) => path !== undefined && check.scratch(path))) check.found.add("tmp"); } else if (sub === "worktree" && args[0] === "add") { // `git worktree add [options] []`. const path = positionals(args.slice(1), new Set(["-b", "-B", "--reason"]))[0]; @@ -346,20 +351,31 @@ const POSTS: Record> = { issue: { create: true, comment: true, edit: false }, }; -function gh(args: string[], input: (file: string) => string | undefined, check: Check) { +/** + * Check a `gh` command that posts text. `input` reads a body file (`-` for + * stdin), and `literal` gives a word's text, or `undefined` when the shell + * expands it; either way an unknown body passes. + */ +function gh( + args: string[], + input: (file: string) => string | undefined, + literal: (word: string) => string | undefined, + check: Check, +) { const [group, command, ...rest] = args; + const file = (path: string | undefined) => (path === undefined ? undefined : input(path)); let body: (string | undefined)[]; - let titles: string[] = []; + let titles: (string | undefined)[] = []; let authored: boolean; if (group === "api") { const fields = [command ?? "", ...rest]; - const raw = values(fields, ["-f", "--raw-field"]).filter((field) => field.startsWith("body=")); - const typed = values(fields, ["-F", "--field"]).filter((field) => field.startsWith("body=")); + const field = (names: string[]) => + values(fields, names) + .filter((value) => value.startsWith("body=")) + .map((value) => literal(value)?.slice("body=".length)); body = [ - ...raw.map((field) => field.slice("body=".length)), - ...typed - .map((field) => field.slice("body=".length)) - .map((value) => (value.startsWith("@") ? input(value.slice(1)) : value)), + ...field(["-f", "--raw-field"]), + ...field(["-F", "--field"]).map((value) => (value?.startsWith("@") ? file(value.slice(1)) : value)), ]; const method = values(fields, ["-X", "--method"]).at(-1)?.toUpperCase(); authored = method !== "PATCH" && method !== "PUT"; @@ -367,15 +383,17 @@ function gh(args: string[], input: (file: string) => string | undefined, check: const post = POSTS[group ?? ""]?.[command ?? ""]; if (post === undefined) return; authored = post; - body = [...values(rest, ["-b", "--body"]), ...values(rest, ["-F", "--body-file"]).map((file) => input(file))]; - titles = values(rest, ["-t", "--title"]); + body = [ + ...values(rest, ["-b", "--body"]).map(literal), + ...values(rest, ["-F", "--body-file"]).map((path) => file(literal(path))), + ]; + titles = values(rest, ["-t", "--title"]).map(literal); } if (body.length === 0) return; - // A body that cannot be read here, such as one piped in, is left to the prose. - // So is one holding an expansion, such as `"$body"` or an unquoted here-document's `$(cat f)`, - // since the shell rather than the hook knows its text. - if (body.some((text) => text === undefined || /\$[\w{(]|`/.test(text))) return; + // A body that cannot be read here, such as one piped in or expanded by the shell, is left to the prose. + if (body.some((text) => text === undefined)) return; const text = body.join("\n"); - if (authored && adds("", [...titles, text].join("\n"))) check.found.add("emDash"); + if (authored && adds("", [...titles.filter((title) => title !== undefined), text].join("\n"))) + check.found.add("emDash"); if (authored && !/^\(written by .+\)\s*$/im.test(text)) check.found.add("footer"); } diff --git a/src/cli/shell.ts b/src/cli/shell.ts index 4176096e..1fa0b75a 100644 --- a/src/cli/shell.ts +++ b/src/cli/shell.ts @@ -14,8 +14,15 @@ export interface Simple { argv: string[]; redirects: Redirect[]; stdin?: string; + /** The words in `argv` holding a parameter expansion or command substitution, whose text only the shell knows. */ + expanded?: string[]; + /** Whether `stdin` holds an expansion, from a here-string or a here-document with an unquoted delimiter. */ + stdinExpanded?: boolean; } +/** An expansion the shell performs in a word or an unquoted here-document. */ +const EXPANSION = /\$[\w{(@*#?$!-]|`/; + const SEPARATORS = new Set([";", "&", "|", "(", ")", "\n"]); /** @@ -24,21 +31,30 @@ const SEPARATORS = new Set([";", "&", "|", "(", ")", "\n"]); */ export function parse(line: string): Simple[] { const commands: Simple[] = []; - const heredocs: { command: Simple; delimiter: string; tabs: boolean }[] = []; + const heredocs: { command: Simple; delimiter: string; tabs: boolean; literal: boolean }[] = []; let command: Simple = { argv: [], redirects: [] }; let word: string | null = null; let redirect: string | null = null; + // Whether the current word holds an expansion, and whether any of it was quoted. + let expands = false; + let quoted = false; let i = 0; const endWord = () => { if (word === null) return; - if (redirect === null) command.argv.push(word); - else if (redirect === "<<" || redirect === "<<-") - heredocs.push({ command, delimiter: word, tabs: redirect === "<<-" }); - else if (redirect === "<<<") command.stdin = `${word}\n`; - else command.redirects.push({ op: redirect, target: word }); + if (redirect === null) { + command.argv.push(word); + if (expands) (command.expanded ??= []).push(word); + } else if (redirect === "<<" || redirect === "<<-") + heredocs.push({ command, delimiter: word, tabs: redirect === "<<-", literal: quoted }); + else if (redirect === "<<<") { + command.stdin = `${word}\n`; + if (expands) command.stdinExpanded = true; + } else command.redirects.push({ op: redirect, target: word }); word = null; redirect = null; + expands = false; + quoted = false; }; const endCommand = () => { endWord(); @@ -66,6 +82,8 @@ export function parse(line: string): Simple[] { while (i < line.length) { const c = line[i]!; + if (c === "'" || c === '"' || c === "\\") quoted = true; + if ((c === "$" && line[i + 1] !== "'" && EXPANSION.test(line.slice(i, i + 2))) || c === "`") expands = true; if (c === "\\") { if (line[i + 1] === "\n") i += 2; else { @@ -84,10 +102,14 @@ export function parse(line: string): Simple[] { if (line[i + 1] !== "\n") text += line[i + 1]; i += 2; } else if (line[i] === "$" && line[i + 1] === "(") { + expands = true; const end = closing(i + 2); text += line.slice(i, end); i = end; - } else text += line[i++]; + } else { + if (EXPANSION.test(line.slice(i, i + 2))) expands = true; + text += line[i++]; + } } append(text); i++; @@ -130,7 +152,7 @@ export function parse(line: string): Simple[] { /** Read the bodies of the here-documents opened on the line just ended, returning the index after them. */ function readHeredocs(from: number): number { let at = from; - for (const { command: owner, delimiter, tabs } of heredocs.splice(0)) { + for (const { command: owner, delimiter, tabs, literal } of heredocs.splice(0)) { const body: string[] = []; while (at < line.length) { const end = line.indexOf("\n", at); @@ -141,6 +163,7 @@ export function parse(line: string): Simple[] { body.push(text); } owner.stdin = body.length > 0 ? `${body.join("\n")}\n` : ""; + if (!literal && EXPANSION.test(owner.stdin)) owner.stdinExpanded = true; } return at; } diff --git a/test/rules.test.ts b/test/rules.test.ts index 107cff38..5be5cc81 100644 --- a/test/rules.test.ts +++ b/test/rules.test.ts @@ -36,7 +36,11 @@ test("parse splits commands and attaches redirections and here-documents", () => { argv: ["cat"], redirects: [{ op: ">", target: ".tmp/body.md" }], stdin: "line $x\n" }, { argv: ["gh", "x"], redirects: [] }, ]); - expect(parse("echo $(git push) # git rebase")).toEqual([{ argv: ["echo", "$(git push)"], redirects: [] }]); + expect(parse("echo $(git push) # git rebase")).toEqual([ + { argv: ["echo", "$(git push)"], redirects: [], expanded: ["$(git push)"] }, + ]); + expect(parse("cat < "$TMPDIR/body.md" < Date: Sat, 7 Nov 2026 20:50:49 -0800 Subject: [PATCH 05/10] fix(hooks): treat a body file the same command line writes as unknown Drops the in-command file tracking that read a here-document's text as the body file's contents. A file an earlier command on the line writes now reads as unknown and passes; `--body-file -` with a here-document stays checked. Co-Authored-By: Claude Opus 5.5 --- README.md | 8 +++++--- src/cli/rules.ts | 23 +++++++++++++---------- test/hooks.test.ts | 5 ++++- test/rules.test.ts | 10 +++++++--- 4 files changed, 29 insertions(+), 17 deletions(-) diff --git a/README.md b/README.md index 1cc1ae11..f9d79c9a 100644 --- a/README.md +++ b/README.md @@ -1844,9 +1844,11 @@ hook on shell commands and file edits denies, with the rule as the reason: line; - a scratch file written under `/tmp`, outside the checkout. -A body it cannot read, such as one piped into `gh`, passes. `quest context` -still prints these rules, under a last `Checked` section that the hook leaves -out, for agents without it. +A body it cannot read passes, such as one piped into `gh`, or a body file the +same command line writes first; pass the body on stdin with `--body-file -` and +a here-document to have it checked. `quest context` still prints these rules, +under a last `Checked` section that the hook leaves out, for agents without +it. `quest` must be on the `PATH` the agent starts with, as for the worktree hooks. Codex asks you to trust the hooks once, and after each upgrade that diff --git a/src/cli/rules.ts b/src/cli/rules.ts index b162977a..00497002 100644 --- a/src/cli/rules.ts +++ b/src/cli/rules.ts @@ -4,7 +4,7 @@ // call itself shows, and lets through what it cannot tell apart. import { existsSync, readFileSync } from "node:fs"; -import { dirname, isAbsolute, join } from "node:path"; +import { dirname, join, resolve } from "node:path"; import { parse } from "./shell"; @@ -121,7 +121,7 @@ function checkoutRoot(cwd: string): string { function read(cwd: string, path: string): string | undefined { try { - return readFileSync(isAbsolute(path) ? path : join(cwd, path), "utf8"); + return readFileSync(resolve(cwd, path), "utf8"); } catch { return undefined; } @@ -173,18 +173,21 @@ function dropOptions(args: string[]): string[] { /** * Check each simple command in a shell command line, following `sh -c` into - * its script, which inherits the assignments in `inherited`. + * its script, which inherits the assignments in `inherited`. A file an earlier + * command on the line writes holds what the shell will put there, not what is + * on disk, so it reads as unknown. */ function shell(line: string, check: Check, inherited: ReadonlySet = new Set()) { const { found, scratch, cwd } = check; - const written = new Map(); + const written = new Set(); + const write = (path: string) => written.add(resolve(cwd, path)); for (const simple of parse(line)) { const env = new Set(inherited); const argv = unwrap(simple.argv, env); for (const { op, target } of simple.redirects) { if ([">", ">>", ">|", "&>", "&>>"].includes(op)) { if (scratch(target)) found.add("tmp"); - if (simple.stdin !== undefined && !simple.stdinExpanded) written.set(target, simple.stdin); + write(target); } } if (argv === null) continue; @@ -197,21 +200,21 @@ function shell(line: string, check: Check, inherited: ReadonlySet = new const asked = env.has(USER_ASKED); const from = simple.redirects.find((redirect) => redirect.op === "<")?.target; // Text the shell expands is unknown here, so it reads as unreadable. + const disk = (file: string) => (written.has(resolve(cwd, file)) ? undefined : read(cwd, file)); const stdin = simple.stdinExpanded ? undefined - : (simple.stdin ?? (from === undefined ? undefined : (written.get(from) ?? read(cwd, from)))); - const input = (file: string) => (file === "-" ? stdin : (written.get(file) ?? read(cwd, file))); + : (simple.stdin ?? (from === undefined ? undefined : disk(from))); + const input = (file: string) => (file === "-" ? stdin : disk(file)); const literal = (value: string) => (simple.expanded?.some((word) => word.endsWith(value)) ? undefined : value); if (name === "tee") { if (args.some((arg) => !arg.startsWith("-") && scratch(arg))) found.add("tmp"); - for (const arg of args) - if (!arg.startsWith("-") && simple.stdin !== undefined && !simple.stdinExpanded) - written.set(arg, simple.stdin); + for (const arg of args) if (!arg.startsWith("-")) write(arg); } else if (name === "mkdir" || name === "touch") { if (args.some(scratch)) found.add("tmp"); } else if (["cp", "mv", "ln", "rsync"].includes(name!)) { const last = args.filter((arg) => !arg.startsWith("-")).at(-1); if (last !== undefined && scratch(last)) found.add("tmp"); + if (last !== undefined) write(last); } else if (name === "mktemp") mktemp(args, check); else if (name === "git") git(args, asked, input, check); else if (name === "gh") gh(args, input, literal, check); diff --git a/test/hooks.test.ts b/test/hooks.test.ts index 03919f76..df2f4303 100644 --- a/test/hooks.test.ts +++ b/test/hooks.test.ts @@ -152,7 +152,10 @@ test("setup installs the worktree hooks on request, keeps them on rerun, and rep const settings = read(path); expect(settings.hooks.WorktreeCreate).toEqual([{ hooks: [worktree] }]); expect(settings.hooks.SessionEnd).toEqual([{ hooks: [{ ...worktree, timeout: 60 }] }]); - expect(settings.hooks.SessionStart).toEqual([{ matcher: CLAUDE_HOOKS[0]!.matcher, hooks: [ours] }, { hooks: [worktree] }]); + expect(settings.hooks.SessionStart).toEqual([ + { matcher: CLAUDE_HOOKS[0]!.matcher, hooks: [ours] }, + { hooks: [worktree] }, + ]); expect(Object.keys(settings.hooks).sort()).toEqual( [...new Set([...CLAUDE_HOOKS, ...WORKTREE_HOOKS].map((h) => h.event))].sort(), ); diff --git a/test/rules.test.ts b/test/rules.test.ts index 5be5cc81..6b808c73 100644 --- a/test/rules.test.ts +++ b/test/rules.test.ts @@ -171,10 +171,14 @@ test("a new GitHub post needs the footer and no em dashes; edits and unreadable expect(bash(`gh pr create --title "a ${DASH} b" --body-file - <<'EOF'\nBody\n\n${FOOTER}\nEOF`)).toEqual([ RULES.emDash, ]); - expect(bash(`cat > .tmp/body.md <<'EOF'\nBody\nEOF\ngh issue create --title t --body-file .tmp/body.md`)).toEqual([ - RULES.footer, - ]); writeFileSync(join(cwd, "body.md"), `Body\n\n${FOOTER}\n`); + writeFileSync(join(cwd, "stale.md"), "Body\n"); + // A body file the same line writes holds what the shell puts there, not what is on disk, so it passes. + expect(bash(`cat > stale.md < ./stale.md <<'EOF'\nBody\nEOF\ngh issue create --title t --body-file stale.md`)).toEqual([]); + expect(bash("echo x | tee stale.md && gh pr comment 5 --body-file - < stale.md")).toEqual([]); + expect(bash("cp other.md stale.md; gh pr comment 5 --body-file stale.md")).toEqual([]); + expect(bash("gh pr comment 5 --body-file stale.md; cat > stale.md <<'EOF'\nx\nEOF")).toEqual([RULES.footer]); expect(bash("gh pr create --title t --body-file body.md")).toEqual([]); expect(bash("gh pr create --title t --body-file - < body.md")).toEqual([]); expect(bash("gh api repos/o/r/issues/5/comments -f body=Done")).toEqual([RULES.footer]); From e100158ec3376b62fa88a1ea707bb5ec6efe3bc5 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Sat, 7 Nov 2026 20:55:57 -0800 Subject: [PATCH 06/10] fix(hooks): check refspecs beside --tags, and honor TMPDIR= for mktemp Co-Authored-By: Claude Opus 5.5 --- src/cli/rules.ts | 25 ++++++++++++++++--------- test/rules.test.ts | 5 +++++ 2 files changed, 21 insertions(+), 9 deletions(-) diff --git a/src/cli/rules.ts b/src/cli/rules.ts index 00497002..489a8ba6 100644 --- a/src/cli/rules.ts +++ b/src/cli/rules.ts @@ -215,14 +215,17 @@ function shell(line: string, check: Check, inherited: ReadonlySet = new const last = args.filter((arg) => !arg.startsWith("-")).at(-1); if (last !== undefined && scratch(last)) found.add("tmp"); if (last !== undefined) write(last); - } else if (name === "mktemp") mktemp(args, check); + } else if (name === "mktemp") mktemp(args, env, check); else if (name === "git") git(args, asked, input, check); else if (name === "gh") gh(args, input, literal, check); } } -/** `mktemp` creates under `/tmp` unless given a directory or a template outside it. */ -function mktemp(args: string[], check: Check) { +/** + * `mktemp` creates under `$TMPDIR`, which is `/tmp` unless an assignment in + * `env` sets it, unless given a directory or a template outside it. + */ +function mktemp(args: string[], env: ReadonlySet, check: Check) { let dir: string | undefined; let tmpdir = false; let template: string | undefined; @@ -233,10 +236,12 @@ function mktemp(args: string[], check: Check) { else if (arg === "--tmpdir" || /^-[a-z]*t[a-z]*$/.test(arg)) tmpdir = true; else if (!arg.startsWith("-")) template = arg; } - if (dir !== undefined) { - if (check.scratch(dir)) check.found.add("tmp"); - } else if (template === undefined || tmpdir) check.found.add("tmp"); - else if (check.scratch(template)) check.found.add("tmp"); + const assigned = [...env] + .filter((assignment) => assignment.startsWith("TMPDIR=")) + .at(-1) + ?.slice("TMPDIR=".length); + if (dir === undefined && (template === undefined || tmpdir)) dir = assigned || "/tmp"; + if (check.scratch(dir ?? template!)) check.found.add("tmp"); } /** Options of `git` itself that take the next word. */ @@ -324,8 +329,10 @@ function push(args: string[], asked: boolean, check: Check) { flags.some((flag) => /^(--force|--force-with-lease(=.*)?|--mirror|-[a-zA-Z]*f[a-zA-Z]*)$/.test(flag)); if (forced && !asked) check.found.add("rewrite"); const deleting = flags.some((flag) => flag === "--delete" || /^-[a-zA-Z]*d[a-zA-Z]*$/.test(flag)); - const whole = flags.some((flag) => ["--tags", "--all", "--branches", "--mirror"].includes(flag)); - if (whole || deleting) return; + const whole = flags.some((flag) => ["--all", "--branches", "--mirror"].includes(flag)); + // `--tags` pushes every tag beside any refspecs given, which still need a `:`. + const tagsOnly = flags.includes("--tags") && refspecs.length === 0; + if (whole || deleting || tagsOnly) return; if (refspecs.length === 0 || refspecs.some((spec) => !spec.includes(":"))) check.found.add("refspec"); } diff --git a/test/rules.test.ts b/test/rules.test.ts index 6b808c73..ef87bad5 100644 --- a/test/rules.test.ts +++ b/test/rules.test.ts @@ -52,6 +52,7 @@ test.each([ "git -C repo push origin refs/heads/a:refs/heads/a feature", "direnv exec . git push origin main", "bash -c 'git push origin main'", + "git push --tags origin feature", ])("a push without an explicit refspec is blocked: %s", (command) => { expect(bash(command)).toEqual([RULES.refspec]); }); @@ -62,6 +63,7 @@ test.each([ "git push -o ci.skip origin refs/heads/a:refs/heads/a", "git push origin --delete feature", "git push --tags origin", + "git push --tags origin HEAD:refs/heads/a", "git log --grep 'git push origin main'", "echo git push", ])("a push with explicit refspecs, or no push, passes: %s", (command) => { @@ -103,6 +105,7 @@ test("scratch writes under /tmp are blocked, reads and checkout paths pass", () "git clone --separate-git-dir /tmp/repo.git /tmp/source .tmp/clone", "mktemp -d", "mktemp -t body.XXXX", + "TMPDIR=/tmp/x mktemp -d", 'cat > "$TMPDIR/body.md" < .tmp/log 2>/dev/null", "mktemp -p .tmp", "mktemp .tmp/body.XXXX", + 'TMPDIR="$PWD/.tmp" mktemp -d', + "env TMPDIR=.tmp mktemp -t body.XXXX", "cp /tmp/a .tmp/a", "git clone /tmp/source .tmp/clone", "git clone --depth 1 /tmp/source", From a0d8c95f1275c0783ace67c5a48bc2464f9ab714 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Sat, 7 Nov 2026 21:11:34 -0800 Subject: [PATCH 07/10] fix(hooks): read relative body files as unknown after a cd on the line Co-Authored-By: Claude Opus 5.5 --- src/cli/rules.ts | 10 +++++++--- test/rules.test.ts | 8 +++++++- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/src/cli/rules.ts b/src/cli/rules.ts index 489a8ba6..77a463dd 100644 --- a/src/cli/rules.ts +++ b/src/cli/rules.ts @@ -4,7 +4,7 @@ // call itself shows, and lets through what it cannot tell apart. import { existsSync, readFileSync } from "node:fs"; -import { dirname, join, resolve } from "node:path"; +import { dirname, isAbsolute, join, resolve } from "node:path"; import { parse } from "./shell"; @@ -175,12 +175,14 @@ function dropOptions(args: string[]): string[] { * Check each simple command in a shell command line, following `sh -c` into * its script, which inherits the assignments in `inherited`. A file an earlier * command on the line writes holds what the shell will put there, not what is - * on disk, so it reads as unknown. + * on disk, so it reads as unknown, as does any relative path once a command + * on the line may have changed the directory. */ function shell(line: string, check: Check, inherited: ReadonlySet = new Set()) { const { found, scratch, cwd } = check; const written = new Set(); const write = (path: string) => written.add(resolve(cwd, path)); + let moved = false; for (const simple of parse(line)) { const env = new Set(inherited); const argv = unwrap(simple.argv, env); @@ -192,6 +194,7 @@ function shell(line: string, check: Check, inherited: ReadonlySet = new } if (argv === null) continue; const [name, ...args] = argv; + if (["cd", "pushd", "popd"].includes(name!)) moved = true; if (["bash", "sh", "zsh", "dash"].includes(name!)) { const flag = args.findIndex((arg) => /^-[a-z]*c[a-z]*$/.test(arg)); if (flag !== -1 && args[flag + 1] !== undefined) shell(args[flag + 1]!, check, env); @@ -200,7 +203,8 @@ function shell(line: string, check: Check, inherited: ReadonlySet = new const asked = env.has(USER_ASKED); const from = simple.redirects.find((redirect) => redirect.op === "<")?.target; // Text the shell expands is unknown here, so it reads as unreadable. - const disk = (file: string) => (written.has(resolve(cwd, file)) ? undefined : read(cwd, file)); + const disk = (file: string) => + (moved && !isAbsolute(file)) || written.has(resolve(cwd, file)) ? undefined : read(cwd, file); const stdin = simple.stdinExpanded ? undefined : (simple.stdin ?? (from === undefined ? undefined : disk(from))); diff --git a/test/rules.test.ts b/test/rules.test.ts index ef87bad5..01666c06 100644 --- a/test/rules.test.ts +++ b/test/rules.test.ts @@ -1,6 +1,6 @@ // The rules `quest hook` checks on a tool call, and the shell parsing they rest on. -import { writeFileSync } from "node:fs"; +import { mkdirSync, writeFileSync } from "node:fs"; import { join } from "node:path"; import { beforeEach, expect, test } from "vitest"; @@ -184,6 +184,12 @@ test("a new GitHub post needs the footer and no em dashes; edits and unreadable expect(bash("echo x | tee stale.md && gh pr comment 5 --body-file - < stale.md")).toEqual([]); expect(bash("cp other.md stale.md; gh pr comment 5 --body-file stale.md")).toEqual([]); expect(bash("gh pr comment 5 --body-file stale.md; cat > stale.md <<'EOF'\nx\nEOF")).toEqual([RULES.footer]); + // After a `cd`, a relative body file may be another one of the same name, so it passes. + mkdirSync(join(cwd, "sub"), { recursive: true }); + writeFileSync(join(cwd, "sub", "stale.md"), `Body\n\n${FOOTER}\n`); + expect(bash("cd sub && gh pr comment 5 --body-file stale.md")).toEqual([]); + expect(bash("cd sub && gh pr comment 5 --body-file - < stale.md")).toEqual([]); + expect(bash(`cd sub && gh pr comment 5 --body-file ${join(cwd, "stale.md")}`)).toEqual([RULES.footer]); expect(bash("gh pr create --title t --body-file body.md")).toEqual([]); expect(bash("gh pr create --title t --body-file - < body.md")).toEqual([]); expect(bash("gh api repos/o/r/issues/5/comments -f body=Done")).toEqual([RULES.footer]); From 4b6d00c274dd42dd0c8435c06517bfe36595e4b7 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Sat, 7 Nov 2026 22:06:23 -0800 Subject: [PATCH 08/10] fix(hooks): check inline bodies only, never body files on disk The maintainer's pick at the second round-3 design checkpoint: a body file may hold something else by the time the line posts it, so the hook checks only --body, body= fields, git commit -m, and --body-file - with a here-document. The per-line cd and write tracking goes with it, and the footer and em-dash rules stay in prose for bodies posted from a file. Co-Authored-By: Claude Opus 5.5 --- README.md | 8 +++++--- assets/CONTEXT.md | 2 +- src/cli/rules.ts | 48 +++++++++++++++------------------------------- test/rules.test.ts | 30 ++++++++--------------------- 4 files changed, 29 insertions(+), 59 deletions(-) diff --git a/README.md b/README.md index f9d79c9a..440a8bd1 100644 --- a/README.md +++ b/README.md @@ -1844,9 +1844,11 @@ hook on shell commands and file edits denies, with the rule as the reason: line; - a scratch file written under `/tmp`, outside the checkout. -A body it cannot read passes, such as one piped into `gh`, or a body file the -same command line writes first; pass the body on stdin with `--body-file -` and -a here-document to have it checked. `quest context` still prints these rules, +It checks only a body on the command line itself: `--body`, a `body=` field, +`git commit -m`, or `--body-file -` with a here-document. A body file, or one +piped into `gh`, passes unread, since the file may hold something else by the +time the command runs; `quest context` keeps the footer and em-dash rules in +prose for that case. `quest context` still prints these rules, under a last `Checked` section that the hook leaves out, for agents without it. diff --git a/assets/CONTEXT.md b/assets/CONTEXT.md index 0a5b90f5..7957cd8e 100644 --- a/assets/CONTEXT.md +++ b/assets/CONTEXT.md @@ -55,7 +55,7 @@ Read-only discovery needs no new checkout when your context or own checkout answ Put PR bodies, review drafts, logs, patches, helper scripts, and temporary clones in its `.tmp/`, so they disappear with the worktree. If the repository doesn't ignore `.tmp/`, add it to the file `git rev-parse --git-path info/exclude` names. Never write them elsewhere in or beside the checkout, or in a directory other agents share. -Pass PR bodies to `gh` on stdin (`--body-file -`). +Pass PR bodies to `gh` on stdin (`--body-file -`) with a here-document, where the hook checks them; one posted from a file goes unchecked, so it must still end with the `(Written by ...)` line and hold no em dashes. Before ending a task, pausing for a decision, or handing off, commit a checkpoint and report what remains and which checks passed or did not run. Finish with a clean checkout: intended changes committed and pushed, incidental generated changes restored, and pre-existing or unrelated changes preserved. diff --git a/src/cli/rules.ts b/src/cli/rules.ts index 77a463dd..ca3c6677 100644 --- a/src/cli/rules.ts +++ b/src/cli/rules.ts @@ -4,7 +4,7 @@ // call itself shows, and lets through what it cannot tell apart. import { existsSync, readFileSync } from "node:fs"; -import { dirname, isAbsolute, join, resolve } from "node:path"; +import { dirname, join, resolve } from "node:path"; import { parse } from "./shell"; @@ -94,10 +94,9 @@ function adds(before: string, after: string): boolean { type Rule = keyof typeof RULES; -/** One call's check: the rules found broken so far, and where its relative paths resolve. */ +/** One call's check: the rules found broken so far. */ interface Check { found: Set; - cwd: string; /** Whether `path` is scratch under `/tmp`, written as is or through `$TMPDIR`, outside the checkout holding `cwd`. */ scratch: (path: string) => boolean; } @@ -106,7 +105,6 @@ function check(cwd: string): Check { const root = checkoutRoot(cwd); return { found: new Set(), - cwd, scratch: (path) => /^(\/tmp|\$TMPDIR|\$\{TMPDIR[^}]*\})(\/|$)/.test(path) && !`${path}/`.startsWith(`${root}/`), }; } @@ -173,52 +171,36 @@ function dropOptions(args: string[]): string[] { /** * Check each simple command in a shell command line, following `sh -c` into - * its script, which inherits the assignments in `inherited`. A file an earlier - * command on the line writes holds what the shell will put there, not what is - * on disk, so it reads as unknown, as does any relative path once a command - * on the line may have changed the directory. + * its script, which inherits the assignments in `inherited`. Bodies are read + * only from the line itself, never from disk: a file may hold something else + * by the time the line runs it. */ function shell(line: string, check: Check, inherited: ReadonlySet = new Set()) { - const { found, scratch, cwd } = check; - const written = new Set(); - const write = (path: string) => written.add(resolve(cwd, path)); - let moved = false; + const { found, scratch } = check; for (const simple of parse(line)) { const env = new Set(inherited); const argv = unwrap(simple.argv, env); - for (const { op, target } of simple.redirects) { - if ([">", ">>", ">|", "&>", "&>>"].includes(op)) { - if (scratch(target)) found.add("tmp"); - write(target); - } - } + for (const { op, target } of simple.redirects) + if ([">", ">>", ">|", "&>", "&>>"].includes(op) && scratch(target)) found.add("tmp"); if (argv === null) continue; const [name, ...args] = argv; - if (["cd", "pushd", "popd"].includes(name!)) moved = true; if (["bash", "sh", "zsh", "dash"].includes(name!)) { const flag = args.findIndex((arg) => /^-[a-z]*c[a-z]*$/.test(arg)); if (flag !== -1 && args[flag + 1] !== undefined) shell(args[flag + 1]!, check, env); continue; } const asked = env.has(USER_ASKED); - const from = simple.redirects.find((redirect) => redirect.op === "<")?.target; - // Text the shell expands is unknown here, so it reads as unreadable. - const disk = (file: string) => - (moved && !isAbsolute(file)) || written.has(resolve(cwd, file)) ? undefined : read(cwd, file); - const stdin = simple.stdinExpanded - ? undefined - : (simple.stdin ?? (from === undefined ? undefined : disk(from))); - const input = (file: string) => (file === "-" ? stdin : disk(file)); + // A here-document or here-string the shell expands is unknown here, as is any other stdin or file. + const stdin = simple.stdinExpanded ? undefined : simple.stdin; + const input = (file: string) => (file === "-" ? stdin : undefined); const literal = (value: string) => (simple.expanded?.some((word) => word.endsWith(value)) ? undefined : value); if (name === "tee") { if (args.some((arg) => !arg.startsWith("-") && scratch(arg))) found.add("tmp"); - for (const arg of args) if (!arg.startsWith("-")) write(arg); } else if (name === "mkdir" || name === "touch") { if (args.some(scratch)) found.add("tmp"); } else if (["cp", "mv", "ln", "rsync"].includes(name!)) { const last = args.filter((arg) => !arg.startsWith("-")).at(-1); if (last !== undefined && scratch(last)) found.add("tmp"); - if (last !== undefined) write(last); } else if (name === "mktemp") mktemp(args, env, check); else if (name === "git") git(args, asked, input, check); else if (name === "gh") gh(args, input, literal, check); @@ -366,9 +348,9 @@ const POSTS: Record> = { }; /** - * Check a `gh` command that posts text. `input` reads a body file (`-` for - * stdin), and `literal` gives a word's text, or `undefined` when the shell - * expands it; either way an unknown body passes. + * Check a `gh` command that posts text. `input` gives a body file's text + * (only `-`, stdin from the line, is known), and `literal` gives a word's + * text, or `undefined` when the shell expands it; an unknown body passes. */ function gh( args: string[], @@ -404,7 +386,7 @@ function gh( titles = values(rest, ["-t", "--title"]).map(literal); } if (body.length === 0) return; - // A body that cannot be read here, such as one piped in or expanded by the shell, is left to the prose. + // A body not on the line, such as a file, one piped in, or one the shell expands, is left to the prose. if (body.some((text) => text === undefined)) return; const text = body.join("\n"); if (authored && adds("", [...titles.filter((title) => title !== undefined), text].join("\n"))) diff --git a/test/rules.test.ts b/test/rules.test.ts index 01666c06..d4a21853 100644 --- a/test/rules.test.ts +++ b/test/rules.test.ts @@ -1,6 +1,6 @@ // The rules `quest hook` checks on a tool call, and the shell parsing they rest on. -import { mkdirSync, writeFileSync } from "node:fs"; +import { writeFileSync } from "node:fs"; import { join } from "node:path"; import { beforeEach, expect, test } from "vitest"; @@ -169,40 +169,26 @@ test("commit messages may not add em dashes", () => { expect(bash('git commit -m "fix: a, b"')).toEqual([]); }); -test("a new GitHub post needs the footer and no em dashes; edits and unreadable bodies pass", () => { +test("a new GitHub post needs the footer and no em dashes; edits and bodies off the line pass", () => { expect(bash(`gh pr comment 5 --body "Done.\n\n${FOOTER}"`)).toEqual([]); expect(bash('gh pr comment 5 --body "Done."')).toEqual([RULES.footer]); expect(bash(`gh pr comment 5 --body "Done ${DASH} mostly.\n\n${FOOTER}"`)).toEqual([RULES.emDash]); expect(bash(`gh pr create --title "a ${DASH} b" --body-file - <<'EOF'\nBody\n\n${FOOTER}\nEOF`)).toEqual([ RULES.emDash, ]); - writeFileSync(join(cwd, "body.md"), `Body\n\n${FOOTER}\n`); - writeFileSync(join(cwd, "stale.md"), "Body\n"); - // A body file the same line writes holds what the shell puts there, not what is on disk, so it passes. - expect(bash(`cat > stale.md < ./stale.md <<'EOF'\nBody\nEOF\ngh issue create --title t --body-file stale.md`)).toEqual([]); - expect(bash("echo x | tee stale.md && gh pr comment 5 --body-file - < stale.md")).toEqual([]); - expect(bash("cp other.md stale.md; gh pr comment 5 --body-file stale.md")).toEqual([]); - expect(bash("gh pr comment 5 --body-file stale.md; cat > stale.md <<'EOF'\nx\nEOF")).toEqual([RULES.footer]); - // After a `cd`, a relative body file may be another one of the same name, so it passes. - mkdirSync(join(cwd, "sub"), { recursive: true }); - writeFileSync(join(cwd, "sub", "stale.md"), `Body\n\n${FOOTER}\n`); - expect(bash("cd sub && gh pr comment 5 --body-file stale.md")).toEqual([]); - expect(bash("cd sub && gh pr comment 5 --body-file - < stale.md")).toEqual([]); - expect(bash(`cd sub && gh pr comment 5 --body-file ${join(cwd, "stale.md")}`)).toEqual([RULES.footer]); - expect(bash("gh pr create --title t --body-file body.md")).toEqual([]); - expect(bash("gh pr create --title t --body-file - < body.md")).toEqual([]); + // A body file is never read: it may hold something else by the time the line posts it. + writeFileSync(join(cwd, "stale.md"), `Body ${DASH}\n`); + expect(bash("gh pr comment 5 --body-file stale.md")).toEqual([]); + expect(bash("gh pr comment 5 --body-file - < stale.md")).toEqual([]); + expect(bash("git commit -F stale.md")).toEqual([]); + expect(bash("gh api repos/o/r/issues/5/comments -F body=@stale.md")).toEqual([]); expect(bash("gh api repos/o/r/issues/5/comments -f body=Done")).toEqual([RULES.footer]); - expect(bash("gh api repos/o/r/issues/5/comments -F body=@body.md")).toEqual([]); expect(bash("gh pr edit 5 --body Done")).toEqual([]); expect(bash("gh api -X PATCH repos/o/r/pulls/5 -f body=Done")).toEqual([]); expect(bash("gh pr review 5 --approve")).toEqual([]); expect(bash("cat .tmp/body.md | gh pr comment 5 --body-file -")).toEqual([]); - expect(bash("gh pr comment 5 --body-file missing.md")).toEqual([]); expect(bash('gh pr comment 5 --body "$body"')).toEqual([]); expect(bash("gh pr comment 5 --body-file - < Date: Sat, 7 Nov 2026 22:44:51 -0800 Subject: [PATCH 09/10] fix(hooks): pass help modes, read-only operands, $TMPDIR, and --repo refspecs Co-Authored-By: Claude Opus 5.5 --- src/cli/rules.ts | 22 +++++++++++++++++----- test/rules.test.ts | 14 +++++++++++++- 2 files changed, 30 insertions(+), 6 deletions(-) diff --git a/src/cli/rules.ts b/src/cli/rules.ts index ca3c6677..96cd86a6 100644 --- a/src/cli/rules.ts +++ b/src/cli/rules.ts @@ -97,7 +97,10 @@ type Rule = keyof typeof RULES; /** One call's check: the rules found broken so far. */ interface Check { found: Set; - /** Whether `path` is scratch under `/tmp`, written as is or through `$TMPDIR`, outside the checkout holding `cwd`. */ + /** + * Whether `path` is scratch under `/tmp` outside the checkout holding `cwd`. + * A `$TMPDIR` path is unknown, since `TMPDIR` may name the checkout itself. + */ scratch: (path: string) => boolean; } @@ -105,7 +108,7 @@ function check(cwd: string): Check { const root = checkoutRoot(cwd); return { found: new Set(), - scratch: (path) => /^(\/tmp|\$TMPDIR|\$\{TMPDIR[^}]*\})(\/|$)/.test(path) && !`${path}/`.startsWith(`${root}/`), + scratch: (path) => /^\/tmp(\/|$)/.test(path) && !`${path}/`.startsWith(`${root}/`), }; } @@ -197,9 +200,12 @@ function shell(line: string, check: Check, inherited: ReadonlySet = new if (name === "tee") { if (args.some((arg) => !arg.startsWith("-") && scratch(arg))) found.add("tmp"); } else if (name === "mkdir" || name === "touch") { - if (args.some(scratch)) found.add("tmp"); + // `touch -r ` reads its reference file, and `-d`/`-t` take a date, as `mkdir -m` takes a mode. + const valued = name === "mkdir" ? ["-m"] : ["-r", "-d", "-t"]; + if (positionals(args, new Set(valued)).some(scratch)) found.add("tmp"); } else if (["cp", "mv", "ln", "rsync"].includes(name!)) { - const last = args.filter((arg) => !arg.startsWith("-")).at(-1); + const target = values(args, ["-t", "--target-directory"]).at(-1); + const last = target ?? args.filter((arg) => !arg.startsWith("-")).at(-1); if (last !== undefined && scratch(last)) found.add("tmp"); } else if (name === "mktemp") mktemp(args, env, check); else if (name === "git") git(args, asked, input, check); @@ -217,6 +223,8 @@ function mktemp(args: string[], env: ReadonlySet, check: Check) { let template: string | undefined; for (let i = 0; i < args.length; i++) { const arg = args[i]!; + // These create nothing. + if (["--help", "--version", "--dry-run"].includes(arg) || /^-[a-z]*u[a-z]*$/.test(arg)) return; if (arg === "-p") dir = args[++i]; else if (arg.startsWith("--tmpdir=")) dir = arg.slice("--tmpdir=".length); else if (arg === "--tmpdir" || /^-[a-z]*t[a-z]*$/.test(arg)) tmpdir = true; @@ -237,6 +245,7 @@ function git(argv: string[], asked: boolean, input: (file: string) => string | u let i = 0; while (i < argv.length && argv[i]!.startsWith("-")) i += GIT_VALUE_OPTIONS.has(argv[i]!) ? 2 : 1; const [sub, ...args] = argv.slice(i); + if (args.includes("--help") || args.includes("-h")) return; if (sub === "push") push(args, asked, check); else if (sub === "rebase") { if (!asked && !args.some((arg) => ["--abort", "--quit", "--show-current-patch"].includes(arg))) @@ -301,15 +310,18 @@ const PUSH_VALUE_OPTIONS = new Set(["-o", "--push-option", "--repo", "--receive- function push(args: string[], asked: boolean, check: Check) { const positionals: string[] = []; const flags: string[] = []; + let repo = false; for (let i = 0; i < args.length; i++) { const arg = args[i]!; if (arg === "--") positionals.push(...args.slice(i + 1)); if (arg === "--") break; + if (arg === "--repo" || arg.startsWith("--repo=")) repo = true; if (PUSH_VALUE_OPTIONS.has(arg)) i++; else if (arg.startsWith("-")) flags.push(arg); else positionals.push(arg); } - const refspecs = positionals.slice(1); + // `--repo` names the remote, so every positional is a refspec. + const refspecs = repo ? positionals : positionals.slice(1); const forced = refspecs.some((spec) => spec.startsWith("+")) || flags.some((flag) => /^(--force|--force-with-lease(=.*)?|--mirror|-[a-zA-Z]*f[a-zA-Z]*)$/.test(flag)); diff --git a/test/rules.test.ts b/test/rules.test.ts index d4a21853..cacd4f1b 100644 --- a/test/rules.test.ts +++ b/test/rules.test.ts @@ -53,6 +53,7 @@ test.each([ "direnv exec . git push origin main", "bash -c 'git push origin main'", "git push --tags origin feature", + "git push --repo origin main refs/heads/a:refs/heads/a", ])("a push without an explicit refspec is blocked: %s", (command) => { expect(bash(command)).toEqual([RULES.refspec]); }); @@ -64,6 +65,8 @@ test.each([ "git push origin --delete feature", "git push --tags origin", "git push --tags origin HEAD:refs/heads/a", + "git push --repo origin refs/heads/a:refs/heads/a", + "git push --help", "git log --grep 'git push origin main'", "echo git push", ])("a push with explicit refspecs, or no push, passes: %s", (command) => { @@ -88,6 +91,7 @@ test("a rebase is blocked unless the user asked, or it ends one", () => { expect(bash(`${USER_ASKED} bash -c 'git rebase origin/main'`)).toEqual([]); expect(bash("bash -c 'git rebase origin/main'")).toEqual([DENIED_REWRITE]); expect(bash("git rebase --abort")).toEqual([]); + expect(bash("git rebase --help")).toEqual([]); expect(bash("git pull --no-rebase origin main")).toEqual([]); expect(bash("gh pr merge 5 --rebase")).toEqual([]); }); @@ -106,7 +110,8 @@ test("scratch writes under /tmp are blocked, reads and checkout paths pass", () "mktemp -d", "mktemp -t body.XXXX", "TMPDIR=/tmp/x mktemp -d", - 'cat > "$TMPDIR/body.md" < "$TMPDIR/body.md" < Date: Sat, 7 Nov 2026 22:49:52 -0800 Subject: [PATCH 10/10] fix(hooks): anchor the footer to the body's end, and narrow help and target options Co-Authored-By: Claude Opus 5.5 --- src/cli/rules.ts | 9 +++++---- test/rules.test.ts | 5 +++++ 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/src/cli/rules.ts b/src/cli/rules.ts index 96cd86a6..82768575 100644 --- a/src/cli/rules.ts +++ b/src/cli/rules.ts @@ -201,10 +201,11 @@ function shell(line: string, check: Check, inherited: ReadonlySet = new if (args.some((arg) => !arg.startsWith("-") && scratch(arg))) found.add("tmp"); } else if (name === "mkdir" || name === "touch") { // `touch -r ` reads its reference file, and `-d`/`-t` take a date, as `mkdir -m` takes a mode. - const valued = name === "mkdir" ? ["-m"] : ["-r", "-d", "-t"]; + const valued = name === "mkdir" ? ["-m", "--mode"] : ["-r", "--reference", "-d", "--date", "-t"]; if (positionals(args, new Set(valued)).some(scratch)) found.add("tmp"); } else if (["cp", "mv", "ln", "rsync"].includes(name!)) { - const target = values(args, ["-t", "--target-directory"]).at(-1); + // rsync's `-t` keeps times instead. + const target = name === "rsync" ? undefined : values(args, ["-t", "--target-directory"]).at(-1); const last = target ?? args.filter((arg) => !arg.startsWith("-")).at(-1); if (last !== undefined && scratch(last)) found.add("tmp"); } else if (name === "mktemp") mktemp(args, env, check); @@ -245,7 +246,7 @@ function git(argv: string[], asked: boolean, input: (file: string) => string | u let i = 0; while (i < argv.length && argv[i]!.startsWith("-")) i += GIT_VALUE_OPTIONS.has(argv[i]!) ? 2 : 1; const [sub, ...args] = argv.slice(i); - if (args.includes("--help") || args.includes("-h")) return; + if (args[0] === "--help" || args[0] === "-h") return; if (sub === "push") push(args, asked, check); else if (sub === "rebase") { if (!asked && !args.some((arg) => ["--abort", "--quit", "--show-current-patch"].includes(arg))) @@ -403,5 +404,5 @@ function gh( const text = body.join("\n"); if (authored && adds("", [...titles.filter((title) => title !== undefined), text].join("\n"))) check.found.add("emDash"); - if (authored && !/^\(written by .+\)\s*$/im.test(text)) check.found.add("footer"); + if (authored && !/(^|\n)\(written by [^\n]+\)\s*$/i.test(text)) check.found.add("footer"); } diff --git a/test/rules.test.ts b/test/rules.test.ts index cacd4f1b..884aa399 100644 --- a/test/rules.test.ts +++ b/test/rules.test.ts @@ -54,6 +54,7 @@ test.each([ "bash -c 'git push origin main'", "git push --tags origin feature", "git push --repo origin main refs/heads/a:refs/heads/a", + "git push -o --help origin main", ])("a push without an explicit refspec is blocked: %s", (command) => { expect(bash(command)).toEqual([RULES.refspec]); }); @@ -112,6 +113,7 @@ test("scratch writes under /tmp are blocked, reads and checkout paths pass", () "TMPDIR=/tmp/x mktemp -d", "touch -m /tmp/x", "cp -t /tmp/out a.txt", + "rsync -t .tmp/source /tmp/destination", ]) expect(bash(command), command).toEqual([RULES.tmp]); for (const command of [ @@ -130,6 +132,8 @@ test("scratch writes under /tmp are blocked, reads and checkout paths pass", () 'cat > "$TMPDIR/body.md" < { test("a new GitHub post needs the footer and no em dashes; edits and bodies off the line pass", () => { expect(bash(`gh pr comment 5 --body "Done.\n\n${FOOTER}"`)).toEqual([]); expect(bash('gh pr comment 5 --body "Done."')).toEqual([RULES.footer]); + expect(bash(`gh pr comment 5 --body "${FOOTER}\nMore."`)).toEqual([RULES.footer]); expect(bash(`gh pr comment 5 --body "Done ${DASH} mostly.\n\n${FOOTER}"`)).toEqual([RULES.emDash]); expect(bash(`gh pr create --title "a ${DASH} b" --body-file - <<'EOF'\nBody\n\n${FOOTER}\nEOF`)).toEqual([ RULES.emDash,