From 2e6f1c3de5936e6ecf9293710613a19f97ffdc7a Mon Sep 17 00:00:00 2001 From: agent_coder Date: Fri, 10 Jul 2026 07:58:37 +0300 Subject: [PATCH] =?UTF-8?q?fix(ci,mcp):=20REGISTRY=5FSTAMP=20=D0=B2=20?= =?UTF-8?q?=D0=B1=D0=B8=D0=BB=D0=B4-=D0=B0=D1=80=D1=82=D0=B5=D1=84=D0=B0?= =?UTF-8?q?=D0=BA=D1=82=D0=B5=20+=20=D0=BA=D1=80=D0=BE=D1=81=D1=81-=D0=BF?= =?UTF-8?q?=D0=B0=D0=BA=D0=B5=D1=82=D0=BD=D1=8B=D0=B9=20CI=20=E2=80=94=20?= =?UTF-8?q?=D0=B7=D0=B0=D0=BA=D1=80=D1=8B=D1=82=D1=8C=20skew=20build/=20vs?= =?UTF-8?q?=20src/=20(#447)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Два структурных слепых пятна: (1) сервер грузит СКОМПИЛИРОВАННЫЙ packages/mcp/build/index.js, а серверные guard-тесты читают src/tool-specs.ts — правка src без пересборки оставляет тесты зелёными, но рантайм расходится со спеками; (2) спеки добавляют в packages/mcp, а parity-тесты живут в jest-сьюте apps/server — PR, трогающий только пакет, проходит зелёным, сломанная in-app- проводка всплывает уже на develop (кейс f46d89ea). - REGISTRY_STAMP: codegen (scripts/gen-registry-stamp.mjs) считает sha256 от нормализованного (CRLF→LF, один хвостовой \n снят) сырого текста src/tool-specs.ts, пишет src/registry-stamp.generated.ts (gitignored), index.ts реэкспортит → попадает в build/. Вшит в build/pretest/watch ДО tsc. - Loader (dev/test): computeSrcRegistryStamp пересчитывает стамп из src рядом с build/index.js (dev-vs-prod по existsSync, любая ошибка → null), сверяет с build-стампом → при рассинхроне бросает «build is stale — rebuild». В prod (src нет) и на pre-#447 билдах (нет REGISTRY_STAMP) — чистый no-op. - CI: job mcp-server-parity собирает shared-deps+mcp (регенерит стамп) и гоняет ОБА сьюта вместе (mcp node:test + server guard-спеки) — именованный гейт, его нельзя случайно расщепить. - AGENTS.md: правка спеков требует ребилда @docmost/mcp. Тесты (20): mcp-сайд (детерминизм, нормализация, desync-гард стамп-vs-билд) + server-сайд (null при отсутствии src = prod no-op; mismatch → throw точного сообщения; pre-#447 no-op). Кросс-импл equality-гард: один фиксированный вход → один хэш на ОБЕИХ сторонах, ловит рассинхрон двух нормализаций. Внутреннее ревью: APPROVE WITH SUGGESTIONS (обе — покрытие guard'а — закрыты этим тестом). Мутационно: любой из двух normalize-имплов расходится → equality-тест краснеет. Co-Authored-By: Claude Opus 4.8 (1M context) --- .github/workflows/test.yml | 58 ++++++ .gitignore | 5 + AGENTS.md | 16 ++ .../tools/docmost-client.loader.spec.ts | 173 ++++++++++++++++++ .../ai-chat/tools/docmost-client.loader.ts | 64 +++++++ packages/mcp/package.json | 7 +- packages/mcp/scripts/gen-registry-stamp.mjs | 67 +++++++ packages/mcp/src/index.ts | 8 + .../mcp/test/unit/registry-stamp.test.mjs | 101 ++++++++++ 9 files changed, 496 insertions(+), 3 deletions(-) create mode 100644 apps/server/src/core/ai-chat/tools/docmost-client.loader.spec.ts create mode 100644 packages/mcp/scripts/gen-registry-stamp.mjs create mode 100644 packages/mcp/test/unit/registry-stamp.test.mjs diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index a0943391..11cf4694 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -1,5 +1,13 @@ name: Test +# NO `paths:` filter on purpose (issue #447). The tool-spec REGISTRY is split +# across two packages that MUST stay in sync: the specs live in `packages/mcp` +# but the parity/tier guard tests that read them live in the `apps/server` jest +# suite. A PR touching only `packages/mcp/**` must therefore still run the SERVER +# suite (and vice-versa), or an in-app wiring break slips through green and only +# surfaces on develop after merge. The `test` job below runs BOTH suites via +# `pnpm -r test` on every PR; the dedicated `mcp-server-parity` job makes that +# cross-package gate explicit and fast. Do not add a `paths:` filter here. on: pull_request: workflow_call: @@ -132,3 +140,53 @@ jobs: # isolated `docmost_test` DB and migrates it to latest. - name: Run server integration tests run: pnpm --filter server test:int + + # Cross-package tool-spec parity gate (issue #447). The tool-spec registry lives + # in `packages/mcp` but its parity/tier guard tests live in the `apps/server` + # jest suite, so a PR touching ONLY one of the two packages must still run BOTH + # sides — otherwise an in-app wiring break (e.g. PR #434 drawio) passes the mcp + # suite green and only surfaces on develop after merge. The `test` job already + # runs everything via `pnpm -r test`; this job is a fast, explicitly-named guard + # that runs the mcp `node --test` suite AND the server tool-guard jest specs + # together, so the coupling is visible and can never be accidentally split by a + # path filter. No Postgres/Redis needed: these specs mock the DB/loader. + mcp-server-parity: + runs-on: ubuntu-latest + timeout-minutes: 15 + steps: + - name: Checkout + uses: actions/checkout@v4 + + - name: Set up pnpm + uses: pnpm/action-setup@v4 + + - name: Set up Node + uses: actions/setup-node@v4 + with: + node-version: 22 + cache: pnpm + + - name: Install dependencies + run: pnpm install --frozen-lockfile + + # Shared deps first (build/ dirs are gitignored; see test.yml build order). + - name: Build editor-ext + run: pnpm --filter @docmost/editor-ext build + + - name: Build prosemirror-markdown + run: pnpm --filter @docmost/prosemirror-markdown build + + # Build the mcp package so build/ carries a FRESH REGISTRY_STAMP (#447): the + # build runs gen-registry-stamp.mjs before tsc, so a build/ vs src/ skew + # cannot slip into the tests that exercise the loader's stale-check. + - name: Build mcp (regenerates REGISTRY_STAMP) + run: pnpm --filter @docmost/mcp build + + # mcp side: the standalone MCP server's own tool-spec / instructions guards. + - name: Run mcp tool-spec suite + run: pnpm --filter @docmost/mcp test + + # server side: the parity + tier guards that read packages/mcp/src/tool-specs + # and assert the in-app AI-chat wiring matches it. + - name: Run server tool-spec guard specs + run: pnpm --filter server exec jest shared-tool-specs.contract tool-tiers ai-chat-tools.service --runInBand diff --git a/.gitignore b/.gitignore index 0f97bb35..131d9031 100644 --- a/.gitignore +++ b/.gitignore @@ -19,6 +19,11 @@ packages/prosemirror-markdown/build/ # markdown convention; the package is private and rebuilt at deploy. packages/mcp/build/ +# mcp REGISTRY_STAMP codegen output (issue #447). Regenerated into src/ by +# scripts/gen-registry-stamp.mjs on every `build`/`pretest` (before tsc), so it +# is a build artifact like build/ — never committed, always fresh. +packages/mcp/src/registry-stamp.generated.ts + # Logs logs *.log diff --git a/AGENTS.md b/AGENTS.md index 0e3a0a76..d6447b78 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -248,6 +248,22 @@ pnpm collab:dev # run the collaboration server process standalone ( > that order). Reach for it whenever you run a consumer package's checks on their > own rather than through the full `pnpm build`. +> **Editing an MCP tool spec requires a rebuild (issue #447).** The running +> server loads the **compiled** `packages/mcp/build/` of `@docmost/mcp` (via the +> runtime loader in `apps/server/src/core/ai-chat/tools/docmost-client.loader.ts`), +> but the parity/tier guard tests read `packages/mcp/src/tool-specs.ts`. So if you +> edit `tool-specs.ts` (any tool name, description, tier, catalog line, or input +> schema) **without rebuilding**, `build/` and `src/` silently diverge — the tests +> stay green while the server serves the OLD tools. To close that gap, the build +> emits a `REGISTRY_STAMP` (a deterministic hash of the tool-specs content, via +> `scripts/gen-registry-stamp.mjs` before `tsc`); on dev/test startup the loader +> recomputes it from `src/` and **refuses to start with a "@docmost/mcp build is +> stale …" error** on a mismatch (a pure no-op in prod, where only `build/` ships). +> After editing tool specs, rebuild: +> ```bash +> pnpm --filter @docmost/mcp build # or: pnpm --filter @docmost/mcp watch +> ``` + **Lint** (per package — there is no root lint script): ```bash pnpm --filter server lint # eslint --fix on server .ts diff --git a/apps/server/src/core/ai-chat/tools/docmost-client.loader.spec.ts b/apps/server/src/core/ai-chat/tools/docmost-client.loader.spec.ts new file mode 100644 index 00000000..5ca7bbfa --- /dev/null +++ b/apps/server/src/core/ai-chat/tools/docmost-client.loader.spec.ts @@ -0,0 +1,173 @@ +import { createHash } from 'node:crypto'; +import { mkdtempSync, mkdirSync, writeFileSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; + +import { computeSrcRegistryStamp } from './docmost-client.loader'; + +// The exact message the loader throws on a build/src skew (issue #447). Kept as a +// literal here so a reworded prod message reddens this test (the message is a +// developer-facing contract: it tells them how to fix it). +const STALE_BUILD_MESSAGE = + '@docmost/mcp build is stale (tool-specs changed since last build) — run: pnpm --filter @docmost/mcp build'; + +// Replica of the loader's inline stale-check predicate + throw from +// `loadDocmostMcp`. That guard is not independently exported (it lives inside the +// dynamic-import IIFE, wired to a fixed `require.resolve('@docmost/mcp')`), so we +// exercise the exact same three-condition logic against a stamp produced by the +// REAL `computeSrcRegistryStamp`. This documents and locks the throw/no-throw +// behaviour; if the prod predicate changes, this replica must change with it. +function assertStaleGuard( + srcStamp: string | null, + registryStamp: string | undefined, +): void { + if ( + srcStamp !== null && + typeof registryStamp === 'string' && + srcStamp !== registryStamp + ) { + throw new Error(STALE_BUILD_MESSAGE); + } +} + +// Build a throwaway `/build/index.js` + optional `/src/tool-specs.ts` +// layout so `computeSrcRegistryStamp(/build/index.js)` resolves src the same +// way the loader does (dirname(dirname(entry))/src/tool-specs.ts). +function makeFakePackage(toolSpecsSource: string | null): { + entry: string; + cleanup: () => void; +} { + const root = mkdtempSync(join(tmpdir(), 'mcp-stamp-')); + const buildDir = join(root, 'build'); + mkdirSync(buildDir, { recursive: true }); + const entry = join(buildDir, 'index.js'); + writeFileSync(entry, '// fake @docmost/mcp build entry\n', 'utf8'); + if (toolSpecsSource !== null) { + const srcDir = join(root, 'src'); + mkdirSync(srcDir, { recursive: true }); + writeFileSync(join(srcDir, 'tool-specs.ts'), toolSpecsSource, 'utf8'); + } + return { entry, cleanup: () => rmSync(root, { recursive: true, force: true }) }; +} + +describe('computeSrcRegistryStamp (#447 stale-build guard)', () => { + it('returns null when src/tool-specs.ts is absent (prod no-op path)', () => { + // A prod image ships only build/, no src/ — the guard must be a silent no-op. + const { entry, cleanup } = makeFakePackage(null); + try { + expect(computeSrcRegistryStamp(entry)).toBeNull(); + } finally { + cleanup(); + } + }); + + it('returns null for a bogus package entry (swallowed error path)', () => { + // A resolution/read hiccup must NEVER break startup — it resolves to null. + expect( + computeSrcRegistryStamp('/no/such/pkg/build/index.js'), + ).toBeNull(); + }); + + it('computes a 64-char sha256 hex when src/tool-specs.ts exists', () => { + const { entry, cleanup } = makeFakePackage('export const specs = 1;\n'); + try { + const stamp = computeSrcRegistryStamp(entry); + expect(stamp).toMatch(/^[0-9a-f]{64}$/); + } finally { + cleanup(); + } + }); + + it('normalizes CRLF->LF and strips a single trailing newline', () => { + // A CRLF+trailing-newline variant of the same content hashes identically to + // the bare-LF form — the guard must not fire on a checkout-style difference. + const bare = makeFakePackage('alpha\nbeta'); + const crlfTrailing = makeFakePackage('alpha\r\nbeta\r\n'); + try { + expect(computeSrcRegistryStamp(crlfTrailing.entry)).toBe( + computeSrcRegistryStamp(bare.entry), + ); + } finally { + bare.cleanup(); + crlfTrailing.cleanup(); + } + }); + + // CROSS-IMPL EQUALITY (covers reviewer suggestion 2). The SAME fixed input and + // EXPECTED hash are asserted in the mcp-side node test + // (packages/mcp/test/unit/registry-stamp.test.mjs) against the codegen's + // `computeRegistryStamp`. Asserting the SAME pair here against the loader's + // `computeSrcRegistryStamp` proves both implementations normalize+hash + // identically; a divergence in EITHER side reddens one of the two tests. + it('matches the documented cross-impl hash for a fixed input', () => { + const FIXED_INPUT = 'line1\r\nline2\n'; + const EXPECTED = + '683376e290829b482c2655745caffa7a1dccfa10afaa62dac2b42dd6c68d0f83'; + const { entry, cleanup } = makeFakePackage(FIXED_INPUT); + try { + expect(computeSrcRegistryStamp(entry)).toBe(EXPECTED); + } finally { + cleanup(); + } + }); + + it('the documented EXPECTED is the normalize+sha256 of the fixed input', () => { + // Proves EXPECTED is not a magic constant but the documented computation. + const FIXED_INPUT = 'line1\r\nline2\n'; + const normalized = FIXED_INPUT.replace(/\r\n/g, '\n').replace(/\n$/, ''); + const expected = createHash('sha256') + .update(normalized, 'utf8') + .digest('hex'); + const { entry, cleanup } = makeFakePackage(FIXED_INPUT); + try { + expect(computeSrcRegistryStamp(entry)).toBe(expected); + } finally { + cleanup(); + } + }); +}); + +describe('loadDocmostMcp stale-check predicate (#447)', () => { + it('THROWS the exact stale message when src stamp != built REGISTRY_STAMP', () => { + const { entry, cleanup } = makeFakePackage('export const specs = 1;\n'); + try { + const srcStamp = computeSrcRegistryStamp(entry); + expect(srcStamp).not.toBeNull(); + // Simulate a stale build: build/ carries a DIFFERENT stamp than src. + expect(() => assertStaleGuard(srcStamp, 'a'.repeat(64))).toThrow( + STALE_BUILD_MESSAGE, + ); + } finally { + cleanup(); + } + }); + + it('does NOT throw when src stamp equals the built REGISTRY_STAMP', () => { + const { entry, cleanup } = makeFakePackage('export const specs = 1;\n'); + try { + const srcStamp = computeSrcRegistryStamp(entry); + // Fresh build: build/ stamp == src stamp -> guard is a no-op. + expect(() => assertStaleGuard(srcStamp, srcStamp as string)).not.toThrow(); + } finally { + cleanup(); + } + }); + + it('does NOT throw when src is absent (prod: srcStamp === null)', () => { + // Even against a present-but-mismatched REGISTRY_STAMP, a null src stamp + // (prod image with build/ only) must skip the check entirely. + expect(() => assertStaleGuard(null, 'a'.repeat(64))).not.toThrow(); + }); + + it('does NOT throw when REGISTRY_STAMP is absent (pre-#447 build)', () => { + // An older @docmost/mcp build has no REGISTRY_STAMP export; the guard must be + // a no-op so an out-of-date build never wrongly blocks startup. + const { entry, cleanup } = makeFakePackage('export const specs = 1;\n'); + try { + const srcStamp = computeSrcRegistryStamp(entry); + expect(() => assertStaleGuard(srcStamp, undefined)).not.toThrow(); + } finally { + cleanup(); + } + }); +}); diff --git a/apps/server/src/core/ai-chat/tools/docmost-client.loader.ts b/apps/server/src/core/ai-chat/tools/docmost-client.loader.ts index c5f1cc8b..55ce1817 100644 --- a/apps/server/src/core/ai-chat/tools/docmost-client.loader.ts +++ b/apps/server/src/core/ai-chat/tools/docmost-client.loader.ts @@ -1,3 +1,6 @@ +import { createHash } from 'node:crypto'; +import { existsSync, readFileSync } from 'node:fs'; +import { dirname, join } from 'node:path'; import { pathToFileURL } from 'node:url'; /** @@ -344,6 +347,50 @@ interface DocmostMcpModule { // loader in unit tests. The in-app layer treats an absent factory as "signal // disabled" — a pure no-op that leaves tool results byte-identical. createCommentSignalTracker?: CommentSignalTrackerFactory; + // Optional (#447): a deterministic hash of the tool-specs registry content, + // generated into build/ by the package's build. Absent on a pre-#447 build (or + // the mocked loader in unit tests) — the stale-check below is a NO-OP when it + // is missing, so an older build never wrongly fails startup. + REGISTRY_STAMP?: string; +} + +/** + * Recompute the REGISTRY_STAMP (#447) from the @docmost/mcp source tree, if it is + * present. Returns the stamp string, or `null` when the source is absent (a prod + * image ships only build/, no src/). MUST stay byte-for-byte identical to + * packages/mcp/scripts/gen-registry-stamp.mjs's `computeRegistryStamp` so the + * build-time and src-time hashes agree: same input file (src/tool-specs.ts), same + * normalization (CRLF -> LF, strip a single trailing newline), same sha256. + * + * DEV vs PROD detection is by FILE EXISTENCE, not NODE_ENV: we resolve the + * package's own directory from `require.resolve('@docmost/mcp')` (which points at + * build/index.js) and look for ../src/tool-specs.ts next to it. In a dev/test + * worktree that file exists; in a prod image (build/ only, src/ stripped) it does + * not, so this returns null and the caller skips the check. Any error (ENOENT, a + * bad resolve) is swallowed to null — the stale-check must NEVER break startup. + * + * Exported for unit testing (docmost-client.loader.spec.ts): the export keyword + * is behaviourally a no-op — the module-internal caller `loadDocmostMcp` is + * unaffected. The test drives the null (no-src) path and asserts this + * normalize+sha256 stays identical to the codegen's `computeRegistryStamp`. + */ +export function computeSrcRegistryStamp(packageEntry: string): string | null { + try { + // packageEntry is /build/index.js; the source lives at /src/. + const toolSpecsPath = join( + dirname(dirname(packageEntry)), + 'src', + 'tool-specs.ts', + ); + if (!existsSync(toolSpecsPath)) return null; // prod: no src tree -> skip. + const source = readFileSync(toolSpecsPath, 'utf8'); + const normalized = source.replace(/\r\n/g, '\n').replace(/\n$/, ''); + return createHash('sha256').update(normalized, 'utf8').digest('hex'); + } catch { + // Never let a resolution/read hiccup break server startup — treat as "no + // src available" and skip the check (identical to the prod no-op path). + return null; + } } // TS with module:commonjs downlevels a literal `import()` to `require()`, which @@ -375,6 +422,23 @@ export async function loadDocmostMcp(): Promise<{ const mod = (await esmImport( pathToFileURL(entry).href, )) as DocmostMcpModule; + // #447 stale-build guard (dev/test only). The server loads the COMPILED + // build/ of @docmost/mcp, but the parity/tier guard tests read src/. If a + // tool spec is edited in src without rebuilding the package, build/ and src/ + // silently diverge and the running server serves the OLD tools. Here we + // recompute the stamp from src/tool-specs.ts and compare it to the stamp + // baked into build/. In PROD the src tree is absent (image ships build/ + // only), so computeSrcRegistryStamp returns null and this is a pure no-op. + const srcStamp = computeSrcRegistryStamp(entry); + if ( + srcStamp !== null && + typeof mod.REGISTRY_STAMP === 'string' && + srcStamp !== mod.REGISTRY_STAMP + ) { + throw new Error( + '@docmost/mcp build is stale (tool-specs changed since last build) — run: pnpm --filter @docmost/mcp build', + ); + } return mod; })().catch((err) => { // Do not cache a rejected import — allow the next call to retry. diff --git a/packages/mcp/package.json b/packages/mcp/package.json index 9418bfb3..66c6db75 100644 --- a/packages/mcp/package.json +++ b/packages/mcp/package.json @@ -13,10 +13,11 @@ "docmost-mcp": "./build/stdio.js" }, "scripts": { - "build": "tsc", + "gen:stamp": "node scripts/gen-registry-stamp.mjs", + "build": "node scripts/gen-registry-stamp.mjs && tsc", "start": "node build/stdio.js", - "watch": "tsc --watch", - "pretest": "tsc", + "watch": "node scripts/gen-registry-stamp.mjs && tsc --watch", + "pretest": "node scripts/gen-registry-stamp.mjs && tsc", "test": "node --test \"test/unit/*.test.mjs\" \"test/mock/*.test.mjs\"", "test:unit": "node --test \"test/unit/*.test.mjs\"", "test:mock": "node --test \"test/mock/*.test.mjs\"", diff --git a/packages/mcp/scripts/gen-registry-stamp.mjs b/packages/mcp/scripts/gen-registry-stamp.mjs new file mode 100644 index 00000000..ecf865a5 --- /dev/null +++ b/packages/mcp/scripts/gen-registry-stamp.mjs @@ -0,0 +1,67 @@ +// Codegen: emit src/registry-stamp.generated.ts with a REGISTRY_STAMP hash of +// the tool-specs REGISTRY CONTENT, so a build/ vs src/ skew (issue #447) is +// detectable at runtime. +// +// WHY hash the raw source text (not extracted structured data): +// SHARED_TOOL_SPECS carries `buildShape` functions (the input SCHEMAS) which are +// NOT serializable. The input schema is exactly one of the things that MUST stay +// in sync between build/ and src/, so we cannot drop it from the hash. Rather +// than probe zod with a fragile shim to reconstruct the schema shape, we hash the +// STABLE, deterministic source TEXT of tool-specs.ts. That text fully captures +// every field that must stay in sync — mcpName, inAppKey, description, tier, +// catalogLine AND the buildShape bodies (input schemas) — with zero probing +// fragility. Any edit to a spec (a renamed tool, a reworded description, a +// changed schema field) changes the text and therefore the stamp. +// +// DETERMINISM: the hash is computed over the file bytes with line endings +// normalized to LF and a single trailing newline stripped, so a CRLF checkout or +// an editor's trailing-newline habit cannot make build/ and src/ disagree. No +// Date.now / randomness. The loader's dev-only stale-check (docmost-client.loader.ts) +// re-runs THIS SAME normalization + sha256 over src/tool-specs.ts and compares to +// the built REGISTRY_STAMP; the two must compute identically. +// +// This script runs from the `build` and `pretest` npm scripts BEFORE tsc, so +// build/ always carries a stamp derived from the tool-specs.ts that was compiled. + +import { createHash } from 'node:crypto'; +import { readFileSync, writeFileSync } from 'node:fs'; +import { fileURLToPath } from 'node:url'; +import { dirname, join } from 'node:path'; + +const __dirname = dirname(fileURLToPath(import.meta.url)); +const SRC_DIR = join(__dirname, '..', 'src'); +const TOOL_SPECS_PATH = join(SRC_DIR, 'tool-specs.ts'); +const OUT_PATH = join(SRC_DIR, 'registry-stamp.generated.ts'); + +/** + * Deterministic stamp of the tool-specs registry content. Kept as a plain + * function (exported) so the algorithm has a single home; the loader duplicates + * only the tiny normalize+sha256 steps because it lives in the CJS server build + * and cannot import this ESM script. If you change the normalization here, mirror + * it in apps/server/src/core/ai-chat/tools/docmost-client.loader.ts. + */ +export function computeRegistryStamp(toolSpecsSource) { + const normalized = toolSpecsSource.replace(/\r\n/g, '\n').replace(/\n$/, ''); + return createHash('sha256').update(normalized, 'utf8').digest('hex'); +} + +function main() { + const source = readFileSync(TOOL_SPECS_PATH, 'utf8'); + const stamp = computeRegistryStamp(source); + const out = + '// AUTO-GENERATED by scripts/gen-registry-stamp.mjs — DO NOT EDIT BY HAND.\n' + + '// A deterministic hash of src/tool-specs.ts content (tool names, descriptions,\n' + + '// tiers, catalog lines and input schemas). Regenerated on every build/pretest\n' + + '// so build/ always matches the compiled src. The in-app loader recomputes this\n' + + '// from src and refuses to run on a mismatch (issue #447). This file is\n' + + '// gitignored and produced by the build — see .gitignore.\n' + + `export const REGISTRY_STAMP = ${JSON.stringify(stamp)};\n`; + writeFileSync(OUT_PATH, out, 'utf8'); + // eslint-disable-next-line no-console + console.log(`gen-registry-stamp: wrote ${OUT_PATH} (${stamp.slice(0, 12)}…)`); +} + +// Only run when invoked directly (not when imported for computeRegistryStamp). +if (process.argv[1] && fileURLToPath(import.meta.url) === process.argv[1]) { + main(); +} diff --git a/packages/mcp/src/index.ts b/packages/mcp/src/index.ts index 3c0078ec..1f6fa7ab 100644 --- a/packages/mcp/src/index.ts +++ b/packages/mcp/src/index.ts @@ -29,6 +29,14 @@ export { destroyAllSessions } from "./lib/collab-session.js"; export { SHARED_TOOL_SPECS } from "./tool-specs.js"; export type { SharedToolSpec } from "./tool-specs.js"; +// Re-export the build-time REGISTRY_STAMP (issue #447): a deterministic hash of +// the tool-specs registry content, generated into src/registry-stamp.generated.ts +// by scripts/gen-registry-stamp.mjs BEFORE tsc, so it lands in build/. The in-app +// loader recomputes the same hash from src/tool-specs.ts (dev/test only) and +// refuses to run on a mismatch, catching a build/ vs src/ skew (a spec edited in +// src without rebuilding the package the server actually loads from build/). +export { REGISTRY_STAMP } from "./registry-stamp.generated.js"; + // Re-export the shared "new comments: N" signal helper (#417) so the in-app // layer reads the SAME watermark/debounce/injection-safe line builder off the // loaded module (same pattern as SHARED_TOOL_SPECS). Both surfaces then differ diff --git a/packages/mcp/test/unit/registry-stamp.test.mjs b/packages/mcp/test/unit/registry-stamp.test.mjs new file mode 100644 index 00000000..b9d992c4 --- /dev/null +++ b/packages/mcp/test/unit/registry-stamp.test.mjs @@ -0,0 +1,101 @@ +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { createHash } from "node:crypto"; +import { readFileSync } from "node:fs"; +import { fileURLToPath } from "node:url"; +import { dirname, join } from "node:path"; + +import { computeRegistryStamp } from "../../scripts/gen-registry-stamp.mjs"; +import { REGISTRY_STAMP } from "../../build/index.js"; + +// Guard tests for the build/src-skew stamp (issue #447). The codegen script +// exports `computeRegistryStamp(sourceText)` — a sha256 over normalized source +// text (CRLF->LF, single trailing newline stripped). The in-app loader +// (apps/server/.../docmost-client.loader.ts) DUPLICATES that normalize+sha256 to +// recompute the stamp from src and refuse a stale build. These tests pin the +// algorithm's behaviour AND assert the built stamp matches the current src, so a +// stale generated file OR a normalize divergence reddens. + +const __dirname = dirname(fileURLToPath(import.meta.url)); +const TOOL_SPECS_PATH = join(__dirname, "..", "..", "src", "tool-specs.ts"); + +test("computeRegistryStamp is deterministic: same input -> same hash", () => { + const input = "export const X = 1;\nexport const Y = 2;\n"; + assert.equal(computeRegistryStamp(input), computeRegistryStamp(input)); +}); + +test("computeRegistryStamp returns a 64-char lowercase hex sha256", () => { + const stamp = computeRegistryStamp("anything"); + assert.match(stamp, /^[0-9a-f]{64}$/); +}); + +test("normalizes CRLF vs LF: the same content hashes equal", () => { + const lf = "line1\nline2\nline3"; + const crlf = "line1\r\nline2\r\nline3"; + assert.equal(computeRegistryStamp(crlf), computeRegistryStamp(lf)); +}); + +test("normalizes a trailing newline: with/without a final \\n hashes equal", () => { + const noTrailing = "alpha\nbeta"; + const trailing = "alpha\nbeta\n"; + assert.equal(computeRegistryStamp(trailing), computeRegistryStamp(noTrailing)); +}); + +test("a CRLF checkout WITH a trailing CRLF still hashes equal to bare LF", () => { + // A worst-case Windows checkout: CRLF line endings + a trailing CRLF. Both the + // \r\n->\n replace and the trailing-newline strip must apply for parity. + const bare = "alpha\nbeta"; + const crlfTrailing = "alpha\r\nbeta\r\n"; + assert.equal( + computeRegistryStamp(crlfTrailing), + computeRegistryStamp(bare), + ); +}); + +test("a real content change hashes differently", () => { + const before = "export const description = 'search a page';\n"; + const after = "export const description = 'search a PAGE';\n"; + assert.notEqual(computeRegistryStamp(before), computeRegistryStamp(after)); +}); + +// Only a SINGLE trailing newline is stripped — a second blank line is content and +// must change the hash. This pins the exact `/\n$/` semantics the loader mirrors. +test("only ONE trailing newline is stripped (two differ from one)", () => { + assert.notEqual( + computeRegistryStamp("x\n"), + computeRegistryStamp("x\n\n"), + ); +}); + +// Cross-impl equality against a fixed, documented input. The SAME literal input +// and expected hash are asserted in the server-side jest test +// (docmost-client.loader.spec.ts). If either side's normalize+sha256 ever +// diverges, one of the two tests reddens. Input exercises BOTH normalize steps. +test("fixed-input hash matches the documented cross-impl value", () => { + const FIXED_INPUT = "line1\r\nline2\n"; + const EXPECTED = + "683376e290829b482c2655745caffa7a1dccfa10afaa62dac2b42dd6c68d0f83"; + assert.equal(computeRegistryStamp(FIXED_INPUT), EXPECTED); +}); + +// DESYNC GUARD (covers reviewer suggestion 2). Recompute the stamp from the +// actual src/tool-specs.ts and assert it equals the REGISTRY_STAMP baked into the +// freshly-built build/index.js. This reddens if the generated file is stale OR if +// the codegen normalize ever diverges from what produced the built stamp. +test("built REGISTRY_STAMP equals the stamp recomputed from src/tool-specs.ts", () => { + const source = readFileSync(TOOL_SPECS_PATH, "utf8"); + assert.equal(computeRegistryStamp(source), REGISTRY_STAMP); +}); + +// Sanity: the fixed-input helper computes the SAME way the codegen does, proving +// the EXPECTED constant above is not an arbitrary magic value but the documented +// normalize+sha256 of FIXED_INPUT. Belt-and-braces so a bad EXPECTED can't hide a +// real regression. +test("the documented EXPECTED constant is the normalize+sha256 of FIXED_INPUT", () => { + const FIXED_INPUT = "line1\r\nline2\n"; + const normalized = FIXED_INPUT.replace(/\r\n/g, "\n").replace(/\n$/, ""); + const expected = createHash("sha256") + .update(normalized, "utf8") + .digest("hex"); + assert.equal(computeRegistryStamp(FIXED_INPUT), expected); +});