fix(tools): restore external tool calling across OpenCode, OpenClaw, and Codex

[samson910022]

Restores reliable external tool calling across OpenCode, OpenClaw/Codex-style Responses clients, and Anthropic clients.\n\nPhase 1A: explicit external-tool policy/choice semantics, OMIT preserves tool schema, Anthropic metadata, empty tool results.\nPhase 1B: stream-safe tool validation, cross-channel parsing, merged tool artifacts, collector finish/error handling.\nPhase 2A: Responses SSE output assembler, argument accumulation, response identity, hosted-search ordering.\nPhase 2B: Codex additional_tools/namespace/custom declarations, continuation metadata, explicit unsupported-tool errors.\nPhase 3A: unified env/file/default config merge and deployment sources.\nPhase 3B: native tool discovery fail-closed plus readiness/metrics.\nPhase 4: Anthropic tool-loop and Interactions contract regression coverage.\nFinal: unified non-stream errors and parallel semantics.\n\nVerification: npm run typecheck, npm run build, npm test -- --runInBand, randomized Jest, and phased multi-subagent reviews.

[github-actions]

Verdict matrix

• Roles: gateway_safety, api_compat, docs_config
• FINAL_VERDICT: COMMENT

Sensitive paths

• .env.example
• Dockerfile
• docker-compose.yml
• entrypoint.sh
• index.ts
• src/config/proxy-config.ts
• src/errors/upstream.ts
• src/proxy.ts
• src/routes/chat.ts
• src/routes/interactions.ts
• src/routes/messages.ts
• src/routes/responses.ts
• src/routes/system.ts
• src/stream/collector.ts
• src/tool-runtime/parser.ts
• src/tool-runtime/policy.ts
• src/tool-runtime/registry.ts
• src/tool-runtime/router.ts

Deterministic pre-checks

• possible-password: .env.example,AGENTS.md,CHANGELOG.md:953
• possible-password: .env.example,AGENTS.md,CHANGELOG.md:1488

gateway_safety

VERDICT: APPROVE

Summary Rationale

The PR rigorously hardens the configuration matrix, external tool calling contracts, error categorization, and container port validation across OpenCode2API without violating any [provider] safety invariants.

• Checklist 1 (Secrets): No credentials or sensitive data are committed or exposed; logs report only boolean configuration presence (Configured / Not configured), and dummy proxy URIs in .env.example are synthetic documentation examples.
• Checklist 2 (Auth discipline): Authentication gating logic (API_KEY/OPENCODE_API_KEYS) remains intact with canonical env precedence; /health remains unauthenticated and /health/details defaults to requiring auth (HEALTH_DETAILS_REQUIRE_AUTH=true).
• Checklist 3 (Tool safety): Default-deny switch DISABLE_TOOLS=true is preserved and unified as the default fallback in src/config/proxy-config.ts:76; reserved __proxy_* metadata injection is prevented in Anthropic converters; tool execution output is omitted from logs.
• Checklist 4 (Bind/port matrix): BIND_HOST, OPENCODE_PROXY_PORT, OPENCODE_SERVER_PORT, and OPENCODE_SERVER_URL maintain consistent strict parsing (1..65535, http/https) across index.ts, src/config/proxy-config.ts, entrypoint.sh, Dockerfile, compose, and documentation.
• Checklist 5 & 6 (Lifecycle, Errors & Retry): transformUpstreamError maintains proper contract isolation (reserving tool_discovery_unavailable only for proxy-authored discovery faults and passing unmarked upstream 503s as server_error); retries continue to use safe bounds and jitter backoff.
• Checklist 7 & 8 (Hygiene & Invariants): Logging is restricted to operational counts/limits (limit, dropped, kept); index.ts is not imported by tests.

Findings

• BLOCKING: None.
• SHOULD_FIX: None.
• NITS: None.

api_compat

API Compatibility Review

This is a very large PR touching the config merge layer, tool policy, stream handling, error surfaces, and multiple route files. I'll focus on the wire-shape compatibility checklist.

---

VERDICT: NEEDS_CHANGES

---

BLOCKING

1. src/config/proxy-config.ts — resolveDisableTools default changed from false to true
src/config/proxy-config.ts:76: The library-facing resolveDisableTools default flipped from false to true. This is a breaking change for library consumers (anyone calling buildProxyConfig() without explicitly setting DISABLE_TOOLS). The CHANGELOG mentions it, but the AGENTS.md §4 table still says the library default is true without a migration note in the function's JSDoc. More critically, any downstream test or integration that relied on buildProxyConfig({}) having tools enabled will silently break.

   Risk: Not a wire-shape break per se, but a silent behavioral regression for library consumers. The CHANGELOG entry is the only notice. Should be gated behind a major version bump or at minimum a prominent deprecation warning at startup.

