Fix review custom instructions submission
This commit is contained in:
@@ -245,12 +245,14 @@ export default function contextFilter(omp: HookAPI): void {
|
||||
| `select(title, options)` | Show a selection dialog |
|
||||
| `confirm(title, message)` | Show a yes/no dialog |
|
||||
| `input(title, placeholder?)` | Show a text input dialog |
|
||||
| `editor(title, prefill?, { signal }?)` | Show a multi-line editor |
|
||||
| `editor(title, prefill?, { signal }?, { promptStyle }?)` | Show a multi-line editor |
|
||||
| `setEditorText(text)` | Set the input editor content |
|
||||
| `getEditorText()` | Get current input editor content |
|
||||
| `custom(factory)` | Render a custom TUI component |
|
||||
| `theme` | Current theme object |
|
||||
|
||||
Pass `{ promptStyle: true }` as the fourth argument when Enter should submit and Shift+Enter should insert a newline. The default hook editor behavior keeps Enter as newline and Ctrl+Enter as submit.
|
||||
|
||||
`ctx.hasUI` is `false` in headless/print/subagent mode — always guard interactive calls.
|
||||
|
||||
## Further reading
|
||||
|
||||
@@ -2,6 +2,12 @@
|
||||
|
||||
## [Unreleased]
|
||||
|
||||
### Fixed
|
||||
|
||||
- Fixed `/review` custom prompt orchestration text to use static prompt templates and consistently instruct reviewer task delegation.
|
||||
- Fixed `/review` custom-instructions submission on terminals that cannot distinguish Ctrl+Enter by using prompt-style input where Enter submits and Shift+Enter inserts a newline.
|
||||
- Fixed hook editor submissions sending large-paste placeholders such as `[paste #1 +27 lines]` instead of the pasted content.
|
||||
|
||||
## [15.3.2] - 2026-05-25
|
||||
### Added
|
||||
|
||||
@@ -8594,4 +8600,4 @@ Initial public release.
|
||||
- Git branch display in footer
|
||||
- Message queueing during streaming responses
|
||||
- OAuth integration for Gmail and Google Calendar access
|
||||
- HTML export with syntax highlighting and collapsible sections
|
||||
- HTML export with syntax highlighting and collapsible sections
|
||||
|
||||
@@ -14,6 +14,8 @@
|
||||
import { prompt } from "@oh-my-pi/pi-utils";
|
||||
import type { CustomCommand, CustomCommandAPI } from "../../../../extensibility/custom-commands/types";
|
||||
import type { HookCommandContext } from "../../../../extensibility/hooks/types";
|
||||
import reviewCustomRequestTemplate from "../../../../prompts/review-custom-request.md" with { type: "text" };
|
||||
import reviewHeadlessRequestTemplate from "../../../../prompts/review-headless-request.md" with { type: "text" };
|
||||
import reviewRequestTemplate from "../../../../prompts/review-request.md" with { type: "text" };
|
||||
import * as git from "../../../../utils/git";
|
||||
|
||||
@@ -225,6 +227,14 @@ function buildReviewPrompt(mode: string, stats: DiffStats, rawDiff: string, addi
|
||||
});
|
||||
}
|
||||
|
||||
function buildCustomReviewPrompt(instructions: string): string {
|
||||
return prompt.render(reviewCustomRequestTemplate, { instructions });
|
||||
}
|
||||
|
||||
function buildHeadlessReviewPrompt(focus?: string): string {
|
||||
return prompt.render(reviewHeadlessRequestTemplate, { focus });
|
||||
}
|
||||
|
||||
export class ReviewCommand implements CustomCommand {
|
||||
name = "review";
|
||||
description = "Launch interactive code review";
|
||||
@@ -233,8 +243,7 @@ export class ReviewCommand implements CustomCommand {
|
||||
|
||||
async execute(args: string[], ctx: HookCommandContext): Promise<string | undefined> {
|
||||
if (!ctx.hasUI) {
|
||||
const base = "Use the Task tool to run the 'reviewer' agent to review recent code changes.";
|
||||
return args.length > 0 ? `${base} Focus: ${args.join(" ")}` : base;
|
||||
return buildHeadlessReviewPrompt(args.length > 0 ? args.join(" ") : undefined);
|
||||
}
|
||||
|
||||
// Inline args act as additional instructions appended to the generated prompt.
|
||||
@@ -379,7 +388,12 @@ export class ReviewCommand implements CustomCommand {
|
||||
|
||||
case 4: {
|
||||
// Custom instructions - still uses the old approach since user provides context
|
||||
const instructions = await ctx.ui.editor("Enter custom review instructions", "Review the following:\n\n");
|
||||
const instructions = await ctx.ui.editor(
|
||||
"Enter custom review instructions",
|
||||
"Review the following:\n\n",
|
||||
undefined,
|
||||
{ promptStyle: true },
|
||||
);
|
||||
if (!instructions?.trim()) return undefined;
|
||||
|
||||
// For custom, we still try to get current diff for context
|
||||
@@ -402,17 +416,7 @@ export class ReviewCommand implements CustomCommand {
|
||||
);
|
||||
}
|
||||
|
||||
// No diff available, just pass instructions
|
||||
return `## Code Review Request
|
||||
|
||||
### Mode
|
||||
Custom review instructions
|
||||
|
||||
### Instructions
|
||||
|
||||
${instructions}
|
||||
|
||||
Use the Task tool with \`agent: "reviewer"\` to execute this review.`;
|
||||
return buildCustomReviewPrompt(instructions);
|
||||
}
|
||||
|
||||
default:
|
||||
|
||||
@@ -139,9 +139,16 @@ export interface HookUIContext {
|
||||
* Supports Ctrl+G to open external editor ($VISUAL or $EDITOR).
|
||||
* @param title - Title describing what is being edited
|
||||
* @param prefill - Optional initial text
|
||||
* @param options - Optional dialog controls such as an abort signal
|
||||
* @param editorOptions - Optional editor behavior; `promptStyle` makes Enter submit and Shift+Enter insert a newline
|
||||
* @returns Edited text, or undefined if cancelled (Escape)
|
||||
*/
|
||||
editor(title: string, prefill?: string, options?: { signal?: AbortSignal }): Promise<string | undefined>;
|
||||
editor(
|
||||
title: string,
|
||||
prefill?: string,
|
||||
options?: { signal?: AbortSignal },
|
||||
editorOptions?: { promptStyle?: boolean },
|
||||
): Promise<string | undefined>;
|
||||
|
||||
/**
|
||||
* Get the current theme for styling text with ANSI codes.
|
||||
|
||||
@@ -17,6 +17,10 @@ export interface HookEditorOptions {
|
||||
promptStyle?: boolean;
|
||||
}
|
||||
|
||||
function isCtrlEnterSubmit(keyData: string): boolean {
|
||||
return matchesKey(keyData, "ctrl+enter") || (keyData.charCodeAt(0) === 10 && keyData.length > 1);
|
||||
}
|
||||
|
||||
export class HookEditorComponent extends Container {
|
||||
#editor: Editor;
|
||||
#onSubmitCallback: (value: string) => void;
|
||||
@@ -78,6 +82,10 @@ export class HookEditorComponent extends Container {
|
||||
}
|
||||
}
|
||||
|
||||
#submitCurrentText(): void {
|
||||
this.#onSubmitCallback(this.#editor.getExpandedText());
|
||||
}
|
||||
|
||||
/** Prompt-style: raw Enter submits; Editor owns newline-producing sequences. */
|
||||
#handlePromptStyleInput(keyData: string): void {
|
||||
// Prompt-style keeps Escape as an explicit cancel key and also honors app.interrupt remaps.
|
||||
@@ -94,7 +102,7 @@ export class HookEditorComponent extends Container {
|
||||
|
||||
// Submit on any plain Enter encoding, including terminals that report unmodified Enter as LF.
|
||||
if (matchesKey(keyData, "enter") || matchesKey(keyData, "return")) {
|
||||
this.#onSubmitCallback(this.#editor.getText());
|
||||
this.#submitCurrentText();
|
||||
return;
|
||||
}
|
||||
|
||||
@@ -105,8 +113,8 @@ export class HookEditorComponent extends Container {
|
||||
/** Hook-style: Enter=newline, Ctrl+Enter=submit (original behavior) */
|
||||
#handleHookStyleInput(keyData: string): void {
|
||||
// Ctrl+Enter to submit. Use key matching so lock-key and keypad Enter variants work.
|
||||
if (matchesKey(keyData, "ctrl+enter")) {
|
||||
this.#onSubmitCallback(this.#editor.getText());
|
||||
if (isCtrlEnterSubmit(keyData)) {
|
||||
this.#submitCurrentText();
|
||||
return;
|
||||
}
|
||||
|
||||
|
||||
@@ -0,0 +1,22 @@
|
||||
## Code Review Request
|
||||
|
||||
### Mode
|
||||
|
||||
Custom review instructions
|
||||
|
||||
### Distribution Guidelines
|
||||
|
||||
Use the `task` tool with `agent: "reviewer"` and a `tasks` array.
|
||||
Create exactly **1 reviewer task**. Its assignment must include the custom instructions below.
|
||||
|
||||
### Reviewer Instructions
|
||||
|
||||
Reviewer MUST:
|
||||
1. Follow the custom instructions below
|
||||
2. Read the referenced files or workspace context needed to evaluate them
|
||||
3. Call `report_finding` per issue
|
||||
4. Call `yield` with verdict when done
|
||||
|
||||
### Custom Instructions
|
||||
|
||||
{{instructions}}
|
||||
@@ -0,0 +1,16 @@
|
||||
## Code Review Request
|
||||
|
||||
### Mode
|
||||
|
||||
Headless review request
|
||||
|
||||
### Distribution Guidelines
|
||||
|
||||
Use the `task` tool with `agent: "reviewer"` and a `tasks` array.
|
||||
Create exactly **1 reviewer task** for recent code changes.
|
||||
|
||||
{{#if focus}}
|
||||
### Focus
|
||||
|
||||
{{focus}}
|
||||
{{/if}}
|
||||
@@ -23,14 +23,13 @@ _No files to review._
|
||||
|
||||
### Distribution Guidelines
|
||||
|
||||
{{#when agentCount "==" 1}}Use **1 reviewer agent**.{{else}}Spawn **{{agentCount}} reviewer agents** in parallel.{{/when}}
|
||||
Use the `task` tool with `agent: "reviewer"` and a `tasks` array.
|
||||
{{#when agentCount "==" 1}}Create exactly **1 reviewer task**.{{else}}Spawn **{{agentCount}} reviewer agents** in parallel.{{/when}}
|
||||
{{#if multiAgent}}
|
||||
Group files by locality, e.g.:
|
||||
- Same directory/module → same agent
|
||||
- Related functionality → same agent
|
||||
- Tests with their implementation files → same agent
|
||||
|
||||
You MUST use Task tool with `agent: "reviewer"` and `tasks` array.
|
||||
{{/if}}
|
||||
|
||||
### Reviewer Instructions
|
||||
|
||||
@@ -0,0 +1,153 @@
|
||||
import { afterEach, describe, expect, it } from "bun:test";
|
||||
import * as fs from "node:fs/promises";
|
||||
import * as os from "node:os";
|
||||
import * as path from "node:path";
|
||||
import { $ } from "bun";
|
||||
import { ReviewCommand } from "../../../src/extensibility/custom-commands/bundled/review";
|
||||
import type { CustomCommandAPI } from "../../../src/extensibility/custom-commands/types";
|
||||
import type { HookCommandContext } from "../../../src/extensibility/hooks/types";
|
||||
|
||||
const LEGACY_TASK_INSTRUCTION = 'Use the Task tool with `agent: "reviewer"` to execute this review.';
|
||||
const REVIEWER_TASK_INSTRUCTION = 'Use the `task` tool with `agent: "reviewer"` and a `tasks` array.';
|
||||
|
||||
interface EditorCall {
|
||||
title: string;
|
||||
prefill: string | undefined;
|
||||
editorOptions: { promptStyle?: boolean } | undefined;
|
||||
}
|
||||
|
||||
describe("ReviewCommand", () => {
|
||||
let tmpDir: string | undefined;
|
||||
|
||||
afterEach(async () => {
|
||||
if (tmpDir) {
|
||||
await fs.rm(tmpDir, { recursive: true, force: true });
|
||||
tmpDir = undefined;
|
||||
}
|
||||
});
|
||||
|
||||
async function createTempDir(): Promise<string> {
|
||||
tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), "omp-review-command-"));
|
||||
return tmpDir;
|
||||
}
|
||||
|
||||
async function createGitRepoWithUncommittedChange(): Promise<string> {
|
||||
const dir = await createTempDir();
|
||||
await $`git init`.cwd(dir).quiet();
|
||||
await $`git config user.name Omp Test`.cwd(dir).quiet();
|
||||
await $`git config user.email omp-test@example.com`.cwd(dir).quiet();
|
||||
await Bun.write(path.join(dir, "review-target.ts"), "export const value = 1;\n");
|
||||
await $`git add review-target.ts`.cwd(dir).quiet();
|
||||
await $`git commit -m initial`.cwd(dir).quiet();
|
||||
await Bun.write(path.join(dir, "review-target.ts"), "export const value = 2;\n");
|
||||
return dir;
|
||||
}
|
||||
|
||||
function createContext(options?: {
|
||||
selectedMode?: string;
|
||||
editorValue?: string | undefined;
|
||||
onEditorCall?: (call: EditorCall) => void;
|
||||
}): HookCommandContext {
|
||||
return {
|
||||
hasUI: true,
|
||||
ui: {
|
||||
select: () => Promise.resolve(options?.selectedMode ?? "4. Custom review instructions"),
|
||||
editor: (
|
||||
title: string,
|
||||
prefill?: string,
|
||||
_options?: { signal?: AbortSignal },
|
||||
editorOptions?: { promptStyle?: boolean },
|
||||
) => {
|
||||
options?.onEditorCall?.({ title, prefill, editorOptions });
|
||||
return Promise.resolve(options?.editorValue);
|
||||
},
|
||||
notify: () => {},
|
||||
},
|
||||
} as unknown as HookCommandContext;
|
||||
}
|
||||
|
||||
it("uses prompt-style input for custom review instructions", async () => {
|
||||
const dir = await createTempDir();
|
||||
let editorCall: EditorCall | undefined;
|
||||
|
||||
const command = new ReviewCommand({ cwd: dir } as unknown as CustomCommandAPI);
|
||||
const ctx = createContext({
|
||||
editorValue: "Check authentication boundaries",
|
||||
onEditorCall: call => {
|
||||
editorCall = call;
|
||||
},
|
||||
});
|
||||
|
||||
const result = await command.execute([], ctx);
|
||||
|
||||
expect(editorCall).toEqual({
|
||||
title: "Enter custom review instructions",
|
||||
prefill: "Review the following:\n\n",
|
||||
editorOptions: { promptStyle: true },
|
||||
});
|
||||
expect(result).toContain("Check authentication boundaries");
|
||||
});
|
||||
|
||||
it("renders custom review instructions through the reviewer task prompt when no diff is available", async () => {
|
||||
const dir = await createTempDir();
|
||||
const command = new ReviewCommand({ cwd: dir } as unknown as CustomCommandAPI);
|
||||
const ctx = createContext({
|
||||
editorValue: "Check authentication boundaries",
|
||||
});
|
||||
|
||||
const result = await command.execute([], ctx);
|
||||
|
||||
expect(result).toBeDefined();
|
||||
const promptText = result!;
|
||||
expect(promptText).toContain("Custom review instructions");
|
||||
expect(promptText).toContain(REVIEWER_TASK_INSTRUCTION);
|
||||
expect(promptText).toContain("Check authentication boundaries");
|
||||
expect(promptText).not.toContain(LEGACY_TASK_INSTRUCTION);
|
||||
});
|
||||
|
||||
it("does not submit empty custom review instructions", async () => {
|
||||
const values = [undefined, "", " \n\t "];
|
||||
|
||||
for (const editorValue of values) {
|
||||
const dir = await createTempDir();
|
||||
const command = new ReviewCommand({ cwd: dir } as unknown as CustomCommandAPI);
|
||||
const ctx = createContext({ editorValue });
|
||||
|
||||
const result = await command.execute([], ctx);
|
||||
|
||||
expect(result).toBeUndefined();
|
||||
await fs.rm(dir, { recursive: true, force: true });
|
||||
tmpDir = undefined;
|
||||
}
|
||||
});
|
||||
|
||||
it("includes reviewer task orchestration for single-agent diff reviews", async () => {
|
||||
const dir = await createGitRepoWithUncommittedChange();
|
||||
const command = new ReviewCommand({ cwd: dir } as unknown as CustomCommandAPI);
|
||||
const ctx = createContext({
|
||||
selectedMode: "2. Review uncommitted changes",
|
||||
});
|
||||
|
||||
const result = await command.execute([], ctx);
|
||||
|
||||
expect(result).toBeDefined();
|
||||
const promptText = result!;
|
||||
expect(promptText).toContain(REVIEWER_TASK_INSTRUCTION);
|
||||
expect(promptText).toContain("Create exactly **1 reviewer task**");
|
||||
expect(promptText).not.toContain(LEGACY_TASK_INSTRUCTION);
|
||||
});
|
||||
|
||||
it("renders headless review requests through the reviewer task prompt", async () => {
|
||||
const command = new ReviewCommand({ cwd: "/tmp" } as unknown as CustomCommandAPI);
|
||||
const ctx = { hasUI: false } as unknown as HookCommandContext;
|
||||
|
||||
const result = await command.execute(["focus", "auth"], ctx);
|
||||
|
||||
expect(result).toBeDefined();
|
||||
const promptText = result!;
|
||||
expect(promptText).toContain("Headless review request");
|
||||
expect(promptText).toContain(REVIEWER_TASK_INSTRUCTION);
|
||||
expect(promptText).toContain("focus auth");
|
||||
expect(promptText).not.toContain(LEGACY_TASK_INSTRUCTION);
|
||||
});
|
||||
});
|
||||
@@ -37,6 +37,10 @@ function renderLines(component: HookEditorComponent, width = 120): string[] {
|
||||
return Bun.stripANSI(component.render(width).join("\n")).split("\n");
|
||||
}
|
||||
|
||||
function largePasteText(): string {
|
||||
return Array.from({ length: 11 }, (_, index) => `pasted line ${index + 1}`).join("\n");
|
||||
}
|
||||
|
||||
type TestContext = InteractiveModeContext & {
|
||||
editorContainer: {
|
||||
children: unknown[];
|
||||
@@ -126,6 +130,35 @@ describe("HookEditorComponent default (hook) mode", () => {
|
||||
}
|
||||
});
|
||||
|
||||
it("submits LF-prefixed modified Enter sequences", () => {
|
||||
const onSubmit = vi.fn();
|
||||
const onCancel = vi.fn();
|
||||
const component = new HookEditorComponent(createTui(), "Prompt", "draft", onSubmit, onCancel);
|
||||
|
||||
component.handleInput("\n\x1b[13;5u");
|
||||
|
||||
expect(onSubmit).toHaveBeenCalledTimes(1);
|
||||
expect(onSubmit).toHaveBeenCalledWith("draft");
|
||||
expect(onCancel).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("expands large paste markers when submitting on Ctrl+Enter", () => {
|
||||
const onSubmit = vi.fn();
|
||||
const onCancel = vi.fn();
|
||||
const component = new HookEditorComponent(createTui(), "Prompt", undefined, onSubmit, onCancel);
|
||||
const pasted = largePasteText();
|
||||
|
||||
component.handleInput(`\x1b[200~${pasted}\x1b[201~`);
|
||||
|
||||
expect(renderText(component)).toContain("[paste #1 +11 lines]");
|
||||
|
||||
component.handleInput("\x1b[13;5u");
|
||||
|
||||
expect(onSubmit).toHaveBeenCalledTimes(1);
|
||||
expect(onSubmit).toHaveBeenCalledWith(pasted);
|
||||
expect(onCancel).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("cancels on Escape", () => {
|
||||
const onSubmit = vi.fn();
|
||||
const onCancel = vi.fn();
|
||||
@@ -185,6 +218,25 @@ describe("HookEditorComponent prompt-style mode", () => {
|
||||
expect(onCancel).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("expands large paste markers when submitting on Enter", () => {
|
||||
const onSubmit = vi.fn();
|
||||
const onCancel = vi.fn();
|
||||
const component = new HookEditorComponent(createTui(), "Prompt", undefined, onSubmit, onCancel, {
|
||||
promptStyle: true,
|
||||
});
|
||||
const pasted = largePasteText();
|
||||
|
||||
component.handleInput(`\x1b[200~${pasted}\x1b[201~`);
|
||||
|
||||
expect(renderText(component)).toContain("[paste #1 +11 lines]");
|
||||
|
||||
component.handleInput("\r");
|
||||
|
||||
expect(onSubmit).toHaveBeenCalledTimes(1);
|
||||
expect(onSubmit).toHaveBeenCalledWith(pasted);
|
||||
expect(onCancel).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("inserts newline on Shift+Enter instead of submitting", () => {
|
||||
const onSubmit = vi.fn();
|
||||
const onCancel = vi.fn();
|
||||
|
||||
Reference in New Issue
Block a user