feat(cli): merge init and experimental commands #565
Conversation
Implement src/core/legacy-cleanup.ts with detection and cleanup functions for all legacy OpenSpec artifact types: Detection functions: - detectLegacyConfigFiles() - checks for config files with OpenSpec markers (CLAUDE.md, CLINE.md, CODEBUDDY.md, COSTRICT.md, QODER.md, IFLOW.md, AGENTS.md, QWEN.md) - detectLegacySlashCommands() - checks for old /openspec:* command directories and files across all 21 tool integrations - detectLegacyStructureFiles() - checks for openspec/AGENTS.md and openspec/project.md (project.md preserved for migration hint) - detectLegacyArtifacts() - orchestrates all detection Utility functions: - hasOpenSpecMarkers() - checks if content has OpenSpec markers - isOnlyOpenSpecContent() - checks if file is 100% OpenSpec content - removeMarkerBlock() - surgically removes marker blocks from mixed content Cleanup functions: - cleanupLegacyArtifacts() - orchestrates removal with proper edge cases: - Deletes files that are 100% OpenSpec content - Removes marker blocks from files with mixed content - Deletes legacy slash command directories and files - Preserves openspec/project.md (shows migration hint only) Formatting functions: - formatDetectionSummary() - formats what was detected before cleanup - formatCleanupSummary() - formats what was cleaned up after This is task 1.1 for the merge-init-experimental change.
…locks - Add removeMarkerBlock() function to file-system.ts that properly handles inline marker mentions by using findMarkerIndex/isMarkerOnOwnLine - Refactor legacy-cleanup.ts to use the shared utility - Export removeMarkerBlock from utils/index.ts for reusability - Add comprehensive tests for inline marker mention edge cases - Add tests for shell-style markers and various whitespace scenarios The new implementation correctly ignores markers mentioned inline within text and only removes actual marker blocks that are on their own lines.
- Add standalone formatProjectMdMigrationHint() function for reusable migration hint output directing users to migrate project.md content to config.yaml's "context:" field - Update formatDetectionSummary() to include the migration hint when project.md is detected (not just in cleanup summary) - Refactor formatCleanupSummary() to use the new function for consistency - Add unit tests for the new function and updated behavior
Rewrites the init command tests to verify the new experimental workflow implementation. The new tests cover: - OpenSpec directory structure creation (specs, changes, archive) - config.yaml generation with default schema - 9 Agent Skills creation for various tools (Claude, Cursor, Windsurf, etc.) - 9 slash commands generation using tool-specific adapters - Multi-tool support (--tools all, --tools none, specific tools) - Extend mode (re-running init) - Tool-specific adapters (Gemini TOML, Continue .prompt, etc.) - Error handling for invalid tools and permissions Removes old tests for legacy config file generation (AGENTS.md, CLAUDE.md, project.md, etc.) as the new init command uses Agent Skills instead.
Update the update command tests to match the new implementation that refreshes skills and opsx commands instead of config files. Changes: - Remove old ToolRegistry import (deleted module) - Rewrite tests to verify skill file updates - Rewrite tests to verify opsx command generation - Add tests for multi-tool support (Claude, Cursor, Qwen, Windsurf) - Add tests for error handling and tool detection - Fix test assertions to match actual skill template names The update command now: - Detects configured tools by checking skill directories - Updates SKILL.md files with latest skill templates - Generates opsx commands using tool-specific adapters
- Replace tool list with simplified supported tools section (skills-based) - Update init instructions to document --tools flag, --force, and legacy cleanup - Replace project.md with config.yaml documentation - Update workflow examples to use /opsx:* commands instead of /openspec:* - Add command reference table for slash commands - Update Team Adoption and Updating sections for new workflow - Replace Experimental Features with Workflow Customization section
…o workflow - Delete src/core/configurators/ directory (ToolRegistry, all config generators) - Delete legacy templates (agents-template, claude-template, project-template, etc.) - Move experimental commands to src/commands/workflow/ with cleaner structure - Remove experimental setup.ts and index.ts (functionality merged into init) - Update CLI to register workflow commands directly instead of through experimental - Update openspec update command to refresh skills/commands instead of config files - Update tests for new command structure
📝 WalkthroughWalkthroughConverts the experimental artifact workflow into an OPSX skill-and-slash-command model: removes per-tool configurators and registries, adds centralized skill/command templates and generators, introduces legacy artifact detection/cleanup, consolidates workflow CLI commands, and refactors init/update to a skills-dir–based, interactive/non-interactive flow. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as "CLI (src/cli/index.ts)"
participant Init as "InitCommand (src/core/init.ts)"
participant Shared as "Shared (tool-detection & skill-generation)"
participant Legacy as "LegacyCleanup (src/core/legacy-cleanup.ts)"
participant FS as "FileSystem / FileSystemUtils"
User->>CLI: run `openspec init [--tools] [--force]`
CLI->>Init: construct & execute InitCommand(options)
Init->>Shared: getToolStates(projectRoot)
Shared-->>Init: tool states
Init->>Legacy: detectLegacyArtifacts(projectRoot)
alt legacy artifacts found
Legacy-->>Init: detection result
Init->>User: prompt for cleanup (interactive) / require --force (non-interactive)
User-->>Init: confirm / pass --force
Init->>Legacy: cleanupLegacyArtifacts(projectRoot, detection)
Legacy-->>Init: cleanup summary
end
Init->>Shared: getSkillTemplates() & getCommandContents()
Shared-->>Init: templates & command contents
Init->>FS: write SKILL.md and command files under each tool's `skillsDir`
FS-->>Init: created/refreshed/errors
Init->>CLI: render success summary
CLI->>User: "OpenSpec Setup Complete" + next steps
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
Greptile SummaryThis PR successfully merges the experimental workflow into the main init command, unifying the setup experience while maintaining backward compatibility. The refactoring removes the legacy tool configurator system (ToolRegistry, wizard-based prompts) and replaces it with a cleaner skill-based approach that generates Agent Skills and Key changes:
Migration safety:
Testing: Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant CLI as CLI (index.ts)
participant Init as InitCommand
participant Legacy as legacy-cleanup
participant FS as FileSystem
participant Skills as Skill Generation
User->>CLI: openspec init --tools claude
CLI->>Init: execute(targetPath)
Init->>FS: Check if openspec/ exists
FS-->>Init: extendMode = true/false
Init->>Legacy: detectLegacyArtifacts()
Legacy->>FS: Check for config files (CLAUDE.md, AGENTS.md, etc.)
Legacy->>FS: Check for slash command dirs (.claude/commands/openspec/, etc.)
Legacy->>FS: Check for openspec/AGENTS.md
Legacy-->>Init: LegacyDetectionResult
alt Has legacy artifacts && interactive
Init->>User: Prompt: "Upgrade and clean up legacy files?"
User-->>Init: Confirm
Init->>Legacy: cleanupLegacyArtifacts()
Legacy->>FS: Delete/modify legacy files
Legacy-->>Init: CleanupResult
end
Init->>Init: getSelectedTools() with --tools flag
Init->>Init: validateTools()
Init->>FS: Create openspec/ structure
FS->>FS: mkdir specs, changes, changes/archive
Init->>Skills: generateSkillsAndCommands(claude)
Skills->>FS: Write .claude/skills/openspec-*/SKILL.md (9 files)
Skills->>FS: Write .claude/commands/opsx/*.md (9 files)
Skills-->>Init: Results
Init->>FS: Create openspec/config.yaml (if not exists)
Init->>User: Display success message
User->>User: Restart IDE for skills to load
|
Review CompleteYour review story is ready! Comment !reviewfast on this PR to re-generate the story. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@README.md`:
- Around line 262-264: Remove the shell prompt characters from the example
commands so markdownlint rule MD014 passes: edit the README.md code block lines
that currently contain "$ openspec list", "$ openspec validate
add-profile-filters", and "$ openspec show add-profile-filters" (and the similar
occurrence around line 288) to be plain commands "openspec list", "openspec
validate add-profile-filters", and "openspec show add-profile-filters" without
the leading "$ ".
In `@src/core/init.ts`:
- Around line 682-686: The status message always prints "openspec/config.yaml"
even when only openspec/config.yml exists; update the logging in init.ts (the
block using configStatus, DEFAULT_SCHEMA) to print the actual filename present:
query the result from createConfig() (or check fs.existsSync for
"openspec/config.yaml" vs "openspec/config.yml") and use that filename in the
console.log for both the 'created' and 'exists' branches so the message
accurately reflects the real file name.
In `@src/core/legacy-cleanup.ts`:
- Around line 129-136: The migration hint is not shown when only
openspec/project.md exists because result.hasLegacyArtifacts is computed without
including result.hasProjectMd; update the assignment that sets
result.hasLegacyArtifacts (the boolean expression combining result.configFiles,
result.slashCommandDirs, result.slashCommandFiles, result.hasOpenspecAgents,
result.hasRootAgentsWithMarkers) to also OR in result.hasProjectMd so the
presence of openspec/project.md triggers the migration hint.
- Around line 436-458: The code double-processes the root AGENTS.md and can
attempt to read/delete it after the legacy config cleanup already removed it;
update the AGENTS.md handling block (guarded by
detection.hasRootAgentsWithMarkers) to first verify the file still exists (e.g.,
via FileSystemUtils.exists or equivalent) before calling
FileSystemUtils.readFile/remove/write; reference the rootAgentsPath variable and
functions isOnlyOpenSpecContent, removeMarkerBlock, FileSystemUtils.readFile and
FileSystemUtils.writeFile so the check prevents attempts to process a file that
was already deleted and avoids the spurious "Failed to handle AGENTS.md" errors.
In `@src/utils/file-system.ts`:
- Around line 264-303: The function removeMarkerBlock currently uses trim()
which removes leading whitespace and normalizes line endings; change it to
preserve leading indentation and original newline style by replacing trim() with
trimEnd() (so leading spaces remain) and detect the file's newline sequence
(e.g., const newline = content.includes('\r\n') ? '\r\n' : '\n') then return
result.trimEnd() === '' ? '' : result.trimEnd() + newline; update references in
removeMarkerBlock and keep findMarkerIndex usage unchanged so only trailing
whitespace/newline handling is modified.
In `@test/core/legacy-cleanup.test.ts`:
- Around line 845-856: Replace the hardcoded expectedTools array with a dynamic
list derived from CommandAdapterRegistry.getAll(): call
CommandAdapterRegistry.getAll(), map to each adapter's id (or adapter.id) to
build expected IDs, then assert parity with LEGACY_SLASH_COMMAND_PATHS by
comparing keys (e.g., ensure every id exists in LEGACY_SLASH_COMMAND_PATHS and
lengths match or assert exact set equality). Refer to
CommandAdapterRegistry.getAll() and LEGACY_SLASH_COMMAND_PATHS (and adapter.id)
when locating the code to update.
🧹 Nitpick comments (5)
src/core/update.ts (1)
42-118: DRY up skill directory names to avoid drift.
SKILL_NAMESduplicates thedirNamelist inskillTemplates. If one list changes, detection and update can diverge. Consider a sharedSKILL_TEMPLATESconstant to drive both.♻️ Suggested refactor
-const SKILL_NAMES = [ - 'openspec-explore', - 'openspec-new-change', - 'openspec-continue-change', - 'openspec-apply-change', - 'openspec-ff-change', - 'openspec-sync-specs', - 'openspec-archive-change', - 'openspec-bulk-archive-change', - 'openspec-verify-change', -]; +const SKILL_TEMPLATES = [ + { dirName: 'openspec-explore', getTemplate: getExploreSkillTemplate }, + { dirName: 'openspec-new-change', getTemplate: getNewChangeSkillTemplate }, + { dirName: 'openspec-continue-change', getTemplate: getContinueChangeSkillTemplate }, + { dirName: 'openspec-apply-change', getTemplate: getApplyChangeSkillTemplate }, + { dirName: 'openspec-ff-change', getTemplate: getFfChangeSkillTemplate }, + { dirName: 'openspec-sync-specs', getTemplate: getSyncSpecsSkillTemplate }, + { dirName: 'openspec-archive-change', getTemplate: getArchiveChangeSkillTemplate }, + { dirName: 'openspec-bulk-archive-change', getTemplate: getBulkArchiveChangeSkillTemplate }, + { dirName: 'openspec-verify-change', getTemplate: getVerifyChangeSkillTemplate }, +]; +const SKILL_NAMES = SKILL_TEMPLATES.map((t) => t.dirName);- const skillTemplates = [ - { template: getExploreSkillTemplate(), dirName: 'openspec-explore' }, - { template: getNewChangeSkillTemplate(), dirName: 'openspec-new-change' }, - { template: getContinueChangeSkillTemplate(), dirName: 'openspec-continue-change' }, - { template: getApplyChangeSkillTemplate(), dirName: 'openspec-apply-change' }, - { template: getFfChangeSkillTemplate(), dirName: 'openspec-ff-change' }, - { template: getSyncSpecsSkillTemplate(), dirName: 'openspec-sync-specs' }, - { template: getArchiveChangeSkillTemplate(), dirName: 'openspec-archive-change' }, - { template: getBulkArchiveChangeSkillTemplate(), dirName: 'openspec-bulk-archive-change' }, - { template: getVerifyChangeSkillTemplate(), dirName: 'openspec-verify-change' }, - ]; + const skillTemplates = SKILL_TEMPLATES.map(({ dirName, getTemplate }) => ({ + dirName, + template: getTemplate(), + }));openspec/changes/merge-init-experimental/tasks.md (1)
1-67: Consider archiving this completed change set.
All tasks are checked off. Per repo workflow, move this change bundle toopenspec/changes/archive/2026-01-23-merge-init-experimental/(and update specs accordingly) once merged. Based on learnings, ...test/cli-e2e/basic.test.ts (1)
8-15: Consider usingFileSystemUtils.fileExistsinstead of duplicating.This local
fileExistshelper duplicates the static method inFileSystemUtils(seesrc/utils/file-system.tslines 83-93). While minor, using the shared utility would reduce duplication.♻️ Suggested refactor
import { runCLI, cliProjectRoot } from '../helpers/run-cli.js'; import { AI_TOOLS } from '../../src/core/config.js'; +import { FileSystemUtils } from '../../src/utils/file-system.js'; -async function fileExists(filePath: string): Promise<boolean> { - try { - await fs.access(filePath); - return true; - } catch { - return false; - } -} +const fileExists = FileSystemUtils.fileExists.bind(FileSystemUtils);test/core/init.test.ts (1)
401-417: Consider extracting test helpers to a shared module.The
fileExistsanddirectoryExistshelpers are duplicated across test files (also intest/cli-e2e/basic.test.ts). Consider extracting totest/helpers/for reuse.test/core/update.test.ts (1)
24-26: Redundantvi.restoreAllMocks()inbeforeEach.
vi.restoreAllMocks()is already called inafterEach(line 30), so calling it again inbeforeEachis redundant for the first test. However, it doesn't cause issues and provides defense-in-depth ifafterEachfails.♻️ Optional cleanup
beforeEach(async () => { // Create a temporary test directory testDir = path.join(os.tmpdir(), `openspec-test-${randomUUID()}`); await fs.mkdir(testDir, { recursive: true }); // Create openspec directory const openspecDir = path.join(testDir, 'openspec'); await fs.mkdir(openspecDir, { recursive: true }); updateCommand = new UpdateCommand(); - - // Clear all mocks before each test - vi.restoreAllMocks(); });
- Move AGENTS.md from openspec/ to project root - Add shared module with tool-detection and skill-generation utilities - Update legacy-cleanup with improved cleanup logic - Enhance update.ts with additional functionality - Add comprehensive tests for shared modules
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/core/init.ts`:
- Around line 536-542: The console messages use hard-coded counts ("9 skills and
9 commands" / "9 skills") which will diverge from reality; update the logging in
the block that computes hasCommands (uses hasCommands, results.commandsSkipped,
successfulTools, toolDirs) to compute the counts dynamically from the
template/contents arrays (e.g., use skillTemplates.length and
commandContents.length or equivalent) and interpolate those numeric values into
the strings so the output reflects actual counts.
♻️ Duplicate comments (3)
test/core/legacy-cleanup.test.ts (1)
904-915: Hardcoded expected tools list may drift from actual registry.The test hardcodes 21 tools and only verifies they exist in
LEGACY_SLASH_COMMAND_PATHSviatoHaveProperty(). If new adapters are added to the command registry, this test won't catch missing entries inLEGACY_SLASH_COMMAND_PATHS.src/core/legacy-cleanup.ts (1)
126-133: Migration hint can be skipped when onlyopenspec/project.mdexists.
hasLegacyArtifactsdoesn't includehasProjectMd, so the init flow won't surface the migration hint whenopenspec/project.mdis the only legacy artifact present. Based on the spec requirement (Lines 110-131 in spec.md), the migration hint should be displayed whenproject.mdexists.🔧 Suggested fix
result.hasLegacyArtifacts = result.configFiles.length > 0 || result.slashCommandDirs.length > 0 || result.slashCommandFiles.length > 0 || result.hasOpenspecAgents || - result.hasRootAgentsWithMarkers; + result.hasRootAgentsWithMarkers || + result.hasProjectMd;src/core/init.ts (1)
555-559: Config status message is inaccurate when onlyconfig.ymlexists.The message always prints
openspec/config.yamleven whenconfig.ymlis the file that exists. This was flagged in a prior review.
🧹 Nitpick comments (10)
src/core/shared/skill-generation.ts (1)
101-117: YAML special characters in template fields may produce invalid frontmatter.If
template.name,template.description, or other string fields contain YAML special characters (:,#,',", newlines), the generated frontmatter will be malformed. Consider quoting string values or using a YAML library for proper serialization.♻️ Suggested fix: quote string values
export function generateSkillContent( template: SkillTemplate, generatedByVersion: string ): string { return `--- -name: ${template.name} -description: ${template.description} +name: "${template.name.replace(/"/g, '\\"')}" +description: "${template.description.replace(/"/g, '\\"')}" license: ${template.license || 'MIT'} compatibility: ${template.compatibility || 'Requires openspec CLI.'} metadata: author: ${template.metadata?.author || 'openspec'} version: "${template.metadata?.version || '1.0'}" generatedBy: "${generatedByVersion}" --- ${template.instructions} `; }src/core/legacy-cleanup.ts (2)
185-196: Unused loop variabletoolId.The
toolIdvariable from theObject.entries()destructuring is not used in this loop iteration. Consider usingObject.values()instead.♻️ Suggested fix
- for (const [toolId, pattern] of Object.entries(LEGACY_SLASH_COMMAND_PATHS)) { + for (const pattern of Object.values(LEGACY_SLASH_COMMAND_PATHS)) { if (pattern.type === 'directory' && pattern.path) { const dirPath = FileSystemUtils.joinPath(projectPath, pattern.path); if (await FileSystemUtils.directoryExists(dirPath)) { directories.push(pattern.path); } } else if (pattern.type === 'files' && pattern.pattern) { // For file-based patterns, check for individual files const foundFiles = await findLegacySlashCommandFiles(projectPath, pattern.pattern); files.push(...foundFiles); } }
242-244: Silent catch block hides potential errors.The empty catch block at line 242 silently swallows all errors. While the comment explains the intent, unexpected errors (permission issues, symlink loops) will be hidden.
♻️ Consider logging at debug level
} catch { - // Directory doesn't exist or can't be read + // Directory doesn't exist or can't be read - safe to ignore + // Could add debug logging here if troubleshooting is needed }test/core/shared/skill-generation.test.ts (1)
11-14: Consider deriving expected template count programmatically.The tests hardcode
9as the expected template count in multiple places. If a template is added or removed, all these assertions need manual updates. Consider computing the expected count from a single source of truth or at least defining it as a constant.♻️ Suggested approach
// At top of describe block const EXPECTED_TEMPLATE_COUNT = 9; // Update in one place if count changes // Then use in tests expect(templates).toHaveLength(EXPECTED_TEMPLATE_COUNT);Alternatively, if strict count isn't critical, test for
> 0and focus on specific expected entries.Also applies to: 51-54, 80-83
test/core/update.test.ts (1)
556-569: Consider using a more robust version import pattern.The dynamic import of
package.jsonworks but may be fragile if the test file location changes. This pattern is used consistently throughout the test file, so it's acceptable for now.test/core/init.test.ts (1)
412-428: Helper functions duplicate FileSystemUtils functionality.These helper functions replicate
FileSystemUtils.fileExistsandFileSystemUtils.directoryExists. Consider importing from the utils module for consistency, though this is minor for test code.♻️ Consider using FileSystemUtils
+import { FileSystemUtils } from '../../src/utils/file-system.js'; -async function fileExists(filePath: string): Promise<boolean> { - try { - await fs.access(filePath); - return true; - } catch { - return false; - } -} - -async function directoryExists(dirPath: string): Promise<boolean> { - try { - const stats = await fs.stat(dirPath); - return stats.isDirectory(); - } catch { - return false; - } -} +// Use FileSystemUtils.fileExists and FileSystemUtils.directoryExists insteadsrc/core/update.ts (1)
350-396: Skill and command creation for upgraded tools mirrors the update loop.The code structure is nearly identical to the main update loop (lines 106-144). Consider extracting a shared helper to reduce duplication.
♻️ Consider extracting a shared tool update helper
The skill/command creation logic in
upgradeLegacyTools(lines 361-382) is very similar to the main update loop (lines 112-133). A shared helper function would reduce duplication:private async updateToolSkillsAndCommands( projectPath: string, toolId: string, skillTemplates: SkillTemplateEntry[], commandContents: CommandContent[] ): Promise<void> { const tool = AI_TOOLS.find((t) => t.value === toolId); if (!tool?.skillsDir) return; const skillsDir = path.join(projectPath, tool.skillsDir, 'skills'); for (const { template, dirName } of skillTemplates) { const skillDir = path.join(skillsDir, dirName); const skillFile = path.join(skillDir, 'SKILL.md'); const skillContent = generateSkillContent(template, OPENSPEC_VERSION); await FileSystemUtils.writeFile(skillFile, skillContent); } const adapter = CommandAdapterRegistry.get(tool.value); if (adapter) { const generatedCommands = generateCommands(commandContents, adapter); for (const cmd of generatedCommands) { const commandFile = path.join(projectPath, cmd.path); await FileSystemUtils.writeFile(commandFile, cmd.fileContent); } } }src/core/shared/tool-detection.ts (1)
167-168: Consider caching getToolSkillStatus call.
getToolSkillStatusis called twice for the same tool—once to getgeneratedByVersionloop condition (implicitly viafs.existsSync) and once explicitly on line 167. The status could be cached to avoid redundant checks.♻️ Minor optimization to avoid redundant status check
+ const status = getToolSkillStatus(projectRoot, toolId); + const configured = status.configured; + // Find the first skill file that exists and read its version - for (const skillName of SKILL_NAMES) { - const skillFile = path.join(skillsDir, skillName, 'SKILL.md'); - if (fs.existsSync(skillFile)) { - generatedByVersion = extractGeneratedByVersion(skillFile); - break; + if (configured) { + for (const skillName of SKILL_NAMES) { + const skillFile = path.join(skillsDir, skillName, 'SKILL.md'); + if (fs.existsSync(skillFile)) { + generatedByVersion = extractGeneratedByVersion(skillFile); + break; + } } } - const configured = getToolSkillStatus(projectRoot, toolId).configured; const needsUpdate = configured && (generatedByVersion === null || generatedByVersion !== currentVersion);src/core/init.ts (2)
171-188: Consider throwing errors instead of callingprocess.exit()directly.Using
process.exit()in class methods makes unit testing difficult and prevents graceful error handling by callers. The CLI layer should be responsible for exit codes.♻️ Suggested approach
if (!canPrompt) { // Non-interactive mode without --force: abort - console.log(chalk.red('Legacy files detected in non-interactive mode.')); - console.log(chalk.dim('Run interactively to upgrade, or use --force to auto-cleanup.')); - process.exit(1); + throw new Error( + 'Legacy files detected in non-interactive mode. ' + + 'Run interactively to upgrade, or use --force to auto-cleanup.' + ); } // Interactive mode: prompt for confirmation const { confirm } = await import('@inquirer/prompts'); const shouldCleanup = await confirm({ message: 'Upgrade and clean up legacy files?', default: true, }); if (!shouldCleanup) { - console.log(chalk.dim('Initialization cancelled.')); - console.log(chalk.dim('Run with --force to skip this prompt, or manually remove legacy files.')); - process.exit(0); + throw new Error( + 'Initialization cancelled. Run with --force to skip this prompt, or manually remove legacy files.' + ); }Then handle exit codes in the CLI entry point by catching these errors.
497-503: Silent error swallowing obscures config creation failures.If
serializeConfig()orwriteFile()throws, the error is silently discarded. Consider logging at debug level or including error details for troubleshooting.♻️ Suggested improvement
try { const yamlContent = serializeConfig({ schema: DEFAULT_SCHEMA }); await FileSystemUtils.writeFile(configPath, yamlContent); return 'created'; - } catch { + } catch (error) { + // Log for debugging but don't fail initialization + console.error(chalk.dim(`Config creation skipped: ${(error as Error).message}`)); return 'skipped'; }
Change from experimental reference to reflect the merged workflow.
- Handle both forward and backward slashes in path parsing - Normalize paths before regex matching for legacy artifact detection - Use regex split for both path separators in tool directory extraction - Handle CRLF line endings when cleaning up multiple blank lines - Add retry logic for test file cleanup to handle Windows file locking
Replace hard-coded "9 skills and 9 commands" with dynamic values from getSkillTemplates().length and getCommandContents().length to prevent the message from diverging from reality when skills/commands change.
- Remove shell prompt characters from README bash examples (MD014) - Show actual config filename (config.yaml vs config.yml) in init output - Include hasProjectMd in hasLegacyArtifacts to show migration hint - Add existence check before AGENTS.md deletion to avoid spurious errors - Preserve leading whitespace and original newline style in file operations - Use dynamic tool list from CommandAdapterRegistry in tests
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@README.md`:
- Around line 203-224: Update the example in openspec/config.yaml under the
rules.specs entry to reference OpenSpec's required scenario format: replace "Use
Given/When/Then format for scenarios" with guidance to use the "#### Scenario:"
header and the WHEN/THEN structure (e.g., "Use '#### Scenario:' headers and
WHEN/THEN format for scenarios") so the README's sample context matches OpenSpec
conventions.
🧹 Nitpick comments (4)
README.md (1)
199-201: Broaden “Restart your IDE” to “Restart your tool/client.”Claude Code and other non‑IDE tools may not map to “IDE”; consider wording that applies to all clients.
src/core/legacy-cleanup.ts (3)
232-253: ReDoS risk is mitigated since patterns come from hardcoded constants.The static analysis tool flags regex construction from variables as a potential ReDoS vector. However, the
patternparameter originates exclusively from the hardcodedLEGACY_SLASH_COMMAND_PATHSconstant (line 194), not user input. The patterns likeopenspec-*.mdare simple globs that translate to non-catastrophic regex patterns.For additional safety, consider adding a comment documenting this constraint, or validating patterns at compile time.
📝 Optional: Add defensive comment
// Convert glob pattern to regex // openspec-*.md -> /^openspec-.*\.md$/ // openspec-*.prompt.md -> /^openspec-.*\.prompt\.md$/ // openspec-*.toml -> /^openspec-.*\.toml$/ + // Note: Pattern comes from LEGACY_SLASH_COMMAND_PATHS constant, not user input const regexPattern = filePart .replace(/[.+^${}()|[\]\\]/g, '\\$&') // Escape regex special chars except * .replace(/\*/g, '.*'); // Replace * with .*
296-320:isOnlyOpenSpecContentusesindexOfinstead of line-boundary-awarefindMarkerIndex.The helper functions
hasOpenSpecMarkers(line 296-300) andisOnlyOpenSpecContent(line 308-320) use plainindexOfto locate markers, while the rest of the codebase (includingremoveMarkerBlockat line 330) usesfindMarkerIndexwhich ensures markers are on their own lines.This inconsistency could cause
isOnlyOpenSpecContentto incorrectly returntruewhen markers appear inline within other content, though this scenario is unlikely in practice.♻️ Optional: Use findMarkerIndex for consistency
If you want strict consistency with other marker handling:
+import { findMarkerIndex } from '../utils/file-system.js'; + export function isOnlyOpenSpecContent(content: string): boolean { - const startIndex = content.indexOf(OPENSPEC_MARKERS.start); - const endIndex = content.indexOf(OPENSPEC_MARKERS.end); + const startIndex = findMarkerIndex(content, OPENSPEC_MARKERS.start); + const endIndex = startIndex !== -1 + ? findMarkerIndex(content, OPENSPEC_MARKERS.end, startIndex + OPENSPEC_MARKERS.start.length) + : findMarkerIndex(content, OPENSPEC_MARKERS.end); if (startIndex === -1 || endIndex === -1 || endIndex <= startIndex) { return false; } // ... }Note: This would require exporting
findMarkerIndexfrom file-system.ts.
599-616: Same ReDoS consideration as line 238 - patterns are from constants.Same analysis applies here: the regex is constructed from
LEGACY_SLASH_COMMAND_PATHSconstant patterns, not user input. The risk is minimal.
| ### Optional: Configure Project Context | ||
|
|
||
| After `openspec init` completes, you'll receive a suggested prompt to help populate your project context: | ||
| After `openspec init`, you can customize `openspec/config.yaml` to inject project-specific context into all artifacts: | ||
|
|
||
| ```text | ||
| Populate your project context: | ||
| "Please read openspec/project.md and help me fill it out with details about my project, tech stack, and conventions" | ||
| ```yaml | ||
| # openspec/config.yaml | ||
| schema: spec-driven | ||
|
|
||
| context: | | ||
| Tech stack: TypeScript, React, Node.js | ||
| Testing: Vitest for unit tests | ||
| Style: ESLint with Prettier | ||
| rules: | ||
| proposal: | ||
| - Include rollback plan | ||
| specs: | ||
| - Use Given/When/Then format for scenarios | ||
| ``` | ||
| Use `openspec/project.md` to define project-level conventions, standards, architectural patterns, and other guidelines that should be followed across all changes. | ||
| This context is automatically included in artifact instructions, helping the AI understand your project's conventions. | ||
There was a problem hiding this comment.
Align scenario formatting rule with OpenSpec conventions.
The snippet says “Given/When/Then,” but OpenSpec specs require WHEN/THEN with #### Scenario: headers. Suggest updating the rule text to avoid drift. Based on learnings, use WHEN/THEN and #### Scenario: format.
💡 Proposed wording tweak
- - Use Given/When/Then format for scenarios
+ - Use WHEN/THEN format with `#### Scenario:` headers🤖 Prompt for AI Agents
In `@README.md` around lines 203 - 224, Update the example in openspec/config.yaml
under the rules.specs entry to reference OpenSpec's required scenario format:
replace "Use Given/When/Then format for scenarios" with guidance to use the
"#### Scenario:" header and the WHEN/THEN structure (e.g., "Use '#### Scenario:'
headers and WHEN/THEN format for scenarios") so the README's sample context
matches OpenSpec conventions.
Reverts the README documentation changes that were included in PR #565. These changes document features that aren't released yet. A separate PR will be opened to re-apply these changes when the package is released.
Reverts the README documentation changes that were included in PR #565. These changes document features that aren't released yet. A separate PR will be opened to re-apply these changes when the package is released.
Summary
experimental/toworkflow/directoryopenspec updateto refresh skills/commands instead of config filesBreaking Changes
openspec initnow uses the skill-based workflow (previously in experimental)experimentalcommand is now a hidden alias toinitTest plan
--toolsflag--forceflag for CI environments🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Refactor
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.