From fe4f9f57cac6e136e2ae23c913c371f762e6c411 Mon Sep 17 00:00:00 2001 From: Michael Moffett Date: Thu, 13 Aug 2026 12:08:01 -0600 Subject: [PATCH 1/3] feat(cli): install the Solana dev skills on first scaffold Runs the command from #567 on the run that scaffolds a txtx.yml. The child is spawned with all three stdio handles nulled and is never waited on, so a slow, failing or absent install cannot affect startup. It is reaped on a detached thread so it does not sit defunct. Two tests cover the invocation and all three failure modes: a missing binary, a non-zero exit, and a hang. --- crates/cli/src/scaffold/mod.rs | 81 ++++++++++++++++++++++++++++++++++ 1 file changed, 81 insertions(+) diff --git a/crates/cli/src/scaffold/mod.rs b/crates/cli/src/scaffold/mod.rs index 33dffe4e2..221bae33f 100644 --- a/crates/cli/src/scaffold/mod.rs +++ b/crates/cli/src/scaffold/mod.rs @@ -1,6 +1,7 @@ use std::{ env, fs::{self, File}, + process::{Command, Stdio}, }; use dialoguer::{Confirm, Input, MultiSelect, console::Style, theme::ColorfulTheme}; @@ -134,6 +135,35 @@ impl ProgramMetadata { } } +const DEV_SKILL_REPO: &str = "https://github.com/solana-foundation/solana-dev-skill"; + +/// Builds the skill install exactly as issue #567 specifies it. +fn dev_skill_install_command() -> Command { + let mut command = Command::new("npx"); + command.args(["-y", "skills", "add", DEV_SKILL_REPO, "--skill", "*", "-y"]); + command +} + +/// Starts the skill install and returns, whatever becomes of it. +/// +/// This runs on the way to booting someone's surfnet, so it gets no say in +/// that: never waited on, output never reaching the terminal, and nothing at +/// all on a machine without Node — the usual case for a Rust user. +fn spawn_dev_skill_install(mut command: Command) { + let Ok(mut child) = command + .stdin(Stdio::null()) + .stdout(Stdio::null()) + .stderr(Stdio::null()) + .spawn() + else { + return; + }; + // Reaped off-thread: the child leaves no defunct entry, the scaffold no wait. + let _ = hiro_system_kit::thread_named("Dev Skill Install").spawn(move || { + let _ = child.wait(); + }); +} + pub fn scaffold_in_memory_iac( framework: &Framework, programs: &[ProgramMetadata], @@ -206,6 +236,10 @@ pub fn scaffold_iac_layout( base_location: &FileLocation, auto_generate_runbooks: bool, ) -> Result<(), String> { + // Reached only when the project has no `txtx.yml` yet, so this is a first + // start; #567 asks for the skills here. Ahead of the prompts, to overlap. + spawn_dev_skill_install(dev_skill_install_command()); + let mut target_location = base_location.clone(); target_location.append_path("target")?; @@ -514,3 +548,50 @@ pub fn scaffold_iac_layout( Ok(()) } + +#[cfg(test)] +mod tests { + use std::{ + process::Command, + time::{Duration, Instant}, + }; + + use super::{DEV_SKILL_REPO, dev_skill_install_command, spawn_dev_skill_install}; + + #[cfg(unix)] + fn sh(script: &str) -> Command { + let mut command = Command::new("sh"); + command.args(["-c", script]); + command + } + + /// #567 supplied this invocation literally: the `-y` flags keep it off a + /// prompt and `--skill "*"` is what makes it the whole bundle. + #[test] + fn the_install_is_the_command_issue_567_asked_for() { + let command = dev_skill_install_command(); + assert_eq!(command.get_program(), "npx"); + assert_eq!( + command.get_args().collect::>(), + ["-y", "skills", "add", DEV_SKILL_REPO, "--skill", "*", "-y"] + ); + } + + /// The three ways this goes wrong on a real machine: no Node at all, an + /// install that fails, an install that hangs. Each returns at once and + /// returns nothing, so the scaffold has neither a value to branch on nor a + /// wait to be held by — its result and its output are the same either way. + #[cfg(unix)] + #[test] + fn no_outcome_of_the_install_reaches_the_scaffold() { + let started = Instant::now(); + spawn_dev_skill_install(Command::new("surfpool-567-no-such-binary")); + spawn_dev_skill_install(sh("exit 1")); + spawn_dev_skill_install(sh("sleep 30")); + assert!( + started.elapsed() < Duration::from_secs(5), + "the scaffold waited on the install: {:?}", + started.elapsed() + ); + } +} From 834d249c3f56904318178be915a184153fd4ed1a Mon Sep 17 00:00:00 2001 From: Michael Moffett Date: Thu, 13 Aug 2026 13:43:55 -0600 Subject: [PATCH 2/3] fix(cli): pin the skill install and run it after the scaffold Three follow-ups on the first-scaffold install, from review of the previous commit. The invocation carried no version, so a first scaffold ran whatever the registry called latest at that moment. It is pinned to skills@1.5.22. An exact version is the only form that resolves the same way twice; a range still floats to the newest release inside it. The child inherited the process working directory rather than the project being scaffolded. Those differ whenever -m points at a manifest outside the current directory, so the skills could land somewhere other than the project. It now runs in the manifest's directory. The spawn sat at the top of scaffold_iac_layout, so a cancelled prompt or a failure part way through left an install running behind a scaffold that never finished, and the next start began a second one. It now runs only once the scaffold has finished, from either exit that reaches that point. One added test pins the working directory. The existing test that pins the invocation now pins the version with it, and fails on a range or a bare package name. --- crates/cli/src/scaffold/mod.rs | 83 +++++++++++++++++++++++++++++----- 1 file changed, 72 insertions(+), 11 deletions(-) diff --git a/crates/cli/src/scaffold/mod.rs b/crates/cli/src/scaffold/mod.rs index 221bae33f..99b2914b1 100644 --- a/crates/cli/src/scaffold/mod.rs +++ b/crates/cli/src/scaffold/mod.rs @@ -137,10 +137,26 @@ impl ProgramMetadata { const DEV_SKILL_REPO: &str = "https://github.com/solana-foundation/solana-dev-skill"; -/// Builds the skill install exactly as issue #567 specifies it. -fn dev_skill_install_command() -> Command { +/// Pinned: an unversioned `npx skills` runs whatever the registry calls latest +/// at the moment of someone's first scaffold, which is not a thing this can +/// promise anyone. An exact version is the only form that resolves the same way +/// twice; a range still floats to the newest release inside it. +const DEV_SKILL_INSTALLER: &str = "skills@1.5.22"; + +/// Builds the skill install as issue #567 specifies it, pinned, and rooted at +/// the project being scaffolded rather than wherever the process happens to sit. +fn dev_skill_install_command(base_location: &FileLocation) -> Command { let mut command = Command::new("npx"); - command.args(["-y", "skills", "add", DEV_SKILL_REPO, "--skill", "*", "-y"]); + command.args([ + "-y", + DEV_SKILL_INSTALLER, + "add", + DEV_SKILL_REPO, + "--skill", + "*", + "-y", + ]); + command.current_dir(base_location.expect_path_buf()); command } @@ -236,10 +252,6 @@ pub fn scaffold_iac_layout( base_location: &FileLocation, auto_generate_runbooks: bool, ) -> Result<(), String> { - // Reached only when the project has no `txtx.yml` yet, so this is a first - // start; #567 asks for the skills here. Ahead of the prompts, to overlap. - spawn_dev_skill_install(dev_skill_install_command()); - let mut target_location = base_location.clone(); target_location.append_path("target")?; @@ -462,6 +474,9 @@ pub fn scaffold_iac_layout( // "file {} already exists. choose a different runbook name, or rename the existing file", // runbook_file_location.to_string() // )) + // The scaffold succeeded — `txtx.yml` and the runbooks tree are on + // disk — so the install belongs here too. No later start re-enters. + spawn_dev_skill_install(dev_skill_install_command(base_location)); return Ok(()); } false => { @@ -546,17 +561,29 @@ pub fn scaffold_iac_layout( println!("Deployment canceled"); } + // Last, so the install only ever follows a scaffold that finished. Every + // exit above this line is an `Err` from a `?`, prompt cancellations + // included, and leaves no install running behind it. The one exception is + // the early `Ok(())` when `main.tx` already exists, which is a finished + // scaffold and starts its own. `txtx.yml` is written well above here, so + // the next start is no longer a scaffold and neither call site can fire twice. + spawn_dev_skill_install(dev_skill_install_command(base_location)); + Ok(()) } #[cfg(test)] mod tests { use std::{ + path::Path, process::Command, time::{Duration, Instant}, }; - use super::{DEV_SKILL_REPO, dev_skill_install_command, spawn_dev_skill_install}; + use super::{ + DEV_SKILL_INSTALLER, DEV_SKILL_REPO, FileLocation, dev_skill_install_command, + spawn_dev_skill_install, + }; #[cfg(unix)] fn sh(script: &str) -> Command { @@ -566,14 +593,48 @@ mod tests { } /// #567 supplied this invocation literally: the `-y` flags keep it off a - /// prompt and `--skill "*"` is what makes it the whole bundle. + /// prompt and `--skill "*"` is what makes it the whole bundle. The one + /// departure from its text is the pinned version, which is here in full so + /// that dropping the pin has to fail this. #[test] fn the_install_is_the_command_issue_567_asked_for() { - let command = dev_skill_install_command(); + let base = FileLocation::from_path_string("/tmp/surfpool-567-scaffold").unwrap(); + let command = dev_skill_install_command(&base); assert_eq!(command.get_program(), "npx"); assert_eq!( command.get_args().collect::>(), - ["-y", "skills", "add", DEV_SKILL_REPO, "--skill", "*", "-y"] + [ + "-y", + DEV_SKILL_INSTALLER, + "add", + DEV_SKILL_REPO, + "--skill", + "*", + "-y" + ] + ); + let (package, version) = DEV_SKILL_INSTALLER + .split_once('@') + .expect("installer is pinned"); + assert_eq!(package, "skills"); + assert!( + version.split('.').count() == 3 + && version + .split('.') + .all(|p| !p.is_empty() && p.bytes().all(|b| b.is_ascii_digit())), + "the installer must stay pinned to an exact version, not a range: {version}" + ); + } + + /// The install writes into the project being scaffolded, which is the + /// manifest's directory rather than the shell's. `surfpool start -m + /// ../elsewhere/txtx.yml` scaffolds a tree the caller is not standing in. + #[test] + fn the_install_runs_in_the_scaffolded_project_not_the_process_cwd() { + let base = FileLocation::from_path_string("/tmp/surfpool-567-scaffold").unwrap(); + assert_eq!( + dev_skill_install_command(&base).get_current_dir(), + Some(Path::new("/tmp/surfpool-567-scaffold")) ); } From ad6bfaa6a67774c395d6763acfcade7cb09f94ba Mon Sep 17 00:00:00 2001 From: Michael Moffett Date: Mon, 24 Aug 2026 08:19:21 -0600 Subject: [PATCH 3/3] fix(cli): stop a declined confirmation from starting the install Declining the deployment prompt printed "Deployment canceled" and fell through to the install, which spawned against the project anyway. Route the install through the confirmation so a decline builds no command, and replace the comment above it, which claimed every exit on that path was an Err. --- crates/cli/src/scaffold/mod.rs | 29 +++++++++++++++++++++-------- 1 file changed, 21 insertions(+), 8 deletions(-) diff --git a/crates/cli/src/scaffold/mod.rs b/crates/cli/src/scaffold/mod.rs index 99b2914b1..4f4d97824 100644 --- a/crates/cli/src/scaffold/mod.rs +++ b/crates/cli/src/scaffold/mod.rs @@ -180,6 +180,13 @@ fn spawn_dev_skill_install(mut command: Command) { }); } +/// The install that follows the confirmation, or nothing. A declined +/// confirmation cancels the deployment, and the install is part of what it +/// cancels. +fn install_after_confirmation(confirmation: bool, base_location: &FileLocation) -> Option { + confirmation.then(|| dev_skill_install_command(base_location)) +} + pub fn scaffold_in_memory_iac( framework: &Framework, programs: &[ProgramMetadata], @@ -561,13 +568,10 @@ pub fn scaffold_iac_layout( println!("Deployment canceled"); } - // Last, so the install only ever follows a scaffold that finished. Every - // exit above this line is an `Err` from a `?`, prompt cancellations - // included, and leaves no install running behind it. The one exception is - // the early `Ok(())` when `main.tx` already exists, which is a finished - // scaffold and starts its own. `txtx.yml` is written well above here, so - // the next start is no longer a scaffold and neither call site can fire twice. - spawn_dev_skill_install(dev_skill_install_command(base_location)); + // Last, so the install only follows a scaffold that finished. + if let Some(command) = install_after_confirmation(confirmation, base_location) { + spawn_dev_skill_install(command); + } Ok(()) } @@ -582,7 +586,7 @@ mod tests { use super::{ DEV_SKILL_INSTALLER, DEV_SKILL_REPO, FileLocation, dev_skill_install_command, - spawn_dev_skill_install, + install_after_confirmation, spawn_dev_skill_install, }; #[cfg(unix)] @@ -638,6 +642,15 @@ mod tests { ); } + /// Declining the confirmation cancels the deployment, and the install is + /// part of what it cancels. Nothing is built, so nothing is spawned. + #[test] + fn a_declined_confirmation_starts_no_install() { + let base = FileLocation::from_path_string("/tmp/surfpool-567-scaffold").unwrap(); + assert!(install_after_confirmation(false, &base).is_none()); + assert!(install_after_confirmation(true, &base).is_some()); + } + /// The three ways this goes wrong on a real machine: no Node at all, an /// install that fails, an install that hangs. Each returns at once and /// returns nothing, so the scaffold has neither a value to branch on nor a