2. src/converters/anthropic.ts:336-339 — anthropicToolChoiceToChat drops bare {type:'tool'} without name
   Previously {type:'tool'} without name returned 'required'; now it returns undefined (falls through). Combined with the new validateAnthropicToolChoice at the route level (which rejects bare {type:'tool'} with 400), this is intentional — but the converter is also called directly from non-route code paths (e.g., translator tests, internal conversion). A direct call with {type:'tool'} now silently degrades to undefined (auto) instead of 'required', which changes tool-forcing semantics for any caller that bypasses the route validator.

   src/converters/anthropic.ts:339: The comment in CHANGELOG says "direct converter calls degrade to auto" — confirm this is intentional and locked by test. The tests/messages-anthropic.test.js diff is not shown, so I cannot verify coverage.

   Severity: blocking if there are internal callers that pass {type:'tool'} without name expecting 'required' behavior. The PR body says it's locked by test, but the test diff is truncated.

---

SHOULD_FIX

3. src/errors/upstream.ts:420-426 — upstream 503 now returns 503 instead of 502
   This is a wire-shape change: clients that previously saw 502 for upstream 503 errors will now see 503. The tool_discovery_unavailable marker is correctly gated by isLocalToolDiscoveryError, but the generic 503 branch (type: 'server_error', code: 'server_error') is new. Clients relying on 502 as the "upstream broke" signal may need updating.

   The CHANGELOG documents this ("upstream 503 now passes through as 503"), but it's a wire-shape change for all four routes. Verify that retry logic (isTransientUpstreamError) still treats 503 as retryable — if it only checked for 502, retries would stop.

4. src/proxy.ts — buildSystemPrompt signature changed: new externalToolPrompt parameter inserted at position 2
   src/proxy.ts:360: The second parameter is now externalToolPrompt (was reasoningEffort). All call sites must be updated. The diff is truncated so I cannot verify all call sites were updated. If any call site still passes reasoningEffort as the second argument, the reasoning effort string would be injected as a tool prompt.

5. src/stream/collector.ts — ID-less tool tracking refactored to per-tool-name Set
   The diff is truncated, but the CHANGELOG describes significant changes to the collector's tool-tracking logic. The finish==='tool' / idle-exemption logic is explicitly mentioned as changed (counter → Set, replay detection, settled set). This needs careful verification that:

   • flushHeaders timing is unchanged
   • finish === 'tool' still correctly exempts from idle timeout
   • The settled-set replay detection doesn't accidentally drop legitimate re-emitted tool parts

