fix: use skill references for adapterless tools#1181
Open
leno23 wants to merge 5 commits into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughReplace hardcoded /opsx:* references with tool-aware skill references for adapterless tools. Add a central workflow→skill mapping and instruction-transformer selector, wire it into init/update SKILL.md generation, adjust success/getting-started output, and extend tests for Kimi and Vibe adapterless flows. ChangesAdapterless Tool Skill Reference Transformation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/update.ts`:
- Around line 58-69: The getSkillInstructionTransformer helper is duplicated;
extract the function into a single shared module named command-references,
export it, and have both callers import it instead of defining duplicates;
ensure the extracted implementation uses the same logic (return
transformToHyphenCommands for toolId 'opencode' or 'pi', return a closure that
calls transformToToolSkillReferences(instructions, toolId) when
hasCommandAdapter is false, otherwise return undefined), update imports in the
files that used the duplicate (replace local definitions with import {
getSkillInstructionTransformer } from the command-references module), and remove
the duplicated definitions in both places.
In `@src/utils/command-references.ts`:
- Around line 22-34: Duplicate WORKFLOW_TO_SKILL mapping exists as
WORKFLOW_TO_SKILL_REFERENCE and WORKFLOW_TO_SKILL_DIR; remove the duplication by
exporting WORKFLOW_TO_SKILL_REFERENCE from src/utils/command-references.ts and
updating the other module to import it instead of defining WORKFLOW_TO_SKILL_DIR
locally: export the constant WORKFLOW_TO_SKILL_REFERENCE, then in the module
that defines WORKFLOW_TO_SKILL_DIR replace that local constant with an import
and alias (e.g., import { WORKFLOW_TO_SKILL_REFERENCE as WORKFLOW_TO_SKILL_DIR
}) and delete the duplicate object to keep a single source of truth.
🪄 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: 0fad2a79-bf71-470c-abe4-7895bfcd6165
📒 Files selected for processing (5)
src/core/init.tssrc/core/update.tssrc/utils/command-references.tstest/core/init.test.tstest/utils/command-references.test.ts
Contributor
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)
272-279:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdapterless legacy-upgrade guidance still points to nonexistent
/opsx:*commands.Line 275-Line 277 always print command invocations, even when
newlyConfiguredToolsare adapterless and no commands exist. This reintroduces incorrect getting-started guidance on the update path.💡 Minimal fix sketch
import { getSkillInstructionTransformer, + transformToToolSkillReferences, } from '../utils/command-references.js'; @@ 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'); + const adapterlessOnly = newlyConfiguredTools.every((id) => !CommandAdapterRegistry.has(id)); + if (adapterlessOnly) { + const toolId = newlyConfiguredTools[0]; + console.log(` ${transformToToolSkillReferences('/opsx:new', toolId)} Start a new change`); + console.log(` ${transformToToolSkillReferences('/opsx:continue', toolId)} Create the next artifact`); + console.log(` ${transformToToolSkillReferences('/opsx:apply', toolId)} Implement tasks`); + } else { + console.log(' /opsx:new Start a new change'); + console.log(' /opsx:continue Create the next artifact'); + 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 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 272 - 279, The getting-started block prints /opsx:* commands unconditionally even for adapterless upgrades; modify the logic around newlyConfiguredTools so the guidance only prints when at least one newlyConfiguredTools item exposes commands (e.g., check for a non-adapterless tool or tool.commands?.length > 0). Concretely, wrap the console.log lines (the block that prints 'Getting started' and the /opsx:* examples) in an if that tests newlyConfiguredTools.some(tool => tool.adapterless !== true || (tool.commands && tool.commands.length > 0)), so adapterless-only upgrades won't show nonexistent command hints.
🧹 Nitpick comments (1)
src/core/profile-sync-drift.ts (1)
15-15: ⚡ Quick winAvoid the unchecked
as Record<WorkflowId, string>cast on the shared mapping.Line 15 hides key-coverage drift between
ALL_WORKFLOWSandWORKFLOW_TO_SKILL_REFERENCE; if they diverge, laterpath.join(..., dirName)calls can receiveundefined.💡 Fail-fast guard to keep single source of truth
type WorkflowId = (typeof ALL_WORKFLOWS)[number]; @@ -export const WORKFLOW_TO_SKILL_DIR = WORKFLOW_TO_SKILL_REFERENCE as Record<WorkflowId, string>; +const missingWorkflowMappings = ALL_WORKFLOWS.filter( + (workflow) => WORKFLOW_TO_SKILL_REFERENCE[workflow] == null +); +if (missingWorkflowMappings.length > 0) { + throw new Error( + `Missing workflow-to-skill mapping for: ${missingWorkflowMappings.join(', ')}` + ); +} + +export const WORKFLOW_TO_SKILL_DIR = + WORKFLOW_TO_SKILL_REFERENCE as Record<WorkflowId, 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/profile-sync-drift.ts` at line 15, The unchecked cast on WORKFLOW_TO_SKILL_DIR hides missing keys between WORKFLOW_TO_SKILL_REFERENCE and ALL_WORKFLOWS causing undefined dirs; remove the 'as Record<WorkflowId,string>' cast and instead build/validate the record at runtime: iterate ALL_WORKFLOWS and ensure each key exists on WORKFLOW_TO_SKILL_REFERENCE (use a reduce or map to assemble a new object), throw a clear error if any workflow key is missing, then export the validated Object.freeze(...) as WORKFLOW_TO_SKILL_DIR so downstream path.join calls never receive undefined.
🤖 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 272-279: The getting-started block prints /opsx:* commands
unconditionally even for adapterless upgrades; modify the logic around
newlyConfiguredTools so the guidance only prints when at least one
newlyConfiguredTools item exposes commands (e.g., check for a non-adapterless
tool or tool.commands?.length > 0). Concretely, wrap the console.log lines (the
block that prints 'Getting started' and the /opsx:* examples) in an if that
tests newlyConfiguredTools.some(tool => tool.adapterless !== true ||
(tool.commands && tool.commands.length > 0)), so adapterless-only upgrades won't
show nonexistent command hints.
---
Nitpick comments:
In `@src/core/profile-sync-drift.ts`:
- Line 15: The unchecked cast on WORKFLOW_TO_SKILL_DIR hides missing keys
between WORKFLOW_TO_SKILL_REFERENCE and ALL_WORKFLOWS causing undefined dirs;
remove the 'as Record<WorkflowId,string>' cast and instead build/validate the
record at runtime: iterate ALL_WORKFLOWS and ensure each key exists on
WORKFLOW_TO_SKILL_REFERENCE (use a reduce or map to assemble a new object),
throw a clear error if any workflow key is missing, then export the validated
Object.freeze(...) as WORKFLOW_TO_SKILL_DIR so downstream path.join calls never
receive undefined.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 30342f0f-6c67-4e95-9527-6b18b802d86e
📒 Files selected for processing (4)
src/core/init.tssrc/core/profile-sync-drift.tssrc/core/update.tssrc/utils/command-references.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/core/init.ts
alfred-openspec
requested changes
Jun 8, 2026
alfred-openspec
left a comment
alfred-openspec
left a comment
Collaborator
There was a problem hiding this comment.
Thanks for the follow-up. I rechecked this at c3c17a957af6e9ee298918fdd624e4297f311f8f and it still needs changes before merge.
Blocking issues:
-
The branch does not compile.
src/core/init.tsremoved the localWORKFLOW_TO_SKILL_DIRmap, butremoveSkillDirs()still references it at line 749. A localpnpm install --frozen-lockfile --prefer-offlinetriggered the prepare/build step and failed withTS2304: Cannot find name 'WORKFLOW_TO_SKILL_DIR'. -
The legacy/newly-configured
openspec updateonboarding path still prints/opsx:new,/opsx:continue, and/opsx:applyunconditionally. That means adapterless-only tools like Kimi/Vibe can still be told to use slash commands that were not generated. The init path is improved, but update needs the same command-capable vs skills-only distinction and regression coverage.
The direction looks right, but please fix the compile break and add update-path coverage for adapterless-only tools before this lands.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1155.
What changed
/opsx:*references into skill references for tools that do not have a command adapter./skill:openspec-*invocations while keeping Mistral Vibe on plainopenspec-*skill names.openspec updateskill regeneration.Validation
pnpm exec vitest run test/utils/command-references.test.ts test/core/init.test.ts test/core/update.test.tspnpm run buildNote: local build prints the existing Node engine warning because this machine has Node 20.11.1 and the package asks for >=20.19.0, but the build completed successfully.
Summary by CodeRabbit
New Features
Refactor
Tests