feat(mcp): log the failures the MCP server hides from its clients - #106
Open
carldebilly wants to merge 2 commits into
Open
carldebilly wants to merge 2 commits into
carldebilly wants to merge 2 commits into
Conversation
Repl.Mcp had no logging at all, yet three of its paths absorb a failure on purpose: a command's failure detail is withheld from the remote client, a failed catalog rebuild is hidden behind the connection's previous catalog, and a failed roots prime is swallowed so a command that never reads roots still runs. Each was right for the client and left the operator with nothing — InvalidateRouting() looking like a no-op for as long as a rebuild kept failing, or a client stalling on roots/list invisibly. McpLoggerDiagnostics follows ReplResultFlowLoggerDiagnostics: the ILoggerFactory is taken from the services when present, NullLoggerFactory otherwise, under the category "Repl.Mcp", with source-generated [LoggerMessage] methods (event ids 2001-2006). No wiring changes: AddReplLogging() already reaches every MCP path. The two failures that can repeat on every request warn once per episode and log repeats at Debug. The stale catalog is republished stale, so every request retries the build — tools/call and prompts/get included, on every connection — and logging each retry would repeat the same exception for as long as the failure lasts. The episode is tracked per connection and per failing routing version, under the SnapshotGate that already serialises those builds, and a successful rebuild logs the recovery. The roots prime tracks its episode with an Interlocked flag, since concurrent tool calls prime concurrently. TDD: all three tests red first, with only the log assertion failing. Each policy was then falsified with a substitution that still compiles — warning on every retry, dropping the recovery log, inverting which failure of an episode warns, dropping the rendered detail — and each turned exactly its own test red. Refs #99
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: 32ef61efc2
ℹ️ 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".
Two ways the once-per-episode logging ended an episode that had not ended, both raised in review on #106. A roots prime skipped during the stand-down after a slow failure returned normally, and a normal return reset the episode — so once the stand-down lifted and the still-broken client failed again, the same outage warned a second time. PrimeCurrentAsync now reports whether it actually fetched, and only a real fetch ends the episode. The catalog recovery was logged at the version the build set out to serve, and whenever the build returned. A build that races a routing change is republished stale, and one that races a visibility retraction starts over at a newer version. The recovery is now read back from the entry actually published: only a current one ends the episode, and it is logged at the version it was built at. TDD: each of the three new tests red first for the reason given in review — a second 2003 after the skipped prime; a 2005 while the published catalog was still stale; a 2005 naming version 2 when version 3 was published. Each fix was then falsified with a compiling substitution and turned its own test red. Refs #99
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
1 participant
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.
Closes #99.
Repl.Mcphad no logging, yet three paths absorb a failure on purpose. Each is still hidden from the MCP client exactly as before; the operator now gets a log entry under the categoryRepl.Mcp:The two failures that can repeat on every request (2002 and 2003) warn once per episode and log repeats at Debug. The stale catalog is republished stale, so every request retries the build, on every connection, and would otherwise log the same exception on each retry.
The shape follows
ReplResultFlowLoggerDiagnostics: theILoggerFactorycomes from DI when present andNullLoggerFactoryotherwise. No wiring change was needed, becauseAddReplLogging()already reaches every MCP path.docs/mcp-reference.mdhas a new Operator diagnostics section, which includes a caution about stdout undermcp serveon stdio.This is also the logging #104 depends on: that issue sanitises a cold connection's disclosure and needs somewhere to put the detail it stops sending.
Test plan
Given_McpDiagnostics: one test per path, pinned to category, level and event id. All red before the fix, with only the log assertion failing.docs/mcp-reference.md.