refactor: simplify reviewer — discriminated union meta, minimal prompt
- ReviewerMeta → discriminated union: approved | rejected (with issues)
- Remove method-heavy prompt — agent has built-in code review capability
- Prompt now just says: project path, threadId for context, approve or reject
- No non-blocking suggestions (they get ignored anyway)
- ReviewerConfig simplified to just { cwd }
This commit is contained in:
@@ -29,6 +29,7 @@ function toolCallResponse(argsJson: string): Response {
|
|||||||
|
|
||||||
function makeCtx(): ThreadContext {
|
function makeCtx(): ThreadContext {
|
||||||
return {
|
return {
|
||||||
|
threadId: "01TEST00000000000000000000",
|
||||||
start: {
|
start: {
|
||||||
role: START,
|
role: START,
|
||||||
content: "task",
|
content: "task",
|
||||||
@@ -49,14 +50,14 @@ describe("createReviewerRole", () => {
|
|||||||
mock.restore();
|
mock.restore();
|
||||||
});
|
});
|
||||||
|
|
||||||
test("runs reviewer extract", async () => {
|
test("approved verdict", async () => {
|
||||||
globalThis.fetch = (() =>
|
globalThis.fetch = (() =>
|
||||||
Promise.resolve(
|
Promise.resolve(
|
||||||
toolCallResponse(JSON.stringify({ approved: true })),
|
toolCallResponse(JSON.stringify({ status: "approved" })),
|
||||||
)) as unknown as typeof fetch;
|
)) as unknown as typeof fetch;
|
||||||
|
|
||||||
const agent: AgentFn = async (_ctx, prompt) => {
|
const agent: AgentFn = async (_ctx, prompt) => {
|
||||||
expect(prompt).toContain("git diff");
|
expect(prompt).toContain("code reviewer");
|
||||||
expect(prompt).toContain(DEFAULT_REVIEWER_CONFIG.cwd);
|
expect(prompt).toContain(DEFAULT_REVIEWER_CONFIG.cwd);
|
||||||
return "review done";
|
return "review done";
|
||||||
};
|
};
|
||||||
@@ -64,16 +65,33 @@ describe("createReviewerRole", () => {
|
|||||||
const role = createReviewerRole(agent, {
|
const role = createReviewerRole(agent, {
|
||||||
provider,
|
provider,
|
||||||
dryRun: null,
|
dryRun: null,
|
||||||
dryRunMeta: { approved: true },
|
dryRunMeta: { status: "approved" },
|
||||||
});
|
});
|
||||||
const out = await role(makeCtx());
|
const out = await role(makeCtx());
|
||||||
expect(out.meta).toEqual({ approved: true });
|
expect(out.meta).toEqual({ status: "approved" });
|
||||||
});
|
});
|
||||||
|
|
||||||
test("includes uncaged-workflow thread hint when threadId set", async () => {
|
test("rejected verdict with issues", async () => {
|
||||||
globalThis.fetch = (() =>
|
globalThis.fetch = (() =>
|
||||||
Promise.resolve(
|
Promise.resolve(
|
||||||
toolCallResponse(JSON.stringify({ approved: false })),
|
toolCallResponse(JSON.stringify({ status: "rejected", issues: ["secrets in code"] })),
|
||||||
|
)) as unknown as typeof fetch;
|
||||||
|
|
||||||
|
const agent: AgentFn = async () => "found problems";
|
||||||
|
|
||||||
|
const role = createReviewerRole(agent, {
|
||||||
|
provider,
|
||||||
|
dryRun: null,
|
||||||
|
dryRunMeta: { status: "approved" },
|
||||||
|
});
|
||||||
|
const out = await role(makeCtx());
|
||||||
|
expect(out.meta).toEqual({ status: "rejected", issues: ["secrets in code"] });
|
||||||
|
});
|
||||||
|
|
||||||
|
test("prompt includes threadId from context", async () => {
|
||||||
|
globalThis.fetch = (() =>
|
||||||
|
Promise.resolve(
|
||||||
|
toolCallResponse(JSON.stringify({ status: "approved" })),
|
||||||
)) as unknown as typeof fetch;
|
)) as unknown as typeof fetch;
|
||||||
|
|
||||||
let seen = "";
|
let seen = "";
|
||||||
@@ -84,15 +102,10 @@ describe("createReviewerRole", () => {
|
|||||||
|
|
||||||
const role = createReviewerRole(
|
const role = createReviewerRole(
|
||||||
agent,
|
agent,
|
||||||
{ provider, dryRun: null, dryRunMeta: { approved: false } },
|
{ provider, dryRun: null, dryRunMeta: { status: "approved" } },
|
||||||
{
|
{ cwd: "/proj" },
|
||||||
cwd: "/proj",
|
|
||||||
conventionsPath: null,
|
|
||||||
extraChecks: [],
|
|
||||||
threadId: "01ABCDEF234567890ABCDEFGH",
|
|
||||||
},
|
|
||||||
);
|
);
|
||||||
await role(makeCtx());
|
await role(makeCtx());
|
||||||
expect(seen).toContain("uncaged-workflow thread 01ABCDEF234567890ABCDEFGH");
|
expect(seen).toContain("uncaged-workflow thread 01TEST00000000000000000000");
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|||||||
@@ -3,91 +3,52 @@ import { createRole } from "@uncaged/workflow-agent-llm";
|
|||||||
import type { LlmProvider } from "@uncaged/workflow-util-role";
|
import type { LlmProvider } from "@uncaged/workflow-util-role";
|
||||||
import * as z from "zod/v4";
|
import * as z from "zod/v4";
|
||||||
|
|
||||||
export const reviewerMetaSchema = z.object({
|
export const reviewerMetaSchema = z.discriminatedUnion("status", [
|
||||||
approved: z.boolean().describe("true if the diff is clean and ready to merge"),
|
z.object({
|
||||||
});
|
status: z.literal("approved"),
|
||||||
|
}),
|
||||||
|
z.object({
|
||||||
|
status: z.literal("rejected"),
|
||||||
|
issues: z.array(z.string()).describe("blocking issues that must be fixed"),
|
||||||
|
}),
|
||||||
|
]);
|
||||||
export type ReviewerMeta = z.infer<typeof reviewerMetaSchema>;
|
export type ReviewerMeta = z.infer<typeof reviewerMetaSchema>;
|
||||||
|
|
||||||
export type ReviewerConfig = {
|
export type ReviewerConfig = {
|
||||||
cwd: string;
|
cwd: string;
|
||||||
conventionsPath: string | null;
|
|
||||||
extraChecks: ReadonlyArray<string>;
|
|
||||||
/** When non-null, prompts reference `uncaged-workflow thread <id>`. */
|
|
||||||
threadId: string | null;
|
|
||||||
};
|
};
|
||||||
|
|
||||||
export const DEFAULT_REVIEWER_CONFIG: ReviewerConfig = {
|
export const DEFAULT_REVIEWER_CONFIG: ReviewerConfig = {
|
||||||
cwd: ".",
|
cwd: ".",
|
||||||
conventionsPath: "CONVENTIONS.md",
|
|
||||||
extraChecks: [],
|
|
||||||
threadId: null,
|
|
||||||
};
|
};
|
||||||
|
|
||||||
function summarizeThreadContext(ctx: ThreadContext): string {
|
|
||||||
const lines: string[] = [`Initial prompt:\n${ctx.start.content}`];
|
|
||||||
for (const step of ctx.steps) {
|
|
||||||
const snippet = step.content.length > 600 ? `${step.content.slice(0, 600)}…` : step.content;
|
|
||||||
lines.push(`\n### ${step.role}\n${snippet}`);
|
|
||||||
}
|
|
||||||
return lines.join("\n");
|
|
||||||
}
|
|
||||||
|
|
||||||
function reviewerPrompt(config: ReviewerConfig, ctx: ThreadContext): string {
|
function reviewerPrompt(config: ReviewerConfig, ctx: ThreadContext): string {
|
||||||
const { cwd, conventionsPath, extraChecks, threadId } = config;
|
const { cwd } = config;
|
||||||
|
|
||||||
const conventionsBlock =
|
return `You are a code reviewer. The project is at \`${cwd}\`.
|
||||||
conventionsPath !== null ? `Read project conventions: \`cat ${cwd}/${conventionsPath}\`\n` : "";
|
|
||||||
|
|
||||||
const threadBlock =
|
## Context
|
||||||
threadId !== null
|
|
||||||
? `Read the workflow thread for context: \`uncaged-workflow thread ${threadId}\`\n`
|
|
||||||
: `## Thread context (no thread id)\n\n${summarizeThreadContext(ctx)}\n`;
|
|
||||||
|
|
||||||
const extraBlock =
|
Use \`uncaged-workflow thread ${ctx.threadId}\` to read the full workflow thread for context on what was done and why.
|
||||||
extraChecks.length > 0
|
|
||||||
? `\n### Project-specific checks\n${extraChecks.map((c) => `- ${c}`).join("\n")}\n`
|
|
||||||
: "";
|
|
||||||
|
|
||||||
return `You are a **code reviewer**. You run after the coder and before the tester.
|
## Task
|
||||||
|
|
||||||
**IMPORTANT: The project is at \`${cwd}\`. Always \`cd ${cwd}\` first.**
|
Review the current git diff in \`${cwd}\`. Give a clear **approve** or **reject** verdict.
|
||||||
|
|
||||||
${threadBlock}
|
Only reject for **blocking issues** — things that must be fixed before merge. Do not mention minor style preferences or non-blocking suggestions; they will be ignored.
|
||||||
${conventionsBlock}
|
|
||||||
## Your job — static analysis of the git diff
|
|
||||||
|
|
||||||
Run these commands and analyze the output:
|
End with your verdict:
|
||||||
|
|
||||||
1. **\`cd ${cwd} && git diff --stat\`** — see what files changed
|
|
||||||
2. **\`cd ${cwd} && git diff\`** — read the actual diff
|
|
||||||
3. **\`cd ${cwd} && git status --short\`** — check for untracked files
|
|
||||||
|
|
||||||
## Checklist
|
|
||||||
|
|
||||||
### Reject (approved: false) — tell coder exactly what to fix
|
|
||||||
- **Garbage files**: build artifacts, lockfiles, IDE config that should not be committed
|
|
||||||
- **Secrets/credentials**: API keys, tokens, passwords hardcoded in the diff
|
|
||||||
- **Unrelated changes**: files modified outside the scope of the task
|
|
||||||
${
|
|
||||||
conventionsPath !== null
|
|
||||||
? `- **Convention violations**: patterns that contradict ${conventionsPath}\n`
|
|
||||||
: ""
|
|
||||||
}${extraBlock}
|
|
||||||
### Approve (approved: true) — no comment needed
|
|
||||||
- Diff is clean, focused, follows project standards
|
|
||||||
|
|
||||||
End with:
|
|
||||||
\`\`\`json
|
\`\`\`json
|
||||||
{ "approved": true }
|
{ "status": "approved" }
|
||||||
\`\`\`
|
\`\`\`
|
||||||
or
|
or
|
||||||
\`\`\`json
|
\`\`\`json
|
||||||
{ "approved": false }
|
{ "status": "rejected", "issues": ["issue 1", "issue 2"] }
|
||||||
\`\`\``;
|
\`\`\``;
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Code review role: agent inspects git diffs; structured extract yields `approved`.
|
* Code review role: agent inspects git diffs; structured extract yields approve/reject verdict.
|
||||||
*/
|
*/
|
||||||
export function createReviewerRole(
|
export function createReviewerRole(
|
||||||
adapter: AgentFn,
|
adapter: AgentFn,
|
||||||
|
|||||||
@@ -25,6 +25,7 @@ function makeCtx(
|
|||||||
steps: ThreadContext<SolveIssueMeta>["steps"],
|
steps: ThreadContext<SolveIssueMeta>["steps"],
|
||||||
): ThreadContext<SolveIssueMeta> {
|
): ThreadContext<SolveIssueMeta> {
|
||||||
return {
|
return {
|
||||||
|
threadId: "01TEST00000000000000000000",
|
||||||
start: makeStart(maxRounds),
|
start: makeStart(maxRounds),
|
||||||
steps,
|
steps,
|
||||||
};
|
};
|
||||||
@@ -52,7 +53,9 @@ function reviewerStep(approved: boolean): RoleStep<SolveIssueMeta> {
|
|||||||
return {
|
return {
|
||||||
role: "reviewer",
|
role: "reviewer",
|
||||||
content: "rev",
|
content: "rev",
|
||||||
meta: { approved },
|
meta: approved
|
||||||
|
? { status: "approved" as const }
|
||||||
|
: { status: "rejected" as const, issues: ["needs fix"] },
|
||||||
timestamp: 3,
|
timestamp: 3,
|
||||||
};
|
};
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -21,7 +21,7 @@ export const solveIssueModerator: Moderator<SolveIssueMeta> = (ctx) => {
|
|||||||
}
|
}
|
||||||
|
|
||||||
if (last.role === "reviewer") {
|
if (last.role === "reviewer") {
|
||||||
if (last.meta.approved === true) {
|
if (last.meta.status === "approved") {
|
||||||
return "committer";
|
return "committer";
|
||||||
}
|
}
|
||||||
if (ctx.steps.length < maxRounds - 1) {
|
if (ctx.steps.length < maxRounds - 1) {
|
||||||
|
|||||||
@@ -46,7 +46,7 @@ const CODER_DRY_RUN_META: CoderMeta = {
|
|||||||
};
|
};
|
||||||
|
|
||||||
const REVIEWER_DRY_RUN_META: ReviewerMeta = {
|
const REVIEWER_DRY_RUN_META: ReviewerMeta = {
|
||||||
approved: true,
|
status: "approved",
|
||||||
};
|
};
|
||||||
|
|
||||||
const COMMITTER_DRY_RUN_META: CommitterMeta = {
|
const COMMITTER_DRY_RUN_META: CommitterMeta = {
|
||||||
@@ -88,11 +88,8 @@ export type SolveIssueRoles = {
|
|||||||
|
|
||||||
export function createSolveIssueRoles(config: SolveIssueRolesConfig): SolveIssueRoles {
|
export function createSolveIssueRoles(config: SolveIssueRolesConfig): SolveIssueRoles {
|
||||||
const extract = resolveExtract(config);
|
const extract = resolveExtract(config);
|
||||||
const reviewerGit = {
|
const reviewerConfig = {
|
||||||
cwd: config.workdir,
|
cwd: config.workdir,
|
||||||
conventionsPath: null,
|
|
||||||
extraChecks: [],
|
|
||||||
threadId: null,
|
|
||||||
};
|
};
|
||||||
const committerGit = {
|
const committerGit = {
|
||||||
cwd: config.workdir,
|
cwd: config.workdir,
|
||||||
@@ -131,7 +128,7 @@ export function createSolveIssueRoles(config: SolveIssueRolesConfig): SolveIssue
|
|||||||
dryRun: extract.dryRun,
|
dryRun: extract.dryRun,
|
||||||
dryRunMeta: REVIEWER_DRY_RUN_META,
|
dryRunMeta: REVIEWER_DRY_RUN_META,
|
||||||
},
|
},
|
||||||
reviewerGit,
|
reviewerConfig,
|
||||||
);
|
);
|
||||||
|
|
||||||
const committer: Role<CommitterMeta> = createCommitterRole(
|
const committer: Role<CommitterMeta> = createCommitterRole(
|
||||||
|
|||||||
Reference in New Issue
Block a user