diff --git a/pkg/cli/cmd_namespace.go b/pkg/cli/cmd_namespace.go index c598b41..d17d2fd 100644 --- a/pkg/cli/cmd_namespace.go +++ b/pkg/cli/cmd_namespace.go @@ -45,16 +45,26 @@ func newNamespaceListCmd(deps *Deps) *cobra.Command { cmd := &cobra.Command{ Use: "list", Short: "list the namespaces you belong to", - Args: cobra.NoArgs, + Long: `List the namespaces you belong to as NAMESPACE, KIND, ROLE, HUB. + +With no --hub every configured hub is queried and each row names its source +hub, so the same namespace name on two hubs stays two distinct rows. A hub that +cannot be reached is reported on stderr and the remaining hubs still list.`, + Args: cobra.NoArgs, RunE: func(cmd *cobra.Command, _ []string) error { // --hub is the global keg-resolution flag (default: the resolved default hub). opts := tapper.NamespaceListOptions{Hub: globalKegTarget(deps).Hub} - nss, err := deps.Tap.NamespaceList(cmd.Context(), opts) + res, err := deps.Tap.NamespaceList(cmd.Context(), opts) if err != nil { return err } - for _, ns := range nss { - fmt.Fprintf(cmd.OutOrStdout(), "@%s\t%s\t%s\n", ns.Name, ns.Kind, ns.Role) + for _, ns := range res.Namespaces { + fmt.Fprintf(cmd.OutOrStdout(), "@%s\t%s\t%s\t%s\n", ns.Name, ns.Kind, ns.Role, ns.Hub) + } + // Warnings go to stderr so the rows stay pipeable, and name the hub + // that failed so a partial listing is never mistaken for a full one. + for _, w := range res.Warnings { + fmt.Fprintf(cmd.ErrOrStderr(), "warning: %s\n", w) } return nil }, diff --git a/pkg/keg/keg_integer_schema_test.go b/pkg/keg/keg_integer_schema_test.go new file mode 100644 index 0000000..6c85c87 --- /dev/null +++ b/pkg/keg/keg_integer_schema_test.go @@ -0,0 +1,84 @@ +package keg_test + +import ( + "testing" + + "github.com/jlrickert/tapper/pkg/keg" + "github.com/stretchr/testify/require" +) + +// changeSchema is the schema from tapper#91: a required field typed integer. +const changeSchema = `type: change +meta: + type: object + required: [type, contract_count] + properties: + type: {const: change} + contract_count: {type: integer, minimum: 1} +markdown: + requireTitle: true +` + +// TestCreate_IntegerFrontmatterSatisfiesSchema is the issue's reproduction, end +// to end through the real create path rather than at the metadata layer alone. +// Before the fix this failed with: +// +// validating /properties/contract_count: type: 1 has type "string", want "integer" +// +// which was a closed loop: the node could not be created, and meta could not +// repair it because meta validates the whole node including markdown sections. +func TestCreate_IntegerFrontmatterSatisfiesSchema(t *testing.T) { + fx := NewSandbox(t) + ctx := fx.Context() + k := keg.NewLocalKeg(newTestMemoryRepo(fx.Runtime()), fx.Runtime()) + require.NoError(t, k.Init(ctx)) + require.NoError(t, k.CreateSchema(ctx, "change", []byte(changeSchema))) + + body := "---\ntype: change\ncontract_count: 1\n---\n\n# Widen the contract\n\nBody text.\n" + results, err := k.CreateNodes(ctx, []keg.NodeCreate{ + {Key: "n", Schema: "change", Body: []byte(body)}, + }) + require.NoError(t, err) + require.Len(t, results, 1) + if v := results[0].Validation; v != nil { + require.True(t, v.Valid, "schema validation failed: %+v", v) + } + + // And the value is stored as a number, not a quoted string. + meta, err := k.GetMeta(ctx, results[0].ID) + require.NoError(t, err) + require.Contains(t, meta.ToYAML(), "contract_count: 1") + require.NotContains(t, meta.ToYAML(), `contract_count: "1"`) +} + +// TestUpdate_IntegerFrontmatterSurvivesEdit covers the edit half of the issue: +// a body carrying inline frontmatter runs through the same metadata path. +func TestUpdate_IntegerFrontmatterSurvivesEdit(t *testing.T) { + fx := NewSandbox(t) + ctx := fx.Context() + k := keg.NewLocalKeg(newTestMemoryRepo(fx.Runtime()), fx.Runtime()) + require.NoError(t, k.Init(ctx)) + require.NoError(t, k.CreateSchema(ctx, "change", []byte(changeSchema))) + + results, err := k.CreateNodes(ctx, []keg.NodeCreate{{ + Key: "n", + Schema: "change", + Body: []byte("---\ntype: change\ncontract_count: 1\n---\n\n# Title\n\nBody.\n"), + }}) + require.NoError(t, err) + + _, err = k.UpdateNode(ctx, keg.NodeUpdateOptions{ + ID: results[0].ID, + Schema: "change", + Content: []byte("---\ntype: change\ncontract_count: 7\n---\n\n# Title\n\nEdited.\n"), + HasContent: true, + // Writes are guarded; carry the hash the create returned. + ExpectedHash: results[0].Hash, + }) + require.NoError(t, err) + + meta, err := k.GetMeta(ctx, results[0].ID) + require.NoError(t, err) + require.Contains(t, meta.ToYAML(), "contract_count: 7") + require.NotContains(t, meta.ToYAML(), `contract_count: "7"`) +} diff --git a/pkg/keg/node_meta.go b/pkg/keg/node_meta.go index a702ad9..7ce0cb2 100644 --- a/pkg/keg/node_meta.go +++ b/pkg/keg/node_meta.go @@ -4,7 +4,9 @@ import ( "bytes" "context" "fmt" + "math" "sort" + "strconv" "strings" "time" @@ -524,11 +526,12 @@ func valueToYAMLNode(v any) *yaml.Node { case bool: return &yaml.Node{Kind: yaml.ScalarNode, Tag: "!!bool", Value: fmt.Sprint(t)} case int, int8, int16, int32, int64: - return &yaml.Node{Kind: yaml.ScalarNode, Tag: "!!str", Value: fmt.Sprint(t)} + return &yaml.Node{Kind: yaml.ScalarNode, Tag: "!!int", Value: fmt.Sprint(t)} case uint, uint8, uint16, uint32, uint64: - return &yaml.Node{Kind: yaml.ScalarNode, Tag: "!!str", Value: fmt.Sprint(t)} + return &yaml.Node{Kind: yaml.ScalarNode, Tag: "!!int", Value: fmt.Sprint(t)} case float32, float64: - return &yaml.Node{Kind: yaml.ScalarNode, Tag: "!!str", Value: fmt.Sprint(t)} + tag, text := formatYAMLFloat(t) + return &yaml.Node{Kind: yaml.ScalarNode, Tag: tag, Value: text} case time.Time: return &yaml.Node{Kind: yaml.ScalarNode, Tag: "!!str", Value: t.Format(time.RFC3339)} case []string: @@ -546,13 +549,45 @@ func valueToYAMLNode(v any) *yaml.Node { return seq case map[string]any: mnode := &yaml.Node{Kind: yaml.MappingNode, Tag: "!!map"} - for k, v2 := range t { + keys := make([]string, 0, len(t)) + for k := range t { + keys = append(keys, k) + } + // Sorted so repeated writes of the same value produce byte-identical + // YAML; ranging a Go map directly would reorder keys run to run. + sort.Strings(keys) + for _, k := range keys { mnode.Content = append(mnode.Content, &yaml.Node{Kind: yaml.ScalarNode, Tag: "!!str", Value: k}, - valueToYAMLNode(v2)) + valueToYAMLNode(t[k])) } return mnode default: return &yaml.Node{Kind: yaml.ScalarNode, Tag: "!!str", Value: fmt.Sprint(v)} } } + +// formatYAMLFloat renders a float as a YAML scalar and reports the tag it +// should carry. JSON has no integer type, so a value that arrived over the MCP +// wire as `1` reaches us as float64(1). Tagging that !!float and writing "1" +// would make the emitter spell out an explicit `!!float` tag, and writing "1.0" +// would fail a JSON Schema `type: integer` check that the same value satisfies +// in JSON. Integral floats therefore serialize as plain integers, which is the +// representation both YAML and JSON Schema agree on. +func formatYAMLFloat(v any) (string, string) { + var f float64 + switch t := v.(type) { + case float32: + f = float64(t) + case float64: + f = t + } + if math.IsInf(f, 0) || math.IsNaN(f) { + // Not representable as a plain YAML scalar of either numeric tag. + return "!!str", fmt.Sprint(f) + } + if f == math.Trunc(f) && math.Abs(f) < 1e15 { + return "!!int", strconv.FormatInt(int64(f), 10) + } + return "!!float", strconv.FormatFloat(f, 'g', -1, 64) +} diff --git a/pkg/keg/node_meta_test.go b/pkg/keg/node_meta_test.go index dd34a57..2f15670 100644 --- a/pkg/keg/node_meta_test.go +++ b/pkg/keg/node_meta_test.go @@ -7,6 +7,7 @@ import ( "github.com/jlrickert/tapper/pkg/keg" "github.com/stretchr/testify/require" + "gopkg.in/yaml.v3" ) func TestParseMeta_EmptyReturnsEmptyMeta(t *testing.T) { @@ -181,3 +182,69 @@ tags: require.Contains(t, out, "- gamma") require.NotContains(t, out, "- alpha") } + +// TestSetAttrs_PreservesScalarTypes covers tapper#91: SetAttrs used to tag every +// numeric scalar !!str, so a schema field typed `integer` could never be +// satisfied through create or through an edit carrying inline frontmatter. +func TestSetAttrs_PreservesScalarTypes(t *testing.T) { + t.Parallel() + ctx := context.Background() + + m, err := keg.ParseMeta(ctx, []byte("# initial\ntype: change\n")) + require.NoError(t, err) + + require.NoError(t, m.SetAttrs(ctx, map[string]any{ + // YAML frontmatter decodes an integer literal as int. + "contract_count": 1, + // JSON (the MCP attrs path) has no integer type, so the same value + // arrives as float64 and must still write as an integer. + "json_count": float64(2), + "ratio": 0.75, + "enabled": true, + "label": "text", + })) + + out := m.ToYAML() + require.Contains(t, out, "contract_count: 1") + require.Contains(t, out, "json_count: 2") + require.Contains(t, out, "ratio: 0.75") + require.Contains(t, out, "enabled: true") + require.Contains(t, out, "label: text") + // The bug's signature was a quoted scalar where a number belonged. + require.NotContains(t, out, `"1"`) + require.NotContains(t, out, `"0.75"`) + + // Round-trip through YAML to confirm the emitted tags actually resolve back + // to numbers rather than to strings that merely look unquoted. + var back map[string]any + require.NoError(t, yaml.Unmarshal([]byte(out), &back)) + require.Equal(t, 1, back["contract_count"]) + require.Equal(t, 2, back["json_count"]) + require.Equal(t, 0.75, back["ratio"]) + require.Equal(t, true, back["enabled"]) + require.Equal(t, "text", back["label"]) +} + +// TestSetAttrs_NestedMapsAreDeterministic guards the sorted key order in +// valueToYAMLNode: ranging a Go map directly reordered nested keys run to run, +// which made otherwise identical writes produce different meta.yaml bytes. +func TestSetAttrs_NestedMapsAreDeterministic(t *testing.T) { + t.Parallel() + ctx := context.Background() + + render := func() string { + m, err := keg.ParseMeta(ctx, []byte("# initial\n")) + require.NoError(t, err) + require.NoError(t, m.SetAttrs(ctx, map[string]any{ + "nested": map[string]any{ + "zulu": 1, "alpha": 2, "mike": 3, "delta": 4, "papa": 5, + }, + })) + return m.ToYAML() + } + + first := render() + for i := 0; i < 8; i++ { + require.Equal(t, first, render()) + } +} diff --git a/pkg/mcp/auth_error_internal_test.go b/pkg/mcp/auth_error_internal_test.go new file mode 100644 index 0000000..2ddbd7d --- /dev/null +++ b/pkg/mcp/auth_error_internal_test.go @@ -0,0 +1,116 @@ +package mcp + +import ( + "errors" + "fmt" + "strings" + "testing" + + "github.com/jlrickert/tapper/pkg/keg" + "github.com/jlrickert/tapper/pkg/tapper" +) + +// TestErrorResult_AuthFailuresCarryGuidance covers tapper#87: an auth failure +// used to fall through to bare error text with no structured content, leaving +// an agent no indication that reorienting is the refresh boundary. +func TestErrorResult_AuthFailuresCarryGuidance(t *testing.T) { + tests := []struct { + name string + err error + wantCode string + wantReorient bool + }{ + { + name: "unauthorized", + err: fmt.Errorf("hub call: %w", keg.ErrUnauthorized), + wantCode: "UNAUTHORIZED", + wantReorient: true, + }, + { + name: "token rejected", + err: fmt.Errorf("hub: %w (401 Unauthorized)", tapper.ErrTokenRejected), + wantCode: "UNAUTHORIZED", + wantReorient: true, + }, + { + // A grant the user does not have is not fixed by logging in, so + // this must not send the agent into a login/reorient loop. + name: "forbidden", + err: fmt.Errorf("hub call: %w", keg.ErrForbidden), + wantCode: "FORBIDDEN", + wantReorient: false, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + res := errorResult(tc.err) + if !res.IsError { + t.Fatalf("IsError = false, want true") + } + structured, ok := res.StructuredContent.(map[string]any) + if !ok { + t.Fatalf("StructuredContent = %T, want map[string]any", res.StructuredContent) + } + if got := structured["code"]; got != tc.wantCode { + t.Fatalf("code = %v, want %v", got, tc.wantCode) + } + if got := structured["reorientRequired"]; got != tc.wantReorient { + t.Fatalf("reorientRequired = %v, want %v", got, tc.wantReorient) + } + if got := structured["operationPerformed"]; got != false { + t.Fatalf("operationPerformed = %v, want false", got) + } + action, _ := structured["action"].(string) + if strings.TrimSpace(action) == "" { + t.Fatalf("action is empty; the agent gets no actionable guidance") + } + if tc.wantReorient { + if !strings.Contains(action, "tap auth login") || !strings.Contains(action, "reorient") { + t.Fatalf("action %q does not mention logging in and reorienting", action) + } + // The issue explicitly asks that guidance never tell a caller + // to kill a host-owned process. + for _, banned := range []string{"restart the host", "kill", "restart your"} { + if strings.Contains(strings.ToLower(action), banned) && + !strings.Contains(action, "restarting the host is not required") { + t.Fatalf("action %q tells the caller to restart or kill the host", action) + } + } + } + }) + } +} + +// TestErrorResult_AuthFailuresLeakNoCredentials guards the acceptance criterion +// that no credential detail reaches the agent transcript. +func TestErrorResult_AuthFailuresLeakNoCredentials(t *testing.T) { + const secret = "thub_supersecrettokenvalue" + res := errorResult(fmt.Errorf("request with token %s: %w", secret, keg.ErrUnauthorized)) + + structured, _ := res.StructuredContent.(map[string]any) + if action, _ := structured["action"].(string); strings.Contains(action, secret) { + t.Fatalf("action leaked the credential") + } + if code, _ := structured["code"].(string); strings.Contains(code, secret) { + t.Fatalf("code leaked the credential") + } +} + +// TestErrorResult_NonAuthErrorsAreUnchanged confirms the new branch did not +// swallow the precondition paths that sit next to it. +func TestErrorResult_NonAuthErrorsAreUnchanged(t *testing.T) { + res := errorResult(errors.New("something ordinary went wrong")) + if res.StructuredContent != nil { + t.Fatalf("StructuredContent = %v, want nil for a plain error", res.StructuredContent) + } + + res = errorResult(fmt.Errorf("write: %w", keg.ErrPreconditionRequired)) + structured, ok := res.StructuredContent.(map[string]any) + if !ok { + t.Fatalf("precondition error lost its structured content") + } + if structured["code"] != keg.RemoteCodePreconditionRequired { + t.Fatalf("code = %v, want %v", structured["code"], keg.RemoteCodePreconditionRequired) + } +} diff --git a/pkg/mcp/server.go b/pkg/mcp/server.go index e03e5c1..858e56b 100644 --- a/pkg/mcp/server.go +++ b/pkg/mcp/server.go @@ -220,6 +220,10 @@ func errorResult(err error) *sdkmcp.CallToolResult { IsError: true, } } + if errors.Is(err, keg.ErrUnauthorized) || errors.Is(err, keg.ErrForbidden) || + errors.Is(err, tapper.ErrTokenRejected) { + return unauthorizedResult(err) + } if errors.Is(err, keg.ErrPreconditionRequired) { return &sdkmcp.CallToolResult{ Content: []sdkmcp.Content{&sdkmcp.TextContent{Text: err.Error()}}, @@ -337,3 +341,39 @@ func truncatePayload(s string, limit int) string { } return s[:limit] + "...(truncated)" } + +// unauthorizedResult explains what an agent can actually do about a rejected +// credential. Orientation reloads the credential store as well as the config +// (see KegService.ReloadAuthStore), so a token written by `tap auth login` in +// another shell takes effect on the next orient without restarting the host +// process — advice worth giving explicitly, since an agent that reads a bare +// 401 tends to treat the whole session as dead (tapper#87). +// +// A 403 is kept distinct. The credential was accepted and the user simply +// lacks the grant, so logging in again changes nothing and reorientRequired +// stays false rather than sending the agent round a loop that cannot succeed. +// +// Neither message carries a token, header, or credential detail: both reach an +// agent transcript. +func unauthorizedResult(err error) *sdkmcp.CallToolResult { + code := "UNAUTHORIZED" + action := "Authentication changed or expired. Run `tap auth login` if needed, then reorient. " + + "Reorientation refreshes the current MCP session; restarting the host is not required." + reorient := true + if errors.Is(err, keg.ErrForbidden) && !errors.Is(err, keg.ErrUnauthorized) { + code = "FORBIDDEN" + action = "The credential was accepted but lacks permission for this operation. " + + "Logging in again will not grant it; request access to the keg or namespace instead." + reorient = false + } + return &sdkmcp.CallToolResult{ + Content: []sdkmcp.Content{&sdkmcp.TextContent{Text: code + ": " + err.Error() + "\n\n" + action}}, + StructuredContent: map[string]any{ + "code": code, + "reorientRequired": reorient, + "operationPerformed": false, + "action": action, + }, + IsError: true, + } +} diff --git a/pkg/mcp/tools_namespace.go b/pkg/mcp/tools_namespace.go index 9caf1b4..67f9e73 100644 --- a/pkg/mcp/tools_namespace.go +++ b/pkg/mcp/tools_namespace.go @@ -25,13 +25,18 @@ func registerNamespaceTools(srv *sdkmcp.Server, tap *tapper.Tap, _ KegDefaults) OpenWorldHint: boolPtr(true), }, }, func(ctx context.Context, _ *sdkmcp.CallToolRequest, in namespaceListInput) (*sdkmcp.CallToolResult, any, error) { - nss, err := tap.NamespaceList(ctx, tapper.NamespaceListOptions{Hub: in.Hub}) + res, err := tap.NamespaceList(ctx, tapper.NamespaceListOptions{Hub: in.Hub}) if err != nil { return errorResult(err), nil, nil } - lines := make([]string, 0, len(nss)) - for _, ns := range nss { - lines = append(lines, fmt.Sprintf("@%s\t%s\t%s", ns.Name, ns.Kind, ns.Role)) + lines := make([]string, 0, len(res.Namespaces)+len(res.Warnings)) + for _, ns := range res.Namespaces { + // The hub is part of the identity of a row: the same namespace + // name can exist on more than one configured hub. + lines = append(lines, fmt.Sprintf("@%s\t%s\t%s\t%s", ns.Name, ns.Kind, ns.Role, ns.Hub)) + } + for _, w := range res.Warnings { + lines = append(lines, "warning: "+w) } return linesResult(lines), nil, nil }) diff --git a/pkg/mcp/tools_write.go b/pkg/mcp/tools_write.go index 71cc4ad..e0acdc4 100644 --- a/pkg/mcp/tools_write.go +++ b/pkg/mcp/tools_write.go @@ -78,13 +78,16 @@ type createInput struct { Keg string `json:"keg,omitempty" jsonschema:"keg alias (uses default if empty)"` } type createNodeInput struct { - Key string `json:"key"` - Schema string `json:"schema,omitempty" jsonschema:"schema selected for this write; required when strict policy and agent mode both block"` - Title string `json:"title,omitempty"` - Lead string `json:"lead,omitempty"` - Body string `json:"body,omitempty"` - Tags []string `json:"tags,omitempty"` - Attrs map[string]string `json:"attrs,omitempty"` + Key string `json:"key"` + Schema string `json:"schema,omitempty" jsonschema:"schema selected for this write; required when strict policy and agent mode both block"` + Title string `json:"title,omitempty"` + Lead string `json:"lead,omitempty"` + Body string `json:"body,omitempty"` + Tags []string `json:"tags,omitempty"` + // map[string]any, not map[string]string: a string-typed map generates an + // `additionalProperties: {type: string}` schema, which cannot express an + // integer at all, so schema fields typed `integer` become unwritable. + Attrs map[string]any `json:"attrs,omitempty"` } type createNodeOutput struct { diff --git a/pkg/tapper/hub_namespaces.go b/pkg/tapper/hub_namespaces.go index 546df01..b5897ad 100644 --- a/pkg/tapper/hub_namespaces.go +++ b/pkg/tapper/hub_namespaces.go @@ -28,6 +28,11 @@ type HubNamespace struct { Name string `json:"name"` Kind string `json:"kind,omitempty"` // user|org Role string `json:"role,omitempty"` // caller's role + + // Hub names the configured hub this row came from. It is filled in + // locally by NamespaceList, not by the hub, so identical namespace names + // on different hubs stay distinguishable. Not part of namespaceWire. + Hub string `json:"hub,omitempty"` } func namespaceMembersPath(namespace string) string { diff --git a/pkg/tapper/keg_service.go b/pkg/tapper/keg_service.go index ec3c8ee..24af8c4 100644 --- a/pkg/tapper/keg_service.go +++ b/pkg/tapper/keg_service.go @@ -42,6 +42,26 @@ func (s *KegService) ensureCache() { } } +// ReloadAuthStore drops the cached credential store so the next resolution +// reads it back from disk. +// +// The store is otherwise loaded once per process. A `tap auth login` run in a +// separate shell writes a new token to disk that a long-lived MCP server would +// never see, so telling an agent to log in and reorient was advice that could +// not work (tapper#87). Orientation is already the reload boundary for +// configuration; this puts credentials on the same boundary. +func (s *KegService) ReloadAuthStore() { + s.cacheMu.Lock() + defer s.cacheMu.Unlock() + s.authStoreOnce = sync.Once{} + s.authStore = nil + s.authStorePath = "" + s.authResolver = nil + // Kegs resolved earlier captured the old resolver, so dropping the store + // alone would leave them holding the stale credential. + s.kegCache = nil +} + func (s *KegService) tokenResolver() keg.TokenResolver { s.authStoreOnce.Do(func() { defer func() { diff --git a/pkg/tapper/keg_service_reload_internal_test.go b/pkg/tapper/keg_service_reload_internal_test.go new file mode 100644 index 0000000..87d3b72 --- /dev/null +++ b/pkg/tapper/keg_service_reload_internal_test.go @@ -0,0 +1,64 @@ +package tapper + +import ( + "path/filepath" + "testing" + + "github.com/jlrickert/cli-toolkit/sandbox" + "github.com/jlrickert/tapper/pkg/keg" +) + +// TestReloadAuthStore_PicksUpCredentialsWrittenAfterFirstLoad covers the half of +// tapper#87 that makes the guidance true. The credential store is loaded once +// per process, so a `tap auth login` run in another shell was invisible to a +// long-lived MCP server and reorienting could not clear a 401. +func TestReloadAuthStore_PicksUpCredentialsWrittenAfterFirstLoad(t *testing.T) { + fx := sandbox.NewSandbox(t, &sandbox.Options{ + Home: filepath.FromSlash("/home/testuser"), + User: "testuser", + }) + if err := fx.Setwd("/home/testuser"); err != nil { + t.Fatalf("setwd: %v", err) + } + tap, err := NewTap(TapOptions{Root: "/home/testuser", Runtime: fx.Runtime()}) + if err != nil { + t.Fatalf("new tap: %v", err) + } + + const hubURL = "https://atlas.foldwise.ai" + target := &keg.Target{HubURL: hubURL, Url: hubURL + "/api/v1/@ns/kegs/example"} + + // First resolution happens before any login, caching an empty store. + if tok := tap.KegService.tokenResolver().ResolveToken(target); tok != "" { + t.Fatalf("token before login = %q, want empty", tok) + } + + // Simulate `tap auth login` in a separate process writing the store. + store := &AuthStore{data: &authStoreDTO{}} + store.Set(hubURL, AuthEntry{AccessToken: "thub_fresh"}) + if err := store.Save(fx.Context(), fx.Runtime(), tap.PathService.AuthStorePath()); err != nil { + t.Fatalf("save auth store: %v", err) + } + + // Without a reload the process keeps its startup view of credentials. + if tok := tap.KegService.tokenResolver().ResolveToken(target); tok != "" { + t.Fatalf("token = %q before reload; the cache should still be stale", tok) + } + + tap.KegService.ReloadAuthStore() + + if tok := tap.KegService.tokenResolver().ResolveToken(target); tok != "thub_fresh" { + t.Fatalf("token after reload = %q, want %q", tok, "thub_fresh") + } +} + +// TestReloadAuthStore_ClearsResolvedKegCache confirms the reload also drops +// kegs resolved earlier. They captured the old resolver, so leaving them cached +// would keep the stale credential in play despite the store being reloaded. +func TestReloadAuthStore_ClearsResolvedKegCache(t *testing.T) { + svc := &KegService{kegCache: map[string]keg.Keg{"stale": nil}} + svc.ReloadAuthStore() + if len(svc.kegCache) != 0 { + t.Fatalf("kegCache retained %d entries after reload", len(svc.kegCache)) + } +} diff --git a/pkg/tapper/tap_batch.go b/pkg/tapper/tap_batch.go index 7c8e2f7..a1b8022 100644 --- a/pkg/tapper/tap_batch.go +++ b/pkg/tapper/tap_batch.go @@ -15,7 +15,7 @@ type BatchCreateNode struct { Lead string Body string Tags []string - Attrs map[string]string + Attrs map[string]any } type BatchCreateOptions struct { KegTargetOptions @@ -30,7 +30,7 @@ func (t *Tap) CreateBatch(ctx context.Context, opts BatchCreateOptions) ([]keg.C ctx = keg.WithDefaultValidationActor(ctx, keg.ValidationActorHuman) nodes := make([]keg.NodeCreate, len(opts.Nodes)) for i, item := range opts.Nodes { - nodes[i] = keg.NodeCreate{Key: item.Key, Schema: item.Schema, Title: item.Title, Lead: item.Lead, Body: []byte(item.Body), Tags: item.Tags, Attrs: createAttrsFromStrings(item.Attrs)} + nodes[i] = keg.NodeCreate{Key: item.Key, Schema: item.Schema, Title: item.Title, Lead: item.Lead, Body: []byte(item.Body), Tags: item.Tags, Attrs: item.Attrs} } return k.CreateNodes(ctx, nodes) } diff --git a/pkg/tapper/tap_flight.go b/pkg/tapper/tap_flight.go index f74225e..2a2125b 100644 --- a/pkg/tapper/tap_flight.go +++ b/pkg/tapper/tap_flight.go @@ -172,7 +172,7 @@ func (t *Tap) resolveWriteFlightRef(raw string) (FlightRef, HubEntry, string, er if err != nil { return FlightRef{}, HubEntry{}, "", err } - ref, err := ParseFlightRef(raw, defaultFlightNamespace(cfg)) + ref, err := ParseFlightRef(raw, t.defaultFlightNamespace(cfg)) if err != nil { return FlightRef{}, HubEntry{}, "", err } @@ -200,10 +200,23 @@ func (t *Tap) resolveWriteFlightRef(raw string) (FlightRef, HubEntry, string, er return ref, entry, hubName, nil } -func defaultFlightNamespace(cfg *Config) string { +// defaultFlightNamespace supplies the namespace for a flight reference that +// omits one. Precedence: +// +// active KEG's namespace → defaultNamespace → fallbackNamespace → +// the resolved hub's per-hub defaultNamespace +// +// The active KEG comes first because a bare flight name typed while working in +// an org KEG means a flight in that org, not one in the user's personal +// namespace. Resolving it last put flights in the wrong namespace silently +// (tapper#74); an explicitly qualified @namespace/+slug never reaches here. +func (t *Tap) defaultFlightNamespace(cfg *Config) string { if cfg == nil { return "" } + if ns := t.activeKegNamespace(cfg); ns != "" { + return ns + } if ns := strings.TrimSpace(cfg.resolveNamespaceForName()); ns != "" { return ns } @@ -218,6 +231,33 @@ func defaultFlightNamespace(cfg *Config) string { return "" } +// activeKegNamespace returns the namespace of the KEG currently in context, or +// "" when no KEG is selected or the selector names no namespace. The selector +// chain mirrors resolveIdentity and resolveKegAdminRef: defaultKeg → project +// alias → fallbackKeg. +// +// Only a namespace the selector states explicitly counts. Running the selector +// through resolveNamespaceHub would fill an omitted namespace from +// defaultNamespace, so a bare keg name would report the personal namespace as +// though the KEG had named it, and this step would stop being distinguishable +// from the one after it. +func (t *Tap) activeKegNamespace(cfg *Config) string { + if t == nil || cfg == nil { + return "" + } + selector := strings.TrimSpace(cfg.DefaultKeg()) + if selector == "" { + selector = strings.TrimSpace(cfg.LookupAlias(t.Runtime, t.Root)) + } + if selector == "" { + selector = strings.TrimSpace(cfg.FallbackKeg()) + } + if selector == "" { + return "" + } + return strings.TrimPrefix(strings.TrimSpace(parseKegRef(selector).Namespace), "@") +} + func hubCoverFromFlightCover(cover []FlightCover) []HubFlightCover { out := make([]HubFlightCover, 0, len(cover)) for _, c := range cover { diff --git a/pkg/tapper/tap_flight_namespace_internal_test.go b/pkg/tapper/tap_flight_namespace_internal_test.go new file mode 100644 index 0000000..1fce8ef --- /dev/null +++ b/pkg/tapper/tap_flight_namespace_internal_test.go @@ -0,0 +1,120 @@ +package tapper + +import ( + "path/filepath" + "testing" + + "github.com/jlrickert/cli-toolkit/sandbox" +) + +// newFlightNamespaceTap builds a Tap over a sandboxed filesystem holding the +// given user config. In-package so the unexported namespace resolvers are +// reachable directly; the exported CreateFlight path would need a live hub. +func newFlightNamespaceTap(t *testing.T, userConfig string) *Tap { + t.Helper() + fx := sandbox.NewSandbox(t, &sandbox.Options{ + Home: filepath.FromSlash("/home/testuser"), + User: "testuser", + }) + if err := fx.Setwd("/home/testuser"); err != nil { + t.Fatalf("setwd: %v", err) + } + tap, err := NewTap(TapOptions{Root: "/home/testuser", Runtime: fx.Runtime()}) + if err != nil { + t.Fatalf("new tap: %v", err) + } + if err := fx.Runtime().AtomicWriteFile(tap.PathService.UserConfig(), []byte(userConfig), 0o644); err != nil { + t.Fatalf("write config: %v", err) + } + return tap +} + +// TestDefaultFlightNamespace covers tapper#74: an unqualified flight name used +// to resolve to the personal namespace even when the active KEG was in an org +// namespace, silently creating the flight in the wrong place. +func TestDefaultFlightNamespace(t *testing.T) { + const hubs = "hubs:\n atlas:\n kind: remote\n url: https://atlas.foldwise.ai\n token: tok\n" + + tests := []struct { + name string + config string + want string + }{ + { + name: "active org keg wins over personal default namespace", + config: hubs + "defaultNamespace: jlrickert\ndefaultKeg: \"@foldwise/notes\"\n", + want: "foldwise", + }, + { + name: "active personal keg resolves to the personal namespace", + config: hubs + "defaultNamespace: jlrickert\ndefaultKeg: \"@jlrickert/notes\"\n", + want: "jlrickert", + }, + { + name: "fallback keg supplies the namespace when no default keg is set", + config: hubs + "defaultNamespace: jlrickert\nfallbackKeg: \"@foldwise/notes\"\n", + want: "foldwise", + }, + { + name: "default keg outranks fallback keg", + config: hubs + "defaultKeg: \"@foldwise/notes\"\nfallbackKeg: \"@other/notes\"\n", + want: "foldwise", + }, + { + name: "no active keg falls back to the configured default namespace", + config: hubs + "defaultNamespace: jlrickert\n", + want: "jlrickert", + }, + { + name: "no active keg and no default namespace falls back to the hub default", + config: "hubs:\n atlas:\n kind: remote\n url: https://atlas.foldwise.ai\n token: tok\n defaultNamespace: hubns\n", + want: "hubns", + }, + { + name: "a bare keg name states no namespace and does not shadow the default", + // The selector names no namespace, so the active-KEG step must + // decline rather than reporting the personal namespace as though + // the KEG had named it. + config: hubs + "defaultNamespace: jlrickert\ndefaultKeg: notes\n", + want: "jlrickert", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + tap := newFlightNamespaceTap(t, tc.config) + cfg, err := tap.ConfigService.Config() + if err != nil { + t.Fatalf("config: %v", err) + } + if got := tap.defaultFlightNamespace(cfg); got != tc.want { + t.Fatalf("defaultFlightNamespace = %q, want %q", got, tc.want) + } + }) + } +} + +// TestResolveWriteFlightRef_QualifiedRefIsPreserved confirms an explicit +// @namespace/+slug is never rewritten by the defaulting above. +func TestResolveWriteFlightRef_QualifiedRefIsPreserved(t *testing.T) { + tap := newFlightNamespaceTap(t, + "hubs:\n atlas:\n kind: remote\n url: https://atlas.foldwise.ai\n token: tok\n"+ + "defaultNamespace: jlrickert\ndefaultKeg: \"@foldwise/notes\"\n") + + ref, _, _, err := tap.resolveWriteFlightRef("@other/+plan") + if err != nil { + t.Fatalf("resolveWriteFlightRef: %v", err) + } + if ref.Namespace != "other" { + t.Fatalf("namespace = %q, want %q", ref.Namespace, "other") + } + + // And the unqualified form picks up the active KEG's namespace. + ref, _, _, err = tap.resolveWriteFlightRef("plan") + if err != nil { + t.Fatalf("resolveWriteFlightRef unqualified: %v", err) + } + if ref.Namespace != "foldwise" { + t.Fatalf("unqualified namespace = %q, want %q", ref.Namespace, "foldwise") + } +} diff --git a/pkg/tapper/tap_launch.go b/pkg/tapper/tap_launch.go index 87e9adc..29e5c1e 100644 --- a/pkg/tapper/tap_launch.go +++ b/pkg/tapper/tap_launch.go @@ -339,7 +339,7 @@ func (t *Tap) ResolveLaunch(opts LaunchOptions) (*LaunchResult, error) { warnings []string ) if rootRef := strings.TrimSpace(t.ActiveFlightName(opts.Flight)); rootRef != "" { - parsed, err := ParseFlightRef(rootRef, defaultFlightNamespace(cfg)) + parsed, err := ParseFlightRef(rootRef, t.defaultFlightNamespace(cfg)) if err != nil { return nil, fmt.Errorf("resolve launch flight %q: %w", rootRef, err) } diff --git a/pkg/tapper/tap_namespace.go b/pkg/tapper/tap_namespace.go index bb4740c..33101f8 100644 --- a/pkg/tapper/tap_namespace.go +++ b/pkg/tapper/tap_namespace.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "net/url" + "sort" "strings" ) @@ -11,12 +12,20 @@ import ( // creating org namespaces. Backs `tap namespace`. The hub endpoints live under // /api/v1/namespaces and /api/v1/@{namespace}/members. -// NamespaceListOptions selects which hub to query for namespaces. Empty uses -// the resolved default hub. +// NamespaceListOptions selects which hub to query for namespaces. An empty Hub +// aggregates every configured hub the user can reach. type NamespaceListOptions struct { Hub string } +// NamespaceListResult carries the aggregated rows plus a note for each hub that +// could not be reached. Warnings are non-fatal by design: one unreachable hub +// must not blank out the namespaces the others returned. +type NamespaceListResult struct { + Namespaces []HubNamespace + Warnings []string +} + // NamespaceMembersOptions selects the namespace whose members to list. Namespace // is the selector; empty resolves the default namespace. Hub pins the hub. type NamespaceMembersOptions struct { @@ -66,13 +75,82 @@ type NamespaceCreateResult struct { // (distinct from the keg grant roles). var namespaceMemberRoles = map[string]bool{"owner": true, "admin": true, "member": true} -// NamespaceList returns the namespaces the caller belongs to on a hub. -func (t *Tap) NamespaceList(ctx context.Context, opts NamespaceListOptions) ([]HubNamespace, error) { - hubURL, token, err := t.resolveHubEndpoint(opts.Hub) +// NamespaceList returns the namespaces the caller belongs to. With no --hub it +// aggregates across every configured hub rather than only the selected one, +// which previously hid memberships on every other hub and gave no way to tell +// which hub a row came from (tapper#73). Every row carries its source hub, so +// the same namespace name on two hubs stays two distinct rows. +// +// With an explicit --hub only that hub is queried and its errors surface +// directly. In aggregate mode an unreachable or unauthenticated hub is recorded +// as a warning and skipped. This mirrors HubListKegs. +// +// Local namespaces are not represented. Tapper has no local hub kind: config +// admits only remote and readonly, and every resolver rejects anything else. +func (t *Tap) NamespaceList(ctx context.Context, opts NamespaceListOptions) (NamespaceListResult, error) { + explicit := strings.TrimSpace(opts.Hub) + if explicit != "" { + hubURL, token, err := t.resolveHubEndpoint(explicit) + if err != nil { + return NamespaceListResult{}, err + } + nss, err := ListNamespaces(ctx, hubURL, token) + if err != nil { + return NamespaceListResult{}, err + } + return NamespaceListResult{Namespaces: tagNamespaceHub(nss, explicit)}, nil + } + + cfg, err := t.ConfigService.Config() if err != nil { - return nil, err + return NamespaceListResult{}, err + } + + var result NamespaceListResult + for _, name := range t.allHubNames(cfg) { + entry, ok := cfg.Hub(name) + if !ok { + continue + } + if kind := hubKindOrDefault(entry.Kind); kind != HubKindRemote && kind != HubKindReadonly { + result.Warnings = append(result.Warnings, + fmt.Sprintf("hub %q: unsupported kind %q", name, kind)) + continue + } + hubURL, token, hErr := remoteHubEndpoint(t, name, entry) + if hErr != nil { + result.Warnings = append(result.Warnings, fmt.Sprintf("hub %q: %v", name, hErr)) + continue + } + nss, lErr := ListNamespaces(ctx, hubURL, token) + if lErr != nil { + result.Warnings = append(result.Warnings, fmt.Sprintf("hub %q: %v", name, lErr)) + if lg := t.Runtime.Logger(); lg != nil { + lg.Warn("namespace list: skipping hub", "hub", name, "err", lErr) + } + continue + } + result.Namespaces = append(result.Namespaces, tagNamespaceHub(nss, name)...) + } + + sort.Slice(result.Namespaces, func(i, j int) bool { + a, b := result.Namespaces[i], result.Namespaces[j] + if a.Hub != b.Hub { + return a.Hub < b.Hub + } + return a.Name < b.Name + }) + return result, nil +} + +// tagNamespaceHub stamps each row with the hub it came from. +func tagNamespaceHub(nss []HubNamespace, hub string) []HubNamespace { + out := make([]HubNamespace, 0, len(nss)) + for _, ns := range nss { + ns.Hub = hub + out = append(out, ns) } - return ListNamespaces(ctx, hubURL, token) + return out } // NamespaceMembers returns the member roster of a namespace. diff --git a/pkg/tapper/tap_namespace_test.go b/pkg/tapper/tap_namespace_test.go index 5f422d6..7eb954b 100644 --- a/pkg/tapper/tap_namespace_test.go +++ b/pkg/tapper/tap_namespace_test.go @@ -3,11 +3,13 @@ package tapper_test import ( "context" "encoding/json" + "fmt" "io" "net/http" "net/http/httptest" "testing" + "github.com/jlrickert/cli-toolkit/sandbox" kegpkg "github.com/jlrickert/tapper/pkg/keg" "github.com/jlrickert/tapper/pkg/tapper" "github.com/stretchr/testify/require" @@ -25,12 +27,14 @@ func TestNamespaceList(t *testing.T) { }) }) tap, fx, _ := newRemoteHubTap(t, h) - nss, err := tap.NamespaceList(fx.Context(), tapper.NamespaceListOptions{}) + res, err := tap.NamespaceList(fx.Context(), tapper.NamespaceListOptions{}) require.NoError(t, err) + // Rows are sorted by hub then namespace, and each names its source hub. require.Equal(t, []tapper.HubNamespace{ - {Name: "jlrickert", Kind: "user", Role: "owner"}, - {Name: "acme", Kind: "org", Role: "admin"}, - }, nss) + {Name: "acme", Kind: "org", Role: "admin", Hub: "atlas"}, + {Name: "jlrickert", Kind: "user", Role: "owner", Hub: "atlas"}, + }, res.Namespaces) + require.Empty(t, res.Warnings) } func TestNamespaceMembers(t *testing.T) { @@ -133,3 +137,112 @@ func TestCreateNamespaceDisabled(t *testing.T) { require.ErrorIs(t, err, kegpkg.ErrNotSupported) require.Contains(t, err.Error(), "disabled for remote clients") } + +// newTwoHubTap wires two independent hub servers into one config, so aggregate +// listing has something to aggregate. +func newTwoHubTap(t *testing.T, atlas, homelab http.Handler) (*tapper.Tap, *sandbox.Sandbox) { + t.Helper() + fx := NewSandbox(t) + require.NoError(t, fx.Setwd("/home/testuser")) + atlasSrv := httptest.NewServer(atlas) + t.Cleanup(atlasSrv.Close) + homelabSrv := httptest.NewServer(homelab) + t.Cleanup(homelabSrv.Close) + + tap, err := tapper.NewTap(tapper.TapOptions{Root: "/home/testuser", Runtime: fx.Runtime()}) + require.NoError(t, err) + cfg := fmt.Sprintf("hubs:\n"+ + " atlas:\n kind: remote\n url: %s\n token: tok\n"+ + " homelab:\n kind: remote\n url: %s\n token: tok\n"+ + "defaultHub: atlas\n", atlasSrv.URL, homelabSrv.URL) + require.NoError(t, fx.Runtime().AtomicWriteFile(tap.PathService.UserConfig(), []byte(cfg), 0o644)) + return tap, fx +} + +func namespaceHandler(t *testing.T, nss ...tapper.HubNamespace) http.Handler { + t.Helper() + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + require.Equal(t, "/api/v1/namespaces", r.URL.Path) + _ = json.NewEncoder(w).Encode(nss) + }) +} + +// TestNamespaceList_AggregatesEveryHub covers tapper#73: listing showed only the +// selected hub's memberships, hiding every other configured hub and giving no +// way to attribute a row. +func TestNamespaceList_AggregatesEveryHub(t *testing.T) { + t.Parallel() + tap, fx := newTwoHubTap(t, + namespaceHandler(t, tapper.HubNamespace{Name: "foldwise", Kind: "org", Role: "owner"}), + namespaceHandler(t, tapper.HubNamespace{Name: "homestuff", Kind: "org", Role: "member"}), + ) + + res, err := tap.NamespaceList(fx.Context(), tapper.NamespaceListOptions{}) + require.NoError(t, err) + require.Empty(t, res.Warnings) + require.Equal(t, []tapper.HubNamespace{ + {Name: "foldwise", Kind: "org", Role: "owner", Hub: "atlas"}, + {Name: "homestuff", Kind: "org", Role: "member", Hub: "homelab"}, + }, res.Namespaces) +} + +// TestNamespaceList_SameNameOnTwoHubsStaysDistinct guards the acceptance +// criterion that identical namespace names remain distinguishable. +func TestNamespaceList_SameNameOnTwoHubsStaysDistinct(t *testing.T) { + t.Parallel() + tap, fx := newTwoHubTap(t, + namespaceHandler(t, tapper.HubNamespace{Name: "shared", Kind: "org", Role: "owner"}), + namespaceHandler(t, tapper.HubNamespace{Name: "shared", Kind: "org", Role: "member"}), + ) + + res, err := tap.NamespaceList(fx.Context(), tapper.NamespaceListOptions{}) + require.NoError(t, err) + require.Equal(t, []tapper.HubNamespace{ + {Name: "shared", Kind: "org", Role: "owner", Hub: "atlas"}, + {Name: "shared", Kind: "org", Role: "member", Hub: "homelab"}, + }, res.Namespaces) +} + +// TestNamespaceList_UnreachableHubPreservesOthers covers the criterion that one +// bad hub must not blank the listing, and must be named in the report. +func TestNamespaceList_UnreachableHubPreservesOthers(t *testing.T) { + t.Parallel() + broken := http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + http.Error(w, "boom", http.StatusInternalServerError) + }) + tap, fx := newTwoHubTap(t, + namespaceHandler(t, tapper.HubNamespace{Name: "foldwise", Kind: "org", Role: "owner"}), + broken, + ) + + res, err := tap.NamespaceList(fx.Context(), tapper.NamespaceListOptions{}) + require.NoError(t, err) + require.Equal(t, []tapper.HubNamespace{ + {Name: "foldwise", Kind: "org", Role: "owner", Hub: "atlas"}, + }, res.Namespaces) + require.Len(t, res.Warnings, 1) + require.Contains(t, res.Warnings[0], "homelab") +} + +// TestNamespaceList_ExplicitHubNarrowsAndSurfacesErrors confirms --hub keeps its +// old single-hub behaviour, including propagating that hub's failure. +func TestNamespaceList_ExplicitHubNarrowsAndSurfacesErrors(t *testing.T) { + t.Parallel() + broken := http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + http.Error(w, "boom", http.StatusInternalServerError) + }) + tap, fx := newTwoHubTap(t, + namespaceHandler(t, tapper.HubNamespace{Name: "foldwise", Kind: "org", Role: "owner"}), + broken, + ) + + res, err := tap.NamespaceList(fx.Context(), tapper.NamespaceListOptions{Hub: "atlas"}) + require.NoError(t, err) + require.Equal(t, []tapper.HubNamespace{ + {Name: "foldwise", Kind: "org", Role: "owner", Hub: "atlas"}, + }, res.Namespaces) + + // An explicit hub surfaces its own error rather than degrading to a warning. + _, err = tap.NamespaceList(fx.Context(), tapper.NamespaceListOptions{Hub: "homelab"}) + require.Error(t, err) +} diff --git a/pkg/tapper/tap_orient.go b/pkg/tapper/tap_orient.go index 9339eae..b1bf0d3 100644 --- a/pkg/tapper/tap_orient.go +++ b/pkg/tapper/tap_orient.go @@ -42,6 +42,12 @@ func (t *Tap) Orient(ctx context.Context, opts OrientOptions) (string, error) { if t != nil && t.ConfigService != nil { t.ConfigService.Reload() } + // Credentials reload on the same boundary. `tap auth login` runs in a + // separate process, so without this a long-lived MCP session keeps the + // token it loaded at startup and reorienting cannot clear a 401 (#87). + if t != nil && t.KegService != nil { + t.KegService.ReloadAuthStore() + } flightName := t.ActiveFlightName(opts.Flight) flight, flightNote := t.resolveOrientFlight(ctx, flightName) available, warnings := t.OrientationKegsForFlight(ctx, flight)