Repository navigation
mcpx: shared 100-value completion cap on both daemon /v1/complete paths (#2878) - #3505
Conversation
…hs (#2878) The daemon applied no spec cap: local fallback truncated only to the raw completion.maxValues (250 returned 250; 0/unset was unlimited), and upstream replies passed through verbatim, while the MCP surface clamped to 100. Adds settings.SpecCompletionCap + settings.EffectiveCompletionCap (setting in [1,100] lowers, anything else resolves to 100) used by the daemon local path, a new capUpstreamCompletion on the passthrough path, and the mcpserver clamp, so the surfaces share one computation. Daemon regression tests cover the cap table, local settings 250/0/-1/100/50 over 150 candidates, and upstream truncation/passthrough.
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 3 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cadd8f5823
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| payload = capUpstreamCompletion(payload, | ||
| settings.EffectiveCompletionCap(cs.Int("completion.maxValues"))) |
There was a problem hiding this comment.
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 👍 / 👎.
There was a problem hiding this comment.
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.
… P2 on #3505) The daemon now caps /v1/complete replies, so mcpBackend.Complete forwarding only values made the MCP surface recompute total/hasMore from the already-capped slice (total=50, hasMore=false for a 150-match set capped at 50). The Backend.Complete contract now returns mcpserver.Completion{Values, Total, TotalKnown}: the CLI backend carries the daemon-reported total, and the MCP surface uses it when known (falling back to len(values) for direct answers), still applying its own cap to the values page. Regression test: a backend page of 50 with total 150 reports total=150/hasMore=true at caps 100 and 10.
Fixes #2878.
Problem
The daemon
/v1/completesurface never applied the MCP 100-value completion cap, while the MCP surface clamped the same setting (triage task 4061, CONFIRMED on main; register CMP-06):completion.maxValues— setting 250 returned up to 250 values; setting 0 (including the unset default, since the settingsInt()is a bareAtoi) disabled truncation entirely.Fix (pinned by triage)
One shared computation in
internal/settings:SpecCompletionCap = 100andEffectiveCompletionCap(n)— a setting in [1,100] lowers the cap; unset/zero/negative/over-cap resolves to 100.settings.EffectiveCompletionCap(cs.Int("completion.maxValues"))intolocalCompletion(total/hasMore still report the full ranked count).capUpstreamCompletion: truncates values to the cap, keeps the upstream total, recomputes hasMore; undecodable payloads pass through byte-identical.internal/mcpserverroutes its existing clamp through the same helper and drops its privatespecMaxCompletionsconstant, so the surfaces cannot drift again.Tests
internal/daemon/completion_cap_test.go: cap table (250/101/100/99/50/1/0/-5), local path over 150 candidates for settings 250/0/-1/100/50, upstream truncation to 100 with total 150/hasMore true, under-cap and undecodable payloads unchanged.go test ./internal/daemon/full package: ok.go test ./internal/mcpserver/: ok.internal/settings: 4 failures byte-identical to clean main (pre-existing: compendium/env/effect-probe registry tests), no new failures.Queue task 5456 (repo-health-code-pilot), Lane D.
Need help on this PR? Tag
@codesmith-botwith what you need.