fix(ai): keep the eval fixtures out of the compiled build - #257
fix(ai): keep the eval fixtures out of the compiled build#257NathanTarbert wants to merge 3 commits into
Conversation
1da6772 to
1299639
Compare
jerelvelarde
left a comment
There was a problem hiding this comment.
All three claims in your message check out, and I verified each rather than reading it. One blocker, which the fix introduces rather than misses.
What holds
Each parser layer is now pinned separately. Ran the three mutations on this branch:
| Mutation | On this PR | On merged main |
|---|---|---|
headerRegion → whole block |
1 failed | survived |
isCode → !!path |
1 failed | survived |
| title precedence flipped | 1 failed | survived |
That is precisely the follow-up I asked for on #242, and it was the item I was most willing to let go.
The dist claims hold. Built it: find ai/dist -name '*.test.*' returns 0, and no fixture reply text survives. One precision note so nobody reopens this — @copilotkitnext still greps in dist, in eval/rules.js and a comment in index.js. That is the DEAD_PACKAGE detector regex, which has to ship. The concern was test data in the artifact, and that is gone.
Blocker — this stops typechecking the tests
ai/tsconfig.json now excludes **/*.test.ts, **/__tests__/** and **/__fixtures__/**. That file is also what typecheck runs against: tsc --project ai/tsconfig.json --noEmit. So the exclusion removes every test in the package from typechecking, and vitest.config has no typecheck block to pick it up.
Verified both directions with the same injected error:
const __probe: number = "definitely not a number";
- on merged
main:ai/src/eval/rules.test.ts(210,7): error TS2322: Type 'string' is not assignable to type 'number'. - on this branch:
pnpm typecheckis clean.
So the PR trades "tests ship in dist" for "test type errors reach main", which is the worse of the two — the first is a packaging wart, the second means a test can assert against a shape that no longer exists and only vitest's runtime behaviour tells you. It also lands right after #242 added SearchResult.kind, which is exactly the kind of type change whose test fallout you want a compiler to find.
Smallest fix that keeps both properties: leave tsconfig.json inclusive and give the build its own config.
// ai/tsconfig.build.json
{ "extends": "./tsconfig.json",
"exclude": ["node_modules", "dist", "**/*.test.ts", "**/__tests__/**", "**/__fixtures__/**"] }…with build:ai pointing at tsconfig.build.json and typecheck left as it is. A separate tsconfig.test.json added to the typecheck chain works too; I mildly prefer the build-specific one because the default config then stays the one that sees everything, which is the safer default to inherit.
Worth a test that fails when a test file stops being typechecked, if you can think of a cheap one — this is a gap that reappears silently, and the whole PR is about artifacts containing what nobody meant them to contain.
Not blocking
Moving the fixtures to src/eval/__fixtures__/historical-failures.ts is the right home, and excluding __fixtures__ from the build is right. It is only the shared-config-with-typecheck that causes the problem — the exclusion list itself is correct.
Send the tsconfig split and I will re-verify straight away; everything else here is done.
… layer The non-blocking follow-ups from Jerel's reviews of #241 and #242. ## The fixtures really do ship, and not re-exporting them was never the fix Verified against a built `dist` rather than reasoned about: `tsc` emits per file and `index.ts` imports `./eval/harness.js`, so while HISTORICAL_FAILURES and TARGET_SHAPE lived in `harness.ts` the reconstructed bad replies shipped in `dist/eval/harness.js` no matter what the entry point declared. `grep -rl "useCopilotFabricatedRender" dist/` hit it. They now live in `eval/__fixtures__/historical-failures.ts`, and `ai/tsconfig.json` excludes `**/*.test.ts`, `**/__tests__/**` and `**/__fixtures__/**` from the build. `harness.ts` no longer imports them, which it cannot: a compiled module importing an excluded one emits a broken build. Excluding test material also fixes something wider that was never raised — every `.test.ts` in the package was being compiled into `dist`. That is now 0 files. Post-build audit: no fixture reply text, no invented hook name, no `__fixtures__` directory. `@copilotkitnext` still appears in `dist/eval/rules.js`, and has to — that is the rule which bans it. A bundle grep for the dead package name hits the rule, not a fabricated example of it. The comment in `index.ts` now says that instead of claiming an exclusion it did not deliver. ## Each parser layer is pinned separately Three layers independently prevent a docs block being read as code — the title prefers TITLE over PATH, `isCode` requires the absence of TITLE, and headers are read only from above `CONTENT:`. The redundancy is deliberate, and it meant no single layer was pinned: reverting any one alone left the suite green, so someone simplifying one away in six months would have got a clean run and a quietly reduced defence. Two tests isolate a layer each: - a block carrying BOTH TITLE and PATH must read as docs and keep its SOURCE. Kills the title-order revert and the `isCode` revert. - a docs block with no SOURCE header and a line-initial `SOURCE:` inside its content must not adopt that line as its citation. Kills the header-region revert. Verified: each single-layer revert now fails exactly one test, where before all three survived alone. `isCode` also gains a comment naming what it depends on — that a code hit never carries a TITLE, which is a server-side contract we do not own. If a code result ever gains one, `source` falls back to an absent `SOURCE` header and the file path silently stops being citable. The both-headers test is what makes that change in behaviour visible. Verification: ai package 377 tests, typecheck 10/10, test 10/10.
Addresses the blocker on #257. Jerel was right, and the regression was mine: the exclusions went into `ai/tsconfig.json`, which is also what `typecheck` reads (`tsc --project ai/tsconfig.json --noEmit`) — so excluding test material stopped it being typechecked at all. Reproduced his proof before changing anything. With `const __probe: number = 'definitely not a number'` in `eval/rules.test.ts`: - merged main: error TS2322: Type 'string' is not assignable to type 'number' - #257 as pushed: pnpm typecheck clean, 10/10 So the PR traded "tests ship in dist" for "test type errors reach main", which is the worse half — vitest exercises runtime behaviour, so a test asserting against a shape that no longer exists surfaces as a confusing failure or not at all. It landed directly after #242 added `SearchResult.kind`, which is exactly the kind of change whose test fallout wants a compiler. `ai/tsconfig.build.json` now carries the exclusions and `build:ai` points at it. The default config goes back to seeing everything, so anything inheriting it inherits the safer default. Both properties verified together: typecheck catches the injected error again, and a clean build emits 18 JS files with 0 test files and no `__fixtures__`. He asked for a test that fails when a test file stops being typechecked. There is one now — `__tests__/typecheck-config.test.ts` asserts that whatever `typecheck` reads does not exclude test material, that the build config does, and that `build:ai` invokes the build config rather than the inclusive one. Checked in both directions: re-adding the exclusion to `tsconfig.json` fails it, and pointing `build:ai` back at `tsconfig.json` fails it. Two notes on writing that guard, since both bugs were the same shape as the change itself: - The first version stripped tsconfig comments by hand, and its block-comment pattern matched inside the exclude values — a doubled-star glob followed by a slash-star extension reads as a comment opener. It collapsed three patterns into one mangled string and failed against config that was correct. It now uses `ts.parseConfigFileTextToJson`. - Explaining that in a docstring then broke the file, because writing those globs literally inside a block comment closes it early. They are described rather than quoted. Not addressed here, and worth its own change: `db`, `queue` and `shared` have no such split, so their tests are still compiled into their published output. Same packaging wart, three more packages, and the same fix applies to each.
|
Thanks Jerel — the blocker was right and the regression was mine. Fixed in I reproduced your proof before touching anything. With So the PR traded "tests ship in Took your preferred shape: The guard you asked for
Two things about writing that guard are worth recording, because both bugs were the same shape as the change itself:
On your precision noteAgreed, and thanks for stating it — Not in this PR
|
1299639 to
c261303
Compare
The format check added in #250 caught these on its first run against a real PR — five of this PR's own files were unformatted. Prettier preserves the comments in tsconfig.build.json, and the build config still parses: a clean build emits 18 JS files with 0 test files. Worth noting the gate did exactly what it was added for, on the first opportunity, against the person who added it.
The eval fixtures stay out of the compiled build, and each layer of the snippet parser is pinned by its own test. These are the non-blocking follow-ups from Jerel's reviews of #241 and #242, both now merged.
The fixtures really did ship
Verified against a built
distrather than reasoned about.tscemits per file andindex.tsimports./eval/harness.js, so whileHISTORICAL_FAILURESandTARGET_SHAPElived inharness.tsthe reconstructed bad replies shipped indist/eval/harness.jsno matter what the entry point re-exported:They now live in
eval/__fixtures__/historical-failures.ts, andai/tsconfig.jsonexcludes**/*.test.ts,**/__tests__/**and**/__fixtures__/**.harness.tsno longer imports them — it can't, since a compiled module importing an excluded one emits a broken build.Excluding test material also fixes something wider that nobody had raised: every
.test.tsin the package was being compiled intodist. That is now 0 files.Post-build audit:
distuseCopilotFabricatedRenderdist/eval/__fixtures__/@copilotkitnextdist/eval/rules.jsonlyThat last row is expected and cannot be otherwise —
rules.jsis the rule that bans the package, so it has to name it. A bundle grep for the dead package hits the rule, not a fabricated example of it.index.ts's comment now says that, instead of claiming an exclusion it did not deliver.Each parser layer is pinned separately
Three layers independently prevent a docs block being read as code: the title prefers
TITLEoverPATH,isCoderequires the absence ofTITLE, and headers are read only from aboveCONTENT:. The redundancy is deliberate — and it meant no single layer was pinned. Reverting any one alone left the suite green, so simplifying one away in six months would have produced a clean run and a quietly reduced defence.Two tests isolate a layer each:
TITLEandPATHmust read as docs and keep itsSOURCE. Kills the title-order revert and theisCoderevert.SOURCEheader and a line-initialSOURCE:in its content must not adopt that line as its citation. Kills the header-region revert.Verified: each single-layer revert now fails exactly one test, where before all three survived alone.
isCodealso gains a comment naming what it depends on — that a code hit never carries aTITLE, which is a server-side contract we don't own. If a code result ever gains one,sourcefalls back to an absentSOURCEheader and the file path silently stops being citable, which the reply rules then turn into a handoff. The both-headers test is what makes that change in behaviour visible rather than silent.Verification
aipackage 377 tests,turbo run typecheck10/10,turbo run test10/10.The remaining item from those reviews is filed rather than fixed: #254 covers the
max_tokensbudgets sitting below a thinking turn, which is the gating item for anyone actually swapping a model.