fix(review): review screen starts fail-closed with exact-count approve (Spec fix-review-defaults) - #1481
Merged
Conversation
…prove uses it (Spec fix-review-defaults) ReviewTool.default_allowed is true only for an approved enabled tool or a pending/changed read tool with a clean current scan that is not held. mcpproxy review approve blocks every other tool unless --all or --tools is given, and its prompt names the exact count.
…exact-count approve label (Spec fix-review-defaults) Both screens read default_allowed from the core, show the selection hint, name the exact count on the approve button, add an explicit Approve all, keep the user's choices across reloads and re-send the attempted block list on the force retry.
…docs (Spec fix-review-defaults)
Deploying mcpproxy-docs with
|
| Latest commit: |
5f7dacd
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://07310bce.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://fix-review-fail-closed-defau.mcpproxy-docs.pages.dev |
F2.1: review approve rejects --tools/--except on a quarantined server with no captured tools instead of silently dropping them and approving blind. F3.1: ReviewScreen clears the force-retry block list and closes the force dialog when serverName changes; a forced approve without a stored block list recomputes from the current selection instead of reusing another server's.
Contributor
📦 Build ArtifactsWorkflow Run: View Run Available Artifacts
How to DownloadOption 1: GitHub Web UI (easiest)
Option 2: GitHub CLI gh run download 37086006049 --repo smart-mcp-proxy/mcpproxy-go
|
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…changed Review.vue showed the spinner on every background reload, which unmounted ReviewScreen and dropped its per-instance explicit checks and unchecks. Only the first load blanks the page now, and a failed background reload shows its error above the screen instead of replacing it.
- web: approve()/rescan() capture the server name and drop a response for a server the screen no longer shows (no error, no force dialog); the serverName watcher also resets approving. Late-response vitest cases added (fail without the fix). - Approve all wording: every pending or changed tool, previously blocked tools stay blocked (Web tooltip, macOS help, CLI --all help, docs, spec text). The fail-closed behaviour is unchanged. - macOS: ReviewTool decodes held_reason/held_signals so mergeSelection notices a hold-only change; XCTest varies held_reason.
11 tasks
…2 and research D43)
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
github-actions Bot
pushed a commit
that referenced
this pull request
Oct 3, 2026
…le reach (#1482) Docs, changelog and verified fixes from a first-run, upgrade and user-test pass on `main` (335477e). ## Docs - `docs/features/profiles.md`: profiles are optional. Without an effective profile, no profile-level restriction applies (a scoped credential's own grant still does). The quick start is presented as being for scoped access. - `docs/configuration.md` (`quarantined` row): explains the real default. A server added through the UI, CLI or API follows its trust mode. A first-seen config-file server with no `quarantined` key and no `config.db` record is held for review. An explicit value wins, and a recorded server keeps its state. Links to the admission rules. - `docs/features/security-quarantine.md`: the hand-editing section names the extra conditions: quarantine is enabled, and the trust mode is not `auto` (including legacy `auto_approve_tool_changes`/`skip_quarantine` resolving to `auto`), matching `ServerConfig.EffectiveTrustMode`. - `docs/getting-started/quick-start.mdx`: a plain note that a server added by editing the file is held for review until approved. - `CHANGELOG.md`: the security, review, catalog, telemetry, CLI and first-run fixes merged since v0.69.0 (#1463, #1467–#1473, #1481) plus this PR's. ## Fixes (each reproduced in an isolated instance, regression test fails on main) - **Upgrade start advisory repeated.** `reportPreFixAdmissions` ran on every gate pass (initial gate, `LoadConfiguredServers`, every publish), so an upgrade start logged the "predate the config-load admission gate" advisory 2–5 times. It is now reported once per server per process. Gate decisions are unchanged. - **Benign startup race logged as ERROR.** Both the supervisor reconcile and `LoadConfiguredServers` → `AddServer` asked a not-yet-ready client to connect. The second call was correctly refused with "connection already in progress or established (state: Connecting)" but logged at ERROR, and server identity registration was skipped. A new `managed.ErrConnectAlreadyActive` sentinel (same message text) lets `AddServer` treat it as success at debug level. - **`set_profile` under-reported a switchable client's reach.** After a successful switch, `resolveEffectiveProfileForJustSetSlug` treated the client binding as authoritative and reported the binding's servers instead of the selected profile's. ## Verification - Cross-model review: codex gpt-6.1-sol, chunked. Three low findings: two doc-wording findings are applied, and the third (the per-process advisory set is never cleared) is a deliberate trade-off. - Live re-verification on isolated instances: each symptom was reproduced on a main build and is gone on this branch. The upgrade case was run over a v0.52.1 data dir.
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
2 participants
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.
What changes for users
The review screen used to start every "Allow this tool" checkbox checked, including write and destructive tools whose definitions were never verified. It now fails closed.
GET /api/v1/servers/{id}/reviewcarriesdefault_allowed(always present). It is true for an approved, enabled tool, and for a pending or changed read tool whose scan verdict is clean and that is not held. Write, destructive, unannotated, unknown, not-scanned, warnings, dangerous and held tools are false. An older core has no field, which every surface reads as false.mcpproxy review approve <server>on a quarantined server blocks every tool outside the default selection.--allallows every tool,--tools a,b(now valid on a quarantined server) allows exactly those,--exceptsubtracts, and--allwith--toolsor an unknown tool name is an error before any write. The prompt now follows the review read and names the count and the blocked tools. Table output prints one summary line first; JSON and YAML stay the bare REST object. Behaviour change:review approve <server> --yesused to allow every tool; add--allfor that. The command has not shipped in a release.inspect_quarantinedcarriesdefault_allowed(captured tools from the composer, live toolsfalse);inspect_toolsdoes not. Noquarantine_securityoperation approves a server.Findings and spec
Codex user test F-02 (medium, security default). Spec:
specs/109-ux-navigation-consistency(FR-021, FR-023, US2-2; research D41; tasks T199 to T210; contracts, data model, quickstart recipe, acceptance index and parity matrix row 6 updated). Docs updated:docs/features/security-quarantine.md,docs/cli/review-commands.md,docs/cli/command-reference.md,docs/api/rest-api.md.Accepted residual: a server that declares
readOnlyHintdishonestly still gets its read tools pre-checked once the scan is clean, because annotations are self-declared and the scan checks descriptions. The tier stays visible and the tool can be blocked later.Tests added
TestReviewDefaultAllowed(tier by status by verdict by disabled by held table), coverage-driven composer cases, JSON key present when false, MCP captured and live parity, CLI default selection,--all,--tools, prompt count, JSON output,reviewApproveSelectiontable, three goldens.review-screen-default-selection.spec.ts(helpers and component); three assertions inreview-screen.spec.tsre-baselined on purpose because they pinned the old all-checked default.ReviewPresentationTests(selection, merge, labels, hint parity with the Web sentence) andReviewPayloadTestsdecode cases.