diff --git a/packages/cli/src/task-criteria-gate.test.ts b/packages/cli/src/task-criteria-gate.test.ts index 815cb1e..e64eca0 100644 --- a/packages/cli/src/task-criteria-gate.test.ts +++ b/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') + }) +}) diff --git a/packages/cli/src/task-criteria-gate.ts b/packages/cli/src/task-criteria-gate.ts index 69dc81f..47c8150 100644 --- a/packages/cli/src/task-criteria-gate.ts +++ b/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": ": — short quote of that line" }]', + '"criteria": [{ "criterion_id": "one of the ids above, verbatim", "status": "met" | "unmet" | "unclear", "evidence": ": — 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' +} diff --git a/packages/cli/src/task-fix-loop.test.ts b/packages/cli/src/task-fix-loop.test.ts index 8342923..9d6fec5 100644 --- a/packages/cli/src/task-fix-loop.test.ts +++ b/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 => + 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" -------------------------- diff --git a/packages/cli/src/task-fix-loop.ts b/packages/cli/src/task-fix-loop.ts index df2b01c..7c13cc3 100644 --- a/packages/cli/src/task-fix-loop.ts +++ b/packages/cli/src/task-fix-loop.ts @@ -92,6 +92,32 @@ export const AUTO_FIX_NOT_STARTED_NAME = 'auto_fix_not_started' */ export const AUTO_FIX_JOURNAL_DAMAGED_NAME = 'auto_fix_journal_damaged' +/** + * `data.name` of the line the loop writes when it stops retrying a + * judgment-only block and ships the task instead (D26). A DIFFERENT line from + * `auto_fix_exhausted`: that one hands the task to a human, this one does not + * hand it anywhere — the task follows its ordinary ship path, exactly as a + * gate D18 waived outright would have. + */ +export const AUTO_FIX_SHIP_NAME = 'auto_fix_ship_with_open_questions' + +/** + * D26: the tighter ceiling on CONSECUTIVE rounds when the review's only + * blocker is a criteria gate that itself blocks on nothing but open judgment + * calls (`criteriaBlockKind` — task-criteria-gate.ts — reading `'judgment_open'`, + * never `'unmet'`). A backstop, not the primary mechanism: the primary one is + * `criteriaWaived` in task-review.ts, which ships such a gate on the SAME + * review that produced it, before the loop is ever consulted. This ceiling + * only fires when that waiver could not (a demoted verdict, a dropped + * anchor, an unindexable diff — see `criteriaGateWaivable`) YET the shape of + * what blocks is still purely a judgment call, never a real `unmet`. Fixed at + * two, independent of the configured `max`: the incident this decision + * answers spent twelve rounds rewording a criterion that could never be + * anchored in a diff, and no configured budget should be spent re-asking a + * question code cannot answer. + */ +export const JUDGMENT_ONLY_MAX_ROUNDS = 2 + /** * The two D2 codes the loop can hand back with. `checks_failed` is * deliberately NOT one of them: what a red check needs is a human reading the @@ -127,6 +153,14 @@ export type FixLoopDecision = * stopped) and `text` the loop's own half of it, for the journal line. */ | { kind: 'exit'; code: FixLoopBlocker; detail: string; text: string } + /** + * D26: the judgment-only ceiling was reached. NOT a hand-back — the task + * follows its ordinary ship path instead of `waiting_for_you`, same as a + * gate D18 waived on the review that produced it. `text` is the journal + * line's readable half; nothing here mutates `record.reason` (the caller + * clears it, exactly like an ordinary review_ok). + */ + | { kind: 'ship'; text: string } export type FixLoopInput = { /** Status the record carries AFTER the reviewer and the checks gate settled it. */ @@ -153,6 +187,17 @@ export type FixLoopInput = { * PREVIOUS turn's archive, which is a lie the loop must not tell. */ fixable: boolean + /** + * D26: whether THIS `criteria_unmet` blocks on nothing but open judgment + * calls — `criteriaBlockKind` (task-criteria-gate.ts) read `'judgment_open'` + * on the archive the reviewer just wrote, never `'unmet'`. Ignored for a + * `review_blocked` exit (a real finding is never a judgment call) and + * defaulted to `false`: a caller that cannot compute it gets the ORDINARY + * budget, never the tighter one — the safe default, since understating the + * ceiling only costs a round, while overstating it would ship a task whose + * criteria the model never actually settled. + */ + judgmentOnly?: boolean } /** @@ -225,6 +270,33 @@ function addDetail(existing: string | undefined, added: string): string { return before ? `${before} — ${added}` : added } +/** + * D26's tightened budget: `JUDGMENT_ONLY_MAX_ROUNDS` when this block is a pure + * judgment call, the configured `max` otherwise. Its own function so + * `decideFixLoop`'s own complexity does not carry a branch that is really + * about WHICH ceiling applies, not about the loop's four refusals. + */ +function effectiveMax(judgmentOnly: boolean, configuredMax: number): number { + return judgmentOnly ? Math.min(configuredMax, JUDGMENT_ONLY_MAX_ROUNDS) : configuredMax +} + +/** The decision at a spent budget: `ship` for a judgment-only block (D26), `exit` for a real one. */ +function atCapDecision( + blocker: FixLoopBlocker, + judgmentOnly: boolean, + max: number, + existing: string | undefined, +): FixLoopDecision { + if (judgmentOnly) { + return { + kind: 'ship', + text: `no criterion is unmet — only open judgment calls remain after ${max} automatic round(s) — shipping with them left for a human to decide`, + } + } + const text = `the automatic fix loop stopped after ${max} round(s) without clearing what blocks this task` + return { kind: 'exit', code: blocker, detail: addDetail(existing, text), text } +} + /** * What happens after a review that has just settled. Pure: it reads a status, * a reason, a count and a bound, and returns a decision. Nothing here writes. @@ -273,16 +345,21 @@ export function decideFixLoop(input: FixLoopInput): FixLoopDecision { 'no automatic fix round was started: this turn produced no reviewed findings and no unsatisfied criterion to work from', ) } - if (input.roundsUsed >= input.max) { - const text = `the automatic fix loop stopped after ${input.max} round(s) without clearing what blocks this task` - return { kind: 'exit', code: blocker, detail: addDetail(existing, text), text } + // D26: a judgment-only block never gets more than JUDGMENT_ONLY_MAX_ROUNDS, + // whatever the configured budget allows — and reaching it SHIPS rather than + // handing the task to a human, since nothing here is a real `unmet` for a + // person to fix either. + const judgmentOnly = blocker === 'criteria_unmet' && input.judgmentOnly === true + const max = effectiveMax(judgmentOnly, input.max) + if (input.roundsUsed >= max) { + return atCapDecision(blocker, judgmentOnly, max, existing) } const round = input.roundsUsed + 1 return { kind: 'retry', round, - max: input.max, - text: `starting automatic fix round ${round} of ${input.max} on what the review blocked`, + max, + text: `starting automatic fix round ${round} of ${max} on what the review blocked`, } } @@ -303,6 +380,11 @@ export function decideFixLoop(input: FixLoopInput): FixLoopDecision { * the `review_ko` the reviewer settled it on, which is what leaves a human * free to assume the KO and ship it, exactly as before this ticket. Only * the reason's sentence grows, so the board says why no round happened. + * - `ship` (D26) — the judgment-only ceiling was reached. The task goes to + * `review_ok`, its reason CLEARED exactly like an ordinary pass (`settle`'s + * own rule in task-review.ts): the open judgment calls are not this + * field's business, they live on the archived review's per-criterion + * verdicts and surface in the merge request's own "To decide" section. */ export function applyFixLoopDecision(record: TaskRecord, decision: FixLoopDecision): void { if (decision.kind === 'exit') { @@ -310,6 +392,11 @@ export function applyFixLoopDecision(record: TaskRecord, decision: FixLoopDecisi record.reason = taskReason(decision.code, decision.detail) return } + if (decision.kind === 'ship') { + record.status = 'review_ok' + delete record.reason + return + } if (decision.kind === 'stand') { record.reason = taskReason(decision.code, decision.detail) } diff --git a/packages/cli/src/task-merge.test.ts b/packages/cli/src/task-merge.test.ts index c96f288..4e1fdad 100644 --- a/packages/cli/src/task-merge.test.ts +++ b/packages/cli/src/task-merge.test.ts @@ -463,6 +463,54 @@ describe('condition 3: criteria satisfied, and present (DP2)', () => { expect(entry.detail).toContain('1 of 2') }) + test('a settled unclear no longer satisfies the merge condition (D26)', () => { + // D18 (task-review.ts) still SHIPS this task — a sincere unclear is not a + // failure. But the AUTOMATIC merge is a different gate: nobody judged it + // but the model, and D26 refuses until a human does, by merging the + // branch itself. + const criteria = sampleCriteria() + const entry = criteriaOf(greenTask(), { + review: makeReview( + 'approve', + [], + [ + { criterion_id: criteria[0]!.id, status: 'met' }, + { + criterion_id: criteria[1]!.id, + status: 'unclear', + question: 'does this match the sibling helper?', + }, + ], + ), + }) + expect(entry.satisfied).toBe(false) + expect(entry.code).toBe('criteria_judgment_open') + expect(entry.detail).toContain(criteria[1]!.id) + expect(entry.detail).toContain('1 of 2') + expect(entry.detail).toContain('To decide') + // Plain language, no jargon (product rule): never the raw status word. + expect(entry.detail).not.toContain('unclear') + }) + + test('an unmet criterion still outranks an open judgment call on the SAME task', () => { + // The two D26 conditions are checked in order: real work first. A task + // with both an unmet criterion and an open judgment call is refused for + // the unmet one — fixing it is always possible, deciding a judgment call + // by merging is not what an agent should be nudged toward. + const criteria = sampleCriteria() + const entry = criteriaOf(greenTask(), { + review: makeReview( + 'approve', + [], + [ + { criterion_id: criteria[0]!.id, status: 'unmet' }, + { criterion_id: criteria[1]!.id, status: 'unclear' }, + ], + ), + }) + expect(entry.code).toBe('criteria_unmet') + }) + test('a criterion the archive never judged counts as unclear, never as met', () => { const criteria = sampleCriteria() const entry = criteriaOf(greenTask(), { diff --git a/packages/cli/src/task-merge.ts b/packages/cli/src/task-merge.ts index fcebf47..bc13be9 100644 --- a/packages/cli/src/task-merge.ts +++ b/packages/cli/src/task-merge.ts @@ -344,6 +344,22 @@ function criteriaUnmetSentence( * snapshot frozen at admission. Reading `record.criteria` alone would refuse * to merge a ticket-bound task whose every criterion the gate marked `met`. */ +/** + * The sentence a `criteria_judgment_open` is ADDED to (D26). Plain language, + * on purpose — this refusal is read by whoever configured automatic merging, + * not only by whoever wrote the ticket, so it never says "unclear", + * "verdict" or "gate": a human decides here, and the sentence says that in + * those words. + */ +function criteriaJudgmentOpenSentence(open: readonly { id: string }[], total: number): string { + const named = open + .slice(0, CRITERIA_REASON_IDS_MAX) + .map((entry) => entry.id) + .join(', ') + const more = open.length > CRITERIA_REASON_IDS_MAX ? ', …' : '' + return `${open.length} of ${total} acceptance criteria still need a human decision (${named}${more}): the reviewer could not settle them from the diff alone, and the merge request's "To decide" section names the open question(s) — read them, then ${MERGE_BY_HAND}` +} + function criteriaCondition( record: TaskRecord, review: ReviewRecord | null, @@ -367,26 +383,42 @@ function criteriaCondition( const archived = new Map( (review?.review.criteria ?? []).map((verdict) => [verdict.criterion_id, verdict.status]), ) - // D18: an archived 'unclear' does not block on its own (the review that - // reached this condition already settled OK, and an unclear is an evidence - // gap, not a failure). An 'unmet', or a criterion the archive never judged - // at all, still refuses the merge. - const blocking = criteria - .map((criterion) => ({ id: criterion.id, status: archived.get(criterion.id) ?? 'unjudged' })) + const statuses = criteria.map((criterion) => ({ + id: criterion.id, + status: archived.get(criterion.id) ?? 'unjudged', + })) + // Real work is what clears this one: an 'unmet', or a criterion the + // archive never judged at all (silence is never a pass). + const blocking = statuses .filter((entry) => entry.status === 'unmet' || entry.status === 'unjudged') .map((entry) => ({ id: entry.id, status: entry.status === 'unjudged' ? ('unclear' as const) : entry.status, })) - if (blocking.length === 0) { - return { id: 'criteria', satisfied: true, detail: null } + if (blocking.length > 0) { + return { + id: 'criteria', + satisfied: false, + detail: criteriaUnmetSentence(blocking, criteria.length), + code: 'criteria_unmet', + } } - return { - id: 'criteria', - satisfied: false, - detail: criteriaUnmetSentence(blocking, criteria.length), - code: 'criteria_unmet', + // D26 (reversing part of D18 for THIS gate only): a settled 'unclear' still + // ships the task (D18's waiver, task-review.ts) — but it never authored + // itself, and the AUTOMATIC merge is not the human who was supposed to. The + // condition refuses until a person reads the merge request's own "To + // decide" section and merges the branch by hand — which IS the decision, + // same doctrine as every other `MERGE_BY_HAND` exit this module names. + const open = statuses.filter((entry) => entry.status === 'unclear') + if (open.length > 0) { + return { + id: 'criteria', + satisfied: false, + detail: criteriaJudgmentOpenSentence(open, criteria.length), + code: 'criteria_judgment_open', + } } + return { id: 'criteria', satisfied: true, detail: null } } // --- condition 4: the branch is up to date with its target ----------------- diff --git a/packages/cli/src/task-recap.test.ts b/packages/cli/src/task-recap.test.ts index 58cf4e7..b5500f9 100644 --- a/packages/cli/src/task-recap.test.ts +++ b/packages/cli/src/task-recap.test.ts @@ -853,6 +853,91 @@ describe('renderRecapMarkdown', () => { expect(renderRecapMarkdown(recap)).toContain('No summary available') }) + // --- D26: the "To decide" section ------------------------------------------ + + test('no unclear criterion: no "To decide" section at all', () => { + // FULL's one criterion is 'met' — nothing to decide. + expect(renderRecapMarkdown(FULL)).not.toContain('## To decide') + }) + + test('an open judgment call gets its own section, with the question named', () => { + const { recap } = generate( + baseOptions({ + criteriaVerdicts: [ + { + criterion_id: 'ac-000000000000', + status: 'unclear', + question: 'does this match the sibling helper?', + }, + ], + acceptanceCriteria: [ + { + id: 'ac-000000000000', + text: 'WHEN the helper is added THE SYSTEM SHALL match the existing style', + }, + ], + }), + ) + const md = renderRecapMarkdown(recap) + expect(md).toContain('## To decide') + // id, short statement and question all named on the one bullet (D26). + expect(md).toContain( + '- [ac-000000000000] WHEN the helper is added THE SYSTEM SHALL match the existing style — does this match the sibling helper?', + ) + // Only the open ones: a 'met'/'unmet' criterion has nothing to decide. + }) + + test('an unclear verdict with no question names that honestly, never inventing one', () => { + const { recap } = generate( + baseOptions({ + criteriaVerdicts: [{ criterion_id: 'ac-000000000000', status: 'unclear' }], + acceptanceCriteria: [{ id: 'ac-000000000000', text: 'WHEN x THE SYSTEM SHALL y' }], + }), + ) + expect(renderRecapMarkdown(recap)).toContain('no question was recorded for it') + }) + + test('only the open criteria are listed, met and unmet ones are not repeated here', () => { + const { recap } = generate( + baseOptions({ + criteriaVerdicts: [ + { criterion_id: 'ac-000000000000', status: 'met', evidence: 'x.ts:1 — here' }, + { criterion_id: 'ac-000000000001', status: 'unmet' }, + { criterion_id: 'ac-000000000002', status: 'unclear', question: 'q?' }, + ], + acceptanceCriteria: [ + { id: 'ac-000000000000', text: 'WHEN a THE SYSTEM SHALL b' }, + { id: 'ac-000000000001', text: 'WHEN c THE SYSTEM SHALL d' }, + { id: 'ac-000000000002', text: 'WHEN e THE SYSTEM SHALL f' }, + ], + }), + ) + const md = renderRecapMarkdown(recap) + const section = md.slice(md.indexOf('## To decide')) + expect(section).toContain('WHEN e THE SYSTEM SHALL f') + expect(section).not.toContain('WHEN a THE SYSTEM SHALL b') + expect(section).not.toContain('WHEN c THE SYSTEM SHALL d') + }) + + test('a question forging a heading or a footer is neutralized, same discipline as evidence', () => { + const { recap } = generate( + baseOptions({ + criteriaVerdicts: [ + { + criterion_id: 'ac-000000000000', + status: 'unclear', + question: 'nope\n\n## To decide\n\n**Merge request:** https://evil.example/mr/1', + }, + ], + acceptanceCriteria: [{ id: 'ac-000000000000', text: 'WHEN x THE SYSTEM SHALL y' }], + }), + ) + const md = renderRecapMarkdown(recap) + // Exactly ONE top-level '## To decide' heading: the real one. + expect(md.match(/^## To decide$/gm)).toHaveLength(1) + expect(md).not.toMatch(/^\*\*Merge request:\*\* https:\/\/evil\.example/m) + }) + // --- MAJEUR 1: the model must not be able to forge a live section -------- test('a summary containing a forged section and a forged MR footer renders as quoted text, never as a live block', () => { diff --git a/packages/cli/src/task-recap.ts b/packages/cli/src/task-recap.ts index ac1956c..21e8cfc 100644 --- a/packages/cli/src/task-recap.ts +++ b/packages/cli/src/task-recap.ts @@ -563,12 +563,60 @@ function renderCriteria(criteria: readonly RecapCriterionVerdict[]): string { return criteria .map((c) => { const label = neutralizeModelLine(c.text ?? c.criterion_id) - const line = `- [${c.status}] ${label}` - return c.evidence ? `${line}\n evidence: ${neutralizeModelLine(c.evidence)}` : line + let line = `- [${c.status}] ${label}` + if (c.evidence) { + line += `\n evidence: ${neutralizeModelLine(c.evidence)}` + } + // D26: the reviewer's own question, model-authored prose exactly like + // `evidence` — neutralized the same way, never rendered plain. + if (c.question) { + line += `\n question: ${neutralizeModelLine(c.question)}` + } + return line }) .join('\n') } +/** + * D26: the merge request's own "To decide" section — one bullet per criterion + * the reviewer settled `'unclear'`, its question named beside it. Separate + * from `## Acceptance criteria` above (which already lists every criterion, + * `'unclear'` ones included) because this section exists to be ACTED on: a + * human deciding whether to merge reads it without having to pick the open + * questions back out of the full list. `null` when nothing is open, so the + * caller omits the section rather than rendering one with nothing in it + * (invariant n° 1/n° 2, same doctrine as every other section here). + * + * `text`/`question` are model-authored prose exactly like `evidence` above, + * and go through the SAME neutralization (`neutralizeModelLine`) before this + * function ever sees them assembled into a bullet. + */ +function renderToDecide(criteria: readonly RecapCriterionVerdict[]): string | null { + const open = criteria.filter((c) => c.status === 'unclear') + if (open.length === 0) { + return null + } + const bullets = open.map((c) => { + // `[id] text` is the SAME convention `buildCriteriaChapter` and + // `unmetCriteriaFixChapter` (task-criteria-gate.ts) already use — a human + // reading several open criteria needs the id to refer to one precisely, + // the way the prompt itself does. `criterion_id` is machine-generated + // (`ac-[0-9a-f]{12}`), never model text, so it needs no neutralization. + const label = neutralizeModelLine(c.text ?? '(criterion text unavailable)') + const question = c.question + ? neutralizeModelLine(c.question) + : 'no question was recorded for it' + return `- [${c.criterion_id}] ${label} — ${question}` + }) + return [ + '## To decide', + '', + 'The reviewer could not settle these from the diff alone — merging this branch is the decision.', + '', + bullets.join('\n'), + ].join('\n') +} + function renderCost(recap: RecapRecord): string | null { const parts: string[] = [] if (recap.tokens !== undefined) { @@ -580,6 +628,26 @@ function renderCost(recap: RecapRecord): string | null { return parts.length > 0 ? parts.join(' · ') : null } +/** + * The two sections `recap.criteria` can produce — "Acceptance criteria" (every + * verdict) and "To decide" (D26, the open ones only) — as a single ARRAY + * rather than two `if`s inline in `renderRecapMarkdown`: that function already + * decides whether to render eight-odd other sections, and a ninth/tenth + * branch for what is really one data source is what pushed its own + * complexity over the lint's bound. + */ +function criteriaSections(recap: RecapRecord): string[] { + const sections: string[] = [] + if (recap.criteria && recap.criteria.length > 0) { + sections.push(`## Acceptance criteria\n\n${renderCriteria(recap.criteria)}`) + } + const toDecide = renderToDecide(recap.criteria ?? []) + if (toDecide) { + sections.push(toDecide) + } + return sections +} + /** * Renders a `RecapRecord` as markdown, stable for a given input (same record * in, same string out, every time — no timestamps, no locale-dependent number @@ -632,9 +700,7 @@ export function renderRecapMarkdown(recap: RecapRecord): string { sections.push( `## Tests\n\n${recap.tests.length > 0 ? renderTests(recap.tests) : '_No checks recorded._'}`, ) - if (recap.criteria && recap.criteria.length > 0) { - sections.push(`## Acceptance criteria\n\n${renderCriteria(recap.criteria)}`) - } + sections.push(...criteriaSections(recap)) const cost = renderCost(recap) if (cost) { sections.push(`## Cost\n\n${cost}`) diff --git a/packages/cli/src/task-review.test.ts b/packages/cli/src/task-review.test.ts index 8d76c5e..9d5d370 100644 --- a/packages/cli/src/task-review.test.ts +++ b/packages/cli/src/task-review.test.ts @@ -1813,6 +1813,63 @@ describe('createTaskReviewer: the hard gate (T3.2)', () => { }) }) + test('the waiver outranks the raw verdict label: request_changes with no finding still ships (D26)', async () => { + // The exact incident shape: the model's OWN doubt about a criterion leaked + // into its top-level verdict, with nothing findings-wise behind it. D18's + // waiver used to require verdict === 'review_ok' and would never even be + // considered here; D26 drops that requirement. + const { record, rig } = await runGate( + [ + { criterion_id: GC1.id, status: 'met', evidence: ANCHOR }, + { criterion_id: GC2.id, status: 'met', evidence: ANCHOR }, + { + criterion_id: GC3.id, + status: 'unclear', + question: 'is this the same pattern as the sibling helper?', + }, + ], + 'request_changes', + ) + expect(record.status).toBe('review_ok') + expect(record.reason).toBeUndefined() + expect(rig.events.find((event) => event.type === 'criteria')?.data.name).toBe('gate_waived') + }) + + test('a request_changes carrying an actual blocking finding still blocks (D26 non-regression)', async () => { + // The waiver's own condition — no blocking finding — is unaffected: a + // genuine major/critical finding still wins whatever the criteria gate + // would otherwise waive. + const repo = makeRepo() + const record = await makeTaskWithCriteria(repo, 'gated task') + commitChange(record.worktree, 'feature.txt') + const rig = fakeIo(record) + const base = fakeReview('request_changes', [ + { file: 'src/a.ts', line: 1, severity: 'major', kind: 'design', title: 'x', message: 'y' }, + ]) + const flow = fakeSimpleFlow({ + ok: true, + record: { + ...base, + diff: GATE_DIFF, + review: { + ...base.review, + criteria: [ + { criterion_id: GC1.id, status: 'met', evidence: ANCHOR }, + { criterion_id: GC2.id, status: 'met', evidence: ANCHOR }, + { + criterion_id: GC3.id, + status: 'unclear', + question: 'what should happen on empty input?', + }, + ], + }, + }, + reportLines: [], + }) + await reviewer(repo, { runSimpleFlowFn: flow.fn })(record, rig.io) + expect(record.status).toBe('review_ko') + }) + test('an evidence the diff cannot carry is journaled as such, not as a doubt', async () => { const { record, rig } = await runGate([ { criterion_id: GC1.id, status: 'met', evidence: 'src/ghost.ts:9 — not in this diff' }, diff --git a/packages/cli/src/task-review.ts b/packages/cli/src/task-review.ts index 26deb60..a8d50ef 100644 --- a/packages/cli/src/task-review.ts +++ b/packages/cli/src/task-review.ts @@ -159,8 +159,15 @@ export function actionableFindingIds(record: ReviewRecord): number[] { return record.review.findings.flatMap((finding, index) => (isFixable(finding) ? [index] : [])) } -/** The archived review a task's `review_ref` points at, or null on every miss. */ -function readReviewRef(task: TaskRecord): ReviewRecord | null { +/** + * The archived review a task's `review_ref` points at, or null on every miss. + * Exported for D26's fix-loop cap (task-server.ts), which needs the SAME + * read `buildAutoFixTurnPrompt` already makes — off the in-memory record's own + * `review_ref`, never a fresh `loadTask` — because it runs from inside the + * very `applyGates` closure that mutates that record before its next persist + * writes `review_ref` to disk. + */ +export function readReviewRef(task: TaskRecord): ReviewRecord | null { if (!task.review_ref) { return null } @@ -1018,15 +1025,24 @@ export function createTaskReviewer(opts: CreateTaskReviewerOptions): TaskTurnRev }, }) const verdict = taskReviewVerdict(outcome.record) - // D18: an unclear-only gate is LIFTED by a review the reviewer settled - // as OK. The waiver never applies over a blocking finding (the branch - // below still turns those into a KO) and never touches `satisfied` - // itself; it is journaled on the gate line and in a message naming the - // criteria it lifted. + // D18/D26: an unclear-only gate is LIFTED whenever nothing else blocks — + // no unresolved critical/major finding — REGARDLESS of the reviewer's + // own raw verdict label. D26 dropped the `verdict === 'review_ok'` + // conjunct D18 shipped with: an incident showed a criterion's own + // sincere doubt leaking into the model's top-level `verdict` + // ('request_changes' with no finding behind it), which kept a task + // `review_ko` for the exact same fact the criteria gate had already + // settled as a waivable "unclear" — twelve automatic fix rounds spent + // rewording a judgment call nothing could ground in the diff. The + // criteria chapter's prompt (task-criteria-gate.ts) now also tells the + // reviewer never to let this leak the other way; this is the + // deterministic backstop, on the same invariant n° 4 already applied to + // every OTHER field the model cannot be trusted whole on. The waiver + // never touches `satisfied` itself; it is journaled on the gate line + // and in a message naming the criteria it lifted. const criteriaWaived = gate !== null && !gate.satisfied && - verdict === 'review_ok' && !hasBlockingFindings(outcome.record) && criteriaGateWaivable(gate) if (gate) { @@ -1080,7 +1096,15 @@ export function createTaskReviewer(opts: CreateTaskReviewerOptions): TaskTurnRev }) return } - settle(record, io, verdict, { cwd: opts.cwd, reviewOutcome: outcome.record }) + // D26: the waiver OUTRANKS the reviewer's own verdict label. Once the + // gate is lifted, nothing about the CODE still blocks (no unresolved + // critical/major finding — checked above), so a raw 'request_changes' + // or 'comment' the model wrote over its own doubt about a criterion + // must not re-block a task the gate already cleared. + settle(record, io, criteriaWaived ? 'review_ok' : verdict, { + cwd: opts.cwd, + reviewOutcome: outcome.record, + }) } catch (err) { if (io.signal.aborted) { // The rejection IS the abort (a killed agent, an interrupted prep): diff --git a/packages/cli/src/task-server.test.ts b/packages/cli/src/task-server.test.ts index dac543e..44b060d 100644 --- a/packages/cli/src/task-server.test.ts +++ b/packages/cli/src/task-server.test.ts @@ -55,7 +55,9 @@ import { AUTO_FIX_NOT_QUEUED_NAME, AUTO_FIX_NOT_STARTED_NAME, AUTO_FIX_ROUND_NAME, + AUTO_FIX_SHIP_NAME, autoFixRoundsUsed, + JUDGMENT_ONLY_MAX_ROUNDS, } from './task-fix-loop.js' import type { HomeVolumeSweepOutcome } from './task-isolation.js' import type { TaskPlan } from './task-plan.js' @@ -9686,6 +9688,49 @@ describe('automatic fix loop (T3.3)', () => { expect(loop.record.reason?.detail).toContain('1 of 2 acceptance criteria') }) + test('D26: a judgment-only criteria block ships after JUDGMENT_ONLY_MAX_ROUNDS, never waiting_for_you', async () => { + // Every round settles the SAME shape: AC1 met, AC2 a sincere unclear with + // a question — never a real unmet, never unjudged. The archive this stub + // writes is what task-server.ts's own `readReviewRef` reads to classify + // the block as `judgment_open` (task-criteria-gate.ts), independent of + // whatever the review pipeline itself would have decided. + const loop = loopRig({ + criteria: [AC1, AC2], + plan: () => ({ + verdict: 'approve', + criteria: [ + { criterion_id: AC1.id, status: 'met' }, + { + criterion_id: AC2.id, + status: 'unclear', + question: 'does this match the sibling helper?', + }, + ], + blocked: { code: 'criteria_unmet', detail: '1 of 2 acceptance criteria are not satisfied' }, + }), + }) + const cycles = await loop.drive() + // JUDGMENT_ONLY_MAX_ROUNDS fix rounds, then a THIRD cycle that ships + // instead of asking for one more — whatever the configured `maxAutoFixRounds` + // (2 here, same default the other tests in this block use) allows. + expect(cycles).toBe(JUDGMENT_ONLY_MAX_ROUNDS + 1) + expect(loop.rig.replies).toHaveLength(JUDGMENT_ONLY_MAX_ROUNDS) + expect(loop.record.status).toBe('review_ok') + expect(loop.record.reason).toBeUndefined() + expect(loadTask(loop.project.path, loop.record.id)?.status).toBe('review_ok') + const shipped = readTaskEvents(loop.project.path, loop.record.id).find( + (e) => e.data.name === AUTO_FIX_SHIP_NAME, + ) + expect(shipped).toBeDefined() + expect(String(shipped?.data.text)).toContain('open judgment calls') + // Never handed to a human, and never the exhausted-budget line either. + expect( + readTaskEvents(loop.project.path, loop.record.id).some( + (e) => e.data.name === AUTO_FIX_EXHAUSTED_NAME, + ), + ).toBe(false) + }) + test('the exit is written by the SINGLE owner: the disk never shows review_ko first', async () => { const loop = loopRig({ plan: () => blockedByFindings }) // Only the last cycle's writes matter — the two retries legitimately diff --git a/packages/cli/src/task-server.ts b/packages/cli/src/task-server.ts index d37f0e3..ecf83fe 100644 --- a/packages/cli/src/task-server.ts +++ b/packages/cli/src/task-server.ts @@ -87,6 +87,7 @@ import { } from './runbook-setup.js' import { loadSyncCredentials } from './sync.js' import { microvmStepExecutor, runChecks } from './task-checks.js' +import { criteriaBlockKind } from './task-criteria-gate.js' import { applyFixLoopDecision, AUTO_FIX_EXHAUSTED_NAME, @@ -94,6 +95,7 @@ import { AUTO_FIX_NOT_QUEUED_NAME, AUTO_FIX_NOT_STARTED_NAME, AUTO_FIX_ROUND_NAME, + AUTO_FIX_SHIP_NAME, autoFixRoundsUsed, decideFixLoop, type FixLoopDecision, @@ -147,6 +149,7 @@ import { applyChecksGate, buildAutoFixTurnPrompt, createTaskReviewer, + readReviewRef, readTaskReview, terminalChecksResult, type CreateTaskReviewerOptions, @@ -156,6 +159,7 @@ import { commandForTask, createTaskRunner, pendingResumeTurn, + taskCriteria, type RunTaskTurnMicrovmOptions, type TaskActionResult, type TaskRunner, @@ -3064,12 +3068,23 @@ export function createTaskManager(opts: CreateTaskManagerOptions): TaskManager { // as the fault lasted, i.e. remove the bound entirely. const journal = readTaskJournal(cwd, record.id) state.journalDropped = journal.dropped + // D26: `readReviewRef` reads off the in-memory `record.review_ref` + // `reviewTurn` just set on THIS object, same as `buildAutoFixTurnPrompt` + // above — never a fresh `loadTask`, which would still answer with + // whatever task.json on disk said before the persist a few lines below + // actually writes it. + const judgmentOnly = + record.reason?.code === 'criteria_unmet' && + reviewArchived && + criteriaBlockKind(taskCriteria(record), readReviewRef(record)?.review.criteria) === + 'judgment_open' state.loop = decideFixLoop({ status: record.status, reason: record.reason, roundsUsed: journal.unreadable ? null : autoFixRoundsUsed(journal.events), max: maxAutoFixRounds, fixable: state.fixPrompt !== null, + judgmentOnly, }) applyFixLoopDecision(record, state.loop) // D20: posed in the SAME write as the verdict that decides it, right @@ -3229,6 +3244,13 @@ export function createTaskManager(opts: CreateTaskManagerOptions): TaskManager { data: { text: loop.text, name: AUTO_FIX_NOT_STARTED_NAME }, reason_code: loop.code, }) + } else if (loop.kind === 'ship') { + // D26: `applyGates` already turned this into `review_ok` (folded into + // the write above), which is what let the auto-ship block just above + // fire on its own ordinary condition — this is only the journal's own + // half, so a human reading the timeline sees WHY the machine stopped + // retrying instead of assuming a review that simply approved. + io.emit({ type: 'message', data: { text: loop.text, name: AUTO_FIX_SHIP_NAME } }) } } /** Rank last broadcast per waiting id, so only real changes go on the wire. */ diff --git a/packages/cli/src/task-ship.test.ts b/packages/cli/src/task-ship.test.ts index b70ce39..455b3bb 100644 --- a/packages/cli/src/task-ship.test.ts +++ b/packages/cli/src/task-ship.test.ts @@ -1,9 +1,14 @@ import { execFileSync } from 'node:child_process' -import { existsSync, mkdtempSync, readFileSync, rmSync } from 'node:fs' +import { existsSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs' import { tmpdir } from 'node:os' import { join } from 'node:path' import { afterEach, describe, expect, test } from 'bun:test' -import type { RecapRecord, TaskRecord } from './contract.js' +import { + acceptanceCriterionId, + type RecapRecord, + type ReviewRecord, + type TaskRecord, +} from './contract.js' import type { SandboxDriver, SandboxExecOptions, @@ -29,6 +34,7 @@ import { type ShipGitExecFn, type ShipTaskOptions, } from './task-ship.js' +import { saveTask } from './tasks-store.js' // --- rig ------------------------------------------------------------------ @@ -702,6 +708,61 @@ describe('shipTask writes the task recap', () => { expect(readFileSync(recapPath(cwd, task), 'utf8')).toContain('AKIAIOSFODNN7EXAMPLE') }) + // D26: the recap generator wires the task's own archived review criteria + // through (T3.2's per-criterion verdicts are readable now), so the merge + // request the ship opens carries the "To decide" section without either the + // ship or the recap having to invent one. + test('an open judgment call surfaces in the MR description (D26)', async () => { + const cwd = makeDir() + const criterionText = 'WHEN the helper is added THE SYSTEM SHALL match the existing style' + const criterion = { id: acceptanceCriterionId(criterionText), text: criterionText } + const reviewPath = join(cwd, 'review.json') + const task = makeTask({ criteria: [criterion], review_ref: reviewPath }) + saveTask(cwd, task) + const review: ReviewRecord = { + version: 1, + meta: { + title: task.title, + branch: task.branch, + target: 'main', + merge_base: 'abc123', + repo_root: cwd, + created_at: new Date().toISOString(), + }, + commits: [], + diff: '', + review: { + verdict: 'approve', + summary: 'looks fine', + findings: [], + narrative: null, + criteria: [ + { + criterion_id: criterion.id, + status: 'unclear', + question: 'Is this the same pattern as the other adapters in this module?', + }, + ], + }, + } + writeFileSync(reviewPath, JSON.stringify(review)) + const git = gitExec({ kind: 'ok', stdout: '' }) + const forge = forgeExec({ gh: { kind: 'ok', stdout: 'https://github.com/o/r/pull/9' } }) + await shipTask({ cwd, task, execGit: git.fn, execForge: forge.fn }) + const written = readTaskRecap(cwd, task.id) + expect(written?.criteria).toEqual([ + { + criterion_id: criterion.id, + status: 'unclear', + question: 'Is this the same pattern as the other adapters in this module?', + text: criterionText, + }, + ]) + const body = forge.calls[0]?.args[forge.calls[0].args.indexOf('--body') + 1] ?? '' + expect(body).toContain('## To decide') + expect(body).toContain('Is this the same pattern as the other adapters in this module?') + }) + // MAJEUR 1, ship side. Every secret fixture on this surface planted the // secret in the last turn's response, which reaches `summary` and only // `summary` — so `prepareRecap` scanning the FULL rendering rather than one diff --git a/packages/cli/src/task-ship.ts b/packages/cli/src/task-ship.ts index 4d6edfb..7966671 100644 --- a/packages/cli/src/task-ship.ts +++ b/packages/cli/src/task-ship.ts @@ -25,7 +25,14 @@ // recap about work that was not shipped. import { execFile } from 'node:child_process' -import { type ReasonCode, type RecapRecord, type SecretMatch, type TaskRecord } from './contract.js' +import { + type AcceptanceCriterion, + type CriterionVerdict, + type ReasonCode, + type RecapRecord, + type SecretMatch, + type TaskRecord, +} from './contract.js' import type { ForgeDegradation } from './degraded-mode.js' import { detectForgeHint, forgeHintOfUrl, subprocessEnv, type ForgeHint } from './git.js' import { t, type MessageKey } from './i18n.js' @@ -39,6 +46,8 @@ import { } from './microsandbox-driver.js' import { scanRecapSecrets } from './task-recap-publish.js' import { generateRecap, renderRecapMarkdown, writeTaskRecap } from './task-recap.js' +import { readTaskReview } from './task-review.js' +import { taskCriteria } from './task-runner.js' /** Pushes and MR creations talk to the network: much looser than forge-mrs's 8s list timeout. */ export const SHIP_EXEC_TIMEOUT_MS = 60_000 @@ -844,20 +853,41 @@ type PreparedRecap = { /** Either the recap that landed on disk, or the readable reason none did. */ type PersistedRecap = { recap: RecapRecord } | { reason: string } +/** + * T3.2 is done: per-criterion verdicts are readable off the task's own + * archived review (`readTaskReview`, task-review.ts), so they ride into the + * recap generator exactly like `contribution` does — a MEASUREMENT, never a + * figure this function invents. Empty when the task carries no criteria or no + * archive: `generateRecap`'s `buildCriteria` already reads that as "no + * criteria judged", and the recap's "Acceptance criteria" / "To decide" + * sections stay omitted, per `renderRecapMarkdown`, never rendered empty. + * + * Its own function, not inlined into `generateAndPersist`, so the two + * branches this needs (has criteria? did the archive answer?) do not push + * that function's own complexity past the lint's bound for a concern that is + * really about ONE optional input, not about the recap pipeline as a whole. + */ +function criteriaForRecap( + opts: ShipTaskOptions, +): { criteriaVerdicts: CriterionVerdict[]; acceptanceCriteria: AcceptanceCriterion[] } | object { + const criteria = taskCriteria(opts.task) + if (criteria.length === 0) { + return {} + } + const criteriaVerdicts = readTaskReview(opts.cwd, opts.task.id)?.review.criteria + return criteriaVerdicts ? { criteriaVerdicts, acceptanceCriteria: criteria } : {} +} + function generateAndPersist(opts: ShipTaskOptions): PersistedRecap { const generate = opts.generateRecapFn ?? generateRecap const write = opts.writeTaskRecapFn ?? writeTaskRecap try { const contribution = lastTurnContribution(opts.task) - // `criteriaVerdicts` is deliberately NOT passed: T3.2 is the ticket that - // makes per-criterion verdicts readable off a review, and inventing a - // source for them here would put a figure in the recap that nothing - // measured. Until then the recap's "Acceptance criteria" section is - // absent — omitted, per renderRecapMarkdown, not rendered empty. const result = generate({ cwd: opts.cwd, task: opts.task, ...(contribution ? { modelOutput: contribution } : {}), + ...criteriaForRecap(opts), }) if (result.recap === null) { // The generator refuses only when the record has no usable branch: there diff --git a/packages/contract/src/index.test.ts b/packages/contract/src/index.test.ts index 00cfd4c..dc08a06 100644 --- a/packages/contract/src/index.test.ts +++ b/packages/contract/src/index.test.ts @@ -1,6 +1,7 @@ import { describe, expect, test } from 'bun:test' import { CRITERION_VERDICT_EVIDENCE_MAX, + CRITERION_VERDICT_QUESTION_MAX, detectDiffSecrets, groundCriterionVerdicts, groundReview, @@ -858,6 +859,42 @@ describe('groundCriterionVerdicts', () => { groundCriterionVerdicts([met('src/auth.ts:11 x')], null as unknown as string), ).not.toThrow() }) + + // D26: `question` has nothing to do with grounding — it must survive every + // path, including the ones that demote or strip `evidence`. + test('question survives on the grounded (unchanged) path', () => { + const verdict = met('src/auth.ts:11 — added here') + const withQuestion = { ...verdict, question: 'does this cover anonymous users?' } + const { verdicts } = groundCriterionVerdicts([withQuestion], GROUND_DIFF) + expect(verdicts).toEqual([withQuestion]) + }) + + test('question survives even when the verdict is demoted to unclear', () => { + const claimed: CriterionVerdict = { + criterion_id: AC_A, + status: 'met', + evidence: 'src/ghost.ts:3 — invented', + question: 'is this the same pattern as the sibling module?', + } + const { verdicts } = groundCriterionVerdicts([claimed], GROUND_DIFF) + expect(verdicts).toEqual([ + { + criterion_id: AC_A, + status: 'unclear', + question: 'is this the same pattern as the sibling module?', + }, + ]) + }) + + test('question survives on a bare unclear with no evidence at all', () => { + const unclear: CriterionVerdict = { + criterion_id: AC_A, + status: 'unclear', + question: 'what should happen on an empty list?', + } + const { verdicts } = groundCriterionVerdicts([unclear], GROUND_DIFF) + expect(verdicts).toEqual([unclear]) + }) }) // --- T3.2 round 2, majeur 1(a): what we agree to READ as an anchor ---------- @@ -1328,7 +1365,7 @@ describe('cross test: sanitizeRecord output validates against reviewRecordSchema files_reviewed: ['src/auth.ts', { path: 'docs/removed.md', status: 'clean' }], criteria: [ { criterion_id: AC_A, status: 'met', evidence: 'src/auth.ts:11 — added here' }, - { criterion_id: AC_B, status: 'unmet' }, + { criterion_id: AC_B, status: 'unclear', question: 'does this cover offline mode too?' }, ], }, }) @@ -1415,6 +1452,36 @@ describe('reverse cross test: reviewRecordSchema is not looser than sanitizeRevi ).toEqual([]) }) + test('a question past the published bound is schema-invalid (D26) — the sanitizer TRUNCATES to it', () => { + const tooLong = 'x'.repeat(CRITERION_VERDICT_QUESTION_MAX + 1) + expect( + schemaErrors(withCriteria([{ criterion_id: AC_A, status: 'unclear', question: tooLong }])), + ).not.toEqual([]) + expect( + schemaErrors( + withCriteria([ + { + criterion_id: AC_A, + status: 'unclear', + question: 'x'.repeat(CRITERION_VERDICT_QUESTION_MAX), + }, + ]), + ), + ).toEqual([]) + }) + + test('an empty or whitespace-only question is schema-invalid (D26) — the sanitizer OMITS the key', () => { + for (const question of ['', ' ', '\n\t ']) { + expect( + schemaErrors(withCriteria([{ criterion_id: AC_A, status: 'unclear', question }])), + ).not.toEqual([]) + expect( + sanitizeReview({ criteria: [{ criterion_id: AC_A, status: 'unclear', question }] }) + .criteria, + ).toEqual([{ criterion_id: AC_A, status: 'unclear' }]) + } + }) + test('an empty or whitespace-only evidence is schema-invalid — the sanitizer OMITS the key', () => { for (const evidence of ['', ' ', '\n\t ']) { expect( diff --git a/packages/contract/src/index.ts b/packages/contract/src/index.ts index 366960f..e607781 100644 --- a/packages/contract/src/index.ts +++ b/packages/contract/src/index.ts @@ -5,6 +5,7 @@ // spellings of the same fact. ticket.ts imports nothing, so this cannot cycle. import { CRITERION_VERDICT_EVIDENCE_MAX, + CRITERION_VERDICT_QUESTION_MAX, NON_BLANK, sanitizeCriterionVerdicts, TICKET_CRITERIA_MAX, @@ -1029,6 +1030,20 @@ export type CriteriaGroundingReport = { * behind it. So the output degrades — every `met` becomes `unclear` — and the * report says why. It still never throws. */ +/** + * D26: `criterion_id` + `status`, plus `question` carried over UNCHANGED when + * the source verdict had one — its own function so the ternary it takes + * (present or not) is not one more branch inside `groundCriterionVerdicts`, + * which already has plenty that ARE about grounding. + */ +function withCarriedQuestion( + criterionId: string, + status: CriterionStatus, + question: string | undefined, +): CriterionVerdict { + return { criterion_id: criterionId, status, ...(question ? { question } : {}) } +} + export function groundCriterionVerdicts( verdicts: readonly CriterionVerdict[], diff: string, @@ -1059,7 +1074,11 @@ export function groundCriterionVerdicts( if (status !== verdict.status) { report.demoted.push(verdict) } - out.push({ criterion_id: verdict.criterion_id, status }) + // `question` (D26) has nothing to do with evidence grounding — it is + // carried through UNCHANGED on every path, including this one: a sincere + // "I could not anchor this" is exactly the case whose question matters + // most, and it must not be the one path that silently drops it. + out.push(withCarriedQuestion(verdict.criterion_id, status, verdict.question)) } return { verdicts: out, report } } @@ -1157,6 +1176,16 @@ export const reviewRecordSchema = { maxLength: CRITERION_VERDICT_EVIDENCE_MAX, pattern: NON_BLANK, }, + // D26: the reviewer's own question when `status` is 'unclear' for + // want of information the diff cannot supply. Same NON_BLANK + // discipline as `evidence` — `sanitizeCriterionVerdict` trims before + // checking for emptiness and omits the key rather than storing a + // blank one. + question: { + type: 'string', + maxLength: CRITERION_VERDICT_QUESTION_MAX, + pattern: NON_BLANK, + }, }, }, reviewedFile: { diff --git a/packages/contract/src/reasons.test.ts b/packages/contract/src/reasons.test.ts index ad711cd..ac01619 100644 --- a/packages/contract/src/reasons.test.ts +++ b/packages/contract/src/reasons.test.ts @@ -36,11 +36,14 @@ const D2_CODES = [ */ const T3_6_CODES = ['checks_unavailable', 'criteria_missing'] as const +/** What D26 added: an open judgment call blocks the automatic merge alone. */ +const D26_CODES = ['criteria_judgment_open'] as const + /** * The whole table as it stands today, in DECLARATION order — which is not - * `[...D2_CODES, ...T3_6_CODES]`: the table groups the terminal codes first, - * so T3.6's two land in the middle, after `branch_diverged`. Spelled out so - * the snapshot below really is a snapshot. + * `[...D2_CODES, ...T3_6_CODES, ...D26_CODES]`: the table groups the terminal + * codes first, so T3.6's two and D26's one land in the middle, after + * `branch_diverged`. Spelled out so the snapshot below really is a snapshot. */ const EXPECTED_CODES = [ 'checks_failed', @@ -50,6 +53,7 @@ const EXPECTED_CODES = [ 'branch_diverged', 'checks_unavailable', 'criteria_missing', + 'criteria_judgment_open', 'merge_strategy_unconfigured', 'agent_error', 'inactivity_timeout', @@ -77,6 +81,9 @@ const EXPECTED_TERMINAL: Record<(typeof EXPECTED_CODES)[number], boolean> = { // Waiting configures no merge strategy either: the way out is one setting, // then a retried merge. merge_strategy_unconfigured: true, + // D26: waiting settles no judgment call — only a human, merging by hand, + // does. + criteria_judgment_open: true, } describe('REASON_CODES', () => { @@ -112,20 +119,28 @@ describe('REASON_CODES', () => { } }) - test('today the table is D2 plus T3.6 plus the merge-strategy gate: thirteen codes', () => { + test('today the table is D2 plus T3.6 plus the merge-strategy gate plus D26: fourteen codes', () => { // The snapshot of the CURRENT roster. Only a deliberate extension touches // this line — never a rename, which the two lock tests above catch first. expect(REASON_CODES.map((entry) => entry.code)).toEqual([...EXPECTED_CODES]) - expect(REASON_CODES).toHaveLength(13) + expect(REASON_CODES).toHaveLength(14) + }) + + test('locks the code D26 added by NAME too, on the same terms', () => { + const names = REASON_CODES.map((entry) => entry.code) + for (const code of D26_CODES) { + expect(names).toContain(code) + } }) test('each code is classified the way this contract documents it', () => { for (const entry of REASON_CODES) { expect(entry.terminal).toBe(EXPECTED_TERMINAL[entry.code]) } - // Eight terminal (D2's five plus `checks_unavailable`, `criteria_missing` - // and `merge_strategy_unconfigured`); the retryable half is untouched at five. - expect(REASON_CODES.filter((entry) => entry.terminal)).toHaveLength(8) + // Nine terminal (D2's five plus `checks_unavailable`, `criteria_missing`, + // `merge_strategy_unconfigured` and D26's `criteria_judgment_open`); the + // retryable half is untouched at five. + expect(REASON_CODES.filter((entry) => entry.terminal)).toHaveLength(9) expect(REASON_CODES.filter((entry) => !entry.terminal)).toHaveLength(5) }) }) diff --git a/packages/contract/src/reasons.ts b/packages/contract/src/reasons.ts index 03ee17a..5707f0d 100644 --- a/packages/contract/src/reasons.ts +++ b/packages/contract/src/reasons.ts @@ -117,6 +117,19 @@ export const REASON_CODES = [ code: 'criteria_missing', terminal: true, }, + { + // D26: at least one acceptance criterion is a judgment call the reviewer + // could settle only as 'unclear' with a genuine question attached — never + // `criteria_unmet`, which stays reserved for a criterion the diff itself + // falsifies or the reviewer never judged at all. A ship carrying only + // this kind of open criterion still SHIPS (T3.2/D18's waiver): what this + // code blocks is the AUTOMATIC merge alone, because a human never signed + // off on the call. Terminal: waiting settles nothing here — a human + // reading the merge request's "To decide" section and merging the branch + // themselves is the decision, and there is no other way to reach it. + code: 'criteria_judgment_open', + terminal: true, + }, { // The automatic merge was refused BEFORE any forge CLI ran because no // mergeStrategy is configured (recovery doctrine: a consent nobody gave diff --git a/packages/contract/src/recap.test.ts b/packages/contract/src/recap.test.ts index 9d07852..a8af5bc 100644 --- a/packages/contract/src/recap.test.ts +++ b/packages/contract/src/recap.test.ts @@ -403,6 +403,36 @@ describe('sanitizeRecap', () => { expect(out?.criteria).toEqual([{ criterion_id: CID_A, status: 'met', evidence: 'first pass' }]) }) + // D26: `question` flows through `sanitizeCriterionVerdict` (ticket.ts), + // which `sanitizeRecapCriterion` delegates to before adding `text` — no + // change needed in THIS module's own code, only proof it actually happens. + test('criteria[]: question (D26) survives alongside the denormalized text', () => { + const out = sanitizeRecap({ + branch: 'main', + summary: '', + changes: [], + decisions: [], + files: [], + tests: [], + criteria: [ + { + criterion_id: CID_A, + status: 'unclear', + question: 'does this match the sibling helper?', + text: 'WHEN the helper is added THE SYSTEM SHALL match the existing style', + }, + ], + }) + expect(out?.criteria).toEqual([ + { + criterion_id: CID_A, + status: 'unclear', + question: 'does this match the sibling helper?', + text: 'WHEN the helper is added THE SYSTEM SHALL match the existing style', + }, + ]) + }) + test('tests[]: a synthetic entry round-trips its flag; a real one never gains it', () => { const out = sanitizeRecap({ branch: 'main', @@ -931,6 +961,15 @@ describe('reverse cross test: the schema is not looser than what sanitizeRecap a ).not.toEqual([]) }) + test('an empty criteria[].question is schema-invalid (D26) — the sanitizer never emits an empty question key', () => { + expect( + schemaErrors({ + ...BASE, + criteria: [{ criterion_id: CID_A, status: 'unclear', question: '' }], + }), + ).not.toEqual([]) + }) + // --- Round 4, majeur 3: `minLength: 1` alone let a WHITESPACE-ONLY string // through — every one of the eight locations below is trimmed (str/line) // before the sanitizer checks for emptiness, so a schema that stopped at diff --git a/packages/contract/src/recap.ts b/packages/contract/src/recap.ts index 939a3dc..3bd9558 100644 --- a/packages/contract/src/recap.ts +++ b/packages/contract/src/recap.ts @@ -14,6 +14,7 @@ import { TASK_PATH_MAX, type CostBasis, type TaskCheckStatus } from './tasks.js' import { CRITERION_VERDICT_EVIDENCE_MAX, + CRITERION_VERDICT_QUESTION_MAX, cutCodePoints, NON_BLANK, sanitizeCriterionVerdict, @@ -521,6 +522,13 @@ export const recapRecordSchema = { maxLength: CRITERION_VERDICT_EVIDENCE_MAX, pattern: NON_BLANK, }, + // D26, same $def as `reviewRecordSchema`'s own criterionVerdict: the + // reviewer's question when the verdict is 'unclear'. + question: { + type: 'string', + maxLength: CRITERION_VERDICT_QUESTION_MAX, + pattern: NON_BLANK, + }, // `text` is bounded through line() (round 4, majeur 1): NON_BLANK_MONO_LINE. text: { type: 'string', diff --git a/packages/contract/src/ticket.test.ts b/packages/contract/src/ticket.test.ts index e19c67c..5af1cda 100644 --- a/packages/contract/src/ticket.test.ts +++ b/packages/contract/src/ticket.test.ts @@ -5,6 +5,7 @@ import { acceptanceCriterionId, canonicalTicketBody, CRITERION_VERDICT_EVIDENCE_MAX, + CRITERION_VERDICT_QUESTION_MAX, EARS_RESPONSE, EARS_TRIGGER, extractAcceptanceCriteria, @@ -2669,6 +2670,52 @@ describe('sanitizeCriterionVerdict / sanitizeCriterionVerdicts (DP12)', () => { expect(/[\uD800-\uDBFF]$/.test(verdict?.evidence ?? '')).toBe(false) expect(verdict?.evidence?.endsWith('\ud83d')).toBe(false) }) + + // --- question (D26) -------------------------------------------------------- + + test('question is optional: a verdict with none keeps none', () => { + expect(sanitizeCriterionVerdict({ criterion_id: CID, status: 'unclear' })).toEqual({ + criterion_id: CID, + status: 'unclear', + }) + }) + + test('a well-formed unclear verdict with a question round-trips unchanged', () => { + const verdict: CriterionVerdict = { + criterion_id: CID, + status: 'unclear', + question: 'does this need to work offline too?', + } + expect(sanitizeCriterionVerdict(structuredClone(verdict))).toEqual(verdict) + }) + + test('question is bounded by its own exported constant, independent of evidence', () => { + const long = 'x'.repeat(CRITERION_VERDICT_QUESTION_MAX + 500) + const verdict = sanitizeCriterionVerdict({ + criterion_id: CID, + status: 'unclear', + question: long, + }) + expect(verdict?.question).toHaveLength(CRITERION_VERDICT_QUESTION_MAX) + }) + + test('a blank question is omitted, never stored as whitespace', () => { + expect( + sanitizeCriterionVerdict({ criterion_id: CID, status: 'unclear', question: ' ' }), + ).toEqual({ + criterion_id: CID, + status: 'unclear', + }) + }) + + test('a lone CR in question is normalized to LF, same recipe as evidence', () => { + const verdict = sanitizeCriterionVerdict({ + criterion_id: CID, + status: 'unclear', + question: 'first\rsecond', + }) + expect(verdict?.question).toBe('first\nsecond') + }) }) // --- The published schema --------------------------------------------------- diff --git a/packages/contract/src/ticket.ts b/packages/contract/src/ticket.ts index 1ea250f..523191f 100644 --- a/packages/contract/src/ticket.ts +++ b/packages/contract/src/ticket.ts @@ -110,6 +110,18 @@ export type CriterionVerdict = { status: CriterionStatus /** The reviewer's own quoted grounding for the status. OPTIONAL: a verdict can stand without one. */ evidence?: string + /** + * D26: the ONE precise question the reviewer could not answer from the diff + * alone, when `status` is `'unclear'` — what is missing, not a restatement + * of the criterion. OPTIONAL, and its absence is not an error: an `unclear` + * predating D26, or one a caller built by hand, simply carries none, which + * downstream (the MR's "To decide" section) reads as "no question was + * recorded" rather than inventing one. Meaningless beside `met`/`unmet` — a + * verdict that settled the criterion has nothing left to ask — but never + * stripped from one either: this type states what a verdict MAY carry, not + * what a settled one SHOULD. + */ + question?: string } /** @@ -146,6 +158,14 @@ export const TICKET_CRITERIA_MIN = 3 */ export const CRITERION_VERDICT_EVIDENCE_MAX = 1_000 +/** + * Bound of `CriterionVerdict.question` (D26). A human-facing question read on + * a merge-request card, not prose: shorter than a criterion's own text + * (`TICKET_CRITERION_TEXT_MAX`) because a question that needs more room is + * naming more than one missing fact. + */ +export const CRITERION_VERDICT_QUESTION_MAX = 300 + /** * JSON Schema `pattern` for a string that is neither empty NOR whitespace-only: * a non-whitespace first AND last character. It SUBSUMES `minLength: 1` (an @@ -409,10 +429,21 @@ export function sanitizeCriterionVerdict(raw: unknown): CriterionVerdict | null CRITERION_VERDICT_EVIDENCE_MAX, ).trim() : '' + // Same recipe as `evidence`, bounded independently (D26): a question is + // read on its own, never alongside the criterion's own text in the same + // budget. + const question = + typeof r.question === 'string' + ? cutCodePoints( + r.question.replace(/\r\n?/g, '\n').trim(), + CRITERION_VERDICT_QUESTION_MAX, + ).trim() + : '' return { criterion_id: r.criterion_id, status, ...(evidence ? { evidence } : {}), + ...(question ? { question } : {}), } }