FN-6235: source reviewer prompts from workflow IR
Deduplicate reviewer policy text by making workflow IR review seams the engine's built-in prompt source. - Move the canonical built-in reviewer prompt into core agent prompts and export seam prompt resolution helpers. - Resolve reviewer prompts from explicit role overrides first, then workflow IR review seams, with the built-in prompt as a fallback. - Update prompt cache and reviewer tests to cover single-source reviewer prompt behavior. - Add a patch changeset for the published Fusion package. Files changed: .../FN-6235-reviewer-prompt-single-source.md | 5 + packages/core/src/__tests__/agent-prompts.test.ts | 8 +- packages/core/src/agent-prompts.ts | 109 +++++++++-- packages/core/src/index.ts | 2 + packages/core/src/workflow-ir-resolver.ts | 31 ++- .../src/__tests__/prompt-cache-integration.test.ts | 10 +- .../reviewer-prompt-single-source.test.ts | 160 +++++++++++++++ packages/engine/src/__tests__/reviewer.test.ts | 97 ++++----- packages/engine/src/prompt-layers.ts | 2 +- packages/engine/src/reviewer.ts | 217 ++------------------- 10 files changed, 370 insertions(+), 271 deletions(-) Fusion-Task-Id: FN-6235 Fusion-Task-Lineage: 725932d1-2469-4507-a0f9-0946c83a1572
This commit is contained in:
gsxdsm
5
.changeset/FN-6235-reviewer-prompt-single-source.md
Normal file
5
.changeset/FN-6235-reviewer-prompt-single-source.md
Normal file
@@ -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.
|
||||
@@ -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", () => {
|
||||
|
||||
@@ -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/<id>/<file>\` 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
|
||||
|
||||
@@ -314,7 +314,9 @@ export {
|
||||
export {
|
||||
resolveWorkflowIrForTask,
|
||||
resolveWorkflowIrById,
|
||||
resolveSeamPromptFromIr,
|
||||
resolvePlanningPromptFromIr,
|
||||
resolveTaskSeamPrompt,
|
||||
resolveTaskPlanningPrompt,
|
||||
type WorkflowIrResolverStore,
|
||||
} from "./workflow-ir-resolver.js";
|
||||
|
||||
@@ -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<string, WorkflowIr>,
|
||||
): Promise<string | undefined> {
|
||||
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<string, WorkflowIr>,
|
||||
): Promise<string | undefined> {
|
||||
const ir = await resolveWorkflowIrForTask(store, taskId, irCache);
|
||||
return resolvePlanningPromptFromIr(ir);
|
||||
return resolveTaskSeamPrompt(store, taskId, "planning", irCache);
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
@@ -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);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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<typeof reviewStep>[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"));
|
||||
});
|
||||
});
|
||||
@@ -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/");
|
||||
});
|
||||
});
|
||||
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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/<id>/<file>\` 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)
|
||||
: "";
|
||||
|
||||
Reference in New Issue
Block a user