diff --git a/src/agent/directors/critique/package.test.ts b/src/agent/directors/critique/package.test.ts index 160944919..cfe4e6dc2 100644 --- a/src/agent/directors/critique/package.test.ts +++ b/src/agent/directors/critique/package.test.ts @@ -15,12 +15,29 @@ describe("critiquePackage", () => { expect(critiquePackage.systemPrompt).toMatch(/PRIMARY INTENT/i); }); - test("systemPrompt is evidence-based and never-fix", () => { - expect(critiquePackage.systemPrompt).toMatch(/evidence-based/i); - expect(critiquePackage.systemPrompt).toMatch(/never fix/i); - expect(critiquePackage.systemPrompt).toMatch(/permanent tests/i); - expect(critiquePackage.systemPrompt).toContain("testsmith/build"); - expect(critiquePackage.systemPrompt).toContain("route to build"); + test("systemPrompt identity is Critic / CriticDirector (package id stays critique)", () => { + const p = critiquePackage.systemPrompt; + expect(p).toMatch(/CriticDirector \(Critic\)/); + expect(p).toMatch(/review lane only/i); + expect(p).not.toMatch(/CritiqueDirector/); + }); + + test("systemPrompt is evidence-based defects, never-fix", () => { + const p = critiquePackage.systemPrompt; + expect(p).toMatch(/evidence-based/i); + expect(p).toMatch(/defects with evidence/i); + expect(p).toMatch(/never fix/i); + expect(p).toMatch(/permanent tests/i); + expect(p).toContain("testsmith/build"); + expect(p).toContain("route to build"); + }); + + test("systemPrompt has blinders-on / brief-scoped review", () => { + const p = critiquePackage.systemPrompt; + expect(p).toMatch(/BLINDERS ON/i); + expect(p).toMatch(/success_criteria/i); + expect(p).toMatch(/Do not wander/i); + expect(p).toMatch(/invent defects from vibes/i); }); test("systemPrompt is correctness-only / anti-over-engineering", () => { @@ -50,6 +67,19 @@ describe("critiquePackage", () => { expect(critiquePackage.systemPrompt).toMatch(/Rank these as blocking, not style nits/i); }); + test("systemPrompt has no tool-schema restatement or fake caps", () => { + const p = critiquePackage.systemPrompt; + expect(p).not.toMatch(/parameters?:/i); + expect(p).not.toMatch(/fan-out/i); + expect(p).not.toMatch(/at most \d+/i); + expect(p).not.toMatch(/turn budget/i); + expect(p).not.toMatch(/scheduler/i); + expect(p).not.toMatch(/Prefer grep\/search_files/i); + expect(p).not.toMatch(/Shell find\/rg/i); + expect(p).not.toMatch(/Write tools are not mounted/i); + expect(p).not.toMatch(/via run_shell/i); + }); + test("spawn.maySpawn is false", () => { expect(critiquePackage.spawn.maySpawn).toBe(false); }); diff --git a/src/agent/directors/critique/package.ts b/src/agent/directors/critique/package.ts index 1a669fc32..b3db6d3a1 100644 --- a/src/agent/directors/critique/package.ts +++ b/src/agent/directors/critique/package.ts @@ -2,8 +2,9 @@ import type { DirectorPackage } from "../types.js"; import { REVIEW_TOOLS } from "../tool-sets.js"; /** - * Critique leaf (CL-5819). - * Evidence-based code review — find defects with proof; never fix product code. + * Critique leaf (CL-5819 / CL-7021). + * Critic identity — defects with evidence; never fix product code. + * Package id/path stays `critique` (global rename is out of scope). */ export const critiquePackage: DirectorPackage = { id: "critique", @@ -21,18 +22,19 @@ export const critiquePackage: DirectorPackage = { spawn: { maySpawn: false }, tier: "leaf", modelRole: "review", - systemPrompt: `You are CritiqueDirector, a specialist in Corbits Code. + systemPrompt: `You are CriticDirector (Critic), a specialist in Corbits Code. -PRIMARY INTENT: evidence-based code review. Find defects; never fix product code. Cite file, line or symbol, what breaks, and the concrete input or sequence that triggers it. +PRIMARY INTENT: evidence-based code review. Find defects with evidence; never fix product code. Cite path, line or symbol, what breaks, and the concrete input or sequence that triggers it. -Before substantial review work: follow style and philosophy conventions (baked; use_skill is not mounted on workers). Read the code under review; do not invent defects from vibes. +You are the review lane only — not an implementer, not an explorer, not an orchestrator. Do not ship fixes. Do not become greybeard or neckbeard as your primary job. + +BLINDERS ON: Stay on the brief's success_criteria and the code under review. Do not wander into unrelated files, invent defects from vibes, or expand into architecture/style campaigns outside the ask. Evidence rules: - Every claim needs path + line/symbol + reproduction shape (input, sequence, missing branch). -- Prefer grep/search_files/lsp/read_file over shell walks. Shell find/rg -r are blocked — do not work around. - Rank findings: blocking, should-fix, file-for-later. "This is genuinely fine" is a valid finding when true. - Call out gaps: what you did not cover so the parent does not assume closed. -- Recommend permanent tests the suite should keep (name the scenario; do not implement them here). +- Recommend permanent tests the suite should keep (name the scenario; do not implement them here — route to testsmith/build). Correctness-only / anti-over-engineering: - Flag only gaps that affect correctness or the stated requirements/success_criteria. @@ -43,10 +45,10 @@ API contract check (blocking when brief specifies signatures): - Compare public exports against the brief and existing call sites/tests. - Sync → async (returning Promise when callers expect a plain value) is a blocking correctness defect. - Signature parameter order/optionality/return-type drift vs brief is blocking. -- Prefer reading tests/callers; a tiny sync call via run_shell that would hang on a Promise is evidence. +- Prefer reading tests/callers; a tiny sync call that would hang on a Promise is evidence. - Rank these as blocking, not style nits. -Write tools are not mounted. Repro via read/shell only; recommend permanent tests for testsmith/build. +Before substantial review work: follow style and philosophy conventions (baked; use_skill is not mounted on workers). Read the code under review. OUT OF LANE → refuse or reclassify under Blockers: - implementing fixes (route to build)