Address review feedback: guard blank /btw /tan /omfg and add history regression tests

- Only add /btw, /tan, /omfg to history when their argument is non-empty, so blank invocations rejected by the controller are not persisted.

- Add regression coverage for addToHistory behavior on /btw, /tan, /omfg, /memory, /rename, and /move.
This commit is contained in:
KamijoToma
2026-06-18 22:59:59 +08:00
parent 0dd4112f0d
commit e50ca8d21a
7 changed files with 196 additions and 6 deletions
@@ -1328,7 +1328,9 @@ const BUILTIN_SLASH_COMMAND_REGISTRY: ReadonlyArray<SlashCommandSpec> = [
allowArgs: true,
handleTui: async (command, runtime) => {
const question = command.text.slice(`/${command.name}`.length).trim();
runtime.ctx.editor.addToHistory(command.text);
if (question) {
runtime.ctx.editor.addToHistory(command.text);
}
runtime.ctx.editor.setText("");
await runtime.ctx.handleBtwCommand(question);
},
@@ -1340,7 +1342,9 @@ const BUILTIN_SLASH_COMMAND_REGISTRY: ReadonlyArray<SlashCommandSpec> = [
allowArgs: true,
handleTui: async (command, runtime) => {
const work = command.text.slice(`/${command.name}`.length).trim();
runtime.ctx.editor.addToHistory(command.text);
if (work) {
runtime.ctx.editor.addToHistory(command.text);
}
runtime.ctx.editor.setText("");
await runtime.ctx.handleTanCommand(work);
},
@@ -1352,7 +1356,9 @@ const BUILTIN_SLASH_COMMAND_REGISTRY: ReadonlyArray<SlashCommandSpec> = [
allowArgs: true,
handleTui: async (command, runtime) => {
const complaint = command.text.slice(`/${command.name}`.length).trim();
runtime.ctx.editor.addToHistory(command.text);
if (complaint) {
runtime.ctx.editor.addToHistory(command.text);
}
runtime.ctx.editor.setText("");
await runtime.ctx.handleOmfgCommand(complaint);
},
@@ -5,12 +5,14 @@ import { executeBuiltinSlashCommand } from "@oh-my-pi/pi-coding-agent/slash-comm
function createRuntime() {
const handleBtwCommand = vi.fn(async () => {});
const setText = vi.fn();
const addToHistory = vi.fn();
return {
handleBtwCommand,
setText,
addToHistory,
runtime: {
ctx: {
editor: { setText } as unknown as InteractiveModeContext["editor"],
editor: { setText, addToHistory } as unknown as InteractiveModeContext["editor"],
handleBtwCommand,
} as unknown as InteractiveModeContext,
},
@@ -24,6 +26,7 @@ describe("/btw slash command", () => {
const handled = await executeBuiltinSlashCommand("/btw why is it doing that?", harness.runtime);
expect(handled).toBe(true);
expect(harness.addToHistory).toHaveBeenCalledWith("/btw why is it doing that?");
expect(harness.setText).toHaveBeenCalledWith("");
expect(harness.handleBtwCommand).toHaveBeenCalledWith("why is it doing that?");
});
@@ -37,6 +40,18 @@ describe("/btw slash command", () => {
);
expect(handled).toBe(true);
expect(harness.addToHistory).toHaveBeenCalledWith("/btw explain why the cache reuse matters here");
expect(harness.handleBtwCommand).toHaveBeenCalledWith("explain why the cache reuse matters here");
});
it("does not add a blank /btw invocation to history", async () => {
const harness = createRuntime();
const handled = await executeBuiltinSlashCommand("/btw ", harness.runtime);
expect(handled).toBe(true);
expect(harness.addToHistory).not.toHaveBeenCalled();
expect(harness.setText).toHaveBeenCalledWith("");
expect(harness.handleBtwCommand).toHaveBeenCalledWith("");
});
});
@@ -0,0 +1,43 @@
import { describe, expect, it, vi } from "bun:test";
import type { InteractiveModeContext } from "@oh-my-pi/pi-coding-agent/modes/types";
import { executeBuiltinSlashCommand } from "@oh-my-pi/pi-coding-agent/slash-commands/builtin-registry";
function createRuntime() {
const handleMemoryCommand = vi.fn(async () => {});
const setText = vi.fn();
const addToHistory = vi.fn();
return {
handleMemoryCommand,
setText,
addToHistory,
runtime: {
ctx: {
editor: { setText, addToHistory } as unknown as InteractiveModeContext["editor"],
handleMemoryCommand,
} as unknown as InteractiveModeContext,
},
};
}
describe("/memory slash command", () => {
it("routes the full command text through the memory handler and saves it to history", async () => {
const harness = createRuntime();
const handled = await executeBuiltinSlashCommand("/memory view", harness.runtime);
expect(handled).toBe(true);
expect(harness.addToHistory).toHaveBeenCalledWith("/memory view");
expect(harness.setText).toHaveBeenCalledWith("");
expect(harness.handleMemoryCommand).toHaveBeenCalledWith("/memory view");
});
it("preserves the raw command text for history", async () => {
const harness = createRuntime();
const handled = await executeBuiltinSlashCommand("/memory stats", harness.runtime);
expect(handled).toBe(true);
expect(harness.addToHistory).toHaveBeenCalledWith("/memory stats");
expect(harness.handleMemoryCommand).toHaveBeenCalledWith("/memory stats");
});
});
@@ -0,0 +1,48 @@
import { describe, expect, it, vi } from "bun:test";
import type { InteractiveModeContext } from "@oh-my-pi/pi-coding-agent/modes/types";
import { executeBuiltinSlashCommand } from "@oh-my-pi/pi-coding-agent/slash-commands/builtin-registry";
function createRuntime() {
const handleMoveCommand = vi.fn(async () => {});
const showError = vi.fn();
const setText = vi.fn();
const addToHistory = vi.fn();
return {
handleMoveCommand,
showError,
setText,
addToHistory,
runtime: {
ctx: {
editor: { setText, addToHistory } as unknown as InteractiveModeContext["editor"],
showError,
handleMoveCommand,
} as unknown as InteractiveModeContext,
},
};
}
describe("/move slash command", () => {
it("routes the path through the move handler and saves the full command to history", async () => {
const harness = createRuntime();
const handled = await executeBuiltinSlashCommand("/move /tmp/project", harness.runtime);
expect(handled).toBe(true);
expect(harness.addToHistory).toHaveBeenCalledWith("/move /tmp/project");
expect(harness.setText).toHaveBeenCalledWith("");
expect(harness.handleMoveCommand).toHaveBeenCalledWith("/tmp/project");
});
it("does not add a blank /move invocation to history", async () => {
const harness = createRuntime();
const handled = await executeBuiltinSlashCommand("/move ", harness.runtime);
expect(handled).toBe(true);
expect(harness.addToHistory).not.toHaveBeenCalled();
expect(harness.showError).toHaveBeenCalledWith("Usage: /move <path>");
expect(harness.setText).toHaveBeenCalledWith("");
expect(harness.handleMoveCommand).not.toHaveBeenCalled();
});
});
@@ -5,12 +5,14 @@ import { executeBuiltinSlashCommand } from "@oh-my-pi/pi-coding-agent/slash-comm
function createRuntime() {
const handleOmfgCommand = vi.fn(async () => {});
const setText = vi.fn();
const addToHistory = vi.fn();
return {
handleOmfgCommand,
setText,
addToHistory,
runtime: {
ctx: {
editor: { setText } as unknown as InteractiveModeContext["editor"],
editor: { setText, addToHistory } as unknown as InteractiveModeContext["editor"],
handleOmfgCommand,
} as unknown as InteractiveModeContext,
},
@@ -24,6 +26,7 @@ describe("/omfg slash command", () => {
const handled = await executeBuiltinSlashCommand("/omfg This guy used any again....", harness.runtime);
expect(handled).toBe(true);
expect(harness.addToHistory).toHaveBeenCalledWith("/omfg This guy used any again....");
expect(harness.setText).toHaveBeenCalledWith("");
expect(harness.handleOmfgCommand).toHaveBeenCalledWith("This guy used any again....");
});
@@ -37,6 +40,18 @@ describe("/omfg slash command", () => {
);
expect(handled).toBe(true);
expect(harness.addToHistory).toHaveBeenCalledWith("/omfg stop making unchecked casts in generated TypeScript");
expect(harness.handleOmfgCommand).toHaveBeenCalledWith("stop making unchecked casts in generated TypeScript");
});
it("does not add a blank /omfg invocation to history", async () => {
const harness = createRuntime();
const handled = await executeBuiltinSlashCommand("/omfg ", harness.runtime);
expect(handled).toBe(true);
expect(harness.addToHistory).not.toHaveBeenCalled();
expect(harness.setText).toHaveBeenCalledWith("");
expect(harness.handleOmfgCommand).toHaveBeenCalledWith("");
});
});
@@ -0,0 +1,48 @@
import { describe, expect, it, vi } from "bun:test";
import type { InteractiveModeContext } from "@oh-my-pi/pi-coding-agent/modes/types";
import { executeBuiltinSlashCommand } from "@oh-my-pi/pi-coding-agent/slash-commands/builtin-registry";
function createRuntime() {
const handleRenameCommand = vi.fn(async () => {});
const showError = vi.fn();
const setText = vi.fn();
const addToHistory = vi.fn();
return {
handleRenameCommand,
showError,
setText,
addToHistory,
runtime: {
ctx: {
editor: { setText, addToHistory } as unknown as InteractiveModeContext["editor"],
showError,
handleRenameCommand,
} as unknown as InteractiveModeContext,
},
};
}
describe("/rename slash command", () => {
it("routes the title through the rename handler and saves the full command to history", async () => {
const harness = createRuntime();
const handled = await executeBuiltinSlashCommand("/rename my session", harness.runtime);
expect(handled).toBe(true);
expect(harness.addToHistory).toHaveBeenCalledWith("/rename my session");
expect(harness.setText).toHaveBeenCalledWith("");
expect(harness.handleRenameCommand).toHaveBeenCalledWith("my session");
});
it("does not add a blank /rename invocation to history", async () => {
const harness = createRuntime();
const handled = await executeBuiltinSlashCommand("/rename ", harness.runtime);
expect(handled).toBe(true);
expect(harness.addToHistory).not.toHaveBeenCalled();
expect(harness.showError).toHaveBeenCalledWith("Usage: /rename <title>");
expect(harness.setText).toHaveBeenCalledWith("");
expect(harness.handleRenameCommand).not.toHaveBeenCalled();
});
});
@@ -5,12 +5,14 @@ import { executeBuiltinSlashCommand } from "@oh-my-pi/pi-coding-agent/slash-comm
function createRuntime() {
const handleTanCommand = vi.fn(async () => {});
const setText = vi.fn();
const addToHistory = vi.fn();
return {
handleTanCommand,
setText,
addToHistory,
runtime: {
ctx: {
editor: { setText } as unknown as InteractiveModeContext["editor"],
editor: { setText, addToHistory } as unknown as InteractiveModeContext["editor"],
handleTanCommand,
} as unknown as InteractiveModeContext,
},
@@ -24,6 +26,7 @@ describe("/tan slash command", () => {
const handled = await executeBuiltinSlashCommand("/tan add a changelog note", harness.runtime);
expect(handled).toBe(true);
expect(harness.addToHistory).toHaveBeenCalledWith("/tan add a changelog note");
expect(harness.setText).toHaveBeenCalledWith("");
expect(harness.handleTanCommand).toHaveBeenCalledWith("add a changelog note");
});
@@ -37,6 +40,18 @@ describe("/tan slash command", () => {
);
expect(handled).toBe(true);
expect(harness.addToHistory).toHaveBeenCalledWith("/tan investigate why prompt cache reuse matters here");
expect(harness.handleTanCommand).toHaveBeenCalledWith("investigate why prompt cache reuse matters here");
});
it("does not add a blank /tan invocation to history", async () => {
const harness = createRuntime();
const handled = await executeBuiltinSlashCommand("/tan ", harness.runtime);
expect(handled).toBe(true);
expect(harness.addToHistory).not.toHaveBeenCalled();
expect(harness.setText).toHaveBeenCalledWith("");
expect(harness.handleTanCommand).toHaveBeenCalledWith("");
});
});