feat(mcp)!: adopt stable SDK and typed recovery surface - #241
Conversation
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
![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: 20e4add906
ℹ️ 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".
| case "sync_threads": | ||
| var request mcpcontract.SyncThreadsInput | ||
| if json.Unmarshal([]byte(job.Request), &request) == nil { | ||
| return &mcpcontract.ToolCallArguments{Selection: request.Selection, Repositories: append([]mcpcontract.RepositoryRef(nil), request.Repositories...), Threads: append([]mcpcontract.ThreadRef(nil), request.Threads...), Kind: request.Kind, State: request.State} |
There was a problem hiding this comment.
For a repository-mode github.sync_threads job, this constructs arguments containing selection, repositories, kind, and state, but the advertised follow-up is corpus.get_threads, whose input requires a nonempty threads array. Following the newly typed terminal-job call therefore fails instead of reading the synchronized results; derive exact thread refs from the job result or route repository-mode jobs to a compatible read.
Useful? React with 👍 / 👎.
| case "index_repositories": | ||
| var request mcpcontract.IndexRepositoriesInput | ||
| if json.Unmarshal([]byte(job.Request), &request) == nil { | ||
| return &mcpcontract.ToolCallArguments{Repositories: indexRepositoriesToRefs(request.Repositories)} |
There was a problem hiding this comment.
Every completed index_repositories job advertises corpus.search_code, but these arguments only contain a repositories field; SearchCodeInput has no such field and requires a nonempty query. Replaying this typed follow-up is therefore rejected with query is required, so the follow-up must either include a meaningful search input or point to a read surface that accepts repository references.
Useful? React with 👍 / 👎.
| func workflowRetryCall(tool string, ref mcpcontract.ThreadRef) mcpcontract.ToolCall { | ||
| args := &mcpcontract.ToolCallArguments{PullRequests: []mcpcontract.ThreadRef{ref}, MaxRequests: 1000} | ||
| return mcpcontract.ToolCall{Tool: tool, Arguments: args} |
There was a problem hiding this comment.
When sync_pull_request_feedback returns a transient or request-budget error, workflowFailure uses this generic call, but github.sync_pull_request_feedback requires one to four channels. The emitted recovery contains only pull_requests and max_requests, so following it is rejected before a retry job can start; preserve the original feedback channels and related selection arguments.
Useful? React with 👍 / 👎.
| item.Status = "retryable" | ||
| item.Code = "ci_coverage_incomplete" | ||
| item.Message = "one or more CI collections reached a configured item bound" | ||
| item.NextAction = "Retry only this pull request with larger run or job bounds." | ||
| item.Recovery = recoveryPlan("facet_incomplete", item.Message, mcpcontract.ToolCall{Tool: mcpcontract.ToolSyncCIFailures, Arguments: &mcpcontract.ToolCallArguments{PullRequests: []mcpcontract.ThreadRef{ref}, Logs: in.Logs, MaxRunsPerPR: in.MaxRunsPerPR, MaxJobsPerRun: in.MaxJobsPerRun, MaxLogBytesPerJob: in.MaxLogBytesPerJob, MaxRequests: in.MaxRequests}}) |
There was a problem hiding this comment.
When CI coverage is incomplete because a run or job collection reached its configured limit, this recovery call repeats the exact same max_runs_per_pr and max_jobs_per_run values. The retried request therefore reaches the same boundary and remains incomplete; raise the bound that was exhausted, subject to the accepted maximum, rather than replaying unchanged limits.
Useful? React with 👍 / 👎.
| item.Status = "retryable" | ||
| item.Code = "feedback_coverage_incomplete" | ||
| item.Message = "one or more feedback channels reached max_items_per_channel" | ||
| item.NextAction = "Retry only this pull request with a larger max_items_per_channel bound." | ||
| item.Recovery = recoveryPlan("facet_incomplete", item.Message, mcpcontract.ToolCall{Tool: mcpcontract.ToolSyncPullRequestFeedback, Arguments: &mcpcontract.ToolCallArguments{PullRequests: []mcpcontract.ThreadRef{ref}, Channels: append([]string(nil), in.Channels...), ThreadState: in.ThreadState, MaxItemsPerChannel: in.MaxItemsPerChannel * 2, MaxRequests: in.MaxRequests}}) |
There was a problem hiding this comment.
If feedback is incomplete with max_items_per_channel above 500, doubling it produces a recovery value above the tool's maximum of 1000. For example, an accepted request with 600 yields a typed retry with 1200 that validation immediately rejects; cap or otherwise calculate the next bound within the advertised range.
Useful? React with 👍 / 👎.
| if len(prIDs) == 0 { | ||
| for _, index := range candidateIndexes { | ||
| out.Items[index] = mcpcontract.BatchItem[mcpcontract.PortfolioOverlapOutput]{Key: in.Candidates[index].Kind + ":" + in.Candidates[index].Ref, Status: "unavailable", Reason: "pull_requests_not_stored", Message: "none of the requested pull requests are available in the local corpus", NextAction: "Sync the exact authored pull requests, then retry this comparison."} | ||
| out.Items[index] = mcpcontract.BatchItem[mcpcontract.PortfolioOverlapOutput]{Key: in.Candidates[index].Kind + ":" + in.Candidates[index].Ref, Status: "unavailable", Reason: "thread_not_indexed", Message: "none of the requested pull requests are available in the local corpus", Recovery: recoveryPlan("thread_not_indexed", "Sync the exact authored pull requests, then retry this comparison.", syncPullRequestCalls(pullRequests)...)} |
There was a problem hiding this comment.
When none of the requested pull requests is stored, this recovery invokes github.sync_pull_request_portfolio directly. That operation starts with HydrateThread, which requires the repository and exact thread header to already exist locally, so it cannot repair the reported thread_not_indexed state and the subsequent comparison remains unavailable; recover with repository/header synchronization before running the portfolio refresh.
Useful? React with 👍 / 👎.
| currentKind := kind | ||
| if in.Selection == "threads" { | ||
| currentKind = current.kind | ||
| } | ||
| opts := SyncOptions{Kind: currentKind, State: state, Since: since, Numbers: current.numbers, MaxItems: in.LimitPerRepository, MaxPages: maxPages, MaxRequests: current.maxRequests} |
There was a problem hiding this comment.
For exact-thread mode, a caller can request kind: issue for a number that GitHub identifies as a pull request. Passing the requested kind here causes syncThreadWriter.store to discard the fetched object, but the task still reports complete with zero updates, producing a false successful synchronization and an unavailable follow-up read; detect that no fetched thread matched the requested kind and return an unavailable or failed item.
Useful? React with 👍 / 👎.
| if !facet.Complete { | ||
| result["status"] = "retryable" | ||
| result["next_action"] = "Retry this pull request with a larger max_pages bound." | ||
| result["recovery"] = recoveryPlan("facet_incomplete", "Retry this pull request with a larger max_pages bound.", syncPullRequestCalls([]mcpcontract.ThreadRef{ref})...) |
There was a problem hiding this comment.
When hydrated PR details or reviews are incomplete because maxPages was reached, this message promises a larger bound but syncPullRequestCalls supplies no status_max_pages, so the recovery uses the default of 3. A default-capped request is repeated unchanged, while an original request above 3 is retried with an even smaller bound; include an increased status_max_pages in the typed call.
Useful? React with 👍 / 👎.
| storedRepo, err := c.GetRepository(ctx, owner, repo) | ||
| if err != nil || storedRepo == nil { | ||
| if err == nil { | ||
| err = errors.New("repository is not stored") | ||
| } | ||
| return nil, err |
There was a problem hiding this comment.
When a facet URI names a repository or thread that is no longer present, these plain errors are not recognized by mcpserver.isNotFound, so resources/read returns a generic server error instead of the MCP resource-not-found response used by the other thread resources. Return the repository's typed not-found failure for both missing objects so stale or manually instantiated resource URIs are handled correctly.
Useful? React with 👍 / 👎.
| thread := threads[corpus.ThreadKey{RepositoryID: repo.ID, Kind: input.Kind, Number: input.Number}] | ||
| if thread == nil { | ||
| item.Status, item.Reason, item.Message = "unavailable", "not_indexed", "thread is not present in the local corpus" | ||
| item.NextAction = "Call github.sync_threads in thread selection mode with this exact reference." | ||
| item.Status, item.Reason, item.Message = "unavailable", "thread_not_indexed", "thread is not present in the local corpus" | ||
| item.Recovery = recoveryPlan(item.Reason, item.Message, syncThreadCall(input)) |
There was a problem hiding this comment.
corpus.get_threads explicitly permits a thread reference with an omitted kind, but when that read reports the thread unavailable this recovery forwards the same empty kind to github.sync_threads. The sync handler requires every exact thread to be either issue or pull_request, so the typed recovery is rejected instead of acquiring the object; resolve the kind before emitting the call or require it consistently on the original read.
Useful? React with 👍 / 👎.
1 participant
Description
This PR upgrades the MCP Go SDK from
v1.7.0-pre.3to stablev1.7.0and makes the typed MCP surface the only supported contract.The server now uses versioned
gitcontribute.recovery.v1plans with ordered typed tool calls, adds bounded offline facet reads throughcorpus.get_thread_facetsand facet resources, and preserves issue/pull-request kind through synchronization, hydration, jobs, and retries.Change
v1.7.0for typed tool registration, schema validation, annotations, resource templates, resource reads, and protocol negotiation.next_action/suggested_actionsfields withRecoveryPlanand typedToolCallarguments.corpus.get_thread_facetsand canonical facet resource URIs for large persisted payloads.Review order: contracts and module upgrade, application recovery/facet/kind handling, MCP registration/resource routing, then regression tests and documentation.
Testing
make verify— uncached tests, golangci-lint, module-tidiness, generated-output, and AGENTS validation passed.go test ./...— passed.go mod verify— all modules verified.git diff --check— passed.Compatibility and scope