[BUG] SaveConfig silently deletes every api_key after the first and leaves a dangling fallback

Quick Summary

A model_list entry with more than one api_keys value loses every key after the first on a plain LoadConfig → SaveConfig round trip. The surviving entry keeps a fallbacks reference to a model name that no longer exists anywhere in the config.

This is silent data loss of user credentials. Any code path that calls SaveConfig triggers it — including picoclaw onboard --enc, which loads and re-saves an existing config (cmd/picoclaw/internal/onboard/helpers.go:64-78). Reproduced end to end with the real binary, see below.

Environment & Tools

• PicoClaw Version: bbf6893c (current main)
• Go Version: go1.25.13
• AI Model & Provider: N/A — reproduces at the config layer, no provider involved
• Operating System: macOS 15.7.4 (darwin/arm64); not OS-specific
• Channels: N/A

Steps to Reproduce

1. Write a config.json with one model that has two API keys:

{
  "version": 3,
  "model_list": [
    {
      "model_name": "my-model",
      "provider": "openai",
      "model": "gpt-4o",
      "api_keys": ["sk-primary", "sk-secondary"]
    }
  ]
}

1. LoadConfig(path) — then SaveConfig(path, cfg) — then LoadConfig(path) again. No encryption, no passphrase, no rotation. Nothing else touches the config.

2. Read the files on disk.

Drop-in failing test (public API only, package config_test, put it in pkg/config/):

package config_test

import (
	"os"
	"path/filepath"
	"testing"

	"github.com/sipeed/picoclaw/pkg/config"
)

func TestSaveConfig_DropsExtraAPIKeys(t *testing.T) {
	dir := t.TempDir()
	path := filepath.Join(dir, "config.json")

	const raw = `{
  "version": 3,
  "model_list": [
    {
      "model_name": "my-model",
      "provider": "openai",
      "model": "gpt-4o",
      "api_keys": ["sk-primary", "sk-secondary"]
    }
  ]
}
`
	if err := os.WriteFile(path, []byte(raw), 0o600); err != nil {
		t.Fatalf("WriteFile() error: %v", err)
	}

	cfg, err := config.LoadConfig(path)
	if err != nil {
		t.Fatalf("first LoadConfig() error: %v", err)
	}

	// After load, the multi-key entry has been expanded into a primary plus one
	// virtual entry per extra key.
	if got := keysOf(cfg, "my-model"); len(got) != 1 || got[0] != "sk-primary" {
		t.Fatalf("after load: my-model api_keys = %v, want [sk-primary]", got)
	}
	if got := keysOf(cfg, "my-model__key_1"); len(got) != 1 || got[0] != "sk-secondary" {
		t.Fatalf("after load: my-model__key_1 api_keys = %v, want [sk-secondary]", got)
	}

	if err := config.SaveConfig(path, cfg); err != nil {
		t.Fatalf("SaveConfig() error: %v", err)
	}

	reloaded, err := config.LoadConfig(path)
	if err != nil {
		t.Fatalf("second LoadConfig() error: %v", err)
	}

	if got := keysOf(reloaded, "my-model__key_1"); len(got) != 1 || got[0] != "sk-secondary" {
		t.Errorf("after save+reload: my-model__key_1 api_keys = %v, want [sk-secondary]; the extra key was dropped", got)
	}

	names := map[string]bool{}
	for _, m := range reloaded.ModelList {
		names[m.ModelName] = true
	}
	for _, m := range reloaded.ModelList {
		for _, fb := range m.Fallbacks {
			if !names[fb] {
				t.Errorf("after save+reload: model %q has dangling fallback %q", m.ModelName, fb)
			}
		}
	}
}

func keysOf(cfg *config.Config, modelName string) []string {
	for _, m := range cfg.ModelList {
		if m.ModelName == modelName {
			return m.APIKeys.Values()
		}
	}
	return nil
}

$ go test ./pkg/config/ -run TestSaveConfig_DropsExtraAPIKeys -count=1
--- FAIL: TestSaveConfig_DropsExtraAPIKeys
    multikey_save_repro_test.go:63: after save+reload: my-model__key_1 api_keys = [], want [sk-secondary]; the extra key was dropped
    multikey_save_repro_test.go:74: after save+reload: model "my-model" has dangling fallback "my-model__key_1"
FAIL

(The rest of pkg/config is green on the same run: 342 passed, 1 failed — only this test.)

❌ Actual Behavior

sk-secondary is gone from disk, and my-model now points its failover at a model that does not exist.

config.json after the save:

"model_list": [
  {
    "model_name": "my-model",
    "provider": "openai",
    "model": "gpt-4o",
    "fallbacks": [
      "my-model__key_1"
    ]
  }
],

