Conversation
…or autonomous mode - Removed MCP permission settings from the application, including related functions and UI components. - Updated SDK options to always bypass permissions and allow unrestricted tool access in autonomous mode. - Adjusted related components and services to reflect the removal of MCP permission configurations, ensuring a cleaner and more efficient codebase.
|
Warning Rate limit exceeded@webdevcody has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 7 minutes and 12 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
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. 📝 WalkthroughWalkthroughThis PR removes MCP permission toggle settings ( Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes The PR spans 25+ files across backend, frontend, types, tests, and build infrastructure. While changes within each cohort are homogeneous (same refactoring repeated), the overall scope is large and interconnected—removing an established permission system throughout the codebase, introducing autonomous mode permission handling, and refactoring multi-file launcher orchestration. The launcher-utils module introduces significant new complexity with process management, port resolution, and health checking logic. Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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 focuses on enhancing application stability and developer experience. It resolves a memory leak in the UI by proactively clearing performance metrics and simplifies the management of Multi-Cloud Platform (MCP) tool permissions by defaulting to an autonomous bypass mode. Additionally, a new production launch script has been added to streamline the deployment and startup process for both web and desktop environments. 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 fix for a memory leak in the UI, which is a welcome improvement. However, it also includes a major change to hardcode bypassPermissions across the application, effectively enabling a fully autonomous mode with significant security implications. While this simplifies some logic by removing configurable permission settings, it grants broad, dangerous permissions to operations that should be read-only, such as spec generation, feature generation, and even simple title generation. This poses a critical security risk. I've left specific comments on these areas with suggestions to restore more restrictive permissions. The memory leak fix in apps/ui/src/app.tsx is good, but I've suggested a small improvement to ensure it only runs in development mode.
| // AUTONOMOUS MODE: Always bypass permissions | ||
| permissionMode: 'bypassPermissions', | ||
| allowDangerouslySkipPermissions: true, |
There was a problem hiding this comment.
Using bypassPermissions for title generation is a critical security risk. This operation is purely text-based and should not have any access to the file system or shell commands. Please set permissionMode to 'default'. The allowedTools is already correctly set to an empty array.
| // AUTONOMOUS MODE: Always bypass permissions | |
| permissionMode: 'bypassPermissions', | |
| allowDangerouslySkipPermissions: true, | |
| permissionMode: 'default', |
| // Using "acceptEdits" can cause Claude to write files to unexpected locations | ||
| // See: https://github.com/AutoMaker-Org/automaker/issues/149 | ||
| permissionMode: 'default', | ||
| // AUTONOMOUS MODE: Base options already set bypassPermissions and allowDangerouslySkipPermissions |
There was a problem hiding this comment.
By removing permissionMode: 'default', createSpecGenerationOptions now inherits bypassPermissions from getBaseOptions. This grants spec generation full, unrestricted access to the file system and shell commands, which is a significant security risk for an operation that should be read-only. Please consider restoring a more restrictive permission mode for spec generation to prevent unintended side effects. For example, you could override permissionMode and allowDangerouslySkipPermissions.
| // AUTONOMOUS MODE: Base options already set bypassPermissions and allowDangerouslySkipPermissions | |
| // Override permissionMode - spec generation only needs read-only tools. | |
| // Reverting to 'default' to maintain read-only access for security. | |
| permissionMode: 'default', | |
| allowDangerouslySkipPermissions: false, |
| ...getBaseOptions(), | ||
| // Override permissionMode - feature generation only needs read-only tools | ||
| permissionMode: 'default', | ||
| // AUTONOMOUS MODE: Base options already set bypassPermissions and allowDangerouslySkipPermissions |
There was a problem hiding this comment.
Similar to spec generation, createFeatureGenerationOptions now inherits bypassPermissions. This is a security risk for what should be a read-only operation. Please consider restoring a more restrictive permission mode.
| // AUTONOMOUS MODE: Base options already set bypassPermissions and allowDangerouslySkipPermissions | |
| // Override permissionMode - feature generation only needs read-only tools. | |
| // Reverting to 'default' to maintain read-only access for security. | |
| permissionMode: 'default', | |
| allowDangerouslySkipPermissions: false, |
| // AUTONOMOUS MODE: Always bypass permissions | ||
| permissionMode: 'bypassPermissions', | ||
| allowDangerouslySkipPermissions: true, |
There was a problem hiding this comment.
Using bypassPermissions for prompt enhancement is overly permissive and poses a security risk. This operation should not require dangerous permissions. Please consider using a more restrictive mode like 'default' or the original 'acceptEdits'. Since this operation shouldn't need to write files, 'default' with an empty allowedTools array seems safest, but restoring 'acceptEdits' is also an improvement.
| // AUTONOMOUS MODE: Always bypass permissions | |
| permissionMode: 'bypassPermissions', | |
| allowDangerouslySkipPermissions: true, | |
| permissionMode: 'acceptEdits', |
| useEffect(() => { | ||
| const clearPerfEntries = () => { | ||
| performance.clearMarks(); | ||
| performance.clearMeasures(); | ||
| }; | ||
| const interval = setInterval(clearPerfEntries, 5000); | ||
| return () => clearInterval(interval); | ||
| }, []); |
There was a problem hiding this comment.
The fix to clear performance entries to prevent a memory leak is a good addition. However, this should only run in development mode, as performance monitoring is typically not needed in production and this setInterval could add unnecessary overhead. You can wrap this useEffect's logic in a condition like if (import.meta.env.DEV) { ... }.
| useEffect(() => { | |
| const clearPerfEntries = () => { | |
| performance.clearMarks(); | |
| performance.clearMeasures(); | |
| }; | |
| const interval = setInterval(clearPerfEntries, 5000); | |
| return () => clearInterval(interval); | |
| }, []); | |
| useEffect(() => { | |
| if (import.meta.env.DEV) { | |
| const clearPerfEntries = () => { | |
| performance.clearMarks(); | |
| performance.clearMeasures(); | |
| }; | |
| const interval = setInterval(clearPerfEntries, 5000); | |
| return () => clearInterval(interval); | |
| } | |
| }, []); |
![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 (4)
TODO.md (1)
1-17: Consider whether TODO.md belongs in this PR.This new file documents various bugs and UX improvements, but the PR is specifically titled "Fix memory leak" and removes MCP auto-approval/unrestricted-tools configuration. While line 12 mentions memory leaks, the rest of the content appears unrelated to this PR's scope.
Consider:
- Moving this file to a separate PR focused on documentation/planning
- Creating individual GitHub issues for each item instead
- If keeping it here, update the PR description to mention adding a TODO file
Minor: Possible typo on line 8
The phrase "nessa tabs" may be a typo. Did you mean "nested tabs"?
apps/ui/src/app.tsx (1)
18-27: Add a development-mode check to the performance cleanup.The implementation correctly addresses React 19.2's performance scheduler memory leak issue, but the periodic clearing runs in production builds despite the comment referencing "dev mode." Since performance marks/measures are primarily used for development profiling in React DevTools, this cleanup is unnecessary in production.
Add a development-mode check:
Suggested improvement
useEffect(() => { + if (process.env.NODE_ENV !== 'development') return; + const clearPerfEntries = () => { performance.clearMarks(); performance.clearMeasures(); }; const interval = setInterval(clearPerfEntries, 5000); return () => clearInterval(interval); }, []);start.mjs (1)
683-684: Fixed 2-second sleep may be insufficient for Vite preview startup.Using a fixed
sleep(2000)to wait for the Vite preview server is fragile. Consider implementing a health check or port availability check similar to the server startup flow for more reliable startup detection.apps/server/src/lib/sdk-options.ts (1)
298-303: Consider removing redundant bypassOptions from buildMcpOptions.Since
getBaseOptions()already setspermissionMode: 'bypassPermissions'andallowDangerouslySkipPermissions: true, thebypassOptionsinbuildMcpOptionsis redundant. The comment on line 298 acknowledges this ("though base options already set this").You could simplify by removing
bypassOptionsentirely and relying on the base options, or keep it for explicit documentation purposes. Either approach is acceptable.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
TODO.mdapps/server/src/lib/sdk-options.tsapps/server/src/lib/settings-helpers.tsapps/server/src/providers/claude-provider.tsapps/server/src/routes/enhance-prompt/routes/enhance.tsapps/server/src/routes/features/routes/generate-title.tsapps/server/src/services/agent-service.tsapps/server/src/services/auto-mode-service.tsapps/server/tests/unit/lib/settings-helpers.test.tsapps/ui/src/app.tsxapps/ui/src/components/views/settings-view/mcp-servers/components/index.tsapps/ui/src/components/views/settings-view/mcp-servers/components/mcp-permission-settings.tsxapps/ui/src/components/views/settings-view/mcp-servers/hooks/use-mcp-servers.tsapps/ui/src/components/views/settings-view/mcp-servers/mcp-servers-section.tsxapps/ui/src/hooks/use-settings-migration.tsapps/ui/src/lib/http-api-client.tsapps/ui/src/store/app-store.tslibs/types/src/provider.tslibs/types/src/settings.tspackage.jsonstart.mjs
💤 Files with no reviewable changes (9)
- apps/server/src/lib/settings-helpers.ts
- apps/ui/src/components/views/settings-view/mcp-servers/components/index.ts
- libs/types/src/provider.ts
- apps/ui/src/lib/http-api-client.ts
- apps/server/src/services/agent-service.ts
- apps/ui/src/components/views/settings-view/mcp-servers/components/mcp-permission-settings.tsx
- libs/types/src/settings.ts
- apps/ui/src/store/app-store.ts
- apps/server/src/services/auto-mode-service.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/hooks/use-settings-migration.tsapps/server/src/routes/enhance-prompt/routes/enhance.tsapps/server/src/providers/claude-provider.tsapps/server/tests/unit/lib/settings-helpers.test.tsapps/ui/src/components/views/settings-view/mcp-servers/mcp-servers-section.tsxapps/ui/src/app.tsxapps/ui/src/components/views/settings-view/mcp-servers/hooks/use-mcp-servers.tsapps/server/src/routes/features/routes/generate-title.tsapps/server/src/lib/sdk-options.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/hooks/use-settings-migration.tsapps/server/src/routes/enhance-prompt/routes/enhance.tsapps/server/src/providers/claude-provider.tsapps/server/tests/unit/lib/settings-helpers.test.tsapps/ui/src/components/views/settings-view/mcp-servers/mcp-servers-section.tsxapps/ui/src/app.tsxapps/ui/src/components/views/settings-view/mcp-servers/hooks/use-mcp-servers.tsapps/server/src/routes/features/routes/generate-title.tsapps/server/src/lib/sdk-options.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/enhance-prompt/routes/enhance.tsapps/server/src/providers/claude-provider.tsapps/server/src/routes/features/routes/generate-title.tsapps/server/src/lib/sdk-options.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/views/settings-view/mcp-servers/mcp-servers-section.tsxapps/ui/src/app.tsx
📚 Learning: 2025-12-28T05:07:48.147Z
Learnt from: CR
Repo: AutoMaker-Org/automaker PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-28T05:07:48.147Z
Learning: Frontend UI must use TanStack Router for file-based routing, organize components in components/views/, implement stores with Zustand, and use custom hooks in hooks/ directory
Applied to files:
apps/ui/src/app.tsx
🪛 GitHub Actions: Format Check
start.mjs
[error] 1-1: Prettier formatting check failed in 'start.mjs'. Run 'prettier --write' to fix code style issues.
🪛 LanguageTool
TODO.md
[grammar] ~8-~8: Ensure spelling is correct
Context: ...t should just be one page. I don't need nessa tabs and all these nested buttons. It's...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~9-~9: Use a hyphen to join words.
Context: ... as it's going through if there's any to do items we can see those update live - ...
(QB_NEW_EN_HYPHEN)
[grammar] ~10-~10: Use a hyphen to join words.
Context: ...e first thing I should see when I double click the card. - I went away to mass ed...
(QB_NEW_EN_HYPHEN)
[grammar] ~12-~12: Use a hyphen to join words.
Context: ... the configuration of all them. - Double check and debug if there's memory leaks....
(QB_NEW_EN_HYPHEN)
⏰ 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 (10)
apps/server/tests/unit/lib/settings-helpers.test.ts (1)
2-2: Removal of getMCPPermissionSettings is consistent and complete.The function has been properly removed from the source file (
apps/server/src/lib/settings-helpers.ts) with no remaining usages in the codebase. The import and test removal in this file aligns correctly with the source changes.package.json (1)
16-16: ✓ start.mjs exists and properly configured.The start script in package.json correctly references
start.mjs, which is present at the repository root and contains comprehensive production launch logic. The script handles dependency installation, builds shared packages and server, launches web or Electron modes with port management, and includes proper cleanup handling.apps/ui/src/hooks/use-settings-migration.ts (1)
340-340: AppStore state type correctly excludes removed MCP permission fields. The fieldsmcpAutoApproveToolsandmcpUnrestrictedToolshave been removed from the AppStore state type definition, and no remaining references exist in the codebase. TheloadMCPServersFromServerfunction's update to only setmcpServersis consistent with this type change.apps/server/src/routes/features/routes/generate-title.ts (1)
99-101: Autonomous mode change looks appropriate for this endpoint.Since this endpoint uses
allowedTools: [](no tools), thebypassPermissionsflag has minimal security implications. The title generation is a simple text completion task that doesn't require tool access.apps/server/src/routes/enhance-prompt/routes/enhance.ts (1)
167-169: Autonomous mode change is appropriate here.Similar to the title generation endpoint, this enhancement endpoint uses
allowedTools: [], making the permission bypass safe for this text transformation use case.apps/ui/src/components/views/settings-view/mcp-servers/mcp-servers-section.tsx (1)
4-4: Clean removal of MCP permission settings UI.The import and component usage have been updated correctly to remove the MCPPermissionSettings component. This aligns with the PR's goal of shifting to autonomous mode.
apps/server/src/providers/claude-provider.ts (1)
66-86: Significant security posture change - verify this is intentional.This change unconditionally bypasses all permissions and allows dangerous operations. When MCP servers are configured, tools are also unrestricted. This is a substantial shift from the previous conditional permission handling.
Please ensure this security posture change is:
- Intentional and documented in project security guidelines
- Communicated to users who may rely on permission prompts as a safety mechanism
- Acceptable for all deployment scenarios (especially multi-tenant or shared environments)
apps/ui/src/components/views/settings-view/mcp-servers/hooks/use-mcp-servers.ts (1)
24-24: Clean removal of MCP permission state from hook.The destructuring has been updated correctly to remove the unused
mcpAutoApproveToolsandmcpUnrestrictedToolsstate and setters, consistent with the autonomous mode changes.apps/server/src/lib/sdk-options.ts (2)
255-264: Autonomous mode implemented at the base level.The base options now set
bypassPermissionsandallowDangerouslySkipPermissionsby default, which propagates to all SDK option builders. This is a clean centralized approach.
289-307: Tool restriction logic is clear and well-documented.The simplified logic where
shouldRestrictTools = !hasMcpServersis easy to understand: when MCP servers are configured, the agent has access to all tools; otherwise, it's restricted to the preset tools. This aligns with the autonomous mode design.
| @@ -0,0 +1,722 @@ | |||
| #!/usr/bin/env node | |||
There was a problem hiding this comment.
Prettier formatting check failed.
The pipeline indicates a formatting issue in this file. Run prettier --write start.mjs to fix code style issues before merging.
🧰 Tools
🪛 GitHub Actions: Format Check
[error] 1-1: Prettier formatting check failed in 'start.mjs'. Run 'prettier --write' to fix code style issues.
🤖 Prompt for AI Agents
In start.mjs around lines 1 to 1, Prettier formatting failed for the file; run
the formatter (e.g. prettier --write start.mjs) to apply the project's Prettier
rules, review the changes, save and commit the reformatted file (git add
start.mjs && git commit -m "chore: format start.mjs with Prettier") so the
pipeline passes.
| await new Promise((resolve) => { | ||
| electronProcess.on('close', () => { | ||
| // Also kill vite preview when electron closes | ||
| if (webProcess && !webProcess.killed && webProcess.pid) { | ||
| killProcessTree(webProcess.pid); | ||
| } | ||
| resolve(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Await killProcessTree before resolving the promise.
The killProcessTree call is not awaited, so the promise resolves before the web process is fully terminated.
Proposed fix
await new Promise((resolve) => {
- electronProcess.on('close', () => {
+ electronProcess.on('close', async () => {
// Also kill vite preview when electron closes
if (webProcess && !webProcess.killed && webProcess.pid) {
- killProcessTree(webProcess.pid);
+ await killProcessTree(webProcess.pid);
}
resolve();
});
});📝 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.
| await new Promise((resolve) => { | |
| electronProcess.on('close', () => { | |
| // Also kill vite preview when electron closes | |
| if (webProcess && !webProcess.killed && webProcess.pid) { | |
| killProcessTree(webProcess.pid); | |
| } | |
| resolve(); | |
| }); | |
| }); | |
| await new Promise((resolve) => { | |
| electronProcess.on('close', async () => { | |
| // Also kill vite preview when electron closes | |
| if (webProcess && !webProcess.killed && webProcess.pid) { | |
| await killProcessTree(webProcess.pid); | |
| } | |
| resolve(); | |
| }); | |
| }); |
🤖 Prompt for AI Agents
In start.mjs around lines 700 to 708, the close handler resolves the awaiting
Promise before the web process tree is actually terminated because
killProcessTree is not awaited; make the close callback async (or return a
Promise) and await killProcessTree(webProcess.pid) before calling resolve (only
when webProcess exists and has a pid), so the outer await only finishes once
killProcessTree completes.
- Added a new script (dev.mjs) to start the application in development mode with hot reloading using Vite. - The script includes functionality for installing Playwright browsers, resolving port configurations, and launching either a web or desktop application. - Removed the old init.mjs script, which was previously responsible for launching the application. - Updated package.json to reference the new dev.mjs script for the development command. - Introduced a shared utilities module (launcher-utils.mjs) for common functionalities used in both development and production scripts.
- Revised instructions for starting Automaker, changing from `npm run dev` to `npm run start` for production mode. - Added a setup wizard for authentication on first run, with options for using Claude Code CLI or entering an API key. - Clarified development mode instructions, emphasizing the use of `npm run dev` for live reload and hot module replacement.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (3)
start.mjs (3)
1-1: Prettier formatting check failed.The pipeline indicates a formatting issue in this file. Run
prettier --write start.mjsto fix code style issues before merging.
163-166: Missingawaitbeforecleanup()in error path.When the server fails to start,
cleanup()is called withoutawait, which means the process could exit before child processes are properly terminated.Proposed fix
if (!processes.server) { - cleanup(); + await cleanup(); process.exit(1); }
241-249: AwaitkillProcessTreebefore resolving the promise.The
killProcessTreecall is not awaited, so the promise resolves before the web process is fully terminated.Proposed fix
await new Promise((resolve) => { - processes.electron.on('close', () => { + processes.electron.on('close', async () => { // Also kill vite preview when electron closes if (processes.web && !processes.web.killed && processes.web.pid) { - killProcessTree(processes.web.pid); + await killProcessTree(processes.web.pid); } resolve(); }); });
🧹 Nitpick comments (4)
.husky/pre-commit (2)
3-22: Consider whether this complexity is necessary.The script adds significant complexity to handle nvm detection (Herd vs standard), environment setup, and path management. While thorough, this might be overkill for a pre-commit hook. Most development environments already have
nodeandnpmin their PATH.If this complexity is needed to solve a specific issue encountered by the team, consider adding comments explaining the problem being solved.
15-15: Silent error suppression could hide real issues.Both lines intentionally suppress errors with
2>/dev/nullfor optional nvm loading. While this is reasonable for making nvm optional, it could hide legitimate configuration errors or broken nvm installations.Consider whether you want to log warnings (to stderr) when nvm setup fails unexpectedly, or document that errors are intentionally suppressed for optional behavior.
Also applies to: 18-18
start.mjs (1)
220-221: Consider more robust readiness check instead of fixed sleep.The hardcoded 2-second sleep assumes the Vite preview server will be ready, but this may be insufficient on slower systems or unnecessarily long on faster ones. Consider checking for server readiness (e.g., polling for successful HTTP connection) or capturing stdout to detect the "ready" message.
scripts/launcher-utils.mjs (1)
285-299: Consider logging or returning kill status.The function silently swallows all errors from
treeKill, which means callers have no way to know if the process was actually terminated. Consider logging errors or returning a boolean to indicate success/failure.🔎 Suggested improvement
export function killProcessTree(pid) { return new Promise((resolve) => { if (!pid) { resolve(); return; } treeKill(pid, 'SIGTERM', (err) => { if (err) { + log(`Warning: SIGTERM failed for PID ${pid}, trying SIGKILL`, 'yellow'); treeKill(pid, 'SIGKILL', () => resolve()); } else { resolve(); } }); }); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.husky/pre-commitdev.mjsinit.mjspackage.jsonscripts/launcher-utils.mjsstart.mjs
💤 Files with no reviewable changes (1)
- init.mjs
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🧰 Additional context used
🧬 Code graph analysis (1)
scripts/launcher-utils.mjs (2)
dev.mjs (8)
require(31-31)crossSpawn(32-32)resolvePortConfiguration(88-88)choice(99-99)processes(41-45)cleanup(94-94)cleanup(180-180)fs(38-38)start.mjs (6)
resolvePortConfiguration(134-134)choice(145-145)processes(46-50)cleanup(140-140)cleanup(261-261)fs(43-43)
🪛 GitHub Actions: Format Check
scripts/launcher-utils.mjs
[warning] 1-1: Code style issues found by Prettier. Run 'prettier --write' to fix.
⏰ 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
| #!/usr/bin/env sh | ||
|
|
||
| # Try to load nvm if available (optional - works without it too) | ||
| if [ -z "$NVM_DIR" ]; then | ||
| # Check for Herd's nvm first (macOS with Herd) | ||
| if [ -s "$HOME/Library/Application Support/Herd/config/nvm/nvm.sh" ]; then | ||
| export NVM_DIR="$HOME/Library/Application Support/Herd/config/nvm" | ||
| # Then check standard nvm location | ||
| elif [ -s "$HOME/.nvm/nvm.sh" ]; then | ||
| export NVM_DIR="$HOME/.nvm" | ||
| fi | ||
| fi | ||
|
|
||
| # Source nvm if found (silently skip if not available) | ||
| [ -n "$NVM_DIR" ] && [ -s "$NVM_DIR/nvm.sh" ] && \. "$NVM_DIR/nvm.sh" 2>/dev/null | ||
|
|
||
| # Load node version from .nvmrc if using nvm (silently skip if nvm not available) | ||
| [ -f .nvmrc ] && command -v nvm >/dev/null 2>&1 && nvm use >/dev/null 2>&1 | ||
|
|
||
| # Ensure common system paths are in PATH (for systems without nvm) | ||
| # This helps find node/npm installed via Homebrew, system packages, etc. | ||
| export PATH="$PATH:/usr/local/bin:/opt/homebrew/bin:/usr/bin" | ||
|
|
||
| # Run lint-staged - works with or without nvm | ||
| # Prefer npx, fallback to npm exec, both work with system-installed Node.js | ||
| if command -v npx >/dev/null 2>&1; then | ||
| npx lint-staged | ||
| elif command -v npm >/dev/null 2>&1; then | ||
| npm exec -- lint-staged | ||
| else | ||
| echo "Error: Neither npx nor npm found in PATH." | ||
| echo "Please ensure Node.js is installed (via nvm, Homebrew, system package manager, etc.)" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
This change appears unrelated to the PR's stated objective.
The PR title is "Fix memory leak" and the summary mentions removing MCP governance flags and UI memory-leak mitigation. However, this file introduces a complex nvm detection and Node.js environment setup wrapper for the pre-commit hook, which seems entirely unrelated to the core objective.
Consider moving this to a separate PR for better change isolation and traceability, or clarify how this relates to the memory leak fix.
🤖 Prompt for AI Agents
.husky/pre-commit lines 1-34: This pre-commit hook change adds an extensive
nvm/Node.js environment detection and PATH modifications that are unrelated to
the PR title "Fix memory leak"; separate unrelated infrastructure changes into
their own PR or justify their inclusion. To fix: remove this file change from
the current branch (restore previous .husky/pre-commit) and open a separate PR
that documents why the nvm wrapper is needed, or add a short note in this PR
explaining how the hook change is required for the memory-leak work and link
relevant CI/dev environments; ensure the memory-leak PR only contains the
leak-related code and tests.
…ment - Changed permissionMode settings in enhance and generate title routes to improve edit acceptance and default behavior. - Refactored performance measurement cleanup in the App component to only execute in development mode, preventing unnecessary operations in production. - Simplified the startServerAndWait function signature for better readability.
…nd tests - Changed permissionMode from 'default' to 'bypassPermissions' in sdk-options and claude-provider unit tests. - Added allowDangerouslySkipPermissions flag in claude-provider test to enhance permission handling.
- Added .claude/hans/ to .gitignore to prevent tracking of specific directory. - Updated cleanup calls in dev.mjs and start.mjs to use await for proper asynchronous handling. - Enhanced error handling during cleanup in case of failures. - Improved server failure handling in startServerAndWait function to ensure proper termination of failed processes.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
start.mjs (2)
1-1: Prettier formatting issue previously flagged.The CI pipeline has flagged formatting issues in this file. Run
prettier --write start.mjsto fix.
241-249: Missing await on killProcessTree in electron close handler.The close handler resolves before the web process tree is fully terminated because
killProcessTreeis not awaited.
🧹 Nitpick comments (1)
start.mjs (1)
221-221: Consider adding health check instead of fixed sleep.The fixed 2-second sleep works but could be more robust with a retry-based health check on the vite preview server, similar to the server health check pattern.
💡 Optional improvement with health check
You could create a simple health check for the vite preview server:
- // Wait for vite preview to start - await sleep(2000); + // Wait for vite preview to be ready + log('Waiting for preview server...', 'yellow'); + let previewReady = false; + for (let i = 0; i < 10; i++) { + try { + const response = await fetch(`http://localhost:${webPort}`); + if (response.ok || response.status === 404) { + previewReady = true; + break; + } + } catch { + // Server not ready yet + } + await sleep(500); + } + if (!previewReady) { + log('Warning: Preview server may not be ready', 'yellow'); + }
📜 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 (8)
.gitignoreapps/server/src/routes/features/routes/generate-title.tsapps/server/tests/unit/lib/sdk-options.test.tsapps/server/tests/unit/providers/claude-provider.test.tsapps/ui/src/app.tsxdev.mjsscripts/launcher-utils.mjsstart.mjs
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/ui/src/app.tsx
- dev.mjs
- scripts/launcher-utils.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/tests/unit/providers/claude-provider.test.tsapps/server/src/routes/features/routes/generate-title.tsapps/server/tests/unit/lib/sdk-options.test.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/tests/unit/providers/claude-provider.test.tsapps/server/src/routes/features/routes/generate-title.tsapps/server/tests/unit/lib/sdk-options.test.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/features/routes/generate-title.ts
🧬 Code graph analysis (1)
start.mjs (1)
scripts/launcher-utils.mjs (18)
createRestrictedFs(50-77)log(88-90)runNpmAndWait(142-151)printHeader(483-488)ensureDependencies(648-659)resolvePortConfiguration(362-438)printModeMenu(493-501)createCleanupHandler(512-532)setupSignalHandlers(538-550)choice(387-389)prompt(337-349)startServerAndWait(561-637)runNpx(160-172)webPort(371-371)serverPort(372-372)sleep(310-312)corsOriginEnv(433-433)killProcessTree(285-299)
⏰ 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 (8)
.gitignore (1)
84-84: Add context for the new ignored directory.The addition of
.claude/hans/to the gitignore is consistent with the existing.claude/pattern on line 83. However, the PR title indicates this PR is for "Fix memory leak," and it's unclear how ignoring this directory relates to the memory leak fix described in the AI summary (PerformanceMeasure cleanup, removal of MCP permission settings, etc.).Please clarify: Is
.claude/hans/directory cleanup related to the memory leak fix, or is it an incidental project cleanup that should be separated into a distinct commit or issue?apps/server/src/routes/features/routes/generate-title.ts (1)
92-101: LGTM - Security concern properly addressed.The change to
permissionMode: 'default'correctly addresses the security concern raised in the previous review. Since this is a text-only operation withallowedTools: []andmaxTurns: 1, the default permission mode is appropriate and safe. There are no file system or shell operations that would require elevated permissions.apps/server/tests/unit/lib/sdk-options.test.ts (1)
237-237: LGTM! Test expectation correctly reflects autonomous mode behavior.The updated expectation for
permissionMode: 'bypassPermissions'correctly aligns with the PR's shift to autonomous mode that unconditionally bypasses permissions when MCP servers exist.apps/server/tests/unit/providers/claude-provider.test.ts (1)
76-77: LGTM! Test expectations align with autonomous mode changes.The test correctly expects both
permissionMode: 'bypassPermissions'andallowDangerouslySkipPermissions: true, which reflects the new autonomous mode behavior where permissions are bypassed by default when MCP servers are configured.start.mjs (4)
56-116: LGTM! Well-structured build orchestration.The build logic correctly ensures packages and server are always rebuilt while optimizing by only building UI/Electron when missing. Error handling is appropriate.
163-166: LGTM! Cleanup properly awaited on server startup failure.The error path correctly awaits cleanup before exiting, ensuring processes are terminated.
147-191: LGTM! Web application mode implementation is sound.The production web mode correctly starts the compiled server, uses vite preview for static file serving, and manages processes appropriately.
259-268: LGTM! Error handler properly awaits cleanup.The global error handler correctly awaits cleanup with proper error handling before exiting. This ensures child processes are terminated even if cleanup itself encounters errors.
- Introduced a new option to launch the application in a Docker container (Isolated Mode) from the main menu. - Added checks for the ANTHROPIC_API_KEY environment variable to ensure proper API functionality. - Updated process management to include Docker, allowing for better cleanup and handling of spawned processes. - Enhanced user prompts and logging for improved clarity during the launch process.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Removed Features
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.