fix(init): only advertise slash commands the profile installs#1410
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughOnboarding guidance is now derived from installed workflows and tool capabilities. Initialization, updates, and the welcome screen avoid advertising unavailable commands, render skill-specific references, and include profile-aware tests and changeset documentation. ChangesOnboarding and skill surfaces
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant InitCommand
participant showWelcomeScreen
participant getOnboardingCommands
InitCommand->>InitCommand: resolve active profile workflows
InitCommand->>showWelcomeScreen: pass active workflows
showWelcomeScreen->>getOnboardingCommands: filter installed workflow hints
getOnboardingCommands-->>showWelcomeScreen: return matching commands
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/core/onboarding-commands.ts (1)
11-15: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExport the
OnboardingCommandtype.Since
OnboardingCommandis used as the return type of the exportedgetOnboardingCommandsfunction, it's best practice to export it. This prevents potentialTS4023compiler errors in strict configurations where declaration files are emitted.💡 Proposed fix
-type OnboardingCommand = { +export type OnboardingCommand = { workflow: WorkflowId; command: string;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/onboarding-commands.ts` around lines 11 - 15, Export the OnboardingCommand type so it is publicly available alongside the exported getOnboardingCommands function and can be referenced in emitted declarations. Keep its existing fields and types unchanged.src/core/init.ts (1)
817-820: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate active workflow resolution.
This block recalculates the active workflows and repeats the profile cast. You can reuse
this.getActiveWorkflows()to streamline the logic.♻️ Proposed refactor
if (successfulTools.length > 0) { - const globalConfig = getGlobalConfig(); - const profile: Profile = (this.profileOverride as Profile) ?? globalConfig.profile ?? 'core'; - const delivery: Delivery = globalConfig.delivery ?? 'both'; - const workflows = getProfileWorkflows(profile, globalConfig.workflows); + const delivery: Delivery = getGlobalConfig().delivery ?? 'both'; + const workflows = this.getActiveWorkflows(); const toolDirs = [...new Set(successfulTools.map((t) => t.skillsDir))].join(', ');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/init.ts` around lines 817 - 820, Replace the local profile and workflow resolution in the initialization block with this.getActiveWorkflows(), reusing its established profile handling and active-workflow calculation. Keep the globalConfig lookup for delivery unchanged and remove only the now-redundant profile and workflows declarations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/init.ts`:
- Around line 255-259: Update getActiveWorkflows to obtain the profile override
through this.resolveProfileOverride() instead of casting this.profileOverride as
Profile, while preserving the existing global profile and 'core' fallbacks so
invalid interactive profile values are validated before workflow resolution and
prompting.
---
Nitpick comments:
In `@src/core/init.ts`:
- Around line 817-820: Replace the local profile and workflow resolution in the
initialization block with this.getActiveWorkflows(), reusing its established
profile handling and active-workflow calculation. Keep the globalConfig lookup
for delivery unchanged and remove only the now-redundant profile and workflows
declarations.
In `@src/core/onboarding-commands.ts`:
- Around line 11-15: Export the OnboardingCommand type so it is publicly
available alongside the exported getOnboardingCommands function and can be
referenced in emitted declarations. Keep its existing fields and types
unchanged.
🪄 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: 3bd2b25e-8304-4195-a83e-0124ab524381
📒 Files selected for processing (8)
.changeset/profile-aware-onboarding-commands.mdsrc/core/init.tssrc/core/onboarding-commands.tssrc/core/update.tssrc/ui/welcome-screen.tstest/core/onboarding-commands.test.tstest/core/update.test.tstest/ui/welcome-screen.test.ts
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
The profile-aware hints and coverage look right, but please have getActiveWorkflows() call resolveProfileOverride() instead of casting the raw override. As written, an invalid interactive --profile renders and waits on the welcome screen before failing, so this should validate before prompting.
|
Thanks — all three CodeRabbit findings are addressed in caf78c7, along with two further issues found by an independent adversarial review pass. Addressed from this review
Found separately, also fixed here
Also added coverage for the Not changed, deliberately: the |
|
@alfred-openspec Addressed in caf78c7 — private getActiveWorkflows(): string[] {
const globalCfg = getGlobalConfig();
const activeProfile: Profile = this.resolveProfileOverride() ?? globalCfg.profile ?? 'core';
return [...getProfileWorkflows(activeProfile, globalCfg.workflows)];
}I also moved the explicit validation call above the welcome-screen block in The push also fixes two things an adversarial review pass turned up, both described in the updated PR body: the quick-start block overflowed the welcome screen's 59-column budget on profiles installing CI is green on linux, macOS, and Windows. Ready for another look. |
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
The profile filtering, width guard, early profile validation, and 140 focused tests look good, but this head overlaps the approved #1404 fix in the same legacy-upgrade menu and currently pins a dead Codex instruction: the new test expects /opsx:apply even though Codex has no slash-command surface. Please merge or rebase onto #1404, combine getOnboardingCommands workflow filtering with its capability-aware reference selection, and assert that the Codex apply-only case names the openspec-apply-change skill rather than /opsx:apply.
|
@alfred-openspec You are right on the substance and I have fixed it in a8f3489 — thank you, this was a real dead instruction. Confirmed the claim before acting: What changed. A new expect(output).toContain('/openspec-apply-change');
expect(output).not.toContain('/opsx:apply');Verified end-to-end against the built CLI, not just unit tests — Two things I did not do, and why — please push back if you disagree.
Also unchanged and worth a follow-up: Full suite 2,043 passing (the 17 |
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
The Codex case is fixed, but the core blocker remains for other skill-only tools. At this exact head, labelOnboardingCommand('/opsx:propose', ['kimi'], 'both') returns /openspec-propose; Kimi's documented invocation is /skill:openspec-propose. #1404 at f5c2de1 is already approved and implements the tool-specific transformer plus mixed-tool handling, so please rebase/merge it and reuse that path instead of landing a parallel generic labeler that is merge-order dependent.
a8f3489 to
6e084b0
Compare
|
Rebased onto #1404 as you asked ( I dropped my Verified against the built CLI, all three surfaces:
Kimi now gets One thing to flag, since it means #1404 alone does not close #1409. #1404 rewrites the same const entries: Array<[string, string]> = [
[referenceFor('/opsx:new'), 'Start a new change'],
[referenceFor('/opsx:continue'), 'Create the next artifact'],
[referenceFor('/opsx:apply'), 'Implement tasks'],
];On the default I had to touch one of your tests — Full suite 2,056 passing (17 Merge order: #1404 first, then this retargets to |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/core/init.test.ts (1)
954-1162: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract repeated
logCallsboilerplate into a helper.The same
(console.log as unknown as { mock: { calls: unknown[][] } }).mock.calls.flat().map(String)cast is repeated ~9 times across this new suite. A small helper (ideally usingvi.mocked(console.log)instead of the unsafeas unknown ascast) would remove the duplication and the type-safety bypass.♻️ Proposed helper
+function getLogCalls(): string[] { + return vi.mocked(console.log).mock.calls.flat().map(String); +}Then replace each occurrence, e.g.:
- const logCalls = (console.log as unknown as { mock: { calls: unknown[][] } }).mock.calls.flat().map(String); + const logCalls = getLogCalls();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/core/init.test.ts` around lines 954 - 1162, Extract the repeated console-log call collection into a shared test helper near the suite setup, preferably using vi.mocked(console.log) to avoid the unsafe cast. Replace every local logCalls declaration in these tests with the helper while preserving the existing flattening and string conversion behavior.src/core/update.ts (1)
334-348: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider extracting the shared "syntax-neutral onboarding reference" resolver. These two sites encode the same three-branch rule (command form → skills-invocable neutral skill → tool-specific skill reference, with a syntax-neutral fallback when tools disagree). Keeping two copies risks the fallback contract drifting between the
updateandmigrationsurfaces.
src/core/update.ts#L334-L348: replace the inlinereferenceForset-building with a call to a sharedresolveOnboardingReference(command, tools, delivery).src/core/migration.ts#L219-L232: deriveproposeReferencefrom the same shared helper (passing/opsx:propose) instead of the duplicated set-building.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/update.ts` around lines 334 - 348, The duplicated onboarding-reference resolution logic must be centralized to preserve one fallback contract. Add a shared resolveOnboardingReference(command, tools, delivery) helper implementing the command-form, skills-invocable neutral form, tool-specific transformer, and disagreement fallback rules; update src/core/update.ts lines 334-348 to call it for referenceFor, and update src/core/migration.ts lines 219-232 to derive proposeReference through the same helper using /opsx:propose.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/core/update.ts`:
- Around line 334-348: The duplicated onboarding-reference resolution logic must
be centralized to preserve one fallback contract. Add a shared
resolveOnboardingReference(command, tools, delivery) helper implementing the
command-form, skills-invocable neutral form, tool-specific transformer, and
disagreement fallback rules; update src/core/update.ts lines 334-348 to call it
for referenceFor, and update src/core/migration.ts lines 219-232 to derive
proposeReference through the same helper using /opsx:propose.
In `@test/core/init.test.ts`:
- Around line 954-1162: Extract the repeated console-log call collection into a
shared test helper near the suite setup, preferably using vi.mocked(console.log)
to avoid the unsafe cast. Replace every local logCalls declaration in these
tests with the helper while preserving the existing flattening and string
conversion behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e988996d-6ca6-4db3-b84b-986131a19a8e
📒 Files selected for processing (23)
.changeset/adapterless-skill-references.md.changeset/profile-aware-onboarding-commands.mdscripts/generate-skillssh.mjsskills/openspec-apply-change/SKILL.mdskills/openspec-explore/SKILL.mdskills/openspec-ff-change/SKILL.mdskills/openspec-onboard/SKILL.mdskills/openspec-propose/SKILL.mdskills/openspec-update-change/SKILL.mdsrc/core/init.tssrc/core/migration.tssrc/core/onboarding-commands.tssrc/core/update.tssrc/ui/welcome-screen.tssrc/utils/command-references.tssrc/utils/index.tstest/core/init.test.tstest/core/migration.test.tstest/core/onboarding-commands.test.tstest/core/templates/skillssh-parity.test.tstest/core/update.test.tstest/ui/welcome-screen.test.tstest/utils/command-references.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- .changeset/profile-aware-onboarding-commands.md
- src/ui/welcome-screen.ts
- test/ui/welcome-screen.test.ts
|
Both CodeRabbit nitpicks on the latest pass land on #1404's code, not this PR's. Confirmed with
They are only visible here because this PR now stacks on No outstanding actionable items on this PR's own diff. CI green on linux, macOS, and Windows; |
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Verified at 6e084b0 on top of approved #1404 f5c2de1: profile filtering now composes with the tool-specific reference path, including Kimi, Codex, mixed-tool, legacy-upgrade, and width-bound cases. Build, clean skill regeneration, 218 focused tests, and exact-head CI pass.
Fixes #1409. The `openspec init` welcome screen and the `openspec update` legacy-upgrade menu hardcoded /opsx:new and /opsx:continue. The default core profile is propose/explore/apply/update/sync/archive, so it never generates them and users were told to run commands that did not exist. getOnboardingCommands() holds the hints in lifecycle order and returns only those whose workflow is installed; both surfaces print its result. In `update` the set is what the newly configured tools actually received, since a legacy upgrade installs an inferred subset for Codex. Stacked on #1404, which decides how each hint is spelled per tool. This commit decides which hints appear; #1404's referenceFor/printStartHints decide the reference form, so Kimi still gets /skill:openspec-*. The welcome screen's quick-start block is width-constrained: it renders beside a 24-column art column and only animates at MIN_WIDTH (60) or wider, and the animation moves the cursor up a fixed count of logical lines. A wrapped line desyncs it, so descriptions are capped at DESCRIPTION_BUDGET and a test asserts no rendered line exceeds 59. Also validates --profile before the welcome screen rather than casting it, so an invalid value fails before the user presses Enter.
6e084b0 to
4e02766
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/update.ts (1)
333-360: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winExclude tools that produced no invocation surface.
An adapterless legacy tool under
delivery: 'commands'is added tonewlyConfiguredToolseven though it receives neither skills nor commands. It then contributes a/skill:reference and workflows here, advertising artifacts that do not exist. Filter the tools used byreferenceForandinstalledWorkflowsto those for whichshouldGenerateSkillsForTool(...) || shouldGenerateCommandsForTool(...)is true, and add an update regression case for this delivery mode.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/update.ts` around lines 333 - 360, Filter newlyConfiguredTools before referenceFor and installedWorkflows to retain only tools where shouldGenerateSkillsForTool(toolId, delivery) or shouldGenerateCommandsForTool(toolId, delivery) is true, preventing adapterless legacy tools from advertising nonexistent artifacts. Add a regression test covering delivery: 'commands' with a tool that produces neither skills nor commands.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/core/update.ts`:
- Around line 333-360: Filter newlyConfiguredTools before referenceFor and
installedWorkflows to retain only tools where
shouldGenerateSkillsForTool(toolId, delivery) or
shouldGenerateCommandsForTool(toolId, delivery) is true, preventing adapterless
legacy tools from advertising nonexistent artifacts. Add a regression test
covering delivery: 'commands' with a tool that produces neither skills nor
commands.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6ef521da-03f2-46b9-bb5e-3263d399481b
📒 Files selected for processing (9)
.changeset/profile-aware-onboarding-commands.mdsrc/core/init.tssrc/core/onboarding-commands.tssrc/core/update.tssrc/ui/welcome-screen.tstest/core/init.test.tstest/core/onboarding-commands.test.tstest/core/update.test.tstest/ui/welcome-screen.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- test/core/onboarding-commands.test.ts
- src/core/onboarding-commands.ts
- src/ui/welcome-screen.ts
- src/core/init.ts
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
The patch is identical to the previously approved stack and now sits cleanly on merged #1404. Exact-head build and 209 focused tests pass; every currently eligible legacy-upgrade tool produces a valid surface under commands-only delivery, so the flagged zero-surface path is not reachable.
2 participants
Status: LGTM — ready for review.
Fixes #1409
What was wrong
Two onboarding surfaces told users to run
/opsx:newand/opsx:continue. Neither is in the defaultcoreprofile (propose/explore/apply/update/sync/archive), so OpenSpec never generates them. Users followed the instructions, the command wasn't there, and it looked like the install had failed.openspec updatebefore this change — not one of the three listed commands matches what was installed:The same three appear on the
initwelcome screen. Note #1404 rewrites this same block to fix the reference form but keeps the three hardcoded workflows, so the bug survives it.How it was fixed
getOnboardingCommands()holds the hints in lifecycle order and returns only those whose workflow is installed. Both surfaces print its result, then hand each one to #1404'sreferenceFor/printStartHintsfor spelling. After, oncore:A custom profile still sees its own:
Two subtleties:
updatefilters by what was installed, not by the profile. A legacy upgrade infers Codex's workflows from its prompt filenames, so that tool can receive a narrower set. The menu lists the union of what the newly configured tools actually got.DESCRIPTION_BUDGETso the widest rendered line stays at 59, the budget the screen has always kept.Also:
--profileis now validated before the welcome screen instead of being cast, so an invalid value fails before the user presses Enter rather than after. (getProfileWorkflowsnever throws on an unknown profile — anything that isn't literally'custom'returnsCORE_WORKFLOWS— so the old cast silently rendered a plausible core screen.)Proof it works
Real output from the built CLI, three tool surfaces:
--tools claude(deliveryboth)/opsx:propose "your idea".claude/commands/opsx/propose.md--tools kimi(no adapter)/skill:openspec-propose "your idea"--tools claude, deliveryskills/openspec-propose "your idea"test/core/onboarding-commands.test.ts— core yieldspropose/apply; a custom profile yieldsnew/continue; order is stable; empty when nothing applies; descriptions stay within the width budget.test/ui/welcome-screen.test.ts— the rendered screen contains the installed commands and not the others, drops the quick-start block entirely when none apply, and no rendered line exceeds 59 columns.test/core/update.test.ts— the legacy-upgrade test asserted/opsx:new, i.e. it pinned the bug; it now asserts/opsx:proposeand the absence ofnew/continue. Added a custom-profile case. fix(init): use skill references for tools without a command adapter #1404's Codex menu test now uses anopsx-new.mdfixture so it still proves the skill-reference form under filtering, and additionally asserts the rest of the profile is not advertised.test/core/init.test.ts— pins thatshowWelcomeScreenreceives the profile's workflows, so the wiring can't silently regress.zsh-installerfailures are the known local Oh My Zsh issue (test(completion): isolate ZshInstaller tests from a real Oh My Zsh install #1400), unaffected in CI.tscclean, lint clean,madge --circular dist/clean.Notes / nits
mainsince May 28. Happy to close this if fix: clarify onboarding across tool syntax, workflows, and delivery #962 is rebased and landed instead./opsx:*. It renders before tool selection, so onlydeliveryis knowable there, and skill names do not fit the layout:/openspec-continue-changeputs the line at 70 columns against the 59 budget. Redesigning that is fix: clarify onboarding across tool syntax, workflows, and delivery #962's territory. Theinitandupdatecompletion hints, which do know their tools, go through fix(init): use skill references for tools without a command adapter #1404.showWelcomeScreen()now takes the active workflows. It is not re-exported from the package entry point, so this is not a public API change.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests