feat: ship with open judgment criteria instead of looping

packages/cli/src/task-criteria-gate.test.ts

@@ -16,6 +16,7 @@ import {
   buildCriteriaChapter,
   combineCriteriaOutcomes,
   CRITERIA_REASON_IDS_MAX,
+  criteriaBlockKind,
   criteriaUnmetDetail,
   mergeCriterionVerdicts,
   partitionCriteriaByProof,
@@ -104,6 +105,17 @@ describe('buildCriteriaChapter', () => {
     // …and it names the anchor the evidence must open with.
     expect(chapter).toContain('"path:line"')
   })
+
+  test('D26: requires a question on every unclear, and forbids it feeding the top-level verdict', () => {
+    const chapter = buildCriteriaChapter(TASK_CRITERIA)
+    expect(chapter).toContain('"question"')
+    expect(chapter).toMatch(/REQUIRED/)
+    expect(chapter).toContain('never feeds "verdict"')
+    // The escape hatch the incident exploited: a hesitant "unclear" on a
+    // judgment call must be named as never a valid default.
+    expect(chapter).toMatch(/DECIDE/)
+    expect(chapter).toContain('never a way to avoid choosing')
+  })
 })
 
 // --- exactly one status per criterion --------------------------------------
@@ -980,3 +992,45 @@ describe('combineCriteriaOutcomes', () => {
     expect(outcome.satisfied).toBe(false)
   })
 })
+
+// --- criteriaBlockKind (D26) -------------------------------------------------
+
+describe('criteriaBlockKind', () => {
+  test('no criteria at all reads as satisfied — the caller owns criteria_missing', () => {
+    expect(criteriaBlockKind([], undefined)).toBe('satisfied')
+  })
+
+  test('every criterion met is satisfied', () => {
+    expect(
+      criteriaBlockKind(TASK_CRITERIA, [
+        { criterion_id: C1.id, status: 'met' },
+        { criterion_id: C2.id, status: 'met' },
+        { criterion_id: C3.id, status: 'met' },
+      ]),
+    ).toBe('satisfied')
+  })
+
+  test('one unmet is unmet, whatever else is unclear', () => {
+    expect(
+      criteriaBlockKind(TASK_CRITERIA, [
+        { criterion_id: C1.id, status: 'unmet' },
+        { criterion_id: C2.id, status: 'unclear' },
+        { criterion_id: C3.id, status: 'met' },
+      ]),
+    ).toBe('unmet')
+  })
+
+  test('a criterion the archive never judged is unmet, not judgment_open — silence is never a doubt', () => {
+    expect(criteriaBlockKind(TASK_CRITERIA, [{ criterion_id: C1.id, status: 'met' }])).toBe('unmet')
+  })
+
+  test('nothing unmet, at least one unclear: judgment_open', () => {
+    expect(
+      criteriaBlockKind(TASK_CRITERIA, [
+        { criterion_id: C1.id, status: 'met' },
+        { criterion_id: C2.id, status: 'met' },
+        { criterion_id: C3.id, status: 'unclear' },
+      ]),
+    ).toBe('judgment_open')
+  })
+})

packages/cli/src/task-criteria-gate.ts

@@ -62,6 +62,16 @@ export const CRITERIA_REASON_IDS_MAX = 6
  * trust: `resolveCriteria` below cannot see those fields even if they are
  * emitted. Saying it in the prompt only stops the model from spending its
  * output on something that goes nowhere.
