fix(strategies): keep a playbook revision in the type slot its id lives in

pkg/mcp/strategies/playbook.go

@@ -879,6 +879,9 @@ func WriteUserPlaybook(dir, typeName, id, body string, activate bool) (validatio
 	if err != nil {
 		return nil, fmt.Errorf("render yaml: %w", err)
 	}
+	if other := userTypeDirHolding(dir, id); other != "" && other != typeName {
+		return []string{fmt.Sprintf("playbook %q already lives in the %q type dir; an id can live in only one type dir", id, other)}, nil
+	}
 	typeDir := filepath.Join(dir, typeName)
 	if err := os.MkdirAll(typeDir, 0o700); err != nil {
 		return nil, fmt.Errorf("create type dir %s: %w", typeDir, err)
@@ -887,6 +890,25 @@ func WriteUserPlaybook(dir, typeName, id, body string, activate bool) (validatio
 	return nil, atomicWrite(target, rendered)
 }
 
+// userTypeDirHolding returns the type dir under dir that already holds
+// <id>.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.
+func userTypeDirHolding(dir, id string) string {
+	entries, err := os.ReadDir(dir)
+	if err != nil {
+		return ""
+	}
+	for _, e := range entries {
+		if !e.IsDir() || e.Name() == ProposalsSubdir || !playbookTypePattern.MatchString(e.Name()) {
+			continue
+		}
+		if _, err := os.Stat(filepath.Join(dir, e.Name(), id+".yaml")); err == nil {
+			return e.Name()
+		}
+	}
+	return ""
+}
+
 // proposalIDPattern guards what we'll accept as a proposal id, used
 // in the draft filename. Same family as the playbook id pattern so
 // nothing escapes the proposals dir.

pkg/mcp/strategies/playbook_test.go

@@ -171,7 +171,6 @@ nodes:
 	assert.False(t, present, "broken playbook should have been skipped")
 }
 
-
 // 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
@@ -786,3 +785,30 @@ nodes:
 	require.Empty(t, errs)
 	assert.Equal(t, DispatchDefault, pb.Dispatch)
 }
+
+// TestWriteUserPlaybook_RejectsIDInAnotherTypeDir: an id lives in
+// exactly one type dir. Writing it under a second one would leave two
+// files the loader refuses to load, so the write is rejected as a
+// validation error that names the existing slot.
+func TestWriteUserPlaybook_RejectsIDInAnotherTypeDir(t *testing.T) {
+	t.Parallel()
+	dir := t.TempDir()
+	body := `id: backup
+schema_version: 1
+symptom: "first"
+entrypoint: a
+nodes:
+  a:
+    description: "step"
+`
+	errs, err := WriteUserPlaybook(dir, "verification", "backup", body, true)
+	require.NoError(t, err)
+	require.Empty(t, errs)
+
+	errs, err = WriteUserPlaybook(dir, "investigation", "backup", body, true)
+	require.NoError(t, err)
+	require.Len(t, errs, 1)
+	assert.Contains(t, errs[0], "verification")
+	_, statErr := os.Stat(filepath.Join(dir, "investigation", "backup.yaml"))
+	assert.ErrorIs(t, statErr, os.ErrNotExist, "no second copy is written")
+}

pkg/mcp/strategies/specs.go

