Feat/dynamic filesystem resolver#13150
Draft
intojhanurag wants to merge 3 commits intomastra-ai:mainfrom
Draft
Conversation
… in Workspace Signed-off-by: intojhanurag <aojharaj2004@gmail.com>
Signed-off-by: intojhanurag <aojharaj2004@gmail.com>
🦋 Changeset detectedLatest commit: c2ecd33 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 |
|
@intojhanurag is attempting to deploy a commit to the Mastra Team on Vercel. A member of the Team first needs to authorize it. |
Contributor
WalkthroughAdds per-request dynamic filesystem resolution to Workspace via a resolver function (receiving requestContext). Workspace gains resolver plumbing and helpers; workspace tools now resolve filesystem at runtime, accept execution context, and enforce read-only constraints for resolver-based filesystems. Tests added for resolver behavior and access control. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 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)
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
🧹 Nitpick comments (2)
packages/core/src/workspace/workspace.ts (1)
514-523: Logger not propagated to dynamically resolved filesystems.Filesystems created by the resolver won't have the Mastra logger set (since
__setLoggeronly propagates tothis._fs). If the resolver createsMastraFilesystemsubclasses, their logging will be silent.Consider calling
__setLoggeron the resolved filesystem withinresolveFilesystem()if a logger has been set on the workspace. This would require storing the logger reference.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/workspace/workspace.ts` around lines 514 - 523, resolveFilesystem currently returns filesystems from _filesystemResolver without propagating the workspace logger, so dynamically resolved MastraFilesystem instances remain un-logged; store the workspace logger when set (e.g., a private field) and, inside resolveFilesystem, after obtaining the result from _filesystemResolver (or when returning this._fs), call result.__setLogger(this._logger) (or similar) if a logger exists and the returned object has __setLogger, ensuring both dynamically resolved and default filesystems receive the workspace logger.packages/core/src/workspace/tools/tools.ts (1)
86-97: Misleading error when resolver is configured butrequestContextis absent.When a workspace uses a resolver-based filesystem, if a tool is invoked without
requestContextin the context,getFs()falls through to theworkspace.filesystemcheck (line 95), which isundefinedfor resolver-based workspaces. The thrownFilesystemNotAvailableErrormessage says "Workspace does not have a filesystem configured", which is incorrect — a resolver is configured, but the context is missing.Consider adding an explicit check:
Proposed fix for a clearer error message
async function getFs(context?: ToolExecutionContext): Promise<WorkspaceFilesystem> { if (context?.requestContext) { const fs = await workspace.resolveFilesystem({ requestContext: context.requestContext }); if (!fs) throw new FilesystemNotAvailableError(); return fs; } + // If a resolver is configured but no requestContext was provided, give a clear error + if (workspace.hasFilesystemConfig() && !workspace.filesystem) { + throw new WorkspaceError( + 'Filesystem resolver requires requestContext, but none was provided', + 'NO_REQUEST_CONTEXT', + ); + } if (!workspace.filesystem) throw new FilesystemNotAvailableError(); return workspace.filesystem; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/workspace/tools/tools.ts` around lines 86 - 97, The current getFs function throws FilesystemNotAvailableError with a misleading message when a resolver-based filesystem is configured but no requestContext is provided; update getFs to explicitly detect if workspace.resolveFilesystem (the resolver) exists and, when context?.requestContext is missing, throw a clearer error (e.g., a new or augmented FilesystemNotAvailableError message) that states the resolver requires a requestContext rather than saying the workspace has no filesystem; adjust error creation or message in getFs (referencing getFs, workspace.resolveFilesystem, workspace.filesystem, and FilesystemNotAvailableError) so callers can distinguish "no filesystem configured" vs "resolver requires requestContext".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.changeset/dynamic-filesystem-resolver.md:
- Line 5: Update the changeset text to show the public API before/after for the
Workspace `filesystem` option (example: previous static `filesystem:
WorkspaceFilesystem` vs new resolver form `filesystem: ({ requestContext }) =>
WorkspaceFilesystem`), remove internal implementation details about tool
injection and runtime enforcement, and instead clearly state the user-facing
outcome: per-request filesystem routing enabled and common use cases
(multi-tenant routing, per-request mounts). Also add a short "Why" sentence
explaining the benefit (flexible routing of filesystem access per request) and
mark this as a minor feature.
In `@packages/core/src/workspace/workspace.ts`:
- Around line 396-401: The current branch treats any function in
config.filesystem as a resolver, but class constructors would match typeof ===
'function' and be misinterpreted; update the conditional around
config.filesystem to detect and reject class constructors (e.g., use
Function.prototype.toString to check for /^class\s/ or check prototype
characteristics) and throw a clear error when a class is passed, otherwise
assign this._filesystemResolver as before; keep the else branch that assigns
this._fs unchanged. Ensure you update the logic that sets
this._filesystemResolver and reference config.filesystem,
this._filesystemResolver, and this._fs when making the change.
---
Nitpick comments:
In `@packages/core/src/workspace/tools/tools.ts`:
- Around line 86-97: The current getFs function throws
FilesystemNotAvailableError with a misleading message when a resolver-based
filesystem is configured but no requestContext is provided; update getFs to
explicitly detect if workspace.resolveFilesystem (the resolver) exists and, when
context?.requestContext is missing, throw a clearer error (e.g., a new or
augmented FilesystemNotAvailableError message) that states the resolver requires
a requestContext rather than saying the workspace has no filesystem; adjust
error creation or message in getFs (referencing getFs,
workspace.resolveFilesystem, workspace.filesystem, and
FilesystemNotAvailableError) so callers can distinguish "no filesystem
configured" vs "resolver requires requestContext".
In `@packages/core/src/workspace/workspace.ts`:
- Around line 514-523: resolveFilesystem currently returns filesystems from
_filesystemResolver without propagating the workspace logger, so dynamically
resolved MastraFilesystem instances remain un-logged; store the workspace logger
when set (e.g., a private field) and, inside resolveFilesystem, after obtaining
the result from _filesystemResolver (or when returning this._fs), call
result.__setLogger(this._logger) (or similar) if a logger exists and the
returned object has __setLogger, ensuring both dynamically resolved and default
filesystems receive the workspace logger.
…and update changeset Signed-off-by: intojhanurag <aojharaj2004@gmail.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/core/src/workspace/workspace.ts (2)
509-531:resolveFilesystemdoes not validate thatrequestContextis defined.If a caller passes
{ requestContext: undefined as any }, the resolver receivesundefinedand will likely throw a confusing error inside the resolver function (e.g.,Cannot read properties of undefined (reading 'get')). Consider a defensive guard, especially since this is a public API surface.This is low risk since TypeScript enforces the type at compile time, but runtime callers (e.g., the tool layer wiring) could pass unexpected values.
🛡️ Optional defensive guard
async resolveFilesystem({ requestContext, }: { requestContext: RequestContext; }): Promise<WorkspaceFilesystem | undefined> { if (this._filesystemResolver) { + if (!requestContext) { + throw new WorkspaceError( + 'requestContext is required when using a filesystem resolver', + 'INVALID_CONFIG', + this.id, + ); + } return await this._filesystemResolver({ requestContext }); } return this._fs; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/workspace/workspace.ts` around lines 509 - 531, resolveFilesystem currently passes whatever is given to this._filesystemResolver and can forward an undefined requestContext; add a defensive guard at the top of resolveFilesystem to validate that the parameter requestContext is defined and of expected shape (RequestContext) and throw a clear, descriptive error (e.g., "resolveFilesystem requires a valid requestContext") if it is not, before calling this._filesystemResolver or returning this._fs; reference the resolveFilesystem method and the _filesystemResolver field when adding this check so runtime callers get a helpful error instead of an internal "cannot read property" crash.
83-92:filesystemgetter returnsundefinedwhen a resolver is used — document this clearly.The
filesystemgetter (Line 486-490) returnsthis._fswhich isundefinedwhen a resolver is configured. This is tested and intentional, but several other methods also silently return empty/partial results in resolver mode:
getPathContext()— no filesystem infogetInfo()— no filesystem sectionrebuildSearchIndex()/ auto-indexing — no-opskillsgetter — falls back toLocalSkillSourceThis is architecturally sound (static operations can't use per-request resolution), but consider adding a note in the
filesystemgetter JSDoc that it returnsundefinedwhen using a resolver, and to useresolveFilesystem()instead.Also applies to: 486-490
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/workspace/workspace.ts` around lines 83 - 92, Update the JSDoc for the filesystem getter to explicitly state that when a WorkspaceFilesystemResolver is configured the getter returns undefined (because _fs is only set for static filesystems) and to instruct callers to use resolveFilesystem(requestContext) for per-request resolution; reference the filesystem getter and resolveFilesystem() by name and mention that methods like getPathContext(), getInfo(), rebuildSearchIndex(), and the skills getter operate without filesystem info in resolver mode so consumers should call resolveFilesystem() at execution time for dynamic behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/core/src/workspace/workspace.ts`:
- Around line 509-531: resolveFilesystem currently passes whatever is given to
this._filesystemResolver and can forward an undefined requestContext; add a
defensive guard at the top of resolveFilesystem to validate that the parameter
requestContext is defined and of expected shape (RequestContext) and throw a
clear, descriptive error (e.g., "resolveFilesystem requires a valid
requestContext") if it is not, before calling this._filesystemResolver or
returning this._fs; reference the resolveFilesystem method and the
_filesystemResolver field when adding this check so runtime callers get a
helpful error instead of an internal "cannot read property" crash.
- Around line 83-92: Update the JSDoc for the filesystem getter to explicitly
state that when a WorkspaceFilesystemResolver is configured the getter returns
undefined (because _fs is only set for static filesystems) and to instruct
callers to use resolveFilesystem(requestContext) for per-request resolution;
reference the filesystem getter and resolveFilesystem() by name and mention that
methods like getPathContext(), getInfo(), rebuildSearchIndex(), and the skills
getter operate without filesystem info in resolver mode so consumers should call
resolveFilesystem() at execution time for dynamic behavior.
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
1 participant
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.
Description
Workspace options like skills, instructions, model, tools, and memory already support dynamic functions that receive
requestContext, but the filesystem only accepted a static instance.This PR adds support for a resolver function
({ requestContext }) => WorkspaceFilesystem, allowing a single Workspace instance to serve different filesystems per request.This enables multi-role agents to scope each role to a different filesystem root or permissions.
Key Changes
WorkspaceFilesystemResolvertype and widened filesystem config to accept ithasFilesystemConfig()andresolveFilesystem()public methods onWorkspacegetFs()readOnlyenforcement for dynamically resolved filesystems on write/edit/delete/mkdir toolsRelated Issue(s)
Fixes #13133
Type of Change
Checklist
Summary by CodeRabbit
New Features
Behavior Changes
Tests