diff --git a/docs/plans/2026-08-10-001-feat-skill-update-deletion-handling-plan.md b/docs/plans/2026-08-10-001-feat-skill-update-deletion-handling-plan.md new file mode 100644 index 00000000..87fdb962 --- /dev/null +++ b/docs/plans/2026-08-10-001-feat-skill-update-deletion-handling-plan.md @@ -0,0 +1,296 @@ +--- +title: "feat(cli): Update skills with safe upstream-deletion handling" +date: 2026-08-10 +type: feat +artifact_contract: ce-unified-plan/v1 +artifact_readiness: implementation-ready +product_contract_source: ce-plan-bootstrap +execution: code +--- + +# feat(cli): Update skills with safe upstream-deletion handling + +## Goal Capsule + +- **Objective:** Add `allagents skill update [skills...]` with project, user, and combined scope support, including a clear confirmation flow when an installed skill disappeared from its upstream source. +- **Authority:** User confirmation controls deletion. AllAgents may mutate only configuration and client artifacts it already owns. +- **Execution profile:** Preflight remote sources without mutating the persistent cache, batch confirmed removals, refresh accepted sources, and sync each affected scope once. +- **Stop conditions:** Do not classify a fetch/discovery failure as deletion. Do not refresh an affected source when deletion is declined or cannot be confirmed safely. +- **Tail ownership:** Complete automated verification, isolated project-scope UAT, and document exact UAT commands/results in the draft PR. + +--- + +## Product Contract + +### Summary + +AllAgents can add, remove, and list skills, but it has no skill-focused update command. Users currently have to update whole plugins, and a normal sync can silently purge an AllAgents-managed skill after its upstream source removes it. The new command makes that destructive transition visible and consent-driven while retaining scriptable behavior. + +### Problem Frame + +Skill installations are source-driven: one plugin or repository can provide several enabled skills to several clients. The default symlink mode points client installations into a repository-level cache, so updating the cache before asking about a deleted skill can already break the supposedly retained local copy. Deletion detection therefore has to happen against a temporary upstream checkout before the persistent source cache or managed client paths change. + +### Requirements + +**Command and scope** + +- R1. `allagents skill update [skills...]` updates enabled installed skills in project, user, or both scopes. +- R2. Explicit `--scope project|user|all` wins; interactive invocations without a scope offer Project, User, and All; non-interactive or `--yes` invocations choose project when a project config exists and otherwise user. +- R3. Skill-name filters are case-insensitive and select touched physical refresh units and survivor updates. Once a unit is touched, deletion preflight covers every enabled installed sibling backed by that cache across project and user scope because refreshing the shared checkout can affect them all. If an impacted installation lies outside the selected mutation scope, the unit is retained/skipped with guidance to rerun using `--scope all`. + +**Deletion safety** + +- R4. Before persistent refresh, AllAgents compares its enabled pre-update skill inventory with full-depth discovery from a temporary checkout of each physical refresh unit. Inventory is derived from raw plugin entries so object-form `pin`, inline refs, marketplace plugin selection, configured selector, scope, and qualified skill subpath are retained. A unit is the connected dependency graph of canonical remote/ref/cache nodes required to resolve its entries: one node for a direct source, a marketplace node for embedded entries, or marketplace plus external-repository nodes for external entries. Shared nodes join aliases, marketplace siblings, and cross-scope installs into one decision boundary. +- R5. Fetch, authentication, malformed manifest, missing declared root, and incomplete discovery are reported as source failures and never interpreted as upstream deletions. An existing resolved root with a valid empty inventory may classify installed skills as deleted. A valid marketplace that no longer declares an installed plugin is a distinct authoritative removal: confirmation explicitly removes that entire config entry and every AllAgents-managed artifact from it, not only its skills; declining retains the old marketplace checkout and local artifacts. +- R6. Interactive deletion candidates are grouped by physical refresh unit, list every affected selected-scope installation, and require one affirmative confirmation per unit. The prompt states that Yes removes the named skills and updates survivors, while No keeps them and skips every update backed by that unit. Marketplace-entry removal uses separate copy naming the whole plugin entry and managed-artifact consequence. +- R7. Confirmed deletions use an atomic per-scope/per-refresh-unit configuration transform, preserving unrelated plugin-entry fields and user-owned client files. All new config bytes are prepared before any rename; on any transform, write, or exact-revision checkout-node update failure, every config and cache node belonging to that unit is restored to its original bytes/revision and the unit is not synced. +- R8. All deletion decisions are collected before mutation. Declining keeps local copies by skipping persistent refresh for that unit. Cancelling any prompt aborts before configuration or persistent caches are mutated. +- R9. `--yes`, JSON, and non-TTY execution never delete implicitly. They report retained deleted-upstream skills, skip affected units, and continue safe updates elsewhere. + +**Results and compatibility** + +- R10. Preflight records the inspected commit for every checkout node. Accepted units advance external/dependency nodes first and marketplace/root nodes last to those exact commits; rollback metadata retains every previous revision until reconciliation succeeds. After accepted caches/configs are reconciled, each selected scope syncs at most once with `offline: true`, so a declined unit cannot refresh indirectly during whole-scope validation. Removing a confirmed-missing marketplace config entry before sync lets the existing previous sync state purge its owned artifacts even when it was the last plugin. +- R11. Nested skills with duplicate leaf names are addressed by qualified subpath, while existing bare-name configurations remain compatible. +- R12. The command exposes deterministic JSON output without prompts or decorative output. Results use `updated`, `removed`, `retained`, `skipped`, `failed`, or `cancelled`. Complete/no-op/retained-safe runs exit 0 with `success: true`; cancellation exits 0 with `success: false`; operational failures exit 1 with partial results; invalid scope or filters exit 2. Human output follows the same result model. +- R13. Existing `skill add`, `skill remove`, `plugin update`, and workspace sync behavior remains compatible. + +### Key Flows + +- F1. Interactive update with confirmed deletion + - **Trigger:** A user runs `allagents skill update` in a project workspace. + - **Steps:** Select scope, preflight sources, review grouped warnings, confirm deletion, batch-prune owned configuration, refresh accepted sources, sync once, and view separate totals. + - **Outcome:** Deleted skills are removed and surviving skills are current. +- F2. Interactive retention + - **Trigger:** The preflight finds one or more deleted-upstream skills and the user answers No. + - **Steps:** Keep current configuration and cache for that source, report that its update was skipped, and process unrelated sources. + - **Outcome:** The retained local copies still work and no destructive mutation occurred for the affected source. +- F3. Automated update + - **Trigger:** The command runs with `--yes`, JSON output, or non-TTY input. + - **Steps:** Auto-resolve scope, preflight sources, retain any deletion candidates without prompting, skip their sources, update safe sources, and emit structured results. + - **Outcome:** Automation is deterministic and never authorizes deletion by omission. + +### Acceptance Examples + +- AE1. Given a project source with seven enabled skills whose upstream now contains five, when the user confirms removal, then two skills are removed, five survivors update, and the summary reports both counts. +- AE2. Given the same source, when the user declines, then neither the persistent cache nor managed copies for that source change and the summary reports two retained skills plus one skipped source. +- AE3. Given a non-interactive invocation and a deletion candidate, when the update runs, then no prompt appears, no deletion occurs, and the affected source is retained/skipped. +- AE4. Given a deeply nested skill that remains at its tracked qualified path and another skill with the same leaf name elsewhere, when full-depth discovery runs, then neither identity is conflated or falsely classified as deleted. +- AE5. Given one failing source and one healthy source, when update runs, then the failure is reported, the healthy source still updates, and the command exits non-zero. +- AE6. Given project and user entries that share one persistent checkout, when only project scope is selected and upstream deleted a user-installed sibling, then the checkout remains unchanged and the user is told to rerun with `--scope all`. + +### Scope Boundaries + +- The command updates remote GitHub and marketplace-backed sources already supported by AllAgents. Local-path sources are listed as skipped because there is no remote to refresh. +- This work does not introduce a per-skill canonical archive or retain deleted skills while simultaneously refreshing the same repository-level source; safe retention skips that source. +- This work does not change general workspace-sync deletion policy or migrate sync-state to per-skill content hashes. +- TUI plugin-update deduplication is deferred; the new command should reuse core orchestration that a future TUI action can call. + +--- + +## Planning Contract + +### Key Technical Decisions + +- KTD1. **Preflight in temporary checkouts.** Resolve each unique remote source into a disposable checkout and discover skill paths there before touching persistent caches. This preserves the meaning of a No answer under symlink installs and follows the safety shape of `vercel-labs/skills` without copying its lock-file model. +- KTD2. **Skip an affected source on retention.** AllAgents cannot update surviving skills from a repository-level cache while truthfully retaining a deleted symlinked skill without adding a new archival subsystem. Skipping only that source is the smallest deterministic ownership-safe behavior. +- KTD3. **Treat the connected checkout graph as the transaction boundary.** Build nodes from canonical remote, effective ref/pin, and cache path; connect marketplace manifests to embedded or external plugin checkouts; then attach every project/user/plugin entry consuming any shared node. A retained or out-of-scope impacted entry blocks the entire connected unit. +- KTD4. **Separate orchestration from presentation.** A core/service result describes per-scope, per-refresh-unit, and per-skill outcomes. The cmd-ts handler owns Clack prompts and human/JSON rendering, enabling focused unit tests without terminal coupling. +- KTD5. **Batch through configuration ownership.** Prepare every selector mutation for a refresh unit in memory, validate the complete result, then replace affected config files atomically. Do not recursively invoke `skill remove`, which would repeat scans, prompts, syncs, and outros. +- KTD6. **Safe automation retains.** Match upstream's important safety semantic: `--yes` means no questions, not permission to delete. JSON and non-TTY execution follow the same retention rule. +- KTD7. **Reconcile inspected revisions, then sync offline.** Preflight returns a typed resolution plus immutable SHA for every checkout node. For an accepted unit, stage all config transforms, record original bytes/SHAs, advance leaf/external nodes before their marketplace/root nodes, commit config replacements, and run scope sync offline. On failure, restore config bytes and reset changed nodes in reverse order before unrelated units continue. For an authoritatively removed marketplace entry, the staged transform removes its complete plugin entry so offline sync can purge paths from previous sync state without resolving the missing entry. + +### High-Level Technical Design + +```mermaid +flowchart TB + A[Resolve scope and filters] --> B[Snapshot enabled installed skills] + B --> C[Build physical units across both scopes] + C --> D[Clone exact ref and discover in temp] + D --> E{Typed preflight result} + E -->|Resolution failure| F[Record failure; do not refresh] + E -->|Resolved / valid empty / entry removed| G{Deletion candidates?} + G -->|No| L[Queue exact revision] + G -->|Yes| H{All impacts in scope?} + H -->|No| J[Retain and skip unit] + H -->|Yes| P{Interactive confirmation} + P -->|Yes| I[Stage atomic config transforms] + P -->|No or automation| J + P -->|Cancel| K[Abort before mutation] + I --> L + L --> Q[Advance cache to inspected SHA] + Q --> M[Sync each changed scope offline once] + F --> N[Aggregate results] + J --> N + M --> N + N --> O[Human summary or JSON envelope] +``` + +### System-Wide Impact + +- **Configuration:** Allowlist removals can delete a plugin entry when its last enabled skill disappears; blocklist/implicit installations rely on the confirmed sync purge and do not add meaningless exclusions for already-absent upstream content. +- **Cache:** Preflight checkouts are temporary and cleaned safely; persistent caches change only after the source is accepted for refresh. +- **Clients:** Sync retains its existing selective ownership boundary and purges only AllAgents-tracked paths after confirmation. +- **Automation:** JSON and non-TTY output remain prompt-free and deterministic. + +### Typed preflight outcomes + +| Outcome | Meaning | Deletion classification | +|---|---|---| +| `resolved` | The selected plugin root exists and discovery completed, with zero or more skills. | Missing installed qualified paths are deletion candidates; zero skills is authoritative. | +| `plugin-removed` | A valid marketplace no longer declares the installed plugin entry. | The entire plugin entry is a removal candidate; confirmation names its skills and all managed plugin artifacts. | +| `local` | The entry resolves to a local path with no remote refresh unit. | Never a deletion candidate; report skipped. | +| `failed` | Clone/auth/ref/manifest/root/discovery resolution failed or was incomplete. | Never a deletion candidate; report failed and leave cache/config unchanged. | + +### Upstream parity and intentional differences + +| Behavior | `vercel-labs/skills` | AllAgents design | +|---|---|---| +| Detect before mutation | Temporary source tree versus lock data. | Temporary exact-ref checkout versus source-derived cross-scope inventory. | +| Interactive deletion | Prompt per source; Yes removes, No keeps. | Prompt per physical refresh unit; Yes atomically removes, No keeps and skips the whole shared unit. | +| `--yes` / non-TTY | Warn and skip deletion. | Retain and skip the affected unit; continue unrelated units. | +| Survivor update after No | Continues because installs are independent copies. | Skips the unit because managed copies may symlink into one shared checkout. | +| Shared cross-scope cache | Not applicable. | Any out-of-scope impacted install blocks refresh and recommends `--scope all`. | + +### JSON and exit contract + +| Condition | `success` | Exit | Required result detail | +|---|---:|---:|---| +| Updated, no-op, or safe retained/skipped units only | `true` | 0 | Per-unit and per-skill statuses plus totals. | +| User cancels during decision collection | `false` | 0 | `cancelled`; no mutation occurred. | +| One or more operational/preflight/reconciliation failures | `false` | 1 | Failed units and successful partial work remain visible. | +| Invalid scope/filter invocation | `false` | 2 | Validation error; orchestration never starts. | + +### Risks and Mitigations + +- A marketplace plugin may be embedded or point to an external repository. Resolve the temporary discovery root with the same manifest rules as persistent update and test both shapes. +- External marketplace entries form a compound unit. Record marketplace and external checkout SHAs, advance external nodes before the manifest node, and roll back changed nodes in reverse order on failure. +- Direct GitHub subpaths, marketplace siblings, and project/user entries can share one cache checkout. Canonicalize the physical unit and inventory both scopes before making any decision. +- Object `pin` and inline `@ref` can select a different revision than the raw source. Share one effective-source resolver with normal sync and make the ref part of the unit key. +- Snapshot discovery can seed the process-level fetch cache with offline results. Keep preflight cache-independent or reset the fetch cache before persistent refresh. +- A successful fresh clone can currently collapse to a skipped plugin-update result. Decide sync eligibility from successful accepted refresh work, not only `action === updated`. +- Multiple removed siblings can leave an empty allowlist if each helper sees the original snapshot. Compute the whole config transform in memory, write once, and test last-skill removal. +- Qualified nested selectors can be lost if helpers use leaf names. Prefer the configured subpath selector and preserve bare-name fallback. + +--- + +## Implementation Units + +### U1. Preflight and update orchestration + +- **Goal:** Build a testable, non-mutating preflight and physical-refresh-unit update engine. +- **Requirements:** R1-R6, R8-R10, R13; F1-F3; AE6. +- **Dependencies:** None. +- **Files:** `src/core/skill-update.ts`, `src/core/git.ts`, `src/core/plugin.ts`, `src/core/sync.ts`, `src/core/skills.ts`, `src/models/workspace-config.ts`, `tests/unit/core/skill-update.test.ts`. +- **Approach:** Build inventory from raw project and user plugin entries using a shared effective-source/ref resolver; construct connected units from canonical remote/ref/cache nodes; resolve direct, embedded, and external marketplace discovery roots in temporary checkouts; return typed preflight outcomes and every exact SHA; compare qualified subpaths; collect all decisions; advance accepted dependency nodes leaf-first with reverse rollback; then sync changed scopes once with `offline: true`. +- **Execution note:** Start with failing pure detection/orchestration tests, then add source-resolution integration coverage. +- **Patterns to follow:** `src/core/plugin.ts` update result/dependency injection, `src/core/skills.ts` full-depth discovery, `src/core/git.ts` safe temp lifecycle, and `src/core/sync.ts` scope sync contracts. +- **Test scenarios:** + - Covers AE1. Two of seven enabled qualified paths disappear; both are candidates and five survivors remain refreshable. + - Covers AE2. A declined source is never passed to persistent refresh; when a second source triggers the scope-wide sync, the unchanged retained cache keeps the deleted skill intact. + - Covers AE3. Non-interactive policy retains candidates and marks the source skipped. + - Covers AE4. Nested duplicate leaf names at different subpaths are compared without collision. + - Covers AE5. Discovery failure records a failure and cannot generate deletion candidates; another source still completes. + - Covers AE6. A user-scope impacted sibling blocks a project-only refresh of their shared cache; selecting all scopes permits one grouped decision. + - Direct repository subpaths, two plugins from one marketplace, and source aliases resolve to one refresh unit when they share cache/ref. + - Inline refs and object-form pins inspect and refresh the same immutable revision; different refs remain separate units. + - Valid empty discovery and an authoritatively removed marketplace entry produce deletion candidates; missing roots, malformed manifests, and fetch failures do not. + - An external marketplace survivor update advances both inspected checkout nodes in dependency order; injected second-node failure restores both original SHAs. + - Disabled skills and local-path sources are excluded from deletion prompts and remote refresh respectively. + - Temporary checkout cleanup runs on success and thrown discovery errors. +- **Verification:** Focused tests prove no persistent refresh occurs before every unit decision is known, and declined cache SHAs remain byte-for-byte unchanged after another unit triggers offline scope sync. + +### U2. Ownership-aware batch removal + +- **Goal:** Remove confirmed deleted skills coherently without repeated command/sync UX. +- **Requirements:** R7, R10-R11, R13; AE1. +- **Dependencies:** U1. +- **Files:** `src/cli/skill-removal.ts`, `src/core/workspace-modify.ts`, `src/core/user-workspace.ts`, `tests/unit/cli/skill-removal.test.ts`, `tests/unit/core/workspace-modify-skills.test.ts`, `tests/unit/core/user-workspace-skills.test.ts`. +- **Approach:** Add a pure batch transform over parsed project/user configuration keyed by the exact plugin entry and qualified selector; preserve object-entry fields; keep an empty allowlist when the source still owns non-skill artifacts, and remove the source only when it is demonstrably a standalone skill source; treat none/blocklist entries as confirmed artifact purges without writing stale exclusions. Serialize and validate every affected config before temp-file-plus-rename replacement; retain original bytes for unit rollback. +- **Execution note:** Characterize existing single-skill behavior before extending it. +- **Patterns to follow:** Existing `removeInstalledSkill`, plugin-entry mutation helpers, and marketplace cascade ownership tests. +- **Test scenarios:** + - Multiple allowlisted deletions remove selectors sequentially; the last deletion preserves a source with commands/hooks/MCP through an empty allowlist and removes only a standalone skill source. + - A qualified nested selector is removed without affecting another skill with the same leaf name. + - Object fields for clients, install mode, artifact exclusions, and pin survive partial pruning. + - A blocklist/implicit source does not gain an exclusion for an upstream-absent skill. + - A confirmed marketplace-entry removal deletes the complete plugin config entry, preserves unrelated entries, and lets an empty-plan offline sync purge its previously tracked artifacts. + - A validation/write failure in the second deletion leaves all config files in that refresh unit unchanged and prevents cache refresh, while unrelated units continue. +- **Verification:** Project and user configuration fixtures show only the intended owned fields changed, and fault-injection tests prove per-unit all-or-nothing replacement. + +### U3. CLI UX, metadata, and structured output + +- **Goal:** Expose the orchestration as a predictable interactive and scriptable command. +- **Requirements:** R1-R3, R6, R8-R12; F1-F3. +- **Dependencies:** U1, U2. +- **Files:** `src/cli/commands/plugin-skills.ts`, `src/cli/metadata/plugin-skills.ts`, `src/cli/skill-arg-normalizer.ts`, `tests/unit/cli/skill-update.test.ts`, `tests/unit/cli/agent-help.test.ts`. +- **Approach:** Register `update`; validate scope/filter values; use Clack for scope and confirmation prompts; group warnings once per physical refresh unit with every impacted scope/plugin listed; collect all decisions before execution; interpret No as retain/skip and cancel as pre-mutation abort; render one concise summary; produce the same result model and exit contract through JSON without UI noise. +- **Patterns to follow:** Existing skill search scope picker, global JSON envelope helpers, and enriched command metadata. +- **Test scenarios:** + - Interactive scope picker offers Project, User, and All and cancellation exits without orchestration. + - Explicit scope and skill filters reach only matching candidates. + - One warning and one confirmation cover multiple deletions and aliases sharing a checkout; an out-of-scope impacted installation blocks the unit instead of receiving an independent unsafe decision. + - No retains and skips; cancel aborts; Yes calls one batch removal. + - `--yes`, JSON, and non-TTY modes do not prompt or delete. + - Summaries distinguish removed, updated, retained, skipped, and failed counts; partial failure exits non-zero. + - Cancellation returns `success: false`/exit 0 with no mutation; retained-only automation returns `success: true`/exit 0; operational failure returns exit 1; invalid scope/filter returns exit 2. + - Help metadata and singular/plural command normalization recognize the new command. +- **Verification:** Captured stdout/stderr and JSON snapshots contain no nested removal-command noise and no prompt escape sequences in JSON. + +### U4. End-to-end safety and user documentation + +- **Goal:** Prove the feature through the built CLI without touching the developer's real environment. +- **Requirements:** R1-R13; AE1-AE3, AE5-AE6. +- **Dependencies:** U3. +- **Files:** `tests/e2e/skill-update.test.ts`, `docs/src/content/docs/docs/reference/cli.mdx`. +- **Approach:** Use a temporary project, `ALLAGENTS_TEST_HOME`, and a disposable Git remote/cache fixture with two skills; establish a red behavior check before implementation, then verify Yes, No, non-interactive, failure, and surviving update paths through real CLI processes. Document command, scope, safety, and retention semantics. +- **Execution note:** Prefer a project-scope runtime smoke test over tests that use the real home directory. +- **Patterns to follow:** `tests/e2e/plugin-update.test.ts`, `tests/e2e/plugin-skills.test.ts`, and test environment helpers under `tests/helpers/`. +- **Test scenarios:** + - Covers AE1. Delete one upstream skill and modify a survivor; Yes removes the deleted artifact/config selector and updates the survivor. + - Covers AE2. No preserves the deleted skill and unchanged source cache while another source can update. + - Covers AE3. Non-TTY retains and reports without prompting. + - Covers AE5. One unavailable source yields a non-zero result while the healthy source still updates and is reported in the result. + - Covers AE6. Shared project/user cache remains at the original SHA for project-only execution; `--scope all` permits the grouped update. + - A sole implicit/blocklist marketplace plugin removed from its manifest is explicitly confirmed, removed from config, and purged from sync state without validation failure. + - An external marketplace plugin updates both its manifest checkout and changed survivor checkout to their preflighted revisions. + - Declining one source while accepting another leaves the declined cache SHA and managed artifact unchanged after the accepted source's offline scope sync. + - Project config, client artifacts, and sync-state remain isolated under temporary directories. +- **Verification:** The built `dist/index.js` passes isolated UAT, and the PR body or comment records exact setup, commands, observed prompt choices, filesystem assertions, and results. + +--- + +## Verification Contract + +| Gate | Coverage | Done signal | +|---|---|---| +| Focused unit tests | U1-U3 | Skill-update, removal, workspace mutation, and help tests pass. | +| Focused E2E | U4 | The disposable project/remote fixture passes Yes, No, and non-interactive paths. | +| Static quality | All | `bun run typecheck` and `bun run lint` pass. | +| Build | All | `bun run build` produces a runnable CLI. | +| Full unit suite | All | `bun test` passes. | +| Full E2E suite | All | `bun run test:e2e` passes. | +| Manual UAT | U3-U4 | Built CLI behavior and filesystem results are recorded on the draft PR; the real home and current repository configuration are untouched. | + +--- + +## Definition of Done + +- `allagents skill update` supports documented scope and filtering behavior. +- Upstream deletion is detected before persistent cache mutation and cannot be inferred from a failed source check. +- Interactive Yes removes confirmed owned copies/configuration; No retains them by skipping the affected source; cancellation mutates nothing. +- `--yes`, JSON, and non-TTY modes never remove deleted-upstream skills implicitly. +- Qualified nested identities, pins/refs, shared-cache aliases/scopes, multi-delete batching, last-skill source removal, and atomic failure rollback have automated coverage. +- Human and JSON summaries distinguish updates, removals, retentions, skips, and failures. +- Build, typecheck, lint, unit tests, E2E tests, and isolated manual UAT pass. +- Exact UAT steps and observed results are present in the draft PR body or a PR comment. + +--- + +## Appendix + +### Sources and research + +- `vercel-labs/skills` current implementation at commit `941a7bcfeca4bf07913b9fb6f8ed81f20ff5297c`: +- Upstream deleted-skill change, PR #1218 / commit `6b29809b7c438f1e3142f7de76572bacb0e72b72`: +- Upstream update tests: +- Existing AllAgents source refresh and sync patterns: `src/core/plugin.ts`, `src/core/skills.ts`, `src/core/sync.ts`, `src/cli/skill-removal.ts`, and `src/cli/commands/plugin.ts`. diff --git a/docs/src/content/docs/docs/reference/cli.mdx b/docs/src/content/docs/docs/reference/cli.mdx index 4a40ecba..eab543f2 100644 --- a/docs/src/content/docs/docs/reference/cli.mdx +++ b/docs/src/content/docs/docs/reference/cli.mdx @@ -134,6 +134,7 @@ allagents plugin marketplace update [name] allagents skill list [--scope ] allagents skill remove [--plugin ] [--scope ] allagents skill add [--from ] [--plugin ] [--scope ] +allagents skill update [skill...] [--scope ] [--yes] ``` ### plugin list @@ -257,6 +258,50 @@ allagents skill add brainstorming --plugin superpowers After enabling, the skill is removed from `disabledSkills` and sync is run to restore it. +### skill update + +Check installed remote skills for upstream changes, update surviving skills, and safely reconcile skills that were deleted upstream. + +```bash +allagents skill update +allagents skill update code-review glow-api +allagents skill update --scope user +allagents skill update --scope all +allagents skill update --yes +allagents --json skill update --scope project +``` + +| Argument or flag | Description | +|------|-------------| +| `[skill...]` | Update only the physical sources containing the named enabled skills. Names, qualified paths, and `plugin:path` selectors are accepted. All enabled siblings sharing a selected source are still checked for deletion safety. | +| `-s, --scope ` | Scope: `project` (default when a project config exists), `user`, or `all`. | +| `-y, --yes` | Run without prompts. This does **not** authorize deletion: a source with upstream deletions is retained and skipped. | + +AllAgents performs a read-only preflight against disposable checkouts before changing a config or plugin cache. When an installed skill has disappeared upstream, interactive terminals list the affected skill copies and ask once for the shared physical source: + +- **Yes** removes the deleted skill selectors or standalone skill entries, advances that source to the inspected revision, and updates its surviving skills. +- **No** keeps the local copies and skips every update from that physical source, including survivor updates. +- Cancelling any confirmation stops the whole operation before the first mutation. + +Non-interactive runs—including `--yes`, redirected input/output, CI, and `--json`—behave like **No** for sources with deletions. This makes unattended updates safe by default. Sources without deletion candidates can still update normally. + +Plugin subpaths, marketplace entries, and project/user installs can share one physical cache. A decision therefore applies to the complete connected cache unit rather than only the config spelling that selected it. If an update in one scope would affect a deleted skill in an unselected scope, AllAgents keeps the cache unchanged and asks you to rerun with `--scope all`. A failed or declined unit does not prevent independent physical sources from updating. + +After accepted caches advance, AllAgents syncs affected clients from those exact cached revisions in offline mode. Declined caches are not refreshed indirectly by the final sync. + +With `--json`, each physical source result has one of these statuses: + +| Status | Meaning | +|------|-------------| +| `updated` | The source advanced and surviving skills were synced; no deletion was required. | +| `removed` | Confirmed deleted skills were removed and surviving skills were updated. | +| `retained` | Deleted local copies were kept, so the complete shared source was left unchanged. | +| `skipped` | A physical update unit was intentionally skipped without an operational failure. | +| `failed` | Preflight, transaction, or offline sync failed for this source. Independent sources may still succeed. | +| `cancelled` | Confirmation was cancelled before any changes were made. | + +Local plugin sources are listed separately in `data.skippedLocalSources`. The JSON summary reports per-skill `updated`, `removed`, and `retained` counts plus physical-source `skipped`, `failed`, and `cancelled` counts. Usage errors exit with status 2, operational failures with status 1, and safe retained/skipped results with status 0. + :::tip `allagents skill ...` is the canonical singular form. The plural alias `allagents skills ...` is also accepted and produces identical output. diff --git a/src/cli/agent-help.ts b/src/cli/agent-help.ts index 8511af12..cdcd728d 100644 --- a/src/cli/agent-help.ts +++ b/src/cli/agent-help.ts @@ -19,6 +19,7 @@ import { skillsAddMeta, skillsRemoveMeta, skillsSearchMeta, + skillsUpdateMeta, } from './metadata/plugin-skills.js'; const allCommands: AgentCommandMeta[] = [ @@ -38,6 +39,7 @@ const allCommands: AgentCommandMeta[] = [ skillsAddMeta, skillsRemoveMeta, skillsSearchMeta, + skillsUpdateMeta, updateMeta, ]; @@ -82,14 +84,23 @@ function resolveAlias(commandPath: string): string { } /** - * Look up a meta by the runtime command path (e.g. "skills list"). + * Look up metadata by a runtime command path (e.g. "skill update foo"). * Resolves deprecated aliases (e.g. "workspace status" -> "status"). * Used by index.ts to validate `--json=` against the per-command - * allowlist before dispatching. + * allowlist before dispatching. A longest-prefix match allows command metadata + * to resolve when positional arguments follow the command tokens. */ -export function findMetaByCommand(commandPath: string): AgentCommandMeta | undefined { +export function findMetaByCommand( + commandPath: string, +): AgentCommandMeta | undefined { if (!commandPath) return undefined; - return allCommands.find((c) => c.command === resolveAlias(commandPath)); + const resolved = resolveAlias(commandPath); + const exact = allCommands.find((command) => command.command === resolved); + if (exact) return exact; + + return allCommands + .filter((command) => resolved.startsWith(`${command.command} `)) + .sort((a, b) => b.command.length - a.command.length)[0]; } export function printAgentHelp(args: string[], version: string): void { diff --git a/src/cli/commands/plugin-skills.ts b/src/cli/commands/plugin-skills.ts index 0661fec1..99adcf19 100644 --- a/src/cli/commands/plugin-skills.ts +++ b/src/cli/commands/plugin-skills.ts @@ -11,11 +11,7 @@ import { restPositionals, string, } from 'cmd-ts'; -import { - CONFIG_DIR, - WORKSPACE_CONFIG_FILE, - getHomeDir, -} from '../../constants.js'; +import { getHomeDir } from '../../constants.js'; import { addMarketplace, findMarketplace, @@ -87,13 +83,8 @@ import { skillsSearchMeta, } from '../metadata/plugin-skills.js'; import { removeInstalledSkill } from '../skill-removal.js'; - -/** - * Check if a directory has a project-level .allagents config - */ -function hasProjectConfig(dir: string): boolean { - return existsSync(join(dir, CONFIG_DIR, WORKSPACE_CONFIG_FILE)); -} +import { hasProjectSkillConfig } from '../skill-update.js'; +import { skillUpdateCmd } from './skill-update.js'; /** * Determine effective scope when no --scope flag is provided. @@ -101,7 +92,7 @@ function hasProjectConfig(dir: string): boolean { */ function resolveScope(cwd: string): 'user' | 'project' { if (isUserConfigPath(cwd)) return 'user'; - if (hasProjectConfig(cwd)) return 'project'; + if (hasProjectSkillConfig(cwd)) return 'project'; return 'user'; } @@ -291,7 +282,7 @@ const listCmd = command({ handler: async ({ scope }) => { try { const cwd = process.cwd(); - const inProjectDir = !isUserConfigPath(cwd) && hasProjectConfig(cwd); + const inProjectDir = !isUserConfigPath(cwd) && hasProjectSkillConfig(cwd); // Resolve which scopes to display const showUser = scope !== 'project'; @@ -2358,7 +2349,7 @@ async function installFromSearch(repos: string[]): Promise { const installableRepos: string[] = []; for (const repo of repos) { - const isInstalledProject = hasProjectConfig(workspacePath) + const isInstalledProject = hasProjectSkillConfig(workspacePath) ? await hasPlugin(repo, workspacePath) : false; const isInstalledUser = await hasUserPlugin(repo); @@ -2614,5 +2605,6 @@ export const skillsCmd = conciseSubcommands({ remove: removeCmd, add: addCmd, search: searchCmd, + update: skillUpdateCmd, }, }); diff --git a/src/cli/commands/skill-update.ts b/src/cli/commands/skill-update.ts new file mode 100644 index 00000000..46d5252a --- /dev/null +++ b/src/cli/commands/skill-update.ts @@ -0,0 +1,234 @@ +import chalk from 'chalk'; +import { + command, + flag, + option, + optional, + restPositionals, + string, +} from 'cmd-ts'; +import { buildDescription } from '../help.js'; +import { isJsonMode, jsonOutput } from '../json-output.js'; +import { skillsUpdateMeta } from '../metadata/plugin-skills.js'; +import { + executePreparedSkillUpdate, + findUnmatchedSkillUpdateFilters, + hasProjectSkillConfig, + normalizeSkillUpdateScopes, + prepareSkillUpdate, + resolveNonInteractiveSkillUpdateDecisions, + skillUpdateExitCode, + skillUpdateSummary, + unitDisplayName, +} from '../skill-update.js'; +import { terminalSafe } from '../terminal-output.js'; + +class SkillUpdateUsageError extends Error {} + +export const skillUpdateCmd = command({ + name: 'update', + description: buildDescription(skillsUpdateMeta), + args: { + skills: restPositionals({ type: string, displayName: 'skills' }), + scope: option({ + type: optional(string), + long: 'scope', + short: 's', + description: 'Scope: project, user, or all', + }), + yes: flag({ + long: 'yes', + short: 'y', + description: 'Run without prompts (deleted-upstream skills are retained)', + }), + }, + handler: async ({ skills, scope, yes }) => { + try { + const workspacePath = process.cwd(); + const interactive = + Boolean(process.stdout.isTTY && process.stdin.isTTY) && + !isJsonMode() && + !yes; + let selectedScope = scope; + + // Validate explicit input before opening any UI. + if ( + selectedScope !== undefined && + !['project', 'user', 'all'].includes(selectedScope) + ) { + throw new SkillUpdateUsageError( + `Invalid scope '${selectedScope}'. Expected project, user, or all.`, + ); + } + + if (!selectedScope && interactive) { + const p = await import('@clack/prompts'); + const selected = await p.select({ + message: 'Update scope', + options: [ + { value: 'project', label: 'Project' }, + { value: 'user', label: 'User' }, + { value: 'all', label: 'All' }, + ], + initialValue: hasProjectSkillConfig(workspacePath) + ? 'project' + : 'user', + }); + if (p.isCancel(selected)) { + p.cancel('Skill update cancelled'); + return; + } + selectedScope = selected as string; + } + selectedScope ??= hasProjectSkillConfig(workspacePath) + ? 'project' + : 'user'; + const scopes = normalizeSkillUpdateScopes(selectedScope); + + if (!isJsonMode()) console.log('Checking for skill updates…'); + const prepared = await prepareSkillUpdate({ + workspacePath, + scopes, + ...(skills.length > 0 && { filters: skills }), + }); + const unmatched = findUnmatchedSkillUpdateFilters( + prepared.inventory, + scopes, + skills, + ); + if (unmatched.length > 0) { + throw new SkillUpdateUsageError( + `No enabled installed skill matched: ${unmatched.join(', ')}`, + ); + } + + let decisions = resolveNonInteractiveSkillUpdateDecisions(prepared.plan); + if (interactive) { + decisions = {}; + const p = await import('@clack/prompts'); + for (const unit of prepared.plan.units) { + if (unit.deleted.length === 0) continue; + const displayName = terminalSafe(unitDisplayName(unit)); + if (unit.blockedByOutOfScope) { + decisions[unit.id] = 'retain'; + p.log.warn( + `${displayName} also backs deleted skills outside the selected scope. Local copies will be kept and this source will not be updated. Rerun with --scope all to review them together.`, + ); + continue; + } + + const removedEntries = unit.installations.filter((installation) => + unit.removedInstallationIds.includes(installation.id), + ); + const heading = + removedEntries.length > 0 + ? `The following installed plugin entries were deleted from ${displayName} upstream:` + : `The following skills from ${displayName} appear to have been deleted upstream:`; + const lines = unit.deleted.map( + (skill) => + ` • ${terminalSafe(skill.pluginName)}:${terminalSafe(skill.subpath)} (${terminalSafe(skill.scope)})`, + ); + p.log.warn(`${heading}\n${lines.join('\n')}`); + const confirmed = await p.confirm({ + message: + removedEntries.length > 0 + ? `Remove ${removedEntries.length} local plugin entr${removedEntries.length === 1 ? 'y' : 'ies'} and update the surviving skills? No keeps the local copies and skips every update from this source.` + : `Remove ${unit.deleted.length} local skill cop${unit.deleted.length === 1 ? 'y' : 'ies'} and update the surviving skills? No keeps them and skips every update from this source.`, + initialValue: false, + }); + if (p.isCancel(confirmed)) { + decisions[unit.id] = 'cancel'; + break; + } + decisions[unit.id] = confirmed ? 'remove' : 'retain'; + } + } + + const result = await executePreparedSkillUpdate( + prepared, + decisions, + workspacePath, + ); + const summary = skillUpdateSummary(result); + const planById = new Map( + prepared.plan.units.map((unit) => [unit.id, unit]), + ); + const results = result.units.map((unitResult) => { + const planned = planById.get(unitResult.id); + return { + ...unitResult, + source: planned ? unitDisplayName(planned) : unitResult.id, + }; + }); + + if (isJsonMode()) { + jsonOutput({ + success: result.success, + command: 'skill update', + data: { + scopes, + results, + syncedScopes: result.syncedScopes, + skippedLocalSources: prepared.inventory.skippedLocalSources, + summary, + }, + }); + } else { + for (const local of prepared.inventory.skippedLocalSources) { + console.log( + `${chalk.dim('–')} Skipped local source ${terminalSafe(local)}`, + ); + } + for (const unitResult of results) { + const label = terminalSafe(unitResult.source); + switch (unitResult.status) { + case 'updated': + console.log(`${chalk.green('✓')} Updated ${label}`); + break; + case 'removed': + console.log( + `${chalk.green('✓')} Removed deleted skills and updated ${label}`, + ); + break; + case 'retained': + console.log( + `${chalk.yellow('!')} Kept local copies and skipped updates for ${label}`, + ); + break; + case 'skipped': + console.log(`${chalk.dim('–')} Skipped ${label}`); + break; + case 'cancelled': + console.log( + `${chalk.yellow('!')} Update cancelled before changes`, + ); + break; + case 'failed': + console.error( + `${chalk.red('✗')} Failed ${label}${unitResult.error ? `: ${terminalSafe(unitResult.error)}` : ''}`, + ); + break; + } + } + if (result.units.length === 0) console.log('No skill updates found.'); + else if (result.success) { + console.log( + `Done: ${summary.updated} updated, ${summary.removed} removed, ${summary.retained} retained, ${summary.skipped} skipped.`, + ); + } + } + + const exitCode = skillUpdateExitCode(result); + if (exitCode !== 0) process.exit(exitCode); + } catch (error) { + const message = error instanceof Error ? error.message : String(error); + const exitCode = error instanceof SkillUpdateUsageError ? 2 : 1; + if (isJsonMode()) { + jsonOutput({ success: false, command: 'skill update', error: message }); + } else { + console.error(`Error: ${terminalSafe(message)}`); + } + process.exit(exitCode); + } + }, +}); diff --git a/src/cli/metadata/plugin-skills.ts b/src/cli/metadata/plugin-skills.ts index 33d94c2e..c4d5e65d 100644 --- a/src/cli/metadata/plugin-skills.ts +++ b/src/cli/metadata/plugin-skills.ts @@ -63,6 +63,75 @@ export const skillsRemoveMeta: AgentCommandMeta = { }, }; +export const skillsUpdateMeta: AgentCommandMeta = { + command: 'skill update', + description: + 'Update installed skills and safely reconcile skills deleted upstream', + whenToUse: + 'To refresh project or user skills while preserving local copies unless an interactive deletion is explicitly confirmed', + examples: [ + 'allagents skill update', + 'allagents skill update code-review glow-api', + 'allagents skill update --scope user', + 'allagents skill update --scope all', + 'allagents skill update --yes', + 'allagents --json skill update --scope project', + ], + expectedOutput: + 'Reports updated, removed, retained, skipped, failed, or cancelled refresh units', + positionals: [ + { + name: 'skills', + type: 'string', + required: false, + description: + 'Optional installed skill names or qualified subpaths to update', + }, + ], + options: [ + { + flag: '--scope', + short: '-s', + type: 'string', + description: 'Scope: project, user, or all', + }, + { + flag: '--yes', + short: '-y', + type: 'boolean', + description: + 'Run without prompts (deleted-upstream skills are retained, not removed)', + }, + ], + outputSchema: { + scopes: ['string'], + results: [ + { + id: 'string', + source: 'string', + status: 'string', + error: 'string?', + }, + ], + syncedScopes: ['string'], + skippedLocalSources: ['string'], + summary: { + updated: 'number', + removed: 'number', + retained: 'number', + skipped: 'number', + failed: 'number', + cancelled: 'number', + }, + }, + jsonFields: [ + 'id', + 'source', + 'status', + 'error', + ] as const, +}; + export const skillsSearchMeta: AgentCommandMeta = { command: 'skill search', description: diff --git a/src/cli/skill-arg-normalizer.ts b/src/cli/skill-arg-normalizer.ts index 37589b2b..fd5b62e7 100644 --- a/src/cli/skill-arg-normalizer.ts +++ b/src/cli/skill-arg-normalizer.ts @@ -1,4 +1,10 @@ -const SKILL_SUBCOMMANDS = new Set(['list', 'remove', 'add', 'search']); +const SKILL_SUBCOMMANDS = new Set([ + 'list', + 'remove', + 'add', + 'search', + 'update', +]); export function normalizeSkillAlias(args: string[]): string[] { if (args.length === 0) return args; diff --git a/src/cli/skill-removal.ts b/src/cli/skill-removal.ts index e1cedb89..30a50726 100644 --- a/src/cli/skill-removal.ts +++ b/src/cli/skill-removal.ts @@ -1,13 +1,20 @@ -import { addDisabledSkill, removeEnabledSkill, removePlugin } from '../core/workspace-modify.js'; -import { getAllSkillsFromPlugins, type SkillInfo } from '../core/skills.js'; +import { type SkillInfo, getAllSkillsFromPlugins } from '../core/skills.js'; import { addUserDisabledSkill, removeUserEnabledSkill, removeUserPlugin, } from '../core/user-workspace.js'; +import { + addDisabledSkill, + removeEnabledSkill, + removePlugin, +} from '../core/workspace-modify.js'; export interface RemoveInstalledSkillOptions { - targetSkill: Pick; + targetSkill: Pick< + SkillInfo, + 'name' | 'pluginName' | 'pluginSource' | 'pluginSkillsMode' + >; isUser: boolean; workspacePath: string; allSkills?: SkillInfo[]; @@ -23,8 +30,11 @@ export async function removeInstalledSkill( options: RemoveInstalledSkillOptions, ): Promise { const { targetSkill, isUser, workspacePath } = options; - const allSkills = options.allSkills ?? await getAllSkillsFromPlugins(workspacePath); - const pluginSkills = allSkills.filter((skill) => skill.pluginSource === targetSkill.pluginSource); + const allSkills = + options.allSkills ?? (await getAllSkillsFromPlugins(workspacePath)); + const pluginSkills = allSkills.filter( + (skill) => skill.pluginSource === targetSkill.pluginSource, + ); const remainingEnabledSkills = pluginSkills.filter( (skill) => !skill.disabled && skill.name !== targetSkill.name, ); @@ -40,9 +50,14 @@ export async function removeInstalledSkill( } const skillKey = `${targetSkill.pluginName}:${targetSkill.name}`; - const result = targetSkill.pluginSkillsMode === 'allowlist' - ? isUser ? await removeUserEnabledSkill(skillKey) : await removeEnabledSkill(skillKey, workspacePath) - : isUser ? await addUserDisabledSkill(skillKey) : await addDisabledSkill(skillKey, workspacePath); + const result = + targetSkill.pluginSkillsMode === 'allowlist' + ? isUser + ? await removeUserEnabledSkill(skillKey) + : await removeEnabledSkill(skillKey, workspacePath) + : isUser + ? await addUserDisabledSkill(skillKey) + : await addDisabledSkill(skillKey, workspacePath); if (!result.success) { return { success: false, error: result.error ?? 'Unknown error' }; @@ -50,6 +65,9 @@ export async function removeInstalledSkill( return { success: true, - action: targetSkill.pluginSkillsMode === 'allowlist' ? 'removed-skill' : 'disabled-skill', + action: + targetSkill.pluginSkillsMode === 'allowlist' + ? 'removed-skill' + : 'disabled-skill', }; } diff --git a/src/cli/skill-update-reconciliation.ts b/src/cli/skill-update-reconciliation.ts new file mode 100644 index 00000000..a27b50de --- /dev/null +++ b/src/cli/skill-update-reconciliation.ts @@ -0,0 +1,406 @@ +import { randomUUID } from 'node:crypto'; +import { readFile, rename, stat, unlink, writeFile } from 'node:fs/promises'; +import { basename, dirname, join } from 'node:path'; +import { dump, load } from 'js-yaml'; +import { CONFIG_DIR, WORKSPACE_CONFIG_FILE } from '../constants.js'; +import type { + PreparedUnitReconciliation, + SkillUpdateInstallation, + SkillUpdateUnit, +} from '../core/skill-update.js'; +import { getUserWorkspaceConfigPath } from '../core/user-workspace.js'; +import { + pruneDisabledSkillsForPlugin, + pruneEnabledSkillsForPlugin, +} from '../core/workspace-modify.js'; +import { + type WorkspaceConfig, + WorkspaceConfigSchema, + getPluginSource, +} from '../models/workspace-config.js'; + +export interface CreateSkillUpdateReconcilerOptions { + workspacePath: string; + userConfigPath?: string; + /** Fault-injection seam used to prove multi-file rollback. */ + beforeReplace?: (path: string) => void | Promise; +} + +interface ConfigStage { + path: string; + original: string; + tempPath: string; +} + +function configPathForInstallation( + installation: SkillUpdateInstallation, + options: CreateSkillUpdateReconcilerOptions, +): string { + return installation.scope === 'project' + ? join(options.workspacePath, CONFIG_DIR, WORKSPACE_CONFIG_FILE) + : (options.userConfigPath ?? getUserWorkspaceConfigPath()); +} + +function parseConfig(content: string, path: string): WorkspaceConfig { + const raw = load(content); + const parsed = WorkspaceConfigSchema.safeParse(raw); + if (!parsed.success) { + throw new Error( + `Invalid workspace config at ${path}: ${parsed.error.issues + .map((issue) => issue.message) + .join('; ')}`, + ); + } + // Validate with the schema, but transform the raw object so Zod defaults and + // client shorthand normalization do not rewrite unrelated user config. + return raw as WorkspaceConfig; +} + +function removePluginEntry(config: WorkspaceConfig, index: number): void { + const entry = config.plugins[index]; + if (!entry) throw new Error(`Plugin entry at index ${index} not found`); + const source = getPluginSource(entry); + config.plugins.splice(index, 1); + pruneDisabledSkillsForPlugin(config, source); + pruneEnabledSkillsForPlugin(config, source); +} + +function selectorForImpact( + allowlist: string[], + impact: SkillUpdateUnit['deleted'][number], + allowBareWithQualified = false, +): string | undefined { + if (impact.selector && allowlist.includes(impact.selector)) { + return impact.selector; + } + if (allowlist.includes(impact.subpath)) return impact.subpath; + // Older bare-name configs did not retain a qualified selector. Only use the + // leaf fallback when there is no competing qualified selector in the entry. + if ( + allowlist.includes(impact.name) && + (allowBareWithQualified || + !allowlist.some( + (selector) => + selector !== impact.name && + selector.split('/').at(-1) === impact.name, + )) + ) { + return impact.name; + } + return undefined; +} + +function reconcileSelectorList( + selectors: string[], + deleted: SkillUpdateUnit['deleted'], + survivors: SkillUpdateUnit['survivors'], + prefix = '', +): string[] { + const localSelectors = prefix + ? selectors + .filter((selector) => selector.startsWith(prefix)) + .map((selector) => selector.slice(prefix.length)) + : selectors; + const replacements = new Map(); + + for (const impact of deleted) { + const selector = selectorForImpact( + localSelectors, + impact, + prefix.length > 0, + ); + if (!selector) continue; + + const survivingPaths = + selector === impact.name + ? survivors + .filter( + (survivor) => + survivor.name === impact.name && + survivor.subpath !== impact.subpath, + ) + .map((survivor) => `${prefix}${survivor.subpath}`) + : []; + replacements.set(`${prefix}${selector}`, survivingPaths); + } + + if (replacements.size === 0) return selectors; + + const replacementValues = new Set([...replacements.values()].flat()); + const emittedReplacementValues = new Set(); + const reconciled: string[] = []; + + for (const selector of selectors) { + const replacement = replacements.get(selector); + if (replacement) { + for (const survivingSelector of replacement) { + if (emittedReplacementValues.has(survivingSelector)) continue; + reconciled.push(survivingSelector); + emittedReplacementValues.add(survivingSelector); + } + continue; + } + if ( + replacementValues.has(selector) && + emittedReplacementValues.has(selector) + ) { + continue; + } + reconciled.push(selector); + if (replacementValues.has(selector)) { + emittedReplacementValues.add(selector); + } + } + + return reconciled; +} + +function reconcileLegacySelectors( + config: WorkspaceConfig, + installations: SkillUpdateInstallation[], + unit: SkillUpdateUnit, +): void { + if (config.version !== undefined && config.version >= 2) return; + + const installationIdsByPlugin = new Map>(); + for (const installation of installations) { + const ids = + installationIdsByPlugin.get(installation.pluginName) ?? new Set(); + ids.add(installation.id); + installationIdsByPlugin.set(installation.pluginName, ids); + } + + for (const [pluginName, installationIds] of installationIdsByPlugin) { + const deleted = unit.deleted.filter((impact) => + installationIds.has(impact.installationId), + ); + if (deleted.length === 0) continue; + const survivors = unit.survivors.filter((impact) => + installationIds.has(impact.installationId), + ); + const prefix = `${pluginName}:`; + + if (config.enabledSkills) { + config.enabledSkills = reconcileSelectorList( + config.enabledSkills, + deleted, + survivors, + prefix, + ); + if (config.enabledSkills.length === 0) config.enabledSkills = undefined; + } + if (config.disabledSkills) { + config.disabledSkills = reconcileSelectorList( + config.disabledSkills, + deleted, + survivors, + prefix, + ); + if (config.disabledSkills.length === 0) config.disabledSkills = undefined; + } + } +} + +function transformConfig( + original: string, + path: string, + installations: SkillUpdateInstallation[], + unit: SkillUpdateUnit, +): string { + const config = parseConfig(original, path); + const removedInstallationIds = new Set(unit.removedInstallationIds ?? []); + const survivorInstallationIds = new Set( + unit.survivors.map((impact) => impact.installationId), + ); + const deletedByInstallation = new Map(); + for (const impact of unit.deleted) { + const deleted = deletedByInstallation.get(impact.installationId) ?? []; + deleted.push(impact); + deletedByInstallation.set(impact.installationId, deleted); + } + const removeIndexes = new Set(); + + for (const installation of installations) { + const entry = config.plugins[installation.configIndex]; + if (!entry) { + throw new Error( + `Plugin entry ${installation.configIndex} for ${installation.rawSource} no longer exists in ${path}`, + ); + } + if (getPluginSource(entry) !== installation.rawSource) { + throw new Error( + `Plugin entry ${installation.configIndex} changed in ${path}; expected ${installation.rawSource}`, + ); + } + + if (removedInstallationIds.has(installation.id)) { + removeIndexes.add(installation.configIndex); + continue; + } + + const deleted = deletedByInstallation.get(installation.id) ?? []; + if (deleted.length === 0) continue; + if ( + installation.standaloneSkillSource && + !survivorInstallationIds.has(installation.id) + ) { + removeIndexes.add(installation.configIndex); + continue; + } + if (typeof entry === 'string') continue; + if (!Array.isArray(entry.skills)) { + // Implicit and blocklist installs are purged by the confirmed offline + // sync. Recording an exclusion for content that no longer exists would + // leave stale user configuration behind. + continue; + } + + for (const impact of deleted) { + if (!selectorForImpact(entry.skills, impact)) { + throw new Error( + `Configured selector for ${installation.pluginName}:${impact.subpath} no longer matches ${path}`, + ); + } + } + const survivors = unit.survivors.filter( + (impact) => impact.installationId === installation.id, + ); + entry.skills = reconcileSelectorList(entry.skills, deleted, survivors); + if (entry.skills.length === 0 && installation.standaloneSkillSource) { + removeIndexes.add(installation.configIndex); + } + } + + reconcileLegacySelectors(config, installations, unit); + + for (const index of [...removeIndexes].sort((left, right) => right - left)) { + removePluginEntry(config, index); + } + + const replacement = dump(config, { lineWidth: -1 }); + parseConfig(replacement, path); + return replacement; +} + +function tempPathFor(path: string, label: string): string { + return join( + dirname(path), + `.${basename(path)}.allagents-${label}-${randomUUID()}.tmp`, + ); +} + +async function writeTemp( + path: string, + content: string, + mode: number, +): Promise { + await writeFile(path, content, { + encoding: 'utf-8', + flag: 'wx', + mode, + }); +} + +async function cleanupTemp(path: string): Promise { + await unlink(path).catch((error: NodeJS.ErrnoException) => { + if (error.code !== 'ENOENT') throw error; + }); +} + +/** + * Build the config side of a physical skill-update transaction. Preparation + * validates and stages every affected file without changing live config. The + * returned commit replaces files atomically one at a time and restores every + * earlier replacement if a later one fails. + */ +export function createSkillUpdateReconciler( + options: CreateSkillUpdateReconcilerOptions, +): (unit: SkillUpdateUnit) => Promise { + return async (unit: SkillUpdateUnit) => { + const affectedInstallationIds = new Set([ + ...unit.deleted.map((impact) => impact.installationId), + ...(unit.removedInstallationIds ?? []), + ]); + const byPath = new Map(); + for (const installation of unit.installations) { + if (!affectedInstallationIds.has(installation.id)) continue; + const path = configPathForInstallation(installation, options); + const entries = byPath.get(path) ?? []; + entries.push(installation); + byPath.set(path, entries); + } + + const stages: ConfigStage[] = []; + try { + for (const [path, installations] of byPath) { + const original = await readFile(path, 'utf-8'); + const replacement = transformConfig( + original, + path, + installations, + unit, + ); + if (replacement === original) continue; + const tempPath = tempPathFor(path, 'next'); + const currentStat = await stat(path); + await writeTemp(tempPath, replacement, currentStat.mode); + stages.push({ path, original, tempPath }); + } + } catch (error) { + await Promise.all(stages.map((stage) => cleanupTemp(stage.tempPath))); + throw error; + } + + let state: 'prepared' | 'committed' | 'rolled-back' = 'prepared'; + const committed: ConfigStage[] = []; + + const restore = async (stage: ConfigStage): Promise => { + const current = await readFile(stage.path, 'utf-8'); + if (current === stage.original) return; + const restorePath = tempPathFor(stage.path, 'rollback'); + const currentStat = await stat(stage.path); + try { + await writeTemp(restorePath, stage.original, currentStat.mode); + await rename(restorePath, stage.path); + } finally { + await cleanupTemp(restorePath); + } + }; + + const rollback = async (): Promise => { + if (state === 'rolled-back') return; + for (const stage of [...committed].reverse()) await restore(stage); + await Promise.all(stages.map((stage) => cleanupTemp(stage.tempPath))); + state = 'rolled-back'; + }; + + return { + commit: async () => { + if (state !== 'prepared') { + throw new Error(`Config reconciliation is already ${state}`); + } + // Fail before the first rename if any config changed since preflight. + for (const stage of stages) { + if ((await readFile(stage.path, 'utf-8')) !== stage.original) { + await rollback(); + throw new Error( + `Workspace config changed during update: ${stage.path}`, + ); + } + } + try { + for (const stage of stages) { + await options.beforeReplace?.(stage.path); + await rename(stage.tempPath, stage.path); + committed.push(stage); + } + state = 'committed'; + } catch (error) { + await rollback(); + throw error; + } + }, + rollback, + }; + }; +} diff --git a/src/cli/skill-update.ts b/src/cli/skill-update.ts new file mode 100644 index 00000000..faaae95c --- /dev/null +++ b/src/cli/skill-update.ts @@ -0,0 +1,989 @@ +import { execFile } from 'node:child_process'; +import { existsSync } from 'node:fs'; +import { mkdtemp, readFile, rm } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import { basename, join, relative, resolve } from 'node:path'; +import { normalize, sep } from 'node:path'; +import { promisify } from 'node:util'; +import { CONFIG_DIR, WORKSPACE_CONFIG_FILE } from '../constants.js'; +import { + type MarketplaceEntry, + findMarketplace, + getMarketplacesDir, + parseLocation, + parsePluginSpec, +} from '../core/marketplace.js'; +import { getPluginName, resetFetchCache } from '../core/plugin.js'; +import { + type CheckoutNode, + type InstallationInspection, + type InstalledSkill, + type SkillUpdateDecision, + type SkillUpdateExecutionResult, + type SkillUpdateInstallation, + type SkillUpdateInventoryFailure, + type SkillUpdatePreflight, + type SkillUpdateScope, + type SkillUpdateUnitInput, + type UnitInspection, + buildSkillUpdatePreflight, + createGitHubSkillUpdateInstallation, + executeSkillUpdatePlan, + matchesSkillUpdateFilter, + resolveCheckoutSubpath, +} from '../core/skill-update.js'; +import { discoverSkillEntriesFromPluginRoot } from '../core/skills.js'; +import { syncUserWorkspace, syncWorkspace } from '../core/sync.js'; +import { getUserWorkspaceConfigPath } from '../core/user-workspace.js'; +import type { + PluginEntry, + PluginSkillsConfig, + WorkspaceConfig, +} from '../models/workspace-config.js'; +import { + WorkspaceConfigSchema, + getEffectivePluginSource, + getPluginSource, +} from '../models/workspace-config.js'; +import { parseMarketplaceManifest } from '../utils/marketplace-manifest-parser.js'; +import { + getPluginCachePath, + isGitHubUrl, + parseGitHubUrl, +} from '../utils/plugin-path.js'; +import { createSkillUpdateReconciler } from './skill-update-reconciliation.js'; + +const execFileAsync = promisify(execFile); +const SKILL_UPDATE_GIT_TIMEOUT_MS = 300_000; +const SKILL_UPDATE_GIT_CONFIG = [ + '-c', + 'filter.lfs.required=false', + '-c', + 'filter.lfs.smudge=', + '-c', + 'filter.lfs.clean=', + '-c', + 'filter.lfs.process=', +]; + +async function runSkillUpdateGit( + args: string[], + cwd?: string, +): Promise { + try { + const { stdout } = await execFileAsync( + 'git', + [...SKILL_UPDATE_GIT_CONFIG, ...args], + { + ...(cwd && { cwd }), + env: { + ...process.env, + GIT_LFS_SKIP_SMUDGE: '1', + GIT_TERMINAL_PROMPT: '0', + }, + maxBuffer: 4 * 1024 * 1024, + timeout: SKILL_UPDATE_GIT_TIMEOUT_MS, + }, + ); + return String(stdout).trim(); + } catch (error) { + const detail = + error && typeof error === 'object' && 'stderr' in error + ? String(error.stderr).trim() + : ''; + throw new Error( + `git ${args.join(' ')} failed${cwd ? ` in ${cwd}` : ''}${detail ? `: ${detail}` : ''}`, + { cause: error }, + ); + } +} + +function skillUpdateGitHubUrl(owner: string, repo: string): string { + return `https://github.com/${owner}/${repo}.git`; +} + +async function cloneSkillUpdateCheckout( + url: string, + ref?: string, +): Promise { + const directory = await mkdtemp(join(tmpdir(), 'allagents-skill-update-')); + try { + await runSkillUpdateGit([ + 'clone', + '--depth', + '1', + ...(ref ? ['--branch', ref] : []), + '--', + url, + directory, + ]); + return directory; + } catch (error) { + await rm(directory, { recursive: true, force: true }).catch(() => {}); + throw error; + } +} + +async function cleanupSkillUpdateCheckout(directory: string): Promise { + const normalizedDirectory = normalize(resolve(directory)); + const normalizedTemp = normalize(resolve(tmpdir())); + if ( + normalizedDirectory === normalizedTemp || + !normalizedDirectory.startsWith(`${normalizedTemp}${sep}`) + ) { + throw new Error( + 'Refusing to clean a skill update checkout outside the temp directory', + ); + } + await rm(directory, { recursive: true, force: true }); +} + +export interface SkillUpdateInventory { + installations: SkillUpdateInstallation[]; + skippedLocalSources: string[]; + failures: SkillUpdateInventoryFailure[]; +} + +export interface PrepareSkillUpdateOptions { + workspacePath: string; + scopes: SkillUpdateScope[]; + filters?: string[]; +} + +export interface PreparedSkillUpdate { + inventory: SkillUpdateInventory; + plan: SkillUpdatePreflight; +} + +/** Normalize the public scope flag after the caller has resolved its default. */ +export function normalizeSkillUpdateScopes( + scope: string | undefined, +): SkillUpdateScope[] { + switch (scope ?? 'project') { + case 'project': + return ['project']; + case 'user': + return ['user']; + case 'all': + return ['project', 'user']; + default: + throw new Error( + `Invalid scope '${scope}'. Expected project, user, or all.`, + ); + } +} + +/** --yes suppresses questions; it intentionally never authorizes deletion. */ +export function resolveNonInteractiveSkillUpdateDecisions( + plan: SkillUpdatePreflight, +): Record { + return Object.fromEntries( + plan.units + .filter((unit) => unit.deleted.length > 0) + .map((unit) => [unit.id, 'retain' as const]), + ); +} + +export function findUnmatchedSkillUpdateFilters( + inventory: SkillUpdateInventory, + scopes: SkillUpdateScope[], + filters: string[], +): string[] { + const selected = new Set(scopes); + return filters.filter((filter) => { + return !inventory.installations.some( + (installation) => + selected.has(installation.scope) && + matchesSkillUpdateFilter(installation, filter), + ); + }); +} + +function configPath(scope: SkillUpdateScope, workspacePath: string): string { + return scope === 'project' + ? join(workspacePath, CONFIG_DIR, WORKSPACE_CONFIG_FILE) + : getUserWorkspaceConfigPath(); +} + +async function readConfig( + scope: SkillUpdateScope, + workspacePath: string, +): Promise { + const path = configPath(scope, workspacePath); + if (!existsSync(path)) return null; + const { load } = await import('js-yaml'); + const raw = load(await readFile(path, 'utf-8')); + const parsed = WorkspaceConfigSchema.safeParse(raw); + if (!parsed.success) { + throw new Error( + `Invalid workspace config at ${path}: ${parsed.error.issues + .map((issue) => issue.message) + .join('; ')}`, + ); + } + return raw as WorkspaceConfig; +} + +async function revision(path: string): Promise { + if (!existsSync(path)) throw new Error(`Checkout not found: ${path}`); + const sha = await runSkillUpdateGit(['rev-parse', 'HEAD'], path); + if (!sha) throw new Error(`Could not resolve checkout revision: ${path}`); + return sha; +} + +function posixPath(path: string): string { + return path.split(/[\\/]/).join('/'); +} + +function enabledSkills( + entries: Awaited>, + pluginName: string, + config: WorkspaceConfig, + pluginSkills: PluginSkillsConfig | undefined, +): InstalledSkill[] { + const isV1 = config.version === undefined || config.version < 2; + const topDisabled = new Set(config.disabledSkills ?? []); + const topEnabled = config.enabledSkills + ? new Set(config.enabledSkills) + : undefined; + const hasTopEnabled = + isV1 && + topEnabled !== undefined && + [...topEnabled].some((value) => value.startsWith(`${pluginName}:`)); + + return entries.map(({ name, subpath }) => { + let enabled = true; + let selector: string | undefined; + if (Array.isArray(pluginSkills)) { + selector = pluginSkills.includes(subpath) + ? subpath + : pluginSkills.includes(name) + ? name + : undefined; + enabled = selector !== undefined; + } else if (pluginSkills) { + enabled = + !pluginSkills.exclude.includes(subpath) && + !pluginSkills.exclude.includes(name); + } else if (isV1) { + const nameKey = `${pluginName}:${name}`; + const pathKey = `${pluginName}:${subpath}`; + enabled = hasTopEnabled + ? (topEnabled?.has(nameKey) ?? false) || + (topEnabled?.has(pathKey) ?? false) + : !topDisabled.has(nameKey) && !topDisabled.has(pathKey); + } + return { + name, + subpath, + enabled, + ...(selector && { selector }), + }; + }); +} + +function pluginSkillsConfig( + plugin: PluginEntry, +): PluginSkillsConfig | undefined { + return typeof plugin === 'string' ? undefined : plugin.skills; +} + +async function isStandaloneSkillRoot( + root: string, + entries: Awaited>, +): Promise { + if (!existsSync(join(root, 'SKILL.md')) || entries.length !== 1) return false; + return ![ + '.claude-plugin', + '.github', + '.mcp.json', + 'agents', + 'commands', + 'hooks', + 'mcp.json', + ].some((artifact) => existsSync(join(root, artifact))); +} + +function nodeForMarketplace(entry: MarketplaceEntry): CheckoutNode | null { + if (entry.source.type === 'local') return null; + const managedPath = resolveCheckoutSubpath(getMarketplacesDir(), entry.name); + if (resolve(entry.path) !== resolve(managedPath)) { + throw new Error( + `Remote marketplace '${entry.name}' is outside the AllAgents-managed cache: ${entry.path}`, + ); + } + if (entry.source.type === 'github') { + const parsed = parseLocation(entry.source.location); + return { + id: managedPath, + cachePath: managedPath, + remoteUrl: skillUpdateGitHubUrl(parsed.owner, parsed.repo), + role: 'root', + currentSha: '', + ...(parsed.branch && { ref: parsed.branch }), + }; + } + return { + id: managedPath, + cachePath: managedPath, + remoteUrl: entry.source.location, + role: 'root', + currentSha: '', + }; +} + +async function marketplaceForScope( + name: string, + scope: SkillUpdateScope, + workspacePath: string, + sourceLocation?: string, +): Promise { + return findMarketplace( + name, + sourceLocation, + scope === 'project' ? workspacePath : undefined, + ); +} + +async function inventoryDirect( + scope: SkillUpdateScope, + configIndex: number, + plugin: PluginEntry, + config: WorkspaceConfig, +): Promise { + const effectiveSource = getEffectivePluginSource(plugin); + const parsed = parseGitHubUrl(effectiveSource); + if (!parsed) return null; + const cachePath = getPluginCachePath( + parsed.owner, + parsed.repo, + parsed.branch, + ); + const root = resolveCheckoutSubpath(cachePath, parsed.subpath ?? ''); + if (!existsSync(root)) { + throw new Error( + `Cached plugin root not found for ${effectiveSource}: ${root}`, + ); + } + const discovered = await discoverSkillEntriesFromPluginRoot(root); + const pluginName = getPluginName(root); + const skills = enabledSkills( + discovered, + pluginName, + config, + pluginSkillsConfig(plugin), + ); + if (!skills.some((skill) => skill.enabled)) return null; + return createGitHubSkillUpdateInstallation({ + scope, + configIndex, + plugin, + pluginName, + currentSha: await revision(cachePath), + skills, + standaloneSkillSource: await isStandaloneSkillRoot(root, discovered), + }); +} + +async function inventoryMarketplace( + scope: SkillUpdateScope, + configIndex: number, + plugin: PluginEntry, + config: WorkspaceConfig, + workspacePath: string, +): Promise { + const rawSource = getPluginSource(plugin); + const spec = parsePluginSpec(rawSource); + if (!spec) return null; + const marketplace = await marketplaceForScope( + spec.marketplaceName, + scope, + workspacePath, + spec.owner && spec.repo ? `${spec.owner}/${spec.repo}` : undefined, + ); + if (!marketplace) { + throw new Error( + `Marketplace '${spec.marketplaceName}' for ${rawSource} is not registered in ${scope} scope`, + ); + } + const marketplaceNode = nodeForMarketplace(marketplace); + if (!marketplaceNode) return 'local'; + marketplaceNode.currentSha = await revision(marketplaceNode.cachePath); + + const manifest = await parseMarketplaceManifest(marketplace.path); + if (!manifest.success) { + throw new Error( + `Could not inventory marketplace '${marketplace.name}': ${manifest.error}`, + ); + } + if (manifest.warnings.length > 0) { + throw new Error( + `Could not safely inventory marketplace '${marketplace.name}': ${manifest.warnings.join('; ')}`, + ); + } + const manifestPlugin = manifest.data.plugins.find( + (entry) => entry.name === spec.plugin, + ); + + let root: string; + let rootNode: CheckoutNode = marketplaceNode; + const nodes: CheckoutNode[] = [marketplaceNode]; + if (manifestPlugin && typeof manifestPlugin.source === 'object') { + const parsed = parseGitHubUrl(manifestPlugin.source.url); + if (!parsed) { + throw new Error( + `External marketplace plugin '${rawSource}' is not backed by a supported GitHub source`, + ); + } + const cachePath = getPluginCachePath( + parsed.owner, + parsed.repo, + parsed.branch, + ); + rootNode = { + id: cachePath, + cachePath, + remoteUrl: skillUpdateGitHubUrl(parsed.owner, parsed.repo), + role: 'dependency', + currentSha: await revision(cachePath), + ...(parsed.branch && { ref: parsed.branch }), + }; + nodes.unshift(rootNode); + root = resolveCheckoutSubpath(cachePath, parsed.subpath ?? ''); + } else { + const source = + manifestPlugin && typeof manifestPlugin.source === 'string' + ? manifestPlugin.source + : join(spec.subpath ?? 'plugins', spec.plugin); + root = resolveCheckoutSubpath(marketplaceNode.cachePath, source); + } + if (!existsSync(root)) { + throw new Error(`Installed marketplace plugin root not found: ${root}`); + } + const discovered = await discoverSkillEntriesFromPluginRoot(root); + const skills = enabledSkills( + discovered, + spec.plugin, + config, + pluginSkillsConfig(plugin), + ); + if (!skills.some((skill) => skill.enabled)) return null; + + return { + id: `${scope}:${configIndex}`, + scope, + configIndex, + rawSource, + effectiveSource: rawSource, + pluginName: spec.plugin, + rootNodeId: rootNode.id, + rootSubpath: + rootNode.id === marketplaceNode.id + ? posixPath(relative(marketplace.path, root)) + : posixPath(relative(rootNode.cachePath, root)), + nodes, + skills, + marketplace: { + nodeId: marketplaceNode.id, + pluginName: spec.plugin, + }, + }; +} + +/** Snapshot raw config entries across both scopes before any remote checkout. */ +export async function buildSkillUpdateInventory( + workspacePath: string, + selectedScopes: SkillUpdateScope[] = ['project', 'user'], +): Promise { + const installations: SkillUpdateInstallation[] = []; + const skippedLocalSources: string[] = []; + const failures: SkillUpdateInventoryFailure[] = []; + const deferred: SkillUpdateInstallation[] = []; + const deferredErrors: Array< + SkillUpdateInventoryFailure & { errorCause: unknown } + > = []; + const selected = new Set(selectedScopes); + + const potentialNodeIds = async ( + source: string, + plugin: PluginEntry, + scope: SkillUpdateScope, + ): Promise => { + const direct = parseGitHubUrl(getEffectivePluginSource(plugin)); + if (direct) { + return [getPluginCachePath(direct.owner, direct.repo, direct.branch)]; + } + const spec = parsePluginSpec(source); + if (!spec) return []; + const marketplace = await marketplaceForScope( + spec.marketplaceName, + scope, + workspacePath, + spec.owner && spec.repo ? `${spec.owner}/${spec.repo}` : undefined, + ); + const node = marketplace ? nodeForMarketplace(marketplace) : null; + return node ? [node.id] : []; + }; + + // Inventory selected scopes first, then attach healthy consumers from the + // other scope only when their physical checkout graph intersects. This keeps + // an unrelated broken user plugin from blocking a project-only update while + // still making shared caches a cross-scope safety boundary. + for (const scope of ['project', 'user'] as const) { + let config: WorkspaceConfig | null; + try { + config = await readConfig(scope, workspacePath); + } catch (error) { + if (selected.has(scope)) { + failures.push({ + id: `inventory:${scope}:config`, + scope, + source: configPath(scope, workspacePath), + nodeIds: [], + error: error instanceof Error ? error.message : String(error), + }); + } + continue; + } + if (!config) continue; + for (const [configIndex, plugin] of config.plugins.entries()) { + const rawSource = getPluginSource(plugin); + const effectiveSource = getEffectivePluginSource(plugin); + let installation: SkillUpdateInstallation | null | 'local'; + try { + // Inline Git refs also use `@` (owner/repo@ref). Direct GitHub + // recognition must therefore take precedence over marketplace syntax. + if (isGitHubUrl(effectiveSource)) { + installation = await inventoryDirect( + scope, + configIndex, + plugin, + config, + ); + } else if (parsePluginSpec(rawSource)) { + installation = await inventoryMarketplace( + scope, + configIndex, + plugin, + config, + workspacePath, + ); + } else { + installation = 'local'; + } + } catch (error) { + const failure = { + id: `inventory:${scope}:${configIndex}`, + scope, + source: rawSource, + nodeIds: [] as string[], + error: error instanceof Error ? error.message : String(error), + }; + try { + failure.nodeIds = await potentialNodeIds(rawSource, plugin, scope); + } catch { + // The original inventory error remains authoritative. An unknown + // physical identity becomes a standalone selected-scope failure. + } + if (selected.has(scope)) failures.push(failure); + else deferredErrors.push({ ...failure, errorCause: error }); + continue; + } + if (installation === 'local') { + if (selected.has(scope)) { + skippedLocalSources.push(`${scope}:${rawSource}`); + } + } else if (installation) { + (selected.has(scope) ? installations : deferred).push(installation); + } + } + } + + const touchedNodeIds = new Set( + installations.flatMap((installation) => + installation.nodes.map((node) => node.id), + ), + ); + let changed = true; + while (changed) { + changed = false; + for (let index = deferred.length - 1; index >= 0; index--) { + const installation = deferred[index]; + if (!installation?.nodes.some((node) => touchedNodeIds.has(node.id))) { + continue; + } + deferred.splice(index, 1); + installations.push(installation); + for (const node of installation.nodes) touchedNodeIds.add(node.id); + changed = true; + } + } + const sharedFailures = deferredErrors.filter((candidate) => + candidate.nodeIds.some((nodeId) => touchedNodeIds.has(nodeId)), + ); + for (const sharedFailure of sharedFailures) { + failures.push({ + id: sharedFailure.id, + scope: sharedFailure.scope, + source: sharedFailure.source, + nodeIds: sharedFailure.nodeIds, + error: `Could not safely inventory shared source: ${sharedFailure.errorCause instanceof Error ? sharedFailure.errorCause.message : String(sharedFailure.errorCause)}`, + }); + } + return { installations, skippedLocalSources, failures }; +} + +async function inspectInstallation( + installation: SkillUpdateInstallation, + checkoutPaths: Map, +): Promise { + let root: string; + if (installation.marketplace) { + const marketplaceCheckout = checkoutPaths.get( + installation.marketplace.nodeId, + ); + if (!marketplaceCheckout) { + throw new Error( + `Inspected marketplace checkout missing for ${installation.rawSource}`, + ); + } + const manifest = await parseMarketplaceManifest(marketplaceCheckout); + if (!manifest.success) { + return { + installationId: installation.id, + outcome: 'failed', + error: manifest.error, + }; + } + if (manifest.warnings.length > 0) { + return { + installationId: installation.id, + outcome: 'failed', + error: manifest.warnings.join('; '), + }; + } + const entry = manifest.data.plugins.find( + (candidate) => candidate.name === installation.marketplace?.pluginName, + ); + if (!entry) { + return { installationId: installation.id, outcome: 'plugin-removed' }; + } + if (typeof entry.source === 'object') { + const parsed = parseGitHubUrl(entry.source.url); + if (!parsed) { + return { + installationId: installation.id, + outcome: 'failed', + error: `Unsupported external source for ${installation.rawSource}`, + }; + } + const expectedNode = getPluginCachePath( + parsed.owner, + parsed.repo, + parsed.branch, + ); + if (expectedNode !== installation.rootNodeId) { + return { + installationId: installation.id, + outcome: 'failed', + error: `External source for ${installation.rawSource} changed; run plugin update before skill update`, + }; + } + const checkout = checkoutPaths.get(expectedNode); + if (!checkout) { + throw new Error( + `Inspected external checkout missing for ${expectedNode}`, + ); + } + root = resolveCheckoutSubpath(checkout, parsed.subpath ?? ''); + } else { + root = resolveCheckoutSubpath(marketplaceCheckout, entry.source); + } + } else { + const checkout = checkoutPaths.get(installation.rootNodeId); + if (!checkout) { + throw new Error( + `Inspected checkout missing for ${installation.rawSource}`, + ); + } + root = resolveCheckoutSubpath(checkout, installation.rootSubpath); + } + + if (!existsSync(root)) { + return { + installationId: installation.id, + outcome: 'failed', + error: `Declared plugin root no longer exists for ${installation.rawSource}`, + }; + } + const warnings: string[] = []; + const discovered = await discoverSkillEntriesFromPluginRoot(root, warnings); + if (warnings.length > 0) { + return { + installationId: installation.id, + outcome: 'failed', + error: warnings.join('; '), + }; + } + return { + installationId: installation.id, + outcome: 'resolved', + skills: discovered.map(({ name, subpath }) => ({ name, subpath })), + }; +} + +/** Inspect direct, embedded-marketplace, and external-marketplace units in temp clones. */ +export async function inspectSkillUpdateUnit( + unit: SkillUpdateUnitInput, +): Promise { + const checkoutPaths = new Map(); + try { + const nodes: UnitInspection['nodes'] = []; + const cloneNode = async (node: CheckoutNode): Promise => { + if (checkoutPaths.has(node.id)) return; + const checkout = await cloneSkillUpdateCheckout(node.remoteUrl, node.ref); + checkoutPaths.set(node.id, checkout); + nodes.push({ nodeId: node.id, sha: await revision(checkout) }); + }; + + // Marketplace roots authoritatively determine whether an external plugin + // still exists. Inspect them before touching dependencies that may now be + // obsolete or unreachable. + for (const node of unit.nodes.filter( + (candidate) => candidate.role === 'root', + )) { + await cloneNode(node); + } + const installations: InstallationInspection[] = []; + const removed = new Set(); + for (const installation of unit.installations.filter( + (candidate) => candidate.marketplace, + )) { + const marketplaceCheckout = checkoutPaths.get( + installation.marketplace?.nodeId ?? '', + ); + if (!marketplaceCheckout) continue; + const manifest = await parseMarketplaceManifest(marketplaceCheckout); + if (!manifest.success || manifest.warnings.length > 0) continue; + const entry = manifest.data.plugins.find( + (candidate) => candidate.name === installation.marketplace?.pluginName, + ); + if (!entry) { + removed.add(installation.id); + installations.push({ + installationId: installation.id, + outcome: 'plugin-removed', + }); + } + } + + const neededNodeIds = new Set( + unit.installations + .filter((installation) => !removed.has(installation.id)) + .flatMap((installation) => installation.nodes.map((node) => node.id)), + ); + for (const node of unit.nodes.filter( + (candidate) => + candidate.role === 'dependency' && neededNodeIds.has(candidate.id), + )) { + await cloneNode(node); + } + for (const installation of unit.installations) { + if (removed.has(installation.id)) continue; + installations.push( + await inspectInstallation(installation, checkoutPaths), + ); + } + const failed = installations.find((entry) => entry.outcome === 'failed'); + return failed?.outcome === 'failed' + ? { + outcome: 'failed', + nodes, + installations, + error: failed.error, + } + : { outcome: 'resolved', nodes, installations }; + } catch (error) { + return { + outcome: 'failed', + nodes: [], + installations: [], + error: error instanceof Error ? error.message : String(error), + }; + } finally { + await Promise.all( + [...checkoutPaths.values()].map((path) => + cleanupSkillUpdateCheckout(path).catch(() => {}), + ), + ); + } +} + +export async function prepareSkillUpdate( + options: PrepareSkillUpdateOptions, +): Promise { + const inventory = await buildSkillUpdateInventory( + options.workspacePath, + options.scopes, + ); + const plan = await buildSkillUpdatePreflight( + { + installations: inventory.installations, + selectedScopes: options.scopes, + failures: inventory.failures, + ...(options.filters && { filters: options.filters }), + }, + { inspectUnit: inspectSkillUpdateUnit }, + ); + return { inventory, plan }; +} + +function normalizeRemoteUrl(url: string): string { + const parsed = parseGitHubUrl(url); + if (parsed && !url.startsWith('/') && !url.startsWith('file:')) { + return `github:${parsed.owner.toLocaleLowerCase()}/${parsed.repo.toLocaleLowerCase()}`; + } + return url.replace(/[\\/]$/, '').replace(/\.git$/, ''); +} + +async function assertExpectedOrigin(node: CheckoutNode): Promise { + // `git remote get-url` applies url..insteadOf rewriting. Read the + // configured value so an isolated/local transport override cannot make a + // correctly configured GitHub checkout fail the origin safety check. + const origin = await runSkillUpdateGit( + ['config', '--get', 'remote.origin.url'], + node.cachePath, + ); + if (normalizeRemoteUrl(origin) !== normalizeRemoteUrl(node.remoteUrl)) { + throw new Error( + `Refusing to update ${node.cachePath}: origin '${origin}' does not match expected remote '${node.remoteUrl}'`, + ); + } +} + +export async function moveSkillUpdateCheckout( + node: CheckoutNode, + sha: string, +): Promise { + await assertExpectedOrigin(node); + + try { + await runSkillUpdateGit( + ['fetch', '--depth', '1', 'origin', node.ref ?? 'HEAD'], + node.cachePath, + ); + } catch (refError) { + try { + await runSkillUpdateGit( + ['fetch', '--depth', '1', 'origin', sha], + node.cachePath, + ); + } catch (shaError) { + throw new Error( + `Could not fetch ref '${node.ref ?? 'HEAD'}' or inspected revision '${sha}': ${refError instanceof Error ? refError.message : String(refError)}; ${shaError instanceof Error ? shaError.message : String(shaError)}`, + ); + } + } + try { + await runSkillUpdateGit( + ['cat-file', '-e', `${sha}^{commit}`], + node.cachePath, + ); + } catch { + // Some servers do not include the exact preflight commit in the shallow + // ref fetch. Try the immutable SHA before declaring the transaction failed. + await runSkillUpdateGit( + ['fetch', '--depth', '1', 'origin', sha], + node.cachePath, + ); + } + await runSkillUpdateGit(['reset', '--hard', sha], node.cachePath); +} + +async function restoreCheckout(node: CheckoutNode, sha: string): Promise { + await assertExpectedOrigin(node); + await runSkillUpdateGit( + ['cat-file', '-e', `${sha}^{commit}`], + node.cachePath, + ); + await runSkillUpdateGit(['reset', '--hard', sha], node.cachePath); +} + +export async function executePreparedSkillUpdate( + prepared: PreparedSkillUpdate, + decisions: Record, + workspacePath: string, +): Promise { + const result = await executeSkillUpdatePlan(prepared.plan, decisions, { + advanceNode: moveSkillUpdateCheckout, + restoreNode: restoreCheckout, + reconcileUnit: createSkillUpdateReconciler({ workspacePath }), + syncScope: async (scope) => { + resetFetchCache(); + const syncResult = + scope === 'project' + ? await syncWorkspace(workspacePath, { offline: true }) + : await syncUserWorkspace({ offline: true }); + return syncResult.success + ? { success: true } + : { success: false, error: `Offline ${scope} sync failed` }; + }, + }); + return result; +} + +export function hasProjectSkillConfig(workspacePath: string): boolean { + return existsSync(join(workspacePath, CONFIG_DIR, WORKSPACE_CONFIG_FILE)); +} + +export function unitDisplayName( + unit: SkillUpdatePreflight['units'][number], +): string { + const sources = [ + ...new Set(unit.installations.map((entry) => entry.rawSource)), + ]; + return sources.length > 0 ? sources.join(', ') : basename(unit.id); +} + +export interface SkillUpdateSummary { + updated: number; + removed: number; + retained: number; + skipped: number; + failed: number; + cancelled: number; +} + +export function skillUpdateSummary( + result: SkillUpdateExecutionResult, +): SkillUpdateSummary { + const summary: SkillUpdateSummary = { + updated: 0, + removed: 0, + retained: 0, + skipped: 0, + failed: 0, + cancelled: 0, + }; + for (const unit of result.units) { + summary.updated += unit.skillCounts.updated; + summary.removed += unit.skillCounts.removed; + summary.retained += unit.skillCounts.retained; + if (unit.status === 'retained' || unit.status === 'skipped') { + summary.skipped += 1; + } else if (unit.status === 'failed') { + summary.failed += 1; + } else if (unit.status === 'cancelled') { + summary.cancelled += 1; + } + } + return summary; +} + +/** Map an orchestration result to the documented process exit contract. */ +export function skillUpdateExitCode( + result: SkillUpdateExecutionResult, +): number { + if (result.cancelled) return 0; + return result.success ? 0 : 1; +} diff --git a/src/cli/terminal-output.ts b/src/cli/terminal-output.ts new file mode 100644 index 00000000..054e23df --- /dev/null +++ b/src/cli/terminal-output.ts @@ -0,0 +1,106 @@ +/** + * Render repository- and user-controlled text safely in an interactive terminal. + * + * ANSI/OSC sequences can alter earlier output, set terminal titles, or forge + * prompt/status lines. C0/C1 and bidirectional formatting controls can do the + * same without being visible. Remove complete terminal escape sequences, then + * replace any remaining control characters with a visible replacement marker. + * JSON output intentionally does not use this helper so automation receives + * the original value. + */ +const ESC = 0x1b; +const CSI = 0x9b; +const OSC = 0x9d; +const STRING_TERMINATOR = 0x9c; +const REPLACEMENT = '\uFFFD'; + +function consumeControlString( + value: string, + start: number, + allowBell: boolean, +): number { + for (let index = start; index < value.length; index += 1) { + const code = value.charCodeAt(index); + if ((allowBell && code === 0x07) || code === STRING_TERMINATOR) { + return index + 1; + } + if (code === ESC && value.charCodeAt(index + 1) === 0x5c) { + return index + 2; + } + } + return value.length; +} + +function consumeCsi(value: string, start: number): number { + for (let index = start; index < value.length; index += 1) { + const code = value.charCodeAt(index); + if (code >= 0x40 && code <= 0x7e) return index + 1; + } + return value.length; +} + +function consumeEscape(value: string, start: number): number { + let index = start; + while (index < value.length) { + const code = value.charCodeAt(index); + index += 1; + if (code < 0x20 || code > 0x2f) break; + } + return index; +} + +function isDirectionControl(code: number): boolean { + return ( + code === 0x061c || + code === 0x200e || + code === 0x200f || + (code >= 0x202a && code <= 0x202e) || + (code >= 0x2066 && code <= 0x2069) + ); +} + +export function terminalSafe(value: unknown): string { + const text = String(value); + let safe = ''; + + for (let index = 0; index < text.length; ) { + const code = text.charCodeAt(index); + if (code === ESC) { + const next = text.charCodeAt(index + 1); + if (next === 0x5d) { + index = consumeControlString(text, index + 2, true); + } else if ([0x50, 0x58, 0x5e, 0x5f].includes(next)) { + index = consumeControlString(text, index + 2, false); + } else if (next === 0x5b) { + index = consumeCsi(text, index + 2); + } else { + index = consumeEscape(text, index + 1); + } + continue; + } + if (code === OSC) { + index = consumeControlString(text, index + 1, true); + continue; + } + if ([0x90, 0x98, 0x9e, 0x9f].includes(code)) { + index = consumeControlString(text, index + 1, false); + continue; + } + if (code === CSI) { + index = consumeCsi(text, index + 1); + continue; + } + if ( + code <= 0x1f || + (code >= 0x7f && code <= 0x9f) || + isDirectionControl(code) + ) { + safe += REPLACEMENT; + } else { + safe += text[index]; + } + index += 1; + } + + return safe; +} diff --git a/src/core/skill-update.ts b/src/core/skill-update.ts new file mode 100644 index 00000000..1c6bd845 --- /dev/null +++ b/src/core/skill-update.ts @@ -0,0 +1,745 @@ +import { existsSync, realpathSync } from 'node:fs'; +import { isAbsolute, relative, resolve, sep } from 'node:path'; +import { + type PluginEntry, + getEffectivePluginSource, + getPluginSource, +} from '../models/workspace-config.js'; +import { getPluginCachePath, parseGitHubUrl } from '../utils/plugin-path.js'; +import { cleanupTempDir, cloneToTemp, gitHubUrl } from './git.js'; +import { discoverSkillEntriesFromPluginRoot } from './skills.js'; + +export type SkillUpdateScope = 'project' | 'user'; +export type CheckoutNodeRole = 'dependency' | 'root'; + +export interface CheckoutNode { + /** Stable physical identity. Cache paths are preferred because aliases share them. */ + id: string; + cachePath: string; + remoteUrl: string; + ref?: string; + role: CheckoutNodeRole; + /** Revision present before update, used for transaction rollback. */ + currentSha: string; +} + +export interface InstalledSkill { + name: string; + subpath: string; + /** Exact config selector, retained to disambiguate duplicate leaf names. */ + selector?: string; + enabled: boolean; +} + +export interface SkillUpdateInstallation { + id: string; + scope: SkillUpdateScope; + configIndex: number; + rawSource: string; + effectiveSource: string; + pluginName: string; + rootNodeId: string; + rootSubpath: string; + nodes: CheckoutNode[]; + skills: InstalledSkill[]; + /** Marketplace lookup metadata used to resolve the new plugin root from the inspected manifest. */ + marketplace?: { + nodeId: string; + pluginName: string; + }; + /** True only when inventory proved this config entry owns skills and nothing else. */ + standaloneSkillSource?: boolean; +} + +export interface DiscoveredSkill { + name: string; + subpath: string; +} + +export type InstallationInspection = + | { + installationId: string; + outcome: 'resolved'; + skills: DiscoveredSkill[]; + } + | { + installationId: string; + outcome: 'plugin-removed'; + skills?: DiscoveredSkill[]; + } + | { + installationId: string; + outcome: 'local'; + skills?: DiscoveredSkill[]; + } + | { + installationId: string; + outcome: 'failed'; + skills?: DiscoveredSkill[]; + error: string; + }; + +export interface InspectedNodeRevision { + nodeId: string; + sha: string; +} + +export type UnitInspection = { + outcome: 'resolved' | 'plugin-removed' | 'local' | 'failed'; + nodes: InspectedNodeRevision[]; + installations: InstallationInspection[]; + error?: string; +}; + +export interface SkillUpdateUnitInput { + id: string; + nodes: CheckoutNode[]; + installations: SkillUpdateInstallation[]; +} + +export interface SkillUpdateSkillImpact extends InstalledSkill { + installationId: string; + scope: SkillUpdateScope; + pluginName: string; + source: string; +} + +export interface SkillUpdateUnit extends SkillUpdateUnitInput { + outcome: UnitInspection['outcome']; + inspectedNodes: InspectedNodeRevision[]; + deleted: SkillUpdateSkillImpact[]; + survivors: SkillUpdateSkillImpact[]; + /** Installations whose marketplace entry was authoritatively removed upstream. */ + removedInstallationIds: string[]; + blockedByOutOfScope: boolean; + error?: string; +} + +export interface SkillUpdatePreflight { + selectedScopes: SkillUpdateScope[]; + units: SkillUpdateUnit[]; +} + +export interface BuildSkillUpdatePreflightInput { + installations: SkillUpdateInstallation[]; + selectedScopes: SkillUpdateScope[]; + filters?: string[]; + failures?: SkillUpdateInventoryFailure[]; +} + +export interface SkillUpdateInventoryFailure { + id: string; + scope: SkillUpdateScope; + source: string; + nodeIds: string[]; + error: string; +} + +export interface BuildSkillUpdatePreflightDeps { + inspectUnit: (unit: SkillUpdateUnitInput) => Promise; +} + +export interface CreateGitHubSkillUpdateInstallationInput { + scope: SkillUpdateScope; + configIndex: number; + plugin: PluginEntry; + pluginName: string; + currentSha: string; + skills: InstalledSkill[]; + standaloneSkillSource?: boolean; +} + +export interface InspectRemoteSkillUpdateDeps { + cloneNode?: (node: CheckoutNode) => Promise; + getRevision?: (checkoutPath: string) => Promise; + pathExists?: (path: string) => boolean; + discoverPluginSkills?: (pluginRoot: string) => Promise; + cleanup?: (checkoutPath: string) => Promise; +} + +function pathIsWithin(base: string, candidate: string): boolean { + const pathFromBase = relative(base, candidate); + return ( + pathFromBase === '' || + (pathFromBase !== '..' && + !pathFromBase.startsWith(`..${sep}`) && + !isAbsolute(pathFromBase)) + ); +} + +/** Resolve an untrusted repository subpath without allowing checkout escapes. */ +export function resolveCheckoutSubpath( + checkoutPath: string, + subpath: string, +): string { + if (!subpath) return checkoutPath; + if (isAbsolute(subpath)) { + throw new Error(`Plugin subpath '${subpath}' must not be absolute`); + } + + const lexicalBase = resolve(checkoutPath); + const candidate = resolve(lexicalBase, subpath); + if (!pathIsWithin(lexicalBase, candidate)) { + throw new Error( + `Plugin subpath '${subpath}' resolves outside its checkout`, + ); + } + + // A lexically contained path can still escape through a symlink. Check the + // resolved target whenever it exists; missing roots are classified later. + if (existsSync(candidate)) { + const realBase = realpathSync(lexicalBase); + const realCandidate = realpathSync(candidate); + if (!pathIsWithin(realBase, realCandidate)) { + throw new Error( + `Plugin subpath '${subpath}' resolves outside its checkout`, + ); + } + } + return candidate; +} + +export type SkillUpdateDecision = 'remove' | 'retain' | 'cancel'; + +export interface PreparedUnitReconciliation { + commit: () => Promise; + rollback: () => Promise; +} + +export interface ExecuteSkillUpdateDeps { + advanceNode: (node: CheckoutNode, sha: string) => Promise; + restoreNode: (node: CheckoutNode, sha: string) => Promise; + reconcileUnit: (unit: SkillUpdateUnit) => Promise; + syncScope: ( + scope: SkillUpdateScope, + options: { offline: true }, + ) => Promise<{ success: boolean; error?: string }>; +} + +export type SkillUpdateExecutionStatus = + | 'updated' + | 'removed' + | 'retained' + | 'skipped' + | 'failed' + | 'cancelled'; + +export interface SkillUpdateSkillCounts { + updated: number; + removed: number; + retained: number; +} + +export interface SkillUpdateUnitExecution { + id: string; + status: SkillUpdateExecutionStatus; + /** Per-skill impacts; status remains the physical refresh-unit outcome. */ + skillCounts: SkillUpdateSkillCounts; + error?: string; +} + +export interface SkillUpdateExecutionResult { + success: boolean; + cancelled: boolean; + units: SkillUpdateUnitExecution[]; + syncedScopes: SkillUpdateScope[]; +} + +/** + * Convert a direct GitHub config entry plus its pre-update skill snapshot into + * the canonical physical-cache inventory consumed by preflight. + */ +export function createGitHubSkillUpdateInstallation( + input: CreateGitHubSkillUpdateInstallationInput, +): SkillUpdateInstallation | null { + const rawSource = getPluginSource(input.plugin); + const effectiveSource = getEffectivePluginSource(input.plugin); + const parsed = parseGitHubUrl(effectiveSource); + if (!parsed) return null; + + const cachePath = getPluginCachePath( + parsed.owner, + parsed.repo, + parsed.branch, + ); + const node: CheckoutNode = { + id: cachePath, + cachePath, + remoteUrl: gitHubUrl(parsed.owner, parsed.repo), + role: 'root', + currentSha: input.currentSha, + ...(parsed.branch && { ref: parsed.branch }), + }; + + return { + id: `${input.scope}:${input.configIndex}`, + scope: input.scope, + configIndex: input.configIndex, + rawSource, + effectiveSource, + pluginName: input.pluginName, + rootNodeId: node.id, + rootSubpath: parsed.subpath ?? '', + nodes: [node], + skills: input.skills, + ...(input.standaloneSkillSource && { standaloneSkillSource: true }), + }; +} + +function uniqueById(values: T[]): T[] { + return [...new Map(values.map((value) => [value.id, value])).values()]; +} + +/** + * Build connected components of installations joined by any checkout node. + * This makes the physical cache graph—not config spelling—the decision boundary. + */ +export function buildPhysicalRefreshUnits( + installations: SkillUpdateInstallation[], +): SkillUpdateUnitInput[] { + const remaining = new Set(installations.map((entry) => entry.id)); + const byId = new Map(installations.map((entry) => [entry.id, entry])); + const nodeConsumers = new Map>(); + + for (const entry of installations) { + for (const node of entry.nodes) { + const consumers = nodeConsumers.get(node.id) ?? new Set(); + consumers.add(entry.id); + nodeConsumers.set(node.id, consumers); + } + } + + const units: SkillUpdateUnitInput[] = []; + while (remaining.size > 0) { + const first = remaining.values().next().value as string; + const queue = [first]; + let cursor = 0; + const component: SkillUpdateInstallation[] = []; + + while (cursor < queue.length) { + const id = queue[cursor++]; + if (!id || !remaining.delete(id)) continue; + const entry = byId.get(id); + if (!entry) continue; + component.push(entry); + for (const node of entry.nodes) { + for (const consumer of nodeConsumers.get(node.id) ?? []) { + if (remaining.has(consumer)) queue.push(consumer); + } + } + } + + const nodes = uniqueById(component.flatMap((entry) => entry.nodes)); + const id = nodes + .map((node) => node.id) + .sort((a, b) => a.localeCompare(b)) + .join('::'); + units.push({ id, nodes, installations: component }); + } + + return units; +} + +/** + * Inspect a direct or already-resolved multi-checkout unit in disposable clones. + * Missing declared roots and unreadable discovery are typed failures, whereas an + * existing root containing no skills is an authoritative empty result. + */ +export async function inspectRemoteSkillUpdateUnit( + unit: SkillUpdateUnitInput, + deps: InspectRemoteSkillUpdateDeps = {}, +): Promise { + const cloneNode = + deps.cloneNode ?? + (async (node: CheckoutNode) => { + const url = + node.remoteUrl || + (() => { + throw new Error(`Missing remote URL for ${node.id}`); + })(); + return cloneToTemp(url, node.ref); + }); + const getRevision = + deps.getRevision ?? + (async (checkoutPath: string) => { + const { default: simpleGit } = await import('simple-git'); + const sha = (await simpleGit(checkoutPath).revparse(['HEAD'])).trim(); + if (!sha) + throw new Error( + `Could not resolve inspected revision for ${checkoutPath}`, + ); + return sha; + }); + const pathExists = deps.pathExists ?? existsSync; + const discoverPluginSkills = + deps.discoverPluginSkills ?? + (async (pluginRoot: string) => { + const warnings: string[] = []; + const skills = await discoverSkillEntriesFromPluginRoot( + pluginRoot, + warnings, + ); + if (warnings.length > 0) throw new Error(warnings.join('\n')); + return skills.map(({ name, subpath }) => ({ name, subpath })); + }); + const cleanup = deps.cleanup ?? cleanupTempDir; + const checkoutPaths = new Map(); + + try { + const nodes: InspectedNodeRevision[] = []; + for (const node of unit.nodes) { + const checkoutPath = await cloneNode(node); + checkoutPaths.set(node.id, checkoutPath); + nodes.push({ nodeId: node.id, sha: await getRevision(checkoutPath) }); + } + + const installations: InstallationInspection[] = []; + for (const installation of unit.installations) { + const checkoutPath = checkoutPaths.get(installation.rootNodeId); + if (!checkoutPath) { + throw new Error( + `Inspected checkout missing for ${installation.rootNodeId}`, + ); + } + const pluginRoot = resolveCheckoutSubpath( + checkoutPath, + installation.rootSubpath, + ); + if (!pathExists(pluginRoot)) { + throw new Error( + `Configured plugin declared root '${installation.rootSubpath || '.'}' no longer exists in ${installation.effectiveSource}`, + ); + } + installations.push({ + installationId: installation.id, + outcome: 'resolved', + skills: await discoverPluginSkills(pluginRoot), + }); + } + + return { outcome: 'resolved', nodes, installations }; + } catch (error) { + return { + outcome: 'failed', + nodes: [], + installations: [], + error: error instanceof Error ? error.message : String(error), + }; + } finally { + await Promise.all( + [...checkoutPaths.values()].map((checkoutPath) => + cleanup(checkoutPath).catch(() => {}), + ), + ); + } +} + +export function matchesSkillUpdateFilter( + installation: SkillUpdateInstallation, + filter: string, +): boolean { + const expected = filter.toLocaleLowerCase(); + return installation.skills.some( + (skill) => + skill.enabled && + (skill.name.toLocaleLowerCase() === expected || + skill.subpath.toLocaleLowerCase() === expected || + `${installation.pluginName}:${skill.subpath}`.toLocaleLowerCase() === + expected), + ); +} + +function impact( + installation: SkillUpdateInstallation, + skill: InstalledSkill, +): SkillUpdateSkillImpact { + return { + ...skill, + installationId: installation.id, + scope: installation.scope, + pluginName: installation.pluginName, + source: installation.rawSource, + }; +} + +/** + * Inspect every selected physical unit without mutating persistent state. + * Filters choose units; once chosen, every enabled sibling consumer is checked. + */ +export async function buildSkillUpdatePreflight( + input: BuildSkillUpdatePreflightInput, + deps: BuildSkillUpdatePreflightDeps, +): Promise { + const selectedScopes = new Set(input.selectedScopes); + const filters = input.filters?.filter(Boolean) ?? []; + const physicalUnits = buildPhysicalRefreshUnits(input.installations).filter( + (unit) => + unit.installations.some( + (entry) => + selectedScopes.has(entry.scope) && + entry.nodes.length > 0 && + (filters.length === 0 || + filters.some((filter) => matchesSkillUpdateFilter(entry, filter))), + ), + ); + + const units: SkillUpdateUnit[] = []; + for (const unit of physicalUnits) { + const nodeIds = new Set(unit.nodes.map((node) => node.id)); + const sharedFailures = (input.failures ?? []).filter((failure) => + failure.nodeIds.some((nodeId) => nodeIds.has(nodeId)), + ); + let inspection: UnitInspection; + if (sharedFailures.length > 0) { + inspection = { + outcome: 'failed', + nodes: [], + installations: [], + error: sharedFailures + .map((failure) => `${failure.source}: ${failure.error}`) + .join('; '), + }; + } else { + try { + inspection = await deps.inspectUnit(unit); + } catch (error) { + inspection = { + outcome: 'failed', + nodes: [], + installations: [], + error: error instanceof Error ? error.message : String(error), + }; + } + } + + const deleted: SkillUpdateSkillImpact[] = []; + const survivors: SkillUpdateSkillImpact[] = []; + + if (inspection.outcome !== 'failed') { + const inspectedByInstallation = new Map( + inspection.installations.map((entry) => [entry.installationId, entry]), + ); + for (const entry of unit.installations) { + const installed = entry.skills.filter((skill) => skill.enabled); + const inspected = inspectedByInstallation.get(entry.id); + if (!inspected || inspected.outcome === 'failed') continue; + if (inspected.outcome === 'plugin-removed') { + deleted.push(...installed.map((skill) => impact(entry, skill))); + continue; + } + if (inspected.outcome === 'local') { + survivors.push(...installed.map((skill) => impact(entry, skill))); + continue; + } + + const available = new Set( + inspected.skills.map((skill) => skill.subpath), + ); + for (const skill of installed) { + (available.has(skill.subpath) ? survivors : deleted).push( + impact(entry, skill), + ); + } + } + } + + const failedInstallation = inspection.installations.find( + (entry) => entry.outcome === 'failed', + ); + const removedInstallationIds = inspection.installations + .filter((entry) => entry.outcome === 'plugin-removed') + .map((entry) => entry.installationId); + const outcome = failedInstallation ? 'failed' : inspection.outcome; + const error = + inspection.error ?? + (failedInstallation?.outcome === 'failed' + ? failedInstallation.error + : undefined); + const blockedByOutOfScope = unit.installations.some( + (installation) => !selectedScopes.has(installation.scope), + ); + + units.push({ + ...unit, + outcome, + inspectedNodes: inspection.nodes, + deleted: outcome === 'failed' ? [] : deleted, + survivors: outcome === 'failed' ? [] : survivors, + removedInstallationIds: + outcome === 'failed' ? [] : removedInstallationIds, + blockedByOutOfScope, + ...(error && { error }), + }); + } + + const unitNodeIds = new Set( + physicalUnits.flatMap((unit) => unit.nodes.map((node) => node.id)), + ); + for (const failure of input.failures ?? []) { + if (!selectedScopes.has(failure.scope)) continue; + if (failure.nodeIds.some((nodeId) => unitNodeIds.has(nodeId))) continue; + units.push({ + id: failure.id, + nodes: [], + installations: [], + outcome: 'failed', + inspectedNodes: [], + deleted: [], + survivors: [], + removedInstallationIds: [], + blockedByOutOfScope: false, + error: `${failure.source}: ${failure.error}`, + }); + } + + return { selectedScopes: input.selectedScopes, units }; +} + +function execution( + unit: SkillUpdateUnit, + status: SkillUpdateExecutionStatus, + error?: string, +): SkillUpdateUnitExecution { + const skillCounts: SkillUpdateSkillCounts = { + updated: + status === 'updated' || status === 'removed' ? unit.survivors.length : 0, + removed: status === 'removed' ? unit.deleted.length : 0, + retained: status === 'retained' ? unit.deleted.length : 0, + }; + return { id: unit.id, status, skillCounts, ...(error && { error }) }; +} + +/** + * Execute a fully inspected plan. The caller must collect interactive decisions + * first; any cancellation aborts this function before the first mutation. + */ +export async function executeSkillUpdatePlan( + plan: SkillUpdatePreflight, + decisions: Record, + deps: ExecuteSkillUpdateDeps, +): Promise { + if (Object.values(decisions).includes('cancel')) { + return { + success: false, + cancelled: true, + units: plan.units.map((unit) => execution(unit, 'cancelled')), + syncedScopes: [], + }; + } + + const results: SkillUpdateUnitExecution[] = []; + const scopesToSync = new Set(); + + for (const unit of plan.units) { + if (unit.outcome === 'failed') { + results.push(execution(unit, 'failed', unit.error)); + continue; + } + if (unit.outcome === 'local') { + results.push(execution(unit, 'skipped')); + continue; + } + + if (unit.blockedByOutOfScope) { + results.push( + execution(unit, unit.deleted.length > 0 ? 'retained' : 'skipped'), + ); + continue; + } + + if (unit.deleted.length > 0) { + const decision = decisions[unit.id] ?? 'retain'; + if (decision !== 'remove') { + results.push(execution(unit, 'retained')); + continue; + } + } + + const revisionByNode = new Map( + unit.inspectedNodes.map((entry) => [entry.nodeId, entry.sha]), + ); + const orderedNodes = unit.nodes + .filter((node) => revisionByNode.has(node.id)) + .sort((left, right) => { + if (left.role === right.role) return left.id.localeCompare(right.id); + return left.role === 'dependency' ? -1 : 1; + }); + const changedNodes: CheckoutNode[] = []; + let prepared: PreparedUnitReconciliation | undefined; + + try { + prepared = await deps.reconcileUnit(unit); + for (const node of orderedNodes) { + const sha = revisionByNode.get(node.id); + if (!sha) throw new Error(`Missing inspected revision for ${node.id}`); + // Include the active node in rollback even if advance fails after a + // destructive reset but before its promise settles. + changedNodes.push(node); + await deps.advanceNode(node, sha); + } + await prepared.commit(); + + for (const installation of unit.installations) { + if (plan.selectedScopes.includes(installation.scope)) { + scopesToSync.add(installation.scope); + } + } + results.push( + execution(unit, unit.deleted.length > 0 ? 'removed' : 'updated'), + ); + } catch (error) { + const errors = [error instanceof Error ? error.message : String(error)]; + if (prepared) { + try { + await prepared.rollback(); + } catch (rollbackError) { + errors.push( + `Config rollback failed: ${rollbackError instanceof Error ? rollbackError.message : String(rollbackError)}`, + ); + } + } + for (const node of changedNodes.reverse()) { + try { + await deps.restoreNode(node, node.currentSha); + } catch (rollbackError) { + errors.push( + `Checkout rollback failed for ${node.id}: ${rollbackError instanceof Error ? rollbackError.message : String(rollbackError)}`, + ); + } + } + results.push(execution(unit, 'failed', errors.join('; '))); + } + } + + const syncedScopes: SkillUpdateScope[] = []; + for (const scope of plan.selectedScopes) { + if (!scopesToSync.has(scope)) continue; + try { + const syncResult = await deps.syncScope(scope, { offline: true }); + if (syncResult.success) { + syncedScopes.push(scope); + continue; + } + results.push({ + id: `sync:${scope}`, + status: 'failed', + skillCounts: { updated: 0, removed: 0, retained: 0 }, + ...(syncResult.error && { error: syncResult.error }), + }); + } catch (error) { + results.push({ + id: `sync:${scope}`, + status: 'failed', + skillCounts: { updated: 0, removed: 0, retained: 0 }, + error: error instanceof Error ? error.message : String(error), + }); + } + } + + return { + success: results.every((result) => result.status !== 'failed'), + cancelled: false, + units: results, + syncedScopes, + }; +} diff --git a/src/core/skills.ts b/src/core/skills.ts index 9d6dad7c..656e0ce1 100644 --- a/src/core/skills.ts +++ b/src/core/skills.ts @@ -1,4 +1,4 @@ -import { existsSync, lstatSync, type Dirent } from 'node:fs'; +import { type Dirent, existsSync, lstatSync } from 'node:fs'; import { readFile, readdir } from 'node:fs/promises'; import { basename, join, relative, resolve } from 'node:path'; import { load } from 'js-yaml'; @@ -106,6 +106,27 @@ export async function discoverNestedSkillEntries( return walkForSkillMd(scanRoot, scanRoot, warnings); } +/** Discover all skills owned by a resolved plugin root. */ +export async function discoverSkillEntriesFromPluginRoot( + pluginPath: string, + warnings: string[] = [], +): Promise { + const skillsDir = join(pluginPath, 'skills'); + if (existsSync(skillsDir)) { + return discoverNestedSkillEntries(skillsDir, warnings); + } + + const nestedSkills = await discoverNestedSkillEntries(pluginPath, warnings); + if (nestedSkills.length > 0) return nestedSkills; + + const rootSkillMd = join(pluginPath, 'SKILL.md'); + if (!existsSync(rootSkillMd)) return []; + const skillContent = await readFile(rootSkillMd, 'utf-8'); + const metadata = parseSkillMetadata(skillContent); + const skillName = metadata?.name ?? basename(pluginPath); + return [{ name: skillName, subpath: skillName, skillPath: pluginPath }]; +} + async function walkForSkillMd( scanRoot: string, currentDir: string, @@ -184,8 +205,6 @@ export async function getAllSkillsFromPlugins( const pluginPath = resolved.path; const pluginName = resolved.pluginName ?? getPluginName(pluginPath); - const skillsDir = join(pluginPath, 'skills'); - // Inline plugin-level skills config (v2+); undefined = all enabled (or use v1 fallback below) const pluginSkillsConfig: PluginSkillsConfig | undefined = typeof pluginEntry === 'string' ? undefined : pluginEntry.skills; @@ -196,29 +215,7 @@ export async function getAllSkillsFromPlugins( enabledSkills && [...enabledSkills].some((s) => s.startsWith(`${pluginName}`)); - let skillEntries: DiscoveredSkillEntry[]; - if (existsSync(skillsDir)) { - // Standard layout: plugin/skills//, possibly nested deeper. - skillEntries = await discoverNestedSkillEntries(skillsDir); - } else { - const nestedSkills = await discoverNestedSkillEntries(pluginPath); - if (nestedSkills.length > 0) { - skillEntries = nestedSkills; - } else { - // Root-level single-skill layout: plugin/SKILL.md - const rootSkillMd = join(pluginPath, 'SKILL.md'); - if (existsSync(rootSkillMd)) { - const skillContent = await readFile(rootSkillMd, 'utf-8'); - const metadata = parseSkillMetadata(skillContent); - const skillName = metadata?.name ?? basename(pluginPath); - skillEntries = [ - { name: skillName, subpath: skillName, skillPath: pluginPath }, - ]; - } else { - skillEntries = []; - } - } - } + const skillEntries = await discoverSkillEntriesFromPluginRoot(pluginPath); const pluginSkillsMode: SkillInfo['pluginSkillsMode'] = pluginSkillsConfig === undefined diff --git a/src/core/sync.ts b/src/core/sync.ts index 24636505..ea63126c 100644 --- a/src/core/sync.ts +++ b/src/core/sync.ts @@ -19,8 +19,7 @@ import type { } from '../models/workspace-config.js'; import { getPluginClients, - getPluginPin, - getPluginSource, + getEffectivePluginSource, getPluginExclude, getClientTypes, normalizeClientEntry, @@ -1248,14 +1247,7 @@ export function buildPluginSyncPlans( const workspaceClientTypes = getClientTypes(clientEntries); const plans = plugins.map((plugin) => { - const rawSource = getPluginSource(plugin); - const pin = getPluginPin(plugin); - // Apply the optional `pin:` workspace.yaml field by splicing `@` into - // GitHub-shaped sources. Marketplace and local sources ignore the pin - // since they don't go through fetchPlugin. - const source = pin && isGitHubUrl(rawSource) && !rawSource.includes('@') - ? `${rawSource}@${pin}` - : rawSource; + const source = getEffectivePluginSource(plugin); const pluginClientTypes = getPluginClients(plugin) ?? workspaceClientTypes; if (pluginClientTypes.length === 0) { diff --git a/src/models/workspace-config.ts b/src/models/workspace-config.ts index 34134697..cb00839c 100644 --- a/src/models/workspace-config.ts +++ b/src/models/workspace-config.ts @@ -206,6 +206,39 @@ export function getPluginSource(plugin: PluginEntry): string { return typeof plugin === 'string' ? plugin : plugin.source; } +/** + * Resolve the source exactly as sync will fetch it, including object-form pins. + * An inline ref remains authoritative when both forms are present. + */ +export function getEffectivePluginSource(plugin: PluginEntry): string { + const source = getPluginSource(plugin); + const pin = getPluginPin(plugin); + if (!pin) return source; + if ( + source.startsWith('.') || + source.startsWith('/') || + source.includes('\\') || + /^[a-zA-Z]:/.test(source) || + (/^[a-z]+:\/\//i.test(source) && + !/^https?:\/\/(?:www\.)?github\.com\//i.test(source)) || + source.slice(0, source.indexOf('/')).includes('@') + ) { + return source; + } + + if (/\/tree\/|\/blob\//.test(source)) return source; + const shorthand = source + .replace(/^https?:\/\/(?:www\.)?github\.com\//, '') + .replace(/^github\.com\//, '') + .replace(/^gh:/, ''); + const parts = shorthand.split('/'); + const repoSegment = parts[1]; + if (!repoSegment || repoSegment.includes('@')) return source; + + parts[1] = `${repoSegment}@${pin}`; + return parts.join('/'); +} + /** * Resolve optional plugin-level clients from plugin entry */ diff --git a/tests/e2e/plugin-update.test.ts b/tests/e2e/plugin-update.test.ts index 34ae9329..2442619a 100644 --- a/tests/e2e/plugin-update.test.ts +++ b/tests/e2e/plugin-update.test.ts @@ -158,51 +158,55 @@ describe('plugin update e2e', () => { }); }); - test('updates the same plugin independently when installed in both scopes', () => { - for (const scope of ['user', 'project']) { - const addResult = runCli(workspaceDir, homeDir, [ + test( + 'updates the same plugin independently when installed in both scopes', + () => { + for (const scope of ['user', 'project']) { + const addResult = runCli(workspaceDir, homeDir, [ + 'plugin', + 'marketplace', + 'add', + marketplaceDir, + '--scope', + scope, + ]); + expect(addResult.exitCode).toBe(0); + + const installResult = runCli(workspaceDir, homeDir, [ + 'plugin', + 'install', + 'demo@project-marketplace', + '--scope', + scope, + ]); + expect(installResult.exitCode).toBe(0); + } + + const updateResult = runCli(workspaceDir, homeDir, [ 'plugin', - 'marketplace', - 'add', - marketplaceDir, - '--scope', - scope, - ]); - expect(addResult.exitCode).toBe(0); - - const installResult = runCli(workspaceDir, homeDir, [ - 'plugin', - 'install', + 'update', 'demo@project-marketplace', '--scope', - scope, + 'all', ]); - expect(installResult.exitCode).toBe(0); - } - const updateResult = runCli(workspaceDir, homeDir, [ - 'plugin', - 'update', - 'demo@project-marketplace', - '--scope', - 'all', - ]); - - expect(updateResult.exitCode).toBe(0); - const payload = JSON.parse(updateResult.stdout); - expect(payload.success).toBe(true); - expect(payload.data.results).toHaveLength(2); - expect(payload.data.results).toEqual([ - { - plugin: 'demo@project-marketplace', - success: true, - action: 'updated', - }, - { - plugin: 'demo@project-marketplace', - success: true, - action: 'updated', - }, - ]); - }); + expect(updateResult.exitCode).toBe(0); + const payload = JSON.parse(updateResult.stdout); + expect(payload.success).toBe(true); + expect(payload.data.results).toHaveLength(2); + expect(payload.data.results).toEqual([ + { + plugin: 'demo@project-marketplace', + success: true, + action: 'updated', + }, + { + plugin: 'demo@project-marketplace', + success: true, + action: 'updated', + }, + ]); + }, + 10_000, + ); }); diff --git a/tests/e2e/skill-update.test.ts b/tests/e2e/skill-update.test.ts new file mode 100644 index 00000000..eee9c4c0 --- /dev/null +++ b/tests/e2e/skill-update.test.ts @@ -0,0 +1,873 @@ +import { + afterEach, + beforeAll, + beforeEach, + describe, + expect, + test, +} from 'bun:test'; +import { existsSync } from 'node:fs'; +import { mkdir, mkdtemp, readFile, rm, writeFile } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { dump, load } from 'js-yaml'; +import simpleGit from 'simple-git'; +import type { WorkspaceConfig } from '../../src/models/workspace-config.js'; + +interface CliResult { + exitCode: number; + stdout: string; + stderr: string; +} + +interface SkillUpdateFixture { + root: string; + workspace: string; + home: string; + upstream: string; + remote: string; + cache: string; + gitConfig: string; + initialSha: string; + updatedSha: string; +} + +interface RemoteSourceFixture { + slug: string; + source: string; + worktree: string; + remote: string; + cache: string; + initialSha: string; + updatedSha: string; +} + +const decoder = new TextDecoder(); +const cliEntry = join(import.meta.dir, '..', '..', 'dist', 'index.js'); + +beforeAll(() => { + const build = Bun.spawnSync(['bun', 'run', 'build'], { + cwd: join(import.meta.dir, '..', '..'), + stderr: 'pipe', + stdout: 'pipe', + }); + if (build.exitCode !== 0) { + throw new Error( + `CLI build failed:\n${decoder.decode(build.stdout)}${decoder.decode(build.stderr)}`, + ); + } + if (!existsSync(cliEntry)) { + throw new Error(`Built CLI not found at ${cliEntry}`); + } +}); + +function cliEnv(fixture: SkillUpdateFixture): Record { + return { + ...process.env, + ALLAGENTS_TEST_HOME: fixture.home, + HOME: fixture.home, + USERPROFILE: fixture.home, + XDG_CONFIG_HOME: join(fixture.home, '.config'), + GIT_CONFIG_GLOBAL: fixture.gitConfig, + GIT_TERMINAL_PROMPT: '0', + NO_COLOR: '1', + } as Record; +} + +function runCli(fixture: SkillUpdateFixture, args: string[]): CliResult { + const proc = Bun.spawnSync([cliEntry, ...args], { + cwd: fixture.workspace, + env: cliEnv(fixture), + stderr: 'pipe', + stdout: 'pipe', + }); + + return { + exitCode: proc.exitCode, + stdout: decoder.decode(proc.stdout), + stderr: decoder.decode(proc.stderr), + }; +} + +async function runInteractiveCli( + fixture: SkillUpdateFixture, + args: string[], + input: string, +): Promise { + const shellQuote = (value: string): string => + `'${value.replaceAll("'", `'\\''`)}'`; + const command = `stty cols 160 rows 40; exec ${[cliEntry, ...args].map(shellQuote).join(' ')}`; + const proc = Bun.spawn(['script', '-qefc', command, '/dev/null'], { + cwd: fixture.workspace, + env: cliEnv(fixture), + stdin: 'pipe', + stdout: 'pipe', + stderr: 'pipe', + }); + const stdoutReader = proc.stdout.getReader(); + const streamDecoder = new TextDecoder(); + let stdout = ''; + let inputSent = false; + const readStdout = (async () => { + while (true) { + const { done, value } = await stdoutReader.read(); + if (done) break; + stdout += streamDecoder.decode(value, { stream: true }); + if (!inputSent && stdout.includes('deleted upstream')) { + inputSent = true; + await new Promise((resolve) => setTimeout(resolve, 75)); + proc.stdin.write(input); + proc.stdin.end(); + } + } + stdout += streamDecoder.decode(); + })(); + const [exitCode, stderr] = await Promise.all([ + proc.exited, + new Response(proc.stderr).text(), + readStdout, + ]).then(([code, error]) => [code, error] as const); + const result = { + exitCode, + stdout, + stderr, + }; + return result; +} + +async function writeSkill( + root: string, + name: string, + body: string, +): Promise { + const directory = join(root, 'skills', name); + await mkdir(directory, { recursive: true }); + await writeFile( + join(directory, 'SKILL.md'), + `---\nname: ${name}\ndescription: ${name} fixture\n---\n${body}\n`, + ); +} + +async function appendGitRedirect( + fixture: SkillUpdateFixture, + remote: string, + slug: string, +): Promise { + const existing = await readFile(fixture.gitConfig, 'utf8'); + await writeFile( + fixture.gitConfig, + `${existing}[url "file://${remote}"]\n\tinsteadOf = https://github.com/uat/${slug}.git\n`, + ); +} + +async function createRemoteSource( + fixture: SkillUpdateFixture, + slug: string, + initialSkills: Array<{ name: string; body: string }>, + updatedSkills: Array<{ name: string; body: string }>, +): Promise { + const worktree = join(fixture.root, `${slug}-work`); + const remote = join(fixture.root, `${slug}.git`); + const cache = join( + fixture.home, + '.allagents', + 'plugins', + 'marketplaces', + `uat-${slug}`, + ); + await mkdir(worktree, { recursive: true }); + const git = simpleGit(worktree); + await git.init(); + await git.checkoutLocalBranch('main'); + await git.addConfig('user.name', 'AllAgents UAT'); + await git.addConfig('user.email', 'allagents@example.test'); + for (const skill of initialSkills) { + await writeSkill(worktree, skill.name, skill.body); + } + await git.add('.'); + await git.commit('fixture v1'); + const initialSha = (await git.revparse(['HEAD'])).trim(); + await simpleGit().raw(['init', '--bare', remote]); + await git.addRemote('origin', remote); + await git.push(['-u', 'origin', 'main']); + await simpleGit(remote).raw(['symbolic-ref', 'HEAD', 'refs/heads/main']); + await appendGitRedirect(fixture, remote, slug); + + const clone = Bun.spawnSync( + ['git', 'clone', `https://github.com/uat/${slug}.git`, cache], + { + env: cliEnv(fixture), + stderr: 'pipe', + stdout: 'pipe', + }, + ); + if (clone.exitCode !== 0) { + throw new Error(`fixture clone failed: ${decoder.decode(clone.stderr)}`); + } + await simpleGit(cache).remote([ + 'set-url', + 'origin', + `https://github.com/uat/${slug}.git`, + ]); + + await rm(join(worktree, 'skills'), { recursive: true, force: true }); + for (const skill of updatedSkills) { + await writeSkill(worktree, skill.name, skill.body); + } + await git.add(['-A']); + await git.commit('fixture v2'); + await git.push('origin', 'main'); + const updatedSha = (await git.revparse(['HEAD'])).trim(); + + return { + slug, + source: `uat/${slug}`, + worktree, + remote, + cache, + initialSha, + updatedSha, + }; +} + +async function writeEmbeddedMarketplaceVersion( + root: string, + keepBody: string, + includeGone: boolean, +): Promise { + await rm(join(root, 'plugins'), { recursive: true, force: true }); + await mkdir(join(root, '.claude-plugin'), { recursive: true }); + await writeFile( + join(root, '.claude-plugin', 'marketplace.json'), + `${JSON.stringify( + { + name: 'uat-market', + description: 'Embedded marketplace E2E fixture', + plugins: [ + { + name: 'demo', + description: 'Embedded demo plugin', + source: './plugins/demo', + }, + ], + }, + null, + 2, + )}\n`, + ); + const pluginRoot = join(root, 'plugins', 'demo'); + await writeSkill(pluginRoot, 'keep', keepBody); + if (includeGone) await writeSkill(pluginRoot, 'gone', '# gone v1'); +} + +async function createEmbeddedMarketplaceSource( + fixture: SkillUpdateFixture, +): Promise { + const slug = 'skill-market-e2e'; + const worktree = join(fixture.root, `${slug}-work`); + const remote = join(fixture.root, `${slug}.git`); + const cache = join( + fixture.home, + '.allagents', + 'plugins', + 'marketplaces', + 'uat-market', + ); + await mkdir(worktree, { recursive: true }); + const git = simpleGit(worktree); + await git.init(); + await git.checkoutLocalBranch('main'); + await git.addConfig('user.name', 'AllAgents UAT'); + await git.addConfig('user.email', 'allagents@example.test'); + await writeEmbeddedMarketplaceVersion(worktree, '# keep v1', true); + await git.add('.'); + await git.commit('marketplace v1'); + const initialSha = (await git.revparse(['HEAD'])).trim(); + await simpleGit().raw(['init', '--bare', remote]); + await git.addRemote('origin', remote); + await git.push(['-u', 'origin', 'main']); + await simpleGit(remote).raw(['symbolic-ref', 'HEAD', 'refs/heads/main']); + await appendGitRedirect(fixture, remote, slug); + + const clone = Bun.spawnSync( + ['git', 'clone', `https://github.com/uat/${slug}.git`, cache], + { + env: cliEnv(fixture), + stderr: 'pipe', + stdout: 'pipe', + }, + ); + if (clone.exitCode !== 0) { + throw new Error( + `marketplace fixture clone failed: ${decoder.decode(clone.stderr)}`, + ); + } + await simpleGit(cache).remote([ + 'set-url', + 'origin', + `https://github.com/uat/${slug}.git`, + ]); + + await writeEmbeddedMarketplaceVersion(worktree, '# keep v2', false); + await git.add(['-A']); + await git.commit('marketplace v2'); + await git.push('origin', 'main'); + const updatedSha = (await git.revparse(['HEAD'])).trim(); + + await writeFile( + join(fixture.workspace, '.allagents', 'marketplaces.json'), + `${JSON.stringify( + { + version: 1, + marketplaces: { + 'uat-market': { + name: 'uat-market', + source: { type: 'github', location: `uat/${slug}` }, + path: cache, + }, + }, + }, + null, + 2, + )}\n`, + ); + await writeProjectConfig(fixture, [ + { source: 'demo@uat-market', skills: ['keep', 'gone'] }, + ]); + + return { + slug, + source: 'demo@uat-market', + worktree, + remote, + cache, + initialSha, + updatedSha, + }; +} + +async function createFixture(): Promise { + const root = await mkdtemp(join(tmpdir(), 'allagents-e2e-skill-update-')); + const workspace = join(root, 'workspace'); + const home = join(root, 'home'); + const upstream = join(root, 'upstream-work'); + const remote = join(root, 'upstream.git'); + const cache = join( + home, + '.allagents', + 'plugins', + 'marketplaces', + 'uat-skill-update-e2e', + ); + const gitConfig = join(root, 'gitconfig'); + + await mkdir(join(workspace, '.allagents'), { recursive: true }); + await mkdir(home, { recursive: true }); + await mkdir(upstream, { recursive: true }); + + const upstreamGit = simpleGit(upstream); + await upstreamGit.init(); + await upstreamGit.checkoutLocalBranch('main'); + await upstreamGit.addConfig('user.name', 'AllAgents UAT'); + await upstreamGit.addConfig('user.email', 'allagents@example.test'); + await writeSkill(upstream, 'keep', '# keep v1'); + await writeSkill(upstream, 'gone', '# gone v1'); + await upstreamGit.add('.'); + await upstreamGit.commit('fixture v1'); + const initialSha = (await upstreamGit.revparse(['HEAD'])).trim(); + + await simpleGit().raw(['init', '--bare', remote]); + await upstreamGit.addRemote('origin', remote); + await upstreamGit.push(['-u', 'origin', 'main']); + await simpleGit(remote).raw(['symbolic-ref', 'HEAD', 'refs/heads/main']); + + await writeFile( + gitConfig, + `[protocol "file"]\n\tallow = always\n[url "file://${remote}"]\n\tinsteadOf = https://github.com/uat/skill-update-e2e.git\n`, + ); + + await mkdir(join(cache, '..'), { recursive: true }); + const clone = Bun.spawnSync( + ['git', 'clone', 'https://github.com/uat/skill-update-e2e.git', cache], + { + env: { + ...process.env, + GIT_CONFIG_GLOBAL: gitConfig, + GIT_TERMINAL_PROMPT: '0', + }, + stdout: 'pipe', + stderr: 'pipe', + }, + ); + if (clone.exitCode !== 0) { + throw new Error(`fixture clone failed: ${decoder.decode(clone.stderr)}`); + } + await simpleGit(cache).remote([ + 'set-url', + 'origin', + 'https://github.com/uat/skill-update-e2e.git', + ]); + + const config: WorkspaceConfig = { + repositories: [], + plugins: [ + { + source: 'uat/skill-update-e2e', + skills: ['keep', 'gone'], + }, + ], + clients: ['claude'], + version: 2, + }; + await writeFile( + join(workspace, '.allagents', 'workspace.yaml'), + dump(config), + ); + + await writeSkill(upstream, 'keep', '# keep v2'); + await rm(join(upstream, 'skills', 'gone'), { recursive: true }); + await upstreamGit.add(['-A']); + await upstreamGit.commit('fixture v2'); + await upstreamGit.push('origin', 'main'); + const updatedSha = (await upstreamGit.revparse(['HEAD'])).trim(); + + return { + root, + workspace, + home, + upstream, + remote, + cache, + gitConfig, + initialSha, + updatedSha, + }; +} + +async function cacheSha(fixture: SkillUpdateFixture): Promise { + return (await simpleGit(fixture.cache).revparse(['HEAD'])).trim(); +} + +async function checkoutSha(path: string): Promise { + return (await simpleGit(path).revparse(['HEAD'])).trim(); +} + +async function writeProjectConfig( + fixture: SkillUpdateFixture, + plugins: WorkspaceConfig['plugins'], +): Promise { + const config: WorkspaceConfig = { + repositories: [], + plugins, + clients: ['claude'], + version: 2, + }; + await writeFile( + join(fixture.workspace, '.allagents', 'workspace.yaml'), + dump(config), + ); +} + +async function writeUserConfig( + fixture: SkillUpdateFixture, + plugins: WorkspaceConfig['plugins'], +): Promise { + const config: WorkspaceConfig = { + repositories: [], + plugins, + clients: ['claude'], + version: 2, + }; + await mkdir(join(fixture.home, '.allagents'), { recursive: true }); + await writeFile( + join(fixture.home, '.allagents', 'workspace.yaml'), + dump(config), + ); +} + +async function readWorkspaceConfig(path: string): Promise { + return load(await readFile(path, 'utf8')) as WorkspaceConfig; +} + +function expectPluginSkills( + config: WorkspaceConfig, + index: number, + skills: string[], +): void { + const plugin = config.plugins[index]; + expect(typeof plugin === 'string' ? undefined : plugin?.skills).toEqual( + skills, + ); +} + +async function addUnreachableSiblingSource( + fixture: SkillUpdateFixture, +): Promise<{ cache: string; sha: string }> { + const cache = join( + fixture.home, + '.allagents', + 'plugins', + 'marketplaces', + 'uat-unreachable-skill-update', + ); + await writeFile( + fixture.gitConfig, + `[protocol "file"]\n\tallow = always\n[url "file://${fixture.remote}"]\n\tinsteadOf = https://github.com/uat/skill-update-e2e.git\n\tinsteadOf = https://github.com/uat/unreachable-skill-update.git\n`, + ); + const clone = Bun.spawnSync( + [ + 'git', + 'clone', + 'https://github.com/uat/unreachable-skill-update.git', + cache, + ], + { + env: { + ...process.env, + GIT_CONFIG_GLOBAL: fixture.gitConfig, + GIT_TERMINAL_PROMPT: '0', + }, + stdout: 'pipe', + stderr: 'pipe', + }, + ); + if (clone.exitCode !== 0) { + throw new Error( + `sibling fixture clone failed: ${decoder.decode(clone.stderr)}`, + ); + } + await simpleGit(cache).remote([ + 'set-url', + 'origin', + 'https://github.com/uat/unreachable-skill-update.git', + ]); + const sha = (await simpleGit(cache).revparse(['HEAD'])).trim(); + + await writeFile( + fixture.gitConfig, + `[protocol "file"]\n\tallow = always\n[url "file://${fixture.remote}"]\n\tinsteadOf = https://github.com/uat/skill-update-e2e.git\n[url "file://${join(fixture.root, 'missing.git')}"]\n\tinsteadOf = https://github.com/uat/unreachable-skill-update.git\n`, + ); + const config: WorkspaceConfig = { + repositories: [], + plugins: [ + { source: 'uat/skill-update-e2e', skills: ['keep'] }, + { source: 'uat/unreachable-skill-update', skills: ['keep'] }, + ], + clients: ['claude'], + version: 2, + }; + await writeFile( + join(fixture.workspace, '.allagents', 'workspace.yaml'), + dump(config), + ); + return { cache, sha }; +} + +describe('skill update CLI e2e', () => { + let fixtures: SkillUpdateFixture[] = []; + + beforeEach(() => { + fixtures = []; + }); + + afterEach(async () => { + await Promise.all( + fixtures.map((fixture) => + rm(fixture.root, { recursive: true, force: true }), + ), + ); + }); + + test('non-interactive mode retains deleted skills and preserves the shared cache', async () => { + const fixture = await createFixture(); + fixtures.push(fixture); + + const result = runCli(fixture, [ + '--json', + 'skill', + 'update', + '--scope', + 'project', + '--yes', + ]); + + expect(result.exitCode).toBe(0); + expect(result.stderr).toBe(''); + const payload = JSON.parse(result.stdout); + expect(payload.success).toBe(true); + expect(payload.data.summary).toMatchObject({ + updated: 0, + removed: 0, + retained: 1, + skipped: 1, + failed: 0, + }); + expect(payload.data.results).toHaveLength(1); + expect(payload.data.results[0]).toMatchObject({ + status: 'retained', + skillCounts: { updated: 0, removed: 0, retained: 1 }, + }); + expect(await cacheSha(fixture)).toBe(fixture.initialSha); + expect(existsSync(join(fixture.cache, 'skills', 'gone', 'SKILL.md'))).toBe( + true, + ); + expect( + await readFile(join(fixture.cache, 'skills', 'keep', 'SKILL.md'), 'utf8'), + ).toContain('# keep v1'); + }); + + test('a confirmed deletion removes the selector and advances surviving skills', async () => { + const fixture = await createFixture(); + fixtures.push(fixture); + const result = await runInteractiveCli( + fixture, + ['skill', 'update', '--scope', 'project'], + 'y\n', + ); + + expect(result.exitCode).toBe(0); + expect(result.stderr).toBe(''); + expect(result.stdout).toContain('appear to have been deleted upstream'); + expect(result.stdout).toContain('Removed deleted skills and updated'); + expect(await cacheSha(fixture)).toBe(fixture.updatedSha); + expect(existsSync(join(fixture.cache, 'skills', 'gone'))).toBe(false); + expect( + await readFile(join(fixture.cache, 'skills', 'keep', 'SKILL.md'), 'utf8'), + ).toContain('# keep v2'); + + const rawConfig = await readFile( + join(fixture.workspace, '.allagents', 'workspace.yaml'), + 'utf8', + ); + expectPluginSkills(load(rawConfig) as WorkspaceConfig, 0, ['keep']); + }); + + test('interactive No retains one source while an independent healthy source updates', async () => { + const fixture = await createFixture(); + fixtures.push(fixture); + const healthy = await createRemoteSource( + fixture, + 'healthy-skill-update-e2e', + [{ name: 'healthy', body: '# healthy v1' }], + [{ name: 'healthy', body: '# healthy v2' }], + ); + await writeProjectConfig(fixture, [ + { source: 'uat/skill-update-e2e', skills: ['keep', 'gone'] }, + { source: healthy.source, skills: ['healthy'] }, + ]); + + const seed = runCli(fixture, ['update', '--offline']); + expect(seed.exitCode).toBe(0); + const configPath = join( + fixture.workspace, + '.allagents', + 'workspace.yaml', + ); + const goneArtifact = join( + fixture.workspace, + '.claude', + 'skills', + 'gone', + 'SKILL.md', + ); + const configBefore = await readFile(configPath, 'utf8'); + const goneBefore = await readFile(goneArtifact, 'utf8'); + + const result = await runInteractiveCli( + fixture, + ['skill', 'update', '--scope', 'project'], + 'n\n', + ); + + expect(result.exitCode).toBe(0); + expect(result.stderr).toBe(''); + expect(result.stdout).toContain( + 'No keeps them and skips every update from this source', + ); + expect(result.stdout).toContain('Kept local copies and skipped updates'); + expect(result.stdout).toContain(`Updated ${healthy.source}`); + expect(await readFile(configPath, 'utf8')).toBe(configBefore); + expect(await cacheSha(fixture)).toBe(fixture.initialSha); + expect(await readFile(goneArtifact, 'utf8')).toBe(goneBefore); + expect(await checkoutSha(healthy.cache)).toBe(healthy.updatedSha); + expect( + await readFile( + join(fixture.workspace, '.claude', 'skills', 'healthy', 'SKILL.md'), + 'utf8', + ), + ).toContain('# healthy v2'); + }, 15_000); + + test('interactive cancellation leaves config, cache, and synced artifacts unchanged', async () => { + const fixture = await createFixture(); + fixtures.push(fixture); + const seed = runCli(fixture, ['update', '--offline']); + expect(seed.exitCode).toBe(0); + const configPath = join( + fixture.workspace, + '.allagents', + 'workspace.yaml', + ); + const keepArtifact = join( + fixture.workspace, + '.claude', + 'skills', + 'keep', + 'SKILL.md', + ); + const goneArtifact = join( + fixture.workspace, + '.claude', + 'skills', + 'gone', + 'SKILL.md', + ); + const configBefore = await readFile(configPath, 'utf8'); + const keepBefore = await readFile(keepArtifact, 'utf8'); + const goneBefore = await readFile(goneArtifact, 'utf8'); + + const result = await runInteractiveCli( + fixture, + ['skill', 'update', '--scope', 'project'], + '\u0003', + ); + + expect(result.exitCode).toBe(0); + expect(result.stderr).toBe(''); + expect(result.stdout).toContain('Update cancelled before changes'); + expect(await readFile(configPath, 'utf8')).toBe(configBefore); + expect(await cacheSha(fixture)).toBe(fixture.initialSha); + expect(await readFile(keepArtifact, 'utf8')).toBe(keepBefore); + expect(await readFile(goneArtifact, 'utf8')).toBe(goneBefore); + }); + + test('shared project and user cache blocks project-only update and reconciles once with scope all', async () => { + const fixture = await createFixture(); + fixtures.push(fixture); + await writeUserConfig(fixture, [ + { source: 'uat/skill-update-e2e', skills: ['keep', 'gone'] }, + ]); + + const projectOnly = runCli(fixture, [ + '--json', + 'skill', + 'update', + '--scope', + 'project', + '--yes', + ]); + expect(projectOnly.exitCode).toBe(0); + const projectPayload = JSON.parse(projectOnly.stdout); + expect(projectPayload.data.results).toHaveLength(1); + expect(projectPayload.data.results[0]).toMatchObject({ + status: 'retained', + skillCounts: { removed: 0, retained: 2 }, + }); + expect(await cacheSha(fixture)).toBe(fixture.initialSha); + + const scopeAll = await runInteractiveCli( + fixture, + ['skill', 'update', '--scope', 'all'], + 'y\n', + ); + expect(scopeAll.exitCode).toBe(0); + expect(scopeAll.stderr).toBe(''); + expect(scopeAll.stdout).toContain('gone (project)'); + expect(scopeAll.stdout).toContain('gone (user)'); + expect(scopeAll.stdout).toContain('Removed deleted skills and updated'); + expect(await cacheSha(fixture)).toBe(fixture.updatedSha); + + const projectConfig = await readWorkspaceConfig( + join(fixture.workspace, '.allagents', 'workspace.yaml'), + ); + const userConfig = await readWorkspaceConfig( + join(fixture.home, '.allagents', 'workspace.yaml'), + ); + expectPluginSkills(projectConfig, 0, ['keep']); + expectPluginSkills(userConfig, 0, ['keep']); + }); + + test('confirmed embedded marketplace deletion advances the marketplace cache', async () => { + const fixture = await createFixture(); + fixtures.push(fixture); + const marketplace = await createEmbeddedMarketplaceSource(fixture); + + const result = await runInteractiveCli( + fixture, + ['skill', 'update', '--scope', 'project'], + 'y\n', + ); + + expect(result.exitCode).toBe(0); + expect(result.stderr).toBe(''); + expect(result.stdout).toContain( + 'skills from demo@uat-market appear to have been deleted upstream', + ); + expect(result.stdout).toContain('Removed deleted skills and updated'); + expect(await checkoutSha(marketplace.cache)).toBe(marketplace.updatedSha); + expect( + existsSync( + join( + marketplace.cache, + 'plugins', + 'demo', + 'skills', + 'gone', + 'SKILL.md', + ), + ), + ).toBe(false); + expect( + await readFile( + join( + marketplace.cache, + 'plugins', + 'demo', + 'skills', + 'keep', + 'SKILL.md', + ), + 'utf8', + ), + ).toContain('# keep v2'); + const config = await readWorkspaceConfig( + join(fixture.workspace, '.allagents', 'workspace.yaml'), + ); + expectPluginSkills(config, 0, ['keep']); + }); + + test('one unreachable source fails without blocking an independent healthy update', async () => { + const fixture = await createFixture(); + fixtures.push(fixture); + const unreachable = await addUnreachableSiblingSource(fixture); + + const result = runCli(fixture, [ + '--json', + 'skill', + 'update', + '--scope', + 'project', + '--yes', + ]); + + expect(result.exitCode).toBe(1); + expect(result.stderr).toBe(''); + const payload = JSON.parse(result.stdout); + expect(payload.success).toBe(false); + expect(payload.data.summary).toMatchObject({ + updated: 1, + failed: 1, + removed: 0, + retained: 0, + }); + expect( + payload.data.results + .map((entry: { status: string }) => entry.status) + .sort(), + ).toEqual(['failed', 'updated']); + expect(await cacheSha(fixture)).toBe(fixture.updatedSha); + expect((await simpleGit(unreachable.cache).revparse(['HEAD'])).trim()).toBe( + unreachable.sha, + ); + }); +}); diff --git a/tests/unit/cli/agent-help.test.ts b/tests/unit/cli/agent-help.test.ts index 2c4cef3f..f9ebe32c 100644 --- a/tests/unit/cli/agent-help.test.ts +++ b/tests/unit/cli/agent-help.test.ts @@ -18,6 +18,7 @@ import { skillsAddMeta, skillsRemoveMeta, skillsSearchMeta, + skillsUpdateMeta, } from '../../../src/cli/metadata/plugin-skills.js'; import type { AgentCommandMeta } from '../../../src/cli/help.js'; @@ -38,6 +39,7 @@ const allCommands: AgentCommandMeta[] = [ skillsAddMeta, skillsRemoveMeta, skillsSearchMeta, + skillsUpdateMeta, updateMeta, ]; @@ -68,8 +70,8 @@ describe('extractAgentHelpFlag', () => { }); describe('agent command metadata', () => { - test('contains exactly 17 commands', () => { - expect(allCommands.length).toBe(17); + test('contains exactly 18 commands', () => { + expect(allCommands.length).toBe(18); }); test('all expected commands are present', () => { @@ -90,6 +92,7 @@ describe('agent command metadata', () => { 'skill list', 'skill remove', 'skill search', + 'skill update', 'status', 'update', ]); @@ -151,6 +154,11 @@ describe('findMetaByCommand', () => { expect(meta!.command).toBe('status'); }); + test('resolves command metadata when rest-positionals follow the command', () => { + const meta = findMetaByCommand('skill update code-review glow-api'); + expect(meta?.command).toBe('skill update'); + }); + test('returns undefined for unknown command', () => { expect(findMetaByCommand('workspace frobnicate')).toBeUndefined(); }); diff --git a/tests/unit/cli/index-json-fields.test.ts b/tests/unit/cli/index-json-fields.test.ts new file mode 100644 index 00000000..ad830040 --- /dev/null +++ b/tests/unit/cli/index-json-fields.test.ts @@ -0,0 +1,28 @@ +import { describe, expect, test } from 'bun:test'; +import { join } from 'node:path'; + +const cliEntry = join(import.meta.dir, '..', '..', '..', 'src', 'cli', 'index.ts'); + +describe('CLI JSON field validation', () => { + test('validates skill update fields when skill filters follow the command', () => { + const proc = Bun.spawnSync( + [ + 'bun', + 'run', + cliEntry, + '--json=definitely-invalid', + 'skill', + 'update', + 'code-review', + ], + { stdout: 'pipe', stderr: 'pipe' }, + ); + + expect(proc.exitCode).toBe(2); + expect(proc.stderr.toString()).toContain( + 'Unknown JSON field: "definitely-invalid"', + ); + expect(proc.stderr.toString()).toContain('Available fields:'); + expect(proc.stdout.toString()).toBe(''); + }); +}); diff --git a/tests/unit/cli/skill-arg-normalizer.test.ts b/tests/unit/cli/skill-arg-normalizer.test.ts index d1114f81..57b795fa 100644 --- a/tests/unit/cli/skill-arg-normalizer.test.ts +++ b/tests/unit/cli/skill-arg-normalizer.test.ts @@ -26,6 +26,13 @@ describe('normalizeSkillHelpArgs', () => { }); describe('normalizeSkillArgs', () => { + test('keeps update filters attached to the update subcommand', () => { + expect(normalizeSkillArgs(['skills', 'update', 'code-review'])).toEqual([ + 'skill', + 'update', + 'code-review', + ]); + }); test('keeps non-skill commands unchanged', () => { expect(normalizeSkillArgs(['plugin', 'install', 'foo'])).toEqual(['plugin', 'install', 'foo']); }); diff --git a/tests/unit/cli/skill-removal.test.ts b/tests/unit/cli/skill-removal.test.ts index 68069af1..2849c2bb 100644 --- a/tests/unit/cli/skill-removal.test.ts +++ b/tests/unit/cli/skill-removal.test.ts @@ -2,7 +2,7 @@ import { describe, it, expect, beforeEach, afterEach } from 'bun:test'; import { mkdtemp, mkdir, readFile, rm, writeFile } from 'node:fs/promises'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; -import { load, dump } from 'js-yaml'; +import { dump, load } from 'js-yaml'; import { removeInstalledSkill } from '../../../src/cli/skill-removal.js'; import type { WorkspaceConfig } from '../../../src/models/workspace-config.js'; import { diff --git a/tests/unit/cli/skill-update-command.test.ts b/tests/unit/cli/skill-update-command.test.ts new file mode 100644 index 00000000..2546ca06 --- /dev/null +++ b/tests/unit/cli/skill-update-command.test.ts @@ -0,0 +1,677 @@ +import { afterEach, describe, expect, test } from 'bun:test'; +import { mkdir, mkdtemp, rm, writeFile } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { dump } from 'js-yaml'; +import { + buildSkillUpdateInventory, + inspectSkillUpdateUnit, + moveSkillUpdateCheckout, + normalizeSkillUpdateScopes, + resolveNonInteractiveSkillUpdateDecisions, + skillUpdateExitCode, + skillUpdateSummary, +} from '../../../src/cli/skill-update.js'; +import { + executeSkillUpdatePlan, + type SkillUpdatePreflight, +} from '../../../src/core/skill-update.js'; +import { getPluginCachePath } from '../../../src/utils/plugin-path.js'; + +const originalTestHome = process.env.ALLAGENTS_TEST_HOME; + +afterEach(() => { + if (originalTestHome === undefined) delete process.env.ALLAGENTS_TEST_HOME; + else process.env.ALLAGENTS_TEST_HOME = originalTestHome; +}); + +function runGit(path: string, args: string[]): string { + const result = Bun.spawnSync(['git', '-C', path, ...args], { + stdout: 'pipe', + stderr: 'pipe', + }); + if (result.exitCode !== 0) { + throw new Error( + `git ${args.join(' ')} failed in ${path}: ${result.stderr.toString().trim()}`, + ); + } + return result.stdout.toString().trim(); +} + +async function initCheckout(path: string): Promise { + await mkdir(path, { recursive: true }); + runGit(path, ['init']); + runGit(path, ['config', '--local', 'user.name', 'Skill Update Test']); + runGit(path, [ + 'config', + '--local', + 'user.email', + 'skill-update@example.test', + ]); + runGit(path, ['add', '.']); + runGit(path, ['commit', '-m', 'fixture']); +} + +describe('skill update command adapters', () => { + test('creates git fixtures without depending on the simple-git module', async () => { + const root = await mkdtemp(join(tmpdir(), 'allagents-skill-update-git-')); + try { + await writeFile(join(root, 'fixture.txt'), 'fixture'); + await initCheckout(root); + + expect(runGit(root, ['rev-parse', '--is-inside-work-tree'])).toBe('true'); + expect(runGit(root, ['log', '-1', '--format=%s'])).toBe('fixture'); + } finally { + await rm(root, { recursive: true, force: true }); + } + }); + + test('normalizes project, user, and all scopes', () => { + expect(normalizeSkillUpdateScopes('project')).toEqual(['project']); + expect(normalizeSkillUpdateScopes('user')).toEqual(['user']); + expect(normalizeSkillUpdateScopes('all')).toEqual([ + 'project', + 'user', + ]); + expect(normalizeSkillUpdateScopes(undefined)).toEqual(['project']); + }); + + test('retains deletion units in every non-interactive mode, including --yes', () => { + const plan = { + selectedScopes: ['project'], + units: [ + { + id: 'deleted-cache', + nodes: [], + installations: [], + outcome: 'resolved', + inspectedNodes: [], + deleted: [ + { + name: 'gone', + subpath: 'gone', + enabled: true, + installationId: 'project:0', + scope: 'project', + pluginName: 'skills', + source: 'acme/skills', + }, + ], + survivors: [], + removedInstallationIds: [], + blockedByOutOfScope: false, + }, + { + id: 'healthy-cache', + nodes: [], + installations: [], + outcome: 'resolved', + inspectedNodes: [], + deleted: [], + survivors: [], + removedInstallationIds: [], + blockedByOutOfScope: false, + }, + ], + } satisfies SkillUpdatePreflight; + + expect(resolveNonInteractiveSkillUpdateDecisions(plan)).toEqual({ + 'deleted-cache': 'retain', + }); + expect(resolveNonInteractiveSkillUpdateDecisions(plan)).toEqual({ + 'deleted-cache': 'retain', + }); + }); + + test('summarizes skill impacts while retaining physical-unit status details', async () => { + const skills = Array.from({ length: 7 }, (_, index) => ({ + name: `skill-${index + 1}`, + subpath: `skill-${index + 1}`, + enabled: true, + })); + const plan = { + selectedScopes: ['project'], + units: [ + { + id: 'seven-skill-cache', + nodes: [], + installations: [], + outcome: 'resolved', + inspectedNodes: [], + deleted: skills.slice(5).map((skill) => ({ + ...skill, + installationId: 'project:0', + scope: 'project', + pluginName: 'skills', + source: 'acme/skills', + })), + survivors: skills.slice(0, 5).map((skill) => ({ + ...skill, + installationId: 'project:0', + scope: 'project', + pluginName: 'skills', + source: 'acme/skills', + })), + removedInstallationIds: [], + blockedByOutOfScope: false, + }, + ], + } satisfies SkillUpdatePreflight; + const result = await executeSkillUpdatePlan( + plan, + { 'seven-skill-cache': 'remove' }, + { + advanceNode: async () => {}, + restoreNode: async () => {}, + reconcileUnit: async () => ({ + commit: async () => {}, + rollback: async () => {}, + }), + syncScope: async () => ({ success: true }), + }, + ); + + expect(result.units).toEqual([ + { + id: 'seven-skill-cache', + status: 'removed', + skillCounts: { updated: 5, removed: 2, retained: 0 }, + }, + ]); + expect(skillUpdateSummary(result)).toEqual({ + updated: 5, + removed: 2, + retained: 0, + skipped: 0, + failed: 0, + cancelled: 0, + }); + }); + + test('reports retained skill and skipped-source totals for non-interactive deletion safety', async () => { + const plan = { + selectedScopes: ['project'], + units: [ + { + id: 'deleted-cache', + nodes: [], + installations: [], + outcome: 'resolved', + inspectedNodes: [], + deleted: ['gone-a', 'gone-b'].map((subpath) => ({ + name: subpath, + subpath, + enabled: true, + installationId: 'project:0', + scope: 'project', + pluginName: 'skills', + source: 'acme/skills', + })), + survivors: [], + removedInstallationIds: [], + blockedByOutOfScope: false, + }, + ], + } satisfies SkillUpdatePreflight; + const result = await executeSkillUpdatePlan( + plan, + resolveNonInteractiveSkillUpdateDecisions(plan), + { + advanceNode: async () => { + throw new Error('retained sources must not advance'); + }, + restoreNode: async () => {}, + reconcileUnit: async () => { + throw new Error('retained sources must not reconcile'); + }, + syncScope: async () => ({ success: true }), + }, + ); + + expect(skillUpdateSummary(result)).toMatchObject({ + retained: 2, + skipped: 1, + }); + }); + + test('maps cancellation to success false with exit zero', () => { + expect( + skillUpdateExitCode({ + success: false, + cancelled: true, + units: [], + syncedScopes: [], + }), + ).toBe(0); + expect( + skillUpdateExitCode({ + success: false, + cancelled: false, + units: [], + syncedScopes: [], + }), + ).toBe(1); + }); + + test('builds one connected marketplace graph for embedded and external plugins', async () => { + const root = await mkdtemp(join(tmpdir(), 'allagents-skill-update-inventory-')); + const home = join(root, 'home'); + const workspace = join(root, 'workspace'); + const marketplace = join(home, '.allagents/plugins/marketplaces/catalog'); + process.env.ALLAGENTS_TEST_HOME = home; + + try { + const external = getPluginCachePath('acme', 'external-skills'); + await mkdir(join(workspace, '.allagents'), { recursive: true }); + await mkdir(join(home, '.allagents'), { recursive: true }); + await mkdir(join(marketplace, '.claude-plugin'), { recursive: true }); + await mkdir(join(marketplace, 'plugins/embedded/skills/inside'), { + recursive: true, + }); + await mkdir(join(external, 'skills/outside'), { recursive: true }); + await writeFile( + join(marketplace, 'plugins/embedded/skills/inside/SKILL.md'), + '---\nname: inside\ndescription: embedded\n---\n', + ); + await writeFile( + join(external, 'skills/outside/SKILL.md'), + '---\nname: outside\ndescription: external\n---\n', + ); + await writeFile( + join(marketplace, '.claude-plugin/marketplace.json'), + JSON.stringify({ + name: 'catalog', + description: 'test catalog', + plugins: [ + { + name: 'embedded', + description: 'embedded plugin', + source: './plugins/embedded', + }, + { + name: 'external', + description: 'external plugin', + source: { + source: 'url', + url: 'https://github.com/acme/external-skills', + }, + }, + ], + }), + ); + await initCheckout(marketplace); + await initCheckout(external); + await writeFile( + join(home, '.allagents/marketplaces.json'), + JSON.stringify({ + version: 1, + marketplaces: { + catalog: { + name: 'catalog', + source: { type: 'github', location: 'acme/catalog' }, + path: marketplace, + }, + }, + }), + ); + await writeFile( + join(workspace, '.allagents/workspace.yaml'), + dump({ + version: 2, + repositories: [], + clients: ['copilot'], + plugins: ['embedded@catalog', 'external@catalog'], + }), + ); + + const inventory = await buildSkillUpdateInventory(workspace); + + expect(inventory.installations).toHaveLength(2); + const embedded = inventory.installations.find( + (entry) => entry.pluginName === 'embedded', + ); + const externalEntry = inventory.installations.find( + (entry) => entry.pluginName === 'external', + ); + expect(embedded?.nodes.map((node) => node.id)).toEqual([marketplace]); + expect(externalEntry?.nodes.map((node) => node.id)).toEqual([ + external, + marketplace, + ]); + expect(externalEntry?.nodes[0]?.role).toBe('dependency'); + expect(embedded?.marketplace?.nodeId).toBe(marketplace); + expect(externalEntry?.marketplace?.nodeId).toBe(marketplace); + } finally { + await rm(root, { recursive: true, force: true }); + } + }); + + test('classifies a plugin absent from a valid inspected marketplace as removed', async () => { + const root = await mkdtemp(join(tmpdir(), 'allagents-skill-update-removed-')); + try { + await mkdir(join(root, '.claude-plugin'), { recursive: true }); + await writeFile( + join(root, '.claude-plugin/marketplace.json'), + JSON.stringify({ + name: 'catalog', + description: 'test catalog', + plugins: [], + }), + ); + await initCheckout(root); + const sha = runGit(root, ['rev-parse', 'HEAD']); + const result = await inspectSkillUpdateUnit({ + id: root, + nodes: [ + { + id: root, + cachePath: root, + remoteUrl: root, + role: 'root', + currentSha: sha, + }, + ], + installations: [ + { + id: 'project:0', + scope: 'project', + configIndex: 0, + rawSource: 'gone@catalog', + effectiveSource: 'gone@catalog', + pluginName: 'gone', + rootNodeId: root, + rootSubpath: 'plugins/gone', + nodes: [], + skills: [{ name: 'gone', subpath: 'gone', enabled: true }], + marketplace: { nodeId: root, pluginName: 'gone' }, + }, + ], + }); + + expect(result.outcome).toBe('resolved'); + expect(result.installations).toEqual([ + { installationId: 'project:0', outcome: 'plugin-removed' }, + ]); + } finally { + await rm(root, { recursive: true, force: true }); + } + }); + + test('does not fetch an obsolete external dependency after its marketplace entry is removed', async () => { + const root = await mkdtemp(join(tmpdir(), 'allagents-skill-update-obsolete-')); + try { + await mkdir(join(root, '.claude-plugin'), { recursive: true }); + await writeFile( + join(root, '.claude-plugin/marketplace.json'), + JSON.stringify({ name: 'catalog', description: 'test', plugins: [] }), + ); + await initCheckout(root); + const sha = runGit(root, ['rev-parse', 'HEAD']); + const missing = join(root, 'missing-dependency.git'); + const result = await inspectSkillUpdateUnit({ + id: root, + nodes: [ + { + id: missing, + cachePath: missing, + remoteUrl: missing, + role: 'dependency', + currentSha: 'old', + }, + { + id: root, + cachePath: root, + remoteUrl: root, + role: 'root', + currentSha: sha, + }, + ], + installations: [ + { + id: 'project:0', + scope: 'project', + configIndex: 0, + rawSource: 'gone@catalog', + effectiveSource: 'gone@catalog', + pluginName: 'gone', + rootNodeId: missing, + rootSubpath: '', + nodes: [], + skills: [{ name: 'gone', subpath: 'gone', enabled: true }], + marketplace: { nodeId: root, pluginName: 'gone' }, + }, + ], + }); + + expect(result.outcome).toBe('resolved'); + expect(result.nodes).toEqual([{ nodeId: root, sha }]); + expect(result.installations[0]?.outcome).toBe('plugin-removed'); + } finally { + await rm(root, { recursive: true, force: true }); + } + }); + + test('records a selected inventory failure while preserving healthy independent units', async () => { + const root = await mkdtemp(join(tmpdir(), 'allagents-skill-update-failures-')); + const home = join(root, 'home'); + const workspace = join(root, 'workspace'); + process.env.ALLAGENTS_TEST_HOME = home; + try { + const healthy = getPluginCachePath('acme', 'healthy-skills'); + await mkdir(join(healthy, 'skills/keep'), { recursive: true }); + await writeFile( + join(healthy, 'skills/keep/SKILL.md'), + '---\nname: keep\ndescription: keep\n---\n', + ); + await initCheckout(healthy); + await mkdir(join(workspace, '.allagents'), { recursive: true }); + await writeFile( + join(workspace, '.allagents/workspace.yaml'), + dump({ + version: 2, + repositories: [], + clients: ['copilot'], + plugins: ['acme/healthy-skills', 'acme/missing-skills'], + }), + ); + + const inventory = await buildSkillUpdateInventory(workspace, ['project']); + + expect(inventory.installations.map((entry) => entry.rawSource)).toEqual([ + 'acme/healthy-skills', + ]); + expect(inventory.failures).toHaveLength(1); + expect(inventory.failures[0]).toMatchObject({ + scope: 'project', + source: 'acme/missing-skills', + }); + } finally { + await rm(root, { recursive: true, force: true }); + } + }); + + test('rejects a remote marketplace registry path outside the managed cache', async () => { + const root = await mkdtemp(join(tmpdir(), 'allagents-skill-update-registry-')); + const home = join(root, 'home'); + const workspace = join(root, 'workspace'); + const outside = join(root, 'outside-marketplace'); + process.env.ALLAGENTS_TEST_HOME = home; + try { + await mkdir(join(outside, '.claude-plugin'), { recursive: true }); + await writeFile( + join(outside, '.claude-plugin/marketplace.json'), + JSON.stringify({ name: 'catalog', description: 'test', plugins: [] }), + ); + await initCheckout(outside); + await mkdir(join(home, '.allagents'), { recursive: true }); + await mkdir(join(workspace, '.allagents'), { recursive: true }); + await writeFile( + join(home, '.allagents/marketplaces.json'), + JSON.stringify({ + version: 1, + marketplaces: { + catalog: { + name: 'catalog', + source: { type: 'github', location: 'acme/catalog' }, + path: outside, + }, + }, + }), + ); + await writeFile( + join(workspace, '.allagents/workspace.yaml'), + dump({ + version: 2, + repositories: [], + clients: ['copilot'], + plugins: ['gone@catalog'], + }), + ); + + const inventory = await buildSkillUpdateInventory(workspace, ['project']); + + expect(inventory.installations).toEqual([]); + expect(inventory.failures[0]?.error).toContain('managed cache'); + } finally { + await rm(root, { recursive: true, force: true }); + } + }); + + test('verifies origin before resetting and falls back to the inspected SHA when the ref is gone', async () => { + const root = await mkdtemp(join(tmpdir(), 'allagents-skill-update-move-')); + const remote = join(root, 'remote.git'); + const work = join(root, 'work'); + const cache = join(root, 'cache'); + try { + await mkdir(remote, { recursive: true }); + runGit(remote, ['init', '--bare', '--initial-branch=main']); + runGit(root, ['clone', remote, work]); + runGit(work, ['config', '--local', 'user.name', 'Skill Update Test']); + runGit(work, [ + 'config', + '--local', + 'user.email', + 'skill-update@example.test', + ]); + await writeFile(join(work, 'SKILL.md'), 'first'); + runGit(work, ['add', '.']); + runGit(work, ['commit', '-m', 'first']); + runGit(work, ['push', 'origin', 'main']); + const sha = runGit(work, ['rev-parse', 'HEAD']); + runGit(root, ['clone', remote, cache]); + + await expect( + moveSkillUpdateCheckout( + { + id: cache, + cachePath: cache, + remoteUrl: join(root, 'different.git'), + ref: 'deleted-ref', + role: 'root', + currentSha: sha, + }, + sha, + ), + ).rejects.toThrow('origin'); + + await moveSkillUpdateCheckout( + { + id: cache, + cachePath: cache, + remoteUrl: remote, + ref: 'deleted-ref', + role: 'root', + currentSha: sha, + }, + sha, + ); + expect(runGit(cache, ['rev-parse', 'HEAD'])).toBe(sha); + } finally { + await rm(root, { recursive: true, force: true }); + } + }); + + test('ignores an unrelated broken user cache during a project-only inventory', async () => { + const root = await mkdtemp(join(tmpdir(), 'allagents-skill-update-scope-')); + const home = join(root, 'home'); + const workspace = join(root, 'workspace'); + process.env.ALLAGENTS_TEST_HOME = home; + try { + const projectCache = getPluginCachePath('acme', 'project-skills'); + await mkdir(join(projectCache, 'skills/keep'), { recursive: true }); + await writeFile( + join(projectCache, 'skills/keep/SKILL.md'), + '---\nname: keep\ndescription: keep\n---\n', + ); + await initCheckout(projectCache); + await mkdir(join(workspace, '.allagents'), { recursive: true }); + await mkdir(join(home, '.allagents'), { recursive: true }); + await writeFile( + join(workspace, '.allagents/workspace.yaml'), + dump({ + version: 2, + repositories: [], + clients: ['copilot'], + plugins: ['acme/project-skills'], + }), + ); + await writeFile( + join(home, '.allagents/workspace.yaml'), + dump({ + version: 2, + repositories: [], + clients: ['codex'], + plugins: ['unrelated/missing-skills'], + }), + ); + + const inventory = await buildSkillUpdateInventory(workspace, [ + 'project', + ]); + + expect(inventory.installations).toHaveLength(1); + expect(inventory.installations[0]?.scope).toBe('project'); + } finally { + await rm(root, { recursive: true, force: true }); + } + }); + + test('inventories a direct GitHub source with an inline ref instead of treating it as a marketplace spec', async () => { + const root = await mkdtemp(join(tmpdir(), 'allagents-skill-update-ref-')); + const home = join(root, 'home'); + const workspace = join(root, 'workspace'); + process.env.ALLAGENTS_TEST_HOME = home; + try { + const cache = getPluginCachePath('acme', 'ref-skills', 'v2'); + await mkdir(join(cache, 'skills/keep'), { recursive: true }); + await writeFile( + join(cache, 'skills/keep/SKILL.md'), + '---\nname: keep\ndescription: keep\n---\n', + ); + await initCheckout(cache); + await mkdir(join(workspace, '.allagents'), { recursive: true }); + await writeFile( + join(workspace, '.allagents/workspace.yaml'), + dump({ + version: 2, + repositories: [], + clients: ['copilot'], + plugins: ['acme/ref-skills@v2'], + }), + ); + + const inventory = await buildSkillUpdateInventory(workspace, [ + 'project', + ]); + + expect(inventory.installations).toHaveLength(1); + expect(inventory.installations[0]?.rawSource).toBe( + 'acme/ref-skills@v2', + ); + expect(inventory.installations[0]?.nodes[0]?.ref).toBe('v2'); + expect(inventory.installations[0]?.marketplace).toBeUndefined(); + } finally { + await rm(root, { recursive: true, force: true }); + } + }); +}); diff --git a/tests/unit/cli/skill-update-reconciliation.test.ts b/tests/unit/cli/skill-update-reconciliation.test.ts new file mode 100644 index 00000000..3e69e368 --- /dev/null +++ b/tests/unit/cli/skill-update-reconciliation.test.ts @@ -0,0 +1,579 @@ +import { describe, it, expect, beforeEach, afterEach } from 'bun:test'; +import { mkdtemp, mkdir, readFile, rm, writeFile } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { dump, load } from 'js-yaml'; +import { createSkillUpdateReconciler } from '../../../src/cli/skill-update-reconciliation.js'; +import type { + CheckoutNode, + SkillUpdateInstallation, + SkillUpdateUnit, +} from '../../../src/core/skill-update.js'; +import { executeSkillUpdatePlan } from '../../../src/core/skill-update.js'; +import type { WorkspaceConfig } from '../../../src/models/workspace-config.js'; + +describe('createSkillUpdateReconciler', () => { + let tmpDir: string; + let userConfigPath: string; + + beforeEach(async () => { + tmpDir = await mkdtemp(join(tmpdir(), 'allagents-skill-reconcile-')); + userConfigPath = join(tmpDir, 'home/.allagents/workspace.yaml'); + await mkdir(join(tmpDir, '.allagents'), { recursive: true }); + await mkdir(join(tmpDir, 'home/.allagents'), { recursive: true }); + }); + + afterEach(async () => { + await rm(tmpDir, { recursive: true, force: true }); + }); + + function installation( + overrides: Partial & + Pick, + ): SkillUpdateInstallation { + return { + effectiveSource: overrides.rawSource, + pluginName: 'skills', + rootNodeId: 'cache', + rootSubpath: '', + nodes: [], + skills: [], + ...overrides, + }; + } + + function unit( + installations: SkillUpdateInstallation[], + deletedByInstallation: Record< + string, + Array<{ name: string; subpath: string; selector?: string }> + >, + removedInstallationIds: string[] = [], + survivorsByInstallation: Record< + string, + Array<{ name: string; subpath: string; selector?: string }> + > = {}, + ): SkillUpdateUnit { + return { + id: 'cache', + nodes: [], + installations, + outcome: removedInstallationIds.length > 0 ? 'plugin-removed' : 'resolved', + inspectedNodes: [], + deleted: installations.flatMap((entry) => + (deletedByInstallation[entry.id] ?? []).map((skill) => ({ + ...skill, + enabled: true, + installationId: entry.id, + scope: entry.scope, + pluginName: entry.pluginName, + source: entry.rawSource, + })), + ), + survivors: installations.flatMap((entry) => + (survivorsByInstallation[entry.id] ?? []).map((skill) => ({ + ...skill, + enabled: true, + installationId: entry.id, + scope: entry.scope, + pluginName: entry.pluginName, + source: entry.rawSource, + })), + ), + blockedByOutOfScope: false, + removedInstallationIds, + }; + } + + it('batch-prunes exact qualified selectors and preserves object fields', async () => { + const source = 'https://github.com/acme/skills'; + const config: WorkspaceConfig = { + version: 2, + repositories: [], + clients: ['copilot'], + plugins: [ + { + source, + clients: ['copilot'], + install: 'file', + exclude: ['commands/**'], + pin: 'v2', + skills: ['nested/one/review', 'nested/two/review', 'keep'], + }, + ], + }; + await writeFile(join(tmpDir, '.allagents/workspace.yaml'), dump(config), 'utf-8'); + + const entry = installation({ + id: 'project:0', + scope: 'project', + configIndex: 0, + rawSource: source, + }); + const prepared = await createSkillUpdateReconciler({ + workspacePath: tmpDir, + userConfigPath, + })( + unit([entry], { + [entry.id]: [ + { name: 'review', subpath: 'nested/one/review', selector: 'nested/one/review' }, + { name: 'keep', subpath: 'keep', selector: 'keep' }, + ], + }), + ); + + // Preparing the transaction must not alter the live config. + expect(load(await readFile(join(tmpDir, '.allagents/workspace.yaml'), 'utf-8'))).toEqual(config); + await prepared.commit(); + + const updated = load( + await readFile(join(tmpDir, '.allagents/workspace.yaml'), 'utf-8'), + ) as WorkspaceConfig; + expect(updated.plugins).toEqual([ + { + source, + clients: ['copilot'], + install: 'file', + exclude: ['commands/**'], + pin: 'v2', + skills: ['nested/two/review'], + }, + ]); + }); + + it('expands a shared bare allowlist selector to surviving qualified paths', async () => { + const source = 'https://github.com/acme/skills'; + const config: WorkspaceConfig = { + version: 2, + repositories: [], + clients: ['copilot'], + plugins: [ + { + source, + clients: ['copilot'], + skills: ['before', 'review', 'nested/two/review', 'after'], + }, + ], + }; + await writeFile(join(tmpDir, '.allagents/workspace.yaml'), dump(config), 'utf-8'); + const entry = installation({ + id: 'project:0', + scope: 'project', + configIndex: 0, + rawSource: source, + }); + + const prepared = await createSkillUpdateReconciler({ + workspacePath: tmpDir, + userConfigPath, + })( + unit( + [entry], + { + [entry.id]: [ + { name: 'review', subpath: 'nested/one/review', selector: 'review' }, + ], + }, + [], + { + [entry.id]: [ + { name: 'review', subpath: 'nested/two/review', selector: 'review' }, + { name: 'review', subpath: 'nested/three/review', selector: 'review' }, + ], + }, + ), + ); + expect( + load( + await readFile(join(tmpDir, '.allagents/workspace.yaml'), 'utf-8'), + ), + ).toEqual(config); + await prepared.commit(); + + const updated = load( + await readFile(join(tmpDir, '.allagents/workspace.yaml'), 'utf-8'), + ) as WorkspaceConfig; + expect(updated.plugins).toEqual([ + { + source, + clients: ['copilot'], + skills: [ + 'before', + 'nested/two/review', + 'nested/three/review', + 'after', + ], + }, + ]); + }); + + it('reconciles legacy top-level selectors for confirmed deletions', async () => { + const source = 'https://github.com/acme/skills'; + const config: WorkspaceConfig = { + version: 1, + repositories: [], + clients: ['copilot'], + plugins: [{ source, clients: ['copilot'] }], + enabledSkills: [ + 'skills:before', + 'skills:review', + 'skills:nested/two/review', + 'skills:gone', + 'other:gone', + ], + disabledSkills: [ + 'skills:review', + 'skills:nested/two/review', + 'skills:gone', + 'other:gone', + ], + }; + await writeFile(join(tmpDir, '.allagents/workspace.yaml'), dump(config), 'utf-8'); + const entry = installation({ + id: 'project:0', + scope: 'project', + configIndex: 0, + rawSource: source, + pluginName: 'skills', + }); + + const prepared = await createSkillUpdateReconciler({ + workspacePath: tmpDir, + userConfigPath, + })( + unit( + [entry], + { + [entry.id]: [ + { name: 'review', subpath: 'nested/one/review', selector: 'review' }, + { name: 'gone', subpath: 'gone', selector: 'gone' }, + ], + }, + [], + { + [entry.id]: [ + { name: 'review', subpath: 'nested/two/review', selector: 'review' }, + { name: 'review', subpath: 'nested/three/review', selector: 'review' }, + ], + }, + ), + ); + expect( + load( + await readFile(join(tmpDir, '.allagents/workspace.yaml'), 'utf-8'), + ), + ).toEqual(config); + await prepared.commit(); + + const updated = load( + await readFile(join(tmpDir, '.allagents/workspace.yaml'), 'utf-8'), + ) as WorkspaceConfig; + expect(updated.enabledSkills).toEqual([ + 'skills:before', + 'skills:nested/two/review', + 'skills:nested/three/review', + 'other:gone', + ]); + expect(updated.disabledSkills).toEqual([ + 'skills:nested/two/review', + 'skills:nested/three/review', + 'other:gone', + ]); + expect(updated.plugins).toEqual([{ source, clients: ['copilot'] }]); + }); + + it('keeps an empty allowlist for non-skill artifacts but removes a standalone source', async () => { + const sharedSource = 'https://github.com/acme/shared'; + const standaloneSource = 'https://github.com/acme/solo/tree/main/skill'; + const config: WorkspaceConfig = { + repositories: [], + clients: ['copilot'], + plugins: [ + { source: sharedSource, exclude: ['hooks/**'], skills: ['gone'] }, + { source: standaloneSource, skills: ['solo'] }, + ], + }; + await writeFile(join(tmpDir, '.allagents/workspace.yaml'), dump(config), 'utf-8'); + const shared = installation({ + id: 'project:0', + scope: 'project', + configIndex: 0, + rawSource: sharedSource, + }); + const standalone = installation({ + id: 'project:1', + scope: 'project', + configIndex: 1, + rawSource: standaloneSource, + standaloneSkillSource: true, + }); + + const prepared = await createSkillUpdateReconciler({ + workspacePath: tmpDir, + userConfigPath, + })( + unit([shared, standalone], { + [shared.id]: [{ name: 'gone', subpath: 'gone', selector: 'gone' }], + [standalone.id]: [{ name: 'solo', subpath: 'solo', selector: 'solo' }], + }), + ); + await prepared.commit(); + + const updated = load( + await readFile(join(tmpDir, '.allagents/workspace.yaml'), 'utf-8'), + ) as WorkspaceConfig; + expect(updated.plugins).toEqual([ + { source: sharedSource, exclude: ['hooks/**'], skills: [] }, + ]); + }); + + it('removes a standalone raw-string source before the string-entry early return', async () => { + const standaloneSource = 'https://github.com/acme/solo/tree/main/skill'; + const config: WorkspaceConfig = { + repositories: [], + clients: ['copilot'], + plugins: [standaloneSource], + }; + await writeFile(join(tmpDir, '.allagents/workspace.yaml'), dump(config), 'utf-8'); + const standalone = installation({ + id: 'project:0', + scope: 'project', + configIndex: 0, + rawSource: standaloneSource, + standaloneSkillSource: true, + }); + + const prepared = await createSkillUpdateReconciler({ + workspacePath: tmpDir, + userConfigPath, + })( + unit([standalone], { + [standalone.id]: [ + { name: 'solo', subpath: 'solo', selector: 'solo' }, + ], + }), + ); + await prepared.commit(); + + const updated = load( + await readFile(join(tmpDir, '.allagents/workspace.yaml'), 'utf-8'), + ) as WorkspaceConfig; + expect(updated.plugins).toEqual([]); + }); + + it('does not add stale exclusions for implicit or blocklist entries', async () => { + const implicitSource = 'https://github.com/acme/implicit'; + const blocklistSource = 'https://github.com/acme/blocklist'; + const config: WorkspaceConfig = { + repositories: [], + clients: ['copilot'], + plugins: [ + implicitSource, + { source: blocklistSource, skills: { exclude: ['already-disabled'] } }, + ], + }; + await writeFile(join(tmpDir, '.allagents/workspace.yaml'), dump(config), 'utf-8'); + const implicit = installation({ + id: 'project:0', + scope: 'project', + configIndex: 0, + rawSource: implicitSource, + }); + const blocklist = installation({ + id: 'project:1', + scope: 'project', + configIndex: 1, + rawSource: blocklistSource, + }); + + const prepared = await createSkillUpdateReconciler({ + workspacePath: tmpDir, + userConfigPath, + })( + unit([implicit, blocklist], { + [implicit.id]: [{ name: 'gone', subpath: 'gone' }], + [blocklist.id]: [{ name: 'gone', subpath: 'gone' }], + }), + ); + await prepared.commit(); + + expect( + load(await readFile(join(tmpDir, '.allagents/workspace.yaml'), 'utf-8')), + ).toEqual(config); + }); + + it('removes only installations authoritatively removed from a marketplace', async () => { + const removedSource = 'gone@marketplace'; + const retainedSource = 'keep@marketplace'; + const config: WorkspaceConfig = { + repositories: [], + clients: ['copilot'], + plugins: [ + { source: removedSource, clients: ['copilot'], skills: ['gone'] }, + { source: retainedSource, skills: ['keep'] }, + ], + disabledSkills: ['gone:old', 'keep:old'], + }; + await writeFile(join(tmpDir, '.allagents/workspace.yaml'), dump(config), 'utf-8'); + const removed = installation({ + id: 'project:0', + scope: 'project', + configIndex: 0, + rawSource: removedSource, + }); + const retained = installation({ + id: 'project:1', + scope: 'project', + configIndex: 1, + rawSource: retainedSource, + }); + + const prepared = await createSkillUpdateReconciler({ + workspacePath: tmpDir, + userConfigPath, + })( + unit( + [removed, retained], + { [removed.id]: [{ name: 'gone', subpath: 'gone' }] }, + [removed.id], + ), + ); + await prepared.commit(); + + const updated = load( + await readFile(join(tmpDir, '.allagents/workspace.yaml'), 'utf-8'), + ) as WorkspaceConfig; + expect(updated.plugins).toEqual([{ source: retainedSource, skills: ['keep'] }]); + expect(updated.disabledSkills).toEqual(['keep:old']); + }); + + it('restores every scope when the second config replacement fails', async () => { + const projectSource = 'https://github.com/acme/project'; + const userSource = 'https://github.com/acme/user'; + const projectConfig: WorkspaceConfig = { + repositories: [], + clients: ['copilot'], + plugins: [{ source: projectSource, skills: ['gone'] }], + }; + const userConfig: WorkspaceConfig = { + repositories: [], + clients: ['codex'], + plugins: [{ source: userSource, skills: ['gone'] }], + }; + const projectConfigPath = join(tmpDir, '.allagents/workspace.yaml'); + await writeFile(projectConfigPath, dump(projectConfig), 'utf-8'); + await writeFile(userConfigPath, dump(userConfig), 'utf-8'); + const project = installation({ + id: 'project:0', + scope: 'project', + configIndex: 0, + rawSource: projectSource, + }); + const user = installation({ + id: 'user:0', + scope: 'user', + configIndex: 0, + rawSource: userSource, + }); + let liveReplacement = 0; + + const prepared = await createSkillUpdateReconciler({ + workspacePath: tmpDir, + userConfigPath, + beforeReplace: (path) => { + if (!path.endsWith('workspace.yaml')) return; + liveReplacement++; + if (liveReplacement === 2) throw new Error('injected second replacement failure'); + }, + })( + unit([project, user], { + [project.id]: [{ name: 'gone', subpath: 'gone', selector: 'gone' }], + [user.id]: [{ name: 'gone', subpath: 'gone', selector: 'gone' }], + }), + ); + + await expect(prepared.commit()).rejects.toThrow('injected second replacement failure'); + await prepared.rollback(); + expect(await readFile(projectConfigPath, 'utf-8')).toBe(dump(projectConfig)); + expect(await readFile(userConfigPath, 'utf-8')).toBe(dump(userConfig)); + }); + + it('integrates config failure with checkout rollback and skips sync', async () => { + const projectSource = 'https://github.com/acme/project'; + const userSource = 'https://github.com/acme/user'; + const projectConfig: WorkspaceConfig = { + repositories: [], + clients: ['copilot'], + plugins: [{ source: projectSource, skills: ['gone'] }], + }; + const userConfig: WorkspaceConfig = { + repositories: [], + clients: ['codex'], + plugins: [{ source: userSource, skills: ['gone'] }], + }; + const projectConfigPath = join(tmpDir, '.allagents/workspace.yaml'); + await writeFile(projectConfigPath, dump(projectConfig), 'utf-8'); + await writeFile(userConfigPath, dump(userConfig), 'utf-8'); + const project = installation({ + id: 'project:0', + scope: 'project', + configIndex: 0, + rawSource: projectSource, + }); + const user = installation({ + id: 'user:0', + scope: 'user', + configIndex: 0, + rawSource: userSource, + }); + const checkout: CheckoutNode = { + id: '/cache/acme', + cachePath: '/cache/acme', + remoteUrl: 'https://github.com/acme/shared.git', + role: 'root', + currentSha: 'old-sha', + }; + const updateUnit = { + ...unit([project, user], { + [project.id]: [{ name: 'gone', subpath: 'gone', selector: 'gone' }], + [user.id]: [{ name: 'gone', subpath: 'gone', selector: 'gone' }], + }), + nodes: [checkout], + inspectedNodes: [{ nodeId: checkout.id, sha: 'new-sha' }], + }; + let liveReplacement = 0; + const advanced: string[] = []; + const restored: string[] = []; + let syncCalls = 0; + + const result = await executeSkillUpdatePlan( + { selectedScopes: ['project', 'user'], units: [updateUnit] }, + { [updateUnit.id]: 'remove' }, + { + advanceNode: async (node, sha) => advanced.push(`${node.id}:${sha}`), + restoreNode: async (node, sha) => restored.push(`${node.id}:${sha}`), + reconcileUnit: createSkillUpdateReconciler({ + workspacePath: tmpDir, + userConfigPath, + beforeReplace: () => { + liveReplacement++; + if (liveReplacement === 2) { + throw new Error('injected second replacement failure'); + } + }, + }), + syncScope: async () => { + syncCalls++; + return { success: true }; + }, + }, + ); + + expect(result.success).toBe(false); + expect(advanced).toEqual(['/cache/acme:new-sha']); + expect(restored).toEqual(['/cache/acme:old-sha']); + expect(syncCalls).toBe(0); + expect(await readFile(projectConfigPath, 'utf-8')).toBe(dump(projectConfig)); + expect(await readFile(userConfigPath, 'utf-8')).toBe(dump(userConfig)); + }); +}); diff --git a/tests/unit/cli/terminal-output.test.ts b/tests/unit/cli/terminal-output.test.ts new file mode 100644 index 00000000..063b6be6 --- /dev/null +++ b/tests/unit/cli/terminal-output.test.ts @@ -0,0 +1,22 @@ +import { describe, expect, test } from 'bun:test'; +import { terminalSafe } from '../../../src/cli/terminal-output.js'; + +describe('terminalSafe', () => { + test('removes ANSI and terminal command sequences', () => { + expect( + terminalSafe( + 'source\u001b[31m-red\u001b[0m\u001b]2;forged title\u0007-end', + ), + ).toBe('source-red-end'); + }); + + test('makes line breaks and direction overrides visible', () => { + expect(terminalSafe('plugin\nforged\u202estatus')).toBe( + 'plugin�forged�status', + ); + }); + + test('accepts non-string error values', () => { + expect(terminalSafe(42)).toBe('42'); + }); +}); diff --git a/tests/unit/core/skill-update.test.ts b/tests/unit/core/skill-update.test.ts new file mode 100644 index 00000000..bec3e2a4 --- /dev/null +++ b/tests/unit/core/skill-update.test.ts @@ -0,0 +1,668 @@ +import { describe, expect, it, mock } from 'bun:test'; +import { mkdir, mkdtemp, rm, symlink } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { + buildPhysicalRefreshUnits, + buildSkillUpdatePreflight, + createGitHubSkillUpdateInstallation, + executeSkillUpdatePlan, + inspectRemoteSkillUpdateUnit, + resolveCheckoutSubpath, + type CheckoutNode, + type SkillUpdateInstallation, + type UnitInspection, +} from '../../../src/core/skill-update.js'; +import { getEffectivePluginSource } from '../../../src/models/workspace-config.js'; + +const projectNode: CheckoutNode = { + id: '/cache/acme-skills', + cachePath: '/cache/acme-skills', + remoteUrl: 'https://github.com/acme/skills.git', + role: 'root', + currentSha: 'old-sha', +}; + +function installation( + overrides: Partial = {}, +): SkillUpdateInstallation { + return { + id: 'project:0', + scope: 'project', + configIndex: 0, + rawSource: 'acme/skills', + effectiveSource: 'acme/skills', + pluginName: 'skills', + rootNodeId: projectNode.id, + rootSubpath: '', + nodes: [projectNode], + skills: [ + { name: 'keep', subpath: 'keep', enabled: true }, + { name: 'deleted', subpath: 'deleted', enabled: true }, + ], + ...overrides, + }; +} + +function resolved( + installationId: string, + subpaths: string[], +): UnitInspection { + return { + outcome: 'resolved', + nodes: [{ nodeId: projectNode.id, sha: 'new-sha' }], + installations: [ + { + installationId, + outcome: 'resolved', + skills: subpaths.map((subpath) => ({ + name: subpath.split('/').at(-1) ?? subpath, + subpath, + })), + }, + ], + }; +} + +describe('getEffectivePluginSource', () => { + it('applies object pins while preserving inline refs', () => { + expect( + getEffectivePluginSource({ source: 'acme/skills', pin: 'v2' }), + ).toBe('acme/skills@v2'); + expect( + getEffectivePluginSource({ source: 'acme/skills@v1', pin: 'v2' }), + ).toBe('acme/skills@v1'); + expect( + getEffectivePluginSource({ + source: 'https://github.com/acme/skills', + pin: 'v2', + }), + ).toBe('acme/skills@v2'); + expect( + getEffectivePluginSource({ source: './local/plugin', pin: 'v2' }), + ).toBe('./local/plugin'); + expect( + getEffectivePluginSource({ + source: 'plugin@acme/marketplace', + pin: 'v2', + }), + ).toBe('plugin@acme/marketplace'); + }); +}); + +describe('createGitHubSkillUpdateInstallation', () => { + it('canonicalizes aliases to one cache node and separates pinned refs', () => { + const shorthand = createGitHubSkillUpdateInstallation({ + scope: 'project', + configIndex: 0, + plugin: 'acme/skills', + pluginName: 'skills', + currentSha: 'old', + skills: [{ name: 'keep', subpath: 'keep', enabled: true }], + }); + const url = createGitHubSkillUpdateInstallation({ + scope: 'user', + configIndex: 0, + plugin: 'https://github.com/acme/skills', + pluginName: 'skills', + currentSha: 'old', + skills: [{ name: 'keep', subpath: 'keep', enabled: true }], + }); + const pinned = createGitHubSkillUpdateInstallation({ + scope: 'project', + configIndex: 1, + plugin: { source: 'acme/skills', pin: 'v2' }, + pluginName: 'skills', + currentSha: 'v2-old', + skills: [{ name: 'keep', subpath: 'keep', enabled: true }], + }); + + expect(shorthand?.nodes[0]?.id).toBe(url?.nodes[0]?.id); + expect(shorthand?.rootSubpath).toBe(''); + expect(pinned?.nodes[0]?.ref).toBe('v2'); + expect(pinned?.nodes[0]?.id).not.toBe(shorthand?.nodes[0]?.id); + }); + + it('groups direct-source siblings by physical checkout while keeping refs separate', () => { + const sibling = installation({ + id: 'project:1', + configIndex: 1, + rawSource: 'acme/skills/plugins/sibling', + effectiveSource: 'acme/skills/plugins/sibling', + rootSubpath: 'plugins/sibling', + }); + const pinnedNode: CheckoutNode = { + ...projectNode, + id: '/cache/acme-skills-v2', + cachePath: '/cache/acme-skills-v2', + ref: 'v2', + }; + const pinned = installation({ + id: 'project:2', + configIndex: 2, + rawSource: 'acme/skills@v2', + effectiveSource: 'acme/skills@v2', + rootNodeId: pinnedNode.id, + nodes: [pinnedNode], + }); + + const units = buildPhysicalRefreshUnits([ + installation(), + sibling, + pinned, + ]); + + expect(units).toHaveLength(2); + expect( + units.find((unit) => unit.id === projectNode.id)?.installations.map( + (entry) => entry.rawSource, + ), + ).toEqual(['acme/skills', 'acme/skills/plugins/sibling']); + }); +}); + +describe('buildSkillUpdatePreflight', () => { + it('preflights every sibling in a touched cache and compares qualified paths', async () => { + const inspectUnit = mock(async () => + resolved('project:0', ['keep', 'group-b/shared']), + ); + const result = await buildSkillUpdatePreflight( + { + installations: [ + installation({ + skills: [ + { name: 'keep', subpath: 'keep', enabled: true }, + { name: 'shared', subpath: 'group-a/shared', enabled: true }, + { name: 'shared', subpath: 'group-b/shared', enabled: true }, + ], + }), + ], + selectedScopes: ['project'], + filters: ['keep'], + }, + { inspectUnit }, + ); + + expect(inspectUnit).toHaveBeenCalledTimes(1); + expect(result.units).toHaveLength(1); + expect(result.units[0]?.deleted.map((skill) => skill.subpath)).toEqual([ + 'group-a/shared', + ]); + expect(result.units[0]?.survivors.map((skill) => skill.subpath)).toEqual([ + 'keep', + 'group-b/shared', + ]); + }); + + it('fails closed when discovery fails', async () => { + const result = await buildSkillUpdatePreflight( + { + installations: [installation()], + selectedScopes: ['project'], + }, + { + inspectUnit: async () => ({ + outcome: 'failed', + nodes: [], + installations: [], + error: 'manifest is malformed', + }), + }, + ); + + expect(result.units[0]?.outcome).toBe('failed'); + expect(result.units[0]?.deleted).toEqual([]); + expect(result.units[0]?.error).toContain('malformed'); + }); + + it('types inventory failures while continuing healthy independent units', async () => { + const inspectUnit = mock(async () => resolved('project:0', ['keep', 'deleted'])); + const result = await buildSkillUpdatePreflight( + { + installations: [installation()], + selectedScopes: ['project'], + failures: [ + { + id: 'inventory:project:1', + scope: 'project', + source: 'acme/broken', + nodeIds: ['/cache/broken'], + error: 'checkout is unreadable', + }, + ], + }, + { inspectUnit }, + ); + + expect(inspectUnit).toHaveBeenCalledTimes(1); + expect(result.units.map((unit) => [unit.id, unit.outcome])).toEqual([ + [projectNode.id, 'resolved'], + ['inventory:project:1', 'failed'], + ]); + }); + + it('fails a shared component closed without inspecting or mutating it', async () => { + const inspectUnit = mock(async () => resolved('project:0', ['keep'])); + const result = await buildSkillUpdatePreflight( + { + installations: [installation()], + selectedScopes: ['project'], + failures: [ + { + id: 'inventory:user:0', + scope: 'user', + source: 'acme/shared-broken', + nodeIds: [projectNode.id], + error: 'shared checkout inventory failed', + }, + ], + }, + { inspectUnit }, + ); + + expect(inspectUnit).not.toHaveBeenCalled(); + expect(result.units).toHaveLength(1); + expect(result.units[0]).toMatchObject({ + id: projectNode.id, + outcome: 'failed', + error: expect.stringContaining('shared checkout inventory failed'), + }); + }); + + it('blocks a selected-scope update when a shared-cache deletion affects another scope', async () => { + const user = installation({ + id: 'user:0', + scope: 'user', + configIndex: 0, + }); + const inspectUnit = async (): Promise => ({ + outcome: 'resolved', + nodes: [{ nodeId: projectNode.id, sha: 'new-sha' }], + installations: [ + { installationId: 'project:0', outcome: 'resolved', skills: [{ name: 'keep', subpath: 'keep' }] }, + { installationId: 'user:0', outcome: 'resolved', skills: [{ name: 'keep', subpath: 'keep' }] }, + ], + }); + + const result = await buildSkillUpdatePreflight( + { + installations: [installation(), user], + selectedScopes: ['project'], + }, + { inspectUnit }, + ); + + expect(result.units[0]?.blockedByOutOfScope).toBe(true); + expect(result.units[0]?.deleted).toHaveLength(2); + }); + + it('blocks every update when the physical checkout has any out-of-scope consumer', async () => { + const user = installation({ + id: 'user:0', + scope: 'user', + configIndex: 0, + skills: [{ name: 'keep', subpath: 'keep', enabled: true }], + }); + const result = await buildSkillUpdatePreflight( + { + installations: [ + installation({ + skills: [{ name: 'keep', subpath: 'keep', enabled: true }], + }), + user, + ], + selectedScopes: ['project'], + }, + { + inspectUnit: async () => ({ + outcome: 'resolved', + nodes: [{ nodeId: projectNode.id, sha: 'new-sha' }], + installations: [ + { + installationId: 'project:0', + outcome: 'resolved', + skills: [{ name: 'keep', subpath: 'keep' }], + }, + { + installationId: 'user:0', + outcome: 'resolved', + skills: [{ name: 'keep', subpath: 'keep' }], + }, + ], + }), + }, + ); + + expect(result.units[0]?.deleted).toEqual([]); + expect(result.units[0]?.blockedByOutOfScope).toBe(true); + + const advanceNode = mock(async () => {}); + const executionResult = await executeSkillUpdatePlan(result, {}, { + advanceNode, + restoreNode: async () => {}, + reconcileUnit: async () => ({ commit: async () => {}, rollback: async () => {} }), + syncScope: async () => ({ success: true }), + }); + expect(advanceNode).not.toHaveBeenCalled(); + expect(executionResult.units[0]?.status).toBe('skipped'); + }); + + it('retains the exact installation IDs authoritatively removed by a marketplace', async () => { + const retained = installation({ + id: 'project:1', + configIndex: 1, + rawSource: 'keep@marketplace', + effectiveSource: 'keep@marketplace', + skills: [{ name: 'keep', subpath: 'keep', enabled: true }], + }); + const result = await buildSkillUpdatePreflight( + { + installations: [installation(), retained], + selectedScopes: ['project'], + }, + { + inspectUnit: async () => ({ + outcome: 'resolved', + nodes: [{ nodeId: projectNode.id, sha: 'new-sha' }], + installations: [ + { installationId: 'project:0', outcome: 'plugin-removed' }, + { + installationId: 'project:1', + outcome: 'resolved', + skills: [{ name: 'keep', subpath: 'keep' }], + }, + ], + }), + }, + ); + + expect(result.units[0]?.removedInstallationIds).toEqual(['project:0']); + expect(result.units[0]?.deleted.map((skill) => skill.installationId)).toEqual([ + 'project:0', + 'project:0', + ]); + expect(result.units[0]?.survivors.map((skill) => skill.installationId)).toEqual([ + 'project:1', + ]); + }); +}); + +describe('inspectRemoteSkillUpdateUnit', () => { + it('treats a valid empty plugin root as authoritative and always cleans up', async () => { + const cleanup = mock(async () => {}); + const result = await inspectRemoteSkillUpdateUnit( + { + id: projectNode.id, + nodes: [projectNode], + installations: [installation()], + }, + { + cloneNode: async () => '/tmp/inspected', + getRevision: async () => 'inspected-sha', + pathExists: () => true, + discoverPluginSkills: async () => [], + cleanup, + }, + ); + + expect(result).toEqual({ + outcome: 'resolved', + nodes: [{ nodeId: projectNode.id, sha: 'inspected-sha' }], + installations: [ + { installationId: 'project:0', outcome: 'resolved', skills: [] }, + ], + }); + expect(cleanup).toHaveBeenCalledWith('/tmp/inspected'); + }); + + it('rejects parent, absolute, and symlink subpaths outside the checkout', async () => { + const root = await mkdtemp(join(tmpdir(), 'allagents-skill-path-')); + const checkout = join(root, 'checkout'); + const outside = join(root, 'outside'); + await mkdir(checkout); + await mkdir(outside); + await symlink(outside, join(checkout, 'escaped-link')); + try { + expect(() => resolveCheckoutSubpath(checkout, '../outside')).toThrow( + 'outside its checkout', + ); + expect(() => resolveCheckoutSubpath(checkout, outside)).toThrow( + 'absolute', + ); + expect(() => resolveCheckoutSubpath(checkout, 'escaped-link')).toThrow( + 'outside its checkout', + ); + } finally { + await rm(root, { recursive: true, force: true }); + } + }); + + it('classifies a missing declared root as failure, never deletion', async () => { + const cleanup = mock(async () => {}); + const result = await inspectRemoteSkillUpdateUnit( + { + id: projectNode.id, + nodes: [projectNode], + installations: [installation({ rootSubpath: 'plugins/missing' })], + }, + { + cloneNode: async () => '/tmp/inspected', + getRevision: async () => 'inspected-sha', + pathExists: () => false, + discoverPluginSkills: async () => [], + cleanup, + }, + ); + + expect(result.outcome).toBe('failed'); + expect(result.installations).toEqual([]); + expect(result.error).toContain('declared root'); + expect(cleanup).toHaveBeenCalledTimes(1); + }); +}); + +describe('executeSkillUpdatePlan', () => { + it('collects decisions before mutation, leaves retained units untouched, and syncs offline once', async () => { + const secondNode: CheckoutNode = { + id: '/cache/healthy', + cachePath: '/cache/healthy', + remoteUrl: 'https://github.com/acme/healthy.git', + role: 'root', + currentSha: 'healthy-old', + }; + const plan = await buildSkillUpdatePreflight( + { + installations: [ + installation(), + installation({ + id: 'project:1', + configIndex: 1, + rawSource: 'acme/healthy', + effectiveSource: 'acme/healthy', + nodes: [secondNode], + rootNodeId: secondNode.id, + skills: [{ name: 'healthy', subpath: 'healthy', enabled: true }], + }), + ], + selectedScopes: ['project'], + }, + { + inspectUnit: async (unit) => { + if (unit.nodes[0]?.id === projectNode.id) { + return resolved('project:0', ['keep']); + } + return { + outcome: 'resolved', + nodes: [{ nodeId: secondNode.id, sha: 'healthy-new' }], + installations: [ + { + installationId: 'project:1', + outcome: 'resolved', + skills: [{ name: 'healthy', subpath: 'healthy' }], + }, + ], + }; + }, + }, + ); + const advanced: string[] = []; + const syncScope = mock(async () => ({ success: true })); + + const result = await executeSkillUpdatePlan( + plan, + { [projectNode.id]: 'retain' }, + { + advanceNode: async (node, sha) => advanced.push(`${node.id}:${sha}`), + restoreNode: async () => {}, + reconcileUnit: async () => ({ commit: async () => {}, rollback: async () => {} }), + syncScope, + }, + ); + + expect(advanced).toEqual(['/cache/healthy:healthy-new']); + expect(syncScope).toHaveBeenCalledTimes(1); + expect(syncScope).toHaveBeenCalledWith('project', { offline: true }); + expect(result.units.find((unit) => unit.id === projectNode.id)?.status).toBe( + 'retained', + ); + }); + + it('cancels before any mutation when a decision is cancelled', async () => { + const plan = await buildSkillUpdatePreflight( + { installations: [installation()], selectedScopes: ['project'] }, + { inspectUnit: async () => resolved('project:0', ['keep']) }, + ); + const advanceNode = mock(async () => {}); + const reconcileUnit = mock(async () => ({ + commit: async () => {}, + rollback: async () => {}, + })); + + const result = await executeSkillUpdatePlan( + plan, + { [projectNode.id]: 'cancel' }, + { + advanceNode, + restoreNode: async () => {}, + reconcileUnit, + syncScope: async () => ({ success: true }), + }, + ); + + expect(result.cancelled).toBe(true); + expect(result.success).toBe(false); + expect(result.units.every((unit) => unit.status === 'cancelled')).toBe(true); + expect(advanceNode).not.toHaveBeenCalled(); + expect(reconcileUnit).not.toHaveBeenCalled(); + }); + + it('restores already advanced dependency nodes when a later node fails', async () => { + const dependency: CheckoutNode = { + id: '/cache/dependency', + cachePath: '/cache/dependency', + remoteUrl: 'https://github.com/acme/dependency.git', + role: 'dependency', + currentSha: 'dependency-old', + }; + const root: CheckoutNode = { + ...projectNode, + currentSha: 'root-old', + }; + const plan = await buildSkillUpdatePreflight( + { + installations: [installation({ nodes: [root, dependency] })], + selectedScopes: ['project'], + }, + { + inspectUnit: async () => ({ + outcome: 'resolved', + nodes: [ + { nodeId: dependency.id, sha: 'dependency-new' }, + { nodeId: root.id, sha: 'root-new' }, + ], + installations: [ + { + installationId: 'project:0', + outcome: 'resolved', + skills: [ + { name: 'keep', subpath: 'keep' }, + { name: 'deleted', subpath: 'deleted' }, + ], + }, + ], + }), + }, + ); + const advanced: string[] = []; + const restored: string[] = []; + const rollback = mock(async () => {}); + const syncScope = mock(async () => ({ success: true })); + + const result = await executeSkillUpdatePlan(plan, {}, { + advanceNode: async (node) => { + advanced.push(node.id); + if (node.role === 'root') throw new Error('root checkout failed'); + }, + restoreNode: async (node, sha) => restored.push(`${node.id}:${sha}`), + reconcileUnit: async () => ({ commit: async () => {}, rollback }), + syncScope, + }); + + expect(advanced).toEqual([dependency.id, root.id]); + expect(restored).toEqual([ + `${root.id}:root-old`, + `${dependency.id}:dependency-old`, + ]); + expect(rollback).toHaveBeenCalledTimes(1); + expect(syncScope).not.toHaveBeenCalled(); + expect(result.success).toBe(false); + }); + + it('reports reconciliation and checkout rollback failures', async () => { + const plan = await buildSkillUpdatePreflight( + { installations: [installation()], selectedScopes: ['project'] }, + { inspectUnit: async () => resolved('project:0', ['keep', 'deleted']) }, + ); + const result = await executeSkillUpdatePlan(plan, {}, { + advanceNode: async () => { + throw new Error('advance failed after reset'); + }, + restoreNode: async () => { + throw new Error('restore failed'); + }, + reconcileUnit: async () => ({ + commit: async () => {}, + rollback: async () => { + throw new Error('config rollback failed'); + }, + }), + syncScope: async () => ({ success: true }), + }); + + expect(result.units[0]?.error).toContain('advance failed after reset'); + expect(result.units[0]?.error).toContain('config rollback failed'); + expect(result.units[0]?.error).toContain('restore failed'); + }); + + it('turns a rejected scope sync into a failed partial result', async () => { + const plan = await buildSkillUpdatePreflight( + { installations: [installation()], selectedScopes: ['project'] }, + { inspectUnit: async () => resolved('project:0', ['keep', 'deleted']) }, + ); + const result = await executeSkillUpdatePlan(plan, {}, { + advanceNode: async () => {}, + restoreNode: async () => {}, + reconcileUnit: async () => ({ commit: async () => {}, rollback: async () => {} }), + syncScope: async () => { + throw new Error('sync exploded'); + }, + }); + + expect(result.success).toBe(false); + expect(result.units.at(-1)).toMatchObject({ + id: 'sync:project', + status: 'failed', + error: 'sync exploded', + }); + }); +});