refactor(hooks/builtins): let DMR own its unload conventions

Address review feedback on #2706:

- Export pkg/model/provider/dmr.ProviderType and UnloadURL so the
  unload builtin becomes a dumb dispatcher and DMR owns the provider
  literal + the /v1 -> /_unload URL convention (sibling to the
  existing /_configure helper). Move the URL-resolution table test
  into the dmr package where the helper now lives.
- Gate the FromAgentModels snapshot in executeOnAgentSwitchHooks on
  executor.Has(EventOnAgentSwitch) so audit-free deployments don't
  pay the team-lookup + per-model allocation on every agent switch.
- Add a mixed-providers test pinning the per-element DMR filter:
  cloud entries (openai, anthropic) in the snapshot must be silently
  skipped, only DMR endpoints get POSTed.
- Add a comment on http.DefaultClient explaining why the SSRF-safe
  client used by http_post is wrong here (DMR is loopback).

Author: dgageot

pkg/hooks/builtins/unload.go

@@ -8,11 +8,11 @@ import (
 	"io"
 	"log/slog"
 	"net/http"
-	"net/url"
 	"strings"
 	"time"
 
 	"github.com/docker/docker-agent/pkg/hooks"
+	"github.com/docker/docker-agent/pkg/model/provider/dmr"
 )
 
 // Unload is the registered name of the on_agent_switch builtin that
