fix(core): handle Windows path separators in workspace skill discovery#17671
fix(core): handle Windows path separators in workspace skill discovery#17671Sandy-1711 wants to merge 5 commits into
Conversation
WorkspaceSkills split paths on '/' only, so absolute Windows paths passed via new Workspace({ skills: [...] }) loaded with the wrong name or failed to load, and skills under node_modules were classified as local instead of external.
Add separator-tolerant path helpers, make getParentPath handle backslashes, and fix the SKILL.md path detection and source classification. Closes mastra-ai#17670.
🦋 Changeset detectedLatest commit: 62839ab The changes in this PR will be included in the next version bump. This PR includes changesets to release 20 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
@Sandy-1711 is attempting to deploy a commit to the Mastra Team on Vercel. A member of the Team first needs to authorize it. |
PR triageLinked issue check passed (#17670). Mastra uses CodeRabbit for automated code reviews. Please address all feedback from CodeRabbit by either making changes to your PR or leaving a comment explaining why you disagree with the feedback. Since CodeRabbit is an AI, it may occasionally provide incorrect feedback. PR complexity score
Applied label: Changed test gateChanged Test Gate is pending. The |
WalkthroughThis PR fixes Windows path separator handling in workspace skill discovery. Workspace skills previously hardcoded ChangesWindows Path Separator Support
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a 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.
🧹 Nitpick comments (2)
packages/core/src/workspace/skills/workspace-skills.test.ts (2)
2980-3036: ⚡ Quick winOptional: Add test using constructor-level skills config for realism.
The PR objectives specifically mention "when passed to new Workspace({ skills: [...] })", but all four tests use
addSkill()instead of the constructor-levelskillsparameter. Adding at least one test that passes a Windows path via the constructor would validate the full discovery flow including glob resolution and would better match the user-facing API described in the PR objectives.💡 Suggested test case
+ it('discovers a skill via constructor with Windows directory path', async () => { + const filesystem = createMockFilesystem({ + 'C:\\Users\\me\\skills\\my-skill/SKILL.md': MY_SKILL_MD, + }); + const skills = new WorkspaceSkillsImpl({ + source: filesystem, + skills: ['C:\\Users\\me\\skills'], + }); + + const result = await skills.get('my-skill'); + expect(result?.name).toBe('my-skill'); + });🤖 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 `@packages/core/src/workspace/skills/workspace-skills.test.ts` around lines 2980 - 3036, Add a test that passes a Windows path via the WorkspaceSkillsImpl constructor-level skills config (e.g., new WorkspaceSkillsImpl({ source: filesystem, skills: ['C:\\Users\\me\\skills\\my-skill'] })) instead of calling addSkill(), so the constructor/glob resolution flow is exercised; create a mock filesystem entry for the Windows SKILL.md (reuse MY_SKILL_MD), instantiate WorkspaceSkillsImpl with the skills array containing a Windows directory or SKILL.md path, then assert discovery (e.g., await skills.get('my-skill') and expect name/source as in the existing tests).
3025-3036: ⚡ Quick winOptional: Add test for managed source (.mastra) with Windows path.
The tests validate external (node_modules) classification but not managed (.mastra/skills) classification with Windows paths. Based on the upstream contract,
#determineSourcenormalizes backslashes to forward slashes before checking for.mastra/skills, so adding a test case would verify that code path.💡 Suggested test case
+ it('classifies a Windows .mastra/skills path as a managed skill', async () => { + const filesystem = createMockFilesystem({ + 'C:\\.mastra\\skills\\my-skill/SKILL.md': MY_SKILL_MD, + }); + const skills = new WorkspaceSkillsImpl({ source: filesystem, skills: [] }); + + await skills.addSkill('C:\\.mastra\\skills\\my-skill'); + + const result = await skills.get('my-skill'); + expect(result?.source.type).toBe('managed'); + });🤖 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 `@packages/core/src/workspace/skills/workspace-skills.test.ts` around lines 3025 - 3036, Add a test to cover managed (.mastra) sources with Windows-style backslashes: create a mock filesystem entry using a Windows path like '.mastra\\skills\\my-skill\\SKILL.md' (or call addSkill with ' .mastra\\skills\\my-skill'), instantiate WorkspaceSkillsImpl, call addSkill(' .mastra\\skills\\my-skill') and assert get('my-skill') returns the skill; this verifies determineSource's backslash-to-slash normalization logic used when classifying managed sources in WorkspaceSkillsImpl (look for determineSource, WorkspaceSkillsImpl.addSkill, and WorkspaceSkillsImpl.get).
🤖 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 `@packages/core/src/workspace/skills/workspace-skills.test.ts`:
- Around line 2980-3036: Add a test that passes a Windows path via the
WorkspaceSkillsImpl constructor-level skills config (e.g., new
WorkspaceSkillsImpl({ source: filesystem, skills:
['C:\\Users\\me\\skills\\my-skill'] })) instead of calling addSkill(), so the
constructor/glob resolution flow is exercised; create a mock filesystem entry
for the Windows SKILL.md (reuse MY_SKILL_MD), instantiate WorkspaceSkillsImpl
with the skills array containing a Windows directory or SKILL.md path, then
assert discovery (e.g., await skills.get('my-skill') and expect name/source as
in the existing tests).
- Around line 3025-3036: Add a test to cover managed (.mastra) sources with
Windows-style backslashes: create a mock filesystem entry using a Windows path
like '.mastra\\skills\\my-skill\\SKILL.md' (or call addSkill with '
.mastra\\skills\\my-skill'), instantiate WorkspaceSkillsImpl, call addSkill('
.mastra\\skills\\my-skill') and assert get('my-skill') returns the skill; this
verifies determineSource's backslash-to-slash normalization logic used when
classifying managed sources in WorkspaceSkillsImpl (look for determineSource,
WorkspaceSkillsImpl.addSkill, and WorkspaceSkillsImpl.get).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7c6c3cf9-01c6-494a-9106-2ad8cc6d9601
📒 Files selected for processing (3)
.changeset/workspace-skills-windows-paths.mdpackages/core/src/workspace/skills/workspace-skills.test.tspackages/core/src/workspace/skills/workspace-skills.ts
wardpeet
left a comment
wardpeet
left a comment
There was a problem hiding this comment.
If run is green, this works! Thank you for the contribution
|
@wardpeet thanks for the review! The required checks (Lint, Lint docs, Merge Test Reports) are stuck on "Expected — waiting for status" because I updated my branch. Could you please approve the workflow runs? |
|
Rebased on latest main — this now includes #17690 which fixed the harness.tools CI flake. Could a maintainer approve the workflow run? |
3 participants
WorkspaceSkills split paths on '/' only, so absolute Windows paths passed via
new Workspace({ skills: [...] })loaded with the wrong name or failed to load, and skills undernode_moduleswere classified as local instead of external.Description
WorkspaceSkillsin@mastra/corehardcoded/as the path separator, so consumer-supplied absolute Windows paths (e.g.C:\Users\me\skills\my-skill) were mishandled:addSkill/#discoverDirectSkillderiveddirNamevia.split('/').pop(), so on Windows it became either the entire path string orunknown. Since the skill name must match its directory name, the skill loaded with the wrong name or failed validation entirely.#getParentPathonly looked for/, so the parent directory of a backslash path resolved incorrectly.endsWith('/SKILL.md')checks never matched a backslash path, so a path pointing directly at aSKILL.mdfile took the wrong branch.#determineSourcesplit on/fornode_modulesdetection, so external skills were misclassified aslocal(and the same applied to.mastramanaged paths).The fix keeps workspace-internal paths POSIX and only adds separator tolerance where consumer-supplied absolute paths enter:
splitPathSegments()(splits on/or\) andisSkillFilePath()(separator-tolerantSKILL.mddetection).#getParentPathuseMath.max(lastIndexOf('/'), lastIndexOf('\\')).#determineSourceto segment-matchnode_moduleson both separators and normalize before the.mastra/skillscheck.Skills passed by absolute path now resolve identically on Windows and POSIX:
Related issue(s)
Fixes #17670
Type of change
Checklist
Summary by CodeRabbit
Bug Fixes
node_modulesto correctly identify them as external rather than local.Tests