Skip to content

fix(build): ship the artifacts main's source actually builds, and make the check say so - #38

Merged
andrei-hasna merged 2 commits into
mainfrom
fix/hc-00151-regenerate-mcp-bundle
Jul 27, 2026
Merged

fix(build): ship the artifacts main's source actually builds, and make the check say so#38
andrei-hasna merged 2 commits into
mainfrom
fix/hc-00151-regenerate-mcp-bundle

Conversation

@andrei-hasna

@andrei-hasna andrei-hasna commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Closes 20803062 (HC-00151). Base origin/main 64d05ae.

The defect: one check, two names, two answers

bun scripts/verify-generated-artifacts.mjs   -> exit 0
bun run verify:generated                     -> exit 1

package.json:58 prefixed a bun run build, and the rebuild did not reproduce the committed
artifacts. The exit-0 form had been cited in CHANGELOG.md:62-63 as evidence that a rebuilt
bundle matched its source — something it never checked. PR #36 corrected that wording; this PR
fixes the two underlying facts.

Part 1 — the artifacts are stale, and the whole diff is 9 lines

Rebuilding from clean main modifies exactly two files:

 M bin/knowledge-mcp.js      (5 lines: +3 -2)
 M dist/index.js             (4 lines: +2 -2)

Two independent causes, isolated:

  1. Genuine staleness since fix(smoke): validate remote mktemp -d before using it as an rm -rf target #33. package.json is inlined into the bundles and its files
    array gained scripts/lib/remote-temp-dir.mjs in 354a855. Occurrences of
    remote-temp-dir in the artifacts committed at 64d05ae: bin/knowledge.js 1,
    bin/knowledge-mcp.js 0 (positive control: package.json itself, 1). So main shipped
    an mcp bundle built before fix(smoke): validate remote mktemp -d before using it as an rm -rf target #33.
  2. zod codegen drift. Three collapses of empty else blocks —
    } else if (ctx.target === "openapi-3.0") {} else {} losing the trailing else {}. Dead-code
    elimination, semantically identical. Absent from bin/knowledge.js only because that bundle
    is --minifyed.

Absorbed rather than pinned around, because the rebuild is deterministic: two consecutive
builds produce byte-identical output for all six bundles (cmp clean both files), and the four
unaffected bundles reproduce their committed bytes exactly.

bin/knowledge.js is rebuilt here too, for one reason worth stating plainly: package.json is
inlined into it, so editing the verify:generated script line in Part 2 changes that bundle.
Its only content change is that script string.

Why CI never caught it: .github/workflows/ci.yml runs Verify generated artifacts after
Run tests, and the pre-existing context pack… test failure aborts the job first. The staleness
was masked by an unrelated red test.

Part 2 — making the check mean something

Measured against the old script: it exited 0 with src/cli.ts diverged from
bin/knowledge.js, and 0 with bin/knowledge.js corrupted by 30 junk bytes. It did not
require that file to be byte-stable at all.

before after
rebuild only if you remembered the && prefix inside the script; there is no other way to run it
diff gate bin/knowledge-mcp.js + dist the bin and dist directories
pattern scan 4 hardcoded files (already drifted — omitted bin/knowledge-serve.js, dist/serve.js) derived from git ls-files
dirty tree before rebuild silently folded into the result named and refused
dead regex reports clean forever fails the check

The last row is the one that matters most. Each stale pattern now carries a fixture it must
match
and a counter-fixture it must not, checked before anything else. A regex that can no
longer match anything reports "clean" forever, which is a check that passes while measuring
nothing.

That self-check earned its place immediately. The first fixture written for
/path:\s*decodeURIComponent\([^)]*\.pathname\)/ was
decodeURIComponent(new URL(uri).pathname) — and the script rejected it, because [^)]* cannot
cross a nested ). The fixture is now verbatim the line 2bea200 removed from
src/source-ref.ts, and the pattern's real bound is documented at the pattern rather than left
as a surprise: it does not catch the inline new URL(...) form. Widening it would trade a
known bound for unknown false positives across four minified bundles.

Verification — four probes, unpiped exit codes

V1  verify:generated on the committed tree            rc=0   "6 generated bundles rebuild
                                                              byte-identically and carry no
                                                              stale generated code."
V2  git status --porcelain after V1                   empty
V3b plant a reachable source change in src/mcp.js     rc=1   diff shows +"ok_untag_PLANTED"
V3c commit a corrupted bin/knowledge.js (the real
    main scenario) then verify                        rc=1   "the committed generated artifacts
                                                              are not what the current source builds"
