[REHEARSAL] post-drop head 8c3b95b on main @ 6306f8e — do not merge, do not review - #13
[REHEARSAL] post-drop head 8c3b95b on main @ 6306f8e — do not merge, do not review#13michael-moffett wants to merge 4 commits into
Conversation
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.
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.
Greptile SummaryThis PR adds a fire-and-forget installation of Solana development skills after a fresh on-disk scaffold is confirmed.
Confidence Score: 3/5This PR should not be merged until remote package installation requires specific user consent rather than inheriting the runbook-generation confirmation. A fresh noninteractive scaffold now silently executes an npm installer and mutates the project even though the documented option only skips runbook prompts; the mutable skill source also makes installation output non-reproducible. Files Needing Attention: crates/cli/src/scaffold/mod.rs
|
| Filename | Overview |
|---|---|
| crates/cli/src/scaffold/mod.rs | Adds asynchronous remote skill installation, but reuses deployment confirmation for an undisclosed side effect and leaves the installed repository content unpinned. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Fresh on-disk scaffold] --> B{--yes enabled?}
B -->|Yes| C[confirmation = true]
B -->|No| D[Prompt to continue deployment]
D -->|Accept/default yes| C
D -->|Decline| E[Cancel deployment]
C --> F[Spawn silent npx skills install]
F --> G[Continue Surfpool startup without waiting]
Reviews (1): 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}
| if let Some(command) = install_after_confirmation(confirmation, base_location) { | ||
| spawn_dev_skill_install(command); |
There was a problem hiding this comment.
When a fresh scaffold uses --yes or accepts the default deployment prompt, the same confirmation silently runs npx skills@1.5.22 add ... and mutates the project even though neither path specifically authorizes remote package execution. How this was verified: The runbook confirmation value flows directly into install_after_confirmation, which spawns the installer with all output suppressed.
|
|
||
| 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. | ||
| const DEV_SKILL_INSTALLER: &str = "skills@1.5.22"; |
There was a problem hiding this comment.
DEV_SKILL_INSTALLER is pinned for repeatability, but DEV_SKILL_REPO still resolves the repository's mutable default branch and selects every skill. Identical Surfpool versions and inputs can therefore install different files over time, undermining reproducibility and making audits or rollbacks harder.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
Internal CI rehearsal, purpose served. Closing to keep the fork clean. |
1 participant
DO NOT MERGE. DO NOT REVIEW. Internal CI rehearsal on a fork. No maintainer action is wanted or expected. Supersedes #11.
Base sha:
6306f8ee56de85687d42593b803622fbdc69e146(main)Head sha:
8c3b95b9842be22b494281bb225a94954698243a(fix/761-cancel-guard)Merge-base:
77f9d435c55966554a7f345011dadcee9a4f47d6Purpose: confirm the Rust workflow jobs still run and pass against current
mainafter the S1 change. Three-dot diff is a single file,crates/cli/src/scaffold/mod.rs, so onlyrust.ymlis expected to fire —sdk_node.ymlandbenchmark.ymlare correctly filtered out by theirpaths:keys, and the four release workflows have nopull_requesttrigger.This PR will be closed once its checks settle.