From e74cf732ed151b523f078eb9367b079f8be03a94 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= <54936225+sourcehawk@users.noreply.github.com> Date: Wed, 2 Sep 2026 13:01:49 +0200 Subject: [PATCH] fix(strategies): keep loading when a user playbook id spans two type dirs loadUserDir returned an error when one id had a copy under two type dirs, and that error failed the whole strategies MCP at startup. Every new session then reported the server as CONNECTION_CLOSED and lost all playbook tools. A vault in that state is what an approve from before WriteUserPlaybook refused the second slot leaves behind, so any operator who approved a cross-type revision on an older build was locked out until they hand-repaired the directory. Keep the copy in the first type dir (directory order is stable) and soft-skip the later one with a stderr warning naming both files, matching how the loader already treats invalid or legacy files. Co-Authored-By: Claude Fable 5.1 --- pkg/mcp/strategies/playbook.go | 17 +++++++++--- pkg/mcp/strategies/playbook_test.go | 40 +++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+), 4 deletions(-) diff --git a/pkg/mcp/strategies/playbook.go b/pkg/mcp/strategies/playbook.go index 365d00b..5743ca4 100644 --- a/pkg/mcp/strategies/playbook.go +++ b/pkg/mcp/strategies/playbook.go @@ -339,8 +339,10 @@ func LoadPlaybooksFrom(pluginDir, systemDir string, userDirs ...string) (map[str // playbook they don't want. // // Per-id invariant: an id may NOT span multiple type directories in -// the same user dir. The loader rejects collisions so the type of an -// id stays unambiguous. +// the same user dir. WriteUserPlaybook refuses to create a second slot; +// if one exists anyway, the loader keeps the copy in the first type dir +// (directory order) and soft-skips the rest so the type of an id stays +// unambiguous without taking the server down. func loadUserDir(dir string) (map[string]*Playbook, error) { typeDirs, err := os.ReadDir(dir) if err != nil { @@ -401,7 +403,14 @@ func loadUserDir(dir string) (map[string]*Playbook, error) { } pb.Type = typeName if existingType, dup := idType[pb.ID]; dup && existingType != typeName { - return nil, fmt.Errorf("user playbook id %q exists in multiple type dirs (%s and %s); pick one", pb.ID, existingType, typeName) + // Soft-skip the later copy rather than hard-fail: a + // hard error here takes the whole strategies MCP down + // on startup for every new session, dragging every + // playbook with it. Directory order is stable, so the + // first type dir keeps the id; warn so the operator can + // delete the stray copy. + fmt.Fprintf(os.Stderr, "strategies: skipping user playbook %s/%s: id %q already loaded from %s/%s (an id lives in one type dir; delete the copy that should not exist)\n", typeName, name, pb.ID, existingType, name) + continue } idType[pb.ID] = typeName out[pb.ID] = pb @@ -892,7 +901,7 @@ func WriteUserPlaybook(dir, typeName, id, body string, activate bool) (validatio // userTypeDirHolding returns the type dir under dir that already holds // .yaml, or "" when none does. Keeps an id in a single type slot: -// loadUserDir refuses a user dir where one id spans two type dirs. +// loadUserDir only honours one copy when an id spans two type dirs. func userTypeDirHolding(dir, id string) string { entries, err := os.ReadDir(dir) if err != nil { diff --git a/pkg/mcp/strategies/playbook_test.go b/pkg/mcp/strategies/playbook_test.go index 2663ef1..8abea9c 100644 --- a/pkg/mcp/strategies/playbook_test.go +++ b/pkg/mcp/strategies/playbook_test.go @@ -171,6 +171,46 @@ nodes: assert.False(t, present, "broken playbook should have been skipped") } +// TestLoadPlaybooksFrom_IdInTwoTypeDirs keeps the strategies MCP up +// when one id has a copy under two type dirs — the state an older +// approve left behind before WriteUserPlaybook refused the second slot. +// The first type dir in directory order wins, the other copy is skipped +// with a stderr warning, and every other playbook still loads. +func TestLoadPlaybooksFrom_IdInTwoTypeDirs(t *testing.T) { + t.Parallel() + dir := t.TempDir() + body := func(symptom string) string { + return `id: dup +symptom: "` + symptom + `" +entrypoint: a +nodes: + a: + description: "step" +` + } + other := `id: other +symptom: "unaffected neighbour" +entrypoint: a +nodes: + a: + description: "step" +` + for typ, content := range map[string]string{"investigation": body("first slot"), "verification": body("second slot")} { + require.NoError(t, os.MkdirAll(filepath.Join(dir, typ), 0o700), "mkdir %s", typ) + require.NoError(t, os.WriteFile(filepath.Join(dir, typ, "dup.yaml"), []byte(content), 0o600), "write %s", typ) + } + require.NoError(t, os.WriteFile(filepath.Join(dir, "verification", "other.yaml"), []byte(other), 0o600), "write other") + + books, err := LoadPlaybooksFrom("", "", dir) + require.NoError(t, err, "an id spanning two type dirs must not fail the whole load") + dup, ok := books["dup"] + require.True(t, ok, "dup should load from one of its slots") + assert.Equal(t, "investigation", dup.Type, "first type dir in directory order wins") + assert.Equal(t, "first slot", dup.Symptom, "the winning copy is the one in the first type dir") + _, ok = books["other"] + assert.True(t, ok, "neighbouring playbooks must still load") +} + // TestLoadPlaybooksFrom_IgnoresRepoArtefacts confirms README.md, // LICENSE, dot-prefixed dirs (.git, .github) and other repo-management // files at the system-dir root are skipped without error. The clone