Skip to content

Commit fad9528

Browse files
Overhaul Critic prompt (#644)
Rewrite Critic identity on the critique package: defects with evidence and no fixes, blinders-on to the brief, no tool-schema restatement or fake caps. Keep package id/path as critique. Closes CL-7021
1 parent 02b6951 commit fad9528

2 files changed

Lines changed: 47 additions & 15 deletions

File tree

src/agent/directors/critique/package.test.ts

Lines changed: 36 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -15,12 +15,29 @@ describe("critiquePackage", () => {
1515
expect(critiquePackage.systemPrompt).toMatch(/PRIMARY INTENT/i);
1616
});
1717

18-
test("systemPrompt is evidence-based and never-fix", () => {
19-
expect(critiquePackage.systemPrompt).toMatch(/evidence-based/i);
20-
expect(critiquePackage.systemPrompt).toMatch(/never fix/i);
21-
expect(critiquePackage.systemPrompt).toMatch(/permanent tests/i);
22-
expect(critiquePackage.systemPrompt).toContain("testsmith/build");
23-
expect(critiquePackage.systemPrompt).toContain("route to build");
18+
test("systemPrompt identity is Critic / CriticDirector (package id stays critique)", () => {
19+
const p = critiquePackage.systemPrompt;
20+
expect(p).toMatch(/CriticDirector \(Critic\)/);
21+
expect(p).toMatch(/review lane only/i);
22+
expect(p).not.toMatch(/CritiqueDirector/);
23+
});
24+
25+
test("systemPrompt is evidence-based defects, never-fix", () => {
26+
const p = critiquePackage.systemPrompt;
27+
expect(p).toMatch(/evidence-based/i);
28+
expect(p).toMatch(/defects with evidence/i);
29+
expect(p).toMatch(/never fix/i);
30+
expect(p).toMatch(/permanent tests/i);
31+
expect(p).toContain("testsmith/build");
32+
expect(p).toContain("route to build");
33+
});
34+
35+
test("systemPrompt has blinders-on / brief-scoped review", () => {
36+
const p = critiquePackage.systemPrompt;
37+
expect(p).toMatch(/BLINDERS ON/i);
38+
expect(p).toMatch(/success_criteria/i);
39+
expect(p).toMatch(/Do not wander/i);
40+
expect(p).toMatch(/invent defects from vibes/i);
2441
});
2542

2643
test("systemPrompt is correctness-only / anti-over-engineering", () => {
@@ -50,6 +67,19 @@ describe("critiquePackage", () => {
5067
expect(critiquePackage.systemPrompt).toMatch(/Rank these as blocking, not style nits/i);
5168
});
5269

70+
test("systemPrompt has no tool-schema restatement or fake caps", () => {
71+
const p = critiquePackage.systemPrompt;
72+
expect(p).not.toMatch(/parameters?:/i);
73+
expect(p).not.toMatch(/fan-out/i);
74+
expect(p).not.toMatch(/at most \d+/i);
75+
expect(p).not.toMatch(/turn budget/i);
76+
expect(p).not.toMatch(/scheduler/i);
77+
expect(p).not.toMatch(/Prefer grep\/search_files/i);
78+
expect(p).not.toMatch(/Shell find\/rg/i);
79+
expect(p).not.toMatch(/Write tools are not mounted/i);
80+
expect(p).not.toMatch(/via run_shell/i);
81+
});
82+
5383
test("spawn.maySpawn is false", () => {
5484
expect(critiquePackage.spawn.maySpawn).toBe(false);
5585
});

src/agent/directors/critique/package.ts

Lines changed: 11 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,9 @@ import type { DirectorPackage } from "../types.js";
22
import { REVIEW_TOOLS } from "../tool-sets.js";
33

44
/**
5-
* Critique leaf (CL-5819).
6-
* Evidence-based code review — find defects with proof; never fix product code.
5+
* Critique leaf (CL-5819 / CL-7021).
6+
* Critic identity — defects with evidence; never fix product code.
7+
* Package id/path stays `critique` (global rename is out of scope).
78
*/
89
export const critiquePackage: DirectorPackage = {
910
id: "critique",
@@ -21,18 +22,19 @@ export const critiquePackage: DirectorPackage = {
2122
spawn: { maySpawn: false },
2223
tier: "leaf",
2324
modelRole: "review",
24-
systemPrompt: `You are CritiqueDirector, a specialist in Corbits Code.
25+
systemPrompt: `You are CriticDirector (Critic), a specialist in Corbits Code.
2526
26-
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.
27+
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.
2728
28-
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.
29+
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.
30+
31+
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.
2932
3033
Evidence rules:
3134
- Every claim needs path + line/symbol + reproduction shape (input, sequence, missing branch).
32-
- Prefer grep/search_files/lsp/read_file over shell walks. Shell find/rg -r are blocked — do not work around.
3335
- Rank findings: blocking, should-fix, file-for-later. "This is genuinely fine" is a valid finding when true.
3436
- Call out gaps: what you did not cover so the parent does not assume closed.
35-
- Recommend permanent tests the suite should keep (name the scenario; do not implement them here).
37+
- Recommend permanent tests the suite should keep (name the scenario; do not implement them here — route to testsmith/build).
3638
3739
Correctness-only / anti-over-engineering:
3840
- Flag only gaps that affect correctness or the stated requirements/success_criteria.
@@ -43,10 +45,10 @@ API contract check (blocking when brief specifies signatures):
4345
- Compare public exports against the brief and existing call sites/tests.
4446
- Sync → async (returning Promise when callers expect a plain value) is a blocking correctness defect.
4547
- Signature parameter order/optionality/return-type drift vs brief is blocking.
46-
- Prefer reading tests/callers; a tiny sync call via run_shell that would hang on a Promise is evidence.
48+
- Prefer reading tests/callers; a tiny sync call that would hang on a Promise is evidence.
4749
- Rank these as blocking, not style nits.
4850
49-
Write tools are not mounted. Repro via read/shell only; recommend permanent tests for testsmith/build.
51+
Before substantial review work: follow style and philosophy conventions (baked; use_skill is not mounted on workers). Read the code under review.
5052
5153
OUT OF LANE → refuse or reclassify under Blockers:
5254
- implementing fixes (route to build)

0 commit comments

Comments
 (0)