diff --git a/.changeset/FN-6235-reviewer-prompt-single-source.md b/.changeset/FN-6235-reviewer-prompt-single-source.md new file mode 100644 index 0000000000..638f16fade --- /dev/null +++ b/.changeset/FN-6235-reviewer-prompt-single-source.md @@ -0,0 +1,5 @@ +--- +"@runfusion/fusion": patch +--- + +Resolve the built-in reviewer base prompt from the workflow IR `review` node instead of an engine-local `REVIEWER_SYSTEM_PROMPT` duplicate. The canonical reviewer policy now lives in the `default-reviewer` agent prompt / built-in workflow seam, with reconciled superset content that preserves the FN-5928/FN-6229 surface-enumeration and symptom-verification gates, undersplit-task guidance, test-quality rules, worktree-boundary review, and the embedded port-4040 safety rule. diff --git a/packages/core/src/__tests__/agent-prompts.test.ts b/packages/core/src/__tests__/agent-prompts.test.ts index f926f0c415..002cae9f4f 100644 --- a/packages/core/src/__tests__/agent-prompts.test.ts +++ b/packages/core/src/__tests__/agent-prompts.test.ts @@ -10,7 +10,7 @@ import { } from "../agent-prompts.js"; import { BUILTIN_CODING_WORKFLOW_IR } from "../builtin-coding-workflow-ir.js"; import { renderTriagePolicyPlaceholders } from "../builtin-workflow-settings.js"; -import { resolvePlanningPromptFromIr } from "../workflow-ir-resolver.js"; +import { resolvePlanningPromptFromIr, resolveSeamPromptFromIr } from "../workflow-ir-resolver.js"; import type { AgentPromptsConfig, AgentPromptTemplate } from "../types.js"; import type { WorkflowIr } from "../workflow-ir-types.js"; @@ -286,13 +286,14 @@ describe("resolveAgentPrompt", () => { expect(renderedPrompt).not.toContain("{{"); }); - it("resolves custom planning prompts and ignores IRs without planning prompts", () => { + it("resolves custom seam prompts and ignores IRs without matching prompts", () => { const customIr: WorkflowIr = { version: "v1", name: "custom", nodes: [ { id: "start", kind: "start" }, { id: "planning", kind: "prompt", config: { seam: "planning", prompt: "custom planning prompt" } }, + { id: "review", kind: "prompt", config: { seam: "review", prompt: "custom review prompt" } }, ], edges: [], }; @@ -304,7 +305,10 @@ describe("resolveAgentPrompt", () => { }; expect(resolvePlanningPromptFromIr(customIr)).toBe("custom planning prompt"); + expect(resolveSeamPromptFromIr(customIr, "review")).toBe("custom review prompt"); + expect(resolveSeamPromptFromIr(BUILTIN_CODING_WORKFLOW_IR, "review")).toBe(resolveAgentPrompt("reviewer")); expect(resolvePlanningPromptFromIr(noPlanningIr)).toBeUndefined(); + expect(resolveSeamPromptFromIr(noPlanningIr, "review")).toBeUndefined(); }); it("built-in triage prompt requires surface enumeration for bug-fix specs", () => { diff --git a/packages/core/src/agent-prompts.ts b/packages/core/src/agent-prompts.ts index 8de01a32f5..fb9f6a125b 100644 --- a/packages/core/src/agent-prompts.ts +++ b/packages/core/src/agent-prompts.ts @@ -7,11 +7,10 @@ * - Additional role variants (senior-engineer, strict-reviewer, concise-triage) * - A resolver function that merges custom templates from project settings with built-ins * - * NOTE: The built-in prompt texts are derived from the engine's hardcoded prompts - * (EXECUTOR_SYSTEM_PROMPT, TRIAGE_SYSTEM_PROMPT, REVIEWER_SYSTEM_PROMPT, and the - * merger prompt). They should be kept in sync when the engine prompts change. - * Since @fusion/core cannot import @fusion/engine (circular dependency), these - * are maintained as inline strings. + * NOTE: Built-in prompt texts that feed workflow seams live here as the canonical + * source for @fusion/core and @fusion/engine. Engine code should resolve triage + * and reviewer built-ins through workflow IR seam prompts instead of carrying + * duplicate policy constants. * * @module agent-prompts */ @@ -19,7 +18,7 @@ import type { AgentCapability, AgentPromptTemplate, AgentPromptsConfig } from "./types.js"; // --------------------------------------------------------------------------- -// Built-in prompt text (derived from engine constants — keep in sync) +// Built-in prompt text (canonical source for workflow seam prompts) // --------------------------------------------------------------------------- const EXECUTOR_PROMPT_TEXT = `You are a task execution agent for "fn", an AI-orchestrated task board. @@ -538,11 +537,26 @@ Use this exact checklist (keep it verbatim — do not expand or reorder): Only inject this section when the task genuinely touches frontend UI. Omit it for backend-only, config-only, or documentation-only tasks.`;; +// FN-6235: single source for the built-in reviewer policy; the engine REVIEWER_SYSTEM_PROMPT duplicate was removed. const REVIEWER_PROMPT_TEXT = `You are an independent code and plan reviewer. +## Your Role +You are an objective quality gate for plans, code, and specs. +You are neither the implementor's advocate nor adversary: your job is evidence-based assessment that protects delivery quality. + You provide quality assessment for task implementations. You have full read access to the codebase and can run commands to inspect code. +## What to Look For +- Correctness against stated requirements +- Edge-case handling and failure-path behavior +- Test adequacy (behavior-focused coverage, meaningful assertions) +- Consistency with existing project patterns and conventions +- Security, data-safety, and permission boundary concerns +- Performance implications where changes affect hot paths or heavy operations + +Review efficiently: prioritize high-impact correctness/risk issues first. Do not spend blocking attention on style nits when substantive defects exist. + ## Verdict Criteria - **APPROVE** — Step will achieve its stated outcomes. Minor suggestions go in @@ -556,6 +570,11 @@ access to the codebase and can run commands to inspect code. ### APPROVE vs REVISE +Concrete examples: +- APPROVE: implementation satisfies outcomes; only optional cleanup or minor wording suggestions remain. +- REVISE: a required behavior is missing, tests are insufficient for changed behavior, or a likely regression exists. +- RETHINK: the approach conflicts with architecture/task goals such that incremental edits are unlikely to rescue it. + **APPROVE** when: - The approach will work, but you see a cleaner alternative - Documentation style could improve @@ -568,6 +587,7 @@ access to the codebase and can run commands to inspect code. - Backward compatibility is broken without migration - Code outside the task's File Scope is deleted, removed, or gutted (out-of-scope removal) - Existing functionality is removed without a corresponding changeset explaining the removal +- Code changes were made outside the assigned task worktree, unless the path is an expected exception such as project memory or task attachments ### Do NOT issue REVISE for - STATUS/formatting preferences @@ -610,7 +630,7 @@ access to the codebase and can run commands to inspect code. ### Test Gaps - [Missing test scenarios] -- [For bug fixes, call out any repro-only regression test that does not assert the invariant across the enumerated surfaces. Issue REVISE when coverage stops at the single reported case instead of spanning the \`## Surface Enumeration\` checklist (FN-5893; see FN-5787/FN-5789/FN-5803, FN-5797/FN-5875/FN-5919, and FN-5751).] +- [For bug fixes and UI-affordance add/remove changes, call out any single-surface-only test that doesn't verify the invariant across the spec's enumerated surfaces. For UI-affordance removals, also flag tests that don't verify the removed affordance's container/wrapper is fully cleaned up on both desktop and mobile breakpoints. Issue REVISE when coverage stops at the single reported surface (FN-6134; see FN-6115→FN-6118→FN-6123 for the motivating multi-task incident). Keep enforcing FN-5893 for bug fixes; see FN-5787/FN-5789/FN-5803, FN-5797/FN-5875/FN-5919, and FN-5751.] ### Suggestions - [Optional improvements, not blocking] @@ -635,18 +655,85 @@ access to the codebase and can run commands to inspect code. - **File scope accuracy:** [All affected files listed? No extras?] - **Dependency correctness:** [Dependencies exist and are appropriate?] - **Testing requirements:** [Real automated tests required, not just typechecks?] -- **Surface enumeration:** [For bug-fix specs, is \`## Surface Enumeration\` present and does it enumerate the relevant providers/bridges/execution paths, desktop + mobile breakpoints/platforms, empty/undefined/duplicate/populated states, and shared hooks/components/modules/helpers? Missing or incomplete coverage is a blocking REVISE.] +- **Surface enumeration:** [For bug-fix specs and UI-affordance add/remove specs, is \`## Surface Enumeration\` present and does it enumerate the relevant providers/bridges/execution paths, desktop + mobile breakpoints/platforms, empty/undefined/duplicate/populated states, and shared hooks/components/modules/helpers? For UI-affordance add/remove tasks, also verify: (a) the spec searches for ALL components rendering the affordance, not just the one the user pointed at; (b) the spec explicitly addresses leftover shells after removal across desktop and mobile breakpoints. Missing or incomplete coverage is a blocking REVISE.] +- **Symptom verification:** [For bug-class/bug-fix specs only, is \`## Symptom Verification\` present and complete with **Original symptom**, **Exact reproduction**, and **Assertion it is gone**? A bug-class spec whose final verification only checks green build/tests without reproducing the original failure and asserting it no longer occurs is a blocking REVISE under FN-5893. Missing, empty, or incomplete \`## Symptom Verification\` is a blocking REVISE for bug-class specs; feature/docs/non-bug specs are not required to carry it.] - **Documentation completeness:** [Must Update / Check If Affected sections present?] +- **Dangling task-document references:** [No \`.fusion/tasks//\` path is cited in Context, Steps, or File Scope unless the file exists or is explicitly created as a \`(new)\` artifact in this spec. References to nonexistent task-local artifacts are a blocking REVISE.] - **Sizing & review level:** [Size and review level appropriate for the work?] -- **Subtask breakdown:** [Were complex tasks appropriately split into 2-5 child tasks? A task with 8+ implementation steps, affecting 3+ packages, should have been divided] +- **Subtask breakdown:** [Only flag genuinely oversized specs (12+ implementation steps, OR 5+ truly independent deliverables that could ship separately). Do NOT flag a coherent vertical change just because it touches multiple packages. When borderline, prefer leaving the task whole.] - **User comment coverage:** [Were all user comments addressed? Every user comment must be reflected in the spec — missing coverage is a blocking REVISE] ### Suggestions - [Optional improvements, not blocking] \`\`\` -## Safety Rules -- **NEVER kill processes on port 4040.** Port 4040 is the production dashboard. If you need to test server endpoints, start a server on a different port (\`--port 0\` for random). If port 4040 is occupied, use a different port — do NOT kill the occupant. Issue REVISE if the executor kills or attempts to kill processes on port 4040.`; +## Spec Review — Undersplit Task Detection + +When reviewing specs, assess whether the task should have been broken into subtasks. The bar for splitting is high — most tasks should remain whole. Coordination overhead (worktrees, dependency wiring, merge sequencing) is real, so splitting must clearly pay for itself. + +**Default position:** do NOT flag undersplit. Reach for it only when the spec is genuinely oversized. + +**Flag as REVISE only when ALL of the following are true:** +- The spec has 12+ implementation steps, OR contains 5+ clearly independent deliverables that could be shipped separately by different people +- The deliverables are NOT a coherent vertical change (a single feature touching core + dashboard + tests is coherent — do not split it) +- Splitting would produce children that each have ≥4 steps and a clearly distinct scope + +If the spec is borderline (under those thresholds, or arguable), put your splitting suggestion in the **Suggestions** section instead of REVISE — the planner can take it or leave it. + +**How to flag an undersplit task (only when the criteria above are met):** +Say explicitly: "This task should be broken into subtasks because [specific reason]." +Recommend the number of child tasks (2-5) and what each should cover. +Instruct the planner to: +1. Use the \`fn_task_create\` tool to create 2–5 child tasks from the oversized spec +2. Do NOT write a parent PROMPT.md — the parent will be closed automatically after children are created + (Not write a parent PROMPT.md is also unacceptable.) +3. Make each child cover one coherent deliverable with clear scope boundaries + +Example REVISE feedback for a genuinely oversized task: +"This task has 14 steps and contains 4 independent deliverables (engine integration, dashboard UI, CLI command, migration tooling) that could ship separately. Use fn_task_create to split into: (1) engine logic, (2) dashboard UI, (3) CLI integration, (4) migration tooling. Do not write a parent PROMPT." + +**Do NOT flag if ANY of these apply:** +- The spec has 11 or fewer implementation steps +- Steps are sequential and tightly coupled (e.g., a pipeline where each step depends on the previous) +- The task is a vertical change touching multiple packages for one coherent feature (typical in this monorepo) +- The task is a bug fix, regardless of how many files it touches +- Splitting would create coordination overhead that exceeds the benefit + +## Plan Granularity + +When reviewing plans, assess whether the approach achieves the step's OUTCOMES — +not whether every function and parameter is listed. + +Good plan: identifies key behavioral changes, calls out risks, has a testing strategy. +Do NOT demand function-level implementation checklists. + +## Test Quality Review + +When reviewing tests, check that they verify observable behavior and regression risk (not only implementation trivia). +Flag REVISE when key edge cases or failure modes for changed behavior are untested. +For bug fixes, apply FN-5893 strictly: if the regression test only reproduces the reported case instead of asserting the invariant across the spec's \`## Surface Enumeration\` surfaces, issue REVISE. Treat that as a repro-only regression test; issue REVISE when coverage stops at the single reported case instead of spanning the \`## Surface Enumeration\` checklist. Use the motivating recurrences (FN-5787/FN-5789/FN-5803, FN-5797/FN-5875/FN-5919, and FN-5751) as concrete examples of why repro-only coverage is insufficient. +For bug-class/bug-fix specs, also enforce symptom-based acceptance: if the spec is missing \`## Symptom Verification\`, leaves it empty/incomplete, lacks **Original symptom**, **Exact reproduction**, or **Assertion it is gone**, or its final verification only checks green build/tests without reproducing the original failure condition and asserting it no longer occurs, issue REVISE. Do not require \`## Symptom Verification\` for feature/docs/non-bug specs. +For UI-affordance add/remove changes, apply the same surface-enumeration strictness: if the test only checks the single surface the user reported instead of all enumerated surfaces, issue REVISE. For UI-affordance removals, require coverage/evidence that empty button shells, orphaned click targets, now-unused wrappers, and dangling aria-labels are cleaned up across desktop and mobile breakpoints; FN-6115/FN-6118/FN-6123 is the motivating recurrence. + +## Worktree Boundary Review + +For code reviews, verify that implementation changes are in the assigned task +worktree. The review request includes the current worktree path. Inspect git +state and recent commits from that worktree, and treat changes outside it as a +blocking REVISE unless they are expected project-root state such as +\`.fusion/memory/\` files, task attachments, or other explicitly documented +Fusion metadata. If you see edits or commits in the primary project checkout +instead of the task worktree, call that out directly and ask the worker to move +the changes into the assigned worktree. + +## Rules + +- Be specific — reference actual files and line numbers +- Be constructive — suggest fixes, not just problems +- Be proportional — don't block on style nits +- Output your review as plain text (not to a file) +- **NEVER kill processes on port 4040.** Port 4040 is the production dashboard. If you need to test server endpoints, start a server on a different port (\`--port 0\` for random). If port 4040 is occupied, use a different port — do NOT kill the occupant. Issue REVISE if the executor kills or attempts to kill processes on port 4040. +`; /** * Base merger prompt text (without commit format instructions, which are diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index fc4911f7f3..dd7bf475dd 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -314,7 +314,9 @@ export { export { resolveWorkflowIrForTask, resolveWorkflowIrById, + resolveSeamPromptFromIr, resolvePlanningPromptFromIr, + resolveTaskSeamPrompt, resolveTaskPlanningPrompt, type WorkflowIrResolverStore, } from "./workflow-ir-resolver.js"; diff --git a/packages/core/src/workflow-ir-resolver.ts b/packages/core/src/workflow-ir-resolver.ts index c3649314d7..5f25a733a2 100644 --- a/packages/core/src/workflow-ir-resolver.ts +++ b/packages/core/src/workflow-ir-resolver.ts @@ -26,31 +26,50 @@ export interface WorkflowIrResolverStore { } /** - * Extract the planning seam prompt from a resolved workflow IR. + * Extract a prompt seam's prompt text from a resolved workflow IR. * - * Planning seam nodes are prompt nodes with `config.seam === "planning"`; + * Seam prompt nodes are prompt nodes with `config.seam === seam`; * `config.prompt` carries the text installed by builtinPromptConfig or a custom * workflow author. Empty/missing prompts return undefined so callers can apply * their own fail-soft fallback. */ -export function resolvePlanningPromptFromIr(ir: WorkflowIr): string | undefined { +export function resolveSeamPromptFromIr(ir: WorkflowIr, seam: string): string | undefined { for (const node of ir.nodes) { if (node.kind !== "prompt") continue; - if (node.config?.seam !== "planning") continue; + if (node.config?.seam !== seam) continue; const prompt = node.config.prompt; if (typeof prompt === "string" && prompt.trim().length > 0) return prompt; } return undefined; } +/** Extract the planning seam prompt from a resolved workflow IR. */ +export function resolvePlanningPromptFromIr(ir: WorkflowIr): string | undefined { + return resolveSeamPromptFromIr(ir, "planning"); +} + +/** Resolve a task's seam prompt via its selected workflow IR. */ +export async function resolveTaskSeamPrompt( + store: WorkflowIrResolverStore, + taskId: string, + seam: string, + irCache?: Map, +): Promise { + try { + const ir = await resolveWorkflowIrForTask(store, taskId, irCache); + return resolveSeamPromptFromIr(ir, seam); + } catch { + return undefined; + } +} + /** Resolve a task's planning seam prompt via its selected workflow IR. */ export async function resolveTaskPlanningPrompt( store: WorkflowIrResolverStore, taskId: string, irCache?: Map, ): Promise { - const ir = await resolveWorkflowIrForTask(store, taskId, irCache); - return resolvePlanningPromptFromIr(ir); + return resolveTaskSeamPrompt(store, taskId, "planning", irCache); } /** diff --git a/packages/engine/src/__tests__/prompt-cache-integration.test.ts b/packages/engine/src/__tests__/prompt-cache-integration.test.ts index 9e2b1e7194..df33df7c93 100644 --- a/packages/engine/src/__tests__/prompt-cache-integration.test.ts +++ b/packages/engine/src/__tests__/prompt-cache-integration.test.ts @@ -1,13 +1,15 @@ import { describe, it, expect } from "vitest"; +import { resolveAgentPrompt } from "@fusion/core"; import { buildPromptLayers, collapsePromptLayers, type SystemPromptLayers } from "../prompt-layers.js"; -import { REVIEWER_SYSTEM_PROMPT } from "../reviewer.js"; + +const DEFAULT_REVIEWER_PROMPT = resolveAgentPrompt("reviewer"); describe("cross-session prompt cache integration", () => { const MEMORY_INSTRUCTIONS = "\n## Memory\n\nUse fn_memory_search to look up relevant context."; function simulateReviewerSession(sessionIndex: number): SystemPromptLayers { return buildPromptLayers({ - basePrompt: REVIEWER_SYSTEM_PROMPT, + basePrompt: DEFAULT_REVIEWER_PROMPT, agentInstructions: `Session ${sessionIndex}: custom instructions that vary per agent.`, memorySection: MEMORY_INSTRUCTIONS, pluginContributions: sessionIndex % 2 === 0 @@ -42,8 +44,8 @@ describe("cross-session prompt cache integration", () => { } }); - it("stable prefix starts with REVIEWER_SYSTEM_PROMPT", () => { + it("stable prefix starts with the canonical default reviewer prompt", () => { const layers = simulateReviewerSession(0); - expect(layers.stable.startsWith(REVIEWER_SYSTEM_PROMPT)).toBe(true); + expect(layers.stable.startsWith(DEFAULT_REVIEWER_PROMPT)).toBe(true); }); }); diff --git a/packages/engine/src/__tests__/reviewer-prompt-single-source.test.ts b/packages/engine/src/__tests__/reviewer-prompt-single-source.test.ts new file mode 100644 index 0000000000..64a448cc1b --- /dev/null +++ b/packages/engine/src/__tests__/reviewer-prompt-single-source.test.ts @@ -0,0 +1,160 @@ +import { readFileSync } from "node:fs"; +import { resolve } from "node:path"; +import { fileURLToPath } from "node:url"; +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { + BUILTIN_CODING_WORKFLOW_IR, + resolveAgentPrompt, + resolveSeamPromptFromIr, + type WorkflowIr, +} from "@fusion/core"; + +vi.mock("../pi.js", () => ({ + createFnAgent: vi.fn(), + describeModel: vi.fn().mockReturnValue("mock-provider/mock-model"), + promptWithFallback: vi.fn(async (session, prompt, options) => { + if (options === undefined) { + await session.prompt(prompt); + } else { + await session.prompt(prompt, options); + } + }), +})); + +import { reviewStep } from "../reviewer.js"; +import { createFnAgent } from "../pi.js"; + +const mockedCreateFnAgent = vi.mocked(createFnAgent); + +function createMockSession(reviewText = "### Verdict: APPROVE\n### Summary\nLooks good.") { + return { + session: { + prompt: vi.fn().mockResolvedValue(undefined), + subscribe: vi.fn().mockImplementation((cb: any) => { + cb({ + type: "message_update", + assistantMessageEvent: { type: "text_delta", delta: reviewText }, + }); + }), + dispose: vi.fn(), + }, + } as any; +} + +function createStore(workflowId = "builtin:coding", customIr?: WorkflowIr) { + return { + getSettings: vi.fn().mockResolvedValue({}), + getTaskWorkflowSelection: vi.fn().mockReturnValue({ workflowId, stepIds: [] }), + getWorkflowDefinition: vi.fn().mockImplementation(async (id: string) => { + if (customIr && id === workflowId) return { ir: customIr }; + return undefined; + }), + } as any; +} + +async function captureReviewerSystemPrompt(options: Parameters[7] = {}) { + mockedCreateFnAgent.mockResolvedValue(createMockSession()); + await reviewStep( + "/tmp/worktree", + "FN-6235", + 1, + "Review prompt source", + "plan", + "# Plan", + undefined, + options, + ); + return mockedCreateFnAgent.mock.calls[0][0].systemPrompt as string; +} + +beforeEach(() => { + vi.clearAllMocks(); +}); + +describe("reviewer prompt single source", () => { + it("does not reintroduce an engine reviewer policy constant", () => { + const reviewerSource = readFileSync( + resolve(fileURLToPath(new URL("..", import.meta.url)), "reviewer.ts"), + "utf8", + ); + + expect(reviewerSource).not.toMatch(/export const REVIEWER_SYSTEM_PROMPT\s*=/); + expect(reviewerSource).not.toMatch(/export const [A-Z_]*REVIEWER[A-Z_]*SYSTEM_PROMPT\s*=/); + }); + + it("keeps builtin coding review seam byte-identical to the default reviewer prompt", () => { + expect(resolveSeamPromptFromIr(BUILTIN_CODING_WORKFLOW_IR, "review")).toBe(resolveAgentPrompt("reviewer")); + }); + + it("uses the builtin coding IR review-node prompt when no user override is set", async () => { + const systemPrompt = await captureReviewerSystemPrompt({ store: createStore() }); + + expect(systemPrompt).toBe(resolveSeamPromptFromIr(BUILTIN_CODING_WORKFLOW_IR, "review")); + }); + + it("uses a selected custom workflow review-node prompt", async () => { + const customIr: WorkflowIr = { + version: "v1", + name: "custom-reviewer", + nodes: [ + { id: "start", kind: "start" }, + { id: "review", kind: "prompt", config: { seam: "review", prompt: "custom workflow reviewer prompt" } }, + ], + edges: [], + }; + + const systemPrompt = await captureReviewerSystemPrompt({ store: createStore("WF-review", customIr) }); + + expect(systemPrompt).toBe("custom workflow reviewer prompt"); + }); + + it("preserves reviewer user-override precedence over workflow IR prompts", async () => { + const customIr: WorkflowIr = { + version: "v1", + name: "custom-reviewer", + nodes: [ + { id: "review", kind: "prompt", config: { seam: "review", prompt: "workflow prompt should not win" } }, + ], + edges: [], + }; + + const systemPrompt = await captureReviewerSystemPrompt({ + store: createStore("WF-review", customIr), + agentPrompts: { + templates: [{ + id: "custom-reviewer", + name: "Custom Reviewer", + description: "Project reviewer override", + role: "reviewer", + prompt: "user override reviewer prompt", + }], + roleAssignments: { reviewer: "custom-reviewer" }, + }, + }); + + expect(systemPrompt).toBe("user override reviewer prompt"); + }); + + it("falls back to a non-empty default reviewer prompt when no store is provided", async () => { + const systemPrompt = await captureReviewerSystemPrompt(); + + expect(systemPrompt).toBe(resolveAgentPrompt("reviewer")); + expect(systemPrompt.trim().length).toBeGreaterThan(0); + }); + + it.each(["plan", "code", "spec"] as const)("uses the same resolved base prompt for %s reviews", async (reviewType) => { + mockedCreateFnAgent.mockResolvedValue(createMockSession()); + await reviewStep( + "/tmp/worktree", + "FN-6235", + 1, + "Review prompt source", + reviewType, + "# Prompt", + undefined, + { store: createStore() }, + ); + + expect(mockedCreateFnAgent.mock.calls[0][0].systemPrompt).toBe(resolveAgentPrompt("reviewer")); + }); +}); diff --git a/packages/engine/src/__tests__/reviewer.test.ts b/packages/engine/src/__tests__/reviewer.test.ts index bd73680e2c..f1c9d09124 100644 --- a/packages/engine/src/__tests__/reviewer.test.ts +++ b/packages/engine/src/__tests__/reviewer.test.ts @@ -12,9 +12,12 @@ vi.mock("../pi.js", () => ({ }), })); -import { reviewStep, REVIEWER_SYSTEM_PROMPT } from "../reviewer.js"; +import { resolveAgentPrompt } from "@fusion/core"; +import { reviewStep } from "../reviewer.js"; import { createFnAgent, promptWithFallback } from "../pi.js"; +const DEFAULT_REVIEWER_PROMPT = resolveAgentPrompt("reviewer"); + const mockedCreateFnAgent = vi.mocked(createFnAgent); const mockedPromptWithFallback = vi.mocked(promptWithFallback); const CONTEXT_LIMIT_ERROR = "exceeded model token limit: 262144 (requested: 262879)"; @@ -293,55 +296,55 @@ describe("reviewStep — spec review type", () => { describe("FN-5928 surface-enumeration review-gate wording", () => { it("requires spec reviews to block missing or incomplete surface enumeration for bug-fix specs", () => { - expect(REVIEWER_SYSTEM_PROMPT).toContain("**Surface enumeration:**"); - expect(REVIEWER_SYSTEM_PROMPT).toMatch( + expect(DEFAULT_REVIEWER_PROMPT).toContain("**Surface enumeration:**"); + expect(DEFAULT_REVIEWER_PROMPT).toMatch( /For bug-fix specs and UI-affordance add\/remove specs, is `## Surface Enumeration` present[\s\S]*Missing or incomplete coverage is a blocking REVISE\./, ); - expect(REVIEWER_SYSTEM_PROMPT).toContain("desktop + mobile breakpoints/platforms"); - expect(REVIEWER_SYSTEM_PROMPT).toContain("shared hooks/components/modules/helpers"); - expect(REVIEWER_SYSTEM_PROMPT).toContain("bug-fix specs and UI-affordance add/remove specs"); + expect(DEFAULT_REVIEWER_PROMPT).toContain("desktop + mobile breakpoints/platforms"); + expect(DEFAULT_REVIEWER_PROMPT).toContain("shared hooks/components/modules/helpers"); + expect(DEFAULT_REVIEWER_PROMPT).toContain("bug-fix specs and UI-affordance add/remove specs"); }); it("requires code reviews to reject repro-only regression tests for bug fixes", () => { - expect(REVIEWER_SYSTEM_PROMPT).toMatch( + expect(DEFAULT_REVIEWER_PROMPT).toMatch( /For bug fixes, apply FN-5893 strictly: if the regression test only reproduces the reported case instead of asserting the invariant across the spec's `## Surface Enumeration` surfaces, issue REVISE\./, ); - expect(REVIEWER_SYSTEM_PROMPT).toContain("single-surface-only test"); - expect(REVIEWER_SYSTEM_PROMPT).toContain("doesn't verify the invariant across the spec's enumerated surfaces"); - expect(REVIEWER_SYSTEM_PROMPT).toContain("Keep enforcing FN-5893 for bug fixes"); - expect(REVIEWER_SYSTEM_PROMPT).toContain("FN-5787/FN-5789/FN-5803"); - expect(REVIEWER_SYSTEM_PROMPT).toContain("FN-5797/FN-5875/FN-5919"); - expect(REVIEWER_SYSTEM_PROMPT).toContain("FN-5751"); + expect(DEFAULT_REVIEWER_PROMPT).toContain("single-surface-only test"); + expect(DEFAULT_REVIEWER_PROMPT).toContain("doesn't verify the invariant across the spec's enumerated surfaces"); + expect(DEFAULT_REVIEWER_PROMPT).toContain("Keep enforcing FN-5893 for bug fixes"); + expect(DEFAULT_REVIEWER_PROMPT).toContain("FN-5787/FN-5789/FN-5803"); + expect(DEFAULT_REVIEWER_PROMPT).toContain("FN-5797/FN-5875/FN-5919"); + expect(DEFAULT_REVIEWER_PROMPT).toContain("FN-5751"); }); it("requires spec reviews to block bug-class specs missing symptom verification", () => { - expect(REVIEWER_SYSTEM_PROMPT).toContain("**Symptom verification:**"); - expect(REVIEWER_SYSTEM_PROMPT).toMatch( + expect(DEFAULT_REVIEWER_PROMPT).toContain("**Symptom verification:**"); + expect(DEFAULT_REVIEWER_PROMPT).toMatch( /For bug-class\/bug-fix specs only, is `## Symptom Verification` present and complete with \*\*Original symptom\*\*, \*\*Exact reproduction\*\*, and \*\*Assertion it is gone\*\*\?/, ); - expect(REVIEWER_SYSTEM_PROMPT).toContain( + expect(DEFAULT_REVIEWER_PROMPT).toContain( "A bug-class spec whose final verification only checks green build/tests without reproducing the original failure and asserting it no longer occurs is a blocking REVISE under FN-5893", ); - expect(REVIEWER_SYSTEM_PROMPT).toContain( + expect(DEFAULT_REVIEWER_PROMPT).toContain( "Missing, empty, or incomplete `## Symptom Verification` is a blocking REVISE for bug-class specs", ); - expect(REVIEWER_SYSTEM_PROMPT).toContain("feature/docs/non-bug specs are not required to carry it"); + expect(DEFAULT_REVIEWER_PROMPT).toContain("feature/docs/non-bug specs are not required to carry it"); }); it("requires code reviews to reject green-build-only symptom acceptance for bug fixes", () => { - expect(REVIEWER_SYSTEM_PROMPT).toMatch( + expect(DEFAULT_REVIEWER_PROMPT).toMatch( /For bug-class\/bug-fix specs, also enforce symptom-based acceptance:[\s\S]*final verification only checks green build\/tests without reproducing the original failure condition and asserting it no longer occurs, issue REVISE\./, ); - expect(REVIEWER_SYSTEM_PROMPT).toContain("lacks **Original symptom**, **Exact reproduction**, or **Assertion it is gone**"); - expect(REVIEWER_SYSTEM_PROMPT).toContain("Do not require `## Symptom Verification` for feature/docs/non-bug specs"); + expect(DEFAULT_REVIEWER_PROMPT).toContain("lacks **Original symptom**, **Exact reproduction**, or **Assertion it is gone**"); + expect(DEFAULT_REVIEWER_PROMPT).toContain("Do not require `## Symptom Verification` for feature/docs/non-bug specs"); }); it("requires spec/code reviews to enforce surface enumeration for UI-affordance add/remove tasks", () => { - expect(REVIEWER_SYSTEM_PROMPT).toContain("leftover shells after removal"); - expect(REVIEWER_SYSTEM_PROMPT).toContain("For bug fixes and UI-affordance add/remove changes"); - expect(REVIEWER_SYSTEM_PROMPT).toContain("UI-affordance removals"); - expect(REVIEWER_SYSTEM_PROMPT).toContain("For UI-affordance add/remove changes, apply the same surface-enumeration strictness"); - expect(REVIEWER_SYSTEM_PROMPT).toContain("FN-6115/FN-6118/FN-6123"); + expect(DEFAULT_REVIEWER_PROMPT).toContain("leftover shells after removal"); + expect(DEFAULT_REVIEWER_PROMPT).toContain("For bug fixes and UI-affordance add/remove changes"); + expect(DEFAULT_REVIEWER_PROMPT).toContain("UI-affordance removals"); + expect(DEFAULT_REVIEWER_PROMPT).toContain("For UI-affordance add/remove changes, apply the same surface-enumeration strictness"); + expect(DEFAULT_REVIEWER_PROMPT).toContain("FN-6115/FN-6118/FN-6123"); }); it("demonstrates the gate firing on a single-component UI-removal spec", () => { @@ -349,11 +352,11 @@ describe("FN-5928 surface-enumeration review-gate wording", () => { "## Mission\nRemove the workflow-row chevron from WorkflowRow.tsx only."; expect(singleComponentRemovalSpec).toContain("WorkflowRow.tsx only"); - expect(REVIEWER_SYSTEM_PROMPT).toContain("searches for ALL components rendering the affordance"); - expect(REVIEWER_SYSTEM_PROMPT).toContain("not just the one the user pointed at"); - expect(REVIEWER_SYSTEM_PROMPT).toContain("leftover shells after removal"); - expect(REVIEWER_SYSTEM_PROMPT).toContain("empty button shells"); - expect(REVIEWER_SYSTEM_PROMPT).toContain("Issue REVISE when coverage stops at the single reported surface"); + expect(DEFAULT_REVIEWER_PROMPT).toContain("searches for ALL components rendering the affordance"); + expect(DEFAULT_REVIEWER_PROMPT).toContain("not just the one the user pointed at"); + expect(DEFAULT_REVIEWER_PROMPT).toContain("leftover shells after removal"); + expect(DEFAULT_REVIEWER_PROMPT).toContain("empty button shells"); + expect(DEFAULT_REVIEWER_PROMPT).toContain("Issue REVISE when coverage stops at the single reported surface"); }); }); @@ -876,24 +879,24 @@ describe("reviewStep — validator model overrides", () => { }); }); -describe("REVIEWER_SYSTEM_PROMPT", () => { +describe("default reviewer prompt", () => { it("includes subtask breakdown criterion in spec review", () => { - expect(REVIEWER_SYSTEM_PROMPT).toContain("Subtask breakdown"); - expect(REVIEWER_SYSTEM_PROMPT).toContain( + expect(DEFAULT_REVIEWER_PROMPT).toContain("Subtask breakdown"); + expect(DEFAULT_REVIEWER_PROMPT).toContain( "12+ implementation steps", ); }); it("biases the reviewer toward keeping tasks whole", () => { - expect(REVIEWER_SYSTEM_PROMPT).toContain("The bar for splitting is high"); - expect(REVIEWER_SYSTEM_PROMPT).toContain( + expect(DEFAULT_REVIEWER_PROMPT).toContain("The bar for splitting is high"); + expect(DEFAULT_REVIEWER_PROMPT).toContain( "Default position:** do NOT flag undersplit", ); - expect(REVIEWER_SYSTEM_PROMPT).toContain("12+ implementation steps"); + expect(DEFAULT_REVIEWER_PROMPT).toContain("12+ implementation steps"); }); it("downgrades borderline undersplit findings to non-blocking suggestions", () => { - expect(REVIEWER_SYSTEM_PROMPT).toContain( + expect(DEFAULT_REVIEWER_PROMPT).toContain( "Suggestions** section instead of REVISE", ); }); @@ -901,25 +904,25 @@ describe("REVIEWER_SYSTEM_PROMPT", () => { it("instructs planner to use fn_task_create for genuinely oversized tasks", () => { // The reviewer's REVISE feedback must explicitly direct the planner to // create child tasks via fn_task_create rather than just flagging the issue. - expect(REVIEWER_SYSTEM_PROMPT).toContain("fn_task_create"); - expect(REVIEWER_SYSTEM_PROMPT).toContain( + expect(DEFAULT_REVIEWER_PROMPT).toContain("fn_task_create"); + expect(DEFAULT_REVIEWER_PROMPT).toContain( "create 2–5 child tasks", ); - expect(REVIEWER_SYSTEM_PROMPT).toContain( + expect(DEFAULT_REVIEWER_PROMPT).toContain( "Not write a parent PROMPT.md", ); }); it("includes user comment coverage criterion in spec review format", () => { - expect(REVIEWER_SYSTEM_PROMPT).toContain("User comment coverage"); - expect(REVIEWER_SYSTEM_PROMPT).toContain("missing coverage is a blocking REVISE"); + expect(DEFAULT_REVIEWER_PROMPT).toContain("User comment coverage"); + expect(DEFAULT_REVIEWER_PROMPT).toContain("missing coverage is a blocking REVISE"); }); it("includes worktree boundary guidance for code reviews", () => { - expect(REVIEWER_SYSTEM_PROMPT).toContain("Worktree Boundary Review"); - expect(REVIEWER_SYSTEM_PROMPT).toContain("assigned task worktree"); - expect(REVIEWER_SYSTEM_PROMPT).toContain("blocking REVISE"); - expect(REVIEWER_SYSTEM_PROMPT).toContain(".fusion/memory/"); + expect(DEFAULT_REVIEWER_PROMPT).toContain("Worktree Boundary Review"); + expect(DEFAULT_REVIEWER_PROMPT).toContain("assigned task worktree"); + expect(DEFAULT_REVIEWER_PROMPT).toContain("blocking REVISE"); + expect(DEFAULT_REVIEWER_PROMPT).toContain(".fusion/memory/"); }); }); diff --git a/packages/engine/src/prompt-layers.ts b/packages/engine/src/prompt-layers.ts index d307afd735..569627e5ef 100644 --- a/packages/engine/src/prompt-layers.ts +++ b/packages/engine/src/prompt-layers.ts @@ -17,7 +17,7 @@ export interface SystemPromptLayers { } export interface PromptLayerInput { - /** The base role system prompt (e.g. REVIEWER_SYSTEM_PROMPT). */ + /** The base role system prompt (for reviewer, the workflow IR review seam prompt). */ basePrompt: string; /** Resolved agent instructions (instructionsText + instructionsPath + soul). */ agentInstructions?: string; diff --git a/packages/engine/src/reviewer.ts b/packages/engine/src/reviewer.ts index 240899e595..1730c5d1d3 100644 --- a/packages/engine/src/reviewer.ts +++ b/packages/engine/src/reviewer.ts @@ -1,4 +1,4 @@ -// port-4040-allowlist: this file embeds the "never kill port 4040" rule in the reviewer prompt. +// port-4040-allowlist: reviewer prompts resolve from @fusion/core agent-prompts, which embeds the "never kill port 4040" rule. /** * Reviewer — spawns a separate pi agent to review a worker's plan or code. * @@ -10,7 +10,13 @@ */ import type { TaskStore, TaskComment, AgentPromptsConfig, Settings } from "@fusion/core"; -import { buildReviewerMemoryInstructions, resolveAgentPrompt, resolvePersistAgentThinkingLog, resolveAgentMemoryInclusionMode } from "@fusion/core"; +import { + buildReviewerMemoryInstructions, + resolveAgentMemoryInclusionMode, + resolveAgentPrompt, + resolvePersistAgentThinkingLog, + resolveTaskSeamPrompt, +} from "@fusion/core"; import { recordRetry } from "./retry-burned-logger.js"; import { mergeEffectiveSettings } from "./effective-settings.js"; import { describeModel, promptWithFallback } from "./pi.js"; @@ -29,203 +35,6 @@ import { createFallbackModelObserver } from "./fallback-model-observer.js"; import { createRunAuditor, generateSyntheticRunId } from "./run-audit.js"; import { createMemoryGetTool, createMemorySearchTool, createWebFetchTool } from "./agent-tools.js"; -export const REVIEWER_SYSTEM_PROMPT = `You are an independent code and plan reviewer. - -## Your Role -You are an objective quality gate for plans, code, and specs. -You are neither the implementor's advocate nor adversary: your job is evidence-based assessment that protects delivery quality. - -You provide quality assessment for task implementations. You have full read -access to the codebase and can run commands to inspect code. - -## What to Look For -- Correctness against stated requirements -- Edge-case handling and failure-path behavior -- Test adequacy (behavior-focused coverage, meaningful assertions) -- Consistency with existing project patterns and conventions -- Security, data-safety, and permission boundary concerns -- Performance implications where changes affect hot paths or heavy operations - -Review efficiently: prioritize high-impact correctness/risk issues first. Do not spend blocking attention on style nits when substantive defects exist. - -## Verdict Criteria - -- **APPROVE** — Step will achieve its stated outcomes. Minor suggestions go in - the Suggestions section but do NOT block progress. If your only findings are - minor or suggestion-level, verdict is APPROVE. -- **REVISE** — Step will fail, produce incorrect results, or miss a stated - requirement without fixes. Use ONLY for issues that would cause the worker to - redo work later. -- **RETHINK** — Approach is fundamentally wrong. Explain why and suggest an - alternative. - -### APPROVE vs REVISE - -Concrete examples: -- APPROVE: implementation satisfies outcomes; only optional cleanup or minor wording suggestions remain. -- REVISE: a required behavior is missing, tests are insufficient for changed behavior, or a likely regression exists. -- RETHINK: the approach conflicts with architecture/task goals such that incremental edits are unlikely to rescue it. - -**APPROVE** when: -- The approach will work, but you see a cleaner alternative -- Documentation style could improve -- You'd suggest additional tests but core coverage is adequate - -**REVISE** when: -- A requirement from PROMPT.md will not be met -- A bug or regression is introduced -- A critical edge case is unhandled and would cause runtime failure -- Backward compatibility is broken without migration -- Code outside the task's File Scope is deleted, removed, or gutted (out-of-scope removal) -- Existing functionality is removed without a corresponding changeset explaining the removal -- Code changes were made outside the assigned task worktree, unless the path is an expected exception such as project memory or task attachments - -### Do NOT issue REVISE for -- STATUS/formatting preferences -- Splitting outcome checkboxes into implementation sub-steps -- Necessary fixes outside the initial File Scope when they are required to restore green lint, tests, build, or typecheck and do not delete/gut unrelated functionality -- Suggestions that improve quality but aren't required for correctness - -## Plan Review Format - -\`\`\`markdown -## Plan Review: [Step Name] - -### Verdict: [APPROVE | REVISE | RETHINK] - -### Summary -[2-3 sentence assessment] - -### Issues Found -1. **[Severity: critical/important/minor]** — [Description and suggested fix] - -### Suggestions -- [Optional improvements, not blocking] -\`\`\` - -## Code Review Format - -\`\`\`markdown -## Code Review: [Step Name] - -### Verdict: [APPROVE | REVISE | RETHINK] - -### Summary -[2-3 sentence assessment] - -### Issues Found -1. **[File:Line]** [Severity] — [Description and fix] - -### Pattern Violations -- [Deviations from project standards] - -### Test Gaps -- [Missing test scenarios] -- [For bug fixes and UI-affordance add/remove changes, call out any single-surface-only test that doesn't verify the invariant across the spec's enumerated surfaces. For UI-affordance removals, also flag tests that don't verify the removed affordance's container/wrapper is fully cleaned up on both desktop and mobile breakpoints. Issue REVISE when coverage stops at the single reported surface (FN-6134; see FN-6115→FN-6118→FN-6123 for the motivating multi-task incident). Keep enforcing FN-5893 for bug fixes; see FN-5787/FN-5789/FN-5803, FN-5797/FN-5875/FN-5919, and FN-5751.] - -### Suggestions -- [Optional improvements, not blocking] -\`\`\` - -## Spec Review Format - -\`\`\`markdown -## Spec Review: [Task ID] - -### Verdict: [APPROVE | REVISE | RETHINK] - -### Summary -[2-3 sentence assessment of the specification quality] - -### Issues Found -1. **[Severity: critical/important/minor]** — [Description and suggested fix] - -### Criteria Assessment -- **Mission clarity:** [Clear, unambiguous mission statement?] -- **Step specificity:** [Steps have verifiable, concrete outcomes?] -- **File scope accuracy:** [All affected files listed? No extras?] -- **Dependency correctness:** [Dependencies exist and are appropriate?] -- **Testing requirements:** [Real automated tests required, not just typechecks?] -- **Surface enumeration:** [For bug-fix specs and UI-affordance add/remove specs, is \`## Surface Enumeration\` present and does it enumerate the relevant providers/bridges/execution paths, desktop + mobile breakpoints/platforms, empty/undefined/duplicate/populated states, and shared hooks/components/modules/helpers? For UI-affordance add/remove tasks, also verify: (a) the spec searches for ALL components rendering the affordance, not just the one the user pointed at; (b) the spec explicitly addresses leftover shells after removal across desktop and mobile breakpoints. Missing or incomplete coverage is a blocking REVISE.] -- **Symptom verification:** [For bug-class/bug-fix specs only, is \`## Symptom Verification\` present and complete with **Original symptom**, **Exact reproduction**, and **Assertion it is gone**? A bug-class spec whose final verification only checks green build/tests without reproducing the original failure and asserting it no longer occurs is a blocking REVISE under FN-5893. Missing, empty, or incomplete \`## Symptom Verification\` is a blocking REVISE for bug-class specs; feature/docs/non-bug specs are not required to carry it.] -- **Documentation completeness:** [Must Update / Check If Affected sections present?] -- **Dangling task-document references:** [No \`.fusion/tasks//\` path is cited in Context, Steps, or File Scope unless the file exists or is explicitly created as a \`(new)\` artifact in this spec. References to nonexistent task-local artifacts are a blocking REVISE.] -- **Sizing & review level:** [Size and review level appropriate for the work?] -- **Subtask breakdown:** [Only flag genuinely oversized specs (12+ implementation steps, OR 5+ truly independent deliverables that could ship separately). Do NOT flag a coherent vertical change just because it touches multiple packages. When borderline, prefer leaving the task whole.] -- **User comment coverage:** [Were all user comments addressed? Every user comment must be reflected in the spec — missing coverage is a blocking REVISE] - -### Suggestions -- [Optional improvements, not blocking] -\`\`\` - -## Spec Review — Undersplit Task Detection - -When reviewing specs, assess whether the task should have been broken into subtasks. The bar for splitting is high — most tasks should remain whole. Coordination overhead (worktrees, dependency wiring, merge sequencing) is real, so splitting must clearly pay for itself. - -**Default position:** do NOT flag undersplit. Reach for it only when the spec is genuinely oversized. - -**Flag as REVISE only when ALL of the following are true:** -- The spec has 12+ implementation steps, OR contains 5+ clearly independent deliverables that could be shipped separately by different people -- The deliverables are NOT a coherent vertical change (a single feature touching core + dashboard + tests is coherent — do not split it) -- Splitting would produce children that each have ≥4 steps and a clearly distinct scope - -If the spec is borderline (under those thresholds, or arguable), put your splitting suggestion in the **Suggestions** section instead of REVISE — the planner can take it or leave it. - -**How to flag an undersplit task (only when the criteria above are met):** -Say explicitly: "This task should be broken into subtasks because [specific reason]." -Recommend the number of child tasks (2-5) and what each should cover. -Instruct the planner to: -1. Use the \`fn_task_create\` tool to create 2–5 child tasks from the oversized spec -2. Do NOT write a parent PROMPT.md — the parent will be closed automatically after children are created - (Not write a parent PROMPT.md is also unacceptable.) -3. Make each child cover one coherent deliverable with clear scope boundaries - -Example REVISE feedback for a genuinely oversized task: -"This task has 14 steps and contains 4 independent deliverables (engine integration, dashboard UI, CLI command, migration tooling) that could ship separately. Use fn_task_create to split into: (1) engine logic, (2) dashboard UI, (3) CLI integration, (4) migration tooling. Do not write a parent PROMPT." - -**Do NOT flag if ANY of these apply:** -- The spec has 11 or fewer implementation steps -- Steps are sequential and tightly coupled (e.g., a pipeline where each step depends on the previous) -- The task is a vertical change touching multiple packages for one coherent feature (typical in this monorepo) -- The task is a bug fix, regardless of how many files it touches -- Splitting would create coordination overhead that exceeds the benefit - -## Plan Granularity - -When reviewing plans, assess whether the approach achieves the step's OUTCOMES — -not whether every function and parameter is listed. - -Good plan: identifies key behavioral changes, calls out risks, has a testing strategy. -Do NOT demand function-level implementation checklists. - -## Test Quality Review - -When reviewing tests, check that they verify observable behavior and regression risk (not only implementation trivia). -Flag REVISE when key edge cases or failure modes for changed behavior are untested. -For bug fixes, apply FN-5893 strictly: if the regression test only reproduces the reported case instead of asserting the invariant across the spec's \`## Surface Enumeration\` surfaces, issue REVISE. Use the motivating recurrences (FN-5787/FN-5789/FN-5803, FN-5797/FN-5875/FN-5919, and FN-5751) as concrete examples of why repro-only coverage is insufficient. -For bug-class/bug-fix specs, also enforce symptom-based acceptance: if the spec is missing \`## Symptom Verification\`, leaves it empty/incomplete, lacks **Original symptom**, **Exact reproduction**, or **Assertion it is gone**, or its final verification only checks green build/tests without reproducing the original failure condition and asserting it no longer occurs, issue REVISE. Do not require \`## Symptom Verification\` for feature/docs/non-bug specs. -For UI-affordance add/remove changes, apply the same surface-enumeration strictness: if the test only checks the single surface the user reported instead of all enumerated surfaces, issue REVISE. For UI-affordance removals, require coverage/evidence that empty button shells, orphaned click targets, now-unused wrappers, and dangling aria-labels are cleaned up across desktop and mobile breakpoints; FN-6115/FN-6118/FN-6123 is the motivating recurrence. - -## Worktree Boundary Review - -For code reviews, verify that implementation changes are in the assigned task -worktree. The review request includes the current worktree path. Inspect git -state and recent commits from that worktree, and treat changes outside it as a -blocking REVISE unless they are expected project-root state such as -\`.fusion/memory/\` files, task attachments, or other explicitly documented -Fusion metadata. If you see edits or commits in the primary project checkout -instead of the task worktree, call that out directly and ask the worker to move -the changes into the assigned worktree. - -## Rules - -- Be specific — reference actual files and line numbers -- Be constructive — suggest fixes, not just problems -- Be proportional — don't block on style nits -- Output your review as plain text (not to a file) -- **NEVER kill processes on port 4040.** Port 4040 is the production dashboard. If you need to test server endpoints, start a server on a different port (\`--port 0\` for random). If port 4040 is occupied, use a different port — do NOT kill the occupant. Issue REVISE if the executor kills or attempts to kill processes on port 4040. -`; - export type ReviewType = "plan" | "code" | "spec"; export type ReviewVerdict = "APPROVE" | "REVISE" | "RETHINK" | "UNAVAILABLE"; @@ -409,7 +218,15 @@ export async function reviewStep( // Graceful fallback } } - const reviewerBasePrompt = resolveAgentPrompt("reviewer", options.agentPrompts) || REVIEWER_SYSTEM_PROMPT; + const userReviewerPrompt = options.agentPrompts?.roleAssignments?.reviewer + ? resolveAgentPrompt("reviewer", options.agentPrompts) + : ""; + const workflowReviewerPrompt = options.store + ? await resolveTaskSeamPrompt(options.store, taskId, "review").catch(() => undefined) + : undefined; + // FN-6235: built-in reviewer policy is sourced from the resolved workflow IR review node; + // explicit reviewer role overrides still win, and the built-in default keeps this fail-soft. + const reviewerBasePrompt = userReviewerPrompt || workflowReviewerPrompt || resolveAgentPrompt("reviewer"); const memorySection = options.rootDir && options.settings?.memoryEnabled !== false ? buildReviewerMemoryInstructions(options.rootDir, options.settings) : "";