Skip to content

fix(compact): resolve unresolved review findings and harden parsing #12

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
wynxing merged 1 commit into from
Apr 14, 2026
Merged

fix(compact): resolve unresolved review findings and harden parsing #12

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 35 additions & 27 deletions internal/config/loader.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,10 +38,12 @@ type persistedContextConfig struct {
}

type persistedCompactConfig struct {
ManualStrategy string `yaml:"manual_strategy,omitempty"`
ManualKeepRecentMessages int `yaml:"manual_keep_recent_messages,omitempty"`
MaxSummaryChars int `yaml:"max_summary_chars,omitempty"`
MicroCompactDisabled bool `yaml:"micro_compact_disabled,omitempty"`
ManualStrategy string `yaml:"manual_strategy,omitempty"`
ManualKeepRecentMessages int `yaml:"manual_keep_recent_messages,omitempty"`
MaxSummaryChars int `yaml:"max_summary_chars,omitempty"`
MicroCompactDisabled bool `yaml:"micro_compact_disabled,omitempty"`
MicroCompactRetainedToolSpans int `yaml:"micro_compact_retained_tool_spans,omitempty"`
MaxArchivedPromptChars int `yaml:"max_archived_prompt_chars,omitempty"`
}

type persistedAutoCompactConfig struct {
Expand Down Expand Up @@ -237,10 +239,12 @@ func marshalPersistedConfig(snapshot Config) ([]byte, error) {
func newPersistedContextConfig(cfg ContextConfig) persistedContextConfig {
return persistedContextConfig{
Compact: persistedCompactConfig{
ManualStrategy: cfg.Compact.ManualStrategy,
ManualKeepRecentMessages: cfg.Compact.ManualKeepRecentMessages,
MaxSummaryChars: cfg.Compact.MaxSummaryChars,
MicroCompactDisabled: cfg.Compact.MicroCompactDisabled,
ManualStrategy: cfg.Compact.ManualStrategy,
ManualKeepRecentMessages: cfg.Compact.ManualKeepRecentMessages,
MaxSummaryChars: cfg.Compact.MaxSummaryChars,
MicroCompactDisabled: cfg.Compact.MicroCompactDisabled,
MicroCompactRetainedToolSpans: cfg.Compact.MicroCompactRetainedToolSpans,
MaxArchivedPromptChars: cfg.Compact.MaxArchivedPromptChars,
},
AutoCompact: persistedAutoCompactConfig{
Enabled: cfg.AutoCompact.Enabled,
Expand All @@ -253,10 +257,12 @@ func newPersistedContextConfig(cfg ContextConfig) persistedContextConfig {
func fromPersistedContextConfig(file persistedContextConfig, defaults ContextConfig) ContextConfig {
out := ContextConfig{
Compact: CompactConfig{
ManualStrategy: strings.TrimSpace(file.Compact.ManualStrategy),
ManualKeepRecentMessages: file.Compact.ManualKeepRecentMessages,
MaxSummaryChars: file.Compact.MaxSummaryChars,
MicroCompactDisabled: file.Compact.MicroCompactDisabled,
ManualStrategy: strings.TrimSpace(file.Compact.ManualStrategy),
ManualKeepRecentMessages: file.Compact.ManualKeepRecentMessages,
MaxSummaryChars: file.Compact.MaxSummaryChars,
MicroCompactDisabled: file.Compact.MicroCompactDisabled,
MicroCompactRetainedToolSpans: file.Compact.MicroCompactRetainedToolSpans,
MaxArchivedPromptChars: file.Compact.MaxArchivedPromptChars,
},
AutoCompact: AutoCompactConfig{
Enabled: file.AutoCompact.Enabled,
Expand Down Expand Up @@ -288,24 +294,26 @@ func assembleProviders(builtin []ProviderConfig, custom []ProviderConfig) ([]Pro
return nil
}

for _, provider := range builtin {
candidate := cloneProviderConfig(provider)
if candidate.Source == "" {
candidate.Source = ProviderSourceBuiltin
}
if err := appendProvider(candidate); err != nil {
return nil, err
}
sections := []struct {
providers []ProviderConfig
source ProviderSource
}{
{providers: builtin, source: ProviderSourceBuiltin},
{providers: custom, source: ProviderSourceCustom},
}
for _, provider := range custom {
candidate := cloneProviderConfig(provider)
if candidate.Source == "" {
candidate.Source = ProviderSourceCustom
}
if err := appendProvider(candidate); err != nil {
return nil, err

for _, section := range sections {
for _, provider := range section.providers {
candidate := cloneProviderConfig(provider)
if candidate.Source == "" {
candidate.Source = section.source
}
if err := appendProvider(candidate); err != nil {
return nil, err
}
}
}

return assembled, nil
}

Expand Down
153 changes: 93 additions & 60 deletions internal/config/loader_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,20 @@ import (
"testing"
)

func writeLoaderConfig(t *testing.T, loader *Loader, raw string) {
t.Helper()
if err := os.MkdirAll(loader.BaseDir(), 0o755); err != nil {
t.Fatalf("mkdir base dir: %v", err)
}
content := raw
if strings.Contains(raw, "\n") {
content = strings.TrimSpace(raw) + "\n"
}
if err := os.WriteFile(loader.ConfigPath(), []byte(content), 0o644); err != nil {
t.Fatalf("write config: %v", err)
}
}

func TestLoaderLoadMissingConfigCreatesDefault(t *testing.T) {
t.Parallel()

Expand All @@ -32,12 +46,7 @@ func TestLoaderLoadMalformedYAML(t *testing.T) {
t.Parallel()

loader := NewLoader(t.TempDir(), testDefaultConfig())
if err := os.MkdirAll(loader.BaseDir(), 0o755); err != nil {
t.Fatalf("mkdir base dir: %v", err)
}
if err := os.WriteFile(loader.ConfigPath(), []byte("providers:\n - name: [\n"), 0o644); err != nil {
t.Fatalf("write malformed config: %v", err)
}
writeLoaderConfig(t, loader, "providers:\n - name: [\n")

_, err := loader.Load(context.Background())
if err == nil || !strings.Contains(err.Error(), "parse config file") {
Expand All @@ -49,18 +58,13 @@ func TestLoaderRejectsLegacyWorkdirKey(t *testing.T) {
t.Parallel()

loader := NewLoader(t.TempDir(), testDefaultConfig())
if err := os.MkdirAll(loader.BaseDir(), 0o755); err != nil {
t.Fatalf("mkdir base dir: %v", err)
}
raw := `
selected_provider: openai
current_model: gpt-4.1
workdir: .
shell: powershell
`
if err := os.WriteFile(loader.ConfigPath(), []byte(strings.TrimSpace(raw)+"\n"), 0o644); err != nil {
t.Fatalf("write legacy config: %v", err)
}
writeLoaderConfig(t, loader, raw)

_, err := loader.Load(context.Background())
if err == nil || !strings.Contains(err.Error(), "field workdir not found") {
Expand All @@ -72,18 +76,13 @@ func TestLoaderRejectsLegacyDefaultWorkdirKey(t *testing.T) {
t.Parallel()

loader := NewLoader(t.TempDir(), testDefaultConfig())
if err := os.MkdirAll(loader.BaseDir(), 0o755); err != nil {
t.Fatalf("mkdir base dir: %v", err)
}
raw := `
selected_provider: openai
current_model: gpt-4.1
default_workdir: .
shell: powershell
`
if err := os.WriteFile(loader.ConfigPath(), []byte(strings.TrimSpace(raw)+"\n"), 0o644); err != nil {
t.Fatalf("write legacy config: %v", err)
}
writeLoaderConfig(t, loader, raw)

_, err := loader.Load(context.Background())
if err == nil || !strings.Contains(err.Error(), "field default_workdir not found") {
Expand Down Expand Up @@ -111,9 +110,6 @@ func TestLoaderRejectsLegacyProvidersFormatOnLoad(t *testing.T) {
t.Parallel()

loader := NewLoader(t.TempDir(), testDefaultConfig())
if err := os.MkdirAll(loader.BaseDir(), 0o755); err != nil {
t.Fatalf("mkdir base dir: %v", err)
}

legacy := `
selected_provider: openai
Expand All @@ -126,9 +122,7 @@ providers:
model: gpt-5.4
api_key_env: OPENAI_API_KEY
`
if err := os.WriteFile(loader.ConfigPath(), []byte(strings.TrimSpace(legacy)+"\n"), 0o644); err != nil {
t.Fatalf("write legacy config: %v", err)
}
writeLoaderConfig(t, loader, legacy)

_, err := loader.Load(context.Background())
if err == nil || !strings.Contains(err.Error(), "field providers not found") {
Expand All @@ -140,17 +134,12 @@ func TestLoaderPreservesSelectionStateOnLoad(t *testing.T) {
t.Parallel()

loader := NewLoader(t.TempDir(), testDefaultConfig())
if err := os.MkdirAll(loader.BaseDir(), 0o755); err != nil {
t.Fatalf("mkdir base dir: %v", err)
}

raw := `
selected_provider: missing-provider
shell: powershell
`
if err := os.WriteFile(loader.ConfigPath(), []byte(strings.TrimSpace(raw)+"\n"), 0o644); err != nil {
t.Fatalf("write config: %v", err)
}
writeLoaderConfig(t, loader, raw)

cfg, err := loader.Load(context.Background())
if err != nil {
Expand All @@ -177,17 +166,12 @@ func TestLoaderPreservesMissingCurrentModelOnLoad(t *testing.T) {
t.Parallel()

loader := NewLoader(t.TempDir(), testDefaultConfig())
if err := os.MkdirAll(loader.BaseDir(), 0o755); err != nil {
t.Fatalf("mkdir base dir: %v", err)
}

raw := `
selected_provider: openai
shell: powershell
`
if err := os.WriteFile(loader.ConfigPath(), []byte(strings.TrimSpace(raw)+"\n"), 0o644); err != nil {
t.Fatalf("write config: %v", err)
}
writeLoaderConfig(t, loader, raw)

cfg, err := loader.Load(context.Background())
if err != nil {
Expand Down Expand Up @@ -223,9 +207,7 @@ func TestLoaderAllowsSelectedCustomProviderWithEmptyCurrentModel(t *testing.T) {
selected_provider: company-gateway
shell: powershell
`
if err := os.WriteFile(loader.ConfigPath(), []byte(strings.TrimSpace(rawConfig)+"\n"), 0o644); err != nil {
t.Fatalf("write config: %v", err)
}
writeLoaderConfig(t, loader, rawConfig)

providerYAML := `
name: company-gateway
Expand Down Expand Up @@ -263,9 +245,7 @@ selected_provider: company-gateway
current_model: deepseek-coder
shell: powershell
`
if err := os.WriteFile(loader.ConfigPath(), []byte(strings.TrimSpace(rawConfig)+"\n"), 0o644); err != nil {
t.Fatalf("write config: %v", err)
}
writeLoaderConfig(t, loader, rawConfig)

providerYAML := `
name: company-gateway
Expand Down Expand Up @@ -523,9 +503,7 @@ selected_provider: company-gateway
current_model: server-model
shell: powershell
`
if err := os.WriteFile(loader.ConfigPath(), []byte(strings.TrimSpace(rawConfig)+"\n"), 0o644); err != nil {
t.Fatalf("write config: %v", err)
}
writeLoaderConfig(t, loader, rawConfig)

providerYAML := `
name: company-gateway
Expand Down Expand Up @@ -582,9 +560,7 @@ selected_provider: company-gateway
current_model: server-model
shell: powershell
`
if err := os.WriteFile(loader.ConfigPath(), []byte(strings.TrimSpace(rawConfig)+"\n"), 0o644); err != nil {
t.Fatalf("write config: %v", err)
}
writeLoaderConfig(t, loader, rawConfig)

providerYAML := `
name: company-gateway
Expand Down Expand Up @@ -764,9 +740,6 @@ func TestLoaderMemoConfigPreservesExplicitFalse(t *testing.T) {
t.Parallel()

loader := NewLoader(t.TempDir(), testDefaultConfig())
if err := os.MkdirAll(loader.BaseDir(), 0o755); err != nil {
t.Fatalf("mkdir base dir: %v", err)
}

raw := `
selected_provider: openai
Expand All @@ -777,9 +750,7 @@ memo:
auto_extract: false
max_index_lines: 123
`
if err := os.WriteFile(loader.ConfigPath(), []byte(strings.TrimSpace(raw)+"\n"), 0o644); err != nil {
t.Fatalf("write config: %v", err)
}
writeLoaderConfig(t, loader, raw)

cfg, err := loader.Load(context.Background())
if err != nil {
Expand Down Expand Up @@ -812,18 +783,13 @@ func TestLoaderMemoConfigAppliesDefaultsWhenSectionMissing(t *testing.T) {
t.Parallel()

loader := NewLoader(t.TempDir(), testDefaultConfig())
if err := os.MkdirAll(loader.BaseDir(), 0o755); err != nil {
t.Fatalf("mkdir base dir: %v", err)
}

raw := `
selected_provider: openai
current_model: gpt-4.1
shell: powershell
`
if err := os.WriteFile(loader.ConfigPath(), []byte(strings.TrimSpace(raw)+"\n"), 0o644); err != nil {
t.Fatalf("write config: %v", err)
}
writeLoaderConfig(t, loader, raw)

cfg, err := loader.Load(context.Background())
if err != nil {
Expand All @@ -839,3 +805,70 @@ shell: powershell
t.Fatalf("expected memo.max_index_lines to be defaulted, got %d", cfg.Memo.MaxIndexLines)
}
}

func TestLoaderLoadsCompactExtendedFields(t *testing.T) {
t.Parallel()

loader := NewLoader(t.TempDir(), testDefaultConfig())

raw := `
selected_provider: openai
current_model: gpt-4.1
shell: powershell
context:
compact:
manual_strategy: keep_recent
manual_keep_recent_messages: 9
max_summary_chars: 900
micro_compact_retained_tool_spans: 4
max_archived_prompt_chars: 4096
`
writeLoaderConfig(t, loader, raw)

cfg, err := loader.Load(context.Background())
if err != nil {
t.Fatalf("Load() error = %v", err)
}
if cfg.Context.Compact.MicroCompactRetainedToolSpans != 4 {
t.Fatalf("expected micro_compact_retained_tool_spans=4, got %d", cfg.Context.Compact.MicroCompactRetainedToolSpans)
}
if cfg.Context.Compact.MaxArchivedPromptChars != 4096 {
t.Fatalf("expected max_archived_prompt_chars=4096, got %d", cfg.Context.Compact.MaxArchivedPromptChars)
}
}

func TestLoaderSaveRoundTripsCompactExtendedFields(t *testing.T) {
t.Parallel()

loader := NewLoader(t.TempDir(), testDefaultConfig())
cfg := loader.DefaultConfig()
cfg.Context.Compact.MicroCompactRetainedToolSpans = 5
cfg.Context.Compact.MaxArchivedPromptChars = 3072

if err := loader.Save(context.Background(), &cfg); err != nil {
t.Fatalf("Save() error = %v", err)
}

data, err := os.ReadFile(loader.ConfigPath())
if err != nil {
t.Fatalf("read config: %v", err)
}
text := string(data)
if !strings.Contains(text, "micro_compact_retained_tool_spans: 5") {
t.Fatalf("expected persisted micro_compact_retained_tool_spans, got:\n%s", text)
}
if !strings.Contains(text, "max_archived_prompt_chars: 3072") {
t.Fatalf("expected persisted max_archived_prompt_chars, got:\n%s", text)
}

loaded, err := loader.Load(context.Background())
if err != nil {
t.Fatalf("Load() error = %v", err)
}
if loaded.Context.Compact.MicroCompactRetainedToolSpans != 5 {
t.Fatalf("expected round-trip micro_compact_retained_tool_spans=5, got %d", loaded.Context.Compact.MicroCompactRetainedToolSpans)
}
if loaded.Context.Compact.MaxArchivedPromptChars != 3072 {
t.Fatalf("expected round-trip max_archived_prompt_chars=3072, got %d", loaded.Context.Compact.MaxArchivedPromptChars)
}
}
Loading
Loading