From ef002d9a5d5e39456004040dcf61fdbe54706535 Mon Sep 17 00:00:00 2001 From: SiYue-ZO <2835601846@qq.com> Date: Thu, 11 Jun 2026 16:00:18 +0800 Subject: [PATCH] fix: ensure dm_scope and dimensions stay in sync across all config paths The reviewer identified two bugs in the original PR: 1. PATCH /api/config leaves session.dimensions stale: LoadConfig() derives dimensions from the old dm_scope, and the merge carries those stale dimensions forward. ApplyDmScope() then exits early because dimensions is already populated, causing a mismatch between dm_scope (new) and dimensions (old). 2. Legacy/default configs omit dm_scope in GET response: configs with explicit dimensions but no dm_scope (including DefaultConfig) return no dm_scope field, causing the frontend to fall back to its default ('per-channel-peer'), which may not match the actual dimensions. Fix: - Add DeriveDmScope() to reverse-map known dimensions arrays to dm_scope when dm_scope is empty. - Call it in LoadConfig(), PUT handler, PATCH handler, and ResetToDefaults() for consistent normalization. - In PATCH handler, clear stale dimensions from the merge result when the patch contains session.dm_scope but not session.dimensions, allowing ApplyDmScope() to re-derive from the new scope. - Add comprehensive unit tests for DeriveDmScope() and scope transition scenarios. --- pkg/config/config.go | 24 +++++++++ pkg/config/config_test.go | 101 ++++++++++++++++++++++++++++++++++++++ web/backend/api/config.go | 16 ++++++ 3 files changed, 141 insertions(+) diff --git a/pkg/config/config.go b/pkg/config/config.go index 20cdd74c2..655e39aa3 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -7,6 +7,7 @@ import ( "math/rand" "os" "path/filepath" + "slices" "strconv" "strings" "sync/atomic" @@ -372,6 +373,26 @@ func (s *SessionConfig) ApplyDmScope() { } } +// DeriveDmScope sets DmScope based on Dimensions when DmScope is empty. +// This handles legacy/fresh configs that only have explicit Dimensions +// without a corresponding DmScope value, ensuring the API response always +// includes a dm_scope that matches the actual runtime dimensions. +func (s *SessionConfig) DeriveDmScope() { + if s.DmScope != "" || len(s.Dimensions) == 0 { + return + } + switch { + case slices.Equal(s.Dimensions, []string{"chat", "sender"}): + s.DmScope = "per-channel-peer" + case slices.Equal(s.Dimensions, []string{"chat"}): + s.DmScope = "per-channel" + case slices.Equal(s.Dimensions, []string{"sender"}): + s.DmScope = "per-peer" + } + // Dimensions not matching any known scope mapping (custom array) + // is fine — DmScope stays empty and the UI can handle it. +} + // RoutingConfig controls the intelligent model routing feature. // When enabled, each incoming message is scored against structural features // (message length, code blocks, tool call history, conversation depth, attachments). @@ -1498,6 +1519,7 @@ func LoadConfig(path string) (*Config, error) { } cfg.Session.ApplyDmScope() + cfg.Session.DeriveDmScope() return cfg, nil } @@ -1720,6 +1742,8 @@ func ResetToDefaults(configPath string) error { return fmt.Errorf("backup before reset: %w", err) } cfg := DefaultConfig() + cfg.Session.ApplyDmScope() + cfg.Session.DeriveDmScope() if err := cfg.SecurityCopyFrom(configPath); err != nil { logger.WarnF("could not preserve security config", map[string]any{"error": err}) } diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go index 12cd70734..52f8f4782 100644 --- a/pkg/config/config_test.go +++ b/pkg/config/config_test.go @@ -1787,6 +1787,107 @@ func TestSessionConfig_ApplyDmScope(t *testing.T) { } } +func TestSessionConfig_DeriveDmScope(t *testing.T) { + tests := []struct { + name string + dimensions []string + dmScope string + wantScope string + }{ + { + name: "per-channel-peer from dimensions", + dimensions: []string{"chat", "sender"}, + wantScope: "per-channel-peer", + }, + { + name: "per-channel from dimensions", + dimensions: []string{"chat"}, + wantScope: "per-channel", + }, + { + name: "per-peer from dimensions", + dimensions: []string{"sender"}, + wantScope: "per-peer", + }, + { + name: "custom dimensions does not set scope", + dimensions: []string{"chat", "extra"}, + wantScope: "", + }, + { + name: "empty dimensions does not set scope", + dimensions: nil, + wantScope: "", + }, + { + name: "existing dm_scope is not overwritten", + dimensions: []string{"chat", "sender"}, + dmScope: "per-channel", + wantScope: "per-channel", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + s := &SessionConfig{ + DmScope: tt.dmScope, + Dimensions: tt.dimensions, + } + s.DeriveDmScope() + if s.DmScope != tt.wantScope { + t.Errorf("DmScope = %q, want %q", s.DmScope, tt.wantScope) + } + }) + } +} + +func TestSessionConfig_ApplyDmScope_ClearsStaleDimensions(t *testing.T) { + // Simulates the PATCH handler scenario: dm_scope changed but stale + // dimensions remain from the old scope. After clearing dimensions, + // ApplyDmScope should re-derive from the new dm_scope. + tests := []struct { + name string + dmScope string + want []string + }{ + { + name: "per-channel-peer to per-channel", + dmScope: "per-channel", + want: []string{"chat"}, + }, + { + name: "per-channel-peer to per-peer", + dmScope: "per-peer", + want: []string{"sender"}, + }, + { + name: "per-channel-peer to global", + dmScope: "global", + want: nil, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + s := &SessionConfig{ + DmScope: tt.dmScope, + Dimensions: []string{"chat", "sender"}, // stale from per-channel-peer + } + // Simulate what the PATCH handler does: clear dimensions when dm_scope changes + s.Dimensions = nil + s.ApplyDmScope() + if len(s.Dimensions) != len(tt.want) { + t.Fatalf("Dimensions = %v, want %v", s.Dimensions, tt.want) + } + for i, v := range tt.want { + if s.Dimensions[i] != v { + t.Errorf("Dimensions[%d] = %q, want %q", i, s.Dimensions[i], v) + } + } + }) + } +} + func TestDefaultConfig_WorkspacePath_Default(t *testing.T) { t.Setenv("PICOCLAW_HOME", "") diff --git a/web/backend/api/config.go b/web/backend/api/config.go index 50d9b0293..f567fe524 100644 --- a/web/backend/api/config.go +++ b/web/backend/api/config.go @@ -77,6 +77,7 @@ func (h *Handler) handleUpdateConfig(w http.ResponseWriter, r *http.Request) { return } cfg.Session.ApplyDmScope() + cfg.Session.DeriveDmScope() if execAllowRemoteOmitted(body) { cfg.Tools.Exec.AllowRemote = config.DefaultConfig().Tools.Exec.AllowRemote } @@ -165,6 +166,20 @@ func (h *Handler) handlePatchConfig(w http.ResponseWriter, r *http.Request) { // Recursively merge patch into base mergeMap(base, patch) + + // When the patch updates dm_scope, the old derived dimensions from the + // base must be cleared so that ApplyDmScope() can re-derive them from + // the new dm_scope value. Otherwise the stale dimensions survive the + // merge and ApplyDmScope() exits early due to its precedence guard. + if sess, ok := base["session"].(map[string]any); ok { + if patchSess, patchHasSession := patch["session"].(map[string]any); patchHasSession { + if _, hasDmScope := patchSess["dm_scope"]; hasDmScope { + if _, hasDimsInPatch := patchSess["dimensions"]; !hasDimsInPatch { + delete(sess, "dimensions") + } + } + } + } if err = normalizeChannelArrayFields(base); err != nil { http.Error(w, fmt.Sprintf("Invalid channel array field: %v", err), http.StatusBadRequest) return @@ -183,6 +198,7 @@ func (h *Handler) handlePatchConfig(w http.ResponseWriter, r *http.Request) { return } newCfg.Session.ApplyDmScope() + newCfg.Session.DeriveDmScope() // Restore security fields (tokens/keys) from the loaded config before validation, // because private fields are lost during JSON round-trip.