feat: resolve npm sources via npm pack#3
Merged
Conversation
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>
a6dc226 to
bd98f1a
Compare
…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>
Contributor
Author
|
Added a security hardening commit (3984df6): The 3 CodeQL alerts currently shown are pre-existing |
This was referenced Jun 28, 2026
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>
5 tasks
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>
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What landed
resolveSourcenow resolves{ type: "npm" }sources. An npm source isnpm packed and extracted under<cacheRoot>/npm/<key>, and the resolvedpublished version acts as the durable
shapin (mirroring a git sha), withthe requested
versionechoed back asref. Exact-version pins are servedfrom the npm cache; unpinned / range specs re-fetch every resolve to honor
"latest". Both the existing
GitRunnerand a newNpmFetcherare injectable sotests run with no real git/npm.
New public exports (barrel
src/index.ts):type NpmFetcher,defaultNpmFetcher.npmPackArgsis intentionally kept internal toresolve.ts(not re-exported).Why it stalled before
The repo enforces a 100% coverage gate. The real
defaultNpmFetcherrunsnpm pack+tar, and itsversion ? pkg@version : pkg/--registryargvbranches 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 theversion + registry branches are covered by a fast offline unit test (TC-025),
plus an offline integration test that
npm packs a local folder through thereal
defaultNpmFetcher(TC-024) and throughresolveSourcewith no injectedfetcher (covers the
?? defaultNpmFetcherfallback). Coverage is back to 100%on branches/functions/lines/statements.
Spec backsync
The clean
origin/mainspec said npm was unsupported; it now matches the code:defaultNpmFetcherparagraph, CON-1reworded to "per-source package-manager subprocess" (git for git;
npm pack+tarfor npm), AC-3 narrowed tourl-only, new AC-8..AC-11.npm pack+tar; the zero-gitsettled-reconcile guarantee is kept intact.
urldeferred), §3 + module table(
resolve.tsgainsNpmFetcher/defaultNpmFetcher/npmPackArgs), §8 sourcetable + pinning (npm resolved version = the
shapin), §10 (UnsupportedSourceError=
urlonly).url-only; new TC-022..TC-025 mapped toFR-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-discoveryPR. This PR lands first and uses TC-022..TC-025 fornpm 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: cleanvitest run: 27 passedquire validate: 0 errors (pre-existing EARS warnings on NFR-001/StR-002 only)🤖 Generated with Claude Code