From b7cb20ce0e0ddc4028fdfea6b4c7aec082595dfa Mon Sep 17 00:00:00 2001 From: Jay0130-a <3421906768@qq.com> Date: Wed, 14 Oct 2026 15:50:39 +0800 Subject: [PATCH] fix(dsh-plugin): register the browser tools one by one so one name collision cannot take the whole suite down `registerBrowserTools` mapped over BROWSER_TOOL_SPECS and called ctx.tools.register for each entry. The host tool registry rejects a duplicate name by throwing, so the `.map()` aborted at the first rejection: on a profile that also mounts `dsh-builtin-browser` -- which already owns `browser_session`, spec 0 -- zero of the six tools registered, while the plugin still looked healthy. The only trace was a console.warn inside armLazyTools, which an Electron desktop host never shows (#444). A collision on a later spec was quieter still: the tools before it DID register, but their disposers were discarded along with the pending entries, so unloading the plugin could not release them. Register each spec on its own, keep the disposers of the ones that succeeded, and report every conflict by tool name. When nothing registered we still throw, preserving armLazyTools' retry-at-the-next-lifecycle-boundary path. --- CHANGELOG.md | 8 ++ .../src/browser-tools.ts | 38 +++++- .../tests/tools.test.ts | 116 +++++++++++++++++- 3 files changed, 157 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 73ee0ce0..137b535d 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/packages/dsh-plugin-browserskill/src/browser-tools.ts b/packages/dsh-plugin-browserskill/src/browser-tools.ts index d6e644cc..4f90f13d 100644 --- a/packages/dsh-plugin-browserskill/src/browser-tools.ts +++ b/packages/dsh-plugin-browserskill/src/browser-tools.ts @@ -391,12 +391,42 @@ function indexOperations(definitions: ToolDefinition[]): ReadonlyMap 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(); }; diff --git a/packages/dsh-plugin-browserskill/tests/tools.test.ts b/packages/dsh-plugin-browserskill/tests/tools.test.ts index 7d7a1d3d..e3cd8d98 100644 --- a/packages/dsh-plugin-browserskill/tests/tools.test.ts +++ b/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(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); + }); +});