Windows 3/5: desktop, auth, sync, and remote clients - #1008
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
14615bd to
8cdf335
Compare
8cb0bf0 to
c4f935e
Compare
|
@CodeRabbit review. @greptile-apps review. |
|
✅ Action performedReview finished.
|
8cdf335 to
1525333
Compare
c4f935e to
936722b
Compare
1525333 to
37dfcd6
Compare
936722b to
34cbcdd
Compare
37dfcd6 to
b1a3fa3
Compare
34cbcdd to
a3e370e
Compare
b1a3fa3 to
3a5a140
Compare
a3e370e to
f2eabb4
Compare
3a5a140 to
48634b8
Compare
2d24eb1 to
7ee4417
Compare
8418d96 to
cf15001
Compare
7ee4417 to
ecbfd45
Compare
cf15001 to
bae30ae
Compare
4a4789a to
9370628
Compare
bae30ae to
65a421f
Compare
a5ef5d8 to
aeab583
Compare
A tracked CLI launch is rendered once, in POSIX form, and that rendering
is the one ADE persists and displays. On macOS the line is then handed to
`/bin/bash -lc`, which speaks exactly those quoting rules, so it survives
verbatim. Windows had no equivalent: `directShellLaunchForCommandLine`
returned nothing on win32, leaving callers with a bare `startupCommand`
that ptyService typed into the default shell — PowerShell — whose rules
the line was never written in.
Measured on Windows 11, PowerShell 5.1.26100, through a real ConPTY, with
a shim recording the argv the CLI actually received:
typed codex --no-alt-screen --model gpt-5.5 \
-c "model_reasoning_effort=\"high\"" resume <id>
argv ..., "-c", "model_reasoning_effort=\", "high\", ...
codex started with a corrupt `-c` value and a stray positional, and its
header rendered `model: gpt-5.5 \`. The other shells fail differently: a
multi-line prompt reaches cmd.exe as six arguments with `%USERPROFILE%`
expanded and the newlines gone, and `'C:\…\opencode.exe' serve` is a hard
ParserError in PowerShell, where a quoted path is a string expression
rather than an invocation.
Prefer a direct spawn. `resolveCanonicalCommandLineLaunch` recovers the
argv behind a canonical line and accepts it only when re-rendering that
argv reproduces the line byte for byte, so the recovery is lossless by
construction and anything that is not plain argv — a pipeline, a redirect,
or a line some other producer emitted under different quoting rules —
falls back to today's behaviour instead of being mis-split. A leading run
of `NAME=value` assignments is lifted into env rather than passed as argv.
Nine of the ten resume and fork shapes across the five providers now spawn
directly and no shell sees the quoting at all.
Where a line genuinely must be typed — a direct spawn that failed and fell
back to the shell — render it at the write site, for the shell that won the
spawn race, via `commandArrayToWindowsShellLine`. That is the only point
where the receiving shell is actually known. PowerShell needs the call
operator for a quoted executable, and Windows PowerShell 5.1 builds native
command lines without escaping embedded quotes, so values are pre-escaped
to CRT rules there and left alone under pwsh 7+, which gets it right on its
own.
Two supporting corrections keep the canonical form recoverable: the POSIX
parser now reads back the ANSI-C `$'…'` quoting that `quoteShellArg` emits
for control characters — without it, multi-line prompts, the arguments that
most need recovering, were exactly the ones that could not be — and the
Windows shell-fallback line uses the shared quoter, since a second
differently-shaped POSIX quoter would silently fail the round-trip check.
Based-on: nsxdavid/ADE#999
Importing a discovered session builds its launch from the canonical POSIX
resume command. For four providers that line is plain argv and the launch
boundary can recover it. Droid's is not: it is a genuine shell script that
mktemps a settings file, `printf`s JSON into it and chains with `&&`, using
`$(…)` and `${TMPDIR:-/tmp}`. On Windows that was handed to PowerShell,
which has none of those constructs.
cliLaunch already knows this and returns a `powershell.exe -Command` launch
for Droid when asked for win32, but the import path asked
`buildTrackedCliResumeCommand`, which is pinned to the POSIX rendering
because that is what gets persisted and displayed. Take the platform-aware
structured launch for its command and args when one exists, and keep
recovering argv from the canonical line otherwise. `startupCommand` is
unchanged, so the persisted and displayed form stays POSIX.
Based-on: nsxdavid/ADE#999
Both lookups turned a cwd into a directory name with `replace(/\//g, "-")`, which transforms nothing on Windows — its separators are backslashes — and leaves the drive colon in place. NTFS cannot hold a name containing `:`. `claudeProjectDirForCwd` therefore looked for `…\.claude\projects\C:\Users\me\repo`, a path that can never resolve, so Claude session recovery from local JSONL storage was dead on Windows. Claude's rule is every non-alphanumeric to `-`, runs not collapsed, nothing trimmed — confirmed against this machine's real `~/.claude/projects`, where three cwds including a worktree path now resolve to directories that exist on disk. `discoveryUtils.claudeProjectSlugForCwd` already encodes exactly that, so reuse it rather than add a fourth copy. `resolveDroidSessionIdFromStorage` degraded rather than failed: the expected project directory never existed, so it scanned every session directory and leaned on its `record.cwd === args.cwd` filter to stay correct. This is match precision and cost, not a wrong answer. Droid escapes differently again — `sanitizePathToDirectoryName` replaces only path separators, so dots, underscores and spaces survive, collapses runs of them, strips the drive colon as a deliberate special case, and always prefixes a dash. Match directory names by that rule, case-folded on win32 because Windows paths are case-insensitive and the vendor's escaping is not. Three providers, three mutually incompatible rules; none generalises to another. The Droid rule here is transcribed second-hand from `@factory/droid-sdk@0.2.0` rather than read from an installed SDK — Droid is not installed on this machine and there is no `~/.factory` to corroborate against, unlike the Claude side. Its worked examples are covered, and the degrade-not-fail shape bounds the risk. A TODO marks the unification with `droidProjectSlugForCwd()` in externalSessions/discoverDroid.ts once the branches share a base. Based-on: nsxdavid/ADE#999
`quoteWindowsCmdArg` mapped `%` to `%%`, which is a batch-*file* escape. The strings it quotes go on a `cmd /d /s /c "…"` command line, where `%%` is not an escape at all — it survives literally. Measured through a real `.cmd` shim, comparing the current helper against leaving `%` alone: "100% done" -> "100%% done" vs "100% done" "50%" -> "50%%" vs "50%" "%USERPROFILE%" -> "%C:\Users\me%" vs "C:\Users\me" The doubling never prevented expansion — it expanded anyway and left a stray `%` on each side — so it was pure corruption. Nor is there a correct alternative: inside a quoted argument `^` is inert, and `%^VAR%` arrives verbatim as `%^VAR%`, blocking expansion but corrupting the text a different way. Undefined names already round-trip literally. So leave `%` alone: literal percent signs are now correct, which is a strict improvement, and the docblock records why no escape exists and where callers must keep `%VAR%` text off the command line instead. Checked every consumer of this helper before changing it. `resolveWindowsCmdInvocation`, `openCodeServerManager`'s `set X=Y&&"exe" …` line and `registerIpc`'s `cmd /c start ""` are all command lines, so all three were corrupting and all three are fixed. Nothing relied on the doubling. The three other `%`-doubling sites in the repo are separate implementations that target real batch files (`adeCliService.createWindowsShimScript`, `cursorSdkHooks.windowsBatchQuote`) where `%%` is correct, and are untouched. An OpenCode expectation moves as a direct consequence: a directory genuinely named `100% dev` reached cmd.exe as `100%% dev` and could not be launched. POSIX needs no equivalent: `%` is not special to `sh`. Based-on: nsxdavid/ADE#999
# Conflicts: # apps/desktop/src/main/services/externalSessions/discoveryUtils.ts
# Conflicts: # apps/desktop/src/main/services/externalSessions/discoveryUtils.test.ts # apps/desktop/src/renderer/components/onboarding/AiRuntimesBand.tsx
# Conflicts: # apps/desktop/src/main/services/ai/authDetector.ts
Each of these was left deliberately by a lane that could not reach the other lane's code. Now that they share a base, there is one home each. - discoveryUtils no longer carries its own copy of Cursor's vendor slug rule; it imports cursorProjectSlug() from shared/cursorProjectSlug.ts. The bodies were byte-identical, so behaviour is unchanged. - discoverCursor's resolveCursorProjectCwd/cursorProjectCwdCandidates compensation (retry with a leading `<letter>-` stripped) is gone. The drive-aware anchors in cursorSlugCwdCandidates() already consume the drive segment, and `discovers Cursor transcripts and resolves cwd from the project slug` passes without the retry. - discoveryUtils.slashEscapedCwd() had no production caller left once droid's droidProjectSlugForCwd() landed. Removed with the assertion that expected "C:-Users-dev-ADE", a name NTFS cannot hold. - ptyService's second-hand reimplementation of Droid's slug rule now imports droidProjectSlugForCwd() from discoverDroid.ts. Same output; the shared one additionally resolves/realpaths the cwd first and lower-cases on win32, which foldDroidProjectSlug() then repeats idempotently.
…shim `resolveFromDirs` now resolves through PATHEXT only on win32, because Windows cannot execute an extension-less file. This fixture wrote a bare `opencode` sh script into a bin dir on PATH — a shape no Windows install produces — so the binary was correctly reported missing and the import threw. Write `opencode.cmd` there instead, which is what `npm i -g opencode-ai` actually leaves behind.
spawnAsync wraps any extension-less command in cmd.exe /d /s /c, so "where" paid ~15ms of wrapper overhead on every lookup. Naming the executable outright skips it. Measured on this machine: direct where.exe 58.8-60.2ms vs cmd-wrapped where 74.4-74.8ms. The sibling defect in authDetector.commandExists was both a latency and a correctness bug, because cmd.exe always launches and reports its own exit code 1 for a command it cannot find. This site only ever tested status !== 0, so it was never wrong -- only slow. Based-on: nsxdavid/ADE#999
quoteCmdSetValue mapped % to %%, which is correct only inside a batch file. This builds a line for a live cmd prompt, where %% survives literally -- so a directory named "100% dev" was rendered as "100%% dev" and the command the user copied could not run. Nothing can escape % on a live command line, so leaving it untouched is the correct behaviour rather than a compromise: literal percent signs now round-trip. The caller already depended on this, keeping the trailing %PATH% outside this function precisely so it still expands. The same mapping was removed from processExecution.quoteWindowsCmdArg for the same reason. Three other %%-doubling sites in the repo target real batch files and remain correct. Based-on: nsxdavid/ADE#999
…verlay 29adbe1 fixed the sidebar-hover stall by moving the rail's expansion into an absolutely positioned `.ade-sidebar-flyout`, so the panes to its right never saw a resize. That worked, but it changed the behaviour: hovering expanded the rail *over* the content instead of pushing it aside. macOS pushes. This reverts that part of 29adbe1 — the rail animates its own width in normal flow again, and `.ade-sidebar-flyout` is gone — and replaces it with a fix that attacks the actual cost. There is no separate macOS code path. Both platforms run this same CSS; macOS gets away with it purely because of refresh rate. A 200ms transition is ~12 frames at 60 Hz and ~48 at 240 Hz, and the per-frame cost is identical. So the goal is not to imitate a different macOS implementation, it is to make push cheap on a high-refresh display. The expense was never "layout happened". It was that every frame of the transition resized the react-resizable-panels groups inside <main>, and their ResizeObserver answers a resize by reading offsetWidth for the group and each panel (a forced synchronous layout) and then re-rendering every pane through React. Forty-eight of those in 200ms is what pinned the main thread. None of that intermediate work is load bearing: rrp sizes panels with flex-grow ratios and flexBasis 0, so the browser already scales the panes correctly as the container narrows, for free, every frame. The observer callback only re-derives pixel constraints, and that only matters at the size the group comes to rest at. So renderer/lib/layoutSettle.ts installs a ResizeObserver wrapper that can hold delivery for observers watching elements inside a scope, and AppShell holds <main> for the length of the rail's transition, releasing on `transitionend` (with a 400ms backstop). Release re-observes the held targets rather than replaying stale entries, so consumers get real settled geometry. Measured on Windows at 240 Hz (CDP CPU profile + rAF intervals, 6s hover sweep over the rail, dev build, /review with 4 panels), 3 runs each: main thread busy 61.3/61.5/47.6% -> 28.9/31.0/33.0% (flyout: 19.5/18.9/15.4%) rrp self time 1545/1403/985ms -> 46/57/74ms (flyout: 23/34/22ms) frame interval p50 4.2ms -> 4.2ms frames over 8ms 308/984 (31%) -> 4/1491 (0.3%) (flyout: 1/1466) frames over 20ms 1-3 -> 1 Push is preserved and verified geometrically: over one expansion the rail goes 52 -> 124 -> 140px, <main> goes x=52 w=1275 -> x=140 w=1187, and the first pane goes 306 -> 290 -> 285px. The flyout build leaves <main> at x=52 w=1275 throughout, which is the overlay this replaces. The residual gap to the flyout build is `(program)` — browser style, layout and paint for the reflow the push genuinely performs. That is the honest cost of pushing content and macOS pays it too; no JS frame accounts for it. Based-on: nsxdavid/ADE#999
@cursor/sdk publishes optional platform packages for darwin-arm64,
darwin-x64, linux-arm64, linux-x64 and win32-x64 — as of 1.0.23 there is
no win32-arm64. ADE's Cursor provider is built entirely on that SDK, so
on Windows on ARM every chat, model discovery and auth probe fails at
import("@cursor/sdk"). Ship without the provider rather than with a
broken one. This is an SDK packaging gap, not a Cursor gap: Cursor's own
CLI installer does support ARM64, so terminal launches of `cursor-agent`
are deliberately left alone.
shared/providerPlatformSupport.ts is the single source of truth and
carries the revisit condition. buildProviderConnections() is the
authoritative enforcement point in main: availableProviders.cursor, the
cursor-family model filter and every status consumer derive from it.
The three surfaces that enumerate providers from hardcoded lists rather
than from status — Settings > Agents and Models, onboarding, and the
model picker rail — read the same predicate through a new synchronous
preload runtimeTarget bridge, because navigator.platform reports "Win32"
on Windows on ARM and cannot distinguish the architecture.
loadCursorSdk() and ade-cli's Cursor Cloud loader assert the same
predicate so a stale setting restored from an x64 machine, a persisted
default or a deep link fails with the explained blocker instead of an
opaque ERR_MODULE_NOT_FOUND. The failure is tagged so
isCursorSdkResolutionError() treats it as an unusable module, which stops
model discovery from falling back to the HTTP API and advertising models
no chat could run.
Claude, Codex, Droid and OpenCode are untouched, as are Windows x64,
macOS and Linux on every architecture.
Based-on: nsxdavid/ADE#999
…desktop-sync # Conflicts: # apps/desktop/src/main/services/externalSessions/discoverCursor.ts # apps/desktop/src/main/services/externalSessions/discoverDroid.ts # apps/desktop/src/main/services/externalSessions/discoverProviders.test.ts # apps/desktop/src/main/services/externalSessions/externalSessionsService.ts # apps/desktop/src/renderer/components/onboarding/AiRuntimesBand.tsx
@factory/droid-sdk's ProcessTransport.connect() spawns the CLI with only
{ stdio, cwd, env } -- no windowsHide -- and createSession() exposes no way
to pass spawn options through. On Windows that allocates a console for
droid.exe, and with Windows Terminal as the default console host it is not
a brief flash: a real window titled with the droid path opens and stays for
the life of the session.
Reproduced with droid v0.186.0 spawned from a CREATE_NO_WINDOW parent, the
way the Electron-forked worker runs:
without windowsHide -> conhost child of the droid PID, plus a visible
WindowsTerminal window titled
"C:/Users/arul2/bin/droid.exe"
with windowsHide -> console still allocated but hidden; no visible
window
Since the SDK takes no options, default windowsHide for droid spawns only,
matched on the executable basename so no other spawn site in the process
can be affected and an explicit windowsHide from a caller always wins.
Applied in two places, because two of them spawn droid:
- droidSdkWorker, before the SDK is loaded
- droidModelsDiscovery, which calls createSession() in-process and so
spawns droid straight from the Electron main process on a passive
model-warm path
Verified end to end against the built worker: forking the real
dist/droidSdkWorker.cjs from a windowless parent reached "ready" with a
live droid.exe and zero visible droid windows.
Based-on: nsxdavid/ADE#999
(cherry picked from commit 2d4ffab21857c397b6a9f12d419a69d7b0aa2b85)
Verified against a real droid.exe v0.186.0 at C:\Users\arul2\bin with no Factory
account. Forced detection returned authenticated:true, verified:true while
`droid exec "say hi"` returned "Authentication failed. Please log in using
/login or set a valid FACTORY_API_KEY environment variable."
Two compounding causes, both measured here. `droid exec --list-tools` exits 0
with no account at all — it prints the local tool policy and never contacts
Factory — and that was the branch asserting authentication. And the real refusal
string matched none of STRONG_UNAUTH_INDICATORS, so even a genuine failure could
not be recognised. Dropped the probe as an auth signal and added
/authentication failed/i to the shared indicator list.
The two fallback probes were dead code masked by that bug, and expensive.
`droid --help` lists exec, daemon, search, update, mcp, plugin, computer and
help — `whoami` and `account status` are not subcommands, so droid took each as
a *prompt* and booted the full interactive TUI until spawnAsync's timeout reaped
it. A forced refresh spawned a 150MB agent twice to learn nothing. Both are
gone, and CLI_AUTH_PROBES.droid now lists only documented flags: `version` had
the same defect one fallback deeper.
Droid exposes no cheap auth probe — the only authoritative signal is a real
`droid exec` round trip, which costs a model call on a signed-in machine — so
detection now stops at "installed, auth unknown". verified:false keeps this out
of the explicitly-signed-out state, and the connection renders as "installed but
no credentials were detected", which is exactly true. The forced and passive
paths now agree; before, forcing a refresh made the answer worse.
After, forced, with the real binary present:
{"cli":"droid","installed":true,"path":"C:\Users\arul2\bin\droid.exe",
"authenticated":false,"verified":false}
and detection of all four CLIs completes in 954ms.
hasDroidConfiguredCredentials is left as-is but documented: ~/.factory/settings.json
holds UI preferences only ({"logoAnimation":"off"} on this machine) and nothing
credential-shaped exists anywhere under ~/.factory, while a stack trace in
~/.factory/logs names a dedicated CredentialsStorage module. Confirming the real
location needs a signed-in account, so the comment records the evidence and the
invariant that `false` means "no credential ADE can see", never "signed out".
The replaced test asserted the bug in its name.
Based-on: nsxdavid/ADE#999
(cherry picked from commit f189e8cdbeb3ced3bb77331d6c9ff1d820455d3f)
3e23c30
into
codex/windows-cli-shells-providers
Stack position
Windows 3/5 · parent: #1007 · downstream: #1009
This is layer 3 of stack #1011. No layer merges independently; PR #1010 is the cumulative full-system head.
Based on the Windows implementation by @nsxdavid in #999.
Responsibilities
Exclusions
Packaging/signing/updating belongs to #1009. WSL, ARM64, and native Windows computer use are excluded.
Validation
Draft checkpoint: direct-parent typecheck passes. CurrentUser DPAPI account protection is integrated; coordinator reproduced 52 CLI credential/sync tests and 23 desktop pairing tests on native Windows. OAuth and cross-client Computer Use proof remain pending.
Evidence and provenance