Merge branch 'main' of https://github.com/sipeed/picoclaw

pkg/config/config.go

@@ -106,6 +106,7 @@ func (c *Config) WithSecurity(sec *SecurityConfig) *Config {
 		c.security = sec
 		return c
 	}
+	sec = normalizeSecurityConfig(sec)
 	err := applySecurityConfig(c, sec)
 	if err != nil {
 		return nil
@@ -1845,6 +1846,7 @@ func SaveConfig(path string, cfg *Config) error {
 		logger.ErrorC("config", "security is nil")
 		return fmt.Errorf("security is nil")
 	}
+	cfg.security = normalizeSecurityConfig(cfg.security)
 	// Ensure version is always set when saving
 	if cfg.Version == 0 {
 		cfg.Version = CurrentVersion
@@ -2023,6 +2025,7 @@ func SaveConfig(path string, cfg *Config) error {
 	if err != nil {
 		return err
 	}
+	logger.Infof("saving config to %s", path)
 	return fileutil.WriteFileAtomic(path, data, 0o600)
 }
 
@@ -2086,6 +2089,11 @@ func (c *Config) ValidateModelList() error {
 
 func (c *Config) SecurityCopyFrom(cfg *Config) {
 	c.security = cfg.security
+	if c.security != nil {
+		if err := applySecurityConfig(c, c.security); err != nil {
+			logger.Errorf("failed to apply security config in SecurityCopyFrom: %v", err)
+		}
+	}
 }
 
 func MergeAPIKeys(apiKey string, apiKeys []string) []string {

pkg/config/security.go

@@ -25,13 +25,32 @@ const (
 	SecurityConfigFile = ".security.yml"
 )
 
+func normalizeSecurityConfig(sec *SecurityConfig) *SecurityConfig {
+	if sec == nil {
+		sec = &SecurityConfig{}
+	}
+	if sec.ModelList == nil {
+		sec.ModelList = map[string]ModelSecurityEntry{}
+	}
+	if sec.Channels == nil {
+		sec.Channels = &ChannelsSecurity{}
+	}
+	if sec.Web == nil {
+		sec.Web = &WebToolsSecurity{}
+	}
+	if sec.Skills == nil {
+		sec.Skills = &SkillsSecurity{}
+	}
+	return sec
+}
+
 // SecurityConfig stores all sensitive data (API keys, tokens, secrets, passwords)
 // This data is loaded from security.yml and kept separate from the main config
 type SecurityConfig struct {
 	// Model API keys. Map key is model_name, can include suffix like "abc:0", "abc:1"
 	// for load balancing with same model_name. The suffix ":N" is used to distinguish
 	// multiple configs that share the same base model_name.
-	ModelList map[string]ModelSecurityEntry `yaml:"model_list,omitempty"`
+	ModelList map[string]ModelSecurityEntry `yaml:"model_list"`
 
 	// Channel tokens/secrets
 	Channels *ChannelsSecurity `yaml:"channels,omitempty"`
@@ -191,7 +210,7 @@ func loadSecurityConfig(securityPath string) (*SecurityConfig, error) {
 	data, err := os.ReadFile(securityPath)
 	if err != nil {
 		if os.IsNotExist(err) {
-			return &SecurityConfig{}, nil
+			return normalizeSecurityConfig(nil), nil
 		}
 		return nil, fmt.Errorf("failed to read security config: %w", err)
 	}
@@ -210,7 +229,7 @@ func loadSecurityConfig(securityPath string) (*SecurityConfig, error) {
 		return nil, err
 	}
 
-	return &sec, nil
+	return normalizeSecurityConfig(&sec), nil
 }
 
 // saveSecurityConfig saves the security configuration to security.yml

pkg/config/security_integration_test.go

@@ -17,13 +17,12 @@ import (
 
 // Test JSON unmarshal of private fields
 func TestJSONUnmarshalPrivateFields(t *testing.T) {
-	//nolint: govet
 	type testStruct struct {
 		PublicField  string `json:"public"`
-		privateField string `json:"private"`
+		privateField string
 	}
 
-	data := `{"public": "pub", "private": "priv"}`
+	data := `{"public": "pub", "privateField": "priv"}`
 	var s testStruct
 	if err := json.Unmarshal([]byte(data), &s); err != nil {
 		t.Fatalf("JSON unmarshal failed: %v", err)
@@ -35,9 +34,8 @@ func TestJSONUnmarshalPrivateFields(t *testing.T) {
 	if s.PublicField != "pub" {
 		t.Errorf("PublicField = %q, want 'pub'", s.PublicField)
 	}
-	// This should fail because privateField is unexported
+	if s.privateField != "" {
-	if s.privateField != "priv" {
+		t.Errorf("privateField = %q, want empty because unexported fields are ignored", s.privateField)
-		t.Logf("privateField = %q, want 'priv' - THIS IS EXPECTED TO FAIL", s.privateField)
 	}
 }
 

pkg/config/security_test.go

@@ -20,6 +20,9 @@ func TestSecurityConfig(t *testing.T) {
 		require.NoError(t, err)
 		assert.NotNil(t, sec)
 		assert.Empty(t, sec.ModelList)
+		assert.NotNil(t, sec.Channels)
+		assert.NotNil(t, sec.Web)
+		assert.NotNil(t, sec.Skills)
 	})
 }
 

web/backend/api/config.go

@@ -54,6 +54,15 @@ func (h *Handler) handleUpdateConfig(w http.ResponseWriter, r *http.Request) {
 		cfg.Tools.Exec.AllowRemote = config.DefaultConfig().Tools.Exec.AllowRemote
 	}
 
+	// Load existing config and copy security credentials before validation,
+	// so that security-managed fields (e.g. pico token) are available.
+	oldCfg, err := config.LoadConfig(h.configPath)
+	if err != nil {
+		http.Error(w, fmt.Sprintf("Failed to load config: %v", err), http.StatusInternalServerError)
+		return
+	}
+	cfg.SecurityCopyFrom(oldCfg)
+
 	if errs := validateConfig(&cfg); len(errs) > 0 {
 		w.Header().Set("Content-Type", "application/json")
 		w.WriteHeader(http.StatusBadRequest)
@@ -64,13 +73,7 @@ func (h *Handler) handleUpdateConfig(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	logger.Infof("new config: %+v", cfg)
+	logger.Infof("configuration updated successfully")
-	oldCfg, err := config.LoadConfig(h.configPath)
-	if err != nil {
-		http.Error(w, fmt.Sprintf("Failed to load config: %v", err), http.StatusInternalServerError)
-		return
-	}
-	cfg.SecurityCopyFrom(oldCfg)
 
 	if err := config.SaveConfig(h.configPath, &cfg); err != nil {
 		http.Error(w, fmt.Sprintf("Failed to save config: %v", err), http.StatusInternalServerError)
@@ -149,6 +152,10 @@ func (h *Handler) handlePatchConfig(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
+	// Copy security credentials before validation so security-managed
+	// fields (e.g. pico token) are available for validation checks.
+	newCfg.SecurityCopyFrom(cfg)
+
 	if errs := validateConfig(&newCfg); len(errs) > 0 {
 		w.Header().Set("Content-Type", "application/json")
 		w.WriteHeader(http.StatusBadRequest)
@@ -159,8 +166,6 @@ func (h *Handler) handlePatchConfig(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	newCfg.SecurityCopyFrom(cfg)
-
 	if err := config.SaveConfig(h.configPath, &newCfg); err != nil {
 		http.Error(w, fmt.Sprintf("Failed to save config: %v", err), http.StatusInternalServerError)
 		return

web/backend/api/config_test.go

@@ -4,6 +4,8 @@ import (
 	"bytes"
 	"net/http"
 	"net/http/httptest"
+	"os"
+	"path/filepath"
 	"testing"
 
 	"github.com/sipeed/picoclaw/pkg/config"
@@ -141,6 +143,120 @@ func TestHandlePatchConfig_AllowsInvalidExecRegexPatternsWhenExecDisabled(t *tes
 	}
 }
 
+// setupPicoEnabledEnv creates a test environment with Pico channel enabled and
+// its token stored only in .security.yml (not in the JSON payload).
+func setupPicoEnabledEnv(t *testing.T) (string, func()) {
+	t.Helper()
+
+	tmp := t.TempDir()
+	oldHome := os.Getenv("HOME")
+	oldPicoHome := os.Getenv("PICOCLAW_HOME")
+
+	if err := os.Setenv("HOME", tmp); err != nil {
+		t.Fatalf("set HOME: %v", err)
+	}
+	if err := os.Setenv("PICOCLAW_HOME", filepath.Join(tmp, ".picoclaw")); err != nil {
+		t.Fatalf("set PICOCLAW_HOME: %v", err)
+	}
+
+	cfg := config.DefaultConfig()
+	cfg.ModelList = []*config.ModelConfig{{
+		ModelName: "custom-default",
+		Model:     "openai/gpt-4o",
+	}}
+	cfg.Agents.Defaults.ModelName = "custom-default"
+	cfg.Channels.Pico.Enabled = true
+	cfg.WithSecurity(&config.SecurityConfig{
+		ModelList: map[string]config.ModelSecurityEntry{
+			"custom-default": {APIKeys: []string{"sk-default"}},
+		},
+		Channels: &config.ChannelsSecurity{
+			Pico: &config.PicoSecurity{Token: "test-pico-token"},
+		},
+	})
+
+	configPath := filepath.Join(tmp, "config.json")
+	if err := config.SaveConfig(configPath, cfg); err != nil {
+		t.Fatalf("SaveConfig error: %v", err)
+	}
+
+	cleanup := func() {
+		_ = os.Setenv("HOME", oldHome)
+		if oldPicoHome == "" {
+			_ = os.Unsetenv("PICOCLAW_HOME")
+		} else {
+			_ = os.Setenv("PICOCLAW_HOME", oldPicoHome)
+		}
+	}
+	return configPath, cleanup
+}
+
+func TestHandleUpdateConfig_SucceedsWhenPicoTokenInSecurityOnly(t *testing.T) {
+	configPath, cleanup := setupPicoEnabledEnv(t)
+	defer cleanup()
+
+	h := NewHandler(configPath)
+	mux := http.NewServeMux()
+	h.RegisterRoutes(mux)
+
+	// PUT request with pico enabled but no token in JSON — token is in .security.yml
+	req := httptest.NewRequest(http.MethodPut, "/api/config", bytes.NewBufferString(`{
+		"version": 1,
+		"agents": {
+			"defaults": {
+				"workspace": "~/.picoclaw/workspace",
+				"model_name": "custom-default"
+			}
+		},
+		"channels": {
+			"pico": {
+				"enabled": true,
+				"ping_interval": 30,
+				"read_timeout": 60,
+				"write_timeout": 10,
+				"max_connections": 100
+			}
+		},
+		"model_list": [
+			{
+				"model_name": "custom-default",
+				"model": "openai/gpt-4o",
+				"api_keys": ["sk-default"]
+			}
+		]
+	}`))
+	req.Header.Set("Content-Type", "application/json")
+
+	rec := httptest.NewRecorder()
+	mux.ServeHTTP(rec, req)
+	if rec.Code != http.StatusOK {
+		t.Fatalf("PUT /api/config status = %d, want %d, body=%s", rec.Code, http.StatusOK, rec.Body.String())
+	}
+}
+
+func TestHandlePatchConfig_SucceedsWhenPicoTokenInSecurityOnly(t *testing.T) {
+	configPath, cleanup := setupPicoEnabledEnv(t)
+	defer cleanup()
+
+	h := NewHandler(configPath)
+	mux := http.NewServeMux()
+	h.RegisterRoutes(mux)
+
+	// PATCH request changing an unrelated field — pico token still in .security.yml
+	req := httptest.NewRequest(http.MethodPatch, "/api/config", bytes.NewBufferString(`{
+		"gateway": {
+			"log_level": "info"
+		}
+	}`))
+	req.Header.Set("Content-Type", "application/json")
+
+	rec := httptest.NewRecorder()
+	mux.ServeHTTP(rec, req)
+	if rec.Code != http.StatusOK {
+		t.Fatalf("PATCH /api/config status = %d, want %d, body=%s", rec.Code, http.StatusOK, rec.Body.String())
+	}
+}
+
 func TestHandlePatchConfig_AllowsInvalidDenyRegexPatternsWhenDenyPatternsDisabled(t *testing.T) {
 	configPath, cleanup := setupOAuthTestEnv(t)
 	defer cleanup()

web/backend/api/models.go

@@ -108,7 +108,12 @@ func (h *Handler) handleAddModel(w http.ResponseWriter, r *http.Request) {
 	}
 	defer r.Body.Close()
 
-	var mc config.ModelConfig
+	type custom struct {
+		config.ModelConfig
+		APIKey string `json:"api_key"`
+	}
+
+	var mc custom
 	if err = json.Unmarshal(body, &mc); err != nil {
 		http.Error(w, fmt.Sprintf("Invalid JSON: %v", err), http.StatusBadRequest)
 		return
@@ -119,13 +124,17 @@ func (h *Handler) handleAddModel(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
+	if mc.APIKey != "" {
+		mc.ModelConfig.SetAPIKey(mc.APIKey)
+	}
+
 	cfg, err := config.LoadConfig(h.configPath)
 	if err != nil {
 		http.Error(w, fmt.Sprintf("Failed to load config: %v", err), http.StatusInternalServerError)
 		return
 	}
 
-	cfg.ModelList = append(cfg.ModelList, &mc)
+	cfg.ModelList = append(cfg.ModelList, &mc.ModelConfig)
 
 	if err := config.SaveConfig(h.configPath, cfg); err != nil {
 		http.Error(w, fmt.Sprintf("Failed to save config: %v", err), http.StatusInternalServerError)
@@ -307,16 +316,25 @@ func (h *Handler) handleSetDefaultModel(w http.ResponseWriter, r *http.Request)
 }
 
 // maskAPIKey returns a masked version of an API key for safe display.
-// Keys longer than 8 chars show prefix + last 4 chars: "sk-****abcd"
+// Keys longer than 12 chars show prefix + last 4 chars: "sk-****abcd".
+// Keys 9-12 chars show prefix + last 2 chars: "sk-****cd".
 // Shorter keys are fully masked as "****".
 // Empty keys return empty string.
+// Ensure at least 40% of the key will not be displayed.
 func maskAPIKey(key string) string {
 	if key == "" {
 		return ""
 	}
+
 	if len(key) <= 8 {
 		return "****"
 	}
+
+	// Show first 3 chars and last 2 chars
+	if len(key) <= 12 {
+		return key[:3] + "****" + key[len(key)-2:]
+	}
+
 	// Show first 3 chars and last 4 chars
 	return key[:3] + "****" + key[len(key)-4:]
 }

web/backend/api/models_test.go

@@ -1,9 +1,11 @@
 package api
 
 import (
+	"bytes"
 	"encoding/json"
 	"net/http"
 	"net/http/httptest"
+	"strings"
 	"sync"
 	"testing"
 	"time"
@@ -315,3 +317,112 @@ func TestHandleListModels_NormalizesWildcardLocalAPIBaseForProbe(t *testing.T) {
 		t.Fatalf("probe api base = %q, want %q", gotProbe, "http://127.0.0.1:8000/v1|custom-model|")
 	}
 }
+
+func TestHandleAddModel_PersistsAPIKey(t *testing.T) {
+	configPath, cleanup := setupOAuthTestEnv(t)
+	defer cleanup()
+
+	h := NewHandler(configPath)
+	mux := http.NewServeMux()
+	h.RegisterRoutes(mux)
+
+	rec := httptest.NewRecorder()
+	req := httptest.NewRequest(http.MethodPost, "/api/models", bytes.NewBufferString(`{
+		"model_name":"new-model",
+		"model":"openai/gpt-4o-mini",
+		"api_key":"sk-new-model-key"
+	}`))
+	req.Header.Set("Content-Type", "application/json")
+	mux.ServeHTTP(rec, req)
+
+	if rec.Code != http.StatusOK {
+		t.Fatalf("status = %d, want %d, body=%s", rec.Code, http.StatusOK, rec.Body.String())
+	}
+
+	cfg, err := config.LoadConfig(configPath)
+	if err != nil {
+		t.Fatalf("LoadConfig() error = %v", err)
+	}
+	if len(cfg.ModelList) != 2 {
+		t.Fatalf("len(model_list) = %d, want 2", len(cfg.ModelList))
+	}
+
+	added := cfg.ModelList[1]
+	if added.ModelName != "new-model" {
+		t.Fatalf("model_name = %q, want %q", added.ModelName, "new-model")
+	}
+	if added.APIKey() != "sk-new-model-key" {
+		t.Fatalf("api_key = %q, want %q", added.APIKey(), "sk-new-model-key")
+	}
+}
+
+func TestMaskAPIKey(t *testing.T) {
+	tests := []struct {
+		name string
+		key  string
+		want string
+	}{
+		{
+			name: "empty key",
+			key:  "",
+			want: "",
+		},
+		{
+			name: "short key fully masked",
+			key:  "abcd",
+			want: "****",
+		},
+		{
+			name: "length 8 boundary fully masked",
+			key:  "12345678",
+			want: "****",
+		},
+		{
+			name: "length 9 boundary shows last 2",
+			key:  "123456789",
+			want: "123****89",
+		},
+		{
+			name: "length 12 boundary shows last 2",
+			key:  "abcdefghijkl",
+			want: "abc****kl",
+		},
+		{
+			name: "length 13 boundary shows last 4",
+			key:  "abcdefghijklm",
+			want: "abc****jklm",
+		},
+		{
+			name: "typical api key",
+			key:  "sk-1234567890abcd",
+			want: "sk-****abcd",
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			got := maskAPIKey(tc.key)
+			if got != tc.want {
+				t.Fatalf("maskAPIKey(%q) = %q, want %q", tc.key, got, tc.want)
+			}
+
+			if tc.key != "" {
+				displayed := strings.Replace(tc.want, "****", "", 1)
+				if len(tc.key) <= 8 {
+					if displayed != "" {
+						t.Fatalf("maskAPIKey(%q) displayed part = %q, want empty", tc.key, displayed)
+					}
+				} else {
+					if len(displayed)*10 > len(tc.key)*6 {
+						t.Fatalf(
+							"maskAPIKey(%q) displayed length = %d, want at most 60%% of %d",
+							tc.key,
+							len(displayed),
+							len(tc.key),
+						)
+					}
+				}
+			}
+		})
+	}
+}

web/backend/api/pico_test.go

@@ -6,6 +6,7 @@ import (
 	"net/http"
 	"net/http/httptest"
 	"net/url"
+	"os"
 	"path/filepath"
 	"strconv"
 	"testing"
@@ -154,6 +155,44 @@ func TestEnsurePicoChannel_PreservesUserSettings(t *testing.T) {
 	}
 }
 
+func TestEnsurePicoChannel_ExistingConfigWithoutSecurityFile(t *testing.T) {
+	configPath := filepath.Join(t.TempDir(), "config.json")
+
+	cfg := config.DefaultConfig()
+	raw, err := json.Marshal(cfg)
+	if err != nil {
+		t.Fatalf("Marshal() error = %v", err)
+	}
+	if err = os.WriteFile(configPath, raw, 0o600); err != nil {
+		t.Fatalf("WriteFile() error = %v", err)
+	}
+
+	h := NewHandler(configPath)
+
+	changed, err := h.ensurePicoChannel("")
+	if err != nil {
+		t.Fatalf("ensurePicoChannel() error = %v", err)
+	}
+	if !changed {
+		t.Fatal("ensurePicoChannel() should report changed when pico is missing")
+	}
+
+	cfg, err = config.LoadConfig(configPath)
+	if err != nil {
+		t.Fatalf("LoadConfig() error = %v", err)
+	}
+
+	if !cfg.Channels.Pico.Enabled {
+		t.Error("expected Pico to be enabled after setup")
+	}
+	if cfg.Channels.Pico.Token() == "" {
+		t.Error("expected a non-empty token after setup")
+	}
+	if _, err := os.Stat(filepath.Join(filepath.Dir(configPath), config.SecurityConfigFile)); err != nil {
+		t.Fatalf("expected .security.yml to be created: %v", err)
+	}
+}
+
 func TestEnsurePicoChannel_Idempotent(t *testing.T) {
 	configPath := filepath.Join(t.TempDir(), "config.json")
 	h := NewHandler(configPath)