build(desktop): stabilize Windows dev and distribution workflows#449
Open
PerishCode wants to merge 48 commits intomainfrom
Open
build(desktop): stabilize Windows dev and distribution workflows#449PerishCode wants to merge 48 commits intomainfrom
PerishCode wants to merge 48 commits intomainfrom
Conversation
|
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:
📝 WalkthroughWalkthroughMigrates desktop dev tooling from Bash to a Node.js CLI, makes runtime manifest creation asynchronous, adds archive-based sidecar extraction (tar.gz and zip) with cache fingerprinting and Windows symlink fallbacks, introduces Windows packaging and smoke-test tooling, and adds EOL normalization via .gitattributes. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as dev-cli.mjs
participant Lock as Lock Manager
participant Cache as Sidecar Cache
participant Extract as Archive Extractor
participant Build as Build Pipeline
participant Electron as Electron Process
User->>CLI: start
CLI->>Lock: acquire single-instance lock
Lock-->>CLI: lock obtained
CLI->>Cache: check sidecar cache fingerprint
alt cache hit
Cache-->>CLI: cached sidecar usable
else cache miss
CLI->>Extract: extract packaged sidecar (tar.gz or zip)
Extract-->>CLI: sidecar prepared
CLI->>Cache: write cache metadata
end
CLI->>Build: check/run pnpm build tasks (unless reuse)
Build-->>CLI: build artifacts ready
CLI->>Electron: spawn detached electron
Electron-->>CLI: PID recorded
CLI->>Lock: persist session state and release lock
CLI-->>User: started (PID/logs)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dd6ed36760
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/scripts/prepare-openclaw-sidecar.mjs (1)
990-1016:⚠️ Potential issue | 🟠 MajorGive
openclaw.cmdthe same bundled-runner fallback as the POSIX launcher.The Windows wrapper always executes
node ..., while the shell launcher below falls back toOPENCLAW_ELECTRON_EXECUTABLE. If PATH does not expose the expected Node binary,dist:winhard-fails even though the packaged app already has a bundled runner available.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/scripts/prepare-openclaw-sidecar.mjs` around lines 990 - 1016, Update the Windows wrapper written by the writeFile call that creates "openclaw.cmd" so it mirrors the POSIX wrapper fallback: instead of unconditionally executing node with packagedOpenclawEntry, first check for node on PATH (e.g. using where/node check) and exec node if found; otherwise, if the OPENCLAW_ELECTRON_EXECUTABLE env var is set and points to an executable, run it with ELECTRON_RUN_AS_NODE=1 and the same packagedOpenclawEntry and arguments as the POSIX launcher. Keep the rest of the batch wrapper logic consistent with the POSIX script (use packagedOpenclawEntry and preserve %* argument passthrough).
🧹 Nitpick comments (5)
apps/desktop/scripts/lib/sidecar-paths.mjs (1)
67-84: Consider logging when falling back to copy on Windows.The Windows symlink failure fallback silently switches from symlink to copy semantics. While this works correctly for the current use cases, a log message would aid debugging when the two behaviors diverge (symlinks reflect source mutations; copies are snapshots).
🔧 Optional: Add fallback logging
} catch (error) { if (process.platform !== "win32") { throw error; } + console.log( + `[sidecar-paths] symlink failed, falling back to copy: ${sourcePath} -> ${targetPath}`, + ); await cp(sourcePath, targetPath, { recursive: true, dereference: true, filter: (source) => basename(source) !== ".bin", }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/scripts/lib/sidecar-paths.mjs` around lines 67 - 84, The catch block for symlink failure silently falls back to cp, so add a log entry before performing the copy to indicate the symlink failed and we're using a copy fallback; specifically, inside the catch(error) branch where process.platform === "win32" before calling cp(sourcePath, targetPath, ...), call the existing logger (or process.stdout/processLogger) to record a message containing the error and the sourcePath/targetPath and that a recursive copy will be used as fallback for symlink in the symlink => cp fallback code path.apps/desktop/scripts/prepare-runtime-sidecars.mjs (1)
14-23: Same argument quoting consideration asopenclaw-runtime/postinstall.mjs.The
join(" ")pattern doesn't quote arguments. Current usage (["run", script]) is safe, but the pattern should be consistent across both files if one is updated.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/scripts/prepare-runtime-sidecars.mjs` around lines 14 - 23, The createCommandSpec function builds a Windows cmd.exe invocation using ["pnpm", ...args].join(" ") which doesn't quote individual args; update createCommandSpec to safely quote or escape each argument when composing the single string for cmd.exe (handle spaces and special chars), e.g. transform args into a properly quoted string before passing it as the single /c argument, keeping the same branch for command === "pnpm" || command === "pnpm.cmd" and leaving the non-Windows return { command, args } unchanged.openclaw-runtime/postinstall.mjs (1)
14-23: Arguments containing spaces or special characters would break the command.The
join(" ")approach on line 18 doesn't quote arguments. While current usage only passes simple flags (ci,--no-audit, etc.), this pattern is fragile if future callers pass arguments with spaces or shell metacharacters.🛡️ Optional: Quote arguments for robustness
function createCommandSpec(command, args) { if (process.platform === "win32" && (command === "npm" || command === "npm.cmd")) { + const quotedArgs = args.map((arg) => + arg.includes(" ") || arg.includes('"') ? `"${arg.replace(/"/g, '\\"')}"` : arg + ); return { command: "cmd.exe", - args: ["/d", "/s", "/c", ["npm", ...args].join(" ")], + args: ["/d", "/s", "/c", ["npm", ...quotedArgs].join(" ")], }; } return { command, args }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openclaw-runtime/postinstall.mjs` around lines 14 - 23, The Windows branch in createCommandSpec builds a single command string with ["npm", ...args].join(" ") which fails for arguments containing spaces or shell metacharacters; change this to safely escape and quote each argument before joining (e.g., replace the join with args.map(escapeAndQuote).join(" ") where escapeAndQuote properly escapes backslashes/quotes and wraps the arg in double-quotes when needed) so the returned args array contains a single, robust cmd.exe /c string that preserves argument boundaries.smoke/Dockerfile (1)
1-5: Drop root for the smoke image.This container only needs to run a Node script; keeping the default root user adds avoidable privilege and can leave root-owned files in the mounted workspace.
Possible fix
FROM node:22-bookworm WORKDIR /workspace/smoke +USER node CMD ["node", "./feishu-ws-smoke.mjs"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@smoke/Dockerfile` around lines 1 - 5, The Dockerfile currently runs as root (base image node:22-bookworm) which is unnecessary; modify the Dockerfile to create or switch to a non-root user and ensure WORKDIR /workspace/smoke is owned by that user before switching: either use the existing unprivileged 'node' user from the base image (add chown -R node:node /workspace/smoke and then USER node) or add a dedicated user (e.g., addgroup/adduser, chown the WORKDIR, and then USER smoke); keep the existing CMD ["node", "./feishu-ws-smoke.mjs"] unchanged but ensure the node binary and script are executable by the non-root user.apps/desktop/scripts/dist-win.mjs (1)
184-236: Consider adding error handling for the main orchestration.The
main()function runs multiple sequential build steps but has no top-level try/catch. If an early step fails, the error message fromrun()will propagate, but adding explicit error context could improve debugging for Windows-specific failures.💡 Optional: Add top-level error handling with context
async function main() { + const startTime = Date.now(); const env = { ...process.env, NEXU_WORKSPACE_ROOT: repoRoot, }; // ... rest of function ... - await runElectronBuilder( - [ - "--win", - // ... - ], - { - cwd: electronRoot, - env: { - ...env, - CSC_IDENTITY_AUTO_DISCOVERY: "false", - }, - }, - ); + await runElectronBuilder( + [ + "--win", + // ... + ], + { + cwd: electronRoot, + env: { + ...env, + CSC_IDENTITY_AUTO_DISCOVERY: "false", + }, + }, + ); + console.log(`[dist:win] completed in ${((Date.now() - startTime) / 1000).toFixed(1)}s`); } -await main(); +await main().catch((error) => { + console.error("[dist:win] build failed:", error.message); + process.exit(1); +});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/scripts/dist-win.mjs` around lines 184 - 236, Wrap the orchestration in main() with top-level try/catch (or add a try/catch around the await calls when invoking main) to capture and log errors with Windows-specific context before exiting non-zero; catch errors thrown by run(), runElectronBuilder(), getElectronVersion(), execFileSync, etc., use a clear processLogger/error console message that includes the step or buildVersion/electronVersion context, and call process.exit(1) (or rethrow after logging) to ensure failures produce an explicit, contextualized error and non-zero exit code.
🤖 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/desktop/main/runtime/manifests.ts`:
- Around line 352-353: The branch that falls back to execFileSync("tar",
["-xzf", archivePath, "-C", tempExtractedSidecarRoot]) must be removed: when
archiveMetadata is missing or archiveMetadata.format === "tar.gz" replace the
PATH shell-out with an in-process extraction (e.g., require and call a JS tar
extractor such as the 'tar' module to extract archivePath into
tempExtractedSidecarRoot using streams and zlib) or, if we intentionally no
longer support legacy tar archives, throw a clear migration error referencing
archiveMetadata and archivePath; update the code paths around archiveMetadata,
execFileSync, and tempExtractedSidecarRoot accordingly so no PATH-dependent
external binary is invoked at startup.
In `@apps/desktop/scripts/dev-cli.mjs`:
- Around line 504-512: The cleanup currently kills whatever PID is in
state.electronPid and every PID returned by listListeningPids(defaultPorts);
change this to first validate each candidate PID before calling killPid by
checking its command line and/or working directory to ensure it belongs to this
workspace or is the expected electron binary. Specifically, when handling
state.electronPid (from readState) and each PID from
listListeningPids(defaultPorts), call a helper (e.g.,
getProcessCmdline/getProcessCwd) and only call killPid if the cmdline or cwd
contains the workspace identifier or matches the known electron executable name;
otherwise skip and log a warning so real port conflicts can surface (update the
logic around readState, state.electronPid, listListeningPids, and killPid to
perform this verification).
- Around line 679-689: Replace the manual file:// string assembly with
pathToFileURL to correctly encode Windows drive letters, spaces and UNC paths:
import pathToFileURL from the node 'url' module (e.g., add an ESM import for {
pathToFileURL }) and change the target in the control() function (the line that
currently sets target = `file://${join(appDir, "dist", "index.html")}`) to use
pathToFileURL(join(appDir, "dist", "index.html")).href (or .toString()) so the
spawn calls receive a correct, encoded file URL.
- Around line 26-35: createCommandSpec currently joins args into a single string
for cmd.exe which breaks when paths contain spaces; change it so on Windows you
either (A) quote each argument that contains spaces before joining (preserving
proper escaping) or preferably (B) avoid joining entirely and invoke the
resolved pnpm binary directly with an argument array to spawn/spawnSync.
Specifically, update createCommandSpec to resolve the pnpm binary
programmatically (use require.resolve or equivalent to locate the pnpm CLI
rather than relying on PATH) and return command set to that resolved binary and
args as an array (or, if sticking with cmd.exe, build a correctly quoted command
string by wrapping each arg with quotes when necessary) so that options like
"--dir" with rootDir/appDir containing spaces are passed intact. Ensure you
modify the branch that checks process.platform === "win32" and references "pnpm"
so it no longer constructs an unquoted "pnpm ...".
In `@apps/desktop/scripts/prepare-openclaw-sidecar.mjs`:
- Around line 472-495: The function currently waits for yazl's
ZipFile.outputStream to close but not for the destination write stream, which
can still be flushing; update createZipArchive (and the piping logic around
ZipFile.outputStream and createWriteStream) to capture the write stream (e.g.,
const writeStream = createWriteStream(archivePath)), pipe outputStream into
that, and wait for the write stream's 'finish' (and handle its 'error') before
resolving—i.e., replace or extend the existing completion promise that listens
on outputStream with listeners on the destination writeStream ('finish' and
'error') so the function only resolves once the file has been fully flushed to
disk.
In `@smoke/feishu-ws-smoke.mjs`:
- Around line 78-111: The argument parsing loop fails to check bounds and misses
a continue for the "--domain" flag; inside the function that iterates argv (the
for loop handling flags like "--reply", "--account", "--config", "--app-id",
"--app-secret", "--domain"), add a guard before reading argv[index + 1] (e.g.,
ensure index + 1 < argv.length) and handle the error case (return an error,
throw, or set a default) for each flag that consumes a value, and add the
missing continue after setting options.domain so control flow matches the other
flag cases.
---
Outside diff comments:
In `@apps/desktop/scripts/prepare-openclaw-sidecar.mjs`:
- Around line 990-1016: Update the Windows wrapper written by the writeFile call
that creates "openclaw.cmd" so it mirrors the POSIX wrapper fallback: instead of
unconditionally executing node with packagedOpenclawEntry, first check for node
on PATH (e.g. using where/node check) and exec node if found; otherwise, if the
OPENCLAW_ELECTRON_EXECUTABLE env var is set and points to an executable, run it
with ELECTRON_RUN_AS_NODE=1 and the same packagedOpenclawEntry and arguments as
the POSIX launcher. Keep the rest of the batch wrapper logic consistent with the
POSIX script (use packagedOpenclawEntry and preserve %* argument passthrough).
---
Nitpick comments:
In `@apps/desktop/scripts/dist-win.mjs`:
- Around line 184-236: Wrap the orchestration in main() with top-level try/catch
(or add a try/catch around the await calls when invoking main) to capture and
log errors with Windows-specific context before exiting non-zero; catch errors
thrown by run(), runElectronBuilder(), getElectronVersion(), execFileSync, etc.,
use a clear processLogger/error console message that includes the step or
buildVersion/electronVersion context, and call process.exit(1) (or rethrow after
logging) to ensure failures produce an explicit, contextualized error and
non-zero exit code.
In `@apps/desktop/scripts/lib/sidecar-paths.mjs`:
- Around line 67-84: The catch block for symlink failure silently falls back to
cp, so add a log entry before performing the copy to indicate the symlink failed
and we're using a copy fallback; specifically, inside the catch(error) branch
where process.platform === "win32" before calling cp(sourcePath, targetPath,
...), call the existing logger (or process.stdout/processLogger) to record a
message containing the error and the sourcePath/targetPath and that a recursive
copy will be used as fallback for symlink in the symlink => cp fallback code
path.
In `@apps/desktop/scripts/prepare-runtime-sidecars.mjs`:
- Around line 14-23: The createCommandSpec function builds a Windows cmd.exe
invocation using ["pnpm", ...args].join(" ") which doesn't quote individual
args; update createCommandSpec to safely quote or escape each argument when
composing the single string for cmd.exe (handle spaces and special chars), e.g.
transform args into a properly quoted string before passing it as the single /c
argument, keeping the same branch for command === "pnpm" || command ===
"pnpm.cmd" and leaving the non-Windows return { command, args } unchanged.
In `@openclaw-runtime/postinstall.mjs`:
- Around line 14-23: The Windows branch in createCommandSpec builds a single
command string with ["npm", ...args].join(" ") which fails for arguments
containing spaces or shell metacharacters; change this to safely escape and
quote each argument before joining (e.g., replace the join with
args.map(escapeAndQuote).join(" ") where escapeAndQuote properly escapes
backslashes/quotes and wraps the arg in double-quotes when needed) so the
returned args array contains a single, robust cmd.exe /c string that preserves
argument boundaries.
In `@smoke/Dockerfile`:
- Around line 1-5: The Dockerfile currently runs as root (base image
node:22-bookworm) which is unnecessary; modify the Dockerfile to create or
switch to a non-root user and ensure WORKDIR /workspace/smoke is owned by that
user before switching: either use the existing unprivileged 'node' user from the
base image (add chown -R node:node /workspace/smoke and then USER node) or add a
dedicated user (e.g., addgroup/adduser, chown the WORKDIR, and then USER smoke);
keep the existing CMD ["node", "./feishu-ws-smoke.mjs"] unchanged but ensure the
node binary and script are executable by the non-root user.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d323e986-21b4-41fb-a3f1-737e00964bee
⛔ Files ignored due to path filters (2)
openclaw-runtime/package-lock.jsonis excluded by!**/package-lock.jsonpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (22)
.gitattributesAGENTS.mdapps/desktop/dev.shapps/desktop/main/index.tsapps/desktop/main/runtime/manifests.tsapps/desktop/package.jsonapps/desktop/scripts/dev-cli.mjsapps/desktop/scripts/dist-win.mjsapps/desktop/scripts/lib/sidecar-paths.mjsapps/desktop/scripts/prepare-controller-sidecar.mjsapps/desktop/scripts/prepare-openclaw-sidecar.mjsapps/desktop/scripts/prepare-runtime-sidecars.mjsapps/desktop/scripts/prepare-web-sidecar.mjsopenclaw-runtime/postinstall.mjspackage.jsonscripts/dev.mjsscripts/setup-git-hooks.mjssmoke/Dockerfilesmoke/README.mdsmoke/feishu-ws-smoke.mjssmoke/package.jsonspecs/guides/desktop-runtime-guide.md
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/desktop/main/bootstrap.ts`:
- Around line 93-101: Wrap the renameSync call in a try/catch to prevent an
unhandled exception from crashing startup on Windows: inside the existing
conditional that checks process.platform, overrideUserDataPath, userDataPath vs
legacyWindowsUserDataPath and path existence, change userDataPath from const to
let, attempt the renameSync(legacyWindowsUserDataPath, userDataPath) inside a
try block, and in the catch log the error (including error.code like
EBUSY/EPERM) and fall back to continuing startup without renaming or choose an
alternate recovery (e.g., leave legacy path untouched or set a different
userDataPath); ensure error handling references renameSync,
legacyWindowsUserDataPath, and userDataPath so the app no longer crashes on
rename failures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e8956844-9023-433e-b30f-93254a5b638b
📒 Files selected for processing (3)
apps/desktop/main/bootstrap.tsapps/desktop/package.jsonapps/desktop/scripts/dist-win.mjs
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/desktop/package.json
- apps/desktop/scripts/dist-win.mjs
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/desktop/scripts/dist-win.mjs (1)
14-23: Windows command arguments with spaces will break.When
repoRootor other arguments contain spaces, joining args with a single space on Line 18 produces an incorrectly tokenized command. For example,--dir C:/Program Files/projectbecomes three separate arguments.♻️ Proposed fix: Quote arguments containing spaces
function createCommandSpec(command, args) { if (process.platform === "win32" && (command === "pnpm" || command === "pnpm.cmd")) { + const quotedArgs = args.map((arg) => + /[\s&|<>^]/.test(arg) ? `"${arg.replace(/"/g, '\\"')}"` : arg + ); return { command: "cmd.exe", - args: ["/d", "/s", "/c", ["pnpm", ...args].join(" ")], + args: ["/d", "/s", "/c", ["pnpm", ...quotedArgs].join(" ")], }; } return { command, args }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/scripts/dist-win.mjs` around lines 14 - 23, In createCommandSpec the PNPM command string is built with args.join(" "), which breaks when any arg (like repoRoot) contains spaces; change the construction to safely quote/escape individual arguments before joining so cmd.exe receives a single command string with preserved arguments. Implement a helper (used in createCommandSpec) that for each arg escapes embedded double quotes/backslashes and wraps the arg in double quotes if it contains whitespace or special chars, then build args: ["/d","/s","/c", ["pnpm", ...args].map(quoteAndEscape).join(" ")]; this preserves tokenization on Windows when invoking "cmd.exe".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/desktop/scripts/dist-win.mjs`:
- Around line 14-23: In createCommandSpec the PNPM command string is built with
args.join(" "), which breaks when any arg (like repoRoot) contains spaces;
change the construction to safely quote/escape individual arguments before
joining so cmd.exe receives a single command string with preserved arguments.
Implement a helper (used in createCommandSpec) that for each arg escapes
embedded double quotes/backslashes and wraps the arg in double quotes if it
contains whitespace or special chars, then build args: ["/d","/s","/c", ["pnpm",
...args].map(quoteAndEscape).join(" ")]; this preserves tokenization on Windows
when invoking "cmd.exe".
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e9d3f990-ef5a-4c18-becc-9269def6d6b7
📒 Files selected for processing (2)
apps/desktop/package.jsonapps/desktop/scripts/dist-win.mjs
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/package.json
Switch the NSIS flow to an assisted installer so Windows install and uninstall are visible and data cleanup can be opted into without silent surprises. Restore executable resource editing and use tombstone-based app-data cleanup so shortcuts show the Nexu icon and uninstall stays fast without flashing a console window.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/desktop/build/installer.nsh`:
- Around line 53-56: The RunOnce registry value name currently uses only
GetTickCount() (System::Call 'kernel32::GetTickCount() i .r1') which can collide
when multiple tombstones are enqueued (written via WriteRegStr with
NEXU_RUNONCE_VALUE_PREFIX and NEXU_CLEANUP_SCRIPT), causing later writes to
overwrite earlier ones; change the value name generation in both the
single-write block (where GetTickCount() is used) and the multi-enqueue block
(the code handling multiple tombstones) to include a unique suffix per
tombstone—either append the tombstone basename (derived from the tombstone
path/argument like $1) or append an incrementing counter variable that you
increment for each WriteRegStr call—so each WriteRegStr uses
NEXU_RUNONCE_VALUE_PREFIX + uniqueSuffix to avoid collisions.
- Line 8: The installer currently writes and executes a predictable helper at
NEXU_CLEANUP_SCRIPT in %TEMP% and uses GetTickCount() for RunOnce names,
allowing TOCTOU and name-collision replay; fix by removing the mutable .vbs
helper and instead schedule the cleanup inline (for example create a one-shot
delete command in the RunOnce value itself or spawn cmd.exe /c "timeout & del
..." from an installer-owned, non-world-writable location), or if a helper file
is unavoidable write it to a unique, non-temporary, single‑use path (include a
GUID/PID/timestamp in the filename) and mark it non-replayable; also replace
GetTickCount() as the RunOnce key with a cryptographically-unique identifier
(GUID) or include PID+high-resolution timestamp to ensure uniqueness and
one-shot behavior.
In `@apps/desktop/scripts/dist-win.mjs`:
- Around line 169-170: The console.log that prints the entire env-derived config
object (the line using console.log("[dist:win] generated build-config.json from
env:", JSON.stringify(config))) may leak secrets; change it to avoid printing
verbatim config values—use the existing configPath and either log only the
resolved file path (configPath) or a redacted summary such as a list of config
keys or masked values for sensitive keys before or after the await writeFile
call; update the log message referenced here so it no longer includes
JSON.stringify(config) but instead uses a safe summary (e.g., configPath and/or
redactedKeys) to preserve intent without exposing secrets.
- Around line 37-63: The current try/catch blocks around preparing sharp and
`@img` swallow all errors (logging "skipping ...") which can hide failures and
leave stale native binaries; update the error handling in the sharp dereference
logic (around lstat(sharpPath), realpath(sharpPath), rm/sharp cp) and the `@img`
copy logic (pnpmImgPath/sharpImgPath, lstat, rm, cp) to fail fast: after logging
the error include rethrowing the caught error (or simply remove the outer
try/catch so the exception propagates). Ensure you reference the same symbols
(sharpPath, pnpmImgPath, imgPath, lstat, realpath, rm, cp, rmWithRetriesOptions)
so the error bubbles up to packaging instead of silently continuing.
- Around line 14-19: In createCommandSpec, when building the cmd.exe /c command
for pnpm, the current args.join(" ") loses argument boundaries and breaks paths
with spaces; fix this by quoting/escaping each element in the args array before
joining (e.g., wrap each arg in quotes and escape any embedded quotes) so
arguments like --dir "C:\Users\Jane Doe\..." remain one token, or alternatively
avoid cmd.exe entirely and return command: "pnpm" with args: [...args] on
Windows so the spawn call can pass the argument array directly; update the logic
in createCommandSpec to perform one of these two safe strategies.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0962f68b-d171-46ef-865b-41b1bba250c5
📒 Files selected for processing (3)
apps/desktop/build/installer.nshapps/desktop/package.jsonapps/desktop/scripts/dist-win.mjs
* build(desktop): skip exe editing for unsigned Windows dist * fix(openclaw-runtime): stabilize Windows postinstall Keep the full install path available for debugging while making pruned installs skip Windows-native optional dependencies that break postinstall flows. * fix(desktop): align dev health check with runtime state * fix(openclaw-runtime): refresh cache on installer changes * build(desktop): add windows CI coverage * fix(desktop): avoid sidecar copy symlink loops on windows * fix(desktop): clean runtime plugin self-links on windows * build(desktop): scope windows CI to build validation * fix(desktop): restore mac dist executable discovery --------- Co-authored-by: mRcfps <1402401442@qq.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/desktop-ci-check.mjs (1)
546-549:⚠️ Potential issue | 🟡 MinorOrphaned
tmuxSessionreference is dead code.
collectAppProcessResults()never setstmuxSessionon the returned object (it only returnsmainProcessandauxiliaryProcess). This reference will always evaluate totruevia optional chaining fallback.🧹 Suggested cleanup
function probesPassed(results, diagnostics) { return ( results.portResults.every((entry) => entry.listening) && results.apiReady.body.includes('"ready":true') && results.webReady.body.includes('"ready":true') && results.webSurface.body.includes('<div id="root"></div>') && results.openclawHealth.ok && results.browserControlListening && - results.appProcessResults.mainProcess.alive && - (results.appProcessResults.tmuxSession?.alive ?? true) && + results.appProcessResults.mainProcess.alive && diagnosticsChecksPassed(diagnostics) ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/desktop-ci-check.mjs` around lines 546 - 549, The condition references a non-existent tmuxSession property on results.appProcessResults (collectAppProcessResults only returns mainProcess and auxiliaryProcess), so remove the orphaned "(results.appProcessResults.tmuxSession?.alive ?? true)" check; if you intended to verify the auxiliary process instead, replace it with "results.appProcessResults.auxiliaryProcess?.alive ?? true" so the code uses the actual property returned by collectAppProcessResults and keeps the existing mainProcess.alive and diagnosticsChecksPassed(diagnostics) checks.
♻️ Duplicate comments (6)
apps/desktop/scripts/dist-win.mjs (3)
199-204:⚠️ Potential issue | 🟠 MajorAvoid logging build config verbatim—may leak DSN or credential-bearing URLs.
Line 202 logs the entire
configobject includingNEXU_DESKTOP_SENTRY_DSNandNEXU_UPDATE_FEED_URLif present. This undercuts the log-sanitizing goal of the PR. As per coding guidelines, credentials must never appear in logs.Suggested fix: log only the file path
await writeFile(configPath, JSON.stringify(config, null, 2)); - console.log( - "[dist:win] generated build-config.json from env:", - JSON.stringify(config), - ); + console.log("[dist:win] generated build-config.json:", configPath);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/scripts/dist-win.mjs` around lines 199 - 204, The current console.log prints the entire config object (variable config) which can expose secrets like NEXU_DESKTOP_SENTRY_DSN and NEXU_UPDATE_FEED_URL; update the logging in the block after writeFile(configPath, ...) so it does not emit the full JSON: either log only the file path (configPath) or a sanitized version of config that redacts DSNs/URLs (remove or mask keys NEXU_DESKTOP_SENTRY_DSN and NEXU_UPDATE_FEED_URL) before stringifying; locate the console.log call in the dist-win.mjs snippet and replace it accordingly.
41-76:⚠️ Potential issue | 🟠 MajorSilently skipping sharp/
@imgfailures may cause downstream packaging errors.Both try/catch blocks log "skipping" for all errors, but the electron-builder config unconditionally bundles these paths as
extraResources. A partial dereference leaves stale or missing binaries, causing cryptic failures later. Consider distinguishingENOENT(acceptable to skip) from other errors (should fail fast).Suggested fix: fail fast on non-ENOENT errors
+function isEnoent(error) { + return error && typeof error === "object" && "code" in error && error.code === "ENOENT"; +} + async function dereferencePnpmSymlinks() { // ... sharp handling ... } catch (error) { - console.log( - `[dist:win] skipping sharp: ${error instanceof Error ? error.message : String(error)}`, - ); + if (isEnoent(error)) { + console.log(`[dist:win] sharp not present; skipping dereference`); + } else { + throw new Error(`Failed to materialize sharp: ${error instanceof Error ? error.message : String(error)}`); + } } // ... similar for `@img` ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/scripts/dist-win.mjs` around lines 41 - 76, The current try/catch around dereferencing sharp (using lstat(sharpPath), realpath, rm, cp) and the subsequent block that copies `@img` (using sharpImgPath, lstat, rm, cp) swallow all errors; change both catch handlers to only silently ignore errors whose error.code === 'ENOENT' and for any other error rethrow or exit (e.g., throw error or process.exit(1)) after logging the full error (include error.stack or error.message) so failures other than "not found" fail fast and surface useful diagnostics.
15-27:⚠️ Potential issue | 🟠 MajorWindows argument quoting loses path boundaries.
The
args.join(" ")on line 22 breaks when--dirpaths contain spaces (e.g.,C:\Users\Jane Doe\...). cmd.exe parses the joined string and splits at spaces. This is a build-time script, so it may work in CI environments, but will fail on developer machines with spaces in usernames.Suggested fix: quote arguments containing spaces
function createCommandSpec(command, args) { if ( process.platform === "win32" && (command === "pnpm" || command === "pnpm.cmd") ) { + const quotedArgs = args.map((arg) => + arg.includes(" ") ? `"${arg}"` : arg + ); return { command: "cmd.exe", - args: ["/d", "/s", "/c", ["pnpm", ...args].join(" ")], + args: ["/d", "/s", "/c", ["pnpm", ...quotedArgs].join(" ")], }; } return { command, args }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/scripts/dist-win.mjs` around lines 15 - 27, In createCommandSpec, the current use of ["pnpm", ...args].join(" ") loses path boundaries because args.join(" ") doesn't quote arguments with spaces; update the logic that builds the cmd.exe invocation so each argument that contains spaces or special cmd characters is wrapped in quotes (and inner quotes escaped) before joining, i.e., map over args to quote them when necessary instead of a raw args.join(" "), so the returned object for the pnpm branch still uses command: "cmd.exe" and args: ["/d","/s","/c", quotedJoinedString].apps/desktop/scripts/dev-cli.mjs (3)
770-781:⚠️ Potential issue | 🟡 MinorUse
pathToFileURL()for correct Windows file URL construction.
file://${join(...)}produces invalid URLs on Windows—the drive letter is interpreted as a protocol and spaces are not encoded. UsepathToFileURL()fromnode:urlwhich correctly handles drive letters, path separators, and percent-encoding.Suggested fix
-import { fileURLToPath } from "node:url"; +import { fileURLToPath, pathToFileURL } from "node:url";async function control() { - const target = `file://${join(appDir, "dist", "index.html")}`; + const target = pathToFileURL(join(appDir, "dist", "index.html")).href;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/scripts/dev-cli.mjs` around lines 770 - 781, The file URL is being constructed manually in the control() function using `file://${join(appDir, "dist", "index.html")}`, which produces invalid URLs on Windows; import `pathToFileURL` from 'url' and replace that manual construction with `pathToFileURL(join(appDir, "dist", "index.html")).toString()` (or .href) to produce a correct, encoded file URL; update the top-level imports to include `pathToFileURL` and use this new `target` variable in the existing spawn calls inside control().
26-38:⚠️ Potential issue | 🟠 MajorWindows argument quoting loses path boundaries.
Same issue as other files:
args.join(" ")on line 33 breaks paths containing spaces. Forpnpm --dir rootDircommands whererootDirmay beC:\Users\Jane Doe\..., this causes command parsing failures.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/scripts/dev-cli.mjs` around lines 26 - 38, The Windows branch in createCommandSpec builds a single cmd.exe /c string with args.join(" "), which loses path boundaries for args containing spaces (e.g., C:\Users\Jane Doe). Update the pnpm branch in createCommandSpec to safely quote/escape each arg before joining (e.g., map each arg to a quoted/escaped form like wrap in double quotes and escape any internal quotes) so you produce ["/d","/s","/c", ["pnpm", ...args.map(escapeAndQuote)].join(" ")]. Ensure the escaping handles existing quotes in args and use the same createCommandSpec function to locate the change.
547-563:⚠️ Potential issue | 🟠 MajorResidual process cleanup may kill unrelated services.
killResidualProcessesterminates any process listening on ports 18789/50800/50810 and any PID stored instate.json, without verifying the process belongs to this workspace. A stale state file or an unrelated local service on these ports will be killed. Consider validating the process command line or working directory before terminating.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/scripts/dev-cli.mjs` around lines 547 - 563, killResidualProcesses currently kills any PID from state.json and any PID returned by listListeningPids(defaultPorts) without confirming ownership; update it to validate target processes before calling killPid by inspecting each PID's command line or working directory (use a helper like getProcessCommand/getProcessCwd or platform ps utilities) and only kill if the cmd/cwd matches this workspace or clearly belongs to our app (e.g., contains "electron" or the repo name). For the state.electronPid path (readState -> state.electronPid) also verify the PID's cmd/cwd matches before killing, and if a PID is stale/unrelated, avoid killing and consider cleaning state via removeState only when safe. Ensure you update any helpers and callers (killResidualProcesses, listListeningPids, killPid, readState, removeState, defaultPorts) to perform these checks so unrelated services are not terminated.
🧹 Nitpick comments (5)
apps/desktop/scripts/lib/sidecar-paths.mjs (1)
147-156: Consider parallelizing entry processing for large directories.The sequential
awaitloop processes entries one at a time. For directories with many entries, this could be slow. Parallelizing with bounded concurrency could improve performance.♻️ Optional: Parallel entry processing with Promise.all
- for (const entry of entries) { - await copyDirectoryTree( - resolve(sourcePath, entry), - resolve(targetPath, entry), - { - ...options, - baseSourcePath: options.baseSourcePath ?? sourcePath, - }, - ); - } + await Promise.all( + entries.map((entry) => + copyDirectoryTree( + resolve(sourcePath, entry), + resolve(targetPath, entry), + { + ...options, + baseSourcePath: options.baseSourcePath ?? sourcePath, + }, + ), + ), + );Note: Sequential processing is safer and may be intentional to avoid file system contention. Only apply if performance profiling shows this is a bottleneck.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/scripts/lib/sidecar-paths.mjs` around lines 147 - 156, The for-await loop in copyDirectoryTree that sequentially awaits copyDirectoryTree for each entry (the block iterating over entries) should be changed to run entries in parallel with bounded concurrency: create a concurrency limit (either add a concurrency option to copyDirectoryTree or use a lightweight limiter like p-limit/semaphore) and map entries to tasks calling copyDirectoryTree(resolve(sourcePath, entry), resolve(targetPath, entry), {...options, baseSourcePath: options.baseSourcePath ?? sourcePath}), then await Promise.all on the limited tasks; ensure errors propagate and respect existing behavior (preserve baseSourcePath handling) and avoid unbounded parallelism to prevent filesystem contention..github/workflows/desktop-ci-dev.yml (2)
98-100: Windows step validates build pipeline but not runtime health.The Windows step runs
pnpm --filter@nexu/desktopbuildwhich exercises the build tooling but not the full runtime unit health check that macOS performs viapnpm check:dev. This is reasonable for initial Windows smoke testing—consider expanding to runtime checks once the Windows dev-cli flow is stable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/desktop-ci-dev.yml around lines 98 - 100, The Windows workflow step titled "Verify Windows desktop build pipeline" currently only runs "pnpm --filter `@nexu/desktop` build"; update that step to also run the runtime health check used on macOS by invoking "pnpm check:dev" (either run it after the build or combine both commands) so the Windows job validates both the build and the dev runtime health for the desktop package.
84-90: Consider separating toolchain version checks by platform.The PowerShell block with embedded
matrix.oscomparison is functional but could be cleaner with separate platform-specific steps. However, this is a minor concern and the current approach works.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/desktop-ci-dev.yml around lines 84 - 90, The "Show desktop toolchain versions" step mixes platform logic inside a pwsh run block using "${{ matrix.os }}"; split this into two clearer steps instead: keep the existing "Show desktop toolchain versions" step to always run pnpm exec electron --version (shell: pwsh), and add a separate conditional step (e.g., name "Show tmux version on macOS") that runs tmux -V with an if: matrix.os == 'macos' condition and shell: bash/pwsh as appropriate; reference the step name "Show desktop toolchain versions" and the matrix variable "matrix.os" to locate where to extract the platform-specific command.scripts/desktop-check-dist.mjs (1)
34-46: Quoting pattern present but not exercised with path arguments.The
createCommandSpecfunction has the sameargs.join(" ")pattern, but in this file it's only used fortaskkilland simple commands without user-provided paths. Low risk in current usage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/desktop-check-dist.mjs` around lines 34 - 46, createCommandSpec currently builds cmd.exe args by joining args with " " which can mis-handle path arguments with spaces; update the branch that returns {command: "cmd.exe", args: [...] } so that instead of ["pnpm", ...args].join(" ") you either (a) preserve argument boundaries by passing each arg as a separate element to cmd.exe (e.g., args: ["/d","/s","/c","pnpm", ...args]) if the consumer supports it, or (b) safely quote/escape each element (map args through a quoting/escaping helper) before joining, ensuring functions like createCommandSpec, the pnpm branch, and the cmd.exe arg construction handle path arguments containing spaces correctly.openclaw-runtime/install-runtime.mjs (1)
9-21: Quoting pattern note forcreateCommandSpec.The
args.join(" ")pattern on line 16 can break if any argument contains spaces. For this script's usage (npm commands with flags like--omit=peer,--no-audit), this is safe since no user-provided paths are passed. However, this pattern is repeated across multiple files in this PR—consider extracting a shared utility if future use cases need path arguments.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openclaw-runtime/install-runtime.mjs` around lines 9 - 21, The current createCommandSpec function uses args.join(" ") which can break on arguments containing spaces; replace that ad-hoc join with a shared safe-quoting/joining utility (e.g., shellQuoteArgs or joinCommandArgs) and use it wherever args.join(" ") appears across the PR so arguments are correctly escaped when forming the cmd.exe command string; update createCommandSpec to call that utility instead of args.join(" "), and add the new utility in a common module to reuse it in other files.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@scripts/desktop-ci-check.mjs`:
- Around line 546-549: The condition references a non-existent tmuxSession
property on results.appProcessResults (collectAppProcessResults only returns
mainProcess and auxiliaryProcess), so remove the orphaned
"(results.appProcessResults.tmuxSession?.alive ?? true)" check; if you intended
to verify the auxiliary process instead, replace it with
"results.appProcessResults.auxiliaryProcess?.alive ?? true" so the code uses the
actual property returned by collectAppProcessResults and keeps the existing
mainProcess.alive and diagnosticsChecksPassed(diagnostics) checks.
---
Duplicate comments:
In `@apps/desktop/scripts/dev-cli.mjs`:
- Around line 770-781: The file URL is being constructed manually in the
control() function using `file://${join(appDir, "dist", "index.html")}`, which
produces invalid URLs on Windows; import `pathToFileURL` from 'url' and replace
that manual construction with `pathToFileURL(join(appDir, "dist",
"index.html")).toString()` (or .href) to produce a correct, encoded file URL;
update the top-level imports to include `pathToFileURL` and use this new
`target` variable in the existing spawn calls inside control().
- Around line 26-38: The Windows branch in createCommandSpec builds a single
cmd.exe /c string with args.join(" "), which loses path boundaries for args
containing spaces (e.g., C:\Users\Jane Doe). Update the pnpm branch in
createCommandSpec to safely quote/escape each arg before joining (e.g., map each
arg to a quoted/escaped form like wrap in double quotes and escape any internal
quotes) so you produce ["/d","/s","/c", ["pnpm",
...args.map(escapeAndQuote)].join(" ")]. Ensure the escaping handles existing
quotes in args and use the same createCommandSpec function to locate the change.
- Around line 547-563: killResidualProcesses currently kills any PID from
state.json and any PID returned by listListeningPids(defaultPorts) without
confirming ownership; update it to validate target processes before calling
killPid by inspecting each PID's command line or working directory (use a helper
like getProcessCommand/getProcessCwd or platform ps utilities) and only kill if
the cmd/cwd matches this workspace or clearly belongs to our app (e.g., contains
"electron" or the repo name). For the state.electronPid path (readState ->
state.electronPid) also verify the PID's cmd/cwd matches before killing, and if
a PID is stale/unrelated, avoid killing and consider cleaning state via
removeState only when safe. Ensure you update any helpers and callers
(killResidualProcesses, listListeningPids, killPid, readState, removeState,
defaultPorts) to perform these checks so unrelated services are not terminated.
In `@apps/desktop/scripts/dist-win.mjs`:
- Around line 199-204: The current console.log prints the entire config object
(variable config) which can expose secrets like NEXU_DESKTOP_SENTRY_DSN and
NEXU_UPDATE_FEED_URL; update the logging in the block after
writeFile(configPath, ...) so it does not emit the full JSON: either log only
the file path (configPath) or a sanitized version of config that redacts
DSNs/URLs (remove or mask keys NEXU_DESKTOP_SENTRY_DSN and NEXU_UPDATE_FEED_URL)
before stringifying; locate the console.log call in the dist-win.mjs snippet and
replace it accordingly.
- Around line 41-76: The current try/catch around dereferencing sharp (using
lstat(sharpPath), realpath, rm, cp) and the subsequent block that copies `@img`
(using sharpImgPath, lstat, rm, cp) swallow all errors; change both catch
handlers to only silently ignore errors whose error.code === 'ENOENT' and for
any other error rethrow or exit (e.g., throw error or process.exit(1)) after
logging the full error (include error.stack or error.message) so failures other
than "not found" fail fast and surface useful diagnostics.
- Around line 15-27: In createCommandSpec, the current use of ["pnpm",
...args].join(" ") loses path boundaries because args.join(" ") doesn't quote
arguments with spaces; update the logic that builds the cmd.exe invocation so
each argument that contains spaces or special cmd characters is wrapped in
quotes (and inner quotes escaped) before joining, i.e., map over args to quote
them when necessary instead of a raw args.join(" "), so the returned object for
the pnpm branch still uses command: "cmd.exe" and args: ["/d","/s","/c",
quotedJoinedString].
---
Nitpick comments:
In @.github/workflows/desktop-ci-dev.yml:
- Around line 98-100: The Windows workflow step titled "Verify Windows desktop
build pipeline" currently only runs "pnpm --filter `@nexu/desktop` build"; update
that step to also run the runtime health check used on macOS by invoking "pnpm
check:dev" (either run it after the build or combine both commands) so the
Windows job validates both the build and the dev runtime health for the desktop
package.
- Around line 84-90: The "Show desktop toolchain versions" step mixes platform
logic inside a pwsh run block using "${{ matrix.os }}"; split this into two
clearer steps instead: keep the existing "Show desktop toolchain versions" step
to always run pnpm exec electron --version (shell: pwsh), and add a separate
conditional step (e.g., name "Show tmux version on macOS") that runs tmux -V
with an if: matrix.os == 'macos' condition and shell: bash/pwsh as appropriate;
reference the step name "Show desktop toolchain versions" and the matrix
variable "matrix.os" to locate where to extract the platform-specific command.
In `@apps/desktop/scripts/lib/sidecar-paths.mjs`:
- Around line 147-156: The for-await loop in copyDirectoryTree that sequentially
awaits copyDirectoryTree for each entry (the block iterating over entries)
should be changed to run entries in parallel with bounded concurrency: create a
concurrency limit (either add a concurrency option to copyDirectoryTree or use a
lightweight limiter like p-limit/semaphore) and map entries to tasks calling
copyDirectoryTree(resolve(sourcePath, entry), resolve(targetPath, entry),
{...options, baseSourcePath: options.baseSourcePath ?? sourcePath}), then await
Promise.all on the limited tasks; ensure errors propagate and respect existing
behavior (preserve baseSourcePath handling) and avoid unbounded parallelism to
prevent filesystem contention.
In `@openclaw-runtime/install-runtime.mjs`:
- Around line 9-21: The current createCommandSpec function uses args.join(" ")
which can break on arguments containing spaces; replace that ad-hoc join with a
shared safe-quoting/joining utility (e.g., shellQuoteArgs or joinCommandArgs)
and use it wherever args.join(" ") appears across the PR so arguments are
correctly escaped when forming the cmd.exe command string; update
createCommandSpec to call that utility instead of args.join(" "), and add the
new utility in a common module to reuse it in other files.
In `@scripts/desktop-check-dist.mjs`:
- Around line 34-46: createCommandSpec currently builds cmd.exe args by joining
args with " " which can mis-handle path arguments with spaces; update the branch
that returns {command: "cmd.exe", args: [...] } so that instead of ["pnpm",
...args].join(" ") you either (a) preserve argument boundaries by passing each
arg as a separate element to cmd.exe (e.g., args: ["/d","/s","/c","pnpm",
...args]) if the consumer supports it, or (b) safely quote/escape each element
(map args through a quoting/escaping helper) before joining, ensuring functions
like createCommandSpec, the pnpm branch, and the cmd.exe arg construction handle
path arguments containing spaces correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d37a11bb-ba0f-4fae-9bb8-728e36541544
📒 Files selected for processing (19)
.github/workflows/desktop-ci-dev.yml.github/workflows/desktop-ci-dist.ymlapps/desktop/scripts/dev-cli.mjsapps/desktop/scripts/dist-win.mjsapps/desktop/scripts/lib/sidecar-paths.mjsapps/desktop/scripts/prepare-controller-sidecar.mjsapps/desktop/scripts/prepare-openclaw-sidecar.mjsapps/desktop/scripts/prepare-runtime-sidecars.mjsopenclaw-runtime/README.mdopenclaw-runtime/install-runtime.mjsopenclaw-runtime/package.jsonopenclaw-runtime/postinstall-cache.mjsopenclaw-runtime/postinstall.mjspackage.jsonscripts/clean-runtime-plugin-installs.mjsscripts/desktop-check-dev.mjsscripts/desktop-check-dist.mjsscripts/desktop-ci-check.mjssmoke/feishu-ws-smoke.mjs
✅ Files skipped from review due to trivial changes (2)
- openclaw-runtime/postinstall-cache.mjs
- openclaw-runtime/README.md
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/desktop/scripts/prepare-runtime-sidecars.mjs
- apps/desktop/scripts/prepare-controller-sidecar.mjs
- smoke/feishu-ws-smoke.mjs
- apps/desktop/scripts/prepare-openclaw-sidecar.mjs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c96eecd4c4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Deploying nexu-docs with
|
| Latest commit: |
9887ae1
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://7e25c78e.nexu-docs.pages.dev |
| Branch Preview URL: | https://feat-windows-distribution-sm.nexu-docs.pages.dev |
* feat: add local dev supervisor workflow * feat: refine local dev workflow and desktop runtime scaffolding * docs: add dev workflow faq * fix: remove nested controller tsx watcher * refactor: share ensure guards across dev process helpers * chore: remove stale task notes * refactor: centralize local dev path resolution * refactor: move dev orchestration into scripts/dev Keep @nexu/dev-utils focused on atomic helpers so service-level controller and web flows stay easier to reason about and recover. Add lightweight session tracing so leaked dev processes can be correlated and cleaned up without heavy self-healing. * refactor: clarify scripts dev module boundaries * feat: externalize dev runtime ownership * feat: split local dev into explicit service controls * refactor: remove legacy desktop dev launchers * fix: run desktop local dev through the Vite supervisor * fix: harden local dev stack flow * chore: sync workspace lockfiles * fix: restore desktop dev auth session
…#651) * chore: introduce shared desktop lifecycle contract * chore: move desktop platform lifecycle behind adapters * chore: centralize desktop platform compatibility * chore: stage patched OpenClaw runtime for local dev * chore: log staged OpenClaw runtime usage * chore: speed up windows desktop build iteration * chore: disable win executable editing for local builds * chore: ignore local build cache
PerishCode
changed the title
PerishCode
changed the title
PerishCode
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Stabilize the Windows desktop path end to end across local development, packaged distribution, CI validation, and installer UX, while continuing the desktop platform/lifecycle cleanup that supports those flows.
Why
This branch grew into the main Windows stabilization line for the desktop product. Before these changes, the Windows path still had gaps in several places: local startup could be brittle, packaged runtime behavior still depended on platform-specific assumptions, Windows packaging was only partially covered in CI, and repeat installer runs did not clearly communicate upgrade, reinstall, or downgrade behavior. In parallel, parts of the desktop runtime lifecycle were still too spread across launchd- and platform-specific codepaths, which made the Windows work harder to extend safely.
How
dist:winpackaging flow and smoke-distribution supportdesktop-ci-dist-fullscripts/devflow and unify desktop dev launch underscripts/devscripts/devlogging while preserving the HMR-first control surfaceAffected areas
Checklist
pnpm typecheckpassespnpm lintpassespnpm testpassespnpm generate-typesrun (if API routes/schemas changed)anytypes introduced (useunknownwith narrowing)Notes for reviewers
Desktop CI Dist Fullwas manually triggered and passed with the added Windows packaging job.pnpm --filter @nexu/desktop dist:winwas rerun locally after the latest icon, installer, and release-cleanup changes.