fix(serve): post-merge P2 corrections from Codex review on #4282

packages/acp-bridge/src/bridgeErrors.ts

@@ -193,6 +193,78 @@ export class WorkspaceInitConflictError extends Error {
   }
 }
 
+/**
+ * #4297 fold-in 1 (16:32:44-round S1). Thrown by `initWorkspace` when
+ * the configured `context.fileName` resolves outside the bound
+ * workspace via path arithmetic (e.g. `../outside.md`). Translated
+ * to HTTP 400 by the route — distinguishable from a generic 500 so
+ * an operator sees "your workspace config is wrong" rather than
+ * "the daemon is broken." The `filename` and `boundWorkspace`
+ * fields let clients display a precise diagnostic.
+ */
+export class WorkspaceInitPathEscapeError extends Error {
+  readonly filename: string;
+  readonly boundWorkspace: string;
+  constructor(filename: string, boundWorkspace: string) {
+    super(
+      `Configured workspace context filename ${JSON.stringify(filename)} ` +
+        `resolves outside the bound workspace ${JSON.stringify(boundWorkspace)}. ` +
+        `Refusing to write.`,
+    );
+    this.name = 'WorkspaceInitPathEscapeError';
+    this.filename = filename;
+    this.boundWorkspace = boundWorkspace;
+  }
+}
+
+/**
+ * #4297 fold-in 1 (16:32:44-round S1). Thrown by `initWorkspace` when
+ * the target file is itself a symlink, OR when the parent path
+ * canonicalizes (via `realpath`) outside the bound workspace.
+ * Translated to HTTP 400 by the route — same operator-clarity
+ * rationale as `WorkspaceInitPathEscapeError`. `target` is the
+ * resolved path the bridge attempted, `kind` distinguishes the two
+ * symlink scenarios for diagnostics.
+ */
+export class WorkspaceInitSymlinkError extends Error {
+  readonly target: string;
+  readonly kind: 'target' | 'parent';
+  constructor(target: string, kind: 'target' | 'parent', detail: string) {
+    super(detail);
+    this.name = 'WorkspaceInitSymlinkError';
+    this.target = target;
+    this.kind = kind;
+  }
+}
+
+/**
+ * #4297 fold-in 10 (qwen-latest, addresses #3263954690). Thrown by
+ * `initWorkspace` when the target file's inode misbehaved at write
+ * time IN A NON-SYMLINK WAY — typically a TOCTOU race against a
+ * concurrent writer:
+ *   - `'eexist'`: a regular file (or symlink) appeared at the target
+ *     path between the absence check and our atomic `'wx'` create.
+ *   - `'enoent'`: the target was deleted between the content check
+ *     and the `O_NOFOLLOW` overwrite (concurrent git checkout, editor
+ *     save, etc.).
+ *
+ * Split out from `WorkspaceInitSymlinkError` so the HTTP error code
+ * isn't misleading: an operator chasing a `workspace_init_race`
+ * code knows it's a benign concurrent-modification window, not a
+ * symlink attack vector. Same 400 mapping as the sibling class —
+ * the route layer still recognizes both.
+ */
+export class WorkspaceInitRaceError extends Error {
+  readonly target: string;
+  readonly kind: 'eexist' | 'enoent';
+  constructor(target: string, kind: 'eexist' | 'enoent', detail: string) {
+    super(detail);
+    this.name = 'WorkspaceInitRaceError';
+    this.target = target;
+    this.kind = kind;
+  }
+}
+
 /**
  * #4282 fold-in 1 (gpt-5.5 C5). Thrown by `restartMcpServer` when the
  * caller asks for a server name that isn't in the daemon's

packages/acp-bridge/src/bridgeOptions.ts

@@ -216,6 +216,18 @@ export interface BridgeOptions {
     toolName: string,
     enabled: boolean,
   ) => Promise<void>;
+  /**
+   * #4282 fold-in 5 (Codex P2-1). Optional override for the basename
+   * (or single relative path) of the workspace context file written
+   * by `POST /workspace/init`. When omitted, falls back to
+   * `getCurrentGeminiMdFilename()` — the process-global value, which
+   * the daemon parent never updates because it doesn't go through
+   * `loadCliConfig`. Production callers (`runQwenServe`) snapshot the
+   * resolved filename from the workspace's merged settings at boot
+   * and pass it here so init writes the same file the ACP child
+   * reads. Bridge tests can pass any literal.
+   */
+  contextFilename?: string;
   /**
    * #4175 Wave 5 PR 22b/2 — optional injection seam for daemon-host
    * status cells (env snapshot, daemon preflight). Production

packages/cli/src/acp-integration/acpAgent.ts

@@ -1515,8 +1515,94 @@ class QwenAgent implements Agent {
             reason: 'budget_would_exceed' as const,
           };
         }
+        // #4282 fold-in 5 (Codex P2-2). Re-read settings to pick up
+        // any `tools.disabled` toggles applied since this ACP child
+        // booted. The original snapshot was frozen by Config's
+        // constructor; without this refresh, a `setWorkspaceToolEnabled`
+        // call followed by the documented `mcp restart` would
+        // re-register a just-disabled MCP tool because
+        // `discoverMcpToolsForServer` walks `ToolRegistry.registerTool`,
+        // which consults `Config.getDisabledTools()`.
+        //
+        // #4297 fold-in 3 (wenshao critical, addresses #3260725526):
+        // read the MERGED settings (User + System + Workspace union)
+        // rather than the workspace scope alone. The bootstrap Config
+        // received `merged.tools?.disabled`, so user/system policy is
+        // already enforced by `ToolRegistry.registerTool`. Replacing
+        // the in-memory set with the workspace scope alone would
+        // silently drop higher-scope entries — a user-level disable
+        // would survive boot but vanish after the first MCP restart,
+        // letting `discoverMcpToolsForServer` re-register the
+        // user-disabled tool.
+        //
+        // The asymmetry vs. the persist-write path is deliberate:
+        // `runQwenServe.persistDisabledTools` writes to
+        // `SettingScope.Workspace` only so the workspace file doesn't
+        // bake in user/system entries (the fold-in 1 H2 fix). Reads
+        // need the union; writes need the scope. The two paths look
+        // alike but answer different questions.
+        try {
+          const fresh = loadSettings(this.config.getTargetDir());
+          const mergedDisabled = fresh.merged.tools?.disabled;
+          // #4297 fold-in 7 (qwen-latest critical, addresses
+          // #3262625101): a malformed `tools.disabled` (boolean,
+          // string, object — hand-edited settings.json) DOESN'T
+          // throw, so the catch block below would never fire. The
+          // ternary used to silently substitute `[]`, clearing the
+          // entire disabled set with zero operator signal. Detect
+          // and stderr-log the malformed shape before clearing so a
+          // misconfigured settings file is loud rather than silent.
+          if (mergedDisabled !== undefined && !Array.isArray(mergedDisabled)) {
+            process.stderr.write(
+              `qwen serve: MCP restart for ${JSON.stringify(serverName)}: ` +
+                `tools.disabled has unexpected type ${typeof mergedDisabled}; ` +
+                `clearing disabled set — check settings.json. ` +
+                `Expected an array of strings.\n`,
+            );
+          }
+          const disabledList = Array.isArray(mergedDisabled)
+            ? mergedDisabled.filter((v): v is string => typeof v === 'string')
+            : [];
+          this.config.setDisabledTools(new Set(disabledList));
+        } catch (err) {
+          // #4297 fold-in 2 (wenshao S3): settings load failures are
+          // non-fatal — fall through with the existing in-memory
+          // snapshot. The MCP restart still runs; only the
+          // disabledTools sync is skipped. Surface a stderr line so a
+          // persistent failure (corrupted settings.json, permission
+          // denied) is visible to operators rather than silently
+          // breaking the documented "toggle + restart" workflow with
+          // zero diagnostic.
+          process.stderr.write(
+            `qwen serve: MCP restart for ${JSON.stringify(serverName)} ` +
+              `could not refresh disabledTools from merged settings ` +
+              `(${err instanceof Error ? err.message : String(err)}); ` +
+              `proceeding with the bootstrap snapshot — recently toggled ` +
+              `tools may not take effect until daemon restart.\n`,
+          );
+        }
+        // #4297 fold-in 9 (gpt-5.5 critical, addresses #3263088414):
+        // route through `ToolRegistry.discoverToolsForServer` instead
+        // of calling `manager.discoverMcpToolsForServer` directly.
+        // The registry wrapper PURGES this server's existing
+        // `DiscoveredMCPTool` entries (and its `revealedDeferred`
+        // markers) plus its prompts before rediscovery, so a
+        // toggle-disable-then-restart workflow actually removes the
+        // newly-disabled tool from the live registry. Without the
+        // wrapper, `registerTool` would only consult the refreshed
+        // `disabledTools` set for newly-discovered tools — entries
+        // that were already in the registry from the prior boot
+        // would keep serving requests, silently breaking the
+        // documented "toggle + restart" promise.
         const start = Date.now();
-        await manager.discoverMcpToolsForServer(serverName, this.config);
+        const toolRegistry = this.config.getToolRegistry();
+        if (!toolRegistry) {
+          throw RequestError.internalError(
+            undefined,
+            'ToolRegistry unavailable on this Config',
+          );
+        }
+        await toolRegistry.discoverToolsForServer(serverName);
         // #4282 gpt-5.5 C4 fold-in: `discoverMcpToolsForServer`
         // catches reconnect/discovery errors internally (logs and
         // resolves void) so a broken MCP server would otherwise