fix(security): scanner Docker-skip follows per-server isolation mode (MCP-34.4)

CodexReviewer correctness finding on #781: the scanner skip was wired from a
single service-wide isolation mode (cfg.DockerIsolation.ResolvedMode()), but
isolation resolves per-server — a per-server isolation.mode overrides the
global (internal/upstream/core IsolationManager). So a server pinned to
isolation.mode:docker would wrongly have its Docker scanners skipped under a
global sandbox default, and vice versa, contradicting the per-server design and
the served docs.

Thread the scanned server's RESOLVED mode through the scan:
- ScanRequest.IsolationMode carries the per-server mode; engine.StartScan uses
  effectiveIsolationMode(req) (per-server wins, else engine-wide default) and
  passes it to resolveScanners(ids, mode).
- Service.SetIsolationModeResolver injects a per-server resolver; StartScan
  (both passes) populates req.IsolationMode via resolveIsolationMode(server).
- server.go wires the resolver from core.IsolationManager.ResolveMode against
  the live per-server config; SetIsolationMode stays as the engine-wide default
  fallback. Scanner package stays decoupled from the resolver.

Tests: per-server override cases (docker under global sandbox runs Docker
scanners; sandbox/none under global docker skips), effectiveIsolationMode
precedence, and Service.resolveIsolationMode fallback. go test -race +
golangci-lint v2 green.

Co-Authored-By: Paperclip <noreply@paperclip.ing>

Author: Dumbris

internal/security/scanner/engine.go

