refactor: aligned plan criteria dimemsion semantically

Author: jahilldev

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

@@ -55,7 +55,7 @@ const CONFIG: Record<string, unknown> = {
 };
 
 const PASS_REVIEW_JSON =
-  '```json\n{"confidence":90,"verdict":"PASS","dimensions":{"correctness":true,"tests":true,"planCoverage":true,"security":true},"issues":[]}\n```';
+  '```json\n{"confidence":90,"verdict":"PASS","dimensions":{"correctness":true,"tests":true,"criteria":true,"security":true},"issues":[]}\n```';
 
 const okRun = {
   runId: 'run1',

api/src/orchestrator/orchestrator.service.ts

@@ -1539,7 +1539,7 @@ export class OrchestratorService {
           dimensions: {
             correctness: false,
             tests: false,
-            planCoverage: false,
+            criteria: false,
             security: false,
           },
           issues: [

api/src/review/review.model.ts

@@ -13,7 +13,7 @@ export const UNPARSEABLE_REVIEW_TITLE = 'Unparseable review output';
  * Rubric dimensions the reviewer grades independently. Each is a hard gate: the
  * verdict can only be PASS if every dimension holds. Confidence is advisory only.
  */
-export const REVIEW_DIMENSIONS = ['correctness', 'tests', 'planCoverage', 'security'] as const;
+export const REVIEW_DIMENSIONS = ['correctness', 'tests', 'criteria', 'security'] as const;
 export type ReviewDimension = (typeof REVIEW_DIMENSIONS)[number];
 export type ReviewDimensions = Record<ReviewDimension, boolean>;
 

api/src/review/review.prompts.ts

@@ -32,12 +32,12 @@ export function buildReviewPrompt(ctx: ReviewPromptContext): string {
   // The repo's tests/build already ran and passed in the dedicated VERIFY stage
   // before this review (a failure would have routed to REVISE, not here).
   parts.push(
-    `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 plan coverage.`,
+    `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.`,
   );
 
   if (ctx.outOfPlanFiles && ctx.outOfPlanFiles.length > 0) {
     parts.push(
-      `--- 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.planCoverage" to false. Files:\n${ctx.outOfPlanFiles.map((f) => `- ${f}`).join('\n')}\n--- END SCOPE CHECK ---`,
+      `--- 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 ---`,
     );
   }
 
@@ -74,7 +74,7 @@ If the server fails to start, skip the browser step and note it in your summary
   "dimensions": {
     "correctness": <true|false: the change is logically correct and resolves the issue>,
     "tests": <true|false: automated tests meaningfully encode each acceptance criterion — they exercise the new behaviour and would fail without it — and no existing test was weakened, skipped, or deleted>,
-    "planCoverage": <true|false: every acceptance criterion in the plan is met. Necessary supporting changes (build/test fixes, shared config, repairing other packages the verify gate needs) are fine — only set false for missing criteria or material, unjustified, unrelated divergence>,
+    "criteria": <true|false: every acceptance criterion in the plan is met. Necessary supporting changes (build/test fixes, shared config, repairing other packages the verify gate needs) are fine — only set false for missing criteria or material, unjustified, unrelated divergence>,
     "security": <true|false: no injection, secret-leak, auth, or unsafe-input problems introduced>
   },
   "summary": "<one-paragraph assessment>",
@@ -92,7 +92,7 @@ If the server fails to start, skip the browser step and note it in your summary
         `- Start the response with a single \`\`\`json fenced block — no preamble, narrative, or prose before it.\n` +
         `- \`verdict\` MUST be the string "PASS" or "FAIL" (uppercase) — NOT a boolean (\`true\`/\`false\`), number, or any other word.\n` +
         `- \`confidence\` MUST be present, as an integer 0-100.\n` +
-        `- \`dimensions\` MUST contain all four boolean keys: \`correctness\`, \`tests\`, \`planCoverage\`, \`security\`.\n` +
+        `- \`dimensions\` MUST contain all four boolean keys: \`correctness\`, \`tests\`, \`criteria\`, \`security\`.\n` +
         `- 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.`,
     );
   }

api/src/review/review.utility.spec.ts

@@ -8,7 +8,7 @@ import {
 } from './review.utility.js';
 import { type ReviewResult } from './review.model.js';
 
-const PASS_DIMS = { correctness: true, tests: true, planCoverage: true, security: true };
+const PASS_DIMS = { correctness: true, tests: true, criteria: true, security: true };
 
 function result(overrides: Partial<ReviewResult> = {}): ReviewResult {
   return {
@@ -24,25 +24,25 @@ function result(overrides: Partial<ReviewResult> = {}): ReviewResult {
 describe('parseReview', () => {
   it('parses a fenced json verdict with rubric dimensions', () => {
     const out =
-      'Here is my review:\n```json\n{"confidence":92,"verdict":"PASS","dimensions":{"correctness":true,"tests":true,"planCoverage":true,"security":true},"issues":[]}\n```';
+      'Here is my review:\n```json\n{"confidence":92,"verdict":"PASS","dimensions":{"correctness":true,"tests":true,"criteria":true,"security":true},"issues":[]}\n```';
     const r = parseReview(out);
     expect(r?.confidence).toBe(92);
     expect(r?.verdict).toBe('PASS');
     expect(r?.dimensions.tests).toBe(true);
     expect(r?.verifyOk).toBeNull();
   });
 
-  it('accepts stringy "pass"/"fail" dimension values and snake_case keys', () => {
+  it('accepts stringy "pass"/"fail" dimension values', () => {
     const r = parseReview(
-      '{"confidence":50,"dimensions":{"correctness":"pass","tests":"fail","plan_coverage":"yes","security":true},"issues":[]}',
+      '{"confidence":50,"dimensions":{"correctness":"pass","tests":"fail","criteria":"yes","security":true},"issues":[]}',
     );
     expect(r?.dimensions.tests).toBe(false);
-    expect(r?.dimensions.planCoverage).toBe(true);
+    expect(r?.dimensions.criteria).toBe(true);
   });
 
   it('derives FAIL when a dimension fails even with no explicit verdict', () => {
     const r = parseReview(
-      '{"confidence":80,"dimensions":{"correctness":false,"tests":true,"planCoverage":true,"security":true},"issues":[]}',
+      '{"confidence":80,"dimensions":{"correctness":false,"tests":true,"criteria":true,"security":true},"issues":[]}',
     );
     expect(r?.verdict).toBe('FAIL');
   });
@@ -68,7 +68,7 @@ describe('parseReview', () => {
   it('returns null for an off-schema verdict (must re-run, not salvage)', () => {
     expect(
       parseReview(
-        '```json\n{"verdict":false,"dimensions":{"correctness":false,"tests":true,"planCoverage":false,"security":true}}\n```',
+        '```json\n{"verdict":false,"dimensions":{"correctness":false,"tests":true,"criteria":false,"security":true}}\n```',
       ),
     ).toBeNull();
   });
@@ -105,9 +105,9 @@ describe('meetsThreshold', () => {
 
 describe('failedDimensions', () => {
   it('lists the human labels of failing dimensions', () => {
-    expect(failedDimensions({ ...PASS_DIMS, tests: false, planCoverage: false })).toEqual([
+    expect(failedDimensions({ ...PASS_DIMS, tests: false, criteria: false })).toEqual([
       'tests',
-      'plan coverage',
+      'acceptance criteria',
     ]);
   });
 });

api/src/review/review.utility.ts

@@ -21,19 +21,22 @@ const boolish = z.preprocess((v) => {
   if (typeof v === 'boolean') {
     return v;
   }
+
   if (typeof v === 'string') {
     const s = v.trim().toLowerCase();
+
     if (['pass', 'passed', 'yes', 'true', 'ok', 'y', '1'].includes(s)) {
       return true;
     }
+
     if (['fail', 'failed', 'no', 'false', 'n', '0'].includes(s)) {
       return false;
     }
   }
+
   return undefined;
 }, z.boolean().default(true));
 
-// Tolerate snake_case / loose key names for the multi-word dimension.
 const dimensionsSchema = z.preprocess(
   (v) => {
     if (!v || typeof v !== 'object') {
@@ -43,14 +46,14 @@ const dimensionsSchema = z.preprocess(
     return {
       correctness: o.correctness,
       tests: o.tests,
-      planCoverage: o.planCoverage ?? o.plan_coverage ?? o.coverage,
+      criteria: o.criteria,
       security: o.security,
     };
   },
   z.object({
     correctness: boolish,
     tests: boolish,
-    planCoverage: boolish,
+    criteria: boolish,
     security: boolish,
   }),
 );
@@ -69,7 +72,7 @@ const reviewSchema = z.object({
 const ALL_DIMENSIONS_PASS: ReviewDimensions = {
   correctness: true,
   tests: true,
-  planCoverage: true,
+  criteria: true,
   security: true,
 };
 
@@ -169,7 +172,7 @@ export function meetsThreshold(result: ReviewResult): boolean {
 const DIMENSION_LABELS: Record<ReviewDimension, string> = {
   correctness: 'correctness',
   tests: 'tests',
-  planCoverage: 'plan coverage',
+  criteria: 'acceptance criteria',
   security: 'security',
 };