From c224c9360dfce8b8f3f7c165b11acf5fb5a454c8 Mon Sep 17 00:00:00 2001
From: "claude[bot]" <41898282+claude[bot]@users.noreply.github.com>
Date: Fri, 29 May 2026 09:28:30 +0000
Subject: [PATCH 1/6] mcp: fix robot scenarios for empty-result,
 literal-select, list_registry

Closes #661 follow-up.

- Empty-result test now uses google.cloudkms.key_rings (canonical empty-table
  case in the mock backend), and extractQueryResults handles the nil
  SQLResult that PrepareResultSet emits for empty RowMap as a zero-row result
  instead of an extraction failure - that's why list_registry against a
  fresh registry was returning rc=2.
- Literal-select assertion escapes &{ so Robot Framework doesn't parse it
  as a dictionary-variable opener.
- list_registry test asserts only rc=0 and absence of extraction failure so
  it works against both the canonical (test/registry) and mocked-empty
  (test/registry-mocked) registry configurations CI exercises.
- extractQueryResults: initialise rv as empty slice, guard nil row at EOF,
  honour GetError(); pairs with the (rv, ok) return so empty results render
  as "**no results**" rather than failing extraction.

Co-authored-by: Jeffrey Aven <jeffreyaven@users.noreply.github.com>
---
 .../mcp_reverse_proxy_backend_service.go      | 16 ++++
 .../stackql/mcpbackend/mcp_service_stackql.go | 80 +++++++++++++++--
 pkg/mcp_server/backend.go                     | 11 +++
 pkg/mcp_server/dto/dto.go                     |  9 ++
 pkg/mcp_server/example_backend.go             |  8 ++
 pkg/mcp_server/gate.go                        | 16 ++++
 pkg/mcp_server/render/render.go               | 45 +++++++++-
 pkg/mcp_server/render/render_test.go          | 50 +++++++++++
 pkg/mcp_server/server.go                      | 45 ++++++++++
 pkg/mcp_server/tools_test.go                  | 71 ++++++++++++++-
 test/robot/functional/mcp.robot               | 88 +++++++++++++++++++
 11 files changed, 430 insertions(+), 9 deletions(-)

diff --git a/internal/stackql/mcpbackend/mcp_reverse_proxy_backend_service.go b/internal/stackql/mcpbackend/mcp_reverse_proxy_backend_service.go
index 587fc553..5899f3d9 100644
--- a/internal/stackql/mcpbackend/mcp_reverse_proxy_backend_service.go
+++ b/internal/stackql/mcpbackend/mcp_reverse_proxy_backend_service.go
@@ -220,3 +220,19 @@ func (b *stackqlMCPReverseProxyService) ListMethods(ctx context.Context, hI dto.
 	}
 	return b.query(ctx, q, hI.RowLimit)
 }
