feat(coding-agent): added structured code review with hidden tools and explicit opt-in

- 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.
- Added example review tools (report_finding, submit_review) with structured findings accumulation and verdict rendering.
- Added /review command for interactive code review with branch comparison, uncommitted changes, and commit review modes.
- Updated bundled reviewer agent to use structured review tools with priority-based findings (P0-P3) and formal verdict submission.
This commit is contained in:
can1357
2026-01-03 05:08:20 +01:00
parent 83809771d9
commit c70303ade9
12 changed files with 729 additions and 138 deletions
@@ -0,0 +1,79 @@
# Review Tools
Structured code review tools with findings accumulation and verdict rendering.
## Components
- **`report_finding`** - Report individual findings with priority (P0-P3), location, confidence
- **`submit_review`** - Submit final verdict with grouped findings summary
- **`/review`** - Interactive command to launch code review
Both tools have `hidden: true` - they only appear when explicitly listed in an agent's tools.
## Installation
From the repository root:
```bash
# Install review tools
mkdir -p ~/.pi/agent/tools/review
ln -sf "$(pwd)/packages/coding-agent/examples/custom-tools/review/index.ts" ~/.pi/agent/tools/review/index.ts
# Install /review command
mkdir -p ~/.pi/agent/commands/review
ln -sf "$(pwd)/packages/coding-agent/examples/custom-tools/review/commands/review/index.ts" ~/.pi/agent/commands/review/index.ts
```
## Usage with Subagent
The `reviewer` agent in the subagent example uses these tools. Make sure both subagent and review are installed:
```bash
# Also install subagent tools if not already done
# See: examples/custom-tools/subagent/README.md
```
Then use `/review`:
```
/review
```
This opens an interactive menu:
1. Review against a base branch (PR style)
2. Review uncommitted changes
3. Review a specific commit
4. Custom review instructions
## Tool Schemas
### report_finding
```typescript
{
title: string; // ≤80 chars, prefixed with [P0-P3]
body: string; // Markdown explanation
priority: 0 | 1 | 2 | 3;
confidence: number; // 0.0-1.0
file_path: string;
line_start: number;
line_end: number;
}
```
### submit_review
```typescript
{
overall_correctness: "correct" | "incorrect";
explanation: string; // 1-3 sentences
confidence: number; // 0.0-1.0
}
```
## Priority Levels
- **P0**: Drop everything. Blocking release/operations.
- **P1**: Urgent. Address in next cycle.
- **P2**: Normal. Fix eventually.
- **P3**: Low. Nice to have.
@@ -0,0 +1,160 @@
/**
* /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 { CustomCommandAPI, CustomCommandFactory } from "@oh-my-pi/pi-coding-agent";
const factory: CustomCommandFactory = (api: CustomCommandAPI) => {
return {
name: "review",
description: "Launch interactive code review",
async execute(_args, ctx) {
if (!ctx.hasUI) {
return "Review command requires interactive mode. Run `pi` without --print flag.";
}
// 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;
const modeNum = parseInt(mode[0]);
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;
}
const baseBranch = await ctx.ui.select("Select base branch to compare against", branches);
if (!baseBranch) return;
const currentBranch = await getCurrentBranch(api);
return `Use the subagent 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;
}
return `Use the subagent 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;
}
const selected = await ctx.ui.select("Select commit to review", commits);
if (!selected) return;
// Extract commit hash from selection (format: "abc1234 - message")
const hash = selected.split(" ")[0];
return `Use the subagent 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;
return `Use the subagent tool to run the "reviewer" agent with this task:
${instructions}`;
}
default:
return;
}
},
};
};
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: string) => 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: string) => c.trim())
.filter(Boolean);
} catch {
return [];
}
}
export default factory;
@@ -0,0 +1,291 @@
/**
* Review Tools - report_finding and submit_review
*
* Used by code review agents to report findings in a structured way.
* Both tools are hidden by default - only enabled when explicitly listed in agent's tools.
*
* - report_finding: Accumulates findings in session state
* - submit_review: Collects all findings and renders final verdict
*/
import type {
CustomTool,
CustomToolContext,
CustomToolFactory,
CustomToolSessionEvent,
} from "@oh-my-pi/pi-coding-agent";
interface Finding {
title: string;
body: string;
priority: 0 | 1 | 2 | 3;
confidence: number;
file_path: string;
line_start: number;
line_end: number;
}
interface FindingDetails {
finding: Finding;
findings: Finding[];
}
interface SubmitDetails {
overall_correctness: "correct" | "incorrect";
explanation: string;
confidence: number;
findings: Finding[];
}
const PRIORITY_LABELS: Record<number, string> = {
0: "P0",
1: "P1",
2: "P2",
3: "P3",
};
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.",
3: "Low. Nice to have.",
};
const factory: CustomToolFactory = (pi) => {
const { Type } = pi.typebox;
const { Text, Container, Spacer, Markdown, StringEnum } = pi.pi;
// In-memory state (reconstructed from session on load)
let findings: Finding[] = [];
const reconstructState = (_event: CustomToolSessionEvent, ctx: CustomToolContext) => {
findings = [];
for (const entry of ctx.sessionManager.getBranch()) {
if (entry.type !== "message") continue;
const msg = entry.message;
if (msg.role !== "toolResult") continue;
if (msg.toolName === "report_finding") {
const details = msg.details as FindingDetails | undefined;
if (details?.findings) {
findings = details.findings;
}
} else if (msg.toolName === "submit_review") {
// After submit_review, findings are cleared for next review
findings = [];
}
}
};
// report_finding tool
const FindingParams = Type.Object({
title: Type.String({
description: "≤80 chars, imperative, prefixed with [P0-P3]. E.g., '[P1] Un-padding slices along wrong dimension'",
}),
body: Type.String({
description: "Markdown explaining why this is a problem. One paragraph max.",
}),
priority: Type.Union([Type.Literal(0), Type.Literal(1), Type.Literal(2), Type.Literal(3)], {
description: "0=P0 (critical), 1=P1 (urgent), 2=P2 (normal), 3=P3 (low)",
}),
confidence: Type.Number({
minimum: 0,
maximum: 1,
description: "Confidence score 0.0-1.0",
}),
file_path: Type.String({ description: "Absolute path to the file" }),
line_start: Type.Number({ description: "Start line of the issue" }),
line_end: Type.Number({ description: "End line of the issue" }),
});
const reportFinding: CustomTool<typeof FindingParams, FindingDetails> = {
name: "report_finding",
label: "Report Finding",
description:
"Report a code review finding. Use this for each issue found. Call submit_review when done reviewing.",
parameters: FindingParams,
hidden: true,
onSession: reconstructState,
async execute(_toolCallId, params, _onUpdate, _ctx, _signal) {
const finding: Finding = {
title: params.title,
body: params.body,
priority: params.priority,
confidence: params.confidence,
file_path: params.file_path,
line_start: params.line_start,
line_end: params.line_end,
};
findings.push(finding);
return {
content: [
{
type: "text",
text: `Finding recorded: ${finding.title} (${findings.length} total)`,
},
],
details: { finding, findings: [...findings] },
};
},
renderCall(args, theme) {
const priority = PRIORITY_LABELS[args.priority] ?? "P?";
const color = args.priority === 0 ? "error" : args.priority === 1 ? "warning" : "muted";
return new Text(
`${theme.fg("toolTitle", theme.bold("report_finding "))}${theme.fg(color, `[${priority}]`)} ${theme.fg("dim", args.title.replace(/^\[P\d\]\s*/, ""))}`,
0,
0,
);
},
renderResult(result, _options, theme) {
const { details } = result;
if (!details) {
const text = result.content[0];
return new Text(text?.type === "text" ? text.text : "", 0, 0);
}
const { finding } = details;
const priority = PRIORITY_LABELS[finding.priority] ?? "P?";
const color = finding.priority === 0 ? "error" : finding.priority === 1 ? "warning" : "muted";
const location = `${finding.file_path}:${finding.line_start}${finding.line_end !== finding.line_start ? `-${finding.line_end}` : ""}`;
return new Text(
`${theme.fg("success", "✓")} ${theme.fg(color, `[${priority}]`)} ${theme.fg("dim", location)}`,
0,
0,
);
},
};
// submit_review tool
const SubmitParams = Type.Object({
overall_correctness: StringEnum(["correct", "incorrect"] as const, {
description: "Whether the patch is correct (no bugs, tests won't break)",
}),
explanation: Type.String({
description: "1-3 sentence explanation justifying the verdict",
}),
confidence: Type.Number({
minimum: 0,
maximum: 1,
description: "Overall confidence score 0.0-1.0",
}),
});
const submitReview: CustomTool<typeof SubmitParams, SubmitDetails> = {
name: "submit_review",
label: "Submit Review",
description:
"Submit the final review verdict. Call this after all findings have been reported. Summarizes all findings and provides overall assessment.",
parameters: SubmitParams,
hidden: true,
onSession: reconstructState,
async execute(_toolCallId, params, _onUpdate, _ctx, _signal) {
const result: SubmitDetails = {
overall_correctness: params.overall_correctness as "correct" | "incorrect",
explanation: params.explanation,
confidence: params.confidence,
findings: [...findings],
};
// Group findings by priority
const byPriority = findings.reduce(
(acc, f) => {
acc[f.priority] = acc[f.priority] || [];
acc[f.priority].push(f);
return acc;
},
{} as Record<number, Finding[]>,
);
let summary = `## Review Summary\n\n`;
summary += `**Verdict:** ${params.overall_correctness === "correct" ? "✓ Patch is correct" : "✗ Patch is incorrect"}\n`;
summary += `**Confidence:** ${(params.confidence * 100).toFixed(0)}%\n\n`;
summary += `${params.explanation}\n\n`;
if (findings.length > 0) {
summary += `### Findings (${findings.length})\n\n`;
for (const priority of [0, 1, 2, 3]) {
const group = byPriority[priority];
if (!group || group.length === 0) continue;
summary += `#### ${PRIORITY_LABELS[priority]} - ${PRIORITY_DESCRIPTIONS[priority]}\n\n`;
for (const f of group) {
const location = `${f.file_path}:${f.line_start}${f.line_end !== f.line_start ? `-${f.line_end}` : ""}`;
summary += `- **${f.title}** (${location})\n ${f.body}\n\n`;
}
}
} else {
summary += `No findings reported.\n`;
}
// Clear findings for next review
const savedFindings = [...findings];
findings = [];
return {
content: [{ type: "text", text: summary }],
details: { ...result, findings: savedFindings },
};
},
renderCall(args, theme) {
const verdict = args.overall_correctness === "correct" ? "correct" : "incorrect";
const color = args.overall_correctness === "correct" ? "success" : "error";
return new Text(
`${theme.fg("toolTitle", theme.bold("submit_review "))}${theme.fg(color, verdict)} ${theme.fg("dim", `(${(args.confidence * 100).toFixed(0)}%)`)}`,
0,
0,
);
},
renderResult(result, { expanded }, theme) {
const { details } = result;
if (!details) {
const text = result.content[0];
return new Text(text?.type === "text" ? text.text : "", 0, 0);
}
const container = new Container();
const verdictColor = details.overall_correctness === "correct" ? "success" : "error";
const verdictIcon = details.overall_correctness === "correct" ? "✓" : "✗";
container.addChild(
new Text(
`${theme.fg(verdictColor, verdictIcon)} Patch is ${theme.fg(verdictColor, details.overall_correctness)} ${theme.fg("dim", `(${(details.confidence * 100).toFixed(0)}% confidence)`)}`,
0,
0,
),
);
if (details.findings.length > 0) {
container.addChild(new Spacer(1));
container.addChild(
new Text(theme.fg("muted", `${details.findings.length} finding(s) reported`), 0, 0),
);
}
if (expanded && details.findings.length > 0) {
container.addChild(new Spacer(1));
for (const f of details.findings) {
const priority = PRIORITY_LABELS[f.priority] ?? "P?";
const color = f.priority === 0 ? "error" : f.priority === 1 ? "warning" : "dim";
const location = `${f.file_path}:${f.line_start}`;
container.addChild(
new Text(` ${theme.fg(color, `[${priority}]`)} ${theme.fg("dim", location)}`, 0, 0),
);
}
}
return container;
},
};
return [reportFinding, submitReview];
};
export default factory;
@@ -1,35 +1,60 @@
---
name: reviewer
description: Code review specialist for quality and security analysis
tools: read, grep, find, ls, bash
tools: read, grep, find, ls, bash, report_finding, submit_review
model: claude-sonnet-4-5
---
You are a senior code reviewer. Analyze code for quality, security, and maintainability.
You are acting as a reviewer for a proposed code change made by another engineer.
Bash is for read-only commands only: `git diff`, `git log`, `git show`. Do NOT modify files or run builds.
Assume tool permissions are not perfectly enforceable; keep all bash usage strictly read-only.
Strategy:
1. Run `git diff` to see recent changes (if applicable)
2. Read the modified files
3. Check for bugs, security issues, code smells
# Review Strategy
Output format:
1. Run `git diff` to see the changes being reviewed
2. Read the modified files for full context
3. Analyze for bugs, security issues, and code quality problems
4. Use `report_finding` for each issue found
5. Use `submit_review` to provide final verdict
## Files Reviewed
- `path/to/file.ts` (lines X-Y)
# What to Flag
## Critical (must fix)
- `file.ts:42` - Issue description
Only flag issues where ALL of these apply:
## Warnings (should fix)
- `file.ts:100` - Issue description
1. It meaningfully impacts the accuracy, performance, security, or maintainability of the code
2. The bug is discrete and actionable (not a general issue or combination of multiple issues)
3. Fixing it doesn't demand rigor not present elsewhere in the codebase
4. The bug was introduced in this commit (don't flag pre-existing bugs)
5. The author would likely fix the issue if made aware of it
6. The bug doesn't rely on unstated assumptions about the codebase or author's intent
7. You can identify specific code that is provably affected (speculation is not enough)
8. The issue is clearly not an intentional change by the author
## Suggestions (consider)
- `file.ts:150` - Improvement idea
# Priority Levels
## Summary
Overall assessment in 2-3 sentences.
- **P0**: Drop everything to fix. Blocking release, operations, or major usage. Only use for universal issues that do not depend on assumptions about inputs.
- **P1**: Urgent. Should be addressed in the next cycle.
- **P2**: Normal. To be fixed eventually.
- **P3**: Low. Nice to have.
Be specific with file paths and line numbers.
# Comment Guidelines
1. Be clear about WHY the issue is a bug
2. Communicate severity appropriately - don't overstate
3. Keep body to one paragraph max
4. Code snippets should be ≤3 lines, wrapped in markdown code tags
5. Clearly state what conditions are necessary for the bug to arise
6. Tone: matter-of-fact, not accusatory or overly positive
7. Write so the author can immediately grasp the idea without close reading
8. Avoid flattery and phrases like "Great job...", "Thanks for..."
# Output
- Use `report_finding` for each issue. Continue until you've listed every qualifying finding.
- If there is no finding that a person would definitely want to fix, prefer outputting no findings.
- Ignore trivial style unless it obscures meaning or violates documented standards.
- Use `submit_review` at the end with your overall verdict:
- **correct**: Existing code and tests will not break, patch is free of bugs and blocking issues
- **incorrect**: Has bugs or blocking issues that must be addressed
Ignore non-blocking issues (style, formatting, typos, documentation, nits) when determining correctness.