Improve build process#4254
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the project's build and development workflow by replacing wasm-pack and cargo-watch with a custom Rust-based build tool (cargo-run) that directly manages wasm-bindgen compilation, process supervision, and file watching. Feedback on these changes highlights several critical issues: a compilation error in the automatic package installation logic, an inverted skip condition for the Rust Wasm Toolchain requirement, and an infinite loop vulnerability in the process supervisor when no children are provided. Additionally, the reviewer noted that output_unchecked incorrectly uses run instead of output, and that build failures are currently ignored due to a lack of error propagation in Sequence::wait and build_wasm.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| fn output_unchecked(self) -> Result<std::process::Output, Error> { | ||
| let e = self.unchecked().stdout_capture().stderr_capture(); | ||
| duct::Expression::run(&e).map_err(Error::Command) | ||
| } |
There was a problem hiding this comment.
The duct::Expression::run method returns Result<(), std::io::Error>, but output_unchecked expects Result<std::process::Output, Error>. To capture and return the process output, use duct::Expression::output instead.
| fn output_unchecked(self) -> Result<std::process::Output, Error> { | |
| let e = self.unchecked().stdout_capture().stderr_capture(); | |
| duct::Expression::run(&e).map_err(Error::Command) | |
| } | |
| fn output_unchecked(self) -> Result<std::process::Output, Error> { | |
| let e = self.unchecked().stdout_capture().stderr_capture(); | |
| duct::Expression::output(&e).map_err(Error::Command) | |
| } |
| match &dep.install { | ||
| InstallAction::Command(install_cmd) => { | ||
| let parts: Vec<&str> = install_cmd.split_whitespace().collect(); | ||
| let expr = cmd(parts[0], parts[1..].iter().copied()).unchecked(); | ||
| match Expression::run(&expr) { | ||
| Ok(output) if output.status.success() => successfully_installed.push(i), | ||
| Ok(_) => eprintln!("Failed to install {}", dep.name), | ||
| Err(e) => return Err(Error::Command(e)), | ||
| } | ||
| } |
There was a problem hiding this comment.
The Expression::run method returns Result<(), std::io::Error>, so matching on Ok(output) if output.status.success() will cause a compilation error. Matching on Ok(()) is sufficient as run already checks for a successful exit status.
match &dep.install {
InstallAction::Command(install_cmd) => {
let parts: Vec<&str> = install_cmd.split_whitespace().collect();
let expr = cmd(parts[0], parts[1..].iter().copied()).unchecked();
match Expression::run(&expr) {
Ok(()) => successfully_installed.push(i),
Err(e) => return Err(Error::Command(e)),
}
}| pub fn sequence<I: IntoIterator<Item = Expression>>(expressions: I) -> Sequence { | ||
| let expressions: Vec<Expression> = expressions.into_iter().collect(); | ||
| let current: Arc<Mutex<Option<Arc<duct::Handle>>>> = Arc::new(Mutex::new(None)); | ||
| let killed = Arc::new(AtomicBool::new(false)); | ||
|
|
||
| let worker_current = Arc::clone(¤t); | ||
| let worker_killed = Arc::clone(&killed); | ||
| let worker = std::thread::spawn(move || { | ||
| for expr in expressions { | ||
| if worker_killed.load(Ordering::SeqCst) { | ||
| return; | ||
| } | ||
| let handle = match expr.start() { | ||
| Ok(h) => Arc::new(h), | ||
| Err(e) => { | ||
| eprintln!("sequence: failed to start step: {e}"); | ||
| return; | ||
| } | ||
| }; | ||
| { | ||
| let mut slot = worker_current.lock().unwrap(); | ||
| if worker_killed.load(Ordering::SeqCst) { | ||
| let _ = handle.kill(); | ||
| return; | ||
| } | ||
| *slot = Some(Arc::clone(&handle)); | ||
| } | ||
| let result = handle.wait().map(|_| ()); | ||
| worker_current.lock().unwrap().take(); | ||
| if worker_killed.load(Ordering::SeqCst) { | ||
| return; | ||
| } | ||
| if let Err(e) = result { | ||
| eprintln!("sequence: step failed: {e}"); | ||
| return; | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| Sequence { | ||
| current, | ||
| killed, | ||
| worker: Some(worker), | ||
| } | ||
| } | ||
|
|
||
| pub struct Sequence { | ||
| current: Arc<Mutex<Option<Arc<duct::Handle>>>>, | ||
| killed: Arc<AtomicBool>, | ||
| worker: Option<JoinHandle<()>>, | ||
| } | ||
| impl Sequence { | ||
| pub fn kill(&self) { | ||
| let slot = self.current.lock().unwrap(); | ||
| self.killed.store(true, Ordering::SeqCst); | ||
| if let Some(handle) = slot.as_ref() { | ||
| let _ = handle.kill(); | ||
| } | ||
| } | ||
|
|
||
| pub fn wait(&mut self) { | ||
| if let Some(w) = self.worker.take() { | ||
| let _ = w.join(); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Currently, Sequence::wait discards the exit status of the background thread, preventing build failures from being propagated. Updating Sequence to return Result<(), Error> from wait allows caller sites to handle failures properly.
pub fn sequence<I: IntoIterator<Item = Expression>>(expressions: I) -> Sequence {
let expressions: Vec<Expression> = expressions.into_iter().collect();
let current: Arc<Mutex<Option<Arc<duct::Handle>>>> = Arc::new(Mutex::new(None));
let killed = Arc::new(AtomicBool::new(false));
let worker_current = Arc::clone(¤t);
let worker_killed = Arc::clone(&killed);
let worker = std::thread::spawn(move || {
for expr in expressions {
if worker_killed.load(Ordering::SeqCst) {
return Ok(());
}
let handle = match expr.start() {
Ok(h) => Arc::new(h),
Err(e) => {
eprintln!("sequence: failed to start step: {e}");
return Err(Error::Command(e));
}
};
{
let mut slot = worker_current.lock().unwrap();
if worker_killed.load(Ordering::SeqCst) {
let _ = handle.kill();
return Ok(());
}
*slot = Some(Arc::clone(&handle));
}
let result = handle.wait().map(|_| ());
worker_current.lock().unwrap().take();
if worker_killed.load(Ordering::SeqCst) {
return Ok(());
}
if let Err(e) = result {
eprintln!("sequence: step failed: {e}");
return Err(Error::Command(e));
}
}
Ok(())
});
Sequence {
current,
killed,
worker: Some(worker),
}
}
pub struct Sequence {
current: Arc<Mutex<Option<Arc<duct::Handle>>>>,
killed: Arc<AtomicBool>,
worker: Option<JoinHandle<Result<(), Error>>>,
}
impl Sequence {
pub fn kill(&self) {
let slot = self.current.lock().unwrap();
self.killed.store(true, Ordering::SeqCst);
if let Some(handle) = slot.as_ref() {
let _ = handle.kill();
}
}
pub fn wait(&mut self) -> Result<(), Error> {
if let Some(w) = self.worker.take() {
w.join().unwrap_or_else(|_| Err(Error::Command(std::io::Error::other("worker thread panicked"))))
} else {
Ok(())
}
}
}| pub fn build_wasm(release: bool, native: bool) -> Result<(), Error> { | ||
| sequence(build_wasm_steps(release, native)).wait(); | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Propagate the build result from sequence().wait() so that any compilation failures are correctly reported to the caller instead of silently continuing.
pub fn build_wasm(release: bool, native: bool) -> Result<(), Error> {
sequence(build_wasm_steps(release, native)).wait()
}| Requirement { | ||
| command: "rustc", | ||
| args: &["--print", "target-libdir", "--target", "wasm32-unknown-unknown"], | ||
| check: Check::Matches(&|out| std::path::Path::new(out.trim()).is_dir()), | ||
| name: "Rust Wasm Toolchain", | ||
| install: "rustup target add wasm32-unknown-unknown".into(), | ||
| skip: Some(&|task| !matches!(task.target, Target::Cli)), | ||
| ..Default::default() | ||
| }, |
There was a problem hiding this comment.
The skip condition for the Rust Wasm Toolchain is inverted. It is currently skipped for everything except the CLI target, but it should be skipped only for the CLI target (since only Web and Desktop targets compile to Wasm).
| Requirement { | |
| command: "rustc", | |
| args: &["--print", "target-libdir", "--target", "wasm32-unknown-unknown"], | |
| check: Check::Matches(&|out| std::path::Path::new(out.trim()).is_dir()), | |
| name: "Rust Wasm Toolchain", | |
| install: "rustup target add wasm32-unknown-unknown".into(), | |
| skip: Some(&|task| !matches!(task.target, Target::Cli)), | |
| ..Default::default() | |
| }, | |
| Requirement { | |
| command: "rustc", | |
| args: &["--print", "target-libdir", "--target", "wasm32-unknown-unknown"], | |
| check: Check::Matches(&|out| std::path::Path::new(out.trim()).is_dir()), | |
| name: "Rust Wasm Toolchain", | |
| install: "rustup target add wasm32-unknown-unknown".into(), | |
| skip: Some(&|task| matches!(task.target, Target::Cli)), | |
| ..Default::default() | |
| }, |
| let mut handles: Vec<(String, Arc<duct::ReaderHandle>)> = Vec::new(); | ||
| for (label, expr) in children { | ||
| let handle = expr.stderr_to_stdout().reader().map_err(Error::Command)?; | ||
| handles.push((label.into(), Arc::new(handle))); | ||
| } |
There was a problem hiding this comment.
If children is empty, handles will be empty, causing the supervisor loop to run infinitely. Adding an early return when handles is empty prevents this.
let mut handles: Vec<(String, Arc<duct::ReaderHandle>)> = Vec::new();
for (label, expr) in children {
let handle = expr.stderr_to_stdout().reader().map_err(Error::Command)?;
handles.push((label.into(), Arc::new(handle)));
}
if handles.is_empty() {
return Ok(());
}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
9 issues found across 19 files
Confidence score: 2/5
- In
tools/cargo-run/src/requirements.rs, the Rust Wasm Toolchainskiplogic is inverted and the confirmation defaults ([y/N]and[Y/n]) are mishandled, so required setup can be skipped and unresolved requirements can be accepted by default; merging as-is risks broken Web/Desktop runs and confusing setup behavior — correct the target match and align Enter/default handling with the prompt text before merging. - In
tools/cargo-run/src/cmd.rs,supervisecan hang with zero children, may report the wrong child outcome, andSequence::waitdrops worker errors, which can mask failing build steps or return misleading success — add an empty-input early return, preserve true earliest exit/failure semantics, and propagate thread results back to callers. - In
tools/cargo-run/src/frontend.rsandtools/cargo-run/src/watch.rs,build_wasmcurrently returnsOk(())even when steps fail and file watching skips non-.rsinputs like manifests, so failed or stale wasm artifacts can look healthy — propagateSequence::waiterrors and include manifest/dependency input changes in rebuild triggers. - In
.github/workflows/build.yml, downloaded toolchain binaries are executed without integrity checks, creating a CI supply-chain exposure if artifacts are tampered with — verify checksums/signatures (or equivalent trusted provenance) before use.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tools/cargo-run/src/requirements.rs">
<violation number="1" location="tools/cargo-run/src/requirements.rs:211">
P2: `[Y/n]` prompt handling is incorrect: pressing Enter does not take the default yes path. This causes skipped auto-install even when the prompt advertises yes as default.</violation>
</file>
<file name="tools/cargo-run/src/cmd.rs">
<violation number="1" location="tools/cargo-run/src/cmd.rs:114">
P1: `supervise` hangs forever when called with no children. Add an early return before the wait loop for the empty case.</violation>
<violation number="2" location="tools/cargo-run/src/cmd.rs:115">
P2: `supervise` does not reliably select the earliest-exiting child; it selects the first finished handle in vector order at poll time. This can incorrectly return success while another child already failed.</violation>
<violation number="3" location="tools/cargo-run/src/cmd.rs:218">
P1: `Sequence::wait` discards the worker thread's result. If any step in the sequence fails (e.g., `cargo build` or `wasm-bindgen`), the error is printed to stderr but never propagated to the caller. The worker thread should return `Result<(), Error>` and `wait()` should return that result so callers can detect failures.</violation>
</file>
<file name="tools/cargo-run/src/watch.rs">
<violation number="1" location="tools/cargo-run/src/watch.rs:11">
P2: Watcher ignores non-`.rs` Rust build inputs, so manifest changes do not trigger rebuilds. This can leave wasm output stale after dependency/feature changes.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| } | ||
|
|
||
| let mut first: Option<(String, std::process::ExitStatus)> = None; | ||
| while first.is_none() { |
There was a problem hiding this comment.
P1: supervise hangs forever when called with no children. Add an early return before the wait loop for the empty case.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tools/cargo-run/src/cmd.rs, line 114:
<comment>`supervise` hangs forever when called with no children. Add an early return before the wait loop for the empty case.</comment>
<file context>
@@ -0,0 +1,256 @@
+ }
+
+ let mut first: Option<(String, std::process::ExitStatus)> = None;
+ while first.is_none() {
+ for (label, handle) in handles.iter() {
+ if let Ok(Some(output)) = handle.try_wait() {
</file context>
| } | ||
| } | ||
|
|
||
| pub fn wait(&mut self) { |
There was a problem hiding this comment.
P1: Sequence::wait discards the worker thread's result. If any step in the sequence fails (e.g., cargo build or wasm-bindgen), the error is printed to stderr but never propagated to the caller. The worker thread should return Result<(), Error> and wait() should return that result so callers can detect failures.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tools/cargo-run/src/cmd.rs, line 218:
<comment>`Sequence::wait` discards the worker thread's result. If any step in the sequence fails (e.g., `cargo build` or `wasm-bindgen`), the error is printed to stderr but never propagated to the caller. The worker thread should return `Result<(), Error>` and `wait()` should return that result so callers can detect failures.</comment>
<file context>
@@ -0,0 +1,256 @@
+ }
+ }
+
+ pub fn wait(&mut self) {
+ if let Some(w) = self.worker.take() {
+ let _ = w.join();
</file context>
| use std::time::Duration; | ||
|
|
||
| const EXCLUDED_DIRECTORIES: &[&str] = &["target", ".git", "frontend/node_modules", "frontend/dist", "frontend/wrapper/pkg"]; | ||
| const INCLUDED_EXTENSIONS: &[&str] = &["rs"]; |
There was a problem hiding this comment.
P2: Watcher ignores non-.rs Rust build inputs, so manifest changes do not trigger rebuilds. This can leave wasm output stale after dependency/feature changes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tools/cargo-run/src/watch.rs, line 11:
<comment>Watcher ignores non-`.rs` Rust build inputs, so manifest changes do not trigger rebuilds. This can leave wasm output stale after dependency/feature changes.</comment>
<file context>
@@ -0,0 +1,76 @@
+use std::time::Duration;
+
+const EXCLUDED_DIRECTORIES: &[&str] = &["target", ".git", "frontend/node_modules", "frontend/dist", "frontend/wrapper/pkg"];
+const INCLUDED_EXTENSIONS: &[&str] = &["rs"];
+
+const DEBOUNCE: Duration = Duration::from_millis(500);
</file context>
| std::io::stdin().read_line(&mut input).map_err(|e| Error::Io(e, "Failed to read from stdin".into()))?; | ||
| let input = input.trim(); | ||
|
|
||
| if input.eq_ignore_ascii_case("y") || input.eq_ignore_ascii_case("yes") { |
There was a problem hiding this comment.
P2: [Y/n] prompt handling is incorrect: pressing Enter does not take the default yes path. This causes skipped auto-install even when the prompt advertises yes as default.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tools/cargo-run/src/requirements.rs, line 211:
<comment>`[Y/n]` prompt handling is incorrect: pressing Enter does not take the default yes path. This causes skipped auto-install even when the prompt advertises yes as default.</comment>
<file context>
@@ -158,43 +180,149 @@ pub fn check(task: &Task) -> Result<(), Error> {
+ std::io::stdin().read_line(&mut input).map_err(|e| Error::Io(e, "Failed to read from stdin".into()))?;
+ let input = input.trim();
+
+ if input.eq_ignore_ascii_case("y") || input.eq_ignore_ascii_case("yes") {
+ let mut successfully_installed = Vec::new();
+
</file context>
|
|
||
| let mut first: Option<(String, std::process::ExitStatus)> = None; | ||
| while first.is_none() { | ||
| for (label, handle) in handles.iter() { |
There was a problem hiding this comment.
P2: supervise does not reliably select the earliest-exiting child; it selects the first finished handle in vector order at poll time. This can incorrectly return success while another child already failed.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tools/cargo-run/src/cmd.rs, line 115:
<comment>`supervise` does not reliably select the earliest-exiting child; it selects the first finished handle in vector order at poll time. This can incorrectly return success while another child already failed.</comment>
<file context>
@@ -0,0 +1,256 @@
+
+ let mut first: Option<(String, std::process::ExitStatus)> = None;
+ while first.is_none() {
+ for (label, handle) in handles.iter() {
+ if let Ok(Some(output)) = handle.try_wait() {
+ first = Some((label.clone(), output.status));
</file context>
Performance Benchmark Results
|
…duction The devDependencies hold the build tooling (Vite, etc.), which npm omits in environments that set NODE_ENV=production, like CI does for the Vite build. The install timestamp check now also covers package-installer.js itself so changes to the install process trigger a reinstall.
timon-schelling
force-pushed
the
improve-build-process
branch
from
f651f2b to
afa28cb
Compare
There was a problem hiding this comment.
1 issue found across 5 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tools/cargo-run/src/cmd.rs">
<violation number="1" location="tools/cargo-run/src/cmd.rs:130">
P2: `SetConsoleCtrlHandler` return value is ignored, so handler registration failures are silent. Check the result and surface a warning/error so Windows Ctrl+C cleanup is not silently bypassed.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| INTERRUPTED.store(false, Ordering::SeqCst); | ||
| static REGISTER: Once = Once::new(); | ||
| REGISTER.call_once(|| unsafe { | ||
| SetConsoleCtrlHandler(Some(handler), 1); |
There was a problem hiding this comment.
P2: SetConsoleCtrlHandler return value is ignored, so handler registration failures are silent. Check the result and surface a warning/error so Windows Ctrl+C cleanup is not silently bypassed.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tools/cargo-run/src/cmd.rs, line 130:
<comment>`SetConsoleCtrlHandler` return value is ignored, so handler registration failures are silent. Check the result and surface a warning/error so Windows Ctrl+C cleanup is not silently bypassed.</comment>
<file context>
@@ -60,44 +84,95 @@ impl ExpressionExt for Expression {
+ INTERRUPTED.store(false, Ordering::SeqCst);
+ static REGISTER: Once = Once::new();
+ REGISTER.call_once(|| unsafe {
+ SetConsoleCtrlHandler(Some(handler), 1);
+ });
+ }
</file context>
| SetConsoleCtrlHandler(Some(handler), 1); | |
| if SetConsoleCtrlHandler(Some(handler), 1) == 0 { | |
| eprintln!("warning: failed to register console Ctrl+C handler; supervised children may not shut down cleanly"); | |
| } |
2 participants
Replaces parts of #4228. Closes #2919.