Repository navigation
mcpx: guard unknown write outcomes (#1977) - #4721
Conversation
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 5 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. CodeRabbit Verify
|
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: 2a1859e2c1
ℹ️ 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 err != nil && dispatched.Load() && !toolReadOnly(p, tool) && r.writeLedger != nil { | ||
| return nil, r.recordUnknownOutcome(p, server, tool, args, surface) |
There was a problem hiding this comment.
When an upstream answers a dispatched write with a conclusive JSON-RPC rejection, such as -32602 for invalid parameters, mcpclient.CallTool returns that response as an error. This condition treats it like a lost response, replaces the original diagnostic with ErrUnknownOutcome, and durably blocks every subsequent write to the server until an operator clears it. Distinguish explicit rejections from genuinely unknown outcomes before recording the ledger entry so callers can correct rejected requests without requiring manual recovery.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f2f9225: callWithLedger now skips recording when mcpclient.IsConclusiveUpstreamError(err) (any JSON-RPC error without the mcpxStreamLost marker, e.g. -32602). Only a lost response records ErrUnknownOutcome. Covered by TestIsConclusiveUpstreamError.
| tools, _, _ := p.CachedSchemas() | ||
| for _, candidate := range tools { | ||
| if candidate.Name == tool { | ||
| return policy.Classify(candidate.Annotations).ReadOnly |
There was a problem hiding this comment.
With write policy allow and the destructive floor disabled, applyWritePolicy skips schema refresh. If the cached read-only annotation has expired or been invalidated by tools/list_changed, this helper still trusts it: Pool.Invalidate clears the timestamp but retains the old tools. A tool that has become writable can therefore execute, lose its response, and be exempted from recording an unknown outcome, leaving retries unblocked. Require a fresh annotation before granting the read-only exemption; stale or invalidated metadata should conservatively retain the write block.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f2f9225: toolReadOnly now returns false on missing (zero timestamp), invalidated (Invalidate clears timestamp), or expired (SchemaExpired) catalogues, and the ledger gate refreshes fail-closed. Covered by TestToolReadOnlyRejectsStaleCatalogue.
| record, blocked := r.writeLedger.Lookup(serverName) | ||
| if !blocked { | ||
| return policy.Decision{}, false |
There was a problem hiding this comment.
If a server has an uncleared unknown write and a lexicographically earlier alias for the same process is added before restarting the daemon, this lookup no longer finds its persisted block. ResolveAll sorts names, and NewRegistry shares pools by process identity, so the new alias creates the pool and becomes p.Name(). Both the original server and its aliases then consult the new name rather than the name stored in the ledger, allowing the original write to be retried without an explicit clear. Resolve persisted blocks across all names sharing the pool, or use an identity that remains stable when aliases are added.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f2f9225: added ledgerLookupFor checking every name sharing the pool plus requested/canonical spellings, and recordUnknownOutcome persists under CanonicalName(requested) so a restart reordering aliases keeps the block. Clear path (clearUnknownOutcome) clears all sharing names. Covered by TestLedgerBlockSurvivesSharedPoolAliasChange and TestRecordUnknownOutcomeUsesRequestedServerName.
| l.path = filepath.Join(stateDir, writeLedgerFilename) | ||
| data, err := os.ReadFile(l.path) |
There was a problem hiding this comment.
When two configuration-specific daemons run under the same state directory, they both open this fixed ledger filename: Paths.ForConfig changes only the socket and info paths, not State. Each ledger loads its own in-memory snapshot and Record or Clear replaces the entire file using only a process-local mutex. If both daemons start with an empty ledger, daemon A records an unknown write, and daemon B subsequently records another, B overwrites A's persisted block. Restarting A then silently permits retries of its unresolved write. Give each configuration-specific daemon an isolated ledger, or coordinate updates through shared transactional storage.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f2f9225: OpenWriteLedger(stateDir, configSources...) keys the filename unknown-writes-.json, so config-specific daemons sharing one state dir no longer overwrite each other; empty sources keep the legacy filename. Covered by TestWriteLedgerIsolatedPerConfig.
- conclusive JSON-RPC errors no longer record unknown outcomes - read-only exemption requires fresh catalogue - ledger lookup/clear span shared-pool aliases - ledger isolated per config sources
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Summary
mcpx writes clear <server>andPOST /v1/writes/{server}/clear; generate CLI/API/plugin entries from the operation table.Evidence
origin/mainhad only theLedgerCheckhook stub and no ledger or clear operation.go test ./internal/daemon -count=1,go test ./internal/mcpclient -count=1, andgo test ./internal/api -count=1pass. Targeted CLI parity and stop-all tests pass.TestStopAll); its isolated rerun passed.mcpx-session.tscopy drift (missedAt = 0vsDate.now()) on base. The generatedops.gen.tscopies are synchronized.Merge Danger
Door: two-way. Blast Radius: bounded.
A restart-persistent block is intentionally conservative and remains until an operator confirms the upstream outcome and clears it.
Open in [code]smith · Need help on this PR? Tag
@codesmith-botwith what you need.