-
Notifications
You must be signed in to change notification settings - Fork 963
feat: add support to apply slash command #244
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughThe Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (3)
test/core/init.test.ts (1)
1054-1133: Good coverage for fresh vs extend detection.The new cases correctly assert configured flags across scenarios. Consider adding explicit marker-gating tests in extend mode to lock in the new logic:
- CLAUDE.md exists without markers + no slash cmd markers ⇒ configured === false
- Slash cmd file with markers exists but CLAUDE.md without markers ⇒ configured === false (for tools requiring both)
Also, you can DRY repeated choice lookups with a tiny helper:
const getChoice = (callIdx: number, value: string) => mockPrompt.mock.calls[callIdx][0].choices.find((c: any) => c.value === value);src/core/init.ts (2)
631-651: Extend-mode gating in getExistingToolStates prevents false positives.Only compute configured states when OpenSpec is initialized; otherwise default to false. This matches the intended UX.
A small trace (debug-level) per tool when extendMode is true would help diagnose why a tool isn’t detected as configured (e.g., “missing markers in CLAUDE.md” vs “no slash commands found”). Keep it behind a quiet flag to avoid noisy output.
657-719: Marker-based configured-state detection is solid; consider a tiny helper to reduce duplication.You check for start/end markers twice (config and slash files). Extracting a local hasMarkers(content: string) makes this easier to keep consistent and test.
Apply example refactor:
+ private hasMarkers(content: string): boolean { + return content.includes(OPENSPEC_MARKERS.start) && content.includes(OPENSPEC_MARKERS.end); + } + private async isToolConfigured( projectPath: string, toolId: string ): Promise<boolean> { @@ - if (await FileSystemUtils.fileExists(configPath)) { + if (await FileSystemUtils.fileExists(configPath)) { try { const content = await FileSystemUtils.readFile(configPath); - hasConfigFile = content.includes(OPENSPEC_MARKERS.start) && content.includes(OPENSPEC_MARKERS.end); + hasConfigFile = this.hasMarkers(content); } catch { // If we can't read the file, treat it as not configured hasConfigFile = false; } } @@ if (await FileSystemUtils.fileExists(absolute)) { try { const content = await FileSystemUtils.readFile(absolute); - if (content.includes(OPENSPEC_MARKERS.start) && content.includes(OPENSPEC_MARKERS.end)) { + if (this.hasMarkers(content)) { hasSlashCommands = true; break; // Found at least one valid slash command } } catch { // If we can't read the file, continue checking othersOptional: when both are required, you could surface which prerequisite failed in a debug channel to aid users during extend runs.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/core/configurators/slash/opencode.ts(1 hunks)src/core/init.ts(3 hunks)test/core/init.test.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-21T03:05:07.846Z
Learnt from: CR
PR: Fission-AI/OpenSpec#0
File: AGENTS.md:0-0
Timestamp: 2025-10-21T03:05:07.846Z
Learning: Use @/openspec/AGENTS.md to learn how to create and apply change proposals
Applied to files:
src/core/configurators/slash/opencode.ts
🧬 Code graph analysis (1)
src/core/init.ts (4)
src/core/config.ts (2)
AI_TOOLS(19-34)OPENSPEC_MARKERS(3-6)src/core/configurators/registry.ts (1)
ToolRegistry(7-37)src/utils/file-system.ts (1)
FileSystemUtils(44-187)src/core/configurators/slash/registry.ts (1)
SlashCommandRegistry(16-60)
🔇 Additional comments (3)
src/core/configurators/slash/opencode.ts (1)
25-30: Apply template now accepts arguments; looks good.Frontmatter closes correctly and the new guidance plus $ARGUMENTS meets the PR goal.
If the apply command expects a change identifier (not free-form), consider using a wrapper for parity with archive and other tools.
Optional diff:
-The user has requested to implement the following change proposal. Find the change proposal and follow the instructions below. If you're not sure or if ambiguous, ask for clarification from the user. -<UserRequest> - $ARGUMENTS -</UserRequest> +The user has requested to implement the following change proposal. Locate the change by ID and follow the instructions below. If unsure or ambiguous, ask for clarification. +<ChangeId> + $ARGUMENTS +</ChangeId>src/core/init.ts (2)
24-25: Importing OPENSPEC_MARKERS is correct and necessary.
392-392: Passing extendMode into getExistingToolStates is the right call.Prevents false “already configured” signals during first-time init.
This change adds the $ARGUMENTS property to the OpenCode apply slash command template, allowing users to pass arguments when invoking the apply command. The template now includes instructions for the agent to find and implement the change proposal, with guidance to ask for clarification when ambiguous.
45266b6 to
ba75a35
Compare
Summary
$ARGUMENTSproperty to the OpenCode apply slash command templateTest plan
openspec initoropenspec update🤖 Generated with Claude Code
Summary by CodeRabbit