@@ -32,13 +32,15 @@ type Engine struct {
 	// docker × AppArmor transition would otherwise deny entrypoint exec.
 	disableNoNewPrivileges bool
 
-	// isolationMode is the resolved global isolation mode ("docker", "sandbox",
-	// "none", or "" == docker for back-compat). Scanner *plugins* are
-	// Docker-based (Spec 039); under "sandbox"/"none" the host runs no Docker
-	// for scanners, so Docker-based scanners are cleanly skipped (prefailed with
-	// an honest, mode-specific reason) while in-process scanners still run. This
-	// is MCP-34.4 / D3 option (b): clean, surfaced degradation rather than a
-	// misleading "pull the image" failure. Set via Service.SetIsolationMode.
+	// isolationMode is the engine-wide DEFAULT isolation mode ("docker",
+	// "sandbox", "none", or "" == docker for back-compat), used only when a
+	// ScanRequest does not carry a per-server IsolationMode. The per-server
+	// resolved mode on the request takes precedence (MCP-34.4) so a server with
+	// an explicit isolation.mode override gets the right scanner behaviour.
+	// Scanner *plugins* are Docker-based (Spec 039); under "sandbox"/"none" the
+	// host runs no Docker for scanners, so Docker-based scanners are cleanly
+	// skipped (prefailed with an honest reason) while in-process scanners still
+	// run — D3 option (b). Set via Service.SetIsolationMode.
 	isolationMode string
 
 	// Track active scans (one per server)
@@ -67,6 +69,14 @@ type ScanRequest struct {
 	Env            map[string]string // Additional environment variables
 	ScanContext    *ScanContext      // Context metadata (set by service)
 	ScanPass       int               // 1 = security scan (fast), 2 = supply chain audit (background)
+	// IsolationMode is THIS server's resolved isolation mode ("docker",
+	// "sandbox", "none", or "" to fall back to the engine's service-wide
+	// default). Set per-server by the Service so the Docker-scanner skip
+	// (MCP-34.4) follows the scanned server's effective mode — a per-server
+	// isolation.mode override beats the global mode (internal/upstream/core
+	// IsolationManager.ResolveMode). Under "sandbox"/"none" Docker scanner
+	// plugins are skipped; under "docker" they run.
+	IsolationMode string
 	// PeerTools is a cross-server snapshot (other servers' current tool
 	// definitions, keyed by server name) used by the in-process tpa-descriptions
 	// scanner to build a multi-server RegistryView so the deterministic
@@ -109,8 +119,11 @@ func (e *Engine) StartScan(ctx context.Context, req ScanRequest, callback ScanCa
 		return existing, fmt.Errorf("scan already in progress for server %s (job %s)", req.ServerName, existing.ID)
 	}
 
-	// Determine which scanners to use
-	resolved, err := e.resolveScanners(req.ScannerIDs)
+	// Determine which scanners to use. The Docker-scanner skip (MCP-34.4)
+	// follows THIS server's resolved isolation mode (per-server override beats
+	// the global default); fall back to the engine-wide default when the
+	// request didn't carry one.
+	resolved, err := e.resolveScanners(req.ScannerIDs, e.effectiveIsolationMode(req.IsolationMode))
 	if err != nil {
 		e.mu.Unlock()
 		return nil, err
@@ -195,7 +208,30 @@ type resolvedScanner struct {
 // executeScan loop records it as a failed scanner in the job. That way the
 // aggregated scan report shows "X of Y scanners failed" instead of pretending
 // nothing is wrong.
-func (e *Engine) resolveScanners(requestedIDs []string) ([]resolvedScanner, error) {
+// effectiveIsolationMode resolves the isolation mode to use for a scan: the
+// per-server mode carried on the ScanRequest when present, otherwise the
+// engine-wide default set via Service.SetIsolationMode. This is what lets the
+// Docker-scanner skip (MCP-34.4) honour a per-server isolation.mode override.
+func (e *Engine) effectiveIsolationMode(requestMode string) string {
+	if requestMode != "" {
+		return requestMode
+	}
+	return e.isolationMode
+}
+
+// resolveScanners determines which scanners to use.
+//
+// Unlike the old behaviour (silently dropping scanners with missing Docker
+// images), this now includes every enabled scanner in the result. When a
+// scanner's image is absent locally it is flagged with prefail so the
+// executeScan loop records it as a failed scanner in the job. That way the
+// aggregated scan report shows "X of Y scanners failed" instead of pretending
+// nothing is wrong.
+//
+// isolationMode is THIS server's resolved isolation mode (per-server override
+// already applied by the caller); under "sandbox"/"none" Docker scanner plugins
+// are skipped (MCP-34.4).
+func (e *Engine) resolveScanners(requestedIDs []string, isolationMode string) ([]resolvedScanner, error) {
 	all := e.registry.List()
 
 	// Helper: check whether a scanner's image is present locally. Returns a
@@ -214,9 +250,12 @@ func (e *Engine) resolveScanners(requestedIDs []string) ([]resolvedScanner, erro
 		// "pull the image locally" guidance below — there is nothing to pull.
 		// They stay in the resolved set (recorded as failed), which downgrades
 		// the scan summary to "degraded" rather than a silent all-clear.
-		if mode := e.isolationMode; mode == "sandbox" || mode == "none" {
-			return fmt.Sprintf("Docker-based scanner %s skipped: isolation mode %q runs no Docker containers, so Docker scanner plugins cannot run on this host. "+
-				"In-process scanners still ran. To run Docker scanners, set isolation.mode to \"docker\" on a host with a working Docker daemon. "+
+		// `isolationMode` is per-server resolved, so a server pinned to
+		// isolation.mode:docker still runs Docker scanners under a global
+		// sandbox default, and vice versa.
+		if mode := isolationMode; mode == "sandbox" || mode == "none" {
+			return fmt.Sprintf("Docker-based scanner %s skipped: isolation mode %q runs no Docker containers, so Docker scanner plugins cannot run for this server. "+
+				"In-process scanners still ran. To run Docker scanners, set isolation.mode to \"docker\" for this server on a host with a working Docker daemon. "+
 				"See docs/errors/MCPX_DOCKER_SNAP_APPARMOR.md.", s.ID, mode)
 		}
 		if e.docker == nil {

internal/security/scanner/engine_test.go

@@ -433,7 +433,7 @@ func TestEngineResolveScanners(t *testing.T) {
 	// Resolve all installed. The Docker scanner we just enabled plus the
 	// always-installed in-process scanner (tpa-descriptions) should both
 	// resolve (MCP-2082).
-	scanners, err := engine.resolveScanners(nil)
+	scanners, err := engine.resolveScanners(nil, "")
 	if err != nil {
 		t.Fatalf("resolveScanners: %v", err)
 	}
@@ -452,7 +452,7 @@ func TestEngineResolveScanners(t *testing.T) {
 	}
 
 	// Resolve specific
-	scanners, err = engine.resolveScanners([]string{"mcp-scan"})
+	scanners, err = engine.resolveScanners([]string{"mcp-scan"}, "")
 	if err != nil {
 		t.Fatalf("resolveScanners specific: %v", err)
 	}
@@ -461,13 +461,13 @@ func TestEngineResolveScanners(t *testing.T) {
 	}
 
 	// Resolve non-installed
-	_, err = engine.resolveScanners([]string{"cisco-mcp-scanner"})
+	_, err = engine.resolveScanners([]string{"cisco-mcp-scanner"}, "")
 	if err == nil {
 		t.Error("expected error for non-installed scanner")
 	}
 
 	// Resolve nonexistent
-	_, err = engine.resolveScanners([]string{"nonexistent"})
+	_, err = engine.resolveScanners([]string{"nonexistent"}, "")
 	if err == nil {
 		t.Error("expected error for nonexistent scanner")
 	}
@@ -586,7 +586,7 @@ func TestEngineInProcessScannerAlwaysAvailable(t *testing.T) {
 	docker := NewDockerRunner(logger)
 	engine := NewEngine(docker, registry, dir, logger)
 
-	resolved, err := engine.resolveScanners(nil)
+	resolved, err := engine.resolveScanners(nil, "")
 	if err != nil {
 		t.Fatalf("resolveScanners: %v", err)
 	}
@@ -618,11 +618,11 @@ func TestEngineResolveScannersSkipsDockerUnderSandbox(t *testing.T) {
 
 			// A non-nil docker runner with a real image present would normally
 			// let the Docker scanner pass checkImage; the sandbox/none gate must
-			// short-circuit before any Docker probe.
+			// short-circuit before any Docker probe. The per-server resolved mode
+			// is passed directly into resolveScanners (MCP-34.4).
 			engine := NewEngine(nil, registry, dir, logger)
-			engine.isolationMode = mode
 
-			resolved, err := engine.resolveScanners(nil)
+			resolved, err := engine.resolveScanners(nil, mode)
 			if err != nil {
 				t.Fatalf("resolveScanners: %v", err)
 			}
@@ -664,10 +664,12 @@ func TestEngineResolveScannersSkipsDockerUnderSandbox(t *testing.T) {
 	}
 }
 
-// TestEngineResolveScannersDockerModeUnaffected is the back-compat guard: with
-// the default isolation mode (docker / empty) the sandbox skip path is inert,
-// so a Docker scanner with a nil docker runner resolves without a prefail (the
-// historical behaviour relied on by existing tests).
+// TestEngineResolveScannersDockerModeUnaffected is the back-compat / per-server
+// guard: with mode "docker" (or "" — fall back to engine default) the sandbox
+// skip path is inert, so a Docker scanner with a nil docker runner resolves
+// without a prefail. The "docker" case is exactly the per-server override that
+// must keep running Docker scanners even under a global sandbox default
+// (MCP-34.4).
 func TestEngineResolveScannersDockerModeUnaffected(t *testing.T) {
 	for _, mode := range []string{"", "docker"} {
 		t.Run("mode="+mode, func(t *testing.T) {
@@ -677,9 +679,8 @@ func TestEngineResolveScannersDockerModeUnaffected(t *testing.T) {
 			registry.scanners["mcp-scan"].Status = ScannerStatusInstalled
 
 			engine := NewEngine(nil, registry, dir, logger)
-			engine.isolationMode = mode
 
-			resolved, err := engine.resolveScanners([]string{"mcp-scan"})
+			resolved, err := engine.resolveScanners([]string{"mcp-scan"}, mode)
 			if err != nil {
 				t.Fatalf("resolveScanners: %v", err)
 			}
@@ -693,6 +694,23 @@ func TestEngineResolveScannersDockerModeUnaffected(t *testing.T) {
 	}
 }
 
+// TestEngineEffectiveIsolationMode locks the precedence used by StartScan: the
+// per-request (per-server) mode wins; an empty request falls back to the
+// engine-wide default (MCP-34.4).
+func TestEngineEffectiveIsolationMode(t *testing.T) {
+	e := &Engine{isolationMode: "sandbox"}
+	if got := e.effectiveIsolationMode("docker"); got != "docker" {
+		t.Errorf("per-server 'docker' must win over engine default 'sandbox'; got %q", got)
+	}
+	if got := e.effectiveIsolationMode(""); got != "sandbox" {
+		t.Errorf("empty request must fall back to engine default 'sandbox'; got %q", got)
+	}
+	e2 := &Engine{isolationMode: ""}
+	if got := e2.effectiveIsolationMode("none"); got != "none" {
+		t.Errorf("per-server 'none' must win; got %q", got)
+	}
+}
+
 func TestEngineParseResultsSARIF(t *testing.T) {
 	dir := t.TempDir()
 	logger := zap.NewNop()

internal/security/scanner/service.go

@@ -123,6 +123,13 @@ type Service struct {
 	pulls          *pullManager
 	logger         *zap.Logger
 
+	// isolationModeResolver returns a server's resolved isolation mode
+	// ("docker"/"sandbox"/"none", or "" if unknown). Injected by the wiring
+	// layer so the scanner honours per-server isolation.mode overrides
+	// (MCP-34.4) without the scanner package depending on the isolation
+	// resolver. Nil ⇒ fall back to the engine-wide default.
+	isolationModeResolver func(serverName string) string
+
 	// In-memory scan summary cache — avoids expensive BBolt reads per server
 	summaryCache   map[string]*ScanSummary
 	summaryCacheMu sync.RWMutex
@@ -172,25 +179,53 @@ func (s *Service) SetScannerDisableNoNewPrivileges(disable bool) {
 	}
 }
 
-// SetIsolationMode records the resolved global isolation mode ("docker",
-// "sandbox", "none", or "" == docker) so the scan engine can decide whether
-// Docker-based scanner plugins can run. Under "sandbox"/"none" the host runs no
-// Docker for scanners, so Docker scanner plugins are cleanly skipped (the scan
-// summary degrades) while in-process scanners still run. This is MCP-34.4 / D3
-// option (b): clean, surfaced degradation on snap-docker / non-Docker hosts.
+// SetIsolationMode records the engine-wide DEFAULT isolation mode ("docker",
+// "sandbox", "none", or "" == docker), used when a scan has no per-server mode
+// (no resolver wired, or the resolver returns ""). Per-server resolution via
+// SetIsolationModeResolver takes precedence. Under "sandbox"/"none" the host
+// runs no Docker for scanners, so Docker scanner plugins are cleanly skipped
+// (the scan summary degrades) while in-process scanners still run — MCP-34.4 /
+// D3 option (b): clean, surfaced degradation on snap-docker / non-Docker hosts.
 func (s *Service) SetIsolationMode(mode string) {
 	if s.engine == nil {
 		return
 	}
 	s.engine.isolationMode = mode
 	if mode == "sandbox" || mode == "none" {
-		s.logger.Warn("Isolation mode runs no Docker for scanner plugins; Docker-based scanners "+
-			"will be skipped and the security scan will report 'degraded' for affected servers. "+
-			"In-process scanners (e.g. tpa-descriptions) still run. See docs/errors/MCPX_DOCKER_SNAP_APPARMOR.md.",
+		s.logger.Warn("Default isolation mode runs no Docker for scanner plugins; Docker-based scanners "+
+			"will be skipped and the security scan will report 'degraded' for affected servers "+
+			"(per-server isolation.mode:docker overrides). In-process scanners (e.g. tpa-descriptions) "+
+			"still run. See docs/errors/MCPX_DOCKER_SNAP_APPARMOR.md.",
 			zap.String("isolation_mode", mode))
 	}
 }
 
+// SetIsolationModeResolver injects a per-server isolation-mode resolver so the
+// Docker-scanner skip (MCP-34.4) follows each scanned server's RESOLVED mode —
+// a per-server isolation.mode override beats the global default. The resolver
+// returns "docker"/"sandbox"/"none", or "" to fall back to the engine-wide
+// default. Wired in the server layer from the upstream IsolationManager so the
+// scanner package stays decoupled from the resolver.
+func (s *Service) SetIsolationModeResolver(resolver func(serverName string) string) {
+	s.isolationModeResolver = resolver
+}
+
+// resolveIsolationMode returns the isolation mode to apply to a scan of
+// serverName: the per-server resolver result when it yields a concrete mode,
+// otherwise the engine-wide default. Falls back gracefully when no resolver is
+// wired (e.g. unit tests) so behaviour matches SetIsolationMode alone.
+func (s *Service) resolveIsolationMode(serverName string) string {
+	if s.isolationModeResolver != nil {
+		if mode := s.isolationModeResolver(serverName); mode != "" {
+			return mode
+		}
+	}
+	if s.engine != nil {
+		return s.engine.isolationMode
+	}
+	return ""
+}
+
 // SetFetchPackageSource toggles whether the source resolver may fetch the
 // published source of package-runner servers (npx/uvx) for scanning. See
 // SecurityConfig.ScannerFetchPackageSource (MCP-2206). Default is enabled.
@@ -681,11 +716,12 @@ func (a *scanCallbackAdapter) OnScanFailed(job *ScanJob, err error) {
 // After Pass 1 completes, Pass 2 (supply chain audit) is auto-started in the background.
 func (s *Service) StartScan(ctx context.Context, serverName string, dryRun bool, scannerIDs []string, sourceDir string) (*ScanJob, error) {
 	req := ScanRequest{
-		ServerName: serverName,
-		DryRun:     dryRun,
-		ScannerIDs: scannerIDs,
-		SourceDir:  sourceDir,
-		ScanPass:   ScanPassSecurityScan,
+		ServerName:    serverName,
+		DryRun:        dryRun,
+		ScannerIDs:    scannerIDs,
+		SourceDir:     sourceDir,
+		ScanPass:      ScanPassSecurityScan,
+		IsolationMode: s.resolveIsolationMode(serverName),
 	}
 
 	// Build scan context for transparency
@@ -892,9 +928,10 @@ func (s *Service) startPass2(serverName string, serverInfo *ServerInfo) {
 	ctx := context.Background()
 
 	req := ScanRequest{
-		ServerName: serverName,
-		DryRun:     false,
-		ScanPass:   ScanPassSupplyChainAudit,
+		ServerName:    serverName,
+		DryRun:        false,
+		ScanPass:      ScanPassSupplyChainAudit,
+		IsolationMode: s.resolveIsolationMode(serverName),
 	}
 
 	// Build scan context

internal/security/scanner/service_test.go

@@ -391,6 +391,43 @@ func TestServiceSetIsolationMode(t *testing.T) {
 	}
 }
 
+// TestServiceResolveIsolationModePerServer verifies the per-server resolver
+// (MCP-34.4 review fix): a per-server resolved mode takes precedence over the
+// engine-wide default, so a server pinned to isolation.mode:docker keeps
+// running Docker scanners under a global sandbox default, and a server resolved
+// to sandbox/none skips them under a global docker default. A "" resolver
+// result (or no resolver) falls back to the engine-wide default.
+func TestServiceResolveIsolationModePerServer(t *testing.T) {
+	svc, _, _ := newTestService(t)
+	svc.SetIsolationMode("sandbox") // engine-wide default
+
+	// No resolver wired yet → fall back to the engine default.
+	if got := svc.resolveIsolationMode("any"); got != "sandbox" {
+		t.Errorf("with no resolver, expected fallback to engine default 'sandbox', got %q", got)
+	}
+
+	perServer := map[string]string{
+		"pinned-docker":  "docker", // overrides the global sandbox default
+		"pinned-none":    "none",
+		"pinned-sandbox": "sandbox",
+		"inherits":       "", // resolver yields "" → fall back to default
+	}
+	svc.SetIsolationModeResolver(func(serverName string) string { return perServer[serverName] })
+
+	cases := map[string]string{
+		"pinned-docker":  "docker",
+		"pinned-none":    "none",
+		"pinned-sandbox": "sandbox",
+		"inherits":       "sandbox", // "" → engine default
+		"unknown-server": "sandbox", // not in map → "" → engine default
+	}
+	for server, want := range cases {
+		if got := svc.resolveIsolationMode(server); got != want {
+			t.Errorf("resolveIsolationMode(%q) = %q, want %q", server, got, want)
+		}
+	}
+}
+
 func TestServiceListScannersEmpty(t *testing.T) {
 	svc, _, _ := newTestService(t)