@@ -57,7 +57,7 @@ func ToolSpecs() []toolspec.ToolSpec {
 		{
 			Server:      "triagent-strategies",
 			Name:        "get_playbook_raw",
-			Description: "Return raw YAML for one playbook by id (base for a proposed update).",
+			Description: "Return raw YAML and the type slot for one playbook by id (base for a proposed update; a revision keeps the id and the slot).",
 			Inputs:      toolspec.FromStruct(getPlaybookRawIn{}),
 		},
 		{

pkg/mcp/strategies/tools_proposal.go

@@ -3,6 +3,8 @@ package strategies
 import (
 	"context"
 	"fmt"
+	"os"
+	"path/filepath"
 	"strings"
 
 	"github.com/modelcontextprotocol/go-sdk/mcp"
@@ -209,17 +211,13 @@ type getPlaybookRawOut struct {
 	ID     string `json:"id"`
 	YAML   string `json:"yaml"`
 	Source string `json:"source"` // "system" or "user"
+	Type   string `json:"type"`   // the type slot the id lives in; a revision must be filed under the same slot
 }
 
-// getPlaybookRaw returns the raw YAML for one loaded playbook. Used by
-// the agent during a proposal flow to base an update on an existing
-// playbook (read it, modify it, propose under the same or a fresh id).
-//
-// "Raw" is stronger for the system set (we have the original YAML in
-// EmbeddedRawPlaybooks and return it byte-for-byte). User playbooks
-// aren't tracked with their raw bytes; we re-serialise from the parsed
-// in-memory representation, which loses comments and exact formatting
-// but stays semantically identical.
+// getPlaybookRaw returns the raw YAML for one loaded playbook plus the
+// type slot it lives in. Used by the agent during a proposal flow to
+// base an update on an existing playbook (read it, modify it, propose
+// under the same id and type, or under a fresh id).
 func (s *Server) getPlaybookRaw(ctx context.Context, req *mcp.CallToolRequest, in getPlaybookRawIn) (*mcp.CallToolResult, getPlaybookRawOut, error) {
 	if in.ID == "" {
 		return errorResult("id is required"), getPlaybookRawOut{}, nil
@@ -228,20 +226,45 @@ func (s *Server) getPlaybookRaw(ctx context.Context, req *mcp.CallToolRequest, i
 	if !ok {
 		return errorResult(fmt.Sprintf("playbook %q not found; call list_playbooks for valid ids", in.ID)), getPlaybookRawOut{}, nil
 	}
+	out := getPlaybookRawOut{ID: in.ID, Type: pb.Type}
+	// Raw bytes come from the tier the loaded copy was read from, so
+	// comments and formatting survive. Locked metas live under the
+	// launcher-bundled dir; for the rest the active copy wins — a
+	// user-dir file overrides the plugin tier, so a revision has to
+	// start from it or the operator's local edits vanish on approve.
+	if pb.Locked {
+		if data, err := os.ReadFile(filepath.Join(s.systemPlaybooksDir, pb.Type, in.ID+".yaml")); err == nil {
+			out.YAML, out.Source = string(data), "system"
+			return nil, out, nil
+		}
+	} else if pb.Type != "" && s.userPlaybooksDir != "" {
+		// The loader soft-skips a user file that fails to parse or
+		// validate and keeps the plugin copy active, so only a file
+		// that would have loaded counts as the override.
+		if data, err := os.ReadFile(filepath.Join(s.userPlaybooksDir, pb.Type, in.ID+".yaml")); err == nil {
+			if parsed, errs := ParseAndValidatePlaybookYAML(data); len(errs) == 0 && parsed.ID == in.ID {
+				out.YAML, out.Source = string(data), "user"
+				return nil, out, nil
+			}
+		}
+	}
 	systemRaw, err := SystemRawPlaybooks(s.pluginPlaybooksDir)
 	if err != nil {
 		return errorResult(fmt.Sprintf("read system playbooks: %v", err)), getPlaybookRawOut{}, nil
 	}
 	if raw, isSystem := systemRaw[in.ID]; isSystem {
-		return nil, getPlaybookRawOut{ID: in.ID, YAML: raw.YAML, Source: "system"}, nil
+		out.YAML, out.Source = raw.YAML, "system"
+		return nil, out, nil
 	}
-	// User playbook: re-serialise the parsed form. Round-trips cleanly
-	// for our own validator but loses comments/exact whitespace.
+	// No file on disk to hand back (in-memory fixtures): re-serialise
+	// the parsed form. Round-trips cleanly for our own validator but
+	// loses comments/exact whitespace.
 	rendered, err := RenderPlaybookYAML(pb)
 	if err != nil {
 		return errorResult(fmt.Sprintf("render user playbook %q: %v", in.ID, err)), getPlaybookRawOut{}, nil
 	}
-	return nil, getPlaybookRawOut{ID: in.ID, YAML: rendered, Source: "user"}, nil
+	out.YAML, out.Source = rendered, "user"
+	return nil, out, nil
 }
 
 // ── validate_playbook ──────────────────────────────────────────────────────
@@ -275,7 +298,7 @@ func (s *Server) validatePlaybook(ctx context.Context, req *mcp.CallToolRequest,
 
 type proposePlaybookDraftIn struct {
 	YAML string `json:"yaml" jsonschema:"the full playbook YAML to propose. Omit the version field — playbooks no longer carry a version field; git history is the version record."`
-	Type string `json:"type" jsonschema:"the type slot the playbook belongs to (one of the names returned by playbook_types — e.g. 'investigation' or 'general'). Required: the launcher routes the draft into <type>/ under the user playbooks dir, and the type slot is the source of truth for classification (no longer a YAML field)."`
+	Type string `json:"type" jsonschema:"the type slot the playbook belongs to (one of the names returned by playbook_types — e.g. 'investigation' or 'general'). Required: the launcher routes the draft into <type>/ under the user playbooks dir, and the type slot is the source of truth for classification (no longer a YAML field). A revision of an existing id must use the slot get_playbook_raw reports for it."`
 	Why  string `json:"why,omitempty" jsonschema:"one-sentence justification — surfaced in the diff card so the operator can audit later why the agent thought this was worth proposing"`
 }
 
@@ -329,6 +352,13 @@ func (s *Server) proposePlaybookDraft(ctx context.Context, req *mcp.CallToolRequ
 	if len(errs) > 0 {
 		return nil, proposePlaybookDraftOut{ValidationErrors: errs}, nil
 	}
+	// A revision stays in the slot its id already lives in. Filing it
+	// under another type would land a second <type>/<id>.yaml on
+	// approve, and the loader refuses a user dir where one id spans two
+	// type dirs — taking every strategies tool down with it.
+	if existing, ok := s.playbooks[pb.ID]; ok && existing.Type != "" && existing.Type != in.Type {
+		return errorResult(fmt.Sprintf("playbook %q lives in the %q type slot; pass type=%q to revise it (get_playbook_raw reports the slot), or pick a new id to file under %q", pb.ID, existing.Type, existing.Type, in.Type)), proposePlaybookDraftOut{}, nil
+	}
 
 	// Drafts no longer carry a version stamp. The version field has been
 	// removed from playbooks; git history is the version record. pb.Active

pkg/mcp/strategies/tools_proposal_test.go

@@ -3,6 +3,9 @@ package strategies
 import (
 	"context"
 	"encoding/json"
+	"fmt"
+	"os"
+	"path/filepath"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -112,3 +115,140 @@ nodes:
 	assert.Equal(t, "testpb", keys["playbook_id"])
 	assert.Equal(t, "investigation", keys["type"])
 }
+
+// TestProposePlaybookDraft_RejectsTypeMismatchForExistingID pins the
+// type slot of an existing id: a revision filed under a different
+// type would land as a second file next to the original on approve,
+// which the loader refuses to load at all.
+func TestProposePlaybookDraft_RejectsTypeMismatchForExistingID(t *testing.T) {
+	t.Parallel()
+	srv := newServerWithUserPlaybooksDir(t)
+	srv.playbooks["release_checks"] = &Playbook{
+		ID:         "release_checks",
+		Symptom:    "existing",
+		Entrypoint: "a",
+		Nodes:      map[string]Node{"a": {Description: "a", TerminalAdvice: "done"}},
+		Type:       "verification",
+	}
+	yaml := `id: release_checks
+schema_version: 1
+symptom: revised
+entrypoint: a
+nodes:
+  a:
+    description: a
+    terminal_advice: done
+`
+	res, out, err := srv.proposePlaybookDraft(context.Background(), nil, proposePlaybookDraftIn{
+		YAML: yaml,
+		Type: "investigation",
+		Why:  "test",
+	})
+	require.NoError(t, err)
+	require.NotNil(t, res)
+	assert.True(t, res.IsError, "a revision must stay in the type slot the id already lives in")
+	assert.Contains(t, textOf(res), "verification", "the error names the slot the id lives in")
+	assert.Empty(t, out.ProposalID)
+
+	res, out, err = srv.proposePlaybookDraft(context.Background(), nil, proposePlaybookDraftIn{
+		YAML: yaml,
+		Type: "verification",
+		Why:  "test",
+	})
+	require.NoError(t, err)
+	assert.Nil(t, res, "the matching slot is accepted")
+	assert.NotEmpty(t, out.ProposalID)
+}
+
+// TestGetPlaybookRaw_ReportsTypeAndPrefersUserOverride: the agent
+// bases a revision on what get_playbook_raw returns, so it must be
+// the active (user-overridden) copy, and it must carry the type slot
+// the revision has to be filed under.
+func TestGetPlaybookRaw_ReportsTypeAndPrefersUserOverride(t *testing.T) {
+	t.Parallel()
+	const body = `id: release_checks
+schema_version: 1
+symptom: %s
+entrypoint: a
+nodes:
+  a:
+    description: a
+    terminal_advice: done
+`
+	pluginDir := t.TempDir()
+	userDir := t.TempDir()
+	require.NoError(t, os.MkdirAll(filepath.Join(pluginDir, "verification"), 0o755))
+	require.NoError(t, os.MkdirAll(filepath.Join(userDir, "verification"), 0o755))
+	require.NoError(t, os.WriteFile(filepath.Join(pluginDir, "verification", "release_checks.yaml"), []byte(fmt.Sprintf(body, "upstream copy")), 0o644))
+	require.NoError(t, os.WriteFile(filepath.Join(userDir, "verification", "release_checks.yaml"), []byte(fmt.Sprintf(body, "local override")), 0o644))
+
+	srv, err := New(Options{PluginPlaybooksDir: pluginDir, UserPlaybooksDir: userDir})
+	require.NoError(t, err)
+	res, out, err := srv.getPlaybookRaw(context.Background(), nil, getPlaybookRawIn{ID: "release_checks"})
+	require.NoError(t, err)
+	require.Nil(t, res)
+	assert.Equal(t, "verification", out.Type)
+	assert.Equal(t, "user", out.Source)
+	assert.Contains(t, out.YAML, "local override")
+	assert.NotContains(t, out.YAML, "upstream copy")
+}
+
+// TestGetPlaybookRaw_LockedReadsSystemDirBytes: locked metas live at
+// <systemPlaybooksDir>/<type>/<id>.yaml; the raw bytes come from there
+// (comments intact) and the source is reported as "system".
+func TestGetPlaybookRaw_LockedReadsSystemDirBytes(t *testing.T) {
+	t.Parallel()
+	systemDir := t.TempDir()
+	require.NoError(t, os.MkdirAll(filepath.Join(systemDir, "system"), 0o755))
+	body := `# keep this comment
+id: locked_meta
+schema_version: 1
+symptom: locked
+entrypoint: a
+nodes:
+  a:
+    description: a
+    terminal_advice: done
+`
+	require.NoError(t, os.WriteFile(filepath.Join(systemDir, "system", "locked_meta.yaml"), []byte(body), 0o644))
+
+	srv, err := New(Options{SystemPlaybooksDir: systemDir})
+	require.NoError(t, err)
+	res, out, err := srv.getPlaybookRaw(context.Background(), nil, getPlaybookRawIn{ID: "locked_meta"})
+	require.NoError(t, err)
+	require.Nil(t, res)
+	assert.Equal(t, "system", out.Type)
+	assert.Equal(t, "system", out.Source)
+	assert.Equal(t, body, out.YAML)
+}
+
+// TestGetPlaybookRaw_SkipsInvalidUserOverride: the loader soft-skips a
+// user file that fails to parse or validate and keeps the plugin copy
+// active, so the raw bytes must come from the copy that actually loaded.
+func TestGetPlaybookRaw_SkipsInvalidUserOverride(t *testing.T) {
+	t.Parallel()
+	pluginDir := t.TempDir()
+	userDir := t.TempDir()
+	require.NoError(t, os.MkdirAll(filepath.Join(pluginDir, "verification"), 0o755))
+	require.NoError(t, os.MkdirAll(filepath.Join(userDir, "verification"), 0o755))
+	valid := `id: release_checks
+schema_version: 1
+symptom: upstream copy
+entrypoint: a
+nodes:
+  a:
+    description: a
+    terminal_advice: done
+`
+	require.NoError(t, os.WriteFile(filepath.Join(pluginDir, "verification", "release_checks.yaml"), []byte(valid), 0o644))
+	// Entrypoint names a node that does not exist: parses, fails validation.
+	require.NoError(t, os.WriteFile(filepath.Join(userDir, "verification", "release_checks.yaml"), []byte("id: release_checks\nschema_version: 1\nsymptom: broken\nentrypoint: missing\nnodes:\n  a:\n    description: a\n"), 0o644))
+
+	srv, err := New(Options{PluginPlaybooksDir: pluginDir, UserPlaybooksDir: userDir})
+	require.NoError(t, err)
+	res, out, err := srv.getPlaybookRaw(context.Background(), nil, getPlaybookRawIn{ID: "release_checks"})
+	require.NoError(t, err)
+	require.Nil(t, res)
+	assert.Equal(t, "system", out.Source)
+	assert.Equal(t, valid, out.YAML)
+}

system/playbook_proposal.yaml

@@ -219,14 +219,16 @@ nodes:
   read_existing_for_revision:
     description: |
       Fetch the YAML of the existing playbook you'll be revising. Keep
-      the SAME id when you draft your version — the launcher's
-      proposal flow auto-bumps the version field (v1 → v2 → v3 …) on
-      approval and writes <id>.<new_version>.yaml alongside the
-      existing one.
-
-      Don't invent a fresh id like "<original>_v2"; same-id versioning
-      means the operator picks which version is active in one place,
-      and rolling back from v3 → v2 stays one click away.
+      the SAME id when you draft your version, and pass the `type`
+      that get_playbook_raw reports as `type` to
+      playbook_proposal_draft — an id lives in exactly one type slot,
+      and a draft filed under a different slot is rejected. On
+      approval the launcher overwrites <type>/<id>.yaml; git history
+      is the version record.
+
+      Don't invent a fresh id like "<original>_v2"; same-id revisions
+      keep one active version per id and let the operator roll back
+      through the file's history.
     suggested_calls:
       - tool: triagent-strategies/get_playbook_raw
         args: