feat: simplify skill installation with profiles and smart defaults#726
feat: simplify skill installation with profiles and smart defaults#726
Conversation
… defaults Introduces a profile system (core/custom) to reduce the default workflow count from 10 to 4, auto-detects AI tools during init, adds a new `propose` workflow combining new+ff, fixes multi-select keybindings, and adds backwards-compatible migration for existing users.
|
Task completed. Here is my review of PR #726: PR Review: feat: simplify skill installation with profiles and smart defaultsOverall AssessmentThis is a well-structured feature PR that introduces a profile system, smart tool detection, and a new Issues1. Duplicated
|
📝 WalkthroughWalkthroughIntroduces a profile and delivery system (core vs custom; skills/commands/both), filesystem-based tool detection, a one-time migration for existing projects, profile-aware init/update flows, propose workflow templates, template filtering by workflow, and updated multi-select keybindings. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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 SummaryIntroduces a profile system to simplify skill installation, reducing default workflows from 10 to 4 ( Key changes:
Confidence Score: 5/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[User runs openspec init/update] --> B{Migration needed?}
B -->|No profile field| C[Check for installed workflows]
C -->|Workflows found| D[Migrate to custom profile]
C -->|No workflows| E[Use core profile default]
B -->|Profile exists| E
E --> F{Profile Type}
F -->|core| G[Load CORE_WORKFLOWS<br/>propose, explore, apply, archive]
F -->|custom| H[Load custom workflows from config]
G --> I{Delivery Mode}
H --> I
I -->|both| J[Generate skills + commands]
I -->|skills| K[Generate skills only]
I -->|commands| L[Generate commands only]
J --> M[Filter by profile workflows]
K --> M
L --> M
M --> N[Write filtered artifacts to tool directories]
N --> O{Update operation?}
O -->|Yes| P[Detect new tools]
O -->|Yes| Q[Show extra workflows note]
O -->|No| R[Show getting started]
P --> S[Complete]
Q --> S
R --> S
Last reviewed commit: 72115cd |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/prompts/searchable-multi-select.ts (1)
71-96:⚠️ Potential issue | 🟡 MinorStale validation error persists after toggling selection.
When validation fails on Enter (line 76), the error message is displayed. If the user then presses Space to change their selection, the error message remains visible because nothing clears the
errorstate on toggle. Consider clearing the error when the selection changes.Proposed fix
// Space to toggle selection if (key.name === 'space') { const choice = filteredChoices[cursor]; if (choice) { + setError(null); if (selectedSet.has(choice.value)) { setSelectedValues(selectedValues.filter(v => v !== choice.value)); } else { setSelectedValues([...selectedValues, choice.value]); } } return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/prompts/searchable-multi-select.ts` around lines 71 - 96, The validation error set in the Enter handler (via validate and setError) is not cleared when the user toggles selections with Space; update the Space branch (where key.name === 'space', choice, and calls to setSelectedValues) to clear the error before modifying selections (call setError('') or setError(undefined)) so changing selection hides the stale validation message; make this change in the same block that uses filteredChoices[cursor], selectedSet, and setSelectedValues.test/core/update.test.ts (1)
1339-1370:⚠️ Potential issue | 🟡 MinorFix contradictory test name and incomplete skill verification list.
The test comment correctly states "Legacy upgrade uses unfiltered templates (all skills)", but the test name
"should create core profile skills when upgrading legacy tools"contradicts this. More importantly, the assertion list checks only 9 skills and omitsopenspec-onboardandopenspec-propose, which are part of the complete 11-skill set created by unfiltered legacy upgrade (seeskill-generation.tslines 67–68).Either rename the test to reflect "all skills" and add both missing skills to the assertion list, or clarify the test's intent if it should only verify a subset.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/core/update.test.ts` around lines 1339 - 1370, The test name and assertions are inconsistent: the it(...) title ("should create core profile skills when upgrading legacy tools") implies a subset but the comment and legacy upgrade behavior use unfiltered templates (all skills). Update the test to either rename the it(...) title to reflect "creates all legacy skills" or change the assertions to match the intended subset; if verifying the unfiltered legacy upgrade, add the two missing skill names "openspec-onboard" and "openspec-propose" to the skillNames array used by the loop that calls UpdateCommand.execute(testDir) and checks FileSystemUtils.fileExists(skillFile); reference UpdateCommand, the skillNames array in this test, FileSystemUtils.fileExists, and the full skill list in skill-generation.ts (lines ~67–68) to ensure the test checks all 11 skills.src/core/init.ts (1)
527-532:⚠️ Potential issue | 🟡 MinorMisleading "no adapter" message when commands are skipped due to delivery config.
When
deliveryis'skills', every tool is pushed tocommandsSkipped(Line 531). The display message at Line 631 says"(no adapter)", which is incorrect — the real reason is the delivery setting. Users may think their tool lacks command support.Proposed fix: distinguish skip reasons
- } else { - commandsSkipped.push(tool.value); } + if (!shouldGenerateCommands) { + // Delivery is skills-only; don't report as "no adapter" + } else if (!adapter) { + commandsSkipped.push(tool.value); + }Or track two separate lists (
commandsSkippedNoAdaptervscommandsSkippedDelivery) and display different messages.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/init.ts` around lines 527 - 532, The code currently always pushes tool.value into commandsSkipped regardless of why (see commandsSkipped and delivery checks), causing the UI to later show "(no adapter)" incorrectly; modify the logic so skipped reasons are tracked: either create two lists (e.g., commandsSkippedNoAdapter and commandsSkippedDelivery) and push tool.value into the appropriate list when delivery === 'skills' vs when there's truly no adapter, or push a small struct/tuple like { name: tool.value, reason: 'delivery'|'no-adapter' } into commandsSkipped; then update the display logic that currently emits "(no adapter)" to inspect the reason and show "(skipped due to delivery)" when reason === 'delivery' (or use the corresponding list) and keep "(no adapter)" only for no-adapter cases.src/core/update.ts (1)
533-536:⚠️ Potential issue | 🟡 MinorPass
profileWorkflowstogetSkillTemplates()andgetCommandContents()in the legacy upgrade path.Lines 535-536 generate skill templates without the active profile's workflow filter. Unlike the main update loop (lines 156-157), which respects the profile, legacy-upgraded tools receive all 11 workflows. Since they're created at the current version, they won't be re-processed, creating inconsistency if a profile restricts to fewer workflows (e.g.,
coreprofile with 4 workflows).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/update.ts` around lines 533 - 536, The legacy-upgrade branch constructs skill templates without applying the active profile's workflow filter: pass the existing profileWorkflows variable into getSkillTemplates(...) and getCommandContents(...) so newlyConfigured tools are created using the same workflow filter as the main update path; update the calls near newlyConfigured to call getSkillTemplates(profileWorkflows) and getCommandContents(profileWorkflows) to ensure legacy-upgraded tools respect the active profile's workflows.
🧹 Nitpick comments (13)
openspec/changes/simplify-skill-installation/specs/cli-init/spec.md (1)
114-127: Consider adding a scenario for re-init whenprofileis already set.The two migration scenarios cover "no profile, has workflows" → migrate, and "no profile, no workflows" → skip. The case "profile already set" is implicit (no migration), but an explicit scenario would complete the decision matrix and avoid ambiguity during implementation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openspec/changes/simplify-skill-installation/specs/cli-init/spec.md` around lines 114 - 127, Add an explicit scenario to the init spec covering the case where a project is re-initialized but the global config already contains a profile: describe that when the user runs `openspec init` and the global config has a `profile` field (regardless of whether workflow files exist), the system SHALL NOT perform the one-time migration and SHALL proceed using the existing profile; reference the same migration behavior note (see `specs/cli-update/spec.md`) to show migration is skipped when `profile` is present.test/prompts/searchable-multi-select.test.ts (2)
83-93: State-index helpers are tightly coupled touseStatecall order in the source.
getSelectedValues()→state[1],getStatus()→state[3],getError()→state[4]depend on the exact order ofuseStatecalls insearchable-multi-select.ts. If anyone reorders or adds a newuseStatecall in the source, these tests will silently read the wrong state slot and produce confusing failures. Consider adding a brief comment documenting this coupling.Suggested documentation
+// NOTE: State indices correspond to useState call order in searchable-multi-select.ts: +// 0 = searchText, 1 = selectedValues, 2 = cursor, 3 = status, 4 = error function getSelectedValues(): string[] { return (state[1] as string[]) ?? []; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/prompts/searchable-multi-select.test.ts` around lines 83 - 93, The helper functions getSelectedValues(), getStatus(), and getError() rely on specific indices in the shared state array (state[1], state[3], state[4]) which are tied to the order of useState calls in searchable-multi-select.ts; add a short comment above these helpers that documents this coupling (mapping index -> meaning) and warns future editors that reordering or adding useState calls in searchable-multi-select.ts requires updating these indices to avoid silent test breakage.
106-121: Fire-and-forget promise never resolves in tests that skip Enter.
mod.searchableMultiSelect(...)at line 108 returns a promise that only resolves whendone()is called (i.e., Enter is pressed). In tests that only press Space or Tab, this promise hangs indefinitely. While Vitest doesn't fail on unresolved promises, this could cause issues with future test-runner strictness or leak detection.Consider tracking and resolving the promise in
afterEachif it matters, or add a comment acknowledging this is intentional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/prompts/searchable-multi-select.test.ts` around lines 106 - 121, The test calls mod.searchableMultiSelect(...) which returns a promise that only resolves when done() (Enter) is pressed, so tests that only press Space/Tab leave that promise pending; capture the returned promise (e.g., store it in a variable like pendingSearchablePromise when calling searchableMultiSelect) and add an afterEach cleanup that resolves or cancels it (by invoking the internal done() handler or a provided cancel/cleanup API from createSearchableMultiSelect/prompt) to ensure no dangling promises, or alternatively add a clear comment next to the call acknowledging the intentional fire-and-forget behavior if you decide not to change behavior.src/cli/index.ts (1)
98-99: Consider using Commander's.choices()for early--profilevalidation.The
--profileflag currently accepts any string and relies on downstream validation inInitCommand. Commander v14 supports.choices()on theOptionclass, which would reject invalid values at parse time with a helpful error message.To implement, import
Optionfrom commander and use.addOption():Suggested change
-import { Command } from 'commander'; +import { Command, Option } from 'commander';.option('--tools <tools>', toolsOptionDescription) .option('--force', 'Auto-cleanup legacy files without prompting') - .option('--profile <profile>', 'Override global config profile (core or custom)') + .addOption(new Option('--profile <profile>', 'Override global config profile (core or custom)').choices(['core', 'custom']))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/index.ts` around lines 98 - 99, Replace the free-form --profile option with a Commander Option that enforces allowed values: import Option from commander, and instead of the existing .option('--profile <profile>', ...) call use .addOption(new Option('--profile <profile>').choices(['core','custom']).description('Override global config profile (core or custom)')) so invalid profiles are rejected at parse time; keep the action handler signature and downstream InitCommand usage unchanged.src/core/config-schema.ts (1)
13-23:KNOWN_TOP_LEVEL_KEYSis not automatically derived from the schema — new optional fields must be manually added.
workflowsis inGlobalConfigSchemabut not inDEFAULT_CONFIG, so it must be explicitly appended to theSet. Any future optional schema field that's also omitted fromDEFAULT_CONFIGwill silently be rejected byopenspec config setunless the same manual addition is remembered.Consider either keeping all schema-known keys in
DEFAULT_CONFIG(withundefinedfor optional ones) or maintaining a separate explicit allowlist:♻️ Explicit allowlist alternative
-const KNOWN_TOP_LEVEL_KEYS = new Set([...Object.keys(DEFAULT_CONFIG), 'workflows']); +// Explicit list: must be kept in sync with GlobalConfigSchema fields +const KNOWN_TOP_LEVEL_KEYS = new Set<string>([ + 'featureFlags', + 'profile', + 'delivery', + 'workflows', +]);Also applies to: 38-38
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/config-schema.ts` around lines 13 - 23, KNOWN_TOP_LEVEL_KEYS is missing schema-only optional fields (e.g., workflows in GlobalConfigSchema) which causes openspec config set to reject them; update the code so the keyset is derived from GlobalConfigSchema or ensure DEFAULT_CONFIG includes every schema key (use undefined for optional ones) or explicitly add omitted optional keys like "workflows" to KNOWN_TOP_LEVEL_KEYS; specifically modify the logic that builds KNOWN_TOP_LEVEL_KEYS (or DEFAULT_CONFIG) to include keys from GlobalConfigSchema or append "workflows" to the Set so new optional fields are not silently rejected.test/core/init.test.ts (1)
80-93:openspec-onboardis absent from the non-core negative assertions.
onboardis not inCORE_WORKFLOWS(['propose', 'explore', 'apply', 'archive']) but is omitted from the list of skills asserted to NOT be created (lines 81–88). The same gap exists foropsx/onboard.mdin the command assertions. Adding it would close the coverage gap for all non-core workflows.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/core/init.test.ts` around lines 80 - 93, Add the missing non-core "onboard" checks: update the nonCoreSkillNames array in test/core/init.test.ts to include 'openspec-onboard' and also add the corresponding negative assertion for the opsx/onboard.md command file in the command assertions block (the same place where other opsx/*.md negatives are checked) so the test asserts that both the skill folder SKILL.md and opsx/onboard.md are not created.test/commands/config.test.ts (1)
253-262: This test exercises no production code — it only validates its own arithmetic.The test body computes
isCoreMatchfromCORE_WORKFLOWS, asserts it'strue, and never calls any production function. Even if the actualconfig profilecommand had a bug for the exactly-core selection case, this test would still pass. Consider replacing it with a call to the real command or at minimum callsaveGlobalConfig/getGlobalConfigto verify the expected config state gets written.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/commands/config.test.ts` around lines 253 - 262, The test currently only compares CORE_WORKFLOWS to itself and exercises no production code; change it to call the real config/profile code and assert persisted state: invoke the command or function that applies the profile (e.g., the CLI handler or API that implements "config profile" — locate and call the function used in tests for other profiles), then read back using getGlobalConfig (or saveGlobalConfig/getGlobalConfig pair) and assert that the saved profile field equals 'core' and that the saved workflows match CORE_WORKFLOWS; keep references to CORE_WORKFLOWS, the config/profile command handler, and getGlobalConfig/saveGlobalConfig to find the right code to update.src/core/global-config.ts (1)
113-131: Schema evolution checks (lines 123–129) are redundant given the preceding spread.
{ ...DEFAULT_CONFIG, ...parsed }already covers the "field absent from loaded config" case:JSON.parsenever produces a key with valueundefined—it simply omits missing keys—soDEFAULT_CONFIG.profile/DEFAULT_CONFIG.deliveryalready win via the spread when those keys are absent fromparsed. The explicit checks add noise without changing behaviour.♻️ Proposed simplification
const merged: GlobalConfig = { ...DEFAULT_CONFIG, ...parsed, // Deep merge featureFlags featureFlags: { ...DEFAULT_CONFIG.featureFlags, ...(parsed.featureFlags || {}) } }; - // Schema evolution: apply defaults for new fields if not present in loaded config - if (parsed.profile === undefined) { - merged.profile = DEFAULT_CONFIG.profile; - } - if (parsed.delivery === undefined) { - merged.delivery = DEFAULT_CONFIG.delivery; - } - return merged;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/global-config.ts` around lines 113 - 131, The explicit schema-evolution checks that set merged.profile and merged.delivery when parsed.profile or parsed.delivery are undefined are redundant because the spread merge const merged = { ...DEFAULT_CONFIG, ...parsed, featureFlags: { ...DEFAULT_CONFIG.featureFlags, ...(parsed.featureFlags || {}) } } already preserves DEFAULT_CONFIG values for keys missing from parsed; remove the if (parsed.profile === undefined) { merged.profile = DEFAULT_CONFIG.profile; } and if (parsed.delivery === undefined) { merged.delivery = DEFAULT_CONFIG.delivery; } blocks to simplify the function and rely on the existing merge logic that produces merged.src/core/shared/skill-generation.ts (1)
56-101: LGTM — filtering logic is correct; optional improvement for eager instantiation.The
Set-based filtering,workflowIdmapping, and pass-through ofworkflowFiltertogetCommandContentsare all correct. One minor note: all templates are eagerly instantiated on every call, even whenworkflowFilterreduces the result to 4 entries. For the current template count this is negligible, but lazy instantiation (e.g., building only matched entries) would avoid wasted work if the template list grows significantly.♻️ Lazy-instantiation variant (only worthwhile if template count grows)
export function getSkillTemplates(workflowFilter?: readonly string[]): SkillTemplateEntry[] { + type LazyEntry = { factory: () => SkillTemplate; dirName: string; workflowId: string }; + const all: LazyEntry[] = [ + { factory: getExploreSkillTemplate, dirName: 'openspec-explore', workflowId: 'explore' }, + { factory: getNewChangeSkillTemplate, dirName: 'openspec-new-change', workflowId: 'new' }, // ... etc. + ]; + const filterSet = workflowFilter ? new Set(workflowFilter) : null; + return all + .filter(e => !filterSet || filterSet.has(e.workflowId)) + .map(e => ({ template: e.factory(), dirName: e.dirName, workflowId: e.workflowId })); - const all: SkillTemplateEntry[] = [ ... ]; // current eager form - if (!workflowFilter) return all; - const filterSet = new Set(workflowFilter); - return all.filter(entry => filterSet.has(entry.workflowId)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/shared/skill-generation.ts` around lines 56 - 101, The current getSkillTemplates and getCommandTemplates eagerly instantiate every template in the local all arrays even when workflowFilter will select only a subset; to fix, change to lazy instantiation by creating a mapping from workflowId/id to a template factory (or switch to building entries only for matches), then if workflowFilter is provided iterate the filterSet and call the corresponding factory to produce and push only matched entries (otherwise keep the existing eager path when no filter is provided); update getSkillTemplates (refs: getExploreSkillTemplate, getNewChangeSkillTemplate, etc., and property workflowId) and getCommandTemplates (refs: getOpsxExploreCommandTemplate, getOpsxNewCommandTemplate, etc., and property id) to use the lazy factory approach.src/core/templates/workflows/propose.ts (1)
1-6: Inaccurate module comment — this is a net-new file, not a split.Lines 4-5 say "This file is generated by splitting the legacy monolithic templates file", but
propose.tsis a brand-new workflow template, not extracted from any existing file. This comment is copy-pasted from the other workflow modules where it is accurate.✏️ Suggested fix
/** - * Skill Template Workflow Modules - * - * This file is generated by splitting the legacy monolithic - * templates file into workflow-focused modules. + * Propose Workflow Templates + * + * Provides the skill and command templates for the openspec-propose workflow, + * which combines change creation and artifact generation into a single step. */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/templates/workflows/propose.ts` around lines 1 - 6, The file header comment in propose.ts incorrectly states it was generated by splitting a legacy monolithic templates file; update the top-of-file comment to reflect that propose.ts is a net-new workflow template (remove or replace the lines "This file is generated by splitting the legacy monolithic templates file into workflow-focused modules." with a brief accurate description such as "Net-new workflow template for the propose workflow"). Edit the comment block at the top of propose.ts (the module header) to be accurate and keep any existing licensing/attribution lines intact.src/core/migration.ts (1)
62-74: Double config read is a necessary pattern, but the side-effect on save is worth documenting.
migrateIfNeededreads the config file twice — once viagetGlobalConfig()(line 62, to get the merged object to mutate) and again manually (lines 65–74, to check whetherprofilewas explicitly set vs defaulted). The double read is intentional becausegetGlobalConfig()always populatesprofileanddeliveryfrom defaults, makingconfig.profileuseless for detecting intent.A side-effect: the object saved on line 92 is the merged config, so
delivery: 'both'is persisted even if the user never set it. Sincedeliveryis new to this PR this is harmless, but worth a brief comment to prevent future confusion.📝 Suggested inline documentation
// Migrate: set profile to custom with detected workflows + // Note: `config` is the defaults-merged object from getGlobalConfig(), so + // persisting it will also materialise default values (e.g., delivery:'both') + // for newly-added fields. This is intentional for first-run migration. config.profile = 'custom'; config.workflows = installedWorkflows; saveGlobalConfig(config);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/migration.ts` around lines 62 - 74, migrateIfNeeded currently calls getGlobalConfig() to obtain the merged config and then re-reads the raw file into rawConfig to detect whether fields like profile were explicitly set; add a concise inline comment by that double-read block (referencing migrateIfNeeded, getGlobalConfig, and rawConfig) explaining this pattern and its side-effect: the subsequent save persists the merged object (so defaults such as delivery will be written even if not user-set). Ensure the comment also notes this is intentional and harmless for the new delivery field to prevent future confusion.src/core/update.ts (2)
237-246: Legacy upgrade "Getting started" message references old workflow commands, not profile-aware ones.The message hardcodes
/opsx:newand/opsx:continue, but after this PR the primary entry point is/opsx:proposefor core-profile users. Consider making this message consistent with the active profile, similar to whatinit.tsdoes at the end (Lines 652-660).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/update.ts` around lines 237 - 246, The onboarding message shown when newlyConfiguredTools.length > 0 references hardcoded legacy commands (/opsx:new, /opsx:continue, /opsx:apply) instead of using the profile-aware entrypoint; update this block to select and display the correct command for the active profile (e.g., /opsx:propose for core-profile) the same way init.ts does. Replace the hardcoded lines with a lookup or helper that returns the profile-specific entry command (reuse the logic or helper from init.ts), and output that command in the "Getting started" message so the instructions match the current profile and tooling. Ensure you still show Learn more and preserve the bold header and spacing.
49-64:WORKFLOW_TO_SKILL_DIRduplicates knowledge already present ingetSkillTemplates().This map is defined in both
src/core/update.tsandsrc/core/migration.ts, but the authoritative source isgetSkillTemplates()insrc/core/shared/skill-generation.ts, which returns each workflow'sdirNamealongside its template and ID. If a workflow is added or its directory name changes, these hardcoded maps must be updated in lockstep, risking inconsistency.Consider deriving
WORKFLOW_TO_SKILL_DIRfromgetSkillTemplates()to maintain a single source of truth:// Build the map once at module load const WORKFLOW_TO_SKILL_DIR = Object.fromEntries( getSkillTemplates().map(({ workflowId, dirName }) => [workflowId, dirName]) );This eliminates duplication across
update.tsandmigration.ts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/update.ts` around lines 49 - 64, WORKFLOW_TO_SKILL_DIR duplicates the workflow->dirName mapping already produced by getSkillTemplates(); replace the hardcoded Record in update.ts (and remove the duplicate in migration.ts) by deriving the map from getSkillTemplates() at module load (e.g., build an object via mapping each template's workflowId to its dirName) so the single source of truth is getSkillTemplates(); update references to WORKFLOW_TO_SKILL_DIR to use this derived map and ensure imports reference getSkillTemplates()/skill template provider rather than hardcoded values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openspec/changes/simplify-skill-installation/design.md`:
- Around line 139-193: The section numbering is out of order: "### 8. Existing
User Migration" and "### 9. Generic Next-Step Guidance" appear before "### 7.
Fix Multi-Select Keybindings"; renumber or reorder these headings so the
document flows sequentially (e.g., move "Existing User Migration" and "Generic
Next-Step Guidance" after "Fix Multi-Select Keybindings" or renumber them to
7/8/9 consistently), and update any in-file references or cross-links that
mention the old section numbers to the new numbers; locate headings by the exact
strings "### 8. Existing User Migration", "### 9. Generic Next-Step Guidance",
and "### 7. Fix Multi-Select Keybindings" to apply the fix.
In `@src/commands/config.ts`:
- Around line 78-86: Update the display logic in the printing block that shows
workflows (the code using CORE_WORKFLOWS and config.workflows in
src/commands/config.ts) to base the label on the effective profile/workflows
returned by getProfileWorkflows rather than merely the presence of
config.workflows; call getProfileWorkflows(config.profile, config.workflows) (or
otherwise compute the effective workflows) and if the effective list equals
CORE_WORKFLOWS print "workflows: <list> (from core profile)", otherwise if
config.workflows was actually the source show "workflows: <list> (explicit)";
use CORE_WORKFLOWS, getProfileWorkflows, and config.workflows to decide the
correct label.
- Around line 341-347: The block that runs npx openspec update should stop doing
a dynamic import and must handle failures: add execSync to the top-level import
from 'node:child_process' (alongside the existing spawn import) and replace the
dynamic import in the applyNow branch; wrap the execSync('npx openspec update',
{ stdio: 'inherit', cwd: projectDir }) call in a try/catch so failures
(SpawnSyncError) are caught and report a friendly error (use console.error or
the module logger) that includes the error message and a short hint, then return
or process.exit(1) as appropriate; refer to execSync, spawn, applyNow and
projectDir to locate the change.
In `@src/core/init.ts`:
- Around line 117-120: Resolve profile/workflows once in execute(): call
getGlobalConfig(), compute profile using this.profileOverride ||
globalConfig.profile || 'core', then compute workflows via
getProfileWorkflows(profile, globalConfig.workflows) and pass both the resolved
globalConfig/profile/workflows into generateSkillsAndCommands() and
displaySuccessMessage() as parameters; remove the duplicated
getGlobalConfig()/profile/workflows resolution from generateSkillsAndCommands()
and displaySuccessMessage() so those helpers use the passed-in values
(references: execute(), generateSkillsAndCommands(), displaySuccessMessage(),
getGlobalConfig(), getProfileWorkflows, this.profileOverride, profile,
workflows).
- Line 119: The code unsafely casts this.profileOverride to Profile; before
using it (the const profile assignment) validate that this.profileOverride is
one of the allowed Profile union members (e.g., 'core' | 'custom' | ...) instead
of using as Profile, and only use it when valid; otherwise fall back to
globalConfig.profile or 'core' and optionally log a warning. Implement a small
type-guard or lookup (used where profileOverride is read in init.ts) to check
allowed values, then replace the direct cast at the const profile = ... line
with conditional selection using that validator so getProfileWorkflows and
CORE_WORKFLOWS only receive legitimate Profile values.
In `@src/core/shared/tool-detection.ts`:
- Around line 32-44: SKILL_NAMES is missing the 'openspec-onboard' entry which
causes getToolSkillStatus (and its skill existence loop) to skip tracking the
onboard skill; update the SKILL_NAMES constant to include 'openspec-onboard'
(matching COMMAND_IDS and getSkillTemplates behavior) so that functions like
getToolSkillStatus, getSkillTemplates, and any code relying on SKILL_NAMES
correctly count and check the onboard skill.
In `@src/core/update.ts`:
- Around line 333-352: The silent catch in removeSkillDirs hides filesystem
errors; change it to catch the error (e.g., catch (err)) and emit a warning
including the skillDir path and err details using the module/class logger (same
pattern as used in removeCommandFiles) so failures to rm (fs.promises.rm) are
visible; apply the same change to removeCommandFiles and any other similar catch
blocks that swallow fs errors so the removal count accurately reflects failed
removals and provides diagnostic info.
In `@test/core/init.test.ts`:
- Around line 13-29: configTempDir is declared inside beforeEach so afterEach
cannot remove it, leaking temp dirs; declare configTempDir alongside testDir and
originalEnv at the top-level of the test suite, assign it inside beforeEach
(where it is currently created), and add an await fs.rm(configTempDir, {
recursive: true, force: true }) in afterEach (mirror the cleanup for testDir);
apply the same change to the second describe block that currently declares its
own local configTempDir so both suites properly clean up their temp config
directories.
- Line 3: Remove the unused import statement for fsSync: delete the line
importing "fsSync" (import * as fsSync from 'fs') since all file operations use
the existing "fs" import; ensure no references to fsSync remain (search for the
symbol fsSync) and run tests to confirm nothing breaks.
---
Outside diff comments:
In `@src/core/init.ts`:
- Around line 527-532: The code currently always pushes tool.value into
commandsSkipped regardless of why (see commandsSkipped and delivery checks),
causing the UI to later show "(no adapter)" incorrectly; modify the logic so
skipped reasons are tracked: either create two lists (e.g.,
commandsSkippedNoAdapter and commandsSkippedDelivery) and push tool.value into
the appropriate list when delivery === 'skills' vs when there's truly no
adapter, or push a small struct/tuple like { name: tool.value, reason:
'delivery'|'no-adapter' } into commandsSkipped; then update the display logic
that currently emits "(no adapter)" to inspect the reason and show "(skipped due
to delivery)" when reason === 'delivery' (or use the corresponding list) and
keep "(no adapter)" only for no-adapter cases.
In `@src/core/update.ts`:
- Around line 533-536: The legacy-upgrade branch constructs skill templates
without applying the active profile's workflow filter: pass the existing
profileWorkflows variable into getSkillTemplates(...) and
getCommandContents(...) so newlyConfigured tools are created using the same
workflow filter as the main update path; update the calls near newlyConfigured
to call getSkillTemplates(profileWorkflows) and
getCommandContents(profileWorkflows) to ensure legacy-upgraded tools respect the
active profile's workflows.
In `@src/prompts/searchable-multi-select.ts`:
- Around line 71-96: The validation error set in the Enter handler (via validate
and setError) is not cleared when the user toggles selections with Space; update
the Space branch (where key.name === 'space', choice, and calls to
setSelectedValues) to clear the error before modifying selections (call
setError('') or setError(undefined)) so changing selection hides the stale
validation message; make this change in the same block that uses
filteredChoices[cursor], selectedSet, and setSelectedValues.
In `@test/core/update.test.ts`:
- Around line 1339-1370: The test name and assertions are inconsistent: the
it(...) title ("should create core profile skills when upgrading legacy tools")
implies a subset but the comment and legacy upgrade behavior use unfiltered
templates (all skills). Update the test to either rename the it(...) title to
reflect "creates all legacy skills" or change the assertions to match the
intended subset; if verifying the unfiltered legacy upgrade, add the two missing
skill names "openspec-onboard" and "openspec-propose" to the skillNames array
used by the loop that calls UpdateCommand.execute(testDir) and checks
FileSystemUtils.fileExists(skillFile); reference UpdateCommand, the skillNames
array in this test, FileSystemUtils.fileExists, and the full skill list in
skill-generation.ts (lines ~67–68) to ensure the test checks all 11 skills.
---
Nitpick comments:
In `@openspec/changes/simplify-skill-installation/specs/cli-init/spec.md`:
- Around line 114-127: Add an explicit scenario to the init spec covering the
case where a project is re-initialized but the global config already contains a
profile: describe that when the user runs `openspec init` and the global config
has a `profile` field (regardless of whether workflow files exist), the system
SHALL NOT perform the one-time migration and SHALL proceed using the existing
profile; reference the same migration behavior note (see
`specs/cli-update/spec.md`) to show migration is skipped when `profile` is
present.
In `@src/cli/index.ts`:
- Around line 98-99: Replace the free-form --profile option with a Commander
Option that enforces allowed values: import Option from commander, and instead
of the existing .option('--profile <profile>', ...) call use .addOption(new
Option('--profile <profile>').choices(['core','custom']).description('Override
global config profile (core or custom)')) so invalid profiles are rejected at
parse time; keep the action handler signature and downstream InitCommand usage
unchanged.
In `@src/core/config-schema.ts`:
- Around line 13-23: KNOWN_TOP_LEVEL_KEYS is missing schema-only optional fields
(e.g., workflows in GlobalConfigSchema) which causes openspec config set to
reject them; update the code so the keyset is derived from GlobalConfigSchema or
ensure DEFAULT_CONFIG includes every schema key (use undefined for optional
ones) or explicitly add omitted optional keys like "workflows" to
KNOWN_TOP_LEVEL_KEYS; specifically modify the logic that builds
KNOWN_TOP_LEVEL_KEYS (or DEFAULT_CONFIG) to include keys from GlobalConfigSchema
or append "workflows" to the Set so new optional fields are not silently
rejected.
In `@src/core/global-config.ts`:
- Around line 113-131: The explicit schema-evolution checks that set
merged.profile and merged.delivery when parsed.profile or parsed.delivery are
undefined are redundant because the spread merge const merged = {
...DEFAULT_CONFIG, ...parsed, featureFlags: { ...DEFAULT_CONFIG.featureFlags,
...(parsed.featureFlags || {}) } } already preserves DEFAULT_CONFIG values for
keys missing from parsed; remove the if (parsed.profile === undefined) {
merged.profile = DEFAULT_CONFIG.profile; } and if (parsed.delivery ===
undefined) { merged.delivery = DEFAULT_CONFIG.delivery; } blocks to simplify the
function and rely on the existing merge logic that produces merged.
In `@src/core/migration.ts`:
- Around line 62-74: migrateIfNeeded currently calls getGlobalConfig() to obtain
the merged config and then re-reads the raw file into rawConfig to detect
whether fields like profile were explicitly set; add a concise inline comment by
that double-read block (referencing migrateIfNeeded, getGlobalConfig, and
rawConfig) explaining this pattern and its side-effect: the subsequent save
persists the merged object (so defaults such as delivery will be written even if
not user-set). Ensure the comment also notes this is intentional and harmless
for the new delivery field to prevent future confusion.
In `@src/core/shared/skill-generation.ts`:
- Around line 56-101: The current getSkillTemplates and getCommandTemplates
eagerly instantiate every template in the local all arrays even when
workflowFilter will select only a subset; to fix, change to lazy instantiation
by creating a mapping from workflowId/id to a template factory (or switch to
building entries only for matches), then if workflowFilter is provided iterate
the filterSet and call the corresponding factory to produce and push only
matched entries (otherwise keep the existing eager path when no filter is
provided); update getSkillTemplates (refs: getExploreSkillTemplate,
getNewChangeSkillTemplate, etc., and property workflowId) and
getCommandTemplates (refs: getOpsxExploreCommandTemplate,
getOpsxNewCommandTemplate, etc., and property id) to use the lazy factory
approach.
In `@src/core/templates/workflows/propose.ts`:
- Around line 1-6: The file header comment in propose.ts incorrectly states it
was generated by splitting a legacy monolithic templates file; update the
top-of-file comment to reflect that propose.ts is a net-new workflow template
(remove or replace the lines "This file is generated by splitting the legacy
monolithic templates file into workflow-focused modules." with a brief accurate
description such as "Net-new workflow template for the propose workflow"). Edit
the comment block at the top of propose.ts (the module header) to be accurate
and keep any existing licensing/attribution lines intact.
In `@src/core/update.ts`:
- Around line 237-246: The onboarding message shown when
newlyConfiguredTools.length > 0 references hardcoded legacy commands (/opsx:new,
/opsx:continue, /opsx:apply) instead of using the profile-aware entrypoint;
update this block to select and display the correct command for the active
profile (e.g., /opsx:propose for core-profile) the same way init.ts does.
Replace the hardcoded lines with a lookup or helper that returns the
profile-specific entry command (reuse the logic or helper from init.ts), and
output that command in the "Getting started" message so the instructions match
the current profile and tooling. Ensure you still show Learn more and preserve
the bold header and spacing.
- Around line 49-64: WORKFLOW_TO_SKILL_DIR duplicates the workflow->dirName
mapping already produced by getSkillTemplates(); replace the hardcoded Record in
update.ts (and remove the duplicate in migration.ts) by deriving the map from
getSkillTemplates() at module load (e.g., build an object via mapping each
template's workflowId to its dirName) so the single source of truth is
getSkillTemplates(); update references to WORKFLOW_TO_SKILL_DIR to use this
derived map and ensure imports reference getSkillTemplates()/skill template
provider rather than hardcoded values.
In `@test/commands/config.test.ts`:
- Around line 253-262: The test currently only compares CORE_WORKFLOWS to itself
and exercises no production code; change it to call the real config/profile code
and assert persisted state: invoke the command or function that applies the
profile (e.g., the CLI handler or API that implements "config profile" — locate
and call the function used in tests for other profiles), then read back using
getGlobalConfig (or saveGlobalConfig/getGlobalConfig pair) and assert that the
saved profile field equals 'core' and that the saved workflows match
CORE_WORKFLOWS; keep references to CORE_WORKFLOWS, the config/profile command
handler, and getGlobalConfig/saveGlobalConfig to find the right code to update.
In `@test/core/init.test.ts`:
- Around line 80-93: Add the missing non-core "onboard" checks: update the
nonCoreSkillNames array in test/core/init.test.ts to include 'openspec-onboard'
and also add the corresponding negative assertion for the opsx/onboard.md
command file in the command assertions block (the same place where other
opsx/*.md negatives are checked) so the test asserts that both the skill folder
SKILL.md and opsx/onboard.md are not created.
In `@test/prompts/searchable-multi-select.test.ts`:
- Around line 83-93: The helper functions getSelectedValues(), getStatus(), and
getError() rely on specific indices in the shared state array (state[1],
state[3], state[4]) which are tied to the order of useState calls in
searchable-multi-select.ts; add a short comment above these helpers that
documents this coupling (mapping index -> meaning) and warns future editors that
reordering or adding useState calls in searchable-multi-select.ts requires
updating these indices to avoid silent test breakage.
- Around line 106-121: The test calls mod.searchableMultiSelect(...) which
returns a promise that only resolves when done() (Enter) is pressed, so tests
that only press Space/Tab leave that promise pending; capture the returned
promise (e.g., store it in a variable like pendingSearchablePromise when calling
searchableMultiSelect) and add an afterEach cleanup that resolves or cancels it
(by invoking the internal done() handler or a provided cancel/cleanup API from
createSearchableMultiSelect/prompt) to ensure no dangling promises, or
alternatively add a clear comment next to the call acknowledging the intentional
fire-and-forget behavior if you decide not to change behavior.
| ### 8. Existing User Migration | ||
|
|
||
| When `openspec init` or `openspec update` encounters a project with existing workflows but no `profile` field in global config, it performs a one-time migration to preserve the user's current setup. | ||
|
|
||
| **Rationale:** Without migration, existing users would default to `core` profile, causing `propose` to be added on top of their 10 workflows — making things worse, not better. Migration ensures existing users keep exactly what they have. | ||
|
|
||
| **Triggered by:** Both `init` (re-init on existing project) and `update`. The migration check is a shared function called early in both commands, before profile resolution. | ||
|
|
||
| **Detection logic:** | ||
| ```typescript | ||
| // Shared migration check, called by both init and update: | ||
| function migrateIfNeeded(projectPath: string, tools: AiTool[]): void { | ||
| const globalConfig = readGlobalConfig(); | ||
| if (globalConfig.profile) return; // already migrated or explicitly set | ||
|
|
||
| const installedWorkflows = scanInstalledWorkflows(projectPath, tools); | ||
| if (installedWorkflows.length === 0) return; // new user, use core defaults | ||
|
|
||
| // Existing user — migrate to custom profile | ||
| writeGlobalConfig({ | ||
| ...globalConfig, | ||
| profile: 'custom', | ||
| delivery: 'both', | ||
| workflows: installedWorkflows, | ||
| }); | ||
| } | ||
| ``` | ||
|
|
||
| **Scanning logic:** | ||
| - Scan all tool directories (`.claude/skills/`, `.cursor/skills/`, etc.) for workflow directories/files | ||
| - Match only against `ALL_WORKFLOWS` constant — ignore user-created custom skills/commands | ||
| - Map directory names back to workflow IDs (e.g., `openspec-explore/` → `explore`, `opsx-explore.md` → `explore`) | ||
| - Take the union of detected workflow names across all tools | ||
|
|
||
| **Edge cases:** | ||
| - **User manually deleted some workflows:** Migration scans what's actually installed, respecting their choices | ||
| - **Multiple projects with different workflow sets:** First project to trigger migration sets global config; subsequent projects use it | ||
| - **User has custom (non-OpenSpec) skills in the directory:** Ignored — scanner only matches known workflow IDs from `ALL_WORKFLOWS` | ||
| - **Migration is idempotent:** If `profile` is already set in config, no re-migration occurs | ||
| - **Non-interactive (CI):** Same migration logic, no confirmation needed — it's preserving existing state | ||
|
|
||
| **Alternatives considered:** | ||
| - Migrate during `init` instead of `update`: Init already has its own flow (tool selection, etc.). Mixing migration with init creates confusing UX | ||
| - Don't migrate, just default to core: Breaks existing users by adding `propose` and showing "extra workflows" warnings | ||
| - Migrate at global config read time: Too implicit, hard to show feedback to user | ||
|
|
||
| ### 9. Generic Next-Step Guidance in Templates | ||
|
|
||
| Workflow templates use generic, concept-based next-step guidance rather than referencing specific workflow commands. For example, instead of "run `/opsx:propose`", templates say "create a change proposal". | ||
|
|
||
| **Rationale:** Conditional cross-referencing (where each template checks which other workflows are installed and renders different command names) adds significant complexity to template generation, testing, and maintenance. Generic guidance avoids this entirely while still being useful — users already know their installed workflows. | ||
|
|
||
| **Note:** If we find that users consistently struggle to map concepts to commands, we can revisit this with conditional cross-references. For now, simplicity wins. | ||
|
|
||
| ### 7. Fix Multi-Select Keybindings |
There was a problem hiding this comment.
Section numbering is out of order — sections 8 and 9 precede section 7 in the file.
Sections 8 ("Existing User Migration") and 9 ("Generic Next-Step Guidance") were inserted before the existing section 7 ("Fix Multi-Select Keybindings") rather than after it. Consider renumbering or reordering to keep the document sequential.
🧰 Tools
🪛 LanguageTool
[style] ~183-~183: To elevate your writing, try using a synonym here.
Context: ... global config read time: Too implicit, hard to show feedback to user ### 9. Generi...
(HARD_TO)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openspec/changes/simplify-skill-installation/design.md` around lines 139 -
193, The section numbering is out of order: "### 8. Existing User Migration" and
"### 9. Generic Next-Step Guidance" appear before "### 7. Fix Multi-Select
Keybindings"; renumber or reorder these headings so the document flows
sequentially (e.g., move "Existing User Migration" and "Generic Next-Step
Guidance" after "Fix Multi-Select Keybindings" or renumber them to 7/8/9
consistently), and update any in-file references or cross-links that mention the
old section numbers to the new numbers; locate headings by the exact strings
"### 8. Existing User Migration", "### 9. Generic Next-Step Guidance", and "###
7. Fix Multi-Select Keybindings" to apply the fix.
| console.log(`\nProfile settings:`); | ||
| console.log(` profile: ${config.profile} ${profileSource}`); | ||
| console.log(` delivery: ${config.delivery} ${deliverySource}`); | ||
| if (config.workflows) { | ||
| console.log(` workflows: ${config.workflows.join(', ')} (explicit)`); | ||
| } else { | ||
| const coreList = CORE_WORKFLOWS.join(', '); | ||
| console.log(` workflows: ${coreList} (from core profile)`); | ||
| } |
There was a problem hiding this comment.
config list always shows (explicit) for core profile workflows, which is misleading.
After openspec config profile core (or the interactive picker selects exactly core workflows), config.workflows is written to disk. The config list display on lines 81–86 then shows workflows: propose, explore, apply, archive (explicit) — but for core profile, getProfileWorkflows ignores config.workflows entirely, so labelling it "explicit" suggests user control that doesn't exist.
The display logic should be driven by the effective profile, not the presence of the workflows key:
🔧 Proposed fix for config list display
- if (config.workflows) {
- console.log(` workflows: ${config.workflows.join(', ')} (explicit)`);
- } else {
- const coreList = CORE_WORKFLOWS.join(', ');
- console.log(` workflows: ${coreList} (from core profile)`);
- }
+ if (config.profile === 'core') {
+ console.log(` workflows: ${CORE_WORKFLOWS.join(', ')} (from core profile)`);
+ } else if (config.workflows && config.workflows.length > 0) {
+ console.log(` workflows: ${config.workflows.join(', ')} (explicit)`);
+ } else {
+ console.log(` workflows: (none)`);
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/commands/config.ts` around lines 78 - 86, Update the display logic in the
printing block that shows workflows (the code using CORE_WORKFLOWS and
config.workflows in src/commands/config.ts) to base the label on the effective
profile/workflows returned by getProfileWorkflows rather than merely the
presence of config.workflows; call getProfileWorkflows(config.profile,
config.workflows) (or otherwise compute the effective workflows) and if the
effective list equals CORE_WORKFLOWS print "workflows: <list> (from core
profile)", otherwise if config.workflows was actually the source show
"workflows: <list> (explicit)"; use CORE_WORKFLOWS, getProfileWorkflows, and
config.workflows to decide the correct label.
| // Resolve profile (--profile flag overrides global config) (task 7.7) | ||
| const globalConfig = getGlobalConfig(); | ||
| const profile: Profile = (this.profileOverride as Profile) ?? globalConfig.profile ?? 'core'; | ||
| const workflows = getProfileWorkflows(profile, globalConfig.workflows); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Profile/delivery/workflows resolved four separate times — DRY violation and latent inconsistency risk.
getGlobalConfig() is called and profile/delivery/workflows are resolved independently in execute() (Line 118), generateSkillsAndCommands() (Line 481), and twice in displaySuccessMessage() (Lines 608, 648). Each invocation re-reads from disk and re-applies the same override/default logic.
Resolve once in execute() and thread the result through as a parameter to the downstream methods.
Sketch: resolve once, pass through
// In execute(), after line 120:
+ const resolvedConfig = { profile, delivery, workflows };
// Pass to generateSkillsAndCommands:
- const results = await this.generateSkillsAndCommands(projectPath, validatedTools);
+ const results = await this.generateSkillsAndCommands(projectPath, validatedTools, resolvedConfig);
// Pass to displaySuccessMessage:
- this.displaySuccessMessage(projectPath, validatedTools, results, configStatus);
+ this.displaySuccessMessage(projectPath, validatedTools, results, configStatus, resolvedConfig);Then remove the duplicated getGlobalConfig() + resolution logic from both helper methods.
Also applies to: 480-484, 606-611, 647-650
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/init.ts` around lines 117 - 120, Resolve profile/workflows once in
execute(): call getGlobalConfig(), compute profile using this.profileOverride ||
globalConfig.profile || 'core', then compute workflows via
getProfileWorkflows(profile, globalConfig.workflows) and pass both the resolved
globalConfig/profile/workflows into generateSkillsAndCommands() and
displaySuccessMessage() as parameters; remove the duplicated
getGlobalConfig()/profile/workflows resolution from generateSkillsAndCommands()
and displaySuccessMessage() so those helpers use the passed-in values
(references: execute(), generateSkillsAndCommands(), displaySuccessMessage(),
getGlobalConfig(), getProfileWorkflows, this.profileOverride, profile,
workflows).
| private async removeSkillDirs(skillsDir: string): Promise<number> { | ||
| let removed = 0; | ||
|
|
||
| for (const workflow of ALL_WORKFLOWS) { | ||
| const dirName = WORKFLOW_TO_SKILL_DIR[workflow]; | ||
| if (!dirName) continue; | ||
|
|
||
| const skillDir = path.join(skillsDir, dirName); | ||
| try { | ||
| if (fs.existsSync(skillDir)) { | ||
| await fs.promises.rm(skillDir, { recursive: true, force: true }); | ||
| removed++; | ||
| } | ||
| } catch { | ||
| // Ignore errors | ||
| } | ||
| } | ||
|
|
||
| return removed; | ||
| } |
There was a problem hiding this comment.
Silent catch blocks hide file-system errors during delivery cleanup.
If rm or unlink fails due to permission errors or other non-transient issues, the user sees an incorrect removal count and has no indication that cleanup was incomplete. At minimum, log a warning so users can diagnose stale artifacts.
Proposed fix: log warnings instead of silently ignoring
} catch {
- // Ignore errors
+ // Log but don't fail the overall update
+ console.log(chalk.dim(` Warning: could not remove ${skillDir}`));
}Apply the same pattern to removeCommandFiles:
} catch {
- // Ignore errors
+ console.log(chalk.dim(` Warning: could not remove ${fullPath}`));
}Also applies to: 354-382
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/update.ts` around lines 333 - 352, The silent catch in
removeSkillDirs hides filesystem errors; change it to catch the error (e.g.,
catch (err)) and emit a warning including the skillDir path and err details
using the module/class logger (same pattern as used in removeCommandFiles) so
failures to rm (fs.promises.rm) are visible; apply the same change to
removeCommandFiles and any other similar catch blocks that swallow fs errors so
the removal count accurately reflects failed removals and provides diagnostic
info.
…ofile validation Address post-implementation review findings: update now detects profile/delivery drift even when template versions are current, recognizes command-only installs as configured tools, init validates --profile values and applies delivery cleanup on re-init. Specs and design docs updated with new scenarios and rationale.
- Add error handling for execSync in config profile apply and use static import - Fix config list showing "(explicit)" for core profile workflows misleadingly - Add missing 'openspec-onboard' to SKILL_NAMES for parity with COMMAND_IDS - Remove unused fsSync import in init tests - Fix configTempDir leak in test afterEach cleanup
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
src/core/shared/tool-detection.ts (1)
96-107:fullyConfiguredsemantics may be misleading with profile-driven installs.With the new profile system, core profile installs only 4 of 11 skills.
fullyConfigured(line 105:skillCount === SKILL_NAMES.length) will therefore befalsefor any core-profile user, even if their installation is complete per their profile. Since no critical code path depends onfullyConfiguredbeingtrue, this is not a bug, but consumers may find it confusing.Consider either renaming to
allSkillsPresentto clarify it means "every known skill exists" regardless of profile, or accepting a profile-aware count. Low priority — the current behavior is technically correct.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/shared/tool-detection.ts` around lines 96 - 107, The returned property fullyConfigured is misleading because it compares skillCount to SKILL_NAMES regardless of profile; update the return shape to either (a) rename fullyConfigured to allSkillsPresent and keep the current semantics (so change the property name in the return object and update any consumers to use allSkillsPresent), or (b) make it profile-aware by comparing skillCount to the active profile's expected skill list (use the profile's list instead of SKILL_NAMES) and return fullyConfigured only when all profile-required skills are present; reference SKILL_NAMES, skillCount and the return object in tool-detection.ts when making the change.src/core/update.ts (1)
265-274: Onboarding message for legacy-upgraded tools references/opsx:newand/opsx:continue, which are not in the core profile.After legacy upgrade, the
upgradeLegacyToolsmethod (line 670) callsgetSkillTemplates()without a workflow filter, installing all workflows. So the getting-started message referencing/opsx:newand/opsx:continueis technically correct for this path.However, if the user later runs
openspec update, the core profile (default) will only generate 4 workflows (propose, explore, apply, archive). The/opsx:newand/opsx:continuecommands will still exist but won't be refreshed. Consider updating this message to reference/opsx:proposeinstead, which is the core-profile equivalent that consolidatesnew+ff.Suggested update for the getting-started message
if (newlyConfiguredTools.length > 0) { console.log(); console.log(chalk.bold('Getting started:')); - console.log(' /opsx:new Start a new change'); - console.log(' /opsx:continue Create the next artifact'); - console.log(' /opsx:apply Implement tasks'); + console.log(' /opsx:propose Move from idea to implementation-ready'); + console.log(' /opsx:explore Explore the codebase'); + console.log(' /opsx:apply Implement tasks'); console.log(); console.log(`Learn more: ${chalk.cyan('https://github.com/Fission-AI/OpenSpec')}`); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/update.ts` around lines 265 - 274, The onboarding message printed when newlyConfiguredTools.length > 0 should not reference /opsx:new and /opsx:continue for the core profile; update the message in the block that prints "Getting started:" (the code using newlyConfiguredTools) to recommend /opsx:propose (which consolidates new+ff) and keep /opsx:apply and /opsx:archive as appropriate; if you want to preserve legacy accuracy from upgradeLegacyTools and getSkillTemplates(), consider making the message conditional on whether upgradeLegacyTools() path installed non-core workflows, but minimally change the static lines in the printing block to show /opsx:propose instead of /opsx:new and /opsx:continue.src/commands/config.ts (1)
269-277: Core preset writesworkflowsto config, whichgetProfileWorkflows('core', ...)ignores.Line 272 sets
config.workflows = [...CORE_WORKFLOWS]when applying thecorepreset. SincegetProfileWorkflows('core', customWorkflows)returnsCORE_WORKFLOWSregardless ofcustomWorkflows, the written value is never consumed for profile resolution. This is harmless but creates a subtle inconsistency: the storedworkflowsarray looks "explicit" but has no functional effect for core profile.This is consistent with the interactive picker path (line 328) which also always writes workflows, so the pattern is at least internally consistent. No action required unless you want to clean it up.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/config.ts` around lines 269 - 277, The code sets config.workflows = [...CORE_WORKFLOWS] when preset === 'core' but getProfileWorkflows('core', customWorkflows) ignores customWorkflows and always returns CORE_WORKFLOWS, producing a stored value that's never used; either stop writing workflows for the core preset (remove the config.workflows assignment in the preset === 'core' branch) or change getProfileWorkflows to consult the passed customWorkflows for the 'core' profile so the stored array is respected (choose one approach and make the change consistently with the interactive picker path).test/core/update.test.ts (1)
1309-1340: Addopenspec-onboardandopenspec-proposeto the legacy upgrade skill check.The test comment correctly states that legacy upgrade uses unfiltered templates, but the assertion only verifies 9 of the 11 skills. Since
getSkillTemplates()called without arguments returns all 11 templates (including the two newer skills), adding assertions foropenspec-onboardandopenspec-proposeimproves test completeness.Add onboard and propose to the legacy upgrade skill check
const skillNames = [ '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', + 'openspec-onboard', + 'openspec-propose', ];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/core/update.test.ts` around lines 1309 - 1340, Update the test "should create core profile skills when upgrading legacy tools" by adding the two missing legacy templates to the asserted skill list: include 'openspec-onboard' and 'openspec-propose' in the skillNames array so the loop verifies all templates returned by getSkillTemplates() are present after running new UpdateCommand({ force: true }).execute(testDir); keep the rest of the test (skillsDir, FileSystemUtils.fileExists checks) unchanged.src/core/init.ts (2)
649-710:(this.profileOverride as Profile)bypassesresolveProfileOverride()indisplaySuccessMessage.Lines 653 and 699 use
(this.profileOverride as Profile)directly, while the rest of the class consistently usesthis.resolveProfileOverride()for the validated resolution. This is safe at runtime (an invalid profile would have thrown earlier inexecute()), but the inconsistency creates a maintenance hazard — future refactors that calldisplaySuccessMessagein a different order could introduce a silent cast of an invalid value.♻️ Proposed fix
- const profile: Profile = (this.profileOverride as Profile) ?? globalConfig.profile ?? 'core'; + const profile: Profile = this.resolveProfileOverride() ?? globalConfig.profile ?? 'core';Apply the same change to line 699 (
activeProfile).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/init.ts` around lines 649 - 710, In displaySuccessMessage replace the direct casts of this.profileOverride with a validated call to this.resolveProfileOverride(): specifically change the profile assignment used for the skills/commands counts (currently setting profile via (this.profileOverride as Profile) at the block that defines profile) and the activeProfile assignment near the "Getting started" section (currently using (this.profileOverride as Profile)) to use this.resolveProfileOverride() so both profile and activeProfile are resolved/validated the same way as elsewhere in the class.
527-567:if (flag)+if (!flag)should beif/else— mutually exclusive branches.
shouldGenerateSkillsandshouldGenerateCommandsare constant booleans within the loop. Writing two separateifblocks instead ofif/elseis misleading and technically evaluates the condition twice.♻️ Proposed refactor
- if (shouldGenerateSkills) { + if (shouldGenerateSkills) { // ... generate skills } - if (!shouldGenerateSkills) { + else { const skillsDir = path.join(projectPath, tool.skillsDir, 'skills'); removedSkillCount += await this.removeSkillDirs(skillsDir); } - if (shouldGenerateCommands) { + if (shouldGenerateCommands) { // ... generate commands } - if (!shouldGenerateCommands) { + else { removedCommandCount += await this.removeCommandFiles(projectPath, tool.value); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/init.ts` around lines 527 - 567, Replace the paired conditional blocks that test the same boolean twice with if/else to make the mutually exclusive logic explicit: for the skills branch, convert the separate if (shouldGenerateSkills) { ... } and if (!shouldGenerateSkills) { removedSkillCount += await this.removeSkillDirs(skillsDir); } into if (shouldGenerateSkills) { /* generate using transformToHyphenCommands, generateSkillContent, FileSystemUtils.writeFile */ } else { removedSkillCount += await this.removeSkillDirs(skillsDir); }; do the same for commands by turning if (shouldGenerateCommands) { /* use CommandAdapterRegistry.get, generateCommands, FileSystemUtils.writeFile, commandsSkipped push */ } else { removedCommandCount += await this.removeCommandFiles(projectPath, tool.value); }. Keep all existing identifiers (shouldGenerateSkills, shouldGenerateCommands, transformToHyphenCommands, generateSkillContent, FileSystemUtils.writeFile, removeSkillDirs, CommandAdapterRegistry.get, generateCommands, removeCommandFiles, commandsSkipped, removedSkillCount, removedCommandCount) and preserve existing behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openspec/changes/simplify-skill-installation/proposal.md`:
- Line 149: The proposal's line "Workflow templates conditionally reference only
installed workflows in 'next steps' guidance" conflicts with the design decision
in design.md section 9 which specifies using generic, concept-based guidance;
update the bullet in proposal.md (the line containing that quoted phrase) to
match design.md section 9 by replacing the conditional/installed-workflows
wording with language that states templates provide generic, concept-based
next-step guidance rather than conditionally referencing installed workflows,
and ensure the phrasing mirrors the terminology used in design.md for
consistency.
---
Duplicate comments:
In `@openspec/changes/simplify-skill-installation/design.md`:
- Around line 151-212: The section ordering in design.md is incorrect: "### 8.
Existing User Migration" and "### 9. Generic Next-Step Guidance" appear before
"### 7. Fix Multi-Select Keybindings"; reorder those headings so numerical
sequence is correct (place the "Fix Multi-Select Keybindings" section before the
"Existing User Migration" and "Generic Next-Step Guidance" sections) and ensure
the following sections (10–12) remain sequential; update only the heading order
and any adjacent cross-references, leaving content unchanged.
In `@src/core/init.ts`:
- Around line 510-514: The code recomputes profile/delivery/workflows in
multiple places causing duplicated disk reads and inconsistency; compute these
once (using getGlobalConfig(), this.resolveProfileOverride(),
getProfileWorkflows()) in execute() and then pass the resolved values into
generateSkillsAndCommands and displaySuccessMessage (or store them on the
instance as fields) so those functions use the same precomputed profile,
delivery, and workflows instead of calling getGlobalConfig() again.
In `@src/core/shared/tool-detection.ts`:
- Around line 24-25: SKILL_NAMES was missing 'openspec-onboard' and
'openspec-propose' and must match COMMAND_IDS; update the SKILL_NAMES array in
tool-detection.ts to include those two entries (same spelling and order as in
COMMAND_IDS) so both arrays have 11 entries and consistent coverage, then run
related unit/integration tests or any code that relies on SKILL_NAMES length
(e.g., validation in detectSupportedTools or usages of SKILL_NAMES) to ensure no
other callsites assume the old set.
In `@src/core/update.ts`:
- Around line 468-487: The silent catch blocks in removeSkillDirs and
removeCommandFiles hide failures and incorrectly increment the removed counter;
update both functions (removeSkillDirs, removeCommandFiles) to catch the error
into a variable, log a warning including the error details and the path (use
processLogger or the existing logger), and only increment the removed counter
after a successful fs.promises.rm or unlink; keep references to ALL_WORKFLOWS
and WORKFLOW_TO_SKILL_DIR when locating skillDir and ensure errors do not cause
the removed tally to be inflated or swallowed silently.
---
Nitpick comments:
In `@src/commands/config.ts`:
- Around line 269-277: The code sets config.workflows = [...CORE_WORKFLOWS] when
preset === 'core' but getProfileWorkflows('core', customWorkflows) ignores
customWorkflows and always returns CORE_WORKFLOWS, producing a stored value
that's never used; either stop writing workflows for the core preset (remove the
config.workflows assignment in the preset === 'core' branch) or change
getProfileWorkflows to consult the passed customWorkflows for the 'core' profile
so the stored array is respected (choose one approach and make the change
consistently with the interactive picker path).
In `@src/core/init.ts`:
- Around line 649-710: In displaySuccessMessage replace the direct casts of
this.profileOverride with a validated call to this.resolveProfileOverride():
specifically change the profile assignment used for the skills/commands counts
(currently setting profile via (this.profileOverride as Profile) at the block
that defines profile) and the activeProfile assignment near the "Getting
started" section (currently using (this.profileOverride as Profile)) to use
this.resolveProfileOverride() so both profile and activeProfile are
resolved/validated the same way as elsewhere in the class.
- Around line 527-567: Replace the paired conditional blocks that test the same
boolean twice with if/else to make the mutually exclusive logic explicit: for
the skills branch, convert the separate if (shouldGenerateSkills) { ... } and if
(!shouldGenerateSkills) { removedSkillCount += await
this.removeSkillDirs(skillsDir); } into if (shouldGenerateSkills) { /* generate
using transformToHyphenCommands, generateSkillContent, FileSystemUtils.writeFile
*/ } else { removedSkillCount += await this.removeSkillDirs(skillsDir); }; do
the same for commands by turning if (shouldGenerateCommands) { /* use
CommandAdapterRegistry.get, generateCommands, FileSystemUtils.writeFile,
commandsSkipped push */ } else { removedCommandCount += await
this.removeCommandFiles(projectPath, tool.value); }. Keep all existing
identifiers (shouldGenerateSkills, shouldGenerateCommands,
transformToHyphenCommands, generateSkillContent, FileSystemUtils.writeFile,
removeSkillDirs, CommandAdapterRegistry.get, generateCommands,
removeCommandFiles, commandsSkipped, removedSkillCount, removedCommandCount) and
preserve existing behavior.
In `@src/core/shared/tool-detection.ts`:
- Around line 96-107: The returned property fullyConfigured is misleading
because it compares skillCount to SKILL_NAMES regardless of profile; update the
return shape to either (a) rename fullyConfigured to allSkillsPresent and keep
the current semantics (so change the property name in the return object and
update any consumers to use allSkillsPresent), or (b) make it profile-aware by
comparing skillCount to the active profile's expected skill list (use the
profile's list instead of SKILL_NAMES) and return fullyConfigured only when all
profile-required skills are present; reference SKILL_NAMES, skillCount and the
return object in tool-detection.ts when making the change.
In `@src/core/update.ts`:
- Around line 265-274: The onboarding message printed when
newlyConfiguredTools.length > 0 should not reference /opsx:new and
/opsx:continue for the core profile; update the message in the block that prints
"Getting started:" (the code using newlyConfiguredTools) to recommend
/opsx:propose (which consolidates new+ff) and keep /opsx:apply and /opsx:archive
as appropriate; if you want to preserve legacy accuracy from upgradeLegacyTools
and getSkillTemplates(), consider making the message conditional on whether
upgradeLegacyTools() path installed non-core workflows, but minimally change the
static lines in the printing block to show /opsx:propose instead of /opsx:new
and /opsx:continue.
In `@test/core/update.test.ts`:
- Around line 1309-1340: Update the test "should create core profile skills when
upgrading legacy tools" by adding the two missing legacy templates to the
asserted skill list: include 'openspec-onboard' and 'openspec-propose' in the
skillNames array so the loop verifies all templates returned by
getSkillTemplates() are present after running new UpdateCommand({ force: true
}).execute(testDir); keep the rest of the test (skillsDir,
FileSystemUtils.fileExists checks) unchanged.
| - `init` validates `--profile` values (`core` or `custom`) and errors on invalid input | ||
| - Migration message mentions `propose` and suggests `openspec config profile core` to opt in | ||
| - After migration, users can opt into `core` profile via `openspec config profile core` | ||
| - Workflow templates conditionally reference only installed workflows in "next steps" guidance |
There was a problem hiding this comment.
Documentation inconsistency with design.md section 9.
Line 149 states templates "conditionally reference only installed workflows" in next-step guidance, but design.md section 9 explicitly documents the opposite decision — generic, concept-based guidance was chosen to avoid conditional cross-referencing complexity. Consider aligning this bullet with the design decision.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openspec/changes/simplify-skill-installation/proposal.md` at line 149, The
proposal's line "Workflow templates conditionally reference only installed
workflows in 'next steps' guidance" conflicts with the design decision in
design.md section 9 which specifies using generic, concept-based guidance;
update the bullet in proposal.md (the line containing that quoted phrase) to
match design.md section 9 by replacing the conditional/installed-workflows
wording with language that states templates provide generic, concept-based
next-step guidance rather than conditionally referencing installed workflows,
and ensure the phrasing mirrors the terminology used in design.md for
consistency.
1 participant
Summary
core(4 workflows: propose, explore, apply, archive) andcustomprofiles, reducing default install from 10 to 4 workflowsproposeworkflow: Combinesnew+ffinto a single command — users go from idea to implementation-ready in one stepopenspec config profilecustomprofile on firstupdateNew Files
src/core/profiles.ts— Profile definitions, CORE_WORKFLOWS/ALL_WORKFLOWS constantssrc/core/available-tools.ts— Auto-detect AI tools from project directoriessrc/core/migration.ts— Shared migration logic (scanInstalledWorkflows, migrateIfNeeded)src/core/templates/workflows/propose.ts— New propose workflow templateModified Files (key changes)
src/core/global-config.ts— Extended with profile/delivery/workflows fieldssrc/commands/config.ts— Addedconfig profilesubcommand with interactive pickersrc/core/init.ts— Smart defaults, tool auto-detection, profile support, --profile flagsrc/core/update.ts— Profile-aware updates, delivery changes, migration, new tool detectionsrc/core/shared/skill-generation.ts— Workflow filtering by profilesrc/prompts/searchable-multi-select.ts— Fixed keybindings (Space/Enter)Test plan
openspec config profileinteractive picker🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements