docs(page-tree): correct isUnloadedBranch comment — no DnD-guard consumer (#525 review)

The comment falsely listed a 'DnD move guard' consumer; no DnD path routes through the predicate (local DnD/create-page use the raw index-based insert). List the real consumers (handleToggle + realtime insertByPosition/placeByPosition) and note the local raw-insert path as a #525 follow-up. Comment-only. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-07-12 04:13:49 +03:00
parent c765483947 9a94c8df18
commit 0af7eb30c3
150 changed files with 8361 additions and 1057 deletions
@@ -6,10 +6,13 @@ describe("estimateTokens", () => {
    expect(estimateTokens("")).toBe(0);
  });

-  it("ceils chars/4 so any non-empty text is at least 1 token", () => {
+  // #490: migrated onto the shared @docmost/token-estimate module (chars/2.5, up
+  // from the old client-only chars/4) so the client counter and the server replay
+  // budgeter can never diverge.
+  it("ceils chars/2.5 so any non-empty text is at least 1 token", () => {
    expect(estimateTokens("a")).toBe(1);
-    expect(estimateTokens("abcd")).toBe(1);
-    expect(estimateTokens("abcde")).toBe(2);
-    expect(estimateTokens("12345678")).toBe(2);
+    expect(estimateTokens("ab")).toBe(1);
+    expect(estimateTokens("abcde")).toBe(2); // 5 / 2.5 = 2
+    expect(estimateTokens("x".repeat(10))).toBe(4); // 10 / 2.5 = 4
  });
 });
@@ -2,18 +2,10 @@
 * Rough client-side token estimation for AI-chat UI affordances.
 *
 * No provider streams exact per-token usage mid-stream, so any in-flight figure
- * is a CLIENT ESTIMATE (chars/≈4 heuristic). Pure + unit-testable: it never runs
- * a real BPE tokenizer (that would be O(n²) on the hot path, bloat the bundle,
- * and be wrong for Gemini/Ollama anyway). Used by the in-body reasoning counter
- * ("Thinking · N tokens").
+ * is a CLIENT ESTIMATE. This re-exports the SHARED estimator from
+ * `@docmost/token-estimate` (chars/2.5) so the in-body counter and the server's
+ * replay budgeter use the SAME heuristic — two divergent estimators would mean
+ * "the badge shows 60%" while "the budgeter already trimmed" (#490). Used by the
+ * in-body reasoning counter ("Thinking · N tokens").
 */
-
-/**
- * Rough token estimate for a piece of text using the standard chars/≈4 heuristic.
- * Returns 0 for empty/whitespace-free-of-content input, and ceils so any
- * non-empty text counts as at least one token.
- */
-export function estimateTokens(text: string): number {
-  if (!text) return 0;
-  return Math.ceil(text.length / 4);
-}
+export { estimateTokens } from "@docmost/token-estimate";
@@ -89,6 +89,23 @@ describe("describeChatError", () => {
    expect(view.title).not.toBe("AI provider not configured");
  });

+  it("classifies a token-degeneration abort under the SAME 'Response stopped.' marker the live view shows (#495)", () => {
+    // The exact reason the server persists in metadata.error on a degeneration
+    // abort (ai-chat.service OUTPUT_DEGENERATION_ERROR). Live, this event shows
+    // the neutral "Response stopped." notice; the persisted banner MUST match it
+    // so live and refetch never disagree.
+    const view = describeChatError(
+      "Output degeneration detected (repeated token loop)",
+      t,
+    );
+    expect(view.title).toBe("Response stopped.");
+    expect(view.detail).toBe(
+      "The answer was stopped automatically because the model fell into a repeated output loop.",
+    );
+    // Regression guard: it must NOT fall through to the generic heading.
+    expect(view.title).not.toBe("Something went wrong");
+  });
+
  it("classifies a dropped connection (ECONNRESET) as a lost-connection error", () => {
    expect(
      describeChatError("Cannot connect to API: read ECONNRESET", t).title,
@@ -77,6 +77,22 @@ export function describeChatError(
    };
  }

+  // Our own token-degeneration abort (#444): the server aborts a runaway
+  // repetition loop and persists this exact reason in metadata.error. LIVE, the
+  // same abort surfaces as the neutral "Response stopped." notice (the client
+  // cannot tell it from a manual Stop mid-stream), so the persisted banner must
+  // read the SAME "Response stopped." marker — otherwise the live view and a
+  // later refetch show two different texts for one event. The detail explains the
+  // loop-guard cause without contradicting the shared heading.
+  if (/output degeneration detected|repeated token loop/i.test(msg)) {
+    return {
+      title: t("Response stopped."),
+      detail: t(
+        "The answer was stopped automatically because the model fell into a repeated output loop.",
+      ),
+    };
+  }
+
  if (/"statusCode"\s*:\s*403\b/.test(msg)) {
    return {
      title: t("AI chat is disabled"),
@@ -0,0 +1,65 @@
+import { describe, it, expect } from "vitest";
+import * as Y from "yjs";
+import { yHistoryAvailability } from "./use-toolbar-state.ts";
+
+// Undo/redo availability is derived from the Yjs UndoManager's PRIVATE
+// `undoStack` / `redoStack` fields (see use-toolbar-state.ts for why we read the
+// stack lengths directly instead of the expensive `editor.can().undo()` dry-run).
+// These tests lock in the behavior AND pin the library shape so a yjs / y-undo
+// upgrade that renames/restructures those internals fails loudly here rather than
+// silently enabling/disabling the toolbar buttons in production.
+describe("yHistoryAvailability", () => {
+  it("reports availability from the stack lengths", () => {
+    expect(yHistoryAvailability({ undoStack: [], redoStack: [] })).toEqual({
+      canUndo: false,
+      canRedo: false,
+    });
+    expect(
+      yHistoryAvailability({ undoStack: [{}], redoStack: [] }),
+    ).toEqual({ canUndo: true, canRedo: false });
+    expect(
+      yHistoryAvailability({ undoStack: [{}], redoStack: [{}, {}] }),
+    ).toEqual({ canUndo: true, canRedo: true });
+  });
+
+  it("returns null when the private stack shape is unrecognized (upgrade guard)", () => {
+    // Simulates a yjs / y-undo upgrade that renames or restructures the private
+    // fields: the caller then falls back to the safe prosemirror-history default
+    // instead of throwing on `.length` of undefined or reading garbage.
+    expect(yHistoryAvailability(undefined)).toBeNull();
+    expect(yHistoryAvailability(null)).toBeNull();
+    expect(yHistoryAvailability({})).toBeNull();
+    expect(yHistoryAvailability({ undoStack: 5, redoStack: 5 })).toBeNull();
+    // Only one stack present (partial rename) is still not trusted.
+    expect(yHistoryAvailability({ undoStack: [] })).toBeNull();
+  });
+
+  it("pin-test: a real yjs UndoManager still exposes undoStack/redoStack arrays", () => {
+    const doc = new Y.Doc();
+    const text = doc.getText("prosemirror");
+    const undoManager = new Y.UndoManager(text);
+
+    // Fresh manager: both stacks empty -> nothing to undo/redo.
+    expect(yHistoryAvailability(undoManager)).toEqual({
+      canUndo: false,
+      canRedo: false,
+    });
+
+    // A tracked edit must push onto the private undoStack. If a future yjs
+    // renames these fields, yHistoryAvailability(undoManager) returns null and
+    // the expectation below fails loudly.
+    text.insert(0, "hello");
+    undoManager.stopCapturing();
+    expect(yHistoryAvailability(undoManager)).toEqual({
+      canUndo: true,
+      canRedo: false,
+    });
+
+    // Undoing moves the item to the redoStack -> redo becomes available.
+    undoManager.undo();
+    expect(yHistoryAvailability(undoManager)).toEqual({
+      canUndo: false,
+      canRedo: true,
+    });
+  });
+});
@@ -35,6 +35,30 @@ export interface ToolbarState {
 // When neither history backend is installed (the pre-sync static editor —
 // mainExtensions only, undoRedo disabled), both fall through to 0 -> false,
 // matching the previous `safeCan` behavior.
+// Reads the Yjs UndoManager's undo/redo availability from its stack lengths.
+//
+// `undoStack` / `redoStack` are PRIVATE y-undo / yjs internals, so we touch them
+// defensively: a yjs or y-undo upgrade that renames or restructures these fields
+// must not silently mis-drive the toolbar buttons (nor throw on `.length` of
+// `undefined`). We only trust them when they are actually arrays; otherwise this
+// returns null and the caller falls back to a safe default. The pin-test in
+// use-toolbar-state.test.ts asserts the current library shape, so an upgrade that
+// breaks this contract fails loudly there instead of failing silently in the UI.
+export function yHistoryAvailability(
+  undoManager: unknown,
+): { canUndo: boolean; canRedo: boolean } | null {
+  if (!undoManager || typeof undoManager !== "object") return null;
+  const { undoStack, redoStack } = undoManager as {
+    undoStack?: unknown;
+    redoStack?: unknown;
+  };
+  if (!Array.isArray(undoStack) || !Array.isArray(redoStack)) return null;
+  return {
+    canUndo: undoStack.length > 0,
+    canRedo: redoStack.length > 0,
+  };
+}
+
 function historyAvailability(editor: Editor): {
  canUndo: boolean;
  canRedo: boolean;
@@ -43,16 +67,14 @@ function historyAvailability(editor: Editor): {

  // Collaboration history (Yjs) takes precedence when present.
  const yState = yUndoPluginKey.getState(state) as
-    | { undoManager?: { undoStack: unknown[]; redoStack: unknown[] } }
+    | { undoManager?: unknown }
    | undefined;
-  if (yState?.undoManager) {
-    return {
-      canUndo: yState.undoManager.undoStack.length > 0,
-      canRedo: yState.undoManager.redoStack.length > 0,
-    };
-  }
+  const yAvail = yHistoryAvailability(yState?.undoManager);
+  if (yAvail) return yAvail;

  // Plain prosemirror-history (returns 0 when the history plugin is absent).
+  // This is also the safe default when a Yjs UndoManager is present but its
+  // private stack shape is no longer recognized (yHistoryAvailability -> null).
  return {
    canUndo: undoDepth(state) > 0,
    canRedo: redoDepth(state) > 0,
@@ -9,7 +9,7 @@ import { Italic } from "@tiptap/extension-italic";
 import { Link } from "@tiptap/extension-link";
 import { gitmostInsertTranscriptIntoEditor } from "./gitmost-recording.ts";

-const ZWSP = ""; // U+200B, the helper's block-trigger neutralizer
+const ZWSP = ""; // U+200B — asserted ABSENT (the block-escape lives in the serializer now)

 /**
 * #377 — the web-side bridge must append the native host's transcript below the
@@ -18,8 +18,9 @@ const ZWSP = ""; // U+200B, the helper's block-trigger neutralizer
 * regression would be caught), asserting the resulting document rather than
 * mocking the editor: transcript present -> "Transcript" heading + one paragraph
 * per non-empty line; content is inserted as LITERAL TEXT (no HTML/markdown
- * parsing); col-0 markdown block triggers are neutralized so git-sync keeps them
- * paragraphs; absent/empty/non-string -> no-op.
+ * parsing); col-0 markdown block triggers are stored verbatim (the git-sync
+ * serializer block-escapes them, so no client-side ZWSP is needed);
+ * absent/empty/non-string -> no-op.
 */
 describe("gitmostInsertTranscriptIntoEditor", () => {
  const makeEditor = () =>
@@ -91,19 +92,22 @@ describe("gitmostInsertTranscriptIntoEditor", () => {
    editor.destroy();
  });

-  it("neutralizes col-0 markdown block triggers with a leading ZWSP (git-sync safety)", () => {
+  it("inserts col-0 markdown block triggers as verbatim paragraph text (no ZWSP workaround)", () => {
    const editor = makeEditor();
-    // Trigger lines (some with a leaked indent) + a normal prefixed line.
+    // Trigger lines (some with a leaked indent) + a normal prefixed line. The
+    // git-sync serializer now block-escapes a leading trigger itself, so the
+    // bridge inserts each line's TEXT byte-exact (only the leaked indent is
+    // trimmed) — no invisible ZWSP is prepended anymore.
    const inserted = gitmostInsertTranscriptIntoEditor(
      editor,
      [
        "- dash",
-        "  > quote", // leading indent must be trimmed then neutralized
+        "  > quote", // leading indent is trimmed, text otherwise verbatim
        "# hash",
        "1. one",
        "> [!info] note",
        "```js",
-        "---", // solid thematic break -> horizontalRule (text-losing) if unneutralized
+        "---",
        "***",
        "___",
        "You: normal line",
@@ -116,20 +120,23 @@ describe("gitmostInsertTranscriptIntoEditor", () => {
      .map((n: any) => n.content?.[0]?.text)
      .filter((t: any) => typeof t === "string") as string[];

-    // Every block-trigger line is prefixed with the invisible ZWSP (indent
-    // trimmed first); the normal `You:` line is left byte-exact.
+    // Each trigger line is stored as its own byte-exact text (indent trimmed);
+    // the git-sync round-trip keeps it a paragraph via the serializer's
+    // block-escape, so no ZWSP is needed here.
    expect(texts).toEqual([
-      ZWSP + "- dash",
-      ZWSP + "> quote",
-      ZWSP + "# hash",
-      ZWSP + "1. one",
-      ZWSP + "> [!info] note",
-      ZWSP + "```js",
-      ZWSP + "---",
-      ZWSP + "***",
-      ZWSP + "___",
+      "- dash",
+      "> quote",
+      "# hash",
+      "1. one",
+      "> [!info] note",
+      "```js",
+      "---",
+      "***",
+      "___",
      "You: normal line",
    ]);
+    // Guard: no invisible ZWSP leaked into any inserted line.
+    for (const t of texts) expect(t).not.toContain(ZWSP);

    editor.destroy();
  });
@@ -240,45 +240,22 @@ export async function gitmostUploadFileToEditor(
  }
 }

-// Zero-width space (U+200B). Prepended to a transcript line that begins with a
-// markdown BLOCK trigger: it is invisible in the rendered doc but shifts the
-// trigger off column 0, so the git-sync doc->markdown->doc round-trip keeps the
-// line a plain paragraph (see GITMOST_MD_BLOCK_TRIGGER_RE).
-const GITMOST_ZWSP = "";
-
-// A markdown BLOCK-level construct that, sitting at column 0 of a paragraph
-// line, the git-sync markdown serializer (packages/prosemirror-markdown
-// markdown-converter.ts, `case "paragraph"`) would re-parse into a NON-paragraph
-// block on the doc->markdown->doc cycle. That serializer emits paragraph text
-// verbatim with NO block-escape (the pre-existing root cause), so a leading
-// `#`/`-`/`*`/`+`/`>`, an ordered-list `N.`/`N)`, a code fence ```/~~~, a table
-// `|`, or a `> [!info]` callout opener would silently become a heading / list /
-// quote / code block / table / callout. The final alternative matches a WHOLE-
-// LINE thematic break — solid `---`/`***`/`___` or spaced `- - -`/`_ _ _` (3+ of
-// the same `-`/`*`/`_`) — which round-trips into a `horizontalRule`; because
-// that node carries NO text, an un-neutralized separator line would LOSE its
-// text entirely (worse than the list/quote case). This matches a TRIMMED line's
-// start; the transcript's own `You:` / `Speaker N:` prefix begins with a letter
-// and never matches, so prefixed lines are left byte-exact.
-const GITMOST_MD_BLOCK_TRIGGER_RE =
-  /^(?:#{1,6}(?:\s|$)|[-*+](?:\s|$)|>|\d+[.)](?:\s|$)|```|~~~|\||([-*_])(?:\s*\1){2,}\s*$)/;
-
 // Append a transcript block BELOW the recording's audio node in a live editor:
 // a "Transcript" heading followed by one paragraph per non-empty transcript
 // line. The transcript is plain text, `\n`-separated, each line already
 // formatted as `You: ...` / `Speaker N: ...` by the native host — line text is
 // inserted as a TEXT node (never HTML/markdown), so there is no injection or
 // mark-parsing surface. Each kept line is trimmed (drops an indent that would
-// both leak into the display and, at col 0, form a markdown block trigger) and,
-// if it still begins with a col-0 markdown block trigger, gets an invisible
-// zero-width space prepended so the git-sync round-trip cannot turn it into a
-// list/quote/heading/callout/code/table (defensive boundary against the
-// serializer's missing block-escape). This is best-effort and meant to run
-// AFTER the audio has already been inserted; the caller must guard against a
-// throw so a transcript failure never fails the (already successful) recording.
-// Returns true when a block was inserted, false when there was nothing to
-// insert (transcript undefined/empty/not-a-string). A non-string value is a
-// no-op, not an error.
+// leak into the display). A line that begins with a col-0 markdown block
+// trigger (`#`/`-`/`>`/`1.`/fence/`---`/…) needs no client-side workaround: the
+// git-sync serializer (packages/prosemirror-markdown, `case "paragraph"`) now
+// block-escapes such a leading trigger, so the doc->markdown->doc round-trip
+// keeps the line a paragraph on its own — the former invisible-ZWSP defense is
+// gone. This is best-effort and meant to run AFTER the audio has already been
+// inserted; the caller must guard against a throw so a transcript failure never
+// fails the (already successful) recording. Returns true when a block was
+// inserted, false when there was nothing to insert (transcript
+// undefined/empty/not-a-string). A non-string value is a no-op, not an error.
 export function gitmostInsertTranscriptIntoEditor(
  editor: Editor,
  transcript: unknown,
@@ -288,13 +265,7 @@ export function gitmostInsertTranscriptIntoEditor(
    .split("\n")
    // Trim each line and drop blank (whitespace-only) ones.
    .map((line) => line.trim())
-    .filter((line) => line.length > 0)
-    // Neutralize a col-0 markdown block trigger with an invisible ZWSP so the
-    // git-sync round-trip keeps the line a paragraph. Host lines (`You:` /
-    // `Speaker N:`) never match and stay byte-exact.
-    .map((line) =>
-      GITMOST_MD_BLOCK_TRIGGER_RE.test(line) ? GITMOST_ZWSP + line : line,
-    );
+    .filter((line) => line.length > 0);
  if (lines.length === 0) return false;

  const content = [
@@ -281,9 +281,11 @@ const SpaceTree = forwardRef<SpaceTreeApi, SpaceTreeProps>(function SpaceTree(
      setOpenTreeNodes((prev) => ({ ...prev, [id]: isOpen }));
      if (isOpen) {
        const node = treeModel.find(data, id) as SpaceTreeNode | null;
-        // Same "unloaded branch" predicate the insert paths use (`isUnloadedBranch`)
-        // so the lazy-load gate and the realtime/DnD inserts can never disagree
-        // about what counts as unloaded (#525).
+        // Same "unloaded branch" predicate the realtime insert paths use
+        // (`isUnloadedBranch`) so the lazy-load gate and the realtime inserts
+        // (`insertByPosition` / `placeByPosition`) can never disagree about what
+        // counts as unloaded (#525). Note: local raw `insert` (DnD/create-page)
+        // does not yet route through it — see #525 follow-up.
        if (treeModel.isUnloadedBranch(node)) {
          const fetched = await fetchAllAncestorChildren({
            pageId: id,
@@ -49,9 +49,11 @@ export const treeModel = {
  // resetting collapsed branches), NOT `children: undefined` — so a predicate that
  // only checks `=== undefined` misses the real case and materializes a misleading
  // partial list (#525). This is the SINGLE source of truth for "should a
-  // fetch/materialize be deferred?", shared by the lazy-load gate (`handleToggle`),
-  // the realtime insert path (`insertByPosition`) and the DnD move guard, so they
-  // can never drift apart again. A parent WITHOUT `hasChildren` is genuinely empty
+  // fetch/materialize be deferred?", shared by the lazy-load gate (`handleToggle`)
+  // and the realtime insert paths (`insertByPosition` / `placeByPosition`), so they
+  // can never drift apart again. (The local raw `insert` primitive and its DnD/
+  // create-page callers do NOT yet route through this predicate — see #525
+  // follow-up.) A parent WITHOUT `hasChildren` is genuinely empty
  // (no server children) — inserting its first child is correct, not deferred.
  isUnloadedBranch<T extends object>(
    node: TreeNode<T> | null | undefined,
@@ -13,8 +13,7 @@ let currentAlias: IShareAlias | null = null;
 let availabilityResult: {
  valid: boolean;
  available: boolean;
-  currentPageId: string | null;
-} = { valid: true, available: true, currentPageId: null };
+} = { valid: true, available: true };

 vi.mock("@/features/share/queries/share-query.ts", () => ({
  useShareAliasForPageQuery: () => ({ data: currentAlias }),
@@ -56,7 +55,7 @@ describe("ShareAliasSection — taken-name handling is never a dead end", () =>
  beforeEach(() => {
    setMutateAsync.mockReset();
    currentAlias = null;
-    availabilityResult = { valid: true, available: true, currentPageId: null };
+    availabilityResult = { valid: true, available: true };
  });

  it("shows a 'will move it here' HINT (not a terminal error) when the name belongs to another page, and keeps Save enabled", async () => {
@@ -65,7 +64,6 @@ describe("ShareAliasSection — taken-name handling is never a dead end", () =>
    availabilityResult = {
      valid: true,
      available: false,
-      currentPageId: "page-X",
    };

    renderSection("page-Y");
@@ -97,7 +95,6 @@ describe("ShareAliasSection — taken-name handling is never a dead end", () =>
    availabilityResult = {
      valid: true,
      available: false,
-      currentPageId: "page-X",
    };
    // The server rejects the un-confirmed save asking the client to confirm.
    setMutateAsync.mockRejectedValueOnce({
@@ -106,7 +103,6 @@ describe("ShareAliasSection — taken-name handling is never a dead end", () =>
        status: 409,
        data: {
          code: "ALIAS_REASSIGN_REQUIRED",
-          currentPageId: "page-X",
          currentPageTitle: "Alias Test Page X",
        },
      },
@@ -48,7 +48,6 @@ export default function ShareAliasSection({
  const [availability, setAvailability] = useState<{
    valid: boolean;
    available: boolean;
-    currentPageId: string | null;
  } | null>(null);
  const [reassign, setReassign] = useState<{
    alias: string;
@@ -76,7 +75,6 @@ export default function ShareAliasSection({
        setAvailability({
          valid: res.valid,
          available: res.available,
-          currentPageId: res.currentPageId,
        });
      } catch {
        setAvailability(null);
@@ -108,7 +108,6 @@ export interface IShareAliasAvailability {
  alias: string;
  valid: boolean;
  available: boolean;
-  currentPageId: string | null;
 }

 export interface ISharedPageTree {
@@ -28,6 +28,7 @@ import {
  IAiMcpServerCreate,
  IAiMcpServerUpdate,
 } from "@/features/workspace/services/ai-mcp-server-service.ts";
+import { resolveToolAllowlist } from "./ai-mcp-server-form.utils.ts";

 const formSchema = z.object({
  name: z.string().min(1),
@@ -121,13 +122,20 @@ export default function AiMcpServerForm({
  async function handleSubmit(values: FormValues) {
    const headers = resolveHeaders();

+    // An empty tag field means "no restriction" (sent as null) — since #476 the
+    // server persists a literal `[]` as deny-all (zero tools). But a server that
+    // was ALREADY deny-all loads into an empty field too; sending null there
+    // would silently widen it to allow-all on a routine edit, so preserve `[]`.
+    // See resolveToolAllowlist for the full rationale.
+    const toolAllowlist = resolveToolAllowlist(values.toolAllowlist, server);
+
    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 +148,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,
@@ -0,0 +1,29 @@
+import { describe, it, expect } from "vitest";
+import { resolveToolAllowlist } from "./ai-mcp-server-form.utils.ts";
+
+describe("resolveToolAllowlist", () => {
+  it("sends the typed tools when the field is non-empty", () => {
+    expect(resolveToolAllowlist(["a", "b"], { toolAllowlist: null })).toEqual([
+      "a",
+      "b",
+    ]);
+  });
+
+  it("creates as null (unrestricted) when empty and there is no server", () => {
+    expect(resolveToolAllowlist([], undefined)).toBeNull();
+  });
+
+  it("sends null for an empty field on a previously-unrestricted server", () => {
+    expect(resolveToolAllowlist([], { toolAllowlist: null })).toBeNull();
+  });
+
+  it("preserves deny-all: an empty field on a `[]` server stays `[]`, not null", () => {
+    // The core #476/#477 guard: editing a deny-all server (rename/toggle) with
+    // an empty tag field must NOT silently widen it to allow-all.
+    expect(resolveToolAllowlist([], { toolAllowlist: [] })).toEqual([]);
+  });
+
+  it("still sends explicit tools even if the server was deny-all", () => {
+    expect(resolveToolAllowlist(["x"], { toolAllowlist: [] })).toEqual(["x"]);
+  });
+});
@@ -0,0 +1,22 @@
+import { IAiMcpServer } from "@/features/workspace/services/ai-mcp-server-service.ts";
+
+// Resolve the tool allowlist value to persist from the form field.
+//
+// An empty tag field normally means "no restriction" and is sent as null so
+// the server drops the column (all tools allowed). But a server that was
+// ALREADY deny-all (a stored literal `[]`, meaning zero tools — creatable via
+// the API) loads into the form as an empty field too. Coercing that empty
+// field to null on submit would SILENTLY widen a deny-all server to allow-all
+// on any routine edit (rename, toggle) — the exact silent-widen class #476
+// closed on the read side. So when the edited server was deny-all, preserve
+// `[]` (deny-all); only a genuinely-unrestricted server (stored null/absent)
+// stays null.
+export function resolveToolAllowlist(
+  fieldValue: string[],
+  server?: Pick<IAiMcpServer, "toolAllowlist">,
+): string[] | null {
+  if (fieldValue.length > 0) return fieldValue;
+  const wasDenyAll =
+    Array.isArray(server?.toolAllowlist) && server.toolAllowlist.length === 0;
+  return wasDenyAll ? [] : null;
+}
@@ -6,6 +6,8 @@ import {
  nextReindexPollInterval,
  isReindexComplete,
  isReindexButtonLoading,
+  reindexRunKey,
+  isNewReindexRun,
 } from './ai-provider-settings';

 describe('resolveCardStatus', () => {
@@ -221,6 +223,128 @@ describe('isReindexComplete', () => {
  });
 });

+describe('reindexRunKey', () => {
+  it('is null when the status carries no run identity', () => {
+    expect(reindexRunKey(undefined)).toBeNull();
+    expect(
+      reindexRunKey({ reindexing: false, indexedPages: 5, totalPages: 5 }),
+    ).toBeNull();
+  });
+
+  it('is null for a legacy/degraded record with an empty runId', () => {
+    // The server sends runId='' for a record written before the field existed;
+    // the client must treat that as "no identity" (fall back to prior behaviour).
+    expect(
+      reindexRunKey({
+        reindexing: true,
+        indexedPages: 0,
+        totalPages: 10,
+        runId: '',
+        reindexStartedAt: 1000,
+      }),
+    ).toBeNull();
+  });
+
+  it('folds runId and startedAt into one stable key', () => {
+    expect(
+      reindexRunKey({
+        reindexing: true,
+        indexedPages: 0,
+        totalPages: 10,
+        runId: 'run-a',
+        reindexStartedAt: 1000,
+      }),
+    ).toBe('run-a:1000');
+  });
+
+  it('changes when the runId changes for the same startedAt', () => {
+    const a = reindexRunKey({
+      reindexing: true,
+      indexedPages: 0,
+      totalPages: 10,
+      runId: 'run-a',
+      reindexStartedAt: 1000,
+    });
+    const b = reindexRunKey({
+      reindexing: true,
+      indexedPages: 0,
+      totalPages: 10,
+      runId: 'run-b',
+      reindexStartedAt: 1000,
+    });
+    expect(a).not.toBe(b);
+  });
+
+  it('changes when the same runId restarts at a new startedAt', () => {
+    const a = reindexRunKey({
+      reindexing: true,
+      indexedPages: 0,
+      totalPages: 10,
+      runId: 'run-a',
+      reindexStartedAt: 1000,
+    });
+    const b = reindexRunKey({
+      reindexing: true,
+      indexedPages: 0,
+      totalPages: 10,
+      runId: 'run-a',
+      reindexStartedAt: 2000,
+    });
+    expect(a).not.toBe(b);
+  });
+});
+
+describe('isNewReindexRun (poll keying on runId)', () => {
+  // Derive the status shape from the helper itself so the test needs no export
+  // of the component-internal ReindexStatus type.
+  type ReindexStatusLike = NonNullable<Parameters<typeof reindexRunKey>[0]>;
+  const run = (runId: string, startedAt: number): ReindexStatusLike => ({
+    reindexing: true,
+    indexedPages: 0,
+    totalPages: 10,
+    runId,
+    reindexStartedAt: startedAt,
+  });
+
+  it('first identity after none latched is a NEW run', () => {
+    expect(isNewReindexRun(null, run('run-a', 1000))).toBe(true);
+  });
+
+  it('the SAME identity is not a new run (same run being watched)', () => {
+    const key = reindexRunKey(run('run-a', 1000));
+    expect(isNewReindexRun(key, run('run-a', 1000))).toBe(false);
+  });
+
+  it('a DIFFERENT runId is a new run (reset per-run poll state)', () => {
+    const key = reindexRunKey(run('run-a', 1000));
+    expect(isNewReindexRun(key, run('run-b', 1000))).toBe(true);
+  });
+
+  it('an identity-less poll (no runId / cleared record) is never a new run', () => {
+    const key = reindexRunKey(run('run-a', 1000));
+    expect(
+      isNewReindexRun(key, {
+        reindexing: false,
+        indexedPages: 10,
+        totalPages: 10,
+      }),
+    ).toBe(false);
+  });
+
+  it('a legacy empty-runId poll does not spuriously reset a latched run', () => {
+    const key = reindexRunKey(run('run-a', 1000));
+    expect(
+      isNewReindexRun(key, {
+        reindexing: true,
+        indexedPages: 3,
+        totalPages: 10,
+        runId: '',
+        reindexStartedAt: 1000,
+      }),
+    ).toBe(false);
+  });
+});
+
 describe('isReindexButtonLoading', () => {
  it('loads while the POST mutation is pending', () => {
    expect(
@@ -173,9 +173,43 @@ export function resolveKeyField(
 // Subset of the status payload that drives the reindex poll decisions.
 type ReindexStatus = Pick<
  IAiSettings,
-  "reindexing" | "indexedPages" | "totalPages"
+  "reindexing" | "indexedPages" | "totalPages" | "runId" | "reindexStartedAt"
 >;

+/**
+ * A stable per-RUN key for the reindex poll: `runId:startedAt`, or `null` when
+ * the status carries no run identity (no active run, or a legacy/degraded
+ * server record with an empty runId). Two polls of the SAME run share a key; a
+ * new run mints a fresh runId and so a different key.
+ *
+ * This is the single place the client turns the server's run identity into the
+ * value it keys on — it removes the "is this the same run I've been watching or
+ * a brand-new one?" ambiguity that made a class of reindex-status bugs (a stale
+ * pre-reindex snapshot vs a fresh run) get fixed twice (#262). `startedAt` is
+ * folded in so a run that somehow reuses a runId but restarted is still new.
+ */
+export function reindexRunKey(status: ReindexStatus | undefined): string | null {
+  const runId = status?.runId;
+  if (!runId) return null;
+  return `${runId}:${status?.reindexStartedAt ?? ""}`;
+}
+
+/**
+ * Decide whether the latest poll represents a NEW reindex run relative to the
+ * run key the client last latched (`prevKey`, `null` if none yet). True only
+ * when the status carries an identity AND it differs from the latched one — the
+ * signal to reset any per-run poll state (the "seen active" latch / progress the
+ * UI held). The same identity (or no identity) is NOT a new run, so an unchanged
+ * or identity-less poll never resets mid-run.
+ */
+export function isNewReindexRun(
+  prevKey: string | null,
+  status: ReindexStatus | undefined,
+): boolean {
+  const key = reindexRunKey(status);
+  return key !== null && key !== prevKey;
+}
+
 /**
 * Decide the TanStack Query `refetchInterval` while a reindex may be running.
 * Returns the poll interval (ms) to keep polling, or `false` to stop.
@@ -320,6 +354,13 @@ export default function AiProviderSettings() {
  // counter at 0 until a manual reload. A ref (not state) because it must not
  // trigger a render and is only ever read where `reindexing` is already false.
  const reindexSeenActiveRef = useRef(false);
+  // The run identity (runId:startedAt) the current poll window is keyed on. When
+  // a poll reports a DIFFERENT runId the server has started a NEW run, so we
+  // re-latch to it and reset `reindexSeenActiveRef` — a fresh run must never
+  // inherit the previous run's "seen active"/completion state (which would stop
+  // polling immediately or read the old run's counters as this run's). null =
+  // no run keyed yet (steady state, or a legacy record without a runId).
+  const reindexRunKeyRef = useRef<string | null>(null);

  // Only admins may read the (masked) AI settings; the server enforces this too.
  const { data: settings, isLoading } = useAiSettingsQuery(isAdmin, (query) =>
@@ -336,6 +377,14 @@ export default function AiProviderSettings() {
  // unmount because the deadline state goes away with the component.
  useEffect(() => {
    if (reindexDeadline === null) return;
+    // Key the poll on the run identity: if this poll carries a runId different
+    // from the one we latched, the server started a NEW run, so adopt it and
+    // drop the per-run "seen active" latch (a fresh run must not inherit the
+    // previous run's completion state). Same runId => same run, leave it alone.
+    if (isNewReindexRun(reindexRunKeyRef.current, settings)) {
+      reindexRunKeyRef.current = reindexRunKey(settings);
+      reindexSeenActiveRef.current = false;
+    }
    // Latch "we have seen the active run" the moment a poll reports it, so the
    // completion check below (and the refetchInterval's) only fires once the run
    // has genuinely started — never on the stale pre-reindex snapshot.
@@ -1220,6 +1269,10 @@ export default function AiProviderSettings() {
                  // immediately.
                  onSuccess: () => {
                    reindexSeenActiveRef.current = false;
+                    // Forget the previous run's identity so the first poll of
+                    // this window (carrying the new run's runId) is recognized
+                    // as a new run and keyed afresh.
+                    reindexRunKeyRef.current = null;
                    setReindexDeadline(Date.now() + REINDEX_POLL_CAP_MS);
                  },
                })
@@ -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;
@@ -51,6 +51,14 @@ export interface IAiSettings {
  // True while a full workspace reindex is actively running; the counts above
  // then reflect the live run progress (done climbs 0 -> total).
  reindexing?: boolean;
+  // Identity of the ACTIVE reindex run (present only while `reindexing`). The
+  // poll keys on `runId`: a changed value means a NEW run (reset the per-run
+  // poll state the UI latched), the same value is the run already being watched.
+  // Absent/empty ('') => no identity available; the client keeps prior behaviour.
+  runId?: string;
+  // Epoch-ms the active run started; paired with `runId` so a restart with a
+  // recycled id is still detected as a new run.
+  reindexStartedAt?: number;
 }

 // Update payload. Key semantics (same for `apiKey` and `embeddingApiKey`):