Fix OpenCode command directory handling#675
Fix OpenCode command directory handling#675JcMinarro wants to merge 6 commits intoFission-AI:mainfrom
Conversation
Use the documented .opencode/commands path with legacy migration and update-only behavior to prevent mismatched locations and unintended command creation.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
📝 WalkthroughWalkthroughRename OpenCode command directory from Changes
Sequence Diagram(s)sequenceDiagram
participant Flow as Init/Update Flow
participant FS as FileSystem
participant User as User Prompt
Flow->>FS: Check for `.opencode/command/`
FS-->>Flow: Exists / NotFound
alt legacy exists
Flow->>FS: Ensure `.opencode/commands/` exists
Flow->>FS: For each file: move or copy+unlink to `.opencode/commands/`
FS-->>Flow: Files moved / overwritten
Flow->>FS: Is legacy dir empty?
FS-->>Flow: Empty / NotEmpty
alt Empty and canPrompt
Flow->>User: Prompt to delete `.opencode/command/`?
User-->>Flow: Confirm / Decline
alt Confirm
Flow->>FS: Delete `.opencode/command/`
FS-->>Flow: Deleted
else Decline
Flow-->>Flow: Keep legacy dir
end
else
Flow-->>Flow: Skip prompt
end
end
Flow->>Flow: Continue generate/update command files
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@openspec/changes/archive/2026-02-06-fix-opencode-commands-dir/tasks.md`:
- Around line 17-18: The numbered checklist skips 3.2 between the entries "3.1
Update any tests or fixtures that assert the OpenCode command directory path"
and "3.3 Run existing tests related to OpenSpec init/update and ensure they
pass"; either renumber "3.3" to "3.2" or add a short NOTE line explaining that
3.2 was intentionally removed (and why) so the sequence is clear; update the
checklist text accordingly to keep numbering consistent and pass linters/readers
that expect sequential task numbers.
In `@src/core/shared/opencode-migration.ts`:
- Around line 17-26: The migration currently prompts to delete an empty legacy
directory even when no files were moved (causing duplicate prompts on a second
invocation); modify the migration function in opencode-migration.ts to track
whether any files were actually moved (e.g., introduce a movedCount or moved
boolean updated inside the loop that processes entries) and change the cleanup
prompt condition to only ask to delete the legacy directory when movedCount > 0
(or moved === true) AND remaining.length === 0; alternatively, if entries.length
=== 0 at the start, return early to avoid the prompt. Refer to the existing
variables entries, remaining and the prompt/cleanup logic to locate where to add
the check.
In `@src/core/update.ts`:
- Around line 66-68: The file currently calls migrateOpenCodeCommands twice:
once unconditionally via await migrateOpenCodeCommands(resolvedProjectPath,
isInteractive()) and again inside the per-tool handling for the "opencode" tool,
causing duplicate prompts; remove the early/unconditional invocation (the
standalone await migrateOpenCodeCommands(...) call) and keep the migration call
that runs inside the per-tool loop (the "opencode" handling) so migration
happens only when the opencode tool is processed, using the existing
migrateOpenCodeCommands function and isInteractive() flag.
🧹 Nitpick comments (4)
test/core/command-generation/adapters.test.ts (1)
483-495: Good coverage for the archive frontmatter; consider adding a negative assertion for non-archive commands.The positive test for archive-specific
$ARGUMENTSfrontmatter is well-structured. To prevent regressions where archive arguments accidentally leak into non-archive commands, consider addingexpect(output).not.toContain('arguments:')to the existing non-archive format test (around line 449–455).🧪 Suggested negative assertion in the existing non-archive test
it('should format file with description frontmatter', () => { const output = opencodeAdapter.formatFile(sampleContent); expect(output).toContain('---\n'); expect(output).toContain('description: Enter explore mode for thinking'); expect(output).toContain('---\n\n'); expect(output).toContain('This is the command body.'); + expect(output).not.toContain('arguments:'); });src/core/init.ts (1)
434-434:canPromptis recomputed on every loop iteration but never changes.
this.canPromptInteractively()depends only on instance fields and environment state, neither of which changes between loop iterations. Consider hoisting this above theforloop to avoid redundant calls (it's already computed at line 97).src/core/update.ts (1)
116-116:canPromptcomputed inside the loop but is loop-invariant.Same as in
init.ts—isInteractive()doesn't depend on loop state. Consider hoisting it above the loop.test/core/update.test.ts (1)
149-178: Missing test coverage formigrateOpenCodeCommands.There is no test in this file (or elsewhere in the repository) that verifies the migration behavior:
- Moving files from
.opencode/command/→.opencode/commands/- Handling the case where the legacy directory doesn't exist (no-op)
- The cleanup prompt when the legacy directory is emptied
- The double-invocation scenario (called in both
execute()andhandleLegacyCleanup())Consider adding dedicated tests for the migration function, either here or in a new test file.
openspec/changes/archive/2026-02-06-fix-opencode-commands-dir/tasks.md
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Aligns OpenCode command generation/update behavior with the documented .opencode/commands/ directory, adds archive frontmatter arguments for OpenCode, and introduces a migration step for legacy .opencode/command/ content during init/update.
Changes:
- Update OpenCode adapter output path to
.opencode/commands/and add archive frontmatterargumentscontent. - Update
openspec updateto refresh only existing command files (skip creating missing ones) and add OpenCode legacy command directory migration. - Refresh specs/docs and adjust tests to reflect the new update semantics and OpenCode path.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/core/update.test.ts | Adjusts command update tests to pre-create command files (since update now only refreshes existing files). |
| test/core/command-generation/adapters.test.ts | Updates OpenCode expected path and adds a test for archive frontmatter arguments. |
| src/core/update.ts | Adds OpenCode legacy command migration and skips writing command files that don’t already exist. |
| src/core/shared/opencode-migration.ts | New helper to migrate .opencode/command → .opencode/commands with optional cleanup prompt. |
| src/core/shared/index.ts | Exports the new OpenCode migration helper. |
| src/core/init.ts | Runs OpenCode migration during init when configuring the OpenCode tool. |
| src/core/command-generation/adapters/opencode.ts | Changes OpenCode command output directory and injects archive frontmatter arguments. |
| openspec/specs/cli-update/spec.md | Updates spec references to .opencode/commands/ and archive $ARGUMENTS requirement. |
| openspec/changes/archive/2026-02-06-fix-opencode-commands-dir/tasks.md | Archives work tracking for the change. |
| openspec/changes/archive/2026-02-06-fix-opencode-commands-dir/specs/cli-update/spec.md | Archived delta spec capturing the behavior change. |
| openspec/changes/archive/2026-02-06-fix-opencode-commands-dir/proposal.md | Archived proposal describing why/what changed. |
| openspec/changes/archive/2026-02-06-fix-opencode-commands-dir/design.md | Archived design/migration plan for OpenCode commands dir change. |
| openspec/changes/archive/2026-02-06-fix-opencode-commands-dir/.openspec.yaml | Archived change metadata. |
| docs/supported-tools.md | Updates documented OpenCode commands directory to .opencode/commands/. |
Comments suppressed due to low confidence (1)
test/core/update.test.ts:215
- This test sets up command files and verifies they still exist after update, but it doesn't assert that update actually refreshed their contents. That makes the test pass even if the update loop skips writing or writes incorrect content. Consider asserting the updated content for at least one representative command (or checking marker/frontmatter expectations across the set).
it('should update all 9 opsx commands when tool is configured', async () => {
// Set up a configured tool
const skillsDir = path.join(testDir, '.claude', 'skills');
await fs.mkdir(path.join(skillsDir, 'openspec-explore'), {
recursive: true,
});
await fs.writeFile(
path.join(skillsDir, 'openspec-explore', 'SKILL.md'),
'old content'
);
const commandsDir = path.join(testDir, '.claude', 'commands', 'opsx');
await fs.mkdir(commandsDir, { recursive: true });
const commandIds = [
'explore',
'new',
'continue',
'apply',
'ff',
'sync',
'archive',
'bulk-archive',
'verify',
];
for (const cmdId of commandIds) {
await fs.writeFile(path.join(commandsDir, `${cmdId}.md`), 'old command');
}
await updateCommand.execute(testDir);
for (const cmdId of commandIds) {
const cmdFile = path.join(commandsDir, `${cmdId}.md`);
const exists = await FileSystemUtils.fileExists(cmdFile);
expect(exists).toBe(true);
}
});
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| await updateCommand.execute(testDir); | ||
|
|
||
| // Check opsx command files were created |
There was a problem hiding this comment.
The assertion comment says the opsx command files were "created", but the test now pre-creates the command file and update is intended to only refresh existing files. Update the comment to reflect the intended behavior (refreshed/updated) to avoid confusion when reading test failures.
| // Check opsx command files were created | |
| // Check opsx command files were updated/refreshed and still exist |
src/core/update.ts
Outdated
| if (tool.value === 'opencode') { | ||
| if (canPrompt) { | ||
| spinner.stop(); | ||
| } | ||
| await migrateOpenCodeCommands(resolvedProjectPath, canPrompt); | ||
| if (canPrompt) { | ||
| spinner.start(); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
migrateOpenCodeCommands is invoked twice during update (once before legacy cleanup and again inside the per-tool loop for opencode). If .opencode/command is empty, this can lead to duplicate prompts to delete the directory in a single run. Consider migrating in only one place (e.g., only within the opencode tool block, or only once up-front) to avoid redundant work/prompting.
| if (tool.value === 'opencode') { | |
| if (canPrompt) { | |
| spinner.stop(); | |
| } | |
| await migrateOpenCodeCommands(resolvedProjectPath, canPrompt); | |
| if (canPrompt) { | |
| spinner.start(); | |
| } | |
| } |
| const sourcePath = path.join(legacyDir, entry.name); | ||
| const destinationPath = path.join(nextDir, entry.name); | ||
| if (await FileSystemUtils.fileExists(destinationPath)) { | ||
| await fs.promises.unlink(destinationPath); | ||
| } | ||
| await fs.promises.rename(sourcePath, destinationPath); | ||
| } |
There was a problem hiding this comment.
The migration currently deletes any existing destination file in .opencode/commands/ before moving the legacy file over. If a user already has newer/customized files in the new directory, this will irreversibly overwrite them. Safer options: skip files that already exist in the destination, or rename the legacy file (e.g., add a suffix) and/or prompt before overwriting.
Add focused migration tests and adjust update migration flow to avoid duplicate prompts while still upgrading legacy directories.
|
@1code review this PR |
|
Task completed. I now have a thorough picture of the PR. Here's my review: PR Review: Fix OpenCode command directory handling (#675)SummaryThis PR fixes a path mismatch where OpenSpec was generating OpenCode command files under Positive Aspects
IssuesBug:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/core/legacy-cleanup.ts`:
- Line 51: The init flow calls detectLegacyArtifacts and cleanupLegacyArtifacts
before migrateOpenCodeCommands so files still in `.opencode/command/` are
missed; reorder the calls in init.ts so migrateOpenCodeCommands runs first, then
call detectLegacyArtifacts and cleanupLegacyArtifacts afterwards (mirror the
sequence used in update.ts), ensuring migrateOpenCodeCommands executes prior to
any legacy detection or deletion.
In `@src/core/update.ts`:
- Around line 111-112: Remove the unused declaration "const canPrompt =
isInteractive();" from the tool-update loop in src/core/update.ts: delete that
statement (it is unused in the loop surrounding the update logic) and ensure
nothing else in the scope depends on the canPrompt variable or the side effects
of calling isInteractive(); if isInteractive() has necessary side effects, call
it where those effects are required instead of assigning to an unused variable.
🧹 Nitpick comments (1)
src/core/shared/opencode-migration.ts (1)
43-53: Minor: prompt is unreachable in non-interactive environments that passcanPrompt = false.This is fine by design, but worth noting: if
canPromptisfalse, the legacy directory will remain even when empty. Consider logging a message so the user knows cleanup is pending.💡 Optional: inform the user when auto-cleanup is skipped
const remaining = await fs.promises.readdir(legacyDir); if (changedAny && remaining.length === 0 && canPrompt) { const { confirm } = await import('@inquirer/prompts'); const shouldRemove = await confirm({ message: 'OpenCode commands have moved to .opencode/commands. The old .opencode/command directory is now empty and can be removed. Delete it?', default: true, }); if (shouldRemove) { await fs.promises.rm(legacyDir, { recursive: true, force: true }); } + } else if (changedAny && remaining.length === 0 && !canPrompt) { + // Non-interactive: auto-remove the empty legacy directory silently + await fs.promises.rm(legacyDir, { recursive: true, force: true }); }
- Reorder init.ts to call migrateOpenCodeCommands BEFORE handleLegacyCleanup to ensure legacy OpenCode commands are detected and cleaned up properly. This aligns with the correct execution order already present in update.ts. - Remove unused 'canPrompt' variable from update.ts tool-update loop (leftover from refactoring, variable was assigned but never referenced).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/core/init.ts`:
- Line 436: In generateSkillsAndCommands remove the unused local binding by
deleting the unused assignment "const canPrompt =
this.canPromptInteractively();" (or, if the intent was to gate logic, replace
usages to actually check canPrompt) so there are no unused variables; locate the
declaration in the generateSkillsAndCommands method and either remove that line
or use the canPrompt value where interactive prompting was intended, and run the
linter to ensure no other unused leftovers remain.
Remove the unused 'canPrompt' local binding from generateSkillsAndCommands method in init.ts. The variable was declared but never referenced.
2 participants
Summary
.opencode/commands/directory and migrate legacy.opencode/command/files during init/updateopenspec updateonly refreshes existing command files and adds OpenCode archive frontmatter argumentsReferences
Testing
Summary by CodeRabbit
Bug Fixes
New Features
Documentation
Tests