From bd98f1a01014dcd88c18208b39fd4301c42985e3 Mon Sep 17 00:00:00 2001 From: Peter Krenesky Date: Sat, 27 Jun 2026 15:26:45 -0700 Subject: [PATCH 1/4] feat: resolve npm sources via npm pack MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Land npm source resolution in `resolveSource`. An `npm` source is now `npm pack`ed and extracted under `/npm/`; the resolved published version acts as the durable `sha` pin (mirroring a git sha) and the requested `version` is echoed as `ref`. Exact-version pins are served from cache; unpinned/range specs re-fetch to honor "latest". Both the `GitRunner` and the new `NpmFetcher` are injectable for testing. This stalled previously because the network-only `defaultNpmFetcher` could not reach 100% coverage. Resolved by extracting a pure, exported `npmPackArgs` helper (unit-tested offline for the version + `--registry` branches) and an offline integration test that `npm pack`s a local folder. `defaultNpmFetcher` and `type NpmFetcher` are added to the public barrel; `npmPackArgs` stays internal to `resolve.ts`. Backsync the spec to match: FR-004 (npm behavior, `defaultNpmFetcher`, CON-1, new AC-8..AC-11), NFR-003 (per-source package-manager subprocess as the side effect, zero-git settled path intact), spec.md (§2.1/§2.2/§3/§8/§10), and tests.md (TC-007 retitled url-only; new TC-022..TC-025). TC-022..054 is a shared block also used by the concurrent feat/plugin-discovery PR and will be reconciled at the second merge (this PR lands first). Gates: tsc clean, 27 tests pass, 100% coverage (branches/functions/lines/ statements), eslint + prettier clean, quire validate 0 errors. Co-Authored-By: Claude Opus 4.8 (1M context) --- spec/functional/FR-004-source-resolution.md | 65 ++++-- .../NFR-003-synchronous-zero-git-hot-path.md | 26 ++- spec/spec.md | 61 +++--- spec/tests.md | 189 ++++++++++-------- src/index.ts | 2 + src/resolve.ts | 103 +++++++++- tests/index.test.ts | 145 +++++++++++++- 7 files changed, 444 insertions(+), 147 deletions(-) diff --git a/spec/functional/FR-004-source-resolution.md b/spec/functional/FR-004-source-resolution.md index 207baa1..d227b50 100644 --- a/spec/functional/FR-004-source-resolution.md +++ b/spec/functional/FR-004-source-resolution.md @@ -18,10 +18,13 @@ relationships: The library SHALL export `resolveSource(source, opts)` which synchronously fetches a source to a local directory and returns a `ResolvedSource` (`dir`, `sha?`, -`ref?`). `git` (via the injectable `GitRunner`, default `defaultGitRunner` using -`execFileSync`) SHALL be the only side effect. `ResolveOptions` carries a -`cacheRoot` (sources cached under `/git/`) and an optional `git` -runner. The function SHALL validate the source via `normalizeSource` ([FR-001](./FR-001-typed-source-union.md)) and +`ref?`). A per-source package-manager subprocess SHALL be the only side effect: +`git` for git sources (via the injectable `GitRunner`, default `defaultGitRunner` +using `execFileSync`); `npm pack` + `tar` for `npm` sources (via the injectable +`NpmFetcher`, default `defaultNpmFetcher`). `ResolveOptions` carries a `cacheRoot` +(git sources cached under `/git/`, npm sources under +`/npm/`), an optional `git` runner, and an optional `npm` fetcher. +The function SHALL validate the source via `normalizeSource` ([FR-001](./FR-001-typed-source-union.md)) and expand git URLs via `toGitUrl` ([FR-002](./FR-002-git-url-shorthand.md)). ## Behavior @@ -31,8 +34,16 @@ expand git URLs via `toGitUrl` ([FR-002](./FR-002-git-url-shorthand.md)). - **`path` source**: resolve `source.path` to an absolute path; if it does not exist, throw `SourceError("path source not found: ")`; otherwise return `{ dir }` (no `sha`, no `ref`). -- **`url` / `npm` source**: throw `UnsupportedSourceError` (message mentions the - type is not yet supported) — these reserved types are not resolved. +- **`url` source**: throw `UnsupportedSourceError` (message mentions the type is + not yet supported) — this reserved type is not resolved. +- **`npm` source**: cache under `/npm/` (key sanitized from + `@`); invoke the `NpmFetcher` to `npm pack` the + package tarball and extract it, then return `{ dir, sha, ref }` where `dir` is + the extracted content root, `sha` is the **resolved published version** (the + durable pin, mirroring a git sha), and `ref` echoes back the requested + `source.version`. An **exact-version** pin (`X.Y.Z[-+…]`) whose extracted + `package.json` and recorded version are already cached is reused without + re-fetching; an unpinned or range spec re-fetches every time to honor "latest". - **git sources** (`github`, `git`, `git-subdir`): - Compute the clone URL via `toGitUrl(source.repo | source.url)` and the cache dir `/git/` (non-`[A-Za-z0-9._-]` chars replaced @@ -68,24 +79,40 @@ spec.md §14 Known Limitations). "pipe"] })`; an injected `GitRunner` replaces it so tests run with no real git (FR-004-AC-7). +**`defaultNpmFetcher`.** The default fetcher runs `npm pack --pack-destination + --json` (where `` is `@` when a version is given, +else ``, plus `--registry ` when supplied), parses the JSON +metadata for the tarball `filename` and resolved `version`, then extracts the +tarball with `tar -xzf -C ` and returns `{ dir: +/package, version }`. The `npm pack` argv is built by the pure, exported +`npmPackArgs(spec, destDir)` helper so the version and `--registry` branches are +unit-testable offline (FR-004-AC-11). `npm pack` of a **local folder** runs +offline (no registry), which is how the default fetcher is integration-tested +(FR-004-AC-10). An injected `NpmFetcher` replaces the default so tests run with no +real `npm`/network (FR-004-AC-8, -AC-9). + ## Constraints -| ID | Constraint | Type | Validation | -| ------------ | ----------------------------------------------------------------------------------------------------------------------------------- | ------------- | ------------- | -| FR-004-CON-1 | Git is the sole side effect; resolution performs no other I/O beyond filesystem reads/dir creation and the `git` subprocess. | architectural | Test (TC-011) | -| FR-004-CON-2 | The clone is blobless and no-checkout (`--filter=blob:none --no-checkout`); subdir sources sparse-checkout only the requested path. | performance | Test (TC-008) | +| ID | Constraint | Type | Validation | +| ------------ | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------------- | ------------- | +| FR-004-CON-1 | The only side effect is the per-source package-manager subprocess (`git` for git sources; `npm pack` + `tar` for npm sources); resolution performs no other network or I/O beyond filesystem reads/dir creation. | architectural | Test (TC-011) | +| FR-004-CON-2 | The clone is blobless and no-checkout (`--filter=blob:none --no-checkout`); subdir sources sparse-checkout only the requested path. | performance | Test (TC-008) | ## Acceptance Criteria -| ID | Criteria | Verification | -| ----------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------------- | -| FR-004-AC-1 | A `path` source for an existing directory returns `{ dir }` pointing at that directory (its contents are readable). | Test (TC-006) | -| FR-004-AC-2 | A `path` source for a non-existent directory throws `SourceError`. | Test (TC-006) | -| FR-004-AC-3 | A `url` source and an `npm` source each throw `UnsupportedSourceError`. | Test (TC-007) | -| FR-004-AC-4 | A `git-subdir` source pinned to a tag returns `dir` ending in the subdir path, contains the subdir's files, and reports the tag's `sha` and the requested `ref`. | Test (TC-008) | -| FR-004-AC-5 | A whole-repo `git` source with no pin resolves to `HEAD` (latest commit); re-resolving the same cached URL at a tag exercises the fetch branch and resolves that tag's sha. | Test (TC-009) | -| FR-004-AC-6 | A `git` source pinned by `sha` checks out exactly that commit. | Test (TC-010) | -| FR-004-AC-7 | A `github` source resolved with an injected `GitRunner` performs no real git: the returned `sha` is the runner's output and the first git argv is `clone`. | Test (TC-011) | +| ID | Criteria | Verification | +| ------------ | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------------- | +| FR-004-AC-1 | A `path` source for an existing directory returns `{ dir }` pointing at that directory (its contents are readable). | Test (TC-006) | +| FR-004-AC-2 | A `path` source for a non-existent directory throws `SourceError`. | Test (TC-006) | +| FR-004-AC-3 | A `url` source throws `UnsupportedSourceError`. | Test (TC-007) | +| FR-004-AC-4 | A `git-subdir` source pinned to a tag returns `dir` ending in the subdir path, contains the subdir's files, and reports the tag's `sha` and the requested `ref`. | Test (TC-008) | +| FR-004-AC-5 | A whole-repo `git` source with no pin resolves to `HEAD` (latest commit); re-resolving the same cached URL at a tag exercises the fetch branch and resolves that tag's sha. | Test (TC-009) | +| FR-004-AC-6 | A `git` source pinned by `sha` checks out exactly that commit. | Test (TC-010) | +| FR-004-AC-7 | A `github` source resolved with an injected `GitRunner` performs no real git: the returned `sha` is the runner's output and the first git argv is `clone`. | Test (TC-011) | +| FR-004-AC-8 | An `npm` source resolved with an injected `NpmFetcher` extracts the tarball content (manifest present at the returned `dir`) and pins the **resolved version** as `sha`, echoing the requested version as `ref`. | Test (TC-022) | +| FR-004-AC-9 | An **exact-version** `npm` pin is served from cache on a second resolve (fetcher invoked once); an **unpinned** spec re-fetches every resolve (fetcher invoked each time) and pins the resolved version returned by the fetcher. | Test (TC-023) | +| FR-004-AC-10 | `defaultNpmFetcher` packs and extracts a **local** package folder via `npm pack` + `tar`, fully offline, returning the package version and an extracted content root containing `package.json`. | Test (TC-024) | +| FR-004-AC-11 | `npmPackArgs` builds the correct argv for a pinned spec (`@`), an unpinned spec (``), and includes `--registry ` when a registry is supplied. | Test (TC-025) | ## Dependencies diff --git a/spec/non-functional/NFR-003-synchronous-zero-git-hot-path.md b/spec/non-functional/NFR-003-synchronous-zero-git-hot-path.md index 2dccf9e..0ebb57b 100644 --- a/spec/non-functional/NFR-003-synchronous-zero-git-hot-path.md +++ b/spec/non-functional/NFR-003-synchronous-zero-git-hot-path.md @@ -14,19 +14,20 @@ relationships: ## Statement -All operations SHALL be **synchronous**, with the `git` subprocess (via -`GitRunner`) as the only external side effect; the library SHALL NOT introduce -async/Promise APIs or non-git network calls. A `lazy` reconcile of an +All operations SHALL be **synchronous**, with a per-source package-manager +subprocess — `git` (via `GitRunner`) and, for npm sources, `npm pack` + `tar` (via +`NpmFetcher`) — as the only external side effect; the library SHALL NOT introduce +async/Promise APIs or any other network calls. A `lazy` reconcile of an already-settled manifest SHALL perform **zero** git invocations so it is safe on a per-CLI-invocation hot path. ## Measurement and Evaluation -| Metric | Target | Threshold | Method | -| ------------------------------------------------------------- | ------ | --------- | ---------- | -| Git invocations on a 2nd lazy reconcile of a settled manifest | 0 | 0 | Test | -| Promise-returning functions in the public API | 0 | 0 | Inspection | -| Non-git external side effects in `resolveSource` | 0 | 0 | Analysis | +| Metric | Target | Threshold | Method | +| ---------------------------------------------------------------------- | ------ | --------- | ---------- | +| Git invocations on a 2nd lazy reconcile of a settled manifest | 0 | 0 | Test | +| Promise-returning functions in the public API | 0 | 0 | Inspection | +| External side effects in `resolveSource` beyond `git`/`npm pack`+`tar` | 0 | 0 | Analysis | ## Verification @@ -34,6 +35,9 @@ per-CLI-invocation hot path. settled manifest asserts the count is exactly `0` ([FR-007-AC-2](../functional/FR-007-reconcile.md)). - Inspect the public API in `src/index.ts`: every exported function is synchronous (no `async`, no `Promise<...>` return types). -- `resolveSource` performs only filesystem reads/dir creation and the injected - `git` subprocess; with an injected fake `GitRunner` it resolves with no real git - ([FR-004-AC-7](../functional/FR-004-source-resolution.md)). +- `resolveSource` performs only filesystem reads/dir creation and the per-source + package-manager subprocess (`git`; or `npm pack` + `tar` for npm sources); with + an injected fake `GitRunner` it resolves git sources with no real git + ([FR-004-AC-7](../functional/FR-004-source-resolution.md)), and with an injected + fake `NpmFetcher` it resolves npm sources with no real `npm`/network + ([FR-004-AC-8](../functional/FR-004-source-resolution.md)). diff --git a/spec/spec.md b/spec/spec.md index f49ddf3..3611f65 100644 --- a/spec/spec.md +++ b/spec/spec.md @@ -62,16 +62,19 @@ and reconciling a manifest's default set into a host-owned target directory.** This specification governs: - **Typed `Source` union and validation** — the `github` / `git-subdir` / `git` / - `path` source descriptors (implemented) and the reserved `npm` / `url` - descriptors, `normalizeSource` structural validation, and the `toGitUrl` + `path` / `npm` source descriptors (implemented) and the reserved `url` + descriptor, `normalizeSource` structural validation, and the `toGitUrl` `owner/repo` shorthand expansion ([FR-001](./functional/FR-001-typed-source-union.md), [FR-002](./functional/FR-002-git-url-shorthand.md)). - **Marketplace manifest validation** — `MarketplaceManifest` / `MarketplaceEntry` shapes and `validateMarketplaceManifest`, which validates a host-parsed plain object (the host owns YAML/JSON parsing, keeping this library dep-free) ([FR-003](./functional/FR-003-manifest-validation.md)). -- **Source resolution** — `resolveSource`, a synchronous, `git`-only fetcher that - blobless-clones, sparse-checks-out subdirs, checks out a sha/ref/HEAD, returns a - durable `{dir, sha, ref}` pin, and throws `UnsupportedSourceError` for reserved - types; the `GitRunner` is injectable for testing ([FR-004](./functional/FR-004-source-resolution.md)). +- **Source resolution** — `resolveSource`, a synchronous fetcher that, for git + sources, blobless-clones, sparse-checks-out subdirs, checks out a sha/ref/HEAD, + and returns a durable `{dir, sha, ref}` pin, and, for `npm` sources, `npm +pack`s + extracts the tarball under `/npm/` and pins the resolved + published version as the `sha`; it throws `UnsupportedSourceError` for the + reserved `url` type. The `GitRunner` and `NpmFetcher` are injectable for testing + ([FR-004](./functional/FR-004-source-resolution.md)). - **Install registry** — `InstalledPlugin` / `PluginRegistry`, `readRegistry` / `writeRegistry` (atomic temp+rename), and `upsertPlugin` (last-write-wins by name) ([FR-005](./functional/FR-005-install-registry.md)). @@ -89,10 +92,10 @@ This specification does not govern: - **YAML / JSON parsing.** The host parses manifest text and passes a plain object to `validateMarketplaceManifest`; this library adds no parser dependency. -- **`npm` and `url` source resolution.** The `npm` and `url` descriptors exist in - the type so hosts can build install specs (e.g. an oclif `plugins:install` - bridge), but their resolution is deliberately deferred — `resolveSource` throws - `UnsupportedSourceError` ([FR-004](./functional/FR-004-source-resolution.md)). +- **`url` source resolution.** The `url` descriptor exists in the type so hosts can + build install specs, but its resolution is deliberately deferred — + `resolveSource` throws `UnsupportedSourceError` ([FR-004](./functional/FR-004-source-resolution.md)). (The + `npm` descriptor **is** now resolved, via `npm pack` + `tar`.) - **What a "name" means.** The library never inspects payload contents to derive a module name; the host supplies a `readName(dir)` callback ([FR-006](./functional/FR-006-single-entry-install.md)). - **What the materialized files are for.** The library copies/symlinks bytes into @@ -112,17 +115,18 @@ This specification does not govern: `@agent-ix/ts-plugin-kit` is a single publishable TypeScript library (npm package `@agent-ix/ts-plugin-kit`) with **zero runtime dependencies**. It exposes the building blocks below as pure ES-module functions; every operation is synchronous -and `git` (via `execFileSync`) is the only side effect. - -| Module | Responsibility | -| -------------- | ---------------------------------------------------------------------------------------------------- | -| `sources.ts` | `Source` union, `SourceType`, `SourceError`, `UnsupportedSourceError`, `normalizeSource`, `toGitUrl` | -| `manifest.ts` | `MarketplaceEntry`, `MarketplaceManifest`, `ManifestError`, `validateMarketplaceManifest` | -| `resolve.ts` | `GitRunner`, `defaultGitRunner`, `ResolveOptions`, `ResolvedSource`, `resolveSource` | -| `registry.ts` | `InstalledPlugin`, `PluginRegistry`, `readRegistry`, `writeRegistry`, `upsertPlugin` | -| `install.ts` | `InstallOptions`, `installEntry` | -| `reconcile.ts` | `ReconcileOptions`, `ReconcileResult`, `reconcile` | -| `index.ts` | The public barrel re-exporting the above | +and the only side effect is a per-source package-manager subprocess (via +`execFileSync`): `git` for git sources, and `npm pack` + `tar` for `npm` sources. + +| Module | Responsibility | +| -------------- | -------------------------------------------------------------------------------------------------------------------------------------- | +| `sources.ts` | `Source` union, `SourceType`, `SourceError`, `UnsupportedSourceError`, `normalizeSource`, `toGitUrl` | +| `manifest.ts` | `MarketplaceEntry`, `MarketplaceManifest`, `ManifestError`, `validateMarketplaceManifest` | +| `resolve.ts` | `GitRunner`, `defaultGitRunner`, `NpmFetcher`, `defaultNpmFetcher`, `npmPackArgs`, `ResolveOptions`, `ResolvedSource`, `resolveSource` | +| `registry.ts` | `InstalledPlugin`, `PluginRegistry`, `readRegistry`, `writeRegistry`, `upsertPlugin` | +| `install.ts` | `InstallOptions`, `installEntry` | +| `reconcile.ts` | `ReconcileOptions`, `ReconcileResult`, `reconcile` | +| `index.ts` | The public barrel re-exporting the above | ### 3.2 Intended Users @@ -239,7 +243,7 @@ A plugin/marketplace `source` is a discriminated union keyed by `type` ([FR-001] | `git` | `url`, `ref?`, `sha?` | git clone of the whole repo at pin | | `path` | `path` | local directory (dev); returned as-is | | `url` | `url`, `ref?`, `sha?` | **reserved** — `UnsupportedSourceError` | -| `npm` | `package`, `version?`, `registry?` | **reserved** — `UnsupportedSourceError` (hosts may build install specs from it) | +| `npm` | `package`, `version?`, `registry?` | `npm pack` tarball + extract; the resolved published version is the durable pin | `normalizeSource` validates that the required string fields of each variant are non-empty, throwing `SourceError` on malformed input ([FR-001](./functional/FR-001-typed-source-union.md)). `toGitUrl` passes @@ -250,9 +254,12 @@ shorthand to `https://github.com/owner/repo.git` ([FR-002](./functional/FR-002-g `resolveSource` records a **durable commit sha** as the pin for every git source, in addition to echoing back the requested `ref` ([FR-004](./functional/FR-004-source-resolution.md)). Resolution checks out -`source.sha ?? source.ref ?? "HEAD"` in that precedence. Because the resolved sha -is persisted in the registry record, drift can be detected (sync mode) and a -settled lazy reconcile can short-circuit with no git at all ([FR-007](./functional/FR-007-reconcile.md), [NFR-003](./non-functional/NFR-003-synchronous-zero-git-hot-path.md)). +`source.sha ?? source.ref ?? "HEAD"` in that precedence. For an `npm` source, the +**resolved published version** plays the role of the `sha` pin (and the requested +`source.version` is echoed as `ref`); exact-version pins are reused from the npm +cache, while unpinned/range specs re-fetch to honor "latest". Because the resolved +sha/version is persisted in the registry record, drift can be detected (sync mode) +and a settled lazy reconcile can short-circuit with no git at all ([FR-007](./functional/FR-007-reconcile.md), [NFR-003](./non-functional/NFR-003-synchronous-zero-git-hot-path.md)). --- @@ -295,8 +302,8 @@ plugin data dir ([NFR-004](./non-functional/NFR-004-cache-target-isolation.md)). - `SourceError` — a structurally invalid source descriptor, or a `path` source whose directory does not exist ([FR-001](./functional/FR-001-typed-source-union.md), [FR-004](./functional/FR-004-source-resolution.md)). -- `UnsupportedSourceError` — a valid-but-reserved `url` / `npm` source passed to - `resolveSource` ([FR-004](./functional/FR-004-source-resolution.md)). +- `UnsupportedSourceError` — a valid-but-reserved `url` source passed to + `resolveSource` (the `npm` source is now resolved) ([FR-004](./functional/FR-004-source-resolution.md)). - `ManifestError` — a malformed manifest object or entry passed to `validateMarketplaceManifest` ([FR-003](./functional/FR-003-manifest-validation.md)). - A missing or **shape-invalid** registry file (valid JSON whose `plugins` is not diff --git a/spec/tests.md b/spec/tests.md index 9647f7e..1ac2e53 100644 --- a/spec/tests.md +++ b/spec/tests.md @@ -19,18 +19,24 @@ Every Test Case (TC) below references a real test in `tests/index.test.ts` by it `describe` / `test` string. No TC is aspirational — each maps to code that runs today under `make test` (vitest) at the 100% coverage gate (NFR-002). +> **Concurrent-PR note.** TC-022…TC-054 are a shared TC-ID block also claimed by +> the concurrent `feat/plugin-discovery` PR. This (`feat/npm-source-resolution`) +> PR lands first and uses TC-022…TC-025 for npm source resolution; the two +> matrices will be **reconciled at the second merge** so the IDs do not collide. + Tests fall into the following types: -| Type | Description | -| ----------- | ------------------------------------------------------------------- | -| Unit | Pure-function / in-memory tests. Run under vitest. | -| Unit (git) | Unit tests that drive a **local bare git fixture** (no network). | -| Unit (fake) | Unit tests using an injected fake `GitRunner` (no real git at all). | +| Type | Description | +| ----------- | -------------------------------------------------------------------------------------------------------------------------- | +| Unit | Pure-function / in-memory tests. Run under vitest. | +| Unit (git) | Unit tests that drive a **local bare git fixture** (no network). | +| Unit (fake) | Unit tests using an injected fake `GitRunner` / `NpmFetcher` (no real git/npm). | +| Unit (npm) | Unit tests that run the real `defaultNpmFetcher` against a **local package folder** via `npm pack` (offline, no registry). | -There are **no** integration tests: the library's only side effect is the local -`git` subprocess, exercised against a temp bare repo created in `beforeAll`. The -`url` / `npm` source types are reserved and verified only via their -`UnsupportedSourceError` (TC-007). +The library's side effects are the local `git` subprocess (exercised against a +temp bare repo created in `beforeAll`) and, for npm sources, `npm pack` + `tar` +(exercised offline against a local package folder, TC-024). The `url` source type +is reserved and verified only via its `UnsupportedSourceError` (TC-007). --- @@ -73,44 +79,48 @@ partial clones work offline. ## Functional Requirement Coverage -| Functional Req | Acceptance Criteria | Test Case · Case String | Coverage Status | -| -------------- | ----------------------------------------------------------------- | ---------------------------------------------------------------------------------------------------------------------- | --------------- | -| FR-001 | AC-1: all six valid source shapes accepted | TC-001 — `normalizeSource › "accepts every valid shape"` | ✅ Unit | -| FR-001 | AC-2: null / no-`type` → SourceError | TC-002 — `normalizeSource › "rejects malformed input"` | ✅ Unit | -| FR-001 | AC-3: missing required field → SourceError naming the field | TC-002 — `normalizeSource › "rejects malformed input"` | ✅ Unit | -| FR-001 | AC-4: unknown type → SourceError "unknown source type" | TC-002 — `normalizeSource › "rejects malformed input"` | ✅ Unit | -| FR-002 | AC-1: `owner/repo` → GitHub https URL | TC-003 — `"toGitUrl expands shorthand and passes through URLs"` | ✅ Unit | -| FR-002 | AC-2: `owner/repo.git` strips trailing `.git` | TC-003 — `"toGitUrl expands shorthand and passes through URLs"` | ✅ Unit | -| FR-002 | AC-3: full `https://` URL passes through | TC-003 — `"toGitUrl expands shorthand and passes through URLs"` | ✅ Unit | -| FR-002 | AC-4: `git@…` scp-style URL passes through | TC-003 — `"toGitUrl expands shorthand and passes through URLs"` | ✅ Unit | -| FR-002 | AC-5: surrounding whitespace is trimmed | TC-003 — `"toGitUrl expands shorthand and passes through URLs"` (padded-input assertion) | ✅ Unit | -| FR-003 | AC-1: valid manifest with name round-trips | TC-004 — `validateMarketplaceManifest › "accepts a valid manifest (with and without a name)"` | ✅ Unit | -| FR-003 | AC-2: missing name → `name === undefined` | TC-004 — `validateMarketplaceManifest › "accepts a valid manifest (with and without a name)"` | ✅ Unit | -| FR-003 | AC-3: null / non-object manifest → ManifestError | TC-005 — `validateMarketplaceManifest › "rejects malformed manifests and entries"` | ✅ Unit | -| FR-003 | AC-4: bad schemaVersion / non-array entries → ManifestError | TC-005 — `validateMarketplaceManifest › "rejects malformed manifests and entries"` | ✅ Unit | -| FR-003 | AC-5: null entry / missing entry name → ManifestError | TC-005 — `validateMarketplaceManifest › "rejects malformed manifests and entries"` | ✅ Unit | -| FR-003 | AC-6: invalid entry source → SourceError | TC-005 — `validateMarketplaceManifest › "rejects malformed manifests and entries"` | ✅ Unit | -| FR-004 | AC-1: path source returns the dir | TC-006 — `resolveSource › "path source returns the dir; missing path throws"` | ✅ Unit | -| FR-004 | AC-2: missing path → SourceError | TC-006 — `resolveSource › "path source returns the dir; missing path throws"` | ✅ Unit | -| FR-004 | AC-3: url / npm → UnsupportedSourceError | TC-007 — `resolveSource › "url and npm sources are not yet supported"` | ✅ Unit | -| FR-004 | AC-4: git-subdir sparse-checkout at tag → dir/sha/ref | TC-008 — `resolveSource › "git-subdir sparse-checks out only the subdir at a tag"` | ✅ Unit (git) | -| FR-004 | AC-5: whole-repo HEAD when unpinned; re-fetch existing cache | TC-009 — `resolveSource › "whole-repo git resolves to HEAD when unpinned, and re-fetches an existing cache"` | ✅ Unit (git) | -| FR-004 | AC-6: sha pin checks out the exact commit | TC-010 — `resolveSource › "sha pin checks out the exact commit"` | ✅ Unit (git) | -| FR-004 | AC-7: github + injected runner needs no real git | TC-011 — `resolveSource › "github source + injected runner needs no real git"` | ✅ Unit (fake) | -| FR-004 | CON-1: git is the sole side effect | TC-011 — `resolveSource › "github source + injected runner needs no real git"` | ✅ Unit (fake) | -| FR-004 | CON-2: blobless + sparse (subdir only) | TC-008 — `resolveSource › "git-subdir sparse-checks out only the subdir at a tag"` | ✅ Unit (git) | -| FR-005 | AC-1: missing / shape-invalid (`{}`) registry read as empty | TC-012 — `registry › "missing and malformed files read as empty"` | ✅ Unit | -| FR-005 | AC-2: atomic write + nested-dir creation round-trips | TC-013 — `registry › "write is atomic and round-trips; upsert replaces by name"` | ✅ Unit | -| FR-005 | AC-3: upsert replaces by name (count stays 1) | TC-013 — `registry › "write is atomic and round-trips; upsert replaces by name"` | ✅ Unit | -| FR-006 | AC-1: named git-subdir entry materializes + records | TC-014 — `installEntry › "materializes a named git-subdir entry and records it"` | ✅ Unit (git) | -| FR-006 | AC-2: name derived via readName when absent | TC-015 — `installEntry › "derives the name via readName when the entry has none"` | ✅ Unit (git) | -| FR-006 | AC-3: entry.path against a whole-repo source | TC-016 — `installEntry › "honors entry.path against a whole-repo source"` | ✅ Unit (git) | -| FR-006 | AC-4: symlink mode; re-install replaces | TC-017 — `installEntry › "symlink mode links instead of copying, and re-install replaces"` | ✅ Unit (git) | -| FR-007 | AC-1: lazy installs enabled, skips disabled | TC-018 — `reconcile › "lazy installs the enabled set, skips disabled, and is idempotent with zero git on the 2nd run"` | ✅ Unit (git) | -| FR-007 | AC-2: 2nd lazy reconcile → unchanged, zero git | TC-018 — `reconcile › "lazy installs the enabled set, skips disabled, and is idempotent with zero git on the 2nd run"` | ✅ Unit (git) | -| FR-007 | AC-3: sync unchanged on stable ref; updated on moved pin | TC-019 — `reconcile › "sync re-resolves: unchanged on a stable ref, updated on a moved pin"` | ✅ Unit (git) | -| FR-007 | AC-4: lazy re-materializes when target dir is gone | TC-020 — `reconcile › "lazy re-materializes when the target dir is gone"` | ✅ Unit (git) | -| FR-007 | AC-5: lazy sha pin → unchanged when matches, updated when differs | TC-021 — `reconcile › "lazy honors a sha pin: unchanged when it matches, updated when it differs"` | ✅ Unit (git) | +| Functional Req | Acceptance Criteria | Test Case · Case String | Coverage Status | +| -------------- | ----------------------------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------- | ------------------ | +| FR-001 | AC-1: all six valid source shapes accepted | TC-001 — `normalizeSource › "accepts every valid shape"` | ✅ Unit | +| FR-001 | AC-2: null / no-`type` → SourceError | TC-002 — `normalizeSource › "rejects malformed input"` | ✅ Unit | +| FR-001 | AC-3: missing required field → SourceError naming the field | TC-002 — `normalizeSource › "rejects malformed input"` | ✅ Unit | +| FR-001 | AC-4: unknown type → SourceError "unknown source type" | TC-002 — `normalizeSource › "rejects malformed input"` | ✅ Unit | +| FR-002 | AC-1: `owner/repo` → GitHub https URL | TC-003 — `"toGitUrl expands shorthand and passes through URLs"` | ✅ Unit | +| FR-002 | AC-2: `owner/repo.git` strips trailing `.git` | TC-003 — `"toGitUrl expands shorthand and passes through URLs"` | ✅ Unit | +| FR-002 | AC-3: full `https://` URL passes through | TC-003 — `"toGitUrl expands shorthand and passes through URLs"` | ✅ Unit | +| FR-002 | AC-4: `git@…` scp-style URL passes through | TC-003 — `"toGitUrl expands shorthand and passes through URLs"` | ✅ Unit | +| FR-002 | AC-5: surrounding whitespace is trimmed | TC-003 — `"toGitUrl expands shorthand and passes through URLs"` (padded-input assertion) | ✅ Unit | +| FR-003 | AC-1: valid manifest with name round-trips | TC-004 — `validateMarketplaceManifest › "accepts a valid manifest (with and without a name)"` | ✅ Unit | +| FR-003 | AC-2: missing name → `name === undefined` | TC-004 — `validateMarketplaceManifest › "accepts a valid manifest (with and without a name)"` | ✅ Unit | +| FR-003 | AC-3: null / non-object manifest → ManifestError | TC-005 — `validateMarketplaceManifest › "rejects malformed manifests and entries"` | ✅ Unit | +| FR-003 | AC-4: bad schemaVersion / non-array entries → ManifestError | TC-005 — `validateMarketplaceManifest › "rejects malformed manifests and entries"` | ✅ Unit | +| FR-003 | AC-5: null entry / missing entry name → ManifestError | TC-005 — `validateMarketplaceManifest › "rejects malformed manifests and entries"` | ✅ Unit | +| FR-003 | AC-6: invalid entry source → SourceError | TC-005 — `validateMarketplaceManifest › "rejects malformed manifests and entries"` | ✅ Unit | +| FR-004 | AC-1: path source returns the dir | TC-006 — `resolveSource › "path source returns the dir; missing path throws"` | ✅ Unit | +| FR-004 | AC-2: missing path → SourceError | TC-006 — `resolveSource › "path source returns the dir; missing path throws"` | ✅ Unit | +| FR-004 | AC-3: url → UnsupportedSourceError | TC-007 — `resolveSource › "url sources are not yet supported"` | ✅ Unit | +| FR-004 | AC-4: git-subdir sparse-checkout at tag → dir/sha/ref | TC-008 — `resolveSource › "git-subdir sparse-checks out only the subdir at a tag"` | ✅ Unit (git) | +| FR-004 | AC-5: whole-repo HEAD when unpinned; re-fetch existing cache | TC-009 — `resolveSource › "whole-repo git resolves to HEAD when unpinned, and re-fetches an existing cache"` | ✅ Unit (git) | +| FR-004 | AC-6: sha pin checks out the exact commit | TC-010 — `resolveSource › "sha pin checks out the exact commit"` | ✅ Unit (git) | +| FR-004 | AC-7: github + injected runner needs no real git | TC-011 — `resolveSource › "github source + injected runner needs no real git"` | ✅ Unit (fake) | +| FR-004 | CON-1: git is the sole side effect | TC-011 — `resolveSource › "github source + injected runner needs no real git"` | ✅ Unit (fake) | +| FR-004 | CON-2: blobless + sparse (subdir only) | TC-008 — `resolveSource › "git-subdir sparse-checks out only the subdir at a tag"` | ✅ Unit (git) | +| FR-004 | AC-8: npm resolves+extracts+pins resolved version (fake fetcher) | TC-022 — `resolveSource › "npm source downloads, extracts, and pins the resolved version"` + `"… resolves via the default fetcher when none is injected"` | ✅ Unit (fake/npm) | +| FR-004 | AC-9: exact-version pin cached; unpinned re-fetches | TC-023 — `resolveSource › "exact-version npm pins are cached; unpinned specs re-fetch"` | ✅ Unit (fake) | +| FR-004 | AC-10: defaultNpmFetcher local-pack offline (npm pack + tar) | TC-024 — `resolveSource › "defaultNpmFetcher packs and extracts a local package offline"` | ✅ Unit (npm) | +| FR-004 | AC-11: npmPackArgs builds pinned/unpinned + registry argv | TC-025 — `resolveSource › "npmPackArgs builds pinned, unpinned, and registry argv"` | ✅ Unit | +| FR-005 | AC-1: missing / shape-invalid (`{}`) registry read as empty | TC-012 — `registry › "missing and malformed files read as empty"` | ✅ Unit | +| FR-005 | AC-2: atomic write + nested-dir creation round-trips | TC-013 — `registry › "write is atomic and round-trips; upsert replaces by name"` | ✅ Unit | +| FR-005 | AC-3: upsert replaces by name (count stays 1) | TC-013 — `registry › "write is atomic and round-trips; upsert replaces by name"` | ✅ Unit | +| FR-006 | AC-1: named git-subdir entry materializes + records | TC-014 — `installEntry › "materializes a named git-subdir entry and records it"` | ✅ Unit (git) | +| FR-006 | AC-2: name derived via readName when absent | TC-015 — `installEntry › "derives the name via readName when the entry has none"` | ✅ Unit (git) | +| FR-006 | AC-3: entry.path against a whole-repo source | TC-016 — `installEntry › "honors entry.path against a whole-repo source"` | ✅ Unit (git) | +| FR-006 | AC-4: symlink mode; re-install replaces | TC-017 — `installEntry › "symlink mode links instead of copying, and re-install replaces"` | ✅ Unit (git) | +| FR-007 | AC-1: lazy installs enabled, skips disabled | TC-018 — `reconcile › "lazy installs the enabled set, skips disabled, and is idempotent with zero git on the 2nd run"` | ✅ Unit (git) | +| FR-007 | AC-2: 2nd lazy reconcile → unchanged, zero git | TC-018 — `reconcile › "lazy installs the enabled set, skips disabled, and is idempotent with zero git on the 2nd run"` | ✅ Unit (git) | +| FR-007 | AC-3: sync unchanged on stable ref; updated on moved pin | TC-019 — `reconcile › "sync re-resolves: unchanged on a stable ref, updated on a moved pin"` | ✅ Unit (git) | +| FR-007 | AC-4: lazy re-materializes when target dir is gone | TC-020 — `reconcile › "lazy re-materializes when the target dir is gone"` | ✅ Unit (git) | +| FR-007 | AC-5: lazy sha pin → unchanged when matches, updated when differs | TC-021 — `reconcile › "lazy honors a sha pin: unchanged when it matches, updated when it differs"` | ✅ Unit (git) | --- @@ -127,38 +137,46 @@ partial clones work offline. ## Test Case Summary -| Test ID | Title | Type | Priority | Traces To | Status | -| ------- | --------------------------------------------------------------- | ----------- | -------- | ----------------------------------------------- | ------ | -| TC-001 | normalizeSource accepts every valid shape | Unit | P0 | FR-001-AC-1 | ✅ | -| TC-002 | normalizeSource rejects malformed input | Unit | P0 | FR-001-AC-2, -AC-3, -AC-4 | ✅ | -| TC-003 | toGitUrl expands shorthand / passes through URLs | Unit | P1 | FR-002-AC-1, -AC-2, -AC-3, -AC-4 | ✅ | -| TC-004 | validateMarketplaceManifest accepts valid (with/without name) | Unit | P0 | FR-003-AC-1, -AC-2 | ✅ | -| TC-005 | validateMarketplaceManifest rejects malformed manifests/entries | Unit | P0 | FR-003-AC-3, -AC-4, -AC-5, -AC-6 | ✅ | -| TC-006 | resolveSource path source: returns dir / missing throws | Unit | P0 | FR-004-AC-1, -AC-2 | ✅ | -| TC-007 | resolveSource url & npm unsupported | Unit | P1 | FR-004-AC-3 | ✅ | -| TC-008 | resolveSource git-subdir sparse-checkout at a tag | Unit (git) | P0 | FR-004-AC-4, -CON-2 | ✅ | -| TC-009 | resolveSource whole-repo HEAD + re-fetch existing cache | Unit (git) | P0 | FR-004-AC-5 | ✅ | -| TC-010 | resolveSource sha pin checks out exact commit | Unit (git) | P0 | FR-004-AC-6 | ✅ | -| TC-011 | resolveSource github + injected runner (no real git) | Unit (fake) | P0 | FR-004-AC-7, -CON-1, NFR-003 | ✅ | -| TC-012 | registry missing / malformed reads empty | Unit | P0 | FR-005-AC-1 | ✅ | -| TC-013 | registry atomic write round-trip + upsert by name | Unit | P0 | FR-005-AC-2, -AC-3 | ✅ | -| TC-014 | installEntry named git-subdir materializes + records | Unit (git) | P0 | FR-006-AC-1, US-002-AC-2 | ✅ | -| TC-015 | installEntry derives name via readName | Unit (git) | P0 | FR-006-AC-2, US-002-AC-1 | ✅ | -| TC-016 | installEntry honors entry.path on whole-repo source | Unit (git) | P1 | FR-006-AC-3 | ✅ | -| TC-017 | installEntry symlink mode + re-install replaces | Unit (git) | P1 | FR-006-AC-4, US-002-AC-3 | ✅ | -| TC-018 | reconcile lazy install/skip + zero-git 2nd run | Unit (git) | P0 | FR-007-AC-1, -AC-2, US-001-AC-1, -AC-2, NFR-003 | ✅ | -| TC-019 | reconcile sync unchanged-stable / updated-moved | Unit (git) | P0 | FR-007-AC-3, US-001-AC-3 | ✅ | -| TC-020 | reconcile lazy re-materializes vanished target | Unit (git) | P1 | FR-007-AC-4 | ✅ | -| TC-021 | reconcile lazy sha pin unchanged/updated | Unit (git) | P0 | FR-007-AC-5 | ✅ | +| Test ID | Title | Type | Priority | Traces To | Status | +| ------- | --------------------------------------------------------------- | --------------- | -------- | ----------------------------------------------- | ------ | +| TC-001 | normalizeSource accepts every valid shape | Unit | P0 | FR-001-AC-1 | ✅ | +| TC-002 | normalizeSource rejects malformed input | Unit | P0 | FR-001-AC-2, -AC-3, -AC-4 | ✅ | +| TC-003 | toGitUrl expands shorthand / passes through URLs | Unit | P1 | FR-002-AC-1, -AC-2, -AC-3, -AC-4 | ✅ | +| TC-004 | validateMarketplaceManifest accepts valid (with/without name) | Unit | P0 | FR-003-AC-1, -AC-2 | ✅ | +| TC-005 | validateMarketplaceManifest rejects malformed manifests/entries | Unit | P0 | FR-003-AC-3, -AC-4, -AC-5, -AC-6 | ✅ | +| TC-006 | resolveSource path source: returns dir / missing throws | Unit | P0 | FR-004-AC-1, -AC-2 | ✅ | +| TC-007 | resolveSource url sources unsupported | Unit | P1 | FR-004-AC-3 | ✅ | +| TC-008 | resolveSource git-subdir sparse-checkout at a tag | Unit (git) | P0 | FR-004-AC-4, -CON-2 | ✅ | +| TC-009 | resolveSource whole-repo HEAD + re-fetch existing cache | Unit (git) | P0 | FR-004-AC-5 | ✅ | +| TC-010 | resolveSource sha pin checks out exact commit | Unit (git) | P0 | FR-004-AC-6 | ✅ | +| TC-011 | resolveSource github + injected runner (no real git) | Unit (fake) | P0 | FR-004-AC-7, -CON-1, NFR-003 | ✅ | +| TC-012 | registry missing / malformed reads empty | Unit | P0 | FR-005-AC-1 | ✅ | +| TC-013 | registry atomic write round-trip + upsert by name | Unit | P0 | FR-005-AC-2, -AC-3 | ✅ | +| TC-014 | installEntry named git-subdir materializes + records | Unit (git) | P0 | FR-006-AC-1, US-002-AC-2 | ✅ | +| TC-015 | installEntry derives name via readName | Unit (git) | P0 | FR-006-AC-2, US-002-AC-1 | ✅ | +| TC-016 | installEntry honors entry.path on whole-repo source | Unit (git) | P1 | FR-006-AC-3 | ✅ | +| TC-017 | installEntry symlink mode + re-install replaces | Unit (git) | P1 | FR-006-AC-4, US-002-AC-3 | ✅ | +| TC-018 | reconcile lazy install/skip + zero-git 2nd run | Unit (git) | P0 | FR-007-AC-1, -AC-2, US-001-AC-1, -AC-2, NFR-003 | ✅ | +| TC-019 | reconcile sync unchanged-stable / updated-moved | Unit (git) | P0 | FR-007-AC-3, US-001-AC-3 | ✅ | +| TC-020 | reconcile lazy re-materializes vanished target | Unit (git) | P1 | FR-007-AC-4 | ✅ | +| TC-021 | reconcile lazy sha pin unchanged/updated | Unit (git) | P0 | FR-007-AC-5 | ✅ | +| TC-022 | resolveSource npm resolve+extract+pin (fake & default fetcher) | Unit (fake/npm) | P0 | FR-004-AC-8 | ✅ | +| TC-023 | resolveSource exact-cache vs unpinned-refetch (fake fetcher) | Unit (fake) | P0 | FR-004-AC-9 | ✅ | +| TC-024 | defaultNpmFetcher local-pack offline (npm pack + tar) | Unit (npm) | P1 | FR-004-AC-10 | ✅ | +| TC-025 | npmPackArgs pinned/unpinned + registry argv | Unit | P1 | FR-004-AC-11 | ✅ | + +> TC-022…TC-025 belong to the shared TC-022…TC-054 block also used by the +> concurrent `feat/plugin-discovery` PR; IDs are reconciled at the second merge +> (this PR lands first). --- ## Constraint Boundary Tests -| Constraint | Boundary / Case | Test Value | Test Case | Expected | -| ------------ | --------------------------- | ------------------------- | --------- | ------------------------------------------ | -| FR-004-CON-1 | git is the sole side effect | injected fake `GitRunner` | TC-011 | resolves with no real git; argv[0]=`clone` | -| FR-004-CON-2 | blobless + sparse | `git-subdir` at `v0.2.0` | TC-008 | only the subdir present; tag sha resolved | +| Constraint | Boundary / Case | Test Value | Test Case | Expected | +| ------------ | -------------------------------------------------- | ---------------------------------------- | -------------- | ---------------------------------------------- | +| FR-004-CON-1 | package-manager subprocess is the sole side effect | injected fake `GitRunner` / `NpmFetcher` | TC-011, TC-022 | resolves with no real git/npm; argv[0]=`clone` | +| FR-004-CON-2 | blobless + sparse | `git-subdir` at `v0.2.0` | TC-008 | only the subdir present; tag sha resolved | --- @@ -168,7 +186,7 @@ partial clones work offline. | ---------------------------- | -------------------------------------------------- | --------- | ------ | | `SourceError` | null / no-`type` / missing field / unknown type | TC-002 | ✅ | | `SourceError` | `path` source dir does not exist | TC-006 | ✅ | -| `UnsupportedSourceError` | `url` / `npm` passed to `resolveSource` | TC-007 | ✅ | +| `UnsupportedSourceError` | `url` passed to `resolveSource` (npm now resolves) | TC-007 | ✅ | | `ManifestError` | non-object / bad schemaVersion / non-array entries | TC-005 | ✅ | | `ManifestError` | null entry / entry missing a non-empty `name` | TC-005 | ✅ | | `SourceError` (via manifest) | entry with an invalid `source.type` | TC-005 | ✅ | @@ -189,13 +207,15 @@ partial clones work offline. ## Coverage Summary -- **Acceptance Criteria → Test Case coverage: 36 of 36 functional ACs (100%) map to +- **Acceptance Criteria → Test Case coverage: 40 of 40 functional ACs (100%) map to an executed Test Case.** All ACs of FR-001…FR-007 (incl. FR-002-AC-5 whitespace - trimming via the padded-input assertion in TC-003), both FR-004 constraints, and - all 10 user-story ACs map to a real test in `tests/index.test.ts`. NFR-001…NFR-004 - are covered by the coverage gate, the zero-git assertion, inspection, and analysis. -- The 21 TCs are **1:1** with the tests in `tests/index.test.ts`. All pass under - `make test` at the 100% coverage gate. + trimming via the padded-input assertion in TC-003, and the four new FR-004 npm ACs + AC-8…AC-11), both FR-004 constraints, and all 10 user-story ACs map to a real test + in `tests/index.test.ts`. NFR-001…NFR-004 are covered by the coverage gate, the + zero-git assertion, inspection, and analysis. +- The 25 TCs map to the tests in `tests/index.test.ts` (TC-022 spans both the + fake-fetcher and default-fetcher npm tests). All pass under `make test` at the + 100% coverage gate. - All six test-matrix rules are satisfied: every AC has a TC (Rule 1); the copy/symlink materialize options are both exercised (Rule 2, TC-014/TC-017); the two FR-004 constraints are boundary-tested (Rule 3); every documented error is @@ -230,6 +250,7 @@ Recorded during /spec-review. None block the spec; each is a candidate test/hard on `tests/index.test.ts` (the test strings do not yet embed `TC-XXX` labels). The `Test Case · Case String` column is the canonical pointer from each AC to the exact `describe › test` it is verified by. -- The `url` and `npm` source variants are deliberately unimplemented (FR-004-AC-3); - there is therefore no happy-path TC for them, only the `UnsupportedSourceError` - assertion (TC-007). This is correct, not a coverage gap. +- The `url` source variant is deliberately unimplemented (FR-004-AC-3); there is + therefore no happy-path TC for it, only the `UnsupportedSourceError` assertion + (TC-007). This is correct, not a coverage gap. The `npm` variant **is** now + resolved (TC-022…TC-025). diff --git a/src/index.ts b/src/index.ts index 685ac2c..46ae2af 100644 --- a/src/index.ts +++ b/src/index.ts @@ -21,9 +21,11 @@ export { export { type GitRunner, + type NpmFetcher, type ResolveOptions, type ResolvedSource, defaultGitRunner, + defaultNpmFetcher, resolveSource, } from "./resolve.js"; diff --git a/src/resolve.ts b/src/resolve.ts index 09d9ae1..2ea1396 100644 --- a/src/resolve.ts +++ b/src/resolve.ts @@ -1,5 +1,5 @@ import { execFileSync } from "node:child_process"; -import { existsSync, mkdirSync } from "node:fs"; +import { existsSync, mkdirSync, readFileSync, writeFileSync } from "node:fs"; import { dirname, join, resolve as resolvePath } from "node:path"; import { @@ -25,10 +25,57 @@ export const defaultGitRunner: GitRunner = (args, opts) => { return { stdout }; }; +/** + * Downloads and extracts an npm package tarball into `destDir`, returning the + * resolved content root and exact published version. Injectable so tests fake + * it without a real `npm`/network. The default uses `npm pack` + `tar`. + */ +export interface NpmFetcher { + ( + spec: { package: string; version?: string; registry?: string }, + destDir: string, + ): { dir: string; version: string }; +} + +/** + * Build the `npm pack` argv for a spec. Pure + exported so the version (`pkg` + * vs `pkg@version`) and `--registry` branches stay unit-testable offline. + */ +export function npmPackArgs( + spec: { package: string; version?: string; registry?: string }, + destDir: string, +): string[] { + const ref = spec.version ? `${spec.package}@${spec.version}` : spec.package; + const args = ["pack", ref, "--pack-destination", destDir, "--json"]; + if (spec.registry) args.push("--registry", spec.registry); + return args; +} + +/** Default fetcher: `npm pack ` then extract the tarball with `tar`. */ +export const defaultNpmFetcher: NpmFetcher = (spec, destDir) => { + mkdirSync(destDir, { recursive: true }); + const out = execFileSync("npm", npmPackArgs(spec, destDir), { + encoding: "utf8", + stdio: ["ignore", "pipe", "pipe"], + }); + // Packing a local dir runs its `prepack`, whose stdout precedes the JSON; + // slice from the array start so lifecycle noise doesn't break the parse. + const meta = JSON.parse(out.slice(out.indexOf("[")))[0] as { + filename: string; + version: string; + }; + execFileSync("tar", ["-xzf", join(destDir, meta.filename), "-C", destDir], { + stdio: ["ignore", "pipe", "pipe"], + }); + // npm tarballs always extract under a top-level `package/` directory. + return { dir: join(destDir, "package"), version: meta.version }; +}; + export interface ResolveOptions { /** Root under which sources are cached (`/git/`). */ cacheRoot: string; git?: GitRunner; + npm?: NpmFetcher; } export interface ResolvedSource { @@ -61,7 +108,11 @@ export function resolveSource( return { dir }; } - if (source.type === "url" || source.type === "npm") { + if (source.type === "npm") { + return resolveNpm(source, opts); + } + + if (source.type === "url") { throw new UnsupportedSourceError( `source type "${source.type}" is not yet supported`, ); @@ -89,3 +140,51 @@ export function resolveSource( const dir = subdir ? join(cacheDir, subdir) : cacheDir; return { dir, sha, ref: source.ref }; } + +/** True for a concrete, immutable version pin (e.g. `1.2.3`, `1.2.3-rc.1`). */ +function isExactVersion(version: string | undefined): version is string { + return !!version && /^\d+\.\d+\.\d+([-+].*)?$/.test(version); +} + +/** + * Resolve an npm source by downloading+extracting its tarball into the cache. + * The exact published version acts as the durable `sha` pin (mirroring git). + * Exact-version pins are cached; unpinned/range specs re-fetch to honor "latest". + */ +function resolveNpm( + source: Extract, + opts: ResolveOptions, +): ResolvedSource { + const fetch = opts.npm ?? defaultNpmFetcher; + const cacheDir = join( + opts.cacheRoot, + "npm", + cacheKey(`${source.package}@${source.version ?? "latest"}`), + ); + const verFile = join(cacheDir, ".resolved-version"); + const contentDir = join(cacheDir, "package"); + + if ( + isExactVersion(source.version) && + existsSync(join(contentDir, "package.json")) && + existsSync(verFile) + ) { + return { + dir: contentDir, + sha: readFileSync(verFile, "utf8").trim(), + ref: source.version, + }; + } + + mkdirSync(cacheDir, { recursive: true }); + const { dir, version } = fetch( + { + package: source.package, + version: source.version, + registry: source.registry, + }, + cacheDir, + ); + writeFileSync(verFile, version + "\n"); + return { dir, sha: version, ref: source.version }; +} diff --git a/tests/index.test.ts b/tests/index.test.ts index fbd11a7..8429e7a 100644 --- a/tests/index.test.ts +++ b/tests/index.test.ts @@ -15,6 +15,7 @@ import { ManifestError, SourceError, UnsupportedSourceError, + defaultNpmFetcher, installEntry, normalizeSource, readRegistry, @@ -27,8 +28,10 @@ import { type GitRunner, type InstallOptions, type InstalledPlugin, + type NpmFetcher, type Source, } from "../src"; +import { npmPackArgs } from "../src/resolve.js"; // ── Fixture: a local bare git repo with two tagged versions, no network ────── @@ -253,13 +256,10 @@ describe("resolveSource", () => { ).toThrow(SourceError); }); - test("url and npm sources are not yet supported", () => { + test("url sources are not yet supported", () => { expect(() => resolveSource({ type: "url", url: "u" }, opts())).toThrow( UnsupportedSourceError, ); - expect(() => resolveSource({ type: "npm", package: "p" }, opts())).toThrow( - UnsupportedSourceError, - ); }); test("git-subdir sparse-checks out only the subdir at a tag", () => { @@ -298,6 +298,143 @@ describe("resolveSource", () => { expect(r.sha).toBe("fakesha"); expect(calls[0][0]).toBe("clone"); }); + + // A fake fetcher that materializes a flattened module tarball (manifest at the + // content root) without a real npm/network, and counts how often it runs. + function fakeNpm(version: string) { + const state = { calls: 0 }; + const fetcher: NpmFetcher = (spec, destDir) => { + state.calls++; + const pkgDir = join(destDir, "package"); + mkdirSync(pkgDir, { recursive: true }); + const name = spec.package.replace(/^@[^/]+\//, ""); + writeFileSync( + join(pkgDir, "manifest.yaml"), + `name: ${name}\nversion: ${version}\n`, + ); + writeFileSync( + join(pkgDir, "package.json"), + JSON.stringify({ name: spec.package, version }), + ); + // exact pins resolve to themselves; ranges/latest resolve to `version`. + const resolved = /^\d+\.\d+\.\d+$/.test(spec.version ?? "") + ? (spec.version as string) + : version; + return { dir: pkgDir, version: resolved }; + }; + return { fetcher, state }; + } + + test("npm source downloads, extracts, and pins the resolved version", () => { + const { fetcher, state } = fakeNpm("0.4.0"); + const r = resolveSource( + { + type: "npm", + package: "@agent-ix/spec-artifacts-app", + version: "0.4.0", + }, + opts({ npm: fetcher }), + ); + expect(existsSync(join(r.dir, "manifest.yaml"))).toBe(true); + expect(r.sha).toBe("0.4.0"); + expect(r.ref).toBe("0.4.0"); + expect(state.calls).toBe(1); + }); + + test("exact-version npm pins are cached; unpinned specs re-fetch", () => { + const base = opts(); + + const pinned = fakeNpm("0.4.0"); + const oPinned = { ...base, npm: pinned.fetcher }; + const src = { + type: "npm" as const, + package: "@agent-ix/x", + version: "0.4.0", + }; + resolveSource(src, oPinned); + resolveSource(src, oPinned); + expect(pinned.state.calls).toBe(1); // second resolve hits the cache + + const unpinned = fakeNpm("9.9.9"); + const oUnpinned = { ...base, npm: unpinned.fetcher }; + const latest = { type: "npm" as const, package: "@agent-ix/y" }; + const a = resolveSource(latest, oUnpinned); + resolveSource(latest, oUnpinned); + expect(unpinned.state.calls).toBe(2); // unpinned always re-fetches + expect(a.sha).toBe("9.9.9"); // resolved version is the durable pin + }); + + // TC-025 / FR-004-AC-11: argv builder covers the version + registry branches + // purely (offline), so the network-only ref/registry paths stay tested. + test("npmPackArgs builds pinned, unpinned, and registry argv", () => { + expect(npmPackArgs({ package: "p", version: "1.2.3" }, "/d")).toEqual([ + "pack", + "p@1.2.3", + "--pack-destination", + "/d", + "--json", + ]); + expect(npmPackArgs({ package: "p" }, "/d")).toEqual([ + "pack", + "p", + "--pack-destination", + "/d", + "--json", + ]); + expect(npmPackArgs({ package: "p", registry: "https://r" }, "/d")).toEqual([ + "pack", + "p", + "--pack-destination", + "/d", + "--json", + "--registry", + "https://r", + ]); + }); + + // TC-024 / FR-004-AC-10: the real fetcher packs+extracts a LOCAL folder via + // `npm pack` + `tar`, fully offline (no registry, no network). + test("defaultNpmFetcher packs and extracts a local package offline", () => { + const fixtureDir = mkdtempSync(join(tmpdir(), "tpk-mod-")); + writeFileSync( + join(fixtureDir, "package.json"), + JSON.stringify({ name: "local-mod", version: "1.2.3" }), + ); + writeFileSync( + join(fixtureDir, "manifest.yaml"), + "name: local-mod\nversion: 1.2.3\n", + ); + const destDir = mkdtempSync(join(tmpdir(), "tpk-dest-")); + try { + const r = defaultNpmFetcher({ package: fixtureDir }, destDir); + expect(r.version).toBe("1.2.3"); + expect(existsSync(join(r.dir, "package.json"))).toBe(true); + } finally { + rmSync(fixtureDir, { recursive: true, force: true }); + rmSync(destDir, { recursive: true, force: true }); + } + }); + + // FR-004-AC-8 via the DEFAULT fetcher: resolveSource with no injected `npm` + // falls back to defaultNpmFetcher, packing a local folder offline. + test("npm source resolves via the default fetcher when none is injected", () => { + const fixtureDir = mkdtempSync(join(tmpdir(), "tpk-srcmod-")); + writeFileSync( + join(fixtureDir, "package.json"), + JSON.stringify({ name: "local-mod", version: "1.2.3" }), + ); + writeFileSync( + join(fixtureDir, "manifest.yaml"), + "name: local-mod\nversion: 1.2.3\n", + ); + try { + const r = resolveSource({ type: "npm", package: fixtureDir }, opts()); + expect(existsSync(join(r.dir, "manifest.yaml"))).toBe(true); + expect(r.sha).toBe("1.2.3"); + } finally { + rmSync(fixtureDir, { recursive: true, force: true }); + } + }); }); // ── install ────────────────────────────────────────────────────────────────── From 3984df674f141cd31331f07aa300f024da0dcc56 Mon Sep 17 00:00:00 2001 From: Peter Krenesky Date: Sat, 27 Jun 2026 16:15:02 -0700 Subject: [PATCH 2/4] harden: reject option-like npm package to prevent npm pack flag injection `resolveNpm` passes the npm `package` into `npm pack ` via `execFileSync`. An untrusted package beginning with `-` would be read by npm as a CLI flag (second-order command-line injection). `normalizeSource` now rejects an npm `package` that begins with `-` (new `reqArg` guard), so it can never reach the `npm pack` argv as an option. Adds FR-004-CON-3 + TC-026. Co-Authored-By: Claude Opus 4.8 (1M context) --- spec/functional/FR-004-source-resolution.md | 1 + spec/tests.md | 17 ++++++++++------- src/sources.ts | 16 +++++++++++++++- tests/index.test.ts | 4 ++++ 4 files changed, 30 insertions(+), 8 deletions(-) diff --git a/spec/functional/FR-004-source-resolution.md b/spec/functional/FR-004-source-resolution.md index d227b50..9324390 100644 --- a/spec/functional/FR-004-source-resolution.md +++ b/spec/functional/FR-004-source-resolution.md @@ -97,6 +97,7 @@ real `npm`/network (FR-004-AC-8, -AC-9). | ------------ | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------------- | ------------- | | FR-004-CON-1 | The only side effect is the per-source package-manager subprocess (`git` for git sources; `npm pack` + `tar` for npm sources); resolution performs no other network or I/O beyond filesystem reads/dir creation. | architectural | Test (TC-011) | | FR-004-CON-2 | The clone is blobless and no-checkout (`--filter=blob:none --no-checkout`); subdir sources sparse-checkout only the requested path. | performance | Test (TC-008) | +| FR-004-CON-3 | `normalizeSource` rejects an `npm` `package` beginning with `-`, so it cannot reach `npm pack` as a CLI flag (second-order command-line-injection guard). | security | Test (TC-026) | ## Acceptance Criteria diff --git a/spec/tests.md b/spec/tests.md index 1ac2e53..1b7c89c 100644 --- a/spec/tests.md +++ b/spec/tests.md @@ -21,7 +21,7 @@ today under `make test` (vitest) at the 100% coverage gate (NFR-002). > **Concurrent-PR note.** TC-022…TC-054 are a shared TC-ID block also claimed by > the concurrent `feat/plugin-discovery` PR. This (`feat/npm-source-resolution`) -> PR lands first and uses TC-022…TC-025 for npm source resolution; the two +> PR lands first and uses TC-022…TC-026 for npm source resolution; the two > matrices will be **reconciled at the second merge** so the IDs do not collide. Tests fall into the following types: @@ -109,6 +109,7 @@ partial clones work offline. | FR-004 | AC-9: exact-version pin cached; unpinned re-fetches | TC-023 — `resolveSource › "exact-version npm pins are cached; unpinned specs re-fetch"` | ✅ Unit (fake) | | FR-004 | AC-10: defaultNpmFetcher local-pack offline (npm pack + tar) | TC-024 — `resolveSource › "defaultNpmFetcher packs and extracts a local package offline"` | ✅ Unit (npm) | | FR-004 | AC-11: npmPackArgs builds pinned/unpinned + registry argv | TC-025 — `resolveSource › "npmPackArgs builds pinned, unpinned, and registry argv"` | ✅ Unit | +| FR-004 | CON-3: rejects option-like npm package (injection guard) | TC-026 — `normalizeSource › "rejects malformed input"` (option-like `-x` package assertion) | ✅ Unit | | FR-005 | AC-1: missing / shape-invalid (`{}`) registry read as empty | TC-012 — `registry › "missing and malformed files read as empty"` | ✅ Unit | | FR-005 | AC-2: atomic write + nested-dir creation round-trips | TC-013 — `registry › "write is atomic and round-trips; upsert replaces by name"` | ✅ Unit | | FR-005 | AC-3: upsert replaces by name (count stays 1) | TC-013 — `registry › "write is atomic and round-trips; upsert replaces by name"` | ✅ Unit | @@ -164,8 +165,9 @@ partial clones work offline. | TC-023 | resolveSource exact-cache vs unpinned-refetch (fake fetcher) | Unit (fake) | P0 | FR-004-AC-9 | ✅ | | TC-024 | defaultNpmFetcher local-pack offline (npm pack + tar) | Unit (npm) | P1 | FR-004-AC-10 | ✅ | | TC-025 | npmPackArgs pinned/unpinned + registry argv | Unit | P1 | FR-004-AC-11 | ✅ | +| TC-026 | normalizeSource rejects option-like npm package (`-x`) | Unit | P0 | FR-004-CON-3 | ✅ | -> TC-022…TC-025 belong to the shared TC-022…TC-054 block also used by the +> TC-022…TC-026 belong to the shared TC-022…TC-054 block also used by the > concurrent `feat/plugin-discovery` PR; IDs are reconciled at the second merge > (this PR lands first). @@ -173,10 +175,11 @@ partial clones work offline. ## Constraint Boundary Tests -| Constraint | Boundary / Case | Test Value | Test Case | Expected | -| ------------ | -------------------------------------------------- | ---------------------------------------- | -------------- | ---------------------------------------------- | -| FR-004-CON-1 | package-manager subprocess is the sole side effect | injected fake `GitRunner` / `NpmFetcher` | TC-011, TC-022 | resolves with no real git/npm; argv[0]=`clone` | -| FR-004-CON-2 | blobless + sparse | `git-subdir` at `v0.2.0` | TC-008 | only the subdir present; tag sha resolved | +| Constraint | Boundary / Case | Test Value | Test Case | Expected | +| ------------ | -------------------------------------------------- | ---------------------------------------- | -------------- | ---------------------------------------------------- | +| FR-004-CON-1 | package-manager subprocess is the sole side effect | injected fake `GitRunner` / `NpmFetcher` | TC-011, TC-022 | resolves with no real git/npm; argv[0]=`clone` | +| FR-004-CON-2 | blobless + sparse | `git-subdir` at `v0.2.0` | TC-008 | only the subdir present; tag sha resolved | +| FR-004-CON-3 | option-like npm package rejected | `{type:"npm", package:"-x"}` | TC-026 | `SourceError` "must not begin with -"; no `npm pack` | --- @@ -253,4 +256,4 @@ Recorded during /spec-review. None block the spec; each is a candidate test/hard - The `url` source variant is deliberately unimplemented (FR-004-AC-3); there is therefore no happy-path TC for it, only the `UnsupportedSourceError` assertion (TC-007). This is correct, not a coverage gap. The `npm` variant **is** now - resolved (TC-022…TC-025). + resolved (TC-022…TC-026). diff --git a/src/sources.ts b/src/sources.ts index e9b66b6..5ed3cc2 100644 --- a/src/sources.ts +++ b/src/sources.ts @@ -47,6 +47,20 @@ function req(value: unknown, field: string): string { return value; } +/** + * Like {@link req}, but also rejects an option-like value (leading `-`). Source + * fields that flow into a `git`/`npm` argv must not be interpretable as a CLI + * flag — e.g. a package named `-x` would become an `npm pack` option (a + * second-order command-line injection), so it is rejected up front. + */ +function reqArg(value: unknown, field: string): string { + const v = req(value, field); + if (v.startsWith("-")) { + throw new SourceError(`source field "${field}" must not begin with "-"`); + } + return v; +} + /** Validate a source descriptor, throwing {@link SourceError} on malformed input. */ export function normalizeSource(source: Source): Source { if (!source || typeof (source as { type?: unknown }).type !== "string") { @@ -70,7 +84,7 @@ export function normalizeSource(source: Source): Source { req(source.path, "path"); return source; case "npm": - req(source.package, "package"); + reqArg(source.package, "package"); return source; default: throw new SourceError( diff --git a/tests/index.test.ts b/tests/index.test.ts index 8429e7a..ea04589 100644 --- a/tests/index.test.ts +++ b/tests/index.test.ts @@ -151,6 +151,10 @@ describe("normalizeSource", () => { expect(() => normalizeSource({ type: "url" } as Source)).toThrow(/url/); expect(() => normalizeSource({ type: "path" } as Source)).toThrow(/path/); expect(() => normalizeSource({ type: "npm" } as Source)).toThrow(/package/); + // An option-like package would inject a flag into `npm pack` (TC-026). + expect(() => normalizeSource({ type: "npm", package: "-x" })).toThrow( + /must not begin with/, + ); expect(() => normalizeSource({ type: "bogus" } as unknown as Source), ).toThrow(/unknown source type/); From dfb3cd6d6cf9c8b74bc3224b4a9e107bee4bcf7c Mon Sep 17 00:00:00 2001 From: Peter Krenesky Date: Sat, 27 Jun 2026 19:13:03 -0700 Subject: [PATCH 3/4] harden: robust npm pack parse, fetcher-dir contract, cache hygiene Address the non-blocking /code-review follow-ups on npm source resolution (PR #3, issue #5): - Robust npm pack --json parse: extract the new pure, exported `parseNpmPackJson` that scans candidate `[` positions and takes the first parseable non-empty array, so prepack/prepare lifecycle noise (including stray `[` brackets) no longer mis-parses. No-bracket or empty-array output now throws a descriptive SourceError instead of a cryptic one. defaultNpmFetcher delegates to it. - Document the NpmFetcher contract: a fetcher MUST extract to `/package` because a later exact-version cache hit serves `/package` directly and ignores a custom fetcher's returned dir. - Refresh stale docstrings now that npm is also a source: resolveSource side-effect note, ResolveOptions.cacheRoot, and ResolvedSource.sha. - Disk hygiene: clear the npm cache dir on the miss/re-fetch path so unpinned "latest" re-fetches don't accumulate stale tarballs. Tests added for prepack noise, no-bracket and empty-array parse errors, and tarball non-accumulation. src/resolve.ts stays at 100% coverage. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/resolve.ts | 69 ++++++++++++++++++++++++++++++++++++++------- tests/index.test.ts | 51 +++++++++++++++++++++++++++++++-- 2 files changed, 108 insertions(+), 12 deletions(-) diff --git a/src/resolve.ts b/src/resolve.ts index 2ea1396..8a37286 100644 --- a/src/resolve.ts +++ b/src/resolve.ts @@ -1,5 +1,11 @@ import { execFileSync } from "node:child_process"; -import { existsSync, mkdirSync, readFileSync, writeFileSync } from "node:fs"; +import { + existsSync, + mkdirSync, + readFileSync, + rmSync, + writeFileSync, +} from "node:fs"; import { dirname, join, resolve as resolvePath } from "node:path"; import { @@ -29,6 +35,14 @@ export const defaultGitRunner: GitRunner = (args, opts) => { * Downloads and extracts an npm package tarball into `destDir`, returning the * resolved content root and exact published version. Injectable so tests fake * it without a real `npm`/network. The default uses `npm pack` + `tar`. + * + * **Contract.** A fetcher MUST extract the package so its content root lives at + * `/package` (the layout `npm pack` tarballs already use), and SHOULD + * return that path as `dir`. `resolveNpm` records only the resolved `version` + * (the durable pin) durably; on a later exact-version cache hit it serves + * `/package` directly and does **not** re-read the `dir` a custom + * fetcher returned. A fetcher that extracts elsewhere is therefore honored on + * the miss path but silently ignored on subsequent cached resolves. */ export interface NpmFetcher { ( @@ -51,6 +65,37 @@ export function npmPackArgs( return args; } +/** + * Robustly parse the metadata array `npm pack --json` prints. Packing a local + * dir runs its `prepack`/`prepare`, whose stdout precedes the JSON and may even + * contain stray `[` brackets (e.g. `[build] done`), so a naive + * `indexOf("[")` slice mis-parses. Instead, scan candidate `[` positions and + * take the first that parses to a non-empty JSON array. Throws a descriptive + * `SourceError` when no parseable array (or an empty one) is found. Exported + * pure so the noise/garbage branches stay unit-testable offline. + */ +export function parseNpmPackJson(out: string): { + filename: string; + version: string; +} { + for (let i = out.indexOf("["); i >= 0; i = out.indexOf("[", i + 1)) { + let parsed: { filename: string; version: string }[]; + try { + parsed = JSON.parse(out.slice(i)); + } catch { + continue; + } + if (parsed.length === 0) + throw new SourceError( + `npm pack returned no package metadata: ${out.trim()}`, + ); + return parsed[0]; + } + throw new SourceError( + `could not parse npm pack --json output: ${out.trim()}`, + ); +} + /** Default fetcher: `npm pack ` then extract the tarball with `tar`. */ export const defaultNpmFetcher: NpmFetcher = (spec, destDir) => { mkdirSync(destDir, { recursive: true }); @@ -58,12 +103,7 @@ export const defaultNpmFetcher: NpmFetcher = (spec, destDir) => { encoding: "utf8", stdio: ["ignore", "pipe", "pipe"], }); - // Packing a local dir runs its `prepack`, whose stdout precedes the JSON; - // slice from the array start so lifecycle noise doesn't break the parse. - const meta = JSON.parse(out.slice(out.indexOf("[")))[0] as { - filename: string; - version: string; - }; + const meta = parseNpmPackJson(out); execFileSync("tar", ["-xzf", join(destDir, meta.filename), "-C", destDir], { stdio: ["ignore", "pipe", "pipe"], }); @@ -72,7 +112,10 @@ export const defaultNpmFetcher: NpmFetcher = (spec, destDir) => { }; export interface ResolveOptions { - /** Root under which sources are cached (`/git/`). */ + /** + * Root under which sources are cached: git sources under + * `/git/`, npm sources under `/npm/`. + */ cacheRoot: string; git?: GitRunner; npm?: NpmFetcher; @@ -81,7 +124,7 @@ export interface ResolveOptions { export interface ResolvedSource { /** Absolute path to the resolved content root. */ dir: string; - /** Resolved commit sha (git sources). */ + /** Durable pin: the resolved commit sha (git sources) or published version (npm sources). */ sha?: string; /** The git ref that was requested, echoed back for the registry record. */ ref?: string; @@ -93,7 +136,9 @@ function cacheKey(url: string): string { /** * Fetch a source to a local directory and return its resolved content root + - * durable sha pin. Synchronous: git is the only side effect. + * durable pin. Synchronous: the only side effect is the per-source + * package-manager subprocess (`git` for git sources; `npm pack` + `tar` for npm + * sources) plus the filesystem reads/writes under `cacheRoot`. */ export function resolveSource( source: Source, @@ -176,6 +221,10 @@ function resolveNpm( }; } + // Cache miss (unpinned/range re-fetch, or a not-yet-cached pin): clear any + // prior tarball + extraction so the cache dir doesn't accumulate stale + // artifacts across "latest" re-fetches. + rmSync(cacheDir, { recursive: true, force: true }); mkdirSync(cacheDir, { recursive: true }); const { dir, version } = fetch( { diff --git a/tests/index.test.ts b/tests/index.test.ts index ea04589..58b7521 100644 --- a/tests/index.test.ts +++ b/tests/index.test.ts @@ -5,11 +5,12 @@ import { mkdirSync, mkdtempSync, readFileSync, + readdirSync, rmSync, writeFileSync, } from "node:fs"; import { tmpdir } from "node:os"; -import { join } from "node:path"; +import { dirname, join } from "node:path"; import { ManifestError, @@ -31,7 +32,7 @@ import { type NpmFetcher, type Source, } from "../src"; -import { npmPackArgs } from "../src/resolve.js"; +import { npmPackArgs, parseNpmPackJson } from "../src/resolve.js"; // ── Fixture: a local bare git repo with two tagged versions, no network ────── @@ -439,6 +440,52 @@ describe("resolveSource", () => { rmSync(fixtureDir, { recursive: true, force: true }); } }); + + // Robust npm pack --json parsing: a local pack runs `prepack`/`prepare` + // whose stdout precedes the JSON, so the parser must skip lifecycle noise + // (including stray `[` brackets) and fail descriptively on garbage output. + test("parseNpmPackJson skips prepack noise and parses the trailing array", () => { + const out = `[build] starting\n[build] done\n[{"filename":"p-1.2.3.tgz","version":"1.2.3"}]\n`; + expect(parseNpmPackJson(out)).toEqual({ + filename: "p-1.2.3.tgz", + version: "1.2.3", + }); + }); + + test("parseNpmPackJson throws SourceError on no-bracket output", () => { + expect(() => parseNpmPackJson("build done, no json here\n")).toThrow( + SourceError, + ); + }); + + test("parseNpmPackJson throws SourceError on an empty array", () => { + expect(() => parseNpmPackJson("[]\n")).toThrow(SourceError); + }); + + // Disk hygiene: an unpinned re-fetch must not let stale tarballs pile up in + // the npm cache dir; the prior fetch's artifacts are cleared before re-fetch. + test("unpinned re-fetch does not accumulate stale tarballs", () => { + let n = 0; + const fetcher: NpmFetcher = (_spec, destDir) => { + n++; + // mimic `npm pack` leaving a uniquely-named tarball in destDir + writeFileSync(join(destDir, `pkg-${n}.tgz`), ""); + const pkgDir = join(destDir, "package"); + mkdirSync(pkgDir, { recursive: true }); + writeFileSync( + join(pkgDir, "package.json"), + JSON.stringify({ version: "1.0.0" }), + ); + return { dir: pkgDir, version: "1.0.0" }; + }; + const o = opts({ npm: fetcher }); + const src = { type: "npm" as const, package: "@agent-ix/z" }; + resolveSource(src, o); + const r = resolveSource(src, o); + const cacheDir = dirname(r.dir); + const tarballs = readdirSync(cacheDir).filter((f) => f.endsWith(".tgz")); + expect(tarballs).toEqual(["pkg-2.tgz"]); // prior tarball cleaned up + }); }); // ── install ────────────────────────────────────────────────────────────────── From 37f28ca8ffdd72c9767179228c037fb191aab36b Mon Sep 17 00:00:00 2001 From: Peter Krenesky Date: Sat, 27 Jun 2026 19:31:35 -0700 Subject: [PATCH 4/4] fix: stricter npm-pack parse + spec/matrix backsync for npm robustness MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Close PR #3 review findings. parseNpmPackJson now scans ALL candidate `[` positions and accepts the first array whose first element is npm-pack metadata (an object with a string `filename`), skipping empty/non-matching/noise arrays and throwing a descriptive SourceError only when none match. This fixes two latent bugs: a bare `[]` could halt the scan, and a numeric noise array like `[1,2,3]` was wrongly accepted (yielding `parsed[0]=1` → confusing tar failure). Added Red-first tests for the numeric and no-string-filename rejection paths, plus scan-past-empty / scan-past-numeric guards. Backsync the spec + matrix for the previously untraced behaviors: - FR-004-AC-12 (robust npm-pack-json parse) + FR-004-CON-4 (parse-failure) - FR-004-AC-13 (unpinned re-fetch cache hygiene) - TC-027 (robust-parse / parse-failure) and TC-028 (cache-hygiene) rows - corrected Coverage Summary counts (40 ACs, 4 CONs, 6 US ACs, 28 TCs) Gates: tsc, eslint, prettier, vitest 100% coverage, quire validate — green. Co-Authored-By: Claude Opus 4.8 (1M context) --- spec/functional/FR-004-source-resolution.md | 47 ++++++----- spec/tests.md | 94 +++++++++++---------- src/resolve.ts | 28 +++--- tests/index.test.ts | 32 +++++++ 4 files changed, 128 insertions(+), 73 deletions(-) diff --git a/spec/functional/FR-004-source-resolution.md b/spec/functional/FR-004-source-resolution.md index 9324390..cd4c6ef 100644 --- a/spec/functional/FR-004-source-resolution.md +++ b/spec/functional/FR-004-source-resolution.md @@ -44,6 +44,8 @@ expand git URLs via `toGitUrl` ([FR-002](./FR-002-git-url-shorthand.md)). `source.version`. An **exact-version** pin (`X.Y.Z[-+…]`) whose extracted `package.json` and recorded version are already cached is reused without re-fetching; an unpinned or range spec re-fetches every time to honor "latest". + On a re-fetch (or any cache miss) the npm cache dir is cleared before + re-extracting so stale tarball artifacts do not accumulate (FR-004-AC-13). - **git sources** (`github`, `git`, `git-subdir`): - Compute the clone URL via `toGitUrl(source.repo | source.url)` and the cache dir `/git/` (non-`[A-Za-z0-9._-]` chars replaced @@ -86,34 +88,41 @@ metadata for the tarball `filename` and resolved `version`, then extracts the tarball with `tar -xzf -C ` and returns `{ dir: /package, version }`. The `npm pack` argv is built by the pure, exported `npmPackArgs(spec, destDir)` helper so the version and `--registry` branches are -unit-testable offline (FR-004-AC-11). `npm pack` of a **local folder** runs +unit-testable offline (FR-004-AC-11). Metadata parsing is delegated to the pure, +exported `parseNpmPackJson(out)` helper, which scans past lifecycle-script stdout +noise and returns the first npm-pack metadata object (an object with a string +`filename`), throwing a descriptive `SourceError` on no-array/empty/invalid +output (FR-004-AC-12, FR-004-CON-4). `npm pack` of a **local folder** runs offline (no registry), which is how the default fetcher is integration-tested (FR-004-AC-10). An injected `NpmFetcher` replaces the default so tests run with no real `npm`/network (FR-004-AC-8, -AC-9). ## Constraints -| ID | Constraint | Type | Validation | -| ------------ | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------------- | ------------- | -| FR-004-CON-1 | The only side effect is the per-source package-manager subprocess (`git` for git sources; `npm pack` + `tar` for npm sources); resolution performs no other network or I/O beyond filesystem reads/dir creation. | architectural | Test (TC-011) | -| FR-004-CON-2 | The clone is blobless and no-checkout (`--filter=blob:none --no-checkout`); subdir sources sparse-checkout only the requested path. | performance | Test (TC-008) | -| FR-004-CON-3 | `normalizeSource` rejects an `npm` `package` beginning with `-`, so it cannot reach `npm pack` as a CLI flag (second-order command-line-injection guard). | security | Test (TC-026) | +| ID | Constraint | Type | Validation | +| ------------ | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------------- | ------------- | +| FR-004-CON-1 | The only side effect is the per-source package-manager subprocess (`git` for git sources; `npm pack` + `tar` for npm sources); resolution performs no other network or I/O beyond filesystem reads/dir creation. | architectural | Test (TC-011) | +| FR-004-CON-2 | The clone is blobless and no-checkout (`--filter=blob:none --no-checkout`); subdir sources sparse-checkout only the requested path. | performance | Test (TC-008) | +| FR-004-CON-3 | `normalizeSource` rejects an `npm` `package` beginning with `-`, so it cannot reach `npm pack` as a CLI flag (second-order command-line-injection guard). | security | Test (TC-026) | +| FR-004-CON-4 | When `npm pack --json` output contains no metadata array (no array at all, an empty array, or an array whose first element is not an object with a string `filename`), `parseNpmPackJson` throws a descriptive `SourceError` rather than returning garbage or a confusing downstream failure. | robustness | Test (TC-027) | ## Acceptance Criteria -| ID | Criteria | Verification | -| ------------ | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------------- | -| FR-004-AC-1 | A `path` source for an existing directory returns `{ dir }` pointing at that directory (its contents are readable). | Test (TC-006) | -| FR-004-AC-2 | A `path` source for a non-existent directory throws `SourceError`. | Test (TC-006) | -| FR-004-AC-3 | A `url` source throws `UnsupportedSourceError`. | Test (TC-007) | -| FR-004-AC-4 | A `git-subdir` source pinned to a tag returns `dir` ending in the subdir path, contains the subdir's files, and reports the tag's `sha` and the requested `ref`. | Test (TC-008) | -| FR-004-AC-5 | A whole-repo `git` source with no pin resolves to `HEAD` (latest commit); re-resolving the same cached URL at a tag exercises the fetch branch and resolves that tag's sha. | Test (TC-009) | -| FR-004-AC-6 | A `git` source pinned by `sha` checks out exactly that commit. | Test (TC-010) | -| FR-004-AC-7 | A `github` source resolved with an injected `GitRunner` performs no real git: the returned `sha` is the runner's output and the first git argv is `clone`. | Test (TC-011) | -| FR-004-AC-8 | An `npm` source resolved with an injected `NpmFetcher` extracts the tarball content (manifest present at the returned `dir`) and pins the **resolved version** as `sha`, echoing the requested version as `ref`. | Test (TC-022) | -| FR-004-AC-9 | An **exact-version** `npm` pin is served from cache on a second resolve (fetcher invoked once); an **unpinned** spec re-fetches every resolve (fetcher invoked each time) and pins the resolved version returned by the fetcher. | Test (TC-023) | -| FR-004-AC-10 | `defaultNpmFetcher` packs and extracts a **local** package folder via `npm pack` + `tar`, fully offline, returning the package version and an extracted content root containing `package.json`. | Test (TC-024) | -| FR-004-AC-11 | `npmPackArgs` builds the correct argv for a pinned spec (`@`), an unpinned spec (``), and includes `--registry ` when a registry is supplied. | Test (TC-025) | +| ID | Criteria | Verification | +| ------------ | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------------- | +| FR-004-AC-1 | A `path` source for an existing directory returns `{ dir }` pointing at that directory (its contents are readable). | Test (TC-006) | +| FR-004-AC-2 | A `path` source for a non-existent directory throws `SourceError`. | Test (TC-006) | +| FR-004-AC-3 | A `url` source throws `UnsupportedSourceError`. | Test (TC-007) | +| FR-004-AC-4 | A `git-subdir` source pinned to a tag returns `dir` ending in the subdir path, contains the subdir's files, and reports the tag's `sha` and the requested `ref`. | Test (TC-008) | +| FR-004-AC-5 | A whole-repo `git` source with no pin resolves to `HEAD` (latest commit); re-resolving the same cached URL at a tag exercises the fetch branch and resolves that tag's sha. | Test (TC-009) | +| FR-004-AC-6 | A `git` source pinned by `sha` checks out exactly that commit. | Test (TC-010) | +| FR-004-AC-7 | A `github` source resolved with an injected `GitRunner` performs no real git: the returned `sha` is the runner's output and the first git argv is `clone`. | Test (TC-011) | +| FR-004-AC-8 | An `npm` source resolved with an injected `NpmFetcher` extracts the tarball content (manifest present at the returned `dir`) and pins the **resolved version** as `sha`, echoing the requested version as `ref`. | Test (TC-022) | +| FR-004-AC-9 | An **exact-version** `npm` pin is served from cache on a second resolve (fetcher invoked once); an **unpinned** spec re-fetches every resolve (fetcher invoked each time) and pins the resolved version returned by the fetcher. | Test (TC-023) | +| FR-004-AC-10 | `defaultNpmFetcher` packs and extracts a **local** package folder via `npm pack` + `tar`, fully offline, returning the package version and an extracted content root containing `package.json`. | Test (TC-024) | +| FR-004-AC-11 | `npmPackArgs` builds the correct argv for a pinned spec (`@`), an unpinned spec (``), and includes `--registry ` when a registry is supplied. | Test (TC-025) | +| FR-004-AC-12 | `parseNpmPackJson` robustly parses the `npm pack --json` metadata: it scans past lifecycle-script stdout noise (including stray `[` brackets, empty arrays, and non-metadata noise arrays) and returns the first array whose first element is an npm-pack metadata object (an object with a string `filename`); on no-array/empty/invalid output it throws a descriptive `SourceError`. | Test (TC-027) | +| FR-004-AC-13 | An **unpinned** (or range) `npm` re-fetch clears any prior cached tarball + extraction before re-extracting, so the npm cache dir does not accumulate stale tarball artifacts across "latest" re-fetches. | Test (TC-028) | ## Dependencies diff --git a/spec/tests.md b/spec/tests.md index 1b7c89c..31d43e5 100644 --- a/spec/tests.md +++ b/spec/tests.md @@ -21,7 +21,7 @@ today under `make test` (vitest) at the 100% coverage gate (NFR-002). > **Concurrent-PR note.** TC-022…TC-054 are a shared TC-ID block also claimed by > the concurrent `feat/plugin-discovery` PR. This (`feat/npm-source-resolution`) -> PR lands first and uses TC-022…TC-026 for npm source resolution; the two +> PR lands first and uses TC-022…TC-028 for npm source resolution; the two > matrices will be **reconciled at the second merge** so the IDs do not collide. Tests fall into the following types: @@ -110,6 +110,9 @@ partial clones work offline. | FR-004 | AC-10: defaultNpmFetcher local-pack offline (npm pack + tar) | TC-024 — `resolveSource › "defaultNpmFetcher packs and extracts a local package offline"` | ✅ Unit (npm) | | FR-004 | AC-11: npmPackArgs builds pinned/unpinned + registry argv | TC-025 — `resolveSource › "npmPackArgs builds pinned, unpinned, and registry argv"` | ✅ Unit | | FR-004 | CON-3: rejects option-like npm package (injection guard) | TC-026 — `normalizeSource › "rejects malformed input"` (option-like `-x` package assertion) | ✅ Unit | +| FR-004 | AC-12: robust npm-pack-json parse (skip noise; metadata object) | TC-027 — `resolveSource › "parseNpmPackJson skips prepack noise and parses the trailing array"` (+ scans-past-empty / scans-past-numeric tests) | ✅ Unit | +| FR-004 | CON-4: descriptive SourceError on no-array/empty/invalid output | TC-027 — `resolveSource › "parseNpmPackJson throws SourceError on no-bracket output"` (+ empty-array / numeric / no-string-filename throw tests) | ✅ Unit | +| FR-004 | AC-13: unpinned re-fetch clears stale tarballs (cache hygiene) | TC-028 — `resolveSource › "unpinned re-fetch does not accumulate stale tarballs"` | ✅ Unit (fake) | | FR-005 | AC-1: missing / shape-invalid (`{}`) registry read as empty | TC-012 — `registry › "missing and malformed files read as empty"` | ✅ Unit | | FR-005 | AC-2: atomic write + nested-dir creation round-trips | TC-013 — `registry › "write is atomic and round-trips; upsert replaces by name"` | ✅ Unit | | FR-005 | AC-3: upsert replaces by name (count stays 1) | TC-013 — `registry › "write is atomic and round-trips; upsert replaces by name"` | ✅ Unit | @@ -138,36 +141,38 @@ partial clones work offline. ## Test Case Summary -| Test ID | Title | Type | Priority | Traces To | Status | -| ------- | --------------------------------------------------------------- | --------------- | -------- | ----------------------------------------------- | ------ | -| TC-001 | normalizeSource accepts every valid shape | Unit | P0 | FR-001-AC-1 | ✅ | -| TC-002 | normalizeSource rejects malformed input | Unit | P0 | FR-001-AC-2, -AC-3, -AC-4 | ✅ | -| TC-003 | toGitUrl expands shorthand / passes through URLs | Unit | P1 | FR-002-AC-1, -AC-2, -AC-3, -AC-4 | ✅ | -| TC-004 | validateMarketplaceManifest accepts valid (with/without name) | Unit | P0 | FR-003-AC-1, -AC-2 | ✅ | -| TC-005 | validateMarketplaceManifest rejects malformed manifests/entries | Unit | P0 | FR-003-AC-3, -AC-4, -AC-5, -AC-6 | ✅ | -| TC-006 | resolveSource path source: returns dir / missing throws | Unit | P0 | FR-004-AC-1, -AC-2 | ✅ | -| TC-007 | resolveSource url sources unsupported | Unit | P1 | FR-004-AC-3 | ✅ | -| TC-008 | resolveSource git-subdir sparse-checkout at a tag | Unit (git) | P0 | FR-004-AC-4, -CON-2 | ✅ | -| TC-009 | resolveSource whole-repo HEAD + re-fetch existing cache | Unit (git) | P0 | FR-004-AC-5 | ✅ | -| TC-010 | resolveSource sha pin checks out exact commit | Unit (git) | P0 | FR-004-AC-6 | ✅ | -| TC-011 | resolveSource github + injected runner (no real git) | Unit (fake) | P0 | FR-004-AC-7, -CON-1, NFR-003 | ✅ | -| TC-012 | registry missing / malformed reads empty | Unit | P0 | FR-005-AC-1 | ✅ | -| TC-013 | registry atomic write round-trip + upsert by name | Unit | P0 | FR-005-AC-2, -AC-3 | ✅ | -| TC-014 | installEntry named git-subdir materializes + records | Unit (git) | P0 | FR-006-AC-1, US-002-AC-2 | ✅ | -| TC-015 | installEntry derives name via readName | Unit (git) | P0 | FR-006-AC-2, US-002-AC-1 | ✅ | -| TC-016 | installEntry honors entry.path on whole-repo source | Unit (git) | P1 | FR-006-AC-3 | ✅ | -| TC-017 | installEntry symlink mode + re-install replaces | Unit (git) | P1 | FR-006-AC-4, US-002-AC-3 | ✅ | -| TC-018 | reconcile lazy install/skip + zero-git 2nd run | Unit (git) | P0 | FR-007-AC-1, -AC-2, US-001-AC-1, -AC-2, NFR-003 | ✅ | -| TC-019 | reconcile sync unchanged-stable / updated-moved | Unit (git) | P0 | FR-007-AC-3, US-001-AC-3 | ✅ | -| TC-020 | reconcile lazy re-materializes vanished target | Unit (git) | P1 | FR-007-AC-4 | ✅ | -| TC-021 | reconcile lazy sha pin unchanged/updated | Unit (git) | P0 | FR-007-AC-5 | ✅ | -| TC-022 | resolveSource npm resolve+extract+pin (fake & default fetcher) | Unit (fake/npm) | P0 | FR-004-AC-8 | ✅ | -| TC-023 | resolveSource exact-cache vs unpinned-refetch (fake fetcher) | Unit (fake) | P0 | FR-004-AC-9 | ✅ | -| TC-024 | defaultNpmFetcher local-pack offline (npm pack + tar) | Unit (npm) | P1 | FR-004-AC-10 | ✅ | -| TC-025 | npmPackArgs pinned/unpinned + registry argv | Unit | P1 | FR-004-AC-11 | ✅ | -| TC-026 | normalizeSource rejects option-like npm package (`-x`) | Unit | P0 | FR-004-CON-3 | ✅ | - -> TC-022…TC-026 belong to the shared TC-022…TC-054 block also used by the +| Test ID | Title | Type | Priority | Traces To | Status | +| ------- | ---------------------------------------------------------------- | --------------- | -------- | ----------------------------------------------- | ------ | +| TC-001 | normalizeSource accepts every valid shape | Unit | P0 | FR-001-AC-1 | ✅ | +| TC-002 | normalizeSource rejects malformed input | Unit | P0 | FR-001-AC-2, -AC-3, -AC-4 | ✅ | +| TC-003 | toGitUrl expands shorthand / passes through URLs | Unit | P1 | FR-002-AC-1, -AC-2, -AC-3, -AC-4 | ✅ | +| TC-004 | validateMarketplaceManifest accepts valid (with/without name) | Unit | P0 | FR-003-AC-1, -AC-2 | ✅ | +| TC-005 | validateMarketplaceManifest rejects malformed manifests/entries | Unit | P0 | FR-003-AC-3, -AC-4, -AC-5, -AC-6 | ✅ | +| TC-006 | resolveSource path source: returns dir / missing throws | Unit | P0 | FR-004-AC-1, -AC-2 | ✅ | +| TC-007 | resolveSource url sources unsupported | Unit | P1 | FR-004-AC-3 | ✅ | +| TC-008 | resolveSource git-subdir sparse-checkout at a tag | Unit (git) | P0 | FR-004-AC-4, -CON-2 | ✅ | +| TC-009 | resolveSource whole-repo HEAD + re-fetch existing cache | Unit (git) | P0 | FR-004-AC-5 | ✅ | +| TC-010 | resolveSource sha pin checks out exact commit | Unit (git) | P0 | FR-004-AC-6 | ✅ | +| TC-011 | resolveSource github + injected runner (no real git) | Unit (fake) | P0 | FR-004-AC-7, -CON-1, NFR-003 | ✅ | +| TC-012 | registry missing / malformed reads empty | Unit | P0 | FR-005-AC-1 | ✅ | +| TC-013 | registry atomic write round-trip + upsert by name | Unit | P0 | FR-005-AC-2, -AC-3 | ✅ | +| TC-014 | installEntry named git-subdir materializes + records | Unit (git) | P0 | FR-006-AC-1, US-002-AC-2 | ✅ | +| TC-015 | installEntry derives name via readName | Unit (git) | P0 | FR-006-AC-2, US-002-AC-1 | ✅ | +| TC-016 | installEntry honors entry.path on whole-repo source | Unit (git) | P1 | FR-006-AC-3 | ✅ | +| TC-017 | installEntry symlink mode + re-install replaces | Unit (git) | P1 | FR-006-AC-4, US-002-AC-3 | ✅ | +| TC-018 | reconcile lazy install/skip + zero-git 2nd run | Unit (git) | P0 | FR-007-AC-1, -AC-2, US-001-AC-1, -AC-2, NFR-003 | ✅ | +| TC-019 | reconcile sync unchanged-stable / updated-moved | Unit (git) | P0 | FR-007-AC-3, US-001-AC-3 | ✅ | +| TC-020 | reconcile lazy re-materializes vanished target | Unit (git) | P1 | FR-007-AC-4 | ✅ | +| TC-021 | reconcile lazy sha pin unchanged/updated | Unit (git) | P0 | FR-007-AC-5 | ✅ | +| TC-022 | resolveSource npm resolve+extract+pin (fake & default fetcher) | Unit (fake/npm) | P0 | FR-004-AC-8 | ✅ | +| TC-023 | resolveSource exact-cache vs unpinned-refetch (fake fetcher) | Unit (fake) | P0 | FR-004-AC-9 | ✅ | +| TC-024 | defaultNpmFetcher local-pack offline (npm pack + tar) | Unit (npm) | P1 | FR-004-AC-10 | ✅ | +| TC-025 | npmPackArgs pinned/unpinned + registry argv | Unit | P1 | FR-004-AC-11 | ✅ | +| TC-026 | normalizeSource rejects option-like npm package (`-x`) | Unit | P0 | FR-004-CON-3 | ✅ | +| TC-027 | parseNpmPackJson robust parse + descriptive parse-failure errors | Unit | P0 | FR-004-AC-12, -CON-4 | ✅ | +| TC-028 | unpinned npm re-fetch clears stale tarballs (cache hygiene) | Unit (fake) | P1 | FR-004-AC-13 | ✅ | + +> TC-022…TC-028 belong to the shared TC-022…TC-054 block also used by the > concurrent `feat/plugin-discovery` PR; IDs are reconciled at the second merge > (this PR lands first). @@ -175,11 +180,12 @@ partial clones work offline. ## Constraint Boundary Tests -| Constraint | Boundary / Case | Test Value | Test Case | Expected | -| ------------ | -------------------------------------------------- | ---------------------------------------- | -------------- | ---------------------------------------------------- | -| FR-004-CON-1 | package-manager subprocess is the sole side effect | injected fake `GitRunner` / `NpmFetcher` | TC-011, TC-022 | resolves with no real git/npm; argv[0]=`clone` | -| FR-004-CON-2 | blobless + sparse | `git-subdir` at `v0.2.0` | TC-008 | only the subdir present; tag sha resolved | -| FR-004-CON-3 | option-like npm package rejected | `{type:"npm", package:"-x"}` | TC-026 | `SourceError` "must not begin with -"; no `npm pack` | +| Constraint | Boundary / Case | Test Value | Test Case | Expected | +| ------------ | -------------------------------------------------- | --------------------------------------------- | -------------- | ------------------------------------------------------ | +| FR-004-CON-1 | package-manager subprocess is the sole side effect | injected fake `GitRunner` / `NpmFetcher` | TC-011, TC-022 | resolves with no real git/npm; argv[0]=`clone` | +| FR-004-CON-2 | blobless + sparse | `git-subdir` at `v0.2.0` | TC-008 | only the subdir present; tag sha resolved | +| FR-004-CON-3 | option-like npm package rejected | `{type:"npm", package:"-x"}` | TC-026 | `SourceError` "must not begin with -"; no `npm pack` | +| FR-004-CON-4 | no metadata array in `npm pack --json` output | `""`, `"[]"`, `"[1,2,3]"`, `'[{"name":"p"}]'` | TC-027 | `SourceError` "could not parse npm pack --json output" | --- @@ -212,12 +218,14 @@ partial clones work offline. - **Acceptance Criteria → Test Case coverage: 40 of 40 functional ACs (100%) map to an executed Test Case.** All ACs of FR-001…FR-007 (incl. FR-002-AC-5 whitespace - trimming via the padded-input assertion in TC-003, and the four new FR-004 npm ACs - AC-8…AC-11), both FR-004 constraints, and all 10 user-story ACs map to a real test - in `tests/index.test.ts`. NFR-001…NFR-004 are covered by the coverage gate, the - zero-git assertion, inspection, and analysis. -- The 25 TCs map to the tests in `tests/index.test.ts` (TC-022 spans both the - fake-fetcher and default-fetcher npm tests). All pass under `make test` at the + trimming via the padded-input assertion in TC-003, the four npm-resolution ACs + AC-8…AC-11, and the two npm-robustness ACs AC-12 robust-parse and AC-13 + cache-hygiene), all four FR-004 constraints (CON-1…CON-4), and all 6 user-story ACs + map to a real test in `tests/index.test.ts`. NFR-001…NFR-004 are covered by the + coverage gate, the zero-git assertion, inspection, and analysis. +- The 28 TCs map to the tests in `tests/index.test.ts` (TC-022 spans both the + fake-fetcher and default-fetcher npm tests; TC-027 spans the five `parseNpmPackJson` + noise/no-bracket/empty/numeric/no-filename tests). All pass under `make test` at the 100% coverage gate. - All six test-matrix rules are satisfied: every AC has a TC (Rule 1); the copy/symlink materialize options are both exercised (Rule 2, TC-014/TC-017); the @@ -256,4 +264,4 @@ Recorded during /spec-review. None block the spec; each is a candidate test/hard - The `url` source variant is deliberately unimplemented (FR-004-AC-3); there is therefore no happy-path TC for it, only the `UnsupportedSourceError` assertion (TC-007). This is correct, not a coverage gap. The `npm` variant **is** now - resolved (TC-022…TC-026). + resolved (TC-022…TC-028). diff --git a/src/resolve.ts b/src/resolve.ts index 8a37286..ddf728f 100644 --- a/src/resolve.ts +++ b/src/resolve.ts @@ -68,28 +68,34 @@ export function npmPackArgs( /** * Robustly parse the metadata array `npm pack --json` prints. Packing a local * dir runs its `prepack`/`prepare`, whose stdout precedes the JSON and may even - * contain stray `[` brackets (e.g. `[build] done`), so a naive - * `indexOf("[")` slice mis-parses. Instead, scan candidate `[` positions and - * take the first that parses to a non-empty JSON array. Throws a descriptive - * `SourceError` when no parseable array (or an empty one) is found. Exported - * pure so the noise/garbage branches stay unit-testable offline. + * contain stray `[` brackets (e.g. `[build] done`) or whole noise arrays (an + * empty `[]` or a numeric `[1,2,3]`), so a naive `indexOf("[")` slice + * mis-parses. Instead, scan **all** candidate `[` positions and accept the + * first that parses to an array whose first element is npm-pack metadata — an + * object carrying a string `filename`. Empty arrays, non-array-of-object noise, + * and unparseable slices are skipped. Throws a descriptive `SourceError` when no + * matching metadata array is found. Exported pure so the noise/garbage branches + * stay unit-testable offline. */ export function parseNpmPackJson(out: string): { filename: string; version: string; } { for (let i = out.indexOf("["); i >= 0; i = out.indexOf("[", i + 1)) { - let parsed: { filename: string; version: string }[]; + let parsed: unknown; try { parsed = JSON.parse(out.slice(i)); } catch { continue; } - if (parsed.length === 0) - throw new SourceError( - `npm pack returned no package metadata: ${out.trim()}`, - ); - return parsed[0]; + const first = (parsed as unknown[])[0]; + if ( + first && + typeof first === "object" && + typeof (first as { filename?: unknown }).filename === "string" + ) { + return first as { filename: string; version: string }; + } } throw new SourceError( `could not parse npm pack --json output: ${out.trim()}`, diff --git a/tests/index.test.ts b/tests/index.test.ts index 58b7521..6e2d989 100644 --- a/tests/index.test.ts +++ b/tests/index.test.ts @@ -462,6 +462,38 @@ describe("resolveSource", () => { expect(() => parseNpmPackJson("[]\n")).toThrow(SourceError); }); + // A bare `[]` printed before the real metadata array (e.g. an empty + // prepack-script result) must be scanned past, not treated as the answer. + test("parseNpmPackJson scans past a bare empty array before the real array", () => { + const out = `[]\n[{"filename":"p-1.2.3.tgz","version":"1.2.3"}]\n`; + expect(parseNpmPackJson(out)).toEqual({ + filename: "p-1.2.3.tgz", + version: "1.2.3", + }); + }); + + // A non-metadata noise array (e.g. `[1,2,3]` from a lifecycle script) must be + // scanned past in favor of the real npm-pack metadata array. + test("parseNpmPackJson scans past a numeric noise array before the real array", () => { + const out = `[1,2,3]\n[{"filename":"p-1.2.3.tgz","version":"1.2.3"}]\n`; + expect(parseNpmPackJson(out)).toEqual({ + filename: "p-1.2.3.tgz", + version: "1.2.3", + }); + }); + + // A numeric array is not npm-pack metadata: accepting it would yield + // `parsed[0] = 1` and a confusing tar failure, so it must throw instead. + test("parseNpmPackJson throws SourceError on a non-metadata numeric array", () => { + expect(() => parseNpmPackJson("[1,2,3]\n")).toThrow(SourceError); + }); + + // An array whose first element is an object lacking a string `filename` is not + // npm-pack metadata and must throw rather than be wrongly accepted. + test("parseNpmPackJson throws SourceError when the first element has no string filename", () => { + expect(() => parseNpmPackJson('[{"name":"p"}]\n')).toThrow(SourceError); + }); + // Disk hygiene: an unpinned re-fetch must not let stale tarballs pile up in // the npm cache dir; the prior fetch's artifacts are cleared before re-fetch. test("unpinned re-fetch does not accumulate stale tarballs", () => {