Improve workspace base routing and history project discovery#665
Draft
alicomert wants to merge 6 commits into
Draft
Improve workspace base routing and history project discovery#665alicomert wants to merge 6 commits into
alicomert wants to merge 6 commits into
Conversation
Contributor
📝 WalkthroughWalkthroughAdded Linux/systemd daemon management and a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as server/cli.js
participant PortProbe as Port Probe
participant SysMgr as Daemon Manager
participant Systemctl as systemctl
participant Service as cloudcli.service
User->>CLI: cloudcli start
CLI->>PortProbe: probe port (e.g., 3001)
PortProbe-->>CLI: port not open
CLI->>SysMgr: attempt system daemon install/start (mode=system)
SysMgr->>Systemctl: daemon-reload, enable --now cloudcli.service
Systemctl-->>SysMgr: started / active?
alt active and healthy
SysMgr-->>CLI: daemon ready
CLI-->>User: exit (daemon running)
else permission error
SysMgr-->>CLI: EACCES
CLI->>SysMgr: attempt user daemon install/start (mode=user)
SysMgr->>Systemctl: --user daemon-reload, enable --now cloudcli.service
Systemctl-->>SysMgr: started / active?
SysMgr-->>CLI: daemon ready
CLI-->>User: exit (user daemon running)
end
sequenceDiagram
participant App as Frontend
participant ProjectsAPI as server/projects.js
participant Codex as Codex sessions
participant Gemini as Gemini CLI entries
participant SessionMgr as Session Manager
participant Config as ~/.claude/project-config.json
App->>ProjectsAPI: getProjects()
ProjectsAPI->>ProjectsAPI: autoSyncTrackedProjects()
ProjectsAPI->>Codex: scan session CWDs
Codex-->>ProjectsAPI: discovered paths
ProjectsAPI->>Gemini: list entries (~/.gemini/tmp)
Gemini-->>ProjectsAPI: project roots
ProjectsAPI->>SessionMgr: query in-memory sessions
SessionMgr-->>ProjectsAPI: session sources
ProjectsAPI->>Config: merge discovered (autoDiscovered=true)
Config-->>ProjectsAPI: updated config
ProjectsAPI-->>App: return projects (manual + detected)
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (8)
src/components/project-creation-wizard/components/WorkspacePathField.tsx (1)
93-94: Localize new placeholder strings instead of hardcoding English.These new placeholders are user-facing and should come from i18n resources to keep wizard UX consistent across locales.
♻️ Suggested refactor
- placeholder={ - workspaceType === 'existing' - ? 'my-existing-project or /absolute/path' - : 'my-new-project or /absolute/path' - } + placeholder={ + workspaceType === 'existing' + ? t('wizard.workspacePath.existingPlaceholder') + : t('wizard.workspacePath.newPlaceholder') + }Add matching keys in locale files, e.g.:
wizard.workspacePath.existingPlaceholderwizard.workspacePath.newPlaceholder🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/project-creation-wizard/components/WorkspacePathField.tsx` around lines 93 - 94, The placeholder strings in WorkspacePathField are hardcoded English; replace them with i18n lookups (e.g., use the component's translation function or useTranslation hook) and add matching keys like wizard.workspacePath.existingPlaceholder and wizard.workspacePath.newPlaceholder to the locale resource files; update the conditional that currently returns 'my-existing-project or /absolute/path' / 'my-new-project or /absolute/path' to call the translation function with those keys so placeholders are localized.vite.config.js (1)
31-31: Heads-up on strict-port DX.With
strictPort: true,vitewill now exit ifVITE_PORT(default 5173) is already in use rather than falling back to the next port. This matches the daemon workflow's deterministic-port expectation, but contributors runningnpm run clientalongside another Vite instance will hit hard failures. Consider documenting this (e.g., in the README daemon section) so the failure mode is expected.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vite.config.js` at line 31, The review notes that setting strictPort: true in vite.config.js causes Vite to exit if VITE_PORT (default 5173) is in use; update the documentation to warn contributors about this behavior and how to avoid hard failures when running npm run client concurrently. Add a short note in the README daemon section (or developer/startup docs) explaining that strictPort is enabled, that VITE_PORT will not auto-increment, how to change VITE_PORT or stop other Vite instances, and include an example command to run the client on a different port or how to export VITE_PORT before starting.server/cli.js (1)
638-773: Optional: dedupe the daemon bootstrap helpers shared withserver/index.js.
isPortOpen,waitForPortOpen,printSystemDaemonActiveNotice,printUserDaemonActiveNotice,isSystemPermissionError, and the bulk ofmaybeAutoDaemonStart/maybeAutoDaemonBootstrapFromIndexare duplicated betweenserver/cli.jsandserver/index.js. They're intentionally split across two entry points (CLI wrapper vs. directnode server/index.js), but all six helpers are pure and could live in a single module (e.g.,server/utils/daemon-bootstrap.js) and be imported from both — that way a fix to the permission-error regex or the health-check timing only needs to happen once. Safe to defer; flagging so the duplication doesn't silently drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/cli.js` around lines 638 - 773, The duplicated daemon bootstrap helpers (isPortOpen, waitForPortOpen, printSystemDaemonActiveNotice, printUserDaemonActiveNotice, isSystemPermissionError) and related logic in maybeAutoDaemonStart should be extracted into a shared module and imported by both server/cli.js and server/index.js; create a new module (e.g., daemon-bootstrap) that exports those functions plus buildAutoInstallArgs and maybeAutoDaemonStart/maybeAutoDaemonBootstrapFromIndex as needed, update both files to import and call the shared functions (preserving signatures and behavior), and ensure error messages and environment checks remain identical so fixes (like the permission regex in isSystemPermissionError or timing in waitForPortOpen) are applied once.server/routes/projects.js (1)
71-95: Relative paths can traverse aboveWORKSPACES_BASEvia...
path.join(WORKSPACES_BASE, trimmedPath)normalizes..segments, sonormalizeWorkspacePath('../../etc/foo')produces a path outside the base (though it will still fail theWORKSPACES_ROOTcontainment check and/orFORBIDDEN_PATHSguards, so it is not a security hole). If the intent of "resolve short names into the base" is strict, consider rejecting inputs that start with..after normalization, or clamping relative inputs to not escapeWORKSPACES_BASE. Otherwise the subtlety is worth a code comment so future callers don't rely on "short path ⇒ under base".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes/projects.js` around lines 71 - 95, The normalizeWorkspacePath function currently joins relative inputs with WORKSPACES_BASE which allows '..' segments to escape the base; change it to reject or clamp such inputs by resolving against WORKSPACES_BASE (e.g., compute resolved = path.resolve(WORKSPACES_BASE, trimmedPath)) and then verify the resolved path is contained within WORKSPACES_BASE (or return WORKSPACES_BASE / throw) if it is not; update handling for Windows absolute detection (WINDOWS_ABSOLUTE_PATH_PATTERN) to still allow absolute paths to be resolved via path.resolve(trimmedPath) and keep special-case '~' handling.server/index.js (1)
596-666: Minor:defaultRootvsresolvedWorkspaceBasecomparison is inconsistent ifWORKSPACES_BASEis a symlink.Line 613 guards
mkdirwithresolvedPath === defaultRoot(unresolved), while line 653's "show common dirs under home" usesresolvedPath === resolvedWorkspaceBase(realpath'd). IfWORKSPACES_BASEis a symlink,validation.resolvedPathwill be the realpath'd target, so themkdirbranch won't fire (fine — it already exists) but the two branches then disagree about what "at the base" means. Not user-visible in practice, but using the realpath'd form for both keeps behavior consistent:Proposed tweak
- if (resolvedPath === defaultRoot) { + if (resolvedPath === defaultRoot || resolvedPath === (await fsPromises.realpath(defaultRoot).catch(() => defaultRoot))) { await fs.promises.mkdir(resolvedPath, { recursive: true }); }Or compute
resolvedWorkspaceBaseonce before thetryblock and reuse for both checks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/index.js` around lines 596 - 666, The code inconsistently compares unresolved WORKSPACES_BASE (defaultRoot) to validation.resolvedPath when deciding to mkdir but later compares resolvedPath to resolvedWorkspaceBase (realpath) to decide common-dir suggestions; compute resolvedWorkspaceBase (realpath of WORKSPACES_BASE) once before the access/mkdir try block and use that resolvedWorkspaceBase for both the mkdir check and the "is base" comparison, i.e. replace usages of defaultRoot in the mkdir guard and the earlier fallback validateWorkspacePath logic with the single resolvedWorkspaceBase value (retain validateWorkspacePath and resolvedPath logic) so both branches use the realpath-consistent base; update references to defaultRoot/WORKSPACES_BASE to resolvedWorkspaceBase around fs.promises.mkdir and the final resolvedPath === resolvedWorkspaceBase check.server/daemon-manager.js (1)
134-149:buildDaemonExecStartsilently picks a non-existent fallback path.
resolveFrontendDaemonEntry(lines 178-197) correctly verifies each candidate exists and throws a descriptive error otherwise.buildDaemonExecStartdoes the opposite: if the primarycliCandidateisn't found, it unconditionally usespath.join(appRoot, 'dist-server', 'server', 'cli.js')without checking whether that file exists. When it doesn't (e.g., user installed from source without running the build), install succeeds butsystemctl startlater emitsstatus=203/EXECwith no hint about what went wrong.Suggested mirror of `resolveFrontendDaemonEntry`
function buildDaemonExecStart({ appRoot, serverPort, databasePath, nodeExecPath, cliEntry }) { const nodeExec = nodeExecPath || process.execPath || 'node'; - const cliCandidate = cliEntry - ? path.resolve(cliEntry) - : (process.argv[1] ? path.resolve(process.argv[1]) : path.join(appRoot, 'dist-server', 'server', 'cli.js')); - const resolvedCliEntry = fs.existsSync(cliCandidate) - ? cliCandidate - : path.join(appRoot, 'dist-server', 'server', 'cli.js'); + const candidates = []; + if (cliEntry) candidates.push(path.resolve(cliEntry)); + if (process.argv[1]) candidates.push(path.resolve(process.argv[1])); + candidates.push(path.join(appRoot, 'dist-server', 'server', 'cli.js')); + candidates.push(path.join(appRoot, 'server', 'cli.js')); + const resolvedCliEntry = candidates.find(c => fs.existsSync(c)); + if (!resolvedCliEntry) { + throw new Error( + `Backend daemon entry was not found. Checked: ${candidates.join(', ')}. ` + + `Run "npm run build:server" or ensure server/cli.js exists.` + ); + } const args = [nodeExec, resolvedCliEntry, 'start', '--port', String(serverPort)];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/daemon-manager.js` around lines 134 - 149, buildDaemonExecStart silently falls back to appRoot/dist-server/server/cli.js without verifying it exists; update it to mirror resolveFrontendDaemonEntry by checking each candidate (cliEntry, process.argv[1], and appRoot/dist-server/server/cli.js) with fs.existsSync and if none exist throw a clear Error describing the missing CLI entry (include the attempted paths) instead of returning a non-existent path; adjust references to cliCandidate and resolvedCliEntry so the function either returns a verified path or throws so systemd failures surface as a descriptive install-time error.server/daemon/manager.js (2)
554-563:readDaemonLogsloads the entire log file into memory.For a long-running daemon with no rotation (see comment at lines 389-420), this
fs.readFileSynccan OOM or stall the event loop. For tailing the last N lines, prefer streaming from the tail:♻️ Proposed refactor — tail via file handle
- const raw = fs.readFileSync(LOG_FILE, 'utf8'); - const lineList = raw.split(/\r?\n/); - const limited = lineList.slice(Math.max(0, lineList.length - lines)).join('\n'); - return { exists: true, content: limited.trim(), logFile: LOG_FILE }; + const { size } = fs.statSync(LOG_FILE); + // Heuristic: 512 bytes/line, with an 8 KiB floor and 4 MiB ceiling. + const readBytes = Math.min(size, Math.max(8 * 1024, Math.min(lines * 512, 4 * 1024 * 1024))); + const fd = fs.openSync(LOG_FILE, 'r'); + try { + const buf = Buffer.alloc(readBytes); + fs.readSync(fd, buf, 0, readBytes, Math.max(0, size - readBytes)); + const tail = buf.toString('utf8').split(/\r?\n/); + const limited = tail.slice(Math.max(0, tail.length - lines)).join('\n'); + return { exists: true, content: limited.trim(), logFile: LOG_FILE }; + } finally { + fs.closeSync(fd); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/daemon/manager.js` around lines 554 - 563, readDaemonLogs currently calls fs.readFileSync(LOG_FILE) which can OOM or block the event loop for very large logs; change it to stream/read from the tail instead: open the LOG_FILE (use fs.open or fs.createReadStream), use fs.stat to get file size and calculate a start offset for the last N lines (or read backwards in chunks until you have N newlines), then stream only that portion into memory, assemble the final N lines, and return the same object shape ({ exists, content, logFile }). Keep the existing ensureDaemonDir() and LOG_FILE references and ensure the implementation handles the file-not-exists case and errors gracefully without blocking the event loop.
389-420: Daemon log file has no rotation or size bound.
fs.openSync(LOG_FILE, 'a')+stdio: [ignore, logFd, logFd]appends forever. A long-lived daemon with verbose logging will grow~/.cloudcli/daemon/daemon.logindefinitely, andreadDaemonLogsbelow reads the whole file into memory (see separate comment at lines 554-563).Consider one of:
- Rotate on startup when file exceeds a threshold (rename to
daemon.log.1, open fresh file).- Use a size-capped ring buffer or rely on
logrotate/newsyslogwith documentation.- Truncate to last N bytes at startup.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/daemon/manager.js` around lines 389 - 420, The daemon log is opened with fs.openSync(LOG_FILE, 'a') and will grow unbounded; change the startup behavior in manager.js to rotate or truncate the log before opening it: introduce a LOG_ROTATE_THRESHOLD (e.g. bytes) and, before calling fs.openSync(LOG_FILE, 'a'), check fs.statSync(LOG_FILE).size and if it exceeds the threshold rename LOG_FILE to LOG_FILE + '.1' (fs.renameSync) or truncate to the last N bytes (read file tail and overwrite) so a fresh file is opened for the spawned child (spawn(..., stdio: ['ignore', logFd, logFd])). Update any code that reads the log (readDaemonLogs) to handle rotated files or to only read the tail to avoid loading the whole file into memory. Ensure these changes occur around the existing LOG_FILE open and spawn block and keep writePidFile/writeStateFile behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.env.example:
- Around line 29-31: The .env.example documents SHELL_SESSION_TIMEOUT_MINUTES
but server/index.js hardcodes PTY_SESSION_TIMEOUT; update server/index.js to
read process.env.SHELL_SESSION_TIMEOUT_MINUTES, parse it as an integer with a
default of 30, compute PTY_SESSION_TIMEOUT = minutes * 60 * 1000 and use that
value where the existing PTY_SESSION_TIMEOUT constant and setTimeout are used
(references: PTY_SESSION_TIMEOUT constant and the setTimeout call around the
shell session cleanup logic). Ensure invalid or missing env values fall back to
30 minutes and the parsed value is used consistently, or alternatively remove
the SHELL_SESSION_TIMEOUT_MINUTES lines from .env.example if you prefer not to
support runtime configuration.
In `@package.json`:
- Around line 27-30: The current "dev" npm script runs "server:dev" which calls
"daemon install --mode system" causing Linux-only, sudo-required behavior and
re-install on every run; change the dev entrypoint to a cross-platform,
iterative dev flow: update "dev" to run the frontend and backend in
watch/restart mode (use "server:dev-watch" instead of "server:dev"), or
implement a guard around "daemon install" so it runs once (e.g., a bootstrap
script that checks for existing unit/linger) and then uses "daemon restart" for
subsequent runs; also change the daemon mode flag from "--mode system" to a
non-privileged default like "--mode auto" or "--mode user" to avoid requiring
elevated privileges on developers' machines, and ensure any new bootstrap script
references the existing vite-daemon.js behavior rather than invoking
ensureSystemctl directly.
In `@server/daemon-manager.js`:
- Around line 613-617: The warning message prints unconditionally for install
with --mode=auto even when nothing fell back; update the condition around the
console.log in the switch for parsed.subcommand 'install' so it only logs when
auto would have selected user but the user bus is down: i.e., require
parsed.options.mode === 'auto' && mode === 'system' && userUnitInstalled &&
!userBus.ok (reference parsed.subcommand, parsed.options.mode, mode, userBus.ok,
userUnitInstalled and resolveDaemonMode) so the message is only shown when a
real fallback occurred.
- Around line 525-601: The code currently throws when mode === 'user' &&
!userBus.ok which prevents the doctor path from running; update the guard in the
top-level check (the throw for mode/user/userBus.ok) to skip throwing when
parsed.subcommand === 'doctor' so the doctor branch can run, i.e. change the
condition to only throw if parsed.subcommand !== 'doctor' (or move the throw
after the doctor branch), referencing the symbols mode, userBus.ok,
parsed.subcommand and the doctor branch starting at the block that inspects
linger, userState/systemState and prints "CloudCLI Daemon Doctor".
- Around line 199-238: The frontend daemon currently assumes vite is present
which will break global installs; either move "vite" from devDependencies to
dependencies in package.json so require/imports in server/vite-daemon.js
succeed, or gate creation of the systemd unit by checking for vite before
building/installing the unit: use require.resolve('vite') (wrap in try/catch)
where resolveFrontendDaemonEntry or buildFrontendDaemonServiceUnit is invoked
and skip generating/installing the frontend unit (or fallback to a no-op) if
vite cannot be resolved, ensuring
buildFrontendDaemonExecStart/buildFrontendDaemonServiceUnit are only called when
vite is available.
In `@server/daemon/manager.js`:
- Around line 423-463: stopDaemon currently uses readPidFile() +
isProcessRunning(pid) which can hit a recycled PID; before calling
terminateProcess or treating a PID as our daemon, verify the process identity
against the daemon state (e.g. STATE_FILE.startedAt and expected command).
Implement a short platform-aware check after readPidFile() and before
terminateProcess: on Linux read /proc/<pid>/cmdline and /proc/<pid>/stat start
time (or compare cmdline to expected binary/args), on other platforms use ps to
fetch command and start time (ps -o lstart,command) and compare to
STATE_FILE.startedAt or expected command; if the check fails, clearDaemonFiles
if appropriate and return { stopped: false, reason: 'pid_mismatch', pid } (or
treat as stale_pid) instead of terminating the process.
In `@server/projects.js`:
- Around line 236-238: The encodeProjectName function is overly
aggressive—change it to only perform Claude's established '/' → '-' encoding
(after path.resolve normalization) instead of replacing _ , whitespace, :, ~, or
backslashes; update encodeProjectName to preserve underscores and other
characters so generated keys match the decoders that call
projectName.replace(/-/g, '/'), and if Windows drive-letter handling is
required, implement a separate reversible scheme for ':' and '\\' rather than
collapsing them into '-' so keys remain unique and reversible for functions like
mergeDiscoveredProjectsIntoConfig, addProjectManually, and
extractProjectDirectory.
In `@server/vite-daemon.js`:
- Around line 16-36: The argument parsing loop in server/vite-daemon.js assigns
parsed.host and parsed.port from argv[++i] without checking that a next token
exists or isn't another flag, causing undefined/NaN and tight systemd restart
loops; update the for-loop handling of '--host'/'--port' (and their '=') to
first verify i+1 < argv.length and that argv[i+1] does not start with '-' before
consuming it, otherwise emit a clear error (or print usage) and exit non-zero;
ensure parsed.strictPort handling remains unchanged and consider returning a
non-retriable exit code or a short delay before exit to avoid tight systemd
restarts.
---
Nitpick comments:
In `@server/cli.js`:
- Around line 638-773: The duplicated daemon bootstrap helpers (isPortOpen,
waitForPortOpen, printSystemDaemonActiveNotice, printUserDaemonActiveNotice,
isSystemPermissionError) and related logic in maybeAutoDaemonStart should be
extracted into a shared module and imported by both server/cli.js and
server/index.js; create a new module (e.g., daemon-bootstrap) that exports those
functions plus buildAutoInstallArgs and
maybeAutoDaemonStart/maybeAutoDaemonBootstrapFromIndex as needed, update both
files to import and call the shared functions (preserving signatures and
behavior), and ensure error messages and environment checks remain identical so
fixes (like the permission regex in isSystemPermissionError or timing in
waitForPortOpen) are applied once.
In `@server/daemon-manager.js`:
- Around line 134-149: buildDaemonExecStart silently falls back to
appRoot/dist-server/server/cli.js without verifying it exists; update it to
mirror resolveFrontendDaemonEntry by checking each candidate (cliEntry,
process.argv[1], and appRoot/dist-server/server/cli.js) with fs.existsSync and
if none exist throw a clear Error describing the missing CLI entry (include the
attempted paths) instead of returning a non-existent path; adjust references to
cliCandidate and resolvedCliEntry so the function either returns a verified path
or throws so systemd failures surface as a descriptive install-time error.
In `@server/daemon/manager.js`:
- Around line 554-563: readDaemonLogs currently calls fs.readFileSync(LOG_FILE)
which can OOM or block the event loop for very large logs; change it to
stream/read from the tail instead: open the LOG_FILE (use fs.open or
fs.createReadStream), use fs.stat to get file size and calculate a start offset
for the last N lines (or read backwards in chunks until you have N newlines),
then stream only that portion into memory, assemble the final N lines, and
return the same object shape ({ exists, content, logFile }). Keep the existing
ensureDaemonDir() and LOG_FILE references and ensure the implementation handles
the file-not-exists case and errors gracefully without blocking the event loop.
- Around line 389-420: The daemon log is opened with fs.openSync(LOG_FILE, 'a')
and will grow unbounded; change the startup behavior in manager.js to rotate or
truncate the log before opening it: introduce a LOG_ROTATE_THRESHOLD (e.g.
bytes) and, before calling fs.openSync(LOG_FILE, 'a'), check
fs.statSync(LOG_FILE).size and if it exceeds the threshold rename LOG_FILE to
LOG_FILE + '.1' (fs.renameSync) or truncate to the last N bytes (read file tail
and overwrite) so a fresh file is opened for the spawned child (spawn(...,
stdio: ['ignore', logFd, logFd])). Update any code that reads the log
(readDaemonLogs) to handle rotated files or to only read the tail to avoid
loading the whole file into memory. Ensure these changes occur around the
existing LOG_FILE open and spawn block and keep writePidFile/writeStateFile
behavior unchanged.
In `@server/index.js`:
- Around line 596-666: The code inconsistently compares unresolved
WORKSPACES_BASE (defaultRoot) to validation.resolvedPath when deciding to mkdir
but later compares resolvedPath to resolvedWorkspaceBase (realpath) to decide
common-dir suggestions; compute resolvedWorkspaceBase (realpath of
WORKSPACES_BASE) once before the access/mkdir try block and use that
resolvedWorkspaceBase for both the mkdir check and the "is base" comparison,
i.e. replace usages of defaultRoot in the mkdir guard and the earlier fallback
validateWorkspacePath logic with the single resolvedWorkspaceBase value (retain
validateWorkspacePath and resolvedPath logic) so both branches use the
realpath-consistent base; update references to defaultRoot/WORKSPACES_BASE to
resolvedWorkspaceBase around fs.promises.mkdir and the final resolvedPath ===
resolvedWorkspaceBase check.
In `@server/routes/projects.js`:
- Around line 71-95: The normalizeWorkspacePath function currently joins
relative inputs with WORKSPACES_BASE which allows '..' segments to escape the
base; change it to reject or clamp such inputs by resolving against
WORKSPACES_BASE (e.g., compute resolved = path.resolve(WORKSPACES_BASE,
trimmedPath)) and then verify the resolved path is contained within
WORKSPACES_BASE (or return WORKSPACES_BASE / throw) if it is not; update
handling for Windows absolute detection (WINDOWS_ABSOLUTE_PATH_PATTERN) to still
allow absolute paths to be resolved via path.resolve(trimmedPath) and keep
special-case '~' handling.
In `@src/components/project-creation-wizard/components/WorkspacePathField.tsx`:
- Around line 93-94: The placeholder strings in WorkspacePathField are hardcoded
English; replace them with i18n lookups (e.g., use the component's translation
function or useTranslation hook) and add matching keys like
wizard.workspacePath.existingPlaceholder and wizard.workspacePath.newPlaceholder
to the locale resource files; update the conditional that currently returns
'my-existing-project or /absolute/path' / 'my-new-project or /absolute/path' to
call the translation function with those keys so placeholders are localized.
In `@vite.config.js`:
- Line 31: The review notes that setting strictPort: true in vite.config.js
causes Vite to exit if VITE_PORT (default 5173) is in use; update the
documentation to warn contributors about this behavior and how to avoid hard
failures when running npm run client concurrently. Add a short note in the
README daemon section (or developer/startup docs) explaining that strictPort is
enabled, that VITE_PORT will not auto-increment, how to change VITE_PORT or stop
other Vite instances, and include an example command to run the client on a
different port or how to export VITE_PORT before starting.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7a32b375-4195-4220-ab35-cf5b5fab28a1
📒 Files selected for processing (23)
.env.exampleREADME.mdpackage.jsonserver/cli.jsserver/daemon-manager.jsserver/daemon/manager.jsserver/index.jsserver/projects.jsserver/routes/projects.jsserver/vite-daemon.jssrc/components/project-creation-wizard/components/FolderBrowserModal.tsxsrc/components/project-creation-wizard/components/WorkspacePathField.tsxsrc/components/project-creation-wizard/data/workspaceApi.tssrc/components/project-creation-wizard/types.tssrc/components/sidebar/view/subcomponents/SidebarProjectList.tsxsrc/i18n/locales/de/sidebar.jsonsrc/i18n/locales/en/sidebar.jsonsrc/i18n/locales/ja/sidebar.jsonsrc/i18n/locales/ko/sidebar.jsonsrc/i18n/locales/ru/sidebar.jsonsrc/i18n/locales/zh-CN/sidebar.jsonsrc/types/app.tsvite.config.js
Comment on lines
+29
to
+31
| # Shell reconnect timeout after websocket disconnect (minutes) | ||
| # Default: 30 | ||
| # SHELL_SESSION_TIMEOUT_MINUTES=30 |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP 'SHELL_SESSION_TIMEOUT_MINUTES|PTY_SESSION_TIMEOUT' --type=js --type=tsRepository: siteboon/claudecodeui
Length of output: 190
🏁 Script executed:
cat -n server/index.js | sed -n '210,225p'Repository: siteboon/claudecodeui
Length of output: 699
🏁 Script executed:
cat -n server/index.js | sed -n '1910,1925p'Repository: siteboon/claudecodeui
Length of output: 719
🏁 Script executed:
rg 'process.env.SHELL_SESSION_TIMEOUT_MINUTES' --type=js --type=tsRepository: siteboon/claudecodeui
Length of output: 47
🏁 Script executed:
cat .env.example | head -35Repository: siteboon/claudecodeui
Length of output: 1281
SHELL_SESSION_TIMEOUT_MINUTES is documented but not implemented.
The .env.example file documents SHELL_SESSION_TIMEOUT_MINUTES as a configurable setting with a default of 30 minutes. However, in server/index.js (line 217), the timeout is hardcoded as const PTY_SESSION_TIMEOUT = 30 * 60 * 1000; and this constant is used directly in the setTimeout call (line 1918). The code does not read SHELL_SESSION_TIMEOUT_MINUTES from the environment, so users cannot actually configure this value. Either implement environment variable support for the timeout or remove these lines from .env.example.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.env.example around lines 29 - 31, The .env.example documents
SHELL_SESSION_TIMEOUT_MINUTES but server/index.js hardcodes PTY_SESSION_TIMEOUT;
update server/index.js to read process.env.SHELL_SESSION_TIMEOUT_MINUTES, parse
it as an integer with a default of 30, compute PTY_SESSION_TIMEOUT = minutes *
60 * 1000 and use that value where the existing PTY_SESSION_TIMEOUT constant and
setTimeout are used (references: PTY_SESSION_TIMEOUT constant and the setTimeout
call around the shell session cleanup logic). Ensure invalid or missing env
values fall back to 30 minutes and the parsed value is used consistently, or
alternatively remove the SHELL_SESSION_TIMEOUT_MINUTES lines from .env.example
if you prefer not to support runtime configuration.
Comment on lines
+27
to
+30
| "dev": "npm run server:dev", | ||
| "server": "node dist-server/server/cli.js start", | ||
| "server:dev": "node server/cli.js daemon install --mode system --port 3001 --frontend-port 5173", | ||
| "server:dev-watch": "node server/cli.js daemon restart --mode system --port 3001 --frontend-port 5173", |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -t f 'daemon-manager.js' | xargs -I{} rg -nP 'systemctl|--mode\s+system' {} -C2
fd -t f 'vite-daemon.js'Repository: siteboon/claudecodeui
Length of output: 3450
🏁 Script executed:
#!/bin/bash
# Check vite-daemon.js content
fd -t f 'vite-daemon.js' -exec cat {} \;
# Check for install guard logic
rg -n 'install.*function|function.*install' server/daemon-manager.js -A5 -B2 | head -50Repository: siteboon/claudecodeui
Length of output: 252
🏁 Script executed:
#!/bin/bash
# Check vite-daemon.js content
cat server/vite-daemon.js
# Check for install guard logic and daemon install implementation
rg -n 'case.*install|function.*install' server/daemon-manager.js -A10 | head -60Repository: siteboon/claudecodeui
Length of output: 3690
🏁 Script executed:
#!/bin/bash
# Look at the full install case and what happens after creating unit files
rg -n 'case.*install' server/daemon-manager.js -A100 | head -120
# Check if there's any existing file check before installing
rg -n 'existsSync.*servicePath|already.*installed' server/daemon-manager.js -B2 -A2Repository: siteboon/claudecodeui
Length of output: 8727
🏁 Script executed:
#!/bin/bash
# Check the exact package.json scripts
sed -n '25,35p' package.json
# Check if there's a daemon restart that might be better suited for dev
rg -n 'case.*restart|case.*start' server/daemon-manager.js -A10 | head -40Repository: siteboon/claudecodeui
Length of output: 2030
npm run dev will fail on non-Linux systems and requires elevated privileges for each invocation.
Three significant issues with the new dev setup:
-
Linux-only:
daemon install --mode systeminvokessystemctl(viaensureSystemctl()), which is unavailable on macOS/Windows. Contributors on those platforms cannot runnpm run devat all. The priorconcurrently+tsxflow was cross-platform. -
Broken dev loop:
daemon installis a one-time setup command that runs on every invocation ofnpm run devwithout a guard check. Each run reinstalls systemd units and requires elevated privileges (sudo), yet the npm script doesn't invokesudo— it will fail with permission errors. The workflow should be: one-time setup, thendaemon restartfor iterative development (whichnpm run server:dev-watchprovides, butdevdoesn't use it). -
Inappropriate mode:
--mode systemcreates a system-wide daemon requiringsudo, which is unsuitable for local development. Consider--mode user(requires user linger) or--mode auto(falls back gracefully) for better developer experience.
The vite-daemon.js setup is correct and will properly spawn the Vite frontend via systemd, but the dev entrypoint needs restructuring. Clarify the intended workflow: Is npm run dev meant to be a one-time setup command, or should it use daemon restart after initial install?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package.json` around lines 27 - 30, The current "dev" npm script runs
"server:dev" which calls "daemon install --mode system" causing Linux-only,
sudo-required behavior and re-install on every run; change the dev entrypoint to
a cross-platform, iterative dev flow: update "dev" to run the frontend and
backend in watch/restart mode (use "server:dev-watch" instead of "server:dev"),
or implement a guard around "daemon install" so it runs once (e.g., a bootstrap
script that checks for existing unit/linger) and then uses "daemon restart" for
subsequent runs; also change the daemon mode flag from "--mode system" to a
non-privileged default like "--mode auto" or "--mode user" to avoid requiring
elevated privileges on developers' machines, and ensure any new bootstrap script
references the existing vite-daemon.js behavior rather than invoking
ensureSystemctl directly.
Comment on lines
+199
to
+238
| function buildFrontendDaemonExecStart({ appRoot, frontendPort, nodeExecPath, cliEntry }) { | ||
| const nodeExec = nodeExecPath || process.execPath || 'node'; | ||
| const frontendEntry = resolveFrontendDaemonEntry(appRoot, cliEntry); | ||
| const args = [ | ||
| nodeExec, | ||
| frontendEntry, | ||
| '--host', | ||
| '0.0.0.0', | ||
| '--port', | ||
| String(frontendPort), | ||
| '--strictPort', | ||
| ]; | ||
| return args.map(quoteSystemdArg).join(' '); | ||
| } | ||
|
|
||
| function buildFrontendDaemonServiceUnit({ appRoot, frontendPort, nodeExecPath, cliEntry }) { | ||
| const execStart = buildFrontendDaemonExecStart({ | ||
| appRoot, | ||
| frontendPort, | ||
| nodeExecPath, | ||
| cliEntry, | ||
| }); | ||
| return `[Unit] | ||
| Description=CloudCLI Frontend (Vite Dev Server) | ||
| After=network.target | ||
|
|
||
| [Service] | ||
| Type=simple | ||
| WorkingDirectory=${appRoot} | ||
| ExecStart=${execStart} | ||
| Environment=HOST=0.0.0.0 | ||
| Environment=CI=true | ||
| Environment=CLOUDCLI_DAEMON_MANAGED=1 | ||
| Restart=always | ||
| RestartSec=2 | ||
|
|
||
| [Install] | ||
| WantedBy=default.target | ||
| `; | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether vite is a dependency or devDependency, and whether the generated
# frontend unit's ExecStart target can import it from the installed tree.
set -u
fd -HI -a package.json -d 2 | head -5 | while read -r pkg; do
echo "=== $pkg ==="
jq -r '{
name,
deps_vite: (.dependencies.vite // "absent"),
devDeps_vite: (.devDependencies.vite // "absent"),
peerDeps_vite: (.peerDependencies.vite // "absent"),
optionalDeps_vite: (.optionalDependencies.vite // "absent")
}' "$pkg"
done
# Also grep vite-daemon.js's import to confirm the resolution path
rg -nP "from ['\"]vite['\"]" --type=jsRepository: siteboon/claudecodeui
Length of output: 593
🏁 Script executed:
# Check the resolveFrontendDaemonEntry function and related logic
rg -A 10 "function resolveFrontendDaemonEntry" server/daemon-manager.js
# Look for any conditional logic around frontend unit installation
rg -B 5 -A 5 "buildFrontendDaemonServiceUnit" server/daemon-manager.js | head -40
# Check if there's any gating based on vite availability
rg -n "vite|require.resolve|resolveFrontendDaemonEntry" server/daemon-manager.js | head -20Repository: siteboon/claudecodeui
Length of output: 1808
🏁 Script executed:
# Get the complete resolveFrontendDaemonEntry function
sed -n '178,197p' server/daemon-manager.js
# Check if there's any error handling or gating before creating the frontend unit
rg -B 10 "frontendUnitContent = buildFrontendDaemonServiceUnit" server/daemon-manager.js | head -30
# Check if vite-daemon.js exists and what it does
cat server/vite-daemon.js | head -50Repository: siteboon/claudecodeui
Length of output: 3000
🏁 Script executed:
# Check if there's any try-catch or error handling around the frontend unit creation
rg -B 20 -A 10 "frontendUnitContent = buildFrontendDaemonServiceUnit" server/daemon-manager.js | tail -50
# Check if vite is installed in the current environment or if there's a workaround
grep -r "require.resolve\|import.*vite" server/ --include="*.js" | head -10
# Look for any npm scripts or installation hooks that might handle this
grep -A 5 -B 5 "vite" package.json | head -30Repository: siteboon/claudecodeui
Length of output: 2948
Move vite to production dependencies or gate the frontend daemon installation.
The server/vite-daemon.js file directly imports createServer from 'vite', but vite is currently a devDependency only. When users install globally with npm install -g @cloudcli-ai/cloudcli``, npm skips devDependencies, causing the daemon to fail with ERR_MODULE_NOT_FOUND on startup. The systemd unit's `Restart=always` setting will cause it to restart indefinitely.
Either promote vite to dependencies in package.json, or add a check in the daemon installation logic (similar to resolveFrontendDaemonEntry) that uses require.resolve('vite') to gate creation of the frontend systemd unit when vite is unavailable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/daemon-manager.js` around lines 199 - 238, The frontend daemon
currently assumes vite is present which will break global installs; either move
"vite" from devDependencies to dependencies in package.json so require/imports
in server/vite-daemon.js succeed, or gate creation of the systemd unit by
checking for vite before building/installing the unit: use
require.resolve('vite') (wrap in try/catch) where resolveFrontendDaemonEntry or
buildFrontendDaemonServiceUnit is invoked and skip generating/installing the
frontend unit (or fallback to a no-op) if vite cannot be resolved, ensuring
buildFrontendDaemonExecStart/buildFrontendDaemonServiceUnit are only called when
vite is available.
Comment on lines
+525
to
+601
| if (mode === 'user' && !userBus.ok) { | ||
| throw new Error( | ||
| `Could not connect to your systemd user session.\n${userBus.detail}\n` + | ||
| `Try ${c.bright('cloudcli daemon install --mode system')} or enable user linger: ${c.bright(`sudo loginctl enable-linger ${os.userInfo().username}`)}` | ||
| ); | ||
| } | ||
|
|
||
| if (parsed.subcommand === 'doctor') { | ||
| const linger = probeLinger(); | ||
| const userUnitInstalled = fs.existsSync(getDaemonServicePath('user')); | ||
| const userFrontendUnitInstalled = fs.existsSync(getFrontendServicePath('user')); | ||
| const systemUnitInstalled = fs.existsSync(getDaemonServicePath('system')); | ||
| const systemFrontendUnitInstalled = fs.existsSync(getFrontendServicePath('system')); | ||
| const selectedModePort = getPortFromServiceUnit(servicePath) || portNum; | ||
| const selectedModeFrontendPort = getPortFromServiceUnit(frontendServicePath) || frontendPortNum; | ||
| const portReachable = await isPortReachable(selectedModePort); | ||
| const frontendPortReachable = await isPortReachable(selectedModeFrontendPort); | ||
| const userState = userBus.ok | ||
| ? { | ||
| backend: getServiceState('user', DAEMON_SERVICE_NAME), | ||
| frontend: getServiceState('user', FRONTEND_DAEMON_SERVICE_NAME), | ||
| } | ||
| : { | ||
| backend: { active: 'unavailable', enabled: 'unavailable' }, | ||
| frontend: { active: 'unavailable', enabled: 'unavailable' }, | ||
| }; | ||
| const systemState = { | ||
| backend: getServiceState('system', DAEMON_SERVICE_NAME), | ||
| frontend: getServiceState('system', FRONTEND_DAEMON_SERVICE_NAME), | ||
| }; | ||
| const logsResult = readLogs(mode, 50, false, [DAEMON_SERVICE_NAME, FRONTEND_DAEMON_SERVICE_NAME]); | ||
| const logsText = (logsResult.stdout || logsResult.stderr || '').trim(); | ||
| const lastErrorLine = findLatestErrorLine(logsText); | ||
|
|
||
| console.log(`\n${c.bright('CloudCLI Daemon Doctor')}\n`); | ||
| console.log(`${c.info('[INFO]')} Requested mode: ${c.bright(parsed.options.mode)}`); | ||
| console.log(`${c.info('[INFO]')} Resolved mode: ${c.bright(mode)}`); | ||
| console.log(`${c.info('[INFO]')} user-bus: ${userBus.ok ? c.ok('ok') : c.warn('unavailable')}`); | ||
| if (!userBus.ok) { | ||
| console.log(` ${c.dim(userBus.detail)}`); | ||
| } | ||
| console.log(`${c.info('[INFO]')} linger: ${c.bright(linger.value)}${linger.detail ? ` ${c.dim(`(${linger.detail})`)}` : ''}`); | ||
| console.log(`${c.info('[INFO]')} user backend: ${userUnitInstalled ? c.ok('installed') : c.warn('missing')} (${c.dim(getDaemonServicePath('user'))})`); | ||
| console.log(`${c.info('[INFO]')} user frontend: ${userFrontendUnitInstalled ? c.ok('installed') : c.warn('missing')} (${c.dim(getFrontendServicePath('user'))})`); | ||
| console.log(`${c.info('[INFO]')} system backend: ${systemUnitInstalled ? c.ok('installed') : c.warn('missing')} (${c.dim(getDaemonServicePath('system'))})`); | ||
| console.log(`${c.info('[INFO]')} system frontend:${systemFrontendUnitInstalled ? c.ok('installed') : c.warn('missing')} (${c.dim(getFrontendServicePath('system'))})`); | ||
| console.log(`${c.info('[INFO]')} user state: backend active=${c.bright(userState.backend.active)} enabled=${c.bright(userState.backend.enabled)} | frontend active=${c.bright(userState.frontend.active)} enabled=${c.bright(userState.frontend.enabled)}`); | ||
| console.log(`${c.info('[INFO]')} system state: backend active=${c.bright(systemState.backend.active)} enabled=${c.bright(systemState.backend.enabled)} | frontend active=${c.bright(systemState.frontend.active)} enabled=${c.bright(systemState.frontend.enabled)}`); | ||
| console.log(`${c.info('[INFO]')} backend port: ${c.bright(String(selectedModePort))} (${portReachable ? c.ok('reachable') : c.warn('not reachable')})`); | ||
| console.log(`${c.info('[INFO]')} frontend port: ${c.bright(String(selectedModeFrontendPort))} (${frontendPortReachable ? c.ok('reachable') : c.warn('not reachable')})`); | ||
| if (lastErrorLine) { | ||
| console.log(`${c.warn('[WARN]')} Latest error: ${lastErrorLine}`); | ||
| } | ||
|
|
||
| console.log(`\n${c.bright('Machine Readable')}`); | ||
| console.log(`MODE_REQUESTED=${parsed.options.mode}`); | ||
| console.log(`MODE_RESOLVED=${mode}`); | ||
| console.log(`USER_BUS_OK=${userBus.ok}`); | ||
| console.log(`LINGER=${linger.value}`); | ||
| console.log(`USER_BACKEND_UNIT_INSTALLED=${userUnitInstalled}`); | ||
| console.log(`USER_FRONTEND_UNIT_INSTALLED=${userFrontendUnitInstalled}`); | ||
| console.log(`SYSTEM_BACKEND_UNIT_INSTALLED=${systemUnitInstalled}`); | ||
| console.log(`SYSTEM_FRONTEND_UNIT_INSTALLED=${systemFrontendUnitInstalled}`); | ||
| console.log(`USER_BACKEND_ACTIVE=${userState.backend.active}`); | ||
| console.log(`USER_BACKEND_ENABLED=${userState.backend.enabled}`); | ||
| console.log(`USER_FRONTEND_ACTIVE=${userState.frontend.active}`); | ||
| console.log(`USER_FRONTEND_ENABLED=${userState.frontend.enabled}`); | ||
| console.log(`SYSTEM_BACKEND_ACTIVE=${systemState.backend.active}`); | ||
| console.log(`SYSTEM_BACKEND_ENABLED=${systemState.backend.enabled}`); | ||
| console.log(`SYSTEM_FRONTEND_ACTIVE=${systemState.frontend.active}`); | ||
| console.log(`SYSTEM_FRONTEND_ENABLED=${systemState.frontend.enabled}`); | ||
| console.log(`BACKEND_PORT=${selectedModePort}`); | ||
| console.log(`BACKEND_PORT_REACHABLE=${portReachable}`); | ||
| console.log(`FRONTEND_PORT=${selectedModeFrontendPort}`); | ||
| console.log(`FRONTEND_PORT_REACHABLE=${frontendPortReachable}`); | ||
| console.log(`LAST_ERROR_LINE=${JSON.stringify(lastErrorLine || '')}\n`); | ||
| return; |
Contributor
There was a problem hiding this comment.
daemon doctor aborts on the very failure it's meant to diagnose.
When a user runs cloudcli daemon doctor --mode user on a host where the systemd user bus is unreachable (missing session, no linger, sudo-su'd into the account, etc.), the if (mode === 'user' && !userBus.ok) throw at line 525 fires before the doctor branch at line 532 can print diagnostics. This is the exact scenario doctor exists to report on — it already has userBus.ok ? ... : { active: 'unavailable', ... } handling in its output.
Suggested reorder
- if (mode === 'user' && !userBus.ok) {
- throw new Error(
- `Could not connect to your systemd user session.\n${userBus.detail}\n` +
- `Try ${c.bright('cloudcli daemon install --mode system')} or enable user linger: ${c.bright(`sudo loginctl enable-linger ${os.userInfo().username}`)}`
- );
- }
-
if (parsed.subcommand === 'doctor') {
// ... existing doctor body ...
return;
}
+
+ if (mode === 'user' && !userBus.ok) {
+ throw new Error(
+ `Could not connect to your systemd user session.\n${userBus.detail}\n` +
+ `Try ${c.bright('cloudcli daemon install --mode system')} or enable user linger: ${c.bright(`sudo loginctl enable-linger ${os.userInfo().username}`)}`
+ );
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (mode === 'user' && !userBus.ok) { | |
| throw new Error( | |
| `Could not connect to your systemd user session.\n${userBus.detail}\n` + | |
| `Try ${c.bright('cloudcli daemon install --mode system')} or enable user linger: ${c.bright(`sudo loginctl enable-linger ${os.userInfo().username}`)}` | |
| ); | |
| } | |
| if (parsed.subcommand === 'doctor') { | |
| const linger = probeLinger(); | |
| const userUnitInstalled = fs.existsSync(getDaemonServicePath('user')); | |
| const userFrontendUnitInstalled = fs.existsSync(getFrontendServicePath('user')); | |
| const systemUnitInstalled = fs.existsSync(getDaemonServicePath('system')); | |
| const systemFrontendUnitInstalled = fs.existsSync(getFrontendServicePath('system')); | |
| const selectedModePort = getPortFromServiceUnit(servicePath) || portNum; | |
| const selectedModeFrontendPort = getPortFromServiceUnit(frontendServicePath) || frontendPortNum; | |
| const portReachable = await isPortReachable(selectedModePort); | |
| const frontendPortReachable = await isPortReachable(selectedModeFrontendPort); | |
| const userState = userBus.ok | |
| ? { | |
| backend: getServiceState('user', DAEMON_SERVICE_NAME), | |
| frontend: getServiceState('user', FRONTEND_DAEMON_SERVICE_NAME), | |
| } | |
| : { | |
| backend: { active: 'unavailable', enabled: 'unavailable' }, | |
| frontend: { active: 'unavailable', enabled: 'unavailable' }, | |
| }; | |
| const systemState = { | |
| backend: getServiceState('system', DAEMON_SERVICE_NAME), | |
| frontend: getServiceState('system', FRONTEND_DAEMON_SERVICE_NAME), | |
| }; | |
| const logsResult = readLogs(mode, 50, false, [DAEMON_SERVICE_NAME, FRONTEND_DAEMON_SERVICE_NAME]); | |
| const logsText = (logsResult.stdout || logsResult.stderr || '').trim(); | |
| const lastErrorLine = findLatestErrorLine(logsText); | |
| console.log(`\n${c.bright('CloudCLI Daemon Doctor')}\n`); | |
| console.log(`${c.info('[INFO]')} Requested mode: ${c.bright(parsed.options.mode)}`); | |
| console.log(`${c.info('[INFO]')} Resolved mode: ${c.bright(mode)}`); | |
| console.log(`${c.info('[INFO]')} user-bus: ${userBus.ok ? c.ok('ok') : c.warn('unavailable')}`); | |
| if (!userBus.ok) { | |
| console.log(` ${c.dim(userBus.detail)}`); | |
| } | |
| console.log(`${c.info('[INFO]')} linger: ${c.bright(linger.value)}${linger.detail ? ` ${c.dim(`(${linger.detail})`)}` : ''}`); | |
| console.log(`${c.info('[INFO]')} user backend: ${userUnitInstalled ? c.ok('installed') : c.warn('missing')} (${c.dim(getDaemonServicePath('user'))})`); | |
| console.log(`${c.info('[INFO]')} user frontend: ${userFrontendUnitInstalled ? c.ok('installed') : c.warn('missing')} (${c.dim(getFrontendServicePath('user'))})`); | |
| console.log(`${c.info('[INFO]')} system backend: ${systemUnitInstalled ? c.ok('installed') : c.warn('missing')} (${c.dim(getDaemonServicePath('system'))})`); | |
| console.log(`${c.info('[INFO]')} system frontend:${systemFrontendUnitInstalled ? c.ok('installed') : c.warn('missing')} (${c.dim(getFrontendServicePath('system'))})`); | |
| console.log(`${c.info('[INFO]')} user state: backend active=${c.bright(userState.backend.active)} enabled=${c.bright(userState.backend.enabled)} | frontend active=${c.bright(userState.frontend.active)} enabled=${c.bright(userState.frontend.enabled)}`); | |
| console.log(`${c.info('[INFO]')} system state: backend active=${c.bright(systemState.backend.active)} enabled=${c.bright(systemState.backend.enabled)} | frontend active=${c.bright(systemState.frontend.active)} enabled=${c.bright(systemState.frontend.enabled)}`); | |
| console.log(`${c.info('[INFO]')} backend port: ${c.bright(String(selectedModePort))} (${portReachable ? c.ok('reachable') : c.warn('not reachable')})`); | |
| console.log(`${c.info('[INFO]')} frontend port: ${c.bright(String(selectedModeFrontendPort))} (${frontendPortReachable ? c.ok('reachable') : c.warn('not reachable')})`); | |
| if (lastErrorLine) { | |
| console.log(`${c.warn('[WARN]')} Latest error: ${lastErrorLine}`); | |
| } | |
| console.log(`\n${c.bright('Machine Readable')}`); | |
| console.log(`MODE_REQUESTED=${parsed.options.mode}`); | |
| console.log(`MODE_RESOLVED=${mode}`); | |
| console.log(`USER_BUS_OK=${userBus.ok}`); | |
| console.log(`LINGER=${linger.value}`); | |
| console.log(`USER_BACKEND_UNIT_INSTALLED=${userUnitInstalled}`); | |
| console.log(`USER_FRONTEND_UNIT_INSTALLED=${userFrontendUnitInstalled}`); | |
| console.log(`SYSTEM_BACKEND_UNIT_INSTALLED=${systemUnitInstalled}`); | |
| console.log(`SYSTEM_FRONTEND_UNIT_INSTALLED=${systemFrontendUnitInstalled}`); | |
| console.log(`USER_BACKEND_ACTIVE=${userState.backend.active}`); | |
| console.log(`USER_BACKEND_ENABLED=${userState.backend.enabled}`); | |
| console.log(`USER_FRONTEND_ACTIVE=${userState.frontend.active}`); | |
| console.log(`USER_FRONTEND_ENABLED=${userState.frontend.enabled}`); | |
| console.log(`SYSTEM_BACKEND_ACTIVE=${systemState.backend.active}`); | |
| console.log(`SYSTEM_BACKEND_ENABLED=${systemState.backend.enabled}`); | |
| console.log(`SYSTEM_FRONTEND_ACTIVE=${systemState.frontend.active}`); | |
| console.log(`SYSTEM_FRONTEND_ENABLED=${systemState.frontend.enabled}`); | |
| console.log(`BACKEND_PORT=${selectedModePort}`); | |
| console.log(`BACKEND_PORT_REACHABLE=${portReachable}`); | |
| console.log(`FRONTEND_PORT=${selectedModeFrontendPort}`); | |
| console.log(`FRONTEND_PORT_REACHABLE=${frontendPortReachable}`); | |
| console.log(`LAST_ERROR_LINE=${JSON.stringify(lastErrorLine || '')}\n`); | |
| return; | |
| if (parsed.subcommand === 'doctor') { | |
| const linger = probeLinger(); | |
| const userUnitInstalled = fs.existsSync(getDaemonServicePath('user')); | |
| const userFrontendUnitInstalled = fs.existsSync(getFrontendServicePath('user')); | |
| const systemUnitInstalled = fs.existsSync(getDaemonServicePath('system')); | |
| const systemFrontendUnitInstalled = fs.existsSync(getFrontendServicePath('system')); | |
| const selectedModePort = getPortFromServiceUnit(servicePath) || portNum; | |
| const selectedModeFrontendPort = getPortFromServiceUnit(frontendServicePath) || frontendPortNum; | |
| const portReachable = await isPortReachable(selectedModePort); | |
| const frontendPortReachable = await isPortReachable(selectedModeFrontendPort); | |
| const userState = userBus.ok | |
| ? { | |
| backend: getServiceState('user', DAEMON_SERVICE_NAME), | |
| frontend: getServiceState('user', FRONTEND_DAEMON_SERVICE_NAME), | |
| } | |
| : { | |
| backend: { active: 'unavailable', enabled: 'unavailable' }, | |
| frontend: { active: 'unavailable', enabled: 'unavailable' }, | |
| }; | |
| const systemState = { | |
| backend: getServiceState('system', DAEMON_SERVICE_NAME), | |
| frontend: getServiceState('system', FRONTEND_DAEMON_SERVICE_NAME), | |
| }; | |
| const logsResult = readLogs(mode, 50, false, [DAEMON_SERVICE_NAME, FRONTEND_DAEMON_SERVICE_NAME]); | |
| const logsText = (logsResult.stdout || logsResult.stderr || '').trim(); | |
| const lastErrorLine = findLatestErrorLine(logsText); | |
| console.log(`\n${c.bright('CloudCLI Daemon Doctor')}\n`); | |
| console.log(`${c.info('[INFO]')} Requested mode: ${c.bright(parsed.options.mode)}`); | |
| console.log(`${c.info('[INFO]')} Resolved mode: ${c.bright(mode)}`); | |
| console.log(`${c.info('[INFO]')} user-bus: ${userBus.ok ? c.ok('ok') : c.warn('unavailable')}`); | |
| if (!userBus.ok) { | |
| console.log(` ${c.dim(userBus.detail)}`); | |
| } | |
| console.log(`${c.info('[INFO]')} linger: ${c.bright(linger.value)}${linger.detail ? ` ${c.dim(`(${linger.detail})`)}` : ''}`); | |
| console.log(`${c.info('[INFO]')} user backend: ${userUnitInstalled ? c.ok('installed') : c.warn('missing')} (${c.dim(getDaemonServicePath('user'))})`); | |
| console.log(`${c.info('[INFO]')} user frontend: ${userFrontendUnitInstalled ? c.ok('installed') : c.warn('missing')} (${c.dim(getFrontendServicePath('user'))})`); | |
| console.log(`${c.info('[INFO]')} system backend: ${systemUnitInstalled ? c.ok('installed') : c.warn('missing')} (${c.dim(getDaemonServicePath('system'))})`); | |
| console.log(`${c.info('[INFO]')} system frontend:${systemFrontendUnitInstalled ? c.ok('installed') : c.warn('missing')} (${c.dim(getFrontendServicePath('system'))})`); | |
| console.log(`${c.info('[INFO]')} user state: backend active=${c.bright(userState.backend.active)} enabled=${c.bright(userState.backend.enabled)} | frontend active=${c.bright(userState.frontend.active)} enabled=${c.bright(userState.frontend.enabled)}`); | |
| console.log(`${c.info('[INFO]')} system state: backend active=${c.bright(systemState.backend.active)} enabled=${c.bright(systemState.backend.enabled)} | frontend active=${c.bright(systemState.frontend.active)} enabled=${c.bright(systemState.frontend.enabled)}`); | |
| console.log(`${c.info('[INFO]')} backend port: ${c.bright(String(selectedModePort))} (${portReachable ? c.ok('reachable') : c.warn('not reachable')})`); | |
| console.log(`${c.info('[INFO]')} frontend port: ${c.bright(String(selectedModeFrontendPort))} (${frontendPortReachable ? c.ok('reachable') : c.warn('not reachable')})`); | |
| if (lastErrorLine) { | |
| console.log(`${c.warn('[WARN]')} Latest error: ${lastErrorLine}`); | |
| } | |
| console.log(`\n${c.bright('Machine Readable')}`); | |
| console.log(`MODE_REQUESTED=${parsed.options.mode}`); | |
| console.log(`MODE_RESOLVED=${mode}`); | |
| console.log(`USER_BUS_OK=${userBus.ok}`); | |
| console.log(`LINGER=${linger.value}`); | |
| console.log(`USER_BACKEND_UNIT_INSTALLED=${userUnitInstalled}`); | |
| console.log(`USER_FRONTEND_UNIT_INSTALLED=${userFrontendUnitInstalled}`); | |
| console.log(`SYSTEM_BACKEND_UNIT_INSTALLED=${systemUnitInstalled}`); | |
| console.log(`SYSTEM_FRONTEND_UNIT_INSTALLED=${systemFrontendUnitInstalled}`); | |
| console.log(`USER_BACKEND_ACTIVE=${userState.backend.active}`); | |
| console.log(`USER_BACKEND_ENABLED=${userState.backend.enabled}`); | |
| console.log(`USER_FRONTEND_ACTIVE=${userState.frontend.active}`); | |
| console.log(`USER_FRONTEND_ENABLED=${userState.frontend.enabled}`); | |
| console.log(`SYSTEM_BACKEND_ACTIVE=${systemState.backend.active}`); | |
| console.log(`SYSTEM_BACKEND_ENABLED=${systemState.backend.enabled}`); | |
| console.log(`SYSTEM_FRONTEND_ACTIVE=${systemState.frontend.active}`); | |
| console.log(`SYSTEM_FRONTEND_ENABLED=${systemState.frontend.enabled}`); | |
| console.log(`BACKEND_PORT=${selectedModePort}`); | |
| console.log(`BACKEND_PORT_REACHABLE=${portReachable}`); | |
| console.log(`FRONTEND_PORT=${selectedModeFrontendPort}`); | |
| console.log(`FRONTEND_PORT_REACHABLE=${frontendPortReachable}`); | |
| console.log(`LAST_ERROR_LINE=${JSON.stringify(lastErrorLine || '')}\n`); | |
| return; | |
| } | |
| if (mode === 'user' && !userBus.ok) { | |
| throw new Error( | |
| `Could not connect to your systemd user session.\n${userBus.detail}\n` + | |
| `Try ${c.bright('cloudcli daemon install --mode system')} or enable user linger: ${c.bright(`sudo loginctl enable-linger ${os.userInfo().username}`)}` | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/daemon-manager.js` around lines 525 - 601, The code currently throws
when mode === 'user' && !userBus.ok which prevents the doctor path from running;
update the guard in the top-level check (the throw for mode/user/userBus.ok) to
skip throwing when parsed.subcommand === 'doctor' so the doctor branch can run,
i.e. change the condition to only throw if parsed.subcommand !== 'doctor' (or
move the throw after the doctor branch), referencing the symbols mode,
userBus.ok, parsed.subcommand and the doctor branch starting at the block that
inspects linger, userState/systemState and prints "CloudCLI Daemon Doctor".
Comment on lines
+613
to
+617
| switch (parsed.subcommand) { | ||
| case 'install': { | ||
| if (parsed.options.mode === 'auto' && mode === 'system' && !userBus.ok) { | ||
| console.log(`${c.warn('[WARN]')} User mode is unavailable; auto mode is falling back to system mode.`); | ||
| } |
Contributor
There was a problem hiding this comment.
Misleading "falling back" message on fresh auto install.
resolveDaemonMode unconditionally returns 'system' for subcommand === 'install' with auto mode (line 319), independent of userBus.ok. So the warning here ("User mode is unavailable; auto mode is falling back to system mode.") prints on any healthy --mode=auto install, implying a degradation that didn't occur. Either drop this warning for install or scope it to cases where auto would have picked user if the bus were up (i.e., userUnitInstalled && !userBus.ok).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/daemon-manager.js` around lines 613 - 617, The warning message prints
unconditionally for install with --mode=auto even when nothing fell back; update
the condition around the console.log in the switch for parsed.subcommand
'install' so it only logs when auto would have selected user but the user bus is
down: i.e., require parsed.options.mode === 'auto' && mode === 'system' &&
userUnitInstalled && !userBus.ok (reference parsed.subcommand,
parsed.options.mode, mode, userBus.ok, userUnitInstalled and resolveDaemonMode)
so the message is only shown when a real fallback occurred.
Comment on lines
+423
to
+463
| export async function stopDaemon() { | ||
| ensureDaemonDir(); | ||
| const pid = readPidFile(); | ||
|
|
||
| if (!pid) { | ||
| return { stopped: false, reason: 'not_running' }; | ||
| } | ||
|
|
||
| if (!isProcessRunning(pid)) { | ||
| clearDaemonFiles(); | ||
| return { stopped: true, reason: 'stale_pid', pid }; | ||
| } | ||
|
|
||
| try { | ||
| terminateProcess(pid, process.platform === 'win32'); | ||
| } catch (error) { | ||
| if (!isProcessRunning(pid)) { | ||
| clearDaemonFiles(); | ||
| return { stopped: true, reason: 'stopped', pid }; | ||
| } | ||
|
|
||
| return { stopped: false, reason: 'termination_failed', pid, error: error.message }; | ||
| } | ||
|
|
||
| let stopped = await waitForProcessExit(pid, 10000); | ||
| if (!stopped && process.platform !== 'win32') { | ||
| try { | ||
| terminateProcess(pid, true); | ||
| } catch { | ||
| // Best effort. | ||
| } | ||
| stopped = await waitForProcessExit(pid, 5000); | ||
| } | ||
|
|
||
| if (stopped) { | ||
| clearDaemonFiles(); | ||
| return { stopped: true, reason: 'stopped', pid }; | ||
| } | ||
|
|
||
| return { stopped: false, reason: 'timeout', pid }; | ||
| } |
Contributor
There was a problem hiding this comment.
Minor: PID recycling can target an unrelated process after reboot.
readPidFile() + isProcessRunning(pid) returns true for any live process with that PID, not just our daemon. After a reboot or long uptime, the OS may have recycled the PID to an unrelated process, which stopDaemon would then SIGTERM (and potentially SIGKILL).
Low probability in practice for user-scoped daemons, but cheap to harden: cross-check the process command line / start time against STATE_FILE.startedAt before terminating, or wrap PID reads with process.platform-specific verification (/proc/<pid>/cmdline on Linux, ps -o lstart/command elsewhere).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/daemon/manager.js` around lines 423 - 463, stopDaemon currently uses
readPidFile() + isProcessRunning(pid) which can hit a recycled PID; before
calling terminateProcess or treating a PID as our daemon, verify the process
identity against the daemon state (e.g. STATE_FILE.startedAt and expected
command). Implement a short platform-aware check after readPidFile() and before
terminateProcess: on Linux read /proc/<pid>/cmdline and /proc/<pid>/stat start
time (or compare cmdline to expected binary/args), on other platforms use ps to
fetch command and start time (ps -o lstart,command) and compare to
STATE_FILE.startedAt or expected command; if the check fails, clearDaemonFiles
if appropriate and return { stopped: false, reason: 'pid_mismatch', pid } (or
treat as stale_pid) instead of terminating the process.
Comment on lines
+236
to
+238
| function encodeProjectName(projectPath) { | ||
| return path.resolve(projectPath).replace(/[\\/:\s~_]/g, '-'); | ||
| } |
Contributor
There was a problem hiding this comment.
encodeProjectName encoding diverges from Claude's /↔- convention and will create duplicate project entries.
The module-level docstring (lines 9–12) and every decoder in this file (projectName.replace(/-/g, '/') at lines 432, 489, 526, 550, 563, 567, 744, 2234) assume the only encoded character is / → -. The new regex /[\\/:\s~_]/g additionally replaces _, whitespace, :, ~, and \\.
Concrete collision: path /home/alice/my_project
- Claude stores its folder as
-home-alice-my_project(drives the main loop ingetProjectsviaexistingProjects). encodeProjectNameproduces-home-alice-my-project.
Consequences:
mergeDiscoveredProjectsIntoConfigwrites config under a key that never matches Claude's actual folder, so!existingProjects.has(projectName)at line 726 is always true → the project is rendered twice in the UI (once from the Claude folder in the first loop, once as a tracked/auto-discovered entry).addProjectManually(line 1435) similarly produces a config key that can't be correlated with an existing Claude folder, silently fragmenting state.- Downstream
extractProjectDirectoryrelies onoriginalPathfrom config keyed by the encoded name; mismatched keys mean cached paths won't be found.
Suggest restricting this helper to Claude's established convention and letting path.resolve handle normalization:
🛠️ Proposed fix
function encodeProjectName(projectPath) {
- return path.resolve(projectPath).replace(/[\\/:\s~_]/g, '-');
+ return path.resolve(projectPath).replace(/\//g, '-');
}If Windows/drive-letter support is desired, handle : and \\ with distinct escape sequences (or a separate, reversible scheme) rather than collapsing them into -, otherwise paths that differ only in these characters will collide.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function encodeProjectName(projectPath) { | |
| return path.resolve(projectPath).replace(/[\\/:\s~_]/g, '-'); | |
| } | |
| function encodeProjectName(projectPath) { | |
| return path.resolve(projectPath).replace(/\//g, '-'); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/projects.js` around lines 236 - 238, The encodeProjectName function is
overly aggressive—change it to only perform Claude's established '/' → '-'
encoding (after path.resolve normalization) instead of replacing _ , whitespace,
:, ~, or backslashes; update encodeProjectName to preserve underscores and other
characters so generated keys match the decoders that call
projectName.replace(/-/g, '/'), and if Windows drive-letter handling is
required, implement a separate reversible scheme for ':' and '\\' rather than
collapsing them into '-' so keys remain unique and reversible for functions like
mergeDiscoveredProjectsIntoConfig, addProjectManually, and
extractProjectDirectory.
Comment on lines
+16
to
+36
| for (let i = 0; i < argv.length; i++) { | ||
| const arg = argv[i]; | ||
| if (arg === '--host') { | ||
| parsed.host = argv[++i]; | ||
| } else if (arg.startsWith('--host=')) { | ||
| parsed.host = arg.split('=')[1]; | ||
| } else if (arg === '--port' || arg === '-p') { | ||
| parsed.port = Number(argv[++i]); | ||
| } else if (arg.startsWith('--port=')) { | ||
| parsed.port = Number(arg.split('=')[1]); | ||
| } else if (arg === '--strictPort') { | ||
| parsed.strictPort = true; | ||
| } else if (arg === '--no-strictPort') { | ||
| parsed.strictPort = false; | ||
| } else if (arg === '--help' || arg === '-h') { | ||
| console.log('Usage: node server/vite-daemon.js [--host 0.0.0.0] [--port 5173] [--strictPort]'); | ||
| process.exit(0); | ||
| } else { | ||
| throw new Error(`Unknown argument: ${arg}`); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Minor: argument without value silently produces undefined, and fatal parse errors loop under systemd.
parsed.host = argv[++i] / parsed.port = Number(argv[++i]) don't check i+1 < argv.length, so node vite-daemon.js --host yields host=undefined and node vite-daemon.js --port yields port=NaN (the latter is caught later, the former isn't validated). Combined with Restart=always/RestartSec=2 in the generated frontend unit, a typo propagated via cloudcli daemon install puts the service into a tight restart loop. Consider validating that the next token exists and is not another flag before consuming it, and/or adding a small startup delay/exit code that systemd can treat as non-retriable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/vite-daemon.js` around lines 16 - 36, The argument parsing loop in
server/vite-daemon.js assigns parsed.host and parsed.port from argv[++i] without
checking that a next token exists or isn't another flag, causing undefined/NaN
and tight systemd restart loops; update the for-loop handling of
'--host'/'--port' (and their '=') to first verify i+1 < argv.length and that
argv[i+1] does not start with '-' before consuming it, otherwise emit a clear
error (or print usage) and exit non-zero; ensure parsed.strictPort handling
remains unchanged and consider returning a non-retriable exit code or a short
delay before exit to avoid tight systemd restarts.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
server/daemon-manager.js (1)
617-626:⚠️ Potential issue | 🟡 Minor
daemon doctor --mode userstill aborts before the doctor branch can run.The
mode === 'user' && !userBus.okguard at lines 617–626 still fires before theparsed.subcommand === 'doctor'branch at line 628, so runningcloudcli daemon doctor --mode useron a host with an unreachable user bus throws the generic user-session error instead of rendering the diagnostics the doctor was designed to produce. Reorder sodoctorruns (it already handles!userBus.okgracefully), then apply the throw.🛠️ Suggested reorder
- if (mode === 'user' && !userBus.ok) { - const installSystemCommand = buildDaemonCliCommand( - { subcommand: 'install', mode: 'system' }, - daemonCommandContext - ); - throw new Error( - `Could not connect to your systemd user session.\n${userBus.detail}\n` + - `Try ${c.bright(installSystemCommand)} or enable user linger: ${c.bright(`sudo loginctl enable-linger ${os.userInfo().username}`)}` - ); - } - if (parsed.subcommand === 'doctor') { // ... existing doctor body ... return; } + + if (mode === 'user' && !userBus.ok) { + const installSystemCommand = buildDaemonCliCommand( + { subcommand: 'install', mode: 'system' }, + daemonCommandContext + ); + throw new Error( + `Could not connect to your systemd user session.\n${userBus.detail}\n` + + `Try ${c.bright(installSystemCommand)} or enable user linger: ${c.bright(`sudo loginctl enable-linger ${os.userInfo().username}`)}` + ); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/daemon-manager.js` around lines 617 - 626, The guard that throws when mode === 'user' && !userBus.ok runs before the parsed.subcommand === 'doctor' branch, causing `daemon doctor --mode user` to abort early; move or change the check so the doctor branch is evaluated first (or exempt parsed.subcommand === 'doctor' from the throw) — modify the conditional around userBus (referencing mode, userBus.ok, and parsed.subcommand) so that if parsed.subcommand === 'doctor' the code enters the doctor handling logic (which already handles !userBus.ok) and only throw the generic user-session error for other subcommands.
🧹 Nitpick comments (1)
server/cli.js (1)
647-674: Duplicate port-reachability logic withdaemon-manager.js.
isPortOpen/waitForPortOpenhere are functionally identical toisPortReachable/waitForHealth's TCP fallback inserver/daemon-manager.js(lines 415–430, 451–466). Consider exporting a single helper fromdaemon-manager.js(or a sharedserver/utils/module) and importing it here to keep the timeout/behavior semantics aligned as the code evolves.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/cli.js` around lines 647 - 674, Duplicate TCP port-check logic exists: replace local isPortOpen and waitForPortOpen with a shared helper by exporting the existing TCP fallback functions from daemon-manager.js (isPortReachable / waitForHealth TCP fallback) or move that logic to a new shared utility module and import it into this file; update all callers here to use the exported helper (e.g., import isPortReachable or waitForHealth-equivalent and call it instead of isPortOpen/waitForPortOpen) so timeout values and behavior remain consistent across both modules.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/cli.js`:
- Around line 761-769: The code currently calls waitForPortOpen(daemonPort,
25_000) before checking isSystemPermissionError(systemError), causing
unnecessary 25s delays for known permission failures; change the control flow in
the catch block (around handleDaemonCommand(['install','--mode=system',...])
error handling) to first test isSystemPermissionError(systemError) and, if true,
skip or only briefly probe the port (e.g. a short timeout or no wait) so the
user-mode fallback runs fast; keep the long 25_000 wait only for
ambiguous/non-permission errors where a delayed daemon start is plausible, and
apply the same refactor to the analogous user-fallback catch path that currently
calls waitForPortOpen before checking isSystemPermissionError (references:
waitForPortOpen, isSystemPermissionError, handleDaemonCommand, daemonPort,
printSystemDaemonActiveNotice).
- Around line 263-274: The restart call is hardcoded to '--mode=system' which
will fail when only a user daemon is installed; update the invocation of
handleDaemonCommand in the restart branch to use '--mode=auto' instead of
'--mode=system' so resolveDaemonMode can pick user or system appropriately
(affecting the restart path that currently checks hasInstalledDaemonUnit());
also update the user-facing hint text elsewhere that prints the restart command
(the "restart --mode system" hint) to suggest '--mode=auto' so users get the
correct command.
In `@server/daemon-manager.js`:
- Around line 451-466: The TCP fallback in waitForHealth can mark a
stale/foreign listener healthy too early; update waitForHealth (which calls
requestHealthOnce and isPortReachable) to not immediately return {ok: true,
probe: 'tcp'} on first TCP success—instead track when the first HTTP failure
occurred and only accept TCP fallback after a minimum HTTP-failure window (e.g.,
3–5s), or alternatively return a non-success result (e.g., { ok: false, probe:
'tcp' }) so healthCheckOrThrow still performs its journalctl dump; implement the
chosen approach inside waitForHealth (use a local firstHttpFailureTimestamp
variable and DAEMON_TCP_FALLBACK_DELAY_MS or change the returned ok flag) and
update callers (healthCheckOrThrow) to treat probe:'tcp' as "unverified" if you
choose the non-success return.
---
Duplicate comments:
In `@server/daemon-manager.js`:
- Around line 617-626: The guard that throws when mode === 'user' && !userBus.ok
runs before the parsed.subcommand === 'doctor' branch, causing `daemon doctor
--mode user` to abort early; move or change the check so the doctor branch is
evaluated first (or exempt parsed.subcommand === 'doctor' from the throw) —
modify the conditional around userBus (referencing mode, userBus.ok, and
parsed.subcommand) so that if parsed.subcommand === 'doctor' the code enters the
doctor handling logic (which already handles !userBus.ok) and only throw the
generic user-session error for other subcommands.
---
Nitpick comments:
In `@server/cli.js`:
- Around line 647-674: Duplicate TCP port-check logic exists: replace local
isPortOpen and waitForPortOpen with a shared helper by exporting the existing
TCP fallback functions from daemon-manager.js (isPortReachable / waitForHealth
TCP fallback) or move that logic to a new shared utility module and import it
into this file; update all callers here to use the exported helper (e.g., import
isPortReachable or waitForHealth-equivalent and call it instead of
isPortOpen/waitForPortOpen) so timeout values and behavior remain consistent
across both modules.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 83b1cd9e-5286-4c4d-848d-d8e5f52402cd
📒 Files selected for processing (3)
server/cli.jsserver/daemon-manager.jsserver/index.js
🚧 Files skipped from review as they are similar to previous changes (1)
- server/index.js
Comment on lines
+263
to
+274
| if (options.restartDaemon) { | ||
| if (!hasInstalledDaemonUnit()) { | ||
| console.log(`${c.warn('[WARN]')} No daemon unit detected; skipping restart.`); | ||
| return; | ||
| } | ||
| console.log(`${c.info('[INFO]')} Restarting daemon service...`); | ||
| await handleDaemonCommand(['restart', '--mode=system'], { | ||
| appRoot: APP_ROOT, | ||
| defaultPort: process.env.SERVER_PORT || process.env.PORT || '3001', | ||
| color: c, | ||
| }); | ||
| console.log(`${c.ok('[OK]')} Daemon restart completed.`); |
Contributor
There was a problem hiding this comment.
Hardcoded --mode=system will fail user-only daemon installs.
hasInstalledDaemonUnit() returns true if any of the four unit files (user/system × backend/frontend) exist, but the restart call is pinned to --mode=system. On a machine where only the user daemon was installed (e.g., via cloudcli daemon install --mode user), systemctl enable/restart cloudcli.service will fail because the system unit file doesn't exist, and the update will report a restart error even though a perfectly valid user daemon is running.
Use --mode=auto so resolveDaemonMode picks system when the system unit is present and user otherwise.
🛠️ Proposed fix
if (options.restartDaemon) {
if (!hasInstalledDaemonUnit()) {
console.log(`${c.warn('[WARN]')} No daemon unit detected; skipping restart.`);
return;
}
console.log(`${c.info('[INFO]')} Restarting daemon service...`);
- await handleDaemonCommand(['restart', '--mode=system'], {
+ await handleDaemonCommand(['restart', '--mode=auto'], {
appRoot: APP_ROOT,
defaultPort: process.env.SERVER_PORT || process.env.PORT || '3001',
color: c,
+ cliEntry: process.argv[1],
+ nodeExecPath: process.execPath,
});
console.log(`${c.ok('[OK]')} Daemon restart completed.`);
}Also note: the hint on line 281 surfaces a restart --mode system command, which has the same limitation; consider --mode auto there too.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/cli.js` around lines 263 - 274, The restart call is hardcoded to
'--mode=system' which will fail when only a user daemon is installed; update the
invocation of handleDaemonCommand in the restart branch to use '--mode=auto'
instead of '--mode=system' so resolveDaemonMode can pick user or system
appropriately (affecting the restart path that currently checks
hasInstalledDaemonUnit()); also update the user-facing hint text elsewhere that
prints the restart command (the "restart --mode system" hint) to suggest
'--mode=auto' so users get the correct command.
Comment on lines
+761
to
+769
| } catch (systemError) { | ||
| const healthySoon = await waitForPortOpen(daemonPort); | ||
| if (healthySoon) { | ||
| console.log(`${c.warn('[WARN]')} System daemon health check was delayed, but port ${daemonPort} is now reachable.`); | ||
| printSystemDaemonActiveNotice(daemonPort); | ||
| return true; | ||
| } | ||
|
|
||
| if (!isSystemPermissionError(systemError)) { |
Contributor
There was a problem hiding this comment.
Unnecessary 25 s wait on non-permission errors before escalating or falling back.
When handleDaemonCommand(['install', '--mode=system', ...]) throws, waitForPortOpen(daemonPort, 25_000) is invoked unconditionally before isSystemPermissionError(systemError) is consulted. On a fresh host with no prior daemon and a clear permission denied/non-permission install failure, the port will never become reachable, so users pay a full 25 s delay before seeing the real error or the user-mode fallback.
Short-circuit when the error is clearly non-recoverable at this level, e.g. probe only briefly (or skip it) if isSystemPermissionError(systemError) is true (so we fall through to the user fallback fast), and keep the longer wait only for ambiguous cases. Same applies to the user-fallback path at line 797.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/cli.js` around lines 761 - 769, The code currently calls
waitForPortOpen(daemonPort, 25_000) before checking
isSystemPermissionError(systemError), causing unnecessary 25s delays for known
permission failures; change the control flow in the catch block (around
handleDaemonCommand(['install','--mode=system',...]) error handling) to first
test isSystemPermissionError(systemError) and, if true, skip or only briefly
probe the port (e.g. a short timeout or no wait) so the user-mode fallback runs
fast; keep the long 25_000 wait only for ambiguous/non-permission errors where a
delayed daemon start is plausible, and apply the same refactor to the analogous
user-fallback catch path that currently calls waitForPortOpen before checking
isSystemPermissionError (references: waitForPortOpen, isSystemPermissionError,
handleDaemonCommand, daemonPort, printSystemDaemonActiveNotice).
Comment on lines
+451
to
+466
| async function waitForHealth(port, timeoutMs = DAEMON_HEALTH_TIMEOUT_MS) { | ||
| const deadline = Date.now() + timeoutMs; | ||
| let lastError = null; | ||
| while (Date.now() < deadline) { | ||
| try { | ||
| return await requestHealthOnce(port); | ||
| } catch (error) { | ||
| lastError = error; | ||
| if (await isPortReachable(port, 700)) { | ||
| return { ok: true, statusCode: null, probe: 'tcp' }; | ||
| } | ||
| await sleep(DAEMON_HEALTH_RETRY_INTERVAL_MS); | ||
| } | ||
| } | ||
| throw new Error(lastError?.message || `Service did not become healthy on port ${port}`); | ||
| } |
Contributor
There was a problem hiding this comment.
TCP fallback in waitForHealth can report a stale/foreign listener as healthy.
If /health keeps erroring (e.g., the app crashes in a loop but systemd hasn't torn down the socket yet, or an unrelated process is already bound to the port), isPortReachable(port, 700) will succeed and waitForHealth returns { ok: true, probe: 'tcp' } on the very first iteration. That then skips the journalctl error dump in healthCheckOrThrow and misleads the installer into thinking the daemon is healthy.
Consider only accepting the TCP fallback after HTTP has failed for a minimum duration (e.g., retry HTTP for several seconds first), or treating probe: 'tcp' as "port bound but health endpoint unverified" and continuing to retry HTTP rather than returning success immediately.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/daemon-manager.js` around lines 451 - 466, The TCP fallback in
waitForHealth can mark a stale/foreign listener healthy too early; update
waitForHealth (which calls requestHealthOnce and isPortReachable) to not
immediately return {ok: true, probe: 'tcp'} on first TCP success—instead track
when the first HTTP failure occurred and only accept TCP fallback after a
minimum HTTP-failure window (e.g., 3–5s), or alternatively return a non-success
result (e.g., { ok: false, probe: 'tcp' }) so healthCheckOrThrow still performs
its journalctl dump; implement the chosen approach inside waitForHealth (use a
local firstHttpFailureTimestamp variable and DAEMON_TCP_FALLBACK_DELAY_MS or
change the returned ok flag) and update callers (healthCheckOrThrow) to treat
probe:'tcp' as "unverified" if you choose the non-success return.
Collaborator
|
Hey @alicomert , thanks for the PR. There has been some big refactor recently around workspace and session fetching and a lot of code has moved around. Can you take a look at the current setup and see if it resolves the issues? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
WORKSPACES_BASEdefault (<home>/cloudcli/projects) and normalize short workspace paths into that base/root/<base>carve-out for root deployments~resolves to the managed workspace baseWhy this change
Users reported two core issues:
This patch keeps security boundaries but makes project creation and discovery practical for real deployments.
Validation
npm run typechecknpm run buildNotes
projects.detectedFromHistorywas added for all shipped locales.Summary by CodeRabbit
New Features
Documentation