feat: add native Anthropic support to ToolHive gateway - #1339
Conversation
8236635 to
bb9b5a5
Compare
samuv
changed the title
Cursory review (Standards + Spec)Fully or partially written by an AI agent. Two-axis review of StandardsHard violations
Judgement-call smells
Spec(a) Missing/partial: None found. All three acceptance scenarios (registration, discovery-path derivation, metadata threading, shared token source, transport stripping, independent outcomes, default compatibility, status/UI presentation) have direct test coverage, including the requirement to carry output/thinking limits into the emitted Anthropic (b) Scope creep — The spec asks for concurrent fetch only across the ToolHive pair ("Fetch both ToolHive protocol entries concurrently under the existing operation-wide bounds"). (c) Implemented-but-questionable: Same fragile substring-match flagged under Standards — Summary: Standards — 1 hard violation (missing tracker update) + 4 judgement-call smells, worst being the hint-string coupling between Worth a decision: was widening background/on-demand refresh concurrency to all providers (not just ToolHive) intentional? If so, it probably deserves its own test coverage and a line in the ADR/acceptance doc; if not, scope it back to the ToolHive pair. |
kantord
left a comment
kantord
left a comment
There was a problem hiding this comment.
sorry, see the comments above
bb9b5a5 to
2113e55
Compare
|
Addressed the review findings in the rebased branch:
The app and TUI keep package-local ToolHive-family predicates because the TUI cannot import Verified with |
2113e55 to
7674de7
Compare
|
Rebased onto current
Verified with |
7674de7 to
ff721eb
Compare
ff721eb to
803bd07
Compare
JAORMX
left a comment
JAORMX
left a comment
There was a problem hiding this comment.
Panel review of 803bd07, taking the unified provider configuration/commands (#1448) and follow-up #1456 into account. The separate ToolHive protocol identities and shared externally owned credential lifecycle fit the current architecture. Requesting changes for one correctness blocker: startup-discovered native metadata is not published before the default provider/session can be minted. Other comments are explicitly non-blocking improvements or optional coverage/polish. The duplicate Spec/Standards recovery-command finding is posted once. CI was green; live e2e was skipped. No local tests or linters were run. AI-assisted review.
| diag.Log(ctx, port.LevelWarn, "toolhive LLM gateway: "+statusHintFor(entry, statusEmpty), | ||
| "provider", pid, "base_url", entry.baseURL) | ||
| defaultEmpty = defaultEmpty || reg.defaultID == pid | ||
| default: |
There was a problem hiding this comment.
Blocker — publish native probe metadata before reminting the default. This successful probe records models in reg.outcomes, but does not publish them into reg.meta before the default is selected/reminted below. Build initially uses modelSnapshot (inventory floors), and the later live refresh is asynchronous, so an immediate session can run without the authoritative metadata that startup already fetched. For a gateway-specific alias absent from the embedded catalog, that means a 4096 output-token fallback and prefix-guessed thinking instead of the returned output/thinking descriptor; manual thinking for an adaptive-only model can produce a provider rejection. A text-only model can also receive adapter-passthrough image capability. Please publish successful probe metadata before remint/session construction and add a real Build-to-immediate-request regression. The existing native metadata/wire test manually calls reg.meta.mergeSwap, which bypasses this gap (AC1.3).
There was a problem hiding this comment.
Fixed in 947cf06: successful startup probes now publish per-protocol metadata before default selection/remint. The new real Build-to-immediate-native-request test verifies max_tokens=64000 and adaptive thinking without a manual metadata merge.
|
|
||
| var toolhiveDirectStatusHints = map[string]string{ | ||
| statusUnreachable: "check gateway connectivity or use `--toolhive-llm-mode proxy`", | ||
| statusUnauthorized: "re-auth with `mecatui llm login` or `thv llm setup`", |
There was a problem hiding this comment.
Important, non-blocking — this recovery command was removed by #1448. Direct-mode unauthorized status now recommends mecatui llm login, but the supported command is mecatui providers login toolhive. Both protocol identities share that existing ToolHive credential flow, so no new login identity is needed. Please update the hint to that command or retain only the working thv llm setup alternative. This is both the AC3.3 actionable-remediation gap and the ADR0333 command-surface mismatch; posting it once rather than duplicating the two panel axes.
There was a problem hiding this comment.
Updated the direct-mode hint to the supported mecatui providers login toolhive command, while retaining thv llm setup as the alternative.
| anthropicoption.WithHTTPClient(client), | ||
| anthropicoption.WithMaxRetries(0), | ||
| )) | ||
| entry.lister = anthropicLister{inner: anthropic.NewLister("", baseURL, client)} |
There was a problem hiding this comment.
Important, non-blocking — keep native catalog error bodies out of diagnostics (CWE-532). Wiring the Anthropic lister here adds an OIDC-bearing catalog request to a path whose listerStatusError.Error() includes the SDK error text/response body. resolveProviderModels subsequently logs that error verbatim during refresh (internal/app/modellister.go:695). If a gateway or intermediary diagnostic response reflects the received Authorization header, the ToolHive bearer is persisted in Mecatl logs. Preserve the HTTP status for classification but expose a response-body-free diagnostic error. A regression can return a 401 body containing a sentinel credential and assert that diagnostics include provider/state but not the sentinel or response body.
There was a problem hiding this comment.
Fixed: Anthropic catalog API errors now expose only a sanitized HTTP status while preserving StatusCode(). Provider-level diagnostics coverage reflects a bearer sentinel in a 401 body and proves it is not logged.
| if reg.Default() != providerToolhive { | ||
| t.Fatalf("Default() = %q, want %q", reg.Default(), providerToolhive) | ||
| } | ||
| native, _ := reg.Lookup(providerToolhiveAnthropic) |
There was a problem hiding this comment.
Important, non-blocking test coverage — also drive the legacy direct-mode Responses entry (AC2.1/AC3.4). This test lists both protocols but only streams through toolhive-anthropic. Please also select toolhive in direct mode and assert Responses JSON reaches /gateway/v1/responses with the authoritative bearer. The existing proxy inference coverage does not protect the refactored direct entry against an adapter/path wiring regression.
There was a problem hiding this comment.
Added direct-mode legacy coverage: toolhive now streams Responses JSON through /gateway/v1/responses with the fresh authoritative bearer and no x-api-key.
| } | ||
| } | ||
|
|
||
| func TestToolhiveNativeAnthropic_Scenario2_TransportSecurity(t *testing.T) { |
There was a problem hiding this comment.
Important, non-blocking test coverage — exercise transport policy after remint (AC2.3). The direct remint coverage currently checks only that the legacy remint returns a non-nil provider; the native remint is not driven. This helper-level transport test does not prove either adapter's remint closure retains the bearer client and redirect policy. Please drive reminted providers through capture transports and assert protocol-correct paths, authoritative bearer-only authentication, and redirect refusal. Dropping an option from a remint closure should fail the test.
There was a problem hiding this comment.
Added coverage that drives both reminted providers and verifies protocol-correct paths, bearer-only authentication, and redirect refusal via a zero-hit redirect target.
| return []modelEntry{{ID: l.id}}, nil | ||
| } | ||
|
|
||
| func TestToolhiveProtocolRefreshesStartConcurrently(t *testing.T) { |
There was a problem hiding this comment.
Important, non-blocking test coverage — simultaneous starts do not prove the deadline contract (AC3.1). These tests manually release both listers, and concurrentProbeLister ignores its context. They would stay green if the Build/background/stale-refresh timeout contexts disappeared. Please add context-aware stalled-sibling cases that verify the required bounds (1.5s/10s/2s, without necessarily waiting the full duration) and that the healthy protocol's result survives when its sibling reaches the deadline.
There was a problem hiding this comment.
Added context-aware coverage for the 1.5s startup probe, 10s background operation, and 2s stale-refresh bounds. The startup and stale tests stall one protocol through deadline expiry and verify the healthy sibling is still published.
| } | ||
| } | ||
|
|
||
| func TestToolhiveNativeAnthropic_Scenario3_IndependentOutcomes(t *testing.T) { |
There was a problem hiding this comment.
Advisory — extend the independent last-known-good transition proof (AC3.1). This covers OpenAI success followed by OpenAI failure/native success. An additional step failing both protocols after each has acquired a distinct successful catalog would verify that both retained catalogs and counts remain separate, catching cross-overwrite/aliasing after native success. Optional coverage improvement, not a merge blocker.
There was a problem hiding this comment.
Extended the transition proof: after each protocol acquires a distinct successful catalog, both endpoints fail and each provider retains only its own last-known-good inventory.
| return false | ||
| } | ||
|
|
||
| func TestToolhiveNativeAnthropic_Scenario1_Registration(t *testing.T) { |
There was a problem hiding this comment.
Advisory — add a native persisted-selector regression. Registration and direct adapter streaming do not exercise Service-level persistence/rehydration. The existing rehydrate_toolhive_test.go selects only providerToolhive. Consider extending it to select, persist, reload, and run toolhive-anthropic, including temporarily unavailable discovery, so a future native-only rejection in the factory/rehydration path is caught. Optional coverage improvement, not a merge blocker.
There was a problem hiding this comment.
Extended the two-Build persistence/rehydration test to cover both toolhive and toolhive-anthropic selectors, including request-time behavior after discovery becomes unavailable.
| } | ||
| } | ||
|
|
||
| func containsString(values []string, want string) bool { |
There was a problem hiding this comment.
Advisory / optional polish: slices.Contains already implements this helper. Import slices, replace the two calls, and remove containsString; no new abstraction is needed. Not a merge blocker.
There was a problem hiding this comment.
Done: replaced the local helper with slices.Contains.
803bd07 to
bad85ad
Compare
|
Addressed the panel review on the latest
Update: rebased again after #1461 corrected the duplicate ADR on |
Co-Authored-By: OpenAI Codex <codex@openai.com> Signed-off-by: Samuele Verzi <samu@stacklok.com>
bad85ad to
947cf06
Compare
3 participants
Summary
toolhive-anthropicas a distinct native Anthropic Messages provider while preservingtoolhiveas the OpenAI Responses defaultProtocols
toolhive:GET /v1/models,POST /v1/responsestoolhive-anthropic:GET /anthropic/v1/models,POST /anthropic/v1/messagesVerification
task fmttask docstask site:buildtask api:checktask linttask testgo run ./cmd/mecademointernal/appandcmd/mecatui/uiteststask ac-trace-strictcurrently stops on the pre-existingdocs/acceptance/callmcpwithquery-broker-support.mdstatusproposed, which the checker does not recognize.