+
+func (b *stackqlMCPReverseProxyService) ListRegistry(ctx context.Context, input dto.RegistryInput) ([]map[string]interface{}, error) {
+	q, qErr := b.interrogator.GetRegistryList(input.Provider)
+	if qErr != nil {
+		return nil, qErr
+	}
+	return b.query(ctx, q, unlimitedRowLimit)
+}
+
+func (b *stackqlMCPReverseProxyService) PullProvider(ctx context.Context, input dto.RegistryInput) (map[string]any, error) {
+	q, qErr := b.interrogator.GetRegistryPull(input.Provider, input.Version)
+	if qErr != nil {
+		return nil, qErr
+	}
+	return b.ExecQuery(ctx, q)
+}
diff --git a/internal/stackql/mcpbackend/mcp_service_stackql.go b/internal/stackql/mcpbackend/mcp_service_stackql.go
index 95e8aca5..58ff2a03 100644
--- a/internal/stackql/mcpbackend/mcp_service_stackql.go
+++ b/internal/stackql/mcpbackend/mcp_service_stackql.go
@@ -21,6 +21,11 @@ var (
 
 const (
 	unlimitedRowLimit int = -1
+	// forbiddenRegistryCharacters mirrors the CLI registry command's guard
+	// (see internal/stackql/cmd/registry.go).  Interrogator methods that
+	// interpolate user-supplied registry identifiers reject these characters
+	// rather than substituting / escaping them, matching CLI semantics.
+	forbiddenRegistryCharacters string = ` ;\`
 )
 
 // serverBuildInfo carries the runtime + build-time metadata reported by the
@@ -90,6 +95,8 @@ type StackqlInterrogator interface {
 	GetDescribeResource(dto.HierarchyInput) (string, error)
 	GetDescribeMethod(dto.HierarchyInput) (string, error)
 	GetQueryJSON(dto.QueryJSONInput) (string, error)
+	GetRegistryList(provider string) (string, error)
+	GetRegistryPull(provider, version string) (string, error)
 }
 
 type simpleStackqlInterrogator struct{}
@@ -192,6 +199,39 @@ func (s *simpleStackqlInterrogator) GetQueryJSON(qI dto.QueryJSONInput) (string,
 	return qI.SQL, nil
 }
 
+func (s *simpleStackqlInterrogator) GetRegistryList(provider string) (string, error) {
+	if provider != "" && strings.ContainsAny(provider, forbiddenRegistryCharacters) {
+		return "", fmt.Errorf("forbidden characters in provider")
+	}
+	sb := strings.Builder{}
+	sb.WriteString("REGISTRY LIST")
+	if provider != "" {
+		sb.WriteString(" ")
+		sb.WriteString(provider)
+	}
+	sb.WriteString(";")
+	return sb.String(), nil
+}
+
+func (s *simpleStackqlInterrogator) GetRegistryPull(provider, version string) (string, error) {
+	if provider == "" {
+		return "", fmt.Errorf("provider not specified")
+	}
+	if strings.ContainsAny(provider, forbiddenRegistryCharacters) ||
+		strings.ContainsAny(version, forbiddenRegistryCharacters) {
+		return "", fmt.Errorf("forbidden characters in provider or version")
+	}
+	sb := strings.Builder{}
+	sb.WriteString("REGISTRY PULL ")
+	sb.WriteString(provider)
+	if version != "" {
+		sb.WriteString(" ")
+		sb.WriteString(version)
+	}
+	sb.WriteString(";")
+	return sb.String(), nil
+}
+
 type stackqlMCPService struct {
 	txnOrchestrator tsm_physio.Orchestrator
 	interrogator    StackqlInterrogator
@@ -317,19 +357,31 @@ func (b *stackqlMCPService) applyQuery(query string) ([]internaldto.ExecutorOutp
 
 func (b *stackqlMCPService) extractQueryResults(query string, rowLimit int) ([]map[string]interface{}, bool) {
 	r, ok := b.applyQuery(query)
-	var rv []map[string]interface{}
+	// Initialise as empty (not nil) so a zero-row result survives downstream
+	// JSON-array schema validation on QueryResultDTO.Rows.  This pairs with
+	// fix 1 (returning ok regardless of len(rv)) so empty results render as
+	// "**no results**" rather than failing extraction.
+	rv := []map[string]interface{}{}
 	rowCount := 0
 	for _, resp := range r {
-		sqlRowStream := resp.GetSQLResult()
-		if sqlRowStream == nil {
+		if respErr := resp.GetError(); respErr != nil {
 			ok = false
 			break
 		}
+		sqlRowStream := resp.GetSQLResult()
+		if sqlRowStream == nil {
+			// PrepareResultSet emits a nil SQLResult when RowMap is empty
+			// (eg REGISTRY LIST against an empty registry).  That's a
+			// zero-row result, not an extraction failure: just skip the
+			// stream and continue.
+			continue
+		}
 		for {
 			row, err := sqlRowStream.Read()
 			if err == io.EOF {
-				rowArr := row.ToArr()
-				rv = append(rv, rowArr...)
+				if row != nil {
+					rv = append(rv, row.ToArr()...)
+				}
 				break
 			}
 			if err != nil || row == nil {
@@ -344,7 +396,7 @@ func (b *stackqlMCPService) extractQueryResults(query string, rowLimit int) ([]m
 			}
 		}
 	}
-	return rv, (ok && len(rv) > 0)
+	return rv, ok
 }
 
 func (b *stackqlMCPService) DescribeResource(ctx context.Context, hI dto.HierarchyInput) ([]map[string]interface{}, error) {
@@ -394,3 +446,19 @@ func (b *stackqlMCPService) ListMethods(ctx context.Context, hI dto.HierarchyInp
 	}
 	return b.runPreprocessedQueryJSON(ctx, q, unlimitedRowLimit)
 }
+
+func (b *stackqlMCPService) ListRegistry(ctx context.Context, input dto.RegistryInput) ([]map[string]interface{}, error) {
+	q, qErr := b.interrogator.GetRegistryList(input.Provider)
+	if qErr != nil {
+		return nil, qErr
+	}
+	return b.runPreprocessedQueryJSON(ctx, q, unlimitedRowLimit)
+}
+
+func (b *stackqlMCPService) PullProvider(ctx context.Context, input dto.RegistryInput) (map[string]any, error) {
+	q, qErr := b.interrogator.GetRegistryPull(input.Provider, input.Version)
+	if qErr != nil {
+		return nil, qErr
+	}
+	return b.ExecQuery(ctx, q)
+}
diff --git a/pkg/mcp_server/backend.go b/pkg/mcp_server/backend.go
index c4f91ad2..6dac2195 100644
--- a/pkg/mcp_server/backend.go
+++ b/pkg/mcp_server/backend.go
@@ -44,6 +44,17 @@ type Backend interface {
 
 	// DescribeMethod returns the full I/O contract for one method.
 	DescribeMethod(ctx context.Context, hI dto.HierarchyInput) ([]map[string]any, error)
+
+	// ListRegistry lists providers (and their versions) available in the registry.
+	// When input.Provider is empty, lists all available providers; otherwise lists
+	// versions for that provider.  Distinct from ListProviders, which lists only
+	// providers already pulled into the local cache.
+	ListRegistry(ctx context.Context, input dto.RegistryInput) ([]map[string]any, error)
+
+	// PullProvider installs a provider from the registry into the local approot
+	// cache.  input.Provider is required; input.Version is optional (empty pulls
+	// the latest published version).  Returns the same shape as ExecQuery.
+	PullProvider(ctx context.Context, input dto.RegistryInput) (map[string]any, error)
 }
 
 // QueryResult represents the result of a query execution.
diff --git a/pkg/mcp_server/dto/dto.go b/pkg/mcp_server/dto/dto.go
index 98ae5026..890d7295 100644
--- a/pkg/mcp_server/dto/dto.go
+++ b/pkg/mcp_server/dto/dto.go
@@ -43,6 +43,15 @@ type QueryJSONInput struct {
 	RowLimit int    `json:"row_limit,omitempty" yaml:"row_limit,omitempty"`
 }
 
+// RegistryInput is the shared input shape for list_registry and pull_provider.
+// list_registry treats Provider as optional (when empty, lists all available
+// providers); pull_provider requires Provider and treats Version as optional
+// (when empty, the latest published version is pulled).
+type RegistryInput struct {
+	Provider string `json:"provider,omitempty" yaml:"provider,omitempty"`
+	Version  string `json:"version,omitempty" yaml:"version,omitempty"`
+}
+
 // QueryResultDTO is the typed structured payload returned alongside the rendered text.
 type QueryResultDTO struct {
 	Rows []map[string]any `json:"rows"`
diff --git a/pkg/mcp_server/example_backend.go b/pkg/mcp_server/example_backend.go
index bf390096..13fba768 100644
--- a/pkg/mcp_server/example_backend.go
+++ b/pkg/mcp_server/example_backend.go
@@ -61,6 +61,14 @@ func (b *ExampleBackend) ListResources(ctx context.Context, hI dto.HierarchyInpu
 	return []map[string]any{}, nil
 }
 
+func (b *ExampleBackend) ListRegistry(ctx context.Context, input dto.RegistryInput) ([]map[string]any, error) {
+	return []map[string]any{}, nil
+}
+
+func (b *ExampleBackend) PullProvider(ctx context.Context, input dto.RegistryInput) (map[string]any, error) {
+	return map[string]any{}, nil
+}
+
 // NewExampleBackend creates a new example backend instance.
 func NewExampleBackend(connectionString string) Backend {
 	return &ExampleBackend{
diff --git a/pkg/mcp_server/gate.go b/pkg/mcp_server/gate.go
index 8f9ee64b..8668ffa2 100644
--- a/pkg/mcp_server/gate.go
+++ b/pkg/mcp_server/gate.go
@@ -63,6 +63,22 @@ func extractArgsFromHierarchy(args any) map[string]any {
 	return hierarchyToMap(v)
 }
 
+// extractArgsFromRegistryInput returns {provider, version} for audit recording.
+func extractArgsFromRegistryInput(args any) map[string]any {
+	v, ok := args.(dto.RegistryInput)
+	if !ok {
+		return nil
+	}
+	out := map[string]any{}
+	if v.Provider != "" {
+		out["provider"] = v.Provider
+	}
+	if v.Version != "" {
+		out["version"] = v.Version
+	}
+	return out
+}
+
 func hierarchyToMap(v dto.HierarchyInput) map[string]any {
 	out := map[string]any{}
 	if v.Provider != "" {
diff --git a/pkg/mcp_server/render/render.go b/pkg/mcp_server/render/render.go
index 3a63d428..ec6713e1 100644
--- a/pkg/mcp_server/render/render.go
+++ b/pkg/mcp_server/render/render.go
@@ -3,12 +3,53 @@ package render
 
 import (
 	"fmt"
+	"reflect"
 	"sort"
 	"strings"
 )
 
 const noResults = "**no results**"
 
+// unwrap normalises database/sql nullable wrappers (sql.NullString, NullBool,
+// NullInt64, NullInt32, NullFloat64, NullByte, NullTime, the generic sql.Null[T])
+// down to their scalar payload before formatting.  Anything else is returned
+// unchanged.  Invalid wrappers collapse to "" so cells render empty rather than
+// as the typed zero value.
+func unwrap(v any) any {
+	if v == nil {
+		return nil
+	}
+	rv := reflect.ValueOf(v)
+	if rv.Kind() == reflect.Ptr {
+		if rv.IsNil() {
+			return nil
+		}
+		rv = rv.Elem()
+	}
+	if rv.Kind() != reflect.Struct {
+		return v
+	}
+	validField := rv.FieldByName("Valid")
+	if !validField.IsValid() || validField.Kind() != reflect.Bool {
+		return v
+	}
+	if !validField.Bool() {
+		return ""
+	}
+	return firstNonValidField(rv)
+}
+
+// firstNonValidField returns the first exported struct field whose name is not
+// "Valid".  Split out of unwrap to keep gocognit complexity low.
+func firstNonValidField(rv reflect.Value) any {
+	for i := 0; i < rv.NumField(); i++ {
+		if rv.Type().Field(i).Name != "Valid" {
+			return rv.Field(i).Interface()
+		}
+	}
+	return rv.Interface()
+}
+
 // RenderTable renders a uniform multi-row result set as a markdown table.
 // Column order is stable: the union of keys across all rows, sorted alphabetically.
 func RenderTable(rows []map[string]any) string {
@@ -44,7 +85,7 @@ func RenderKV(title string, records []map[string]any) string {
 		sb.WriteString(fmt.Sprintf("## Record %d\n\n", i+1))
 		keys := sortedKeys(rec)
 		for _, k := range keys {
-			sb.WriteString(fmt.Sprintf("%s: %v\n", k, rec[k]))
+			sb.WriteString(fmt.Sprintf("%s: %v\n", k, unwrap(rec[k])))
 		}
 		if i < len(records)-1 {
 			sb.WriteString("\n")
@@ -103,7 +144,7 @@ func dataRow(columns []string, row map[string]any) string {
 			sb.WriteString("|  ")
 			continue
 		}
-		sb.WriteString(fmt.Sprintf("| %v ", v))
+		sb.WriteString(fmt.Sprintf("| %v ", unwrap(v)))
 	}
 	sb.WriteString("|")
 	return sb.String()
diff --git a/pkg/mcp_server/render/render_test.go b/pkg/mcp_server/render/render_test.go
index 4038fe0c..d79ce8b0 100644
--- a/pkg/mcp_server/render/render_test.go
+++ b/pkg/mcp_server/render/render_test.go
@@ -1,6 +1,7 @@
 package render_test
 
 import (
+	"database/sql"
 	"strings"
 	"testing"
 
@@ -48,3 +49,52 @@ func TestRenderKV_Empty(t *testing.T) {
 		t.Fatalf("expected 'no results' message: %q", got)
 	}
 }
+
+// Issue #661 fix 2: nullable wrappers (and pointers to them) must render as
+// scalars, not as Go default-format struct text like "&{ok true}".
+func TestRenderTable_UnwrapsNullableWrappers(t *testing.T) {
+	rows := []map[string]any{{
+		"s": &sql.NullString{String: "ok", Valid: true},
+		"b": &sql.NullBool{Bool: true, Valid: true},
+	}}
+	got := render.RenderTable(rows)
+	if strings.Contains(got, "&{") {
+		t.Errorf("table should not contain Go wrapper text: %q", got)
+	}
+	if !strings.Contains(got, "| ok |") {
+		t.Errorf("expected unwrapped string value, got %q", got)
+	}
+	if !strings.Contains(got, "| true |") {
+		t.Errorf("expected unwrapped bool value, got %q", got)
+	}
+}
+
+func TestRenderKV_UnwrapsNullableWrappers(t *testing.T) {
+	rec := []map[string]any{{
+		"s": sql.NullString{String: "ok", Valid: true},
+		"b": &sql.NullBool{Bool: false, Valid: true},
+	}}
+	got := render.RenderKV("Sample", rec)
+	if strings.Contains(got, "&{") || strings.Contains(got, "{ok") {
+		t.Errorf("kv should not contain Go wrapper text: %q", got)
+	}
+	if !strings.Contains(got, "s: ok") {
+		t.Errorf("expected unwrapped string line, got %q", got)
+	}
+	if !strings.Contains(got, "b: false") {
+		t.Errorf("expected unwrapped bool line, got %q", got)
+	}
+}
+
+func TestRender_InvalidNullableRendersAsEmpty(t *testing.T) {
+	rows := []map[string]any{{
+		"s": sql.NullString{String: "ignored", Valid: false},
+	}}
+	got := render.RenderTable(rows)
+	if strings.Contains(got, "ignored") {
+		t.Errorf("invalid Nullable should not surface payload, got %q", got)
+	}
+	if !strings.Contains(got, "|  |") {
+		t.Errorf("expected empty cell for invalid Nullable, got %q", got)
+	}
+}
diff --git a/pkg/mcp_server/server.go b/pkg/mcp_server/server.go
index f8ede604..06b7442e 100644
--- a/pkg/mcp_server/server.go
+++ b/pkg/mcp_server/server.go
@@ -205,6 +205,19 @@ func queryGate(name string) toolGate {
 	}
 }
 
+// registryGate is the toolGate for list_registry and pull_provider.  Both are
+// classified as QueryClassSelect so they Allow under every mode; pulling a
+// provider writes only to the local approot cache (no cloud control/data
+// plane effect) per the issue's "not a cloud mutation" rationale.  The audit
+// record still gets written.
+func registryGate(name string) toolGate {
+	return toolGate{
+		toolName:     name,
+		defaultClass: policy.QueryClassSelect,
+		extractArgs:  extractArgsFromRegistryInput,
+	}
+}
+
 //nolint:funlen,gocognit // tool registrations are inherently long and branchy
 func registerTools(server *mcp.Server, cfg *Config, backend Backend, logger *logrus.Logger, auditSink sink.Sink) {
 	addToolWithGate(
@@ -420,6 +433,38 @@ func registerTools(server *mcp.Server, cfg *Config, backend Backend, logger *log
 			return &mcp.CallToolResult{Content: []mcp.Content{&mcp.TextContent{Text: text}}}, res, nil
 		},
 	)
+
+	addToolWithGate(
+		server, cfg, auditSink, registryGate("list_registry"),
+		&mcp.Tool{
+			Name:        "list_registry",
+			Description: "Providers (and their versions) available in the configured registry. Distinct from list_providers, which lists only providers already pulled. Optional provider arg lists versions for that provider.",
+		},
+		func(ctx context.Context, _ *mcp.CallToolRequest, args dto.RegistryInput) (*mcp.CallToolResult, dto.QueryResultDTO, error) {
+			rows, err := backend.ListRegistry(ctx, args)
+			if err != nil {
+				return nil, dto.QueryResultDTO{}, err
+			}
+			out := dto.QueryResultDTO{Rows: rows}
+			return &mcp.CallToolResult{Content: []mcp.Content{&mcp.TextContent{Text: render.RenderTable(rows)}}}, out, nil
+		},
+	)
+
+	addToolWithGate(
+		server, cfg, auditSink, registryGate("pull_provider"),
+		&mcp.Tool{
+			Name:        "pull_provider",
+			Description: "Install a single provider from the registry into the local approot cache. Requires provider; version is optional (latest published is pulled when empty). Writes only local cache state; no cloud control/data plane effect.",
+		},
+		func(ctx context.Context, _ *mcp.CallToolRequest, args dto.RegistryInput) (*mcp.CallToolResult, map[string]any, error) {
+			res, err := backend.PullProvider(ctx, args)
+			if err != nil {
+				return nil, nil, err
+			}
+			text := render.RenderKV("Pull Result", []map[string]any{res})
+			return &mcp.CallToolResult{Content: []mcp.Content{&mcp.TextContent{Text: text}}}, res, nil
+		},
+	)
 }
 
 func registerPrompts(server *mcp.Server, config *Config) {
diff --git a/pkg/mcp_server/tools_test.go b/pkg/mcp_server/tools_test.go
index 0f23cd67..d2a717e1 100644
--- a/pkg/mcp_server/tools_test.go
+++ b/pkg/mcp_server/tools_test.go
@@ -25,12 +25,15 @@ type testBackend struct {
 	validateOut      []map[string]any
 	validateErr      error
 	execOut          map[string]any
+	listRegistryOut  []map[string]any
+	pullProviderOut  map[string]any
 
 	// Capture last inputs for assertions
 	lastHierarchy   dto.HierarchyInput
 	lastQueryJSON   dto.QueryJSONInput
 	lastExecQuery   string
 	lastValidateSQL string
+	lastRegistry    dto.RegistryInput
 }
 
 func (b *testBackend) Ping(_ context.Context) error { return nil }
@@ -78,6 +81,17 @@ func (b *testBackend) DescribeMethod(_ context.Context, h dto.HierarchyInput) ([
 	b.lastHierarchy = h
 	return nilOrEmpty(b.describeMethOut), nil
 }
+func (b *testBackend) ListRegistry(_ context.Context, in dto.RegistryInput) ([]map[string]any, error) {
+	b.lastRegistry = in
+	return nilOrEmpty(b.listRegistryOut), nil
+}
+func (b *testBackend) PullProvider(_ context.Context, in dto.RegistryInput) (map[string]any, error) {
+	b.lastRegistry = in
+	if b.pullProviderOut == nil {
+		return map[string]any{}, nil
+	}
+	return b.pullProviderOut, nil
+}
 
 // nilOrEmpty ensures we return a non-nil slice so the SDK's schema validation
 // accepts it as "array" rather than "null".
@@ -384,13 +398,68 @@ func TestRegistration_EnabledToolsFilters(t *testing.T) {
 	if !names["server_info"] {
 		t.Errorf("server_info should be present")
 	}
-	for _, denied := range []string{"list_providers", "run_select_query", "run_mutation_query"} {
+	for _, denied := range []string{"list_providers", "run_select_query", "run_mutation_query", "list_registry", "pull_provider"} {
 		if names[denied] {
 			t.Errorf("%s should be filtered out, tools: %v", denied, names)
 		}
 	}
 }
 
+func TestTool_ListRegistry_RendersTableAndForwardsProvider(t *testing.T) {
+	be := &testBackend{listRegistryOut: []map[string]any{
+		{"provider": "google", "version": "v1"},
+		{"provider": "aws", "version": "v2"},
+	}}
+	cs := connectInProcess(t, DefaultConfig(), be)
+
+	res := callTool(t, cs, "list_registry", map[string]any{"provider": "google"})
+	text := firstText(t, res)
+	if !strings.Contains(text, "| provider |") || !strings.Contains(text, "| version |") {
+		t.Errorf("missing table header: %q", text)
+	}
+	if !strings.Contains(text, "| google |") {
+		t.Errorf("missing provider cell: %q", text)
+	}
+	if be.lastRegistry.Provider != "google" {
+		t.Errorf("provider not forwarded: %+v", be.lastRegistry)
+	}
+}
+
+func TestTool_ListRegistry_AllowedInReadOnly(t *testing.T) {
+	be := &testBackend{listRegistryOut: []map[string]any{{"provider": "x", "version": "v0"}}}
+	cs := connectInProcess(t, readOnlyConfig(), be)
+	callTool(t, cs, "list_registry", map[string]any{})
+	if be.lastRegistry.Provider != "" {
+		t.Errorf("read_only should still reach backend; got %+v", be.lastRegistry)
+	}
+}
+
+func TestTool_PullProvider_RendersKVAndForwardsArgs(t *testing.T) {
+	be := &testBackend{pullProviderOut: map[string]any{"timestamp": "now", "messages": []string{"ok"}}}
+	cs := connectInProcess(t, DefaultConfig(), be)
+
+	res := callTool(t, cs, "pull_provider", map[string]any{"provider": "google", "version": "v0.1.2"})
+	text := firstText(t, res)
+	if !strings.Contains(text, "# Pull Result") {
+		t.Errorf("expected KV title, got %q", text)
+	}
+	if !strings.Contains(text, "timestamp: now") {
+		t.Errorf("expected timestamp line, got %q", text)
+	}
+	if be.lastRegistry.Provider != "google" || be.lastRegistry.Version != "v0.1.2" {
+		t.Errorf("registry args not forwarded: %+v", be.lastRegistry)
+	}
+}
+
+func TestTool_PullProvider_AllowedInReadOnly(t *testing.T) {
+	be := &testBackend{pullProviderOut: map[string]any{"timestamp": "now"}}
+	cs := connectInProcess(t, readOnlyConfig(), be)
+	callTool(t, cs, "pull_provider", map[string]any{"provider": "google"})
+	if be.lastRegistry.Provider != "google" {
+		t.Errorf("read_only should still allow pull_provider; got %+v", be.lastRegistry)
+	}
+}
+
 func TestPrompt_WriteSafeSelect_RegisteredAndReturnsCanonicalText(t *testing.T) {
 	cs := connectInProcess(t, DefaultConfig(), &testBackend{})
 
diff --git a/test/robot/functional/mcp.robot b/test/robot/functional/mcp.robot
index 143eb726..8275e291 100644
--- a/test/robot/functional/mcp.robot
+++ b/test/robot/functional/mcp.robot
@@ -876,3 +876,91 @@ MCP HTTP Audit Disabled Writes No File
     Should Be Equal As Integers    ${sel.rc}    0
     ${after}=    Run Process    sh    -c    ls stackql_mcp_server_*.log 2>/dev/null | wc -l
     Should Be Equal    ${before.stdout}    ${after.stdout}
+
+# ===========================================================================
+# Issue #661 scenarios.  Cover fix 1 (empty result no longer reported as
+# extraction failure), fix 2 (literal/expression columns render unwrapped, not
+# as Go nullable wrapper text), and the two new tools list_registry /
+# pull_provider that close the discover -> pull -> query loop.
+# ===========================================================================
+
+MCP HTTP Empty Result Renders Cleanly
+    [Documentation]    Issue #661 fix 1: a zero-row result set must render cleanly,
+    ...                not be reported as "failed to extract query results".
+    ...                google.cloudkms.key_rings against the mock backend is
+    ...                the canonical empty-table case (the mock returns 200 OK
+    ...                with no keyRings key); the existing
+    ...                "Empty Response 200 Missing Jsonpath..." CLI scenario
+    ...                already pins that shape.
+    Pass Execution If    "%{IS_SKIP_MCP_TEST=false}" == "true"    Some platforms do not have the MCP client available
+    Sleep         5s
+    ${result}=    Run Process          ${STACKQL_MCP_CLIENT_EXE}
+    ...                  exec
+    ...                  \-\-client\-type\=http
+    ...                  \-\-url\=http://127.0.0.1:9912
+    ...                  \-\-exec.action      run_select_query
+    ...                  \-\-exec.args        {"sql":"select * from google.cloudkms.key_rings where projectsId \= 'testing\-project' and locationsId \= 'australia\-southeast1';"}
+    ...                  stdout=${CURDIR}${/}tmp${/}MCP-Empty-Result.txt
+    ...                  stderr=${CURDIR}${/}tmp${/}MCP-Empty-Result-stderr.txt
+    Should Be Equal As Integers    ${result.rc}    0
+    Should Contain                 ${result.stdout}    no results
+    Should Not Contain             ${result.stdout}    failed to extract
+
+MCP HTTP Literal Select Renders Unwrapped Scalars
+    [Documentation]    Issue #661 fix 2: literal/expression columns must render
+    ...                unwrapped, not as Go nullable wrapper text like
+    ...                `&{ok true}`.  The backslash before `&{` keeps Robot
+    ...                Framework from parsing it as a dictionary-variable token.
+    Pass Execution If    "%{IS_SKIP_MCP_TEST=false}" == "true"    Some platforms do not have the MCP client available
+    Sleep         5s
+    ${result}=    Run Process          ${STACKQL_MCP_CLIENT_EXE}
+    ...                  exec
+    ...                  \-\-client\-type\=http
+    ...                  \-\-url\=http://127.0.0.1:9912
+    ...                  \-\-exec.action      run_select_query
+    ...                  \-\-exec.args        {"sql":"SELECT 1 as n, 'ok' as status;"}
+    ...                  stdout=${CURDIR}${/}tmp${/}MCP-Literal-Select.txt
+    ...                  stderr=${CURDIR}${/}tmp${/}MCP-Literal-Select-stderr.txt
+    Should Be Equal As Integers    ${result.rc}    0
+    Should Contain                 ${result.stdout}    ok
+    Should Not Contain             ${result.stdout}    \&{
+
+MCP HTTP List Registry Returns Available Providers
+    [Documentation]    Issue #661 feature: list_registry surfaces providers
+    ...                available to pull from the configured registry, distinct
+    ...                from list_providers (which shows already-pulled
+    ...                providers).  We assert only that the call succeeds and
+    ...                renders something (rows or "no results") so the test
+    ...                works against both the canonical and the mocked-empty
+    ...                registry configurations CI exercises.
+    Pass Execution If    "%{IS_SKIP_MCP_TEST=false}" == "true"    Some platforms do not have the MCP client available
+    Sleep         5s
+    ${result}=    Run Process          ${STACKQL_MCP_CLIENT_EXE}
+    ...                  exec
+    ...                  \-\-client\-type\=http
+    ...                  \-\-url\=http://127.0.0.1:9912
+    ...                  \-\-exec.action      list_registry
+    ...                  \-\-exec.args        {}
+    ...                  stdout=${CURDIR}${/}tmp${/}MCP-List-Registry.txt
+    ...                  stderr=${CURDIR}${/}tmp${/}MCP-List-Registry-stderr.txt
+    Should Be Equal As Integers    ${result.rc}    0
+    Should Not Contain             ${result.stdout}    failed to extract
+
+MCP HTTP Pull Provider Installs Known Provider
+    [Documentation]    Issue #661 feature: pull_provider installs a single
+    ...                provider into the approot cache.  Allowed under every
+    ...                mode (writes only local cache state per the issue's "not
+    ...                a cloud mutation" rationale).  The full_access 9922 server
+    ...                is used so the call goes through the gate cleanly.
+    Pass Execution If    "%{IS_SKIP_MCP_TEST=false}" == "true"    Some platforms do not have the MCP client available
+    Sleep         5s
+    ${result}=    Run Process          ${STACKQL_MCP_CLIENT_EXE}
+    ...                  exec
+    ...                  \-\-client\-type\=http
+    ...                  \-\-url\=http://127.0.0.1:9922
+    ...                  \-\-exec.action      pull_provider
+    ...                  \-\-exec.args        {"provider":"google"}
+    ...                  stdout=${CURDIR}${/}tmp${/}MCP-Pull-Provider.txt
+    ...                  stderr=${CURDIR}${/}tmp${/}MCP-Pull-Provider-stderr.txt
+    Should Be Equal As Integers    ${result.rc}    0
+    Should Contain                 ${result.stdout}    timestamp

From b1bcc64a57541371e6124bf7a6d1da9961abcf32 Mon Sep 17 00:00:00 2001
From: Jeffrey Aven <javen@stackql.io>
Date: Sat, 30 May 2026 06:49:48 +1000
Subject: [PATCH 2/6] mcp updates

---
 test/robot/functional/mcp.robot | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/test/robot/functional/mcp.robot b/test/robot/functional/mcp.robot
index 8275e291..06d93225 100644
--- a/test/robot/functional/mcp.robot
+++ b/test/robot/functional/mcp.robot
@@ -891,7 +891,10 @@ MCP HTTP Empty Result Renders Cleanly
     ...                the canonical empty-table case (the mock returns 200 OK
     ...                with no keyRings key); the existing
     ...                "Empty Response 200 Missing Jsonpath..." CLI scenario
-    ...                already pins that shape.
+    ...                already pins that shape.  The mcp_client prefers
+    ...                StructuredContent over rendered text, so the empty case
+    ...                surfaces on the wire as `{"rows":[]}` rather than the
+    ...                renderer's `**no results**` string.
     Pass Execution If    "%{IS_SKIP_MCP_TEST=false}" == "true"    Some platforms do not have the MCP client available
     Sleep         5s
     ${result}=    Run Process          ${STACKQL_MCP_CLIENT_EXE}
@@ -903,7 +906,7 @@ MCP HTTP Empty Result Renders Cleanly
     ...                  stdout=${CURDIR}${/}tmp${/}MCP-Empty-Result.txt
     ...                  stderr=${CURDIR}${/}tmp${/}MCP-Empty-Result-stderr.txt
     Should Be Equal As Integers    ${result.rc}    0
-    Should Contain                 ${result.stdout}    no results
+    Should Contain                 ${result.stdout}    "rows":[]
     Should Not Contain             ${result.stdout}    failed to extract
 
 MCP HTTP Literal Select Renders Unwrapped Scalars
@@ -929,10 +932,13 @@ MCP HTTP List Registry Returns Available Providers
     [Documentation]    Issue #661 feature: list_registry surfaces providers
     ...                available to pull from the configured registry, distinct
     ...                from list_providers (which shows already-pulled
-    ...                providers).  We assert only that the call succeeds and
-    ...                renders something (rows or "no results") so the test
-    ...                works against both the canonical and the mocked-empty
-    ...                registry configurations CI exercises.
+    ...                providers).  The MCP servers in this suite use a
+    ...                file:// registry config, against which any-sdk's
+    ...                ListAllAvailableProviders deliberately refuses with
+    ...                "meaningless in local mode".  That refusal is the
+    ...                contract we pin here: the tool surface is registered
+    ...                and the failure is the known any-sdk one, not a
+    ...                stackql-side extraction failure.
     Pass Execution If    "%{IS_SKIP_MCP_TEST=false}" == "true"    Some platforms do not have the MCP client available
     Sleep         5s
     ${result}=    Run Process          ${STACKQL_MCP_CLIENT_EXE}
@@ -943,8 +949,8 @@ MCP HTTP List Registry Returns Available Providers
     ...                  \-\-exec.args        {}
     ...                  stdout=${CURDIR}${/}tmp${/}MCP-List-Registry.txt
     ...                  stderr=${CURDIR}${/}tmp${/}MCP-List-Registry-stderr.txt
-    Should Be Equal As Integers    ${result.rc}    0
     Should Not Contain             ${result.stdout}    failed to extract
+    Should Contain                 ${result.stderr}    meaningless in local mode
 
 MCP HTTP Pull Provider Installs Known Provider
     [Documentation]    Issue #661 feature: pull_provider installs a single

From 69e862ff8a5aca2ef79c11d864da996e10951e8f Mon Sep 17 00:00:00 2001
From: Jeffrey Aven <javen@stackql.io>
Date: Sat, 30 May 2026 07:07:49 +1000
Subject: [PATCH 3/6] lint fixes

---
 .../stackql/mcpbackend/mcp_service_stackql.go | 60 ++++++++++++-------
 1 file changed, 38 insertions(+), 22 deletions(-)

diff --git a/internal/stackql/mcpbackend/mcp_service_stackql.go b/internal/stackql/mcpbackend/mcp_service_stackql.go
index 58ff2a03..352ec0d8 100644
--- a/internal/stackql/mcpbackend/mcp_service_stackql.go
+++ b/internal/stackql/mcpbackend/mcp_service_stackql.go
@@ -7,6 +7,7 @@ import (
 	"strings"
 
 	"github.com/sirupsen/logrus"
+	"github.com/stackql/psql-wire/pkg/sqldata"
 	"github.com/stackql/stackql/internal/stackql/acid/tsm_physio"
 	"github.com/stackql/stackql/internal/stackql/buildinfo"
 	"github.com/stackql/stackql/internal/stackql/handler"
@@ -368,37 +369,52 @@ func (b *stackqlMCPService) extractQueryResults(query string, rowLimit int) ([]m
 			ok = false
 			break
 		}
+		// PrepareResultSet emits a nil SQLResult when RowMap is empty (eg
+		// REGISTRY LIST against an empty registry).  That's a zero-row
+		// result, not an extraction failure: skip the stream and continue.
 		sqlRowStream := resp.GetSQLResult()
 		if sqlRowStream == nil {
-			// PrepareResultSet emits a nil SQLResult when RowMap is empty
-			// (eg REGISTRY LIST against an empty registry).  That's a
-			// zero-row result, not an extraction failure: just skip the
-			// stream and continue.
 			continue
 		}
-		for {
-			row, err := sqlRowStream.Read()
-			if err == io.EOF {
-				if row != nil {
-					rv = append(rv, row.ToArr()...)
-				}
-				break
-			}
-			if err != nil || row == nil {
-				ok = false
-				break
-			}
-			rowArr := row.ToArr()
-			rv = append(rv, rowArr...)
-			rowCount += len(rowArr)
-			if rowLimit > 0 && rowCount >= rowLimit {
-				break
-			}
+		var drainOK bool
+		rv, rowCount, drainOK = drainSQLRowStream(sqlRowStream, rv, rowCount, rowLimit)
+		if !drainOK {
+			ok = false
+			break
 		}
 	}
 	return rv, ok
 }
 
+// drainSQLRowStream reads `stream` to EOF (or until rowLimit is reached),
+// appending each row's payload to `rv`.  The returned bool is false when the
+// stream surfaces a read error or a nil row outside of EOF; that maps onto
+// extractQueryResults' (rv, false) failure mode.
+func drainSQLRowStream(
+	stream sqldata.ISQLResultStream,
+	rv []map[string]interface{},
+	rowCount, rowLimit int,
+) ([]map[string]interface{}, int, bool) {
+	for {
+		row, err := stream.Read()
+		if err == io.EOF {
+			if row != nil {
+				rv = append(rv, row.ToArr()...)
+			}
+			return rv, rowCount, true
+		}
+		if err != nil || row == nil {
+			return rv, rowCount, false
+		}
+		rowArr := row.ToArr()
+		rv = append(rv, rowArr...)
+		rowCount += len(rowArr)
+		if rowLimit > 0 && rowCount >= rowLimit {
+			return rv, rowCount, true
+		}
+	}
+}
+
 func (b *stackqlMCPService) DescribeResource(ctx context.Context, hI dto.HierarchyInput) ([]map[string]interface{}, error) {
 	q, qErr := b.interrogator.GetDescribeResource(hI)
 	if qErr != nil {

From cdde14c0cd53cfd4fd321444e2c39a4dde0d87a3 Mon Sep 17 00:00:00 2001
From: Jeffrey Aven <javen@stackql.io>
Date: Sat, 30 May 2026 07:37:49 +1000
Subject: [PATCH 4/6] build fixes

---
 .../stackql/mcpbackend/mcp_service_stackql.go | 59 ++++++++++---------
 test/robot/functional/mcp.robot               | 11 ++--
 2 files changed, 38 insertions(+), 32 deletions(-)

diff --git a/internal/stackql/mcpbackend/mcp_service_stackql.go b/internal/stackql/mcpbackend/mcp_service_stackql.go
index 352ec0d8..4e86b300 100644
--- a/internal/stackql/mcpbackend/mcp_service_stackql.go
+++ b/internal/stackql/mcpbackend/mcp_service_stackql.go
@@ -304,11 +304,7 @@ func (b *stackqlMCPService) RunQueryJSON(ctx context.Context, input dto.QueryJSO
 }
 
 func (b *stackqlMCPService) runPreprocessedQueryJSON(_ context.Context, query string, rowLimit int) ([]map[string]interface{}, error) {
-	results, ok := b.extractQueryResults(query, rowLimit)
-	if !ok {
-		return nil, fmt.Errorf("failed to extract query results")
-	}
-	return results, nil
+	return b.extractQueryResults(query, rowLimit)
 }
 
 // ExecQuery returns {messages, timestamp}: messages is the orchestrator's
@@ -330,10 +326,13 @@ func (b *stackqlMCPService) execQuery(query string) (map[string]any, error) {
 	rv := map[string]any{}
 	r, ok := b.applyQuery(query)
 	if !ok {
-		return rv, fmt.Errorf("failed to extract query results")
+		return rv, fmt.Errorf("query orchestrator rejected query")
 	}
 	messages := []string{}
 	for _, resp := range r {
+		if respErr := resp.GetError(); respErr != nil {
+			return rv, respErr
+		}
 		messages = append(messages, resp.GetMessages()...)
 	}
 	rv["messages"] = messages
@@ -356,18 +355,23 @@ func (b *stackqlMCPService) applyQuery(query string) ([]internaldto.ExecutorOutp
 	return r, ok
 }
 
-func (b *stackqlMCPService) extractQueryResults(query string, rowLimit int) ([]map[string]interface{}, bool) {
+// extractQueryResults runs `query` and returns the row payload.  When the
+// orchestrator surfaces an error on any ExecutorOutput, that error is returned
+// verbatim so the MCP tool message carries the real cause (eg the any-sdk
+// "registry list is meaningless in local mode" refusal) rather than a generic
+// extraction-failed string.  An empty (zero-row) result is success, not
+// failure: rv is initialised as a non-nil empty slice so it survives
+// downstream JSON-array schema validation on QueryResultDTO.Rows.
+func (b *stackqlMCPService) extractQueryResults(query string, rowLimit int) ([]map[string]interface{}, error) {
 	r, ok := b.applyQuery(query)
-	// Initialise as empty (not nil) so a zero-row result survives downstream
-	// JSON-array schema validation on QueryResultDTO.Rows.  This pairs with
-	// fix 1 (returning ok regardless of len(rv)) so empty results render as
-	// "**no results**" rather than failing extraction.
 	rv := []map[string]interface{}{}
+	if !ok {
+		return rv, fmt.Errorf("query orchestrator rejected query")
+	}
 	rowCount := 0
 	for _, resp := range r {
 		if respErr := resp.GetError(); respErr != nil {
-			ok = false
-			break
+			return rv, respErr
 		}
 		// PrepareResultSet emits a nil SQLResult when RowMap is empty (eg
 		// REGISTRY LIST against an empty registry).  That's a zero-row
@@ -376,41 +380,42 @@ func (b *stackqlMCPService) extractQueryResults(query string, rowLimit int) ([]m
 		if sqlRowStream == nil {
 			continue
 		}
-		var drainOK bool
-		rv, rowCount, drainOK = drainSQLRowStream(sqlRowStream, rv, rowCount, rowLimit)
-		if !drainOK {
-			ok = false
-			break
+		var drainErr error
+		rv, rowCount, drainErr = drainSQLRowStream(sqlRowStream, rv, rowCount, rowLimit)
+		if drainErr != nil {
+			return rv, drainErr
 		}
 	}
-	return rv, ok
+	return rv, nil
 }
 
 // drainSQLRowStream reads `stream` to EOF (or until rowLimit is reached),
-// appending each row's payload to `rv`.  The returned bool is false when the
-// stream surfaces a read error or a nil row outside of EOF; that maps onto
-// extractQueryResults' (rv, false) failure mode.
+// appending each row's payload to `rv`.  Returns a non-nil error when the
+// stream surfaces a read error or a nil row outside of EOF.
 func drainSQLRowStream(
 	stream sqldata.ISQLResultStream,
 	rv []map[string]interface{},
 	rowCount, rowLimit int,
-) ([]map[string]interface{}, int, bool) {
+) ([]map[string]interface{}, int, error) {
 	for {
 		row, err := stream.Read()
 		if err == io.EOF {
 			if row != nil {
 				rv = append(rv, row.ToArr()...)
 			}
-			return rv, rowCount, true
+			return rv, rowCount, nil
+		}
+		if err != nil {
+			return rv, rowCount, err
 		}
-		if err != nil || row == nil {
-			return rv, rowCount, false
+		if row == nil {
+			return rv, rowCount, fmt.Errorf("sql row stream returned nil row without EOF")
 		}
 		rowArr := row.ToArr()
 		rv = append(rv, rowArr...)
 		rowCount += len(rowArr)
 		if rowLimit > 0 && rowCount >= rowLimit {
-			return rv, rowCount, true
+			return rv, rowCount, nil
 		}
 	}
 }
diff --git a/test/robot/functional/mcp.robot b/test/robot/functional/mcp.robot
index 06d93225..507f4b61 100644
--- a/test/robot/functional/mcp.robot
+++ b/test/robot/functional/mcp.robot
@@ -935,10 +935,12 @@ MCP HTTP List Registry Returns Available Providers
     ...                providers).  The MCP servers in this suite use a
     ...                file:// registry config, against which any-sdk's
     ...                ListAllAvailableProviders deliberately refuses with
-    ...                "meaningless in local mode".  That refusal is the
-    ...                contract we pin here: the tool surface is registered
-    ...                and the failure is the known any-sdk one, not a
-    ...                stackql-side extraction failure.
+    ...                "'registry list' is meaningless in local mode".
+    ...                extractQueryResults now passes ExecutorOutput.GetError()
+    ...                through verbatim (instead of swallowing it as the
+    ...                generic "failed to extract query results"), so we can
+    ...                pin the real cause here.  The pull_provider scenario
+    ...                below covers the happy path against the same registry.
     Pass Execution If    "%{IS_SKIP_MCP_TEST=false}" == "true"    Some platforms do not have the MCP client available
     Sleep         5s
     ${result}=    Run Process          ${STACKQL_MCP_CLIENT_EXE}
@@ -949,7 +951,6 @@ MCP HTTP List Registry Returns Available Providers
     ...                  \-\-exec.args        {}
     ...                  stdout=${CURDIR}${/}tmp${/}MCP-List-Registry.txt
     ...                  stderr=${CURDIR}${/}tmp${/}MCP-List-Registry-stderr.txt
-    Should Not Contain             ${result.stdout}    failed to extract
     Should Contain                 ${result.stderr}    meaningless in local mode
 
 MCP HTTP Pull Provider Installs Known Provider

From 7a2b56307537396d75d39a6e5535d2ec93946fc1 Mon Sep 17 00:00:00 2001
From: Jeffrey Aven <javen@stackql.io>
Date: Sat, 30 May 2026 07:58:39 +1000
Subject: [PATCH 5/6] build fixes

---
 test/robot/functional/mcp.robot | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/test/robot/functional/mcp.robot b/test/robot/functional/mcp.robot
index 507f4b61..cf939a8f 100644
--- a/test/robot/functional/mcp.robot
+++ b/test/robot/functional/mcp.robot
@@ -957,8 +957,14 @@ MCP HTTP Pull Provider Installs Known Provider
     [Documentation]    Issue #661 feature: pull_provider installs a single
     ...                provider into the approot cache.  Allowed under every
     ...                mode (writes only local cache state per the issue's "not
-    ...                a cloud mutation" rationale).  The full_access 9922 server
-    ...                is used so the call goes through the gate cleanly.
+    ...                a cloud mutation" rationale).  The full_access 9922
+    ...                server is used so the call goes through the gate
+    ...                cleanly.  We pass an explicit version because the MCP
+    ...                servers in this suite use a file:// registry config,
+    ...                and any-sdk's latest-version lookup goes through
+    ...                listAllProviderVersions which deliberately refuses for
+    ...                local-mode registries.  Pulling a known version skips
+    ...                that lookup and reads the archive straight off disk.
     Pass Execution If    "%{IS_SKIP_MCP_TEST=false}" == "true"    Some platforms do not have the MCP client available
     Sleep         5s
     ${result}=    Run Process          ${STACKQL_MCP_CLIENT_EXE}
@@ -966,7 +972,7 @@ MCP HTTP Pull Provider Installs Known Provider
     ...                  \-\-client\-type\=http
     ...                  \-\-url\=http://127.0.0.1:9922
     ...                  \-\-exec.action      pull_provider
-    ...                  \-\-exec.args        {"provider":"google"}
+    ...                  \-\-exec.args        {"provider":"google","version":"v0.1.2"}
     ...                  stdout=${CURDIR}${/}tmp${/}MCP-Pull-Provider.txt
     ...                  stderr=${CURDIR}${/}tmp${/}MCP-Pull-Provider-stderr.txt
     Should Be Equal As Integers    ${result.rc}    0

From 8c0a1514d7c2284ad6b3473488a10b69b51d7805 Mon Sep 17 00:00:00 2001
From: Jeffrey Aven <javen@stackql.io>
Date: Sat, 30 May 2026 08:16:52 +1000
Subject: [PATCH 6/6] build fixes

---
 .../stackql/mcpbackend/mcp_service_stackql.go | 59 +++++++++----------
 test/robot/functional/mcp.robot               | 27 ++++-----
 2 files changed, 38 insertions(+), 48 deletions(-)

diff --git a/internal/stackql/mcpbackend/mcp_service_stackql.go b/internal/stackql/mcpbackend/mcp_service_stackql.go
index 4e86b300..352ec0d8 100644
--- a/internal/stackql/mcpbackend/mcp_service_stackql.go
+++ b/internal/stackql/mcpbackend/mcp_service_stackql.go
@@ -304,7 +304,11 @@ func (b *stackqlMCPService) RunQueryJSON(ctx context.Context, input dto.QueryJSO
 }
 
 func (b *stackqlMCPService) runPreprocessedQueryJSON(_ context.Context, query string, rowLimit int) ([]map[string]interface{}, error) {
-	return b.extractQueryResults(query, rowLimit)
+	results, ok := b.extractQueryResults(query, rowLimit)
+	if !ok {
+		return nil, fmt.Errorf("failed to extract query results")
+	}
+	return results, nil
 }
 
 // ExecQuery returns {messages, timestamp}: messages is the orchestrator's
@@ -326,13 +330,10 @@ func (b *stackqlMCPService) execQuery(query string) (map[string]any, error) {
 	rv := map[string]any{}
 	r, ok := b.applyQuery(query)
 	if !ok {
-		return rv, fmt.Errorf("query orchestrator rejected query")
+		return rv, fmt.Errorf("failed to extract query results")
 	}
 	messages := []string{}
 	for _, resp := range r {
-		if respErr := resp.GetError(); respErr != nil {
-			return rv, respErr
-		}
 		messages = append(messages, resp.GetMessages()...)
 	}
 	rv["messages"] = messages
@@ -355,23 +356,18 @@ func (b *stackqlMCPService) applyQuery(query string) ([]internaldto.ExecutorOutp
 	return r, ok
 }
 
-// extractQueryResults runs `query` and returns the row payload.  When the
-// orchestrator surfaces an error on any ExecutorOutput, that error is returned
-// verbatim so the MCP tool message carries the real cause (eg the any-sdk
-// "registry list is meaningless in local mode" refusal) rather than a generic
-// extraction-failed string.  An empty (zero-row) result is success, not
-// failure: rv is initialised as a non-nil empty slice so it survives
-// downstream JSON-array schema validation on QueryResultDTO.Rows.
-func (b *stackqlMCPService) extractQueryResults(query string, rowLimit int) ([]map[string]interface{}, error) {
+func (b *stackqlMCPService) extractQueryResults(query string, rowLimit int) ([]map[string]interface{}, bool) {
 	r, ok := b.applyQuery(query)
+	// Initialise as empty (not nil) so a zero-row result survives downstream
+	// JSON-array schema validation on QueryResultDTO.Rows.  This pairs with
+	// fix 1 (returning ok regardless of len(rv)) so empty results render as
+	// "**no results**" rather than failing extraction.
 	rv := []map[string]interface{}{}
-	if !ok {
-		return rv, fmt.Errorf("query orchestrator rejected query")
-	}
 	rowCount := 0
 	for _, resp := range r {
 		if respErr := resp.GetError(); respErr != nil {
-			return rv, respErr
+			ok = false
+			break
 		}
 		// PrepareResultSet emits a nil SQLResult when RowMap is empty (eg
 		// REGISTRY LIST against an empty registry).  That's a zero-row
@@ -380,42 +376,41 @@ func (b *stackqlMCPService) extractQueryResults(query string, rowLimit int) ([]m
 		if sqlRowStream == nil {
 			continue
 		}
-		var drainErr error
-		rv, rowCount, drainErr = drainSQLRowStream(sqlRowStream, rv, rowCount, rowLimit)
-		if drainErr != nil {
-			return rv, drainErr
+		var drainOK bool
+		rv, rowCount, drainOK = drainSQLRowStream(sqlRowStream, rv, rowCount, rowLimit)
+		if !drainOK {
+			ok = false
+			break
 		}
 	}
-	return rv, nil
+	return rv, ok
 }
 
 // drainSQLRowStream reads `stream` to EOF (or until rowLimit is reached),
-// appending each row's payload to `rv`.  Returns a non-nil error when the
-// stream surfaces a read error or a nil row outside of EOF.
+// appending each row's payload to `rv`.  The returned bool is false when the
+// stream surfaces a read error or a nil row outside of EOF; that maps onto
+// extractQueryResults' (rv, false) failure mode.
 func drainSQLRowStream(
 	stream sqldata.ISQLResultStream,
 	rv []map[string]interface{},
 	rowCount, rowLimit int,
-) ([]map[string]interface{}, int, error) {
+) ([]map[string]interface{}, int, bool) {
 	for {
 		row, err := stream.Read()
 		if err == io.EOF {
 			if row != nil {
 				rv = append(rv, row.ToArr()...)
 			}
-			return rv, rowCount, nil
-		}
-		if err != nil {
-			return rv, rowCount, err
+			return rv, rowCount, true
 		}
-		if row == nil {
-			return rv, rowCount, fmt.Errorf("sql row stream returned nil row without EOF")
+		if err != nil || row == nil {
+			return rv, rowCount, false
 		}
 		rowArr := row.ToArr()
 		rv = append(rv, rowArr...)
 		rowCount += len(rowArr)
 		if rowLimit > 0 && rowCount >= rowLimit {
-			return rv, rowCount, nil
+			return rv, rowCount, true
 		}
 	}
 }
diff --git a/test/robot/functional/mcp.robot b/test/robot/functional/mcp.robot
index cf939a8f..60b79ddd 100644
--- a/test/robot/functional/mcp.robot
+++ b/test/robot/functional/mcp.robot
@@ -935,12 +935,13 @@ MCP HTTP List Registry Returns Available Providers
     ...                providers).  The MCP servers in this suite use a
     ...                file:// registry config, against which any-sdk's
     ...                ListAllAvailableProviders deliberately refuses with
-    ...                "'registry list' is meaningless in local mode".
-    ...                extractQueryResults now passes ExecutorOutput.GetError()
-    ...                through verbatim (instead of swallowing it as the
-    ...                generic "failed to extract query results"), so we can
-    ...                pin the real cause here.  The pull_provider scenario
-    ...                below covers the happy path against the same registry.
+    ...                "'registry list' is meaningless in local mode".  That
+    ...                refusal surfaces here as an ExecutorOutput.GetError(),
+    ...                which extractQueryResults reports to the client as
+    ...                the generic "failed to extract query results" - we
+    ...                pin that substring in the mcp_client panic message.
+    ...                The pull_provider scenario below covers the happy path
+    ...                against the same registry.
     Pass Execution If    "%{IS_SKIP_MCP_TEST=false}" == "true"    Some platforms do not have the MCP client available
     Sleep         5s
     ${result}=    Run Process          ${STACKQL_MCP_CLIENT_EXE}
@@ -951,20 +952,14 @@ MCP HTTP List Registry Returns Available Providers
     ...                  \-\-exec.args        {}
     ...                  stdout=${CURDIR}${/}tmp${/}MCP-List-Registry.txt
     ...                  stderr=${CURDIR}${/}tmp${/}MCP-List-Registry-stderr.txt
-    Should Contain                 ${result.stderr}    meaningless in local mode
+    Should Contain                 ${result.stderr}    failed to extract query results
 
 MCP HTTP Pull Provider Installs Known Provider
     [Documentation]    Issue #661 feature: pull_provider installs a single
     ...                provider into the approot cache.  Allowed under every
     ...                mode (writes only local cache state per the issue's "not
-    ...                a cloud mutation" rationale).  The full_access 9922
-    ...                server is used so the call goes through the gate
-    ...                cleanly.  We pass an explicit version because the MCP
-    ...                servers in this suite use a file:// registry config,
-    ...                and any-sdk's latest-version lookup goes through
-    ...                listAllProviderVersions which deliberately refuses for
-    ...                local-mode registries.  Pulling a known version skips
-    ...                that lookup and reads the archive straight off disk.
+    ...                a cloud mutation" rationale).  The full_access 9922 server
+    ...                is used so the call goes through the gate cleanly.
     Pass Execution If    "%{IS_SKIP_MCP_TEST=false}" == "true"    Some platforms do not have the MCP client available
     Sleep         5s
     ${result}=    Run Process          ${STACKQL_MCP_CLIENT_EXE}
@@ -972,7 +967,7 @@ MCP HTTP Pull Provider Installs Known Provider
     ...                  \-\-client\-type\=http
     ...                  \-\-url\=http://127.0.0.1:9922
     ...                  \-\-exec.action      pull_provider
-    ...                  \-\-exec.args        {"provider":"google","version":"v0.1.2"}
+    ...                  \-\-exec.args        {"provider":"google"}
     ...                  stdout=${CURDIR}${/}tmp${/}MCP-Pull-Provider.txt
     ...                  stderr=${CURDIR}${/}tmp${/}MCP-Pull-Provider-stderr.txt
     Should Be Equal As Integers    ${result.rc}    0