From 775e94d47284d6563afdb174230a24085919e250 Mon Sep 17 00:00:00 2001 From: Griffen Fargo <3642037+gfargo@users.noreply.github.com> Date: Tue, 5 May 2026 20:46:29 -0400 Subject: [PATCH] fix(bench): dep-bump fixture should reflect post-ignore-filter content (#845) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The dep-bump fixture from #849 included `yarn.lock` and reported 27 seconds of LLM work for it. That's a bench-fixture artifact, not a real-world cost. Lockfiles live in `DEFAULT_IGNORED_FILES` and the `.lock` extension lives in `DEFAULT_IGNORED_EXTENSIONS` (see `src/lib/config/constants.ts`), so `getChanges` strips them before the diff-condensing pipeline ever sees them on a real `coco commit`. - Drop `yarn.lock` from DEP_BUMP_FILES; the realistic shape is just `package.json` + `CHANGELOG.md`. - Update the dep-bump-shape test to assert the post-filter invariant (no lockfiles in the fixture) instead of asserting a lockfile is present. - Add a guard test that fails loudly if any future fixture accidentally drifts back into including a default-ignored file. - Re-baseline. dep-bump now reports 0 ms / 0 LLM calls (early exit, total budget already under threshold), reflecting what a real dep bump costs the pipeline. The "lockfile fast-path" optimization angle from the original plan is dropped — the existing ignore filter already handles that, and any pipeline-level skip would be redundant. --- .bench/baseline.json | 34 +++++++------- .../default/__fixtures__/index.test.ts | 47 +++++++++++++++++-- src/lib/parsers/default/__fixtures__/index.ts | 12 +++-- 3 files changed, 67 insertions(+), 26 deletions(-) diff --git a/.bench/baseline.json b/.bench/baseline.json index 0d7143a..74fcbbe 100644 --- a/.bench/baseline.json +++ b/.bench/baseline.json @@ -1,5 +1,5 @@ { - "capturedAt": "2026-05-06T00:29:28.140Z", + "capturedAt": "2026-05-06T00:45:56.229Z", "node": "v22.13.0", "platform": "darwin-arm64", "options": { @@ -13,7 +13,7 @@ "fixture": "tiny", "fileCount": 5, "approxTokens": 790, - "durationMs": 2, + "durationMs": 1, "llmCalls": 0, "llmTotalMs": 0, "llmTotalPromptTokens": 0 @@ -22,18 +22,18 @@ "fixture": "medium", "fileCount": 25, "approxTokens": 36150, - "durationMs": 31124, + "durationMs": 31137, "llmCalls": 20, - "llmTotalMs": 106333, + "llmTotalMs": 106348, "llmTotalPromptTokens": 34237 }, { "fixture": "large", "fileCount": 50, "approxTokens": 83410, - "durationMs": 72151, + "durationMs": 72093, "llmCalls": 41, - "llmTotalMs": 244112, + "llmTotalMs": 244101, "llmTotalPromptTokens": 74197 }, { @@ -42,23 +42,23 @@ "approxTokens": 17600, "durationMs": 15967, "llmCalls": 11, - "llmTotalMs": 54726, + "llmTotalMs": 54727, "llmTotalPromptTokens": 18937 }, { "fixture": "refactor", "fileCount": 30, "approxTokens": 32650, - "durationMs": 33994, + "durationMs": 33999, "llmCalls": 28, - "llmTotalMs": 153871, + "llmTotalMs": 153888, "llmTotalPromptTokens": 52430 }, { "fixture": "initial-commit", "fileCount": 50, "approxTokens": 83410, - "durationMs": 72291, + "durationMs": 72285, "llmCalls": 41, "llmTotalMs": 245148, "llmTotalPromptTokens": 74546 @@ -67,19 +67,19 @@ "fixture": "docs-update", "fileCount": 9, "approxTokens": 15050, - "durationMs": 18563, + "durationMs": 18570, "llmCalls": 8, "llmTotalMs": 56293, "llmTotalPromptTokens": 13908 }, { "fixture": "dep-bump", - "fileCount": 3, - "approxTokens": 8450, - "durationMs": 27158, - "llmCalls": 1, - "llmTotalMs": 27141, - "llmTotalPromptTokens": 19597 + "fileCount": 2, + "approxTokens": 450, + "durationMs": 0, + "llmCalls": 0, + "llmTotalMs": 0, + "llmTotalPromptTokens": 0 } ] } \ No newline at end of file diff --git a/src/lib/parsers/default/__fixtures__/index.test.ts b/src/lib/parsers/default/__fixtures__/index.test.ts index b20d160..2d2e008 100644 --- a/src/lib/parsers/default/__fixtures__/index.test.ts +++ b/src/lib/parsers/default/__fixtures__/index.test.ts @@ -1,3 +1,5 @@ +import { DEFAULT_IGNORED_EXTENSIONS, DEFAULT_IGNORED_FILES } from '../../../config/constants' +import { DiffNode } from '../../../types' import { allFixtures, depBumpFixture, @@ -49,6 +51,31 @@ describe('bench fixtures (#845)', () => { expect(firstSnapshot).toBe(secondSnapshot) }) + // Guard against the class of mistake that shipped in the prior + // dep-bump fixture: any file matching the default ignore list + // (lockfiles, node_modules, .map / .lock extensions) would be + // stripped before the pipeline ever sees it on a real commit, so + // including such a file in a fixture produces bench numbers that + // can't translate to user-facing wins. Fail loudly if any fixture + // accidentally drifts back into that shape. + it('no fixture file matches DEFAULT_IGNORED_FILES or DEFAULT_IGNORED_EXTENSIONS', () => { + const offending: string[] = [] + const collect = (node: DiffNode, fixtureName: string) => { + node.diffs.forEach((diff) => { + const basename = diff.file.split('/').pop() || diff.file + if (DEFAULT_IGNORED_FILES.includes(basename)) { + offending.push(`${fixtureName}: ${diff.file} (matches DEFAULT_IGNORED_FILES)`) + } + if (DEFAULT_IGNORED_EXTENSIONS.some((ext) => diff.file.toLowerCase().endsWith(ext))) { + offending.push(`${fixtureName}: ${diff.file} (matches DEFAULT_IGNORED_EXTENSIONS)`) + } + }) + node.children.forEach((child) => collect(child, fixtureName)) + } + allFixtures.forEach((fixture) => collect(fixture.rootNode, fixture.name)) + expect(offending).toEqual([]) + }) + it('refactor scenario includes a rename diff', () => { const collectedDiffs: string[] = [] const walk = (node: typeof refactorFixture.rootNode) => { @@ -59,10 +86,20 @@ describe('bench fixtures (#845)', () => { expect(collectedDiffs.some((diff) => diff.includes('rename from'))).toBe(true) }) - it('dep-bump scenario is dominated by a lockfile-shaped modification', () => { - const lockfileDiff = depBumpFixture.rootNode.diffs.find((diff) => diff.file.endsWith('.lock')) - expect(lockfileDiff).toBeDefined() - expect(lockfileDiff?.diff).toContain('diff --git') - expect(lockfileDiff?.tokenCount).toBeGreaterThan(1000) + it('dep-bump scenario reflects post-filter content (no lockfiles)', () => { + // Lockfiles live in DEFAULT_IGNORED_FILES + DEFAULT_IGNORED_EXTENSIONS, + // so they're stripped before the pipeline ever sees the diff. The + // fixture should mirror what the pipeline would actually receive + // for a dependabot-style commit: package.json + CHANGELOG, both + // small. + const allDiffs: typeof depBumpFixture.rootNode.diffs = [] + const walk = (node: typeof depBumpFixture.rootNode) => { + allDiffs.push(...node.diffs) + node.children.forEach(walk) + } + walk(depBumpFixture.rootNode) + expect(allDiffs.find((diff) => diff.file.endsWith('.lock'))).toBeUndefined() + expect(allDiffs.find((diff) => diff.file.endsWith('-lock.json'))).toBeUndefined() + expect(allDiffs.find((diff) => diff.file === 'package.json')).toBeDefined() }) }) diff --git a/src/lib/parsers/default/__fixtures__/index.ts b/src/lib/parsers/default/__fixtures__/index.ts index 79b3cee..2ea878e 100644 --- a/src/lib/parsers/default/__fixtures__/index.ts +++ b/src/lib/parsers/default/__fixtures__/index.ts @@ -290,13 +290,17 @@ const DOCS_UPDATE_FILES: FileSpec[] = [ ] /** - * Dep bump: the dependabot-style commit. Tiny content change in - * package.json, large lockfile delta. Pipeline should mostly - * skip-trivial these. + * Dep bump: the dependabot-style commit. Just the human-readable + * surface — package.json + an optional CHANGELOG entry. Lockfiles + * (yarn.lock, package-lock.json, pnpm-lock.yaml, *.lock) are in + * `DEFAULT_IGNORED_FILES` / `DEFAULT_IGNORED_EXTENSIONS` + * (`src/lib/config/constants.ts`), so the pipeline never sees them + * on a real `coco commit`. An earlier draft of this fixture + * included `yarn.lock` and reported 27 s of LLM work for it; that + * was a bench-fixture artifact, not a real-world cost. */ const DEP_BUMP_FILES: FileSpec[] = [ { path: 'package.json', tokens: 250, shape: 'modification' }, - { path: 'yarn.lock', tokens: 8000, shape: 'modification' }, { path: 'CHANGELOG.md', tokens: 200, shape: 'modification' }, ]