diff --git a/frontend/components/playbooks/PlaybookEditor.reducer.ts b/frontend/components/playbooks/PlaybookEditor.reducer.ts index 27d93d6b..ee022e1e 100644 --- a/frontend/components/playbooks/PlaybookEditor.reducer.ts +++ b/frontend/components/playbooks/PlaybookEditor.reducer.ts @@ -6,17 +6,16 @@ import { type PlaybookNode, } from "@/lib/playbook"; -// Parser for the playbook drawer. Validates the three required fields -// the editor's UI relies on, dedupes tabs by playbook_id (latest draft -// for each id wins). +// Parser for the playbook drawer. Validates the two ids the editor's +// UI relies on (the card fetches the diff bodies itself), dedupes tabs +// by playbook_id (latest draft for each id wins). export function parsePlaybookProposal( raw: unknown, ): { key: string; payload: ProposalDraftPayload } | null { const r = raw as Partial; if ( typeof r?.proposal_id !== "string" || - typeof r?.playbook_id !== "string" || - typeof r?.new_yaml !== "string" + typeof r?.playbook_id !== "string" ) { return null; } diff --git a/frontend/components/playbooks/ProposalBodyTabs.tsx b/frontend/components/playbooks/ProposalBodyTabs.tsx index 4129b579..f774db31 100644 --- a/frontend/components/playbooks/ProposalBodyTabs.tsx +++ b/frontend/components/playbooks/ProposalBodyTabs.tsx @@ -10,7 +10,7 @@ import { import dynamic from "next/dynamic"; import { parsePlaybookYAML, type Playbook } from "@/lib/playbook"; import { PlaybookGraph } from "@/components/playbooks/PlaybookGraph"; -import type { ProposalDraftPayload } from "@/components/playbooks/ProposalCard"; +import type { ProposalBody } from "@/components/playbooks/ProposalCard"; // react-diff-viewer-continued is client-only and pulls in // styled-components. Dynamic import keeps SSR clean. @@ -24,14 +24,14 @@ const DiffViewer = dynamic(() => import("react-diff-viewer-continued"), { type Tab = "diagram" | "yaml"; type Props = { - payload: ProposalDraftPayload; + body: ProposalBody; // Initial tab. Auto-overridden to "yaml" if proposed YAML fails to // parse. Default: "diagram". defaultTab?: Tab; }; -export function ProposalBodyTabs({ payload, defaultTab = "diagram" }: Props) { - const parsed = useMemo(() => parseSides(payload), [payload]); +export function ProposalBodyTabs({ body, defaultTab = "diagram" }: Props) { + const parsed = useMemo(() => parseSides(body), [body]); const proposedBroken = parsed.proposed === null; const baseBroken = parsed.base === "error"; @@ -73,7 +73,7 @@ export function ProposalBodyTabs({ payload, defaultTab = "diagram" }: Props) { baseBroken={baseBroken} /> ) : ( - + )} ); @@ -149,13 +149,13 @@ function DiagramPane({ ); } -function YamlPane({ payload }: { payload: ProposalDraftPayload }) { - const isNew = !payload.base_yaml || payload.base_yaml.trim() === ""; +function YamlPane({ body }: { body: ProposalBody }) { + const isNew = !body.base_yaml || body.base_yaml.trim() === ""; return (
({ + default: () => + function DiffStub(props: { oldValue: string; newValue: string }) { + return ( +
+ {props.oldValue} + {props.newValue} +
+ ); + }, +})); + +// The tool result carries only the ids; the bodies come from the +// proposal endpoint. +const payload: ProposalDraftPayload = { + proposal_id: "prop-aaaaaaaaaaaa", + playbook_id: "testpb", + why: "because", +}; + +const newYaml = + "id: testpb\nschema_version: 1\nsymptom: s\nentrypoint: a\nnodes:\n a:\n description: a\n terminal_advice: done\n"; +const baseYaml = + "id: testpb\nschema_version: 1\nsymptom: old\nentrypoint: a\nnodes:\n a:\n description: a\n terminal_advice: done\n"; + +beforeEach(() => { + vi.restoreAllMocks(); +}); + +describe("ProposalCard body hydration", () => { + it("fetches base/new YAML from the proposal endpoint when the payload has none", async () => { + vi.spyOn(api, "getPlaybookProposal").mockResolvedValue({ + proposal_id: payload.proposal_id, + status: "pending", + playbook_id: "testpb", + base_yaml: baseYaml, + new_yaml: newYaml, + }); + render(); + expect(await screen.findByText("current → proposed")).toBeInTheDocument(); + expect((await screen.findByTestId("diff-new")).textContent).toBe(newYaml); + expect(screen.getByTestId("diff-old").textContent).toBe(baseYaml); + expect(screen.getByRole("button", { name: /approve/i })).toBeInTheDocument(); + }); + + it("shows no new-vs-update label while the body is still loading", async () => { + vi.spyOn(api, "getPlaybookProposal").mockReturnValue(new Promise(() => {})); + render(); + expect(await screen.findByText(/loading diff/i)).toBeInTheDocument(); + expect(screen.queryByText("new playbook")).not.toBeInTheDocument(); + expect(screen.queryByText("current → proposed")).not.toBeInTheDocument(); + }); + + it("treats an empty fetched base as a new playbook", async () => { + vi.spyOn(api, "getPlaybookProposal").mockResolvedValue({ + proposal_id: payload.proposal_id, + status: "pending", + playbook_id: "testpb", + new_yaml: newYaml, + }); + render(); + expect(await screen.findByText("new playbook")).toBeInTheDocument(); + }); + + it("prefers bodies already present on the payload", async () => { + const spy = vi.spyOn(api, "getPlaybookProposal").mockResolvedValue({ + proposal_id: payload.proposal_id, + status: "pending", + }); + render( + , + ); + expect((await screen.findByTestId("diff-new")).textContent).toBe(newYaml); + expect(spy).toHaveBeenCalledOnce(); + }); + + it("fills in a missing base from the server when the payload only has new_yaml", async () => { + vi.spyOn(api, "getPlaybookProposal").mockResolvedValue({ + proposal_id: payload.proposal_id, + status: "pending", + playbook_id: "testpb", + base_yaml: baseYaml, + new_yaml: "id: other\n", + }); + render( + , + ); + expect(await screen.findByText("current → proposed")).toBeInTheDocument(); + expect(screen.getByTestId("diff-old").textContent).toBe(baseYaml); + expect(screen.getByTestId("diff-new").textContent).toBe(newYaml); + }); + + it("does not call a pending proposal resolved when the fetch fails transiently", async () => { + vi.spyOn(api, "getPlaybookProposal").mockRejectedValue(new Error("network down")); + render(); + expect(await screen.findByText(/couldn't load the proposal diff/i)).toBeInTheDocument(); + expect(screen.queryByText(/no longer available/i)).not.toBeInTheDocument(); + expect(screen.getByRole("button", { name: /approve/i })).toBeInTheDocument(); + }); + + it("explains when a resolved proposal's body is no longer available", async () => { + vi.spyOn(api, "getPlaybookProposal").mockResolvedValue({ + proposal_id: payload.proposal_id, + status: "declined", + }); + render(); + expect(await screen.findByText(/declined\./)).toBeInTheDocument(); + expect(screen.getByText(/no longer available/i)).toBeInTheDocument(); + expect(screen.queryByTestId("diff-stub")).not.toBeInTheDocument(); + }); +}); + +describe("parsePlaybookProposal", () => { + it("accepts a tool result that carries only the ids", () => { + const out = parsePlaybookProposal({ proposal_id: "p1", playbook_id: "testpb" }); + expect(out?.key).toBe("testpb"); + expect(out?.payload.proposal_id).toBe("p1"); + }); +}); diff --git a/frontend/components/playbooks/ProposalCard.tsx b/frontend/components/playbooks/ProposalCard.tsx index a5ccae70..94f6fc35 100644 --- a/frontend/components/playbooks/ProposalCard.tsx +++ b/frontend/components/playbooks/ProposalCard.tsx @@ -7,16 +7,26 @@ import { ProposalBodyTabs } from "@/components/playbooks/ProposalBodyTabs"; // Payload returned by the triagent-strategies/playbook_proposal_draft tool. // Keep this in lockstep with proposePlaybookDraftOut in -// mcp/internal/strategies/tools_proposal.go. +// pkg/mcp/strategies/tools_proposal.go. The tool result identifies the +// draft only; the YAML bodies are optional because the card fetches them +// from GET /api/playbook-proposals/{id} (the tool result lives in the +// model's context and is size-capped there). Callers that already hold +// the bodies (the editor's own fetch) may pass them to skip the wait. export type ProposalDraftPayload = { proposal_id: string; playbook_id: string; base_yaml?: string; - new_yaml: string; + new_yaml?: string; why?: string; message?: string; }; +// ProposalBody is the diff pair the card renders once it has it. +export type ProposalBody = { + base_yaml?: string; + new_yaml: string; +}; + type Status = // Initial state on mount: we ask the server whether the proposal is // still pending before showing any actions, so an old chat reloaded @@ -51,6 +61,8 @@ type Props = { // proposal tab, lifted-state surfaces) can clean up. Optional — // the chat-only usage doesn't need it. onResolved?: (kind: "approved" | "declined") => void; + // Initial body tab. Forwarded to ProposalBodyTabs. + defaultTab?: "diagram" | "yaml"; }; export function ProposalCard({ @@ -58,21 +70,43 @@ export function ProposalCard({ onSendRefinement, dismissed = false, onResolved, + defaultTab, }: Props) { const [status, setStatus] = useState({ kind: "checking" }); const [refinement, setRefinement] = useState(""); - const isNew = !payload.base_yaml || payload.base_yaml.trim() === ""; + // The diff pair. Seeded from the payload when the caller already has + // it; otherwise filled in by the mount fetch below while the draft is + // still pending. Resolved proposals have no draft on disk any more, + // so a card re-mounted after approve/decline may never get a body. + const [body, setBody] = useState(() => + typeof payload.new_yaml === "string" + ? { base_yaml: payload.base_yaml, new_yaml: payload.new_yaml } + : null, + ); + // Unknown until the body is known: the header shows no + // new-vs-update label while hydrating rather than guessing "new". + const isNew = body ? !body.base_yaml || body.base_yaml.trim() === "" : null; // On mount, ask the server whether this proposal is still pending. // Reloading a chat after the operator approved/declined elsewhere // would otherwise show stale Approve/Decline buttons; the server - // tracks the outcome via a tiny resolution ledger. + // tracks the outcome via a tiny resolution ledger. The same response + // carries the diff bodies for a pending draft. useEffect(() => { let cancelled = false; (async () => { try { const res = await api.getPlaybookProposal(payload.proposal_id); if (cancelled) return; + if (typeof res.new_yaml === "string") { + const fetched = { base_yaml: res.base_yaml, new_yaml: res.new_yaml }; + // A payload that already carried new_yaml keeps it, but a + // missing base is filled in from the server so the card + // doesn't call an update "new playbook". + setBody((prev) => + prev ? { base_yaml: prev.base_yaml ?? fetched.base_yaml, new_yaml: prev.new_yaml } : fetched, + ); + } if (res.status === "approved") { setStatus({ kind: "approved", @@ -169,9 +203,11 @@ export function ProposalCard({ {payload.playbook_id} - - {isNew ? "new playbook" : "current → proposed"} - + {isNew !== null && ( + + {isNew ? "new playbook" : "current → proposed"} + + )}
@@ -180,7 +216,26 @@ export function ProposalCard({ )}
- + {body ? ( + + ) : status.kind === "checking" ? ( +
+ loading diff… +
+ ) : status.kind === "pending" ? ( + // Still pending but the fetch didn't deliver a body (a + // transient error fell back to pending). The draft exists; + // only this load failed. +
+ Couldn't load the proposal diff. Reload the page to retry; the + draft is still pending. +
+ ) : ( +
+ The proposal body is no longer available — the draft was + removed when the proposal was resolved. +
+ )}
{/* Footer: status / actions. Dismissed proposals (handled diff --git a/frontend/lib/events.ts b/frontend/lib/events.ts index eedd8f6f..a245b6f9 100644 --- a/frontend/lib/events.ts +++ b/frontend/lib/events.ts @@ -171,7 +171,8 @@ export const SUMMARIZE_TOOL_NAME = "mcp__triagent-strategies__summarize"; // Wire-format name of the playbook-proposal draft tool. SessionView // renders this tool's result inline as a diff card with Approve / // Decline controls; the structured-output JSON carries -// {proposal_id, base_yaml, new_yaml, …}. The agent +// {proposal_id, playbook_id, …} and the card fetches the diff bodies +// from GET /api/playbook-proposals/{id}. The agent // is instructed (via the playbook_proposal meta-playbook + system // prompt) to call this preemptively, no chat-side ask required. export const PROPOSE_PLAYBOOK_DRAFT_TOOL_NAME = diff --git a/internal/server/handlers_proposals.go b/internal/server/handlers_proposals.go index 1f0fbc3a..2c2ecb6d 100644 --- a/internal/server/handlers_proposals.go +++ b/internal/server/handlers_proposals.go @@ -3,6 +3,7 @@ package server import ( "encoding/json" "errors" + "fmt" "net/http" "os" "path/filepath" @@ -10,6 +11,7 @@ import ( "strings" "time" + "github.com/sourcehawk/triagent/pkg/mcp/strategies" "gopkg.in/yaml.v3" ) @@ -185,9 +187,13 @@ func (a *apiHandlers) handleGetProposal(w http.ResponseWriter, r *http.Request) return } // Determine base YAML from the currently-loaded playbook set so - // the diff renders against what's actually live (system embedded + // the diff renders against what's actually live (plugin, system, // or user override). - baseYAML := loadBaseForID(a.opts.UserPlaybooksDir, playbookID) + baseYAML, err := loadBaseForID(a.opts, playbookID) + if err != nil { + writeError(w, http.StatusInternalServerError, err.Error()) + return + } writeJSON(w, http.StatusOK, map[string]any{ "proposal_id": proposalID, "playbook_id": playbookID, @@ -340,17 +346,28 @@ func readDraftFromDir(dir, proposalID string) (playbookID, body string, err erro return "", "", os.ErrNotExist } -// loadBaseForID returns the YAML of the currently-saved user playbook -// with the given id (single-file layout: /.yaml). Returns "" -// for a brand-new id or one that only exists in the system set (no user -// file yet). -func loadBaseForID(dir, id string) string { - if groups, err := loadUserPlaybookGroups(dir); err == nil { - if g, ok := groups[id]; ok { - return g.YAML - } +// loadBaseForID returns the canonical YAML of the playbook the running +// strategies MCP would resolve for id: the same plugin → user → system +// merge the MCP loads at startup (user overrides plugin; the locked +// system tier wins over both), re-rendered through the shared +// serialiser so the proposal diff surfaces semantic deltas rather than +// on-disk formatting. Returns "" for a brand-new id, which the diff card +// renders as a new playbook; a set that fails to load is an error, not an +// empty base, so an update is never mislabelled as new. +func loadBaseForID(opts Options, id string) (string, error) { + books, err := strategies.LoadPlaybooksFrom(opts.PluginPlaybooksDir, opts.SystemPlaybooksDir, opts.UserPlaybooksDir) + if err != nil { + return "", fmt.Errorf("load playbook set: %w", err) + } + pb, ok := books[id] + if !ok { + return "", nil + } + rendered, err := strategies.RenderPlaybookYAML(pb) + if err != nil { + return "", fmt.Errorf("render base playbook %s: %w", id, err) } - return "" + return rendered, nil } // proposalResolution is the outcome marker we persist when a playbook diff --git a/internal/server/handlers_proposals_test.go b/internal/server/handlers_proposals_test.go index b0aceb09..f2bf6d32 100644 --- a/internal/server/handlers_proposals_test.go +++ b/internal/server/handlers_proposals_test.go @@ -235,3 +235,56 @@ func TestHandleGetProposal_PendingHasStatusField(t *testing.T) { assert.Equal(t, "pending", body["status"]) assert.Contains(t, body["new_yaml"].(string), "broker-crashloop", "new_yaml missing draft body") } + +// The chat card diffs against this endpoint's base_yaml (the tool +// result no longer inlines it), so the base must mirror the strategies +// MCP's loaded set: a system-tier playbook with no user override is +// still a real base, and it is rendered canonically so the diff shows +// semantic deltas rather than the on-disk file's formatting. +func TestHandleGetProposal_BaseComesFromLoadedSetRenderedCanonically(t *testing.T) { + t.Parallel() + userDir := t.TempDir() + systemDir := t.TempDir() + const pb = "id: broker-crashloop\nschema_version: 1\nsymptom: 'broker restarts'\nentrypoint: a\nnodes:\n a:\n description: a\n terminal_advice: done\n" + require.NoError(t, os.MkdirAll(filepath.Join(systemDir, "investigation"), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(systemDir, "investigation", "broker-crashloop.yaml"), []byte(pb), 0o644)) + typeDir := filepath.Join(userDir, "proposals", "investigation") + require.NoError(t, os.MkdirAll(typeDir, 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(typeDir, "broker-crashloop__prop-ffffffffffff.yaml"), []byte(pb), 0o644)) + + a := &apiHandlers{opts: Options{UserPlaybooksDir: userDir, SystemPlaybooksDir: systemDir}} + rec := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodGet, "/api/playbook-proposals/prop-ffffffffffff", nil) + req.SetPathValue("id", "prop-ffffffffffff") + a.handleGetProposal(rec, req) + + require.Equal(t, http.StatusOK, rec.Code, "body %s", rec.Body.String()) + var body map[string]any + require.NoError(t, json.Unmarshal(rec.Body.Bytes(), &body)) + base, _ := body["base_yaml"].(string) + assert.Contains(t, base, "broker restarts", "system-tier playbook must serve as the diff base") + assert.NotContains(t, base, "symptom: ", "base must be canonically rendered, not the raw file") +} + +// A playbook set that fails to load must surface as an error, not as an +// empty base: an empty base renders the proposal as a brand-new playbook, +// which would silently mislabel an update. +func TestHandleGetProposal_LoadFailureIsAnError(t *testing.T) { + t.Parallel() + userDir := t.TempDir() + typeDir := filepath.Join(userDir, "proposals", "investigation") + require.NoError(t, os.MkdirAll(typeDir, 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(typeDir, "broker-crashloop__prop-aaaaaaaaaaaa.yaml"), []byte("id: broker-crashloop\n"), 0o644)) + // A YAML at the user dir root violates the /.yaml layout and + // makes the loader fail. + require.NoError(t, os.WriteFile(filepath.Join(userDir, "stray.yaml"), []byte("id: stray\n"), 0o644)) + + a := &apiHandlers{opts: Options{UserPlaybooksDir: userDir}} + rec := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodGet, "/api/playbook-proposals/prop-aaaaaaaaaaaa", nil) + req.SetPathValue("id", "prop-aaaaaaaaaaaa") + a.handleGetProposal(rec, req) + + assert.Equal(t, http.StatusInternalServerError, rec.Code, "body %s", rec.Body.String()) + assert.Contains(t, rec.Body.String(), "load playbook set") +} diff --git a/pkg/mcp/strategies/playbook.go b/pkg/mcp/strategies/playbook.go index 2a0433ac..e25ad7ba 100644 --- a/pkg/mcp/strategies/playbook.go +++ b/pkg/mcp/strategies/playbook.go @@ -875,7 +875,7 @@ func WriteUserPlaybook(dir, typeName, id, body string, activate bool) (validatio } pb.Version = "" // strip the legacy field pb.Active = boolPtr(activate) // launcher's intent overrides the body - rendered, err := renderPlaybookYAML(pb) + rendered, err := RenderPlaybookYAML(pb) if err != nil { return nil, fmt.Errorf("render yaml: %w", err) } @@ -1053,13 +1053,13 @@ func randRead(b []byte) (int, error) { return cryptoRand.Read(b) } -// renderPlaybookYAML re-serialises a parsed Playbook back to YAML. +// RenderPlaybookYAML re-serialises a parsed Playbook back to YAML. // Used by get_playbook_raw for user playbooks (we only kept their raw // bytes for the embedded set; user files round-trip through the // parser). Lossy on comments/exact whitespace but semantically // identical, which is sufficient for an agent reading the current // playbook as a base for a proposed update. -func renderPlaybookYAML(pb *Playbook) (string, error) { +func RenderPlaybookYAML(pb *Playbook) (string, error) { body, err := yaml.Marshal(pb) if err != nil { return "", fmt.Errorf("marshal playbook: %w", err) diff --git a/pkg/mcp/strategies/playbook_test.go b/pkg/mcp/strategies/playbook_test.go index 73abe977..ff31ea5a 100644 --- a/pkg/mcp/strategies/playbook_test.go +++ b/pkg/mcp/strategies/playbook_test.go @@ -361,7 +361,7 @@ func TestRenderPlaybookYAML_OmitsEmptyVersion(t *testing.T) { "a": {Description: "step"}, }, } - out, err := renderPlaybookYAML(pb) + out, err := RenderPlaybookYAML(pb) require.NoError(t, err) // Use "\nversion:" (line-start anchor) to avoid false-matching // "schema_version:" — consistent with TestWriteUserPlaybook_StripsVersion. diff --git a/pkg/mcp/strategies/server.go b/pkg/mcp/strategies/server.go index eeb92036..dd98e7d5 100644 --- a/pkg/mcp/strategies/server.go +++ b/pkg/mcp/strategies/server.go @@ -190,7 +190,7 @@ func (s *Server) register() { mcp.AddTool(s.impl, &mcp.Tool{ Name: "playbook_proposal_draft", - Description: "Submit a draft playbook for the operator's inline review. Writes the YAML to a draft store; the launcher's chat UI renders a diff card vs the currently-loaded version, and the operator approves or declines via that card. NO chat-side confirmation is required before this call — the agent should call it as soon as it has a candidate. On approve, the launcher writes the proposal body to //.yaml (overwriting any existing user file) and records a git commit in the user dir's repo with the operator's chosen message (or an auto-generated one). The version field is not stamped — git history is the version axis. Returns the proposal_id (operator-facing UI uses it) and base_yaml/new_yaml (for the diff view).", + Description: "Submit a draft playbook for the operator's inline review. Writes the YAML to a draft store; the launcher's chat UI renders a diff card vs the currently-loaded version, and the operator approves or declines via that card. NO chat-side confirmation is required before this call — the agent should call it as soon as it has a candidate. On approve, the launcher writes the proposal body to //.yaml (overwriting any existing user file) and records a git commit in the user dir's repo with the operator's chosen message (or an auto-generated one). The version field is not stamped — git history is the version axis. Returns the proposal_id (operator-facing UI uses it to fetch and render the diff); the YAML bodies are not echoed back.", }, telemetry.Wrap("playbook_proposal_draft", s.proposePlaybookDraft)) mcp.AddTool(s.impl, &mcp.Tool{ diff --git a/pkg/mcp/strategies/tools_proposal.go b/pkg/mcp/strategies/tools_proposal.go index db3815fd..497426ec 100644 --- a/pkg/mcp/strategies/tools_proposal.go +++ b/pkg/mcp/strategies/tools_proposal.go @@ -233,7 +233,7 @@ func (s *Server) getPlaybookRaw(ctx context.Context, req *mcp.CallToolRequest, i } // User playbook: re-serialise the parsed form. Round-trips cleanly // for our own validator but loses comments/exact whitespace. - rendered, err := renderPlaybookYAML(pb) + rendered, err := RenderPlaybookYAML(pb) if err != nil { return errorResult(fmt.Sprintf("render user playbook %q: %v", in.ID, err)), getPlaybookRawOut{}, nil } @@ -275,21 +275,18 @@ type proposePlaybookDraftIn struct { 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"` } -// proposePlaybookDraftOut intentionally carries everything the -// frontend's diff card needs in one round-trip: the proposal id (so -// the approve/decline POST can target it), the base YAML to diff -// against, and the new YAML the agent is proposing. The tool result -// is still the structured-output JSON, which the frontend unwraps -// (same pattern as summarize). A separate `/api/playbook-proposals/` -// endpoint exists for refetch, but the inline payload means the diff -// renders as soon as the tool result lands. +// proposePlaybookDraftOut identifies the draft; it deliberately does not +// carry the YAML bodies. The tool result lands in the model's context and +// is subject to the CLI's per-result size cap, and a real playbook is tens +// of KB per side, so inlining base + new would blow the cap and replace the +// JSON with an error string the chat UI can't parse. The frontend's diff +// card fetches both bodies from `GET /api/playbook-proposals/` using +// the proposal_id. type proposePlaybookDraftOut struct { ProposalID string `json:"proposal_id"` PlaybookID string `json:"playbook_id"` Type string `json:"type"` // the type slot the draft was filed under BaseVersion string `json:"base_version,omitempty"` // empty when this is a brand-new id - BaseYAML string `json:"base_yaml,omitempty"` // empty when no current version exists - NewYAML string `json:"new_yaml"` Why string `json:"why,omitempty"` Message string `json:"message"` // ValidationErrors lists structural validator failures when the supplied @@ -336,7 +333,7 @@ func (s *Server) proposePlaybookDraft(ctx context.Context, req *mcp.CallToolRequ // canonical diff so the operator doesn't see the agent's value // and assume the AI is making that call. pb.Active = nil - canonicalNew, err := renderPlaybookYAML(pb) + canonicalNew, err := RenderPlaybookYAML(pb) if err != nil { return errorResult(fmt.Sprintf("render canonical proposal yaml: %v", err)), proposePlaybookDraftOut{}, nil } @@ -348,18 +345,11 @@ func (s *Server) proposePlaybookDraft(ctx context.Context, req *mcp.CallToolRequ return errorResult("validation failed: " + strings.Join(writeErrs, "; ")), proposePlaybookDraftOut{}, nil } - // Resolve the current loaded version (if any) to feed the diff - // view. New ids return empty base fields — the UI handles that. - // Both sides go through renderPlaybookYAML so the diff only - // surfaces semantic deltas, not formatting / quoting / wrap - // noise from the agent's input vs the upstream's on-disk - // representation. - var baseVersion, baseYAML string + // Report the currently loaded version (if any) so the agent knows + // whether it proposed an update or a brand-new id. + var baseVersion string if existing, ok := s.playbooks[pb.ID]; ok { baseVersion = existing.Version - if rendered, err := renderPlaybookYAML(existing); err == nil { - baseYAML = rendered - } } msg := fmt.Sprintf("Proposal %s queued for %s/%s. The operator reviews + approves in the chat panel; nothing is loaded into the running playbook set until then.", proposalID, in.Type, pb.ID) @@ -371,8 +361,6 @@ func (s *Server) proposePlaybookDraft(ctx context.Context, req *mcp.CallToolRequ PlaybookID: pb.ID, Type: in.Type, BaseVersion: baseVersion, - BaseYAML: baseYAML, - NewYAML: canonicalNew, Why: in.Why, Message: msg, }, nil diff --git a/pkg/mcp/strategies/tools_proposal_test.go b/pkg/mcp/strategies/tools_proposal_test.go index 7e5efc6c..0ed13f6f 100644 --- a/pkg/mcp/strategies/tools_proposal_test.go +++ b/pkg/mcp/strategies/tools_proposal_test.go @@ -2,6 +2,7 @@ package strategies import ( "context" + "encoding/json" "testing" "github.com/stretchr/testify/assert" @@ -76,3 +77,38 @@ nodes: assert.Empty(t, out.ValidationErrors) assert.NotEmpty(t, out.ProposalID) } + +func TestProposePlaybookDraft_ResultOmitsYAMLBodies(t *testing.T) { + t.Parallel() + srv := newServerWithUserPlaybooksDir(t) + yaml := `id: testpb +schema_version: 1 +symptom: test +entrypoint: a +nodes: + a: + description: a + terminal_advice: done +` + _, out, err := srv.proposePlaybookDraft(context.Background(), nil, proposePlaybookDraftIn{ + YAML: yaml, + Type: "investigation", + Why: "test", + }) + require.NoError(t, err) + require.NotEmpty(t, out.ProposalID) + + // The tool result lands in the model's context. A real playbook is + // tens of KB per side, and inlining both diff bodies pushed the result + // past the CLI's per-result cap, which replaces the JSON with an error + // string the chat UI can't parse. The launcher serves the bodies via + // GET /api/playbook-proposals/{id}; the result only identifies the draft. + raw, err := json.Marshal(out) + require.NoError(t, err) + var keys map[string]any + require.NoError(t, json.Unmarshal(raw, &keys)) + assert.NotContains(t, keys, "new_yaml") + assert.NotContains(t, keys, "base_yaml") + assert.Equal(t, "testpb", keys["playbook_id"]) + assert.Equal(t, "investigation", keys["type"]) +} diff --git a/pkg/mcp/strategies/walker_test.go b/pkg/mcp/strategies/walker_test.go index c5188a77..3f4ef733 100644 --- a/pkg/mcp/strategies/walker_test.go +++ b/pkg/mcp/strategies/walker_test.go @@ -683,10 +683,10 @@ nodes: require.Empty(t, errs) require.Equal(t, "round_trip_target", pb.Nodes["a"].DelegateTo) - rendered, err := renderPlaybookYAML(pb) + rendered, err := RenderPlaybookYAML(pb) require.NoError(t, err) assert.Contains(t, rendered, "delegate_to: round_trip_target", - "renderPlaybookYAML must preserve delegate_to") + "RenderPlaybookYAML must preserve delegate_to") // And the rendered output must itself parse + validate cleanly. pb2, errs2 := ParseAndValidatePlaybookYAML([]byte(rendered))