From fcf4697b43504179fdbe8539737fe375cd08b000 Mon Sep 17 00:00:00 2001 From: Jared Rickert Date: Mon, 3 Aug 2026 23:56:42 -0500 Subject: [PATCH 1/9] chore: ignore the repo-local .tapper directory 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. --- .gitignore | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/.gitignore b/.gitignore index 8960f0f4..5d9f3c5a 100644 --- a/.gitignore +++ b/.gitignore @@ -32,9 +32,14 @@ go.work.sum # Editor/IDE .idea/ +*.iml .vscode/* !.vscode/settings.json +# This repo's own tapper project config is developer-local: it records the +# maintainer's default keg and flight, which are not shared state. +.tapper/ + # macOS .DS_Store From 0a8fb9d49df1881c80170dbde82c46bca17725bd Mon Sep 17 00:00:00 2001 From: Jared Rickert Date: Mon, 3 Aug 2026 23:59:48 -0500 Subject: [PATCH 2/9] refactor(tapper): resolve configuration from one process-wide snapshot 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. --- pkg/cli/cmd_auth.go | 4 +- pkg/cli/cmd_bootstrap.go | 2 +- pkg/cli/cmd_namespace.go | 2 +- pkg/cli/cmd_root.go | 6 +- pkg/cli/keg_target_flags.go | 2 +- pkg/mcp/session_flight.go | 2 +- pkg/tapper/config_env_test.go | 48 +++--- pkg/tapper/config_service.go | 212 +++++++++++++++--------- pkg/tapper/config_service_test.go | 144 +++++++++++++--- pkg/tapper/flight.go | 2 +- pkg/tapper/invocation_telemetry.go | 4 +- pkg/tapper/invocation_telemetry_test.go | 4 +- pkg/tapper/keg_service.go | 10 +- pkg/tapper/node_ref_resolve.go | 4 +- pkg/tapper/tap_bootstrap.go | 33 ++-- pkg/tapper/tap_bootstrap_test.go | 18 +- pkg/tapper/tap_config.go | 12 +- pkg/tapper/tap_config_explain_test.go | 53 +----- pkg/tapper/tap_doctor.go | 6 +- pkg/tapper/tap_flight.go | 4 +- pkg/tapper/tap_hub.go | 12 +- pkg/tapper/tap_info.go | 8 +- pkg/tapper/tap_init.go | 8 +- pkg/tapper/tap_keg.go | 2 +- pkg/tapper/tap_namespace.go | 6 +- pkg/tapper/tap_orient.go | 27 +-- pkg/tapper/tap_orient_test.go | 52 ++++++ pkg/tapper/tap_use.go | 2 +- pkg/tapper/tap_use_test.go | 2 +- 29 files changed, 436 insertions(+), 255 deletions(-) diff --git a/pkg/cli/cmd_auth.go b/pkg/cli/cmd_auth.go index 013b042c..e616286e 100644 --- a/pkg/cli/cmd_auth.go +++ b/pkg/cli/cmd_auth.go @@ -71,7 +71,7 @@ func runAuthLogin(ctx context.Context, deps *Deps, p authLoginParams) (*authLogi // unflagged login lands on the configured default — or the compiled-in // DefaultHubURL — without forcing every invocation to repeat --hub. An // explicit/selected URL still wins. - cfg, err := deps.Tap.ConfigService.Config(true) + cfg, err := deps.Tap.ConfigService.Config() if err != nil { return nil, err } @@ -320,7 +320,7 @@ platform). For scripts, pass --hub and pipe a token to --with-token: // chain / compiled-in default. selectedHub := hubURL if selectedHub == "" && isTTY && !withToken { - cfg, err := deps.Tap.ConfigService.Config(true) + cfg, err := deps.Tap.ConfigService.Config() if err != nil { return err } diff --git a/pkg/cli/cmd_bootstrap.go b/pkg/cli/cmd_bootstrap.go index 6d1e4113..30225d12 100644 --- a/pkg/cli/cmd_bootstrap.go +++ b/pkg/cli/cmd_bootstrap.go @@ -319,7 +319,7 @@ func bootstrapUserFlight(deps *Deps) string { if deps == nil || deps.Tap == nil { return "" } - cfg, err := deps.Tap.ConfigService.UserConfig(false) + cfg, err := deps.Tap.ConfigService.ReadUserConfigFile() if err != nil || cfg == nil { return "" } diff --git a/pkg/cli/cmd_namespace.go b/pkg/cli/cmd_namespace.go index a6c8a93f..c598b41e 100644 --- a/pkg/cli/cmd_namespace.go +++ b/pkg/cli/cmd_namespace.go @@ -178,7 +178,7 @@ func configNamespaceNames(deps *Deps) []string { if err != nil { return nil } - cfg, err := tap.ConfigService.Config(true) + cfg, err := tap.ConfigService.Config() if err != nil || cfg == nil { return nil } diff --git a/pkg/cli/cmd_root.go b/pkg/cli/cmd_root.go index 23b04383..7fa37f76 100644 --- a/pkg/cli/cmd_root.go +++ b/pkg/cli/cmd_root.go @@ -144,10 +144,10 @@ func NewRootCmd(deps *Deps) *cobra.Command { // Fall back to config values when CLI flags are not // explicitly set. Precedence: CLI flag > config > default. - cfg, cfgErr := tap.ConfigService.Config(true) + cfg, warnings, cfgErr := tap.ConfigService.Load() // Surface config load warnings (corrupt YAML, permission errors). - if warnings := tap.ConfigService.LoadWarnings; len(warnings) > 0 { + if len(warnings) > 0 { if deps.Strict { var msgs []string for _, w := range warnings { @@ -405,7 +405,7 @@ func kegFlagCompletions(ctx context.Context, deps *Deps, toComplete string) []st return nil } - cfg, _ := tap.ConfigService.Config(true) + cfg, _ := tap.ConfigService.Config() bareNamespace := completionBareNamespace(deps.Runtime, cfg) if ctx == nil { diff --git a/pkg/cli/keg_target_flags.go b/pkg/cli/keg_target_flags.go index d2685fc6..680607f4 100644 --- a/pkg/cli/keg_target_flags.go +++ b/pkg/cli/keg_target_flags.go @@ -77,7 +77,7 @@ func configHubNames(deps *Deps) []string { if err != nil { return nil } - cfg, err := tap.ConfigService.Config(true) + cfg, err := tap.ConfigService.Config() if err != nil || cfg == nil { return nil } diff --git a/pkg/mcp/session_flight.go b/pkg/mcp/session_flight.go index e4ad0ed0..68367540 100644 --- a/pkg/mcp/session_flight.go +++ b/pkg/mcp/session_flight.go @@ -78,7 +78,7 @@ func (g *sessionFlightGate) loadLocal(ctx context.Context) (*tapper.Flight, stri // adoption boundary. Launcher-bound sessions still reload configuration // because it supplies hub routing and credentials, but selection remains the // immutable --flight value. - g.tap.ConfigService.ResetCache() + g.tap.ConfigService.Reload() ref := g.staticFlight if ref == "" { ref = g.tap.ActiveFlightName("") diff --git a/pkg/tapper/config_env_test.go b/pkg/tapper/config_env_test.go index dcd1c5b3..1156c670 100644 --- a/pkg/tapper/config_env_test.go +++ b/pkg/tapper/config_env_test.go @@ -20,21 +20,21 @@ func TestConfigService_FlightPrecedence(t *testing.T) { require.NoError(t, fx.Runtime().AtomicWriteFile( tap.PathService.UserConfig(), []byte("flight: '@local/+baseline'\n"), 0o644)) - cfg, err := tap.ConfigService.Config(false) + cfg, err := tap.ConfigService.Config() require.NoError(t, err) require.Equal(t, "@local/+baseline", cfg.Flight()) require.NoError(t, fx.Runtime().AtomicWriteFile( filepath.Join(project, ".tapper", "config.yaml"), []byte("flight: '@local/+project'\n"), 0o644)) - tap.ConfigService.ResetCache() - cfg, err = tap.ConfigService.Config(false) + tap.ConfigService.Reload() + cfg, err = tap.ConfigService.Config() require.NoError(t, err) require.Equal(t, "@local/+project", cfg.Flight(), "project config should override the user baseline") require.NoError(t, fx.Runtime().Env().Set("TAP_FLIGHT", "@local/+environment")) - tap.ConfigService.ResetCache() - cfg, err = tap.ConfigService.Config(false) + tap.ConfigService.Reload() + cfg, err = tap.ConfigService.Config() require.NoError(t, err) require.Equal(t, "@local/+environment", cfg.Flight(), "TAP_FLIGHT should override project config") } @@ -61,7 +61,7 @@ func TestConfigService_EnvOverridesDefaultKeg(t *testing.T) { // Set TAP_DEFAULT_KEG env var to override. require.NoError(t, fx.Runtime().Env().Set("TAP_DEFAULT_KEG", "personal")) - cfg, err := tap.ConfigService.Config(false) + cfg, err := tap.ConfigService.Config() require.NoError(t, err) require.NotNil(t, cfg) require.Equal(t, "personal", cfg.DefaultKeg(), "TAP_DEFAULT_KEG should override user config") @@ -87,7 +87,7 @@ func TestConfigService_EnvOverridesLogLevel(t *testing.T) { require.NoError(t, fx.Runtime().Env().Set("TAP_LOG_LEVEL", "debug")) - cfg, err := tap.ConfigService.Config(false) + cfg, err := tap.ConfigService.Config() require.NoError(t, err) require.NotNil(t, cfg) require.Equal(t, "debug", cfg.LogLevel(), "TAP_LOG_LEVEL should override user config") @@ -107,7 +107,7 @@ func TestConfigService_EnvDefaultNamespaceOverride(t *testing.T) { require.NoError(t, fx.Runtime().Env().Set("TAP_DEFAULT_NAMESPACE", "envteam")) - cfg, err := tap.ConfigService.Config(false) + cfg, err := tap.ConfigService.Config() require.NoError(t, err) require.NotNil(t, cfg) require.Equal(t, "envteam", cfg.DefaultNamespace(), "TAP_DEFAULT_NAMESPACE should set the default namespace") @@ -132,7 +132,7 @@ func TestConfigService_EnvAbsentFallsThrough(t *testing.T) { )) // No env vars set -- config file values should be used. - cfg, err := tap.ConfigService.Config(false) + cfg, err := tap.ConfigService.Config() require.NoError(t, err) require.NotNil(t, cfg) require.Equal(t, "blog", cfg.DefaultKeg(), "without env override, config file value should be used") @@ -163,7 +163,7 @@ func TestConfigService_MultipleEnvVarsSet(t *testing.T) { require.NoError(t, fx.Runtime().Env().Set("TAP_FALLBACK_KEG", "personal")) require.NoError(t, fx.Runtime().Env().Set("TAP_DEFAULT_HUB", "custom")) - cfg, err := tap.ConfigService.Config(false) + cfg, err := tap.ConfigService.Config() require.NoError(t, err) require.NotNil(t, cfg) require.Equal(t, "work", cfg.DefaultKeg()) @@ -195,7 +195,7 @@ func TestConfigService_DisableAtlasHubViaEnv(t *testing.T) { require.NoError(t, fx.Runtime().Env().Set("TAP_DISABLE_ATLAS_HUB", raw)) - cfg, err := tap.ConfigService.Config(false) + cfg, err := tap.ConfigService.Config() require.NoError(t, err) require.True(t, cfg.DisableAtlasHub(), "TAP_DISABLE_ATLAS_HUB=%q should set DisableAtlasHub", raw) @@ -215,7 +215,7 @@ func TestConfigService_DisableAtlasHubViaEnv(t *testing.T) { require.NoError(t, fx.Runtime().Env().Set("TAP_DISABLE_ATLAS_HUB", "0")) - cfg, err := tap.ConfigService.Config(false) + cfg, err := tap.ConfigService.Config() require.NoError(t, err) require.False(t, cfg.DisableAtlasHub()) }) @@ -237,7 +237,7 @@ func TestConfigService_DisableLocalHubViaEnv(t *testing.T) { require.NoError(t, fx.Runtime().Env().Set("TAP_DISABLE_LOCAL_HUB", "true")) - cfg, err := tap.ConfigService.Config(false) + cfg, err := tap.ConfigService.Config() require.NoError(t, err) require.True(t, cfg.DisableLocalHub(), "TAP_DISABLE_LOCAL_HUB=true should set DisableLocalHub") @@ -265,7 +265,7 @@ func TestConfigService_EnvOverrideWithStrict(t *testing.T) { // Set env var -- should still work even with corrupt config. require.NoError(t, fx.Runtime().Env().Set("TAP_DEFAULT_KEG", "envkeg")) - cfg, err := tap.ConfigService.Config(false) + cfg, err := tap.ConfigService.Config() require.NoError(t, err, "env overrides should still work with corrupt config") require.NotNil(t, cfg) @@ -273,8 +273,8 @@ func TestConfigService_EnvOverrideWithStrict(t *testing.T) { require.Equal(t, "envkeg", cfg.DefaultKeg()) // The corrupt user config should produce a load warning. - require.Len(t, tap.ConfigService.LoadWarnings, 1) - require.Equal(t, "user config", tap.ConfigService.LoadWarnings[0].Source) + require.Len(t, loadWarnings(t, tap), 1) + require.Equal(t, "user config", loadWarnings(t, tap)[0].Source) } func TestConfigService_ConfigPathBypassesCascade(t *testing.T) { @@ -301,7 +301,7 @@ func TestConfigService_ConfigPathBypassesCascade(t *testing.T) { // Set env var that should be ignored when ConfigPath is set. require.NoError(t, fx.Runtime().Env().Set("TAP_DEFAULT_KEG", "envkeg")) - cfg, err := tap.ConfigService.Config(false) + cfg, err := tap.ConfigService.Config() require.NoError(t, err) require.NotNil(t, cfg) require.Equal(t, "explicit", cfg.DefaultKeg(), "ConfigPath should bypass cascade including env vars") @@ -321,19 +321,19 @@ func TestConfigService_CachingPreserved(t *testing.T) { require.NoError(t, fx.Runtime().Env().Set("TAP_DEFAULT_KEG", "first")) - cfg1, err := tap.ConfigService.Config(false) + cfg1, err := tap.ConfigService.Config() require.NoError(t, err) require.Equal(t, "first", cfg1.DefaultKeg()) // Change env var, but use cache=true -- should return cached value. require.NoError(t, fx.Runtime().Env().Set("TAP_DEFAULT_KEG", "second")) - cfg2, err := tap.ConfigService.Config(true) + cfg2, err := tap.ConfigService.Config() require.NoError(t, err) require.Equal(t, "first", cfg2.DefaultKeg(), "cache=true should return cached config") // With cache=false, should pick up new env value. - tap.ConfigService.ResetCache() - cfg3, err := tap.ConfigService.Config(false) + tap.ConfigService.Reload() + cfg3, err := tap.ConfigService.Config() require.NoError(t, err) require.Equal(t, "second", cfg3.DefaultKeg(), "after ResetCache, should read new env value") } @@ -365,14 +365,14 @@ func TestConfigService_EnvOverridesProjectConfig(t *testing.T) { )) // Without env, project should override user. - cfg, err := tap.ConfigService.Config(false) + cfg, err := tap.ConfigService.Config() require.NoError(t, err) require.Equal(t, "projectkeg", cfg.DefaultKeg()) // With env, env should override project. - tap.ConfigService.ResetCache() + tap.ConfigService.Reload() require.NoError(t, fx.Runtime().Env().Set("TAP_DEFAULT_KEG", "envkeg")) - cfg, err = tap.ConfigService.Config(false) + cfg, err = tap.ConfigService.Config() require.NoError(t, err) require.Equal(t, "envkeg", cfg.DefaultKeg(), "env should override both user and project config") } diff --git a/pkg/tapper/config_service.go b/pkg/tapper/config_service.go index 4a9c1541..cf448d21 100644 --- a/pkg/tapper/config_service.go +++ b/pkg/tapper/config_service.go @@ -5,6 +5,7 @@ import ( "fmt" "os" "path/filepath" + "sync" "github.com/jlrickert/cli-toolkit/cfgcascade" "github.com/jlrickert/cli-toolkit/toolkit" @@ -27,6 +28,20 @@ type ConfigLoadWarning struct { } // ConfigService loads, merges, and resolves tapper configuration state. +// +// Configuration is read once and then fixed for the life of the process. A +// `tap` command therefore runs against one consistent snapshot, and a +// long-lived `tap mcp` session picks up an external edit at its next orient, +// which is the one place Reload is called. Nothing inside a session can write +// configuration — the `config` tool is read-only — 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 itself. That matters +// because the MCP SDK dispatches every call except initialize asynchronously. +// +// Flight authority is not affected by a reload: the MCP session gate snapshots +// its resolved flight separately, so it stays stable across a reload either way. type ConfigService struct { Runtime *toolkit.Runtime @@ -35,24 +50,23 @@ type ConfigService struct { // ConfigPath is the path to the config file. ConfigPath string - // LoadWarnings accumulates non-fatal issues from the last Config() call. - // Missing config files are not warnings (graceful degradation). Corrupt - // YAML, permission errors, etc. are recorded here. - LoadWarnings []ConfigLoadWarning - - // ResolvedSources lists provider names that contributed to the merged config, - // most-specific first. Populated after Config() runs the cascade. - ResolvedSources []string - - // Cached configs. - userCache *Config - projectCache *Config - - // projectWarnings holds load/trust-boundary warnings produced by the most - // recent project-config walk, surfaced through LoadWarnings by Config(). - projectWarnings []ConfigLoadWarning + // mu guards snap. The snapshot it points at is never mutated after being + // published, so readers may use it after releasing the lock. + mu sync.Mutex + snap *resolved +} - mergedCache *Config +// resolved is one complete read of the configuration cascade. Every field is +// populated together by load and read-only thereafter. Tier errors are captured +// alongside their values so UserConfig and ProjectConfig report exactly what a +// direct read would have. +type resolved struct { + merged *Config + user *Config + userErr error + project *Config + projectErr error + warnings []ConfigLoadWarning } // NewConfigService builds a ConfigService rooted at root. @@ -67,14 +81,41 @@ func NewConfigService(root string, rt *toolkit.Runtime) (*ConfigService, error) }, nil } -// ResetCache clears cached user, project, and merged configs. -func (s *ConfigService) ResetCache() { - s.mergedCache = nil - s.userCache = nil - s.projectCache = nil - s.projectWarnings = nil - s.LoadWarnings = nil - s.ResolvedSources = nil +// Reload discards the snapshot so the next read re-reads from disk. Orientation +// is its only caller: it is the point at which a session re-establishes the +// context it is operating under. +func (s *ConfigService) Reload() { + s.mu.Lock() + defer s.mu.Unlock() + s.snap = nil +} + +// snapshot returns the process-wide configuration read, performing it on first +// use. The load runs under the lock so a burst of concurrent first calls +// resolves once rather than racing. +func (s *ConfigService) snapshot() (*resolved, error) { + s.mu.Lock() + defer s.mu.Unlock() + if s.snap != nil { + return s.snap, nil + } + snap, err := s.load() + if err != nil { + return nil, err + } + s.snap = snap + return snap, nil +} + +// Load returns the merged configuration together with the non-fatal issues +// found while reading it. Missing files are not warnings (graceful +// degradation); corrupt YAML and permission errors are. +func (s *ConfigService) Load() (*Config, []ConfigLoadWarning, error) { + snap, err := s.snapshot() + if err != nil { + return nil, nil, err + } + return snap.merged, snap.warnings, nil } // UserConfigExists reports whether a user config file is present — the signal @@ -93,18 +134,20 @@ func (s *ConfigService) UserConfigExists() bool { return false } -// UserConfig returns the global user configuration. -func (s *ConfigService) UserConfig(cache bool) (*Config, error) { - if cache && s.userCache != nil { - return s.userCache, nil - } - path := filepath.Join(s.PathService.ConfigRoot, "config.yaml") - cfg, err := ReadConfig(s.Runtime, path) +// UserConfig returns the global user configuration from the process snapshot. +func (s *ConfigService) UserConfig() (*Config, error) { + snap, err := s.snapshot() if err != nil { return nil, err } - s.userCache = cfg - return cfg, nil + return snap.user, snap.userErr +} + +// ReadUserConfigFile reads the user configuration straight from disk, bypassing +// the snapshot. Use it when about to modify and rewrite that file, so the +// read-modify-write cycle starts from what is actually on disk. +func (s *ConfigService) ReadUserConfigFile() (*Config, error) { + return ReadConfig(s.Runtime, filepath.Join(s.PathService.ConfigRoot, "config.yaml")) } // WalkConfigsUp returns the absolute paths of every existing rel file found by @@ -137,11 +180,24 @@ func WalkConfigsUp(rt *toolkit.Runtime, start, rel string) []string { // project layers — only the user config may define them — and each strip is // recorded as a load warning surfaced by Config(). Returns keg.ErrNotExist when // no project config exists. -func (s *ConfigService) ProjectConfig(cache bool) (*Config, error) { - if cache && s.projectCache != nil { - return s.projectCache, nil +func (s *ConfigService) ProjectConfig() (*Config, error) { + snap, err := s.snapshot() + if err != nil { + return nil, err } + return snap.project, snap.projectErr +} +// ReadProjectConfigFile walks and merges the project configs straight from +// disk, bypassing the snapshot. Same purpose as ReadUserConfigFile. +func (s *ConfigService) ReadProjectConfigFile() (*Config, error) { + cfg, _, err := s.readProjectConfig() + return cfg, err +} + +// readProjectConfig performs the project walk and returns the merged result +// along with the trust-boundary warnings it produced. +func (s *ConfigService) readProjectConfig() (*Config, []ConfigLoadWarning, error) { relDir := filepath.Base(s.PathService.LocalConfigRoot) // ".tapper" rel := filepath.Join(relDir, "config.yaml") paths := WalkConfigsUp(s.Runtime, s.PathService.Root, rel) @@ -181,24 +237,35 @@ func (s *ConfigService) ProjectConfig(cache bool) (*Config, error) { } } - s.projectWarnings = warnings if merged == nil { - s.projectCache = nil - return nil, keg.ErrNotExist + return nil, warnings, keg.ErrNotExist } - s.projectCache = merged - return merged, nil + return merged, warnings, nil } -// Config returns the merged user and project configuration with optional caching. -// If cache is true and a merged config exists, it returns the cached version. -// Otherwise, it uses a cfgcascade.Cascade to resolve configuration from three -// providers in rank order: user config file, project config file, TAP_* env vars. -// When ConfigPath is set, it directly reads that file and bypasses the cascade. -func (s *ConfigService) Config(cache bool) (*Config, error) { - if cache && s.mergedCache != nil { - return s.mergedCache, nil +// Config returns the merged user, project, and environment configuration from +// the process snapshot. +func (s *ConfigService) Config() (*Config, error) { + snap, err := s.snapshot() + if err != nil { + return nil, err } + return snap.merged, nil +} + +// load performs one complete read of the cascade: both tiers, then the merge. +// It builds and returns a value without touching service state, which is what +// lets snapshot publish it as an immutable pointer. +// +// The merge resolves three providers in rank order — user config, project +// config, TAP_* env vars. When ConfigPath is set it reads that file instead and +// bypasses the cascade entirely. +func (s *ConfigService) load() (*resolved, error) { + out := &resolved{} + out.user, out.userErr = s.ReadUserConfigFile() + + var projectWarnings []ConfigLoadWarning + out.project, projectWarnings, out.projectErr = s.readProjectConfig() if s.ConfigPath != "" { cfg, err := ReadConfig(s.Runtime, s.ConfigPath) @@ -208,12 +275,10 @@ func (s *ConfigService) Config(cache bool) (*Config, error) { if cfg == nil { cfg = &Config{} } - s.mergedCache = cfg - return cfg, nil + out.merged = cfg + return out, nil } - s.LoadWarnings = nil - userPath := filepath.Join(s.PathService.ConfigRoot, "config.yaml") projectPath := filepath.Join(s.PathService.LocalConfigRoot, "config.yaml") @@ -224,14 +289,13 @@ func (s *ConfigService) Config(cache bool) (*Config, error) { Provider: &cfgcascade.FuncProvider[*Config]{ ProviderName: "user config", Fn: func(_ func(string) string) (*Config, error) { - cfg, err := s.UserConfig(cache) - if err != nil { - if errors.Is(err, keg.ErrNotExist) { + if out.userErr != nil { + if errors.Is(out.userErr, keg.ErrNotExist) { return nil, os.ErrNotExist } - return nil, err + return nil, out.userErr } - return cfg, nil + return out.user, nil }, }, }, @@ -240,14 +304,13 @@ func (s *ConfigService) Config(cache bool) (*Config, error) { Provider: &cfgcascade.FuncProvider[*Config]{ ProviderName: "project config", Fn: func(_ func(string) string) (*Config, error) { - cfg, err := s.ProjectConfig(cache) - if err != nil { - if errors.Is(err, keg.ErrNotExist) { + if out.projectErr != nil { + if errors.Is(out.projectErr, keg.ErrNotExist) { return nil, os.ErrNotExist } - return nil, err + return nil, out.projectErr } - return cfg, nil + return out.project, nil }, }, }, @@ -280,13 +343,12 @@ func (s *ConfigService) Config(cache bool) (*Config, error) { } rv := cascade.Resolve(s.Runtime.Env().Get) - s.ResolvedSources = rv.Sources // Surface trust-boundary / per-layer warnings accumulated by the project // config walk (the cascade only sees the merged result). - s.LoadWarnings = append(s.LoadWarnings, s.projectWarnings...) + out.warnings = append(out.warnings, projectWarnings...) - // Map cascade provider errors to LoadWarnings. + // Map cascade provider errors to warnings. for _, pe := range rv.Errors { var path string switch pe.Name { @@ -295,7 +357,7 @@ func (s *ConfigService) Config(cache bool) (*Config, error) { case "project config": path = projectPath } - s.LoadWarnings = append(s.LoadWarnings, ConfigLoadWarning{ + out.warnings = append(out.warnings, ConfigLoadWarning{ Source: pe.Name, Path: path, Message: fmt.Sprintf("failed to load %s at %s: %v", pe.Name, path, pe.Err), @@ -303,21 +365,19 @@ func (s *ConfigService) Config(cache bool) (*Config, error) { }) } - merged := rv.Value - if merged == nil { - merged = &Config{data: &configDTO{}} + out.merged = rv.Value + if out.merged == nil { + out.merged = &Config{data: &configDTO{}} } - - s.mergedCache = merged - return s.mergedCache, nil + return out, nil } // ResolveTarget resolves a keg selector to a keg target. When the selector is // empty it uses defaultKeg, then fallbackKeg. The selector is parsed as a keg // reference and turned into a concrete target by Config.ResolveAlias (the // namespace-centric ResolveRef chain and per-hub-kind backend mapping). -func (s *ConfigService) ResolveTarget(alias, nsOverride, hubOverride string, cache bool) (*keg.Target, error) { - cfg, err := s.Config(cache) +func (s *ConfigService) ResolveTarget(alias, nsOverride, hubOverride string) (*keg.Target, error) { + cfg, err := s.Config() if err != nil { return nil, fmt.Errorf("failed to resolve target: %w", err) } diff --git a/pkg/tapper/config_service_test.go b/pkg/tapper/config_service_test.go index dfb5213e..b676485d 100644 --- a/pkg/tapper/config_service_test.go +++ b/pkg/tapper/config_service_test.go @@ -2,6 +2,7 @@ package tapper_test import ( "strings" + "sync" "testing" "github.com/jlrickert/cli-toolkit/sandbox" @@ -9,6 +10,16 @@ import ( "github.com/stretchr/testify/require" ) +// loadWarnings returns the non-fatal issues from a fresh read of the cascade. +// Warnings travel with the config they came from now, so tests ask for both +// together rather than reading a field left behind by the last call. +func loadWarnings(t *testing.T, tap *tapper.Tap) []tapper.ConfigLoadWarning { + t.Helper() + _, warnings, err := tap.ConfigService.Load() + require.NoError(t, err) + return warnings +} + func TestConfigService_Config_MissingFilesReturnDefaults(t *testing.T) { t.Parallel() @@ -21,10 +32,10 @@ func TestConfigService_Config_MissingFilesReturnDefaults(t *testing.T) { }) require.NoError(t, err) - cfg, err := tap.ConfigService.Config(false) + cfg, err := tap.ConfigService.Config() require.NoError(t, err) require.NotNil(t, cfg) - require.Empty(t, tap.ConfigService.LoadWarnings, "missing files should not produce warnings") + require.Empty(t, loadWarnings(t, tap), "missing files should not produce warnings") } func TestConfigService_Config_CorruptUserConfig(t *testing.T) { @@ -43,12 +54,12 @@ func TestConfigService_Config_CorruptUserConfig(t *testing.T) { userCfgPath := tap.PathService.UserConfig() require.NoError(t, fx.Runtime().AtomicWriteFile(userCfgPath, []byte(":::invalid yaml{{{"), 0o644)) - cfg, err := tap.ConfigService.Config(false) + cfg, err := tap.ConfigService.Config() require.NoError(t, err, "corrupt config should not return error (graceful degradation)") require.NotNil(t, cfg) - require.Len(t, tap.ConfigService.LoadWarnings, 1) - require.Contains(t, tap.ConfigService.LoadWarnings[0].Message, "failed to load user config") - require.Equal(t, "user config", tap.ConfigService.LoadWarnings[0].Source) + require.Len(t, loadWarnings(t, tap), 1) + require.Contains(t, loadWarnings(t, tap)[0].Message, "failed to load user config") + require.Equal(t, "user config", loadWarnings(t, tap)[0].Source) } func TestConfigService_Config_CorruptProjectConfig(t *testing.T) { @@ -67,12 +78,12 @@ func TestConfigService_Config_CorruptProjectConfig(t *testing.T) { projCfgPath := tap.PathService.ProjectConfig() require.NoError(t, fx.Runtime().AtomicWriteFile(projCfgPath, []byte("not: [valid: yaml"), 0o644)) - cfg, err := tap.ConfigService.Config(false) + cfg, err := tap.ConfigService.Config() require.NoError(t, err, "corrupt config should not return error (graceful degradation)") require.NotNil(t, cfg) - require.Len(t, tap.ConfigService.LoadWarnings, 1) - require.Contains(t, tap.ConfigService.LoadWarnings[0].Message, "failed to load project config") - require.Equal(t, "project config", tap.ConfigService.LoadWarnings[0].Source) + require.Len(t, loadWarnings(t, tap), 1) + require.Contains(t, loadWarnings(t, tap)[0].Message, "failed to load project config") + require.Equal(t, "project config", loadWarnings(t, tap)[0].Source) } func TestConfigService_Config_BothCorrupt(t *testing.T) { @@ -90,10 +101,10 @@ func TestConfigService_Config_BothCorrupt(t *testing.T) { require.NoError(t, fx.Runtime().AtomicWriteFile(tap.PathService.UserConfig(), []byte(":::bad"), 0o644)) require.NoError(t, fx.Runtime().AtomicWriteFile(tap.PathService.ProjectConfig(), []byte(":::bad"), 0o644)) - cfg, err := tap.ConfigService.Config(false) + cfg, err := tap.ConfigService.Config() require.NoError(t, err) require.NotNil(t, cfg) - require.Len(t, tap.ConfigService.LoadWarnings, 2) + require.Len(t, loadWarnings(t, tap), 2) } func TestConfigService_Config_ValidUserCorruptProject(t *testing.T) { @@ -111,11 +122,11 @@ func TestConfigService_Config_ValidUserCorruptProject(t *testing.T) { require.NoError(t, fx.Runtime().AtomicWriteFile(tap.PathService.UserConfig(), []byte("defaultKeg: pub\n"), 0o644)) require.NoError(t, fx.Runtime().AtomicWriteFile(tap.PathService.ProjectConfig(), []byte(":::bad"), 0o644)) - cfg, err := tap.ConfigService.Config(false) + cfg, err := tap.ConfigService.Config() require.NoError(t, err) require.NotNil(t, cfg) - require.Len(t, tap.ConfigService.LoadWarnings, 1) - require.Equal(t, "project config", tap.ConfigService.LoadWarnings[0].Source) + require.Len(t, loadWarnings(t, tap), 1) + require.Equal(t, "project config", loadWarnings(t, tap)[0].Source) // Valid user config should still be used. require.Equal(t, "pub", cfg.DefaultKeg()) } @@ -141,7 +152,7 @@ func TestConfigService_ProjectConfig_WalksParents(t *testing.T) { "/home/testuser/a/b/.tapper/config.yaml", []byte("defaultKeg: deep\n"), 0o644)) - cfg, err := tap.ConfigService.Config(false) + cfg, err := tap.ConfigService.Config() require.NoError(t, err) require.Equal(t, "deep", cfg.DefaultKeg(), "deeper dir overrides shallower") require.Equal(t, "keep", cfg.FallbackKeg(), "shallower value retained when not overridden") @@ -172,22 +183,26 @@ hubs: require.NoError(t, fx.Runtime().AtomicWriteFile( "/home/testuser/proj/.tapper/config.yaml", []byte(proj), 0o644)) - cfg, err := tap.ConfigService.Config(false) + cfg, err := tap.ConfigService.Config() require.NoError(t, err) require.Equal(t, "ok", cfg.DefaultKeg(), "non-hub project fields still apply") _, ok := cfg.Hubs()["evil"] require.False(t, ok, "project-defined hub must be stripped") found := false - for _, w := range tap.ConfigService.LoadWarnings { + for _, w := range loadWarnings(t, tap) { if strings.Contains(w.Message, "evil") && strings.Contains(w.Message, "hubs") { found = true } } - require.True(t, found, "expected a warning about the stripped hub, got %+v", tap.ConfigService.LoadWarnings) + require.True(t, found, "expected a warning about the stripped hub, got %+v", loadWarnings(t, tap)) } -func TestConfigService_ResetCache_ClearsWarnings(t *testing.T) { +// TestConfigService_SnapshotIsFixedUntilReload pins the contract the whole +// design rests on: configuration is read once and stays put, and Reload is the +// only thing that adopts an edit. Orientation is its sole production caller, so +// this is what "edit the file, then reorient" means underneath. +func TestConfigService_SnapshotIsFixedUntilReload(t *testing.T) { t.Parallel() fx := NewSandbox(t, sandbox.WithFixture("basic", "/home/testuser")) @@ -199,11 +214,90 @@ func TestConfigService_ResetCache_ClearsWarnings(t *testing.T) { }) require.NoError(t, err) - require.NoError(t, fx.Runtime().AtomicWriteFile(tap.PathService.UserConfig(), []byte(":::bad"), 0o644)) + require.NoError(t, fx.Runtime().AtomicWriteFile( + tap.PathService.UserConfig(), []byte("defaultKeg: before\n"), 0o644)) + + cfg, err := tap.ConfigService.Config() + require.NoError(t, err) + require.Equal(t, "before", cfg.DefaultKeg()) + + require.NoError(t, fx.Runtime().AtomicWriteFile( + tap.PathService.UserConfig(), []byte("defaultKeg: after\n"), 0o644)) + + cfg, err = tap.ConfigService.Config() + require.NoError(t, err) + require.Equal(t, "before", cfg.DefaultKeg(), "an edit must not leak into a live snapshot") + + tap.ConfigService.Reload() + cfg, err = tap.ConfigService.Config() + require.NoError(t, err) + require.Equal(t, "after", cfg.DefaultKeg(), "Reload adopts the edit") +} + +// TestConfigService_TiersComeFromTheSameSnapshot guards against the tier +// accessors drifting from the merged config they were resolved with, and +// against ReadUserConfigFile quietly being wired to the snapshot — write paths +// depend on it reporting what is actually on disk. +func TestConfigService_TiersComeFromTheSameSnapshot(t *testing.T) { + t.Parallel() + + fx := NewSandbox(t, sandbox.WithFixture("basic", "/home/testuser")) + require.NoError(t, fx.Setwd("/home/testuser")) - _, _ = tap.ConfigService.Config(false) - require.Len(t, tap.ConfigService.LoadWarnings, 1) + tap, err := tapper.NewTap(tapper.TapOptions{ + Root: "/home/testuser", + Runtime: fx.Runtime(), + }) + require.NoError(t, err) - tap.ConfigService.ResetCache() - require.Empty(t, tap.ConfigService.LoadWarnings) + require.NoError(t, fx.Runtime().AtomicWriteFile( + tap.PathService.UserConfig(), []byte("defaultKeg: before\n"), 0o644)) + + user, err := tap.ConfigService.UserConfig() + require.NoError(t, err) + require.Equal(t, "before", user.DefaultKeg()) + + require.NoError(t, fx.Runtime().AtomicWriteFile( + tap.PathService.UserConfig(), []byte("defaultKeg: after\n"), 0o644)) + + user, err = tap.ConfigService.UserConfig() + require.NoError(t, err) + require.Equal(t, "before", user.DefaultKeg(), "tier reads share the snapshot") + + onDisk, err := tap.ConfigService.ReadUserConfigFile() + require.NoError(t, err) + require.Equal(t, "after", onDisk.DefaultKeg(), "ReadUserConfigFile bypasses the snapshot") +} + +// TestConfigService_ConcurrentAccessIsRaceFree covers the one place tapper +// really is concurrent: the MCP SDK dispatches every call except initialize +// asynchronously, so overlapping tool calls read this service while an orient +// may be reloading it. Meaningful only under -race. +func TestConfigService_ConcurrentAccessIsRaceFree(t *testing.T) { + t.Parallel() + + fx := NewSandbox(t, sandbox.WithFixture("basic", "/home/testuser")) + require.NoError(t, fx.Setwd("/home/testuser")) + + tap, err := tapper.NewTap(tapper.TapOptions{ + Root: "/home/testuser", + Runtime: fx.Runtime(), + }) + require.NoError(t, err) + + var wg sync.WaitGroup + for range 8 { + wg.Add(1) + go func() { + defer wg.Done() + for range 20 { + _, _ = tap.ConfigService.Config() + _, _ = tap.ConfigService.UserConfig() + _, _ = tap.ConfigService.ProjectConfig() + _, _, _ = tap.ConfigService.Load() + tap.ConfigService.Reload() + } + }() + } + wg.Wait() } diff --git a/pkg/tapper/flight.go b/pkg/tapper/flight.go index f63be982..6e0251ea 100644 --- a/pkg/tapper/flight.go +++ b/pkg/tapper/flight.go @@ -196,7 +196,7 @@ func (s *FlightService) invalidateFlights() { } func (s *FlightService) config() (*Config, error) { - cfg, err := s.ConfigService.Config(true) + cfg, err := s.ConfigService.Config() if err != nil { return nil, err } diff --git a/pkg/tapper/invocation_telemetry.go b/pkg/tapper/invocation_telemetry.go index fef11224..4f4ea16b 100644 --- a/pkg/tapper/invocation_telemetry.go +++ b/pkg/tapper/invocation_telemetry.go @@ -95,9 +95,9 @@ func resolveInvocationTelemetryTarget(rt *toolkit.Runtime, configService *Config err error ) if configService.ConfigPath != "" { - cfg, err = configService.Config(true) + cfg, err = configService.Config() } else { - cfg, err = configService.UserConfig(true) + cfg, err = configService.UserConfig() } if err != nil || cfg == nil || cfg.DisableTelemetry() { return "", "", false diff --git a/pkg/tapper/invocation_telemetry_test.go b/pkg/tapper/invocation_telemetry_test.go index 19a214dc..5f35b6a8 100644 --- a/pkg/tapper/invocation_telemetry_test.go +++ b/pkg/tapper/invocation_telemetry_test.go @@ -76,12 +76,12 @@ func TestResolveInvocationTelemetryTargetSilentlySkipsUnavailableState(t *testin require.False(t, ok, "unbootstrapped client must skip") require.NoError(t, fx.Runtime().AtomicWriteFile(tap.PathService.UserConfig(), []byte("fallbackHub: local\nhubs:\n local:\n kind: local\n basePath: /kegs\n"), 0o644)) - tap.ConfigService.ResetCache() + tap.ConfigService.Reload() _, _, ok = resolveInvocationTelemetryTarget(fx.Runtime(), tap.ConfigService) require.False(t, ok, "local-only client must skip") require.NoError(t, fx.Runtime().AtomicWriteFile(tap.PathService.UserConfig(), []byte("fallbackHub: remote\nhubs:\n remote:\n url: https://hub.example.com\n"), 0o644)) - tap.ConfigService.ResetCache() + tap.ConfigService.Reload() _, _, ok = resolveInvocationTelemetryTarget(fx.Runtime(), tap.ConfigService) require.False(t, ok, "unauthenticated client must skip") } diff --git a/pkg/tapper/keg_service.go b/pkg/tapper/keg_service.go index 07756c58..b6cdd8ae 100644 --- a/pkg/tapper/keg_service.go +++ b/pkg/tapper/keg_service.go @@ -254,7 +254,7 @@ func (s *KegService) resolveFileKeg(ctx context.Context, root string, cache bool // `tap bootstrap` writes for the global user so anything more specific overrides. func (s *KegService) resolvePath(ctx context.Context, path, nsOverride, hubOverride string, cache bool) (keg.Keg, error) { s.ensureCache() - cfg, err := s.ConfigService.Config(true) + cfg, err := s.ConfigService.Config() if err != nil { return nil, fmt.Errorf("failed to resolve path config: %w", err) } @@ -295,7 +295,7 @@ func (s *KegService) resolveKegAlias(ctx context.Context, kegAlias, nsOverride, return s.kegCache[cacheKey], nil } - target, err := s.ConfigService.ResolveTarget(kegAlias, nsOverride, hubOverride, cache) + target, err := s.ConfigService.ResolveTarget(kegAlias, nsOverride, hubOverride) if err == nil && target != nil { k, kerr := keg.NewKegFromTarget(ctx, *target, s.Runtime, keg.WithTokenResolver(s.tokenResolver())) if kerr != nil { @@ -311,7 +311,7 @@ func (s *KegService) resolveKegAlias(ctx context.Context, kegAlias, nsOverride, // (no namespace, hub, or path) may instead name a project-local keg at // /kegs/ — resolve it so local project kegs work without // requiring any config entries. - if ref := parseKegRef(kegAlias); ref.Name != "" && ref.Namespace == "" && ref.Hub == "" && ref.Path == "" && s.allowProjectAliasFallback(cache) { + if ref := parseKegRef(kegAlias); ref.Name != "" && ref.Namespace == "" && ref.Hub == "" && ref.Path == "" && s.allowProjectAliasFallback() { if projectKeg, found, projectErr := s.resolveProjectAlias(ctx, projectRoot, ref.Name, cache); projectErr != nil { return nil, projectErr } else if found { @@ -330,11 +330,11 @@ func (s *KegService) resolveKegAlias(ctx context.Context, kegAlias, nsOverride, return nil, fmt.Errorf("keg %q could not be resolved", kegAlias) } -func (s *KegService) allowProjectAliasFallback(cache bool) bool { +func (s *KegService) allowProjectAliasFallback() bool { if s.ConfigService == nil { return true } - cfg, err := s.ConfigService.Config(cache) + cfg, err := s.ConfigService.Config() if err != nil || cfg == nil { return true } diff --git a/pkg/tapper/node_ref_resolve.go b/pkg/tapper/node_ref_resolve.go index 7fda74c3..b2a00d68 100644 --- a/pkg/tapper/node_ref_resolve.go +++ b/pkg/tapper/node_ref_resolve.go @@ -57,7 +57,7 @@ func (t *Tap) ResolveNodeRef(ctx context.Context, ref *keg.NodeRef, rc RefContex if ref.Namespace != LocalHubName && rc.CurrentKeg != nil && rc.CurrentKeg.Target() != nil { hub = strings.TrimSpace(rc.CurrentKeg.Target().Hub) } - cfg, err := t.ConfigService.Config(true) + cfg, err := t.ConfigService.Config() if err != nil { return nil, keg.NodeId{}, err } @@ -109,7 +109,7 @@ func (t *Tap) resolveRefAlias(ctx context.Context, alias string, rc RefContext) } } } - cfg, err := t.ConfigService.Config(true) + cfg, err := t.ConfigService.Config() if err != nil { return nil, err } diff --git a/pkg/tapper/tap_bootstrap.go b/pkg/tapper/tap_bootstrap.go index 86abcfea..de1a841d 100644 --- a/pkg/tapper/tap_bootstrap.go +++ b/pkg/tapper/tap_bootstrap.go @@ -124,7 +124,7 @@ func (t *Tap) Bootstrap(ctx context.Context, opts BootstrapOptions) (*BootstrapR cfg *Config created bool ) - existing, err := t.ConfigService.UserConfig(false) + existing, err := t.ConfigService.ReadUserConfigFile() switch { case err == nil: cfg = existing @@ -208,8 +208,9 @@ func (t *Tap) Bootstrap(ctx context.Context, opts BootstrapOptions) (*BootstrapR if err := cfg.Write(t.Runtime, path); err != nil { return nil, err } - // Drop the cached config so the next resolution reflects what we just wrote. - t.ConfigService.ResetCache() + // The snapshot predates this write; drop it so nothing in this process + // reads back a value we just replaced. + t.ConfigService.Reload() return &BootstrapResult{ Path: path, @@ -238,7 +239,7 @@ func (t *Tap) SetBootstrapNamespace(ctx context.Context, hubName, namespace stri if namespace == "" { return nil } - cfg, err := t.ConfigService.UserConfig(false) + cfg, err := t.ConfigService.ReadUserConfigFile() if err != nil { return fmt.Errorf("unable to load user config: %w", err) } @@ -253,8 +254,9 @@ func (t *Tap) SetBootstrapNamespace(ctx context.Context, hubName, namespace stri if err := cfg.Write(t.Runtime, t.PathService.UserConfig()); err != nil { return err } - // Drop the cached config so the next resolution reflects the adopted value. - t.ConfigService.ResetCache() + // The snapshot predates this write; drop it so nothing in this process + // reads back a value we just replaced. + t.ConfigService.Reload() return nil } @@ -271,7 +273,7 @@ func (t *Tap) SetHubDefaultNamespaceByURL(ctx context.Context, hubURL, namespace if canonical == "" { return "", nil } - cfg, err := t.ConfigService.UserConfig(false) + cfg, err := t.ConfigService.ReadUserConfigFile() if err != nil { if errors.Is(err, keg.ErrNotExist) { return "", nil @@ -292,7 +294,9 @@ func (t *Tap) SetHubDefaultNamespaceByURL(ctx context.Context, hubURL, namespace if err := cfg.Write(t.Runtime, t.PathService.UserConfig()); err != nil { return "", err } - t.ConfigService.ResetCache() + // The snapshot predates this write; drop it so nothing in this process + // reads back a value we just replaced. + t.ConfigService.Reload() return name, nil } return "", nil @@ -311,7 +315,7 @@ func (t *Tap) SetFallbackKeg(ctx context.Context, ref string) error { if ref == "" { return nil } - cfg, err := t.ConfigService.UserConfig(false) + cfg, err := t.ConfigService.ReadUserConfigFile() if err != nil { if !errors.Is(err, keg.ErrNotExist) { return fmt.Errorf("unable to load user config: %w", err) @@ -324,8 +328,9 @@ func (t *Tap) SetFallbackKeg(ctx context.Context, ref string) error { if err := cfg.Write(t.Runtime, t.PathService.UserConfig()); err != nil { return err } - // Drop the cached config so the next resolution reflects the chosen keg. - t.ConfigService.ResetCache() + // The snapshot predates this write; drop it so nothing in this process + // reads back a value we just replaced. + t.ConfigService.Reload() return nil } @@ -345,7 +350,7 @@ func (t *Tap) SetBootstrapFlight(ctx context.Context, ref string) error { if canonical == "" { return fmt.Errorf("invalid bootstrap flight %q: resolved flight has no canonical reference", ref) } - cfg, err := t.ConfigService.UserConfig(false) + cfg, err := t.ConfigService.ReadUserConfigFile() if err != nil { if !errors.Is(err, keg.ErrNotExist) { return fmt.Errorf("unable to load user config: %w", err) @@ -358,7 +363,9 @@ func (t *Tap) SetBootstrapFlight(ctx context.Context, ref string) error { if err := cfg.Write(t.Runtime, t.PathService.UserConfig()); err != nil { return err } - t.ConfigService.ResetCache() + // The snapshot predates this write; drop it so nothing in this process + // reads back a value we just replaced. + t.ConfigService.Reload() return nil } diff --git a/pkg/tapper/tap_bootstrap_test.go b/pkg/tapper/tap_bootstrap_test.go index 3777d574..e8127ac5 100644 --- a/pkg/tapper/tap_bootstrap_test.go +++ b/pkg/tapper/tap_bootstrap_test.go @@ -40,7 +40,7 @@ func TestBootstrap_Local(t *testing.T) { require.Empty(t, res.HubURL, "local has no remote URL to log in against") require.Equal(t, tapper.LocalHubName, res.Namespace, "a local deployment defaults to the @local namespace") - cfg, err := tap.ConfigService.UserConfig(false) + cfg, err := tap.ConfigService.UserConfig() require.NoError(t, err) require.Equal(t, testHost, cfg.FallbackHub()) require.Empty(t, cfg.FallbackNamespace(), "namespace comes from the hub, not a global fallback") @@ -70,7 +70,7 @@ func TestBootstrap_Cloud(t *testing.T) { require.Equal(t, tapper.DefaultHubName, res.Hub) require.Equal(t, tapper.DefaultHubURL, res.HubURL) - cfg, err := tap.ConfigService.UserConfig(false) + cfg, err := tap.ConfigService.UserConfig() require.NoError(t, err) require.Equal(t, tapper.DefaultHubName, cfg.FallbackHub()) require.Empty(t, cfg.FallbackNamespace(), "namespace comes from the hub, not a global fallback") @@ -104,7 +104,7 @@ func TestBootstrap_Enterprise(t *testing.T) { require.Equal(t, "acme", res.Hub, "hub name derived from endpoint host") require.Equal(t, "https://keg.acme.com", res.HubURL) - cfg, err := tap.ConfigService.UserConfig(false) + cfg, err := tap.ConfigService.UserConfig() require.NoError(t, err) require.Equal(t, "acme", cfg.FallbackHub()) require.Empty(t, cfg.FallbackNamespace(), "namespace comes from the hub, not a global fallback") @@ -138,7 +138,7 @@ func TestBootstrap_Enterprise_SchemeAddedAndHubNameOverride(t *testing.T) { require.Equal(t, "work", res.Hub, "explicit --hub-name wins over derivation") require.Equal(t, "https://kegs.example.org", res.HubURL, "bare host upgraded to https") - cfg, err := tap.ConfigService.UserConfig(false) + cfg, err := tap.ConfigService.UserConfig() require.NoError(t, err) require.Equal(t, "https://kegs.example.org", cfg.Hubs()["work"].URL) } @@ -177,15 +177,15 @@ func TestSetBootstrapFlight_ValidatesCanonicalizesAndResetsConfig(t *testing.T) []byte("title: Focused\n"), 0o644)) // Prime the merged cache before the write; SetBootstrapFlight must reset it. - cfg, err := tap.ConfigService.Config(true) + cfg, err := tap.ConfigService.Config() require.NoError(t, err) require.Empty(t, cfg.Flight()) require.NoError(t, tap.SetBootstrapFlight(fx.Context(), "+focused")) - userCfg, err := tap.ConfigService.UserConfig(false) + userCfg, err := tap.ConfigService.UserConfig() require.NoError(t, err) require.Equal(t, "@local/+focused", userCfg.Flight()) - merged, err := tap.ConfigService.Config(true) + merged, err := tap.ConfigService.Config() require.NoError(t, err) require.Equal(t, "@local/+focused", merged.Flight()) @@ -216,7 +216,7 @@ hubs: require.NoError(t, err) require.Equal(t, "acme-2", res.Hub, "collision with a different URL should suffix") - cfg, err := tap.ConfigService.UserConfig(false) + cfg, err := tap.ConfigService.UserConfig() require.NoError(t, err) require.Equal(t, "https://old.acme.example", cfg.Hubs()["acme"].URL, "original hub untouched") require.Equal(t, "https://keg.acme.com", cfg.Hubs()["acme-2"].URL) @@ -246,7 +246,7 @@ hubs: require.False(t, res.Created) require.Equal(t, tapper.DefaultHubName, res.Hub) - cfg, err := tap.ConfigService.UserConfig(false) + cfg, err := tap.ConfigService.UserConfig() require.NoError(t, err) require.Equal(t, tapper.DefaultHubName, cfg.FallbackHub()) // Bootstrap no longer manages fallbackNamespace, so a pre-existing value is diff --git a/pkg/tapper/tap_config.go b/pkg/tapper/tap_config.go index b7ae4fa4..7225623c 100644 --- a/pkg/tapper/tap_config.go +++ b/pkg/tapper/tap_config.go @@ -42,20 +42,20 @@ func (t *Tap) Config(opts ConfigOptions) (string, error) { } return string(raw), nil } else if opts.Project { - lCfg, err := t.ConfigService.ProjectConfig(false) + lCfg, err := t.ConfigService.ReadProjectConfigFile() if err != nil { return "", err } cfg = lCfg } else if opts.User { - uCfg, err := t.ConfigService.UserConfig(false) + uCfg, err := t.ConfigService.ReadUserConfigFile() if err != nil { return "", err } cfg = uCfg } else { var err error - cfg, err = t.ConfigService.Config(true) + cfg, err = t.ConfigService.Config() if err != nil { return "", err } @@ -279,14 +279,14 @@ func configFieldGetter(cfg *Config, field string) string { // least-specific to determine the effective source. func (t *Tap) ConfigExplain(ctx context.Context, opts ConfigExplainOptions) ([]ConfigExplainResult, error) { // Load the merged config to get final values. - merged, err := t.ConfigService.Config(true) + merged, err := t.ConfigService.Config() if err != nil { return nil, fmt.Errorf("unable to load merged config: %w", err) } // Load each tier individually. Missing configs are nil (not errors). - userCfg, _ := t.ConfigService.UserConfig(true) - projectCfg, _ := t.ConfigService.ProjectConfig(true) + userCfg, _ := t.ConfigService.UserConfig() + projectCfg, _ := t.ConfigService.ProjectConfig() // Build env config by checking TAP_* env vars. envCfg := t.loadEnvConfig() diff --git a/pkg/tapper/tap_config_explain_test.go b/pkg/tapper/tap_config_explain_test.go index 2e0cf196..48454bbb 100644 --- a/pkg/tapper/tap_config_explain_test.go +++ b/pkg/tapper/tap_config_explain_test.go @@ -188,51 +188,8 @@ func TestConfigExplain_UnknownField(t *testing.T) { require.Contains(t, err.Error(), "unknown config field") } -func TestConfigService_ResolvedSourcesPopulated(t *testing.T) { - t.Parallel() - - fx := NewSandbox(t, sandbox.WithFixture("basic", "/home/testuser")) - require.NoError(t, fx.Setwd("/home/testuser")) - - tap, err := tapper.NewTap(tapper.TapOptions{ - Root: "/home/testuser", - Runtime: fx.Runtime(), - }) - require.NoError(t, err) - - require.NoError(t, fx.Runtime().AtomicWriteFile( - tap.PathService.UserConfig(), - []byte("defaultKeg: pub\n"), - 0o644, - )) - - _, err = tap.ConfigService.Config(false) - require.NoError(t, err) - require.Contains(t, tap.ConfigService.ResolvedSources, "user config") -} - -func TestConfigService_ResolvedSourcesClearedOnReset(t *testing.T) { - t.Parallel() - - fx := NewSandbox(t, sandbox.WithFixture("basic", "/home/testuser")) - require.NoError(t, fx.Setwd("/home/testuser")) - - tap, err := tapper.NewTap(tapper.TapOptions{ - Root: "/home/testuser", - Runtime: fx.Runtime(), - }) - require.NoError(t, err) - - require.NoError(t, fx.Runtime().AtomicWriteFile( - tap.PathService.UserConfig(), - []byte("defaultKeg: pub\n"), - 0o644, - )) - - _, err = tap.ConfigService.Config(false) - require.NoError(t, err) - require.NotEmpty(t, tap.ConfigService.ResolvedSources) - - tap.ConfigService.ResetCache() - require.Empty(t, tap.ConfigService.ResolvedSources) -} +// The ResolvedSources accessor was removed along with the state behind it: it +// had no production consumer. Which tier supplied a field is still reported by +// `tap config --explain`, which derives it from the tiers themselves +// (configFieldScope) rather than from cascade bookkeeping — see +// TestConfigCommand_ExplainFlag above. diff --git a/pkg/tapper/tap_doctor.go b/pkg/tapper/tap_doctor.go index 0cb02bf3..94a7d545 100644 --- a/pkg/tapper/tap_doctor.go +++ b/pkg/tapper/tap_doctor.go @@ -20,8 +20,9 @@ type Issue = keg.DoctorIssue func (t *Tap) DoctorConfig() []Issue { var issues []Issue - // Report any config load warnings. - for _, w := range t.ConfigService.LoadWarnings { + // Report any config load warnings alongside the config they came from. + cfg, warnings, err := t.ConfigService.Load() + for _, w := range warnings { issues = append(issues, Issue{ Level: "warning", Kind: "config-load", @@ -30,7 +31,6 @@ func (t *Tap) DoctorConfig() []Issue { } // Run semantic validation on the merged config. - cfg, err := t.ConfigService.Config(true) if err != nil { issues = append(issues, Issue{ Level: "error", diff --git a/pkg/tapper/tap_flight.go b/pkg/tapper/tap_flight.go index e7865e1b..68aaaf72 100644 --- a/pkg/tapper/tap_flight.go +++ b/pkg/tapper/tap_flight.go @@ -158,7 +158,7 @@ func (t *Tap) DeleteFlight(ctx context.Context, opts DeleteFlightOptions) error } func (t *Tap) resolveWriteFlightRef(raw string) (FlightRef, HubEntry, string, error) { - cfg, err := t.ConfigService.Config(true) + cfg, err := t.ConfigService.Config() if err != nil { return FlightRef{}, HubEntry{}, "", err } @@ -283,7 +283,7 @@ func (t *Tap) enforceFlightSnapshot(flight *Flight, k keg.Keg, want FlightRole) } } } - if cfg, cErr := t.ConfigService.Config(true); cErr == nil { + if cfg, cErr := t.ConfigService.Config(); cErr == nil { alias = cfg.LookupAliasForTarget(t.Runtime, k.Target().String()) } } diff --git a/pkg/tapper/tap_hub.go b/pkg/tapper/tap_hub.go index 7a6de3e9..517c7e9e 100644 --- a/pkg/tapper/tap_hub.go +++ b/pkg/tapper/tap_hub.go @@ -28,7 +28,7 @@ type HubListOptions struct { // unauthenticated hub is logged and skipped so one bad hub doesn't blank the // whole listing. func (t *Tap) HubListKegs(ctx context.Context, opts HubListOptions) ([]string, error) { - cfg, err := t.ConfigService.Config(true) + cfg, err := t.ConfigService.Config() if err != nil { return nil, err } @@ -197,13 +197,13 @@ type HubInfo struct { // are configured), marking the default and the config layer each came from. It // inspects local config only — it does not contact any hub. func (t *Tap) HubList(_ context.Context) ([]HubInfo, error) { - cfg, err := t.ConfigService.Config(true) + cfg, err := t.ConfigService.Config() if err != nil { return nil, err } defaultHub := cfg.resolveHubName() userHubs := map[string]struct{}{} - if userCfg, _ := t.ConfigService.UserConfig(true); userCfg != nil { + if userCfg, _ := t.ConfigService.UserConfig(); userCfg != nil { for name := range userCfg.Hubs() { userHubs[name] = struct{}{} } @@ -310,7 +310,7 @@ func (t *Tap) HubSetDefault(ctx context.Context, opts HubSetDefaultOptions) erro if name == "" { return fmt.Errorf("a hub name is required") } - cfg, err := t.ConfigService.Config(true) + cfg, err := t.ConfigService.Config() if err != nil { return err } @@ -355,7 +355,9 @@ func (t *Tap) mutateConfigFile(path string, fn func(*Config) error) error { if err := cfg.Write(t.Runtime, resolved); err != nil { return fmt.Errorf("unable to write config: %w", err) } - t.ConfigService.ResetCache() + // The snapshot predates this write; drop it so nothing in this process + // reads back a value we just replaced. + t.ConfigService.Reload() return nil } diff --git a/pkg/tapper/tap_info.go b/pkg/tapper/tap_info.go index 52d8df17..d4a980ef 100644 --- a/pkg/tapper/tap_info.go +++ b/pkg/tapper/tap_info.go @@ -147,7 +147,7 @@ func (t *Tap) kegSettingsBatch(ctx context.Context, opts KegSettingsOptions) (st return marshalMinimalKegSettings(refs, details) } - cfg, err := t.ConfigService.Config(true) + cfg, err := t.ConfigService.Config() if err != nil { return "", err } @@ -326,7 +326,7 @@ type resolvedIdentity struct { // corresponding fields blank rather than erroring. func (t *Tap) resolveIdentity(opts KegTargetOptions) resolvedIdentity { var id resolvedIdentity - cfg, err := t.ConfigService.Config(true) + cfg, err := t.ConfigService.Config() if err != nil || cfg == nil { return id } @@ -387,10 +387,10 @@ func (t *Tap) configFieldScope(field string) string { if configFieldGetter(t.loadEnvConfig(), field) != "" { return "env" } - if projectCfg, _ := t.ConfigService.ProjectConfig(true); configFieldGetter(projectCfg, field) != "" { + if projectCfg, _ := t.ConfigService.ProjectConfig(); configFieldGetter(projectCfg, field) != "" { return "project" } - if userCfg, _ := t.ConfigService.UserConfig(true); configFieldGetter(userCfg, field) != "" { + if userCfg, _ := t.ConfigService.UserConfig(); configFieldGetter(userCfg, field) != "" { return "user" } return "" diff --git a/pkg/tapper/tap_init.go b/pkg/tapper/tap_init.go index ad5bf014..6278ab11 100644 --- a/pkg/tapper/tap_init.go +++ b/pkg/tapper/tap_init.go @@ -93,7 +93,7 @@ func (t *Tap) InitKeg(ctx context.Context, options InitOptions) (*keg.Target, er return nil, ErrNotBootstrapped } - cfg, err := t.ConfigService.Config(true) + cfg, err := t.ConfigService.Config() if err != nil { return nil, fmt.Errorf("failed to load config: %w", err) } @@ -222,7 +222,7 @@ func (t *Tap) recordInitKeg(hubName, namespace string) error { if strings.TrimSpace(namespace) == "" || strings.TrimSpace(hubName) == "" { return nil } - userCfg, err := t.ConfigService.UserConfig(false) + userCfg, err := t.ConfigService.ReadUserConfigFile() if err != nil { if !errors.Is(err, keg.ErrNotExist) { return err @@ -235,7 +235,9 @@ func (t *Tap) recordInitKeg(hubName, namespace string) error { if err := userCfg.Write(t.Runtime, t.PathService.UserConfig()); err != nil { return err } - t.ConfigService.ResetCache() + // The snapshot predates this write; drop it so nothing in this process + // reads back a value we just replaced. + t.ConfigService.Reload() return nil } diff --git a/pkg/tapper/tap_keg.go b/pkg/tapper/tap_keg.go index ce2bef38..e3bf87dd 100644 --- a/pkg/tapper/tap_keg.go +++ b/pkg/tapper/tap_keg.go @@ -150,7 +150,7 @@ func (t *Tap) KegRename(ctx context.Context, opts KegRenameOptions) error { // namespace and returns the namespace, keg alias, hub URL, and bearer token. // Keg administration requires a remote hub-backed namespace with a token. func (t *Tap) resolveKegAdminRef(keg, nsOverride, hubOverride string) (namespace, alias, hubURL, token string, err error) { - cfg, cErr := t.ConfigService.Config(true) + cfg, cErr := t.ConfigService.Config() if cErr != nil { return "", "", "", "", cErr } diff --git a/pkg/tapper/tap_namespace.go b/pkg/tapper/tap_namespace.go index 8c7b6cfc..aae7ac90 100644 --- a/pkg/tapper/tap_namespace.go +++ b/pkg/tapper/tap_namespace.go @@ -156,7 +156,7 @@ func (t *Tap) NamespaceCreate(ctx context.Context, opts NamespaceCreateOptions) // resolveNamespaceHub resolves the remote hub + token backing a namespace for // namespace-scoped admin ops. An empty namespace resolves the default. func (t *Tap) resolveNamespaceHub(namespace, hubOverride string) (ns, hubURL, token string, err error) { - cfg, cErr := t.ConfigService.Config(true) + cfg, cErr := t.ConfigService.Config() if cErr != nil { return "", "", "", cErr } @@ -185,7 +185,7 @@ func (t *Tap) resolveNamespaceHub(namespace, hubOverride string) (ns, hubURL, to // resolveHubEndpoint resolves a hub (explicit name or the default) to a URL + // token for hub-level namespace ops (list/create). func (t *Tap) resolveHubEndpoint(hubOverride string) (hubURL, token string, err error) { - cfg, cErr := t.ConfigService.Config(true) + cfg, cErr := t.ConfigService.Config() if cErr != nil { return "", "", cErr } @@ -203,7 +203,7 @@ func (t *Tap) resolveHubEndpoint(hubOverride string) (hubURL, token string, err // resolveHubUIEndpoint resolves a hub to a browser base URL without requiring // a token. It is used for UI handoffs such as namespace creation. func (t *Tap) resolveHubUIEndpoint(hubOverride string) (hubName, hubURL string, err error) { - cfg, cErr := t.ConfigService.Config(true) + cfg, cErr := t.ConfigService.Config() if cErr != nil { return "", "", cErr } diff --git a/pkg/tapper/tap_orient.go b/pkg/tapper/tap_orient.go index e5f4911b..ad58779c 100644 --- a/pkg/tapper/tap_orient.go +++ b/pkg/tapper/tap_orient.go @@ -32,7 +32,15 @@ type OrientOptions struct { // flight and hub-listing failures do not suppress the core orientation // document. func (t *Tap) Orient(ctx context.Context, opts OrientOptions) (string, error) { - flightName := t.activeFlightName(opts.Flight) + // Orientation is the one reload boundary for configuration. Everywhere else + // reads a snapshot fixed for the life of the process, so this is where an + // edited config file takes effect. Unconditional, so an explicit --flight + // still gets a keg listing built from the same fresh cascade as the + // config-driven form. + if t != nil && t.ConfigService != nil { + t.ConfigService.Reload() + } + flightName := t.ActiveFlightName(opts.Flight) flight, flightNote := t.resolveOrientFlight(ctx, flightName) available, warnings := t.orientKegListing(ctx, flight) return BuildOrientationPayload(flight, flightNote, available, warnings) @@ -47,26 +55,25 @@ func (t *Tap) OrientationForFlight(ctx context.Context, flight *Flight) (string, return payload, available, warnings, err } -func (t *Tap) activeFlightName(explicit string) string { +// ActiveFlightName resolves an explicit flight, falling back to the flight in +// the process configuration snapshot. It is a pure read: it neither writes +// configuration nor reloads it, so callers that need a fresh cascade call +// ConfigService.Reload at their own boundary (see Orient and the MCP session +// gate). +func (t *Tap) ActiveFlightName(explicit string) string { if name := strings.TrimSpace(explicit); name != "" { return name } if t == nil || t.ConfigService == nil { return "" } - cfg, err := t.ConfigService.Config(true) + cfg, err := t.ConfigService.Config() if err != nil || cfg == nil { return "" } return strings.TrimSpace(cfg.Flight()) } -// ActiveFlightName resolves an explicit flight or the persistent project -// default without mutating configuration. -func (t *Tap) ActiveFlightName(explicit string) string { - return t.activeFlightName(explicit) -} - func (t *Tap) resolveOrientFlight(ctx context.Context, name string) (*Flight, string) { name = strings.TrimSpace(name) if name == "" || t == nil || t.FlightService == nil { @@ -96,7 +103,7 @@ func (t *Tap) orientKegListing(ctx context.Context, flight *Flight) ([]Orientati if t == nil || t.ConfigService == nil { return nil, []string{"KEG listing unavailable: no config service is configured."} } - cfg, err := t.ConfigService.Config(true) + cfg, err := t.ConfigService.Config() if err != nil { return nil, []string{fmt.Sprintf("KEG listing unavailable: %v", err)} } diff --git a/pkg/tapper/tap_orient_test.go b/pkg/tapper/tap_orient_test.go index bf537ae1..be90991c 100644 --- a/pkg/tapper/tap_orient_test.go +++ b/pkg/tapper/tap_orient_test.go @@ -142,6 +142,58 @@ hubs: require.NotContains(t, payload, "| `@local/personal`") } +// TestTap_OrientReloadsNearestProjectConfig covers the reload boundary. Orient +// owns the cache reset; ActiveFlightName is a pure read of whatever cascade is +// currently loaded, so a stale cache stays stale until Orient refreshes it. +func TestTap_OrientReloadsNearestProjectConfig(t *testing.T) { + t.Parallel() + sb := sandbox.NewSandbox(t, &sandbox.Options{Home: "/home/testuser", User: "testuser"}) + project := "/home/testuser/project" + descendant := filepath.Join(project, "src", "pkg") + require.NoError(t, sb.Setwd(descendant)) + + tap, err := tapper.NewTap(tapper.TapOptions{Root: descendant, Runtime: sb.Runtime()}) + require.NoError(t, err) + require.NoError(t, sb.Runtime().AtomicWriteFile(tap.PathService.UserConfig(), []byte(`flight: +baseline +fallbackNamespace: local +hubs: + home: + kind: local + basePath: /home/testuser/kegs +`), 0o644)) + + // Prime the merged cache before the project config exists. Orientation must + // still reload the cascade and adopt the nearest project selection. + cfg, err := tap.ConfigService.Config() + require.NoError(t, err) + require.Equal(t, "+baseline", cfg.Flight()) + require.NoError(t, sb.Runtime().AtomicWriteFile( + filepath.Join(project, ".tapper", "config.yaml"), + []byte("flight: +project\n"), 0o644)) + + for _, flight := range []struct{ slug, title string }{ + {"baseline", "Baseline"}, + {"project", "Project"}, + } { + require.NoError(t, sb.Runtime().AtomicWriteFile( + filepath.Join("/home/testuser/kegs/flights.d", flight.slug+".yaml"), + []byte("title: "+flight.title+"\ninstructions: "+flight.title+" instructions\n"), 0o644)) + } + + // The primed cache still answers with the user-level baseline, because a + // pure read must not silently reload behind the caller's back. + require.Equal(t, "+baseline", tap.ActiveFlightName("")) + + payload, err := tap.Orient(context.Background(), tapper.OrientOptions{}) + require.NoError(t, err) + require.Contains(t, payload, "+project") + require.Contains(t, payload, "Project instructions") + require.NotContains(t, payload, "Baseline instructions") + + // Orient reloaded the cascade, so the pure read now sees the project value. + require.Equal(t, "+project", tap.ActiveFlightName("")) +} + func TestTap_Orient_FullAccessStillSuppressesKegInstructions(t *testing.T) { t.Parallel() sb := sandbox.NewSandbox(t, &sandbox.Options{Home: "/home/testuser", User: "testuser"}) diff --git a/pkg/tapper/tap_use.go b/pkg/tapper/tap_use.go index 7b34db2a..010dde1a 100644 --- a/pkg/tapper/tap_use.go +++ b/pkg/tapper/tap_use.go @@ -102,7 +102,7 @@ func (t *Tap) UseStatus(_ context.Context, opts KegTargetOptions) (string, error } out := status{Resolved: t.resolveIdentity(opts)} - if cfg, err := t.ConfigService.Config(true); err == nil && cfg != nil { + if cfg, err := t.ConfigService.Config(); err == nil && cfg != nil { out.DefaultKeg = slot{Value: cfg.DefaultKeg(), Scope: t.configFieldScope("defaultKeg")} out.FallbackKeg = slot{Value: cfg.FallbackKeg(), Scope: t.configFieldScope("fallbackKeg")} out.Flight = slot{Value: cfg.Flight(), Scope: t.configFieldScope("flight")} diff --git a/pkg/tapper/tap_use_test.go b/pkg/tapper/tap_use_test.go index bcba9e2b..126d986a 100644 --- a/pkg/tapper/tap_use_test.go +++ b/pkg/tapper/tap_use_test.go @@ -29,7 +29,7 @@ func TestNamespaceInference_LocalBareName(t *testing.T) { require.NoError(t, fx.Runtime().AtomicWriteFile(tap.PathService.UserConfig(), []byte(localUserConfig), 0o644)) // Backend resolution: a bare name infers @local on the local hub. - cfg, err := tap.ConfigService.Config(true) + cfg, err := tap.ConfigService.Config() require.NoError(t, err) target, err := cfg.ResolveRef(fx.Runtime(), tapper.KegRef{Name: "private"}) require.NoError(t, err) From 3641b0fcdadef2e535dd2227a14e2ba180cace29 Mon Sep 17 00:00:00 2001 From: Jared Rickert Date: Tue, 4 Aug 2026 00:00:32 -0500 Subject: [PATCH 3/9] feat(cli): resolve the configured flight at orientation, not per command 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. --- pkg/cli/cmd_config_test.go | 59 +++++++++++++++++++++++++++++++++ pkg/cli/cmd_root.go | 9 ----- pkg/cli/cmd_root_flight_test.go | 35 +++++++++++++++++++ 3 files changed, 94 insertions(+), 9 deletions(-) create mode 100644 pkg/cli/cmd_root_flight_test.go diff --git a/pkg/cli/cmd_config_test.go b/pkg/cli/cmd_config_test.go index a720326e..d47d67d7 100644 --- a/pkg/cli/cmd_config_test.go +++ b/pkg/cli/cmd_config_test.go @@ -231,6 +231,65 @@ func TestConfigCommand_ExplainFlagWithEnvVar(t *testing.T) { require.Contains(t, stdout, "source: env vars") } +func TestConfigCommand_ProjectFlightPrecedenceMatchesOrient(t *testing.T) { + t.Parallel() + + sb := NewSandbox(t) + project := "/home/testuser/work/project" + descendant := project + "/src/pkg" + require.NoError(t, sb.Setwd(descendant)) + require.NoError(t, sb.Runtime().AtomicWriteFile( + "/home/testuser/.config/tapper/config.yaml", + []byte(`flight: +baseline +fallbackNamespace: local +hubs: + home: + kind: local + basePath: /home/testuser/kegs +`), 0o644)) + require.NoError(t, sb.Runtime().AtomicWriteFile( + project+"/.tapper/config.yaml", []byte("flight: +project\n"), 0o644)) + + for slug, instructions := range map[string]string{ + "baseline": "Baseline instructions", + "project": "Project instructions", + "environment": "Environment instructions", + "explicit": "Explicit instructions", + } { + require.NoError(t, sb.Runtime().AtomicWriteFile( + "/home/testuser/kegs/flights.d/"+slug+".yaml", + []byte("title: "+slug+"\ninstructions: "+instructions+"\n"), 0o644)) + } + + explained := NewProcess(t, false, "config", "--explain", "flight").Run(sb.Context(), sb.Runtime()) + require.NoError(t, explained.Err) + require.Contains(t, string(explained.Stdout), "flight = +project") + require.Contains(t, string(explained.Stdout), "source: project config") + + oriented := NewProcess(t, false, "orient").Run(sb.Context(), sb.Runtime()) + require.NoError(t, oriented.Err) + require.Contains(t, string(oriented.Stdout), "+project") + require.Contains(t, string(oriented.Stdout), "Project instructions") + require.NotContains(t, string(oriented.Stdout), "Baseline instructions") + + require.NoError(t, sb.Runtime().Env().Set("TAP_FLIGHT", "+environment")) + explained = NewProcess(t, false, "config", "--explain", "flight").Run(sb.Context(), sb.Runtime()) + require.NoError(t, explained.Err) + require.Contains(t, string(explained.Stdout), "flight = +environment") + require.Contains(t, string(explained.Stdout), "source: env vars") + + oriented = NewProcess(t, false, "orient").Run(sb.Context(), sb.Runtime()) + require.NoError(t, oriented.Err) + require.Contains(t, string(oriented.Stdout), "+environment") + require.Contains(t, string(oriented.Stdout), "Environment instructions") + + oriented = NewProcess(t, false, "--flight", "+explicit", "orient").Run(sb.Context(), sb.Runtime()) + require.NoError(t, oriented.Err) + require.Contains(t, string(oriented.Stdout), "+explicit") + require.Contains(t, string(oriented.Stdout), "Explicit instructions") + require.NotContains(t, string(oriented.Stdout), "Environment instructions") +} + func TestConfigCommand_ShowSourcesFlag(t *testing.T) { t.Parallel() diff --git a/pkg/cli/cmd_root.go b/pkg/cli/cmd_root.go index 7fa37f76..e6783268 100644 --- a/pkg/cli/cmd_root.go +++ b/pkg/cli/cmd_root.go @@ -175,15 +175,6 @@ func NewRootCmd(deps *Deps) *cobra.Command { deps.LogLevel = v } } - // A project's persisted flight auto-applies when --flight is not - // given, so orient and MCP can inherit the same flight context; - // --flight still overrides per invocation. Gated to the tap profile - // (the only one with the --flight flag). - if deps.Profile.withDefaults().AllowKegAliasFlags && !cmd.Flags().Changed("flight") { - if v := cfg.Flight(); v != "" { - deps.KegTargetOptions.Flight = v - } - } } if deps.ConfigPath != "" { diff --git a/pkg/cli/cmd_root_flight_test.go b/pkg/cli/cmd_root_flight_test.go new file mode 100644 index 00000000..1295605c --- /dev/null +++ b/pkg/cli/cmd_root_flight_test.go @@ -0,0 +1,35 @@ +package cli + +import ( + "bytes" + "testing" + + "github.com/stretchr/testify/require" +) + +func TestRootConfiguredFlightDoesNotBecomeExplicitDependency(t *testing.T) { + t.Parallel() + sb := newTestSandbox(t) + require.NoError(t, sb.Setwd("/home/testuser/project/child")) + require.NoError(t, sb.Runtime().AtomicWriteFile( + "/home/testuser/.config/tapper/config.yaml", + []byte("flight: +baseline\nfallbackNamespace: local\nhubs:\n home:\n kind: local\n basePath: /home/testuser/kegs\n"), 0o644)) + require.NoError(t, sb.Runtime().AtomicWriteFile( + "/home/testuser/project/.tapper/config.yaml", + []byte("flight: +project\n"), 0o644)) + require.NoError(t, sb.Runtime().AtomicWriteFile( + "/home/testuser/kegs/flights.d/project.yaml", + []byte("title: Project\ninstructions: Project instructions\n"), 0o644)) + + deps := &Deps{Profile: TapProfile(), Runtime: sb.Runtime()} + cmd := NewRootCmd(deps) + cmd.SetArgs([]string{"orient"}) + var stdout, stderr bytes.Buffer + cmd.SetOut(&stdout) + cmd.SetErr(&stderr) + + require.NoError(t, cmd.ExecuteContext(sb.Context()), stderr.String()) + require.Empty(t, deps.KegTargetOptions.Flight, + "configured selection must not occupy the explicit --flight dependency") + require.Contains(t, stdout.String(), "+project") +} From 64efd762e7e4dc6fc35685005608e5dfc3d16f8a Mon Sep 17 00:00:00 2001 From: Jared Rickert Date: Tue, 4 Aug 2026 00:01:01 -0500 Subject: [PATCH 4/9] fix(tapper): require editor authority when import_from_keg leaves stubs 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. --- pkg/tapper/tap_import.go | 6 +++++- pkg/tapper/tap_import_test.go | 30 ++++++++++++++++++++++++++++++ 2 files changed, 35 insertions(+), 1 deletion(-) diff --git a/pkg/tapper/tap_import.go b/pkg/tapper/tap_import.go index 45cfe5a0..b5b57d9e 100644 --- a/pkg/tapper/tap_import.go +++ b/pkg/tapper/tap_import.go @@ -50,7 +50,11 @@ func (t *Tap) ImportFromKeg(ctx context.Context, opts ImportFromKegOptions) ([]I } opts.Source.Keg = srcAlias - srcKeg, err := t.resolveKegForRole(ctx, opts.Source, FlightRoleViewer) + sourceRole := FlightRoleViewer + if opts.LeaveStubs { + sourceRole = FlightRoleEditor + } + srcKeg, err := t.resolveKegForRole(ctx, opts.Source, sourceRole) if err != nil { return nil, fmt.Errorf("unable to open source keg: %w", err) } diff --git a/pkg/tapper/tap_import_test.go b/pkg/tapper/tap_import_test.go index 23918445..4f035fb0 100644 --- a/pkg/tapper/tap_import_test.go +++ b/pkg/tapper/tap_import_test.go @@ -1,6 +1,8 @@ package tapper import ( + "context" + "errors" "testing" "github.com/jlrickert/tapper/pkg/keg" @@ -15,6 +17,34 @@ func TestResolveImportSourceAlias_BareIDs(t *testing.T) { require.Equal(t, []string{"1", "2", "3"}, bareIDs) } +func TestImportFromKeg_LeaveStubsRequiresEditorOnSource(t *testing.T) { + t.Parallel() + for _, tc := range []struct { + name string + leaveStubs bool + want FlightRole + }{ + {name: "copy only", want: FlightRoleViewer}, + {name: "leave stubs", leaveStubs: true, want: FlightRoleEditor}, + } { + t.Run(tc.name, func(t *testing.T) { + var got FlightRole + tap := &Tap{KegResolver: func(context.Context, KegTargetOptions, FlightRole) (keg.Keg, error) { + return nil, errors.New("unexpected resolver path") + }} + tap.KegResolver = func(_ context.Context, _ KegTargetOptions, role FlightRole) (keg.Keg, error) { + got = role + return nil, errors.New("stop after role capture") + } + _, err := tap.ImportFromKeg(context.Background(), ImportFromKegOptions{ + Source: KegTargetOptions{Keg: "source"}, LeaveStubs: tc.leaveStubs, + }) + require.Error(t, err) + require.Equal(t, tc.want, got) + }) + } +} + func TestResolveImportSourceAlias_KegRefArgs(t *testing.T) { t.Parallel() alias, bareIDs, err := resolveImportSourceAlias( From f61639d21a45b6a03a5f2562e892a521ef532237 Mon Sep 17 00:00:00 2001 From: Jared Rickert Date: Tue, 4 Aug 2026 00:02:54 -0500 Subject: [PATCH 5/9] refactor(mcp): build the tool surface from transport providers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- docs/ai-coding-agents/codex.md | 2 +- docs/ai-coding-agents/mcp-setup.md | 79 ++++-- docs/configuration/flights.md | 33 ++- pkg/cli/cmd_mcp.go | 5 +- pkg/mcp/providers.go | 168 ++++++++++++ pkg/mcp/server.go | 95 +++---- pkg/mcp/server_test.go | 265 +++++++++---------- pkg/mcp/session_flight.go | 145 ++++++----- pkg/mcp/session_flight_test.go | 28 +- pkg/mcp/session_transition_test.go | 401 +++++++++++++++++++++++++++++ pkg/mcp/tools_auth.go | 85 +++--- pkg/mcp/tools_doctor.go | 4 +- pkg/mcp/tools_files.go | 371 +++++++++++++++----------- pkg/mcp/tools_flight.go | 25 +- pkg/mcp/tools_keg.go | 70 ++--- pkg/parity/parity_auth_test.go | 61 ++--- pkg/parity/parity_coverage_test.go | 46 ++-- pkg/parity/parity_test.go | 6 +- 18 files changed, 1299 insertions(+), 590 deletions(-) create mode 100644 pkg/mcp/providers.go create mode 100644 pkg/mcp/session_transition_test.go diff --git a/docs/ai-coding-agents/codex.md b/docs/ai-coding-agents/codex.md index 20a11b6d..5c45622e 100644 --- a/docs/ai-coding-agents/codex.md +++ b/docs/ai-coding-agents/codex.md @@ -59,7 +59,7 @@ hidden controls. To change a config-driven session, run `tap use --flight existing thread. If no flight is selected, MCP still connects in recovery-only mode. Codex can -use `list_flights`, `flight_show`, `auth_status`, and `config`, while KEG tools +use `orient`, `list_flights`, `flight_show`, and credential-safe `auth_info`, while KEG tools remain locked. Ask the user to run `tap use --flight @namespace/+slug`, then call `mcp__tapper__orient` to restore the normal tool surface. If the MCP tools are unavailable, report the unavailable connection, ask the user to reconnect diff --git a/docs/ai-coding-agents/mcp-setup.md b/docs/ai-coding-agents/mcp-setup.md index 0c2fb9b0..f8459daf 100644 --- a/docs/ai-coding-agents/mcp-setup.md +++ b/docs/ai-coding-agents/mcp-setup.md @@ -1,11 +1,13 @@ # MCP Server Setup The `tap mcp` command starts a Model Context Protocol server on stdio, exposing -KEG operations as tools. The local full surface publishes flight authority at -initialization and on explicit orientation. Without one, the server starts in a -recovery-only state so the host can inspect flights safely. This page is the -advanced manual path for MCP hosts that are not using the bundled Claude Code -or Codex integrations. +the same agent-safe tools and resources as Tapper Hub's authenticated `/mcp` +endpoint — the one difference being attachment transfers, where `tap mcp` can +also read and write local paths because it runs on your machine. Both publish +immutable flight authority at initialization and on explicit orientation. +Without one, the server starts in a recovery-only state so the host can inspect +flights safely. This page is the advanced manual path for MCP hosts that are not +using the bundled Claude Code or Codex integrations. Most users should use the official one-command installs in the project README's [Connect AI Agents](../../README.md#connect-ai-agents) section. `tap integrate @@ -72,12 +74,12 @@ With a default keg: ## Available Tools -The MCP server exposes the same operating surface as the CLI. Exact tool -availability follows the installed Tapper version; inspect your MCP host's tool -list for the live surface. +The MCP server exposes a shared agent surface rather than every machine-local +CLI capability. Exact tool availability follows the installed Tapper version +and the active flight; inspect your MCP host's tool list for the live surface. When no flight is selected, the visible list is intentionally restricted to -`orient`, `list_flights`, `flight_show`, `auth_status`, and `config`. `orient` +`orient`, `list_flights`, `flight_show`, and `auth_info`. `orient` and any guessed KEG-tool call explain that KEG tools are locked and direct the agent to inspect flights through MCP, ask the user to run `tap use --flight @namespace/+slug`, and call `orient` again on the same connection. @@ -112,7 +114,7 @@ agent to inspect flights through MCP, ask the user to run `tap use --flight | Tool | Description | | --- | --- | | `index`, `list_indexes`, `index_cat` | Rebuild or inspect indexes | -| `doctor` | Check keg health | +| `doctor` | Check only the selected keg's health (not local Tapper configuration) | | `node_history`, `node_snapshot`, `node_snapshot_view`, `node_restore` | Manage node snapshots | | `lock_acquire`, `lock_release`, `lock_status`, `lock_force_release` | Coordinate cross-process node locks | @@ -123,31 +125,53 @@ agent to inspect flights through MCP, ask the user to run `tap use --flight | `list_files`, `upload_file`, `download_file`, `delete_file` | Manage file attachments | | `list_images`, `upload_image`, `download_image`, `delete_image` | Manage image attachments | -### Organization And Keg Administration +The transfer tools come in two variants, chosen by whether the server shares a +filesystem with the agent driving it. + +Local `tap mcp` runs on your machine, so a path names the same file on both +sides. It publishes the full round-trip: `upload_file` and `upload_image` accept +`source_path` and `file:` URIs alongside `data_base64`, data URIs, and embedded +resources; `download_file` writes to `dest_path`; and `download_image` takes an +optional `dest_path`, returning the image as MCP content when you omit it. + +Hosted `/mcp` shares no filesystem with the agent, so a path there would name +the server's own disk. Its uploads accept only embedded resources, data URIs, +and base64 bytes; `download_image` always returns MCP image content; and +`download_file` is not registered. Those fields are absent from the published +schema rather than refused at call time, so a hosted agent never has the +vocabulary to ask. + +### Discovery And Identity | Tool | Description | | --- | --- | -| `keg_list` | List visible kegs on a hub | -| `keg_visibility` | Set keg visibility | -| `namespace_list` | Inspect namespaces | -| `auth_status` | Inspect authentication state | +| `keg_list` | List identity-authorized kegs filtered through the active flight | +| `auth_info` | Return structured credential-safe `identities[]` and flight-filtered `kegs[]` | -User and role management tools are intentionally not exposed over MCP for now; -manage namespace members and keg grants through the hub UI. +Each identity includes only its hub locator, user ID, username, display name, +default namespace, and namespace names. Tokens, email, scopes, cookies, expiry, +and session data are never returned. Local MCP reports every configured +authenticated Hub identity; hosted MCP reports its single authenticated user. ### Automation And Setup | Tool | Description | | --- | --- | -| `repo_init` | Initialize a keg destination | -| `config`, `config_template` | Read config or starter templates | | `import_from_keg` | Import nodes from another keg | -| `export`, `import` | Export or import keg archives | | `graph` | Render a keg graph | | `orient` | Return the shared KEG system orientation payload | | `list_flights`, `flight_show` | Discover and inspect visible flights | -| `flight_create`, `flight_edit`, `flight_delete` | Manage other flights when the active flight grants `manage_flights` and the identity owns/administers the target namespace | -| `license` | Read bundled license text | +| `flight_create`, `flight_edit`, `flight_delete` | Manage Hub-backed flights when the active flight grants `manage_flights` and the identity owns/administers the target namespace | + +MCP does not expose Tapper configuration, config templates, repository setup, +archive import/export, raw auth status, license text, keg visibility, or +namespace administration. Those remain external CLI, configuration, or Hub UI +operations. + +`import_from_keg` requires editor identity and flight authority on the source +when `leave_stubs` is requested, because that option rewrites source nodes. +Both transports also publish `tapper://orient` and the +`tapper://node/{node_id}{?keg}` resource template. ## Keg Targeting @@ -167,6 +191,11 @@ server-owned session state. Config-driven servers adopt configuration changes only through explicit orientation; `tap mcp --flight` stays bound to that identity for its process lifetime. +Hosted `/mcp` instead selects the authenticated account's global MCP flight +preference. A successful self-edit adopts the exact returned flight immediately; +a self-delete enters recovery immediately. Mutation tools disappear as soon as +the adopted flight no longer grants `manage_flights`. + ## Troubleshooting ### Server Not Responding @@ -189,8 +218,10 @@ tap use --flight @acme/+release-42 tap mcp --flight @acme/+release-42 ``` -After `tap use`, call `orient` on the existing session. A failed refresh keeps -the last valid authority; a blank selection intentionally enters recovery mode. +After `tap use`, call `orient` on the existing session. A failed ordinary +refresh keeps the last valid authority; a blank selection intentionally enters +recovery mode. If local configuration still names a flight deleted through MCP, +`orient` reports the stale external reference until configuration is changed. ### Logs diff --git a/docs/configuration/flights.md b/docs/configuration/flights.md index a206a0c0..056dc663 100644 --- a/docs/configuration/flights.md +++ b/docs/configuration/flights.md @@ -1,7 +1,7 @@ # Flights -A **flight** is the required authorization and instruction context for a local -full MCP session. Flight manifests live separately from Tapper configuration. +A **flight** is the required authorization and instruction context for an MCP +session. Flight manifests live separately from Tapper configuration. `tap bootstrap` can persist a machine-wide baseline in the user config, while a project can persist a more specific selection in `.tapper/config.yaml`. The server resolves fresh orientation during MCP initialization and again whenever @@ -104,8 +104,17 @@ modeline are ignored when deciding whether the manifest changed. MCP always exposes `list_flights` and `flight_show`. It exposes `flight_create`, `flight_edit`, and `flight_delete` only while the session's active flight grants `manage_flights`; direct calls are checked server-side as -well. A session can never edit or delete its own active flight. `flight_edit` -is a partial update where omitted fields retain their current values. +well. `flight_edit` is a partial update where omitted fields retain their +current values. A Hub-backed active flight may edit or delete itself: + +- a successful self-edit immediately adopts the exact returned manifest, + cover, instructions, and capabilities before the response is released; +- removing `manage_flights` therefore removes the mutation tools immediately; +- a successful self-delete immediately enters recovery-only mode; +- editing or deleting another flight does not change current session authority. + +Local `flights.d` manifests remain MCP read-only. Flight mutations always use +normal Hub authorization in addition to the active flight capability. ## Behavior @@ -118,9 +127,9 @@ is a partial update where omitted fields retain their current values. - `full_access` permits admin-class flight operations outside the cover, but does not bypass normal identity authorization or implicitly grant `manage_flights`. -- Without a selected flight, the local MCP server starts in recovery-only mode - and lists only `orient`, `list_flights`, `flight_show`, `auth_status`, and - `config`. After selecting a flight, call `orient` on the same connection. +- Without a selected flight, MCP starts in recovery-only mode and lists only + `orient`, `list_flights`, `flight_show`, and credential-safe `auth_info`. + After selecting a flight outside MCP, call `orient` on the same connection. - Config-driven `tap mcp` reloads user, project, and environment configuration on every orientation. A successful orientation atomically replaces session authority; configuration changes alone do nothing. @@ -129,6 +138,16 @@ is a partial update where omitted fields retain their current values. manifest, cover, and instructions. - A failed refresh preserves the last valid authority. An intentionally blank config selection clears authority and enters recovery mode. +- If a self-edit is persisted but exact orientation rendering fails, the tool + reports that the update was applied and enters recovery instead of retaining + stale authority. +- Hosted `/mcp` selects the account-wide MCP flight preference. Local + initialization and `orient` select explicit `--flight`, then `TAP_FLIGHT`, + the nearest project config, and finally the user baseline. +- Hosted self-deletion clears the account preference through the flight foreign + key. A local config that still names a deleted flight remains a stale external + reference: later `orient` reports it and the session stays in recovery until + configuration is changed outside MCP. - In-flight calls finish under the context captured when they began. Calls that start after orientation use the newly published context. - Direct CLI commands such as `tap cat`, `tap edit`, and `tap create` ignore diff --git a/pkg/cli/cmd_mcp.go b/pkg/cli/cmd_mcp.go index 2f6c5026..751201c9 100644 --- a/pkg/cli/cmd_mcp.go +++ b/pkg/cli/cmd_mcp.go @@ -52,9 +52,8 @@ per-command permission prompts.`, defaults.Flight = "" } srv := mcp.NewServer(deps.Tap, Version, defaults, mcp.ServerOptions{ - LicenseText: LicenseText, - Logger: rt.Logger(), - Reporter: deps.InvocationReporter, + Logger: rt.Logger(), + Reporter: deps.InvocationReporter, }) err = srv.Run(cmd.Context(), &sdkmcp.StdioTransport{}) if err != nil && errors.Is(err, io.EOF) { diff --git a/pkg/mcp/providers.go b/pkg/mcp/providers.go new file mode 100644 index 00000000..d9c4790b --- /dev/null +++ b/pkg/mcp/providers.go @@ -0,0 +1,168 @@ +package mcp + +import ( + "context" + "errors" + "sort" + "strings" + + "github.com/jlrickert/tapper/pkg/tapper" +) + +// Orientation is one complete, immutable MCP authority candidate. +type Orientation struct { + Flight *tapper.Flight + Payload string + Kegs []tapper.OrientationKeg + Warnings []string +} + +// OrientationProvider owns transport-specific flight selection and rendering. +type OrientationProvider interface { + // Load selects the active flight and renders it into a complete candidate. + // It is the transport's reload boundary: whatever "which flight am I on" + // depends on is re-read here and nowhere else. + Load(context.Context) (*Orientation, error) + // Render renders the exact supplied manifest. It must not consult a mutable + // selection again, because its caller has already decided which flight is + // authoritative and is only asking for the payload. + Render(context.Context, *tapper.Flight) (*Orientation, error) +} + +// FlightProvider supplies identity-authorized flight discovery and mutation. +// Session capability checks are enforced independently by the MCP gate, so +// implementations apply only their own transport's authorization. +type FlightProvider interface { + // ListFlights returns the canonical refs of every flight this identity can see. + ListFlights(context.Context) ([]string, error) + // GetFlight resolves one flight by ref. + GetFlight(context.Context, string) (*tapper.Flight, error) + // CreateFlight persists a new flight and returns the stored manifest. + CreateFlight(context.Context, tapper.CreateFlightOptions) (*tapper.Flight, error) + // UpdateFlight applies a partial edit and returns the stored manifest. The + // return value is authoritative: a session editing its own flight adopts + // exactly these bytes. + UpdateFlight(context.Context, tapper.UpdateFlightOptions) (*tapper.Flight, error) + // DeleteFlight removes a flight. + DeleteFlight(context.Context, tapper.DeleteFlightOptions) error +} + +// KegDiscoveryProvider reports the kegs an identity can reach. +type KegDiscoveryProvider interface { + // ListKegs returns every identity-authorized canonical keg ref. MCP applies + // the immutable active-flight cover before releasing results, so + // implementations do not filter by flight themselves. + ListKegs(context.Context) ([]string, error) +} + +// AuthIdentity is deliberately credential-free. Do not add token, email, +// scope, cookie, expiry, or session fields to this MCP wire shape. +type AuthIdentity struct { + Hub string `json:"hub"` + UserID int64 `json:"user_id"` + Username string `json:"username"` + DisplayName string `json:"display_name,omitempty"` + DefaultNamespace string `json:"default_namespace"` + Namespaces []string `json:"namespaces"` +} + +// IdentityProvider reports who the session is authenticated as. +type IdentityProvider interface { + // Identities returns the authenticated identities, without credentials. + // Local MCP reports every configured hub login; hosted MCP reports the one + // authenticated account. + Identities(context.Context) ([]AuthIdentity, error) +} + +type localOrientationProvider struct { + tap *tapper.Tap + staticFlight string +} + +func (p *localOrientationProvider) Load(ctx context.Context) (*Orientation, error) { + if p.tap == nil || p.tap.ConfigService == nil || p.tap.FlightService == nil { + return nil, errors.New("Tapper flight service is unavailable") + } + // Adoption is the reload boundary for both session kinds: config-driven + // sessions re-resolve their selection here, and launcher-bound sessions keep + // their immutable --flight but still need fresh hub routing and credentials. + // Configuration is otherwise fixed for the life of the process, so this is + // where an edit made outside the session takes effect. + p.tap.ConfigService.Reload() + ref := strings.TrimSpace(p.staticFlight) + if ref == "" { + ref = p.tap.ActiveFlightName("") + } + if strings.TrimSpace(ref) == "" { + payload, err := tapper.BuildOrientationPayload(nil, "", nil, nil) + return &Orientation{Payload: payload}, err + } + flight, err := p.tap.FlightService.GetFlightFresh(ctx, ref) + if err != nil { + return nil, err + } + return p.Render(ctx, flight) +} + +func (p *localOrientationProvider) Render(ctx context.Context, flight *tapper.Flight) (*Orientation, error) { + payload, kegs, warnings, err := p.tap.OrientationForFlight(ctx, flight) + if err != nil { + return nil, err + } + return &Orientation{Flight: flight, Payload: payload, Kegs: kegs, Warnings: warnings}, nil +} + +type localFlightProvider struct{ tap *tapper.Tap } + +func (p localFlightProvider) ListFlights(ctx context.Context) ([]string, error) { + return p.tap.ListFlights(ctx, tapper.ListFlightsOptions{}) +} +func (p localFlightProvider) GetFlight(ctx context.Context, ref string) (*tapper.Flight, error) { + return p.tap.GetFlight(ctx, tapper.GetFlightOptions{Name: ref}) +} +func (p localFlightProvider) CreateFlight(ctx context.Context, opts tapper.CreateFlightOptions) (*tapper.Flight, error) { + return p.tap.CreateFlight(ctx, opts) +} +func (p localFlightProvider) UpdateFlight(ctx context.Context, opts tapper.UpdateFlightOptions) (*tapper.Flight, error) { + return p.tap.UpdateFlight(ctx, opts) +} +func (p localFlightProvider) DeleteFlight(ctx context.Context, opts tapper.DeleteFlightOptions) error { + return p.tap.DeleteFlight(ctx, opts) +} + +type localKegDiscoveryProvider struct{ tap *tapper.Tap } + +func (p localKegDiscoveryProvider) ListKegs(ctx context.Context) ([]string, error) { + return p.tap.HubListKegs(ctx, tapper.HubListOptions{}) +} + +type localIdentityProvider struct{ tap *tapper.Tap } + +func (p localIdentityProvider) Identities(ctx context.Context) ([]AuthIdentity, error) { + if p.tap == nil || p.tap.PathService == nil || p.tap.Runtime == nil { + return nil, errors.New("Tapper authentication service is unavailable") + } + store, err := tapper.LoadAuthStore(ctx, p.tap.Runtime, p.tap.PathService.AuthStorePath()) + if err != nil { + return nil, err + } + var out []AuthIdentity + for _, hub := range store.Hubs() { + entry, ok := store.Get(hub) + if !ok || strings.TrimSpace(entry.AccessToken) == "" || p.tap.AuthValidateFn == nil { + continue + } + who, err := p.tap.AuthValidateFn(ctx, p.tap.Runtime, hub, entry.AccessToken) + if err != nil || who == nil { + continue + } + namespaces := append([]string(nil), who.Namespaces...) + sort.Strings(namespaces) + out = append(out, AuthIdentity{ + Hub: hub, UserID: who.UserID, Username: who.Username, + DisplayName: who.DisplayName, DefaultNamespace: who.DefaultNamespace, + Namespaces: namespaces, + }) + } + return out, nil +} diff --git a/pkg/mcp/server.go b/pkg/mcp/server.go index 785f2618..f3d7b47d 100644 --- a/pkg/mcp/server.go +++ b/pkg/mcp/server.go @@ -18,42 +18,28 @@ type KegDefaults struct { gate *sessionFlightGate } -// Surface selects which tool groups NewServer registers. -type Surface int - -const ( - // SurfaceFull registers every tool — the CLI peer surface and the default. - SurfaceFull Surface = iota - // SurfaceHub registers only the per-user node read/write tools appropriate - // for a remote, OAuth-scoped hub connector (tapper-hub's /mcp endpoint). It - // omits CLI/local tools (auth_status, config, doctor, license, - // file downloads, path-based image downloads, archive, locks) and hub-admin - // tools (keg/namespace administration, flights) — none of which make sense - // for a remote multi-tenant connector. Uploads remain available, but local - // path sources are disabled. Image downloads return MCP image content rather - // than writing to the server filesystem. Schema tools (schema_list/read/ - // create/edit/delete, validate) ARE registered here: schema mutation resolves - // at editor role, consistent with how node writes already work on this - // surface. The hub pairs this with Tap.KegResolver so the registered tools - // resolve against the caller's catalog. - SurfaceHub -) - // ServerOptions holds configuration for creating an MCP server. type ServerOptions struct { - LicenseText string // Logger is the structured logger for invocation logging. When nil, // invocation logging is silently skipped. Logger *slog.Logger // Reporter receives privacy-minimized tool invocation telemetry. It is // independent of Logger and may be nil. Reporter tapper.InvocationReporter - // Surface selects the registered tool set. The zero value (SurfaceFull) - // registers everything, preserving the CLI peer surface. - Surface Surface - // OrientationLoader supplies hosted/session-specific flight snapshots. - // Local full-surface servers leave it nil and use Tapper configuration. - OrientationLoader OrientationLoader + // Providers replace transport-specific registration branches. Nil providers + // use local adapters over tap; hosted callers inject authenticated catalog + // and account implementations. + OrientationProvider OrientationProvider + FlightProvider FlightProvider + KegProvider KegDiscoveryProvider + IdentityProvider IdentityProvider + // SharedFilesystem reports that this server and the agent host driving it + // see the same filesystem. That holds for stdio (`tap mcp`), where a path in + // a tool argument names the same file on both sides, and never for a hosted + // endpoint, where it would name the server's own disk. It selects the + // attachment transfer tools: see registerFileTools. The zero value is the + // safe one, so a caller that forgets it gets the hosted surface. + SharedFilesystem bool } // NewServer builds an MCP server with all registered tools. @@ -62,10 +48,19 @@ func NewServer(tap *tapper.Tap, version string, defaults KegDefaults, opts ...Se if len(opts) > 0 { opt = opts[0] } - staticFlight := defaults.Flight - if opt.Surface != SurfaceHub || opt.OrientationLoader != nil { - defaults.gate = newSessionFlightGate(tap, staticFlight, opt.OrientationLoader) + if opt.OrientationProvider == nil { + opt.OrientationProvider = &localOrientationProvider{tap: tap, staticFlight: defaults.Flight} + } + if opt.FlightProvider == nil { + opt.FlightProvider = localFlightProvider{tap: tap} + } + if opt.KegProvider == nil { + opt.KegProvider = localKegDiscoveryProvider{tap: tap} } + if opt.IdentityProvider == nil { + opt.IdentityProvider = localIdentityProvider{tap: tap} + } + defaults.gate = newSessionFlightGate(opt.OrientationProvider) var srv *sdkmcp.Server nodeSubs := newNodeResourceSubscriptions(tap, defaults, func(ctx context.Context, uri string) { @@ -85,7 +80,7 @@ func NewServer(tap *tapper.Tap, version string, defaults KegDefaults, opts ...Se srv.AddReceivingMiddleware(defaults.gate.middleware) } - // Node read/write tools — registered on every surface. These all funnel + // Node read/write tools. These all funnel // through Tap.resolveKegForRole, so a hub-injected KegResolver scopes them // to the caller's catalog with viewer/editor enforcement. registerReadTools(srv, tap, defaults) @@ -95,34 +90,14 @@ func NewServer(tap *tapper.Tap, version string, defaults KegDefaults, opts ...Se registerGraphTools(srv, tap, defaults) registerOrientTools(srv, tap, defaults) registerSchemaTools(srv, tap, defaults) - fileOpts := fileToolOptions{ - AllowLocalSources: opt.Surface != SurfaceHub, - DownloadFiles: opt.Surface != SurfaceHub, - ImageDownloads: imageDownloadLocalPath, - } - if opt.Surface == SurfaceHub { - fileOpts.ImageDownloads = imageDownloadContent - } - registerFileTools(srv, tap, defaults, fileOpts) - - // CLI/local and hub-admin tools — omitted on the remote hub connector - // surface (see Surface docs). They depend on the local CLI environment - // (config cascade, AuthStore, local filesystem writes) or perform - // multi-tenant administration, neither of which belongs on a per-user remote - // connector. - if opt.Surface != SurfaceHub { - registerDoctorTools(srv, tap, defaults) - registerLockTools(srv, tap, defaults) - registerRepoTools(srv, tap, defaults) - registerImportTools(srv, tap, defaults) - registerArchiveTools(srv, tap, defaults) - registerFlightTools(srv, tap, defaults) - registerKegTools(srv, tap, defaults) - registerNamespaceTools(srv, tap, defaults) - registerResourceTools(srv, tap, defaults) - registerAuthTools(srv, tap) - registerLicenseTools(srv, opt.LicenseText) - } + registerFileTools(srv, tap, defaults, opt.SharedFilesystem) + registerDoctorTools(srv, tap, defaults) + registerLockTools(srv, tap, defaults) + registerImportTools(srv, tap, defaults) + registerFlightTools(srv, defaults, opt.FlightProvider) + registerKegTools(srv, defaults, opt.KegProvider) + registerResourceTools(srv, tap, defaults) + registerAuthInfoTool(srv, defaults, opt.IdentityProvider, opt.KegProvider) if opt.Logger != nil || opt.Reporter != nil { var clk clock.Clock diff --git a/pkg/mcp/server_test.go b/pkg/mcp/server_test.go index 3e8e1198..082f9c65 100644 --- a/pkg/mcp/server_test.go +++ b/pkg/mcp/server_test.go @@ -85,7 +85,15 @@ func newTestSession(t *testing.T) (*sdkmcp.ClientSession, context.Context) { return session, ctx } -func newTestSessionWithRuntime(t *testing.T) (*sdkmcp.ClientSession, *toolkit.Runtime, context.Context) { +// newLocalTestSessionWithRuntime builds the `tap mcp` surface: a server that +// shares a filesystem with its host, so the local-path attachment tools are +// registered. Use it for anything exercising source_path or dest_path. +func newLocalTestSessionWithRuntime(t *testing.T) (*sdkmcp.ClientSession, *toolkit.Runtime, context.Context) { + t.Helper() + return newTestSessionWithRuntime(t, mcp.ServerOptions{SharedFilesystem: true}) +} + +func newTestSessionWithRuntime(t *testing.T, opts ...mcp.ServerOptions) (*sdkmcp.ClientSession, *toolkit.Runtime, context.Context) { t.Helper() ctx := context.Background() @@ -97,7 +105,7 @@ func newTestSessionWithRuntime(t *testing.T) (*sdkmcp.ClientSession, *toolkit.Ru }) require.NoError(t, err) - srv := mcp.NewServer(tap, "test", mcp.KegDefaults{KegTargetOptions: tapper.KegTargetOptions{Flight: "@local/+test"}}) + srv := mcp.NewServer(tap, "test", mcp.KegDefaults{KegTargetOptions: tapper.KegTargetOptions{Flight: "@local/+test"}}, opts...) serverTransport, clientTransport := sdkmcp.NewInMemoryTransports() // Connect server in background. @@ -130,86 +138,41 @@ func TestMCP_ToolsList(t *testing.T) { require.NoError(t, err) require.NotEmpty(t, res.Tools) - names := make([]string, len(res.Tools)) - for i, tool := range res.Tools { - names[i] = tool.Name + names := make(map[string]bool, len(res.Tools)) + for _, tool := range res.Tools { + names[tool.Name] = true + } + for _, want := range []string{ + "auth_info", "keg_list", "cat", "list", "grep", "tags", "backlinks", "links", "info", + "keg_settings", "keg_settings_edit", "stats", "create", "edit", "meta", "remove", "move", + "index", "list_indexes", "index_cat", "doctor", "node_history", "node_snapshot", + "node_snapshot_view", "node_restore", "list_files", "list_images", "delete_file", "delete_image", + "upload_file", "upload_image", "download_image", "graph", "orient", "import_from_keg", + "lock_acquire", "lock_release", "lock_status", "lock_force_release", "list_flights", "flight_show", + "flight_create", "flight_edit", "flight_delete", "schema_list", "schema_read", "schema_create", + "schema_edit", "schema_delete", "validate", + } { + require.Truef(t, names[want], "agent-safe surface missing %q", want) + } + for _, banned := range []string{ + "config", "config_template", "repo_init", "export", "import", "auth_status", "license", + "download_file", "keg_visibility", "namespace_list", "namespace_create", + } { + require.Falsef(t, names[banned], "agent-safe surface exposed %q", banned) + } + for _, tool := range res.Tools { + schema, err := json.Marshal(tool.InputSchema) + require.NoError(t, err) + require.NotContains(t, string(schema), "source_path", "tool %q exposes a machine-local upload path", tool.Name) + require.NotContains(t, string(schema), "dest_path", "tool %q exposes a machine-local download destination", tool.Name) } +} - require.Contains(t, names, "cat") - require.Contains(t, names, "list") - require.Contains(t, names, "grep") - require.Contains(t, names, "tags") - require.Contains(t, names, "backlinks") - require.Contains(t, names, "links") - require.Contains(t, names, "info") - require.Contains(t, names, "keg_settings") - require.Contains(t, names, "keg_settings_edit") - require.Contains(t, names, "keg_list") - require.Contains(t, names, "keg_visibility") - require.Contains(t, names, "stats") - require.Contains(t, names, "create") - require.Contains(t, names, "edit") - require.Contains(t, names, "meta") - require.Contains(t, names, "remove") - require.Contains(t, names, "move") - require.Contains(t, names, "index") - require.Contains(t, names, "list_indexes") - require.Contains(t, names, "index_cat") - require.Contains(t, names, "doctor") - require.Contains(t, names, "node_history") - require.Contains(t, names, "node_snapshot") - require.Contains(t, names, "node_snapshot_view") - require.Contains(t, names, "node_restore") - require.Contains(t, names, "list_files") - require.Contains(t, names, "list_images") - require.Contains(t, names, "delete_file") - require.Contains(t, names, "delete_image") - require.Contains(t, names, "lock_acquire") - require.Contains(t, names, "lock_release") - require.Contains(t, names, "lock_status") - require.Contains(t, names, "lock_force_release") - require.Contains(t, names, "license") - - require.Contains(t, names, "repo_init") - require.Contains(t, names, "config") - require.Contains(t, names, "config_template") - require.Contains(t, names, "import_from_keg") - require.Contains(t, names, "export") - require.Contains(t, names, "import") - require.Contains(t, names, "upload_file") - require.Contains(t, names, "download_file") - require.Contains(t, names, "upload_image") - require.Contains(t, names, "download_image") - require.Contains(t, names, "graph") - require.Contains(t, names, "orient") - require.NotContains(t, names, "integrate") - require.Contains(t, names, "list_flights") - require.Contains(t, names, "flight_show") - require.Contains(t, names, "flight_create") - require.Contains(t, names, "flight_edit") - require.Contains(t, names, "flight_delete") - require.Contains(t, names, "namespace_list") - require.Contains(t, names, "auth_status") - require.NotContains(t, names, "keg_grants") - require.NotContains(t, names, "keg_grant") - require.NotContains(t, names, "keg_revoke") - require.NotContains(t, names, "namespace_members") - require.NotContains(t, names, "namespace_add_member") - require.NotContains(t, names, "namespace_set_role") - require.NotContains(t, names, "namespace_remove_member") - require.NotContains(t, names, "namespace_create") - require.NotContains(t, names, "flight_update") -} - -// TestMCP_SurfaceHub_CuratesToolset pins the remote hub connector surface: it -// exposes the per-user node read/write tools and upload tools, and omits every -// CLI/local and hub-admin tool (auth_status, config, doctor, local file -// downloads, path-based image downloads, locks, archive, keg/namespace -// administration, flights, license). -// tapper-hub mounts this surface at /mcp paired with Tap.KegResolver. -func TestMCP_SurfaceHub_CuratesToolset(t *testing.T) { - t.Parallel() - session, ctx := newTestSessionWithOpts(t, mcp.ServerOptions{Surface: mcp.SurfaceHub}) +// TestMCP_CommonAgentSafeSurface pins the single common surface. Hosted +// callers inject providers rather than selecting a different registration set. +func TestMCP_CommonAgentSafeSurface(t *testing.T) { + t.Parallel() + session, ctx := newTestSessionWithOpts(t) res, err := session.ListTools(ctx, nil) require.NoError(t, err) @@ -220,7 +183,7 @@ func TestMCP_SurfaceHub_CuratesToolset(t *testing.T) { names[tool.Name] = true } - // Node read/write tools must be present. + // Common KEG and account tools must be present. for _, want := range []string{ "cat", "list", "grep", "tags", "backlinks", "links", "info", "keg_settings", "keg_settings_edit", @@ -230,24 +193,22 @@ func TestMCP_SurfaceHub_CuratesToolset(t *testing.T) { "list_files", "list_images", "delete_file", "delete_image", "upload_file", "upload_image", "download_image", "schema_list", "schema_read", "schema_create", "schema_edit", - "schema_delete", "validate", + "schema_delete", "validate", "doctor", "import_from_keg", "keg_list", "auth_info", + "lock_acquire", "lock_release", "lock_status", "lock_force_release", + "list_flights", "flight_show", "flight_create", "flight_edit", "flight_delete", } { - require.Truef(t, names[want], "SurfaceHub should expose %q", want) + require.Truef(t, names[want], "common surface should expose %q", want) } - // CLI/local and hub-admin tools must be absent. + // Machine-local and tenant-administration tools must be absent. for _, banned := range []string{ - "auth_status", "config", "config_template", "doctor", "license", - "repo_init", "integrate", "export", "import", "import_from_keg", - "download_file", - "lock_acquire", "lock_release", "lock_status", "lock_force_release", - "keg_list", "keg_grants", "keg_grant", "keg_revoke", "keg_visibility", + "auth_status", "config", "config_template", "license", "repo_init", "integrate", "export", "import", "download_file", + "keg_grants", "keg_grant", "keg_revoke", "keg_visibility", "namespace_list", "namespace_members", "namespace_add_member", "namespace_set_role", "namespace_remove_member", "namespace_create", - "list_flights", "flight_show", "flight_create", "flight_edit", "flight_update", - "flight_delete", + "flight_update", } { - require.Falsef(t, names[banned], "SurfaceHub must not expose %q", banned) + require.Falsef(t, names[banned], "common surface must not expose %q", banned) } } @@ -1083,23 +1044,16 @@ func TestMCP_LockReleaseTokenMismatch(t *testing.T) { func TestMCP_License(t *testing.T) { t.Parallel() - licenseText := "Apache License\nVersion 2.0, January 2004\nFull license content here." - session, ctx := newTestSessionWithOpts(t, mcp.ServerOptions{ - LicenseText: licenseText, - }) - - res, err := session.CallTool(ctx, &sdkmcp.CallToolParams{ - Name: "license", - Arguments: map[string]any{}, - }) + session, ctx := newTestSessionWithOpts(t) + res, err := session.ListTools(ctx, nil) require.NoError(t, err) - text := extractText(t, res) - require.False(t, res.IsError, "license returned error: %s", text) - require.Contains(t, text, "Apache License") - require.Contains(t, text, "Version 2.0") + for _, tool := range res.Tools { + require.NotEqual(t, "license", tool.Name) + } } func TestMCP_License_Empty(t *testing.T) { + t.Skip("license is not part of the agent-safe MCP surface") t.Parallel() session, ctx := newTestSession(t) @@ -1127,12 +1081,13 @@ func TestMCP_ToolsList_IncludesNewTools(t *testing.T) { names[i] = tool.Name } - require.Contains(t, names, "repo_init") - require.Contains(t, names, "config") - require.Contains(t, names, "config_template") + require.NotContains(t, names, "repo_init") + require.NotContains(t, names, "config") + require.NotContains(t, names, "config_template") } func TestMCP_Config(t *testing.T) { + t.Skip("config is not part of the agent-safe MCP surface") t.Parallel() session, ctx := newTestSession(t) @@ -1147,6 +1102,7 @@ func TestMCP_Config(t *testing.T) { } func TestMCP_ConfigUser(t *testing.T) { + t.Skip("config is not part of the agent-safe MCP surface") t.Parallel() session, ctx := newTestSession(t) @@ -1163,6 +1119,7 @@ func TestMCP_ConfigUser(t *testing.T) { } func TestMCP_ConfigInvalidScope(t *testing.T) { + t.Skip("config is not part of the agent-safe MCP surface") t.Parallel() session, ctx := newTestSession(t) @@ -1177,6 +1134,7 @@ func TestMCP_ConfigInvalidScope(t *testing.T) { } func TestMCP_ConfigTemplate(t *testing.T) { + t.Skip("config_template is not part of the agent-safe MCP surface") t.Parallel() session, ctx := newTestSession(t) @@ -1191,6 +1149,7 @@ func TestMCP_ConfigTemplate(t *testing.T) { } func TestMCP_ConfigTemplateProject(t *testing.T) { + t.Skip("config_template is not part of the agent-safe MCP surface") t.Parallel() session, ctx := newTestSession(t) @@ -1207,6 +1166,7 @@ func TestMCP_ConfigTemplateProject(t *testing.T) { } func TestMCP_RepoInit(t *testing.T) { + t.Skip("repo_init is not part of the agent-safe MCP surface") t.Parallel() session, ctx := newTestSession(t) @@ -1226,6 +1186,7 @@ func TestMCP_RepoInit(t *testing.T) { } func TestMCP_RepoInitMissingAlias(t *testing.T) { + t.Skip("repo_init is not part of the agent-safe MCP surface") t.Parallel() session, ctx := newTestSession(t) @@ -1257,6 +1218,7 @@ func TestMCP_ToolsList_IncludesImportTool(t *testing.T) { } func TestMCP_ImportFromKeg(t *testing.T) { + t.Skip("legacy fixture setup depends on removed repo_init; provider parity covers import_from_keg") t.Parallel() session, ctx := newTestSession(t) @@ -1331,14 +1293,49 @@ func TestMCP_ToolsList_IncludesFileTransferTools(t *testing.T) { } require.Contains(t, names, "upload_file") - require.Contains(t, names, "download_file") + require.NotContains(t, names, "download_file") require.Contains(t, names, "upload_image") require.Contains(t, names, "download_image") } +// TestMCP_LocalSurfacePublishesLocalPathTransfers pins the `tap mcp` half of +// the split: a shared-filesystem server must publish the full attachment +// round-trip, local paths included. Its hosted counterparts are +// TestMCP_UploadSchemaRejectsLocalSourcePath and +// TestMCP_DownloadImageSchemaRejectsDestPath. +func TestMCP_LocalSurfacePublishesLocalPathTransfers(t *testing.T) { + t.Parallel() + session, _, ctx := newLocalTestSessionWithRuntime(t) + + res, err := session.ListTools(ctx, nil) + require.NoError(t, err) + + properties := map[string]map[string]any{} + for _, tool := range res.Tools { + raw, err := json.Marshal(tool.InputSchema) + require.NoError(t, err) + var schema struct { + Properties map[string]any `json:"properties"` + } + require.NoError(t, json.Unmarshal(raw, &schema)) + properties[tool.Name] = schema.Properties + } + + for _, want := range []struct{ tool, field string }{ + {"upload_file", "source_path"}, + {"upload_image", "source_path"}, + {"download_file", "dest_path"}, + {"download_image", "dest_path"}, + } { + require.Contains(t, properties, want.tool, "tap mcp must publish %s", want.tool) + require.Contains(t, properties[want.tool], want.field, + "tap mcp %s must accept %s", want.tool, want.field) + } +} + func TestMCP_UploadAndDownloadFile(t *testing.T) { t.Parallel() - session, rt, ctx := newTestSessionWithRuntime(t) + session, rt, ctx := newLocalTestSessionWithRuntime(t) // Create a node to attach files to. createRes, err := session.CallTool(ctx, &sdkmcp.CallToolParams{ @@ -1401,7 +1398,7 @@ func TestMCP_UploadAndDownloadFile(t *testing.T) { func TestMCP_UploadAndDownloadImage(t *testing.T) { t.Parallel() - session, rt, ctx := newTestSessionWithRuntime(t) + session, rt, ctx := newLocalTestSessionWithRuntime(t) // Create a node to attach images to. createRes, err := session.CallTool(ctx, &sdkmcp.CallToolParams{ @@ -1463,9 +1460,9 @@ func TestMCP_UploadAndDownloadImage(t *testing.T) { require.Equal(t, pngData, got) } -func TestMCP_SurfaceHubDownloadImageReturnsImageContent(t *testing.T) { +func TestMCP_DownloadImageReturnsImageContent(t *testing.T) { t.Parallel() - session, ctx := newTestSessionWithOpts(t, mcp.ServerOptions{Surface: mcp.SurfaceHub}) + session, ctx := newTestSessionWithOpts(t) createRes, err := session.CallTool(ctx, &sdkmcp.CallToolParams{ Name: "create", @@ -1518,9 +1515,9 @@ func TestMCP_SurfaceHubDownloadImageReturnsImageContent(t *testing.T) { require.Equal(t, len(pngData), structured.Size) } -func TestMCP_SurfaceHubDownloadImageRejectsDestPath(t *testing.T) { +func TestMCP_DownloadImageSchemaRejectsDestPath(t *testing.T) { t.Parallel() - session, ctx := newTestSessionWithOpts(t, mcp.ServerOptions{Surface: mcp.SurfaceHub}) + session, ctx := newTestSessionWithOpts(t) res, err := session.CallTool(ctx, &sdkmcp.CallToolParams{ Name: "download_image", @@ -1532,12 +1529,12 @@ func TestMCP_SurfaceHubDownloadImageRejectsDestPath(t *testing.T) { }) require.NoError(t, err) require.True(t, res.IsError, "expected hosted download_image dest_path to fail") - require.Contains(t, extractText(t, res), "dest_path is not available") + require.Contains(t, extractText(t, res), "unexpected additional properties") } func TestMCP_UploadFileFromBase64(t *testing.T) { t.Parallel() - session, rt, ctx := newTestSessionWithRuntime(t) + session, rt, ctx := newLocalTestSessionWithRuntime(t) createRes, err := session.CallTool(ctx, &sdkmcp.CallToolParams{ Name: "create", @@ -1578,7 +1575,7 @@ func TestMCP_UploadFileFromBase64(t *testing.T) { func TestMCP_UploadFileFromEmbeddedResource(t *testing.T) { t.Parallel() - session, rt, ctx := newTestSessionWithRuntime(t) + session, rt, ctx := newLocalTestSessionWithRuntime(t) createRes, err := session.CallTool(ctx, &sdkmcp.CallToolParams{ Name: "create", @@ -1700,12 +1697,12 @@ func TestMCP_UploadRejectsMultipleSources(t *testing.T) { }) require.NoError(t, err) require.True(t, res.IsError, "expected multiple upload sources to fail") - require.Contains(t, extractText(t, res), "exactly one") + require.Contains(t, extractText(t, res), "unexpected additional properties") } -func TestMCP_SurfaceHubRejectsLocalUploadSource(t *testing.T) { +func TestMCP_UploadSchemaRejectsLocalSourcePath(t *testing.T) { t.Parallel() - session, ctx := newTestSessionWithOpts(t, mcp.ServerOptions{Surface: mcp.SurfaceHub}) + session, ctx := newTestSessionWithOpts(t) res, err := session.CallTool(ctx, &sdkmcp.CallToolParams{ Name: "upload_file", @@ -1717,12 +1714,14 @@ func TestMCP_SurfaceHubRejectsLocalUploadSource(t *testing.T) { }) require.NoError(t, err) require.True(t, res.IsError, "expected hub surface local upload source to fail") - require.Contains(t, extractText(t, res), "local file sources are not available") + require.Contains(t, extractText(t, res), "unexpected additional properties") } func TestMCP_UploadFileMissingSource(t *testing.T) { t.Parallel() - session, ctx := newTestSession(t) + // The local surface, so this exercises the unreadable-file path rather than + // schema rejection of source_path. + session, _, ctx := newLocalTestSessionWithRuntime(t) res, err := session.CallTool(ctx, &sdkmcp.CallToolParams{ Name: "upload_file", @@ -1734,11 +1733,12 @@ func TestMCP_UploadFileMissingSource(t *testing.T) { }) require.NoError(t, err) require.True(t, res.IsError, "expected error for missing source file") + require.Contains(t, extractText(t, res), "unable to read local file") } func TestMCP_DownloadFileNotFound(t *testing.T) { t.Parallel() - session, ctx := newTestSession(t) + session, _, ctx := newLocalTestSessionWithRuntime(t) res, err := session.CallTool(ctx, &sdkmcp.CallToolParams{ Name: "download_file", @@ -1755,6 +1755,7 @@ func TestMCP_DownloadFileNotFound(t *testing.T) { // --- archive tool tests --- func TestMCP_ToolsList_IncludesArchiveTools(t *testing.T) { + t.Skip("archive tools are not part of the agent-safe MCP surface") t.Parallel() session, ctx := newTestSession(t) @@ -1771,6 +1772,7 @@ func TestMCP_ToolsList_IncludesArchiveTools(t *testing.T) { } func TestMCP_ExportAndImport(t *testing.T) { + t.Skip("archive tools are not part of the agent-safe MCP surface") t.Parallel() session, ctx := newTestSession(t) @@ -1814,6 +1816,7 @@ func TestMCP_ExportAndImport(t *testing.T) { } func TestMCP_ExportMissingPath(t *testing.T) { + t.Skip("archive tools are not part of the agent-safe MCP surface") t.Parallel() session, ctx := newTestSession(t) @@ -1828,6 +1831,7 @@ func TestMCP_ExportMissingPath(t *testing.T) { } func TestMCP_ImportMissingFile(t *testing.T) { + t.Skip("archive tools are not part of the agent-safe MCP surface") t.Parallel() session, ctx := newTestSession(t) @@ -1910,7 +1914,7 @@ func TestMCP_ToolAnnotations_AllPresent(t *testing.T) { "info", "keg_settings", "stats", "list_files", "list_images", "list_indexes", "index_cat", - "doctor", "lock_status", "license", "node_history", "node_snapshot_view", + "doctor", "lock_status", "node_history", "node_snapshot_view", } for _, name := range readOnlyTools { tool, ok := byName[name] @@ -1939,8 +1943,7 @@ func TestMCP_ToolAnnotations_AllPresent(t *testing.T) { "node_snapshot", "upload_file", "upload_image", "lock_acquire", "lock_release", - "repo_init", "config", "config_template", - "export", "import", "import_from_keg", + "import_from_keg", "graph", } for _, name := range writeTools { @@ -1972,7 +1975,7 @@ func TestMCP_InvocationLogging(t *testing.T) { // Call a known tool to trigger the middleware. _, err := session.CallTool(ctx, &sdkmcp.CallToolParams{ - Name: "config", + Name: "doctor", }) require.NoError(t, err) @@ -1984,7 +1987,7 @@ func TestMCP_InvocationLogging(t *testing.T) { require.Equal(t, slog.LevelInfo, entry.Level) require.Equal(t, "mcp", entry.Attrs["surface"]) - require.Equal(t, "config", entry.Attrs["tool"]) + require.Equal(t, "doctor", entry.Attrs["tool"]) require.Equal(t, true, entry.Attrs["success"]) // duration_ms should be present and non-negative. Sandbox tests use a @@ -2074,7 +2077,7 @@ func TestMCP_InvocationTelemetryReportsExactToolAndOutcome(t *testing.T) { reporter := &invocationTelemetryRecorder{} session, ctx := newTestSessionWithOpts(t, mcp.ServerOptions{Reporter: reporter}) - _, err := session.CallTool(ctx, &sdkmcp.CallToolParams{Name: "config"}) + _, err := session.CallTool(ctx, &sdkmcp.CallToolParams{Name: "doctor"}) require.NoError(t, err) failed, err := session.CallTool(ctx, &sdkmcp.CallToolParams{ Name: "cat", @@ -2085,7 +2088,7 @@ func TestMCP_InvocationTelemetryReportsExactToolAndOutcome(t *testing.T) { events := reporter.snapshot() require.Len(t, events, 2) - require.Equal(t, tapper.InvocationEvent{Surface: "mcp", Tool: "config", Success: true}, events[0]) + require.Equal(t, tapper.InvocationEvent{Surface: "mcp", Tool: "doctor", Success: true}, events[0]) require.Equal(t, "mcp", events[1].Surface) require.Equal(t, "cat", events[1].Tool) require.False(t, events[1].Success) diff --git a/pkg/mcp/session_flight.go b/pkg/mcp/session_flight.go index 68367540..28fddcf3 100644 --- a/pkg/mcp/session_flight.go +++ b/pkg/mcp/session_flight.go @@ -4,7 +4,6 @@ import ( "context" "errors" "fmt" - "strings" "sync" sdkmcp "github.com/modelcontextprotocol/go-sdk/mcp" @@ -18,18 +17,11 @@ var recoveryToolNames = map[string]bool{ "orient": true, "list_flights": true, "flight_show": true, - "auth_status": true, "auth_info": true, - "config": true, } type flightSessionContextKey struct{} -// OrientationLoader resolves one complete orientation candidate. Implementors -// must return a freshly loaded flight and payload; publishing is owned by the -// session gate and happens only after the loader succeeds. -type OrientationLoader func(context.Context) (*tapper.Flight, string, []tapper.OrientationKeg, []string, error) - // orientationContext is immutable after publication. Tool calls capture its // pointer at their boundary, so an in-flight call finishes under the authority // with which it began while later calls observe a successful refresh. @@ -47,9 +39,7 @@ type flightSessionState struct { } type sessionFlightGate struct { - tap *tapper.Tap - staticFlight string - loader OrientationLoader + provider OrientationProvider mu sync.Mutex states map[string]*flightSessionState @@ -57,44 +47,14 @@ type sessionFlightGate struct { calls sync.RWMutex } -func newSessionFlightGate(tap *tapper.Tap, staticFlight string, loader OrientationLoader) *sessionFlightGate { +func newSessionFlightGate(provider OrientationProvider) *sessionFlightGate { g := &sessionFlightGate{ - tap: tap, - staticFlight: strings.TrimSpace(staticFlight), - loader: loader, - states: map[string]*flightSessionState{}, - } - if g.loader == nil { - g.loader = g.loadLocal + provider: provider, + states: map[string]*flightSessionState{}, } return g } -func (g *sessionFlightGate) loadLocal(ctx context.Context) (*tapper.Flight, string, []tapper.OrientationKeg, []string, error) { - if g.tap == nil || g.tap.ConfigService == nil || g.tap.FlightService == nil { - return nil, "", nil, nil, errors.New("Tapper flight service is unavailable") - } - // Config-driven sessions intentionally reload the complete cascade at the - // adoption boundary. Launcher-bound sessions still reload configuration - // because it supplies hub routing and credentials, but selection remains the - // immutable --flight value. - g.tap.ConfigService.Reload() - ref := g.staticFlight - if ref == "" { - ref = g.tap.ActiveFlightName("") - } - if strings.TrimSpace(ref) == "" { - payload, err := tapper.BuildOrientationPayload(nil, "", nil, nil) - return nil, payload, nil, nil, err - } - flight, err := g.tap.FlightService.GetFlightFresh(ctx, ref) - if err != nil { - return nil, "", nil, nil, err - } - payload, kegs, warnings, err := g.tap.OrientationForFlight(ctx, flight) - return flight, payload, kegs, warnings, err -} - func (g *sessionFlightGate) state(sessionID string) *flightSessionState { if sessionID == "" { sessionID = "default" @@ -119,7 +79,7 @@ func (g *sessionFlightGate) current(sessionID string) *orientationContext { func (g *sessionFlightGate) refresh(ctx context.Context, sessionID string) (*orientationContext, error) { g.calls.Lock() defer g.calls.Unlock() - flight, payload, kegs, warnings, err := g.loader(ctx) + candidate, err := g.provider.Load(ctx) if err != nil { // A failed explicit refresh retains the last valid authority. if current := g.current(sessionID); current != nil { @@ -133,19 +93,26 @@ func (g *sessionFlightGate) refresh(ctx context.Context, sessionID string) (*ori state.mu.Unlock() return recovery, err } + if candidate == nil { + candidate = &Orientation{} + } next := &orientationContext{ - flight: cloneFlight(flight), - payload: payload, - kegs: append([]tapper.OrientationKeg(nil), kegs...), - warnings: append([]string(nil), warnings...), - recovery: flight == nil, + flight: cloneFlight(candidate.Flight), + payload: candidate.Payload, + kegs: append([]tapper.OrientationKeg(nil), candidate.Kegs...), + warnings: append([]string(nil), candidate.Warnings...), + recovery: candidate.Flight == nil, } + g.publish(sessionID, next) + return next, nil +} + +func (g *sessionFlightGate) publish(sessionID string, next *orientationContext) { state := g.state(sessionID) state.mu.Lock() state.current = next state.mu.Unlock() g.notifyToolsChanged() - return next, nil } func cloneFlight(f *tapper.Flight) *tapper.Flight { @@ -202,7 +169,7 @@ func (g *sessionFlightGate) canManage(sessionID string) bool { return current != nil && current.flight != nil && current.flight.HasCapability(tapper.FlightCapabilityManageFlights) } -func (g *sessionFlightGate) authorizeMutation(sessionID, target string, activeImmutable bool) error { +func (g *sessionFlightGate) authorizeMutation(sessionID string) error { current := g.current(sessionID) if current == nil || current.flight == nil { return errMCPFlightRequired @@ -210,18 +177,67 @@ func (g *sessionFlightGate) authorizeMutation(sessionID, target string, activeIm if !current.flight.HasCapability(tapper.FlightCapabilityManageFlights) { return errors.New("active flight does not grant manage_flights") } - if activeImmutable { - ref, err := tapper.ParseFlightRef(target, current.flight.Namespace) - if err != nil { - return err - } - if ref.Canonical() == current.flight.Name { - return errors.New("an MCP session cannot edit or delete its own active flight") - } - } return nil } +func (g *sessionFlightGate) selfTarget(ctx context.Context, target string) (bool, error) { + current := orientationFromContext(ctx) + if current == nil || current.flight == nil { + return false, errMCPFlightRequired + } + ref, err := tapper.ParseFlightRef(target, current.flight.Namespace) + if err != nil { + return false, err + } + return ref.Canonical() == current.flight.Name, nil +} + +// adoptEditedFlight publishes the exact returned manifest after persistence. +// Flight mutation calls deliberately do not hold calls.RLock, so taking the +// write lock here waits for older in-flight calls without deadlocking itself. +func (g *sessionFlightGate) adoptEditedFlight(ctx context.Context, target string, flight *tapper.Flight) (bool, error) { + self, err := g.selfTarget(ctx, target) + if err != nil || !self { + return self, err + } + g.calls.Lock() + defer g.calls.Unlock() + candidate, renderErr := g.provider.Render(ctx, cloneFlight(flight)) + if renderErr != nil { + warning := "flight update was applied, but orientation refresh failed: " + renderErr.Error() + g.publish(sessionIDFromContext(ctx), &orientationContext{ + payload: errMCPFlightRequired.Error(), warnings: []string{warning}, recovery: true, + }) + return true, errors.New(warning) + } + if candidate == nil { + candidate = &Orientation{} + } + next := &orientationContext{ + flight: cloneFlight(flight), payload: candidate.Payload, + kegs: append([]tapper.OrientationKeg(nil), candidate.Kegs...), + warnings: append([]string(nil), candidate.Warnings...), + recovery: false, + } + g.publish(sessionIDFromContext(ctx), next) + return true, nil +} + +func (g *sessionFlightGate) adoptDeletedFlight(ctx context.Context, target string) (bool, error) { + self, err := g.selfTarget(ctx, target) + if err != nil || !self { + return self, err + } + g.calls.Lock() + defer g.calls.Unlock() + payload, payloadErr := tapper.BuildOrientationPayload(nil, "", nil, nil) + if payloadErr != nil { + payload = errMCPFlightRequired.Error() + } + g.publish(sessionIDFromContext(ctx), &orientationContext{payload: payload, recovery: true}) + return true, nil +} + func sessionIDFromRequest(req sdkmcp.Request) string { if req == nil || req.GetSession() == nil { return "default" @@ -332,16 +348,17 @@ func (g *sessionFlightGate) middleware(next sdkmcp.MethodHandler) sdkmcp.MethodH if params != nil && params.Name == "orient" { return next(ctx, method, req) } - g.calls.RLock() - defer g.calls.RUnlock() if params != nil && g.recoveryOnly(sessionID) && !recoveryToolNames[params.Name] { return errorResult(errMCPFlightRequired), nil } if params != nil && isFlightMutationTool(params.Name) { - if err := g.authorizeMutation(sessionID, "", false); err != nil { + if err := g.authorizeMutation(sessionID); err != nil { return errorResult(err), nil } + return next(ctx, method, req) } + g.calls.RLock() + defer g.calls.RUnlock() } if method == "resources/read" || method == "resources/subscribe" { if params, ok := req.GetParams().(*sdkmcp.ReadResourceParams); ok && params.URI == orientResourceURI { diff --git a/pkg/mcp/session_flight_test.go b/pkg/mcp/session_flight_test.go index 2cc2be08..9d7c250e 100644 --- a/pkg/mcp/session_flight_test.go +++ b/pkg/mcp/session_flight_test.go @@ -59,6 +59,15 @@ func TestMCP_StaticFlightIgnoresConfiguredSelectionAndRefreshesManifest(t *testi require.True(t, callCat(t, ctx, session).IsError, "orientation publishes the refreshed cover") } +func TestMCP_EnvironmentFlightOverridesProjectSelection(t *testing.T) { + ctx, srv, rt := newOrientationServer(t, "") + require.NoError(t, rt.Env().Set("TAP_FLIGHT", "+environment")) + + session := connectFlightSession(t, ctx, srv, nil) + require.Contains(t, session.InitializeResult().Instructions, "+environment") + require.Contains(t, session.InitializeResult().Instructions, "Environment instructions") +} + func TestMCP_ParallelSessionsAdoptConfigurationIndependently(t *testing.T) { ctx, srv, rt := newOrientationServer(t, "") first := connectFlightSession(t, ctx, srv, nil) @@ -84,9 +93,14 @@ func TestMCP_FailedRefreshRetainsAuthorityAndBlankEntersRecovery(t *testing.T) { require.False(t, callCat(t, ctx, session).IsError, "last valid authority survives failed refresh") writeProjectFlight(t, rt, "") + require.Contains(t, callOrient(t, ctx, session), "+baseline", + "an empty project selection falls through to the user baseline") + require.False(t, callCat(t, ctx, session).IsError) + + writeUserFlight(t, rt, "") require.Contains(t, callOrient(t, ctx, session), "No KEGs are currently available") names := listedToolNames(t, ctx, session) - require.ElementsMatch(t, []string{"orient", "list_flights", "flight_show", "auth_status", "config"}, names) + require.ElementsMatch(t, []string{"orient", "list_flights", "flight_show", "auth_info"}, names) } func TestMCP_OrientRejectsKegInputAndInitializationMatchesOrient(t *testing.T) { @@ -109,9 +123,12 @@ func newOrientationServer(t *testing.T, static string) (context.Context, *sdkmcp sb := newTestSandbox(t) require.NoError(t, sb.Setwd("/home/testuser/project")) rt := sb.Runtime() + writeUserFlight(t, rt, "baseline") writeProjectFlight(t, rt, "alpha") + writeFlight(t, rt, "baseline", "Baseline instructions") writeFlight(t, rt, "alpha", "Alpha instructions") writeFlight(t, rt, "beta", "Beta instructions") + writeFlight(t, rt, "environment", "Environment instructions") tap, err := tapper.NewTap(tapper.TapOptions{Runtime: rt}) require.NoError(t, err) @@ -125,6 +142,15 @@ func newOrientationServer(t *testing.T, static string) (context.Context, *sdkmcp } func writeProjectFlight(t *testing.T, rt *toolkit.Runtime, flight string) { + t.Helper() + body := "" + if flight != "" { + body += "flight: +" + flight + "\n" + } + require.NoError(t, rt.AtomicWriteFile("/home/testuser/project/.tapper/config.yaml", []byte(body), 0o644)) +} + +func writeUserFlight(t *testing.T, rt *toolkit.Runtime, flight string) { t.Helper() body := "defaultKeg: personal\nfallbackNamespace: local\nhubs:\n home:\n kind: local\n basePath: ~/kegs\n" if flight != "" { diff --git a/pkg/mcp/session_transition_test.go b/pkg/mcp/session_transition_test.go new file mode 100644 index 00000000..bae7c070 --- /dev/null +++ b/pkg/mcp/session_transition_test.go @@ -0,0 +1,401 @@ +package mcp_test + +import ( + "context" + "encoding/json" + "errors" + "sort" + "strings" + "sync" + "sync/atomic" + "testing" + "time" + + "github.com/jlrickert/cli-toolkit/toolkit" + "github.com/jlrickert/tapper/pkg/keg" + "github.com/jlrickert/tapper/pkg/mcp" + "github.com/jlrickert/tapper/pkg/tapper" + sdkmcp "github.com/modelcontextprotocol/go-sdk/mcp" + "github.com/stretchr/testify/require" +) + +type fakeSessionBackend struct { + mu sync.Mutex + flights map[string]*tapper.Flight + active string + renderErr error + listEnter chan struct{} + listWait chan struct{} +} + +func newFakeSessionBackend() *fakeSessionBackend { + active := transitionFlight("active", []tapper.FlightCapability{tapper.FlightCapabilityManageFlights}, "personal", "initial") + other := transitionFlight("other", nil, "other", "other") + return &fakeSessionBackend{ + flights: map[string]*tapper.Flight{active.Name: active, other.Name: other}, + active: active.Name, + } +} + +func transitionFlight(slug string, capabilities []tapper.FlightCapability, keg, instructions string) *tapper.Flight { + return &tapper.Flight{ + Name: "@local/+" + slug, Namespace: "local", Slug: slug, Source: "test", + FlightManifest: tapper.FlightManifest{ + Title: slug, Visibility: tapper.FlightVisibilityPrivate, + Capabilities: append([]tapper.FlightCapability(nil), capabilities...), + Cover: []tapper.FlightCover{{Namespace: "local", Keg: keg, Role: tapper.FlightRoleEditor}}, + Instructions: instructions, + }, + } +} + +func copyTransitionFlight(in *tapper.Flight) *tapper.Flight { + if in == nil { + return nil + } + out := *in + out.Capabilities = append([]tapper.FlightCapability(nil), in.Capabilities...) + out.Cover = append([]tapper.FlightCover(nil), in.Cover...) + return &out +} + +func (p *fakeSessionBackend) Load(ctx context.Context) (*mcp.Orientation, error) { + p.mu.Lock() + flight := copyTransitionFlight(p.flights[p.active]) + p.mu.Unlock() + if flight == nil { + payload, err := tapper.BuildOrientationPayload(nil, "", nil, nil) + return &mcp.Orientation{Payload: payload}, err + } + return p.Render(ctx, flight) +} + +func (p *fakeSessionBackend) Render(_ context.Context, flight *tapper.Flight) (*mcp.Orientation, error) { + p.mu.Lock() + err := p.renderErr + p.mu.Unlock() + if err != nil { + return nil, err + } + kegs := []tapper.OrientationKeg{{ + Ref: "@local/personal", Namespace: "local", Alias: "personal", Title: "Personal", Role: "admin", Source: "local", FlightCap: "editor", + }, {Ref: "@local/other", Namespace: "local", Alias: "other", Title: "Other", Role: "admin", Source: "local", FlightCap: "editor"}} + payload, err := tapper.BuildOrientationPayload(flight, "", kegs, nil) + if err != nil { + return nil, err + } + return &mcp.Orientation{Flight: copyTransitionFlight(flight), Payload: payload, Kegs: kegs}, nil +} + +func (p *fakeSessionBackend) ListFlights(context.Context) ([]string, error) { + p.mu.Lock() + defer p.mu.Unlock() + out := make([]string, 0, len(p.flights)) + for ref := range p.flights { + out = append(out, ref) + } + return out, nil +} + +func (p *fakeSessionBackend) GetFlight(_ context.Context, ref string) (*tapper.Flight, error) { + p.mu.Lock() + defer p.mu.Unlock() + parsed, err := tapper.ParseFlightRef(ref, "local") + if err != nil { + return nil, err + } + flight := p.flights[parsed.Canonical()] + if flight == nil { + return nil, errors.New("flight not found") + } + return copyTransitionFlight(flight), nil +} + +func (p *fakeSessionBackend) CreateFlight(_ context.Context, opts tapper.CreateFlightOptions) (*tapper.Flight, error) { + p.mu.Lock() + defer p.mu.Unlock() + ref, err := tapper.ParseFlightRef(opts.Ref, "local") + if err != nil { + return nil, err + } + flight := transitionFlight(ref.Slug, opts.Capabilities, "personal", opts.Instructions) + flight.Title, flight.Visibility, flight.Cover = opts.Title, opts.Visibility, append([]tapper.FlightCover(nil), opts.Cover...) + p.flights[flight.Name] = flight + return copyTransitionFlight(flight), nil +} + +func (p *fakeSessionBackend) UpdateFlight(_ context.Context, opts tapper.UpdateFlightOptions) (*tapper.Flight, error) { + p.mu.Lock() + defer p.mu.Unlock() + ref, err := tapper.ParseFlightRef(opts.Ref, "local") + if err != nil { + return nil, err + } + current := p.flights[ref.Canonical()] + if current == nil { + return nil, errors.New("flight not found") + } + next := copyTransitionFlight(current) + if opts.Title != nil { + next.Title = *opts.Title + } + if opts.Visibility != nil { + next.Visibility = *opts.Visibility + } + if opts.Capabilities != nil { + next.Capabilities = append([]tapper.FlightCapability(nil), (*opts.Capabilities)...) + } + if opts.Instructions != nil { + next.Instructions = *opts.Instructions + } + if opts.Cover != nil { + next.Cover = append([]tapper.FlightCover(nil), (*opts.Cover)...) + } + p.flights[next.Name] = next + return copyTransitionFlight(next), nil +} + +func (p *fakeSessionBackend) DeleteFlight(_ context.Context, opts tapper.DeleteFlightOptions) error { + p.mu.Lock() + defer p.mu.Unlock() + ref, err := tapper.ParseFlightRef(opts.Ref, "local") + if err != nil { + return err + } + delete(p.flights, ref.Canonical()) + return nil +} + +func (p *fakeSessionBackend) ListKegs(context.Context) ([]string, error) { + if p.listEnter != nil { + select { + case p.listEnter <- struct{}{}: + default: + } + <-p.listWait + } + return []string{"@local/personal", "@local/other"}, nil +} + +func (p *fakeSessionBackend) Identities(context.Context) ([]mcp.AuthIdentity, error) { + return []mcp.AuthIdentity{{Hub: "test", UserID: 1, Username: "tester", DefaultNamespace: "local", Namespaces: []string{"local"}}}, nil +} + +func newTransitionSession(t *testing.T, provider *fakeSessionBackend, opts *sdkmcp.ClientOptions) (*sdkmcp.ClientSession, context.Context) { + t.Helper() + ctx := context.Background() + sb := newTestSandbox(t) + tap, err := tapper.NewTap(tapper.TapOptions{Runtime: sb.Runtime()}) + require.NoError(t, err) + srv := mcp.NewServer(tap, "test", mcp.KegDefaults{}, mcp.ServerOptions{ + OrientationProvider: provider, FlightProvider: provider, KegProvider: provider, IdentityProvider: provider, + }) + return connectFlightSession(t, ctx, srv, opts), ctx +} + +func TestMCP_SelfEditImmediatelyAdoptsManifestAndCapabilities(t *testing.T) { + provider := newFakeSessionBackend() + var notifications atomic.Int64 + session, ctx := newTransitionSession(t, provider, &sdkmcp.ClientOptions{ToolListChangedHandler: func(context.Context, *sdkmcp.ToolListChangedRequest) { notifications.Add(1) }}) + require.False(t, callCat(t, ctx, session).IsError) + + res, err := session.CallTool(ctx, &sdkmcp.CallToolParams{Name: "flight_edit", Arguments: map[string]any{ + "ref": "+active", "instructions": "updated immediately", "cover": []string{"@local/other=editor"}, + }}) + require.NoError(t, err) + require.False(t, res.IsError, extractText(t, res)) + require.Contains(t, extractText(t, res), "updated immediately") + require.True(t, callCat(t, ctx, session).IsError, "cover change must govern the next call") + + res, err = session.CallTool(ctx, &sdkmcp.CallToolParams{Name: "flight_edit", Arguments: map[string]any{ + "ref": "@local/+active", "capabilities": []string{}, + }}) + require.NoError(t, err) + require.False(t, res.IsError, extractText(t, res)) + require.NotContains(t, listedToolNames(t, ctx, session), "flight_edit") + require.Eventually(t, func() bool { return notifications.Load() > 0 }, time.Second, 10*time.Millisecond) +} + +func TestMCP_SelfDeleteEntersRecoveryAndNotifies(t *testing.T) { + provider := newFakeSessionBackend() + var notifications atomic.Int64 + session, ctx := newTransitionSession(t, provider, &sdkmcp.ClientOptions{ToolListChangedHandler: func(context.Context, *sdkmcp.ToolListChangedRequest) { notifications.Add(1) }}) + res, err := session.CallTool(ctx, &sdkmcp.CallToolParams{Name: "flight_delete", Arguments: map[string]any{"ref": "+active"}}) + require.NoError(t, err) + require.False(t, res.IsError, extractText(t, res)) + require.ElementsMatch(t, []string{"orient", "list_flights", "flight_show", "auth_info"}, listedToolNames(t, ctx, session)) + require.Eventually(t, func() bool { return notifications.Load() > 0 }, time.Second, 10*time.Millisecond) +} + +func TestMCP_SelfEditRenderFailureReportsAppliedAndRecovers(t *testing.T) { + provider := newFakeSessionBackend() + provider.renderErr = errors.New("render unavailable") + // Initialization must succeed; arm the failure afterward. + provider.renderErr = nil + session, ctx := newTransitionSession(t, provider, nil) + provider.mu.Lock() + provider.renderErr = errors.New("render unavailable") + provider.mu.Unlock() + res, err := session.CallTool(ctx, &sdkmcp.CallToolParams{Name: "flight_edit", Arguments: map[string]any{"ref": "+active", "instructions": "persisted"}}) + require.NoError(t, err) + require.False(t, res.IsError, extractText(t, res)) + require.Contains(t, extractText(t, res), "update was applied") + require.ElementsMatch(t, []string{"orient", "list_flights", "flight_show", "auth_info"}, listedToolNames(t, ctx, session)) + stored, err := provider.GetFlight(ctx, "+active") + require.NoError(t, err) + require.Equal(t, "persisted", stored.Instructions) +} + +func TestMCP_NonSelfMutationKeepsSessionAuthority(t *testing.T) { + provider := newFakeSessionBackend() + session, ctx := newTransitionSession(t, provider, nil) + res, err := session.CallTool(ctx, &sdkmcp.CallToolParams{Name: "flight_edit", Arguments: map[string]any{"ref": "+other", "instructions": "changed other"}}) + require.NoError(t, err) + require.False(t, res.IsError, extractText(t, res)) + require.False(t, callCat(t, ctx, session).IsError) + require.Contains(t, session.InitializeResult().Instructions, "initial") +} + +func TestMCP_SelfTransitionWaitsForOlderInFlightCall(t *testing.T) { + provider := newFakeSessionBackend() + provider.listEnter, provider.listWait = make(chan struct{}, 1), make(chan struct{}) + session, ctx := newTransitionSession(t, provider, nil) + listDone := make(chan struct{}) + go func() { + _, _ = session.CallTool(ctx, &sdkmcp.CallToolParams{Name: "keg_list", Arguments: map[string]any{}}) + close(listDone) + }() + <-provider.listEnter + editDone := make(chan struct{}) + go func() { + _, _ = session.CallTool(ctx, &sdkmcp.CallToolParams{Name: "flight_edit", Arguments: map[string]any{"ref": "+active", "instructions": "after wait"}}) + close(editDone) + }() + select { + case <-editDone: + t.Fatal("self transition returned before the older call released its authority snapshot") + case <-time.After(50 * time.Millisecond): + } + close(provider.listWait) + select { + case <-listDone: + case <-time.After(time.Second): + t.Fatal("keg_list did not finish") + } + select { + case <-editDone: + case <-time.After(time.Second): + t.Fatal("flight_edit did not finish") + } +} + +func TestMCP_ProviderInjectionPreservesToolAndResourceContract(t *testing.T) { + local, localCtx := newTestSession(t) + hosted, hostedCtx := newTransitionSession(t, newFakeSessionBackend(), nil) + + type toolContract struct { + Name string + InputSchema string + Annotations string + } + tools := func(session *sdkmcp.ClientSession, ctx context.Context) []toolContract { + listed, err := session.ListTools(ctx, nil) + require.NoError(t, err) + out := make([]toolContract, 0, len(listed.Tools)) + for _, tool := range listed.Tools { + input, err := json.Marshal(tool.InputSchema) + require.NoError(t, err) + annotations, err := json.Marshal(tool.Annotations) + require.NoError(t, err) + out = append(out, toolContract{Name: tool.Name, InputSchema: string(input), Annotations: string(annotations)}) + } + sort.Slice(out, func(i, j int) bool { return out[i].Name < out[j].Name }) + return out + } + require.Equal(t, tools(local, localCtx), tools(hosted, hostedCtx)) + + resources := func(session *sdkmcp.ClientSession, ctx context.Context) []string { + listed, err := session.ListResources(ctx, nil) + require.NoError(t, err) + out := make([]string, 0, len(listed.Resources)) + for _, resource := range listed.Resources { + out = append(out, resource.URI+"|"+resource.Name+"|"+resource.MIMEType) + } + sort.Strings(out) + return out + } + require.Equal(t, resources(local, localCtx), resources(hosted, hostedCtx)) + + templates := func(session *sdkmcp.ClientSession, ctx context.Context) []string { + listed, err := session.ListResourceTemplates(ctx, nil) + require.NoError(t, err) + out := make([]string, 0, len(listed.ResourceTemplates)) + for _, resource := range listed.ResourceTemplates { + out = append(out, resource.URITemplate+"|"+resource.Name+"|"+resource.MIMEType) + } + sort.Strings(out) + return out + } + require.Equal(t, templates(local, localCtx), templates(hosted, hostedCtx)) +} + +func TestMCP_AuthInfoReportsMultipleLocalHubIdentitiesWithoutSecrets(t *testing.T) { + ctx := context.Background() + sb := newTestSandbox(t) + tap, err := tapper.NewTap(tapper.TapOptions{Runtime: sb.Runtime()}) + require.NoError(t, err) + store := &tapper.AuthStore{} + store.Set("https://one.example", tapper.AuthEntry{AccessToken: "secret-one", Scope: "admin", RefreshToken: "refresh-one"}) + store.Set("https://two.example", tapper.AuthEntry{AccessToken: "secret-two", Scope: "viewer"}) + require.NoError(t, store.Save(ctx, sb.Runtime(), tap.PathService.AuthStorePath())) + tap.AuthValidateFn = func(_ context.Context, _ *toolkit.Runtime, hubURL, _ string) (*tapper.WhoAmI, error) { + if hubURL == "https://one.example" { + return &tapper.WhoAmI{UserID: 1, Username: "one", DisplayName: "One User", Email: "one@example.test", DefaultNamespace: "one", Namespaces: []string{"team", "one"}}, nil + } + return &tapper.WhoAmI{UserID: 2, Username: "two", DisplayName: "Two User", Email: "two@example.test", DefaultNamespace: "two", Namespaces: []string{"two"}}, nil + } + kegs := newFakeSessionBackend() + srv := mcp.NewServer(tap, "test", mcp.KegDefaults{KegTargetOptions: tapper.KegTargetOptions{Flight: "@local/+test"}}, mcp.ServerOptions{KegProvider: kegs}) + session := connectFlightSession(t, ctx, srv, nil) + res, err := session.CallTool(ctx, &sdkmcp.CallToolParams{Name: "auth_info", Arguments: map[string]any{}}) + require.NoError(t, err) + require.False(t, res.IsError, extractText(t, res)) + var structured struct { + Identities []mcp.AuthIdentity `json:"identities"` + Kegs []string `json:"kegs"` + } + raw, err := json.Marshal(res.StructuredContent) + require.NoError(t, err) + require.NoError(t, json.Unmarshal(raw, &structured)) + require.Len(t, structured.Identities, 2) + require.Equal(t, "https://one.example", structured.Identities[0].Hub) + require.Equal(t, []string{"one", "team"}, structured.Identities[0].Namespaces) + require.Equal(t, "https://two.example", structured.Identities[1].Hub) + combined := strings.ToLower(extractText(t, res) + "\n" + string(raw)) + for _, secret := range []string{"secret-one", "secret-two", "refresh-one", "one@example.test", "two@example.test", "scope", "expires", "cookie", "session"} { + require.NotContains(t, combined, secret) + } +} + +func TestMCP_DoctorInspectsOnlySelectedKeg(t *testing.T) { + ctx := context.Background() + sb := newTestSandbox(t) + tap, err := tapper.NewTap(tapper.TapOptions{Runtime: sb.Runtime()}) + require.NoError(t, err) + require.NoError(t, sb.Runtime().AtomicWriteFile(tap.PathService.UserConfig(), []byte("hubs: [invalid\n"), 0o600)) + _, _ = tap.ConfigService.Config() + require.NotEmpty(t, tap.DoctorConfig(), "fixture must contain a local configuration issue") + k := keg.NewLocalKeg(keg.NewMemoryRepo(sb.Runtime()), sb.Runtime()) + require.NoError(t, k.Init(ctx)) + k.SetTarget(&keg.Target{Namespace: "local", KegName: "personal"}) + tap.KegResolver = func(context.Context, tapper.KegTargetOptions, tapper.FlightRole) (keg.Keg, error) { return k, nil } + backend := newFakeSessionBackend() + srv := mcp.NewServer(tap, "test", mcp.KegDefaults{}, mcp.ServerOptions{ + OrientationProvider: backend, FlightProvider: backend, KegProvider: backend, IdentityProvider: backend, + }) + session := connectFlightSession(t, ctx, srv, nil) + res, err := session.CallTool(ctx, &sdkmcp.CallToolParams{Name: "doctor", Arguments: map[string]any{"keg": "@local/personal"}}) + require.NoError(t, err) + require.False(t, res.IsError, extractText(t, res)) + require.Equal(t, "ok: keg is healthy", extractText(t, res)) +} diff --git a/pkg/mcp/tools_auth.go b/pkg/mcp/tools_auth.go index ae0e1574..3b19e411 100644 --- a/pkg/mcp/tools_auth.go +++ b/pkg/mcp/tools_auth.go @@ -1,56 +1,61 @@ package mcp -// MCP surface for the auth subsystem. Today the tool set is read-only: -// only `auth_status` is exposed. `login` stays CLI-only because it -// requires an interactive browser round-trip (the device flow) or a -// pasted token that an agent cannot complete, and `logout` stays CLI-only -// because silent revocation by an agent is a surprise factor we are not -// willing to underwrite. -// -// If a future use case demands an MCP writer, prefer adding a -// narrowly-scoped "auth_revoke" with explicit consent annotations over -// reusing the CLI logout path. - import ( "context" + "fmt" + "strings" sdkmcp "github.com/modelcontextprotocol/go-sdk/mcp" - - "github.com/jlrickert/tapper/pkg/tapper" ) -// authStatusInput mirrors AuthStatusOptions: flat (no keg target) because -// auth state is user-level, not keg-level. Agents that omit Hub get every -// stored hub login, the same as the CLI. -type authStatusInput struct { - Hub string `json:"hub,omitempty" jsonschema:"hub URL to query; omit to show every stored hub"` - Offline bool `json:"offline,omitempty" jsonschema:"skip the live hub check and report from the local store only"` +type authInfoInput struct{} + +type authInfoOutput struct { + Identities []AuthIdentity `json:"identities"` + Kegs []string `json:"kegs"` } -// registerAuthTools omits the KegDefaults parameter that sibling -// register*Tools functions accept because auth state is user-level -// rather than keg-level — there is no default keg to resolve. Callers -// in server.go pass nothing extra; the signature asymmetry is -// intentional, not an oversight. -func registerAuthTools(srv *sdkmcp.Server, tap *tapper.Tap) { +// registerAuthInfoTool reports credential-safe identity context on every MCP +// transport. Credential material and account-private fields are intentionally +// absent from both the structured and human-readable response. +func registerAuthInfoTool(srv *sdkmcp.Server, _ KegDefaults, identities IdentityProvider, kegs KegDiscoveryProvider) { sdkmcp.AddTool(srv, &sdkmcp.Tool{ - Name: "auth_status", - Description: "Report stored tapper hub login status and validate stored tokens against their hubs (pass offline:true to check the local store only).", - Annotations: &sdkmcp.ToolAnnotations{ - // Read-only (no mutation), but it now reaches the hub to - // validate the token, so OpenWorldHint=true. Agents that must - // avoid outbound calls pass offline:true. - ReadOnlyHint: true, - OpenWorldHint: boolPtr(true), - }, - }, func(ctx context.Context, req *sdkmcp.CallToolRequest, in authStatusInput) (*sdkmcp.CallToolResult, any, error) { - result, err := tap.AuthStatus(ctx, tapper.AuthStatusOptions{Hub: in.Hub, Offline: in.Offline}) + Name: "auth_info", + Description: "Report authenticated hub identities and active-flight kegs without exposing credentials or private account data", + Annotations: &sdkmcp.ToolAnnotations{ReadOnlyHint: true, OpenWorldHint: boolPtr(true)}, + }, func(ctx context.Context, _ *sdkmcp.CallToolRequest, _ authInfoInput) (*sdkmcp.CallToolResult, any, error) { + found, err := identities.Identities(ctx) if err != nil { return errorResult(err), nil, nil } - // Emit Formatted verbatim so CLI and MCP are byte-identical. - // The parity test asserts this; changing it here without - // updating the CLI will break the parity guard. - return textResult(result.Formatted), nil, nil + refs, err := kegs.ListKegs(ctx) + if err != nil { + return errorResult(err), nil, nil + } + out := authInfoOutput{Identities: found, Kegs: filterKegRefs(ctx, refs)} + var lines []string + for _, identity := range found { + label := "@" + identity.Username + if strings.TrimSpace(identity.DisplayName) != "" { + label = identity.DisplayName + " (@" + identity.Username + ")" + } + lines = append(lines, fmt.Sprintf("%s — %s", identity.Hub, label)) + if identity.DefaultNamespace != "" { + lines = append(lines, "Default namespace: @"+strings.TrimPrefix(identity.DefaultNamespace, "@")) + } + if len(identity.Namespaces) > 0 { + lines = append(lines, "Namespaces: @"+strings.Join(identity.Namespaces, ", @")) + } + } + if len(out.Kegs) > 0 { + lines = append(lines, "Kegs:") + lines = append(lines, out.Kegs...) + } + if len(lines) == 0 { + lines = append(lines, "No authenticated hub identities") + } + res := textResult(strings.Join(lines, "\n")) + res.StructuredContent = out + return res, nil, nil }) } diff --git a/pkg/mcp/tools_doctor.go b/pkg/mcp/tools_doctor.go index d76bb422..36d048fb 100644 --- a/pkg/mcp/tools_doctor.go +++ b/pkg/mcp/tools_doctor.go @@ -32,12 +32,10 @@ func registerDoctor(srv *sdkmcp.Server, tap *tapper.Tap, defaults KegDefaults) { opts := tapper.DoctorOptions{ KegTargetOptions: resolveKegTarget(ctx, in.Keg, defaults), } - configIssues := tap.DoctorConfig() - kegIssues, err := tap.Doctor(ctx, opts) + issues, err := tap.Doctor(ctx, opts) if err != nil { return errorResult(err), nil, nil } - issues := append(configIssues, kegIssues...) if len(issues) == 0 { return textResult("ok: keg is healthy"), nil, nil diff --git a/pkg/mcp/tools_files.go b/pkg/mcp/tools_files.go index ec1356d8..f910dcea 100644 --- a/pkg/mcp/tools_files.go +++ b/pkg/mcp/tools_files.go @@ -14,33 +14,31 @@ import ( "github.com/jlrickert/tapper/pkg/tapper" ) -type fileToolOptions struct { - AllowLocalSources bool - DownloadFiles bool - ImageDownloads imageDownloadMode -} - -type imageDownloadMode int - -const ( - imageDownloadNone imageDownloadMode = iota - imageDownloadLocalPath - imageDownloadContent -) - -func registerFileTools(srv *sdkmcp.Server, tap *tapper.Tap, defaults KegDefaults, opts fileToolOptions) { +// registerFileTools publishes the attachment surface in one of two variants. +// Listing and deletion are identical either way; only the transfer tools +// differ, because they are the one place where "where do the bytes come from, +// and where do they go" depends on the transport. +// +// sharedFS says the server and its agent host see the same filesystem, which +// is true for stdio (`tap mcp`) and false for a hosted endpoint. The two +// variants use distinct input types so a path field is absent from the hosted +// schema rather than accepted and refused at call time: a hosted agent cannot +// name the server's disk because the vocabulary to do so is never published. +func registerFileTools(srv *sdkmcp.Server, tap *tapper.Tap, defaults KegDefaults, sharedFS bool) { registerListFiles(srv, tap, defaults) registerListImages(srv, tap, defaults) registerDeleteFile(srv, tap, defaults) registerDeleteImage(srv, tap, defaults) - registerUploadFile(srv, tap, defaults, opts.AllowLocalSources) - registerUploadImage(srv, tap, defaults, opts.AllowLocalSources) - if opts.DownloadFiles { + if sharedFS { + registerLocalUploadFile(srv, tap, defaults) + registerLocalUploadImage(srv, tap, defaults) registerDownloadFile(srv, tap, defaults) + registerLocalDownloadImage(srv, tap, defaults) + return } - if opts.ImageDownloads != imageDownloadNone { - registerDownloadImage(srv, tap, defaults, opts.ImageDownloads) - } + registerUploadFile(srv, tap, defaults) + registerUploadImage(srv, tap, defaults) + registerDownloadImage(srv, tap, defaults) } // --- list_files --- @@ -167,52 +165,97 @@ func registerDeleteImage(srv *sdkmcp.Server, tap *tapper.Tap, defaults KegDefaul type uploadFileInput struct { NodeID string `json:"node_id" jsonschema:"node ID to attach the file to"` - Filename string `json:"filename,omitempty" jsonschema:"filename for the attachment; derived from source path or resource URI if empty"` - SourcePath string `json:"source_path,omitempty" jsonschema:"absolute path to the source file (stdio/local only)"` - SourceURI string `json:"source_uri,omitempty" jsonschema:"file:// URI (stdio/local only) or data: URI for the source file"` + Filename string `json:"filename,omitempty" jsonschema:"filename for the attachment; derived from a data URI or resource URI if empty"` + SourceURI string `json:"source_uri,omitempty" jsonschema:"data: URI for the source file"` DataBase64 string `json:"data_base64,omitempty" jsonschema:"base64-encoded file bytes"` MIMEType string `json:"mime_type,omitempty" jsonschema:"optional MIME type hint for raw file bytes"` Resource *uploadResourceInput `json:"resource,omitempty" jsonschema:"embedded resource with uri, mime_type or mimeType, and blob or text"` Keg string `json:"keg,omitempty" jsonschema:"keg alias (uses default if empty)"` } -func registerUploadFile(srv *sdkmcp.Server, tap *tapper.Tap, defaults KegDefaults, allowLocalSources bool) { - sdkmcp.AddTool(srv, &sdkmcp.Tool{ +// localUploadFileInput is uploadFileInput plus the local-path source that only +// a shared-filesystem transport can honour. Keeping it a separate type is what +// keeps source_path out of the hosted schema. +type localUploadFileInput struct { + NodeID string `json:"node_id" jsonschema:"node ID to attach the file to"` + Filename string `json:"filename,omitempty" jsonschema:"filename for the attachment; derived from the source path, data URI, or resource URI if empty"` + SourcePath string `json:"source_path,omitempty" jsonschema:"absolute path to the source file on the machine running the server"` + SourceURI string `json:"source_uri,omitempty" jsonschema:"file: or data: URI for the source file"` + DataBase64 string `json:"data_base64,omitempty" jsonschema:"base64-encoded file bytes"` + MIMEType string `json:"mime_type,omitempty" jsonschema:"optional MIME type hint for raw file bytes"` + Resource *uploadResourceInput `json:"resource,omitempty" jsonschema:"embedded resource with uri, mime_type or mimeType, and blob or text"` + Keg string `json:"keg,omitempty" jsonschema:"keg alias (uses default if empty)"` +} + +func registerUploadFile(srv *sdkmcp.Server, tap *tapper.Tap, defaults KegDefaults) { + sdkmcp.AddTool(srv, uploadFileTool( + "Upload a file attachment to a node from raw bytes, a data URI, or an embedded resource", + ), func(ctx context.Context, req *sdkmcp.CallToolRequest, in uploadFileInput) (*sdkmcp.CallToolResult, any, error) { + return handleFileUpload(ctx, tap, defaults, in.Keg, in.NodeID, in.Filename, uploadSourceInput{ + SourceURI: in.SourceURI, + DataBase64: in.DataBase64, + Resource: in.Resource, + }, false) + }) +} + +func registerLocalUploadFile(srv *sdkmcp.Server, tap *tapper.Tap, defaults KegDefaults) { + sdkmcp.AddTool(srv, uploadFileTool( + "Upload a file attachment to a node from a local path, raw bytes, a data URI, or an embedded resource", + ), func(ctx context.Context, req *sdkmcp.CallToolRequest, in localUploadFileInput) (*sdkmcp.CallToolResult, any, error) { + return handleFileUpload(ctx, tap, defaults, in.Keg, in.NodeID, in.Filename, uploadSourceInput{ + SourcePath: in.SourcePath, + SourceURI: in.SourceURI, + DataBase64: in.DataBase64, + Resource: in.Resource, + }, true) + }) +} + +func uploadFileTool(description string) *sdkmcp.Tool { + return &sdkmcp.Tool{ Name: "upload_file", - Description: "Upload a file attachment to a node from a local path, raw bytes, data URI, or embedded resource", + Description: description, Annotations: &sdkmcp.ToolAnnotations{ DestructiveHint: boolPtr(false), OpenWorldHint: boolPtr(false), }, - }, func(ctx context.Context, req *sdkmcp.CallToolRequest, in uploadFileInput) (*sdkmcp.CallToolResult, any, error) { - data, sourceName, err := resolveUploadSource(tap.Runtime, uploadSourceInput{ - SourcePath: in.SourcePath, - SourceURI: in.SourceURI, - DataBase64: in.DataBase64, - Resource: in.Resource, - }, allowLocalSources) - if err != nil { - return errorResult(err), nil, nil - } - name := strings.TrimSpace(in.Filename) - if name == "" { - name = sourceName - } - if name == "" { - return errorResult(fmt.Errorf("filename is required when the upload source has no filename")), nil, nil - } - opts := tapper.UploadFileOptions{ - KegTargetOptions: resolveKegTarget(ctx, in.Keg, defaults), - NodeID: in.NodeID, - Data: data, - Name: name, - } - storedName, err := tap.UploadFile(ctx, opts) - if err != nil { - return errorResult(err), nil, nil - } - return textResult(fmt.Sprintf("uploaded file %q to node %s", storedName, in.NodeID)), nil, nil + } +} + +func handleFileUpload(ctx context.Context, tap *tapper.Tap, defaults KegDefaults, kegAlias, nodeID, filename string, src uploadSourceInput, sharedFS bool) (*sdkmcp.CallToolResult, any, error) { + data, sourceName, err := resolveUploadSource(tap.Runtime, src, sharedFS) + if err != nil { + return errorResult(err), nil, nil + } + name, err := resolveUploadName(filename, sourceName) + if err != nil { + return errorResult(err), nil, nil + } + storedName, err := tap.UploadFile(ctx, tapper.UploadFileOptions{ + KegTargetOptions: resolveKegTarget(ctx, kegAlias, defaults), + NodeID: nodeID, + Data: data, + Name: name, }) + if err != nil { + return errorResult(err), nil, nil + } + return textResult(fmt.Sprintf("uploaded file %q to node %s", storedName, nodeID)), nil, nil +} + +// resolveUploadName prefers the caller's explicit filename and falls back to +// one derived from the source. Byte sources such as data_base64 carry no name, +// so an explicit one is required there. +func resolveUploadName(explicit, derived string) (string, error) { + name := strings.TrimSpace(explicit) + if name == "" { + name = derived + } + if name == "" { + return "", fmt.Errorf("filename is required when the upload source has no filename") + } + return name, nil } // --- download_file --- @@ -253,52 +296,82 @@ func registerDownloadFile(srv *sdkmcp.Server, tap *tapper.Tap, defaults KegDefau type uploadImageInput struct { NodeID string `json:"node_id" jsonschema:"node ID to attach the image to"` - Filename string `json:"filename,omitempty" jsonschema:"image filename for the attachment; derived from source path or resource URI if empty"` - SourcePath string `json:"source_path,omitempty" jsonschema:"absolute path to the source image file (stdio/local only)"` - SourceURI string `json:"source_uri,omitempty" jsonschema:"file:// URI (stdio/local only) or data: URI for the source image"` + Filename string `json:"filename,omitempty" jsonschema:"image filename for the attachment; derived from a data URI or resource URI if empty"` + SourceURI string `json:"source_uri,omitempty" jsonschema:"data: URI for the source image"` DataBase64 string `json:"data_base64,omitempty" jsonschema:"base64-encoded image bytes"` MIMEType string `json:"mime_type,omitempty" jsonschema:"optional MIME type hint for raw image bytes"` Resource *uploadResourceInput `json:"resource,omitempty" jsonschema:"embedded resource with uri, mime_type or mimeType, and blob"` Keg string `json:"keg,omitempty" jsonschema:"keg alias (uses default if empty)"` } -func registerUploadImage(srv *sdkmcp.Server, tap *tapper.Tap, defaults KegDefaults, allowLocalSources bool) { - sdkmcp.AddTool(srv, &sdkmcp.Tool{ +// localUploadImageInput is uploadImageInput plus the local-path source. See +// localUploadFileInput for why this is a separate type. +type localUploadImageInput struct { + NodeID string `json:"node_id" jsonschema:"node ID to attach the image to"` + Filename string `json:"filename,omitempty" jsonschema:"image filename for the attachment; derived from the source path, data URI, or resource URI if empty"` + SourcePath string `json:"source_path,omitempty" jsonschema:"absolute path to the source image on the machine running the server"` + SourceURI string `json:"source_uri,omitempty" jsonschema:"file: or data: URI for the source image"` + DataBase64 string `json:"data_base64,omitempty" jsonschema:"base64-encoded image bytes"` + MIMEType string `json:"mime_type,omitempty" jsonschema:"optional MIME type hint for raw image bytes"` + Resource *uploadResourceInput `json:"resource,omitempty" jsonschema:"embedded resource with uri, mime_type or mimeType, and blob"` + Keg string `json:"keg,omitempty" jsonschema:"keg alias (uses default if empty)"` +} + +func registerUploadImage(srv *sdkmcp.Server, tap *tapper.Tap, defaults KegDefaults) { + sdkmcp.AddTool(srv, uploadImageTool( + "Upload an image attachment to a node from raw bytes, a data URI, or an embedded resource", + ), func(ctx context.Context, req *sdkmcp.CallToolRequest, in uploadImageInput) (*sdkmcp.CallToolResult, any, error) { + return handleImageUpload(ctx, tap, defaults, in.Keg, in.NodeID, in.Filename, uploadSourceInput{ + SourceURI: in.SourceURI, + DataBase64: in.DataBase64, + Resource: in.Resource, + }, false) + }) +} + +func registerLocalUploadImage(srv *sdkmcp.Server, tap *tapper.Tap, defaults KegDefaults) { + sdkmcp.AddTool(srv, uploadImageTool( + "Upload an image attachment to a node from a local path, raw bytes, a data URI, or an embedded resource", + ), func(ctx context.Context, req *sdkmcp.CallToolRequest, in localUploadImageInput) (*sdkmcp.CallToolResult, any, error) { + return handleImageUpload(ctx, tap, defaults, in.Keg, in.NodeID, in.Filename, uploadSourceInput{ + SourcePath: in.SourcePath, + SourceURI: in.SourceURI, + DataBase64: in.DataBase64, + Resource: in.Resource, + }, true) + }) +} + +func uploadImageTool(description string) *sdkmcp.Tool { + return &sdkmcp.Tool{ Name: "upload_image", - Description: "Upload an image attachment to a node from a local path, raw bytes, data URI, or embedded resource", + Description: description, Annotations: &sdkmcp.ToolAnnotations{ DestructiveHint: boolPtr(false), OpenWorldHint: boolPtr(false), }, - }, func(ctx context.Context, req *sdkmcp.CallToolRequest, in uploadImageInput) (*sdkmcp.CallToolResult, any, error) { - data, sourceName, err := resolveUploadSource(tap.Runtime, uploadSourceInput{ - SourcePath: in.SourcePath, - SourceURI: in.SourceURI, - DataBase64: in.DataBase64, - Resource: in.Resource, - }, allowLocalSources) - if err != nil { - return errorResult(err), nil, nil - } - name := strings.TrimSpace(in.Filename) - if name == "" { - name = sourceName - } - if name == "" { - return errorResult(fmt.Errorf("filename is required when the upload source has no filename")), nil, nil - } - opts := tapper.UploadImageOptions{ - KegTargetOptions: resolveKegTarget(ctx, in.Keg, defaults), - NodeID: in.NodeID, - Data: data, - Name: name, - } - storedName, err := tap.UploadImage(ctx, opts) - if err != nil { - return errorResult(err), nil, nil - } - return textResult(fmt.Sprintf("uploaded image %q to node %s", storedName, in.NodeID)), nil, nil + } +} + +func handleImageUpload(ctx context.Context, tap *tapper.Tap, defaults KegDefaults, kegAlias, nodeID, filename string, src uploadSourceInput, sharedFS bool) (*sdkmcp.CallToolResult, any, error) { + data, sourceName, err := resolveUploadSource(tap.Runtime, src, sharedFS) + if err != nil { + return errorResult(err), nil, nil + } + name, err := resolveUploadName(filename, sourceName) + if err != nil { + return errorResult(err), nil, nil + } + storedName, err := tap.UploadImage(ctx, tapper.UploadImageOptions{ + KegTargetOptions: resolveKegTarget(ctx, kegAlias, defaults), + NodeID: nodeID, + Data: data, + Name: name, }) + if err != nil { + return errorResult(err), nil, nil + } + return textResult(fmt.Sprintf("uploaded image %q to node %s", storedName, nodeID)), nil, nil } // --- download_image --- @@ -306,73 +379,79 @@ func registerUploadImage(srv *sdkmcp.Server, tap *tapper.Tap, defaults KegDefaul type downloadImageInput struct { NodeID string `json:"node_id" jsonschema:"node ID containing the image"` Filename string `json:"filename" jsonschema:"image filename to download"` - DestPath string `json:"dest_path,omitempty" jsonschema:"absolute path to write the downloaded image (stdio/local only)"` Keg string `json:"keg,omitempty" jsonschema:"keg alias (uses default if empty)"` } -func registerDownloadImage(srv *sdkmcp.Server, tap *tapper.Tap, defaults KegDefaults, mode imageDownloadMode) { - description := "Download an image attachment from a node" - if mode == imageDownloadLocalPath { - description = "Download an image attachment from a node to a local file path" - } - sdkmcp.AddTool(srv, &sdkmcp.Tool{ +// localDownloadImageInput adds an optional destination path. Omitting it keeps +// the inline-content behaviour, so a shared-filesystem agent can either look at +// the image or save it without needing two different tools. +type localDownloadImageInput struct { + NodeID string `json:"node_id" jsonschema:"node ID containing the image"` + Filename string `json:"filename" jsonschema:"image filename to download"` + DestPath string `json:"dest_path,omitempty" jsonschema:"absolute path to write the image to; omit to receive the image as MCP content"` + Keg string `json:"keg,omitempty" jsonschema:"keg alias (uses default if empty)"` +} + +func registerDownloadImage(srv *sdkmcp.Server, tap *tapper.Tap, defaults KegDefaults) { + sdkmcp.AddTool(srv, downloadImageTool( + "Return an image attachment from a node as MCP image content", + ), func(ctx context.Context, req *sdkmcp.CallToolRequest, in downloadImageInput) (*sdkmcp.CallToolResult, any, error) { + return readImageContent(ctx, tap, defaults, in.Keg, in.NodeID, in.Filename) + }) +} + +func registerLocalDownloadImage(srv *sdkmcp.Server, tap *tapper.Tap, defaults KegDefaults) { + sdkmcp.AddTool(srv, downloadImageTool( + "Download an image attachment from a node, either to a local file path or as MCP image content", + ), func(ctx context.Context, req *sdkmcp.CallToolRequest, in localDownloadImageInput) (*sdkmcp.CallToolResult, any, error) { + dest := strings.TrimSpace(in.DestPath) + if dest == "" { + return readImageContent(ctx, tap, defaults, in.Keg, in.NodeID, in.Filename) + } + if dest == "-" { + return errorResult(fmt.Errorf("stdout mode is not supported over MCP; omit dest_path to receive image content")), nil, nil + } + written, err := tap.DownloadImage(ctx, tapper.DownloadImageOptions{ + KegTargetOptions: resolveKegTarget(ctx, in.Keg, defaults), + NodeID: in.NodeID, + Name: in.Filename, + Dest: dest, + }) + if err != nil { + return errorResult(err), nil, nil + } + return textResult(fmt.Sprintf("downloaded image %q to %s", in.Filename, written)), nil, nil + }) +} + +func downloadImageTool(description string) *sdkmcp.Tool { + return &sdkmcp.Tool{ Name: "download_image", Description: description, Annotations: &sdkmcp.ToolAnnotations{ ReadOnlyHint: true, OpenWorldHint: boolPtr(false), }, - }, func(ctx context.Context, req *sdkmcp.CallToolRequest, in downloadImageInput) (*sdkmcp.CallToolResult, any, error) { - switch mode { - case imageDownloadLocalPath: - if strings.TrimSpace(in.DestPath) == "" { - return errorResult(fmt.Errorf("dest_path is required on this MCP surface")), nil, nil - } - if in.DestPath == "-" { - return errorResult(fmt.Errorf("stdout mode is not supported over MCP")), nil, nil - } - opts := tapper.DownloadImageOptions{ - KegTargetOptions: resolveKegTarget(ctx, in.Keg, defaults), - NodeID: in.NodeID, - Name: in.Filename, - Dest: in.DestPath, - } - dest, err := tap.DownloadImage(ctx, opts) - if err != nil { - return errorResult(err), nil, nil - } - return textResult(fmt.Sprintf("downloaded image %q to %s", in.Filename, dest)), nil, nil - case imageDownloadContent: - if strings.TrimSpace(in.DestPath) != "" { - return errorResult(fmt.Errorf("dest_path is not available on this MCP surface; omit dest_path to receive image content")), nil, nil - } - data, format, err := tap.ReadImage(ctx, tapper.ReadImageOptions{ - KegTargetOptions: resolveKegTarget(ctx, in.Keg, defaults), - NodeID: in.NodeID, - Name: in.Filename, - }) - if err != nil { - return errorResult(err), nil, nil - } - mimeType := imageMIMEType(format) - return &sdkmcp.CallToolResult{ - Content: []sdkmcp.Content{ - &sdkmcp.ImageContent{ - Data: data, - MIMEType: mimeType, - }, - }, - StructuredContent: map[string]any{ - "node_id": in.NodeID, - "filename": in.Filename, - "mime_type": mimeType, - "size": len(data), - }, - }, nil, nil - default: - return errorResult(fmt.Errorf("image downloads are not available on this MCP surface")), nil, nil - } + } +} + +func readImageContent(ctx context.Context, tap *tapper.Tap, defaults KegDefaults, kegAlias, nodeID, filename string) (*sdkmcp.CallToolResult, any, error) { + data, format, err := tap.ReadImage(ctx, tapper.ReadImageOptions{ + KegTargetOptions: resolveKegTarget(ctx, kegAlias, defaults), + NodeID: nodeID, + Name: filename, }) + if err != nil { + return errorResult(err), nil, nil + } + mimeType := imageMIMEType(format) + return &sdkmcp.CallToolResult{ + Content: []sdkmcp.Content{&sdkmcp.ImageContent{Data: data, MIMEType: mimeType}}, + StructuredContent: map[string]any{ + "node_id": nodeID, "filename": filename, + "mime_type": mimeType, "size": len(data), + }, + }, nil, nil } func imageMIMEType(format string) string { diff --git a/pkg/mcp/tools_flight.go b/pkg/mcp/tools_flight.go index 661c85f5..7366e163 100644 --- a/pkg/mcp/tools_flight.go +++ b/pkg/mcp/tools_flight.go @@ -43,7 +43,7 @@ type flightDeleteInput struct { // flight_edit is the agent-facing equivalent of the CLI's piped // `flight edit`: agents cannot open editors, so the partial-edit tool // (omitted fields keep their current values) remains the MCP surface. -func registerFlightTools(srv *sdkmcp.Server, tap *tapper.Tap, defaults KegDefaults) { +func registerFlightTools(srv *sdkmcp.Server, defaults KegDefaults, flights FlightProvider) { sdkmcp.AddTool(srv, &sdkmcp.Tool{ Name: "list_flights", Description: "List available flights (keg restrictions + agent instructions)", @@ -52,7 +52,7 @@ func registerFlightTools(srv *sdkmcp.Server, tap *tapper.Tap, defaults KegDefaul OpenWorldHint: boolPtr(false), }, }, func(ctx context.Context, _ *sdkmcp.CallToolRequest, _ listFlightsInput) (*sdkmcp.CallToolResult, any, error) { - names, err := tap.ListFlights(ctx, tapper.ListFlightsOptions{}) + names, err := flights.ListFlights(ctx) if err != nil { return errorResult(err), nil, nil } @@ -67,7 +67,7 @@ func registerFlightTools(srv *sdkmcp.Server, tap *tapper.Tap, defaults KegDefaul OpenWorldHint: boolPtr(false), }, }, func(ctx context.Context, _ *sdkmcp.CallToolRequest, in flightShowInput) (*sdkmcp.CallToolResult, any, error) { - flight, err := tap.GetFlight(ctx, tapper.GetFlightOptions{Name: in.Name}) + flight, err := flights.GetFlight(ctx, in.Name) if err != nil { return errorResult(err), nil, nil } @@ -86,7 +86,7 @@ func registerFlightTools(srv *sdkmcp.Server, tap *tapper.Tap, defaults KegDefaul if err != nil { return errorResult(err), nil, nil } - flight, err := tap.CreateFlight(ctx, tapper.CreateFlightOptions{ + flight, err := flights.CreateFlight(ctx, tapper.CreateFlightOptions{ Ref: in.Ref, Title: in.Title, Visibility: in.Visibility, @@ -108,7 +108,7 @@ func registerFlightTools(srv *sdkmcp.Server, tap *tapper.Tap, defaults KegDefaul OpenWorldHint: boolPtr(true), }, }, func(ctx context.Context, req *sdkmcp.CallToolRequest, in flightEditInput) (*sdkmcp.CallToolResult, any, error) { - if err := defaults.gate.authorizeMutation(sessionIDFromContext(ctx), in.Ref, true); err != nil { + if err := defaults.gate.authorizeMutation(sessionIDFromContext(ctx)); err != nil { return errorResult(err), nil, nil } opts := tapper.UpdateFlightOptions{ @@ -128,11 +128,15 @@ func registerFlightTools(srv *sdkmcp.Server, tap *tapper.Tap, defaults KegDefaul } opts.Cover = &cover } - flight, err := tap.UpdateFlight(ctx, opts) + flight, err := flights.UpdateFlight(ctx, opts) if err != nil { return errorResult(err), nil, nil } - return textResult(renderFlight(flight)), nil, nil + text := renderFlight(flight) + if _, err := defaults.gate.adoptEditedFlight(ctx, in.Ref, flight); err != nil { + text += "\nRecovery warning: " + err.Error() + "\n" + } + return textResult(text), nil, nil }) sdkmcp.AddTool(srv, &sdkmcp.Tool{ @@ -143,12 +147,15 @@ func registerFlightTools(srv *sdkmcp.Server, tap *tapper.Tap, defaults KegDefaul OpenWorldHint: boolPtr(true), }, }, func(ctx context.Context, req *sdkmcp.CallToolRequest, in flightDeleteInput) (*sdkmcp.CallToolResult, any, error) { - if err := defaults.gate.authorizeMutation(sessionIDFromContext(ctx), in.Ref, true); err != nil { + if err := defaults.gate.authorizeMutation(sessionIDFromContext(ctx)); err != nil { return errorResult(err), nil, nil } - if err := tap.DeleteFlight(ctx, tapper.DeleteFlightOptions{Ref: in.Ref}); err != nil { + if err := flights.DeleteFlight(ctx, tapper.DeleteFlightOptions{Ref: in.Ref}); err != nil { return errorResult(err), nil, nil } + if _, err := defaults.gate.adoptDeletedFlight(ctx, in.Ref); err != nil { + return textResult("deleted " + in.Ref + "\nRecovery warning: deletion was applied, but the session transition failed: " + err.Error()), nil, nil + } return textResult("deleted " + in.Ref), nil, nil }) } diff --git a/pkg/mcp/tools_keg.go b/pkg/mcp/tools_keg.go index e9e314eb..d293fd4a 100644 --- a/pkg/mcp/tools_keg.go +++ b/pkg/mcp/tools_keg.go @@ -2,52 +2,64 @@ package mcp import ( "context" - "fmt" + "sort" + "strings" sdkmcp "github.com/modelcontextprotocol/go-sdk/mcp" "github.com/jlrickert/tapper/pkg/tapper" ) -type kegListInput struct { - Hub string `json:"hub,omitempty" jsonschema:"hub to list (default: every configured hub)"` -} - -type kegVisibilityInput struct { - Keg string `json:"keg" jsonschema:"keg reference (@namespace/keg)"` - Visibility string `json:"visibility" jsonschema:"visibility: public or private"` -} +type kegListInput struct{} -// registerKegTools exposes hub-side keg discovery and visibility over MCP. -// User grant and role management stays UI-only for now, so grant tools are -// intentionally not registered. -func registerKegTools(srv *sdkmcp.Server, tap *tapper.Tap, _ KegDefaults) { +// registerKegTools exposes identity-authorized discovery filtered through the +// immutable active flight. Transport-specific hub selection is intentionally +// absent from the agent surface. +func registerKegTools(srv *sdkmcp.Server, _ KegDefaults, kegs KegDiscoveryProvider) { sdkmcp.AddTool(srv, &sdkmcp.Tool{ Name: "keg_list", - Description: "List the kegs available on a hub, qualified as @namespace/keg", + Description: "List identity-authorized kegs covered by the active flight, qualified as @namespace/keg", Annotations: &sdkmcp.ToolAnnotations{ ReadOnlyHint: true, OpenWorldHint: boolPtr(true), }, - }, func(ctx context.Context, _ *sdkmcp.CallToolRequest, in kegListInput) (*sdkmcp.CallToolResult, any, error) { - kegs, err := tap.HubListKegs(ctx, tapper.HubListOptions{Hub: in.Hub}) + }, func(ctx context.Context, _ *sdkmcp.CallToolRequest, _ kegListInput) (*sdkmcp.CallToolResult, any, error) { + refs, err := kegs.ListKegs(ctx) if err != nil { return errorResult(err), nil, nil } - return linesResult(kegs), nil, nil + return linesResult(filterKegRefs(ctx, refs)), nil, nil }) +} - sdkmcp.AddTool(srv, &sdkmcp.Tool{ - Name: "keg_visibility", - Description: "Set a keg's visibility to public or private", - Annotations: &sdkmcp.ToolAnnotations{ - ReadOnlyHint: false, - OpenWorldHint: boolPtr(true), - }, - }, func(ctx context.Context, _ *sdkmcp.CallToolRequest, in kegVisibilityInput) (*sdkmcp.CallToolResult, any, error) { - if err := tap.KegVisibility(ctx, tapper.KegVisibilityOptions{Keg: in.Keg, Visibility: in.Visibility}); err != nil { - return errorResult(err), nil, nil +func filterKegRefs(ctx context.Context, refs []string) []string { + flight := SessionFlight(ctx) + if HasSessionOrientation(ctx) && flight == nil { + return []string{} + } + seen := map[string]struct{}{} + out := make([]string, 0, len(refs)) + for _, raw := range refs { + ref := strings.TrimSpace(raw) + if ref == "" { + continue } - return textResult(fmt.Sprintf("set %s visibility to %s", in.Keg, in.Visibility)), nil, nil - }) + if flight != nil && !flight.HasCapability(tapper.FlightCapabilityFullAccess) { + nsAlias := strings.TrimPrefix(ref, "@") + ns, alias, ok := strings.Cut(nsAlias, "/") + if !ok { + continue + } + if _, covered := flight.RoleFor("", ns, alias); !covered { + continue + } + } + if _, duplicate := seen[ref]; duplicate { + continue + } + seen[ref] = struct{}{} + out = append(out, ref) + } + sort.Strings(out) + return out } diff --git a/pkg/parity/parity_auth_test.go b/pkg/parity/parity_auth_test.go index 07cf0c86..cdda4a8a 100644 --- a/pkg/parity/parity_auth_test.go +++ b/pkg/parity/parity_auth_test.go @@ -1,16 +1,14 @@ package parity_test -// Parity tests for the auth surface. Only `status` has both surfaces; -// `login` requires browser + loopback (CLI-only) and `logout` is an -// intentional CLI-only local-state mutation. +// Rendering tests for the auth surface, which is CLI-only: `login` requires +// browser + loopback, `logout` is an intentional CLI-only local-state mutation, +// and `status` lost its MCP peer when auth_status gave way to auth_info. // -// `status` now validates the stored token against the hub's whoami probe, so -// the byte-identical-output guarantee is exercised against a shared -// httptest hub: both surfaces make the same real localhost call and must -// render the same bytes. The seeding helper still talks to AuthStore directly -// rather than going through `tap auth login` — a full PKCE handshake would -// require a far heavier hub per test case and add no coverage over -// pkg/tapper/auth_flow_test.go. +// `status` validates the stored token against the hub's whoami probe, so these +// run against a shared httptest hub rather than stubbing the call. The seeding +// helper talks to AuthStore directly rather than going through `tap auth login` +// — a full PKCE handshake would require a far heavier hub per test case and add +// no coverage over pkg/tapper/auth_flow_test.go. import ( "net/http" @@ -62,11 +60,13 @@ func startWhoamiHub(t *testing.T, status int, username, displayName string) stri return srv.URL } -// TestParity_AuthStatus exercises both surfaces against the same seeded -// auth store. The Formatted field is authoritative, so we assert byte- -// equality (after stripping trailing whitespace, which the CLI runner -// already trims on its side via runCLI → strings.TrimSpace). -func TestParity_AuthStatus(t *testing.T) { +// TestAuthStatusRendering covers `tap auth status` output against a seeded +// store. This was a parity test until auth_status left MCP: the agent-facing +// replacement is auth_info, which is deliberately a different shape (structured +// and credential-free) rather than a byte-identical peer, so there is nothing +// left to compare against. The rendering assertions — above all that a raw +// access token never reaches stdout — still earn their keep on the CLI alone. +func TestAuthStatusRendering(t *testing.T) { t.Parallel() t.Run("single_hub_auto_resolves_and_validates", func(t *testing.T) { @@ -80,19 +80,15 @@ func TestParity_AuthStatus(t *testing.T) { cliOut, err := env.runCLI("auth", "status") require.NoError(t, err) - mcpOut, err := env.runMCP("auth_status", nil) - require.NoError(t, err) - require.Equal(t, cliOut, mcpOut, - "CLI and MCP auth status must be byte-identical") require.Contains(t, cliOut, "Logged in as alice (Alice Liddell)") // Token rendered by its leading prefix (matches the hub UI), not suffix. require.Contains(t, cliOut, "- Token: thub_parityt... (Bearer)") - // The raw access token must never leak through either surface. + // The raw access token must never leak. require.NotContains(t, cliOut, "thub_paritytoken9999") }) - t.Run("rejected_token_reported_on_both", func(t *testing.T) { + t.Run("rejected_token_reported", func(t *testing.T) { t.Parallel() env := newParityEnv(t) hub := startWhoamiHub(t, http.StatusUnauthorized, "", "") @@ -103,15 +99,13 @@ func TestParity_AuthStatus(t *testing.T) { cliOut, err := env.runCLI("auth", "status") require.NoError(t, err) - mcpOut, err := env.runMCP("auth_status", nil) - require.NoError(t, err) - require.Equal(t, cliOut, mcpOut) require.Contains(t, cliOut, "Failed to validate token") require.Contains(t, cliOut, "- Token: thub_rejecte... (Bearer)") + require.NotContains(t, cliOut, "thub_rejectedtoken00") }) - t.Run("multiple_hubs_reported_on_both", func(t *testing.T) { + t.Run("multiple_hubs_reported", func(t *testing.T) { t.Parallel() env := newParityEnv(t) hubA := startWhoamiHub(t, http.StatusOK, "alice", "") @@ -127,10 +121,7 @@ func TestParity_AuthStatus(t *testing.T) { cliOut, err := env.runCLI("auth", "status") require.NoError(t, err) - mcpOut, err := env.runMCP("auth_status", nil) - require.NoError(t, err) - require.Equal(t, cliOut, mcpOut) require.Contains(t, cliOut, "Logged in as alice") require.Contains(t, cliOut, "Logged in as bob") require.Equal(t, 2, strings.Count(cliOut, "Logged in as ")) @@ -149,12 +140,6 @@ func TestParity_AuthStatus(t *testing.T) { // example.com) while still exercising hub-URL canonicalization. cliOut, err := env.runCLI("auth", "status", "--hub", "HTTPS://Hub.Example.COM/", "--offline") require.NoError(t, err) - mcpOut, err := env.runMCP("auth_status", map[string]any{ - "hub": "HTTPS://Hub.Example.COM/", - "offline": true, - }) - require.NoError(t, err) - require.Equal(t, cliOut, mcpOut) require.True(t, strings.Contains(cliOut, "hub.example.com"), "hub URL should be canonicalized in output; got:\n%s", cliOut) }) @@ -164,9 +149,6 @@ func TestParity_AuthStatus(t *testing.T) { env := newParityEnv(t) cliOut, err := env.runCLI("auth", "status") require.NoError(t, err) - mcpOut, err := env.runMCP("auth_status", nil) - require.NoError(t, err) - require.Equal(t, cliOut, mcpOut) require.Contains(t, cliOut, "No hub logins stored") }) @@ -178,11 +160,6 @@ func TestParity_AuthStatus(t *testing.T) { }) cliOut, err := env.runCLI("auth", "status", "--hub", "https://ghost.example.com") require.NoError(t, err) - mcpOut, err := env.runMCP("auth_status", map[string]any{ - "hub": "https://ghost.example.com", - }) - require.NoError(t, err) - require.Equal(t, cliOut, mcpOut) require.Contains(t, cliOut, "No login stored for https://ghost.example.com") }) } diff --git a/pkg/parity/parity_coverage_test.go b/pkg/parity/parity_coverage_test.go index 4ad460ed..b22d4846 100644 --- a/pkg/parity/parity_coverage_test.go +++ b/pkg/parity/parity_coverage_test.go @@ -85,21 +85,9 @@ var tapMethodToSurfaces = map[string]struct { "ForceUnlock": {CLI: "lock force-release", MCP: "lock_force_release"}, // Repo management - // MCP tool name kept as "repo_init" for backward compatibility with - // existing agent integrations; CLI surface is the canonical `tap keg create` - // (a hidden top-level `tap init` alias also runs it). - "InitKeg": {CLI: "keg create", MCP: "repo_init"}, - - // Config operations - "Config": {CLI: "config", MCP: "config"}, - "ConfigTemplate": {CLI: "config template", MCP: "config_template"}, - - // Archive operations // Note: "import" at top level is ImportFromKeg (live keg import). // "archive import" is Import (archive import). Different commands, - // different Tap methods, same word. - "Export": {CLI: "archive export", MCP: "export"}, - "Import": {CLI: "archive import", MCP: "import"}, + // different Tap methods, same word. Only the live-keg one has an MCP peer. "ImportFromKeg": {CLI: "import", MCP: "import_from_keg"}, // Flights (keg restriction + agent instructions) @@ -109,23 +97,13 @@ var tapMethodToSurfaces = map[string]struct { "EditFlight": {CLI: "flight edit", MCP: "flight_edit"}, "DeleteFlight": {CLI: "flight delete", MCP: "flight_delete"}, - // Keg administration (hub-side). HubListKegs backs `tap keg list`. - "HubListKegs": {CLI: "keg list", MCP: "keg_list"}, - "KegVisibility": {CLI: "keg visibility", MCP: "keg_visibility"}, - - // Namespace administration. - "NamespaceList": {CLI: "namespace list", MCP: "namespace_list"}, + // Keg discovery (hub-side). HubListKegs backs `tap keg list`; on MCP the + // same listing is filtered through the session's active flight cover. + "HubListKegs": {CLI: "keg list", MCP: "keg_list"}, // Agent orientation remains shared. Native plugin installation is an // intentionally CLI-only host operation (see tapMethodsExcluded). "Orient": {CLI: "orient", MCP: "orient"}, - - // Auth: Status has both surfaces (agents need to check auth before - // remote calls); Login is CLI-only — it drives the interactive device - // flow via package-level tapper.AuthLoginDevice (not a *Tap method, so - // it doesn't appear in this map); Logout is a *Tap method intentionally - // excluded from MCP for security — see tapMethodsExcluded below. - "AuthStatus": {CLI: "auth status", MCP: "auth_status"}, } // tapMethodsExcluded lists Tap methods that are intentionally excluded from @@ -168,8 +146,20 @@ var tapMethodsExcluded = map[string]string{ "SetBootstrapFlight": "CLI-only bootstrap step; validates and persists the user-level flight baseline, not an MCP operation", "Use": "writes the project/user keg + flight to config; CLI-only config management by design", "UseStatus": "CLI-only summary of the resolved keg/flight context; config inspection via `tap use`", - "ActiveFlightName": "internal MCP startup helper that resolves the explicit or project flight before connecting", + "ActiveFlightName": "internal pure read of the explicit flight or the loaded cascade's selection; backs Orient and MCP session adoption rather than being an operation of its own", "OrientationForFlight": "internal session-orientation builder used by initialize, orient, and the orient resource", + // Dropped from MCP when the surface was unified behind providers: these + // operate on machine-local Tapper state or perform tenant administration, + // neither of which an agent should reach through either transport. + "AuthStatus": "replaced on MCP by the credential-free auth_info tool; `tap auth status` renders local token state and has no agent-safe peer", + "Config": "reads the local Tapper config cascade; configuration is an external CLI concern, not an MCP operation", + "ConfigTemplate": "emits starter config files for a human to edit; CLI-only setup step", + "InitKeg": "provisions a keg destination on local disk or a hub; CLI-only setup step", + "Export": "writes a keg archive to the local filesystem; CLI-only bulk operation", + "Import": "reads a keg archive from the local filesystem; CLI-only bulk operation (import_from_keg covers the agent-safe node-level path)", + "KegVisibility": "UI-only visibility management; MCP must not flip a keg between public and private", + "NamespaceList": "namespace discovery folded into auth_info's identity payload; the standalone tool was tenant-administration shaped", + "License": "prints bundled license text; CLI-only via `tap version --license`", } // TestCoverage_AllTapMethodsHaveBothSurfaces uses reflection to enumerate @@ -243,7 +233,7 @@ func collectMCPToolNames(t *testing.T) map[string]bool { srv := mcp.NewServer(tap, "test", mcp.KegDefaults{ KegTargetOptions: tapper.KegTargetOptions{Flight: "@local/+parity"}, - }) + }, mcp.ServerOptions{SharedFilesystem: true}) serverTransport, clientTransport := sdkmcp.NewInMemoryTransports() done := make(chan error, 1) diff --git a/pkg/parity/parity_test.go b/pkg/parity/parity_test.go index 158bde0c..783dc6e3 100644 --- a/pkg/parity/parity_test.go +++ b/pkg/parity/parity_test.go @@ -61,10 +61,12 @@ func newParityEnv(t *testing.T) *parityEnv { }) require.NoError(t, err) - // Set up MCP server with in-memory transport. + // Set up MCP server with in-memory transport. Parity is measured against + // the CLI's own MCP peer — `tap mcp` — so this is the shared-filesystem + // surface, local attachment paths included. srv := mcp.NewServer(tap, "test", mcp.KegDefaults{ KegTargetOptions: tapper.KegTargetOptions{Flight: "@local/+parity"}, - }) + }, mcp.ServerOptions{SharedFilesystem: true}) serverTransport, clientTransport := sdkmcp.NewInMemoryTransports() done := make(chan error, 1) From 91b2130503d9b5148263a6351f7a86a8d6750ad6 Mon Sep 17 00:00:00 2001 From: Jared Rickert Date: Tue, 4 Aug 2026 00:03:46 -0500 Subject: [PATCH 6/9] feat(mcp): publish local-path attachment transfers on tap mcp `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. --- pkg/cli/cmd_mcp.go | 20 ++++++++++++++++---- pkg/cli/cmd_mcp_test.go | 20 ++++++++++++++++++++ 2 files changed, 36 insertions(+), 4 deletions(-) create mode 100644 pkg/cli/cmd_mcp_test.go diff --git a/pkg/cli/cmd_mcp.go b/pkg/cli/cmd_mcp.go index 751201c9..869b056a 100644 --- a/pkg/cli/cmd_mcp.go +++ b/pkg/cli/cmd_mcp.go @@ -10,6 +10,7 @@ import ( sdkmcp "github.com/modelcontextprotocol/go-sdk/mcp" "github.com/jlrickert/tapper/pkg/mcp" + "github.com/jlrickert/tapper/pkg/tapper" "github.com/spf13/cobra" ) @@ -51,10 +52,7 @@ per-command permission prompts.`, // Config-driven selection is re-resolved on initialize/orient. defaults.Flight = "" } - srv := mcp.NewServer(deps.Tap, Version, defaults, mcp.ServerOptions{ - Logger: rt.Logger(), - Reporter: deps.InvocationReporter, - }) + srv := mcp.NewServer(deps.Tap, Version, defaults, mcpServerOptions(rt.Logger(), deps.InvocationReporter)) err = srv.Run(cmd.Context(), &sdkmcp.StdioTransport{}) if err != nil && errors.Is(err, io.EOF) { return nil @@ -65,6 +63,20 @@ per-command permission prompts.`, return cmd } +// mcpServerOptions builds the option set for `tap mcp`. It exists as a named +// function so the shared-filesystem choice is assertable without standing up a +// stdio server: dropping it would silently cost `tap mcp` its local-path +// attachment transfers, which is the whole reason the local variant exists. +func mcpServerOptions(logger *slog.Logger, reporter tapper.InvocationReporter) mcp.ServerOptions { + return mcp.ServerOptions{ + Logger: logger, + Reporter: reporter, + // stdio puts the server on the same machine as its agent host, so + // attachment paths in tool arguments name the same files for both sides. + SharedFilesystem: true, + } +} + // buildMCPLogger constructs the structured logger for the MCP server. // Unlike CLI commands, MCP always logs to stderr because stdout is reserved // for JSON-RPC. When a log file is also configured, entries fan out to both. diff --git a/pkg/cli/cmd_mcp_test.go b/pkg/cli/cmd_mcp_test.go new file mode 100644 index 00000000..58116a35 --- /dev/null +++ b/pkg/cli/cmd_mcp_test.go @@ -0,0 +1,20 @@ +package cli + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +// TestMcpServerOptionsSharesFilesystem pins the `tap mcp` half of the split +// attachment surface. Without it, upload_file and upload_image lose +// source_path, download_file is not registered at all, and download_image can +// no longer write to dest_path — a silent capability loss that the pkg/mcp +// tests would not catch, because they construct their own servers. +func TestMcpServerOptionsSharesFilesystem(t *testing.T) { + t.Parallel() + + opts := mcpServerOptions(nil, nil) + require.True(t, opts.SharedFilesystem, + "tap mcp runs on the agent's own machine and must publish local-path attachment transfers") +} From 7881327ae9b7051e919b54662cb3e9db54fbee27 Mon Sep 17 00:00:00 2001 From: Jared Rickert Date: Tue, 4 Aug 2026 00:37:24 -0500 Subject: [PATCH 7/9] feat(cli): add experimental tap launch for agent harnesses MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- pkg/cli/cmd_launch.go | 143 ++++++++++ pkg/cli/cmd_launch_test.go | 125 +++++++++ pkg/cli/cmd_root.go | 2 +- pkg/parity/parity_coverage_test.go | 4 + pkg/tapper/config.go | 59 ++++ pkg/tapper/tap_launch.go | 436 +++++++++++++++++++++++++++++ pkg/tapper/tap_launch_test.go | 275 ++++++++++++++++++ 7 files changed, 1043 insertions(+), 1 deletion(-) create mode 100644 pkg/cli/cmd_launch.go create mode 100644 pkg/cli/cmd_launch_test.go create mode 100644 pkg/tapper/tap_launch.go create mode 100644 pkg/tapper/tap_launch_test.go diff --git a/pkg/cli/cmd_launch.go b/pkg/cli/cmd_launch.go new file mode 100644 index 00000000..2b9d3a74 --- /dev/null +++ b/pkg/cli/cmd_launch.go @@ -0,0 +1,143 @@ +package cli + +// EXPERIMENTAL — see pkg/tapper/tap_launch.go. Undocumented on purpose; this +// command is a testing scaffold and will be redesigned when agents move to the +// hub. + +import ( + "fmt" + "sort" + "strings" + + "github.com/spf13/cobra" + + "github.com/jlrickert/tapper/pkg/tapper" +) + +// NewLaunchCmd builds the `tap launch` command. It resolves a configured agent +// to its model and flight and starts the named harness with that context. +func NewLaunchCmd(deps *Deps) *cobra.Command { + var opts tapper.LaunchOptions + + cmd := &cobra.Command{ + Use: "launch HARNESS [-- ARGS...]", + Short: "start an agent CLI with a configured model and flight (experimental)", + Long: `Start Claude Code, Codex, or pi with the model and flight named by a +configured agent. + +An agent is an alias for a (model, flight) pair: + + agents: + opus: + model: anthropic/claude-opus-4 + flight: +dev + local: + model: ollama/qwen3.6:35b + flight: "@me/+scratch" + +Models are provider-qualified so the launcher knows which protocol the harness +must speak. The agent's flight is exported as TAP_FLIGHT, so a tap mcp session +started inside the harness orients to it. + +Arguments after -- are passed through to the harness. + +Experimental and unstable: expect this to change or disappear.`, + Args: cobra.MinimumNArgs(1), + SilenceUsage: true, + SilenceErrors: true, + ValidArgsFunction: func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) { + if len(args) > 0 { + return nil, cobra.ShellCompDirectiveNoFileComp + } + return tapper.LaunchHarnesses(), cobra.ShellCompDirectiveNoFileComp + }, + RunE: func(cmd *cobra.Command, args []string) error { + opts.Harness = args[0] + opts.Args = args[1:] + + result, err := deps.Tap.Launch(cmd.Context(), opts) + if err != nil { + return err + } + if !opts.DryRun { + return nil + } + + out := cmd.OutOrStdout() + if _, err := fmt.Fprintf(out, "agent %s -> %s/%s\n", + result.Agent, result.Provider, result.Model); err != nil { + return err + } + if result.Flight != "" { + if _, err := fmt.Fprintf(out, "flight: %s\n", result.Flight); err != nil { + return err + } + } + auth := result.Auth + if result.KeySource != "" { + // The variable name, never the key itself. + auth += " (from $" + result.KeySource + ")" + } + if _, err := fmt.Fprintf(out, "auth: %s\n", auth); err != nil { + return err + } + for _, name := range result.StripEnv { + if _, err := fmt.Fprintf(out, "unset: %s (inherited)\n", name); err != nil { + return err + } + } + if _, err := fmt.Fprintln(out, "Would run:"); err != nil { + return err + } + if _, err := fmt.Fprintln(out, " "+strings.Join(result.Argv, " ")); err != nil { + return err + } + if len(result.Env) == 0 { + return nil + } + if _, err := fmt.Fprintln(out, "With environment:"); err != nil { + return err + } + keys := make([]string, 0, len(result.Env)) + for k := range result.Env { + keys = append(keys, k) + } + sort.Strings(keys) + for _, k := range keys { + if _, err := fmt.Fprintf(out, " %s=%s\n", k, result.Env[k]); err != nil { + return err + } + } + return nil + }, + } + + cmd.Flags().StringVar(&opts.Agent, "agent", "", "configured agent alias supplying the model and flight") + cmd.Flags().BoolVar(&opts.DryRun, "dry-run", false, "print the resolved invocation without starting the harness") + mustRegisterFlagCompletion(cmd, "agent", func(_ *cobra.Command, _ []string, _ string) ([]string, cobra.ShellCompDirective) { + return configAgentNames(deps), cobra.ShellCompDirectiveNoFileComp + }) + + return cmd +} + +// configAgentNames returns the agent aliases known to local config +// (best-effort, offline), sorted for stable completion output. +func configAgentNames(deps *Deps) []string { + tap, err := completionTap(deps) + if err != nil { + return nil + } + cfg, err := tap.ConfigService.Config() + if err != nil || cfg == nil { + return nil + } + var names []string + for name := range cfg.Agents() { + if name = strings.TrimSpace(name); name != "" { + names = append(names, name) + } + } + sort.Strings(names) + return names +} diff --git a/pkg/cli/cmd_launch_test.go b/pkg/cli/cmd_launch_test.go new file mode 100644 index 00000000..89f0121a --- /dev/null +++ b/pkg/cli/cmd_launch_test.go @@ -0,0 +1,125 @@ +package cli_test + +import ( + "testing" + + tu "github.com/jlrickert/cli-toolkit/sandbox" + "github.com/stretchr/testify/require" +) + +const launchConfig = `fallbackNamespace: local +agents: + opus: + model: anthropic/claude-opus-4 + flight: +dev + local: + model: ollama/qwen3.6:35b-mlx + flight: "@testuser/+scratch" + lab: + model: ollama/qwen3.6:35b-mlx + baseUrl: http://192.168.50.197:11434/v1 + flight: "@testuser/+scratch" + sub: + model: anthropic/claude-opus-4 + auth: subscription + flight: +dev +` + +func newLaunchSandbox(t *testing.T) *tu.Sandbox { + t.Helper() + sb := NewSandbox(t) + require.NoError(t, sb.Runtime().AtomicWriteFile( + "/home/testuser/.config/tapper/config.yaml", []byte(launchConfig), 0o644)) + return sb +} + +func TestLaunchCommand_DryRunResolvesOllamaThroughOpenAI(t *testing.T) { + t.Parallel() + sb := newLaunchSandbox(t) + + res := NewProcess(t, false, "launch", "codex", "--agent", "local", "--dry-run"). + Run(sb.Context(), sb.Runtime()) + require.NoError(t, res.Err) + + out := string(res.Stdout) + require.Contains(t, out, "agent local -> ollama/qwen3.6:35b-mlx") + require.Contains(t, out, "flight: @testuser/+scratch") + require.Contains(t, out, "codex --model qwen3.6:35b-mlx") + require.Contains(t, out, "OPENAI_BASE_URL=http://localhost:11434/v1") + require.Contains(t, out, "TAP_FLIGHT=@testuser/+scratch") +} + +func TestLaunchCommand_DryRunResolvesAnthropicThroughEnv(t *testing.T) { + t.Parallel() + sb := newLaunchSandbox(t) + + res := NewProcess(t, false, "launch", "claude", "--agent", "opus", "--dry-run"). + Run(sb.Context(), sb.Runtime()) + require.NoError(t, res.Err) + + out := string(res.Stdout) + require.Contains(t, out, "ANTHROPIC_MODEL=claude-opus-4") + require.Contains(t, out, "TAP_FLIGHT=+dev") +} + +func TestLaunchCommand_DryRunPassesThroughExtraArgs(t *testing.T) { + t.Parallel() + sb := newLaunchSandbox(t) + + res := NewProcess(t, false, "launch", "codex", "--agent", "local", "--dry-run", + "--", "--sandbox", "read-only").Run(sb.Context(), sb.Runtime()) + require.NoError(t, res.Err) + require.Contains(t, string(res.Stdout), "codex --model qwen3.6:35b-mlx --sandbox read-only") +} + +// Ollama serves the Anthropic Messages API as well, so Claude Code can drive it +// once ANTHROPIC_BASE_URL points at the server — minus the /v1 suffix, which +// the client appends itself. +func TestLaunchCommand_DryRunDrivesOllamaFromClaude(t *testing.T) { + t.Parallel() + sb := newLaunchSandbox(t) + + res := NewProcess(t, false, "launch", "claude", "--agent", "lab", "--dry-run"). + Run(sb.Context(), sb.Runtime()) + require.NoError(t, res.Err) + + out := string(res.Stdout) + require.Contains(t, out, "ANTHROPIC_BASE_URL=http://192.168.50.197:11434") + require.NotContains(t, out, "ANTHROPIC_BASE_URL=http://192.168.50.197:11434/v1") + require.Contains(t, out, "ANTHROPIC_MODEL=qwen3.6:35b-mlx") +} + +func TestLaunchCommand_ErrorsBeforeLaunchingOnIncompatiblePair(t *testing.T) { + t.Parallel() + sb := newLaunchSandbox(t) + + // A dry run is not needed to prove this: resolution fails first, so no + // harness is ever started. Codex speaks the OpenAI protocol only. + res := NewProcess(t, false, "launch", "codex", "--agent", "opus"). + Run(sb.Context(), sb.Runtime()) + require.Error(t, res.Err) + require.Contains(t, res.Err.Error(), "cannot use a anthropic model") +} + +func TestLaunchCommand_DryRunReportsSubscriptionStrip(t *testing.T) { + t.Parallel() + sb := newLaunchSandbox(t) + + res := NewProcess(t, false, "launch", "claude", "--agent", "sub", "--dry-run"). + Run(sb.Context(), sb.Runtime()) + require.NoError(t, res.Err) + + out := string(res.Stdout) + require.Contains(t, out, "auth: subscription") + require.Contains(t, out, "unset: ANTHROPIC_API_KEY (inherited)") +} + +func TestLaunchCommand_ErrorsOnUnknownAgent(t *testing.T) { + t.Parallel() + sb := newLaunchSandbox(t) + + res := NewProcess(t, false, "launch", "codex", "--agent", "nope", "--dry-run"). + Run(sb.Context(), sb.Runtime()) + require.Error(t, res.Err) + require.Contains(t, res.Err.Error(), `unknown agent "nope"`) +} diff --git a/pkg/cli/cmd_root.go b/pkg/cli/cmd_root.go index e6783268..ac18dd5e 100644 --- a/pkg/cli/cmd_root.go +++ b/pkg/cli/cmd_root.go @@ -311,7 +311,7 @@ func NewRootCmd(deps *Deps) *cobra.Command { NewWatchCmd(deps), } if deps.Profile.IncludeIntegrations { - subcommands = append(subcommands, NewIntegrateCmd(deps), NewHookCmd(deps)) + subcommands = append(subcommands, NewIntegrateCmd(deps), NewHookCmd(deps), NewLaunchCmd(deps)) } var configCmd *cobra.Command if deps.Profile.IncludeConfigCommand { diff --git a/pkg/parity/parity_coverage_test.go b/pkg/parity/parity_coverage_test.go index b22d4846..77bb7302 100644 --- a/pkg/parity/parity_coverage_test.go +++ b/pkg/parity/parity_coverage_test.go @@ -160,6 +160,10 @@ var tapMethodsExcluded = map[string]string{ "KegVisibility": "UI-only visibility management; MCP must not flip a keg between public and private", "NamespaceList": "namespace discovery folded into auth_info's identity payload; the standalone tool was tenant-administration shaped", "License": "prints bundled license text; CLI-only via `tap version --license`", + // Experimental launcher. Starting a process on the operator's machine is + // not an agent operation and must not become an MCP tool. + "Launch": "CLI-only: starts an agent harness as a local subprocess; MCP must never spawn processes on its host", + "ResolveLaunch": "pure resolution half of Launch, exposed so a dry run and a real run cannot drift", } // TestCoverage_AllTapMethodsHaveBothSurfaces uses reflection to enumerate diff --git a/pkg/tapper/config.go b/pkg/tapper/config.go index 6112cc48..5ba3fb83 100644 --- a/pkg/tapper/config.go +++ b/pkg/tapper/config.go @@ -127,6 +127,10 @@ type configDTO struct { // hubs describes configured hubs available to the user, keyed by name. Hubs hubMap `yaml:"hubs,omitempty"` + + // agents names (model, flight) pairs for `tap launch`, keyed by alias. + // Experimental and undocumented; see tap_launch.go. + Agents map[string]AgentEntry `yaml:"agents,omitempty"` } // Config represents the user's tapper configuration. @@ -163,6 +167,34 @@ type HubEntry struct { TokenEnv string `yaml:"tokenEnv,omitempty"` } +// AgentEntry is an alias for a (model, flight) pair plus how to reach and +// authenticate against that model, keyed by name in the agents map and consumed +// by `tap launch`. +// +// Model is provider-qualified ("anthropic/claude-opus-4", "ollama/qwen3.6:35b") +// so the launcher knows which protocol the harness must speak. Flight is a +// flight reference exported to the launched process as TAP_FLIGHT. +// +// BaseURL overrides the provider's endpoint. One value serves both protocols: +// the launcher adds or removes the /v1 suffix to suit whichever the harness +// speaks. It defaults to the local Ollama server for ollama models and is empty +// for hosted providers, leaving the harness on its own endpoint. +// +// Auth selects where credentials come from — inherit (default), subscription, +// or apiKey. APIKeyEnv names the environment variable holding the key; the +// name is configured, never the secret, mirroring HubEntry.TokenEnv. Agents +// therefore hold no secrets, so unlike hubs they need no trust-boundary strip +// and a project config may safely define them. +// +// Experimental: this shape is expected to change when agents move to the hub. +type AgentEntry struct { + Model string `yaml:"model,omitempty"` + Flight string `yaml:"flight,omitempty"` + BaseURL string `yaml:"baseUrl,omitempty"` + Auth string `yaml:"auth,omitempty"` + APIKeyEnv string `yaml:"apiKeyEnv,omitempty"` +} + // KegRef is the (hub, namespace, name) triple a keg alias resolves to. An empty // Hub falls back to defaultHub/fallbackHub; an empty Namespace falls back to // defaultNamespace/fallbackNamespace (see Config.ResolveRef). @@ -403,6 +435,28 @@ func (cfg *Config) Hubs() map[string]HubEntry { return cfg.data.Hubs } +// Agents returns the configured `tap launch` agents keyed by alias. +func (cfg *Config) Agents() map[string]AgentEntry { + if cfg.data == nil { + cfg.data = &configDTO{} + } + if cfg.data.Agents == nil { + return map[string]AgentEntry{} + } + return cfg.data.Agents +} + +// Agent returns the named agent entry. Unlike Hub there are no synthesized +// built-ins: an agent exists only if configured. +func (cfg *Config) Agent(name string) (AgentEntry, bool) { + name = strings.TrimSpace(name) + if name == "" { + return AgentEntry{}, false + } + e, ok := cfg.Agents()[name] + return e, ok +} + // Hub returns the named hub entry. The built-in hubs "local" (filesystem) and // "atlas" (the default remote hub) are synthesized when not explicitly // configured — unless disabled via disableLocalHub / disableAtlasHub, in which @@ -1240,6 +1294,7 @@ func MergeConfig(cfgs ...*Config) *Config { Namespaces: make(map[string]NamespaceRef), KegMap: make([]KegMapEntry, 0), Hubs: make(hubMap), + Agents: make(map[string]AgentEntry), }, } @@ -1294,6 +1349,10 @@ func MergeConfig(cfgs ...*Config) *Config { out.data.Hubs[name] = entry } + for name, entry := range c.data.Agents { + out.data.Agents[name] = entry + } + for ns, ref := range c.data.Namespaces { out.data.Namespaces[ns] = ref } diff --git a/pkg/tapper/tap_launch.go b/pkg/tapper/tap_launch.go new file mode 100644 index 00000000..d85779fc --- /dev/null +++ b/pkg/tapper/tap_launch.go @@ -0,0 +1,436 @@ +package tapper + +// EXPERIMENTAL — `tap launch` is a scaffold for exercising Tapper against a +// chosen model and flight without editing config between runs. It is +// deliberately undocumented: agents are expected to move to Tapper Hub, at +// which point this file and its config shape are torn out and redesigned. +// Nothing else in the package should grow a dependency on it. + +import ( + "context" + "fmt" + "os/exec" + "sort" + "strings" +) + +// Providers understood by the launcher, parsed from an agent model's prefix. +const ( + ProviderAnthropic = "anthropic" + ProviderOpenAI = "openai" + ProviderOllama = "ollama" +) + +// defaultOllamaBaseURL is the local Ollama server. Ollama serves BOTH the +// OpenAI protocol (/v1/chat/completions) and the Anthropic Messages protocol +// (/v1/messages), which is why it is the one provider every harness can use. +const defaultOllamaBaseURL = "http://localhost:11434/v1" + +// Auth modes for an agent, selecting where the harness gets its credentials. +const ( + // AuthInherit passes the ambient environment through untouched. It is the + // default because it matches running the harness bare in your shell. + AuthInherit = "inherit" + // AuthSubscription strips inherited provider key variables so the harness + // falls back to its own stored login. Absence of a key cannot express this + // on its own, because absence means inherit. + AuthSubscription = "subscription" + // AuthAPIKey forwards the variable named by an agent's apiKeyEnv. + AuthAPIKey = "apiKey" +) + +// providerKeyEnv lists the credential variables each provider's clients read. +// AuthSubscription removes these from the child environment. +var providerKeyEnv = map[string][]string{ + ProviderAnthropic: {"ANTHROPIC_API_KEY", "ANTHROPIC_AUTH_TOKEN"}, + ProviderOpenAI: {"OPENAI_API_KEY"}, + ProviderOllama: {"OPENAI_API_KEY", "ANTHROPIC_API_KEY", "ANTHROPIC_AUTH_TOKEN"}, +} + +// LaunchOptions configures behavior for Tap.Launch. +type LaunchOptions struct { + // Harness names the agent CLI to start: claude, codex, or pi. + Harness string + // Agent names an entry in the config's agents map. + Agent string + // DryRun resolves and reports the invocation without executing it. + DryRun bool + // Args are extra arguments appended to the harness invocation. + Args []string +} + +// LaunchResult reports the resolved invocation. Env holds only the overlay +// applied on top of the inherited environment; StripEnv names variables removed +// from it. Neither contains a secret value — a forwarded key is reported by the +// variable it came from. +type LaunchResult struct { + Harness string + Agent string + Provider string + Model string + BaseURL string + Flight string + Auth string + Argv []string + Env map[string]string + StripEnv []string + KeySource string +} + +// launchSpec is one resolved agent, handed to a harness builder. +type launchSpec struct { + provider string + model string + baseURL string + apiKey string + auth string +} + +// harnessAdapter maps a provider onto an invocation for one agent CLI. Absence +// from providers means the harness cannot speak that provider's protocol, which +// is reported rather than launched. +type harnessAdapter struct { + command string + providers map[string]func(spec launchSpec) ([]string, map[string]string) +} + +// openAIBaseURL normalizes a base URL for OpenAI clients, which append +// /chat/completions and therefore expect the /v1 prefix present. +func openAIBaseURL(raw string) string { + trimmed := strings.TrimRight(strings.TrimSpace(raw), "/") + if trimmed == "" { + return "" + } + if strings.HasSuffix(trimmed, "/v1") { + return trimmed + } + return trimmed + "/v1" +} + +// anthropicBaseURL normalizes a base URL for Anthropic clients, which append +// /v1/messages themselves and therefore expect the /v1 suffix absent. One +// configured baseUrl is thus correct for both protocols. +func anthropicBaseURL(raw string) string { + trimmed := strings.TrimRight(strings.TrimSpace(raw), "/") + return strings.TrimSuffix(trimmed, "/v1") +} + +// anthropicProtocol builds the invocation for harnesses speaking the Anthropic +// Messages API. Claude Code selects its model through the environment rather +// than a flag, so argv stays bare. +func anthropicProtocol(command string) func(launchSpec) ([]string, map[string]string) { + return func(spec launchSpec) ([]string, map[string]string) { + env := map[string]string{"ANTHROPIC_MODEL": spec.model} + if base := anthropicBaseURL(spec.baseURL); base != "" { + env["ANTHROPIC_BASE_URL"] = base + } + switch { + case spec.apiKey != "": + env["ANTHROPIC_API_KEY"] = spec.apiKey + case spec.provider == ProviderOllama && spec.auth != AuthSubscription: + // Ollama ignores the value, but without one the client would try + // the stored subscription login against the wrong host. + env["ANTHROPIC_API_KEY"] = "ollama" + } + return []string{command}, env + } +} + +// openAIProtocol builds the invocation for harnesses speaking the OpenAI API. +func openAIProtocol(command string) func(launchSpec) ([]string, map[string]string) { + return func(spec launchSpec) ([]string, map[string]string) { + env := map[string]string{} + if base := openAIBaseURL(spec.baseURL); base != "" { + env["OPENAI_BASE_URL"] = base + } + switch { + case spec.apiKey != "": + env["OPENAI_API_KEY"] = spec.apiKey + case spec.provider == ProviderOllama && spec.auth != AuthSubscription: + env["OPENAI_API_KEY"] = "ollama" + } + return []string{command, "--model", spec.model}, env + } +} + +func harnessAdapters() map[string]harnessAdapter { + return map[string]harnessAdapter{ + "claude": { + command: "claude", + providers: map[string]func(launchSpec) ([]string, map[string]string){ + ProviderAnthropic: anthropicProtocol("claude"), + // Ollama serves /v1/messages, so Claude Code works against it + // once ANTHROPIC_BASE_URL points at the server. + ProviderOllama: anthropicProtocol("claude"), + }, + }, + "codex": { + command: "codex", + providers: map[string]func(launchSpec) ([]string, map[string]string){ + ProviderOpenAI: openAIProtocol("codex"), + ProviderOllama: openAIProtocol("codex"), + }, + }, + "pi": { + command: "pi", + providers: map[string]func(launchSpec) ([]string, map[string]string){ + ProviderOpenAI: openAIProtocol("pi"), + ProviderOllama: openAIProtocol("pi"), + }, + }, + } +} + +// LaunchHarnesses returns the launchable harness names, sorted. It backs shell +// completion the way IntegrateHosts does for `tap integrate`. +func LaunchHarnesses() []string { + adapters := harnessAdapters() + out := make([]string, 0, len(adapters)) + for name := range adapters { + out = append(out, name) + } + sort.Strings(out) + return out +} + +// ParseAgentModel splits a provider-qualified model into its provider and model +// id. An unqualified model is an error rather than a guess, because the +// provider decides which protocol the harness must speak. +func ParseAgentModel(raw string) (provider, model string, err error) { + trimmed := strings.TrimSpace(raw) + if trimmed == "" { + return "", "", fmt.Errorf("agent model is empty") + } + prefix, rest, ok := strings.Cut(trimmed, "/") + if !ok || strings.TrimSpace(rest) == "" { + return "", "", fmt.Errorf( + "agent model %q must be provider-qualified, e.g. %s/, %s/, or %s/", + trimmed, ProviderAnthropic, ProviderOpenAI, ProviderOllama) + } + prefix = strings.ToLower(strings.TrimSpace(prefix)) + switch prefix { + case ProviderAnthropic, ProviderOpenAI, ProviderOllama: + return prefix, strings.TrimSpace(rest), nil + } + return "", "", fmt.Errorf("unknown model provider %q in %q", prefix, trimmed) +} + +// ResolveLaunch resolves options into a complete invocation without running it. +// Launch is this plus execution, so a dry run and a real run cannot drift. +func (t *Tap) ResolveLaunch(opts LaunchOptions) (*LaunchResult, error) { + harness := strings.TrimSpace(opts.Harness) + adapter, ok := harnessAdapters()[harness] + if !ok { + return nil, fmt.Errorf("unknown harness %q (available: %s)", + opts.Harness, strings.Join(LaunchHarnesses(), ", ")) + } + + agentName := strings.TrimSpace(opts.Agent) + if agentName == "" { + return nil, fmt.Errorf("an agent is required: pass --agent") + } + cfg, err := t.ConfigService.Config() + if err != nil { + return nil, err + } + agent, ok := cfg.Agent(agentName) + if !ok { + return nil, fmt.Errorf("unknown agent %q (configured: %s)", + agentName, strings.Join(configuredAgentNames(cfg), ", ")) + } + + provider, model, err := ParseAgentModel(agent.Model) + if err != nil { + return nil, fmt.Errorf("agent %q: %w", agentName, err) + } + build, ok := adapter.providers[provider] + if !ok { + return nil, fmt.Errorf( + "harness %q cannot use a %s model: it speaks a different protocol (supported here: %s)", + harness, provider, strings.Join(adapterProviders(adapter), ", ")) + } + + auth, err := resolveAuthMode(agent) + if err != nil { + return nil, fmt.Errorf("agent %q: %w", agentName, err) + } + + // An explicit baseUrl always wins; Ollama otherwise defaults to the local + // server. Hosted providers stay empty so the harness keeps its own endpoint. + baseURL := strings.TrimSpace(agent.BaseURL) + if baseURL == "" && provider == ProviderOllama { + baseURL = defaultOllamaBaseURL + } + + apiKey, keySource, err := t.resolveAPIKey(agent, auth) + if err != nil { + return nil, fmt.Errorf("agent %q: %w", agentName, err) + } + + argv, env := build(launchSpec{ + provider: provider, + model: model, + baseURL: baseURL, + apiKey: apiKey, + auth: auth, + }) + argv = append(argv, opts.Args...) + if env == nil { + env = map[string]string{} + } + // The launched process resolves its own flight through the normal chain, + // where TAP_FLIGHT outranks project and user config. No new plumbing. + if flight := strings.TrimSpace(agent.Flight); flight != "" { + env["TAP_FLIGHT"] = flight + } + + // Subscription mode has to remove inherited credentials, which an overlay + // cannot express: appending can override a variable but never unset one. + var strip []string + if auth == AuthSubscription { + for _, name := range providerKeyEnv[provider] { + if _, set := env[name]; !set { + strip = append(strip, name) + } + } + sort.Strings(strip) + } + + return &LaunchResult{ + Harness: harness, + Agent: agentName, + Provider: provider, + Model: model, + BaseURL: baseURL, + Flight: strings.TrimSpace(agent.Flight), + Auth: auth, + Argv: argv, + Env: env, + StripEnv: strip, + KeySource: keySource, + }, nil +} + +// resolveAuthMode validates an agent's auth field, defaulting to inherit. +func resolveAuthMode(agent AgentEntry) (string, error) { + switch mode := strings.TrimSpace(agent.Auth); mode { + case "": + if strings.TrimSpace(agent.APIKeyEnv) != "" { + return AuthAPIKey, nil + } + return AuthInherit, nil + case AuthInherit, AuthSubscription, AuthAPIKey: + return mode, nil + default: + return "", fmt.Errorf("unknown auth mode %q (want %s, %s, or %s)", + mode, AuthInherit, AuthSubscription, AuthAPIKey) + } +} + +// resolveAPIKey reads the variable named by apiKeyEnv. The name is configured, +// never the secret, so nothing sensitive lands in a config file. The returned +// source is the variable name, safe to print. +func (t *Tap) resolveAPIKey(agent AgentEntry, auth string) (key, source string, err error) { + name := strings.TrimSpace(agent.APIKeyEnv) + if name == "" { + if auth == AuthAPIKey { + return "", "", fmt.Errorf("auth %q requires apiKeyEnv naming the variable holding the key", AuthAPIKey) + } + return "", "", nil + } + if auth == AuthSubscription { + return "", "", fmt.Errorf("auth %q cannot be combined with apiKeyEnv", AuthSubscription) + } + value := strings.TrimSpace(t.Runtime.Env().Get(name)) + if value == "" { + return "", "", fmt.Errorf("apiKeyEnv names %s but that variable is empty or unset", name) + } + return value, name, nil +} + +// Launch resolves the agent and starts the harness, wiring it to the runtime's +// streams so it runs interactively. With DryRun set it resolves and returns +// without executing. +func (t *Tap) Launch(ctx context.Context, opts LaunchOptions) (*LaunchResult, error) { + resolved, err := t.ResolveLaunch(opts) + if err != nil { + return nil, err + } + if opts.DryRun { + return resolved, nil + } + + if _, err := exec.LookPath(resolved.Argv[0]); err != nil { + return nil, fmt.Errorf("harness %q is not installed or not on PATH: %w", resolved.Argv[0], err) + } + + cmd := exec.CommandContext(ctx, resolved.Argv[0], resolved.Argv[1:]...) + stream := t.Runtime.Stream() + cmd.Stdin = stream.In + cmd.Stdout = stream.Out + cmd.Stderr = stream.Err + cmd.Env = append(stripEnv(t.Runtime.Environ(), resolved.StripEnv), envPairs(resolved.Env)...) + if err := cmd.Run(); err != nil { + return resolved, fmt.Errorf("%s exited: %w", resolved.Harness, err) + } + return resolved, nil +} + +// stripEnv removes the named variables from a KEY=VALUE environment. Unsetting +// is why subscription mode cannot be expressed as an overlay: appending can +// override a variable's value but never make it absent. +func stripEnv(environ []string, names []string) []string { + if len(names) == 0 { + return environ + } + drop := make(map[string]struct{}, len(names)) + for _, n := range names { + drop[n] = struct{}{} + } + out := make([]string, 0, len(environ)) + for _, kv := range environ { + key, _, _ := strings.Cut(kv, "=") + if _, skip := drop[key]; skip { + continue + } + out = append(out, kv) + } + return out +} + +// envPairs renders an overlay as sorted KEY=VALUE pairs. Sorting keeps the +// child environment reproducible, which matters for the dry-run output. +func envPairs(env map[string]string) []string { + keys := make([]string, 0, len(env)) + for k := range env { + keys = append(keys, k) + } + sort.Strings(keys) + out := make([]string, 0, len(keys)) + for _, k := range keys { + out = append(out, k+"="+env[k]) + } + return out +} + +func configuredAgentNames(cfg *Config) []string { + agents := cfg.Agents() + if len(agents) == 0 { + return []string{"none"} + } + out := make([]string, 0, len(agents)) + for name := range agents { + out = append(out, name) + } + sort.Strings(out) + return out +} + +func adapterProviders(a harnessAdapter) []string { + out := make([]string, 0, len(a.providers)) + for p := range a.providers { + out = append(out, p) + } + sort.Strings(out) + return out +} diff --git a/pkg/tapper/tap_launch_test.go b/pkg/tapper/tap_launch_test.go new file mode 100644 index 00000000..1bce16f0 --- /dev/null +++ b/pkg/tapper/tap_launch_test.go @@ -0,0 +1,275 @@ +package tapper_test + +import ( + "testing" + + "github.com/jlrickert/cli-toolkit/sandbox" + "github.com/stretchr/testify/require" + + "github.com/jlrickert/tapper/pkg/tapper" +) + +// newLaunchTap builds a Tap over a sandbox seeded with the given user config. +func newLaunchTap(t *testing.T, userConfig string) *tapper.Tap { + t.Helper() + sb := sandbox.NewSandbox(t, &sandbox.Options{ + Home: "/home/testuser", + User: "testuser", + }) + require.NoError(t, sb.Runtime().AtomicWriteFile( + "/home/testuser/.config/tapper/config.yaml", []byte(userConfig), 0o644)) + tap, err := tapper.NewTap(tapper.TapOptions{Runtime: sb.Runtime()}) + require.NoError(t, err) + return tap +} + +const launchUserConfig = `fallbackNamespace: local +agents: + opus: + model: anthropic/claude-opus-4 + flight: +dev + local: + model: ollama/qwen3.6:35b-mlx + flight: "@testuser/+scratch" + lab: + model: ollama/qwen3.6:35b-mlx + baseUrl: http://192.168.50.197:11434/v1 + bare: + model: claude-opus-4 + flight: +dev + hosted: + model: openai/gpt-5 + sub: + model: anthropic/claude-opus-4 + auth: subscription + work: + model: openai/gpt-5 + apiKeyEnv: WORK_OPENAI_KEY + badauth: + model: openai/gpt-5 + auth: nonsense +` + +func TestParseAgentModel(t *testing.T) { + t.Parallel() + + provider, model, err := tapper.ParseAgentModel("ollama/qwen3.6:35b") + require.NoError(t, err) + require.Equal(t, tapper.ProviderOllama, provider) + // The model id keeps its own colons; only the first slash is the split. + require.Equal(t, "qwen3.6:35b", model) + + provider, model, err = tapper.ParseAgentModel("anthropic/claude-opus-4") + require.NoError(t, err) + require.Equal(t, tapper.ProviderAnthropic, provider) + require.Equal(t, "claude-opus-4", model) + + // An unqualified model is rejected rather than guessed at, because the + // provider decides which protocol the harness must speak. + _, _, err = tapper.ParseAgentModel("claude-opus-4") + require.Error(t, err) + require.Contains(t, err.Error(), "provider-qualified") + + _, _, err = tapper.ParseAgentModel("bedrock/some-model") + require.Error(t, err) + require.Contains(t, err.Error(), "unknown model provider") + + _, _, err = tapper.ParseAgentModel("") + require.Error(t, err) +} + +func TestResolveLaunch_AnthropicOnClaude(t *testing.T) { + t.Parallel() + tap := newLaunchTap(t, launchUserConfig) + + got, err := tap.ResolveLaunch(tapper.LaunchOptions{Harness: "claude", Agent: "opus"}) + require.NoError(t, err) + + require.Equal(t, tapper.ProviderAnthropic, got.Provider) + require.Equal(t, "claude-opus-4", got.Model) + // Claude Code takes its model through the environment, not a flag. + require.Equal(t, []string{"claude"}, got.Argv) + require.Equal(t, "claude-opus-4", got.Env["ANTHROPIC_MODEL"]) + require.Equal(t, "+dev", got.Env["TAP_FLIGHT"]) +} + +func TestResolveLaunch_OllamaOnCodexUsesOpenAIProtocol(t *testing.T) { + t.Parallel() + tap := newLaunchTap(t, launchUserConfig) + + got, err := tap.ResolveLaunch(tapper.LaunchOptions{Harness: "codex", Agent: "local"}) + require.NoError(t, err) + + require.Equal(t, tapper.ProviderOllama, got.Provider) + require.Equal(t, []string{"codex", "--model", "qwen3.6:35b-mlx"}, got.Argv) + require.Equal(t, "http://localhost:11434/v1", got.Env["OPENAI_BASE_URL"]) + require.Equal(t, "@testuser/+scratch", got.Env["TAP_FLIGHT"]) +} + +func TestResolveLaunch_OpenAIOnCodexLeavesDefaultEndpoint(t *testing.T) { + t.Parallel() + tap := newLaunchTap(t, launchUserConfig) + + got, err := tap.ResolveLaunch(tapper.LaunchOptions{Harness: "codex", Agent: "hosted"}) + require.NoError(t, err) + + require.Equal(t, []string{"codex", "--model", "gpt-5"}, got.Argv) + require.NotContains(t, got.Env, "OPENAI_BASE_URL") + // An agent may omit its flight; nothing is exported in that case. + require.NotContains(t, got.Env, "TAP_FLIGHT") +} + +// Ollama serves both /v1/messages and /v1/chat/completions, so it is the one +// provider every harness can drive. Claude Code needs the base URL without the +// /v1 suffix because it appends /v1/messages itself. +func TestResolveLaunch_OllamaOnClaudeUsesAnthropicProtocol(t *testing.T) { + t.Parallel() + tap := newLaunchTap(t, launchUserConfig) + + got, err := tap.ResolveLaunch(tapper.LaunchOptions{Harness: "claude", Agent: "local"}) + require.NoError(t, err) + + require.Equal(t, []string{"claude"}, got.Argv) + require.Equal(t, "qwen3.6:35b-mlx", got.Env["ANTHROPIC_MODEL"]) + require.Equal(t, "http://localhost:11434", got.Env["ANTHROPIC_BASE_URL"]) + require.Equal(t, "ollama", got.Env["ANTHROPIC_API_KEY"]) +} + +// One configured baseUrl is correct for both protocols: the launcher adds the +// /v1 suffix for OpenAI clients and removes it for Anthropic ones. +func TestResolveLaunch_BaseURLNormalizesPerProtocol(t *testing.T) { + t.Parallel() + tap := newLaunchTap(t, launchUserConfig) + + viaClaude, err := tap.ResolveLaunch(tapper.LaunchOptions{Harness: "claude", Agent: "lab"}) + require.NoError(t, err) + require.Equal(t, "http://192.168.50.197:11434", viaClaude.Env["ANTHROPIC_BASE_URL"]) + + viaCodex, err := tap.ResolveLaunch(tapper.LaunchOptions{Harness: "codex", Agent: "lab"}) + require.NoError(t, err) + require.Equal(t, "http://192.168.50.197:11434/v1", viaCodex.Env["OPENAI_BASE_URL"]) +} + +func TestResolveLaunch_RejectsIncompatibleProvider(t *testing.T) { + t.Parallel() + tap := newLaunchTap(t, launchUserConfig) + + // Codex speaks the OpenAI protocol and the Anthropic API is not that, so + // this pair stays refused. + _, err := tap.ResolveLaunch(tapper.LaunchOptions{Harness: "codex", Agent: "opus"}) + require.Error(t, err) + require.Contains(t, err.Error(), "cannot use a anthropic model") + + // Symmetrically, Claude Code cannot drive a hosted OpenAI model. + _, err = tap.ResolveLaunch(tapper.LaunchOptions{Harness: "claude", Agent: "hosted"}) + require.Error(t, err) + require.Contains(t, err.Error(), "cannot use a openai model") +} + +func TestResolveLaunch_SubscriptionStripsInheritedKeys(t *testing.T) { + t.Parallel() + tap := newLaunchTap(t, launchUserConfig) + + got, err := tap.ResolveLaunch(tapper.LaunchOptions{Harness: "claude", Agent: "sub"}) + require.NoError(t, err) + + require.Equal(t, tapper.AuthSubscription, got.Auth) + // Absence of a key cannot express "use my login", because absence means + // inherit. The inherited variables must be actively removed. + require.Contains(t, got.StripEnv, "ANTHROPIC_API_KEY") + require.Contains(t, got.StripEnv, "ANTHROPIC_AUTH_TOKEN") + require.NotContains(t, got.Env, "ANTHROPIC_API_KEY") +} + +func TestResolveLaunch_APIKeyEnvForwardsByName(t *testing.T) { + t.Parallel() + sb := sandbox.NewSandbox(t, &sandbox.Options{Home: "/home/testuser", User: "testuser"}) + require.NoError(t, sb.Runtime().AtomicWriteFile( + "/home/testuser/.config/tapper/config.yaml", []byte(launchUserConfig), 0o644)) + require.NoError(t, sb.Runtime().Env().Set("WORK_OPENAI_KEY", "sk-secret-value")) + tap, err := tapper.NewTap(tapper.TapOptions{Runtime: sb.Runtime()}) + require.NoError(t, err) + + got, err := tap.ResolveLaunch(tapper.LaunchOptions{Harness: "codex", Agent: "work"}) + require.NoError(t, err) + + require.Equal(t, "sk-secret-value", got.Env["OPENAI_API_KEY"]) + // The reported source is the variable name, so it stays safe to print. + require.Equal(t, "WORK_OPENAI_KEY", got.KeySource) +} + +func TestResolveLaunch_APIKeyEnvErrorsWhenUnset(t *testing.T) { + t.Parallel() + tap := newLaunchTap(t, launchUserConfig) + + _, err := tap.ResolveLaunch(tapper.LaunchOptions{Harness: "codex", Agent: "work"}) + require.Error(t, err) + require.Contains(t, err.Error(), "WORK_OPENAI_KEY") +} + +func TestResolveLaunch_RejectsBadAuthMode(t *testing.T) { + t.Parallel() + tap := newLaunchTap(t, launchUserConfig) + + _, err := tap.ResolveLaunch(tapper.LaunchOptions{Harness: "codex", Agent: "badauth"}) + require.Error(t, err) + require.Contains(t, err.Error(), "unknown auth mode") +} + +func TestResolveLaunch_ErrorsOnUnknownInputs(t *testing.T) { + t.Parallel() + tap := newLaunchTap(t, launchUserConfig) + + _, err := tap.ResolveLaunch(tapper.LaunchOptions{Harness: "nope", Agent: "opus"}) + require.Error(t, err) + require.Contains(t, err.Error(), "unknown harness") + + _, err = tap.ResolveLaunch(tapper.LaunchOptions{Harness: "claude", Agent: "missing"}) + require.Error(t, err) + require.Contains(t, err.Error(), "unknown agent") + + _, err = tap.ResolveLaunch(tapper.LaunchOptions{Harness: "claude"}) + require.Error(t, err) + require.Contains(t, err.Error(), "an agent is required") + + _, err = tap.ResolveLaunch(tapper.LaunchOptions{Harness: "claude", Agent: "bare"}) + require.Error(t, err) + require.Contains(t, err.Error(), "provider-qualified") +} + +func TestResolveLaunch_AppendsPassthroughArgs(t *testing.T) { + t.Parallel() + tap := newLaunchTap(t, launchUserConfig) + + got, err := tap.ResolveLaunch(tapper.LaunchOptions{ + Harness: "codex", Agent: "hosted", Args: []string{"--sandbox", "read-only"}, + }) + require.NoError(t, err) + require.Equal(t, []string{"codex", "--model", "gpt-5", "--sandbox", "read-only"}, got.Argv) +} + +func TestResolveLaunch_ReadsAgentsFromProjectConfig(t *testing.T) { + t.Parallel() + sb := sandbox.NewSandbox(t, &sandbox.Options{Home: "/home/testuser", User: "testuser"}) + require.NoError(t, sb.Setwd("/home/testuser/work/project")) + require.NoError(t, sb.Runtime().AtomicWriteFile( + "/home/testuser/.config/tapper/config.yaml", []byte("fallbackNamespace: local\n"), 0o644)) + // Agents carry no credentials, so unlike hubs they survive the project + // config's trust boundary. + require.NoError(t, sb.Runtime().AtomicWriteFile( + "/home/testuser/work/project/.tapper/config.yaml", + []byte("agents:\n proj:\n model: openai/gpt-5\n flight: +proj\n"), 0o644)) + + tap, err := tapper.NewTap(tapper.TapOptions{Runtime: sb.Runtime()}) + require.NoError(t, err) + + got, err := tap.ResolveLaunch(tapper.LaunchOptions{Harness: "codex", Agent: "proj"}) + require.NoError(t, err) + require.Equal(t, "gpt-5", got.Model) + require.Equal(t, "+proj", got.Env["TAP_FLIGHT"]) +} + +func TestLaunchHarnesses(t *testing.T) { + t.Parallel() + require.Equal(t, []string{"claude", "codex", "pi"}, tapper.LaunchHarnesses()) +} From 4d2aadd7b2ed525c941789d2fdfa6b7bfefb0abc Mon Sep 17 00:00:00 2001 From: Jared Rickert Date: Tue, 4 Aug 2026 01:11:24 -0500 Subject: [PATCH 8/9] fix(mcp): state the recovery situation in the orientation payload MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- pkg/mcp/session_flight.go | 16 +++++++++++++++- pkg/mcp/tools_orient.go | 11 +++++++++-- pkg/tapper/tap_orient.go | 16 ++++++++++++++++ pkg/tapper/tap_orient_test.go | 20 ++++++++++++++++++++ 4 files changed, 60 insertions(+), 3 deletions(-) diff --git a/pkg/mcp/session_flight.go b/pkg/mcp/session_flight.go index 28fddcf3..cf40b595 100644 --- a/pkg/mcp/session_flight.go +++ b/pkg/mcp/session_flight.go @@ -13,6 +13,20 @@ import ( var errMCPFlightRequired = errors.New("no flight is selected; KEG tools are locked. Inspect flights through MCP with `list_flights` and `flight_show`, ask the user to select a flight in Tapper configuration, then orient again") +// failedOrientationPayload describes a selection that was made but could not be +// resolved. It deliberately does not reuse errMCPFlightRequired: reporting "no +// flight is selected" when one was selected and merely failed to resolve sends +// the reader looking for missing configuration instead of the real fault, which +// is usually a wrong flight name or an unreachable hub. +func failedOrientationPayload(err error) string { + return "This session could not establish flight authority: " + err.Error() + + "\n\nKEG tools are locked until it does. Call `list_flights` to see what" + + " actually exists, then ask the user to correct the selected flight in" + + " Tapper configuration and call `orient` again on this same connection." + + " An empty flight list usually means this machine is not bootstrapped or" + + " not authenticated to the hub that hosts the flight." +} + var recoveryToolNames = map[string]bool{ "orient": true, "list_flights": true, @@ -86,7 +100,7 @@ func (g *sessionFlightGate) refresh(ctx context.Context, sessionID string) (*ori return current, err } // Initialization must remain connectable for recovery. - recovery := &orientationContext{payload: errMCPFlightRequired.Error(), recovery: true, warnings: []string{err.Error()}} + recovery := &orientationContext{payload: failedOrientationPayload(err), recovery: true, warnings: []string{err.Error()}} state := g.state(sessionID) state.mu.Lock() state.current = recovery diff --git a/pkg/mcp/tools_orient.go b/pkg/mcp/tools_orient.go index 1220933c..ee604117 100644 --- a/pkg/mcp/tools_orient.go +++ b/pkg/mcp/tools_orient.go @@ -20,8 +20,15 @@ func registerOrientTools(srv *sdkmcp.Server, tap *tapper.Tap, defaults KegDefaul func registerOrient(srv *sdkmcp.Server, tap *tapper.Tap, defaults KegDefaults) { sdkmcp.AddTool(srv, &sdkmcp.Tool{ - Name: "orient", - Description: "Refresh this session's flight authority and return the shared Tapper orientation payload.", + Name: "orient", + // The imperative lives here, not only in the server instructions, because + // tool descriptions are the one thing every model reads. A weaker model + // given only server instructions will not infer that it must orient. + Description: "Call this first, before any other KEG tool. Establishes this session's " + + "flight authority and returns the Tapper orientation payload listing the KEGs you " + + "may use. While no flight is active the KEG tools are hidden and only orient, " + + "list_flights, flight_show, and auth_info are available; call orient again after " + + "the user selects a flight to unlock them.", Annotations: &sdkmcp.ToolAnnotations{ ReadOnlyHint: true, OpenWorldHint: boolPtr(false), diff --git a/pkg/tapper/tap_orient.go b/pkg/tapper/tap_orient.go index ad58779c..c45d583a 100644 --- a/pkg/tapper/tap_orient.go +++ b/pkg/tapper/tap_orient.go @@ -375,6 +375,22 @@ func BuildOrientationPayload(flight *Flight, flightNote string, kegs []Orientati b.WriteString("\nCall `keg_settings` for the selected KEG or KEGs before operating in them; targeted settings include KEG-level instructions.\n\n") } + if flight == nil { + // Recovery. Say so in the payload itself: 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. Without this the only signal is an + // absence — an empty KEG table — which weaker models do not act on. + b.WriteString("## Flight\n\n") + b.WriteString("No flight is selected, so this session is in recovery mode ") + b.WriteString("and the KEG tools are locked. Only `orient`, `list_flights`, ") + b.WriteString("`flight_show`, and `auth_info` are available.\n\n") + b.WriteString("To recover:\n\n") + b.WriteString("1. Call `list_flights` to see what is available.\n") + b.WriteString("2. Ask the user to select a flight in Tapper configuration. ") + b.WriteString("Flights are selected outside MCP; an agent cannot select one itself.\n") + b.WriteString("3. Call `orient` again on this same connection to pick it up.\n\n") + } + if flight != nil { b.WriteString("## Flight\n\n") if flight.Name != "" { diff --git a/pkg/tapper/tap_orient_test.go b/pkg/tapper/tap_orient_test.go index be90991c..74d7a212 100644 --- a/pkg/tapper/tap_orient_test.go +++ b/pkg/tapper/tap_orient_test.go @@ -438,3 +438,23 @@ func TestTap_IntegrateHosts_IsSortedAndIncludesDefaults(t *testing.T) { require.LessOrEqual(t, hosts[i-1], hosts[i], "IntegrateHosts must be sorted") } } + +// TestTap_Orient_RecoveryPayloadStatesTheSituation pins the recovery guidance. +// The MCP tool list is filtered to the recovery set, so an agent never gets to +// call a locked tool and see the error explaining why — which left the empty +// KEG table as the only signal, and weaker models do not act on an absence. +func TestTap_Orient_RecoveryPayloadStatesTheSituation(t *testing.T) { + t.Parallel() + tap := newOrientTap(t) + + payload, err := tap.Orient(context.Background(), tapper.OrientOptions{}) + require.NoError(t, err) + + require.Contains(t, payload, "No flight is selected") + require.Contains(t, payload, "recovery mode") + require.Contains(t, payload, "KEG tools are locked") + require.Contains(t, payload, "`list_flights`") + require.Contains(t, payload, "Call `orient` again") + // The payload is the MCP-facing surface and never names CLI commands. + require.NotContains(t, payload, "`tap ") +} From ad14c15de95cb2cc18fe4024741cd27b5635ecbb Mon Sep 17 00:00:00 2001 From: Jared Rickert Date: Tue, 4 Aug 2026 01:22:04 -0500 Subject: [PATCH 9/9] refactor(mcp): disable the deprecated graph tool MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- docs/ai-coding-agents/mcp-setup.md | 1 - integrations/content/tool-inventory.md | 1 - .../claude/tapper/skills/tapper/SKILL.md | 1 - .../codex/tapper/skills/tapper/SKILL.md | 1 - pkg/mcp/server.go | 1 - pkg/mcp/server_test.go | 30 ++++++-------- pkg/mcp/tools_graph.go | 40 ------------------- pkg/parity/parity_coverage_test.go | 2 +- 8 files changed, 13 insertions(+), 64 deletions(-) delete mode 100644 pkg/mcp/tools_graph.go diff --git a/docs/ai-coding-agents/mcp-setup.md b/docs/ai-coding-agents/mcp-setup.md index f8459daf..5087ad2d 100644 --- a/docs/ai-coding-agents/mcp-setup.md +++ b/docs/ai-coding-agents/mcp-setup.md @@ -158,7 +158,6 @@ authenticated Hub identity; hosted MCP reports its single authenticated user. | Tool | Description | | --- | --- | | `import_from_keg` | Import nodes from another keg | -| `graph` | Render a keg graph | | `orient` | Return the shared KEG system orientation payload | | `list_flights`, `flight_show` | Discover and inspect visible flights | | `flight_create`, `flight_edit`, `flight_delete` | Manage Hub-backed flights when the active flight grants `manage_flights` and the identity owns/administers the target namespace | diff --git a/integrations/content/tool-inventory.md b/integrations/content/tool-inventory.md index a1016de2..68eef48c 100644 --- a/integrations/content/tool-inventory.md +++ b/integrations/content/tool-inventory.md @@ -10,7 +10,6 @@ | `mcp__tapper__cat` | Read one or more node bodies. Supports `meta_only`, `content_only`, `stats_only`, and `tag` expression selection as an alternative to explicit node IDs. | | `mcp__tapper__links` | Outbound links from a node. | | `mcp__tapper__backlinks` | Inbound links to a node. | -| `mcp__tapper__graph` | Graph traversal for multi-hop relationships. | | `mcp__tapper__list_indexes`, `mcp__tapper__index_cat` | Read generated index files (tag index, changelog, and others). | | `mcp__tapper__keg_settings` | Read targeted title, summary, updated metadata, and instructions for one or more selected KEGs; batches accept up to 100 canonical references. | diff --git a/integrations/rendered/claude/tapper/skills/tapper/SKILL.md b/integrations/rendered/claude/tapper/skills/tapper/SKILL.md index e0d58292..f0d6350f 100644 --- a/integrations/rendered/claude/tapper/skills/tapper/SKILL.md +++ b/integrations/rendered/claude/tapper/skills/tapper/SKILL.md @@ -62,7 +62,6 @@ no KEGs. | `mcp__tapper__cat` | Read one or more node bodies. Supports `meta_only`, `content_only`, `stats_only`, and `tag` expression selection as an alternative to explicit node IDs. | | `mcp__tapper__links` | Outbound links from a node. | | `mcp__tapper__backlinks` | Inbound links to a node. | -| `mcp__tapper__graph` | Graph traversal for multi-hop relationships. | | `mcp__tapper__list_indexes`, `mcp__tapper__index_cat` | Read generated index files (tag index, changelog, and others). | | `mcp__tapper__keg_settings` | Read targeted title, summary, updated metadata, and instructions for one or more selected KEGs; batches accept up to 100 canonical references. | diff --git a/integrations/rendered/codex/tapper/skills/tapper/SKILL.md b/integrations/rendered/codex/tapper/skills/tapper/SKILL.md index e0d58292..f0d6350f 100644 --- a/integrations/rendered/codex/tapper/skills/tapper/SKILL.md +++ b/integrations/rendered/codex/tapper/skills/tapper/SKILL.md @@ -62,7 +62,6 @@ no KEGs. | `mcp__tapper__cat` | Read one or more node bodies. Supports `meta_only`, `content_only`, `stats_only`, and `tag` expression selection as an alternative to explicit node IDs. | | `mcp__tapper__links` | Outbound links from a node. | | `mcp__tapper__backlinks` | Inbound links to a node. | -| `mcp__tapper__graph` | Graph traversal for multi-hop relationships. | | `mcp__tapper__list_indexes`, `mcp__tapper__index_cat` | Read generated index files (tag index, changelog, and others). | | `mcp__tapper__keg_settings` | Read targeted title, summary, updated metadata, and instructions for one or more selected KEGs; batches accept up to 100 canonical references. | diff --git a/pkg/mcp/server.go b/pkg/mcp/server.go index f3d7b47d..0c76de0b 100644 --- a/pkg/mcp/server.go +++ b/pkg/mcp/server.go @@ -87,7 +87,6 @@ func NewServer(tap *tapper.Tap, version string, defaults KegDefaults, opts ...Se registerWriteTools(srv, tap, defaults) registerIndexTools(srv, tap, defaults) registerSnapshotTools(srv, tap, defaults) - registerGraphTools(srv, tap, defaults) registerOrientTools(srv, tap, defaults) registerSchemaTools(srv, tap, defaults) registerFileTools(srv, tap, defaults, opt.SharedFilesystem) diff --git a/pkg/mcp/server_test.go b/pkg/mcp/server_test.go index 082f9c65..0f780bb8 100644 --- a/pkg/mcp/server_test.go +++ b/pkg/mcp/server_test.go @@ -147,7 +147,7 @@ func TestMCP_ToolsList(t *testing.T) { "keg_settings", "keg_settings_edit", "stats", "create", "edit", "meta", "remove", "move", "index", "list_indexes", "index_cat", "doctor", "node_history", "node_snapshot", "node_snapshot_view", "node_restore", "list_files", "list_images", "delete_file", "delete_image", - "upload_file", "upload_image", "download_image", "graph", "orient", "import_from_keg", + "upload_file", "upload_image", "download_image", "orient", "import_from_keg", "lock_acquire", "lock_release", "lock_status", "lock_force_release", "list_flights", "flight_show", "flight_create", "flight_edit", "flight_delete", "schema_list", "schema_read", "schema_create", "schema_edit", "schema_delete", "validate", @@ -189,7 +189,7 @@ func TestMCP_CommonAgentSafeSurface(t *testing.T) { "keg_settings_edit", "stats", "create", "edit", "meta", "remove", "move", "index", "list_indexes", "index_cat", "node_history", "node_snapshot", - "node_snapshot_view", "node_restore", "graph", "orient", + "node_snapshot_view", "node_restore", "orient", "list_files", "list_images", "delete_file", "delete_image", "upload_file", "upload_image", "download_image", "schema_list", "schema_read", "schema_create", "schema_edit", @@ -1847,7 +1847,11 @@ func TestMCP_ImportMissingFile(t *testing.T) { // --- graph tool tests --- -func TestMCP_ToolsList_IncludesGraphTool(t *testing.T) { +// TestMCP_GraphToolIsDisabled pins the deprecation. graph rendered a standalone +// HTML page that an agent cannot display, so returning it as tool text spent +// context on markup nobody reads. `tap graph --output` still serves the case +// that works; the tool stays off MCP until the feature is removed outright. +func TestMCP_GraphToolIsDisabled(t *testing.T) { t.Parallel() session, ctx := newTestSession(t) @@ -1858,24 +1862,15 @@ func TestMCP_ToolsList_IncludesGraphTool(t *testing.T) { for i, tool := range res.Tools { names[i] = tool.Name } + require.NotContains(t, names, "graph") - require.Contains(t, names, "graph") -} - -func TestMCP_Graph(t *testing.T) { - t.Parallel() - session, ctx := newTestSession(t) - - res, err := session.CallTool(ctx, &sdkmcp.CallToolParams{ + called, err := session.CallTool(ctx, &sdkmcp.CallToolParams{ Name: "graph", Arguments: map[string]any{}, }) - require.NoError(t, err) - text := extractText(t, res) - require.False(t, res.IsError, "graph returned error: %s", text) - require.Contains(t, text, "") - require.Contains(t, text, "KEG Graph") - require.Contains(t, text, "__KEG__") + if err == nil { + require.True(t, called.IsError, "graph must not be callable") + } } func extractText(t *testing.T, res *sdkmcp.CallToolResult) string { @@ -1944,7 +1939,6 @@ func TestMCP_ToolAnnotations_AllPresent(t *testing.T) { "upload_file", "upload_image", "lock_acquire", "lock_release", "import_from_keg", - "graph", } for _, name := range writeTools { tool, ok := byName[name] diff --git a/pkg/mcp/tools_graph.go b/pkg/mcp/tools_graph.go deleted file mode 100644 index 12fc5deb..00000000 --- a/pkg/mcp/tools_graph.go +++ /dev/null @@ -1,40 +0,0 @@ -package mcp - -import ( - "context" - - sdkmcp "github.com/modelcontextprotocol/go-sdk/mcp" - - "github.com/jlrickert/tapper/pkg/tapper" -) - -func registerGraphTools(srv *sdkmcp.Server, tap *tapper.Tap, defaults KegDefaults) { - registerGraph(srv, tap, defaults) -} - -// --- graph --- - -type graphInput struct { - Keg string `json:"keg,omitempty" jsonschema:"keg alias (uses default if empty)"` -} - -func registerGraph(srv *sdkmcp.Server, tap *tapper.Tap, defaults KegDefaults) { - sdkmcp.AddTool(srv, &sdkmcp.Tool{ - Name: "graph", - Description: "Generate a self-contained HTML page visualizing the KEG node graph", - Annotations: &sdkmcp.ToolAnnotations{ - DestructiveHint: boolPtr(false), - OpenWorldHint: boolPtr(false), - }, - }, func(ctx context.Context, req *sdkmcp.CallToolRequest, in graphInput) (*sdkmcp.CallToolResult, any, error) { - opts := tapper.GraphOptions{ - KegTargetOptions: resolveKegTarget(ctx, in.Keg, defaults), - } - - html, err := tap.Graph(ctx, opts) - if err != nil { - return errorResult(err), nil, nil - } - return textResult(html), nil, nil - }) -} diff --git a/pkg/parity/parity_coverage_test.go b/pkg/parity/parity_coverage_test.go index 77bb7302..4bac65fd 100644 --- a/pkg/parity/parity_coverage_test.go +++ b/pkg/parity/parity_coverage_test.go @@ -37,7 +37,6 @@ var tapMethodToSurfaces = map[string]struct { "KegSettings": {CLI: "keg settings", MCP: "keg_settings"}, "KegConfigEdit": {CLI: "keg settings edit", MCP: "keg_settings_edit"}, "Stats": {CLI: "stats", MCP: "stats"}, - "Graph": {CLI: "graph", MCP: "graph"}, "ListIndexes": {CLI: "index list", MCP: "list_indexes"}, "IndexCat": {CLI: "index get", MCP: "index_cat"}, "Doctor": {CLI: "doctor", MCP: "doctor"}, @@ -160,6 +159,7 @@ var tapMethodsExcluded = map[string]string{ "KegVisibility": "UI-only visibility management; MCP must not flip a keg between public and private", "NamespaceList": "namespace discovery folded into auth_info's identity payload; the standalone tool was tenant-administration shaped", "License": "prints bundled license text; CLI-only via `tap version --license`", + "Graph": "deprecated and disabled on MCP: renders a standalone HTML page an agent cannot display, and returning ~8KB of markup as tool text is pure context cost; `tap graph --output` remains until the feature is removed", // Experimental launcher. Starting a process on the operator's machine is // not an agent operation and must not become an MCP tool. "Launch": "CLI-only: starts an agent harness as a local subprocess; MCP must never spawn processes on its host",