feat(js): add agents backend interface#5451
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a pluggable backend abstraction (AgentBackend and LocalFilesystemBackend) for agent file operations, refactoring the filesystem and skills middleware to use this new interface instead of direct Node.js fs calls. Feedback on these changes highlights a cross-platform path resolution issue on Windows where absolute paths with drive letters are not correctly stripped, a potential runtime TypeError in read_file.ts if result.content is undefined, and silent error handling in skills.ts when backend.ls() fails.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| const rootDirectory = | ||
| config.rootDirectory.replace(/^[/\\]+/, '') || '.'; | ||
| const resolveBackendPath = (requestedPath: string) => { | ||
| const normalizedPath = requestedPath.replace(/^[/\\]+/, ''); | ||
| const backendPath = path.normalize( | ||
| path.join(rootDirectory, normalizedPath) | ||
| ); | ||
| const relativeToRoot = path.relative(rootDirectory, backendPath); | ||
| if ( | ||
| relativeToRoot === '..' || | ||
| relativeToRoot.startsWith(`..${path.sep}`) || | ||
| path.isAbsolute(relativeToRoot) | ||
| ) { | ||
| throw new Error('Access denied: Path is outside of root directory.'); | ||
| } | ||
| return p; | ||
| } | ||
| return backendPath; | ||
| }; |
There was a problem hiding this comment.
On Windows, absolute paths (e.g., C:\\tmp) are not stripped of their drive letters by replace(/^[/\]+/, ''). This leaves rootDirectory as an absolute path, which subsequently causes LocalFilesystemBackend.resolvePath to throw an 'Access denied' error because the resolved path does not start with the backend's cwd.
Using path.parse to extract and strip the root component provides a robust, cross-platform solution for both Unix and Windows absolute paths.
const parsedRoot = path.parse(config.rootDirectory).root;
const rootDirectory =
(parsedRoot
? config.rootDirectory.substring(parsedRoot.length)
: config.rootDirectory) || '.';
const resolveBackendPath = (requestedPath: string) => {
const parsedRequested = path.parse(requestedPath);
const normalizedPath = parsedRequested.root
? requestedPath.substring(parsedRequested.root.length)
: requestedPath;
const backendPath = path.normalize(
path.join(rootDirectory, normalizedPath)
);
const relativeToRoot = path.relative(rootDirectory, backendPath);
if (
relativeToRoot === '..'
|| relativeToRoot.startsWith('..' + path.sep)
|| path.isAbsolute(relativeToRoot)
) {
throw new Error('Access denied: Path is outside of root directory.');
}
return backendPath;
};References
- In Node.js, use
path.septo split file paths for cross-platform compatibility.
|
|
||
| if (isImage && mimeType) { | ||
| const buffer = await fs.readFile(targetFile); | ||
| const buffer = Buffer.from(result.content as Uint8Array); |
There was a problem hiding this comment.
If result.content is undefined (for example, if the file is empty or there is an issue), calling Buffer.from(result.content as Uint8Array) will throw a runtime TypeError. Using a fallback like ?? '' ensures safe handling.
| const buffer = Buffer.from(result.content as Uint8Array); | |
| const buffer = Buffer.from(result.content ?? ''); |
| const result = await backend.ls(p); | ||
| const files = result.files ?? []; |
There was a problem hiding this comment.
If backend.ls(p) fails, it returns an object containing an error property instead of throwing. Silently ignoring this error will result in skills failing to load without any indication. Throwing an error when result.error is present ensures it is caught by the surrounding try/catch block, preserving the original error-handling behavior.
const result = await backend.ls(p);
if (result.error) {
throw new Error(result.error);
}
const files = result.files ?? [];
This PR introduce a "backend" concept to the agents primitive. How it works:
/backendssub-export defines aAgentBackendinterface, providing some initial abstractions (ls,read,writeetc).LocalFilesystemBackendimplements this, implementation via Nodefs.Sessioninterface addsattachBackendandgetBackendmethods.defineAgentis invoked, the session attaches aLocalFilesystemBackend, with a working directory of the current working directory.filesystemandskillsthen pulls in the current backend via session context.fs, and instead invoke the backend implementations methods.Note; this is currently intended to be a drop-in implementation, with no public api being modified.
Potential breaking change
Currently, a user can provide an agent filesystem access (
filesystem({ rootDirectory: "/tmp" })). In this PR, the rootDirectory becomes relative to the backend:In my opinion, this is better DX - you define the working directory once for your agent, and subsequent paths are resolved from the parent, vs duplicating the same path across the tools.
Follow up
This PR could be followed up with further PRs to expand functionality:
backendfactory:Then middleware such as filesystem and skills all use "process.cwd() + /.sandbox/${ctx.sessionId}" as the base directory, allowing for isolation per agent.