Fix typed Context Builder prompt handoff (#526)#529
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a Context Builder MCP provider wiring gap where typed responses (plan/question/review) could fail when the target tab’s committed prompt was empty, by resolving a request-local “typed prompt” that prefers an existing committed prompt and otherwise falls back to sanitized caller instructions (while keeping discovery-only guidance out of the follow-up prompt and preserving canonical tab state).
Changes:
- Add a typed prompt resolver that (a) preserves non-empty committed prompts and (b) otherwise sanitizes caller
instructionsby removing only exact lowercase<discovery_agent-guidelines>…</discovery_agent-guidelines>blocks, failing closed on malformed/variant markup. - Thread the resolved prompt through follow-up generation and the returned MCP result payload (without rewriting tab prompt state).
- Add regression coverage for provider-path handoff plus deterministic unit coverage for resolver precedence/grammar; update the curated test-suite contract ledger.
Maintainer-guidance check
- User impact and invariant: Typed MCP Context Builder runs should succeed even if the tab prompt is empty, and typed generation must not use discovery output or discovery-only guidance as the effective prompt; tab prompt state must remain the canonical committed state.
- Root-cause confidence: confirmed (matches the described wiring gap between discovery-only
instructionsand typed follow-up prompt source). - Authority: MCPContextBuilderToolProvider controls the prompt used for typed generation; tab prompt state remains owned by the workspace/tab store.
- State-safety risks: low; the change explicitly avoids mutating tab state and only affects request-local prompt resolution.
- Scale and observability risks: low; the sanitizer is linear over the provided instructions and only runs when the committed prompt is unusable for typed generation.
- Recommended scope: now (small, coherent fix with targeted tests at the owning boundary).
- Validation boundary: provider-path integration test plus resolver/disposition unit coverage (as added in this PR).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
Sources/RepoPrompt/Infrastructure/MCP/WindowTools/MCPContextBuilderToolProvider.swift |
Introduces request-local typed prompt resolution/sanitization and threads resolved prompt through typed follow-up generation and tool result packaging. |
Tests/RepoPromptTests/ContextBuilder/ContextBuilderMCPProgressTimelineTests.swift |
Adds provider-path regression test for empty committed prompt + caller instructions handoff, plus unit tests for resolver precedence and reserved-markup grammar. |
Scripts/Fixtures/test-suite-contract-ledger.tsv |
Registers the two new tests in the curated contract ledger with rationale/coverage notes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
baron
left a comment
There was a problem hiding this comment.
Approved — no blocking findings.
This fixes #526 at the correct authority boundary. The request-local resolver preserves a genuine committed tab prompt when available and otherwise uses sanitized caller instructions, without promoting discovery output or mutating canonical tab state.
The additional handling for <discovery_agent-guidelines> is important because the tool contract explicitly says those hints are discovery-only. The resolver’s fail-closed behavior is conservative, and the provider-path regression test plus adversarial parser coverage give good confidence in the handoff.
Two non-blocking follow-ups worth considering:
- Preserve more specific diagnostics when instructions are rejected or only discovery output exists, rather than reporting the generic “without a prompt” error.
- Add an explicit resolver case for non-empty instructions with an empty effective prompt and
usedAgentOutputAsPrompt == false.
Neither concern should block this PR. The current implementation is preferable to forwarding caller instructions verbatim, which would leak discovery-only guidance into the typed follow-up model.
|
Thanks for the thoughtful review and the two concrete follow-ups. Both are addressed in
The resolver still performs one fail-closed sanitization pass, successful prompt precedence is unchanged, and the existing provider/lifecycle coverage remains green. The hosted pipeline is also fully green (8/8). |
baron
left a comment
There was a problem hiding this comment.
Approved — the follow-up addresses both non-blocking review points, with no new blocking findings.
The resolver now preserves distinct, safe outcomes for a genuinely missing caller task, discovery-guidelines-only input, malformed reserved markup, and copied discovery output without an independent caller prompt. The resulting diagnostics are materially clearer while retaining the stable “without a prompt” stem and avoiding disclosure of withheld instruction content.
The added resolver case also pins the previously missing sibling path: non-empty caller instructions with an empty effective prompt and usedAgentOutputAsPrompt == false.
I reviewed the incremental change from 97efadc to bd15a1e; it addresses the requested follow-ups cleanly, and the current CI checks are green.
|
@mplibunao thank you so much for the awesome issues and this PR. Also thank you for addressing the optional follow up. We intend to ship this in the next release. |
Summary
Closes #526.
Root cause
MCP instructions were used for discovery, but typed generation later relied only on the committed tab prompt. Empty-prompt commits can contain discovery fallback prose marked with provenance, so the provider correctly rejected that prose but had no independent caller-task prompt to use.
Changes
Review and refactor gates
Validation
Local PR-ready limitation
The repository-wide PR-ready lint lane is currently blocked by SwiftFormat 0.62.1 reporting thousands of pre-existing findings in unrelated files. None of the reported findings are in this PR's three changed paths; the changed Swift files pass scoped checks.