feat: added interactive /review command with multi-mode selection
- Added interactive /review command with mode selection for base branch, uncommitted changes, specific commit, or custom instructions. - Added subprocess tool registry for extracting and rendering tool data from subprocess agents. - Added combined review result rendering with verdict and findings tree structure. - Changed bundled commands to be overridable by user/project commands with same name. - Removed findings_count parameter from submit_review tool in favor of automatic counting. - Added fix npm script for auto-fixable lint issues with unsafe fixes.
This commit is contained in:
can1357
@@ -272,10 +272,12 @@ See `docs/bun-migration-guide.md` for full migration reference.
|
||||
|
||||
## Commands
|
||||
|
||||
- After code changes: `npm run check` (get full output, no tail)
|
||||
- NEVER run: `npm run dev`, `npm run build`, `npm test`
|
||||
- Only run specific tests if user instructs: `npm test -- test/specific.test.ts`
|
||||
- After code changes: `bun run check` (get full output, no tail)
|
||||
- For auto-fixable lint issues: `bun run fix` (includes unsafe fixes)
|
||||
- NEVER run: `bun run dev`, `bun run build`, `bun test`
|
||||
- Only run specific tests if user instructs: `bun test test/specific.test.ts`
|
||||
- NEVER commit unless user asks
|
||||
- Do NOT use `tsc` or `npx tsc` - always use `bun run check` which runs the correct type checker
|
||||
|
||||
## GitHub Issues
|
||||
|
||||
|
||||
@@ -11,6 +11,7 @@
|
||||
"build": "bun --cwd=packages/web-ui run build:css",
|
||||
"dev": "bun --cwd=packages/web-ui run dev",
|
||||
"check": "biome check --write . && bun --cwd=packages/coding-agent run check && bun --cwd=packages/web-ui run check",
|
||||
"fix": "biome check --write --unsafe . && bun --cwd=packages/web-ui run fix",
|
||||
"test": "bun run --filter '*' test",
|
||||
"version:patch": "npm version patch -ws --no-git-tag-version && bun scripts/sync-versions.js && rm -rf node_modules packages/*/node_modules bun.lockb && bun install",
|
||||
"version:minor": "npm version minor -ws --no-git-tag-version && bun scripts/sync-versions.js && rm -rf node_modules packages/*/node_modules bun.lockb && bun install",
|
||||
|
||||
@@ -1,9 +1,10 @@
|
||||
# Changelog
|
||||
|
||||
## [Unreleased]
|
||||
|
||||
### Added
|
||||
|
||||
- Added subprocess tool registry for extracting and rendering tool data from subprocess agents in real-time
|
||||
- Added combined review result rendering showing verdict and findings in a tree structure
|
||||
- Auto-read file mentions: Reference files with `@path/to/file.ext` syntax in prompts to automatically inject their contents, eliminating manual Read tool calls
|
||||
- Added `hidden` property for custom tools to exclude them from default tool list unless explicitly requested
|
||||
- Added `explicitTools` option to `createAgentSession` for enabling hidden tools by name
|
||||
@@ -16,9 +17,18 @@
|
||||
|
||||
### Changed
|
||||
|
||||
- Changed `/review` command from markdown to interactive TypeScript with mode selection menu (branch comparison, uncommitted changes, commit review, custom)
|
||||
- Changed bundled commands to be overridable by user/project commands with same name
|
||||
- Changed subprocess termination to wait for message_end event to capture accurate token counts
|
||||
- Changed token counting in subprocess to accumulate across messages instead of overwriting
|
||||
- Updated bundled `reviewer` agent to use structured review tools with priority-based findings (P0-P3) and formal verdict submission
|
||||
- Task tool now streams artifacts in real-time: input written before spawn, session jsonl written by subprocess, output written at completion
|
||||
|
||||
### Removed
|
||||
|
||||
- Removed `findings_count` parameter from `submit_review` tool - findings are now counted automatically
|
||||
- Removed artifacts location display from task tool output
|
||||
|
||||
### Fixed
|
||||
|
||||
- Fixed Task tool output artifacts (`out.md`) containing duplicated text from streaming updates
|
||||
|
||||
@@ -33,7 +33,7 @@
|
||||
"clean": "rm -rf dist",
|
||||
"build": "tsgo -p tsconfig.build.json && chmod +x dist/cli.js && npm run copy-assets",
|
||||
"build:binary": "npm run build && bun build --compile ./dist/cli.js --outfile dist/pi && npm run copy-binary-assets",
|
||||
"copy-assets": "mkdir -p dist/modes/interactive/theme && cp src/modes/interactive/theme/*.json dist/modes/interactive/theme/ && mkdir -p dist/core/export-html && cp src/core/export-html/template.html src/core/export-html/template.css src/core/export-html/template.js dist/core/export-html/",
|
||||
"copy-assets": "mkdir -p dist/modes/interactive/theme && cp src/modes/interactive/theme/*.json dist/modes/interactive/theme/ && mkdir -p dist/core/export-html && cp src/core/export-html/template.html src/core/export-html/template.css src/core/export-html/template.js dist/core/export-html/ && mkdir -p dist/core/tools/task/bundled-agents && cp src/core/tools/task/bundled-agents/*.md dist/core/tools/task/bundled-agents/",
|
||||
"copy-binary-assets": "cp package.json dist/ && cp README.md dist/ && cp CHANGELOG.md dist/ && mkdir -p dist/theme && cp src/modes/interactive/theme/*.json dist/theme/ && mkdir -p dist/export-html && cp src/core/export-html/template.html src/core/export-html/template.css src/core/export-html/template.js dist/export-html/ && cp -r docs dist/ && cp -r examples dist/",
|
||||
"test": "vitest --run",
|
||||
"prepublishOnly": "npm run clean && npm run build"
|
||||
|
||||
@@ -1,12 +0,0 @@
|
||||
---
|
||||
name: review
|
||||
description: Launch code review with the reviewer agent
|
||||
---
|
||||
|
||||
Use the Task tool to run the "reviewer" agent with this task:
|
||||
|
||||
$@
|
||||
|
||||
The reviewer agent will analyze the code changes and report findings using the report_finding and submit_review tools.
|
||||
|
||||
If no specific instructions are provided, review the most recent changes (uncommitted or last commit).
|
||||
@@ -0,0 +1,156 @@
|
||||
/**
|
||||
* /review command - Interactive code review launcher
|
||||
*
|
||||
* Provides a menu to select review mode:
|
||||
* 1. Review against a base branch (PR style)
|
||||
* 2. Review uncommitted changes
|
||||
* 3. Review a specific commit
|
||||
* 4. Custom review instructions
|
||||
*/
|
||||
|
||||
import type { HookCommandContext } from "../../../hooks/types";
|
||||
import type { CustomCommand, CustomCommandAPI } from "../../types";
|
||||
|
||||
export function createReviewCommand(api: CustomCommandAPI): CustomCommand {
|
||||
return {
|
||||
name: "review",
|
||||
description: "Launch interactive code review",
|
||||
|
||||
async execute(_args: string[], ctx: HookCommandContext): Promise<string | undefined> {
|
||||
if (!ctx.hasUI) {
|
||||
return "Use the Task tool to run the 'reviewer' agent to review recent code changes.";
|
||||
}
|
||||
|
||||
// Main menu
|
||||
const mode = await ctx.ui.select("Review Mode", [
|
||||
"1. Review against a base branch (PR Style)",
|
||||
"2. Review uncommitted changes",
|
||||
"3. Review a specific commit",
|
||||
"4. Custom review instructions",
|
||||
]);
|
||||
|
||||
if (!mode) return undefined;
|
||||
|
||||
const modeNum = parseInt(mode[0], 10);
|
||||
|
||||
switch (modeNum) {
|
||||
case 1: {
|
||||
// PR-style review against base branch
|
||||
const branches = await getGitBranches(api);
|
||||
if (branches.length === 0) {
|
||||
ctx.ui.notify("No git branches found", "error");
|
||||
return undefined;
|
||||
}
|
||||
|
||||
const baseBranch = await ctx.ui.select("Select base branch to compare against", branches);
|
||||
if (!baseBranch) return undefined;
|
||||
|
||||
const currentBranch = await getCurrentBranch(api);
|
||||
return `Use the Task tool to run the "reviewer" agent with this task:
|
||||
|
||||
Review the changes between "${baseBranch}" and "${currentBranch}".
|
||||
|
||||
Run \`git diff ${baseBranch}...${currentBranch}\` to see the changes, then analyze the modified files.`;
|
||||
}
|
||||
|
||||
case 2: {
|
||||
// Uncommitted changes
|
||||
const status = await getGitStatus(api);
|
||||
if (!status.trim()) {
|
||||
ctx.ui.notify("No uncommitted changes found", "warning");
|
||||
return undefined;
|
||||
}
|
||||
|
||||
return `Use the Task tool to run the "reviewer" agent with this task:
|
||||
|
||||
Review all uncommitted changes in the working directory.
|
||||
|
||||
Run \`git diff\` for unstaged changes and \`git diff --cached\` for staged changes.`;
|
||||
}
|
||||
|
||||
case 3: {
|
||||
// Specific commit
|
||||
const commits = await getRecentCommits(api, 20);
|
||||
if (commits.length === 0) {
|
||||
ctx.ui.notify("No commits found", "error");
|
||||
return undefined;
|
||||
}
|
||||
|
||||
const selected = await ctx.ui.select("Select commit to review", commits);
|
||||
if (!selected) return undefined;
|
||||
|
||||
// Extract commit hash from selection (format: "abc1234 message")
|
||||
const hash = selected.split(" ")[0];
|
||||
|
||||
return `Use the Task tool to run the "reviewer" agent with this task:
|
||||
|
||||
Review commit ${hash}.
|
||||
|
||||
Run \`git show ${hash}\` to see the changes introduced by this commit.`;
|
||||
}
|
||||
|
||||
case 4: {
|
||||
// Custom instructions
|
||||
const instructions = await ctx.ui.editor(
|
||||
"Enter custom review instructions",
|
||||
"Review the following:\n\n",
|
||||
);
|
||||
if (!instructions?.trim()) return undefined;
|
||||
|
||||
return `Use the Task tool to run the "reviewer" agent with this task:
|
||||
|
||||
${instructions}`;
|
||||
}
|
||||
|
||||
default:
|
||||
return undefined;
|
||||
}
|
||||
},
|
||||
};
|
||||
}
|
||||
|
||||
async function getGitBranches(api: CustomCommandAPI): Promise<string[]> {
|
||||
try {
|
||||
const result = await api.exec("git", ["branch", "-a", "--format=%(refname:short)"]);
|
||||
if (result.code !== 0) return [];
|
||||
return result.stdout
|
||||
.split("\n")
|
||||
.map((b) => b.trim())
|
||||
.filter(Boolean);
|
||||
} catch {
|
||||
return [];
|
||||
}
|
||||
}
|
||||
|
||||
async function getCurrentBranch(api: CustomCommandAPI): Promise<string> {
|
||||
try {
|
||||
const result = await api.exec("git", ["branch", "--show-current"]);
|
||||
return result.stdout.trim() || "HEAD";
|
||||
} catch {
|
||||
return "HEAD";
|
||||
}
|
||||
}
|
||||
|
||||
async function getGitStatus(api: CustomCommandAPI): Promise<string> {
|
||||
try {
|
||||
const result = await api.exec("git", ["status", "--porcelain"]);
|
||||
return result.stdout;
|
||||
} catch {
|
||||
return "";
|
||||
}
|
||||
}
|
||||
|
||||
async function getRecentCommits(api: CustomCommandAPI, count: number): Promise<string[]> {
|
||||
try {
|
||||
const result = await api.exec("git", ["log", `-${count}`, "--oneline", "--no-decorate"]);
|
||||
if (result.code !== 0) return [];
|
||||
return result.stdout
|
||||
.split("\n")
|
||||
.map((c) => c.trim())
|
||||
.filter(Boolean);
|
||||
} catch {
|
||||
return [];
|
||||
}
|
||||
}
|
||||
|
||||
export default createReviewCommand;
|
||||
@@ -11,6 +11,7 @@ import * as typebox from "@sinclair/typebox";
|
||||
import { CONFIG_DIR_NAME, getAgentDir } from "../../config";
|
||||
import * as piCodingAgent from "../../index";
|
||||
import { execCommand } from "../exec";
|
||||
import { createReviewCommand } from "./bundled/review";
|
||||
import type {
|
||||
CustomCommand,
|
||||
CustomCommandAPI,
|
||||
@@ -141,6 +142,24 @@ export interface LoadCustomCommandsOptions {
|
||||
agentDir?: string;
|
||||
}
|
||||
|
||||
/**
|
||||
* Load bundled commands (shipped with pi-coding-agent).
|
||||
*/
|
||||
function loadBundledCommands(sharedApi: CustomCommandAPI): LoadedCustomCommand[] {
|
||||
const bundled: LoadedCustomCommand[] = [];
|
||||
|
||||
// Add bundled commands here
|
||||
const reviewCommand = createReviewCommand(sharedApi);
|
||||
bundled.push({
|
||||
path: "bundled:review",
|
||||
resolvedPath: "bundled:review",
|
||||
command: reviewCommand,
|
||||
source: "bundled",
|
||||
});
|
||||
|
||||
return bundled;
|
||||
}
|
||||
|
||||
/**
|
||||
* Discover and load custom commands from standard locations.
|
||||
*/
|
||||
@@ -163,6 +182,13 @@ export async function loadCustomCommands(options: LoadCustomCommandsOptions = {}
|
||||
pi: piCodingAgent,
|
||||
};
|
||||
|
||||
// 1. Load bundled commands first (lowest priority - can be overridden)
|
||||
for (const loaded of loadBundledCommands(sharedApi)) {
|
||||
seenNames.add(loaded.command.name);
|
||||
commands.push(loaded);
|
||||
}
|
||||
|
||||
// 2. Load user/project commands (can override bundled)
|
||||
for (const { path: commandPath, source } of paths) {
|
||||
const { commands: loadedCommands, error } = await loadCommandModule(commandPath, cwd, sharedApi);
|
||||
|
||||
@@ -173,13 +199,22 @@ export async function loadCustomCommands(options: LoadCustomCommandsOptions = {}
|
||||
|
||||
if (loadedCommands) {
|
||||
for (const command of loadedCommands) {
|
||||
// Check for name conflicts
|
||||
if (seenNames.has(command.name)) {
|
||||
errors.push({
|
||||
path: commandPath,
|
||||
error: `Command name "${command.name}" conflicts with existing command`,
|
||||
});
|
||||
continue;
|
||||
// Allow overriding bundled commands, but not user/project conflicts
|
||||
const existingIdx = commands.findIndex((c) => c.command.name === command.name);
|
||||
if (existingIdx !== -1) {
|
||||
const existing = commands[existingIdx];
|
||||
if (existing.source === "bundled") {
|
||||
// Override bundled command
|
||||
commands.splice(existingIdx, 1);
|
||||
seenNames.delete(command.name);
|
||||
} else {
|
||||
// Conflict between user/project commands
|
||||
errors.push({
|
||||
path: commandPath,
|
||||
error: `Command name "${command.name}" conflicts with existing command`,
|
||||
});
|
||||
continue;
|
||||
}
|
||||
}
|
||||
|
||||
seenNames.add(command.name);
|
||||
|
||||
@@ -91,7 +91,7 @@ export type CustomCommandFactory = (
|
||||
) => CustomCommand | CustomCommand[] | Promise<CustomCommand | CustomCommand[]>;
|
||||
|
||||
/** Source of a loaded custom command */
|
||||
export type CustomCommandSource = "user" | "project";
|
||||
export type CustomCommandSource = "bundled" | "user" | "project";
|
||||
|
||||
/** Loaded custom command with metadata */
|
||||
export interface LoadedCustomCommand {
|
||||
|
||||
@@ -582,12 +582,11 @@ export async function createAgentSession(options: CreateAgentSessionOptions = {}
|
||||
const contextFiles = options.contextFiles ?? discoverContextFiles(cwd, agentDir);
|
||||
time("discoverContextFiles");
|
||||
|
||||
// Hook runner - created early for hooks
|
||||
let hookRunner: HookRunner | undefined;
|
||||
// Hook runner - always created (needed for custom command context even without hooks)
|
||||
let loadedHooks: LoadedHook[] = [];
|
||||
if (options.hooks !== undefined) {
|
||||
if (options.hooks.length > 0) {
|
||||
const loadedHooks = createLoadedHooksFromDefinitions(options.hooks);
|
||||
hookRunner = new HookRunner(loadedHooks, cwd, sessionManager, modelRegistry);
|
||||
loadedHooks = createLoadedHooksFromDefinitions(options.hooks);
|
||||
}
|
||||
} else {
|
||||
// Discover hooks, merging with additional paths
|
||||
@@ -597,10 +596,9 @@ export async function createAgentSession(options: CreateAgentSessionOptions = {}
|
||||
for (const { path, error } of errors) {
|
||||
console.error(`Failed to load hook "${path}": ${error}`);
|
||||
}
|
||||
if (hooks.length > 0) {
|
||||
hookRunner = new HookRunner(hooks, cwd, sessionManager, modelRegistry);
|
||||
}
|
||||
loadedHooks = hooks;
|
||||
}
|
||||
const hookRunner = new HookRunner(loadedHooks, cwd, sessionManager, modelRegistry);
|
||||
|
||||
const sessionContext = {
|
||||
getSessionFile: () => sessionManager.getSessionFile() ?? null,
|
||||
|
||||
@@ -18,7 +18,7 @@ const PRIORITY_LABELS: Record<number, string> = {
|
||||
3: "P3",
|
||||
};
|
||||
|
||||
const PRIORITY_DESCRIPTIONS: Record<number, string> = {
|
||||
const _PRIORITY_DESCRIPTIONS: Record<number, string> = {
|
||||
0: "Drop everything to fix. Blocking release, operations, or major usage.",
|
||||
1: "Urgent. Should be addressed in the next cycle.",
|
||||
2: "Normal. To be fixed eventually.",
|
||||
@@ -121,16 +121,12 @@ const SubmitReviewParams = Type.Object({
|
||||
maximum: 1,
|
||||
description: "Overall confidence score 0.0-1.0",
|
||||
}),
|
||||
findings_count: Type.Number({
|
||||
description: "Total number of findings reported",
|
||||
}),
|
||||
});
|
||||
|
||||
interface SubmitReviewDetails {
|
||||
overall_correctness: "correct" | "incorrect";
|
||||
explanation: string;
|
||||
confidence: number;
|
||||
findings_count: number;
|
||||
}
|
||||
|
||||
export const submitReviewTool: AgentTool<typeof SubmitReviewParams, SubmitReviewDetails, Theme> = {
|
||||
@@ -141,17 +137,16 @@ export const submitReviewTool: AgentTool<typeof SubmitReviewParams, SubmitReview
|
||||
hidden: true,
|
||||
|
||||
async execute(_toolCallId, params, _signal, _onUpdate, _ctx) {
|
||||
const { overall_correctness, explanation, confidence, findings_count } = params;
|
||||
const { overall_correctness, explanation, confidence } = params;
|
||||
|
||||
let summary = `## Review Summary\n\n`;
|
||||
summary += `**Verdict:** ${overall_correctness === "correct" ? "✓ Patch is correct" : "✗ Patch is incorrect"}\n`;
|
||||
summary += `**Confidence:** ${(confidence * 100).toFixed(0)}%\n`;
|
||||
summary += `**Findings:** ${findings_count}\n\n`;
|
||||
summary += `**Confidence:** ${(confidence * 100).toFixed(0)}%\n\n`;
|
||||
summary += explanation;
|
||||
|
||||
return {
|
||||
content: [{ type: "text", text: summary }],
|
||||
details: { overall_correctness, explanation, confidence, findings_count },
|
||||
details: { overall_correctness, explanation, confidence },
|
||||
};
|
||||
},
|
||||
|
||||
@@ -184,11 +179,6 @@ export const submitReviewTool: AgentTool<typeof SubmitReviewParams, SubmitReview
|
||||
),
|
||||
);
|
||||
|
||||
if (details.findings_count > 0) {
|
||||
container.addChild(new Spacer(1));
|
||||
container.addChild(new Text(theme.fg("muted", `${details.findings_count} finding(s) reported`), 0, 0));
|
||||
}
|
||||
|
||||
if (expanded) {
|
||||
container.addChild(new Spacer(1));
|
||||
container.addChild(new Text(theme.fg("dim", details.explanation), 0, 0));
|
||||
@@ -205,3 +195,74 @@ export function createReportFindingTool(): AgentTool<typeof ReportFindingParams,
|
||||
export function createSubmitReviewTool(): AgentTool<typeof SubmitReviewParams, SubmitReviewDetails, Theme> {
|
||||
return submitReviewTool;
|
||||
}
|
||||
|
||||
// Re-export types for external use
|
||||
export type { ReportFindingDetails, SubmitReviewDetails };
|
||||
|
||||
// ─────────────────────────────────────────────────────────────────────────────
|
||||
// Subprocess tool handlers - registered for extraction/rendering in task tool
|
||||
// ─────────────────────────────────────────────────────────────────────────────
|
||||
|
||||
import path from "node:path";
|
||||
import { subprocessToolRegistry } from "./task/subprocess-tool-registry";
|
||||
|
||||
// Register report_finding handler
|
||||
subprocessToolRegistry.register<ReportFindingDetails>("report_finding", {
|
||||
extractData: (event) => event.result?.details as ReportFindingDetails | undefined,
|
||||
|
||||
renderInline: (data, theme) => {
|
||||
const priority = PRIORITY_LABELS[data.priority] ?? "P?";
|
||||
const color = data.priority === 0 ? "error" : data.priority === 1 ? "warning" : "muted";
|
||||
const titleText = data.title.replace(/^\[P\d\]\s*/, "");
|
||||
const loc = `${path.basename(data.file_path)}:${data.line_start}`;
|
||||
return new Text(`${theme.fg(color, `[${priority}]`)} ${titleText} ${theme.fg("dim", loc)}`, 0, 0);
|
||||
},
|
||||
|
||||
renderFinal: (allData, theme, expanded) => {
|
||||
const container = new Container();
|
||||
const displayCount = expanded ? allData.length : Math.min(3, allData.length);
|
||||
|
||||
for (let i = 0; i < displayCount; i++) {
|
||||
const data = allData[i];
|
||||
const priority = PRIORITY_LABELS[data.priority] ?? "P?";
|
||||
const color = data.priority === 0 ? "error" : data.priority === 1 ? "warning" : "muted";
|
||||
const titleText = data.title.replace(/^\[P\d\]\s*/, "");
|
||||
const loc = `${path.basename(data.file_path)}:${data.line_start}`;
|
||||
|
||||
container.addChild(
|
||||
new Text(` ${theme.fg(color, `[${priority}]`)} ${titleText} ${theme.fg("dim", loc)}`, 0, 0),
|
||||
);
|
||||
|
||||
if (expanded && data.body) {
|
||||
container.addChild(new Text(` ${theme.fg("dim", data.body)}`, 0, 0));
|
||||
}
|
||||
}
|
||||
|
||||
if (allData.length > displayCount) {
|
||||
container.addChild(new Text(theme.fg("dim", ` ... ${allData.length - displayCount} more findings`), 0, 0));
|
||||
}
|
||||
|
||||
return container;
|
||||
},
|
||||
});
|
||||
|
||||
// Register submit_review handler
|
||||
subprocessToolRegistry.register<SubmitReviewDetails>("submit_review", {
|
||||
extractData: (event) => event.result?.details as SubmitReviewDetails | undefined,
|
||||
|
||||
// Terminate subprocess after review is submitted
|
||||
shouldTerminate: () => true,
|
||||
|
||||
renderInline: (data, theme) => {
|
||||
const verdictColor = data.overall_correctness === "correct" ? "success" : "error";
|
||||
const verdictIcon = data.overall_correctness === "correct" ? "✓" : "✗";
|
||||
return new Text(
|
||||
`${theme.fg(verdictColor, verdictIcon)} Review: ${theme.fg(verdictColor, data.overall_correctness)} (${(data.confidence * 100).toFixed(0)}%)`,
|
||||
0,
|
||||
0,
|
||||
);
|
||||
},
|
||||
|
||||
// Note: renderFinal is NOT used for submit_review - we use the combined
|
||||
// renderReviewResult in render.ts to show verdict + findings together
|
||||
});
|
||||
|
||||
@@ -1,7 +1,7 @@
|
||||
---
|
||||
name: reviewer
|
||||
description: Code review specialist for quality and security analysis
|
||||
tools: read, grep, glob, ls, bash, report_finding, submit_review
|
||||
tools: read, grep, find, ls, bash, report_finding, submit_review
|
||||
model: pi/slow, gpt-5.2-codex, gpt-5.2, codex, gpt
|
||||
---
|
||||
|
||||
@@ -48,6 +48,12 @@ Only flag issues where ALL of these apply:
|
||||
7. Write so the author can immediately grasp the idea without close reading
|
||||
8. Avoid flattery and phrases like "Great job...", "Thanks for..."
|
||||
|
||||
# CRITICAL
|
||||
|
||||
You MUST call `submit_review` before ending your response, even if you found no issues.
|
||||
The review is only considered complete when `submit_review` is called.
|
||||
Failure to call `submit_review` means the review was not submitted.
|
||||
|
||||
# Output
|
||||
|
||||
- Use `report_finding` for each issue. Continue until you've listed every qualifying finding.
|
||||
|
||||
@@ -232,6 +232,7 @@ export async function runSubprocess(options: ExecutorOptions): Promise<SingleRes
|
||||
let stderr = "";
|
||||
let finalOutput = "";
|
||||
let resolved = false;
|
||||
let pendingTermination = false; // Set when shouldTerminate fires, wait for message_end
|
||||
const jsonlEvents: string[] = [];
|
||||
|
||||
// Handle abort signal
|
||||
@@ -308,8 +309,15 @@ export async function runSubprocess(options: ExecutorOptions): Promise<SingleRes
|
||||
isError: event.isError,
|
||||
})
|
||||
) {
|
||||
proc.kill("SIGTERM");
|
||||
resolved = true;
|
||||
// Don't kill immediately - wait for message_end to get token counts
|
||||
pendingTermination = true;
|
||||
// Safety timeout in case message_end never arrives
|
||||
setTimeout(() => {
|
||||
if (!resolved) {
|
||||
proc.kill("SIGTERM");
|
||||
resolved = true;
|
||||
}
|
||||
}, 2000);
|
||||
}
|
||||
}
|
||||
break;
|
||||
@@ -345,7 +353,13 @@ export async function runSubprocess(options: ExecutorOptions): Promise<SingleRes
|
||||
// Extract usage (prefer message.usage, fallback to event.usage)
|
||||
const messageUsage = event.message?.usage || event.usage;
|
||||
if (messageUsage) {
|
||||
progress.tokens = (messageUsage.input_tokens || 0) + (messageUsage.output_tokens || 0);
|
||||
// Accumulate tokens across messages (not overwrite)
|
||||
progress.tokens += (messageUsage.input_tokens || 0) + (messageUsage.output_tokens || 0);
|
||||
}
|
||||
// If pending termination, now we have tokens - terminate
|
||||
if (pendingTermination && !resolved) {
|
||||
proc.kill("SIGTERM");
|
||||
resolved = true;
|
||||
}
|
||||
break;
|
||||
}
|
||||
|
||||
@@ -30,6 +30,9 @@ import {
|
||||
taskSchema,
|
||||
} from "./types";
|
||||
|
||||
// Import review tools for side effects (registers subprocess tool handlers)
|
||||
import "../review";
|
||||
|
||||
/** Session context interface */
|
||||
interface SessionContext {
|
||||
getSessionFile: () => string | null;
|
||||
|
||||
@@ -10,9 +10,18 @@ import type { Component } from "@oh-my-pi/pi-tui";
|
||||
import { Container, Text } from "@oh-my-pi/pi-tui";
|
||||
import type { Theme } from "../../../modes/interactive/theme/theme";
|
||||
import type { RenderResultOptions } from "../../custom-tools/types";
|
||||
import type { ReportFindingDetails, SubmitReviewDetails } from "../review";
|
||||
import { subprocessToolRegistry } from "./subprocess-tool-registry";
|
||||
import type { AgentProgress, SingleResult, TaskParams, TaskToolDetails } from "./types";
|
||||
|
||||
/** Priority labels for review findings */
|
||||
const PRIORITY_LABELS: Record<number, string> = {
|
||||
0: "P0",
|
||||
1: "P1",
|
||||
2: "P2",
|
||||
3: "P3",
|
||||
};
|
||||
|
||||
/**
|
||||
* Format token count for display (e.g., 1.5k, 25k).
|
||||
*/
|
||||
@@ -160,6 +169,78 @@ function renderAgentProgress(progress: AgentProgress, isLast: boolean, expanded:
|
||||
return lines;
|
||||
}
|
||||
|
||||
/**
|
||||
* Render review result with combined verdict + findings in tree structure.
|
||||
*/
|
||||
function renderReviewResult(
|
||||
summary: SubmitReviewDetails,
|
||||
findings: ReportFindingDetails[],
|
||||
continuePrefix: string,
|
||||
expanded: boolean,
|
||||
theme: Theme,
|
||||
): string[] {
|
||||
const lines: string[] = [];
|
||||
|
||||
// Verdict line
|
||||
const verdictColor = summary.overall_correctness === "correct" ? "success" : "error";
|
||||
const verdictIcon = summary.overall_correctness === "correct" ? "✓" : "✗";
|
||||
lines.push(
|
||||
`${continuePrefix}${theme.fg(verdictColor, verdictIcon)} Patch is ${theme.fg(verdictColor, summary.overall_correctness)} ${theme.fg("dim", `(${(summary.confidence * 100).toFixed(0)}% confidence)`)}`,
|
||||
);
|
||||
|
||||
// Explanation preview (first ~80 chars when collapsed, full when expanded)
|
||||
if (summary.explanation) {
|
||||
if (expanded) {
|
||||
// Full explanation, wrapped
|
||||
const explanationLines = summary.explanation.split("\n");
|
||||
for (const line of explanationLines) {
|
||||
lines.push(`${continuePrefix}${theme.fg("dim", line)}`);
|
||||
}
|
||||
} else {
|
||||
// Preview: first sentence or ~100 chars
|
||||
const preview = truncate(`${summary.explanation.split(/[.!?]/)[0]}.`, 100);
|
||||
lines.push(`${continuePrefix}${theme.fg("dim", preview)}`);
|
||||
}
|
||||
}
|
||||
|
||||
// Findings in tree structure
|
||||
if (findings.length > 0) {
|
||||
lines.push(`${continuePrefix}`); // Spacing
|
||||
const displayCount = expanded ? findings.length : Math.min(3, findings.length);
|
||||
|
||||
for (let i = 0; i < displayCount; i++) {
|
||||
const finding = findings[i];
|
||||
const isLastFinding = i === displayCount - 1 && (expanded || findings.length <= 3);
|
||||
const findingPrefix = isLastFinding ? "└─" : "├─";
|
||||
const findingContinue = isLastFinding ? " " : "│ ";
|
||||
|
||||
const priority = PRIORITY_LABELS[finding.priority] ?? "P?";
|
||||
const color = finding.priority === 0 ? "error" : finding.priority === 1 ? "warning" : "muted";
|
||||
const titleText = finding.title.replace(/^\[P\d\]\s*/, "");
|
||||
const loc = `${path.basename(finding.file_path)}:${finding.line_start}`;
|
||||
|
||||
lines.push(
|
||||
`${continuePrefix}${findingPrefix} ${theme.fg(color, `[${priority}]`)} ${titleText} ${theme.fg("dim", loc)}`,
|
||||
);
|
||||
|
||||
// Show body when expanded
|
||||
if (expanded && finding.body) {
|
||||
// Wrap body text
|
||||
const bodyLines = finding.body.split("\n");
|
||||
for (const bodyLine of bodyLines) {
|
||||
lines.push(`${continuePrefix}${findingContinue}${theme.fg("dim", bodyLine)}`);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
if (!expanded && findings.length > 3) {
|
||||
lines.push(`${continuePrefix}${theme.fg("dim", `... ${findings.length - 3} more findings`)}`);
|
||||
}
|
||||
}
|
||||
|
||||
return lines;
|
||||
}
|
||||
|
||||
/**
|
||||
* Render final result for a single agent.
|
||||
*/
|
||||
@@ -177,7 +258,9 @@ function renderAgentResult(result: SingleResult, isLast: boolean, expanded: bool
|
||||
// Main status line
|
||||
let statusLine = `${prefix} ${theme.fg(iconColor, icon)} ${theme.fg("accent", result.agent)}`;
|
||||
statusLine += `: ${theme.fg(iconColor, statusText)}`;
|
||||
statusLine += ` · ${theme.fg("dim", `${formatTokens(result.tokens)} tokens`)}`;
|
||||
if (result.tokens > 0) {
|
||||
statusLine += ` · ${theme.fg("dim", `${formatTokens(result.tokens)} tokens`)}`;
|
||||
}
|
||||
statusLine += ` · ${theme.fg("dim", formatDuration(result.durationMs))}`;
|
||||
|
||||
if (result.truncated) {
|
||||
@@ -186,10 +269,25 @@ function renderAgentResult(result: SingleResult, isLast: boolean, expanded: bool
|
||||
|
||||
lines.push(statusLine);
|
||||
|
||||
// Check for extracted tool data with custom renderers
|
||||
// Check for review result (submit_review + report_finding)
|
||||
const submitReviewData = result.extractedToolData?.submit_review as SubmitReviewDetails[] | undefined;
|
||||
const reportFindingData = result.extractedToolData?.report_finding as ReportFindingDetails[] | undefined;
|
||||
|
||||
if (submitReviewData && submitReviewData.length > 0) {
|
||||
// Use combined review renderer
|
||||
const summary = submitReviewData[submitReviewData.length - 1];
|
||||
const findings = reportFindingData ?? [];
|
||||
lines.push(...renderReviewResult(summary, findings, continuePrefix, expanded, theme));
|
||||
return lines;
|
||||
}
|
||||
|
||||
// Check for extracted tool data with custom renderers (skip review tools)
|
||||
let hasCustomRendering = false;
|
||||
if (result.extractedToolData) {
|
||||
for (const [toolName, dataArray] of Object.entries(result.extractedToolData)) {
|
||||
// Skip review tools - handled above
|
||||
if (toolName === "submit_review" || toolName === "report_finding") continue;
|
||||
|
||||
const handler = subprocessToolRegistry.getHandler(toolName);
|
||||
if (handler?.renderFinal && (dataArray as unknown[]).length > 0) {
|
||||
hasCustomRendering = true;
|
||||
@@ -287,11 +385,7 @@ export function renderResult(
|
||||
summary += ` · ${theme.fg("dim", formatDuration(details.totalDurationMs))}`;
|
||||
lines.push(summary);
|
||||
|
||||
// Artifacts location
|
||||
if (details.outputPaths && details.outputPaths.length > 0) {
|
||||
const artifactsDir = path.dirname(details.outputPaths[0]);
|
||||
lines.push(`${theme.fg("dim", "Artifacts:")} ${theme.fg("muted", artifactsDir)}`);
|
||||
}
|
||||
// Artifacts suppressed from user view - available via session file
|
||||
}
|
||||
|
||||
if (lines.length === 0) {
|
||||
|
||||
@@ -0,0 +1,89 @@
|
||||
/**
|
||||
* Registry for handling tool events from subprocess agents.
|
||||
*
|
||||
* Tools can register handlers to:
|
||||
* - Extract structured data from their execution results
|
||||
* - Trigger subprocess termination on completion
|
||||
* - Provide custom rendering for realtime/final display
|
||||
*/
|
||||
|
||||
import type { Component } from "@oh-my-pi/pi-tui";
|
||||
import type { Theme } from "../../../modes/interactive/theme/theme";
|
||||
|
||||
/** Event from subprocess tool execution (parsed from JSONL) */
|
||||
export interface SubprocessToolEvent {
|
||||
toolName: string;
|
||||
toolCallId: string;
|
||||
args?: Record<string, unknown>;
|
||||
result?: {
|
||||
content: Array<{ type: string; text?: string }>;
|
||||
details?: unknown;
|
||||
};
|
||||
isError?: boolean;
|
||||
}
|
||||
|
||||
/** Handler for subprocess tool events */
|
||||
export interface SubprocessToolHandler<TData = unknown> {
|
||||
/**
|
||||
* Extract structured data from tool result.
|
||||
* Extracted data is accumulated in progress.extractedToolData[toolName][].
|
||||
*/
|
||||
extractData?: (event: SubprocessToolEvent) => TData | undefined;
|
||||
|
||||
/**
|
||||
* Whether this tool's completion should terminate the subprocess.
|
||||
* Return true to send SIGTERM after the tool completes.
|
||||
*/
|
||||
shouldTerminate?: (event: SubprocessToolEvent) => boolean;
|
||||
|
||||
/**
|
||||
* Render a single data item inline during streaming progress.
|
||||
* Called for each tool execution end event.
|
||||
*/
|
||||
renderInline?: (data: TData, theme: Theme) => Component;
|
||||
|
||||
/**
|
||||
* Render accumulated data in the final result view.
|
||||
* Called once with all accumulated data for this tool.
|
||||
*/
|
||||
renderFinal?: (allData: TData[], theme: Theme, expanded: boolean) => Component;
|
||||
}
|
||||
|
||||
/** Registry for subprocess tool handlers */
|
||||
class SubprocessToolRegistryImpl {
|
||||
private handlers = new Map<string, SubprocessToolHandler>();
|
||||
|
||||
/**
|
||||
* Register a handler for a tool's subprocess events.
|
||||
*/
|
||||
register<T>(toolName: string, handler: SubprocessToolHandler<T>): void {
|
||||
this.handlers.set(toolName, handler as SubprocessToolHandler);
|
||||
}
|
||||
|
||||
/**
|
||||
* Get the handler for a tool, if registered.
|
||||
*/
|
||||
getHandler(toolName: string): SubprocessToolHandler | undefined {
|
||||
return this.handlers.get(toolName);
|
||||
}
|
||||
|
||||
/**
|
||||
* Check if a tool has a registered handler.
|
||||
*/
|
||||
hasHandler(toolName: string): boolean {
|
||||
return this.handlers.has(toolName);
|
||||
}
|
||||
|
||||
/**
|
||||
* Get all registered tool names.
|
||||
*/
|
||||
getRegisteredTools(): string[] {
|
||||
return Array.from(this.handlers.keys());
|
||||
}
|
||||
}
|
||||
|
||||
/** Singleton registry instance */
|
||||
export const subprocessToolRegistry = new SubprocessToolRegistryImpl();
|
||||
|
||||
/** Type helper for extracted tool data in progress/result */
|
||||
export type ExtractedToolData = Record<string, unknown[]>;
|
||||
@@ -57,7 +57,6 @@ export interface ReviewSummary {
|
||||
overall_correctness: "correct" | "incorrect";
|
||||
explanation: string;
|
||||
confidence: number;
|
||||
findings_count: number;
|
||||
}
|
||||
|
||||
/** Structured review data extracted from reviewer agent */
|
||||
|
||||
@@ -1,6 +1,9 @@
|
||||
# Changelog
|
||||
|
||||
## [Unreleased]
|
||||
### Added
|
||||
|
||||
- Added `getText()` method to Text component for retrieving current text content
|
||||
|
||||
## [1.341.0] - 2026-01-03
|
||||
|
||||
@@ -66,4 +69,4 @@ Initial release under @oh-my-pi scope. See previous releases at [badlogic/pi-mon
|
||||
|
||||
### Fixed
|
||||
|
||||
- **Readline-style Ctrl+W**: Now skips trailing whitespace before deleting the preceding word, matching standard readline behavior. ([#306](https://github.com/badlogic/pi-mono/pull/306) by [@kim0](https://github.com/kim0))
|
||||
- **Readline-style Ctrl+W**: Now skips trailing whitespace before deleting the preceding word, matching standard readline behavior. ([#306](https://github.com/badlogic/pi-mono/pull/306) by [@kim0](https://github.com/kim0))
|
||||
@@ -22,6 +22,10 @@ export class Text implements Component {
|
||||
this.customBgFn = customBgFn;
|
||||
}
|
||||
|
||||
getText(): string {
|
||||
return this.text;
|
||||
}
|
||||
|
||||
setText(text: string): void {
|
||||
this.text = text;
|
||||
this.cachedText = undefined;
|
||||
|
||||
@@ -8,7 +8,8 @@
|
||||
"build": "vite build",
|
||||
"preview": "vite preview",
|
||||
"clean": "rm -rf dist",
|
||||
"check": "biome check --write . && tsgo --noEmit"
|
||||
"check": "biome check --write . && tsgo --noEmit",
|
||||
"fix": "biome check --write --unsafe ."
|
||||
},
|
||||
"dependencies": {
|
||||
"@mariozechner/mini-lit": "^0.2.0",
|
||||
|
||||
@@ -14,7 +14,8 @@
|
||||
"dev:example": "bun --cwd=example run dev",
|
||||
"dev": "bun run dev:css & bun run dev:example",
|
||||
"build:css": "tailwindcss -i ./src/app.css -o ./dist/app.css --minify",
|
||||
"check": "biome check --write . && tsgo --noEmit && bun --cwd=example run check"
|
||||
"check": "biome check --write . && tsgo --noEmit && bun --cwd=example run check",
|
||||
"fix": "biome check --write --unsafe . && bun --cwd=example run fix"
|
||||
},
|
||||
"dependencies": {
|
||||
"@lmstudio/sdk": "^1.5.0",
|
||||
|
||||
Reference in New Issue
Block a user