feat(engine): surface connected MCP server instructions - #243
Open
andybons wants to merge 4 commits into
Open
Conversation
andybons
requested review from
dpup
and
a lite review from Copilot
and removed request for
dpup
September 2, 2026 19:12
There was a problem hiding this comment.
🟡 Changes recommended
The new instructions renderer and its tests currently allow (and don’t correctly exercise) empty/whitespace instruction entries, and MCPManager.Instructions() trims text despite documenting it as verbatim.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR surfaces MCP server–provided initialize.instructions text to the engine’s native session lane by storing it in mcp.Client, exposing it via MCPManager.Instructions(), and rendering a frozen <mcp_instructions> system segment early in the cached system prefix (ahead of the deferred MCP catalog) to keep prompt-cache bytes stable across turns.
Changes:
- Persist MCP
InitializeResult.Instructionsinmcp.Clientand expose it via an accessor. - Add
MCPManager.Instructions()and a newengine/mcp_instructions.gorenderer that emits a stable per-connected-server instructions system segment. - Update config/docs guidance for
append_system_prompt(platform-owned, byte-stable segments) and add unit/e2e coverage for instruction surfacing and system placement.
File summaries
| File | Description |
|---|---|
| mcp/client.go | Stores server initialize.instructions on successful initialize and exposes it via Client.Instructions(). |
| engine/mcp.go | Adds MCPServerInstructions and MCPManager.Instructions() to snapshot connected servers’ instruction text and tool names. |
| engine/mcp_test.go | Extends the fake MCP HTTP server to optionally return initialize.instructions. |
| engine/mcp_instructions.go | New renderer and session memoization for a frozen <mcp_instructions> system segment. |
| engine/mcp_instructions_test.go | Tests for rendering, neutralization, freezing behavior, live transport reading, and system-array placement stability. |
| engine/engine.go | Inserts the frozen MCP instructions segment into the system array before the deferred MCP catalog. |
| engine/AGENTS.md | Documents the “system array byte-stable within a session” rule and references the new frozen segment pattern. |
| docs/engine-request-cycle.md | Updates append_system_prompt contract and documents the connected MCP instructions segment behavior/placement. |
| docs/deploy-modal.md | Aligns deploy guidance with the updated append_system_prompt contract and byte-stability rule. |
| config/config.go | Updates AppendSystemPrompt documentation to reflect platform-owned facts/policy and byte-stability constraints. |
Review details
- Files reviewed: 10/10 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
andybons
added a commit
that referenced
this pull request
Sep 2, 2026
Three findings from the Copilot review on #243. renderMCPInstructions documents "" when no connected server set any instructions, but it rendered a <server> element for any entry it was handed, blank Text included. MCPManager.Instructions filters those, so no shipped caller hit it -- but this function takes the narrow mcpInstructionsReader precisely so another registry can implement it, and an unfiltered entry produced a block that says nothing at full prompt-prefix cost. It now drops a blank entry and returns "" when none survives, which makes the documented contract the function's own property rather than a caller's. MCPServerInstructions.Text was documented as InitializeResult.Instructions verbatim while Instructions stored the TRIMMED value. Trimming is a DISPLAY concern owned by the renderer, so Instructions now keeps the server's own bytes and trims only for the blank check. The renderer trims once, where the display form is decided. No rendered output changes. TestRenderMCPInstructionsAbsentCases had a case named "connected servers set no instructions" that passed entries: nil -- byte-identical to the "no servers connected" case above it, so the behavior it named was never exercised. It is replaced by two cases that pass a connected entry with empty and with whitespace-only Text. Red-verified: dropping the blank-entry filter fails both new cases with an empty <server> element. go test -race ./engine/ passes.
There was a problem hiding this comment.
🟡 Changes recommended
Untrusted MCP server instructions are currently unbounded in the cached system-prompt prefix, which can cause excessive token/cost and context exhaustion; the segment should apply a size cap with a truncation marker.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 10/10 changed files
- Comments generated: 1
- Review effort level: Lite
A native session never saw an MCP server's own usage guidance. engine/ mcp.go kept the tool list from ListAllTools and threw away the InitializeResult, so InitializeResult.Instructions -- the spec's channel for "how to use this server" text that no tool description carries -- reached the model on the delegated claude-code lane only, which gets it from the CLI's own --mcp-config handling. The same box behaved differently depending on which lane served it, and a server's guidance had no home but the operator's append_system_prompt. mcp.Client now keeps Instructions alongside the ServerInfo, protocol version, and capabilities it already kept. MCPManager.Instructions reports it per CONNECTED server with that server's namespaced tool names, mirroring Status. renderMCPInstructions builds one system segment: <mcp_instructions> <server name="..." tools="mcp__...__x, ...">text</server> </mcp_instructions> Placement is a cache decision, and it is the whole design. The segment sits in the SYSTEM array, before the deferred-MCP catalog: this is the stable half of the MCP context, and the catalog changes when tool search selects a tool. Session.mcpInstructionsSegment renders once and freezes. A system-array change invalidates the system and messages caches together, so re-rendering when a server's state changes re-processes the whole conversation. Live state keeps its own channel that costs nothing to change (mcpStatusSegment). The cost of freezing is one case: a server whose first attempt failed and whose retry later succeeds contributes tools but not instructions for that session. Server text is untrusted. The renderer defangs the block's own markup, so a server cannot forge a sibling <server> element under another server's name -- the defense renderTaskNotifications already applies to a child's result text. Entries are sorted in the renderer as well as the manager: a prompt-cache prefix must not depend on a caller's ordering. The contract for append_system_prompt widens to match, in config.Config, engine.Config, and both docs: platform-owned facts AND the platform policy that depends on them, never project instructions, never tool shape -- a server now states its own usage itself. Both places also gain the byte-stability rule these segments always needed. Verification: unit coverage for rendering, the four absent cases, markup neutralization, and the freeze; an end-to-end test over the real HTTP transport proves the wire field is read (red-verified by dropping the assignment in mcp.Client.Initialize); a placement test proves the segment reaches the system array and is byte-identical on turn 2 after a server drops off. go test -race ./... green.
Three findings from the Copilot review on #243. renderMCPInstructions documents "" when no connected server set any instructions, but it rendered a <server> element for any entry it was handed, blank Text included. MCPManager.Instructions filters those, so no shipped caller hit it -- but this function takes the narrow mcpInstructionsReader precisely so another registry can implement it, and an unfiltered entry produced a block that says nothing at full prompt-prefix cost. It now drops a blank entry and returns "" when none survives, which makes the documented contract the function's own property rather than a caller's. MCPServerInstructions.Text was documented as InitializeResult.Instructions verbatim while Instructions stored the TRIMMED value. Trimming is a DISPLAY concern owned by the renderer, so Instructions now keeps the server's own bytes and trims only for the blank check. The renderer trims once, where the display form is decided. No rendered output changes. TestRenderMCPInstructionsAbsentCases had a case named "connected servers set no instructions" that passed entries: nil -- byte-identical to the "no servers connected" case above it, so the behavior it named was never exercised. It is replaced by two cases that pass a connected entry with empty and with whitespace-only Text. Red-verified: dropping the blank-entry filter fails both new cases with an empty <server> element. go test -race ./engine/ passes.
CI runs against the merge with main, and main has since added TestEverySessionFieldIsClassifiedForSnapshotting. This branch adds Session.mcpInstrSeg and Session.mcpInstrLoaded, so the merge failed that guard even though both commits pass on their own: the test requires every Session field to be named in snapshottedSessionFields or in snapshotExcludedSessionFields with a reason. Both are excluded. They are a lazy render-once cache of the connected-MCP- server instructions block, rendered on first use from the live registry rather than replayed from the journal -- the same pattern the sibling instrLoaded/instrSeg/instrErr/instrPath entries are already excluded for. Snapshotting them would persist a rendering of registry state a reload re-derives anyway. Also rebases onto main, which is how the failure surfaced: the branch was six commits behind and the guard did not exist when its first CI run passed. Verification: go test -race ./engine/ and go build/vet ./... pass.
andybons
force-pushed
the
feat/mcp-server-instructions
branch
from
September 2, 2026 23:26
27eb5f4 to
b991511
Compare
A Copilot finding on #243. A server's instructions text is UNTRUSTED -- it arrives from whatever process the server config points at -- and it was spliced into the block unbounded. That matters more here than it would in a message. This block is a SYSTEM segment, written to the prompt cache once and re-read for every turn of the session, so a verbose or hostile server does not cost one large message: it inflates the cached prefix for the whole session, and far enough out it crowds the context window the conversation needs. The text is now capped at mcpInstructionsPerServerCap runes through capRunes, which appends taskLogTruncationMarker, so a model reading truncated guidance can tell that it is truncated. The cap is PER SERVER because the two inputs have different trust: the text is remote and unbounded, while the SET of servers is operator configuration in this box's own harness.json. A per-block cap would let one verbose server silently swallow a later server's guidance; per-server bounds the total at cap x servers, which the operator already controls. 4000 matches taskNotificationResultCap, the same bound this engine already applies to a child's untrusted Result text, and it sits well above what real servers send. Red-verified: removing the capRunes call fails TestRenderMCPInstructionsCapsServerText, which also asserts a server inside the cap is passed through unmarked and untrimmed. go test -race ./engine/ passes.
There was a problem hiding this comment.
🟢 Approval recommended
The changes are cohesive, include defensive handling for untrusted server text (neutralization + caps), and are backed by targeted unit and end-to-end tests covering the new behavior and cache-stability guarantees.
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 0 new
- Review effort level: Lite
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The problem
A native harness session never saw an MCP server's own usage guidance.
engine/mcp.gokept the tool list fromListAllToolsand discarded theInitializeResult, soInitializeResult.Instructions— the spec's channel for "how to use this server" text that no tool description carries — reached the model on the delegated claude-code lane only, which gets it from the CLI's own--mcp-confighandling. The same box behaved differently depending on which lane served it, and a server's guidance had nowhere to live but the operator'sappend_system_prompt.Both opencode and Claude Code surface server instructions; harness's native loop was the outlier, and inconsistent with harness's own claude-code lane.
The design
mcp.Clientnow keepsInstructionsalongside theServerInfo, protocol version, and capabilities it already kept — one field and one accessor, no signature changes.MCPManager.Instructionsreports it per connected server with that server's namespaced tool names, mirroringStatus(it never triggers a connect, and a server that set no text contributes nothing).renderMCPInstructionsbuilds one system segment:Placement is a cache decision, and it is the whole design.
mcpCatalogSegmentchanges when tool search selects a deferred tool, so it stays last.Session.mcpInstructionsSegmentrenders once and freezes. A system-array change invalidates the system and messages caches together (the tools cache survives), so re-rendering the block whenever a server's connection state changed would re-process the entire conversation on that turn. Liveness already has a channel that costs nothing to change — the ambient degraded-server block inmcp_status.go.This is deliberately not opencode's connected-only-per-turn filter — that is the one part of their design that puts volatile state in the cached prefix. It matches how
vercel-labs/fxsplits the same problem (static context in the cached prefix, transient context in an uncached overlay).Server text is untrusted: the renderer defangs the block's own markup, so a server cannot forge a sibling
<server>element under another server's name — the same defenserenderTaskNotificationsapplies to a child's result text. Entries are sorted in the renderer as well as in the manager, because a prompt-cache prefix must not depend on a caller's ordering.Contract change
append_system_promptsaid "environment facts… do not use it for tool instructions." With servers now stating their own usage, the rule widens to platform-owned facts and the platform policy that depends on them, keeping two hard exclusions: never project instructions (that isAGENTS.md), never tool shape. Updated inconfig.Config,engine.Config,docs/engine-request-cycle.md, anddocs/deploy-modal.md, which also gain the byte-stability rule these segments always needed.Verification
Unit coverage for rendering, the four "nothing to say" cases, markup neutralization, and the freeze (a server appearing and another dropping between turns leaves the segment byte-identical). An end-to-end test over the real HTTP transport proves the wire field is actually read — red-verified by dropping the assignment in
mcp.Client.Initialize. A placement test drives two real prompts and proves the segment reaches the system array, in the right position, byte-identical on turn 2.go test -race ./...green (22 packages),go vetclean.Note for meetneptune/boxes
internal/api/mcp_orchestration.go'sorchestrationInstructionscurrently reaches claude-code boxes only. Once this ships and the harness floor moves, the GitHub-events and fleet segments can move out of that repo'sappend_system_promptand into the server's own instructions, where they are present only when the server is attached.