feat: implement secure file system access and path validation #332
Conversation
- Introduced a restricted file system wrapper to ensure all file operations are confined to the script's directory, enhancing security. - Updated various modules to utilize the new secure file system methods, replacing direct fs calls with validated operations. - Enhanced path validation in the server routes and context loaders to prevent unauthorized access to the file system. - Adjusted environment variable handling to use centralized methods for reading and writing API keys, ensuring consistent security practices. This change improves the overall security posture of the application by enforcing strict file access controls and validating paths before any operations are performed.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughCentralizes filesystem and system-path access behind secure platform APIs, replaces direct fs usage across server, UI, and libs, enforces ALLOWED_ROOT_DIRECTORY checks, converts several sync flows (notably terminal session creation) to async, adds sandbox signals and UI dialogs, and pins Node/dependency versions. (≤50 words) Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as UI (Web/Electron)
participant Server as Server API
participant TerminalSvc as TerminalService
participant Platform as secureFs / system-paths
Note over UI,Server: User requests terminal session creation
UI->>Server: POST /api/terminal/sessions { cwd?, shell? }
Server->>TerminalSvc: createSession({ cwd, shell }) (await)
alt cwd provided
TerminalSvc->>Platform: systemPathStat / systemPathExists (validate cwd)
else no cwd
TerminalSvc->>Platform: find default home via system-path helpers
end
TerminalSvc->>Platform: build sanitized env (allowlist)
TerminalSvc->>Platform: detect shell via getShellPaths + systemPathExists
TerminalSvc->>Platform: spawn shell process (validated cwd + cleanEnv)
alt spawned OK
TerminalSvc-->>Server: TerminalSession (created)
Server-->>UI: 201 { session }
else refused (max sessions)
TerminalSvc-->>Server: null
Server-->>UI: 429 { current, max }
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (2)**/*.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**/*.{ts,tsx}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (1)
Comment |
Summary of ChangesHello @webdevcody, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly bolsters the application's security posture by introducing a comprehensive secure file system wrapper and centralizing all file access operations. The changes ensure that file interactions are strictly confined to designated areas, preventing directory traversal vulnerabilities and unauthorized access. This refactoring also improves code maintainability by consolidating path definitions and file system utilities into dedicated modules, making the application more robust and easier to audit for security compliance. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant refactoring of file system (FS) operations across the application, centralizing all FS access through a new @automaker/platform library for enhanced security and maintainability. The changes involve replacing direct fs module imports with secureFs for user-controlled paths and system-paths for system-level tool detection. Specifically, apps/server modules (auth.ts, describe-image.ts, browse.ts, validate-path.ts, setup/common.ts, setup/get-claude-status.ts, setup/routes/delete-api-key.ts, setup/routes/gh-status.ts, terminal-service.ts) were updated to use the new secureFs and system-paths utilities for reading, writing, checking existence, and validating paths, including env file operations. The secure-fs.ts module itself was expanded to expose synchronous FS methods and new .env file manipulation functions, all with integrated path validation. In apps/ui, the Electron main process (main.ts) was updated to initialize and utilize the new platform-specific FS helpers for user data, app bundle access, and system path checks, ensuring that all file dialogs and server startup processes adhere to the new security model. The init.mjs script also received a restricted fs wrapper to limit operations to its own directory. Review comments highlighted a security regression where mode: 0o600 was removed from sensitive file write operations in auth.ts for session and API key files, recommending its re-introduction via secureFs. Another comment pointed out a redundant try/catch block in main.ts during Node.js executable validation, suggesting a simplification to provide better error context. Lastly, a security concern was raised regarding the init.mjs script's path validation, recommending the use of fsNative.realpathSync() to prevent symlink traversal vulnerabilities.
| if (command !== 'node') { | ||
| try { | ||
| if (!systemPathExists(command)) { | ||
| throw new Error( | ||
| `Node.js executable not found at: ${command} (source: ${nodeResult.source})` | ||
| ); | ||
| } | ||
| } catch { | ||
| throw new Error(`Node.js executable not found at: ${command} (source: ${nodeResult.source})`); | ||
| } | ||
| } |
There was a problem hiding this comment.
This try/catch block is redundant. The catch block throws a very similar error to the one in the try block, and it swallows the original error message which can be useful for debugging. This can be simplified to a single try/catch that provides more context on failure.
if (command !== 'node') {
try {
if (!systemPathExists(command)) {
throw new Error(`Node.js executable not found at: ${command} (source: ${nodeResult.source})`);
}
} catch (error) {
const originalError = error instanceof Error ? error.message : String(error);
throw new Error(`Failed to verify Node.js executable at: ${command} (source: ${nodeResult.source}). Reason: ${originalError}`);
}
}![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (9)
apps/server/.env.example (1)
25-27: Comment-default inconsistency.The comment on line 26 suggests using
"*"for development, but the default is nowhttp://localhost:3007. Consider updating the comment to reflect the new recommended approach, or clarify that the specific origin is the preferred development default.Suggested comment update
# CORS origin - which domains can access the API -# Use "*" for development, set specific origin for production +# Default: http://localhost:3007 (web app dev server). Use "*" only if needed for broader access. CORS_ORIGIN=http://localhost:3007libs/platform/src/secure-fs.ts (1)
579-591: Edge case handling could benefit from a clarifying comment.The logic handles empty files and trailing newlines, but the behavior of replacing the last empty line (line 588) rather than appending might surprise maintainers. Consider adding a brief comment.
Suggested clarification
if (!found) { - // Add the key at the end + // Add the key at the end, preserving file structure if (newLines.length > 0 && newLines[newLines.length - 1].trim() !== '') { newLines.push(`${key}=${value}`); } else { - // Replace last empty line or add to empty file + // For empty files or files with trailing blank line, replace rather than append + // to avoid accumulating extra blank lines if (newLines.length === 0 || (newLines.length === 1 && newLines[0] === '')) { newLines[0] = `${key}=${value}`; } else { newLines[newLines.length - 1] = `${key}=${value}`; } } }libs/platform/src/system-paths.ts (1)
655-684: Consider Promise-based alternatives alongside callbacks.The callback-based
electronAppStatandelectronAppReadFilefunctions are fine for compatibility, but modern code would benefit from Promise-based versions. This is optional given the callback pattern works.apps/server/src/lib/auth.ts (1)
77-79: Minor: Inconsistent encoding parameter format.Line 79 uses the string format
'utf-8'while line 116 uses the object format{ encoding: 'utf-8' }. Both work, but consistency improves readability.🔎 Proposed fix for consistency
- await secureFs.writeFile(SESSIONS_FILE, JSON.stringify(sessions), 'utf-8'); + await secureFs.writeFile(SESSIONS_FILE, JSON.stringify(sessions), { encoding: 'utf-8' });Also applies to: 115-116
apps/server/src/routes/fs/routes/validate-path.ts (1)
8-8: Unused import:PathNotAllowedError.
PathNotAllowedErroris imported but not used in this file. The early validation at lines 24-35 directly checksisPathAllowed()rather than catching a thrown error.🔎 Proposed fix to remove unused import
-import { isPathAllowed, PathNotAllowedError, getAllowedRootDirectory } from '@automaker/platform'; +import { isPathAllowed, getAllowedRootDirectory } from '@automaker/platform';apps/ui/src/main.ts (1)
373-396: Redundant try-catch blocks for tsx resolution.The outer try-catch (lines 374-396) catches the same error type as the inner try-catch (lines 388-395), leading to duplicate fallback logic. Consider consolidating.
🔎 Proposed consolidation
- // Check for tsx in app bundle paths - try { - if (electronAppExists(path.join(serverNodeModules, 'dist/cli.mjs'))) { - tsxCliPath = path.join(serverNodeModules, 'dist/cli.mjs'); - } else if (electronAppExists(path.join(rootNodeModules, 'dist/cli.mjs'))) { - tsxCliPath = path.join(rootNodeModules, 'dist/cli.mjs'); - } else { - try { - tsxCliPath = require.resolve('tsx/cli.mjs', { - paths: [path.join(__dirname, '../../server')], - }); - } catch { - throw new Error("Could not find tsx. Please run 'npm install' in the server directory."); - } - } - } catch { - try { - tsxCliPath = require.resolve('tsx/cli.mjs', { - paths: [path.join(__dirname, '../../server')], - }); - } catch { - throw new Error("Could not find tsx. Please run 'npm install' in the server directory."); - } - } + // Check for tsx in app bundle paths + if (electronAppExists(path.join(serverNodeModules, 'dist/cli.mjs'))) { + tsxCliPath = path.join(serverNodeModules, 'dist/cli.mjs'); + } else if (electronAppExists(path.join(rootNodeModules, 'dist/cli.mjs'))) { + tsxCliPath = path.join(rootNodeModules, 'dist/cli.mjs'); + } else { + try { + tsxCliPath = require.resolve('tsx/cli.mjs', { + paths: [path.join(__dirname, '../../server')], + }); + } catch { + throw new Error("Could not find tsx. Please run 'npm install' in the server directory."); + } + }libs/platform/src/node-finder.ts (2)
13-24: Unused import:systemPathReadFileSync.
systemPathReadFileSyncis imported but not used in this file.🔎 Proposed fix
import { systemPathExists, systemPathIsExecutable, systemPathReaddirSync, - systemPathReadFileSync, getNvmPaths, getFnmPaths, getNodeSystemPaths, getScoopNodePath, getChocolateyNodePath, getWslVersionPath, } from './system-paths.js';
113-145: Unused parameter:_homeDirinfindNodeMacOS.The
_homeDirparameter is unused since path discovery now delegates to centralized helpers (getNodeSystemPaths,getNvmPaths,getFnmPaths). Consider removing it if the signature doesn't need to match a common interface.🔎 Proposed fix
-function findNodeMacOS(_homeDir: string): NodeFinderResult | null { +function findNodeMacOS(): NodeFinderResult | null {And update the call site in
findNodeExecutableaccordingly.libs/utils/src/context-loader.ts (1)
41-49: Consider narrowingreadFilereturn type in interface.The
ContextFsModule.readFilesignature returnsPromise<string | Buffer>, but the implementation always calls it with'utf-8'encoding and then casts tostring. Consider narrowing the return type or documenting the expected behavior.🔎 Option: Document encoding expectation
export interface ContextFsModule { access: (path: string) => Promise<void>; readdir: (path: string) => Promise<string[]>; + /** When called with encoding, should return string */ readFile: (path: string, encoding?: BufferEncoding) => Promise<string | Buffer>; }Alternatively, if you control the interface consumers, consider:
- readFile: (path: string, encoding?: BufferEncoding) => Promise<string | Buffer>; + readFile: (path: string, encoding: BufferEncoding) => Promise<string>;
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
apps/server/.env.exampleapps/server/src/lib/auth.tsapps/server/src/lib/secure-fs.tsapps/server/src/routes/context/routes/describe-image.tsapps/server/src/routes/fs/routes/browse.tsapps/server/src/routes/fs/routes/validate-path.tsapps/server/src/routes/setup/common.tsapps/server/src/routes/setup/get-claude-status.tsapps/server/src/routes/setup/routes/delete-api-key.tsapps/server/src/routes/setup/routes/gh-status.tsapps/server/src/routes/terminal/routes/sessions.tsapps/server/src/services/terminal-service.tsapps/ui/src/lib/http-api-client.tsapps/ui/src/main.tsdocs/migration-plan-nextjs-to-vite.mdinit.mjslibs/platform/src/index.tslibs/platform/src/node-finder.tslibs/platform/src/secure-fs.tslibs/platform/src/system-paths.tslibs/utils/src/context-loader.ts
💤 Files with no reviewable changes (1)
- docs/migration-plan-nextjs-to-vite.md
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always import from shared packages (@automaker/*), never from old relative paths
Files:
apps/server/src/routes/terminal/routes/sessions.tsapps/server/src/lib/auth.tsapps/server/src/routes/setup/routes/delete-api-key.tsapps/server/src/routes/fs/routes/browse.tsapps/server/src/routes/context/routes/describe-image.tsapps/ui/src/main.tsapps/server/src/lib/secure-fs.tsapps/server/src/services/terminal-service.tsapps/ui/src/lib/http-api-client.tslibs/platform/src/index.tsapps/server/src/routes/setup/routes/gh-status.tsapps/server/src/routes/fs/routes/validate-path.tslibs/platform/src/node-finder.tslibs/utils/src/context-loader.tsapps/server/src/routes/setup/get-claude-status.tsapps/server/src/routes/setup/common.tslibs/platform/src/secure-fs.tslibs/platform/src/system-paths.ts
apps/server/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
createEventEmitter()fromlib/events.tsfor all server operations to emit events that stream to frontend via WebSocket
Files:
apps/server/src/routes/terminal/routes/sessions.tsapps/server/src/lib/auth.tsapps/server/src/routes/setup/routes/delete-api-key.tsapps/server/src/routes/fs/routes/browse.tsapps/server/src/routes/context/routes/describe-image.tsapps/server/src/lib/secure-fs.tsapps/server/src/services/terminal-service.tsapps/server/src/routes/setup/routes/gh-status.tsapps/server/src/routes/fs/routes/validate-path.tsapps/server/src/routes/setup/get-claude-status.tsapps/server/src/routes/setup/common.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
resolveModelString()from @automaker/model-resolver to convert model aliases (haiku, sonnet, opus) to full model names
Files:
apps/server/src/routes/terminal/routes/sessions.tsapps/server/src/lib/auth.tsapps/server/src/routes/setup/routes/delete-api-key.tsapps/server/src/routes/fs/routes/browse.tsapps/server/src/routes/context/routes/describe-image.tsapps/ui/src/main.tsapps/server/src/lib/secure-fs.tsapps/server/src/services/terminal-service.tsapps/ui/src/lib/http-api-client.tslibs/platform/src/index.tsapps/server/src/routes/setup/routes/gh-status.tsapps/server/src/routes/fs/routes/validate-path.tslibs/platform/src/node-finder.tslibs/utils/src/context-loader.tsapps/server/src/routes/setup/get-claude-status.tsapps/server/src/routes/setup/common.tslibs/platform/src/secure-fs.tslibs/platform/src/system-paths.ts
🧠 Learnings (2)
📚 Learning: 2025-12-28T05:07:48.138Z
Learnt from: CR
Repo: AutoMaker-Org/automaker PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-28T05:07:48.138Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Always import from shared packages (automaker/*), never from old relative paths
Applied to files:
apps/server/src/routes/fs/routes/browse.tsapps/server/src/routes/fs/routes/validate-path.tsapps/server/src/routes/setup/common.ts
📚 Learning: 2025-12-28T05:07:48.138Z
Learnt from: CR
Repo: AutoMaker-Org/automaker PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-28T05:07:48.138Z
Learning: Store project-specific rules in `.automaker/context/` and load them into agent prompts via `loadContextFiles()` from automaker/utils
Applied to files:
libs/utils/src/context-loader.ts
🧬 Code graph analysis (10)
apps/server/src/routes/terminal/routes/sessions.ts (1)
apps/server/src/services/terminal-service.ts (1)
getTerminalService(551-556)
apps/server/src/lib/auth.ts (1)
apps/server/src/lib/secure-fs.ts (1)
secureFs(8-39)
apps/server/src/routes/setup/routes/delete-api-key.ts (1)
apps/server/src/lib/secure-fs.ts (1)
secureFs(8-39)
apps/server/src/services/terminal-service.ts (2)
libs/platform/src/system-paths.ts (3)
systemPathExists(243-248)getWslVersionPath(215-217)systemPathReadFileSync(305-313)apps/server/src/lib/secure-fs.ts (1)
secureFs(8-39)
apps/server/src/routes/setup/routes/gh-status.ts (2)
libs/platform/src/index.ts (3)
getExtendedPath(71-71)getGitHubCliPaths(63-63)systemPathAccess(81-81)libs/platform/src/system-paths.ts (3)
getExtendedPath(222-232)getGitHubCliPaths(31-47)systemPathAccess(255-265)
libs/platform/src/node-finder.ts (2)
libs/platform/src/index.ts (9)
systemPathIsExecutable(82-82)systemPathExists(80-80)systemPathReaddirSync(87-87)NodeFinderResult(55-55)getNodeSystemPaths(75-75)getNvmPaths(73-73)getFnmPaths(74-74)getScoopNodePath(76-76)getChocolateyNodePath(77-77)libs/platform/src/system-paths.ts (8)
systemPathIsExecutable(271-285)systemPathExists(243-248)systemPathReaddirSync(343-348)getNodeSystemPaths(174-192)getNvmPaths(133-142)getFnmPaths(147-169)getScoopNodePath(197-199)getChocolateyNodePath(204-210)
libs/utils/src/context-loader.ts (1)
apps/server/src/lib/secure-fs.ts (1)
secureFs(8-39)
apps/server/src/routes/setup/common.ts (1)
apps/server/src/lib/secure-fs.ts (1)
secureFs(8-39)
libs/platform/src/secure-fs.ts (1)
libs/platform/src/index.ts (1)
validatePath(41-41)
libs/platform/src/system-paths.ts (1)
libs/platform/src/index.ts (15)
getGitHubCliPaths(63-63)getClaudeCliPaths(64-64)getClaudeConfigDir(65-65)getClaudeCredentialPaths(66-66)getClaudeSettingsPath(67-67)getClaudeStatsCachePath(68-68)getClaudeProjectsDir(69-69)getShellPaths(70-70)getNvmPaths(73-73)getFnmPaths(74-74)getNodeSystemPaths(75-75)getScoopNodePath(76-76)getChocolateyNodePath(77-77)getWslVersionPath(78-78)getExtendedPath(71-71)
🪛 ast-grep (0.40.3)
libs/platform/src/secure-fs.ts
[warning] 567-567: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^${escapeRegex(key)}=)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 605-605: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^${escapeRegex(key)}=)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🪛 GitHub Actions: Format Check
init.mjs
[warning] 1-1: Code style issues found in init.mjs. Run 'prettier --write' to fix.
🪛 GitHub Actions: Test Suite
apps/server/src/routes/terminal/routes/sessions.ts
[error] 1-1: Vitest coverage run failed. Command 'vitest run --coverage' exited with code 1.
apps/server/src/lib/auth.ts
[error] 1-1: Vitest coverage run failed. Command 'vitest run --coverage' exited with code 1.
apps/server/.env.example
[error] 1-1: Vitest coverage run failed. Command 'vitest run --coverage' exited with code 1.
apps/server/src/routes/setup/routes/delete-api-key.ts
[error] 1-1: Vitest coverage run failed. Command 'vitest run --coverage' exited with code 1.
apps/server/src/routes/fs/routes/browse.ts
[error] 1-1: Vitest coverage run failed. Command 'vitest run --coverage' exited with code 1.
apps/server/src/routes/context/routes/describe-image.ts
[error] 1-1: Vitest coverage run failed. Command 'vitest run --coverage' exited with code 1.
apps/server/src/lib/secure-fs.ts
[error] 1-1: Vitest coverage run failed. Command 'vitest run --coverage' exited with code 1.
apps/server/src/services/terminal-service.ts
[error] 88-88: TerminalService.detectShell failed due to SystemPaths denial for PowerShell core path
[error] 230-230: TypeError: .toMatch() expects to receive a string, but got undefined
apps/server/src/routes/setup/routes/gh-status.ts
[error] 1-1: Vitest coverage run failed. Command 'vitest run --coverage' exited with code 1.
apps/server/src/routes/fs/routes/validate-path.ts
[error] 1-1: Vitest coverage run failed. Command 'vitest run --coverage' exited with code 1.
apps/server/src/routes/setup/get-claude-status.ts
[error] 1-1: Vitest coverage run failed. Command 'vitest run --coverage' exited with code 1.
apps/server/src/routes/setup/common.ts
[error] 1-1: Vitest coverage run failed. Command 'vitest run --coverage' exited with code 1.
libs/platform/src/system-paths.ts
[error] 245-245: SystemPaths: Access denied: pwsh.exe is not an allowed system path
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: e2e
🔇 Additional comments (40)
init.mjs (2)
38-45: Good path containment implementation.The path validation correctly handles both the exact base directory match and subdirectory containment using
path.septo prevent partial path matches (e.g.,/home/user-evilwouldn't match/home/user).
50-63: LGTM - Minimal fs wrapper with proper validation.The wrapper exposes only the necessary operations (
existsSync,mkdirSync,createWriteStream) and validates all paths throughvalidateScriptPathbefore delegating to the native fs module.apps/server/src/services/terminal-service.ts (2)
168-212: LGTM - Async working directory validation with security boundary enforcement.The conversion to async with
secureFs.statproperly enforces theALLOWED_ROOT_DIRECTORYboundary. The fallback to home directory on access denial or non-existent paths is appropriate behavior.
127-143: LGTM - WSL detection using centralized path helper.The WSL detection correctly uses
getWslVersionPath()andsystemPathReadFileSync()from the platform module, ensuring consistent path handling.apps/server/src/routes/terminal/routes/sessions.ts (1)
24-50: LGTM - Proper async handling and rate limiting response.The handler correctly awaits the now-async
createSessionand returns a 429 status with helpful details when the session limit is reached.libs/platform/src/secure-fs.ts (2)
566-577: Static analysis ReDoS warning is a false positive here.The
escapeRegexfunction (lines 625-627) properly escapes all regex metacharacters before constructing the pattern. Since the escaped key produces a literal match pattern (^ESCAPED_KEY=), there's no backtracking risk. The resulting regex has O(n) matching complexity.
322-410: LGTM - Consistent synchronous wrappers with path validation.The synchronous methods follow a consistent pattern of validating the path before delegating to the native fs module. The lack of retry logic (compared to async methods) is acceptable for sync operations typically used in initialization paths.
libs/platform/src/system-paths.ts (2)
418-442: LGTM - Path validation with proper containment checking.The
isAllowedSystemPathfunction correctly normalizes paths and checks both exact matches and directory containment using thepath.sepsuffix pattern to prevent partial path matching.
730-787: LGTM - Robust auth indicator detection with proper error handling.The function aggregates authentication indicators from multiple sources and appropriately swallows errors since this is detection logic where partial results are acceptable.
apps/server/src/routes/setup/routes/delete-api-key.ts (2)
19-29: LGTM - Clean migration to centralized secureFs.removeEnvKey.The implementation correctly delegates to the platform's secure filesystem module, simplifying the code while maintaining proper error propagation. Follows the coding guidelines for importing from
@automaker/*shared packages.
31-82: LGTM - Proper API key deletion flow with comprehensive cleanup.The handler correctly validates input, clears the key from in-memory storage, process environment, and the .env file in sequence. Error handling properly catches and formats errors for the response.
apps/server/src/routes/setup/common.ts (2)
7-7: LGTM!The import correctly uses the shared package
@automaker/platformas per the coding guidelines.
40-50: Good simplification using centralized env key handling.The refactor to use
secureFs.writeEnvKeycentralizes the .env file management logic, removing manual file parsing. The error handling improvement to rethrow after logging ensures callers are aware of failures.apps/server/src/routes/setup/routes/gh-status.ts (2)
8-8: LGTM!Correctly imports centralized path utilities from
@automaker/platformfollowing the coding guidelines.
45-58: Centralized GitHub CLI discovery looks good.The refactor to use
getGitHubCliPaths()andsystemPathAccess()properly centralizes OS-specific path handling. Note thatsystemPathAccess()may throw anErrorfor paths outside the allowed system paths list, which is correctly caught by the outer try/catch. The silent catch is appropriate here since we're iterating through fallback locations.apps/ui/src/lib/http-api-client.ts (1)
566-578: LGTM!The updated path validation handling correctly:
- Extends the response type to include
isAllowed- Uses
result.isAllowed !== falsewhich correctly handles bothundefined(backward compatibility) and explicitfalse- Provides a fallback error message when
result.erroris absentapps/server/src/routes/fs/routes/browse.ts (2)
9-9: LGTM!Import correctly uses
@automaker/platformper coding guidelines.
43-52: Good security improvement for parent path exposure.The logic correctly prevents leaking parent directories outside the allowed root:
hasParentchecks both that we're not at filesystem root AND the parent is within allowed boundssafeParentPathis null when navigation would escape the allowed rootThis effectively prevents directory traversal attacks in the file browser UI.
apps/server/src/lib/auth.ts (1)
14-14: LGTM!The import from
./secure-fs.jsis acceptable as it's a local re-export module that provides backward compatibility while delegating to@automaker/platform.apps/server/src/lib/secure-fs.ts (1)
6-39: LGTM!Clean re-export module that:
- Correctly imports from
@automaker/platformper coding guidelines- Provides backward compatibility for existing imports
- Properly exposes the new synchronous methods needed for auth.ts changes
apps/server/src/routes/fs/routes/validate-path.ts (2)
23-35: Good early path validation before filesystem access.The validation correctly:
- Checks path allowance before any filesystem operations (defense in depth)
- Provides a helpful error message including the allowed root directory when configured
- Returns
isAllowed: falseto match the client-side contract
46-50: LGTM!The success response correctly includes
isAllowed: trueto match the updated client-side contract inhttp-api-client.ts.apps/server/src/routes/context/routes/describe-image.ts (4)
18-18: LGTM: Secure filesystem import.The switch from direct
fstosecureFswrapper properly centralizes path validation for all file operations in this endpoint.
58-99: LGTM: Unicode path normalization with secure filesystem.The
findActualFilePathfunction correctly usessecureFsfor all file system operations (existence checks, directory reads) while maintaining the Unicode space normalization logic for macOS screenshot filenames.
284-294: LGTM: Stat call uses secure filesystem.The type annotation
ReturnType<typeof secureFs.statSync>properly reflects the new secure filesystem abstraction.
296-307:readImageAsBase64correctly uses secure filesystem with path validation.The utility properly enforces centralized path validation through
secureFs.readFile(), which callsvalidatePath()before any filesystem access. The security boundary is maintained.apps/server/src/routes/setup/get-claude-status.ts (3)
7-10: LGTM: Centralized platform imports.Good adoption of centralized platform utilities for CLI path discovery and authentication indicators. As per coding guidelines, importing from shared packages (
@automaker/platform) is preferred over relative paths.
38-60: LGTM: CLI discovery with centralized paths.The refactored CLI discovery loop properly uses
getClaudeCliPaths()andsystemPathAccess()for cross-platform path validation. The earlybreakon finding a valid path is efficient.
81-112: LGTM: Indicator-based authentication detection.The switch to
getClaudeAuthIndicators()centralizes authentication state detection and eliminates scattered file reads. The priority order (activity → settings/sessions → credentials → env → in-memory) is logical.apps/ui/src/main.ts (5)
15-35: LGTM: Comprehensive platform imports for Electron.The imports properly separate concerns between userData operations, app bundle operations, and system path operations. This organization supports the centralized security model.
97-120: LGTM: API key handling with userData helpers.The
ensureApiKeyfunction correctly useselectronUserDataExists,electronUserDataReadFileSync, andelectronUserDataWriteFileSyncfor CSRF protection key management. File permissions (mode: 0o600) are preserved for security.
605-625: LGTM: Path security initialization order.The path helpers are correctly initialized before any file operations:
setElectronUserDataPathsets userData basesetElectronAppPathssets app bundle paths (dev vs prod)initAllowedPathsinitializes security boundariesThis ensures all subsequent file operations are properly validated.
717-733: LGTM: IPC directory validation enforces security boundary.The directory selection dialog now validates the selected path against
ALLOWED_ROOT_DIRECTORYusingisPathAllowed(). The error dialog provides clear feedback to users when a disallowed path is selected.
269-334: Path traversal attacks are properly mitigated. TheelectronAppStat,electronAppReadFile, andelectronAppExistsfunctions all validate paths throughisElectronAppPath, which usespath.resolve()to normalize paths and verify they remain within allowed app directories. Paths like/../../../etc/passwdare safely rejected.Likely an incorrect or invalid review comment.
libs/platform/src/index.ts (1)
58-117: LGTM: Comprehensive system-paths API surface.The exports properly expose:
- CLI tool path discovery (
getClaudeCliPaths,findClaudeCliPath)- System path operations with validation (
systemPathExists,systemPathAccess, etc.)- Electron-specific helpers for userData and app bundle operations
- Script directory utilities for bounded file operations
The dual export pattern (namespace
systemPaths+ named exports) provides flexibility for consumers.libs/platform/src/node-finder.ts (3)
63-69: LGTM: Executable check with centralized validation.The
isExecutablefunction properly delegates tosystemPathIsExecutable, which handles platform-specific executable checks (X_OK on Unix, F_OK on Windows) with path validation.
76-108: LGTM: Version manager discovery with secure path operations.The
findNodeFromVersionManagerfunction correctly:
- Uses
systemPathExistsfor base path validation- Uses
systemPathReaddirSyncfor directory enumeration- Maintains semantic version sorting with pre-release deprioritization
- Returns early on first executable match
180-223: LGTM: Windows Node discovery with package manager support.Good coverage of Windows Node.js installation sources: Program Files, NVM for Windows, fnm, Scoop, and Chocolatey. The centralized helpers ensure consistent path validation across all discovery methods.
libs/utils/src/context-loader.ts (2)
13-13: LGTM: Default to secure filesystem.Switching to
secureFsfrom@automaker/platformas the default ensures all context file loading respects the centralized path validation. Based on learnings, this aligns with loading project-specific rules from.automaker/context/.
154-207: LGTM: Context loading with DI support.The
loadContextFilesfunction correctly:
- Defaults to
secureFsfor production use- Accepts custom
fsModulefor testing/DI- Threads the module through to
loadContextMetadata- Maintains existing error handling semantics
- Introduced a .nvmrc file to specify the Node.js version (22) for the project, ensuring consistent development environments. - Enhanced error handling in the startServer function to provide clearer messages when the Node.js executable cannot be found, improving debugging experience. - Updated package.json files across various modules to enforce Node.js version compatibility and ensure consistent dependency versions. These changes aim to streamline development processes and enhance the application's reliability by enforcing version control and improving error reporting.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (3)
libs/platform/src/system-paths.ts (1)
113-127: Shell paths partially address previous feedback; short-form names still missing.The addition of
pwsh.exe(line 119), PowerShell 7-preview (line 122), and/usr/bin/*paths (line 126) addresses most of the previous review feedback. However, short-form shell names without paths (e.g.,"bash","zsh","sh","powershell") are still missing, which means shells referenced via$SHELLenvironment variable using short names may not match indetectShell.Complete the fix by adding short-form names
export function getShellPaths(): string[] { if (process.platform === 'win32') { return [ process.env.COMSPEC || 'cmd.exe', 'cmd.exe', 'powershell.exe', + 'powershell', 'pwsh.exe', // PowerShell Core short form + 'pwsh', 'C:\\Windows\\System32\\WindowsPowerShell\\v1.0\\powershell.exe', 'C:\\Program Files\\PowerShell\\7\\pwsh.exe', 'C:\\Program Files\\PowerShell\\7-preview\\pwsh.exe', // Preview versions ]; } - return ['/bin/zsh', '/bin/bash', '/bin/sh', '/usr/bin/zsh', '/usr/bin/bash']; + return [ + '/bin/zsh', + '/bin/bash', + '/bin/sh', + '/usr/bin/zsh', + '/usr/bin/bash', + 'zsh', // Short forms for $SHELL matching + 'bash', + 'sh', + ]; }Based on previous review feedback.
apps/server/src/services/terminal-service.ts (1)
83-94: Verify PowerShell paths are in the system-paths allowlist.The hardcoded paths for PowerShell detection (
C:\Windows\System32\WindowsPowerShell\v1.0\powershell.exeandC:\Program Files\PowerShell\7\pwsh.exe) must be included ingetAllAllowedSystemPaths()fromsystem-paths.ts. If not,systemPathExistswill throw "Access denied" at runtime.#!/bin/bash # Description: Check if PowerShell paths are included in getAllAllowedSystemPaths # Search for getShellPaths or shell path definitions in system-paths.ts rg -n "PowerShell|pwsh|powershell" --type ts libs/platform/src/apps/ui/src/main.ts (1)
351-364: Redundant try/catch structure can be simplified.The outer try/catch on line 352-363 catches the error from
systemPathExistsand re-throws a similar error, swallowing the original message. This can be simplified.Suggested simplification
if (command !== 'node') { - try { - if (!systemPathExists(command)) { - throw new Error( - `Node.js executable not found at: ${command} (source: ${nodeResult.source})` - ); - } - } catch (error) { - const originalError = error instanceof Error ? error.message : String(error); - throw new Error( - `Failed to verify Node.js executable at: ${command} (source: ${nodeResult.source}). Reason: ${originalError}` - ); - } + try { + if (!systemPathExists(command)) { + throw new Error( + `Node.js executable not found at: ${command} (source: ${nodeResult.source})` + ); + } + } catch (error) { + if (error instanceof Error && error.message.includes('not found')) { + throw error; // Re-throw our own error as-is + } + // Wrap unexpected errors (e.g., permission issues from systemPathExists) + const reason = error instanceof Error ? error.message : String(error); + throw new Error( + `Failed to verify Node.js executable at: ${command} (source: ${nodeResult.source}). Reason: ${reason}` + ); + } }
🧹 Nitpick comments (2)
apps/server/src/providers/claude-provider.ts (1)
23-34: Consider centralizing environment sanitization logic.The
buildCleanEnv()function provides valuable security functionality that may be needed in other parts of the codebase. Consider moving this to a shared utility package like@automaker/platformor@automaker/utilsfor reuse.Suggested centralization approach
Create a new utility in
@automaker/platform:// libs/platform/src/env-utils.ts export function buildCleanEnvironment( blocklist: string[] = ['PORT', 'DATA_DIR', 'NODE_PATH'], prefixBlocklist: string[] = ['AUTOMAKER_', 'ANTHROPIC_'] ): Record<string, string | undefined> { const cleanEnv: Record<string, string | undefined> = {}; for (const [key, value] of Object.entries(process.env)) { const isBlocked = blocklist.includes(key) || prefixBlocklist.some(prefix => key.startsWith(prefix)); if (!isBlocked) { cleanEnv[key] = value; } } return cleanEnv; }Then import and use:
import { buildCleanEnvironment } from '@automaker/platform'; // In sdkOptions: env: buildCleanEnvironment(),apps/ui/src/main.ts (1)
280-288: Consider reducing redundant existence checks.The
electronAppExistsis called multiple times on different paths within the same request handler. This could be simplified with a single helper that returns the first existing path.Optional refactor for cleaner logic
- try { - if (!electronAppExists(filePath) && !electronAppExists(possibleFile)) { - filePath = path.join(staticPath, 'index.html'); - } else if (electronAppExists(possibleFile)) { - filePath = possibleFile; - } - } catch { - filePath = path.join(staticPath, 'index.html'); - } + try { + // Try original path, then with .html extension, fallback to index.html + if (electronAppExists(possibleFile)) { + filePath = possibleFile; + } else if (!electronAppExists(filePath)) { + filePath = path.join(staticPath, 'index.html'); + } + } catch { + filePath = path.join(staticPath, 'index.html'); + }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (20)
.nvmrcapps/server/package.jsonapps/server/src/lib/auth.tsapps/server/src/providers/claude-provider.tsapps/server/src/services/terminal-service.tsapps/server/tests/unit/services/terminal-service.test.tsapps/ui/package.jsonapps/ui/src/main.tsinit.mjslibs/dependency-resolver/package.jsonlibs/git-utils/package.jsonlibs/model-resolver/package.jsonlibs/platform/package.jsonlibs/platform/src/secure-fs.tslibs/platform/src/system-paths.tslibs/prompts/package.jsonlibs/prompts/src/defaults.tslibs/types/package.jsonlibs/utils/package.jsonpackage.json
✅ Files skipped from review due to trivial changes (1)
- .nvmrc
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/server/src/lib/auth.ts
- init.mjs
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always import from shared packages (@automaker/*), never from old relative paths
Files:
apps/server/src/services/terminal-service.tslibs/prompts/src/defaults.tsapps/server/src/providers/claude-provider.tsapps/ui/src/main.tsapps/server/tests/unit/services/terminal-service.test.tslibs/platform/src/system-paths.tslibs/platform/src/secure-fs.ts
apps/server/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
createEventEmitter()fromlib/events.tsfor all server operations to emit events that stream to frontend via WebSocket
Files:
apps/server/src/services/terminal-service.tsapps/server/src/providers/claude-provider.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
resolveModelString()from @automaker/model-resolver to convert model aliases (haiku, sonnet, opus) to full model names
Files:
apps/server/src/services/terminal-service.tslibs/prompts/src/defaults.tsapps/server/src/providers/claude-provider.tsapps/ui/src/main.tsapps/server/tests/unit/services/terminal-service.test.tslibs/platform/src/system-paths.tslibs/platform/src/secure-fs.ts
🧠 Learnings (2)
📚 Learning: 2025-12-28T05:07:48.138Z
Learnt from: CR
Repo: AutoMaker-Org/automaker PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-28T05:07:48.138Z
Learning: Packages can only depend on packages above them in the dependency chain: types → (utils|prompts|platform|model-resolver|dependency-resolver) → git-utils → (server|ui)
Applied to files:
apps/server/package.jsonlibs/git-utils/package.json
📚 Learning: 2025-12-28T05:07:48.138Z
Learnt from: CR
Repo: AutoMaker-Org/automaker PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-28T05:07:48.138Z
Learning: Applies to **/*.{ts,tsx} : Use `resolveModelString()` from automaker/model-resolver to convert model aliases (haiku, sonnet, opus) to full model names
Applied to files:
libs/model-resolver/package.json
🧬 Code graph analysis (3)
apps/server/src/services/terminal-service.ts (2)
libs/platform/src/system-paths.ts (3)
systemPathExists(246-251)getWslVersionPath(218-220)systemPathReadFileSync(308-316)apps/server/src/lib/secure-fs.ts (1)
secureFs(8-39)
apps/server/tests/unit/services/terminal-service.test.ts (1)
apps/server/src/lib/secure-fs.ts (1)
secureFs(8-39)
libs/platform/src/system-paths.ts (2)
libs/platform/src/index.ts (15)
getGitHubCliPaths(63-63)getClaudeCliPaths(64-64)getClaudeConfigDir(65-65)getClaudeCredentialPaths(66-66)getClaudeSettingsPath(67-67)getClaudeStatsCachePath(68-68)getClaudeProjectsDir(69-69)getShellPaths(70-70)getNvmPaths(73-73)getFnmPaths(74-74)getNodeSystemPaths(75-75)getScoopNodePath(76-76)getChocolateyNodePath(77-77)getWslVersionPath(78-78)getExtendedPath(71-71)init.mjs (1)
fs(52-65)
🪛 ast-grep (0.40.3)
libs/platform/src/secure-fs.ts
[warning] 576-576: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^${escapeRegex(key)}=)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 614-614: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^${escapeRegex(key)}=)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🔇 Additional comments (32)
libs/prompts/src/defaults.ts (1)
305-307: Port protection is consistently applied across templates—coverage complete through feature prompt substitution.The port protection warning in lines 305-307 is consistent with the feature prompt template (lines 211-213). The other auto-mode prompt templates (follow-up, continuation, and pipeline step) inherit this warning indirectly through the
{{featurePrompt}}template variable, which substitutes the feature prompt that includes the port protection message.However, the hardcoding of ports 3007 and 3008 across both templates remains a concern. These ports should be extracted to a configuration constant or environment variable to allow flexibility across deployments.
apps/server/src/providers/claude-provider.ts (1)
78-80: LGTM with dependencies on environment filtering improvements.The integration of environment sanitization into the SDK options is well-documented and correctly implemented. However, effectiveness depends on addressing the API key leakage concern flagged in the earlier comment.
package.json (2)
5-7: Duplicate verification needed: Node 22.x constraint.Same Node engine constraint as other packages in this PR. See the earlier comment on libs/types/package.json for verification requirements.
59-66: Pinned dependency versions are verified secure; continue monitoring as a best practice.All dependencies and devDependencies are pinned to exact versions, which improves build reproducibility but prevents automatic security updates. Verification shows these specific versions (cross-spawn@7.0.6, rehype-sanitize@6.0.0, tree-kill@1.2.2, husky@9.1.7, lint-staged@16.2.7, prettier@3.7.4) have no known CVEs. Maintain a process to periodically check for and apply security updates when available.
apps/server/package.json (1)
8-10: Duplicate verification needed: Node 22.x constraint.Same Node engine constraint as other packages. Refer to earlier verification requirements.
libs/model-resolver/package.json (2)
21-23: Duplicate verification needed: Node 22.x constraint.Consistent with other packages in this PR. Refer to earlier verification requirements.
25-30: Dependency pinning is consistent across packages.Dependencies and devDependencies have been pinned to exact versions, matching the pattern established across the repository. This maintains consistency and deterministic builds.
libs/types/package.json (1)
18-20: No action needed. Node 22 is a stable LTS release (moved to Maintenance LTS on Oct 28, 2025) with security support through April 30, 2027, making it appropriate for production use. The constraint is also consistently applied across all packages in the repository.libs/git-utils/package.json (1)
21-31: LGTM! Engine constraints and exact version pinning improve build reproducibility.The addition of Node.js version constraints and removal of caret ranges from dependencies ensures consistent builds across environments and prevents unexpected breaking changes from transitive dependency updates.
libs/prompts/package.json (1)
21-30: LGTM! Consistent version management across packages.The engine constraints and exact version pinning align with the monorepo-wide policy for reproducible builds.
libs/dependency-resolver/package.json (1)
28-37: LGTM! Version management follows established pattern.Consistent with the other package updates in this PR.
libs/platform/src/secure-fs.ts (2)
318-419: LGTM! Synchronous wrappers maintain consistent security posture.The sync operations properly validate paths before delegating to the native fs module, maintaining the same defense-in-depth approach as the async operations. The overload signatures for
readdirSynccorrectly preserve type information forwithFileTypes.
14-14: LGTM! Type definitions align with native fs module.The
WriteFileOptionsandWriteFileSyncOptionsinterfaces correctly match the native fs module signatures, and the export ofDirentandStatstypes enables proper type checking for consumers of the secure-fs module.Also applies to: 168-175, 325-329
libs/platform/src/system-paths.ts (3)
377-445: LGTM! Path validation logic is secure and well-structured.The
isAllowedSystemPathfunction properly:
- Normalizes paths to prevent traversal attacks
- Checks exact matches first for performance
- Validates subdirectory relationships using
path.septo prevent false positives (e.g.,/home/userwon't match/home/username)
241-371: LGTM! System path operations enforce consistent security boundaries.All system path operations properly validate against the allowlist before accessing files, and error messages clearly communicate security restrictions. The
systemPathIsExecutablefunction correctly handles platform differences between Windows (F_OK) and POSIX (X_OK).
447-690: LGTM! Electron and script directory operations maintain proper isolation.The module-level state management for Electron userData, script directories, and app bundles is appropriate for this use case. All operations:
- Validate initialization before use with clear error messages
- Properly construct paths using
path.jointo prevent traversal- Use callback-based APIs for Electron compatibility where needed
The support for multiple app directories (line 590) is a good design for handling both development (source) and production (packaged) modes.
apps/server/tests/unit/services/terminal-service.test.ts (4)
5-20: Well-structured mock setup for platform utilities.The mock configuration for
@automaker/platformproperly preserves actual implementations while overriding the specific security-related functions (systemPathExists,systemPathReadFileSync,getWslVersionPath,isAllowedSystemPath). SettingisAllowedSystemPathto always returntrueis appropriate for testing since it allows focusing on the terminal service logic without security restrictions.
43-48: Default mocks ensure consistent test behavior.Good practice to set up default return values for all mocked dependencies in
beforeEach. This ensures each test starts from a known state and makes individual test overrides clearer.
240-264: Tests properly updated for async createSession.The conversion to
async/awaitpattern is correctly applied. The null checks with non-null assertions (session!) are appropriate here since tests explicitly verifyexpect(session).not.toBeNull()before accessing properties.
283-294: Good coverage for directory validation fallback.This test verifies that when
secureFs.statrejects (simulating a non-existent or disallowed path), the service falls back to the home directory. This is important for the security boundary enforcement.apps/server/src/services/terminal-service.ts (4)
12-17: Good separation between secure FS and system paths.The comment clearly explains the purpose of each import:
secureFsfor user-controllable paths (enforcingALLOWED_ROOT_DIRECTORY), and system paths utilities for shell binaries outside the allowed directory. This distinction is important for security auditing.
168-212: Async path resolution with proper security enforcement.The conversion to async with
secureFs.statenables proper enforcement ofALLOWED_ROOT_DIRECTORY. The null byte check (line 180) prevents path traversal attacks. Good fallback to home directory on validation failure.
260-278: Environment sanitization prevents variable pollution.Filtering out Automaker-specific variables (
PORT,DATA_DIR,AUTOMAKER_API_KEY,NODE_PATH) from spawned shells is a good practice. This prevents conflicts when users run development servers or other tools that might use these variables.
242-256: Session creation properly awaits async path resolution.The
createSessionmethod correctly awaitsresolveWorkingDirectoryand handles the async flow. The comment on line 255-256 documents the security boundary enforcement.libs/utils/package.json (1)
20-30: Exact version pinning improves build reproducibility.Pinning dependencies to exact versions (no
^or~) ensures consistent builds across environments. The Node 22.x engine constraint aligns with the.nvmrcfile mentioned in the PR. This approach reduces the risk of unexpected breaking changes from transitive dependency updates.libs/platform/package.json (1)
20-30: Consistent version pinning across platform package.The same exact version pinning strategy is applied here, maintaining consistency across the monorepo. The
p-limitdependency at6.2.0is properly pinned for the throttling functionality used in secure-fs.apps/ui/package.json (2)
13-15: Engine constraint aligns with monorepo standards.The Node 22.x constraint matches other packages in the monorepo, ensuring consistent runtime requirements across all modules.
78-79: React 19.2.3 is a valid version available on npm. No action needed.apps/ui/src/main.ts (4)
15-35: Comprehensive import of centralized platform utilities.Good organization of imports with clear comments grouping them by purpose (userData operations, app bundle operations, system path operations). This improves code readability and makes security auditing easier.
97-120: API key management properly uses centralized methods.The migration from direct
fscalls toelectronUserDataReadFileSync/electronUserDataWriteFileSyncensures consistent path handling within the validated userData directory.
608-628: Path security initialization order is correct.The initialization sequence is proper:
setElectronUserDataPathfirstsetElectronAppPathswith appropriate paths for dev/prod- Set
DATA_DIRinprocess.env- Call
initAllowedPaths()lastThis ensures all path helpers are configured before security validation begins.
720-734: Directory dialog enforces ALLOWED_ROOT_DIRECTORY.Good security practice to validate the user-selected directory against
isPathAllowedbefore returning it. The error message provides clear guidance when a path is rejected, including the allowed root if configured.
| "@types/node": "22.19.3", | ||
| "typescript": "5.9.3" |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
TypeScript latest version 5.9 5.10 2024 2025
💡 Result:
TypeScript's latest stable major release is 5.9 (final released Aug 1, 2025). [1][2][3]
There is no official TypeScript 5.10 release from the Microsoft TypeScript project as of Dec 31, 2025 — only 5.9 (and its patch releases) are listed in the official release notes/releases. [1][2]
Sources:
- Announcing TypeScript 5.9 (Microsoft Dev Blog), Aug 1, 2025. [1]
- microsoft/TypeScript releases (GitHub) — TypeScript 5.9 listed as latest. [2]
- Coverage: TypeScript 5.9 release notes/news (InfoWorld/InfoQ). [3]
🏁 Script executed:
# Check the git history of this file to see what the previous version was
cd libs/types && git log --oneline -20 package.json 2>/dev/null || echo "Git not available or file not tracked"Repository: AutoMaker-Org/automaker
Length of output: 138
🏁 Script executed:
# Search for any comments or documentation about why exact pinning was chosen
rg -i "pinning|exact|version" libs/types/ --type json --type md --type txtRepository: AutoMaker-Org/automaker
Length of output: 112
Consider implications of exact version pinning versus caret ranges.
The dependencies have been pinned to exact versions (@types/node: 22.19.3, typescript: 5.9.3), removing the caret ranges. This is actually an upgrade—TypeScript 5.9.3 is the latest stable release (August 2025), not a downgrade from the previous ^5.7.3 constraint. While exact pinning improves build reproducibility, it prevents automatic patch updates and requires manual dependency management for security fixes and minor improvements.
🤖 Prompt for AI Agents
In libs/types/package.json lines 22-23 the dev deps were changed from caret
ranges to exact pins for @types/node and typescript; update them to use caret
ranges (e.g. ^22.19.3 and ^5.9.3) to allow automatic patch/minor updates, or if
strict reproducibility is required keep exact pins but document the decision in
the PR and enable automated security/patch updates (Dependabot/Renovate) so
critical fixes are not missed.
Changed the resolved URL for the @electron/node-gyp dependency in package-lock.json from SSH to HTTPS for improved accessibility and compatibility across different environments.
- Updated the TerminalService to utilize getShellPaths() for better shell detection across platforms. - Improved logic for detecting user-configured shells in WSL and added fallbacks for various platforms. - Enhanced unit tests to mock shell paths for comprehensive cross-platform testing, ensuring accurate shell detection behavior. These changes aim to streamline shell detection and improve the user experience across different operating systems.
- Start API key initialization immediately upon importing the HTTP API client module to ensure the init promise is created early. - Log errors during API key initialization to aid in debugging. Additionally, added a version field to the setup store for proper state hydration, aligning with the app-store pattern.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/ui/src/lib/http-api-client.tsapps/ui/src/store/setup-store.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/ui/src/lib/http-api-client.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always import from shared packages (@automaker/*), never from old relative paths
Files:
apps/ui/src/store/setup-store.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
resolveModelString()from @automaker/model-resolver to convert model aliases (haiku, sonnet, opus) to full model names
Files:
apps/ui/src/store/setup-store.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: e2e
| }), | ||
| { | ||
| name: 'automaker-setup', | ||
| version: 1, // Add version field for proper hydration (matches app-store pattern) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n apps/ui/src/store/setup-store.ts | head -200Repository: AutoMaker-Org/automaker
Length of output: 6075
🏁 Script executed:
rg "persist\(" --type ts --type tsx -A 10 | head -100Repository: AutoMaker-Org/automaker
Length of output: 94
🏁 Script executed:
rg "persist\(" -A 10 | grep -E "(persist|name:|version:|migrate)" | head -60Repository: AutoMaker-Org/automaker
Length of output: 152
🏁 Script executed:
cat -n apps/ui/src/store/app-store.ts | grep -A 20 "persist("Repository: AutoMaker-Org/automaker
Length of output: 863
🏁 Script executed:
cat -n apps/ui/src/store/app-store.ts | tail -50Repository: AutoMaker-Org/automaker
Length of output: 2901
🏁 Script executed:
sed -n '2850,2960p' apps/ui/src/store/app-store.tsRepository: AutoMaker-Org/automaker
Length of output: 5129
🏁 Script executed:
sed -n '1000,1050p' apps/ui/src/store/app-store.tsRepository: AutoMaker-Org/automaker
Length of output: 1640
🏁 Script executed:
sed -n '1015,1030p' apps/ui/src/store/app-store.tsRepository: AutoMaker-Org/automaker
Length of output: 538
🏁 Script executed:
grep -n "name.*automaker-app\|version.*migrate" apps/ui/src/store/app-store.tsRepository: AutoMaker-Org/automaker
Length of output: 49
🏁 Script executed:
sed -n '2750,2800p' apps/ui/src/store/app-store.tsRepository: AutoMaker-Org/automaker
Length of output: 1745
🏁 Script executed:
sed -n '2795,2820p' apps/ui/src/store/app-store.tsRepository: AutoMaker-Org/automaker
Length of output: 1403
🏁 Script executed:
sed -n '2802,2860p' apps/ui/src/store/app-store.tsRepository: AutoMaker-Org/automaker
Length of output: 2616
🏁 Script executed:
cat -n apps/ui/src/store/setup-store.ts | grep -A 2 "version:"Repository: AutoMaker-Org/automaker
Length of output: 427
🏁 Script executed:
rg "migrate.*version" -A 5 -B 5 --type tsRepository: AutoMaker-Org/automaker
Length of output: 738
🌐 Web query:
Zustand persist middleware version field behavior state clearing
💡 Result:
Short answer: persist's version is a numeric schema tag (default 0). On init, if the saved object's version !== your persist option's version, Zustand will NOT use the stored state unless you supply a migrate function that returns a compatible state. That behavior effectively lets you "clear" or replace persisted state by bumping version and either (a) not providing a migrate (causing the stored value to be ignored), (b) providing a migrate that returns an empty/initial state, or (c) programmatically removing the item from storage. The persist merge behavior is a shallow merge by default; use merge/onRehydrateStorage for custom handling. [1][2]
Quick pointers:
- version: number (default 0). If mismatch, stored value is ignored unless migrate handles it. [1][2]
- migrate(persistedState, version) should return the new-state (sync or Promise). Use it to transform older schemas into the current one. [1][2]
- merge: default shallow merge; override to control how persisted data is merged with initial state. [2]
- You can also clear storage directly (storage.removeItem or via custom storage). [2]
Sources:
- Zustand docs — Persisting store data / version & migrate. [1]
- Zustand middleware reference (persist options: version, migrate, merge, onRehydrateStorage). [2]
References:
[1] https://zustand.docs.pmnd.rs/integrations/persisting-store-data
[2] https://zustand.docs.pmnd.rs/middlewares/persist
Good practice to add version field for state versioning infrastructure.
Adding version: 1 to the persist configuration establishes versioning support for state migrations. This is particularly valuable given that the persisted state includes authentication data (claudeAuthStatus), ensuring that incompatible cached state will be cleared on schema evolution.
Note: Without a migrate function, Zustand ignores the persisted state if the stored version doesn't match. If future schema changes are needed, consider adding a migrate function similar to the pattern used in app-store.ts to handle version-specific transformations rather than simply discarding persisted data.
🤖 Prompt for AI Agents
In apps/ui/src/store/setup-store.ts around line 175, you've added "version: 1"
which is good for state versioning but there is no migrate function; add a
migrate function (patterned after app-store.ts) to the persist config that
accepts (persistedState, version) and returns a transformed state for older
versions or the current initial state, so that when the stored version differs
Zustand can safely migrate authentication-related fields (e.g.,
claudeAuthStatus) instead of discarding them; implement version checks and
transformations for prior versions and fall back to the default state when
migration is not possible.
…ndling - Introduced a new command for validating tests, providing detailed instructions for running tests and fixing failures based on code changes. - Updated the environment variable handling in the Claude provider to only allow explicitly defined variables, enhancing security and preventing leakage of sensitive information. - Improved feature loading to handle errors more gracefully and load features concurrently, optimizing performance. - Centralized port configuration for the Automaker application to prevent accidental termination of critical services.
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)
apps/ui/src/main.ts (1)
54-55: Import port constants from@automaker/platforminstead of redefining locally.These port values are already exported from
@automaker/platform(seelibs/platform/src/config/ports.ts). Using the centralized constants ensures a single source of truth, aligning with the PR's objective of centralizing configuration.🔎 Proposed fix
Update the imports at the top of the file:
import { findNodeExecutable, buildEnhancedPath, initAllowedPaths, isPathAllowed, getAllowedRootDirectory, + STATIC_PORT, + SERVER_PORT, // Electron userData operations setElectronUserDataPath, ... } from '@automaker/platform';Then remove the local definitions:
-const SERVER_PORT = 3008; -const STATIC_PORT = 3007;
🧹 Nitpick comments (5)
apps/server/src/services/feature-loader.ts (2)
204-204: Consider validating parsed JSON before type assertion.The parsed JSON is cast to
Featurewithout validation. If the JSON is malformed or missing required fields (beyond justid), this could lead to runtime errors or unexpected behavior in downstream code. Consider adding a validation function to verify the parsed object conforms to the Feature interface.Example validation approach
function isValidFeature(obj: any): obj is Feature { return ( obj && typeof obj.id === 'string' && typeof obj.category === 'string' && typeof obj.description === 'string' && (obj.steps === undefined || Array.isArray(obj.steps)) // Add other required field checks ); } // Then in the loading code: const feature = JSON.parse(content); if (!isValidFeature(feature)) { logger.warn(`[FeatureLoader] Feature ${featureId} has invalid structure`); return null; } return feature;
226-230: Optional: Consider more robust feature ID parsing for sorting.The sorting logic assumes a specific feature ID format (
feature-{timestamp}-{random}) and uses string splitting to extract the timestamp. If the ID format changes or if external systems create features with different ID patterns, the sorting could fail silently (falling back to timestamp 0).More defensive parsing approach
// Sort by creation order (feature IDs contain timestamp) features.sort((a, b) => { - const aTime = a.id ? parseInt(a.id.split('-')[1] || '0') : 0; - const bTime = b.id ? parseInt(b.id.split('-')[1] || '0') : 0; + const extractTimestamp = (id: string) => { + const match = id.match(/^feature-(\d+)-/); + return match ? parseInt(match[1], 10) : 0; + }; + const aTime = a.id ? extractTimestamp(a.id) : 0; + const bTime = b.id ? extractTimestamp(b.id) : 0; return aTime - bTime; });apps/ui/src/main.ts (1)
376-399: Simplify redundant nested try/catch blocks for tsx resolution.The outer try/catch (lines 391-399) duplicates the inner catch logic (lines 383-389). This can be simplified to a single try/catch.
🔎 Proposed simplification
let tsxCliPath: string; - // Check for tsx in app bundle paths - try { - if (electronAppExists(path.join(serverNodeModules, 'dist/cli.mjs'))) { - tsxCliPath = path.join(serverNodeModules, 'dist/cli.mjs'); - } else if (electronAppExists(path.join(rootNodeModules, 'dist/cli.mjs'))) { - tsxCliPath = path.join(rootNodeModules, 'dist/cli.mjs'); - } else { - try { - tsxCliPath = require.resolve('tsx/cli.mjs', { - paths: [path.join(__dirname, '../../server')], - }); - } catch { - throw new Error("Could not find tsx. Please run 'npm install' in the server directory."); - } - } - } catch { + // Check for tsx in app bundle paths + if (electronAppExists(path.join(serverNodeModules, 'dist/cli.mjs'))) { + tsxCliPath = path.join(serverNodeModules, 'dist/cli.mjs'); + } else if (electronAppExists(path.join(rootNodeModules, 'dist/cli.mjs'))) { + tsxCliPath = path.join(rootNodeModules, 'dist/cli.mjs'); + } else { try { tsxCliPath = require.resolve('tsx/cli.mjs', { paths: [path.join(__dirname, '../../server')], }); } catch { throw new Error("Could not find tsx. Please run 'npm install' in the server directory."); } - } + }libs/platform/src/secure-fs.ts (2)
168-175: Consolidate duplicate interface definitions.
WriteFileOptionsandWriteFileSyncOptionsare identical. Consider using a single shared type to avoid duplication.Suggested consolidation
/** - * Options for writeFile + * Options for writeFile and writeFileSync */ export interface WriteFileOptions { encoding?: BufferEncoding; mode?: number; flag?: string; } -// ... later in file ... -/** - * Options for writeFileSync - */ -export interface WriteFileSyncOptions { - encoding?: BufferEncoding; - mode?: number; - flag?: string; -} - export function writeFileSync( filePath: string, data: string | Buffer, - options?: WriteFileSyncOptions + options?: WriteFileOptions ): void {Also applies to: 325-329
464-483: Consider handling quoted values in .env parsing.The parser doesn't strip surrounding quotes from values. For example,
KEY="value"returns"value"(with quotes) rather thanvalue. Many .env parsers (like dotenv) strip these quotes.If this is intentional, consider adding a doc comment clarifying the behavior. If not, you may want to strip matching quotes:
Optional: Strip surrounding quotes
if (equalIndex > 0) { const key = trimmed.slice(0, equalIndex).trim(); - const value = trimmed.slice(equalIndex + 1).trim(); + let value = trimmed.slice(equalIndex + 1).trim(); + // Strip matching surrounding quotes + if ((value.startsWith('"') && value.endsWith('"')) || + (value.startsWith("'") && value.endsWith("'"))) { + value = value.slice(1, -1); + } result[key] = value; }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.claude/commands/validate-tests.mdapps/server/src/providers/claude-provider.tsapps/server/src/services/feature-loader.tsapps/ui/src/main.tslibs/platform/src/config/ports.tslibs/platform/src/index.tslibs/platform/src/secure-fs.tslibs/prompts/package.jsonlibs/prompts/src/defaults.ts
✅ Files skipped from review due to trivial changes (1)
- .claude/commands/validate-tests.md
🚧 Files skipped from review as they are similar to previous changes (3)
- libs/prompts/package.json
- apps/server/src/providers/claude-provider.ts
- libs/prompts/src/defaults.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always import from shared packages (@automaker/*), never from old relative paths
Files:
apps/server/src/services/feature-loader.tslibs/platform/src/config/ports.tslibs/platform/src/index.tslibs/platform/src/secure-fs.tsapps/ui/src/main.ts
apps/server/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
createEventEmitter()fromlib/events.tsfor all server operations to emit events that stream to frontend via WebSocket
Files:
apps/server/src/services/feature-loader.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
resolveModelString()from @automaker/model-resolver to convert model aliases (haiku, sonnet, opus) to full model names
Files:
apps/server/src/services/feature-loader.tslibs/platform/src/config/ports.tslibs/platform/src/index.tslibs/platform/src/secure-fs.tsapps/ui/src/main.ts
🧠 Learnings (1)
📚 Learning: 2025-12-28T05:07:48.138Z
Learnt from: CR
Repo: AutoMaker-Org/automaker PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-28T05:07:48.138Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Always import from shared packages (automaker/*), never from old relative paths
Applied to files:
apps/ui/src/main.ts
🧬 Code graph analysis (4)
apps/server/src/services/feature-loader.ts (2)
libs/types/src/index.ts (1)
Feature(23-23)apps/ui/src/store/app-store.ts (1)
Feature(261-277)
libs/platform/src/config/ports.ts (1)
libs/platform/src/index.ts (3)
STATIC_PORT(120-120)SERVER_PORT(120-120)RESERVED_PORTS(120-120)
libs/platform/src/secure-fs.ts (1)
libs/platform/src/index.ts (1)
validatePath(41-41)
apps/ui/src/main.ts (3)
libs/platform/src/index.ts (12)
electronUserDataExists(103-103)electronUserDataReadFileSync(101-101)electronUserDataWriteFileSync(102-102)electronAppExists(112-112)electronAppStat(115-115)electronAppReadFile(116-116)systemPathExists(80-80)setElectronUserDataPath(98-98)setElectronAppPaths(111-111)initAllowedPaths(39-39)isPathAllowed(40-40)getAllowedRootDirectory(43-43)libs/platform/src/system-paths.ts (9)
electronUserDataExists(537-541)electronUserDataReadFileSync(508-517)electronUserDataWriteFileSync(522-532)electronAppExists(652-657)electronAppStat(682-694)electronAppReadFile(699-711)systemPathExists(270-275)setElectronUserDataPath(481-483)setElectronAppPaths(613-616)libs/types/src/settings.ts (1)
WindowBounds(80-91)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: e2e
🔇 Additional comments (16)
apps/server/src/services/feature-loader.ts (1)
188-223: LGTM! Concurrent loading improves performance.The refactor from sequential to concurrent feature loading using Promise.all is well-executed. The error handling appropriately returns null for missing files, parse errors, and missing required fields, then filters out these nulls to produce the final feature list. The built-in concurrency limiting in secureFs ensures the system isn't overwhelmed.
libs/platform/src/config/ports.ts (1)
1-15: LGTM!Clean implementation of centralized port configuration. The
as constassertion ensures type safety, and the JSDoc comments clearly document the purpose of each port.libs/platform/src/index.ts (2)
58-117: LGTM!Well-organized exports with clear categorization. The dual export pattern (namespace + individual exports) provides flexibility for consumers to import either way.
119-120: LGTM!Port configuration exports are properly re-exported from the platform package.
apps/ui/src/main.ts (9)
15-35: LGTM!Imports correctly use the
@automaker/platformpackage as required by the coding guidelines, with clear categorization comments.
97-120: LGTM!API key handling correctly uses centralized
electronUserData*methods with proper error handling and secure file permissions (0o600).
140-151: LGTM!Icon path validation correctly uses
electronAppExistswith proper error handling and fallback tonull.
162-194: LGTM!Window bounds persistence correctly uses centralized
electronUserData*methods with proper JSON validation.
280-326: LGTM!Static server correctly uses centralized
electronApp*methods for serving files from the app bundle, with proper SPA routing fallback.
350-364: LGTM!Good improvement addressing the previous review feedback. The error handling now properly distinguishes between access errors and not-found cases, providing useful context in error messages.
419-456: LGTM!Server startup correctly computes
serverRootfor both dev and production, passes appropriate environment variables, and uses the correct working directory for the spawned process.
608-628: LGTM!Path security helpers are properly initialized early in the startup sequence, with appropriate paths configured for both development and production modes.
720-736: LGTM!Excellent security enhancement validating selected directories against
ALLOWED_ROOT_DIRECTORYwith a user-friendly error message when the path is not allowed.libs/platform/src/secure-fs.ts (3)
14-14: LGTM!Import correctly brings in the synchronous fs module and necessary types for the new sync API surface.
331-419: LGTM!Synchronous methods correctly mirror the async counterparts with consistent path validation. The lack of throttling/retry is expected for sync operations.
429-570: LGTM on the env file operation wrappers.The async/sync pairs are well-structured with appropriate ENOENT handling, and path validation is consistently applied.
| function updateEnvContent(content: string, key: string, value: string): string { | ||
| const lines = content.split('\n'); | ||
| const keyPrefix = `${key}=`; | ||
| let found = false; | ||
|
|
||
| const newLines = lines.map((line) => { | ||
| if (line.trim().startsWith(keyPrefix)) { | ||
| found = true; | ||
| return `${key}=${value}`; | ||
| } | ||
| return line; | ||
| }); | ||
|
|
||
| if (!found) { | ||
| // Add the key at the end | ||
| if (newLines.length > 0 && newLines[newLines.length - 1].trim() !== '') { | ||
| newLines.push(`${key}=${value}`); | ||
| } else { | ||
| // Replace last empty line or add to empty file | ||
| if (newLines.length === 0 || (newLines.length === 1 && newLines[0] === '')) { | ||
| newLines[0] = `${key}=${value}`; | ||
| } else { | ||
| newLines[newLines.length - 1] = `${key}=${value}`; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Ensure file ends with newline | ||
| let result = newLines.join('\n'); | ||
| if (!result.endsWith('\n')) { | ||
| result += '\n'; | ||
| } | ||
| return result; | ||
| } |
There was a problem hiding this comment.
Inconsistent key matching between parse and update logic.
There's a mismatch between how parseEnvContent and updateEnvContent identify keys:
parseEnvContent(line 476) handlesKEY = valueformat (spaces around=) by trimming the keyupdateEnvContent(line 581) usesstartsWith(keyPrefix)wherekeyPrefix = "${key}=", which won't matchKEY = value
If an .env file contains API_KEY = secret123, calling writeEnvKey(path, "API_KEY", "newvalue") will add a duplicate entry instead of updating the existing one.
Suggested fix: Match key more flexibly
function updateEnvContent(content: string, key: string, value: string): string {
const lines = content.split('\n');
- const keyPrefix = `${key}=`;
let found = false;
const newLines = lines.map((line) => {
- if (line.trim().startsWith(keyPrefix)) {
+ const trimmed = line.trim();
+ const equalIndex = trimmed.indexOf('=');
+ if (equalIndex > 0 && trimmed.slice(0, equalIndex).trim() === key) {
found = true;
return `${key}=${value}`;
}
return line;
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function updateEnvContent(content: string, key: string, value: string): string { | |
| const lines = content.split('\n'); | |
| const keyPrefix = `${key}=`; | |
| let found = false; | |
| const newLines = lines.map((line) => { | |
| if (line.trim().startsWith(keyPrefix)) { | |
| found = true; | |
| return `${key}=${value}`; | |
| } | |
| return line; | |
| }); | |
| if (!found) { | |
| // Add the key at the end | |
| if (newLines.length > 0 && newLines[newLines.length - 1].trim() !== '') { | |
| newLines.push(`${key}=${value}`); | |
| } else { | |
| // Replace last empty line or add to empty file | |
| if (newLines.length === 0 || (newLines.length === 1 && newLines[0] === '')) { | |
| newLines[0] = `${key}=${value}`; | |
| } else { | |
| newLines[newLines.length - 1] = `${key}=${value}`; | |
| } | |
| } | |
| } | |
| // Ensure file ends with newline | |
| let result = newLines.join('\n'); | |
| if (!result.endsWith('\n')) { | |
| result += '\n'; | |
| } | |
| return result; | |
| } | |
| function updateEnvContent(content: string, key: string, value: string): string { | |
| const lines = content.split('\n'); | |
| let found = false; | |
| const newLines = lines.map((line) => { | |
| const trimmed = line.trim(); | |
| const equalIndex = trimmed.indexOf('='); | |
| if (equalIndex > 0 && trimmed.slice(0, equalIndex).trim() === key) { | |
| found = true; | |
| return `${key}=${value}`; | |
| } | |
| return line; | |
| }); | |
| if (!found) { | |
| // Add the key at the end | |
| if (newLines.length > 0 && newLines[newLines.length - 1].trim() !== '') { | |
| newLines.push(`${key}=${value}`); | |
| } else { | |
| // Replace last empty line or add to empty file | |
| if (newLines.length === 0 || (newLines.length === 1 && newLines[0] === '')) { | |
| newLines[0] = `${key}=${value}`; | |
| } else { | |
| newLines[newLines.length - 1] = `${key}=${value}`; | |
| } | |
| } | |
| } | |
| // Ensure file ends with newline | |
| let result = newLines.join('\n'); | |
| if (!result.endsWith('\n')) { | |
| result += '\n'; | |
| } | |
| return result; | |
| } |
| function removeEnvKeyFromContent(content: string, key: string): string { | ||
| const lines = content.split('\n'); | ||
| const keyPrefix = `${key}=`; | ||
| const newLines = lines.filter((line) => !line.trim().startsWith(keyPrefix)); | ||
|
|
||
| // Remove trailing empty lines | ||
| while (newLines.length > 0 && newLines[newLines.length - 1].trim() === '') { | ||
| newLines.pop(); | ||
| } | ||
|
|
||
| // Ensure file ends with newline if there's content | ||
| let result = newLines.join('\n'); | ||
| if (result.length > 0 && !result.endsWith('\n')) { | ||
| result += '\n'; | ||
| } | ||
| return result; |
There was a problem hiding this comment.
Same key-matching inconsistency applies here.
This function has the same issue as updateEnvContent — it won't match lines in KEY = value format. Apply the same fix for consistency:
Suggested fix
function removeEnvKeyFromContent(content: string, key: string): string {
const lines = content.split('\n');
- const keyPrefix = `${key}=`;
- const newLines = lines.filter((line) => !line.trim().startsWith(keyPrefix));
+ const newLines = lines.filter((line) => {
+ const trimmed = line.trim();
+ const equalIndex = trimmed.indexOf('=');
+ if (equalIndex > 0 && trimmed.slice(0, equalIndex).trim() === key) {
+ return false; // Remove this line
+ }
+ return true;
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function removeEnvKeyFromContent(content: string, key: string): string { | |
| const lines = content.split('\n'); | |
| const keyPrefix = `${key}=`; | |
| const newLines = lines.filter((line) => !line.trim().startsWith(keyPrefix)); | |
| // Remove trailing empty lines | |
| while (newLines.length > 0 && newLines[newLines.length - 1].trim() === '') { | |
| newLines.pop(); | |
| } | |
| // Ensure file ends with newline if there's content | |
| let result = newLines.join('\n'); | |
| if (result.length > 0 && !result.endsWith('\n')) { | |
| result += '\n'; | |
| } | |
| return result; | |
| function removeEnvKeyFromContent(content: string, key: string): string { | |
| const lines = content.split('\n'); | |
| const newLines = lines.filter((line) => { | |
| const trimmed = line.trim(); | |
| const equalIndex = trimmed.indexOf('='); | |
| if (equalIndex > 0 && trimmed.slice(0, equalIndex).trim() === key) { | |
| return false; // Remove this line | |
| } | |
| return true; | |
| }); | |
| // Remove trailing empty lines | |
| while (newLines.length > 0 && newLines[newLines.length - 1].trim() === '') { | |
| newLines.pop(); | |
| } | |
| // Ensure file ends with newline if there's content | |
| let result = newLines.join('\n'); | |
| if (result.length > 0 && !result.endsWith('\n')) { | |
| result += '\n'; | |
| } | |
| return result; | |
| } |
🤖 Prompt for AI Agents
In libs/platform/src/secure-fs.ts around lines 613 to 628,
removeEnvKeyFromContent currently only detects env lines that start with "KEY="
and therefore misses lines like "KEY = value"; replace the startsWith check with
a regex-based match that trims leading whitespace and allows optional spaces
around the '=' (e.g. build a RegExp that matches ^\s*<escapedKey>\s*=) and use
that to filter out matching lines; ensure you escape the key for use in the
regex and keep the existing trailing-empty-line removal and final newline
preservation logic unchanged.
- Introduced a new endpoint to check if the application is running in a containerized environment, allowing the UI to display appropriate risk warnings. - Added a confirmation dialog for users when running outside a sandbox, requiring acknowledgment of potential risks before proceeding. - Implemented a rejection screen for users who deny sandbox risk confirmation, providing options to restart in a container or reload the application. - Updated the main application logic to handle sandbox status checks and user responses effectively, enhancing security and user experience.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
apps/ui/src/lib/electron.ts (1)
432-650: Consider consolidating duplicate interface definitions.The
ElectronAPIinterface is defined in both this file andapps/ui/src/types/electron.d.ts. Consider importing the interface from the types file instead of maintaining a duplicate definition to prevent them from diverging over time.🔎 Example approach to consolidate
Import the interface from the types file:
import type { ElectronAPI } from '@/types/electron';This would eliminate the duplicate definition and ensure consistency across the codebase.
apps/ui/src/components/dialogs/sandbox-risk-dialog.tsx (1)
31-39: Consider cleanup for timeout on unmount.The
setTimeoutinhandleCopyisn't cleaned up if the component unmounts while the timeout is pending, which could cause a state update on an unmounted component.🔎 Proposed fix using useRef for cleanup
+import { useState, useRef, useEffect } from 'react'; -import { useState } from 'react'; ... export function SandboxRiskDialog({ open, onConfirm, onDeny }: SandboxRiskDialogProps) { const [copied, setCopied] = useState(false); + const timeoutRef = useRef<ReturnType<typeof setTimeout> | null>(null); + + useEffect(() => { + return () => { + if (timeoutRef.current) { + clearTimeout(timeoutRef.current); + } + }; + }, []); const handleCopy = async () => { try { await navigator.clipboard.writeText(DOCKER_COMMAND); setCopied(true); - setTimeout(() => setCopied(false), 2000); + timeoutRef.current = setTimeout(() => setCopied(false), 2000); } catch (err) { console.error('Failed to copy:', err); } };apps/ui/src/components/dialogs/sandbox-rejection-screen.tsx (2)
12-12: Extract shared constant to avoid duplication.
DOCKER_COMMANDis duplicated betweensandbox-risk-dialog.tsxand this file. Consider extracting it to a shared location.🔎 Suggested approach
Create a shared constants file or export from one component:
// In a shared constants file, e.g., @/config/constants.ts export const DOCKER_COMMAND = 'npm run dev:docker';Then import in both dialog components.
23-31: Same timeout cleanup consideration applies here.Similar to
SandboxRiskDialog, thesetTimeoutinhandleCopycould benefit from cleanup on unmount. Given these components share the same copy logic, this reinforces extracting a reusableuseCopyToClipboardhook.apps/ui/src/routes/__root.tsx (1)
305-310: Consider consolidatingSandboxRiskDialogrendering.The dialog is rendered identically in three places (login route, setup route, and main layout). Since it's controlled by
sandboxStatusand renders as a modal overlay, you could render it once at the end ofRootLayoutContentto reduce duplication.🔎 Suggested consolidation
Remove the dialog from login and setup route returns, and keep only the one at lines 361-366 which is already in the main return. Since the dialog uses
open={sandboxStatus === 'needs-confirmation'}, it will show as an overlay regardless of which route content is underneath.// Show login page (full screen, no sidebar) if (isLoginRoute) { return ( <main className="h-screen overflow-hidden" data-testid="app-container"> <Outlet /> - {/* Show sandbox dialog on top of login page if needed */} - <SandboxRiskDialog - open={sandboxStatus === 'needs-confirmation'} - onConfirm={handleSandboxConfirm} - onDeny={handleSandboxDeny} - /> </main> ); }Apply similar change to the setup route. The main layout's dialog instance will handle all cases.
Also applies to: 333-338, 361-366
apps/ui/src/main.ts (1)
376-399: Simplify the nested try/catch blocks for tsx path resolution.The nested try/catch structure is complex. The outer catch (line 391) falls through to identical logic in its body, making the outer try/catch redundant.
🔎 Proposed simplification
let tsxCliPath: string; - // Check for tsx in app bundle paths - try { - if (electronAppExists(path.join(serverNodeModules, 'dist/cli.mjs'))) { - tsxCliPath = path.join(serverNodeModules, 'dist/cli.mjs'); - } else if (electronAppExists(path.join(rootNodeModules, 'dist/cli.mjs'))) { - tsxCliPath = path.join(rootNodeModules, 'dist/cli.mjs'); - } else { - try { - tsxCliPath = require.resolve('tsx/cli.mjs', { - paths: [path.join(__dirname, '../../server')], - }); - } catch { - throw new Error("Could not find tsx. Please run 'npm install' in the server directory."); - } - } - } catch { + // Check for tsx in app bundle paths + const serverTsxPath = path.join(serverNodeModules, 'dist/cli.mjs'); + const rootTsxPath = path.join(rootNodeModules, 'dist/cli.mjs'); + + try { + if (electronAppExists(serverTsxPath)) { + tsxCliPath = serverTsxPath; + } else if (electronAppExists(rootTsxPath)) { + tsxCliPath = rootTsxPath; + } else { + // Fallback to require.resolve + tsxCliPath = require.resolve('tsx/cli.mjs', { + paths: [path.join(__dirname, '../../server')], + }); + } + } catch { try { tsxCliPath = require.resolve('tsx/cli.mjs', { paths: [path.join(__dirname, '../../server')], }); } catch { throw new Error("Could not find tsx. Please run 'npm install' in the server directory."); } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
apps/server/src/routes/health/index.tsapps/server/src/routes/health/routes/environment.tsapps/ui/src/components/dialogs/index.tsapps/ui/src/components/dialogs/sandbox-rejection-screen.tsxapps/ui/src/components/dialogs/sandbox-risk-dialog.tsxapps/ui/src/lib/electron.tsapps/ui/src/lib/http-api-client.tsapps/ui/src/main.tsapps/ui/src/preload.tsapps/ui/src/routes/__root.tsxapps/ui/src/types/electron.d.tsdocker-compose.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/ui/src/lib/http-api-client.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always import from shared packages (@automaker/*), never from old relative paths
Files:
apps/ui/src/types/electron.d.tsapps/ui/src/components/dialogs/sandbox-risk-dialog.tsxapps/ui/src/components/dialogs/index.tsapps/ui/src/preload.tsapps/ui/src/components/dialogs/sandbox-rejection-screen.tsxapps/ui/src/lib/electron.tsapps/server/src/routes/health/routes/environment.tsapps/server/src/routes/health/index.tsapps/ui/src/main.tsapps/ui/src/routes/__root.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
resolveModelString()from @automaker/model-resolver to convert model aliases (haiku, sonnet, opus) to full model names
Files:
apps/ui/src/types/electron.d.tsapps/ui/src/components/dialogs/sandbox-risk-dialog.tsxapps/ui/src/components/dialogs/index.tsapps/ui/src/preload.tsapps/ui/src/components/dialogs/sandbox-rejection-screen.tsxapps/ui/src/lib/electron.tsapps/server/src/routes/health/routes/environment.tsapps/server/src/routes/health/index.tsapps/ui/src/main.tsapps/ui/src/routes/__root.tsx
apps/server/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
createEventEmitter()fromlib/events.tsfor all server operations to emit events that stream to frontend via WebSocket
Files:
apps/server/src/routes/health/routes/environment.tsapps/server/src/routes/health/index.ts
🧠 Learnings (2)
📚 Learning: 2025-12-30T01:02:07.114Z
Learnt from: illia1f
Repo: AutoMaker-Org/automaker PR: 324
File: apps/ui/src/components/views/board-view/components/kanban-card/kanban-card.tsx:122-131
Timestamp: 2025-12-30T01:02:07.114Z
Learning: Tailwind CSS v4 uses postfix syntax for the important modifier: append ! at the end of the utility class (e.g., backdrop-blur-[0px]! or hover:bg-red-500!). The older v3 style used a prefix (!) at the start (e.g., !backdrop-blur-[0px]); prefer the postfix form for consistency across TSX files.
Applied to files:
apps/ui/src/components/dialogs/sandbox-risk-dialog.tsxapps/ui/src/components/dialogs/sandbox-rejection-screen.tsxapps/ui/src/routes/__root.tsx
📚 Learning: 2025-12-28T05:07:48.138Z
Learnt from: CR
Repo: AutoMaker-Org/automaker PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-28T05:07:48.138Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Always import from shared packages (automaker/*), never from old relative paths
Applied to files:
apps/ui/src/main.ts
🧬 Code graph analysis (3)
apps/server/src/routes/health/index.ts (2)
apps/ui/src/utils/router.ts (1)
router(11-15)apps/server/src/routes/health/routes/environment.ts (1)
createEnvironmentHandler(14-20)
apps/ui/src/main.ts (2)
libs/platform/src/index.ts (12)
electronUserDataExists(103-103)electronUserDataReadFileSync(101-101)electronUserDataWriteFileSync(102-102)electronAppExists(112-112)electronAppStat(115-115)electronAppReadFile(116-116)systemPathExists(80-80)setElectronUserDataPath(98-98)setElectronAppPaths(111-111)initAllowedPaths(39-39)isPathAllowed(40-40)getAllowedRootDirectory(43-43)libs/platform/src/system-paths.ts (9)
electronUserDataExists(537-541)electronUserDataReadFileSync(508-517)electronUserDataWriteFileSync(522-532)electronAppExists(652-657)electronAppStat(682-694)electronAppReadFile(699-711)systemPathExists(270-275)setElectronUserDataPath(481-483)setElectronAppPaths(613-616)
apps/ui/src/routes/__root.tsx (4)
apps/ui/src/lib/http-api-client.ts (1)
checkSandboxEnvironment(301-321)apps/ui/src/lib/electron.ts (1)
isElectron(676-686)apps/ui/src/components/dialogs/sandbox-rejection-screen.tsx (1)
SandboxRejectionScreen(14-90)apps/ui/src/components/dialogs/sandbox-risk-dialog.tsx (1)
SandboxRiskDialog(28-112)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: e2e
🔇 Additional comments (15)
apps/ui/src/types/electron.d.ts (1)
468-468: LGTM: Clean API addition.The optional
quitmethod is appropriately typed and uses the optional modifier for backward compatibility.apps/ui/src/preload.ts (1)
53-55: LGTM: Consistent IPC mapping.The
quitmethod follows the established pattern for IPC communication and aligns with the type definition inelectron.d.ts.docker-compose.yml (1)
54-56: LGTM: Well-documented environment flag.The
IS_CONTAINERIZEDenvironment variable is clearly documented and appropriately set totruefor the containerized context.apps/ui/src/components/dialogs/index.ts (1)
6-7: LGTM: Standard barrel exports.The new dialog exports follow the established pattern in this file.
apps/ui/src/lib/electron.ts (1)
435-435: LGTM: API addition matches type definition.The
quitmethod addition is consistent with the interface definition intypes/electron.d.ts.apps/server/src/routes/health/index.ts (1)
22-24: LGTM! Environment route appropriately placed as unauthenticated.The
/environmentendpoint correctly exposes only the containerization status without sensitive information, making it safe to be unauthenticated for pre-login UI checks.apps/server/src/routes/health/routes/environment.ts (1)
1-20: Clean implementation following existing route patterns.The handler correctly uses a factory function pattern consistent with other routes, properly types the response, and safely exposes only the containerization boolean.
apps/ui/src/components/dialogs/sandbox-risk-dialog.tsx (1)
41-48: Good defensive approach preventing accidental dismissal.The combination of empty
onOpenChange,onPointerDownOutside, andonEscapeKeyDownhandlers ensures users must explicitly choose an action, which is appropriate for this security-critical dialog.apps/ui/src/routes/__root.tsx (2)
41-53: Well-structured state machine for sandbox status.The initialization from
sessionStorageand the clear state transitions (pending→containerized/needs-confirmation→confirmed/denied) make the flow easy to follow.
136-156: No action required—electronAPI.quit() is properly exposed.The
quitmethod is correctly exposed in the preload script (line 55 of preload.ts) and the correspondingapp:quitIPC handler is registered in main.ts (line 843). The fallback error handling in your code is appropriate and defensive.apps/ui/src/main.ts (5)
15-35: Good adoption of centralized platform APIs.The comprehensive import of path validation and FS operations from
@automaker/platformaligns with the PR's security objectives. As per coding guidelines, imports from shared packages are preferred.
351-364: Improved error handling for Node.js executable validation.The refactored code now properly separates the existence check from error handling and provides clear error messages with source context. This addresses the previous review feedback about redundant try/catch blocks.
720-734: Good security gate for directory selection.Validating user-selected paths against
ALLOWED_ROOT_DIRECTORYbefore returning them prevents access outside the permitted scope. The user-facing error dialog provides clear guidance.
841-846: Newapp:quithandler completes the sandbox denial flow.This IPC handler enables the UI to gracefully quit the application when the user denies sandbox risk in Electron mode.
614-620: Both call signatures are correctly supported by the function definition.The function signature
setElectronAppPaths(appDirOrDirs: string | string[], resourcesPath?: string): voidaccepts both patterns:
- Dev mode passes an array (valid, matches
string[])- Production mode passes a string and optional resources path (valid, matches
stringand optionalresourcesPath)No changes needed.
…iguration - Introduced a new command for validating project builds, providing detailed instructions for running builds and intelligently fixing failures based on recent changes. - Refactored port configuration by centralizing it in the @automaker/types package for improved maintainability and backward compatibility. - Updated imports in various modules to reflect the new centralized port configuration, ensuring consistent usage across the application.
- Added an environment variable to simulate a containerized environment, allowing the application to skip sandbox confirmation dialogs during testing. - This change aims to streamline the testing process by reducing unnecessary user interactions while ensuring the application behaves as expected in a containerized setup.
This change improves the overall security posture of the application by enforcing strict file access controls and validating paths before any operations are performed.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.