refactor(commands): modularize artifact workflow into separate files #562
Conversation
Split the monolithic artifact-workflow.ts into separate modules under src/commands/experimental/: - index.ts: main exports and command registration - status.ts: status display logic - new-change.ts: change creation logic - schemas.ts: Zod schemas - setup.ts: setup command logic - templates.ts: template generation - shared.ts: shared utilities - instructions.ts: instruction generation Also extracted init wizard logic to src/core/init/wizard.ts.
📝 WalkthroughWalkthroughThis PR removes the monolithic Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as Client (CLI)
participant ExpIdx as Experimental Index
participant Cmd as Command Handler
participant FS as Filesystem
participant Adapter as Tool Adapter Registry
CLI->>ExpIdx: register commands / parse input
CLI->>ExpIdx: invoke "artifact-experimental-setup" or other command
ExpIdx->>Cmd: dispatch to specific handler
Cmd->>FS: validate project, changes, schemas, read/write files
Cmd->>Adapter: (if setup) enumerate adapters, create slash commands
Adapter-->>Cmd: report created/failed commands
Cmd-->>CLI: print results / JSON or formatted text
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
Review CompleteYour review story is ready! Comment !reviewfast on this PR to re-generate the story. |
Greptile SummaryRefactored monolithic Key changes:
Code quality improvements:
This is a pure refactoring with no functional changes - all commands maintain their original behavior. Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant CLI as CLI (index.ts)
participant Reg as Command Registry<br/>(experimental/index.ts)
participant Status as Status Command
participant Instructions as Instructions Command
participant Setup as Setup Command
participant NewChange as New Change Command
participant Shared as Shared Utilities
participant Core as Core Modules<br/>(artifact-graph, init/wizard)
Note over CLI,Core: User runs: openspec status --change my-change
CLI->>Reg: registerArtifactWorkflowCommands(program)
Reg->>CLI: Register all experimental commands
CLI->>Status: statusCommand(options)
Status->>Shared: validateChangeExists()
Status->>Core: loadChangeContext()
Status->>Core: formatChangeStatus()
Status-->>CLI: Display formatted status
Note over CLI,Core: User runs: openspec instructions proposal --change my-change
CLI->>Instructions: instructionsCommand(artifactId, options)
Instructions->>Shared: validateChangeExists()
Instructions->>Core: loadChangeContext()
Instructions->>Core: generateInstructions()
Instructions-->>CLI: Display artifact instructions
Note over CLI,Core: User runs: openspec artifact-experimental-setup --tool claude
CLI->>Setup: artifactExperimentalSetupCommand(options)
Setup->>Core: getSkillTemplates()
Setup->>Core: generateCommands()
Setup-->>CLI: Generate skills and slash commands
Note over CLI,Core: User runs: openspec new change feature-x
CLI->>NewChange: newChangeCommand(name, options)
NewChange->>Shared: validateChangeName()
NewChange->>Core: createChange()
NewChange-->>CLI: Create change directory
Note over CLI,Core: Original monolithic artifact-workflow.ts (1301 lines)<br/>split into 8 focused modules
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/commands/experimental/templates.ts`:
- Around line 53-60: The current detection for the template source using
startsWith can misclassify paths with shared prefixes; update the logic around
schemaDir/projectSchemasDir/userSchemasDir that sets the local variable source
so it uses path.relative to each base (projectSchemasDir and userSchemasDir) and
treats schemaDir as inside a base only when the relative path does not start
with '..' and is not absolute; replace the three-branch startsWith checks with
these relative checks so 'project', 'user', or fallback 'package' are determined
robustly (refer to variables schemaDir, projectSchemasDir, userSchemasDir and
the source assignment in templates.ts).
In `@src/core/init/wizard.ts`:
- Around line 1-11: The static import of `@inquirer/core` should be replaced with
a dynamic import performed at runtime inside the wizard initializer: remove the
top-level import and inside the exported function toolSelectionWizard (or a lazy
initializer for toolSelectionWizardPrompt) do const { createPrompt, useKeypress,
usePagination, useState, isEnterKey, isSpaceKey, isUpKey, isDownKey,
isBackspaceKey } = await import('@inquirer/core'); then move or recreate the
createPrompt(...) call that defines toolSelectionWizardPrompt (which currently
uses ToolWizardConfig) inside that function (or implement a singleton cache so
createPrompt is only run once), and finally invoke and return
toolSelectionWizardPrompt(config); ensure all references to those symbols are
updated to use the locally imported bindings.
🧹 Nitpick comments (7)
src/core/init/wizard.ts (1)
118-119: Consider capping page size for terminal height constraints.Setting
pageSizeto the total number of choices disables pagination scrolling. With 20+ AI tools inAI_TOOLS, this could overflow smaller terminal windows.♻️ Optional: cap to reasonable terminal height
- const pageSize = Math.max(config.choices.length, 1); + const pageSize = Math.min(config.choices.length, 15);Or use
process.stdout.rowsfor dynamic sizing if available.src/commands/experimental/shared.ts (2)
53-55: Consider following the NO_COLOR standard more closely.The NO_COLOR standard specifies that when
NO_COLORexists, "regardless of its value", color should be disabled. The current check only handles'1'or'true'.Suggested fix
export function isColorDisabled(): boolean { - return process.env.NO_COLOR === '1' || process.env.NO_COLOR === 'true'; + return 'NO_COLOR' in process.env; }
100-109: Empty catch block hides potential errors.If
readdirfails for reasons other than the directory not existing (e.g., permission denied), returning an empty array silently masks the issue. Consider logging or differentiating error types.Suggested improvement
} catch { + // Directory doesn't exist or is inaccessible - return empty list return []; }src/commands/experimental/setup.ts (2)
75-80: Redundant validation check aftersearchableMultiSelect.The
validatecallback on line 75 already ensures at least one tool is selected. The check on lines 78-80 is defensive but unreachable in normal operation since the prompt won't resolve until validation passes.Consider removing the redundant check or keeping it only as a safeguard against future changes to the prompt behavior:
- if (selectedTools.length === 0) { - throw new Error('At least one tool must be selected'); - } + // Note: validate callback ensures this, but kept as safeguard + if (selectedTools.length === 0) { + throw new Error('At least one tool must be selected'); + }
166-213: Consider extracting per-tool setup logic.The loop body (lines 170-212) is substantial. Extracting it into a helper function like
setupToolSkillsAndCommands(tool, projectRoot, skillTemplates, commandContents)would improve testability and readability.src/commands/experimental/instructions.ts (2)
290-292: Empty catch block masks errors.Similar to shared.ts, this silently returns
falsefor any error, including permission issues. Consider at least logging in debug mode or checking for specific error codes.
244-300: Replace custom glob implementation withfast-glob, which is already a project dependency.The manual glob matching has real issues: the extension extraction regex (line 270) fails on multi-dot patterns like
*.test.ts(it only captures.tsif the pattern is*.ts). Sincefast-globis already available as a dependency (and already imported insrc/core/artifact-graph/state.ts), use it directly instead of maintaining a custom implementation that handles only basic*and**patterns.
Resolve modify/delete conflict in src/commands/artifact-workflow.ts: - Applied refresh detection functionality to modular setup.ts - Added getToolExperimentalStatus and getExperimentalToolStates functions - Added EXPERIMENTAL_SKILL_NAMES constant - Updated interactive prompt to show configured status and pre-select - Added wasConfigured tracking to distinguish Created vs Refreshed output - Simplified success output format - Renamed command from artifact-experimental-setup to experimental
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/commands/experimental/setup.ts`:
- Around line 363-369: The log message always prints "openspec/config.yaml
(exists)" even when openspec/config.yml is the file found; update the check
around configPath, configYmlPath and configExists to determine which specific
file exists (use configPath vs configYmlPath) and print the actual existing path
(e.g., "openspec/config.yml (exists)" or "openspec/config.yaml (exists)") rather
than the hardcoded string; modify the block that currently references
configPath/configYmlPath to set a variable like foundConfigPath based on
fs.existsSync and log that variable.
- Around line 127-185: The current guard requires options.tool even when
options.selectedTools is provided programmatically; modify the check so it only
prompts/throws when neither options.tool nor a non-empty options.selectedTools
exists (use isInteractive(options) && validTools.length > 0 only when
interactive). When using searchableMultiSelect, set options.selectedTools and
only set options.tool = options.selectedTools[0] if options.tool is not already
provided; leave toolsToSetup as const toolsToSetup = options.selectedTools ||
[options.tool!] so non-interactive callers that pass selectedTools are honored.
Ensure references: options.tool, options.selectedTools, getToolsWithSkillsDir(),
isInteractive(), searchableMultiSelect, toolsToSetup.
- Fix template source detection using path.relative instead of startsWith to prevent misclassification of paths with shared prefixes - Fix config file log message to show actual file name (config.yaml vs config.yml) - Fix selectedTools option to be honored when provided programmatically
Summary
artifact-workflow.ts(1301 lines) into 8 focused modules undersrc/commands/experimental/src/core/init/wizard.tsTest plan
openspec initstill works correctly🤖 Generated with Claude Code
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.