refactor: unify SpecKit/OpenSpec command management (Phase 06)#820
Conversation
Extract shared prompt management logic (metadata load, bundled/user prompt resolution, customization save/reset, command lookup) into a reusable factory at src/main/spec-command-manager.ts. Both managers now delegate to this shared base while keeping their source-specific refresh strategies (SpecKit: release ZIP + unzip, OpenSpec: AGENTS.md section parsing). IPC handler names (speckit:*, openspec:*) and all public exports are unchanged, so preload, renderer services, and tests continue to work without modification. Files touched: - src/main/spec-command-manager.ts (new, shared factory) - src/main/speckit-manager.ts (thin wrapper over shared base + ZIP refresh) - src/main/openspec-manager.ts (thin wrapper over shared base + AGENTS.md refresh)
📝 WalkthroughWalkthroughRefactored per-feature prompt/command storage into a shared SpecCommandManager ( Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller (open/speckit APIs)
participant Manager as SpecCommandManager
participant FS as FileSystem (userData)
participant Bundle as Bundled Resources
Caller->>Manager: getPrompts() / getMetadata() / getCommand(id)
Manager->>FS: loadUserCustomizations(), check user prompt files (*.md)
alt user override exists
FS-->>Manager: user prompt content + stored metadata
else
Manager->>Bundle: load bundled metadata & prompts (or dev src/prompts)
Bundle-->>Manager: bundled content
end
Manager-->>Caller: resolved prompts/metadata (merged, with isModified flags)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 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)
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 extracts the common prompt CRUD, customization tracking, and metadata caching logic shared by Confidence Score: 5/5
Important Files Changed
Class Diagram%%{init: {'theme': 'neutral'}}%%
classDiagram
class SpecCommandManagerConfig {
+logContext: string
+filePrefix: string
+bundledDirName: string
+customizationsFileName: string
+userPromptsDirName: string
+commands: readonly SpecCommandDefinition[]
+defaultMetadata: SpecMetadata
}
class SpecCommandManager {
+getMetadata() Promise~SpecMetadata~
+getPrompts() Promise~SpecCommand[]~
+savePrompt(id, content) Promise~void~
+resetPrompt(id) Promise~string~
+getCommand(id) Promise~SpecCommand~
+getCommandBySlash(slash) Promise~SpecCommand~
+getUserPromptsPath() string
+loadUserCustomizations() Promise~StoredData~
+saveUserCustomizations(data) Promise~void~
+getBundledMetadata() Promise~SpecMetadata~
}
class createSpecCommandManager {
<<factory>>
+createSpecCommandManager(config) SpecCommandManager
}
class OpenSpecManager {
+getOpenSpecMetadata()
+getOpenSpecPrompts()
+saveOpenSpecPrompt()
+resetOpenSpecPrompt()
+refreshOpenSpecPrompts()
}
class SpecKitManager {
+getSpeckitMetadata()
+getSpeckitPrompts()
+saveSpeckitPrompt()
+resetSpeckitPrompt()
+refreshSpeckitPrompts()
}
createSpecCommandManager --> SpecCommandManager : returns
SpecCommandManagerConfig --> createSpecCommandManager : config input
OpenSpecManager --> SpecCommandManager : delegates to manager
SpecKitManager --> SpecCommandManager : delegates to manager
Reviews (1): Last reviewed commit: "refactor: unify SpecKit/OpenSpec command..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| export interface SpecCommandDefinition { | ||
| id: string; | ||
| command: string; | ||
| description: string; | ||
| isCustom: boolean; | ||
| } |
There was a problem hiding this comment.
command field is declared but never consumed
SpecCommandDefinition.command is required by the interface and populated in every command list, but getBundledPrompts() never reads it — id and description are the only fields used. The outgoing SpecCommand.command is always reconstructed as `/${filePrefix}.${id}` (line 248). Keeping the field in the definition creates a silent contract gap: if a definition ever has a command that doesn't match /${filePrefix}.${id}, getCommandBySlash() would find the command via the reconstructed string, making the declared command value misleading.
Consider removing the field from SpecCommandDefinition (it's always derivable) or actually reading it in getPrompts() so the definition is the source of truth.
| export interface SpecCommandDefinition { | |
| id: string; | |
| command: string; | |
| description: string; | |
| isCustom: boolean; | |
| } | |
| export interface SpecCommandDefinition { | |
| id: string; | |
| description: string; | |
| isCustom: boolean; | |
| } |
| /** Helpers used by refresh implementations. */ | ||
| getUserPromptsPath(): string; | ||
| loadUserCustomizations(): Promise<StoredData | null>; | ||
| saveUserCustomizations(data: StoredData): Promise<void>; | ||
| getBundledMetadata(): Promise<SpecMetadata>; |
There was a problem hiding this comment.
StoredData type leaked through public interface
loadUserCustomizations and saveUserCustomizations expose StoredData in the SpecCommandManager interface, but StoredData (and StoredPrompt) are not exported. Callers in openspec-manager.ts and speckit-manager.ts must construct objects of this shape without a named type, relying on structural inference. This compiles today (TypeScript is structural), but makes the interface harder to document and use safely — any future field added to StoredData must be hunted down in all callers manually.
Exporting StoredData (and StoredPrompt) would make the contract explicit:
export interface StoredPrompt {
content: string;
isModified: boolean;
modifiedAt?: string;
}
export interface StoredData {
metadata: SpecMetadata;
prompts: Record<string, StoredPrompt>;
}![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/spec-command-manager.ts`:
- Around line 113-119: The current loadUserCustomizations() function swallows
all errors; change its catch to inspect the error code and only return null for
ENOENT, otherwise re-throw the error so malformed JSON and permission errors
surface; update the catch in loadUserCustomizations() (and the analogous
user-prompt and metadata fallback functions referenced later) to check err.code
=== 'ENOENT' before returning null and re-throw for any other error.
- Around line 257-270: savePrompt currently accepts any id and persists it,
creating dead/invalid customization state because getPrompts() only knows
configured commands and resetPrompt() throws for unknown ids; update savePrompt
to validate the id before saving by loading the available prompt definitions
(via getPrompts or the bundled metadata from getBundledMetadata()) and
rejecting/throwing if the given id is not present, only proceeding to call
loadUserCustomizations(), modify customizations.prompts[id], and
saveUserCustomizations(customizations) when the id is recognized; keep existing
logging (logger.info) but move it after the validation and successful save and
surface a clear error when id validation fails.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 964332d9-8e1c-4ca4-9b99-e94a75d51cd6
📒 Files selected for processing (3)
src/main/openspec-manager.tssrc/main/spec-command-manager.tssrc/main/speckit-manager.ts
| async function savePrompt(id: string, content: string): Promise<void> { | ||
| const customizations = (await loadUserCustomizations()) ?? { | ||
| metadata: await getBundledMetadata(), | ||
| prompts: {}, | ||
| }; | ||
|
|
||
| customizations.prompts[id] = { | ||
| content, | ||
| isModified: true, | ||
| modifiedAt: new Date().toISOString(), | ||
| }; | ||
|
|
||
| await saveUserCustomizations(customizations); | ||
| logger.info(`Saved customization for ${filePrefix}.${id}`, logContext); |
There was a problem hiding this comment.
Reject unknown command ids before persisting.
savePrompt() will currently report success for any id, but getPrompts() only returns configured commands and resetPrompt() throws for the same invalid id. That leaves dead JSON state behind and makes bad saves look successful.
🧭 Validate the id up front
async function savePrompt(id: string, content: string): Promise<void> {
+ if (!commands.some((cmd) => cmd.id === id)) {
+ throw new Error(`Unknown ${filePrefix} command: ${id}`);
+ }
+
const customizations = (await loadUserCustomizations()) ?? {
metadata: await getBundledMetadata(),
prompts: {},
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/spec-command-manager.ts` around lines 257 - 270, savePrompt
currently accepts any id and persists it, creating dead/invalid customization
state because getPrompts() only knows configured commands and resetPrompt()
throws for unknown ids; update savePrompt to validate the id before saving by
loading the available prompt definitions (via getPrompts or the bundled metadata
from getBundledMetadata()) and rejecting/throwing if the given id is not
present, only proceeding to call loadUserCustomizations(), modify
customizations.prompts[id], and saveUserCustomizations(customizations) when the
id is recognized; keep existing logging (logger.info) but move it after the
validation and successful save and surface a clear error when id validation
fails.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/main/spec-command-manager.ts (2)
260-273:⚠️ Potential issue | 🟡 MinorReject unknown prompt ids before saving.
savePrompt()still persists arbitrary ids, which leaves dead customization state behind:getPrompts()never surfaces those entries, whileresetPrompt()throws for the same invalid id. Validate againstcommandsup front.🧭 Validate the id before mutating customizations
async function savePrompt(id: string, content: string): Promise<void> { + if (!commands.some((cmd) => cmd.id === id)) { + throw new Error(`Unknown ${filePrefix} command: ${id}`); + } + const customizations = (await loadUserCustomizations()) ?? { metadata: await getBundledMetadata(), prompts: {}, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/spec-command-manager.ts` around lines 260 - 273, savePrompt currently writes any arbitrary id into user customizations which creates dead entries (getPrompts won't list them and resetPrompt will later throw); before mutating customizations in savePrompt(id, content) validate that id exists in the known command set (e.g., the commands registry used by getPrompts/resetPrompt) and reject/throw (or return an error) for unknown ids. Locate savePrompt and add an upfront check against the existing commands collection (the same source used by getPrompts/resetPrompt) so only known prompt ids are persisted.
144-197:⚠️ Potential issue | 🟠 MajorOnly fall back to the placeholder for missing bundled prompt files.
Both bundled
readFile()branches currently turn any fs failure into"Prompt not available", which masks permission/I/O/package errors as if the prompt were simply absent. Keep the placeholder path forENOENT, but re-throw everything else.🔧 Narrow the bundled fallback to missing files only
if (cmd.isCustom) { try { const promptPath = path.join(bundledPromptsDir, `${filePrefix}.${cmd.id}.md`); const prompt = await fs.readFile(promptPath, 'utf-8'); result[cmd.id] = { prompt, description: cmd.description, isCustom: cmd.isCustom, }; - } catch (error) { + } catch (error: unknown) { + if ((error as NodeJS.ErrnoException).code !== 'ENOENT') throw error; logger.warn(`Failed to load bundled prompt for ${cmd.id}: ${error}`, logContext); result[cmd.id] = { prompt: `# ${cmd.id}\n\nPrompt not available.`, description: cmd.description, isCustom: cmd.isCustom, }; } continue; } @@ try { const promptPath = path.join(bundledPromptsDir, `${filePrefix}.${cmd.id}.md`); const prompt = await fs.readFile(promptPath, 'utf-8'); result[cmd.id] = { prompt, description: cmd.description, isCustom: cmd.isCustom, }; - } catch (error) { + } catch (error: unknown) { + if ((error as NodeJS.ErrnoException).code !== 'ENOENT') throw error; logger.warn(`Failed to load bundled prompt for ${cmd.id}: ${error}`, logContext); result[cmd.id] = { prompt: `# ${cmd.id}\n\nPrompt not available.`, description: cmd.description, isCustom: cmd.isCustom, }; }As per coding guidelines, "Do not silently swallow errors. Let unhandled exceptions bubble up to Sentry for error tracking in production."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/spec-command-manager.ts` around lines 144 - 197, The bundled-prompt catch blocks in the commands loop (the branches handling cmd.isCustom and the final "Fall back to bundled prompts" section) currently convert any fs error into the placeholder prompt; change both catch handlers to only swallow ENOENT by checking (error as NodeJS.ErrnoException).code === 'ENOENT' and then set the placeholder, but re-throw the error for any other errno so permission/I/O/package errors are not masked; keep the same logger.warn for ENOENT cases and ensure the checks reference cmd.id, bundledPromptsDir and the readFile calls to locate the exact blocks to modify.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/main/spec-command-manager.ts`:
- Around line 260-273: savePrompt currently writes any arbitrary id into user
customizations which creates dead entries (getPrompts won't list them and
resetPrompt will later throw); before mutating customizations in savePrompt(id,
content) validate that id exists in the known command set (e.g., the commands
registry used by getPrompts/resetPrompt) and reject/throw (or return an error)
for unknown ids. Locate savePrompt and add an upfront check against the existing
commands collection (the same source used by getPrompts/resetPrompt) so only
known prompt ids are persisted.
- Around line 144-197: The bundled-prompt catch blocks in the commands loop (the
branches handling cmd.isCustom and the final "Fall back to bundled prompts"
section) currently convert any fs error into the placeholder prompt; change both
catch handlers to only swallow ENOENT by checking (error as
NodeJS.ErrnoException).code === 'ENOENT' and then set the placeholder, but
re-throw the error for any other errno so permission/I/O/package errors are not
masked; keep the same logger.warn for ENOENT cases and ensure the checks
reference cmd.id, bundledPromptsDir and the readFile calls to locate the exact
blocks to modify.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b773fe85-2b0d-47e7-94db-5ce61ef7c27f
📒 Files selected for processing (3)
src/main/openspec-manager.tssrc/main/spec-command-manager.tssrc/main/speckit-manager.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/speckit-manager.ts
jSydorowicz21
added
refactor
2 participants
Summary
Extracts shared command management logic from
speckit-manager.tsandopenspec-manager.tsinto a newsrc/main/spec-command-manager.tsfactory. Both managers now delegate common operations (prompt CRUD, customization tracking, metadata caching) to the shared base while keeping their phase-specific refresh strategies.Net: -108 lines across 3 files (1003 -> 895)
Shared base
New:
src/main/spec-command-manager.ts(313 lines)createSpecCommandManager(config)returning{ getMetadata, getPrompts, getCommand, getCommandBySlash, savePrompt, resetPrompt }SpecCommand,SpecMetadata,SpecCommandDefinitionRefactored managers
src/main/speckit-manager.ts: 531 -> 319 lines. Keeps SpecKit-specific GitHub release ZIP + unzip refresh strategy for pulling commands from the spec-kit repo.src/main/openspec-manager.ts: 472 -> 261 lines. Keeps OpenSpec-specific AGENTS.md section-parsing refresh strategy for pulling commands from the openspec repo.All public exports (
SpecKitCommand,OpenSpecCommand,getSpeckitMetadata,getOpenSpecMetadata, etc.) are preserved so existing consumers keep working with zero changes.IPC handlers preserved unchanged
Verified all 12 handlers still registered with identical channel names:
speckit:getMetadata,speckit:getPrompts,speckit:getCommand,speckit:savePrompt,speckit:resetPrompt,speckit:refreshopenspec:getMetadata,openspec:getPrompts,openspec:getCommand,openspec:savePrompt,openspec:resetPrompt,openspec:refreshIPC handler files (
src/main/ipc/handlers/speckit.ts,openspec.ts) were intentionally NOT unified - they already delegate to their managers, and unifying them would require a generic handler factory that adds boilerplate without meaningful savings, at the cost of making the external IPC channel registration harder to read.Test plan
npm run lintpasses clean (all 3 tsconfigs).speckit/andmodifiedflag updates.openspec/andmodifiedflag updatesRisk
Medium. Both managers' refresh strategies (GitHub ZIP vs. AGENTS.md parsing) are preserved as-is; only CRUD + metadata caching was lifted. Existing integration tests (30 pass) cover the delegation paths.
Summary by CodeRabbit