feat: add deus-machine CLI — npm install, headless server, desktop installer#227
Merged
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new cross‑platform CLI package ( Changes
Sequence Diagram(s)sequenceDiagram
participant User as Terminal
participant CLI as Deus CLI
participant Onboard as Onboarding
participant Config as Config Store
participant Agent as Agent Server (bundled)
participant Backend as Backend Server (bundled)
User->>CLI: run `deus start` (or auto-detect)
CLI->>Onboard: hasCompletedOnboarding?
alt not completed
CLI->>Onboard: runOnboarding()
Onboard->>Config: save onboarding/auth/relay
end
CLI->>Agent: spawn agent-server (child)
Agent-->>CLI: LISTEN_URL token
CLI->>Backend: spawn backend with AGENT_SERVER_URL
Backend-->>CLI: emits [BACKEND_PORT] marker
CLI->>Config: writeServerInfo({ pid, port, url })
CLI->>User: print "Deus is running" + pairing/status
User->>CLI: SIGINT/SIGTERM
CLI->>Agent: forward SIGTERM
CLI->>Backend: forward SIGTERM
CLI->>Config: clearServerInfo()
CLI->>User: exit
sequenceDiagram
participant GH as GitHub Actions
participant Repo as Checkout
participant Builder as Build Job (Bun/Node)
participant Registry as npm Registry
GH->>Repo: checkout release tag
GH->>Builder: setup Node/Bun, install deps, build CLI (apps/cli)
Builder->>Repo: read `apps/cli/package.json`
Builder->>Registry: npm publish `apps/cli` (authenticated via NODE_AUTH_TOKEN)
Registry-->>GH: publish success/failure
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (8)
apps/cli/src/desktop.ts (1)
184-213: Consider adding User-Agent header to redirect request.The initial GitHub API request includes proper headers, but the redirect follow on line 198 only sets
User-AgentwithoutAcceptheader. While this works, consistency would be better.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/src/desktop.ts` around lines 184 - 213, In fetchRelease, the redirected GET call only sets "User-Agent" and omits the "Accept" header; update the redirected request (the second get invoked when loc is present) to include the same headers object used in the initial request (both "User-Agent": "deus-cli" and "Accept": "application/vnd.github.v3+json") so the follow-up request uses consistent headers when parsing the GitHub API response.scripts/bump-version.sh (1)
25-31: Consider logging when the CLI package is skipped.The conditional check is good defensive coding, but silently skipping the CLI version bump could mask issues during release. If
apps/cli/package.jsonis expected to always exist in release contexts, consider logging a warning when it's missing.🔧 Suggested improvement
# 2. apps/cli/package.json (npm publish uses this) CLI_PKG="$REPO_ROOT/apps/cli/package.json" if [ -f "$CLI_PKG" ]; then jq --arg v "$VERSION" '.version = $v' "$CLI_PKG" > "$CLI_PKG.tmp" mv "$CLI_PKG.tmp" "$CLI_PKG" echo " apps/cli/package.json -> $VERSION" +else + echo " apps/cli/package.json not found (skipping)" fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/bump-version.sh` around lines 25 - 31, The script silently skips bumping apps/cli/package.json when the file does not exist; update the if [ -f "$CLI_PKG" ] check around the CLI_PKG variable to add an else branch that logs a warning to stderr (or using echo) indicating the CLI package was not found and the version bump was skipped, including the expected path ($CLI_PKG) and the intended $VERSION so CI/release owners can diagnose missing files; keep the existing success path (jq -> tmp -> mv -> echo) unchanged..husky/_/pre-push (1)
1-5: Minor inconsistency: missing stderr suppression.The sibling hooks (
prepare-commit-msg,post-commit) suppress stderr with2>/dev/null, but this hook does not. Consider adding it for consistency to avoid noise in the terminal if theentireCLI is unavailable.🔧 Suggested fix for consistency
#!/bin/sh # Entire CLI hooks # Pre-push hook: push session logs alongside user's push # $1 is the remote name (e.g., "origin") -entire hooks git pre-push "$1" || true +entire hooks git pre-push "$1" 2>/dev/null || true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.husky/_/pre-push around lines 1 - 5, The pre-push hook currently invokes the entire CLI without suppressing stderr ("entire hooks git pre-push \"$1\" || true"), causing inconsistent terminal noise compared to sibling hooks (prepare-commit-msg, post-commit); update the command to redirect stderr to /dev/null by appending "2>/dev/null" so the hook becomes consistent and silent when the entire CLI is unavailable.apps/cli/build.ts (1)
97-104: Dead code: modifiedpkgobject is never written.Step 4 reads and modifies
package.jsonbut never writes it back. The comment says "the original is fine for publishing" which means lines 99-103 have no effect. Consider removing this dead code or adding a comment explaining if it's intentional prep for future use.♻️ Option: Remove dead code
- // Step 4: Generate a standalone package.json for the published CLI - console.log("\n4. Generating publish-ready package.json..."); - const pkg = JSON.parse(readFileSync(join(cliDir, "package.json"), "utf-8")); - // Ensure we don't ship "private: true" or monorepo-specific fields - delete pkg.scripts; - pkg.main = "dist/cli.js"; - pkg.type = "module"; - // Note: we don't overwrite — the original is fine for publishing - console.log("\n✓ Build complete!");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/build.ts` around lines 97 - 104, The snippet reads and mutates a local pkg object (via readFileSync and JSON.parse) but never writes it back, making the changes dead code; either remove the entire block that reads and mutates pkg (the lines touching pkg, delete pkg.scripts, pkg.main, pkg.type) if the change is intentionally not published, or persist the modified package.json by writing pkg back (use writeFileSync(join(cliDir, "package.json"), JSON.stringify(pkg, null, 2)) or similar) so the modifications to pkg take effect; reference the variables and functions pkg, readFileSync, join, cliDir, delete pkg.scripts, pkg.main, and pkg.type when locating the code to change.apps/cli/src/start.ts (1)
326-335: Consider a more robust Electron detection.The check
command.includes("Electron")(line 328) relies on the path string containing "Electron". This could break if Electron is installed in a directory with a different name or symlinked.♻️ More robust approach
const processEnv: Record<string, string> = {}; - if (command !== process.execPath && command.includes("Electron")) { + // Set ELECTRON_RUN_AS_NODE if running via a different binary (likely Electron) + if (command !== process.execPath) { processEnv.ELECTRON_RUN_AS_NODE = "1"; }Setting
ELECTRON_RUN_AS_NODEfor any non-default binary is generally safe—it's a no-op for regular Node.js.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/src/start.ts` around lines 326 - 335, The current Electron detection uses command.includes("Electron"), which is brittle; instead, whenever the binary is not the default Node (i.e., command !== process.execPath) set ELECTRON_RUN_AS_NODE="1" (it's a no-op for normal Node), so update the block that constructs processEnv (remove the includes("Electron") check) and ensure the env passed to spawn (the env spread around processEnv) still includes ELECTRON_RUN_AS_NODE when command !== process.execPath; refer to the variables/functionality around command, process.execPath, processEnv, ELECTRON_RUN_AS_NODE and the spawn(...) call to make the change.apps/cli/src/config.ts (2)
97-103: Consider atomic write for server info consistency.
writeServerInfowrites directly to the target file, whilesaveConfiguses the atomic temp-file-then-rename pattern. For consistency and to avoid partial writes if the process is killed mid-write:♻️ Proposed fix for atomic write
export function writeServerInfo(info: ServerInfo): void { const dir = getConfigDir(); mkdirSync(dir, { recursive: true }); const path = getServerInfoPath(); - writeFileSync(path, JSON.stringify(info, null, 2) + "\n", "utf-8"); + const tmp = path + ".tmp." + randomBytes(4).toString("hex"); + try { + writeFileSync(tmp, JSON.stringify(info, null, 2) + "\n", "utf-8"); + renameSync(tmp, path); + } catch { + try { unlinkSync(tmp); } catch {} + throw new Error(`Failed to write server info to ${path}`); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/src/config.ts` around lines 97 - 103, writeServerInfo currently writes directly to the target path which can produce partial files; change it to perform an atomic write similar to saveConfig by writing the JSON into a temp file in the same directory (use getConfigDir() and getServerInfoPath() to derive names), fsync the temp file, close it, then rename the temp file to the final path (renameSync) to guarantee atomic replace; ensure mkdirSync(dir, { recursive: true }) still runs before creating the temp file and preserve UTF-8 encoding and the trailing newline in the written content.
113-120: PID check has a rare race condition with PID reuse.
process.kill(pid, 0)verifies a process exists but doesn't confirm it's the same process that wrote the server info. If the original process died and a new unrelated process was assigned the same PID, this returns stale/invalid server info.This is a low-probability edge case. If needed, you could store and verify the
startedAttimestamp or additional process metadata (e.g., process start time from/proc/<pid>/staton Linux).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/src/config.ts` around lines 113 - 120, The PID-only liveness check using process.kill(info.pid, 0) can be fooled by PID reuse; update the verification to also compare stored process metadata (e.g., info.startedAt or a stored startTimestamp) against the actual process start time or a timestamp observed when the server wrote its info before trusting it. Concretely: when writing server info include a startedAt (or startTimestamp) field, and in the check that currently uses process.kill(info.pid, 0) and calls clearServerInfo(), additionally read/validate that info.startedAt matches the current process start time (or compare it to /proc/<pid>/stat start time on Linux) and only treat the PID as live if both PID and start time match; otherwise call clearServerInfo() and return null. Reference: info.pid, info.startedAt, process.kill(...), clearServerInfo().apps/web/src/shared/config/backend.config.ts (1)
55-90: Usets-patternforDeploymentModebranching.This if/else chain on the discriminated union
DeploymentModeshould usets-patternwith.exhaustive()to catch missing cases at compile time. All four cases ("web-production", "web-standalone", "electron", "web-dev") are currently handled, but the last two lack explicit branches and rely on implicit fallback.♻️ Refactor sketch
+import { match } from "ts-pattern"; + export async function resolveBackendEndpoints(serverId?: string): Promise<BackendEndpoints> { const mode = getDeploymentMode(); - - if (mode === "web-production") { - // In relay mode, serverId comes from the URL (/s/{serverId}/...) - // or can be passed explicitly. - const id = serverId || getServerIdFromUrl(); - if (!id) { - throw new Error("No server ID available for relay connection"); - } - // Cache is per-serverId — invalidate if serverId changes (strict equality) - if (cachedEndpoints && cachedRelayServerId === id) { - return cachedEndpoints; - } - const encodedId = encodeURIComponent(id); - cachedRelayServerId = id; - cachedEndpoints = { - wsUrl: `${RELAY_BASE_URL}/api/servers/${encodedId}/connect`, - // In relay mode, HTTP is tunneled over WS — apiBase is a placeholder - // that should never be used directly (client.ts intercepts requests). - apiBase: `${RELAY_BASE_URL}/api/servers/${encodedId}`, - }; - return cachedEndpoints; - } - - if (mode === "web-standalone") { - // Standalone mode: the CLI server proxies /api and /ws to the backend. - // Connect using the same origin the page was loaded from. - if (cachedEndpoints) return cachedEndpoints; - const wsProtocol = window.location.protocol === "https:" ? "wss:" : "ws:"; - cachedEndpoints = { - wsUrl: `${wsProtocol}//${window.location.host}/ws`, - apiBase: `${window.location.origin}/api`, - }; - return cachedEndpoints; - } - - // Electron or web-dev: resolve local port - if (cachedEndpoints) return cachedEndpoints; - - const port = await getBackendPort(); - cachedEndpoints = { - wsUrl: `ws://localhost:${port}/ws`, - apiBase: `http://localhost:${port}/api`, - }; - return cachedEndpoints; + + return match(mode) + .with("web-production", async () => { + const id = serverId || getServerIdFromUrl(); + if (!id) { + throw new Error("No server ID available for relay connection"); + } + if (cachedEndpoints && cachedRelayServerId === id) { + return cachedEndpoints; + } + const encodedId = encodeURIComponent(id); + cachedRelayServerId = id; + cachedEndpoints = { + wsUrl: `${RELAY_BASE_URL}/api/servers/${encodedId}/connect`, + apiBase: `${RELAY_BASE_URL}/api/servers/${encodedId}`, + }; + return cachedEndpoints; + }) + .with("web-standalone", async () => { + if (cachedEndpoints) return cachedEndpoints; + const wsProtocol = window.location.protocol === "https:" ? "wss:" : "ws:"; + cachedEndpoints = { + wsUrl: `${wsProtocol}//${window.location.host}/ws`, + apiBase: `${window.location.origin}/api`, + }; + return cachedEndpoints; + }) + .with("electron", "web-dev", async () => { + if (cachedEndpoints) return cachedEndpoints; + const port = await getBackendPort(); + cachedEndpoints = { + wsUrl: `ws://localhost:${port}/ws`, + apiBase: `http://localhost:${port}/api`, + }; + return cachedEndpoints; + }) + .exhaustive(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/shared/config/backend.config.ts` around lines 55 - 90, The function resolveBackendEndpoints uses an if/else chain on getDeploymentMode() — convert it to a ts-pattern match on getDeploymentMode() to make the DeploymentMode exhaustive and catch missing cases at compile time; replace the mode checks with a match(getDeploymentMode()).with("web-production", () => { ...build and return cachedEndpoints using cachedRelayServerId, encodeURIComponent(serverId), RELAY_BASE_URL... }), .with("web-standalone", () => { ...build and return cachedEndpoints using window.location, wsProtocol... }), add explicit branches for "electron" and "web-dev" that either construct appropriate endpoints or throw a clear Error/return a sensible default, and finish with .exhaustive() so the compiler forces handling of all DeploymentMode variants while reusing existing symbols resolveBackendEndpoints, cachedEndpoints and cachedRelayServerId.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.husky/_/commit-msg:
- Around line 1-4: The commit-msg hook (.husky/_/commit-msg) shells out to the
external "entire" CLI which is not installed or documented, causing commit
failures; fix by either (A) adding "entire" to devDependencies in package.json
(and run npm/yarn install as part of setup), update DEVELOPMENT.md to document
why "entire" is required and add it to any setup scripts (e.g.,
bootstrap/install scripts) so clones get it automatically, or (B) if the hook is
nonessential, remove or replace the line invoking entire in the commit-msg hook
with an internal script or npm script that replicates the needed behavior;
ensure references to the commit-msg hook and the "entire" command are updated
accordingly.
In `@apps/cli/src/config.ts`:
- Around line 16-22: The config currently stores the API key in plaintext via
the DeusConfig.anthropic_api_key field; update the implementation so the key is
not persisted in config.json: remove anthropic_api_key from saved JSON and add
get/set helpers that use an OS keychain (e.g., keytar) to store/retrieve the API
key (fall back to environment vars if keychain not available); where config is
written (functions like saveConfig or loadConfig), ensure you do not serialize
anthropic_api_key and, if you must keep a local file, set strict permissions
(fs.chmod 0o600) immediately after creating it; also add a short user-facing
note (CLI output or README) warning users about the remaining risk if they
choose to store keys locally.
- Around line 74-89: The saveConfig function writes the config file with default
permissions; update it to create the temporary file with restrictive mode and
ensure the final config file has restrictive permissions (e.g., 0o600) after
renaming. Specifically, when creating tmp in saveConfig use writeFileSync with
options that set mode to 0o600 (or alternatively call chmodSync on tmp before
rename), and after renameSync(path, tmp) call chmodSync(path, 0o600) to
guarantee the saved file is owner-readable/writable only; keep the existing
cleanup in the catch block unchanged.
In `@apps/cli/src/desktop.ts`:
- Around line 314-324: The Linux AppImage install branch (case "linux") can
throw from execSync, chmod, mkdirSync or cp; wrap the sequence that uses
filePath, installDir, destPath, execSync and mkdirSync in a try/catch, call
s.fail with a helpful message and include the caught error details, ensure the
function returns false (or an error code) on failure instead of letting the
exception escape, and only call launchDesktop(destPath) and s.succeed(...)
inside the try when all steps succeed.
In `@apps/cli/src/login.ts`:
- Around line 248-264: The installAndLogin function currently spawns "npm
install -g `@anthropic-ai/claude-code`"; replace that with the sanctioned bun
workflow by invoking the tool via bunx instead of npm (i.e., call spawnSync with
"bunx" and the package "@anthropic-ai/claude-code"), update the timeout/stdio
usage as needed, and change the user hint text and printed example from the npm
command to the bun equivalent (e.g., use bunx `@anthropic-ai/claude-code`) while
keeping the existing failure handling (s.fail, blank, hint, skipAuth) intact.
- Around line 135-140: The code returns config.auth_method cast to
AuthResult["method"] without runtime validation, which can break if config is
corrupted; update the logic around the opts?.force check to validate
config.auth_method against the allowed AuthResult method values (e.g., a small
whitelist/enum of valid methods) before returning it, and if the value is
invalid fall back to a safe default or treat it as not set (so the flow
continues to prompt or compute credentials); reference the existing symbols
config.auth_method, opts?.force, and AuthResult["method"] when adding the
validation and fallback handling.
In `@apps/cli/src/qr.ts`:
- Around line 461-471: The catch block in printQR currently swallows all QR
generation errors silently; update printQR to catch the thrown error from
generateQR (referencing the printQR function and generateQR call), log a clear
fallback message via console.error or console.log indicating the QR could not be
rendered and include the original data (or a truncated version) and the error
message for debugging, and optionally print the raw data/URL so the user can
still use it; keep the existing indent behavior for any printed fallback text.
In `@apps/web/src/shared/config/backend.config.ts`:
- Around line 80-90: cachedEndpoints is reused across modes so a stale endpoint
set can be returned when mode changes; update the logic around cachedEndpoints
to be keyed by mode (e.g., cachedEndpointsByMode[mode]) or clear/replace
cachedEndpoints whenever mode differs, and ensure the early-return uses the
mode-specific cache entry inside the branch handling "web-standalone" and the
other mode branches (refer to symbols cachedEndpoints and mode and the
"web-standalone"/"web-production" branches) so each mode always gets its correct
wsUrl/apiBase.
---
Nitpick comments:
In @.husky/_/pre-push:
- Around line 1-5: The pre-push hook currently invokes the entire CLI without
suppressing stderr ("entire hooks git pre-push \"$1\" || true"), causing
inconsistent terminal noise compared to sibling hooks (prepare-commit-msg,
post-commit); update the command to redirect stderr to /dev/null by appending
"2>/dev/null" so the hook becomes consistent and silent when the entire CLI is
unavailable.
In `@apps/cli/build.ts`:
- Around line 97-104: The snippet reads and mutates a local pkg object (via
readFileSync and JSON.parse) but never writes it back, making the changes dead
code; either remove the entire block that reads and mutates pkg (the lines
touching pkg, delete pkg.scripts, pkg.main, pkg.type) if the change is
intentionally not published, or persist the modified package.json by writing pkg
back (use writeFileSync(join(cliDir, "package.json"), JSON.stringify(pkg, null,
2)) or similar) so the modifications to pkg take effect; reference the variables
and functions pkg, readFileSync, join, cliDir, delete pkg.scripts, pkg.main, and
pkg.type when locating the code to change.
In `@apps/cli/src/config.ts`:
- Around line 97-103: writeServerInfo currently writes directly to the target
path which can produce partial files; change it to perform an atomic write
similar to saveConfig by writing the JSON into a temp file in the same directory
(use getConfigDir() and getServerInfoPath() to derive names), fsync the temp
file, close it, then rename the temp file to the final path (renameSync) to
guarantee atomic replace; ensure mkdirSync(dir, { recursive: true }) still runs
before creating the temp file and preserve UTF-8 encoding and the trailing
newline in the written content.
- Around line 113-120: The PID-only liveness check using process.kill(info.pid,
0) can be fooled by PID reuse; update the verification to also compare stored
process metadata (e.g., info.startedAt or a stored startTimestamp) against the
actual process start time or a timestamp observed when the server wrote its info
before trusting it. Concretely: when writing server info include a startedAt (or
startTimestamp) field, and in the check that currently uses
process.kill(info.pid, 0) and calls clearServerInfo(), additionally
read/validate that info.startedAt matches the current process start time (or
compare it to /proc/<pid>/stat start time on Linux) and only treat the PID as
live if both PID and start time match; otherwise call clearServerInfo() and
return null. Reference: info.pid, info.startedAt, process.kill(...),
clearServerInfo().
In `@apps/cli/src/desktop.ts`:
- Around line 184-213: In fetchRelease, the redirected GET call only sets
"User-Agent" and omits the "Accept" header; update the redirected request (the
second get invoked when loc is present) to include the same headers object used
in the initial request (both "User-Agent": "deus-cli" and "Accept":
"application/vnd.github.v3+json") so the follow-up request uses consistent
headers when parsing the GitHub API response.
In `@apps/cli/src/start.ts`:
- Around line 326-335: The current Electron detection uses
command.includes("Electron"), which is brittle; instead, whenever the binary is
not the default Node (i.e., command !== process.execPath) set
ELECTRON_RUN_AS_NODE="1" (it's a no-op for normal Node), so update the block
that constructs processEnv (remove the includes("Electron") check) and ensure
the env passed to spawn (the env spread around processEnv) still includes
ELECTRON_RUN_AS_NODE when command !== process.execPath; refer to the
variables/functionality around command, process.execPath, processEnv,
ELECTRON_RUN_AS_NODE and the spawn(...) call to make the change.
In `@apps/web/src/shared/config/backend.config.ts`:
- Around line 55-90: The function resolveBackendEndpoints uses an if/else chain
on getDeploymentMode() — convert it to a ts-pattern match on getDeploymentMode()
to make the DeploymentMode exhaustive and catch missing cases at compile time;
replace the mode checks with a match(getDeploymentMode()).with("web-production",
() => { ...build and return cachedEndpoints using cachedRelayServerId,
encodeURIComponent(serverId), RELAY_BASE_URL... }), .with("web-standalone", ()
=> { ...build and return cachedEndpoints using window.location, wsProtocol...
}), add explicit branches for "electron" and "web-dev" that either construct
appropriate endpoints or throw a clear Error/return a sensible default, and
finish with .exhaustive() so the compiler forces handling of all DeploymentMode
variants while reusing existing symbols resolveBackendEndpoints, cachedEndpoints
and cachedRelayServerId.
In `@scripts/bump-version.sh`:
- Around line 25-31: The script silently skips bumping apps/cli/package.json
when the file does not exist; update the if [ -f "$CLI_PKG" ] check around the
CLI_PKG variable to add an else branch that logs a warning to stderr (or using
echo) indicating the CLI package was not found and the version bump was skipped,
including the expected path ($CLI_PKG) and the intended $VERSION so CI/release
owners can diagnose missing files; keep the existing success path (jq -> tmp ->
mv -> echo) unchanged.
🪄 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: defaults
Review profile: CHILL
Plan: Lite
Run ID: 78489177-2894-45f2-9857-e32381e522e9
📒 Files selected for processing (27)
.github/workflows/release.yml.gitignore.husky/_/commit-msg.husky/_/post-commit.husky/_/pre-push.husky/_/prepare-commit-msgapps/cli/README.mdapps/cli/build.tsapps/cli/package.jsonapps/cli/src/cli.tsapps/cli/src/config.tsapps/cli/src/desktop.tsapps/cli/src/lib/format.tsapps/cli/src/lib/http.tsapps/cli/src/login.tsapps/cli/src/onboarding.tsapps/cli/src/pair.tsapps/cli/src/prompt.tsapps/cli/src/qr.tsapps/cli/src/start.tsapps/cli/src/status.tsapps/cli/src/ui.tsapps/cli/tsconfig.jsonapps/web/index.htmlapps/web/src/shared/config/backend.config.tspackage.jsonscripts/bump-version.sh
Comment on lines
+135
to
+140
| if (!opts?.force && config.auth_method) { | ||
| return { | ||
| method: config.auth_method as AuthResult["method"], | ||
| apiKey: config.anthropic_api_key, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Type assertion may fail on corrupted config.
If config.auth_method contains an unexpected string (from manual editing or version mismatch), the cast as AuthResult["method"] won't catch it at runtime. Consider validating the value:
🛡️ Proposed fix to validate auth method
+const VALID_AUTH_METHODS = ["claude_cli", "api_key", "env", "skipped"] as const;
+
export async function runAuthSetup(opts?: { force?: boolean }): Promise<AuthResult> {
const config = loadConfig();
// If not forced and already configured, return saved config
- if (!opts?.force && config.auth_method) {
+ if (!opts?.force && config.auth_method && VALID_AUTH_METHODS.includes(config.auth_method as any)) {
return {
method: config.auth_method as AuthResult["method"],
apiKey: config.anthropic_api_key,
};
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/cli/src/login.ts` around lines 135 - 140, The code returns
config.auth_method cast to AuthResult["method"] without runtime validation,
which can break if config is corrupted; update the logic around the opts?.force
check to validate config.auth_method against the allowed AuthResult method
values (e.g., a small whitelist/enum of valid methods) before returning it, and
if the value is invalid fall back to a safe default or treat it as not set (so
the flow continues to prompt or compute credentials); reference the existing
symbols config.auth_method, opts?.force, and AuthResult["method"] when adding
the validation and fallback handling.
Comment on lines
+248
to
+264
| async function installAndLogin(): Promise<AuthResult> { | ||
| blank(); | ||
| const s = createSpinner("Installing Claude Code CLI..."); | ||
|
|
||
| const result = spawnSync("npm", ["install", "-g", "@anthropic-ai/claude-code"], { | ||
| stdio: ["pipe", "pipe", "pipe"], | ||
| timeout: 120_000, | ||
| }); | ||
|
|
||
| if (result.status !== 0) { | ||
| s.fail("Installation failed"); | ||
| blank(); | ||
| hint("Try installing manually:"); | ||
| console.log(` ${c.cyan("npm install -g @anthropic-ai/claude-code")}`); | ||
| blank(); | ||
| return skipAuth(); | ||
| } |
There was a problem hiding this comment.
Use bun instead of npm per coding guidelines.
Line 252 uses npm install -g but the coding guidelines state: "Always use bun for package management... Never use npm or yarn."
♻️ Proposed fix
async function installAndLogin(): Promise<AuthResult> {
blank();
const s = createSpinner("Installing Claude Code CLI...");
- const result = spawnSync("npm", ["install", "-g", "@anthropic-ai/claude-code"], {
+ const result = spawnSync("bun", ["add", "-g", "@anthropic-ai/claude-code"], {
stdio: ["pipe", "pipe", "pipe"],
timeout: 120_000,
});
if (result.status !== 0) {
s.fail("Installation failed");
blank();
hint("Try installing manually:");
- console.log(` ${c.cyan("npm install -g `@anthropic-ai/claude-code`")}`);
+ console.log(` ${c.cyan("bun add -g `@anthropic-ai/claude-code`")}`);
blank();
return skipAuth();
}As per coding guidelines: "Always use bun for package management (bun add, bun install), running scripts (bun run), and executing tools (bunx). Never use npm or yarn."
📝 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
| async function installAndLogin(): Promise<AuthResult> { | |
| blank(); | |
| const s = createSpinner("Installing Claude Code CLI..."); | |
| const result = spawnSync("npm", ["install", "-g", "@anthropic-ai/claude-code"], { | |
| stdio: ["pipe", "pipe", "pipe"], | |
| timeout: 120_000, | |
| }); | |
| if (result.status !== 0) { | |
| s.fail("Installation failed"); | |
| blank(); | |
| hint("Try installing manually:"); | |
| console.log(` ${c.cyan("npm install -g @anthropic-ai/claude-code")}`); | |
| blank(); | |
| return skipAuth(); | |
| } | |
| async function installAndLogin(): Promise<AuthResult> { | |
| blank(); | |
| const s = createSpinner("Installing Claude Code CLI..."); | |
| const result = spawnSync("bun", ["add", "-g", "@anthropic-ai/claude-code"], { | |
| stdio: ["pipe", "pipe", "pipe"], | |
| timeout: 120_000, | |
| }); | |
| if (result.status !== 0) { | |
| s.fail("Installation failed"); | |
| blank(); | |
| hint("Try installing manually:"); | |
| console.log(` ${c.cyan("bun add -g `@anthropic-ai/claude-code`")}`); | |
| blank(); | |
| return skipAuth(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/cli/src/login.ts` around lines 248 - 264, The installAndLogin function
currently spawns "npm install -g `@anthropic-ai/claude-code`"; replace that with
the sanctioned bun workflow by invoking the tool via bunx instead of npm (i.e.,
call spawnSync with "bunx" and the package "@anthropic-ai/claude-code"), update
the timeout/stdio usage as needed, and change the user hint text and printed
example from the npm command to the bun equivalent (e.g., use bunx
`@anthropic-ai/claude-code`) while keeping the existing failure handling (s.fail,
blank, hint, skipAuth) intact.
Comment on lines
+80
to
+90
| if (mode === "web-standalone") { | ||
| // Standalone mode: the CLI server proxies /api and /ws to the backend. | ||
| // Connect using the same origin the page was loaded from. | ||
| if (cachedEndpoints) return cachedEndpoints; | ||
| const wsProtocol = window.location.protocol === "https:" ? "wss:" : "ws:"; | ||
| cachedEndpoints = { | ||
| wsUrl: `${wsProtocol}//${window.location.host}/ws`, | ||
| apiBase: `${window.location.origin}/api`, | ||
| }; | ||
| return cachedEndpoints; | ||
| } |
There was a problem hiding this comment.
Cache is not keyed by deployment mode, so stale endpoints can be returned.
At Line 83 and Line 93, cachedEndpoints is reused without confirming the mode it was built for. If the app transitions between web-production and web-standalone routes in-session, this can return the wrong endpoint set.
🛠️ Suggested fix
let cachedEndpoints: BackendEndpoints | null = null;
+let cachedEndpointsMode: DeploymentMode | null = null;
/** The serverId that the cached endpoints were built for (relay mode only). */
let cachedRelayServerId: string | null = null;
export async function resolveBackendEndpoints(serverId?: string): Promise<BackendEndpoints> {
const mode = getDeploymentMode();
if (mode === "web-production") {
@@
- if (cachedEndpoints && cachedRelayServerId === id) {
+ if (
+ cachedEndpoints &&
+ cachedEndpointsMode === "web-production" &&
+ cachedRelayServerId === id
+ ) {
return cachedEndpoints;
}
@@
const encodedId = encodeURIComponent(id);
cachedRelayServerId = id;
+ cachedEndpointsMode = "web-production";
cachedEndpoints = {
@@
if (mode === "web-standalone") {
@@
- if (cachedEndpoints) return cachedEndpoints;
+ if (cachedEndpoints && cachedEndpointsMode === "web-standalone") return cachedEndpoints;
const wsProtocol = window.location.protocol === "https:" ? "wss:" : "ws:";
+ cachedEndpointsMode = "web-standalone";
cachedEndpoints = {
wsUrl: `${wsProtocol}//${window.location.host}/ws`,
apiBase: `${window.location.origin}/api`,
@@
// Electron or web-dev: resolve local port
- if (cachedEndpoints) return cachedEndpoints;
+ if (cachedEndpoints && cachedEndpointsMode === mode) return cachedEndpoints;
const port = await getBackendPort();
+ cachedEndpointsMode = mode;
cachedEndpoints = {
wsUrl: `ws://localhost:${port}/ws`,
apiBase: `http://localhost:${port}/api`,
@@
function invalidateEndpointCache(): void {
cachedEndpoints = null;
+ cachedEndpointsMode = null;
cachedRelayServerId = null;
}Also applies to: 93-93
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/shared/config/backend.config.ts` around lines 80 - 90,
cachedEndpoints is reused across modes so a stale endpoint set can be returned
when mode changes; update the logic around cachedEndpoints to be keyed by mode
(e.g., cachedEndpointsByMode[mode]) or clear/replace cachedEndpoints whenever
mode differs, and ensure the early-return uses the mode-specific cache entry
inside the branch handling "web-standalone" and the other mode branches (refer
to symbols cachedEndpoints and mode and the "web-standalone"/"web-production"
branches) so each mode always gets its correct wsUrl/apiBase.
…nd remote pairing New CLI package (apps/cli/) published as `deus-machine` on npm. Smart auto-detection: desktop machines install/launch the Electron app, servers start headless mode with interactive onboarding (Claude CLI detection, API key setup, remote access). Commands: deus, deus start, deus install, deus pair, deus login, deus status. Includes zero-dep QR code generator for pairing URLs, interactive terminal prompts, and animated DEUS ASCII banner. 64KB bundle, zero runtime dependencies. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: deeb59356ac7
- Delete serve.ts (replaced by start.ts) - Extract shared lib/format.ts (formatUptime, formatTimeAgo, formatBytes) - Extract shared lib/http.ts (httpGet, httpPost for backend communication) - Remove duplicate utilities from pair.ts, status.ts, start.ts - Total: 3566 → 3012 lines (-15%) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 63d3dcc83317
- Shutdown now waits up to 5s for child processes to exit before force-killing, letting the agent-server drain active turns. clearServerInfo() runs after children exit, not before. - stdout listener is removed after pattern match to prevent unbounded buffer growth on long-running processes. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 933acf964733
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 7312cca020e0
The build script referenced ../../node_modules/.bin/tsx which doesn't exist. Changed to bunx tsx build.ts to match all other monorepo build scripts. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: c2fa8dd96699
- Add publish-cli job that builds and publishes deus-machine to npm after GitHub Release is created - Sync CLI version in bump-version.sh so root and CLI versions match - Requires NPM_TOKEN secret in GitHub repo settings Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 0cc37c157ab2
- Remove duplicate formatBytes from desktop.ts, use shared lib/format.ts - Remove dead code: c.success/error/warn/info aliases, step(), termWidth() - Remove unused imports: spawn from login.ts, sym/warn/gradientText from cli.ts - Unexport OnboardingResult (only used internally) - Fix redundant identical ternary branches in prompt.ts select hint Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 6eb180cfa556
zvadaadam
force-pushed
the
zvadaadam/npm-cli-installer
branch
from
0227e6d to
892cdb0
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
apps/cli/src/config.ts (1)
16-20:⚠️ Potential issue | 🟠 MajorSecrets are still persisted to
config.json.
anthropic_api_keyis still serialized into the same JSON file that is written with default permissions for both the temp file and final file. On multi-user systems that's an avoidable secret leak.Also applies to: 74-83
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/src/config.ts` around lines 16 - 20, The interface DeusConfig currently includes anthropic_api_key which causes the secret to be serialized into config.json; remove anthropic_api_key from the persisted DeusConfig and stop writing it to disk (and from any temp-file write path), instead move secret storage to a secure store (OS keychain / credential manager) or accept it only from environment variables at runtime; update any load/save functions that reference DeusConfig/anthropic_api_key to exclude it and add dedicated helpers (e.g., getAnthropicApiKey/setAnthropicApiKey) that use secure storage or env access so the key is never written into config.json or its temp file copies.
🧹 Nitpick comments (1)
.github/workflows/release.yml (1)
286-290: Prefer a single canonical CLI build entrypoint to prevent drift.These inline build commands can diverge from repo scripts over time. Consider calling one script (
build:clior a dedicatedbuild:cli:publish) so CI and local release flow stay in sync.Refactor option
- - name: Build CLI - run: | - bun run build:agent-server - bun run build:backend - cd apps/cli && bun run build + - name: Build CLI + run: bun run build:cli🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/release.yml around lines 286 - 290, The workflow's "Build CLI" step currently runs inline commands (bun run build:agent-server, bun run build:backend, cd apps/cli && bun run build) which can drift from repo scripts; replace these inline commands with a single canonical script invocation (e.g., bun run build:cli or bun run build:cli:publish) in the "Build CLI" step and, if missing, add a top-level package.json script (build:cli or build:cli:publish) that runs the three commands (build:agent-server, build:backend, and the apps/cli build) so CI and local release use the same entrypoint.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/release.yml:
- Line 23: The NPM_TOKEN secret is documented as optional but the publish-cli
job requires it; update the release.yml to mark NPM_TOKEN as required in the
variables/comments and add a fail-fast validation before the publish-cli step:
add a short step (e.g., step id/label "validate-npm-token" or functionally named
validateNpmToken) in the job that checks that secrets.NPM_TOKEN is non-empty and
exits with an error if missing, or gate the publish-cli step with an if:
condition that ensures secrets.NPM_TOKEN exists; ensure the publish-cli step
name/ID remains identifiable so the pre-check clearly protects publish-cli from
running without NPM_TOKEN.
In `@apps/cli/src/login.ts`:
- Around line 135-139: The fast-path return uses config.auth_method but leaves
previously saved API keys on disk, so when switching away from "api_key" you
must clear stored secrets; update the branches that set config.auth_method
(check places using opts?.force and assignments to config.auth_method) to also
null out config.anthropic_api_key and config.openai_api_key (or delete those
fields) whenever auth_method is changed to "env", "claude_cli", "skipped", etc.,
and ensure the persisted config is saved after the change so subsequent
fast-path returns (the code returning AuthResult with method and apiKey) cannot
return a stale manual key.
- Around line 51-64: The Windows discovery fails because there is no win32
candidate path and the shell fallback uses process.env.SHELL which is invalid on
Windows; update the detection logic in the function that builds candidates (the
candidates array in detectClaudeCli or equivalent in apps/cli/src/login.ts) to
add typical Windows install locations (e.g., paths under %ProgramFiles% and
%AppData% for `@anthropic-ai/claude-code`\cli.js and a plain "claude" executable
candidate), and change the PATH fallback shell invocation to use
process.env.ComSpec || "cmd.exe" when platform() === "win32" so PATH-based
detection works on Windows; apply the same changes to the duplicated block
around lines 86-93 (the second candidates construction) so both discovery
branches include win32 handling.
In `@apps/cli/src/prompt.ts`:
- Around line 57-60: The prompt helpers in apps/cli/src/prompt.ts call
stdin.setRawMode(true) unconditionally and must be guarded by a TTY check:
before calling stdin.setRawMode(true) inside each prompt helper (the Promise
blocks that reference stdin.isRaw and stdin.resume(), e.g., the primary prompt
function and the two other helper functions in the file), check stdin.isTTY (or
process.stdin.isTTY) and if false either skip enabling raw mode and fall back to
a non-raw input path or throw a clear, user-facing error indicating interactive
input is required; update all three places that call setRawMode to use this
guard and ensure stdin.setRawMode(false) is only called when raw mode was
actually enabled.
In `@apps/cli/src/qr.ts`:
- Around line 425-455: The QR rendering uses a 1-module quiet zone; change it to
a 4-module quiet zone by setting qSize = size + 8 (i.e. 2 * margin where margin
= 4) and update the QR-to-terminal row/col mapping to account for that margin:
replace the current topR/botR/col computations (topR = r - 1, botR = r, col = c
- 1) with formulas that subtract the margin (e.g. topR = r - margin, botR = r -
margin + 1, col = c - margin) and keep the existing bounds checks against
size/final so out-of-range positions render as light modules. Ensure the
for-loop that builds lines still iterates r in steps of 2 and uses the updated
qSize and margin consistently.
In `@apps/cli/src/start.ts`:
- Around line 101-109: During shutdown the code uses child.process.killed (which
is true only if kill() was called) so it attaches an "exit" listener to
processes that may have already exited, causing Promise.all to hang; replace the
liveness check to use child.process.exitCode (or another definitive exit
indicator) and handle already-exited children by resolving immediately: for each
item in children, if child.process.exitCode !== null then skip/resolve
immediately, otherwise push a Promise that attaches an "exit" listener and then
calls child.process.kill("SIGTERM"); apply the same fix for the similar block
around the 114-129 region so you never add an exit listener after the process
has already emitted "exit".
- Around line 342-346: startProcess() currently resolves null on timeout or
premature exit but leaves the spawned child running; update the timeout and
premature-exit handlers to terminate the spawned ChildProcess before resolving.
Specifically, when the timeout callback or the early-exit branch sets resolved =
true and calls resolve(null), first check the ChildProcess instance created by
startProcess (the spawned child variable) and call its graceful termination
(e.g., child.kill() / child.kill('SIGTERM')) and wait/ensure it has exited (or
force-kill if it doesn't) before resolving; apply the same termination logic to
the other identical block (the branch around lines 364-369) so no orphaned
processes remain.
---
Duplicate comments:
In `@apps/cli/src/config.ts`:
- Around line 16-20: The interface DeusConfig currently includes
anthropic_api_key which causes the secret to be serialized into config.json;
remove anthropic_api_key from the persisted DeusConfig and stop writing it to
disk (and from any temp-file write path), instead move secret storage to a
secure store (OS keychain / credential manager) or accept it only from
environment variables at runtime; update any load/save functions that reference
DeusConfig/anthropic_api_key to exclude it and add dedicated helpers (e.g.,
getAnthropicApiKey/setAnthropicApiKey) that use secure storage or env access so
the key is never written into config.json or its temp file copies.
---
Nitpick comments:
In @.github/workflows/release.yml:
- Around line 286-290: The workflow's "Build CLI" step currently runs inline
commands (bun run build:agent-server, bun run build:backend, cd apps/cli && bun
run build) which can drift from repo scripts; replace these inline commands with
a single canonical script invocation (e.g., bun run build:cli or bun run
build:cli:publish) in the "Build CLI" step and, if missing, add a top-level
package.json script (build:cli or build:cli:publish) that runs the three
commands (build:agent-server, build:backend, and the apps/cli build) so CI and
local release use the same entrypoint.
🪄 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: defaults
Review profile: CHILL
Plan: Lite
Run ID: 3cc0a0d5-26f9-4c3a-ad4a-078e81edc86d
📒 Files selected for processing (27)
.github/workflows/release.yml.gitignore.husky/_/commit-msg.husky/_/post-commit.husky/_/pre-push.husky/_/prepare-commit-msgapps/cli/README.mdapps/cli/build.tsapps/cli/package.jsonapps/cli/src/cli.tsapps/cli/src/config.tsapps/cli/src/desktop.tsapps/cli/src/lib/format.tsapps/cli/src/lib/http.tsapps/cli/src/login.tsapps/cli/src/onboarding.tsapps/cli/src/pair.tsapps/cli/src/prompt.tsapps/cli/src/qr.tsapps/cli/src/start.tsapps/cli/src/status.tsapps/cli/src/ui.tsapps/cli/tsconfig.jsonapps/web/index.htmlapps/web/src/shared/config/backend.config.tspackage.jsonscripts/bump-version.sh
✅ Files skipped from review due to trivial changes (10)
- .husky/_/post-commit
- .husky/_/commit-msg
- apps/cli/tsconfig.json
- .husky/_/pre-push
- apps/web/index.html
- .gitignore
- apps/cli/package.json
- apps/cli/README.md
- apps/cli/src/lib/http.ts
- .husky/_/prepare-commit-msg
🚧 Files skipped from review as they are similar to previous changes (9)
- scripts/bump-version.sh
- package.json
- apps/web/src/shared/config/backend.config.ts
- apps/cli/src/status.ts
- apps/cli/src/lib/format.ts
- apps/cli/src/cli.ts
- apps/cli/build.ts
- apps/cli/src/pair.ts
- apps/cli/src/desktop.ts
Comment on lines
+51
to
+64
| const os = platform(); | ||
| if (os === "darwin") { | ||
| candidates.push( | ||
| "/opt/homebrew/lib/node_modules/@anthropic-ai/claude-code/cli.js", | ||
| "/usr/local/lib/node_modules/@anthropic-ai/claude-code/cli.js", | ||
| join(homedir(), ".npm/lib/node_modules/@anthropic-ai/claude-code/cli.js"), | ||
| ); | ||
| } else if (os === "linux") { | ||
| candidates.push( | ||
| "/usr/lib/node_modules/@anthropic-ai/claude-code/cli.js", | ||
| "/usr/local/lib/node_modules/@anthropic-ai/claude-code/cli.js", | ||
| join(homedir(), ".npm/lib/node_modules/@anthropic-ai/claude-code/cli.js"), | ||
| ); | ||
| } |
There was a problem hiding this comment.
Windows discovery never succeeds automatically.
There is no win32 candidate-path branch, and the PATH fallback shells out through process.env.SHELL || "/bin/sh". On Windows that usually means detectClaudeCli() falls through to installed: false even when claude is already on PATH.
🪟 Suggested fix
const os = platform();
if (os === "darwin") {
@@
} else if (os === "linux") {
candidates.push(
"/usr/lib/node_modules/@anthropic-ai/claude-code/cli.js",
"/usr/local/lib/node_modules/@anthropic-ai/claude-code/cli.js",
join(homedir(), ".npm/lib/node_modules/@anthropic-ai/claude-code/cli.js"),
);
}
@@
- try {
- const shell = process.env.SHELL || "/bin/sh";
- const claudePath = execSync(`${shell} -lc "command -v claude"`, {
- encoding: "utf-8",
- timeout: 5000,
- stdio: ["pipe", "pipe", "pipe"],
- }).trim();
+ try {
+ const lookup = platform() === "win32"
+ ? spawnSync("where", ["claude"], {
+ encoding: "utf-8",
+ timeout: 5000,
+ stdio: ["pipe", "pipe", "pipe"],
+ })
+ : spawnSync(process.env.SHELL || "/bin/sh", ["-lc", "command -v claude"], {
+ encoding: "utf-8",
+ timeout: 5000,
+ stdio: ["pipe", "pipe", "pipe"],
+ });
+
+ const claudePath = lookup.stdout.trim().split(/\r?\n/)[0] ?? "";Also applies to: 86-93
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/cli/src/login.ts` around lines 51 - 64, The Windows discovery fails
because there is no win32 candidate path and the shell fallback uses
process.env.SHELL which is invalid on Windows; update the detection logic in the
function that builds candidates (the candidates array in detectClaudeCli or
equivalent in apps/cli/src/login.ts) to add typical Windows install locations
(e.g., paths under %ProgramFiles% and %AppData% for
`@anthropic-ai/claude-code`\cli.js and a plain "claude" executable candidate), and
change the PATH fallback shell invocation to use process.env.ComSpec ||
"cmd.exe" when platform() === "win32" so PATH-based detection works on Windows;
apply the same changes to the duplicated block around lines 86-93 (the second
candidates construction) so both discovery branches include win32 handling.
Comment on lines
+342
to
+346
| const timeout = setTimeout(() => { | ||
| if (!resolved) { | ||
| resolved = true; | ||
| resolve(null); | ||
| } |
There was a problem hiding this comment.
Terminate timed-out children before reporting startup failure.
startProcess() resolves null on timeout or premature exit but never stops the spawned child. If the readiness marker is late or changes, deus start exits while the backend or agent-server keeps running orphaned in the background.
🧹 Suggested fix
return new Promise((resolve) => {
@@
children.push({ process: child, name });
let buffer = "";
let resolved = false;
+ let timeout: ReturnType<typeof setTimeout>;
+
+ function finish(value: string | null, killChild = false) {
+ if (resolved) return;
+ resolved = true;
+ clearTimeout(timeout);
+ child.stdout?.removeListener("data", onStdout);
+ child.removeListener("exit", onExit);
+ child.removeListener("error", onError);
+ if (killChild && child.exitCode === null && child.signalCode === null) {
+ child.kill("SIGTERM");
+ }
+ buffer = "";
+ resolve(value);
+ }
- const timeout = setTimeout(() => {
- if (!resolved) {
- resolved = true;
- resolve(null);
- }
- }, timeoutMs);
+ timeout = setTimeout(() => finish(null, true), timeoutMs);
@@
buffer += data.toString();
const match = buffer.match(waitFor);
if (match && !resolved) {
- resolved = true;
- clearTimeout(timeout);
- // Stop listening and release the buffer — process may run for days
- child.stdout?.removeListener("data", onStdout);
- buffer = "";
- resolve(match[1]);
+ finish(match[1]);
}
}
child.stdout?.on("data", onStdout);
-
- child.on("exit", () => {
- if (!resolved) {
- resolved = true;
- clearTimeout(timeout);
- resolve(null);
- }
- });
+ function onExit() { finish(null); }
+ function onError() { finish(null, true); }
+ child.once("exit", onExit);
+ child.once("error", onError);
});
}Also applies to: 364-369
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/cli/src/start.ts` around lines 342 - 346, startProcess() currently
resolves null on timeout or premature exit but leaves the spawned child running;
update the timeout and premature-exit handlers to terminate the spawned
ChildProcess before resolving. Specifically, when the timeout callback or the
early-exit branch sets resolved = true and calls resolve(null), first check the
ChildProcess instance created by startProcess (the spawned child variable) and
call its graceful termination (e.g., child.kill() / child.kill('SIGTERM')) and
wait/ensure it has exited (or force-kill if it doesn't) before resolving; apply
the same termination logic to the other identical block (the branch around lines
364-369) so no orphaned processes remain.
…lifecycle - Format all CLI files with prettier (fixes CI format:check) - Add comments in empty catch blocks (fixes lint no-empty) - Add CLI entry points to knip.json (fixes knip) - Remove .husky/_/ hooks referencing non-existent 'entire' CLI - Set restrictive file permissions (0o600) on config and server info - Wrap Linux AppImage install in try/catch (matches macOS pattern) - Add QR fallback message on generation failure - Fix shutdown liveness: use exitCode/signalCode instead of .killed - Kill orphaned child processes on startProcess timeout - Add Windows PATH discovery (where command) for Claude CLI detection - Clear stale API keys when switching auth methods (persistAuth helper) - Guard interactive prompts with TTY check (prevents crash in Docker/CI) - Increase QR quiet zone from 1 to 4 modules per ISO 18004 - Add NPM_TOKEN fail-fast validation in release workflow - Reclassify NPM_TOKEN as required in release.yml docs Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 798bc656c005
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (2)
apps/cli/src/login.ts (2)
142-147:⚠️ Potential issue | 🟡 MinorAuth method validation still missing.
The
config.auth_methodis cast toAuthResult["method"]without runtime validation. If the config file is corrupted or manually edited with an invalid value, this could cause subtle bugs. Consider validating against allowed values.🛡️ Proposed fix
+const VALID_AUTH_METHODS = ["claude_cli", "api_key", "env", "skipped"] as const; + export async function runAuthSetup(opts?: { force?: boolean }): Promise<AuthResult> { const config = loadConfig(); // If not forced and already configured, return saved config - if (!opts?.force && config.auth_method) { + if (!opts?.force && config.auth_method && VALID_AUTH_METHODS.includes(config.auth_method)) { return { method: config.auth_method as AuthResult["method"], apiKey: config.anthropic_api_key, }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/src/login.ts` around lines 142 - 147, The code returns config.auth_method cast to AuthResult["method"] without runtime validation, which can allow invalid values from a corrupted config; update the logic in login.ts (the block using opts?.force, config.auth_method and returning AuthResult["method"]) to validate config.auth_method against the allowed AuthResult method enum/union before returning (e.g., check membership in the permitted set or use a helper isValidAuthMethod), and if invalid fall back to null/undefined or throw/force a re-prompt so the function never returns an invalid method value.
255-271: 🛠️ Refactor suggestion | 🟠 MajorUse
buninstead ofnpmper coding guidelines.Line 259 uses
npm install -gbut the coding guidelines require usingbunfor all package management.♻️ Proposed fix
async function installAndLogin(): Promise<AuthResult> { blank(); const s = createSpinner("Installing Claude Code CLI..."); - const result = spawnSync("npm", ["install", "-g", "@anthropic-ai/claude-code"], { + const result = spawnSync("bun", ["add", "-g", "@anthropic-ai/claude-code"], { stdio: ["pipe", "pipe", "pipe"], timeout: 120_000, }); if (result.status !== 0) { s.fail("Installation failed"); blank(); hint("Try installing manually:"); - console.log(` ${c.cyan("npm install -g `@anthropic-ai/claude-code`")}`); + console.log(` ${c.cyan("bun add -g `@anthropic-ai/claude-code`")}`); blank(); return skipAuth(); }As per coding guidelines: "Always use
bunfor package management (bun add,bun install), running scripts (bun run), and executing tools (bunx). Never usenpmoryarn."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/src/login.ts` around lines 255 - 271, The installAndLogin function currently spawns "npm install -g `@anthropic-ai/claude-code`"; update it to use Bun per guidelines by replacing the spawnSync invocation to run "bunx" with ["@anthropic-ai/claude-code"] (i.e., spawnSync("bunx", ["@anthropic-ai/claude-code"], ...)) and update the user hint/console message that shows the manual install command to use bunx (e.g., `bunx `@anthropic-ai/claude-code``) so both the runtime command and the displayed hint follow the Bun convention.
🧹 Nitpick comments (3)
apps/cli/src/start.ts (1)
364-370: Early exit handler leaves child inchildrenarray without explicit termination.When the child process exits prematurely (before emitting the readiness marker), the handler resolves
nullbut the child is already in thechildrenarray (line 336). Since the process has exited, this is fine—it won't be orphaned. However, consider clearing the timeout here for consistency:♻️ Optional cleanup
child.on("exit", () => { if (!resolved) { resolved = true; clearTimeout(timeout); + child.stdout?.removeListener("data", onStdout); resolve(null); } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/src/start.ts` around lines 364 - 370, The child.on("exit") early-exit handler should both clear the timeout and remove the exited child from the children array to avoid leaving stale references; inside the handler where you set resolved = true and clearTimeout(timeout), also find and splice the exited child out of the children array (use the same child reference used when pushing into children) before calling resolve(null).apps/cli/build.ts (1)
95-102: Package.json modifications are computed but never written.Lines 97-101 read
package.json, deletescripts, and setmain/type, but the modifiedpkgobject is never written back. The comment on line 102 says "we don't overwrite" but the code still performs these mutations. Either remove this dead code or write the modified package.json if it's actually needed for publishing.- // Step 4: Generate a standalone package.json for the published CLI - console.log("\n4. Generating publish-ready package.json..."); - const pkg = JSON.parse(readFileSync(join(cliDir, "package.json"), "utf-8")); - // Ensure we don't ship "private: true" or monorepo-specific fields - delete pkg.scripts; - pkg.main = "dist/cli.js"; - pkg.type = "module"; - // Note: we don't overwrite — the original is fine for publishing + // Step 4 removed — original package.json is fine for publishing🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/build.ts` around lines 95 - 102, The code reads and mutates the package.json into the local variable pkg (via readFileSync and JSON.parse) but never persists it; either remove the dead mutations or write the modified object back. If you intend to publish with the adjusted package, serialize and save pkg (e.g., using writeFileSync(join(cliDir, "package.json"), JSON.stringify(pkg, null, 2))) after the deletions and property assignments; otherwise remove the block that mutates pkg (the lines manipulating pkg.scripts, pkg.main, pkg.type) so there’s no misleading dead code. Ensure you update any callers that expect the file to be written if you choose to persist it.apps/cli/src/desktop.ts (1)
197-203: Useawait collectBody(...)instead of chained.then(...).These are sequential promise flows inside an
asynccodepath already. As per coding guidelines, "Always use async/await for promises. Never use.then()chains for sequential operations."Also applies to: 212-218
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/src/desktop.ts` around lines 197 - 203, Replace the .then(...) chains that call collectBody(r) with async/await: await the result (e.g., const b = await collectBody(r)) then run the JSON.parse inside a try/catch and call resolve(parsed) or resolve(null) on failure; do this for both occurrences referencing collectBody, JSON.parse and resolve so the flow uses async/await instead of chained .then().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/cli/src/config.ts`:
- Around line 24-30: Document that DeusConfig.anthropic_api_key is stored
unencrypted in the CLI config file (persisted by the CLI) and thus is written as
plaintext at rest; update the CLI README to warn users about this security
consideration, recommend using auth_method "claude_cli" or OS keychain
integration as more secure alternatives, and list mitigations (restrict file
permissions, avoid using shared machines, rotate keys regularly) so users can
make an informed choice between API key auth and Claude CLI auth.
In `@apps/cli/src/desktop.ts`:
- Line 35: The linux candidate list incorrectly includes the CLI shim path,
causing findInstalledApp() to detect "/usr/bin/deus" as the desktop AppImage and
making launchDesktop() spawn the CLI; remove "/usr/bin/deus" (and any CLI shim
paths) from the linux candidates array and ensure findInstalledApp() only checks
actual desktop artifact names (e.g., "Deus.AppImage" locations) so
launchDesktop() targets the real desktop binary rather than the CLI shim.
- Around line 139-152: installForPlatform currently returns a boolean that
conflates recoverable fallbacks (macOS DMG opened manually) with unrecoverable
failures (Linux permission error); change the flow so installForPlatform (or the
lower-level platform installers) either throws on hard failures or returns a
discriminated status (e.g., "installed" | "opened-dmg" | "failed" with an error
payload), then update the caller in installDesktop to treat "failed" (or caught
exceptions) as a hard error and call process.exit(1) after logging the error,
while treating "opened-dmg" as a non-fatal success path; keep the unlinkSync
cleanup as-is and ensure processLogger/console shows the actual error when
exiting.
- Around line 49-53: The function that detects a GUI (hasDisplay) currently
returns true for darwin and for linux when DISPLAY/WAYLAND_DISPLAY exists,
ignoring SSH; update the logic to treat SSH sessions as headless: check
process.env.SSH_CONNECTION (and any other SSH indicators you use) before
returning true for GUI platforms. Concretely, in the code that calls platform(),
existsSync, and returns based on os === "darwin" / "linux", ensure you
short-circuit and return false if process.env.SSH_CONNECTION is set (and keep
the existing Windows check), so macOS and Linux only return true when not an SSH
session.
- Around line 188-220: fetchRelease() and downloadFile() currently have no HTTP
timeouts (which can hang) and downloadFile resolves on file.on("finish") instead
of file.on("close") (racy on Windows). Add socket/request timeouts for both
GitHub GETs in fetchRelease() (initial and redirected get calls) and for the
HTTP request in downloadFile() so that on timeout the request is aborted and the
promise rejects/resolves null consistently; ensure you attach timeouts to the
ClientRequest (req.setTimeout or req.on("socket", s => s.setTimeout(...))) and
handle the timeout callback to call req.abort() and cleanup. In downloadFile(),
replace file.on("finish") with file.on("close") to wait for the file descriptor
to be closed before resolving, and also apply the same request/socket timeout
handling and error cleanup there. Also update the redirected GETs mentioned in
the diff (the secondary get callback) and the other similar block around lines
233-275 to include identical timeout logic and handlers.
- Around line 305-307: When installing in the block that checks
existsSync(appPath) (around the execSync call that runs cp -R "${appPath}"
"${destPath}" and the s.succeed call), ensure the destination is removed before
copying to avoid nested or stale contents: run a safe removal (execSync `rm -rf
"${destPath}"`) prior to execSync copying, keeping the same quoting and stdio
options, and preserve existing success/error handling so destPath is fully
replaced rather than merged.
In `@apps/cli/src/start.ts`:
- Around line 200-217: The hint shown when config.relay_enabled is false is
misleading; update the message to accurately instruct users how to enable the
relay instead of suggesting `deus pair` (which only generates a code). Change
the branch that checks config.relay_enabled to display a clear action such as
"re-run onboarding or edit config to enable relay" (referencing
config.relay_enabled and showPairCode to locate the block) and ensure the
failure catch for showPairCode still advises retrying pairing once the relay
connects (referencing showPairCode and the `deus pair` command).
---
Duplicate comments:
In `@apps/cli/src/login.ts`:
- Around line 142-147: The code returns config.auth_method cast to
AuthResult["method"] without runtime validation, which can allow invalid values
from a corrupted config; update the logic in login.ts (the block using
opts?.force, config.auth_method and returning AuthResult["method"]) to validate
config.auth_method against the allowed AuthResult method enum/union before
returning (e.g., check membership in the permitted set or use a helper
isValidAuthMethod), and if invalid fall back to null/undefined or throw/force a
re-prompt so the function never returns an invalid method value.
- Around line 255-271: The installAndLogin function currently spawns "npm
install -g `@anthropic-ai/claude-code`"; update it to use Bun per guidelines by
replacing the spawnSync invocation to run "bunx" with
["@anthropic-ai/claude-code"] (i.e., spawnSync("bunx",
["@anthropic-ai/claude-code"], ...)) and update the user hint/console message
that shows the manual install command to use bunx (e.g., `bunx
`@anthropic-ai/claude-code``) so both the runtime command and the displayed hint
follow the Bun convention.
---
Nitpick comments:
In `@apps/cli/build.ts`:
- Around line 95-102: The code reads and mutates the package.json into the local
variable pkg (via readFileSync and JSON.parse) but never persists it; either
remove the dead mutations or write the modified object back. If you intend to
publish with the adjusted package, serialize and save pkg (e.g., using
writeFileSync(join(cliDir, "package.json"), JSON.stringify(pkg, null, 2))) after
the deletions and property assignments; otherwise remove the block that mutates
pkg (the lines manipulating pkg.scripts, pkg.main, pkg.type) so there’s no
misleading dead code. Ensure you update any callers that expect the file to be
written if you choose to persist it.
In `@apps/cli/src/desktop.ts`:
- Around line 197-203: Replace the .then(...) chains that call collectBody(r)
with async/await: await the result (e.g., const b = await collectBody(r)) then
run the JSON.parse inside a try/catch and call resolve(parsed) or resolve(null)
on failure; do this for both occurrences referencing collectBody, JSON.parse and
resolve so the flow uses async/await instead of chained .then().
In `@apps/cli/src/start.ts`:
- Around line 364-370: The child.on("exit") early-exit handler should both clear
the timeout and remove the exited child from the children array to avoid leaving
stale references; inside the handler where you set resolved = true and
clearTimeout(timeout), also find and splice the exited child out of the children
array (use the same child reference used when pushing into children) before
calling resolve(null).
🪄 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: defaults
Review profile: CHILL
Plan: Lite
Run ID: 5ccfb740-a92a-448f-bc3f-4986821d5ab9
📒 Files selected for processing (14)
.github/workflows/release.ymlapps/cli/build.tsapps/cli/src/cli.tsapps/cli/src/config.tsapps/cli/src/desktop.tsapps/cli/src/login.tsapps/cli/src/onboarding.tsapps/cli/src/pair.tsapps/cli/src/prompt.tsapps/cli/src/qr.tsapps/cli/src/start.tsapps/cli/src/status.tsapps/cli/src/ui.tsknip.json
🚧 Files skipped from review as they are similar to previous changes (4)
- .github/workflows/release.yml
- apps/cli/src/status.ts
- apps/cli/src/cli.ts
- apps/cli/src/onboarding.ts
Comment on lines
+24
to
+30
| export interface DeusConfig { | ||
| onboarding_completed: boolean; | ||
| auth_method: "claude_cli" | "api_key" | "env" | "skipped" | null; | ||
| anthropic_api_key?: string; | ||
| relay_enabled: boolean; | ||
| installed_at?: string; | ||
| } |
There was a problem hiding this comment.
API key stored in plaintext remains a security consideration.
The anthropic_api_key is persisted to ~/.config/deus/config.json. While file permissions are now restrictive (0600), users should be aware that:
- The key is not encrypted at rest
- OS keychain integration would be more secure but adds complexity
Consider documenting this in the CLI README so users can make informed decisions about using API key auth vs. Claude CLI auth.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/cli/src/config.ts` around lines 24 - 30, Document that
DeusConfig.anthropic_api_key is stored unencrypted in the CLI config file
(persisted by the CLI) and thus is written as plaintext at rest; update the CLI
README to warn users about this security consideration, recommend using
auth_method "claude_cli" or OS keychain integration as more secure alternatives,
and list mitigations (restrict file permissions, avoid using shared machines,
rotate keys regularly) so users can make an informed choice between API key auth
and Claude CLI auth.
Comment on lines
+139
to
+152
| const installed = await installForPlatform(os, downloadPath, s3); | ||
|
|
||
| // Cleanup temp file | ||
| try { | ||
| unlinkSync(downloadPath); | ||
| } catch { | ||
| // ignore cleanup failure | ||
| } | ||
|
|
||
| if (installed) { | ||
| blank(); | ||
| success("Deus is ready!"); | ||
| blank(); | ||
| } |
There was a problem hiding this comment.
Propagate hard install failures with a non-zero exit.
A Linux permission error returns false, but installDesktop() only handles the truthy path and otherwise falls through with exit code 0. The boolean result also conflates macOS “opened the DMG manually” with a real failure, so callers can’t distinguish recoverable fallback from an actual broken install.
Also applies to: 278-344
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/cli/src/desktop.ts` around lines 139 - 152, installForPlatform currently
returns a boolean that conflates recoverable fallbacks (macOS DMG opened
manually) with unrecoverable failures (Linux permission error); change the flow
so installForPlatform (or the lower-level platform installers) either throws on
hard failures or returns a discriminated status (e.g., "installed" |
"opened-dmg" | "failed" with an error payload), then update the caller in
installDesktop to treat "failed" (or caught exceptions) as a hard error and call
process.exit(1) after logging the error, while treating "opened-dmg" as a
non-fatal success path; keep the unlinkSync cleanup as-is and ensure
processLogger/console shows the actual error when exiting.
…ards - Remove /usr/bin/deus CLI shim from linux desktop candidates (false positive) - Add SSH_CONNECTION/SSH_TTY check to hasDisplay() for all platforms - rm -rf existing Deus.app before cp -R to prevent nested installs on macOS - Add HTTP socket timeouts to fetchRelease (30s) and downloadFile (120s) - Consume redirect responses to free sockets in downloadFile - Use file "close" event instead of "finish" for reliable write completion - Fix misleading relay hint: suggest "deus login" instead of "deus pair" - Add TTY guards to animatedBanner, spinner, statusLine, progressBar - Degrade gracefully to plain text output when stderr is piped Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 73f0b671e9c0
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
apps/cli/src/desktop.ts (1)
139-154:⚠️ Potential issue | 🟠 MajorDifferentiate a manual-open fallback from a real install failure.
This caller only checks the truthy path, but
installForPlatform()returns the samefalsefor “opened the DMG for manual install” and for actual failures like a Linux copy error. That makesdeus installfall through as success even when the install is broken. Return an explicit status, or throw on hard failures, so the caller can exit non-zero only for real failures.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/src/desktop.ts` around lines 139 - 154, installForPlatform currently returns false for both a manual-open DMG fallback and hard failures, so update its contract to return an explicit status or throw on hard failures (e.g., return enum InstallResult { Success, ManualOpen, Failure } or throw on Failure). Then change the caller in desktop.ts (the block using createSpinner/installed/unlinkSync) to handle the new values: treat Success as before (show success and exit 0), treat ManualOpen by informing the user the DMG was opened for manual install and exit 0, and treat Failure by logging the error and exiting non-zero (or catch the thrown error and exit non-zero). Ensure unlinkSync cleanup remains in the finally/try block so temp file is always removed.
🧹 Nitpick comments (1)
apps/cli/src/desktop.ts (1)
198-205: Useawaitfor the response parsing path.These branches are doing sequential async work with nested
.then()calls, which makes the control flow harder to follow than it needs to be here. Converting the callbacks toasyncand awaitingcollectBody(...)keeps this flatter and consistent with the rest of the repo.As per coding guidelines, "Always use async/await for promises. Never use
.then()chains for sequential operations."Also applies to: 216-222
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/src/desktop.ts` around lines 198 - 205, The callback passed to get uses nested .then chains for sequential async work (collectBody then parse), making control flow hard to follow; change the callback to an async function so you can await collectBody(r) and then JSON.parse inside a try/catch, calling resolve(parsed) on success and resolve(null) on failure; update both occurrences that call get and use collectBody so they follow the same async/await pattern (identify the get(...) callback, collectBody, and resolve usage to locate the spots).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/cli/src/desktop.ts`:
- Around line 161-177: The platform-case handlers (cases "darwin", "win32",
"linux") currently assume any cpuArch and may pick an installer for unsupported
CPUs; update each case to explicitly accept only supported architectures and
return null for unsupported ones: in the "darwin" branch only allow "arm64" and
"x64" (compute archSuffix only for those, otherwise return null), and in the
"win32" and "linux" branches validate cpuArch against the supported set you
decide (e.g., "x64" and "arm64") and return null when cpuArch is not in that
set; keep the existing matcher/description logic but only after the arch check
so the CLI fails early for unsupported CPU architectures (refer to cpuArch and
the "darwin"/"win32"/"linux" case blocks to locate the changes).
- Around line 293-329: The DMG mount (mountPoint) must be captured outside the
try and always detached in a finally block so we never leave /Volumes/...
mounted; refactor the block that calls execSync(`hdiutil attach ...`) to assign
mountPoint to an outer-scoped variable (e.g., let mountPoint: string |
undefined), keep the existing logic that computes mountPoint from mountOutput,
and move the detach call execSync(`hdiutil detach "${mountPoint}" -quiet`, {
stdio: "pipe" }) into a finally where you only call it if mountPoint is truthy
(and optionally wrap the detach in its own try/catch to ignore detach failures).
Ensure filePath, existsSync, launchDesktop, and the existing returns remain
unchanged except that all returns happen inside try but cleanup runs in finally.
In `@apps/cli/src/start.ts`:
- Around line 352-359: The readiness code currently sets resolved=true and
removes the stdout listener then leaves the process without long-lived
supervision; change this so that after detecting readiness (inside the block
using child, onStdout, buffer, resolved, timeout, resolve) you still attach
persistent 'exit' and 'error' handlers (or ensure existing handlers ignore the
resolved check) that run a teardown function which kills the sibling process,
clears/updates server.json, and calls process.exit(1) on unexpected death; apply
the same change to the similar logic referenced around the other block (the one
at 364-370) so the CLI exits non-zero and cleans up if backend/agent-server dies
after startup.
- Around line 148-152: The branch that currently calls process.exit(1) directly
(e.g., the agentServerUrl failure path that calls s1.fail() and blank()) must
instead invoke the existing child-cleanup routine (the function that sends
SIGTERM and waits for children to exit—e.g., cleanup, shutdownChildren, or
whatever central teardown function is defined) and await its completion before
exiting non-zero; update the agentServerUrl block (and the other spots that
directly call process.exit(1)) to call that cleanup function with the current
children array/context and only call process.exit(1) after the cleanup promise
resolves (or times out) so spawned processes are not orphaned.
---
Duplicate comments:
In `@apps/cli/src/desktop.ts`:
- Around line 139-154: installForPlatform currently returns false for both a
manual-open DMG fallback and hard failures, so update its contract to return an
explicit status or throw on hard failures (e.g., return enum InstallResult {
Success, ManualOpen, Failure } or throw on Failure). Then change the caller in
desktop.ts (the block using createSpinner/installed/unlinkSync) to handle the
new values: treat Success as before (show success and exit 0), treat ManualOpen
by informing the user the DMG was opened for manual install and exit 0, and
treat Failure by logging the error and exiting non-zero (or catch the thrown
error and exit non-zero). Ensure unlinkSync cleanup remains in the finally/try
block so temp file is always removed.
---
Nitpick comments:
In `@apps/cli/src/desktop.ts`:
- Around line 198-205: The callback passed to get uses nested .then chains for
sequential async work (collectBody then parse), making control flow hard to
follow; change the callback to an async function so you can await collectBody(r)
and then JSON.parse inside a try/catch, calling resolve(parsed) on success and
resolve(null) on failure; update both occurrences that call get and use
collectBody so they follow the same async/await pattern (identify the get(...)
callback, collectBody, and resolve usage to locate the spots).
🪄 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: defaults
Review profile: CHILL
Plan: Lite
Run ID: 5f5bb016-6a82-49a2-940a-7a2f6e3130a6
📒 Files selected for processing (3)
apps/cli/src/desktop.tsapps/cli/src/start.tsapps/cli/src/ui.ts
Comment on lines
+161
to
+177
| case "darwin": { | ||
| const archSuffix = cpuArch === "arm64" ? "arm64" : "x64"; | ||
| return { | ||
| matcher: (name) => name.endsWith(".dmg") && name.includes(archSuffix), | ||
| description: `macOS DMG (${archSuffix})`, | ||
| }; | ||
| } | ||
| case "win32": | ||
| return { | ||
| matcher: (name) => name.endsWith(".exe") && !name.includes("blockmap"), | ||
| description: "Windows installer (exe)", | ||
| }; | ||
| case "linux": | ||
| return { | ||
| matcher: (name) => name.endsWith(".AppImage"), | ||
| description: "Linux AppImage", | ||
| }; |
There was a problem hiding this comment.
Reject unsupported CPU architectures instead of guessing.
The Darwin branch maps every non-arm64 CPU to x64, and the Windows/Linux branches accept any cpuArch. On unsupported machines this will pick whatever installer happens to match instead of taking the explicit unsupported-platform path above. Return null for unsupported arches here so the CLI fails early with the correct message.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/cli/src/desktop.ts` around lines 161 - 177, The platform-case handlers
(cases "darwin", "win32", "linux") currently assume any cpuArch and may pick an
installer for unsupported CPUs; update each case to explicitly accept only
supported architectures and return null for unsupported ones: in the "darwin"
branch only allow "arm64" and "x64" (compute archSuffix only for those,
otherwise return null), and in the "win32" and "linux" branches validate cpuArch
against the supported set you decide (e.g., "x64" and "arm64") and return null
when cpuArch is not in that set; keep the existing matcher/description logic but
only after the arch check so the CLI fails early for unsupported CPU
architectures (refer to cpuArch and the "darwin"/"win32"/"linux" case blocks to
locate the changes).
- Move hdiutil detach to finally block so DMG is always unmounted - Replace process.exit(1) with shutdown(1) to clean up children on failure - Add post-startup child supervision: detect crashes, teardown, exit non-zero - Remove dead Step 4 in build.ts (package.json mutations never written to disk) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: ce36ee39953e
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
apps/cli/src/start.ts (2)
202-240:⚠️ Potential issue | 🟠 MajorInstall crash supervision before the pairing flow.
The long-lived
exitlistener is registered only aftersleep(1500)andshowPairCode(). If either child dies in that window, the one-shot listener insidestartProcess()is already inert (resolved === true), and this lateron("exit")misses the already-fired event. The result is a hung CLI with staleserver.json.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/src/start.ts` around lines 202 - 240, Register the child crash supervision before the pairing flow so exit events aren’t missed; move the loop that attaches child.process.on("exit", ...) (which uses children, shuttingDown, warn, and shutdown(1)) to run before the sleep(1500) and showPairCode() calls (i.e., set up supervision immediately after children are spawned and before calling showPairCode), ensuring the exit listeners are active during the pairing window.
346-385:⚠️ Potential issue | 🔴 CriticalAdd error event handler to
startProcess()to prevent Promise hangs on spawn failures.
ChildProcessemits"error"when the binary cannot be found or is unreadable, and Node.js documents that"exit"may not fire afterward. The current code lacks an error handler, causing the Promise to hang indefinitely if spawn fails—bypassingshutdown(1)entirely.Suggested fix
return new Promise((resolve) => { @@ let buffer = ""; let resolved = false; + let timeout: ReturnType<typeof setTimeout>; - const timeout = setTimeout(() => { - if (!resolved) { - resolved = true; - child.kill("SIGTERM"); - resolve(null); - } - }, timeoutMs); + function finish(value: string | null, killChild = false) { + if (resolved) return; + resolved = true; + clearTimeout(timeout); + child.stdout?.removeListener("data", onStdout); + child.removeListener("exit", onExit); + child.removeListener("error", onError); + if (killChild && child.exitCode === null && child.signalCode === null) { + child.kill("SIGTERM"); + } + buffer = ""; + resolve(value); + } + + timeout = setTimeout(() => finish(null, true), timeoutMs); @@ buffer += data.toString(); const match = buffer.match(waitFor); if (match && !resolved) { - resolved = true; - clearTimeout(timeout); - // Stop listening and release the buffer — process may run for days - child.stdout?.removeListener("data", onStdout); - buffer = ""; - resolve(match[1]); + finish(match[1]); } } @@ - child.on("exit", () => { - if (!resolved) { - resolved = true; - clearTimeout(timeout); - resolve(null); - } - }); + function onExit() { + finish(null); + } + + function onError() { + finish(null); + } + + child.once("exit", onExit); + child.once("error", onError); }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/src/start.ts` around lines 346 - 385, The startProcess promise lacks a ChildProcess "error" handler, so if spawn fails the promise can hang; add an error listener on the spawned child (the same ChildProcess referenced by "child") that, when triggered, sets resolved = true, clears the timeout, removes the stdout "data" listener (onStdout), and resolves the promise with null (and optionally performs any cleanup/logging similar to the "exit" handler) so the function (startProcess) always settles on spawn errors as well as normal exits.apps/cli/src/desktop.ts (2)
160-177:⚠️ Potential issue | 🟠 MajorReject unsupported CPU architectures before picking an asset.
Line 162 still maps every non-
arm64macOS machine tox64, and the Windows/Linux branches do not validatecpuArchat all. That contradicts the support hint above and can download a binary that cannot run on the current machine.♻️ Suggested fix
switch (os) { case "darwin": { - const archSuffix = cpuArch === "arm64" ? "arm64" : "x64"; + if (cpuArch !== "arm64" && cpuArch !== "x64") return null; + const archSuffix = cpuArch; return { matcher: (name) => name.endsWith(".dmg") && name.includes(archSuffix), description: `macOS DMG (${archSuffix})`, }; } case "win32": + if (cpuArch !== "x64") return null; return { matcher: (name) => name.endsWith(".exe") && !name.includes("blockmap"), description: "Windows installer (exe)", }; case "linux": + if (cpuArch !== "x64") return null; return { matcher: (name) => name.endsWith(".AppImage"), description: "Linux AppImage", };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/src/desktop.ts` around lines 160 - 177, The switch handling OS-specific asset selection currently assumes any non-"arm64" mac is "x64" and doesn't validate cpuArch for "win32" or "linux"; update the logic (in the switch on os using variables cpuArch, matcher, description) to first check cpuArch against supported values per OS and reject/return an explicit error or null for unsupported architectures instead of defaulting to "x64". For "darwin" only accept "arm64" or "x64" (do not coerce other values), for "win32" validate known Windows arches (e.g., "x64", "arm64") before creating the matcher, and for "linux" validate supported linux arches before returning an AppImage matcher; ensure the returned object or function signals unsupported CPU (throw or return null) so the caller won't download an incompatible binary.
136-153:⚠️ Potential issue | 🟠 MajorNormalize install outcomes before returning from
installDesktop().Lines 136-147 only clean up after
downloadFile()resolves, so a rejected download leaves the temp artifact behind. Then Lines 149-153 ignore thefalsepath even thoughinstallForPlatform()uses it for real failures as well as the macOS “opened DMG manually” fallback. Return a discriminated status, or throw on hard failures, and wrap download/install in atry/finallyso the command can always unlink the temp file and exit non-zero only for actual failures.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/src/desktop.ts` around lines 136 - 153, The current flow can leave the temp file if downloadFile rejects and treats installForPlatform's false as a silent failure; wrap the downloadFile + installForPlatform call inside a try/finally so unlinkSync(downloadPath) always runs, propagate download errors, and normalize install outcomes by either having installForPlatform throw on hard failures and return a discriminated success value (e.g., 'installed' | 'opened-manually') or by having installDesktop throw when installed === false; update the installDesktop logic (the call site that uses downloadFile, installForPlatform, unlinkSync and success/blank calls) to handle the discriminated return (show the appropriate message for 'opened-manually') or rethrow to ensure the CLI exits non-zero on real failures.
🧹 Nitpick comments (1)
apps/cli/src/desktop.ts (1)
199-205: Useawaitinstead of nested.then()callbacks here.These branches are just sequential body-read + parse work, so the
.then()nesting adds another control-flow shape for no real gain. A small async helper keeps it flat and matches the repo standard.As per coding guidelines, "Always use async/await for promises. Never use
.then()chains for sequential operations."Also applies to: 216-221
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/src/desktop.ts` around lines 199 - 205, The nested .then() on collectBody should be rewritten using async/await to flatten control flow: await the result of collectBody(r) into a variable, then run JSON.parse inside a try/catch and call resolve with the parsed object or null on failure; apply the same change to the similar block referenced at the other location (lines 216-221). Locate the callback where collectBody(r).then(...) is used, replace the .then() callback with "const b = await collectBody(r)" (or equivalent in the enclosing async function), then perform "try { resolve(JSON.parse(b)) } catch { resolve(null) }".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/cli/build.ts`:
- Around line 101-102: Update the release hint strings in build.ts so they
recommend Bun instead of npm: replace the two console.log calls that output "cd
apps/cli && npm publish" (and the preceding "To publish:" block) to print the
Bun equivalent "cd apps/cli && bun publish" to match the repository's Bun-only
workflow; locate the console.log calls in build.ts that emit the publish hint
and change the message text accordingly.
In `@apps/cli/src/desktop.ts`:
- Around line 257-279: The current download promise resolves on file.on("close")
which can still occur for a truncated transfer; modify the logic around res,
file, req, downloaded and totalSize so the promise only resolves after the HTTP
response is fully complete and the expected bytes were written: listen for
response errors (res.on("error")) and destroy/close the file stream and reject
on those errors, wait for both the response end/completion (check res.complete
or res.on("end")) and file.close, and when content-length is present assert
downloaded === totalSize before calling resolve (otherwise destroy the file and
reject). Also ensure req errors/timeouts still destroy the file stream to avoid
leaving truncated files and use progressBar/progressBarDone the same way but
only after the completeness checks pass.
In `@apps/cli/src/start.ts`:
- Around line 87-90: The signal handlers are passing the signal string into
shutdown, causing exitCode to become non-numeric; update the listeners so they
call shutdown with a numeric code (e.g., 0 or a mapped code) instead of passing
the signal argument directly, or wrap them to ignore the handler argument and
call shutdown(Number) — apply this change for the shutdown function usage in
start.ts (referenced as shutdown and the signal event listeners around the
second occurrence noted at lines 127-131) so exitCode is always a number before
calling process.exit().
---
Duplicate comments:
In `@apps/cli/src/desktop.ts`:
- Around line 160-177: The switch handling OS-specific asset selection currently
assumes any non-"arm64" mac is "x64" and doesn't validate cpuArch for "win32" or
"linux"; update the logic (in the switch on os using variables cpuArch, matcher,
description) to first check cpuArch against supported values per OS and
reject/return an explicit error or null for unsupported architectures instead of
defaulting to "x64". For "darwin" only accept "arm64" or "x64" (do not coerce
other values), for "win32" validate known Windows arches (e.g., "x64", "arm64")
before creating the matcher, and for "linux" validate supported linux arches
before returning an AppImage matcher; ensure the returned object or function
signals unsupported CPU (throw or return null) so the caller won't download an
incompatible binary.
- Around line 136-153: The current flow can leave the temp file if downloadFile
rejects and treats installForPlatform's false as a silent failure; wrap the
downloadFile + installForPlatform call inside a try/finally so
unlinkSync(downloadPath) always runs, propagate download errors, and normalize
install outcomes by either having installForPlatform throw on hard failures and
return a discriminated success value (e.g., 'installed' | 'opened-manually') or
by having installDesktop throw when installed === false; update the
installDesktop logic (the call site that uses downloadFile, installForPlatform,
unlinkSync and success/blank calls) to handle the discriminated return (show the
appropriate message for 'opened-manually') or rethrow to ensure the CLI exits
non-zero on real failures.
In `@apps/cli/src/start.ts`:
- Around line 202-240: Register the child crash supervision before the pairing
flow so exit events aren’t missed; move the loop that attaches
child.process.on("exit", ...) (which uses children, shuttingDown, warn, and
shutdown(1)) to run before the sleep(1500) and showPairCode() calls (i.e., set
up supervision immediately after children are spawned and before calling
showPairCode), ensuring the exit listeners are active during the pairing window.
- Around line 346-385: The startProcess promise lacks a ChildProcess "error"
handler, so if spawn fails the promise can hang; add an error listener on the
spawned child (the same ChildProcess referenced by "child") that, when
triggered, sets resolved = true, clears the timeout, removes the stdout "data"
listener (onStdout), and resolves the promise with null (and optionally performs
any cleanup/logging similar to the "exit" handler) so the function
(startProcess) always settles on spawn errors as well as normal exits.
---
Nitpick comments:
In `@apps/cli/src/desktop.ts`:
- Around line 199-205: The nested .then() on collectBody should be rewritten
using async/await to flatten control flow: await the result of collectBody(r)
into a variable, then run JSON.parse inside a try/catch and call resolve with
the parsed object or null on failure; apply the same change to the similar block
referenced at the other location (lines 216-221). Locate the callback where
collectBody(r).then(...) is used, replace the .then() callback with "const b =
await collectBody(r)" (or equivalent in the enclosing async function), then
perform "try { resolve(JSON.parse(b)) } catch { resolve(null) }".
🪄 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: defaults
Review profile: CHILL
Plan: Lite
Run ID: 35a6733a-65ad-427f-9218-13690a22b0d8
📒 Files selected for processing (3)
apps/cli/build.tsapps/cli/src/desktop.tsapps/cli/src/start.ts
- Wrap signal handlers to prevent passing signal name as exit code
(process.on("SIGINT", shutdown) → () => shutdown() to avoid TypeError)
- Validate download completeness: check res.complete and byte count
before resolving, propagate response errors to file stream
- Use bun publish in build.ts hint instead of npm publish
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 1103759c41b1
zvadaadam
force-pushed
the
zvadaadam/npm-cli-installer
branch
from
33e4e1d to
098727c
Compare
This was referenced Apr 4, 2026
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
1 participant
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
apps/cli/package published asdeus-machineon npm — smart auto-detection launches the desktop app on Mac/Windows or starts a headless server on Linux/SSH/Dockerdeus,deus start,deus install,deus pair,deus login,deus statusTest plan
bun run buildin apps/cli/)deus --helpanddeus --versionoutput correctlydeus statusanddeus pairhandle "no server running" gracefullydeus startspawns agent-server + backenddeus installdownloads and installs desktop app from GitHub releases🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Other