feat(claude): install sdd slash commands#352
Conversation
There was a problem hiding this comment.
Pull request overview
This PR enables managed SDD slash-command installation for the Claude agent by adding Claude-specific command assets and making command asset lookup agent-aware across install/uninstall and golden tests.
Changes:
- Enabled slash-command support for the Claude adapter and defined
~/.claude/commandsas its commands directory. - Added Claude-formatted SDD command assets (
internal/assets/claude/commands/sdd-*.md) and golden fixtures for them. - Centralized agent-aware embedded command asset selection via
assets.SDDCommandsAssetDir()and updated SDD inject/uninstall to use it.
Reviewed changes
Copilot reviewed 29 out of 29 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
internal/agents/claude/adapter.go |
Turns on slash-command support and returns Claude commands dir path. |
internal/agents/claude/adapter_test.go |
Adds coverage asserting slash-command support and commands dir. |
internal/assets/commands.go |
Adds agent-aware command asset directory resolver (Claude vs default). |
internal/assets/claude/commands/sdd-*.md |
New Claude-native SDD slash commands. |
internal/assets/assets_test.go |
Extends embedded-asset layout/readability checks to include Claude commands. |
internal/components/sdd/inject.go |
Loads slash-command assets from agent-specific embedded directory. |
internal/components/sdd/inject_test.go |
Verifies Claude command files are written and contain Claude frontmatter. |
internal/components/uninstall/service.go |
Removes managed command files using agent-specific embedded directory. |
internal/components/uninstall/service_test.go |
Ensures Claude uninstall removes only managed commands and preserves user commands. |
internal/components/sdd/commands.go |
Updates OpenCode command enumeration to include sdd-explore. |
internal/components/sdd/commands_test.go |
Tightens command-set expectations to the full 9-command SDD set. |
internal/components/golden_test.go |
Adds Claude command golden assertions; ensures OpenCode expected command set includes sdd-onboard. |
testdata/golden/sdd-claude-cmd-*.golden |
New golden fixtures for the Claude command set. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| CONTEXT: | ||
| - Working directory: !`echo -n "$(pwd)"` | ||
| - Current project: !`echo -n "$(basename $(pwd))"` |
There was a problem hiding this comment.
The shell substitution uses basename $(pwd) without quoting the pwd output. If the working directory path contains spaces, basename will receive multiple arguments and the project name extraction can fail. Prefer quoting (e.g., basename "$(pwd)" or basename "$PWD") inside the substitution.
| - Current project: !`echo -n "$(basename $(pwd))"` | |
| - Current project: !`echo -n "$(basename "$(pwd)")"` |
|
|
||
| CONTEXT: | ||
| - Working directory: !`echo -n "$(pwd)"` | ||
| - Current project: !`echo -n "$(basename $(pwd))"` |
There was a problem hiding this comment.
The Current project shell substitution uses basename $(pwd) without quoting; this breaks when the path contains spaces. Quote the pwd result (or use $PWD) so command rendering is robust.
| - Current project: !`echo -n "$(basename $(pwd))"` | |
| - Current project: !`echo -n "$(basename "$PWD")"` |
|
|
||
| CONTEXT: | ||
| - Working directory: !`echo -n "$(pwd)"` | ||
| - Current project: !`echo -n "$(basename $(pwd))"` |
There was a problem hiding this comment.
The Current project line uses basename $(pwd) without quoting, which can fail for working directories containing spaces. Quote the pwd output (or use $PWD) inside the substitution.
| - Current project: !`echo -n "$(basename $(pwd))"` | |
| - Current project: !`echo -n "$(basename "$PWD")"` |
|
|
||
| CONTEXT: | ||
| - Working directory: !`echo -n "$(pwd)"` | ||
| - Current project: !`echo -n "$(basename $(pwd))"` |
There was a problem hiding this comment.
The shell substitution uses basename $(pwd) without quoting. In directories with spaces this can produce the wrong project name or error. Quote the pwd output (or use $PWD) inside the basename call.
| - Current project: !`echo -n "$(basename $(pwd))"` | |
| - Current project: !`echo -n "$(basename "$PWD")"` |
|
|
||
| CONTEXT: | ||
| - Working directory: !`echo -n "$(pwd)"` | ||
| - Current project: !`echo -n "$(basename $(pwd))"` |
There was a problem hiding this comment.
The Current project shell substitution uses basename $(pwd) without quoting; this breaks for paths with spaces. Quote the pwd output (or use $PWD) in the basename call.
| - Current project: !`echo -n "$(basename $(pwd))"` | |
| - Current project: !`echo -n "$(basename "$(pwd)")"` |
|
|
||
| CONTEXT: | ||
| - Working directory: !`echo -n "$(pwd)"` | ||
| - Current project: !`echo -n "$(basename $(pwd))"` |
There was a problem hiding this comment.
The Current project shell substitution uses basename $(pwd) without quoting; if the path contains spaces, basename can mis-handle arguments. Quote the pwd output (or use $PWD) inside the basename call.
| - Current project: !`echo -n "$(basename $(pwd))"` | |
| - Current project: !`echo -n "$(basename "$(pwd)")"` |
|
|
||
| CONTEXT: | ||
| - Working directory: !`echo -n "$(pwd)"` | ||
| - Current project: !`echo -n "$(basename $(pwd))"` |
There was a problem hiding this comment.
The Current project shell substitution uses basename $(pwd) without quoting; paths with spaces will break. Quote the pwd output (or use $PWD) in the basename call.
| - Current project: !`echo -n "$(basename $(pwd))"` | |
| - Current project: !`echo -n "$(basename "$PWD")"` |
|
|
||
| CONTEXT: | ||
| - Working directory: !`echo -n "$(pwd)"` | ||
| - Current project: !`echo -n "$(basename $(pwd))"` |
There was a problem hiding this comment.
The Current project shell substitution uses basename $(pwd) without quoting. This can fail when the working directory contains spaces; quote the pwd output (or use $PWD) inside the basename call.
| - Current project: !`echo -n "$(basename $(pwd))"` | |
| - Current project: !`echo -n "$(basename "$PWD")"` |
|
|
||
| CONTEXT: | ||
| - Working directory: !`echo -n "$(pwd)"` | ||
| - Current project: !`echo -n "$(basename $(pwd))"` |
There was a problem hiding this comment.
The Current project shell substitution uses basename $(pwd) without quoting; directories with spaces can cause incorrect parsing. Quote the pwd output (or use $PWD) inside the basename call.
| - Current project: !`echo -n "$(basename $(pwd))"` | |
| - Current project: !`echo -n "$(basename "$PWD")"` |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 30 out of 30 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 35 out of 35 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // GitHub Actions injects a repository-scoped GITHUB_TOKEN into CI. When that | ||
| // token is forwarded into our Linux E2E containers, the public engram releases | ||
| // endpoint can respond 401/403 for a different repository. Retry anonymously | ||
| // before failing because the release metadata is public. | ||
| if token != "" && (status == http.StatusUnauthorized || status == http.StatusForbidden) { | ||
| version, _, retryErr := fetchLatestEngramVersionRequest("") | ||
| if retryErr == nil { | ||
| return version, nil | ||
| } | ||
| } | ||
|
|
||
| return "", err | ||
| } |
There was a problem hiding this comment.
In fetchLatestEngramVersion(), if the token-authenticated request returns 401/403 and the anonymous retry fails for a different reason, the function currently returns the original error from the first attempt. That can be misleading and make troubleshooting harder. Consider returning the retry error (or wrapping both errors) when the retry is attempted and fails.
| func fetchLatestEngramVersion() (string, error) { | ||
| token := githubToken() | ||
| version, status, err := fetchLatestEngramVersionRequest(token) | ||
| if err == nil { | ||
| return version, nil | ||
| } | ||
|
|
||
| // GitHub Actions injects a repository-scoped GITHUB_TOKEN into CI. When that | ||
| // token is forwarded into our Linux E2E containers, the public engram releases | ||
| // endpoint can respond 401/403 for a different repository. Retry anonymously | ||
| // before failing because the release metadata is public. | ||
| if token != "" && (status == http.StatusUnauthorized || status == http.StatusForbidden) { | ||
| version, _, retryErr := fetchLatestEngramVersionRequest("") | ||
| if retryErr == nil { | ||
| return version, nil | ||
| } | ||
| } | ||
|
|
||
| return "", err | ||
| } |
There was a problem hiding this comment.
The PR title/description are focused on Claude SDD slash commands, but this change also modifies engram download behavior (GitHub API retry logic) and introduces new Qwen setup semantics. Consider splitting these engram-related changes into a separate PR (or update the PR description/linked issue to explicitly cover them) so the review scope matches the stated intent and risk surface.
🔗 Linked Issue
Closes #337
🏷️ PR Type
What kind of change does this PR introduce?
type:bug— Bug fix (non-breaking change that fixes an issue)type:feature— New feature (non-breaking change that adds functionality)type:docs— Documentation onlytype:refactor— Code refactoring (no functional changes)type:chore— Build, CI, or tooling changestype:breaking-change— Breaking change (fix or feature that changes existing behavior)📝 Summary
~/.claude/commands.📂 Changes
internal/agents/claude/adapter.gointernal/assets/claude/commands/*internal/assets/commands.gointernal/components/sdd/*internal/components/uninstall/*testdata/golden/sdd-claude-cmd-*.golden🧪 Test Plan
Unit Tests
go test ./internal/components/sdd ./internal/components/uninstall ./internal/components ./internal/assets ./internal/agents/claudeE2E Tests (Docker required)
go test ./...)cd e2e && ./docker-test.sh)🤖 Automated Checks
The following checks run automatically on this PR:
Closes/Fixes/Resolves #Nstatus:approvedtype:*Labeltype:*label must be applied✅ Contributor Checklist
status:approvedtype:*label to this PRgo test ./...)cd e2e && ./docker-test.sh)Co-Authored-Bytrailers💬 Notes for Reviewers