From fe4f9f57cac6e136e2ae23c913c371f762e6c411 Mon Sep 17 00:00:00 2001 From: Michael Moffett Date: Thu, 13 Aug 2026 12:08:01 -0600 Subject: [PATCH 01/15] 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 02/15] 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 03/15] 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 8c3b95b9842be22b494281bb225a94954698243a Mon Sep 17 00:00:00 2001 From: Michael Moffett Date: Mon, 24 Aug 2026 14:37:08 -0600 Subject: [PATCH 04/15] 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 10341c8c01dfbde3f5e051e5fff47c96442584d2 Mon Sep 17 00:00:00 2001 From: Michael Moffett Date: Sat, 29 Aug 2026 11:39:30 -0600 Subject: [PATCH 05/15] 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 a05fcff2f90b21ae0e5945573fdcc3ccfb6d81e8 Mon Sep 17 00:00:00 2001 From: Michael Moffett Date: Sat, 29 Aug 2026 11:39:51 -0600 Subject: [PATCH 06/15] 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 357a4a511338644b3df886c7f4242f44a792cd6e Mon Sep 17 00:00:00 2001 From: Michael Moffett Date: Sat, 29 Aug 2026 15:46:53 -0600 Subject: [PATCH 07/15] 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 dd5d7f4ca5e03d4e855b82367492675811dc0299 Mon Sep 17 00:00:00 2001 From: Michael Moffett Date: Sat, 29 Aug 2026 17:02:43 -0600 Subject: [PATCH 08/15] 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 fca773965308d0be10ed8d730a89e336a84198c4 Mon Sep 17 00:00:00 2001 From: Michael Moffett Date: Sat, 29 Aug 2026 17:03:12 -0600 Subject: [PATCH 09/15] 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 a75693e727180de5153caeb510982ab3ecbff835 Mon Sep 17 00:00:00 2001 From: Michael Moffett Date: Sat, 29 Aug 2026 17:03:35 -0600 Subject: [PATCH 10/15] 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 af1b32e81de6bc7d26bf544d7cb7e40e6210a01b Mon Sep 17 00:00:00 2001 From: Michael Moffett Date: Sat, 29 Aug 2026 17:04:03 -0600 Subject: [PATCH 11/15] 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 d0ef99b1c1a69629246daaba780a0360909b6457 Mon Sep 17 00:00:00 2001 From: Michael Moffett Date: Sun, 30 Aug 2026 11:39:20 -0600 Subject: [PATCH 12/15] 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 From bcac703a4327a2746d1744371fee20f77660f2b4 Mon Sep 17 00:00:00 2001 From: Michael Moffett Date: Mon, 31 Aug 2026 07:28:32 -0600 Subject: [PATCH 13/15] fix(cli): the invocation departs from #567 twice, not once --agent universal sat in the asserted arg array eight lines below a comment saying the pin was the only departure. Name both, and say what the second buys, since the reviewer reads the comment and the array together. Also drop the version re-parse below it: the arg-vector assert already fails if the pin changes shape. The "*" guard stays; that typo is one character wide. And drop three test doc-comments whose names already carry the claim. --- crates/cli/src/scaffold/mod.rs | 24 +++++------------------- 1 file changed, 5 insertions(+), 19 deletions(-) diff --git a/crates/cli/src/scaffold/mod.rs b/crates/cli/src/scaffold/mod.rs index 899da3e8c..d86eb2082 100644 --- a/crates/cli/src/scaffold/mod.rs +++ b/crates/cli/src/scaffold/mod.rs @@ -682,8 +682,11 @@ mod tests { command } - /// #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. + /// #567 supplied this invocation literally; we depart from its text twice. The pin is + /// spelled out here so that dropping it has to fail this. `--agent universal` is the + /// second: without it the installer detects none of its 77 registered agents, falls + /// through to fanout, and drops `.claude/`, a non-hidden `agent/` and `skills-lock.json` + /// into a project that asked for none of them. #[test] fn the_install_is_the_command_issue_567_asked_for() { let base = FileLocation::from_path_string("/tmp/surfpool-567-scaffold").unwrap(); @@ -706,17 +709,6 @@ mod tests { // "*" 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"); - 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}" - ); } /// `surfpool start -m ../elsewhere/txtx.yml` scaffolds a tree the caller is not standing in, @@ -772,8 +764,6 @@ mod tests { } } - /// 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(); @@ -793,8 +783,6 @@ mod tests { ); } - /// 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_is_remembered() { let (base, home, location) = scratch(); @@ -822,8 +810,6 @@ mod tests { 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 - /// 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 9750fbe0b456d73c7ed6fa35ca13410daead0b24 Mon Sep 17 00:00:00 2001 From: Michael Moffett Date: Mon, 31 Aug 2026 07:45:26 -0600 Subject: [PATCH 14/15] test(cli): the pin needs an assert the arg vector cannot give it The arg-vector assert_eq! spells DEV_SKILL_INSTALLER on both sides, so it moves with the const: set the pin to skills@latest and that assert still passes. The re-parse dropped in bcac703 was the only thing in the tree asserting the pin's value, so dropping it made the comment above it -- "dropping it has to fail this" -- and the const's own "Exact, not a range" both untrue. Back, compact: three dotted numeric parts, and a why so the next reader does not delete it for the same reason again. --- crates/cli/src/scaffold/mod.rs | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/crates/cli/src/scaffold/mod.rs b/crates/cli/src/scaffold/mod.rs index d86eb2082..25e2c0ad9 100644 --- a/crates/cli/src/scaffold/mod.rs +++ b/crates/cli/src/scaffold/mod.rs @@ -709,6 +709,17 @@ mod tests { // "*" 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, "*"); + // The arg-vector assert above spells DEV_SKILL_INSTALLER on both sides, so it moves with + // the const and cannot see the value change. This is the only assertion on the pin itself. + let (_, version) = DEV_SKILL_INSTALLER + .split_once('@') + .expect("installer is pinned"); + let exact = version.split('.').count() == 3 + && version.bytes().all(|b| b.is_ascii_digit() || b == b'.'); + assert!( + exact, + "the installer must be pinned to an exact version, not a range: {version}" + ); } /// `surfpool start -m ../elsewhere/txtx.yml` scaffolds a tree the caller is not standing in, From 15b9930e749a5f74f9ce25f6f1936ce3c8ada85b Mon Sep 17 00:00:00 2001 From: Michael Moffett Date: Mon, 31 Aug 2026 09:43:59 -0600 Subject: [PATCH 15/15] fix(cli): --yes answers our prompts, not a third party's --yes with nothing on record installed the solana-dev skill without ever asking and recorded nothing, so every later scaffold silently reinstalled. The flag pre-answers surfpool's own prompts; it was never consent to pull mutable, unpinned content from a repo nobody was asked about. An absent answer is now a skip, announced on stdout with the run that does ask. A recorded yes still installs and a recorded no still skips, so a user who consented once is not re-asked and CI keeps working for them; --yes still never prompts. Nothing installs unless a human said yes. --- crates/cli/src/scaffold/mod.rs | 62 ++++++++++++++++++++++++++-------- 1 file changed, 48 insertions(+), 14 deletions(-) diff --git a/crates/cli/src/scaffold/mod.rs b/crates/cli/src/scaffold/mod.rs index 25e2c0ad9..33a97a8aa 100644 --- a/crates/cli/src/scaffold/mod.rs +++ b/crates/cli/src/scaffold/mod.rs @@ -227,8 +227,11 @@ fn record_dev_skill_answer(home: &Path, consented: bool) { let _ = File::create(marker); } -/// A recorded answer and an install on disk both outrank `--yes`. Paths are arguments so the -/// gate is testable without a real home directory. +/// Only a recorded answer, or an install already on disk, decides this; `--yes` decides nothing +/// and alone never installs. It pre-answers surfpool's own prompts, and nobody has been asked +/// about a third-party repo whose contents we do not pin. What it still does is suppress the +/// prompt, so a non-interactive run skips rather than blocks. Paths are arguments so the gate is +/// testable without a real home directory. fn dev_skill_install_if_wanted( base_location: &FileLocation, base: &Path, @@ -241,7 +244,17 @@ fn dev_skill_install_if_wanted( } let consented = match dev_skill_answer(home) { Some(recorded) => recorded, - None if auto_accept => true, + // An absent answer is not a yes, and a silent skip is the same invisible install by + // another name, so say it and name the run that does ask. Nothing is recorded: the + // question is still open. + None if auto_accept => { + println!( + "{} {} (run `surfpool start` without --yes to be asked)", + green!("Skipped the installer for"), + DEV_SKILL_DIR + ); + false + } None => match ask() { Some(answer) => { record_dev_skill_answer(home, answer); @@ -662,9 +675,9 @@ mod tests { use tempfile::TempDir; use super::{ - DEV_SKILL_AGENT, DEV_SKILL_DECLINED_MARKER, DEV_SKILL_DIR, DEV_SKILL_INSTALLER, - DEV_SKILL_REPO, FileLocation, dev_skill_answer, dev_skill_install_command, - dev_skill_install_if_wanted, spawn_dev_skill_install, + DEV_SKILL_ACCEPTED_MARKER, DEV_SKILL_AGENT, DEV_SKILL_DECLINED_MARKER, DEV_SKILL_DIR, + DEV_SKILL_INSTALLER, 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. @@ -751,18 +764,19 @@ mod tests { } } - /// 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. + /// Under `--yes`, the marker decides and only the marker does. The remembered answer is read + /// before the flag rather than after, so a CI machine neither relitigates a no hourly nor + /// re-asks a user who already said yes. Neither case may reach the prompt. #[test] 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 marker = home.path().join(match declined { + true => DEV_SKILL_DECLINED_MARKER, + false => DEV_SKILL_ACCEPTED_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") @@ -775,6 +789,26 @@ mod tests { } } + /// `--yes` is not an answer. It pre-answers surfpool's own prompts; it was never consent to + /// install a third-party repo nobody was asked about. With no marker on either path it + /// installs nothing and writes nothing, so the next interactive scaffold still asks — and it + /// still does not prompt, because suppressing the prompt is the half of the flag that is real. + #[test] + fn the_yes_flag_is_not_an_answer() { + let (base, home, location) = scratch(); + assert!( + dev_skill_install_if_wanted(&location, base.path(), home.path(), true, || { + panic!("--yes must not prompt") + }) + .is_none(), + "--yes with nothing on record installed a skill nobody was asked about" + ); + assert!( + dev_skill_answer(home.path()).is_none(), + "a skip recorded as an answer closes a question that is still open" + ); + } + #[test] fn a_declined_prompt_starts_no_install_and_is_remembered() { let (base, home, location) = scratch();