diff --git a/src/commands/batchApply.ts b/src/commands/batchApply.ts index 204eac6..da87cde 100644 --- a/src/commands/batchApply.ts +++ b/src/commands/batchApply.ts @@ -10,6 +10,7 @@ import { presentCliResultInOutput } from "../logging/outputChannel.js"; import { activeWorkspaceFolder } from "../workspace/readiness.js"; +import { serializePatchloomArgs } from "./quickActions.js"; // Batch replace is PATH OLD NEW (not CLI `replace OLD --new NEW path`). See CLI 0.18+ batch --help. // doc.update / doc.delete_where are the multi-match siblings of doc.set / doc.delete @@ -44,9 +45,9 @@ export function isEmptyBatchPlan(plan: string): boolean { return parseBatchOperationCount(plan) === 0; } -/** CLI argv for Batch Apply. Global --contain first (CLI 0.10+ path guard). */ +/** CLI argv for Batch Apply. Flags come from serializePatchloomArgs, not from scanning operands. */ export function buildBatchApplyArgs(): string[] { - return ["--contain", "batch", "--apply"]; + return serializePatchloomArgs({ args: ["batch"], apply: true, contain: true }); } export async function batchApply(): Promise { diff --git a/src/commands/quickActions.ts b/src/commands/quickActions.ts index 8a00ee7..7e2bf7e 100644 --- a/src/commands/quickActions.ts +++ b/src/commands/quickActions.ts @@ -27,7 +27,31 @@ export interface PlannedQuickAction { readonly title: string; readonly targetPath: string; readonly targetArgIndices: readonly number[]; + /** Subcommand and operands. Do not put `--apply` or `--contain` here. */ readonly args: readonly string[]; + /** When true, `serializePatchloomArgs` appends `--apply`. */ + readonly apply?: boolean; +} + +export interface PatchloomInvocation { + readonly args: readonly string[]; + readonly apply?: boolean; + /** When true, prefix global `--contain`. `executePatchloom` defaults this to true. */ + readonly contain?: boolean; +} + +/** + * Build argv from flags plus operands. Never scans `args` for flag names. + */ +export function serializePatchloomArgs(invocation: PatchloomInvocation): string[] { + const argv = [...invocation.args]; + if (invocation.contain) { + argv.unshift("--contain"); + } + if (invocation.apply) { + argv.push("--apply"); + } + return argv; } export function presentSearchOutcome( @@ -398,7 +422,7 @@ export async function runQuickAction(): Promise { } const action = buildSearchQuickAction(folder.uri.fsPath, pattern, glob || undefined); - const result = await executePatchloom(binaryPath, action.args, folder.uri.fsPath); + const result = await executePatchloom(binaryPath, action, folder.uri.fsPath); const log = getPatchloomLog(); const outcome = presentSearchOutcome(log, result); @@ -445,7 +469,7 @@ export async function runQuickAction(): Promise { const action = buildSearchQuickAction(folder.uri.fsPath, pattern, glob || undefined, { filesWithoutMatch: true }); - const result = await executePatchloom(binaryPath, action.args, folder.uri.fsPath); + const result = await executePatchloom(binaryPath, action, folder.uri.fsPath); const log = getPatchloomLog(); const outcome = presentSearchOutcome(log, result); @@ -501,7 +525,7 @@ export async function runQuickAction(): Promise { } const action = buildCreateQuickAction(absolutePath, content); - const result = await executePatchloom(binaryPath, action.args, folder.uri.fsPath); + const result = await executePatchloom(binaryPath, action, folder.uri.fsPath); if (result.exitCode !== 0) { await vscode.window.showErrorMessage(`Patchloom create failed: ${formatCliOutput(result)}`); @@ -578,7 +602,7 @@ export async function runQuickAction(): Promise { } const action = buildDocGetQuickAction(target.absolutePath, selector); - const result = await executePatchloom(binaryPath, action.args, target.workspaceFolder.uri.fsPath); + const result = await executePatchloom(binaryPath, action, target.workspaceFolder.uri.fsPath); if (result.exitCode !== 0) { await vscode.window.showErrorMessage(`Patchloom doc get failed: ${formatCliOutput(result)}`); @@ -1061,7 +1085,7 @@ export async function runQuickAction(): Promise { const staged = await stageExternalPatchInWorkspace(folder.uri.fsPath, patchUri[0].fsPath); try { const planned = retargetQuickAction(action, staged.patchPath); - const result = await executePatchloom(binaryPath, planned.args, folder.uri.fsPath); + const result = await executePatchloom(binaryPath, planned, folder.uri.fsPath); const log = getPatchloomLog(); presentCliResultInOutput(log, result); @@ -1111,7 +1135,7 @@ export async function runQuickAction(): Promise { const staged = await stageExternalPatchInWorkspace(folder.uri.fsPath, patchUri[0].fsPath); try { const planned = retargetQuickAction(action, staged.patchPath); - const result = await executePatchloom(binaryPath, planned.args, folder.uri.fsPath); + const result = await executePatchloom(binaryPath, planned, folder.uri.fsPath); const log = getPatchloomLog(); const outcome = presentPatchMergeOutcome(log, result); @@ -1151,7 +1175,7 @@ export async function runQuickAction(): Promise { } const action = buildUndoQuickAction(folder.uri.fsPath); - const result = await executePatchloom(binaryPath, action.args, folder.uri.fsPath); + const result = await executePatchloom(binaryPath, action, folder.uri.fsPath); if (result.exitCode !== 0) { await vscode.window.showWarningMessage(formatUndoFailureMessage(result)); @@ -1302,7 +1326,8 @@ export function buildCreateQuickAction(filePath: string, content = ""): PlannedQ targetPath: filePath, targetArgIndices: [1], // CLI requires --content/--stdin and --apply; preview-only create returns exit 2 and does not write. - args: ["create", filePath, "--content", content, "--apply"] + args: ["create", filePath, "--content", content], + apply: true }; } @@ -1474,12 +1499,13 @@ export function buildPatchApplyQuickAction(patchPath: string): PlannedQuickActio title: `Apply patch ${path.basename(patchPath)}`, targetPath: patchPath, targetArgIndices: [2], - args: ["patch", "apply", patchPath, "--apply"] + args: ["patch", "apply", patchPath], + apply: true }; } export function buildPatchMergeQuickAction(patchPath: string, allowConflicts: boolean): PlannedQuickAction { - const args: string[] = ["patch", "merge", patchPath, "--apply"]; + const args: string[] = ["patch", "merge", patchPath]; if (allowConflicts) { args.push("--allow-conflicts"); } @@ -1487,7 +1513,8 @@ export function buildPatchMergeQuickAction(patchPath: string, allowConflicts: bo title: `Merge patch ${path.basename(patchPath)}`, targetPath: patchPath, targetArgIndices: [2], - args + args, + apply: true }; } @@ -1496,7 +1523,8 @@ export function buildUndoQuickAction(workspacePath: string): PlannedQuickAction title: "Undo last patchloom change", targetPath: workspacePath, targetArgIndices: [], - args: ["undo", "--apply"] + args: ["undo"], + apply: true }; } @@ -1512,7 +1540,7 @@ export function isAllowedPreviewMiss(action: PlannedQuickAction, exitCode: numbe if (exitCode !== 3) { return false; } - // PlannedQuickAction.args are pre-contain. executePatchloom prepends --contain later. + // PlannedQuickAction.args are operands only. executePatchloom adds --contain. // doc update and delete-where: exit 3 is a path miss (key not found), not a soft no-op. return !( action.args[0] === "doc" && @@ -1535,17 +1563,7 @@ export function retargetQuickAction(action: PlannedQuickAction, nextTargetPath: }; } -export function withApplyFlag(args: readonly string[]): string[] { - return args.at(-1) === "--apply" ? [...args] : [...args, "--apply"]; -} -/** - * Prepend the global `--contain` flag so CLI ops cannot escape the cwd workspace. - * Global flags must appear before the subcommand (`patchloom --contain replace ...`). - */ -export function withContainFlag(args: readonly string[]): string[] { - return args[0] === "--contain" ? [...args] : ["--contain", ...args]; -} async function previewAndMaybeApply( binaryPath: string, @@ -1591,7 +1609,11 @@ async function previewAndMaybeApply( return; } - const result = await executePatchloom(binaryPath, withApplyFlag(action.args), target.workspaceFolder.uri.fsPath); + const result = await executePatchloom( + binaryPath, + { args: action.args, apply: true }, + target.workspaceFolder.uri.fsPath + ); if (result.exitCode !== 0) { presentCliResultInOutput(getPatchloomLog(), result); await vscode.window.showErrorMessage( @@ -1620,7 +1642,7 @@ async function buildPreviewDocument( try { await fs.writeFile(tempPath, originalContent, "utf8"); const previewAction = retargetQuickAction(action, tempPath); - const result = await executePatchloom(binaryPath, withApplyFlag(previewAction.args), tempDir); + const result = await executePatchloom(binaryPath, { args: previewAction.args, apply: true }, tempDir); if (result.exitCode !== 0 && !isAllowedPreviewMiss(action, result.exitCode)) { presentCliResultInOutput(getPatchloomLog(), result); throw new Error(formatQuickActionCliOutput(result)); @@ -1873,18 +1895,16 @@ async function ensureWorkspaceFileReady(target: WorkspaceFileTarget): Promise { - const finalArgs = options.contain === false ? [...args] : withContainFlag(args); + const finalArgs = serializePatchloomArgs({ + args: invocation.args, + apply: invocation.apply, + contain: invocation.contain !== false + }); const log = getPatchloomLog(); const runtime = await getPatchloomRuntimeConfig(); const env = mergePatchloomEnv(process.env, runtime.extraEnv); diff --git a/test/unit/patchloomCli.test.ts b/test/unit/patchloomCli.test.ts index c25a3ca..1086f9d 100644 --- a/test/unit/patchloomCli.test.ts +++ b/test/unit/patchloomCli.test.ts @@ -37,7 +37,7 @@ import { buildReplaceQuickAction, buildSearchQuickAction, retargetQuickAction, - withApplyFlag + serializePatchloomArgs } from "../../src/commands/quickActions.js"; import { configureMcpTargets, inspectMcpTargets } from "../../src/mcp/config.js"; import { performManagedInstall } from "../../src/install/managed.js"; @@ -380,7 +380,7 @@ describe("patchloom CLI integration", async () => { await fs.writeFile(previewFile, originalContent, "utf8"); const previewAction = retargetQuickAction(action, previewFile); - const applyArgs = withApplyFlag([...previewAction.args]); + const applyArgs = serializePatchloomArgs({ args: [...previewAction.args], apply: true }); await execFileAsync(binaryPath, applyArgs, { cwd: previewDir, timeout: 5000 }); @@ -529,7 +529,7 @@ describe("patchloom CLI integration", async () => { await fs.writeFile(file, "---\na: 1\n---\nb: 2\n", "utf8"); const action = buildDocMergeQuickAction(file, '{"c": 3}', "0"); - await execFileAsync(binaryPath, withApplyFlag(action.args), { timeout: 5000 }); + await execFileAsync(binaryPath, serializePatchloomArgs({ args: action.args, apply: true }), { timeout: 5000 }); const content = await fs.readFile(file, "utf8"); assert.match(content, /a:\s*1/, "first document field preserved"); @@ -556,7 +556,7 @@ describe("patchloom CLI integration", async () => { await fs.writeFile(file, "alpha\nbeta\n", "utf8"); const action = buildInsertAfterMatchQuickAction(file, "beta", "gamma"); - await execFileAsync(binaryPath, withApplyFlag(action.args), { timeout: 5000 }); + await execFileAsync(binaryPath, serializePatchloomArgs({ args: action.args, apply: true }), { timeout: 5000 }); const content = await fs.readFile(file, "utf8"); assert.equal(content, "alpha\nbeta\ngamma\n"); @@ -576,7 +576,7 @@ describe("patchloom CLI integration", async () => { await fs.writeFile(file, "fn foo() {\n let a = 1;\n}\n", "utf8"); const action = buildApplyFragmentQuickAction(file, "after", "fn foo() {", " let x = 2;"); - await execFileAsync(binaryPath, withApplyFlag(action.args), { timeout: 5000 }); + await execFileAsync(binaryPath, serializePatchloomArgs({ args: action.args, apply: true }), { timeout: 5000 }); const content = await fs.readFile(file, "utf8"); assert.equal(content, "fn foo() {\n let x = 2;\n let a = 1;\n}\n"); @@ -700,7 +700,11 @@ describe("patchloom CLI integration", async () => { ); const action = buildPatchApplyQuickAction(patchFile); - await execFileAsync(binaryPath, action.args, { cwd: dir, timeout: 5000 }); + await execFileAsync( + binaryPath, + serializePatchloomArgs({ args: action.args, apply: action.apply }), + { cwd: dir, timeout: 5000 } + ); const content = await fs.readFile(file, "utf8"); assert.match(content, /fn new\(\) \{\}/); @@ -723,9 +727,11 @@ describe("patchloom CLI integration", async () => { const action = buildCreateQuickAction(dest, "x"); try { - await execFileAsync(binaryPath, ["--json", ...action.args], { - timeout: 5000 - }); + await execFileAsync( + binaryPath, + ["--json", ...serializePatchloomArgs({ args: action.args, apply: action.apply })], + { timeout: 5000 } + ); assert.fail("create through a file parent should fail"); } catch (error) { const failed = error as { stdout?: string; stderr?: string }; @@ -754,7 +760,7 @@ describe("patchloom CLI integration", async () => { ].join("\n"), "utf8"); const action = buildDocSetQuickAction(file, "service_a.retries", "3"); - await execFileAsync(binaryPath, withApplyFlag(action.args), { timeout: 5000 }); + await execFileAsync(binaryPath, serializePatchloomArgs({ args: action.args, apply: true }), { timeout: 5000 }); const content = await fs.readFile(file, "utf8"); assert.match(content, /<<: \*shared/, "alias merge key should be preserved"); @@ -779,7 +785,7 @@ describe("patchloom CLI integration", async () => { ); const action = buildDocUpdateQuickAction(file, "items[*].enabled", "false"); - await execFileAsync(binaryPath, withApplyFlag(action.args), { timeout: 5000 }); + await execFileAsync(binaryPath, serializePatchloomArgs({ args: action.args, apply: true }), { timeout: 5000 }); const content = JSON.parse(await fs.readFile(file, "utf8")) as { items: Array<{ enabled: boolean }> }; assert.deepEqual(content.items, [{ enabled: false }, { enabled: false }]); @@ -803,7 +809,7 @@ describe("patchloom CLI integration", async () => { ); const action = buildDocDeleteWhereQuickAction(file, "items", "name=stale"); - await execFileAsync(binaryPath, withApplyFlag(action.args), { timeout: 5000 }); + await execFileAsync(binaryPath, serializePatchloomArgs({ args: action.args, apply: true }), { timeout: 5000 }); const content = JSON.parse(await fs.readFile(file, "utf8")) as { items: Array<{ name: string }> }; assert.deepEqual(content.items, [{ name: "keep" }]); diff --git a/test/unit/quickActions.test.ts b/test/unit/quickActions.test.ts index c689c15..74ce55a 100644 --- a/test/unit/quickActions.test.ts +++ b/test/unit/quickActions.test.ts @@ -45,8 +45,7 @@ import { presentUndoSuccess, resolveWorkspaceRelativePath, retargetQuickAction, - withApplyFlag, - withContainFlag + serializePatchloomArgs } from "../../src/commands/quickActions.js"; import type { PatchloomLog } from "../../src/logging/outputChannel.js"; @@ -248,60 +247,30 @@ test("retargetQuickAction swaps only the target path arguments", () => { ]); }); -test("withApplyFlag appends apply once", () => { - assert.deepEqual(withApplyFlag(["replace", "old", "--new", "new", "README.md"]), [ - "replace", - "old", - "--new", - "new", - "README.md", - "--apply" - ]); - assert.deepEqual(withApplyFlag(["replace", "old", "--new", "new", "README.md", "--apply"]), [ - "replace", - "old", - "--new", - "new", - "README.md", - "--apply" - ]); -}); - -test("withApplyFlag still appends when user text is --apply", () => { - assert.deepEqual(withApplyFlag(["replace", "--apply", "--new", "x", "f.txt"]), [ - "replace", "--apply", "--new", "x", "f.txt", "--apply" - ]); -}); - -test("withContainFlag prefixes global --contain once", () => { - assert.deepEqual(withContainFlag(["replace", "old", "--new", "new", "f.txt"]), [ - "--contain", - "replace", - "old", - "--new", - "new", - "f.txt" - ]); - assert.deepEqual(withContainFlag(["--contain", "batch", "--apply"]), [ - "--contain", - "batch", - "--apply" - ]); - assert.deepEqual(withContainFlag(["doc", "set", "a.json", "port", "1", "--contain"]), [ - "--contain", - "doc", - "set", - "a.json", - "port", - "1", - "--contain" - ]); +test("serializePatchloomArgs writes flags from booleans, not from scanning args", () => { + assert.deepEqual( + serializePatchloomArgs({ args: ["replace", "old", "--new", "new", "README.md"], apply: true }), + ["replace", "old", "--new", "new", "README.md", "--apply"] + ); + assert.deepEqual( + serializePatchloomArgs({ args: ["replace", "old", "--new", "new", "f.txt"], contain: true }), + ["--contain", "replace", "old", "--new", "new", "f.txt"] + ); + assert.deepEqual( + serializePatchloomArgs({ args: ["batch"], apply: true, contain: true }), + ["--contain", "batch", "--apply"] + ); }); -test("withContainFlag still prefixes when user text is --contain", () => { - assert.deepEqual(withContainFlag(["replace", "--contain", "--new", "x", "f.txt"]), [ - "--contain", "replace", "--contain", "--new", "x", "f.txt" - ]); +test("serializePatchloomArgs still adds flags when user text matches a flag name", () => { + assert.deepEqual( + serializePatchloomArgs({ args: ["replace", "--apply", "--new", "x", "f.txt"], apply: true }), + ["replace", "--apply", "--new", "x", "f.txt", "--apply"] + ); + assert.deepEqual( + serializePatchloomArgs({ args: ["replace", "--contain", "--new", "x", "f.txt"], contain: true }), + ["--contain", "replace", "--contain", "--new", "x", "f.txt"] + ); }); test("buildPrependQuickAction builds a file prepend command", () => { @@ -402,9 +371,9 @@ test("buildCreateQuickAction builds a create command with content and apply", () "create", "/workspace/demo/src/newfile.ts", "--content", - "hello", - "--apply" + "hello" ]); + assert.equal(action.apply, true); assert.deepEqual(action.targetArgIndices, [1]); }); @@ -414,9 +383,9 @@ test("buildCreateQuickAction allows empty content", () => { "create", "/workspace/demo/empty.txt", "--content", - "", - "--apply" + "" ]); + assert.equal(action.apply, true); }); test("buildSearchQuickAction preserves spaces in pattern as a single arg", () => { @@ -497,9 +466,9 @@ test("buildCreateQuickAction with spaces in path", () => { "create", "/workspace/my project/src/new file.ts", "--content", - "x", - "--apply" + "x" ]); + assert.equal(action.apply, true); }); test("buildDocGetQuickAction with deeply nested selector", () => { @@ -515,9 +484,9 @@ test("buildCreateQuickAction with unicode filename", () => { "create", "/workspace/demo/docs/日本語.md", "--content", - "# title", - "--apply" + "# title" ]); + assert.equal(action.apply, true); }); test("buildSearchQuickAction with unicode pattern", () => { @@ -651,7 +620,8 @@ test("buildUndoQuickAction builds an undo command", () => { assert.equal(action.title, "Undo last patchloom change"); assert.equal(action.targetPath, "/workspace/demo"); assert.deepEqual(action.targetArgIndices, []); - assert.deepEqual(action.args, ["undo", "--apply"]); + assert.deepEqual(action.args, ["undo"]); + assert.equal(action.apply, true); }); // --- #120: remaining Quick Actions --- @@ -769,7 +739,8 @@ test("buildPatchApplyQuickAction builds a patch apply command", () => { assert.equal(action.title, "Apply patch changes.patch"); assert.deepEqual(action.targetArgIndices, [2]); - assert.deepEqual(action.args, ["patch", "apply", "/workspace/demo/changes.patch", "--apply"]); + assert.deepEqual(action.args, ["patch", "apply", "/workspace/demo/changes.patch"]); + assert.equal(action.apply, true); }); test("retargetQuickAction works with patch apply command", () => { @@ -779,6 +750,7 @@ test("retargetQuickAction works with patch apply command", () => { assert.equal(retargeted.args[2], "/tmp/preview/fix.patch"); assert.equal(retargeted.args[0], "patch"); assert.equal(retargeted.args[1], "apply"); + assert.equal(retargeted.apply, true); }); // --- patch merge Quick Action (v0.2.0+) --- @@ -788,7 +760,8 @@ test("buildPatchMergeQuickAction builds a patch merge command", () => { assert.equal(action.title, "Merge patch changes.patch"); assert.deepEqual(action.targetArgIndices, [2]); - assert.deepEqual(action.args, ["patch", "merge", "/workspace/demo/changes.patch", "--apply"]); + assert.deepEqual(action.args, ["patch", "merge", "/workspace/demo/changes.patch"]); + assert.equal(action.apply, true); }); test("buildPatchMergeQuickAction includes allow-conflicts flag when enabled", () => { @@ -796,7 +769,8 @@ test("buildPatchMergeQuickAction includes allow-conflicts flag when enabled", () assert.equal(action.title, "Merge patch stale.diff"); assert.deepEqual(action.targetArgIndices, [2]); - assert.deepEqual(action.args, ["patch", "merge", "/workspace/demo/stale.diff", "--apply", "--allow-conflicts"]); + assert.deepEqual(action.args, ["patch", "merge", "/workspace/demo/stale.diff", "--allow-conflicts"]); + assert.equal(action.apply, true); }); test("retargetQuickAction works with patch merge command", () => {