V3d dirty bin/dist before the rebuild                rc=1   precondition names the paths
V3e empty one stale-pattern fixture                   rc=1   "no longer matches its own fixture
                                                              — it cannot detect anything"
V3f revert everything, verify again                   rc=0   tree clean, HEAD unchanged

A probe that did NOT fire, recorded rather than dropped. Appending an unused
const KNOWLEDGE_MCP_PLANTED_DRIFT = "…" to src/mcp.js left the check at rc=0. That is
correct, and it bounds what this check proves: the bundler eliminates the dead const, so the
bundle genuinely did not change. The gate verifies the committed bundles equal what the current
source builds
— not that every source edit is reflected in a bundle. The first plant tried was
that one, and treating its rc=0 as a verifier defect would have been wrong.

Test suite, env -u HASNA_KNOWLEDGE_API_URL -u HASNA_KNOWLEDGE_API_KEY bun test --timeout 60000:

rc=1   262 pass / 2 skip / 1 fail / 2488 expect() calls, 265 tests across 38 files
sole failure: knowledge cli > context pack and proposal context commands return bounded agent JSON
git status --porcelain after the suite: empty

That failure is pre-existing and unrelated: same sole failure at base 64d05ae and on PR #36.

HASNA_KNOWLEDGE_API_URL and HASNA_KNOWLEDGE_API_KEY are exported in a plain shell on this
box and flip the CLI into cloud mode. bun test without env -u reports 99 failures
instead of 1. Unset both or the measurement is worthless.

Explicitly not done

  • dist/index.js's zod drift is absorbed, not pinned. Pinning zod would stop the churn but
    is a dependency-policy decision with its own blast radius; absorbing a 4-line dead-code
    difference is the smaller, reversible move. PR build: commit bun.lock and fix the lockfile ignore rules #37 commits a lockfile, which makes the churn
    reproducible rather than random.
  • End-to-end script behaviour is not unit-tested, and tests/generated-artifacts.test.ts
    says so at the bottom of the file. Asserting "exits 1 on planted divergence" from inside the
    suite needs a real bun run build, which overwrites bin/ and dist/ while other test files
    are running. A guard that corrupts the tree it guards is a worse trade than the coverage. The
    composed pieces are unit-tested; the end-to-end path runs in CI on every PR — and the four
    probes above exercise it by hand.
  • A design smell found and not fixed: any package.json edit invalidates every bundle,
    because the whole file is inlined. That makes routine version bumps look like artifact drift.
    Narrowing what gets inlined is a separate change.

Ordering

