Promote deferred MCP tools on a registry-miss instead of 404ing

[timkjr]

Summary

Under the shipped default (core preset, defer mode), most MCP tools are held in a deferred/lazy catalog and only reachable after tools_search promotes them into the live registry. Three internal dispatch paths look the tool up in the live registry only, with no fallback to the deferred catalog, so a perfectly valid tool 404s / errors until something else happens to promote it first:

• Handler.CallToolStrict (internal/server/handler.go) — every internal /v1/* dashboard route goes through this. Concretely, the dashboard's Processes and Communities pages call get_processes / get_communities by name and get tool "get_processes" is not registered, even though the tool exists and works fine once promoted.
• Handler.handleToolCall (POST /v1/tools/{name}, the public REST tool-invocation endpoint) — same gap, returns 404 tool_not_found. This one already returns an available_tools list on the 404 response, which suggests the intent was a complete/helpful surface rather than a deliberate "must call tools_search first" gate — so I've treated this as the same bug rather than a separate design decision.
• newLocalToolExecutor (cmd/gortex/server_router.go) — the federation router's local-dispatch path has the identical pattern; a remote server routing a deferred tool call to this daemon would 404 the same way.

None of these needed new machinery — (*mcp.Server).EnsureToolPromoted already existed for exactly this purpose (built for the CLI's gortex call path) and is already tested (internal/mcp/promote_on_demand_test.go).

Changes

• Handler gains a promoteTool func(name string) bool field + SetToolPromoter setter (nil-safe).
• New Handler.getToolOrPromote helper: look up live, and on a miss, consult the promoter and retry once. CallToolStrict and handleToolCall both now go through this instead of duplicating the retry logic.
• SetToolPromoter(srv.EnsureToolPromoted) wired at every place that constructs a Handler/eval.Handler: cmd/gortex/daemon.go, cmd/gortex/mcp.go, cmd/gortex/eval_server.go, and bench/daemon-latency/main.go (the one benchmark that was missing it).
• newLocalToolExecutor gets the equivalent promote-then-retry against *mcp.Server directly (it predates Handler, so it can't share getToolOrPromote).

Tests

• TestCallToolStrict_PromotesDeferredTool / TestCallToolStrict_PromoterDeclines_StillMissing (internal/server/handler_strict_test.go)
• TestToolCallPromotesDeferredTool (internal/server/handler_test.go) — same fix, through the HTTP POST /v1/tools/{name} path specifically, since it serializes a different response shape than CallToolStrict.
• newLocalToolExecutor's fix isn't independently re-tested — it's a direct, mechanical mirror of the already-tested EnsureToolPromoted contract, and go test ./cmd/gortex/... covers the surrounding router behavior.
• Full suite: go build ./... and go test ./internal/server/... ./cmd/gortex/... ./internal/mcp/... ./bench/... all green.

Considered and declined

handleToolCall promotes via the context-less EnsureToolPromoted, not the session-aware EnsureToolPromotedForSession. I considered switching to the session-aware variant since this route is externally reachable, but handleToolCall's existing GetTool lookup already doesn't do session-based gating either — a live tool is callable here regardless of session today. Promoting via the plain variant doesn't newly bypass anything relative to that existing baseline, so I left it as-is rather than threading a context.Context through the promoter signature for a gap that isn't actually a regression. Happy to revisit if there's a reason session gating matters here that I'm missing.

[zzet]

Hey @timkjr! Thanks for addressing the deferred-tool failures affecting the dashboard. Looking at the technical side of the PR, the sequential happy path works, and the constructor wiring is complete. However, I don’t think this PR is ready to merge yet.

1. Concurrent promotion can return a false 404

In internal/server/handler.go:101-105 and cmd/gortex/server_router.go:35-41, GetTool is retried only when EnsureToolPromoted returns true.

The lazy registry marks a tool as promoted before its registration callback completes. This permits the following race:

1. Request A marks the tool promoted but has not yet called AddTool.
2. Request B sees GetTool == nil.
3. B’s promotion returns false because A already marked the tool.
4. B skips the retry and returns 404/not-registered.

Please make promotion atomic from the callers’ perspective - e.g., singleflight/in-flight synchronization where concurrent callers wait until registration finishes. An immediate unconditional retry is not sufficient.

A synchronized two-request regression test is needed for both Handler and newLocalToolExecutor.

2. Context-free promotion weakens the facade/session boundary

The new promoter uses global EnsureToolPromoted, while EnsureToolPromotedForSession checks facade-v1, hide-mode, host, and editing exclusions before mutating the shared registry.

The wrapped tool handler still performs its call-time gate, so I am not claiming an authentication bypass. The problem is that a blocked session can still promote a legacy tool globally before execution is rejected, causing cross-session registry/tool-list churn and undermining the facade/defer boundary.

If generic promotion remains, the promoter should accept context.Context, bind the HTTP session before lookup, and use EnsureToolPromotedForSession.

Architecturally, I would prefer not to promote arbitrary legacy names at all.

3. Malformed federation JSON can be invoked as empty arguments

cmd/gortex/server_router.go:51-69 discards JSON parsing errors and continues with nil arguments. This predates the PR, but the new branch expands that behavior to every deferred tool.

Please validate and reject malformed input before lookup, promotion, or invocation, returning a 400-equivalent response. Add a test proving malformed input causes neither promotion nor handler execution.

4. Test coverage is insufficient at the changed boundary

The new tests use synchronous fake promoter callbacks. They do not exercise:

• The real lazy registry in core/defer mode.
• Concurrent cold promotion.
• newLocalToolExecutor, currently uncovered.
• Facade-v1/hide/session policy through REST and federation.
• Malformed input.
• Real Mcp-Session-Id or overlay-session behavior.

GitHub currently reports no CI checks for this PR.

At minimum, I would require focused race/integration coverage along these lines:

go test -race ./internal/server -run 'Test(CallToolStrict|ToolCall|Processes|Communities|Dashboard)'
go test -race ./internal/mcp -run 'Test.*(Promot|Facade|Tool.*Gate)'
go test -race ./cmd/gortex -run 'Test.*(LocalToolExecutor|Router|DaemonV1)'

Speaking about the big picture: I have an Architectural concern

This PR fixes the immediate web symptom by making the generic legacy dispatcher more capable. That moves in the opposite direction from facade v1 and makes /v1/tools/{name} harder to retire.

The failing dashboard calls already have facade replacements:

• get_processes → analyze {kind: "processes"}
• get_communities → analyze {kind: "communities"}

My preferred short-term fix is:

1. Change the typed /v1/processes, /v1/communities, and dashboard handlers to call the eager analyze facade.
2. Do not auto-promote arbitrary names through public /v1/tools/{name}.
3. Review federation promotion separately with session-aware semantics and direct tests.

For the web application, I recommend a small server-side Next.js BFF:

• Browser calls typed, same-origin /api/gortex/* routes.
• The BFF holds the daemon token server-side and connects to Streamable MCP /mcp.
• Only allowlisted facade tool/operation pairs are exposed.
• No arbitrary browser-controlled tool names, daemon URLs, cwd, or raw MCP pass-through.
• Existing hooks can remain stable while src/lib/api.ts changes transport.

This also avoids exposing NEXT_PUBLIC_GORTEX_TOKEN to browser JavaScript and putting the SSE token in a URL.

The remaining facade gaps—raw graph snapshot, enriched repo statistics, guard inventory, activity/event parity—should become a few typed operations under existing facade tools, with narrowly typed legacy REST retained temporarily where necessary.

Other observations

• I found no off-by-one error, introduced dead code, hardcoded secret, or removed HTTP authentication.
• All four production Handler construction sites are wired correctly.
• The sequential functionality works; the concerns above are concurrency, input handling, policy boundaries, and long-term interface direction.

Overall, I got feedback about a broken dashboard (it was expected with the migration to the facade v1), and if the scope for the web/gortex updates is too big/vague, I can work on fixing the dashboard functionality soon (next week or the following week).

[timkjr]

Reworked per your review — the generic promotion approach is gone. Thanks for the detailed feedback; the facade direction is clearly right.

What changed

1. Dashboard routes now use the eager analyze facade — no generic promotion.

• internal/server/dashboard.go: handleProcesses, handleCommunities, handleContracts, handleContractsValidate, and the dashboard snapshot now call analyze {kind: processes|communities|contracts} instead of the deferred legacy names.
• internal/mcp/facade_tools.go: wrapLegacyFacade routes a bare analyze(kind=…) call to handleFacade when the kind is a facade-aliased operation (processes → get_processes, communities → get_communities, contracts → contracts). handleFacade holds the captured legacy handler directly — no registry lookup, no promotion, no tools/list_changed churn. Native dispatcher kinds (hotspots, dead_code, …) keep their existing path.
• The generic promoteTool hook, SetToolPromoter, and getToolOrPromote are removed from Handler; the four production wire sites drop the call. /v1/tools/{name} no longer auto-promotes arbitrary names.

2. Race fix — lazyToolRegistry.Promote is now atomic.

• The promoted mark and the live AddTool happen under one lock, so a concurrent caller can never observe a tool marked promoted but not yet registered.
• EnsureToolPromoted now returns liveness ("re-check GetTool") rather than "I transitioned it", so a racing caller that didn't do the transition still retries instead of 404ing.
• Regression test: TestPromote_ConcurrentCallersNeverFalse404 — two synchronized goroutines racing the same promotion; both must observe the tool live.

3. Federation path is session-aware.

• newLocalToolExecutor now uses EnsureToolPromotedForSession + WithAuthorizedToolCall, mirroring daemon_mcp.go — a facade-v1/hide-mode session cannot mutate the shared lazy registry before the call gate rejects the tool.

4. Malformed federation JSON rejected with 400.

• newLocalToolExecutor validates the body before any lookup, promotion, or invocation; malformed input returns 400 invalid_json and never runs the handler. Tests prove neither promotion nor handler execution happens.

5. Test coverage at the changed boundaries.

• TestPromote_ConcurrentCallersNeverFalse404 (real lazy registry, concurrent cold promotion)
• TestAnalyzeAliasedKindFromLegacySession / TestAnalyzeAliasedKindWithIDReachesProcessDetail (facade alias through real MCP dispatch, asserts no promotion)
• TestLocalExecutor_MalformedJSONRejectedBeforePromotion / TestLocalExecutor_MalformedFlatArgsRejected (malformed input → no promotion, no handler)
• TestLocalExecutor_ValidNestedArgsDispatches / TestLocalExecutor_ValidFlatArgsDispatches / TestLocalExecutor_UnknownTool404 (executor coverage, previously missing)
• All pass under -race for your specified suites: go test -race ./internal/server -run 'Test(CallToolStrict|ToolCall|Processes|Communities|Dashboard)', go test -race ./internal/mcp -run 'Test.*(Promot|Facade|Tool.*Gate)', go test -race ./cmd/gortex -run 'Test.*(LocalToolExecutor|Router|DaemonV1)'.

Architectural note: the remaining facade gaps you listed (raw graph snapshot, enriched repo stats, guard inventory, activity/event parity) are untouched here — I kept the scope to the dashboard breakage. Happy to do those as follow-ups, or take them on if you'd like.

The web app's processDetail was the one direct consumer of the deferred get_processes name; it now calls analyze(kind=processes, id=…) (gortexhq/web#17).

[timkjr]

Follow-up on the rework (60c94f6): two of the rework's own tests did not actually regression-test the change, now fixed. Also: the connection to #661.

Test corrections (60c94f6)

1. TestAnalyzeAliasedKindFromLegacySession was not testing the legacy path. It used facadeFrameCaller, whose initialize handshake with a non-empty client name makes the session a facade-v1 session (clientDefaultPolicy — the exact behavior Streamable HTTP door serves a different tool surface when clientInfo.name is empty: intentional? #661 describes). The facade alias already worked there pre-rework, so the test passed on both old and new code. Rewritten to invoke the analyze tool's registered handler directly with a bare context — exactly what the HTTP dashboard path does via CallToolStrict (no MCP session, no client name). Verified: fails on the pre-rework code (unknown analyze kind: processes), passes on the rework.

2. TestPromote_ConcurrentCallersNeverFalse404 was timing-dependent. The pre-fix race window (mark under lock, AddTool outside) is a few instructions wide; the old test passed on old code. Rewritten with a deterministic interleaving barrier. Honest limitation: the test cannot deterministically FAIL on old code — any release ordering that makes the failure deterministic deadlocks the fixed code (whose Promote blocks on the held lock). It exercises the concurrent path, passes under -race, and documents the contract; the race fix itself is the atomic lock change (mark + AddTool under one lock).

On #661

I read the issue (surface keyed on clientInfo.name; empty name → 55-tool legacy catalogue, non-empty → 21-tool facade; mcp-go's empty-name default and reconnect surface-drop). The rework's relationship to it:

• The dashboard breakage you referenced ('feedback about a broken dashboard') is the same root cause: the web app's HTTP path has no client name → legacy surface → analyze {kind: processes} was 'unknown analyze kind'. The rework fixes that symptom via facadeAnalyzeKindAliased, which is surface-independent (it consults the static facade registry, not the session policy) — so the aliased kinds now work on both surfaces.
• It does not fix Streamable HTTP door serves a different tool surface when clientInfo.name is empty: intentional? #661 itself: the surface fork and the reconnect surface-drop remain open. Those need daemon-side changes (pin the surface to session lineage, or serve one surface with explicit opt-in for the flat catalogue) — separate from this PR.
• The rework is consistent with Streamable HTTP door serves a different tool surface when clientInfo.name is empty: intentional? #661's 'one surface' direction: aliased kinds behave identically regardless of client identity.

Happy to take #661 as a follow-up if you want it.

[zzet]

Request changes on exact head 9fd00d470eb47541cd56ddf5c874985318905355.

The atomic promotion direction is reasonable, but the following issues need to be addressed before merge.

1. High — session-specific tool policy can be bypassed

cmd/gortex/server_router.go:60-73 checks IsToolEnabledForSession only when the tool is absent and then unconditionally applies WithAuthorizedToolCall. An already-live tool is therefore executed without a policy check. For a registry miss, internal/server/handler.go:365-379 routes using raw r.Context() before attaching Mcp-Session-Id at lines 447-454, so the new check evaluates the daemon/default surface and can globally promote a tool hidden for that session.

Reproduce: promote get_processes under the default surface, configure session facade-session as facade-v1/hide, then POST /v1/tools/get_processes with Mcp-Session-Id: facade-session. The hidden tool is invoked.

Fix: attach the session/overlay context before Decide; fail closed on !IsToolEnabledForSession before both lookup and promotion; only mark the call authorized after that check. Add an end-to-end Handler + Router test proving the hidden tool remains unpromoted and uncalled.

2. Medium — JSON null invokes the handler

cmd/gortex/server_router.go:36-59 accepts both top-level null and {"arguments": null}. Both unmarshals succeed with a nil map and the handler runs with nil arguments.

Reproduce with a probe handler and body []byte("null"): the executor returns success and handlerRan becomes true; expected is 400 with no promotion or invocation.

Fix: require a non-nil JSON object at the root and require arguments to be an object when present. Add tests for both null forms.

3. High — the concurrency regression test never executes

internal/mcp/lazy_tools_test.go:436-475 starts two goroutines that wait on <-start, but the test never closes start, reads results, or asserts anything. It returns green while leaking both goroutines and still passes if the old racy Promote implementation is restored.

Fix: start the callers deterministically, release the promotion barrier, collect both results with a bounded timeout, assert both observe the live tool, and verify both goroutines exit.

4. Medium — the router promotion branch has no end-to-end coverage

The successful cases in cmd/gortex/server_router_test.go manually register already-live tools; the only registry miss uses an unknown name. Deleting the new promotion block leaves these tests green.

Fix: use a real deferred tool and cover cold promotion, two concurrent cold calls, hidden-session denial without promotion, and malformed/null input without invocation.

Please also run:

go build ./cmd/gortex/... ./internal/mcp/... ./internal/server/...
go test -race ./cmd/gortex/... ./internal/mcp/... ./internal/server/...

GitHub currently reports the workflows with no jobs, so the exact head does not yet have independent CI validation.