Conversation
Embed the Zeron icon as resource 1, which GPUI loads from the executable, for Windows debug and release builds.
Separate iOS, edge, and shared Rust test triggers, restrict deployment runs to relevant paths, and build the Windows GUI fixture only for explicit native GUI runs.
…native-support
Contain children at creation, preserve launch and stdio behavior, and share startup-attribute handling. Keep idle terminal batching demand-driven and cover cleanup, discovery, and launch regressions.
Remove unrelated workflow reorganization, retain shared Rust coverage, and shorten the Windows setup guide.
wingleeio
left a comment
wingleeio
left a comment
There was a problem hiding this comment.
Reviewed a48e1fb. Requesting changes for the three issues below: a failed Windows update can remove the launchable installation, Claude availability no longer honors its environment override, and Windows npm Codex discovery is not used by account login.
Validation on Linux: 174 engine unit tests, 130 harness unit tests, 8 auth integration tests, and 5 updater unit tests passed (317 total). A separate isolated regression probe reproduced the Claude override failure. I inspected both pinned UI dependency changes and the new updater dependency. I could not run the Windows build, ConPTY/Job Object tests, GUI probes, or a real Windows update here; this head currently has no successful Windows CI result.
WorkOS on Windows: I found no Windows-specific blocker in the implemented authentication path. The desktop uses GPUI's Windows ShellExecuteW browser opener, then receives the OAuth callback on http://127.0.0.1:<port>/callback; exchange, refresh, and organization requests go through the existing edge API. It does not depend on registering a custom URL scheme. Session persistence uses the new application data directory, shared by GUI and CLI. WorkOS documents loopback redirects for native clients (redirect requirements); the configured client must allow the callback URI/port pattern.
That is a source-level compatibility assessment plus passing stub-server auth tests on Linux, not confirmation of an authenticated Windows run. Please include cargo test --release --locked -p zeron-engine --test auth in Windows CI (the current workflow skips that integration target), and verify browser sign-in, app restart/session restore, refresh, org selection, and logout on Windows before claiming authenticated support. Windows session-file privacy currently relies on inherited directory ACLs; the Unix-only 0600 enforcement does not apply there.
The PR description also needs updating: the current diff adds portable packaging, publication, and in-app updates, although the description still lists them as follow-ups.
A failed Windows update could remove the launchable installation: self-replace renames the running executable away and schedules its deletion before copying the replacement back. Replace it with a hand-rolled swap that copies and re-verifies first, moves the running image aside only then, and restores the backup on every post-move failure; the relaunched instance deletes the renamed backup once the old image exits, and the next update attempt heals a crash between the two renames. The new failure-injection test holds a real sharing violation on the incoming copy after the old image has moved. Claude availability ignored CLAUDE_CODE_EXECUTABLE: installed() used bare discovery while the launch resolver honored the override, so an override-only install showed as unavailable (also on Linux/macOS). installed() now uses the validated launch resolver, with a cross-platform override-only availability test. Codex account login spawned a bare `codex` command that neither followed the npm payload layout nor honored CODEX_EXECUTABLE. The engine now launches `codex login` through the harness's own resolution and child-PATH composition via codex::login_command. Windows CI additionally runs the zeron-engine auth integration tests.
Both the resolver and the spawner were hard-coded to .exe, so
npm-installed agents were undiscoverable and unlaunchable: npm leaves
only a `name.cmd` shim on PATH with the real binary buried under
node_modules. Discovery is now PATHEXT-aware — per directory,
extensions in PATHEXT order, limited to what we can launch (.com,
.exe, .bat, .cmd) — and overrides accept batch shims that exist on
disk, so availability and launches agree.
Resolved .cmd/.bat scripts spawn through `cmd.exe /d /s /c` with
cross-spawn-style per-argument escaping: the shim is the only
interpreted layer, agent arguments stay literal (spaces, backslashes,
quotes, unicode, cmd metacharacters round-trip verbatim — proven by
re-invoking a real program through a shim fixture), and Job Object
ownership covers the whole tree through cmd.exe's membership. The
codex node_modules vendor special-case is deleted: it searched the
wrong vendor subdirectory (`bin/` instead of `codex/`), and its
fixture encoded that wrong layout, so CI passed while real npm
installs failed. GUI launches additionally backfill %APPDATA%\npm,
%LOCALAPPDATA%\{pnpm,Programs\nodejs}, scoop shims, and
%USERPROFILE%\{.local\bin,.bun\bin} from PATH.
opencode and cursor resolution migrate off raw $HOME to the shared
USERPROFILE-aware home helper; cursor also probes
%LOCALAPPDATA%\cursor-agent. The engine catalog and login tests use
the real npm shim layout, batch-override harness tests assert safe
launch instead of rejection, and the shell-script fixture suites are
gated to Unix where they actually run.
wingleeio
left a comment
wingleeio
left a comment
There was a problem hiding this comment.
Reviewed 73b245c. The three earlier requests are addressed on this head (the swap now copies and verifies before moving the running image aside, Claude installed() uses the launch resolver, and Codex login goes through codex::login_command). Requesting changes again for one behavior regression and three new tests that cannot pass on Linux.
Validation on Linux (this head): cargo check --locked --workspace --all-targets clean; engine 174 unit tests, auth 8, updater 10, app 5, claude 9, codex 20, cursor 20, acp_quiet 1, acp_stall 6 all passing. Failing: harness unit tests 129/131, tests/acp.rs 26/27, codex_login_resolution 3/4, claude_availability 2/3 — details inline. The terminals integration test (m5_repos_diffs_terminals) passed 21/22 with the one failure being a pre-existing SHA-prefix fuzzy-match flake that also reproduces independently of this branch. I rebuilt against the pinned Zui/gpui-base revisions; both match their upstream main heads.
CI: I approved the fork workflow runs on this head. Preview networking and UI tests (all five jobs) are green. Windows tests: green on this head (build, shader layout, app/updater, engine+harness unit, native harness integration, catalog, auth, UI, and the no-HOME CLI startup probe; the native GUI job is dispatch-only and was not run). CI, a fresh release build, and the GUI probes are no longer "pending" for this revision..
Nits (no change required to land):
release.yml:publishnow requires the Windows job, so a Windows-only build failure (fxc discovery, runner image drift) blocks Linux and macOS releases. That is a reasonable "every platform" policy, just calling it out as a deliberate change from the previous comment's intent.crates/update/src/lib.rsrelease_base:ZERON_RELEASES_URLis honored on every platform and is not HTTPS-checked, unlike the Windowszeron-update.jsonfeed. Consider the samehttps://guard.process/windows/command.rsquote_batch_argument: agent arguments passed to a.cmdshim are quoted but not%-escaped, so a%NAME%inside an argv element is still expanded by cmd.exe. Today every harness argv is program-controlled (flags, model ids, settings JSON), so this is not reachable from prompts; worth a comment or^%escaping so it stays that way.terminals/windows.rsProcess::wait_for: terminating the job when the shell exits also kills anything the user detached from that shell (start notepad, background servers), which differs from the Unix terminals where orphans survive. If intentional, a doc note inwindows-development.mdwould help.
| let spec_args: Vec<String> = self.spec.args.iter().map(|a| a.to_string()).collect(); | ||
| if let Some(p) = &self.executable { | ||
| return Ok(Launch::Program(p.clone(), spec_args)); | ||
| return crate::executable::validate_native_override(p) |
There was a problem hiding this comment.
[P2] Model discovery now errors instead of falling back for a missing override
models() starts with self.resolve_launch()?. On main, an executable/env override that does not exist passed through here and failed at spawn, so discover_models() returned Err and models() served (self.spec.models)(). With validate_native_override the ? fires first, so a stale GROK_EXECUTABLE/PI_ACP_EXECUTABLE/etc. now fails model discovery outright. The pre-existing regression test for this path, models_fall_back_to_the_static_catalog_when_the_probe_fails in tests/acp.rs, fails on this head with NotInstalled("/nonexistent/never-a-pi-acp does not exist").
Meanwhile AcpHarness::installed() (line ~1125) still returns true for any Some(executable) or non-empty env override, so the registry can advertise an ACP agent as installed whose models() and run() both fail. That is the exact inconsistency this PR fixed for Claude and Codex. Please pick one: keep the static-catalog fallback (return Ok(static) when resolve_launch() fails here), or make ACP consistent with the other harnesses by gating installed() on resolve_launch().is_ok() and updating the test to expect the error. Either way the existing test needs to reflect the chosen behavior.
| assert_eq!( | ||
| find_on_paths_with( | ||
| "tool", | ||
| vec![bin.join("tool.CmD")], |
There was a problem hiding this comment.
[P2] Two new unit tests assume a case-insensitive filesystem
windows_extra_candidates_resolve_through_pathext_variants writes tool.cmd and expects the tool.CmD extra to resolve, and windows_pathext_environment_reorders_extensions (line 429) writes agent.cmd and expects the .CMD PATHEXT variant to resolve. Both go through is_file() on the real filesystem, so they pass on NTFS and default APFS but fail on Linux and case-sensitive macOS volumes:
windows_extra_candidates_resolve_through_pathext_variants: left: None, right: Some(".../bin/tool.CmD")
windows_pathext_environment_reorders_extensions: shim variant resolves
This breaks cargo test -p zeron-harness --lib on Linux, which is the suite the PR description cites as the shared regression check. Please either match the on-disk case in these two tests (keep the case-insensitivity claim to the candidate_names/extra_variants output assertions, which need no filesystem) or gate the case-dependent assertions with #[cfg(windows)]. Both were added in 6f5a40a.
| let bin = dir.path().join("bin"); | ||
| std::fs::create_dir_all(&bin).unwrap(); | ||
| // The shim plays both roles itself. | ||
| std::fs::copy(fake_codex(&bin), bin.join("codex.cmd")).unwrap(); |
There was a problem hiding this comment.
[P2] codex_login_runs_the_npm_cmd_shim cannot pass on Unix
This binary is not cfg(windows)-gated, but .cmd discovery only exists through the Windows PATHEXT variants: on Unix find_on_paths("codex", ..) looks for a file literally named codex, so bin/codex.cmd is never a candidate. On a runner without codex the child fails with NotInstalled while the parent expects true. On a dev machine with /usr/local/bin/codex it is worse: the absolute fallback resolves the real CLI, start_login launches a real codex login (against the throwaway CODEX_HOME, so no credentials are touched), and the assertion fails on the URL instead:
left: "", right: "https://auth.openai.com/authorize?probe=fake"
Please gate this test (or the whole file) with #[cfg(windows)], and add the same /opt/homebrew/bin/codex / /usr/local/bin/codex guard the without_any_cli test already has to the override-only test so it never reaches a real CLI.
| std::fs::write(&exe, if cfg!(windows) { b"MZ" } else { b"sh" }).unwrap(); | ||
| // PATH without any claude: only the override makes Claude installed. | ||
| probe(dir.path(), Some(&exe), true); | ||
| probe(dir.path(), None, false); |
There was a problem hiding this comment.
[P3] Second probe has no guard for a system-wide claude
The expected = false probe here removes HOME and empties PATH, but resolve_claude_executable still checks the absolute /opt/homebrew/bin/claude and /usr/local/bin/claude fallbacks, so on any machine with claude installed there the child reports installed = true and this test fails. The sibling claude_availability_reports_not_installed_without_any_cli already skips in that case; please apply the same guard (or drop this duplicate probe, since the sibling covers it).
|
Pushed three commits to this branch (maintainer edit) so it can land without another round trip:
Linux on the merged head: engine 177, harness 131, acp 28, claude_availability 3, codex_login_resolution 3, terminals 22, auth 8, updater 5, app 10 — all passing; |
Resolve Cargo.toml (keep main's newer Zui/gpui-base pins), crates/ui/Cargo.toml (keep both the windows-render-fixture and appshots-fixture examples), and Cargo.lock (main's lockfile plus this branch's windows-sys/embed-resource dependency entries).
1064191 to
d106661
Compare
…breaks - AcpHarness::installed() now validates `executable`/env overrides the same way resolve_launch does, so a missing override is not advertised as an installed agent whose model discovery and launch both fail. The static-catalog fallback test now uses an executable that exists but cannot speak ACP (the probe-failure case it was written for); a new test covers the missing-override case as not-installed + NotInstalled. - executable.rs: the two PATHEXT-casing tests wrote one case to disk and looked up another, which only resolves on case-insensitive filesystems. Match the on-disk name so they pass on Linux too. - codex_login_resolution: the npm .cmd shim test depends on PATHEXT discovery and is now cfg(windows). - claude_availability: the not-installed probe skips on machines with a system-wide /usr/local/bin or /opt/homebrew/bin claude, like its sibling.
d106661 to
61306e4
Compare
appshot_destination is serde(skip) outside macOS and Linux, so it reloads as the default there. The round-trip test hard-coded NewSession and could only pass where the field is persisted; the first Windows CI run surfaced it.
Use the GUI subsystem and attach to an existing parent console before argument parsing, preserving redirected standard handles. Add a binary subsystem and CLI output regression check to Windows CI.
…ole-popups-313
Stop native caption hit-testing at the tab scroller so Windows does not treat the tab controls as draggable titlebar space. Preserve horizontal scrolling and exercise the production titlebar wrapper in tab tests. Verified native tab clicks and add-menu actions.
Sync remaining Windows fixes from Zeron zeronsh#313
3 participants
Adds native Windows source builds: application storage and locking, agent discovery/launching, terminal cleanup, and desktop fixes. Includes Windows tests and shared Linux/macOS regression checks.
Source builds only; installers, managed updates, and background services remain follow-ups. The renderer changes come from Zui #7.
Local validation: 321 engine/harness tests passed, plus a Unix-path cross-compilation check. CI, a fresh release build, and GUI validation remain pending for this revision.
See the Windows guide for setup and limitations.