.security.yml after the save:

model_list:
  my-model:0:
    api_keys:
      - sk-primary

There is no my-model__key_1 entry in either file. The key is not written anywhere; it is lost. Reloading produces a single model whose only fallback is unresolvable.

Same loss through the CLI, no test code involved

picoclaw onboard --enc on an existing config takes the "preserve the existing config" branch (cmd/picoclaw/internal/onboard/helpers.go:65-71) and then calls SaveConfig (:75), so it hits the same path. Run against the same two-key config.json above, on a clean $HOME so that ~/.ssh/picoclaw_ed25519.key does not exist yet:

$ printf 'testpass123\ntestpass123\n' | picoclaw onboard --enc
Set up credential encryption
-----------------------------
Enter passphrase for credential encryption:
Confirm passphrase:
SSH key generated: /…/.ssh/picoclaw_ed25519.key

 picoclaw is ready!

Exit code 0, no warning. config.json afterwards:

"model_list": [
  {
    "model_name": "my-model",
    "provider": "openai",
    "model": "gpt-4o",
    "fallbacks": [
      "my-model__key_1"
    ]
  }
]

.security.yml afterwards holds exactly one key:

model_list:
  my-model:0:
    api_keys:
      - enc://rpffZ+ntv0X2+cQ9kqou8axKVVuyOUfm1Q2WNTidBvoabHr2Gf1FjC1voIMyWxMTOISWd7wT

Decrypting it back with the passphrase confirms which key survived:

model=my-model  virtual=false  api_keys=[sk-primary]  fallbacks=[my-model__key_1]

grep -r sk-secondary over the whole config directory and $HOME returns nothing. The config directory contains only config.json, .security.yml and workspace/ — no .bak.

✅ Expected Behavior

A LoadConfig → SaveConfig round trip is lossless: config.json still declares api_keys: ["sk-primary", "sk-secondary"] for my-model, and no fallbacks entry refers to a model that is not in model_list.

Additional Context

Mechanism

Two correct-in-isolation behaviours that do not compose:

1. expandMultiKeyModels — pkg/config/config.go:1780-1860. On load, a multi-key entry is split into a primary holding keys[0] plus one isVirtual: true entry per extra key (:1802-1823), and the primary's Fallbacks are prepended with the virtual names (:1850-1851). This is the intended failover design and it works.

2. SaveConfig — pkg/config/config.go:1672-1682. Before serializing, virtual models are filtered out:

   go复制

   nonVirtualModels := make([]*ModelConfig, 0, len(cfg.ModelList))
   for _, m := range cfg.ModelList {
       if !m.isVirtual {
           nonVirtualModels = append(nonVirtualModels, m)
       }
   }

   Correct — virtual entries must not be persisted as real models. But nothing reverses step 1 first. The extra keys live only on the virtual entries, so filtering them drops the keys, while the Fallbacks slice written from the primary still names them.

In short: expansion is applied on load and never un-applied on save. SaveConfig is not the inverse of LoadConfig for this shape.

Impact

• Any SaveConfig caller silently destroys the operator's failover API keys.
• The damage is invisible until the primary key hits a rate limit or is revoked, at which point failover resolves to a nonexistent model.
• There is no safety net on this path. MakeBackup (pkg/config/config.go:1637) is called only by the version-migration branches (:1374, :1428, :1480) and by ResetToDefaults (:1768). SaveConfig itself does not back anything up, and it writes with WriteFileAtomic (:1700), so on an already-current-version config the old file is replaced in place and the lost key is unrecoverable. (The repro above leaves exactly two files in the directory: config.json and .security.yml — no .bak.)

Two possible directions (maintainer's call — deliberately not sending a PR)

A. Un-expand on save: rebuild the original list from primary + virtual entries. Inverse of expandMultiKeyModels. Before serializing, group virtual entries back onto their primary by the __key_N naming, restore api_keys as [primary, virtual...], and strip the synthesized fallback names. Keeps a single source of truth (ModelList) and fixes both symptoms at once. Cost: the un-expansion has to stay in sync with the expansion, and the __key_N name is load-bearing.

B. Keep the raw pre-expansion list beside the expanded one. Store the unexpanded model_list on Config at load time (an unexported field next to isVirtual) and have SaveConfig serialize that instead of reconstructing it. No inverse function to maintain and no reliance on name parsing. Cost: two representations must be kept coherent whenever runtime code mutates ModelList.

I have not sent a PR because either direction touches the model-expansion design, and the choice belongs to whoever owns it. Happy to implement whichever you prefer.