feat(cli): install the Solana dev skills on first scaffold - #761
feat(cli): install the Solana dev skills on first scaffold#761michael-moffett wants to merge 4 commits into
Conversation
Greptile SummaryThe PR adds a fire-and-forget Solana development-skill installation after successful first-time scaffolding.
Confidence Score: 3/5The PR is not yet safe to merge because first-time scaffolding still automatically installs skill content from a mutable repository revision. The npm installer is now pinned and the prior control-flow issues are fixed, but the command still passes an unversioned GitHub repository URL, so the installed skill content can change or be compromised independently of this release. Files Needing Attention: crates/cli/src/scaffold/mod.rs Important Files Changed
Reviews (4): Last reviewed commit: "fix(cli): drop the install from the exis..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| let mut command = Command::new("npx"); | ||
| command.args(["-y", "skills", "add", DEV_SKILL_REPO, "--skill", "*", "-y"]); |
There was a problem hiding this comment.
If the latest npm release of skills is compromised, the first-scaffold path resolves and executes that mutable package through npx -y without user consent, causing arbitrary code to run with the Surfpool user's permissions. How this was verified: The changed command supplies an unversioned package name to npx -y and spawns it automatically before any user prompt.
| fn dev_skill_install_command() -> Command { | ||
| let mut command = Command::new("npx"); | ||
| command.args(["-y", "skills", "add", DEV_SKILL_REPO, "--skill", "*", "-y"]); | ||
| command | ||
| } |
There was a problem hiding this comment.
When the configured manifest's parent differs from the process launch directory, this command inherits the process cwd instead of using base_location, causing project-local skill files to be installed outside the newly scaffolded project.
| // start; #567 asks for the skills here. Ahead of the prompts, to overlap. | ||
| spawn_dev_skill_install(dev_skill_install_command()); |
There was a problem hiding this comment.
When an early prompt is canceled or a later scaffold operation fails, the detached installer has already started and continues modifying the environment, causing an unsuccessful scaffold to leave partial side effects and a retry to start another installation.
| } | ||
| } | ||
|
|
||
| const DEV_SKILL_REPO: &str = "https://github.com/solana-foundation/solana-dev-skill"; |
There was a problem hiding this comment.
If the repository's default revision is compromised or unexpectedly changed before a first scaffold, the automatic command installs every skill from that mutable revision without visible output, persisting repository-controlled instructions that can direct subsequent agent activity with the user's permissions.
How this was verified: The command pins the npm installer but passes a bare GitHub repository URL without an immutable revision.
Runs the command from solana-foundation#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.
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.
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.
4c79eaf to
ad6bfaa
Compare
|
Hello! I corrected the last one. The cancel 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.
There was a problem hiding this comment.
I wouldn't want a CLI I use to automatically install a skill without asking me. At a minimum this should:
- Check if the skill is already installed and not prompt anything if so
- If not installed, ask the user if they want it instealled
- Install if yes, never ask again if no
If you want to make these changes, lmk. If this is beyond the scope you originally signed up for, lmk and I'll add these notes to the issue
2 participants
Implements #567.
On a first
surfpool start, the run that scaffolds atxtx.yml, this runs the command from the issue:The command in the issue is unversioned; this pins it to
skills@1.5.22so a first scaffold runs a known version rather than whatever is latest that day.Fire and forget as the issue asks: the scaffold spawns the child and never waits on it. The install runs behind the confirmation, so declining the prompt starts nothing, and a later start is no longer a scaffold so it cannot start a second one.
Four tests cover it: the invocation, the working directory, a declined confirmation building nothing to spawn, and the spawn driven through a missing binary, a non-zero exit and a hang. The last shells out to
sh, so it is gated#[cfg(unix)].+152 / -0, one file.
cargo test -p surfpool-cli: 44 passed, 0 failed.