From 69a71f5da25f4d1ccd3cfd2f77055b576e1ccf1e Mon Sep 17 00:00:00 2001 From: garrytan Date: Mon, 5 Oct 2026 19:23:49 +0000 Subject: [PATCH] fix(office-hours): only blocking findings send a design back for another review round Review findings carry severity blocking|minor (verdict schema v2). PASS when no blocking findings remain; minor findings are recorded in Reviewer Concerns and the Spec Review table, never a reason for another round. CONVERGENCE needs a persisting blocking prior. A persisting or unverified blocking prior cannot be relabeled minor. The 3-round cap is unchanged. --- lib/office-hours-review.ts | 47 ++++++++++++----- office-hours/sections/design-and-handoff.md | 18 ++++--- scripts/resolvers/spec-review.ts | 18 ++++--- test/gen-skill-docs.test.ts | 2 +- test/office-hours-completion.test.ts | 27 ++++++++-- test/office-hours-review.test.ts | 57 ++++++++++++++++++--- 6 files changed, 132 insertions(+), 37 deletions(-) diff --git a/lib/office-hours-review.ts b/lib/office-hours-review.ts index a3b9b13bb..c00036e8a 100644 --- a/lib/office-hours-review.ts +++ b/lib/office-hours-review.ts @@ -5,9 +5,12 @@ import * as path from 'node:path'; export const OFFICE_HOURS_DIMENSIONS = ['completeness', 'consistency', 'clarity', 'scope', 'feasibility'] as const; export type OfficeHoursDimension = typeof OFFICE_HOURS_DIMENSIONS[number]; +export const OFFICE_HOURS_SEVERITIES = ['blocking', 'minor'] as const; +export type OfficeHoursSeverity = typeof OFFICE_HOURS_SEVERITIES[number]; export interface OfficeHoursFinding { id: string; dimension: OfficeHoursDimension; + severity: OfficeHoursSeverity; problem: string; remedy: string; } @@ -18,7 +21,7 @@ export interface OfficeHoursPriorStatus { current_id: string | null; } export interface OfficeHoursReview { - version: 1; + version: 2; round: number; document: string; quality_score: number; @@ -32,6 +35,8 @@ export interface OfficeHoursReviewMetrics { issues_found: number; issues_fixed: number; remaining: number; + remaining_blocking: number; + remaining_minor: number; quality_score: number | null; attempted_fix_rounds: number; } @@ -49,7 +54,7 @@ function nonempty(value: unknown, label: string): string { export function validateOfficeHoursReview(value: unknown, previous?: OfficeHoursReview): OfficeHoursReview { const review = object(value, ['version', 'round', 'document', 'quality_score', 'dimensions', 'findings', 'prior'], 'artifact'); - if (review.version !== 1) fail('unsupported version'); + if (review.version !== 2) fail('unsupported version; expected 2'); if (!Number.isInteger(review.round) || review.round < 1 || review.round > 3 || review.round !== (previous?.round ?? 0) + 1) fail('rounds must be contiguous, starting at 1, with at most 3 rounds'); nonempty(review.document, 'document'); @@ -61,11 +66,12 @@ export function validateOfficeHoursReview(value: unknown, previous?: OfficeHours if (!Array.isArray(review.findings) || !Array.isArray(review.prior)) fail('findings and prior must be arrays'); const ids = new Set(); for (const raw of review.findings) { - const finding = object(raw, ['id', 'dimension', 'problem', 'remedy'], 'finding'); + const finding = object(raw, ['id', 'dimension', 'severity', 'problem', 'remedy'], 'finding'); if (typeof finding.id !== 'string' || !new RegExp(`^R${review.round}-[1-9][0-9]*$`).test(finding.id) || ids.has(finding.id)) fail('finding ids must be unique R- identifiers'); ids.add(finding.id); if (!OFFICE_HOURS_DIMENSIONS.includes(finding.dimension)) fail(`invalid dimension for ${finding.id}`); + if (!OFFICE_HOURS_SEVERITIES.includes(finding.severity)) fail(`${finding.id} severity must be blocking or minor`); nonempty(finding.problem, `${finding.id} problem`); nonempty(finding.remedy, `${finding.id} remedy`); } @@ -74,6 +80,8 @@ export function validateOfficeHoursReview(value: unknown, previous?: OfficeHours if (dimensions[dimension] !== expected) fail(`${dimension} must be ${expected} for its canonical findings`); } const previousIds = new Set(previous?.findings.map(finding => finding.id) ?? []); + const blocking = new Set([...(previous?.findings ?? []), ...review.findings as OfficeHoursFinding[]] + .filter(finding => finding.severity === 'blocking').map(finding => finding.id)); const covered = new Set(); const currentLinks = new Set(); for (const raw of review.prior) { @@ -89,15 +97,19 @@ export function validateOfficeHoursReview(value: unknown, previous?: OfficeHours } else { if (currentLinks.has(status.current_id)) fail('distinct prior findings cannot merge into one current finding'); currentLinks.add(status.current_id); + if (blocking.has(status.id) && !blocking.has(status.current_id)) fail(`${status.status} blocking ${status.id} cannot be downgraded to minor`); } } if (covered.size !== previousIds.size) fail('prior must cover each preceding finding exactly once'); return review as OfficeHoursReview; } +const blockingIn = (review: OfficeHoursReview) => review.findings.filter(finding => finding.severity === 'blocking'); +/** Minor findings are recorded, never a reason for another round. */ function stopFor(review: OfficeHoursReview): OfficeHoursReviewStop { - if (review.findings.length === 0) return 'PASS'; - if (review.prior.some(status => status.status === 'persisting')) return 'CONVERGENCE'; + const blocking = new Set(blockingIn(review).map(finding => finding.id)); + if (blocking.size === 0) return 'PASS'; + if (review.prior.some(status => status.status === 'persisting' && blocking.has(status.current_id!))) return 'CONVERGENCE'; return review.round === 3 ? 'MAX_ITERATIONS' : 'CONTINUE'; } function metricsFor(rounds: readonly OfficeHoursReview[]): OfficeHoursReviewMetrics { @@ -107,6 +119,8 @@ function metricsFor(rounds: readonly OfficeHoursReview[]): OfficeHoursReviewMetr issues_found: rounds.reduce((sum, review) => sum + review.findings.length, 0), issues_fixed: rounds.reduce((sum, review) => sum + review.prior.filter(status => status.status === 'resolved').length, 0), remaining: last?.findings.length ?? 0, + remaining_blocking: last ? blockingIn(last).length : 0, + remaining_minor: last ? last.findings.length - blockingIn(last).length : 0, quality_score: last?.quality_score ?? null, attempted_fix_rounds: Math.max(0, rounds.length - 1), }; @@ -165,9 +179,9 @@ export function renderOfficeHoursReviewerPrompt({ document, verdictPath, previou if (previous && stopFor(previous) !== 'CONTINUE') fail('cannot prepare another round after a terminal review'); const round = (previous?.round ?? 0) + 1; const example = { - version: 1, round, document, quality_score: 7, + version: 2, round, document, quality_score: 7, dimensions: { completeness: 'PASS', consistency: 'PASS', clarity: 'ISSUES', scope: 'PASS', feasibility: 'PASS' }, - findings: [{ id: `R${round}-1`, dimension: 'clarity', problem: "The fallback's user-visible behavior is unspecified.", + findings: [{ id: `R${round}-1`, dimension: 'clarity', severity: 'blocking', problem: "The fallback's user-visible behavior is unspecified.", remedy: 'Choose and document whether the fallback warns the user or is intentionally silent.' }], prior: [], }; @@ -182,11 +196,19 @@ Return only that printed \`OFFICE_HOURS_VERDICT round=${round} sha256= pat The saved JSON is your sole findings inventory: include every unresolved problem and necessary remedy, including minor findings that a short conclusion might omit. Use one finding per distinct obligation. An exact duplicate shares a finding; a shared component does not combine separate decisions, behavior, or effort. +## Severity + +Give every finding a severity. Only blocking findings send the design back for another round; minor findings are recorded for the user and never require another round on their own. +- **blocking**: a contradiction; a safety or correctness risk; an unsupported claim the recommendation depends on; missing behavior the committed approach needs; or a persisting blocking prior obligation. Example: the design promises the roster is never stored, yet its sync step saves it nightly. +- **minor**: clarity, wording, or polish that does not change a decision or behavior. Example: the Recommended Approach repeats the problem statement's wording and could be shorter. +When unsure whether a gap changes a decision or behavior, it is blocking. + This is an /office-hours design and coaching document, produced before engineering planning. The startup-mode 'The Assignment' and both modes' 'What I noticed about how you think' sections are intentional: evaluate their evidence and usefulness; do not remove them merely because they are coaching content. Unknown customer facts may remain explicit Open Questions or assignments; do not invent answers. Still flag unsupported claims, contradictions, safety/correctness risks, and missing behavior needed by the approach the document actually commits to. Labeling a contradiction or a required behavior an open question does not resolve it. On re-review, classify EVERY preceding finding as resolved, persisting, or unverified. Cite the specific document decision/behavior proving the status or the missing evidence. Absence from the new findings list is not confirmation. A new refinement of an accepted fix is new unless the same specific original obligation demonstrably remains unmet. For persisting/unverified issues, include that unmet obligation in the current findings and reference its current ID. Distinct prior obligations must retain distinct current findings. +Classify minor and blocking preceding findings alike. A persisting or unverified blocking finding stays blocking until resolved; never relabel it minor. Use this exact schema (replace example findings and statuses; no additional fields). The round and document below are assigned values: @@ -194,7 +216,7 @@ Use this exact schema (replace example findings and statuses; no additional fiel ${JSON.stringify(example, null, 2)} \`\`\` -Finding IDs are R${round}-; dimension names are the five lowercase keys above. Supply a quality score from 1 to 10. A dimension is ISSUES exactly when it has findings; otherwise PASS. +Finding IDs are R${round}-; dimension names are the five lowercase keys above; severity is blocking or minor. Supply a quality score from 1 to 10. A dimension is ISSUES exactly when it has findings; otherwise PASS. Round 1 has an empty prior array. In later rounds, replace the example's empty prior array with one status for EVERY finding in the complete preceding verdict below: {"id":"","status":"resolved","evidence":"Specific document decision proving resolution","current_id":null} or {"id":"","status":"persisting","evidence":"Same original obligation still unmet at this document passage","current_id":"R${round}-1"}. @@ -223,7 +245,7 @@ const marker = (kind: 'concerns' | 'report', edge: 'start' | 'end') => `[\s\S]*/, rendered.concerns); + evidence.output = evidence.output.replace(/[\s\S]*/, rendered.report); + const saved = [{ ...artifacts[0], content: JSON.stringify(minor) }]; + evidence.toolCalls = evidence.toolCalls.slice(0, 4); + evidence.toolCalls[2].output = officeHoursVerdictReceipt(1, saved[0].path, saved[0].content); + evidence.toolCalls[3].input!.content = saved[0].content; + expect(() => validateOfficeHoursReviewArtifacts(evidence, saved)).not.toThrow(); + expect(evidence.designContent).toContain('Disposition: COMPLETED'); + for (const [, problem, remedy] of problems) { + expect(evidence.designContent).toContain(problem); + expect(evidence.designContent).toContain(remedy); + } + }); + test('accepts a complete, reviewer-owned six-finding report without a judge', () => { const { evidence, artifacts } = structured(); expect(() => validateOfficeHoursReviewArtifacts(evidence, artifacts)).not.toThrow(); @@ -930,8 +949,8 @@ Next: /plan-eng-review after observing the workflow. The user declined launching test('rejects a five-finding count for the actual six', () => { const { evidence, artifacts } = structured(); - evidence.output = evidence.output.replace('Unresolved findings in the last completed inventory: 6.', - 'Unresolved findings in the last completed inventory: 5.'); + evidence.output = evidence.output.replace('Unresolved findings in the last completed inventory: 6 (6 blocking, 0 minor).', + 'Unresolved findings in the last completed inventory: 5 (5 blocking, 0 minor).'); expect(() => validateOfficeHoursReviewArtifacts(evidence, artifacts)).toThrow('computed metrics'); }); diff --git a/test/office-hours-review.test.ts b/test/office-hours-review.test.ts index 4007fa5e6..0ae398018 100644 --- a/test/office-hours-review.test.ts +++ b/test/office-hours-review.test.ts @@ -10,12 +10,12 @@ import { import { expectMentions } from './helpers/prompt-structure'; const cli = path.resolve(import.meta.dir, '../bin/gstack-office-hours-review'); -function review(round = 1, count = 1): OfficeHoursReview { +function review(round = 1, count = 1, severity: 'blocking' | 'minor' = 'blocking'): OfficeHoursReview { return { - version: 1, round, document: '/tmp/design.md', quality_score: 7, + version: 2, round, document: '/tmp/design.md', quality_score: 7, dimensions: { completeness: count ? 'ISSUES' : 'PASS', consistency: 'PASS', clarity: 'PASS', scope: 'PASS', feasibility: 'PASS' }, findings: Array.from({ length: count }, (_, index) => ({ - id: `R${round}-${index + 1}`, dimension: 'completeness', + id: `R${round}-${index + 1}`, dimension: 'completeness', severity, problem: `Problem ${index + 1} remains undefined.`, remedy: `Define behavior ${index + 1}.`, })), prior: [], }; @@ -41,7 +41,9 @@ describe('office-hours canonical review schema', () => { }); test.each([ - ['version', (item: any) => { item.version = 2; }], + ['version 1', (item: any) => { item.version = 1; }], + ['missing severity', (item: any) => { delete item.findings[0].severity; }], + ['unknown severity', (item: any) => { item.findings[0].severity = 'major'; }], ['unexpected fields', (item: any) => { item.summary = 'PASS'; }], ['missing dimension', (item: any) => { delete item.dimensions.scope; }], ['false dimension pass', (item: any) => { item.dimensions.completeness = 'PASS'; }], @@ -74,6 +76,8 @@ describe('office-hours canonical review schema', () => { ['blank resolution evidence', (item: any) => { item.prior[1].evidence = ''; }], ['different document', (item: any) => { item.document = '/tmp/other.md'; }], ['merged unresolved prior obligations', (item: any) => { item.prior[1].status = 'unverified'; item.prior[1].current_id = 'R2-1'; }], + ['persisting blocking finding downgraded to minor', (item: any) => { item.findings[0].severity = 'minor'; }], + ['unverified blocking finding downgraded to minor', (item: any) => { item.prior[0].status = 'unverified'; item.findings[0].severity = 'minor'; }], ])('rejects %s', (_name, change) => { const [first, second] = recurrence(); change(second); @@ -114,14 +118,55 @@ describe('office-hours prepared reviewer input', () => { }); }); +describe('office-hours severity-based convergence', () => { + test('minor findings alone PASS and are recorded, never fixed by another round', () => { + const result = renderOfficeHoursReview([review(1, 2, 'minor')]); + expect(result.stop).toBe('PASS'); + expect(result.metrics).toMatchObject({ iterations: 1, remaining: 2, remaining_blocking: 0, remaining_minor: 2 }); + for (const output of [result.concerns, result.report]) { + expect(output).toContain('Disposition: COMPLETED'); + expect(output).toContain('### R1-1 — completeness (minor)'); + expect(output).toContain('Define behavior 2.'); + expect(output).toContain('2 open minor finding(s) below are recorded for the user, not fixed'); + } + expect(result.report).toContain('| 1 | 0 | 2 | 0 | 7/10 |'); + expect(() => renderOfficeHoursReviewerPrompt({ document: '/tmp/design.md', verdictPath: '/tmp/round-2.json', previous: review(1, 1, 'minor') })).toThrow('terminal'); + }); + + test('blocking findings CONTINUE until round 3 even beside minor ones', () => { + const first = review(1, 1); + first.findings.push({ ...first.findings[0], id: 'R1-2', severity: 'minor' }); + expect(assessOfficeHoursReviews([first]).stop).toBe('CONTINUE'); + expect(renderOfficeHoursReview([first], 'timeout').report).toContain('| 1 | 1 | 1 | 0 | 7/10 |'); + }); + + test('a persisting blocking prior converges; a persisting minor prior does not', () => { + expect(assessOfficeHoursReviews(recurrence()).stop).toBe('CONVERGENCE'); + const [first, second] = recurrence(); + first.findings[0].severity = 'minor'; + second.findings[0].severity = 'minor'; + expect(assessOfficeHoursReviews([first, second]).stop).toBe('PASS'); + second.findings[0].severity = 'blocking'; + expect(assessOfficeHoursReviews([first, second]).stop).toBe('CONVERGENCE'); + }); + + test('the reviewer prompt defines both severities with an example of each', () => { + const prompt = renderOfficeHoursReviewerPrompt({ document: '/tmp/design.md', verdictPath: '/tmp/round-1.json' }); + expect(prompt).toContain('"severity": "blocking"'); + expect(prompt).toMatch(/- \*\*blocking\*\*: a contradiction;[^\n]+Example:/); + expect(prompt).toMatch(/- \*\*minor\*\*: clarity, wording, or polish[^\n]+Example:/); + expect(prompt).toMatch(/never relabel it minor/); + }); +}); + describe('office-hours lifecycle and deterministic preservation', () => { test('convergence precedes further editing even when another finding is new', () => { const rounds = recurrence(); - rounds[1].findings.push({ id: 'R2-2', dimension: 'clarity', problem: 'Encoding fallback has no user-visible policy.', remedy: 'Specify whether the latin-1 fallback emits a warning or is intentionally silent.' }); + rounds[1].findings.push({ id: 'R2-2', dimension: 'clarity', severity: 'minor', problem: 'Encoding fallback has no user-visible policy.', remedy: 'Specify whether the latin-1 fallback emits a warning or is intentionally silent.' }); rounds[1].dimensions.clarity = 'ISSUES'; const result = renderOfficeHoursReview(rounds); expect(result.stop).toBe('CONVERGENCE'); - expect(result.metrics).toEqual({ iterations: 2, issues_found: 4, issues_fixed: 1, remaining: 2, quality_score: 7, attempted_fix_rounds: 1 }); + expect(result.metrics).toEqual({ iterations: 2, issues_found: 4, issues_fixed: 1, remaining: 2, remaining_blocking: 1, remaining_minor: 1, quality_score: 7, attempted_fix_rounds: 1 }); for (const output of [result.concerns, result.report]) { for (const finding of rounds[1].findings) { expect(output).toContain(finding.problem);