Skip to content

fix(harness): let a host tool replace an unallowed built-in tool of the same name - #615

Merged
andybons merged 3 commits into
mainfrom
fix/host-tool-builtin-name
Oct 14, 2026
Merged

andybons merged 3 commits into
mainfrom
fix/host-tool-builtin-name

Conversation

@andybons

@andybons andybons commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

Problem

A host tool named like a built-in tool, for example bash, fails New. An sdk/embed page tool or a Go Options.Tools tool with that name makes session/new fail with internal, also when the allowed list leaves the built-in out. The browser default set excludes bash, so a page that wants its own bash tool cannot have one.

Design

Spec: docs/architecture.md Tool names (Four rules, AllowedTools bullet), Runtime (New), Built-in tools, and the wasm Tools row. Design: docs/design/acp-wasm.md section 5.1, 5.6.

  • New no longer rejects an Options.Tools name from read_file, write_file, edit_file, glob, grep, ls, bash (Runtime.shadowable, the one owner of the rule).
  • The session source drops the built-in tool of that name, so the model sees one tool with the schema of the host.
  • checkModel fails Create with invalid_request (ErrInvalidRequest, ACP invalid params) and names the tool when AllowedTools is nil, because the built-in tool is then allowed too.
  • Plugin tools, MCP tools, read_tool_result, session_info, process, task, model keep their rules. The spec is silent on plugin and MCP tools, so only host tools change.

Semantic change

AllowedTools is a flat list of names, so a name in an explicit list cannot tell the built-in from the host tool. With a list, the name selects the host tool (the sdk/embed default list always holds the page tool names, so this is the only reading that works there). With nil, the collision is the clear error.

Verification

  • Rows: TestContractRuntimeEmbedderToolReplacesAnUnlistedBuiltinOfTheSameName, ...FailsCreateWhenEveryToolIsAllowed, TestNodeWasmPageToolReplacesABuiltinTool, TestNodeWasmPageToolWithABuiltinNameFailsWhenEveryToolIsAllowed (HARNESS_E2E_NODE=1), browser row page tool named bash replaces the built-in bash (HARNESS_E2E_BROWSER=1). Before the fix: tool name "bash" is empty, repeated, or reserved and internal (-32603).
  • go build ./... && go vet ./... && go test -race ./... && gofmt: pass (one unrelated CLI timing test flaked under load, passes alone with -count=3).

CI job times

Job main this PR
Static 40 s 39 s
Gates 35 s 30 s
Unit 56 s 62 s
WebAssembly 27 s 15 s
Contract 89 s 69 s
Runtime contract 61 s 49 s
Node wasm 65 s 67 s
Browser 120 s 79 s

No job grew by more than 6 s. The new rows run in parallel inside existing jobs and take under 2 s each (the browser row shares one Chromium run).

Copilot AI balanced review requested due to automatic review settings October 14, 2026 02:50

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Reopened unrestricted sessions bypass collision validation, and the binding specification contains contradictory name-validation language.

2 open findings
What changed in this PR

Enables host tools to replace same-named WorkDir built-ins when an explicit allowlist is used.

Changes:

  • Adds built-in shadowing and collision validation.
  • Updates architecture documentation and changelog.
  • Adds Go, Node/Wasm, and browser coverage.
File Description
tools.go Removes shadowed built-ins from session sources.
runtime.go Permits shadowable host tool names.
models.go Rejects unrestricted collisions.
e2e/​contract_runtime_toolcall_test.go Tests runtime behavior.
e2e/​browser_test.go Adds browser coverage.
e2e/​acp_node_test.go Adds Node/Wasm coverage.
sdk/​embed/​test/​browser/​page.mjs Defines the browser host tool.
cmd/​harness-wasm/​node/​host.mjs Defines the Node host tool.
docs/​architecture.md Specifies replacement semantics.
CHANGELOG.md Records the user-visible change.

🧠 Review effort: Balanced


💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread models.go Outdated
Comment thread docs/architecture.md Outdated
…he same name

A host tool named like a built-in tool, such as bash, failed New. A host
that cannot run bash (sdk/embed in a browser or Node, with page tools)
saw session/new fail with `internal`, even though its allowed list left
the built-in out of reach.

Design: New no longer rejects an Options.Tools name from read_file,
write_file, edit_file, glob, grep, ls, or bash. The session source drops
the built-in tool of that name, so the model sees one tool with the
schema of the host. Create fails with invalid_request, naming the tool,
when AllowedTools is nil, because the built-in tool is then allowed too.
read_tool_result, session_info, process, task, model, plugin tools, and
MCP tools keep their rules.

Semantic change: a list in AllowedTools that names bash selects the host
tool; a nil list with a host tool named bash fails Create.

Verification: runtime rows for both cases; Node wasm rows through
HARNESS_E2E_NODE=1; a browser row through HARNESS_E2E_BROWSER=1; the
handoff check.
@andybons
andybons force-pushed the fix/host-tool-builtin-name branch from f6971b6 to 42f46ea Compare October 14, 2026 03:05
@andybons
andybons merged commit 9f400ab into main Oct 14, 2026
8 checks passed
@andybons
andybons deleted the fix/host-tool-builtin-name branch October 14, 2026 03:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants