Repository navigation
feat(quarantine): rescan automatically after a tool definition change (Spec 118) - #1689
Merged
Merged
Conversation
… (Spec 118) In manual and scan trust modes, a changed or newly pending tool now gets the in-process TPA scan of its current definition recorded as hold evidence (held_reason awaiting_review + held_verdict clean for a clean verdict), and a debounced informational server rescan brings the review scan line back to current. Gated by security.auto_baseline_scan and the new security.auto_rescan_on_change (env MCPPROXY_AUTO_RESCAN_ON_CHANGE). RT-02 is untouched: a clean verdict never approves a changed contract. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Deploying mcpproxy-docs with
|
| Latest commit: |
9d99067
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://52edd0d9.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://claude-zen-grothendieck-51c9.mcpproxy-docs.pages.dev |
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
…n (Spec 118) Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Dumbris
enabled auto-merge (squash)
October 20, 2026 16:17
1 task done
Dumbris
added a commit
that referenced
this pull request
Oct 22, 2026
## Description `TestInformationalRescan_BusyServerIsRetriedNotDropped` (Spec 118, #1689) failed on `main` in 4 of the last 8 unit-test runs and was the only red check on contributor PRs #1727, #1725 and #1715. **Cause.** `fireInformationalRescan` deletes the server's entry from `infoRescanTimers` when the debounce timer fires and re-creates it only after the gates and the claim ran. On every busy fire there is a short window in which "is a timer armed" is false. The test sampled the map at one fixed instant (`time.Sleep(4 * debounce)`), which under CI load lands in that window. Reproduced locally: 1 failure in 150 runs with `GOMAXPROCS=1` under 2×ncpu CPU burners. **Fix (test only).** Read `infoRescanSeq` under the mutex before scheduling; the change itself takes `before+1`, so `require.Eventually(rescanSeq("srv") > before+1)` is a timing-free proof that a busy fire re-armed the timer. The "no scan attempted on top of a claimed slot" and "a busy slot is not a failed attempt" assertions are unchanged. **Verification.** 400 runs of the fixed test under the same CPU contention: 0 failures. `go test -race -count=3 -run TestInformationalRescan ./internal/server/` green. `golangci-lint` with the CI v2 config clean. Cross-reviewed with opencode (gpt-6.1-sol), two rounds: round 1 found the baseline read could hit the same gap (fixed by reading the counter before scheduling), round 2 CLEAN. No production code changes. ## Second flake, same PR `TestRefreshConcurrency_ScansDuringASlowSelfCheckSeeOldOrNew` (from #1720) failed the darwin-amd64 Build Binaries job on this PR: `119.6ms is not less than 109.7ms (baseline median 1.19ms)`. Both refresh-concurrency guards compared the single worst of 300 scans against `50*median+50ms`, which one GC pause on a macOS runner exceeds. They now bound p90 relative to the baseline and keep an absolute 2 s ceiling on the worst scan (no scheduler stall reaches it, any wait behind the gate or a fetch deadline does). 30 runs under CPU contention: 0 failures. opencode Sol 6.1 round 1 flagged that p90 alone could hide one scan absorbing a timed-out wait; the absolute ceiling is the answer to that.
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.
What
After a tool definition changes, the review screen no longer says "Scan out of date … Rescan" with an unscanned held tool. In
manualandscantrust modes (notauto):recordScanHold. A clean verdict is stored asheld_reason: awaiting_review,held_verdict: clean, so it is distinct fromscan_findings(threat) andscan_coverage(precaution). Web UI shows "Scan clean, awaiting your review";tools listHELD showsclean.Runtime.AddToolDefinitionsChangedObserver, brings the review scan line back to "current". It reuses the claim, the run mutex and themaxInformationalScanAttemptscap; a busy slot re-arms instead of failing. Skips disabled,auto, and quarantined scan-mode servers.security.auto_baseline_scanAND the newsecurity.auto_rescan_on_change(*bool, default on, envMCPPROXY_AUTO_RESCAN_ON_CHANGE, env wins). Wired through loader, env, hot-reload,/config/apply, swagger, docs.RT-02 invariant is untouched
No approval decision, status transition or gate changed. A clean verdict never approves a changed or new tool in manual or scan mode (tests assert status stays
changed/pendingandApprovedHashdoes not move).awaiting_reviewalso does not widen the review screens' default selection (reviewDefaultAllowedstill requires an emptyheld_reason).Notes: the
trust_mode: scangate's own scan of new tools is not switched off by either setting. A pending tool whose definition moves is now rescanned, which replaces stale evidence of the old definition (one legacy-adoption test now pins its carry-over with the switch off). Spec:specs/118-auto-rescan-on-change/spec.md.Verification
go test -race ./internal/runtime/...ok; config, storage, httpapi, cmd/mcpproxy ok; CI-style-tags serverrace run (serveredition, config, oauth, server, httpapi, storage) ok./scripts/test-api-e2e.sh: 69 passed, 0 failedvue-tscclean🤖 Generated with Claude Code