Skip to content

fix(desktop): keep harnesses available when login-shell capture is slow - #459

Merged
lukemaj merged 2 commits into
mainfrom
fix/458-login-shell-capture
Oct 18, 2026
Merged

lukemaj merged 2 commits into
mainfrom
fix/458-login-shell-capture

Conversation

@lukemaj

@lukemaj lukemaj commented Oct 18, 2026

Copy link
Copy Markdown
Contributor

What changed: Desktop login-shell capture now waits up to 12 seconds and reports a prepared environment only when that capture returns a PATH.

Why: A warm interactive zsh takes about 3.5 seconds. The old 5 second cap killed probes that had not finished, and POSIX startup still reported the environment as prepared, so the server skipped its own PATH capture and installed harnesses disappeared.

So what: This stays a draft. Independent exact-head review is still required, and the parent dispatches that review. Do not merge, install, or restart Cavallo from this PR.

Problem

On macOS, DesktopShellEnvironment.installIntoProcess runs zsh -ilc and a marker-delimited capture of PATH and related variables. The capture timeout was 5 seconds. A probe that had not exited was killed, partial output was discarded, and the POSIX branch still returned prepared true. The server then skipped fixPath. The same timeout and unconditional prepared flag are still on upstream d218d5b397d5c23098b697a44ae282b094d68ac9. Upstream issue 10534 is open. Open PR 11275 changes job-control flags for a different issue and was not ported.

Expected behavior: a login shell that returns PATH within a bounded window keeps that PATH and may count as prepared. A slow or empty capture must not claim success. launchctl getenv PATH can still fill the process PATH and must leave server recovery enabled.

Change

LOGIN_SHELL_CAPTURE_TIMEOUT_MS is 12000 and is shared by the desktop POSIX probe and readEnvironmentFromLoginShell, which the server uses for its second capture. Windows probes stay at 5 seconds. installPosixEnvironment returns true only when a candidate's captured environment contains PATH. Launchctl merge, caching, PATH order, and the WSL handoff are unchanged. There is no retry loop and no per-provider path pin.

The fork feature login-shell-capture records the five carried upstream files.

Scope and approval

Issue 458 is the requested repair. It is one existing-owner bug: slow or failed login-shell capture was treated as a successful preparation.

Verification

RED, before the production edit, on the public desktop and shared seams:

  • The shared login-shell probe still used timeout: 5000 while the test expected 12000.
  • A launchctl-only PATH and an empty login-shell capture both reported prepared true.
  • A PATH that arrived after 6 seconds was discarded and the desktop probe fell through to launchctl.

GREEN, on the sources in 8e129c71a666fdc29e40c2dd6a530f06b1d13fac (tests at 21:21 Warsaw, 2026-10-18, immediately before that commit; the commit did not change those source lines):

./node_modules/.bin/vp test run apps/desktop/src/shell/DesktopShellEnvironment.test.ts packages/shared/src/shell.test.ts apps/server/src/os-jank.test.ts

3 files, 80 tests passed, duration 1.30s.

./node_modules/.bin/vp run --filter @t3tools/shared --filter @t3tools/desktop --filter t3 typecheck exited 0 on these shell sources before the fast-forward onto c1d3960a1d. That fast-forward did not edit the shell owner. vp lint on the five source and test files reported 0 warnings and 0 errors. scripts/fork-check.sh and node scripts/fork-features.mjs check both printed OK on this commit.

Not run: repo-wide vp check, the full test suite, and full CI. CI on this draft is the remaining automated gate.

Read-only Cavallo measurement of the real multi-variable zsh -ilc capture completed in 2.547s with empty stderr. zsh -lc true completed in 0.031s and is the wrong semantics because it skips interactive startup. Live packaged provider discovery was not run. No Cavallo, Rocky, or bestia process was restarted.

Website impact: none. No public site page describes this startup path.

Version: not bumped. This repository has no VERSION file, and Chromeria builds are identified by t3codeCommitHash.

Elon record

