feat(cu): show that the machine is being driven, stop when it locks, and keep it awake while it runs - #1889
Conversation
4331b37 to
ab509dc
Compare
|
Repaired the wiring this PR was split away from. Everything in the first push was correct in isolation and unreachable from production: the status item was a local inside assembleDesktopTools that was never returned, so clearForSession, destroy and setStopHandler had no caller in the app, and screen-lock.ts's locked() had none either. Finding 1, the power-save blocker. This was a regression against main and the reason to fix the rest. tool-assembly maps onLiveChanged(true) to keepSystemAwake.hold('computer-use'), and the only two paths to onLiveChanged(false) were clearForSession and destroy. With neither called, one Computer Use action anywhere in the app's life started a prevent-app-suspension blocker held until the process exited, and turning the 保持系统唤醒 setting off could no longer stop it — correct for the new refcount, fatal without the release. On main the blocker tracked the setting exactly. A turn ending now retires the item, in both the normal path and the stream-error path, which is what gives the hold back; stop, archive and delete do the same, and quit destroys it as a backstop. Finding 2, the menu bar item. Same missing clearForSession, so the tray survived every session end. And the menu draws its rows enabled: stopHandler !== undefined, so with no installed handler the one place a person can stop a background run showed a permanently greyed-out row. The session IPC now installs the handler, pointed at the same stopSession function the in-app stop button runs, so the two cannot drift apart. The sessions:stop IPC handler became a thin wrapper over that function rather than owning the body. Finding 3, the lock guard. This one needed a producer, not just a caller. screen_locked existed as an outcome code in core, a session status, a block reason and a screenUnlocked release, with nothing on either side of it — locking the screen mid-run did not stop Computer Use from driving the machine, which also falsified the justification this PR added to keep-system-awake.ts for choosing prevent-app-suspension. The guard is now consulted in buildComputerUseTools ahead of the observation and action leases, refuses with screen_locked, and latches the session state that keeps observation closed until an explicit release. That placement is deliberate: the refusal is a session-state decision and the driver has no session state to latch it in, so the probe goes to the tool layer rather than into the executor's dispatch. The probe takes the session id, which the work branch's version did not — without it, a run whose very first call lands on a locked screen latches screen_locked while being unknown to the guard that owns screenUnlocked, and stays latched for the rest of its life. Second-order fix included: ensureSubscribed is now reached, so powerMonitor is subscribed and the unlock release has a producer. Finding 4, the sessions set. Same root cause, fixed by the same clearForSession calls on stop, archive and delete. On testing. The reason this class of defect survived is that the existing suites supply by hand exactly what production failed to supply — a test that calls setStopHandler itself proves nothing. So the new tests assert the call sites. createSessionStreamer is driven with a fake status item and a real turn-completion event, asserting that a finished turn and a dead turn both clear. The runtime gate is tested through the tool surface, not through a lock monitor: a monitor that answers correctly and is never asked is exactly the defect. The remaining call sites live in modules that import electron at the top level and cannot load under node --test, so those are pinned as source contracts, the way this repo already pins main-process invariants. Negative controls. With the runtime gate removed, 4 of 5 gate tests go red (the fifth asserts that an absent probe means no guard, and correctly stays green). With every desktop wiring edit stashed, all 7 wiring tests go red. Both restored and green after. What I could not verify locally. apps/desktop cannot be built on this machine — @astryxdesign/core and lucide-react 404 — so @maka/ui does not build and no Electron or Playwright run was possible. Under node --test the runtime computer-use and cua suites are 114/114, computer-use is 135/135, and the two new suites are 5/5 and 7/7. The remaining failures in the wider desktop and runtime suites are the local node v22.11 node:sqlite gap and the missing @astryxdesign/core, both present before this change. One scope note. Wiring the screen_locked producer required additive pass-through in packages/runtime and packages/computer-use: an optional screenLocked dep on buildComputerUseTools, threaded through selectComputerUseBackend and createComputerUseHost. Nothing else in either package changed, and with the dep absent behaviour is identical. It arguably belongs in a separate runtime PR, but the guard in this PR is inert without it, so splitting it would ship the same dead code again. |
|
Second-round review fixes. The cross-PR merge hazard first, because it is the one neither branch can catch alone. This PR and #1882 both rewrite the same expression, the Both PRs now assert their own wrappers. The new assertion here reads the production expression and requires both While doing that trial merge I found a second, quieter version of the same problem in this PR's own test. The status-item teardown assertion located its window with Now this branch's own findings.
The refused attempt is covered, by the And the guard leaked. It holds the ids it will release on unlock and had no turn-end caller at all: the session IPC cleared it on delete, stop and archive, while a turn that merely finished left its id in the set for the lifetime of the process. Two hundred turns in distinct sessions across a day leaves two hundred ids held and every unlock walking all of them. The status item was cleared on both of session-stream's turn-end paths and its sibling was not, which is the kind of asymmetry nobody notices until the set is what is being measured. Both paths clear both now. Last, the menu bar spoke English literals in an app that resolves a UI locale at boot, and whose picture-in-picture sibling labels the same two actions in Chinese, so the menu bar and the mirror disagreed about what language the product speaks on the same machine at the same moment. The three strings move into a UiCatalog, in the same shape the drag-to-grant card already uses for main-process copy. The locale is read on every menu rebuild rather than captured once, so changing the language in Settings shows up without a relaunch, and it resolves the persisted preference rather than only the system language. Every new assertion was negative-controlled by actually reverting the code, rebuilding and watching it go red, then restoring it and watching it go green. The only failures I saw anywhere were shell-env and Rive process-kill timeouts under a load average of 34, and both pass in isolation. |
a92c4d8 to
6d2f5c5
Compare
|
Rebased onto current main (34e7ddb) and added one assertion. Rebase. The first attempt hit two conflicts, both from #1953 landing the maka-cu backend selector. In select-backend.ts that PR restructured the options into a shared CommonSelection plus two per-executor types, and moved onTrace out of the shared block; screenLocked went into CommonSelection rather than being duplicated, because the refusal is decided above the backend and does not become true or false depending on which executor is selected. In sessions-ipc-main.ts main replaced the three emitSessionsChanged calls after a stop with the stoppedTurnBroadcasts loop, while this branch extracted that body into a named stopSession; the resolution keeps both. A second rebase onto the newer main was clean. CI. The typecheck and e2e reds this branch was carrying were both inherited. typecheck was knip reporting statusBadgeVariant in apps/desktop/src/renderer/settings/settings-status-badge.ts as an unused export, a file this branch does not touch; the identical failure was on main at the same commit. e2e_shard 1/2 was the plan-reminders edit-dialog focus assertion, also failing on main at the same commit, and it reproduced on a rerun rather than being flaky. Both are fixed upstream, so this is now rebased past them. The storybook check is skipped here — this branch changes nothing storybook reads. The assertion. The lock guard was wired and nothing asserted the middle of the wire. This file already pinned the top — tool assembly hands createComputerUseHost a screenLocked — and the runtime screen-lock suite pinned the bottom — buildComputerUseTools refuses when its screenLocked says so. Between them sit two forwarding hops nobody looked at: computer-use-host.ts handing the probe to selectComputerUseBackend, and select-backend.ts handing it to buildComputerUseTools. Deleting both left the whole repo green, with the guard completely dead in production: desktop, computer-use, runtime, this file and the screen-lock suite all passed. The new test drives the real chain instead of reading it — a real createComputerUseHost over a temp manifest and a stand-in binary, whose tool must refuse. Nothing is stubbed between the two ends, so a dropped hop turns the refusal into a dispatch. Negative-controlled by deleting each hop in turn and rebuilding; both produce: AssertionError [ERR_ASSERTION]: the call reached the driver instead of being refused, That is the production consequence stated exactly: with the hop gone, an observe on a locked machine goes past the missing guard and reaches the driver. Restoring either hop returns the file to green. selectComputerUseBackend is darwin-only and CI is Linux, so the test forces the platform for its duration rather than skipping. The neighbouring select-backend tests skip, which makes them pass on CI whether or not the thing they assert still exists — that is the failure mode being guarded, so it is not repeated here. Local runs on the final rebase: desktop 1440/1440, computer-use 244/244, presence-wiring and screen-lock 24/24. Runtime carries seven failures that are all environmental on this machine and byte-identical to main in the files that fail — three from an unapplied provider-utils patch in the symlinked tree, four from macOS resolving mkdtemp paths as /var while the executor realpaths to /private/var. Merge-order note. This PR and #1882 both extract the same ipcMain.handle('sessions:stop') body into an async function stopSession. A union of the two produces two declarations of it in the same scope and an unclosed ipcMain.handle, which does not parse; that hunk needs a hand-written resolution keeping one function whose body carries both branches' per-session clears, one setStopHandler per surface, and one handler. The overlay: expression in tool-assembly.ts also conflicts, and the correct resolution nests all three wrappers rather than choosing one. All three wrong resolutions still fail loudly after this rebase: taking the mirror's side reddens this branch's wiring test, taking this branch's side reddens the mirror's, and the naive union fails tsc with TS1117. |
…and keep it awake while it runs Three things a background Computer Use run needs and did not have. A menu bar item is the only place a person can see that the machine is being driven when the app is not in front, and the only place they can stop it. Not the cursor: the cursor is hidden whenever the window being driven is covered by something else, which is most of the time. It has no timer — it reflects the number of live sessions and its only action is to stop them. Locking the screen stops the run. Background operation is the point, but "the user is elsewhere on the same machine" and "the user locked the machine and left" are different situations and only the first is what this was built for. It matters for a second reason: with the screen locked, an observation comes back looking normal and containing only the menu bar, and the executor does not report an error. A whole afternoon went into a tree-shape theory before `CGSSessionScreenIsLocked` explained it. A run holds the machine awake for as long as it is driving something. The physical-input guard means a run works precisely while the user is not touching anything — which is exactly when idle sleep lands and kills it. The assertion is `prevent-app-suspension`, never `prevent-display-sleep`: the latter would also suppress the automatic lock, which contradicts the guard above. One holder name for all of Computer Use rather than one per session — the assertion is about whether the machine may suspend, and that does not get more true with a second session. The status item art is added to the packaged resources. Without it a packaged build resolves an empty NativeImage and Electron silently shows no icon at all.
Splitting this branch out by file theme kept every component and left behind the lifecycle that drives them. The status item was a local inside `assembleDesktopTools` that was never returned, so `clearForSession`, `destroy` and `setStopHandler` had no caller anywhere in the app, and `screen-lock.ts`'s `locked()` had none either. The worst of it was a regression against main. `onLiveChanged(true)` takes out a `prevent-app-suspension` blocker, and the only two paths to `onLiveChanged(false)` were the two methods nothing called — so one Computer Use action anywhere in the app's life started a blocker held until the process exited, and turning 保持系统唤醒 off could no longer stop it. On main the blocker tracked the setting exactly. A turn ending now retires the item, which is what gives the hold back; so do stop, archive and delete. The menu bar item draws its rows `enabled: stopHandler !== undefined`, so without an installed handler the one place a person can stop a background run showed them a greyed-out row. The session IPC installs it, pointed at the same function the in-app stop button runs. The lock guard needed a producer, not just a caller. `screen_locked` existed as an outcome code, a session status, a block reason and a `screenUnlocked` release with nothing on either side of it; the tool surface now asks before every action and every observation, refuses with that outcome, and latches the session state that keeps observation closed until the machine comes back. The probe takes the session id so a run whose first call lands on a locked screen is recorded as one to release — otherwise it would latch and never leave. Tests assert the call sites rather than the components. The components already had suites, and those suites passed the whole time by handing production's job to themselves.
`keepSystemAwake` is the whole point of the refcount rewrite and nothing tested that production supplies it. Deleting it from boot.ts — the only caller of `assembleDesktopTools` — leaves `tsc --noEmit` at exit 0, because the dep is optional, and leaves all 61 presence tests passing. What it costs at runtime is the feature: `onLiveChanged` fires into `undefined`, no `prevent-app-suspension` blocker is ever taken, and a background Computer Use run dies to idle sleep, which is the exact failure this branch exists to prevent. Optional is right for the dependency and wrong for the one production call site, so the call site is what is pinned now. The wiring test also asserts that the overlay hook the host is given is still wrapped by both presence hooks. That expression is a merge conflict by construction: every feature that wants to watch actions go past wraps the one before it, so resolving the conflict by taking one side compiles, type-checks, and leaves both branches' suites green while one branch's presence is disconnected. Measured: replacing it with the bare cursor hook leaves tsc at exit 0 and all 61 presence tests passing with a tray that never appears — so `onLiveChanged` never fires and the keep-awake hold is never taken. Every one of those tests builds the wrapper itself, which is why none of them notices. `withComputerUseScreenLock` was documented with a claim that is not true. It said a session earned a release by having tried to drive the machine, "including the attempt that was refused, since `onActionBegin` runs before the backend sees the action". A probe against the built runtime says otherwise: with `screenLocked` forced true, three calls came back `maka_computer failed: screen_locked`, the probe saw the session three times, and `onActionBegin` saw nothing at all. It has one call site, inside `runWithPresentation`, and the refusal returns several hundred lines above it. The refused attempt is covered — by the `screenLocked` callback in tool assembly, which records the session as it answers. What the wrapper covers is the other producer, and it is the only thing that does: the host's own lock probe has documented blind spots, so a dispatch can go through and come back with a `screen_locked` outcome from the executor, latching the session state from `applyTypedOutcomeState` long after `onActionBegin` ran. Nothing else would have recorded that session, and `screenUnlocked` is the only way out of the state it just entered. The comment now says that, the desktop test that repeated the false claim in its name says it too, and a new gate test in packages/runtime asks the real tool rather than trusting either of them. And the guard leaked. It holds the ids it will release on unlock, and it had no turn-end caller at all: the session IPC cleared it on delete, stop and archive, while a turn that merely finished left its id in the set for the lifetime of the process. Two hundred turns in distinct sessions across a day left two hundred ids held and every unlock walking all of them. The status item was cleared on both of session-stream's turn-end paths and its sibling was not, which is the kind of asymmetry nobody notices until the set is what is being measured. Both paths clear both now. Last, the menu bar spoke English literals in an app that resolves a UI locale at boot, and whose picture-in-picture sibling labels the same two actions in Chinese — so the menu bar and the mirror disagreed about what language the product speaks, on the same machine, at the same moment. The three strings move into a `UiCatalog`, in the same shape the drag-to-grant card already uses for main-process copy. The locale is read on every menu rebuild rather than captured once, so changing the language in Settings shows up without a relaunch, and it resolves the persisted preference rather than only the system language.
The wiring suite pinned the top of the chain — tool assembly hands `createComputerUseHost` a `screenLocked` — and the runtime's screen-lock suite pinned the bottom — `buildComputerUseTools` refuses when its `screenLocked` says so. Between them are two forwarding hops nothing looked at: `computer-use-host.ts` handing the probe to `selectComputerUseBackend`, and `select-backend.ts` handing it to `buildComputerUseTools`. Deleting both leaves the whole repo green. desktop, computer-use, runtime, this file and the screen-lock suite all pass with the guard completely dead in production, which is the same shape of defect the rest of this file exists to catch: every component correct, and nothing calling them. So the new test drives the real chain instead of reading it. A real `createComputerUseHost` over a temp manifest and a stand-in binary, and the tool it hands back must refuse. Nothing is stubbed between the two ends, so a dropped hop anywhere along it turns the refusal into a dispatch — which is exactly what the negative control shows: with either hop deleted the observe call goes past the missing guard and reaches the driver. `selectComputerUseBackend` is darwin-only and CI is Linux, so the platform is forced for the duration rather than skipped. The neighbouring select-backend tests skip, which makes them pass on CI whether or not the thing they assert is still there; that is the failure mode being guarded, so it is not repeated here.
6d2f5c5 to
8291065
Compare
|
Rebased onto origin/main at f0245c1, the commit that merged #1882. Force-pushed as 8291065. The conflicts were in tool-assembly.ts, boot.ts, sessions-ipc-main.ts, app-lifecycle.ts and session-stream.ts, and almost all of them were both sides adding to the same list: imports, destructured fields, per-session clear calls, teardown entries. Those were resolved as unions. Two places needed a decision rather than a union. The overlay expression. Both PRs wrap the same hook, and taking either side wholesale compiles clean while leaving the other feature's entire data path dead. The resolution nests all three, with the mirror innermost: overlay: withComputerUseStatusItem( The sessions:stop handler. Both PRs extract the same handler body into an async function stopSession so their own stop controls can be pointed at it, so a union would have produced two declarations of it and an unclosed ipcMain.handle. It is hand-written instead: one stopSession that clears the cursor, the mirror, the lock guard and the status item, then one setStopHandler for the mirror, one for the status item, and one ipcMain.handle delegating to it. What was verified, not assumed. Both wiring suites pass on the rebased tree: computer-use-pip-wiring and computer-use-presence-wiring, 20 tests, 0 failures. Both assertions were negative-controlled. Removing withComputerUsePip from the expression and rebuilding turns the pip suite red on "the overlay hook Computer Use is given is wrapped by the mirror", with the message that a mirror outside the overlay chain is never handed a frame. Removing the two presence wrappers instead turns the presence suite red on "the overlay hook Computer Use is given is wrapped by both presence hooks", with the message that no status item in the chain means no live-session count, so no menu bar indicator and no keep-awake hold. Each guard sees only its own absence, which is what makes them worth having here. The expression was restored afterwards and the working tree is byte-identical to the pushed commit. Nothing from #1882 was lost. There are no deletions in git diff origin/main...HEAD, and every file that only #1882 touched is untouched by this branch: pip-window.ts, pip-feed.ts, pip-motion.ts, pip-electron.ts, the overlay sources, all four pip test files, and the script and config changes. The only overlap is the five shared main-process files above. Tests: desktop main 1520 passed, 0 failed. @maka/computer-use 244 passed, 0 failed. @maka/runtime 2769 passed, 3 failed, and those three are the known pre-existing failures in model-factory-tool-call-index, whose stack lands inside a prebuilt @ai-sdk/provider-utils rather than in this repo. No new failures anywhere. biome check is clean on all five files touched, and scripts/check-console.mjs passes. |
1 participant
Three things a background Computer Use run needs and did not have.
A menu bar item
The only place a person can see that the machine is being driven when the app is not in front, and the only place they can stop it.
Not the cursor — the cursor is hidden whenever the window being driven is covered by something else, which is most of the time.
No timer: it reflects the number of live sessions, and its only action is to stop them.
Stop when the screen locks
Background operation is the point, but these are different situations and only the first is what this was built for:
There is a second reason, and it cost an afternoon. With the screen locked, an observation comes back looking normal and containing only the menu bar, and the executor reports no error. The tree looked structurally wrong, so the investigation went after the tree.
CGSSessionScreenIsLockedwas the answer.Keep the machine awake while a run is in progress
The physical-input guard means a run works precisely while the user is not touching anything — which is exactly when idle sleep lands and kills it.
The assertion is
prevent-app-suspension, neverprevent-display-sleep. The latter would also suppress the automatic lock, which contradicts the guard above: this feature stops itself when the screen locks rather than preventing the lock.One holder name for all of Computer Use rather than one per session — the assertion is about whether the machine may suspend, and that answer does not get more true with a second session.
Packaging
resources/statusis added to the packaged files. Without it a packaged build resolves an emptyNativeImageand Electron silently shows no icon at all — no error, no icon.Shape
Three new modules and two decorators over the existing overlay hook, so nothing that already works changes behaviour:
keepSystemAwakebecomes an optional dep onassembleDesktopTools; without it the run simply does not hold the machine awake.Verification
The three test files are 811 of the 1265 added lines. Local desktop build cannot run on this machine (upstream's
@astryxdesign/core@0.2.0does not resolve from this registry); the wiring typechecks clean, and the session events it uses —screenLocked/screenUnlocked— already exist onComputerUseToolSet.Touches
tool-assembly.ts, as does #1882. Whichever lands second I will rebase.