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.
This commit is contained in:
parent
921d753cc0
commit
ef002d9a5d
3 changed files with 141 additions and 0 deletions
|
|
@ -7,6 +7,7 @@ import (
|
||||||
"math/rand"
|
"math/rand"
|
||||||
"os"
|
"os"
|
||||||
"path/filepath"
|
"path/filepath"
|
||||||
|
"slices"
|
||||||
"strconv"
|
"strconv"
|
||||||
"strings"
|
"strings"
|
||||||
"sync/atomic"
|
"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.
|
// RoutingConfig controls the intelligent model routing feature.
|
||||||
// When enabled, each incoming message is scored against structural features
|
// When enabled, each incoming message is scored against structural features
|
||||||
// (message length, code blocks, tool call history, conversation depth, attachments).
|
// (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.ApplyDmScope()
|
||||||
|
cfg.Session.DeriveDmScope()
|
||||||
|
|
||||||
return cfg, nil
|
return cfg, nil
|
||||||
}
|
}
|
||||||
|
|
@ -1720,6 +1742,8 @@ func ResetToDefaults(configPath string) error {
|
||||||
return fmt.Errorf("backup before reset: %w", err)
|
return fmt.Errorf("backup before reset: %w", err)
|
||||||
}
|
}
|
||||||
cfg := DefaultConfig()
|
cfg := DefaultConfig()
|
||||||
|
cfg.Session.ApplyDmScope()
|
||||||
|
cfg.Session.DeriveDmScope()
|
||||||
if err := cfg.SecurityCopyFrom(configPath); err != nil {
|
if err := cfg.SecurityCopyFrom(configPath); err != nil {
|
||||||
logger.WarnF("could not preserve security config", map[string]any{"error": err})
|
logger.WarnF("could not preserve security config", map[string]any{"error": err})
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -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) {
|
func TestDefaultConfig_WorkspacePath_Default(t *testing.T) {
|
||||||
t.Setenv("PICOCLAW_HOME", "")
|
t.Setenv("PICOCLAW_HOME", "")
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -77,6 +77,7 @@ func (h *Handler) handleUpdateConfig(w http.ResponseWriter, r *http.Request) {
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
cfg.Session.ApplyDmScope()
|
cfg.Session.ApplyDmScope()
|
||||||
|
cfg.Session.DeriveDmScope()
|
||||||
if execAllowRemoteOmitted(body) {
|
if execAllowRemoteOmitted(body) {
|
||||||
cfg.Tools.Exec.AllowRemote = config.DefaultConfig().Tools.Exec.AllowRemote
|
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
|
// Recursively merge patch into base
|
||||||
mergeMap(base, patch)
|
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 {
|
if err = normalizeChannelArrayFields(base); err != nil {
|
||||||
http.Error(w, fmt.Sprintf("Invalid channel array field: %v", err), http.StatusBadRequest)
|
http.Error(w, fmt.Sprintf("Invalid channel array field: %v", err), http.StatusBadRequest)
|
||||||
return
|
return
|
||||||
|
|
@ -183,6 +198,7 @@ func (h *Handler) handlePatchConfig(w http.ResponseWriter, r *http.Request) {
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
newCfg.Session.ApplyDmScope()
|
newCfg.Session.ApplyDmScope()
|
||||||
|
newCfg.Session.DeriveDmScope()
|
||||||
|
|
||||||
// Restore security fields (tokens/keys) from the loaded config before validation,
|
// Restore security fields (tokens/keys) from the loaded config before validation,
|
||||||
// because private fields are lost during JSON round-trip.
|
// because private fields are lost during JSON round-trip.
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue