mirror of
https://github.com/backnotprop/plannotator.git
synced 2026-09-14 14:17:26 +08:00
feat(review): custom reviews as Agent Skills + whole-file/general findings (#955)
Custom reviews: pick an Agent Skill and its SKILL.md body becomes the review prompt (read live from global skill folders, never copied; default review byte-identical). Agent findings can be a line, a whole file, or a general review-level comment, and are never silently dropped. Plus: agent-icon engine picker, icon-only file-header opener, semantic diff restored as its own dock panel, and a dead-code sweep.
This commit is contained in:
Michael Ramos
@@ -312,6 +312,9 @@ During normal plan review, an Archive sidebar tab provides the same browsing via
|
||||
| `/api/external-annotations` | PATCH | Update fields on a single annotation (`?id=`) |
|
||||
| `/api/external-annotations` | DELETE | Remove by `?id=`, `?source=`, or clear all |
|
||||
| `/api/agents/capabilities` | GET | Check available agent providers (claude, codex, tour) |
|
||||
| `/api/agents/review-profiles` | GET | List launchable review profiles (enabled skills + builtin default) |
|
||||
| `/api/agents/skills` | GET | List all discovered skills for the add-a-review picker (each flagged `enabled`) |
|
||||
| `/api/agents/review-skills` | POST | Enable a skill as a review (body: `{ name }`); writes `review-skills.json` |
|
||||
| `/api/agents/jobs/stream` | GET | SSE stream for real-time agent job status updates |
|
||||
| `/api/agents/jobs` | GET | Snapshot of agent jobs (polling fallback, `?since=N` for version gating) |
|
||||
| `/api/agents/jobs` | POST | Launch an agent job (body: `{ provider, command, label }`) |
|
||||
|
||||
@@ -259,7 +259,7 @@ At the end, output an overall correctness verdict.
|
||||
codex exec \
|
||||
--output-schema ~/.plannotator/codex-review-schema.json \
|
||||
-o /tmp/plannotator-codex-<uuid>.json \
|
||||
--full-auto \
|
||||
--sandbox workspace-write \
|
||||
--ephemeral \
|
||||
-C <working-directory> \
|
||||
"<system-prompt>\n\n---\n\n<user-message>"
|
||||
|
||||
@@ -79,6 +79,10 @@ export interface AgentJobHandlerOptions {
|
||||
diffScope?: string;
|
||||
/** Diff context snapshot at launch (stored on AgentJobInfo for per-job "Copy All"). */
|
||||
diffContext?: AgentJobInfo["diffContext"];
|
||||
/** Resolved review profile id at launch time. Stored on AgentJobInfo. */
|
||||
reviewProfileId?: string;
|
||||
/** Resolved review profile label at launch time. Stored on AgentJobInfo. */
|
||||
reviewProfileLabel?: string;
|
||||
} | null>;
|
||||
/** Called when a job completes successfully — parse results and push annotations. */
|
||||
onJobComplete?: (job: AgentJobInfo, meta: { outputPath?: string; stdout?: string; cwd?: string }) => void | Promise<void>;
|
||||
@@ -118,15 +122,16 @@ export function createAgentJobHandler(options: AgentJobHandlerOptions) {
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
// --- Process lifecycle ---
|
||||
function spawnJob(
|
||||
id: string,
|
||||
provider: string,
|
||||
command: string[],
|
||||
label: string,
|
||||
outputPath?: string,
|
||||
spawnOptions?: { captureStdout?: boolean; stdinPrompt?: string; cwd?: string; prompt?: string; engine?: string; model?: string; effort?: string; reasoningEffort?: string; fastMode?: boolean; prUrl?: string; diffScope?: string; diffContext?: AgentJobInfo["diffContext"] },
|
||||
spawnOptions?: { captureStdout?: boolean; stdinPrompt?: string; cwd?: string; prompt?: string; engine?: string; model?: string; effort?: string; reasoningEffort?: string; fastMode?: boolean; prUrl?: string; diffScope?: string; diffContext?: AgentJobInfo["diffContext"]; reviewProfileId?: string; reviewProfileLabel?: string },
|
||||
): AgentJobInfo {
|
||||
const id = crypto.randomUUID();
|
||||
const source = jobSource(id);
|
||||
|
||||
const info: AgentJobInfo = {
|
||||
@@ -146,6 +151,8 @@ export function createAgentJobHandler(options: AgentJobHandlerOptions) {
|
||||
...(spawnOptions?.prUrl && { prUrl: spawnOptions.prUrl }),
|
||||
...(spawnOptions?.diffScope && { diffScope: spawnOptions.diffScope }),
|
||||
...(spawnOptions?.diffContext && { diffContext: spawnOptions.diffContext }),
|
||||
...(spawnOptions?.reviewProfileId && { reviewProfileId: spawnOptions.reviewProfileId }),
|
||||
...(spawnOptions?.reviewProfileLabel && { reviewProfileLabel: spawnOptions.reviewProfileLabel }),
|
||||
};
|
||||
|
||||
let proc: ChildProcess | null = null;
|
||||
@@ -271,7 +278,6 @@ export function createAgentJobHandler(options: AgentJobHandlerOptions) {
|
||||
}
|
||||
jobOutputPaths.delete(id);
|
||||
jobOutputPaths.delete(`${id}:cwd`);
|
||||
|
||||
broadcast({ type: "job:completed", job: { ...entry.info } });
|
||||
});
|
||||
|
||||
@@ -405,6 +411,22 @@ export function createAgentJobHandler(options: AgentJobHandlerOptions) {
|
||||
if (url.pathname === JOBS && req.method === "POST") {
|
||||
try {
|
||||
const body = await parseBody(req);
|
||||
|
||||
// Reject unknown fields rather than silently ignoring them (per the
|
||||
// custom-reviews spec — a typo'd field should fail loud, not no-op).
|
||||
const KNOWN_JOB_FIELDS = new Set([
|
||||
"provider", "command", "label",
|
||||
"engine", "model", "reasoningEffort", "effort", "fastMode",
|
||||
"reviewProfileId",
|
||||
]);
|
||||
if (body && typeof body === "object") {
|
||||
const unknown = Object.keys(body).filter((k) => !KNOWN_JOB_FIELDS.has(k));
|
||||
if (unknown.length > 0) {
|
||||
json(res, { error: `Unknown field(s): ${unknown.join(", ")}` }, 400);
|
||||
return true;
|
||||
}
|
||||
}
|
||||
|
||||
const provider = typeof body.provider === "string" ? body.provider : "";
|
||||
let rawCommand = Array.isArray(body.command) ? body.command : [];
|
||||
let command = rawCommand.filter((c: unknown): c is string => typeof c === "string");
|
||||
@@ -431,6 +453,9 @@ export function createAgentJobHandler(options: AgentJobHandlerOptions) {
|
||||
let jobPrUrl: string | undefined;
|
||||
let jobDiffScope: string | undefined;
|
||||
let jobDiffContext: AgentJobInfo["diffContext"] | undefined;
|
||||
let jobReviewProfileId: string | undefined;
|
||||
let jobReviewProfileLabel: string | undefined;
|
||||
const jobId = crypto.randomUUID();
|
||||
if (options.buildCommand) {
|
||||
// Thread config from POST body to buildCommand
|
||||
const config: Record<string, unknown> = {};
|
||||
@@ -439,6 +464,7 @@ export function createAgentJobHandler(options: AgentJobHandlerOptions) {
|
||||
if (typeof body.reasoningEffort === "string") config.reasoningEffort = body.reasoningEffort;
|
||||
if (typeof body.effort === "string") config.effort = body.effort;
|
||||
if (body.fastMode === true) config.fastMode = true;
|
||||
if (typeof body.reviewProfileId === "string") config.reviewProfileId = body.reviewProfileId;
|
||||
const built = await options.buildCommand(provider, Object.keys(config).length > 0 ? config : undefined);
|
||||
if (built) {
|
||||
command = built.command;
|
||||
@@ -456,6 +482,8 @@ export function createAgentJobHandler(options: AgentJobHandlerOptions) {
|
||||
jobPrUrl = built.prUrl;
|
||||
jobDiffScope = built.diffScope;
|
||||
jobDiffContext = built.diffContext;
|
||||
jobReviewProfileId = built.reviewProfileId;
|
||||
jobReviewProfileLabel = built.reviewProfileLabel;
|
||||
}
|
||||
}
|
||||
|
||||
@@ -464,7 +492,7 @@ export function createAgentJobHandler(options: AgentJobHandlerOptions) {
|
||||
return true;
|
||||
}
|
||||
|
||||
const job = spawnJob(provider, command, label, outputPath, {
|
||||
const job = spawnJob(jobId, provider, command, label, outputPath, {
|
||||
captureStdout,
|
||||
stdinPrompt,
|
||||
cwd: spawnCwd,
|
||||
@@ -477,6 +505,8 @@ export function createAgentJobHandler(options: AgentJobHandlerOptions) {
|
||||
prUrl: jobPrUrl,
|
||||
diffScope: jobDiffScope,
|
||||
diffContext: jobDiffContext,
|
||||
reviewProfileId: jobReviewProfileId,
|
||||
reviewProfileLabel: jobReviewProfileLabel,
|
||||
});
|
||||
json(res, { job }, 201);
|
||||
} catch (err) {
|
||||
|
||||
@@ -46,7 +46,7 @@ import type { WorktreePool } from "../generated/worktree-pool.js";
|
||||
|
||||
import { createEditorAnnotationHandler } from "./annotations.js";
|
||||
import { createAgentJobHandler } from "./agent-jobs.js";
|
||||
import type { AgentJobInfo } from "../generated/agent-jobs.js";
|
||||
import { type AgentJobInfo, REVIEW_OUTPUT_FAILED, markJobReviewFailed } from "../generated/agent-jobs.js";
|
||||
import { createExternalAnnotationHandler } from "./external-annotations.js";
|
||||
import {
|
||||
handleDraftRequest,
|
||||
@@ -76,7 +76,7 @@ import {
|
||||
} from "./pr.js";
|
||||
import { getRepoInfo } from "./project.js";
|
||||
import {
|
||||
CODEX_REVIEW_SYSTEM_PROMPT,
|
||||
composeCodexReviewPrompt,
|
||||
buildCodexCommand,
|
||||
generateOutputPath,
|
||||
parseCodexOutput,
|
||||
@@ -84,7 +84,7 @@ import {
|
||||
} from "../generated/codex-review.js";
|
||||
import { buildAgentReviewUserMessage, buildAgentReviewUserMessageForTarget, type WorkspaceReviewPromptContext } from "../generated/agent-review-message.js";
|
||||
import {
|
||||
CLAUDE_REVIEW_PROMPT,
|
||||
composeClaudeReviewPrompt,
|
||||
buildClaudeCommand,
|
||||
parseClaudeStreamOutput,
|
||||
transformClaudeFindings,
|
||||
@@ -111,6 +111,11 @@ import {
|
||||
SemanticDiffResponseCache,
|
||||
} from "../generated/semantic-diff.js";
|
||||
import type { SemanticDiffAvailability, SemanticDiffResponse } from "../generated/semantic-diff-types.js";
|
||||
import { discoverCuratedSkills, resolveRequestedReviewProfile, listAllSkills, enableReviewSkill } from "../generated/review-skill-loader.js";
|
||||
import {
|
||||
BUILTIN_DEFAULT_PROFILE,
|
||||
type ReviewProfilesResponse,
|
||||
} from "../generated/review-profiles.js";
|
||||
import {
|
||||
canStageFiles,
|
||||
detectRemoteDefaultCompareTarget,
|
||||
@@ -155,6 +160,9 @@ const piCodeNavRuntime: CodeNavRuntime = {
|
||||
},
|
||||
};
|
||||
|
||||
// Review ingestion completion semantics (REVIEW_OUTPUT_FAILED,
|
||||
// markJobReviewFailed) now live in the shared agent-jobs module.
|
||||
|
||||
/** Detect if running inside WSL (Windows Subsystem for Linux) */
|
||||
function detectWSL(): boolean {
|
||||
if (process.platform !== "linux") return false;
|
||||
@@ -518,6 +526,15 @@ export async function startReviewServer(options: {
|
||||
: null;
|
||||
const launchPrUrl = prMeta?.url;
|
||||
const launchDiffScope = isPRMode ? currentPRDiffScope : undefined;
|
||||
|
||||
const requestedProfileId =
|
||||
typeof config?.reviewProfileId === "string" ? config.reviewProfileId : undefined;
|
||||
// Resolve the requested review, or throw a clear error. An unresolvable
|
||||
// non-default id (renamed/removed skill, stale cookie, malformed request)
|
||||
// never silently downgrades to the default — explicit selection is
|
||||
// authoritative at this boundary.
|
||||
const reviewProfile = resolveRequestedReviewProfile(requestedProfileId);
|
||||
|
||||
const diffContext: AgentJobInfo["diffContext"] | undefined = workspacePrompt
|
||||
? { mode: String(currentDiffType), worktreePath: null }
|
||||
: prMeta
|
||||
@@ -537,16 +554,20 @@ export async function startReviewServer(options: {
|
||||
prMetadata: prMeta,
|
||||
config,
|
||||
});
|
||||
return built ? { ...built, prUrl: launchPrUrl, diffScope: launchDiffScope, diffContext } : built;
|
||||
return built ? { ...built, prUrl: launchPrUrl, diffScope: launchDiffScope, diffContext, reviewProfileId: reviewProfile.id, reviewProfileLabel: reviewProfile.label } : built;
|
||||
}
|
||||
|
||||
// A custom review skill carries its own instructions and becomes the whole
|
||||
// prompt; strip the default framing prose from the user message so only the
|
||||
// git/PR context remains. The default review keeps today's message verbatim.
|
||||
const isCustomReview = reviewProfile.source === "user";
|
||||
const userMessage = workspacePrompt
|
||||
? buildAgentReviewUserMessageForTarget({
|
||||
kind: "workspace",
|
||||
patch: currentPatch,
|
||||
workspace: workspacePrompt,
|
||||
})
|
||||
: buildAgentReviewUserMessage(currentPatch, currentDiffType as DiffType, userMessageOptions, prMeta);
|
||||
}, isCustomReview)
|
||||
: buildAgentReviewUserMessage(currentPatch, currentDiffType as DiffType, userMessageOptions, prMeta, isCustomReview);
|
||||
const jobLabel = workspacePrompt ? "Workspace Review" : "Code Review";
|
||||
|
||||
if (provider === "codex") {
|
||||
@@ -554,17 +575,17 @@ export async function startReviewServer(options: {
|
||||
const reasoningEffort = typeof config?.reasoningEffort === "string" && config.reasoningEffort ? config.reasoningEffort : undefined;
|
||||
const fastMode = config?.fastMode === true;
|
||||
const outputPath = generateOutputPath();
|
||||
const prompt = CODEX_REVIEW_SYSTEM_PROMPT + "\n\n---\n\n" + userMessage;
|
||||
const prompt = composeCodexReviewPrompt(userMessage, reviewProfile);
|
||||
const command = await buildCodexCommand({ cwd, outputPath, prompt, model, reasoningEffort, fastMode });
|
||||
return { command, outputPath, prompt, cwd, label: jobLabel, model, reasoningEffort, fastMode: fastMode || undefined, prUrl: launchPrUrl, diffScope: launchDiffScope, diffContext };
|
||||
return { command, outputPath, prompt, cwd, label: jobLabel, model, reasoningEffort, fastMode: fastMode || undefined, prUrl: launchPrUrl, diffScope: launchDiffScope, diffContext, reviewProfileId: reviewProfile.id, reviewProfileLabel: reviewProfile.label };
|
||||
}
|
||||
|
||||
if (provider === "claude") {
|
||||
const model = typeof config?.model === "string" && config.model ? config.model : undefined;
|
||||
const effort = typeof config?.effort === "string" && config.effort ? config.effort : undefined;
|
||||
const prompt = CLAUDE_REVIEW_PROMPT + "\n\n---\n\n" + userMessage;
|
||||
const prompt = composeClaudeReviewPrompt(userMessage, reviewProfile);
|
||||
const { command, stdinPrompt } = buildClaudeCommand(prompt, model, effort);
|
||||
return { command, stdinPrompt, prompt, cwd, label: jobLabel, captureStdout: true, model, effort, prUrl: launchPrUrl, diffScope: launchDiffScope, diffContext };
|
||||
return { command, stdinPrompt, prompt, cwd, label: jobLabel, captureStdout: true, model, effort, prUrl: launchPrUrl, diffScope: launchDiffScope, diffContext, reviewProfileId: reviewProfile.id, reviewProfileLabel: reviewProfile.label };
|
||||
}
|
||||
|
||||
return null;
|
||||
@@ -582,9 +603,33 @@ export async function startReviewServer(options: {
|
||||
prRepo: getDisplayRepo(jobPrMeta),
|
||||
} : jobPrUrl ? { prUrl: jobPrUrl } : {};
|
||||
|
||||
if (job.provider === "codex" && meta.outputPath) {
|
||||
const output = await parseCodexOutput(meta.outputPath);
|
||||
if (!output) return;
|
||||
// Only tag annotations with a *custom* profile — the default review needs no tag.
|
||||
const profileLabel =
|
||||
job.reviewProfileId && job.reviewProfileId !== BUILTIN_DEFAULT_PROFILE.id
|
||||
? job.reviewProfileLabel
|
||||
: undefined;
|
||||
|
||||
// Map findings onto annotations and ingest. Shared by both engine branches;
|
||||
// no-ops on an empty set so a clean (zero-finding) review stays "done".
|
||||
const ingest = <T extends object>(transformed: readonly T[], logTag: string) => {
|
||||
if (transformed.length === 0) return;
|
||||
const annotations = transformed.map((a) => ({
|
||||
...a,
|
||||
...jobPrContext,
|
||||
...(jobDiffScope && { diffScope: jobDiffScope }),
|
||||
...(profileLabel && { reviewProfileLabel: profileLabel }),
|
||||
}));
|
||||
const result = externalAnnotations.addAnnotations({ annotations });
|
||||
if ("error" in result) console.error(`[${logTag}] addAnnotations error:`, result.error);
|
||||
};
|
||||
|
||||
if (job.provider === "codex") {
|
||||
const output = meta.outputPath ? await parseCodexOutput(meta.outputPath) : null;
|
||||
if (!output) {
|
||||
// Process exited 0 but output is missing/unparseable — not a green run.
|
||||
markJobReviewFailed(job, REVIEW_OUTPUT_FAILED);
|
||||
return;
|
||||
}
|
||||
|
||||
const hasBlockingFindings = output.findings.some(f => f.priority !== null && f.priority <= 1);
|
||||
job.summary = {
|
||||
@@ -593,46 +638,47 @@ export async function startReviewServer(options: {
|
||||
confidence: output.overall_confidence_score,
|
||||
};
|
||||
|
||||
if (output.findings.length > 0) {
|
||||
const annotations = transformReviewFindings(
|
||||
ingest(
|
||||
transformReviewFindings(
|
||||
output.findings,
|
||||
job.source,
|
||||
cwd,
|
||||
"Codex",
|
||||
workspace ? (filePath) => workspace.normalizeAnnotationPath(filePath) : undefined,
|
||||
)
|
||||
.map(a => ({ ...a, ...jobPrContext, ...(jobDiffScope && { diffScope: jobDiffScope }) }));
|
||||
const result = externalAnnotations.addAnnotations({ annotations });
|
||||
if ("error" in result) console.error(`[codex-review] addAnnotations error:`, result.error);
|
||||
}
|
||||
),
|
||||
"codex-review",
|
||||
);
|
||||
return;
|
||||
}
|
||||
|
||||
if (job.provider === "claude" && meta.stdout) {
|
||||
const output = parseClaudeStreamOutput(meta.stdout);
|
||||
if (job.provider === "claude") {
|
||||
const stdout = meta.stdout ?? "";
|
||||
const output = parseClaudeStreamOutput(stdout);
|
||||
if (!output) {
|
||||
console.error(`[claude-review] Failed to parse output (${meta.stdout.length} bytes, last 200: ${meta.stdout.slice(-200)})`);
|
||||
console.error(`[claude-review] Failed to parse output (${stdout.length} bytes, last 200: ${stdout.slice(-200)})`);
|
||||
markJobReviewFailed(job, REVIEW_OUTPUT_FAILED);
|
||||
return;
|
||||
}
|
||||
|
||||
const total = output.summary.important + output.summary.nit + output.summary.pre_existing;
|
||||
// Recompute the verdict from the findings we actually render. Nothing is
|
||||
// dropped now (un-pinnable findings become file/general comments), so the
|
||||
// count reflects reality and the card can never claim more than it shows.
|
||||
const transformed = transformClaudeFindings(
|
||||
output.findings,
|
||||
job.source,
|
||||
cwd,
|
||||
workspace ? (filePath) => workspace.normalizeAnnotationPath(filePath) : undefined,
|
||||
);
|
||||
const counts = { important: 0, nit: 0, pre_existing: 0 };
|
||||
for (const a of transformed) counts[a.severity]++;
|
||||
const total = counts.important + counts.nit + counts.pre_existing;
|
||||
job.summary = {
|
||||
correctness: output.summary.important === 0 ? "Correct" : "Issues Found",
|
||||
explanation: `${output.summary.important} important, ${output.summary.nit} nit, ${output.summary.pre_existing} pre-existing`,
|
||||
confidence: total === 0 ? 1.0 : Math.max(0, 1.0 - (output.summary.important * 0.2)),
|
||||
correctness: counts.important === 0 ? "Correct" : "Issues Found",
|
||||
explanation: `${counts.important} important, ${counts.nit} nit, ${counts.pre_existing} pre-existing`,
|
||||
confidence: total === 0 ? 1.0 : Math.max(0, 1.0 - (counts.important * 0.2)),
|
||||
};
|
||||
|
||||
if (output.findings.length > 0) {
|
||||
const annotations = transformClaudeFindings(
|
||||
output.findings,
|
||||
job.source,
|
||||
cwd,
|
||||
workspace ? (filePath) => workspace.normalizeAnnotationPath(filePath) : undefined,
|
||||
)
|
||||
.map(a => ({ ...a, ...jobPrContext, ...(jobDiffScope && { diffScope: jobDiffScope }) }));
|
||||
const result = externalAnnotations.addAnnotations({ annotations });
|
||||
if ("error" in result) console.error(`[claude-review] addAnnotations error:`, result.error);
|
||||
}
|
||||
ingest(transformed, "claude-review");
|
||||
return;
|
||||
}
|
||||
|
||||
@@ -1343,6 +1389,53 @@ export async function startReviewServer(options: {
|
||||
await handleUploadRequest(req, res);
|
||||
} else if (url.pathname === "/api/agents" && req.method === "GET") {
|
||||
json(res, { agents: [] });
|
||||
} else if (
|
||||
url.pathname === "/api/agents/review-profiles" &&
|
||||
req.method === "GET"
|
||||
) {
|
||||
// Custom reviews discovery. Reloaded per request, no file watching.
|
||||
// Catalog only — directory listing, no SKILL.md bodies read here.
|
||||
// Bodies are read at launch, for the one selected skill.
|
||||
const body: ReviewProfilesResponse = {
|
||||
profiles: [
|
||||
{
|
||||
id: BUILTIN_DEFAULT_PROFILE.id,
|
||||
label: BUILTIN_DEFAULT_PROFILE.label,
|
||||
source: BUILTIN_DEFAULT_PROFILE.source,
|
||||
default: BUILTIN_DEFAULT_PROFILE.default,
|
||||
},
|
||||
...discoverCuratedSkills().map((s) => ({
|
||||
id: `skill:${s.name}`,
|
||||
label: s.name,
|
||||
source: "user" as const,
|
||||
sourcePath: s.sourcePath,
|
||||
})),
|
||||
],
|
||||
};
|
||||
json(res, body);
|
||||
} else if (url.pathname === "/api/agents/skills" && req.method === "GET") {
|
||||
// All discovered skills for the "add a review" picker, each flagged
|
||||
// with whether it is already enabled.
|
||||
json(res, { skills: listAllSkills() });
|
||||
} else if (url.pathname === "/api/agents/review-skills" && req.method === "POST") {
|
||||
// Enable a skill as a review (curation write to review-skills.json).
|
||||
let name: unknown;
|
||||
try {
|
||||
const body = await parseBody(req);
|
||||
name = body.name;
|
||||
} catch {
|
||||
json(res, { error: "Invalid JSON" }, 400);
|
||||
return;
|
||||
}
|
||||
if (typeof name !== "string" || name.length === 0) {
|
||||
json(res, { error: "`name` is required." }, 400);
|
||||
return;
|
||||
}
|
||||
try {
|
||||
json(res, enableReviewSkill(name));
|
||||
} catch (err) {
|
||||
json(res, { error: err instanceof Error ? err.message : "Could not enable review." }, 400);
|
||||
}
|
||||
} else if (url.pathname === "/api/git-add" && req.method === "POST") {
|
||||
try {
|
||||
const body = await parseBody(req);
|
||||
|
||||
@@ -7,19 +7,22 @@ cd "$(dirname "$0")"
|
||||
rm -rf generated
|
||||
mkdir -p generated generated/ai/providers
|
||||
|
||||
for f in feedback-templates prompts review-core diff-paths cli-pagination jj-core vcs-core review-args storage draft project pr-types pr-provider pr-stack pr-github pr-gitlab checklist integrations-common repo reference-common favicon code-file resolve-file annotate-reference-roots-node config external-annotation agent-jobs agent-terminal worktree worktree-pool html-to-markdown html-assets html-assets-node url-to-markdown tour annotate-args at-reference review-workspace-node review-workspace pfm-reminder improvement-hooks code-nav data-dir semantic-diff-types semantic-diff source-save source-save-node workspace-status open-in-apps; do
|
||||
for f in feedback-templates prompts review-core diff-paths cli-pagination jj-core vcs-core review-args storage draft project pr-types pr-provider pr-stack pr-github pr-gitlab checklist integrations-common repo reference-common favicon code-file resolve-file annotate-reference-roots-node config external-annotation agent-jobs agent-terminal worktree worktree-pool html-to-markdown html-assets html-assets-node url-to-markdown tour annotate-args at-reference review-workspace-node review-workspace pfm-reminder improvement-hooks code-nav data-dir semantic-diff-types semantic-diff source-save source-save-node workspace-status open-in-apps review-profiles; do
|
||||
src="../../packages/shared/$f.ts"
|
||||
printf '// @generated — DO NOT EDIT. Source: packages/shared/%s.ts\n' "$f" | cat - "$src" > "generated/$f.ts"
|
||||
done
|
||||
|
||||
# Vendor review agent modules from packages/server/ — rewrite imports for generated/ layout
|
||||
for f in agent-review-message codex-review claude-review path-utils; do
|
||||
for f in agent-review-message codex-review claude-review path-utils review-skill-loader; do
|
||||
src="../../packages/server/$f.ts"
|
||||
printf '// @generated — DO NOT EDIT. Source: packages/server/%s.ts\n' "$f" | cat - "$src" \
|
||||
| sed 's|from "./vcs"|from "./review-core.js"|' \
|
||||
| sed 's|from "./pr"|from "./pr-provider.js"|' \
|
||||
| sed 's|from "./path-utils"|from "./path-utils.js"|' \
|
||||
| sed 's|from "./review-skill-loader"|from "./review-skill-loader.js"|' \
|
||||
| sed 's|from "@plannotator/shared/review-workspace"|from "./review-workspace.js"|' \
|
||||
| sed 's|from "@plannotator/shared/review-profiles"|from "./review-profiles.js"|' \
|
||||
| sed 's|from "@plannotator/shared/external-annotation"|from "./external-annotation.js"|' \
|
||||
| sed 's|from "@plannotator/shared/data-dir"|from "./data-dir"|' \
|
||||
> "generated/$f.ts"
|
||||
done
|
||||
|
||||
@@ -0,0 +1,47 @@
|
||||
# Custom Reviews
|
||||
|
||||
A custom review is an Agent Skill. Point Plannotator at a skill and it runs the
|
||||
review using that skill's instructions. The skill becomes the review. The
|
||||
default review instructions are not added, and findings come back the same way.
|
||||
|
||||
## Turn one on
|
||||
|
||||
You already keep skills in your global folders (`~/.claude/skills`,
|
||||
`~/.codex/skills`, `~/.agents/skills`). List the ones you want as reviews in
|
||||
`~/.plannotator/review-skills.json`:
|
||||
|
||||
```json
|
||||
{ "version": 1, "enabled": ["security-review", "api-contracts"] }
|
||||
```
|
||||
|
||||
The names are the skill folder names. Run a code review, pick the skill, run it.
|
||||
Findings come back the way they do today.
|
||||
|
||||
No `review-skills.json`? You get the default review, unchanged.
|
||||
|
||||
## How it works
|
||||
|
||||
Plannotator reads the skill's `SKILL.md` body at launch and uses it as the
|
||||
review prompt. The skill defines the review; the default review prompt is
|
||||
dropped, and the user message is trimmed to the git or PR context the agent
|
||||
needs to find the changes. The read is live. Edit the skill the normal way and
|
||||
the next review picks it up. Nothing is copied.
|
||||
|
||||
Some skills carry `references/`, `scripts/`, or `assets/`. For those, Plannotator
|
||||
tells the agent where the skill folder is, and the agent opens those files on
|
||||
demand from where they already live.
|
||||
|
||||
## What counts as a skill
|
||||
|
||||
The name is the folder name. The instructions are the `SKILL.md` body with the
|
||||
leading frontmatter block stripped off. Plannotator does not read the
|
||||
frontmatter: no `name`, no `description`, no YAML.
|
||||
|
||||
Global skills only. A skill checked into a repo is ignored. (A pull request from
|
||||
a fork could otherwise drop instructions straight into your reviewer.)
|
||||
|
||||
## Coming from JSON profiles
|
||||
|
||||
The old `~/.plannotator/reviews/*.json` profiles are gone. To move one over: put
|
||||
its instructions text in a `SKILL.md` under a global skill folder, then add that
|
||||
skill's name to `~/.plannotator/review-skills.json`.
|
||||
@@ -81,6 +81,7 @@ import {
|
||||
REVIEW_PR_SUMMARY_PANEL_ID,
|
||||
REVIEW_PR_COMMENTS_PANEL_ID,
|
||||
REVIEW_PR_CHECKS_PANEL_ID,
|
||||
REVIEW_SEMANTIC_DIFF_PANEL_ID,
|
||||
REVIEW_ALL_FILES_PANEL_ID,
|
||||
REVIEW_CODE_NAV_PANEL_ID,
|
||||
} from './dock/reviewPanelTypes';
|
||||
@@ -126,6 +127,7 @@ const ReviewApp: React.FC = () => {
|
||||
// at call time instead of a stale closure capture.
|
||||
const isAllFilesActiveRef = useRef(isAllFilesActive);
|
||||
isAllFilesActiveRef.current = isAllFilesActive;
|
||||
const [isSemanticDiffActive, setIsSemanticDiffActive] = useState(false);
|
||||
const [semanticDiffAvailable, setSemanticDiffAvailable] = useState(false);
|
||||
const [isDiffPanelActive, setIsDiffPanelActive] = useState(false);
|
||||
const [allFilesVisibleFile, setAllFilesVisibleFile] = useState<string | null>(null);
|
||||
@@ -292,6 +294,7 @@ const ReviewApp: React.FC = () => {
|
||||
const filesRef = useRef(files);
|
||||
filesRef.current = files;
|
||||
const needsInitialDiffPanel = useRef(true);
|
||||
const semanticDiffAutoFallbackPending = useRef(false);
|
||||
|
||||
// PR context (lifted from sidebar so center dock PR panels can access it)
|
||||
const { prContext, isLoading: isPRContextLoading, error: prContextError, fetchContext: fetchPRContext } = usePRContext(prMetadata ?? null);
|
||||
@@ -301,6 +304,7 @@ const ReviewApp: React.FC = () => {
|
||||
const openDiffFile = useCallback((filePath: string) => {
|
||||
const file = files.find(candidate => candidate.path === filePath);
|
||||
if (!file) return;
|
||||
semanticDiffAutoFallbackPending.current = false;
|
||||
|
||||
if (!dockApi) {
|
||||
const fileIndex = files.findIndex(candidate => candidate.path === filePath);
|
||||
@@ -482,7 +486,9 @@ const ReviewApp: React.FC = () => {
|
||||
existing.api.setTitle(`References: ${request.symbol}`);
|
||||
existing.api.setActive();
|
||||
} else {
|
||||
const refPanel = isAllFilesActive
|
||||
const refPanel = isSemanticDiffActive
|
||||
? REVIEW_SEMANTIC_DIFF_PANEL_ID
|
||||
: isAllFilesActive
|
||||
? REVIEW_ALL_FILES_PANEL_ID
|
||||
: REVIEW_DIFF_PANEL_ID;
|
||||
dockApi.addPanel({
|
||||
@@ -493,7 +499,7 @@ const ReviewApp: React.FC = () => {
|
||||
initialHeight: 250,
|
||||
});
|
||||
}
|
||||
}, [codeNav.resolve, dockApi, isAllFilesActive, gitContext, agentCwd]);
|
||||
}, [codeNav.resolve, dockApi, isAllFilesActive, isSemanticDiffActive, gitContext, agentCwd]);
|
||||
|
||||
// Check AI capabilities on mount
|
||||
useEffect(() => {
|
||||
@@ -653,10 +659,12 @@ const ReviewApp: React.FC = () => {
|
||||
event.api.onDidActivePanelChange((panel) => {
|
||||
if (!panel) {
|
||||
setIsAllFilesActive(false);
|
||||
setIsSemanticDiffActive(false);
|
||||
setIsDiffPanelActive(false);
|
||||
return;
|
||||
}
|
||||
setIsAllFilesActive(panel.id === REVIEW_ALL_FILES_PANEL_ID);
|
||||
setIsSemanticDiffActive(panel.id === REVIEW_SEMANTIC_DIFF_PANEL_ID);
|
||||
setIsDiffPanelActive(isReviewDiffPanelId(panel.id));
|
||||
if (!isReviewDiffPanelId(panel.id)) return;
|
||||
const filePath = getReviewDiffPanelFilePath(panel.params);
|
||||
@@ -677,6 +685,7 @@ const ReviewApp: React.FC = () => {
|
||||
: undefined;
|
||||
const hideHeaders =
|
||||
lonePanel?.id === REVIEW_DIFF_PANEL_ID ||
|
||||
lonePanel?.id === REVIEW_SEMANTIC_DIFF_PANEL_ID ||
|
||||
lonePanel?.id === REVIEW_ALL_FILES_PANEL_ID;
|
||||
for (const group of event.api.groups) {
|
||||
group.header.hidden = hideHeaders;
|
||||
@@ -766,6 +775,7 @@ const ReviewApp: React.FC = () => {
|
||||
|
||||
const openAllFilesPanel = useCallback(() => {
|
||||
if (!dockApi) return;
|
||||
semanticDiffAutoFallbackPending.current = false;
|
||||
const existing = dockApi.getPanel(REVIEW_ALL_FILES_PANEL_ID);
|
||||
if (existing) { existing.api.setActive(); return; }
|
||||
dockApi.addPanel({
|
||||
@@ -775,10 +785,56 @@ const ReviewApp: React.FC = () => {
|
||||
});
|
||||
}, [dockApi]);
|
||||
|
||||
const openSemanticDiffPanel = useCallback((options?: { autoFallbackOnError?: boolean }) => {
|
||||
if (!dockApi) return;
|
||||
semanticDiffAutoFallbackPending.current = options?.autoFallbackOnError === true;
|
||||
if (!semanticDiffAvailable) {
|
||||
openAllFilesPanel();
|
||||
return;
|
||||
}
|
||||
const existing = dockApi.getPanel(REVIEW_SEMANTIC_DIFF_PANEL_ID);
|
||||
if (existing) { existing.api.setActive(); return; }
|
||||
dockApi.addPanel({
|
||||
id: REVIEW_SEMANTIC_DIFF_PANEL_ID,
|
||||
component: REVIEW_PANEL_TYPES.SEMANTIC_DIFF,
|
||||
title: 'Semantic diff',
|
||||
});
|
||||
}, [dockApi, openAllFilesPanel, semanticDiffAvailable]);
|
||||
|
||||
const handleSemanticDiffUnavailable = useCallback(() => {
|
||||
semanticDiffAutoFallbackPending.current = false;
|
||||
setSemanticDiffAvailable(false);
|
||||
dockApi?.getPanel(REVIEW_SEMANTIC_DIFF_PANEL_ID)?.api.close();
|
||||
openAllFilesPanel();
|
||||
}, [dockApi, openAllFilesPanel]);
|
||||
|
||||
const handleSemanticDiffLoadSuccess = useCallback(() => {
|
||||
semanticDiffAutoFallbackPending.current = false;
|
||||
}, []);
|
||||
|
||||
const handleSemanticDiffLoadError = useCallback(() => {
|
||||
if (!semanticDiffAutoFallbackPending.current) return false;
|
||||
if (dockApi?.activePanel?.id !== REVIEW_SEMANTIC_DIFF_PANEL_ID) {
|
||||
// The user has already moved on; don't steal focus by auto-opening All files.
|
||||
semanticDiffAutoFallbackPending.current = false;
|
||||
return false;
|
||||
}
|
||||
semanticDiffAutoFallbackPending.current = false;
|
||||
dockApi?.getPanel(REVIEW_SEMANTIC_DIFF_PANEL_ID)?.api.close();
|
||||
openAllFilesPanel();
|
||||
return true;
|
||||
}, [dockApi, openAllFilesPanel]);
|
||||
|
||||
const applySemanticDiffAdvert = useCallback((semanticDiff?: SemanticDiffAdvert) => {
|
||||
if (!semanticDiff) return;
|
||||
setSemanticDiffAvailable(semanticDiff.available === true);
|
||||
}, []);
|
||||
const available = semanticDiff.available === true;
|
||||
setSemanticDiffAvailable(available);
|
||||
if (!available) {
|
||||
semanticDiffAutoFallbackPending.current = false;
|
||||
dockApi?.getPanel(REVIEW_SEMANTIC_DIFF_PANEL_ID)?.api.close();
|
||||
if (isSemanticDiffActive) openAllFilesPanel();
|
||||
}
|
||||
}, [dockApi, isSemanticDiffActive, openAllFilesPanel]);
|
||||
|
||||
// Open the All files overview on first load. Semantic diff stays available via
|
||||
// the file-tree nav entry, but it's no longer the default landing view.
|
||||
@@ -1583,7 +1639,11 @@ const ReviewApp: React.FC = () => {
|
||||
openDiffFile,
|
||||
onAllFilesVisibleFileChange: setAllFilesVisibleFile,
|
||||
isAllFilesActive,
|
||||
isSemanticDiffActive,
|
||||
semanticDiffAvailable,
|
||||
onSemanticDiffUnavailable: handleSemanticDiffUnavailable,
|
||||
onSemanticDiffLoadError: handleSemanticDiffLoadError,
|
||||
onSemanticDiffLoadSuccess: handleSemanticDiffLoadSuccess,
|
||||
openTourPanel: handleOpenTour,
|
||||
onCodeNavRequest: handleCodeNavRequest,
|
||||
codeNavResult: codeNav.result,
|
||||
@@ -1604,7 +1664,8 @@ const ReviewApp: React.FC = () => {
|
||||
handleAskAI, handleAskAIForFile, handleViewAIResponse, handleClickAIMarker,
|
||||
aiHistoryForSelection, getAIHistoryForFile, agentJobs.jobs, prMetadata, prContext,
|
||||
isPRContextLoading, prContextError, fetchPRContext, platformUser, openDiffFile,
|
||||
handleOpenTour, isAllFilesActive, semanticDiffAvailable, handleAddAnnotationForFile,
|
||||
handleOpenTour, isAllFilesActive, isSemanticDiffActive, semanticDiffAvailable,
|
||||
handleSemanticDiffUnavailable, handleSemanticDiffLoadError, handleSemanticDiffLoadSuccess, handleAddAnnotationForFile,
|
||||
handleCodeNavRequest, codeNav.result, codeNav.isLoading, codeNav.activeSymbol,
|
||||
]);
|
||||
|
||||
@@ -2369,6 +2430,9 @@ const ReviewApp: React.FC = () => {
|
||||
<FileTree
|
||||
files={files}
|
||||
activeFileIndex={activeFileIndex}
|
||||
onSelectSemanticDiff={() => openSemanticDiffPanel()}
|
||||
isSemanticDiffActive={isSemanticDiffActive}
|
||||
semanticDiffAvailable={semanticDiffAvailable}
|
||||
onSelectAllFiles={openAllFilesPanel}
|
||||
isAllFilesActive={isAllFilesActive}
|
||||
scrollHighlightIndex={isAllFilesActive && allFilesVisibleFile ? files.findIndex(f => f.path === allFilesVisibleFile) : undefined}
|
||||
|
||||
@@ -264,12 +264,13 @@ export const FileHeader: React.FC<FileHeaderProps> = ({
|
||||
{/* File actions: open in app (when launchable), copy path, copy file
|
||||
diff. canOpen=false in PR review without a local checkout — those
|
||||
files aren't on disk — but copy actions remain. */}
|
||||
{/* Icon-only in the header (the picked app's name shows in the dropdown),
|
||||
matching the plan/annotate side. */}
|
||||
<OpenInAppButton
|
||||
filePath={filePath}
|
||||
base={state?.agentCwd ?? null}
|
||||
diffText={patch}
|
||||
canOpen={!(state?.prMetadata && !state?.agentCwd) && status !== 'deleted'}
|
||||
showLabel={!isCompact}
|
||||
/>
|
||||
</div>
|
||||
</div>
|
||||
|
||||
@@ -10,7 +10,6 @@ import { WorktreePicker } from './WorktreePicker';
|
||||
import { getReviewSearchSideLabel, type ReviewSearchFileGroup, type ReviewSearchMatch } from '../utils/reviewSearch';
|
||||
import type { DiffFile } from '../types';
|
||||
import { OverlayScrollArea } from '@plannotator/ui/components/OverlayScrollArea';
|
||||
import { SemanticDiffAccordion } from './SemanticDiffAccordion';
|
||||
|
||||
interface FileTreeProps {
|
||||
files: DiffFile[];
|
||||
@@ -61,6 +60,9 @@ interface FileTreeProps {
|
||||
activeSearchMatchId?: string | null;
|
||||
onSelectSearchMatch?: (matchId: string) => void;
|
||||
onStepSearchMatch?: (direction: 1 | -1) => void;
|
||||
onSelectSemanticDiff?: () => void;
|
||||
isSemanticDiffActive?: boolean;
|
||||
semanticDiffAvailable?: boolean;
|
||||
onSelectAllFiles?: () => void;
|
||||
isAllFilesActive?: boolean;
|
||||
scrollHighlightIndex?: number;
|
||||
@@ -113,6 +115,9 @@ export const FileTree: React.FC<FileTreeProps> = ({
|
||||
activeSearchMatchId,
|
||||
onSelectSearchMatch,
|
||||
onStepSearchMatch,
|
||||
onSelectSemanticDiff,
|
||||
isSemanticDiffActive = false,
|
||||
semanticDiffAvailable = false,
|
||||
onSelectAllFiles,
|
||||
isAllFilesActive = false,
|
||||
scrollHighlightIndex,
|
||||
@@ -451,6 +456,19 @@ export const FileTree: React.FC<FileTreeProps> = ({
|
||||
)
|
||||
) : (
|
||||
<>
|
||||
{semanticDiffAvailable && onSelectSemanticDiff && (
|
||||
<button
|
||||
onClick={onSelectSemanticDiff}
|
||||
className={`w-full flex items-center gap-2 px-2 py-1.5 rounded text-xs transition-colors mb-0.5 ${
|
||||
isSemanticDiffActive
|
||||
? 'bg-primary/15 text-primary font-medium'
|
||||
: 'text-muted-foreground hover:bg-muted hover:text-foreground'
|
||||
}`}
|
||||
>
|
||||
<span className="w-3.5 h-3.5 flex flex-shrink-0 items-center justify-center" aria-hidden="true">∆</span>
|
||||
<span>Semantic diff</span>
|
||||
</button>
|
||||
)}
|
||||
{onSelectAllFiles && (
|
||||
<button
|
||||
onClick={onSelectAllFiles}
|
||||
@@ -477,7 +495,7 @@ export const FileTree: React.FC<FileTreeProps> = ({
|
||||
node={node}
|
||||
expandedFolders={expandedFolders}
|
||||
onToggleFolder={handleToggleFolder}
|
||||
activeFileIndex={isAllFilesActive ? -1 : activeFileIndex}
|
||||
activeFileIndex={isAllFilesActive || isSemanticDiffActive ? -1 : activeFileIndex}
|
||||
scrollHighlightIndex={isAllFilesActive ? scrollHighlightIndex : undefined}
|
||||
onSelectFile={onSelectFile}
|
||||
onDoubleClickFile={onDoubleClickFile}
|
||||
@@ -494,8 +512,6 @@ export const FileTree: React.FC<FileTreeProps> = ({
|
||||
</div>
|
||||
</OverlayScrollArea>
|
||||
|
||||
<SemanticDiffAccordion />
|
||||
|
||||
{/* Footer */}
|
||||
<div className="px-2 py-1.5 border-t border-border/50 text-xs text-muted-foreground">
|
||||
<div className="flex items-center justify-between">
|
||||
|
||||
@@ -1,8 +1,9 @@
|
||||
import React, { useState } from 'react';
|
||||
import { CodeAnnotation, type EditorAnnotation } from '@plannotator/ui/types';
|
||||
import { CodeAnnotation, type CodeAnnotationScope, type EditorAnnotation } from '@plannotator/ui/types';
|
||||
import { isCurrentUser } from '@plannotator/ui/utils/identity';
|
||||
import { EditorAnnotationCard } from '@plannotator/ui/components/EditorAnnotationCard';
|
||||
import { CopyButton } from './CopyButton';
|
||||
import { copyLocationPrefix } from '../utils/annotationDisplay';
|
||||
import { ConventionalLabelBadge } from './ConventionalLabelPicker';
|
||||
import { HighlightedCode } from './HighlightedCode';
|
||||
import { detectLanguage } from '../utils/detectLanguage';
|
||||
@@ -52,7 +53,7 @@ interface ReviewSidebarProps {
|
||||
// Agent props
|
||||
agentJobs?: AgentJobInfo[];
|
||||
agentCapabilities?: AgentCapabilities | null;
|
||||
onAgentLaunch?: (params: { provider?: string; command?: string[]; label?: string; engine?: string; model?: string; reasoningEffort?: string; effort?: string; fastMode?: boolean }) => void;
|
||||
onAgentLaunch?: (params: { provider?: string; command?: string[]; label?: string; engine?: string; model?: string; reasoningEffort?: string; effort?: string; fastMode?: boolean; reviewProfileId?: string }) => void;
|
||||
onAgentKillJob?: (id: string) => void;
|
||||
onAgentKillAll?: () => void;
|
||||
externalAnnotations?: Array<{ source?: string }>;
|
||||
@@ -86,9 +87,9 @@ const SuggestionPreview: React.FC<{ code: string; originalCode?: string; languag
|
||||
);
|
||||
};
|
||||
|
||||
const FILE_SCOPE_FIRST = { file: 0, line: 1 } as const;
|
||||
const SCOPE_ORDER = { general: 0, file: 1, line: 2 } as const;
|
||||
|
||||
function getAnnotationScope(annotation: CodeAnnotation): 'line' | 'file' {
|
||||
function getAnnotationScope(annotation: CodeAnnotation): CodeAnnotationScope {
|
||||
return annotation.scope ?? 'line';
|
||||
}
|
||||
|
||||
@@ -97,12 +98,12 @@ function compareCodeAnnotations(a: CodeAnnotation, b: CodeAnnotation): number {
|
||||
const bScope = getAnnotationScope(b);
|
||||
|
||||
if (aScope !== bScope) {
|
||||
return FILE_SCOPE_FIRST[aScope] - FILE_SCOPE_FIRST[bScope];
|
||||
return SCOPE_ORDER[aScope] - SCOPE_ORDER[bScope];
|
||||
}
|
||||
|
||||
return aScope === 'file'
|
||||
? b.createdAt - a.createdAt
|
||||
: a.lineStart - b.lineStart;
|
||||
return aScope === 'line'
|
||||
? a.lineStart - b.lineStart
|
||||
: b.createdAt - a.createdAt;
|
||||
}
|
||||
|
||||
|
||||
@@ -157,13 +158,22 @@ export const ReviewSidebar: React.FC<ReviewSidebarProps> = /* React.memo */({
|
||||
}
|
||||
};
|
||||
|
||||
// Group annotations by file, optionally by PR first
|
||||
const { groupedAnnotations, prGroups, isMultiPR } = React.useMemo(() => {
|
||||
const prUrls = new Set(annotations.map(a => a.prUrl).filter(Boolean));
|
||||
// Split out general (review-level) comments — they belong to no file — then
|
||||
// group the rest by file, optionally by PR first.
|
||||
const { generalAnnotations, groupedAnnotations, prGroups, isMultiPR } = React.useMemo(() => {
|
||||
const general: CodeAnnotation[] = [];
|
||||
const placed: CodeAnnotation[] = [];
|
||||
for (const ann of annotations) {
|
||||
if ((ann.scope ?? 'line') === 'general') general.push(ann);
|
||||
else placed.push(ann);
|
||||
}
|
||||
general.sort((a, b) => b.createdAt - a.createdAt);
|
||||
|
||||
const prUrls = new Set(placed.map(a => a.prUrl).filter(Boolean));
|
||||
const multiPR = prUrls.size > 1;
|
||||
|
||||
const grouped = new Map<string, CodeAnnotation[]>();
|
||||
for (const ann of annotations) {
|
||||
for (const ann of placed) {
|
||||
const existing = grouped.get(ann.filePath) || [];
|
||||
existing.push(ann);
|
||||
grouped.set(ann.filePath, existing);
|
||||
@@ -175,7 +185,7 @@ export const ReviewSidebar: React.FC<ReviewSidebarProps> = /* React.memo */({
|
||||
let prs: Map<string, Map<string, CodeAnnotation[]>> | null = null;
|
||||
if (multiPR) {
|
||||
prs = new Map();
|
||||
for (const ann of annotations) {
|
||||
for (const ann of placed) {
|
||||
const prKey = ann.prUrl ?? '_none';
|
||||
if (!prs.has(prKey)) prs.set(prKey, new Map());
|
||||
const fileMap = prs.get(prKey)!;
|
||||
@@ -190,14 +200,16 @@ export const ReviewSidebar: React.FC<ReviewSidebarProps> = /* React.memo */({
|
||||
}
|
||||
}
|
||||
|
||||
return { groupedAnnotations: grouped, prGroups: prs, isMultiPR: multiPR };
|
||||
return { generalAnnotations: general, groupedAnnotations: grouped, prGroups: prs, isMultiPR: multiPR };
|
||||
}, [annotations]);
|
||||
|
||||
if (!isOpen) return null;
|
||||
|
||||
function renderAnnotationCard(annotation: CodeAnnotation) {
|
||||
const isSelected = selectedAnnotationId === annotation.id;
|
||||
const isFileScope = getAnnotationScope(annotation) === 'file';
|
||||
const scope = getAnnotationScope(annotation);
|
||||
const isFileScope = scope === 'file';
|
||||
const isGeneralScope = scope === 'general';
|
||||
return (
|
||||
<div
|
||||
key={annotation.id}
|
||||
@@ -210,7 +222,11 @@ export const ReviewSidebar: React.FC<ReviewSidebarProps> = /* React.memo */({
|
||||
>
|
||||
<div className="flex items-center justify-between mb-1.5">
|
||||
<div className="flex items-center gap-2">
|
||||
{isFileScope ? (
|
||||
{isGeneralScope ? (
|
||||
<span className="text-[9px] font-semibold uppercase tracking-wider px-1.5 py-0.5 rounded bg-primary/10 text-primary">
|
||||
general
|
||||
</span>
|
||||
) : isFileScope ? (
|
||||
<span className="text-[9px] font-semibold uppercase tracking-wider px-1.5 py-0.5 rounded bg-primary/10 text-primary">
|
||||
file
|
||||
</span>
|
||||
@@ -227,6 +243,11 @@ export const ReviewSidebar: React.FC<ReviewSidebarProps> = /* React.memo */({
|
||||
{annotation.conventionalLabel && (
|
||||
<ConventionalLabelBadge label={annotation.conventionalLabel} decorations={annotation.decorations} />
|
||||
)}
|
||||
{annotation.reviewProfileLabel && (
|
||||
<span className="text-[9px] uppercase tracking-wider px-1.5 py-0.5 rounded bg-accent/10 text-accent/90">
|
||||
{annotation.reviewProfileLabel}
|
||||
</span>
|
||||
)}
|
||||
{annotation.author && (
|
||||
<span className={`text-[10px] truncate max-w-[100px] ${isCurrentUser(annotation.author) ? 'text-muted-foreground/50' : 'text-muted-foreground/70'}`}>
|
||||
{annotation.author}{isCurrentUser(annotation.author) && ' (me)'}
|
||||
@@ -242,14 +263,14 @@ export const ReviewSidebar: React.FC<ReviewSidebarProps> = /* React.memo */({
|
||||
{renderInlineMarkdown(annotation.text)}
|
||||
</div>
|
||||
)}
|
||||
{annotation.suggestedCode && (
|
||||
{annotation.suggestedCode && !isGeneralScope && (
|
||||
<div className="mt-1.5">
|
||||
<SuggestionPreview code={annotation.suggestedCode} originalCode={annotation.originalCode} language={detectLanguage(annotation.filePath)} />
|
||||
</div>
|
||||
)}
|
||||
<div className="flex items-center justify-end gap-1 mt-1.5 opacity-0 group-hover:opacity-100 transition-opacity">
|
||||
{annotation.text && (
|
||||
<CopyButton text={`${annotation.filePath}:${annotation.lineStart}${annotation.lineEnd !== annotation.lineStart ? `-${annotation.lineEnd}` : ''}\n${annotation.text}${annotation.reasoning ? `\n\nReasoning: ${annotation.reasoning}` : ''}`} variant="inline" />
|
||||
<CopyButton text={`${copyLocationPrefix(annotation, scope)}${annotation.text}${annotation.reasoning ? `\n\nReasoning: ${annotation.reasoning}` : ''}`} variant="inline" />
|
||||
)}
|
||||
<button
|
||||
onClick={(e) => {
|
||||
@@ -312,6 +333,16 @@ export const ReviewSidebar: React.FC<ReviewSidebarProps> = /* React.memo */({
|
||||
</div>
|
||||
) : (
|
||||
<div className="p-2 space-y-4">
|
||||
{generalAnnotations.length > 0 && (
|
||||
<div>
|
||||
<div className="sticky top-0 z-10 bg-background/95 backdrop-blur-sm px-2 py-1 text-xs font-medium text-muted-foreground">
|
||||
General
|
||||
</div>
|
||||
<div className="space-y-1">
|
||||
{generalAnnotations.map((annotation) => renderAnnotationCard(annotation))}
|
||||
</div>
|
||||
</div>
|
||||
)}
|
||||
{isMultiPR && prGroups ? (
|
||||
Array.from(prGroups.entries()).map(([prUrl, fileMap]) => {
|
||||
const sample = fileMap.values().next().value?.[0];
|
||||
|
||||
@@ -81,12 +81,17 @@ function buildAnnotationFileComments(
|
||||
.filter(c => c.body.length > 0);
|
||||
}
|
||||
|
||||
// The review-level body: file-scoped comments (prefixed with their path) plus
|
||||
// general (review-wide) comments, which belong to no file. Both ride here so
|
||||
// neither is dropped from a PR submission.
|
||||
function buildFileScopedBody(annotations: CodeAnnotation[]): string {
|
||||
return annotations
|
||||
.filter(a => a.scope === 'file')
|
||||
.map(a => a.text ? `**${a.filePath}:** ${a.text}` : '')
|
||||
.filter(Boolean)
|
||||
.join('\n\n');
|
||||
const parts: string[] = [];
|
||||
for (const a of annotations) {
|
||||
const scope = a.scope ?? 'line';
|
||||
if (scope === 'file' && a.text) parts.push(`**${a.filePath}:** ${a.text}`);
|
||||
else if (scope === 'general' && a.text) parts.push(a.text);
|
||||
}
|
||||
return parts.join('\n\n');
|
||||
}
|
||||
|
||||
export function buildReviewSubmission(
|
||||
@@ -154,7 +159,9 @@ export function buildReviewSubmission(
|
||||
const sample = annotations[0];
|
||||
const fileComments = buildAnnotationFileComments(annotations);
|
||||
const fileScopedBody = buildFileScopedBody(annotations);
|
||||
const uniqueFiles = new Set(annotations.map(a => a.filePath));
|
||||
// Exclude the "" sentinel path of general (review-level) comments so they
|
||||
// don't inflate the file count.
|
||||
const uniqueFiles = new Set(annotations.map(a => a.filePath).filter(p => p.length > 0));
|
||||
|
||||
if (prUrl === currentKey && editorFileComments.length > 0) {
|
||||
fileComments.push(...editorFileComments);
|
||||
|
||||
@@ -1,177 +0,0 @@
|
||||
import React, { useCallback, useEffect, useMemo, useState } from 'react';
|
||||
import { useReviewStateOptional } from '../dock/ReviewStateContext';
|
||||
import { useResizablePanel } from '@plannotator/ui/hooks/useResizablePanel';
|
||||
import {
|
||||
SemanticDiffRows,
|
||||
groupSemanticChangesByFile,
|
||||
lineSelectionForChange,
|
||||
} from '../dock/panels/semanticDiffShared';
|
||||
import { loadSemanticDiff } from '../hooks/useFileSemanticChanges';
|
||||
import type {
|
||||
SemanticDiffOkResponse,
|
||||
SemanticDiffChange,
|
||||
SemanticDiffBinaryChange,
|
||||
} from '@plannotator/shared/semantic-diff-types';
|
||||
|
||||
type LoadState =
|
||||
| { status: 'idle' | 'loading' | 'unavailable' | 'error' }
|
||||
| { status: 'ready' | 'empty'; data: SemanticDiffOkResponse };
|
||||
|
||||
/**
|
||||
* Sidebar-constrained semantic diff: the same entity rows as the dock panel,
|
||||
* collapsed into an accordion pinned at the bottom of the file tree. Reuses the
|
||||
* existing /api/semantic-diff endpoint and the shared SemanticDiffRows markup;
|
||||
* clicking a change navigates exactly like the panel/badge (openDiffFile +
|
||||
* line select). Self-contained via ReviewStateContext.
|
||||
*/
|
||||
export const SemanticDiffAccordion: React.FC = () => {
|
||||
const state = useReviewStateOptional();
|
||||
const [open, setOpen] = useState(false);
|
||||
const [loadState, setLoadState] = useState<LoadState>({ status: 'idle' });
|
||||
|
||||
// Vertical resize for the expanded body (reuses the sidebar resize hook on
|
||||
// the y-axis; drag the top handle up to grow the panel, double-click to reset).
|
||||
const resize = useResizablePanel({
|
||||
axis: 'y',
|
||||
side: 'bottom',
|
||||
storageKey: 'pn-semantic-diff-height',
|
||||
defaultWidth: 240,
|
||||
minWidth: 96,
|
||||
maxWidth: 500,
|
||||
});
|
||||
|
||||
const rawPatch = state?.rawPatch;
|
||||
const semanticDiffAvailable = state?.semanticDiffAvailable ?? false;
|
||||
|
||||
useEffect(() => {
|
||||
if (!semanticDiffAvailable) {
|
||||
setLoadState({ status: 'unavailable' });
|
||||
return;
|
||||
}
|
||||
// Reuse the shared, per-patch cache the file-header badges use — one request
|
||||
// for both surfaces (with its retry/backoff) instead of a duplicate fetch.
|
||||
let cancelled = false;
|
||||
setLoadState({ status: 'loading' });
|
||||
loadSemanticDiff(rawPatch ?? '').then((data) => {
|
||||
if (cancelled) return;
|
||||
if (data.status === 'unavailable') {
|
||||
setLoadState({ status: 'unavailable' });
|
||||
return;
|
||||
}
|
||||
if (data.status === 'error') {
|
||||
setLoadState({ status: 'error' });
|
||||
return;
|
||||
}
|
||||
setLoadState(
|
||||
data.changes.length === 0 && data.binaryChanges.length === 0
|
||||
? { status: 'empty', data }
|
||||
: { status: 'ready', data },
|
||||
);
|
||||
});
|
||||
return () => {
|
||||
cancelled = true;
|
||||
};
|
||||
}, [rawPatch, semanticDiffAvailable]);
|
||||
|
||||
const grouped = useMemo(() => {
|
||||
if (loadState.status !== 'ready' && loadState.status !== 'empty') return [];
|
||||
return groupSemanticChangesByFile(loadState.data.changes, loadState.data.binaryChanges);
|
||||
}, [loadState]);
|
||||
|
||||
const count = useMemo(
|
||||
() => grouped.reduce((n, g) => n + g.changes.length + g.binaryChanges.length, 0),
|
||||
[grouped],
|
||||
);
|
||||
|
||||
const openChange = useCallback(
|
||||
(change: SemanticDiffChange) => {
|
||||
state?.openDiffFile(change.filePath);
|
||||
state?.onLineSelection(lineSelectionForChange(change));
|
||||
},
|
||||
[state],
|
||||
);
|
||||
const openBinaryChange = useCallback(
|
||||
(change: SemanticDiffBinaryChange) => {
|
||||
state?.openDiffFile(change.filePath);
|
||||
state?.onLineSelection(null);
|
||||
},
|
||||
[state],
|
||||
);
|
||||
|
||||
if (!semanticDiffAvailable || loadState.status === 'unavailable') return null;
|
||||
|
||||
return (
|
||||
<div className="border-t border-border/50 flex-shrink-0">
|
||||
{open && (
|
||||
<div
|
||||
onPointerDown={resize.handleProps.onPointerDown}
|
||||
onDoubleClick={resize.handleProps.onDoubleClick}
|
||||
style={resize.handleProps.style}
|
||||
role="separator"
|
||||
aria-orientation="horizontal"
|
||||
title="Drag to resize · double-click to reset"
|
||||
className={`h-1 -mt-px cursor-row-resize transition-colors ${
|
||||
resize.isDragging ? 'bg-primary/40' : 'hover:bg-border'
|
||||
}`}
|
||||
/>
|
||||
)}
|
||||
<button
|
||||
type="button"
|
||||
onClick={() => setOpen((v) => !v)}
|
||||
className="w-full flex items-center gap-1 px-2 py-1.5 text-xs text-muted-foreground hover:text-foreground hover:bg-muted transition-colors"
|
||||
title="Semantic diff — changed functions, classes, and other entities"
|
||||
>
|
||||
<svg
|
||||
className={`w-3 h-3 flex-shrink-0 transition-transform ${open ? 'rotate-90' : ''}`}
|
||||
fill="none"
|
||||
viewBox="0 0 24 24"
|
||||
stroke="currentColor"
|
||||
strokeWidth={2}
|
||||
>
|
||||
<path strokeLinecap="round" strokeLinejoin="round" d="M9 5l7 7-7 7" />
|
||||
</svg>
|
||||
<span className="font-medium">Semantic diff</span>
|
||||
{count > 0 && <span className="ml-auto tabular-nums text-muted-foreground/70">{count}</span>}
|
||||
</button>
|
||||
{open && (
|
||||
<div
|
||||
className="semantic-diff-accordion overflow-auto border-t border-border/40"
|
||||
style={{ height: resize.size }}
|
||||
>
|
||||
{loadState.status === 'loading' && (
|
||||
<div className="px-3 py-2 text-xs text-muted-foreground/70">Loading…</div>
|
||||
)}
|
||||
{loadState.status === 'error' && (
|
||||
<div className="px-3 py-2 text-xs text-destructive">Semantic diff failed.</div>
|
||||
)}
|
||||
{(loadState.status === 'ready' || loadState.status === 'empty') && grouped.length === 0 && (
|
||||
<div className="px-3 py-2 text-xs text-muted-foreground/70">No semantic changes.</div>
|
||||
)}
|
||||
{grouped.map((group) => {
|
||||
const slash = group.filePath.lastIndexOf('/');
|
||||
const dir = slash === -1 ? '' : group.filePath.slice(0, slash + 1);
|
||||
const name = slash === -1 ? group.filePath : group.filePath.slice(slash + 1);
|
||||
return (
|
||||
<section className="semantic-diff-file" key={group.filePath}>
|
||||
<header className="semantic-diff-file-header">
|
||||
<span className="semantic-diff-path" title={group.filePath}>
|
||||
{dir && <span className="semantic-diff-path-dir">{dir}</span>}
|
||||
<span className="semantic-diff-path-name">{name}</span>
|
||||
</span>
|
||||
</header>
|
||||
<div className="semantic-diff-rows">
|
||||
<SemanticDiffRows
|
||||
changes={group.changes}
|
||||
binaryChanges={group.binaryChanges}
|
||||
onOpenChange={openChange}
|
||||
onOpenBinary={openBinaryChange}
|
||||
/>
|
||||
</div>
|
||||
</section>
|
||||
);
|
||||
})}
|
||||
</div>
|
||||
)}
|
||||
</div>
|
||||
);
|
||||
};
|
||||
@@ -112,6 +112,10 @@ export interface ReviewState {
|
||||
onAllFilesVisibleFileChange: (filePath: string | null) => void;
|
||||
isAllFilesActive: boolean;
|
||||
semanticDiffAvailable: boolean;
|
||||
isSemanticDiffActive: boolean;
|
||||
onSemanticDiffUnavailable: () => void;
|
||||
onSemanticDiffLoadError: () => boolean;
|
||||
onSemanticDiffLoadSuccess: () => void;
|
||||
|
||||
// Tour
|
||||
openTourPanel: (jobId: string) => void;
|
||||
|
||||
@@ -8,6 +8,7 @@ import { CopyButton } from '../../components/CopyButton';
|
||||
import { LiveLogViewer } from '../../components/LiveLogViewer';
|
||||
import { ScrollFade } from '../../components/ScrollFade';
|
||||
import { exportReviewFeedback } from '../../utils/exportFeedback';
|
||||
import { annotationScope, copyLocationPrefix } from '../../utils/annotationDisplay';
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Panel
|
||||
@@ -83,7 +84,8 @@ export const ReviewAgentJobDetailPanel: React.FC<IDockviewPanelProps> = (props)
|
||||
const dismissedCount = useMemo(() => displayAnnotations.filter((d) => d.dismissed).length, [displayAnnotations]);
|
||||
|
||||
const handleAnnotationClick = useCallback((ann: CodeAnnotation) => {
|
||||
state.openDiffFile(ann.filePath);
|
||||
// General comments belong to no file — nothing to open in the diff.
|
||||
if (ann.filePath) state.openDiffFile(ann.filePath);
|
||||
state.onSelectAnnotation(ann.id);
|
||||
}, [state.openDiffFile, state.onSelectAnnotation]);
|
||||
|
||||
@@ -408,7 +410,8 @@ function AnnotationRow({ annotation: ann, dismissed, onClick }: {
|
||||
dismissed: boolean;
|
||||
onClick: (ann: CodeAnnotation) => void;
|
||||
}) {
|
||||
const copyText = ann.text ? `${ann.filePath}:${ann.lineStart}${ann.lineEnd !== ann.lineStart ? `-${ann.lineEnd}` : ''}\n${ann.text}${ann.reasoning ? `\n\nReasoning: ${ann.reasoning}` : ''}` : '';
|
||||
const scope = annotationScope(ann);
|
||||
const copyText = ann.text ? `${copyLocationPrefix(ann, scope)}${ann.text}${ann.reasoning ? `\n\nReasoning: ${ann.reasoning}` : ''}` : '';
|
||||
const severity = ann.severity ? SEVERITY_STYLES[ann.severity] : null;
|
||||
return (
|
||||
<div
|
||||
@@ -421,12 +424,29 @@ function AnnotationRow({ annotation: ann, dismissed, onClick }: {
|
||||
{severity && (
|
||||
<span className={`w-2 h-2 rounded-full flex-shrink-0 ${severity.dot}`} title={severity.label} />
|
||||
)}
|
||||
<span className={`font-mono truncate ${dismissed ? 'line-through text-muted-foreground' : 'text-primary'}`}>
|
||||
{ann.filePath}
|
||||
</span>
|
||||
<span className="text-muted-foreground flex-shrink-0">
|
||||
L{ann.lineStart}{ann.lineEnd !== ann.lineStart ? `–${ann.lineEnd}` : ''}
|
||||
</span>
|
||||
{scope === 'general' ? (
|
||||
<span className={`font-mono uppercase tracking-wider flex-shrink-0 ${dismissed ? 'line-through text-muted-foreground' : 'text-primary'}`}>
|
||||
general
|
||||
</span>
|
||||
) : (
|
||||
<>
|
||||
<span className={`font-mono truncate ${dismissed ? 'line-through text-muted-foreground' : 'text-primary'}`}>
|
||||
{ann.filePath}
|
||||
</span>
|
||||
{scope === 'file' ? (
|
||||
<span className="text-muted-foreground flex-shrink-0 uppercase tracking-wider">file</span>
|
||||
) : (
|
||||
<span className="text-muted-foreground flex-shrink-0">
|
||||
L{ann.lineStart}{ann.lineEnd !== ann.lineStart ? `–${ann.lineEnd}` : ''}
|
||||
</span>
|
||||
)}
|
||||
</>
|
||||
)}
|
||||
{ann.reviewProfileLabel && (
|
||||
<span className="px-1.5 py-0.5 rounded text-[9px] uppercase tracking-wider bg-accent/10 text-accent/90 flex-shrink-0">
|
||||
{ann.reviewProfileLabel}
|
||||
</span>
|
||||
)}
|
||||
{dismissed && (
|
||||
<span className="px-1 py-0.5 rounded text-[10px] uppercase tracking-wider bg-muted text-muted-foreground/60">dismissed</span>
|
||||
)}
|
||||
|
||||
@@ -0,0 +1,188 @@
|
||||
import React, { useCallback, useEffect, useMemo, useState } from 'react';
|
||||
import type {
|
||||
SemanticDiffBinaryChange,
|
||||
SemanticDiffChange,
|
||||
SemanticDiffResponse,
|
||||
} from '@plannotator/shared/semantic-diff-types';
|
||||
import { useReviewState } from '../ReviewStateContext';
|
||||
import {
|
||||
SemanticDiffRows,
|
||||
groupSemanticChangesByFile,
|
||||
lineSelectionForChange,
|
||||
} from './semanticDiffShared';
|
||||
|
||||
type SemanticDiffOkResponse = Extract<SemanticDiffResponse, { status: 'ok' }>;
|
||||
type SemanticDiffErrorResponse = Extract<SemanticDiffResponse, { status: 'error' }>;
|
||||
|
||||
type LoadState =
|
||||
| { status: 'idle' | 'loading' }
|
||||
| { status: 'ready'; data: SemanticDiffOkResponse }
|
||||
| { status: 'empty'; data: SemanticDiffOkResponse }
|
||||
| { status: 'error'; error: SemanticDiffErrorResponse | Error };
|
||||
|
||||
function formatSummary(data: SemanticDiffOkResponse): string {
|
||||
const summary = data.summary;
|
||||
const parts = [
|
||||
`${summary.added} added`,
|
||||
`${summary.modified} modified`,
|
||||
`${summary.deleted} deleted`,
|
||||
];
|
||||
if (summary.renamed > 0) parts.push(`${summary.renamed} renamed`);
|
||||
if (summary.moved > 0) parts.push(`${summary.moved} moved`);
|
||||
if (summary.reordered > 0) parts.push(`${summary.reordered} reordered`);
|
||||
if (summary.binary > 0) parts.push(`${summary.binary} binary`);
|
||||
if (summary.orphan > 0) parts.push(`${summary.orphan} orphans`);
|
||||
return `Summary: ${parts.join(', ')} across ${summary.fileCount} files`;
|
||||
}
|
||||
|
||||
function formatLoadError(error: SemanticDiffErrorResponse | Error): string {
|
||||
return error.message || 'Semantic diff failed.';
|
||||
}
|
||||
|
||||
function splitFilePath(filePath: string): { dir: string; name: string } {
|
||||
const lastSlash = filePath.lastIndexOf('/');
|
||||
if (lastSlash === -1) return { dir: '', name: filePath };
|
||||
return { dir: filePath.slice(0, lastSlash + 1), name: filePath.slice(lastSlash + 1) };
|
||||
}
|
||||
|
||||
export function ReviewSemanticDiffPanel() {
|
||||
const state = useReviewState();
|
||||
const {
|
||||
rawPatch,
|
||||
semanticDiffAvailable,
|
||||
onSemanticDiffUnavailable,
|
||||
onSemanticDiffLoadError,
|
||||
onSemanticDiffLoadSuccess,
|
||||
openDiffFile,
|
||||
onLineSelection,
|
||||
} = state;
|
||||
const [loadState, setLoadState] = useState<LoadState>({ status: 'idle' });
|
||||
const [retryCount, setRetryCount] = useState(0);
|
||||
|
||||
useEffect(() => {
|
||||
if (!semanticDiffAvailable) return;
|
||||
|
||||
const controller = new AbortController();
|
||||
setLoadState({ status: 'loading' });
|
||||
|
||||
fetch('/api/semantic-diff', { signal: controller.signal })
|
||||
.then((res) => {
|
||||
if (!res.ok) throw new Error('Semantic diff failed');
|
||||
return res.json() as Promise<SemanticDiffResponse>;
|
||||
})
|
||||
.then((data) => {
|
||||
if (controller.signal.aborted) return;
|
||||
if (data.status === 'unavailable') {
|
||||
onSemanticDiffUnavailable();
|
||||
return;
|
||||
}
|
||||
if (data.status === 'error') {
|
||||
if (onSemanticDiffLoadError()) return;
|
||||
setLoadState({ status: 'error', error: data });
|
||||
return;
|
||||
}
|
||||
onSemanticDiffLoadSuccess();
|
||||
setLoadState(data.changes.length === 0 && data.binaryChanges.length === 0
|
||||
? { status: 'empty', data }
|
||||
: { status: 'ready', data });
|
||||
})
|
||||
.catch((error) => {
|
||||
if (controller.signal.aborted) return;
|
||||
console.error('Failed to load semantic diff:', error);
|
||||
if (onSemanticDiffLoadError()) return;
|
||||
setLoadState({ status: 'error', error: error instanceof Error ? error : new Error(String(error)) });
|
||||
});
|
||||
|
||||
return () => controller.abort();
|
||||
}, [
|
||||
rawPatch,
|
||||
retryCount,
|
||||
semanticDiffAvailable,
|
||||
onSemanticDiffUnavailable,
|
||||
onSemanticDiffLoadError,
|
||||
onSemanticDiffLoadSuccess,
|
||||
]);
|
||||
|
||||
const groupedChanges = useMemo(() => {
|
||||
if (loadState.status !== 'ready' && loadState.status !== 'empty') return [];
|
||||
return groupSemanticChangesByFile(loadState.data.changes, loadState.data.binaryChanges);
|
||||
}, [loadState]);
|
||||
|
||||
const openChange = useCallback((change: SemanticDiffChange) => {
|
||||
openDiffFile(change.filePath);
|
||||
onLineSelection(lineSelectionForChange(change));
|
||||
}, [openDiffFile, onLineSelection]);
|
||||
|
||||
const openBinaryChange = useCallback((change: SemanticDiffBinaryChange) => {
|
||||
openDiffFile(change.filePath);
|
||||
onLineSelection(null);
|
||||
}, [openDiffFile, onLineSelection]);
|
||||
|
||||
if (!semanticDiffAvailable) return null;
|
||||
|
||||
if (loadState.status === 'idle' || loadState.status === 'loading') {
|
||||
return (
|
||||
<div className="semantic-diff-panel">
|
||||
<div className="semantic-diff-terminal" aria-live="polite">
|
||||
<div className="semantic-diff-loading">Running semantic diff...</div>
|
||||
</div>
|
||||
</div>
|
||||
);
|
||||
}
|
||||
|
||||
if (loadState.status === 'error') {
|
||||
return (
|
||||
<div className="semantic-diff-panel">
|
||||
<div className="semantic-diff-terminal" aria-live="polite">
|
||||
<div className="semantic-diff-error" role="alert">
|
||||
Semantic diff failed: {formatLoadError(loadState.error)}
|
||||
</div>
|
||||
<button
|
||||
type="button"
|
||||
className="semantic-diff-retry"
|
||||
onClick={() => setRetryCount((count) => count + 1)}
|
||||
>
|
||||
↻ retry
|
||||
</button>
|
||||
</div>
|
||||
</div>
|
||||
);
|
||||
}
|
||||
|
||||
return (
|
||||
<div className="semantic-diff-panel">
|
||||
<div className="semantic-diff-terminal" aria-label="Semantic diff">
|
||||
{groupedChanges.map((group) => (
|
||||
<section className="semantic-diff-file" key={group.filePath}>
|
||||
<header className="semantic-diff-file-header">
|
||||
<span className="semantic-diff-path" title={group.filePath}>
|
||||
{(() => {
|
||||
const { dir, name } = splitFilePath(group.filePath);
|
||||
return (
|
||||
<>
|
||||
{dir && <span className="semantic-diff-path-dir">{dir}</span>}
|
||||
<span className="semantic-diff-path-name">{name}</span>
|
||||
</>
|
||||
);
|
||||
})()}
|
||||
</span>
|
||||
</header>
|
||||
<div className="semantic-diff-rows">
|
||||
<SemanticDiffRows
|
||||
changes={group.changes}
|
||||
binaryChanges={group.binaryChanges}
|
||||
onOpenChange={openChange}
|
||||
onOpenBinary={openBinaryChange}
|
||||
/>
|
||||
</div>
|
||||
</section>
|
||||
))}
|
||||
|
||||
{loadState.status === 'empty' && (
|
||||
<div className="semantic-diff-empty">No semantic changes found.</div>
|
||||
)}
|
||||
<div className="semantic-diff-summary">{formatSummary(loadState.data)}</div>
|
||||
</div>
|
||||
</div>
|
||||
);
|
||||
}
|
||||
@@ -6,6 +6,7 @@ import { ReviewPRCommentsPanel } from './panels/ReviewPRCommentsPanel';
|
||||
import { ReviewPRChecksPanel } from './panels/ReviewPRChecksPanel';
|
||||
import { ReviewAllFilesDiffPanel } from './panels/ReviewAllFilesDiffPanel';
|
||||
import { ReviewCodeNavPanel } from './panels/ReviewCodeNavPanel';
|
||||
import { ReviewSemanticDiffPanel } from './panels/ReviewSemanticDiffPanel';
|
||||
|
||||
/**
|
||||
* Component registry for dockview — maps panel type strings to React components.
|
||||
@@ -19,4 +20,5 @@ export const reviewPanelComponents = {
|
||||
[REVIEW_PANEL_TYPES.PR_CHECKS]: ReviewPRChecksPanel,
|
||||
[REVIEW_PANEL_TYPES.ALL_FILES]: ReviewAllFilesDiffPanel,
|
||||
[REVIEW_PANEL_TYPES.CODE_NAV]: ReviewCodeNavPanel,
|
||||
[REVIEW_PANEL_TYPES.SEMANTIC_DIFF]: ReviewSemanticDiffPanel,
|
||||
} as const;
|
||||
|
||||
@@ -13,6 +13,7 @@ export const REVIEW_PANEL_TYPES = {
|
||||
PR_CHECKS: 'review-pr-checks',
|
||||
ALL_FILES: 'review-all-files',
|
||||
CODE_NAV: 'review-code-nav',
|
||||
SEMANTIC_DIFF: 'review-semantic-diff',
|
||||
} as const;
|
||||
|
||||
export const REVIEW_DIFF_PANEL_ID = 'review-diff';
|
||||
@@ -29,6 +30,7 @@ export const REVIEW_PR_COMMENTS_PANEL_ID = 'review-pr-comments';
|
||||
export const REVIEW_PR_CHECKS_PANEL_ID = 'review-pr-checks';
|
||||
export const REVIEW_ALL_FILES_PANEL_ID = 'review-all-files';
|
||||
export const REVIEW_CODE_NAV_PANEL_ID = 'review-code-nav';
|
||||
export const REVIEW_SEMANTIC_DIFF_PANEL_ID = 'review-semantic-diff';
|
||||
|
||||
export function isReviewDiffPanelId(panelId: string): boolean {
|
||||
return panelId === REVIEW_DIFF_PANEL_ID;
|
||||
|
||||
@@ -32,7 +32,7 @@ async function fetchSemanticDiff(): Promise<SemanticDiffResponse> {
|
||||
return res.json() as Promise<SemanticDiffResponse>;
|
||||
}
|
||||
|
||||
export function loadSemanticDiff(rawPatch: string): Promise<SemanticDiffResponse> {
|
||||
function loadSemanticDiff(rawPatch: string): Promise<SemanticDiffResponse> {
|
||||
if (cacheKey === rawPatch && cachePromise) return cachePromise;
|
||||
cacheKey = rawPatch;
|
||||
|
||||
|
||||
@@ -329,25 +329,6 @@ diffs-container {
|
||||
cursor: pointer;
|
||||
}
|
||||
|
||||
/* Sidebar accordion: compact the panel rows so they fit the narrow file tree
|
||||
(the panel sized them via .semantic-diff-terminal, which we don't use here). */
|
||||
.semantic-diff-accordion {
|
||||
font-family: var(--diff-font-override, var(--font-mono));
|
||||
font-size: 0.75rem;
|
||||
line-height: 1.4;
|
||||
font-variant-numeric: tabular-nums;
|
||||
}
|
||||
.semantic-diff-accordion .semantic-diff-file {
|
||||
margin: 0 0 0.5rem;
|
||||
}
|
||||
.semantic-diff-accordion .semantic-diff-file-header {
|
||||
padding: 0 0.375rem 0.25rem;
|
||||
margin-bottom: 0.125rem;
|
||||
}
|
||||
.semantic-diff-accordion .semantic-diff-row {
|
||||
min-height: 1.5rem;
|
||||
padding: 0.125rem 0.375rem;
|
||||
}
|
||||
|
||||
.semantic-diff-retry {
|
||||
display: inline-flex;
|
||||
|
||||
@@ -0,0 +1,21 @@
|
||||
import type { CodeAnnotation, CodeAnnotationScope } from '@plannotator/ui/types';
|
||||
|
||||
/** A code annotation's scope, defaulting to 'line' for older/external data. */
|
||||
export function annotationScope(a: CodeAnnotation): CodeAnnotationScope {
|
||||
return a.scope ?? 'line';
|
||||
}
|
||||
|
||||
/**
|
||||
* The location prefix for an annotation's copied text. General comments belong
|
||||
* to no file, so they carry no prefix; file comments carry just the path; line
|
||||
* comments carry path + line range. Never emits the "" / 0 sentinels that stand
|
||||
* in for "no file / no line" on file and general comments.
|
||||
*/
|
||||
export function copyLocationPrefix(
|
||||
a: CodeAnnotation,
|
||||
scope: CodeAnnotationScope = annotationScope(a),
|
||||
): string {
|
||||
if (scope === 'general') return '';
|
||||
if (scope === 'file') return `${a.filePath}\n`;
|
||||
return `${a.filePath}:${a.lineStart}${a.lineEnd !== a.lineStart ? `-${a.lineEnd}` : ''}\n`;
|
||||
}
|
||||
@@ -296,4 +296,24 @@ describe("exportReviewFeedback", () => {
|
||||
expect(result).toContain("Review scope: layer");
|
||||
expect(result).not.toContain("Review scope: full-stack");
|
||||
});
|
||||
|
||||
it("general comments render under a General section, not a file/line group", () => {
|
||||
const result = exportReviewFeedback([
|
||||
ann({ id: "g", scope: "general", filePath: "", lineStart: 0, lineEnd: 0, text: "the overall approach is off" }),
|
||||
]);
|
||||
expect(result).toContain("## General");
|
||||
expect(result).toContain("the overall approach is off");
|
||||
// No bogus line heading for a review-level comment.
|
||||
expect(result).not.toContain("Line 0");
|
||||
});
|
||||
|
||||
it("mixes line and general: both appear, general in its own section", () => {
|
||||
const result = exportReviewFeedback([
|
||||
ann({ id: "l", text: "line issue" }),
|
||||
ann({ id: "g", scope: "general", filePath: "", lineStart: 0, lineEnd: 0, text: "review-wide note" }),
|
||||
]);
|
||||
expect(result).toContain("line issue");
|
||||
expect(result).toContain("## General");
|
||||
expect(result).toContain("review-wide note");
|
||||
});
|
||||
});
|
||||
|
||||
@@ -117,6 +117,23 @@ function formatFileAnnotations(fileAnnotations: CodeAnnotation[], headingLevel =
|
||||
return output;
|
||||
}
|
||||
|
||||
function renderGeneralComments(annotations: CodeAnnotation[]): string {
|
||||
let output = '## General\n\n';
|
||||
for (const ann of annotations) {
|
||||
const prefix = formatConventionalPrefix(ann.conventionalLabel, ann.decorations);
|
||||
if (ann.text) {
|
||||
output += `${prefix}${ann.text}\n`;
|
||||
} else if (prefix) {
|
||||
output += `${prefix.trimEnd()}\n`;
|
||||
}
|
||||
if (ann.reasoning) {
|
||||
output += `\n**Reasoning:** ${ann.reasoning}\n`;
|
||||
}
|
||||
output += '\n';
|
||||
}
|
||||
return output;
|
||||
}
|
||||
|
||||
function groupByFile(annotations: CodeAnnotation[]): Map<string, CodeAnnotation[]> {
|
||||
const grouped = new Map<string, CodeAnnotation[]>();
|
||||
for (const ann of annotations) {
|
||||
@@ -170,7 +187,13 @@ export function exportReviewFeedback(
|
||||
return '# Code Review\n\nNo feedback provided.';
|
||||
}
|
||||
|
||||
const prUrls = new Set(annotations.map(a => a.prUrl).filter(Boolean));
|
||||
// General (review-level) comments belong to no file — render them in their own
|
||||
// section and group only the rest by file.
|
||||
const general = annotations.filter(a => (a.scope ?? 'line') === 'general');
|
||||
const placed = annotations.filter(a => (a.scope ?? 'line') !== 'general');
|
||||
const generalSection = general.length > 0 ? renderGeneralComments(general) : '';
|
||||
|
||||
const prUrls = new Set(placed.map(a => a.prUrl).filter(Boolean));
|
||||
const isMultiPR = prUrls.size > 1;
|
||||
const singlePrUrl = prUrls.size === 1 ? [...prUrls][0] : null;
|
||||
const prMismatch = singlePrUrl && prMeta && singlePrUrl !== prMeta.url;
|
||||
@@ -188,7 +211,8 @@ export function exportReviewFeedback(
|
||||
`${prMeta.url}\n\n`
|
||||
: `# Code Review Feedback\n\n${diffContext ? `**Diff:** ${describeDiff(diffContext)}\n\n` : ''}`;
|
||||
|
||||
output += renderScopedGroups(annotations, '##');
|
||||
output += renderScopedGroups(placed, '##');
|
||||
output += generalSection;
|
||||
return output;
|
||||
}
|
||||
|
||||
@@ -196,7 +220,7 @@ export function exportReviewFeedback(
|
||||
let output = isMultiPR ? '# Multi-PR Review\n\n' : '# Code Review\n\n';
|
||||
|
||||
const byPR = new Map<string, CodeAnnotation[]>();
|
||||
for (const ann of annotations) {
|
||||
for (const ann of placed) {
|
||||
const key = ann.prUrl ?? '_none';
|
||||
const existing = byPR.get(key) || [];
|
||||
existing.push(ann);
|
||||
@@ -222,5 +246,6 @@ export function exportReviewFeedback(
|
||||
output += renderScopedGroups(prAnnotations, '###');
|
||||
}
|
||||
|
||||
output += generalSection;
|
||||
return output;
|
||||
}
|
||||
|
||||
@@ -0,0 +1,147 @@
|
||||
/**
|
||||
* Launch-plumbing tests for the custom-reviews `reviewProfileId` field.
|
||||
*
|
||||
* These exercise the Bun POST /api/agents/jobs handler contract that the Pi
|
||||
* mirror must match byte-for-byte:
|
||||
* - `reviewProfileId` is parsed from the body and forwarded into buildCommand.
|
||||
* - Unknown fields are rejected (fail loud, not silently ignored).
|
||||
* - An absent id forwards no `reviewProfileId` (review.ts resolves that to
|
||||
* builtin:default).
|
||||
* - The launched job carries the reviewProfileId/Label stamped by buildCommand.
|
||||
*
|
||||
* `Bun.which` is mocked so capability detection reports providers available
|
||||
* regardless of whether the host has the CLIs installed (CI parity).
|
||||
*/
|
||||
import { afterEach, beforeEach, describe, expect, test } from "bun:test";
|
||||
|
||||
const realWhich = Bun.which;
|
||||
beforeEach(() => {
|
||||
// Make every provider "available" so the capability gate doesn't depend on
|
||||
// the host having claude/codex installed.
|
||||
(Bun as { which: typeof Bun.which }).which = (() => "/fake/bin") as typeof Bun.which;
|
||||
});
|
||||
afterEach(() => {
|
||||
(Bun as { which: typeof Bun.which }).which = realWhich;
|
||||
});
|
||||
|
||||
// Imported after the which-mock is in place at module top so the factory's
|
||||
// one-time capability scan sees available providers. createAgentJobHandler reads
|
||||
// Bun.which at call time (inside the factory), so import timing is irrelevant —
|
||||
// but the per-test beforeEach guarantees availability anyway.
|
||||
const { createAgentJobHandler } = await import("./agent-jobs");
|
||||
|
||||
function post(body: unknown): Request {
|
||||
return new Request("http://localhost/api/agents/jobs", {
|
||||
method: "POST",
|
||||
headers: { "content-type": "application/json" },
|
||||
body: JSON.stringify(body),
|
||||
});
|
||||
}
|
||||
|
||||
const JOBS_URL = new URL("http://localhost/api/agents/jobs");
|
||||
|
||||
describe("POST /api/agents/jobs — reviewProfileId launch plumbing", () => {
|
||||
test("forwards reviewProfileId into buildCommand config", async () => {
|
||||
let seenConfig: Record<string, unknown> | undefined;
|
||||
const handler = createAgentJobHandler({
|
||||
mode: "review",
|
||||
getServerUrl: () => "http://localhost:1234",
|
||||
getCwd: () => "/tmp",
|
||||
async buildCommand(_provider, config) {
|
||||
seenConfig = config;
|
||||
// Return a no-op command that won't actually spawn anything useful.
|
||||
return { command: ["true"], reviewProfileId: "user:security", reviewProfileLabel: "Security" };
|
||||
},
|
||||
});
|
||||
|
||||
const res = await handler.handle(post({ provider: "codex", reviewProfileId: "user:security" }), JOBS_URL);
|
||||
|
||||
expect(res?.status).toBe(201);
|
||||
expect(seenConfig?.reviewProfileId).toBe("user:security");
|
||||
handler.killAll();
|
||||
});
|
||||
|
||||
test("rejects unknown fields with 400", async () => {
|
||||
const handler = createAgentJobHandler({
|
||||
mode: "review",
|
||||
getServerUrl: () => "http://localhost:1234",
|
||||
getCwd: () => "/tmp",
|
||||
async buildCommand() {
|
||||
return { command: ["true"] };
|
||||
},
|
||||
});
|
||||
|
||||
const res = await handler.handle(post({ provider: "codex", reviewPrompt: "inline" }), JOBS_URL);
|
||||
|
||||
expect(res?.status).toBe(400);
|
||||
const json = await res!.json();
|
||||
expect(json.error).toContain("reviewPrompt");
|
||||
handler.killAll();
|
||||
});
|
||||
|
||||
test("absent reviewProfileId forwards no reviewProfileId in config", async () => {
|
||||
let seenConfig: Record<string, unknown> | undefined;
|
||||
let called = false;
|
||||
const handler = createAgentJobHandler({
|
||||
mode: "review",
|
||||
getServerUrl: () => "http://localhost:1234",
|
||||
getCwd: () => "/tmp",
|
||||
async buildCommand(_provider, config) {
|
||||
called = true;
|
||||
seenConfig = config;
|
||||
return { command: ["true"] };
|
||||
},
|
||||
});
|
||||
|
||||
const res = await handler.handle(post({ provider: "codex" }), JOBS_URL);
|
||||
|
||||
expect(res?.status).toBe(201);
|
||||
expect(called).toBe(true);
|
||||
// No config keys at all → buildCommand receives undefined (review.ts maps
|
||||
// that to builtin:default).
|
||||
expect(seenConfig?.reviewProfileId).toBeUndefined();
|
||||
handler.killAll();
|
||||
});
|
||||
|
||||
test("launched job carries reviewProfileId and reviewProfileLabel stamped by buildCommand", async () => {
|
||||
const handler = createAgentJobHandler({
|
||||
mode: "review",
|
||||
getServerUrl: () => "http://localhost:1234",
|
||||
getCwd: () => "/tmp",
|
||||
async buildCommand() {
|
||||
return {
|
||||
command: ["true"],
|
||||
reviewProfileId: "user:api-contracts",
|
||||
reviewProfileLabel: "API Contracts",
|
||||
};
|
||||
},
|
||||
});
|
||||
|
||||
const res = await handler.handle(post({ provider: "claude", reviewProfileId: "user:api-contracts" }), JOBS_URL);
|
||||
|
||||
expect(res?.status).toBe(201);
|
||||
const { job } = await res!.json();
|
||||
expect(job.reviewProfileId).toBe("user:api-contracts");
|
||||
expect(job.reviewProfileLabel).toBe("API Contracts");
|
||||
handler.killAll();
|
||||
});
|
||||
|
||||
test("a job launched without a profile omits the profile fields entirely", async () => {
|
||||
const handler = createAgentJobHandler({
|
||||
mode: "review",
|
||||
getServerUrl: () => "http://localhost:1234",
|
||||
getCwd: () => "/tmp",
|
||||
async buildCommand() {
|
||||
return { command: ["true"] };
|
||||
},
|
||||
});
|
||||
|
||||
const res = await handler.handle(post({ provider: "codex" }), JOBS_URL);
|
||||
|
||||
expect(res?.status).toBe(201);
|
||||
const { job } = await res!.json();
|
||||
expect("reviewProfileId" in job).toBe(false);
|
||||
expect("reviewProfileLabel" in job).toBe(false);
|
||||
handler.killAll();
|
||||
});
|
||||
});
|
||||
@@ -87,6 +87,10 @@ export interface AgentJobHandlerOptions {
|
||||
diffScope?: string;
|
||||
/** Diff context snapshot at launch (stored on AgentJobInfo for per-job "Copy All"). */
|
||||
diffContext?: AgentJobInfo["diffContext"];
|
||||
/** Resolved review profile id at launch time. Stored on AgentJobInfo. */
|
||||
reviewProfileId?: string;
|
||||
/** Resolved review profile label at launch time. Stored on AgentJobInfo. */
|
||||
reviewProfileLabel?: string;
|
||||
} | null>;
|
||||
/**
|
||||
* Called after a job process exits with exit code 0.
|
||||
@@ -132,13 +136,13 @@ export function createAgentJobHandler(options: AgentJobHandlerOptions): AgentJob
|
||||
|
||||
// --- Process lifecycle ---
|
||||
function spawnJob(
|
||||
id: string,
|
||||
provider: string,
|
||||
command: string[],
|
||||
label: string,
|
||||
outputPath?: string,
|
||||
spawnOptions?: { captureStdout?: boolean; stdinPrompt?: string; cwd?: string; prompt?: string; engine?: string; model?: string; effort?: string; reasoningEffort?: string; fastMode?: boolean; prUrl?: string; diffScope?: string; diffContext?: AgentJobInfo["diffContext"] },
|
||||
spawnOptions?: { captureStdout?: boolean; stdinPrompt?: string; cwd?: string; prompt?: string; engine?: string; model?: string; effort?: string; reasoningEffort?: string; fastMode?: boolean; prUrl?: string; diffScope?: string; diffContext?: AgentJobInfo["diffContext"]; reviewProfileId?: string; reviewProfileLabel?: string },
|
||||
): AgentJobInfo {
|
||||
const id = crypto.randomUUID();
|
||||
const source = jobSource(id);
|
||||
|
||||
const info: AgentJobInfo = {
|
||||
@@ -158,6 +162,8 @@ export function createAgentJobHandler(options: AgentJobHandlerOptions): AgentJob
|
||||
...(spawnOptions?.prUrl && { prUrl: spawnOptions.prUrl }),
|
||||
...(spawnOptions?.diffScope && { diffScope: spawnOptions.diffScope }),
|
||||
...(spawnOptions?.diffContext && { diffContext: spawnOptions.diffContext }),
|
||||
...(spawnOptions?.reviewProfileId && { reviewProfileId: spawnOptions.reviewProfileId }),
|
||||
...(spawnOptions?.reviewProfileLabel && { reviewProfileLabel: spawnOptions.reviewProfileLabel }),
|
||||
};
|
||||
|
||||
let proc: ReturnType<typeof Bun.spawn> | null = null;
|
||||
@@ -299,7 +305,6 @@ export function createAgentJobHandler(options: AgentJobHandlerOptions): AgentJob
|
||||
}
|
||||
jobOutputPaths.delete(id);
|
||||
jobOutputPaths.delete(`${id}:cwd`);
|
||||
|
||||
broadcast({ type: "job:completed", job: { ...entry.info } });
|
||||
}).catch(() => {
|
||||
// Guard against unhandled rejection from unexpected runtime errors
|
||||
@@ -429,6 +434,24 @@ export function createAgentJobHandler(options: AgentJobHandlerOptions): AgentJob
|
||||
if (url.pathname === JOBS && req.method === "POST") {
|
||||
try {
|
||||
const body = await req.json();
|
||||
|
||||
// Reject unknown fields rather than silently ignoring them (per the
|
||||
// custom-reviews spec — a typo'd field should fail loud, not no-op).
|
||||
const KNOWN_JOB_FIELDS = new Set([
|
||||
"provider", "command", "label",
|
||||
"engine", "model", "reasoningEffort", "effort", "fastMode",
|
||||
"reviewProfileId",
|
||||
]);
|
||||
if (body && typeof body === "object") {
|
||||
const unknown = Object.keys(body).filter((k) => !KNOWN_JOB_FIELDS.has(k));
|
||||
if (unknown.length > 0) {
|
||||
return Response.json(
|
||||
{ error: `Unknown field(s): ${unknown.join(", ")}` },
|
||||
{ status: 400 },
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
const provider = typeof body.provider === "string" ? body.provider : "";
|
||||
let rawCommand = Array.isArray(body.command) ? body.command : [];
|
||||
let command = rawCommand.filter((c: unknown): c is string => typeof c === "string");
|
||||
@@ -457,6 +480,9 @@ export function createAgentJobHandler(options: AgentJobHandlerOptions): AgentJob
|
||||
let jobPrUrl: string | undefined;
|
||||
let jobDiffScope: string | undefined;
|
||||
let jobDiffContext: AgentJobInfo["diffContext"] | undefined;
|
||||
let jobReviewProfileId: string | undefined;
|
||||
let jobReviewProfileLabel: string | undefined;
|
||||
const jobId = crypto.randomUUID();
|
||||
if (options.buildCommand) {
|
||||
// Thread config from POST body to buildCommand
|
||||
const config: Record<string, unknown> = {};
|
||||
@@ -465,6 +491,7 @@ export function createAgentJobHandler(options: AgentJobHandlerOptions): AgentJob
|
||||
if (typeof body.reasoningEffort === "string") config.reasoningEffort = body.reasoningEffort;
|
||||
if (typeof body.effort === "string") config.effort = body.effort;
|
||||
if (body.fastMode === true) config.fastMode = true;
|
||||
if (typeof body.reviewProfileId === "string") config.reviewProfileId = body.reviewProfileId;
|
||||
const built = await options.buildCommand(provider, Object.keys(config).length > 0 ? config : undefined);
|
||||
if (built) {
|
||||
command = built.command;
|
||||
@@ -482,6 +509,8 @@ export function createAgentJobHandler(options: AgentJobHandlerOptions): AgentJob
|
||||
jobPrUrl = built.prUrl;
|
||||
jobDiffScope = built.diffScope;
|
||||
jobDiffContext = built.diffContext;
|
||||
jobReviewProfileId = built.reviewProfileId;
|
||||
jobReviewProfileLabel = built.reviewProfileLabel;
|
||||
}
|
||||
}
|
||||
|
||||
@@ -492,7 +521,7 @@ export function createAgentJobHandler(options: AgentJobHandlerOptions): AgentJob
|
||||
);
|
||||
}
|
||||
|
||||
const job = spawnJob(provider, command, label, outputPath, {
|
||||
const job = spawnJob(jobId, provider, command, label, outputPath, {
|
||||
captureStdout,
|
||||
stdinPrompt,
|
||||
cwd: spawnCwd,
|
||||
@@ -505,6 +534,8 @@ export function createAgentJobHandler(options: AgentJobHandlerOptions): AgentJob
|
||||
prUrl: jobPrUrl,
|
||||
diffScope: jobDiffScope,
|
||||
diffContext: jobDiffContext,
|
||||
reviewProfileId: jobReviewProfileId,
|
||||
reviewProfileLabel: jobReviewProfileLabel,
|
||||
});
|
||||
return Response.json({ job }, { status: 201 });
|
||||
} catch (err) {
|
||||
|
||||
@@ -112,6 +112,91 @@ describe("buildAgentReviewUserMessage", () => {
|
||||
});
|
||||
});
|
||||
|
||||
describe("buildAgentReviewUserMessage — contextOnly (custom review skill)", () => {
|
||||
test("keeps the git context but drops the review framing prose", () => {
|
||||
const message = buildAgentReviewUserMessage(
|
||||
patch,
|
||||
"last-commit",
|
||||
{ defaultBranch: "origin/main" },
|
||||
undefined,
|
||||
true,
|
||||
);
|
||||
|
||||
expect(message).toContain("the code changes introduced in the last commit");
|
||||
expect(message).toContain("git diff HEAD~1..HEAD");
|
||||
expect(message).not.toContain("Review the");
|
||||
expect(message).not.toContain("Provide prioritized, actionable findings.");
|
||||
expect(message).not.toContain(patch);
|
||||
});
|
||||
|
||||
test("falls back to the inline patch without the review framing line", () => {
|
||||
const message = buildAgentReviewUserMessage(patch, "p4-default", undefined, undefined, true);
|
||||
|
||||
expect(message).toContain(patch);
|
||||
expect(message).not.toContain("Review the following code changes");
|
||||
expect(message).not.toContain("provide prioritized findings");
|
||||
});
|
||||
|
||||
test("PR full-stack drops the review line but keeps the URL and stack context", () => {
|
||||
const prMetadata = {
|
||||
url: "https://github.com/o/r/pull/7",
|
||||
baseBranch: "main",
|
||||
} as Parameters<typeof buildAgentReviewUserMessage>[3];
|
||||
const message = buildAgentReviewUserMessage(
|
||||
patch,
|
||||
"branch",
|
||||
{ prDiffScope: "full-stack" },
|
||||
prMetadata,
|
||||
true,
|
||||
);
|
||||
|
||||
expect(message).toContain("https://github.com/o/r/pull/7");
|
||||
expect(message).toContain("This is a stacked PR.");
|
||||
expect(message).toContain(patch);
|
||||
expect(message).not.toContain("Full-stack review of");
|
||||
expect(message).not.toContain("Review the complete diff");
|
||||
});
|
||||
|
||||
test("PR local-access is pure context — identical for default and custom", () => {
|
||||
const prMetadata = {
|
||||
url: "https://github.com/o/r/pull/9",
|
||||
baseBranch: "main",
|
||||
} as Parameters<typeof buildAgentReviewUserMessage>[3];
|
||||
const opts = { hasLocalAccess: true };
|
||||
|
||||
const dflt = buildAgentReviewUserMessage(patch, "branch", opts, prMetadata, false);
|
||||
const custom = buildAgentReviewUserMessage(patch, "branch", opts, prMetadata, true);
|
||||
|
||||
// This branch carries no framing prose, only context, so stripping does
|
||||
// nothing — the two must be byte-identical, and the diff instruction stays.
|
||||
expect(custom).toBe(dflt);
|
||||
expect(custom).toContain("git diff origin/main...HEAD");
|
||||
expect(custom).not.toContain("Provide prioritized");
|
||||
});
|
||||
|
||||
test("workspace drops the opening review line but keeps the path-reporting rules", () => {
|
||||
const message = buildAgentReviewUserMessageForTarget(
|
||||
{
|
||||
kind: "workspace",
|
||||
patch,
|
||||
workspace: {
|
||||
root: "/tmp/workspace",
|
||||
repos: [
|
||||
{ label: "api", cwd: "/tmp/workspace/api", changed: true, vcsType: "git", gitRef: "Uncommitted changes" },
|
||||
],
|
||||
},
|
||||
},
|
||||
true,
|
||||
);
|
||||
|
||||
expect(message).not.toContain("Review the local workspace changes");
|
||||
expect(message).toContain("must exactly match the path shown in the diff");
|
||||
expect(message).toContain("Do not use bare repo-relative paths like `src/file.ts`");
|
||||
expect(message).toContain("workspace root: /tmp/workspace");
|
||||
expect(message).toContain(patch);
|
||||
});
|
||||
});
|
||||
|
||||
describe("getLocalDiffInstruction", () => {
|
||||
test("returns null for non-local diff types", () => {
|
||||
expect(getLocalDiffInstruction("p4-default")).toBeNull();
|
||||
|
||||
@@ -79,32 +79,45 @@ export function buildWorkspacePromptContextLines(
|
||||
];
|
||||
}
|
||||
|
||||
export function buildAgentReviewUserMessageForTarget(target: AgentReviewTarget): string {
|
||||
export function buildAgentReviewUserMessageForTarget(
|
||||
target: AgentReviewTarget,
|
||||
contextOnly = false,
|
||||
): string {
|
||||
if (target.kind === "workspace") {
|
||||
return buildWorkspaceReviewUserMessage(target.patch, target.workspace);
|
||||
return buildWorkspaceReviewUserMessage(target.patch, target.workspace, contextOnly);
|
||||
}
|
||||
return buildAgentReviewUserMessage(
|
||||
target.patch,
|
||||
target.diffType,
|
||||
target.options,
|
||||
target.kind === "pr" ? target.prMetadata : undefined,
|
||||
contextOnly,
|
||||
);
|
||||
}
|
||||
|
||||
/** Build the dynamic user message shared by local Claude and Codex review jobs. */
|
||||
/**
|
||||
* Build the dynamic user message shared by local Claude and Codex review jobs.
|
||||
*
|
||||
* `contextOnly` strips the "Review… / provide findings" framing prose and keeps
|
||||
* only the git/PR context the agent needs to locate the changes. Used by custom
|
||||
* review skills, which carry their own instructions and must not inherit the
|
||||
* default review's framing. With `contextOnly` off the output is byte-identical
|
||||
* to today's prompt.
|
||||
*/
|
||||
export function buildAgentReviewUserMessage(
|
||||
patch: string,
|
||||
diffType: DiffType,
|
||||
options?: AgentReviewUserMessageOptions,
|
||||
prMetadata?: PRMetadata,
|
||||
contextOnly = false,
|
||||
): string {
|
||||
if (prMetadata) {
|
||||
if (options?.prDiffScope === "full-stack") {
|
||||
return [
|
||||
`Full-stack review of ${prMetadata.url}`,
|
||||
contextOnly ? prMetadata.url : `Full-stack review of ${prMetadata.url}`,
|
||||
"",
|
||||
"This is a stacked PR. The diff below shows ALL accumulated changes from the repository default branch through this PR's head (not just this PR's own layer).",
|
||||
"Review the complete diff for issues that span the stack.",
|
||||
...(contextOnly ? [] : ["Review the complete diff for issues that span the stack."]),
|
||||
"",
|
||||
"```diff",
|
||||
patch,
|
||||
@@ -112,6 +125,9 @@ export function buildAgentReviewUserMessage(
|
||||
].join("\n");
|
||||
}
|
||||
if (options?.hasLocalAccess) {
|
||||
// Pure context already (where the checkout is, how to diff against the
|
||||
// base) — no "Review… / provide findings" framing to strip, so this is
|
||||
// identical for default and custom reviews regardless of contextOnly.
|
||||
return [
|
||||
prMetadata.url,
|
||||
"",
|
||||
@@ -125,11 +141,14 @@ export function buildAgentReviewUserMessage(
|
||||
|
||||
const instruction = getLocalDiffInstruction(diffType, options?.defaultBranch);
|
||||
if (instruction) {
|
||||
if (contextOnly) {
|
||||
return `Changeset: ${instruction.target}.\n${instruction.inspect}`;
|
||||
}
|
||||
return `Review ${instruction.target}. ${instruction.inspect} Provide prioritized, actionable findings.`;
|
||||
}
|
||||
|
||||
return [
|
||||
"Review the following code changes and provide prioritized findings.",
|
||||
contextOnly ? "Code changes:" : "Review the following code changes and provide prioritized findings.",
|
||||
"",
|
||||
"```diff",
|
||||
patch,
|
||||
@@ -140,10 +159,12 @@ export function buildAgentReviewUserMessage(
|
||||
function buildWorkspaceReviewUserMessage(
|
||||
patch: string,
|
||||
workspace: WorkspaceReviewPromptContext,
|
||||
contextOnly = false,
|
||||
): string {
|
||||
return [
|
||||
"Review the local workspace changes across multiple nested VCS repositories.",
|
||||
"",
|
||||
...(contextOnly
|
||||
? []
|
||||
: ["Review the local workspace changes across multiple nested VCS repositories.", ""]),
|
||||
...buildWorkspacePromptContextLines(workspace, { includeReportingInstruction: true }),
|
||||
"",
|
||||
"```diff",
|
||||
|
||||
@@ -1,4 +1,9 @@
|
||||
import { toRelativePath } from "./path-utils";
|
||||
import {
|
||||
composeReviewPrompt,
|
||||
type ResolvedReviewProfile,
|
||||
} from "@plannotator/shared/review-profiles";
|
||||
import { classifyFindingPlacement } from "@plannotator/shared/external-annotation";
|
||||
|
||||
/**
|
||||
* Claude Code Review Agent — prompt, command builder, and JSONL output parser.
|
||||
@@ -20,9 +25,9 @@ export type ClaudeSeverity = "important" | "nit" | "pre_existing";
|
||||
|
||||
export interface ClaudeFinding {
|
||||
severity: ClaudeSeverity;
|
||||
file: string;
|
||||
line: number;
|
||||
end_line: number;
|
||||
file?: string | null; // null for a general (review-level) comment
|
||||
line?: number | null; // null for a whole-file or general comment
|
||||
end_line?: number | null;
|
||||
description: string;
|
||||
reasoning: string;
|
||||
}
|
||||
@@ -49,9 +54,13 @@ export const CLAUDE_REVIEW_SCHEMA_JSON = JSON.stringify({
|
||||
type: "object",
|
||||
properties: {
|
||||
severity: { type: "string", enum: ["important", "nit", "pre_existing"] },
|
||||
file: { type: "string" },
|
||||
line: { type: "integer" },
|
||||
end_line: { type: "integer" },
|
||||
// Nullable, not omitted: keep every property in `required` so the
|
||||
// schema is valid under strict structured-output validators too. A
|
||||
// whole-file finding sets line/end_line null; a general finding also
|
||||
// sets file null.
|
||||
file: { type: ["string", "null"] },
|
||||
line: { type: ["integer", "null"] },
|
||||
end_line: { type: ["integer", "null"] },
|
||||
description: { type: "string" },
|
||||
reasoning: { type: "string" },
|
||||
},
|
||||
@@ -161,6 +170,10 @@ Step 5: Deduplicate and rank
|
||||
- Within each severity, sort by file path and line number
|
||||
|
||||
Step 6: Return structured JSON output matching the schema.
|
||||
Place each finding by how specific it is: give file and line for a line-level
|
||||
issue; give file and set line null for a whole-file issue; set file and line
|
||||
null for a general, review-level note. Never invent a line you are unsure of —
|
||||
drop to a file or general placement instead of guessing.
|
||||
If no issues are found, return an empty findings array with zeroed summary.
|
||||
|
||||
## Hard constraints
|
||||
@@ -175,6 +188,23 @@ Step 6: Return structured JSON output matching the schema.
|
||||
- Do NOT use gh pr comment or any commenting tool
|
||||
- Your only output is the structured JSON findings`;
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Prompt composition
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
/**
|
||||
* Compose Claude's review prompt: the immutable system prompt, the resolved
|
||||
* profile's Custom Review Profile section (omitted for builtin:default), then
|
||||
* the user review message. For builtin:default / no profile the output is
|
||||
* byte-identical to today's `CLAUDE_REVIEW_PROMPT + "\n\n---\n\n" + userMessage`.
|
||||
*/
|
||||
export function composeClaudeReviewPrompt(
|
||||
userMessage: string,
|
||||
reviewProfile?: ResolvedReviewProfile,
|
||||
): string {
|
||||
return composeReviewPrompt(CLAUDE_REVIEW_PROMPT, reviewProfile, userMessage);
|
||||
}
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Command builder
|
||||
// ---------------------------------------------------------------------------
|
||||
@@ -296,23 +326,29 @@ export function transformClaudeFindings(
|
||||
reasoning: string;
|
||||
author: string;
|
||||
}> {
|
||||
return findings
|
||||
.filter(f => f.file && typeof f.line === "number")
|
||||
.map(f => ({
|
||||
// Route every finding by what it carries — nothing is dropped. A finding
|
||||
// with no usable file becomes a general comment; with a file but no line, a
|
||||
// whole-file comment; otherwise a line comment.
|
||||
return findings.map(f => {
|
||||
const rawFile = typeof f.file === "string" ? f.file : "";
|
||||
const filePath = rawFile
|
||||
? (pathTransform ? pathTransform(toRelativePath(rawFile, cwd)) : toRelativePath(rawFile, cwd))
|
||||
: "";
|
||||
const placement = classifyFindingPlacement(filePath, f.line, f.end_line);
|
||||
return {
|
||||
source,
|
||||
filePath: pathTransform
|
||||
? pathTransform(toRelativePath(f.file, cwd))
|
||||
: toRelativePath(f.file, cwd),
|
||||
lineStart: f.line,
|
||||
lineEnd: f.end_line ?? f.line,
|
||||
filePath: placement.filePath,
|
||||
lineStart: placement.lineStart,
|
||||
lineEnd: placement.lineEnd,
|
||||
type: "comment",
|
||||
side: "new",
|
||||
scope: "line",
|
||||
scope: placement.scope,
|
||||
text: `[${f.severity}] ${f.description}`,
|
||||
severity: f.severity,
|
||||
reasoning: f.reasoning,
|
||||
author: "Claude Code",
|
||||
}));
|
||||
};
|
||||
});
|
||||
}
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
@@ -11,6 +11,11 @@ import { appendFile, mkdir, unlink, writeFile, readFile } from "node:fs/promises
|
||||
import { existsSync } from "node:fs";
|
||||
import { toRelativePath } from "./path-utils";
|
||||
import { getPlannotatorDataDir } from "@plannotator/shared/data-dir";
|
||||
import {
|
||||
composeReviewPrompt,
|
||||
type ResolvedReviewProfile,
|
||||
} from "@plannotator/shared/review-profiles";
|
||||
import { classifyFindingPlacement } from "@plannotator/shared/external-annotation";
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Debug log — only active when PLANNOTATOR_DEBUG is set
|
||||
@@ -38,7 +43,7 @@ async function debugLog(label: string, data?: unknown): Promise<void> {
|
||||
// can't read, so we materialize the schema to a real file.
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
const CODEX_REVIEW_SCHEMA = JSON.stringify({
|
||||
export const CODEX_REVIEW_SCHEMA = JSON.stringify({
|
||||
type: "object",
|
||||
properties: {
|
||||
findings: {
|
||||
@@ -50,12 +55,15 @@ const CODEX_REVIEW_SCHEMA = JSON.stringify({
|
||||
body: { type: "string" },
|
||||
confidence_score: { type: "number" },
|
||||
priority: { type: ["integer", "null"] },
|
||||
// Nullable, not omittable: OpenAI strict structured output requires
|
||||
// every property to appear in `required`. A whole-file finding sets
|
||||
// line_range to null; a general finding sets code_location to null.
|
||||
code_location: {
|
||||
type: "object",
|
||||
type: ["object", "null"],
|
||||
properties: {
|
||||
absolute_file_path: { type: "string" },
|
||||
line_range: {
|
||||
type: "object",
|
||||
type: ["object", "null"],
|
||||
properties: {
|
||||
start: { type: "integer" },
|
||||
end: { type: "integer" },
|
||||
@@ -151,6 +159,8 @@ At the beginning of the finding title, tag the bug with priority level. For exam
|
||||
|
||||
Additionally, include a numeric priority field in the JSON output for each finding: set "priority" to 0 for P0, 1 for P1, 2 for P2, or 3 for P3. If a priority cannot be determined, omit the field or use null.
|
||||
|
||||
Place each finding by how specific it is. For a line-level issue, set code_location with the file and a line_range. For a whole-file issue, set code_location with the file path and line_range null. For a general, review-wide point, set code_location null. Do not invent a line range you are unsure of — drop to a whole-file or general placement instead.
|
||||
|
||||
At the end of your findings, output an "overall correctness" verdict of whether or not the patch should be considered "correct".
|
||||
Correct implies that existing code and tests will not break, and the patch is free of bugs and other blocking issues.
|
||||
Ignore non-blocking issues such as style, formatting, typos, documentation, and other nits.
|
||||
@@ -158,6 +168,24 @@ Ignore non-blocking issues such as style, formatting, typos, documentation, and
|
||||
FORMATTING GUIDELINES:
|
||||
The finding description should be one paragraph.`;
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Prompt composition
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
/**
|
||||
* Compose Codex's review prompt: the immutable system prompt, the resolved
|
||||
* profile's Custom Review Profile section (omitted for builtin:default), then
|
||||
* the user review message. For builtin:default / no profile the output is
|
||||
* byte-identical to today's
|
||||
* `CODEX_REVIEW_SYSTEM_PROMPT + "\n\n---\n\n" + userMessage`.
|
||||
*/
|
||||
export function composeCodexReviewPrompt(
|
||||
userMessage: string,
|
||||
reviewProfile?: ResolvedReviewProfile,
|
||||
): string {
|
||||
return composeReviewPrompt(CODEX_REVIEW_SYSTEM_PROMPT, reviewProfile, userMessage);
|
||||
}
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Command builder
|
||||
// ---------------------------------------------------------------------------
|
||||
@@ -212,7 +240,7 @@ export function generateOutputPath(): string {
|
||||
|
||||
export interface CodexCodeLocation {
|
||||
absolute_file_path: string;
|
||||
line_range: { start: number; end: number };
|
||||
line_range?: { start: number; end: number } | null; // null for a whole-file comment
|
||||
}
|
||||
|
||||
export interface CodexFinding {
|
||||
@@ -220,7 +248,7 @@ export interface CodexFinding {
|
||||
body: string;
|
||||
confidence_score: number;
|
||||
priority: number | null;
|
||||
code_location: CodexCodeLocation;
|
||||
code_location?: CodexCodeLocation | null; // null for a general (review-level) comment
|
||||
}
|
||||
|
||||
export interface CodexReviewOutput {
|
||||
@@ -295,25 +323,27 @@ export function transformReviewFindings(
|
||||
author?: string,
|
||||
pathTransform?: (path: string) => string,
|
||||
): ReviewAnnotationInput[] {
|
||||
const annotations = findings
|
||||
.filter((f) =>
|
||||
f.code_location?.absolute_file_path &&
|
||||
typeof f.code_location?.line_range?.start === "number" &&
|
||||
typeof f.code_location?.line_range?.end === "number"
|
||||
)
|
||||
.map((f) => ({
|
||||
// Route every finding by what it carries — nothing is dropped. No usable file
|
||||
// becomes a general comment; a file without a line range, a whole-file comment.
|
||||
const annotations = findings.map((f) => {
|
||||
const loc = f.code_location;
|
||||
const rawFile = loc && typeof loc.absolute_file_path === "string" ? loc.absolute_file_path : "";
|
||||
const filePath = rawFile
|
||||
? (pathTransform ? pathTransform(toRelativePath(rawFile, cwd)) : toRelativePath(rawFile, cwd))
|
||||
: "";
|
||||
const placement = classifyFindingPlacement(filePath, loc?.line_range?.start, loc?.line_range?.end);
|
||||
return {
|
||||
source,
|
||||
filePath: pathTransform
|
||||
? pathTransform(toRelativePath(f.code_location.absolute_file_path, cwd))
|
||||
: toRelativePath(f.code_location.absolute_file_path, cwd),
|
||||
lineStart: f.code_location.line_range.start,
|
||||
lineEnd: f.code_location.line_range.end,
|
||||
filePath: placement.filePath,
|
||||
lineStart: placement.lineStart,
|
||||
lineEnd: placement.lineEnd,
|
||||
type: "comment",
|
||||
side: "new",
|
||||
scope: "line",
|
||||
scope: placement.scope,
|
||||
text: `${f.title}\n\n${f.body}`.trim(),
|
||||
author: author ?? "Review Agent",
|
||||
}));
|
||||
};
|
||||
});
|
||||
|
||||
debugLog("TRANSFORM_FINDINGS", {
|
||||
inputCount: findings.length,
|
||||
|
||||
@@ -0,0 +1,117 @@
|
||||
/**
|
||||
* Ingestion-phase tests: a provider that exits 0 but returns empty/garbage
|
||||
* output fails the job instead of silently showing "done".
|
||||
*/
|
||||
import { describe, expect, test } from "bun:test";
|
||||
import { parseClaudeStreamOutput, transformClaudeFindings, CLAUDE_REVIEW_SCHEMA_JSON, type ClaudeFinding } from "./claude-review";
|
||||
import { transformReviewFindings, CODEX_REVIEW_SCHEMA, type CodexFinding } from "./codex-review";
|
||||
import { classifyFindingPlacement } from "@plannotator/shared/external-annotation";
|
||||
import {
|
||||
markJobReviewFailed,
|
||||
REVIEW_OUTPUT_FAILED,
|
||||
type AgentJobInfo,
|
||||
} from "@plannotator/shared/agent-jobs";
|
||||
|
||||
describe("completion semantics — empty/unparseable output fails the job", () => {
|
||||
// Guards the fix: a provider that exits 0 with nothing/garbage must fail the
|
||||
// job (REVIEW_OUTPUT_FAILED), not silently leave it "done" with no findings.
|
||||
test("parseClaudeStreamOutput returns null for empty/whitespace stdout", () => {
|
||||
expect(parseClaudeStreamOutput("")).toBeNull();
|
||||
expect(parseClaudeStreamOutput(" \n ")).toBeNull();
|
||||
});
|
||||
|
||||
test("parseClaudeStreamOutput returns null for unparseable stdout", () => {
|
||||
expect(parseClaudeStreamOutput("not json\n{ broken")).toBeNull();
|
||||
});
|
||||
|
||||
test("markJobReviewFailed flips the job to failed with a calm, leak-free reason", () => {
|
||||
const job = { status: "running" } as unknown as AgentJobInfo;
|
||||
markJobReviewFailed(job, REVIEW_OUTPUT_FAILED);
|
||||
expect(job.status).toBe("failed");
|
||||
expect(job.error).toBe(REVIEW_OUTPUT_FAILED);
|
||||
// No schema/CLI internals leaked in the user-facing reason.
|
||||
expect(REVIEW_OUTPUT_FAILED).not.toContain("stdout");
|
||||
expect(REVIEW_OUTPUT_FAILED).not.toContain("JSON");
|
||||
});
|
||||
});
|
||||
|
||||
describe("placement classifier — file + line, file only, or general", () => {
|
||||
test("file and a usable line → line", () => {
|
||||
expect(classifyFindingPlacement("src/a.ts", 10, 12)).toEqual({
|
||||
scope: "line", filePath: "src/a.ts", lineStart: 10, lineEnd: 12,
|
||||
});
|
||||
});
|
||||
|
||||
test("end line defaults to start when absent", () => {
|
||||
expect(classifyFindingPlacement("src/a.ts", 10, undefined)).toEqual({
|
||||
scope: "line", filePath: "src/a.ts", lineStart: 10, lineEnd: 10,
|
||||
});
|
||||
});
|
||||
|
||||
test("file but no line → file (line zeroed)", () => {
|
||||
expect(classifyFindingPlacement("src/a.ts", undefined, undefined)).toEqual({
|
||||
scope: "file", filePath: "src/a.ts", lineStart: 0, lineEnd: 0,
|
||||
});
|
||||
});
|
||||
|
||||
test("no file → general (path and line zeroed)", () => {
|
||||
expect(classifyFindingPlacement("", 10, 12)).toEqual({
|
||||
scope: "general", filePath: "", lineStart: 0, lineEnd: 0,
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
describe("Codex schema is OpenAI strict-mode compliant", () => {
|
||||
// Regression: OpenAI structured output requires every property of an object
|
||||
// with additionalProperties:false to appear in `required`. Optional fields
|
||||
// must be nullable-and-required, not omitted from `required`. Dropping a key
|
||||
// (e.g. line_range) from `required` makes codex exec fail with a 400 before
|
||||
// the review runs.
|
||||
function assertStrict(node: any, path: string): void {
|
||||
if (!node || typeof node !== "object") return;
|
||||
const types = Array.isArray(node.type) ? node.type : node.type ? [node.type] : [];
|
||||
if (types.includes("object") && node.properties) {
|
||||
expect(node.additionalProperties, `${path}: additionalProperties must be false`).toBe(false);
|
||||
const props = Object.keys(node.properties);
|
||||
const required: string[] = node.required ?? [];
|
||||
for (const p of props) {
|
||||
expect(required, `${path}: property "${p}" must be in required`).toContain(p);
|
||||
assertStrict(node.properties[p], `${path}.${p}`);
|
||||
}
|
||||
}
|
||||
if (types.includes("array") && node.items) assertStrict(node.items, `${path}[]`);
|
||||
}
|
||||
|
||||
test("Codex schema: every object property is required (nullable, never omitted)", () => {
|
||||
assertStrict(JSON.parse(CODEX_REVIEW_SCHEMA), "codex");
|
||||
});
|
||||
|
||||
test("Claude schema: every object property is required (nullable, never omitted)", () => {
|
||||
assertStrict(JSON.parse(CLAUDE_REVIEW_SCHEMA_JSON), "claude");
|
||||
});
|
||||
});
|
||||
|
||||
describe("transforms route every finding — nothing is dropped", () => {
|
||||
test("Claude: a finding with no file/line becomes a general comment, not a drop", () => {
|
||||
const findings: ClaudeFinding[] = [
|
||||
{ severity: "important", file: "src/a.ts", line: 3, end_line: 4, description: "bug", reasoning: "r" },
|
||||
{ severity: "nit", file: "src/b.ts", description: "whole file", reasoning: "r" },
|
||||
{ severity: "important", description: "overall approach is off", reasoning: "r" },
|
||||
];
|
||||
const out = transformClaudeFindings(findings, "claude");
|
||||
expect(out).toHaveLength(3);
|
||||
expect(out.map(a => a.scope)).toEqual(["line", "file", "general"]);
|
||||
expect(out[2].filePath).toBe("");
|
||||
});
|
||||
|
||||
test("Codex: missing code_location becomes general, missing line_range becomes file", () => {
|
||||
const findings: CodexFinding[] = [
|
||||
{ title: "[P1] x", body: "b", confidence_score: 1, priority: 1, code_location: { absolute_file_path: "/repo/src/a.ts", line_range: { start: 2, end: 5 } } },
|
||||
{ title: "[P2] y", body: "b", confidence_score: 1, priority: 2, code_location: { absolute_file_path: "/repo/src/b.ts" } },
|
||||
{ title: "[P2] z", body: "b", confidence_score: 1, priority: 2 },
|
||||
];
|
||||
const out = transformReviewFindings(findings, "codex", "/repo");
|
||||
expect(out).toHaveLength(3);
|
||||
expect(out.map(a => a.scope)).toEqual(["line", "file", "general"]);
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,130 @@
|
||||
import { describe, expect, test } from "bun:test";
|
||||
import { composeClaudeReviewPrompt, CLAUDE_REVIEW_PROMPT } from "./claude-review";
|
||||
import { composeCodexReviewPrompt, CODEX_REVIEW_SYSTEM_PROMPT } from "./codex-review";
|
||||
import { buildAgentReviewUserMessage } from "./agent-review-message";
|
||||
import {
|
||||
BUILTIN_DEFAULT_PROFILE,
|
||||
BUILTIN_DEFAULT_ID,
|
||||
type ResolvedReviewProfile,
|
||||
} from "@plannotator/shared/review-profiles";
|
||||
|
||||
// Stand-in for buildAgentReviewUserMessage(...) output. The composer treats it
|
||||
// as opaque text, so a literal is enough to pin placement and byte-equality.
|
||||
const userMessage = "Review of the current code changes.\n\n```diff\n+const x = 1;\n```";
|
||||
|
||||
const security: ResolvedReviewProfile = {
|
||||
id: "user:security",
|
||||
label: "Security",
|
||||
instructions: "Focus only on security-impacting issues.",
|
||||
source: "user",
|
||||
};
|
||||
|
||||
// The exact prompt today, before this phase, for the default path.
|
||||
const claudeDefault = CLAUDE_REVIEW_PROMPT + "\n\n---\n\n" + userMessage;
|
||||
const codexDefault = CODEX_REVIEW_SYSTEM_PROMPT + "\n\n---\n\n" + userMessage;
|
||||
|
||||
describe("review prompt composition — default is byte-identical", () => {
|
||||
test("Claude: absent profile matches today's prompt exactly", () => {
|
||||
expect(composeClaudeReviewPrompt(userMessage)).toBe(claudeDefault);
|
||||
});
|
||||
|
||||
test("Claude: builtin:default matches today's prompt exactly", () => {
|
||||
expect(composeClaudeReviewPrompt(userMessage, BUILTIN_DEFAULT_PROFILE)).toBe(claudeDefault);
|
||||
});
|
||||
|
||||
test("Codex: absent profile matches today's prompt exactly", () => {
|
||||
expect(composeCodexReviewPrompt(userMessage)).toBe(codexDefault);
|
||||
});
|
||||
|
||||
test("Codex: builtin:default matches today's prompt exactly", () => {
|
||||
expect(composeCodexReviewPrompt(userMessage, BUILTIN_DEFAULT_PROFILE)).toBe(codexDefault);
|
||||
});
|
||||
|
||||
test("a custom profile with empty instructions still yields the default prompt", () => {
|
||||
const blank: ResolvedReviewProfile = { ...security, instructions: " " };
|
||||
expect(composeClaudeReviewPrompt(userMessage, blank)).toBe(claudeDefault);
|
||||
expect(composeCodexReviewPrompt(userMessage, blank)).toBe(codexDefault);
|
||||
});
|
||||
|
||||
test("the reserved default id wins even if a profile claims source=user", () => {
|
||||
// A malformed profile that should never exist. The id guard must take
|
||||
// precedence so the reserved default can never be replaced.
|
||||
const reserved: ResolvedReviewProfile = {
|
||||
id: BUILTIN_DEFAULT_ID,
|
||||
label: "Reserved",
|
||||
instructions: "Custom instructions that must not be used.",
|
||||
source: "user",
|
||||
};
|
||||
expect(composeClaudeReviewPrompt(userMessage, reserved)).toBe(claudeDefault);
|
||||
expect(composeCodexReviewPrompt(userMessage, reserved)).toBe(codexDefault);
|
||||
});
|
||||
});
|
||||
|
||||
describe("custom review end to end — skill replaces, message is context-only", () => {
|
||||
test("custom skill prompt is the skill body plus the stripped context message", () => {
|
||||
const skill: ResolvedReviewProfile = {
|
||||
id: "skill:security",
|
||||
label: "Security",
|
||||
instructions: "Audit only authn and authz boundaries.",
|
||||
source: "user",
|
||||
};
|
||||
// review.ts wires isCustomReview === true to both the context-only message
|
||||
// and the replacing composer. This locks that combined contract.
|
||||
const contextMessage = buildAgentReviewUserMessage(
|
||||
"diff --git a/x b/x\n+x\n",
|
||||
"last-commit",
|
||||
{ defaultBranch: "origin/main" },
|
||||
undefined,
|
||||
true,
|
||||
);
|
||||
const prompt = composeClaudeReviewPrompt(contextMessage, skill);
|
||||
|
||||
expect(prompt).toContain("Audit only authn and authz boundaries.");
|
||||
expect(prompt).toContain("## Returning your findings");
|
||||
expect(prompt).toContain("git diff HEAD~1..HEAD");
|
||||
expect(prompt).not.toContain(CLAUDE_REVIEW_PROMPT);
|
||||
expect(prompt).not.toContain("Review the code changes introduced");
|
||||
expect(prompt).not.toContain("Provide prioritized, actionable findings.");
|
||||
});
|
||||
});
|
||||
|
||||
describe("review prompt composition — custom skill replaces the provider prompt", () => {
|
||||
test("Claude: skill body, then the reporting reminder, then the user message", () => {
|
||||
const prompt = composeClaudeReviewPrompt(userMessage, security);
|
||||
|
||||
expect(prompt).toStartWith("Focus only on security-impacting issues.");
|
||||
expect(prompt).toEndWith(userMessage);
|
||||
// Output-contract reminder is appended for custom skills.
|
||||
expect(prompt).toContain("## Returning your findings");
|
||||
// The default methodology is gone — no provider prompt, no section wrapper.
|
||||
expect(prompt).not.toContain(CLAUDE_REVIEW_PROMPT);
|
||||
expect(prompt).not.toContain("## Custom Review Profile");
|
||||
// Order: skill body → reporting reminder → user message.
|
||||
expect(prompt.indexOf("## Returning your findings")).toBeGreaterThan(prompt.indexOf("Focus only"));
|
||||
expect(prompt.indexOf(userMessage)).toBeGreaterThan(prompt.indexOf("## Returning your findings"));
|
||||
});
|
||||
|
||||
test("Codex: skill body, then the reporting reminder, then the user message", () => {
|
||||
const prompt = composeCodexReviewPrompt(userMessage, security);
|
||||
|
||||
expect(prompt).toStartWith("Focus only on security-impacting issues.");
|
||||
expect(prompt).toEndWith(userMessage);
|
||||
expect(prompt).toContain("## Returning your findings");
|
||||
expect(prompt).not.toContain(CODEX_REVIEW_SYSTEM_PROMPT);
|
||||
expect(prompt).not.toContain("## Custom Review Profile");
|
||||
});
|
||||
|
||||
test("the skill body is used verbatim, trimmed, with no label or source added", () => {
|
||||
const profile: ResolvedReviewProfile = {
|
||||
id: "skill:perf",
|
||||
label: "Performance",
|
||||
instructions: " Flag N+1 queries. ",
|
||||
source: "user",
|
||||
};
|
||||
const prompt = composeClaudeReviewPrompt(userMessage, profile);
|
||||
expect(prompt).toStartWith("Flag N+1 queries.\n\n## Returning your findings");
|
||||
expect(prompt).toEndWith(userMessage);
|
||||
expect(prompt).not.toContain("Profile:");
|
||||
expect(prompt).not.toContain("Source:");
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,351 @@
|
||||
import { afterEach, beforeEach, describe, expect, test } from "bun:test";
|
||||
import { mkdirSync, mkdtempSync, rmSync, symlinkSync, writeFileSync } from "node:fs";
|
||||
import { tmpdir } from "node:os";
|
||||
import { join } from "node:path";
|
||||
import { BUILTIN_DEFAULT_ID } from "@plannotator/shared/review-profiles";
|
||||
import {
|
||||
BUILTIN_DEFAULT_PROFILE,
|
||||
type ResolvedReviewProfile,
|
||||
} from "@plannotator/shared/review-profiles";
|
||||
import {
|
||||
discoverCuratedSkills,
|
||||
discoverSkills,
|
||||
enableReviewSkill,
|
||||
listAllSkills,
|
||||
loadReviewProfiles,
|
||||
readCuratedSkillNames,
|
||||
resolveRequestedReviewProfile,
|
||||
stripFrontmatter,
|
||||
} from "./review-skill-loader";
|
||||
|
||||
// Launch-time resolution used by review.ts / serverReview.ts. Tested directly so
|
||||
// both runtimes' resolution stays pinned without standing up a full review server.
|
||||
const resolveLaunchProfile = resolveRequestedReviewProfile;
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Test 1 — Body extraction (no frontmatter parsing)
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
describe("stripFrontmatter", () => {
|
||||
test("removes only the leading --- block and returns the body", () => {
|
||||
const raw = "---\nname: security-review\ndescription: x\n---\n# Body\n\ntext";
|
||||
expect(stripFrontmatter(raw)).toBe("# Body\n\ntext");
|
||||
});
|
||||
|
||||
test("no frontmatter → the whole file is the body", () => {
|
||||
const raw = "# Just a heading\n\nno frontmatter here";
|
||||
expect(stripFrontmatter(raw)).toBe(raw);
|
||||
});
|
||||
|
||||
test("an internal --- (markdown rule) in the body is not stripped", () => {
|
||||
const raw = "---\nname: x\n---\nintro\n\n---\n\nafter the rule";
|
||||
expect(stripFrontmatter(raw)).toBe("intro\n\n---\n\nafter the rule");
|
||||
});
|
||||
|
||||
test("empty body after frontmatter → empty string", () => {
|
||||
const raw = "---\nname: x\n---\n";
|
||||
expect(stripFrontmatter(raw)).toBe("");
|
||||
});
|
||||
|
||||
test("CRLF + BOM frontmatter is tolerated", () => {
|
||||
const raw = "---\r\nname: x\r\n---\r\nbody line";
|
||||
expect(stripFrontmatter(raw)).toBe("body line");
|
||||
});
|
||||
});
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Discovery / curation harness
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
let home: string;
|
||||
let dataDir: string;
|
||||
const savedEnv: Record<string, string | undefined> = {};
|
||||
|
||||
function setEnv(key: string, value: string | undefined) {
|
||||
if (!(key in savedEnv)) savedEnv[key] = process.env[key];
|
||||
if (value === undefined) delete process.env[key];
|
||||
else process.env[key] = value;
|
||||
}
|
||||
|
||||
/** Create a skill dir `<root>/<name>/SKILL.md` with the given body. */
|
||||
function writeSkill(root: string, name: string, body = `# ${name}\n\ninstructions`) {
|
||||
const dir = join(root, name);
|
||||
mkdirSync(dir, { recursive: true });
|
||||
writeFileSync(join(dir, "SKILL.md"), `---\nname: ${name}\n---\n${body}`);
|
||||
return dir;
|
||||
}
|
||||
|
||||
function writeCuration(enabled: unknown, version: unknown = 1) {
|
||||
writeFileSync(
|
||||
join(dataDir, "review-skills.json"),
|
||||
JSON.stringify({ version, enabled }),
|
||||
);
|
||||
}
|
||||
|
||||
beforeEach(() => {
|
||||
const base = mkdtempSync(join(tmpdir(), "plannotator-skills-"));
|
||||
home = join(base, "home");
|
||||
dataDir = join(base, "data");
|
||||
mkdirSync(home, { recursive: true });
|
||||
mkdirSync(dataDir, { recursive: true });
|
||||
|
||||
setEnv("PLANNOTATOR_DATA_DIR", dataDir);
|
||||
// Point every root at isolated dirs under the fake home so the host's real
|
||||
// ~/.claude etc. are never scanned. HOME isolates ~/.agents/skills, which has
|
||||
// no env override (Bun's homedir() honors HOME).
|
||||
setEnv("HOME", home);
|
||||
setEnv("CLAUDE_CONFIG_DIR", join(home, ".claude"));
|
||||
setEnv("CODEX_HOME", join(home, ".codex"));
|
||||
setEnv("XDG_CONFIG_HOME", join(home, ".config"));
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
for (const [key, value] of Object.entries(savedEnv)) {
|
||||
if (value === undefined) delete process.env[key];
|
||||
else process.env[key] = value;
|
||||
}
|
||||
for (const key of Object.keys(savedEnv)) delete savedEnv[key];
|
||||
rmSync(join(home, ".."), { recursive: true, force: true });
|
||||
});
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Test 2 — Root resolution + env overrides + realpath-dedup + clash
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
describe("discoverSkills — root resolution", () => {
|
||||
test("env overrides point discovery at the right dirs (all three roots)", () => {
|
||||
writeSkill(join(home, ".claude", "skills"), "claude-skill");
|
||||
writeSkill(join(home, ".codex", "skills"), "codex-skill");
|
||||
writeSkill(join(home, ".config", "agents", "skills"), "universal-skill");
|
||||
|
||||
const names = discoverSkills().map((s) => s.name).sort();
|
||||
expect(names).toEqual(["claude-skill", "codex-skill", "universal-skill"]);
|
||||
});
|
||||
|
||||
test("walks the skills/<category>/<skill> catalog layout one level deeper", () => {
|
||||
const claude = join(home, ".claude", "skills");
|
||||
writeSkill(join(claude, "category"), "nested-skill");
|
||||
|
||||
const found = discoverSkills().find((s) => s.name === "nested-skill");
|
||||
expect(found).toBeDefined();
|
||||
expect(found!.sourcePath).toBe(join(claude, "category", "nested-skill"));
|
||||
});
|
||||
|
||||
test("cross-root name clash → first-seen wins (Claude before Codex)", () => {
|
||||
writeSkill(join(home, ".claude", "skills"), "dup", "# claude version");
|
||||
writeSkill(join(home, ".codex", "skills"), "dup", "# codex version");
|
||||
|
||||
const dups = discoverSkills().filter((s) => s.name === "dup");
|
||||
expect(dups).toHaveLength(1);
|
||||
expect(dups[0].root).toBe("claude");
|
||||
});
|
||||
|
||||
test("two roots resolving to the same path dedupe (no double discovery)", () => {
|
||||
// Aim the Claude and Codex roots at one on-disk dir: CODEX_HOME is a symlink
|
||||
// to the real CLAUDE_CONFIG_DIR, so .claude/skills and .codex/skills realpath
|
||||
// to the same place and must collapse to one discovery.
|
||||
writeSkill(join(home, ".claude", "skills"), "shared-skill");
|
||||
symlinkSync(join(home, ".claude"), join(home, ".codex-link"));
|
||||
setEnv("CODEX_HOME", join(home, ".codex-link"));
|
||||
|
||||
const matches = discoverSkills().filter((s) => s.name === "shared-skill");
|
||||
expect(matches).toHaveLength(1);
|
||||
});
|
||||
});
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Test 3 — Curation filter (membership; missing name; absent/malformed)
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
describe("loadReviewProfiles — curation filter", () => {
|
||||
test("a discovered skill is a review iff its name is in `enabled`", () => {
|
||||
const root = join(home, ".claude", "skills");
|
||||
writeSkill(root, "security-review", "# Security\n\ncheck auth");
|
||||
writeSkill(root, "not-curated");
|
||||
writeCuration(["security-review"]);
|
||||
|
||||
const profiles = loadReviewProfiles();
|
||||
const ids = profiles.map((p) => p.id);
|
||||
expect(ids).toContain(BUILTIN_DEFAULT_ID);
|
||||
expect(ids).toContain("skill:security-review");
|
||||
expect(ids).not.toContain("skill:not-curated");
|
||||
|
||||
const sec = profiles.find((p) => p.id === "skill:security-review")!;
|
||||
expect(sec.label).toBe("security-review");
|
||||
expect(sec.source).toBe("user");
|
||||
expect(sec.instructions).toBe("# Security\n\ncheck auth");
|
||||
expect(sec.sourcePath).toBe(join(root, "security-review"));
|
||||
});
|
||||
|
||||
test("an enabled name with no matching skill is dropped (not fatal)", () => {
|
||||
writeSkill(join(home, ".claude", "skills"), "present");
|
||||
writeCuration(["present", "ghost"]);
|
||||
|
||||
const ids = loadReviewProfiles().map((p) => p.id);
|
||||
expect(ids).toContain("skill:present");
|
||||
expect(ids).not.toContain("skill:ghost");
|
||||
});
|
||||
|
||||
test("absent curation → only builtin:default", () => {
|
||||
writeSkill(join(home, ".claude", "skills"), "available");
|
||||
const profiles = loadReviewProfiles();
|
||||
expect(profiles).toHaveLength(1);
|
||||
expect(profiles[0].id).toBe(BUILTIN_DEFAULT_ID);
|
||||
});
|
||||
|
||||
test("malformed curation (bad version) → only builtin:default", () => {
|
||||
writeSkill(join(home, ".claude", "skills"), "available");
|
||||
writeCuration(["available"], 2);
|
||||
const profiles = loadReviewProfiles();
|
||||
expect(profiles).toHaveLength(1);
|
||||
expect(profiles[0].id).toBe(BUILTIN_DEFAULT_ID);
|
||||
});
|
||||
|
||||
test("empty enabled array → only builtin:default", () => {
|
||||
writeSkill(join(home, ".claude", "skills"), "available");
|
||||
writeCuration([]);
|
||||
const profiles = loadReviewProfiles();
|
||||
expect(profiles).toHaveLength(1);
|
||||
expect(profiles[0].id).toBe(BUILTIN_DEFAULT_ID);
|
||||
});
|
||||
});
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Test 5 — Trust gating: repo-local .claude/skills is NOT discovered
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Launch-time resolution — reviewProfileId → curated skill body; absent → default
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
describe("launch resolution", () => {
|
||||
test("a curated skill id resolves to that skill's live body", () => {
|
||||
const root = join(home, ".claude", "skills");
|
||||
writeSkill(root, "security-review", "# Security\n\ncheck auth");
|
||||
writeCuration(["security-review"]);
|
||||
|
||||
const profile = resolveLaunchProfile("skill:security-review");
|
||||
expect(profile.id).toBe("skill:security-review");
|
||||
expect(profile.label).toBe("security-review");
|
||||
expect(profile.source).toBe("user");
|
||||
expect(profile.instructions).toBe("# Security\n\ncheck auth");
|
||||
});
|
||||
|
||||
test("absent reviewProfileId → builtin:default", () => {
|
||||
writeSkill(join(home, ".claude", "skills"), "security-review");
|
||||
writeCuration(["security-review"]);
|
||||
expect(resolveLaunchProfile(undefined)).toBe(BUILTIN_DEFAULT_PROFILE);
|
||||
});
|
||||
|
||||
test("the reserved default id → builtin:default (no throw)", () => {
|
||||
expect(resolveLaunchProfile(BUILTIN_DEFAULT_ID)).toBe(BUILTIN_DEFAULT_PROFILE);
|
||||
});
|
||||
|
||||
test("an unknown / uncurated id throws instead of silently running default", () => {
|
||||
writeSkill(join(home, ".claude", "skills"), "not-curated");
|
||||
writeCuration([]);
|
||||
// Renamed/removed skill or stale cookie — fail loud, never quietly downgrade.
|
||||
expect(() => resolveLaunchProfile("skill:not-curated")).toThrow(/not available/);
|
||||
expect(() => resolveLaunchProfile("skill:does-not-exist")).toThrow(/not available/);
|
||||
});
|
||||
|
||||
test("a curated skill with an empty body throws (could not be loaded)", () => {
|
||||
writeSkill(join(home, ".claude", "skills"), "blank", "");
|
||||
writeCuration(["blank"]);
|
||||
expect(() => resolveLaunchProfile("skill:blank")).toThrow(/could not be loaded/);
|
||||
});
|
||||
});
|
||||
|
||||
describe("skill files pointer (point at the real folder, no copy)", () => {
|
||||
test("a skill with extra files prepends a pointer to its real directory", () => {
|
||||
const root = join(home, ".claude", "skills");
|
||||
const dir = writeSkill(root, "with-refs", "# Body\n\ncheck auth");
|
||||
mkdirSync(join(dir, "references"), { recursive: true });
|
||||
writeFileSync(join(dir, "references", "owasp.md"), "checklist");
|
||||
writeCuration(["with-refs"]);
|
||||
|
||||
const profile = resolveLaunchProfile("skill:with-refs");
|
||||
// Points at the skill's REAL directory — no copy is made.
|
||||
expect(
|
||||
profile.instructions.startsWith(
|
||||
`This review skill's files (references, scripts, assets) are at: ${dir}`,
|
||||
),
|
||||
).toBe(true);
|
||||
// The body still follows the pointer line.
|
||||
expect(profile.instructions.endsWith("# Body\n\ncheck auth")).toBe(true);
|
||||
});
|
||||
|
||||
test("an instruction-only skill (just SKILL.md) gets no pointer line", () => {
|
||||
writeSkill(join(home, ".claude", "skills"), "plain", "# Body\n\njust instructions");
|
||||
writeCuration(["plain"]);
|
||||
|
||||
const profile = resolveLaunchProfile("skill:plain");
|
||||
expect(profile.instructions).toBe("# Body\n\njust instructions");
|
||||
expect(profile.instructions).not.toContain("This review skill's files");
|
||||
});
|
||||
});
|
||||
|
||||
describe("trust gating — global roots only", () => {
|
||||
test("a repo-local .claude/skills/<name>/SKILL.md is not discovered", () => {
|
||||
// A project checkout living somewhere under the fake home, with its own
|
||||
// .claude/skills — must never be scanned (global-only).
|
||||
const repo = join(home, "work", "some-repo");
|
||||
writeSkill(join(repo, ".claude", "skills"), "repo-only-skill");
|
||||
writeCuration(["repo-only-skill"]);
|
||||
|
||||
const ids = loadReviewProfiles().map((p) => p.id);
|
||||
expect(ids).not.toContain("skill:repo-only-skill");
|
||||
expect(ids).toEqual([BUILTIN_DEFAULT_ID]);
|
||||
});
|
||||
});
|
||||
|
||||
describe("the documented ~/.agents/skills root is scanned", () => {
|
||||
test("a skill in ~/.agents/skills is discovered and loadable", () => {
|
||||
writeSkill(join(home, ".agents", "skills"), "agents-review");
|
||||
writeCuration(["agents-review"]);
|
||||
expect(loadReviewProfiles().map((p) => p.id)).toContain("skill:agents-review");
|
||||
});
|
||||
});
|
||||
|
||||
describe("listAllSkills — the add-a-review picker source", () => {
|
||||
test("lists every discovered skill, flagged by enabled state", () => {
|
||||
const root = join(home, ".claude", "skills");
|
||||
writeSkill(root, "security-review");
|
||||
writeSkill(root, "perf-review");
|
||||
writeCuration(["security-review"]);
|
||||
|
||||
const all = listAllSkills();
|
||||
const byName = new Map(all.map((s) => [s.name, s.enabled]));
|
||||
expect(byName.get("security-review")).toBe(true);
|
||||
expect(byName.get("perf-review")).toBe(false);
|
||||
});
|
||||
|
||||
test("no curation file → everything is not-enabled", () => {
|
||||
writeSkill(join(home, ".claude", "skills"), "perf-review");
|
||||
expect(listAllSkills().every((s) => !s.enabled)).toBe(true);
|
||||
});
|
||||
});
|
||||
|
||||
describe("enableReviewSkill — curation write", () => {
|
||||
test("adds a real skill name to review-skills.json (creates the file)", () => {
|
||||
writeSkill(join(home, ".claude", "skills"), "security-review");
|
||||
const { enabled } = enableReviewSkill("security-review");
|
||||
expect(enabled).toEqual(["security-review"]);
|
||||
expect([...(readCuratedSkillNames() ?? [])]).toEqual(["security-review"]);
|
||||
});
|
||||
|
||||
test("dedupes and preserves existing enabled names", () => {
|
||||
const root = join(home, ".claude", "skills");
|
||||
writeSkill(root, "security-review");
|
||||
writeSkill(root, "perf-review");
|
||||
writeCuration(["security-review"]);
|
||||
|
||||
enableReviewSkill("security-review"); // already enabled → no duplicate
|
||||
const { enabled } = enableReviewSkill("perf-review");
|
||||
expect(enabled.sort()).toEqual(["perf-review", "security-review"]);
|
||||
});
|
||||
|
||||
test("rejects a name with no matching discovered skill", () => {
|
||||
expect(() => enableReviewSkill("does-not-exist")).toThrow();
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,466 @@
|
||||
/**
|
||||
* Review Skill Loader
|
||||
*
|
||||
* A custom review is a curated Agent Skill. This loader discovers skills in the
|
||||
* user's *global* skill roots, filters them to the ones the user explicitly
|
||||
* curated in `${PLANNOTATOR_DATA_DIR}/review-skills.json`, and maps each into a
|
||||
* ResolvedReviewProfile whose `instructions` is the skill's SKILL.md body.
|
||||
*
|
||||
* Server-side (node:fs). Vendored to Pi. The runtime-agnostic prompt-composition
|
||||
* spine lives in @plannotator/shared/review-profiles; this file only does disk
|
||||
* I/O + curation, then hands a ResolvedReviewProfile to that composer.
|
||||
*
|
||||
* Trust model (v1): global, user-owned roots only (`~/.claude/skills`,
|
||||
* `~/.codex/skills`, `~/.config/agents/skills`), honoring the standard env
|
||||
* overrides (`CLAUDE_CONFIG_DIR`, `CODEX_HOME`, `XDG_CONFIG_HOME`). Project/repo
|
||||
* skills are NOT discovered (the fork-trust problem). See docs/custom-reviews.md.
|
||||
*
|
||||
* Skip-and-log discipline: an unreadable dir / file is skipped with one log
|
||||
* line and never throws. Read on each request — no file watching, no cache.
|
||||
*/
|
||||
|
||||
import {
|
||||
existsSync,
|
||||
mkdirSync,
|
||||
readdirSync,
|
||||
readFileSync,
|
||||
realpathSync,
|
||||
statSync,
|
||||
writeFileSync,
|
||||
} from "node:fs";
|
||||
import { homedir } from "node:os";
|
||||
import { join } from "node:path";
|
||||
import { getPlannotatorDataDir } from "@plannotator/shared/data-dir";
|
||||
import {
|
||||
BUILTIN_DEFAULT_PROFILE,
|
||||
type ResolvedReviewProfile,
|
||||
} from "@plannotator/shared/review-profiles";
|
||||
|
||||
/**
|
||||
* Oversized-body bound. A giant SKILL.md would blow up the review prompt; over
|
||||
* this length the skill is dropped with a log line and falls through to the
|
||||
* built-in default. This is the old MAX_INSTRUCTIONS_LEN value, re-homed here.
|
||||
*/
|
||||
export const MAX_SKILL_BODY_LEN = 20_000;
|
||||
|
||||
/** Directories never descended during discovery. */
|
||||
const SKIP_DIRS = new Set(["node_modules", ".git", "dist", "build", "__pycache__"]);
|
||||
|
||||
type SkillRoot = "claude" | "codex" | "universal";
|
||||
|
||||
/** A skill discovered on disk — catalog stage, no body read, no frontmatter read. */
|
||||
export interface DiscoveredSkill {
|
||||
/** The skill's directory name. */
|
||||
name: string;
|
||||
/** Absolute path to the skill directory. */
|
||||
sourcePath: string;
|
||||
/** Absolute path to SKILL.md. */
|
||||
skillMdPath: string;
|
||||
/** Which global root it came from. */
|
||||
root: SkillRoot;
|
||||
}
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Root resolution
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
/**
|
||||
* The ordered global skill roots, honoring env overrides. First-seen wins on a
|
||||
* cross-root name clash, so order matters: Claude → Codex → universal.
|
||||
*
|
||||
* Roots that resolve (via realpath) to the same on-disk directory are deduped,
|
||||
* keeping the first occurrence.
|
||||
*/
|
||||
export function resolveGlobalSkillRoots(): Array<{ dir: string; root: SkillRoot }> {
|
||||
// Prefer $HOME (where the user's dotfiles live, and what every other skill
|
||||
// tool keys off), falling back to the OS home. homedir() caches at process
|
||||
// start and ignores a later HOME, so $HOME is also what makes this testable.
|
||||
const home = process.env.HOME?.trim() || homedir();
|
||||
const claudeHome = process.env.CLAUDE_CONFIG_DIR?.trim() || join(home, ".claude");
|
||||
const codexHome = process.env.CODEX_HOME?.trim() || join(home, ".codex");
|
||||
// Universal root. Two locations are in the wild: the documented/de-facto
|
||||
// ~/.agents/skills (where the installer puts skills and Claude symlinks them)
|
||||
// and the XDG path ${XDG_CONFIG_HOME:-~/.config}/agents/skills. Scan both; the
|
||||
// realpath dedup below collapses them when they point at the same dir.
|
||||
const configHome = process.env.XDG_CONFIG_HOME?.trim() || join(home, ".config");
|
||||
|
||||
const candidates: Array<{ dir: string; root: SkillRoot }> = [
|
||||
{ dir: join(claudeHome, "skills"), root: "claude" },
|
||||
{ dir: join(codexHome, "skills"), root: "codex" },
|
||||
{ dir: join(home, ".agents", "skills"), root: "universal" },
|
||||
{ dir: join(configHome, "agents", "skills"), root: "universal" },
|
||||
];
|
||||
|
||||
// Dedup by realpath so two roots pointing at the same dir (e.g. via a symlink,
|
||||
// or CLAUDE_CONFIG_DIR and CODEX_HOME aimed at one place) collapse to one.
|
||||
// Keep first occurrence.
|
||||
const seen = new Set<string>();
|
||||
const roots: Array<{ dir: string; root: SkillRoot }> = [];
|
||||
for (const candidate of candidates) {
|
||||
let key: string;
|
||||
try {
|
||||
key = realpathSync(candidate.dir);
|
||||
} catch {
|
||||
// Dir doesn't exist or is unreadable; key on the literal path so a
|
||||
// non-existent root still dedupes against an identical literal.
|
||||
key = candidate.dir;
|
||||
}
|
||||
if (seen.has(key)) continue;
|
||||
seen.add(key);
|
||||
roots.push(candidate);
|
||||
}
|
||||
return roots;
|
||||
}
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Discovery
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
/** True iff `dir/SKILL.md` exists and is a regular file. */
|
||||
function hasSkillMd(dir: string): boolean {
|
||||
try {
|
||||
return statSync(join(dir, "SKILL.md")).isFile();
|
||||
} catch {
|
||||
return false;
|
||||
}
|
||||
}
|
||||
|
||||
/** List immediate subdirectories of `dir` (skipping known noise dirs), or []. */
|
||||
function listSubdirs(dir: string): string[] {
|
||||
let entries: import("node:fs").Dirent[];
|
||||
try {
|
||||
entries = readdirSync(dir, { withFileTypes: true });
|
||||
} catch (err) {
|
||||
console.error(
|
||||
`[plannotator] Could not read skill root ${dir}: ${
|
||||
err instanceof Error ? err.message : String(err)
|
||||
}`,
|
||||
);
|
||||
return [];
|
||||
}
|
||||
return entries
|
||||
.filter((e) => e.isDirectory() && !SKIP_DIRS.has(e.name))
|
||||
.map((e) => e.name);
|
||||
}
|
||||
|
||||
/**
|
||||
* Discover skills across the global roots. A skill is any directory containing a
|
||||
* SKILL.md; its `name` is the directory name (no frontmatter read).
|
||||
*
|
||||
* Container layout: roots are walked one extra level so the catalog layout
|
||||
* `skills/<category>/<skill>/SKILL.md` is found, matching the reference walk.
|
||||
* A child dir that itself holds a SKILL.md is taken as the skill and not
|
||||
* descended into.
|
||||
*
|
||||
* Dedup by skill `name` across roots — first-seen wins, ordered Claude → Codex
|
||||
* → universal (the same first-seen-wins clash story as the old JSON design).
|
||||
*/
|
||||
export function discoverSkills(): DiscoveredSkill[] {
|
||||
const byName = new Map<string, DiscoveredSkill>();
|
||||
|
||||
const add = (dir: string, root: SkillRoot) => {
|
||||
const name = dir.replace(/^.*[\\/]/, "");
|
||||
if (byName.has(name)) return; // first-seen wins on a cross-root name clash
|
||||
byName.set(name, {
|
||||
name,
|
||||
sourcePath: dir,
|
||||
skillMdPath: join(dir, "SKILL.md"),
|
||||
root,
|
||||
});
|
||||
};
|
||||
|
||||
for (const { dir: rootDir, root } of resolveGlobalSkillRoots()) {
|
||||
if (!existsSync(rootDir)) continue;
|
||||
|
||||
for (const childName of listSubdirs(rootDir)) {
|
||||
const childDir = join(rootDir, childName);
|
||||
if (hasSkillMd(childDir)) {
|
||||
add(childDir, root);
|
||||
continue; // don't descend past a discovered skill
|
||||
}
|
||||
// Walk one extra level for the `skills/<category>/<skill>/` catalog layout.
|
||||
for (const grandName of listSubdirs(childDir)) {
|
||||
const grandDir = join(childDir, grandName);
|
||||
if (hasSkillMd(grandDir)) add(grandDir, root);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
return [...byName.values()];
|
||||
}
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Body extraction (no frontmatter parsing)
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
/**
|
||||
* Return the SKILL.md body. We do NOT parse frontmatter — we strip only the
|
||||
* leading `---…---` block (a split, not a parse) so we don't inject YAML noise,
|
||||
* and return everything after it. CRLF/BOM safe.
|
||||
*
|
||||
* No leading `---` block → the whole file is the body.
|
||||
*/
|
||||
export function stripFrontmatter(raw: string): string {
|
||||
// Tolerate a UTF-8 BOM and either line ending.
|
||||
const text = raw.replace(/^/, "");
|
||||
const match = text.match(/^---\r?\n[\s\S]*?\r?\n---[ \t]*(?:\r?\n|$)/);
|
||||
if (!match) return text;
|
||||
return text.slice(match[0].length);
|
||||
}
|
||||
|
||||
/**
|
||||
* True iff the skill directory carries files beyond SKILL.md — `references/`,
|
||||
* `scripts/`, or `assets/` the body may point at by relative path. An unreadable
|
||||
* dir is treated as no extra files; never throws.
|
||||
*/
|
||||
function skillHasExtraFiles(sourcePath: string): boolean {
|
||||
let entries: string[];
|
||||
try {
|
||||
entries = readdirSync(sourcePath);
|
||||
} catch {
|
||||
return false;
|
||||
}
|
||||
return entries.some((name) => name !== "SKILL.md");
|
||||
}
|
||||
|
||||
/**
|
||||
* The one line prepended to a skill's instructions when it carries extra files,
|
||||
* pointing the agent at the skill's REAL directory (read-only, no copy). The
|
||||
* agent's working directory is the repository under review, not the skill dir,
|
||||
* so relative references/scripts/assets must resolve against this absolute base.
|
||||
* The agent reads those files on demand (progressive disclosure) straight from
|
||||
* where the skill already lives — which it can, since it shares the filesystem.
|
||||
*/
|
||||
function skillFilesPointerLine(skillDir: string): string {
|
||||
return `This review skill's files (references, scripts, assets) are at: ${skillDir}\nResolve any relative paths in the instructions below (e.g. references/, scripts/, assets/) against that absolute directory — the working directory is the repository under review, not the skill directory.`;
|
||||
}
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Curation
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
/**
|
||||
* Read the curated skill names from `${dataDir}/review-skills.json`.
|
||||
*
|
||||
* Schema (v1): `{ version: 1, enabled: string[] }`. `enabled` may be empty.
|
||||
* Anything that fails these checks — missing/non-1 `version`, `enabled` not an
|
||||
* array of strings, or unparseable JSON — is treated as no curation (zero
|
||||
* custom reviews), logged once. Absent file → no curation, silent.
|
||||
*
|
||||
* Returns the set of enabled names, or `null` when there is no valid curation.
|
||||
*/
|
||||
export function readCuratedSkillNames(): Set<string> | null {
|
||||
const path = join(getPlannotatorDataDir(), "review-skills.json");
|
||||
if (!existsSync(path)) return null;
|
||||
|
||||
let parsed: unknown;
|
||||
try {
|
||||
parsed = JSON.parse(readFileSync(path, "utf-8"));
|
||||
} catch (err) {
|
||||
console.error(
|
||||
`[plannotator] Ignoring malformed review-skills.json: ${
|
||||
err instanceof Error ? err.message : String(err)
|
||||
}`,
|
||||
);
|
||||
return null;
|
||||
}
|
||||
|
||||
if (typeof parsed !== "object" || parsed === null || Array.isArray(parsed)) {
|
||||
console.error("[plannotator] Ignoring review-skills.json: not an object.");
|
||||
return null;
|
||||
}
|
||||
const { version, enabled } = parsed as Record<string, unknown>;
|
||||
if (version !== 1) {
|
||||
console.error("[plannotator] Ignoring review-skills.json: version must be 1.");
|
||||
return null;
|
||||
}
|
||||
if (!Array.isArray(enabled) || !enabled.every((n) => typeof n === "string")) {
|
||||
console.error(
|
||||
"[plannotator] Ignoring review-skills.json: `enabled` must be an array of strings.",
|
||||
);
|
||||
return null;
|
||||
}
|
||||
return new Set(enabled as string[]);
|
||||
}
|
||||
|
||||
/** A discovered skill plus whether it is currently enabled as a review. */
|
||||
export interface CatalogSkill {
|
||||
name: string;
|
||||
root: SkillRoot;
|
||||
sourcePath: string;
|
||||
enabled: boolean;
|
||||
}
|
||||
|
||||
/**
|
||||
* Every discovered skill, each flagged with whether it is enabled as a review.
|
||||
* Drives the "add a review" picker: the user sees all their skills and turns one
|
||||
* on.
|
||||
*/
|
||||
export function listAllSkills(): CatalogSkill[] {
|
||||
const enabled = readCuratedSkillNames() ?? new Set<string>();
|
||||
return discoverSkills().map((s) => ({
|
||||
name: s.name,
|
||||
root: s.root,
|
||||
sourcePath: s.sourcePath,
|
||||
enabled: enabled.has(s.name),
|
||||
}));
|
||||
}
|
||||
|
||||
/**
|
||||
* Enable a skill as a review by adding its name to
|
||||
* `${dataDir}/review-skills.json`. Creates the file (and the data dir) if absent,
|
||||
* keeps `version: 1`, and dedupes. Returns the updated enabled list.
|
||||
*
|
||||
* Only a name that matches a real discovered skill is accepted, so curation never
|
||||
* points at something that is not there. A malformed existing file is replaced
|
||||
* with a clean one (it was already being ignored).
|
||||
*/
|
||||
export function enableReviewSkill(name: string): { enabled: string[] } {
|
||||
const known = new Set(discoverSkills().map((s) => s.name));
|
||||
if (!known.has(name)) {
|
||||
throw new Error(`No skill named "${name}" found in any global skill root.`);
|
||||
}
|
||||
const current = readCuratedSkillNames() ?? new Set<string>();
|
||||
current.add(name);
|
||||
const enabled = [...current];
|
||||
|
||||
const dir = getPlannotatorDataDir();
|
||||
mkdirSync(dir, { recursive: true });
|
||||
writeFileSync(
|
||||
join(dir, "review-skills.json"),
|
||||
JSON.stringify({ version: 1, enabled }, null, 2) + "\n",
|
||||
);
|
||||
return { enabled };
|
||||
}
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Load + map to ResolvedReviewProfile
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
/**
|
||||
* Map a discovered skill into the existing ResolvedReviewProfile contract so
|
||||
* nothing downstream learns the word "skill". The id is built inline as
|
||||
* `skill:<name>` (an id-string convention; `source` stays `"user"` and adds no
|
||||
* ReviewProfileSource variant). The body is read live at this call.
|
||||
*
|
||||
* Returns `null` when the body is over the size bound (dropped + logged) so the
|
||||
* caller falls through to the built-in default.
|
||||
*/
|
||||
export function resolveSkillProfile(skill: DiscoveredSkill): ResolvedReviewProfile | null {
|
||||
let raw: string;
|
||||
try {
|
||||
raw = readFileSync(skill.skillMdPath, "utf-8");
|
||||
} catch (err) {
|
||||
console.error(
|
||||
`[plannotator] Skipping review skill ${skill.name}: could not read ${
|
||||
skill.skillMdPath
|
||||
}: ${err instanceof Error ? err.message : String(err)}`,
|
||||
);
|
||||
return null;
|
||||
}
|
||||
|
||||
const body = stripFrontmatter(raw);
|
||||
if (!body.trim()) {
|
||||
console.error(
|
||||
`[plannotator] Skipping review skill ${skill.name}: SKILL.md body is empty.`,
|
||||
);
|
||||
return null;
|
||||
}
|
||||
if (body.length > MAX_SKILL_BODY_LEN) {
|
||||
console.error(
|
||||
`[plannotator] Skipping review skill ${skill.name}: SKILL.md body exceeds ${MAX_SKILL_BODY_LEN} chars.`,
|
||||
);
|
||||
return null;
|
||||
}
|
||||
|
||||
// When the skill carries extra files, point the agent at the skill's real
|
||||
// directory so its relative references resolve. No copy is made — the agent
|
||||
// reads those files live, on demand, from where the skill already lives.
|
||||
const instructions = skillHasExtraFiles(skill.sourcePath)
|
||||
? `${skillFilesPointerLine(skill.sourcePath)}\n\n${body}`
|
||||
: body;
|
||||
|
||||
return {
|
||||
id: `skill:${skill.name}`,
|
||||
label: skill.name,
|
||||
instructions,
|
||||
source: "user",
|
||||
sourcePath: skill.sourcePath,
|
||||
};
|
||||
}
|
||||
|
||||
/**
|
||||
* Discover global skills and filter to the curated set.
|
||||
*
|
||||
* A discovered skill becomes a curated review iff its `name` is in
|
||||
* `review-skills.json.enabled`. Names in `enabled` with no matching discovered
|
||||
* skill are dropped with one log line. Absent/malformed curation → empty.
|
||||
*/
|
||||
export function discoverCuratedSkills(): DiscoveredSkill[] {
|
||||
const enabled = readCuratedSkillNames();
|
||||
if (!enabled || enabled.size === 0) return [];
|
||||
|
||||
const discovered = discoverSkills();
|
||||
const byName = new Map(discovered.map((s) => [s.name, s]));
|
||||
|
||||
const curated: DiscoveredSkill[] = [];
|
||||
for (const name of enabled) {
|
||||
const skill = byName.get(name);
|
||||
if (skill) {
|
||||
curated.push(skill);
|
||||
} else {
|
||||
console.error(
|
||||
`[plannotator] Curated review skill "${name}" not found in any global skill root; skipping.`,
|
||||
);
|
||||
}
|
||||
}
|
||||
return curated;
|
||||
}
|
||||
|
||||
/**
|
||||
* Resolve the review profile a launch requested, or throw a clear error.
|
||||
*
|
||||
* The client only sends a reviewProfileId when the user picked a custom review,
|
||||
* so a non-default id that doesn't resolve is a real problem — a renamed or
|
||||
* removed skill, a stale cookie, a malformed request — not a reason to quietly
|
||||
* run the default against the wrong instructions. Explicit selection is
|
||||
* authoritative here. Absent or the reserved default id → the built-in default.
|
||||
*/
|
||||
export function resolveRequestedReviewProfile(
|
||||
requestedProfileId: string | undefined,
|
||||
): ResolvedReviewProfile {
|
||||
if (!requestedProfileId || requestedProfileId === BUILTIN_DEFAULT_PROFILE.id) {
|
||||
return BUILTIN_DEFAULT_PROFILE;
|
||||
}
|
||||
const skill = discoverCuratedSkills().find((s) => `skill:${s.name}` === requestedProfileId);
|
||||
if (!skill) {
|
||||
throw new Error(
|
||||
`Review "${requestedProfileId}" is not available — it may have been renamed or removed. Pick another review.`,
|
||||
);
|
||||
}
|
||||
const resolved = resolveSkillProfile(skill);
|
||||
if (!resolved) {
|
||||
throw new Error(
|
||||
`Review "${skill.name}" could not be loaded — its SKILL.md is unreadable, empty, or too large. Fix the skill or pick another review.`,
|
||||
);
|
||||
}
|
||||
return resolved;
|
||||
}
|
||||
|
||||
/**
|
||||
* Load and resolve review profiles from the curated skills + the built-in
|
||||
* default. Always returns at least `builtin:default` first.
|
||||
*
|
||||
* This is the entry the servers call (same shape as the old loader's
|
||||
* loadReviewProfiles). Bodies are read here, live — for the discovery endpoint
|
||||
* this is harmless (it only reads `id`/`label`/`source`/`sourcePath`); a future
|
||||
* catalog-only path can swap in `discoverCuratedSkills()` directly.
|
||||
*/
|
||||
export function loadReviewProfiles(): ResolvedReviewProfile[] {
|
||||
const profiles: ResolvedReviewProfile[] = [BUILTIN_DEFAULT_PROFILE];
|
||||
for (const skill of discoverCuratedSkills()) {
|
||||
const profile = resolveSkillProfile(skill);
|
||||
if (profile) profiles.push(profile);
|
||||
}
|
||||
return profiles;
|
||||
}
|
||||
+135
-38
@@ -36,7 +36,7 @@ import {
|
||||
checkoutPRHead,
|
||||
type PRDiffScope,
|
||||
} from "@plannotator/shared/pr-stack";
|
||||
import type { AgentJobInfo } from "@plannotator/shared/agent-jobs";
|
||||
import { type AgentJobInfo, REVIEW_OUTPUT_FAILED, markJobReviewFailed } from "@plannotator/shared/agent-jobs";
|
||||
import { getRepoInfo } from "./repo";
|
||||
import { handleImage, handleUpload, handleAgents, handleServerReady, handleDraftSave, handleDraftLoad, handleDraftDelete, handleFavicon, readDraftGenerationFromBody, readDraftGenerationFromUrl, type OpencodeClient } from "./shared-handlers";
|
||||
import { contentHash, deleteDraft } from "./draft";
|
||||
@@ -44,7 +44,7 @@ import { createEditorAnnotationHandler } from "./editor-annotations";
|
||||
import { createExternalAnnotationHandler } from "./external-annotations";
|
||||
import { createAgentJobHandler } from "./agent-jobs";
|
||||
import {
|
||||
CODEX_REVIEW_SYSTEM_PROMPT,
|
||||
composeCodexReviewPrompt,
|
||||
buildCodexCommand,
|
||||
generateOutputPath,
|
||||
parseCodexOutput,
|
||||
@@ -52,7 +52,7 @@ import {
|
||||
} from "./codex-review";
|
||||
import { buildAgentReviewUserMessage, buildAgentReviewUserMessageForTarget, type WorkspaceReviewPromptContext } from "./agent-review-message";
|
||||
import {
|
||||
CLAUDE_REVIEW_PROMPT,
|
||||
composeClaudeReviewPrompt,
|
||||
buildClaudeCommand,
|
||||
parseClaudeStreamOutput,
|
||||
transformClaudeFindings,
|
||||
@@ -66,6 +66,14 @@ import { isWSL } from "./browser";
|
||||
import { handleOpenInApps, handleOpenIn } from "./open-in";
|
||||
import type { LocalWorkspaceReview, WorkspaceDiffType } from "./review-workspace";
|
||||
import { handleCodeNavResolve, extractChangedFiles } from "./code-nav";
|
||||
import { discoverCuratedSkills, resolveRequestedReviewProfile, listAllSkills, enableReviewSkill } from "./review-skill-loader";
|
||||
import {
|
||||
BUILTIN_DEFAULT_PROFILE,
|
||||
type ReviewProfilesResponse,
|
||||
} from "@plannotator/shared/review-profiles";
|
||||
|
||||
// Review ingestion completion semantics (REVIEW_OUTPUT_FAILED,
|
||||
// markJobReviewFailed) now live in @plannotator/shared/agent-jobs.
|
||||
|
||||
// Re-export utilities
|
||||
export { isRemoteSession, getServerPort } from "./remote";
|
||||
@@ -448,6 +456,14 @@ export async function startReviewServer(
|
||||
const launchBase = currentBase;
|
||||
const launchScope = currentPRDiffScope;
|
||||
|
||||
const requestedProfileId =
|
||||
typeof config?.reviewProfileId === "string" ? config.reviewProfileId : undefined;
|
||||
// Resolve the requested review, or throw a clear error. An unresolvable
|
||||
// non-default id (renamed/removed skill, stale cookie, malformed request)
|
||||
// never silently downgrades to the default — explicit selection is
|
||||
// authoritative at this boundary.
|
||||
const reviewProfile = resolveRequestedReviewProfile(requestedProfileId);
|
||||
|
||||
// Agents run inside the PR checkout — wait out the background warmup so
|
||||
// the spawn-time getCwd() below resolves to a path that exists.
|
||||
let cwd: string;
|
||||
@@ -508,16 +524,20 @@ export async function startReviewServer(
|
||||
prMetadata: launchMetadata,
|
||||
config,
|
||||
});
|
||||
return built ? { ...built, prUrl: launchPrUrl, diffScope: launchDiffScope, diffContext } : built;
|
||||
return built ? { ...built, prUrl: launchPrUrl, diffScope: launchDiffScope, diffContext, reviewProfileId: reviewProfile.id, reviewProfileLabel: reviewProfile.label } : built;
|
||||
}
|
||||
|
||||
// A custom review skill carries its own instructions and becomes the whole
|
||||
// prompt; strip the default framing prose from the user message so only the
|
||||
// git/PR context remains. The default review keeps today's message verbatim.
|
||||
const isCustomReview = reviewProfile.source === "user";
|
||||
const userMessage = workspacePrompt
|
||||
? buildAgentReviewUserMessageForTarget({
|
||||
kind: "workspace",
|
||||
patch: launchPatch,
|
||||
workspace: workspacePrompt,
|
||||
})
|
||||
: buildAgentReviewUserMessage(launchPatch, launchDiffType as DiffType, userMessageOptions, launchMetadata);
|
||||
}, isCustomReview)
|
||||
: buildAgentReviewUserMessage(launchPatch, launchDiffType as DiffType, userMessageOptions, launchMetadata, isCustomReview);
|
||||
const jobLabel = workspacePrompt ? "Workspace Review" : "Code Review";
|
||||
|
||||
if (provider === "codex") {
|
||||
@@ -525,17 +545,17 @@ export async function startReviewServer(
|
||||
const reasoningEffort = typeof config?.reasoningEffort === "string" && config.reasoningEffort ? config.reasoningEffort : undefined;
|
||||
const fastMode = config?.fastMode === true;
|
||||
const outputPath = generateOutputPath();
|
||||
const prompt = CODEX_REVIEW_SYSTEM_PROMPT + "\n\n---\n\n" + userMessage;
|
||||
const prompt = composeCodexReviewPrompt(userMessage, reviewProfile);
|
||||
const command = await buildCodexCommand({ cwd, outputPath, prompt, model, reasoningEffort, fastMode });
|
||||
return { command, outputPath, prompt, cwd, label: jobLabel, model, reasoningEffort, fastMode: fastMode || undefined, prUrl: launchPrUrl, diffScope: launchDiffScope, diffContext };
|
||||
return { command, outputPath, prompt, cwd, label: jobLabel, model, reasoningEffort, fastMode: fastMode || undefined, prUrl: launchPrUrl, diffScope: launchDiffScope, diffContext, reviewProfileId: reviewProfile.id, reviewProfileLabel: reviewProfile.label };
|
||||
}
|
||||
|
||||
if (provider === "claude") {
|
||||
const model = typeof config?.model === "string" && config.model ? config.model : undefined;
|
||||
const effort = typeof config?.effort === "string" && config.effort ? config.effort : undefined;
|
||||
const prompt = CLAUDE_REVIEW_PROMPT + "\n\n---\n\n" + userMessage;
|
||||
const prompt = composeClaudeReviewPrompt(userMessage, reviewProfile);
|
||||
const { command, stdinPrompt } = buildClaudeCommand(prompt, model, effort);
|
||||
return { command, stdinPrompt, prompt, cwd, label: jobLabel, captureStdout: true, model, effort, prUrl: launchPrUrl, diffScope: launchDiffScope, diffContext };
|
||||
return { command, stdinPrompt, prompt, cwd, label: jobLabel, captureStdout: true, model, effort, prUrl: launchPrUrl, diffScope: launchDiffScope, diffContext, reviewProfileId: reviewProfile.id, reviewProfileLabel: reviewProfile.label };
|
||||
}
|
||||
|
||||
return null;
|
||||
@@ -553,10 +573,34 @@ export async function startReviewServer(
|
||||
prRepo: getDisplayRepo(jobPrMeta),
|
||||
} : jobPrUrl ? { prUrl: jobPrUrl } : {};
|
||||
|
||||
// Only tag annotations with a *custom* profile — the default review needs no tag.
|
||||
const profileLabel =
|
||||
job.reviewProfileId && job.reviewProfileId !== BUILTIN_DEFAULT_PROFILE.id
|
||||
? job.reviewProfileLabel
|
||||
: undefined;
|
||||
|
||||
// Map findings onto annotations and ingest. Shared by both engine branches;
|
||||
// no-ops on an empty set so a clean (zero-finding) review stays "done".
|
||||
const ingest = <T extends object>(transformed: readonly T[], logTag: string) => {
|
||||
if (transformed.length === 0) return;
|
||||
const annotations = transformed.map((a) => ({
|
||||
...a,
|
||||
...jobPrContext,
|
||||
...(jobDiffScope && { diffScope: jobDiffScope }),
|
||||
...(profileLabel && { reviewProfileLabel: profileLabel }),
|
||||
}));
|
||||
const result = externalAnnotations.addAnnotations({ annotations });
|
||||
if ("error" in result) console.error(`[${logTag}] addAnnotations error:`, result.error);
|
||||
};
|
||||
|
||||
// --- Codex path ---
|
||||
if (job.provider === "codex" && meta.outputPath) {
|
||||
const output = await parseCodexOutput(meta.outputPath);
|
||||
if (!output) return;
|
||||
if (job.provider === "codex") {
|
||||
const output = meta.outputPath ? await parseCodexOutput(meta.outputPath) : null;
|
||||
if (!output) {
|
||||
// Process exited 0 but output is missing/unparseable — not a green run.
|
||||
markJobReviewFailed(job, REVIEW_OUTPUT_FAILED);
|
||||
return;
|
||||
}
|
||||
|
||||
// Override verdict if there are blocking findings (P0/P1) — Codex's
|
||||
// freeform correctness string can say "mostly correct" with real bugs.
|
||||
@@ -567,47 +611,48 @@ export async function startReviewServer(
|
||||
confidence: output.overall_confidence_score,
|
||||
};
|
||||
|
||||
if (output.findings.length > 0) {
|
||||
const annotations = transformReviewFindings(
|
||||
ingest(
|
||||
transformReviewFindings(
|
||||
output.findings,
|
||||
job.source,
|
||||
cwd,
|
||||
"Codex",
|
||||
workspace ? (filePath) => workspace.normalizeAnnotationPath(filePath) : undefined,
|
||||
)
|
||||
.map(a => ({ ...a, ...jobPrContext, ...(jobDiffScope && { diffScope: jobDiffScope }) }));
|
||||
const result = externalAnnotations.addAnnotations({ annotations });
|
||||
if ("error" in result) console.error(`[codex-review] addAnnotations error:`, result.error);
|
||||
}
|
||||
),
|
||||
"codex-review",
|
||||
);
|
||||
return;
|
||||
}
|
||||
|
||||
// --- Claude path ---
|
||||
if (job.provider === "claude" && meta.stdout) {
|
||||
const output = parseClaudeStreamOutput(meta.stdout);
|
||||
if (job.provider === "claude") {
|
||||
const stdout = meta.stdout ?? "";
|
||||
const output = parseClaudeStreamOutput(stdout);
|
||||
if (!output) {
|
||||
console.error(`[claude-review] Failed to parse output (${meta.stdout.length} bytes, last 200: ${meta.stdout.slice(-200)})`);
|
||||
console.error(`[claude-review] Failed to parse output (${stdout.length} bytes, last 200: ${stdout.slice(-200)})`);
|
||||
markJobReviewFailed(job, REVIEW_OUTPUT_FAILED);
|
||||
return;
|
||||
}
|
||||
|
||||
const total = output.summary.important + output.summary.nit + output.summary.pre_existing;
|
||||
// Recompute the verdict from the findings we actually render. Nothing is
|
||||
// dropped now (un-pinnable findings become file/general comments), so the
|
||||
// count reflects reality and the card can never claim more than it shows.
|
||||
const transformed = transformClaudeFindings(
|
||||
output.findings,
|
||||
job.source,
|
||||
cwd,
|
||||
workspace ? (filePath) => workspace.normalizeAnnotationPath(filePath) : undefined,
|
||||
);
|
||||
const counts = { important: 0, nit: 0, pre_existing: 0 };
|
||||
for (const a of transformed) counts[a.severity]++;
|
||||
const total = counts.important + counts.nit + counts.pre_existing;
|
||||
job.summary = {
|
||||
correctness: output.summary.important === 0 ? "Correct" : "Issues Found",
|
||||
explanation: `${output.summary.important} important, ${output.summary.nit} nit, ${output.summary.pre_existing} pre-existing`,
|
||||
confidence: total === 0 ? 1.0 : Math.max(0, 1.0 - (output.summary.important * 0.2)),
|
||||
correctness: counts.important === 0 ? "Correct" : "Issues Found",
|
||||
explanation: `${counts.important} important, ${counts.nit} nit, ${counts.pre_existing} pre-existing`,
|
||||
confidence: total === 0 ? 1.0 : Math.max(0, 1.0 - (counts.important * 0.2)),
|
||||
};
|
||||
|
||||
if (output.findings.length > 0) {
|
||||
const annotations = transformClaudeFindings(
|
||||
output.findings,
|
||||
job.source,
|
||||
cwd,
|
||||
workspace ? (filePath) => workspace.normalizeAnnotationPath(filePath) : undefined,
|
||||
)
|
||||
.map(a => ({ ...a, ...jobPrContext, ...(jobDiffScope && { diffScope: jobDiffScope }) }));
|
||||
const result = externalAnnotations.addAnnotations({ annotations });
|
||||
if ("error" in result) console.error(`[claude-review] addAnnotations error:`, result.error);
|
||||
}
|
||||
ingest(transformed, "claude-review");
|
||||
return;
|
||||
}
|
||||
|
||||
@@ -1415,6 +1460,58 @@ export async function startReviewServer(
|
||||
return handleAgents(options.opencodeClient);
|
||||
}
|
||||
|
||||
// API: Review profiles (custom reviews discovery). Reloaded per
|
||||
// request, no file watching. Profiles come from the user dir plus
|
||||
// builtins.
|
||||
if (url.pathname === "/api/agents/review-profiles" && req.method === "GET") {
|
||||
// Catalog only — directory listing, no SKILL.md bodies read here.
|
||||
// Bodies are read at launch, for the one selected skill.
|
||||
const body: ReviewProfilesResponse = {
|
||||
profiles: [
|
||||
{
|
||||
id: BUILTIN_DEFAULT_PROFILE.id,
|
||||
label: BUILTIN_DEFAULT_PROFILE.label,
|
||||
source: BUILTIN_DEFAULT_PROFILE.source,
|
||||
default: BUILTIN_DEFAULT_PROFILE.default,
|
||||
},
|
||||
...discoverCuratedSkills().map((s) => ({
|
||||
id: `skill:${s.name}`,
|
||||
label: s.name,
|
||||
source: "user" as const,
|
||||
sourcePath: s.sourcePath,
|
||||
})),
|
||||
],
|
||||
};
|
||||
return Response.json(body);
|
||||
}
|
||||
|
||||
// API: All discovered skills, for the "add a review" picker. Each is
|
||||
// flagged with whether it is already enabled as a review.
|
||||
if (url.pathname === "/api/agents/skills" && req.method === "GET") {
|
||||
return Response.json({ skills: listAllSkills() });
|
||||
}
|
||||
|
||||
// API: Enable a skill as a review (curation write to review-skills.json).
|
||||
if (url.pathname === "/api/agents/review-skills" && req.method === "POST") {
|
||||
let name: unknown;
|
||||
try {
|
||||
({ name } = (await req.json()) as { name?: unknown });
|
||||
} catch {
|
||||
return Response.json({ error: "Invalid JSON" }, { status: 400 });
|
||||
}
|
||||
if (typeof name !== "string" || name.length === 0) {
|
||||
return Response.json({ error: "`name` is required." }, { status: 400 });
|
||||
}
|
||||
try {
|
||||
return Response.json(enableReviewSkill(name));
|
||||
} catch (err) {
|
||||
return Response.json(
|
||||
{ error: err instanceof Error ? err.message : "Could not enable review." },
|
||||
{ status: 400 },
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
// API: Annotation draft persistence
|
||||
if (url.pathname === "/api/draft") {
|
||||
if (req.method === "POST") return handleDraftSave(req, draftKey);
|
||||
|
||||
@@ -75,6 +75,10 @@ export interface AgentJobInfo {
|
||||
diffScope?: string;
|
||||
/** Diff context at launch time (see AgentJobDiffContext). */
|
||||
diffContext?: AgentJobDiffContext;
|
||||
/** Resolved review profile id at launch time (e.g. "builtin:default", "user:security"). */
|
||||
reviewProfileId?: string;
|
||||
/** Resolved review profile label — rides on findings so the UI can show a profile tag. */
|
||||
reviewProfileLabel?: string;
|
||||
}
|
||||
|
||||
export interface AgentCapability {
|
||||
@@ -130,3 +134,16 @@ export function isTerminalStatus(status: AgentJobStatus): boolean {
|
||||
export function jobSource(id: string): string {
|
||||
return "agent-" + id.slice(0, 8);
|
||||
}
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Review ingestion completion semantics
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
/** Calm, provider-neutral failure reason. Never leak schema/CLI internals. */
|
||||
export const REVIEW_OUTPUT_FAILED = "Review finished but produced no usable findings.";
|
||||
|
||||
/** Flip a job to failed with a calm one-liner (Code Tour precedent). */
|
||||
export function markJobReviewFailed(job: AgentJobInfo, error: string): void {
|
||||
job.status = "failed";
|
||||
job.error = error;
|
||||
}
|
||||
|
||||
@@ -0,0 +1,52 @@
|
||||
/**
|
||||
* Scope-aware review annotation validation: line requires a file and line,
|
||||
* file requires a file, general requires neither — so a general (review-level)
|
||||
* finding submits cleanly while a broken line finding is still rejected.
|
||||
*/
|
||||
import { describe, expect, test } from "bun:test";
|
||||
import { transformReviewInput } from "./external-annotation";
|
||||
|
||||
function ok(body: unknown) {
|
||||
const r = transformReviewInput(body);
|
||||
if ("error" in r) throw new Error(`expected ok, got error: ${r.error}`);
|
||||
return r.annotations;
|
||||
}
|
||||
|
||||
describe("transformReviewInput — scope-aware location requirements", () => {
|
||||
test("general: accepted with no filePath and no line", () => {
|
||||
const [a] = ok({ source: "claude", scope: "general", text: "overall approach is off" });
|
||||
expect(a.scope).toBe("general");
|
||||
expect(a.filePath).toBe("");
|
||||
expect(a.lineStart).toBe(0);
|
||||
expect(a.lineEnd).toBe(0);
|
||||
});
|
||||
|
||||
test("file: requires filePath, line optional and defaults to 0", () => {
|
||||
const [a] = ok({ source: "claude", scope: "file", filePath: "src/a.ts", text: "whole file" });
|
||||
expect(a.scope).toBe("file");
|
||||
expect(a.filePath).toBe("src/a.ts");
|
||||
expect(a.lineStart).toBe(0);
|
||||
|
||||
const missingFile = transformReviewInput({ source: "claude", scope: "file", text: "x" });
|
||||
expect("error" in missingFile && missingFile.error).toContain("filePath");
|
||||
});
|
||||
|
||||
test("line: still strictly requires filePath, lineStart, lineEnd", () => {
|
||||
const [a] = ok({ source: "claude", scope: "line", filePath: "src/a.ts", lineStart: 3, lineEnd: 5, text: "x" });
|
||||
expect(a.scope).toBe("line");
|
||||
expect(a.lineStart).toBe(3);
|
||||
|
||||
const noLine = transformReviewInput({ source: "claude", scope: "line", filePath: "src/a.ts", text: "x" });
|
||||
expect("error" in noLine && noLine.error).toContain("lineStart");
|
||||
});
|
||||
|
||||
test("default scope is line and keeps the strict line rule", () => {
|
||||
const noLine = transformReviewInput({ source: "claude", filePath: "src/a.ts", text: "x" });
|
||||
expect("error" in noLine && noLine.error).toContain("lineStart");
|
||||
});
|
||||
|
||||
test("an unknown scope is rejected", () => {
|
||||
const r = transformReviewInput({ source: "claude", scope: "review", text: "x" });
|
||||
expect("error" in r && r.error).toContain("invalid scope");
|
||||
});
|
||||
});
|
||||
@@ -190,14 +190,53 @@ interface ReviewAnnotation {
|
||||
createdAt: number;
|
||||
author?: string;
|
||||
source?: string;
|
||||
// Agent review metadata (optional — only set by Claude review findings)
|
||||
// Agent review metadata (optional — only set by agent review findings)
|
||||
severity?: string; // "important" | "nit" | "pre_existing"
|
||||
reasoning?: string; // Validation chain explaining how the issue was confirmed
|
||||
reviewProfileLabel?: string; // Custom review profile that produced this finding
|
||||
}
|
||||
|
||||
const VALID_REVIEW_TYPES = ["comment", "suggestion", "concern"];
|
||||
const VALID_SIDES = ["old", "new"];
|
||||
const VALID_SCOPES = ["line", "file"];
|
||||
const VALID_SCOPES = ["line", "file", "general"];
|
||||
|
||||
/** A review finding's placement, derived from what it carries. */
|
||||
export type FindingPlacement = {
|
||||
scope: "line" | "file" | "general";
|
||||
filePath: string;
|
||||
lineStart: number;
|
||||
lineEnd: number;
|
||||
};
|
||||
|
||||
/**
|
||||
* Classify an agent review finding by what it carries, so nothing is dropped:
|
||||
* file + a usable line → a line comment
|
||||
* file, no line → a whole-file comment
|
||||
* neither → a general (review-level) comment
|
||||
*
|
||||
* For file and general placements the line is 0; for general the path is "".
|
||||
* Consumers branch on `scope`, never on the sentinel values.
|
||||
*/
|
||||
export function classifyFindingPlacement(
|
||||
filePath: string,
|
||||
lineStart: number | null | undefined,
|
||||
lineEnd: number | null | undefined,
|
||||
): FindingPlacement {
|
||||
const hasFile = filePath.length > 0;
|
||||
const hasLine = typeof lineStart === "number";
|
||||
if (hasFile && hasLine) {
|
||||
return {
|
||||
scope: "line",
|
||||
filePath,
|
||||
lineStart,
|
||||
lineEnd: typeof lineEnd === "number" ? lineEnd : lineStart,
|
||||
};
|
||||
}
|
||||
if (hasFile) {
|
||||
return { scope: "file", filePath, lineStart: 0, lineEnd: 0 };
|
||||
}
|
||||
return { scope: "general", filePath: "", lineStart: 0, lineEnd: 0 };
|
||||
}
|
||||
|
||||
export function transformReviewInput(
|
||||
body: unknown,
|
||||
@@ -212,14 +251,40 @@ export function transformReviewInput(
|
||||
const source = requireString(obj, "source", i);
|
||||
if (typeof source !== "string") return source;
|
||||
|
||||
const filePath = requireString(obj, "filePath", i);
|
||||
if (typeof filePath !== "string") return filePath;
|
||||
|
||||
if (typeof obj.lineStart !== "number") {
|
||||
return { error: `annotations[${i}] missing required "lineStart" field` };
|
||||
// scope: optional, defaults to "line"
|
||||
const scope = typeof obj.scope === "string" ? obj.scope : "line";
|
||||
if (!VALID_SCOPES.includes(scope)) {
|
||||
return {
|
||||
error: `annotations[${i}] invalid scope "${scope}". Must be one of: ${VALID_SCOPES.join(", ")}`,
|
||||
};
|
||||
}
|
||||
if (typeof obj.lineEnd !== "number") {
|
||||
return { error: `annotations[${i}] missing required "lineEnd" field` };
|
||||
|
||||
// Location requirements depend on scope:
|
||||
// line → filePath + lineStart + lineEnd required. A finding that claims
|
||||
// a line must carry one, so a broken line finding is rejected
|
||||
// rather than quietly passing as a vaguer comment.
|
||||
// file → filePath required; line ignored (defaults to 0).
|
||||
// general → no file, no line (review-level; defaults to "" / 0).
|
||||
let filePath = "";
|
||||
let lineStart = 0;
|
||||
let lineEnd = 0;
|
||||
if (scope !== "general") {
|
||||
const fp = requireString(obj, "filePath", i);
|
||||
if (typeof fp !== "string") return fp;
|
||||
filePath = fp;
|
||||
if (scope === "line") {
|
||||
if (typeof obj.lineStart !== "number") {
|
||||
return { error: `annotations[${i}] missing required "lineStart" field` };
|
||||
}
|
||||
if (typeof obj.lineEnd !== "number") {
|
||||
return { error: `annotations[${i}] missing required "lineEnd" field` };
|
||||
}
|
||||
lineStart = obj.lineStart;
|
||||
lineEnd = obj.lineEnd;
|
||||
} else {
|
||||
lineStart = typeof obj.lineStart === "number" ? obj.lineStart : 0;
|
||||
lineEnd = typeof obj.lineEnd === "number" ? obj.lineEnd : 0;
|
||||
}
|
||||
}
|
||||
|
||||
// side: optional, defaults to "new"
|
||||
@@ -238,14 +303,6 @@ export function transformReviewInput(
|
||||
};
|
||||
}
|
||||
|
||||
// scope: optional, defaults to "line"
|
||||
const scope = typeof obj.scope === "string" ? obj.scope : "line";
|
||||
if (!VALID_SCOPES.includes(scope)) {
|
||||
return {
|
||||
error: `annotations[${i}] invalid scope "${scope}". Must be one of: ${VALID_SCOPES.join(", ")}`,
|
||||
};
|
||||
}
|
||||
|
||||
// Must have at least text or suggestedCode
|
||||
if (typeof obj.text !== "string" && typeof obj.suggestedCode !== "string") {
|
||||
return {
|
||||
@@ -258,8 +315,8 @@ export function transformReviewInput(
|
||||
type,
|
||||
scope,
|
||||
filePath,
|
||||
lineStart: obj.lineStart,
|
||||
lineEnd: obj.lineEnd,
|
||||
lineStart,
|
||||
lineEnd,
|
||||
side,
|
||||
text: typeof obj.text === "string" ? obj.text : undefined,
|
||||
suggestedCode: typeof obj.suggestedCode === "string" ? obj.suggestedCode : undefined,
|
||||
@@ -267,9 +324,10 @@ export function transformReviewInput(
|
||||
createdAt: Date.now(),
|
||||
author: typeof obj.author === "string" ? obj.author : undefined,
|
||||
source,
|
||||
// Agent review metadata (optional — only set by Claude review findings)
|
||||
// Agent review metadata (optional — only set by agent review findings)
|
||||
...(typeof obj.severity === "string" && { severity: obj.severity }),
|
||||
...(typeof obj.reasoning === "string" && { reasoning: obj.reasoning }),
|
||||
...(typeof obj.reviewProfileLabel === "string" && { reviewProfileLabel: obj.reviewProfileLabel }),
|
||||
});
|
||||
}
|
||||
|
||||
|
||||
@@ -55,7 +55,8 @@
|
||||
"./source-save-node": "./source-save-node.ts",
|
||||
"./browser-paths": "./browser-paths.ts",
|
||||
"./workspace-status": "./workspace-status.ts",
|
||||
"./open-in-apps": "./open-in-apps.ts"
|
||||
"./open-in-apps": "./open-in-apps.ts",
|
||||
"./review-profiles": "./review-profiles.ts"
|
||||
},
|
||||
"dependencies": {
|
||||
"@joplin/turndown-plugin-gfm": "^1.0.64",
|
||||
|
||||
@@ -0,0 +1,143 @@
|
||||
/**
|
||||
* Review Profiles — shared contract types + prompt-composition spine.
|
||||
*
|
||||
* Runtime-agnostic: no node:fs, no node:http, no Bun APIs. The loader that
|
||||
* reads custom reviews from disk lives in packages/server/review-skill-loader.ts
|
||||
* and maps each curated Agent Skill into a ResolvedReviewProfile that this
|
||||
* module's composer renders. Vendored to Pi.
|
||||
*
|
||||
* A review profile is a named bundle of review intent. A custom review skill
|
||||
* fully replaces the provider's system prompt — picking a review runs that
|
||||
* review. The built-in default carries no instructions, so the composer falls
|
||||
* back to the provider prompt and the default review stays byte-for-byte today's.
|
||||
*
|
||||
* See docs/custom-reviews.md.
|
||||
*/
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Types
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
/** Where a resolved profile came from. */
|
||||
export type ReviewProfileSource = "builtin" | "user";
|
||||
|
||||
export interface ReviewProfile {
|
||||
id: string;
|
||||
label: string;
|
||||
/** The injected review instructions. */
|
||||
instructions: string;
|
||||
description?: string;
|
||||
}
|
||||
|
||||
export interface ResolvedReviewProfile extends ReviewProfile {
|
||||
source: ReviewProfileSource;
|
||||
sourcePath?: string;
|
||||
/** True for builtin:default — surfaced to the picker as the pre-selected option. */
|
||||
default?: boolean;
|
||||
}
|
||||
|
||||
/** Response shape for `GET /api/agents/review-profiles`. */
|
||||
export interface ReviewProfilesResponse {
|
||||
profiles: Array<{
|
||||
id: string;
|
||||
label: string;
|
||||
description?: string;
|
||||
source: ReviewProfileSource;
|
||||
sourcePath?: string;
|
||||
default?: boolean;
|
||||
}>;
|
||||
}
|
||||
|
||||
/** Reserved id for the built-in default review. */
|
||||
export const BUILTIN_DEFAULT_ID = "builtin:default";
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Built-in profiles
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
/**
|
||||
* The built-in default review — today's review, preserved. It carries no
|
||||
* custom instructions, so the composer falls back to the provider prompt and
|
||||
* the default prompt stays byte-for-byte today's.
|
||||
*/
|
||||
export const BUILTIN_DEFAULT_PROFILE: ResolvedReviewProfile = {
|
||||
id: BUILTIN_DEFAULT_ID,
|
||||
label: "Default",
|
||||
instructions: "",
|
||||
source: "builtin",
|
||||
default: true,
|
||||
};
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Prompt composition
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
/**
|
||||
* A profile replaces the provider prompt only when it carries instructions and
|
||||
* isn't the reserved built-in default. The default (or any instruction-less
|
||||
* profile) falls back to the provider prompt, keeping it byte-for-byte today's.
|
||||
*/
|
||||
function profileHasCustomSection(profile: ResolvedReviewProfile | undefined): boolean {
|
||||
return (
|
||||
!!profile &&
|
||||
profile.id !== BUILTIN_DEFAULT_ID &&
|
||||
profile.instructions.trim().length > 0
|
||||
);
|
||||
}
|
||||
|
||||
/**
|
||||
* Output-contract reminder appended to a custom review skill's prompt. A skill
|
||||
* carries its own review methodology but does not know how Plannotator wants
|
||||
* results returned — without this, a verdict-style skill collapses good,
|
||||
* line-locatable findings into one block. This covers only HOW to report, never
|
||||
* WHAT to look for, so it never competes with the skill's own methodology.
|
||||
*/
|
||||
export const REPORTING_INSTRUCTIONS = `## Returning your findings
|
||||
|
||||
Hand back what you found as separate findings, not as one combined report.
|
||||
|
||||
- One finding per issue. Don't merge unrelated points into a single entry.
|
||||
- Anchor each finding where it belongs:
|
||||
- About specific code? Give the file and the line(s), so it attaches to that spot in the diff.
|
||||
- About a whole file? Give the file and leave the line out.
|
||||
- A review-wide point? Leave out both the file and the line.
|
||||
- Always produce the code-specific findings first.
|
||||
- If your instructions also ask for a final verdict, summary, or overall judgment, add it as its own review-wide finding with no file and no line. The verdict is in addition to the specific findings, never a replacement for them.
|
||||
|
||||
If the instructions above told you to produce a particular report layout or document, that was for your own reasoning. For the final result, return findings in the structure described here: the specific code findings, plus any verdict as a separate review-wide finding.`;
|
||||
|
||||
/**
|
||||
* Compose the full review prompt deterministically.
|
||||
*
|
||||
* Custom review skill:
|
||||
* <skill instructions>
|
||||
* ## Returning your findings … (output contract, see REPORTING_INSTRUCTIONS)
|
||||
* ---
|
||||
* <user message>
|
||||
*
|
||||
* The skill body fully replaces the provider's system prompt — picking a review
|
||||
* runs that review. The reporting reminder is appended so the skill's findings
|
||||
* come back in Plannotator's shape (line/file/general) instead of one block.
|
||||
*
|
||||
* Built-in default (or any instruction-less profile):
|
||||
* <provider immutable instructions>
|
||||
* ---
|
||||
* <user message>
|
||||
*
|
||||
* The default already states its own output contract, so it gets no reminder and
|
||||
* stays byte-identical to today's `systemPrompt + "\n\n---\n\n" + userMessage`.
|
||||
*/
|
||||
export function composeReviewPrompt(
|
||||
systemPrompt: string,
|
||||
profile: ResolvedReviewProfile | undefined,
|
||||
userMessage: string,
|
||||
): string {
|
||||
if (profileHasCustomSection(profile)) {
|
||||
return (
|
||||
(profile as ResolvedReviewProfile).instructions.trim() +
|
||||
"\n\n" + REPORTING_INSTRUCTIONS +
|
||||
"\n\n---\n\n" + userMessage
|
||||
);
|
||||
}
|
||||
return systemPrompt + "\n\n---\n\n" + userMessage;
|
||||
}
|
||||
@@ -1,4 +1,4 @@
|
||||
import React, { useState, useEffect, useMemo } from 'react';
|
||||
import React, { useState, useEffect, useMemo, useCallback } from 'react';
|
||||
import {
|
||||
Bot,
|
||||
Play,
|
||||
@@ -11,11 +11,14 @@ import {
|
||||
ExternalLink,
|
||||
ChevronDown,
|
||||
Zap,
|
||||
Plus,
|
||||
Search,
|
||||
} from 'lucide-react';
|
||||
import type { AgentJobInfo, AgentCapabilities } from '../types';
|
||||
import { isTerminalStatus } from '@plannotator/shared/agent-jobs';
|
||||
import { cn } from '../lib/utils';
|
||||
import { ReviewAgentsIcon } from './ReviewAgentsIcon';
|
||||
import { ClaudeIcon, CodexIcon } from './icons/AgentIcons';
|
||||
import { useAgentSettings } from '../hooks/useAgentSettings';
|
||||
import type { AgentEngine, AgentMode } from '../hooks/useAgentSettings';
|
||||
|
||||
@@ -78,10 +81,15 @@ const ENGINE_LABEL: Record<AgentEngine, string> = {
|
||||
codex: 'Codex',
|
||||
};
|
||||
|
||||
const ENGINE_ICON: Record<AgentEngine, React.FC<{ className?: string }>> = {
|
||||
claude: ClaudeIcon,
|
||||
codex: CodexIcon,
|
||||
};
|
||||
|
||||
interface AgentsTabProps {
|
||||
jobs: AgentJobInfo[];
|
||||
capabilities: AgentCapabilities | null;
|
||||
onLaunch: (params: { provider?: string; command?: string[]; label?: string; engine?: string; model?: string; reasoningEffort?: string; effort?: string; fastMode?: boolean }) => void;
|
||||
onLaunch: (params: { provider?: string; command?: string[]; label?: string; engine?: string; model?: string; reasoningEffort?: string; effort?: string; fastMode?: boolean; reviewProfileId?: string }) => void;
|
||||
onKillJob: (id: string) => void;
|
||||
onKillAll: () => void;
|
||||
externalAnnotations: Array<{ source?: string }>;
|
||||
@@ -226,7 +234,7 @@ function Toggle({ checked, onChange }: { checked: boolean; onChange: (v: boolean
|
||||
// model picker (whose 7–9 options rule out a segmented control). The popover
|
||||
// opens downward (`top-full`) because the launch panel is pinned to the top of
|
||||
// the tab.
|
||||
function SelectMenu({ value, options, onChange, icon, placeholder }: { value: string; options: Array<{ value: string; label: string }>; onChange: (v: string) => void; icon?: React.ReactNode; placeholder?: string }) {
|
||||
function SelectMenu({ value, options, onChange, icon, placeholder, footerAction }: { value: string; options: Array<{ value: string; label: string }>; onChange: (v: string) => void; icon?: React.ReactNode; placeholder?: string; footerAction?: { label: string; onClick: () => void } }) {
|
||||
const [open, setOpen] = useState(false);
|
||||
const current = options.find((o) => o.value === value);
|
||||
return (
|
||||
@@ -262,6 +270,22 @@ function SelectMenu({ value, options, onChange, icon, placeholder }: { value: st
|
||||
{o.label}
|
||||
</button>
|
||||
))}
|
||||
{footerAction && (
|
||||
<>
|
||||
<div className="my-1 border-t border-border/20" />
|
||||
<button
|
||||
type="button"
|
||||
onClick={() => {
|
||||
setOpen(false);
|
||||
footerAction.onClick();
|
||||
}}
|
||||
className="flex w-full items-center gap-2 rounded-lg px-2.5 py-1.5 text-[11px] text-muted-foreground transition-colors hover:bg-surface-1/50 hover:text-foreground"
|
||||
>
|
||||
<Plus className="shrink-0" size={11} />
|
||||
{footerAction.label}
|
||||
</button>
|
||||
</>
|
||||
)}
|
||||
</div>
|
||||
</>
|
||||
)}
|
||||
@@ -269,6 +293,125 @@ function SelectMenu({ value, options, onChange, icon, placeholder }: { value: st
|
||||
);
|
||||
}
|
||||
|
||||
// --- Add-a-review dialog: a type-ahead picker over every discovered skill ---
|
||||
|
||||
interface CatalogSkill {
|
||||
name: string;
|
||||
root: string;
|
||||
sourcePath: string;
|
||||
enabled: boolean;
|
||||
}
|
||||
|
||||
function AddReviewDialog({
|
||||
onClose,
|
||||
onEnabled,
|
||||
}: {
|
||||
onClose: () => void;
|
||||
onEnabled: (name: string) => void;
|
||||
}) {
|
||||
const [skills, setSkills] = useState<CatalogSkill[] | null>(null);
|
||||
const [query, setQuery] = useState('');
|
||||
const [busy, setBusy] = useState<string | null>(null);
|
||||
const [error, setError] = useState<string | null>(null);
|
||||
|
||||
useEffect(() => {
|
||||
let alive = true;
|
||||
fetch('/api/agents/skills')
|
||||
.then((r) => r.json())
|
||||
.then((d) => {
|
||||
if (alive) setSkills(Array.isArray(d.skills) ? d.skills : []);
|
||||
})
|
||||
.catch(() => {
|
||||
if (alive) setSkills([]);
|
||||
});
|
||||
return () => {
|
||||
alive = false;
|
||||
};
|
||||
}, []);
|
||||
|
||||
const candidates = useMemo(() => {
|
||||
const q = query.trim().toLowerCase();
|
||||
return (skills ?? [])
|
||||
.filter((s) => !s.enabled)
|
||||
.filter((s) => (q ? s.name.toLowerCase().includes(q) : true));
|
||||
}, [skills, query]);
|
||||
|
||||
const enable = async (name: string) => {
|
||||
setBusy(name);
|
||||
setError(null);
|
||||
try {
|
||||
const res = await fetch('/api/agents/review-skills', {
|
||||
method: 'POST',
|
||||
headers: { 'Content-Type': 'application/json' },
|
||||
body: JSON.stringify({ name }),
|
||||
});
|
||||
if (!res.ok) {
|
||||
const d = await res.json().catch(() => ({}));
|
||||
throw new Error(d.error ?? 'Could not add review.');
|
||||
}
|
||||
onEnabled(name);
|
||||
} catch (e) {
|
||||
setError(e instanceof Error ? e.message : 'Could not add review.');
|
||||
setBusy(null);
|
||||
}
|
||||
};
|
||||
|
||||
return (
|
||||
<div className="fixed inset-0 z-50 flex items-center justify-center p-4" role="dialog" aria-modal="true">
|
||||
<div className="absolute inset-0 bg-black/40" onClick={onClose} />
|
||||
<div className="relative z-10 flex max-h-[70vh] w-full max-w-sm flex-col overflow-hidden rounded-xl bg-card shadow-[var(--card-shadow)] ring-1 ring-border/20">
|
||||
<div className="flex items-center justify-between border-b border-border/40 px-3 py-2.5">
|
||||
<span className="text-[12px] font-medium text-foreground">Add a review</span>
|
||||
<button type="button" onClick={onClose} className="text-muted-foreground/50 hover:text-foreground">
|
||||
<X size={13} />
|
||||
</button>
|
||||
</div>
|
||||
|
||||
<div className="border-b border-border/40 p-2">
|
||||
<div className="flex items-center gap-2 rounded-lg border border-border/30 bg-surface-1/30 px-2.5 py-1.5">
|
||||
<Search className="shrink-0 text-muted-foreground/40" size={12} />
|
||||
<input
|
||||
autoFocus
|
||||
value={query}
|
||||
onChange={(e) => setQuery(e.target.value)}
|
||||
placeholder="Filter your skills"
|
||||
className="min-w-0 flex-1 bg-transparent text-[12px] text-foreground/90 outline-none placeholder:text-muted-foreground/40"
|
||||
/>
|
||||
</div>
|
||||
</div>
|
||||
|
||||
<div className="flex-1 overflow-y-auto p-1.5">
|
||||
{skills === null ? (
|
||||
<div className="flex items-center justify-center py-8 text-muted-foreground/40">
|
||||
<Loader2 className="animate-spin" size={14} />
|
||||
</div>
|
||||
) : candidates.length === 0 ? (
|
||||
<p className="px-2 py-8 text-center text-[11px] text-muted-foreground/40">
|
||||
{query ? 'No matching skills.' : 'No skills left to add.'}
|
||||
</p>
|
||||
) : (
|
||||
candidates.map((s) => (
|
||||
<button
|
||||
key={`${s.root}:${s.name}`}
|
||||
type="button"
|
||||
disabled={busy !== null}
|
||||
onClick={() => enable(s.name)}
|
||||
className="flex w-full items-center gap-2 rounded-lg px-2.5 py-1.5 text-left transition-colors hover:bg-surface-1/50 disabled:opacity-50"
|
||||
>
|
||||
<span className="min-w-0 flex-1 truncate text-[12px] text-foreground/90">{s.name}</span>
|
||||
<span className="shrink-0 text-[9px] uppercase tracking-wide text-muted-foreground/40">{s.root}</span>
|
||||
{busy === s.name ? <Loader2 className="shrink-0 animate-spin" size={11} /> : <Plus className="shrink-0 text-muted-foreground/40" size={11} />}
|
||||
</button>
|
||||
))
|
||||
)}
|
||||
</div>
|
||||
|
||||
{error && <p className="border-t border-border/40 px-3 py-2 text-[10px] text-red-500">{error}</p>}
|
||||
</div>
|
||||
</div>
|
||||
);
|
||||
}
|
||||
|
||||
// --- Job card ---
|
||||
|
||||
function JobCard({
|
||||
@@ -370,6 +513,7 @@ export const AgentsTab: React.FC<AgentsTabProps> = ({
|
||||
const {
|
||||
selectedMode,
|
||||
reviewEngine,
|
||||
reviewProfileId,
|
||||
tourEngine,
|
||||
claudeModel,
|
||||
claudeEffort,
|
||||
@@ -383,6 +527,7 @@ export const AgentsTab: React.FC<AgentsTabProps> = ({
|
||||
tourCodexFast,
|
||||
setSelectedMode,
|
||||
setReviewEngine,
|
||||
setReviewProfileId,
|
||||
setTourEngine,
|
||||
setClaudeModel,
|
||||
setClaudeEffort,
|
||||
@@ -396,6 +541,31 @@ export const AgentsTab: React.FC<AgentsTabProps> = ({
|
||||
setTourCodexFast,
|
||||
} = settings;
|
||||
|
||||
// Review profiles (built-in default plus the user's enabled skills). Loaded
|
||||
// from the discovery endpoint and refreshed after a skill is added.
|
||||
const [reviewProfiles, setReviewProfiles] = useState<Array<{ id: string; label: string; default?: boolean }>>([
|
||||
{ id: 'builtin:default', label: 'Default', default: true },
|
||||
]);
|
||||
// Until the list has loaded we can't tell a saved custom pick from a removed
|
||||
// one, so a launch in that window would silently fall back to Default. Gate
|
||||
// launch on this for a custom pick (see canLaunch).
|
||||
const [profilesLoaded, setProfilesLoaded] = useState(false);
|
||||
const [addReviewOpen, setAddReviewOpen] = useState(false);
|
||||
|
||||
const refreshReviewProfiles = useCallback(() => {
|
||||
fetch('/api/agents/review-profiles')
|
||||
.then((r) => r.json())
|
||||
.then((d) => {
|
||||
if (Array.isArray(d.profiles) && d.profiles.length > 0) setReviewProfiles(d.profiles);
|
||||
})
|
||||
.catch(() => {})
|
||||
.finally(() => setProfilesLoaded(true));
|
||||
}, []);
|
||||
|
||||
useEffect(() => {
|
||||
refreshReviewProfiles();
|
||||
}, [refreshReviewProfiles]);
|
||||
|
||||
const claudeAvailable = capabilities?.providers.some((p) => p.id === 'claude' && p.available) ?? false;
|
||||
const codexAvailable = capabilities?.providers.some((p) => p.id === 'codex' && p.available) ?? false;
|
||||
const tourAvailable = capabilities?.providers.some((p) => p.id === 'tour' && p.available) ?? false;
|
||||
@@ -465,10 +635,20 @@ export const AgentsTab: React.FC<AgentsTabProps> = ({
|
||||
[jobs],
|
||||
);
|
||||
|
||||
// A persisted review id can point at something not in the current list: the
|
||||
// profiles may not be loaded yet, or the skill was removed by hand. Treat
|
||||
// anything not in the list as Default, for both the dropdown and the launch.
|
||||
const effectiveReviewProfileId = reviewProfiles.some((p) => p.id === reviewProfileId)
|
||||
? reviewProfileId
|
||||
: 'builtin:default';
|
||||
|
||||
type LaunchParams = Parameters<typeof onLaunch>[0];
|
||||
const buildReviewLaunch = (engine: AgentEngine): LaunchParams => {
|
||||
// Carry the chosen review only when it is a custom one. Absent → the server
|
||||
// resolves to the built-in default.
|
||||
const review = effectiveReviewProfileId !== 'builtin:default' ? { reviewProfileId: effectiveReviewProfileId } : {};
|
||||
if (engine === 'claude') {
|
||||
return { provider: 'claude', label: 'Code Review', model: claudeModel, effort: claudeEffort };
|
||||
return { provider: 'claude', label: 'Code Review', model: claudeModel, effort: claudeEffort, ...review };
|
||||
}
|
||||
return {
|
||||
provider: 'codex',
|
||||
@@ -476,6 +656,7 @@ export const AgentsTab: React.FC<AgentsTabProps> = ({
|
||||
model: codexModel,
|
||||
reasoningEffort: codexReasoning,
|
||||
...(codexFast && { fastMode: true }),
|
||||
...review,
|
||||
};
|
||||
};
|
||||
const buildTourLaunch = (): LaunchParams => ({
|
||||
@@ -488,8 +669,12 @@ export const AgentsTab: React.FC<AgentsTabProps> = ({
|
||||
: { reasoningEffort: tourCodexReasoning, ...(tourCodexFast && { fastMode: true }) }),
|
||||
});
|
||||
|
||||
// For a custom pick, hold launch until the profile list has loaded — otherwise
|
||||
// the saved id can't be found yet and the launch would quietly run Default. A
|
||||
// Default pick has nothing to resolve, so it never waits.
|
||||
const reviewReady = profilesLoaded || reviewProfileId === 'builtin:default';
|
||||
const canLaunch = selectedMode === 'review'
|
||||
? engineAvailable(reviewEngine)
|
||||
? engineAvailable(reviewEngine) && reviewReady
|
||||
: selectedMode === 'tour'
|
||||
? tourAvailable && engineAvailable(tourEngine)
|
||||
: false;
|
||||
@@ -500,7 +685,6 @@ export const AgentsTab: React.FC<AgentsTabProps> = ({
|
||||
};
|
||||
|
||||
const modeOptions = availableModes.map((mode) => ({ value: mode, label: MODE_LABEL[mode] }));
|
||||
const engineOptions = availableEngines.map((engine) => ({ value: engine, label: ENGINE_LABEL[engine] }));
|
||||
const renderStaticChoice = (label: string, icon?: React.ReactNode) => (
|
||||
<div className="flex items-center gap-2 rounded-lg border border-border/30 bg-surface-1/30 px-2.5 py-1.5">
|
||||
{icon}
|
||||
@@ -508,19 +692,42 @@ export const AgentsTab: React.FC<AgentsTabProps> = ({
|
||||
</div>
|
||||
);
|
||||
|
||||
const renderEngineSelect = (value: AgentEngine, onChange: (engine: AgentEngine) => void) => (
|
||||
<ConfigRow label="Engine" stacked>
|
||||
{availableEngines.length > 1 ? (
|
||||
<SelectMenu
|
||||
value={value}
|
||||
options={engineOptions}
|
||||
onChange={(next) => onChange(next as AgentEngine)}
|
||||
/>
|
||||
) : (
|
||||
renderStaticChoice(engineOptions[0]?.label ?? ENGINE_LABEL[value])
|
||||
)}
|
||||
</ConfigRow>
|
||||
);
|
||||
const renderEngineSelect = (value: AgentEngine, onChange: (engine: AgentEngine) => void) => {
|
||||
const StaticIcon = ENGINE_ICON[value];
|
||||
return (
|
||||
<ConfigRow label="Engine" stacked>
|
||||
{availableEngines.length > 1 ? (
|
||||
// Tap an agent's mark to pick it — no dropdown.
|
||||
<div className="flex items-center gap-1.5">
|
||||
{availableEngines.map((engine) => {
|
||||
const Icon = ENGINE_ICON[engine];
|
||||
const selected = value === engine;
|
||||
return (
|
||||
<button
|
||||
key={engine}
|
||||
type="button"
|
||||
onClick={() => onChange(engine)}
|
||||
title={ENGINE_LABEL[engine]}
|
||||
aria-label={ENGINE_LABEL[engine]}
|
||||
aria-pressed={selected}
|
||||
className={cn(
|
||||
'flex h-9 w-9 items-center justify-center rounded-lg border transition-all',
|
||||
selected
|
||||
? 'border-primary/40 bg-primary/5'
|
||||
: 'border-border/30 bg-surface-1/30 opacity-40 hover:opacity-100',
|
||||
)}
|
||||
>
|
||||
<Icon className="h-5 w-5" />
|
||||
</button>
|
||||
);
|
||||
})}
|
||||
</div>
|
||||
) : (
|
||||
renderStaticChoice(ENGINE_LABEL[value], <StaticIcon className="h-4 w-4" />)
|
||||
)}
|
||||
</ConfigRow>
|
||||
);
|
||||
};
|
||||
|
||||
return (
|
||||
<div className="flex flex-col h-full">
|
||||
@@ -549,6 +756,14 @@ export const AgentsTab: React.FC<AgentsTabProps> = ({
|
||||
|
||||
{selectedMode === 'review' && (
|
||||
<>
|
||||
<ConfigRow label="Review" stacked>
|
||||
<SelectMenu
|
||||
value={effectiveReviewProfileId}
|
||||
options={reviewProfiles.map((p) => ({ value: p.id, label: p.label }))}
|
||||
onChange={setReviewProfileId}
|
||||
footerAction={{ label: 'Add new review', onClick: () => setAddReviewOpen(true) }}
|
||||
/>
|
||||
</ConfigRow>
|
||||
{renderEngineSelect(reviewEngine, setReviewEngine)}
|
||||
{reviewEngine === 'claude' && (
|
||||
<>
|
||||
@@ -657,6 +872,21 @@ export const AgentsTab: React.FC<AgentsTabProps> = ({
|
||||
</button>
|
||||
</div>
|
||||
)}
|
||||
|
||||
{addReviewOpen && (
|
||||
<AddReviewDialog
|
||||
onClose={() => setAddReviewOpen(false)}
|
||||
onEnabled={(name) => {
|
||||
const id = `skill:${name}`;
|
||||
// Add optimistically so the dropdown can select it immediately; the
|
||||
// refresh below reconciles against the server.
|
||||
setReviewProfiles((prev) => (prev.some((p) => p.id === id) ? prev : [...prev, { id, label: name }]));
|
||||
setReviewProfileId(id);
|
||||
setAddReviewOpen(false);
|
||||
refreshReviewProfiles();
|
||||
}}
|
||||
/>
|
||||
)}
|
||||
</div>
|
||||
);
|
||||
};
|
||||
|
||||
File diff suppressed because one or more lines are too long
@@ -22,7 +22,7 @@ interface UseAgentJobsReturn {
|
||||
jobs: AgentJobInfo[];
|
||||
jobLogs: Map<string, string>;
|
||||
capabilities: AgentCapabilities | null;
|
||||
launchJob: (params: { provider?: string; command?: string[]; label?: string; engine?: string; model?: string; reasoningEffort?: string; effort?: string; fastMode?: boolean }) => Promise<AgentJobInfo | null>;
|
||||
launchJob: (params: { provider?: string; command?: string[]; label?: string; engine?: string; model?: string; reasoningEffort?: string; effort?: string; fastMode?: boolean; reviewProfileId?: string }) => Promise<AgentJobInfo | null>;
|
||||
killJob: (id: string) => Promise<void>;
|
||||
killAll: () => Promise<void>;
|
||||
}
|
||||
@@ -168,6 +168,7 @@ export function useAgentJobs(
|
||||
reasoningEffort?: string;
|
||||
effort?: string;
|
||||
fastMode?: boolean;
|
||||
reviewProfileId?: string;
|
||||
}): Promise<AgentJobInfo | null> => {
|
||||
try {
|
||||
const res = await fetch(JOBS_URL, {
|
||||
|
||||
@@ -30,6 +30,7 @@ export type AgentEngine = 'claude' | 'codex';
|
||||
interface AgentSettingsState {
|
||||
selectedMode?: AgentMode;
|
||||
reviewEngine: AgentEngine;
|
||||
reviewProfileId: string;
|
||||
tourEngine: AgentEngine;
|
||||
claude: ClaudeSection;
|
||||
codex: CodexSection;
|
||||
@@ -40,6 +41,7 @@ interface AgentSettingsState {
|
||||
const initialState: AgentSettingsState = {
|
||||
selectedMode: 'review',
|
||||
reviewEngine: 'claude',
|
||||
reviewProfileId: 'builtin:default',
|
||||
tourEngine: 'claude',
|
||||
claude: { model: DEFAULT_CLAUDE_MODEL, perModel: {} },
|
||||
codex: { model: DEFAULT_CODEX_MODEL, perModel: {} },
|
||||
@@ -83,6 +85,7 @@ function readCookie(): AgentSettingsState {
|
||||
return {
|
||||
selectedMode: parseMode(parsed.selectedMode) ?? initialState.selectedMode,
|
||||
reviewEngine: parseEngine(parsed.reviewEngine),
|
||||
reviewProfileId: typeof parsed.reviewProfileId === 'string' ? parsed.reviewProfileId : 'builtin:default',
|
||||
tourEngine: parseEngine(parsed.tourEngine),
|
||||
claude: {
|
||||
model: typeof parsed.claude?.model === 'string' ? parsed.claude.model : DEFAULT_CLAUDE_MODEL,
|
||||
@@ -121,6 +124,10 @@ export function useAgentSettings() {
|
||||
setState((s) => ({ ...s, reviewEngine: engine }));
|
||||
}, []);
|
||||
|
||||
const setReviewProfileId = useCallback((id: string) => {
|
||||
setState((s) => ({ ...s, reviewProfileId: id }));
|
||||
}, []);
|
||||
|
||||
const setTourEngine = useCallback((engine: AgentEngine) => {
|
||||
setState((s) => ({ ...s, tourEngine: engine }));
|
||||
}, []);
|
||||
@@ -217,6 +224,7 @@ export function useAgentSettings() {
|
||||
return {
|
||||
selectedMode: state.selectedMode,
|
||||
reviewEngine: state.reviewEngine,
|
||||
reviewProfileId: state.reviewProfileId,
|
||||
tourEngine: state.tourEngine,
|
||||
claudeModel: state.claude.model,
|
||||
claudeEffort,
|
||||
@@ -230,6 +238,7 @@ export function useAgentSettings() {
|
||||
tourCodexFast,
|
||||
setSelectedMode,
|
||||
setReviewEngine,
|
||||
setReviewProfileId,
|
||||
setTourEngine,
|
||||
setClaudeModel,
|
||||
setClaudeEffort,
|
||||
|
||||
@@ -78,7 +78,10 @@ export interface DiffResult {
|
||||
|
||||
// Code Review Types
|
||||
export type CodeAnnotationType = 'comment' | 'suggestion' | 'concern';
|
||||
export type CodeAnnotationScope = 'line' | 'file';
|
||||
// 'general' is a review-level comment tied to no file and no line. For 'general'
|
||||
// (and the file-less case) filePath is "" and lineStart/lineEnd are 0 — consumers
|
||||
// must branch on scope, never read those sentinels as a real path or row.
|
||||
export type CodeAnnotationScope = 'line' | 'file' | 'general';
|
||||
|
||||
/** Conventional Comments label — see https://conventionalcomments.org */
|
||||
export type ConventionalLabel =
|
||||
@@ -118,6 +121,7 @@ export interface CodeAnnotation {
|
||||
source?: string; // External tool identifier (e.g., "eslint") — set when annotation comes from external API
|
||||
severity?: 'important' | 'nit' | 'pre_existing'; // Agent review severity (Claude)
|
||||
reasoning?: string; // Validation chain — how the issue was confirmed (Claude)
|
||||
reviewProfileLabel?: string; // Custom review that produced this finding — shown as a tag
|
||||
conventionalLabel?: ConventionalLabel;
|
||||
decorations?: ConventionalDecoration[];
|
||||
prUrl?: string;
|
||||
|
||||
Reference in New Issue
Block a user