From 8840e4dab4cc130be90b316c9e56a61321d349d8 Mon Sep 17 00:00:00 2001 From: Julian Coy Date: Mon, 20 Jul 2026 21:51:54 -0400 Subject: [PATCH 1/2] Replace positional adapter asset contract with tagged request/result unions and atomic skill-bundle helpers --- .changeset/tagged-adapter-asset-contract.md | 18 + docs/cli/adapters/install.mdx | 2 +- docs/cli/adapters/list.mdx | 5 +- docs/guides/custom-adapters.mdx | 128 +++-- docs/guides/troubleshooting.mdx | 2 +- packages/adapter/src/__tests__/index.test.ts | 80 ++- .../src/__tests__/skill-bundle.test.ts | 420 ++++++++++++++++ packages/adapter/src/api-version.ts | 12 +- packages/adapter/src/asset-fs.ts | 122 ++++- packages/adapter/src/define-adapter.ts | 24 +- packages/adapter/src/index.ts | 19 +- packages/adapter/src/skill-bundle.ts | 457 ++++++++++++++++++ packages/adapter/src/types.ts | 197 +++++++- .../claude-code/src/__tests__/adapter.test.ts | 234 +++++++-- packages/adapters/claude-code/src/index.ts | 99 ++-- .../codex/src/__tests__/adapter.test.ts | 223 ++++++--- packages/adapters/codex/src/index.ts | 185 ++++--- .../opencode/src/__tests__/adapter.test.ts | 241 ++++++--- packages/adapters/opencode/src/index.ts | 101 ++-- .../__tests__/adapter-install-cli.e2e.test.ts | 3 +- .../src/commands/add/__tests__/add.test.ts | 26 +- .../install/__tests__/install-cli.test.ts | 26 +- .../__tests__/adapter-install-errors.test.ts | 12 +- .../src/__tests__/build-pipeline.test.ts | 59 ++- .../engine/src/__tests__/materialize.test.ts | 227 ++++++++- .../engine/src/__tests__/run-install.test.ts | 99 ++-- .../__tests__/api-compatibility.test.ts | 9 +- .../src/adapters/__tests__/inspect.test.ts | 5 +- .../__tests__/placement-managed.test.ts | 7 +- .../src/adapters/__tests__/verify.test.ts | 26 +- .../engine/src/adapters/api-compatibility.ts | 6 +- packages/engine/src/adapters/verify.ts | 8 +- .../src/install/__tests__/run-add.test.ts | 26 +- .../__tests__/run-install.chain.test.ts | 26 +- .../__tests__/run-install.receipt.test.ts | 26 +- .../src/install/__tests__/run-install.test.ts | 44 +- .../src/install/__tests__/run-remove.test.ts | 58 ++- packages/engine/src/install/materialize.ts | 214 ++++++-- 38 files changed, 2889 insertions(+), 587 deletions(-) create mode 100644 .changeset/tagged-adapter-asset-contract.md create mode 100644 packages/adapter/src/__tests__/skill-bundle.test.ts create mode 100644 packages/adapter/src/skill-bundle.ts diff --git a/.changeset/tagged-adapter-asset-contract.md b/.changeset/tagged-adapter-asset-contract.md new file mode 100644 index 00000000..5fd2d275 --- /dev/null +++ b/.changeset/tagged-adapter-asset-contract.md @@ -0,0 +1,18 @@ +--- +'@agent-facets/adapter': minor +'@agent-facets/adapter-claude-code': minor +'@agent-facets/adapter-opencode': minor +'@agent-facets/adapter-codex': minor +--- + +**BREAKING (pre-1.0 minor):** the adapter asset contract is now tagged request/result unions instead of positional parameters, and the adapter API identifier advances from `0.0` to `0.1`. + +`installAsset`, `readAsset`, and `deleteAsset` each take a single request object tagged by `assetType` and return a discriminated result — expected failures (`not-found`, `invalid-companion-path`, `unsupported-scope`, `not-implemented`, `io-failed`) are structured values, never thrown errors. Skill requests carry a companion byte map plus the caller-verified owned companion path set for atomic multi-file skill bundles; agent and command requests structurally cannot carry companions. `defineAdapter` stubs for omitted methods now return `not-implemented` failures instead of throwing. + +The SDK's canonical `ADAPTER_API_VERSION` is now `0.1`, identifying this tagged contract; `defineAdapter()` stamps it and first-party packages publish `"facetAdapterApiVersion": "0.1"`. `0.0` named the earlier positional contract: a CLI that supports only `0.1` classifies a `0.0` adapter as well-formed but unsupported and fails closed (before any contract method or project write) with reinstall guidance. There is no positional/tagged compatibility bridge — an adapter built against `0.0` must be rebuilt against a `0.1` SDK release and reinstalled. + +New SDK helpers: `installSkillBundle` / `readSkillBundle` / `deleteSkillBundle` (staged all-or-nothing bundle replacement with rollback, ownership-set-based deletion, and empty-directory pruning), `installSingleFileAsset` / `readSingleFileAsset` / `deleteSingleFileAsset` (result-shaped single-file operations), and `validateContainedRelativePath` (pre-filesystem containment validation applied to every supplied companion path). + +Every adapter implementing the previous positional contract must migrate. The first-party claude-code, opencode, and codex adapters are migrated in their matching minor releases; codex delete operations now prune emptied directories consistently with the other adapters. + +Release ordering: this SDK release and the three first-party adapter releases publish `0.1` to npm **before** any `agent-facets` CLI release requires `0.1`. Until that CLI ships, existing `0.0` CLIs keep selecting the highest compatible `0.0` adapter release, so this changeset intentionally carries **no** `agent-facets` bump — the CLI change that makes `0.1` the supported set lands in a later release cycle gated on all three first-party adapters having published `facetAdapterApiVersion: 0.1`. diff --git a/docs/cli/adapters/install.mdx b/docs/cli/adapters/install.mdx index a47e6299..ce9cbce4 100644 --- a/docs/cli/adapters/install.mdx +++ b/docs/cli/adapters/install.mdx @@ -80,7 +80,7 @@ Version selectors use the Facet grammar: exact `1.2.3`, major wildcard `1.*`, mi ## Compatible resolution -Every adapter declares the adapter API contract it was built against, and each CLI release supports an exact set of adapter APIs (currently `0.0`). For npm installs, the CLI reads the package's version metadata (the `facetAdapterApiVersion` field each release publishes) and selects the **highest stable release** that both satisfies your version selector and declares a supported API: +Every adapter declares the adapter API contract it was built against, and each CLI release supports an exact set of adapter APIs (currently `0.1`, the tagged request/result contract). A CLI that supports only `0.1` treats an adapter still declaring the earlier positional `0.0` as unsupported; an older `0.0` CLI conversely keeps selecting the highest compatible `0.0` release, so the two lines advance independently. For npm installs, the CLI reads the package's version metadata (the `facetAdapterApiVersion` field each release publishes) and selects the **highest stable release** that both satisfies your version selector and declares a supported API: - A bare name, `*`, or `latest` selects the highest compatible release — independent of npm's `latest` dist-tag. - A wildcard selector (`1.*`, `1.2.*`) selects the highest compatible release in the range. diff --git a/docs/cli/adapters/list.mdx b/docs/cli/adapters/list.mdx index 4146734f..33d12823 100644 --- a/docs/cli/adapters/list.mdx +++ b/docs/cli/adapters/list.mdx @@ -13,14 +13,15 @@ Lists all installed adapters by inspecting the adapter base directory, `$FACET_D ```text Installed adapters: - claude-code api 0.0 supported + claude-code api 0.1 supported + codex api 0.0 unsupported — reinstall: facet adapter install codex opencode api missing unsupported — reinstall: facet adapter install opencode my-tool api unknown broken (bundle failed to load) — reinstall: facet adapter install my-tool ``` ## Output columns -- **API** — the adapter's declared adapter API (`api 0.0`), or `api missing` (no declaration — typically a bundle installed before API versioning), `api malformed ("…")` (a declaration that isn't a valid API identifier), or `api unknown` (the bundle could not be read). +- **API** — the adapter's declared adapter API (`api 0.1`), or `api missing` (no declaration — typically a bundle installed before API versioning), `api malformed ("…")` (a declaration that isn't a valid API identifier), or `api unknown` (the bundle could not be read). An adapter declaring the earlier positional `0.0` is shown as `api 0.0` with an `unsupported` status — rebuild it against a `0.1` SDK release and reinstall. - **Status** — `supported` (usable by this CLI), `unsupported — reinstall: ` (the adapter's API declaration is missing, malformed, or names an API this CLI does not support), or `broken () — reinstall: ` (invalid installation metadata, a missing active bundle, or a bundle that failed to load). Listing stays available when entries are incompatible or broken — run it to find the best available `facet adapter install ` command next to each failing entry. diff --git a/docs/guides/custom-adapters.mdx b/docs/guides/custom-adapters.mdx index b07cefbb..01dde08a 100644 --- a/docs/guides/custom-adapters.mdx +++ b/docs/guides/custom-adapters.mdx @@ -16,19 +16,27 @@ An adapter is a small TypeScript library. You write one file, build it, and inst > - `buildAssetMetadata(data)` (required) — validate/enrich per-asset > manifest metadata; return `Validated`. > - `supportsInstall: true` + `installAsset` / `readAsset` / `deleteAsset` — -> the filesystem I/O that materializes assets. Without these the adapter -> is metadata-only and hidden from the install picker. +> the filesystem I/O that materializes assets. Each takes a single request +> object **tagged by `assetType`** and returns a discriminated result +> (`{ ok: true, … }` or `{ ok: false, failure }`) — expected failures are +> values, never thrown. Without these the adapter is metadata-only and +> hidden from the install picker. > -> Use the SDK's `installAssetFile` / `readAssetFile` / `deleteAssetFile` -> helpers for front-matter-aware file I/O. `Scope` is `'system' | 'user' | -> 'project'`; `AssetType` is `'skill' | 'agent' | 'command'`. +> Use the SDK's skill-bundle helpers (`installSkillBundle` / +> `readSkillBundle` / `deleteSkillBundle`) for the multi-file skill variant, +> and the single-file helpers (`installSingleFileAsset` / +> `readSingleFileAsset` / `deleteSingleFileAsset`) for agents and commands. +> `Scope` is `'system' | 'user' | 'project'`; `AssetType` is +> `'skill' | 'agent' | 'command'`. > > `defineAdapter()` stamps the adapter API version (`apiVersion`, currently -> `0.0`) onto the returned adapter — do NOT set it yourself; the input type -> excludes it. When publishing to npm, the published `package.json` MUST -> declare `"facetAdapterApiVersion": "0.0"` (the canonical constants live at -> `@agent-facets/adapter/api-version`) or the CLI will never select the -> release. +> `0.1`) onto the returned adapter — do NOT set it yourself; the input type +> excludes it. `0.1` identifies the tagged request/result method contract; +> the earlier positional contract (`0.0`) is unsupported by a `0.1` CLI and +> must be migrated. When publishing to npm, the published `package.json` +> MUST declare `"facetAdapterApiVersion": "0.1"` (the canonical constants +> live at `@agent-facets/adapter/api-version`) or the CLI will never select +> the release. > > Build with `tsdown`/`tsc`, then install: > @@ -69,25 +77,33 @@ The SDK is a leaf library with no heavy dependencies -- `@agent-facets/common` -Create `src/index.ts` and default-export a [`defineAdapter`](https://github.com/agent-facets/facets/tree/main/packages/adapter) call. Here is a minimal, working adapter that writes each asset to `~/.my-tool/s/.md`: +Create `src/index.ts` and default-export a [`defineAdapter`](https://github.com/agent-facets/facets/tree/main/packages/adapter) call. Each I/O method takes a single request **tagged by `assetType`** and returns a discriminated result. Skills are multi-file bundles (a primary `SKILL.md` plus companion files); agents and commands are single files. The SDK's bundle and single-file helpers do the heavy lifting: ```ts expandable src/index.ts import { defineAdapter, - installAssetFile, - readAssetFile, - deleteAssetFile, - type Scope, - type AssetType, + installSkillBundle, + readSkillBundle, + deleteSkillBundle, + installSingleFileAsset, + readSingleFileAsset, + deleteSingleFileAsset, + type InstallAssetRequest, + type ReadAssetRequest, + type DeleteAssetRequest, + type SkillBundlePaths, type Validated, type AdapterMetadata, } from '@agent-facets/adapter' import { homedir } from 'node:os' import { join } from 'node:path' -// Where an asset of a given type lives on disk for this tool. -function assetPath(assetType: AssetType, name: string): { file: string } { - return { file: join(homedir(), '.my-tool', `${assetType}s`, `${name}.md`) } +const baseDir = join(homedir(), '.my-tool') + +// A skill lives in its own directory: skills//SKILL.md (+ companions). +function skillPaths(name: string): SkillBundlePaths { + const root = join(baseDir, 'skills', name) + return { root, primaryFile: join(root, 'SKILL.md'), pruneBoundary: baseDir } } export default defineAdapter({ @@ -102,25 +118,59 @@ export default defineAdapter({ return { ok: true, data: meta } }, - // Write the asset. `content` is the asset body; `metadata` becomes - // YAML front-matter. installAssetFile handles the front-matter assembly, - // directory creation, and idempotent overwrite for you. - async installAsset(scope: Scope, assetType: AssetType, name: string, content: string, metadata: unknown) { - return installAssetFile(assetPath(assetType, name), content, metadata as AdapterMetadata) + // Branch on request.assetType. Skills carry a companion byte map plus the + // engine-verified owned-companion path set; the bundle helper stages, + // rolls back, and prunes atomically. Agents and commands are single files. + async installAsset(request: InstallAssetRequest) { + switch (request.assetType) { + case 'skill': + return installSkillBundle(skillPaths(request.name), { + content: request.content, + metadata: request.metadata as Record, + companions: request.companions, + ownedCompanionPaths: request.ownedCompanionPaths, + }) + case 'agent': + return installSingleFileAsset( + { file: join(baseDir, 'agents', `${request.name}.md`) }, + request.content, + request.metadata as Record, + ) + case 'command': + return installSingleFileAsset( + { file: join(baseDir, 'commands', `${request.name}.md`) }, + request.content, + request.metadata as Record, + ) + } }, - async readAsset(scope: Scope, assetType: AssetType, name: string) { - return readAssetFile(assetPath(assetType, name)) + async readAsset(request: ReadAssetRequest) { + switch (request.assetType) { + case 'skill': + return readSkillBundle(skillPaths(request.name), request.ownedCompanionPaths) + case 'agent': + return readSingleFileAsset({ file: join(baseDir, 'agents', `${request.name}.md`) }, 'agent') + case 'command': + return readSingleFileAsset({ file: join(baseDir, 'commands', `${request.name}.md`) }, 'command') + } }, - async deleteAsset(scope: Scope, assetType: AssetType, name: string) { - return deleteAssetFile(assetPath(assetType, name)) + async deleteAsset(request: DeleteAssetRequest) { + switch (request.assetType) { + case 'skill': + return deleteSkillBundle(skillPaths(request.name), request.ownedCompanionPaths) + case 'agent': + return deleteSingleFileAsset({ file: join(baseDir, 'agents', `${request.name}.md`), pruneBoundary: baseDir }) + case 'command': + return deleteSingleFileAsset({ file: join(baseDir, 'commands', `${request.name}.md`), pruneBoundary: baseDir }) + } }, }) ``` - `installAssetFile`, `readAssetFile`, and `deleteAssetFile` are SDK helpers that manage YAML front-matter, create parent directories, and stay byte-stable across a write→read round-trip. Use them instead of hand-rolling file I/O so re-installs don't report phantom drift. + The bundle helpers (`installSkillBundle` / `readSkillBundle` / `deleteSkillBundle`) and single-file helpers (`installSingleFileAsset` / …) manage YAML front-matter, directory creation, atomic staging with rollback, and byte-stable round-trips. Skill helpers only ever touch the primary file plus the caller-supplied owned companion paths, so unowned user files are never read, deleted, or swept into a result. Use them instead of hand-rolling file I/O so re-installs don't report phantom drift. @@ -136,7 +186,7 @@ Everything an adapter can implement, from [`@agent-facets/adapter`](https://gith - The adapter API contract the adapter was built against — **stamped automatically by `defineAdapter()`** (currently `0.0`). You cannot set it in your definition; the input type excludes it. The CLI refuses to load adapters whose declared API it doesn't support. + The adapter API contract the adapter was built against — **stamped automatically by `defineAdapter()`** (currently `0.1`, the tagged request/result contract). You cannot set it in your definition; the input type excludes it. The CLI refuses to load adapters whose declared API it doesn't support, so an adapter built against the earlier positional contract (`0.0`) must be rebuilt against a `0.1` SDK release and reinstalled. @@ -147,19 +197,19 @@ Everything an adapter can implement, from [`@agent-facets/adapter`](https://gith Set to `true` only when all three I/O methods below are implemented. It makes the adapter selectable in the install picker; a metadata-only adapter omits it and stays hidden. - - Write the asset to disk. Return the absolute path (for verbose logs) or `undefined`. + + Install (or replace) an asset. `request` is tagged by `assetType`: the `skill` variant carries `content`, `metadata`, a `companions` byte map, and the engine-verified `ownedCompanionPaths`; `agent`/`command` variants carry `content` and `metadata` only. Return `{ ok: true, primaryPath }` or `{ ok: false, failure }`. - - Read an asset back from disk. + + Read an asset back. The `skill` variant carries `ownedCompanionPaths` (read exactly those, never enumerate the directory). Return `{ ok: true, asset }` or `{ ok: false, failure }` (`failure.code === 'not-found'` when absent). - - Remove an asset. Return its path or `undefined`. + + Remove an asset. The `skill` variant carries `ownedCompanionPaths`; deletion removes the primary plus exactly those, preserving unowned files. Return `{ ok: true, existed, deletedPaths }` or `{ ok: false, failure }`. -Two shared argument types run through the I/O methods: `scope` is `'system'`, `'user'`, or `'project'` (decide your on-disk layout per scope), and `assetType` is `'skill'`, `'agent'`, or `'command'`. +Every request carries `scope` (`'system'`, `'user'`, or `'project'` — decide your on-disk layout per scope) and `name`, tagged by `assetType` (`'skill'`, `'agent'`, or `'command'`). Expected failures are structured values (`not-found`, `invalid-companion-path`, `unsupported-scope`, `not-implemented`, `io-failed`), never thrown errors. You can ship a **metadata-only** adapter: implement just `name` and `buildAssetMetadata`, and omit `supportsInstall`. It validates manifest config during builds without materializing anything -- useful while you're still figuring out a tool's on-disk layout. @@ -224,11 +274,11 @@ Installing by bare name (`facet adapter install my-adapter`) resolves through np ```json package.json { - "facetAdapterApiVersion": "0.0" + "facetAdapterApiVersion": "0.1" } ``` -Declare the adapter API version your SDK release stamps at runtime (currently `0.0`). The CLI skips releases where this field is missing, malformed, or unsupported — a release without it is never selected, and a field that disagrees with the runtime `apiVersion` stamped by `defineAdapter()` fails verification after download. The canonical values are exported from `@agent-facets/adapter/api-version` (`ADAPTER_API_VERSION` and `ADAPTER_API_VERSION_PACKAGE_FIELD`), so release tooling can inject the field instead of hardcoding it. +Declare the adapter API version your SDK release stamps at runtime (currently `0.1`). The CLI skips releases where this field is missing, malformed, or unsupported — a release without it is never selected, and a field that disagrees with the runtime `apiVersion` stamped by `defineAdapter()` fails verification after download. A CLI that supports only `0.1` skips releases still declaring the earlier positional `0.0`, and an older `0.0` CLI conversely keeps selecting your highest compatible `0.0` release — so publish your `0.1` release before requiring a `0.1`-only CLI. The canonical values are exported from `@agent-facets/adapter/api-version` (`ADAPTER_API_VERSION` and `ADAPTER_API_VERSION_PACKAGE_FIELD`), so release tooling can inject the field instead of hardcoding it. ## Share it upstream diff --git a/docs/guides/troubleshooting.mdx b/docs/guides/troubleshooting.mdx index 13b58938..d5a2bdc3 100644 --- a/docs/guides/troubleshooting.mdx +++ b/docs/guides/troubleshooting.mdx @@ -35,7 +35,7 @@ Common errors, what causes them, and how to fix them. Each entry mirrors the CLI - **Cause:** every installed adapter declares the adapter API contract it was built against, and each CLI release supports an exact set (currently `0.0`). A bundle installed before API versioning has **no** declaration; other bundles can carry a malformed declaration, declare an API this CLI doesn't support, or disagree with the npm metadata they were selected by. All four cases fail closed: `facet build`, `facet add`, `facet remove`, and `facet install` stop **before** invoking any adapter method or writing project files. + **Cause:** every installed adapter declares the adapter API contract it was built against, and each CLI release supports an exact set (currently `0.1`, the tagged request/result contract). A bundle installed before API versioning has **no** declaration; a bundle built against the earlier positional contract declares `0.0`, which a `0.1` CLI treats as unsupported; other bundles can carry a malformed declaration, declare an API this CLI doesn't support, or disagree with the npm metadata they were selected by. All these cases fail closed: `facet build`, `facet add`, `facet remove`, and `facet install` stop **before** invoking any adapter method or writing project files. **Fix:** list your installed adapters to see each one's declared API and status, then run the reinstall command printed next to the incompatible entry: diff --git a/packages/adapter/src/__tests__/index.test.ts b/packages/adapter/src/__tests__/index.test.ts index 682aab56..9bac8877 100644 --- a/packages/adapter/src/__tests__/index.test.ts +++ b/packages/adapter/src/__tests__/index.test.ts @@ -12,13 +12,13 @@ function validDefinition(): AdapterDefinition { name: 'test-adapter', buildAssetMetadata: (data) => ({ ok: true, data: (data ?? {}) as Record }), async installAsset() { - return undefined + return { ok: true, primaryPath: '/tmp/test' } }, async readAsset() { - return { content: 'test' } + return { ok: true, asset: { assetType: 'agent', content: 'test' } } }, async deleteAsset() { - return undefined + return { ok: true, existed: true, deletedPaths: ['/tmp/test'] } }, } } @@ -56,8 +56,8 @@ describe('defineAdapter — required field validation', () => { describe('canonical adapter API constants', () => { // The one place tests anchor the spec literals — everywhere else compares // against the exported constants. - test('ADAPTER_API_VERSION is 0.0', () => { - expect(ADAPTER_API_VERSION).toBe('0.0') + test('ADAPTER_API_VERSION is 0.1', () => { + expect(ADAPTER_API_VERSION).toBe('0.1') }) test('ADAPTER_API_VERSION_PACKAGE_FIELD is facetAdapterApiVersion', () => { @@ -95,10 +95,8 @@ describe('defineAdapter — returned adapter shape', () => { test('buildAssetMetadata is callable after creation', () => { const adapter = defineAdapter(validDefinition()) const result = adapter.buildAssetMetadata({ foo: 'bar' }) - expect(result.ok).toBe(true) - if (result.ok) { - expect(result.data).toEqual({ foo: 'bar' }) - } + if (!result.ok) expect.unreachable() + expect(result.data).toEqual({ foo: 'bar' }) }) test('returns a frozen object', () => { @@ -107,7 +105,8 @@ describe('defineAdapter — returned adapter shape', () => { }) test('buildAssetMetadata is bound to the definition (preserves "this")', () => { - const definition = { + const definition: AdapterDefinition & { defaultValue: string } = { + ...validDefinition(), name: 'bound-adapter', defaultValue: 'from-definition', buildAssetMetadata(this: { defaultValue: string }, _data: unknown) { @@ -117,15 +116,6 @@ describe('defineAdapter — returned adapter shape', () => { data: { defaulted: this.defaultValue }, } }, - async installAsset() { - return undefined - }, - async readAsset() { - return { content: 'test' } - }, - async deleteAsset() { - return undefined - }, } const adapter = defineAdapter(definition) @@ -133,10 +123,8 @@ describe('defineAdapter — returned adapter shape', () => { // `this` would be undefined and the call would throw. const buildMeta = adapter.buildAssetMetadata const result = buildMeta({}) - expect(result.ok).toBe(true) - if (result.ok) { - expect(result.data).toEqual({ defaulted: 'from-definition' }) - } + if (!result.ok) expect.unreachable() + expect(result.data).toEqual({ defaulted: 'from-definition' }) }) }) @@ -151,26 +139,31 @@ describe('defineAdapter — stub fallbacks for missing asset methods', () => { return minimal as any } - test('installAsset falls back to a throw-on-call stub when omitted', async () => { + test('installAsset falls back to a structured not-implemented result when omitted', async () => { const adapter = defineAdapter(buildMinimal()) - await expect(adapter.installAsset('project', 'skill', 'foo', 'content', {})).rejects.toThrow( - /does not implement installAsset/, - ) + const result = await adapter.installAsset({ + assetType: 'agent', + scope: 'project', + name: 'foo', + content: 'content', + metadata: {}, + }) + if (result.ok) expect.unreachable() + expect(result.failure).toEqual({ code: 'not-implemented', method: 'installAsset' }) }) - test('readAsset falls back to a throw-on-call stub when omitted', async () => { + test('readAsset falls back to a structured not-implemented result when omitted', async () => { const adapter = defineAdapter(buildMinimal()) - await expect(adapter.readAsset('project', 'skill', 'foo')).rejects.toThrow(/does not implement readAsset/) + const result = await adapter.readAsset({ assetType: 'agent', scope: 'project', name: 'foo' }) + if (result.ok) expect.unreachable() + expect(result.failure).toEqual({ code: 'not-implemented', method: 'readAsset' }) }) - test('deleteAsset falls back to a throw-on-call stub when omitted', async () => { + test('deleteAsset falls back to a structured not-implemented result when omitted', async () => { const adapter = defineAdapter(buildMinimal()) - await expect(adapter.deleteAsset('project', 'skill', 'foo')).rejects.toThrow(/does not implement deleteAsset/) - }) - - test('stub error message includes the adapter name', async () => { - const adapter = defineAdapter(buildMinimal({ name: 'my-custom-adapter' })) - await expect(adapter.installAsset('project', 'skill', 'foo', 'content', {})).rejects.toThrow(/"my-custom-adapter"/) + const result = await adapter.deleteAsset({ assetType: 'agent', scope: 'project', name: 'foo' }) + if (result.ok) expect.unreachable() + expect(result.failure).toEqual({ code: 'not-implemented', method: 'deleteAsset' }) }) test('provided asset methods are used instead of the stub fallback', async () => { @@ -183,25 +176,26 @@ describe('defineAdapter — stub fallbacks for missing asset methods', () => { buildAssetMetadata: (data) => ({ ok: true, data: (data ?? {}) as Record }), async installAsset() { installCalled = true - return undefined + return { ok: true, primaryPath: '/tmp/foo' } }, async readAsset() { readCalled = true - return { content: 'real-content' } + return { ok: true, asset: { assetType: 'command', content: 'real-content' } } }, async deleteAsset() { deleteCalled = true - return undefined + return { ok: true, existed: false, deletedPaths: [] } }, }) - await adapter.installAsset('project', 'skill', 'foo', 'content', {}) - const read = await adapter.readAsset('project', 'skill', 'foo') - await adapter.deleteAsset('project', 'skill', 'foo') + await adapter.installAsset({ assetType: 'command', scope: 'project', name: 'foo', content: 'c', metadata: {} }) + const read = await adapter.readAsset({ assetType: 'command', scope: 'project', name: 'foo' }) + await adapter.deleteAsset({ assetType: 'command', scope: 'project', name: 'foo' }) expect(installCalled).toBe(true) expect(readCalled).toBe(true) expect(deleteCalled).toBe(true) - expect(read.content).toBe('real-content') + if (!read.ok) expect.unreachable() + expect(read.asset.content).toBe('real-content') }) }) diff --git a/packages/adapter/src/__tests__/skill-bundle.test.ts b/packages/adapter/src/__tests__/skill-bundle.test.ts new file mode 100644 index 00000000..dbef79f1 --- /dev/null +++ b/packages/adapter/src/__tests__/skill-bundle.test.ts @@ -0,0 +1,420 @@ +import { afterEach, beforeEach, describe, expect, test } from 'bun:test' +import { chmod, mkdir, mkdtemp, readdir, readFile, rm, symlink, writeFile } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { validateContainedRelativePath } from '../asset-fs.ts' +import { deleteSkillBundle, installSkillBundle, readSkillBundle, type SkillBundlePaths } from '../skill-bundle.ts' + +let baseDir: string + +beforeEach(async () => { + baseDir = await mkdtemp(join(tmpdir(), 'skill-bundle-test-')) +}) + +afterEach(async () => { + await rm(baseDir, { recursive: true, force: true }) +}) + +function paths(name = 'review'): SkillBundlePaths { + const root = join(baseDir, 'skills', name) + return { root, primaryFile: join(root, 'SKILL.md'), pruneBoundary: baseDir } +} + +function bytes(text: string): Uint8Array { + // Copy into a fresh ArrayBuffer-backed view so strict Uint8Array + // generics (Uint8Array vs ArrayBufferLike) line up. + return new Uint8Array(new TextEncoder().encode(text)) +} + +async function exists(path: string): Promise { + return readFile(path).then( + () => true, + () => false, + ) +} + +describe('validateContainedRelativePath', () => { + test('accepts simple and nested relative paths', () => { + expect(validateContainedRelativePath('api.md').ok).toBe(true) + expect(validateContainedRelativePath('references/api.md').ok).toBe(true) + expect(validateContainedRelativePath('a/b/c/d.bin').ok).toBe(true) + }) + + test.each([ + ['', 'empty'], + ['/etc/passwd', 'absolute'], + ['C:/windows', 'Windows drive'], + ['c:relative', 'Windows drive'], + ['a\\b.md', 'backslash'], + ['a/\0/b', 'NUL'], + ['a//b.md', 'empty segment'], + ['./a.md', '"." segment'], + ['../outside.md', '".." segment'], + ['a/../../b.md', '".." segment'], + ])('rejects %j (%s)', (path, _label) => { + expect(validateContainedRelativePath(path).ok).toBe(false) + }) +}) + +describe('installSkillBundle', () => { + test('installs a companion-less skill (empty map, empty owned set)', async () => { + const result = await installSkillBundle(paths(), { + content: '# Review', + metadata: { name: 'review', description: 'reviews things' }, + companions: {}, + ownedCompanionPaths: [], + }) + if (!result.ok) expect.unreachable() + expect(result.primaryPath).toBe(paths().primaryFile) + const written = await readFile(paths().primaryFile, 'utf8') + expect(written).toContain('# Review') + expect(written).toContain('name: review') + }) + + test('installs primary plus companions with verbatim bytes and no metadata leakage', async () => { + const logo = new Uint8Array([0x89, 0x50, 0x4e, 0x47, 0x00, 0x01]) + const result = await installSkillBundle(paths(), { + content: '# Review', + metadata: { name: 'review' }, + companions: { 'references/api.md': bytes('# API docs'), 'assets/logo.png': logo }, + ownedCompanionPaths: [], + }) + if (!result.ok) expect.unreachable() + const api = await readFile(join(paths().root, 'references/api.md')) + expect(new Uint8Array(api)).toEqual(bytes('# API docs')) + const png = await readFile(join(paths().root, 'assets/logo.png')) + expect(new Uint8Array(png)).toEqual(logo) + // No front-matter transformation on companions + expect(api.toString()).not.toContain('---') + }) + + test('writes empty companion bytes', async () => { + const result = await installSkillBundle(paths(), { + content: '# Review', + companions: { 'empty.txt': new Uint8Array() }, + ownedCompanionPaths: [], + }) + if (!result.ok) expect.unreachable() + const empty = await readFile(join(paths().root, 'empty.txt')) + expect(empty.length).toBe(0) + }) + + test('reinstall removes owned companions absent from the new bundle and preserves unowned files', async () => { + const p = paths() + await installSkillBundle(p, { + content: '# v1', + companions: { 'references/old.md': bytes('old'), 'keep.md': bytes('keep') }, + ownedCompanionPaths: [], + }) + // An unowned user file inside the skill directory + await writeFile(join(p.root, 'notes.txt'), 'user notes') + + const result = await installSkillBundle(p, { + content: '# v2', + companions: { 'keep.md': bytes('keep v2') }, + ownedCompanionPaths: ['references/old.md', 'keep.md'], + }) + if (!result.ok) expect.unreachable() + expect(await exists(join(p.root, 'references/old.md'))).toBe(false) + // Directory emptied by owned-file removal is pruned + expect(await exists(join(p.root, 'references'))).toBe(false) + expect(await readFile(join(p.root, 'keep.md'), 'utf8')).toBe('keep v2') + expect(await readFile(join(p.root, 'notes.txt'), 'utf8')).toBe('user notes') + }) + + test('reinstall is idempotent', async () => { + const options = { + content: '# Review', + metadata: { name: 'review' }, + companions: { 'references/api.md': bytes('api') }, + ownedCompanionPaths: ['references/api.md'], + } + const first = await installSkillBundle(paths(), options) + const second = await installSkillBundle(paths(), options) + if (!first.ok || !second.ok) expect.unreachable() + expect(await readFile(join(paths().root, 'references/api.md'), 'utf8')).toBe('api') + }) + + test.each([ + ['../escape.md'], + ['/abs.md'], + ['a\\b.md'], + ['references/../../escape.md'], + ])('rejects escaping companion path %j in the new bundle before any filesystem access', async (bad) => { + const result = await installSkillBundle(paths(), { + content: '# Review', + companions: { [bad]: bytes('x') }, + ownedCompanionPaths: [], + }) + if (result.ok) expect.unreachable() + expect(result.failure.code).toBe('invalid-companion-path') + // Nothing was written — not even the primary + expect(await exists(paths().primaryFile)).toBe(false) + }) + + test('rejects escaping path in the owned set before any filesystem access', async () => { + const result = await installSkillBundle(paths(), { + content: '# Review', + companions: {}, + ownedCompanionPaths: ['../../outside.md'], + }) + if (result.ok) expect.unreachable() + expect(result.failure.code).toBe('invalid-companion-path') + expect(await exists(paths().primaryFile)).toBe(false) + }) + + test('failed companion write rolls back the prior bundle', async () => { + const p = paths() + await installSkillBundle(p, { + content: '# v1', + companions: { 'api.md': bytes('v1 api') }, + ownedCompanionPaths: [], + }) + // Force the companion write to fail: a directory occupies the target path. + await mkdir(join(p.root, 'broken.md')) + + const result = await installSkillBundle(p, { + content: '# v2', + companions: { 'api.md': bytes('v2 api'), 'broken.md': bytes('x') }, + ownedCompanionPaths: ['api.md'], + }) + if (result.ok) expect.unreachable() + expect(result.failure.code).toBe('io-failed') + // Prior bundle intact: primary and companion carry v1 content. + expect(await readFile(p.primaryFile, 'utf8')).toContain('# v1') + expect(await readFile(join(p.root, 'api.md'), 'utf8')).toBe('v1 api') + }) + + test('failed primary write leaves nothing behind for a fresh install', async () => { + const p = paths() + // A directory occupies the primary path. + await mkdir(p.root, { recursive: true }) + await mkdir(p.primaryFile) + + const result = await installSkillBundle(p, { + content: '# Review', + companions: { 'api.md': bytes('api') }, + ownedCompanionPaths: [], + }) + if (result.ok) expect.unreachable() + expect(result.failure.code).toBe('io-failed') + expect(await exists(join(p.root, 'api.md'))).toBe(false) + }) +}) + +describe('readSkillBundle', () => { + test('returns canonical primary content plus exactly the requested owned companions', async () => { + const p = paths() + await installSkillBundle(p, { + content: '# Review', + metadata: { name: 'review', description: 'd' }, + companions: { 'references/api.md': bytes('api'), 'assets/logo.png': bytes('png') }, + ownedCompanionPaths: [], + }) + // Unowned file present in the directory + await writeFile(join(p.root, 'notes.txt'), 'user notes') + + const result = await readSkillBundle(p, ['references/api.md', 'assets/logo.png']) + if (!result.ok) expect.unreachable() + // Canonical content: front-matter storage encoding stripped + expect(result.asset.content).toBe('# Review') + expect(result.asset.metadata).toEqual({ name: 'review', description: 'd' }) + if (result.asset.assetType !== 'skill') expect.unreachable() + expect(Object.keys(result.asset.companions).sort()).toEqual(['assets/logo.png', 'references/api.md']) + expect(new TextDecoder().decode(result.asset.companions['references/api.md'])).toBe('api') + }) + + test('never sweeps unowned files into the result', async () => { + const p = paths() + await installSkillBundle(p, { content: '# Review', companions: {}, ownedCompanionPaths: [] }) + await writeFile(join(p.root, 'notes.txt'), 'user notes') + + const result = await readSkillBundle(p, []) + if (!result.ok) expect.unreachable() + if (result.asset.assetType !== 'skill') expect.unreachable() + expect(result.asset.companions).toEqual({}) + }) + + test('omits owned companions missing from disk (drift signal)', async () => { + const p = paths() + await installSkillBundle(p, { content: '# Review', companions: {}, ownedCompanionPaths: [] }) + + const result = await readSkillBundle(p, ['references/gone.md']) + if (!result.ok) expect.unreachable() + if (result.asset.assetType !== 'skill') expect.unreachable() + expect(result.asset.companions).toEqual({}) + }) + + test('returns not-found for a missing skill', async () => { + const result = await readSkillBundle(paths('missing'), []) + if (result.ok) expect.unreachable() + expect(result.failure.code).toBe('not-found') + }) + + test('rejects an escaping owned path before any filesystem access', async () => { + const result = await readSkillBundle(paths('missing'), ['../secret.md']) + if (result.ok) expect.unreachable() + expect(result.failure.code).toBe('invalid-companion-path') + }) +}) + +describe('deleteSkillBundle', () => { + test('deletes primary plus exactly the owned companions and prunes emptied directories', async () => { + const p = paths() + await installSkillBundle(p, { + content: '# Review', + companions: { 'references/api.md': bytes('api') }, + ownedCompanionPaths: [], + }) + + const result = await deleteSkillBundle(p, ['references/api.md']) + if (!result.ok) expect.unreachable() + expect(result.existed).toBe(true) + expect([...result.deletedPaths].sort()).toEqual([p.primaryFile, join(p.root, 'references/api.md')].sort()) + // The whole skill root is pruned once emptied (boundary is baseDir) + expect(await exists(p.root)).toBe(false) + expect( + await readdir(baseDir).then( + (entries) => entries, + () => null, + ), + ).not.toBeNull() + }) + + test('preserves unowned files and the directories containing them', async () => { + const p = paths() + await installSkillBundle(p, { + content: '# Review', + companions: { 'references/api.md': bytes('api') }, + ownedCompanionPaths: [], + }) + await writeFile(join(p.root, 'notes.txt'), 'user notes') + + const result = await deleteSkillBundle(p, ['references/api.md']) + if (!result.ok) expect.unreachable() + expect(await readFile(join(p.root, 'notes.txt'), 'utf8')).toBe('user notes') + expect(await exists(join(p.root, 'references/api.md'))).toBe(false) + }) + + test('deleting a non-existent skill is success with existed: false', async () => { + const result = await deleteSkillBundle(paths('missing'), []) + if (!result.ok) expect.unreachable() + expect(result.existed).toBe(false) + expect(result.deletedPaths).toEqual([]) + }) + + test('rejects escaping owned paths without deleting anything', async () => { + const p = paths() + await installSkillBundle(p, { content: '# Review', companions: {}, ownedCompanionPaths: [] }) + + for (const bad of ['../outside.md', '/abs.md']) { + const result = await deleteSkillBundle(p, [bad]) + if (result.ok) expect.unreachable() + expect(result.failure.code).toBe('invalid-companion-path') + } + expect(await exists(p.primaryFile)).toBe(true) + }) + + test('failed deletion restores the already-removed files', async () => { + const p = paths() + await installSkillBundle(p, { + content: '# Review', + companions: { 'sub/locked.md': bytes('locked') }, + ownedCompanionPaths: [], + }) + // Make the companion's parent directory read-only so its rm fails + // after the primary has already been deleted. + await chmod(join(p.root, 'sub'), 0o555) + try { + const result = await deleteSkillBundle(p, ['sub/locked.md']) + if (result.ok) expect.unreachable() + expect(result.failure.code).toBe('io-failed') + // Rollback restored the primary. + expect(await readFile(p.primaryFile, 'utf8')).toContain('# Review') + expect(await readFile(join(p.root, 'sub/locked.md'), 'utf8')).toBe('locked') + } finally { + await chmod(join(p.root, 'sub'), 0o755) + } + }) +}) + +describe('skill-bundle containment hardening', () => { + test('rejects a companion that targets the primary file', async () => { + const p = paths() + const result = await installSkillBundle(p, { + content: '# Review', + companions: { 'SKILL.md': bytes('malicious') }, + ownedCompanionPaths: [], + }) + if (result.ok) expect.unreachable() + expect(result.failure.code).toBe('invalid-companion-path') + // A companion NAMED SKILL.md but in a sub-directory is allowed. + const ok = await installSkillBundle(p, { + content: '# Review', + companions: { 'references/SKILL.md': bytes('fine') }, + ownedCompanionPaths: [], + }) + if (!ok.ok) expect.unreachable() + expect(await readFile(join(p.root, 'references/SKILL.md'), 'utf8')).toBe('fine') + }) + + test('rejects the primary file escaping the skill root', async () => { + const root = join(baseDir, 'skills', 'review') + const escaping: SkillBundlePaths = { + root, + primaryFile: join(baseDir, 'victim.md'), + pruneBoundary: baseDir, + } + const result = await installSkillBundle(escaping, { content: '# x', companions: {}, ownedCompanionPaths: [] }) + if (result.ok) expect.unreachable() + expect(result.failure.code).toBe('invalid-companion-path') + expect(await exists(join(baseDir, 'victim.md'))).toBe(false) + }) + + test('rejects companions colliding by portable case folding', async () => { + const p = paths() + const result = await installSkillBundle(p, { + content: '# Review', + companions: { 'References/api.md': bytes('a'), 'references/api.md': bytes('b') }, + ownedCompanionPaths: [], + }) + if (result.ok) expect.unreachable() + expect(result.failure.code).toBe('invalid-companion-path') + }) + + test('rejects a companion whose existing parent directory is a symlink', async () => { + const p = paths() + await installSkillBundle(p, { content: '# Review', companions: {}, ownedCompanionPaths: [] }) + // Create an escape target and a symlink inside the skill root pointing at it. + const outside = join(baseDir, 'outside') + await mkdir(outside, { recursive: true }) + await symlink(outside, join(p.root, 'references')) + + const result = await installSkillBundle(p, { + content: '# Review', + companions: { 'references/api.md': bytes('escaped') }, + ownedCompanionPaths: [], + }) + if (result.ok) expect.unreachable() + expect(result.failure.code).toBe('invalid-companion-path') + // Nothing was written through the symlink. + expect(await exists(join(outside, 'api.md'))).toBe(false) + }) + + test('reads back an owned companion literally named __proto__', async () => { + const p = paths() + // Build the map so `__proto__` is a real own key, not a prototype set. + const companions: Record = Object.create(null) + companions.__proto__ = bytes('proto-bytes') + await installSkillBundle(p, { + content: '# Review', + companions, + ownedCompanionPaths: [], + }) + const result = await readSkillBundle(p, ['__proto__']) + if (!result.ok) expect.unreachable() + if (result.asset.assetType !== 'skill') expect.unreachable() + expect(Object.hasOwn(result.asset.companions, '__proto__')).toBe(true) + expect(new TextDecoder().decode(result.asset.companions.__proto__)).toBe('proto-bytes') + }) +}) diff --git a/packages/adapter/src/api-version.ts b/packages/adapter/src/api-version.ts index 7c580c53..463087e8 100644 --- a/packages/adapter/src/api-version.ts +++ b/packages/adapter/src/api-version.ts @@ -12,10 +12,16 @@ /** * The adapter API contract identifier this SDK stamps into every adapter - * returned by `defineAdapter()`. Identifies the current positional adapter - * method contract. + * returned by `defineAdapter()`. Identifies the current tagged + * request/result adapter method contract. + * + * This supersedes the earlier positional method contract, which was + * identified by `0.0`. A CLI that supports only `0.1` classifies a `0.0` + * adapter as well-formed but unsupported and fails closed — the exact-token + * compatibility machinery cannot inspect method signatures, so the contract + * change is signalled by this identifier, never inferred. */ -export const ADAPTER_API_VERSION = '0.0' as const +export const ADAPTER_API_VERSION = '0.1' as const /** * The top-level `package.json` field where a published npm adapter release diff --git a/packages/adapter/src/asset-fs.ts b/packages/adapter/src/asset-fs.ts index 1fec1b1c..e6e5e940 100644 --- a/packages/adapter/src/asset-fs.ts +++ b/packages/adapter/src/asset-fs.ts @@ -1,7 +1,8 @@ -import { mkdir, readFile, rm, rmdir, writeFile } from 'node:fs/promises' +import { mkdir, readFile, rm, rmdir, stat, writeFile } from 'node:fs/promises' import { dirname, isAbsolute, relative, resolve } from 'node:path' import { splitFrontMatter, validateAssetName } from '@agent-facets/common' import { stringify as stringifyYaml } from 'yaml' +import type { DeleteAssetResult, InstallAssetResult, ReadAssetResult } from './types.ts' /** * Shared filesystem helpers for adapter install/read/delete. @@ -117,8 +118,11 @@ export async function deleteAssetFile(path: AssetPath): Promise { * The non-recursive `rmdir` is load-bearing: it can only ever remove a * directory that is genuinely empty, so pruning can never delete a file * the adapter didn't already delete itself. + * + * Exported for reuse by the skill-bundle helpers; not part of the curated + * public API surface (`index.ts`). */ -async function pruneEmptyParents(startDir: string, boundary: string): Promise { +export async function pruneEmptyParents(startDir: string, boundary: string): Promise { const stop = resolve(boundary) let current = resolve(startDir) while (current !== stop && isStrictlyInside(current, stop)) { @@ -141,6 +145,90 @@ function isStrictlyInside(child: string, parent: string): boolean { return rel.length > 0 && !rel.startsWith('..') && !isAbsolute(rel) } +// --- result-shaped single-file operations (agents and commands) --- + +/** + * Result-shaped install for a single-file asset (agent or command). + * Wraps {@link installAssetFile}, converting I/O exceptions into + * structured `io-failed` values per the tagged adapter contract. + */ +export async function installSingleFileAsset( + path: AssetPath, + body: string, + metadata?: Record, +): Promise { + try { + const primaryPath = await installAssetFile(path, body, metadata) + return { ok: true, primaryPath } + } catch (err) { + return { + ok: false, + failure: { code: 'io-failed', operation: 'write', path: path.file, message: errorMessage(err) }, + } + } +} + +/** + * Result-shaped read for a single-file asset. A missing file returns a + * structured `not-found`; other I/O exceptions become `io-failed`. + */ +export async function readSingleFileAsset(path: AssetPath, assetType: 'agent' | 'command'): Promise { + try { + const { content, metadata } = await readAssetFile(path) + return { ok: true, asset: { assetType, content, metadata } } + } catch (err) { + if (isMissingFileError(err)) return { ok: false, failure: { code: 'not-found' } } + return { + ok: false, + failure: { code: 'io-failed', operation: 'read', path: path.file, message: errorMessage(err) }, + } + } +} + +/** + * Result-shaped delete for a single-file asset. Deleting an absent asset + * is success with `existed: false` (idempotent by contract); I/O + * exceptions become `io-failed`. + */ +export async function deleteSingleFileAsset(path: AssetPath): Promise { + let existed: boolean + try { + // Only ENOENT/ENOTDIR mean "the file wasn't there". Any other stat + // failure (EACCES, EIO, …) is a genuine I/O error: swallowing it would + // report `existed: false` for a file that may have just been deleted, + // corrupting the caller's "was anything removed" bookkeeping. + existed = await stat(path.file).then( + (s) => s.isFile(), + (err) => { + if (isMissingFileError(err)) return false + throw err + }, + ) + await deleteAssetFile(path) + } catch (err) { + return { + ok: false, + failure: { code: 'io-failed', operation: 'delete', path: path.file, message: errorMessage(err) }, + } + } + return { ok: true, existed, deletedPaths: existed ? [path.file] : [] } +} + +/** True for ENOENT/ENOTDIR — the "file didn't exist" errno family. */ +export function isMissingFileError(err: unknown): boolean { + return ( + typeof err === 'object' && + err !== null && + 'code' in err && + ((err as NodeJS.ErrnoException).code === 'ENOENT' || (err as NodeJS.ErrnoException).code === 'ENOTDIR') + ) +} + +/** Render any thrown value as a message string for `io-failed` failures. */ +export function errorMessage(err: unknown): string { + return err instanceof Error ? err.message : String(err) +} + // --- front-matter helpers (exported for adapter-level customization) --- /** @@ -198,3 +286,33 @@ export function assertSafeAssetName(name: string): void { throw new Error(`asset name "${name}" ${check.reason}`) } } + +/** Result of {@link validateContainedRelativePath}. */ +export type ContainedRelativePathResult = { ok: true } | { ok: false; reason: string } + +/** + * Validate a companion path as relative, canonical, and confined below a + * skill root — purely textually, before any filesystem access. + * + * Rejects: empty paths, absolute paths (POSIX and Windows drive/UNC + * forms), backslashes, NUL bytes, and empty, `.`, or `..` segments. + * A path passing this check joined onto the skill root cannot resolve + * outside it. + * + * This runs on every supplied companion path — new-bundle keys and + * caller-supplied owned paths alike — in install, read, and delete. One + * failing path rejects the whole request without touching the filesystem. + */ +export function validateContainedRelativePath(path: string): ContainedRelativePathResult { + if (path.length === 0) return { ok: false, reason: 'path is empty' } + if (path.includes('\0')) return { ok: false, reason: 'path contains a NUL byte' } + if (path.includes('\\')) return { ok: false, reason: 'path contains a backslash' } + if (path.startsWith('/')) return { ok: false, reason: 'path is absolute' } + if (/^[A-Za-z]:/.test(path)) return { ok: false, reason: 'path has a Windows drive prefix' } + for (const segment of path.split('/')) { + if (segment === '') return { ok: false, reason: 'path has an empty segment' } + if (segment === '.') return { ok: false, reason: 'path has a "." segment' } + if (segment === '..') return { ok: false, reason: 'path has a ".." segment' } + } + return { ok: true } +} diff --git a/packages/adapter/src/define-adapter.ts b/packages/adapter/src/define-adapter.ts index 85d064a9..d70f5886 100644 --- a/packages/adapter/src/define-adapter.ts +++ b/packages/adapter/src/define-adapter.ts @@ -41,24 +41,28 @@ export function defineAdapter(definition: AdapterDefinition): Adapter { buildAssetMetadata: definition.buildAssetMetadata.bind(definition), - // CRUD stubs — full implementations deferred to install pipeline + // CRUD stubs — adapters that omit an operation return a structured + // not-implemented failure instead of throwing (errors are values). installAsset: definition.installAsset?.bind(definition) ?? - (async () => { - throw new Error(`Adapter "${definition.name}" does not implement installAsset`) - }), + (async () => ({ + ok: false as const, + failure: { code: 'not-implemented' as const, method: 'installAsset' as const }, + })), readAsset: definition.readAsset?.bind(definition) ?? - (async () => { - throw new Error(`Adapter "${definition.name}" does not implement readAsset`) - }), + (async () => ({ + ok: false as const, + failure: { code: 'not-implemented' as const, method: 'readAsset' as const }, + })), deleteAsset: definition.deleteAsset?.bind(definition) ?? - (async () => { - throw new Error(`Adapter "${definition.name}" does not implement deleteAsset`) - }), + (async () => ({ + ok: false as const, + failure: { code: 'not-implemented' as const, method: 'deleteAsset' as const }, + })), } return Object.freeze(adapter) diff --git a/packages/adapter/src/index.ts b/packages/adapter/src/index.ts index 16c77cc1..7c39be62 100644 --- a/packages/adapter/src/index.ts +++ b/packages/adapter/src/index.ts @@ -1,20 +1,37 @@ export type { AdapterApiVersion } from './api-version.ts' export { ADAPTER_API_VERSION, ADAPTER_API_VERSION_PACKAGE_FIELD } from './api-version.ts' -export type { AssetPath } from './asset-fs.ts' +export type { AssetPath, ContainedRelativePathResult } from './asset-fs.ts' export { assembleAssetContent, assertSafeAssetName, deleteAssetFile, + deleteSingleFileAsset, + errorMessage, installAssetFile, + installSingleFileAsset, + isMissingFileError, readAssetFile, + readSingleFileAsset, splitAssetContent, + validateContainedRelativePath, } from './asset-fs.ts' export { defineAdapter } from './define-adapter.ts' +export type { SkillBundlePaths } from './skill-bundle.ts' +export { deleteSkillBundle, installSkillBundle, readSkillBundle } from './skill-bundle.ts' export type { Adapter, + AdapterAssetFailure, AdapterDefinition, AdapterMetadata, AssetType, + CompanionMap, + DeleteAssetRequest, + DeleteAssetResult, + InstallAssetRequest, + InstallAssetResult, + ReadAsset, + ReadAssetRequest, + ReadAssetResult, Scope, Validated, ValidationError, diff --git a/packages/adapter/src/skill-bundle.ts b/packages/adapter/src/skill-bundle.ts new file mode 100644 index 00000000..74a484e7 --- /dev/null +++ b/packages/adapter/src/skill-bundle.ts @@ -0,0 +1,457 @@ +import { lstat, mkdir, readFile, rm, writeFile } from 'node:fs/promises' +import { basename, dirname, isAbsolute, join, relative, resolve } from 'node:path' +import { + assembleAssetContent, + errorMessage, + isMissingFileError, + pruneEmptyParents, + splitAssetContent, + validateContainedRelativePath, +} from './asset-fs.ts' +import type { + AdapterAssetFailure, + CompanionMap, + DeleteAssetResult, + InstallAssetResult, + ReadAssetResult, +} from './types.ts' + +/** + * Atomic skill-bundle helpers for adapter implementations. + * + * A skill is a multi-file bundle: one primary file (`SKILL.md`, subject to + * front-matter/metadata transformation) plus zero or more companion files + * (opaque bytes, stored verbatim) below the skill root. These helpers + * centralize the security-sensitive machinery — companion-path containment, + * snapshot/rollback staging, ownership-set-based deletion, and + * empty-directory pruning — so adapters built on them inherit correct + * behavior. + * + * Atomicity model: every mutation snapshots the prior state of each file + * it will touch; a handled failure mid-operation rolls the touched files + * back, leaving the prior bundle intact. This covers handled failures + * within one operation — it is not a durable write-ahead log. Recovery + * from a process crash is the caller's idempotent re-install. + * + * Ownership is caller-supplied per operation. These helpers never + * enumerate the skill directory and never touch a path that isn't the + * primary file, a new-bundle companion, or a member of the supplied + * owned-path set — so unowned files can never be read, deleted, or + * swept into results. + */ + +/** Filesystem locations for one skill bundle. */ +export interface SkillBundlePaths { + /** Absolute path of the skill root directory (e.g. `/skills/`). */ + readonly root: string + /** Absolute path of the primary file (e.g. `/SKILL.md`). Must be inside `root`. */ + readonly primaryFile: string + /** + * Absolute path of the adapter-controlled base directory. Directories + * left empty by owned-file removal are pruned upward, stopping before + * this boundary. When absent, pruning stops at (and includes nothing + * above) the skill root's parent. + */ + readonly pruneBoundary?: string +} + +/** + * True when `abs` is a strict descendant of `root` (not equal, not outside), + * using the platform path separator. On Windows `root`/`abs` are + * backslash-separated, so a hardcoded `/` boundary check would reject every + * legitimate companion; `relative`/`isAbsolute` are separator-agnostic. + */ +function isStrictlyBelow(abs: string, root: string): boolean { + const rel = relative(root, abs) + return rel.length > 0 && !rel.startsWith('..') && !isAbsolute(rel) +} + +/** + * A portable collision key for a companion path: NFC-normalized and + * case-folded so paths that are distinct byte sequences but resolve to the + * same file on a case-insensitive (Windows, default macOS) volume are + * detected as duplicates before any write silently overwrites the other. + */ +function companionCollisionKey(relPath: string): string { + return relPath.normalize('NFC').toLowerCase() +} + +/** + * Validate every supplied companion path (new-bundle keys and owned paths) + * and resolve them below the skill root — before any mutating filesystem + * access. Enforces, in order: + * + * - the resolved primary is a strict descendant of the skill root (a public + * SDK caller could otherwise point `primaryFile` at an external file); + * - each companion is textually relative/canonical/contained; + * - the primary filename (`SKILL.md`) is never a companion — it would + * overwrite the assembled primary or delete it as "stale"; + * - no two companions collide by portable case-fold/NFC form; + * - the resolved companion is a strict descendant of the root (defense in + * depth against a textual-validator bug); + * - no already-existing parent directory of a companion is a symlink, so a + * later `mkdir`/`writeFile` cannot follow a link outside the root. + * + * Returns a failure on the first violation without reading, writing, or + * deleting anything mutating (symlink checks are read-only `lstat`s). + */ +async function resolveCompanionPaths( + paths: SkillBundlePaths, + supplied: Iterable, +): Promise<{ ok: true; resolved: Map } | { ok: false; failure: AdapterAssetFailure }> { + const root = resolve(paths.root) + + // The primary file itself must live below the skill root. + const primaryAbs = resolve(paths.primaryFile) + if (!isStrictlyBelow(primaryAbs, root)) { + return { + ok: false, + failure: { + code: 'invalid-companion-path', + path: paths.primaryFile, + reason: 'primary file escapes the skill root', + }, + } + } + const primaryName = basename(primaryAbs) + + const resolvedMap = new Map() + const seenKeys = new Map() + for (const relPath of supplied) { + const check = validateContainedRelativePath(relPath) + if (!check.ok) { + return { ok: false, failure: { code: 'invalid-companion-path', path: relPath, reason: check.reason } } + } + + const abs = resolve(join(root, relPath)) + + // A companion may not target the primary file itself. Otherwise the write + // loop overwrites the assembled primary with opaque bytes, or the + // stale-owned sweep deletes a primary that was just written. A companion + // named SKILL.md in a sub-directory (references/SKILL.md) is fine — it + // resolves to a different path than the primary. + if (abs === primaryAbs) { + return { + ok: false, + failure: { + code: 'invalid-companion-path', + path: relPath, + reason: `companion path collides with the primary file "${primaryName}"`, + }, + } + } + + // Defense-in-depth: the textual validation above already guarantees + // containment; this re-checks the resolved form so a validator bug + // can't silently become a traversal. + if (!isStrictlyBelow(abs, root)) { + return { + ok: false, + failure: { code: 'invalid-companion-path', path: relPath, reason: 'path escapes the skill root' }, + } + } + + // Portable case-fold / NFC duplicate detection: two spellings that map to + // one file on a case-insensitive volume would silently overwrite. + const key = companionCollisionKey(relPath) + const clash = seenKeys.get(key) + if (clash !== undefined && clash !== relPath) { + return { + ok: false, + failure: { + code: 'invalid-companion-path', + path: relPath, + reason: `collides with "${clash}" by case folding or Unicode normalization`, + }, + } + } + seenKeys.set(key, relPath) + + // Reject symlinked existing parents: a lexically valid companion under a + // symlinked directory (references -> /elsewhere) would let a later write + // escape the root. Walk the existing parent chain and reject any symlink. + const parentIssue = await rejectSymlinkedParents(abs, root) + if (parentIssue !== undefined) { + return { ok: false, failure: { code: 'invalid-companion-path', path: relPath, reason: parentIssue } } + } + + resolvedMap.set(relPath, abs) + } + return { ok: true, resolved: resolvedMap } +} + +/** + * Walk from `abs`'s parent up to (but not including) `root`, rejecting any + * intermediate component that exists and is itself a symlink — the escape + * vector where a directory *inside* the skill root (e.g. `references -> + * /elsewhere`) would let a later `mkdir`/`writeFile` land outside the root. + * + * Non-existent parents are fine — `mkdir` will create them inside the root. + * The skill root itself and its ancestors are deliberately not checked: the + * root may legitimately sit under a symlinked ancestor (e.g. macOS `/var -> + * /private/var`), and that says nothing about companion containment. Purely + * read-only (`lstat`, which does not follow links). + */ +async function rejectSymlinkedParents(abs: string, root: string): Promise { + let current = dirname(abs) + const chain: string[] = [] + while (current !== root && isStrictlyBelow(current, root)) { + chain.push(current) + current = dirname(current) + } + for (const dir of chain) { + let info: Awaited> + try { + info = await lstat(dir) + } catch { + // Does not exist yet — safe, mkdir will create it inside the root. + continue + } + if (info.isSymbolicLink()) { + return `parent directory "${dir}" is a symbolic link` + } + } + return undefined +} + +/** Prior state of one file: exact bytes, or null when it did not exist. */ +type Snapshot = Map + +async function snapshotFile(snapshot: Snapshot, absPath: string): Promise { + if (snapshot.has(absPath)) return + try { + snapshot.set(absPath, new Uint8Array(await readFile(absPath))) + } catch (err) { + if (isMissingFileError(err)) { + snapshot.set(absPath, null) + return + } + throw err + } +} + +/** + * Best-effort restore of every snapshotted file: previously-existing files + * get their exact prior bytes back; files that did not exist are removed. + * Returns the first restore error, if any. + */ +async function restoreSnapshot(snapshot: Snapshot): Promise<{ path: string; message: string } | undefined> { + let firstError: { path: string; message: string } | undefined + for (const [absPath, prior] of snapshot) { + try { + if (prior === null) { + await rm(absPath, { force: true }) + } else { + await mkdir(dirname(absPath), { recursive: true }) + await writeFile(absPath, prior) + } + } catch (err) { + firstError ??= { path: absPath, message: errorMessage(err) } + } + } + return firstError +} + +function ioFailure(operation: 'read' | 'write' | 'delete', path: string, err: unknown): AdapterAssetFailure { + return { code: 'io-failed', operation, path, message: errorMessage(err) } +} + +function rollbackFailure(inner: { path: string; message: string }): AdapterAssetFailure { + return { code: 'io-failed', operation: 'rollback', path: inner.path, message: inner.message } +} + +/** + * Install (or replace) a complete skill bundle atomically. + * + * Writes the primary (with front-matter assembly — the only file metadata + * transformation applies to) and every companion verbatim, then removes + * previously-owned companion paths absent from the new bundle. On any + * handled failure the prior state of every touched file is restored, so + * no partial bundle is left behind. Empty directories left by stale-owned + * removal are pruned. + */ +export async function installSkillBundle( + paths: SkillBundlePaths, + options: { + readonly content: string + readonly metadata?: Record + readonly companions: CompanionMap + readonly ownedCompanionPaths: readonly string[] + }, +): Promise { + const newPaths = Object.keys(options.companions) + const validated = await resolveCompanionPaths(paths, new Set([...newPaths, ...options.ownedCompanionPaths])) + if (!validated.ok) return { ok: false, failure: validated.failure } + const { resolved } = validated + + const newSet = new Set(newPaths) + const staleOwned = options.ownedCompanionPaths.filter((p) => !newSet.has(p)) + + const snapshot: Snapshot = new Map() + // Track the path being read so a snapshot failure names the real file, + // not always the primary. + let currentPath = paths.primaryFile + try { + // Snapshot everything we will touch before mutating anything. + await snapshotFile(snapshot, paths.primaryFile) + for (const relPath of newPaths) { + // biome-ignore lint/style/noNonNullAssertion: resolved contains every validated path + currentPath = resolved.get(relPath)! + await snapshotFile(snapshot, currentPath) + } + for (const relPath of staleOwned) { + // biome-ignore lint/style/noNonNullAssertion: resolved contains every validated path + currentPath = resolved.get(relPath)! + await snapshotFile(snapshot, currentPath) + } + } catch (err) { + return { ok: false, failure: ioFailure('read', currentPath, err) } + } + + // Mutate: primary, companions, stale-owned removal. + currentPath = paths.primaryFile + try { + await mkdir(dirname(paths.primaryFile), { recursive: true }) + await writeFile(paths.primaryFile, assembleAssetContent(options.content, options.metadata), 'utf8') + for (const [relPath, bytes] of Object.entries(options.companions)) { + // biome-ignore lint/style/noNonNullAssertion: resolved contains every validated path + const abs = resolved.get(relPath)! + currentPath = abs + await mkdir(dirname(abs), { recursive: true }) + await writeFile(abs, bytes) + } + } catch (err) { + const failure = ioFailure('write', currentPath, err) + const restoreError = await restoreSnapshot(snapshot) + return { ok: false, failure: restoreError ? rollbackFailure(restoreError) : failure } + } + + const deletedDirs: string[] = [] + try { + for (const relPath of staleOwned) { + // biome-ignore lint/style/noNonNullAssertion: resolved contains every validated path + const abs = resolved.get(relPath)! + currentPath = abs + await rm(abs, { force: true }) + deletedDirs.push(dirname(abs)) + } + } catch (err) { + const failure = ioFailure('delete', currentPath, err) + const restoreError = await restoreSnapshot(snapshot) + return { ok: false, failure: restoreError ? rollbackFailure(restoreError) : failure } + } + + await pruneDirs(deletedDirs, paths) + return { ok: true, primaryPath: paths.primaryFile } +} + +/** + * Read a skill bundle: canonical primary content (storage encoding + * stripped via front-matter split) plus the bytes of exactly the + * requested owned companion paths that exist on disk. Owned paths absent + * from disk are simply omitted from the returned map — that omission is + * the caller's drift signal. Never enumerates the skill directory. + */ +export async function readSkillBundle( + paths: SkillBundlePaths, + ownedCompanionPaths: readonly string[], +): Promise { + const validated = await resolveCompanionPaths(paths, ownedCompanionPaths) + if (!validated.ok) return { ok: false, failure: validated.failure } + + let raw: string + try { + raw = await readFile(paths.primaryFile, 'utf8') + } catch (err) { + if (isMissingFileError(err)) return { ok: false, failure: { code: 'not-found' } } + return { ok: false, failure: ioFailure('read', paths.primaryFile, err) } + } + const { content, metadata } = splitAssetContent(raw) + + // Prototype-free map: an owned companion legitimately named `__proto__` + // (or `constructor`, etc.) must become a real entry, not invoke an + // inherited setter and vanish from the result — which a reconciliation + // pass would then read as missing. + const companions: Record = Object.create(null) + for (const [relPath, abs] of validated.resolved) { + try { + companions[relPath] = new Uint8Array(await readFile(abs)) + } catch (err) { + if (isMissingFileError(err)) continue + return { ok: false, failure: ioFailure('read', abs, err) } + } + } + + return { ok: true, asset: { assetType: 'skill', content, metadata, companions } } +} + +/** + * Delete a skill bundle atomically: the primary file plus exactly the + * supplied owned companion paths. Every other file is preserved, and only + * directories left empty by owned-file removal are pruned. On a handled + * failure mid-deletion the already-removed files are restored from their + * snapshots, so the prior bundle remains available. + */ +export async function deleteSkillBundle( + paths: SkillBundlePaths, + ownedCompanionPaths: readonly string[], +): Promise { + const validated = await resolveCompanionPaths(paths, ownedCompanionPaths) + if (!validated.ok) return { ok: false, failure: validated.failure } + + const targets: string[] = [paths.primaryFile, ...validated.resolved.values()] + + // Track the exact path being touched so a failure attributes the error to + // the real file (a companion, or the .meta.json sidecar) rather than + // always blaming the primary. + let currentPath = paths.primaryFile + const snapshot: Snapshot = new Map() + try { + for (const abs of targets) { + currentPath = abs + await snapshotFile(snapshot, abs) + } + } catch (err) { + return { ok: false, failure: ioFailure('read', currentPath, err) } + } + + const existed = [...snapshot.values()].some((bytes) => bytes !== null) + + const deletedPaths: string[] = [] + const deletedDirs: string[] = [] + const sidecarPath = `${paths.primaryFile}.meta.json` + try { + for (const abs of targets) { + if (snapshot.get(abs) === null) continue + currentPath = abs + await rm(abs, { force: true }) + deletedPaths.push(abs) + deletedDirs.push(dirname(abs)) + } + // Legacy sidecar cleanup, mirroring deleteAssetFile. Snapshot it first so + // a mid-delete rollback restores it too, then attribute any rm failure to + // the sidecar path. + await snapshotFile(snapshot, sidecarPath) + currentPath = sidecarPath + await rm(sidecarPath, { force: true }) + } catch (err) { + const failure = ioFailure('delete', currentPath, err) + const restoreError = await restoreSnapshot(snapshot) + return { ok: false, failure: restoreError ? rollbackFailure(restoreError) : failure } + } + + await pruneDirs(deletedDirs, paths) + return { ok: true, existed, deletedPaths } +} + +/** + * Prune empty directories upward from each deletion site. The boundary is + * `pruneBoundary` when supplied (so the skill root itself can be removed + * once emptied), otherwise the skill root's parent. + */ +async function pruneDirs(startDirs: readonly string[], paths: SkillBundlePaths): Promise { + const boundary = paths.pruneBoundary ?? dirname(resolve(paths.root)) + for (const dir of new Set(startDirs)) { + await pruneEmptyParents(dir, boundary) + } +} diff --git a/packages/adapter/src/types.ts b/packages/adapter/src/types.ts index 711188de..1fcb2b88 100644 --- a/packages/adapter/src/types.ts +++ b/packages/adapter/src/types.ts @@ -7,12 +7,182 @@ import type { AdapterApiVersion } from './api-version.ts' */ export type AdapterMetadata = Record +/** + * Canonical map of skill companion paths to opaque bytes. + * + * Keys are paths **relative to the skill root** (e.g. `references/api.md`), + * using forward slashes. Values are exact bytes, stored verbatim — no + * front-matter or metadata transformation ever applies to companions. + * An empty map is legal and is how a companion-less skill is expressed. + */ +export type CompanionMap = Record + +/** + * Install request, tagged by asset type. + * + * The skill variant is the only one that can carry companion files: + * `companions` is the complete new bundle beyond `SKILL.md`, and + * `ownedCompanionPaths` is the caller-verified set of companion paths a + * previous install owned (from the caller's lockfile/receipt records). + * Replacement removes exactly the owned paths absent from the new bundle; + * unowned files are never touched. Adapters never persist ownership or + * infer it from disk — ownership data arrives on every request. + * + * Agent and command variants structurally cannot carry companions or + * ownership sets, and no variant exists for archive-only supplementary + * files (they never reach adapters). + */ +export type InstallAssetRequest = + | { + readonly assetType: 'skill' + readonly scope: Scope + readonly name: string + /** Primary `SKILL.md` text (front-matter transformation applies here only). */ + readonly content: string + readonly metadata: unknown + /** New companion bundle, paths relative to the skill root. `{}` is legal. */ + readonly companions: CompanionMap + /** Caller-verified previously-owned companion paths. `[]` is legal. */ + readonly ownedCompanionPaths: readonly string[] + } + | { + readonly assetType: 'agent' + readonly scope: Scope + readonly name: string + readonly content: string + readonly metadata: unknown + } + | { + readonly assetType: 'command' + readonly scope: Scope + readonly name: string + readonly content: string + readonly metadata: unknown + } + +/** + * Read request, tagged by asset type. + * + * A skill read carries the caller-verified owned companion path set to + * return. The adapter must not enumerate the skill directory — it reads + * exactly the requested owned paths, so unowned files can never be swept + * into a read result. + */ +export type ReadAssetRequest = + | { + readonly assetType: 'skill' + readonly scope: Scope + readonly name: string + /** Owned companion paths whose bytes should be returned. `[]` is legal. */ + readonly ownedCompanionPaths: readonly string[] + } + | { readonly assetType: 'agent'; readonly scope: Scope; readonly name: string } + | { readonly assetType: 'command'; readonly scope: Scope; readonly name: string } + +/** + * Delete request, tagged by asset type. + * + * A skill delete carries the caller-verified owned companion path set; + * the adapter removes the primary plus exactly those paths as one atomic + * operation, preserving every other file and pruning only directories + * left empty by owned-file removal. + */ +export type DeleteAssetRequest = + | { + readonly assetType: 'skill' + readonly scope: Scope + readonly name: string + /** Owned companion paths to delete alongside the primary. `[]` is legal. */ + readonly ownedCompanionPaths: readonly string[] + } + | { readonly assetType: 'agent'; readonly scope: Scope; readonly name: string } + | { readonly assetType: 'command'; readonly scope: Scope; readonly name: string } + +/** + * Structured failure data for adapter asset operations. + * + * Expected failures are values, not thrown errors — the caller branches + * on `code`. Adapters convert their internal I/O exceptions into + * `io-failed`; anything thrown past this boundary is a programmer bug. + */ +export type AdapterAssetFailure = + /** The requested asset does not exist at that scope. */ + | { readonly code: 'not-found' } + /** + * A supplied companion path (new or owned) is malformed or escapes the + * skill root. Detected before any filesystem access; the whole request + * is rejected without reading, writing, or deleting anything. + */ + | { readonly code: 'invalid-companion-path'; readonly path: string; readonly reason: string } + /** The adapter does not support the requested scope. */ + | { readonly code: 'unsupported-scope'; readonly scope: Scope } + /** The adapter does not implement this operation. */ + | { readonly code: 'not-implemented'; readonly method: 'installAsset' | 'readAsset' | 'deleteAsset' } + /** A filesystem operation failed. `operation: 'rollback'` means the + * failure occurred while restoring the prior bundle after another + * failure — the bundle may be partial and needs re-install to converge. */ + | { + readonly code: 'io-failed' + readonly operation: 'read' | 'write' | 'delete' | 'rollback' + readonly path?: string + readonly message: string + } + +/** Result of an install operation. */ +export type InstallAssetResult = + | { + readonly ok: true + /** Absolute path of the written primary file — used for verbose logging. */ + readonly primaryPath: string + } + | { readonly ok: false; readonly failure: AdapterAssetFailure } + +/** + * A successfully read asset, tagged by type. The skill variant carries the + * bytes of exactly the owned companion paths that were requested and exist. + * `content` is canonical logical primary content: adapter-specific storage + * encoding (front-matter wrapping, TOML fields, …) is stripped so callers + * can compare it with portable integrity records. + */ +export type ReadAsset = + | { + readonly assetType: 'skill' + readonly content: string + readonly metadata?: AdapterMetadata + readonly companions: CompanionMap + } + | { readonly assetType: 'agent'; readonly content: string; readonly metadata?: AdapterMetadata } + | { readonly assetType: 'command'; readonly content: string; readonly metadata?: AdapterMetadata } + +/** Result of a read operation. */ +export type ReadAssetResult = + | { readonly ok: true; readonly asset: ReadAsset } + | { readonly ok: false; readonly failure: AdapterAssetFailure } + +/** Result of a delete operation. */ +export type DeleteAssetResult = + | { + readonly ok: true + /** False when the asset did not exist (delete is idempotent — that is success). */ + readonly existed: boolean + /** Absolute paths of every file removed — used for verbose logging. */ + readonly deletedPaths: readonly string[] + } + | { readonly ok: false; readonly failure: AdapterAssetFailure } + /** * The full adapter contract. Returned by `defineAdapter()`. * * An adapter is an AI coding tool (OpenCode, Claude Code, Codex, etc.) * that wraps around an LLM. The adapter is a full abstraction layer * over its tool's storage and configuration. + * + * All three asset operations take tagged requests and return tagged + * results. A skill install is one all-or-nothing operation over the + * complete bundle: the new primary and companions all commit (with + * previously-owned paths absent from the new bundle removed), or the + * prior bundle remains intact. Recovery from an interrupted process is + * the caller's idempotent re-install, so operations must be convergent. */ export interface Adapter { /** Unique adapter name (e.g., "opencode", "claude-code", "codex") */ @@ -43,29 +213,14 @@ export interface Adapter { */ buildAssetMetadata(data: unknown): Validated - /** - * Install an asset at the given scope. Returns the absolute path the - * asset was written to, if available — used for verbose diagnostic - * logging. Returning `void` is backward-compatible (older adapters - * that don't return a path still satisfy the contract). - */ - installAsset( - scope: Scope, - assetType: AssetType, - name: string, - content: string, - metadata: unknown, - ): Promise + /** Install (or replace) an asset. See {@link InstallAssetRequest}. */ + installAsset(request: InstallAssetRequest): Promise - /** Read an asset's content from the given scope */ - readAsset(scope: Scope, assetType: AssetType, name: string): Promise<{ content: string; metadata?: AdapterMetadata }> + /** Read an asset's canonical content. See {@link ReadAssetRequest}. */ + readAsset(request: ReadAssetRequest): Promise - /** - * Delete an asset from the given scope. Returns the absolute path of - * the deleted asset, if available — used for verbose diagnostic - * logging. Returning `void` is backward-compatible. - */ - deleteAsset(scope: Scope, assetType: AssetType, name: string): Promise + /** Delete an asset. See {@link DeleteAssetRequest}. */ + deleteAsset(request: DeleteAssetRequest): Promise } /** diff --git a/packages/adapters/claude-code/src/__tests__/adapter.test.ts b/packages/adapters/claude-code/src/__tests__/adapter.test.ts index 04d7769f..70fd1745 100644 --- a/packages/adapters/claude-code/src/__tests__/adapter.test.ts +++ b/packages/adapters/claude-code/src/__tests__/adapter.test.ts @@ -1,5 +1,5 @@ import { afterEach, beforeEach, describe, expect, test } from 'bun:test' -import { existsSync, mkdtempSync, readFileSync, rmSync } from 'node:fs' +import { existsSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs' import { tmpdir } from 'node:os' import { join } from 'node:path' import { ADAPTER_API_VERSION } from '@agent-facets/adapter' @@ -25,13 +25,11 @@ describe('claude-code adapter — buildAssetMetadata', () => { tools: { Bash: true, Read: false }, permissions: { allow: true }, }) - expect(result.ok).toBe(true) - if (result.ok) { - expect(result.data).toEqual({ - tools: { Bash: true, Read: false }, - permissions: { allow: true }, - }) - } + if (!result.ok) expect.unreachable() + expect(result.data).toEqual({ + tools: { Bash: true, Read: false }, + permissions: { allow: true }, + }) }) test('accepts empty metadata', () => { @@ -66,71 +64,195 @@ describe('claude-code adapter — project-scope I/O round-trip', () => { }) test('skill installs at .claude/skills//SKILL.md', async () => { - await adapter.installAsset('project', 'skill', 'viper-plans/planning', '# plan', {}) - const path = join(workDir, '.claude/skills/viper-plans/planning/SKILL.md') + const result = await adapter.installAsset({ + assetType: 'skill', + scope: 'project', + name: 'planning', + content: '# plan', + metadata: {}, + companions: {}, + ownedCompanionPaths: [], + }) + if (!result.ok) expect.unreachable() + const path = join(workDir, '.claude/skills/planning/SKILL.md') + // macOS tmpdir may resolve through /private — compare the suffix + expect(result.primaryPath.endsWith('.claude/skills/planning/SKILL.md')).toBe(true) expect(readFileSync(path, 'utf8')).toBe('# plan') }) + test('skill installs companions below the skill root with verbatim bytes', async () => { + const result = await adapter.installAsset({ + assetType: 'skill', + scope: 'project', + name: 'planning', + content: '# plan', + metadata: {}, + companions: { 'references/api.md': new TextEncoder().encode('# api') }, + ownedCompanionPaths: [], + }) + if (!result.ok) expect.unreachable() + const companion = join(workDir, '.claude/skills/planning/references/api.md') + expect(readFileSync(companion, 'utf8')).toBe('# api') + }) + test('agent installs at .claude/agents/.md', async () => { - await adapter.installAsset('project', 'agent', 'viper-plans/reviewer', 'agent body', {}) - const path = join(workDir, '.claude/agents/viper-plans/reviewer.md') - expect(readFileSync(path, 'utf8')).toBe('agent body') + const result = await adapter.installAsset({ + assetType: 'agent', + scope: 'project', + name: 'reviewer', + content: 'agent body', + metadata: {}, + }) + if (!result.ok) expect.unreachable() + expect(readFileSync(join(workDir, '.claude/agents/reviewer.md'), 'utf8')).toBe('agent body') }) test('command installs at .claude/commands/.md', async () => { - await adapter.installAsset('project', 'command', 'viper-plans/plan', 'command body', {}) - const path = join(workDir, '.claude/commands/viper-plans/plan.md') - expect(readFileSync(path, 'utf8')).toBe('command body') + const result = await adapter.installAsset({ + assetType: 'command', + scope: 'project', + name: 'plan', + content: 'command body', + metadata: {}, + }) + if (!result.ok) expect.unreachable() + expect(readFileSync(join(workDir, '.claude/commands/plan.md'), 'utf8')).toBe('command body') }) - test('writes YAML front-matter with name + description + adapter extras', async () => { - await adapter.installAsset('project', 'skill', 'viper-plans/planning', '# plan', { + test('writes YAML front-matter with name + description + adapter extras on the primary only', async () => { + await adapter.installAsset({ + assetType: 'skill', + scope: 'project', name: 'planning', - description: 'plan things', - tools: { Bash: true }, + content: '# plan', + metadata: { name: 'planning', description: 'plan things', tools: { Bash: true } }, + companions: { 'notes.md': new TextEncoder().encode('companion body') }, + ownedCompanionPaths: [], }) - const path = join(workDir, '.claude/skills/viper-plans/planning/SKILL.md') - const raw = readFileSync(path, 'utf8') + const raw = readFileSync(join(workDir, '.claude/skills/planning/SKILL.md'), 'utf8') expect(raw).toContain('name: planning') expect(raw).toContain('description: plan things') expect(raw).toContain('Bash: true') expect(raw).toContain('# plan') + // Companion bytes are verbatim — no front-matter injected + expect(readFileSync(join(workDir, '.claude/skills/planning/notes.md'), 'utf8')).toBe('companion body') }) - test('readAsset round-trips body and front-matter metadata', async () => { - await adapter.installAsset('project', 'skill', 'viper-plans/planning', '# plan', { + test('readAsset round-trips body, metadata, and requested owned companions', async () => { + await adapter.installAsset({ + assetType: 'skill', + scope: 'project', name: 'planning', - description: 'plan things', - tools: { Bash: true }, + content: '# plan', + metadata: { name: 'planning', description: 'plan things' }, + companions: { 'references/api.md': new TextEncoder().encode('# api') }, + ownedCompanionPaths: [], }) - const result = await adapter.readAsset('project', 'skill', 'viper-plans/planning') - expect(result.content.trim()).toBe('# plan') - expect(result.metadata).toEqual({ + const result = await adapter.readAsset({ + assetType: 'skill', + scope: 'project', name: 'planning', - description: 'plan things', - tools: { Bash: true }, + ownedCompanionPaths: ['references/api.md'], }) + if (!result.ok) expect.unreachable() + if (result.asset.assetType !== 'skill') expect.unreachable() + expect(result.asset.content.trim()).toBe('# plan') + expect(result.asset.metadata).toEqual({ name: 'planning', description: 'plan things' }) + expect(new TextDecoder().decode(result.asset.companions['references/api.md'])).toBe('# api') }) - test('deleteAsset removes the asset file', async () => { - await adapter.installAsset('project', 'skill', 'viper-plans/planning', '# plan', { + test('readAsset never sweeps unowned files into the result', async () => { + await adapter.installAsset({ + assetType: 'skill', + scope: 'project', name: 'planning', - description: 'plan things', + content: '# plan', + metadata: {}, + companions: {}, + ownedCompanionPaths: [], }) - await adapter.deleteAsset('project', 'skill', 'viper-plans/planning') - const filePath = join(workDir, '.claude/skills/viper-plans/planning/SKILL.md') - expect(existsSync(filePath)).toBe(false) + writeFileSync(join(workDir, '.claude/skills/planning/notes.txt'), 'user notes') + const result = await adapter.readAsset({ + assetType: 'skill', + scope: 'project', + name: 'planning', + ownedCompanionPaths: [], + }) + if (!result.ok) expect.unreachable() + if (result.asset.assetType !== 'skill') expect.unreachable() + expect(result.asset.companions).toEqual({}) }) - test('deleteAsset is a no-op when asset is absent', async () => { - await expect(adapter.deleteAsset('project', 'skill', 'never-installed')).resolves.toBeString() + test('readAsset returns not-found for a missing asset', async () => { + const result = await adapter.readAsset({ assetType: 'agent', scope: 'project', name: 'never-installed' }) + if (result.ok) expect.unreachable() + expect(result.failure.code).toBe('not-found') + }) + + test('deleteAsset removes the skill bundle and preserves unowned files', async () => { + await adapter.installAsset({ + assetType: 'skill', + scope: 'project', + name: 'planning', + content: '# plan', + metadata: {}, + companions: { 'references/api.md': new TextEncoder().encode('# api') }, + ownedCompanionPaths: [], + }) + writeFileSync(join(workDir, '.claude/skills/planning/notes.txt'), 'user notes') + const result = await adapter.deleteAsset({ + assetType: 'skill', + scope: 'project', + name: 'planning', + ownedCompanionPaths: ['references/api.md'], + }) + if (!result.ok) expect.unreachable() + expect(result.existed).toBe(true) + expect(existsSync(join(workDir, '.claude/skills/planning/SKILL.md'))).toBe(false) + expect(existsSync(join(workDir, '.claude/skills/planning/references'))).toBe(false) + expect(readFileSync(join(workDir, '.claude/skills/planning/notes.txt'), 'utf8')).toBe('user notes') + }) + + test('deleteAsset is success with existed: false when asset is absent', async () => { + const result = await adapter.deleteAsset({ + assetType: 'skill', + scope: 'project', + name: 'never-installed', + ownedCompanionPaths: [], + }) + if (!result.ok) expect.unreachable() + expect(result.existed).toBe(false) + }) + + test('installAsset rejects an escaping companion path without writing anything', async () => { + const result = await adapter.installAsset({ + assetType: 'skill', + scope: 'project', + name: 'planning', + content: '# plan', + metadata: {}, + companions: { '../../escape.md': new TextEncoder().encode('x') }, + ownedCompanionPaths: [], + }) + if (result.ok) expect.unreachable() + expect(result.failure.code).toBe('invalid-companion-path') + expect(existsSync(join(workDir, '.claude/skills/planning/SKILL.md'))).toBe(false) }) test('installAsset overwrites unconditionally (idempotent by contract)', async () => { - await adapter.installAsset('project', 'skill', 'viper-plans/planning', 'v1', {}) - await adapter.installAsset('project', 'skill', 'viper-plans/planning', 'v2', {}) - const path = join(workDir, '.claude/skills/viper-plans/planning/SKILL.md') - expect(readFileSync(path, 'utf8')).toBe('v2') + const request = (content: string) => + ({ + assetType: 'skill', + scope: 'project', + name: 'planning', + content, + metadata: {}, + companions: {}, + ownedCompanionPaths: [], + }) as const + await adapter.installAsset(request('v1')) + await adapter.installAsset(request('v2')) + expect(readFileSync(join(workDir, '.claude/skills/planning/SKILL.md'), 'utf8')).toBe('v2') }) }) @@ -150,14 +272,30 @@ describe('claude-code adapter — user-scope base dir', () => { }) test('user scope writes under ~/.claude', async () => { - await adapter.installAsset('user', 'skill', 'viper-plans/planning', '# plan', {}) - const path = join(fakeHome, '.claude/skills/viper-plans/planning/SKILL.md') - expect(readFileSync(path, 'utf8')).toBe('# plan') + const result = await adapter.installAsset({ + assetType: 'skill', + scope: 'user', + name: 'planning', + content: '# plan', + metadata: {}, + companions: {}, + ownedCompanionPaths: [], + }) + if (!result.ok) expect.unreachable() + expect(readFileSync(join(fakeHome, '.claude/skills/planning/SKILL.md'), 'utf8')).toBe('# plan') }) }) describe('claude-code adapter — unsupported scope', () => { - test('system scope throws', async () => { - await expect(adapter.installAsset('system', 'skill', 'x', 'y', {})).rejects.toThrow(/system scope/) + test('system scope returns a structured unsupported-scope failure', async () => { + const result = await adapter.installAsset({ + assetType: 'agent', + scope: 'system', + name: 'x', + content: 'y', + metadata: {}, + }) + if (result.ok) expect.unreachable() + expect(result.failure).toEqual({ code: 'unsupported-scope', scope: 'system' }) }) }) diff --git a/packages/adapters/claude-code/src/index.ts b/packages/adapters/claude-code/src/index.ts index 337ec267..e0af80e6 100644 --- a/packages/adapters/claude-code/src/index.ts +++ b/packages/adapters/claude-code/src/index.ts @@ -1,12 +1,18 @@ import { homedir } from 'node:os' import { join } from 'node:path' import { - type AssetType, + type DeleteAssetRequest, defineAdapter, - deleteAssetFile, - installAssetFile, - readAssetFile, + deleteSingleFileAsset, + deleteSkillBundle, + type InstallAssetRequest, + installSingleFileAsset, + installSkillBundle, + type ReadAssetRequest, + readSingleFileAsset, + readSkillBundle, type Scope, + type SkillBundlePaths, } from '@agent-facets/adapter' import { type } from 'arktype' @@ -42,37 +48,82 @@ export default defineAdapter({ return { ok: true, data: result as Record } }, - async installAsset(scope, assetType, name, content, metadata) { - return installAssetFile({ file: resolvePath(scope, assetType, name) }, content, metadata as Record) + async installAsset(request: InstallAssetRequest) { + const baseDir = baseDirFor(request.scope) + if (baseDir === null) { + return { ok: false as const, failure: { code: 'unsupported-scope' as const, scope: request.scope } } + } + switch (request.assetType) { + case 'skill': + return installSkillBundle(skillPaths(baseDir, request.name), { + content: request.content, + metadata: request.metadata as Record, + companions: request.companions, + ownedCompanionPaths: request.ownedCompanionPaths, + }) + case 'agent': + return installSingleFileAsset( + { file: join(baseDir, 'agents', `${request.name}.md`) }, + request.content, + request.metadata as Record, + ) + case 'command': + return installSingleFileAsset( + { file: join(baseDir, 'commands', `${request.name}.md`) }, + request.content, + request.metadata as Record, + ) + } }, - async readAsset(scope, assetType, name) { - return readAssetFile({ file: resolvePath(scope, assetType, name) }) + async readAsset(request: ReadAssetRequest) { + const baseDir = baseDirFor(request.scope) + if (baseDir === null) { + return { ok: false as const, failure: { code: 'unsupported-scope' as const, scope: request.scope } } + } + switch (request.assetType) { + case 'skill': + return readSkillBundle(skillPaths(baseDir, request.name), request.ownedCompanionPaths) + case 'agent': + return readSingleFileAsset({ file: join(baseDir, 'agents', `${request.name}.md`) }, 'agent') + case 'command': + return readSingleFileAsset({ file: join(baseDir, 'commands', `${request.name}.md`) }, 'command') + } }, - async deleteAsset(scope, assetType, name) { - return deleteAssetFile({ file: resolvePath(scope, assetType, name), pruneBoundary: baseDirFor(scope) }) + async deleteAsset(request: DeleteAssetRequest) { + const baseDir = baseDirFor(request.scope) + if (baseDir === null) { + return { ok: false as const, failure: { code: 'unsupported-scope' as const, scope: request.scope } } + } + switch (request.assetType) { + case 'skill': + return deleteSkillBundle(skillPaths(baseDir, request.name), request.ownedCompanionPaths) + case 'agent': + return deleteSingleFileAsset({ file: join(baseDir, 'agents', `${request.name}.md`), pruneBoundary: baseDir }) + case 'command': + return deleteSingleFileAsset({ file: join(baseDir, 'commands', `${request.name}.md`), pruneBoundary: baseDir }) + } }, }) /** - * Resolve the absolute path for an asset under Claude Code's conventional layout. + * Claude Code's conventional layout: * * user scope → ~/.claude * project scope → /.claude * - * skill → skills//SKILL.md + * skill → skills//SKILL.md (+ companions below skills//) * agent → agents/.md * command → commands/.md * * `` may contain forward slashes for facet-namespacing (e.g., * `viper-plans/planning` → `skills/viper-plans/planning/SKILL.md`). + * + * `system` scope is unsupported; returns `null` so callers can produce a + * structured `unsupported-scope` failure. */ -function resolvePath(scope: Scope, assetType: AssetType, name: string): string { - return join(baseDirFor(scope), relativePathFor(assetType, name)) -} - -function baseDirFor(scope: Scope): string { +function baseDirFor(scope: Scope): string | null { switch (scope) { case 'user': { const home = process.env.HOME ?? homedir() @@ -81,17 +132,11 @@ function baseDirFor(scope: Scope): string { case 'project': return join(process.cwd(), '.claude') case 'system': - throw new Error('claude-code adapter: system scope is not supported') + return null } } -function relativePathFor(assetType: AssetType, name: string): string { - switch (assetType) { - case 'skill': - return join('skills', name, 'SKILL.md') - case 'agent': - return join('agents', `${name}.md`) - case 'command': - return join('commands', `${name}.md`) - } +function skillPaths(baseDir: string, name: string): SkillBundlePaths { + const root = join(baseDir, 'skills', name) + return { root, primaryFile: join(root, 'SKILL.md'), pruneBoundary: baseDir } } diff --git a/packages/adapters/codex/src/__tests__/adapter.test.ts b/packages/adapters/codex/src/__tests__/adapter.test.ts index 51fa888d..67091e1e 100644 --- a/packages/adapters/codex/src/__tests__/adapter.test.ts +++ b/packages/adapters/codex/src/__tests__/adapter.test.ts @@ -1,5 +1,5 @@ import { afterEach, beforeEach, describe, expect, test } from 'bun:test' -import { existsSync, mkdtempSync, readFileSync, rmSync } from 'node:fs' +import { existsSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs' import { tmpdir } from 'node:os' import { join } from 'node:path' import { ADAPTER_API_VERSION } from '@agent-facets/adapter' @@ -88,50 +88,92 @@ describe('codex adapter — project-scope skill I/O', () => { rmSync(workDir, { recursive: true, force: true }) }) + function skillInstall(content: string, metadata: unknown = {}, companions: Record = {}) { + return adapter.installAsset({ + assetType: 'skill', + scope: 'project', + name: 'planning', + content, + metadata, + companions, + ownedCompanionPaths: [], + }) + } + test('skill installs at .agents/skills//SKILL.md', async () => { - await adapter.installAsset('project', 'skill', 'viper-plans/planning', '# plan', {}) - const path = join(workDir, '.agents/skills/viper-plans/planning/SKILL.md') - expect(readFileSync(path, 'utf8')).toBe('# plan') + const result = await skillInstall('# plan') + if (!result.ok) expect.unreachable() + expect(readFileSync(join(workDir, '.agents/skills/planning/SKILL.md'), 'utf8')).toBe('# plan') + }) + + test('skill installs companions below the skill root', async () => { + const result = await skillInstall('# plan', {}, { 'references/api.md': new TextEncoder().encode('# api') }) + if (!result.ok) expect.unreachable() + expect(readFileSync(join(workDir, '.agents/skills/planning/references/api.md'), 'utf8')).toBe('# api') }) test('skill writes YAML front-matter with name + description', async () => { - await adapter.installAsset('project', 'skill', 'viper-plans/planning', '# plan', { - name: 'planning', - description: 'plan things', - }) - const path = join(workDir, '.agents/skills/viper-plans/planning/SKILL.md') - const raw = readFileSync(path, 'utf8') + await skillInstall('# plan', { name: 'planning', description: 'plan things' }) + const raw = readFileSync(join(workDir, '.agents/skills/planning/SKILL.md'), 'utf8') expect(raw).toContain('name: planning') expect(raw).toContain('description: plan things') expect(raw).toContain('# plan') }) test('readAsset round-trips skill body and front-matter metadata', async () => { - await adapter.installAsset('project', 'skill', 'viper-plans/planning', '# plan', { + await skillInstall('# plan', { name: 'planning', description: 'plan things' }) + const result = await adapter.readAsset({ + assetType: 'skill', + scope: 'project', name: 'planning', - description: 'plan things', + ownedCompanionPaths: [], }) - const result = await adapter.readAsset('project', 'skill', 'viper-plans/planning') - expect(result.content.trim()).toBe('# plan') - expect(result.metadata).toEqual({ name: 'planning', description: 'plan things' }) + if (!result.ok) expect.unreachable() + expect(result.asset.content.trim()).toBe('# plan') + expect(result.asset.metadata).toEqual({ name: 'planning', description: 'plan things' }) }) - test('deleteAsset removes skill file', async () => { - await adapter.installAsset('project', 'skill', 'viper-plans/planning', '# plan', {}) - await adapter.deleteAsset('project', 'skill', 'viper-plans/planning') - const filePath = join(workDir, '.agents/skills/viper-plans/planning/SKILL.md') - expect(existsSync(filePath)).toBe(false) + test('deleteAsset removes the skill bundle and prunes the emptied directory', async () => { + await skillInstall('# plan', {}, { 'references/api.md': new TextEncoder().encode('# api') }) + const result = await adapter.deleteAsset({ + assetType: 'skill', + scope: 'project', + name: 'planning', + ownedCompanionPaths: ['references/api.md'], + }) + if (!result.ok) expect.unreachable() + expect(result.existed).toBe(true) + expect(existsSync(join(workDir, '.agents/skills/planning'))).toBe(false) }) - test('deleteAsset is a no-op when skill is absent', async () => { - await expect(adapter.deleteAsset('project', 'skill', 'never-installed')).resolves.toBeUndefined() + test('deleteAsset preserves unowned files in the skill directory', async () => { + await skillInstall('# plan') + writeFileSync(join(workDir, '.agents/skills/planning/notes.txt'), 'user notes') + const result = await adapter.deleteAsset({ + assetType: 'skill', + scope: 'project', + name: 'planning', + ownedCompanionPaths: [], + }) + if (!result.ok) expect.unreachable() + expect(readFileSync(join(workDir, '.agents/skills/planning/notes.txt'), 'utf8')).toBe('user notes') + }) + + test('deleteAsset is success with existed: false when skill is absent', async () => { + const result = await adapter.deleteAsset({ + assetType: 'skill', + scope: 'project', + name: 'never-installed', + ownedCompanionPaths: [], + }) + if (!result.ok) expect.unreachable() + expect(result.existed).toBe(false) }) test('installAsset overwrites skill unconditionally (idempotent)', async () => { - await adapter.installAsset('project', 'skill', 'viper-plans/planning', 'v1', {}) - await adapter.installAsset('project', 'skill', 'viper-plans/planning', 'v2', {}) - const path = join(workDir, '.agents/skills/viper-plans/planning/SKILL.md') - expect(readFileSync(path, 'utf8')).toBe('v2') + await skillInstall('v1') + await skillInstall('v2') + expect(readFileSync(join(workDir, '.agents/skills/planning/SKILL.md'), 'utf8')).toBe('v2') }) }) @@ -154,64 +196,77 @@ describe('codex adapter — project-scope agent I/O', () => { rmSync(workDir, { recursive: true, force: true }) }) + function agentInstall(name: string, content: string, metadata: unknown = {}) { + return adapter.installAsset({ assetType: 'agent', scope: 'project', name, content, metadata }) + } + test('agent installs at .codex/agents/.toml', async () => { - await adapter.installAsset('project', 'agent', 'reviewer', 'You are a reviewer.', { + const result = await agentInstall('reviewer', 'You are a reviewer.', { name: 'reviewer', description: 'Code review specialist', }) - const path = join(workDir, '.codex/agents/reviewer.toml') - expect(existsSync(path)).toBe(true) + if (!result.ok) expect.unreachable() + expect(existsSync(join(workDir, '.codex/agents/reviewer.toml'))).toBe(true) }) test('agent TOML file contains developer_instructions from content', async () => { - await adapter.installAsset('project', 'agent', 'reviewer', 'You are a reviewer.', { + await agentInstall('reviewer', 'You are a reviewer.', { name: 'reviewer', description: 'Code review specialist', }) - const path = join(workDir, '.codex/agents/reviewer.toml') - const raw = readFileSync(path, 'utf8') + const raw = readFileSync(join(workDir, '.codex/agents/reviewer.toml'), 'utf8') expect(raw).toContain('developer_instructions') expect(raw).toContain('You are a reviewer.') expect(raw).toContain('name = "reviewer"') expect(raw).toContain('description = "Code review specialist"') }) - test('readAsset round-trips agent developer_instructions as content', async () => { - await adapter.installAsset('project', 'agent', 'reviewer', 'You are a reviewer.', { + test('readAsset round-trips agent developer_instructions as canonical content', async () => { + await agentInstall('reviewer', 'You are a reviewer.', { name: 'reviewer', description: 'Code review specialist', }) - const result = await adapter.readAsset('project', 'agent', 'reviewer') - expect(result.content).toBe('You are a reviewer.') - expect(result.metadata).toEqual({ name: 'reviewer', description: 'Code review specialist' }) + const result = await adapter.readAsset({ assetType: 'agent', scope: 'project', name: 'reviewer' }) + if (!result.ok) expect.unreachable() + expect(result.asset.content).toBe('You are a reviewer.') + expect(result.asset.metadata).toEqual({ name: 'reviewer', description: 'Code review specialist' }) }) test('deleteAsset removes agent TOML file', async () => { - await adapter.installAsset('project', 'agent', 'reviewer', 'instructions', {}) - await adapter.deleteAsset('project', 'agent', 'reviewer') - const filePath = join(workDir, '.codex/agents/reviewer.toml') - expect(existsSync(filePath)).toBe(false) + await agentInstall('reviewer', 'instructions') + const result = await adapter.deleteAsset({ assetType: 'agent', scope: 'project', name: 'reviewer' }) + if (!result.ok) expect.unreachable() + expect(result.existed).toBe(true) + expect(existsSync(join(workDir, '.codex/agents/reviewer.toml'))).toBe(false) }) - test('deleteAsset is a no-op when agent is absent', async () => { - await expect(adapter.deleteAsset('project', 'agent', 'never-installed')).resolves.toBeUndefined() + test('deleteAsset is success with existed: false when agent is absent', async () => { + const result = await adapter.deleteAsset({ assetType: 'agent', scope: 'project', name: 'never-installed' }) + if (!result.ok) expect.unreachable() + expect(result.existed).toBe(false) + expect(result.deletedPaths).toEqual([]) }) test('installAsset overwrites agent unconditionally (idempotent)', async () => { - await adapter.installAsset('project', 'agent', 'reviewer', 'v1 instructions', {}) - await adapter.installAsset('project', 'agent', 'reviewer', 'v2 instructions', {}) - const result = await adapter.readAsset('project', 'agent', 'reviewer') - expect(result.content).toBe('v2 instructions') + await agentInstall('reviewer', 'v1 instructions') + await agentInstall('reviewer', 'v2 instructions') + const result = await adapter.readAsset({ assetType: 'agent', scope: 'project', name: 'reviewer' }) + if (!result.ok) expect.unreachable() + expect(result.asset.content).toBe('v2 instructions') }) - test('namespaced agent path uses subdirectory', async () => { - await adapter.installAsset('project', 'agent', 'viper-plans/reviewer', 'instructions', {}) - const path = join(workDir, '.codex/agents/viper-plans/reviewer.toml') - expect(existsSync(path)).toBe(true) + test('readAsset returns not-found when the agent TOML file is absent', async () => { + const result = await adapter.readAsset({ assetType: 'agent', scope: 'project', name: 'never-installed' }) + if (result.ok) expect.unreachable() + expect(result.failure.code).toBe('not-found') }) - test('readAsset rejects when the agent TOML file is absent', async () => { - await expect(adapter.readAsset('project', 'agent', 'never-installed')).rejects.toThrow() + test('readAsset returns io-failed for malformed TOML', async () => { + await agentInstall('reviewer', 'instructions') + writeFileSync(join(workDir, '.codex/agents/reviewer.toml'), 'not = [valid toml') + const result = await adapter.readAsset({ assetType: 'agent', scope: 'project', name: 'reviewer' }) + if (result.ok) expect.unreachable() + expect(result.failure.code).toBe('io-failed') }) }) @@ -235,9 +290,15 @@ describe('codex adapter — project-scope command I/O', () => { }) test('command installs at .agents/commands/.md', async () => { - await adapter.installAsset('project', 'command', 'viper-plans/plan', 'command body', {}) - const path = join(workDir, '.agents/commands/viper-plans/plan.md') - expect(readFileSync(path, 'utf8')).toBe('command body') + const result = await adapter.installAsset({ + assetType: 'command', + scope: 'project', + name: 'plan', + content: 'command body', + metadata: {}, + }) + if (!result.ok) expect.unreachable() + expect(readFileSync(join(workDir, '.agents/commands/plan.md'), 'utf8')).toBe('command body') }) }) @@ -261,23 +322,41 @@ describe('codex adapter — user-scope base dirs', () => { }) test('user-scope skill writes under ~/.agents/skills', async () => { - await adapter.installAsset('user', 'skill', 'viper-plans/planning', '# plan', {}) - const path = join(fakeHome, '.agents/skills/viper-plans/planning/SKILL.md') - expect(readFileSync(path, 'utf8')).toBe('# plan') + const result = await adapter.installAsset({ + assetType: 'skill', + scope: 'user', + name: 'planning', + content: '# plan', + metadata: {}, + companions: {}, + ownedCompanionPaths: [], + }) + if (!result.ok) expect.unreachable() + expect(readFileSync(join(fakeHome, '.agents/skills/planning/SKILL.md'), 'utf8')).toBe('# plan') }) test('user-scope agent writes under ~/.codex/agents', async () => { - await adapter.installAsset('user', 'agent', 'reviewer', 'instructions', { + const result = await adapter.installAsset({ + assetType: 'agent', + scope: 'user', name: 'reviewer', + content: 'instructions', + metadata: { name: 'reviewer' }, }) - const path = join(fakeHome, '.codex/agents/reviewer.toml') - expect(existsSync(path)).toBe(true) + if (!result.ok) expect.unreachable() + expect(existsSync(join(fakeHome, '.codex/agents/reviewer.toml'))).toBe(true) }) test('user-scope command writes under ~/.agents/commands', async () => { - await adapter.installAsset('user', 'command', 'plan', 'command body', {}) - const path = join(fakeHome, '.agents/commands/plan.md') - expect(readFileSync(path, 'utf8')).toBe('command body') + const result = await adapter.installAsset({ + assetType: 'command', + scope: 'user', + name: 'plan', + content: 'command body', + metadata: {}, + }) + if (!result.ok) expect.unreachable() + expect(readFileSync(join(fakeHome, '.agents/commands/plan.md'), 'utf8')).toBe('command body') }) }) @@ -286,7 +365,17 @@ describe('codex adapter — user-scope base dirs', () => { // --------------------------------------------------------------------------- describe('codex adapter — unsupported scope', () => { - test('system scope throws', async () => { - await expect(adapter.installAsset('system', 'skill', 'x', 'y', {})).rejects.toThrow(/system scope/) + test('system scope returns a structured unsupported-scope failure', async () => { + const result = await adapter.installAsset({ + assetType: 'skill', + scope: 'system', + name: 'x', + content: 'y', + metadata: {}, + companions: {}, + ownedCompanionPaths: [], + }) + if (result.ok) expect.unreachable() + expect(result.failure).toEqual({ code: 'unsupported-scope', scope: 'system' }) }) }) diff --git a/packages/adapters/codex/src/index.ts b/packages/adapters/codex/src/index.ts index e54e17fa..52b737e8 100644 --- a/packages/adapters/codex/src/index.ts +++ b/packages/adapters/codex/src/index.ts @@ -1,13 +1,23 @@ -import { mkdir, rm } from 'node:fs/promises' +import { mkdir } from 'node:fs/promises' import { homedir } from 'node:os' import { dirname, join } from 'node:path' import { - type AssetType, + type DeleteAssetRequest, defineAdapter, - deleteAssetFile, - installAssetFile, - readAssetFile, + deleteSingleFileAsset, + deleteSkillBundle, + errorMessage, + type InstallAssetRequest, + type InstallAssetResult, + installSingleFileAsset, + installSkillBundle, + isMissingFileError, + type ReadAssetRequest, + type ReadAssetResult, + readSingleFileAsset, + readSkillBundle, type Scope, + type SkillBundlePaths, } from '@agent-facets/adapter' import { type } from 'arktype' import { parse as parseToml, stringify as stringifyToml } from 'smol-toml' @@ -76,71 +86,96 @@ export default defineAdapter({ return { ok: true, data: result as Record } }, - async installAsset(scope, assetType, name, content, metadata) { - const path = resolvePath(scope, assetType, name) - - if (assetType === 'agent') { - await installAgentToml(path, content, metadata as Record) - } else { - await installAssetFile({ file: path }, content, metadata as Record) + async installAsset(request: InstallAssetRequest) { + const baseDir = baseDirFor(request.scope, request.assetType) + if (baseDir === null) { + return { ok: false as const, failure: { code: 'unsupported-scope' as const, scope: request.scope } } + } + switch (request.assetType) { + case 'skill': + return installSkillBundle(skillPaths(baseDir, request.name), { + content: request.content, + metadata: request.metadata as Record, + companions: request.companions, + ownedCompanionPaths: request.ownedCompanionPaths, + }) + case 'agent': + return installAgentToml( + join(baseDir, 'agents', `${request.name}.toml`), + request.content, + request.metadata as Record, + ) + case 'command': + return installSingleFileAsset( + { file: join(baseDir, 'commands', `${request.name}.md`) }, + request.content, + request.metadata as Record, + ) } }, - async readAsset(scope, assetType, name) { - const path = resolvePath(scope, assetType, name) - - if (assetType === 'agent') { - return readAgentToml(path) + async readAsset(request: ReadAssetRequest) { + const baseDir = baseDirFor(request.scope, request.assetType) + if (baseDir === null) { + return { ok: false as const, failure: { code: 'unsupported-scope' as const, scope: request.scope } } + } + switch (request.assetType) { + case 'skill': + return readSkillBundle(skillPaths(baseDir, request.name), request.ownedCompanionPaths) + case 'agent': + return readAgentToml(join(baseDir, 'agents', `${request.name}.toml`)) + case 'command': + return readSingleFileAsset({ file: join(baseDir, 'commands', `${request.name}.md`) }, 'command') } - - return readAssetFile({ file: path }) }, - async deleteAsset(scope, assetType, name) { - const path = resolvePath(scope, assetType, name) - - if (assetType === 'agent') { - await rm(path, { force: true }) - } else { - await deleteAssetFile({ file: path }) + async deleteAsset(request: DeleteAssetRequest) { + const baseDir = baseDirFor(request.scope, request.assetType) + if (baseDir === null) { + return { ok: false as const, failure: { code: 'unsupported-scope' as const, scope: request.scope } } + } + switch (request.assetType) { + case 'skill': + return deleteSkillBundle(skillPaths(baseDir, request.name), request.ownedCompanionPaths) + case 'agent': + // Same lifecycle as any single-file asset: idempotent delete with + // boundary-guarded empty-directory pruning. Codex previously left + // empty agent directories behind; pruning is now consistent. + return deleteSingleFileAsset({ file: join(baseDir, 'agents', `${request.name}.toml`), pruneBoundary: baseDir }) + case 'command': + return deleteSingleFileAsset({ file: join(baseDir, 'commands', `${request.name}.md`), pruneBoundary: baseDir }) } }, }) // --- path resolution --- -function resolvePath(scope: Scope, assetType: AssetType, name: string): string { - return join(baseDirFor(scope, assetType), relativePathFor(assetType, name)) -} - -function baseDirFor(scope: Scope, assetType: AssetType): string { +/** + * Resolve the base directory for a scope + asset type. Agents live in the + * `.codex/` tree; skills and commands live in the `.agents/` tree. + * `system` scope is unsupported; returns `null` so callers can produce a + * structured `unsupported-scope` failure. + */ +function baseDirFor(scope: Scope, assetType: 'skill' | 'agent' | 'command'): string | null { const home = process.env.HOME ?? homedir() switch (scope) { case 'user': { - // Agents live in ~/.codex/; skills + commands live in ~/.agents/ if (assetType === 'agent') return join(home, '.codex') return join(home, '.agents') } case 'project': { - // Agents live in .codex/; skills + commands live in .agents/ if (assetType === 'agent') return join(process.cwd(), '.codex') return join(process.cwd(), '.agents') } case 'system': - throw new Error('codex adapter: system scope is not supported') + return null } } -function relativePathFor(assetType: AssetType, name: string): string { - switch (assetType) { - case 'skill': - return join('skills', name, 'SKILL.md') - case 'agent': - return join('agents', `${name}.toml`) - case 'command': - return join('commands', `${name}.md`) - } +function skillPaths(baseDir: string, name: string): SkillBundlePaths { + const root = join(baseDir, 'skills', name) + return { root, primaryFile: join(root, 'SKILL.md'), pruneBoundary: baseDir } } // --- TOML agent helpers --- @@ -153,32 +188,70 @@ function relativePathFor(assetType: AssetType, name: string): string { * Mirrors how claude-code / opencode treat content as the body and metadata * as the envelope — no content sniffing or format detection. */ -async function installAgentToml(filePath: string, content: string, metadata?: Record): Promise { - await mkdir(dirname(filePath), { recursive: true }) - +async function installAgentToml( + filePath: string, + content: string, + metadata?: Record, +): Promise { const doc: Record = { ...(metadata ?? {}) } if (content.trim().length > 0) { doc.developer_instructions = content } - await Bun.write(filePath, stringifyToml(doc)) + try { + await mkdir(dirname(filePath), { recursive: true }) + await Bun.write(filePath, stringifyToml(doc)) + } catch (err) { + return { + ok: false, + failure: { code: 'io-failed', operation: 'write', path: filePath, message: errorMessage(err) }, + } + } + return { ok: true, primaryPath: filePath } } /** - * Read a Codex agent TOML file. Returns `developer_instructions` as `content` - * and the remaining top-level keys as `metadata`. + * Read a Codex agent TOML file. Returns `developer_instructions` as the + * canonical `content` and the remaining top-level keys as `metadata` — + * the TOML envelope is Codex's storage encoding, stripped on read so + * callers can compare canonical logical content. * - * Throws if the file is missing or contains malformed TOML — mirroring the - * shared `readAssetFile` helper used for skills and commands, so a missing - * agent surfaces as a read failure rather than silently returning empty. + * A missing file is a structured `not-found`; malformed TOML is `io-failed`. */ -async function readAgentToml(filePath: string): Promise<{ content: string; metadata?: Record }> { - const raw = await Bun.file(filePath).text() - const parsed = parseToml(raw) as Record +async function readAgentToml(filePath: string): Promise { + let raw: string + try { + raw = await readTextFile(filePath) + } catch (err) { + if (isMissingFileError(err)) return { ok: false, failure: { code: 'not-found' } } + return { + ok: false, + failure: { code: 'io-failed', operation: 'read', path: filePath, message: errorMessage(err) }, + } + } + + let parsed: Record + try { + parsed = parseToml(raw) as Record + } catch (err) { + return { + ok: false, + failure: { code: 'io-failed', operation: 'read', path: filePath, message: errorMessage(err) }, + } + } const { developer_instructions, ...rest } = parsed const content = typeof developer_instructions === 'string' ? developer_instructions : '' const metadata = Object.keys(rest).length > 0 ? rest : undefined - return { content, metadata } + return { ok: true, asset: { assetType: 'agent', content, metadata } } +} + +/** + * Read a text file, surfacing ENOENT as a thrown errno error. `Bun.file`'s + * `.text()` rejects with an ENOENT-coded error for missing files, which + * `isMissingFileError` recognizes. + */ +async function readTextFile(filePath: string): Promise { + return Bun.file(filePath).text() } diff --git a/packages/adapters/opencode/src/__tests__/adapter.test.ts b/packages/adapters/opencode/src/__tests__/adapter.test.ts index 6c81bbdc..2692acf2 100644 --- a/packages/adapters/opencode/src/__tests__/adapter.test.ts +++ b/packages/adapters/opencode/src/__tests__/adapter.test.ts @@ -1,5 +1,5 @@ import { afterEach, beforeEach, describe, expect, test } from 'bun:test' -import { existsSync, mkdtempSync, readFileSync, rmSync } from 'node:fs' +import { existsSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs' import { tmpdir } from 'node:os' import { join } from 'node:path' import { ADAPTER_API_VERSION } from '@agent-facets/adapter' @@ -22,10 +22,8 @@ describe('opencode adapter — identity', () => { describe('opencode adapter — buildAssetMetadata', () => { test('accepts valid metadata', () => { const result = adapter.buildAssetMetadata({ tools: { grep: true, bash: false }, model: 'gpt-4' }) - expect(result.ok).toBe(true) - if (result.ok) { - expect(result.data).toEqual({ tools: { grep: true, bash: false }, model: 'gpt-4' }) - } + if (!result.ok) expect.unreachable() + expect(result.data).toEqual({ tools: { grep: true, bash: false }, model: 'gpt-4' }) }) test('accepts empty metadata', () => { @@ -45,18 +43,14 @@ describe('opencode adapter — buildAssetMetadata', () => { test('accepts command frontmatter: agent + subtask', () => { const result = adapter.buildAssetMetadata({ agent: 'opencode-adversary', subtask: true }) - expect(result.ok).toBe(true) - if (result.ok) { - expect(result.data).toEqual({ agent: 'opencode-adversary', subtask: true }) - } + if (!result.ok) expect.unreachable() + expect(result.data).toEqual({ agent: 'opencode-adversary', subtask: true }) }) test('accepts agent frontmatter: mode subagent', () => { const result = adapter.buildAssetMetadata({ mode: 'subagent' }) - expect(result.ok).toBe(true) - if (result.ok) { - expect(result.data).toEqual({ mode: 'subagent' }) - } + if (!result.ok) expect.unreachable() + expect(result.data).toEqual({ mode: 'subagent' }) }) test('accepts scoped permission block (string shorthand + nested glob object)', () => { @@ -93,31 +87,68 @@ describe('opencode adapter — project-scope I/O round-trip', () => { }) test('skill installs at .opencode/skills//SKILL.md', async () => { - await adapter.installAsset('project', 'skill', 'viper-plans/planning', '# plan', {}) - const path = join(workDir, '.opencode/skills/viper-plans/planning/SKILL.md') - expect(readFileSync(path, 'utf8')).toBe('# plan') + const result = await adapter.installAsset({ + assetType: 'skill', + scope: 'project', + name: 'planning', + content: '# plan', + metadata: {}, + companions: {}, + ownedCompanionPaths: [], + }) + if (!result.ok) expect.unreachable() + expect(readFileSync(join(workDir, '.opencode/skills/planning/SKILL.md'), 'utf8')).toBe('# plan') + }) + + test('skill installs companions below the skill root', async () => { + const result = await adapter.installAsset({ + assetType: 'skill', + scope: 'project', + name: 'planning', + content: '# plan', + metadata: {}, + companions: { 'scripts/run.ts': new TextEncoder().encode('console.log(1)') }, + ownedCompanionPaths: [], + }) + if (!result.ok) expect.unreachable() + expect(readFileSync(join(workDir, '.opencode/skills/planning/scripts/run.ts'), 'utf8')).toBe('console.log(1)') }) test('agent installs at .opencode/agents/.md', async () => { - await adapter.installAsset('project', 'agent', 'viper-plans/reviewer', 'agent body', {}) - const path = join(workDir, '.opencode/agents/viper-plans/reviewer.md') - expect(readFileSync(path, 'utf8')).toBe('agent body') + const result = await adapter.installAsset({ + assetType: 'agent', + scope: 'project', + name: 'reviewer', + content: 'agent body', + metadata: {}, + }) + if (!result.ok) expect.unreachable() + expect(readFileSync(join(workDir, '.opencode/agents/reviewer.md'), 'utf8')).toBe('agent body') }) test('command installs at .opencode/commands/.md', async () => { - await adapter.installAsset('project', 'command', 'viper-plans/plan', 'command body', {}) - const path = join(workDir, '.opencode/commands/viper-plans/plan.md') - expect(readFileSync(path, 'utf8')).toBe('command body') + const result = await adapter.installAsset({ + assetType: 'command', + scope: 'project', + name: 'plan', + content: 'command body', + metadata: {}, + }) + if (!result.ok) expect.unreachable() + expect(readFileSync(join(workDir, '.opencode/commands/plan.md'), 'utf8')).toBe('command body') }) test('writes YAML front-matter with name + description + adapter extras', async () => { - await adapter.installAsset('project', 'skill', 'viper-plans/planning', '# plan', { + await adapter.installAsset({ + assetType: 'skill', + scope: 'project', name: 'planning', - description: 'plan things', - model: 'sonnet', + content: '# plan', + metadata: { name: 'planning', description: 'plan things', model: 'sonnet' }, + companions: {}, + ownedCompanionPaths: [], }) - const path = join(workDir, '.opencode/skills/viper-plans/planning/SKILL.md') - const raw = readFileSync(path, 'utf8') + const raw = readFileSync(join(workDir, '.opencode/skills/planning/SKILL.md'), 'utf8') expect(raw).toContain('name: planning') expect(raw).toContain('description: plan things') expect(raw).toContain('model: sonnet') @@ -125,69 +156,129 @@ describe('opencode adapter — project-scope I/O round-trip', () => { }) test('agent installs with mode: subagent front-matter and round-trips', async () => { - await adapter.installAsset('project', 'agent', 'openspec-adversary/adversary', 'agent body', { + await adapter.installAsset({ + assetType: 'agent', + scope: 'project', name: 'adversary', - description: 'adversary subagent', - mode: 'subagent', + content: 'agent body', + metadata: { name: 'adversary', description: 'adversary subagent', mode: 'subagent' }, }) - const path = join(workDir, '.opencode/agents/openspec-adversary/adversary.md') - const raw = readFileSync(path, 'utf8') + const raw = readFileSync(join(workDir, '.opencode/agents/adversary.md'), 'utf8') expect(raw).toContain('mode: subagent') expect(raw).toContain('agent body') - const result = await adapter.readAsset('project', 'agent', 'openspec-adversary/adversary') - expect(result.content.trim()).toBe('agent body') - expect(result.metadata).toEqual({ name: 'adversary', description: 'adversary subagent', mode: 'subagent' }) + const result = await adapter.readAsset({ assetType: 'agent', scope: 'project', name: 'adversary' }) + if (!result.ok) expect.unreachable() + expect(result.asset.content.trim()).toBe('agent body') + expect(result.asset.metadata).toEqual({ name: 'adversary', description: 'adversary subagent', mode: 'subagent' }) }) test('command installs with agent + subtask front-matter', async () => { - await adapter.installAsset('project', 'command', 'openspec-adversary/run-adversary', 'command body', { + await adapter.installAsset({ + assetType: 'command', + scope: 'project', name: 'run-adversary', - description: 'authoring half', - agent: 'opencode-adversary', - subtask: true, + content: 'command body', + metadata: { name: 'run-adversary', description: 'authoring half', agent: 'opencode-adversary', subtask: true }, }) - const path = join(workDir, '.opencode/commands/openspec-adversary/run-adversary.md') - const raw = readFileSync(path, 'utf8') + const raw = readFileSync(join(workDir, '.opencode/commands/run-adversary.md'), 'utf8') expect(raw).toContain('agent: opencode-adversary') expect(raw).toContain('subtask: true') expect(raw).toContain('command body') }) - test('readAsset round-trips body and front-matter metadata', async () => { - await adapter.installAsset('project', 'skill', 'viper-plans/planning', '# plan', { + test('readAsset round-trips skill body, metadata, and owned companions', async () => { + await adapter.installAsset({ + assetType: 'skill', + scope: 'project', name: 'planning', - description: 'plan things', - model: 'sonnet', + content: '# plan', + metadata: { name: 'planning', description: 'plan things', model: 'sonnet' }, + companions: { 'references/api.md': new TextEncoder().encode('# api') }, + ownedCompanionPaths: [], }) - const result = await adapter.readAsset('project', 'skill', 'viper-plans/planning') - expect(result.content.trim()).toBe('# plan') - expect(result.metadata).toEqual({ + const result = await adapter.readAsset({ + assetType: 'skill', + scope: 'project', name: 'planning', - description: 'plan things', - model: 'sonnet', + ownedCompanionPaths: ['references/api.md'], }) + if (!result.ok) expect.unreachable() + if (result.asset.assetType !== 'skill') expect.unreachable() + expect(result.asset.content.trim()).toBe('# plan') + expect(result.asset.metadata).toEqual({ name: 'planning', description: 'plan things', model: 'sonnet' }) + expect(new TextDecoder().decode(result.asset.companions['references/api.md'])).toBe('# api') }) - test('deleteAsset removes the asset file', async () => { - await adapter.installAsset('project', 'skill', 'viper-plans/planning', '# plan', { + test('readAsset returns not-found for a missing asset', async () => { + const result = await adapter.readAsset({ assetType: 'command', scope: 'project', name: 'never-installed' }) + if (result.ok) expect.unreachable() + expect(result.failure.code).toBe('not-found') + }) + + test('deleteAsset removes the skill bundle and preserves unowned files', async () => { + await adapter.installAsset({ + assetType: 'skill', + scope: 'project', + name: 'planning', + content: '# plan', + metadata: {}, + companions: { 'references/api.md': new TextEncoder().encode('# api') }, + ownedCompanionPaths: [], + }) + writeFileSync(join(workDir, '.opencode/skills/planning/notes.txt'), 'user notes') + const result = await adapter.deleteAsset({ + assetType: 'skill', + scope: 'project', name: 'planning', - description: 'plan things', + ownedCompanionPaths: ['references/api.md'], }) - await adapter.deleteAsset('project', 'skill', 'viper-plans/planning') - const filePath = join(workDir, '.opencode/skills/viper-plans/planning/SKILL.md') - expect(existsSync(filePath)).toBe(false) + if (!result.ok) expect.unreachable() + expect(result.existed).toBe(true) + expect(existsSync(join(workDir, '.opencode/skills/planning/SKILL.md'))).toBe(false) + expect(readFileSync(join(workDir, '.opencode/skills/planning/notes.txt'), 'utf8')).toBe('user notes') }) - test('deleteAsset is a no-op when asset is absent', async () => { - await expect(adapter.deleteAsset('project', 'skill', 'never-installed')).resolves.toBeString() + test('deleteAsset is success with existed: false when asset is absent', async () => { + const result = await adapter.deleteAsset({ + assetType: 'skill', + scope: 'project', + name: 'never-installed', + ownedCompanionPaths: [], + }) + if (!result.ok) expect.unreachable() + expect(result.existed).toBe(false) + }) + + test('installAsset rejects an escaping owned path without writing anything', async () => { + const result = await adapter.installAsset({ + assetType: 'skill', + scope: 'project', + name: 'planning', + content: '# plan', + metadata: {}, + companions: {}, + ownedCompanionPaths: ['/etc/passwd'], + }) + if (result.ok) expect.unreachable() + expect(result.failure.code).toBe('invalid-companion-path') + expect(existsSync(join(workDir, '.opencode/skills/planning/SKILL.md'))).toBe(false) }) test('installAsset overwrites unconditionally (idempotent by contract)', async () => { - await adapter.installAsset('project', 'skill', 'viper-plans/planning', 'v1', {}) - await adapter.installAsset('project', 'skill', 'viper-plans/planning', 'v2', {}) - const path = join(workDir, '.opencode/skills/viper-plans/planning/SKILL.md') - expect(readFileSync(path, 'utf8')).toBe('v2') + const request = (content: string) => + ({ + assetType: 'skill', + scope: 'project', + name: 'planning', + content, + metadata: {}, + companions: {}, + ownedCompanionPaths: [], + }) as const + await adapter.installAsset(request('v1')) + await adapter.installAsset(request('v2')) + expect(readFileSync(join(workDir, '.opencode/skills/planning/SKILL.md'), 'utf8')).toBe('v2') }) }) @@ -207,14 +298,30 @@ describe('opencode adapter — user-scope base dir', () => { }) test('user scope writes under ~/.config/opencode', async () => { - await adapter.installAsset('user', 'skill', 'viper-plans/planning', '# plan', {}) - const path = join(fakeHome, '.config/opencode/skills/viper-plans/planning/SKILL.md') - expect(readFileSync(path, 'utf8')).toBe('# plan') + const result = await adapter.installAsset({ + assetType: 'skill', + scope: 'user', + name: 'planning', + content: '# plan', + metadata: {}, + companions: {}, + ownedCompanionPaths: [], + }) + if (!result.ok) expect.unreachable() + expect(readFileSync(join(fakeHome, '.config/opencode/skills/planning/SKILL.md'), 'utf8')).toBe('# plan') }) }) describe('opencode adapter — unsupported scope', () => { - test('system scope throws', async () => { - await expect(adapter.installAsset('system', 'skill', 'x', 'y', {})).rejects.toThrow(/system scope/) + test('system scope returns a structured unsupported-scope failure', async () => { + const result = await adapter.installAsset({ + assetType: 'agent', + scope: 'system', + name: 'x', + content: 'y', + metadata: {}, + }) + if (result.ok) expect.unreachable() + expect(result.failure).toEqual({ code: 'unsupported-scope', scope: 'system' }) }) }) diff --git a/packages/adapters/opencode/src/index.ts b/packages/adapters/opencode/src/index.ts index 8a457cc3..042a54dd 100644 --- a/packages/adapters/opencode/src/index.ts +++ b/packages/adapters/opencode/src/index.ts @@ -1,12 +1,18 @@ import { homedir } from 'node:os' import { join } from 'node:path' import { - type AssetType, + type DeleteAssetRequest, defineAdapter, - deleteAssetFile, - installAssetFile, - readAssetFile, + deleteSingleFileAsset, + deleteSkillBundle, + type InstallAssetRequest, + installSingleFileAsset, + installSkillBundle, + type ReadAssetRequest, + readSingleFileAsset, + readSkillBundle, type Scope, + type SkillBundlePaths, } from '@agent-facets/adapter' import { type } from 'arktype' @@ -60,37 +66,82 @@ export default defineAdapter({ return { ok: true, data: result as Record } }, - async installAsset(scope, assetType, name, content, metadata) { - return installAssetFile({ file: resolvePath(scope, assetType, name) }, content, metadata as Record) + async installAsset(request: InstallAssetRequest) { + const baseDir = baseDirFor(request.scope) + if (baseDir === null) { + return { ok: false as const, failure: { code: 'unsupported-scope' as const, scope: request.scope } } + } + switch (request.assetType) { + case 'skill': + return installSkillBundle(skillPaths(baseDir, request.name), { + content: request.content, + metadata: request.metadata as Record, + companions: request.companions, + ownedCompanionPaths: request.ownedCompanionPaths, + }) + case 'agent': + return installSingleFileAsset( + { file: join(baseDir, 'agents', `${request.name}.md`) }, + request.content, + request.metadata as Record, + ) + case 'command': + return installSingleFileAsset( + { file: join(baseDir, 'commands', `${request.name}.md`) }, + request.content, + request.metadata as Record, + ) + } }, - async readAsset(scope, assetType, name) { - return readAssetFile({ file: resolvePath(scope, assetType, name) }) + async readAsset(request: ReadAssetRequest) { + const baseDir = baseDirFor(request.scope) + if (baseDir === null) { + return { ok: false as const, failure: { code: 'unsupported-scope' as const, scope: request.scope } } + } + switch (request.assetType) { + case 'skill': + return readSkillBundle(skillPaths(baseDir, request.name), request.ownedCompanionPaths) + case 'agent': + return readSingleFileAsset({ file: join(baseDir, 'agents', `${request.name}.md`) }, 'agent') + case 'command': + return readSingleFileAsset({ file: join(baseDir, 'commands', `${request.name}.md`) }, 'command') + } }, - async deleteAsset(scope, assetType, name) { - return deleteAssetFile({ file: resolvePath(scope, assetType, name), pruneBoundary: baseDirFor(scope) }) + async deleteAsset(request: DeleteAssetRequest) { + const baseDir = baseDirFor(request.scope) + if (baseDir === null) { + return { ok: false as const, failure: { code: 'unsupported-scope' as const, scope: request.scope } } + } + switch (request.assetType) { + case 'skill': + return deleteSkillBundle(skillPaths(baseDir, request.name), request.ownedCompanionPaths) + case 'agent': + return deleteSingleFileAsset({ file: join(baseDir, 'agents', `${request.name}.md`), pruneBoundary: baseDir }) + case 'command': + return deleteSingleFileAsset({ file: join(baseDir, 'commands', `${request.name}.md`), pruneBoundary: baseDir }) + } }, }) /** - * Resolve the absolute path for an asset under OpenCode's conventional layout. + * OpenCode's conventional layout: * - * user scope → ~/.config/opencode + * user scope → ~/.config/opencode (or $XDG_CONFIG_HOME/opencode) * project scope → /.opencode * - * skill → skills//SKILL.md + * skill → skills//SKILL.md (+ companions below skills//) * agent → agents/.md * command → commands/.md * * `` may contain forward slashes for facet-namespacing (e.g., * `viper-plans/planning` → `skills/viper-plans/planning/SKILL.md`). + * + * `system` scope is unsupported; returns `null` so callers can produce a + * structured `unsupported-scope` failure. */ -function resolvePath(scope: Scope, assetType: AssetType, name: string): string { - return join(baseDirFor(scope), relativePathFor(assetType, name)) -} - -function baseDirFor(scope: Scope): string { +function baseDirFor(scope: Scope): string | null { switch (scope) { case 'user': { const xdg = process.env.XDG_CONFIG_HOME @@ -101,17 +152,11 @@ function baseDirFor(scope: Scope): string { case 'project': return join(process.cwd(), '.opencode') case 'system': - throw new Error('opencode adapter: system scope is not supported') + return null } } -function relativePathFor(assetType: AssetType, name: string): string { - switch (assetType) { - case 'skill': - return join('skills', name, 'SKILL.md') - case 'agent': - return join('agents', `${name}.md`) - case 'command': - return join('commands', `${name}.md`) - } +function skillPaths(baseDir: string, name: string): SkillBundlePaths { + const root = join(baseDir, 'skills', name) + return { root, primaryFile: join(root, 'SKILL.md'), pruneBoundary: baseDir } } diff --git a/packages/cli/src/__tests__/adapter-install-cli.e2e.test.ts b/packages/cli/src/__tests__/adapter-install-cli.e2e.test.ts index 1beae910..428e9518 100644 --- a/packages/cli/src/__tests__/adapter-install-cli.e2e.test.ts +++ b/packages/cli/src/__tests__/adapter-install-cli.e2e.test.ts @@ -3,6 +3,7 @@ import { existsSync } from 'node:fs' import { mkdir, mkdtemp, readdir, rm, stat, writeFile } from 'node:fs/promises' import { tmpdir } from 'node:os' import { join, resolve } from 'node:path' +import { ADAPTER_API_VERSION } from '@agent-facets/adapter/api-version' /** * End-to-end integration tests that spawn the compiled `./dist/facet` binary @@ -558,7 +559,7 @@ describe('facet adapter list — inspection-backed output', () => { const listResult = await runCli(['adapter', 'list'], { FACET_DIR: facetDir }) expect(listResult.exitCode).toBe(0) expect(listResult.stdout).toContain('opencode') - expect(listResult.stdout).toContain('api 0.0') + expect(listResult.stdout).toContain(`api ${ADAPTER_API_VERSION}`) expect(listResult.stdout).toContain('supported') } finally { await rm(facetDir, { recursive: true, force: true }) diff --git a/packages/cli/src/commands/add/__tests__/add.test.ts b/packages/cli/src/commands/add/__tests__/add.test.ts index 5ddf8410..3bdecdaa 100644 --- a/packages/cli/src/commands/add/__tests__/add.test.ts +++ b/packages/cli/src/commands/add/__tests__/add.test.ts @@ -48,14 +48,28 @@ export default { apiVersion: '${ADAPTER_API_VERSION}', supportsInstall: true, buildAssetMetadata(data) { return { ok: true, data: data || {} } }, - async installAsset(scope, type, name, content, metadata) { - await installAssetFile({ file: path(type, name) }, content, metadata) + async installAsset(req) { + const file = path(req.assetType, req.name) + await installAssetFile({ file }, req.content, req.metadata) + return { ok: true, primaryPath: file } }, - async readAsset(scope, type, name) { - return readAssetFile({ file: path(type, name) }) + async readAsset(req) { + try { + const r = await readAssetFile({ file: path(req.assetType, req.name) }) + return { + ok: true, + asset: req.assetType === 'skill' + ? { assetType: 'skill', content: r.content, metadata: r.metadata, companions: {} } + : { assetType: req.assetType, content: r.content, metadata: r.metadata }, + } + } catch { + return { ok: false, failure: { code: 'not-found' } } + } }, - async deleteAsset(scope, type, name) { - await deleteAssetFile({ file: path(type, name) }) + async deleteAsset(req) { + const file = path(req.assetType, req.name) + await deleteAssetFile({ file }) + return { ok: true, existed: true, deletedPaths: [file] } }, } `, diff --git a/packages/cli/src/commands/install/__tests__/install-cli.test.ts b/packages/cli/src/commands/install/__tests__/install-cli.test.ts index 04a15191..7356e313 100644 --- a/packages/cli/src/commands/install/__tests__/install-cli.test.ts +++ b/packages/cli/src/commands/install/__tests__/install-cli.test.ts @@ -54,14 +54,28 @@ export default { apiVersion: '${ADAPTER_API_VERSION}', supportsInstall: true, buildAssetMetadata(data) { return { ok: true, data: data || {} } }, - async installAsset(scope, type, name, content, metadata) { - await installAssetFile({ file: path(type, name) }, content, metadata) + async installAsset(req) { + const file = path(req.assetType, req.name) + await installAssetFile({ file }, req.content, req.metadata) + return { ok: true, primaryPath: file } }, - async readAsset(scope, type, name) { - return readAssetFile({ file: path(type, name) }) + async readAsset(req) { + try { + const r = await readAssetFile({ file: path(req.assetType, req.name) }) + return { + ok: true, + asset: req.assetType === 'skill' + ? { assetType: 'skill', content: r.content, metadata: r.metadata, companions: {} } + : { assetType: req.assetType, content: r.content, metadata: r.metadata }, + } + } catch { + return { ok: false, failure: { code: 'not-found' } } + } }, - async deleteAsset(scope, type, name) { - await deleteAssetFile({ file: path(type, name) }) + async deleteAsset(req) { + const file = path(req.assetType, req.name) + await deleteAssetFile({ file }) + return { ok: true, existed: true, deletedPaths: [file] } }, } `, diff --git a/packages/cli/src/util/__tests__/adapter-install-errors.test.ts b/packages/cli/src/util/__tests__/adapter-install-errors.test.ts index 402316ae..89ce67cf 100644 --- a/packages/cli/src/util/__tests__/adapter-install-errors.test.ts +++ b/packages/cli/src/util/__tests__/adapter-install-errors.test.ts @@ -21,8 +21,8 @@ describe('compatibilityFailureMessage — per-adapter JSON error identity', () = // Substring-adjacent names ("code" ⊂ "claude-code") would mispair // under any .includes()-based matching; the per-failure renderer must not. const failures: AdapterCompatibilityFailure[] = [ - { kind: 'api-missing', adapter: 'code', supported: ['0.0'] }, - { kind: 'api-unsupported', adapter: 'claude-code', found: '9.9', supported: ['0.0'] }, + { kind: 'api-missing', adapter: 'code', supported: ['0.1'] }, + { kind: 'api-unsupported', adapter: 'claude-code', found: '9.9', supported: ['0.1'] }, ] const rows = failures.map((failure) => ({ message: compatibilityFailureMessage(failure), @@ -52,7 +52,7 @@ function noCompatibleReleaseFailure( reason: 'no-compatible-release', packageName: 'pkg', request, - supported: ['0.0'], + supported: ['0.1'], ...(newestConsidered ? { newestConsidered } : {}), }, }, @@ -119,7 +119,7 @@ describe('describeCompatibilityFailure — install target', () => { kind: 'api-unsupported', adapter: 'future-adapter', found: '9.9', - supported: ['0.0'], + supported: ['0.1'], }) expect(described.fix).toContain('facet adapter install future-adapter') }) @@ -129,7 +129,7 @@ describe('describeCompatibilityFailure — install target', () => { { kind: 'api-missing', adapter: '/tmp/facet-adapter-verify-abc123/adapter.mjs', - supported: ['0.0'], + supported: ['0.1'], }, 'my-adapter', ) @@ -149,7 +149,7 @@ describe('describeAdapterInstallFailure — nameless bundle verify failure', () bundlePath, // Nameless bundle: verification falls back to the bundle path // as the adapter identity. - failure: { kind: 'api-missing', adapter: bundlePath, supported: ['0.0'] }, + failure: { kind: 'api-missing', adapter: bundlePath, supported: ['0.1'] }, }, } const described = describeAdapterInstallFailure(failure) diff --git a/packages/engine/src/__tests__/build-pipeline.test.ts b/packages/engine/src/__tests__/build-pipeline.test.ts index ea62d6f0..7f71e97f 100644 --- a/packages/engine/src/__tests__/build-pipeline.test.ts +++ b/packages/engine/src/__tests__/build-pipeline.test.ts @@ -140,13 +140,13 @@ const mockAdapter = defineAdapter({ name: 'mock-adapter', buildAssetMetadata: (data) => ({ ok: true, data: (data ?? {}) as Record }), async installAsset() { - return undefined + return { ok: true as const, primaryPath: '/dev/null' } }, async readAsset() { - return { content: 'Your asset sir...' } + return { ok: true as const, asset: { assetType: 'command' as const, content: 'Your asset sir...' } } }, async deleteAsset() { - return undefined + return { ok: true as const, existed: false, deletedPaths: [] } }, }) @@ -158,13 +158,13 @@ const rejectingAdapter = defineAdapter({ errors: [{ path: 'tools', message: 'Invalid tools config', expected: 'Record', actual: 'string' }], }), async installAsset() { - return undefined + return { ok: true as const, primaryPath: '/dev/null' } }, async readAsset() { - return { content: 'Your asset sir...' } + return { ok: true as const, asset: { assetType: 'command' as const, content: 'Your asset sir...' } } }, async deleteAsset() { - return undefined + return { ok: true as const, existed: false, deletedPaths: [] } }, }) @@ -490,11 +490,15 @@ describe('runBuildPipeline', () => { const mockAdapter = defineAdapter({ name: 'mock-adapter', buildAssetMetadata: (data) => ({ ok: true, data: (data ?? {}) as Record }), - async installAsset(_scope, _assetType, _name, _content, _metadata) {}, - async readAsset(_scope, _assetType, _name) { - return { content: 'Your asset sir...' } + async installAsset() { + return { ok: true as const, primaryPath: '/dev/null' } + }, + async readAsset() { + return { ok: true as const, asset: { assetType: 'command' as const, content: 'Your asset sir...' } } + }, + async deleteAsset() { + return { ok: true as const, existed: false, deletedPaths: [] } }, - async deleteAsset(_scope, _assetType, _name) {}, }) const result = await runBuildPipeline(dir, [mockAdapter]) @@ -534,11 +538,15 @@ describe('runBuildPipeline', () => { }, ], }), - async installAsset(_scope, _assetType, _name, _content, _metadata) {}, - async readAsset(_scope, _assetType, _name) { - return { content: 'Your asset sir...' } + async installAsset() { + return { ok: true as const, primaryPath: '/dev/null' } + }, + async readAsset() { + return { ok: true as const, asset: { assetType: 'command' as const, content: 'Your asset sir...' } } + }, + async deleteAsset() { + return { ok: true as const, existed: false, deletedPaths: [] } }, - async deleteAsset(_scope, _assetType, _name) {}, }) const result = await runBuildPipeline(dir, [rejectingAdapter]) @@ -606,13 +614,13 @@ describe('runBuildPipeline', () => { return { ok: true, data: enrichedData } }, async installAsset() { - return undefined + return { ok: true as const, primaryPath: '/dev/null' } }, async readAsset() { - return { content: 'Your asset sir...' } + return { ok: true as const, asset: { assetType: 'command' as const, content: 'Your asset sir...' } } }, async deleteAsset() { - return undefined + return { ok: true as const, existed: false, deletedPaths: [] } }, }) @@ -1073,6 +1081,23 @@ describe('runBuildPipeline — adapter API preflight', () => { expect(result.failures.map((f) => f.kind)).toEqual(['api-missing', 'api-malformed']) }) + test('a superseded positional 0.0 adapter fails the preflight before any stage', async () => { + // A bundle built against the earlier positional contract declares 0.0, + // which a 0.1-only CLI treats as unsupported. The build fails at the + // preflight before stage 1 and before any contract method is invoked. + const dir = await validFixture('preflight-positional') + const stages: string[] = [] + const result = await runBuildPipeline(dir, [incompatibleAdapter('legacy-positional', '0.0')], (progress) => { + stages.push(progress.stage) + }) + if (result.ok) expect.unreachable() + if (result.kind !== 'adapter-incompatible') expect.unreachable() + expect(result.failures).toEqual([ + { kind: 'api-unsupported', adapter: 'legacy-positional', found: '0.0', supported: [ADAPTER_API_VERSION] }, + ]) + expect(stages).toEqual([]) + }) + test('build with no adapters proceeds and warns about unknown manifest adapters', async () => { const dir = await validFixture('preflight-no-adapters') const result = await runBuildPipeline(dir, []) diff --git a/packages/engine/src/__tests__/materialize.test.ts b/packages/engine/src/__tests__/materialize.test.ts index 33ef9fde..10c5c13b 100644 --- a/packages/engine/src/__tests__/materialize.test.ts +++ b/packages/engine/src/__tests__/materialize.test.ts @@ -32,36 +32,46 @@ function buildRecordingAdapter(name: string): { apiVersion: ADAPTER_API_VERSION, supportsInstall: true, buildAssetMetadata: (data) => ({ ok: true, data: (data ?? {}) as Record }), - async installAsset(_scope, type, n, content, metadata) { - calls.push({ name: n, metadata }) - const file = join(projectRoot, `.${name}`, `${type}s`, `${n}.md`) - mkdirSync(join(projectRoot, `.${name}`, `${type}s`), { recursive: true }) + async installAsset(request) { + calls.push({ name: request.name, metadata: request.metadata }) + const file = join(projectRoot, `.${name}`, `${request.assetType}s`, `${request.name}.md`) + mkdirSync(join(projectRoot, `.${name}`, `${request.assetType}s`), { recursive: true }) // Persist content + metadata as a composite so readAsset can // round-trip them, exercising the materialize skip-if-identical // compare path. - const blob = JSON.stringify({ content, metadata: metadata ?? {} }) + const blob = JSON.stringify({ content: request.content, metadata: request.metadata ?? {} }) writeFileSync(file, blob) + return { ok: true, primaryPath: file } }, - async readAsset(_scope, type, n) { - const file = join(projectRoot, `.${name}`, `${type}s`, `${n}.md`) + async readAsset(request) { + const file = join(projectRoot, `.${name}`, `${request.assetType}s`, `${request.name}.md`) if (!existsSync(file)) { - const err: NodeJS.ErrnoException = new Error('ENOENT') - err.code = 'ENOENT' - throw err + return { ok: false, failure: { code: 'not-found' } } } const blob = readFileSync(file, 'utf8') + let content = blob + let metadata: Record | undefined try { const parsed = JSON.parse(blob) as { content: string; metadata?: Record } - return { content: parsed.content, metadata: parsed.metadata } + content = parsed.content + metadata = parsed.metadata } catch { // Hand-edited file (e.g., the "user edit" test); return raw bytes // so the compare path observes the drift. - return { content: blob } + } + return { + ok: true, + asset: + request.assetType === 'skill' + ? { assetType: 'skill', content, metadata, companions: {} } + : { assetType: request.assetType, content, metadata }, } }, - async deleteAsset(_scope, type, n) { - const file = join(projectRoot, `.${name}`, `${type}s`, `${n}.md`) - if (existsSync(file)) rmSync(file) + async deleteAsset(request) { + const file = join(projectRoot, `.${name}`, `${request.assetType}s`, `${request.name}.md`) + const existed = existsSync(file) + if (existed) rmSync(file) + return { ok: true, existed, deletedPaths: existed ? [file] : [] } }, } return { adapter, calls } @@ -89,15 +99,30 @@ function buildSdkAdapter(name: string): { apiVersion: ADAPTER_API_VERSION, supportsInstall: true, buildAssetMetadata: (data) => ({ ok: true, data: (data ?? {}) as Record }), - async installAsset(_scope, type, n, content, metadata) { + async installAsset(request) { installCalls++ - await installAssetFile(path(type, n), content, metadata as Record | undefined) + const p = path(request.assetType, request.name) + await installAssetFile(p, request.content, request.metadata as Record | undefined) + return { ok: true, primaryPath: p.file } }, - async readAsset(_scope, type, n) { - return readAssetFile(path(type, n)) + async readAsset(request) { + try { + const { content, metadata } = await readAssetFile(path(request.assetType, request.name)) + return { + ok: true, + asset: + request.assetType === 'skill' + ? { assetType: 'skill', content, metadata, companions: {} } + : { assetType: request.assetType, content, metadata }, + } + } catch { + return { ok: false, failure: { code: 'not-found' } } + } }, - async deleteAsset(_scope, type, n) { - await deleteAssetFile(path(type, n)) + async deleteAsset(request) { + const p = path(request.assetType, request.name) + await deleteAssetFile(p) + return { ok: true, existed: true, deletedPaths: [p.file] } }, } return { @@ -467,4 +492,164 @@ describe('materialize — adapter API invariant check', () => { supported: [ADAPTER_API_VERSION], }) }) + + test('a superseded positional 0.0 adapter fails before any method is invoked', async () => { + const manifest: ResolvedFacetManifest = { + name: 'viper-plans', + version: '0.1.0', + skills: { planning: { description: 'planning skill', prompt: '# planning content\n' } }, + } + // A bundle built against the earlier positional contract declares 0.0. + // A 0.1-only CLI must reject it before invoking any contract method, + // exactly as it would any other unsupported API. + const positional = { + name: 'legacy-positional', + apiVersion: '0.0', + supportsInstall: true, + buildAssetMetadata: () => { + throw new Error('contract method invoked despite incompatibility') + }, + async installAsset() { + throw new Error('contract method invoked despite incompatibility') + }, + async readAsset() { + throw new Error('contract method invoked despite incompatibility') + }, + async deleteAsset() { + throw new Error('contract method invoked despite incompatibility') + }, + } as unknown as Adapter + + const result = await materialize({ + facetName: 'viper-plans', + manifest, + adapters: [positional], + oldAssets: [], + newAssets: computeAssetList(manifest), + journal: new InstallJournal(), + }) + if (result.ok) expect.unreachable() + if (result.failure.kind !== 'incompatible-adapter') expect.unreachable() + expect(result.failure.failure).toEqual({ + kind: 'api-unsupported', + adapter: 'legacy-positional', + found: '0.0', + supported: [ADAPTER_API_VERSION], + }) + }) +}) + +describe('materialize — journal undo surfaces structured adapter failures', () => { + test('a failed inverse op is counted by journal.rollback (not silently swallowed)', async () => { + // Two-asset facet: the first install succeeds and records a delete-undo; + // the second install fails, so materialize returns `install-failed`. When + // the caller rolls back, the first asset's inverse delete returns a + // structured `{ ok: false }`. Before the fix, the undo closure ignored + // `result.ok`, so the journal reported a clean rollback while the asset + // was never removed. Now the undo throws, and the journal counts it. + const manifest: ResolvedFacetManifest = { + name: 'viper-plans', + version: '0.1.0', + skills: { + alpha: { description: 'a', prompt: '# a\n' }, + beta: { description: 'b', prompt: '# b\n' }, + }, + } + + let installCount = 0 + const adapter: Adapter = { + name: 'flaky', + apiVersion: ADAPTER_API_VERSION, + supportsInstall: true, + buildAssetMetadata: (data) => ({ ok: true, data: (data ?? {}) as Record }), + async installAsset(request) { + installCount++ + // First forward install (alpha) succeeds; second forward install + // (beta) fails, triggering rollback of alpha. + if (installCount === 2) { + return { ok: false, failure: { code: 'io-failed', operation: 'write', path: request.name, message: 'boom' } } + } + return { ok: true, primaryPath: join(projectRoot, `${request.name}.md`) } + }, + async readAsset() { + // No previous state — both assets are new, so the recorded undo is a + // delete. + return { ok: false, failure: { code: 'not-found' } } + }, + async deleteAsset() { + // The inverse of a new-asset install. Fail it to prove the undo is + // counted rather than swallowed. + return { ok: false, failure: { code: 'io-failed', operation: 'delete', path: 'alpha', message: 'cannot undo' } } + }, + } + + const journal = new InstallJournal() + const newAssets = computeAssetList(manifest) + const result = await materialize({ + facetName: 'viper-plans', + manifest, + adapters: [adapter], + oldAssets: [], + newAssets, + journal, + }) + if (result.ok) expect.unreachable() + expect(result.failure.kind).toBe('install-failed') + + // The successful alpha install left one delete-undo on the journal. + expect(journal.size()).toBe(1) + const rollback = await journal.rollback() + expect(rollback.failures).toBe(1) + expect(rollback.ok).toBe(false) + }) + + test('a successful inverse op replays cleanly', async () => { + const manifest: ResolvedFacetManifest = { + name: 'viper-plans', + version: '0.1.0', + skills: { + alpha: { description: 'a', prompt: '# a\n' }, + beta: { description: 'b', prompt: '# b\n' }, + }, + } + + let installCount = 0 + const deleted: string[] = [] + const adapter: Adapter = { + name: 'flaky', + apiVersion: ADAPTER_API_VERSION, + supportsInstall: true, + buildAssetMetadata: (data) => ({ ok: true, data: (data ?? {}) as Record }), + async installAsset(request) { + installCount++ + if (installCount === 2) { + return { ok: false, failure: { code: 'io-failed', operation: 'write', path: request.name, message: 'boom' } } + } + return { ok: true, primaryPath: join(projectRoot, `${request.name}.md`) } + }, + async readAsset() { + return { ok: false, failure: { code: 'not-found' } } + }, + async deleteAsset(request) { + deleted.push(request.name) + return { ok: true, existed: true, deletedPaths: [join(projectRoot, `${request.name}.md`)] } + }, + } + + const journal = new InstallJournal() + const result = await materialize({ + facetName: 'viper-plans', + manifest, + adapters: [adapter], + oldAssets: [], + newAssets: computeAssetList(manifest), + journal, + }) + if (result.ok) expect.unreachable() + + const rollback = await journal.rollback() + expect(rollback.failures).toBe(0) + expect(rollback.ok).toBe(true) + expect(deleted).toContain('alpha') + }) }) diff --git a/packages/engine/src/__tests__/run-install.test.ts b/packages/engine/src/__tests__/run-install.test.ts index 78a371d8..c01e498d 100644 --- a/packages/engine/src/__tests__/run-install.test.ts +++ b/packages/engine/src/__tests__/run-install.test.ts @@ -43,14 +43,29 @@ function buildFakeAdapter(name: string): Adapter { ok: true, data: (data ?? {}) as Record, }), - async installAsset(_scope, type, n, content, metadata) { - await installAssetFile(path(type, n), content, metadata as Record | undefined) + async installAsset(request) { + const p = path(request.assetType, request.name) + await installAssetFile(p, request.content, request.metadata as Record | undefined) + return { ok: true, primaryPath: p.file } }, - async readAsset(_scope, type, n) { - return readAssetFile(path(type, n)) + async readAsset(request) { + try { + const { content, metadata } = await readAssetFile(path(request.assetType, request.name)) + return { + ok: true, + asset: + request.assetType === 'skill' + ? { assetType: 'skill', content, metadata, companions: {} } + : { assetType: request.assetType, content, metadata }, + } + } catch { + return { ok: false, failure: { code: 'not-found' } } + } }, - async deleteAsset(_scope, type, n) { - await deleteAssetFile(path(type, n)) + async deleteAsset(request) { + const p = path(request.assetType, request.name) + await deleteAssetFile(p) + return { ok: true, existed: true, deletedPaths: [p.file] } }, } return adapter @@ -76,14 +91,29 @@ function buildNestedFakeAdapter(name: string): Adapter { apiVersion: ADAPTER_API_VERSION, supportsInstall: true, buildAssetMetadata: (data) => ({ ok: true, data: (data ?? {}) as Record }), - async installAsset(_scope, type, n, content, metadata) { - await installAssetFile(path(type, n), content, metadata as Record | undefined) + async installAsset(request) { + const p = path(request.assetType, request.name) + await installAssetFile(p, request.content, request.metadata as Record | undefined) + return { ok: true, primaryPath: p.file } }, - async readAsset(_scope, type, n) { - return readAssetFile(path(type, n)) + async readAsset(request) { + try { + const { content, metadata } = await readAssetFile(path(request.assetType, request.name)) + return { + ok: true, + asset: + request.assetType === 'skill' + ? { assetType: 'skill', content, metadata, companions: {} } + : { assetType: request.assetType, content, metadata }, + } + } catch { + return { ok: false, failure: { code: 'not-found' } } + } }, - async deleteAsset(_scope, type, n) { - await deleteAssetFile(path(type, n)) + async deleteAsset(request) { + const p = path(request.assetType, request.name) + await deleteAssetFile(p) + return { ok: true, existed: true, deletedPaths: [p.file] } }, } } @@ -100,25 +130,33 @@ function buildBrokenAdapter(name: string, throwOnCall: number): Adapter { apiVersion: ADAPTER_API_VERSION, supportsInstall: true, buildAssetMetadata: (data) => ({ ok: true, data: (data ?? {}) as Record }), - async installAsset(_scope, type, n, content) { + async installAsset(request) { calls += 1 if (calls >= throwOnCall) throw new Error(`${name}: boom on call ${calls}`) - const file = join(baseDir, `${type}s`, `${n}.md`) - mkdirSync(join(baseDir, `${type}s`), { recursive: true }) - writeFileSync(file, content) + const file = join(baseDir, `${request.assetType}s`, `${request.name}.md`) + mkdirSync(join(baseDir, `${request.assetType}s`), { recursive: true }) + writeFileSync(file, request.content) + return { ok: true, primaryPath: file } }, - async readAsset(_scope, type, n) { - const file = join(baseDir, `${type}s`, `${n}.md`) + async readAsset(request) { + const file = join(baseDir, `${request.assetType}s`, `${request.name}.md`) if (!existsSync(file)) { - const err: NodeJS.ErrnoException = new Error('ENOENT') - err.code = 'ENOENT' - throw err + return { ok: false, failure: { code: 'not-found' } } + } + const content = readFileSync(file, 'utf8') + return { + ok: true, + asset: + request.assetType === 'skill' + ? { assetType: 'skill', content, companions: {} } + : { assetType: request.assetType, content }, } - return { content: readFileSync(file, 'utf8') } }, - async deleteAsset(_scope, type, n) { - const file = join(baseDir, `${type}s`, `${n}.md`) - if (existsSync(file)) rmSync(file) + async deleteAsset(request) { + const file = join(baseDir, `${request.assetType}s`, `${request.name}.md`) + const existed = existsSync(file) + if (existed) rmSync(file) + return { ok: true, existed, deletedPaths: existed ? [file] : [] } }, } as Adapter } @@ -135,15 +173,16 @@ function buildBadReadAdapter(name: string): Adapter { supportsInstall: true, buildAssetMetadata: (data) => ({ ok: true, data: (data ?? {}) as Record }), async installAsset() { - throw new Error('should not be reached: readAsset threw first') + throw new Error('should not be reached: readAsset failed first') }, async readAsset() { - const err: NodeJS.ErrnoException = new Error('EACCES: permission denied') - err.code = 'EACCES' - throw err + return { + ok: false, + failure: { code: 'io-failed', operation: 'read', message: 'EACCES: permission denied' }, + } }, async deleteAsset() { - return undefined + return { ok: true, existed: false, deletedPaths: [] } }, } as Adapter } diff --git a/packages/engine/src/adapters/__tests__/api-compatibility.test.ts b/packages/engine/src/adapters/__tests__/api-compatibility.test.ts index 147e3213..bf1f78c0 100644 --- a/packages/engine/src/adapters/__tests__/api-compatibility.test.ts +++ b/packages/engine/src/adapters/__tests__/api-compatibility.test.ts @@ -42,13 +42,14 @@ describe('classifyApiDeclaration', () => { expect(classifyApiDeclaration(ADAPTER_API_VERSION)).toEqual({ kind: 'supported', api: ADAPTER_API_VERSION }) }) - test.each(['9.9', '1.0', '0.1'])('classifies well-formed but unknown %s as unsupported', (value) => { + test.each(['9.9', '1.0', '0.0'])('classifies well-formed but unknown %s as unsupported', (value) => { expect(classifyApiDeclaration(value)).toEqual({ kind: 'unsupported', api: value }) }) - test('numeric proximity to a supported identifier does not make it supported', () => { - // '0.1' is numerically adjacent to '0.0' but is a different contract. - const result = classifyApiDeclaration('0.1') + test('the superseded positional identifier 0.0 is unsupported by a 0.1-only CLI', () => { + // '0.0' is numerically adjacent to '0.1' but names the earlier + // positional contract — a different, unsupported wire contract. + const result = classifyApiDeclaration('0.0') expect(result.kind).toBe('unsupported') }) diff --git a/packages/engine/src/adapters/__tests__/inspect.test.ts b/packages/engine/src/adapters/__tests__/inspect.test.ts index 8179a6a1..3875ad53 100644 --- a/packages/engine/src/adapters/__tests__/inspect.test.ts +++ b/packages/engine/src/adapters/__tests__/inspect.test.ts @@ -112,7 +112,10 @@ describe('inspectInstalledAdapter — managed', () => { await mkdir(genDir, { recursive: true }) await Bun.write( join(genDir, 'adapter.js'), - await Bun.file(await makeBundle('my-adapter', { apiVersion: '0.1' })).text(), + // '0.0' is the superseded positional contract: well-formed but + // unsupported by a 0.1-only CLI, so support check (which precedes + // metadata equality) classifies it api-unsupported. + await Bun.file(await makeBundle('my-adapter', { apiVersion: '0.0' })).text(), ) await Bun.write( join(baseDir, 'my-adapter', INSTALLATION_RECEIPT_NAME), diff --git a/packages/engine/src/adapters/__tests__/placement-managed.test.ts b/packages/engine/src/adapters/__tests__/placement-managed.test.ts index dd0ec662..d95107cd 100644 --- a/packages/engine/src/adapters/__tests__/placement-managed.test.ts +++ b/packages/engine/src/adapters/__tests__/placement-managed.test.ts @@ -214,12 +214,13 @@ describe('placeAdapterManaged — failure injection', () => { test('metadata/runtime disagreement at the staged path is terminal', async () => { const previous = await installGood() - // Runtime declares the supported API but provenance claims 0.1 — - // the staged re-verification must reject the contradiction. + // Runtime declares the supported API but provenance claims the + // superseded 0.0 — the staged re-verification must reject the + // contradiction (0.0 is unsupported, so support check fires first). const result = await placeAdapterManaged( 'my-adapter', await makeBundle('my-adapter'), - { apiVersion: '0.1', source: npmSource }, + { apiVersion: '0.0', source: npmSource }, baseDir, ) if (result.ok) expect.unreachable() diff --git a/packages/engine/src/adapters/__tests__/verify.test.ts b/packages/engine/src/adapters/__tests__/verify.test.ts index ecfd6fe6..1843c385 100644 --- a/packages/engine/src/adapters/__tests__/verify.test.ts +++ b/packages/engine/src/adapters/__tests__/verify.test.ts @@ -102,6 +102,23 @@ describe('verifyAdapter — ordered checks', () => { }) }) + test('4: superseded positional 0.0 declaration fails as api-unsupported', async () => { + // The exact cutover: a bundle built against the earlier positional + // contract declares 0.0, which is well-formed but unsupported by a + // 0.1-only CLI. It must fail closed before any contract method. + await withBundle(adapterSource(` name: 'legacy-positional', apiVersion: '0.0',`), async (path) => { + const result = await verifyAdapter(path) + if (result.ok) expect.unreachable() + if (result.failure.kind !== 'incompatible') expect.unreachable() + expect(result.failure.failure).toEqual({ + kind: 'api-unsupported', + adapter: 'legacy-positional', + found: '0.0', + supported: [ADAPTER_API_VERSION], + }) + }) + }) + test('4 precedes 5: unsupported runtime declaration wins over metadata equality', async () => { // Even when npm metadata agrees with the runtime declaration, an // unsupported API is classified as unsupported, not as a mismatch. @@ -114,14 +131,19 @@ describe('verifyAdapter — ordered checks', () => { }) test('5: package/runtime disagreement fails as api-metadata-mismatch', async () => { + // Runtime declares the supported API (so the support check passes), + // but the npm package metadata claims a different well-formed token — + // the mismatch check (5) fires. Use a high token that is neither the + // supported API nor the superseded 0.0, so this stays a mismatch + // rather than collapsing into the unsupported-runtime path. await withBundle(adapterSource(` name: 'split-brain', apiVersion: '${ADAPTER_API_VERSION}',`), async (path) => { - const result = await verifyAdapter(path, { expectedApiVersion: '0.1' }) + const result = await verifyAdapter(path, { expectedApiVersion: '9.9' }) if (result.ok) expect.unreachable() if (result.failure.kind !== 'incompatible') expect.unreachable() expect(result.failure.failure).toEqual({ kind: 'api-metadata-mismatch', adapter: 'split-brain', - packageDeclared: '0.1', + packageDeclared: '9.9', runtimeDeclared: ADAPTER_API_VERSION, supported: [ADAPTER_API_VERSION], }) diff --git a/packages/engine/src/adapters/api-compatibility.ts b/packages/engine/src/adapters/api-compatibility.ts index 67d1bafc..bc9bdaa0 100644 --- a/packages/engine/src/adapters/api-compatibility.ts +++ b/packages/engine/src/adapters/api-compatibility.ts @@ -17,9 +17,9 @@ import { ADAPTER_API_VERSION } from '@agent-facets/adapter/api-version' /** * The exact adapter APIs this CLI supports. Derived from the SDK's - * canonical constant — the `0.0` literal lives only in the SDK. A later - * change MAY add further exact identifiers without changing how the SDK - * stamps newly built adapters. + * canonical constant — the current API literal lives only in the SDK. A + * later change MAY add further exact identifiers without changing how the + * SDK stamps newly built adapters. */ export const SUPPORTED_ADAPTER_APIS: readonly string[] = [ADAPTER_API_VERSION] diff --git a/packages/engine/src/adapters/verify.ts b/packages/engine/src/adapters/verify.ts index 5cef0204..ae8d820a 100644 --- a/packages/engine/src/adapters/verify.ts +++ b/packages/engine/src/adapters/verify.ts @@ -24,7 +24,7 @@ export interface VerifiedAdapter { * 3./4./5. `incompatible` — the runtime API declaration is missing, * malformed, unsupported, or disagrees with the npm package * declaration used for selection. - * 6. `invalid-name` / `invalid-shape` — the API `0.0` object is + * 6. `invalid-name` / `invalid-shape` — the verified adapter object is * missing its name or a required method. * * A compatibility contradiction is classified before any adapter @@ -40,15 +40,15 @@ export type VerifyAdapterFailure = /** Result of `verifyAdapter`. Discriminated by `ok`; never throws for expected failures. */ export type VerifyAdapterResult = { ok: true; verified: VerifiedAdapter } | { ok: false; failure: VerifyAdapterFailure } -/** The API `0.0` contract methods every adapter object must expose. */ +/** The contract methods every supported adapter object must expose. */ const REQUIRED_METHODS = ['buildAssetMetadata', 'installAsset', 'readAsset', 'deleteAsset'] as const /** * Verifies that a built adapter.js file exports a valid, compatible * Adapter object. Dynamically imports the file and checks, in order: * importability, default export, runtime API declaration syntax, CLI - * support, optional npm-metadata equality, then the `0.0` name and - * method shape. + * support, optional npm-metadata equality, then the supported adapter's + * name and method shape. * * Importing an ESM bundle necessarily runs its top-level initialization; * no adapter *contract method* is invoked here. diff --git a/packages/engine/src/install/__tests__/run-add.test.ts b/packages/engine/src/install/__tests__/run-add.test.ts index 13db4087..4cd84330 100644 --- a/packages/engine/src/install/__tests__/run-add.test.ts +++ b/packages/engine/src/install/__tests__/run-add.test.ts @@ -98,9 +98,29 @@ export default { apiVersion: '${ADAPTER_API_VERSION}', supportsInstall: true, buildAssetMetadata(data) { return { ok: true, data: data || {} } }, - async installAsset(scope, type, name, content, metadata) { await installAssetFile({ file: path(type, name) }, content, metadata) }, - async readAsset(scope, type, name) { return readAssetFile({ file: path(type, name) }) }, - async deleteAsset(scope, type, name) { await deleteAssetFile({ file: path(type, name) }) }, + async installAsset(req) { + const file = path(req.assetType, req.name) + await installAssetFile({ file }, req.content, req.metadata) + return { ok: true, primaryPath: file } + }, + async readAsset(req) { + try { + const r = await readAssetFile({ file: path(req.assetType, req.name) }) + return { + ok: true, + asset: req.assetType === 'skill' + ? { assetType: 'skill', content: r.content, metadata: r.metadata, companions: {} } + : { assetType: req.assetType, content: r.content, metadata: r.metadata }, + } + } catch { + return { ok: false, failure: { code: 'not-found' } } + } + }, + async deleteAsset(req) { + const file = path(req.assetType, req.name) + await deleteAssetFile({ file }) + return { ok: true, existed: true, deletedPaths: [file] } + }, } `, ) diff --git a/packages/engine/src/install/__tests__/run-install.chain.test.ts b/packages/engine/src/install/__tests__/run-install.chain.test.ts index da056d72..514e06be 100644 --- a/packages/engine/src/install/__tests__/run-install.chain.test.ts +++ b/packages/engine/src/install/__tests__/run-install.chain.test.ts @@ -156,9 +156,29 @@ export default { apiVersion: '${ADAPTER_API_VERSION}', supportsInstall: true, buildAssetMetadata(data) { return { ok: true, data: data || {} } }, - async installAsset(scope, type, name, content, metadata) { await installAssetFile({ file: path(type, name) }, content, metadata) }, - async readAsset(scope, type, name) { return readAssetFile({ file: path(type, name) }) }, - async deleteAsset(scope, type, name) { await deleteAssetFile({ file: path(type, name) }) }, + async installAsset(req) { + const file = path(req.assetType, req.name) + await installAssetFile({ file }, req.content, req.metadata) + return { ok: true, primaryPath: file } + }, + async readAsset(req) { + try { + const r = await readAssetFile({ file: path(req.assetType, req.name) }) + return { + ok: true, + asset: req.assetType === 'skill' + ? { assetType: 'skill', content: r.content, metadata: r.metadata, companions: {} } + : { assetType: req.assetType, content: r.content, metadata: r.metadata }, + } + } catch { + return { ok: false, failure: { code: 'not-found' } } + } + }, + async deleteAsset(req) { + const file = path(req.assetType, req.name) + await deleteAssetFile({ file }) + return { ok: true, existed: true, deletedPaths: [file] } + }, } `, ) diff --git a/packages/engine/src/install/__tests__/run-install.receipt.test.ts b/packages/engine/src/install/__tests__/run-install.receipt.test.ts index 47556f93..7bc32833 100644 --- a/packages/engine/src/install/__tests__/run-install.receipt.test.ts +++ b/packages/engine/src/install/__tests__/run-install.receipt.test.ts @@ -106,9 +106,29 @@ export default { apiVersion: '${ADAPTER_API_VERSION}', supportsInstall: true, buildAssetMetadata(data) { return { ok: true, data: data || {} } }, - async installAsset(scope, type, name, content, metadata) { await installAssetFile({ file: path(type, name) }, content, metadata) }, - async readAsset(scope, type, name) { return readAssetFile({ file: path(type, name) }) }, - async deleteAsset(scope, type, name) { await deleteAssetFile({ file: path(type, name) }) }, + async installAsset(req) { + const file = path(req.assetType, req.name) + await installAssetFile({ file }, req.content, req.metadata) + return { ok: true, primaryPath: file } + }, + async readAsset(req) { + try { + const r = await readAssetFile({ file: path(req.assetType, req.name) }) + return { + ok: true, + asset: req.assetType === 'skill' + ? { assetType: 'skill', content: r.content, metadata: r.metadata, companions: {} } + : { assetType: req.assetType, content: r.content, metadata: r.metadata }, + } + } catch { + return { ok: false, failure: { code: 'not-found' } } + } + }, + async deleteAsset(req) { + const file = path(req.assetType, req.name) + await deleteAssetFile({ file }) + return { ok: true, existed: true, deletedPaths: [file] } + }, } `, ) diff --git a/packages/engine/src/install/__tests__/run-install.test.ts b/packages/engine/src/install/__tests__/run-install.test.ts index de227e79..f16e4939 100644 --- a/packages/engine/src/install/__tests__/run-install.test.ts +++ b/packages/engine/src/install/__tests__/run-install.test.ts @@ -141,9 +141,29 @@ export default { apiVersion: '${ADAPTER_API_VERSION}', supportsInstall: true, buildAssetMetadata(data) { return { ok: true, data: data || {} } }, - async installAsset(scope, type, name, content, metadata) { await installAssetFile({ file: path(type, name) }, content, metadata) }, - async readAsset(scope, type, name) { return readAssetFile({ file: path(type, name) }) }, - async deleteAsset(scope, type, name) { await deleteAssetFile({ file: path(type, name) }) }, + async installAsset(req) { + const file = path(req.assetType, req.name) + await installAssetFile({ file }, req.content, req.metadata) + return { ok: true, primaryPath: file } + }, + async readAsset(req) { + try { + const r = await readAssetFile({ file: path(req.assetType, req.name) }) + return { + ok: true, + asset: req.assetType === 'skill' + ? { assetType: 'skill', content: r.content, metadata: r.metadata, companions: {} } + : { assetType: req.assetType, content: r.content, metadata: r.metadata }, + } + } catch { + return { ok: false, failure: { code: 'not-found' } } + } + }, + async deleteAsset(req) { + const file = path(req.assetType, req.name) + await deleteAssetFile({ file }) + return { ok: true, existed: true, deletedPaths: [file] } + }, } `, ) @@ -545,4 +565,22 @@ describe('runInstall — ADAPTER_INCOMPATIBLE preflight', () => { if (result.failure.code !== 'ADAPTER_INCOMPATIBLE') expect.unreachable() expect(result.failure.failures.map((f) => f.kind)).toEqual(['api-missing', 'api-malformed']) }) + + test('a superseded positional 0.0 adapter fails the preflight before any write', async () => { + // The exact cutover: an adapter built against the earlier positional + // contract declares 0.0. A 0.1-only CLI rejects it at the preflight, + // before any facet processing or state mutation. + const manifestBytes = writeFacets({ 'gate-facet': './some-local-facet' }) + const result = await runInstall({ + projectRoot, + adapters: [incompatibleAdapter('legacy-positional', '0.0')], + }) + if (result.ok) expect.unreachable() + if (result.failure.code !== 'ADAPTER_INCOMPATIBLE') expect.unreachable() + expect(result.failure.failures).toEqual([ + { kind: 'api-unsupported', adapter: 'legacy-positional', found: '0.0', supported: [ADAPTER_API_VERSION] }, + ]) + expect(readFileSync(join(projectRoot, 'facets.json'), 'utf8')).toBe(manifestBytes) + expect(existsSync(join(projectRoot, 'facets.lock'))).toBe(false) + }) }) diff --git a/packages/engine/src/install/__tests__/run-remove.test.ts b/packages/engine/src/install/__tests__/run-remove.test.ts index 3281d878..5363b11b 100644 --- a/packages/engine/src/install/__tests__/run-remove.test.ts +++ b/packages/engine/src/install/__tests__/run-remove.test.ts @@ -98,9 +98,29 @@ export default { apiVersion: '${ADAPTER_API_VERSION}', supportsInstall: true, buildAssetMetadata(data) { return { ok: true, data: data || {} } }, - async installAsset(scope, type, name, content, metadata) { await installAssetFile({ file: path(type, name) }, content, metadata) }, - async readAsset(scope, type, name) { return readAssetFile({ file: path(type, name) }) }, - async deleteAsset(scope, type, name) { await deleteAssetFile({ file: path(type, name) }) }, + async installAsset(req) { + const file = path(req.assetType, req.name) + await installAssetFile({ file }, req.content, req.metadata) + return { ok: true, primaryPath: file } + }, + async readAsset(req) { + try { + const r = await readAssetFile({ file: path(req.assetType, req.name) }) + return { + ok: true, + asset: req.assetType === 'skill' + ? { assetType: 'skill', content: r.content, metadata: r.metadata, companions: {} } + : { assetType: req.assetType, content: r.content, metadata: r.metadata }, + } + } catch { + return { ok: false, failure: { code: 'not-found' } } + } + }, + async deleteAsset(req) { + const file = path(req.assetType, req.name) + await deleteAssetFile({ file }) + return { ok: true, existed: true, deletedPaths: [file] } + }, } `, ) @@ -365,6 +385,38 @@ describe('runRemove — adapter compatibility is not bypassed', () => { expect(readFileSync(join(projectRoot, 'facets.lock'), 'utf8')).toBe(before.lock) expect(readFileSync(receiptPath(projectRoot), 'utf8')).toBe(before.receipt) }) + + test('a superseded positional 0.0 adapter blocks removal before deleting anything', async () => { + // The cutover applies to removal too: a 0.0 adapter is unsupported by + // a 0.1-only CLI, so removal fails before any asset deletion or write. + await installFacet('cowsay', '0.1.1') + expect(existsSync(assetPath('test-adapter', 'cowsay'))).toBe(true) + const { receiptPath } = await import('../receipt.ts') + const before = { + facets: readFileSync(join(projectRoot, 'facets.json'), 'utf8'), + lock: readFileSync(join(projectRoot, 'facets.lock'), 'utf8'), + receipt: readFileSync(receiptPath(projectRoot), 'utf8'), + } + + const result = await runRemove({ + projectRoot, + names: ['cowsay'], + adapters: [incompatibleAdapter('legacy-positional', '0.0')], + }) + + if (result.ok) expect.unreachable() + if (result.phase !== 'install') expect.unreachable() + if (result.install.failure.code !== 'ADAPTER_INCOMPATIBLE') expect.unreachable() + expect(result.install.failure.failures).toEqual([ + { kind: 'api-unsupported', adapter: 'legacy-positional', found: '0.0', supported: [ADAPTER_API_VERSION] }, + ]) + + // Nothing deleted; every project file byte-for-byte unchanged. + expect(existsSync(assetPath('test-adapter', 'cowsay'))).toBe(true) + expect(readFileSync(join(projectRoot, 'facets.json'), 'utf8')).toBe(before.facets) + expect(readFileSync(join(projectRoot, 'facets.lock'), 'utf8')).toBe(before.lock) + expect(readFileSync(receiptPath(projectRoot), 'utf8')).toBe(before.receipt) + }) }) describe('runRemove — install-failure leaves manifest unchanged', () => { diff --git a/packages/engine/src/install/materialize.ts b/packages/engine/src/install/materialize.ts index 9d9e6df9..41d5e2b1 100644 --- a/packages/engine/src/install/materialize.ts +++ b/packages/engine/src/install/materialize.ts @@ -1,4 +1,10 @@ -import type { Adapter } from '@agent-facets/adapter' +import type { + Adapter, + AdapterAssetFailure, + DeleteAssetRequest, + InstallAssetRequest, + ReadAssetRequest, +} from '@agent-facets/adapter' import { splitFrontMatter } from '@agent-facets/common' import type { LockfileAssetEntry, ResolvedFacetManifest } from '@agent-facets/protocol' import { type AdapterCompatibilityFailure, compatibilityFailureFor } from '../adapters/api-compatibility.ts' @@ -93,13 +99,13 @@ export interface MaterializeCounts { * declare a CLI-supported API. Invariant check only: the primary * gates are the command-level fail-closed load and the runInstall * preflight; reaching this arm means an upstream gate was bypassed. - * - `read-failed` — `adapter.readAsset` threw something other than - * ENOENT. ENOENT is the one "file didn't exist" signal we trust; - * anything else (EACCES, EIO, EISDIR, adapter bugs) means we don't - * know whether the asset existed, so the journal must not record - * a delete-undo based on an assumption of absence. - * - `install-failed` — `adapter.installAsset` threw. - * - `delete-failed` — `adapter.deleteAsset` threw. + * - `read-failed` — `adapter.readAsset` returned a failure other than + * `not-found` (or threw, which is an adapter bug). `not-found` is + * the one "asset didn't exist" signal we trust; anything else means + * we don't know whether the asset existed, so the journal must not + * record a delete-undo based on an assumption of absence. + * - `install-failed` — `adapter.installAsset` returned a failure or threw. + * - `delete-failed` — `adapter.deleteAsset` returned a failure or threw. */ export type MaterializeFailure = | { kind: 'unsupported-adapter'; adapter: string } @@ -160,29 +166,19 @@ export async function materialize(opts: MaterializeOptions): Promise } | null = null - try { - previous = await adapter.readAsset(asset.scope, asset.type, asset.name) - } catch (err) { - if (!isFileMissingError(err)) { - return { - ok: false, - failure: { - kind: 'read-failed', - adapter: adapter.name, - asset, - cause: err instanceof Error ? err.message : String(err), - }, - } + // Capture original state for rollback (F14). Treating any failure + // as "didn't exist" would let the journal's delete-undo silently + // delete a pre-existing asset we never read successfully. Narrow to + // the structured `not-found` only and surface everything else as + // `read-failed` — install fails loud before we write anything. + const readOutcome = await readPrevious(adapter, asset) + if (!readOutcome.ok) { + return { + ok: false, + failure: { kind: 'read-failed', adapter: adapter.name, asset, cause: readOutcome.cause }, } - previous = null } + const previous = readOutcome.previous // Skip-if-identical: when the on-disk content + metadata already // matches what we would write, no work is needed and no journal @@ -222,7 +218,19 @@ export async function materialize(opts: MaterializeOptions): Promise { if (previous) { - await adapter.installAsset(asset.scope, asset.type, asset.name, previous.content, previous.metadata ?? {}) + await runUndoInstall(adapter, asset, previous.content, previous.metadata ?? {}) } else { - await adapter.deleteAsset(asset.scope, asset.type, asset.name) + await runUndoDelete(adapter, asset) } }, }) @@ -251,27 +259,30 @@ export async function materialize(opts: MaterializeOptions): Promise } | null = null + const readOutcome = await readPrevious(adapter, asset) + if (!readOutcome.ok) { + return { + ok: false, + failure: { kind: 'read-failed', adapter: adapter.name, asset, cause: readOutcome.cause }, + } + } + const previous = readOutcome.previous + + let deletedPath: string | undefined try { - previous = await adapter.readAsset(asset.scope, asset.type, asset.name) - } catch (err) { - if (!isFileMissingError(err)) { + const result = await adapter.deleteAsset(deleteRequestFor(asset)) + if (!result.ok) { return { ok: false, failure: { - kind: 'read-failed', + kind: 'delete-failed', adapter: adapter.name, asset, - cause: err instanceof Error ? err.message : String(err), + cause: describeAssetFailure(result.failure), }, } } - previous = null - } - - let deletedPath: string | undefined - try { - deletedPath = await adapter.deleteAsset(asset.scope, asset.type, asset.name) + deletedPath = result.deletedPaths[0] } catch (err) { return { ok: false, @@ -290,7 +301,7 @@ export async function materialize(opts: MaterializeOptions): Promise { - await adapter.installAsset(asset.scope, asset.type, asset.name, previous.content, previous.metadata ?? {}) + await runUndoInstall(adapter, asset, previous.content, previous.metadata ?? {}) }, }) } @@ -303,14 +314,113 @@ export async function materialize(opts: MaterializeOptions): Promise } | null } | { ok: false; cause: string } +> { + try { + const result = await adapter.readAsset(readRequestFor(asset)) + if (result.ok) { + return { ok: true, previous: { content: result.asset.content, metadata: result.asset.metadata } } + } + if (result.failure.code === 'not-found') return { ok: true, previous: null } + return { ok: false, cause: describeAssetFailure(result.failure) } + } catch (err) { + return { ok: false, cause: err instanceof Error ? err.message : String(err) } + } +} + +/** + * Run an inverse install during rollback. The adapter contract returns + * structured failures rather than throwing, but the journal counts an undo + * as failed only when it *throws*. So a `{ ok: false }` inverse op — which + * leaves on-disk state un-restored — must be surfaced as a throw here, or + * `InstallJournal.rollback()` would report a clean rollback while an asset + * was never restored. A thrown adapter bug propagates unchanged. */ -function isFileMissingError(err: unknown): boolean { - if (typeof err !== 'object' || err === null) return false - const code = (err as { code?: unknown }).code - return code === 'ENOENT' +async function runUndoInstall( + adapter: Adapter, + asset: LockfileAssetEntry, + content: string, + metadata: Record, +): Promise { + const result = await adapter.installAsset(installRequestFor(asset, content, metadata)) + if (!result.ok) { + throw new Error( + `undo install ${adapter.name}:${asset.type}:${asset.name} failed: ${describeAssetFailure(result.failure)}`, + ) + } +} + +/** Inverse delete during rollback. Same throw-on-`{ ok: false }` rule as {@link runUndoInstall}. */ +async function runUndoDelete(adapter: Adapter, asset: LockfileAssetEntry): Promise { + const result = await adapter.deleteAsset(deleteRequestFor(asset)) + if (!result.ok) { + throw new Error( + `undo delete ${adapter.name}:${asset.type}:${asset.name} failed: ${describeAssetFailure(result.failure)}`, + ) + } +} + +/** Render a structured adapter failure as a one-line cause string. */ +function describeAssetFailure(failure: AdapterAssetFailure): string { + switch (failure.code) { + case 'not-found': + return 'asset not found' + case 'invalid-companion-path': + return `invalid companion path "${failure.path}": ${failure.reason}` + case 'unsupported-scope': + return `scope "${failure.scope}" is not supported by this adapter` + case 'not-implemented': + return `adapter does not implement ${failure.method}` + case 'io-failed': + return `${failure.operation} failed${failure.path ? ` at ${failure.path}` : ''}: ${failure.message}` + } } function contentFor(manifest: ResolvedFacetManifest, asset: LockfileAssetEntry): string { From 38411839b00355c69454aec0b46ad0d8caa6065c Mon Sep 17 00:00:00 2001 From: Julian Coy Date: Thu, 23 Jul 2026 23:55:55 -0400 Subject: [PATCH 2/2] Update custom-adapters.mdx --- docs/guides/custom-adapters.mdx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/guides/custom-adapters.mdx b/docs/guides/custom-adapters.mdx index 01dde08a..c3b6d54b 100644 --- a/docs/guides/custom-adapters.mdx +++ b/docs/guides/custom-adapters.mdx @@ -282,7 +282,7 @@ Declare the adapter API version your SDK release stamps at runtime (currently `0 ## Share it upstream -Built an adapter for a major AI coding assistant? Contributions are welcome. If your adapter targets a widely used tool, open a pull request on the [`facets repository`](https://github.com/agent-facets/facets) to share it upstream as a first-party adapter. +Built an adapter for a major AI coding assistant? Contributions are welcome. If your adapter targets a widely used tool, open a pull request on the [facets repository](https://github.com/agent-facets/facets) to share it upstream as a first-party adapter. A first-party adapter is installable by name (`facet adapter install `) via the CLI, so everyone using that tool benefits.