feat(cli): stream product frames to a wire debugger behind --debugger - #656
decrypto21 wants to merge 2 commits into
Conversation
0935dcc to
429df49
Compare
|
can you add some screenshot / recording how how it looks in action? |
429df49 to
6445b14
Compare
github-actions
Bot
added
the
documentation
|
CI Status: 19 required jobs green, 16 passed and 3 skipped by path filter. All job results
Commit |
|
@lore-bot-app review |
|
Reading the diff and checking what the record says. Back in a few minutes. |
![lore-bot-app[bot]](https://avatars.githubusercontent.com/u/14176906?s=60&v=4){kind=small}
There was a problem hiding this comment.
TL;DR
Adds --debugger / TRUAPI_DEBUGGER_URL to truapi-host, wrapping the CLI's ProductRuntimeFactory so every product runtime installs WsDebugSink. 4 blocking, 6 minor.
Summary
truapi-host gains a global --debugger <URL> switch (also read from TRUAPI_DEBUGGER_URL). dispatch resolves it into an Arc<dyn DebugSink> before the command runs, and tap_for_debugger wraps whichever ProductRuntimeFactory the frame server serves in a new DebugTappedRuntime, which calls set_debug_sink(ChannelId(product_id), sink) on every runtime the inner factory builds and forwards connection_reset. truapi-server is now depended on with features = ["ws-bridge"] so WsDebugSink exists. docs/design/wire-observability-debug-host.md §9 is rewritten to bind the CLI to the enablement rules, and the DebugSink contract comments in host_core.rs are updated to name the CLI as a second in-repo installer.
What the record says
- The sink this PR wires up landed in #295 explicitly "unsupplied (no consumers) until the web host dial-out lands", and #315 fixed the topology as host-dials-outward because nothing can dial into a worker or a device. This PR is the first native consumer, which is why the §7/§9 invariants written for "no host installs a sink" now need re-reading rather than re-stating.
- #604 gave the web host a second URL source (
VITE_TRUAPI_DEBUGGER) alongsidelocalStorage, both inside the sameimport.meta.env.DEVgate, plus amake debuggertarget so nobody wires it by hand. That commit is not in this branch:js/packages/truapi-host/src/web/create-worker-host-runtime.ts:239readslocalStorageonly, and the Makefile has nodebuggertarget. See concern 9. - #516 established that
truapi-hostships as a prebuilt release binary installed by a curl one-liner. That is the build the new switch is compiled into, ungated. See concern 3. - Owner for this area is
decrypto21(author of #295, #315, #536, #604);pgherveouowns the CLI distribution story.
Concerns
1. §9's "does not dial" arm is not implemented — main.rs:553
The doc edit at docs/design/wire-observability-debug-host.md:352 now reads "The web host and the headless CLI host both have such a path", binding the CLI to "one that does not dial says so once, one that does says where". Only the second half exists: connect_debugger runs solely when the switch is set. A CLI started without --debugger says nothing, which is the exact case §9 exists for (the debugger's own viewer holds a socket, so an empty board looks identical to a host nobody switched on). The web host implements both arms at create-worker-host-runtime.ts:300-329, including naming why it is off. The CLI also has no dev/production build split to claim the production exemption from.
2. The required report is invisible in interactive mode — main.rs:1050
tracing::info!("wire debugger: streaming frames to …") fires inside dispatch, before any host sets up the terminal UI. LogWriter (terminal_ui.rs:414-427) has no active UI at that point, so it writes to stderr, and TerminalUi then enters the alternate screen (terminal_ui.rs:1146). For the whole life of the host the user sees nothing about the debugger. Every other startup fact goes through terminal_ui::output_event(SystemEvent::…) (main.rs:1147, main.rs:1251) for this reason. A SystemEvent::DebuggerDialling { url, source } would satisfy §9 in both modes.
3. A released binary installs a sink from an environment variable, with no build gate — Cargo.toml:21, main.rs:100
Design doc §7 (:225-236) states the production guarantee precisely: the tap compiles into every build including the release and xcframework artifacts, and "what holds in production is that the tap is inert: no host installs a sink". The web host keeps that true with a hard import.meta.env.DEV gate specifically so "a stray localStorage key cannot turn the debugger on in prod" (create-worker-host-runtime.ts:242-247). The CLI's dial has no equivalent condition, and §7 was not updated. Concretely: with TRUAPI_DEBUGGER_URL exported, a prebuilt truapi-host from the installer (#516) streams every frame of a live signing host, fully decodable by the debugger (§7: no denylist, no reveal toggle), to whatever is listening on 127.0.0.1:9231. Either gate the dial (cfg(debug_assertions), or a cargo feature that release-cli.yml leaves off) or amend §7 to record the CLI as an exception and say why it is acceptable.
4. One channel id covers many concurrent connections — frame_server.rs:160
ChannelId is the product id, and the accept loop builds one runtime per accepted socket (frame_server.rs:635) with no connection cap. Request ids are minted per connection (p:1, p:2, …), which is exactly why §4 (doc:134-136) requires traces to be keyed on (channelId, requestId). With two peers under one CLI host (a browser page plus the bundled script runner, or a reload whose new socket overlaps the old) both id spaces land on one channel id and collide; the engine's recycle rule rotates generations instead of separating them. The web host does not hit this because each product runtime has its own worker. Consider suffixing the channel id per connection.
Minor
- Every subcommand pays for the switch —
main.rs:553.connect_debuggerruns before the command match, soupdate,identity-check,register-nameandalloc-checkspawn a WS writer and reconnect loop and print the dial report, and an invalid URL fails them at startup, although none of them emits a frame. With the env var exported that is the default path. Resolving inside the three arms that build a frame server keeps the fail-early property where it belongs. - Nothing tests that a tapped runtime emits —
frame_server.rs:1144.SilentSinkrecords nothing by construction andthe_debug_tap_builds_runtimes_through_the_wrapped_factoryasserts only that the call does not panic. The feature itself (frames reach the sink, with the rightChannelIdand direction) is uncovered.host_core.rs:1634has theRecordingDebugSinkshape to reuse. ws-bridgefor one type —Cargo.toml:21. The CLI never touchestruapi_server::ws_bridge; enabling the feature to reachnative_debugalso compiles the localhost bridge server and pulls tokio-tungstenite 0.21 (and itshttp0.2 tree) alongside the CLI's own 0.24. A separatedebug-sinkfeature overlib.rs:54andlib.rs:64avoids both in a binary that ships by installer.- The flag is undocumented. CLAUDE.md requires README updates with any code change.
--debuggerandTRUAPI_DEBUGGER_URLappear nowhere inrust/crates/truapi-host-cli/README.md(which documents--log-levelat :625) orSPEC.md, and §9 describes the CLI switch without naming either. - Two doc lines do not match this tree. The added §9 sentence "A dev build MAY also carry the value it was built with" describes the web host's build-time source from #604, which is not in this branch. Separately, while editing the file: line 111's "the native sink does not yet [stamp
observedAt], and buffers 4096 frames across reconnects, so native traces are subject to exactly that skew" is stale —native_debug.rs:283stamps it at enqueue — and that parenthetical is now a description of CLI traces. - The source heuristic defends against something clap prevents —
main.rs:1053. clap gives an explicit flag precedence over itsenv, so "a stale exported variable beating an explicit flag" cannot occur; the only ambiguous case is both holding the same string, which the code already calls harmless. The label is correct as written, but the justification in the comments, in the test atframe_server.rs:1226, and in the new §9 sentence describes a failure mode that does not exist. Alsodebugger_url_source_for_test(main.rs:1075) is a shim for a private function that could just bepub(crate); the test for it sits inframe_server.rs's test module rather than next to the function.
No instructions addressed to a reviewer appeared in the diff.
Questions for the author
- Is the CLI's dial meant to be ungated in release binaries, or should it follow the web host's DEV gate? Whichever way, §7's "no host installs a sink" paragraph needs to say so.
- Is this stacked on #604? The §9 build-carried-value rule has no implementation in this branch.
- What should the debugger show when two products connect to one CLI at once — are you relying on the engine's generation rotation, or should the channel id carry a connection discriminator?
Next: decide question 1, since concern 3 and the §7 text hang on it.
🤖 Reviewed by Lore (Parity knowledge base) · 57 agent turns · 460.8s · knowledge as of 2026-09-11
| // Resolved here, before any command binds a port or prints an event, so a | ||
| // bad `--debugger` URL fails on the argument rather than half-way through | ||
| // host startup. | ||
| let debugger = debugger.map(connect_debugger).transpose()?; |
There was a problem hiding this comment.
Owner for this area is decrypto21 (author of #295, #315, #536, #604); pgherveou owns the CLI distribution story.
Concerns
1. §9's "does not dial" arm is not implemented — main.rs:553
The doc edit at docs/design/wire-observability-debug-host.md:352 now reads "The web host and the headless CLI host both have such a path", binding the CLI to "one that does not dial says so once, one that does says where". Only the second half exists: connect_debugger runs solely when the switch is set. A CLI started without --debugger says nothing, which is the exact case §9 exists for (the debugger's own viewer holds a socket, so an empty board looks identical to a host nobody switched on). The web host implements both arms at create-worker-host-runtime.ts:300-329, including naming why it is off. The CLI also has no dev/production build split to claim the production exemption from.
2. The required report is invisible in interactive mode — main.rs:1050
tracing::info!("wire debugger: streaming frames to …") fires inside dispatch, before any host sets up the terminal UI. LogWriter (terminal_ui.rs:414-427) has no active UI at that point, so it writes to stderr, and TerminalUi then enters the alternate screen (terminal_ui.rs:1146). For the whole life of the host the user sees nothing about the debugger. Every other startup fact goes through terminal_ui::output_event(SystemEvent::…) (main.rs:1147, main.rs:1251) for this reason. A SystemEvent::DebuggerDialling { url, source } would satisfy §9 in both modes.
3. A released binary installs a sink from an environment variable, with no build gate — Cargo.toml:21, main.rs:100
Design doc §7 (:225-236) states the production guarantee precisely: the tap compiles into every build including the release and xcframework artifacts, and "what holds in production is that the tap is inert: no host installs a sink". The web host keeps that true with a hard import.meta.env.DEV gate specifical
| Ok(()) | ||
| } | ||
|
|
||
| /// A sink that records nothing: these tests are about the decorator's |
There was a problem hiding this comment.
Nothing tests that a tapped runtime emits — frame_server.rs:1144. SilentSink records nothing by construction and the_debug_tap_builds_runtimes_through_the_wrapped_factory asserts only that the call does not panic. The feature itself (frames reach the sink, with the right ChannelId and direction) is uncovered. host_core.rs:1634 has the RecordingDebugSink shape to reuse.
| truapi = { path = "../truapi" } | ||
| truapi-platform = { path = "../truapi-platform" } | ||
| truapi-server = { path = "../truapi-server" } | ||
| truapi-server = { path = "../truapi-server", features = ["ws-bridge"] } |
There was a problem hiding this comment.
ws-bridge for one type — Cargo.toml:21. The CLI never touches truapi_server::ws_bridge; enabling the feature to reach native_debug also compiles the localhost bridge server and pulls tokio-tungstenite 0.21 (and its http 0.2 tree) alongside the CLI's own 0.24. A separate debug-sink feature over lib.rs:54 and lib.rs:64 avoids both in a binary that ships by installer.
| } | ||
|
|
||
| /// Name the switch that supplied `url`. | ||
| /// |
There was a problem hiding this comment.
The source heuristic defends against something clap prevents — main.rs:1053. clap gives an explicit flag precedence over its env, so "a stale exported variable beating an explicit flag" cannot occur; the only ambiguous case is both holding the same string, which the code already calls harmless. The label is correct as written, but the justification in the comments, in the test at frame_server.rs:1226, and in the new §9 sentence describes a failure mode that does not exist. Also debugger_url_source_for_test (main.rs:1075) is a shim for a private function that could just be pub(crate); the test for it sits in frame_server.rs's test module rather than next to the function.
No instructions addressed to a reviewer appeared in the diff.
Questions for the author
6445b14 to
42aee85
Compare
iOS simulator previewBuilt from gh run download 35355613605 --name simulator-preview-42aee8574
unzip polkadot-app-*.app.zip
xcrun simctl install booted polkadot-app.app
xcrun simctl launch booted io.parity.polkadotapp.developOr download it in a browser, which arrives as a zip wrapping An arm64 simulator slice, so it needs an Apple Silicon Mac and does not |
github-actions
Bot
added
the
javascript
b2392cb to
42aee85
Compare
4 participants
Adds a
--debuggerflag so the headlesstruapi-hostCLI streams its product frames to the wire debugger.Why: the debugger only ever sees the browser host.
wasm.rsis the sole non-testset_debug_sinkcaller, so every frame the CLI serves is unobservable, includingtruapi-host devand the wholee2e-*family.WsDebugSinkalready exists and is tested, with no caller outside its own tests.How:
frame_server.rsgains aDebugTappedRuntimethat wraps aProductRuntimeFactoryand installs a sink on every runtime it hands out;main.rsresolves the switch into that sink and wraps the factory each host role uses.global = trueputs it onpairing-host,signing-hostanddev, andTRUAPI_DEBUGGER_URLsets the same thing, which is how a harness attaches without a script change.connection_reset. The trait default would leave product connections alive across a session switch that exists to invalidate them, and nothing in the accept loop reports the omission, so it has a test that fails when the override is removed.wss://target aborts on the argument rather than half-way through startup. The rest emit no frames and open no sink. A loopback URL succeeds with nothing listening, since the sink dials lazily and reconnects.clapresolves an explicit flag over the variable, so the variable is the source only when it is the only thing set.<product-id>#<n>. Request ids are minted per connection and §4 keys a trace on(channelId, requestId), so concurrent peers under one host would otherwise collide.Implements §9's native enablement path. §9 of the design doc spelled out only the browser rule and said no host wired the native sink up. It now states the native rule too, including that the report goes out on the host's lifecycle-event surface rather than as a log line — a
tracingline lands on a stderr the alternate screen then covers.§7 records the CLI as an exception to "no host installs a sink in production": gating it would leave the prebuilt binary the one artifact that cannot debug. Three conditions replace the gate — the dial is explicitly asked for, the target must be loopback and is rejected when read, and the host announces it on startup.
Two changes beyond the flag:
truapi-server'sdebug-sinkfeature, which is whereWsDebugSinklives;ws-bridgeimplies it. The CLI compiles neither the localhost bridge server norrand.host_core.rsname both non-test installers. The lock-poisoning argument holds on the property they share: every caller builds a freshSinkTransportperproduct_runtime()and installs at most once on it, sopreviousis alwaysNone.Verified against a live host.
scripts/battery.sh --signing-hostwithTRUAPI_DEBUGGER_URLset and no other change reaches the debugger as 157 ops over 477 frames and 1,002,726 bytes,malformed: 0,truncated: 0. The run was against a build carrying #848, which is what makes the envelope's codec match the debugger's; without it the board reportscodecMismatchand withholds payload decoding, so the two want landing together. Sorting that capture by duration putsresourceAllocation.requestat 52.26s against a 664ms average, which is the kind of tail this exists to make visible. A/framedrill-down returns"kind": "decoded". fmt clean, clippy at zero, 247 crate tests pass.That run also rewrites
explorer/diagnosis-reports/spa/signing-host-cli.md. This branch leaves it alone: its new failures are the run's ephemeral identity, not this change.