Repository navigation
feat(quarantine): scan is the default trust mode; clean-scanned contract changes auto-approve (Spec 119) - #1740
Merged
Merged
Conversation
…act changes auto-approve (Spec 119) Maintainer decision 2026-10-22: by default the proxy should not challenge the user with approvals. If a tool contract changes and the offline scan is clean, approve it; a server switched to manual keeps asking. - config: an unset trust_mode resolves to scan (DefaultTrustMode); an unrecognized value still fails closed to manual. - runtime gate: under scan a changed contract with a clean, full-coverage verdict is approved in the same pass (reason scan_approved, ApprovedBy scan-approved, tool_auto_approved event); findings or uncovered tools are held with evidence as before; a safer-direction hint flip (readOnlyHint false->true, destructiveHint true->false) is held with GATE-HINT-SAFER-FLIP whatever the verdict; a still-changed record is re-evaluated every pass so pre-upgrade clean holds clear. Only while quarantine is enforced. Manual is unchanged (RT-02 now applies to manual only). - invariant table and qapilot model admit scan_approved for changed->approved; QP-10/S13 updated; mutation M07 replaced by a pending-only reason leak. - Web UI: scan is the presented default, selector copy, add-form default; CLI/MCP help text, generated contracts, OpenAPI; docs and CHANGELOG (Breaking Changes). Tool-surface goldens updated for the description text. - tests: fixtures written against "unset = manual" are pinned to manual through the shared helpers (setupQuarantineRuntime, spec-116 fixture, the counting-upstream fixture); default-tier behaviour has explicit scan tests. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Deploying mcpproxy-docs with
|
| Latest commit: |
ffc87f5
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://e4192189.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://claude-mcpproxy-ui-feedback.mcpproxy-docs.pages.dev |
- a failed scan-approval write restores the whole record before the hold is written, so the hold never carries the new approved hash or cleared Previous* evidence (the next pass's hash-match shortcut would otherwise approve the contract with no scan and no operator) - the GATE-HINT-SAFER-FLIP signal is recorded whatever the verdict, appended to the scan's own signals when the verdict is not clean (FR-007) - qapilot's EffectiveTrust resolves an empty trust mode to scan, matching config.ServerConfig.EffectiveTrustMode since spec 119 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
withHintFlip puts the gate's signal first so SetScanHold's MaxToolHeldSignals cap cannot drop it when a bundle matches that many signatures (review round 2). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…119) Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Contributor
📦 Build ArtifactsWorkflow Run: View Run Available Artifacts
How to DownloadOption 1: GitHub Web UI (easiest)
Option 2: GitHub CLI gh run download 39828900410 --repo smart-mcp-proxy/mcpproxy-go
|
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…ec 119) Under the scan default the admission scan settles clean and releases the server mid-test; these three tests pin the server-level quarantine path. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…edback-6d0ada # Conflicts: # docs/intro.md
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Contributor
|
This overlaps with #1713, so a few points from an operator who runs unattended agents behind the proxy:
None of this changes the decision that |
Member
Author
|
Thanks James. I answered all four points over on #1713, in full. Short version: a follow-up PR will hold any change to any of the four hints, and any input- or output-schema change that is not a pure narrowing, whatever the scan says. The audit event will keep the before-state, and a startup / |
Dumbris
added a commit
that referenced
this pull request
Oct 26, 2026
…proval (Spec 120) (#1781) ## Summary Spec 120 (`specs/120-scan-structural-gate/spec.md`), follow-up to #1740 (Spec 119) and Phase 0 of the maintainer decision posted on #1713 on 2026-10-25 after the reviews by @james-crowley and @kaspermuutra-dotcom. **Must merge before the release that carries the `scan` default (#1740): the two ship together.** Under `trust_mode: scan` a changed tool contract is approved when the offline TPA scan is clean. The scanner reads a schema as text and never sees the annotations, so a new required `bcc` parameter, an `openWorldHint` flipped to `false`, or a defaulted `webhook_url` all passed a clean scan. This PR puts a deterministic structural gate in front of that verdict. - **`contractDelta(approved, live) []Signal`** (`internal/runtime/tool_contract_delta.go`) replaces the spec-119 safer-flip rule at both `checkToolApprovals` gate sites. Any signal holds the change whatever the verdict; signals are prepended so `MaxToolHeldSignals` never drops them. Rules: - `GATE-HINT-CHANGE:<hint>:<old>>:<new>`: any effective change of the four safety hints, either direction, MCP defaults applied (`GATE-HINT-SAFER-FLIP` kept as the label of the two safer flips). - `GATE-SCHEMA-SHAPE:<reason>:<path>`: recursive keyword-path diff of the input schema (new required, type/$ref, enum member added or enum dropped, pattern/format, loosened or removed bound, `additionalProperties` relaxed, default/const, new property whose subtree carries default/const/enum, anyOf/oneOf grew, allOf shrank, items form, unknown approved schema). Narrowing edits pass. - `GATE-OUTPUT-SCHEMA:added|removed|changed` (text keywords stripped). - `GATE-HINT-CHANGE:dispatch:<old>>:<new>` (FR-009): the hash folds hints in by effective value while `DeriveCallWith` reads an explicit default differently from an absent hint, so `{readOnlyHint:false, destructiveHint:true}` → no annotations kept the hash and moved the tool from `call_tool_destructive` to `call_tool_read`. The failing test was written first (`TestHashEqualDispatchDrop_Bites`); it bit, so the hash-match, formula and content shortcuts are closed for a dispatch drift and the revert proof requires the variant too. - Explicitly not built: keyword lists, edit distance, classifier, holding every new optional property. - **Audit**: `tool_auto_approved` for a changed contract (scan-approved and `auto_approve_tool_changes`) now carries `old_hash`, `old_description`, `old_schema`, `old_output_schema`, `old_annotations`, the new values, `approved_by`, `gate_signals`, `scan_verdict`, `scan_bundle`. - **Upgrade visibility**: one-time startup WARN (marker `scan_default_notice_done`, nothing on a fresh install) and a `mcpproxy doctor` finding name every enabled server with no `trust_mode`. - **Contracts and UI**: terminology family `hold_signal` (Go, golden, generated TS consts, Web labels on the hold-evidence chips). Shared fixture `internal/runtime/testdata/contract_delta_cases.json`: Go asserts exact signals (51 cases), vitest asserts every held schema case yields a review line. - **Pilot**: `qapilot.Contract.Shape`, fixture `BenignWide`, QP-20, scenarios S28/S29, mutations M15–M19. - Docs: `security-quarantine.md` "Structural changes are held" with the residual risk stated plainly (paraphrase, one-word edit, text drift, behaviour behind an unchanged definition); spec 119 FR-007 amended; CHANGELOG Breaking Changes entry extended. ## Behaviour changes to existing tests (intentional) - Spec 119's "less-safe flip is scan-approved" is now held (any hint change holds). - A legacy changed record with no recorded previous input schema is held (`approved-unknown`), not scan-approved; a v4 collision whose live schema changes type is held under scan too. - Spec 117 fixtures that used "add an explicit `destructiveHint: true` on a tool approved without hints" as a hash-equal, still-approved change now use a title-only annotation change; that hint move is exactly the FR-009 hold. - Live mcp-go fixtures seed the explicit default hints mcp-go's `NewTool` serves. ## Verification `go test ./internal/runtime ./internal/qapilot ./internal/config ./internal/telemetry ./internal/storage ./internal/contracts`, `internal/httpapi` Spec 109 goldens, server-tag run of `internal/server`, `httpapi`, `serveredition`, `cmd` with the CI skip regex, `./scripts/test-api-e2e.sh`, qapilot conformance + `qapilot report` gate, frontend vitest (2762), both golangci-lint configs (only pre-existing findings outside this diff), `check-upstream-write-sites.sh`, `check-no-unquarantine-callers.sh`. Cross-model review: opencode `github-copilot/gpt-6.1-sol`, three chunks; findings and fixes listed in the first comment.
Dumbris
added a commit
that referenced
this pull request
Oct 30, 2026
… tier (#1802) ## Summary Since #1740 (Spec 119) made `scan` the default trust mode, the Spec 086 clean-scan admission gate has been releasing **every** server added without a `trust_mode`, including one added with `quarantined: true`. `TestE2E_QuarantineConfigApply` has failed on main since then (bisected to #1740). Maintainer decision: server admission stays human. Only an explicit `trust_mode: "scan"` auto-admits; the default tier only auto-approves clean tool changes. - `ServerConfig.AdmitsOnCleanScan()` is true only for an explicit `trust_mode: "scan"`. - `maybeAutoApproveScanSettled`, `maybeStartAdmissionScan` and `scanModeAdmissionOwns` use it. Default-tier servers get the informational (non-gating) baseline scan instead of the admission one. - The approval commit re-checks `AdmitsOnCleanScan` (and that the server is still quarantined) for `scan-auto` approvals inside the same commit, so a `trust_mode` changed while the scan was running cannot release the server. - Web UI: the manual add form sends `trust_mode` only when one is picked. The selector checks no radio for an unset mode (Scan keeps the "current default" badge), so Scan can be chosen explicitly. Previously the form always sent `scan`. - Docs (`intro.md`, `configuration.md`, `security-quarantine.md`), CLI `--trust-mode` help and the CHANGELOG Spec 119 entry describe the rule. ## Tests - New: default-tier server is never auto-approved and gets no admission scan (`scan_admission_test.go`); `scanModeAdmissionOwns` hands it to the informational path; `TestCommitServerApproval_ScanAutoRequiresExplicitScan` (manual and unset refused, explicit scan released). Each fails without the fix. - Frontend: explicit Scan is sent when picked; unset form omits `trust_mode`; selector emits `scan` from the unset state. - Locally: `internal/server` (server tag, CI skip regex), `internal/runtime`, `internal/config`, `cmd/...`, `internal/qapilot/...`, `httpapi`; `TestE2E_QuarantineConfigApply` passes again; `./scripts/test-api-e2e.sh` all passed; vitest 2791 passed (the 2 `activity-views.spec.ts` failures also fail on main locally); `vue-tsc`; both golangci-lint configs report 0 new issues. - Cross-reviewed with opencode Sol 6.1: round 1 found the commit-time race and the add-form opt-in gap (both fixed), round 2 CLEAN.
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
Spec 119 (
specs/119-scan-default-trust/spec.md). Maintainer decision 2026-10-22: by default the proxy should not challenge the user with approvals; if a contract changes and the scan is clean, approve it; a server switched tomanualalways asks.scanis the default trust mode. An unsettrust_moderesolves toscan(config.DefaultTrustMode); an unrecognized value still fails closed tomanual. Every surface that names the default follows (Web UI selector and add form, CLI help, MCP tool schema, generated contracts, OpenAPI, docs).scan, a clean-scanned contract change is approved in the same discovery pass (reasonscan_approved,ApprovedBy: scan-approved, activity eventtool_auto_approved). Findings, uncovered tools, and a safer-direction hint flip (readOnlyHintfalse→true,destructiveHinttrue→false, held with signalGATE-HINT-SAFER-FLIP) are held with evidence. A record that is alreadychangedis re-evaluated every pass, so pre-upgrade clean holds clear without a click. Only while quarantine is enforced globally.manualis unchanged: every change held, clean verdict recorded asawaiting_reviewevidence (the 2026-10-11 RT-02 rule now applies tomanualonly).autounchanged.scan_approvedfor changed→approved; QP-10 and S13 updated; mutation M07 replaced by a pending-only reason leak (auto_approve) so it still bites.Behaviour change for existing installs
A server whose
trust_modeis empty now runs underscanfrom its next discovery pass; its clean-scanned held changes are released. Settrust_mode: "manual"on servers whose tool changes should be reviewed by a person. Servers with an explicittrust_modeorauto_approve_tool_changesare unaffected.Tests
manualthrough the shared helpers (setupQuarantineRuntime, the spec-116 fixture,startCountingUpstreamnow inherits the configured mode); default-tier behaviour has explicitscantests (benign change scan-approved, still-changed clean approved, safer flip held, less-safe flip approved, EXP-6 paraphrase approved under scan / held under manual).upstream_serverstrust_modedescription text only.Verified locally:
go test ./internal/runtime ./internal/config/... ./internal/qapilot/... ./internal/storage/..., the server-tag run ofinternal/server,httpapi,serveredition,cmd/...with the CI skip regex,./scripts/test-api-e2e.sh(69/70, 1 skipped), qapilot conformance +qapilot report(896/896), frontend vitest (2723), both golangci-lint configs.🤖 Generated with Claude Code