From 28ff612a14b249fb495a3aebb03124fa508e2e59 Mon Sep 17 00:00:00 2001 From: Dimitri Kennedy Date: Fri, 23 Oct 2026 23:09:56 -0400 Subject: [PATCH] fix(agents): refuse rebase--helper --add-exec-commands and cover detached task surfaces Close the task catalog follow-ups from #430 and #409. HACK-1411: older Git's `rebase--helper --add-exec-commands cmd` is now refused through a `rebase`-scoped inline flag, so `git config --add` still passes. Pin the mapping of a dashed builtin's trailing arguments (`git-config alias.x bisect--helper` stays refused), pin the lowercased porcelain (`git Rebase--Helper --add-exec-commands cmd`), document and test the new fail-closed collateral (`git push -u origin clone--helper`, `git add bisect--helper`, `git config --add alias.r rebase`), and label the helper case HACK-1373. HACK-1383: decision 7 lists both remedies the detached_checkout refusal offers (`--branch`/MCP `branch`, or `worktree.auto_branch=false`), and a regression covers CLI `task inspect` and both MCP task tools in a detached linked worktree, each surface with its own assertion. Refs HACK-1411, HACK-1383 Co-Authored-By: Claude Opus 5.5 --- docs/reference/ax-task-catalog.md | 21 ++++-- src/lib/task-catalog-recipe.ts | 11 ++- tests/project-task-catalog.test.ts | 107 +++++++++++++++++++++++++++-- 3 files changed, 126 insertions(+), 13 deletions(-) diff --git a/docs/reference/ax-task-catalog.md b/docs/reference/ax-task-catalog.md index 889668e2c..43b1c4160 100644 --- a/docs/reference/ax-task-catalog.md +++ b/docs/reference/ax-task-catalog.md @@ -131,8 +131,11 @@ is not implemented here. runtime instance does not change how a recipe is judged, but the embedded target still names one. A detached linked worktree (for example one made with `git worktree add --detach`) has no implicit instance, so it is - refused as `detached_checkout` unless the caller names one with - `--branch` (MCP `branch`). + refused as `detached_checkout`. The refusal offers two remedies: name an + instance with `--branch` (MCP `branch`), or set `worktree.auto_branch` to + `false` in the project config so the checkout targets the base instance. + The CLI `task list` and `task inspect` and both MCP task tools refuse the + same way. ## Commands @@ -354,7 +357,8 @@ until its recipes are re-reviewed. | An attached `-m` module (`python -mpytest`) as the program's operand | accepted, also after a value option's value (`python -W ignore -mpytest`); `python -mcode`, `python -m`, `python -Ximporttime` and a `-m` taken as an option's value (`python -W -m --`, `python -X -mpytest`) still refused (stdin) | domain test | | `psql -o`/`--output` piped to a command (`-o '\|cmd'`, `-o\|cmd`, `-qo\|cmd`, `--output=\|cmd`, `--out=\|cmd`) | `inline_command`; `psql -o out.txt`, `psql -F'\|'`, `psql -L '\|log'` (a file name) still pass | domain test | | Dashed Git builtins (`/usr/lib/git-core/git-clone -u cmd`, `git-config alias.x '!cmd'`, `git-ls-remote --upload-pack=cmd`, `git-fetch-pack --exec=cmd`) | `inline_command`; `git-clone url dir`, `git-push -u origin main`, `git-config alias.st status` still pass | domain test | -| Git plumbing helpers read as their porcelain (`git-bisect--helper --bisect-run cmd`, `/usr/lib/git-core/git-bisect--helper.exe run cmd`, `git bisect--helper --bisect-run cmd`, `git-submodule--helper foreach cmd`) | `inline_command`; `git-submodule--helper status`, `git-difftool--helper a b`, `git add bisect--helper.txt` still pass | domain test | +| Git plumbing helpers read as their porcelain (`git-bisect--helper --bisect-run cmd`, `/usr/lib/git-core/git-bisect--helper.exe run cmd`, `git bisect--helper --bisect-run cmd`, `git-submodule--helper foreach cmd`, `git-config alias.x bisect--helper`, older Git's `git rebase--helper --add-exec-commands cmd` in any letter case) | `inline_command`; `git-submodule--helper status`, `git-difftool--helper a b`, `git add bisect--helper.txt`, `git config --add alias.st status` still pass | domain test | +| Detached linked worktree with no named instance, on CLI `task list`/`task inspect` and MCP `hack.project.task.list`/`inspect` | `detached_checkout`; naming the instance (`--branch`, MCP `branch`) passes | paired fixture, domain test | | `duckdb -storage-version VER db` | accepted; a SQL operand after the database is still refused | domain test | | `git clone -u` (also after `--attr-source ` or an unknown global option), abbreviated or attached `--upload-pack`/`--receive-pack`/`--exec`, `clone --config`, and `!` alias values (`git config alias.x '!cmd'`, `git -c alias.x=!cmd`) | `inline_command`; `git clone url dir`, `git push -u origin main`, `git add -u`, `git config alias.st status` still pass | domain test | | Caller `GIT_DIR`/`GIT_INDEX_FILE`/`GIT_WORK_TREE` naming another index that tracks a secret | `input_path_invalid` with no digest; a foreign `GIT_DIR` does not hide the checkout's own index | domain test | @@ -415,9 +419,14 @@ until its recipes are re-reviewed. `pickle` or `marshal`, or reads a config file that it evaluates. A Git `--helper` plumbing command (`git-bisect--helper`, `git submodule--helper`) is read as `git `, so it meets the rules for - that subcommand, but a helper's own options are not modeled: the - `--add-exec-commands` mode of older Git's `rebase--helper` is not - refused. Git config writes other than `!` aliases (for example + that subcommand. A helper's own options are modeled only where they run a + command: older Git's `rebase--helper --add-exec-commands` is refused + through a flag scoped to `rebase`. Any whole-word `--helper` + argument is read as `` (fail-closed), so + `git push -u origin clone--helper` (a branch of that name) and + `git add bisect--helper` (a file of that name) are refused. The `rebase`-scoped flag counts when any + operand is `rebase`, and Git abbreviates, so + `git config --add alias.r rebase` is refused too. Git config writes other than `!` aliases (for example `git config core.pager cmd` or `include.path`) are not detected. Nor are `mysql`/`mariadb` option files named in argv (`--defaults-extra-file`, which can set `init-command`) or `--pager`, which runs a command only in diff --git a/src/lib/task-catalog-recipe.ts b/src/lib/task-catalog-recipe.ts index 68e12f6f8..4aa589b23 100644 --- a/src/lib/task-catalog-recipe.ts +++ b/src/lib/task-catalog-recipe.ts @@ -664,7 +664,10 @@ const EVALUATORS: Readonly> = { // Git accepts abbreviated long options (`--upl=cmd`) and a value attached // to a short option (`-u/path/cmd`). A dashed builtin (`git-clone`, // `/usr/lib/git-core/git-config`) is read as `git `, and a plumbing - // helper (`git-bisect--helper`, `git submodule--helper`) as its porcelain. + // helper (`git-bisect--helper`, `git submodule--helper`) as its porcelain, + // so older Git's `rebase--helper --add-exec-commands cmd` meets a + // `rebase`-scoped flag. That flag is scoped because, as a `git` inline + // flag, it would also match `git config --add` (Git abbreviates options). git: evaluator({ inline: [ "-c", @@ -694,7 +697,7 @@ const EVALUATORS: Readonly> = { // cluster: `git log -Scache` and `git commit -mcleanup` are refused // (fail-closed); pass the value as its own argument instead. shortValueOptions: [], - subcommandInline: { clone: ["-u"] }, + subcommandInline: { clone: ["-u"], rebase: ["--add-exec-commands"] }, bangCommands: true, }), // SQL clients given a statement string. A statement can also reach the @@ -864,6 +867,10 @@ const GIT_HELPER = /^([a-z0-9][a-z0-9-]*?)--helper$/i; * and `submodule--helper foreach` the `foreach` rule. Any other argument is * unchanged. A matching argument starts with a letter or digit and holds no * `=` or `!`, so no inline flag, word or `!` value is lost by the rewrite. + * The porcelain is lowercased, like a program name, so a mixed-case helper + * (`git Rebase--Helper`) still meets the subcommand-scoped flags, which are + * keyed by lowercase name. Git itself matches case-sensitively, so this + * only refuses more (fail-closed). */ function gitSubcommand(arg: string): string { const helper = GIT_HELPER.exec(arg); diff --git a/tests/project-task-catalog.test.ts b/tests/project-task-catalog.test.ts index e44e040f7..6d886fdf4 100644 --- a/tests/project-task-catalog.test.ts +++ b/tests/project-task-catalog.test.ts @@ -1323,6 +1323,8 @@ describe("recipe grammar", () => { // HACK-1331: each refused form names the rule that catches it; the paired // accepted forms prove the rule is scoped, not a blanket refusal. const gapCases: readonly { + /** Issue that added the rule, when it is not HACK-1331. */ + readonly issue?: string; readonly rule: string; readonly refused: readonly (readonly string[])[]; readonly accepted: readonly (readonly string[])[]; @@ -1661,9 +1663,18 @@ describe("recipe grammar", () => { { // A `--helper` plumbing command, dashed (also by an absolute // git-core path or with `.exe`) or under `git`, is `git `. + issue: "HACK-1373", rule: "Git plumbing helpers read as their porcelain", refused: [ ["git-bisect--helper", "--bisect-run", "id"], + // HACK-1411: a dashed builtin's trailing arguments are mapped too, + // so an alias defined as the helper meets the `bisect` rule. + ["git-config", "alias.x", "bisect--helper"], + // HACK-1411: older Git's `rebase--helper --add-exec-commands cmd`, + // in any letter case, meets the `rebase`-scoped flag. + ["git", "rebase--helper", "--add-exec-commands", "id"], + ["git-rebase--helper", "--add-exec", "id"], + ["git", "Rebase--Helper", "--add-exec-commands", "id"], ["/usr/lib/git-core/git-bisect--helper", "run", "id"], ["/usr/libexec/git-core/git-bisect--helper.exe", "--bisect-run", "id"], ["exec", "Git-Bisect--Helper", "--bisect-run", "id"], @@ -1681,12 +1692,16 @@ describe("recipe grammar", () => { ["git", "log", "--grep=bisect--helper"], // Only a whole `--helper` word is a helper, not a file name. ["git", "add", "bisect--helper.txt"], + // `--add-exec-commands` is scoped to `rebase`, so `--add` elsewhere + // still passes. + ["git", "config", "--add", "alias.st", "status"], + ["git", "rebase--helper", "--continue"], ], }, ]; for (const gap of gapCases) { - test(`HACK-1331: ${gap.rule}`, () => { + test(`${gap.issue ?? "HACK-1331"}: ${gap.rule}`, () => { for (const argv of gap.refused) { expect({ argv, inline: isInlineCommandArgv(argv) }).toEqual({ argv, @@ -1729,6 +1744,21 @@ describe("recipe grammar", () => { ).toBe(true); }); + test("HACK-1411: documented Git helper fail-closed collateral stays refused", () => { + // Any whole-word `--helper` argument is read as ``, so a + // branch named `clone--helper` scopes `-u` to `clone` and a file named + // `bisect--helper` meets the `bisect` rule. + expect( + isInlineCommandArgv(["git", "push", "-u", "origin", "clone--helper"]) + ).toBe(true); + expect(isInlineCommandArgv(["git", "add", "bisect--helper"])).toBe(true); + // Any operand named `rebase` scopes `--add-exec-commands` to it, and + // Git abbreviates, so `--add` before an alias value `rebase` is refused. + expect( + isInlineCommandArgv(["git", "config", "--add", "alias.r", "rebase"]) + ).toBe(true); + }); + test("effects must be covered by required capabilities", () => { const base = recipe({ id: "ok" }); const pinAccepted = (path: string): boolean => @@ -1917,11 +1947,11 @@ async function writeEmptyRegistry(name: string): Promise { /** * A committed one-recipe project plus a detached linked worktree of it, both - * inside the fixture root. Returns the linked worktree's root. + * inside the fixture root under `name`. Returns the linked worktree's root. */ -async function writeDetachedWorktree(): Promise { - const primary = join(fixture.root, "detached-primary"); - const linked = join(fixture.root, "detached-linked"); +async function writeDetachedWorktree(name = "detached"): Promise { + const primary = join(fixture.root, `${name}-primary`); + const linked = join(fixture.root, `${name}-linked`); await mkdir(join(primary, ".hack"), { recursive: true }); await mkdir(join(primary, "scripts"), { recursive: true }); await writeFile( @@ -2050,4 +2080,71 @@ describe("repository catalog", () => { }, CASE_TIMEOUT_MS ); + + test( + "HACK-1383: CLI task inspect and the MCP task tools refuse an implicit instance in a detached linked worktree", + async () => { + const linked = await writeDetachedWorktree("detached-surfaces"); + const detached = targetRefusalEnvelope({ reason: "detached_checkout" }); + const implicit = { path: linked, env: "base" }; + const named = { ...implicit, branch: REVIEW_INSTANCE }; + + // Each surface has its own path to the selector, so each refusal is + // asserted separately, CLI first: the MCP tools spawn the CLI. + const cliInspect = await runCli( + buildTaskInspectArgs({ ...implicit, task: "lint" }) + ); + expect({ + surface: "cli inspect", + exitCode: cliInspect.exitCode, + envelope: cliInspect.envelope, + }).toEqual({ surface: "cli inspect", exitCode: 1, envelope: detached }); + const mcpList = await runMcp("hack.project.task.list", implicit); + expect({ + surface: "mcp list", + exitCode: mcpList.exitCode, + spawned: mcpList.spawned, + data: mcpList.data, + }).toEqual({ + surface: "mcp list", + exitCode: 1, + spawned: undefined, + data: detached, + }); + const mcpInspect = await runMcp("hack.project.task.inspect", { + ...implicit, + task: "lint", + }); + expect({ + surface: "mcp inspect", + exitCode: mcpInspect.exitCode, + spawned: mcpInspect.spawned, + data: mcpInspect.data, + }).toEqual({ + surface: "mcp inspect", + exitCode: 1, + spawned: undefined, + data: detached, + }); + + // Naming the instance passes on every surface. + const cliNamed = parseTaskInspectEnvelope( + (await runCli(buildTaskInspectArgs({ ...named, task: "lint" }))) + .envelope + ); + expect(cliNamed?.ok && cliNamed.data.task.id).toBe("lint"); + const mcpListNamed = parseTaskListEnvelope( + (await runMcp("hack.project.task.list", named)).data + ); + expect(mcpListNamed?.ok && statuses(mcpListNamed.data.tasks)).toEqual([ + [0, "lint", "reviewed"], + ]); + const mcpInspectNamed = parseTaskInspectEnvelope( + (await runMcp("hack.project.task.inspect", { ...named, task: "lint" })) + .data + ); + expect(mcpInspectNamed?.ok && mcpInspectNamed.data.task.id).toBe("lint"); + }, + CASE_TIMEOUT_MS + ); });