fix(mcp): provide typed recovery for incomplete reads - #254
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: d3a8e9dcb0
ℹ️ 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".
| if snapshot.ClosedTruncated { | ||
| item.Status = "retryable" | ||
| item.Reason = "history_incomplete" | ||
| item.Message = "stored closed issue and pull-request history is incomplete; synchronize coverage before treating these precedents as exhaustive" | ||
| item.Recovery = precedentRecoveryPlan(input, item.Reason) |
There was a problem hiding this comment.
For a fully indexed repository with more than 2,000 closed threads, ClosedTruncated is set because FindPrecedents calls LoadPrecedentRepositories(..., 2000) and the loader compares that bounded slice with the stored COUNT(*); it does not indicate missing GitHub coverage. The returned ensure_coverage action fetches at most 1,000 recent headers and cannot remove the 2,000-candidate read cap, so polling and retrying continues to return history_incomplete and may repeatedly spend GitHub requests. Report this as a query-cap limitation or provide pagination/a larger bound instead of prescribing coverage synchronization.
AGENTS.md reference: AGENTS.md:L31-L33
Useful? React with 👍 / 👎.
| if !ok { | ||
| item.Status, item.Reason = "unavailable", "thread_not_indexed" | ||
| item.Message = "source thread is not present in the local corpus" | ||
| item.Recovery = precedentRecoveryPlan(input, item.Reason) |
There was a problem hiding this comment.
When the repository exists but the requested source thread is absent and is not among its 1,000 most recently updated headers, this repository-target recovery cannot resolve thread_not_indexed: repository-mode ensure_coverage lists headers in descending update order with LimitPerRepository: 1000 and never requests the supplied thread number. After the job, the source remains missing and the retry returns the same recovery plan; use an exact-thread synchronization action for this reason, with any repository-history refresh as a separate step.
AGENTS.md reference: AGENTS.md:L31-L33
Useful? React with 👍 / 👎.
| item.Value = &mcpcontract.PrecedentSet{Matches: precedents, Population: snapshot.ClosedTotal, Considered: len(snapshot.Closed), Truncated: snapshot.ClosedTruncated || len(precedents) < qualifying} | ||
| if item.Value.Truncated { | ||
| out.Status = "partial" | ||
| if snapshot.ClosedTruncated { |
There was a problem hiding this comment.
When a repository sync stops before exhausting GitHub but leaves at most 2,000 closed rows locally, ClosedTruncated is false because it compares only the bounded read with the local row count. The repository's threads coverage facet remains incomplete, but this branch never consults it, so find_precedents returns the item as complete without the newly promised recovery even though most history may be absent. Determine historical completeness from the recorded repository coverage and return ensure_coverage when that facet is missing or incomplete.
AGENTS.md reference: AGENTS.md:L5-L6
Useful? React with 👍 / 👎.
| Target: mcpcontract.CoverageTarget{ | ||
| Type: mcpcontract.CoverageTargetRepository, | ||
| Repository: mcpcontract.RepositoryRef{Owner: ref.Owner, Repo: ref.Repo}, | ||
| }, |
There was a problem hiding this comment.
When a source has a blank or syntactically invalid owner/repository, or a nonpositive number, neither the MCP handler nor FindPrecedents validates it before these new plans are built. An invalid owner/repository produces an ensure_coverage action that is immediately rejected by validateEnsureCoverageTarget, while a nonpositive number with an existing repository can prompt a repository-wide GitHub sync that can never create the requested source. Validate each source first and report failed/invalid_reference without a recovery action so every advertised recovery remains replayable and cannot authorize futile external reads.
AGENTS.md reference: AGENTS.md:L31-L33
Useful? React with 👍 / 👎.
morluto
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: afc513bff0
ℹ️ 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".
| status = "partial" | ||
| } | ||
| return mcpcontract.CheckOutput{ | ||
| Status: status, Coverage: "complete", Truncated: truncated, Recovery: recovery, |
There was a problem hiding this comment.
When a repository has only metadata indexed—including immediately after following the repository_not_indexed recovery—its threads facet can still be missing or incomplete, but both duplicate and collision conversions report Coverage: "complete" and may present an empty result as exhaustive. Consult the repository's stored threads coverage and return an ensure_coverage or thread-sync recovery until that population is complete.
AGENTS.md reference: AGENTS.md:L5-L6
Useful? React with 👍 / 👎.
| if provenance.UnknownCoverage { | ||
| out.Recovery = localThreadSearchRecovery(in) | ||
| out.Provenance.Recovery = out.Recovery |
There was a problem hiding this comment.
For every repository-scoped thread search, offlineReadProvenance is constructed with unknownCoverage=true, so this new branch always returns ensure_coverage, even when the repository's threads facet is already complete. Repeating the search after the job therefore returns the same recovery forever and can cause agents to repeatedly spend GitHub requests; derive unknown coverage from the stored repository facet instead of reacting unconditionally.
AGENTS.md reference: AGENTS.md:L5-L6
Useful? React with 👍 / 👎.
| next.PerFileBytes = min(1024*1024, max(in.PerFileBytes*2, in.PerFileBytes+1)) | ||
| next.TotalBytes = min(4*1024*1024, max(in.TotalBytes*2, in.TotalBytes+1)) | ||
| return recoveryPlan("source_file_too_large", "The selected file exceeded the current byte bound. Retry this exact file with the returned larger bounds or narrow its line range.", mcpcontract.RecoveryAction(next)) |
There was a problem hiding this comment.
When read_source_files reports too_large because one file exceeds the 1 MiB per-file maximum and the request already uses the maximum bounds, both min expressions preserve the existing values and the typed recovery replays the identical file and range. That request is guaranteed to return too_large again, so omit the replay at the cap or generate a genuinely narrower line-range action.
Useful? React with 👍 / 👎.
| if result.Incomplete { | ||
| retry := in | ||
| retry.Limit = min(100, max(in.Limit*2, in.Limit+1)) | ||
| out.RecoveryPlans = append(out.RecoveryPlans, *recoveryPlan("github_search_incomplete", "GitHub marked this search page incomplete. Replay the exact search with a larger page bound, then narrow the query if it remains incomplete.", mcpcontract.RecoveryAction(retry))) |
There was a problem hiding this comment.
When GitHub marks a page after the first as incomplete, increasing Limit while retaining Page changes which results the request addresses: for example, retrying page 2 at 50 items as page 2 at 100 items moves the offset from 50 to 100 and skips the original page's first 50 positions. Replay the same page size or otherwise preserve the original offset, and cover this ordering case with a focused pagination regression test.
AGENTS.md reference: AGENTS.md:L32-L33
Useful? React with 👍 / 👎.
1 participant
Follow-up to #253.
Problem
Several bounded MCP reads exposed
unknown_coverage, truncation, partial, missing, or retryable outcomes without a model-visible, typed next action. An agent could therefore infer absence from incomplete local history, unindexed code, absent repositories, incomplete portfolio or ranking projections, or bounded acquisition results.Change
RecoveryPlanactions for local thread, repository, and code search; related-work checks; code-index gaps and pagination; portfolio reads and jobs; ranking and cluster bounds; fix-pattern reports; merge conflicts; source-file retries; live thread pagination; and concern pagination.ToolCallunion with the corresponding callable operations and document exact recovery semantics in the catalog.Regression coverage
Tests cover unknown local coverage, absent related-work repositories, unscoped code-search absence, nested repository projection gaps, code pagination, portfolio and job recovery, ranking bounds, fix-pattern partial results, live search pagination, concern offsets, and the refactored search handlers.
Validation
make verify— passed (tests, golangci-lint, module tidy, generated-output check, AGENTS validation)go test ./...— passed previously on the same behavior tree; focused app/server/contract tests passed after the file splitReview order
internal/mcpcontractrecovery contracts andToolCallunion