From d80d3c3ed726cf30af6d70a2e9116271426470c6 Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Tue, 25 Aug 2026 02:38:47 +0000 Subject: [PATCH] =?UTF-8?q?fix(test):=20review=20findings=20=E2=80=94=20Wi?= =?UTF-8?q?ndows=20path=20normalization,=20full=20totals=20rebuild,=20ratc?= =?UTF-8?q?het=20coverage?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pre-landing review (5 specialists) found one critical: the ratchet test runs in the curated Windows lane, where path.relative yields backslash skill names that miss the test/ filter and mismatch every POSIX fixture key. Names are now normalized once in buildRatchetBill (toPosixName) and the fixture filter is tightened to test/fixtures/. All eight Bill.totals fields are rebuilt from the filtered list (no fixture-polluted perInvocation/totalMd numbers for future consumers). New coverage: Windows-separator normalization pins, a captureContextBudget round-trip against tree-a (headroom math exact), a stripFields regression pin (interactive/benefits-from absent from renders, hooks/gbrain preserved), and the ceilings test no longer double-reports stale-fixture entries. Co-Authored-By: Claude Fable 5 --- test/context-budget-ratchet.test.ts | 37 +++++++++++++++++++++++++- test/gen-skill-docs.test.ts | 23 ++++++++++++++++ test/helpers/capture-context-budget.ts | 30 ++++++++++++++++++--- 3 files changed, 85 insertions(+), 5 deletions(-) diff --git a/test/context-budget-ratchet.test.ts b/test/context-budget-ratchet.test.ts index 531e9f243..89133a257 100644 --- a/test/context-budget-ratchet.test.ts +++ b/test/context-budget-ratchet.test.ts @@ -26,10 +26,16 @@ */ import { describe, test, expect } from 'bun:test'; import * as fs from 'fs'; +import * as path from 'path'; import { checkBudget } from '../lib/context-bill'; import { buildRatchetBill, + captureContextBudget, + isFixtureSkill, + toPosixName, BUDGET_FIXTURE_PATH, + ALWAYS_ON_HEADROOM, + EAGER_HEADROOM, type ContextBudget, } from './helpers/capture-context-budget'; @@ -43,10 +49,13 @@ const bill = buildRatchetBill(); describe('context-budget ratchet', () => { test('always-on + eager ledgers stay under the fixture ceilings', () => { + // actual === null means "fixture names a skill missing from the tree" — + // the dedicated stale-fixture test below owns that case with a clearer + // message; filtering here keeps one failure from producing two reports. const violations = checkBudget(bill, { alwaysOnTotal: budget.alwaysOnTotal, eagerPerInvocation: budget.eagerPerInvocation, - }); + }).filter((v) => v.actual !== null); const detail = violations .map((v) => ` ${v.ceiling}: ${v.actual} tok > limit ${v.limit}\n ${v.files.join('\n ')}`) .join('\n'); @@ -75,4 +84,30 @@ describe('context-budget ratchet', () => { `Fixture carries ceilings for removed skills: ${stale.join(', ')}. Re-run the capture.`, ).toEqual([]); }); + + // Windows lane: skill names arrive backslash-separated from path.relative; + // the normalization must make the filter and the POSIX fixture keys agree. + test('name normalization handles Windows separators', () => { + expect(toPosixName(['test', 'fixtures', 'context-bill', 'tree-a', 'alpha'].join(path.sep))).toBe( + 'test/fixtures/context-bill/tree-a/alpha', + ); + expect(isFixtureSkill(['test', 'fixtures', 'x'].join(path.sep))).toBe(true); + expect(isFixtureSkill('test/fixtures/context-bill/tree-a/alpha')).toBe(true); + expect(isFixtureSkill('openclaw/skills/gstack-openclaw-retro')).toBe(false); + expect(bill.skills.every((s) => !s.name.includes('\\'))).toBe(true); + }); + + // Round-trip: a fresh capture must pass its own ratchet, and the headroom + // math must be exactly ceil(actual x headroom) — the recovery protocol is + // "re-run the capture", so a corrupt write side poisons every future fixture. + test('captureContextBudget round-trips against its own bill', () => { + const TREE_A = path.join(import.meta.dir, 'fixtures', 'context-bill', 'tree-a'); + const capture = captureContextBudget(TREE_A); + const treeBill = buildRatchetBill(TREE_A); + expect(checkBudget(treeBill, capture)).toEqual([]); + for (const s of treeBill.skills) { + expect(capture.eagerPerInvocation[s.name]).toBe(Math.ceil(s.eagerTokens * EAGER_HEADROOM)); + } + expect(capture.alwaysOnTotal).toBe(Math.ceil(treeBill.totals.alwaysOnTokens * ALWAYS_ON_HEADROOM)); + }); }); diff --git a/test/gen-skill-docs.test.ts b/test/gen-skill-docs.test.ts index 67aa46b18..dab152675 100644 --- a/test/gen-skill-docs.test.ts +++ b/test/gen-skill-docs.test.ts @@ -3295,6 +3295,29 @@ describe('voice-triggers processing', () => { const frontmatter = content.slice(0, fmEnd); expect(frontmatter).not.toContain('voice-triggers:'); }); + + // Gen-time-only keys: interactive + benefits-from are read from the .tmpl by + // buildContext; the generated copy has no reader (the host reads name/ + // description/allowed-tools/hooks; gbrain: is runtime-read and NOT stripped). + // Pin the strip so a stripFields refactor can't silently re-add the always-on + // frontmatter weight — mirrors the voice-triggers pins above. + test('generated SKILL.md strips gen-time-only keys the .tmpl still declares', () => { + const tmpl = fs.readFileSync(path.join(ROOT, 'plan-ceo-review', 'SKILL.md.tmpl'), 'utf-8'); + const tmplFm = tmpl.slice(0, tmpl.indexOf('\n---', 4)); + expect(tmplFm).toContain('interactive:'); + expect(tmplFm).toContain('benefits-from:'); + + const generated = fs.readFileSync(path.join(ROOT, 'plan-ceo-review', 'SKILL.md'), 'utf-8'); + const genFm = generated.slice(0, generated.indexOf('\n---', 4)); + expect(genFm).not.toContain('interactive:'); + expect(genFm).not.toContain('benefits-from:'); + + // The runtime-read and host-read keys survive the strip. + const investigate = fs.readFileSync(path.join(ROOT, 'investigate', 'SKILL.md'), 'utf-8'); + const invFm = investigate.slice(0, investigate.indexOf('\n---', 4)); + expect(invFm).toContain('hooks:'); + expect(invFm).toContain('gbrain:'); + }); }); describe('plan-mode-info resolver (handshake-replacement)', () => { diff --git a/test/helpers/capture-context-budget.ts b/test/helpers/capture-context-budget.ts index 19aaef866..c6519443b 100644 --- a/test/helpers/capture-context-budget.ts +++ b/test/helpers/capture-context-budget.ts @@ -35,9 +35,19 @@ export const BUDGET_FIXTURE_PATH = path.join(REPO_ROOT, 'test', 'fixtures', 'con export const ALWAYS_ON_HEADROOM = 1.05; export const EAGER_HEADROOM = 1.10; +/** + * Skill names come from path.relative in buildBill, which yields backslash + * separators on Windows. The fixture keys are POSIX. Normalize once here so + * the filter, the fixture keys, and checkBudget's name matching agree on + * every platform (the ratchet test runs in the curated Windows lane). + */ +export function toPosixName(name: string): string { + return name.split(path.sep).join('/'); +} + /** Skills that exist only as context-bill test data — never budgeted. */ export function isFixtureSkill(name: string): boolean { - return name.startsWith('test/'); + return toPosixName(name).startsWith('test/fixtures/'); } export interface ContextBudget { @@ -46,20 +56,32 @@ export interface ContextBudget { eagerPerInvocation: Record; } -/** The bill the ratchet grades: repo tree minus test-fixture skill dirs. */ +/** + * The bill the ratchet grades: repo tree minus test-fixture skill dirs, with + * POSIX-normalized names and ALL totals rebuilt from the filtered list (a + * partially-updated totals object would hand fixture-polluted numbers to any + * future consumer of the perInvocation/totalMd fields). + */ export function buildRatchetBill(root: string = REPO_ROOT): Bill { const bill = buildBill(root); - const skills = bill.skills.filter((s) => !isFixtureSkill(s.name)); + const skills = bill.skills + .map((s) => ({ ...s, name: toPosixName(s.name) })) + .filter((s) => !isFixtureSkill(s.name)); return { ...bill, skills, totals: { - ...bill.totals, skillCount: skills.length, alwaysOnBytes: skills.reduce((n, s) => n + s.frontmatterBytes, 0), alwaysOnTokens: skills.reduce((n, s) => n + s.frontmatterTokens, 0), eagerBytesBySkill: Object.fromEntries(skills.map((s) => [s.name, s.eagerBytes])), eagerTokensBySkill: Object.fromEntries(skills.map((s) => [s.name, Math.round(s.eagerTokens)])), + perInvocationBytesBySkill: Object.fromEntries(skills.map((s) => [s.name, s.perInvocationBytes])), + perInvocationTokensBySkill: Object.fromEntries( + skills.map((s) => [s.name, Math.round(s.perInvocationTokens)]), + ), + totalMdBytes: skills.reduce((n, s) => n + s.totalMdBytes, 0), + totalMdTokens: skills.reduce((n, s) => n + s.totalMdTokens, 0), }, }; }