+ *
+ * D26 hardened this chapter after an incident: a purely stylistic criterion
+ * ("same style as the existing helpers") sat "unclear" for twelve automatic
+ * fix rounds because the reviewer never settled it AND let that same
+ * hesitation leak into its own top-level `verdict`, which kept the task on
+ * `review_ko` for a reason the criteria gate alone would have waived. Two
+ * rules below are new for exactly that reason: a judgment call must be
+ * DECIDED whenever the diff gives enough to decide (comparing against the
+ * codebase's own existing patterns counts), "unclear" is reserved for a real
+ * information gap, and one is never used to excuse the other.
  */
 export function buildCriteriaChapter(criteria: readonly AcceptanceCriterion[]): string {
   return [
@@ -70,12 +80,15 @@ export function buildCriteriaChapter(criteria: readonly AcceptanceCriterion[]):
     ...criteria.map((criterion) => `- [${criterion.id}] ${criterion.text}`),
     '',
     'Add ONE more top-level field to the JSON you output, after "files_reviewed":',
-    '"criteria": [{ "criterion_id": "one of the ids above, verbatim", "status": "met" | "unmet" | "unclear", "evidence": "<path>:<line> — short quote of that line" }]',
+    '"criteria": [{ "criterion_id": "one of the ids above, verbatim", "status": "met" | "unmet" | "unclear", "evidence": "<path>:<line> — short quote of that line", "question": "the one precise thing you could not tell from the diff" }]',
     '',
     'Rules for this chapter:',
     '- Exactly one entry per criterion listed above. Never invent an id and never merge two criteria into one entry: an id absent from the list above is discarded, and a criterion you leave out is judged "unclear" anyway.',
     '- "evidence" MUST START with a path from the diff and a new-file line number visible in one of that file\'s @@ hunks, written exactly as "path:line", and only then your quote. An evidence that does not start with such an anchor is removed and the criterion falls back to "unclear".',
     '- "met" means the diff ITSELF shows the criterion satisfied, at that anchor. A criterion you believe is satisfied but cannot anchor in the diff is "unclear", never "met". A commit message is never evidence.',
+    '- DECIDE. A criterion with no judgment call left to make — including a subjective or stylistic one ("same style as the existing helpers", "readable", "consistent naming") — gets "met" or "unmet", not "unclear": compare the diff against the equivalent code already in the repository and rule on what you see. "unclear" is for a genuine gap in what the diff can show — a runtime behavior, a business decision, information that lives outside this diff entirely — never a way to avoid choosing.',
+    '- When, and only when, "status" is "unclear", "question" is REQUIRED: the ONE precise thing you could not settle, addressed to the human who will read it on the merge request — not a restatement of the criterion, not "unsure if this is correct". A criterion you can decide never carries a "question".',
+    '- This chapter never feeds "verdict". A criterion you mark "unclear" is a fact about THAT CRITERION alone: it must never make you write "request_changes" or "comment" for the review as a whole — "verdict" is your judgment of the CODE, findings included, and nothing here.',
     '- Do NOT output a completion percentage, a score, a ratio or an overall criteria verdict. They are not read: the gate is computed from the per-criterion statuses alone.',
   ].join('\n')
 }
@@ -669,3 +682,53 @@ export function mergeCriterionVerdicts(
   }
   return [...merged.values()]
 }
+
+// --- D26: what an archived criteria gate is blocked on -----------------------
+//
+// A criterion the gate settled `unclear` is not a failure (D18 already ships
+// it, waived) — it is a decision nobody but a human can finish making.
+// `criteriaBlockKind` is the SHARED reading of that fact, for the two callers
+// that must treat "genuinely unmet" and "an open judgment call" differently:
+// the automatic fix loop's tighter round cap (task-fix-loop.ts, wired from
+// task-server.ts) and the merge gate's own judgment condition (task-merge.ts).
+// It reads the ARCHIVED verdicts alone, never a live `CriteriaOutcome`,
+// because both callers run after the review that produced them — the merge
+// gate possibly at a later boot. The merge request's own "To decide" section
+// is built separately, in `renderRecapMarkdown` (task-recap.ts), off the
+// SAME per-criterion `text`+`question` the recap already denormalizes.
+
+/**
+ * What an archived criteria gate is blocked on, from the verdicts alone —
+ * `'satisfied'` when the task carries no criterion at all, so a caller with
+ * its own "no criteria" handling (task-merge.ts's `criteria_missing`) never
+ * has to special-case an empty list here too.
+ *
+ *  - `'unmet'` — at least one criterion is `'unmet'`, OR the archive never
+ *    judged it at all (silence is never a pass, same reading `resolveCriteria`
+ *    already applies). Real work is what clears this, and it always was.
+ *  - `'judgment_open'` — nothing is unmet or unjudged, but at least one
+ *    criterion is a settled `'unclear'`: a human's call, not an agent's.
+ *  - `'satisfied'` — every criterion is `'met'`.
+ */
+export type CriteriaBlockKind = 'satisfied' | 'unmet' | 'judgment_open'
+
+export function criteriaBlockKind(
+  criteria: readonly AcceptanceCriterion[],
+  verdicts: readonly CriterionVerdict[] | undefined,
+): CriteriaBlockKind {
+  if (criteria.length === 0) {
+    return 'satisfied'
+  }
+  const byId = new Map((verdicts ?? []).map((verdict) => [verdict.criterion_id, verdict.status]))
+  let sawOpen = false
+  for (const criterion of criteria) {
+    const status = byId.get(criterion.id)
+    if (status === undefined || status === 'unmet') {
+      return 'unmet'
+    }
+    if (status === 'unclear') {
+      sawOpen = true
+    }
+  }
+  return sawOpen ? 'judgment_open' : 'satisfied'
+}

