ci(#476): гейты наблюдаемых свойств перед publish — image-smoke, migration-order на push, allowlist fail-closed, property-тесты

Retrospective of 22.06-10.07 merges showed one recurring miss class: local logic verified, integration property never checked (#361, #353, #452, #172, #435). This lands four gates so each of those classes fails BEFORE the :develop image is pushed: 1. Image boot-smoke in the publish job (develop.yml + scripts/ci/image-smoke.sh): the exact image watchtower pulls is booted against postgres/redis services before the push — /api/health (startup migrator, #361-boot/#353), auth/setup, client dist served, hashed assets immutable + brotli (#452). 2. migration-order gate now also runs on push (test.yml): direct pushes used to bypass the PR-only gate; base = event.before, zero-SHA skips, force-push fails closed. 3. External-MCP tool allowlist fails closed (#172 class): corrupt stored value now reads as [] (deny-all) with an error log instead of null (allow-all); [] round-trips as jsonb [] via jsonbBind({preserveEmpty}) and means deny-all in the toolset filter. The settings form sends null for an empty tag field so existing "unrestricted" servers are not silently narrowed. 4. Property tests for the silent-degradation classes: converter fixpoint through the live server path (mcp e2e), and CollabSession cache-key stability under per-call fresh tokens (#435/#439 lesson) incl. a negative control with the token cache disabled. Closes #476 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2026-07-11 01:02:29 +03:00
parent fe5bd159c4
commit 1e7bd1f9d2
16 changed files with 670 additions and 57 deletions
@@ -62,6 +62,38 @@ jobs:
    needs: [test, e2e-server, e2e-mcp, build]
    runs-on: ubuntu-latest
    timeout-minutes: 30
+    # Image boot-smoke (issue #476): every other job tests code from the working
+    # tree, but the :develop IMAGE that watchtower pulls was never actually
+    # started anywhere (incident classes #353/#452/#361-boot: startup-migrator
+    # crash-loop, runtime module missing from the image, wrong static-asset
+    # headers). The services below back a smoke boot of the exact image right
+    # before it is pushed; a smoke failure blocks the push.
+    services:
+      postgres:
+        # via mirror.gcr.io (Docker Hub pull-through cache; avoids Hub anonymous
+        # pull rate-limit that randomly fails on shared GitHub runner IPs).
+        image: mirror.gcr.io/pgvector/pgvector:pg18
+        env:
+          POSTGRES_DB: docmost
+          POSTGRES_USER: docmost
+          POSTGRES_PASSWORD: docmost
+        ports:
+          - 5432:5432
+        options: >-
+          --health-cmd "pg_isready -U docmost"
+          --health-interval 5s
+          --health-timeout 5s
+          --health-retries 20
+      redis:
+        # via mirror.gcr.io (see postgres note above).
+        image: mirror.gcr.io/library/redis:7
+        ports:
+          - 6379:6379
+        options: >-
+          --health-cmd "redis-cli ping"
+          --health-interval 5s
+          --health-timeout 5s
+          --health-retries 20
    steps:
      - name: Checkout
        uses: actions/checkout@v4
@@ -82,6 +114,37 @@ jobs:
        id: version
        run: echo "value=$(git describe --tags --always)" >> "$GITHUB_OUTPUT"

+      # Load the image into the local docker daemon so it can be booted (the
+      # push step below exports straight to the registry and leaves nothing
+      # runnable locally). CONVENTION: build-args here must stay TEXTUALLY
+      # IDENTICAL to the push step's build-args — same cache scope + same args
+      # means the layers are reused and the image we smoke IS the image we push.
+      - name: Build image for smoke (load, no push)
+        uses: docker/build-push-action@v6
+        with:
+          context: .
+          platforms: linux/amd64
+          build-args: |
+            APP_VERSION=${{ steps.version.outputs.value }}
+            AI_AGENT_ROLES_CATALOG_URL=https://raw.githubusercontent.com/vvzvlad/gitmost/develop/agent-roles-catalog
+          load: true
+          push: false
+          tags: gitmost:smoke
+          cache-from: type=gha,scope=develop-amd64
+
+      # Boot-smoke the exact image against the job services (see the comment on
+      # `services:` above): health (startup migrator), auth/setup, client dist
+      # served, immutable + brotli asset headers. Fails the job (and therefore
+      # the push) on any miss.
+      - name: Smoke the built image
+        run: bash scripts/ci/image-smoke.sh gitmost:smoke
+
+      # The smoke script leaves the container running on failure precisely so
+      # the boot error (migration mismatch, stack trace) is diagnosable here.
+      - name: Dump smoke container log on failure
+        if: failure()
+        run: docker logs gitmost-smoke 2>&1 | tail -200 || true
+
      - name: Build and push develop image
        uses: docker/build-push-action@v6
        with:
@@ -25,37 +25,65 @@ jobs:
  # filename sorts BEFORE migrations already applied on the target branch (and
  # thus in prod). The Kysely startup migrator rejects that as "corrupted
  # migrations" and crash-loops the app on boot (incident #361). This gate fails
-  # the PR so the migration is renamed to a current timestamp before merge. Only
-  # runs for pull_request events (needs a base branch to diff against).
+  # the PR so the migration is renamed to a current timestamp before merge.
+  # Runs for pull_request (diff against the base branch) AND for push (#476
+  # retrospective: a DIRECT push to develop used to bypass this PR-only gate
+  # entirely — now the push is diffed against its `before` SHA; workflow_call
+  # from develop.yml inherits the caller's push event). workflow_dispatch has
+  # nothing to diff against and still skips the job.
  migration-order:
-    if: github.event_name == 'pull_request'
+    if: github.event_name == 'pull_request' || github.event_name == 'push'
    runs-on: ubuntu-latest
    timeout-minutes: 5
    steps:
-      - name: Checkout (full history for the base-branch diff)
+      - name: Checkout (full history for the base diff)
        uses: actions/checkout@v4
        with:
          fetch-depth: 0
-      - name: Added migrations must sort after the newest on the base branch
+      - name: Added migrations must sort after the newest on the base
        env:
          TARGET_BRANCH: ${{ github.base_ref }}
+          BEFORE_SHA: ${{ github.event.before }}
        run: |
          set -euo pipefail
          MIG_DIR="apps/server/src/database/migrations"
-          # checkout above already did fetch-depth:0 (full history). Fetch the base
-          # WITHOUT --depth (a shallow graft would truncate the base history and
-          # break the merge-base when the base has moved ahead of the PR merge —
-          # exactly the long-branch-vs-moving-base case this gate guards, #361).
-          git fetch --no-tags origin "$TARGET_BRANCH"
-          newest_on_target=$(git ls-tree -r --name-only "origin/${TARGET_BRANCH}" "$MIG_DIR" | sort | tail -1)
+          if [ "${GITHUB_EVENT_NAME}" = "pull_request" ]; then
+            # checkout above already did fetch-depth:0 (full history). Fetch the base
+            # WITHOUT --depth (a shallow graft would truncate the base history and
+            # break the merge-base when the base has moved ahead of the PR merge —
+            # exactly the long-branch-vs-moving-base case this gate guards, #361).
+            git fetch --no-tags origin "$TARGET_BRANCH"
+            BASE="origin/${TARGET_BRANCH}"
+          else
+            # push event: compare against the pre-push tip of the branch.
+            if [ "$BEFORE_SHA" = "0000000000000000000000000000000000000000" ]; then
+              echo "::notice::branch creation push — nothing to compare"
+              exit 0
+            fi
+            if ! git cat-file -e "${BEFORE_SHA}^{commit}" 2>/dev/null; then
+              # The before-SHA is not in the clone (a force-push rewrote history).
+              # One recovery attempt — refresh every remote head (cheap: the
+              # checkout is already fetch-depth:0); a fetch failure aborts via
+              # `set -e`, which is fail-closed too.
+              git fetch --no-tags origin '+refs/heads/*:refs/remotes/origin/*'
+            fi
+            if ! git cat-file -e "${BEFORE_SHA}^{commit}" 2>/dev/null; then
+              # FAIL-CLOSED: without the before-SHA there is no base to prove the
+              # ordering against, and a gate whose job is to BLOCK must not guess.
+              echo "::error::force-push detected — verify migration order manually, then re-run via workflow_dispatch"
+              exit 1
+            fi
+            BASE="$BEFORE_SHA"
+          fi
+          newest_on_target=$(git ls-tree -r --name-only "$BASE" "$MIG_DIR" | sort | tail -1)
          # NO `|| true`: a diff failure (e.g. an unresolved merge-base) must fail
          # the job CLOSED — a gate whose job is to BLOCK must never pass on error.
          # `set -e` above already aborts on a non-zero diff exit.
-          added=$(git diff --diff-filter=A --name-only "origin/${TARGET_BRANCH}...HEAD" -- "$MIG_DIR")
+          added=$(git diff --diff-filter=A --name-only "${BASE}...HEAD" -- "$MIG_DIR")
          bad=0
          for f in $added; do
            if [[ "$f" < "$newest_on_target" || "$f" == "$newest_on_target" ]]; then
-              echo "::error::Migration $f sorts at or before the newest on ${TARGET_BRANCH} ($newest_on_target) — rename it with a CURRENT timestamp before merge (do not change its contents). See incident #361."
+              echo "::error::Migration $f sorts at or before the newest on the base ($newest_on_target) — rename it with a CURRENT timestamp before merge (do not change its contents). See incident #361."
              bad=1
            fi
          done
@@ -121,13 +121,20 @@ export default function AiMcpServerForm({
  async function handleSubmit(values: FormValues) {
    const headers = resolveHeaders();

+    // An empty tag field means "no restriction" and must be sent as null —
+    // since #476 the server persists a literal `[]` as deny-all (zero tools),
+    // so an empty array from this form would silently disable every tool of
+    // the server. Deny-all remains expressible via the API, not via this form.
+    const toolAllowlist =
+      values.toolAllowlist.length === 0 ? null : values.toolAllowlist;
+
    if (isEdit && server) {
      const payload: IAiMcpServerUpdate = {
        id: server.id,
        name: values.name,
        transport: values.transport,
        url: values.url,
-        toolAllowlist: values.toolAllowlist,
+        toolAllowlist,
        // Always sent: a blank value clears the stored guidance (server -> null).
        instructions: values.instructions,
        enabled: values.enabled,
@@ -140,7 +147,7 @@ export default function AiMcpServerForm({
        name: values.name,
        transport: values.transport,
        url: values.url,
-        toolAllowlist: values.toolAllowlist,
+        toolAllowlist,
        // Blank => server stores null (no guidance).
        instructions: values.instructions,
        enabled: values.enabled,
@@ -27,7 +27,9 @@ export interface IAiMcpServerCreate {
  // Auth headers map (e.g. { Authorization: 'Bearer ...' }). Encrypted on save;
  // never returned.
  headers?: Record<string, string>;
-  toolAllowlist?: string[];
+  // Omit/null => no restriction; `[]` is persisted verbatim and means
+  // deny-all (zero tools) since #476.
+  toolAllowlist?: string[] | null;
  // Admin-authored prompt guidance (#180). Blank => stored as null.
  instructions?: string;
  enabled?: boolean;
@@ -43,7 +45,9 @@ export interface IAiMcpServerUpdate {
  transport?: McpTransport;
  url?: string;
  headers?: Record<string, string>;
-  toolAllowlist?: string[];
+  // Absent => unchanged; null => no restriction; `[]` is persisted verbatim
+  // and means deny-all (zero tools) since #476.
+  toolAllowlist?: string[] | null;
  // Admin-authored prompt guidance (#180). Absent => unchanged; blank => cleared.
  instructions?: string;
  enabled?: boolean;
@@ -37,10 +37,13 @@ export class CreateMcpServerDto {
  @IsObject()
  headers?: Record<string, string>;

+  // Omit/null => no restriction; `[]` is persisted verbatim and means deny-all
+  // (zero tools) since #476. @IsOptional() skips validation for null as well,
+  // so an explicit null is accepted.
  @IsOptional()
  @IsArray()
  @IsString({ each: true })
-  toolAllowlist?: string[];
+  toolAllowlist?: string[] | null;

  // Admin-authored guidance ("how/when to use this server's tools") injected
  // into the agent system prompt next to the tool descriptions (#180). Trusted,
@@ -38,10 +38,13 @@ export class UpdateMcpServerDto {
  @IsObject()
  headers?: Record<string, string>;

+  // Absent => unchanged; null => no restriction; `[]` is persisted verbatim
+  // and means deny-all (zero tools) since #476. @IsOptional() skips validation
+  // for null as well, so an explicit null is accepted.
  @IsOptional()
  @IsArray()
  @IsString({ each: true })
-  toolAllowlist?: string[];
+  toolAllowlist?: string[] | null;

  // Admin-authored prompt guidance (#180). Absent => unchanged; blank => cleared
  // (stored as null by the repo). Capped to bound prompt/token size.
@@ -0,0 +1,169 @@
+import { type Tool } from 'ai';
+import { McpClientsService } from './mcp-clients.service';
+
+/**
+ * Tool-allowlist filtering semantics on the merged external toolset (#476).
+ *
+ * COVERAGE CHOICE (documented per issue #476): the full corrupt-row chain
+ * (DB value -> repo normalizeRow -> toolsFor filter) is covered on TWO levels
+ * instead of one live-stub-MCP-server integration test:
+ *   (a) apps/server/test/integration/ai-mcp-server-repo.int-spec.ts pins the
+ *       repo read/write semantics against a real Postgres — `[]` round-trips
+ *       as jsonb `[]`, a present-but-corrupt value fails CLOSED to `[]` with
+ *       an error log;
+ *   (b) THIS spec pins what the toolset builder does with the repo's output —
+ *       null = unrestricted, `['alpha']` = only alpha, `[]` (including the
+ *       corrupt-row fallback) = ZERO tools.
+ * Together they prove the end-to-end property "corrupt/empty allowlist can
+ * never widen to all tools" without a live stub HTTP MCP server.
+ *
+ * The drive path mirrors mcp-namespacing.spec.ts: stub the repo's listEnabled,
+ * spy the private `connect` to return a fake client, inspect the merged keys.
+ */
+
+function fakeTool(): Tool {
+  return { description: 'x', inputSchema: undefined } as unknown as Tool;
+}
+
+interface FakeServer {
+  id: string;
+  name: string;
+  transport: string;
+  url: string;
+  headersEnc: string | null;
+  toolAllowlist: string[] | null;
+}
+
+function server(
+  over: Partial<FakeServer> & { id: string; name: string },
+): FakeServer {
+  return {
+    transport: 'http',
+    url: 'https://example.com/mcp',
+    headersEnc: null,
+    toolAllowlist: null,
+    ...over,
+  };
+}
+
+/**
+ * Build a service whose repo returns `servers` and whose fake clients expose
+ * `rawTools` from tools(). Returns the merged tool keys produced by toolsFor.
+ */
+async function mergedKeysFor(
+  servers: FakeServer[],
+  rawTools: Record<string, Tool>,
+): Promise<string[]> {
+  const repoStub = {
+    listEnabled: jest.fn().mockResolvedValue(servers),
+  };
+  const service = new McpClientsService(repoStub as never, {} as never);
+
+  jest
+    .spyOn(
+      service as unknown as { connect: (s: FakeServer) => unknown },
+      'connect',
+    )
+    .mockImplementation(() =>
+      Promise.resolve({
+        tools: () => Promise.resolve(rawTools),
+        close: () => Promise.resolve(),
+      }),
+    );
+
+  const toolset = await service.toolsFor('ws-1');
+  // Release the lease so the service does not hold the fake clients open.
+  await Promise.all(toolset.clients.map((c) => c.close()));
+  return Object.keys(toolset.tools);
+}
+
+describe('external MCP tool-allowlist filtering (via toolsFor, #476)', () => {
+  afterEach(() => jest.restoreAllMocks());
+
+  const RAW = () => ({
+    alpha: fakeTool(),
+    beta: fakeTool(),
+    gamma: fakeTool(),
+  });
+
+  it("['alpha'] lets ONLY alpha through", async () => {
+    const keys = await mergedKeysFor(
+      [server({ id: 'id-1', name: 'srv', toolAllowlist: ['alpha'] })],
+      RAW(),
+    );
+    expect(keys).toEqual(['srv_alpha']);
+  });
+
+  it('null (no restriction) lets every tool through', async () => {
+    const keys = await mergedKeysFor(
+      [server({ id: 'id-1', name: 'srv', toolAllowlist: null })],
+      RAW(),
+    );
+    expect(keys.sort()).toEqual(['srv_alpha', 'srv_beta', 'srv_gamma']);
+  });
+
+  it('[] (deny-all) yields ZERO tools — an empty array is authoritative, not falsy (#476)', async () => {
+    // This is the regression the #476 change guards: `[]` used to fall through
+    // the old `allow.length > 0` check and expose ALL tools. It must expose NONE.
+    const keys = await mergedKeysFor(
+      [server({ id: 'id-1', name: 'srv', toolAllowlist: [] })],
+      RAW(),
+    );
+    expect(keys).toEqual([]);
+  });
+
+  it('the corrupt-row fallback ([] from the repo) also yields ZERO tools (#476)', async () => {
+    // The repo turns a present-but-corrupt tool_allowlist into `[]` (fail-closed,
+    // see normalizeRow in ai-mcp-server.repo.ts + the int-spec); this pins that
+    // the toolset builder honours that fallback as deny-all rather than allow-all.
+    const corruptFallback: string[] = [];
+    const keys = await mergedKeysFor(
+      [server({ id: 'id-1', name: 'srv', toolAllowlist: corruptFallback })],
+      RAW(),
+    );
+    expect(keys).toEqual([]);
+  });
+
+  it('allowlisted names not exposed by the server are ignored (no phantom tools)', async () => {
+    const keys = await mergedKeysFor(
+      [
+        server({
+          id: 'id-1',
+          name: 'srv',
+          toolAllowlist: ['alpha', 'does-not-exist'],
+        }),
+      ],
+      RAW(),
+    );
+    expect(keys).toEqual(['srv_alpha']);
+  });
+
+  it('a deny-all server contributes no prompt instructions (0 tools merged)', async () => {
+    const repoStub = {
+      listEnabled: jest.fn().mockResolvedValue([
+        {
+          ...server({ id: 'id-1', name: 'srv', toolAllowlist: [] }),
+          instructions: 'use the tools wisely',
+        },
+      ]),
+    };
+    const service = new McpClientsService(repoStub as never, {} as never);
+    jest
+      .spyOn(
+        service as unknown as { connect: (s: FakeServer) => unknown },
+        'connect',
+      )
+      .mockImplementation(() =>
+        Promise.resolve({
+          tools: () => Promise.resolve(RAW()),
+          close: () => Promise.resolve(),
+        }),
+      );
+
+    const toolset = await service.toolsFor('ws-1');
+    await Promise.all(toolset.clients.map((c) => c.close()));
+    expect(Object.keys(toolset.tools)).toEqual([]);
+    // mergeNamespaced reported 0 contributed tools, so no guidance is attached.
+    expect(toolset.instructions).toEqual([]);
+  });
+});
@@ -285,9 +285,13 @@ export class McpClientsService {
      try {
        client = await this.connectWithTimeout(server, CONNECT_TIMEOUT_MS);
        const raw = await withTimeout(client.tools(), CONNECT_TIMEOUT_MS);
+        // Allowlist semantics (#476): null/absent = no restriction (all tools);
+        // ANY array — including `[]` — is authoritative, so an EMPTY allowlist
+        // yields ZERO tools (deny-all). Do NOT add a `.length > 0` escape here:
+        // that read `[]` as falsy and silently widened deny-all to allow-all
+        // (the repo also fails corrupt rows closed to `[]` for the same reason).
        const allow = server.toolAllowlist;
-        const picked =
-          Array.isArray(allow) && allow.length > 0 ? pick(raw, allow) : raw;
+        const picked = Array.isArray(allow) ? pick(raw, allow) : raw;
        // Bound each tool's execute with a per-call total-timeout guard before
        // merging, so a single chatty-but-stuck call is aborted after the cap.
        const guarded = wrapToolsWithCallTimeout(picked, callTimeoutMs);
@@ -100,7 +100,8 @@ export class McpServersService {
      transport: dto.transport,
      url: dto.url,
      headersEnc,
-      // undefined => unchanged; [] / value handled by repo (empty => null).
+      // undefined => unchanged; null => no restriction; `[]` is persisted
+      // verbatim and means deny-all (#476).
      toolAllowlist: dto.toolAllowlist,
      // undefined => unchanged; blank => cleared (null) by the repo.
      instructions: dto.instructions,
@@ -35,4 +35,25 @@ describe('jsonbBind', () => {
    expect(out).not.toBeNull();
    expect(out).toBeDefined();
  });
+
+  // preserveEmpty (#476): opts a column OUT of the empty-to-null collapse so an
+  // empty container is persisted verbatim (e.g. `[]` = deny-all for
+  // tool_allowlist). null stays null regardless of the flag.
+  describe('preserveEmpty', () => {
+    it('returns a (non-null) bind for an empty array', () => {
+      const out = jsonbBind([], { preserveEmpty: true });
+      expect(out).not.toBeNull();
+      expect(out).toBeDefined();
+    });
+
+    it('returns a (non-null) bind for an empty object', () => {
+      const out = jsonbBind({}, { preserveEmpty: true });
+      expect(out).not.toBeNull();
+      expect(out).toBeDefined();
+    });
+
+    it('still returns null for null (null means null, flag or not)', () => {
+      expect(jsonbBind(null, { preserveEmpty: true })).toBeNull();
+    });
+  });
 });
@@ -78,7 +78,9 @@ export class AiMcpServerRepo {
        headersEnc: values.headersEnc ?? null,
        // jsonb column: the postgres driver would otherwise encode a JS array as
        // a Postgres array literal. Bind the JSON text and cast it to jsonb.
-        toolAllowlist: jsonbBind(values.toolAllowlist),
+        // preserveEmpty (#476): `[]` is a real value here (deny-all), distinct
+        // from null ("no restriction") — it must round-trip as `[]`, not null.
+        toolAllowlist: jsonbBind(values.toolAllowlist, { preserveEmpty: true }),
        // Plain text column: blank/whitespace-only guidance is stored as null.
        instructions: blankToNull(values.instructions),
        enabled: values.enabled ?? true,
@@ -111,7 +113,10 @@ export class AiMcpServerRepo {
    if (patch.url !== undefined) set.url = patch.url;
    if (patch.headersEnc !== undefined) set.headersEnc = patch.headersEnc;
    if (patch.toolAllowlist !== undefined) {
-      set.toolAllowlist = jsonbBind(patch.toolAllowlist);
+      // preserveEmpty (#476): see insert — `[]` (deny-all) must not become null.
+      set.toolAllowlist = jsonbBind(patch.toolAllowlist, {
+        preserveEmpty: true,
+      });
    }
    if (patch.instructions !== undefined) {
      // Blank/whitespace-only guidance clears the column (stored as null).
@@ -158,7 +163,9 @@ export function blankToNull(value: string | null | undefined): string | null {
 * fix), so the driver hands back a string like `'["a","b"]'` rather than an
 * array. Be tolerant: normalize a JSON string to its value, then accept it only
 * if it is an array of strings; null / a non-array / unparseable value / an
- * array with a non-string element all become null (unrestricted).
+ * array with a non-string element all become null. NOTE: null here only means
+ * "could not parse" — the null-vs-deny-all policy decision lives in
+ * normalizeRow (#476: present-but-corrupt fails CLOSED to `[]`).
 */
 export function parseToolAllowlist(value: unknown): string[] | null {
  // Shape guard only; the legacy double-encoding self-heal lives in
@@ -173,17 +180,20 @@ export function parseToolAllowlist(value: unknown): string[] | null {
 /**
 * Normalize a DB row so `toolAllowlist` is always `string[] | null`.
 *
- * FAIL-OPEN logging: a stored value that is present but cannot be parsed into a
- * string[] (corrupt JSON, a non-array, non-string elements) degrades to `null` =
- * "no restriction", so the agent silently gets ALL of the server's tools. Log
- * one line (server id only, never the contents) so that widening is not silent.
+ * FAIL-CLOSED (#476): a stored value that is PRESENT but cannot be parsed into
+ * a string[] (corrupt JSON, a non-array, non-string elements) degrades to `[]`
+ * = deny-all, so a corrupted allowlist can never silently widen to "the agent
+ * gets ALL of the server's tools" (the old fail-open null). An error line is
+ * logged (server id only, never the contents) so the admin can repair the row.
+ * A column that is truly NULL/absent stays `null` = "no restriction".
 */
 function normalizeRow(row: AiMcpServer): AiMcpServer {
  const parsed = parseToolAllowlist(row.toolAllowlist);
  if (parsed === null && row.toolAllowlist != null) {
-    logger.warn(
-      `Corrupt tool_allowlist for MCP server ${row.id}; ignoring it (no tool restriction applied)`,
+    logger.error(
+      `Corrupt tool_allowlist for MCP server ${row.id}; failing closed (NO tools allowed) — re-save the server's allowlist to repair it`,
    );
+    return { ...row, toolAllowlist: [] };
  }
  return { ...row, toolAllowlist: parsed };
 }
@@ -78,18 +78,30 @@ export function violatedConstraint(err: unknown): string | undefined {
 * verbatim); `::jsonb` then parses it into a real array/object. Read-side
 * parsers repair rows written the old buggy way without a migration.
 *
- * Returns `null` for null/undefined and for "empty" values (an empty array, or
- * an object with no own enumerable keys) — callers treat empty as "clear/unset",
- * so an empty allowlist/config never round-trips as `[]`/`{}`.
+ * Returns `null` for null/undefined. By default it ALSO returns `null` for
+ * "empty" values (an empty array, or an object with no own enumerable keys) —
+ * most callers treat empty as "clear/unset", so an empty config never
+ * round-trips as `[]`/`{}`.
+ *
+ * `preserveEmpty` (issue #476) opts a column OUT of that empty-to-null
+ * normalization so `[]`/`{}` are persisted as real jsonb values. Needed where
+ * empty and null mean DIFFERENT things: an empty `tool_allowlist` is
+ * deny-all ("zero tools allowed"), while null is "no restriction" — collapsing
+ * `[]` to null silently widened deny-all to allow-all. Deliberately an opt-in
+ * flag, NOT a global change: the other jsonb callers (model_config, source)
+ * keep the empty-means-unset contract.
 */
 export function jsonbBind<T>(
  value: T | null | undefined,
+  opts?: { preserveEmpty?: boolean },
 ): RawBuilder<T> | null {
  if (value === null || value === undefined) return null;
-  if (Array.isArray(value)) {
-    if (value.length === 0) return null;
-  } else if (typeof value === 'object') {
-    if (Object.keys(value as object).length === 0) return null;
+  if (!opts?.preserveEmpty) {
+    if (Array.isArray(value)) {
+      if (value.length === 0) return null;
+    } else if (typeof value === 'object') {
+      if (Object.keys(value as object).length === 0) return null;
+    }
  }
  return sql<T>`${JSON.stringify(value)}::text::jsonb`;
 }
@@ -1,5 +1,6 @@
 import { Kysely, sql } from 'kysely';
 import { randomUUID } from 'node:crypto';
+import { Logger } from '@nestjs/common';
 import { AiMcpServerRepo } from '@docmost/db/repos/ai-chat/ai-mcp-server.repo';
 import { getTestDb, destroyTestDb, createWorkspace } from './db';

@@ -54,7 +55,11 @@ describe('AiMcpServerRepo tool_allowlist jsonb round-trip [integration]', () =>
    expect(Array.isArray(found?.toolAllowlist)).toBe(true);
  });

-  it('an empty allowlist is normalized to null (no restriction), not []', async () => {
+  // #476 (deliberate behaviour change): an empty allowlist used to be
+  // normalized to SQL NULL, which downstream means "no restriction" — so an
+  // admin's deny-all `[]` silently became allow-all. It must now round-trip as
+  // a real jsonb `[]` (deny-all), distinct from NULL.
+  it('an empty allowlist round-trips as jsonb [] (deny-all), not null (#476)', async () => {
    const row = await repo.insert({
      workspaceId: ws,
      name: `srv-${randomUUID()}`,
@@ -62,7 +67,27 @@ describe('AiMcpServerRepo tool_allowlist jsonb round-trip [integration]', () =>
      url: 'https://example.com/mcp',
      toolAllowlist: [],
    });
-    // The column is SQL NULL, so jsonb_typeof returns SQL NULL (JS null).
+    // The column holds a real (empty) jsonb ARRAY, not SQL NULL.
+    expect(await jsonbTypeof(row.id)).toBe('array');
+    expect((await repo.findById(row.id, ws))?.toolAllowlist).toEqual([]);
+  });
+
+  it('update to [] persists jsonb [] and update to null clears to SQL NULL (#476)', async () => {
+    const row = await repo.insert({
+      workspaceId: ws,
+      name: `srv-${randomUUID()}`,
+      transport: 'http',
+      url: 'https://example.com/mcp',
+      toolAllowlist: ['search'],
+    });
+
+    // Deny-all via update: [] must survive as a real jsonb array.
+    await repo.update(row.id, ws, { toolAllowlist: [] });
+    expect(await jsonbTypeof(row.id)).toBe('array');
+    expect((await repo.findById(row.id, ws))?.toolAllowlist).toEqual([]);
+
+    // Explicit clear (null) still means "no restriction" = SQL NULL.
+    await repo.update(row.id, ws, { toolAllowlist: null });
    expect(await jsonbTypeof(row.id)).toBeNull();
    expect((await repo.findById(row.id, ws))?.toolAllowlist).toBeNull();
  });
@@ -92,23 +117,60 @@ describe('AiMcpServerRepo tool_allowlist jsonb round-trip [integration]', () =>
    expect(healed?.toolAllowlist).toEqual(['alpha', 'beta']);
  });

-  it('FAIL-OPEN: a present-but-corrupt tool_allowlist reads back as null (no restriction)', async () => {
-    // #185 re-review pt 8: normalizeRow's fail-open branch — the column is
-    // PRESENT but does not parse into a string[] (here a jsonb string scalar
-    // holding non-array JSON). The read must degrade to `null` ("no restriction"),
-    // not crash. (A warn is logged with the server id; not asserted here.)
-    const id = randomUUID();
-    await sql`
-      INSERT INTO ai_mcp_servers (id, workspace_id, name, transport, url, tool_allowlist)
-      VALUES (
-        ${id}, ${ws}, ${`srv-${id}`}, 'http', 'https://example.com/mcp',
-        to_jsonb(${'{"not":"an array"}'}::text)
-      )
-    `.execute(db);
-    // Sanity: the column is present (a jsonb string scalar), not SQL NULL.
-    expect(await jsonbTypeof(id)).toBe('string');
-    // ...yet the read degrades to null (fail-open).
-    expect((await repo.findById(id, ws))?.toolAllowlist).toBeNull();
+  // #476 (deliberate behaviour change, replaces the old FAIL-OPEN pin): a
+  // present-but-corrupt tool_allowlist used to degrade to null ("no
+  // restriction"), silently handing the agent ALL of the server's tools. It
+  // must now FAIL CLOSED to `[]` (deny-all) and log an error.
+  it('FAIL-CLOSED: a present-but-corrupt tool_allowlist reads back as [] (deny-all) + error log (#476)', async () => {
+    const errorSpy = jest
+      .spyOn(Logger.prototype, 'error')
+      .mockImplementation(() => undefined);
+    try {
+      // The column is PRESENT but does not parse into a string[] — a jsonb
+      // string scalar holding unparseable text (a truncated legacy write).
+      const id = randomUUID();
+      await sql`
+        INSERT INTO ai_mcp_servers (id, workspace_id, name, transport, url, tool_allowlist)
+        VALUES (
+          ${id}, ${ws}, ${`srv-${id}`}, 'http', 'https://example.com/mcp',
+          to_jsonb(${'{oops'}::text)
+        )
+      `.execute(db);
+      // Sanity: the column is present (a jsonb string scalar), not SQL NULL.
+      expect(await jsonbTypeof(id)).toBe('string');
+      // ...and the read degrades to [] (fail-closed deny-all), not null.
+      expect((await repo.findById(id, ws))?.toolAllowlist).toEqual([]);
+      // The narrowing is not silent: an error names the server id (never the
+      // corrupt contents).
+      expect(errorSpy).toHaveBeenCalledWith(
+        expect.stringContaining(`Corrupt tool_allowlist for MCP server ${id}`),
+      );
+      expect(
+        errorSpy.mock.calls.some((c) => String(c[0]).includes('{oops')),
+      ).toBe(false);
+    } finally {
+      errorSpy.mockRestore();
+    }
+  });
+
+  it('FAIL-CLOSED: corrupt non-array JSON (an object) also reads back as [] (#476)', async () => {
+    const errorSpy = jest
+      .spyOn(Logger.prototype, 'error')
+      .mockImplementation(() => undefined);
+    try {
+      const id = randomUUID();
+      await sql`
+        INSERT INTO ai_mcp_servers (id, workspace_id, name, transport, url, tool_allowlist)
+        VALUES (
+          ${id}, ${ws}, ${`srv-${id}`}, 'http', 'https://example.com/mcp',
+          to_jsonb(${'{"not":"an array"}'}::text)
+        )
+      `.execute(db);
+      expect(await jsonbTypeof(id)).toBe('string');
+      expect((await repo.findById(id, ws))?.toolAllowlist).toEqual([]);
+    } finally {
+      errorSpy.mockRestore();
+    }
  });
 });

@@ -434,6 +434,71 @@ async function main() {
      }
    }

+    // 6h. markdown converter fixpoint (#476): pins the converter fixpoint
+    // THROUGH the live server/collab path, not just the package tests. The
+    // unit corpus (docmost-md-roundtrip) proves the converter alone is a
+    // fixpoint; this asserts the property survives the real pipeline — export
+    // (REST read, PM -> MD) -> import (MD -> PM -> collab replace -> server
+    // persistence) -> export — where the server schema, the Yjs structural
+    // diff or the collab write path could still mangle the doc while every
+    // unit test stays green. importPageMarkdown is the designed inverse of
+    // exportPageMarkdown (the self-contained envelope with meta/comments
+    // blocks); updatePageMarkdown (client.updatePage) takes plain authoring
+    // markdown and would re-import the envelope blocks as literal content.
+    {
+      const FIXMD = [
+        "# Fixpoint heading",
+        "",
+        "Paragraph with **bold**, *italic* and a [link](https://example.com).",
+        "",
+        "## Second level",
+        "",
+        "- bullet one",
+        "- bullet two",
+        "",
+        "1. ordered one",
+        "2. ordered two",
+        "",
+        "```js",
+        "const answer = 42; // code block must survive byte-identically",
+        "```",
+        "",
+        "| A | B |",
+        "| --- | --- |",
+        "| one | two |",
+        "",
+        ":::info",
+        "Callout body.",
+        ":::",
+      ].join("\n");
+      const fx = await client.createPage("E2E md fixpoint " + Date.now(), FIXMD, spaceId);
+      const fxid = fx.data.id;
+      try {
+        const md1 = await client.exportPageMarkdown(fxid);
+        await client.importPageMarkdown(fxid, md1);
+        await new Promise((r) => setTimeout(r, 16000)); // wait for server persistence
+        const md2 = await client.exportPageMarkdown(fxid);
+        // On failure, name the first diverging line of the two exports.
+        const firstDiff = (a, b) => {
+          const al = a.split("\n");
+          const bl = b.split("\n");
+          for (let i = 0; i < Math.max(al.length, bl.length); i++) {
+            if (al[i] !== bl[i]) {
+              return `first diff at line ${i + 1}: ${JSON.stringify(al[i] ?? "<EOF>")} -> ${JSON.stringify(bl[i] ?? "<EOF>")}`;
+            }
+          }
+          return "same lines, different bytes (line endings?)";
+        };
+        check(
+          "markdown fixpoint: export -> import -> export is byte-identical",
+          md1 === md2,
+          md1 === md2 ? "" : firstDiff(md1, md2),
+        );
+      } finally {
+        try { await client.deletePage(fxid); } catch {}
+      }
+    }
+
    // 7. shares: create (idempotent), public access, list, unshare
    const share = await client.sharePage(pageId);
    check("sharePage: returns public URL", share.publicUrl?.startsWith(`${APP}/share/`), share.publicUrl);
@@ -9,6 +9,7 @@ import {
  __sessionCountForTests,
 } from "../../build/lib/collab-session.js";
 import { withPageLock } from "../../build/lib/page-lock.js";
+import { DocmostClient } from "../../build/client.js";

 // A stand-in for HocuspocusProvider: it shares the ydoc (so the real yjs
 // read/transform/write in CollabSession.mutate runs unchanged), auto-completes
@@ -89,6 +90,7 @@ const ENV_KEYS = [
  "MCP_COLLAB_SESSION_IDLE_MS",
  "MCP_COLLAB_SESSION_MAX_AGE_MS",
  "MCP_COLLAB_SESSION_MAX_ENTRIES",
+  "MCP_COLLAB_TOKEN_TTL_MS",
 ];
 let savedEnv;

@@ -345,6 +347,86 @@ test("replaceImage-shaped flow: acquire under an EXTERNAL page lock does not dea
  );
 });

+// --- #439: the collab-token cache is what makes the session cache ACTUALLY hit ---
+//
+// WHY these two tests exist (the #435 incident): the session registry keys on
+// (wsUrl, pageId, token) for identity isolation, but BOTH production token
+// sources mint a FRESH JWT on every call (the in-app provider re-signs a JWT
+// whose iat/exp changes every second; the external MCP POSTs /auth/collab-token
+// per call). The fresh token per call made the session-registry key unstable,
+// so the prod hit-rate was 0% — connect storms, 25s timeouts, zombie sessions —
+// while every other test in this file stayed green because they pass a FIXED
+// "tok" string. The #439 fix is the per-client collab-token cache
+// (DocmostClient.getCollabTokenWithReauth + MCP_COLLAB_TOKEN_TTL_MS); these
+// tests drive the token through it with a source that returns a DIFFERENT
+// fresh JWT per mint, exactly like prod, so a regression in EITHER the token
+// cache or the registry keying turns them red.
+//
+// getCollabTokenWithReauth is TS-private, but the compiled JS exposes it; the
+// tests call it directly because that is exactly the per-op composition of the
+// production call sites (updatePage etc.: mint the token, then acquire).
+
+test("#439 token cache ON: fresh-JWT-per-mint source, two ops => ONE connect (session cache hits)", async () => {
+  process.env.MCP_COLLAB_TOKEN_TTL_MS = "300000"; // cache ON (explicit, not default-dependent)
+  let mints = 0;
+  const client = new DocmostClient({
+    apiUrl: "http://h/api",
+    getToken: async () => "user-jwt",
+    // Like both prod sources: a DIFFERENT fresh JWT on every mint.
+    getCollabToken: async () => `fresh-jwt-${++mints}`,
+  });
+
+  // Op 1: mint the collab token through the client, then acquire + mutate.
+  const tok1 = await client.getCollabTokenWithReauth();
+  const s1 = await acquireCollabSession("page-1", tok1, "http://h/api");
+  await s1.mutate(() => docWith("one"));
+
+  // Op 2: the same identity mints again — the cache must serve the SAME token.
+  const tok2 = await client.getCollabTokenWithReauth();
+  const s2 = await acquireCollabSession("page-1", tok2, "http://h/api");
+  await s2.mutate(() => docWith("two"));
+
+  assert.equal(mints, 1, "the second op is served from the token cache");
+  assert.equal(tok2, tok1, "stable token => stable session-registry key");
+  assert.equal(s2, s1, "the live session is reused");
+  assert.equal(
+    FakeProvider.connectCount,
+    1,
+    "two mutations over one identity must cost exactly ONE real connect",
+  );
+  assert.equal(__sessionCountForTests(), 1);
+});
+
+test("#439 negative control: token cache OFF (TTL=0) reproduces the #435 churn — two ops => TWO connects", async () => {
+  process.env.MCP_COLLAB_TOKEN_TTL_MS = "0"; // explicit 0 disables the cache (fetch-per-call legacy)
+  let mints = 0;
+  const client = new DocmostClient({
+    apiUrl: "http://h/api",
+    getToken: async () => "user-jwt",
+    getCollabToken: async () => `fresh-jwt-${++mints}`,
+  });
+
+  const tok1 = await client.getCollabTokenWithReauth();
+  const s1 = await acquireCollabSession("page-1", tok1, "http://h/api");
+  await s1.mutate(() => docWith("one"));
+
+  const tok2 = await client.getCollabTokenWithReauth();
+  const s2 = await acquireCollabSession("page-1", tok2, "http://h/api");
+  await s2.mutate(() => docWith("two"));
+
+  assert.equal(mints, 2, "without the cache every op mints its own token");
+  assert.notEqual(tok2, tok1, "unstable token => unstable session-registry key");
+  assert.notEqual(s2, s1, "no session reuse");
+  assert.equal(
+    FakeProvider.connectCount,
+    2,
+    "a full reconnect per op — the #435 storm in miniature",
+  );
+  // The first session lingers under its now-unreachable key until its idle
+  // TTL — the zombie-session symptom of the incident.
+  assert.equal(__sessionCountForTests(), 2);
+});
+
 test("destroyAllSessions tears down every cached session", async () => {
  await acquireCollabSession("page-1", "tok", "http://h/api");
  await acquireCollabSession("page-2", "tok", "http://h/api");
@@ -0,0 +1,79 @@
+#!/usr/bin/env bash
+# Boot-smoke for the exact Docker image that is about to be pushed (issue #476).
+#
+# Retrospective class "local logic is right, the integration property was never
+# checked" (#353/#452/#361): every other CI job builds and tests code from the
+# working tree, but the IMAGE watchtower pulls was never actually started
+# anywhere before this gate. This script boots the built image against the
+# publish job's postgres/redis services and asserts four integration
+# properties end-to-end:
+#   S1  the app boots and /api/health answers (startup migrator + boot)
+#   S2  the first-run workspace setup endpoint works (API + DB writes)
+#   S3  the client dist is inside the image and served
+#   S4  hashed assets are served immutable (#452) with the precompressed
+#       brotli copy shipped in the image
+set -euo pipefail
+
+IMAGE="${1:?usage: image-smoke.sh <image>}"
+
+fail() { echo "FAIL: $*"; exit 1; }
+
+# Boot the exact image that will be pushed, wired to the job services via host
+# network (postgres on localhost:5432, redis on localhost:6379). The container
+# is deliberately NOT removed on failure so the workflow's dump-on-failure step
+# can read `docker logs gitmost-smoke`.
+docker run -d --name gitmost-smoke --network host \
+  -e DATABASE_URL=postgresql://docmost:docmost@localhost:5432/docmost \
+  -e REDIS_URL=redis://localhost:6379 \
+  -e APP_SECRET=ci-smoke-secret-change-me-min-32-characters \
+  -e APP_URL=http://localhost:3000 \
+  "$IMAGE"
+
+# S1: wait for /api/health — covers the startup migrator + boot inside the
+# shipped image (#361-boot, #353 runtime class): a migration the Kysely startup
+# migrator rejects, or a runtime module missing from the image, dies right here.
+healthy=0
+for _ in $(seq 1 60); do
+  if curl -fsS http://localhost:3000/api/health > /dev/null 2>&1; then
+    healthy=1
+    break
+  fi
+  sleep 2
+done
+[ "$healthy" -eq 1 ] || fail "S1: /api/health did not answer within 120s (boot or startup migration failed)"
+echo "OK S1: image booted and /api/health answers"
+
+# S2: the first-run workspace setup works end-to-end (controller -> service ->
+# DB write chain inside the shipped image, not just a static health probe).
+curl -fsS -X POST http://localhost:3000/api/auth/setup \
+  -H "Content-Type: application/json" \
+  -d '{"name":"Smoke","email":"smoke@example.com","password":"SmokePassword123","workspaceName":"Smoke"}' \
+  > /dev/null || fail "S2: POST /api/auth/setup failed"
+echo "OK S2: workspace setup succeeded"
+
+# S3: the client dist is actually inside the image and served — the SPA HTML
+# must reference hashed /assets/ bundles (a broken client COPY in the
+# Dockerfile would serve an empty shell that every other job stays green on).
+HTML=$(curl -fsS http://localhost:3000/) || fail "S3: fetching / failed"
+grep -q '/assets/' <<<"$HTML" || fail "S3: served HTML references no /assets/ bundle (client dist missing from the image?)"
+echo "OK S3: client dist served (HTML references /assets/)"
+
+# S4: hashed /assets/ files must be served with an immutable cache-control
+# (#452 class: static.module.ts resolveStaticAssetHeaders owns the header) AND
+# with the precompressed brotli neighbour. Both checks are mandatory — verified
+# against the code: resolveStaticAssetHeaders marks every /assets/ path
+# immutable, and the client build (vite-plugin-compression2, include covers
+# .js) emits a .br copy next to every bundle that the Dockerfile ships and
+# @fastify/static serves via preCompressed:true.
+ASSET=$(grep -oE '/assets/[A-Za-z0-9._@/-]+\.js' <<<"$HTML" | head -1 || true)
+[ -n "$ASSET" ] || fail "S4: no /assets/*.js path found in the served HTML"
+HDRS=$(curl -fsSI -H 'Accept-Encoding: br' "http://localhost:3000$ASSET") || fail "S4: HEAD $ASSET failed"
+grep -qi '^cache-control:.*immutable' <<<"$HDRS" || fail "S4: $ASSET served without an immutable cache-control (#452)"
+echo "OK S4: hashed asset served with immutable cache-control"
+grep -qi '^content-encoding:.*br' <<<"$HDRS" || fail "S4: $ASSET not served brotli-precompressed (content-encoding: br missing)"
+echo "OK S4: hashed asset served with the precompressed brotli copy"
+
+# Remove the container ONLY on success, so the failure path keeps it around for
+# the workflow's "Dump smoke container log on failure" step.
+docker rm -f gitmost-smoke > /dev/null
+echo "OK image smoke passed"