From c11e44b93af9ff384ff8697c37dd7246e9af4b44 Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Tue, 25 Aug 2026 02:53:25 +0000 Subject: [PATCH] =?UTF-8?q?fix(test):=20adversarial=20findings=20=E2=80=94?= =?UTF-8?q?=20stable=20root=20key,=20symlink-alias=20dedupe,=20fixture-sha?= =?UTF-8?q?pe=20guard?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adversarial review (Claude subagent) verified the fixture's root-skill key was the capture machine's checkout dirname: any non-gstack-named clone (every Conductor worktree) failed the free suite, and the documented re-run-the-capture recovery baked the local dirname into the committed fixture — silent corruption through the tool's own protocol. The root skill is now pinned to ROOT_SKILL_KEY ('gstack', its frontmatter name). Symlink aliases are realpath-deduped (census precedent): connect-chrome no longer gets its own ceiling, so Windows checkouts that materialize the symlink as a plain file can't fail the stale-ceiling set-equality test. New guards: fixture-shape validation (a string alwaysOnTotal can no longer silently disable the ceiling), a mutation pin that the filter shrinks the always-on ledger vs the raw bill, an alwaysOnTotal violation test (the branch was load-bearing with only under-budget coverage), and an atomic temp+rename fixture write. Fixture regenerated: 59 ceilings, alwaysOnTotal 6344. Deferred with a TODO: anchoring transformFrontmatter's denylist strip to the frontmatter block (latent, zero live collisions, pre-existing path). Co-Authored-By: Claude Fable 5 --- TODOS.md | 26 ++++++++++++++ test/context-bill.test.ts | 12 +++++++ test/context-budget-ratchet.test.ts | 24 ++++++++++++- test/fixtures/context-budget.json | 7 ++-- test/helpers/capture-context-budget.ts | 50 ++++++++++++++++++++++---- 5 files changed, 107 insertions(+), 12 deletions(-) diff --git a/TODOS.md b/TODOS.md index 7dd066d25..0c3936069 100644 --- a/TODOS.md +++ b/TODOS.md @@ -691,6 +691,32 @@ or asserts dashboard/report text, then decide skeleton-vs-section placement per **Depends on / blocked by:** Token-reduction program Phases 1-4 landing (carve machinery churn would conflict). +### P3: Anchor transformFrontmatter's denylist strip to the frontmatter block + +**What:** `transformFrontmatter` (scripts/gen-skill-docs.ts:525-530, denylist branch) +deletes the FIRST line matching `^:` anywhere in the file, not just inside +the frontmatter block, and would orphan continuation lines of a block-style YAML +value. Slice the frontmatter, strip within it, reassemble. + +**Why:** Latent mis-strip class: a skill body line beginning `interactive:` or +`benefits-from:` (e.g. a skill documenting the frontmatter contract) would be +silently deleted from the render. Zero live collisions today (verified across all +tracked SKILL.md bodies during the v1.69.x token-reduction Phase 0 review), but +each new stripFields entry widens the exposure. + +**Pros:** Kills the whole latent class; makes stripFields safe to grow. +**Cons:** Touches the generator hot path — needs a full regen + the per-host +golden fixtures re-checked; deserves its own small PR, not a rider. + +**Context:** Found by the Phase 0 adversarial review on branch +`prompt-token-load-reduction` (finding ADV4). The gen-side parser reads only +inline `[...]` array form (gen-skill-docs.ts:751), so block-form YAML for these +keys fails silently twice — worth a validation error at the same time. + +**Effort estimate:** S (human team) → S (CC+gstack) +**Priority:** P3 +**Depends on / blocked by:** none. + ### P3: Revisit plan-ceo-review doctrine carve after the preamble program lands **What:** Re-evaluate carving plan-ceo-review's ~13KB of always-loaded doctrine diff --git a/test/context-bill.test.ts b/test/context-bill.test.ts index 4c44279a3..0ca72e7ac 100644 --- a/test/context-bill.test.ts +++ b/test/context-bill.test.ts @@ -419,6 +419,18 @@ describe("--budget", () => { expect(violations).toHaveLength(1); expect(violations[0].ceiling).toBe("eagerPerInvocation.ghost"); }); + + // The context-budget ratchet (test/context-budget-ratchet.test.ts) made + // this branch load-bearing in CI; it previously had only under-budget + // coverage. + it("checkBudget flags an alwaysOnTotal violation with every skill's frontmatter listed", () => { + const bill = buildBill(TREE_A); + const violations = checkBudget(bill, { alwaysOnTotal: 0 }); + expect(violations).toHaveLength(1); + expect(violations[0].ceiling).toBe("alwaysOnTotal"); + expect(violations[0].actual).toBe(Math.round(bill.totals.alwaysOnTokens)); + expect(violations[0].files.length).toBe(bill.skills.length); + }); }); describe("--exact (opt-in measurement; offline here via an injected fetch)", () => { diff --git a/test/context-budget-ratchet.test.ts b/test/context-budget-ratchet.test.ts index 89133a257..4ef193913 100644 --- a/test/context-budget-ratchet.test.ts +++ b/test/context-budget-ratchet.test.ts @@ -27,7 +27,7 @@ import { describe, test, expect } from 'bun:test'; import * as fs from 'fs'; import * as path from 'path'; -import { checkBudget } from '../lib/context-bill'; +import { buildBill, checkBudget } from '../lib/context-bill'; import { buildRatchetBill, captureContextBudget, @@ -48,6 +48,28 @@ const budget: ContextBudget = JSON.parse(fs.readFileSync(BUDGET_FIXTURE_PATH, 'u const bill = buildRatchetBill(); describe('context-budget ratchet', () => { + // checkBudget only enforces alwaysOnTotal when it is typeof number — a + // string or missing value from a hand edit or bad merge would silently + // turn the always-on ceiling OFF while every test stays green. Validate + // the fixture shape so the guard cannot be disabled by a typo. + test('fixture shape is valid (a malformed fixture must not silently disable ceilings)', () => { + expect(typeof budget.alwaysOnTotal).toBe('number'); + expect(Number.isFinite(budget.alwaysOnTotal)).toBe(true); + const bad = Object.entries(budget.eagerPerInvocation).filter( + ([, v]) => typeof v !== 'number' || !Number.isFinite(v), + ); + expect(bad, `Non-numeric ceilings: ${bad.map(([k]) => k).join(', ')}. Re-run the capture.`).toEqual([]); + }); + + // Mutation pin: the fixture-skill filter must actually shrink the + // always-on sum vs the raw bill (deleting the totals recompute would leak + // fixture tokens under the headroom and never fail a ceiling). + test('filtering fixture skills shrinks the always-on ledger vs the raw bill', () => { + const raw = buildBill(path.join(import.meta.dir, '..')); + expect(bill.skills.some((s) => s.name.startsWith('test/'))).toBe(false); + expect(bill.totals.skillCount).toBeLessThan(raw.totals.skillCount); + expect(bill.totals.alwaysOnTokens).toBeLessThan(raw.totals.alwaysOnTokens); + }); 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 diff --git a/test/fixtures/context-budget.json b/test/fixtures/context-budget.json index 5c41212e3..d20fda828 100644 --- a/test/fixtures/context-budget.json +++ b/test/fixtures/context-budget.json @@ -1,6 +1,6 @@ { "_comment": "Context-budget ratchet ceilings (~tokens). Regenerate: bun test/helpers/capture-context-budget.ts. Headroom: alwaysOnTotal x1.05, eagerPerInvocation x1.1. Graded by test/context-budget-ratchet.test.ts via lib/context-bill.ts checkBudget.", - "alwaysOnTotal": 6423, + "alwaysOnTotal": 6344, "eagerPerInvocation": { "autoplan": 27445, "benchmark": 10114, @@ -10,7 +10,6 @@ "canary": 15492, "careful": 919, "codex": 26760, - "connect-chrome": 9687, "context-restore": 14506, "context-save": 15123, "cso": 20096, @@ -33,7 +32,7 @@ "ios-fix": 13835, "ios-qa": 16132, "ios-sync": 14005, - "land-and-deploy": 28990, + "land-and-deploy": 29370, "landing-report": 14416, "learn": 14086, "make-pdf": 10214, @@ -56,7 +55,7 @@ "scrape": 9196, "setup-browser-cookies": 8377, "setup-deploy": 15246, - "setup-gbrain": 25164, + "setup-gbrain": 25250, "ship": 23847, "skillify": 17092, "spec": 22662, diff --git a/test/helpers/capture-context-budget.ts b/test/helpers/capture-context-budget.ts index c6519443b..ed98d46cf 100644 --- a/test/helpers/capture-context-budget.ts +++ b/test/helpers/capture-context-budget.ts @@ -56,17 +56,50 @@ export interface ContextBudget { eagerPerInvocation: Record; } +/** + * The root SKILL.md's bill name falls back to the checkout directory's + * basename (path.relative gives '' at the root), which is machine-specific: + * a Conductor worktree named anything but "gstack" would mismatch the fixture + * key, and the documented "re-run the capture" recovery would then bake the + * local dirname INTO the committed fixture. Pin it to the skill's frontmatter + * name instead — stable across every clone. + */ +export const ROOT_SKILL_KEY = 'gstack'; + /** * 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). + * POSIX-normalized names, the root skill pinned to ROOT_SKILL_KEY, symlink + * aliases deduped by realpath (connect-chrome -> open-gstack-browser; on + * Windows checkouts the symlink materializes as a plain file and the alias + * dir vanishes, so budgeting it would make the stale-ceiling test + * platform-dependent — same dedupe the skill census uses), 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 - .map((s) => ({ ...s, name: toPosixName(s.name) })) + const candidates = bill.skills + .map((s) => ({ + ...s, + name: s.dir === bill.root ? ROOT_SKILL_KEY : toPosixName(s.name), + })) .filter((s) => !isFixtureSkill(s.name)); + // One ceiling per PHYSICAL skill: group by realpath, prefer the entry whose + // dir IS the realpath (the real dir) over symlink aliases. + const byReal = new Map(); + for (const s of candidates) { + let real: string; + try { + real = fs.realpathSync(s.dir); + } catch { + real = s.dir; + } + const cur = byReal.get(real); + if (!cur || (s.dir === real && cur.dir !== real)) byReal.set(real, s); + } + const kept = new Set(byReal.values()); + const skills = candidates.filter((s) => kept.has(s)); return { ...bill, skills, @@ -102,10 +135,13 @@ export function captureContextBudget(root: string = REPO_ROOT): ContextBudget { }; } -// CLI: write the fixture. +// CLI: write the fixture atomically (temp + rename) — an interrupted capture +// must never leave truncated JSON that breaks the suite at module load. if (import.meta.main) { const budget = captureContextBudget(); - fs.writeFileSync(BUDGET_FIXTURE_PATH, JSON.stringify(budget, null, 2) + '\n'); + const tmp = `${BUDGET_FIXTURE_PATH}.tmp-${process.pid}`; + fs.writeFileSync(tmp, JSON.stringify(budget, null, 2) + '\n'); + fs.renameSync(tmp, BUDGET_FIXTURE_PATH); const n = Object.keys(budget.eagerPerInvocation).length; console.log( `Wrote ${path.relative(REPO_ROOT, BUDGET_FIXTURE_PATH)}: alwaysOnTotal=${budget.alwaysOnTotal} tok, ${n} eager ceilings`,