fix(strategies): render playbook proposals in chat regardless of playbook size

[sourcehawk]

Description

Playbook proposals for real-sized playbooks never appeared in the chat transcript: the agent's draft landed on disk, but the operator saw a raw tool card instead of the diff/approve card. The playbook_proposal_draft tool result inlined base_yaml + new_yaml (~100KB for a real playbook), Claude Code's per-result cap replaced that JSON with an "exceeds maximum allowed tokens" string, and that string is what the launcher persisted and the card tried to JSON.parse. This PR drops the YAML bodies from the tool result and has ProposalCard fetch them from GET /api/playbook-proposals/{id}, which it already called on mount for resolution status. A card re-mounted for an already-resolved proposal now shows a "body no longer available" note rather than a diff, since the draft file is gone by then.

Changes

• playbook_proposal_draft returns only proposal_id, playbook_id, type, base_version, why, message; the tool description says so.
• ProposalCard hydrates the diff from the proposal endpoint; payloads that already carry bodies (the editor's own fetch) are used directly, with a missing base filled in from the server. The header's new-vs-update label waits until the body is known, and a pending proposal whose fetch fails shows a "couldn't load the diff, reload to retry" notice instead of the resolved wording. ProposalBodyTabs takes a body prop; parsePlaybookProposal no longer requires new_yaml.
• GET /api/playbook-proposals/{id} resolves base_yaml from the same plugin → user → system loaded set the strategies MCP uses (system is locked and wins) and renders it through the now-exported strategies.RenderPlaybookYAML, so system-tier playbooks diff against their real base and the diff is free of on-disk formatting noise (previously it read only the raw user file). A set that fails to load is a 500, never an empty base, so an update is never mislabelled as a new playbook.

Related

• propose_wiki_draft still inlines base_md/new_md in its tool result; wiki entries are typically much smaller, but a very large one would hit the same cap. Follow-up.
• The proposal endpoint walks the tiered playbook set per request, matching GET /api/playbooks/{id}. A shared, write-invalidated cache across both handlers is a possible follow-up if the library grows large.

Testing

Go tests pin the contracts: the tool result JSON has no new_yaml/base_yaml keys, the proposal endpoint serves a canonically rendered base for a system-tier playbook with no user override, and a set that fails to load yields a 500. ProposalCard.test.tsx covers hydration from the endpoint, the loading state, the new-playbook case, payload-body precedence and base merging, the transient-failure and resolved fallbacks, and the relaxed parser. go test -race ./..., golangci-lint, the full vitest suite, npm run typecheck, and the Playbook|Proposal e2e subset all pass. Reproduced from the failing session's events.jsonl (seq 822 holds the CLI error string as the tool result); worth poking at the diff card on a proposal against a system playbook, which previously rendered as "new playbook".

🤖 Generated with Claude Code

https://claude.ai/code/session_019EvSnbmYgttST3ZXayYQsT

[Copilot]

🟡 Changes recommended

The updated ProposalCard can display a misleading “body no longer available” message for pending proposals on transient fetch failures and the hydration logic should merge missing base YAML even when the payload already includes new YAML.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Fixes oversized playbook_proposal_draft tool results that previously broke the chat diff card (YAML bodies exceeded the CLI per-result cap, leading to persisted non-JSON strings and JSON.parse failures). The PR shifts diff hydration to GET /api/playbook-proposals/{id} and ensures the proposal endpoint computes a canonical base_yaml consistent with the strategies playbook loader so diffs are stable and accurate for system-tier playbooks.

Changes:

• Stop inlining base_yaml/new_yaml in the playbook_proposal_draft tool result; return identifiers + metadata only.
• Update the proposal endpoint to load the resolved playbook base (plugin/user/system tiers) and render it canonically via exported strategies.RenderPlaybookYAML.
• Update the frontend proposal card to fetch bodies from the proposal endpoint, relax proposal parsing, and add targeted Go + Vitest coverage.

File summaries

File	Description
pkg/mcp/strategies/walker_test.go	Updates tests to use exported RenderPlaybookYAML.
pkg/mcp/strategies/tools_proposal.go	Removes YAML bodies from tool output; continues canonical rendering for stored draft.
pkg/mcp/strategies/tools_proposal_test.go	Adds contract test ensuring tool result omits YAML body fields.
pkg/mcp/strategies/server.go	Updates tool description to reflect fetch-based diff hydration.
pkg/mcp/strategies/playbook.go	Exports RenderPlaybookYAML for reuse outside the package.
pkg/mcp/strategies/playbook_test.go	Updates tests for exported YAML renderer.
internal/server/handlers_proposals.go	Proposal endpoint now computes canonical base from resolved loaded set and uses shared renderer.
internal/server/handlers_proposals_test.go	Adds coverage ensuring system-tier bases are served and canonically rendered.
frontend/lib/events.ts	Updates inline docs for the propose tool payload shape and hydration path.
frontend/components/playbooks/ProposalCard.tsx	Hydrates diff bodies from proposal endpoint; adds resolved-body fallback UI and tab forwarding.
frontend/components/playbooks/ProposalCard.test.tsx	Adds tests for hydration, precedence, resolved fallback, and relaxed parsing.
frontend/components/playbooks/ProposalBodyTabs.tsx	Refactors to accept a body prop rather than the full tool payload.
frontend/components/playbooks/PlaybookEditor.reducer.ts	Relaxes proposal parsing to accept tool results without YAML bodies.

Review details

Suppressed comments (1)

frontend/components/playbooks/ProposalCard.tsx:213

• When status.kind is set to "pending" on a transient fetch error (see catch block), and body is still null, the card shows the "body is no longer available" resolved message. That message is misleading for pending proposals and can confuse operators during temporary network/server issues.

        {body ? (
          <ProposalBodyTabs body={body} defaultTab={defaultTab} />
        ) : status.kind === "checking" ? (
          <div className="flex items-center gap-2 px-2 py-3 text-xs text-zinc-500">
            <Spinner className="h-3 w-3" /> loading diff…

• Files reviewed: 13/13 changed files
• Comments generated: 2
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[sourcehawk]

Review round 1, suppressed finding disposition:

• ProposalCard.tsx:213 (pending proposal + transient fetch error showed the "body no longer available" resolved wording): applied in 95bfb27. A pending card with no body now shows a "couldn't load the proposal diff, reload to retry" notice; the resolved wording only appears for approved/declined/unknown states. Test added.

[Copilot]

🔵 Needs a closer look

The proposal GET handler now reloads/parses the entire tiered playbook set per request and silently falls back to an empty base on load/render errors, which risks noticeable UI latency and misleading “new playbook” diffs.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

internal/server/handlers_proposals.go:355

• loadBaseForID reloads and fully parses the entire tiered playbook set (strategies.LoadPlaybooksFrom(...)) on every GET /api/playbook-proposals/{id}. With a real upstream playbooks clone this can be a lot of filesystem IO/YAML parsing per proposal-card mount, and any load/render error currently degrades silently to base_yaml == "", which makes the UI render the proposal as a “new playbook” rather than showing a retryable error state.

Consider caching the merged playbook map (e.g., per-server with a short TTL / invalidation on write) or resolving/rendering just the single base playbook needed for playbookID, and propagate load/render errors as non-200 so the client can show its “Couldn't load diff” fallback instead of a misleading empty base.

func loadBaseForID(opts Options, id string) string {
	books, err := strategies.LoadPlaybooksFrom(opts.PluginPlaybooksDir, opts.SystemPlaybooksDir, opts.UserPlaybooksDir)
	if err != nil {
		return ""
	}

• Files reviewed: 13/13 changed files
• Comments generated: 0 new
• Review effort level: Lite

[sourcehawk]

Review round 2, suppressed finding disposition (handlers_proposals.go:355, two parts):

• Silent empty base on load/render error: applied. loadBaseForID now returns the error and the handler answers 500, so the card shows its "couldn't load the proposal diff" notice instead of mislabelling an update as a new playbook. Test added.
• Per-request reload of the tiered set / caching: declined. GET /api/playbooks/{id} already walks every tier per request via collectPlaybooks() (plus two git log subprocesses), so this handler matches the launcher's existing pattern; a proposal-card mount is a rare, operator-driven event and the set is small. A shared cache with write invalidation would be a cross-handler change and is out of scope here.

[Copilot]

🟡 Changes recommended

There’s at least one confirmed UI-state bug in ProposalCard during hydration plus a per-request full playbook reload in the proposal endpoint that may need mitigation before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

internal/server/handlers_proposals.go:360

• loadBaseForID reloads and parses the entire tiered playbook set on every GET /api/playbook-proposals/{id}. Since ProposalCard calls this endpoint on mount, this can become noticeably slow as the playbook library grows. Consider caching the merged set (with a short TTL / invalidation on user playbook writes) or adding a targeted resolver that loads only the requested id while preserving tier precedence/locking semantics.

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)

• Files reviewed: 13/13 changed files
• Comments generated: 1
• Review effort level: Lite

[sourcehawk]

Review round 3, suppressed finding disposition (handlers_proposals.go:360, per-request reload of the tiered set): same point as round 2, declined for the same reason. GET /api/playbooks/{id} already walks every tier per request via collectPlaybooks(); this handler follows that pattern, the card mount is a rare operator-driven event, and a shared cache with write invalidation is a cross-handler change outside this fix.

[Copilot]

🟢 Approval recommended

The changes match the stated failure mode, keep tiered base resolution consistent with the strategies loader, and add backend/frontend tests that pin the new contracts.

Review details

• Files reviewed: 13/13 changed files
• Comments generated: 0 new
• Review effort level: Lite