Repository navigation
Conversation
…X-02) Concurrent discovery passes (reactive connect, boot and post-reload sweeps, HandleUpstreamServerChange, approval reindex) and operator approvals wrote the same approval records without mutual exclusion, so a stale pass overwrote a fresher record and tools were stranded pending across reload and restart. - per-server mutex covers checkToolApprovals (gate read, baseline decision and all writes), ApproveTools/ApproveAllTools/baseline promotion, BlockTools/ BlockAllTools, tool toggles, removed-tool deletes and the quarantine flip - passes also consult the storage quarantine flag (fail closed) so a server quarantined mid-flight is never auto-baselined - baseline promotion re-reads each record under the lock and never promotes a record that turned changed - the scanner's baseline+blocks commit runs under the lock via an optional ToolApprovalLocker capability - events and the reindex goroutine run after the lock is released
…-02) tools/approve and security/approve read the approval records back after the write and add approved_count, blocked_count, still_pending, still_changed and held_tools. tools/approve's approved now counts requested tools that actually read approved and lists the rest in not_approved. status stays "approved". review approve and security approve print the counts and warn on stderr when any tool still needs review.
…p pending rewrites (UX-02) With approval writes serialized per server, lock hold time matters. A discovery pass rewrote every unchanged pending record (one fsynced bbolt transaction per tool), and ApproveTools wrote one transaction per tool, so a 165-tool approval waited seconds behind a pass and then took seconds more. - storage: SaveToolApprovals writes many records in one transaction - approveToolsLocked reads the named records in one snapshot and commits them in one transaction (Spec 105 stamping rule preserved) - a pending record that stays pending with an unchanged definition is no longer rewritten
Approval now waits for an in-flight discovery pass on the server.
…rator writes The operator disable injected between a pass's read and its stamp now waits for the pass (per-server tool-approval lock) instead of landing in the window; the tests assert it waits and still survives.
… read (UX-02) An unreadable upstream record now counts as quarantined for the tool-approval pass instead of letting a stale trusted config auto-baseline the server. A missing record still defers to the config. Found by cross-review.
… removals (UX-02 review) - tools/approve and security/approve accept expected_hashes (tool -> review current_hash); a changed or unreviewed target fails with 409 and nothing is written. The review payload exposes current_hash; the CLI and the Web review screen send it. - A security approval promotes the reviewed pending tools inside the same locked operation as the baseline+blocks commit, and the unquarantine no longer re-promotes every pending record, so a tool discovered after the commit stays held. - A discovery pass only removes a tool (index entry, hash, approval record) while no newer pass or approval write ran since its inventory read (per-server inventory generation), so a stale removal cannot delete a fresh operator decision. - tools/approve reports unique applied counts from the runtime and outcome_verified; a failed read-back is never reported as complete, and the CLI warns about it.
…roval notice (UX-02 review)
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Deploying mcpproxy-docs with
|
| Latest commit: |
51eb8a9
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://d4b6d9af.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://fix-qa070-ux02-approval-seri.mcpproxy-docs.pages.dev |
…to safety hints, keep force-retry hashes (UX-02 review) - Inventory tickets are taken before tools/list; a pass whose inventory was captured before one already applied for the server writes nothing, so an older pass reaching the approval lock last cannot delete a fresh block or restore a rug-pulled tool. Last-good snapshots keep the newest ticket. - The review current_hash is a fingerprint of the definition plus safety hints; an annotation-only tier change makes a review-bound approval stale. The rug-pull hash is unchanged. - The Web force retry re-sends the hashes of the decision that triggered it, not those of a review reloaded while the dialog was open.
Contributor
📦 Build ArtifactsWorkflow Run: View Run Available Artifacts
How to DownloadOption 1: GitHub Web UI (easiest)
Option 2: GitHub CLI gh run download 38059742930 --repo smart-mcp-proxy/mcpproxy-go
|
…ixed reviews bound (UX-02 review) - Apply a discovery pass's approval decisions and the index writes derived from them in one critical section under the server's tool-approval lock, so an operator approval or newer pass can no longer land in between and be undone (blocked-tool eviction, stale definitions/annotations). - Publish an inventory to the StateView only while it is still the newest applied one; a stale or overtaken inventory no longer republishes older safety hints that set the dispatch tier. - Take the sweep's inventory ticket per server, immediately before that server's tools/list, so a genuinely newer capture is not dropped as stale. - Give hashless approval records a non-empty review fingerprint, and make the Web UI and CLI fall back to an unbound approval only when no tool carries a hash; a single hashless tool no longer unbinds the whole approval. - Number review loads in the Web UI so a late response for another server, or an older reload, cannot replace the review being approved.
The macOS review sheet now sends expected_hashes (tool name -> the review's current_hash) on POST security/approve and tools/approve, like the CLI and the Web review screen. The request stays unbound only when no tool in the review carries a hash (an older core); a single hashless tool on a newer core is left out of the binding, so the core refuses it unless it is blocked. A 409 "out of date" reloads the review and shows the core's message as a notice instead of an error, and never opens the force dialog. A force retry re-sends the hashes of the attempt that triggered it, so a review that changed while the dialog was open fails as out of date.
…bind approved tools in server approvals (UX-02 review) Inventory tickets were taken by each caller before its tools/list, so a pass that stalled before listing (or was coalesced onto another caller's list) carried an older ticket than a capture that started after it, and its newer inventory was dropped as stale. managed.Client.ListToolsTicketed now takes the ticket in the caller that leads the upstream list, once it owns the list; coalesced callers get the leader's ticket and never adopt an unticketed list. The single-server pass, the quarantined capture and the sweep use it. A review-bound server approval now also checks enabled approved tools, which the unquarantine activates: one whose safety hints changed since the review (annotations are not in the approval hash, so it stays approved) fails as out of date instead of being activated.
…e review sheet per server (UX-02 review) The sheet is keyed by server, so switching servers drops the previous review, selection and pending force decision. Review loads are ordered and only this server's newest response is shown. Server and per-tool approvals capture the block list and expected hashes when the operator clicks, so a review that reloads before the request runs cannot change what is sent; the force and blind confirmations re-send that captured decision.
…proval lock, record empty-review baselines, serialize review approvals (UX-02 review) - applyInventory re-checks quarantine (config or storage) under the server's approval lock, and the config-reload quarantine flip writes storage under that lock before the purge, so a paused pass cannot re-publish a quarantined server to the shared or profile indexes. - A server approval records a durable tool-baseline decision, so a tool first served after approving an empty reviewed inventory stays pending instead of being auto-approved as a first baseline. - Web review: responses answer only the review session that sent them (A -> B -> A no longer lets an old approval clear choices or open force). - macOS review sheet: one approval at a time; force re-sends the attempt that failed.
…provals to the empty snapshot, lock tool selection during approval (UX-02 review)
…the empty snapshot (UX-02 review)
… screen no longer shows (UX-02 review)
…swers for a review the screen no longer shows (UX-02 review)
…er approval unquarantines (UX-02 review) The commit validated the reviewed fingerprints, but the unquarantine ran after the approval lock was released. Annotations are outside the approval hash, so a capture in between that only flipped a reviewed tool's safety hints left it approved and the unquarantine activated the unreviewed version. UnquarantineServerKeepingToolDecisions now takes the blocked tools and expected fingerprints and rechecks them under the same lock as the quarantine flip; on a mismatch the server stays quarantined and the stale review error (409) reaches the caller.
This was referenced Oct 11, 2026
Closed
This was referenced Oct 11, 2026
Dumbris
added a commit
that referenced
this pull request
Oct 11, 2026
…vals are complete (UX-02) Per-server lock around every tool-approval read-modify-write (discovery pass incl. quarantine read and baseline decision, approve/block/toggle, baseline promotion that never promotes a changed tool, removed-tool deletes, quarantine flip, scanner approve-with-blocks), so concurrent writers can no longer strand approved tools as pending. Approval responses and CLI review/security approve report approved/blocked counts and remaining holds. Live-verified on a rebuilt daemon (10/10 fresh 320-tool baselines fully approved; 165 read approved+callable / 15 destructive held via CLI and Web; durable across restart, reload, reconnect, rediscovery; new/changed tools still held). OpenCode gpt-6.1-sol VERDICT: PASS at 3797afb. Supersedes #1555; follow-ups #1565-#1569.
2 tasks done
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.
Summary
Fixes UX-02 from the 0.70.0 UX QA audit. A stable toolset sometimes stayed partly pending after a trusted-server baseline or after an approval, while the approval reported success.
Root cause (shown by deterministic tests and live debug logs): several writers changed one server's tool-approval records at the same time, with no mutual exclusion. Each did read-modify-write on whole records. The writers were the discovery passes (reactive connect, boot sweep, post-reload sweep,
HandleUpstreamServerChange, approval reindex, quarantined capture), operator approve/block/toggle, the server-approval baseline promotion, and the scanner's baseline+blocks commit. When two overlapped, a stale pass overwrote a fresher record:The stranded records then survived hot reload and restart, because no later pass is a baseline pass.
Changes
Runtime: per-server serialization (
internal/runtime/tool_approval_lock.go,tool_quarantine.go,lifecycle.go)Each server gets its own
sync.Mutex(in async.Map). It is never global, so one large server cannot stall approvals on other servers. The lock covers:checkToolApprovalspass: the gate read, the prior-record snapshot, theisBaselinePassdecision and every write. The pass also reads the storage quarantine flag and treats the server as quarantined if either source says so. This fails closed: a storage record that cannot be read or decoded counts as quarantined, and only a missing record defers to the config.QuarantineServerflips storage under the lock before the runtime config catches up, so a server quarantined mid-flight is never auto-baselined.ApproveTools,ApproveAllTools,BlockTools,BlockAllTools, tool toggles and removed-tool deletes.approveToolsLockedre-reads every record under the lock. In pending-only mode it never promotes a record that has becomechanged, which keeps the rug-pull guarantee.QuarantineServer's storage flip, and only that flip. The lock is never held acrossSaveConfiguration,LoadConfiguredServers,HandleUpstreamServerChangeorUnquarantineServer.scanner.ToolApprovalLockercapability on the unquarantiner adapter. This stops a pass holding a stale pending copy from overwriting an operator block.SSE emits and the reindex goroutine run after the lock is released.
Runtime and storage: keep lock hold time short
storage.SaveToolApprovalswrites many records in one bbolt transaction. An approval reads the named records in one snapshot and commits them in one transaction. The Spec 105 stamping rule is applied per record, assaveReadToolApprovaldid.Effect: end-to-end CLI approval of a 180-tool server takes about 2–7s on a loaded host, against 5.4–6.3s on base and about 20s with the lock but without these two changes.
API and CLI: report what actually applied
Fields are additive.
status,server_nameandapprovedkeep their meaning.POST /servers/{id}/security/approveandPOST /servers/{id}/tools/approveread the records back after the write and returnapproved_count,blocked_count,still_pending,still_changedandheld_tools(capped at 50).tools/approve'sapprovednow counts requested tools whose record actually reads approved, and the rest are listed innot_approved. Before, it echoed the request length.mcpproxy review approveandmcpproxy security approveprint the counts in table output. In every output format they print a warning to stderr naming any tool still held.security approvegets the same 30s budget asreview approve, since an approval can now wait for an in-flight pass.docs/api/rest-api.mdanddocs/cli/review-commands.md.Compatibility decisions
statusstays"approved"when the server approval succeeds. Remaining holds are reported through the new counts, not through a new status value, so the Web UI, macOS tray and CLI JSON consumers need no change.ReviewScreen.vueis not touched; another PR owns it.mcpproxy review approve <server>).Verification
Unit and race tests:
internal/runtime/tool_quarantine_concurrency_test.go) pin both lost-update interleavings through a test-only read hook.read_006=pendingandread_003=pending.changedWithToolApprovalLockbeing per serverCommands run:
Live checks used the rebuilt
make buildbinary against scratch daemons with fixture upstreams.The default-selection approval checks used a fixture variant whose trusted server's tools have a different prefix. In the audit's original fixture both servers serve identical names, so the scan returns 180 hard-tier cross-server shadowing findings and the default approve is correctly refused without
--force. That fixture is covered by the diagnosis replay row below.review approve large-review --yes(default selection), 180 toolsstill_pending:0, 165/15 immediately and after 120s--force --all --except <15 delete_*>on the shadowing fixtureThe approved state also stayed the same after each of these:
MCP behaviour after approval:
call_tool_readexecutes a formerly stranded tool, andcall_tool_destructiveon a blocked tool returnsTOOL_BLOCKED./mcp/allbehaves the same: 165 read and 15 destructive listed, the read call executes and the destructive call is blocked.The guards still hold:
Warning: 1 tool(s) on server 'large-review' still need review (0 pending, 1 changed): read_customer_record_005.Cross-review
OpenCode
github-copilot/gpt-6.1-sol(variant high), run read-only:1ea86db55: VERDICT: PASS.The last commit only applies a lint-requested De Morgan rewrite.
🤖 Generated with Claude Code