Requirements and who asked: Gavin asked, in Issue 458, for a robust login-shell probe so installed harnesses stay available when capture is slow or fails, without another Cavallo restart.

Deleted: The job-control port from open PR 11275, ignoring stderr, per-provider path pins, recursive retries, counting launchctl alone as full preparation, and the claim that d275565 alone caused the timeout.

Bottleneck: The 5 second cap against a warm interactive zsh of about 3.5 seconds, plus a prepared flag that skips the server's second capture.

Checked myself: Desktop, shared, and server source; upstream issue 10534 and the full open PR 11275 diff; the read-only Cavallo timings above; focused tests, typecheck, lint, and both fork checks. Live provider discovery was not run.

Model: grok-4.7
Harness: T3 Code / Grok

Refs #458

A warm interactive zsh takes about 3.5s, and the 5s capture cap killed
probes that had not finished. POSIX installIntoProcess still reported
prepared, so the server skipped its own PATH capture.

Share a 12s POSIX capture window and report prepared only when a login
shell returned PATH. A launchctl PATH still applies and leaves server
recovery enabled. Windows and WSL behavior stay as they were.

Model: grok-4.7
Harness: T3 Code / Grok
The feature entry used a compare URL until the draft pull request existed.

Model: grok-4.7
Harness: T3 Code / Grok
@github-actions

Copy link
Copy Markdown
Contributor

Thread transfer impact

✅ Thread transfer remains within every enforced ceiling.

Provider Metric Main baseline This PR Impact PR ceiling
Codex Total thread wire 5.0 KiB 5.0 KiB 0 B (0.0%) 6.8 KiB ✅
Codex Thread snapshot wire 3.8 KiB 3.8 KiB 0 B (0.0%) 4.9 KiB ✅
Codex Live turn WebSocket wire 1.2 KiB 1.2 KiB 0 B (0.0%) 2.0 KiB ✅
Codex Live turn WebSocket decoded 20.9 KiB 20.9 KiB 0 B (0.0%) 29.3 KiB ✅
Codex Live turn messages 2 2 0 (0.0%) 8 ✅
Claude Total thread wire 5.0 KiB 5.0 KiB 0 B (0.0%) 6.8 KiB ✅
Claude Thread snapshot wire 3.8 KiB 3.8 KiB 0 B (0.0%) 4.9 KiB ✅
Claude Live turn WebSocket wire 1.2 KiB 1.2 KiB 0 B (0.0%) 2.0 KiB ✅
Claude Live turn WebSocket decoded 21.2 KiB 21.2 KiB 0 B (0.0%) 29.3 KiB ✅
Claude Live turn messages 2 2 0 (0.0%) 8 ✅

Baseline: c1d3960 · PR result: e9fcf54 · Source CI: success

Scenario and decoded snapshot size

10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.

  • Codex decoded thread snapshot: 108.5 KiB
  • Claude decoded thread snapshot: 108.8 KiB

Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed.

@lukemaj lukemaj left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent review: approve, no blocking findings

Reviewed exact head e9fcf546876a6683f3c283495a045c271617f479 against base c1d3960a1d. The touched files are unchanged on current main (def07e920a), and GitHub reports the PR mergeable. Reviewer: Claude Opus 5.5 (Claude Code), read-only, in a temporary worktree.

Verdict: the change is correct and fixes both defects in #458. It does not prove the Cavallo outcome; the live gate below does that.

