From 37df79b95c1c12657e617d6b8846205419d4a8eb Mon Sep 17 00:00:00 2001 From: dbarr5 Date: Tue, 6 Oct 2026 08:40:30 -0400 Subject: [PATCH] fix(ship): preserve quoted slash arguments and reject invalid flags --- docs/generated/commands.md | 6 +- src/commands/command_manifest_data.ts | 12 +-- src/commands/ship.ts | 119 +++++++++++++++++++++++--- src/commands/slash.ts | 16 +++- test/review_ship_e2e.test.ts | 97 ++++++++++++++++++++- 5 files changed, 222 insertions(+), 28 deletions(-) diff --git a/docs/generated/commands.md b/docs/generated/commands.md index 39bc2f19..0be8ffbe 100644 --- a/docs/generated/commands.md +++ b/docs/generated/commands.md @@ -1,5 +1,5 @@ - + # Generated command reference This reference is generated from the validated, versioned command manifest. Availability is evaluated at runtime; a listed command may still require authentication, a hosted capability, or local tooling. @@ -126,7 +126,7 @@ Command flags: - `--body ` - `--base ` -#### `aether ship [--title t] [--base b]` +#### `aether ship [--title text] [--body text] [--base branch] [--approve publish] [--yes] [--json]` publish the head branch and open a pull request @@ -719,7 +719,7 @@ review changes, pick files or hunks, commit Permission: `unknown` · Availability: `runtime-dependent` · Telemetry: `slash.review` -#### `/ship [--title t] [--base b]` +#### `/ship [--title text] [--body text] [--base branch] [--approve publish] [--yes] [--json] [--help]` publish the head branch and open a pull request diff --git a/src/commands/command_manifest_data.ts b/src/commands/command_manifest_data.ts index e5746f45..e33d39a3 100644 --- a/src/commands/command_manifest_data.ts +++ b/src/commands/command_manifest_data.ts @@ -2741,9 +2741,9 @@ export const COMMAND_MANIFEST_SOURCE: readonly CommandManifestEntry[] = [ "aliases": [], "compatibilityAliases": [], "deprecatedAliases": [], - "args": "[--title t] [--base b]", + "args": "[--title text] [--body text] [--base branch] [--approve publish] [--yes] [--json]", "summary": "publish the head branch and open a pull request", - "detailedHelp": "aether ship [--title t] [--base b]\npublish the head branch and open a pull request", + "detailedHelp": "aether ship [--title text] [--body text] [--base branch] [--approve publish] [--yes] [--json]\npublish the head branch and open a pull request\n--title sets the PR title; --body sets the PR body; --base selects the target branch. --approve publish authorizes publication; --yes alone does not. --json previews the planned argv without publishing.", "section": "Start", "hidden": false, "permissionClass": "destructive", @@ -2823,7 +2823,7 @@ export const COMMAND_MANIFEST_SOURCE: readonly CommandManifestEntry[] = [ "module": "src/commands/command_manifest_data.ts", "symbol": "COMMAND_MANIFEST_SOURCE", "target": "ship", - "usage": "aether ship [--title t] [--base b]", + "usage": "aether ship [--title text] [--body text] [--base branch] [--approve publish] [--yes] [--json]", "visible": true, "disposition": "generated" }, @@ -5634,9 +5634,9 @@ export const COMMAND_MANIFEST_SOURCE: readonly CommandManifestEntry[] = [ "aliases": [], "compatibilityAliases": [], "deprecatedAliases": [], - "args": "[--title t] [--base b]", + "args": "[--title text] [--body text] [--base branch] [--approve publish] [--yes] [--json] [--help]", "summary": "publish the head branch and open a pull request", - "detailedHelp": "/ship [--title t] [--base b]\npublish the head branch and open a pull request", + "detailedHelp": "/ship [--title text] [--body text] [--base branch] [--approve publish] [--yes] [--json] [--help]\npublish the head branch and open a pull request\nQuote values containing spaces or newlines. --title sets the PR title; --body sets the PR body; --base selects the target branch. --approve publish authorizes publication; --yes alone does not. --json previews the planned argv without publishing. --body-file is not supported.", "section": "UVT Tools", "hidden": false, "permissionClass": "unknown", @@ -5658,7 +5658,7 @@ export const COMMAND_MANIFEST_SOURCE: readonly CommandManifestEntry[] = [ "module": "src/commands/command_manifest_data.ts", "symbol": "COMMAND_MANIFEST_SOURCE", "target": "ship", - "usage": "/ship [--title t] [--base b]", + "usage": "/ship [--title text] [--body text] [--base branch] [--approve publish] [--yes] [--json] [--help]", "visible": true, "disposition": "generated" }, diff --git a/src/commands/ship.ts b/src/commands/ship.ts index cf3323b9..2bab69b4 100644 --- a/src/commands/ship.ts +++ b/src/commands/ship.ts @@ -324,20 +324,111 @@ export async function cmdShip(ctx: AppContext, _rest: string[], flags: ShipFlags return runShip(ctx, defaultShipDeps(ctx.flags.cwd, process.stdout), flags); } -/** `/ship` inside the REPL. */ -export async function shipSlash(ctx: AppContext, out: Writable, arg: string): Promise { - const parts = arg.trim().split(/\s+/).filter(Boolean); - const valueOf = (name: string): string | undefined => { - const at = parts.indexOf(name); - return at >= 0 ? parts[at + 1] : undefined; - }; - await runShip(ctx, defaultShipDeps(ctx.flags.cwd, out), { - yes: false, - json: false, - ...(valueOf("--title") !== undefined ? { title: valueOf("--title") } : {}), - ...(valueOf("--base") !== undefined ? { base: valueOf("--base") } : {}), - ...(valueOf("--approve") !== undefined ? { approve: valueOf("--approve") } : {}), - }); +export const SHIP_SLASH_USAGE = + 'usage: /ship [--title ] [--body ] [--base ] [--approve publish] [--yes] [--json] [--help]'; + +interface SlashWord { value: string; quoted: boolean } + +/** Split REPL input into argv without invoking a shell or expanding any text. */ +function shipSlashWords(input: string): SlashWord[] { + const words: SlashWord[] = []; + let value = ""; + let active = false; + let quoted = false; + let quote: "'" | '"' | null = null; + for (let index = 0; index < input.length; index++) { + const char = input[index]!; + if (char === "\\" && quote !== "'") { + const next = input[index + 1]; + if (next === undefined) throw new Error("trailing escape"); + // Escapes quote characters, backslashes and whitespace. Other escapes + // remain literal, including Windows paths and shell-looking text. + if (next === "\\" || next === '"' || next === "'" || next === "$" || next === "`" || /\s/u.test(next) || (quote === null && next === "-")) { + value += next; + index++; + quoted = true; + } else { + value += char; + } + active = true; + continue; + } + if (quote !== null) { + if (char === quote) quote = null; + else value += char; + active = true; + continue; + } + if (char === "'" || char === '"') { + quote = char; + quoted = true; + active = true; + } else if (/\s/u.test(char)) { + if (active) words.push({ value, quoted }); + value = ""; + active = false; + quoted = false; + } else { + value += char; + active = true; + } + } + if (quote !== null) throw new Error("unterminated quote"); + if (active) words.push({ value, quoted }); + return words; +} + +/** Parse exactly the options `/ship` can pass to the shared ship rail. */ +export function parseShipSlashArgs(arg: string): ShipFlags | "help" { + const words = shipSlashWords(arg); + const flags: ShipFlags = { yes: false, json: false }; + const seen = new Set(); + const valued = new Set(["--title", "--body", "--base", "--approve"]); + const boolean = new Set(["--yes", "--json", "--help"]); + for (let index = 0; index < words.length; index++) { + const word = words[index]!; + const equal = word.value.indexOf("="); + const name = equal < 0 ? word.value : word.value.slice(0, equal); + if (!name.startsWith("-") || (word.quoted && equal < 0)) throw new Error(`unexpected argument: ${word.value}`); + if (!valued.has(name) && !boolean.has(name)) throw new Error(`unsupported option: ${name}`); + if (seen.has(name)) throw new Error(`duplicate option: ${name}`); + seen.add(name); + if (boolean.has(name)) { + if (equal >= 0) throw new Error(`${name} does not take a value`); + if (name === "--yes") flags.yes = true; + if (name === "--json") flags.json = true; + continue; + } + const next = equal < 0 ? words[++index] : { value: word.value.slice(equal + 1), quoted: word.quoted }; + if (!next || !next.value.trim() || (equal < 0 && next.value.startsWith("-") && !next.quoted)) { + throw new Error(`${name} needs a value`); + } + if (name === "--title") flags.title = next.value; + if (name === "--body") flags.body = next.value; + if (name === "--base") flags.base = next.value; + if (name === "--approve") flags.approve = next.value; + } + if (seen.has("--help")) { + if (seen.size !== 1) throw new Error("--help must be used alone"); + return "help"; + } + return flags; +} + +/** `/ship` inside the REPL. The optional deps also keep wrapper tests offline. */ +export async function shipSlash(ctx: AppContext, out: Writable, arg: string, deps?: ShipDeps): Promise { + let flags: ShipFlags | "help"; + try { + flags = parseShipSlashArgs(arg); + } catch (error) { + out.write(`✗ ${error instanceof Error ? error.message : String(error)}\n${SHIP_SLASH_USAGE}\n`); + return; + } + if (flags === "help") { + out.write(`${SHIP_SLASH_USAGE}\n`); + return; + } + await runShip(ctx, deps ?? defaultShipDeps(ctx.flags.cwd, out), flags); } /** diff --git a/src/commands/slash.ts b/src/commands/slash.ts index 5e634b22..98298dd0 100644 --- a/src/commands/slash.ts +++ b/src/commands/slash.ts @@ -103,15 +103,23 @@ function byKind(cat: CatalogResponse, kind: Kind): CatalogItem[] { return cat.models.filter((m) => m.kind === kind); } +export function splitSlashCommand(line: string): { cmd: string; arg: string } { + const input = line.slice(1).trim(); + const separator = input.search(/\s/u); + const cmd = (separator < 0 ? input : input.slice(0, separator)).toLowerCase(); + // Keep the argument text intact. /ship parses quoting and may contain a + // literal newline in a quoted PR body; splitting here would corrupt it. + const arg = separator < 0 ? "" : input.slice(separator).trim(); + return { cmd, arg }; +} + export async function handleSlash( ctx: AppContext, line: string, out: Writable, signal?: AbortSignal, ): Promise { - const parts = line.slice(1).trim().split(/\s+/); - const cmd = (parts[0] ?? "").toLowerCase(); - const arg = parts.slice(1).join(" "); + const { cmd, arg } = splitSlashCommand(line); switch (cmd) { case "terminal": @@ -231,7 +239,7 @@ export async function handleSlash( break; } case "agent-create": { - await cmdManagedAgents(ctx, ["create", ...parts.slice(1)], { out, err: out, signal, hooks: createAtsHooks({ output: text => { out.write(text); } }) }); + await cmdManagedAgents(ctx, ["create", ...(arg ? arg.split(/\s+/u) : [])], { out, err: out, signal, hooks: createAtsHooks({ output: text => { out.write(text); } }) }); break; } case "doctor": { diff --git a/test/review_ship_e2e.test.ts b/test/review_ship_e2e.test.ts index efa252f2..ec6b66ca 100644 --- a/test/review_ship_e2e.test.ts +++ b/test/review_ship_e2e.test.ts @@ -15,6 +15,7 @@ import { test } from "node:test"; import assert from "node:assert/strict"; +import { parseArgs } from "node:util"; import { spawnSync } from "node:child_process"; import { chmodSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; import { join } from "node:path"; @@ -25,7 +26,9 @@ import type { RunResult, Runner } from "../src/core/worktree.js"; import { defaultRunner } from "../src/core/worktree.js"; import { readRepoState } from "../src/core/review_state.js"; import { runReview, type ReviewDeps } from "../src/commands/review.js"; -import { runShip, type ShipDeps } from "../src/commands/ship.js"; +import { parseShipSlashArgs, runShip, shipSlash, type ShipDeps } from "../src/commands/ship.js"; +import { CLI_PARSE_OPTIONS } from "../src/commands/cli_registry.js"; +import { splitSlashCommand } from "../src/commands/slash.js"; import { spawnAsyncRun } from "../src/commands/review_counts.js"; import { TEMP_ROOT } from "./tmp_workspace.js"; @@ -153,6 +156,98 @@ const remoteBranches = (remote: string): string[] => .filter(Boolean) .sort(); +test("/ship rejects malformed or unsupported input before any git, push, PR, or confirmation call", async () => { + const cases: Array<[string, RegExp]> = [ + ["--title", /--title needs a value/], + ["--title --base main", /--title needs a value/], + ['--title "unterminated', /unterminated quote/], + ["--title hello\\", /trailing escape/], + ["--body-file proposal.md", /unsupported option: --body-file/], + ["--title first --title second", /duplicate option: --title/], + ["--json --json", /duplicate option: --json/], + ["--title good stray", /unexpected argument: stray/], + ["--title=", /--title needs a value/], + ["--yes=true", /--yes does not take a value/], + ]; + for (const [arg, message] of cases) { + const out = sink(); + let calls = 0; + let prompts = 0; + const deps: ShipDeps = { + run: () => { calls++; throw new Error("git or gh must not run"); }, + cwd: "unused", + out: out.out, + io: { tty: true, note: () => {}, question: async () => { prompts++; return "y"; } }, + }; + await shipSlash(ctx, out.out, arg, deps); + assert.match(out.text(), message, arg); + assert.match(out.text(), /usage: \/ship/, arg); + assert.equal(calls, 0, arg); + assert.equal(prompts, 0, arg); + } +}); + +test("/ship handles escaped spaces, equals values, and help without running Git", async () => { + assert.deepEqual(parseShipSlashArgs('--title=Fix\\ auth\\ and\\ model\\ picker --base="main"'), { + yes: false, json: false, title: "Fix auth and model picker", base: "main", + }); + assert.deepEqual(parseShipSlashArgs('--title "C:\\work\\file $(id)"'), { + yes: false, json: false, title: "C:\\work\\file $(id)", + }); + const out = sink(); + await shipSlash(ctx, out.out, "--help", { + run: () => { throw new Error("Git must not run for help"); }, + cwd: "unused", out: out.out, io: io([]), + }); + assert.match(out.text(), /--title.*--body.*--base.*--approve.*--yes.*--json/); + assert.equal(out.text().includes("--body-file"), false); +}); + +test("/ship and CLI preserve the same quoted Unicode, newlines, and literal shell-looking argv", async () => { + const fixture = repoFixture(); + try { + writeFileSync(join(fixture.repo, "kept.ts"), "changed\n"); + git(fixture.repo, "commit", "-am", "fix: source"); + const title = 'Fix auth and model picker — "safe" $(whoami) `whoami`'; + const body = "line one\nline two ☃ $HOME $(id) `id`"; + const slashArg = '--title "Fix auth and model picker — \\"safe\\" $(whoami) `whoami`" ' + + "--body 'line one\nline two ☃ $HOME $(id) `id`' --base main"; + assert.deepEqual(splitSlashCommand(`/ship ${slashArg}`), { cmd: "ship", arg: slashArg }); + const cli = parseArgs({ + args: ["ship", "--title", title, "--body", body, "--base", "main", "--approve", "publish"], + allowPositionals: true, + strict: true, + options: CLI_PARSE_OPTIONS, + }); + assert.deepEqual(cli.positionals, ["ship"]); + assert.equal(cli.values["title"], title); + assert.equal(cli.values["body"], body); + const parsed = parseShipSlashArgs(slashArg); + assert.notEqual(parsed, "help"); + assert.equal((parsed as { title?: string }).title, cli.values["title"]); + assert.equal((parsed as { body?: string }).body, cli.values["body"]); + + const { run, calls } = railRunner(okGh); + const preview = sink(); + await shipSlash(ctx, preview.out, `${slashArg} --json`, shipDeps(fixture, run, preview.out)); + const planned = JSON.parse(preview.text()) as { commands: Array<{ cmd: string; args: string[] }> }; + const plannedGh = planned.commands.find((command) => command.cmd === "gh")!.args; + assert.equal(plannedGh[plannedGh.indexOf("--title") + 1], title); + assert.equal(plannedGh[plannedGh.indexOf("--body") + 1], body); + assert.equal(calls.some((call) => call[0] === "gh" || call.includes("push")), false); + + const published = sink(); + await shipSlash(ctx, published.out, `${slashArg} --approve publish`, shipDeps(fixture, run, published.out)); + const created = calls.find((call) => call[0] === "gh" && call[1] === "pr" && call[2] === "create"); + assert.ok(created, published.text()); + assert.deepEqual(created.slice(1), plannedGh); + assert.match(published.text(), /Fix auth and model picker/); + assert.match(published.text(), /PR opened:/); + } finally { + fixture.cleanup(); + } +}); + // ── the whole rail ────────────────────────────────────────────────────────── test("the whole rail: edit → select → stage → commit → publish → PR", async () => {