refactor(mcp): build the tool surface from transport providers - #82
Closed
jlrickert wants to merge 9 commits into
Closed
refactor(mcp): build the tool surface from transport providers#82jlrickert wants to merge 9 commits into
jlrickert wants to merge 9 commits into
Conversation
This repo's own project config records the maintainer's default keg and flight. Those are per-developer choices, not shared state, so the directory should never have been commitable in the first place. Also ignores *.iml alongside the existing .idea/ entry.
ConfigService kept four independently-invalidated caches (user, project, merged, warnings) behind a `cache bool` threaded through every read. The flag made each call site restate a question it had no basis to answer, and the caches could disagree with one another mid-process. Configuration is now read once and fixed for the life of the process. A `tap` command runs against one consistent snapshot; a long-lived `tap mcp` session picks up an external edit at its next orient, which is the only place Reload is called. Nothing inside a session can write configuration, so "edit the file, then reorient" is the whole update story. The snapshot is immutable once published, so concurrent readers need no coordination beyond the mutex guarding the pointer. That matters because the MCP SDK dispatches every call except initialize asynchronously. Read-modify-write paths that rewrite a config file need the bytes on disk rather than the snapshot, so they call ReadUserConfigFile / ReadProjectConfigFile and then Reload once the write lands. Load() returns warnings alongside the merged config instead of leaving them on a struct field that only happened to be populated by the last Config() call.
The root command copied the configured flight into KegTargetOptions on every invocation, which meant a persisted `flight:` silently narrowed keg access for direct commands like `tap cat` and `tap edit`. That contradicted the documented rule that direct CLI commands are governed by normal keg authorization rather than flight cover, and it made the configured selection indistinguishable from an explicit --flight once it reached resolution. Selection now happens where it is meaningful: ActiveFlightName reads the snapshot when orientation asks for it, and --flight remains the only way to put a flight into per-command keg targeting. A flight persisted in user or project config therefore no longer narrows direct CLI commands. Pass --flight explicitly to opt a single invocation into flight cover enforcement.
ImportFromKeg resolved the source keg at viewer regardless of options, but leave_stubs rewrites source nodes to point at their new home. A viewer-only flight could therefore mutate a keg it was only authorized to read. The source role now tracks what the operation actually does: viewer for a plain copy, editor when leave_stubs is requested.
NewServer branched on a Surface enum to decide which tools to register, which meant every capability difference between `tap mcp` and the hub's /mcp endpoint was encoded as "which enum am I". The enum conflated things that vary independently — who the caller is, which catalog backs discovery, and whether the server shares a filesystem with its agent — so adding a transport meant auditing every branch. Transport differences are now expressed as four interfaces: OrientationProvider selects and renders flight authority, FlightProvider supplies flight CRUD, KegDiscoveryProvider lists reachable kegs, and IdentityProvider reports who the session is. Nil providers fall back to local adapters over tap; the hub injects authenticated, catalog-backed implementations. Registration itself no longer branches. Two consequences for the published surface: - auth_status gives way to auth_info, which returns structured identities and flight-filtered kegs and is deliberately credential-free. Tokens, email, scopes, cookies, expiry, and session data are absent from the wire shape by construction. - keg_list drops its hub selector and filters through the session's immutable active-flight cover, so discovery cannot report kegs the flight excludes. A session may now edit or delete its own active flight. The previous blanket prohibition meant a flight granting manage_flights could administer every flight except the one it most likely needed to correct. A successful self-edit adopts the exact returned manifest before the response is released, so removing manage_flights removes the mutation tools immediately; a self-delete enters recovery immediately. If the edit persists but rendering fails, the session enters recovery rather than retaining stale authority. Attachment transfers split into two variants behind ServerOptions.SharedFilesystem, since only stdio shares a filesystem with its agent host. The hosted variant omits local-path fields from the published schema rather than refusing them at call time, so a hosted agent has no vocabulary to name the server's own disk. `tap mcp` opts in separately. config, config_template, license, repo_init, export, import, namespace_list, and keg_visibility leave MCP entirely: they operate on machine-local Tapper state or perform tenant administration. The parity coverage map records the rationale per method. Migration notes for embedders: mcp.Surface, SurfaceFull, SurfaceHub, ServerOptions.Surface, ServerOptions.OrientationLoader, and ServerOptions.LicenseText are gone; construct provider implementations instead. The nine tools listed above are no longer registered on any transport.
`tap mcp` runs on the same machine as the agent driving it, so a path in a tool argument names the same file for both sides. It now opts into the shared-filesystem variant and regains the full attachment round-trip: upload_file and upload_image accept source_path and file: URIs alongside byte sources, download_file writes to dest_path, and download_image takes an optional dest_path, still returning MCP image content when omitted. The option set is a named function rather than an inline literal so the choice is assertable without standing up a stdio server. Dropping it would silently cost `tap mcp` these transfers while leaving every other test green, which is exactly how they were lost.
Exercising the MCP and flight changes by hand means repeatedly starting
Claude Code or Codex against a chosen model with a chosen flight, and
making the flight stick means exporting TAP_FLIGHT or editing config
between runs.
`tap launch HARNESS --agent NAME` collapses that. An agent aliases a
model, a flight, an endpoint, and how to authenticate:
agents:
local:
model: ollama/qwen3.6:35b-mlx
baseUrl: http://192.168.50.197:11434/v1
flight: '@homelab/+ecw'
sub:
model: anthropic/claude-opus-4
auth: subscription
work:
model: openai/gpt-5
apiKeyEnv: WORK_OPENAI_KEY
Models are provider-qualified because the provider decides which protocol
the harness must speak; an unqualified model is rejected rather than
guessed at. Ollama serves both the OpenAI API and the Anthropic Messages
API, so it is the one provider every harness can drive — Claude Code
included. Codex against a hosted Anthropic model, and Claude Code against
a hosted OpenAI model, stay refused before anything is spawned.
One baseUrl serves both protocols: the launcher appends /v1 for OpenAI
clients, which add /chat/completions, and strips it for Anthropic ones,
which add /v1/messages themselves.
Auth is explicit because absence cannot express intent — an unset key
means "inherit", which silently prefers an exported API key over a
subscription login. `auth: subscription` therefore removes the inherited
provider key variables from the child environment, which an overlay
cannot do: appending can override a variable but never unset one.
apiKeyEnv names the variable holding a key, never the key itself,
mirroring HubEntry.TokenEnv, so agents hold no secrets and need no
trust-boundary strip in project config.
The agent's flight is exported as TAP_FLIGHT, which needs no new plumbing
— it already outranks project and user config in the flight chain, so a
tap mcp session started inside the harness orients to it.
Launch is ResolveLaunch plus execution, so --dry-run and a real run
cannot drift, and the whole surface is testable without spawning a
harness.
Registered under the existing IncludeIntegrations profile gate, so the
pruned keg binary does not grow the command. Excluded from MCP surface
coverage: spawning processes on the server's host is not an agent
operation.
Experimental and intentionally undocumented — this integrates with
Tapper Hub later and will be redesigned, so docs and the config template
are deliberately untouched.
A session that cannot establish flight authority had almost no way to say so. The tool list is filtered to the recovery set, so an agent never gets to call a locked tool and see the error explaining why — leaving an empty KEG table as the only signal. Models strong enough to reason from an absence recovered; weaker ones did nothing at all. The two recovery paths also had their messages crossed. A blank selection produced the generic orientation document, which never mentions flights being locked. A selection that failed to resolve produced "no flight is selected", which is false and sends the reader hunting for missing configuration instead of the real fault — usually a wrong flight name or an unreachable hub. Now the payload carries a Flight section whenever no flight is active, naming the state and the three steps out of it, and a failed resolution leads with the actual error. Both stay within the payload's MCP-first rule of never naming CLI commands. The orient tool description carries the same imperative, because tool descriptions are the one thing every model reads; server instructions alone are not enough for a small local model.
graph rendered a standalone HTML page — DOCTYPE, stylesheet, and an embedded JavaScript renderer — and returned the whole document as tool text. An agent cannot display it, and the part it could actually use, the node and edge JSON, was buried inside a script tag. The static scaffolding alone ran to roughly 8KB before any nodes, so every call spent context on markup nobody reads. The MCP and CLI renderings had also drifted: GraphOptions.BundleJS is set only by the CLI, so MCP fell through to the inline fallback bundle and produced a visibly different page from the same data. Unregistered rather than deleted outright — Tap.Graph and `tap graph --output` still serve the case that works, writing a file to open in a browser, until the feature is removed. The tool row is dropped from the embedded tool inventory too: that text ships inside the orientation payload, so leaving it would advertise a tool that no longer exists.
Owner
Author
|
Already on Verified identical: #83 has been rebased onto the new main and retargeted to it; #84 follows #83. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
First of a four-PR sequence. Merge in order; each later PR targets the one
before it and retargets to
mainas its parent lands.Reworks the MCP surface so the tool set is assembled from transport providers
rather than wired per transport, and moves configuration and flight resolution
onto a single process-wide snapshot with one reload boundary at orientation.
Commits:
chore: ignore the repo-local.tapperdirectoryrefactor(tapper): resolve configuration from one process-wide snapshotfeat(cli): resolve the configured flight at orientation, not per commandfix(tapper): require editor authority whenimport_from_kegleaves stubsrefactor(mcp): build the tool surface from transport providersfeat(mcp): publish local-path attachment transfers ontap mcpfeat(cli): add experimentaltap launchfor agent harnessesfix(mcp): state the recovery situation in the orientation payloadrefactor(mcp): disable the deprecated graph toolSequence
mainThe telemetry back-off fix is independent of all three and targets
mainseparately.