feat: add MiniMax Code skills support - #1214
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:
📝 WalkthroughWalkthroughMiniMax Code is registered as a global skills-only tool. Shared helpers resolve project and home-based skill paths. Detection, initialization, updates, migration, drift checks, tests, CLI wiring, and documentation support the new target. ChangesMiniMax Code global-skills integration
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/init.ts (1)
667-676:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSuccess output reports command location incorrectly.
The summary prints command counts using
tool.skillsPath, which can state commands are in the skills directory (not true for adapter-based tools) and is especially misleading forminimax-codewhere commands are skipped.Proposed adjustment
- if (skillCount > 0 && commandCount > 0) { - console.log(`${skillCount} skills and ${commandCount} commands in ${toolDirs}/`); - } else if (skillCount > 0) { + if (skillCount > 0) { console.log(`${skillCount} skills in ${toolDirs}/`); - } else if (commandCount > 0) { - console.log(`${commandCount} commands in ${toolDirs}/`); + } + + const generatedCommandTools = successfulTools.filter( + (t) => !results.commandsSkipped.includes(t.value) && CommandAdapterRegistry.get(t.value) + ); + if (commandCount > 0 && generatedCommandTools.length > 0) { + console.log( + `${commandCount} commands generated for ${generatedCommandTools + .map((t) => t.name) + .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 667 - 676, The output message uses toolDirs (constructed from successfulTools.map((t) => t.skillsPath)) to report the location of both skills and commands, but commands are not necessarily stored in the skills directory. To fix this, build a separate set of directories specifically for commands from the appropriate command path property (not skillsPath), and use toolDirs only when reporting skills, and commandDirs only when reporting commands. This ensures the output accurately reflects where each artifact type is actually located.
🤖 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 `@test/core/shared/skill-paths.test.ts`:
- Line 23: The test assertions in the resolveToolSkillsDir test are using
hard-coded POSIX path separators (forward slashes like /repo/app/.claude/skills)
which will fail on Windows systems. Replace all hard-coded path strings in the
expect assertions with path.join() to construct the expected paths
programmatically, ensuring the tests work across all operating systems.
Specifically, replace `/repo/app/.claude/skills` with path.join('/repo/app',
'.claude', 'skills') and apply the same pattern to other similar hard-coded
paths in related assertions.
In `@test/core/workspace/skills.test.ts`:
- Around line 59-81: The test hard-codes a POSIX path string in the expect
assertion, making it platform-sensitive and missing Windows path separator
coverage. In the test for getWorkspaceSkillDirectory function with
'minimax-code', replace the hard-coded string '/home/alex/.minimax/skills' in
the expect call with a dynamic path constructed using path.join() from the
component parts (the HOME directory and the relative path segments), so the
assertion automatically uses the correct path separators for the platform being
tested.
---
Outside diff comments:
In `@src/core/init.ts`:
- Around line 667-676: The output message uses toolDirs (constructed from
successfulTools.map((t) => t.skillsPath)) to report the location of both skills
and commands, but commands are not necessarily stored in the skills directory.
To fix this, build a separate set of directories specifically for commands from
the appropriate command path property (not skillsPath), and use toolDirs only
when reporting skills, and commandDirs only when reporting commands. This
ensures the output accurately reflects where each artifact type is actually
located.
🪄 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: 6fb29e29-71d7-4cd0-bb4d-7c51a2d3994d
📒 Files selected for processing (34)
docs/cli.mddocs/commands.mddocs/supported-tools.mdopenspec/changes/add-minimax-code-tool-support/.openspec.yamlopenspec/changes/add-minimax-code-tool-support/design.mdopenspec/changes/add-minimax-code-tool-support/proposal.mdopenspec/changes/add-minimax-code-tool-support/specs/ai-tool-paths/spec.mdopenspec/changes/add-minimax-code-tool-support/specs/cli-init/spec.mdopenspec/changes/add-minimax-code-tool-support/specs/cli-update/spec.mdopenspec/changes/add-minimax-code-tool-support/specs/workspace-links/spec.mdopenspec/changes/add-minimax-code-tool-support/tasks.mdsrc/cli/index.tssrc/core/available-tools.tssrc/core/config.tssrc/core/init.tssrc/core/migration.tssrc/core/profile-sync-drift.tssrc/core/shared/index.tssrc/core/shared/skill-paths.tssrc/core/shared/tool-detection.tssrc/core/update.tssrc/core/workspace/skills.tstest/commands/config-profile.test.tstest/commands/context-store.test.tstest/commands/workspace.test.tstest/core/available-tools.test.tstest/core/context-store/registry.test.tstest/core/init.test.tstest/core/profile-sync-drift.test.tstest/core/shared/skill-paths.test.tstest/core/shared/tool-detection.test.tstest/core/update.test.tstest/core/workspace/skills.test.tstest/helpers/temp-dir.ts
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Thanks for the thorough MiniMax integration. The global skill resolver shape and test coverage are directionally good, but I found one user-facing correctness issue that should be fixed before merge.
src/core/init.ts now builds toolDirs from successfulTools.map((t) => t.skillsPath) and uses that same value for both skills and commands in the setup summary. That is wrong for adapter-backed tools because commands are generated under command-specific directories, not the skills directory, and it is especially misleading for minimax-code because command generation is skipped entirely. With delivery: both, openspec init --tools minimax-code can report something like N skills and M commands in ~/.minimax/skills/ and only later say commands were skipped. Please split the success summary so skill counts use skill paths and command counts only include tools with a command adapter and their actual command output locations, or omit the command location/count when every selected tool skipped commands.
The other broad pieces I checked look sane: resolveToolSkillsDir() consistently targets home-backed ~/.minimax/skills, update/drift/detection paths now use shared skill-capable helpers, and the workspace skill tests cover preserving global MiniMax skills during commands-only delivery. The path-separator nits in tests are worth cleaning up too, but this summary output bug is the blocking bit because it tells users OpenSpec generated artifacts that do not exist.
eb444bd to
4746cee
Compare
|
@alfred-openspec Thanks for the review. I pushed a follow-up update that splits the init summary for skills and commands.
|
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Reviewed the follow-up MiniMax Code changes. I checked the global skills path abstraction, commands-only preservation, no repo-local .minimax/.mavis fallback, and init/update/workspace behavior. Targeted tests pass in a temp worktree: 4 files, 115 tests. Looks good to me.
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)
docs/cli.md (1)
535-535: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd a language to the fenced command block.
Line 535 starts a fenced block without a language. Markdownlint reports MD040. Use
textfor this CLI syntax block.Proposed fix
-``` +```text🤖 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 `@docs/cli.md` at line 535, Update the fenced command block beginning at line 535 in the CLI documentation to specify the text language by changing its opening fence to use text, while preserving the block contents and closing fence.Source: Linters/SAST tools
🤖 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 `@docs/cli.md`:
- Line 535: Update the fenced command block beginning at line 535 in the CLI
documentation to specify the text language by changing its opening fence to use
text, while preserving the block contents and closing fence.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: fdff6fe4-c545-4c05-ab98-4766e8f185b9
📒 Files selected for processing (6)
.changeset/add-minimax-code-skills.mddocs/cli.mddocs/commands.mddocs/supported-tools.mdsrc/cli/index.tssrc/core/available-tools.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/commands.md
- src/core/available-tools.ts
clay-good
left a comment
clay-good
left a comment
There was a problem hiding this comment.
LGTM. The refreshed diff is limited to MiniMax Code's global skills target and the shared path-resolution seam required by init/update/detection. It does not reintroduce workspace or context-store code, does not change command-generation architecture, preserves existing local-tool behavior, and restricts writes/detection to OpenSpec-managed openspec-* skills. Proof: 3,487 local tests passed; GitHub CI passed on Linux, macOS, and Windows; lint/type-check, release tracking, dependency review, security audit, and CodeRabbit all passed.
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Verified the refreshed MiniMax Code integration at this exact head. The global skills path, managed-directory boundaries, commands-only preservation, init/update behavior, focused 265-test run, and hosted matrix all check out.
737d7a7
There was a problem hiding this comment.
🧹 Nitpick comments (3)
test/core/update.test.ts (2)
198-204: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueName the deletion path this test covers.
The config sets
delivery: 'skills'withworkflows: ['propose'], soopenspec-exploreis a deselected workflow. The test therefore coversremoveUnselectedSkillDirs, not the commands-onlyremoveSkillDirspath. Rename the test to state the deselected-workflow path.removeSkillDirsstays unreachable for a global target, becauseexecutegates it on!hasGlobalSkillTarget(tool).♻️ Proposed test name
- it('should not delete MiniMax skills through a linked directory outside the global skills root', async () => { + it('should not delete deselected MiniMax skills through a linked directory outside the global skills root', async () => {🤖 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/update.test.ts` around lines 198 - 204, Rename the test case around the existing deletion scenario to explicitly identify the removeUnselectedSkillDirs path and the deselected openspec-explore workflow. Keep the test setup and assertions unchanged; do not describe it as covering the commands-only removeSkillDirs path.
164-314: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a shared fixture helper for the four symlink-boundary tests.
The four tests cover four distinct code paths, so the coverage is not redundant. The setup repeats verbatim:
mkdtemp, write the fixture skill, create the link with thewin32junction branch, and remove the directory infinally. Extract a helper that takes the link path and the fixture path and returns the outside directory. The helper keeps thewin32junction branch in one place.🤖 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/update.test.ts` around lines 164 - 314, Extract the repeated temporary-directory, fixture-file, symlink creation, and cleanup logic from the four symlink-boundary tests into a shared test helper. Have the helper accept the link path and fixture path, create the outside directory and link using the existing Windows junction branch, and return the outside directory for cleanup. Update each test to use the helper while preserving its distinct assertions and configuration.src/core/update.ts (1)
1022-1053: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the skill-write block shared with
execute.Lines 1023-1024 and 1040-1052 now duplicate the
skillsDir/skillsRootselection and the skill write fromexecute(lines 273-296). The containment root choice is security relevant. If the two sites diverge later, one path could write without the correct root. A shared private helper that resolves both paths and writes one skill file would keep the choice in one place.🤖 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 1022 - 1053, Extract the duplicated skill-file writing logic from the update flow around getSkillTemplates and the corresponding execute flow into a shared private helper. Have the helper centralize skillsDir/skillsRoot selection, assertPathWithin containment, and writing a single SKILL.md, then replace both call sites while preserving their existing tool and content behavior.
🤖 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 1022-1053: Extract the duplicated skill-file writing logic from
the update flow around getSkillTemplates and the corresponding execute flow into
a shared private helper. Have the helper centralize skillsDir/skillsRoot
selection, assertPathWithin containment, and writing a single SKILL.md, then
replace both call sites while preserving their existing tool and content
behavior.
In `@test/core/update.test.ts`:
- Around line 198-204: Rename the test case around the existing deletion
scenario to explicitly identify the removeUnselectedSkillDirs path and the
deselected openspec-explore workflow. Keep the test setup and assertions
unchanged; do not describe it as covering the commands-only removeSkillDirs
path.
- Around line 164-314: Extract the repeated temporary-directory, fixture-file,
symlink creation, and cleanup logic from the four symlink-boundary tests into a
shared test helper. Have the helper accept the link path and fixture path,
create the outside directory and link using the existing Windows junction
branch, and return the outside directory for cleanup. Update each test to use
the helper while preserving its distinct assertions and configuration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d6e851e1-75e0-4b85-9824-5552c82c36e8
📒 Files selected for processing (5)
docs/cli.mdsrc/core/init.tssrc/core/update.tstest/core/init.test.tstest/core/update.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/cli.md
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Re-reviewed the current head. The previously approved MiniMax integration remains intact, and the merge resolution now confines global skill writes and removals to the selected skills root with symlink-escape regressions. A fresh build, 274 focused integration tests, and the full hosted matrix pass.
3 participants
Status
Ready for refreshed review. The branch is forward-ported to current
main, all conflicts are resolved, and GitHub reports it as mergeable.What was missing / the motivation
OpenSpec did not support MiniMax Code's global skills directory, so MiniMax users could not install the OpenSpec workflows through
openspec init.What it does
minimax-codeas a skills-only tool.~/.minimax/skills.openspec-*skill directories..minimaxor.mavisfallback directories.Proof it works
node build.jseslint .tsc --noEmit--tools allend-to-end install.Notes / nits
Summary by CodeRabbit
New Features
Documentation