diff --git a/pkg/mcp/strategies/playbook.go b/pkg/mcp/strategies/playbook.go index e25ad7ba..365d00bc 100644 --- a/pkg/mcp/strategies/playbook.go +++ b/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 +// .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. diff --git a/pkg/mcp/strategies/playbook_test.go b/pkg/mcp/strategies/playbook_test.go index ff31ea5a..2663ef1c 100644 --- a/pkg/mcp/strategies/playbook_test.go +++ b/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") +} diff --git a/pkg/mcp/strategies/specs.go b/pkg/mcp/strategies/specs.go index 8edc3192..b1e77814 100644 --- a/pkg/mcp/strategies/specs.go +++ b/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{}), }, { diff --git a/pkg/mcp/strategies/tools_proposal.go b/pkg/mcp/strategies/tools_proposal.go index 2d56f235..111c23e2 100644 --- a/pkg/mcp/strategies/tools_proposal.go +++ b/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 / 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 / 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 /.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 diff --git a/pkg/mcp/strategies/tools_proposal_test.go b/pkg/mcp/strategies/tools_proposal_test.go index 0ed13f6f..eb2ad250 100644 --- a/pkg/mcp/strategies/tools_proposal_test.go +++ b/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 +// //.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) +} diff --git a/system/playbook_proposal.yaml b/system/playbook_proposal.yaml index 61be2f9b..fe80b3ac 100644 --- a/system/playbook_proposal.yaml +++ b/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 ..yaml alongside the - existing one. - - Don't invent a fresh id like "_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 /.yaml; git history + is the version record. + + Don't invent a fresh id like "_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: