From 64dcd18df41fc4a4a4a046e55a87ff23873d8fcb Mon Sep 17 00:00:00 2001 From: garrytan Date: Sat, 3 Oct 2026 01:06:31 +0000 Subject: [PATCH] test(W7c): shrink eng-review-routing to structural and meaning checks W7c conversion (t02 #4-#10, W7a s04 F15 pin). Pin lines 88 -> 74 (toContain|toMatch|indexOf|includes|ordered); ~50 full-sentence pins become order checks, field/marker checks and short case-insensitive meaning checks. Remaining gate-tier judge for the plan-eng prose: 'plan-eng-review/SKILL.md sections' (judge input is ENG_REVIEW_EXCERPT, which the first test proves contains review-sections and Recovery routing). Removed assertion groups (invariant -> remaining protection): - Preparation sentences ('Follow the blocks below in order after startup', 'Confidence Calibration and Decision procedure are reference rules'): blocks run in order -> kept free: heading order check now starts at '## Review preparation'; judge 'plan-eng-review/SKILL.md sections'. - Priority/compression sentences ('Shorten only optional commentary', 'never Scope Challenge, Sections 1-4...', 'Never condense, abbreviate or skip a section'): compression cannot drop required stages -> kept free: /complete every required stage, decision gate and output/i and the 'Everything else' stale ban; judge. - Exact seven-label toEqual on Scope Challenge assessments (t02 #8): all seven checks present -> kept free: arrayContaining on the same labels plus the A/B/C heading toEqual (excerpt anchors). - 'Complete these checks before the complexity decision in B': A runs before B -> kept free: heading order A < B < C. - Below-threshold / findings sentences (numbered step full sentences): C always runs, findings approve nothing -> kept free: step-number order, /fewer than 8 files AND fewer than 2 new classes/i, /run C whether B was completed or skipped/i, /approve no remedies/i, '"No issues found"'. - '... STOP before Section 1' exact caps (t02 #4): high complexity halts -> kept free: /at 8\+ files or 2\+ new classes\/services, stop before section 1/i. - High-complexity sentences ('With no proposed cuts...', 'Both retain the same approved feature list...', 'Initial scope selectors need no grid...'): cuts and structure asked separately -> kept free: step order with short anchors, 'Pending remedies not decided here: ' field; judge. - Smaller-arrangement sentences ('offer confirmation of the original arrangement or a pause', 'then return to this structure selector'): a pause authorizes no cut -> kept free: order + /do not continue to C until it is settled/i + /investigate only the agreed question/i. - Selector summary sentences ('post-answer scope summary, not a remedy's pending ledger record', 'Do not invent a pre-answer record afterward', 'Scope Challenge B also uses its own selectors'): answers saved before applying -> kept free: field template order, /a failed save or read blocks advancement/i, '**not persisted**', /these selections approve no engineering remedy/i. - Decision-procedure Read-choreography anchors ('use Read to fetch the entire saved record', 'Read the entire resolution block, including State') and the bold '**STOP until the actual answer arrives.**' anchor (t02 #5/#6, s04 F18): ask once, wait, record before the next choice -> kept free: Compare < Pending-record checkpoint < Send once < AskUserQuestion call < Record < next choice, /stop until the actual answer arrives/i located between Send and Record, /do not apply a remedy ... while the choice awaits an answer/i, /approves no implementation/i; AUQ completeness and kind-note strings kept exact (plan-tune reads them). - 'An Investigate/Defer option must bound the investigation', 'Apply the preamble's Completeness scores or kind-note accordingly': judge only (gate). - Research/outside-voice sentences ('If Aside is unavailable, use host WebSearch', 'Record the actual coverage...'): unavailable research is recorded, not hidden -> kept free: exact 'Search unavailable' output string, 'Outside voice: recorded provider, completed / unavailable / disabled / skipped (reason)' field, /only completed reviews enter cross-model tension/i, TODO < Approval readiness < Required outputs order. - Recovery sentences: kept as /i meaning checks (ExitPlanMode/telemetry wait, do not resend, stop before dependent output, no chat-only success, skip forbidden writes); 'If no recovery is specified or it fails' full sentence -> kept free: 'follow **Blocked outcome**' routing; OUTCOME=error kept. - Late-change sentences ('Refresh affected tests, tasks, dependencies and parallelization', 'Unchanged saved outputs may reuse their successful Review Log'): rerun affected outputs before navigation -> kept free: order of return/Decision procedure/Approval readiness/Required outputs/navigation and the stale-evidence Blocked outcome route; judge. - Final sequence anchor 'Run Learning hooks, including gated Brain Calibration Write-Back; then return to the entrypoint's Section self-check' -> kept free: 'Section self-check' as the last ordered anchor. - Coverage-audit sentences ('For each existing or proposed component in the selected target', 'the selected target. For each existing or proposed feature', 'dedicated tool call before drawing the diagram' (s06 L9 choreography)): plan diagrams cover proposed paths, ship covers changed code -> kept free: /existing or proposed (component|function\/method)/, /future paths remain proposals/i, the four 'Every ...' obligations, /no skipping regression coverage/i, ship has no 'existing or proposed'; gate E2E plan-eng-coverage-audit. - Brain write-back ordered prose list ('reserved default-off gate', 'this runtime does not set it', 'not an instruction to write now', t02 #9 / W7a s04 F15): no gbrain write without the default-off gate and trust policy -> kept free: 'Skip unless `BRAIN_CALIBRATION_WRITEBACK` is set' precedes both write tools, eng prefix says /skip this section/i and /do not enable it or infer permission/i, trust-policy key, /if unknown, skip/i, no env interpolation, other skills carry no eng-only skip prefix. Mutation pass (scratch worktree; plan-eng-review templates and scripts/resolvers/gbrain.ts): 21/21 as expected; 14 reorder/marker/stop/ approval/consent/numeric mutations go red, 7 rewordings of dropped sentences and lowered-register STOP lines stay green. Log: .context/mutation-eng-review-routing.md. --- test/eng-review-routing.test.ts | 158 +++++++++++++------------------- 1 file changed, 65 insertions(+), 93 deletions(-) diff --git a/test/eng-review-routing.test.ts b/test/eng-review-routing.test.ts index 60a89e174..b31dd4509 100644 --- a/test/eng-review-routing.test.ts +++ b/test/eng-review-routing.test.ts @@ -36,181 +36,153 @@ describe('engineering review routing contracts', () => { const excerpt = between(entry, ENG_REVIEW_EXCERPT.startMarker, ENG_REVIEW_EXCERPT.endMarker); ordered(excerpt, ['{{SECTION:review-sections}}', '## Recovery routing', '**Paused question:**', '**Repairable write/read failure:**', '**Late change or missing work:**', '**Blocked outcome:**']); - expect(compact(entry.slice(entry.indexOf(ENG_REVIEW_EXCERPT.endMarker)))).toContain('use Recovery routing above'); + expect(compact(entry.slice(entry.indexOf(ENG_REVIEW_EXCERPT.endMarker)))).toMatch(/use Recovery routing above/i); }); test('preparation establishes permission and evidence before applying review rules', () => { - const preparation = between(section, '## Review preparation', '## Review record and write policy'); - expect(compact(preparation)).toContain('Follow the blocks below in order after startup'); - expect(compact(preparation)).toContain('Confidence Calibration and Decision procedure are reference rules, not additional review passes'); - ordered(section, ['## Review record and write policy', '{{LEARNINGS_SEARCH}}', + ordered(section, ['## Review preparation', '## Review record and write policy', '{{LEARNINGS_SEARCH}}', '## Retrospective learning', '{{CONFIDENCE_CALIBRATION}}', '## Decision procedure', '## Scope Challenge', '### A. Assess the target', '### B. Resolve complexity selectors', '### C. Resolve findings', '### 1. Architecture review']); - expect(entry).toContain('Keep the reviewed target fixed'); + expect(entry).toMatch(/keep the reviewed target fixed/i); }); - test('compression cannot remove mandatory review stages or decision content', () => { + test('compression cannot remove mandatory review stages', () => { const priority = between(entry, '## Priority hierarchy', '## My engineering preferences'); - expect(compact(priority)).toContain('Complete every required stage, decision gate and output'); - expect(compact(priority)).toContain('Shorten only optional commentary'); - expect(compact(priority)).toContain('never Scope Challenge, Sections 1–4, the test diagram or required decision/report content'); + expect(compact(priority)).toMatch(/complete every required stage, decision gate and output/i); expect(priority).not.toContain('Everything else'); - expect(compact(section)).toContain('Never condense, abbreviate or skip a section'); }); - test('Scope Challenge has one named route and completes all seven assessments first', () => { + test('Scope Challenge has one named route and completes its assessments before writing', () => { expect([...scope.matchAll(/^### (.+)$/gm)].map(match => match[1])).toEqual([ 'A. Assess the target', 'B. Resolve complexity selectors', 'C. Resolve findings', ]); - expect([...assessment.matchAll(/^- \*\*([^*]+)\*\*/gm)].map(match => match[1])).toEqual([ + expect([...assessment.matchAll(/^- \*\*([^*]+)\*\*/gm)].map(match => match[1])).toEqual(expect.arrayContaining([ 'What already solves each sub-problem?', 'What minimum changes achieve the goal?', 'Complexity check:', 'Search check:', 'TODOS cross-reference:', 'Completeness check:', 'Distribution check:', - ]); - expect(compact(assessment)).toContain('Complete these checks before the complexity decision in B'); - expect(compact(assessment)).toContain('Do not apply scope changes or write findings into the plan yet'); + ])); + expect(compact(assessment)).toMatch(/do not apply scope changes or write findings into the plan yet/i); expect(scope).not.toContain('Below the threshold, start at step 1'); }); test('below-threshold route skips selectors, never findings or remedy approvals', () => { - expect(compact(complexity)).toContain("With fewer than 8 files AND fewer than 2 new classes/services, skip B's questions and go directly to **C. Resolve findings**"); - expect(findings).toContain('Run C whether B was completed or skipped'); - ordered(compact(findings), ['1. Present numbered Scope Challenge findings', - '2. Resolve each remedy through Decision procedure', - '3. Report accepted/rejected/deferred/pending dispositions', + expect(compact(complexity)).toMatch(/fewer than 8 files AND fewer than 2 new classes\/services, skip B's questions/i); + expect(findings).toMatch(/run C whether B was completed or skipped/i); + ordered(compact(findings), ['1. Present', '2. Resolve each remedy through Decision procedure', '3. Report', 'Continue to Section 1 only when no answer is pending']); - expect(findings).toContain('Findings and scope answers approve no remedies'); - expect(findings).toContain('"No issues found" for an empty list'); + expect(findings).toMatch(/approve no remedies/i); + expect(findings).toContain('"No issues found"'); }); - test('high complexity retains separate cuts and mandatory arrangement choice', () => { - expect(compact(complexity)).toContain('At 8+ files or 2+ new classes/services, STOP before Section 1'); - ordered(compact(complexity), ['Initial scope selectors need no grid or **pre-answer** ledger write', - '1. Explain the complexity', 'Ask each proposed feature cut/deferral separately', - '2. Always ask the structure question when this gate trips, even with no cuts', - '3. Save the actual feature and structure answers as one scope record']); - expect(compact(complexity)).toContain('With no proposed cuts, keep the feature list and go directly to the structure question'); - expect(compact(complexity)).toContain('Both retain the same approved feature list, contracts and approved security/error/test/performance fixes'); + test('high complexity stops before Section 1 and asks cuts and structure separately', () => { + expect(compact(complexity)).toMatch(/at 8\+ files or 2\+ new classes\/services, stop before section 1/i); + ordered(compact(complexity), ['1. Explain the complexity', 'Ask each proposed feature cut/deferral separately', + '2. Always ask the structure question', '3. Save the actual feature and structure answers']); expect(compact(complexity)).toContain('Pending remedies not decided here: '); }); test('no safe smaller arrangement does not authorize scope cuts or bypass the pause', () => { ordered(compact(complexity), ['If no smaller arrangement preserves these commitments', - 'offer confirmation of the original arrangement or a pause to investigate a smaller one', - 'A pause leaves the arrangement undecided', 'then return to this structure selector', - 'Do not continue to C until it is settled']); - expect(compact(complexity)).toContain('investigate only the agreed question'); + 'A pause leaves the arrangement undecided', 'Do not continue to C until it is settled']); + expect(compact(complexity)).toMatch(/do not continue to C until it is settled/i); + expect(compact(complexity)).toMatch(/investigate only the agreed question/i); }); - test('selector answers are verified after answering without invented pending records', () => { + test('selector answers are saved and verified before scope changes apply', () => { const summary = compact(complexity.slice(complexity.indexOf('3. Save the actual'))); ordered(summary, ['feature answers: ; structure: ; accepted scope: ; pending remedies: ', - 'post-answer scope summary, not a remedy\'s pending ledger record', 'Read it back against the actual answers', - 'After verification, apply only accepted scope changes', 'Continue to **C. Resolve findings**']); - expect(summary).toContain('Do not invent a pre-answer record afterward'); - expect(summary).toContain('A failed save or Read blocks advancement'); - expect(summary).toContain('on the permitted read-only route, present and verify it as **not persisted**'); - expect(compact(section)).toContain('Scope Challenge B also uses its own selectors and post-answer scope record'); - expect(compact(section)).toContain('These selections approve no engineering remedy'); + 'Read it back', 'apply only accepted scope changes', 'Continue to **C. Resolve findings**']); + expect(summary).toMatch(/a failed save or read blocks advancement/i); + expect(summary).toContain('**not persisted**'); + expect(compact(section)).toMatch(/these selections approve no engineering remedy/i); }); - test('engineering remedies still require full save Read ask answer apply Read ordering', () => { + test('engineering remedies are saved, asked once, and recorded before the next choice', () => { const procedure = between(section, '## Decision procedure', '## Scope Challenge'); - ordered(procedure, ['**Compare one choice.**', '**Pending-record checkpoint.**', - 'use Read to fetch the entire saved record', '### Send once and wait', - 'AskUserQuestion({ questions: [currentDecision] })', '**STOP until the actual answer arrives.**', - '### Record the answer', 'Read the entire resolution block, including State', - 'For the next choice, use the updated working plan and answer']); - expect(compact(procedure)).toContain('An Investigate/Defer option must bound the investigation'); - expect(compact(procedure)).toContain('It approves no implementation, including a conditional fix'); - expect(compact(procedure)).toContain('Do not apply a remedy, make another call, start the next section or call ExitPlanMode while the choice awaits an answer'); - expect(compact(procedure)).toContain("Apply the preamble's Completeness scores or kind-note accordingly"); + ordered(procedure, ['**Compare one choice.**', '**Pending-record checkpoint.**', '### Send once and wait', + 'AskUserQuestion({ questions: [currentDecision] })', '### Record the answer', 'For the next choice']); + expect(between(procedure, '### Send once and wait', '### Record the answer')).toMatch(/stop until the actual answer arrives/i); + expect(compact(procedure)).toMatch(/approves no implementation, including a conditional fix/i); + expect(compact(procedure)).toMatch(/do not apply a remedy[^.]*call ExitPlanMode while the choice awaits an answer/i); const questions = generateAskUserFormat({ skillName: 'plan-eng-review', host: 'claude', paths: HOST_PATHS.claude } as TemplateContext); expect(questions).toContain('10 = complete, 7 = happy path, 3 = shortcut'); expect(questions).toContain('Note: options differ in kind, not coverage — no completeness score.'); }); test('unavailable research preserves an explicit coverage limit and continues review', () => { - expect(compact(assessment)).toContain('If Aside is unavailable, use host WebSearch for these queries'); - expect(compact(assessment)).toContain('With neither, skip and note: "Search unavailable — proceeding with in-distribution knowledge only."'); + expect(compact(assessment)).toContain('"Search unavailable — proceeding with in-distribution knowledge only."'); const outside = between(section, '### Continue after Outside Voice', '### TODOS.md updates'); - expect(compact(outside)).toContain('Only completed reviews enter Cross-model tension'); - expect(compact(outside)).toContain('Record the actual coverage, including disabled or unavailable outcomes'); + expect(compact(outside)).toMatch(/only completed reviews enter cross-model tension/i); expect(section).toContain('Outside voice: recorded provider, completed / unavailable / disabled / skipped (reason)'); - expect(compact(outside)).toContain('resolve the TODO choices, then check Approval readiness before Required outputs'); + ordered(compact(outside), ['TODO choices', 'Approval readiness', 'Required outputs']); }); test('paused transport and failed persistence have distinct non-success outcomes', () => { const pause = between(recovery, '**Paused question:**', '**Repairable write/read failure:**'); - expect(pause).toContain('without completion telemetry or ExitPlanMode'); - expect(compact(pause)).toContain('that the user may have seen is still pending; do not resend it'); - const failure = between(recovery, '**Repairable write/read failure:**', '**Late change or missing work:**'); - expect(compact(failure)).toContain('Stop before the dependent question or output'); - expect(compact(failure)).toContain('If no recovery is specified or it fails, follow **Blocked outcome**'); - expect(compact(failure)).toContain('Never turn a failed permitted save into a chat-only success'); - const policy = between(section, '## Review record and write policy', '{{LEARNINGS_SEARCH}}'); - expect(compact(policy)).toContain('use **Recovery routing → Repairable write/read failure**, never the routes above'); - expect(compact(policy)).toContain('Skip forbidden writes; otherwise keep their best-effort behavior'); + expect(pause).toMatch(/without completion telemetry or ExitPlanMode/i); + expect(compact(pause)).toMatch(/still pending; do not resend it/i); + const failure = compact(between(recovery, '**Repairable write/read failure:**', '**Late change or missing work:**')); + expect(failure).toMatch(/stop before the dependent question or output/i); + expect(failure).toContain('follow **Blocked outcome**'); + expect(failure).toMatch(/never turn a failed permitted save into a chat-only success/i); + const policy = compact(between(section, '## Review record and write policy', '{{LEARNINGS_SEARCH}}')); + expect(policy).toContain('**Recovery routing → Repairable write/read failure**'); + expect(policy).toMatch(/skip forbidden writes/i); expect(recovery).toContain('`OUTCOME=error`'); expect(recovery).not.toContain('`OUTCOME=success`'); }); test('late changes rerun affected approvals and outputs before another navigation answer', () => { const late = compact(between(recovery, '**Late change or missing work:**', '**Blocked outcome:**')); - ordered(late, ['Return to the affected review stage', 'new or reopened choices use Decision procedure', + ordered(late, ['Return to the affected review stage', 'Decision procedure', 'Repeat Approval readiness', 'Required outputs steps 1–4', 'before choosing navigation again']); - expect(late).toContain('Refresh affected tests, tasks, dependencies and parallelization'); - expect(late).toContain('Unchanged saved outputs may reuse their successful Review Log'); - expect(late).toContain('If a final gate discovers stale evidence, follow **Blocked outcome** first'); + expect(late).toMatch(/stale evidence, follow \*\*Blocked outcome\*\* first/i); const finish = section.slice(section.indexOf('## Required outputs')); - expect(compact(finish)).toContain('A substantive change follows **Recovery routing → Late change or missing work** before navigation resumes'); - expect(compact(finish)).toContain('A next-step answer approves no implementation change'); + expect(compact(finish)).toContain('**Recovery routing → Late change or missing work** before navigation resumes'); + expect(compact(finish)).toMatch(/a next-step answer approves no implementation change/i); ordered(finish, ['{{TASKS_SECTION_EMIT:eng-review}}', '### Completion summary', '{{PLAN_FILE_REVIEW_REPORT}}', '## Review Log', '{{REVIEW_DASHBOARD}}', '## Next Steps — Review Chaining', '## Learning hooks', '{{BRAIN_WRITE_BACK}}']); const sequence = compact(finish.slice(0, finish.indexOf('### Output reference'))); ordered(sequence, ['1. **Prepare the review body.**', '2. **Save and Read back.**', '3. **Log the saved review.**', - '4. **Publish.**', '5. **Choose navigation.**', '6. **Finish.**', - "Run Learning hooks, including gated Brain Calibration Write-Back; then return to the entrypoint's Section self-check"]); + '4. **Publish.**', '5. **Choose navigation.**', '6. **Finish.**', 'Section self-check']); }); test('plan test diagrams cover proposed paths without inventing existing implementation', () => { for (const host of ALL_HOST_CONFIGS) { const ctx = { skillName: 'plan-eng-review', host: host.name, paths: HOST_PATHS[host.name] } as TemplateContext; const audit = generateTestCoverageAuditPlan(ctx); - expect(audit).toContain('For each existing or proposed component in the selected target'); - expect(audit).toContain('Every existing or proposed function/method in scope'); - expect(audit).toContain('the selected target. For each existing or proposed feature'); - expect(audit).toContain('Future paths remain proposals, not runnable code'); - for (const obligation of ['Every conditional branch', 'Every error path', 'Every call to another function', - 'Every edge:', 'dedicated tool call before drawing the diagram', 'No skipping regression coverage']) { + expect(audit).toMatch(/existing or proposed component/); + expect(audit).toMatch(/existing or proposed function\/method/); + expect(audit).toMatch(/future paths remain proposals, not runnable code/i); + for (const obligation of ['Every conditional branch', 'Every error path', 'Every call to another function', 'Every edge:']) { expect(audit).toContain(obligation); } + expect(audit).toMatch(/no skipping regression coverage/i); const ship = generateTestCoverageAuditShip({ ...ctx, skillName: 'ship' }); - expect(ship).toContain('For each changed file, draw an ASCII diagram showing:'); - expect(ship).toContain('Every function/method that was added or modified'); - expect(ship).toContain('the changed code. For each changed feature'); + expect(ship).toMatch(/for each changed file, draw an ASCII diagram/i); + expect(ship).toMatch(/function\/method that was added or modified/); expect(ship).not.toContain('existing or proposed'); } }); - test('reserved calibration gate is explicitly skipped without enabling a write path', () => { + test('calibration write-back stays gated: no write instruction runs without the default-off gate', () => { for (const host of ALL_HOST_CONFIGS) { const ctx = { skillName: 'plan-eng-review', host: host.name, paths: HOST_PATHS[host.name] } as TemplateContext; const output = generateBrainWriteBack(ctx); - ordered(output, ['reserved default-off gate', 'this runtime does not set it', - 'Skip this section and continue the finish sequence', 'Do not enable it or infer permission from brain availability', - 'not an instruction to write now', 'Skip unless']); + const gate = output.indexOf('Skip unless `BRAIN_CALIBRATION_WRITEBACK` is set'); + expect(gate).toBeGreaterThan(0); + expect(output.indexOf('mcp__gbrain__takes_add')).toBeGreaterThan(gate); + expect(output.indexOf('mcp__gbrain__put_page')).toBeGreaterThan(gate); + expect(output.slice(0, gate)).toMatch(/skip this section/i); + expect(output.slice(0, gate)).toMatch(/do not enable it or infer permission/i); expect(output).toContain('brain_trust_policy@=personal'); - expect(output).toContain('If unknown, skip'); - expect(output).toContain('mcp__gbrain__takes_add'); - expect(output).toContain('mcp__gbrain__put_page'); + expect(output).toMatch(/if unknown, skip/i); expect(output).toContain('source_skill: plan-eng-review'); expect(output).not.toContain('${BRAIN_CALIBRATION_WRITEBACK'); for (const skillName of ['office-hours', 'plan-ceo-review', 'plan-design-review', 'plan-devex-review']) { const other = generateBrainWriteBack({ ...ctx, skillName }); expect(other).toStartWith('## Brain Calibration Write-Back (gated)\n\nSkip unless'); - expect(other).not.toContain('reserved default-off gate'); - expect(other).not.toContain('Skip this section and continue the finish sequence'); + expect(other).not.toMatch(/skip this section/i); } } });