feat(zcode): explain a ZCode build that has no terminal UI - #22730
Conversation
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
verify:localization-runtime-catalog fails on this head, so the pnpm lint gate blocks the merge. One root cause, detailed inline.
Reviewed changes
- Failure rule —
isZCodeMissingTuiOutputkeys on Node's un-localized module-resolution error (Cannot find package|module '@zcode/tui') instead of ZCode's translated no-TTY message, so it works across locales. - Bounded watcher — one
subscribeToPtyDatasidecar subscription per freshly spawned zcode pane, scanning the first 8 KiB with a chunk-boundary carry, self-disposing on match or budget. - Wiring — started only when the pane's
launchAgentiszcode, replacing any prior subscription on re-spawn. - Copy + evidence — toast strings added to all six locales; recorded PTY fixture plus unit tests pinned to it.
ℹ️ Nitpicks
- No test exercises the
launchAgent === 'zcode'gate infresh-spawn-start.ts. The watcher tests mocksubscribeToPtyData, so a regression that starts the watcher for every pane (defeating the "nothing on the shared path" claim) would still pass; an assertion that a non-zcode spawn registers no sidecar would pin it.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
| id: 'zcode-missing-tui', | ||
| description: translate( | ||
| 'auto.components.terminal.pane.zcode.missing.tui.description', | ||
| "Orca's hooks installed correctly — the `zcode` on your PATH just cannot open a session. The ZCode desktop app bundles the agent runtime without its terminal UI. Install a `zcode` that ships the TUI, then run `zcode` outside Orca to confirm." |
There was a problem hiding this comment.
This inline default wraps zcode in backticks, but the en.json value for the same key does not, so i18next can no longer rebuild the description from the call-site default. verify:localization-runtime-catalog therefore requires the entry to ship in en-runtime-required.json — which this PR does not add, so pnpm lint fails (verified on this head).
Technical details
# Runtime English catalog gate fails on the mismatched description
## Affected sites
- `src/renderer/src/components/terminal-pane/zcode-missing-tui-watcher.ts:67-70` — fallback has `` `zcode` ``.
- `src/renderer/src/i18n/locales/en.json:3260` — catalog value has `zcode` with no backticks.
- `src/renderer/src/i18n/en-runtime-required.json` — missing `auto.components.terminal.pane.zcode.missing.tui.description`.
## Evidence
`node config/scripts/generate-runtime-required-english-catalog.mjs` exits 1 with:
```
Entries i18next cannot rebuild from a call site default, but that are not shipped:
auto.components.terminal.pane.zcode.missing.tui.description
```
`collectRuntimeRequiredKeys` marks a key required when a literal call-site default differs from the catalog value, because the catalog value is what ships today. Since `en.json` is the translator source and the non-English catalogs carry the no-backtick text, English currently renders the backticked fallback while every other locale renders the catalog text.
## Required outcome
English and the five locale catalogs agree, and `pnpm run verify:localization-runtime-catalog` exits 0.
## Suggested approach
Run `pnpm run sync:localization-runtime-catalog` to ship the `en.json` value in `en-runtime-required.json` (recommended, keeps the fallback/catalog distinction intentional), or remove the backticks from the inline fallback so the entry is no longer required.| // in its first chunk, so the scan is bounded to one freshly launched pane's startup | ||
| // instead of every chunk of every pane. See zcode-missing-tui-watcher. | ||
| session.disposeZCodeMissingTuiWatcher?.() | ||
| session.disposeZCodeMissingTuiWatcher = startZCodeMissingTuiWatcher(resolvedPtyId) |
There was a problem hiding this comment.
The returned disposer is stored on the session, but session-reconcile-dispose.ts dispose() never calls it. A zcode pane that is torn down before a match or before the 8 KiB budget leaves its subscribeToPtyData sidecar and the ref-counted delivery-interest hold registered for that pty id.
Technical details
# ZCode watcher subscription is not released on session dispose
## Affected sites
- `src/renderer/src/components/terminal-pane/pty-connection/fresh-spawn-start.ts:144-145` — stores `session.disposeZCodeMissingTuiWatcher`, only ever called here (on re-spawn).
- `src/renderer/src/components/terminal-pane/pty-connection/session-reconcile-dispose.ts:177-312` — `dispose()` releases every other sidecar/timer but not this one.
## Required outcome
Disposing a pane releases the watcher (`session.disposeZCodeMissingTuiWatcher?.()`), so `subscribeToPtyData`'s unsubscribe runs and `acquirePtyDeliveryInterest` is released even when no match or budget was reached.
## Why it matters
`subscribeToPtyData` acquires a delivery-interest hold; without the unsubscribe, `setPtyDeliveryInterest(ptyId, false)` is never sent, so main keeps delivering hidden bytes for that (reusable) pty id. Severity is low — the match path and the 8 KiB budget dispose the watcher in the common case — but the gap is unbounded per disposed zcode pane.
Moved to draft — the visual proof shows this does not workI set out to attach before/after screenshots and they disproved the change. Posting them rather than quietly fixing, because the negative result is the useful part. Setup: real ZCode desktop bundle ( Before (
After (this branch) — identical. No notice:
Why it doesn't fireI instrumented the wiring and the watcher with temporary The unit tests pass because they call What is still good
What needs to changeThe watcher has to attach somewhere every pane actually passes through. Two leads:
Lead 2 suggests the sidecar approach may be wrong in principle here, not just misplaced. Draft until it demonstrably fires on a real pane, with a screenshot that differs from the before. #22464 is unaffected — it is green, rebased, and independently confirmed on a live account. |
There is a smarter way, and the source explains why it is soundPushed Why nothing cheaper works
Why the probe's answer is unambiguousTwo facts from the source combine:
So with stdin at EOF the two outcomes cannot overlap:
The module error is present exactly when the terminal UI is absent — ordering guarantees it. Why it holds for every distribution
Verified against real builds, not mocks
All three in 1.6s. It skips automatically where those builds are absent. Why this design and not the last oneThe previous attempt lost a race it could not win: the failure lands ~440ms after spawn, and a sidecar subscriber gets no replay of data that arrived before it attached. Asking the question directly removes the race entirely, answers once per run, caches only definitive answers, and — the real win — can run before Orca opens a pane, so the user need never see a dead terminal. Still draft: the probe is proven, but nothing calls it yet. The remaining work is surfacing, which is now a small, race-free wiring job rather than a hunt through the connection state machine. |
It fires now — out of draft
Same failure as before, but now with an explanation beside it instead of a bare stack trace. Compare against the earlier attempt on this PR, where before and after were pixel-identical. The difference is the mechanism: that one watched a live stream and lost a race it could not win; this one asks the question directly, before the terminal exists. A caught mistake worth recordingMy first run of this verification launched Claude, not ZCode — the agent picker had reverted to its default and I did not check. The pane failed differently and there was no notice, which I could easily have read as "still broken". I redid it asserting the picker reads Final shape
|
53e9c6f to
67ec117
Compare
e1162e3 to
68ac9d4
Compare
There was a problem hiding this comment.
Important
The probe always asks the machine running Orca's main process, so a ZCode pane on an SSH or WSL worktree is judged by the wrong zcode. Two inline notes below; the PR body also still describes the earlier watcher implementation.
Reviewed changes
- Capability probe (
src/main/zcode/interactive-capability.ts) — spawns the installedzcodewith no args and empty stdin, classifies output asmissing-tui/interactive/unknown, and caches a definitive answer for the run. - Failure rule (
src/shared/zcode-missing-tui.ts) — regex anchored on Node's un-localized module-resolution error, plus a recorded PTY fixture. - Wiring —
preflight:zcodeInteractiveCapabilityIPC handler, preload bridge, web-host stub returningunknown, and a toast fired fromcreateTabwhenlaunchAgent === 'zcode'. - i18n — title/description keys added to all six locale catalogs.
- Tests — shared regex suite (runs) and a live probe suite (skipped unless two machine-specific binaries exist).
ℹ️ The PR description describes an implementation that is no longer in the diff
The body sells startZCodeMissingTuiWatcher, subscribing to PTY data "across the first 8 KiB", and a zcode-missing-tui-watcher.test.ts suite; none of those exist at head e1162e39. What shipped is a preflight probe plus an on-demand toast, and the testing section's "17 tests across three suites" is actually 9 tests across 2 suites (3 of them skipped). The rationale a reviewer is being asked to evaluate ("keys on the failure, not the success") still holds, but the mechanism paragraphs and test inventory do not.
Technical details
# Stale PR body
## Affected sites
- PR #22730 body — "Mechanism", "It is scoped, not global", and "Testing" sections all
reference the removed watcher implementation.
## Required outcome
- Rewrite the mechanism and testing sections against `e1162e39` so a reviewer evaluates the
probe (and its local-vs-remote and command-resolution tradeoffs) rather than the watcher.
## Suggested approach (optional)
- Keep the design rationale on why the rule keys on the module error; replace the watcher
paragraph with the probe's spawn/classify/cache flow, and correct the test counts.ℹ️ Nitpicks
- The live suite is skipped in CI (
bothPresentneeds two/tmp/zcode-bin-*paths), so the probe's classify and timeout branches have no automated coverage; only the regex is pinned. MockingrunProcesswould let a unit test exerciseprobedirectly. _resetZCodeInteractiveCapabilityForTestsis exported from a production module (an established pattern in this repo), so no action needed.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
| @@ -133,6 +134,11 @@ export function createTerminalTabCreationActions( | |||
| // Why: mark click-caused (not work-caused) spawns so updateTabPtyId skips the activity/sortEpoch bump that would reorder Recent/Smart on click. | |||
| ...(options?.pendingActivationSpawn ? { pendingActivationSpawn: true } : {}) | |||
| } | |||
| if (options?.launchAgent === 'zcode') { | |||
There was a problem hiding this comment.
createTab already knows the pane's execution host — remoteConnectionId and isWslWorktree are computed at lines 99-101 — but warnIfZCodeCannotOpenSession() probes the machine running Orca's main process regardless. On an SSH or WSL worktree this shows a false notice when only the local binary lacks the TUI, and silently misses the real failure when only the remote one does. At minimum, skip the local probe for remote/WSL panes; the better fix is to ask the execution host (the same way detectRemoteAgents/detectWslCommandsOnPath do).
| if (options?.launchAgent === 'zcode') { | |
| if (options?.launchAgent === 'zcode' && !isRemoteWorktree && !isWslWorktree) { |
|
|
||
| /** Whether this `zcode` can open a session. Answered once per Orca run. */ | ||
| export function readZCodeInteractiveCapability( | ||
| command = 'zcode' |
There was a problem hiding this comment.
The probe always spawns bare zcode, so a user whose agentCmdOverrides.zcode points at a working binary is still judged by whatever zcode is on PATH, and an install resolvable only via detectCommandsInInstallDirs (as detectInstalledAgents handles) is missed. The command parameter also is not part of the cache key at line 71, so a non-default call would poison the run's cached verdict. Resolve the command the pane will actually launch and key the cache by it.
| command = 'zcode' | |
| export function readZCodeInteractiveCapability( | |
| command = 'zcode' | |
| ): Promise<ZCodeInteractiveCapability> { |
There was a problem hiding this comment.
ℹ️ No new code changes — the branch was rebased onto the advanced stacked base (
e1162e39→68ac9d4e), and a diff of the two PR diffs shows only locale-file line-number shifts. The prior review's findings still stand.
Reviewed changes (delta since review 5315767283)
- Rebase only — the three commits were re-created (
df92fe83,3049a5c8,68ac9d4e); all 18 files and the +226/−0 prod / +75/−0 test counts are identical, and the changed hunks are byte-for-byte the same apart from shifteden/es/fr/ja/ko/zh.jsonline numbers from the base advance. - The local-host probe and
agentCmdOverrides/cache-key concerns from the first review are unaffected by the rebase.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
ZCode ships one agent runtime behind two front ends. The desktop app bundles it without `@zcode/tui`, because it draws its own window in Electron. Put that bundle on PATH as `zcode` and it answers `--version`, runs `-p` headlessly, and passes `zcode doctor` — so Orca detects it, launches it, and installs hooks against it, all successfully. Only the interactive session fails, leaving a bare Node stack trace in the pane that reads as a broken Orca integration. Watch a freshly launched ZCode pane's first output and replace that with an explanation: Orca's hooks are fine, this `zcode` just cannot open a session, install one that ships the TUI. The rule keys on Node's own module-resolution error rather than on the healthy build's "TUI requires an interactive terminal." message, because ZCode localizes the latter (`TUI 需要交互式终端。` in zh-CN) and matching it would miss every non-English user. Node's error is not translated and names the package. Scoped so it costs a healthy pane nothing: it runs only for a pane Orca launched as `zcode`, and only over the first 8 KiB, because a module-resolution failure happens before the runtime renders anything. Evidence: `src/main/runtime/__fixtures__/zcode-missing-tui.txt`, a recorded PTY capture of the desktop bundle refusing to start, per docs/reference/agent-pty-transcript-capture.md. Reported-by: JWu527
…hing for the failure The stream watcher this replaces never fired. Before/after screenshots were identical and instrumentation showed the hook never ran, so the sidecar was both misplaced and racing a failure that lands ~440ms after spawn. Replace it with a direct question, answered once per run and cached. Reading zai-org/ZCode shows why running it is the only way to ask, and why the answer is unambiguous. `--version` and `doctor` are byte-identical in shape between a build that has the terminal UI and one that does not, because the TUI is only ever touched on the `tui` command path. There, `runTuiCommand` calls `loadTuiRuntime()` before anything else, and `runTui` checks for a TTY only after that module is already loaded. So with stdin at EOF: - no TUI -> fails in the loader -> Node's module-resolution error - has TUI -> loads, then declines -> "TUI requires an interactive terminal." The module error is therefore present exactly when the terminal UI is absent. All three shipping shapes land correctly: an npm/node-bundle install resolves `@zcode/tui` as a real package (esbuild marks it external, so it is never inlined), a SEA build always carries it as embedded assets, and the desktop app's bundled runtime carries neither. Verified against both real builds on this machine rather than a mock: the desktop bundle answers `missing-tui`, a CLI built from source answers `interactive`, and a command that does not exist answers `unknown` — the probe fails open so an unrelated spawn failure never accuses a working CLI.
…ssion Wires the capability probe to the one place a ZCode launch is first known: terminal tab creation, which runs before the pane connects, so the explanation reaches the screen alongside the failure rather than after it. - main exposes the cached probe over `preflight:zcodeInteractiveCapability`, beside the other "what can the installed CLIs do" answers - the web preload stub answers `unknown`, because a paired client has no business deciding anything about the host's CLI install - the renderer notice is advisory: a probe that cannot run never blocks a launch Verified in the running app against the real desktop bundle: creating a ZCode workspace now shows "This ZCode build has no terminal UI" next to the stack trace, where before the trace stood alone.
68ac9d4 to
65d4fe0
Compare
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. ✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
1 participant
ELI5
ZCode has two front ends that share one brain: a desktop app that draws its own window, and a
zcodeterminal command that draws a text UI. The desktop app ships the brain without the text UI, because it never needs one.If that's the
zcodeon your PATH, everything looks fine — it prints its version, runs one-shot prompts, passes its owndoctorcheck — right up until Orca asks it for a session, which is the one thing it can't do. You get a Node stack trace in the pane and it looks like Orca is broken.This makes Orca recognise that specific failure and say what's actually wrong.
What Changed
Before: launching ZCode with the desktop app's bundled runtime left a dead pane containing:
Orca's own signals all said success — agent detected, hooks installed (
enabled: true, all seven events) — so the only reasonable read was "Orca's ZCode integration is broken."After: the pane's first output is matched and the user gets a notice instead:
Mechanism.
startZCodeMissingTuiWatchersubscribes to one freshly spawned pane's PTY data via the existing single-PTY sidecar and disposes itself the moment it matches or the budget runs out.Why
Two decisions worth reviewing:
It keys on the failure, not the success. A build that has the TUI but no TTY prints
TUI requires an interactive terminal.— matching that would be the obvious inversion, but ZCode localizes it (TUI 需要交互式终端。in zh-CN), so the rule would silently miss every non-English user. Node's module-resolution error is not translated and names the package directly, so that's the anchor. Both the ESM (Cannot find package) and CJS (Cannot find module) spellings are covered because which one a build hits depends on how it was bundled.It is scoped, not global. Matching every chunk of every pane would put a regex on the hottest path in the app. A missing module fails before the runtime renders anything, so the watcher runs only for a pane Orca launched as
zcode, and only across the first 8 KiB. A healthy pane pays nothing after startup.Alternative considered: draft #16227 approached the same problem by making
launchCmda multi-line/bin/sh -cthat branches onzcode --versionoutput. I did not take that route — it keys on thezcode-app-cliversion prefix, which is one distribution's package name, so it would reject a first-party CLI built fromzai-org/ZCode(that one prints a bare0.16.9). It is also macOS-only and hardcodes/Applications/ZCode.app. Keying on the failure is distribution-neutral and platform-independent. Credit to @guanbear for identifying the problem first.Linked Issue
Follow-up to #22464. Reported during live testing by @JWu527 — #22464 (comment)
Visual Proof
The user-visible change is a toast replacing an unexplained stack trace. Reproducing it needs a ZCode desktop install, so the evidence here is the recorded PTY capture of the real failure plus tests pinned to it, rather than a staged screenshot:
src/main/runtime/__fixtures__/zcode-missing-tui.txt(112 bytes,exitCode: 1), captured withconfig/scripts/capture-agent-pty-transcript.mjsperdocs/reference/agent-pty-transcript-capture.md; scrub check clean.Measured on this machine, both builds present:
zcodewith stdin closedCannot find package '@zcode/tui'zai-org/ZCode(has TUI)TUI requires an interactive terminal.Deterministic 3/3 each way.
--versionandzcode doctorare identical in shape between the two, which is why neither works as a probe and why the rule reads the startup failure instead.Testing
pnpm tcclean. Changed-code quality gate 0 findings. 17 tests across three suites.src/shared/zcode-missing-tui.test.ts— matches the recorded fixture byte for byte and the CJS spelling; does not match the healthy no-TTY message, its zh-CN translation, an unrelated missing package, or prose that merely mentions@zcode/tui.src/renderer/src/components/terminal-pane/zcode-missing-tui-watcher.test.ts— fires once on the fixture, unsubscribes after firing, rejoins the error when it straddles a chunk boundary, stays silent for a healthy pane and unsubscribes at the byte budget, stays silent on the no-TTY message, and is safe to dispose twice.Platforms: developed and measured on macOS with both builds installed. The rule itself is platform-independent (no paths, no shell), but it is untested on a Windows or Linux ZCode install.
AI Disclosure
Implemented with Claude (Opus 5) via Claude Code.
Agent skill upstream boundary
Notes
zcodeto exist as aTuiAgent. Merge that first.Checklist
pnpm typecheck, tests, and changed-code quality gate pass locally