What I checked

  • Prepared flag. installPosixEnvironment now returns true only when a login-shell candidate returned PATH. A launchctl-only PATH, an empty capture, a timeout and a spawn failure all return false. That value reaches the server through buildWindowsPrimaryConfig, which builds every non-WSL primary including darwin and linux, then through shellEnvironmentPrepared and fixPath, so hydratePosixPath runs again. This now matches the Windows branch.
  • Timeout. The 12s window is shared by the desktop probe and the server's readEnvironmentFromLoginShell. Windows PowerShell probes keep 5s. Launchctl stays at 2s. Upstream pingdotgg/t3code#10534 shows the same failure: about 1.1s warm, more than 5s at cold start, and a 6105ms span (5000ms plus the 1s kill grace). That matches Cavallo's 6042ms trace. A capture that misses the window on a cold desktop launch now gets a second, warm attempt in the server.
  • Regressions. Linux: a failed capture now also re-enables server hydration, and a successful one is unchanged. Windows and WSL: no behavior change. The WSL config does not use the flag. Caching, PATH order, launchctl merge and copying the other variables are unchanged.
  • Tests. At this head, vp test run apps/desktop/src/shell/DesktopShellEnvironment.test.ts packages/shared/src/shell.test.ts apps/server/src/os-jank.test.ts passed: 3 files, 80 tests. RED is real: with base DesktopShellEnvironment.ts and shell.ts restored and the PR tests kept, 5 tests fail: the shared timeout: 5000, the launchctl fallback and the empty capture both reporting prepared true, the 6s PATH falling through to launchctl, and the spawn-failure case. The 6s test uses TestClock against Effect.timeoutOption, so it exercises the real bound. The new os-jank test passes on base too: it checks the existing fixPath behavior the change relies on, and the PR does not claim it as RED. fork-check.sh and fork-features.mjs check print OK.

Non-blocking notes

  1. The Cavallo acceptance gate cannot tell this fix from the mitigation. Gavin's launchctl setenv PATH (runbook 00144d3) is still in place until logout. With it, even the old build finds Codex and Claude: a timed-out probe falls back to launchctl. Seeing all three harnesses after a GUI launch is therefore necessary but not enough. Also confirm in desktop.trace.ndjson that the login-shell runCommandOutput span completed with no DesktopShellEnvironmentCommandTimeoutError, and that installIntoProcess returned prepared true. Removing the mitigation first is the user's decision, not part of this PR.
  2. Startup cost when the shell truly hangs. installIntoProcess runs before whenReady, so no window appears until it finishes. A capture that finishes between 5s and 12s now delays the window by that long, which is the purpose of the change. If the shell hangs, the wait before the window grows from about 8s to about 15s on macOS (12s, 1s kill grace, 2s launchctl). The server then makes a synchronous execFileSync attempt of up to 12s per candidate before listening. In a d275 build that attempt was skipped. On Linux, with up to 3 distinct candidates, both waits multiply. Both stay bounded, and a capture that finishes now counts as success, so I accept this tradeoff. A later improvement could be one server attempt with a shorter cap after the desktop has already timed out.
  3. Test gap. No test shows a capture slower than 12s still being cut off with prepared false, and the new shell.ts comment makes that claim ("still stops a hang"). A test with a delay of about 13s would pin both sides of the bound.
  4. Nit. The five new lines were added at the top of scripts/fork-upstream-edits.txt instead of next to the other apps/desktop entries. The file was not sorted before either, and the fork checks pass.

Not run: live packaged provider discovery, repo-wide checks, any build or install. CI on this head is green.

@lukemaj
lukemaj marked this pull request as ready for review October 18, 2026 20:17
@lukemaj
lukemaj merged commit c95b531 into main Oct 18, 2026
27 checks passed
@lukemaj
lukemaj deleted the fix/458-login-shell-capture branch October 18, 2026 20:17
lukemaj added a commit that referenced this pull request Oct 18, 2026
Render thin and consent research pages through an isolated local public-network browser, preserving origin restrictions, cache continuation, provenance and bounded subpage traversal.

Closes #442. Refs #423.

Proof: 124 focused tests including 13 installed-browser cases, drained effects and no sleeps; scoped typecheck/lint/exports/fork checks. CI39163100867 and Fork39163100918 passed on19a987a5051ae99c10e0f2657ec922f05e898b8c.
Independent review: Claude Opus5.5 R5 PASS on exacthead, comment6254854977.
User-approved ordered landing coordinated on411. Preserves incoming #459 atc95b531803. No deployment/restart/install/cache clearing or version bump. Optional limits remain on447.

Authored with Codex through T3 Code; reviewed with Claude Opus5.5 through Claude Code in T3 Code.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:M vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant