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); + }); +});