Repository navigation
feat(desktop,web,server,node): virtual displays, and the helper in both assistants - #465
Conversation
The menu-bar door proved undiscoverable, so the macOS display helper gets a standing rail section in each assistant window: the enablement steps the menu item never showed, one button opening the helper's own panel, and a link to the setup guide. The section is the rail's only platform fact and drops off where no helper exists; a deep link asking for it elsewhere answers with that machine's own journey.
…roviders A node that cannot serve previews now reads gold when the reason is the Web previews & screens switch, which an owner can turn on, with a link to the guide; an offline or outdated node stays muted because no setting fixes that. The launch form, a preset, and the node's provider list drop a virtual display provider the selected machine's OS cannot run, using one rule in subshell-protocol rather than three copies of it.
…'s output A slow or loaded Linux node answered launch-with-screen with a bare 'not answering within 5000ms' and then reaped the pane dir that held Xvfb.log and x11vnc.log, leaving nothing to diagnose. The refusal now names which of the three readiness gates was still shut (Xvfb socket, rfb bind, listener ownership), the provider logs both children's tails to the daemon journal before the teardown, and the budget is 10s since the launch command has 60s on the wire anyway.
The create failure rendered in one form-level slot under 'Save as preset' no matter which control caused it. screenLaunchRemedy now always names the node's refusal word (bare form included) and the switch preflight carries preview-node-off, so the dashboard can match them (isDisplayLaunchRefusal) and route the refusal into the Virtual display field's own error slot, wired into the checkbox. Everything else keeps the form-level line.
verify() passed -R a bare requirement string; codesign treats an unprefixed -R value as a FILE to load from, so every 'Open Display Helper settings' press died with '<the whole requirement>: No such file or directory' and 'invalid requirement specification' before the helper could install or open. The value must start with = to be a literal spec. The module is macOS-only, which is how it survived review uncompiled on Linux CI; the new test pins the prefix in both debug and release shapes.
codesign shows CDHash= only from -dvvv; at -dvv it exits 0 without the line, so the same-or-older-copy digest comparison answered 'Couldn't verify the display helper's code identity' on every settings press that found an installed helper. First install escaped it only because the missing-copy branch never reaches the hash. macOS-only module; verified on the M5.
…ndow HelperConsent's default argument was UserDefaults(suiteName: bundleID) with the app's OWN bundle id: Foundation rejects that (it logs 'does not make sense and will not work') and returns nil, so the force-unwrap raised EXC_BREAKPOINT inside DisplayHelperApp.init. open exited 0, so the desktop app reported success while the helper died instantly and no panel appeared. The default is .standard, which for the bundled helper already IS its own domain; consent still reaches the daemon over the helper's socket. The test now exercises the DEFAULT initializer against the standard domain, which the old code failed.
build_support::build() rewrote src-tauri/resources/Subshell Display Helper.app every time the build script ran, and tauri dev watches src-tauri - so one edit under native/display-helper put bun run dev:desktop-client into an endless 'File ... changed. Rebuilding application...' loop. The bundle is now built and signed in OUT_DIR, synced into resources only when bytes differ, and gated by a content fingerprint kept outside the watched tree. The script's own tests run from a shim test target (cargo never tests build scripts).
…refused The missing list could not distinguish 'no helper at all' from 'helper enabled, permissions on, BetterDisplay lookup empty', and that distinction decides whether the fault is the helper, BetterDisplay, or the caller.
The launch refusal is raised by the runtime wrapper around the provider, so the allocation-site log never fired. Both gates now say what the helper actually answered.
A pane whose app started a port nobody saved, or that has a screen or a browser watch sitting there, looked identical to an empty one. The count comes from the picker's own reads (same query keys), and the rule is a pure function: discovered ports minus Saved services rows, plus the screen and the browser watch when offered. The trigger's aria-label carries the number.
…name the exits The settings panel was raw AppKit: default fonts on a system background, a title that did not say the product, and no way to get out of the window except closing it. It now draws the dashboard's own palette (Theme.swift converts the oklch tokens from styles.css rather than retyping hexes, and the helper test pins them), uses the heading/label/body/detail roles, calls itself Subshell virtual display helper, and offers Minimize helper next to Quit helper in the panel and the menu bar. The menu bar icon now says when the machine is being looked at: a live capture tints it gold and adds a dot, the tooltip names the viewer count, and the panel carries the same line in gold. Runtime announces capture start and end so the badge tracks the truth instead of the 2s poll.
Pointer control posts real cursor events onto the virtual display, so when that screen goes away mid-session (pane closed, display discarded, helper quit) the local user is left with a pointer parked off every visible display. Measured on the M5, where it cost them the cursor entirely. The helper now restores a stranded pointer to the built-in display when a capture session ends, when it quits, and when the provider discards the screen it was on. The stranding test is pure, so it runs in the helper suite.
420pt wide instead of 620, one measurement column, standard push buttons (the old bezelColor-on-regularSquare controls rendered as mismatched tall boxes), the two permission states aligned in a grid, secondary actions and the exits grouped at the foot, and prose trimmed to what a person reads.
The colours were copied into each surface's stylesheet and a checker compared the copies, which is how the assistant once shipped six near-miss values under Dreamframe's names. They now live in packages/assistant/tokens.css and the three CSS surfaces @import it. packages/, not apps/server/, is deliberate: the macOS display helper's panel (Apache, under native/) must be able to import the same file, and the AGPL boundary runs through apps/server/. Type roles stay literal in each @theme block because var() indirection does not work there; lint:design names any surface that re-declares a shared value differently, and the client's parity test now proves the import, the roles, and the absence of a local shadow. Docs and the auto-loaded rule tell people where a token goes now.
The native AppKit layout kept arriving at the Mac looking wrong: boxed controls of mismatched height, rows clipped by the window, and a mirrored copy of the palette nothing checked. It is now a local page in packages/helper-panel, built on packages/assistant/tokens.css, and the helper hosts it in a web view behind the same HelperSettings API. The page holds no authority. It loads with loadFileURL scoped to its own directory, from a non-persistent data store, cannot open a window or navigate off-bundle, and every act it posts is re-checked in Swift. What follows from that: - lint:design scans the package under the assistant rules, and a test pins the registration, because a scan root that silently matches nothing is the same hole as no scan root. - lint:prose sweeps its string literals: consent copy is shipped text. - The Swift Theme keeps only the three colours and one type role the native chrome still paints, and AccessReport loses the panel line the page now owns. - build_support.rs stages dist into Contents/Resources/panel, before the signature, and prunes hashed assets a previous build left behind. A release refuses to bundle without the page; a debug build carries a placeholder naming the command, since the CI Rust lanes have no node_modules to build it from. - Both desktop apps take @internal/helper-panel as a devDependency, so turbo builds it before either bundles the helper.
The SDK declares the delegate methods unisolated and the properties they hand us main-actor, so the first cut of the bridge read `message.body` and `navigationAction.request` off main and produced two warnings. A Task where the work may wait a runloop turn, and `assumeIsolated` where WebKit has to have the answer synchronously.
The web view opened white. The page was fine; its two assets were not. `loadFileURL` gives the document an opaque origin, so vite's `<script type="module" crossorigin>` and `<link rel="stylesheet" crossorigin>` are CORS requests from a null origin that WebKit refuses: the document arrived, nothing it needed did, and the empty main element showed the web view's own ground. The build now folds the stylesheet and the bundle into index.html (cssCodeSplit off, iife output), and refuses to produce anything else: a missing tag, a leftover ./assets/ reference, or module-only syntax in a classic script all fail the build, because each of those is the same white window by another route. `dist/` holds one 8 KB file, which is also one signed file to check.
Measured at the panel's own 420px in a browser, not by eye at a desk: - Buttons now wear the assistant's `Button` variants rather than my own shapes: min-height 36 at px-4, rounded-md, strong at body size, the sunk plum gradient for the primary. The primary had been a flat --primary, which is the token the palette keeps bright for links and glows. - The primary was stretching to the card's full width (a flex child's default), which reads as a progress bar. It sizes to its label now. - Two dividers became one: sections bring only their `FormSectionTitle` rule, and the bar top is the window's single hairline. The section borders were mine. - The bar's right group ran to x=435 in a 420px document and its last button was cut off. The bar wraps between the two groups instead of inside their labels, since `whitespace-nowrap` on a button is the rule. - The refusal moved above the card, where the assistant's frame puts its problem line: it is why the screen still looks like this, so it has to be read before the failed button is pressed again. - A granted permission offers no "Allow…" button. A prompt that cannot appear looks like a hang. - The login item got its own row and its sentence moved there, so `stateLine` states the state and one line says where to fix approval. - `.panel` no longer sets `min-height: 100vh`, which made the height the page reports to the window depend on the window. The five `--button-primary-*` values move to packages/assistant/tokens.css, which is where the design system says to add a colour once: the SPA and both desktop assistants declared the same five, and the panel needs them too.
The bottom row mixed a plum text link with two buttons that have no outline until you hover them, so it read as three unrelated things that wandered in from other screens. macOS puts a window's help at the bottom left and its exits at the bottom right, and that is where they sit now, all four controls in the card wearing the same shape except the one primary. The window went from 420 to 520, which is what the row of labels needs and what stops a status line from wrapping beside the control that fixes it. The number lives in the page's stylesheet and the window follows the box the document reports, so it is declared once; the container's old width constraint would have fought that and clipped the page.
The state word carries the decision now: "Not allowed" in gold, the role the design system reserves for "the owner can fix this here", and "Allowed" in the page's own ink, because a settled fact should not compete for attention with the row that still needs a click. The button says "Allow" and nothing more, since the row already names the permission; its accessible label keeps the whole phrase, where two buttons called "Allow" would be indistinguishable in a list of controls. Label and value also stay together on the left, with only the control pushed to the right edge. Space-between with three children had put "Allowed" a hand-span away from the permission it answers for.
The bar item is an icon, so the menu it opens now says "Subshell virtual display helper" at the top: a person who clicked it without hovering the tooltip had nothing telling them which helper they were in. Minimize helper was greyed out in front of an open window because the menu is built once, at launch, before show() has ever run, and nothing rebuilt it when the panel appeared. HelperSettings now reports visibility changes (show, minimize, and the red button's willClose, observed rather than delegated so the SDK's actor isolation on NSWindowDelegate is not something we have to assume), and the app rebuilds the menu from them.
"Starts at sign-in with macOS." is already true, so it wears the page's own ink; "Needs approval in System Settings under Login Items." still waits on the person, so it wears gold. That is the same split the permission rows use, and one row of each colour was the tell that the login item had been left reading as metadata. The row's own API took the shape that makes this legible: a label, a state value, and an optional control, so a settled fact simply has no control instead of the caller reaching for a spare element.
Whether the helper comes back after a sign-in is macOS's launch bookkeeping, not part of the person's consent to be captured, so it reads better under its own heading than as the last row of Access. The heading also lets the enable note drop "and starts at sign-in": one fact, said once. The section appears only while the helper is enabled, because a disabled helper has registered nothing to approve and a heading over an empty row is how a panel starts lying about itself.
theogravity
left a comment
There was a problem hiding this comment.
Nine reviewers each covered one slice of the 327-file diff, and every finding below was
re-verified against the head commit by hand. No MAJOR. 19 MINOR, plus non-blocking nits listed
above. The strongest candidates for a fix before merge are the preview Location regression, the
pipe-writer guard that cannot reach its threshold, the missing cursor rescue on a lost capture,
the input budget closing a non-offending socket, the resync verdict reading the previous
connection's base, and the committed docs hero rendering /tmp/subshell-pr465-review as a pane's
working directory.
Two notes on method. PR #463 was merged into this branch rather than into main, so this diff
legitimately carries the pane self-heal work and its changesets; nothing is duplicated on
release. And Swift here is not unchecked: native/display-helper/test.sh compiles every source
but main.swift under -warnings-as-errors and adds a -typecheck pass that covers it, though
the file early-exits off Darwin, so the macOS lane is the only thing that reads it.
Non-blocking nits
packages/helper-panel/src/main.ts:202: every state push rebuilds the open quit dialog and
refocuses Cancel, so a viewer joining mid-Tab makes the red Confirm hard to reach by keyboard.
Build once, update the body text, focus only on creation.packages/helper-panel/src/strings.ts:104:stateLinehas no all-granted wording, so a fully
armed helper reads "Capture and input still require both macOS permissions below." directly
above two rows saying Allowed. Every other line in the file distinguishes its states.apps/server/web/src/hooks/use-pane-diagnostics.ts:27:useState(false)where its model,
use-pane-copy-mode, uses a lazy initializer, so the HUD renders closed for one frame on mount.apps/server/api/src/db/types/index.ts:36:previewProxySettingsis declared inline with
uncommented properties and a plainstringprotocol, where every other table has a
*.db-types.tsfile, per-property JSDoc and a union.apps/server/api/src/api/subshells/views-open.route.ts:144: the settings route gates on
web-previewsonly, so a display-only node cannot read or save preview prefs, which the create
path explicitly supports.apps/client/desktop/ui/src/__tests__/tokens-match-spa.test.ts:29: the ten colour roles are
restated instead of importingCOLOR_ROLESfromscripts/design-tokens.ts, so the two guards
can drift when a role is added.packages/helper-panel/src/...NITs andnative/display-helper/main.swift:172: the comment now
under-describes what checks that file, sincetest.sh:25added a-typecheckpass that
includesmain.swift.e2e/verify-preview-compat.ts: writespreview-website-mobile.pngand
preview-settings-mobile.png, referenced by no page.scripts/design-tokens.ts:513:readSurfaceCssis exported and called by nothing, so it can
drift from the real merge order unnoticed.
|
Review findings whose anchor line is unchanged context in this PR's diff, so they could not be posted inline on the review. Each heading gives
|
|
Addressed the review in ccd6580, with schema/comment cleanup in 964b265 and a test-type correction in ce015b4. I replied to each inline finding separately. Follow-up on the non-inline findings and review-summary items:
The remaining nits are addressed too:
Input-budget concern: retained the existing retryable 1013 close on global exhaustion. This is backpressure, not an assertion that the requesting socket caused the load. Refused reliable input is not acknowledged and can be resent after reconnect; in-flight writes remain charged. Silently refusing without closing would strand retries, while accepting or queueing without a bound would undo the memory protection. Legacy unsequenced clients still lack a lossless retry guarantee; this is a residual availability limitation, not something safely fixed by removing admission limits. Also addressed the CI failures: docs screenshot inventory is consistent (184 docs tests pass); connect-flow login clears cross-port shared cookies (5 scenarios pass); the mobile scroll fixture waits for an executed sentinel rather than tty echo (targeted setup/scroll checks pass). The exact original mobile timing failure was not reproduced locally, so no product scrolling behavior was changed. Independent follow-up review is clean. Focused regression suites, native Mac checks, docs production build/export, and all pre-push checks passed. Fresh GitHub CI and CodeQL still need to report on the pushed revision; local passes are not a claim that those remote checks have finished. |
theogravity
left a comment
There was a problem hiding this comment.
Second review pass
Nine reviewers covered the original 327-file diff; four then verified the fix delta (188b97827 to ce015b458, 52 files, +604/-159) finding by finding. 17 of the 18 legitimate MINORs are fixed correctly, most with regression tests that genuinely fail against the pre-fix head, which I checked case by case rather than taking on trust. #19 is fixed but incomplete. The docs hero image regressed.
Two comments in this pass are not replies to existing threads: the two WebSocket items are findings I dropped from my own posting manifest, so they never reached you. That was my transmission error, not a declined finding. The fish finding I already retracted on thread: your reading is right, fish has accepted the key=value command prefix since 3.1, and env would have put the credential in argv.
Nits, non-blocking
e2e/capture-product-demo.ts:14usesmkdtempSync(path.join(tmpdir(), "demo-")), soapps/docs/public/screenshots/harness-subagents.pngrenders/tmp/demo-tbQki4/desktop/task-boardin the pane header and the codex status line. Same class as the hero, one file outside this delta.apps/node/agent/src/commands/report.ts:60still documents strikes as "Consecutive", which is what the old code meant and what the fix made untrue. Strikes now persist across arbitrary quiet intervals, so the docstring is describing the bug that was removed.- The two new guard tests in
commands-launch.test.ts(:1080and its neighbour) skip thestopWatcher(ctx, S1)cleanup every sibling in the describe block calls. Harmless at a 10 s period, an abandoned-tick leak into another test the moment that period shrinks. packages/helper-panel/src/__tests__/main.test.ts:53,expect(Number.isFinite(resize.width)).toBe(true), cannot fail pre-guard: happy-dom'sgetBoundingClientRectreturns 0 rather than NaN, somain.ts:349'sNumber.isFinitebranch is never exercised. Shape coverage, not branch coverage.packages/helper-panel/src/main.ts:186: Confirm never clearsconfirmingQuit, so a press with nowebkitbridge leaves the dialog painted. Pre-existing, and now recoverable through thesubshellPanelShownpath you added.native/display-helper/Runtime.swift:146is correctly placed, and I checked the thing that looked risky:DisplayCaptureis@MainActor, the sample handler queue is.main, and thelostclosure is non-Sendablecreated in a main-actor method, so callingrestoreStrandedCursor()is compile-checked and the macOS lane builds it under-warnings-as-errors. NoHelperTestsassertion could fail on it though, so it still needs the on-device check: attach a viewer, kill BetterDisplay, confirm the pointer comes back without a helper restart.
Also still pending, not defects
CodeQL has not re-analysed ce015b458, so the scriptString hardening in response-transform.ts (which closes a real breakout of the inline-module re-serialization, and which I verified independently of the reply claiming it) is not yet externally confirmed. The macOS Rust and native lane was still running when this was written.
|
Second-pass fixes are in 7975bdd. Replied individually to all five findings; independent review found no actionable defect in this fix wave. The summary nits are addressed as follows:
Still pending: the live Mac pointer-recovery check. The available Mac node reports missing Screen Recording and Accessibility permissions for Subshell Display Helper. I did not bypass those permissions or claim that attaching a viewer and killing BetterDisplay was tested. The earlier native compilation/typecheck/runtime checks passed, but they do not prove the physical pointer-recovery behavior. This remains a manual validation item once those permissions are granted. Validation for this wave: 37 covering WebSocket tests, 42 node tests, 50 helper-panel tests, 45 design-token tests, 12 Rust helper tests, and 184 docs tests passed, along with relevant types/build/lint and pre-push checks. The redeclaration tests also failed as expected when either production guard was independently removed. All mutations were restored before the final passes. GitHub marks CodeQL alert 19 fixed. CI must now check the newly pushed revision; the local validation and review are not a claim that the new remote run has finished. |
Non-blocking follow-ups from the third verification passNothing here is a merge gate. Each was checked against 1. So a second run on a machine that already ran this once can satisfy that condition from the previous run's test file, exit the loop early, and record a run in which the agent never wrote tests. The script rewrites 2. Two committed demo assets still predate the fixed roots. 3. The changeset omits one user-visible change. I also reopened the |
Four nodes holding retained retries for offline panes can fill the 4,096-frame fleet budget, and the next keystroke on any other node paid for it: the arriving socket was closed with 1013, and closed again on each retry. The close was also load-bearing, so refusing the frame instead would have dropped the character without an ack and let newer ids overtake it. When only the shared node or fleet queue is full, give up the oldest retry still waiting to re-fire and admit the arrival. A connection that breaches its own per-frame or per-socket bound is still closed, and the old last resort stands when there is nothing left to reclaim.
Two gaps in the coverage I asked for on review, both about arms that nothing can currently fail on. The node and socket byte ceilings are the same 16 MiB, so a single socket filling a node trips both and proves neither: these charges are spread over four sockets so only the node's own byte cap can refuse the next frame. And forced viewer recovery was only exercised with a negotiated `sid`, so a regression that dropped the viewer-id fallback's holds under the wrong key stayed green.
…d to write one The demo working directory is a fixed path shared between runs, and the completion loop counts an existing tasks.test.js as proof the agent wrote tests. A run where it skips that step then ends early and ships a green bun test of work nobody did. The scratch root is removed at exit, so nothing else was clearing it.
The count I expected was wrong, not the code. holdFailedInput ends by kicking its drain, because an append is itself a re-fire trigger, so the first attempt is already on the wire before recovery runs. Compare against that count instead of zero: what matters is that a node coming back finds nothing left to re-fire into a socket that is gone. Also drop a comma operator Biome flagged.
|
The remaining mobile-workspace.mp4 and website-preview.mp4 captures are refreshed in 4129db4, along with their WebP posters. Both use neutral fixed demo paths; footage was captured from real agent work and inspected through the full sequence. The phone capture now uses a 22px terminal font, and the stacked website player grows to 340px. At a 390px viewport this makes its text about19px instead of12px. Checked the actual embedded player for readable output, intact controls, full playback, pause/resume, reduced-motion behavior, and save-data opt-in. The desktop website-preview workflow remains readable at its displayed size. Also fixed an iframe-load race in the capture script by waiting for visible app content before inspecting its frame URL. Sample tasks/tests passed; both silent H.264 clips use15fps and fast-start metadata. Website production build, export-asset checks, player test, types/lint and pre-push checks passed. |
|
One item from the follow-ups list is not closed, and I'd rather say so now than let it read as done.
Not a leak. Nothing personal is in that path and it is an anonymous temp directory. It is the one asset that still contradicts the fixed-root rule the other clips now follow, and Separately, for the record on the WS work: the server suite is green on this head, including the four budget cases, so the reclaim change is verified by CI rather than by my local probe alone. Two lanes, |
Changes
Make virtual displays accessible from both desktop assistants and fix web previews that lose styles, images, or development-server requests behind Subshell's proxy. Add optional preview configuration saved per user, node, host, protocol, and port. New previews use those settings while retaining the opaque browser sandbox and target authorization.
Compatibility: node protocol 25 requires servers and connecting nodes to be upgraded together. Explicit CLI
--keystill exposes that argument to process inspection; copied setup commands can remain in shell history. macOS permission grants remain machine-specific.Documentation and screenshots
Web previews & screens now has its own sidebar section: prerequisites, web app previews, browser watching, Linux virtual displays, macOS virtual displays, and agent workflows. Platform installation, configuration, usage, and screenshots live together. Node pages link to those procedures; existing routes and bookmarks are preserved, and app, desktop, helper, and website help links point to the new pages.
Update
/concepts/securityfor preview response rewriting and settings, live sharing revocation, terminal input bounds, enrollment credentials, and helper signature verification. Retain the SSH bulletin analysis and documented trust boundaries.Refresh the assistant and helper screenshots from the shipped UI with sample states, including a running helper with both required permissions granted. The preview guides also include the website, configuration dialog, and virtual-display screenshots.
Validation