diff --git a/feedback.md b/feedback.md index 197b8d0..359e69c 100644 --- a/feedback.md +++ b/feedback.md @@ -1,177 +1,326 @@ -# Sprint 3 — Dep Resolution Plan Review +# Sprint 3 Phase 1 — Tracker Review **Reviewer:** plug-reviewer **Date:** 2026-04-16 -**Verdict:** CHANGES NEEDED +**Verdict:** APPROVED -> See the recent git history of this file to understand the context of this review. +> Cumulative review — covers plan review + Phase 1 changes. --- -## 1. Clear "done" criteria — PASS +## Task 1.1 — tracker.js -Every task has explicit, testable done-when statements. Task 2.2 (resolver) done criteria includes the critical "getInstalled called once per resolve" constraint. Task 4.1 enumerates all three remove flows. Test tasks specify exact case counts. No ambiguity in what "done" means for any individual task. +All requirements met: + +- **`trackInstall` persists new fields:** `installed_as`, `dependencies`, `dependents` are set at lines 52-54, with correct defaults (`'explicit'`, `[]`, `[]`). The spread order is correct — explicit fields override any same-named keys from `...metadata`. +- **Backward-compat normalisation:** `prunableOrphans` (line 93) uses `rec.installed_as ?? 'explicit'` and `rec.dependents ?? []`. `addDependents` (line 70) uses `rec.dependents ?? []`. `removeDependentEdge` (line 107) uses `rec.dependents ?? []`. All read paths that touch these fields have null-coalescing guards, so pre-existing records without the new fields won't break. +- **`addDependents` merge/dedup semantics:** Line 71 uses `[...new Set([...existing, ...newDependents])]` — this correctly merges and deduplicates. Multi-parent scenario (X depends on A, then B added) preserves both. No overwrite bug. +- **`getInstalledRecord`:** Returns `data.installed[name] ?? null` (line 83). Correct. +- **`prunableOrphans`:** Filters `installed_as === 'dependency'` AND `dependents.length === 0` (line 93). Uses backward-compat normalisation. Correct. +- **`removeDependentEdge`:** Filters out `fromName` from `toName`'s dependents array (line 107). Early return on missing record. Saves after mutation. Correct. +- **Early-return guards:** Both `addDependents` (line 69) and `removeDependentEdge` (line 106) return early if the target record doesn't exist. Good defensive coding. + +No issues found. --- -## 2. Cohesion / coupling — PASS with NOTE +## Task 1.2 — tracker.test.js + +All 7 required test cases present and correctly implemented: + +1. **`trackInstall` with `installed_as: 'dependency'`** — verifies persistence of all three new fields. +2. **`trackInstall` default** — verifies `installed_as` defaults to `'explicit'` when omitted. +3. **`addDependents` multi-parent accumulation** — installs with `dependents: ['pkg-a']`, then calls `addDependents` with `['pkg-b']`, asserts result is `['pkg-a', 'pkg-b']`. This is the critical multi-parent test that validates merge semantics. +4. **`addDependents` dedup** — calls `addDependents` twice with `['pkg-a']` on a record that already has `['pkg-a']`, asserts no duplicate. +5. **`addDependents` single-record isolation** — installs two packages, mutates one, verifies the other is unaffected. +6. **`prunableOrphans` subset** — installs an orphan dep, a non-orphan dep, and an explicit package. Verifies only the orphan is returned. Correct boundary testing. +7. **`removeDependentEdge` back-reference** — installs with two dependents, removes one, verifies only the other remains. -Phases are well-scoped: tracker schema (P1), resolver (P2), install wiring (P3), remove wiring (P4), frontmatter (P5). Coupling between phases follows the dependency graph cleanly. +Tests are appended to the existing `tracker.test.js` (not a new file). Imports updated at line 21 to include all four new functions. Existing 9 tests unaffected. -**NOTE:** Task 3.1 is the densest task in the plan — extract helper, wire resolver, loop toInstall, track installed_as, call updateDependents, AND update install.test.js. It's cohesive around the install concern but could easily balloon into a multi-hour session if install.js has surprises. Doer should read install.js thoroughly before starting (the blocker note says this, which is good). +No issues found. --- -## 3. Shared abstractions in earliest tasks — PASS +## Test Results -Phase 1 establishes the tracker schema and all four helper functions (`updateDependents`, `getInstalledRecord`, `prunableOrphans`, `removeDependentEdge`) that Phases 3 and 4 consume. Phase 2 establishes the `resolve()` public API that Phase 3 consumes. Correct ordering. +**264/264 tests pass** across 26 test files. Zero failures, zero skipped. No regressions. --- -## 4. Riskiest assumption validated early — PASS +## Summary -Requirements.md Risk #1 (N file reads per node) is addressed in Phase 2, Task 2.2, which is the earliest phase where the resolver exists. The done criteria explicitly requires "getInstalled called once per resolve." The risk register also calls this out with HIGH impact and clear mitigation. Cannot validate earlier since the resolver doesn't exist until Phase 2. +Phase 1 implementation is clean, correct, and complete. All four tracker helpers match the PLAN.md specification exactly. The `addDependents` merge/dedup semantics — the critical requirement from the plan review's HIGH finding — are correctly implemented using `Set` deduplication. Backward-compat normalisation is consistently applied across all read paths. All 7 new test cases cover the right scenarios including the multi-parent accumulation test that guards against the overwrite bug identified in the plan review. Full test suite green. Approved for Phase 2. +--- --- -## 5. Later tasks reuse early abstractions — PASS +# Sprint 3 Phase 2 — Resolver Review -- Phase 3 reuses `resolve()` (P2), `trackInstall` metadata fields (P1), `updateDependents` (P1). -- Phase 4 reuses `prunableOrphans` (P1), `removeDependentEdge` (P1). -- Phase 5 is independent but wires into install.js (P3). +**Reviewer:** plug-reviewer +**Date:** 2026-04-16 +**Verdict:** APPROVED -No redundant reimplementations. +> Cumulative review — covers Phase 1 + Phase 2 changes. --- -## 6. 2-3 work tasks per phase + VERIFY — PASS +## HIGH +(none) -| Phase | Work tasks | VERIFY | -|-------|-----------|--------| -| 1 | 2 | Yes | -| 2 | 3 | Yes | -| 3 | 3 | Yes | -| 4 | 2 | Yes | -| 5 | 2 | Yes | +## MEDIUM +(none) -All within the 2-3 range. Each VERIFY includes `npm test`, manual checks, and a specific commit message. +## LOW +- `vaultHint` parameter accepted but unused in `resolve()` — resolver.js:80 — not a bug (reserved for Phase 3 install wiring), but worth a note so it doesn't get forgotten. --- -## 7. Each task completable in one session — PASS with NOTE +## Task 2.2 — resolver.js + +### `buildPackageMap()` +- Official vault registries loaded first via `getResolveOrder()` + `fetchRegistry()` with stale-cache fallback. Both wrapped in try/catch — offline-safe. +- Community index loaded second via `fetchCommunityIndex()` with stale-cache fallback. Community packages overwrite official entries on name conflict (community wins). Correct per spec. +- Handles both object and array shapes for community data (line 37: `Array.isArray` check). Defensive. +- Each package entry gets `dependencies: pkg.dependencies ?? []` default (line 40). Safe against missing field. + +### `resolve()` — getInstalled called EXACTLY ONCE +- `getInstalled(isGlobal)` called at line 85 inside `Promise.all`, before DFS begins. The snapshot is passed as a parameter to `dfsResolve` — never re-fetched inside the loop. **Verified.** + +### DFS (`dfsResolve`) +- **Back-edge / cycle detection:** `inStack` set tracks the current DFS path. If `name` is already in `inStack`, it's a back-edge — cycle recorded and return (line 50-53). Correct. +- **Already-visited skip:** `visited` set checked at line 54. Prevents re-processing in diamond dependency graphs. Correct. +- **Optional deps skipped:** `required !== false` check at line 63. Deps with `required: false` are skipped. Correct. +- **Unknown deps silently skipped:** `packageMap.has(depName)` check at line 65. Unknown packages don't crash. Correct. +- **Topological order:** Post-order push — deps processed before the package itself. `inStack.delete` before `visited.add` and array push. Result is dep-first with `pkgName` last. Correct. +- **Installed check:** `Object.prototype.hasOwnProperty.call` at line 72 — safe against prototype pollution. Good. +- **Dedup in output arrays:** `includes()` check before push at lines 74 and 76. Prevents duplicates in `alreadySatisfied` and `toInstall`. Correct (though O(n) — acceptable for package counts). -Task 3.1 is the largest single task. It modifies two files (install.js + install.test.js) across 6 sub-steps. This is completable in one session for a developer who has already read install.js, but the blocker note should be treated as mandatory prep, not optional. +### Public API +- Returns `{ toInstall, alreadySatisfied, cycles }` as specified. + +No issues found. --- -## 8. Dependencies satisfied in order — PASS +## Task 2.3 — resolver.test.js + +All 9 required test cases present: + +1. **No deps** — `toInstall: [pkg]`, empty `alreadySatisfied` and `cycles`. +2. **Single required dep** — `toInstall: [dep, pkg]` (dep-first order verified). +3. **Dep already installed** — `alreadySatisfied: [dep]`, `toInstall: [pkg]`. +4. **Transitive A->B->C** — `toInstall: [C, B, A]` (deep-first topological order). +5. **Cycle A->B->A** — `cycles` contains the back-edge node, both packages still in `toInstall`. +6. **Optional dep skipped** — `required: false` dep not in `toInstall`. +7. **Unknown dep no crash** — missing dep silently skipped, parent still installed. +8. **All deps installed** — all deps in `alreadySatisfied`, only root in `toInstall`. +9. **getInstalled called exactly once** — spy test with multi-dep diamond graph (`dep-m1` and `dep-m2` share a dep). Verifies `getInstalled` called once even with complex graph. -- P1 (tracker) before P2 (resolver uses tracker). -- P2 (resolver) before P3 (install wires resolver). -- P1 (tracker helpers) before P4 (remove uses prunableOrphans, removeDependentEdge). -- P3 (install.js modifications) before P5 (frontmatter wires into install.js). +Mocks are clean: `vi.mock` for registry, community-index, tracker, config. `beforeEach` resets all mocks with sensible defaults (offline/empty). `setupPackages` helper keeps tests concise. -No circular or out-of-order dependencies. +No issues found. --- -## 9. Vague / ambiguous tasks — FAIL (HIGH) +## Cross-cutting Checks + +- **Tests green:** 273/273 pass across 27 test files. Zero failures. +- **Commit history:** 2 commits since Phase 1 (`b3eaf24` feat, `5db3f83` progress) — clean, messages match plan. +- **Files changed:** Only `resolver.js`, `resolver.test.js`, `progress.json` — no scope creep. +- **No `CLAUDE.md` or `.fleet-task.md` committed.** +- **Phase 1 not regressed:** Tracker tests (16 cases) still pass. No tracker files modified. + +--- + +## Summary + +Phase 2 implementation is correct, complete, and well-tested. The resolver correctly builds a merged package map (community-wins), calls `getInstalled` exactly once per resolution, and performs DFS with proper cycle detection, optional-dep skipping, and topological ordering. All 9 test cases cover the specified scenarios. Full test suite green with no regressions. Approved for Phase 3. + +--- +--- + +# Sprint 3 Phase 3 — Install Wiring + TUI Plan Screen Review + +**Reviewer:** plug-reviewer +**Date:** 2026-04-16 +**Verdict:** APPROVED + +> Cumulative review — covers Phase 1 + Phase 2 + Phase 3 changes. -### HIGH: `updateDependents` overwrite semantics cause data corruption +--- -Task 1.1 defines `updateDependents(name, dependents, global)` as: "overwrites `data.installed[name].dependents`; saves." +## HIGH +(none) -Task 3.1 step 5 says: "call `updateDependents(dep.name, [pkgName], isGlobal)` for each dep." +## MEDIUM +- Dead code: JSON abort output unreachable — install.js:144-146 — The outer guard at L132 is `if (!ctx.json && plan.toInstall.length > 1)`, so `ctx.json` is guaranteed false inside. The `if (ctx.json)` branch at L144 can never execute. Collapse to just the `console.log(chalk.yellow('Aborted.'))` path. -**The bug:** If package A depends on X, and later package B also depends on X: -1. `plug install A` → `updateDependents('X', ['A'], false)` → X.dependents = `['A']` -2. `plug install B` → `updateDependents('X', ['B'], false)` → X.dependents = `['B']` — **overwrites, losing `'A'`** +## LOW +- Silent dep skip on resolver lookup failure — install.js:173 — Empty `catch` silently drops deps that `findAllPackages` can't resolve. Acceptable for now, but a `verbose()` log inside the catch would aid debugging without changing behavior. +- `doInstall` closure in `useCallback` — discover.jsx:170 — `handlePlanConfirm` captures `doInstall` (a plain function recreated each render) but the dep array only lists `[installQueue]`. Safe in practice because `doInstall` only uses stable refs/setters and takes `queue` as a param, but `eslint-plugin-react-hooks` exhaustive-deps would flag it. Not blocking. -Now removing B makes X prunable (dependents = []), even though A still depends on X. This is a data integrity bug that will cause incorrect orphan pruning. +--- -**Fix required:** Either: -- (a) Change `updateDependents` to **append** semantics: read current dependents, merge new entries, deduplicate, then save. Rename to `addDependents` for clarity. OR -- (b) Change Task 3.1 step 5 to read-then-merge: `const existing = getInstalledRecord(dep.name, isGlobal).dependents ?? []; updateDependents(dep.name, [...new Set([...existing, pkgName])], isGlobal)`. +## Task 3.1 — install.js -Option (a) is cleaner — the function should own its merge semantics. +### `installSinglePackage` extraction +- Clean extraction. Branches on `rawBaseUrl` presence for community vs official download path. Returns `{ type, destPath, version }` — well-defined contract. +- Community path uses raw `fetch()`; official path uses `downloadFile` helper. Both have meta.json fallback on failure. Correct. -### MEDIUM: `_cascade` flag in Task 4.1 is undefined +### Resolver wiring +- `resolve(pkgName, vault.name, { global: isGlobal })` called at L129, after package lookup and overwrite prompt, before any downloads. Correct ordering. +- `vaultHint` parameter from Phase 2's LOW finding is now consumed — `vault.name` passed through. Good follow-through. -Task 4.1 Cascade flow says: "`runRemove(dep, { global, _cascade: true })` for each dependent first, then target." But `_cascade` is never defined — what does it do? Presumably it suppresses the dependents check on the recursive call to prevent infinite recursion (dependent A might itself have dependents). But this is not stated. Two developers could implement this differently: -- Developer 1: `_cascade` skips the dependents check entirely. -- Developer 2: `_cascade` skips only the user prompt but still checks. +### Dependency install loop +- `installOrder` built from `plan.toInstall` with a guard at L157 that appends root if missing. Defensive and correct. +- `pkgSpecMap` pre-resolves all deps via `findAllPackages` before the install loop begins. Lookup failures silently skipped (see LOW above). +- Loop iterates dep-first order. Each package: download → trackInstall → next. Root gets `installed_as: 'explicit'`; deps get `installed_as: 'dependency'`. Both include `dependencies: [direct dep names]`. Correct per spec. -**Fix required:** Add a one-line definition of `_cascade` behavior. Suggested: "When `_cascade` is true, skip the dependents check and proceed directly to file deletion + trackRemove — this prevents infinite recursion during cascade." +### Reverse-dependency edges +- `addDependents(depName, [pkgName], isGlobal)` called for each non-root dep after all installs complete. Root correctly excluded from addDependents calls. Correct. -Also: what if dependents have their own dependents (A → B → target, where A depends on B depends on target)? Does cascade only remove direct dependents, or does it recurse transitively? This must be specified. +### Already-satisfied handling +- Deps in `plan.alreadySatisfied` are never in `plan.toInstall` (resolver contract from Phase 2), so they are naturally skipped — no re-download, no re-track. Correct. --- -## 10. Hidden dependencies between tasks — PASS with NOTE +## Task 3.2 — install-plan.jsx (new component) -**NOTE:** Task 5.1 modifies install.js, which Task 3.1 also heavily modifies. The plan correctly sequences them (P3 before P5), but the doer should be aware that the insertion point for frontmatter parsing (Task 5.1: "after writing SKILL.md") depends on the final shape of install.js after Task 3.1's refactor. If 3.1 significantly restructures the file, 5.1's description of "after writing SKILL.md" may not map cleanly. +- Props: `{ queue, plan, loading, onConfirm(scope), onCancel }` — well-typed via JSDoc. +- Loading state renders spinner. Plan state renders "Will install" list + "Already satisfied" list + scope selector + footer. +- `confirmedRef` prevents double-confirm on fast key repeats — good defensive pattern. +- Tab toggles scope between `'project'` and `'global'`. `[i]`/`[y]`/Enter confirms; Esc cancels. All input blocked during loading. Correct. +- `queueMap` lookup provides vault/type info for display. Falls back gracefully if a dep isn't in the original queue. -No other hidden dependencies found. +--- + +## Task 3.3 — discover.jsx + +- `'plan'` state added to `DiscoverView` union type. +- `startInstall` now runs resolver first, shows plan screen only when `plan.toInstall.length > 1`. Single-package installs bypass plan screen — current behavior preserved. Correct. +- Resolver failure falls back to direct install (L163-167). Good defensive behavior. +- `ctx.set({ yes: true, json: true })` called only inside `doInstall()` after confirm — NOT during resolver async phase. This prevents flag leakage if the user cancels from the plan screen. Verified. +- `handlePlanConfirm` passes `scope` through to `doInstall` as `{ global: scope === 'global' }`. Correct. +- `handlePlanCancel` resets all state and returns to list view. Correct. --- -## 11. Risk register — PASS with NOTE +## Tests -Five risks identified, all with impact ratings and mitigations. Good coverage of the key risks from requirements.md. +### install.test.js (4 new tests) +1. **Resolver call** — verifies `resolve` called with `pkgName`, vault name, and `{ global: false }`. +2. **Dep tracking** — verifies deps get `installed_as: 'dependency'` and root gets `installed_as: 'explicit'`. +3. **Reverse edges** — verifies `addDependents` called for deps with root as dependent; NOT called for root itself. +4. **Already-satisfied skip** — verifies satisfied deps are not downloaded or tracked. -**Missing risk (should be added):** +### discover.test.jsx (4 new tests) +1. **Plan view for multi-dep** — verifies "Will install" text appears. +2. **Skip plan for no-dep** — verifies "Will install" does NOT appear; goes straight to installing. +3. **Tab toggles scope** — verifies `◉ Project` → Tab → `◉ Global`. +4. **Esc returns to list** — verifies plan view dismissed, list restored. -| Risk | Impact | Mitigation | -|------|--------|------------| -| `updateDependents` overwrite semantics lose prior dependents on multi-parent deps | HIGH | Use append/merge semantics (see finding #9 above) | -| Cascade remove recursion depth on deep dependency chains | LOW | Cap recursion or use iterative approach; add test for A→B→target chain | +All mocks clean. Resolver mock defaults to empty `toInstall` so existing tests are unaffected. --- -## 12. Alignment with requirements.md — PASS +## Cross-cutting Checks -All requirement sections (A1-A5, A6 scope toggle) have corresponding plan phases. All 7 acceptance criteria from requirements.md have matching VERIFY steps. Out-of-scope items correctly excluded. +- **Tests green:** 281/281 pass across 27 test files. Zero failures, zero skipped. +- **Commit history:** `bcc3a52` is the single Phase 3 commit. Message matches plan. +- **Files changed:** `install.js`, `install-plan.jsx` (new), `discover.jsx`, `install.test.js`, `discover.test.jsx`, `progress.json` — all in scope. +- **No `CLAUDE.md` or `.fleet-task.md` committed.** +- **Phase 1 not regressed:** Tracker tests (16 cases) still pass. Tracker functions used correctly. +- **Phase 2 not regressed:** Resolver tests (9 cases) still pass. `resolve()` integrated correctly with `vaultHint` now consumed. --- -## Sprint 3-Specific Concerns +## Summary + +Phase 3 correctly wires the Phase 2 resolver into the install flow and adds a TUI plan screen for multi-dependency installs. The `installSinglePackage` extraction is clean. Dependency ordering, tracking, and reverse-edge recording all follow spec. The TUI plan screen is well-structured with proper scope toggle, cancel flow, and double-confirm guard. One MEDIUM (dead code in abort path) and two LOWs (silent catch, stale closure in useCallback). None are blocking. Full test suite green with 8 new targeted tests. Approved. -### Phase 2 resolver.js — getInstalled() per-node prevention — PASS +--- +--- -Task 2.2 algorithm step 2 explicitly states: "`installedSnapshot = await getInstalled(isGlobal)` — load ONCE; pass into DFS — never call per-node." The done criteria reinforces this. Test 2.3 should include a spy/call-count assertion on `getInstalled` to enforce this at test level — this is implied but not explicitly listed as a test case. +# Sprint 3 Phase 4 — Dependent Check, Cascade/Force, Orphan Pruning -**Recommendation:** Add a 9th test case to Task 2.3: "Verify `getInstalled` called exactly once regardless of dep tree size." This converts the requirement from a code-review check to an automated regression guard. +**Reviewer:** plug-reviewer +**Date:** 2026-04-16 +**Verdict:** APPROVED -### Phase 3 install.js — rawBaseUrl branch condition — PASS +> Cumulative review — covers Phases 1-4 changes. -Task 3.1 step 1 clearly specifies: "If `pkgSpec.rawBaseUrl`: `fetch(rawBaseUrl + entry)` directly; Else: use existing `downloadFile(vault, ...)` path." Unambiguous. +--- -### Phase 3 discover.jsx — ctx.set() timing — PASS +## HIGH +(none) -Task 3.3 explicitly states: "`ctx.set({ yes: true, json: true })` must only be called inside `doInstall()` after user confirms — NOT during resolver async phase." Risk register also covers this (MED impact). Clear constraint. +## MEDIUM +- **Duplicate unlink/error-handling block** — remove.js:66-79 and remove.js:100-113 are identical file-deletion + EACCES/ENOENT handling. Consider extracting a `_unlinkSafe(path)` helper to eliminate the duplication. Not blocking since both paths are tested and correct, but it's a maintenance hazard if error handling ever needs to change. +- **`removeDependentEdge` argument order may confuse** — remove.js:84 calls `removeDependentEdge(name, dep, isGlobal)` where `name` is the package being removed and `dep` is each dependent. The tracker function signature is `removeDependentEdge(fromName, toName, global)` which reads as "remove fromName from toName's dependents array". Semantically correct (removing `name` from each dependent's list), but the loop variable `dep` at the call site makes it look inverted. A rename to `dependent` would clarify. -### Phase 3 install.test.js — mock layering — PASS with NOTE +## LOW +- **No `--json` path for the dependent prompt** — When `ctx.json` is true, the `select()` prompt from `@inquirer/prompts` still fires interactively (line 45). JSON-mode callers (scripts, CI) would hang. Not in scope for this phase, but worth a future issue: in JSON mode, default to force or error out rather than prompting. +- **`_pruneOrphans` recursion depth** — Each orphan removal calls `runRemove` which calls `_pruneOrphans` again. In practice the depth is bounded by the dependency tree size, but a pathological graph could hit stack limits. Acceptable for now. -Task 3.1 step 6 gives the exact `vi.mock` line. However, it doesn't specify how per-test overrides should work when some tests need the resolver to return deps and others need it to return an empty plan. The default mock returns `{ toInstall: [], alreadySatisfied: [], cycles: [] }` — this means existing tests won't break (resolver returns nothing), but new dep-tracking tests need `resolve.mockResolvedValueOnce(...)`. This is implied but stating it explicitly would prevent confusion. +--- -### Phase 4 remove.js — cascade/force/cancel flows — FAIL +## Task 4.1 — remove.js -The three flows are listed but cascade has an undefined recursion mechanism (see finding #9 above). Cancel and Force are clear and distinct. Cascade needs the `_cascade` flag defined and transitive-dependent behavior specified. +### Dependent check +- `dependents = pkg.dependents ?? []` at line 42, defensive against records missing the field. Correct. +- Prompt fires only when `dependents.length > 0 && !options._cascade` (line 44). The `_cascade` guard prevents infinite re-prompting on recursive calls. Clean design. + +### Cancel +- Returns early (line 55). No side effects. Correct. + +### Cascade +- Iterates `dependents` array, calls `runRemove(dep, { global: isGlobal, _cascade: true })` for each (lines 58-60). ONE level deep — the recursive call has `_cascade: true` so it skips its own dependents prompt and falls through to the normal remove path. Correct. +- After dependents are removed, falls through to remove the target (lines 99-124). Correct ordering: dependents first, then target. + +### Force +- Deletes file, calls `trackRemove(name, isGlobal)`, then `removeDependentEdge(name, dep, isGlobal)` for each dependent (lines 82-85). Does NOT remove dependent packages — only severs the edges. Correct per spec. +- Has its own output path (JSON and CLI) and returns after `_pruneOrphans`. No fall-through to the normal path. Correct. + +### Orphan pruning +- `_pruneOrphans(isGlobal)` called after every successful remove (lines 94, 124). Correct — fires for normal, cascade, and force paths. +- `ctx.yes` check skips the `confirm()` prompt and auto-prunes. Correct. +- Orphan removal goes through `runRemove` so it gets full treatment (file deletion + tracker update). Correct. + +### Error handling +- EACCES/EPERM throws with a user-friendly message. ENOENT silently continues (file already gone). Other errors re-thrown. Same pattern as pre-existing code. Correct. --- -## Summary +## Task 4.2 — remove.test.js -**What passed (10 of 12 checklist items):** Done criteria are clear. Abstractions are correctly ordered. Phases are well-sized with VERIFY checkpoints. Dependencies are satisfied in order. Risk register is present and mostly complete. Requirements alignment is full. +All 6 required test cases present: -**What must change before execution (2 HIGH findings):** +1. **Dependents trigger prompt** — verifies `select` called when dependents exist; message contains dependent name. +2. **Cancel leaves both** — verifies both files exist and both tracker records intact after cancel. +3. **Cascade removes target + dependents** — verifies both files deleted, both tracker records removed. +4. **Force removes only target + severs edges** — verifies target file deleted, dependent file intact; target removed from tracker, dependent's `dependents` array no longer contains target. +5. **Orphan prompt fires** — verifies `confirm` called when orphaned dependency exists after remove; orphan kept when user declines. +6. **`--yes` auto-prunes** — verifies `confirm` NOT called; orphan auto-removed from tracker. -1. **`updateDependents` overwrite semantics** — The current spec will silently drop dependents when a package is a dependency of multiple parents. This is a data integrity bug that causes incorrect orphan pruning. Change to append/merge semantics or add read-then-merge logic at the call site. Update Task 1.1 definition, Task 1.2 test cases (add multi-parent test), and Task 3.1 step 5. +Mock setup is clean: `@inquirer/prompts` mocked at module level with `vi.mock`; `select` and `confirm` mocks reset via `vi.clearAllMocks()` in `beforeEach`. `ctx.reset()` in both `beforeEach` and `afterEach` ensures no state leakage between tests. -2. **`_cascade` flag undefined in Task 4.1** — Define what `_cascade: true` does (skip dependents check to prevent infinite recursion). Specify whether cascade is shallow (direct dependents only) or transitive (recursive). Add a note about recursion depth. +No issues found. -**Recommendations (not blocking):** +--- + +## Cross-cutting Checks + +- **Tests green:** 287/287 pass across 27 test files. Zero failures, zero skipped. +- **Commit history:** `5959da2` (feat) + `ca0b9b1` (progress) — clean, messages match plan. +- **Files changed:** `remove.js`, `remove.test.js`, `progress.json` — all in scope. No scope creep. +- **No `CLAUDE.md` or `.fleet-task.md` committed.** +- **Phase 1 not regressed:** Tracker tests (16 cases) still pass. +- **Phase 2 not regressed:** Resolver tests (9 cases) still pass. +- **Phase 3 not regressed:** Install + TUI tests still pass. No install files modified. + +--- + +## Summary -- Add a 9th test case to Task 2.3: verify `getInstalled` called exactly once per resolve. -- Add the `updateDependents` overwrite risk to the risk register. -- Task 3.1 step 6: mention `mockResolvedValueOnce` pattern for per-test overrides. +Phase 4 correctly implements dependent checking with three user-selectable paths (cancel/cascade/force) and automatic orphan pruning. The `_cascade` flag is an elegant solution to prevent recursive prompt loops. All three prompt paths and the orphan pruning flow are covered by well-isolated tests. Two MEDIUMs (duplicate unlink block, confusing variable name at call site) and two LOWs (no JSON-mode guard on prompt, recursion depth). None are blocking. Full test suite green at 287 tests with no regressions across Phases 1-3. Approved. diff --git a/plug/PLAN.md b/plug/PLAN.md deleted file mode 100644 index 3f80d36..0000000 --- a/plug/PLAN.md +++ /dev/null @@ -1,257 +0,0 @@ -# plug — Sprint 3: Dependency Resolution - -> Teach the install/remove lifecycle about dependencies: extend tracker.js schema, add DFS resolver, wire resolver into install + TUI plan screen, add dependent check on remove, and add post-install hook notice. - -**Base branch:** `main` -**Feature branch:** `sprint/dep-resolution` - ---- - -## Tasks - -### Phase 1: Extend Tracker (A1) - -#### Task 1.1: Add schema fields + new helpers to tracker.js -- **Change:** In `src/utils/tracker.js`: - - Persist three new fields in `trackInstall`: `installed_as: metadata.installed_as ?? 'explicit'`, `dependencies: metadata.dependencies ?? []`, `dependents: metadata.dependents ?? []` - - Normalise on any read path touching `rec.installed_as`: `rec.installed_as ?? 'explicit'` - - Add four new exported functions: - - `addDependents(name, newDependents, global)` — **merges** (not overwrites) `newDependents` into `data.installed[name].dependents` (deduplicate); saves. Merge semantics are required: if package X is a dependency of both A and B, `addDependents('X', ['B'], global)` must preserve A in X's dependents list, not erase it. - - `getInstalledRecord(name, global)` — returns `data.installed[name] ?? null` - - `prunableOrphans(global)` — returns names where `installed_as === 'dependency' && dependents.length === 0` - - `removeDependentEdge(fromName, toName, global)` — removes `fromName` from `data.installed[toName].dependents`; saves -- **Files:** `src/utils/tracker.js` -- **Tier:** standard -- **Done when:** `trackInstall` persists new fields; all four helpers exported and functional; `addDependents` uses merge/dedup semantics; backward-compat normalisation in place -- **Blockers:** Read current `src/utils/tracker.js` and `src/utils/tracker.test.js` before modifying — match file-write patterns exactly - -#### Task 1.2: Extend tests/tracker.test.js -- **Change:** Add test cases (do NOT create a new file): - - `trackInstall` with `installed_as: 'dependency'` persists correctly - - `trackInstall` without `installed_as` defaults to `'explicit'` - - `addDependents` appends to existing dependents (multi-parent case: install A then install B both depending on X → X.dependents = ['A', 'B'], not just ['B']) - - `addDependents` deduplicates (calling twice with same name produces no duplicate) - - `addDependents` mutates only the targeted record (other records unaffected) - - `prunableOrphans` returns correct subset - - `removeDependentEdge` removes back-reference correctly -- **Files:** `tests/tracker.test.js` -- **Tier:** standard -- **Done when:** all seven new test cases pass; existing tests unaffected -- **Blockers:** none - -#### VERIFY: Phase 1 -- Run full `npm test` — zero failures -- Confirm `trackInstall` stores new fields; `prunableOrphans` returns correct results -- Commit: `feat(tracker): extend installed.json with installed_as/dependencies/dependents` — one commit, includes tracker.js + tracker.test.js changes -- Push to `sprint/dep-resolution` - ---- - -### Phase 2: Resolver (A2) - -#### Task 2.1: Create src/utils/community-index.js (minimal fetch+cache wrapper) -- **Change:** Create a minimal `src/utils/community-index.js` if it does not already exist from Sprint 2. This is a thin wrapper needed by the resolver. Export: - - `COMMUNITY_INDEX_URL = 'https://raw.githubusercontent.com/dsiddharth2/plugvault/main/community-index.json'` - - `fetchCommunityIndex()` — fetch + 1hr cache, mirrors `fetchRegistry` pattern using `getCacheDir()` - - `getStaleCommunityIndexCache()` — offline fallback - - Cache key: `community-index.json` in same `getCacheDir()` as vault caches - - **IMPORTANT:** If Sprint 2 already landed `src/utils/community-index.js`, read it first and only add/adjust what is missing. Do not overwrite Sprint 2 work. -- **Files:** `src/utils/community-index.js` -- **Tier:** standard -- **Done when:** `fetchCommunityIndex` and `getStaleCommunityIndexCache` exported and functional; module imports cleanly -- **Blockers:** Check if file exists from Sprint 2 first (`git show origin/main:src/utils/community-index.js`) - -#### Task 2.2: Create src/utils/resolver.js -- **Change:** New file implementing DFS dependency resolver. Public API: - ```js - export async function resolve(pkgName, vaultHint = null, options = {}) - // returns Promise<{ toInstall, alreadySatisfied, cycles }> - ``` - Private helpers: - - `buildPackageMap()` — fetch official registry.json → Map; fetch community-index.json → add/override; community wins on name conflict; both catch-wrapped (offline-safe) - - `dfsResolve(name, packageMap, installedSnapshot, toInstall, alreadySatisfied, cycles, visited, inStack)` - - Algorithm: - 1. `packageMap = await buildPackageMap()` - 2. `installedSnapshot = await getInstalled(isGlobal)` — load ONCE; pass into DFS — never call per-node - 3. DFS: `inStack` hit → `cycles.push(name)`, return; `visited` hit → return; for each dep where `required !== false`: DFS(dep.name); after deps: check snapshot → alreadySatisfied or toInstall; mark visited - 4. Result is dep-first topological order; last element is always `pkgName` -- **Files:** `src/utils/resolver.js` (new) -- **Tier:** premium -- **Done when:** `resolve` exported; DFS correctly handles direct deps, transitive deps, cycles, optional deps, already-installed; `getInstalled` called once per resolve -- **Blockers:** Read `src/utils/registry.js` and `src/utils/tracker.js` for import patterns before writing - -#### Task 2.3: Create tests/resolver.test.js -- **Change:** New test file. Mock: `vi.mock('../src/utils/registry.js')`, `vi.mock('../src/utils/community-index.js')`, `vi.mock('../src/utils/tracker.js')`. Cover: - - No deps → `toInstall: [pkg]`, `alreadySatisfied: []`, `cycles: []` - - Single required dep → `toInstall: [dep, pkg]` (dep first) - - Dep already installed → `alreadySatisfied: [dep]`, `toInstall: [pkg]` - - Transitive A→B→C → `toInstall: [C, B, A]` - - Cycle A→B→A → `cycles: ['A']`, `toInstall: [B, A]` - - Optional dep (`required: false`) NOT added to `toInstall` - - Unknown package in deps → silently skipped, no crash - - All deps already installed → `toInstall: [pkg]`, `alreadySatisfied: [all deps]` - - `getInstalled` called exactly once per `resolve()` invocation (spy on `getInstalled`, assert call count = 1 even for multi-dep graphs) -- **Files:** `tests/resolver.test.js` (new) -- **Tier:** standard -- **Done when:** all 9 test cases pass -- **Blockers:** none - -#### VERIFY: Phase 2 -- Run full `npm test` — zero failures -- Confirm resolver handles transitive + cycle cases correctly -- Commit: `feat(resolver): add DFS dependency resolver` — includes resolver.js + community-index.js (if new/changed) + resolver.test.js -- Push to `sprint/dep-resolution` - ---- - -### Phase 3: Wire Install + TUI Plan Screen (A3 + A6) - -#### Task 3.1: Refactor install.js — extract helper + wire resolver -- **Change:** In `src/commands/install.js`: - 1. Extract `installSinglePackage(pkgSpec, isGlobal)` private helper: - - If `pkgSpec.rawBaseUrl`: `fetch(rawBaseUrl + entry)` directly - - Else: use existing `downloadFile(vault, ...)` path - 2. After finding the root package, before downloading: - - `const plan = await resolve(pkgName, vault.name, { global: isGlobal })` - - If CLI path and `plan.toInstall.length > 1`: print plan summary; if `!ctx.yes`, prompt "Proceed? (Y/n)" - 3. Loop `plan.toInstall` in order, calling `installSinglePackage(entry, isGlobal)` for each - 4. Track each dep with `installed_as: 'dependency'`; root with `installed_as: 'explicit'`; both include `dependencies: [direct dep names]` - 5. After all installs: call `addDependents(dep.name, [pkgName], isGlobal)` for each dep - 6. Add `vi.mock('../src/utils/resolver.js', () => ({ resolve: vi.fn().mockResolvedValue({ toInstall: [], alreadySatisfied: [], cycles: [] }) }))` at top of `tests/install.test.js` -- **Files:** `src/commands/install.js`, `tests/install.test.js` -- **Tier:** premium -- **Done when:** resolver called on install; deps tracked as `installed_as: 'dependency'`; root as `'explicit'`; `addDependents` called; already-satisfied deps skipped; `installSinglePackage` branches on `rawBaseUrl` -- **Blockers:** Read current install.js fully before modifying — identify where to inject resolver call and where `trackInstall` is called - -#### Task 3.2: New TUI component src/tui/components/install-plan.jsx -- **Change:** New Ink component. Props: `{ queue, plan, loading, onConfirm(scope: 'project'|'global'), onCancel }`. Renders: - - Spinner while `loading` - - "Will install" section listing `plan.toInstall` with vault + type - - "Already satisfied" section listing `plan.alreadySatisfied` - - Scope selector: `◉ Project (.claude/) ○ Global (~/.claude/)` — Tab toggles - - Footer: `[i] Install [Esc] Cancel` -- **Files:** `src/tui/components/install-plan.jsx` (new) -- **Tier:** standard -- **Done when:** component renders correctly in test; Tab toggles scope; `onConfirm` called with correct scope; `onCancel` called on Esc -- **Blockers:** Read existing TUI components for Ink/React patterns before writing - -#### Task 3.3: Wire plan view in discover.jsx + tests -- **Change:** - 1. Add `'plan'` state to `DiscoverView` union in `src/tui/screens/discover.jsx` - 2. When `startInstall(queue)` called: run `resolve()` in `useEffect`/`useState`; if `plan.toInstall.length > 1` → set view `'plan'`, render ``; on `onConfirm(scope)` → `doInstall(queue, { global: scope === 'global' })`; if single package → `doInstall(queue)` immediately (current behavior preserved) - 3. `ctx.set({ yes: true, json: true })` must only be called inside `doInstall()` after user confirms — NOT during resolver async phase - 4. Extend `tests/tui/discover.test.jsx`: plan view shown after pressing `i` on package with deps; skipped for no-dep package; Tab toggles scope; Esc returns to list -- **Files:** `src/tui/screens/discover.jsx`, `tests/tui/discover.test.jsx` -- **Tier:** standard -- **Done when:** plan view appears for deps, skipped for single package; scope toggle works; Esc cancels; tests pass -- **Blockers:** Read current discover.jsx fully to understand view state machine before adding `'plan'` state - -#### VERIFY: Phase 3 -- Run full `npm test` — zero failures (includes install + discover test extensions) -- Manual CLI: `plug install subagent-driven-development` → resolver prints plan, deps installed first -- Manual CLI: `plug install senior-engineer` (no deps) → installs immediately, no plan screen -- Manual TUI: package with deps → plan screen + scope toggle; no-dep package → installs immediately -- Commit: `feat(install): wire resolver + TUI scope toggle on dep installs only` — includes install.js + install-plan.jsx + discover.jsx + test updates -- Push to `sprint/dep-resolution` - ---- - -### Phase 4: Wire Remove (A4) - -#### Task 4.1: Add dependent check + cascade/force/prune to remove.js -- **Change:** In `src/commands/remove.js` after loading `pkg`: - 1. `const dependents = pkg.dependents ?? []` - 2. If `dependents.length > 0` **and** `!options._cascade`: `select` prompt (Cancel / Remove all cascade / Force remove) - - `_cascade` definition: boolean flag passed in the options object. When `true`, it means "this call was initiated by a cascade — skip the user prompt and proceed with removal immediately." This is what prevents infinite re-prompting when removing dependents recursively. - - **Cancel:** return early. - - **Cascade (shallow):** For each name in `dependents`, call `runRemove(dep, { global, _cascade: true })`. Then remove the original target. Depth: one level only — when `_cascade: true`, the called `runRemove` will skip the prompt for that dependent's own dependents (they are not removed). This is intentional shallow cascade. If deep transitive removal is needed, that is a future enhancement. - - **Force:** delete target file + `trackRemove(name)` + `removeDependentEdge(name, dep, global)` for each dependent. Does NOT remove the dependent packages — just severs the edge and removes the target. - 3. After any successful remove: call `prunableOrphans(isGlobal)`; if orphans exist and `!ctx.yes`: prompt to prune; if yes: `runRemove` each orphan (without `_cascade`) -- **Files:** `src/commands/remove.js` -- **Tier:** standard -- **Done when:** dependents check fires; `_cascade` flag skips re-prompt correctly; Cancel/Cascade/Force all work per definitions above; orphan prune prompt fires; `--yes` auto-prunes -- **Blockers:** Read current remove.js fully before modifying - -#### Task 4.2: Extend tests/remove.test.js -- **Change:** Add test cases: - - Package with dependents triggers `select` prompt - - Cancel leaves both packages in place - - Cascade removes target and all dependents - - Force removes only target, updates dependent records - - After remove, orphan prompt fires if orphans exist - - `--yes` flag auto-prunes orphans without prompting -- **Files:** `tests/remove.test.js` -- **Tier:** standard -- **Done when:** all 6 test cases pass; existing tests unaffected -- **Blockers:** none - -#### VERIFY: Phase 4 -- Run full `npm test` — zero failures -- Manual CLI: `plug remove` on package with dependents → warning fires; orphan prune prompt shown after -- Commit: `feat(remove): dependent check, cascade/force, orphan pruning` -- Push to `sprint/dep-resolution` - ---- - -### Phase 5: Post-Install Hook Notice (A5) - -#### Task 5.1: Create src/utils/frontmatter.js + wire into install.js -- **Change:** - 1. New file `src/utils/frontmatter.js`: - ```js - export function parseFrontmatter(content) - // Regex: match ---\n...\n--- block, split on newlines, parse key: value pairs, handle CRLF - // Returns Record or {} if none/malformed - ``` - 2. In `src/commands/install.js` after writing SKILL.md: call `parseFrontmatter(content)`; if `fm.hook || fm.hooks`: - - CLI: print `⚠ Hook required: '${name}' expects a hook in settings.json` - - JSON mode (`ctx.json`): include `hookRequired: true` in output object -- **Files:** `src/utils/frontmatter.js` (new), `src/commands/install.js` -- **Tier:** standard -- **Done when:** `parseFrontmatter` exports correctly; hook notice fires on install for skill with `hook:` frontmatter; no notice for skills without it -- **Blockers:** none - -#### Task 5.2: Create tests/frontmatter.test.js -- **Change:** New test file covering: - - Standard `---\nkey: value\n---` → correct object - - No frontmatter → `{}` - - Malformed (no closing `---`) → `{}` - - CRLF line endings handled - - `hook:` field parsed correctly -- **Files:** `tests/frontmatter.test.js` (new) -- **Tier:** standard -- **Done when:** all 5 test cases pass -- **Blockers:** none - -#### VERIFY: Phase 5 -- Run full `npm test` — all existing + new tests green (257+ tests from Sprint 2 + new additions) -- Manual: Install a skill with `hook:` frontmatter → warning printed -- Manual: Install a skill without hook frontmatter → no warning -- Regression: Sprint 1 + Sprint 2 fixes still work; inspect installed.json after multi-dep install — all fields correct -- Commit: `feat(install): post-install hook notice` -- Push to `sprint/dep-resolution` - ---- - -## Risk Register - -| Risk | Impact | Mitigation | -|------|--------|------------| -| resolver.js calls `isInstalled()` per-node — file read on every dep | HIGH | Load `getInstalled()` once per `resolve()` call, pass snapshot into DFS. Covered by test (Task 2.3): verify exactly 1 `getInstalled` call per resolve invocation | -| `addDependents`/`addDependents` overwrite semantics — multi-parent packages lose dependents | HIGH | Use merge/append semantics in `addDependents` (dedup). Multi-parent test in Task 1.2 covers this. | -| Community package install path: `rawBaseUrl` + `entry` instead of vault owner/repo | HIGH | `installSinglePackage` branches on `pkgSpec.rawBaseUrl` presence. Intentional vault-abstraction bypass for Sprint 3; B3 adapter tightens in future sprint | -| `ctx.set({ yes, json })` called during resolver async phase (plan view) — state pollution | MED | `ctx.set` must only be inside `doInstall()`, called after user confirms. Test: mock resolver to be slow; assert ctx not mutated until confirm | -| install.test.js mock layering — existing mocks + new resolver mock clash | MED | Add `vi.mock('../src/utils/resolver.js', ...)` at top of install.test.js. Per-test override via `resolve.mockResolvedValueOnce(...)` | -| Sprint 2's `community-index.js` shape differs from what resolver expects | MED | Task 2.1 explicitly checks existing file before writing. Read `src/utils/community-index.js` on main before modifying | - ---- - -## Notes - -- 5 commits total — one per phase. See requirements.md commit strategy. -- VERIFY tasks are checkpoints — doer stops, PM dispatches reviewer before next phase. -- Base branch: `main`. Create `sprint/dep-resolution` from `origin/main`. -- Sprint 2's `community-index.js` already exists — Task 2.1 must NOT overwrite it. -- Do NOT bump Ink, Node engines, or `package.json` version. -- Regression: Sprint 1 + Sprint 2 fixes (non-TTY guard, per-skill subdirs, alt-screen, paste, community discover) must not regress. diff --git a/plug/README.md b/plug/README.md index ec99045..efce67c 100644 --- a/plug/README.md +++ b/plug/README.md @@ -39,18 +39,30 @@ plug init ``` ### `install` -Directly install a package by name. +Directly install a package by name. If the package has dependencies, `plug` resolves and installs them first, then prompts for confirmation (skip with `--yes`). ```bash plug install code-review plug install -g my-vault/api-patterns # Global installation +plug install subagent-driven-development --yes # Auto-confirm dep plan ``` +If an installed skill declares `hook:` in its frontmatter, a notice is printed reminding you to add the hook to `settings.json`. + ### `remove` Uninstall a package. ```bash plug remove code-review +plug remove -g code-review # Remove from global ~/.claude/ +plug remove code-review --yes # Auto-prune orphaned dependencies without prompting ``` +If other packages depend on the target, `plug` shows an interactive prompt with three choices: +- **Cancel** — abort, nothing changes. +- **Remove all (cascade)** — remove dependent packages first (one level), then remove the target. +- **Force remove** — remove only the target; severs dependent edges without touching dependent packages. + +After any successful remove, `plug` checks for orphaned auto-installed dependencies. If any exist, it prompts to remove them. `--yes` skips this confirmation and prunes automatically. + ### `list` List all currently installed packages. Use `--remote` to see all available packages. ```bash diff --git a/plug/docs/api.md b/plug/docs/api.md new file mode 100644 index 0000000..1751d3f --- /dev/null +++ b/plug/docs/api.md @@ -0,0 +1,82 @@ +# API Reference + +## resolver.js + +### `resolve(pkgName, vaultHint?, options?)` + +```ts +resolve(pkgName: string, vaultHint?: string | null, options?: { global?: boolean }) + : Promise<{ toInstall: string[], alreadySatisfied: string[], cycles: string[] }> +``` + +Resolves the full dependency graph for `pkgName` and returns an install plan. + +| Return field | Type | Description | +|---|---|---| +| `toInstall` | `string[]` | Packages to install, dependency-first. Root package is last. | +| `alreadySatisfied` | `string[]` | Packages already in `installed.json` for the given scope. | +| `cycles` | `string[]` | Package names where a back-edge was detected; not added to `toInstall`. | + +`vaultHint` is accepted but not currently used for routing — the full registry+community package map is always built. `options.global` (default `false`) selects which `installed.json` is consulted for the snapshot. + +--- + +## tracker.js (Sprint 3 additions) + +### `addDependents(name, newDependents, global?)` + +```ts +addDependents(name: string, newDependents: string[], global?: boolean): Promise +``` + +Merges `newDependents` into `installed[name].dependents` (dedup via `Set`). No-ops if `name` is not in `installed`. Saves. + +### `getInstalledRecord(name, global?)` + +```ts +getInstalledRecord(name: string, global?: boolean): Promise +``` + +Returns `installed[name]` or `null` if not tracked. + +### `prunableOrphans(global?)` + +```ts +prunableOrphans(global?: boolean): Promise +``` + +Returns names where `installed_as === 'dependency'` and `dependents.length === 0`. These are candidates for automatic pruning after a remove. + +### `removeDependentEdge(fromName, toName, global?)` + +```ts +removeDependentEdge(fromName: string, toName: string, global?: boolean): Promise +``` + +Removes `fromName` from `installed[toName].dependents`. No-ops if `toName` is not tracked. Saves. + +--- + +## frontmatter.js + +### `parseFrontmatter(content)` + +```ts +parseFrontmatter(content: string): Record +``` + +Parses a YAML fence block from the top of a string. Returns `{}` on absent or malformed frontmatter. Never throws. + +`install.js` checks the returned record for `fm.hook || fm.hooks` to decide whether to emit a hook-required notice. The function itself is a pure parser with no install-specific logic. + +--- + +## trackInstall metadata (Sprint 3 additions) + +`trackInstall(name, metadata, global?)` now persists three additional optional fields from `metadata`: + +| Field | Default | Notes | +|---|---|---| +| `installed_as` | `'explicit'` | `'dependency'` for auto-installed deps | +| `dependencies` | `[]` | Direct dep names at install time | +| `dependents` | `[]` | Populated later via `addDependents` | diff --git a/plug/docs/architecture.md b/plug/docs/architecture.md new file mode 100644 index 0000000..acc0fac --- /dev/null +++ b/plug/docs/architecture.md @@ -0,0 +1,113 @@ +# Architecture + +## installed.json Schema (Sprint 3+) + +`installed.json` tracks every installed package under the `installed` key. Each record now carries three additional fields: + +```json +{ + "installed": { + "senior-engineer": { + "type": "skill", + "vault": "official", + "version": "1.0.0", + "path": ".claude/skills/senior-engineer/", + "installedAt": "2026-04-17T00:00:00.000Z", + "installed_as": "explicit", + "dependencies": [], + "dependents": [] + }, + "subagent-driven-development": { + "type": "skill", + "vault": "community", + "version": "1.0.0", + "path": ".claude/skills/subagent-driven-development/", + "installedAt": "2026-04-17T00:00:00.000Z", + "installed_as": "explicit", + "dependencies": ["using-git-worktrees", "writing-plans"], + "dependents": [] + }, + "using-git-worktrees": { + "type": "skill", + "vault": "community", + "version": "1.0.0", + "path": ".claude/skills/using-git-worktrees/", + "installedAt": "2026-04-17T00:00:00.000Z", + "installed_as": "dependency", + "dependencies": [], + "dependents": ["subagent-driven-development"] + } + } +} +``` + +Field semantics: + +| Field | Values | Default | +|-------|--------|---------| +| `installed_as` | `"explicit"` \| `"dependency"` | `"explicit"` | +| `dependencies` | Direct dep names declared at install time | `[]` | +| `dependents` | Packages that required this one | `[]` | + +Older records without these fields are normalised on read: any code path touching `rec.installed_as` uses `rec.installed_as ?? 'explicit'`. No migration is needed. + +## DFS Resolver (`src/utils/resolver.js`) + +The resolver performs a depth-first traversal of the dependency graph to produce an ordered install plan. + +**Entry point:** +```js +resolve(pkgName, vaultHint = null, options = {}) +// returns Promise<{ toInstall: string[], alreadySatisfied: string[], cycles: string[] }> +``` + +**Algorithm:** + +1. `buildPackageMap()` — fetches official registry(ies) then the community index into a single `Map`. Community entries win on name conflict. Both sources are catch-wrapped (offline-safe); a missing source is skipped. +2. `getInstalled(isGlobal)` is called **once** per `resolve()` invocation. The snapshot is passed into DFS — never called per-node. +3. DFS (`dfsResolve`): + - Back-edge (`inStack` hit) → push to `cycles`, return. + - Already-visited → return. + - For each required dep (deps with `required !== false`; unknown deps silently skipped): recurse. + - After all deps: check snapshot → push to `alreadySatisfied` or `toInstall`. + - Mark visited. +4. Result: `toInstall` is in dependency-first order; the root package is always last. + +Optional deps (`required: false`) are silently skipped. Unknown package names in dep lists are silently skipped (offline-safe). + +## Install-Plan TUI Component (`src/tui/components/install-plan.jsx`) + +Props: `{ queue, plan, loading, onConfirm(scope: 'project'|'global'), onCancel }` + +States: +- While `loading`: spinner. +- Ready: "Will install" section (from `plan.toInstall`), "Already satisfied" section (`plan.alreadySatisfied`), scope selector (`Tab` toggles between `project` and `global`), footer `[i] Install [Esc] Cancel`. + +`onConfirm` is called with the selected scope string. `onCancel` is called on Esc. + +The component is rendered by `DiscoverView` when a package has dependencies (`plan.toInstall.length > 1`). Single-package installs bypass the plan screen entirely and call `doInstall` immediately. + +`ctx.set({ yes: true, json: true })` is called inside `doInstall()` only — never during the resolver async phase — to prevent context state pollution while the plan view is displayed. + +## Frontmatter Parser (`src/utils/frontmatter.js`) + +```js +parseFrontmatter(content) +// returns Record — {} on no frontmatter or malformed input +``` + +Parses a YAML fence block at the top of a file: + +``` +--- +key: value +hook: pre-tool-use +--- +``` + +- Regex: `^---\r?\n([\s\S]*?)\r?\n---` — non-greedy, handles CRLF. +- Key-value split uses `indexOf(':')` so colons in values are preserved (e.g., `hook: pre-tool-use:v2`). +- Lines without a colon are skipped; empty keys are skipped. +- Any regex miss returns `{}` — no exception thrown. + +Used by `install.js` after writing the SKILL.md file to disk. Only invoked for `type === 'skill'` packages. diff --git a/plug/docs/decisions.md b/plug/docs/decisions.md new file mode 100644 index 0000000..caffe2d --- /dev/null +++ b/plug/docs/decisions.md @@ -0,0 +1,39 @@ +# Decisions & Trade-offs + +## DFS over topological sort + +The resolver uses iterative DFS rather than a two-pass topological sort (Kahn's algorithm). The dependency graphs for Claude Code skills are small and shallow; DFS produces a valid dep-first ordering in a single pass, handles cycle detection inline via an `inStack` set, and requires no auxiliary data structures (in-degree map, queue). Kahn's would add code for no practical gain at this scale. + +## `addDependents` merge semantics + +`addDependents(name, newDependents, global)` **merges** into the existing `dependents` array (with dedup via `Set`) rather than replacing it. + +Replacing would corrupt multi-parent packages. If package X is a dependency of both A and B: +- After installing A: `X.dependents = ['A']` +- Installing B calls `addDependents('X', ['B'])` → must yield `['A', 'B']`, not `['B']` + +Overwrite semantics would silently erase A from X's dependents, causing a false orphan detection when A is later removed. + +## Hook notice is print-only + +The post-install hook notice (`⚠ Hook required: '' expects a hook in settings.json`) is a warning, not an automatic action. `plug` does not modify `settings.json`. + +Rationale: hook configuration is settings-file surgery that can break the user's existing hooks or add hooks with wrong event names. Silently mutating `settings.json` without user review was judged higher risk than a missed notice. The user retains full control. + +## JSON mode vs CLI mode split for hook notice + +In CLI mode, the notice is printed to stdout as a chalk-yellow warning. In JSON mode (`--json`), the output object gains `hookRequired: true` (conditionally — absent when false). + +Mixing a human-readable warning string into JSON output would break machine consumers. The boolean field lets callers (CI scripts, wrapper tools) detect and act on the hook requirement without parsing prose. + +## Shallow cascade on remove + +The "Remove all (cascade)" interactive choice removes direct dependents one level deep. When a dependent is removed as part of a cascade, it does not further cascade into its own dependents. Deep transitive cascade is deferred to a future sprint. + +Rationale: deep cascade is hard to reason about and easy to trigger accidentally. Shallow cascade covers the common case (A depends on X; removing X cascades to A) with a predictable blast radius. Users who need deeper removal can repeat the command. + +## Community wins on registry name conflict + +In `buildPackageMap()`, community-index entries are loaded after official vault entries and overwrite on name collision. This allows community packages to shadow official ones when both registries contain the same name. + +Rationale: community packages are typically more specialised or customised variants; a community author explicitly publishing under the same name is expressing an intentional override. diff --git a/plug/docs/features/dependency-resolution.md b/plug/docs/features/dependency-resolution.md new file mode 100644 index 0000000..822dfa6 --- /dev/null +++ b/plug/docs/features/dependency-resolution.md @@ -0,0 +1,64 @@ +# Dependency Resolution + +Sprint 3 taught the install/remove lifecycle about dependencies. Packages can declare dependencies; `plug` resolves and installs them automatically. + +## Install + +When you run `plug install `, the resolver checks whether the target package has dependencies: + +- **No dependencies** — installs immediately, same behaviour as before Sprint 3. +- **Has dependencies** — builds an install plan and presents it before proceeding: + - CLI: prints a summary of packages to install and packages already satisfied; prompts "Proceed? (Y/n)" unless `--yes` is set. + - TUI: shows the **Install Plan** screen listing "Will install" and "Already satisfied" sections. Tab toggles between project scope (`.claude/`) and global scope (`~/.claude/`). Press `i` to confirm or Esc to cancel. + +Dependencies are installed **before** the root package (depth-first order). Each dependency is tracked as `installed_as: "dependency"`; the root package is tracked as `installed_as: "explicit"`. + +After all installs, each dependency's `dependents` list is updated to include the root package name. + +### Scope toggle (TUI only) + +The plan screen exposes a scope selector that controls whether the entire install (root + deps) lands in the project scope or globally. The selector only appears when the resolver finds additional packages to install. + +### JSON mode + +``` +plug install --json +``` + +The JSON output includes `hookRequired: true` when the installed skill declares a `hook:` or `hooks:` field in its frontmatter. + +## Remove + +``` +plug remove +plug remove --yes # Auto-prune orphans without prompting +``` + +When removing a package that other packages depend on, `plug` shows an interactive prompt: + +``` +'' is required by: , . How do you want to proceed? +> Cancel + Remove all (cascade) — removes dependents first, then this package + Force remove — removes only this package, severs dependent edges +``` + +| Choice | Behaviour | +|--------|-----------| +| Cancel | Aborts; nothing is changed. | +| Remove all (cascade) | Removes each dependent package first (one level deep), then removes the target. | +| Force remove | Removes only the target; severs dependent edges without removing dependent packages. | + +`--cascade` and `--force` are **not** CLI flags. They are choices in the interactive prompt that appears when dependents are detected. + +**Orphan pruning:** after any successful remove, `plug` checks for installed-as-dependency packages whose `dependents` list is now empty. If any exist, it prompts to prune them. `--yes` (a global flag) auto-prunes without prompting. + +## Post-install hook notice + +If an installed skill's frontmatter declares `hook:` or `hooks:`, `plug` prints: + +``` +⚠ Hook required: '' expects a hook in settings.json +``` + +This is a notice only — `plug` does not modify `settings.json`. The user must wire the hook manually. diff --git a/plug/feedback.md b/plug/feedback.md deleted file mode 100644 index c9127d8..0000000 --- a/plug/feedback.md +++ /dev/null @@ -1,122 +0,0 @@ -# Phase 2 Review — TUI Community Integration (Cumulative) - -**Reviewer:** plug-reviewer -**Date:** 2026-04-16 -**Verdict:** APPROVED - ---- - -## Phase 1 (Community Index Util) — Previously APPROVED - -No regressions. `src/utils/community-index.js` unchanged since Phase 1 approval. All 5 exports still correct. LOW finding (no try/catch on `response.json()`) acknowledged by doer — deferred to Phase 3 tests. - ---- - -## Phase 2 Findings - -### HIGH -- None. - -### MEDIUM -- None. - -### LOW -- None. - ---- - -## Checklist — `src/tui/hooks/use-packages.js` - -- [x] Community try/catch is COMPLETELY ISOLATED from the vault loop's try/catch — lines 57-67 are a separate block after the `for (const vault of vaults)` loop ends at line 55. -- [x] `networkFailCount` is NOT incremented in the community catch block — explicit comment on line 66 documents this intentional choice. -- [x] `staleFallbackCount` IS incremented on stale cache hit (line 64). -- [x] Imports `fetchCommunityIndex`, `getStaleCommunityIndexCache`, `normalizeCommunityPackage` from correct path `../../utils/community-index.js` (line 4). -- [x] Official packages still load if community fetch throws — the community block runs AFTER the vault loop completes and adds to the same `all` array; a throw in the community block is caught and does not clear `all`. - -### Risk check — "all vaults failed" error screen -- [x] VERIFIED SAFE. The error screen triggers at line 74: `if (all.length === 0 && networkFailCount > 0)`. Community failure does not increment `networkFailCount` (line 66 comment). Even if all community packages fail AND no stale cache exists, the error screen only fires if `all.length === 0` AND official vaults also failed. Community-only failure is silently swallowed — correct behavior. - -## Checklist — `src/tui/components/package-item.jsx` - -- [x] `showDeps` prop defaults to `false` (line 29). -- [x] `depStr` logic correct: `★ N dep` (singular when N=1), `★ N deps` (plural when N>1), `no deps` when N=0 or undefined (lines 46-48). `undefined > 0` is `false`, so official packages without `depCount` correctly render `· no deps`. -- [x] Existing separator logic preserved — `depStr` is appended AFTER `updateStr` in the `nameLine` template (line 53), maintaining the `name · vault · version` chain. -- [x] `depStr` only rendered when `showDeps` is truthy (line 53: `${showDeps ? depStr : ''}`). - -## Checklist — `src/tui/components/package-detail.jsx` - -- [x] `isInstalled: boolean` prop replaced with `installedNames: Set` (line 16). `isInstalled` derived locally via `installedNames.has(pkg.name)` (line 17). -- [x] Dep list renders only when `pkg.dependencies?.length > 0` (line 76). -- [x] Required deps: `•`, optional deps: `○` (line 81). -- [x] "Installing this will also install" lists only required + uninstalled deps (line 89: `d.required && !installedNames.has(d.name)`). -- [x] Per-dep installed status shown: `✓ installed` (green) or `not installed` (dim) (lines 84-86). - -## Checklist — `src/tui/screens/discover.jsx` - -- [x] `installedNames={installedNames}` passed to PackageDetail (line 196), NOT the old `isInstalled={...}`. -- [x] `showDeps={true}` passed to PackageList (line 267). -- [x] No new state introduced — `installedNames` was already present from prior work. - -## Checklist — `progress.json` - -- [x] Tasks 2.1, 2.2, 2.3, 2.4, 2.V all marked `completed` with commit SHA `feat(discover): merge community packages into Discover tab`. - -## Checklist — `npm test` - -- [x] 233 passed, 0 failures (25 test files). Matches doer's claim exactly. - -## Manual TUI Verification (NOT automated — headless limitation) - -The following checks were flagged by the doer as unverifiable headlessly. These are NOT failures — they require manual TUI interaction: - -- [ ] Discover tab shows 800+ packages (community merged in) -- [ ] Search "subagent" shows `subagent-driven-development` with `★ 6 deps` -- [ ] Search "senior-engineer" shows `· no deps` -- [ ] Detail view renders dependency list with `•`/`○` markers and installed status - -## Cross-cutting - -- [x] `git log --oneline main..HEAD` shows correct commit chain: Phase 1 review, Phase 1 feat, Phase 2 feat. -- [x] `CLAUDE.md` is NOT committed. -- [x] `.fleet-task.md` is NOT committed. -- [x] No scope creep — only declared files touched (use-packages.js, package-item.jsx, package-detail.jsx, package-list.jsx, discover.jsx, progress.json, feedback.md). -- [x] Commit message matches PLAN.md VERIFY spec. - -## Notes - -- Clean isolation pattern in `use-packages.js` — the community block is self-contained with its own try/catch and does not interact with the vault loop's error tracking. This is the correct architecture for a non-blocking secondary data source. -- The `normalizeCommunityPackage` adapter in `community-index.js` correctly adds `depCount` from `dependencies.length`, which feeds directly into `package-item.jsx`'s display logic without any coupling. -- `package-detail.jsx` prop change from `isInstalled: boolean` to `installedNames: Set` is a clean improvement — it enables per-dependency installed status checks without additional prop drilling. -- The `showDeps` prop guard in `package-item.jsx` ensures the installed tab (which doesn't pass `showDeps`) won't display dep counts, avoiding confusion for official packages that lack dependency metadata. - -**Doer:** Phase 3 complete, all tests green — 257 passed (26 files), 0 failures. 17 new community-index tests + 6 new discover/PackageDetail tests. - ---- - -## Sprint 3 Review — Dependency Resolution (Cumulative) - -**Reviewer:** plug-reviewer -**Date:** 2026-04-16 -**Verdict:** CHANGES NEEDED - ---- - -## Sprint 3 Findings - -### HIGH - -- **Task 1.1: `addDependents` merge semantics ambiguous.** Requirement stated "merges newDependents" but did not specify dedup behavior or show example of multi-parent preservation. Without explicit semantics, implementer could interpret as append-without-check, losing existing dependents when second parent installs. - - **Doer:** fixed in PLAN.md revision — Task 1.1 now includes inline clause: "Merge semantics are required: if package X is a dependency of both A and B, `addDependents('X', ['B'], global)` must preserve A in X's dependents list, not erase it." + test coverage in Task 1.2. - -- **Task 4.1: `_cascade` flag semantics not defined.** The flag is used to control recursion and avoid re-prompting, but its meaning and lifecycle are implicit. Risk of infinite recursion or confused prompt logic if implementer misunderstands when/how to pass it. - - **Doer:** fixed in PLAN.md revision — Task 4.1 now includes dedicated `_cascade` definition paragraph: "boolean flag passed in the options object. When `true`, it means 'this call was initiated by a cascade — skip the user prompt and proceed with removal immediately.' This is what prevents infinite re-prompting when removing dependents recursively." - -### MEDIUM - -- None. - -### LOW - -- None. diff --git a/plug/progress.json b/plug/progress.json deleted file mode 100644 index dadceb4..0000000 --- a/plug/progress.json +++ /dev/null @@ -1,35 +0,0 @@ -{ - "_schema": { - "type": "work | verify", - "status": "pending | completed | blocked" - }, - "project": "plug", - "plan_file": "PLAN.md", - "sprint": "dep-resolution", - "branch": "sprint/dep-resolution", - "base_branch": "main", - "created": "2026-04-16", - "tasks": [ - { "id": "1.1", "phase": 1, "step": "Extend trackInstall with installed_as/dependencies/dependents fields; add addDependents, getInstalledRecord, prunableOrphans, removeDependentEdge to tracker.js", "type": "work", "status": "pending", "tier": "standard", "commit": "", "notes": "" }, - { "id": "1.2", "phase": 1, "step": "Extend tests/tracker.test.js with 7 new cases: installed_as persist, default, addDependents append (multi-parent), addDependents dedup, addDependents single-record mutation, prunableOrphans subset, removeDependentEdge back-reference", "type": "work", "status": "pending", "tier": "standard", "commit": "", "notes": "" }, - { "id": "1.V", "phase": 1, "step": "VERIFY Phase 1: npm test green; commit 'feat(tracker): extend installed.json with installed_as/dependencies/dependents'; push sprint/dep-resolution", "type": "verify", "status": "pending", "tier": "cheap", "commit": "", "notes": "" }, - - { "id": "2.1", "phase": 2, "step": "Check if community-index.js exists from Sprint 2; if missing create minimal fetch+cache wrapper; if exists confirm fetchCommunityIndex + getStaleCommunityIndexCache exported", "type": "work", "status": "pending", "tier": "standard", "commit": "", "notes": "" }, - { "id": "2.2", "phase": 2, "step": "Create src/utils/resolver.js with buildPackageMap + DFS resolver; load getInstalled once per resolve call; resolve() public API returns { toInstall, alreadySatisfied, cycles }", "type": "work", "status": "pending", "tier": "premium", "commit": "", "notes": "" }, - { "id": "2.3", "phase": 2, "step": "Create tests/resolver.test.js covering 9 cases: no deps, single dep, already installed, transitive, cycle, optional dep, unknown dep, all deps installed, getInstalled called exactly once per resolve", "type": "work", "status": "pending", "tier": "standard", "commit": "", "notes": "" }, - { "id": "2.V", "phase": 2, "step": "VERIFY Phase 2: npm test green; commit 'feat(resolver): add DFS dependency resolver'; push sprint/dep-resolution", "type": "verify", "status": "pending", "tier": "cheap", "commit": "", "notes": "" }, - - { "id": "3.1", "phase": 3, "step": "Refactor install.js: extract installSinglePackage helper (branch on rawBaseUrl); wire resolve() call before download; loop toInstall; track installed_as correctly; call addDependents; extend tests/install.test.js with resolver mock + dep-tracking cases", "type": "work", "status": "pending", "tier": "premium", "commit": "", "notes": "" }, - { "id": "3.2", "phase": 3, "step": "Create src/tui/components/install-plan.jsx: spinner, will-install list, already-satisfied list, scope selector (Tab toggles), footer [i]/[Esc]", "type": "work", "status": "pending", "tier": "standard", "commit": "", "notes": "" }, - { "id": "3.3", "phase": 3, "step": "Wire 'plan' view state in discover.jsx; plan screen only when toInstall.length > 1; ctx.set only after confirm; extend tests/tui/discover.test.jsx with plan view + scope toggle + cancel cases", "type": "work", "status": "pending", "tier": "standard", "commit": "", "notes": "" }, - { "id": "3.V", "phase": 3, "step": "VERIFY Phase 3: npm test green; manual CLI single-pkg (no plan screen) + multi-dep (plan screen); commit 'feat(install): wire resolver + TUI scope toggle on dep installs only'; push", "type": "verify", "status": "pending", "tier": "standard", "commit": "", "notes": "" }, - - { "id": "4.1", "phase": 4, "step": "Wire remove.js: check dependents; select prompt (Cancel/Cascade/Force); cascade removes dependents then target; force removes target only + removeDependentEdge; orphan prune prompt after any remove", "type": "work", "status": "pending", "tier": "standard", "commit": "", "notes": "" }, - { "id": "4.2", "phase": 4, "step": "Extend tests/remove.test.js with 6 cases: dependents trigger prompt, cancel, cascade, force, orphan prompt, --yes auto-prune", "type": "work", "status": "pending", "tier": "standard", "commit": "", "notes": "" }, - { "id": "4.V", "phase": 4, "step": "VERIFY Phase 4: npm test green; manual CLI remove with dependents; commit 'feat(remove): dependent check, cascade/force, orphan pruning'; push", "type": "verify", "status": "pending", "tier": "cheap", "commit": "", "notes": "" }, - - { "id": "5.1", "phase": 5, "step": "Create src/utils/frontmatter.js with parseFrontmatter(); wire into install.js: after writing SKILL.md, parse frontmatter, print hook warning (CLI) or set hookRequired:true (JSON mode)", "type": "work", "status": "pending", "tier": "standard", "commit": "", "notes": "" }, - { "id": "5.2", "phase": 5, "step": "Create tests/frontmatter.test.js with 5 cases: standard parse, no frontmatter, malformed, CRLF, hook field", "type": "work", "status": "pending", "tier": "standard", "commit": "", "notes": "" }, - { "id": "5.V", "phase": 5, "step": "VERIFY Phase 5: all tests green; manual hook notice check; regression Sprint 1+2; inspect installed.json; commit 'feat(install): post-install hook notice'; push", "type": "verify", "status": "pending", "tier": "standard", "commit": "", "notes": "" } - ] -} diff --git a/plug/requirements.md b/plug/requirements.md deleted file mode 100644 index 4bf3396..0000000 --- a/plug/requirements.md +++ /dev/null @@ -1,142 +0,0 @@ -# Sprint 3 — Dependency Resolution Requirements - -## Overview - -The plug CLI currently installs packages one at a time with no dependency awareness. Running `plug install subagent-driven-development` installs only that package, leaving `using-git-worktrees` and `writing-plans` silently missing. Sprint 3 teaches the install/remove lifecycle about dependencies. - -**Depends on:** Sprint 2 (community-discover) — `community-index.js` must be in place. Sprint 2 PR #17 is open and CI green; this sprint branches from `main` after merge. - ---- - -## Problem Statement - -1. **No dep resolution on install** — packages with dependencies listed in community-index.json are installed in isolation; dependents silently missing. -2. **No dep tracking in installed.json** — `installed.json` has no `installed_as`, `dependencies`, or `dependents` fields; no way to distinguish explicit vs transitive installs. -3. **No dep check on remove** — removing a package that other packages depend on gives no warning; orphaned deps are left behind. -4. **No post-install hook notice** — skill frontmatter may declare required hooks but install never surfaces this to the user. - ---- - -## Scope - -### A1 — Extend tracker.js -Add `installed_as`, `dependencies`, `dependents` fields to `installed.json` schema. Existing records normalised on read (no migration). Add four new functions: `updateDependents`, `getInstalledRecord`, `prunableOrphans`, `removeDependentEdge`. - -Schema addition per record: -```json -{ - "api-patterns": { - "type": "skill", - "vault": "official", - "version": "1.0.0", - "path": "...", - "installedAt": "...", - "installed_as": "explicit", - "dependencies": ["senior-engineer"], - "dependents": [] - } -} -``` - -`trackInstall` gains three optional fields in `metadata`: -```js -installed_as: metadata.installed_as ?? 'explicit', -dependencies: metadata.dependencies ?? [], -dependents: metadata.dependents ?? [], -``` - -New functions: -- `updateDependents(name, dependents, global)` — sets `data.installed[name].dependents = dependents`; saves -- `getInstalledRecord(name, global)` — returns `data.installed[name] ?? null` -- `prunableOrphans(global)` — names where `installed_as === 'dependency' && dependents.length === 0` -- `removeDependentEdge(fromName, toName, global)` — removes `fromName` from `data.installed[toName].dependents`; saves - -**Backward-compat:** any read path touching `rec.installed_as` must normalise: `rec.installed_as ?? 'explicit'`. - -### A2 — resolver.js (new file) -DFS dependency resolver at `src/utils/resolver.js`. - -Public API: -```js -export async function resolve(pkgName, vaultHint = null, options = {}) -// returns { toInstall, alreadySatisfied, cycles } -``` - -Algorithm: -1. `buildPackageMap()` — fetch official registry.json then community-index.json; community wins on name conflict; both catch-wrapped (offline-safe) -2. Load `getInstalled()` once, pass snapshot into DFS — never call per-node -3. DFS: back-edge → cycle; already-visited → skip; after deps: check snapshot → alreadySatisfied or toInstall; mark visited -4. Result is naturally dep-first (topological), last element is pkgName - -`community-index.js` minimal version (fetch + 1hr cache) is needed even though full B3 normaliser adapter lands in a later sprint. Import `fetchCommunityIndex` from `src/utils/community-index.js` — this already exists from Sprint 2. - -### A3 — Wire install.js + A6 TUI scope toggle -Extract `installSinglePackage(pkgSpec, isGlobal)` private helper from `src/commands/install.js`. - -Wiring in install: -1. Call `resolve(pkgName, vault.name, { global: isGlobal })` -2. If `plan.toInstall.length > 1`: print plan summary; if `!ctx.yes`, prompt "Proceed? (Y/n)" -3. Loop `plan.toInstall` calling `installSinglePackage` -4. Track each dep with `installed_as: 'dependency'`; root with `installed_as: 'explicit'`; both include `dependencies: [direct dep names]` -5. After all installs: call `updateDependents(dep.name, [pkgName], isGlobal)` for each dep - -New TUI component `src/tui/components/install-plan.jsx`: -- Props: `{ queue, plan, loading, onConfirm(scope), onCancel }` -- Renders: spinner while loading; "Will install" list; "Already satisfied" list; scope selector (Tab toggles); footer `[i] Install [Esc] Cancel` - -discover.jsx `'plan'` state: if `plan.toInstall.length > 1` → plan screen; if single package → install immediately (current behavior). - -**Scope toggle UX decision:** plan screen only appears when resolver finds additional packages. Single packages with no deps install immediately — current behavior preserved. - -### A4 — Wire remove.js -After loading pkg from `src/commands/remove.js`: -1. Check `pkg.dependents ?? []` -2. If non-empty: `select` prompt (Cancel / Remove all cascade / Force remove) -3. After any remove: `prunableOrphans()` + prune prompt; `--yes` auto-prunes - -### A5 — Post-install hook notice -New `src/utils/frontmatter.js`: -```js -export function parseFrontmatter(content) -// returns Record or {} if none/malformed -``` -Regex: `---\n...\n---` block, parse `key: value` pairs, handle CRLF. - -In `install.js` after writing SKILL.md: if `fm.hook || fm.hooks` → warn CLI or set `hookRequired: true` in JSON mode. - ---- - -## Out of Scope - -- TUI Discover tab reading community-index.json (Phase B3 — already done in Sprint 2) -- `normalizeCommunityPackage` full adapter -- Community vault badges and dep count display beyond what Sprint 2 delivered -- Package detail dep list with ✓/✗ installed status (Sprint 2 delivered this; Sprint 3 does not change it) - ---- - -## Acceptance Criteria - -1. `npm test` — all existing tests pass + all new tests green -2. `plug install subagent-driven-development` — resolver prints plan, deps installed first, all tracked in installed.json with correct `installed_as` / `dependents` -3. `plug install senior-engineer` (no deps) — installs immediately, no plan screen -4. `plug remove subagent-driven-development` — dependent warning fires if applicable, orphan prune prompt shown after -5. Manual TUI: package with deps → plan screen + scope toggle; no-dep package → installs immediately -6. Inspect installed.json after multi-dep install — `installed_as`, `dependencies`, `dependents` correct on every record -7. Regression: all Sprint 1 + Sprint 2 fixes still work - ---- - -## Key Risks - -1. **resolver.js N-reads** — must call `getInstalled()` once per `resolve()`, pass snapshot into DFS. Per-node `isInstalled()` calls re-read the file each time. -2. **Community package install path** — community packages have `rawBaseUrl + entry`; `installSinglePackage` must branch on presence of `rawBaseUrl`. -3. **ctx mutation timing in discover.jsx** — `ctx.set({ yes: true, json: true })` inside `doInstall()` must NOT fire during resolver async phase (plan view), only after user confirms. -4. **install.test.js mock layering** — existing mocks cover registry, fetcher, tracker. A3 adds resolver import; add `vi.mock('../src/utils/resolver.js', ...)` at the top. - ---- - -## Branch - -- **Branch:** `sprint/dep-resolution` -- **Base:** `main` diff --git a/plug/src/commands/install.js b/plug/src/commands/install.js index 9113658..a8ff25a 100644 --- a/plug/src/commands/install.js +++ b/plug/src/commands/install.js @@ -4,11 +4,13 @@ import fs from 'fs/promises'; import { confirm, select } from '@inquirer/prompts'; import { findPackage, findAllPackages } from '../utils/registry.js'; import { downloadFile } from '../utils/fetcher.js'; -import { trackInstall, isInstalled, getInstalled } from '../utils/tracker.js'; +import { trackInstall, isInstalled, getInstalled, addDependents } from '../utils/tracker.js'; import { getClaudeSkillsDir, getClaudeAgentsDir, getClaudeDirForType, ensureDir } from '../utils/paths.js'; import { createSpinner } from '../utils/ui.js'; import { ctx, verbose } from '../utils/context.js'; import { pkgVersion } from '../utils/pkg-version.js'; +import { resolve } from '../utils/resolver.js'; +import { parseFrontmatter } from '../utils/frontmatter.js'; export function registerInstall(program) { program @@ -124,81 +126,113 @@ export async function runInstall(name, options = {}) { } } - // Download meta.json + entry file - const dlSpinner = createSpinner(`Downloading ${pkgName}...`); - let meta; - let content; - try { - try { - verbose(`Fetching meta.json from ${vault.name}`); - const metaContent = await downloadFile(vault, `${pkg.path}/meta.json`); - meta = JSON.parse(metaContent); - } catch { - // Fall back to registry data if meta.json is unavailable - verbose('meta.json unavailable, falling back to registry data'); - meta = { - type: pkg.type, - entry: `${pkgName}.md`, - version: pkg.version, - }; + // Resolve dependency plan + const plan = await resolve(pkgName, vault.name, { global: isGlobal }); + + // Show plan summary and prompt when there are deps to install + if (!ctx.json && plan.toInstall.length > 1) { + console.log(chalk.cyan(`\nDependency plan for '${pkgName}':`)); + console.log(chalk.white(` Will install (${plan.toInstall.length}):`)); + for (const n of plan.toInstall) { + const label = n === pkgName ? '' : chalk.dim(' (dependency)'); + console.log(` - ${n}${label}`); } + if (plan.alreadySatisfied.length > 0) { + console.log(chalk.dim(` Already satisfied: ${plan.alreadySatisfied.join(', ')}`)); + } + if (!ctx.yes) { + const proceed = await confirm({ message: 'Proceed? (Y/n)', default: true }); + if (!proceed) { + if (ctx.json) { + process.stdout.write(JSON.stringify({ status: 'aborted', name: pkgName }) + '\n'); + } else { + console.log(chalk.yellow('Aborted.')); + } + return; + } + } + } - const entryFile = meta.entry || `${pkgName}.md`; - verbose(`Downloading entry file: ${entryFile}`); - content = await downloadFile(vault, `${pkg.path}/${entryFile}`); - dlSpinner.stop(); - } catch (err) { - dlSpinner.stop(); - throw err; + // Build ordered install list — always include root even if resolver returned empty + const installOrder = [...plan.toInstall]; + if (!installOrder.includes(pkgName)) { + installOrder.push(pkgName); } - // Route to correct directory by type - const entryFile = meta.entry || `${pkgName}.md`; - const type = meta.type || pkg.type || 'command'; - let destPath; - if (type === 'skill') { - // Migrate any legacy flat .claude/skills/SKILL.md before writing new skill - await migrateLegacyFlatSkillFile(getClaudeSkillsDir(isGlobal), isGlobal); - // Each skill gets its own subdir: .claude/skills//SKILL.md - const skillSubdir = path.join(getClaudeSkillsDir(isGlobal), pkgName); - await ensureDir(skillSubdir); - destPath = path.join(skillSubdir, 'SKILL.md'); - } else { - const destDir = getClaudeDirForType(type, isGlobal); - await ensureDir(destDir); - destPath = path.join(destDir, entryFile); + // Build pkgSpec map: root is already resolved; deps need lookup + const pkgSpecMap = new Map(); + pkgSpecMap.set(pkgName, { pkg, vault, rawBaseUrl: pkg.rawBaseUrl ?? null }); + + for (const depName of installOrder) { + if (depName === pkgName || pkgSpecMap.has(depName)) continue; + try { + const depMatches = await findAllPackages(depName); + if (depMatches.length > 0) { + const { pkg: depPkg, vault: depVault } = depMatches[0]; + pkgSpecMap.set(depName, { pkg: depPkg, vault: depVault, rawBaseUrl: depPkg.rawBaseUrl ?? null }); + } + } catch { + // Skip unresolvable deps silently + } } - verbose(`Writing to ${destPath}`); - try { - await fs.writeFile(destPath, content, 'utf8'); - } catch (err) { - if (err.code === 'EACCES' || err.code === 'EPERM') { - throw Object.assign( - new Error(`Cannot write to ${destPath}. Check permissions.`), - { code: err.code }, - ); + // Root's direct dependency names (for tracking) + const rootDirectDeps = (pkg.dependencies ?? []).map(d => (typeof d === 'string' ? d : d.name)); + + // Install each package in dep-first order + let rootInstallInfo = null; + for (const entryName of installOrder) { + const entrySpec = pkgSpecMap.get(entryName); + if (!entrySpec) { + verbose(`Skipping '${entryName}' — not found in any vault`); + continue; } - throw err; + + const isRoot = entryName === pkgName; + const dlSpinner = createSpinner(`Downloading ${entryName}...`); + let installInfo; + try { + installInfo = await installSinglePackage(entrySpec, isGlobal); + dlSpinner.stop(); + } catch (err) { + dlSpinner.stop(); + throw err; + } + + const { type, destPath, version } = installInfo; + const directDeps = isRoot + ? rootDirectDeps + : (entrySpec.pkg.dependencies ?? []).map(d => (typeof d === 'string' ? d : d.name)); + + await trackInstall( + entryName, + { + type, + vault: entrySpec.vault.name, + version, + path: destPath, + installed_as: isRoot ? 'explicit' : 'dependency', + dependencies: directDeps, + }, + isGlobal, + ); + + if (isRoot) rootInstallInfo = { type, destPath, version, hookRequired: installInfo.hookRequired }; + } + + // Record reverse-dependency edges + for (const depName of installOrder) { + if (depName === pkgName) continue; + await addDependents(depName, [pkgName], isGlobal); } - // Track in installed.json - const version = meta.version || pkg.version || pkgVersion; - await trackInstall( - pkgName, - { type, vault: vault.name, version, path: destPath }, - isGlobal, - ); + // Output + const { type, destPath, version, hookRequired } = rootInstallInfo ?? { type: 'command', destPath: '', version: pkgVersion, hookRequired: false }; if (ctx.json) { - process.stdout.write(JSON.stringify({ - status: 'installed', - name: pkgName, - type, - vault: vault.name, - version, - path: destPath, - }) + '\n'); + const out = { status: 'installed', name: pkgName, type, vault: vault.name, version, path: destPath }; + if (hookRequired) out.hookRequired = true; + process.stdout.write(JSON.stringify(out) + '\n'); } else { console.log(chalk.green(`✓ Installed ${pkgName} (${type}) from ${vault.name}`)); console.log(chalk.cyan(` Path: ${destPath}`)); @@ -209,9 +243,107 @@ export async function runInstall(name, options = {}) { } else { console.log(chalk.cyan(` Usage: /${pkgName}`)); } + if (hookRequired) { + console.log(chalk.yellow(`⚠ Hook required: '${pkgName}' expects a hook in settings.json`)); + } } } +/** + * Downloads and writes a single package to disk. Branches on rawBaseUrl for community packages. + * Returns { type, destPath, version }. + */ +async function installSinglePackage(pkgSpec, isGlobal) { + const { pkg, vault, rawBaseUrl } = pkgSpec; + const pkgName = pkg.name; + + let meta; + if (rawBaseUrl) { + try { + const metaUrl = rawBaseUrl.endsWith('/') + ? `${rawBaseUrl}${pkg.path}/meta.json` + : `${rawBaseUrl}/${pkg.path}/meta.json`; + const resp = await fetch(metaUrl.replace(/([^:]\/)\/+/g, '$1')); + if (resp.ok) { + meta = await resp.json(); + } + } catch { /* ignore */ } + } else { + try { + const metaContent = await downloadFile(vault, `${pkg.path}/meta.json`); + meta = JSON.parse(metaContent); + } catch { /* ignore */ } + } + + // Fallback to registry info if meta.json missing or partial + meta = { + type: meta?.type || pkg.type || 'command', + entry: meta?.entry || pkg.entry || `${pkgName}.md`, + version: meta?.version || pkg.version || pkgVersion, + files: meta?.files || pkg.files || [] + }; + + const type = meta.type; + const entryFile = meta.entry; + const filesToDownload = meta.files.length > 0 ? meta.files : [entryFile]; + + let rootDestDir; + if (type === 'skill') { + await migrateLegacyFlatSkillFile(getClaudeSkillsDir(isGlobal), isGlobal); + rootDestDir = path.join(getClaudeSkillsDir(isGlobal), pkgName); + } else { + rootDestDir = getClaudeDirForType(type, isGlobal); + } + await ensureDir(rootDestDir); + + let primaryDestPath = ''; + let primaryContent = ''; + + for (const relativePath of filesToDownload) { + let content; + if (rawBaseUrl) { + let url; + if (rawBaseUrl.endsWith('/')) { + url = relativePath.startsWith('/') + ? `${rawBaseUrl}${relativePath.slice(1)}` + : `${rawBaseUrl}${pkg.path}/${relativePath}`; + } else { + url = relativePath.startsWith('/') + ? `${rawBaseUrl}${relativePath}` + : `${rawBaseUrl}/${pkg.path}/${relativePath}`; + } + url = url.replace(/([^:]\/)\/+/g, '$1'); + verbose(`Downloading ${relativePath} from ${url}`); + const resp = await fetch(url); + if (!resp.ok) throw new Error(`Failed to download ${relativePath}: HTTP ${resp.status}`); + content = await resp.text(); + } else { + verbose(`Downloading ${relativePath} from vault ${vault.name}`); + content = await downloadFile(vault, `${pkg.path}/${relativePath}`); + } + + // Determine local destination + // For skills, we flatten the structure into the skill directory unless we want to preserve subdirs + // For now, let's preserve subdirs relative to the package root + const destPath = (type === 'skill' && relativePath === entryFile) + ? path.join(rootDestDir, 'SKILL.md') + : path.join(rootDestDir, relativePath); + + await ensureDir(path.dirname(destPath)); + await fs.writeFile(destPath, content, 'utf8'); + + if (relativePath === entryFile) { + primaryDestPath = destPath; + primaryContent = content; + } + } + + const fm = type === 'skill' ? parseFrontmatter(primaryContent) : {}; + const hookRequired = !!(fm.hook || fm.hooks); + + return { type, destPath: primaryDestPath, version: meta.version, hookRequired }; +} + /** * Migrates a legacy flat .claude/skills/SKILL.md to .claude/skills//SKILL.md. * Parses the frontmatter for `name:` using a regex. If not parseable, logs a warning @@ -255,9 +387,9 @@ async function migrateLegacyFlatSkillFile(skillsDir, isGlobal) { // Update manifest: find the record pointing at the old path and update it try { const data = await getInstalled(isGlobal); - for (const [name, record] of Object.entries(data.installed || {})) { + for (const [recName, record] of Object.entries(data.installed || {})) { if (record.path === legacyPath) { - await trackInstall(name, { ...record, path: newPath }, isGlobal); + await trackInstall(recName, { ...record, path: newPath }, isGlobal); break; } } diff --git a/plug/src/commands/remove.js b/plug/src/commands/remove.js index e5d1b4d..7ebd007 100644 --- a/plug/src/commands/remove.js +++ b/plug/src/commands/remove.js @@ -1,6 +1,7 @@ import chalk from 'chalk'; import fs from 'fs/promises'; -import { getInstalled, trackRemove } from '../utils/tracker.js'; +import { select, confirm } from '@inquirer/prompts'; +import { getInstalled, trackRemove, prunableOrphans, removeDependentEdge } from '../utils/tracker.js'; import { ctx, verbose } from '../utils/context.js'; export function registerRemove(program) { @@ -38,8 +39,64 @@ export async function runRemove(name, options = {}) { return; } + const dependents = pkg.dependents ?? []; + + if (dependents.length > 0 && !options._cascade) { + const choice = await select({ + message: `'${name}' is required by: ${dependents.join(', ')}. How do you want to proceed?`, + choices: [ + { name: 'Cancel', value: 'cancel' }, + { name: 'Remove all (cascade) — removes dependents first, then this package', value: 'cascade' }, + { name: 'Force remove — removes only this package, severs dependent edges', value: 'force' }, + ], + }); + + if (choice === 'cancel') { + return; + } + + if (choice === 'cascade') { + for (const dep of dependents) { + await runRemove(dep, { global: isGlobal, _cascade: true }); + } + // fall through to remove the target below + } + + if (choice === 'force') { + verbose(`Removing file at ${pkg.path}`); + try { + await fs.unlink(pkg.path); + } catch (err) { + if (err.code === 'EACCES' || err.code === 'EPERM') { + throw Object.assign( + new Error(`Cannot remove '${pkg.path}'. Check permissions.`), + { code: err.code }, + ); + } + if (err.code !== 'ENOENT') { + throw err; + } + verbose(`File already gone (ENOENT), removing from tracker`); + } + + await trackRemove(name, isGlobal); + for (const dep of dependents) { + await removeDependentEdge(name, dep, isGlobal); + } + verbose(`'${name}' force-removed from tracker`); + + if (ctx.json) { + process.stdout.write(JSON.stringify({ status: 'removed', name }) + '\n'); + } else { + console.log(chalk.green(`✓ Removed ${name}`)); + } + + await _pruneOrphans(isGlobal); + return; + } + } + verbose(`Removing file at ${pkg.path}`); - // Delete the .md file try { await fs.unlink(pkg.path); } catch (err) { @@ -52,7 +109,6 @@ export async function runRemove(name, options = {}) { if (err.code !== 'ENOENT') { throw err; } - // ENOENT — file already gone, proceed to remove from tracker verbose(`File already gone (ENOENT), removing from tracker`); } @@ -64,4 +120,29 @@ export async function runRemove(name, options = {}) { } else { console.log(chalk.green(`✓ Removed ${name}`)); } + + await _pruneOrphans(isGlobal); +} + +async function _pruneOrphans(isGlobal) { + const orphans = await prunableOrphans(isGlobal); + if (orphans.length === 0) return; + + if (ctx.yes) { + for (const orphan of orphans) { + await runRemove(orphan, { global: isGlobal }); + } + return; + } + + const shouldPrune = await confirm({ + message: `${orphans.length} orphaned package(s) found: ${orphans.join(', ')}. Remove them?`, + default: false, + }); + + if (shouldPrune) { + for (const orphan of orphans) { + await runRemove(orphan, { global: isGlobal }); + } + } } diff --git a/plug/src/tui/components/install-plan.jsx b/plug/src/tui/components/install-plan.jsx new file mode 100644 index 0000000..0a29fa1 --- /dev/null +++ b/plug/src/tui/components/install-plan.jsx @@ -0,0 +1,112 @@ +import React, { useState, useRef } from 'react'; +import { Box, Text, useInput } from 'ink'; +import Spinner from './spinner.jsx'; + +/** + * Dependency install plan screen. + * Shows what will be installed and what's already satisfied, + * lets the user pick project vs global scope, then confirm or cancel. + * + * @param {{ + * queue: Array<{ name: string, vault: string, type: string }>, + * plan: { toInstall: string[], alreadySatisfied: string[], cycles: string[] }, + * loading: boolean, + * onConfirm: (scope: 'project' | 'global') => void, + * onCancel: () => void, + * }} props + */ +export default function InstallPlan({ queue, plan, loading, onConfirm, onCancel }) { + const [scope, setScope] = useState(/** @type {'project' | 'global'} */ ('project')); + const confirmedRef = useRef(false); + + useInput((input, key) => { + if (loading) return; + + if (key.tab || key.leftArrow || key.rightArrow) { + setScope((s) => (s === 'project' ? 'global' : 'project')); + return; + } + + if (input === 'i' || input === 'y' || key.return) { + if (!confirmedRef.current) { + confirmedRef.current = true; + onConfirm(scope); + } + return; + } + + if (key.escape) { + onCancel(); + } + }); + + if (loading) { + return ( + + + + ); + } + + const toInstall = plan?.toInstall ?? []; + const alreadySatisfied = plan?.alreadySatisfied ?? []; + + // Build a lookup map of vault+type from the original queue + const queueMap = Object.fromEntries((queue ?? []).map((p) => [p.name, p])); + + return ( + + {/* Will install */} + + Will install ({toInstall.length}) + + + {toInstall.map((name) => { + const info = queueMap[name]; + return ( + + + + {name} + {info && ( + ({info.vault} / {info.type}) + )} + + ); + })} + + {/* Already satisfied */} + {alreadySatisfied.length > 0 && ( + + Already satisfied ({alreadySatisfied.length}) + {alreadySatisfied.map((name) => ( + + ✓ + {name} + + ))} + + )} + + {/* Scope selector */} + + + {scope === 'project' ? '◉' : '○'} Project (.claude/) + + {' '} + + {scope === 'global' ? '◉' : '○'} Global (~/.claude/) + + [Tab] toggle + + + {/* Footer */} + + [i] + Install + + [Esc] + Cancel + + + ); +} diff --git a/plug/src/tui/screens/discover.jsx b/plug/src/tui/screens/discover.jsx index 91da26e..eb24f12 100644 --- a/plug/src/tui/screens/discover.jsx +++ b/plug/src/tui/screens/discover.jsx @@ -7,14 +7,16 @@ import Spinner from '../components/spinner.jsx'; import PackageDetail from '../components/package-detail.jsx'; import InstallProgress from '../components/install-progress.jsx'; import InstallComplete from '../components/install-complete.jsx'; +import InstallPlan from '../components/install-plan.jsx'; import { usePackages } from '../hooks/use-packages.js'; import { useSearch } from '../hooks/use-search.js'; import { captureOutput, yieldToInk } from '../utils/capture-stdout.js'; import { runInstall } from '../../commands/install.js'; import { getInstalled } from '../../utils/tracker.js'; +import { resolve } from '../../utils/resolver.js'; import { ctx } from '../../utils/context.js'; -/** @typedef {'list'|'detail'|'installing'|'complete'} DiscoverView */ +/** @typedef {'list'|'detail'|'installing'|'complete'|'plan'} DiscoverView */ /** * Discover screen: lists all packages from configured vaults. @@ -34,6 +36,8 @@ export default function DiscoverScreen({ onInputCapture }) { const [installResults, setInstallResults] = useState([]); const [currentInstalling, setCurrentInstalling] = useState(null); const [installedNames, setInstalledNames] = useState(new Set()); + const [depPlan, setDepPlan] = useState(null); + const [planLoading, setPlanLoading] = useState(false); const installingRef = useRef(false); // Filtered/searched package list @@ -130,15 +134,52 @@ export default function DiscoverScreen({ onInputCapture }) { function startInstall(queue) { if (installingRef.current) return; - installingRef.current = true; setInstallQueue(queue); setInstallResults([]); setCurrentInstalling(null); - setView('installing'); - doInstall(queue).finally(() => { installingRef.current = false; }); + + // Run resolver to check if any deps need installing + setPlanLoading(true); + setDepPlan(null); + setView('plan'); + + const rootPkg = queue[0]; + resolve(rootPkg.name, rootPkg.vault, { global: false }) + .then((plan) => { + setPlanLoading(false); + setDepPlan(plan); + // Single package (no extra deps) → skip plan screen, install immediately + if (plan.toInstall.length <= 1) { + setView('installing'); + installingRef.current = true; + doInstall(queue).finally(() => { installingRef.current = false; }); + } + // else: stay on plan view so user can confirm + }) + .catch(() => { + // Resolver failed — fall back to direct install + setPlanLoading(false); + setView('installing'); + installingRef.current = true; + doInstall(queue).finally(() => { installingRef.current = false; }); + }); } - async function doInstall(queue) { + const handlePlanConfirm = useCallback((scope) => { + const queue = installQueue; + setView('installing'); + installingRef.current = true; + doInstall(queue, { global: scope === 'global' }).finally(() => { installingRef.current = false; }); + }, [installQueue]); + + const handlePlanCancel = useCallback(() => { + installingRef.current = false; + setInstallQueue([]); + setDepPlan(null); + setView('list'); + }, []); + + async function doInstall(queue, installOptions = {}) { const results = []; // Set yes=true to skip interactive prompts, json=true to get structured output ctx.set({ yes: true, json: true }); @@ -151,7 +192,7 @@ export default function DiscoverScreen({ onInputCapture }) { await yieldToInk(); try { const { captured } = await captureOutput(() => - runInstall(`${pkg.vault}/${pkg.name}`, { global: false }) + runInstall(`${pkg.vault}/${pkg.name}`, { global: installOptions.global || false }) ); // Parse JSON output to get install path let installPath = null; @@ -198,6 +239,18 @@ export default function DiscoverScreen({ onInputCapture }) { ); } + if (view === 'plan') { + return ( + + ); + } + if (view === 'installing') { return ( p.name === name && (!vaultName || p.vault === vaultName) + ); + if (communityMatch) { + const normalized = normalizeCommunityPackage(communityMatch); + return { + pkg: normalized, + vault: { name: normalized.vault, url: normalized.vaultUrl } + }; + } + } catch { + // Community failure is non-blocking + } + return null; } @@ -152,5 +174,24 @@ export async function findAllPackages(name) { // Skip unavailable vaults } } + + // Add community matches + try { + const community = await fetchCommunityIndex(); + const communityPkgs = community.packages || community; + const pkgList = Array.isArray(communityPkgs) ? communityPkgs : Object.values(communityPkgs); + for (const p of pkgList) { + if (p.name === name) { + const normalized = normalizeCommunityPackage(p); + results.push({ + pkg: normalized, + vault: { name: normalized.vault, url: normalized.vaultUrl } + }); + } + } + } catch { + // Community failure is non-blocking + } + return results; } diff --git a/plug/src/utils/resolver.js b/plug/src/utils/resolver.js new file mode 100644 index 0000000..6f4683c --- /dev/null +++ b/plug/src/utils/resolver.js @@ -0,0 +1,97 @@ +import { fetchRegistry, getStaleRegistryCache } from './registry.js'; +import { fetchCommunityIndex, getStaleCommunityIndexCache } from './community-index.js'; +import { getInstalled } from './tracker.js'; +import { getResolveOrder } from './config.js'; + +async function buildPackageMap() { + const packageMap = new Map(); + + // Load official vault registries + try { + const vaults = await getResolveOrder(); + for (const vault of vaults) { + let registry = null; + try { + registry = await fetchRegistry(vault); + } catch { + try { registry = await getStaleRegistryCache(vault.name); } catch { /* offline */ } + } + if (!registry) continue; + const packages = registry.packages || {}; + for (const [name, pkgData] of Object.entries(packages)) { + packageMap.set(name, { name, ...pkgData, _vault: vault }); + } + } + } catch { /* offline — continue with community */ } + + // Load community index — community wins on name conflict + try { + let communityData = null; + try { + communityData = await fetchCommunityIndex(); + } catch { + try { communityData = await getStaleCommunityIndexCache(); } catch { /* offline */ } + } + if (communityData) { + const packages = communityData.packages || communityData; + const pkgList = Array.isArray(packages) ? packages : Object.values(packages); + for (const pkg of pkgList) { + if (pkg.name) { + packageMap.set(pkg.name, { ...pkg, dependencies: pkg.dependencies ?? [] }); + } + } + } + } catch { /* offline */ } + + return packageMap; +} + +function dfsResolve(name, packageMap, installedSnapshot, toInstall, alreadySatisfied, cycles, visited, inStack) { + if (inStack.has(name)) { + cycles.push(name); + return; + } + if (visited.has(name)) return; + + inStack.add(name); + + const pkg = packageMap.get(name); + const deps = pkg?.dependencies ?? []; + + for (const dep of deps) { + const depName = typeof dep === 'string' ? dep : dep.name; + const required = typeof dep === 'string' ? true : (dep.required !== false); + if (!required) continue; + if (!packageMap.has(depName)) continue; // unknown dep — silently skip + dfsResolve(depName, packageMap, installedSnapshot, toInstall, alreadySatisfied, cycles, visited, inStack); + } + + inStack.delete(name); + visited.add(name); + + const isAlreadyInstalled = Object.prototype.hasOwnProperty.call(installedSnapshot.installed, name); + if (isAlreadyInstalled) { + if (!alreadySatisfied.includes(name)) alreadySatisfied.push(name); + } else { + if (!toInstall.includes(name)) toInstall.push(name); + } +} + +export async function resolve(pkgName, vaultHint = null, options = {}) { + const isGlobal = options.global ?? false; + + const [packageMap, installedSnapshot] = await Promise.all([ + buildPackageMap(), + getInstalled(isGlobal), + ]); + + const toInstall = []; + const alreadySatisfied = []; + const cycles = []; + const visited = new Set(); + const inStack = new Set(); + + dfsResolve(pkgName, packageMap, installedSnapshot, toInstall, alreadySatisfied, cycles, visited, inStack); + + return { toInstall, alreadySatisfied, cycles }; +} diff --git a/plug/src/utils/tracker.js b/plug/src/utils/tracker.js index 09c9ff1..23420ba 100644 --- a/plug/src/utils/tracker.js +++ b/plug/src/utils/tracker.js @@ -42,18 +42,72 @@ async function saveInstalled(data, global = false) { /** * Records a package as installed. * @param {string} name - * @param {object} metadata - { type, vault, version, path, installedAt } + * @param {object} metadata - { type, vault, version, path, installedAt, installed_as, dependencies, dependents } * @param {boolean} global */ export async function trackInstall(name, metadata, global = false) { const data = await getInstalled(global); data.installed[name] = { ...metadata, + installed_as: metadata.installed_as ?? 'explicit', + dependencies: metadata.dependencies ?? [], + dependents: metadata.dependents ?? [], installedAt: metadata.installedAt || new Date().toISOString(), }; await saveInstalled(data, global); } +/** + * Merges newDependents into data.installed[name].dependents (dedup); saves. + * @param {string} name + * @param {string[]} newDependents + * @param {boolean} global + */ +export async function addDependents(name, newDependents, global = false) { + const data = await getInstalled(global); + const rec = data.installed[name]; + if (!rec) return; + const existing = rec.dependents ?? []; + const merged = [...new Set([...existing, ...newDependents])]; + rec.dependents = merged; + await saveInstalled(data, global); +} + +/** + * Returns data.installed[name] ?? null. + * @param {string} name + * @param {boolean} global + */ +export async function getInstalledRecord(name, global = false) { + const data = await getInstalled(global); + return data.installed[name] ?? null; +} + +/** + * Returns names where installed_as === 'dependency' && dependents.length === 0. + * @param {boolean} global + */ +export async function prunableOrphans(global = false) { + const data = await getInstalled(global); + return Object.entries(data.installed) + .filter(([, rec]) => (rec.installed_as ?? 'explicit') === 'dependency' && (rec.dependents ?? []).length === 0) + .map(([name]) => name); +} + +/** + * Removes fromName from data.installed[toName].dependents; saves. + * @param {string} fromName + * @param {string} toName + * @param {boolean} global + */ +export async function removeDependentEdge(fromName, toName, global = false) { + const data = await getInstalled(global); + const rec = data.installed[toName]; + if (!rec) return; + rec.dependents = (rec.dependents ?? []).filter(d => d !== fromName); + await saveInstalled(data, global); +} + /** * Removes a package from the installed record. * @param {string} name diff --git a/plug/tests/frontmatter.test.js b/plug/tests/frontmatter.test.js new file mode 100644 index 0000000..7f00ea0 --- /dev/null +++ b/plug/tests/frontmatter.test.js @@ -0,0 +1,30 @@ +import { describe, it, expect } from 'vitest'; +import { parseFrontmatter } from '../src/utils/frontmatter.js'; + +describe('parseFrontmatter', () => { + it('parses a standard frontmatter block', () => { + const content = '---\nname: my-skill\nversion: 1.0\n---\n\nBody here.'; + expect(parseFrontmatter(content)).toEqual({ name: 'my-skill', version: '1.0' }); + }); + + it('returns {} when there is no frontmatter', () => { + const content = 'Just some plain content with no frontmatter.'; + expect(parseFrontmatter(content)).toEqual({}); + }); + + it('returns {} when frontmatter has no closing ---', () => { + const content = '---\nname: my-skill\nversion: 1.0\n'; + expect(parseFrontmatter(content)).toEqual({}); + }); + + it('handles CRLF line endings', () => { + const content = '---\r\nname: my-skill\r\nhook: settings\r\n---\r\n\r\nBody.'; + expect(parseFrontmatter(content)).toEqual({ name: 'my-skill', hook: 'settings' }); + }); + + it('parses the hook field correctly', () => { + const content = '---\nname: hook-skill\nhook: pre-tool-use\n---\n'; + const fm = parseFrontmatter(content); + expect(fm.hook).toBe('pre-tool-use'); + }); +}); diff --git a/plug/tests/install.test.js b/plug/tests/install.test.js index ee3c185..cae98e4 100644 --- a/plug/tests/install.test.js +++ b/plug/tests/install.test.js @@ -3,6 +3,10 @@ import os from 'os'; import path from 'path'; import fs from 'fs/promises'; +vi.mock('../src/utils/resolver.js', () => ({ + resolve: vi.fn().mockResolvedValue({ toInstall: [], alreadySatisfied: [], cycles: [] }), +})); + const tmpDir = path.join(os.tmpdir(), `plugvault-install-test-${Date.now()}`); const localSkillsDir = path.join(tmpDir, '.claude', 'skills'); const localCommandsDir = path.join(tmpDir, '.claude', 'commands'); @@ -95,6 +99,7 @@ vi.mock('../src/utils/tracker.js', () => ({ trackInstall: vi.fn(), isInstalled: vi.fn(), getInstalled: vi.fn().mockResolvedValue({ installed: {} }), + addDependents: vi.fn(), })); vi.mock('@inquirer/prompts', () => ({ @@ -104,8 +109,9 @@ vi.mock('@inquirer/prompts', () => ({ const { findPackage, findAllPackages } = await import('../src/utils/registry.js'); const { downloadFile } = await import('../src/utils/fetcher.js'); -const { trackInstall, isInstalled, getInstalled } = await import('../src/utils/tracker.js'); +const { trackInstall, isInstalled, getInstalled, addDependents } = await import('../src/utils/tracker.js'); const { confirm, select } = await import('@inquirer/prompts'); +const { resolve } = await import('../src/utils/resolver.js'); const { runInstall } = await import('../src/commands/install.js'); describe('plug install', () => { @@ -414,4 +420,70 @@ describe('plug install', () => { expect(newContent).toBe('# another-skill content'); }); + // ── Phase 3: resolver integration ──────────────────────────────────────── + + it('calls resolve with pkgName, vault name, and scope before installing', async () => { + await runInstall('code-review', {}); + + expect(resolve).toHaveBeenCalledWith('code-review', 'official', { global: false }); + }); + + it('installs deps as installed_as:dependency and root as installed_as:explicit', async () => { + const depPkg = { name: 'dep-tool', type: 'command', version: '1.0.0', path: 'registry/dep-tool', description: '' }; + // Resolver returns dep only; root is added by install.js guard + resolve.mockResolvedValueOnce({ toInstall: ['dep-tool'], alreadySatisfied: [], cycles: [] }); + findAllPackages + .mockResolvedValueOnce([{ pkg: samplePkg, vault: sampleVault }]) // initial code-review lookup + .mockResolvedValueOnce([{ pkg: depPkg, vault: sampleVault }]); // dep-tool dep lookup + downloadFile + .mockReset() + .mockResolvedValueOnce(JSON.stringify({ name: 'dep-tool', type: 'command', version: '1.0.0', entry: 'dep-tool.md' })) + .mockResolvedValueOnce('# dep-tool content') + .mockResolvedValueOnce(JSON.stringify(sampleMeta)) + .mockResolvedValueOnce('# code-review content'); + + await runInstall('code-review', {}); + + expect(trackInstall).toHaveBeenCalledWith( + 'dep-tool', + expect.objectContaining({ installed_as: 'dependency' }), + false, + ); + expect(trackInstall).toHaveBeenCalledWith( + 'code-review', + expect.objectContaining({ installed_as: 'explicit' }), + false, + ); + }); + + it('calls addDependents for each dep with the root as dependent', async () => { + const depPkg = { name: 'dep-tool', type: 'command', version: '1.0.0', path: 'registry/dep-tool', description: '' }; + resolve.mockResolvedValueOnce({ toInstall: ['dep-tool'], alreadySatisfied: [], cycles: [] }); + findAllPackages + .mockResolvedValueOnce([{ pkg: samplePkg, vault: sampleVault }]) + .mockResolvedValueOnce([{ pkg: depPkg, vault: sampleVault }]); + downloadFile + .mockReset() + .mockResolvedValueOnce(JSON.stringify({ name: 'dep-tool', type: 'command', version: '1.0.0', entry: 'dep-tool.md' })) + .mockResolvedValueOnce('# dep-tool content') + .mockResolvedValueOnce(JSON.stringify(sampleMeta)) + .mockResolvedValueOnce('# code-review content'); + + await runInstall('code-review', {}); + + expect(addDependents).toHaveBeenCalledWith('dep-tool', ['code-review'], false); + expect(addDependents).not.toHaveBeenCalledWith('code-review', expect.anything(), expect.anything()); + }); + + it('skips already-satisfied deps — does not download or track them', async () => { + resolve.mockResolvedValueOnce({ toInstall: ['code-review'], alreadySatisfied: ['some-dep'], cycles: [] }); + + await runInstall('code-review', {}); + + // Only root: 2 downloadFile calls (meta + entry) + expect(downloadFile).toHaveBeenCalledTimes(2); + expect(trackInstall).toHaveBeenCalledTimes(1); + expect(addDependents).not.toHaveBeenCalled(); + }); + }); diff --git a/plug/tests/registry.test.js b/plug/tests/registry.test.js index e610eee..ff9a4d0 100644 --- a/plug/tests/registry.test.js +++ b/plug/tests/registry.test.js @@ -18,7 +18,26 @@ vi.mock('../src/utils/paths.js', async (importOriginal) => { }; }); +vi.mock('../src/utils/community-index.js', () => ({ + fetchCommunityIndex: vi.fn(), + normalizeCommunityPackage: (pkg) => ({ + name: pkg.name, + vault: pkg.vault, + vaultUrl: pkg.vaultUrl, + version: pkg.version ?? '?', + type: pkg.type, + description: pkg.description ?? '', + tags: pkg.tags ?? [], + path: pkg.directory, + entry: pkg.entry, + rawBaseUrl: pkg.rawBaseUrl, + dependencies: pkg.dependencies ?? [], + source: 'community', + }), +})); + const { getCachedRegistry, cacheRegistry, fetchRegistry, findPackage, findAllPackages } = await import('../src/utils/registry.js'); +const { fetchCommunityIndex } = await import('../src/utils/community-index.js'); // registry.json uses an object-based packages schema (key = package name) const sampleRegistry = { @@ -28,6 +47,20 @@ const sampleRegistry = { }, }; +const sampleCommunityIndex = { + packages: [ + { + name: 'requesting-code-review', + vault: 'superpowers', + vaultUrl: 'https://github.com/user/superpowers', + type: 'skill', + version: '1.1.0', + directory: 'skills/requesting-code-review', + rawBaseUrl: 'https://raw.githubusercontent.com/user/superpowers/main', + } + ] +}; + const sampleVault = { name: 'official', owner: 'dsiddharth2', repo: 'plugvault', branch: 'main', private: false }; describe('registry utils', () => { @@ -40,6 +73,7 @@ describe('registry utils', () => { default_vault: 'official', }; await fs.writeFile(configPath, JSON.stringify(config), 'utf8'); + fetchCommunityIndex.mockResolvedValue(sampleCommunityIndex); }); afterEach(async () => { @@ -132,6 +166,15 @@ describe('registry utils', () => { expect(result.pkg.type).toBe('skill'); }); + it('finds community package when not in official vault', async () => { + await cacheRegistry('official', sampleRegistry); + const result = await findPackage('requesting-code-review'); + expect(result).not.toBeNull(); + expect(result.pkg.name).toBe('requesting-code-review'); + expect(result.pkg.source).toBe('community'); + expect(result.vault.name).toBe('superpowers'); + }); + it('returns null when package not found', async () => { await cacheRegistry('official', sampleRegistry); const result = await findPackage('nonexistent'); @@ -149,15 +192,30 @@ describe('registry utils', () => { const result = await findPackage('code-review', 'other-vault'); expect(result).toBeNull(); }); + + it('finds community package by specific vault name', async () => { + const result = await findPackage('requesting-code-review', 'superpowers'); + expect(result).not.toBeNull(); + expect(result.pkg.name).toBe('requesting-code-review'); + expect(result.vault.name).toBe('superpowers'); + }); }); describe('findAllPackages', () => { - it('returns all vaults containing the package', async () => { + it('returns all vaults containing the package including community', async () => { await cacheRegistry('official', sampleRegistry); + // Mock code-review in community too to test multi-source + fetchCommunityIndex.mockResolvedValue({ + packages: [ + ...sampleCommunityIndex.packages, + { name: 'code-review', vault: 'other-vault', type: 'command' } + ] + }); + const results = await findAllPackages('code-review'); - expect(results).toHaveLength(1); - expect(results[0].pkg.name).toBe('code-review'); - expect(results[0].vault.name).toBe('official'); + expect(results).toHaveLength(2); + expect(results.map(r => r.vault.name)).toContain('official'); + expect(results.map(r => r.vault.name)).toContain('other-vault'); }); it('returns empty array when package not found', async () => { diff --git a/plug/tests/remove.test.js b/plug/tests/remove.test.js index 8777386..42100a4 100644 --- a/plug/tests/remove.test.js +++ b/plug/tests/remove.test.js @@ -17,16 +17,26 @@ vi.mock('../src/utils/paths.js', async (importOriginal) => { }; }); +vi.mock('@inquirer/prompts', () => ({ + select: vi.fn(), + confirm: vi.fn(), +})); + const { runRemove } = await import('../src/commands/remove.js'); +const { select, confirm } = await import('@inquirer/prompts'); +const { ctx } = await import('../src/utils/context.js'); describe('plug remove', () => { beforeEach(async () => { await fs.mkdir(path.dirname(localInstalledFile), { recursive: true }); await fs.mkdir(path.dirname(globalInstalledFile), { recursive: true }); + vi.clearAllMocks(); + ctx.reset(); }); afterEach(async () => { await fs.rm(tmpDir, { recursive: true, force: true }); + ctx.reset(); }); it('removes an installed package and deletes the file', async () => { @@ -165,4 +175,227 @@ describe('plug remove', () => { const globalData = JSON.parse(await fs.readFile(globalInstalledFile, 'utf8')); expect(globalData.installed['code-review']).toBeDefined(); }); + + // Phase 4 dependent check tests + + it('package with dependents triggers select prompt', async () => { + const fakeFile = path.join(tmpDir, '.claude', 'commands', 'pkg-x.md'); + await fs.mkdir(path.dirname(fakeFile), { recursive: true }); + await fs.writeFile(fakeFile, '# pkg-x', 'utf8'); + + await fs.writeFile( + localInstalledFile, + JSON.stringify({ + installed: { + 'pkg-x': { + type: 'command', vault: 'official', version: '1.0.0', path: fakeFile, + installed_as: 'explicit', dependents: ['pkg-a'], + }, + 'pkg-a': { + type: 'command', vault: 'official', version: '1.0.0', + path: path.join(tmpDir, '.claude', 'commands', 'pkg-a.md'), + installed_as: 'explicit', dependents: [], + }, + }, + }), + 'utf8', + ); + + select.mockResolvedValue('cancel'); + + await runRemove('pkg-x', {}); + + expect(select).toHaveBeenCalledOnce(); + const callArgs = select.mock.calls[0][0]; + expect(callArgs.message).toContain('pkg-a'); + }); + + it('cancel leaves both packages in place', async () => { + const pkgXFile = path.join(tmpDir, '.claude', 'commands', 'pkg-x.md'); + const pkgAFile = path.join(tmpDir, '.claude', 'commands', 'pkg-a.md'); + await fs.mkdir(path.dirname(pkgXFile), { recursive: true }); + await fs.writeFile(pkgXFile, '# pkg-x', 'utf8'); + await fs.writeFile(pkgAFile, '# pkg-a', 'utf8'); + + await fs.writeFile( + localInstalledFile, + JSON.stringify({ + installed: { + 'pkg-x': { + type: 'command', vault: 'official', version: '1.0.0', path: pkgXFile, + installed_as: 'explicit', dependents: ['pkg-a'], + }, + 'pkg-a': { + type: 'command', vault: 'official', version: '1.0.0', path: pkgAFile, + installed_as: 'explicit', dependents: [], + }, + }, + }), + 'utf8', + ); + + select.mockResolvedValue('cancel'); + + await runRemove('pkg-x', {}); + + // Both files should still exist + await expect(fs.access(pkgXFile)).resolves.toBeUndefined(); + await expect(fs.access(pkgAFile)).resolves.toBeUndefined(); + + // Both should still be in tracker + const data = JSON.parse(await fs.readFile(localInstalledFile, 'utf8')); + expect(data.installed['pkg-x']).toBeDefined(); + expect(data.installed['pkg-a']).toBeDefined(); + }); + + it('cascade removes target and all dependents', async () => { + const pkgXFile = path.join(tmpDir, '.claude', 'commands', 'pkg-x.md'); + const pkgAFile = path.join(tmpDir, '.claude', 'commands', 'pkg-a.md'); + await fs.mkdir(path.dirname(pkgXFile), { recursive: true }); + await fs.writeFile(pkgXFile, '# pkg-x', 'utf8'); + await fs.writeFile(pkgAFile, '# pkg-a', 'utf8'); + + await fs.writeFile( + localInstalledFile, + JSON.stringify({ + installed: { + 'pkg-x': { + type: 'command', vault: 'official', version: '1.0.0', path: pkgXFile, + installed_as: 'explicit', dependents: ['pkg-a'], + }, + 'pkg-a': { + type: 'command', vault: 'official', version: '1.0.0', path: pkgAFile, + installed_as: 'explicit', dependents: [], + }, + }, + }), + 'utf8', + ); + + select.mockResolvedValue('cascade'); + + await runRemove('pkg-x', {}); + + // Both files should be deleted + await expect(fs.access(pkgXFile)).rejects.toThrow(); + await expect(fs.access(pkgAFile)).rejects.toThrow(); + + // Both should be removed from tracker + const data = JSON.parse(await fs.readFile(localInstalledFile, 'utf8')); + expect(data.installed['pkg-x']).toBeUndefined(); + expect(data.installed['pkg-a']).toBeUndefined(); + }); + + it('force removes only target and severs dependent edges', async () => { + const pkgXFile = path.join(tmpDir, '.claude', 'commands', 'pkg-x.md'); + const pkgAFile = path.join(tmpDir, '.claude', 'commands', 'pkg-a.md'); + await fs.mkdir(path.dirname(pkgXFile), { recursive: true }); + await fs.writeFile(pkgXFile, '# pkg-x', 'utf8'); + await fs.writeFile(pkgAFile, '# pkg-a', 'utf8'); + + await fs.writeFile( + localInstalledFile, + JSON.stringify({ + installed: { + 'pkg-x': { + type: 'command', vault: 'official', version: '1.0.0', path: pkgXFile, + installed_as: 'explicit', dependents: ['pkg-a'], + }, + 'pkg-a': { + type: 'command', vault: 'official', version: '1.0.0', path: pkgAFile, + installed_as: 'explicit', dependents: ['pkg-x'], + }, + }, + }), + 'utf8', + ); + + select.mockResolvedValue('force'); + + await runRemove('pkg-x', {}); + + // Only pkg-x file should be deleted + await expect(fs.access(pkgXFile)).rejects.toThrow(); + await expect(fs.access(pkgAFile)).resolves.toBeUndefined(); + + // pkg-x removed from tracker; pkg-a still present + const data = JSON.parse(await fs.readFile(localInstalledFile, 'utf8')); + expect(data.installed['pkg-x']).toBeUndefined(); + expect(data.installed['pkg-a']).toBeDefined(); + + // Edge severed: pkg-a.dependents no longer contains 'pkg-x' + expect(data.installed['pkg-a'].dependents).not.toContain('pkg-x'); + }); + + it('orphan prompt fires after remove when orphans exist', async () => { + const pkgAFile = path.join(tmpDir, '.claude', 'commands', 'pkg-a.md'); + await fs.mkdir(path.dirname(pkgAFile), { recursive: true }); + await fs.writeFile(pkgAFile, '# pkg-a', 'utf8'); + + // dep-x is a dependency with no dependents — already a prunable orphan + await fs.writeFile( + localInstalledFile, + JSON.stringify({ + installed: { + 'pkg-a': { + type: 'command', vault: 'official', version: '1.0.0', path: pkgAFile, + installed_as: 'explicit', dependents: [], + }, + 'dep-x': { + type: 'command', vault: 'official', version: '1.0.0', + path: path.join(tmpDir, '.claude', 'commands', 'dep-x.md'), + installed_as: 'dependency', dependents: [], + }, + }, + }), + 'utf8', + ); + + confirm.mockResolvedValue(false); + + await runRemove('pkg-a', {}); + + // Orphan prompt should have fired + expect(confirm).toHaveBeenCalledOnce(); + + // dep-x still in tracker (user declined) + const data = JSON.parse(await fs.readFile(localInstalledFile, 'utf8')); + expect(data.installed['dep-x']).toBeDefined(); + }); + + it('--yes auto-prunes orphans without prompting', async () => { + const pkgAFile = path.join(tmpDir, '.claude', 'commands', 'pkg-a.md'); + const depXFile = path.join(tmpDir, '.claude', 'commands', 'dep-x.md'); + await fs.mkdir(path.dirname(pkgAFile), { recursive: true }); + await fs.writeFile(pkgAFile, '# pkg-a', 'utf8'); + await fs.writeFile(depXFile, '# dep-x', 'utf8'); + + await fs.writeFile( + localInstalledFile, + JSON.stringify({ + installed: { + 'pkg-a': { + type: 'command', vault: 'official', version: '1.0.0', path: pkgAFile, + installed_as: 'explicit', dependents: [], + }, + 'dep-x': { + type: 'command', vault: 'official', version: '1.0.0', path: depXFile, + installed_as: 'dependency', dependents: [], + }, + }, + }), + 'utf8', + ); + + ctx.set({ yes: true }); + + await runRemove('pkg-a', {}); + + // confirm should NOT have been called (--yes skips prompt) + expect(confirm).not.toHaveBeenCalled(); + + // dep-x should be auto-pruned + const data = JSON.parse(await fs.readFile(localInstalledFile, 'utf8')); + expect(data.installed['dep-x']).toBeUndefined(); + }); }); diff --git a/plug/tests/resolver.test.js b/plug/tests/resolver.test.js new file mode 100644 index 0000000..90c31b4 --- /dev/null +++ b/plug/tests/resolver.test.js @@ -0,0 +1,168 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest'; + +vi.mock('../src/utils/registry.js', () => ({ + fetchRegistry: vi.fn(), + getStaleRegistryCache: vi.fn(), +})); + +vi.mock('../src/utils/community-index.js', () => ({ + fetchCommunityIndex: vi.fn(), + getStaleCommunityIndexCache: vi.fn(), +})); + +vi.mock('../src/utils/tracker.js', () => ({ + getInstalled: vi.fn(), +})); + +vi.mock('../src/utils/config.js', () => ({ + getResolveOrder: vi.fn(), +})); + +import { resolve } from '../src/utils/resolver.js'; +import { fetchRegistry, getStaleRegistryCache } from '../src/utils/registry.js'; +import { fetchCommunityIndex, getStaleCommunityIndexCache } from '../src/utils/community-index.js'; +import { getInstalled } from '../src/utils/tracker.js'; +import { getResolveOrder } from '../src/utils/config.js'; + +function makeRegistry(packages) { + return { packages }; +} + +function makeInstalled(names) { + const installed = {}; + for (const n of names) installed[n] = { type: 'skill', vault: 'official', version: '1.0.0' }; + return { installed }; +} + +beforeEach(() => { + vi.resetAllMocks(); + getResolveOrder.mockResolvedValue([]); + fetchRegistry.mockRejectedValue(new Error('no vault')); + getStaleRegistryCache.mockResolvedValue(null); + fetchCommunityIndex.mockRejectedValue(new Error('offline')); + getStaleCommunityIndexCache.mockResolvedValue(null); + getInstalled.mockResolvedValue(makeInstalled([])); +}); + +function setupPackages(packages) { + fetchCommunityIndex.mockResolvedValue({ packages }); +} + +describe('resolver', () => { + it('no deps → toInstall: [pkg], alreadySatisfied: [], cycles: []', async () => { + setupPackages([{ name: 'senior-engineer', dependencies: [] }]); + getInstalled.mockResolvedValue(makeInstalled([])); + + const result = await resolve('senior-engineer'); + + expect(result.toInstall).toEqual(['senior-engineer']); + expect(result.alreadySatisfied).toEqual([]); + expect(result.cycles).toEqual([]); + }); + + it('single required dep → toInstall: [dep, pkg] (dep first)', async () => { + setupPackages([ + { name: 'pkg-a', dependencies: [{ name: 'dep-x', required: true }] }, + { name: 'dep-x', dependencies: [] }, + ]); + getInstalled.mockResolvedValue(makeInstalled([])); + + const result = await resolve('pkg-a'); + + expect(result.toInstall).toEqual(['dep-x', 'pkg-a']); + expect(result.alreadySatisfied).toEqual([]); + }); + + it('dep already installed → alreadySatisfied: [dep], toInstall: [pkg]', async () => { + setupPackages([ + { name: 'pkg-b', dependencies: [{ name: 'dep-y', required: true }] }, + { name: 'dep-y', dependencies: [] }, + ]); + getInstalled.mockResolvedValue(makeInstalled(['dep-y'])); + + const result = await resolve('pkg-b'); + + expect(result.toInstall).toEqual(['pkg-b']); + expect(result.alreadySatisfied).toEqual(['dep-y']); + }); + + it('transitive A→B→C → toInstall: [C, B, A]', async () => { + setupPackages([ + { name: 'pkg-A', dependencies: [{ name: 'pkg-B', required: true }] }, + { name: 'pkg-B', dependencies: [{ name: 'pkg-C', required: true }] }, + { name: 'pkg-C', dependencies: [] }, + ]); + getInstalled.mockResolvedValue(makeInstalled([])); + + const result = await resolve('pkg-A'); + + expect(result.toInstall).toEqual(['pkg-C', 'pkg-B', 'pkg-A']); + }); + + it('cycle A→B→A → cycles: [A], toInstall contains B and A', async () => { + setupPackages([ + { name: 'cyc-A', dependencies: [{ name: 'cyc-B', required: true }] }, + { name: 'cyc-B', dependencies: [{ name: 'cyc-A', required: true }] }, + ]); + getInstalled.mockResolvedValue(makeInstalled([])); + + const result = await resolve('cyc-A'); + + expect(result.cycles).toContain('cyc-A'); + expect(result.toInstall).toContain('cyc-B'); + expect(result.toInstall).toContain('cyc-A'); + }); + + it('optional dep (required: false) NOT added to toInstall', async () => { + setupPackages([ + { name: 'pkg-opt', dependencies: [{ name: 'opt-dep', required: false }] }, + { name: 'opt-dep', dependencies: [] }, + ]); + getInstalled.mockResolvedValue(makeInstalled([])); + + const result = await resolve('pkg-opt'); + + expect(result.toInstall).toEqual(['pkg-opt']); + expect(result.toInstall).not.toContain('opt-dep'); + }); + + it('unknown package in deps → silently skipped, no crash', async () => { + setupPackages([ + { name: 'pkg-unknown-dep', dependencies: [{ name: 'ghost-pkg', required: true }] }, + ]); + getInstalled.mockResolvedValue(makeInstalled([])); + + const result = await resolve('pkg-unknown-dep'); + + expect(result.toInstall).toEqual(['pkg-unknown-dep']); + expect(result.cycles).toEqual([]); + }); + + it('all deps already installed → toInstall: [pkg], alreadySatisfied: [all deps]', async () => { + setupPackages([ + { name: 'pkg-all-installed', dependencies: [{ name: 'dep-1', required: true }, { name: 'dep-2', required: true }] }, + { name: 'dep-1', dependencies: [] }, + { name: 'dep-2', dependencies: [] }, + ]); + getInstalled.mockResolvedValue(makeInstalled(['dep-1', 'dep-2'])); + + const result = await resolve('pkg-all-installed'); + + expect(result.toInstall).toEqual(['pkg-all-installed']); + expect(result.alreadySatisfied).toContain('dep-1'); + expect(result.alreadySatisfied).toContain('dep-2'); + }); + + it('getInstalled called exactly once per resolve() invocation (even for multi-dep graphs)', async () => { + setupPackages([ + { name: 'root-pkg', dependencies: [{ name: 'dep-m1', required: true }, { name: 'dep-m2', required: true }] }, + { name: 'dep-m1', dependencies: [{ name: 'dep-m2', required: true }] }, + { name: 'dep-m2', dependencies: [] }, + ]); + getInstalled.mockResolvedValue(makeInstalled([])); + + await resolve('root-pkg'); + + expect(getInstalled).toHaveBeenCalledTimes(1); + }); +}); diff --git a/plug/tests/tracker.test.js b/plug/tests/tracker.test.js index 9fe3dfd..6606d00 100644 --- a/plug/tests/tracker.test.js +++ b/plug/tests/tracker.test.js @@ -18,7 +18,7 @@ vi.mock('../src/utils/paths.js', async (importOriginal) => { }; }); -const { getInstalled, trackInstall, trackRemove, isInstalled } = await import('../src/utils/tracker.js'); +const { getInstalled, trackInstall, trackRemove, isInstalled, addDependents, getInstalledRecord, prunableOrphans, removeDependentEdge } = await import('../src/utils/tracker.js'); describe('tracker utils', () => { beforeEach(async () => { @@ -83,4 +83,58 @@ describe('tracker utils', () => { expect(await isInstalled('code-review', true)).toBe(true); expect(await isInstalled('code-review', false)).toBe(false); }); + + it('trackInstall with installed_as: dependency persists correctly', async () => { + await trackInstall('dep-pkg', { type: 'skill', vault: 'official', version: '1.0.0', path: '/p', installed_as: 'dependency' }, false); + const data = await getInstalled(false); + expect(data.installed['dep-pkg'].installed_as).toBe('dependency'); + expect(data.installed['dep-pkg'].dependencies).toEqual([]); + expect(data.installed['dep-pkg'].dependents).toEqual([]); + }); + + it('trackInstall without installed_as defaults to explicit', async () => { + await trackInstall('explicit-pkg', { type: 'skill', vault: 'official', version: '1.0.0', path: '/p' }, false); + const data = await getInstalled(false); + expect(data.installed['explicit-pkg'].installed_as).toBe('explicit'); + }); + + it('addDependents appends to existing dependents (multi-parent accumulation)', async () => { + await trackInstall('shared-dep', { type: 'skill', vault: 'official', version: '1.0.0', path: '/p', dependents: ['pkg-a'] }, false); + await addDependents('shared-dep', ['pkg-b'], false); + const rec = await getInstalledRecord('shared-dep', false); + expect(rec.dependents).toEqual(['pkg-a', 'pkg-b']); + }); + + it('addDependents deduplicates (calling twice with same name produces no duplicate)', async () => { + await trackInstall('dedup-dep', { type: 'skill', vault: 'official', version: '1.0.0', path: '/p', dependents: ['pkg-a'] }, false); + await addDependents('dedup-dep', ['pkg-a'], false); + await addDependents('dedup-dep', ['pkg-a'], false); + const rec = await getInstalledRecord('dedup-dep', false); + expect(rec.dependents).toEqual(['pkg-a']); + }); + + it('addDependents mutates only the targeted record (other records unaffected)', async () => { + await trackInstall('target-dep', { type: 'skill', vault: 'official', version: '1.0.0', path: '/p', dependents: [] }, false); + await trackInstall('other-pkg', { type: 'skill', vault: 'official', version: '1.0.0', path: '/p', dependents: [] }, false); + await addDependents('target-dep', ['pkg-a'], false); + const other = await getInstalledRecord('other-pkg', false); + expect(other.dependents).toEqual([]); + }); + + it('prunableOrphans returns packages with installed_as=dependency and no dependents', async () => { + await trackInstall('orphan-dep', { type: 'skill', vault: 'official', version: '1.0.0', path: '/p', installed_as: 'dependency', dependents: [] }, false); + await trackInstall('needed-dep', { type: 'skill', vault: 'official', version: '1.0.0', path: '/p', installed_as: 'dependency', dependents: ['parent'] }, false); + await trackInstall('explicit-root', { type: 'skill', vault: 'official', version: '1.0.0', path: '/p', installed_as: 'explicit', dependents: [] }, false); + const orphans = await prunableOrphans(false); + expect(orphans).toContain('orphan-dep'); + expect(orphans).not.toContain('needed-dep'); + expect(orphans).not.toContain('explicit-root'); + }); + + it('removeDependentEdge removes back-reference correctly', async () => { + await trackInstall('dep-with-ref', { type: 'skill', vault: 'official', version: '1.0.0', path: '/p', dependents: ['parent-a', 'parent-b'] }, false); + await removeDependentEdge('parent-a', 'dep-with-ref', false); + const rec = await getInstalledRecord('dep-with-ref', false); + expect(rec.dependents).toEqual(['parent-b']); + }); }); diff --git a/plug/tests/tui/discover.test.jsx b/plug/tests/tui/discover.test.jsx index 70c5e35..02b744a 100644 --- a/plug/tests/tui/discover.test.jsx +++ b/plug/tests/tui/discover.test.jsx @@ -20,6 +20,10 @@ vi.mock('../../src/commands/install.js', () => ({ runInstall: vi.fn(async () => {}), })); +vi.mock('../../src/utils/resolver.js', () => ({ + resolve: vi.fn(async () => ({ toInstall: [], alreadySatisfied: [], cycles: [] })), +})); + vi.mock('../../src/utils/context.js', () => ({ ctx: { set: vi.fn() }, })); @@ -28,6 +32,7 @@ vi.mock('../../src/utils/context.js', () => ({ const { default: DiscoverScreen } = await import('../../src/tui/screens/discover.jsx'); const { default: PackageDetail } = await import('../../src/tui/components/package-detail.jsx'); const { usePackages } = await import('../../src/tui/hooks/use-packages.js'); +const { resolve } = await import('../../src/utils/resolver.js'); const MOCK_PACKAGES = [ { name: 'code-review', vault: 'official', version: '1.0.0', type: 'command', description: 'Review code' }, @@ -189,6 +194,120 @@ describe('DiscoverScreen', () => { expect(frame).toContain('not installed'); }); + // ── Plan view ────────────────────────────────────────────────────────────── + + it('shows plan view when package has deps (toInstall.length > 1)', async () => { + vi.mocked(usePackages).mockReturnValue({ + packages: [MOCK_COMMUNITY_PACKAGES[0]], // agent-fleet with deps + loading: false, + error: null, + warning: null, + }); + // Resolver returns multiple packages → plan screen should show + vi.mocked(resolve).mockResolvedValueOnce({ + toInstall: ['dep-one', 'agent-fleet'], + alreadySatisfied: [], + cycles: [], + }); + + const { lastFrame, stdin } = render( {}} />); + await new Promise((r) => setTimeout(r, 50)); + + // Press 'i' to start install + stdin.write('i'); + await new Promise((r) => setTimeout(r, 200)); + + const frame = lastFrame(); + expect(frame).toContain('Will install'); + }); + + it('skips plan view for a no-dep package (installs immediately)', async () => { + vi.mocked(usePackages).mockReturnValue({ + packages: [MOCK_COMMUNITY_PACKAGES[1]], // simple-tool with no deps + loading: false, + error: null, + warning: null, + }); + // Resolver returns only root → skip plan screen + vi.mocked(resolve).mockResolvedValueOnce({ + toInstall: ['simple-tool'], + alreadySatisfied: [], + cycles: [], + }); + + const { runInstall } = await import('../../src/commands/install.js'); + vi.mocked(runInstall).mockResolvedValue(undefined); + + const { lastFrame, stdin } = render( {}} />); + await new Promise((r) => setTimeout(r, 50)); + + stdin.write('i'); + await new Promise((r) => setTimeout(r, 200)); + + // Should go directly to installing view (not plan view) + const frame = lastFrame(); + expect(frame).not.toContain('Will install'); + }); + + it('Tab key toggles scope between Project and Global in plan view', async () => { + vi.mocked(usePackages).mockReturnValue({ + packages: [MOCK_COMMUNITY_PACKAGES[0]], + loading: false, + error: null, + warning: null, + }); + vi.mocked(resolve).mockResolvedValueOnce({ + toInstall: ['dep-one', 'agent-fleet'], + alreadySatisfied: [], + cycles: [], + }); + + const { lastFrame, stdin } = render( {}} />); + await new Promise((r) => setTimeout(r, 50)); + + stdin.write('i'); + await new Promise((r) => setTimeout(r, 200)); + + // Default scope is project + expect(lastFrame()).toContain('◉ Project'); + + // Tab toggles to global + stdin.write('\t'); + await new Promise((r) => setTimeout(r, 50)); + + expect(lastFrame()).toContain('◉ Global'); + }); + + it('Esc in plan view returns to list', async () => { + vi.mocked(usePackages).mockReturnValue({ + packages: [MOCK_COMMUNITY_PACKAGES[0]], + loading: false, + error: null, + warning: null, + }); + vi.mocked(resolve).mockResolvedValueOnce({ + toInstall: ['dep-one', 'agent-fleet'], + alreadySatisfied: [], + cycles: [], + }); + + const { lastFrame, stdin } = render( {}} />); + await new Promise((r) => setTimeout(r, 50)); + + stdin.write('i'); + await new Promise((r) => setTimeout(r, 200)); + + // Plan view is showing + expect(lastFrame()).toContain('Will install'); + + // Esc cancels back to list + stdin.write('\x1B'); + await new Promise((r) => setTimeout(r, 50)); + + expect(lastFrame()).toContain('agent-fleet'); + expect(lastFrame()).not.toContain('Will install'); + }); + it('"Installing this will also install" shows only uninstalled required deps', () => { const pkg = { ...MOCK_COMMUNITY_PACKAGES[0], diff --git a/plug/tests/tui/install-plan.test.jsx b/plug/tests/tui/install-plan.test.jsx new file mode 100644 index 0000000..0d01f63 --- /dev/null +++ b/plug/tests/tui/install-plan.test.jsx @@ -0,0 +1,77 @@ +import React from 'react'; +import { describe, it, expect } from 'vitest'; +import { render } from 'ink-testing-library'; +import InstallPlan from '../../src/tui/components/install-plan.jsx'; + +describe('InstallPlan', () => { + const queue = [{ name: 'code-reviewer', vault: 'superpowers', type: 'skill' }]; + const plan = { toInstall: ['code-reviewer'], alreadySatisfied: [], cycles: [] }; + + it('renders correctly with installation plan', () => { + const { lastFrame } = render( + {}} onCancel={() => {}} /> + ); + const frame = lastFrame(); + expect(frame).toContain('Will install (1)'); + expect(frame).toContain('code-reviewer'); + expect(frame).toContain('Project (.claude/)'); + expect(frame).toContain('Global (~/.claude/)'); + }); + + it('toggles scope on right arrow', async () => { + const { lastFrame, stdin } = render( + {}} onCancel={() => {}} /> + ); + + // Initial state: Project is selected + expect(lastFrame()).toContain('◉ Project'); + expect(lastFrame()).toContain('○ Global'); + + // Wait for useInput to register + await new Promise((r) => setTimeout(r, 100)); + + // Press right arrow + stdin.write('\x1B[C'); + await new Promise((r) => setTimeout(r, 100)); + + expect(lastFrame()).toContain('○ Project'); + expect(lastFrame()).toContain('◉ Global'); + }); + + it('toggles scope on left arrow', async () => { + const { lastFrame, stdin } = render( + {}} onCancel={() => {}} /> + ); + + // Initial state: Project is selected + expect(lastFrame()).toContain('◉ Project'); + + // Wait for useInput to register + await new Promise((r) => setTimeout(r, 100)); + + // Press left arrow + stdin.write('\x1B[D'); + await new Promise((r) => setTimeout(r, 100)); + + expect(lastFrame()).toContain('○ Project'); + expect(lastFrame()).toContain('◉ Global'); + }); + + it('toggles scope on Tab', async () => { + const { lastFrame, stdin } = render( + {}} onCancel={() => {}} /> + ); + + expect(lastFrame()).toContain('◉ Project'); + + // Wait for useInput to register + await new Promise((r) => setTimeout(r, 100)); + + // Press Tab + stdin.write('\t'); + await new Promise((r) => setTimeout(r, 100)); + + expect(lastFrame()).toContain('○ Project'); + expect(lastFrame()).toContain('◉ Global'); + }); +});