Refactor open service onto Effect-backed process spawning#1603
Open
juliusmarminge wants to merge 13 commits intomainfrom
Open
Refactor open service onto Effect-backed process spawning#1603juliusmarminge wants to merge 13 commits intomainfrom
juliusmarminge wants to merge 13 commits intomainfrom
Conversation
- Add external opener support alongside browser/editor launches - Remove the `open` dependency and cover platform behavior in tests
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
github-actions
bot
added
size:XL
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Unused
runtimeparameter threaded through plan helpers- Removed the unused
runtime: OpenRuntimeparameter frommakeLaunchPlanand all seven caller functions that only forwarded it.
- Removed the unused
Or push these changes by commenting:
@cursor push 7ec01e28bd
Preview (7ec01e28bd)
diff --git a/apps/server/src/open.ts b/apps/server/src/open.ts
--- a/apps/server/src/open.ts
+++ b/apps/server/src/open.ts
@@ -185,7 +185,6 @@
}
function makeLaunchPlan(
- runtime: OpenRuntime,
command: string,
args: ReadonlyArray<string>,
options: {
@@ -207,7 +206,7 @@
};
}
-function makeDarwinDefaultPlan(input: OpenExternalInput, runtime: OpenRuntime): LaunchPlan {
+function makeDarwinDefaultPlan(input: OpenExternalInput): LaunchPlan {
const args: string[] = [];
const wait = input.wait ?? false;
@@ -216,7 +215,7 @@
if (input.newInstance) args.push("--new");
args.push(input.target);
- return makeLaunchPlan(runtime, "open", args, {
+ return makeLaunchPlan("open", args, {
wait,
allowNonzeroExitCode: input.allowNonzeroExitCode ?? false,
shell: false,
@@ -226,7 +225,6 @@
function makeDarwinApplicationPlan(
input: OpenExternalInput,
app: { readonly name: string; readonly arguments: ReadonlyArray<string> },
- runtime: OpenRuntime,
): LaunchPlan {
const args: string[] = [];
const wait = input.wait ?? false;
@@ -240,18 +238,14 @@
args.push("--args", ...app.arguments);
}
- return makeLaunchPlan(runtime, "open", args, {
+ return makeLaunchPlan("open", args, {
wait,
allowNonzeroExitCode: input.allowNonzeroExitCode ?? false,
shell: false,
});
}
-function makePowerShellPlan(
- input: OpenExternalInput,
- runtime: OpenRuntime,
- powerShellCommand: string,
-): LaunchPlan {
+function makePowerShellPlan(input: OpenExternalInput, powerShellCommand: string): LaunchPlan {
const encodedParts = ["Start"];
const wait = input.wait ?? false;
@@ -259,7 +253,6 @@
encodedParts.push(quotePowerShellValue(input.target));
return makeLaunchPlan(
- runtime,
powerShellCommand,
[
"-NoProfile",
@@ -277,9 +270,9 @@
);
}
-function makeLinuxDefaultPlan(input: OpenExternalInput, runtime: OpenRuntime): LaunchPlan {
+function makeLinuxDefaultPlan(input: OpenExternalInput): LaunchPlan {
const wait = input.wait ?? false;
- return makeLaunchPlan(runtime, "xdg-open", [input.target], {
+ return makeLaunchPlan("xdg-open", [input.target], {
wait,
allowNonzeroExitCode: input.allowNonzeroExitCode ?? false,
detached: !wait,
@@ -288,8 +281,8 @@
});
}
-function makeWindowsExplorerPlan(input: OpenExternalInput, runtime: OpenRuntime): LaunchPlan {
- return makeLaunchPlan(runtime, "explorer", [input.target], {
+function makeWindowsExplorerPlan(input: OpenExternalInput): LaunchPlan {
+ return makeLaunchPlan("explorer", [input.target], {
wait: false,
allowNonzeroExitCode: false,
shell: false,
@@ -299,9 +292,8 @@
function makeDirectApplicationPlan(
input: OpenExternalInput,
app: { readonly name: string; readonly arguments: ReadonlyArray<string> },
- runtime: OpenRuntime,
): LaunchPlan {
- return makeLaunchPlan(runtime, app.name, [...app.arguments, input.target], {
+ return makeLaunchPlan(app.name, [...app.arguments, input.target], {
wait: input.wait ?? false,
allowNonzeroExitCode: input.allowNonzeroExitCode ?? false,
shell: false,
@@ -319,32 +311,32 @@
for (const app of appCandidates) {
if (app) {
if (runtime.platform === "darwin") {
- plans.push(makeDarwinApplicationPlan(input, app, runtime));
+ plans.push(makeDarwinApplicationPlan(input, app));
} else {
- plans.push(makeDirectApplicationPlan(input, app, runtime));
+ plans.push(makeDirectApplicationPlan(input, app));
}
continue;
}
if (runtime.platform === "darwin") {
- plans.push(makeDarwinDefaultPlan(input, runtime));
+ plans.push(makeDarwinDefaultPlan(input));
continue;
}
if (runtime.platform === "win32" || preferWindowsOpenerOnWsl) {
for (const powerShellCommand of runtime.powerShellCandidates) {
- plans.push(makePowerShellPlan(input, runtime, powerShellCommand));
+ plans.push(makePowerShellPlan(input, powerShellCommand));
}
}
if (runtime.platform === "win32") {
if (!(input.wait ?? false)) {
- plans.push(makeWindowsExplorerPlan(input, runtime));
+ plans.push(makeWindowsExplorerPlan(input));
}
continue;
}
- plans.push(makeLinuxDefaultPlan(input, runtime));
+ plans.push(makeLinuxDefaultPlan(input));
}
return plans;
@@ -654,7 +646,6 @@
return runFirstAvailablePlan(
[
makeLaunchPlan(
- runtime,
editor.command,
shouldUseGotoFlag(editor, input.cwd) ? ["--goto", input.cwd] : [input.cwd],
{You can send follow-ups to this agent here.
- Move browser/editor launch logic out of `open.ts` - Wire server startup and WebSocket tests to `DesktopLauncher` - Add validation and fallback error coverage for launch attempts
github-actions
bot
added
size:XXL
Co-authored-by: codex <codex@users.noreply.github.com>
- Preserve app launch arguments while passing the target URL - Run non-waited direct launches detached with ignored stdio
Contributor
|
You have used all Bugbot PR reviews included in your free trial for your GitHub account on this workspace. To continue using Bugbot reviews, enable Bugbot for your team in the Cursor dashboard. |
- Map spawn failures before nonzero-exit checks in desktop launcher - Add regression coverage for waited direct app launches on Linux
Contributor
|
You have used all Bugbot PR reviews included in your free trial for your GitHub account on this workspace. To continue using Bugbot reviews, enable Bugbot for your team in the Cursor dashboard. |
Member
Author
|
bugbot run |
- Extract shared Windows command/process utilities - Add process tree kill and probe process wrappers - Cover output truncation and Windows command behavior with tests
- Treat container env detection consistently when resolving desktop launch options - Fall back to direct kill if `taskkill` exits nonzero or cannot spawn - Add coverage for the WSL container and taskkill fallback cases
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Autofix Details
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Detached launches skip fallback failure detection
- Added a 500ms grace period after spawn to detect immediate non-zero exits, allowing runFirstAvailablePlan to fall through to subsequent strategies instead of treating all spawned processes as successful.
- ✅ Fixed: Empty app list disables all launch plans
- Changed normalizeAppCandidates to return [undefined] (the platform-default branch) when the resolved candidate list is empty, so an empty app array behaves the same as omitting app entirely.
Or push these changes by commenting:
@cursor push b8c3b02fdf
Preview (b8c3b02fdf)
diff --git a/apps/server/src/process/Layers/DesktopLauncher.ts b/apps/server/src/process/Layers/DesktopLauncher.ts
--- a/apps/server/src/process/Layers/DesktopLauncher.ts
+++ b/apps/server/src/process/Layers/DesktopLauncher.ts
@@ -206,7 +206,7 @@
}
}
- return candidates;
+ return candidates.length > 0 ? candidates : [undefined];
}
function isUriLikeTarget(target: string): boolean {
@@ -479,6 +479,8 @@
* not expose `unref()`, which means it cannot provide true fire-and-forget
* behavior for editor / file-manager launches.
*/
+const DETACHED_SPAWN_GRACE_MS = 500;
+
function defaultSpawnDetached(
input: DetachedSpawnInput,
context: LaunchContext,
@@ -496,22 +498,50 @@
}).pipe(
Effect.flatMap((childProcess) =>
Effect.callback<void, DesktopLauncherSpawnError>((resume) => {
+ let graceTimer: ReturnType<typeof setTimeout> | undefined;
+
+ const onEarlyExit = (code: number | null) => {
+ if (graceTimer !== undefined) clearTimeout(graceTimer);
+ if (code !== null && code !== 0) {
+ resume(
+ Effect.fail(
+ makeSpawnError(
+ context,
+ input.command,
+ input.args,
+ new Error(`Process exited immediately with code ${code}`),
+ ),
+ ),
+ );
+ } else {
+ childProcess.unref();
+ resume(Effect.void);
+ }
+ };
+
const onError = (error: Error) => {
childProcess.off("spawn", onSpawn);
resume(Effect.fail(makeSpawnError(context, input.command, input.args, error)));
};
+
const onSpawn = () => {
childProcess.off("error", onError);
- childProcess.unref();
- resume(Effect.void);
+ childProcess.once("exit", onEarlyExit);
+ graceTimer = setTimeout(() => {
+ childProcess.off("exit", onEarlyExit);
+ childProcess.unref();
+ resume(Effect.void);
+ }, DETACHED_SPAWN_GRACE_MS);
};
childProcess.once("error", onError);
childProcess.once("spawn", onSpawn);
return Effect.sync(() => {
+ if (graceTimer !== undefined) clearTimeout(graceTimer);
childProcess.off("error", onError);
childProcess.off("spawn", onSpawn);
+ childProcess.off("exit", onEarlyExit);
});
}),
),You can send follow-ups to this agent here.
Member
Author
…mpty app lists - defaultSpawnDetached now waits a grace period after spawn to catch processes that exit non-zero immediately, allowing runFirstAvailablePlan to fall through to the next strategy instead of silently succeeding. - normalizeAppCandidates returns [undefined] (the platform-default branch) when the resolved candidate list is empty, so an empty app array behaves the same as omitting app entirely. Applied via @cursor push command
- Prefer PowerShell for detached Windows batch shim launches - Preserve literal args in the encoded Start-Process command - Update launcher tests for the new spawn path
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Autofix Details
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Waited plan drops
windowsVerbatimArgumentsfor cmd wrappers- The
spawnPlanfunction now passesinput.windowsVerbatimArgumentsthrough toChildProcess.makeoptions instead of discarding it, and the test harness was updated to conditionally include the flag only when defined.
- The
- ✅ Fixed: Exported
DesktopLauncherErrorschema is never used- Removed the unused
DesktopLauncherErrorunion schema export fromServices/DesktopLauncher.tssince it had no imports or references anywhere in the codebase.
- Removed the unused
Or push these changes by commenting:
@cursor push 3792dcb889
Preview (3792dcb889)
diff --git a/apps/server/src/process/Layers/DesktopLauncher.test.ts b/apps/server/src/process/Layers/DesktopLauncher.test.ts
--- a/apps/server/src/process/Layers/DesktopLauncher.test.ts
+++ b/apps/server/src/process/Layers/DesktopLauncher.test.ts
@@ -67,6 +67,7 @@
readonly options: {
readonly detached?: boolean;
readonly shell?: boolean | string;
+ readonly windowsVerbatimArguments?: boolean;
readonly stdin?: unknown;
readonly stdout?: unknown;
readonly stderr?: unknown;
@@ -78,6 +79,9 @@
args: [...standardCommand.args],
detached: standardCommand.options.detached,
shell: standardCommand.options.shell,
+ ...(standardCommand.options.windowsVerbatimArguments !== undefined
+ ? { windowsVerbatimArguments: standardCommand.options.windowsVerbatimArguments }
+ : {}),
stdin: standardCommand.options.stdin,
stdout: standardCommand.options.stdout,
stderr: standardCommand.options.stderr,
@@ -153,7 +157,9 @@
args: [...input.args],
detached: input.detached,
shell: input.shell,
- windowsVerbatimArguments: input.windowsVerbatimArguments,
+ ...(input.windowsVerbatimArguments !== undefined
+ ? { windowsVerbatimArguments: input.windowsVerbatimArguments }
+ : {}),
stdin: input.stdin,
stdout: input.stdout,
stderr: input.stderr,
diff --git a/apps/server/src/process/Layers/DesktopLauncher.ts b/apps/server/src/process/Layers/DesktopLauncher.ts
--- a/apps/server/src/process/Layers/DesktopLauncher.ts
+++ b/apps/server/src/process/Layers/DesktopLauncher.ts
@@ -749,6 +749,9 @@
ChildProcess.make(input.command, [...input.args], {
detached: plan.detached,
shell: plan.shell,
+ ...(input.windowsVerbatimArguments !== undefined
+ ? { windowsVerbatimArguments: input.windowsVerbatimArguments }
+ : {}),
...(plan.stdio === "ignore"
? {
stdin: "ignore",
diff --git a/apps/server/src/process/Services/DesktopLauncher.ts b/apps/server/src/process/Services/DesktopLauncher.ts
--- a/apps/server/src/process/Services/DesktopLauncher.ts
+++ b/apps/server/src/process/Services/DesktopLauncher.ts
@@ -127,16 +127,6 @@
}
}
-export const DesktopLauncherError = Schema.Union([
- DesktopLauncherCommandNotFoundError,
- DesktopLauncherDiscoveryError,
- DesktopLauncherLaunchAttemptsExhaustedError,
- DesktopLauncherNonZeroExitError,
- DesktopLauncherSpawnError,
- DesktopLauncherUnknownEditorError,
- DesktopLauncherValidationError,
-]);
-
export interface OpenInEditorInput {
readonly cwd: string;
readonly editor: EditorId;You can send follow-ups to this agent here.
Comment on lines
+578
to
+583
| return Effect.sync(() => { | ||
| if (graceTimer !== undefined) clearTimeout(graceTimer); | ||
| childProcess.off("error", onError); | ||
| childProcess.off("spawn", onSpawn); | ||
| childProcess.off("exit", onEarlyExit); | ||
| }); |
Contributor
There was a problem hiding this comment.
🟡 Medium Layers/DesktopLauncher.ts:578
If the effect is interrupted after spawn but before the grace timer fires or early exit occurs, the cleanup function removes listeners but never calls childProcess.unref(). The parent Node.js process then remains blocked waiting for the child process to exit, defeating the fire-and-forget behavior intended for detached launches.
return Effect.sync(() => {
if (graceTimer !== undefined) clearTimeout(graceTimer);
childProcess.off("error", onError);
childProcess.off("spawn", onSpawn);
childProcess.off("exit", onEarlyExit);
+ childProcess.unref();
});🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/server/src/process/Layers/DesktopLauncher.ts around lines 578-583:
If the effect is interrupted after spawn but before the grace timer fires or early exit occurs, the cleanup function removes listeners but never calls `childProcess.unref()`. The parent Node.js process then remains blocked waiting for the child process to exit, defeating the fire-and-forget behavior intended for detached launches.
Evidence trail:
apps/server/src/process/Layers/DesktopLauncher.ts lines 510-515 (comment explaining unref() requirement), lines 554-556 (unref in onEarlyExit), lines 569-572 (unref in grace timer), lines 578-583 (cleanup function without unref call). git_grep results confirm unref() is only called at lines 555 and 570, not in the cleanup function.
| ? runWaitedPlan(plan, resolvedCommand, context) | ||
| : runDetachedPlan(plan, resolvedCommand, context); | ||
|
|
||
| const runFirstAvailablePlan = Effect.fn("runFirstAvailablePlan")(function* ( |
Contributor
There was a problem hiding this comment.
🟡 Medium Layers/DesktopLauncher.ts:820
On line 838, attempt.failure is accessed but Effect.result returns an Exit type where failures are stored in cause, not failure. This causes undefined to be pushed into the failures array, which then crashes in toLaunchAttemptFailure when accessing error._tag. Consider using Exit.match or checking attempt._tag === 'Failure' and accessing attempt.cause instead.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/server/src/process/Layers/DesktopLauncher.ts around line 820:
On line 838, `attempt.failure` is accessed but `Effect.result` returns an `Exit` type where failures are stored in `cause`, not `failure`. This causes `undefined` to be pushed into the `failures` array, which then crashes in `toLaunchAttemptFailure` when accessing `error._tag`. Consider using `Exit.match` or checking `attempt._tag === 'Failure'` and accessing `attempt.cause` instead.
Evidence trail:
1. apps/server/src/process/Layers/DesktopLauncher.ts lines 833-838 (REVIEWED_COMMIT): Shows `const attempt = yield* Effect.result(runPlan(...))` and `failures.push(attempt.failure)`
2. Effect-TS Exit type definition: https://github.com/Effect-TS/effect/blob/main/packages/effect/src/Exit.ts lines 33-43 - `Failure` interface has `cause: Cause.Cause<E>`, NOT `failure`
3. apps/server/src/process/Layers/DesktopLauncher.ts lines 484-485 (REVIEWED_COMMIT): `toLaunchAttemptFailure(error: LaunchAttemptError)` with `switch (error._tag)` - crashes when error is undefined
kkorenn
added a commit
to kkorenn/k1code
that referenced
this pull request
Apr 2, 2026
- Launch `explorer.exe` with a `vscode://file/...` target for Windows editor opens - Improve Windows spawn handling with `windowsHide` and protocol URI coverage
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
There are 4 total unresolved issues (including 2 from previous reviews).
Autofix Details
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Debug test files accidentally committed to repository
- Deleted both spawned2.txt and tmp-test.cmd, which were debug/test artifacts from manual process spawning tests.
- ✅ Fixed: Non-truncate path updates bytes using full chunk after overflow check
- Changed the non-truncate path to use limitedChunk.nextBytes instead of raw bytes + chunk.byteLength, making byte tracking consistent across both paths.
Or push these changes by commenting:
@cursor push cbe73cb407
Preview (cbe73cb407)
diff --git a/apps/server/spawned2.txt b/apps/server/spawned2.txt
deleted file mode 100644
--- a/apps/server/spawned2.txt
+++ /dev/null
@@ -1,2 +1,0 @@
-started
-started
\ No newline at end of file
diff --git a/apps/server/src/git/Layers/GitCore.ts b/apps/server/src/git/Layers/GitCore.ts
--- a/apps/server/src/git/Layers/GitCore.ts
+++ b/apps/server/src/git/Layers/GitCore.ts
@@ -532,7 +532,7 @@
}
const chunkToDecode = truncateOutputAtMaxBytes ? limitedChunk.chunk : chunk;
- bytes = truncateOutputAtMaxBytes ? limitedChunk.nextBytes : bytes + chunk.byteLength;
+ bytes = limitedChunk.nextBytes;
truncated = truncateOutputAtMaxBytes && limitedChunk.truncated;
const decoded = decoder.decode(chunkToDecode, { stream: !truncated });
diff --git a/apps/server/tmp-test.cmd b/apps/server/tmp-test.cmd
deleted file mode 100644
--- a/apps/server/tmp-test.cmd
+++ /dev/null
@@ -1,3 +1,0 @@
-@echo off
-echo started >> spawned2.txt
-
\ No newline at end of fileYou can send follow-ups to the cloud agent here.
| @@ -0,0 +1,2 @@ | |||
| started | |||
| started | |||
Contributor
There was a problem hiding this comment.
Debug test files accidentally committed to repository
Medium Severity
apps/server/spawned2.txt (containing "started" output) and apps/server/tmp-test.cmd (a batch file that appends to spawned2.txt) are debug/test artifacts that were accidentally included in the commit. These files are not referenced by any production or test code and appear to be leftovers from manual testing of process spawning.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 8d257e2. Configure here.
Comment on lines
+216
to
+218
| function quotePowerShellArgument(value: string): string { | ||
| return `"\`"${value.replaceAll("`", "``").replaceAll('"', '`"')}\`""`; | ||
| } |
Contributor
There was a problem hiding this comment.
🟠 High Layers/DesktopLauncher.ts:216
quotePowerShellArgument returns a string with literal double-quote characters embedded in the value, not just PowerShell quoting. The template "\`"${...}\`"" produces `"...`"" which PowerShell interprets as a string containing literal " characters at the start and end. When used as -FilePath in Start-Process, this causes the launch to fail because the path includes literal quote characters. For example, quotePowerShellArgument("C:\\path\\file.exe") produces "C:\path\file.exe" with quotes treated as part of the filename.
-function quotePowerShellArgument(value: string): string {
- return `"\`"${value.replaceAll("`", "``").replaceAll('"', '`"')}\`""`;
-}🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/server/src/process/Layers/DesktopLauncher.ts around lines 216-218:
`quotePowerShellArgument` returns a string with literal double-quote characters embedded in the value, not just PowerShell quoting. The template `` "\`"${...}\`"" `` produces `` `"...`"" `` which PowerShell interprets as a string containing literal `"` characters at the start and end. When used as `-FilePath` in `Start-Process`, this causes the launch to fail because the path includes literal quote characters. For example, `quotePowerShellArgument("C:\\path\\file.exe")` produces `"C:\path\file.exe"` with quotes treated as part of the filename.
Evidence trail:
- apps/server/src/process/Layers/DesktopLauncher.ts lines 216-217: `quotePowerShellArgument` function definition
- apps/server/src/process/Layers/DesktopLauncher.ts line 355: usage for `-FilePath` in `Start-Process`
- Microsoft docs https://learn.microsoft.com/en-us/powershell/module/microsoft.powershell.core/about/about_quoting_rules: confirms `` `" `` inside double-quoted strings produces literal quote characters
Contributor
ApprovabilityVerdict: Needs human review 3 blocking correctness issues found. This PR introduces a substantial new DesktopLauncher service with complex platform-specific process spawning logic. Multiple unresolved review comments identify potential bugs including a high-severity PowerShell quoting issue, a potential crash from incorrect Effect Exit handling, and accidentally committed debug files. These issues require human review before merging. You can customize Macroscope's approvability policy. Learn more. |
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Duplicate
makeWindowsEditorProtocolTargetacross two files- Extracted makeWindowsEditorProtocolTarget, WINDOWS_EDITOR_URI_SCHEMES, splitLineColumnSuffix, and LINE_COLUMN_SUFFIX_PATTERN into a single shared editorProtocol.ts module, and updated both open.ts and DesktopLauncher.ts to import from it.
Or push these changes by commenting:
@cursor push 980432cbd9
Preview (980432cbd9)
diff --git a/apps/server/src/editorProtocol.ts b/apps/server/src/editorProtocol.ts
new file mode 100644
--- /dev/null
+++ b/apps/server/src/editorProtocol.ts
@@ -1,0 +1,42 @@
+import { pathToFileURL } from "node:url";
+
+import type { EditorId } from "@t3tools/contracts";
+
+export const LINE_COLUMN_SUFFIX_PATTERN = /:\d+(?::\d+)?$/;
+
+export const WINDOWS_EDITOR_URI_SCHEMES: Partial<Record<EditorId, string>> = {
+ vscode: "vscode",
+ "vscode-insiders": "vscode-insiders",
+ vscodium: "vscodium",
+};
+
+export function splitLineColumnSuffix(target: string): {
+ readonly filePath: string;
+ readonly suffix: string;
+} {
+ const match = target.match(LINE_COLUMN_SUFFIX_PATTERN);
+ if (!match) {
+ return { filePath: target, suffix: "" };
+ }
+
+ return {
+ filePath: target.slice(0, -match[0].length),
+ suffix: match[0],
+ };
+}
+
+export function makeWindowsEditorProtocolTarget(
+ editor: EditorId,
+ target: string,
+): string | undefined {
+ const scheme = WINDOWS_EDITOR_URI_SCHEMES[editor];
+ if (!scheme) return undefined;
+
+ const { filePath, suffix } = splitLineColumnSuffix(target);
+ const fileUrl = pathToFileURL(filePath).href;
+ const fileTarget = fileUrl.startsWith("file:///")
+ ? fileUrl.slice("file:///".length)
+ : fileUrl.replace(/^file:\/\//, "");
+
+ return `${scheme}://file/${fileTarget}${suffix}`;
+}
diff --git a/apps/server/src/open.ts b/apps/server/src/open.ts
--- a/apps/server/src/open.ts
+++ b/apps/server/src/open.ts
@@ -9,11 +9,11 @@
import { spawn } from "node:child_process";
import { accessSync, constants, statSync } from "node:fs";
import { extname, join } from "node:path";
-import { pathToFileURL } from "node:url";
-
import { EDITORS, OpenError, type EditorId } from "@t3tools/contracts";
import { Context, Effect, Layer } from "effect";
+import { makeWindowsEditorProtocolTarget } from "./editorProtocol";
+
// ==============================
// Definitions
// ==============================
@@ -36,11 +36,6 @@
}
const TARGET_WITH_POSITION_PATTERN = /^(.*?):(\d+)(?::(\d+))?$/;
-const WINDOWS_EDITOR_URI_SCHEMES: Partial<Record<EditorId, string>> = {
- vscode: "vscode",
- "vscode-insiders": "vscode-insiders",
- vscodium: "vscodium",
-};
function parseTargetPathAndPosition(target: string): {
path: string;
@@ -59,36 +54,6 @@
};
}
-function splitTargetPathAndSuffix(target: string): {
- path: string;
- suffix: string;
-} {
- const parsedTarget = parseTargetPathAndPosition(target);
- if (!parsedTarget) {
- return { path: target, suffix: "" };
- }
-
- return {
- path: parsedTarget.path,
- suffix: parsedTarget.column
- ? `:${parsedTarget.line}:${parsedTarget.column}`
- : `:${parsedTarget.line}`,
- };
-}
-
-function makeWindowsEditorProtocolTarget(editor: EditorId, target: string): string | undefined {
- const scheme = WINDOWS_EDITOR_URI_SCHEMES[editor];
- if (!scheme) return undefined;
-
- const { path, suffix } = splitTargetPathAndSuffix(target);
- const fileUrl = pathToFileURL(path).href;
- const fileTarget = fileUrl.startsWith("file:///")
- ? fileUrl.slice("file:///".length)
- : fileUrl.replace(/^file:\/\//, "");
-
- return `${scheme}://file/${fileTarget}${suffix}`;
-}
-
function resolveCommandEditorArgs(
editor: (typeof EDITORS)[number],
target: string,
diff --git a/apps/server/src/process/Layers/DesktopLauncher.ts b/apps/server/src/process/Layers/DesktopLauncher.ts
--- a/apps/server/src/process/Layers/DesktopLauncher.ts
+++ b/apps/server/src/process/Layers/DesktopLauncher.ts
@@ -9,11 +9,10 @@
*/
import OS from "node:os";
import { spawn as spawnNodeChildProcess } from "node:child_process";
-import { pathToFileURL } from "node:url";
-
import { EDITORS, type EditorId } from "@t3tools/contracts";
import { Array, Effect, FileSystem, Layer, Option, Path, Scope } from "effect";
import { ChildProcess, ChildProcessSpawner } from "effect/unstable/process";
+import { LINE_COLUMN_SUFFIX_PATTERN, makeWindowsEditorProtocolTarget } from "../../editorProtocol";
import {
isWindowsBatchShim,
makeWindowsCmdSpawnArguments,
@@ -113,7 +112,6 @@
type LaunchPlanError = LaunchAttemptError | DesktopLauncherLaunchAttemptsExhaustedError;
-const LINE_COLUMN_SUFFIX_PATTERN = /:\d+(?::\d+)?$/;
const WINDOWS_POWERSHELL_CANDIDATES = ["powershell.exe", "powershell", "pwsh.exe", "pwsh"] as const;
const WSL_POWERSHELL_CANDIDATES = [
"/mnt/c/Windows/System32/WindowsPowerShell/v1.0/powershell.exe",
@@ -121,12 +119,6 @@
"powershell.exe",
"pwsh.exe",
] as const;
-const WINDOWS_EDITOR_URI_SCHEMES: Partial<Record<EditorId, string>> = {
- vscode: "vscode",
- "vscode-insiders": "vscode-insiders",
- vscodium: "vscodium",
-};
-
function shouldUseGotoFlag(editor: (typeof EDITORS)[number], target: string): boolean {
return editor.supportsGoto && LINE_COLUMN_SUFFIX_PATTERN.test(target);
}
@@ -135,24 +127,6 @@
return value.replace(/^"+|"+$/g, "");
}
-function splitLineColumnSuffix(target: string): {
- readonly filePath: string;
- readonly suffix: string;
-} {
- const match = target.match(LINE_COLUMN_SUFFIX_PATTERN);
- if (!match) {
- return {
- filePath: target,
- suffix: "",
- };
- }
-
- return {
- filePath: target.slice(0, -match[0].length),
- suffix: match[0],
- };
-}
-
function resolvePathEnvironmentVariable(env: NodeJS.ProcessEnv): string {
return env.PATH ?? env.Path ?? env.path ?? "";
}
@@ -247,19 +221,6 @@
return runtime.isWsl && !runtime.isInsideContainer && isUriLikeTarget(input.target);
}
-function makeWindowsEditorProtocolTarget(editor: EditorId, target: string): string | undefined {
- const scheme = WINDOWS_EDITOR_URI_SCHEMES[editor];
- if (!scheme) return undefined;
-
- const { filePath, suffix } = splitLineColumnSuffix(target);
- const fileUrl = pathToFileURL(filePath).href;
- const fileTarget = fileUrl.startsWith("file:///")
- ? fileUrl.slice("file:///".length)
- : fileUrl.replace(/^file:\/\//, "");
-
- return `${scheme}://file/${fileTarget}${suffix}`;
-}
-
function makeLaunchPlan(
command: string,
args: ReadonlyArray<string>,You can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit b909b94. Configure here.
| : fileUrl.replace(/^file:\/\//, ""); | ||
|
|
||
| return `${scheme}://file/${fileTarget}${suffix}`; | ||
| } |
Contributor
There was a problem hiding this comment.
Duplicate makeWindowsEditorProtocolTarget across two files
Low Severity
makeWindowsEditorProtocolTarget, WINDOWS_EDITOR_URI_SCHEMES, and their helper functions (splitTargetPathAndSuffix / splitLineColumnSuffix) are independently implemented in both open.ts and DesktopLauncher.ts with near-identical logic. A bug fix in one would need to be mirrored in the other.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit b909b94. Configure here.
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
openpackage.opendependency from the server package.Testing
bun fmtnot runbun lintnot runbun typechecknot runbun run testnot runNote
Medium Risk
Touches cross-platform process spawning/termination and output truncation behavior (Windows/WSL/macOS/Linux), which can affect editor/browser launching and subprocess cleanup. Changes are well-covered by new unit tests but may vary across real environments.
Overview
Refactors desktop open/launch behavior toward an Effect-based
DesktopLauncherservice with structured launch plans, cross-platform fallback logic (including WSL/Windows PowerShell and Windows editor protocol URLs), and richer error reporting.Extracts shared process utilities into
process/(process-tree killing viataskkill, Windows command helpers, output buffer limiting, andrunProbeProcess) and updates callers (processRunner, Codex session shutdown, terminal subprocess probes, and Git output streaming) to use the centralized implementations.Improves Windows launch reliability by special-casing
explorerspawns (noshell: true,windowsHide, protocol URL encoding) and adds extensive test coverage for editor discovery, launch plans, and failure/fallback scenarios.Reviewed by Cursor Bugbot for commit b909b94. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Refactor open service onto Effect-backed cross-platform DesktopLauncher process spawning
DesktopLauncherEffect service inapps/server/src/process/Layers/DesktopLauncher.tswithgetAvailableEditors,openBrowser,openInEditor, andopenExternal, replacing ad-hoc spawn logic inopen.ts.openflags, Windows PowerShell encoded commands andexplorerfor VS Code protocol URIs (vscode://file/...), Linuxxdg-open, and WSL-aware fallbacks with container detection.killTree.tsfor Windows process-tree termination viataskkill,outputBuffer.tsfor byte-limited output accumulation,windowsCommand.tsfor cmd quoting/shell resolution, andrunProbeProcess.tsas a truncate-mode wrapper.processRunner.ts,codexAppServerManager.ts,terminal/Layers/Manager.ts, andgit/Layers/GitCore.tsto consume the new shared utilities.packages/contractsschema defaults fromEffect.succeed(...)to plain thunks (() => value) throughout.defaultSpawnDetacheduses a 500ms grace period to detect immediate failures before unreffing the child process; very fast-exiting GUI processes may be misreported as successful.Macroscope summarized b909b94.