@@ -31,6 +31,11 @@ import (
 // net/http. It carries no runtime-side coupling and silently skips any
 // model whose endpoint isn't reachable as plain HTTP (e.g. cloud
 // providers that don't expose [hooks.ModelEndpoint.BaseURL]).
+//
+// Provider dispatch and URL resolution are owned by
+// [pkg/model/provider/dmr] (see [dmr.ProviderType] and [dmr.UnloadURL]),
+// so this builtin stays a dumb dispatcher and DMR keeps full control
+// of its conventions.
 const Unload = "unload"
 
 // unloadTimeout caps each per-model Unload call so a stalled engine
@@ -47,7 +52,7 @@ func unload(ctx context.Context, in *hooks.Input, _ []string) (*hooks.Output, er
 		return nil, nil
 	}
 	for _, m := range in.FromAgentModels {
-		if m.Provider != "dmr" {
+		if m.Provider != dmr.ProviderType {
 			continue
 		}
 		if err := unloadOne(ctx, m); err != nil {
@@ -63,7 +68,7 @@ func unload(ctx context.Context, in *hooks.Input, _ []string) (*hooks.Output, er
 // (no base_url and no unload_api) is a silent no-op so the hook stays
 // harmless on test / in-process providers.
 func unloadOne(parent context.Context, m hooks.ModelEndpoint) error {
-	endpoint, err := unloadURL(m)
+	endpoint, err := dmr.UnloadURL(m.BaseURL, m.UnloadAPI)
 	if err != nil || endpoint == "" {
 		return err
 	}
@@ -79,6 +84,11 @@ func unloadOne(parent context.Context, m hooks.ModelEndpoint) error {
 
 	slog.DebugContext(ctx, "Unloading model", "url", endpoint, "model", m.Model)
 
+	// Unlike the http_post builtin, the unload target is the
+	// operator-configured DMR base URL — typically a loopback engine
+	// (Docker Desktop socket, 127.0.0.1:12434, …). The SSRF-safe
+	// dialer used by http_post would refuse those addresses by
+	// design, so we use the default client here.
 	resp, err := http.DefaultClient.Do(req)
 	if err != nil {
 		return fmt.Errorf("calling unload endpoint %s: %w", endpoint, err)
@@ -96,36 +106,3 @@ func unloadOne(parent context.Context, m hooks.ModelEndpoint) error {
 	_, _ = io.Copy(io.Discard, resp.Body)
 	return nil
 }
-
-// unloadURL picks the unload endpoint for one model, in this order:
-//
-//  1. unload_api is an absolute URL — used verbatim (lets users point
-//     at a different host than the model's base_url);
-//  2. unload_api is set but relative — rebased onto base_url's
-//     scheme + host (the model's path is dropped);
-//  3. unload_api is unset — the default `_unload` URL is derived from
-//     base_url by replacing its trailing `/v1` segment.
-//
-// Returns ("", nil) when neither base_url nor unload_api is set, so
-// the caller can skip without erroring.
-func unloadURL(m hooks.ModelEndpoint) (string, error) {
-	if strings.HasPrefix(m.UnloadAPI, "http://") || strings.HasPrefix(m.UnloadAPI, "https://") {
-		return m.UnloadAPI, nil
-	}
-	if m.BaseURL == "" && m.UnloadAPI == "" {
-		return "", nil
-	}
-	u, err := url.Parse(m.BaseURL)
-	if err != nil || u.Scheme == "" || u.Host == "" {
-		return "", fmt.Errorf("base_url %q is not absolute; cannot resolve unload endpoint", m.BaseURL)
-	}
-	switch {
-	case m.UnloadAPI == "":
-		u.Path = strings.TrimSuffix(strings.TrimSuffix(u.Path, "/"), "/v1") + "/_unload"
-	case strings.HasPrefix(m.UnloadAPI, "/"):
-		u.Path = m.UnloadAPI
-	default:
-		u.Path = "/" + m.UnloadAPI
-	}
-	return u.String(), nil
-}

pkg/hooks/builtins/unload_test.go

@@ -12,6 +12,7 @@ import (
 	"github.com/stretchr/testify/require"
 
 	"github.com/docker/docker-agent/pkg/hooks"
+	"github.com/docker/docker-agent/pkg/model/provider/dmr"
 )
 
 // dmrInput builds an on_agent_switch [hooks.Input] carrying a single
@@ -26,7 +27,7 @@ func dmrInput(server *httptest.Server, unloadAPI string, opts ...func(*hooks.Inp
 		FromAgent: "from",
 		ToAgent:   "to",
 		FromAgentModels: []hooks.ModelEndpoint{{
-			Provider:  "dmr",
+			Provider:  dmr.ProviderType,
 			Model:     "ai/qwen3",
 			BaseURL:   server.URL + "/engines/v1",
 			UnloadAPI: unloadAPI,
@@ -109,6 +110,43 @@ func TestUnload_HonoursOverrideUnloadAPI(t *testing.T) {
 	assert.Equal(t, "/custom/unload", gotPath)
 }
 
+// TestUnload_FiltersPerElement pins the per-element provider filter:
+// when the snapshot mixes DMR and non-DMR endpoints, only the DMR
+// ones are POSTed to. The non-DMR entries (cloud providers without a
+// reachable unload endpoint) must be silently skipped, not errored
+// on, not POSTed to a fabricated URL.
+func TestUnload_FiltersPerElement(t *testing.T) {
+	t.Parallel()
+
+	var gotModels []string
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		var body struct {
+			Model string `json:"model"`
+		}
+		_ = json.NewDecoder(r.Body).Decode(&body)
+		gotModels = append(gotModels, body.Model)
+		w.WriteHeader(http.StatusOK)
+	}))
+	defer server.Close()
+
+	in := &hooks.Input{
+		FromAgent: "from",
+		ToAgent:   "to",
+		FromAgentModels: []hooks.ModelEndpoint{
+			{Provider: "openai", Model: "gpt-4", BaseURL: "https://api.openai.com/v1"},
+			{Provider: dmr.ProviderType, Model: "ai/qwen3", BaseURL: server.URL + "/engines/v1"},
+			{Provider: "anthropic", Model: "claude", BaseURL: "https://api.anthropic.com"},
+			{Provider: dmr.ProviderType, Model: "ai/llama3.2", BaseURL: server.URL + "/engines/llama.cpp/v1"},
+		},
+	}
+
+	out, err := unload(t.Context(), in, nil)
+	require.NoError(t, err)
+	assert.Nil(t, out)
+	assert.ElementsMatch(t, []string{"ai/qwen3", "ai/llama3.2"}, gotModels,
+		"only DMR models must be POSTed; cloud providers must be silently skipped")
+}
+
 // TestUnload_NoOpInputs pins the cheap-path properties the agent loop
 // relies on: the hook MUST NOT fire any HTTP call when the input
 // describes a transition where unloading would be wrong (back to the
@@ -157,7 +195,7 @@ func TestUnload_NoOpInputs(t *testing.T) {
 			in: func(*httptest.Server) *hooks.Input {
 				return &hooks.Input{
 					FromAgent: "from", ToAgent: "to",
-					FromAgentModels: []hooks.ModelEndpoint{{Provider: "dmr", Model: "ai/qwen3"}},
+					FromAgentModels: []hooks.ModelEndpoint{{Provider: dmr.ProviderType, Model: "ai/qwen3"}},
 				}
 			},
 		},
@@ -194,94 +232,3 @@ func TestUnload_SwallowsServerErrors(t *testing.T) {
 	require.NoError(t, err)
 	assert.Nil(t, out)
 }
-
-// TestUnloadURL covers every branch of the URL-resolution algorithm in
-// one table. Replaces what used to be two separate tables for the now
-// inlined `defaultUnloadURL` and `rebaseURL` helpers.
-func TestUnloadURL(t *testing.T) {
-	t.Parallel()
-
-	tests := []struct {
-		name        string
-		baseURL     string
-		unloadAPI   string
-		want        string
-		errContains string // empty ⇒ expect success
-	}{
-		// Default derivation (no unload_api set).
-		{
-			name:    "default: standard engines path",
-			baseURL: "http://127.0.0.1:12434/engines/v1/",
-			want:    "http://127.0.0.1:12434/engines/_unload",
-		},
-		{
-			name:    "default: no trailing slash",
-			baseURL: "http://127.0.0.1:12434/engines/v1",
-			want:    "http://127.0.0.1:12434/engines/_unload",
-		},
-		{
-			name:    "default: Docker Desktop experimental prefix",
-			baseURL: "http://_/exp/vDD4.40/engines/v1",
-			want:    "http://_/exp/vDD4.40/engines/_unload",
-		},
-		{
-			name:    "default: backend-scoped path",
-			baseURL: "http://127.0.0.1:12434/engines/llama.cpp/v1/",
-			want:    "http://127.0.0.1:12434/engines/llama.cpp/_unload",
-		},
-
-		// Override paths and absolute URLs.
-		{
-			name:      "override: absolute https URL is returned verbatim",
-			baseURL:   "http://anything",
-			unloadAPI: "https://api.example.com/unload",
-			want:      "https://api.example.com/unload",
-		},
-		{
-			name:      "override: rooted path drops base path",
-			baseURL:   "http://localhost:12434/engines/v1",
-			unloadAPI: "/engines/_unload",
-			want:      "http://localhost:12434/engines/_unload",
-		},
-		{
-			name:      "override: relative path is rooted",
-			baseURL:   "http://localhost:12434/engines/v1",
-			unloadAPI: "engines/_unload",
-			want:      "http://localhost:12434/engines/_unload",
-		},
-
-		// Skip / error cases.
-		{
-			name: "skip: no base_url and no unload_api",
-			want: "",
-		},
-		{
-			name:        "error: unload_api set but base_url empty",
-			unloadAPI:   "/engines/_unload",
-			errContains: "is not absolute",
-		},
-		{
-			name:        "error: base_url without scheme",
-			baseURL:     "localhost:12434/engines/v1",
-			unloadAPI:   "/engines/_unload",
-			errContains: "is not absolute",
-		},
-	}
-
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			t.Parallel()
-			got, err := unloadURL(hooks.ModelEndpoint{
-				BaseURL:   tt.baseURL,
-				UnloadAPI: tt.unloadAPI,
-			})
-			if tt.errContains != "" {
-				require.Error(t, err)
-				assert.Contains(t, err.Error(), tt.errContains)
-				return
-			}
-			require.NoError(t, err)
-			assert.Equal(t, tt.want, got)
-		})
-	}
-}

pkg/model/provider/dmr/unload.go

@@ -0,0 +1,52 @@
+package dmr
+
+import (
+	"fmt"
+	"net/url"
+	"strings"
+)
+
+// ProviderType is the canonical [latest.ModelConfig.Provider] value
+// for Docker Model Runner. Exported so callers outside the package
+// (e.g. the `unload` hook builtin) can dispatch on provider type
+// without hard-coding the literal.
+const ProviderType = "dmr"
+
+// UnloadURL resolves the URL of the per-model unload endpoint for a
+// DMR-served model, given the resolved provider base URL and the
+// per-model `unload_api` override (both as they appear on
+// [hooks.ModelEndpoint]).
+//
+// Resolution order:
+//
+//  1. unloadAPI is an absolute URL — used verbatim (lets users point
+//     at a different host than baseURL);
+//  2. unloadAPI is set but relative — rebased onto baseURL's
+//     scheme + host (the model's path is dropped);
+//  3. unloadAPI is unset — the default `_unload` URL is derived from
+//     baseURL by replacing its trailing `/v1` segment, mirroring the
+//     `/v1` → `/_configure` convention the configure path uses.
+//
+// Returns ("", nil) when neither baseURL nor unloadAPI is set, so the
+// caller can skip without erroring (in-process / test providers).
+func UnloadURL(baseURL, unloadAPI string) (string, error) {
+	if strings.HasPrefix(unloadAPI, "http://") || strings.HasPrefix(unloadAPI, "https://") {
+		return unloadAPI, nil
+	}
+	if baseURL == "" && unloadAPI == "" {
+		return "", nil
+	}
+	u, err := url.Parse(baseURL)
+	if err != nil || u.Scheme == "" || u.Host == "" {
+		return "", fmt.Errorf("base_url %q is not absolute; cannot resolve unload endpoint", baseURL)
+	}
+	switch {
+	case unloadAPI == "":
+		u.Path = strings.TrimSuffix(strings.TrimSuffix(u.Path, "/"), "/v1") + "/_unload"
+	case strings.HasPrefix(unloadAPI, "/"):
+		u.Path = unloadAPI
+	default:
+		u.Path = "/" + unloadAPI
+	}
+	return u.String(), nil
+}