From bc75dc9df1087a07ee96cef20e2e56ba8bdacf2f Mon Sep 17 00:00:00 2001 From: Jared Rickert Date: Tue, 1 Sep 2026 22:17:28 -0500 Subject: [PATCH 1/5] fix(keg): preserve numeric scalar types in node metadata valueToYAMLNode tagged every integer and float !!str, so any schema field typed `integer` was unwritable through the MCP create and edit tools. A schema marking such a field required became impossible to satisfy: the node could not be created, and meta could not repair it afterwards because meta validates the whole node including markdown sections. Tag integers !!int and floats !!float, matching the !!bool case that was already correct. Integral floats serialize as plain integers because JSON has no integer type, so a value sent over MCP as 1 arrives as float64(1) and must still satisfy a `type: integer` check. Widen the MCP create tool's attrs parameter to map[string]any. A string-typed Go map generates additionalProperties: {type: string}, which cannot express a number by construction. The CLI --attrs flag keeps its string map, since its values genuinely arrive as text. Also sort nested map keys so repeated writes of the same value produce byte-identical YAML. Fixes #91 --- pkg/keg/node_meta.go | 45 +++++++++++++++++++++++--- pkg/keg/node_meta_test.go | 67 +++++++++++++++++++++++++++++++++++++++ pkg/mcp/tools_write.go | 17 ++++++---- pkg/tapper/tap_batch.go | 4 +-- 4 files changed, 119 insertions(+), 14 deletions(-) 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/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/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) } From fb258a36e4777a4699b9e6233907920f129ff229 Mon Sep 17 00:00:00 2001 From: Jared Rickert Date: Tue, 1 Sep 2026 22:19:38 -0500 Subject: [PATCH 2/5] fix(flight): resolve unqualified flight names against the active KEG An unqualified `tap flight create NAME` resolved its namespace from defaultNamespace, which is the user's personal namespace. Working in an org KEG and typing a bare flight name therefore created the flight under the personal namespace instead. The result was valid, so nothing failed loudly: the flight was simply absent from the org's flights page and could later collide with an explicitly qualified one. Put the active KEG's namespace first in the precedence chain, ahead of defaultNamespace, fallbackNamespace, and the hub's own default. The selector chain matches resolveIdentity and resolveKegAdminRef, so `tap flight create` now agrees with what `tap info` reports as active. Only a namespace the KEG selector states explicitly counts. A bare keg name still falls through to defaultNamespace rather than being resolved into the personal namespace and presented as if the KEG had named it. Explicitly qualified @namespace/+slug references never reach the defaulting path and are unchanged. Fixes #74 --- pkg/tapper/tap_flight.go | 44 ++++++- .../tap_flight_namespace_internal_test.go | 120 ++++++++++++++++++ pkg/tapper/tap_launch.go | 2 +- 3 files changed, 163 insertions(+), 3 deletions(-) create mode 100644 pkg/tapper/tap_flight_namespace_internal_test.go 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) } From d6546e732314a35e563c5d001dd24c52d610addf Mon Sep 17 00:00:00 2001 From: Jared Rickert Date: Tue, 1 Sep 2026 22:22:07 -0500 Subject: [PATCH 3/5] fix(mcp): make auth failures actionable and reload credentials on orient An auth failure fell through to bare error text with no structured content, so an agent whose session lost access had no indication that reorienting is the refresh boundary and tended to treat the session as dead. Telling an agent to log in and reorient would have been false advice on its own. The credential store is loaded once per process, so a token written by `tap auth login` in a separate shell was invisible to a running MCP server and no amount of reorienting could clear the 401. Orientation already reloads configuration; credentials now reload on the same boundary, including the resolved keg cache, whose entries captured the previous resolver. Auth errors now return structured content with a code, an action, and reorientRequired. A 403 is kept distinct: the credential was accepted and the caller simply lacks the grant, so logging in again changes nothing and reorientRequired stays false rather than starting a loop that cannot succeed. Neither message carries token or credential detail. Fixes #87 --- pkg/mcp/auth_error_internal_test.go | 116 ++++++++++++++++++ pkg/mcp/server.go | 40 ++++++ pkg/tapper/keg_service.go | 20 +++ .../keg_service_reload_internal_test.go | 64 ++++++++++ pkg/tapper/tap_orient.go | 6 + 5 files changed, 246 insertions(+) create mode 100644 pkg/mcp/auth_error_internal_test.go create mode 100644 pkg/tapper/keg_service_reload_internal_test.go 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/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_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) From 0694827b64afcb5e7d516412857fd240fe327470 Mon Sep 17 00:00:00 2001 From: Jared Rickert Date: Tue, 1 Sep 2026 22:24:36 -0500 Subject: [PATCH 4/5] feat(namespace): aggregate namespace list across configured hubs `tap namespace list` queried only the selected hub, so memberships on every other configured hub were silently absent, and the output carried no hub column, leaving rows unattributable when more than one hub is configured. With no --hub, every configured remote and readonly hub is now 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 becomes a warning naming that hub and the remaining hubs still list, mirroring how HubListKegs already handles aggregate mode. An explicit --hub keeps its previous single-hub behaviour and still propagates that hub's error. Local namespaces are not included. Tapper has no local hub kind: config admits only remote and readonly, and every resolver rejects anything else. Representing locally-backed namespaces needs that kind to exist first. Refs #73 --- pkg/cli/cmd_namespace.go | 18 ++++- pkg/mcp/tools_namespace.go | 13 +++- pkg/tapper/hub_namespaces.go | 5 ++ pkg/tapper/tap_namespace.go | 92 +++++++++++++++++++++-- pkg/tapper/tap_namespace_test.go | 121 ++++++++++++++++++++++++++++++- 5 files changed, 230 insertions(+), 19 deletions(-) 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/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/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/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) +} From 9cbe653753f22b89d6cc909a6cd7e5f8f6ebf11e Mon Sep 17 00:00:00 2001 From: Jared Rickert Date: Tue, 1 Sep 2026 22:37:24 -0500 Subject: [PATCH 5/5] test(keg): cover the integer-schema reproduction end to end Exercises the create and edit paths through a real keg with the schema from the issue, rather than asserting at the metadata layer alone. Both cases fail before the fix with the exact error the issue reports. Refs #91 --- pkg/keg/keg_integer_schema_test.go | 84 ++++++++++++++++++++++++++++++ 1 file changed, 84 insertions(+) create mode 100644 pkg/keg/keg_integer_schema_test.go 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"`) +}