feat(desktop): Windows native port#2196
Open
sad1k wants to merge 5 commits into
Open
Conversation
- Replace /bin/rm spawn with fs.rm() in git.ts - Replace process.env.HOME fallbacks with os.homedir() - Fix teardown shell selection to use getDefaultShell() - Create cross-platform postinstall.ts and lint.ts build scripts - Update package.json to use .ts scripts instead of .sh
- Add PowerShell 7 preference in getDefaultShell() with pwsh detection - Skip Unix locale probe on Windows, default to UTF-8 - Short-circuit shell-env on Windows (process.env is already complete) - Add PowerShell profile wrapper and cmd.exe init script writers - Add getWindowsShellArgs() and Windows getCommandShellArgs() - Return empty shell env on Windows (no ZDOTDIR/BASH_ENV needed) - Fix teardown: detached spawn guard + tree-kill for Windows process kill
- Guard SIGHUP handler (not emitted on Windows) - Fix detached spawn + windowsHide in terminal-host client - Create PowerShell templates for all 5 agent hooks - Update agent setup to select .ps1 vs .sh templates by platform
- Add Windows x64 build job to build-desktop.yml - Add Windows artifacts to release-desktop.yml - Add Windows test runner to ci.yml - Fix platform-conditional test expectations (FALLBACK_SHELL) - Add Windows buildSafeEnv tests - Skip bash-specific tests on Windows - Update README with Windows prerequisites and support status
- Patch node-pty winpty.gyp: hardcode commit hash and version to avoid bat file execution failures in gyp context, remove Spectre mitigation requirement (not installed by default in VS Build Tools) - Fix copy-native-modules.ts: use rmSync with recursive+force for Windows junctions (Bun creates junctions instead of symlinks) - Fix teardown.ts: use static import for tree-kill instead of await import() inside non-async callback - Add patch-node-pty-win.ts: automated postinstall script that patches node-pty gyp files on Windows - Disable code signing in electron-builder for development builds
Contributor
📝 WalkthroughWalkthroughThis PR adds comprehensive Windows support to the Superset desktop application. Changes include new GitHub Actions workflows for Windows builds and testing, Windows-specific PowerShell hook templates for AI agent integrations, updates to shell environment and process handling for Windows compatibility, new cross-platform build scripts, and Windows-specific node-pty patching. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
16 issues found across 34 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".omc/state/hud-stdin-cache.json">
<violation number="1" location=".omc/state/hud-stdin-cache.json:1">
P2: Remove this local `.omc` state cache file from version control; it contains machine-specific session metadata and should not be committed.</violation>
</file>
<file name=".github/workflows/ci.yml">
<violation number="1" location=".github/workflows/ci.yml:147">
P2: The Windows CI job pins Bun 1.3.2 while the rest of the workflow uses 1.3.6, which can cause version-specific test behavior and inconsistent CI results across platforms.</violation>
</file>
<file name=".omc/state/idle-notif-cooldown.json">
<violation number="1" location=".omc/state/idle-notif-cooldown.json:2">
P3: Avoid committing runtime cooldown state with a fixed timestamp; it creates noisy, non-deterministic diffs and can carry stale notification state across checkouts.
(Based on your team's feedback about keeping PRs focused and avoiding unrelated changes.) [FEEDBACK_USED]</violation>
</file>
<file name=".omc/state/hud-state.json">
<violation number="1" location=".omc/state/hud-state.json:2">
P3: Do not commit this runtime state file; it contains session-specific metadata that will change on every run and add unrelated noise to future PRs.
(Based on your team's feedback about keeping PRs focused and avoiding unrelated changes.) [FEEDBACK_USED]</violation>
</file>
<file name="apps/desktop/src/lib/trpc/routers/workspaces/utils/teardown.ts">
<violation number="1" location="apps/desktop/src/lib/trpc/routers/workspaces/utils/teardown.ts:120">
P2: Handle `treeKill` with a callback so kill failures are surfaced. The current fire-and-forget call can fail silently in the timeout path.
(Based on your team's feedback about handling async failures instead of silently swallowing them.) [FEEDBACK_USED]</violation>
</file>
<file name="apps/desktop/src/main/lib/agent-setup/templates/cursor-hook.template.ps1">
<violation number="1" location="apps/desktop/src/main/lib/agent-setup/templates/cursor-hook.template.ps1:44">
P2: The hook notification call uses an empty catch, so failures are invisible. Log the error (and allow the catch to run) so missed notifications can be debugged.
(Based on your team's feedback about avoiding empty catch blocks that hide failures.) [FEEDBACK_USED]</violation>
</file>
<file name=".github/workflows/build-desktop.yml">
<violation number="1" location=".github/workflows/build-desktop.yml:214">
P2: The Windows build step ignores the workflow’s `electron_builder_config` input, so caller-specified configs never apply to Windows artifacts. Pass the config through like the macOS/Linux jobs do.</violation>
</file>
<file name="apps/desktop/src/main/lib/agent-setup/templates/notify-hook.template.ps1">
<violation number="1" location="apps/desktop/src/main/lib/agent-setup/templates/notify-hook.template.ps1:70">
P2: Coerce optional env vars to empty strings before calling `EscapeDataString`. As written, any missing SUPERSET_* env value triggers `ArgumentNullException` and aborts the hook.</violation>
</file>
<file name="apps/desktop/src/main/lib/agent-setup/agent-wrappers-copilot.ts">
<violation number="1" location="apps/desktop/src/main/lib/agent-setup/agent-wrappers-copilot.ts:11">
P1: Windows now selects a `.ps1` hook script, but hooks JSON still defines only `bash` commands. Copilot CLI expects `powershell` commands on Windows, so Copilot hooks can fail to run there.</violation>
</file>
<file name="apps/desktop/src/main/lib/terminal/env.ts">
<violation number="1" location="apps/desktop/src/main/lib/terminal/env.ts:52">
P3: Don't swallow `execFileSync` failures silently here. Handle expected exit codes explicitly and log unexpected failures so command lookup issues on Windows are diagnosable.
(Based on your team's feedback about avoiding empty catch blocks that hide failures.) [FEEDBACK_USED]</violation>
</file>
<file name="apps/desktop/src/main/lib/agent-setup/templates/codex-wrapper-exec.template.ps1">
<violation number="1" location="apps/desktop/src/main/lib/agent-setup/templates/codex-wrapper-exec.template.ps1:8">
P2: `$$` is the last token of the previous line in PowerShell, not the process id. Use `$PID` (or another stable unique id) in the session log filename so concurrent runs don’t collide or pick up arbitrary tokens.</violation>
<violation number="2" location="apps/desktop/src/main/lib/agent-setup/templates/codex-wrapper-exec.template.ps1:27">
P3: Avoid an empty catch here so failures to invoke the notify hook aren’t silently swallowed. Log a warning with the error instead.
(Based on your team's feedback about avoiding empty catch blocks that hide failures.) [FEEDBACK_USED]</violation>
</file>
<file name="apps/desktop/src/main/lib/agent-setup/templates/gemini-hook.template.ps1">
<violation number="1" location="apps/desktop/src/main/lib/agent-setup/templates/gemini-hook.template.ps1:46">
P3: Don’t swallow failures silently; emit a warning so hook notification errors are traceable.
(Based on your team's feedback about avoiding empty catch blocks that hide failures.) [FEEDBACK_USED]</violation>
</file>
<file name="apps/desktop/src/main/lib/agent-setup/shell-wrappers.ts">
<violation number="1" location="apps/desktop/src/main/lib/agent-setup/shell-wrappers.ts:310">
P2: PowerShell interactive sessions will exit immediately after sourcing the wrapper because `-Command` runs and then quits. Add `-NoExit` so the shell stays open.</violation>
</file>
<file name="apps/desktop/electron-builder.ts">
<violation number="1" location="apps/desktop/electron-builder.ts:201">
P2: `signAndEditExecutable: false` disables Windows executable resource editing, so the configured app icon/metadata won’t be applied to the final `.exe`.</violation>
</file>
<file name="apps/desktop/src/main/lib/agent-setup/templates/copilot-hook.template.ps1">
<violation number="1" location="apps/desktop/src/main/lib/agent-setup/templates/copilot-hook.template.ps1:46">
P3: Don't swallow errors from the hook HTTP call; log a warning so failures are diagnosable when the local endpoint is unavailable.
(Based on your team's feedback about avoiding empty catch blocks that hide failures.) [FEEDBACK_USED]</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Add one-off context when rerunning by tagging
@cubic-dev-aiwith guidance or docs links (includingllms.txt) - Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| import { HOOKS_DIR } from "./paths"; | ||
|
|
||
| export const COPILOT_HOOK_SCRIPT_NAME = "copilot-hook.sh"; | ||
| export const COPILOT_HOOK_SCRIPT_NAME = |
Contributor
There was a problem hiding this comment.
P1: Windows now selects a .ps1 hook script, but hooks JSON still defines only bash commands. Copilot CLI expects powershell commands on Windows, so Copilot hooks can fail to run there.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/main/lib/agent-setup/agent-wrappers-copilot.ts, line 11:
<comment>Windows now selects a `.ps1` hook script, but hooks JSON still defines only `bash` commands. Copilot CLI expects `powershell` commands on Windows, so Copilot hooks can fail to run there.</comment>
<file context>
@@ -8,7 +8,8 @@ import {
import { HOOKS_DIR } from "./paths";
-export const COPILOT_HOOK_SCRIPT_NAME = "copilot-hook.sh";
+export const COPILOT_HOOK_SCRIPT_NAME =
+ process.platform === "win32" ? "copilot-hook.ps1" : "copilot-hook.sh";
</file context>
| @@ -0,0 +1 @@ | |||
| {"session_id":"4522eb68-617b-4b31-81bb-0a2843537759","transcript_path":"C:\\Users\\misha\\.claude\\projects\\C--Users-misha-Desktop-projects-superset-windows\\4522eb68-617b-4b31-81bb-0a2843537759.jsonl","cwd":"C:\\Users\\misha\\Desktop\\projects\\superset-windows\\temp-clone","model":{"id":"claude-opus-4-6","display_name":"Opus 4.6"},"workspace":{"current_dir":"C:\\Users\\misha\\Desktop\\projects\\superset-windows\\temp-clone","project_dir":"C:\\Users\\misha\\Desktop\\projects\\superset-windows","added_dirs":[]},"version":"2.1.71","output_style":{"name":"default"},"cost":{"total_cost_usd":8.046622950000001,"total_duration_ms":1558483,"total_api_duration_ms":1353354,"total_lines_added":2372,"total_lines_removed":29},"context_window":{"total_input_tokens":114151,"total_output_tokens":71600,"context_window_size":200000,"current_usage":{"input_tokens":3,"output_tokens":281,"cache_creation_input_tokens":317,"cache_read_input_tokens":139962},"used_percentage":70,"remaining_percentage":30},"exceeds_200k_tokens":false} No newline at end of file | |||
Contributor
There was a problem hiding this comment.
P2: Remove this local .omc state cache file from version control; it contains machine-specific session metadata and should not be committed.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .omc/state/hud-stdin-cache.json, line 1:
<comment>Remove this local `.omc` state cache file from version control; it contains machine-specific session metadata and should not be committed.</comment>
<file context>
@@ -0,0 +1 @@
+{"session_id":"4522eb68-617b-4b31-81bb-0a2843537759","transcript_path":"C:\\Users\\misha\\.claude\\projects\\C--Users-misha-Desktop-projects-superset-windows\\4522eb68-617b-4b31-81bb-0a2843537759.jsonl","cwd":"C:\\Users\\misha\\Desktop\\projects\\superset-windows\\temp-clone","model":{"id":"claude-opus-4-6","display_name":"Opus 4.6"},"workspace":{"current_dir":"C:\\Users\\misha\\Desktop\\projects\\superset-windows\\temp-clone","project_dir":"C:\\Users\\misha\\Desktop\\projects\\superset-windows","added_dirs":[]},"version":"2.1.71","output_style":{"name":"default"},"cost":{"total_cost_usd":8.046622950000001,"total_duration_ms":1558483,"total_api_duration_ms":1353354,"total_lines_added":2372,"total_lines_removed":29},"context_window":{"total_input_tokens":114151,"total_output_tokens":71600,"context_window_size":200000,"current_usage":{"input_tokens":3,"output_tokens":281,"cache_creation_input_tokens":317,"cache_read_input_tokens":139962},"used_percentage":70,"remaining_percentage":30},"exceeds_200k_tokens":false}
\ No newline at end of file
</file context>
| - name: Setup Bun | ||
| uses: oven-sh/setup-bun@v1 | ||
| with: | ||
| bun-version: "1.3.2" |
Contributor
There was a problem hiding this comment.
P2: The Windows CI job pins Bun 1.3.2 while the rest of the workflow uses 1.3.6, which can cause version-specific test behavior and inconsistent CI results across platforms.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/ci.yml, line 147:
<comment>The Windows CI job pins Bun 1.3.2 while the rest of the workflow uses 1.3.6, which can cause version-specific test behavior and inconsistent CI results across platforms.</comment>
<file context>
@@ -130,3 +130,25 @@ jobs:
+ - name: Setup Bun
+ uses: oven-sh/setup-bun@v1
+ with:
+ bun-version: "1.3.2"
+
+ - name: Install dependencies
</file context>
Suggested change
| bun-version: "1.3.2" | |
| bun-version: "1.3.6" |
| if (child.pid) { | ||
| if (process.platform === "win32") { | ||
| // Windows: can't use negative PID for process group kill | ||
| treeKill(child.pid, "SIGKILL"); |
Contributor
There was a problem hiding this comment.
P2: Handle treeKill with a callback so kill failures are surfaced. The current fire-and-forget call can fail silently in the timeout path.
(Based on your team's feedback about handling async failures instead of silently swallowing them.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/lib/trpc/routers/workspaces/utils/teardown.ts, line 120:
<comment>Handle `treeKill` with a callback so kill failures are surfaced. The current fire-and-forget call can fail silently in the timeout path.
(Based on your team's feedback about handling async failures instead of silently swallowing them.) </comment>
<file context>
@@ -114,7 +114,14 @@ export async function runTeardown({
+ if (child.pid) {
+ if (process.platform === "win32") {
+ // Windows: can't use negative PID for process group kill
+ treeKill(child.pid, "SIGKILL");
+ } else {
+ process.kill(-child.pid, "SIGKILL");
</file context>
Suggested change
| treeKill(child.pid, "SIGKILL"); | |
| treeKill(child.pid, "SIGKILL", (err) => { | |
| if (err) { | |
| console.warn("[teardown] Failed to SIGKILL process tree:", err.message); | |
| } | |
| }); |
| $url = "$baseUrl`?" + ($queryParams -join "&") | ||
|
|
||
| try { | ||
| Invoke-RestMethod -Uri $url -Method GET -TimeoutSec 2 -ErrorAction SilentlyContinue | Out-Null |
Contributor
There was a problem hiding this comment.
P2: The hook notification call uses an empty catch, so failures are invisible. Log the error (and allow the catch to run) so missed notifications can be debugged.
(Based on your team's feedback about avoiding empty catch blocks that hide failures.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/main/lib/agent-setup/templates/cursor-hook.template.ps1, line 44:
<comment>The hook notification call uses an empty catch, so failures are invisible. Log the error (and allow the catch to run) so missed notifications can be debugged.
(Based on your team's feedback about avoiding empty catch blocks that hide failures.) </comment>
<file context>
@@ -0,0 +1,47 @@
+$url = "$baseUrl`?" + ($queryParams -join "&")
+
+try {
+ Invoke-RestMethod -Uri $url -Method GET -TimeoutSec 2 -ErrorAction SilentlyContinue | Out-Null
+} catch {}
+
</file context>
| @@ -0,0 +1,6 @@ | |||
| { | |||
| "timestamp": "2026-03-07T10:28:39.751Z", | |||
Contributor
There was a problem hiding this comment.
P3: Do not commit this runtime state file; it contains session-specific metadata that will change on every run and add unrelated noise to future PRs.
(Based on your team's feedback about keeping PRs focused and avoiding unrelated changes.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .omc/state/hud-state.json, line 2:
<comment>Do not commit this runtime state file; it contains session-specific metadata that will change on every run and add unrelated noise to future PRs.
(Based on your team's feedback about keeping PRs focused and avoiding unrelated changes.) </comment>
<file context>
@@ -0,0 +1,6 @@
+{
+ "timestamp": "2026-03-07T10:28:39.751Z",
+ "backgroundTasks": [],
+ "sessionStartTimestamp": "2026-03-07T10:22:08.995Z",
</file context>
Comment on lines
+52
to
+54
| } catch { | ||
| return false; | ||
| } |
Contributor
There was a problem hiding this comment.
P3: Don't swallow execFileSync failures silently here. Handle expected exit codes explicitly and log unexpected failures so command lookup issues on Windows are diagnosable.
(Based on your team's feedback about avoiding empty catch blocks that hide failures.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/main/lib/terminal/env.ts, line 52:
<comment>Don't swallow `execFileSync` failures silently here. Handle expected exit codes explicitly and log unexpected failures so command lookup issues on Windows are diagnosable.
(Based on your team's feedback about avoiding empty catch blocks that hide failures.) </comment>
<file context>
@@ -37,6 +45,15 @@ export const HOOK_PROTOCOL_VERSION = "2";
+ try {
+ execFileSync("where", [cmd], { stdio: "ignore", timeout: 2000 });
+ return true;
+ } catch {
+ return false;
+ }
</file context>
Suggested change
| } catch { | |
| return false; | |
| } | |
| } catch (err) { | |
| const error = err as { status?: number }; | |
| if (error.status !== 1) { | |
| console.warn("[terminal/env] command lookup failed", err); | |
| } | |
| return false; | |
| } |
| $_payload = "{`"hook_event_name`":`"$_event`"}" | ||
| try { | ||
| & powershell.exe -NonInteractive -NoProfile -File "$_notify" "$_payload" 2>$null | Out-Null | ||
| } catch {} |
Contributor
There was a problem hiding this comment.
P3: Avoid an empty catch here so failures to invoke the notify hook aren’t silently swallowed. Log a warning with the error instead.
(Based on your team's feedback about avoiding empty catch blocks that hide failures.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/main/lib/agent-setup/templates/codex-wrapper-exec.template.ps1, line 27:
<comment>Avoid an empty catch here so failures to invoke the notify hook aren’t silently swallowed. Log a warning with the error instead.
(Based on your team's feedback about avoiding empty catch blocks that hide failures.) </comment>
<file context>
@@ -0,0 +1,92 @@
+ $_payload = "{`"hook_event_name`":`"$_event`"}"
+ try {
+ & powershell.exe -NonInteractive -NoProfile -File "$_notify" "$_payload" 2>$null | Out-Null
+ } catch {}
+ }
+
</file context>
|
|
||
| try { | ||
| Invoke-RestMethod -Uri $url -Method GET -TimeoutSec 2 -ErrorAction SilentlyContinue | Out-Null | ||
| } catch {} |
Contributor
There was a problem hiding this comment.
P3: Don’t swallow failures silently; emit a warning so hook notification errors are traceable.
(Based on your team's feedback about avoiding empty catch blocks that hide failures.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/main/lib/agent-setup/templates/gemini-hook.template.ps1, line 46:
<comment>Don’t swallow failures silently; emit a warning so hook notification errors are traceable.
(Based on your team's feedback about avoiding empty catch blocks that hide failures.) </comment>
<file context>
@@ -0,0 +1,48 @@
+
+try {
+ Invoke-RestMethod -Uri $url -Method GET -TimeoutSec 2 -ErrorAction SilentlyContinue | Out-Null
+} catch {}
+
+exit 0
</file context>
|
|
||
| try { | ||
| Invoke-RestMethod -Uri $url -Method GET -TimeoutSec 2 -ErrorAction SilentlyContinue | Out-Null | ||
| } catch {} |
Contributor
There was a problem hiding this comment.
P3: Don't swallow errors from the hook HTTP call; log a warning so failures are diagnosable when the local endpoint is unavailable.
(Based on your team's feedback about avoiding empty catch blocks that hide failures.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/main/lib/agent-setup/templates/copilot-hook.template.ps1, line 46:
<comment>Don't swallow errors from the hook HTTP call; log a warning so failures are diagnosable when the local endpoint is unavailable.
(Based on your team's feedback about avoiding empty catch blocks that hide failures.) </comment>
<file context>
@@ -0,0 +1,48 @@
+
+try {
+ Invoke-RestMethod -Uri $url -Method GET -TimeoutSec 2 -ErrorAction SilentlyContinue | Out-Null
+} catch {}
+
+exit 0
</file context>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
apps/desktop/src/main/lib/terminal-host/client.ts (1)
1106-1143:⚠️ Potential issue | 🔴 CriticalWindows IPC will fail despite Windows-specific spawn handling.
The spawn call includes Windows-specific process flags (
windowsHide,detachedcheck), but the IPC mechanism still uses Unix domain sockets exclusively. Both client (connect(SOCKET_PATH)) and daemon (newServer.listen(SOCKET_PATH)) hardcodeterminal-host.sockas the endpoint. On Windows, Node.js does not support Unix domain sockets via filesystem paths—it requires named pipes with paths like\\.\pipe\terminal-host. TheexistsSync()andunlinkSync()checks inwaitForDaemon(),tryConnectControl(), andtryConnectStream()will not work for Windows named pipes since they don't exist as filesystem files. The daemon will start successfully but fail to accept connections, leaving the client unable to establish IPC even though the subprocess spawned correctly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/lib/terminal-host/client.ts` around lines 1106 - 1143, The code currently uses a filesystem Unix domain socket name ("terminal-host.sock") everywhere (SOCKET_PATH used by connect(SOCKET_PATH) and newServer.listen(SOCKET_PATH)) and filesystem checks (existsSync/unlinkSync) in waitForDaemon(), tryConnectControl(), and tryConnectStream(), which fails on Windows because Node requires named pipes (e.g. \\.\pipe\<name>) and those aren't regular files; update the SOCKET_PATH generation to be platform-aware (use a named pipe path like \\.\pipe\terminal-host on win32, keep a filesystem socket on POSIX), update newServer.listen and client connect calls to use that platform-specific SOCKET_PATH, and replace existsSync/unlinkSync checks in waitForDaemon/tryConnectControl/tryConnectStream with platform-aware logic (skip file existence/unlink on Windows or use appropriate pipe-ready checks) so the daemon and client can connect on Windows. Ensure references to SOCKET_PATH, waitForDaemon, tryConnectControl, tryConnectStream, newServer.listen, and connect(SOCKET_PATH) are updated accordingly.apps/desktop/src/main/terminal-host/session.ts (1)
950-964:⚠️ Potential issue | 🟠 MajorDefault sessions still bypass the shared Windows shell resolver.
request.shellis forwarded as-is, so any caller that omits it still falls back toSession.getDefaultShell(), which returnscmd.exeon Windows. That skips the newmain/lib/terminal/env.ts#getDefaultShell()pwsh preference for the common create-session path.Suggested fix
-import { buildSafeEnv } from "../lib/terminal/env"; +import { buildSafeEnv, getDefaultShell } from "../lib/terminal/env"; ... - shell: request.shell, + shell: request.shell || getDefaultShell(),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/terminal-host/session.ts` around lines 950 - 964, createSession forwards request.shell directly so callers that omit it fall back to Session.getDefaultShell() (which returns cmd.exe on Windows) instead of using the shared resolver in main/lib/terminal/env.ts#getDefaultShell(); change the session creation to use the shared resolver when request.shell is undefined (i.e. set the shell value to request.shell ?? getDefaultShell()), and add the necessary import for getDefaultShell from main/lib/terminal/env.ts so the common create-session path respects the pwsh preference on Windows.apps/desktop/src/main/lib/terminal/env.ts (1)
57-69:⚠️ Potential issue | 🟠 MajorMove the Windows branch ahead of the
defaultShellfast path.On Windows,
defaultShellis returned before the newpwsh.exedetection runs. Sincedefault-shellresolves to theCOMSPECenvironment variable and typically yields the full path tocmd.exeon Windows, machines will exit the function early with their existing default shell and never reach the new PowerShell 7 preference check.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/lib/terminal/env.ts` around lines 57 - 69, The getDefaultShell function currently returns defaultShell before checking for pwsh on Windows; reorder the logic so the Windows branch is evaluated before the defaultShell fast path: inside getDefaultShell, first detect platform (os.platform()) and if platform === "win32" run commandExistsOnWindows("pwsh.exe") and return "pwsh.exe" or COMSPEC/powershell.exe as needed, otherwise fall back to checking defaultShell and the non-Windows logic; update references to defaultShell and the Windows branch accordingly so pwsh detection isn't bypassed.apps/desktop/src/main/lib/agent-setup/agent-wrappers-copilot.ts (1)
11-23:⚠️ Potential issue | 🟠 MajorUpdate
getCopilotHooksJsonContent()to use thepowershellfield on Windows andbashfield on Unix.The code correctly switches to a
.ps1template on Windows (lines 11–23), butgetCopilotHooksJsonContent()(lines 46–81) always generates hooks JSON with thebashfield. GitHub Copilot CLI requires thepowershellfield on Windows andbashfield on Unix; usingbashfor a PowerShell script will cause the hooks to fail silently on Windows. Update the function to conditionally set the field name based onprocess.platform.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/lib/agent-setup/agent-wrappers-copilot.ts` around lines 11 - 23, getCopilotHooksJsonContent() currently always emits a "bash" field so Windows PowerShell hook templates fail; modify getCopilotHooksJsonContent to choose the JSON field name based on process.platform (use "powershell" on win32, "bash" on other platforms) and populate that field with the template content referenced by COPILOT_HOOK_TEMPLATE_PATH/COPILOT_HOOK_MARKER; update the code that builds the hooks object (inside getCopilotHooksJsonContent) to set either hooksRecord.powershell = scriptContent or hooksRecord.bash = scriptContent accordingly, leaving other keys intact.
🧹 Nitpick comments (3)
apps/desktop/electron-builder.ts (1)
201-202: Consider adding a comment explaining why code signing is disabled.Disabling
signAndEditExecutableandforceCodeSigningaffects all Windows builds, including production releases. While this may be intentional for the initial Windows port, unsigned executables will trigger Windows SmartScreen warnings for end users.📝 Suggested comment addition
+ // TODO: Enable code signing for production Windows releases + // Currently disabled to unblock initial Windows support signAndEditExecutable: false, forceCodeSigning: false,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/electron-builder.ts` around lines 201 - 202, The Electron builder config currently sets signAndEditExecutable: false and forceCodeSigning: false which disables code signing for Windows builds; add a concise comment above these properties in electron-builder.ts explaining why signing is disabled (e.g., initial Windows port/CI limitations), the potential SmartScreen impact for end users, and include a TODO or ticket reference and instructions for enabling signing in production (how to set signAndEditExecutable/forceCodeSigning true and where to provide certificates). Ensure the comment clearly references the signAndEditExecutable and forceCodeSigning flags so future readers know the intent and next steps..github/workflows/ci.yml (1)
144-150: Bun version and action version mismatch with other CI jobs.The Windows test job uses
oven-sh/setup-bun@v1with Bun1.3.2, while all other jobs use@v2with Bun1.3.6. This inconsistency could lead to different behavior across platforms.Additionally, the Windows job is missing the dependency caching step that other jobs have, which will slow down CI runs.
🔧 Proposed fix to align versions and add caching
- name: Setup Bun - uses: oven-sh/setup-bun@v1 + uses: oven-sh/setup-bun@v2 with: - bun-version: "1.3.2" + bun-version: 1.3.6 + + - name: Cache dependencies + uses: actions/cache@v4 + with: + path: ~/.bun/install/cache + key: ${{ runner.os }}-bun-${{ hashFiles('bun.lock') }} - name: Install dependencies🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 144 - 150, The Windows job's "Setup Bun" and "Install dependencies" steps are inconsistent with other jobs; change the action from oven-sh/setup-bun@v1 to oven-sh/setup-bun@v2 and bump bun-version from "1.3.2" to "1.3.6" in the "Setup Bun" step, and add the same dependency caching step used by other jobs (the cache step that runs before "Install dependencies") so Bun dependencies are cached and "Install dependencies" (bun install --frozen) benefits from that cache.apps/desktop/src/main/lib/agent-setup/templates/notify-hook.template.ps1 (1)
83-93: Status code is hardcoded; consider capturing actual response status.The debug output reports
status=200without checking the actual HTTP response code. The bash template captures the real status viacurl -w "%{http_code}". Additionally, the$responsevariable is assigned but never used.Proposed fix to capture actual status
try { if ($DEBUG_HOOKS_ENABLED -eq "1") { - $response = Invoke-RestMethod -Uri $url -Method GET -TimeoutSec 2 -ErrorAction SilentlyContinue - Write-Error "[notify-hook] dispatched status=200" + $webResponse = Invoke-WebRequest -Uri $url -Method GET -TimeoutSec 2 -ErrorAction Stop + Write-Error "[notify-hook] dispatched status=$($webResponse.StatusCode)" } else { Invoke-RestMethod -Uri $url -Method GET -TimeoutSec 2 -ErrorAction SilentlyContinue | Out-Null } } catch { if ($DEBUG_HOOKS_ENABLED -eq "1") { - Write-Error "[notify-hook] dispatched status=error" + $statusCode = if ($_.Exception.Response) { [int]$_.Exception.Response.StatusCode } else { "error" } + Write-Error "[notify-hook] dispatched status=$statusCode" } }Note:
Invoke-WebRequestis used for debug path to accessStatusCode. The non-debug path can remain as-is since status isn't needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/lib/agent-setup/templates/notify-hook.template.ps1` around lines 83 - 93, The debug branch currently hardcodes "status=200" and assigns $response without using it; change the debug branch to call Invoke-WebRequest (or use Invoke-RestMethod and capture its full response) so you can read the actual HTTP status code (use $response.StatusCode) and include that value in the Write-Error message instead of "200"; keep the non-debug branch behavior unchanged, and ensure the $response variable is actually assigned and referenced when $DEBUG_HOOKS_ENABLED is "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 @.github/workflows/build-desktop.yml:
- Around line 197-203: The Windows "Compile app" step that runs "bun run
compile:app" only injects SENTRY_AUTH_TOKEN and SENTRY_DSN_DESKTOP, causing
missing auth/endpoints in the Windows renderer build; update that job step to
export the same build-time environment variables used in the macOS/Linux compile
steps (the web/API endpoint URLs and OAuth client IDs in addition to the Sentry
vars) so "bun run compile:app" receives the full set of config values at compile
time.
- Around line 212-217: The Windows "Build installer" step currently ignores the
workflow input inputs.electron_builder_config; update that step to pass the
selected config into the desktop build command so Windows uses the same builder
config as macOS/Linux — e.g. in the "Build installer" job add either an
environment variable (ELECTRON_BUILDER_CONFIG: ${{
inputs.electron_builder_config }}) or append a CLI flag to the bun run build
invocation (bun run build -- --config ${{ inputs.electron_builder_config }}) so
the build script/electron-builder receives inputs.electron_builder_config and
produces the correct Windows artifact.
In @.omc/state/agent-replay-4522eb68-617b-4b31-81bb-0a2843537759.jsonl:
- Around line 1-10: The file
.omc/state/agent-replay-4522eb68-617b-4b31-81bb-0a2843537759.jsonl is a
session-specific runtime artifact and should be removed from the PR: delete that
file from the commit, and add the .omc/state/ directory to the repository
.gitignore so future session logs aren’t tracked; if the file was already
committed previously, remove it from git history/tracking (e.g., git rm
--cached) and commit the updated .gitignore and removal so the session log is no
longer in version control.
In @.omc/state/hud-state.json:
- Around line 1-6: Remove the committed session snapshot
.omc/state/hud-state.json from the PR and stop tracking the entire runtime state
directory by adding .omc/state/ to .gitignore; then remove the file from the
repository index so it is no longer tracked and create a follow-up commit that
only updates .gitignore (and removes the tracked hud-state.json) without
re-adding runtime state artifacts.
In @.omc/state/hud-stdin-cache.json:
- Line 1: Remove the committed session cache file
.omc/state/hud-stdin-cache.json from the PR and stop tracking the entire
.omc/state/ directory: delete hud-stdin-cache.json from the commit (and rewrite
history or remove the file from the branch if already pushed), add .omc/state/
to .gitignore, and untrack any existing files under .omc/state so local session
paths, usernames, and runtime metrics are not in the repo; verify no other
sensitive files from .omc/state are staged or committed before pushing.
In @.omc/state/idle-notif-cooldown.json:
- Around line 1-3: Delete the runtime state artifact containing the "lastSentAt"
key from the commit (remove the JSON file), add an ignore rule for the
.omc/state/ directory to .gitignore so future runtime artifacts aren’t tracked,
and if the file is already tracked run the appropriate git untrack step (e.g.
git rm --cached) then commit the removal and .gitignore change.
In @.omc/state/subagent-tracking.json:
- Around line 1-44: This file contains transient runtime tracking data (see
top-level "agents" array and fields like "agent_id" and "started_at") and should
not be committed: remove the JSON file from the PR (git rm or stage its
deletion), ensure the directory containing this runtime state is added to your
repository ignore rules (.gitignore) so future files are not tracked, and if the
file was previously tracked use git rm --cached to untrack it before committing
the ignore change.
In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/shell-env.ts`:
- Around line 57-68: The Windows branch currently short-circuits and returns raw
process.env (inside the process.platform === "win32" block), which prevents
shell/profile PATH resolution for getProcessEnvWithShellPath() and
execWithShellEnv(); instead, invoke the selected shell/init script to produce
the environment (or at minimum the PATH) and merge those values into process.env
before returning; update the logic in the function that contains the platform
switch (the block around process.platform === "win32") to run the same
shell-derived env resolution used for macOS/Linux, merge the resulting PATH and
any other shell-provided variables with process.env, and return that combined
env so callers like getProcessEnvWithShellPath(), execWithShellEnv(), and
downstream users (e.g. gh/rg/git callers) see the shell-derived PATH on Windows.
In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/teardown.ts`:
- Around line 45-46: The current teardown logic lets process.env.SHELL override
the shell selection even on Windows, which can force Git Bash/MSYS shells;
change the assignment in teardown.ts so that on Windows (process.platform ===
'win32') you always use getDefaultShell(), and only allow process.env.SHELL to
override on non-Windows platforms—i.e., replace the current const shell =
process.env.SHELL || getDefaultShell() with a platform-gated expression so Shell
selection is correct for getDefaultShell() and the teardown routine.
In `@apps/desktop/src/main/lib/agent-setup/shell-wrappers.ts`:
- Around line 296-324: getWindowsShellArgs currently returns PowerShell args
that dot-source profilePath with -NoProfile and -Command which causes PowerShell
to exit; update the logic in getWindowsShellArgs so that when profilePath exists
you remove the conflicting "-NoProfile", add "-NoExit" and return something like
["-NoLogo", "-NoExit", "-ExecutionPolicy", "Bypass", "-Command", `.
'${profilePath}'`] (refer to the getWindowsShellArgs function and the
profilePath variable) so the profile is sourced and the interactive session
stays open.
In
`@apps/desktop/src/main/lib/agent-setup/templates/codex-wrapper-exec.template.ps1`:
- Line 84: The Codex PowerShell template is never used because
createCodexWrapper() and buildCodexWrapperExecLine() only generate POSIX
scripts; update both functions to detect process.platform === "win32" and select
the PS1 template (the codex-wrapper-exec.template.ps1) and a Windows-specific
exec line when on Windows, otherwise keep the existing SH template/line for
POSIX. Modify buildCodexWrapperExecLine() to return the PowerShell exec
invocation that references $env:REAL_BIN for Windows and the existing bash
invocation for non-Windows, and update createCodexWrapper() to write the chosen
template file (PS1 vs SH) and wire up the correct exec line so the PS1 template
is actually emitted and used on Windows.
In `@apps/desktop/src/main/lib/agent-setup/templates/copilot-hook.template.ps1`:
- Around line 14-20: The switch mapping adds "preToolUse" => "PermissionRequest"
but the hook is never emitted; update getCopilotHooksJsonContent in
agent-wrappers-copilot.ts to include a "preToolUse" hook entry in the generated
hooks JSON (matching the exact "preToolUse" key used in the PowerShell switch)
so Copilot can invoke that branch and trigger PermissionRequest notifications;
ensure the hook name and payload shape match other hooks emitted by
getCopilotHooksJsonContent and the PowerShell template.
In `@apps/desktop/src/main/lib/agent-setup/templates/cursor-hook.template.ps1`:
- Around line 1-47: The Cursor wrapper hardcodes the Unix script name so Windows
PowerShell template never loads; update the CURSOR_HOOK_SCRIPT_NAME and
CURSOR_HOOK_TEMPLATE_PATH in agent-wrappers-cursor.ts to detect platform (use
process.platform === "win32" ? "cursor-hook.ps1" : "cursor-hook.sh") and set the
corresponding template path similarly (choose the .ps1 template when win32, .sh
otherwise) so the Windows cursor-hook.template.ps1 can be used on Windows.
Ensure the rest of the code uses these constants unchanged.
In `@apps/desktop/src/main/lib/agent-setup/templates/gemini-hook.template.ps1`:
- Around line 7-11: The template assigns to PowerShell's automatic variable
$INPUT which can cause unexpected behavior; rename that variable (e.g.
$JsonInput or $StdinContent) and update all references (the -match extraction
that sets $EVENT_TYPE) to use the new name; ensure $EVENT_TYPE and the regex
logic remain unchanged, only replace occurrences of $INPUT with the new variable
to avoid colliding with PowerShell automatic variables.
In `@apps/desktop/src/main/lib/agent-setup/templates/notify-hook.template.ps1`:
- Around line 5-9: Rename the PowerShell variable $INPUT to a non-conflicting
name (e.g. $InputJson) in notify-hook.template.ps1 to avoid clobbering the
automatic $INPUT pipeline variable; update the assignment logic that currently
sets $INPUT (the conditional that assigns from $args or
[Console]::In.ReadToEnd()) to assign to $InputJson instead, and replace every
subsequent reference of $INPUT (including uses at the places currently
referencing it later in the template) with $InputJson so all reads/writes use
the new name.
In `@apps/desktop/src/main/lib/terminal-host/client.ts`:
- Around line 1106-1115: The spawn invocation that creates the daemon currently
sets detached to process.platform !== "win32", causing Windows children to be
tied to the parent's job and killed on parent exit; change the detached option
to true unconditionally in the spawn call that creates child (the
spawn(process.execPath, [daemonScript], { ... }) block) so the daemon can
outlive the Electron process, keep the existing windowsHide handling for
Windows, and ensure the spawned child is unref'd afterwards (child.unref()) if
not already done.
In `@apps/desktop/src/main/terminal-host/signal-handlers.ts`:
- Around line 130-140: Remove the platform guard around the SIGHUP handler so
the listener is registered on all OSes; specifically, delete the surrounding if
(process.platform !== "win32") check and ensure process.on("SIGHUP", ...)
remains and calls shutdownOnce with the same
exitCode/message/stopServerErrorMessage/timeoutMessage payload so stopServer()
cleanup runs when the console closes on Windows as well as other platforms.
In `@scripts/lint.ts`:
- Around line 14-17: The diagnostic check using the regex /Found \d+
(error|info|warning)/ fails for plural outputs like "Found 4 errors"; update the
test that inspects the output variable so the regex matches plural forms (e.g.,
allow trailing "s" for error/info/warning) or use a plural-aware pattern such as
matching (error|info|warning)s? so the if (/.../.test(output)) correctly detects
Biome summaries and calls process.exit(1).
In `@scripts/patch-node-pty-win.ts`:
- Around line 14-27: The script currently hardcodes "node-pty@1.1.0" into
nodePtyBase and silently exits when existsSync(nodePtyBase) is false; update the
logic to either (a) read the node-pty version from apps/desktop/package.json
(parse dependencies/devDependencies to build the path dynamically) and use that
computed directory for nodePtyBase, or (b) at minimum replace the silent
console.log + process.exit with a clear warning that includes the expected path
and a hint to check apps/desktop/package.json (i.e., change the existsSync
branch to processLogger.warn or console.warn with the missing path and exit
code), and ensure references to nodePtyBase and the existsSync check are updated
accordingly so the script fails fast with diagnostic output rather than silently
skipping the patch.
---
Outside diff comments:
In `@apps/desktop/src/main/lib/agent-setup/agent-wrappers-copilot.ts`:
- Around line 11-23: getCopilotHooksJsonContent() currently always emits a
"bash" field so Windows PowerShell hook templates fail; modify
getCopilotHooksJsonContent to choose the JSON field name based on
process.platform (use "powershell" on win32, "bash" on other platforms) and
populate that field with the template content referenced by
COPILOT_HOOK_TEMPLATE_PATH/COPILOT_HOOK_MARKER; update the code that builds the
hooks object (inside getCopilotHooksJsonContent) to set either
hooksRecord.powershell = scriptContent or hooksRecord.bash = scriptContent
accordingly, leaving other keys intact.
In `@apps/desktop/src/main/lib/terminal-host/client.ts`:
- Around line 1106-1143: The code currently uses a filesystem Unix domain socket
name ("terminal-host.sock") everywhere (SOCKET_PATH used by connect(SOCKET_PATH)
and newServer.listen(SOCKET_PATH)) and filesystem checks (existsSync/unlinkSync)
in waitForDaemon(), tryConnectControl(), and tryConnectStream(), which fails on
Windows because Node requires named pipes (e.g. \\.\pipe\<name>) and those
aren't regular files; update the SOCKET_PATH generation to be platform-aware
(use a named pipe path like \\.\pipe\terminal-host on win32, keep a filesystem
socket on POSIX), update newServer.listen and client connect calls to use that
platform-specific SOCKET_PATH, and replace existsSync/unlinkSync checks in
waitForDaemon/tryConnectControl/tryConnectStream with platform-aware logic (skip
file existence/unlink on Windows or use appropriate pipe-ready checks) so the
daemon and client can connect on Windows. Ensure references to SOCKET_PATH,
waitForDaemon, tryConnectControl, tryConnectStream, newServer.listen, and
connect(SOCKET_PATH) are updated accordingly.
In `@apps/desktop/src/main/lib/terminal/env.ts`:
- Around line 57-69: The getDefaultShell function currently returns defaultShell
before checking for pwsh on Windows; reorder the logic so the Windows branch is
evaluated before the defaultShell fast path: inside getDefaultShell, first
detect platform (os.platform()) and if platform === "win32" run
commandExistsOnWindows("pwsh.exe") and return "pwsh.exe" or
COMSPEC/powershell.exe as needed, otherwise fall back to checking defaultShell
and the non-Windows logic; update references to defaultShell and the Windows
branch accordingly so pwsh detection isn't bypassed.
In `@apps/desktop/src/main/terminal-host/session.ts`:
- Around line 950-964: createSession forwards request.shell directly so callers
that omit it fall back to Session.getDefaultShell() (which returns cmd.exe on
Windows) instead of using the shared resolver in
main/lib/terminal/env.ts#getDefaultShell(); change the session creation to use
the shared resolver when request.shell is undefined (i.e. set the shell value to
request.shell ?? getDefaultShell()), and add the necessary import for
getDefaultShell from main/lib/terminal/env.ts so the common create-session path
respects the pwsh preference on Windows.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 144-150: The Windows job's "Setup Bun" and "Install dependencies"
steps are inconsistent with other jobs; change the action from
oven-sh/setup-bun@v1 to oven-sh/setup-bun@v2 and bump bun-version from "1.3.2"
to "1.3.6" in the "Setup Bun" step, and add the same dependency caching step
used by other jobs (the cache step that runs before "Install dependencies") so
Bun dependencies are cached and "Install dependencies" (bun install --frozen)
benefits from that cache.
In `@apps/desktop/electron-builder.ts`:
- Around line 201-202: The Electron builder config currently sets
signAndEditExecutable: false and forceCodeSigning: false which disables code
signing for Windows builds; add a concise comment above these properties in
electron-builder.ts explaining why signing is disabled (e.g., initial Windows
port/CI limitations), the potential SmartScreen impact for end users, and
include a TODO or ticket reference and instructions for enabling signing in
production (how to set signAndEditExecutable/forceCodeSigning true and where to
provide certificates). Ensure the comment clearly references the
signAndEditExecutable and forceCodeSigning flags so future readers know the
intent and next steps.
In `@apps/desktop/src/main/lib/agent-setup/templates/notify-hook.template.ps1`:
- Around line 83-93: The debug branch currently hardcodes "status=200" and
assigns $response without using it; change the debug branch to call
Invoke-WebRequest (or use Invoke-RestMethod and capture its full response) so
you can read the actual HTTP status code (use $response.StatusCode) and include
that value in the Write-Error message instead of "200"; keep the non-debug
branch behavior unchanged, and ensure the $response variable is actually
assigned and referenced when $DEBUG_HOOKS_ENABLED is "1".
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fc72777b-ff77-40d4-8bf2-5d2efa0b91c0
📒 Files selected for processing (34)
.github/workflows/build-desktop.yml.github/workflows/ci.yml.github/workflows/release-desktop.yml.omc/state/agent-replay-4522eb68-617b-4b31-81bb-0a2843537759.jsonl.omc/state/hud-state.json.omc/state/hud-stdin-cache.json.omc/state/idle-notif-cooldown.json.omc/state/subagent-tracking.jsonREADME.mdapps/desktop/electron-builder.tsapps/desktop/scripts/copy-native-modules.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/git.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/shell-env.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/teardown.tsapps/desktop/src/main/lib/agent-setup/agent-wrappers-copilot.tsapps/desktop/src/main/lib/agent-setup/agent-wrappers-gemini.tsapps/desktop/src/main/lib/agent-setup/notify-hook.tsapps/desktop/src/main/lib/agent-setup/shell-wrappers.tsapps/desktop/src/main/lib/agent-setup/templates/codex-wrapper-exec.template.ps1apps/desktop/src/main/lib/agent-setup/templates/copilot-hook.template.ps1apps/desktop/src/main/lib/agent-setup/templates/cursor-hook.template.ps1apps/desktop/src/main/lib/agent-setup/templates/gemini-hook.template.ps1apps/desktop/src/main/lib/agent-setup/templates/notify-hook.template.ps1apps/desktop/src/main/lib/terminal-host/client.tsapps/desktop/src/main/lib/terminal/env.test.tsapps/desktop/src/main/lib/terminal/env.tsapps/desktop/src/main/terminal-host/session.test.tsapps/desktop/src/main/terminal-host/session.tsapps/desktop/src/main/terminal-host/signal-handlers.tsapps/desktop/src/main/terminal-host/terminal-host.tspackage.jsonscripts/lint.tsscripts/patch-node-pty-win.tsscripts/postinstall.ts
Comment on lines
+197
to
+203
| - name: Compile app | ||
| working-directory: apps/desktop | ||
| run: bun run compile:app | ||
| env: | ||
| SENTRY_AUTH_TOKEN: ${{ secrets.SENTRY_AUTH_TOKEN }} | ||
| SENTRY_DSN_DESKTOP: ${{ secrets.SENTRY_DSN_DESKTOP }} | ||
|
|
Contributor
There was a problem hiding this comment.
Pass the same build-time config into the Windows compile step.
compile:app only gets Sentry values here, while the macOS/Linux jobs also inject the web/API URLs and OAuth client IDs. If those values are compiled into the renderer bundle, the Windows artifact will ship with missing auth and endpoint config.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/build-desktop.yml around lines 197 - 203, The Windows
"Compile app" step that runs "bun run compile:app" only injects
SENTRY_AUTH_TOKEN and SENTRY_DSN_DESKTOP, causing missing auth/endpoints in the
Windows renderer build; update that job step to export the same build-time
environment variables used in the macOS/Linux compile steps (the web/API
endpoint URLs and OAuth client IDs in addition to the Sentry vars) so "bun run
compile:app" receives the full set of config values at compile time.
Comment on lines
+212
to
+217
| - name: Build installer | ||
| working-directory: apps/desktop | ||
| run: bun run build | ||
| env: | ||
| CSC_IDENTITY_AUTO_DISCOVERY: "false" | ||
|
|
Contributor
There was a problem hiding this comment.
The Windows packaging step drops inputs.electron_builder_config.
That workflow input is honored on macOS/Linux but ignored here, so callers can select an alternate builder config for those jobs and silently get a different Windows artifact from the same workflow invocation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/build-desktop.yml around lines 212 - 217, The Windows
"Build installer" step currently ignores the workflow input
inputs.electron_builder_config; update that step to pass the selected config
into the desktop build command so Windows uses the same builder config as
macOS/Linux — e.g. in the "Build installer" job add either an environment
variable (ELECTRON_BUILDER_CONFIG: ${{ inputs.electron_builder_config }}) or
append a CLI flag to the bun run build invocation (bun run build -- --config ${{
inputs.electron_builder_config }}) so the build script/electron-builder receives
inputs.electron_builder_config and produces the correct Windows artifact.
Comment on lines
+1
to
+10
| {"t":0,"agent":"system","event":"skill_invoked","skill_name":"superpowers:writing-plans"} | ||
| {"t":0,"agent":"system","event":"skill_invoked","skill_name":"superpowers:subagent-driven-development"} | ||
| {"t":0,"agent":"a4320be","agent_type":"executor","event":"agent_start","parent_mode":"none"} | ||
| {"t":0,"agent":"a2e0742","agent_type":"executor","event":"agent_start","parent_mode":"none"} | ||
| {"t":0,"agent":"a2e0742","agent_type":"executor","event":"agent_stop","success":true,"duration_ms":37720} | ||
| {"t":0,"agent":"a4320be","agent_type":"executor","event":"agent_stop","success":true,"duration_ms":164717} | ||
| {"t":0,"agent":"ab03026","agent_type":"executor","event":"agent_start","parent_mode":"none"} | ||
| {"t":0,"agent":"a63e06f","agent_type":"executor","event":"agent_start","parent_mode":"none"} | ||
| {"t":0,"agent":"ab03026","agent_type":"executor","event":"agent_stop","success":true,"duration_ms":61737} | ||
| {"t":0,"agent":"a63e06f","agent_type":"executor","event":"agent_stop","success":true,"duration_ms":85796} |
Contributor
There was a problem hiding this comment.
Remove session replay log from version control.
This JSONL file is a session-specific event log (note the session ID in the filename: 4522eb68-617b-4b31-81bb-0a2843537759) that records agent lifecycle events for a single development session. Event logs like this are runtime artifacts that should not be committed.
Remove this file from the PR and ensure the .omc/state/ directory is added to .gitignore.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.omc/state/agent-replay-4522eb68-617b-4b31-81bb-0a2843537759.jsonl around
lines 1 - 10, The file
.omc/state/agent-replay-4522eb68-617b-4b31-81bb-0a2843537759.jsonl is a
session-specific runtime artifact and should be removed from the PR: delete that
file from the commit, and add the .omc/state/ directory to the repository
.gitignore so future session logs aren’t tracked; if the file was already
committed previously, remove it from git history/tracking (e.g., git rm
--cached) and commit the updated .gitignore and removal so the session log is no
longer in version control.
Comment on lines
+1
to
+6
| { | ||
| "timestamp": "2026-03-07T10:28:39.751Z", | ||
| "backgroundTasks": [], | ||
| "sessionStartTimestamp": "2026-03-07T10:22:08.995Z", | ||
| "sessionId": "4522eb68-617b-4b31-81bb-0a2843537759" | ||
| } No newline at end of file |
Contributor
There was a problem hiding this comment.
Remove session state snapshot from version control.
This HUD state file contains session-specific data (session ID, timestamps, background tasks) that represents a snapshot from a single development session. Like other runtime state artifacts, this should not be committed to the repository.
Remove this file from the PR and ensure the .omc/state/ directory is added to .gitignore.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.omc/state/hud-state.json around lines 1 - 6, Remove the committed session
snapshot .omc/state/hud-state.json from the PR and stop tracking the entire
runtime state directory by adding .omc/state/ to .gitignore; then remove the
file from the repository index so it is no longer tracked and create a follow-up
commit that only updates .gitignore (and removes the tracked hud-state.json)
without re-adding runtime state artifacts.
| @@ -0,0 +1 @@ | |||
| {"session_id":"4522eb68-617b-4b31-81bb-0a2843537759","transcript_path":"C:\\Users\\misha\\.claude\\projects\\C--Users-misha-Desktop-projects-superset-windows\\4522eb68-617b-4b31-81bb-0a2843537759.jsonl","cwd":"C:\\Users\\misha\\Desktop\\projects\\superset-windows\\temp-clone","model":{"id":"claude-opus-4-6","display_name":"Opus 4.6"},"workspace":{"current_dir":"C:\\Users\\misha\\Desktop\\projects\\superset-windows\\temp-clone","project_dir":"C:\\Users\\misha\\Desktop\\projects\\superset-windows","added_dirs":[]},"version":"2.1.71","output_style":{"name":"default"},"cost":{"total_cost_usd":8.046622950000001,"total_duration_ms":1558483,"total_api_duration_ms":1353354,"total_lines_added":2372,"total_lines_removed":29},"context_window":{"total_input_tokens":114151,"total_output_tokens":71600,"context_window_size":200000,"current_usage":{"input_tokens":3,"output_tokens":281,"cache_creation_input_tokens":317,"cache_read_input_tokens":139962},"used_percentage":70,"remaining_percentage":30},"exceeds_200k_tokens":false} No newline at end of file | |||
Contributor
There was a problem hiding this comment.
Remove session cache with exposed local paths from version control.
This session cache file is particularly problematic because it contains:
- Developer-specific Windows paths:
C:\\Users\\misha\\Desktop\\projects\\superset-windows - Local username exposure:
misha - Session-specific data: session ID, transcript paths, cost metrics
- Runtime metrics: token usage, API costs, performance data
This represents both a repository pollution issue and a privacy/security concern. Local file paths and developer information should never be committed.
Remove this file from the PR and ensure the .omc/state/ directory is added to .gitignore.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.omc/state/hud-stdin-cache.json at line 1, Remove the committed session
cache file .omc/state/hud-stdin-cache.json from the PR and stop tracking the
entire .omc/state/ directory: delete hud-stdin-cache.json from the commit (and
rewrite history or remove the file from the branch if already pushed), add
.omc/state/ to .gitignore, and untrack any existing files under .omc/state so
local session paths, usernames, and runtime metrics are not in the repo; verify
no other sensitive files from .omc/state are staged or committed before pushing.
Comment on lines
+5
to
+9
| if ($args.Count -gt 0) { | ||
| $INPUT = $args[0] | ||
| } else { | ||
| $INPUT = [Console]::In.ReadToEnd() | ||
| } |
Contributor
There was a problem hiding this comment.
Rename $INPUT to avoid conflict with PowerShell automatic variable.
$INPUT is a built-in automatic variable in PowerShell (enumerator for pipeline input). Assigning to it can cause subtle issues or unexpected behavior.
Proposed fix
if ($args.Count -gt 0) {
- $INPUT = $args[0]
+ $InputJson = $args[0]
} else {
- $INPUT = [Console]::In.ReadToEnd()
+ $InputJson = [Console]::In.ReadToEnd()
}Also update all subsequent references from $INPUT to $InputJson (lines 15, 18, 20, 30, 34).
🧰 Tools
🪛 PSScriptAnalyzer (1.24.0)
[warning] 6-6: The Variable 'INPUT' is an automatic variable that is built into PowerShell, assigning to it might have undesired side effects. If assignment is not by design, please use a different name.
(PSAvoidAssignmentToAutomaticVariable)
[warning] 8-8: The Variable 'INPUT' is an automatic variable that is built into PowerShell, assigning to it might have undesired side effects. If assignment is not by design, please use a different name.
(PSAvoidAssignmentToAutomaticVariable)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main/lib/agent-setup/templates/notify-hook.template.ps1`
around lines 5 - 9, Rename the PowerShell variable $INPUT to a non-conflicting
name (e.g. $InputJson) in notify-hook.template.ps1 to avoid clobbering the
automatic $INPUT pipeline variable; update the assignment logic that currently
sets $INPUT (the conditional that assigns from $args or
[Console]::In.ReadToEnd()) to assign to $InputJson instead, and replace every
subsequent reference of $INPUT (including uses at the places currently
referencing it later in the template) with $InputJson so all reads/writes use
the new name.
Comment on lines
1106
to
1115
| child = spawn(process.execPath, [daemonScript], { | ||
| detached: true, | ||
| detached: process.platform !== "win32", | ||
| stdio: logFd >= 0 ? ["ignore", logFd, logFd] : "ignore", | ||
| env: { | ||
| ...process.env, | ||
| ELECTRON_RUN_AS_NODE: "1", | ||
| NODE_ENV: process.env.NODE_ENV, | ||
| }, | ||
| ...(process.platform === "win32" ? { windowsHide: true } : {}), | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check the file and the specific lines
head -n 1120 apps/desktop/src/main/lib/terminal-host/client.ts | tail -n 30Repository: superset-sh/superset
Length of output: 876
🏁 Script executed:
# Get more context around the spawn call - show broader method
sed -n '1090,1130p' apps/desktop/src/main/lib/terminal-host/client.tsRepository: superset-sh/superset
Length of output: 1020
🏁 Script executed:
# Check what happens after the spawn in waitForDaemon or similar
rg -A 10 "spawnDaemon" apps/desktop/src/main/lib/terminal-host/client.ts | head -n 50Repository: superset-sh/superset
Length of output: 1233
🏁 Script executed:
# Look for the full context of child.unref() and waitForDaemon() calls
sed -n '1110,1150p' apps/desktop/src/main/lib/terminal-host/client.tsRepository: superset-sh/superset
Length of output: 971
🏁 Script executed:
# Check if there are any comments or documentation about why detached is conditional
rg -B 5 "detached:" apps/desktop/src/main/lib/terminal-host/client.tsRepository: superset-sh/superset
Length of output: 265
🌐 Web query:
Node.js child_process spawn detached false Windows job object parent exit
💡 Result:
On Windows, Node’s child_process.spawn() ultimately goes through libuv. libuv creates a global Windows Job Object configured with JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE, then assigns every spawned child to that job unless you set detached: true. When the parent Node process exits, its handle to the job is closed, and Windows terminates all processes in that job—so children spawned with the default detached: false typically die when the parent dies (even on crash). [2]
Setting detached: true tells Node/libuv to not put the child into that kill-on-close job, which is why the Node docs say that on Windows it “makes it possible for the child process to continue running after the parent exits.” [1][2]
Two important caveats:
- If the parent process itself is already running inside some other Job (e.g., created by a launcher/service/container) that has kill-on-close semantics and doesn’t allow breakaway, your “detached” child may still be forced into that job and get killed anyway. Windows breakaway only works if the job allows it (e.g.,
JOB_OBJECT_LIMIT_BREAKAWAY_OK) and the process is created with breakaway permission/flags. [3] detachedaffects survival, but not whether the parent “waits”; for true fire-and-forget you usually also needchild.unref()and stdio not tied to the parent (per Node docs). [1]
Sources: Node child_process docs [1]; libuv Windows process implementation (job object + kill-on-close + “not detached ⇒ assigned to job”) [2]; Windows job-object breakaway constraints [3].
[1] (nodejs.org)
[2] (raw.githubusercontent.com)
[3] (stackoverflow.com)
Citations:
- 1: https://nodejs.org/download/nightly/v26.0.0-nightly2026022776215dc993/docs/api/child_process.html?utm_source=openai
- 2: https://raw.githubusercontent.com/libuv/libuv/271d173b/src/win/process.c
- 3: https://stackoverflow.com/questions/76876658/stopping-children-being-terminated-when-parent-process-was-started-in-a-job-i-ca?utm_source=openai
🏁 Script executed:
# Check if there are any related comments or issues about Windows daemon behavior in the codebase
rg -i "windows.*daemon|daemon.*windows|job.*object|detached.*win" apps/desktop/src/main/lib/terminal-host/ -A 3 -B 3Repository: superset-sh/superset
Length of output: 671
🏁 Script executed:
# Check git history to see if this change was intentional
cd apps/desktop/src/main/lib/terminal-host && git log --oneline -20 client.ts 2>/dev/null || echo "Git history not available"Repository: superset-sh/superset
Length of output: 141
🏁 Script executed:
# Look for any Windows-specific terminal host comments or implementation notes
rg -i "windows" apps/desktop/src/main/lib/terminal-host/client.ts -B 2 -A 2Repository: superset-sh/superset
Length of output: 201
🏁 Script executed:
# Check if there's a daemon script and what platform checks it has
fd -name "daemonScript" apps/desktop/src/main/lib/terminal-host/ || fd -name "*daemon*" apps/desktop/src/main/lib/terminal-host/Repository: superset-sh/superset
Length of output: 525
Daemon will not survive parent exit on Windows with detached: false.
On Windows, Node.js uses a global job object with JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE semantics. Children spawned with detached: false (the current Windows behavior) are assigned to this job and automatically terminated when the parent process exits, regardless of child.unref(). Since the daemon is intended to outlive the Electron app (per the code comment and usage pattern), this regresses Windows compatibility. Set detached: true unconditionally to prevent the child from being assigned to the parent's kill-on-close job.
Suggested fix
- detached: process.platform !== "win32",
+ detached: true,📝 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
| child = spawn(process.execPath, [daemonScript], { | |
| detached: true, | |
| detached: process.platform !== "win32", | |
| stdio: logFd >= 0 ? ["ignore", logFd, logFd] : "ignore", | |
| env: { | |
| ...process.env, | |
| ELECTRON_RUN_AS_NODE: "1", | |
| NODE_ENV: process.env.NODE_ENV, | |
| }, | |
| ...(process.platform === "win32" ? { windowsHide: true } : {}), | |
| }); | |
| child = spawn(process.execPath, [daemonScript], { | |
| detached: true, | |
| stdio: logFd >= 0 ? ["ignore", logFd, logFd] : "ignore", | |
| env: { | |
| ...process.env, | |
| ELECTRON_RUN_AS_NODE: "1", | |
| NODE_ENV: process.env.NODE_ENV, | |
| }, | |
| ...(process.platform === "win32" ? { windowsHide: true } : {}), | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main/lib/terminal-host/client.ts` around lines 1106 - 1115,
The spawn invocation that creates the daemon currently sets detached to
process.platform !== "win32", causing Windows children to be tied to the
parent's job and killed on parent exit; change the detached option to true
unconditionally in the spawn call that creates child (the
spawn(process.execPath, [daemonScript], { ... }) block) so the daemon can
outlive the Electron process, keep the existing windowsHide handling for
Windows, and ensure the spawned child is unref'd afterwards (child.unref()) if
not already done.
Comment on lines
+130
to
+140
| // SIGHUP is not emitted on Windows | ||
| if (process.platform !== "win32") { | ||
| process.on("SIGHUP", () => { | ||
| shutdownOnce({ | ||
| exitCode: 0, | ||
| message: "Received SIGHUP, shutting down...", | ||
| stopServerErrorMessage: "Error during stopServer in SIGHUP shutdown", | ||
| timeoutMessage: "Forced exit after SIGHUP shutdown timeout", | ||
| }); | ||
| }); | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In the Node.js version used by this repository, is SIGHUP supported or emitted on Windows, and under what conditions?
💡 Result:
I can’t answer the “version used by this repository” part without the repo URL (or at least the contents of its .nvmrc, .node-version, package.json engines.node, or CI config). Share that and I’ll confirm against the exact Node version it pins.
In Node.js v20.x, SIGHUP is emitted on Windows, but it’s effectively emulated: Node’s docs state that 'SIGHUP' is generated on Windows when the console window is closed (e.g., closing the terminal window). [1] Under the hood, libuv documents the same behavior: on Windows, SIGHUP is generated when the user closes the console window. [2] That console-close action corresponds to Windows sending a CTRL_CLOSE_EVENT (“CTRL+CLOSE signal”) to console-attached processes. [3]
Conditions (Windows):
- Emitted: when the process is attached to a console and the console window is being closed (close button / Alt+F4 on the console window). [1][2][3]
- Not emitted: in scenarios where there is no console to close (e.g., many service/background/non-console contexts), because the “console window closed” condition doesn’t occur. (Repo-specific behavior depends on how it runs.)
Sources:
[1] Node.js v20.11.0 process docs (Signal events / SIGHUP on Windows) (nodejs.org)
[2] libuv signal handle docs (Windows SIGHUP when console window closes) (docs.libuv.org)
[3] Microsoft docs: CTRL+CLOSE signal generated when console is closed (learn.microsoft.com)
Citations:
- 1: https://nodejs.org/download/release/v20.11.0/docs/api/process.html?utm_source=openai
- 2: https://docs.libuv.org/en/v1.x/signal.html?utm_source=openai
- 3: https://learn.microsoft.com/en-us/windows/console/ctrl-close-signal?utm_source=openai
🏁 Script executed:
#!/bin/bash
# Check Node.js version configuration
for file in .nvmrc .node-version package.json .github/workflows/*.yml bun.lock; do
if [ -f "$file" ]; then
echo "=== $file ==="
if [ "$file" = "package.json" ]; then
jq -r '.engines.node // empty' "$file" 2>/dev/null || cat "$file" | grep -A2 '"engines"' || true
else
cat "$file" 2>/dev/null | head -20
fi
fi
doneRepository: superset-sh/superset
Length of output: 5209
🏁 Script executed:
#!/bin/bash
cat -n apps/desktop/src/main/terminal-host/signal-handlers.ts | head -150Repository: superset-sh/superset
Length of output: 5141
Remove the Windows platform gate—Node.js emits SIGHUP on Windows when the console closes.
The comment assumes SIGHUP is unavailable on Windows, but Node.js v20.x documentation confirms that SIGHUP is emitted when the console window closes (Windows maps this to CTRL_CLOSE_EVENT). Without this handler, closing the console window on Windows triggers an ungraceful shutdown, skipping stopServer() cleanup. Remove the if (process.platform !== "win32") gate so SIGHUP listeners work on all platforms.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main/terminal-host/signal-handlers.ts` around lines 130 -
140, Remove the platform guard around the SIGHUP handler so the listener is
registered on all OSes; specifically, delete the surrounding if
(process.platform !== "win32") check and ensure process.on("SIGHUP", ...)
remains and calls shutdownOnce with the same
exitCode/message/stopServerErrorMessage/timeoutMessage payload so stopServer()
cleanup runs when the console closes on Windows as well as other platforms.
Comment on lines
+14
to
+17
| // Check if there are any diagnostics (errors, warnings, or infos) | ||
| if (/Found \d+ (error|info|warning)/.test(output)) { | ||
| process.exit(1); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify biome's actual output format for diagnostics
bunx biome check --help 2>&1 | head -20
# Also check if there's documentation about output format
echo "---"
echo "Testing with a sample file to see actual output format..."Repository: superset-sh/superset
Length of output: 168
🏁 Script executed:
cat -n scripts/lint.tsRepository: superset-sh/superset
Length of output: 731
🏁 Script executed:
# Search for any test files or examples that might show Biome output
fd -e test -e spec | head -20Repository: superset-sh/superset
Length of output: 46
🌐 Web query:
Biome check output format error warning info messages documentation
💡 Result:
In Biome (biome check), “error / warning / info messages” are diagnostics, and the “output format” is controlled by the reporter you select.
Diagnostic severities (info / warn / error)
- Rules emit diagnostics with severity
info,warn, orerror.errordiagnostics make the command exit non‑zero;info/warndo not by default. You can force warnings to fail with--error-on-warnings. [1] - You can filter what severities are printed with
--diagnostic-level=<info|warn|error>(e.g.,--diagnostic-level=errorprints only errors). [2]
Output format (“reporter”) for biome check
biome checksupports--reporter=<default|json|json-pretty|github|junit|summary|gitlab|checkstyle|rdjson|sarif>(and--reporter-file=PATH). [3]- If you’re seeing an “output format” issue with JSON: Biome’s JSON reporter shape has historically been described as experimental / subject to change (so downstream parsers can break if they assume a fixed schema). [4]
“Log” messages vs diagnostics
If what you mean by “info/warn/error messages” are internal logs (not rule diagnostics), use:
--log-level=<none|debug|info|warn|error>and--log-kind=<pretty|compact|json>(optionally--log-file=...). [2]
Example commands
biome check . --reporter=json --diagnostic-level=warn
biome check . --reporter=sarif --reporter-file=biome.sarif
biome check . --log-level=debug --log-kind=jsonIf you paste the exact warning text (“output format error …”) and the command you ran, I can point to the specific reporter/schema expectation that’s failing.
Sources
[1] https://v1.biomejs.dev/linter/
[2] https://biomejs.dev/uk/reference/cli/
[3] https://biomejs.dev/uk/reference/cli/
[4] https://biomejs.dev/internals/changelog_v1/
🏁 Script executed:
rg "Found.*error|Found.*warning|Found.*info" -t ts -t js -t json --max-count=10Repository: superset-sh/superset
Length of output: 132
🏁 Script executed:
# Look for any Biome configuration or test files
fd -e "biome" | head -20Repository: superset-sh/superset
Length of output: 46
🌐 Web query:
Biome check CLI diagnostic summary message output format "Found N"
💡 Result:
Biome’s default (pretty) check/ci end-of-run diagnostic summary uses a standalone line in this shape:
Found <N> errors.(orFound <N> warnings./Found <N> info.) — note the trailing period.
Examples:Found 4 warnings.afterbiome checkoutput [1], andFound 4 errors.afterbiome cioutput [2].
It’s typically preceded by the timing/fix line:
Checked <M> file(s) in <TIME>. No fixes applied.[1][2]
Sources: [1] biomejs/biome#7363, [2] biomejs/biome#7838
Regex uses singular forms but Biome outputs plural forms — diagnostic detection may fail.
Biome outputs summary messages like "Found 4 errors." and "Found 4 warnings." (plural), but the regex /Found \d+ (error|info|warning)/ looks for singular forms. This mismatch prevents the script from detecting diagnostics when Biome runs.
🐛 Fix to match Biome's actual output format
// Check if there are any diagnostics (errors, warnings, or infos)
-if (/Found \d+ (error|info|warning)/.test(output)) {
+if (/Found \d+ (errors?|warnings?|infos?)/.test(output)) {
process.exit(1);
}📝 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
| // Check if there are any diagnostics (errors, warnings, or infos) | |
| if (/Found \d+ (error|info|warning)/.test(output)) { | |
| process.exit(1); | |
| } | |
| // Check if there are any diagnostics (errors, warnings, or infos) | |
| if (/Found \d+ (errors?|warnings?|infos?)/.test(output)) { | |
| process.exit(1); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/lint.ts` around lines 14 - 17, The diagnostic check using the regex
/Found \d+ (error|info|warning)/ fails for plural outputs like "Found 4 errors";
update the test that inspects the output variable so the regex matches plural
forms (e.g., allow trailing "s" for error/info/warning) or use a plural-aware
pattern such as matching (error|info|warning)s? so the if (/.../.test(output))
correctly detects Biome summaries and calls process.exit(1).
Comment on lines
+14
to
+27
| const nodePtyBase = join( | ||
| import.meta.dir, | ||
| "..", | ||
| "node_modules", | ||
| ".bun", | ||
| "node-pty@1.1.0", | ||
| "node_modules", | ||
| "node-pty", | ||
| ); | ||
|
|
||
| if (!existsSync(nodePtyBase)) { | ||
| console.log("node-pty not found, skipping patch"); | ||
| process.exit(0); | ||
| } |
Contributor
There was a problem hiding this comment.
Hardcoded node-pty version creates silent breakage risk on upgrade.
The path node-pty@1.1.0 is hardcoded here but the actual version is defined in apps/desktop/package.json. If the dependency is upgraded, this patch script will silently skip patching (due to the existsSync check) and the Windows build will fail later with cryptic gyp errors.
Consider either:
- Adding a warning log when the path isn't found to aid debugging
- Dynamically resolving the version from package.json
🛠️ Proposed fix to add diagnostic logging
if (!existsSync(nodePtyBase)) {
- console.log("node-pty not found, skipping patch");
+ console.warn(
+ "[patch-node-pty-win] node-pty@1.1.0 not found at expected path, skipping patch.",
+ "If node-pty version was updated, this script needs updating too.",
+ );
process.exit(0);
}📝 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
| const nodePtyBase = join( | |
| import.meta.dir, | |
| "..", | |
| "node_modules", | |
| ".bun", | |
| "node-pty@1.1.0", | |
| "node_modules", | |
| "node-pty", | |
| ); | |
| if (!existsSync(nodePtyBase)) { | |
| console.log("node-pty not found, skipping patch"); | |
| process.exit(0); | |
| } | |
| const nodePtyBase = join( | |
| import.meta.dir, | |
| "..", | |
| "node_modules", | |
| ".bun", | |
| "node-pty@1.1.0", | |
| "node_modules", | |
| "node-pty", | |
| ); | |
| if (!existsSync(nodePtyBase)) { | |
| console.warn( | |
| "[patch-node-pty-win] node-pty@1.1.0 not found at expected path, skipping patch.", | |
| "If node-pty version was updated, this script needs updating too.", | |
| ); | |
| process.exit(0); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/patch-node-pty-win.ts` around lines 14 - 27, The script currently
hardcodes "node-pty@1.1.0" into nodePtyBase and silently exits when
existsSync(nodePtyBase) is false; update the logic to either (a) read the
node-pty version from apps/desktop/package.json (parse
dependencies/devDependencies to build the path dynamically) and use that
computed directory for nodePtyBase, or (b) at minimum replace the silent
console.log + process.exit with a clear warning that includes the expected path
and a hint to check apps/desktop/package.json (i.e., change the existsSync
branch to processLogger.warn or console.warn with the missing path and exit
code), and ensure references to nodePtyBase and the existsSync check are updated
accordingly so the script fails fast with diagnostic output rather than silently
skipping the patch.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/main/lib/agent-setup/agent-wrappers-copilot.ts (1)
46-81:⚠️ Potential issue | 🟠 MajorMake
getCopilotHooksJsonContent()platform-aware to use correct hook key for Windows.The function currently generates hooks.json with only
bash:key for all platforms. However, the actual hook script path is platform-aware:copilot-hook.ps1on Windows andcopilot-hook.shon Unix/Linux.The Copilot CLI schema expects:
powershell:key for PowerShell scripts (Windows)bash:key for bash scripts (Unix/Linux)Currently, Windows will generate
bash: "path/to/copilot-hook.ps1", which Copilot CLI won't execute correctly. Add platform detection togetCopilotHooksJsonContent()to usepowershell:key when on Windows.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/lib/agent-setup/agent-wrappers-copilot.ts` around lines 46 - 81, Update getCopilotHooksJsonContent to choose the hook key based on platform: detect Windows (process.platform === "win32") and use the "powershell" key with the provided hookScriptPath, otherwise use "bash"; apply this conditional key for every hook entry (sessionStart, sessionEnd, userPromptSubmitted, postToolUse) so the generated JSON uses powershell: on Windows and bash: on Unix-like systems while keeping the same structure and timeoutSec values.
♻️ Duplicate comments (4)
apps/desktop/src/main/lib/terminal-host/client.ts (1)
1106-1115:⚠️ Potential issue | 🟠 MajorKeep the daemon detached on Windows as well.
Line 1107 breaks the long-lived daemon model on Windows: Node documents that a Windows child only gets to continue after the parent exits when
detachedistrue, whileunref()only stops the parent from waiting on it. With the current branch, the terminal host can still die with Electron on Windows. (nodejs.org)Suggested fix
try { child = spawn(process.execPath, [daemonScript], { - detached: process.platform !== "win32", + detached: true, stdio: logFd >= 0 ? ["ignore", logFd, logFd] : "ignore", env: { ...process.env, ELECTRON_RUN_AS_NODE: "1", NODE_ENV: process.env.NODE_ENV, }, ...(process.platform === "win32" ? { windowsHide: true } : {}), });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/lib/terminal-host/client.ts` around lines 1106 - 1115, The spawn call that creates the daemon (the child variable in apps/desktop/src/main/lib/terminal-host/client.ts) currently sets detached: process.platform !== "win32", which prevents the daemon from continuing after the parent exits on Windows; change that option to detached: true so the child is detached on all platforms (keep the existing windowsHide option and stdio/env handling as-is), and ensure you still call child.unref() if the code intends the parent not to wait on the child.apps/desktop/src/lib/trpc/routers/workspaces/utils/teardown.ts (1)
46-46:⚠️ Potential issue | 🟠 MajorDon't let
SHELLoverride Windows shell selection.Line 46 still allows an inherited Git Bash/MSYS
SHELLvalue to bypassgetDefaultShell()on Windows, so teardown can start under the wrong interpreter. Gate the env override to non-Windows only.Suggested fix
- const shell = process.env.SHELL || getDefaultShell(); + const shell = + process.platform === "win32" + ? getDefaultShell() + : process.env.SHELL || getDefaultShell();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/teardown.ts` at line 46, The current assignment lets an inherited SHELL override the selected shell on Windows; change the logic that sets the local variable `shell` in teardown.ts so that on Windows (process.platform === 'win32' or existing isWindows helper) it always uses getDefaultShell(), and only on non-Windows platforms allow using process.env.SHELL || getDefaultShell(); update the `shell` assignment accordingly (referencing the `shell` variable and the getDefaultShell() function)..github/workflows/build-desktop.yml (2)
197-203:⚠️ Potential issue | 🟠 MajorPass the same build-time env into the Windows compile step.
bun run compile:apponly gets Sentry vars here, while the macOS/Linux compile steps also inject the OAuth IDs andNEXT_PUBLIC_*endpoints. If those values are compiled into the renderer bundle, the Windows artifact will ship with missing auth and endpoint config.Proposed fix
- name: Compile app working-directory: apps/desktop run: bun run compile:app env: + NEXT_PUBLIC_POSTHOG_KEY: ${{ secrets.NEXT_PUBLIC_POSTHOG_KEY }} + NEXT_PUBLIC_POSTHOG_HOST: ${{ secrets.NEXT_PUBLIC_POSTHOG_HOST }} + NEXT_PUBLIC_OUTLIT_KEY: ${{ secrets.NEXT_PUBLIC_OUTLIT_KEY }} + GOOGLE_CLIENT_ID: ${{ secrets.GOOGLE_CLIENT_ID }} + GH_CLIENT_ID: ${{ secrets.GH_CLIENT_ID }} + NEXT_PUBLIC_WEB_URL: ${{ secrets.NEXT_PUBLIC_WEB_URL }} + NEXT_PUBLIC_API_URL: ${{ secrets.NEXT_PUBLIC_API_URL }} + NEXT_PUBLIC_DOCS_URL: ${{ secrets.NEXT_PUBLIC_DOCS_URL }} + NEXT_PUBLIC_STREAMS_URL: ${{ secrets.NEXT_PUBLIC_STREAMS_URL }} + NEXT_PUBLIC_ELECTRIC_URL: ${{ secrets.NEXT_PUBLIC_ELECTRIC_URL }} + NEXT_PUBLIC_ELECTRIC_PROXY_URL: ${{ secrets.NEXT_PUBLIC_ELECTRIC_PROXY_URL }} SENTRY_AUTH_TOKEN: ${{ secrets.SENTRY_AUTH_TOKEN }} SENTRY_DSN_DESKTOP: ${{ secrets.SENTRY_DSN_DESKTOP }} + SUPERSET_WORKSPACE_NAME: superset🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build-desktop.yml around lines 197 - 203, The Windows "Compile app" step (working-directory: apps/desktop, run: bun run compile:app) only injects SENTRY_* vars; update this step to export the same build-time environment variables used by the macOS/Linux compile steps — include the OAuth client IDs and all NEXT_PUBLIC_* endpoints in addition to SENTRY_AUTH_TOKEN and SENTRY_DSN_DESKTOP so the renderer bundle is compiled with the same auth and endpoint config as other platforms.
212-217:⚠️ Potential issue | 🟠 MajorHonor
inputs.electron_builder_configfor Windows packaging.Line 214 still ignores the workflow input that macOS and Linux pass through. A caller can select an alternate builder config and silently get a different Windows artifact from the same workflow run.
Proposed fix
- name: Build installer working-directory: apps/desktop - run: bun run build + run: bun run build -- --config ${{ inputs.electron_builder_config }} env: CSC_IDENTITY_AUTO_DISCOVERY: "false"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build-desktop.yml around lines 212 - 217, The Windows "Build installer" step currently runs "bun run build" without using the workflow input; update that step (name: "Build installer", working-directory: apps/desktop, run: "bun run build") to pass the workflow input inputs.electron_builder_config through the environment or CLI so Windows uses the same electron-builder config as macOS/Linux (e.g., set an env var like ELECTRON_BUILDER_CONFIG or append a --config arg populated from ${{ inputs.electron_builder_config }}).
🧹 Nitpick comments (1)
apps/desktop/src/main/lib/agent-setup/templates/gemini-hook.template.ps1 (1)
44-46: Consider adding a brief comment explaining the intentional empty catch block.The empty catch block is intentional (webhook failures should be silent to avoid blocking the agent), but PSScriptAnalyzer flags this as a warning. A short comment clarifies the intent for future maintainers.
📝 Proposed fix
try { Invoke-RestMethod -Uri $url -Method GET -TimeoutSec 2 -ErrorAction SilentlyContinue | Out-Null -} catch {} +} catch { + # Silently ignore webhook failures to avoid blocking the agent +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/lib/agent-setup/templates/gemini-hook.template.ps1` around lines 44 - 46, The empty catch after the Invoke-RestMethod call is intentional but needs documentation to silence reviewer warnings; update the try/catch around Invoke-RestMethod (the block calling $url) to include a short inline comment inside the catch (e.g. "Intentionally empty - webhook failures should be silent to avoid blocking the agent") and optionally add the PowerShell PSScriptAnalyzer suppression tag if you want to silence the analyzer (e.g. a brief suppression comment) so future maintainers understand this is deliberate.
🤖 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/electron-builder.ts`:
- Around line 201-202: The Windows config in electron-builder.ts currently sets
signAndEditExecutable: false and forceCodeSigning: false which disables signing
for all release builds; update the build configuration used by
electron-builder.ts and electron-builder.canary.ts (and the shared
baseConfig.win) to provide two variants — a signedWindowsConfig for
public/stable releases and an unsignedWindowsConfig for dev/canary — or gate the
signAndEditExecutable/forceCodeSigning flags by a release channel/environment
variable (e.g., process.env.RELEASE_CHANNEL) so that signAndEditExecutable and
forceCodeSigning are true for production releases and remain false for
dev/canary; ensure electron-builder.canary.ts does not inherit an unsigned-only
baseConfig.win for public release paths.
In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/teardown.ts`:
- Around line 117-123: The code currently calls treeKill(child.pid, "SIGKILL")
on Windows without waiting for completion, causing the teardown timeout to
reject before child processes are actually killed; update the logic in the
teardown routine that handles child termination (the block referencing child.pid
and treeKill) to use the callback-based API: call treeKill(child.pid, "SIGKILL",
(err) => { if (err) reject(errOrTimeout); else resolve()); } ) and only reject
the outer timeout after the callback reports failure; for the non-Windows branch
that uses process.kill(-child.pid, "SIGKILL") wrap it in try/catch and
resolve/reject the same promise path so both branches wait for termination
before the teardown promise settles.
In `@apps/desktop/src/main/lib/agent-setup/notify-hook.ts`:
- Around line 6-7: The Mastra wrapper currently builds the notify invocation as
`bash <path>` which breaks Windows PowerShell scripts; update
getMastraHooksJsonContent to choose the correct invocation based on
NOTIFY_SCRIPT_NAME/platform: if the selected script is a PowerShell script
(process.platform === "win32" or NOTIFY_SCRIPT_NAME endsWith ".ps1") build the
notifyCommand to invoke PowerShell (e.g. use powershell/pwsh with
-ExecutionPolicy Bypass -File and the quoted notifyScriptPath), otherwise keep
the current bash <quotedPath> (or simply the script path for non-shell scripts);
locate getMastraHooksJsonContent and the notifyCommand construction in
agent-wrappers-mastra.ts and replace the hardcoded `bash
${quoteShellPath(notifyScriptPath)}` with this conditional logic so Windows runs
PowerShell scripts correctly.
In `@apps/desktop/src/main/lib/terminal/env.test.ts`:
- Around line 549-559: Update the test "buildSafeEnv excludes secrets on
Windows" to use mixed-case secret environment variable names (e.g.,
"Database_Url" and "Neon_Api_Key" instead of all-caps) so it validates Windows'
case-insensitive filtering; locate the test for buildSafeEnv in env.test.ts and
change the secret keys in the env object to mixed-case, then assert that
buildSafeEnv(..., { platform: "win32" }) still removes those mixed-case keys
while preserving the "Path" value.
In `@README.md`:
- Around line 66-79: The README advertises Windows support but the prebuilt
download CTAs only point to macOS; update the prebuilt download links/CTAs (the
top "Prebuilt" CTA and the bottom download CTA referenced near the macOS link)
to include a Windows download path (e.g., add the .exe or Windows installer
alongside the macOS binary), update the download filenames/labels to show both
macOS and Windows targets, and ensure the "Windows Support" section's guidance
matches the actual download entries so Windows users can find the correct .exe
installer.
---
Outside diff comments:
In `@apps/desktop/src/main/lib/agent-setup/agent-wrappers-copilot.ts`:
- Around line 46-81: Update getCopilotHooksJsonContent to choose the hook key
based on platform: detect Windows (process.platform === "win32") and use the
"powershell" key with the provided hookScriptPath, otherwise use "bash"; apply
this conditional key for every hook entry (sessionStart, sessionEnd,
userPromptSubmitted, postToolUse) so the generated JSON uses powershell: on
Windows and bash: on Unix-like systems while keeping the same structure and
timeoutSec values.
---
Duplicate comments:
In @.github/workflows/build-desktop.yml:
- Around line 197-203: The Windows "Compile app" step (working-directory:
apps/desktop, run: bun run compile:app) only injects SENTRY_* vars; update this
step to export the same build-time environment variables used by the macOS/Linux
compile steps — include the OAuth client IDs and all NEXT_PUBLIC_* endpoints in
addition to SENTRY_AUTH_TOKEN and SENTRY_DSN_DESKTOP so the renderer bundle is
compiled with the same auth and endpoint config as other platforms.
- Around line 212-217: The Windows "Build installer" step currently runs "bun
run build" without using the workflow input; update that step (name: "Build
installer", working-directory: apps/desktop, run: "bun run build") to pass the
workflow input inputs.electron_builder_config through the environment or CLI so
Windows uses the same electron-builder config as macOS/Linux (e.g., set an env
var like ELECTRON_BUILDER_CONFIG or append a --config arg populated from ${{
inputs.electron_builder_config }}).
In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/teardown.ts`:
- Line 46: The current assignment lets an inherited SHELL override the selected
shell on Windows; change the logic that sets the local variable `shell` in
teardown.ts so that on Windows (process.platform === 'win32' or existing
isWindows helper) it always uses getDefaultShell(), and only on non-Windows
platforms allow using process.env.SHELL || getDefaultShell(); update the `shell`
assignment accordingly (referencing the `shell` variable and the
getDefaultShell() function).
In `@apps/desktop/src/main/lib/terminal-host/client.ts`:
- Around line 1106-1115: The spawn call that creates the daemon (the child
variable in apps/desktop/src/main/lib/terminal-host/client.ts) currently sets
detached: process.platform !== "win32", which prevents the daemon from
continuing after the parent exits on Windows; change that option to detached:
true so the child is detached on all platforms (keep the existing windowsHide
option and stdio/env handling as-is), and ensure you still call child.unref() if
the code intends the parent not to wait on the child.
---
Nitpick comments:
In `@apps/desktop/src/main/lib/agent-setup/templates/gemini-hook.template.ps1`:
- Around line 44-46: The empty catch after the Invoke-RestMethod call is
intentional but needs documentation to silence reviewer warnings; update the
try/catch around Invoke-RestMethod (the block calling $url) to include a short
inline comment inside the catch (e.g. "Intentionally empty - webhook failures
should be silent to avoid blocking the agent") and optionally add the PowerShell
PSScriptAnalyzer suppression tag if you want to silence the analyzer (e.g. a
brief suppression comment) so future maintainers understand this is deliberate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 35044ab8-7ad4-4c7a-a1e2-aa61a7e02808
📒 Files selected for processing (24)
.github/workflows/build-desktop.yml.github/workflows/ci.yml.github/workflows/release-desktop.ymlREADME.mdapps/desktop/electron-builder.tsapps/desktop/scripts/copy-native-modules.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/shell-env.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/teardown.tsapps/desktop/src/main/lib/agent-setup/agent-wrappers-copilot.tsapps/desktop/src/main/lib/agent-setup/agent-wrappers-gemini.tsapps/desktop/src/main/lib/agent-setup/notify-hook.tsapps/desktop/src/main/lib/agent-setup/shell-wrappers.tsapps/desktop/src/main/lib/agent-setup/templates/codex-wrapper-exec.template.ps1apps/desktop/src/main/lib/agent-setup/templates/copilot-hook.template.ps1apps/desktop/src/main/lib/agent-setup/templates/cursor-hook.template.ps1apps/desktop/src/main/lib/agent-setup/templates/gemini-hook.template.ps1apps/desktop/src/main/lib/agent-setup/templates/notify-hook.template.ps1apps/desktop/src/main/lib/terminal-host/client.tsapps/desktop/src/main/lib/terminal/env.test.tsapps/desktop/src/main/lib/terminal/env.tsapps/desktop/src/main/terminal-host/session.test.tsapps/desktop/src/main/terminal-host/signal-handlers.tsscripts/patch-node-pty-win.tsscripts/postinstall.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- apps/desktop/src/main/terminal-host/session.test.ts
- apps/desktop/src/main/lib/terminal/env.ts
- .github/workflows/ci.yml
- apps/desktop/scripts/copy-native-modules.ts
- scripts/patch-node-pty-win.ts
- scripts/postinstall.ts
- apps/desktop/src/lib/trpc/routers/workspaces/utils/shell-env.ts
Comment on lines
+201
to
+202
| signAndEditExecutable: false, | ||
| forceCodeSigning: false, |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C2 'electron_builder_config|signAndEditExecutable|forceCodeSigning|CSC_IDENTITY_AUTO_DISCOVERY|CSC_LINK|WIN_CSC' .github/workflows apps/desktopRepository: superset-sh/superset
Length of output: 3847
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if electron-builder.canary.ts exists and compare Windows configs
echo "=== Checking electron-builder.canary.ts ==="
if [ -f "apps/desktop/electron-builder.canary.ts" ]; then
sed -n '/windows:/,/^[[:space:]]*},$/p' apps/desktop/electron-builder.canary.ts | head -30
else
echo "electron-builder.canary.ts not found"
fi
echo -e "\n=== Checking for any Windows code signing config in electron-builder.ts ==="
sed -n '/windows:/,/^[[:space:]]*},$/p' apps/desktop/electron-builder.ts | grep -E 'sign|csc|CSC|certificate'
echo -e "\n=== Checking context around lines 201-202 in electron-builder.ts ==="
sed -n '195,210p' apps/desktop/electron-builder.tsRepository: superset-sh/superset
Length of output: 184
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# List electron-builder files
echo "=== Files in apps/desktop matching electron-builder ==="
ls -la apps/desktop/electron-builder* 2>/dev/null || echo "No electron-builder files found"
# Read the full Windows section from electron-builder.ts with more context
echo -e "\n=== Full Windows config in electron-builder.ts ==="
awk '/windows:/{flag=1} flag{print NR": "$0} /^[[:space:]]*},/{if(flag && prev_close) flag=0} {prev_close=/^[[:space:]]*},/}' apps/desktop/electron-builder.ts | head -50Repository: superset-sh/superset
Length of output: 342
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read electron-builder.ts to see the full context
echo "=== Reading electron-builder.ts ==="
cat -n apps/desktop/electron-builder.ts
echo -e "\n=== Reading electron-builder.canary.ts ==="
cat -n apps/desktop/electron-builder.canary.tsRepository: superset-sh/superset
Length of output: 9765
This disables signing for all release builds, not just development.
Both stable (release-desktop.yml) and canary (release-desktop-canary.yml) release workflows use unsigned Windows configurations:
- Stable release uses
electron-builder.tsdirectly, which has lines 201–202 disabling code signing - Canary release uses
electron-builder.canary.ts, which inherits the unsigned Windows config via...baseConfig.win
No alternate signed Windows builder config exists for the release paths. If code signing should be enabled for public releases, the Windows config must be split into signed and unsigned variants or gated by environment/channel.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/electron-builder.ts` around lines 201 - 202, The Windows config
in electron-builder.ts currently sets signAndEditExecutable: false and
forceCodeSigning: false which disables signing for all release builds; update
the build configuration used by electron-builder.ts and
electron-builder.canary.ts (and the shared baseConfig.win) to provide two
variants — a signedWindowsConfig for public/stable releases and an
unsignedWindowsConfig for dev/canary — or gate the
signAndEditExecutable/forceCodeSigning flags by a release channel/environment
variable (e.g., process.env.RELEASE_CHANNEL) so that signAndEditExecutable and
forceCodeSigning are true for production releases and remain false for
dev/canary; ensure electron-builder.canary.ts does not inherit an unsigned-only
baseConfig.win for public release paths.
Comment on lines
+117
to
+123
| if (child.pid) { | ||
| if (process.platform === "win32") { | ||
| // Windows: can't use negative PID for process group kill | ||
| treeKill(child.pid, "SIGKILL"); | ||
| } else { | ||
| process.kill(-child.pid, "SIGKILL"); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find and read the teardown.ts file
fd -p "teardown.ts" apps/desktop/src/lib/trpcRepository: superset-sh/superset
Length of output: 126
🏁 Script executed:
# Read the teardown.ts file to understand the full context
cat -n apps/desktop/src/lib/trpc/routers/workspaces/utils/teardown.tsRepository: superset-sh/superset
Length of output: 6152
🏁 Script executed:
# Check the tree-kill package to understand its API
fd -p "node_modules/tree-kill" --max-depth 1 && cat node_modules/tree-kill/package.json 2>/dev/null | head -20Repository: superset-sh/superset
Length of output: 46
🌐 Web query:
tree-kill npm package API callback signature
💡 Result:
tree-kill exports a single function with this API:
kill(pid[, signal][, callback])(npmjs.com)callbackis Node-style and is invoked ascallback(err)whereerris anError(orundefined/nullif no error). (npmjs.com)
Citations:
- 1: https://www.npmjs.com/package/tree-kill?utm_source=openai
- 2: https://www.npmjs.com/package/tree-kill?utm_source=openai
Wait for tree-kill to finish before rejecting the timeout.
On Windows, treeKill() is called without a callback at line 120, making it asynchronous. The code then immediately rejects at lines 126-128 without waiting for the tree-kill operation to complete. This causes the teardown promise to resolve while child processes are still alive and holding file locks, leading to intermittent cleanup failures.
Use the callback-based API and only reject after the kill completes:
Suggested fix
if (child.pid) {
if (process.platform === "win32") {
// Windows: can't use negative PID for process group kill
- treeKill(child.pid, "SIGKILL");
+ treeKill(child.pid, "SIGKILL", () => {
+ reject(
+ new Error(`Teardown timed out after ${TEARDOWN_TIMEOUT_MS}ms`),
+ );
+ });
+ return;
} else {
process.kill(-child.pid, "SIGKILL");
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/teardown.ts` around lines
117 - 123, The code currently calls treeKill(child.pid, "SIGKILL") on Windows
without waiting for completion, causing the teardown timeout to reject before
child processes are actually killed; update the logic in the teardown routine
that handles child termination (the block referencing child.pid and treeKill) to
use the callback-based API: call treeKill(child.pid, "SIGKILL", (err) => { if
(err) reject(errOrTimeout); else resolve()); } ) and only reject the outer
timeout after the callback reports failure; for the non-Windows branch that uses
process.kill(-child.pid, "SIGKILL") wrap it in try/catch and resolve/reject the
same promise path so both branches wait for termination before the teardown
promise settles.
Comment on lines
+6
to
+7
| export const NOTIFY_SCRIPT_NAME = | ||
| process.platform === "win32" ? "notify.ps1" : "notify.sh"; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how notify script is invoked across agent wrappers
rg -n "notifyScriptPath|NOTIFY_SCRIPT" apps/desktop/src/main/lib/agent-setup/*.ts | head -50Repository: superset-sh/superset
Length of output: 3531
🏁 Script executed:
cat -n apps/desktop/src/main/lib/agent-setup/notify-hook.tsRepository: superset-sh/superset
Length of output: 1941
🏁 Script executed:
sed -n '60,85p' apps/desktop/src/main/lib/agent-setup/agent-wrappers-mastra.tsRepository: superset-sh/superset
Length of output: 980
🏁 Script executed:
# Check if there's any platform detection or Windows-specific handling for Mastra
rg -n "win32|platform|Windows" apps/desktop/src/main/lib/agent-setup/agent-wrappers-mastra.tsRepository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Check where Mastra wrapper is integrated
rg -n "getMastraHooksJsonContent|setupMastraHooks" apps/desktop/src/main/lib/agent-setup/*.tsRepository: superset-sh/superset
Length of output: 769
🏁 Script executed:
# Check if there's Windows-specific agent setup logic
rg -n "platform.*win32|win32.*platform" apps/desktop/src/main/lib/agent-setup/*.tsRepository: superset-sh/superset
Length of output: 1220
🏁 Script executed:
# Check agent setup initialization to see if Mastra is conditionally enabled
head -50 apps/desktop/src/main/lib/agent-setup/index.tsRepository: superset-sh/superset
Length of output: 1254
🏁 Script executed:
# Check the full setupAgentHooks to see if Mastra creation is conditional
sed -n '36,100p' apps/desktop/src/main/lib/agent-setup/index.tsRepository: superset-sh/superset
Length of output: 1105
🏁 Script executed:
# Check how agent-wrappers.ts uses getMastraHooksJsonContent
sed -n '55,100p' apps/desktop/src/main/lib/agent-setup/agent-wrappers.tsRepository: superset-sh/superset
Length of output: 359
🏁 Script executed:
# Check if Droid and other wrappers have similar bash hardcoding
sed -n '50,80p' apps/desktop/src/main/lib/agent-setup/agent-wrappers-droid.tsRepository: superset-sh/superset
Length of output: 828
🏁 Script executed:
# Check the full getMastraHooksJsonContent function to understand the scope
sed -n '55,100p' apps/desktop/src/main/lib/agent-setup/agent-wrappers-mastra.tsRepository: superset-sh/superset
Length of output: 1649
🏁 Script executed:
# Check if Gemini and Copilot wrappers have similar bash hardcoding issues
rg -n "bash.*quoteShellPath|notifyCommand.*bash" apps/desktop/src/main/lib/agent-setup/agent-wrappers-*.tsRepository: superset-sh/superset
Length of output: 196
🏁 Script executed:
# Check the Droid wrapper to see how it handles the notify script
sed -n '120,145p' apps/desktop/src/main/lib/agent-setup/agent-wrappers-droid.tsRepository: superset-sh/superset
Length of output: 613
Mastra wrapper hardcodes bash invocation, incompatible with Windows PowerShell scripts.
While notify-hook.ts correctly implements platform-aware script selection (notify.ps1 on Windows, notify.sh otherwise), the Mastra wrapper at agent-wrappers-mastra.ts:69 hardcodes bash invocation:
const notifyCommand = `bash ${quoteShellPath(notifyScriptPath)}`;On Windows, this produces bash /path/to/notify.ps1, which fails because bash cannot execute PowerShell scripts. The Droid wrapper correctly passes the script path directly without a shell prefix, which is the proper approach for platform-aware scripts. Update getMastraHooksJsonContent to conditionally invoke powershell on Windows or pass the script path directly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main/lib/agent-setup/notify-hook.ts` around lines 6 - 7, The
Mastra wrapper currently builds the notify invocation as `bash <path>` which
breaks Windows PowerShell scripts; update getMastraHooksJsonContent to choose
the correct invocation based on NOTIFY_SCRIPT_NAME/platform: if the selected
script is a PowerShell script (process.platform === "win32" or
NOTIFY_SCRIPT_NAME endsWith ".ps1") build the notifyCommand to invoke PowerShell
(e.g. use powershell/pwsh with -ExecutionPolicy Bypass -File and the quoted
notifyScriptPath), otherwise keep the current bash <quotedPath> (or simply the
script path for non-shell scripts); locate getMastraHooksJsonContent and the
notifyCommand construction in agent-wrappers-mastra.ts and replace the hardcoded
`bash ${quoteShellPath(notifyScriptPath)}` with this conditional logic so
Windows runs PowerShell scripts correctly.
Comment on lines
+549
to
+559
| it("buildSafeEnv excludes secrets on Windows", () => { | ||
| const env = { | ||
| Path: "C:\\Windows", | ||
| DATABASE_URL: "postgres://secret", | ||
| NEON_API_KEY: "secret-key", | ||
| }; | ||
| const safe = buildSafeEnv(env, { platform: "win32" }); | ||
| expect(safe.Path).toBe("C:\\Windows"); | ||
| expect(safe.DATABASE_URL).toBeUndefined(); | ||
| expect(safe.NEON_API_KEY).toBeUndefined(); | ||
| }); |
Contributor
There was a problem hiding this comment.
Use mixed-case secret keys to exercise the Windows-specific path.
This test only uses canonical uppercase names, so it would still pass if buildSafeEnv() leaked Database_Url or Neon_Api_Key on win32. Using mixed-case keys here would actually validate the case-insensitive filtering that Windows requires.
Suggested test adjustment
it("buildSafeEnv excludes secrets on Windows", () => {
const env = {
Path: "C:\\Windows",
- DATABASE_URL: "postgres://secret",
- NEON_API_KEY: "secret-key",
+ Database_Url: "postgres://secret",
+ Neon_Api_Key: "secret-key",
};
const safe = buildSafeEnv(env, { platform: "win32" });
expect(safe.Path).toBe("C:\\Windows");
- expect(safe.DATABASE_URL).toBeUndefined();
- expect(safe.NEON_API_KEY).toBeUndefined();
+ expect(safe.Database_Url).toBeUndefined();
+ expect(safe.Neon_Api_Key).toBeUndefined();
});📝 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
| it("buildSafeEnv excludes secrets on Windows", () => { | |
| const env = { | |
| Path: "C:\\Windows", | |
| DATABASE_URL: "postgres://secret", | |
| NEON_API_KEY: "secret-key", | |
| }; | |
| const safe = buildSafeEnv(env, { platform: "win32" }); | |
| expect(safe.Path).toBe("C:\\Windows"); | |
| expect(safe.DATABASE_URL).toBeUndefined(); | |
| expect(safe.NEON_API_KEY).toBeUndefined(); | |
| }); | |
| it("buildSafeEnv excludes secrets on Windows", () => { | |
| const env = { | |
| Path: "C:\\Windows", | |
| Database_Url: "postgres://secret", | |
| Neon_Api_Key: "secret-key", | |
| }; | |
| const safe = buildSafeEnv(env, { platform: "win32" }); | |
| expect(safe.Path).toBe("C:\\Windows"); | |
| expect(safe.Database_Url).toBeUndefined(); | |
| expect(safe.Neon_Api_Key).toBeUndefined(); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main/lib/terminal/env.test.ts` around lines 549 - 559,
Update the test "buildSafeEnv excludes secrets on Windows" to use mixed-case
secret environment variable names (e.g., "Database_Url" and "Neon_Api_Key"
instead of all-caps) so it validates Windows' case-insensitive filtering; locate
the test for buildSafeEnv in env.test.ts and change the secret keys in the env
object to mixed-case, then assert that buildSafeEnv(..., { platform: "win32" })
still removes those mixed-case keys while preserving the "Path" value.
Comment on lines
+66
to
+79
| | **OS** | macOS, Windows 10+ (Linux untested) | | ||
| | **Runtime** | [Bun](https://bun.sh/) v1.0+ | | ||
| | **Version Control** | Git 2.20+ | | ||
| | **GitHub CLI** | [gh](https://cli.github.com/) | | ||
| | **Caddy** | [caddy](https://caddyserver.com/docs/install) (for dev server) | | ||
|
|
||
| ### Windows Support | ||
|
|
||
| | Requirement | Details | | ||
| |---|---| | ||
| | OS | Windows 10 1809+ (ConPTY support required) | | ||
| | Developer Mode | Enable in Settings > Privacy & Security > For developers | | ||
| | Git long paths | `git config --global core.longpaths true` | | ||
| | Build Tools | Visual Studio Build Tools with "Desktop development with C++" (for native modules) | |
Contributor
There was a problem hiding this comment.
Add a Windows download path alongside this support claim.
Line 66 now advertises Windows support, but the prebuilt CTAs at Line 15 and Line 85 still point to macOS only. Windows users will end up on a README that says they're supported without showing which .exe to download.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 66 - 79, The README advertises Windows support but
the prebuilt download CTAs only point to macOS; update the prebuilt download
links/CTAs (the top "Prebuilt" CTA and the bottom download CTA referenced near
the macOS link) to include a Windows download path (e.g., add the .exe or
Windows installer alongside the macOS binary), update the download
filenames/labels to show both macOS and Windows targets, and ensure the "Windows
Support" section's guidance matches the actual download entries so Windows users
can find the correct .exe installer.
This was referenced Mar 13, 2026
|
Still no windows supprt... |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Full Windows native support for the Superset desktop application. The app builds, installs, and runs on Windows x64 with feature parity to the macOS version — including terminal sessions, agent hooks, workspace management, and OAuth sign-in.
Phase 1: Cross-Platform Foundation
/bin/rm→fs.rm(),process.env.HOME→os.homedir())getDefaultShell()instead of hardcoded Unix pathspostinstall.tsandlint.tsbuild scripts to replace.shequivalentspackage.jsonto use.tsscripts viabun runPhase 2: Shell Layer & Environment
pwsh.exe) preference ingetDefaultShell()with automatic detectionshell-envon Windows —process.envalready contains the full user environment (unlike macOS GUI apps launched from Finder/Dock)cmd.exeinit script writers for agent binary PATH injectiongetWindowsShellArgs()andgetCommandShellArgs()for PowerShell/cmd argument formattingZDOTDIR/BASH_ENVneeded)detachedspawn guard +tree-killfor Windows process tree terminationPhase 3: Process Management & Agent Hooks
SIGHUPhandler (not emitted on Windows)windowsHidein terminal-host client.ps1vs.shtemplates by platformPhase 4: CI/CD & Testing
build-desktop.ymlrelease-desktop.ymlci.ymlFALLBACK_SHELL)buildSafeEnvtestsPhase 5: Build Fixes
node-ptywinpty gyp: hardcode commit hash/version to avoid.batfile execution failures in gyp context, remove Spectre mitigation requirement (not installed by default in VS Build Tools)copy-native-modules.ts: usermSyncwithrecursive+forcefor Windows junctions (Bun creates junctions instead of symlinks)teardown.ts: use static import fortree-killinstead ofawait import()inside non-async callbackpatch-node-pty-win.ts: automated postinstall script that patches node-pty gyp files on WindowsFiles Changed
Build & Scripts (5 files)
package.json— cross-platform script entriesscripts/postinstall.ts— platform-aware postinstallscripts/lint.ts— cross-platform lint runnerscripts/patch-node-pty-win.ts— node-pty Windows gyp patcherapps/desktop/electron-builder.ts— disable code signing for devShell & Environment (4 files)
apps/desktop/src/main/lib/terminal/env.ts—getDefaultShell()with pwsh detection, Windows localeapps/desktop/src/main/lib/agent-setup/shell-wrappers.ts— PowerShell/cmd wrappers, shell argsapps/desktop/src/lib/trpc/routers/workspaces/utils/shell-env.ts— short-circuit on Windowsapps/desktop/src/lib/trpc/routers/workspaces/utils/teardown.ts— tree-kill, detached guardProcess Management (4 files)
apps/desktop/src/main/terminal-host/signal-handlers.ts— SIGHUP guardapps/desktop/src/main/terminal-host/session.ts— shell selection fixapps/desktop/src/main/terminal-host/terminal-host.ts— platform detectionapps/desktop/src/main/lib/terminal-host/client.ts— detached spawn fixAgent Hooks (8 files)
apps/desktop/src/main/lib/agent-setup/agent-wrappers-copilot.ts— platform template selectionapps/desktop/src/main/lib/agent-setup/agent-wrappers-gemini.ts— platform template selectionapps/desktop/src/main/lib/agent-setup/notify-hook.ts— platform template selection.template.ps1files — PowerShell equivalents of all agent hook scriptsGit & Native Modules (2 files)
apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts—fs.rm()instead of/bin/rmapps/desktop/scripts/copy-native-modules.ts— junction-safe cleanupCI/CD (3 files)
.github/workflows/build-desktop.yml— Windows build job.github/workflows/release-desktop.yml— Windows artifacts.github/workflows/ci.yml— Windows test runnerTests (2 files)
apps/desktop/src/main/lib/terminal/env.test.ts— Windows-specific test casesapps/desktop/src/main/terminal-host/session.test.ts— platform-conditional expectationsTest Plan
electron-vite build+electron-builder)app://protocol for ES module loading)\rfor ConPTY Enter)platform === "win32"guarded)Summary by cubic
Adds native Windows support to the Superset desktop app. The app builds, installs, and runs on Windows x64 with feature parity to macOS (terminal, agent hooks, workspaces, OAuth), plus Windows CI/release and docs.
New Features
Build & CI
Written for commit 93a5ba7. Summary will update on new commits.
Summary by CodeRabbit
New Features
Documentation
Improvements