e2e: packaged-desktop target — the real bundle attaches to the supervised daemon - #1017
Conversation
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
executor-marketing | 6717599 | Commit Preview URL Branch Preview URL |
Jun 14 2026, 04:52 PM |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
executor-cloud | 6717599 | Jun 14 2026, 04:53 PM |
Cloudflare previewTorn down — the PR is closed. |
@executor-js/cli
@executor-js/config
@executor-js/execution
@executor-js/sdk
@executor-js/codemode-core
@executor-js/runtime-quickjs
@executor-js/plugin-file-secrets
@executor-js/plugin-graphql
@executor-js/plugin-keychain
@executor-js/plugin-mcp
@executor-js/plugin-onepassword
@executor-js/plugin-openapi
executor
commit: |
…ised daemon The supervised-daemon attach path (ensureSupervisedConnection → attachToSupervisedDaemon) and the bundled compiled sidecar only run when app.isPackaged is true. Dev electron skips that branch and always spawns its own desktop-sidecar, so the attach behaviour can't be proven against a dev launch — the prior dev-electron attach test could never satisfy its own assertion. Test the production artifact instead. - electron-builder.e2e.config.ts: unsigned bundle (no notarize/hardenedRuntime, `dir` target) so the real package builds without Apple credentials; the release config is untouched. - desktop-packaged vitest project + globalsetup: builds web UI → compiled sidecar → electron-vite → electron-builder, then publishes the launch exe and bundled sidecar path. - desktop-packaged/supervised-attach.test.ts: starts the bundle's own compiled executor-sidecar as the supervised daemon, launches the packaged app, and asserts it attached (manifest still names the daemon's pid/kind, console renders through the bearer-gated daemon) rather than spawning a sidecar. Skips honestly where no GUI display is reachable instead of hanging on launch. - Remove the superseded dev-electron attach test; fix a stale "Basic-auth" comment (the header is bearer).
82b86ae to
6717599
Compare
Greptile SummaryAdds a
Confidence Score: 3/5Safe to merge for macOS/Linux; the finally-block cleanup in the new test has a race that can mask failures or produce spurious errors on Windows. The core logic is solid and fills a genuine coverage gap. The concrete defect is in the finally block: daemon?.kill(SIGTERM) is fire-and-forget and rmSync(home) follows immediately. On Windows, TerminateProcess is async and file handles may still be held open when rmSync runs; a locked-handle error thrown from finally either surfaces as a false test failure or swallows the real failure message. e2e/desktop-packaged/supervised-attach.test.ts — the finally-block cleanup ordering Important Files Changed
Sequence DiagramsequenceDiagram
participant GS as globalsetup
participant EB as electron-builder
participant Test as supervised-attach.test
participant Daemon as executor-sidecar (supervised)
participant App as Packaged Electron App
GS->>EB: build unsigned bundle (dir target)
EB-->>GS: "exe + sidecar paths -> process.env"
Test->>Daemon: "spawn sidecarBin (EXECUTOR_SUPERVISED=1)"
Daemon-->>Test: stdout: EXECUTOR_READY
Daemon->>Daemon: "write server.json (kind=cli-daemon, pid=N)"
Test->>Test: waitForHttp (port ready)
Test->>Test: "read server.json -> daemonPid=N"
Test->>App: "_electron.launch(executablePath, HOME=home)"
Note over App: app.isPackaged=true -> ensureSupervisedConnection()
App->>Daemon: "reads server.json -> connects with bearer token"
App-->>Test: firstWindow()
Test->>Test: "waitFor Settings text -> attach proven via UI"
Test->>Test: "read server.json -> assert kind=cli-daemon, pid=N unchanged"
Note over Test,App: finally block
Test->>App: app.close()
Test->>Daemon: kill(SIGTERM)
Test->>Test: rmSync(home)
Reviews (1): Last reviewed commit: "e2e: packaged-desktop target — the real ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| } finally { | ||
| const page = app?.windows()[0]; | ||
| const video = page?.video(); | ||
| await app?.close().catch(() => {}); | ||
| const recordedPath = await video?.path().catch(() => undefined); | ||
| if (recordedPath && existsSync(recordedPath)) { | ||
| await promisify(execFile)("ffmpeg", [ | ||
| "-y", | ||
| "-i", | ||
| recordedPath, | ||
| "-c:v", | ||
| "libx264", | ||
| "-preset", | ||
| "veryfast", | ||
| "-crf", | ||
| "26", | ||
| "-pix_fmt", | ||
| "yuv420p", | ||
| "-movflags", | ||
| "+faststart", | ||
| join(runDir, "session.mp4"), | ||
| ]).catch(() => {}); | ||
| } | ||
| daemon?.kill("SIGTERM"); | ||
| rmSync(videoTmp, { recursive: true, force: true }); | ||
| rmSync(home, { recursive: true, force: true }); | ||
| } |
There was a problem hiding this comment.
home directory is deleted
daemon?.kill("SIGTERM") sends the signal but returns immediately; rmSync(home, ...) executes synchronously right after with no guarantee the daemon process has actually exited. On Windows, where kill("SIGTERM") calls TerminateProcess(), the OS may still hold file handles open during teardown — causing rmSync to throw EACCES or EBUSY since { force: true } only suppresses ENOENT, not locked-file errors. A throw from a finally block swallows the original test failure and can turn a passing test into a spurious failure.
| const daemonManifest = JSON.parse(readFileSync(manifestPath, "utf8")) as Manifest; | ||
| expect(daemonManifest.kind, "the compiled sidecar advertises itself as cli-daemon").toBe( | ||
| "cli-daemon", | ||
| ); | ||
| const daemonPid = daemonManifest.pid; |
There was a problem hiding this comment.
e2e/AGENTS.md says tests must be black-box, driving the product only through public surfaces (API, web UI, MCP, CLI) and must never poke internal files. Reading server.json from within home/.executor/server-control/ is reading a private implementation artifact, not a public surface. Is there a health-check endpoint, a CLI command like executor status --json, or any other public surface that exposes which pid/kind is currently serving — so the attachment proof could be expressed without depending on the manifest's internal path and schema?
Context Used: e2e/AGENTS.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! Is there a public CLI command or HTTP endpoint (e.g. GET /status) that reports the running daemon's pid and kind, so the attach assertion could be made through a public surface rather than reading server.json directly?
1 participant
What
Adds an e2e target that drives the real electron-builder bundle (not dev
electron), because the supervised-daemon attach path and the bundled compiled
sidecar only run when
app.isPackagedis true.In dev,
boot()skipsensureSupervisedConnectionand always spawns a freshdesktop-sidecar(rewriting the server manifest). So the attach behaviour —the always-on promise that the installed app connects to an OS-supervised
daemon instead of owning its own server — cannot be proven against a dev
launch. The previous dev-electron attach test could never satisfy its own
assertion; this replaces it with one against the production artifact.
Changes
apps/desktop/electron-builder.e2e.config.ts— unsigned packaging config(no notarize / hardenedRuntime,
dirtarget) so the real bundle buildswithout Apple/Windows signing credentials. The release config is untouched.
desktop-packagedvitest project + globalsetup — builds web UI → compiledsidecar → electron-vite → electron-builder, then publishes the launch
executable and the bundled sidecar path.
desktop-packaged/supervised-attach.test.ts— starts the bundle's owncompiled
executor-sidecaras the supervised daemon, launches the packagedapp at the same
HOME, and asserts it attached (the manifest still namesthe daemon's pid and
kind: cli-daemon; the bearer-gated console renders)rather than spawning its own sidecar.
("Basic-auth" → the header is a bearer).
Honesty note on coverage
Driving the packaged Electron window needs a real window-server session (Aqua on
macOS; an X/Wayland display on Linux). In a background/SSH/CI shell there is no
such session and Electron can't open a window, so the scenario skips with a
clear reason there instead of hanging — it runs for real on a logged-in
console or a guest under autologin/Xvfb. The supervised daemon's reboot-survival
(install → real machine reboot → auto-start → data intact) is separately proven
green on real VMs by the
cli-macos,cli-linux, andcli-windowstargets.