refactor: use environment variables for git configuration in test rep… #336
Conversation
…ositories - Updated test repository creation functions to utilize environment variables for git author and committer information, preventing modifications to the user's global git configuration. - This change enhances test isolation and ensures consistent behavior across different environments.
|
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. 📝 WalkthroughWalkthroughReplace repo-level git config in tests with env-driven GIT_AUTHOR_/GIT_COMMITTER_* vars; add cached server URL init and synchronous getter; enable dynamic port discovery at startup; add auto-mode consecutive-failure tracking and pause events; change sandbox-mode default to false; add versioning, types, error-classification, tests, docs, and templates. Changes
Sequence Diagram(s)%%{init: {"themeVariables":{"primaryColor":"#E6F4FF","secondaryColor":"#F3F7FB","tertiaryColor":"#F3F7FB","actorBorder":"#2B6CB0"}}}%%
sequenceDiagram
participant Renderer as UI Renderer
participant IPC as Electron IPC
participant Main as Electron Main
participant Backend as Backend Server
Note over Main,Backend: Startup discovers ports and starts servers
Renderer->>IPC: initServerUrl()
IPC->>Main: request server URL
Main->>Backend: health probe (using discovered serverPort)
Backend-->>Main: respond OK + base URL
Main-->>IPC: return server URL
IPC-->>Renderer: resolve initServerUrl with URL
Renderer->>Renderer: cache URL (cachedServerUrl)
Renderer->>Backend: API/WebSocket connections use getServerUrlSync()
Backend-->>Renderer: API responses / websocket events
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
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 introduces a significant improvement to the testing infrastructure by modifying how Git author and committer details are set up in temporary test repositories. Instead of altering the global Git configuration, the system now utilizes environment variables for these settings. This ensures that tests run in a completely isolated environment, preventing any side effects on the developer's local Git setup and promoting greater consistency in test execution. 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 correctly refactors test repository creation to use environment variables for git author information, which improves test isolation by not modifying the user's global git configuration. The implementation is mostly correct, but I've identified a couple of areas for improvement. There's a case where a git commit is still made without the necessary environment variables, which could cause test failures. Additionally, there's some code duplication that could be refactored for better maintainability. My detailed comments are below.
| // Don't set git config - use environment variables in commit operations instead | ||
| // to avoid affecting user's git config | ||
| // Intentionally skip creating an initial commit |
There was a problem hiding this comment.
While the intention to remove local git configuration is correct, the underlying code path this test exercises appears to be missing the environment variable setup for git commands. The test now calls createCreateHandler, which in turn calls ensureInitialCommit. The ensureInitialCommit function (in apps/server/src/routes/worktree/common.ts) executes a git commit command without the necessary GIT_AUTHOR_... environment variables. This means the test will fail on systems without a global git configuration, which is the exact problem this PR aims to solve. Please ensure that all git commit calls executed within tests use the environment variables.
| const gitEnv = { | ||
| ...process.env, | ||
| GIT_AUTHOR_NAME: 'Test User', | ||
| GIT_AUTHOR_EMAIL: 'test@example.com', | ||
| GIT_COMMITTER_NAME: 'Test User', | ||
| GIT_COMMITTER_EMAIL: 'test@example.com', | ||
| }; |
There was a problem hiding this comment.
This gitEnv object is duplicated in apps/ui/tests/utils/git/worktree.ts (in both createTestGitRepo and commitFile functions). To improve maintainability and ensure consistency, consider extracting this object into a shared constant within a common test utility file. This would avoid repetition and make future updates easier.
- Replaced the existing PNG logo with a new SVG version for improved scalability and quality. - Added the SVG logo file to the project, enhancing visual consistency across different display resolutions.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/ui/tests/utils/git/worktree.ts (1)
86-97: LGTM! Environment-based git configuration improves test isolation.The use of environment variables instead of modifying git config is the correct approach for test isolation. However, note that the
gitEnvobject is duplicated in thecommitFilefunction below (lines 260-266).🔎 Consider extracting gitEnv to a shared constant
To follow DRY principles, consider extracting the
gitEnvobject to a module-level constant:+const TEST_GIT_ENV = { + ...process.env, + GIT_AUTHOR_NAME: 'Test User', + GIT_AUTHOR_EMAIL: 'test@example.com', + GIT_COMMITTER_NAME: 'Test User', + GIT_COMMITTER_EMAIL: 'test@example.com', +}; + export async function createTestGitRepo(tempDir: string): Promise<TestRepo> { // ... existing code ... - const gitEnv = { - ...process.env, - GIT_AUTHOR_NAME: 'Test User', - GIT_AUTHOR_EMAIL: 'test@example.com', - GIT_COMMITTER_NAME: 'Test User', - GIT_COMMITTER_EMAIL: 'test@example.com', - }; - - await execAsync('git add .', { cwd: tmpDir, env: gitEnv }); - await execAsync('git commit -m "Initial commit"', { cwd: tmpDir, env: gitEnv }); + await execAsync('git add .', { cwd: tmpDir, env: TEST_GIT_ENV }); + await execAsync('git commit -m "Initial commit"', { cwd: tmpDir, env: TEST_GIT_ENV });Apply similar changes to the
commitFilefunction.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/server/tests/integration/helpers/git-test-repo.tsapps/server/tests/integration/routes/worktree/create.integration.test.tsapps/ui/tests/utils/git/worktree.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/tests/utils/git/worktree.tsapps/server/tests/integration/routes/worktree/create.integration.test.tsapps/server/tests/integration/helpers/git-test-repo.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/tests/utils/git/worktree.tsapps/server/tests/integration/routes/worktree/create.integration.test.tsapps/server/tests/integration/helpers/git-test-repo.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: Each feature executes in an isolated git worktree created via automaker/git-utils to protect the main branch during AI agent execution
Applied to files:
apps/ui/tests/utils/git/worktree.tsapps/server/tests/integration/routes/worktree/create.integration.test.tsapps/server/tests/integration/helpers/git-test-repo.ts
🪛 GitHub Actions: Test Suite
apps/server/tests/integration/routes/worktree/create.integration.test.ts
[error] 47-47: AssertionError: expected status to not have been called, but it was called when HEAD is missing and initial commit is attempted.
⏰ 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 (1)
apps/server/tests/integration/helpers/git-test-repo.ts (1)
28-39: LGTM! Consistent environment-based approach across test files.The implementation matches the UI test utilities and correctly uses environment variables to avoid modifying git config during tests.
| // Don't set git config - use environment variables in commit operations instead | ||
| // to avoid affecting user's git config |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's read the test file to understand the test structure and expectations
cat -n apps/server/tests/integration/routes/worktree/create.integration.test.ts | head -100Repository: AutoMaker-Org/automaker
Length of output: 2489
🏁 Script executed:
# Let's also find the handler file
fd -e ts -e js routes/worktree/routes/create | grep -v testRepository: AutoMaker-Org/automaker
Length of output: 49
🏁 Script executed:
# Search for the createCreateHandler function and see how it handles git operations
rg -n "createCreateHandler|git commit|execAsync.*git" apps/server/src/routes/worktree/ -A 3 -B 2Repository: AutoMaker-Org/automaker
Length of output: 31044
🏁 Script executed:
cat -n apps/server/src/routes/worktree/routes/create.ts | head -200Repository: AutoMaker-Org/automaker
Length of output: 7533
🏁 Script executed:
# Check if ensureInitialCommit is used in create.ts
rg -n "ensureInitialCommit" apps/server/src/routes/worktree/routes/create.ts -A 2 -B 2Repository: AutoMaker-Org/automaker
Length of output: 459
🏁 Script executed:
# Compare environment variable handling in create.ts vs other handlers
cat -n apps/server/src/routes/worktree/routes/create.ts | grep -A 5 -B 5 "execAsync"Repository: AutoMaker-Org/automaker
Length of output: 1728
🏁 Script executed:
cat -n apps/server/src/routes/worktree/common.ts | sed -n '162,180p'Repository: AutoMaker-Org/automaker
Length of output: 848
🏁 Script executed:
# Check if there's a version that accepts env vars
rg -n "ensureInitialCommit" apps/server/src/routes/worktree/common.ts -A 15 | head -30Repository: AutoMaker-Org/automaker
Length of output: 767
🏁 Script executed:
# Verify the test is actually checking the failure condition
rg -n "GIT_AUTHOR\|GIT_COMMITTER" apps/server/tests/ -A 2 -B 2Repository: AutoMaker-Org/automaker
Length of output: 49
Fix incomplete environment variable implementation in initial commit creation.
The handler calls ensureInitialCommit(projectPath) at line 103, but this function does not accept or pass environment variables to the git commit operation. At apps/server/src/routes/worktree/common.ts:168, the execAsync call only includes { cwd: repoPath } without the required git environment variables (GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL, GIT_COMMITTER_NAME, GIT_COMMITTER_EMAIL).
Without these variables, git commit fails with a configuration error, causing the handler to call res.status(500), which violates the test expectation at line 47.
Modify ensureInitialCommit to accept environment variables as a parameter and pass them to the execAsync call, then update the handler to provide the necessary git environment variables when calling ensureInitialCommit.
🤖 Prompt for AI Agents
In apps/server/tests/integration/routes/worktree/create.integration.test.ts
around lines 18-19 and referring to the handler call at
apps/server/src/routes/worktree/common.ts line ~103 and execAsync at line ~168,
modify ensureInitialCommit to accept an env parameter and forward that env into
the execAsync call; specifically change ensureInitialCommit(repoPath) ->
ensureInitialCommit(repoPath, env) and call execAsync(..., { cwd: repoPath, env:
{ ...process.env, ...env } }) so git receives GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL,
GIT_COMMITTER_NAME and GIT_COMMITTER_EMAIL; then update the handler that invokes
ensureInitialCommit to build and pass those four GIT_* variables (from request
data or test-provided values) so initial git commit succeeds without altering
global git config.
- Introduced a new command for fetching and validating GitHub issues, allowing users to address issues directly from the command line. - Added a release command to bump the version of the application and build the Electron app, ensuring version consistency across UI and server packages. - Updated package.json files for both UI and server to version 0.7.1, reflecting the latest changes. - Implemented version utility in the server to read the version from package.json, enhancing version management across the application.
- Added versioning to global settings, enabling automatic migrations for breaking changes. - Updated default global settings to reflect the new versioning schema. - Implemented logic to disable sandbox mode for existing users during migration from version 1 to 2. - Enhanced error handling for saving migrated settings, ensuring data integrity during updates.
There was a problem hiding this comment.
Actionable comments posted: 5
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/components/views/board-view/components/kanban-card/card-actions.tsx (1)
146-161: Remove unusedhasContextandonVerifyprops, or usehasContextto gate the Resume button.The Resume button renders whenever
onResumeis provided, without checkinghasContext. Additionally, bothhasContextandonVerifyare declared in the props interface but never used in the component body—they are dead code.Either:
- Remove these unused props from
CardActionsPropsto simplify the interface, or- Use
hasContextto conditionally gate the Resume button visibility (e.g.,onResume && hasContext), since the information is being computed and passed by the parent component.
🧹 Nitpick comments (10)
.claude/commands/gh-issue.md (2)
47-59: Clarify expectations for "fix" scope and test coverage.Step 5 mentions running tests and verifying the fix, but doesn't specify:
- Whether all existing tests must pass, or just those related to the issue?
- What to do if the issue requires architectural changes or isn't easily fixable in isolation?
- How to handle cross-team dependencies or breaking changes?
Adding guidance on these edge cases would help ensure consistent behavior across different issue types.
1-67: Well-structured documentation for GitHub issue workflow.The guide is clearly organized and walks through a logical workflow: fetching the issue, validating it, implementing a fix, and reporting results. The step-by-step instructions are easy to follow.
The
gh issue viewJSON fields specified in line 23 are all valid and supported:number,title,state,author,createdAt,labels,url,body, andassigneesare all available in the gh CLI.Consider these improvements:
Context for bash commands — Lines 19 and 23 show bash commands, but don't clarify whether these should be run in the project root, a worktree, or elsewhere. A brief note about expected working directory would help.
Closed issue handling — Line 30 suggests prompting the user if the issue is closed. Consider whether this should be automatic or conditional (e.g., only proceed for bugs, not for closed feature requests).
apps/ui/src/components/views/board-view/hooks/use-board-actions.ts (3)
438-442: Inconsistent logging for currentProject validation.While the added logging and early return are good for debugging, similar checks in
handleRunFeature(Line 326) andhandleVerifyFeature(Line 411) don't include logging. Consider either:
- Adding similar logging to those functions for consistency, or
- Removing this logging if the pattern elsewhere is to fail silently
447-447: Inconsistent logging prefix.This error message includes the
[Board]prefix, but similar messages inhandleRunFeature(Line 331) andhandleVerifyFeature(Line 416) omit it. For consistency and easier log filtering, either add the prefix to all similar messages or remove it from this one.
451-456: Consider reducing log verbosity for production.The function now contains 5 console statements, which may be excessive for production code. Consider:
- Wrapping verbose logging in a debug flag check
- Reducing to only essential error cases
- Using a structured logging library that supports log levels
Lines 451-456 log parameters that are already visible in the error context if something fails, and Line 463 logs the entire result object which may contain sensitive data or create log noise.
🔎 Example with debug flag
+const DEBUG = process.env.NODE_ENV === 'development'; + const handleResumeFeature = useCallback( async (feature: Feature) => { - console.log('[Board] handleResumeFeature called for feature:', feature.id); + if (DEBUG) console.log('[Board] handleResumeFeature called for feature:', feature.id); if (!currentProject) { console.error('[Board] No current project'); return; } try { const api = getElectronAPI(); if (!api?.autoMode) { console.error('[Board] Auto mode API not available'); return; } - console.log('[Board] Calling resumeFeature API...', { - projectPath: currentProject.path, - featureId: feature.id, - useWorktrees, - }); - const result = await api.autoMode.resumeFeature( currentProject.path, feature.id, useWorktrees ); - console.log('[Board] resumeFeature result:', result); - if (result.success) { - console.log('[Board] Feature resume started successfully'); + if (DEBUG) console.log('[Board] Feature resume started successfully'); } else { console.error('[Board] Failed to resume feature:', result.error); await loadFeatures(); }Also applies to: 463-463
apps/ui/src/components/views/board-view/components/kanban-card/card-actions.tsx (1)
18-18: Consider removing unused props.The
hasContextandonVerifyprops are declared and destructured but no longer used in the component body. If these props are not needed for backward compatibility or future use, consider removing them to reduce maintenance burden and clarify the component's API.🔎 Proposed cleanup
interface CardActionsProps { feature: Feature; isCurrentAutoTask: boolean; - hasContext?: boolean; shortcutKey?: string; onEdit: () => void; onViewOutput?: () => void; - onVerify?: () => void; onResume?: () => void; onForceStop?: () => void; onManualVerify?: () => void;export function CardActions({ feature, isCurrentAutoTask, - hasContext, shortcutKey, onEdit, onViewOutput, - onVerify, onResume, onForceStop,Also applies to: 22-22, 36-36, 40-40
apps/server/src/lib/version.ts (1)
18-32: LGTM: Version reading with proper error handling.The path resolution logic is correct, and the caching mechanism improves performance. Error handling appropriately returns a fallback version.
Optional: Use secure-fs for consistency
For consistency with the rest of the codebase, consider using
@automaker/platform'sreadFileSyncwhich includes path validation:-import { readFileSync } from 'fs'; +import { readFileSync } from '@automaker/platform'; import { fileURLToPath } from 'url'; import { dirname, join } from 'path';This is not critical since the path is hardcoded and not derived from user input, but it would align with the project's security practices.
apps/server/src/routes/worktree/routes/create.ts (1)
103-110: Consider making git identity configurable.The hardcoded values
"Automaker"and"automaker@localhost"work for the initial empty commit, but you may want to consider making these configurable via environment variables or configuration file for different deployment scenarios.🔎 Example: using environment variables with fallback
- const gitEnv = { - GIT_AUTHOR_NAME: 'Automaker', - GIT_AUTHOR_EMAIL: 'automaker@localhost', - GIT_COMMITTER_NAME: 'Automaker', - GIT_COMMITTER_EMAIL: 'automaker@localhost', - }; + const gitEnv = { + GIT_AUTHOR_NAME: process.env.AUTOMAKER_GIT_NAME || 'Automaker', + GIT_AUTHOR_EMAIL: process.env.AUTOMAKER_GIT_EMAIL || 'automaker@localhost', + GIT_COMMITTER_NAME: process.env.AUTOMAKER_GIT_NAME || 'Automaker', + GIT_COMMITTER_EMAIL: process.env.AUTOMAKER_GIT_EMAIL || 'automaker@localhost', + };apps/server/src/services/auto-mode-service.ts (1)
2066-2068: Consider log verbosity for production.The added debug logging for stream handling is helpful for diagnostics, but
console.logfor every stream message (line 2106) could be verbose in production. Consider using a debug flag or log level to conditionally enable these logs.apps/ui/src/lib/http-api-client.ts (1)
484-498: Consider conditional debug logging.The enhanced WebSocket logging is useful for debugging connectivity issues, but logging every message with payload/callback details could be verbose in production. Consider gating these logs behind a debug flag or environment variable.
📜 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 (37)
.claude/commands/gh-issue.md.claude/commands/release.md.github/ISSUE_TEMPLATE/bug_report.ymlREADME.mdapps/server/package.jsonapps/server/src/lib/settings-helpers.tsapps/server/src/lib/version.tsapps/server/src/routes/health/routes/detailed.tsapps/server/src/routes/health/routes/index.tsapps/server/src/routes/worktree/common.tsapps/server/src/routes/worktree/routes/create.tsapps/server/src/services/auto-mode-service.tsapps/ui/package.jsonapps/ui/scripts/bump-version.mjsapps/ui/src/components/dialogs/board-background-modal.tsxapps/ui/src/components/layout/sidebar/components/automaker-logo.tsxapps/ui/src/components/ui/description-image-dropzone.tsxapps/ui/src/components/views/board-view.tsxapps/ui/src/components/views/board-view/components/kanban-card/card-actions.tsxapps/ui/src/components/views/board-view/dialogs/agent-output-modal.tsxapps/ui/src/components/views/board-view/hooks/use-board-actions.tsapps/ui/src/components/views/board-view/hooks/use-board-background.tsapps/ui/src/components/views/board-view/hooks/use-board-effects.tsapps/ui/src/components/views/terminal-view.tsxapps/ui/src/components/views/terminal-view/terminal-panel.tsxapps/ui/src/lib/api-fetch.tsapps/ui/src/lib/electron.tsapps/ui/src/lib/http-api-client.tsapps/ui/src/main.tsapps/ui/src/store/app-store.tsapps/ui/src/vite-env.d.tsapps/ui/vite.config.mtslibs/types/src/error.tslibs/types/src/settings.tslibs/utils/src/error-handler.tslibs/utils/src/index.tslibs/utils/tests/error-handler.test.ts
✅ Files skipped from review due to trivial changes (2)
- apps/ui/src/components/views/board-view/dialogs/agent-output-modal.tsx
- .github/ISSUE_TEMPLATE/bug_report.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- README.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/ui/src/vite-env.d.tsapps/ui/src/components/ui/description-image-dropzone.tsxapps/ui/src/store/app-store.tsapps/ui/src/lib/api-fetch.tslibs/types/src/error.tslibs/utils/src/index.tsapps/ui/src/components/dialogs/board-background-modal.tsxapps/server/src/lib/settings-helpers.tsapps/server/src/lib/version.tsapps/ui/src/components/layout/sidebar/components/automaker-logo.tsxapps/server/src/routes/health/routes/detailed.tsapps/ui/src/components/views/board-view/hooks/use-board-actions.tsapps/ui/src/components/views/terminal-view/terminal-panel.tsxlibs/utils/tests/error-handler.test.tsapps/server/src/routes/health/routes/index.tsapps/server/src/services/auto-mode-service.tsapps/ui/src/components/views/board-view/hooks/use-board-effects.tsapps/ui/src/lib/http-api-client.tsapps/server/src/routes/worktree/common.tsapps/ui/src/components/views/board-view/components/kanban-card/card-actions.tsxlibs/types/src/settings.tsapps/ui/src/components/views/board-view/hooks/use-board-background.tsapps/ui/src/components/views/board-view.tsxapps/server/src/routes/worktree/routes/create.tsapps/ui/src/lib/electron.tsapps/ui/src/components/views/terminal-view.tsxlibs/utils/src/error-handler.tsapps/ui/src/main.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/vite-env.d.tsapps/ui/src/components/ui/description-image-dropzone.tsxapps/ui/src/store/app-store.tsapps/ui/src/lib/api-fetch.tslibs/types/src/error.tslibs/utils/src/index.tsapps/ui/src/components/dialogs/board-background-modal.tsxapps/server/src/lib/settings-helpers.tsapps/server/src/lib/version.tsapps/ui/src/components/layout/sidebar/components/automaker-logo.tsxapps/server/src/routes/health/routes/detailed.tsapps/ui/src/components/views/board-view/hooks/use-board-actions.tsapps/ui/src/components/views/terminal-view/terminal-panel.tsxlibs/utils/tests/error-handler.test.tsapps/server/src/routes/health/routes/index.tsapps/server/src/services/auto-mode-service.tsapps/ui/src/components/views/board-view/hooks/use-board-effects.tsapps/ui/src/lib/http-api-client.tsapps/server/src/routes/worktree/common.tsapps/ui/src/components/views/board-view/components/kanban-card/card-actions.tsxlibs/types/src/settings.tsapps/ui/src/components/views/board-view/hooks/use-board-background.tsapps/ui/src/components/views/board-view.tsxapps/server/src/routes/worktree/routes/create.tsapps/ui/src/lib/electron.tsapps/ui/src/components/views/terminal-view.tsxlibs/utils/src/error-handler.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/lib/settings-helpers.tsapps/server/src/lib/version.tsapps/server/src/routes/health/routes/detailed.tsapps/server/src/routes/health/routes/index.tsapps/server/src/services/auto-mode-service.tsapps/server/src/routes/worktree/common.tsapps/server/src/routes/worktree/routes/create.ts
🧠 Learnings (3)
📚 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/ui/description-image-dropzone.tsxapps/ui/src/components/dialogs/board-background-modal.tsxapps/ui/src/components/layout/sidebar/components/automaker-logo.tsxapps/ui/src/components/views/terminal-view/terminal-panel.tsxapps/ui/src/components/views/board-view/components/kanban-card/card-actions.tsxapps/ui/src/components/views/board-view.tsxapps/ui/src/components/views/terminal-view.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: Each feature executes in an isolated git worktree created via automaker/git-utils to protect the main branch during AI agent execution
Applied to files:
apps/server/src/routes/worktree/routes/create.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: Applies to apps/server/src/**/*.{ts,tsx} : Use `createEventEmitter()` from `lib/events.ts` for all server operations to emit events that stream to frontend via WebSocket
Applied to files:
apps/ui/src/main.ts
🧬 Code graph analysis (19)
apps/ui/src/components/ui/description-image-dropzone.tsx (1)
apps/ui/src/lib/http-api-client.ts (1)
getServerUrlSync(70-70)
apps/ui/src/lib/api-fetch.ts (1)
apps/ui/src/lib/http-api-client.ts (1)
getServerUrlSync(70-70)
apps/ui/src/components/dialogs/board-background-modal.tsx (1)
apps/ui/src/lib/http-api-client.ts (1)
getServerUrlSync(70-70)
apps/server/src/lib/version.ts (3)
apps/ui/scripts/bump-version.mjs (3)
__filename(12-12)__dirname(13-13)packageJson(29-29)apps/ui/scripts/update-version.mjs (2)
packageJsonPath(33-33)version(15-15)libs/platform/src/secure-fs.ts (1)
readFileSync(342-348)
apps/ui/src/components/layout/sidebar/components/automaker-logo.tsx (1)
apps/ui/src/lib/utils.ts (1)
cn(5-7)
apps/server/src/routes/health/routes/detailed.ts (1)
apps/server/src/lib/version.ts (1)
getVersion(18-33)
apps/ui/src/components/views/board-view/hooks/use-board-actions.ts (1)
apps/ui/src/lib/electron.ts (1)
getElectronAPI(728-737)
apps/ui/src/components/views/terminal-view/terminal-panel.tsx (1)
apps/ui/src/lib/http-api-client.ts (1)
getServerUrlSync(70-70)
libs/utils/tests/error-handler.test.ts (2)
libs/utils/src/error-handler.ts (3)
isQuotaExhaustedError(73-118)classifyError(150-188)getUserFriendlyErrorMessage(196-219)libs/utils/src/index.ts (3)
isQuotaExhaustedError(12-12)classifyError(14-14)getUserFriendlyErrorMessage(15-15)
apps/server/src/routes/health/routes/index.ts (1)
apps/server/src/lib/version.ts (1)
getVersion(18-33)
apps/server/src/services/auto-mode-service.ts (2)
apps/server/src/lib/events.ts (1)
EventEmitter(10-13)apps/server/src/services/settings-service.ts (1)
SettingsService(104-583)
apps/ui/src/components/views/board-view/hooks/use-board-effects.ts (1)
apps/ui/src/lib/electron.ts (1)
getElectronAPI(728-737)
apps/ui/vite.config.mts (2)
apps/ui/scripts/bump-version.mjs (2)
packageJson(29-29)__dirname(13-13)init.mjs (2)
fs(52-65)__dirname(26-26)
apps/ui/src/components/views/board-view/hooks/use-board-background.ts (1)
apps/ui/src/lib/http-api-client.ts (1)
getServerUrlSync(70-70)
apps/server/src/routes/worktree/routes/create.ts (1)
apps/server/src/routes/worktree/common.ts (1)
ensureInitialCommit(164-186)
apps/ui/scripts/bump-version.mjs (2)
apps/ui/scripts/update-version.mjs (1)
packageJsonPath(33-33)libs/platform/src/secure-fs.ts (2)
readFileSync(342-348)writeFileSync(353-360)
apps/ui/src/lib/electron.ts (1)
apps/ui/src/lib/http-api-client.ts (1)
getServerUrlSync(70-70)
apps/ui/src/components/views/terminal-view.tsx (1)
apps/ui/src/lib/http-api-client.ts (1)
getServerUrlSync(70-70)
libs/utils/src/error-handler.ts (2)
libs/utils/src/index.ts (2)
isQuotaExhaustedError(12-12)extractRetryAfter(13-13)libs/types/src/error.ts (1)
ErrorType(4-11)
🪛 Biome (2.1.2)
apps/ui/scripts/bump-version.mjs
[error] 27-27: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 27-27: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 27-27: return types can only be used in TypeScript files
(parse)
🪛 GitHub Actions: Format Check
apps/ui/scripts/bump-version.mjs
[error] 27-27: SyntaxError: Unexpected token, expected "," (27:37) during format:check (prettier --check).
🪛 LanguageTool
.claude/commands/gh-issue.md
[style] ~51-~51: Consider using a different verb for a more formal wording.
Context: ...or - Check if the issue is already fixed - Look for related code that might...
(FIX_RESOLVE)
⏰ 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 (54)
apps/ui/src/store/app-store.ts (1)
978-978: Verify the behavior change is intentional.The default value for
enableSandboxModechanged fromtruetofalse. This affects new installations or users who reset their settings. Existing users with persisted state will keep their current value.The change is consistent with:
- libs/types/src/settings.ts documentation and default
- apps/server/src/lib/settings-helpers.ts fallback value
However, the PR description focuses on git test configuration and doesn't mention this sandbox mode change. Please confirm this is an intentional part of this PR.
libs/types/src/settings.ts (1)
354-354: LGTM! Documentation and default value are now consistent.The documentation update clarifies that sandbox mode defaults to false and should be enabled for additional security, replacing the previous negative framing about compatibility issues. The default value in
DEFAULT_GLOBAL_SETTINGSnow matches the documentation.Also applies to: 526-526
apps/server/src/lib/settings-helpers.ts (1)
77-77: LGTM! Server-side fallback now matches UI and types defaults.The fallback value changed from
truetofalse, completing the consistent default change across:
- UI initial state (app-store.ts)
- Shared types default (settings.ts)
- Server fallback (this file)
The existing logging on line 78 will help track which default is being used.
apps/ui/package.json (1)
3-3: LGTM! Version bump is coordinated with the server.The version bump to 0.7.1 aligns with the server package version, maintaining consistency across the UI and server components.
apps/ui/src/vite-env.d.ts (1)
12-14: LGTM! Global constant declaration is properly typed.The TypeScript declaration for
__APP_VERSION__provides type safety for the build-time injected global constant.libs/types/src/error.ts (2)
10-10: LGTM! New error type added for quota exhaustion.The
quota_exhausteddiscriminant properly extends the ErrorType union to support quota limit scenarios.
23-23: LGTM! New flag with clear documentation.The
isQuotaExhaustedboolean flag is well-documented and follows the existing pattern of boolean classifier fields in the ErrorInfo interface.apps/server/package.json (1)
3-3: LGTM! Version bump is coordinated with the UI.The version bump to 0.7.1 matches the UI package version, ensuring consistent versioning across the application.
libs/utils/src/index.ts (1)
12-12: LGTM! New error checker export is consistent.The
isQuotaExhaustedErrorexport follows the established naming pattern for error checking functions and complements the new quota exhaustion error handling.libs/utils/src/error-handler.ts (1)
66-118: LGTM! Comprehensive quota exhaustion detection.The implementation effectively detects quota/usage exhaustion scenarios through multiple pattern categories (overloaded/capacity, usage limits, billing issues, upgrade prompts). The prioritization logic correctly places
quota_exhaustedahead ofrate_limitsince it's more specific, and the user-friendly message appropriately references Auto Mode being paused and provides actionable guidance.Also applies to: 156-156, 162-164, 184-184, 207-209
libs/utils/tests/error-handler.test.ts (1)
133-180: LGTM! Comprehensive test coverage for quota exhaustion.The test suite thoroughly validates
isQuotaExhaustedErroracross all pattern categories (overloaded, usage limits, billing/credit issues, upgrade prompts) and edge cases. The integration withclassifyErrorandgetUserFriendlyErrorMessageis properly tested, confirming the flag propagates correctly and user-facing messages are appropriate.Also applies to: 228-252, 400-406
apps/ui/src/components/views/terminal-view.tsx (1)
16-16: LGTM! Centralized server URL resolution.The change correctly replaces the hardcoded fallback (
http://localhost:3008) withgetServerUrlSync(), centralizing URL resolution logic while maintaining environment variable priority. The import follows the project's alias convention.Also applies to: 276-276
apps/ui/src/lib/api-fetch.ts (1)
12-15: LGTM! Excellent consolidation of server URL logic.The refactoring successfully centralizes server URL resolution by delegating to
getServerUrlSync(), eliminating environment variable duplication across the codebase. The updated comment accurately reflects the cached URL approach.apps/ui/src/components/ui/description-image-dropzone.tsx (2)
6-6: LGTM: Centralized server URL resolution.The import of
getServerUrlSyncsupports the centralized server URL resolution pattern used throughout this PR.
95-102: LGTM: Dynamic server URL resolution.The fallback to
getServerUrlSync()whenVITE_SERVER_URLis not set aligns with the PR's goal of centralizing server URL resolution across the UI.apps/ui/src/lib/electron.ts (2)
98-98: LGTM: Centralized server URL resolution.The import of
getServerUrlSyncenables consistent server URL resolution in the Electron API client.
692-711: LGTM: Dynamic server URL for health checks.The use of
getServerUrlSync()as a fallback ensures the health check uses the correct server URL even whenVITE_SERVER_URLis not explicitly set.apps/server/src/lib/version.ts (1)
5-10: LGTM: Standard ES module setup.The ES module setup using
fileURLToPathanddirnameis the correct pattern for accessing__dirnamein ES modules.apps/server/src/routes/health/routes/index.ts (2)
6-6: LGTM: Centralized version utility.The import of
getVersionfrom the new version utility centralizes version reading across health endpoints.
8-15: LGTM: Reliable version reporting.Replacing
process.env.npm_package_versionwithgetVersion()provides a more reliable version source that reads directly frompackage.jsonrather than depending on environment variables that may not be set.apps/ui/src/components/dialogs/board-background-modal.tsx (2)
16-16: LGTM: Centralized server URL resolution.The import of
getServerUrlSyncenables dynamic server URL resolution for background image loading.
63-75: LGTM: Dynamic server URL with cache busting.The use of
getServerUrlSync()as a fallback ensures the background image URL is correctly resolved across different environments. The cache-busting parameter ensures images reload when updated.apps/ui/vite.config.mts (1)
73-75: LGTM!The
defineconfiguration correctly exposes the app version as a build-time constant, following Vite conventions.apps/server/src/routes/health/routes/detailed.ts (1)
7-7: LGTM!The refactor to use
getVersion()centralizes version management and provides better reliability with error handling and caching compared to relying onprocess.env.npm_package_version.Also applies to: 14-14
apps/ui/src/components/views/board-view/hooks/use-board-background.ts (1)
3-3: LGTM!The refactor to use
getServerUrlSync()centralizes server URL resolution, removing the hardcoded localhost fallback and improving consistency across environments.Also applies to: 27-27
apps/ui/src/components/views/terminal-view/terminal-panel.tsx (1)
43-43: LGTM!The refactor to use
getServerUrlSync()provides centralized server URL resolution for WebSocket connections, improving consistency with other UI components.Also applies to: 486-486
apps/ui/src/components/views/board-view.tsx (1)
209-209: LGTM!The addition of
featuresWithContextto theuseBoardEffectshook invocation correctly extends the hook's capabilities to track features with context, consistent with the hook interface updates mentioned in the AI summary.apps/server/src/routes/worktree/common.ts (1)
161-186: LGTM! Clean implementation of environment-based git configuration.The addition of the optional
envparameter and its merge withprocess.envat line 175 correctly enables test isolation without modifying global git config. Error handling is appropriate, and the backward-compatible signature maintains existing call sites.apps/ui/src/components/layout/sidebar/components/automaker-logo.tsx (3)
66-68: LGTM! Clean version display implementation.The version badge styling and placement are appropriate for the collapsed sidebar view.
71-123: LGTM! Well-structured expanded view with version.The restructured layout maintains the existing brand styling while cleanly adding the version display beneath the logo. The margin alignment (
ml-[38.8px]) correctly positions the version text with the icon.
10-10:__APP_VERSION__is properly declared and correctly used.The type declaration
declare const __APP_VERSION__: string;is present inapps/ui/src/vite-env.d.ts. The usage inautomaker-logo.tsxwith the typeof guard and fallback pattern is correct and will not cause TypeScript errors.apps/ui/src/components/views/board-view/hooks/use-board-effects.ts (2)
32-36: LGTM! Correct use of ref to avoid stale closures.Keeping
featuresWithContextRefsynchronized with thefeaturesWithContextstate ensures the event handler at lines 163-176 always accesses the latest value, preventing stale closure bugs.
157-181: LGTM! Proper event-driven context synchronization.This effect correctly re-checks context when features reach terminal states (
auto_mode_errororauto_mode_feature_complete), ensuring the UI reflects backend state even when the features array doesn't trigger a re-render. The use offeaturesWithContextRefprevents stale closure issues.apps/server/src/services/auto-mode-service.ts (9)
193-209: LGTM!The failure tracking constants and properties are well-designed. The 3-failure threshold within a 1-minute window is a reasonable heuristic for detecting systemic issues like quota exhaustion.
216-242: LGTM!The sliding window approach for failure tracking is well-implemented. The immediate pause for known quota/rate limit errors is a good optimization that avoids waiting for the threshold.
244-273: LGTM!The pause signaling logic correctly prevents duplicate pause events and provides informative error messages to the UI with appropriate context (failure count, error type, original message).
275-288: LGTM!The reset and success recording methods correctly manage the failure tracking state.
resetFailureTrackingproperly clears both the array and pause flag for manual restarts, whilerecordSuccessonly clears the failure history (appropriate since the loop would already be stopped if paused).
298-299: LGTM!Resetting failure tracking when the user manually starts auto mode ensures a clean slate, which is the expected UX.
589-591: LGTM!Recording success after feature completion correctly resets the consecutive failure counter, preventing false positives from isolated failures.
619-633: LGTM!The error handling integration correctly tracks failures after emitting the error event and conditionally triggers the pause mechanism. The comment explains the dual-purpose nature of this check (specific errors + generic consecutive failures).
1036-1038: LGTM!Consistent with the
executeFeaturesuccess handling - recording success after follow-up completion maintains the failure tracking state correctly.
1054-1066: LGTM!The error handling in
followUpFeaturecorrectly mirrors the pattern inexecuteFeature, ensuring consistent failure tracking across both execution paths.apps/ui/src/lib/http-api-client.ts (4)
35-51: LGTM!The server URL initialization from Electron IPC is well-structured with appropriate error handling that gracefully falls back to environment variables or defaults.
54-65: LGTM!The fallback chain (cached URL → environment variable → default) is logical and maintains backward compatibility with existing web mode deployments.
67-70: LGTM!The synchronous accessor is a clean export for components that need the URL without async handling. The name appropriately signals it won't return a promise.
131-132: LGTM!Awaiting
initServerUrl()before API key initialization ensures the server URL is available for subsequent API requests. The comment update accurately reflects the new initialization order.apps/ui/src/main.ts (8)
56-62: LGTM!The constants preserve the original default ports while allowing dynamic assignment. The mutable variables are appropriately scoped at module level for use across functions.
64-80: LGTM!The port availability check using a temporary server is a reliable technique. The
oncehandlers and explicitclose()ensure proper cleanup.
82-94: LGTM!The sequential port search with a 100-port range provides sufficient flexibility while maintaining predictable port allocation. The error handling clearly communicates when no port is available.
368-369: LGTM!The static server correctly uses the dynamically assigned port with an informative log message.
474-474: LGTM!The server process correctly receives the dynamically assigned port via the
PORTenvironment variable.
686-699: LGTM!Port discovery before server startup ensures both ports are reserved before use. The conditional logging keeps the output clean when defaults are available.
592-594: LGTM!The window loading URLs correctly use the dynamic
staticPortfor both development fallback and production modes.
902-904: LGTM!The IPC handler correctly returns the dynamically assigned server port, enabling the UI's
initServerUrl()to receive the actual port in use.
| }, | ||
| "dependencies": { | ||
| "@anthropic-ai/claude-agent-sdk": "0.1.72", | ||
| "@anthropic-ai/claude-agent-sdk": "0.1.76", |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
@anthropic-ai/claude-agent-sdk 0.1.73 0.1.74 0.1.75 0.1.76 changelog release notes breaking changes
💡 Result:
I found package/version metadata and pointers to the SDK changelog; specific release notes for 0.1.73 → 0.1.76 are published in the repository CHANGELOG and in the package registry:
- Versions and publish dates (npm/yarn metadata): 0.1.74 (Dec 19, 2025), 0.1.75 (Dec 20, 2025), 0.1.76 (Dec 22, 2025). [1]
- Official docs point to the TypeScript SDK CHANGELOG on the GitHub repo for detailed release notes and breaking-change information. I recommend checking the repository CHANGELOG.md for the exact notes and any breaking changes. [2][3]
Links:
- Package listing (yarn/npm metadata showing versions/dates). [1]
- Agent SDK docs / changelog pointer (links to the repo CHANGELOG.md). [2][3]
If you want, I can fetch the CHANGELOG.md from the TypeScript repo and extract the full release notes and any breaking changes for 0.1.73–0.1.76. Which would you prefer?
🌐 Web query:
anthropic-ai/typescript-sdk claude-agent-sdk GitHub repository CHANGELOG
💡 Result:
I found the repository and its CHANGELOG.md:
- GitHub repo (claude-agent-sdk-typescript) — contains CHANGELOG.md. [1]
- Official Agent SDK docs referencing the TypeScript SDK changelog. [2]
References:
[1] https://github.com/anthropics/claude-agent-sdk-typescript
[2] https://docs.claude.com/en/docs/agent-sdk/overview
🏁 Script executed:
curl -s https://raw.githubusercontent.com/anthropics/claude-agent-sdk-typescript/main/CHANGELOG.md | head -200Repository: AutoMaker-Org/automaker
Length of output: 5342
🏁 Script executed:
curl -s https://raw.githubusercontent.com/anthropics/claude-agent-sdk-typescript/main/CHANGELOG.md | grep -A 3 "## 0.1.7[56]"Repository: AutoMaker-Org/automaker
Length of output: 49
🏁 Script executed:
curl -s "https://registry.npmjs.org/@anthropic-ai/claude-agent-sdk" | jq '.versions | keys[-10:]' 2>/dev/null || echo "Failed to fetch npm data"Repository: AutoMaker-Org/automaker
Length of output: 187
🏁 Script executed:
curl -s "https://api.github.com/repos/anthropics/claude-agent-sdk-typescript/releases" | jq '.[] | select(.tag_name | test("0.1.7[56]|0.1.76")) | {tag_name, name, body}'Repository: AutoMaker-Org/automaker
Length of output: 49
🏁 Script executed:
curl -s "https://api.github.com/repos/anthropics/claude-agent-sdk-typescript/releases?per_page=20" | jq '.[] | {tag_name, published_at, prerelease}'Repository: AutoMaker-Org/automaker
Length of output: 345
🏁 Script executed:
curl -s "https://registry.npmjs.org/@anthropic-ai/claude-agent-sdk/0.1.75" | jq '{version, time}' 2>/dev/null && \
curl -s "https://registry.npmjs.org/@anthropic-ai/claude-agent-sdk/0.1.76" | jq '{version, time}' 2>/dev/nullRepository: AutoMaker-Org/automaker
Length of output: 150
🏁 Script executed:
cd apps/server && cat package.json | grep -A 2 -B 2 "claude-agent-sdk"Repository: AutoMaker-Org/automaker
Length of output: 223
Version 0.1.76 does not appear to be a published release.
The specified version 0.1.76 has no published timestamp in npm and no corresponding GitHub release. The latest published version is 0.1.74 (Dec 19, 2025). Versions 0.1.75 and 0.1.76 exist in npm metadata but show time: null, indicating they are not properly released.
The recent documented versions (0.1.73–0.1.74) contain only bug fixes with no breaking changes. Verify whether the version in package.json should be 0.1.74 instead, or confirm that 0.1.76 is the intended version despite not being published yet.
🤖 Prompt for AI Agents
In apps/server/package.json at line 27, the dependency
"@anthropic-ai/claude-agent-sdk": "0.1.76" points to a version that has no
published timestamp/release; either change it to the latest published stable
"0.1.74" or confirm 0.1.76 is intentionally used once it is published. To fix:
verify on npm and GitHub whether 0.1.76 is published; if not, update
package.json to "0.1.74", run your package manager to regenerate lockfile (npm
install / yarn install) and commit the lockfile changes; if 0.1.76 is intended,
wait for the upstream publish and re-run install before merging, or replace with
a valid git tag/source that matches the intended unreleased commit and document
that decision in the PR.
|
|
||
| // Remove from running features immediately to allow resume | ||
| // The abort signal will still propagate to stop any ongoing execution | ||
| this.runningFeatures.delete(featureId); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check all runningFeatures usage in the file
rg -n "runningFeatures" --type ts apps/server/src/services/auto-mode-service.tsRepository: AutoMaker-Org/automaker
Length of output: 1404
🏁 Script executed:
#!/bin/bash
# Read the relevant sections around the lines mentioned
echo "=== Lines 410-420 (executeFeature entry) ==="
sed -n '410,420p' apps/server/src/services/auto-mode-service.ts
echo -e "\n=== Lines 790-810 (stopFeature) ==="
sed -n '790,810p' apps/server/src/services/auto-mode-service.ts
echo -e "\n=== Lines 800-850 (executeFeature finally block) ==="
sed -n '800,850p' apps/server/src/services/auto-mode-service.ts | head -40Repository: AutoMaker-Org/automaker
Length of output: 2445
🏁 Script executed:
#!/bin/bash
# Get more context around executeFeature to understand the full lifecycle
echo "=== executeFeature method signature and start ==="
sed -n '410,450p' apps/server/src/services/auto-mode-service.tsRepository: AutoMaker-Org/automaker
Length of output: 1637
🏁 Script executed:
#!/bin/bash
# Look for where abortController.signal is used in the execution
echo "=== Search for abortController.signal usage ==="
rg -n "abortController.signal" --type ts apps/server/src/services/auto-mode-service.tsRepository: AutoMaker-Org/automaker
Length of output: 175
🏁 Script executed:
#!/bin/bash
# Look at the full executeFeature method to see the finally block
echo "=== Find executeFeature finally block ==="
rg -n "finally" --type ts -B 5 -A 5 apps/server/src/services/auto-mode-service.ts | head -80Repository: AutoMaker-Org/automaker
Length of output: 806
🏁 Script executed:
#!/bin/bash
# Check what the RunningFeature type contains and if it tracks resources
echo "=== Look for RunningFeature type definition ==="
rg -n "type RunningFeature\|interface RunningFeature" --type ts -A 10 apps/server/src/services/auto-mode-service.tsRepository: AutoMaker-Org/automaker
Length of output: 114
🏁 Script executed:
#!/bin/bash
# Find RunningFeature type with different patterns
echo "=== Search for RunningFeature definition ==="
rg -n "RunningFeature" --type ts apps/server/src/services/auto-mode-service.ts | grep -E "(interface|type|class)" | head -10Repository: AutoMaker-Org/automaker
Length of output: 142
🏁 Script executed:
#!/bin/bash
# Look at more context around line 2396 where abort signal is checked
echo "=== Context around abort signal check (line 2396) ==="
sed -n '2390,2410p' apps/server/src/services/auto-mode-service.tsRepository: AutoMaker-Org/automaker
Length of output: 964
🏁 Script executed:
#!/bin/bash
# Check if there are async operations between abort and finally
echo "=== Check executeFeature main flow ==="
sed -n '414,640p' apps/server/src/services/auto-mode-service.ts | grep -n -E "(await|async|try|catch|finally)" | head -30Repository: AutoMaker-Org/automaker
Length of output: 1023
🏁 Script executed:
#!/bin/bash
# Look for where RunningFeature is actually used - is cleanup just tracking or resource cleanup?
echo "=== Check if RunningFeature resources are used for cleanup ==="
rg -n "\.worktreePath\|\.branchName\|\.projectPath" --type ts apps/server/src/services/auto-mode-service.ts | head -20Repository: AutoMaker-Org/automaker
Length of output: 129
🏁 Script executed:
#!/bin/bash
# Get the full RunningFeature interface definition
echo "=== RunningFeature interface ==="
sed -n '163,175p' apps/server/src/services/auto-mode-service.tsRepository: AutoMaker-Org/automaker
Length of output: 388
🏁 Script executed:
#!/bin/bash
# Check if there are any operations in finally blocks that access the stored RunningFeature
echo "=== Check if finally blocks use the stored RunningFeature object ==="
sed -n '635,650p' apps/server/src/services/auto-mode-service.tsRepository: AutoMaker-Org/automaker
Length of output: 649
🏁 Script executed:
#!/bin/bash
# Understand the sequence: stopFeature deletes, then what happens to ongoing execution?
echo "=== Full stopFeature method ==="
sed -n '780,800p' apps/server/src/services/auto-mode-service.tsRepository: AutoMaker-Org/automaker
Length of output: 644
🏁 Script executed:
#!/antml:keyword
# Check if there's a worktree cleanup that depends on the map entry
echo "=== Search for worktree cleanup logic ==="
rg -n "removeWorktree\|deleteWorktree\|cleanup" --type ts apps/server/src/services/auto-mode-service.ts | head -20Repository: AutoMaker-Org/automaker
Length of output: 108
🏁 Script executed:
#!/bin/bash
# Check if executeFeatureWithContext (called by resumeFeature) also has finally block
echo "=== executeFeatureWithContext method (called by resumeFeature) ==="
rg -n "executeFeatureWithContext" --type ts -A 3 apps/server/src/services/auto-mode-service.ts | head -20Repository: AutoMaker-Org/automaker
Length of output: 469
🏁 Script executed:
#!/bin/bash
# Find executeFeatureWithContext definition and its finally block
echo "=== Search for executeFeatureWithContext implementation ==="
rg -n "private.*executeFeatureWithContext\|async executeFeatureWithContext" --type ts -A 50 apps/server/src/services/auto-mode-service.ts | head -100Repository: AutoMaker-Org/automaker
Length of output: 126
🏁 Script executed:
#!/bin/bash
# Check what happens if old finally deletes after new entry is added
# Look for if there's any defensive code or whether finally just blindly deletes
echo "=== Check if finally block checks if entry exists before deleting ==="
sed -n '1068,1072p' apps/server/src/services/auto-mode-service.tsRepository: AutoMaker-Org/automaker
Length of output: 209
🏁 Script executed:
#!/bin/bash
# Look at executeFeatureWithContext finally block
echo "=== Search for executeFeatureWithContext finally block ==="
sed -n '2608,2700p' apps/server/src/services/auto-mode-service.ts | grep -n -E "finally|delete.*featureId" | head -20Repository: AutoMaker-Org/automaker
Length of output: 125
🏁 Script executed:
#!/an/bash
# Get context around executeFeatureWithContext end
echo "=== executeFeatureWithContext around line 2700+ ==="
wc -l apps/server/src/services/auto-mode-service.tsRepository: AutoMaker-Org/automaker
Length of output: 169
🏁 Script executed:
#!/bin/bash
# Find the end of executeFeatureWithContext method
echo "=== Find executeFeatureWithContext end ==="
rg -n "private async executeFeatureWithContext" --type ts -A 200 apps/server/src/services/auto-mode-service.ts | tail -80Repository: AutoMaker-Org/automaker
Length of output: 2481
🏁 Script executed:
#!/bin/bash
# Check if executeFeatureWithContext adds to runningFeatures and has finally
echo "=== executeFeatureWithContext start and end ==="
sed -n '2608,2650p' apps/server/src/services/auto-mode-service.tsRepository: AutoMaker-Org/automaker
Length of output: 1388
🏁 Script executed:
#!/bin/bash
# Check if there's any entry matching to verify ownership before delete
echo "=== Check how the finally block logic handles the delete ==="
rg -n "finally.*delete\|finally" --type ts -A 3 apps/server/src/services/auto-mode-service.ts | grep -A 3 "finally"Repository: AutoMaker-Org/automaker
Length of output: 127
🏁 Script executed:
#!/bin/bash
# Find all finally blocks that delete featureId to confirm they don't guard
echo "=== All finally blocks with featureId delete ==="
rg -n -B 5 'finally' --type ts apps/server/src/services/auto-mode-service.ts | grep -A 3 "finally"Repository: AutoMaker-Org/automaker
Length of output: 246
🏁 Script executed:
#!/bin/bash
# Check if featureId is stored separately or always matches the execution context
echo "=== Verify the execution context - does featureId parameter match map entry ==="
sed -n '414,435p' apps/server/src/services/auto-mode-service.tsRepository: AutoMaker-Org/automaker
Length of output: 858
🏁 Script executed:
#!/bin/bash
# Let me trace the exact sequence - check if finally blocks have any version tracking
echo "=== Check how finally block decides what to delete ==="
sed -n '635,645p' apps/server/src/services/auto-mode-service.tsRepository: AutoMaker-Org/automaker
Length of output: 521
🏁 Script executed:
#!/bin/bash
# Verify: if old execution's finally runs while new execution is active, what gets deleted?
echo "=== Trace what happens: old finally uses featureId parameter, not the object ==="
sed -n '400,410p' apps/server/src/services/auto-mode-service.ts
echo "---"
sed -n '635,640p' apps/server/src/services/auto-mode-service.tsRepository: AutoMaker-Org/automaker
Length of output: 813
🏁 Script executed:
#!/bin/bash
# Critical check: does finally block have any guard to prevent deleting a newer entry?
echo "=== Is there a guard like checking startTime or version ==="
sed -n '630,650p' apps/server/src/services/auto-mode-service.tsRepository: AutoMaker-Org/automaker
Length of output: 746
🏁 Script executed:
#!/bin/bash
# Critical insight: verify the race condition scenario
# Old execution: abort() called, delete() called immediately
# New execution: checks has(), adds new entry
# Old execution: finally block runs and deletes by featureId
# Does this delete the NEW entry?
echo "=== Verify: finally block has no guard to check which execution it belongs to ==="
echo "The finally block at line 640 does: this.runningFeatures.delete(featureId)"
echo "The featureId is a parameter, not tied to the specific execution instance"
echo "If old execution's finally runs after new execution added an entry, it deletes the new one"
echo ""
echo "=== Check if there's any instance tracking ==="
sed -n '420,430p' apps/server/src/services/auto-mode-service.tsRepository: AutoMaker-Org/automaker
Length of output: 710
🏁 Script executed:
#!/bin/bash
# Check timing: how long is the abort signal propagation vs finally execution?
echo "=== Check if abort signal is waited for anywhere ==="
rg -n "await.*abort\|signal\.aborted" --type ts apps/server/src/services/auto-mode-service.tsRepository: AutoMaker-Org/automaker
Length of output: 119
🏁 Script executed:
#!/antml:keyword
# Look at the abort mechanism - is it synchronous or async?
echo "=== Verify: AbortController.abort() is synchronous but listeners are async ==="
echo "AbortController.abort() is synchronous (sets aborted=true)"
echo "Event listeners like signal.addEventListener('abort') are async"
echo "BUT the finally block runs when the promise settles - could be after new entry added!"Repository: AutoMaker-Org/automaker
Length of output: 354
Race condition is a critical bug—old execution can delete new execution's map entry.
When stopFeature() immediately removes the entry from runningFeatures, it allows resumeFeature() to proceed. However, the old execution's finally block (line 640) blindly calls delete(featureId) with no guard, which will delete the new execution's entry if the old execution's cleanup runs after the new entry is added.
Timeline:
stopFeature()callsabort()and deletes entry immediately (line 797)resumeFeature()passes thehas()check and callsexecuteFeature()- New execution adds its entry to the map (line 429)
- Old execution's
finallyblock runs and deletes byfeatureId(line 640) - New execution's entry is now gone from the map
The RunningFeature object includes a startTime field but the finally block never checks it. Add instance-level tracking (e.g., using startTime or a generation ID) to ensure only the current execution cleans up its own entry.
🤖 Prompt for AI Agents
In apps/server/src/services/auto-mode-service.ts around lines 429, 640 and
794-798, the race happens because stopFeature() removes the runningFeatures
entry immediately and the old execution's finally block unconditionally deletes
by featureId; modify the logic to add instance-level validation: when creating a
RunningFeature entry (line ~429) include a unique execution identifier (e.g.,
startTime or generationId) and store it on the map value, have the finally
cleanup (line ~640) read the current map entry and only delete it if the stored
identifier matches the instance's identifier, and adjust stopFeature() (lines
~794-798) to either set an “aborted” flag on the existing entry or call abort()
without deleting the map entry so that resumeFeature() cannot race—ensure all
checks use the same identifier field to guarantee only the originating execution
removes its own map entry.
| app.on('window-all-closed', () => { | ||
| // Stop the server when all windows are closed, even on macOS | ||
| // This prevents port conflicts when reopening the app | ||
| if (serverProcess && serverProcess.pid) { | ||
| console.log('[Electron] All windows closed, stopping server...'); | ||
| if (process.platform === 'win32') { | ||
| try { | ||
| execSync(`taskkill /f /t /pid ${serverProcess.pid}`, { stdio: 'ignore' }); | ||
| } catch (error) { | ||
| console.error('[Electron] Failed to kill server process:', (error as Error).message); | ||
| } | ||
| } else { | ||
| serverProcess.kill('SIGTERM'); | ||
| } | ||
| serverProcess = null; | ||
| } | ||
|
|
||
| if (staticServer) { | ||
| console.log('[Electron] Stopping static server...'); | ||
| staticServer.close(); | ||
| staticServer = null; | ||
| } | ||
|
|
||
| if (process.platform !== 'darwin') { | ||
| app.quit(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "activate" --type ts apps/ui/src/main.ts -A 5Repository: AutoMaker-Org/automaker
Length of output: 211
🏁 Script executed:
rg -n "startServer\|initServer\|serverProcess\s*=" --type ts apps/ui/src/main.ts -B 2 -A 2Repository: AutoMaker-Org/automaker
Length of output: 49
🏁 Script executed:
rg -n "function createWindow\|const createWindow" --type ts apps/ui/src/main.ts -A 20Repository: AutoMaker-Org/automaker
Length of output: 49
🏁 Script executed:
rg -n "startServer\|new.*Server\|serverProcess.*spawn\|fork" --type ts apps/ui/src/main.ts -B 2 -A 3Repository: AutoMaker-Org/automaker
Length of output: 49
🏁 Script executed:
wc -l apps/ui/src/main.tsRepository: AutoMaker-Org/automaker
Length of output: 90
🏁 Script executed:
rg -n "createWindow" --type ts apps/ui/src/main.tsRepository: AutoMaker-Org/automaker
Length of output: 152
🏁 Script executed:
rg -n "server" --type ts apps/ui/src/main.ts -i | head -30Repository: AutoMaker-Org/automaker
Length of output: 1730
🏁 Script executed:
sed -n '553,600p' apps/ui/src/main.tsRepository: AutoMaker-Org/automaker
Length of output: 1513
🏁 Script executed:
sed -n '690,730p' apps/ui/src/main.tsRepository: AutoMaker-Org/automaker
Length of output: 1286
🏁 Script executed:
sed -n '380,450p' apps/ui/src/main.tsRepository: AutoMaker-Org/automaker
Length of output: 2578
Critical bug: Servers stop but never restart on macOS.
The code stops both serverProcess and staticServer when all windows close, even on macOS where the app remains running in the dock. However, the activate event handler (line 726-730) only creates a new window—it does NOT restart the servers.
This means:
- User closes all windows on macOS → servers stop
- User clicks app icon in dock →
activatefires → new window opens - Window loads but cannot connect to stopped backend server (port 3008) or static server
The app becomes non-functional until manually restarted. Either:
- Remove server shutdown from the
window-all-closedhandler on macOS (restore previous behavior), OR - Restart servers in the
activatehandler when they are stopped
🤖 Prompt for AI Agents
In apps/ui/src/main.ts around lines 733-758, the current window-all-closed
handler stops both serverProcess and staticServer unconditionally which on macOS
leaves the app running but with backends stopped; update behavior so macOS
doesn't leave the app without servers: either (preferable) stop skipping server
shutdown on darwin (i.e., only stop servers when process.platform !== 'darwin')
OR add logic in the existing activate handler (lines ~726-730) to check if
serverProcess or staticServer are null and restart them by calling the same
startup routines used at app launch (handle spawn errors, reassign
serverProcess/staticServer, and log failures), and ensure any pid/process
cleanup and port availability checks are performed before restarting.
| @@ -1,4 +1,5 @@ | |||
| import * as path from 'path'; | |||
| import * as fs from 'fs'; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add error handling for version reading.
The synchronous file read and JSON parse lack error handling. If package.json is missing, malformed, or lacks a version field, the build will fail with an unclear error.
🔎 Proposed fix
// Read version from package.json
-const packageJson = JSON.parse(fs.readFileSync(path.resolve(__dirname, 'package.json'), 'utf-8'));
-const appVersion = packageJson.version;
+let appVersion = '0.0.0';
+try {
+ const packageJson = JSON.parse(fs.readFileSync(path.resolve(__dirname, 'package.json'), 'utf-8'));
+ appVersion = packageJson.version || '0.0.0';
+} catch (error) {
+ console.warn('[vite.config] Failed to read version from package.json:', error);
+}Also applies to: 12-14
🤖 Prompt for AI Agents
In apps/ui/vite.config.mts around lines 2 and 12-14, the synchronous
read/JSON.parse of package.json has no error handling so missing/malformed file
or absent version will crash the build; wrap the file read and JSON.parse in a
try/catch (or check fs.existsSync first), handle parse errors, and validate that
packageJson.version exists, falling back to a sensible default (e.g., "0.0.0" or
an env var) and logging a clear warning so the build continues instead of
throwing.
- Updated the bumpVersion function to use plain JavaScript by removing TypeScript type annotations, improving compatibility with non-TypeScript environments. - Cleaned up whitespace in the bump-version.mjs file for better readability.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/server/src/services/settings-service.ts (1)
131-170: Add comprehensive migration tests for version-based settings updates.The v1→v2 migration logic disables sandbox mode for existing users. Add tests to verify all migration scenarios:
- v1 settings with
enableSandboxMode: truemigrate to v2 withfalse- v1 settings with
enableSandboxMode: falseremain unchanged- v1 settings with
enableSandboxMode: undefineddefault tofalse- v2 settings do not trigger re-migration
- Migration save errors are handled gracefully and logged
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/server/src/services/settings-service.tslibs/types/src/settings.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/settings-service.tslibs/types/src/settings.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/settings-service.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/settings-service.tslibs/types/src/settings.ts
🧬 Code graph analysis (1)
libs/types/src/settings.ts (2)
libs/types/src/index.ts (3)
SETTINGS_VERSION(92-92)CREDENTIALS_VERSION(93-93)PROJECT_SETTINGS_VERSION(94-94)apps/server/src/types/settings.ts (3)
SETTINGS_VERSION(32-32)CREDENTIALS_VERSION(33-33)PROJECT_SETTINGS_VERSION(34-34)
🔇 Additional comments (5)
libs/types/src/settings.ts (3)
473-478: LGTM! Well-organized version constant definitions.Moving the version constants to the top improves readability and makes them easier to locate. The SETTINGS_VERSION bump to 2 correctly signals a breaking change (the enableSandboxMode default flip).
354-354: Clear documentation of the new default behavior.The updated comment accurately reflects the new default value of false and provides helpful security guidance.
506-506: Excellent use of version constants for maintainability.Replacing hardcoded version numbers with named constants (SETTINGS_VERSION, CREDENTIALS_VERSION, PROJECT_SETTINGS_VERSION) ensures consistency and makes future version bumps easier to manage. The enableSandboxMode default change to false aligns with the v1→v2 migration logic in the SettingsService.
Also applies to: 533-533, 543-543, 553-553
apps/server/src/services/settings-service.ts (2)
127-129: Good documentation of migration functionality.The added comment clearly communicates that version-based migrations are now part of this method's behavior.
136-143: Necessary change to support mutation during migration.Changing from
consttoletis required to allow the migration logic to update the result object. The merge logic remains intact and correct.
| // Version-based migrations | ||
| const storedVersion = settings.version || 1; | ||
| let needsSave = false; | ||
|
|
||
| // Migration v1 -> v2: Force enableSandboxMode to false for existing users | ||
| // Sandbox mode can cause issues on some systems, so we're disabling it by default | ||
| if (storedVersion < 2) { | ||
| logger.info('Migrating settings from v1 to v2: disabling sandbox mode'); | ||
| result.enableSandboxMode = false; | ||
| result.version = SETTINGS_VERSION; | ||
| needsSave = true; | ||
| } | ||
|
|
||
| // Save migrated settings if needed | ||
| if (needsSave) { | ||
| try { | ||
| await ensureDataDir(this.dataDir); | ||
| await atomicWriteJson(settingsPath, result); | ||
| logger.info('Settings migration complete'); | ||
| } catch (error) { | ||
| logger.error('Failed to save migrated settings:', error); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the settings-service.ts file to understand the full context
find . -name "settings-service.ts" -type f | head -5Repository: AutoMaker-Org/automaker
Length of output: 113
🏁 Script executed:
# Also search for PR/commit context related to this migration
git log --oneline --all | grep -i "migration\|sandbox" | head -20Repository: AutoMaker-Org/automaker
Length of output: 774
🏁 Script executed:
# Search for tests related to settings migration
fd "settings.*test\|migration.*test" --type fRepository: AutoMaker-Org/automaker
Length of output: 49
🏁 Script executed:
# Read the settings-service.ts file to see the actual implementation
cat -n ./apps/server/src/services/settings-service.ts | head -200Repository: AutoMaker-Org/automaker
Length of output: 7567
🏁 Script executed:
# Get details on the migration commit
git show 6d4ab9c --statRepository: AutoMaker-Org/automaker
Length of output: 842
🏁 Script executed:
# Search for any test files related to settings
find . -path "*/test*" -name "*settings*" -type fRepository: AutoMaker-Org/automaker
Length of output: 219
🏁 Script executed:
# Also check if there are any settings tests in common locations
fd "test|spec" --type f | grep -i settingsRepository: AutoMaker-Org/automaker
Length of output: 175
🏁 Script executed:
# Read the settings-service test file to see migration tests
cat -n ./apps/server/tests/unit/services/settings-service.test.tsRepository: AutoMaker-Org/automaker
Length of output: 25969
🏁 Script executed:
# Check the settings types file for SETTINGS_VERSION and schema
cat -n ./libs/types/src/settings.ts | head -100Repository: AutoMaker-Org/automaker
Length of output: 3664
🏁 Script executed:
# Check the SETTINGS_VERSION value
grep -n "SETTINGS_VERSION" ./libs/types/src/settings.ts | head -20Repository: AutoMaker-Org/automaker
Length of output: 226
🏁 Script executed:
# Look at the DEFAULT_GLOBAL_SETTINGS to understand enableSandboxMode default
grep -A 30 "DEFAULT_GLOBAL_SETTINGS" ./libs/types/src/settings.tsRepository: AutoMaker-Org/automaker
Length of output: 995
🏁 Script executed:
# Check if there are any other migrations or migration patterns in the codebase
grep -rn "Migration v" ./apps/server/src/services/settings-service.tsRepository: AutoMaker-Org/automaker
Length of output: 149
🏁 Script executed:
# Check the git diff for the migration commit to see full context
git show 6d4ab9c -- ./apps/server/src/services/settings-service.tsRepository: AutoMaker-Org/automaker
Length of output: 2605
Persistence failure handling needs addressing.
The forced override of enableSandboxMode = false for v1 users is intentional (per the migration commit). However, the persistence failure issue is real: if atomicWriteJson fails (lines 162-166), the migrated settings are returned but not persisted to disk. This causes the migration to re-run on every app start until the write succeeds, creating noise in logs.
Consider whether you want to:
- Rethrow the error to prevent returning unsaved migrated state
- Mark migration as attempted to prevent repeated attempts
- Accept eventual consistency as reasonable
Additionally, the migration logic lacks test coverage. Consider adding tests for the v1→v2 migration path, including failure scenarios.
🤖 Prompt for AI Agents
In apps/server/src/services/settings-service.ts around lines 145-167, the
migration sets enableSandboxMode and returns the updated settings even if
atomicWriteJson fails, causing repeated migrations on startup; change the
persistence failure handling to throw the error after logging (so callers know
save failed and you don't return an unsaved migrated state), or alternatively
set a flag on-disk/state to mark the migration as attempted before returning —
implement the chosen approach consistently, ensure the try/catch either rethrows
the error (after logger.error) or writes the migration-attempt marker
atomically, and add unit tests for the v1→v2 migration path including the
atomicWriteJson success and failure scenarios.
- Added a new function to check if a port is in use without terminating processes, improving user experience during server startup. - Updated the health check function to accept a dynamic port parameter, allowing for flexible server configurations. - Implemented user prompts for handling port conflicts, enabling users to kill processes, choose different ports, or cancel the operation. - Enhanced CORS configuration to support localhost and IPv6 addresses, ensuring compatibility across different development environments. - Refactored the main function to utilize dynamic port assignments for both the web and server applications, improving overall flexibility.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/ui/src/main.ts (1)
731-763: macOS activate handler doesn't restart stopped servers.This issue was flagged in a previous review. When all windows close on macOS, the servers are stopped (lines 741-759), but the
activatehandler (lines 731-735) only creates a new window without restarting the servers. The app becomes non-functional when re-activated from the dock.Either:
- Skip server shutdown on macOS in
window-all-closed, or- Add server restart logic to the
activatehandler
🧹 Nitpick comments (3)
apps/ui/src/main.ts (1)
766-789: Consider extracting shared cleanup logic.The server/static server shutdown logic is duplicated between
window-all-closed(lines 741-759) andbefore-quit(lines 767-788). A shared helper function would reduce duplication.🔎 Proposed refactor
+function stopServers(): void { + if (serverProcess && serverProcess.pid) { + console.log('[Electron] Stopping server...'); + if (process.platform === 'win32') { + try { + execSync(`taskkill /f /t /pid ${serverProcess.pid}`, { stdio: 'ignore' }); + } catch (error) { + console.error('[Electron] Failed to kill server process:', (error as Error).message); + } + } else { + serverProcess.kill('SIGTERM'); + } + serverProcess = null; + } + + if (staticServer) { + console.log('[Electron] Stopping static server...'); + staticServer.close(); + staticServer = null; + } +} app.on('window-all-closed', () => { - // Stop the server when all windows are closed... - if (serverProcess && serverProcess.pid) { - // ... duplicated code ... - } - if (staticServer) { - // ... duplicated code ... - } + stopServers(); if (process.platform !== 'darwin') { app.quit(); } }); app.on('before-quit', () => { - if (serverProcess && serverProcess.pid) { - // ... duplicated code ... - } - if (staticServer) { - // ... duplicated code ... - } + stopServers(); });init.mjs (2)
472-485: CORS origin handling may unintentionally remove wildcard.The CORS origin computation filters out
'*'on line 480. If a user explicitly setCORS_ORIGIN=*for development convenience, this would be silently replaced with specific origins. This might be intentional for security, but could surprise users.Consider logging when
'*'is replaced:const existing = (process.env.CORS_ORIGIN || '') .split(',') .map((o) => o.trim()) - .filter(Boolean) - .filter((o) => o !== '*'); + .filter(Boolean); + const hadWildcard = existing.includes('*'); + const filtered = existing.filter((o) => o !== '*'); + if (hadWildcard) { + log('Note: CORS_ORIGIN=* replaced with specific origins for security', 'yellow'); + } - const origins = new Set(existing); + const origins = new Set(filtered);
274-282:runNpxdoesn't merge environment likerunNpm.The
runNpmfunction was updated to properly merge custom env withprocess.env, butrunNpxwasn't updated consistently. If custom env is passed torunNpxin the future, the child process won't inherit the full environment.🔎 Proposed fix for consistency
function runNpx(args, options = {}) { + const { env, ...restOptions } = options; const spawnOptions = { stdio: 'inherit', cwd: __dirname, - ...options, + ...restOptions, + env: { + ...process.env, + ...(env || {}), + }, }; // cross-spawn handles Windows .cmd files automatically return crossSpawn('npx', args, spawnOptions); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/server/src/index.tsapps/ui/src/main.tsinit.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/index.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/index.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/index.tsapps/ui/src/main.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: AutoMaker-Org/automaker PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-28T05:07:48.138Z
Learning: Each feature executes in an isolated git worktree created via automaker/git-utils to protect the main branch during AI agent execution
📚 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 apps/server/src/**/*.{ts,tsx} : Use `createEventEmitter()` from `lib/events.ts` for all server operations to emit events that stream to frontend via WebSocket
Applied to files:
apps/ui/src/main.ts
🧬 Code graph analysis (1)
apps/ui/src/main.ts (2)
init.mjs (3)
serverPort(376-376)req(224-226)serverProcess(79-79)apps/server/src/services/dev-server-service.ts (2)
isPortAvailable(34-50)findAvailablePort(103-128)
🪛 GitHub Actions: Format Check
init.mjs
[error] 1-1: Prettier formatting check failed for 'init.mjs'. Run 'prettier --write' to fix code style issues (the command that failed: 'npm run format:check' -> 'prettier --check .').
⏰ 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/src/index.ts (2)
136-140: LGTM! IPv6 loopback support added correctly.The addition of IPv6 loopback address
[::1]properly uses bracket notation as required by RFC 3986 for IPv6 addresses in URLs. This ensures local development works seamlessly for IPv6-enabled environments.
136-140: PR title mentions git config in test repositories, but this change is CORS-related.The PR title states "refactor: use environment variables for git configuration in test repositories," but this file contains CORS enhancements for the server. While the commit messages do mention CORS changes, this inconsistency may indicate multiple unrelated changes bundled together or an incorrect PR description.
apps/ui/src/main.ts (3)
67-99: Port utilities implementation looks correct.The
isPortAvailableandfindAvailablePortfunctions are well-implemented. Usingnet.createServer()with default binding semantics correctly handles dual-stack scenarios. The 100-port search range is reasonable for development.
691-729: Dynamic port discovery at startup is well-structured.The port discovery happens early in the startup flow before servers are started. Proper logging indicates when alternative ports are used, and the error handling with user-facing dialogs is appropriate for a desktop app.
907-909: IPC handler correctly returns dynamic server URL.The
server:getUrlhandler properly uses the dynamically discoveredserverPort.init.mjs (5)
173-179: Clean non-destructive port check utility.The
isPortInUsehelper provides a simple, non-destructive way to check port availability without killing processes.
222-233: Health check correctly parameterized with sensible default.The
checkHealthfunction now accepts a configurable port while maintaining backward compatibility with the default of 3008.
255-269: Environment merging fix ensures proper inheritance.The
runNpmfunction now correctly mergesprocess.envwith custom environment variables, ensuring child processes inherit the full environment. This prevents issues where only the custom env vars would be passed.
391-470: Interactive port resolution provides good UX.The port conflict resolution flow is well-designed:
- Clear prompts with multiple options (kill, use different, cancel)
- Port validation ensures valid range (1024-65535)
- Prevents same port for web and server
- Allows users to override warnings about in-use ports
592-603: Environment propagation to Electron aligns with main.ts.The environment variables passed (
TEST_PORT,PORT,VITE_SERVER_URL,CORS_ORIGIN) correctly align with whatapps/ui/src/main.tsexpects for dynamic port configuration.
…anagement - Removed unnecessary whitespace in the init.mjs file for better readability. - Enhanced the formatting of user prompts to improve clarity during port conflict resolution.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
init.mjs (1)
483-496: Consider adding IPv6 loopback origin.The PR objectives mention "Enhanced CORS to support localhost and IPv6", but only
localhostand127.0.0.1are added here. Consider including the IPv6 loopback address for consistency:origins.add(`http://localhost:${webPort}`); origins.add(`http://127.0.0.1:${webPort}`); + origins.add(`http://[::1]:${webPort}`);Also note that filtering out
'*'(line 491) will replace a user's intentional wildcard CORS setting with specific origins. If this is intentional behavior, it may warrant a comment explaining why.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
init.mjs
⏰ 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 (7)
init.mjs (7)
173-179: LGTM!Clean helper function that provides a clear boolean API for port availability checks without side effects.
222-233: LGTM!Parameterizing the port with a sensible default maintains backward compatibility while enabling dynamic port usage.
255-269: LGTM!Correctly merges custom environment variables with
process.env. This prevents the common pitfall where passingenvto spawn options replaces the entire environment, which would break PATH resolution and other essential variables.
369-481: LGTM!Well-structured interactive flow for port conflict resolution:
- Port range validation (1024-65535) correctly avoids privileged ports
- Prevents using the same port for both web and server
- Offers clear options (kill, use different, cancel) with confirmation for overriding in-use ports
529-544: LGTM!Dynamic port and CORS configuration properly passed to the server process via environment variables.
563-591: LGTM!Health check correctly uses the dynamic
serverPort, and the web application startup properly configuresTEST_PORTandVITE_SERVER_URLto match the selected ports. The user-facing URL message accurately reflects the chosen port.
603-614: LGTM!Electron startup is well-documented and consistently passes all necessary environment variables (
TEST_PORT,PORT,VITE_SERVER_URL,CORS_ORIGIN) to support dynamic port configuration.
- Modified the application to keep the app and servers running when all windows are closed on macOS, aligning with standard macOS behavior. - On other platforms, ensured that the server processes are stopped and the app quits when all windows are closed, preventing potential port conflicts.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/ui/src/main.ts (1)
70-85: Consider binding to explicit host for accuracy.The comment on lines 81-82 claims that calling
server.listen(port)without a host parameter "avoids false-positives when a port is taken on IPv6/dual-stack," but this is potentially misleading. Node's default binding behavior varies by version and system configuration.For more accurate port availability checking that matches your actual server binding:
- If your servers bind to
localhostspecifically, check availability on'127.0.0.1'explicitly- If servers bind to all interfaces, the current approach is acceptable but the comment should be clarified
🔎 Proposed refinement for localhost-specific binding
function isPortAvailable(port: number): Promise<boolean> { return new Promise((resolve) => { const server = net.createServer(); server.once('error', () => { resolve(false); }); server.once('listening', () => { server.close(() => { resolve(true); }); }); - // Use Node's default binding semantics (matches most dev servers) - // This avoids false-positives when a port is taken on IPv6/dual-stack. - server.listen(port); + // Bind to localhost specifically to match server binding behavior + server.listen(port, '127.0.0.1'); }); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/ui/src/main.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/main.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/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 apps/server/src/**/*.{ts,tsx} : Use `createEventEmitter()` from `lib/events.ts` for all server operations to emit events that stream to frontend via WebSocket
Applied to files:
apps/ui/src/main.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
🔇 Additional comments (10)
apps/ui/src/main.ts (10)
14-14: LGTM! Clean addition for port availability checking.The
netimport is appropriately added to support the new dynamic port discovery functionality.
55-66: LGTM! Clear separation of default and runtime port values.The refactoring properly distinguishes between default port preferences (from environment or hardcoded) and the actual ports used at runtime. The comments clearly explain when these values are passed from the root init script.
91-99: LGTM! Solid port discovery implementation.The sequential search with a reasonable upper bound (100 ports) and clear error handling is appropriate for this use case.
373-374: LGTM! Static server correctly uses dynamic port.The static server now binds to the runtime-discovered
staticPortvalue with appropriate logging.
479-479: LGTM! Backend server port correctly configured via environment.The backend receives the dynamically discovered port through the
PORTenvironment variable, ensuring consistency between port discovery and actual usage.
532-532: LGTM! Health check updated for dynamic port.The health check correctly targets the runtime-assigned server port.
597-599: LGTM! Window URLs updated for dynamic ports.The window loading logic correctly references the runtime-assigned static port for both development fallback and production scenarios.
690-704: LGTM! Port discovery properly placed at startup.The port discovery logic runs at the right time (after app is ready, before starting servers) with helpful logging. While there's a theoretical TOCTOU race condition between checking availability and binding, it's unlikely in practice and would be caught by the existing server startup error handlers.
738-764: Excellent! Previous critical macOS bug is now fixed.The window-all-closed handler now correctly implements platform-specific behavior:
- macOS: Servers continue running when all windows close (standard macOS UX). When the user clicks the dock icon, the
activatehandler creates a new window that connects to the still-running servers.- Other platforms: Servers stop and app quits as expected.
This resolves the critical issue flagged in the previous review where macOS users would have non-functional app after closing and reopening windows.
908-908: LGTM! IPC handler returns dynamic server URL.The server URL handler correctly provides the runtime-assigned port to the frontend.
…ositories
Summary by CodeRabbit
Tests
Documentation
New Features
UI
Chores
✏️ Tip: You can customize this high-level summary in your review settings.