Compare commits
3 Commits
| Author | SHA1 | Date | |
|---|---|---|---|
| 2e6f1c3de5 | |||
| f8d37d8956 | |||
| 90168eb926 |
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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 `<pkg>/build/index.js` + optional `<pkg>/src/tool-specs.ts`
|
||||
// layout so `computeSrcRegistryStamp(<pkg>/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();
|
||||
}
|
||||
});
|
||||
});
|
||||
@@ -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 <pkg>/build/index.js; the source lives at <pkg>/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.
|
||||
|
||||
@@ -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\"",
|
||||
|
||||
@@ -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();
|
||||
}
|
||||
+169
-268
@@ -197,166 +197,6 @@ function readCollabTokenTtlMs(): number {
|
||||
return Number.isFinite(raw) ? Math.max(0, raw) : 5 * 60 * 1000;
|
||||
}
|
||||
|
||||
// --- Issue #437: central error diagnostics -------------------------------
|
||||
// The agent only ever sees the thrown exception's `error.message`, so a failed
|
||||
// tool must return an ACTIONABLE message (method, path, status, and the
|
||||
// server's own validation text) instead of the opaque "Request failed with
|
||||
// status code 400". These helpers + the response interceptor in the
|
||||
// constructor are the single authoritative place that text is composed.
|
||||
|
||||
// Overall cap on the composed diagnostic message so the model context stays
|
||||
// compact and a (whitelisted) server string can never blow up the text.
|
||||
const ERROR_MESSAGE_CAP = 300;
|
||||
// Only attempt to JSON.parse an arraybuffer body under this size: a larger
|
||||
// binary body is never a JSON error envelope, so parsing it just wastes memory
|
||||
// (fetchInternalFile uses responseType:"arraybuffer", so a failed file fetch
|
||||
// carries the JSON error envelope as raw bytes here).
|
||||
const ERROR_BUFFER_PARSE_CAP = 4096;
|
||||
|
||||
// Canonical 36-char UUID (8-4-4-4-12 hex). Deliberately version/variant-
|
||||
// AGNOSTIC: the ids are UUIDv7 (e.g. 019f499a-9f8c-7d68-...), so only the
|
||||
// canonical shape/length is enforced, not the version/variant nibble.
|
||||
const FULL_UUID_RE =
|
||||
/^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i;
|
||||
|
||||
/**
|
||||
* Throw an actionable error BEFORE any network call when `value` is not a full
|
||||
* canonical UUID. Absorbs #436: a truncated/short comment id used to reach the
|
||||
* server and bounce back as an opaque 400/404 the agent could not self-correct;
|
||||
* failing fast here names the exact fix.
|
||||
*/
|
||||
export function assertFullUuid(
|
||||
tool: string,
|
||||
param: string,
|
||||
value: string,
|
||||
): void {
|
||||
if (typeof value !== "string" || !FULL_UUID_RE.test(value)) {
|
||||
throw new Error(
|
||||
`${tool}: '${param}' must be the FULL comment UUID (36 chars, e.g. ` +
|
||||
`019f499a-9f8c-7d68-b7be-ce100d7c6c56), got '${value}'. Copy the id ` +
|
||||
`verbatim from list_comments / create_comment output.`,
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
// Keep ONLY the pathname of a request (no host, no query string, no fragment)
|
||||
// so the message never leaks a host or query params. Resolves a relative
|
||||
// config.url against config.baseURL, then discards everything but the path.
|
||||
function requestPath(config: any): string {
|
||||
const rawUrl = typeof config?.url === "string" ? config.url : "";
|
||||
const base =
|
||||
typeof config?.baseURL === "string" ? config.baseURL : undefined;
|
||||
try {
|
||||
// A dummy base makes an absolute config.url parse too; its host is dropped.
|
||||
return new URL(rawUrl, base ?? "http://localhost").pathname;
|
||||
} catch {
|
||||
// Malformed url: still strip any query/fragment manually.
|
||||
return rawUrl.split(/[?#]/)[0] || rawUrl;
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Compose the server-facing message from `error.response.data`, using ONLY the
|
||||
* whitelisted `message`/`error` fields or the HTTP statusText. SECURITY: the
|
||||
* raw response body, headers (Authorization!) and config are NEVER read here —
|
||||
* a string/HTML body (e.g. a proxy's 502 page) is deliberately dropped in
|
||||
* favour of the statusText.
|
||||
*/
|
||||
function extractServerMessage(data: any, statusText: string): string {
|
||||
// class-validator envelope: { message: string | string[], error?: string }.
|
||||
if (
|
||||
data &&
|
||||
typeof data === "object" &&
|
||||
!Buffer.isBuffer(data) &&
|
||||
!(data instanceof ArrayBuffer)
|
||||
) {
|
||||
const msg = (data as any).message;
|
||||
if (Array.isArray(msg)) {
|
||||
const joined = msg.filter((m) => typeof m === "string").join("; ");
|
||||
if (joined) return joined;
|
||||
} else if (typeof msg === "string" && msg) {
|
||||
return msg;
|
||||
}
|
||||
const err = (data as any).error;
|
||||
if (typeof err === "string" && err) return err;
|
||||
return statusText;
|
||||
}
|
||||
|
||||
// Buffer / ArrayBuffer body: attempt a size-capped, guarded JSON.parse so a
|
||||
// failed arraybuffer fetch still surfaces the server's validation text.
|
||||
if (Buffer.isBuffer(data) || data instanceof ArrayBuffer) {
|
||||
const buf = Buffer.isBuffer(data) ? data : Buffer.from(data);
|
||||
if (buf.length > 0 && buf.length <= ERROR_BUFFER_PARSE_CAP) {
|
||||
try {
|
||||
return extractServerMessage(JSON.parse(buf.toString("utf8")), statusText);
|
||||
} catch {
|
||||
return statusText;
|
||||
}
|
||||
}
|
||||
return statusText;
|
||||
}
|
||||
|
||||
// A raw string / HTML body is never surfaced (may echo server internals).
|
||||
return statusText;
|
||||
}
|
||||
|
||||
/**
|
||||
* Reformat an AxiosError's `.message` IN PLACE into an actionable diagnostic:
|
||||
* `<METHOD> <path> failed (<status> <statusText>): <serverMessage>`
|
||||
* or, when the request never got a response:
|
||||
* `<METHOD> <path> failed: <code> (no response from server)`.
|
||||
*
|
||||
* Mutates the SAME error object (never a custom subclass) so the live
|
||||
* axios.isAxiosError / error.response?.status / config._retry checks around the
|
||||
* client keep working, and sets `_docmostFormatted` as a double-processing
|
||||
* guard. A no-op on a non-axios or already-formatted error.
|
||||
*/
|
||||
export function formatDocmostAxiosError(error: any): void {
|
||||
if (!error || error._docmostFormatted) return;
|
||||
if (!axios.isAxiosError(error)) return;
|
||||
|
||||
const config: any = error.config ?? {};
|
||||
const method =
|
||||
typeof config.method === "string" ? config.method.toUpperCase() : "";
|
||||
const methodPath = `${method} ${requestPath(config)}`.trim();
|
||||
const response = error.response;
|
||||
|
||||
let message: string;
|
||||
if (response) {
|
||||
const statusText =
|
||||
typeof response.statusText === "string" ? response.statusText : "";
|
||||
const serverMessage = extractServerMessage(response.data, statusText);
|
||||
message = `${methodPath} failed (${response.status} ${statusText}): ${serverMessage}`;
|
||||
// Full body only to stderr under DEBUG (parity with downloadImage).
|
||||
if (process.env.DEBUG) {
|
||||
console.error(
|
||||
"Docmost request failed; response body:",
|
||||
JSON.stringify(response.data),
|
||||
);
|
||||
}
|
||||
} else {
|
||||
// No response at all (ECONNREFUSED / ETIMEDOUT / ECONNRESET / DNS / timeout).
|
||||
// Use ONLY error.code, never the raw error.message: axios network messages
|
||||
// embed host:port ("connect ECONNREFUSED 127.0.0.1:3000", "getaddrinfo
|
||||
// ENOTFOUND host") and #437's invariant is that the host never reaches the
|
||||
// model-visible message. code is set for essentially every real no-response
|
||||
// error (ECONNREFUSED/ETIMEDOUT/ECONNRESET/ENOTFOUND/ECONNABORTED); the full
|
||||
// native message still goes to stderr under DEBUG.
|
||||
const reason = error.code ?? "network error";
|
||||
message = `${methodPath} failed: ${reason} (no response from server)`;
|
||||
if (process.env.DEBUG) {
|
||||
console.error("Docmost request failed; no response:", error.message);
|
||||
}
|
||||
}
|
||||
|
||||
if (message.length > ERROR_MESSAGE_CAP) {
|
||||
message = message.slice(0, ERROR_MESSAGE_CAP - 1) + "…";
|
||||
}
|
||||
|
||||
error.message = message;
|
||||
(error as any)._docmostFormatted = true;
|
||||
}
|
||||
|
||||
export class DocmostClient {
|
||||
private client: AxiosInstance;
|
||||
private token: string | null = null;
|
||||
@@ -497,22 +337,6 @@ export class DocmostClient {
|
||||
return Promise.reject(error);
|
||||
},
|
||||
);
|
||||
|
||||
// Diagnostics interceptor (issue #437). Registered AFTER the re-login
|
||||
// interceptor so a successful re-login retry (which resolves to a real
|
||||
// response) is never seen here as an error; only a genuine failure reaches
|
||||
// this rejection handler. It reformats error.message IN PLACE (see
|
||||
// formatDocmostAxiosError — kept as a mutation, not a custom Error class, so
|
||||
// the surrounding axios.isAxiosError / error.response?.status / config._retry
|
||||
// checks keep working) and re-rejects the SAME error. The _docmostFormatted
|
||||
// flag makes a re-processed retry-failure a no-op.
|
||||
this.client.interceptors.response.use(
|
||||
(response) => response,
|
||||
(error) => {
|
||||
formatDocmostAxiosError(error);
|
||||
return Promise.reject(error);
|
||||
},
|
||||
);
|
||||
}
|
||||
|
||||
/** Application base URL (API URL without the /api suffix). */
|
||||
@@ -729,16 +553,18 @@ export class DocmostClient {
|
||||
// forever and accumulate duplicates).
|
||||
const MAX_PAGES = 50;
|
||||
|
||||
let page = 1;
|
||||
let cursor: string | undefined;
|
||||
let allItems: T[] = [];
|
||||
let hasNextPage = true;
|
||||
let truncated = false;
|
||||
|
||||
while (hasNextPage && page <= MAX_PAGES) {
|
||||
const response = await this.client.post(endpoint, {
|
||||
for (let page = 0; page < MAX_PAGES; page++) {
|
||||
const payload: Record<string, any> = {
|
||||
...basePayload,
|
||||
limit: clampedLimit,
|
||||
page,
|
||||
});
|
||||
};
|
||||
if (cursor) payload.cursor = cursor;
|
||||
|
||||
const response = await this.client.post(endpoint, payload);
|
||||
|
||||
const data = response.data;
|
||||
const items = data.data?.items || data.items || [];
|
||||
@@ -746,22 +572,28 @@ export class DocmostClient {
|
||||
|
||||
allItems = allItems.concat(items);
|
||||
|
||||
// Stop if the page is empty or shorter than the requested size: a full
|
||||
// page worth of items is the only situation where another page can exist,
|
||||
// so this defends against a stuck hasNextPage flag in addition to it.
|
||||
if (items.length === 0 || items.length < clampedLimit) {
|
||||
// Advance strictly via the server-issued cursor. A missing nextCursor (or
|
||||
// hasNextPage false) means we reached the end. A cursor identical to the
|
||||
// one we just sent means the server did not understand our pagination
|
||||
// param — stop instead of re-fetching page one forever and duplicating.
|
||||
const next = meta?.hasNextPage ? meta?.nextCursor : null;
|
||||
if (!next || next === cursor) {
|
||||
// If the server still reports more pages but stopped issuing a usable
|
||||
// cursor at the ceiling, flag the result as truncated below.
|
||||
if (page === MAX_PAGES - 1 && meta?.hasNextPage) truncated = true;
|
||||
break;
|
||||
}
|
||||
cursor = next;
|
||||
|
||||
hasNextPage = meta?.hasNextPage || false;
|
||||
page++;
|
||||
// Reaching the ceiling with more pages still available means the result
|
||||
// set is truncated.
|
||||
if (page === MAX_PAGES - 1) truncated = true;
|
||||
}
|
||||
|
||||
// If the loop stopped because it hit the MAX_PAGES ceiling while the server
|
||||
// still reported more results (hasNextPage true and the last page was
|
||||
// full), the result set is truncated — warn so the caller is not silently
|
||||
// handed an incomplete list.
|
||||
if (hasNextPage && page > MAX_PAGES) {
|
||||
// still reported more results, the result set is truncated — warn so the
|
||||
// caller is not silently handed an incomplete list.
|
||||
if (truncated) {
|
||||
console.warn(
|
||||
`paginateAll: results from "${endpoint}" truncated at the ${MAX_PAGES}-page cap; more pages exist on the server`,
|
||||
);
|
||||
@@ -795,9 +627,10 @@ export class DocmostClient {
|
||||
* Tree (`tree` true): the space's FULL page hierarchy as a nested tree (each
|
||||
* node has a `children` array). This mode REQUIRES `spaceId` (a page tree is
|
||||
* scoped to one space) and IGNORES `limit` — the whole hierarchy is returned.
|
||||
* It walks the sidebar tree via `enumerateSpacePages`, which performs N
|
||||
* sidebar requests and is bounded by that method's 10000-node cap (and skips
|
||||
* soft-deleted pages server-side).
|
||||
* It fetches the tree via `enumerateSpacePages`, which on the fork server
|
||||
* resolves to a single `/pages/tree` request returning the whole
|
||||
* permission-filtered flat page set (soft-deleted pages excluded
|
||||
* server-side).
|
||||
*/
|
||||
async listPages(spaceId?: string, limit: number = 50, tree: boolean = false) {
|
||||
await this.ensureAuthenticated();
|
||||
@@ -808,8 +641,8 @@ export class DocmostClient {
|
||||
"list_pages: tree mode requires a spaceId (a page tree is scoped to one space). Pass spaceId, or omit tree to get the recent-pages list.",
|
||||
);
|
||||
}
|
||||
const nodes = await this.enumerateSpacePages(spaceId);
|
||||
return buildPageTree(nodes);
|
||||
const { pages } = await this.enumerateSpacePages(spaceId);
|
||||
return buildPageTree(pages);
|
||||
}
|
||||
|
||||
const clampedLimit = Math.max(1, Math.min(100, limit));
|
||||
@@ -831,57 +664,123 @@ export class DocmostClient {
|
||||
async listSidebarPages(spaceId: string, pageId?: string) {
|
||||
await this.ensureAuthenticated();
|
||||
|
||||
// Paginate: the endpoint returns server-paged children, so posting only
|
||||
// { page: 1 } silently dropped every child beyond the first page. Loop on
|
||||
// meta.hasNextPage (with a MAX_PAGES ceiling like paginateAll, guarding
|
||||
// against a stuck hasNextPage flag) and accumulate all children.
|
||||
// Paginate via the server-issued cursor. The server switched from OFFSET
|
||||
// (`page`) to CURSOR (`cursor`/`nextCursor`) pagination, and the global
|
||||
// ValidationPipe(whitelist:true) SILENTLY STRIPS the obsolete `page` field
|
||||
// — so the old offset loop got the SAME first page every time (with
|
||||
// hasNextPage stuck true) and dropped every child beyond the first page.
|
||||
const MAX_PAGES = 50;
|
||||
let page = 1;
|
||||
let cursor: string | undefined;
|
||||
let allItems: any[] = [];
|
||||
let hasNextPage = true;
|
||||
let truncated = false;
|
||||
|
||||
while (hasNextPage && page <= MAX_PAGES) {
|
||||
for (let i = 0; i < MAX_PAGES; i++) {
|
||||
// limit: 100 is the server-side Max; cuts request count 5x vs the default 20.
|
||||
const payload: Record<string, any> = { spaceId, limit: 100 };
|
||||
// Only send pageId when scoping to a page's children; omit it for roots.
|
||||
const payload: Record<string, any> = { spaceId, page };
|
||||
if (pageId) payload.pageId = pageId;
|
||||
if (cursor) payload.cursor = cursor;
|
||||
|
||||
const response = await this.client.post("/pages/sidebar-pages", payload);
|
||||
const data = response.data?.data ?? response.data;
|
||||
const items = data?.items || [];
|
||||
allItems = allItems.concat(items);
|
||||
const data = (await this.client.post("/pages/sidebar-pages", payload)).data
|
||||
?.data;
|
||||
allItems = allItems.concat(data?.items ?? []);
|
||||
|
||||
hasNextPage = data?.meta?.hasNextPage || false;
|
||||
page++;
|
||||
// Advance strictly via the server-issued cursor; a missing/repeated cursor
|
||||
// means the protocol drifted again — stop instead of looping on page one.
|
||||
const next = data?.meta?.hasNextPage ? data?.meta?.nextCursor : null;
|
||||
if (!next || next === cursor) break;
|
||||
cursor = next;
|
||||
|
||||
// Reaching the ceiling with more pages still available means the child
|
||||
// list is truncated (mirrors paginateAll).
|
||||
if (i === MAX_PAGES - 1) truncated = true;
|
||||
}
|
||||
|
||||
// Warn on real truncation (ceiling hit while the server still had pages) so
|
||||
// the caller is not silently handed an incomplete child list.
|
||||
if (truncated) {
|
||||
console.warn(
|
||||
`listSidebarPages: children of "${pageId ?? spaceId}" truncated at the ${MAX_PAGES}-page cap; more pages exist on the server`,
|
||||
);
|
||||
}
|
||||
|
||||
return allItems;
|
||||
}
|
||||
|
||||
/**
|
||||
* Enumerate EVERY page in a space (or in a subtree, when rootPageId is given)
|
||||
* by walking the sidebar-pages tree.
|
||||
* Enumerate EVERY page in a space (or in a subtree, when rootPageId is given).
|
||||
*
|
||||
* Starting set: the children of rootPageId when provided, otherwise the
|
||||
* space root pages. From there it does an iterative breadth-first walk: each
|
||||
* node is collected, and when node.hasChildren is true its direct children
|
||||
* are fetched via listSidebarPages(spaceId, node.id) and enqueued.
|
||||
* Primary path (fork server): a SINGLE `POST /pages/tree` returns the whole
|
||||
* space (or a subtree) as a flat, permission-filtered list in one request, in
|
||||
* the exact node shape buildPageTree consumes. This replaces the old
|
||||
* per-node BFS, which issued N sidebar requests and — after the server moved
|
||||
* to cursor pagination — silently lost every child past the first sidebar
|
||||
* page (the obsolete `page` param was stripped by ValidationPipe).
|
||||
*
|
||||
* This replaces the old "/pages/recent" enumeration, which is a bounded
|
||||
* recent-activity feed (~5000 cap) and therefore misses comments on older
|
||||
* pages that were never recently touched.
|
||||
* The subtree variant (rootPageId given) INCLUDES the root node itself
|
||||
* (getPageAndDescendants seeds with id = rootPageId), unlike the old BFS
|
||||
* which started from the root's children.
|
||||
*
|
||||
* Safeguards: a `visited` Set of page ids prevents re-processing a node
|
||||
* (cycles / duplicate references), and a hard node cap bounds pathological
|
||||
* trees so the walk always terminates.
|
||||
* Fallback path (stdio mode may target STOCK upstream Docmost, which lacks
|
||||
* `/pages/tree`): on a 404/405 it falls back to the cursor-based BFS below,
|
||||
* walking direct children via the fixed cursor listSidebarPages. Safeguards:
|
||||
* a `visited` Set of page ids prevents re-processing a node (cycles /
|
||||
* duplicate references), and a hard node cap bounds pathological trees so the
|
||||
* walk always terminates.
|
||||
*
|
||||
* Returns `{ pages, truncated }`. `truncated` is true ONLY when the fallback
|
||||
* BFS stopped at its MAX_NODES cap — the primary /pages/tree path is uncapped
|
||||
* and always returns the complete set, so it never reports truncation.
|
||||
*/
|
||||
private async enumerateSpacePages(
|
||||
spaceId: string,
|
||||
rootPageId?: string,
|
||||
): Promise<any[]> {
|
||||
): Promise<{ pages: any[]; truncated: boolean }> {
|
||||
await this.ensureAuthenticated();
|
||||
|
||||
// Single request replaces the whole BFS: /pages/tree returns the full
|
||||
// permission-filtered flat page set of a space (or a subtree) at once. This
|
||||
// path is uncapped, so it is never truncated.
|
||||
const payload = rootPageId ? { pageId: rootPageId } : { spaceId };
|
||||
try {
|
||||
const response = await this.client.post("/pages/tree", payload);
|
||||
const pages = (response.data?.data ?? response.data)?.items ?? [];
|
||||
return { pages, truncated: false };
|
||||
} catch (e: any) {
|
||||
// Only fall back when the endpoint is absent (stock upstream Docmost);
|
||||
// any other error is a genuine failure and must propagate.
|
||||
if (
|
||||
!axios.isAxiosError(e) ||
|
||||
(e.response?.status !== 404 && e.response?.status !== 405)
|
||||
) {
|
||||
throw e;
|
||||
}
|
||||
}
|
||||
|
||||
// Fallback: cursor-based breadth-first walk via listSidebarPages.
|
||||
const MAX_NODES = 10000;
|
||||
const result: any[] = [];
|
||||
const visited = new Set<string>();
|
||||
|
||||
// Seed with the root node itself when scoping to a subtree, so its own
|
||||
// comments aren't dropped: the primary /pages/tree seeds
|
||||
// getPageAndDescendants with id = rootPageId (root included), but
|
||||
// listSidebarPages(spaceId, rootPageId) returns only the root's CHILDREN.
|
||||
// The `visited` set below prevents a double-add if the root also appears
|
||||
// among the children. getPageRaw returns a page whose id/title/spaceId are
|
||||
// exactly what buildPageTree and check_new_comments consume.
|
||||
if (rootPageId) {
|
||||
try {
|
||||
const root = await this.getPageRaw(rootPageId);
|
||||
if (root?.id) {
|
||||
result.push(root);
|
||||
visited.add(root.id);
|
||||
}
|
||||
} catch {
|
||||
// Non-fatal: if the root can't be read, fall through to children-only.
|
||||
}
|
||||
}
|
||||
|
||||
// Seed the queue with the starting level (subtree children or roots).
|
||||
const queue: any[] = await this.listSidebarPages(spaceId, rootPageId);
|
||||
|
||||
@@ -906,7 +805,12 @@ export class DocmostClient {
|
||||
}
|
||||
}
|
||||
|
||||
return result;
|
||||
// Truncated only when the cap was hit with the queue still non-empty (real
|
||||
// truncation, not a natural end at exactly MAX_NODES).
|
||||
return {
|
||||
pages: result,
|
||||
truncated: result.length >= MAX_NODES && queue.length > 0,
|
||||
};
|
||||
}
|
||||
|
||||
/** Raw page info including the ProseMirror JSON content and slugId. */
|
||||
@@ -2540,7 +2444,13 @@ export class DocmostClient {
|
||||
let allComments: any[] = [];
|
||||
let cursor: string | null = null;
|
||||
|
||||
do {
|
||||
// Hard ceiling + immovable-cursor guard (mirrors paginateAll): if /comments
|
||||
// ever stops advancing the cursor (the exact #442 drift scenario) this loop
|
||||
// would otherwise spin forever accumulating duplicates.
|
||||
const MAX_PAGES = 50;
|
||||
let truncated = false;
|
||||
|
||||
for (let page = 0; page < MAX_PAGES; page++) {
|
||||
const payload: Record<string, any> = { pageId, limit: 100 };
|
||||
if (cursor) payload.cursor = cursor;
|
||||
|
||||
@@ -2548,8 +2458,23 @@ export class DocmostClient {
|
||||
const data = response.data.data || response.data;
|
||||
const items = data.items || [];
|
||||
allComments = allComments.concat(items);
|
||||
cursor = data.meta?.nextCursor || null;
|
||||
} while (cursor);
|
||||
|
||||
// Advance strictly via the server-issued cursor. A missing nextCursor or a
|
||||
// cursor identical to the one we just sent means the end (or a server that
|
||||
// ignores our pagination param) — stop instead of re-fetching page one.
|
||||
const next: string | null = data.meta?.nextCursor || null;
|
||||
if (!next || next === cursor) break;
|
||||
cursor = next;
|
||||
|
||||
// Reaching the ceiling with a still-advancing cursor means truncation.
|
||||
if (page === MAX_PAGES - 1) truncated = true;
|
||||
}
|
||||
|
||||
if (truncated) {
|
||||
console.warn(
|
||||
`listComments: comments for "${pageId}" truncated at the ${MAX_PAGES}-page cap; more pages exist on the server`,
|
||||
);
|
||||
}
|
||||
|
||||
const mapped = allComments.map((comment: any) => {
|
||||
const markdown = comment.content
|
||||
@@ -2587,8 +2512,6 @@ export class DocmostClient {
|
||||
}
|
||||
|
||||
async getComment(commentId: string) {
|
||||
// Fail fast (#436): reject a truncated id before any network call.
|
||||
assertFullUuid("get_comment", "commentId", commentId);
|
||||
await this.ensureAuthenticated();
|
||||
const response = await this.client.post("/comments/info", { commentId });
|
||||
const comment = response.data.data || response.data;
|
||||
@@ -2678,12 +2601,6 @@ export class DocmostClient {
|
||||
parentCommentId?: string,
|
||||
suggestedText?: string,
|
||||
) {
|
||||
// Fail fast (#436): a provided parent id must be a full UUID before any
|
||||
// network call. Validate only when truthy — a falsy parentCommentId means
|
||||
// "top-level comment" (mirrors the isReply computation below), not a reply.
|
||||
if (parentCommentId) {
|
||||
assertFullUuid("create_comment", "parentCommentId", parentCommentId);
|
||||
}
|
||||
await this.ensureAuthenticated();
|
||||
|
||||
const isReply = !!parentCommentId;
|
||||
@@ -2966,8 +2883,6 @@ export class DocmostClient {
|
||||
}
|
||||
|
||||
async updateComment(commentId: string, content: string) {
|
||||
// Fail fast (#436): reject a truncated id before any network call.
|
||||
assertFullUuid("update_comment", "commentId", commentId);
|
||||
await this.ensureAuthenticated();
|
||||
// NON-canonicalizing on purpose (comment body — see createComment).
|
||||
const jsonContent = await markdownToProseMirror(content);
|
||||
@@ -2983,8 +2898,6 @@ export class DocmostClient {
|
||||
}
|
||||
|
||||
async deleteComment(commentId: string) {
|
||||
// Fail fast (#436): reject a truncated id before any network call.
|
||||
assertFullUuid("delete_comment", "commentId", commentId);
|
||||
await this.ensureAuthenticated();
|
||||
return this.client
|
||||
.post("/comments/delete", { commentId })
|
||||
@@ -2997,8 +2910,6 @@ export class DocmostClient {
|
||||
* rejects resolving a reply. Hits POST /comments/resolve.
|
||||
*/
|
||||
async resolveComment(commentId: string, resolved: boolean) {
|
||||
// Fail fast (#436): reject a truncated id before any network call.
|
||||
assertFullUuid("resolve_comment", "commentId", commentId);
|
||||
await this.ensureAuthenticated();
|
||||
const response = await this.client.post("/comments/resolve", {
|
||||
commentId,
|
||||
@@ -3036,36 +2947,27 @@ export class DocmostClient {
|
||||
);
|
||||
}
|
||||
|
||||
// 1. Enumerate the FULL set of pages in scope by walking the sidebar-pages
|
||||
// tree (a complete page index), NOT the bounded "/pages/recent" feed which
|
||||
// caps at ~5000 recent items and silently misses comments on older pages.
|
||||
// 1. Enumerate the FULL set of pages in scope via the page tree (a complete
|
||||
// page index), NOT the bounded "/pages/recent" feed which caps at ~5000
|
||||
// recent items and silently misses comments on older pages.
|
||||
//
|
||||
// Subtree scope: when parentPageId is given, the scope is that page ITSELF
|
||||
// plus every descendant (enumerateSpacePages walks its children). Otherwise
|
||||
// the scope is the whole space (all roots and their descendants).
|
||||
// plus every descendant. Otherwise the scope is the whole space (all roots
|
||||
// and their descendants).
|
||||
//
|
||||
// NOTE: do NOT pre-filter by page.updatedAt — creating a comment does not
|
||||
// bump it (verified on a live server), so such a filter silently misses
|
||||
// comments on pages that were not otherwise edited. The complete tree walk
|
||||
// already restricts the scope correctly, so no recent-feed allow-list is
|
||||
// needed any more.
|
||||
let pagesInScope: any[];
|
||||
if (parentPageId) {
|
||||
const subtree = await this.enumerateSpacePages(spaceId, parentPageId);
|
||||
// Include the parent page node itself alongside its descendants. Fetch it
|
||||
// so its title/id are available even though it is not returned by its own
|
||||
// children listing.
|
||||
let parentNode: any = { id: parentPageId };
|
||||
try {
|
||||
parentNode = await this.getPageRaw(parentPageId);
|
||||
} catch (e: any) {
|
||||
// Fall back to a minimal node if the parent can't be fetched; its
|
||||
// comments are still attempted below (the fetch there is non-fatal).
|
||||
}
|
||||
pagesInScope = [parentNode, ...subtree];
|
||||
} else {
|
||||
pagesInScope = await this.enumerateSpacePages(spaceId);
|
||||
}
|
||||
//
|
||||
// The subtree scope (parentPageId given) already INCLUDES the root node
|
||||
// itself: /pages/tree seeds getPageAndDescendants with id = parentPageId, so
|
||||
// no separate getPageRaw fetch for the parent is needed.
|
||||
const { pages: pagesInScope, truncated } = await this.enumerateSpacePages(
|
||||
spaceId,
|
||||
parentPageId,
|
||||
);
|
||||
|
||||
// 2. Fetch comments for each page, keep ones created after since
|
||||
const results: any[] = [];
|
||||
@@ -3094,10 +2996,9 @@ export class DocmostClient {
|
||||
0,
|
||||
);
|
||||
|
||||
// enumerateSpacePages caps traversal at 10000 nodes; flag when that cap was
|
||||
// hit so the caller knows the scan may be incomplete (some pages skipped).
|
||||
const truncated = pagesInScope.length >= 10000;
|
||||
|
||||
// `truncated` is reported by enumerateSpacePages: it is true ONLY when the
|
||||
// stdio fallback BFS hit its node cap. The primary /pages/tree path is
|
||||
// uncapped, so a space with legitimately many pages is not falsely flagged.
|
||||
return {
|
||||
since,
|
||||
scope: parentPageId ? `subtree of ${parentPageId}` : `space ${spaceId}`,
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -202,21 +202,6 @@ export class CollabSession {
|
||||
this.ydoc = new Y.Doc();
|
||||
}
|
||||
|
||||
/**
|
||||
* Shared diagnostic suffix (issue #437) appended to the connect-timeout,
|
||||
* persist-timeout and connection-closed error texts: names the offending
|
||||
* pageId and tells the agent this class of failure is transient (retry once)
|
||||
* vs. a persistent collab-server outage, so it can self-correct instead of
|
||||
* blind-looping. The Yjs-encode error is deliberately NOT touched — it
|
||||
* already names the offending attribute.
|
||||
*/
|
||||
private hint(): string {
|
||||
return (
|
||||
`(pageId ${this.pageId}; transient — retry once; persistent failures ` +
|
||||
`mean the collab server is unreachable/overloaded)`
|
||||
);
|
||||
}
|
||||
|
||||
/**
|
||||
* A cached session may be reused only when it is fully ready, still synced,
|
||||
* has not lost its connection, and has not exceeded its max age (invariant 5
|
||||
@@ -247,9 +232,7 @@ export class CollabSession {
|
||||
// The 25s connect timeout: the collab connection never became ready.
|
||||
this.opts?.onConnectTimeout?.();
|
||||
this.teardown(
|
||||
new Error(
|
||||
`Connection timeout to collaboration server ${this.hint()}`,
|
||||
),
|
||||
new Error("Connection timeout to collaboration server"),
|
||||
false,
|
||||
);
|
||||
}, CONNECT_TIMEOUT_MS);
|
||||
@@ -276,7 +259,7 @@ export class CollabSession {
|
||||
if (process.env.DEBUG) console.error("WS Disconnect");
|
||||
this.teardown(
|
||||
new Error(
|
||||
`Collaboration connection closed before the update was persisted/synced ${this.hint()}`,
|
||||
"Collaboration connection closed before the update was persisted/synced",
|
||||
),
|
||||
true,
|
||||
);
|
||||
@@ -285,7 +268,7 @@ export class CollabSession {
|
||||
if (process.env.DEBUG) console.error("WS Close");
|
||||
this.teardown(
|
||||
new Error(
|
||||
`Collaboration connection closed before the update was persisted/synced ${this.hint()}`,
|
||||
"Collaboration connection closed before the update was persisted/synced",
|
||||
),
|
||||
true,
|
||||
);
|
||||
@@ -420,7 +403,7 @@ export class CollabSession {
|
||||
persistTimer = setTimeout(() => {
|
||||
localFinish(
|
||||
new Error(
|
||||
`Timeout waiting for collaboration server to persist the update ${this.hint()}`,
|
||||
"Timeout waiting for collaboration server to persist the update",
|
||||
),
|
||||
);
|
||||
}, PERSIST_TIMEOUT_MS);
|
||||
|
||||
@@ -203,16 +203,14 @@ test("a reply creates without selection or anchoring and is stored as type 'page
|
||||
"reply body",
|
||||
"inline",
|
||||
undefined,
|
||||
// #437: a parentCommentId must be a full canonical UUID.
|
||||
"019f499a-9f8c-7d68-b7be-ce100d7c6c56",
|
||||
"parent-123",
|
||||
);
|
||||
|
||||
assert.equal(result.success, true, "a reply must resolve successfully");
|
||||
assert.ok(createPayload, "/comments/create must have been called");
|
||||
assert.equal(
|
||||
createPayload.parentCommentId,
|
||||
// #437: a parentCommentId must be a full canonical UUID.
|
||||
"019f499a-9f8c-7d68-b7be-ce100d7c6c56",
|
||||
"parent-123",
|
||||
"the reply payload must carry the parentCommentId",
|
||||
);
|
||||
assert.equal(
|
||||
@@ -323,9 +321,7 @@ test("suggestedText on a reply is rejected", async () => {
|
||||
"body",
|
||||
"inline",
|
||||
undefined,
|
||||
// #437: use a valid full UUID so the reply+suggestion rejection fires
|
||||
// (not the id-shape guard).
|
||||
"019f499a-9f8c-7d68-b7be-ce100d7c6c56",
|
||||
"parent-1",
|
||||
"replacement",
|
||||
),
|
||||
/reply/i,
|
||||
|
||||
@@ -0,0 +1,440 @@
|
||||
// Mock-HTTP tests for the cursor-pagination migration in DocmostClient (#442).
|
||||
//
|
||||
// The server switched its list endpoints from OFFSET (`page`) to CURSOR
|
||||
// (`cursor`/`nextCursor`) pagination, and the global ValidationPipe silently
|
||||
// strips the obsolete `page` field — so the old offset loops re-fetched page
|
||||
// one forever (hasNextPage stuck true), dropping every item past the first
|
||||
// page. These tests pin the new cursor behaviour and the immovable-cursor
|
||||
// guard that prevents a silent spin/duplication if the protocol drifts again.
|
||||
//
|
||||
// A local http.createServer stands in for Docmost so everything stays
|
||||
// deterministic and offline (same harness style as reauth.test.mjs).
|
||||
import { test, after } from "node:test";
|
||||
import assert from "node:assert/strict";
|
||||
import http from "node:http";
|
||||
import { DocmostClient } from "../../build/client.js";
|
||||
|
||||
function readBody(req) {
|
||||
return new Promise((resolve) => {
|
||||
let raw = "";
|
||||
req.on("data", (chunk) => {
|
||||
raw += chunk;
|
||||
});
|
||||
req.on("end", () => resolve(raw));
|
||||
});
|
||||
}
|
||||
|
||||
function startServer(handler) {
|
||||
return new Promise((resolve) => {
|
||||
const server = http.createServer(handler);
|
||||
server.listen(0, "127.0.0.1", () => {
|
||||
const { port } = server.address();
|
||||
resolve({ server, baseURL: `http://127.0.0.1:${port}/api` });
|
||||
});
|
||||
});
|
||||
}
|
||||
|
||||
function closeServer(server) {
|
||||
return new Promise((resolve) => server.close(resolve));
|
||||
}
|
||||
|
||||
function sendJson(res, status, obj, extraHeaders = {}) {
|
||||
res.writeHead(status, { "Content-Type": "application/json", ...extraHeaders });
|
||||
res.end(JSON.stringify(obj));
|
||||
}
|
||||
|
||||
const openServers = [];
|
||||
async function spawn(handler) {
|
||||
const { server, baseURL } = await startServer(handler);
|
||||
openServers.push(server);
|
||||
return { server, baseURL };
|
||||
}
|
||||
|
||||
after(async () => {
|
||||
await Promise.all(openServers.map((s) => closeServer(s)));
|
||||
});
|
||||
|
||||
// A login handler shared by every server below.
|
||||
function handleLogin(req, res) {
|
||||
if (req.url === "/api/auth/login") {
|
||||
sendJson(res, 200, { success: true }, {
|
||||
"Set-Cookie": "authToken=t; Path=/; HttpOnly",
|
||||
});
|
||||
return true;
|
||||
}
|
||||
return false;
|
||||
}
|
||||
|
||||
// -----------------------------------------------------------------------------
|
||||
// 1) listSidebarPages: collects every cursor page; #requests == #pages.
|
||||
// -----------------------------------------------------------------------------
|
||||
test("listSidebarPages walks all cursor pages and collects every item", async () => {
|
||||
// Three pages keyed by the cursor the client sends back.
|
||||
const PAGES = {
|
||||
"": { items: [{ id: "a" }, { id: "b" }], nextCursor: "c1" },
|
||||
c1: { items: [{ id: "c" }, { id: "d" }], nextCursor: "c2" },
|
||||
c2: { items: [{ id: "e" }], nextCursor: null },
|
||||
};
|
||||
let requests = 0;
|
||||
const sentLimits = [];
|
||||
|
||||
const { baseURL } = await spawn(async (req, res) => {
|
||||
const raw = await readBody(req);
|
||||
if (handleLogin(req, res)) return;
|
||||
if (req.url === "/api/pages/sidebar-pages") {
|
||||
requests++;
|
||||
const body = JSON.parse(raw || "{}");
|
||||
sentLimits.push(body.limit);
|
||||
const page = PAGES[body.cursor ?? ""] ?? { items: [], nextCursor: null };
|
||||
sendJson(res, 200, {
|
||||
success: true,
|
||||
data: {
|
||||
items: page.items,
|
||||
meta: {
|
||||
hasNextPage: page.nextCursor != null,
|
||||
nextCursor: page.nextCursor,
|
||||
},
|
||||
},
|
||||
});
|
||||
return;
|
||||
}
|
||||
sendJson(res, 404, {});
|
||||
});
|
||||
|
||||
const client = new DocmostClient(baseURL, "user@example.com", "pw");
|
||||
const all = await client.listSidebarPages("space-1");
|
||||
|
||||
assert.equal(requests, 3, "one request per cursor page");
|
||||
assert.deepEqual(
|
||||
all.map((p) => p.id),
|
||||
["a", "b", "c", "d", "e"],
|
||||
"all items across all pages collected in order",
|
||||
);
|
||||
assert.ok(
|
||||
sentLimits.every((l) => l === 100),
|
||||
"requests limit:100 (server-side max)",
|
||||
);
|
||||
});
|
||||
|
||||
// -----------------------------------------------------------------------------
|
||||
// 2) REGRESSION on the bug class: server IGNORES the cursor param and always
|
||||
// returns page one with hasNextPage:true -> the immovable-cursor guard must
|
||||
// terminate the loop with no duplicates, NOT spin to MAX_PAGES.
|
||||
// -----------------------------------------------------------------------------
|
||||
test("listSidebarPages terminates (no dups) when the server ignores the cursor", async () => {
|
||||
let requests = 0;
|
||||
|
||||
const { baseURL } = await spawn(async (req, res) => {
|
||||
await readBody(req);
|
||||
if (handleLogin(req, res)) return;
|
||||
if (req.url === "/api/pages/sidebar-pages") {
|
||||
requests++;
|
||||
// Always the SAME first page with hasNextPage:true and the SAME cursor,
|
||||
// exactly as a server that no longer understands our pagination param.
|
||||
sendJson(res, 200, {
|
||||
success: true,
|
||||
data: {
|
||||
items: [{ id: "x1" }, { id: "x2" }],
|
||||
meta: { hasNextPage: true, nextCursor: "stuck" },
|
||||
},
|
||||
});
|
||||
return;
|
||||
}
|
||||
sendJson(res, 404, {});
|
||||
});
|
||||
|
||||
const client = new DocmostClient(baseURL, "user@example.com", "pw");
|
||||
const all = await client.listSidebarPages("space-1");
|
||||
|
||||
// Request 1 (no cursor) gets "stuck"; request 2 (cursor "stuck") gets "stuck"
|
||||
// again -> guard trips. Far below the MAX_PAGES=50 ceiling; no runaway dups.
|
||||
assert.equal(requests, 2, "stops as soon as the cursor stops moving");
|
||||
assert.equal(all.length, 4, "no runaway accumulation / duplication");
|
||||
});
|
||||
|
||||
// -----------------------------------------------------------------------------
|
||||
// 3a) enumerateSpacePages happy path: a SINGLE /pages/tree request.
|
||||
// -----------------------------------------------------------------------------
|
||||
test("enumerateSpacePages (via list_pages tree) uses one /pages/tree request", async () => {
|
||||
let treeRequests = 0;
|
||||
let sidebarRequests = 0;
|
||||
let treeBody = null;
|
||||
|
||||
const NODES = [
|
||||
{ id: "r1", slugId: "r1s", title: "Root 1", parentPageId: null, hasChildren: true, spaceId: "space-1", position: "a", icon: null, canEdit: true },
|
||||
{ id: "c1", slugId: "c1s", title: "Child 1", parentPageId: "r1", hasChildren: false, spaceId: "space-1", position: "a", icon: null, canEdit: true },
|
||||
{ id: "r2", slugId: "r2s", title: "Root 2", parentPageId: null, hasChildren: false, spaceId: "space-1", position: "b", icon: null, canEdit: true },
|
||||
];
|
||||
|
||||
const { baseURL } = await spawn(async (req, res) => {
|
||||
const raw = await readBody(req);
|
||||
if (handleLogin(req, res)) return;
|
||||
if (req.url === "/api/pages/tree") {
|
||||
treeRequests++;
|
||||
treeBody = JSON.parse(raw || "{}");
|
||||
sendJson(res, 200, { success: true, data: { items: NODES } });
|
||||
return;
|
||||
}
|
||||
if (req.url === "/api/pages/sidebar-pages") {
|
||||
sidebarRequests++;
|
||||
sendJson(res, 200, { success: true, data: { items: [], meta: {} } });
|
||||
return;
|
||||
}
|
||||
sendJson(res, 404, {});
|
||||
});
|
||||
|
||||
const client = new DocmostClient(baseURL, "user@example.com", "pw");
|
||||
// list_pages tree:true -> enumerateSpacePages(spaceId) -> buildPageTree.
|
||||
const tree = await client.listPages("space-1", 50, true);
|
||||
|
||||
assert.equal(treeRequests, 1, "exactly one /pages/tree request for the space");
|
||||
assert.equal(sidebarRequests, 0, "no per-node sidebar BFS requests");
|
||||
assert.deepEqual(treeBody, { spaceId: "space-1" }, "space scope posts spaceId only");
|
||||
// buildPageTree nests c1 under r1; two roots at the top level.
|
||||
assert.equal(tree.length, 2, "two root nodes");
|
||||
const r1 = tree.find((n) => n.id === "r1");
|
||||
assert.equal(r1.children.length, 1, "child nested under its root");
|
||||
assert.equal(r1.children[0].id, "c1");
|
||||
});
|
||||
|
||||
// -----------------------------------------------------------------------------
|
||||
// 3b) enumerateSpacePages fallback: /pages/tree 404 -> cursor BFS via sidebar.
|
||||
// -----------------------------------------------------------------------------
|
||||
test("enumerateSpacePages falls back to the cursor BFS on /pages/tree 404", async () => {
|
||||
let treeRequests = 0;
|
||||
const sidebarCalls = [];
|
||||
|
||||
// Root level: one root with children. Child level (pageId=r1): one leaf.
|
||||
const { baseURL } = await spawn(async (req, res) => {
|
||||
const raw = await readBody(req);
|
||||
if (handleLogin(req, res)) return;
|
||||
if (req.url === "/api/pages/tree") {
|
||||
treeRequests++;
|
||||
// Stock upstream Docmost has no /pages/tree.
|
||||
sendJson(res, 404, { message: "Not Found" });
|
||||
return;
|
||||
}
|
||||
if (req.url === "/api/pages/sidebar-pages") {
|
||||
const body = JSON.parse(raw || "{}");
|
||||
sidebarCalls.push(body.pageId ?? "<root>");
|
||||
if (!body.pageId) {
|
||||
sendJson(res, 200, {
|
||||
success: true,
|
||||
data: {
|
||||
items: [
|
||||
{ id: "r1", title: "Root", parentPageId: null, hasChildren: true },
|
||||
],
|
||||
meta: { hasNextPage: false, nextCursor: null },
|
||||
},
|
||||
});
|
||||
} else if (body.pageId === "r1") {
|
||||
sendJson(res, 200, {
|
||||
success: true,
|
||||
data: {
|
||||
items: [
|
||||
{ id: "c1", title: "Leaf", parentPageId: "r1", hasChildren: false },
|
||||
],
|
||||
meta: { hasNextPage: false, nextCursor: null },
|
||||
},
|
||||
});
|
||||
} else {
|
||||
sendJson(res, 200, {
|
||||
success: true,
|
||||
data: { items: [], meta: { hasNextPage: false, nextCursor: null } },
|
||||
});
|
||||
}
|
||||
return;
|
||||
}
|
||||
sendJson(res, 404, {});
|
||||
});
|
||||
|
||||
const client = new DocmostClient(baseURL, "user@example.com", "pw");
|
||||
const tree = await client.listPages("space-1", 50, true);
|
||||
|
||||
assert.ok(treeRequests >= 1, "the tree endpoint was attempted first");
|
||||
assert.deepEqual(
|
||||
sidebarCalls,
|
||||
["<root>", "r1"],
|
||||
"fell back to the sidebar BFS: roots then the root's children",
|
||||
);
|
||||
assert.equal(tree.length, 1, "one root in the built tree");
|
||||
assert.equal(tree[0].children[0].id, "c1", "leaf nested via the BFS");
|
||||
});
|
||||
|
||||
// -----------------------------------------------------------------------------
|
||||
// 3c) enumerateSpacePages fallback SUBTREE: /pages/tree 404 + a rootPageId ->
|
||||
// the ROOT page itself must be seeded (via getPageRaw) so its own comments
|
||||
// aren't dropped. listSidebarPages(spaceId, root) returns only the root's
|
||||
// CHILDREN, so without the seed the root would be absent. (Finding 1.)
|
||||
// -----------------------------------------------------------------------------
|
||||
test("enumerateSpacePages fallback subtree seeds the ROOT page itself", async () => {
|
||||
const sidebarCalls = [];
|
||||
let infoRequests = 0;
|
||||
const commentedPages = [];
|
||||
|
||||
const { baseURL } = await spawn(async (req, res) => {
|
||||
const raw = await readBody(req);
|
||||
if (handleLogin(req, res)) return;
|
||||
if (req.url === "/api/pages/tree") {
|
||||
// Stock upstream Docmost -> fall back to the BFS.
|
||||
sendJson(res, 404, { message: "Not Found" });
|
||||
return;
|
||||
}
|
||||
if (req.url === "/api/pages/info") {
|
||||
// getPageRaw for the root seed. Shape mirrors a real page-info response.
|
||||
infoRequests++;
|
||||
sendJson(res, 200, {
|
||||
success: true,
|
||||
data: { id: "root", title: "Root", spaceId: "space-1", hasChildren: true },
|
||||
});
|
||||
return;
|
||||
}
|
||||
if (req.url === "/api/pages/sidebar-pages") {
|
||||
const body = JSON.parse(raw || "{}");
|
||||
sidebarCalls.push(body.pageId ?? "<root>");
|
||||
// Children of the root: one leaf. (Root itself is NOT in this list.)
|
||||
const items =
|
||||
body.pageId === "root"
|
||||
? [{ id: "leaf", title: "Leaf", parentPageId: "root", hasChildren: false }]
|
||||
: [];
|
||||
sendJson(res, 200, {
|
||||
success: true,
|
||||
data: { items, meta: { hasNextPage: false, nextCursor: null } },
|
||||
});
|
||||
return;
|
||||
}
|
||||
if (req.url === "/api/comments") {
|
||||
const body = JSON.parse(raw || "{}");
|
||||
commentedPages.push(body.pageId);
|
||||
const items =
|
||||
body.pageId === "root"
|
||||
? [{ id: "cm1", createdAt: "2030-01-01T00:00:00.000Z", content: null }]
|
||||
: [];
|
||||
sendJson(res, 200, {
|
||||
success: true,
|
||||
data: { items, meta: { nextCursor: null } },
|
||||
});
|
||||
return;
|
||||
}
|
||||
sendJson(res, 404, {});
|
||||
});
|
||||
|
||||
const client = new DocmostClient(baseURL, "user@example.com", "pw");
|
||||
// checkNewComments(space, since, parentPageId) exercises the subtree fallback.
|
||||
const result = await client.checkNewComments(
|
||||
"space-1",
|
||||
"2020-01-01T00:00:00.000Z",
|
||||
"root",
|
||||
);
|
||||
|
||||
assert.equal(infoRequests, 1, "root was seeded via one getPageRaw");
|
||||
assert.equal(sidebarCalls[0], "root", "BFS walked the root's children");
|
||||
assert.ok(
|
||||
commentedPages.includes("root"),
|
||||
"the ROOT page is in scope (its comments were fetched) — not dropped",
|
||||
);
|
||||
assert.ok(commentedPages.includes("leaf"), "the descendant is in scope too");
|
||||
assert.equal(result.checkedPages, 2, "root + one descendant scanned");
|
||||
assert.equal(result.totalNewComments, 1, "the root's fresh comment found");
|
||||
});
|
||||
|
||||
// -----------------------------------------------------------------------------
|
||||
// 5) listComments immovable-cursor guard: the server IGNORES the cursor and
|
||||
// keeps returning the same nextCursor -> the loop must terminate (no
|
||||
// infinite loop, no duplicates), not spin forever. (Finding 4.)
|
||||
// -----------------------------------------------------------------------------
|
||||
test("listComments terminates (no dups) when the server ignores the cursor", async () => {
|
||||
let requests = 0;
|
||||
|
||||
const { baseURL } = await spawn(async (req, res) => {
|
||||
await readBody(req);
|
||||
if (handleLogin(req, res)) return;
|
||||
if (req.url === "/api/comments") {
|
||||
requests++;
|
||||
// Always the SAME page with the SAME nextCursor, as a server that no
|
||||
// longer advances the cursor would.
|
||||
sendJson(res, 200, {
|
||||
success: true,
|
||||
data: {
|
||||
items: [{ id: "cm1", createdAt: "2030-01-01T00:00:00.000Z", content: null }],
|
||||
meta: { nextCursor: "stuck" },
|
||||
},
|
||||
});
|
||||
return;
|
||||
}
|
||||
sendJson(res, 404, {});
|
||||
});
|
||||
|
||||
const client = new DocmostClient(baseURL, "user@example.com", "pw");
|
||||
const { items } = await client.listComments("page-1", true);
|
||||
|
||||
// Request 1 (no cursor) gets "stuck"; request 2 (cursor "stuck") gets "stuck"
|
||||
// again -> guard trips. Bounded far below MAX_PAGES=50, no runaway dups.
|
||||
assert.equal(requests, 2, "stops as soon as the cursor stops moving");
|
||||
assert.equal(items.length, 2, "no runaway accumulation / duplication");
|
||||
});
|
||||
|
||||
// -----------------------------------------------------------------------------
|
||||
// 4) check_new_comments subtree: the root is included in scope WITHOUT a
|
||||
// separate getPageRaw (/pages/info) request for the parent.
|
||||
// -----------------------------------------------------------------------------
|
||||
test("checkNewComments subtree includes the root without a separate getPageRaw", async () => {
|
||||
let pageInfoRequests = 0;
|
||||
let treeBody = null;
|
||||
const commentedPages = [];
|
||||
|
||||
// /pages/tree (subtree) returns the parent itself plus a descendant, exactly
|
||||
// as getPageAndDescendants seeds with id = parentPageId.
|
||||
const NODES = [
|
||||
{ id: "parent", title: "Parent", parentPageId: null, hasChildren: true },
|
||||
{ id: "kid", title: "Kid", parentPageId: "parent", hasChildren: false },
|
||||
];
|
||||
|
||||
const { baseURL } = await spawn(async (req, res) => {
|
||||
const raw = await readBody(req);
|
||||
if (handleLogin(req, res)) return;
|
||||
if (req.url === "/api/pages/tree") {
|
||||
treeBody = JSON.parse(raw || "{}");
|
||||
sendJson(res, 200, { success: true, data: { items: NODES } });
|
||||
return;
|
||||
}
|
||||
if (req.url === "/api/pages/info") {
|
||||
// If checkNewComments still fetched the parent separately this would fire.
|
||||
pageInfoRequests++;
|
||||
sendJson(res, 200, { success: true, data: { id: "parent" } });
|
||||
return;
|
||||
}
|
||||
if (req.url === "/api/comments") {
|
||||
const body = JSON.parse(raw || "{}");
|
||||
commentedPages.push(body.pageId);
|
||||
// One fresh comment on the parent, none elsewhere.
|
||||
const items =
|
||||
body.pageId === "parent"
|
||||
? [{ id: "cm1", createdAt: "2030-01-01T00:00:00.000Z", content: null }]
|
||||
: [];
|
||||
sendJson(res, 200, {
|
||||
success: true,
|
||||
data: { items, meta: { nextCursor: null } },
|
||||
});
|
||||
return;
|
||||
}
|
||||
sendJson(res, 404, {});
|
||||
});
|
||||
|
||||
const client = new DocmostClient(baseURL, "user@example.com", "pw");
|
||||
const result = await client.checkNewComments(
|
||||
"space-1",
|
||||
"2020-01-01T00:00:00.000Z",
|
||||
"parent",
|
||||
);
|
||||
|
||||
assert.equal(pageInfoRequests, 0, "no separate getPageRaw for the root");
|
||||
assert.deepEqual(treeBody, { pageId: "parent" }, "subtree scope posts pageId");
|
||||
assert.ok(
|
||||
commentedPages.includes("parent"),
|
||||
"the root itself is in scope (comments fetched for it)",
|
||||
);
|
||||
assert.ok(commentedPages.includes("kid"), "descendants are in scope too");
|
||||
assert.equal(result.checkedPages, 2, "root + one descendant scanned");
|
||||
assert.equal(result.totalNewComments, 1, "the root's fresh comment found");
|
||||
});
|
||||
@@ -297,12 +297,12 @@ test("a response with ONLY authTokenRefresh (no authToken) rejects login", async
|
||||
// -----------------------------------------------------------------------------
|
||||
// 5) paginateAll loop guards.
|
||||
// -----------------------------------------------------------------------------
|
||||
test("paginateAll stops at the MAX_PAGES cap when hasNextPage is always true", async () => {
|
||||
test("paginateAll stops at the MAX_PAGES cap when the server always issues a fresh cursor", async () => {
|
||||
let pageRequests = 0;
|
||||
const LIMIT = 100;
|
||||
|
||||
const { baseURL } = await spawn(async (req, res) => {
|
||||
await readBody(req);
|
||||
const body = JSON.parse((await readBody(req)) || "{}");
|
||||
if (req.url === "/api/auth/login") {
|
||||
sendJson(res, 200, { success: true }, {
|
||||
"Set-Cookie": "authToken=t; Path=/; HttpOnly",
|
||||
@@ -311,15 +311,18 @@ test("paginateAll stops at the MAX_PAGES cap when hasNextPage is always true", a
|
||||
}
|
||||
if (req.url === "/api/spaces") {
|
||||
pageRequests++;
|
||||
// Always return a FULL page (== requested limit) AND hasNextPage:true.
|
||||
// Both the page-length check and the hasNextPage flag say "keep going",
|
||||
// so only the MAX_PAGES ceiling can stop the loop.
|
||||
// Always return a FULL page AND hasNextPage:true with a FRESH nextCursor
|
||||
// that differs from the one the client just sent, so the immovable-cursor
|
||||
// guard never trips — only the MAX_PAGES ceiling can stop the loop.
|
||||
const items = Array.from({ length: LIMIT }, (_, i) => ({
|
||||
id: `s-${pageRequests}-${i}`,
|
||||
}));
|
||||
sendJson(res, 200, {
|
||||
success: true,
|
||||
data: { items, meta: { hasNextPage: true } },
|
||||
data: {
|
||||
items,
|
||||
meta: { hasNextPage: true, nextCursor: `cursor-${pageRequests}` },
|
||||
},
|
||||
});
|
||||
return;
|
||||
}
|
||||
@@ -338,7 +341,7 @@ test("paginateAll stops at the MAX_PAGES cap when hasNextPage is always true", a
|
||||
assert.equal(all.length, 50 * LIMIT, "accumulates one full page per request");
|
||||
});
|
||||
|
||||
test("paginateAll stops early on a short page even if hasNextPage is true", async () => {
|
||||
test("paginateAll stops on the immovable-cursor guard when the server ignores the cursor param", async () => {
|
||||
let pageRequests = 0;
|
||||
const LIMIT = 100;
|
||||
|
||||
@@ -352,15 +355,17 @@ test("paginateAll stops early on a short page even if hasNextPage is true", asyn
|
||||
}
|
||||
if (req.url === "/api/spaces") {
|
||||
pageRequests++;
|
||||
// First page is full; second page is SHORT (fewer than limit). The short
|
||||
// page must stop the loop immediately even though hasNextPage stays true.
|
||||
const count = pageRequests === 1 ? LIMIT : 3;
|
||||
const items = Array.from({ length: count }, (_, i) => ({
|
||||
id: `s-${pageRequests}-${i}`,
|
||||
}));
|
||||
// The bug class: the server IGNORES the pagination param and keeps
|
||||
// returning page one with hasNextPage:true and the SAME nextCursor. The
|
||||
// immovable-cursor guard must stop the loop instead of spinning to
|
||||
// MAX_PAGES and duplicating items.
|
||||
const items = Array.from({ length: LIMIT }, (_, i) => ({ id: `s-${i}` }));
|
||||
sendJson(res, 200, {
|
||||
success: true,
|
||||
data: { items, meta: { hasNextPage: true } },
|
||||
data: {
|
||||
items,
|
||||
meta: { hasNextPage: true, nextCursor: "stuck" },
|
||||
},
|
||||
});
|
||||
return;
|
||||
}
|
||||
@@ -370,8 +375,10 @@ test("paginateAll stops early on a short page even if hasNextPage is true", asyn
|
||||
const client = new DocmostClient(baseURL, "user@example.com", "pw");
|
||||
const all = await client.paginateAll("/spaces", {}, LIMIT);
|
||||
|
||||
assert.equal(pageRequests, 2, "stops right after the first short page");
|
||||
assert.equal(all.length, LIMIT + 3, "full page + short page accumulated");
|
||||
// Request 1 sends no cursor and receives "stuck"; request 2 sends "stuck" and
|
||||
// receives "stuck" again -> guard trips after exactly two requests, no dups.
|
||||
assert.equal(pageRequests, 2, "stops once the cursor stops moving");
|
||||
assert.equal(all.length, 2 * LIMIT, "no runaway accumulation past the guard");
|
||||
});
|
||||
|
||||
test("paginateAll handles both {data:{items,meta}} and {items,meta} envelopes", async () => {
|
||||
@@ -387,16 +394,16 @@ test("paginateAll handles both {data:{items,meta}} and {items,meta} envelopes",
|
||||
}
|
||||
if (req.url === "/api/groups") {
|
||||
bareRequests.push(1);
|
||||
// Page 1: full page, hasNextPage true. Page 2: short page -> stop.
|
||||
// Page 1: hasNextPage true with a next cursor. Page 2: no next -> stop.
|
||||
if (bareRequests.length === 1) {
|
||||
sendJson(res, 200, {
|
||||
items: Array.from({ length: 100 }, (_, i) => ({ id: `g${i}` })),
|
||||
meta: { hasNextPage: true },
|
||||
meta: { hasNextPage: true, nextCursor: "c2" },
|
||||
});
|
||||
} else {
|
||||
sendJson(res, 200, {
|
||||
items: [{ id: "tail" }],
|
||||
meta: { hasNextPage: false },
|
||||
meta: { hasNextPage: false, nextCursor: null },
|
||||
});
|
||||
}
|
||||
return;
|
||||
|
||||
@@ -168,9 +168,7 @@ test("an in-flight mutate rejects with the connection-closed text on disconnect"
|
||||
FakeProvider.last()._disconnect();
|
||||
await assert.rejects(
|
||||
p,
|
||||
// Assert the #437 diagnostic hint tail too (pageId + transient/retry cue),
|
||||
// so a refactor that drops hint() can't pass this vacuously.
|
||||
/Collaboration connection closed before the update was persisted\/synced \(pageId page-1; transient/,
|
||||
/Collaboration connection closed before the update was persisted\/synced/,
|
||||
);
|
||||
});
|
||||
|
||||
@@ -250,11 +248,7 @@ test("connect timeout rejects with the connect-timeout text and fires the metric
|
||||
},
|
||||
});
|
||||
mock.timers.tick(25000);
|
||||
await assert.rejects(
|
||||
p,
|
||||
// Assert the #437 diagnostic hint tail too (pageId + transient/retry cue).
|
||||
/Connection timeout to collaboration server \(pageId page-1; transient/,
|
||||
);
|
||||
await assert.rejects(p, /Connection timeout to collaboration server/);
|
||||
assert.equal(metricFired, 1);
|
||||
assert.equal(__sessionCountForTests(), 0);
|
||||
});
|
||||
|
||||
@@ -1,450 +0,0 @@
|
||||
// Issue #437: central error diagnostics.
|
||||
//
|
||||
// Two surfaces are covered here:
|
||||
// 1. formatDocmostAxiosError — the pure response-interceptor body that
|
||||
// rewrites an AxiosError's `.message` into an actionable diagnostic.
|
||||
// 2. assertFullUuid — the fail-fast comment-id guard (absorbs #436) that must
|
||||
// throw BEFORE any network call.
|
||||
// Plus an end-to-end pass over a real (offline) http server to prove the
|
||||
// interceptor is wired, that a re-login retry leaves a success untouched, and
|
||||
// that a persistent failure gets formatted — and that an invalid comment id
|
||||
// short-circuits every comment tool with ZERO network traffic.
|
||||
import { test, after } from "node:test";
|
||||
import assert from "node:assert/strict";
|
||||
import http from "node:http";
|
||||
import axios, { AxiosError } from "axios";
|
||||
import {
|
||||
DocmostClient,
|
||||
formatDocmostAxiosError,
|
||||
assertFullUuid,
|
||||
} from "../../build/client.js";
|
||||
|
||||
// Build an AxiosError-shaped object the way the interceptor's rejection handler
|
||||
// receives it. Using the real AxiosError ctor makes axios.isAxiosError() true.
|
||||
function makeAxiosError({
|
||||
method = "post",
|
||||
url = "/comments/resolve",
|
||||
baseURL = "http://host.example/api",
|
||||
status,
|
||||
statusText,
|
||||
data,
|
||||
code,
|
||||
message = "Request failed",
|
||||
}) {
|
||||
const config = { method, url, baseURL };
|
||||
const response =
|
||||
status === undefined
|
||||
? undefined
|
||||
: { status, statusText, data, headers: {}, config };
|
||||
return new AxiosError(message, code, config, {}, response);
|
||||
}
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// formatDocmostAxiosError: message-body extraction rules.
|
||||
// ---------------------------------------------------------------------------
|
||||
test("class-validator message array is joined with '; '", () => {
|
||||
const err = makeAxiosError({
|
||||
status: 400,
|
||||
statusText: "Bad Request",
|
||||
data: { message: ["commentId must be a UUID", "resolved must be a boolean"] },
|
||||
});
|
||||
formatDocmostAxiosError(err);
|
||||
assert.equal(
|
||||
err.message,
|
||||
"POST /comments/resolve failed (400 Bad Request): commentId must be a UUID; resolved must be a boolean",
|
||||
);
|
||||
});
|
||||
|
||||
test("a string message is used as-is", () => {
|
||||
const err = makeAxiosError({
|
||||
method: "post",
|
||||
url: "/comments/resolve",
|
||||
status: 400,
|
||||
statusText: "Bad Request",
|
||||
data: { message: "commentId must be a UUID" },
|
||||
});
|
||||
formatDocmostAxiosError(err);
|
||||
assert.equal(
|
||||
err.message,
|
||||
"POST /comments/resolve failed (400 Bad Request): commentId must be a UUID",
|
||||
);
|
||||
});
|
||||
|
||||
test("falls back to data.error when message is absent", () => {
|
||||
const err = makeAxiosError({
|
||||
method: "get",
|
||||
url: "/pages/info",
|
||||
status: 403,
|
||||
statusText: "Forbidden",
|
||||
data: { error: "Forbidden" },
|
||||
});
|
||||
formatDocmostAxiosError(err);
|
||||
assert.equal(err.message, "GET /pages/info failed (403 Forbidden): Forbidden");
|
||||
});
|
||||
|
||||
test("empty object body falls back to statusText", () => {
|
||||
const err = makeAxiosError({
|
||||
status: 404,
|
||||
statusText: "Not Found",
|
||||
url: "/comments/info",
|
||||
data: {},
|
||||
});
|
||||
formatDocmostAxiosError(err);
|
||||
assert.equal(err.message, "POST /comments/info failed (404 Not Found): Not Found");
|
||||
});
|
||||
|
||||
test("HTML/string body is NEVER surfaced — only the statusText", () => {
|
||||
const html = "<html><body>502 Bad Gateway — nginx internals here</body></html>";
|
||||
const err = makeAxiosError({
|
||||
status: 502,
|
||||
statusText: "Bad Gateway",
|
||||
url: "/comments/create",
|
||||
data: html,
|
||||
});
|
||||
formatDocmostAxiosError(err);
|
||||
assert.equal(
|
||||
err.message,
|
||||
"POST /comments/create failed (502 Bad Gateway): Bad Gateway",
|
||||
);
|
||||
assert.ok(!err.message.includes("nginx"), "raw HTML body must not leak");
|
||||
assert.ok(!err.message.includes("<html>"), "raw HTML body must not leak");
|
||||
});
|
||||
|
||||
test("Buffer body carrying JSON is parsed for its message", () => {
|
||||
const buf = Buffer.from(JSON.stringify({ message: "file too large" }), "utf8");
|
||||
const err = makeAxiosError({
|
||||
method: "get",
|
||||
url: "/files/abc/x.png",
|
||||
status: 413,
|
||||
statusText: "Payload Too Large",
|
||||
data: buf,
|
||||
});
|
||||
formatDocmostAxiosError(err);
|
||||
assert.equal(
|
||||
err.message,
|
||||
"GET /files/abc/x.png failed (413 Payload Too Large): file too large",
|
||||
);
|
||||
});
|
||||
|
||||
test("Buffer body with non-JSON garbage falls back to statusText", () => {
|
||||
const buf = Buffer.from("<<< not json at all >>>", "utf8");
|
||||
const err = makeAxiosError({
|
||||
method: "get",
|
||||
url: "/files/abc/x.png",
|
||||
status: 500,
|
||||
statusText: "Internal Server Error",
|
||||
data: buf,
|
||||
});
|
||||
formatDocmostAxiosError(err);
|
||||
assert.equal(
|
||||
err.message,
|
||||
"GET /files/abc/x.png failed (500 Internal Server Error): Internal Server Error",
|
||||
);
|
||||
assert.ok(!err.message.includes("not json"), "raw buffer body must not leak");
|
||||
});
|
||||
|
||||
test("an oversized Buffer body is not parsed (size cap) — statusText only", () => {
|
||||
// A >4KB JSON buffer: even though it IS valid JSON with a message, the size
|
||||
// cap means we do not attempt to parse it, so only the statusText survives.
|
||||
const big = { message: "x".repeat(5000) };
|
||||
const buf = Buffer.from(JSON.stringify(big), "utf8");
|
||||
const err = makeAxiosError({
|
||||
method: "get",
|
||||
url: "/files/abc/x.png",
|
||||
status: 500,
|
||||
statusText: "Internal Server Error",
|
||||
data: buf,
|
||||
});
|
||||
formatDocmostAxiosError(err);
|
||||
assert.equal(
|
||||
err.message,
|
||||
"GET /files/abc/x.png failed (500 Internal Server Error): Internal Server Error",
|
||||
);
|
||||
});
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// formatDocmostAxiosError: no-response and path/method handling.
|
||||
// ---------------------------------------------------------------------------
|
||||
test("no response uses error.code + path + 'no response from server'", () => {
|
||||
const err = makeAxiosError({
|
||||
method: "post",
|
||||
url: "/comments/create",
|
||||
status: undefined,
|
||||
code: "ECONNREFUSED",
|
||||
message: "connect ECONNREFUSED 127.0.0.1:3000",
|
||||
});
|
||||
formatDocmostAxiosError(err);
|
||||
assert.equal(
|
||||
err.message,
|
||||
"POST /comments/create failed: ECONNREFUSED (no response from server)",
|
||||
);
|
||||
});
|
||||
|
||||
test("no response with no code falls back to a neutral reason (raw message not leaked — it may embed host:port)", () => {
|
||||
const err = makeAxiosError({
|
||||
method: "post",
|
||||
url: "/comments/create",
|
||||
status: undefined,
|
||||
// A raw axios network message like "connect ECONNREFUSED 127.0.0.1:3000"
|
||||
// embeds the host; #437's invariant is that it never reaches the message.
|
||||
message: "connect ECONNREFUSED 10.0.0.5:3000",
|
||||
});
|
||||
formatDocmostAxiosError(err);
|
||||
assert.equal(
|
||||
err.message,
|
||||
"POST /comments/create failed: network error (no response from server)",
|
||||
);
|
||||
// And the host must NOT appear anywhere in the model-visible message.
|
||||
assert.ok(!err.message.includes("10.0.0.5"));
|
||||
});
|
||||
|
||||
test("path drops the host and the query string", () => {
|
||||
const err = makeAxiosError({
|
||||
method: "post",
|
||||
url: "/comments/resolve?token=secret&x=1",
|
||||
baseURL: "https://docs.example.com/api",
|
||||
status: 400,
|
||||
statusText: "Bad Request",
|
||||
data: { message: "bad" },
|
||||
});
|
||||
formatDocmostAxiosError(err);
|
||||
assert.equal(
|
||||
err.message,
|
||||
"POST /comments/resolve failed (400 Bad Request): bad",
|
||||
);
|
||||
assert.ok(!err.message.includes("secret"), "query string must not leak");
|
||||
assert.ok(!err.message.includes("docs.example.com"), "host must not leak");
|
||||
});
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// formatDocmostAxiosError: length cap + guard flag + pass-through.
|
||||
// ---------------------------------------------------------------------------
|
||||
test("the overall message is capped at ~300 chars", () => {
|
||||
const err = makeAxiosError({
|
||||
status: 400,
|
||||
statusText: "Bad Request",
|
||||
data: { message: "y".repeat(1000) },
|
||||
});
|
||||
formatDocmostAxiosError(err);
|
||||
assert.ok(err.message.length <= 300, `expected <=300, got ${err.message.length}`);
|
||||
assert.ok(err.message.endsWith("…"), "a truncated message ends with an ellipsis");
|
||||
});
|
||||
|
||||
test("a formatted error is not re-processed (guard flag)", () => {
|
||||
const err = makeAxiosError({
|
||||
status: 400,
|
||||
statusText: "Bad Request",
|
||||
data: { message: "first" },
|
||||
});
|
||||
formatDocmostAxiosError(err);
|
||||
const once = err.message;
|
||||
assert.equal(err._docmostFormatted, true);
|
||||
// Mutate the body and re-run: the guard makes it a no-op.
|
||||
err.response.data = { message: "second" };
|
||||
formatDocmostAxiosError(err);
|
||||
assert.equal(err.message, once, "the guard flag prevents double-processing");
|
||||
});
|
||||
|
||||
test("a non-axios error is passed through untouched", () => {
|
||||
const plain = new Error("boom");
|
||||
formatDocmostAxiosError(plain);
|
||||
assert.equal(plain.message, "boom");
|
||||
assert.equal(plain._docmostFormatted, undefined);
|
||||
});
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// assertFullUuid.
|
||||
// ---------------------------------------------------------------------------
|
||||
const GOOD_UUID = "019f499a-9f8c-7d68-b7be-ce100d7c6c56";
|
||||
|
||||
test("assertFullUuid accepts a full canonical UUID (any version nibble)", () => {
|
||||
assert.doesNotThrow(() => assertFullUuid("resolve_comment", "commentId", GOOD_UUID));
|
||||
// A v4 id also passes (version/variant-agnostic).
|
||||
assert.doesNotThrow(() =>
|
||||
assertFullUuid("get_comment", "commentId", "3d5b7c1e-2f4a-4b6c-8d9e-0f1a2b3c4d5e"),
|
||||
);
|
||||
});
|
||||
|
||||
test("assertFullUuid rejects a truncated prefix", () => {
|
||||
assert.throws(
|
||||
() => assertFullUuid("resolve_comment", "commentId", "019f499a"),
|
||||
(e) =>
|
||||
e.message.startsWith(
|
||||
"resolve_comment: 'commentId' must be the FULL comment UUID",
|
||||
) &&
|
||||
e.message.includes("got '019f499a'") &&
|
||||
e.message.includes("Copy the id verbatim"),
|
||||
);
|
||||
});
|
||||
|
||||
test("assertFullUuid rejects garbage and empty string", () => {
|
||||
assert.throws(
|
||||
() => assertFullUuid("delete_comment", "commentId", "not-a-uuid"),
|
||||
/must be the FULL comment UUID.*got 'not-a-uuid'/s,
|
||||
);
|
||||
assert.throws(
|
||||
() => assertFullUuid("update_comment", "commentId", ""),
|
||||
/must be the FULL comment UUID.*got ''/s,
|
||||
);
|
||||
});
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// End-to-end over an offline http server: interceptor wiring + re-login.
|
||||
// ---------------------------------------------------------------------------
|
||||
function readBody(req) {
|
||||
return new Promise((resolve) => {
|
||||
let raw = "";
|
||||
req.on("data", (c) => (raw += c));
|
||||
req.on("end", () => resolve(raw));
|
||||
});
|
||||
}
|
||||
function startServer(handler) {
|
||||
return new Promise((resolve) => {
|
||||
const server = http.createServer(handler);
|
||||
server.listen(0, "127.0.0.1", () => {
|
||||
const { port } = server.address();
|
||||
resolve({ server, baseURL: `http://127.0.0.1:${port}/api` });
|
||||
});
|
||||
});
|
||||
}
|
||||
function sendJson(res, status, obj, extra = {}) {
|
||||
res.writeHead(status, { "Content-Type": "application/json", ...extra });
|
||||
res.end(JSON.stringify(obj));
|
||||
}
|
||||
const openServers = [];
|
||||
async function spawn(handler) {
|
||||
const { server, baseURL } = await startServer(handler);
|
||||
openServers.push(server);
|
||||
return { baseURL };
|
||||
}
|
||||
after(async () => {
|
||||
await Promise.all(openServers.map((s) => new Promise((r) => s.close(r))));
|
||||
});
|
||||
|
||||
test("a 400 on a JSON endpoint is reformatted by the wired interceptor", async () => {
|
||||
const { baseURL } = await spawn(async (req, res) => {
|
||||
await readBody(req);
|
||||
if (req.url === "/api/auth/login") {
|
||||
sendJson(res, 200, { success: true }, {
|
||||
"Set-Cookie": "authToken=t; Path=/; HttpOnly",
|
||||
});
|
||||
return;
|
||||
}
|
||||
if (req.url === "/api/pages/info") {
|
||||
sendJson(res, 400, { message: "pageId should not be empty" });
|
||||
return;
|
||||
}
|
||||
sendJson(res, 404, {});
|
||||
});
|
||||
|
||||
const client = new DocmostClient(baseURL, "u@example.com", "pw");
|
||||
await assert.rejects(
|
||||
() => client.getPageRaw("x"),
|
||||
(e) => {
|
||||
assert.ok(axios.isAxiosError(e), "still an AxiosError (mutation, not a subclass)");
|
||||
assert.equal(e.response?.status, 400, "error.response?.status still readable");
|
||||
assert.equal(
|
||||
e.message,
|
||||
"POST /pages/info failed (400 Bad Request): pageId should not be empty",
|
||||
);
|
||||
return true;
|
||||
},
|
||||
);
|
||||
});
|
||||
|
||||
test("401 -> re-login -> successful retry: the SUCCESS message is untouched", async () => {
|
||||
let infoCalls = 0;
|
||||
const { baseURL } = await spawn(async (req, res) => {
|
||||
await readBody(req);
|
||||
if (req.url === "/api/auth/login") {
|
||||
sendJson(res, 200, { success: true }, {
|
||||
"Set-Cookie": "authToken=fresh; Path=/; HttpOnly",
|
||||
});
|
||||
return;
|
||||
}
|
||||
if (req.url === "/api/workspace/info") {
|
||||
infoCalls++;
|
||||
if (infoCalls === 1) sendJson(res, 401, { message: "Unauthorized" });
|
||||
else sendJson(res, 200, { success: true, data: { id: "ws" } });
|
||||
return;
|
||||
}
|
||||
sendJson(res, 404, {});
|
||||
});
|
||||
|
||||
const client = new DocmostClient(baseURL, "u@example.com", "pw");
|
||||
client.token = "stale";
|
||||
client.client.defaults.headers.common["Authorization"] = "Bearer stale";
|
||||
|
||||
const result = await client.getWorkspace();
|
||||
assert.equal(result.success, true, "the retried request resolved successfully");
|
||||
assert.equal(infoCalls, 2, "401 then a successful replay");
|
||||
});
|
||||
|
||||
test("401 -> re-login -> persistent failure: formatted AND retry guard intact", async () => {
|
||||
let infoCalls = 0;
|
||||
let loginCalls = 0;
|
||||
const { baseURL } = await spawn(async (req, res) => {
|
||||
await readBody(req);
|
||||
if (req.url === "/api/auth/login") {
|
||||
loginCalls++;
|
||||
sendJson(res, 200, { success: true }, {
|
||||
"Set-Cookie": "authToken=fresh; Path=/; HttpOnly",
|
||||
});
|
||||
return;
|
||||
}
|
||||
if (req.url === "/api/workspace/info") {
|
||||
infoCalls++;
|
||||
// Always 401, even after a fresh login: the _retry guard must stop here.
|
||||
sendJson(res, 401, { message: "token still invalid" });
|
||||
return;
|
||||
}
|
||||
sendJson(res, 404, {});
|
||||
});
|
||||
|
||||
const client = new DocmostClient(baseURL, "u@example.com", "pw");
|
||||
client.token = "stale";
|
||||
client.client.defaults.headers.common["Authorization"] = "Bearer stale";
|
||||
|
||||
await assert.rejects(
|
||||
() => client.getWorkspace(),
|
||||
(e) => {
|
||||
assert.equal(
|
||||
e.message,
|
||||
"POST /workspace/info failed (401 Unauthorized): token still invalid",
|
||||
);
|
||||
return true;
|
||||
},
|
||||
);
|
||||
// The _retry guard is intact: exactly one replay (2 hits), one re-login.
|
||||
assert.equal(infoCalls, 2, "endpoint hit at most twice (one retry only)");
|
||||
assert.equal(loginCalls, 1, "re-login attempted exactly once");
|
||||
});
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// assertFullUuid application points: NO network call when the id is invalid.
|
||||
// A server that counts EVERY request proves the guard short-circuits before
|
||||
// even the login round-trip.
|
||||
// ---------------------------------------------------------------------------
|
||||
test("all 5 comment-id call sites reject a bad id with ZERO network traffic", async () => {
|
||||
let requests = 0;
|
||||
const { baseURL } = await spawn(async (req, res) => {
|
||||
requests++;
|
||||
await readBody(req);
|
||||
sendJson(res, 200, { success: true });
|
||||
});
|
||||
|
||||
const client = new DocmostClient(baseURL, "u@example.com", "pw");
|
||||
const bad = "019f499a"; // truncated
|
||||
|
||||
await assert.rejects(() => client.resolveComment(bad, true), /resolve_comment: 'commentId'/);
|
||||
await assert.rejects(() => client.updateComment(bad, "hi"), /update_comment: 'commentId'/);
|
||||
await assert.rejects(() => client.deleteComment(bad), /delete_comment: 'commentId'/);
|
||||
await assert.rejects(() => client.getComment(bad), /get_comment: 'commentId'/);
|
||||
// createComment validates parentCommentId only when provided.
|
||||
await assert.rejects(
|
||||
() => client.createComment("page-1", "body", "inline", "sel", bad),
|
||||
/create_comment: 'parentCommentId'/,
|
||||
);
|
||||
|
||||
assert.equal(requests, 0, "no request (not even /auth/login) may be issued for a bad id");
|
||||
});
|
||||
@@ -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);
|
||||
});
|
||||
Reference in New Issue
Block a user