Independent of PR #37, but #37 should land first: it commits bun.lock, which is what makes
the byte-comparison this PR gates on reproducible across machines. No file overlap between them
(#37 touches .gitignore, bun.lock, tests/lockfile.test.ts).

PR #39 is stacked on this branch — it changes src/mcp.js and therefore needs the mcp bundle
to already be in sync. Merge order: #37, then this, then #39.

Task: 20803062. Not merged by me — needs an independent adversarial review first.


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

…e the check say so

`bun run verify:generated` was RED on main at 64d05ae while
`bun scripts/verify-generated-artifacts.mjs` was GREEN. One check, two names, two
answers — and the passing one had been cited in CHANGELOG.md as evidence that a
rebuilt bundle matched its source, which it never checked.

## The stale artifacts

    bun scripts/verify-generated-artifacts.mjs   -> exit 0
    bun run verify:generated                     -> exit 1

package.json:58 prefixed a `bun run build`, and the rebuild did not reproduce the
committed files. Rebuilding from clean main leaves exactly two artifacts modified —
`bin/knowledge-mcp.js` and `dist/index.js` — and the whole diff is 9 lines:

  * `"scripts/lib/remote-temp-dir.mjs"` added. package.json is inlined into the
    bundles and its `files` array gained that entry in 354a855 (#33), so the
    committed mcp bundle predates #33. Occurrences of `remote-temp-dir` in the
    committed artifacts at 64d05ae: bin/knowledge.js 1, bin/knowledge-mcp.js 0.
  * three zod codegen collapses of empty else blocks,
    `} else if (ctx.target === "openapi-3.0") {} else {}` -> `} else {}` removed.
    Dead-code elimination, semantically identical, absent from bin/knowledge.js
    only because that bundle is --minify'd.

Both are absorbed rather than pinned around. The rebuild is deterministic: two
consecutive builds produce byte-identical output for all six bundles (`cmp` clean),
and the four unaffected bundles reproduce the committed bytes exactly.

`bin/knowledge.js` is also rebuilt here, for one reason worth stating: package.json
is inlined, so changing the `verify:generated` script line below changes that bundle
too. Its only content change is that script string.

CI never caught this because .github/workflows/ci.yml runs "Verify generated
artifacts" AFTER "Run tests", and the pre-existing context-pack test failure aborts
the job first.

## Making the check mean something

The old script never rebuilt. On a clean checkout its exit 0 meant only "the files I
did not touch are untouched". Measured against it earlier: exit 0 with src/cli.ts
diverged from bin/knowledge.js, and exit 0 with bin/knowledge.js corrupted by 30
junk bytes.

  * ONE ENTRY POINT. The build moved inside the script, so it cannot be run without
    the rebuild and read as a sync check. `verify:generated` is now the script alone,
    and CI calls `bun run verify:generated` instead of reassembling the two halves.
  * NO HARDCODED ARTIFACT LIST. The diff gate covers the `bin` and `dist` directories
    instead of two paths, and the stale-pattern scan derives its files from
    `git ls-files`. The old four-file list had already drifted: it omitted
    bin/knowledge-serve.js and dist/serve.js.
  * A DIRTY-BEFORE PRECONDITION. A post-rebuild `git diff` only means something if the
    generated paths matched the index beforehand. It now fails with the offending
    paths named instead of reporting drift it cannot attribute.
  * THE PATTERNS PROVE THEMSELVES BEFORE A CLEAN RESULT IS TRUSTED. Each stale pattern
    carries a fixture it must match and a counter-fixture it must not; if either fails
    the script exits 1 rather than reporting every artifact clean. A regex that can no
    longer match anything reports "clean" forever.

That self-check earned its place immediately: the first fixture written for
`/path:\s*decodeURIComponent\([^)]*\.pathname\)/` used
`decodeURIComponent(new URL(uri).pathname)` and the script rejected it, because
`[^)]*` cannot cross a nested `)`. The fixture is now verbatim the line 2bea200
removed from src/source-ref.ts, and the known bound is documented at the pattern
rather than left as a surprise.

tests/generated-artifacts.test.ts covers the pieces: directory-wide gate, derived
file list containing all six shipped bundles and nothing outside bin/dist, every
pattern firing on its fixture and rejecting its counter-fixture, a synthetic stale
bundle in a temp root producing one problem per pattern while the counter-fixtures
come back clean, and `verify:generated` staying a single command. End-to-end script
behaviour is deliberately not tested there — it would need a real build, which
overwrites bin/ and dist/ while other test files run.
@andrei-hasna
andrei-hasna force-pushed the fix/hc-00151-regenerate-mcp-bundle branch from a04e2f5 to 19738de Compare July 27, 2026 22:20
Found in adversarial review of #38. `Verify generated artifacts` rebuilds bin/ and
dist/ and compares BYTE-FOR-BYTE, so it can only pass when CI's bun equals the bun
that produced the committed bundles. It did not.

Measured at 19738de, clean tree, unpiped exit codes:

  bun 1.3.14 (local)  bun run verify:generated  rc=0  6 bundles byte-identical
  bun 1.3.13 (CI pin) bun run build             rc=0  then git diff -> rc=1
                                                      bin/knowledge-mcp.js 4 +/-
                                                      dist/index.js        4 +/-

The drift is exactly the inverse of the drift #38 fixes: 1.3.13 keeps the empty
`else {}` blocks that 1.3.14's dead-code elimination collapses. So the churn #38's
description attributes to "zod codegen drift" is a bun minifier difference between
patch releases, and bun.lock cannot pin it — a lockfile pins dependencies, not the
bun binary. The gate was therefore red-by-construction in CI, masked only because
the step never executes (see below).

Changes:
- Pin both CI jobs to bun 1.3.14, the version that built the committed bundles, and
  say at the pin why the version is load-bearing rather than a runtime preference.
- Regression test: every `bun-version:` in ci.yml must agree and must be an exact
  patch. Negative-controlled — skewing the two pins fails it with
  "CI pins more than one bun version: 1.3.13, 1.3.14".
- Correct the closing comment in tests/generated-artifacts.test.ts. It claimed the
  end-to-end path "is exercised by CI on every PR". It is not: the step runs after
  `Run tests` with no `if: always()`, so the pre-existing `context pack` failure
  skips it. Run 30310387905, job test (ubuntu-latest, bun): step 6 failure, step 7
  skipped. Citing a check that does not execute is the defect #38 exists to fix.
- Make the drift message print the running bun and warn against committing a
  rebuild before checking the version, since on a skewed machine "commit it" moves
  the failure into CI instead of fixing it.

Verified after these edits: bun run verify:generated rc=0 (bundles not invalidated —
ci.yml, scripts/ and tests/ are not bundled), tests/generated-artifacts.test.ts
8 pass 0 fail.
@andrei-hasna

Copy link
Copy Markdown
Contributor Author

Adversarial review — APPROVE-WITH-FIXES (fixes pushed as 7a6209a, then merging)

Independent verification on spark01 (aarch64), cloud-flip vars neutralised with
env -u HASNA_KNOWLEDGE_API_URL -u HASNA_KNOWLEDGE_API_KEY, resolved store kind confirmed
local before any number was believed. Unpiped exit codes throughout.

Confirmed — the substantive claim reproduces

probe result
bun run build at main 535daf0, clean tree, then git diff -- bin dist rc=1, bin/knowledge-mcp.js 5 +/-, dist/index.js 4 +/- — exactly the diffstat in the description
bun run verify:generated at 19738de rc=0, "6 generated bundles rebuild byte-identically and carry no stale generated code", tree clean after
the [^)]* bound, re-derived independently fixture true, counter-fixture false, decodeURIComponent(new URL(uri).pathname) false — the documented bound is real
"verbatim the line 2bea200 removed" git show 2bea200 -- src/source-ref.ts removes return { kind: 'file', uri, path: decodeURIComponent(parsed.pathname) }; — verbatim modulo indentation
narrowed claim after the non-firing plant present: "the committed bundles equal what the current source builds — not that every source edit is reflected in a bundle"
e2e-not-unit-tested admission present at the bottom of tests/generated-artifacts.test.ts, and accurate as to why

I reproduced the main drift on bun 1.3.14 while CI pinned 1.3.13, so the staleness is
not a toolchain artefact of one version. No new test failures: main 535daf0 CI already fails
with the identical set (ubuntu 1 fail, macOS 2 fail).

Finding 1 (blocking, fixed in 7a6209a) — the gate was red-by-construction in CI

The byte comparison is bun-version-sensitive, and CI pinned a different bun than the one that
built the committed bundles:

bun 1.3.14 (local)   bun run verify:generated   rc=0   6 bundles byte-identical
bun 1.3.13 (CI pin)  bun run build              rc=0   then git diff -- bin dist  ->  rc=1
                                                       bin/knowledge-mcp.js  4 +/-
                                                       dist/index.js         4 +/-

The 1.3.13 drift is exactly the inverse of the drift this PR fixes — 1.3.13 keeps the empty
else {} blocks 1.3.14's dead-code elimination collapses. Confirmed at both 19738de and
88b00aa.

That reclassifies cause 2 in the description: it is a bun minifier/DCE difference between patch
releases
, not "zod codegen drift" — and so the stated mitigation does not hold. bun.lock
pins dependencies; it cannot pin the bun binary. Fixed by pinning both jobs to 1.3.14 (the
version that built the committed bytes), plus a negative-controlled regression test that all
bun-version: pins agree and are exact patches — skewing them fails with
CI pins more than one bun version: 1.3.13, 1.3.14.

Finding 2 (blocking, fixed in 7a6209a) — the compensating control does not execute

tests/generated-artifacts.test.ts justified not unit-testing the end-to-end path with "the
end-to-end path is exercised by CI on every PR, which is where a real divergence shows up."
It is not:

run 30310387905, job test (ubuntu-latest, bun)
  step 6  Run tests                   failure
  step 7  Verify generated artifacts  SKIPPED

Verify generated artifacts runs after Run tests with no if: always(), so the pre-existing
context pack and proposal context commands return bounded agent JSON failure — red on main
too — skips it on every run. This is the same masking Part 1 identifies as the reason the stale
bundle survived, so citing that step as the compensating control repeats the exact defect this
PR exists to fix. The comment now states the measured truth; the step reordering is filed
separately rather than folded in here.

Finding 3 (non-blocking): the drift message now prints the running bun and warns against
committing a rebuild before checking the version — on a version-skewed machine "commit it" moves
the failure into CI instead of fixing it.

Verified after my fixes

bun run verify:generated rc=0 (my edits touch ci.yml, scripts/ and tests/, none of
which are bundled — checked, not assumed); tests/generated-artifacts.test.ts 8 pass / 0 fail.
Staged secrets scan: 0 matches on a 7406-byte diff, scanner positive-controlled against a planted
token first.

Not verified — stated as a bound

CI's ubuntu-latest/windows-latest are x64; every measurement above is aarch64. I
controlled for bun version, not architecture. If bun's bundler output is arch-dependent the
gate will still fail there once reachable. Unmeasurable on this machine (the x64 bun binary will
not run).

Task 20803062. Formal approval is not possible from this identity ("Can not approve your own
pull request"), so this comment is the review of record.

@andrei-hasna
andrei-hasna merged commit 1b8e127 into main Jul 27, 2026
0 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant