Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 19 additions & 10 deletions pkgs/mcpx/internal/cli/serve.go
Original file line number Diff line number Diff line change
Expand Up @@ -1081,7 +1081,7 @@ func upstreamInvalid(err error) bool {
// Complete forwards completion/complete to the server that owns the ref,
// through /v1/complete, so the two surfaces give the same answer to the same
// question (conflict #9).
func (b mcpBackend) Complete(ctx context.Context, params json.RawMessage) ([]string, error) {
func (b mcpBackend) Complete(ctx context.Context, params json.RawMessage) (mcpserver.Completion, error) {
var p struct {
Ref struct {
Type string `json:"type"`
Expand All @@ -1094,34 +1094,34 @@ func (b mcpBackend) Complete(ctx context.Context, params json.RawMessage) ([]str
} `json:"context"`
}
if err := json.Unmarshal(params, &p); err != nil {
return nil, fmt.Errorf("%w: %v", mcpserver.ErrInvalidParams, err)
return mcpserver.Completion{}, fmt.Errorf("%w: %v", mcpserver.ErrInvalidParams, err)
}
c, err := b.app.ensure(ctx)
if err != nil {
return nil, err
return mcpserver.Completion{}, err
}
ref := map[string]any{"type": p.Ref.Type}
var server string
switch p.Ref.Type {
case "ref/prompt":
ns, inner, err := b.app.resolvePrompt(ctx, c, p.Ref.Name)
if err != nil {
return nil, err
return mcpserver.Completion{}, err
}
server, ref["name"] = ns, inner
case "ref/resource":
if _, ok := artifacts.IDFromURI(p.Ref.URI); ok || p.Ref.URI == artifactTemplate {
// mcpx's own template. Its ids are unguessable by design, so
// there is nothing to offer.
return nil, nil
return mcpserver.Completion{}, nil
}
ns, inner, ok := b.app.resolveURI(ctx, p.Ref.URI)
visible, err := b.app.visibleNamespaces(ctx, c)
if err != nil {
return nil, err
return mcpserver.Completion{}, err
}
if !ok || !visible(ns) {
return nil, fmt.Errorf("%w: no resource template %q", mcpserver.ErrInvalidParams, p.Ref.URI)
return mcpserver.Completion{}, fmt.Errorf("%w: no resource template %q", mcpserver.ErrInvalidParams, p.Ref.URI)
}
server, ref["uri"] = ns, inner
}
Expand All @@ -1135,17 +1135,26 @@ func (b mcpBackend) Complete(ctx context.Context, params json.RawMessage) ([]str
}
raw, err := c.do(ctx, http.MethodPost, "/v1/complete", body)
if err != nil {
return nil, err
return mcpserver.Completion{}, err
}
// The daemon's reply carries total alongside values; keep it, or
// the MCP surface recomputes the count from values the daemon may
// already have capped (#2878).
var out struct {
Completion struct {
Values []string `json:"values"`
Total *int `json:"total"`
} `json:"completion"`
}
if err := json.Unmarshal(raw, &out); err != nil {
return nil, err
return mcpserver.Completion{}, err
}
res := mcpserver.Completion{Values: out.Completion.Values}
if out.Completion.Total != nil {
res.Total = *out.Completion.Total
res.TotalKnown = true
}
return out.Completion.Values, nil
return res, nil
}

// visibleNamespaces is the profile applied as a boundary, not only a view.
Expand Down
4 changes: 2 additions & 2 deletions pkgs/mcpx/internal/cli/serve_ask_raw_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,8 @@ func (b *rawAskBackend) ReadResource(context.Context, string) ([]mcpserver.Resou
func (b *rawAskBackend) GetPrompt(context.Context, string, map[string]string) (string, error) {
return "", nil
}
func (b *rawAskBackend) Complete(context.Context, json.RawMessage) ([]string, error) {
return nil, nil
func (b *rawAskBackend) Complete(context.Context, json.RawMessage) (mcpserver.Completion, error) {
return mcpserver.Completion{}, nil
}
func (b *rawAskBackend) ResourceTemplates(context.Context) ([]mcpserver.ResourceRef, error) {
return nil, nil
Expand Down
94 changes: 94 additions & 0 deletions pkgs/mcpx/internal/daemon/completion_cap_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
package daemon

import (
"encoding/json"
"fmt"
"testing"

"github.com/developing-today/code/pkgs/mcpx/internal/settings"
)

// #2878: the daemon /v1/complete surface never applied the 100-value
// spec cap. The shared computation lives in internal/settings; these
// tests pin the daemon's use of it on both reply paths.

func TestEffectiveCompletionCap(t *testing.T) {
for raw, want := range map[int]int{
250: 100, 101: 100, 100: 100, 99: 99, 50: 50, 1: 1,
0: 100, -5: 100,
} {
if got := settings.EffectiveCompletionCap(raw); got != want {
t.Errorf("EffectiveCompletionCap(%d) = %d, want %d", raw, got, want)
}
}
if settings.SpecCompletionCap != 100 {
t.Errorf("SpecCompletionCap = %d, want 100", settings.SpecCompletionCap)
}
}

func manyPrompts(n int) []string {
names := make([]string, n)
for i := range names {
names[i] = fmt.Sprintf("p%03d", i)
}
return names
}

// The local fallback, fed the cap exactly as handleComplete computes
// it: no setting value can push a reply past 100, 0/unset means the
// default 100 (not unlimited), and a small setting still lowers it.
func TestLocalCompletionSharedCap(t *testing.T) {
req := completeReq{}
req.Ref.Type = "ref/prompt"
for raw, want := range map[int]int{250: 100, 0: 100, -1: 100, 100: 100, 50: 50} {
got := localCompletion(promptsOf(manyPrompts(150)...), nil, req,
settings.EffectiveCompletionCap(raw))
vals := completeValues(t, got)
if len(vals) != want {
t.Errorf("setting %d: %d values, want %d", raw, len(vals), want)
}
if got["total"] != 150 {
t.Errorf("setting %d: total = %v, want 150 (full ranked count)", raw, got["total"])
}
if got["hasMore"] != true {
t.Errorf("setting %d: hasMore = %v, want true", raw, got["hasMore"])
}
}
}

// The upstream passthrough: an over-cap reply is truncated to the
// shared cap with a consistent total/hasMore; anything the cap cannot
// read passes through byte-identical.
func TestCapUpstreamCompletion(t *testing.T) {
values := make([]string, 150)
for i := range values {
values[i] = fmt.Sprintf("v%03d", i)
}
payload, _ := json.Marshal(map[string]any{
"values": values, "total": 150, "hasMore": false,
})
var out struct {
Values []string `json:"values"`
Total int `json:"total"`
HasMore bool `json:"hasMore"`
}
if err := json.Unmarshal(capUpstreamCompletion(payload, 100), &out); err != nil {
t.Fatal(err)
}
if len(out.Values) != 100 || out.Total != 150 || !out.HasMore {
t.Errorf("capped = %d values, total %d, hasMore %v; want 100/150/true",
len(out.Values), out.Total, out.HasMore)
}

small, _ := json.Marshal(map[string]any{
"values": []string{"a", "b"}, "total": 2, "hasMore": false,
})
if got := capUpstreamCompletion(small, 100); string(got) != string(small) {
t.Errorf("under-cap payload changed: %s", got)
}
for _, raw := range []string{`{not json`, `{"foo":1}`, `{"values":"nope"}`} {
if got := capUpstreamCompletion(json.RawMessage(raw), 100); string(got) != raw {
t.Errorf("undecodable payload %q changed to %q", raw, got)
}
}
}
33 changes: 32 additions & 1 deletion pkgs/mcpx/internal/daemon/ops.go
Original file line number Diff line number Diff line change
Expand Up @@ -383,13 +383,15 @@ func (s *Server) handleComplete(w http.ResponseWriter, r *http.Request) {
if json.Unmarshal(raw, &res) == nil && len(res.Completion) > 0 {
payload = res.Completion
}
payload = capUpstreamCompletion(payload,
settings.EffectiveCompletionCap(cs.Int("completion.maxValues")))
Comment on lines +386 to +387

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve completion metadata through the MCP backend

When an upstream returns more values than the configured cap (for example, 100 matches with completion.maxValues=50), truncating here causes the MCP surface to incorrectly report total: 50 and hasMore: false. In internal/cli/serve.go, mcpBackend.Complete extracts only completion.values, discarding the daemon's preserved total and hasMore; internal/mcpserver/server.go then recomputes both from that already-truncated slice. Previously, the upstream values reached the MCP server before truncation, so it correctly reported total: 100 and hasMore: true. Preserve the metadata through the backend interface so MCP clients can still detect additional matches.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 6cba232 — thank you, the regression was real.

The Backend.Complete contract now returns mcpserver.Completion{Values, Total, TotalKnown} instead of just []string. mcpBackend.Complete (internal/cli/serve.go) carries the daemon reply's total through, and the MCP surface (internal/mcpserver/server.go complete()) uses the backend's total when known — still applying its own cap to the values page — falling back to len(values) only for backends that report no count.

So the cited case now reports correctly end to end: upstream 100 matches with completion.maxValues=50 → daemon returns 50 values with total: 100, hasMore: true → MCP surface reports total: 100, hasMore: true (and with a lower MCP-side cap of 10: 10 values, total: 100, hasMore: true).

Regression coverage: TestCompletionPreservesTheBackendsTotal (internal/mcpserver/wp11_test.go) drives a backend page of 50 values with total 150 through the full completion/complete handler at the default cap and at cap 10. Full internal/mcpserver, internal/cli, and internal/daemon test packages pass on the laptop.

writeJSON(w, http.StatusOK, map[string]any{
"completion": payload, "upstream": true, "source": "upstream"})
return
}
writeJSON(w, http.StatusOK, map[string]any{
"completion": localCompletion(p.CachedPrompts(), p.CachedTemplates(), req,
cs.Int("completion.maxValues")),
settings.EffectiveCompletionCap(cs.Int("completion.maxValues"))),
// Said plainly, because a client that cannot tell an empty answer
// from an unimplemented one shows nothing and the user concludes
// completion is broken. `upstream` stays for the callers that read
Expand Down Expand Up @@ -431,6 +433,35 @@ func localCompletion(prompts []mcpclient.Prompt, templates []mcpclient.Resource,
return map[string]any{"values": values, "total": total, "hasMore": total > len(values)}
}

// capUpstreamCompletion applies the shared completion cap to an
// upstream CompleteResult payload (#2878): values are truncated to
// limit, total keeps the upstream full count, and hasMore is
// recomputed from the two. A payload that does not decode as a
// completion object with a values array passes through unchanged —
// capping must never corrupt a reply it cannot read.
func capUpstreamCompletion(payload json.RawMessage, limit int) json.RawMessage {
var obj map[string]any
if json.Unmarshal(payload, &obj) != nil {
return payload
}
rawValues, ok := obj["values"].([]any)
if !ok || len(rawValues) <= limit {
return payload
}
total := len(rawValues)
if t, ok := obj["total"].(float64); ok && int(t) > total {
total = int(t)
}
obj["values"] = rawValues[:limit]
obj["total"] = total
obj["hasMore"] = total > limit
out, err := json.Marshal(obj)
if err != nil {
return payload
}
return out
}

// rankCompletions orders the candidates that match value by relevance
// (issue #2388, Phase 2 of design #2386): tier 1 is an exact
// case-folded match, tier 2 a prefix match (shorter names first, then
Expand Down
38 changes: 29 additions & 9 deletions pkgs/mcpx/internal/mcpserver/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import (
"github.com/developing-today/code/pkgs/mcpx/internal/catalog"
"github.com/developing-today/code/pkgs/mcpx/internal/codegen"
"github.com/developing-today/code/pkgs/mcpx/internal/defaults"
"github.com/developing-today/code/pkgs/mcpx/internal/settings"
"github.com/developing-today/code/pkgs/mcpx/internal/spec"
)

Expand Down Expand Up @@ -63,10 +64,24 @@ type Backend interface {
// Complete answers completion/complete for a ref/prompt or ref/resource
// by asking the upstream server that owns it. params is the request as
// the client sent it. An unknown ref wraps ErrInvalidParams.
Complete(ctx context.Context, params json.RawMessage) ([]string, error)
Complete(ctx context.Context, params json.RawMessage) (Completion, error)
ResourceTemplates(ctx context.Context) ([]ResourceRef, error)
}

// Completion is a completion/complete answer: the values, plus the
// full ranked match count they were drawn from when the backend knows
// it. The daemon caps its /v1/complete reply before the MCP surface
// sees it (#2878), so a backend that only handed over values would
// make this surface report total = len(values) and hasMore = false
// for a reply the daemon had already truncated from a larger set.
// TotalKnown is false when the backend has no count (a direct
// upstream answer): the caller then uses len(values) as before.
type Completion struct {
Values []string
Total int
TotalKnown bool
}

// Notifier is where the server hears about things worth pushing.
//
// Defined as an interface so the protocol package does not depend on the
Expand Down Expand Up @@ -2756,6 +2771,7 @@ func (s *Server) complete(ctx context.Context, params json.RawMessage) (map[stri
Message: `ref.type is "ref/prompt" or "ref/resource", got "` + p.Ref.Type + `"`}
}
var values []string
total := -1 // the backend's full match count, when it reported one
if s.backend != nil {
got, err := s.backend.Complete(ctx, params)
if err != nil {
Expand All @@ -2765,7 +2781,10 @@ func (s *Server) complete(ctx context.Context, params json.RawMessage) (map[stri
}
return nil, &rpcError{Code: code, Message: err.Error()}
}
values = got
values = got.Values
if got.TotalKnown {
total = got.Total
}
}
// The ceiling the specification sets. Read per request, because
// completion.maxValues is a hot setting: a value read once when the
Expand All @@ -2777,11 +2796,12 @@ func (s *Server) complete(ctx context.Context, params json.RawMessage) (map[stri
}
}
// The setting may lower the count, not raise it past what every
// revision's CompleteResult allows.
if max > specMaxCompletions {
max = specMaxCompletions
// revision's CompleteResult allows. The cap computation is shared
// with the daemon surface via settings (#2878).
max = settings.EffectiveCompletionCap(max)
if total < 0 {
total = len(values)
}
total := len(values)
if len(values) > max {
values = values[:max]
}
Expand All @@ -2806,9 +2826,9 @@ func (l lockedEncoder) Encode(v any) error {
return l.enc.Encode(v)
}

// specMaxCompletions is CompleteResult.values' maxItems in every revision: a
// fact of the protocol, not a default.
const specMaxCompletions = 100
// The completion spec cap lives in internal/settings
// (settings.SpecCompletionCap) so the daemon and MCP surfaces share
// one constant and one cap computation (#2878).

// hasTool reports whether tools/call can reach name.
func (s *Server) hasTool(ctx context.Context, name string) (bool, error) {
Expand Down
16 changes: 12 additions & 4 deletions pkgs/mcpx/internal/mcpserver/server_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ func (f *fakeBackend) GetPrompt(_ context.Context, name string, _ map[string]str

// Complete stands in for the upstream server that owns a ref: values only it
// could know, so a test can tell who answered.
func (f *fakeBackend) Complete(_ context.Context, params json.RawMessage) ([]string, error) {
func (f *fakeBackend) Complete(_ context.Context, params json.RawMessage) (mcpserver.Completion, error) {
f.hit("complete")
var p struct {
Ref struct {
Expand All @@ -111,11 +111,19 @@ func (f *fakeBackend) Complete(_ context.Context, params json.RawMessage) ([]str
_ = json.Unmarshal(params, &p)
switch p.Ref.Name {
case "nosuch":
return nil, fmt.Errorf("%w: no prompt named %q", mcpserver.ErrInvalidParams, p.Ref.Name)
return mcpserver.Completion{}, fmt.Errorf("%w: no prompt named %q", mcpserver.ErrInvalidParams, p.Ref.Name)
case "broken":
return nil, errors.New("upstream timed out")
return mcpserver.Completion{}, errors.New("upstream timed out")
case "capped":
// Stands in for the daemon's already-capped /v1/complete reply
// (#2878): fewer values than the full match count, total known.
values := make([]string, 50)
for i := range values {
values[i] = fmt.Sprintf("capped-%02d", i)
}
return mcpserver.Completion{Values: values, Total: 150, TotalKnown: true}, nil
}
return []string{"from-upstream-1", "from-upstream-2", "from-upstream-3"}, nil
return mcpserver.Completion{Values: []string{"from-upstream-1", "from-upstream-2", "from-upstream-3"}}, nil
}

func (f *fakeBackend) ResourceTemplates(context.Context) ([]mcpserver.ResourceRef, error) {
Expand Down
28 changes: 28 additions & 0 deletions pkgs/mcpx/internal/mcpserver/wp11_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -281,6 +281,34 @@ func TestCompletionIsForwardedToTheOwningServer(t *testing.T) {
})
}

// #2878: when the backend's answer was already capped by the daemon,
// the full match count it reported must survive to the client —
// recomputing total from the capped values would claim total=50,
// hasMore=false for a set of 150.
func TestCompletionPreservesTheBackendsTotal(t *testing.T) {
complete := func(s *mcpserver.Server) string {
b, _ := json.Marshal(s.Handle(context.Background(), mcpserver.Request(1, "completion/complete",
map[string]any{"ref": map[string]any{"type": "ref/prompt", "name": "capped"},
"argument": map[string]any{"name": "text", "value": ""}})))
return string(b)
}
b := complete(mcpserver.New(newBackend(), "mcpx", "test"))
if !strings.Contains(b, `"total":150`) || !strings.Contains(b, `"hasMore":true`) {
t.Fatalf("backend total not preserved: %s", b)
}
if !strings.Contains(b, `"capped-49"`) || strings.Contains(b, `"capped-50"`) {
t.Fatalf("values are not the backend's capped page: %s", b)
}
// A lower MCP-side cap truncates the page further but never the total.
s := mcpserver.New(newBackend(), "mcpx", "test")
s.MaxCompletions = func() int { return 10 }
b = complete(s)
if !strings.Contains(b, `"total":150`) || !strings.Contains(b, `"hasMore":true`) ||
!strings.Contains(b, `"capped-09"`) || strings.Contains(b, `"capped-10"`) {
t.Fatalf("lower cap lost the total: %s", b)
}
}

// Conflict #12: prompts/get answered every failure with -32602.
func TestPromptErrorsAreClassified(t *testing.T) {
s := mcpserver.New(newBackend(), "mcpx", "test")
Expand Down
Loading
Loading