Skip to content

Commit 76fb079

Browse files
committed
fix: implemented semantic structure to review prompt
1 parent de20243 commit 76fb079

8 files changed

Lines changed: 103 additions & 63 deletions

File tree

api/.env.example

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -131,6 +131,10 @@ MAX_VERIFY_ATTEMPTS=3
131131
MAX_COMPLETION_RETRIES=3
132132
# Max review passes per cycle before opening a draft PR.
133133
MAX_REVIEW_PASSES=3
134+
# Max CONSECUTIVE review passes whose output can't be parsed into a verdict before giving up on
135+
# self-review and opening a draft PR. Each unparseable pass is re-run with stricter schema guidance;
136+
# only after this many in a row do we bail (a stray single failure among good passes never counts).
137+
MAX_REVIEW_PARSE_RETRIES=2
134138
# Soft cap on lines returned to the IMPLEMENT/REVISE *primary* agent from a single file read.
135139
# Beyond this the read is truncated with a note to delegate the survey to a sub-agent (sub-agents
136140
# are uncapped) — stops the orchestrator bloating its own context by reading large files itself.

api/src/config/config.model.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,10 @@ export const envSchema = z.object({
5353
// anyway. 0 disables the judge loop entirely.
5454
MAX_COMPLETION_RETRIES: intFromString(2),
5555
MAX_REVIEW_PASSES: intFromString(5),
56+
// Max consecutive review passes whose output can't be parsed into a verdict before giving up on
57+
// self-review and opening a draft PR for a human. Each unparseable pass is re-run with stricter
58+
// schema guidance; only after this many in a row do we bail.
59+
MAX_REVIEW_PARSE_RETRIES: intFromString(2),
5660
// Soft cap on lines returned to the IMPLEMENT/REVISE *primary* from a single file read; over
5761
// this, the read is truncated with a "delegate the survey" note (sub-agents are uncapped). Keeps
5862
// the orchestrator from bloating its context by reading large files itself. Forwarded into the

api/src/implement/implement.prompts.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ export function buildImplementPrompt(ctx: ImplementPromptContext): string {
2424
// headings (e.g. a plan's "## Steps") never read as one of the prompt's own sections.
2525
const context: string[] = [
2626
`--- ISSUE: ${ctx.issueTitle} ---\n${ctx.issueBody}\n--- END ISSUE ---`,
27-
`--- APPROVED PLAN ---\n${ctx.plan}\n--- END PLAN ---`,
27+
`--- PLAN ---\n${ctx.plan}\n--- END PLAN ---`,
2828
];
2929

3030
if (ctx.guidance) {

api/src/orchestrator/orchestrator.service.spec.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -133,6 +133,7 @@ function setup(overrides: { job?: Record<string, unknown> } = {}) {
133133
persist: resolved(undefined),
134134
threshold: 85,
135135
maxPasses: 5,
136+
maxParseRetries: 2,
136137
};
137138

138139
const verify = {

api/src/orchestrator/orchestrator.service.ts

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1565,15 +1565,16 @@ export class OrchestratorService {
15651565
return;
15661566
}
15671567

1568-
// Count how many passes in this cycle produced no parseable JSON verdict.
1569-
// After 2 consecutive unparseable passes the model is unlikely to self-correct,
1570-
// so fall through to the draft-PR path rather than burning the entire pass budget.
1568+
// Count this cycle's consecutive unparseable passes (priorUnparseable is already consecutive).
1569+
// After MAX_REVIEW_PARSE_RETRIES in a row the model is unlikely to self-correct, so fall through
1570+
// to the draft-PR path rather than burning the entire pass budget on malformed output.
15711571
const unparseablePasses = priorUnparseable + (parsed === null ? 1 : 0);
1572+
const maxParseRetries = this.review.maxParseRetries;
15721573

15731574
// Only revise when the review produced parseable, actionable issues.
15741575
// If the output couldn't be parsed, retry the review so the agent gets another
1575-
// chance to emit valid JSON — but cap unparseable retries at 2.
1576-
if (pass < maxPasses && (parsed !== null || unparseablePasses < 2)) {
1576+
// chance to emit valid JSON — but cap consecutive unparseable retries.
1577+
if (pass < maxPasses && (parsed !== null || unparseablePasses < maxParseRetries)) {
15771578
if (parsed !== null) {
15781579
await this.jobs.transition(jobId, 'REVISING', {
15791580
reason: `addressing review pass ${pass}`,
@@ -1588,7 +1589,7 @@ export class OrchestratorService {
15881589
}
15891590

15901591
const reason =
1591-
unparseablePasses >= 2
1592+
unparseablePasses >= maxParseRetries
15921593
? `Self-review produced unparseable output ${unparseablePasses} times in a row. Opening a draft PR for human review.`
15931594
: `Self-review didn't reach the confidence threshold after ${pass} passes (best ${result.confidence}/100). Opening a draft PR for human review.`;
15941595

api/src/review/review.prompts.spec.ts

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,12 @@ describe('buildReviewPrompt parseRetry guidance', () => {
1717
expect(p).not.toContain('RETRY');
1818
});
1919

20+
it('forbids writing the verdict to a file on every pass (inline JSON only)', () => {
21+
const p = buildReviewPrompt(baseCtx);
22+
expect(p).toContain('review.json');
23+
expect(p).toMatch(/do NOT write the (review|verdict)/i);
24+
});
25+
2026
it('injects schema-conformance guidance when retrying after an unparseable pass', () => {
2127
const p = buildReviewPrompt({ ...baseCtx, parseRetry: true });
2228
expect(p).toContain('RETRY');

api/src/review/review.prompts.ts

Lines changed: 75 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -1,49 +1,13 @@
11
import { AUTONOMY_NOTICE } from '../agent/agent.prompts.js';
22
import { type ReviewPromptContext } from './review.model.js';
33

4-
export function buildReviewPrompt(ctx: ReviewPromptContext): string {
5-
const parts: string[] = [
6-
`You are Hermes acting as a rigorous senior code reviewer for \`${ctx.repoFullName}\`. The working directory contains a branch with changes committed on top of \`${ctx.baseBranch}\`. Review the diff of this branch against \`${ctx.baseBranch}\` (use git to inspect it). Use \`.olympian/\` as a scratch directory for any temporary files such as diffs — it is excluded from commits automatically. **This is a read-only review: do NOT modify any source files and do NOT use the clarify tool. If you find a bug, record it in the issues array — do not fix it.**`,
7-
`Judge whether the changes correctly and completely resolve the issue, satisfy the plan's acceptance criteria, and meet a professional bar for correctness, security, and tests.`,
8-
`**Read efficiently, but never review less.** Inspect the FULL diff against \`${ctx.baseBranch}\` — every changed file and every hunk. When you need surrounding context (a changed function's callers, a type or constant it relies on, related logic, the tests that cover it), use \`search_files\` to read the specific 20-40 line window instead of loading whole files. This is purely about HOW you read: a lean context won't get compressed mid-review, which is exactly when defects slip through. It must never narrow WHAT you review — follow the diff outward wherever a change could introduce a bug (broken callers, unhandled edge cases, security or data-loss risks, missing or weak tests), and do a complete independent pass even on hunks that look fine. If understanding a change demands reading more, read more.`,
9-
`--- ISSUE: ${ctx.issueTitle} ---\n${ctx.issueBody}\n--- END ISSUE ---`,
10-
`--- APPROVED PLAN ---\n${ctx.plan}\n--- END PLAN ---`,
11-
];
4+
const TESTS_CONTRACT = `# Tests
125
13-
if (ctx.humanFeedback) {
14-
parts.push(
15-
`--- HUMAN PR REVIEW FEEDBACK (highest priority — verify every point is addressed) ---\n${ctx.humanFeedback}\n--- END FEEDBACK ---`,
16-
);
17-
}
6+
The project's automated checks (tests/build) have already passed in a separate VERIFY stage — a green result is therefore a given, not evidence of quality, and the implementer wrote its own tests. **Scrutinise the tests themselves:** confirm an automated test exists for each acceptance criterion, that each genuinely exercises the new behaviour (it would fail without the implementation — watch for trivial, tautological, or assertion-free tests), and that no existing test was weakened, skipped, or deleted to reach green. Treat a missing or gamed test as a tests-dimension failure. Then focus on correctness, security, and full coverage of the acceptance criteria.`;
187

19-
if (ctx.priorIssues && ctx.priorIssues.length > 0) {
20-
const formatted = ctx.priorIssues
21-
.map((issue, i) => {
22-
const loc = issue.file ? ` (${issue.file})` : '';
23-
return `${i + 1}. [${issue.severity}] ${issue.title}${loc}\n ${issue.detail}`;
24-
})
25-
.join('\n');
8+
const BROWSER_CONTRACT = `# Browser smoke-test (optional)
269
27-
parts.push(
28-
`--- ISSUES FROM PRIOR REVIEW PASS (verify each is now resolved) ---\n${formatted}\n--- END PRIOR ISSUES ---\n\nFor each prior issue, explicitly state in your summary whether it is resolved. **If an issue is not fully resolved — including partial fixes — it MUST appear in your JSON "issues" array.** Mentioning it only in the summary is not sufficient; the issues array is the only signal the next revision receives. Then perform a full independent review of all changes to catch any additional problems not listed above.`,
29-
);
30-
}
31-
32-
// The repo's tests/build already ran and passed in the dedicated VERIFY stage
33-
// before this review (a failure would have routed to REVISE, not here).
34-
parts.push(
35-
`The project's automated checks (tests/build) have already passed in a separate VERIFY stage — a green result is therefore a given, not evidence of quality, and the implementer wrote its own tests. **Scrutinise the tests themselves:** confirm an automated test exists for each acceptance criterion, that each genuinely exercises the new behaviour (it would fail without the implementation — watch for trivial, tautological, or assertion-free tests), and that no existing test was weakened, skipped, or deleted to reach green. Treat a missing or gamed test as a tests-dimension failure. Then focus on correctness, security, and full coverage of the acceptance criteria.`,
36-
);
37-
38-
if (ctx.outOfPlanFiles && ctx.outOfPlanFiles.length > 0) {
39-
parts.push(
40-
`--- SCOPE CHECK ---\nThese files were changed but aren't in the approved plan's "Files to change". Out-of-plan changes are frequently legitimate: a fix the build/tests required, a shared type/config, or repairing pre-existing breakage in another package/workspace. The VERIFY stage has already PASSED, so any change the green build depends on is in scope by definition — do NOT ask for it to be reverted. Only raise an issue if a change is clearly unrelated to the task, unnecessary for a passing build, AND risky (a genuine regression or accidental edit). A pure "this is beyond the plan" observation is at most "low" severity — NEVER high/critical — and on its own must not set "dimensions.criteria" to false. Files:\n${ctx.outOfPlanFiles.map((f) => `- ${f}`).join('\n')}\n--- END SCOPE CHECK ---`,
41-
);
42-
}
43-
44-
if (ctx.hasBrowser) {
45-
parts.push(
46-
`A Camofox browser is available for a smoke-test of the running application. **This is secondary to the code review — do not let it block your verdict.**
10+
A Camofox browser is available for a smoke-test of the running application. **This is secondary to the code review — do not let it block your verdict.**
4711
4812
Steps (do them exactly once in this order, then move on):
4913
1. Start the dev server bound to all interfaces so Camofox can reach it through Docker's port proxy. Determine the framework from package.json first, then use the appropriate command:
@@ -58,15 +22,11 @@ Steps (do them exactly once in this order, then move on):
5822
5923
**Ports forwarded to Camofox:** 3000, 3001, 4000, 4200, 5000, 5173, 5174, 8000, 8080, 8888. Navigate to \`http://localhost:<port>\` — do not use the container hostname or any internal IP.
6024
61-
If the server fails to start, skip the browser step and note it in your summary — your code-level verdict still stands.`,
62-
);
63-
}
25+
If the server fails to start, skip the browser step and note it in your summary — your code-level verdict still stands.`;
6426

65-
parts.push(AUTONOMY_NOTICE);
27+
const VERDICT_CONTRACT = `# Output — verdict (required, inline)
6628
67-
parts.push(
68-
`Files changed on this branch:\n${ctx.changedFiles.map((f) => `- ${f}`).join('\n') || '(none detected)'}`,
69-
`Output your verdict as the FIRST thing in your response — a \`\`\`json block before any other text:
29+
Output your verdict as the FIRST thing in your response — a \`\`\`json block before any other text. This inline block is the ONLY thing read; do NOT write it to a file, and do NOT replace it with a prose summary (e.g. "the review is written to review.json"):
7030
\`\`\`json
7131
{
7232
"confidence": <integer 0-100, advisory only — your subjective confidence>,
@@ -83,18 +43,77 @@ If the server fails to start, skip the browser step and note it in your summary
8343
]
8444
}
8545
\`\`\`
86-
**The rubric is the gate, not the confidence number.** Set "verdict" to "PASS" ONLY when ALL FOUR dimensions are true AND there are no high/critical issues. Mark a dimension false the moment you are not confident it fully holds — err toward false. List every concrete problem in "issues" (empty array if none). You may include detailed reasoning after the JSON block.`,
46+
**The rubric is the gate, not the confidence number.** Set "verdict" to "PASS" ONLY when ALL FOUR dimensions are true AND there are no high/critical issues. Mark a dimension false the moment you are not confident it fully holds — err toward false. List every concrete problem in "issues" (empty array if none). You may include detailed reasoning after the JSON block.`;
47+
48+
const RETRY_CONTRACT = `# Retry — your previous output was rejected
49+
50+
IMPORTANT — RETRY: your previous response could not be parsed against the required schema, so this review is being re-run. The verdict was discarded; none of that prior analysis was recorded. Conform EXACTLY this time:
51+
- Start the response with a single \`\`\`json fenced block — no preamble, narrative, or prose before it. Do NOT write the verdict to a file (\`review.json\` or similar) and then summarise — the file is ignored; only this inline block is read.
52+
- \`verdict\` MUST be the string "PASS" or "FAIL" (uppercase) — NOT a boolean (\`true\`/\`false\`), number, or any other word.
53+
- \`confidence\` MUST be present, as an integer 0-100.
54+
- \`dimensions\` MUST contain all four boolean keys: \`correctness\`, \`tests\`, \`criteria\`, \`security\`.
55+
- MOST IMPORTANT — \`issues\`: every concrete problem MUST be a structured object in the \`issues\` array with the exact \`{severity,title,detail,file?}\` shape. This array is the ONLY thing passed to the agent that fixes the code — any finding left out, written as prose, or placed under a stray key (\`rationale\`, \`findings\`, \`explanation\`, …) is INVISIBLE to the fix stage and WILL NOT be fixed. Each \`detail\` must say both what is wrong and how to fix it. Put a FAIL's full reasoning here, not after the block.`;
56+
57+
export function buildReviewPrompt(ctx: ReviewPromptContext): string {
58+
// Injected documents stay wrapped in `--- NAME --- … ---` fences so their own Markdown headings
59+
// never read as one of the prompt's own sections (mirrors the implement/revise prompts).
60+
const context: string[] = [
61+
`--- ISSUE: ${ctx.issueTitle} ---\n${ctx.issueBody}\n--- END ISSUE ---`,
62+
`--- PLAN ---\n${ctx.plan}\n--- END PLAN ---`,
63+
];
64+
65+
if (ctx.humanFeedback) {
66+
context.push(
67+
`--- HUMAN PR REVIEW FEEDBACK (highest priority — verify every point is addressed) ---\n${ctx.humanFeedback}\n--- END FEEDBACK ---`,
68+
);
69+
}
70+
71+
if (ctx.priorIssues && ctx.priorIssues.length > 0) {
72+
const formatted = ctx.priorIssues
73+
.map((issue, i) => {
74+
const loc = issue.file ? ` (${issue.file})` : '';
75+
return `${i + 1}. [${issue.severity}] ${issue.title}${loc}\n ${issue.detail}`;
76+
})
77+
.join('\n');
78+
79+
context.push(
80+
`--- ISSUES FROM PRIOR REVIEW PASS (verify each is now resolved) ---\n${formatted}\n--- END PRIOR ISSUES ---\n\nFor each prior issue, explicitly state in your summary whether it is resolved. **If an issue is not fully resolved — including partial fixes — it MUST appear in your JSON "issues" array.** Mentioning it only in the summary is not sufficient; the issues array is the only signal the next revision receives. Then perform a full independent review of all changes to catch any additional problems not listed above.`,
81+
);
82+
}
83+
84+
if (ctx.outOfPlanFiles && ctx.outOfPlanFiles.length > 0) {
85+
context.push(
86+
`--- SCOPE CHECK ---\nThese files were changed but aren't in the approved plan's "Files to change". Out-of-plan changes are frequently legitimate: a fix the build/tests required, a shared type/config, or repairing pre-existing breakage in another package/workspace. The VERIFY stage has already PASSED, so any change the green build depends on is in scope by definition — do NOT ask for it to be reverted. Only raise an issue if a change is clearly unrelated to the task, unnecessary for a passing build, AND risky (a genuine regression or accidental edit). A pure "this is beyond the plan" observation is at most "low" severity — NEVER high/critical — and on its own must not set "dimensions.criteria" to false. Files:\n${ctx.outOfPlanFiles.map((f) => `- ${f}`).join('\n')}\n--- END SCOPE CHECK ---`,
87+
);
88+
}
89+
90+
context.push(
91+
`--- FILES CHANGED ON THIS BRANCH ---\n${ctx.changedFiles.map((f) => `- ${f}`).join('\n') || '(none detected)'}\n--- END FILES CHANGED ---`,
8792
);
8893

94+
const parts: string[] = [
95+
`# Role
96+
97+
You are Hermes acting as a rigorous senior code reviewer for \`${ctx.repoFullName}\`. The working directory contains a branch with changes committed on top of \`${ctx.baseBranch}\`; review the diff of this branch against \`${ctx.baseBranch}\` (use git to inspect it). Use \`.olympian/\` as a scratch directory for temporary files such as diffs — it is excluded from commits automatically.
98+
99+
**This is a read-only review:** do NOT modify any source files and do NOT use the clarify tool. If you find a bug, record it in the issues array — do not fix it. **Your verdict must be returned INLINE in your final message as the JSON block in "# Output" — do NOT write the review or verdict to a file (e.g. \`review.json\`); a file is NEVER read and the review will be discarded as unparseable.**`,
100+
`# Context\n\n${context.join('\n\n')}`,
101+
`# Reviewing
102+
103+
Judge whether the changes correctly and completely resolve the issue, satisfy the plan's acceptance criteria, and meet a professional bar for correctness, security, and tests.
104+
105+
**Read efficiently, but never review less.** Inspect the FULL diff against \`${ctx.baseBranch}\` — every changed file and every hunk. When you need surrounding context (a changed function's callers, a type or constant it relies on, related logic, the tests that cover it), use \`search_files\` to read the specific 20-40 line window instead of loading whole files. This is purely about HOW you read: a lean context won't get compressed mid-review, which is exactly when defects slip through. It must never narrow WHAT you review — follow the diff outward wherever a change could introduce a bug (broken callers, unhandled edge cases, security or data-loss risks, missing or weak tests), and do a complete independent pass even on hunks that look fine. If understanding a change demands reading more, read more.`,
106+
TESTS_CONTRACT,
107+
];
108+
109+
if (ctx.hasBrowser) {
110+
parts.push(BROWSER_CONTRACT);
111+
}
112+
113+
parts.push(AUTONOMY_NOTICE, VERDICT_CONTRACT);
114+
89115
if (ctx.parseRetry) {
90-
parts.push(
91-
`IMPORTANT — RETRY: your previous response could not be parsed against the required schema, so this review is being re-run. The verdict was discarded; none of that prior analysis was recorded. Conform EXACTLY this time:\n` +
92-
`- Start the response with a single \`\`\`json fenced block — no preamble, narrative, or prose before it.\n` +
93-
`- \`verdict\` MUST be the string "PASS" or "FAIL" (uppercase) — NOT a boolean (\`true\`/\`false\`), number, or any other word.\n` +
94-
`- \`confidence\` MUST be present, as an integer 0-100.\n` +
95-
`- \`dimensions\` MUST contain all four boolean keys: \`correctness\`, \`tests\`, \`criteria\`, \`security\`.\n` +
96-
`- MOST IMPORTANT — \`issues\`: every concrete problem MUST be a structured object in the \`issues\` array with the exact \`{severity,title,detail,file?}\` shape. This array is the ONLY thing passed to the agent that fixes the code — any finding left out, written as prose, or placed under a stray key (\`rationale\`, \`findings\`, \`explanation\`, …) is INVISIBLE to the fix stage and WILL NOT be fixed. Each \`detail\` must say both what is wrong and how to fix it. Put a FAIL's full reasoning here, not after the block.`,
97-
);
116+
parts.push(RETRY_CONTRACT);
98117
}
99118

100119
return parts.join('\n\n');

0 commit comments

Comments
 (0)