6. docker-compose.yml / Dockerfile — all defaults now empty
   While the rationale is sound (code owns defaults, empty env doesn't shadow config.json), this is a deployment-breaking change for users who relied on the image's non-empty defaults without setting env vars or mounting config.json. For example, BIND_HOST was 0.0.0.0 in the Dockerfile; now it's empty, so the code default applies — which is also 0.0.0.0, so it's fine. But OPENCODE_PATH was [provider]; now empty → code default [provider]. Verify every single key's code default matches the old Dockerfile default.

   Specifically check: OPENCODE_EXTERNAL_TOOLS_MODE was proxy-bridge in Dockerfile, now empty → code default proxy-bridge ✓. DISABLE_TOOLS was true, now empty → code default true ✓. OPENCODE_PROXY_MANAGE_BACKEND was false, now empty → code default for prod path is false ✓. Looks correct but should be verified exhaustively.

7. entrypoint.sh — PROXY_PORT unset path
   entrypoint.sh:47-50: When OPENCODE_PROXY_PORT is invalid and PORT is also invalid, PROXY_PORT is set to 10000 for logging only but OPENCODE_PROXY_PORT is unset. The CMD still uses --port 10001 for the backend. The proxy port is then resolved by index.ts from config.json or default. This is correct but subtle — document that the entrypoint intentionally does NOT export a default proxy port.

---

NITS

8. AGENTS.md line counts — Updated line counts (e.g., parser (1934), responses.ts (1798)) will drift immediately on the next change. Consider removing exact counts or automating them.

9. docs/api-reference.md — The new previous_response_id documentation mentions "最近 128 条工具调用元数据" but the constant RESPONSE_TOOL_CALL_STATE_LIMIT = 128 is only in src/proxy.ts. Consider making it a named export or documenting it in the config table.

10. index.ts — verbose enum warning pattern — The warnUnsupportedEnum / warnInvalidPort / warnInvalidUrl calls are repeated 3× per enum config key (canonical, legacy, file). Consider a helper that takes the layer array and labels together.

11. src/config/proxy-config.ts:166 — resolveRetryCountSetting uses MIN_SAFE_INTEGER..MAX_SAFE_INTEGER — This means it accepts negative retry counts (e.g., -3). The downstream resolveMaxRetries clamps to 0–5, but the intermediate config value could be confusing in logs. Consider clamping to 0..5 here or at least 0..MAX.

---

Test gaps

• Truncated test diffs: tests/stream-hardening.test.js, tests/messages-anthropic.test.js, tests/app.test.js, tests/tool-alias.test.js diffs are not visible. The PR claims coverage but I cannot verify.
• Cross-protocol regression: The anthropicToolChoiceToChat behavioral change (bare {type:'tool'} → undefined) needs an explicit test showing the old 'required' return is gone and the new undefined is correct.
• 503 passthrough: Need a test confirming transformUpstreamError returns 503 for a non-discovery upstream 503, and that isTransientUpstreamError still retries it.
• Empty Docker defaults: No integration test verifies that a fresh container with zero env vars and no config.json boots with the documented defaults.

docs_config

VERDICT: COMMENT

BLOCKING:

• None confirmed from docs/config-matrix view. Deterministic possible-password prefindings at .env.example,AGENTS.md,CHANGELOG.md:953,1488 are [REDACTED] in provided diff — cannot verify leak vs API_KEY/PASSWORD name-only placeholder. No secret values visible in readable hunks (Dockerfile, docker-compose.yml, .env.example, config.json.example all empty/placeholder).

SHOULD_FIX:

• Verify .env.example covers OPENCODE_PROXY_BIND_HOST alias: Dockerfile:ENV OPENCODE_PROXY_BIND_HOST=, docker-compose.yml:OPENCODE_PROXY_BIND_HOST=${OPENCODE_PROXY_BIND_HOST:-}, index.ts:resolveStringSetting([BIND_HOST,OPENCODE_PROXY_BIND_HOST,file]), docs/configuration.md:BIND_HOST优先/OPENCODE_PROXY_BIND_HOST后备, AGENTS.md §4 table all list it, but .env.example hunk in diff only shows # BIND_HOST=0.0.0.0 context with no OPENCODE_PROXY_BIND_HOST line. If missing, add # OPENCODE_PROXY_BIND_HOST= to close six-way.
• index.ts still imports normalizeBool,resolveDisableTools from ./src/proxy.js while AGENTS.md §4 declares single merge contract in src/config/proxy-config.ts:90-187 (resolve*Setting, parseStrictInteger/Port, parseHttpUrl, parseToolNameList). If src/proxy.ts re-exports vs duplicates, re-point index.ts import to proxy-config.js or document re-export to avoid drift. Files: index.ts:1-40, src/config/proxy-config.ts:76-86, src/proxy.ts.
• Known-drift unification is explicit but breaking — confirm intentional: resolveDisableTools default false→true (src/config/proxy-config.ts:76), bool tokens unified to 1/true/yes/y/on vs 0/false/no/n/off everywhere (old index.ts:12-22 loose + DEBUG/ISOLATED exact-match gone). CHANGELOG.md + AGENTS.md §4 do document it (library consumers must set DISABLE_TOOLS=false explicitly, yes/on/TRUE now recognized). Keep as documented breaking note, do not silently backport to REQUEST_TIMEOUT_MS 180000 vs 300000, MANAGE_BACKEND false vs true, OMIT_SYSTEM_PROMPT false vs auto-true — those drifts are correctly preserved per AGENTS.md §4, index.ts:295,209-212,313-316, src/config/proxy-config.ts:16,308,374-377.

NITS:

• docs/README.md: removal of A dedicated section in [API Reference] is tracked below + updated status table (Configuration:canonical env names documented, API Reference:needs English canonical + /health/details + /metrics) is EN-first consistent. No English | 中文 bar pointing at nonexistent files, no docs/host-deployments.md link in visible hunks — good.
• docs/configuration.md:🧮合并与正规化规则 + Docker/Compose为什么一律留空 + entrypoint端口校验 correctly explains ENV X=/${X:-} empty-default rationale and entrypoint.sh:normalize_port 1..65535 behavior. Matches Dockerfile, docker-compose.yml:ports/healthcheck ${OPENCODE_PROXY_PORT:-${PORT:-10000}}, docs/docker.md.
• CHANGELOG.md new Phase 3A + Stage entries are EN-first (Unified env/file/default config merge..., Docker six-way symmetry..., etc.) with Chinese gloss after — respects EN-first rule.
• No npm run lint or test:unit === npm test renaming seen in docs hunks — good.

Matrix-close file list for new/changed envs (EXTERNAL_TOOL_POLICY_MODE/DEFAULT_RISK_LEVEL/ALLOWLIST/DENYLIST/REQUIRE_CONFIRMATION_FOR, PORT, RETRY_MAX_RETRIES, UPSTREAM_*):

• .env.example — has canonical + legacy EXTERNAL_TOOL_*, PORT, RETRY_MAX_RETRIES, UPSTREAM_*; check OPENCODE_PROXY_BIND_HOST.
• config.json.example — has EXTERNAL_TOOL_* five keys; pre-existing PORT/RETRY_MAX_RETRIES/UPSTREAM_* assumed present (not in diff).
• Dockerfile — empty ENV for all incl. five new + legacies — done.
• docker-compose.yml — empty ${X:-} for all incl. five new + legacies, ports/healthcheck dual-fallback — done.
• index.ts — resolveEnumSetting/resolveListSetting/resolvePortSetting/resolveRetryCountSetting with canonical>legacy>file — done.
• docs/configuration.md — tables + merge/normalization section + config.json example — done.
• AGENTS.md §4 table — canonical/file/default + resolveDisableTools + single-merge + Docker-empty + drifts — done.

FINAL_VERDICT: COMMENT

Session: response_id=resp_xzC6aoCLMKOB1e8PtfqoiAw,resp_req_vrtx_011CfVar2E6wPdcw6jPj4zY5,chatcmpl-0a023414-f782-4fd5-8675-8727e2bdba54 (chain via previous_response_id, 30min TTL). model=[model] provider=[provider].

[samson910022]

Re-review summary (HEAD 607798a)

Re-reviewed the latest PR state after the follow-up commits 32ed0b1 and 607798a.

Previous blocker — resolved ✅

The prior blocker was the mismatch between the migration guidance and the actual external-tool policy matcher:

• implementation uses exact Set.has() matching;
• CHANGELOG previously recommended EXTERNAL_TOOL_REQUIRE_CONFIRMATION_FOR=delete_*,write_*, which would not match delete_repo / write_file.

This is now fixed in 32ed0b1:

• CHANGELOG.md explicitly states allowlist / denylist / confirmation lists are exact-match only;
• the unsafe wildcard example was removed;
• migration guidance now uses exact names such as delete_repo,write_file;
• docs/configuration.md also explicitly documents that * wildcards are unsupported and that matching is against original / namespaced / declared names.

This now matches src/tool-runtime/policy.ts and closes the security-sensitive migration gap.

Latest follow-up changes

607798a is low risk:

• cleans up minor docs drift in docs/README.md;
• clarifies the Responses continuation-state 128-entry cap / eviction / TTL behavior;
• changes the port-return path in entrypoint.sh from echo to printf '%s\n'.

No new behavioral regression found in these changes.

Rechecked prior findings

• ✅ ID-less tool tracking fix remains sound and is covered by the added regression tests.
• ✅ External-tool visible streaming is intentionally buffered until completion and is now documented as a deliberate fail-closed trade-off.
• ✅ The Anthropic bare {type:'tool'} direct-converter behavior is explicitly tested; the public /v1/messages route rejects the invalid form with 400.
• ✅ Retry counts are effectively clamped by resolveMaxRetries() at runtime; the wider merged value is intentional/tested rather than an unbounded retry path.
• ✅ Latest CI run #78 is green: typecheck/build/Jest, shellcheck, Docker build + health smoke test, and bot lint all passed.

Remaining non-blocking follow-ups

• Concurrent same-name + ID-less backend tool calls still share one tracking bucket by design. This is documented/tested; a future multiset or snapshot-derived tracker could remove the limitation.
• Requests with external tool contracts still buffer visible pure-text output until completion. A future rolling/prefix quarantine could recover lower-latency streaming while preserving fail-closed markup handling.

Verdict

APPROVE

I do not see a remaining merge-blocking correctness, safety, or API-compatibility issue at the current HEAD 607798a.

The remaining items above are follow-up improvements rather than reasons to block this PR.