Conversation
There was a problem hiding this comment.
Pull request overview
This PR consolidates and hardens session/terminal lifecycles across the toolchain by moving several previously synchronous/fragmented flows (QEMU run loop, U-Boot shell I/O, board-session client/server lifecycle) onto async primitives, while introducing a new ostool-server crate and a bundled /admin/ web UI for board pool + TFTP/session administration.
Changes:
- Refactor
ostool run qemuI/O handling to use the shared async terminal core and ensure clean shutdown on success/exit/timeout. - Introduce
ostool-server(Axum-based) with session/board/TFTP management plus embedded Vite/Vue admin UI. - Convert
uboot-shelltests/examples + YMODEM transfer implementation to async I/O (Tokio + futures-io compatibility).
Reviewed changes
Copilot reviewed 84 out of 88 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| uboot-shell/tests/test.rs | Move QEMU serial tests to Tokio async. |
| uboot-shell/src/ymodem.rs | Convert YMODEM sender to async traits. |
| uboot-shell/examples/loady.rs | Async example using Tokio TCP stream. |
| uboot-shell/examples/loady_real.rs | Async serial example via tokio-serial. |
| uboot-shell/examples/env_var.rs | Async env-var example via Tokio. |
| uboot-shell/Cargo.toml | Add async deps (tokio, futures, compat). |
| ostool/src/utils.rs | Expose Command::into_std() for Tokio spawn. |
| ostool/src/run/shell_init.rs | Remove thread-based delayed send helper. |
| ostool/src/run/qemu.rs | Replace sync stream loop with AsyncTerminal pipeline. |
| ostool/src/run/output_matcher.rs | Remove final_result() helper (logic moved). |
| ostool/src/run/mod.rs | Export shell_init for board config normalization. |
| ostool/src/lib.rs | Expose new board module. |
| ostool/src/build/mod.rs | Refactor runner args into structs/boxed enum variant. |
| ostool/src/board/terminal.rs | New: websocket-backed async serial terminal. |
| ostool/src/board/session.rs | New: board session acquisition + heartbeat. |
| ostool/src/board/serial_stream.rs | New: websocket <-> duplex stream bridge for runners. |
| ostool/src/board/mod.rs | New: CLI entrypoints for board listing/running. |
| ostool/src/board/config.rs | New: .board.toml config load/normalize. |
| ostool/Cargo.toml | Add deps for board client + websocket/serial. |
| ostool-server/webui/vite.config.ts | New Vite config (base /admin/). |
| ostool-server/webui/tsconfig.json | New TS config for Vue app. |
| ostool-server/webui/src/vite-env.d.ts | Vite typings reference. |
| ostool-server/webui/src/views/TftpView.vue | Admin view for TFTP config/status. |
| ostool-server/webui/src/views/SessionsView.vue | Admin view for active sessions + release. |
| ostool-server/webui/src/views/ServerView.vue | Admin view for server network config. |
| ostool-server/webui/src/views/OverviewView.vue | Admin overview dashboard. |
| ostool-server/webui/src/views/DtbView.vue | Admin DTB upload/rename/delete UI. |
| ostool-server/webui/src/views/DtbView.test.ts | Vitest coverage for DTB view flows. |
| ostool-server/webui/src/views/BoardsView.vue | Admin boards listing + filtering. |
| ostool-server/webui/src/views/BoardEditorView.test.ts | Vitest coverage for board editor flows. |
| ostool-server/webui/src/utils/time.ts | Lease remaining formatter helper. |
| ostool-server/webui/src/utils/time.test.ts | Tests for lease remaining formatting. |
| ostool-server/webui/src/utils/tftpStatus.ts | TFTP status -> tone/label mapping. |
| ostool-server/webui/src/utils/tftpStatus.test.ts | Tests for TFTP tone mapping. |
| ostool-server/webui/src/types/api.ts | Frontend API typings. |
| ostool-server/webui/src/stores/ui.ts | Pinia UI store for banners/title. |
| ostool-server/webui/src/router/index.ts | Vue-router routes under /admin/. |
| ostool-server/webui/src/main.ts | Vue app bootstrap. |
| ostool-server/webui/src/components/StatusPill.vue | Shared status pill component. |
| ostool-server/webui/src/components/NoticeBanner.vue | Shared notice banner component. |
| ostool-server/webui/src/App.vue | App shell + navigation + notices. |
| ostool-server/webui/src/api/client.ts | Fetch client for admin API endpoints. |
| ostool-server/webui/package.json | Web UI deps/scripts (vite/vitest). |
| ostool-server/webui/index.html | Web UI entry HTML. |
| ostool-server/src/web/mod.rs | Embed/serve admin SPA + static assets. |
| ostool-server/src/tftp/status.rs | TFTP diagnostics helpers (ss/systemctl/etc). |
| ostool-server/src/tftp/service.rs | TFTP manager trait + builtin/system implementations. |
| ostool-server/src/tftp/mod.rs | TFTP module wiring. |
| ostool-server/src/tftp/files.rs | Session-scoped TFTP file storage helpers. |
| ostool-server/src/tftp/config.rs | Re-export TFTP config types. |
| ostool-server/src/state.rs | AppState: boards/sessions/TFTP + cleanup/power-off. |
| ostool-server/src/session.rs | Session + SessionState with fixed TTL/heartbeat. |
| ostool-server/src/serial/ws.rs | Serial websocket handler + power-on/off cleanup. |
| ostool-server/src/serial/network.rs | Network interface enumeration for config/UI. |
| ostool-server/src/serial/discovery.rs | Serial port discovery w/ USB metadata enrichment. |
| ostool-server/src/serial.rs | Serial module exports. |
| ostool-server/src/process.rs | Async command runner utilities. |
| ostool-server/src/power.rs | Power management implementations (custom + relay). |
| ostool-server/src/main.rs | Server binary entrypoint + GC loop + router serve. |
| ostool-server/src/lib.rs | Server crate module exports/re-exports. |
| ostool-server/src/dtb_store/mod.rs | DTB store (list/read/write/rename/delete). |
| ostool-server/src/board_store/mod.rs | Board store module. |
| ostool-server/src/board_store/fs.rs | Per-board TOML persistence. |
| ostool-server/src/board_pool.rs | Board allocation logic (type/tags/leased). |
| ostool-server/src/api/models.rs | API DTOs for sessions/admin/TFTP/etc. |
| ostool-server/src/api/mod.rs | API module wiring. |
| ostool-server/src/api/error.rs | Unified JSON API error type. |
| ostool-server/scripts/ostool-server.service | systemd unit template. |
| ostool-server/scripts/install.sh | Install script (cargo install + config + systemd). |
| ostool-server/Cargo.toml | New server crate manifest + deps. |
| ostool-server/build.rs | Build web UI during Rust build. |
| jkconfig/src/run.rs | Use run_crossterm() with error mapping. |
| jkconfig/Cargo.toml | Make tokio non-optional; adjust feature flags. |
| Cargo.toml | Add ostool-server to workspace; bump toml to 1.0. |
| .gitignore | Ignore server config/data and webui artifacts. |
| .github/workflows/release.yml | Add release asset packaging job (incl. server). |
Files not reviewed (1)
- ostool-server/webui/pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let config_path = Self::default_path(explicit_path)?; | ||
| let mut config = jkconfig::run::<Self>(config_path.clone(), false, &[]) | ||
| .await | ||
| .with_context(|| format!("failed to load board config: {}", config_path.display()))? | ||
| .ok_or_else(|| anyhow!("No board configuration obtained"))?; | ||
| config.replace_strings(tool)?; | ||
| config.normalize(&format!("board config {}", config_path.display()))?; |
There was a problem hiding this comment.
anyhow!(...) is used but the anyhow macro isn't in scope in this module, which will fail to compile. Import it (e.g. use anyhow::anyhow;) or call it fully-qualified (anyhow::anyhow!(...)).
| listen_addr = "0.0.0.0:8080" | ||
| data_dir = "/var/lib/ostool-server" | ||
| board_dir = "/var/lib/ostool-server/boards" | ||
| dtb_dir = "/var/lib/ostool-server/dtbs" | ||
|
|
||
| [lease] | ||
| default_ttl_secs = 900 | ||
| max_ttl_secs = 3600 | ||
| gc_interval_secs = 30 | ||
|
|
||
| [network] | ||
| interface = "" | ||
|
|
||
| [tftp] |
There was a problem hiding this comment.
The generated default config still includes a [lease] section, but ServerConfig no longer has a lease field; this will make ostool-server fail to parse the config on first install. Update the template here to match the new config schema (remove [lease] and any obsolete keys).
| async fn start_if_needed(&self) -> anyhow::Result<()> { | ||
| if !self.config.enabled { | ||
| return Ok(()); | ||
| } | ||
| if self.started.swap(true, Ordering::AcqRel) { | ||
| return Ok(()); | ||
| } | ||
|
|
||
| std::fs::create_dir_all(&self.config.root_dir) | ||
| .with_context(|| format!("failed to create {}", self.config.root_dir.display()))?; | ||
|
|
There was a problem hiding this comment.
started.swap(true, ...) sets the builtin TFTP server as started before directory creation / server startup succeed. If create_dir_all (or later startup) fails, future calls won’t retry and status() may report healthy: true incorrectly. Consider using compare_exchange and only setting started = true after successful server startup (or resetting it on error).
| let last_error = Arc::new(Mutex::new(None)); | ||
| let error_store = last_error.clone(); | ||
|
|
||
| tokio::spawn(async move { | ||
| let result = tokio::task::spawn_blocking(move || match Server::new(&config) { | ||
| Ok(mut server) => server.listen(), | ||
| Err(err) => { | ||
| *error_store.lock().unwrap() = Some(err.to_string()); | ||
| log::error!("builtin tftp server failed to start: {err}"); | ||
| } | ||
| }) | ||
| .await; | ||
|
|
||
| if let Err(err) = result { | ||
| log::error!("builtin tftp server task failed: {err}"); | ||
| } | ||
| }); | ||
|
|
||
| clear_last_error(&self.last_error); | ||
| if let Some(error) = last_error.lock().unwrap().clone() { | ||
| set_last_error(&self.last_error, error); | ||
| } | ||
| Ok(()) |
There was a problem hiding this comment.
The last_error propagation here is racy/ineffective: you read last_error immediately after spawning the server task, before it has a chance to populate the mutex, so failures in Server::new won’t be reflected in self.last_error. If you want status() to surface startup errors, update self.last_error from inside the task (or store the error directly in self.last_error) and consider clearing it only on successful startup.
| } | ||
| Message::Close(_) => break, | ||
| Message::Ping(_) | Message::Pong(_) | Message::Frame(_) => {} | ||
| } |
There was a problem hiding this comment.
Message::Ping frames are ignored. If the server uses ping for keepalive, not replying with Pong can cause the websocket to be closed. Consider handling Ping(payload) by sending Pong(payload) (similar to the server-side implementation).
| .context("failed to flush text serial websocket payload")?; | ||
| } | ||
| Message::Close(_) => break, | ||
| Message::Ping(_) => {} | ||
| Message::Pong(_) | Message::Frame(_) => {} | ||
| } |
There was a problem hiding this comment.
Message::Ping frames are ignored. If the server uses ping for keepalive, not replying with Pong can cause the websocket to be closed. Consider handling Ping(payload) by sending Pong(payload) from the write side (or otherwise ensuring tungstenite replies).
| let webui_dir = Path::new("webui"); | ||
| if !webui_dir.exists() { | ||
| panic!("missing frontend directory: {}", webui_dir.display()); | ||
| } | ||
|
|
||
| let node_modules_marker = webui_dir.join("node_modules/.modules.yaml"); | ||
| let lockfile = webui_dir.join("pnpm-lock.yaml"); | ||
| if !node_modules_marker.exists() { | ||
| if lockfile.exists() { | ||
| run_pnpm(webui_dir, &["install", "--frozen-lockfile"]); | ||
| } else { | ||
| run_pnpm(webui_dir, &["install"]); | ||
| } | ||
| } | ||
|
|
||
| run_pnpm(webui_dir, &["build"]); | ||
|
|
||
| let index_html = Path::new("web/dist/index.html"); |
There was a problem hiding this comment.
This build script unconditionally runs pnpm install/pnpm build, which will fail in environments without Node/pnpm (including typical cargo install/crates.io build environments) and makes the Rust crate hard to build reproducibly offline. Consider gating the webui build behind a feature/env var, or embedding prebuilt assets for releases while keeping a separate workflow for rebuilding the frontend.
| let listener = tokio::net::TcpListener::bind(listen_addr) | ||
| .await | ||
| .with_context(|| format!("failed to bind {listen_addr}"))?; | ||
| info!("ostoold listening on {}", listen_addr); |
There was a problem hiding this comment.
Log message says ostoold listening on ... but the binary/service is ostool-server; this can be confusing when grepping logs. Consider renaming the identifier in this log line.
| info!("ostoold listening on {}", listen_addr); | |
| info!("ostool-server listening on {}", listen_addr); |
- Added main server entry point in `main.rs` to handle configuration and TFTP management. - Created `lib.rs` to define the server's API and module structure. - Implemented session management in `session.rs` to track active sessions. - Developed state management in `state.rs` to maintain application state and configuration. - Introduced TFTP file handling in `files.rs` and TFTP service management in `service.rs`. - Added WebSocket support for serial communication in `ws.rs`. - Implemented command execution utility in `process.rs`. - Established web assets and layout for the admin interface in `assets.rs` and `mod.rs`. - Created configuration structures for TFTP in `config.rs`. - Added tests for file handling and filename sanitization.
…s and tests - Implemented TFTP status and configuration management in TftpView.vue - Created BoardEditorView.vue for creating and editing board configurations - Developed BoardsView.vue for listing and filtering boards - Added OverviewView.vue for displaying admin overview statistics - Introduced SessionsView.vue for managing active sessions - Added server configuration management in ServerView.vue - Created utility functions for TFTP status description and lease time formatting - Added tests for TFTP status and time formatting utilities - Configured TypeScript support with tsconfig.json and vite.config.ts - Set up Vite for building and serving the web application
- Introduced TftpNetworkConfig to ServerConfig for managing TFTP network settings. - Implemented automatic detection of non-loopback network interfaces. - Added functionality to list available serial ports and network interfaces. - Updated BoardForm and ServerView to include TFTP network settings and serial port options. - Enhanced TFTP status to report resolved server IP and netmask. - Updated API client and types to support new endpoints for serial ports and network interfaces. - Added tests for new functionalities and ensured backward compatibility with existing configurations.
- Added `BoardServerClient` for interacting with the ostool-server API. - Implemented session management with `BoardSession`, including heartbeat functionality. - Created commands for listing available board types and running board sessions. - Introduced a terminal interface for serial communication via WebSocket. - Updated CLI to support new board commands and arguments. - Added tests for the new functionality and ensured proper error handling.
- Updated Cargo.toml to include async dependencies: futures, tokio, tokio-serial, and tokio-util. - Refactored example files (env_var.rs, loady.rs, loady_real.rs) to use async/await syntax. - Modified UbootShell implementation to support async I/O operations. - Updated YMODEM protocol implementation for async file transfers. - Enhanced tests to utilize async functions and ensure compatibility with the new async structure.
- Added new API endpoints for board editor functionality. - Introduced BoardEditorDocument type and related data structures. - Replaced existing form handling with JSON Forms for dynamic rendering. - Updated styles for the board editor UI. - Removed legacy board form utilities and tests. - Added tests for the new BoardEditorView and JSON Forms integration. - Enhanced validation and error handling in the board editor.
- Introduced `power_management` field in `BoardConfig` to support custom and relay-based power management. - Implemented `PowerManagementConfig` enum with variants for `CustomPowerManagement` and `ZhongshengRelayPowerManagement`. - Added functionality to execute power actions (on/off) based on the configured power management strategy. - Updated the WebSocket serial interface to handle automatic power-on and power-off actions. - Enhanced the board editor UI to include options for configuring power management settings. - Added tests for power management functionality, including command execution and error handling.
…tion - Deleted the boardEditorUiSchema.ts file as it is no longer needed. - Removed jsonFormsAjv.ts which contained JSON schema validation logic. - Eliminated BoardEditorJsonForms.render.test.ts as it is obsolete. - Updated BoardEditorView and BoardEditorView.test.ts to use a new form state management approach. - Refactored the BoardEditorView.vue to directly manage form state without JSON Forms. - Adjusted BoardsView.vue to remove unnecessary name column from the board table. - Modified SessionsView.vue to display board IDs instead of names. - Enhanced error handling in the Rust backend for board type not found scenarios.
- Updated `ServerControlMessage` struct to make fields public for better accessibility. - Enhanced `BoardRunArgs` to include configuration file paths and server details. - Implemented `prepare_uboot_artifacts` function to handle U-Boot artifact preparation. - Refactored `run_uboot` and `run_uboot_remote` methods in `Tool` to streamline U-Boot execution. - Introduced `RunnerBackend` trait with `LocalBackend` and `RemoteBackend` implementations for better separation of concerns. - Improved error handling and logging throughout the U-Boot execution process. - Updated tests to reflect changes in command parsing and configuration handling.
- Introduced DTB store in AppState to manage device tree binaries. - Updated the web UI to include a new DTB management section with routes and views. - Implemented API endpoints for listing, creating, updating, and deleting DTBs. - Enhanced BoardEditorView to support DTB file uploads and automatic name filling. - Created DtbView for managing existing DTBs, including renaming and replacing files. - Updated backend to handle DTB file uploads and downloads associated with sessions. - Added tests for new DTB functionalities in both frontend and backend.
…以简化配置 feat: 添加DTB上传和编辑功能,支持在界面中选择和修改DTB文件
- Removed LeaseConfig from ServerConfig and related validation logic. - Updated session handling to use SessionState for better encapsulation. - Refactored session allocation to avoid using a BTreeMap for sessions, now using Arc<SessionState>. - Simplified session heartbeat and cleanup logic. - Adjusted API responses to reflect the removal of lease parameters. - Updated frontend API types and views to remove lease configuration fields. - Improved serial websocket handling with timeout management. - Enhanced tests to cover new session management behavior.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 84 out of 88 changed files in this pull request and generated 6 comments.
Files not reviewed (1)
- ostool-server/webui/pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if self.started.swap(true, Ordering::AcqRel) { | ||
| return Ok(()); | ||
| } |
There was a problem hiding this comment.
started.swap(true, ...) marks the builtin TFTP server as started before confirming it actually bound/listened. If Server::new(&config) fails (e.g., port in use / permission denied), started stays true, status() reports healthy: true, and subsequent start_if_needed() calls will never retry. Consider only setting started = true after Server::new succeeds (or resetting it to false on error) and propagating the failure via last_error/returning an error from start_if_needed().
| let last_error = Arc::new(Mutex::new(None)); | ||
| let error_store = last_error.clone(); | ||
|
|
||
| tokio::spawn(async move { | ||
| let result = tokio::task::spawn_blocking(move || match Server::new(&config) { | ||
| Ok(mut server) => server.listen(), | ||
| Err(err) => { | ||
| *error_store.lock().unwrap() = Some(err.to_string()); | ||
| log::error!("builtin tftp server failed to start: {err}"); |
There was a problem hiding this comment.
The local last_error (Arc<Mutex<Option<String>>>) is checked immediately after spawning the server task, so it will almost always still be None even if the task later records an error. As a result, self.last_error may never reflect startup failures. Suggest writing errors directly into self.last_error from the spawned task (or using a channel/JoinHandle to await the startup result) so status() can report accurate diagnostics.
| async fn start_if_needed(&self) -> anyhow::Result<()> { | ||
| if self.config.root_dir.exists() { | ||
| return Ok(()); | ||
| } | ||
|
|
||
| if !self.config.manage_config { | ||
| bail!( | ||
| "TFTP root {} does not exist; create it manually, or enable `tftp.manage_config`", | ||
| self.config.root_dir.display() | ||
| ); | ||
| } |
There was a problem hiding this comment.
SystemTftpdHpaManager::start_if_needed() does not check self.config.enabled. Since ostool-server/src/main.rs calls tftp_manager.start_if_needed().await? unconditionally, a disabled system_tftpd_hpa config can still error out if root_dir doesn’t exist and manage_config is false. Consider early-returning Ok(()) when enabled is false, matching the builtin manager behavior.
| listen_addr = "0.0.0.0:8080" | ||
| data_dir = "/var/lib/ostool-server" | ||
| board_dir = "/var/lib/ostool-server/boards" | ||
| dtb_dir = "/var/lib/ostool-server/dtbs" | ||
|
|
||
| [lease] | ||
| default_ttl_secs = 900 | ||
| max_ttl_secs = 3600 | ||
| gc_interval_secs = 30 | ||
|
|
There was a problem hiding this comment.
The generated default config still includes a [lease] section, but the PR description states lease configuration was removed and sessions use fixed short TTL semantics. If the server no longer reads [lease], this will be confusing (and may cause users to think it’s configurable). Update the template to remove [lease] (or reintroduce parsing/support if it’s still intended).
| echo "" | ||
| echo "==> Installing ostool-server..." | ||
|
|
||
| if [[ -n "$LOCAL_PATH" ]]; then | ||
| LOCAL_PATH="$(cd "$LOCAL_PATH" && pwd)" | ||
| echo "Installing from local source: ${LOCAL_PATH}" | ||
| cargo install --path "${LOCAL_PATH}" | ||
| else | ||
| echo "Installing from crates.io..." | ||
| cargo install "${SERVICE_NAME}" | ||
| fi |
There was a problem hiding this comment.
cargo install ostool-server will execute ostool-server/build.rs, which currently requires pnpm (and Node) to be installed to build the embedded Web UI. The installer script doesn’t check for or install these prerequisites, so a fresh machine will likely fail at this step. Consider adding a preflight check + clear instructions (or installing pnpm), or making the web UI build optional/prebuilt so cargo install works without Node tooling.
| fn run_pnpm(webui_dir: &Path, args: &[&str]) { | ||
| let status = Command::new("pnpm") | ||
| .arg("--dir") | ||
| .arg(webui_dir) | ||
| .args(args) | ||
| .stdin(Stdio::null()) | ||
| .stdout(Stdio::inherit()) | ||
| .stderr(Stdio::inherit()) | ||
| .status() | ||
| .unwrap_or_else(|err| { | ||
| panic!( | ||
| "failed to run `pnpm --dir {} {}`: {err}", | ||
| webui_dir.display(), | ||
| args.join(" ") | ||
| ) |
There was a problem hiding this comment.
build.rs hard-requires pnpm at compile time and panics if it’s missing. This makes cargo build / cargo install fragile in environments without Node tooling (and the failure mode is a panic during build-script execution). Consider a preflight that detects missing pnpm and emits a clearer error, and/or allow opting out via an env var/feature flag or using prebuilt web/dist assets for release builds.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 85 out of 89 changed files in this pull request and generated 3 comments.
Files not reviewed (1)
- ostool-server/webui/pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| run_cmd tee "${CONFIG_FILE}" >/dev/null <<'CONF' | ||
| listen_addr = "0.0.0.0:8080" | ||
| data_dir = "/var/lib/ostool-server" | ||
| board_dir = "/var/lib/ostool-server/boards" | ||
| dtb_dir = "/var/lib/ostool-server/dtbs" | ||
|
|
||
| [lease] | ||
| default_ttl_secs = 900 | ||
| max_ttl_secs = 3600 | ||
| gc_interval_secs = 30 | ||
|
|
||
| [network] | ||
| interface = "" | ||
|
|
||
| [tftp] | ||
| provider = "system_tftpd_hpa" | ||
|
|
||
| [tftp.config] | ||
| enabled = true | ||
| root_dir = "/srv/tftp" | ||
| config_path = "/etc/default/tftpd-hpa" | ||
| service_name = "tftpd-hpa" | ||
| username = "tftp" | ||
| address = ":69" | ||
| options = "-l -s -c" | ||
| manage_config = false | ||
| reconcile_on_start = true | ||
| CONF |
There was a problem hiding this comment.
The installer writes a default config that doesn't match ServerConfig/TftpConfig deserialization: ServerConfig has no [lease] table, and TftpConfig is internally-tagged (provider) so its fields must live directly under [tftp] (there is no [tftp.config]). As written, fresh installs will fail to load/parse config (or at minimum silently ignore obsolete keys). Update the template to remove [lease] and inline the provider-specific fields under [tftp] (and keep [network] as-is).
| unsafe { | ||
| return Err(anyhow::anyhow!( | ||
| "{}", | ||
| OsString::from_encoded_bytes_unchecked(out.stderr).to_string_lossy() | ||
| OsString::from_encoded_bytes_unchecked(stderr_capture.lock().unwrap().clone()) | ||
| .to_string_lossy() | ||
| )); |
There was a problem hiding this comment.
This unsafe conversion of captured stderr bytes (OsString::from_encoded_bytes_unchecked) is avoidable and can be replaced with a safe lossy decode (e.g., String::from_utf8_lossy(&bytes) or platform-specific OsStringExt::from_vec on Unix). Using a safe conversion reduces the surface area of unsafe in error paths without losing useful diagnostics.
| let mut config = jkconfig::run::<Self>(config_path.clone(), false, &[]) | ||
| .await | ||
| .with_context(|| format!("failed to load board config: {}", config_path.display()))? | ||
| .ok_or_else(|| anyhow!("No board configuration obtained"))?; | ||
| config.replace_strings(tool)?; |
There was a problem hiding this comment.
BoardRunConfig::load_or_create() uses the anyhow! macro, but this module only imports anyhow::Context and does not bring anyhow! into scope. This will fail to compile. Fix by switching to anyhow::anyhow!(...) or importing the macro/function (e.g., use anyhow::anyhow;) and then using it consistently.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 85 out of 89 changed files in this pull request and generated 2 comments.
Files not reviewed (1)
- ostool-server/webui/pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| read_task.abort(); | ||
| if let Err(err) = write_task.await | ||
| && !err.is_cancelled() | ||
| { | ||
| log::debug!("serial websocket writer join error: {err}"); | ||
| } | ||
| if let Err(err) = read_task.await | ||
| && !err.is_cancelled() | ||
| { | ||
| log::debug!("serial websocket reader join error: {err}"); | ||
| } | ||
|
|
||
| run_result |
There was a problem hiding this comment.
The read/write websocket tasks’ inner errors are currently ignored: write_task.await / read_task.await only log JoinError, but Ok(Err(_)) from the task is dropped. This can make run_serial_terminal return Ok(()) even when the websocket read/write failed. Please propagate Ok(Err(err)) (and consider not aborting the reader unconditionally) similar to the join handling in run/uboot.rs.
| let status = child.wait().await?; | ||
| let _ = stdout_task.await; | ||
| let _ = stderr_task.await; | ||
| let _ = write_task.await; | ||
|
|
||
| terminal_result?; | ||
|
|
||
| if let Some(result) = match_result.lock().unwrap().take() { | ||
| result?; |
There was a problem hiding this comment.
The child stream tasks and stdin forwarder task results are discarded (let _ = ...await). If any of these tasks returns an Err (e.g. stdout/stderr read fails), the failure is silently ignored and can lead to missing diagnostics or hanging behavior. Please inspect the join results and propagate/log inner task errors (like the join handling pattern used in run/uboot.rs).
Summary
run boardexits cleanly after success or terminal exitWhy
The previous session lifecycle state was split across multiple maps and ad-hoc flags, which made cleanup hard to reason about and contributed to shutdown/exit bugs. This change consolidates session state and removes configuration that no longer affects runtime behavior.
User Impact
ostool-serverno longer exposes editable lease configurationostool run boardshould exit cleanly after success-pattern match or terminal exit instead of hanging and/or reporting a false timeoutValidation
cargo test -q -p ostool-servercargo test -q -p ostoolcargo clippy -q -p ostool-server -- -W clippy::all