diff --git a/packages/workflow-role-reviewer/__tests__/reviewer.test.ts b/packages/workflow-role-reviewer/__tests__/reviewer.test.ts index 8711a70..5e79a0d 100644 --- a/packages/workflow-role-reviewer/__tests__/reviewer.test.ts +++ b/packages/workflow-role-reviewer/__tests__/reviewer.test.ts @@ -29,6 +29,7 @@ function toolCallResponse(argsJson: string): Response { function makeCtx(): ThreadContext { return { + threadId: "01TEST00000000000000000000", start: { role: START, content: "task", @@ -49,14 +50,14 @@ describe("createReviewerRole", () => { mock.restore(); }); - test("runs reviewer extract", async () => { + test("approved verdict", async () => { globalThis.fetch = (() => Promise.resolve( - toolCallResponse(JSON.stringify({ approved: true })), + toolCallResponse(JSON.stringify({ status: "approved" })), )) as unknown as typeof fetch; const agent: AgentFn = async (_ctx, prompt) => { - expect(prompt).toContain("git diff"); + expect(prompt).toContain("code reviewer"); expect(prompt).toContain(DEFAULT_REVIEWER_CONFIG.cwd); return "review done"; }; @@ -64,16 +65,33 @@ describe("createReviewerRole", () => { const role = createReviewerRole(agent, { provider, dryRun: null, - dryRunMeta: { approved: true }, + dryRunMeta: { status: "approved" }, }); 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 = (() => 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; let seen = ""; @@ -84,15 +102,10 @@ describe("createReviewerRole", () => { const role = createReviewerRole( agent, - { provider, dryRun: null, dryRunMeta: { approved: false } }, - { - cwd: "/proj", - conventionsPath: null, - extraChecks: [], - threadId: "01ABCDEF234567890ABCDEFGH", - }, + { provider, dryRun: null, dryRunMeta: { status: "approved" } }, + { cwd: "/proj" }, ); await role(makeCtx()); - expect(seen).toContain("uncaged-workflow thread 01ABCDEF234567890ABCDEFGH"); + expect(seen).toContain("uncaged-workflow thread 01TEST00000000000000000000"); }); }); diff --git a/packages/workflow-role-reviewer/src/reviewer.ts b/packages/workflow-role-reviewer/src/reviewer.ts index 15978a4..bc9979f 100644 --- a/packages/workflow-role-reviewer/src/reviewer.ts +++ b/packages/workflow-role-reviewer/src/reviewer.ts @@ -3,91 +3,52 @@ import { createRole } from "@uncaged/workflow-agent-llm"; import type { LlmProvider } from "@uncaged/workflow-util-role"; import * as z from "zod/v4"; -export const reviewerMetaSchema = z.object({ - approved: z.boolean().describe("true if the diff is clean and ready to merge"), -}); +export const reviewerMetaSchema = z.discriminatedUnion("status", [ + 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; export type ReviewerConfig = { cwd: string; - conventionsPath: string | null; - extraChecks: ReadonlyArray; - /** When non-null, prompts reference `uncaged-workflow thread `. */ - threadId: string | null; }; export const DEFAULT_REVIEWER_CONFIG: ReviewerConfig = { 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 { - const { cwd, conventionsPath, extraChecks, threadId } = config; + const { cwd } = config; - const conventionsBlock = - conventionsPath !== null ? `Read project conventions: \`cat ${cwd}/${conventionsPath}\`\n` : ""; + return `You are a code reviewer. The project is at \`${cwd}\`. - const threadBlock = - threadId !== null - ? `Read the workflow thread for context: \`uncaged-workflow thread ${threadId}\`\n` - : `## Thread context (no thread id)\n\n${summarizeThreadContext(ctx)}\n`; +## Context - const extraBlock = - extraChecks.length > 0 - ? `\n### Project-specific checks\n${extraChecks.map((c) => `- ${c}`).join("\n")}\n` - : ""; +Use \`uncaged-workflow thread ${ctx.threadId}\` to read the full workflow thread for context on what was done and why. - 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} -${conventionsBlock} -## Your job — static analysis of the git diff +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. -Run these commands and analyze the output: - -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: +End with your verdict: \`\`\`json -{ "approved": true } +{ "status": "approved" } \`\`\` or \`\`\`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( adapter: AgentFn, diff --git a/packages/workflow-template-solve-issue/__tests__/solve-issue-template.test.ts b/packages/workflow-template-solve-issue/__tests__/solve-issue-template.test.ts index 981d91d..c215032 100644 --- a/packages/workflow-template-solve-issue/__tests__/solve-issue-template.test.ts +++ b/packages/workflow-template-solve-issue/__tests__/solve-issue-template.test.ts @@ -25,6 +25,7 @@ function makeCtx( steps: ThreadContext["steps"], ): ThreadContext { return { + threadId: "01TEST00000000000000000000", start: makeStart(maxRounds), steps, }; @@ -52,7 +53,9 @@ function reviewerStep(approved: boolean): RoleStep { return { role: "reviewer", content: "rev", - meta: { approved }, + meta: approved + ? { status: "approved" as const } + : { status: "rejected" as const, issues: ["needs fix"] }, timestamp: 3, }; } diff --git a/packages/workflow-template-solve-issue/src/moderator.ts b/packages/workflow-template-solve-issue/src/moderator.ts index 10e3561..2ebdf4c 100644 --- a/packages/workflow-template-solve-issue/src/moderator.ts +++ b/packages/workflow-template-solve-issue/src/moderator.ts @@ -21,7 +21,7 @@ export const solveIssueModerator: Moderator = (ctx) => { } if (last.role === "reviewer") { - if (last.meta.approved === true) { + if (last.meta.status === "approved") { return "committer"; } if (ctx.steps.length < maxRounds - 1) { diff --git a/packages/workflow-template-solve-issue/src/roles.ts b/packages/workflow-template-solve-issue/src/roles.ts index 55dbed8..379561d 100644 --- a/packages/workflow-template-solve-issue/src/roles.ts +++ b/packages/workflow-template-solve-issue/src/roles.ts @@ -46,7 +46,7 @@ const CODER_DRY_RUN_META: CoderMeta = { }; const REVIEWER_DRY_RUN_META: ReviewerMeta = { - approved: true, + status: "approved", }; const COMMITTER_DRY_RUN_META: CommitterMeta = { @@ -88,11 +88,8 @@ export type SolveIssueRoles = { export function createSolveIssueRoles(config: SolveIssueRolesConfig): SolveIssueRoles { const extract = resolveExtract(config); - const reviewerGit = { + const reviewerConfig = { cwd: config.workdir, - conventionsPath: null, - extraChecks: [], - threadId: null, }; const committerGit = { cwd: config.workdir, @@ -131,7 +128,7 @@ export function createSolveIssueRoles(config: SolveIssueRolesConfig): SolveIssue dryRun: extract.dryRun, dryRunMeta: REVIEWER_DRY_RUN_META, }, - reviewerGit, + reviewerConfig, ); const committer: Role = createCommitterRole(