feat: add 'squad preset install <source>' for sharing presets via repo URL (#1224)#1225
feat: add 'squad preset install <source>' for sharing presets via repo URL (#1224)#1225tamirdresher wants to merge 2 commits into
Conversation
🟡 Impact Analysis — PR #1225Risk tier: 🟡 MEDIUM 📊 Summary
🎯 Risk Factors
📦 Modules Affectedroot (1 file)
squad-cli (2 files)
squad-sdk (1 file)
tests (1 file)
|
🛫 PR Readiness Check
PR Scope: 📦🔧 Mixed (product + infrastructure)
|
| Status | Check | Details |
|---|---|---|
| ❌ | Single commit | 2 commits — consider squashing before review |
| ✅ | Not in draft | Ready for review |
| ✅ | Branch up to date | Up to date with dev |
| ❌ | Copilot review | No Copilot review yet — it may still be processing |
| ✅ | Changeset present | Changeset file found |
| ✅ | Scope clean | No .squad/ or docs/proposals/ files |
| ✅ | No merge conflicts | No merge conflicts |
| ❌ | Copilot threads resolved | 2 unresolved Copilot thread(s) — fix and resolve before merging |
| ❌ | CI passing | 4 check(s) still running |
Files Changed (5 files, +581 −3)
| File | +/− |
|---|---|
.changeset/feat-preset-install.md |
+60 −0 |
packages/squad-cli/src/cli/commands/preset.ts |
+56 −1 |
packages/squad-cli/src/cli/core/command-help.ts |
+12 −2 |
packages/squad-sdk/src/presets/index.ts |
+330 −0 |
test/presets.test.ts |
+123 −0 |
Total: +581 −3
This check runs automatically on every push. Fix any ❌ items and push again.
See CONTRIBUTING.md and PR Requirements for details.
|
@bradygaster — fix for #1224. Adds Smoke-tested 6 cases on local build:
Minor bump for both sdk + cli. CHANGELOG gate should pass cleanly (changeset included). Build hit a snag locally — workspaces had a stale real-copy of squad-sdk inside |
There was a problem hiding this comment.
Pull request overview
Adds a new preset distribution workflow by introducing squad preset install <source>, allowing users to install a single preset from a GitHub repo URL (via shallow clone) or a local path into $SQUAD_HOME/presets/<name>/.
Changes:
- SDK: add
installPresetFromSource(source, options)with source resolution (local vs git clone), preset discovery rules, optional rename stamping, and--forceoverwrite behavior. - CLI: add
preset installsubcommand wiring, usage output, and install status messages. - Release: add changeset bumping CLI and SDK as
minor.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| packages/squad-sdk/src/presets/index.ts | Implements the core install-from-source logic, including git shallow-clone + preset discovery/copying. |
| packages/squad-cli/src/cli/commands/preset.ts | Adds the install subcommand routing and user-facing CLI output for installs. |
| .changeset/feat-preset-install.md | Documents the feature and bumps CLI/SDK versions. |
| // Resolve the source into a local working directory + optional sub-path inside it. | ||
| // Returns { workDir, subPath, cleanup } — caller must call cleanup() in a finally. | ||
| const { workDir, subPath, cleanup } = resolveInstallSource(source); | ||
|
|
||
| try { | ||
| // Locate the actual preset directory inside workDir | ||
| const startDir = subPath ? path.join(workDir, subPath) : workDir; | ||
| if (!storage.existsSync(startDir) || !isDirSync(startDir)) { | ||
| throw new Error(`Source path does not exist or is not a directory: ${startDir}`); | ||
| } | ||
|
|
||
| const { presetDir, defaultName } = locatePresetWithinSource(startDir, options.name); | ||
|
|
| try { | ||
| const refArgs = ref ? ['--branch', ref] : []; | ||
| const args = ['clone', '--depth', '1', ...refArgs, cloneUrl, tmpBase]; | ||
| execSync(`git ${args.map(a => /[\s"]/.test(a) ? `"${a.replace(/"/g, '\\"')}"` : a).join(' ')}`, { stdio: 'pipe' }); |
| const presetsSubDir = path.join(startDir, 'presets'); | ||
| if (storage.existsSync(presetsSubDir) && isDirSync(presetsSubDir)) { | ||
| if (nameHint) { | ||
| const candidate = path.join(presetsSubDir, nameHint); | ||
| if (storage.existsSync(path.join(candidate, 'preset.json'))) { | ||
| return { presetDir: candidate, defaultName: nameHint }; |
| const force = args.includes('--force'); | ||
| const nameIdx = args.indexOf('--name'); | ||
| const nameOverride = nameIdx >= 0 ? args[nameIdx + 1] : undefined; | ||
| await presetInstall(source!, nameOverride, force); |
| * | ||
| * @throws Error on any validation failure, manifest invalidity, or destination collision. | ||
| */ | ||
| export function installPresetFromSource(source: string, options: InstallPresetOptions = {}): InstallPresetResult { |
| ` squad preset show <name>\n` + | ||
| ` squad preset apply <name> [--force]\n` + | ||
| ` squad preset save <name>\n` + | ||
| ` squad preset install <source> [--name <override>] [--force]\n` + | ||
| ` squad preset init [--remote]`, |
bradygaster
left a comment
There was a problem hiding this comment.
❌ Flight requests changes. Blocking issues: the new git clone path in packages/squad-sdk/src/presets/index.ts builds an execSync shell string from user-controlled input (shell injection risk), and the documented #preset-name repo URL flow does not resolve multi-preset repos correctly. Please switch to execFileSync/spawnSync with arg arrays and fix fragment handling before approval.
…o URL (bradygaster#1224) Closes bradygaster#1224. Adds a new subcommand that installs a single preset from a GitHub URL or local path into \\/presets/<name>/\ — the peer-to-peer preset sharing flow that was missing in v0.10.0. SDK side (squad-sdk/src/presets/index.ts): - New \installPresetFromSource(source, options)\ function - Resolves source: GitHub URL → shallow git clone --depth 1 to OS temp; local path → use as-is - Locates preset within source via 3 patterns: - dir contains preset.json → single-preset source - dir contains presets/ subdir → require --name to pick - dir IS the presets/ dir → require --name (or auto-pick if only one) - Validates preset.json before any destructive action - Copies preset.json (with optional rename) + agents/ into squad home - Cleans up temp clones in finally block (success or failure) - Exports: installPresetFromSource, InstallPresetOptions, InstallPresetResult CLI side (squad-cli/src/cli/commands/preset.ts): - New 'install' dispatcher case + presetInstall() function - Supports --name <override>, --force - Module docstring + default usage help updated to include 'install' Supported source shapes: https://github.com/owner/repo https://github.com/owner/repo#preset-name (frag as subdir hint) https://github.com/owner/repo/tree/branch/path/... (sub-path) git\@github.com:owner/repo.git (SSH) ./local/path (single preset OR collection) Smoke tested all 6 cases locally: 1. Local single-preset → installs under manifest.name ✅ 2. Idempotent re-install fails without --force ✅ 3. --force overwrites ✅ 4. --name renames + updates manifest.name ✅ 5. Invalid source → clear error ✅ 6. GitHub URL (cloned bradygaster/squad's presets/builtin/default) ✅ Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
4cafee9 to
0c7f354
Compare
…ent semantics, tests, help block Addresses all six review comments on bradygaster#1225: 1. [security] git clone via execSync was vulnerable to shell injection because the command was built as a string. Switched to execFileSync with an argument array (no shell), so source / ref values containing ';' '&&' '|' backticks '\' etc. can no longer be interpreted by a shell. The earlier ad-hoc whitespace/quote escaping was the wrong defence layer. 2. [security] nameHint was used in path.join without validation. A value like '../something' would have escaped the presets/ directory. Added validatePathSegment() that rejects path separators ('/' '\\'), '..', '.', null bytes — same shape as validateName for preset identifiers. Applied both at the public installPresetFromSource() entry AND inside locatePresetWithinSource() as defence-in-depth in case future callers go direct. Also added validateSubPath() for the URL-fragment-derived subPath: rejects absolute paths and '..' segments. 3. [correctness] Fragment semantics: 'repo#some-name' (bare fragment, no slash) was being treated as a literal subPath, so it looked for <clone>/some-name/ and broke the documented <clone>/presets/some-name/ collection layout from the PR description. Restructured resolveInstallSource to return a new nameHint field alongside subPath: - Fragment WITH '/' → literal subPath (e.g. repo#packs/team-a) - Fragment WITHOUT '/' → preset-name HINT (e.g. repo#my-team) The nameHint is forwarded to locatePresetWithinSource without being used as a path segment itself, so the common collection layout now works as advertised. 4. [UX] --name parsing didn't validate that a value was actually provided. 'squad preset install <src> --name' (no value) or '--name --force' (next arg is a flag) silently produced undefined or '--force' as the override and failed downstream with a confusing error. Added an early fail-fast guard with a clear usage hint. 5. [tests] Added 7 focused tests for installPresetFromSource covering the new code path: - single-preset local source (startDir/preset.json present) - collection local source + --name selection - collection source without --name throws with helpful 'specify one with --name' message - --force overwrite of an existing same-name preset - --name rename + manifest.name stamping (other manifest fields preserved) - --name path-escape attempts are rejected (../escape, a/b, a\\b, '.', '..', embedded null bytes) - empty source throws 'required' Remote (URL) branch isn't stubbed here — splitting the git-clone call into a small helper that tests can mock is a separate follow-up. 6. [docs] preset help block in command-help.ts still printed Usage: squad preset <list|show|apply|save|init> without 'install'. Updated to include 'install <source>', the new --name option, and a concise documentation of the fragment semantics from fix #3 so users see the right shapes from --help. Verified locally: 36/36 preset tests pass (29 existing + 7 new); 14/14 command-help tests pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
🏗️ Architectural Review
Automated architectural review — informational only. |
…ent semantics, tests, help block Addresses all six review comments on bradygaster#1225: 1. [security] git clone via execSync was vulnerable to shell injection because the command was built as a string. Switched to execFileSync with an argument array (no shell), so source / ref values containing ';' '&&' '|' backticks etc. can no longer be interpreted by a shell. The earlier ad-hoc whitespace/quote escaping was the wrong defence layer. 2. [security] nameHint was used in path.join without validation. A value like '../something' would have escaped the presets/ directory. Added validatePathSegment() that rejects path separators ('/' '\\'), '..', '.', null bytes. Applied both at the public installPresetFromSource() entry AND inside locatePresetWithinSource() as defence-in-depth in case future callers go direct. Also added validateSubPath() for the URL-fragment-derived subPath: rejects absolute paths and '..' segments. 3. [correctness] Fragment semantics: 'repo#some-name' (bare fragment, no slash) was being treated as a literal subPath, so it looked for <clone>/some-name/ and broke the documented <clone>/presets/some-name/ collection layout from the PR description. Restructured resolveInstallSource to return a new nameHint field alongside subPath: - Fragment WITH '/' -> literal subPath (e.g. repo#packs/team-a) - Fragment WITHOUT '/' -> preset-name HINT (e.g. repo#my-team) The nameHint is forwarded to locatePresetWithinSource without being used as a path segment itself, so the common collection layout now works as advertised. 4. [UX] --name parsing didn't validate that a value was actually provided. 'squad preset install <src> --name' (no value) or '--name --force' (next arg is a flag) silently produced undefined or '--force' as the override and failed downstream with a confusing error. Added an early fail-fast guard with a clear usage hint. 5. [tests] Added 7 focused tests for installPresetFromSource covering the new code path: - single-preset local source (startDir/preset.json present) - collection local source + --name selection - collection source without --name throws with helpful message - --force overwrite of an existing same-name preset - --name rename + manifest.name stamping (other fields preserved) - --name path-escape attempts are rejected - empty source throws 'required' Remote (URL) branch isn't stubbed here — splitting the git-clone call into a small helper that tests can mock is a separate follow-up. 6. [docs] preset help block in command-help.ts still printed Usage: squad preset <list|show|apply|save|init> without 'install'. Updated to include 'install <source>', the new --name option, and a concise documentation of the fragment semantics from fix #3. Verified locally: 36/36 preset tests pass (29 existing + 7 new); 14/14 command-help tests pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
3d3c111 to
8288297
Compare
|
All 6 review comments addressed in commit
Verified locally: 36/36 preset tests pass (29 existing + 7 new); 14/14 command-help tests pass. Branch force-pushed to |
|
@bradygaster ready for re-review — your two blocking concerns are addressed in
Plus 7 new tests in |
feat:
squad preset install <source>for sharing presets via repo URL (#1224)Closes #1224.
What
New subcommand that installs a single preset from a GitHub URL or local path into
$SQUAD_HOME/presets/<name>/:After install, the preset is a normal entry in
$SQUAD_HOME/presets/—squad preset list,squad preset apply <name>, andsquad init --preset <name>all work as today.Why
The existing
squad preset init --remoteflow shares your whole$SQUAD_HOMErepo. There was no way to install just one preset from someone else's repo. Users had to manually clone + copy + apply, or hijackSQUAD_HOME(collides with personal config).Source resolution rules
preset.json→ install that as a single presetpresets/collection → require--name(or#nameURL fragment) to pickpresets/dir → require--name, or auto-pick if only one preset presentImplementation
SDK (
packages/squad-sdk/src/presets/index.ts):installPresetFromSource(source, options)functionimport os from 'node:os'andexecSyncfromnode:child_process(ESM-safe)git clone --depth 1 [--branch <ref>]finallymanifest.nameto match installed name when--nameis usedCLI (
packages/squad-cli/src/cli/commands/preset.ts):'install'dispatcher case +presetInstall()functioninstallSmoke tests (all pass on local build)
manifest.name--forcealready exists--force--namerename + manifest rewritemanifest.nameupdatedNo preset founderrorbradygaster/squad/.../builtin/defaultbrady-defaultwith 5 agentsWhat's NOT in scope (deferred to follow-ups)
squad preset uninstall—rm -rf $SQUAD_HOME/presets/<name>works manuallysquad preset update <name>to pull fresh from originCo-authored-by: Copilot 223556219+Copilot@users.noreply.github.com