fix(dsh-plugin): register browser tools one by one so a name collision cannot take the whole suite down (#444)

CHANGELOG.md

@@ -9,6 +9,14 @@ Starting from 0.2.0, CLI / Extension / DSH Plugin share the same version number.
 
 ### Fixed
 
+- DSH Plugin: a `browser_*` name that another tool already owns in the shared tool
+  scope no longer takes the whole suite down with it. `registerBrowserTools`
+  registers the six specs one by one, so a collision on `browser_session` (spec 0)
+  no longer aborts before the other five are even attempted, the disposers of the
+  tools that did register are kept instead of being dropped with the abort, and
+  each conflict is reported by tool name rather than only through a `console.warn`
+  that an Electron desktop host never shows
+  ([#444](https://github.com/Tencent/BrowserSkill/issues/444)).
 - Extension: input to a background Agent Window tab no longer keeps failing with
   `input_not_ready` after Chrome drops the session's focus override without a detach
   ([#355](https://github.com/Tencent/BrowserSkill/issues/355)). The session's

packages/dsh-plugin-browserskill/src/browser-tools.ts

@@ -391,12 +391,42 @@ function indexOperations(definitions: ToolDefinition[]): ReadonlyMap<string, Too
   return indexed;
 }
 
-/** Register the complete six-tool browser suite; returns its combined disposer. */
+/**
+ * Register the complete six-tool browser suite; returns its combined disposer.
+ *
+ * The specs are registered one by one on purpose. The host tool registry
+ * rejects a duplicate name by throwing, and a plain `.map()` over the specs
+ * aborts at the first rejection: the disposers of the tools that DID register
+ * are lost with the pending entries, and a collision on `browser_session`
+ * (the first spec) leaves the plugin with zero usable tools. The only trace
+ * was a `console.warn` inside `armLazyTools`, which an Electron desktop host
+ * never shows — the plugin looked healthy while exposing nothing (#444).
+ *
+ * Registering per spec keeps every non-colliding tool available, keeps the
+ * disposers of those tools, and reports each conflict with the offending tool
+ * name. When nothing registered we still throw, so `armLazyTools` keeps its
+ * existing "retry at the next lifecycle boundary" behaviour.
+ */
 export function registerBrowserTools(deps: ToolDeps): () => void {
   const definitions = indexOperations(createBrowserOperationDefinitions(deps));
-  const disposers = BROWSER_TOOL_SPECS.map((spec) =>
-    deps.ctx.tools.register(defineBrowserTool(spec, definitions)),
-  ).filter((dispose): dispose is () => void => typeof dispose === "function");
+  const disposers: (() => void)[] = [];
+  const failures: string[] = [];
+  for (const spec of BROWSER_TOOL_SPECS) {
+    try {
+      const dispose = deps.ctx.tools.register(defineBrowserTool(spec, definitions));
+      if (typeof dispose === "function") disposers.push(dispose);
+    } catch (error) {
+      const detail = error instanceof Error ? error.message : String(error);
+      failures.push(`${spec.name} (${detail})`);
+    }
+  }
+  if (failures.length > 0) {
+    const summary =
+      `[dsh-plugin-browserskill] ${failures.length} of ${BROWSER_TOOL_SPECS.length} browser tools ` +
+      `failed to register — another tool already owns the name in this scope: ${failures.join(", ")}`;
+    if (disposers.length === 0) throw new Error(summary);
+    console.warn(`${summary}; the remaining ${disposers.length} tool(s) are active`);
+  }
   return () => {
     for (const dispose of disposers.splice(0)) dispose();
   };

packages/dsh-plugin-browserskill/tests/tools.test.ts

@@ -15,7 +15,7 @@ import {
   createBskRunner,
 } from "../src/runner";
 import { SessionRegistry } from "../src/sessions";
-import type { PluginConfig } from "../src/tools";
+import type { PluginConfig, ToolDeps } from "../src/tools";
 
 const CONFIG: PluginConfig = {
   bskPath: "bsk",
@@ -2032,3 +2032,117 @@ describe("website debug", () => {
     expect(calls).toHaveLength(0);
   });
 });
+
+describe("registerBrowserTools: a name already owned in the shared tool scope", () => {
+  const ALL_TOOLS = [
+    "browser_session",
+    "browser_page",
+    "browser_inspect",
+    "browser_interact",
+    "browser_tabs",
+    "browser_assist",
+  ];
+
+  function depsWithRegister(register: (def: ToolDefinition) => unknown): ToolDeps {
+    const registry = new SessionRegistry(CONFIG.maxSessions);
+    const { runner } = fakeRunner({});
+    const ctx = { tools: { register }, get: () => undefined };
+    return {
+      ctx: ctx as never,
+      runner: runner as BskRunner,
+      registry,
+      config: { ...CONFIG },
+      observation: disabledObservation({ ctx, runner: runner as BskRunner, registry }),
+      queue: new KeyedExecutor(),
+    };
+  }
+
+  /** Collect console.warn output without leaking a spy into the other suites. */
+  function captureWarnings<T>(run: () => T): { result: T; warnings: string[] } {
+    const warnings: string[] = [];
+    const original = console.warn;
+    console.warn = (...args: unknown[]) => {
+      warnings.push(args.map((arg) => String(arg)).join(" "));
+    };
+    try {
+      return { result: run(), warnings };
+    } finally {
+      console.warn = original;
+    }
+  }
+
+  it("registers every tool whose name is still free", () => {
+    // `browser_session` is spec 0, so the old `.map()` aborted before any of
+    // the other five were even attempted (#444).
+    const owned = new Set(["browser_session"]);
+    const registered: string[] = [];
+    const { warnings } = captureWarnings(() =>
+      registerBrowserTools(
+        depsWithRegister((def) => {
+          if (owned.has(def.name)) throw new Error(`"${def.name}" is already registered`);
+          registered.push(def.name);
+          return () => {};
+        }),
+      ),
+    );
+    expect(registered).toEqual(ALL_TOOLS.filter((name) => name !== "browser_session"));
+    expect(warnings).toHaveLength(1);
+    expect(warnings[0]).toContain("browser_session");
+    expect(warnings[0]).toContain("1 of 6");
+  });
+
+  it("reports each conflict by tool name instead of failing silently", () => {
+    const owned = new Set(["browser_tabs", "browser_assist"]);
+    const registered: string[] = [];
+    const { warnings } = captureWarnings(() =>
+      registerBrowserTools(
+        depsWithRegister((def) => {
+          if (owned.has(def.name)) throw new Error("duplicate");
+          registered.push(def.name);
+          return () => {};
+        }),
+      ),
+    );
+    expect(registered).toEqual(ALL_TOOLS.filter((name) => !owned.has(name)));
+    expect(warnings).toHaveLength(1);
+    expect(warnings[0]).toContain("browser_tabs");
+    expect(warnings[0]).toContain("browser_assist");
+    expect(warnings[0]).toContain("2 of 6");
+  });
+
+  it("still disposes the tools that did register", () => {
+    // With the old all-or-nothing map the disposers of the first four specs
+    // were dropped with the abort, so an unload could not release them.
+    const owned = new Set(["browser_tabs"]);
+    const disposed: string[] = [];
+    const { result: dispose } = captureWarnings(() =>
+      registerBrowserTools(
+        depsWithRegister((def) => {
+          if (owned.has(def.name)) throw new Error("duplicate");
+          return () => {
+            disposed.push(def.name);
+          };
+        }),
+      ),
+    );
+    dispose();
+    expect(disposed).toEqual(ALL_TOOLS.filter((name) => !owned.has(name)));
+  });
+
+  it("throws with every name when nothing could register", () => {
+    const registered: string[] = [];
+    const { warnings } = captureWarnings(() => {
+      expect(() =>
+        registerBrowserTools(
+          depsWithRegister((def) => {
+            registered.push(def.name);
+            throw new Error("duplicate");
+          }),
+        ),
+      ).toThrow(/browser_session.*browser_assist/s);
+    });
+    expect(registered).toEqual(ALL_TOOLS);
+    // Nothing survived, so the message is the only diagnostic — no warn.
+    expect(warnings).toHaveLength(0);
+  });
+});