From 323a1374ae661f9867ff148ba13de5367adcea1a Mon Sep 17 00:00:00 2001 From: Michael Moffett Date: Thu, 13 Aug 2026 12:08:01 -0600 Subject: [PATCH 01/12] 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 3dd177de208e4c94779cf3e487eb12d9c9b7d3af Mon Sep 17 00:00:00 2001 From: Michael Moffett Date: Thu, 13 Aug 2026 13:43:55 -0600 Subject: [PATCH 02/12] 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 a99e8ed9dd6571d49a3811d6e3e0a3d5be3a6bff Mon Sep 17 00:00:00 2001 From: Michael Moffett Date: Mon, 24 Aug 2026 08:19:21 -0600 Subject: [PATCH 03/12] 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 From fa48d669e0fae7646e1b66a1a346a376479334aa Mon Sep 17 00:00:00 2001 From: Michael Moffett Date: Mon, 24 Aug 2026 14:37:08 -0600 Subject: [PATCH 04/12] fix(cli): drop the install from the existing-runbook path That call site sat in the arm taken when runbooks/deployment/main.tx is already present, and returned before the confirmation is bound further down, so on that path the install started with nothing to decline. It is removed rather than routed through a prompt because the arm exists to return early. A project with a runbooks tree and no txtx.yml now scaffolds the manifest and installs nothing; the only install left is the one behind the confirmation. --- crates/cli/src/scaffold/mod.rs | 3 --- 1 file changed, 3 deletions(-) diff --git a/crates/cli/src/scaffold/mod.rs b/crates/cli/src/scaffold/mod.rs index 4f4d97824..c13c4bd1e 100644 --- a/crates/cli/src/scaffold/mod.rs +++ b/crates/cli/src/scaffold/mod.rs @@ -481,9 +481,6 @@ 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 => { From a3fc26cc63dea2c9caab246b6c78dc0b519263a5 Mon Sep 17 00:00:00 2001 From: Michael Moffett Date: Sat, 29 Aug 2026 11:39:30 -0600 Subject: [PATCH 05/12] feat(cli): ask before installing the dev skill The install rode the deployment confirmation, so confirming a deploy also consented to a skill nobody had been asked about. It gets its own gate: silent when the skill is already present in the project or in the home directory, silent when a previous decline was recorded, otherwise one prompt naming what lands and where. The recorded decline is checked before --yes rather than after, so a machine that declined once is not talked round by a flag. Declining the deployment now records nothing, because that is "not now", not "never". The invocation also names a single agent. Detecting none of its own agents installed, the installer fanned the skill out to all 77 in its registry, dropping .claude/, a non-hidden agent/ and skills-lock.json into a project that has no .gitignore yet. Naming universal reduces that to the canonical .agents/skills copy and the lock file, which is what the prompt is now able to promise. --- crates/cli/src/scaffold/mod.rs | 207 ++++++++++++++++++++++++++++++--- 1 file changed, 189 insertions(+), 18 deletions(-) diff --git a/crates/cli/src/scaffold/mod.rs b/crates/cli/src/scaffold/mod.rs index c13c4bd1e..59c9821c7 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}, + path::Path, process::{Command, Stdio}, }; @@ -19,7 +20,10 @@ use txtx_core::{ types::RunbookSources, }; -use crate::{cli::DEFAULT_SOLANA_KEYPAIR_PATH, types::Framework}; +use crate::{ + cli::{DEFAULT_SOLANA_KEYPAIR_PATH, get_home_dir}, + types::Framework, +}; pub const SURFPOOL_README_TEMPLATE: &str = include_str!("./templates/readme.md.mst"); @@ -143,8 +147,22 @@ const DEV_SKILL_REPO: &str = "https://github.com/solana-foundation/solana-dev-sk /// 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. +/// Named because the installer, detecting none of its own agents, otherwise fans the +/// skill out to every one of the 77 in its registry: `.claude/`, a non-hidden `agent/` +/// and a lock file all land in a project that asked for none of them. `universal` is +/// its own registry key for the canonical `.agents/skills` copy and symlinks nowhere. +const DEV_SKILL_AGENT: &str = "universal"; + +/// Where `universal` puts the skill, under the project or under a home that took it +/// globally. +const DEV_SKILL_DIR: &str = ".agents/skills/solana-dev"; + +/// A remembered "no". Its existence is the whole answer, so deleting it asks again. +const DEV_SKILL_DECLINED_MARKER: &str = ".config/surfpool/dev-skill-declined"; + +/// Builds the skill install as issue #567 specifies it, pinned, restricted to the one +/// agent whose layout the prompt names, 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([ @@ -154,6 +172,8 @@ fn dev_skill_install_command(base_location: &FileLocation) -> Command { DEV_SKILL_REPO, "--skill", "*", + "--agent", + DEV_SKILL_AGENT, "-y", ]); command.current_dir(base_location.expect_path_buf()); @@ -180,11 +200,64 @@ 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)) +/// Both scopes the install can already have happened in: this project, or a home +/// that took the skill globally. +fn dev_skill_installed(base: &Path, home: &Path) -> bool { + base.join(DEV_SKILL_DIR).exists() || home.join(DEV_SKILL_DIR).exists() +} + +fn dev_skill_declined(home: &Path) -> bool { + home.join(DEV_SKILL_DECLINED_MARKER).exists() +} + +/// `None` when the prompt could not be put on screen at all, which is not an answer +/// and so is not remembered. +fn prompt_for_dev_skill(theme: &ColorfulTheme) -> Option { + Confirm::with_theme(theme) + .with_prompt(format!( + "Install the solana-dev skill? It writes {DEV_SKILL_DIR} and skills-lock.json here" + )) + .default(true) + .interact() + .ok() +} + +/// Failures ignored the way `spawn_dev_skill_install` ignores its own: a marker that +/// would not write costs one more prompt later, not a failed scaffold now. +fn record_dev_skill_declined(home: &Path) { + let marker = home.join(DEV_SKILL_DECLINED_MARKER); + if let Some(parent) = marker.parent() { + let _ = fs::create_dir_all(parent); + } + let _ = File::create(marker); +} + +/// The gate, in the order that decides it: an install already on disk and a recorded +/// no both outrank `--yes`, so a second scaffold stays silent and a machine that has +/// declined once is never asked again. `ask` is reached only when nothing on disk has +/// already answered. Paths are arguments so this is testable without a home directory. +fn dev_skill_install_if_wanted( + base_location: &FileLocation, + base: &Path, + home: &Path, + auto_accept: bool, + ask: impl FnOnce() -> Option, +) -> Option { + if dev_skill_installed(base, home) || dev_skill_declined(home) { + return None; + } + let consented = match auto_accept { + true => true, + false => match ask() { + Some(true) => true, + Some(false) => { + record_dev_skill_declined(home); + false + } + None => false, + }, + }; + consented.then(|| dev_skill_install_command(base_location)) } pub fn scaffold_in_memory_iac( @@ -565,9 +638,19 @@ pub fn scaffold_iac_layout( println!("Deployment canceled"); } - // Last, so the install only follows a scaffold that finished. - if let Some(command) = install_after_confirmation(confirmation, base_location) { + // Last, so the install only follows a scaffold that finished. No longer keyed to + // `confirmation`: declining the deployment is "not now", and never was "never". + let home = get_home_dir(); + let base = base_location.expect_path_buf(); + if let Some(command) = dev_skill_install_if_wanted( + base_location, + &base, + Path::new(&home), + auto_generate_runbooks, + || prompt_for_dev_skill(&theme), + ) { spawn_dev_skill_install(command); + println!("{} {}", green!("Installing"), DEV_SKILL_DIR); } Ok(()) @@ -576,16 +659,28 @@ pub fn scaffold_iac_layout( #[cfg(test)] mod tests { use std::{ + fs::{self, File}, path::Path, process::Command, time::{Duration, Instant}, }; + use tempfile::TempDir; + use super::{ - DEV_SKILL_INSTALLER, DEV_SKILL_REPO, FileLocation, dev_skill_install_command, - install_after_confirmation, spawn_dev_skill_install, + DEV_SKILL_AGENT, DEV_SKILL_DECLINED_MARKER, DEV_SKILL_DIR, DEV_SKILL_INSTALLER, + DEV_SKILL_REPO, FileLocation, dev_skill_install_command, dev_skill_install_if_wanted, + spawn_dev_skill_install, }; + /// Every gate test runs against these, never a real home directory. + fn scratch() -> (TempDir, TempDir, FileLocation) { + let base = TempDir::new().unwrap(); + let home = TempDir::new().unwrap(); + let location = FileLocation::from_path_string(base.path().to_str().unwrap()).unwrap(); + (base, home, location) + } + #[cfg(unix)] fn sh(script: &str) -> Command { let mut command = Command::new("sh"); @@ -611,9 +706,14 @@ mod tests { DEV_SKILL_REPO, "--skill", "*", + "--agent", + DEV_SKILL_AGENT, "-y" ] ); + // "*" is the installer's own alias for every agent it knows, so naming one + // agent and naming all of them are one typo apart. + assert_ne!(DEV_SKILL_AGENT, "*"); let (package, version) = DEV_SKILL_INSTALLER .split_once('@') .expect("installer is pinned"); @@ -639,13 +739,84 @@ mod tests { ); } - /// Declining the confirmation cancels the deployment, and the install is - /// part of what it cancels. Nothing is built, so nothing is spawned. + /// A skill already on disk is the reviewer's first item: say nothing, do nothing. + /// Either scope counts, since a globally installed skill is already available here. #[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()); + fn an_installed_skill_is_left_alone() { + for (in_base, in_home) in [(true, false), (false, true)] { + let (base, home, location) = scratch(); + let root = if in_base { base.path() } else { home.path() }; + fs::create_dir_all(root.join(DEV_SKILL_DIR)).unwrap(); + assert!( + dev_skill_install_if_wanted(&location, base.path(), home.path(), true, || { + panic!("an installed skill must not prompt") + }) + .is_none(), + "installed in base={in_base} home={in_home} still produced an install" + ); + } + } + + /// The remembered no, and the reason it is checked before `--yes` rather than + /// after: "never" has to survive the flag, or a CI machine relitigates it hourly. + #[test] + fn a_recorded_decline_outranks_the_yes_flag() { + let (base, home, location) = scratch(); + let marker = home.path().join(DEV_SKILL_DECLINED_MARKER); + fs::create_dir_all(marker.parent().unwrap()).unwrap(); + File::create(&marker).unwrap(); + assert!( + dev_skill_install_if_wanted(&location, base.path(), home.path(), true, || { + panic!("a recorded decline must not prompt") + }) + .is_none() + ); + } + + /// `--yes` is the codebase's existing "answered in advance", so with nothing on + /// disk saying otherwise it installs without prompting. + #[test] + fn the_yes_flag_installs_without_prompting() { + let (base, home, location) = scratch(); + assert!( + dev_skill_install_if_wanted(&location, base.path(), home.path(), true, || { + panic!("--yes must not prompt") + }) + .is_some() + ); + } + + /// The property the old confirmation test guarded, re-expressed against the gate + /// that replaced it: a no starts no install. It now also has to be remembered. + #[test] + fn a_declined_prompt_starts_no_install_and_is_remembered() { + let (base, home, location) = scratch(); + assert!( + dev_skill_install_if_wanted(&location, base.path(), home.path(), false, || Some(false)) + .is_none() + ); + assert!( + home.path().join(DEV_SKILL_DECLINED_MARKER).exists(), + "a decline that is not written down is a decline that gets asked again" + ); + assert!( + dev_skill_install_if_wanted(&location, base.path(), home.path(), false, || { + panic!("the recorded decline must not prompt again") + }) + .is_none() + ); + } + + /// Accepting is the only path that installs, and a yes is deliberately not + /// written down: installs are per project, so the question is asked per project. + #[test] + fn an_accepted_prompt_installs_and_records_nothing() { + let (base, home, location) = scratch(); + assert!( + dev_skill_install_if_wanted(&location, base.path(), home.path(), false, || Some(true)) + .is_some() + ); + assert!(!home.path().join(DEV_SKILL_DECLINED_MARKER).exists()); } /// The three ways this goes wrong on a real machine: no Node at all, an From 5f7a694d828f0b6d0d94f94c2585f4b222358cb7 Mon Sep 17 00:00:00 2001 From: Michael Moffett Date: Sat, 29 Aug 2026 11:39:51 -0600 Subject: [PATCH 06/12] feat(cli): move the skill installer pin to 1.5.23 The 1.5.22 to 1.5.23 diff is 578 lines and touches none of install, lock-file writing, the .agents constants, the #ref clone parsing or symlinking. It is the interactive picker, which stdio-nulled invocation never reaches, plus a git tree-hash helper confined to the global update command. Its one change on our path adds an agent to the registry, which the named agent makes moot. --- crates/cli/src/scaffold/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/cli/src/scaffold/mod.rs b/crates/cli/src/scaffold/mod.rs index 59c9821c7..868973266 100644 --- a/crates/cli/src/scaffold/mod.rs +++ b/crates/cli/src/scaffold/mod.rs @@ -145,7 +145,7 @@ const DEV_SKILL_REPO: &str = "https://github.com/solana-foundation/solana-dev-sk /// 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"; +const DEV_SKILL_INSTALLER: &str = "skills@1.5.23"; /// Named because the installer, detecting none of its own agents, otherwise fans the /// skill out to every one of the 77 in its registry: `.claude/`, a non-hidden `agent/` From 7848cfe7a088fecb9eeb12e1e0503e14cf23bfea Mon Sep 17 00:00:00 2001 From: Michael Moffett Date: Sat, 29 Aug 2026 15:46:53 -0600 Subject: [PATCH 07/12] docs(cli): cut the comment density to the neighbouring files 57 comment lines to 37 across the production half, ratio 0.107 to 0.070, against scaffold/mod.rs at 0.058 and its siblings at 0.00 to 0.11. Zero code lines changed. Their file carried 25 comment lines and this PR had added 32 more. Comments that narrated the line below them are gone; the ones carrying a decision the code cannot show are kept, shortened where prose had crept in. Kept in full: the DEV_SKILL_AGENT block, because the 77-agent fanout it prevents appears nowhere in the code. Kept, shortened: the gate ordering, since 'a recorded no outranks --yes' is a decision and not a mechanism. Verified: cargo +nightly fmt --check PASS, clippy clean on this file (the 11 crate warnings are pre-existing and none are here). The test bar did not run: postgres is not up on this host after a restart, and their CI supplies it as a service container. A comments-only diff cannot fail tests without failing the compile, which it does not. --- crates/cli/src/scaffold/mod.rs | 36 ++++++++-------------------------- 1 file changed, 8 insertions(+), 28 deletions(-) diff --git a/crates/cli/src/scaffold/mod.rs b/crates/cli/src/scaffold/mod.rs index 868973266..9841e658a 100644 --- a/crates/cli/src/scaffold/mod.rs +++ b/crates/cli/src/scaffold/mod.rs @@ -141,10 +141,7 @@ impl ProgramMetadata { const DEV_SKILL_REPO: &str = "https://github.com/solana-foundation/solana-dev-skill"; -/// 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. +/// Exact, not a range: anything looser installs whatever the registry calls latest that day. const DEV_SKILL_INSTALLER: &str = "skills@1.5.23"; /// Named because the installer, detecting none of its own agents, otherwise fans the @@ -153,16 +150,10 @@ const DEV_SKILL_INSTALLER: &str = "skills@1.5.23"; /// its own registry key for the canonical `.agents/skills` copy and symlinks nowhere. const DEV_SKILL_AGENT: &str = "universal"; -/// Where `universal` puts the skill, under the project or under a home that took it -/// globally. const DEV_SKILL_DIR: &str = ".agents/skills/solana-dev"; -/// A remembered "no". Its existence is the whole answer, so deleting it asks again. const DEV_SKILL_DECLINED_MARKER: &str = ".config/surfpool/dev-skill-declined"; -/// Builds the skill install as issue #567 specifies it, pinned, restricted to the one -/// agent whose layout the prompt names, 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([ @@ -180,11 +171,8 @@ fn dev_skill_install_command(base_location: &FileLocation) -> Command { 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. +/// Fire-and-forget: this sits on the path to booting a surfnet, so a missing Node or a failed +/// install is silence rather than an error, and the wait that reaps the child runs off-thread. fn spawn_dev_skill_install(mut command: Command) { let Ok(mut child) = command .stdin(Stdio::null()) @@ -194,14 +182,11 @@ fn spawn_dev_skill_install(mut command: Command) { 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(); }); } -/// Both scopes the install can already have happened in: this project, or a home -/// that took the skill globally. fn dev_skill_installed(base: &Path, home: &Path) -> bool { base.join(DEV_SKILL_DIR).exists() || home.join(DEV_SKILL_DIR).exists() } @@ -210,8 +195,6 @@ fn dev_skill_declined(home: &Path) -> bool { home.join(DEV_SKILL_DECLINED_MARKER).exists() } -/// `None` when the prompt could not be put on screen at all, which is not an answer -/// and so is not remembered. fn prompt_for_dev_skill(theme: &ColorfulTheme) -> Option { Confirm::with_theme(theme) .with_prompt(format!( @@ -222,8 +205,6 @@ fn prompt_for_dev_skill(theme: &ColorfulTheme) -> Option { .ok() } -/// Failures ignored the way `spawn_dev_skill_install` ignores its own: a marker that -/// would not write costs one more prompt later, not a failed scaffold now. fn record_dev_skill_declined(home: &Path) { let marker = home.join(DEV_SKILL_DECLINED_MARKER); if let Some(parent) = marker.parent() { @@ -232,10 +213,9 @@ fn record_dev_skill_declined(home: &Path) { let _ = File::create(marker); } -/// The gate, in the order that decides it: an install already on disk and a recorded -/// no both outrank `--yes`, so a second scaffold stays silent and a machine that has -/// declined once is never asked again. `ask` is reached only when nothing on disk has -/// already answered. Paths are arguments so this is testable without a home directory. +/// An install already on disk and a recorded no both outrank `--yes`, so a second scaffold stays +/// silent and a machine that has declined once is never asked again. Paths are arguments so the +/// gate is testable without a real home directory. fn dev_skill_install_if_wanted( base_location: &FileLocation, base: &Path, @@ -638,8 +618,8 @@ pub fn scaffold_iac_layout( println!("Deployment canceled"); } - // Last, so the install only follows a scaffold that finished. No longer keyed to - // `confirmation`: declining the deployment is "not now", and never was "never". + // Last, so the install only follows a scaffold that finished, and deliberately not keyed to + // `confirmation`: declining the deployment is "not now", not "never". let home = get_home_dir(); let base = base_location.expect_path_buf(); if let Some(command) = dev_skill_install_if_wanted( From 4ac6fb5890a5b1384bfb3a032eeb62461ecdd072 Mon Sep 17 00:00:00 2001 From: Michael Moffett Date: Sat, 29 Aug 2026 17:02:43 -0600 Subject: [PATCH 08/12] docs(cli): say why the install passes -y twice --- crates/cli/src/scaffold/mod.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/crates/cli/src/scaffold/mod.rs b/crates/cli/src/scaffold/mod.rs index 9841e658a..f37d30a29 100644 --- a/crates/cli/src/scaffold/mod.rs +++ b/crates/cli/src/scaffold/mod.rs @@ -154,6 +154,7 @@ const DEV_SKILL_DIR: &str = ".agents/skills/solana-dev"; const DEV_SKILL_DECLINED_MARKER: &str = ".config/surfpool/dev-skill-declined"; +/// Two `-y`, two programs: npx's auto-installs the package, the skills CLI's skips its prompts. fn dev_skill_install_command(base_location: &FileLocation) -> Command { let mut command = Command::new("npx"); command.args([ From d339d869f12fe4379e4daf7b9abbd6208bf1a029 Mon Sep 17 00:00:00 2001 From: Michael Moffett Date: Sat, 29 Aug 2026 17:03:12 -0600 Subject: [PATCH 09/12] feat(cli): cover the undisplayable prompt, and say why it records nothing --- crates/cli/src/scaffold/mod.rs | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/crates/cli/src/scaffold/mod.rs b/crates/cli/src/scaffold/mod.rs index f37d30a29..b178421b7 100644 --- a/crates/cli/src/scaffold/mod.rs +++ b/crates/cli/src/scaffold/mod.rs @@ -235,6 +235,7 @@ fn dev_skill_install_if_wanted( record_dev_skill_declined(home); false } + // A prompt that could not be shown is not an answer, so nothing is recorded. None => false, }, }; @@ -800,6 +801,18 @@ mod tests { assert!(!home.path().join(DEV_SKILL_DECLINED_MARKER).exists()); } + /// A prompt that could not be shown is not a decline, so it is not remembered as one and + /// the next scaffold still asks. + #[test] + fn an_undisplayable_prompt_installs_nothing_and_records_nothing() { + let (base, home, location) = scratch(); + assert!( + dev_skill_install_if_wanted(&location, base.path(), home.path(), false, || None) + .is_none() + ); + assert!(!home.path().join(DEV_SKILL_DECLINED_MARKER).exists()); + } + /// 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 From 0a333d7045f2c5ab75ca9865e7f20df96df753e4 Mon Sep 17 00:00:00 2001 From: Michael Moffett Date: Sat, 29 Aug 2026 17:03:35 -0600 Subject: [PATCH 10/12] feat(cli): fold the two --yes tests into the one property they share --- crates/cli/src/scaffold/mod.rs | 47 +++++++++++++++------------------- 1 file changed, 21 insertions(+), 26 deletions(-) diff --git a/crates/cli/src/scaffold/mod.rs b/crates/cli/src/scaffold/mod.rs index b178421b7..5f1e895f1 100644 --- a/crates/cli/src/scaffold/mod.rs +++ b/crates/cli/src/scaffold/mod.rs @@ -739,33 +739,28 @@ mod tests { } } - /// The remembered no, and the reason it is checked before `--yes` rather than - /// after: "never" has to survive the flag, or a CI machine relitigates it hourly. + /// Under `--yes`, the marker decides. The remembered no is read before the flag rather + /// than after, or a CI machine relitigates it hourly; absent one, `--yes` is the + /// codebase's existing "answered in advance". Neither case may reach the prompt. #[test] - fn a_recorded_decline_outranks_the_yes_flag() { - let (base, home, location) = scratch(); - let marker = home.path().join(DEV_SKILL_DECLINED_MARKER); - fs::create_dir_all(marker.parent().unwrap()).unwrap(); - File::create(&marker).unwrap(); - assert!( - dev_skill_install_if_wanted(&location, base.path(), home.path(), true, || { - panic!("a recorded decline must not prompt") - }) - .is_none() - ); - } - - /// `--yes` is the codebase's existing "answered in advance", so with nothing on - /// disk saying otherwise it installs without prompting. - #[test] - fn the_yes_flag_installs_without_prompting() { - let (base, home, location) = scratch(); - assert!( - dev_skill_install_if_wanted(&location, base.path(), home.path(), true, || { - panic!("--yes must not prompt") - }) - .is_some() - ); + fn under_the_yes_flag_the_marker_decides() { + for declined in [true, false] { + let (base, home, location) = scratch(); + if declined { + let marker = home.path().join(DEV_SKILL_DECLINED_MARKER); + fs::create_dir_all(marker.parent().unwrap()).unwrap(); + File::create(&marker).unwrap(); + } + let install = + dev_skill_install_if_wanted(&location, base.path(), home.path(), true, || { + panic!("--yes must not prompt") + }); + assert_eq!( + install.is_none(), + declined, + "--yes with a recorded decline={declined} decided the wrong way" + ); + } } /// The property the old confirmation test guarded, re-expressed against the gate From 146a8a55c85e0c4585bb5fb0ae9d6f99f0d0e33d Mon Sep 17 00:00:00 2001 From: Michael Moffett Date: Sat, 29 Aug 2026 17:04:03 -0600 Subject: [PATCH 11/12] docs(cli): drop the test comments that restate their asserts --- crates/cli/src/scaffold/mod.rs | 25 ++++++++++--------------- 1 file changed, 10 insertions(+), 15 deletions(-) diff --git a/crates/cli/src/scaffold/mod.rs b/crates/cli/src/scaffold/mod.rs index 5f1e895f1..a67168e5a 100644 --- a/crates/cli/src/scaffold/mod.rs +++ b/crates/cli/src/scaffold/mod.rs @@ -670,10 +670,8 @@ mod tests { command } - /// #567 supplied this invocation literally: the `-y` flags keep it off a - /// 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. + /// #567 supplied this invocation literally. 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 base = FileLocation::from_path_string("/tmp/surfpool-567-scaffold").unwrap(); @@ -709,9 +707,8 @@ mod tests { ); } - /// 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. + /// `surfpool start -m ../elsewhere/txtx.yml` scaffolds a tree the caller is not standing in, + /// so the manifest's directory is the target and the shell's is not. #[test] fn the_install_runs_in_the_scaffolded_project_not_the_process_cwd() { let base = FileLocation::from_path_string("/tmp/surfpool-567-scaffold").unwrap(); @@ -763,8 +760,8 @@ mod tests { } } - /// The property the old confirmation test guarded, re-expressed against the gate - /// that replaced it: a no starts no install. It now also has to be remembered. + /// The property the old confirmation test guarded, re-expressed against the gate that + /// replaced it, with the memory the gate added. #[test] fn a_declined_prompt_starts_no_install_and_is_remembered() { let (base, home, location) = scratch(); @@ -784,8 +781,8 @@ mod tests { ); } - /// Accepting is the only path that installs, and a yes is deliberately not - /// written down: installs are per project, so the question is asked per project. + /// A yes is deliberately not written down: installs are per project, so the question is + /// asked per project. #[test] fn an_accepted_prompt_installs_and_records_nothing() { let (base, home, location) = scratch(); @@ -808,10 +805,8 @@ mod tests { assert!(!home.path().join(DEV_SKILL_DECLINED_MARKER).exists()); } - /// 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. + /// The three ways this goes wrong on a real machine, in order: no Node at all, an install + /// that fails, an install that hangs. The scaffold is held by none of them. #[cfg(unix)] #[test] fn no_outcome_of_the_install_reaches_the_scaffold() { From 03570b075563a32d9c86da3f698a3e919f417e2f Mon Sep 17 00:00:00 2001 From: Michael Moffett Date: Sun, 30 Aug 2026 11:39:20 -0600 Subject: [PATCH 12/12] feat(cli): record the yes too, so neither answer is asked twice --- crates/cli/src/scaffold/mod.rs | 63 +++++++++++++++++++++------------- 1 file changed, 40 insertions(+), 23 deletions(-) diff --git a/crates/cli/src/scaffold/mod.rs b/crates/cli/src/scaffold/mod.rs index a67168e5a..899da3e8c 100644 --- a/crates/cli/src/scaffold/mod.rs +++ b/crates/cli/src/scaffold/mod.rs @@ -154,6 +154,8 @@ const DEV_SKILL_DIR: &str = ".agents/skills/solana-dev"; const DEV_SKILL_DECLINED_MARKER: &str = ".config/surfpool/dev-skill-declined"; +const DEV_SKILL_ACCEPTED_MARKER: &str = ".config/surfpool/dev-skill-accepted"; + /// Two `-y`, two programs: npx's auto-installs the package, the skills CLI's skips its prompts. fn dev_skill_install_command(base_location: &FileLocation) -> Command { let mut command = Command::new("npx"); @@ -192,8 +194,16 @@ fn dev_skill_installed(base: &Path, home: &Path) -> bool { base.join(DEV_SKILL_DIR).exists() || home.join(DEV_SKILL_DIR).exists() } -fn dev_skill_declined(home: &Path) -> bool { - home.join(DEV_SKILL_DECLINED_MARKER).exists() +/// Both answers are ours to keep: `DEV_SKILL_DIR` is written by an installer we neither wait on +/// nor control, so a yes inferred from it is a yes asked again forever. A no wins a split pair. +fn dev_skill_answer(home: &Path) -> Option { + if home.join(DEV_SKILL_DECLINED_MARKER).exists() { + Some(false) + } else if home.join(DEV_SKILL_ACCEPTED_MARKER).exists() { + Some(true) + } else { + None + } } fn prompt_for_dev_skill(theme: &ColorfulTheme) -> Option { @@ -201,21 +211,23 @@ fn prompt_for_dev_skill(theme: &ColorfulTheme) -> Option { .with_prompt(format!( "Install the solana-dev skill? It writes {DEV_SKILL_DIR} and skills-lock.json here" )) - .default(true) + .default(false) .interact() .ok() } -fn record_dev_skill_declined(home: &Path) { - let marker = home.join(DEV_SKILL_DECLINED_MARKER); +fn record_dev_skill_answer(home: &Path, consented: bool) { + let marker = home.join(match consented { + true => DEV_SKILL_ACCEPTED_MARKER, + false => DEV_SKILL_DECLINED_MARKER, + }); if let Some(parent) = marker.parent() { let _ = fs::create_dir_all(parent); } let _ = File::create(marker); } -/// An install already on disk and a recorded no both outrank `--yes`, so a second scaffold stays -/// silent and a machine that has declined once is never asked again. Paths are arguments so the +/// A recorded answer and an install on disk both outrank `--yes`. Paths are arguments so the /// gate is testable without a real home directory. fn dev_skill_install_if_wanted( base_location: &FileLocation, @@ -224,16 +236,16 @@ fn dev_skill_install_if_wanted( auto_accept: bool, ask: impl FnOnce() -> Option, ) -> Option { - if dev_skill_installed(base, home) || dev_skill_declined(home) { + if dev_skill_installed(base, home) { return None; } - let consented = match auto_accept { - true => true, - false => match ask() { - Some(true) => true, - Some(false) => { - record_dev_skill_declined(home); - false + let consented = match dev_skill_answer(home) { + Some(recorded) => recorded, + None if auto_accept => true, + None => match ask() { + Some(answer) => { + record_dev_skill_answer(home, answer); + answer } // A prompt that could not be shown is not an answer, so nothing is recorded. None => false, @@ -632,7 +644,7 @@ pub fn scaffold_iac_layout( || prompt_for_dev_skill(&theme), ) { spawn_dev_skill_install(command); - println!("{} {}", green!("Installing"), DEV_SKILL_DIR); + println!("{} {}", green!("Started installer for"), DEV_SKILL_DIR); } Ok(()) @@ -651,8 +663,8 @@ mod tests { use super::{ DEV_SKILL_AGENT, DEV_SKILL_DECLINED_MARKER, DEV_SKILL_DIR, DEV_SKILL_INSTALLER, - DEV_SKILL_REPO, FileLocation, dev_skill_install_command, dev_skill_install_if_wanted, - spawn_dev_skill_install, + DEV_SKILL_REPO, FileLocation, dev_skill_answer, dev_skill_install_command, + dev_skill_install_if_wanted, spawn_dev_skill_install, }; /// Every gate test runs against these, never a real home directory. @@ -781,16 +793,21 @@ mod tests { ); } - /// A yes is deliberately not written down: installs are per project, so the question is - /// asked per project. + /// The install writes a directory a third party owns, and #567's own installer currently + /// writes none, so a yes not written down here is a yes asked again on every scaffold. #[test] - fn an_accepted_prompt_installs_and_records_nothing() { + fn an_accepted_prompt_installs_and_is_remembered() { let (base, home, location) = scratch(); assert!( dev_skill_install_if_wanted(&location, base.path(), home.path(), false, || Some(true)) .is_some() ); - assert!(!home.path().join(DEV_SKILL_DECLINED_MARKER).exists()); + assert!( + dev_skill_install_if_wanted(&location, base.path(), home.path(), false, || { + panic!("the recorded accept must not prompt again") + }) + .is_some() + ); } /// A prompt that could not be shown is not a decline, so it is not remembered as one and @@ -802,7 +819,7 @@ mod tests { dev_skill_install_if_wanted(&location, base.path(), home.path(), false, || None) .is_none() ); - assert!(!home.path().join(DEV_SKILL_DECLINED_MARKER).exists()); + assert!(dev_skill_answer(home.path()).is_none()); } /// The three ways this goes wrong on a real machine, in order: no Node at all, an install