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

[sourcehawk]

Description

A proposal sub-agent could file a revision of an existing playbook under a different type than the one the id already lives in, and approve would then write a second <type>/<id>.yaml next to the original. The launcher's list showed whichever copy sorted last (the stale one, matching upstream, so push-PR reported no changes), and the strategies loader refused the whole user dir because one id spanned two type dirs, taking every strategies tool down for new sessions on that profile. This PR pins a revision to the slot its id already lives in at draft time and at write time, and gives the agent the slot to use. A vault that already holds the duplicate still has to be repaired by hand (move the newer file over the older one); the launcher's list loader keeps its last-dir-wins behaviour on such a vault.

Changes

• playbook_proposal_draft rejects a revision whose type differs from the slot the existing id lives in, naming the right slot in the error.
• get_playbook_raw returns the type slot, and returns the active user override rather than the upstream copy so a revision does not silently discard local edits on approve.
• WriteUserPlaybook (approve and editor save) refuses to write an id under a second type dir.
• The proposal playbook's read_existing_for_revision node and the tool descriptions tell the agent the revision keeps the id and the slot; that node also no longer describes the retired version-bump scheme.

Related

• Follow-up: the launcher's loadUserPlaybookGroups still silently picks the last type dir when an id spans two, while the strategies loader hard-fails. A hand-edited vault would get inconsistent handling; the list should surface it as a broken entry.

Testing

Reproduced against a real camunda-profile vault where an approved proposal had landed under investigation/ next to the existing verification/ copy: the list resolved to the stale copy and reported synced, and the strategies loader errored with "exists in multiple type dirs". Three new tests cover the guards: the draft tool rejecting a mismatched slot and accepting the matching one, get_playbook_raw reporting the slot and preferring the user override over the plugin copy, and WriteUserPlaybook refusing a second type dir without writing a file. All three failed before the fix, and the get_playbook_raw test caught the override-precedence bug on its own. make test and make lint are clean; no frontend changes.

🤖 Generated with Claude Code

https://claude.ai/code/session_01MWHCbR64zubr1cf6epZZuD

[Copilot]

🟡 Changes recommended

get_playbook_raw can mislabel and lossy-render locked system-tier playbooks because it doesn’t attempt to read raw YAML from systemPlaybooksDir before falling back to re-serialization.

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

Pull request overview

This PR prevents playbook revisions from being drafted/approved into a different “type slot” than the one an existing playbook ID already occupies, which previously could create duplicate <type>/<id>.yaml files and cause the strategies loader to hard-fail (taking strategies tools down for new sessions).

Changes:

• Enforces “same id, same type slot” for revisions at draft time (playbook_proposal_draft) and at write time (WriteUserPlaybook).
• Extends get_playbook_raw to return the type slot and to prefer the active user override when present (so approvals don’t silently discard local edits).
• Updates proposal playbook guidance + tool description to explicitly instruct agents to keep the same slot and rely on git history for versioning.

File summaries

File	Description
system/playbook_proposal.yaml	Updates proposal-flow instructions to keep revisions in the existing type slot and treat git history as the version record.
pkg/mcp/strategies/tools_proposal.go	Adds slot reporting + user-override preference in get_playbook_raw; rejects draft revisions filed under a mismatched type slot.
pkg/mcp/strategies/tools_proposal_test.go	Adds regression tests covering slot mismatch rejection and get_playbook_raw slot/override behavior.
pkg/mcp/strategies/specs.go	Updates the get_playbook_raw tool description to mention returning the type slot and slot pinning for revisions.
pkg/mcp/strategies/playbook.go	Prevents writing the same playbook ID under multiple type directories via WriteUserPlaybook guard.
pkg/mcp/strategies/playbook_test.go	Adds a test ensuring WriteUserPlaybook rejects cross-type duplication and doesn’t write the second file.

Review details

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

---

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

[Copilot]

🟡 Changes recommended

get_playbook_raw can return bytes from an invalid/skipped user override file (misreporting Source="user"), so it should validate the on-disk YAML before preferring it.

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

Review details

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

[Copilot]

🟢 Approval recommended

The draft-time and write-time guards align with the loader’s single-type-per-id invariant, and the new tests cover the previously failing scenarios.

Review details

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