refactor(#193): tool-host drift-guard + staged plan (shared spec registry already merged) #249

Open
Ghost wants to merge 4 commits from refactor/193-tool-spec-registry into develop
2 changed files with 240 additions and 0 deletions

View File

@@ -5,6 +5,34 @@ import { pathToFileURL } from 'node:url';
* ESM-only `@docmost/mcp` package. We only need the constructor + the read/write * ESM-only `@docmost/mcp` package. We only need the constructor + the read/write
* methods used by the per-user tool adapter; the full client surface lives in * methods used by the per-user tool adapter; the full client surface lives in
* `packages/mcp/src/client.ts`. Signatures here mirror that file exactly. * `packages/mcp/src/client.ts`. Signatures here mirror that file exactly.
*
* DRIFT GUARD: the method NAMES below are runtime-checked against the real
* `DocmostClient` by `packages/mcp/test/unit/client-host-contract.test.mjs`
* (which can import the ESM class directly). If you rename/remove a method here
* or in client.ts, that test fails — so a stale mirror cannot silently ship a
* runtime "x is not a function" into an agent tool call. Keep the two in sync.
*
* STAGED PLAN — full derivation `DocmostClientLike = <real DocmostClient type>`
* (issue #193, layer 3) is intentionally NOT done; it stays a hand-mirror for
* now because of two verified blockers across the ESM(mcp)/CJS(server) boundary:
* 1. `@docmost/mcp` emits NO declaration files (its tsconfig has no
* `declaration`, package.json has no `types`/types-export) and the server
* tsconfig has no path mapping for it — the server only loads it via the
* runtime `import()` trick below, so there is no type to import today.
* 2. The real client methods have inferred, CONCRETE return types; the in-app
* tool adapter reads results through loose `Record<string,unknown>` returns
* + `as` casts (e.g. `(result?.data ?? {}) as { title?: string }`).
* Deriving the exact type would make those casts non-overlapping ("may be a
* mistake") and break the build, and `Partial<DocmostClientLike>` test stubs
* would have to satisfy the full concrete surface.
* To do it safely later (incrementally): (a) turn on `declaration: true` in
* packages/mcp/tsconfig.json + add a `types` export condition and commit the
* emitted `.d.ts`; (b) `import type { DocmostClient } from '@docmost/mcp'` here
* and replace this interface with a `Pick<DocmostClient, ...>` of the consumed
* methods; (c) audit every `as` cast in ai-chat-tools.service.ts against the now
* concrete return types (double-cast through `unknown` only where genuinely
* needed); (d) keep the runtime guard test as a belt-and-braces check. Until
* then the guard test above is the cheap, behaviour-neutral protection.
*/ */
export interface DocmostClientLike { export interface DocmostClientLike {
// --- read --- // --- read ---

View File

@@ -0,0 +1,212 @@
import { test } from "node:test";
import assert from "node:assert/strict";
import { readFileSync } from "node:fs";
import { fileURLToPath } from "node:url";
import { dirname, resolve } from "node:path";
import { DocmostClient } from "../../build/index.js";
// Drift guard for the THIRD hand-written layer of the AI tool set (issue #193,
// layer 3): the in-app server hand-mirrors the DocmostClient method signatures
// it consumes as the `DocmostClientLike` interface in
// apps/server/src/core/ai-chat/tools/docmost-client.loader.ts ("Signatures here
// mirror that file exactly"). That mirror lives across the ESM(mcp)/CJS(server)
// boundary and the package ships NO .d.ts, so the server typecheck cannot verify
// the names against the real class — a rename/removal in client.ts would surface
// only as a runtime "x is not a function" inside an agent tool call.
//
// SCOPE: this guard checks the method-NAME set only, not signatures. It pins the
// contract from the mcp side (ESM, where the real class is directly importable):
// every method the embedding host depends on MUST exist as a function on a real
// DocmostClient instance. If you rename/remove a client method, this fails here
// AND you must update DocmostClientLike to match. It does NOT verify parameter or
// return-type parity — signature drift between the hand-mirror and client.ts can
// still ship silently; full signature/type parity is the deferred staged-plan
// item below.
//
// Keep the HOST_CONTRACT_METHODS NAME list aligned with the method NAMES declared
// in the server's DocmostClientLike interface (the in-app per-user tool adapter
// only — it is a SUBSET of the DocmostClient surface — covers only what the in-app adapter
// consumes; the standalone MCP transport (packages/mcp/src/index.ts) calls additional
// client methods (insertImage/replaceImage/deleteComment/updateComment/insertFootnote/
// uploadImage) that this guard does NOT track — the MCP transport's own typecheck covers those). Full type-derivation
// of DocmostClientLike from this class is deferred (see the staged plan in
// docmost-client.loader.ts): the package emits no declarations and the real
// (inferred, concrete) return types conflict with the host's loose
// `Record<string,unknown>` + `as`-cast result handling.
const HOST_CONTRACT_METHODS = [
// read
"search",
"getPage",
"getWorkspace",
"getSpaces",
"listPages",
"listSidebarPages",
"getOutline",
"getPageJson",
"getNode",
"getTable",
"listComments",
"getComment",
"checkNewComments",
"listShares",
"listPageHistory",
"getPageHistory",
"diffPageVersions",
"exportPageMarkdown",
// write (page)
"createPage",
"updatePage",
"renamePage",
"movePage",
"deletePage",
"editPageText",
"patchNode",
"insertNode",
"deleteNode",
"updatePageJson",
"tableInsertRow",
"tableDeleteRow",
"tableUpdateCell",
"copyPageContent",
"importPageMarkdown",
"sharePage",
"unsharePage",
"restorePageVersion",
"transformPage",
// write (comment)
"createComment",
"resolveComment",
];
test("DocmostClient implements every method the in-app DocmostClientLike mirror declares", () => {
// The constructor is side-effect-free (no network/login on construction): it
// only stores config and creates an axios instance, so it is safe to build a
// throwaway instance here with a dummy token provider.
const client = new DocmostClient({
apiUrl: "http://127.0.0.1:1/api",
getToken: async () => "test-token",
});
const missing = HOST_CONTRACT_METHODS.filter(
(name) => typeof client[name] !== "function",
);
assert.deepEqual(
missing,
[],
`DocmostClient is missing host-contract method(s): ${missing.join(", ")}. ` +
`Update packages/mcp/src/client.ts and/or the server's DocmostClientLike ` +
`interface (apps/server/src/core/ai-chat/tools/docmost-client.loader.ts) ` +
`so the hand-mirrored method NAMES stay aligned (this guards names only, ` +
`not signatures).`,
);
});
test("HOST_CONTRACT_METHODS has no duplicates", () => {
assert.equal(
new Set(HOST_CONTRACT_METHODS).size,
HOST_CONTRACT_METHODS.length,
);
});
// Parse the method names declared in the server's `DocmostClientLike` interface
// body. We read the .ts source as plain text (no TS compiler dep, and the file
// lives in the CJS server tree across the ESM boundary): scan from the
// `export interface DocmostClientLike {` line to its closing brace at column 0,
// matching member-signature lines like ` methodName(`. Nested param-object
// braces (`opts: { ... }`) are indented, so only the interface's own closing
// `}` (column 0) ends the scan.
function parseDocmostClientLikeMethods() {
const here = dirname(fileURLToPath(import.meta.url));
// packages/mcp/test/unit -> repo root is four levels up.
const loaderPath = resolve(
here,
"../../../../apps/server/src/core/ai-chat/tools/docmost-client.loader.ts",
);
let source;
try {
source = readFileSync(loaderPath, "utf8");
} catch (err) {
if (err && err.code === "ENOENT") {
throw new Error(
`Expected monorepo layout; server tree at ${loaderPath} not found. ` +
`This drift-guard reads the server's DocmostClientLike interface via a ` +
`fixed relative path and must run from inside the monorepo checkout.`,
);
}
throw err;
}
const lines = source.split(/\r?\n/);
const startIdx = lines.findIndex((l) =>
/^export interface DocmostClientLike\s*\{/.test(l),
);
assert.notEqual(
startIdx,
-1,
`Could not find "export interface DocmostClientLike {" in ${loaderPath}. ` +
`If the interface was renamed/moved, update this drift-guard test.`,
);
const methods = [];
let closed = false;
// Track whether we are inside a `/* ... */` block comment. Inner lines of a
// block comment need NOT start with `*`, so a `name(` line inside one would be
// falsely parsed as an interface method without this. (`//` line comments can
// never match the method regex below since they start with `/`.)
let inBlockComment = false;
for (let i = startIdx + 1; i < lines.length; i++) {
const line = lines[i];
if (inBlockComment) {
// Stay in the block until we see its closing `*/`.
if (line.includes("*/")) inBlockComment = false;
continue;
}
// Enter a block comment only when it opens without closing on the same line;
// a self-contained `/* ... */` on one line cannot precede a method name we
// care about (such lines start with `/`, so the method regex won't match).
if (line.includes("/*") && !line.includes("*/")) {
inBlockComment = true;
continue;
}
if (/^\}/.test(line)) {
closed = true;
break;
}
// Method-name match: a TS identifier (letters/digits/`_`/`$`, not starting
// with a digit) optionally followed by a generic clause (`method<T>(`), then
// the opening paren of the signature.
const m = /^\s*([A-Za-z_$][A-Za-z0-9_$]*)\s*(?:<[^>]*>)?\(/.exec(line);
if (m) methods.push(m[1]);
}
assert.ok(
closed,
`Did not find the closing brace of DocmostClientLike in ${loaderPath}.`,
);
assert.ok(
methods.length > 0,
`Parsed zero methods from DocmostClientLike in ${loaderPath} — the parser ` +
`is likely out of date with the interface formatting.`,
);
return methods;
}
// The point of the guard is to protect the DocmostClientLike mirror <-> client.ts
// link, but HOST_CONTRACT_METHODS is itself a HAND-COPY of that interface kept in
// sync manually. The list<->interface link must be tested too: a method consumed
// by the adapter and added to DocmostClientLike but forgotten here (or removed
// from the interface but left here) would otherwise escape both the server
// typecheck (pkg emits no .d.ts) and the first test above (name not in the list).
// Assert the two agree BOTH ways.
test("HOST_CONTRACT_METHODS exactly mirrors the server's DocmostClientLike interface", () => {
const interfaceMethods = parseDocmostClientLikeMethods();
assert.deepEqual(
[...HOST_CONTRACT_METHODS].sort(),
[...interfaceMethods].sort(),
`HOST_CONTRACT_METHODS has drifted from the DocmostClientLike interface in ` +
`apps/server/src/core/ai-chat/tools/docmost-client.loader.ts. Add/remove ` +
`method names in HOST_CONTRACT_METHODS so it lists EXACTLY the methods ` +
`declared in that interface (both directions are checked).`,
);
});