packages/cli/src/task-fix-loop.test.ts

@@ -6,6 +6,7 @@ import {
   AUTO_FIX_ROUND_NAME,
   autoFixRoundsUsed,
   decideFixLoop,
+  JUDGMENT_ONLY_MAX_ROUNDS,
   type FixLoopInput,
 } from './task-fix-loop.js'
 import { taskReason } from './tasks-store.js'
@@ -291,6 +292,61 @@ describe('decideFixLoop', () => {
     )
     expect(decision).toMatchObject({ kind: 'stand', code: 'criteria_unmet' })
   })
+
+  // --- D26: the judgment-only ceiling ---------------------------------------
+
+  const judgmentInput = (over: Partial<FixLoopInput> = {}): FixLoopInput =>
+    input({
+      reason: taskReason('criteria_unmet', '1 of 3 not satisfied'),
+      judgmentOnly: true,
+      ...over,
+    })
+
+  test('a judgment-only block never gets more than JUDGMENT_ONLY_MAX_ROUNDS, whatever max allows', () => {
+    for (const configuredMax of [JUDGMENT_ONLY_MAX_ROUNDS, JUDGMENT_ONLY_MAX_ROUNDS + 5, 100]) {
+      expect(decideFixLoop(judgmentInput({ roundsUsed: 0, max: configuredMax })).kind).toBe('retry')
+      expect(
+        decideFixLoop(judgmentInput({ roundsUsed: JUDGMENT_ONLY_MAX_ROUNDS, max: configuredMax }))
+          .kind,
+      ).toBe('ship')
+    }
+  })
+
+  test('reaching the ceiling SHIPS, it does not hand the task to a human', () => {
+    const decision = decideFixLoop(judgmentInput({ roundsUsed: JUDGMENT_ONLY_MAX_ROUNDS }))
+    if (decision.kind !== 'ship') {
+      throw new Error(`expected a ship, got ${decision.kind}`)
+    }
+    expect(decision.text).toContain('open judgment calls')
+  })
+
+  test('a configured max BELOW the ceiling is respected, never rounded up', () => {
+    expect(decideFixLoop(judgmentInput({ roundsUsed: 0, max: 1 })).kind).toBe('retry')
+    expect(decideFixLoop(judgmentInput({ roundsUsed: 1, max: 1 })).kind).toBe('ship')
+  })
+
+  test('judgmentOnly is only read for a criteria_unmet exit — a review_blocked never ships early', () => {
+    const decision = decideFixLoop(
+      input({
+        roundsUsed: JUDGMENT_ONLY_MAX_ROUNDS,
+        max: 10,
+        judgmentOnly: true,
+        reason: taskReason('review_blocked', 'one major finding'),
+      }),
+    )
+    expect(decision.kind).toBe('retry')
+  })
+
+  test('judgmentOnly defaults to false: the ordinary budget applies with no flag at all', () => {
+    const decision = decideFixLoop(
+      input({
+        roundsUsed: JUDGMENT_ONLY_MAX_ROUNDS,
+        max: 10,
+        reason: taskReason('criteria_unmet', '1 of 3 not satisfied'),
+      }),
+    )
+    expect(decision.kind).toBe('retry')
+  })
 })
 
 // --- applyFixLoopDecision -------------------------------------------------
@@ -353,6 +409,22 @@ describe('applyFixLoopDecision', () => {
     applyFixLoopDecision(task, decideFixLoop(input({ status: 'review_ok' })))
     expect(JSON.stringify(task)).toBe(before)
   })
+
+  test('a ship decision (D26) lands on review_ok with the reason cleared', () => {
+    const task = record({ reason: taskReason('criteria_unmet', '1 of 3 not satisfied') })
+    applyFixLoopDecision(
+      task,
+      decideFixLoop(
+        input({
+          roundsUsed: JUDGMENT_ONLY_MAX_ROUNDS,
+          reason: taskReason('criteria_unmet', '1 of 3 not satisfied'),
+          judgmentOnly: true,
+        }),
+      ),
+    )
+    expect(task.status).toBe('review_ok')
+    expect(task.reason).toBeUndefined()
+  })
 })
 
 // --- the structural half of "no dedicated field" --------------------------