fix(strategies): keep playbook tools up when a user id spans two type dirs - #89
Merged
sourcehawk merged 1 commit intoSep 2, 2026
Merged
Conversation
…dirs loadUserDir returned an error when one id had a copy under two type dirs, and that error failed the whole strategies MCP at startup. Every new session then reported the server as CONNECTION_CLOSED and lost all playbook tools. A vault in that state is what an approve from before WriteUserPlaybook refused the second slot leaves behind, so any operator who approved a cross-type revision on an older build was locked out until they hand-repaired the directory. Keep the copy in the first type dir (directory order is stable) and soft-skip the later one with a stderr warning naming both files, matching how the loader already treats invalid or legacy files. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The change is narrowly scoped, preserves existing warning/soft-skip behavior patterns, and includes a focused regression test for the previously fatal startup condition.
Pull request overview
This PR hardens the strategies playbook loader so that a user playbook vault containing the same playbook id under two different type directories no longer causes the entire strategies MCP to fail at startup. Instead, the loader deterministically keeps the first copy encountered (directory order) and soft-skips later duplicates with a stderr warning, preserving tool availability for new sessions.
Changes:
- Update
loadUserDirto soft-skip duplicate IDs spanning multiple type dirs (warn + continue) rather than returning an error. - Add a regression test covering the duplicate-ID-across-type-dirs scenario to ensure load succeeds and selection is deterministic.
File summaries
| File | Description |
|---|---|
| pkg/mcp/strategies/playbook.go | Changes duplicate-ID handling in loadUserDir to warn and continue instead of failing the loader. |
| pkg/mcp/strategies/playbook_test.go | Adds a test ensuring an ID duplicated across two type dirs doesn’t break load and that the “first dir wins” behavior holds. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- 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
deleted the
fix/strategies-loader-tolerates-duplicate-type-dirs
branch
September 2, 2026 12:19
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
An id with a copy under two type dirs in the user playbook vault made
loadUserDirreturn an error, and that error failed the whole strategies MCP at startup, so every new session reportedtriagent-strategiesasCONNECTION_CLOSEDand lost all playbook tools. This PR keeps the copy in the first type dir (directory order is stable) and soft-skips the later one with a stderr warning naming both files, the same treatment the loader already gives invalid and legacy files. The stray copy stays on disk until the operator deletes it; the warning tells them which one.Changes
loadUserDirsoft-skips a second copy of an id under another type dir instead of failing the load.Related
Testing
TestLoadPlaybooksFrom_IdInTwoTypeDirsbuilds a vault withinvestigation/dup.yaml,verification/dup.yamland a neighbour, and asserts the load succeeds, the investigation copy wins, and the neighbour still loads. Reproduced the original failure by running the v0.4.0triagent-mcp serve --kind=strategieswith a real session's env against a vault in this state.make test-goandmake lintpass.🤖 Generated with Claude Code