Skip to content

feat: resolve npm sources via npm pack#3

Merged
kreneskyp merged 4 commits into
mainfrom
feat/npm-source-resolution
Jun 28, 2026
Merged

feat: resolve npm sources via npm pack#3
kreneskyp merged 4 commits into
mainfrom
feat/npm-source-resolution

Conversation

@kreneskyp

Copy link
Copy Markdown
Contributor

What landed

resolveSource now resolves { type: "npm" } sources. An npm source is
npm packed and extracted under <cacheRoot>/npm/<key>, and the resolved
published version
acts as the durable sha pin (mirroring a git sha), with
the requested version echoed back as ref. Exact-version pins are served
from the npm cache; unpinned / range specs re-fetch every resolve to honor
"latest". Both the existing GitRunner and a new NpmFetcher are injectable so
tests run with no real git/npm.

New public exports (barrel src/index.ts): type NpmFetcher, defaultNpmFetcher.
npmPackArgs is intentionally kept internal to resolve.ts (not re-exported).

Why it stalled before

The repo enforces a 100% coverage gate. The real defaultNpmFetcher runs
npm pack + tar, and its version ? pkg@version : pkg / --registry argv
branches are network-only — unreachable from an offline test, so the gate
failed and the work never landed.

Fix: extract a pure, exported npmPackArgs(spec, destDir) helper so the
version + registry branches are covered by a fast offline unit test (TC-025),
plus an offline integration test that npm packs a local folder through the
real defaultNpmFetcher (TC-024) and through resolveSource with no injected
fetcher (covers the ?? defaultNpmFetcher fallback). Coverage is back to 100%
on branches/functions/lines/statements.

Spec backsync

The clean origin/main spec said npm was unsupported; it now matches the code:

  • FR-004 — npm resolution behavior, defaultNpmFetcher paragraph, CON-1
    reworded to "per-source package-manager subprocess" (git for git; npm pack +
    tar for npm), AC-3 narrowed to url-only, new AC-8..AC-11.
  • NFR-003 — side-effect wording covers npm pack + tar; the zero-git
    settled-reconcile guarantee is kept intact.
  • spec.md — §2.1/§2.2 (npm in scope, only url deferred), §3 + module table
    (resolve.ts gains NpmFetcher/defaultNpmFetcher/npmPackArgs), §8 source
    table + pinning (npm resolved version = the sha pin), §10 (UnsupportedSourceError
    = url only).
  • tests.md — TC-007 retitled url-only; new TC-022..TC-025 mapped to
    FR-004-AC-8..AC-11; coverage rows, error-path table, and summary counts updated.

Concurrent-PR reconciliation note

TC-022..TC-054 is a shared TC-ID block also claimed by the concurrent
feat/plugin-discovery PR. This PR lands first and uses TC-022..TC-025 for
npm resolution; the two matrices will be reconciled at the second merge so
the IDs don't collide (noted in spec/tests.md).

Gates

  • tsc --noEmit: clean
  • vitest run: 27 passed
  • coverage: 100% branches / functions / lines / statements
  • eslint + prettier: clean
  • quire validate: 0 errors (pre-existing EARS warnings on NFR-001/StR-002 only)

🤖 Generated with Claude Code

@kreneskyp kreneskyp requested a review from a team as a code owner June 27, 2026 22:18
Land npm source resolution in `resolveSource`. An `npm` source is now
`npm pack`ed and extracted under `<cacheRoot>/npm/<key>`; 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) <noreply@anthropic.com>
@kreneskyp kreneskyp force-pushed the feat/npm-source-resolution branch from a6dc226 to bd98f1a Compare June 27, 2026 22:26
…tion

`resolveNpm` passes the npm `package` into `npm pack <pkg>` 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) <noreply@anthropic.com>
@kreneskyp

Copy link
Copy Markdown
Contributor Author

Added a security hardening commit (3984df6): normalizeSource now rejects an npm package beginning with -, closing the new npm pack argument-injection sink this PR introduces (same second-order-command-line-injection class CodeQL flagged on the pre-existing git argv). FR-004-CON-3 + TC-026 added; coverage stays 100%.

The 3 CodeQL alerts currently shown are pre-existing git argv injections in resolve.ts:20/:77 on main (not introduced here) — those will be addressed in a separate hardening PR.

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
  `<destDir>/package` because a later exact-version cache hit serves
  `<cacheDir>/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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
@kreneskyp kreneskyp merged commit 4d7b41d into main Jun 28, 2026
5 checks passed
kreneskyp added a commit that referenced this pull request Jun 28, 2026
Reconcile the git argv-injection hardening (PR #8) with the npm source
resolution that landed on main (PR #3).

- src/sources.ts: unify to one trimmed `reqArg` guard used on both the npm
  `package` field (#3) and the git argv fields (#8: repo/url/path + optArg
  on ref/sha).
- spec/functional/FR-004: keep #3's npm ACs/CONs; renumber #8's additions
  after the highest existing id — CON-3→CON-5, AC-8/9/10→AC-14/15/16.
- spec/tests.md: renumber #8's git TCs after the npm block —
  TC-022/023/024→TC-029/030/031; update FR-coverage, Test Index, Constraint
  Boundary, Error-Path tables and coverage counts.
- tests/index.test.ts: update the three guard tests' TC tags to
  TC-029/030/031 so spec and code agree 1:1.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
kreneskyp added a commit that referenced this pull request Jun 28, 2026
… + fix CodeQL

Integrate main's npm-source-resolution (PR #3) and git-security (PR #8) work
with the plugin-discovery feature (search.ts). Resolved conflicts in
spec/spec.md, spec/non-functional/NFR-003, and spec/tests.md so both main's
npm/git content and the discovery content survive.

Renumber the discovery test cases off main's TC-001..TC-031 block: the
contiguous discovery block TC-022..TC-060 shifts +10 to TC-032..TC-070
(uniform map, relative order preserved). Updated every occurrence across
tests.md (FR/NFR coverage, Test Index, Constraint Boundary, Error-Path,
Edge Cases, coverage-summary prose), the discovery FR files (FR-008..FR-012),
and the TC tags in tests/search.test.ts. FR-004 (main) left intact.

Fix 4 HIGH CodeQL js/incomplete-url-substring-sanitization alerts in
tests/search.test.ts by replacing url.includes("<host>.com") routing checks
with strict `new URL(url).host === "<host>"` comparisons.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@kreneskyp kreneskyp mentioned this pull request Jun 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant