feat(cli): merge init and experimental commands #565
Conversation
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
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
Contributor
📝 WalkthroughWalkthroughThis PR fundamentally restructures OpenSpec's architecture by replacing a registry-based configurator pattern for AI tools and slash commands with a skills-directory approach, consolidating experimental artifact workflows into primary CLI commands, and introducing legacy artifact cleanup capabilities. Major refactoring of init/update flows and comprehensive test updates accompany these changes. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes This PR involves substantial architectural restructuring across multiple subsystems: elimination of the configurator/registry pattern (30+ files removed), introduction of new legacy-cleanup infrastructure, major refactoring of init/update control flows with new public APIs, template system consolidation, and comprehensive test rewrites. The high heterogeneity of changes (removals, additions, refactors across different domains) combined with logic density in core initialization/update modules and the need to verify how all restructured pieces integrate together demands careful, methodical review. Possibly related PRs
Suggested reviewers
🚥 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}
Contributor
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(); });
Comment on lines
262
to
264
| $ openspec list # Confirm the change folder exists | ||
| $ openspec validate add-profile-filters # Validate spec formatting | ||
| $ openspec show add-profile-filters # Review proposal, tasks, and spec delta |
Contributor
There was a problem hiding this comment.
Drop $ prompts in code blocks to satisfy MD014.
markdownlint flags prompts when no output is shown.
📝 Suggested edits
-$ openspec list # Confirm the change folder exists
-$ openspec validate add-profile-filters # Validate spec formatting
-$ openspec show add-profile-filters # Review proposal, tasks, and spec delta
+openspec list # Confirm the change folder exists
+openspec validate add-profile-filters # Validate spec formatting
+openspec show add-profile-filters # Review proposal, tasks, and spec delta-$ openspec archive add-profile-filters --yes
+openspec archive add-profile-filters --yesAlso applies to: 288-288
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
262-262: Dollar signs used before commands without showing output
(MD014, commands-show-output)
263-263: Dollar signs used before commands without showing output
(MD014, commands-show-output)
264-264: Dollar signs used before commands without showing output
(MD014, commands-show-output)
🤖 Prompt for AI Agents
In `@README.md` around lines 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 "$ ".
Comment on lines
+682
to
+686
| if (configStatus === 'created') { | ||
| console.log(`Config: openspec/config.yaml (schema: ${DEFAULT_SCHEMA})`); | ||
| } else if (configStatus === 'exists') { | ||
| console.log(`Config: openspec/config.yaml (exists)`); | ||
| } else { |
Contributor
There was a problem hiding this comment.
Config status message is inaccurate when only config.yml exists.
createConfig() treats either file as “exists,” but the message always names config.yaml.
📝 Suggested edit
- console.log(`Config: openspec/config.yaml (exists)`);
+ console.log('Config: openspec/config.yaml or config.yml (exists)');🤖 Prompt for AI Agents
In `@src/core/init.ts` around lines 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.
Comment on lines
+129
to
+136
| // Determine if any legacy artifacts exist | ||
| result.hasLegacyArtifacts = | ||
| result.configFiles.length > 0 || | ||
| result.slashCommandDirs.length > 0 || | ||
| result.slashCommandFiles.length > 0 || | ||
| result.hasOpenspecAgents || | ||
| result.hasRootAgentsWithMarkers; | ||
|
|
Contributor
There was a problem hiding this comment.
Migration hint can be skipped when only openspec/project.md exists.
hasLegacyArtifacts doesn’t include hasProjectMd, so the init flow won’t surface the migration hint in that scenario.
🔧 Suggested fix
- result.hasLegacyArtifacts =
- result.configFiles.length > 0 ||
- result.slashCommandDirs.length > 0 ||
- result.slashCommandFiles.length > 0 ||
- result.hasOpenspecAgents ||
- result.hasRootAgentsWithMarkers;
+ result.hasLegacyArtifacts =
+ result.configFiles.length > 0 ||
+ result.slashCommandDirs.length > 0 ||
+ result.slashCommandFiles.length > 0 ||
+ result.hasOpenspecAgents ||
+ result.hasRootAgentsWithMarkers ||
+ result.hasProjectMd;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Determine if any legacy artifacts exist | |
| result.hasLegacyArtifacts = | |
| result.configFiles.length > 0 || | |
| result.slashCommandDirs.length > 0 || | |
| result.slashCommandFiles.length > 0 || | |
| result.hasOpenspecAgents || | |
| result.hasRootAgentsWithMarkers; | |
| // Determine if any legacy artifacts exist | |
| result.hasLegacyArtifacts = | |
| result.configFiles.length > 0 || | |
| result.slashCommandDirs.length > 0 || | |
| result.slashCommandFiles.length > 0 || | |
| result.hasOpenspecAgents || | |
| result.hasRootAgentsWithMarkers || | |
| result.hasProjectMd; |
🤖 Prompt for AI Agents
In `@src/core/legacy-cleanup.ts` around lines 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.
Comment on lines
+436
to
+458
| // Handle root AGENTS.md with OpenSpec markers | ||
| if (detection.hasRootAgentsWithMarkers) { | ||
| const rootAgentsPath = FileSystemUtils.joinPath(projectPath, 'AGENTS.md'); | ||
| try { | ||
| const content = await FileSystemUtils.readFile(rootAgentsPath); | ||
|
|
||
| if (isOnlyOpenSpecContent(content)) { | ||
| await fs.unlink(rootAgentsPath); | ||
| result.deletedFiles.push('AGENTS.md'); | ||
| } else { | ||
| const newContent = removeMarkerBlock(content); | ||
| if (newContent === '') { | ||
| await fs.unlink(rootAgentsPath); | ||
| result.deletedFiles.push('AGENTS.md'); | ||
| } else { | ||
| await FileSystemUtils.writeFile(rootAgentsPath, newContent); | ||
| result.modifiedFiles.push('AGENTS.md'); | ||
| } | ||
| } | ||
| } catch (error: any) { | ||
| result.errors.push(`Failed to handle AGENTS.md: ${error.message}`); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Avoid double-processing root AGENTS.md.
AGENTS.md is already handled via legacy config cleanup, then processed again here, which can surface false “failed to handle” errors after deletion.
🔧 Suggested fix
- if (detection.hasRootAgentsWithMarkers) {
+ if (detection.hasRootAgentsWithMarkers && !detection.configFiles.includes('AGENTS.md')) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Handle root AGENTS.md with OpenSpec markers | |
| if (detection.hasRootAgentsWithMarkers) { | |
| const rootAgentsPath = FileSystemUtils.joinPath(projectPath, 'AGENTS.md'); | |
| try { | |
| const content = await FileSystemUtils.readFile(rootAgentsPath); | |
| if (isOnlyOpenSpecContent(content)) { | |
| await fs.unlink(rootAgentsPath); | |
| result.deletedFiles.push('AGENTS.md'); | |
| } else { | |
| const newContent = removeMarkerBlock(content); | |
| if (newContent === '') { | |
| await fs.unlink(rootAgentsPath); | |
| result.deletedFiles.push('AGENTS.md'); | |
| } else { | |
| await FileSystemUtils.writeFile(rootAgentsPath, newContent); | |
| result.modifiedFiles.push('AGENTS.md'); | |
| } | |
| } | |
| } catch (error: any) { | |
| result.errors.push(`Failed to handle AGENTS.md: ${error.message}`); | |
| } | |
| } | |
| // Handle root AGENTS.md with OpenSpec markers | |
| if (detection.hasRootAgentsWithMarkers && !detection.configFiles.includes('AGENTS.md')) { | |
| const rootAgentsPath = FileSystemUtils.joinPath(projectPath, 'AGENTS.md'); | |
| try { | |
| const content = await FileSystemUtils.readFile(rootAgentsPath); | |
| if (isOnlyOpenSpecContent(content)) { | |
| await fs.unlink(rootAgentsPath); | |
| result.deletedFiles.push('AGENTS.md'); | |
| } else { | |
| const newContent = removeMarkerBlock(content); | |
| if (newContent === '') { | |
| await fs.unlink(rootAgentsPath); | |
| result.deletedFiles.push('AGENTS.md'); | |
| } else { | |
| await FileSystemUtils.writeFile(rootAgentsPath, newContent); | |
| result.modifiedFiles.push('AGENTS.md'); | |
| } | |
| } | |
| } catch (error: any) { | |
| result.errors.push(`Failed to handle AGENTS.md: ${error.message}`); | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@src/core/legacy-cleanup.ts` around lines 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.
Comment on lines
+264
to
+303
| export function removeMarkerBlock( | ||
| content: string, | ||
| startMarker: string, | ||
| endMarker: string | ||
| ): string { | ||
| const startIndex = findMarkerIndex(content, startMarker); | ||
| const endIndex = startIndex !== -1 | ||
| ? findMarkerIndex(content, endMarker, startIndex + startMarker.length) | ||
| : findMarkerIndex(content, endMarker); | ||
|
|
||
| if (startIndex === -1 || endIndex === -1 || endIndex <= startIndex) { | ||
| return content; | ||
| } | ||
|
|
||
| // Find the start of the line containing the start marker | ||
| let lineStart = startIndex; | ||
| while (lineStart > 0 && content[lineStart - 1] !== '\n') { | ||
| lineStart--; | ||
| } | ||
|
|
||
| // Find the end of the line containing the end marker | ||
| let lineEnd = endIndex + endMarker.length; | ||
| while (lineEnd < content.length && content[lineEnd] !== '\n') { | ||
| lineEnd++; | ||
| } | ||
| // Include the trailing newline if present | ||
| if (lineEnd < content.length && content[lineEnd] === '\n') { | ||
| lineEnd++; | ||
| } | ||
|
|
||
| const before = content.substring(0, lineStart); | ||
| const after = content.substring(lineEnd); | ||
|
|
||
| // Clean up double blank lines | ||
| let result = before + after; | ||
| result = result.replace(/\n{3,}/g, '\n\n'); | ||
|
|
||
| // Trim leading/trailing whitespace but preserve content | ||
| return result.trim() === '' ? '' : result.trim() + '\n'; | ||
| } |
Contributor
There was a problem hiding this comment.
Preserve leading whitespace and line endings when removing marker blocks.
trim() strips leading indentation and forces an LF newline, which can alter files with intentional leading whitespace or CRLF endings. Consider using trimEnd() and preserving the original newline style to avoid unintended diffs.
🛠️ Proposed adjustment
- // Clean up double blank lines
- let result = before + after;
- result = result.replace(/\n{3,}/g, '\n\n');
-
- // Trim leading/trailing whitespace but preserve content
- return result.trim() === '' ? '' : result.trim() + '\n';
+ const newline = content.includes('\r\n') ? '\r\n' : '\n';
+
+ // Clean up double blank lines
+ let result = before + after;
+ result = result.replace(/(?:\r?\n){3,}/g, `${newline}${newline}`);
+
+ if (result.trim().length === 0) {
+ return '';
+ }
+
+ // Preserve leading whitespace, only trim trailing
+ return result.trimEnd() + newline;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export function removeMarkerBlock( | |
| content: string, | |
| startMarker: string, | |
| endMarker: string | |
| ): string { | |
| const startIndex = findMarkerIndex(content, startMarker); | |
| const endIndex = startIndex !== -1 | |
| ? findMarkerIndex(content, endMarker, startIndex + startMarker.length) | |
| : findMarkerIndex(content, endMarker); | |
| if (startIndex === -1 || endIndex === -1 || endIndex <= startIndex) { | |
| return content; | |
| } | |
| // Find the start of the line containing the start marker | |
| let lineStart = startIndex; | |
| while (lineStart > 0 && content[lineStart - 1] !== '\n') { | |
| lineStart--; | |
| } | |
| // Find the end of the line containing the end marker | |
| let lineEnd = endIndex + endMarker.length; | |
| while (lineEnd < content.length && content[lineEnd] !== '\n') { | |
| lineEnd++; | |
| } | |
| // Include the trailing newline if present | |
| if (lineEnd < content.length && content[lineEnd] === '\n') { | |
| lineEnd++; | |
| } | |
| const before = content.substring(0, lineStart); | |
| const after = content.substring(lineEnd); | |
| // Clean up double blank lines | |
| let result = before + after; | |
| result = result.replace(/\n{3,}/g, '\n\n'); | |
| // Trim leading/trailing whitespace but preserve content | |
| return result.trim() === '' ? '' : result.trim() + '\n'; | |
| } | |
| export function removeMarkerBlock( | |
| content: string, | |
| startMarker: string, | |
| endMarker: string | |
| ): string { | |
| const startIndex = findMarkerIndex(content, startMarker); | |
| const endIndex = startIndex !== -1 | |
| ? findMarkerIndex(content, endMarker, startIndex + startMarker.length) | |
| : findMarkerIndex(content, endMarker); | |
| if (startIndex === -1 || endIndex === -1 || endIndex <= startIndex) { | |
| return content; | |
| } | |
| // Find the start of the line containing the start marker | |
| let lineStart = startIndex; | |
| while (lineStart > 0 && content[lineStart - 1] !== '\n') { | |
| lineStart--; | |
| } | |
| // Find the end of the line containing the end marker | |
| let lineEnd = endIndex + endMarker.length; | |
| while (lineEnd < content.length && content[lineEnd] !== '\n') { | |
| lineEnd++; | |
| } | |
| // Include the trailing newline if present | |
| if (lineEnd < content.length && content[lineEnd] === '\n') { | |
| lineEnd++; | |
| } | |
| const before = content.substring(0, lineStart); | |
| const after = content.substring(lineEnd); | |
| const newline = content.includes('\r\n') ? '\r\n' : '\n'; | |
| // Clean up double blank lines | |
| let result = before + after; | |
| result = result.replace(/(?:\r?\n){3,}/g, `${newline}${newline}`); | |
| if (result.trim().length === 0) { | |
| return ''; | |
| } | |
| // Preserve leading whitespace, only trim trailing | |
| return result.trimEnd() + newline; | |
| } |
🤖 Prompt for AI Agents
In `@src/utils/file-system.ts` around lines 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.
Comment on lines
+845
to
+856
| it('should cover all tools from the SlashCommandRegistry', () => { | ||
| const expectedTools = [ | ||
| 'claude', 'codebuddy', 'qoder', 'cursor', 'windsurf', 'kilocode', | ||
| 'opencode', 'codex', 'github-copilot', 'amazon-q', 'factory', | ||
| 'gemini', 'auggie', 'cline', 'crush', 'costrict', 'qwen', | ||
| 'roocode', 'antigravity', 'iflow', 'continue', | ||
| ]; | ||
|
|
||
| for (const tool of expectedTools) { | ||
| expect(LEGACY_SLASH_COMMAND_PATHS).toHaveProperty(tool); | ||
| } | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that LEGACY_SLASH_COMMAND_PATHS covers all tools consistently
# Find the actual exported LEGACY_SLASH_COMMAND_PATHS keys in legacy-cleanup.ts
echo "=== Keys in LEGACY_SLASH_COMMAND_PATHS ==="
rg -A 100 'export const LEGACY_SLASH_COMMAND_PATHS' --type ts | head -100
# Check if there's a SlashCommandRegistry or similar that defines the canonical tool list
echo ""
echo "=== Looking for SlashCommandRegistry or tool definitions ==="
rg 'SlashCommandRegistry|slash.*registry' --type ts -i | head -20Repository: Fission-AI/OpenSpec
Length of output: 7612
🏁 Script executed:
#!/bin/bash
# Find SlashCommandRegistry definition
echo "=== Looking for SlashCommandRegistry in registry.ts ==="
cat -n src/core/command-generation/registry.ts | head -200
echo ""
echo "=== Check if there's a SlashCommandRegistry type or constant ==="
rg 'type SlashCommandRegistry|const.*SlashCommandRegistry|interface.*SlashCommand' --type ts -A 5Repository: Fission-AI/OpenSpec
Length of output: 4892
Derive expected tools from CommandAdapterRegistry instead of hardcoding.
The test hardcodes an expected tools list (21 tools) and only checks that these exist in LEGACY_SLASH_COMMAND_PATHS using toHaveProperty(). This approach is brittle because:
- If new adapters are registered in
CommandAdapterRegistry, the test won't catch missing entries inLEGACY_SLASH_COMMAND_PATHS - If tools are added to
LEGACY_SLASH_COMMAND_PATHS, the test passes silently (no verification that the lists match) CommandAdapterRegistry(registered insrc/core/command-generation/registry.ts) is the canonical source of supported tools, not a hardcoded list
Instead, dynamically extract tool IDs from CommandAdapterRegistry.getAll() to ensure the test automatically fails when the two sources diverge.
🤖 Prompt for AI Agents
In `@test/core/legacy-cleanup.test.ts` around lines 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.
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.
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
status,instructions,templates,schemas, andnew changefor managing OpenSpec workflows.--forceoption for automated legacy cleanup.Documentation
✏️ Tip: You can customize this high-level summary in your review settings.