fix(observ): Address the observability review follow-ups

internal/api/ai_tools.go

@@ -400,20 +400,19 @@ func (s *Server) pluginInfos() []pluginapi.Info {
 	return s.pluginHost.Infos()
 }
 
-// pluginToolMutates reports whether a namespaced plugin tool changes state, so the caller
-// applies the write-access gate.
-func (s *Server) pluginToolMutates(plugin, tool string) bool {
+// pluginToolSpec returns the advertised spec for a namespaced plugin tool.
+func (s *Server) pluginToolSpec(plugin, tool string) (pluginapi.ToolSpec, bool) {
 	for _, info := range s.pluginInfos() {
 		if info.Name != plugin {
 			continue
 		}
 		for _, t := range info.Tools {
 			if t.Name == tool {
-				return t.Mutates
+				return t, true
 			}
 		}
 	}
-	return false
+	return pluginapi.ToolSpec{}, false
 }
 
 // runAITool executes one tool call, returning the textual result the
@@ -429,18 +428,26 @@ func (s *Server) runAITool(c *gin.Context, boundDeployment string, call ai.ToolC
 
 	// Plugin-provided tools are dispatched to the owning plugin over its socket.
 	if plugin, tool, ok := parsePluginToolName(call.Name); ok {
-		if s.pluginToolMutates(plugin, tool) {
-			// A state-changing tool requires write access to a deployment, and the plugin is
-			// told which one so it can scope the mutation to that deployment rather than act
-			// on an arbitrary resource the caller named.
-			dep, err := s.toolAllowedDeploymentWrite(c, boundDeployment, args)
-			if err != nil {
-				return "Error: " + err.Error()
-			}
-			if args == nil {
-				args = map[string]interface{}{}
+		if spec, found := s.pluginToolSpec(plugin, tool); found && spec.Mutates {
+			if spec.Global {
+				// A host-wide setting has no deployment scope; gate it on settings-write so
+				// it works in an unscoped assistant session instead of demanding a deployment.
+				if err := s.toolAllowedGlobalWrite(c); err != nil {
+					return "Error: " + err.Error()
+				}
+			} else {
+				// A state-changing tool requires write access to a deployment, and the plugin is
+				// told which one so it can scope the mutation to that deployment rather than act
+				// on an arbitrary resource the caller named.
+				dep, err := s.toolAllowedDeploymentWrite(c, boundDeployment, args)
+				if err != nil {
+					return "Error: " + err.Error()
+				}
+				if args == nil {
+					args = map[string]interface{}{}
+				}
+				args["_deployment"] = dep
 			}
-			args["_deployment"] = dep
 		}
 		result, err := s.pluginHost.ExecTool(plugin, tool, args)
 		if err != nil {
@@ -473,3 +480,14 @@ func (s *Server) toolAllowedDeploymentWrite(c *gin.Context, boundDeployment stri
 	}
 	return name, nil
 }
+
+// toolAllowedGlobalWrite gates a mutating tool that changes a host-wide setting rather than a
+// deployment. It needs settings-write; a nil actor means auth is disabled, which is allowed as
+// elsewhere.
+func (s *Server) toolAllowedGlobalWrite(c *gin.Context) error {
+	actor := auth.GetActorFromContext(c)
+	if actor != nil && !actor.HasPermission(auth.PermSettingsWrite) {
+		return fmt.Errorf("you do not have permission to change this setting")
+	}
+	return nil
+}

internal/api/notifications_test.go

@@ -0,0 +1,137 @@
+package api
+
+import (
+	"bytes"
+	"encoding/json"
+	"net/http"
+	"net/http/httptest"
+	"strings"
+	"testing"
+
+	"github.com/flatrun/agent/internal/auth"
+	"github.com/flatrun/agent/internal/contextkeys"
+	"github.com/flatrun/agent/internal/notify"
+	"github.com/gin-gonic/gin"
+)
+
+func setupNotifyTest(t *testing.T) (*Server, *gin.Engine) {
+	t.Helper()
+	gin.SetMode(gin.TestMode)
+	s := &Server{
+		notify:      notify.NewService(t.TempDir()),
+		pluginToken: "plugin-token",
+	}
+	r := gin.New()
+	r.GET("/notifications/targets", s.getNotificationTargets)
+	r.PUT("/notifications/targets", s.updateNotificationTargets)
+	r.POST("/internal/notify/emit", s.emitNotification)
+	return s, r
+}
+
+func TestGetNotificationTargetsMasksSecret(t *testing.T) {
+	s, r := setupNotifyTest(t)
+	if err := s.notify.Save(notify.Config{Targets: []notify.Target{
+		{ID: "1", Name: "email", URL: "smtp://user:secret@host:587", Enabled: true},
+	}}); err != nil {
+		t.Fatal(err)
+	}
+
+	w := httptest.NewRecorder()
+	r.ServeHTTP(w, httptest.NewRequest(http.MethodGet, "/notifications/targets", nil))
+
+	if w.Code != http.StatusOK {
+		t.Fatalf("status = %d", w.Code)
+	}
+	body := w.Body.String()
+	if strings.Contains(body, "secret") {
+		t.Errorf("response leaked the target credential: %s", body)
+	}
+	if !strings.Contains(body, notify.MaskedURL) {
+		t.Errorf("response should mask the target URL: %s", body)
+	}
+}
+
+func TestUpdateNotificationTargetsPreservesMaskedSecret(t *testing.T) {
+	s, r := setupNotifyTest(t)
+	const real = "smtp://user:secret@host:587"
+	if err := s.notify.Save(notify.Config{Targets: []notify.Target{
+		{ID: "1", Name: "email", URL: real, Enabled: true},
+	}}); err != nil {
+		t.Fatal(err)
+	}
+
+	payload, _ := json.Marshal(notify.Config{Targets: []notify.Target{
+		{ID: "1", Name: "renamed", URL: notify.MaskedURL, Enabled: true},
+	}})
+	w := httptest.NewRecorder()
+	req := httptest.NewRequest(http.MethodPut, "/notifications/targets", bytes.NewReader(payload))
+	req.Header.Set("Content-Type", "application/json")
+	r.ServeHTTP(w, req)
+
+	if w.Code != http.StatusOK {
+		t.Fatalf("status = %d, body = %s", w.Code, w.Body.String())
+	}
+	stored := s.notify.Load()
+	if len(stored.Targets) != 1 || stored.Targets[0].URL != real {
+		t.Errorf("saving a masked URL must keep the stored secret, got %+v", stored.Targets)
+	}
+	if stored.Targets[0].Name != "renamed" {
+		t.Errorf("name change should apply, got %q", stored.Targets[0].Name)
+	}
+}
+
+func TestEmitNotificationRequiresPluginToken(t *testing.T) {
+	_, r := setupNotifyTest(t)
+	body := func() *bytes.Reader { return bytes.NewReader([]byte(`{"title":"t","message":"m"}`)) }
+
+	cases := []struct {
+		name   string
+		token  string
+		status int
+	}{
+		{"no token", "", http.StatusUnauthorized},
+		{"wrong token", "nope", http.StatusUnauthorized},
+		{"correct token", "plugin-token", http.StatusAccepted},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			w := httptest.NewRecorder()
+			req := httptest.NewRequest(http.MethodPost, "/internal/notify/emit", body())
+			req.Header.Set("Content-Type", "application/json")
+			if tc.token != "" {
+				req.Header.Set("X-Plugin-Token", tc.token)
+			}
+			r.ServeHTTP(w, req)
+			if w.Code != tc.status {
+				t.Errorf("status = %d, want %d", w.Code, tc.status)
+			}
+		})
+	}
+}
+
+func TestToolAllowedGlobalWrite(t *testing.T) {
+	s := &Server{}
+	ctx := func(actor *auth.ActorContext) *gin.Context {
+		c, _ := gin.CreateTestContext(httptest.NewRecorder())
+		if actor != nil {
+			c.Set(contextkeys.Actor, actor)
+		}
+		return c
+	}
+
+	if err := s.toolAllowedGlobalWrite(ctx(&auth.ActorContext{Role: auth.RoleAdmin})); err != nil {
+		t.Errorf("admin should be allowed: %v", err)
+	}
+	if err := s.toolAllowedGlobalWrite(ctx(&auth.ActorContext{
+		Role:        auth.RoleViewer,
+		Permissions: []string{string(auth.PermSettingsWrite)},
+	})); err != nil {
+		t.Errorf("actor with settings-write should be allowed: %v", err)
+	}
+	if err := s.toolAllowedGlobalWrite(ctx(&auth.ActorContext{Role: auth.RoleViewer})); err == nil {
+		t.Error("actor without settings-write should be denied")
+	}
+	if err := s.toolAllowedGlobalWrite(ctx(nil)); err != nil {
+		t.Errorf("nil actor (auth disabled) should be allowed: %v", err)
+	}
+}

internal/api/server.go

@@ -3106,7 +3106,7 @@ func (s *Server) updateNotificationTargets(c *gin.Context) {
 		c.JSON(http.StatusBadRequest, gin.H{"error": "invalid request: " + err.Error()})
 		return
 	}
-	if err := s.notify.Save(cfg); err != nil {
+	if err := s.notify.Update(cfg); err != nil {
 		c.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()})
 		return
 	}

internal/notify/notify.go

@@ -5,6 +5,7 @@
 package notify
 
 import (
+	"encoding/json"
 	"fmt"
 	"os"
 	"path/filepath"
@@ -14,6 +15,11 @@ import (
 	"gopkg.in/yaml.v3"
 )
 
+// MaskedURL stands in for a target URL in API responses. A shoutrrr URL carries
+// its credentials inline (an SMTP password, a webhook token), so the real URL is
+// never returned to a client; it is written verbatim only to the on-disk store.
+const MaskedURL = "********"
+
 // Target is one delivery destination. URL is a shoutrrr service URL, e.g.
 // "smtp://user:pass@host:587/?from=x&to=y" or "generic+https://example.com/hook".
 type Target struct {
@@ -23,6 +29,17 @@ type Target struct {
 	Enabled bool   `yaml:"enabled" json:"enabled"`
 }
 
+// MarshalJSON masks the credential-bearing URL. YAML persistence does not use
+// this path, so the stored file keeps the real URL.
+func (t Target) MarshalJSON() ([]byte, error) {
+	type alias Target
+	masked := alias(t)
+	if masked.URL != "" {
+		masked.URL = MaskedURL
+	}
+	return json.Marshal(masked)
+}
+
 // Config is the persisted notification settings.
 type Config struct {
 	Targets []Target `yaml:"targets" json:"targets"`
@@ -57,14 +74,31 @@ func (s *Service) Load() Config {
 func (s *Service) Save(cfg Config) error {
 	s.mu.Lock()
 	defer s.mu.Unlock()
-	if err := os.MkdirAll(filepath.Dir(s.path), 0755); err != nil {
+	if err := os.MkdirAll(filepath.Dir(s.path), 0700); err != nil {
 		return err
 	}
 	data, err := yaml.Marshal(cfg)
 	if err != nil {
 		return err
 	}
-	return os.WriteFile(s.path, data, 0644)
+	return os.WriteFile(s.path, data, 0600)
+}
+
+// Update saves targets, restoring the stored URL for any target whose incoming
+// URL is the mask: a client that received a masked target and saved it back
+// unchanged must not overwrite the real URL with the mask.
+func (s *Service) Update(cfg Config) error {
+	stored := s.Load()
+	byID := make(map[string]string, len(stored.Targets))
+	for _, t := range stored.Targets {
+		byID[t.ID] = t.URL
+	}
+	for i := range cfg.Targets {
+		if cfg.Targets[i].URL == MaskedURL {
+			cfg.Targets[i].URL = byID[cfg.Targets[i].ID]
+		}
+	}
+	return s.Save(cfg)
 }
 
 // Test sends a message to a single URL, so an admin can verify a target before saving it.

internal/notify/notify_test.go

@@ -1,10 +1,47 @@
 package notify
 
 import (
+	"encoding/json"
 	"errors"
+	"strings"
 	"testing"
 )
 
+func TestTargetJSONMasksURL(t *testing.T) {
+	b, err := json.Marshal(Target{ID: "1", Name: "email", URL: "smtp://user:secret@host:587", Enabled: true})
+	if err != nil {
+		t.Fatal(err)
+	}
+	out := string(b)
+	if strings.Contains(out, "secret") {
+		t.Errorf("credential leaked in JSON: %s", out)
+	}
+	if !strings.Contains(out, MaskedURL) {
+		t.Errorf("URL should be masked: %s", out)
+	}
+}
+
+func TestUpdatePreservesMaskedURL(t *testing.T) {
+	s := NewService(t.TempDir())
+	const real = "smtp://user:secret@host:587"
+	if err := s.Save(Config{Targets: []Target{{ID: "1", Name: "email", URL: real, Enabled: true}}}); err != nil {
+		t.Fatal(err)
+	}
+
+	// A client that received a masked URL saves it back with only a name change.
+	if err := s.Update(Config{Targets: []Target{{ID: "1", Name: "renamed", URL: MaskedURL, Enabled: true}}}); err != nil {
+		t.Fatal(err)
+	}
+
+	got := s.Load()
+	if len(got.Targets) != 1 || got.Targets[0].URL != real {
+		t.Errorf("masked update must keep the stored URL, got %+v", got.Targets)
+	}
+	if got.Targets[0].Name != "renamed" {
+		t.Errorf("non-secret changes should still apply, got name %q", got.Targets[0].Name)
+	}
+}
+
 func TestServiceRoundTripAndNotify(t *testing.T) {
 	s := NewService(t.TempDir())