diff --git a/README.md b/README.md index ee484e46..440a8bd1 100644 --- a/README.md +++ b/README.md @@ -1807,33 +1807,61 @@ 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. -`quest setup --worktrees` also installs the +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`. +`quest setup --worktrees` also installs Claude Code's [worktree hooks](#claude-code-checkouts). -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. +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. + +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. + +`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 a3a1bf45..469ea650 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 18bc7922..7957cd8e 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,8 +54,8 @@ 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. -Pass PR bodies to `gh` on stdin (`--body-file -`). +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 -`) 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. @@ -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 ea5fa14b..b292b30f 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 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 18bcec34..53f3d604 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 rule +// 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"; import { HOOK_EVENTS } from "./worktree-hook"; @@ -22,25 +23,67 @@ export const WORKTREE_COMMAND = "quest worktree 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, the matcher narrowing it, the command (`HOOK_COMMAND` by default), and its timeout in seconds. */ +/** + * One installed entry: the event, the matcher narrowing it, if any, the + * command (`HOOK_COMMAND` by default), and the handler's settings beyond its + * command, such as its timeout. + */ export interface HookEntry { event: string; matcher?: string; command?: string; - timeout?: number; + 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 is + * Claude Code's, which also holds the foreground timeout cap and takes the + * worktree hooks. + */ +export interface Target { + name: string; + path: string; + hooks: readonly HookEntry[]; + claude: 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, claude: 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, claude: false }]; +} + /** * The entries `quest setup --worktrees` adds, handing Claude Code's worktrees, * and the subagent and session lifecycles that hold them, to `quest worktree`. @@ -49,7 +92,7 @@ export const HOOKS: readonly HookEntry[] = [ export const WORKTREE_HOOKS: readonly HookEntry[] = HOOK_EVENTS.map((event) => ({ event, command: WORKTREE_COMMAND, - ...(event === "SessionEnd" ? { timeout: 60 } : {}), + ...(event === "SessionEnd" ? { settings: { timeout: 60 } } : {}), })); /** @@ -67,19 +110,18 @@ 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, by default the rule - * hooks plus the worktree hooks when `worktrees` is true, or when it is - * unset and the file already has them. 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 in Claude Code's the + * foreground timeout cap and the worktree hooks when `worktrees` is true, or + * when it is unset and the file already has them. 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; worktrees?: boolean; entries?: readonly HookEntry[] } = {}, -): string[] { +export function setup(target: Target, options: { remove?: boolean; worktrees?: boolean } = {}): string[] { + const { path } = target; + const { remove = false } = options; const original = existsSync(path) ? readFileSync(path, "utf8") : null; let settings: unknown = {}; if (original !== null) { @@ -92,15 +134,17 @@ export function setup( 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 worktrees = options.worktrees ?? hasWorktreeHooks(settings); - const entries = options.entries ?? (worktrees ? [...HOOKS, ...WORKTREE_HOOKS] : HOOKS); - const hooked = withHooks(settings, options.remove ? [] : entries, path); + const worktrees = target.claude && (options.worktrees ?? hasWorktreeHooks(settings)); + const entries = worktrees ? [...target.hooks, ...WORKTREE_HOOKS] : target.hooks; + const hooked = withHooks(settings, remove ? [] : entries, 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.claude) { + // 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 }; @@ -140,22 +184,24 @@ 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")) + if (only && (group.matcher === undefined || typeof group.matcher === "string")) { + const { type: _, command, ...settings } = group.hooks[0] as Json & { command: string }; installed.push({ event, ...(group.matcher === undefined ? {} : { matcher: group.matcher as string }), - ...handlerEntry(group.hooks[0] as Json & { command: string }), + command, + ...(Object.keys(settings).length === 0 ? {} : { settings }), }); - else scattered = true; + } 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, command = HOOK_COMMAND, timeout } of entries) { + for (const { event, matcher, command = HOOK_COMMAND, settings: extra } of entries) { const group = { ...(matcher === undefined ? {} : { matcher }), - hooks: [{ type: "command", command, ...(timeout === undefined ? {} : { timeout }) }], + hooks: [{ type: "command", command, ...extra }], }; kept[event] = [...((kept[event] as unknown[] | undefined) ?? []), group]; } @@ -175,13 +221,6 @@ function isOurs(handler: unknown): handler is Json & { command: string } { ); } -/** The command and timeout of Quest's `handler`, with any other field making it differ from every entry. */ -function handlerEntry(handler: Json & { command: string }): Pick { - const extra = Object.keys(handler).some((key) => !["type", "command", "timeout"].includes(key)); - const timeout = typeof handler.timeout === "number" ? handler.timeout : undefined; - return { command: extra ? `${handler.command}\0` : handler.command, ...(timeout === undefined ? {} : { timeout }) }; -} - /** Whether `settings` holds any of Quest's worktree hooks. */ function hasWorktreeHooks(settings: Json): boolean { if (!isObject(settings.hooks)) return false; @@ -199,7 +238,12 @@ function hasWorktreeHooks(settings: Json): boolean { function sameEntries(a: readonly HookEntry[], b: readonly HookEntry[]): boolean { const key = (entry: HookEntry) => - [entry.event, entry.matcher ?? "", entry.command ?? HOOK_COMMAND, entry.timeout ?? ""].join("\0"); + [ + entry.event, + entry.matcher ?? "", + entry.command ?? HOOK_COMMAND, + JSON.stringify(Object.entries(entry.settings ?? {}).sort()), + ].join("\0"); const left = a.map(key).sort(); const right = b.map(key).sort(); return left.length === right.length && left.every((value, i) => value === right[i]); @@ -209,17 +253,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; @@ -231,6 +279,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 e6921657..97e86a01 100644 --- a/src/cli/main.ts +++ b/src/cli/main.ts @@ -64,7 +64,17 @@ import { roundLines, } from "./review"; import { type Recorded, pullModels, recordRound } from "./review-record"; -import { BASH_MAX_TIMEOUT_MS, HOOKS, WORKTREE_HOOKS, hook, settingsPath, setup } from "./hooks"; +import { + BASH_MAX_TIMEOUT_MS, + CLAUDE_HOOKS, + CODEX_HOOKS, + type HookEntry, + WORKTREE_HOOKS, + 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"; @@ -1394,11 +1404,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") @@ -1421,24 +1432,30 @@ 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" + - "With --worktrees, also installs " + + `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" + + "With --worktrees, also installs Claude Code's " + WORKTREE_HOOKS.map((h) => h.event).join(", ") + " hooks running `quest worktree hook`, which hand Claude Code's worktrees to `quest worktree`; " + "--no-worktrees takes them out, and without either a rerun keeps them as they are. Only one hook " + "can manage Claude's worktrees, so remove any other WorktreeCreate or WorktreeRemove hook first.\n\n" + - "Validates the whole file, then writes it once and prints each change; settings that are not a JSON " + + "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.", ) @@ -1446,18 +1463,28 @@ export function main(argv: string[], out: Output): number | Promise { .option("--no-worktrees", "Remove the worktree hooks") .option("--remove", "Remove Quest's hooks, and env.BASH_MAX_TIMEOUT_MS while it still holds Quest's value") .action((options: { remove?: boolean; worktrees?: boolean }) => { - for (const line of setup(settingsPath(), options)) print(line); + for (const target of targets()) { + const changes = setup(target, options); + 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..82768575 --- /dev/null +++ b/src/cli/rules.ts @@ -0,0 +1,408 @@ +// 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, join, resolve } 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. */ +interface Check { + found: Set; + /** + * 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; +} + +function check(cwd: string): Check { + const root = checkoutRoot(cwd); + return { + found: new Set(), + scratch: (path) => /^\/tmp(\/|$)/.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(resolve(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, 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 } = 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) && scratch(target)) found.add("tmp"); + 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, env); + continue; + } + const asked = env.has(USER_ASKED); + // 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"); + } 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", "--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!)) { + // 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); + else if (name === "git") git(args, asked, input, check); + else if (name === "gh") gh(args, input, literal, 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; + 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; + else if (!arg.startsWith("-")) template = arg; + } + 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. */ +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 (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))) + 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") { + // `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]; + 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"]); + +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); + } + // `--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)); + 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) => ["--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"); +} + +/** + * 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 }, +}; + +/** + * 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[], + 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 | undefined)[] = []; + let authored: boolean; + if (group === "api") { + const fields = [command ?? "", ...rest]; + const field = (names: string[]) => + values(fields, names) + .filter((value) => value.startsWith("body=")) + .map((value) => literal(value)?.slice("body=".length)); + body = [ + ...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"; + } else { + const post = POSTS[group ?? ""]?.[command ?? ""]; + if (post === undefined) return; + authored = post; + 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 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"))) + check.found.add("emDash"); + if (authored && !/(^|\n)\(written by [^\n]+\)\s*$/i.test(text)) check.found.add("footer"); +} diff --git a/src/cli/shell.ts b/src/cli/shell.ts new file mode 100644 index 00000000..1fa0b75a --- /dev/null +++ b/src/cli/shell.ts @@ -0,0 +1,170 @@ +// 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; + /** 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"]); + +/** + * 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; 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); + 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(); + 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 === "'" || 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 { + 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] === "(") { + expands = true; + const end = closing(i + 2); + text += line.slice(i, end); + i = end; + } else { + if (EXPANSION.test(line.slice(i, i + 2))) expands = true; + 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, literal } 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` : ""; + if (!literal && EXPANSION.test(owner.stdin)) owner.stdinExpanded = true; + } + return at; + } +} diff --git a/src/runner/prompt.ts b/src/runner/prompt.ts index 84680573..3608efac 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, type Prompt, type Question, lands } from "./protocol"; @@ -122,7 +123,8 @@ function workflow(job: Job): string { 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 or delete checks the changed quest files against the guide itself. diff --git a/test/hooks.test.ts b/test/hooks.test.ts index d45bda33..df2f4303 100644 --- a/test/hooks.test.ts +++ b/test/hooks.test.ts @@ -8,19 +8,24 @@ import { expect, test } from "vitest"; import { REFERENCE_LINE } from "../src/core"; import { BASH_MAX_TIMEOUT_MS, - HOOKS, + CLAUDE_HOOKS, + CODEX_HOOKS, + CONTEXT_LIMIT, HOOK_COMMAND, + type Target, WORKTREE_COMMAND, WORKTREE_HOOKS, 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, claude: 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")); @@ -29,33 +34,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), { remove: 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), { remove: true })).toEqual([]); expect(read(path)).toEqual({ env: { BASH_MAX_TIMEOUT_MS: "600000" } }); }); @@ -63,12 +68,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), { remove: true })).toHaveLength(2); expect(read(path)).toEqual({ theme: "dark", hooks: { SessionStart: [theirs] }, env: { A: "1" } }); }); @@ -84,21 +89,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, claude: 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, { remove: 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), { remove: true }); expect(read(path)).toEqual({ theme: "dark" }); - expect(setup(path, { remove: true })).toEqual([]); + expect(setup(claude(path), { remove: true })).toEqual([]); }); test("setup installs the worktree hooks on request, keeps them on rerun, and replaces pasted ones", () => { @@ -111,23 +148,30 @@ test("setup installs the worktree hooks on request, keeps them on rerun, and rep hooks: { WorktreeCreate: [{ hooks: [pasted] }], SessionEnd: [{ hooks: [{ ...pasted, timeout: 60 }] }] }, }), ); - setup(path); + setup(claude(path)); 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: 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([...HOOKS, ...WORKTREE_HOOKS].map((h) => h.event))].sort(), + [...new Set([...CLAUDE_HOOKS, ...WORKTREE_HOOKS].map((h) => h.event))].sort(), ); - expect(setup(path)).toEqual([]); - expect(setup(path, { worktrees: true })).toEqual([]); - - expect(setup(path, { worktrees: false })).toEqual([`${path}: installed Quest's hooks`]); - expect(Object.keys(read(path).hooks)).toEqual(HOOKS.map((h) => h.event)); - expect(setup(path)).toEqual([]); - expect(setup(path, { worktrees: true })).toEqual([`${path}: installed Quest's hooks`]); - setup(path, { remove: true }); + expect(setup(claude(path))).toEqual([]); + expect(setup(claude(path), { worktrees: true })).toEqual([]); + + expect(setup(claude(path), { worktrees: false })).toEqual([`${path}: installed Quest's hooks`]); + expect(Object.keys(read(path).hooks)).toEqual(CLAUDE_HOOKS.map((h) => h.event)); + expect(setup(claude(path))).toEqual([]); + expect(setup(claude(path), { worktrees: true })).toEqual([`${path}: installed Quest's hooks`]); + setup(claude(path), { remove: true }); expect(read(path)).toEqual({}); + + const codex = join(tempDir(), "hooks.json"); + setup({ name: "Codex", path: codex, hooks: CODEX_HOOKS, claude: false }, { worktrees: true }); + expect(Object.keys(read(codex).hooks)).toEqual(CODEX_HOOKS.map((h) => h.event)); }); test.each([ @@ -142,8 +186,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), { remove: true })).toThrow(path); expect(readFileSync(path, "utf8")).toBe(content); }); @@ -169,6 +213,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..884aa399 --- /dev/null +++ b/test/rules.test.ts @@ -0,0 +1,211 @@ +// 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: [], expanded: ["$(git push)"] }, + ]); + expect(parse("cat < { + 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 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) => { + 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(`${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([]); +}); + +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", + "git clone -b main https://example.com/r /tmp/r", + "git worktree add -b x /tmp/x", + "git clone --separate-git-dir /tmp/repo.git /tmp/source .tmp/clone", + "mktemp -d", + "mktemp -t body.XXXX", + "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 [ + "cat /tmp/claude-1000/tasks/x.output", + "ls /tmp", + "echo x > .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", + "git worktree add -b x .worktrees/x /tmp/ignored", + // `$TMPDIR` may name the checkout, as a kix runner job sets it. + 'cat > "$TMPDIR/body.md" < { + 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 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, + ]); + // 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 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 "$body"')).toEqual([]); + expect(bash("gh pr comment 5 --body-file - <