Skip to content

Dependency version pinning behind VERYFRONT_DEPENDENCY_PINNING (Phase 0, #240) - #3114

Merged
kojiwakayama merged 32 commits into
mainfrom
phase0/dependency-pinning
Jul 30, 2026
Merged

kojiwakayama merged 32 commits into
mainfrom
phase0/dependency-pinning

Conversation

@kojiwakayama

@kojiwakayama kojiwakayama commented Jul 25, 2026 •

Copy link
Copy Markdown
Contributor

Dependency version pinning (Phase 0 of veryfront/veryfront-issue-inbox#240)

Behind the new VERYFRONT_DEPENDENCY_PINNING env flag (default OFF; flag off is behavior-identical to current main).

  • readProjectDependencyVersions exposes the full merged dependencies map only while pinning is enabled; flag-off reads retain the previous react/veryfront-only cache shape.
  • Bare-import URL construction in the browser BareStrategy, SSR adapter, and dev-server esbuild plugin resolves versions in order: inline specifier version → exact package.json pin → cached npm-registry result for an undeclared package → current unversioned fallback.
  • The npm resolution client is non-blocking and per-project: an exact declaration is cached synchronously; a package with no project declaration resolves dist-tags.latest in the background; failures never fail or block a render.
  • A non-exact project declaration (range, dist-tag, alias, workspace reference, and similar) is never coerced to registry latest, because that could violate the declared constraint. It retains the current fallback until the platform/API update path normalizes it to an exact pin. A declaration change also invalidates older cached and in-flight results.
  • Browser resolution reports a newly resolved exact pin to POST /projects/:projectId/dependencies/resolve best-effort/async; 404 and transport failures are tolerated.

Semver policy for this renderer phase: exact package.json versions win as-is; truly undeclared packages may resolve to latest; non-exact declarations are not locally normalized. Max-satisfying range normalization remains the explicit follow-up decision in issue #240 rather than adding a semver engine to renderer core in this PR.

Part of the staged plan in veryfront/veryfront-issue-inbox#240. This establishes the flag-gated lookup and write-back path while preserving current behavior whenever a safe exact pin is unavailable.

Known flag-off deviations (reviewed, intentional)

Three behavior changes apply regardless of the flag state. They are deliberate correctness improvements, disclosed here because the guarantee above otherwise reads as absolute:

  • SSR adapter import coverage: landed on main via Extend SSR import rewriter to cover side-effect and dynamic import forms #3175 and merged back — the SSR import-coverage hunks are no longer part of this diff; this branch's ssr-adapter delta is now pure pinning plumbing.
  • React version validation: isValidReactVersion accepts the same exact-SemVer forms as dependency pinning (v prefix, prerelease, build metadata) instead of bare X.Y.Z. Previously-rejected exact versions no longer silently fall back to the default React version.
  • Unversioned-import warning: hasVersionSpecifier uses the package-specifier parser, so dist-tags (@next, @beta) count as version specifiers and no longer trigger the reproducibility warning. Logging-only.

Implements Phase 0 renderer-side pin resolution (veryfront-issue-inbox#240).
All behaviour is gated behind VERYFRONT_DEPENDENCY_PINNING=1 (default OFF);
flag-off is byte-identical to the previous code path.

Changes:
- src/release-assets/constants.ts: add DEPENDENCY_PINNING_ENV_FLAG constant
- src/transforms/esm/package-registry.ts: extend readProjectDependencyVersions
  to return full merged deps map; add getProjectDependenciesSync sync accessor;
  add _primeDependenciesCache test helper
- src/transforms/esm/npm-registry-client.ts (new): non-blocking npm registry
  client with in-process per-project cache, first-writer-wins semantics,
  in-flight deduplication, AbortController timeout, and fire-and-forget
  write-back via POST /projects/:id/dependencies/resolve; export isExactSemver
- src/transforms/import-rewriter/strategies/bare-strategy.ts: pin-resolution
  ladder (package.json pin → registry cache → schedule background fetch)
  applied to bare unversioned browser imports when flag is ON; compound ranges
  in package.json are guarded with isExactSemver to prevent malformed URLs
- src/transforms/import-rewriter/ssr-adapter.ts: apply same pin lookup in
  rewriteBareImports; add optional projectDir to SSRRewriteOptions; remove
  dead pinSuffix variable; guard compound ranges with isExactSemver
- src/server/handlers/dev/files/esbuild-plugins.ts: buildPinnedEsmUrl helper
  injects pinned version into esm.sh URLs when flag is ON; remove dead toEsmUrl
- src/transforms/esm/http-bundler.ts: extract buildBareSpecifierEsmUrl helper
  that uses parseBarePackageSpecifier so subpaths like "lodash/fp" produce
  correct "lodash@4.17.21/fp" URLs (not malformed "lodash/fp@4.17.21");
  add optional getPinVersion callback to CreateHTTPPluginOptions;
  wrap postDependencyResolution body in try/catch to prevent unhandled rejection
- src/transforms/esm/npm-registry-client.test.ts (new): 3 suites covering
  flag detection, cache semantics, and mocked-fetch background resolution
- src/transforms/esm/http-bundler.test.ts: add buildBareSpecifierEsmUrl suite
  with subpath and scoped package test cases
- src/transforms/import-rewriter/strategies/bare-strategy.test.ts: added
  flag-off regression suite, pin-ladder suite, and compound range test
@kojiwakayama
kojiwakayama requested a review from kwakayama as a code owner July 25, 2026 21:00
Copilot AI review requested due to automatic review settings July 25, 2026 21:00

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c4f98ccc04

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread src/transforms/import-rewriter/ssr-adapter.ts Outdated
Comment thread src/server/handlers/dev/files/esbuild-plugins.ts Outdated
Comment thread src/transforms/import-rewriter/strategies/bare-strategy.ts Outdated
Comment thread src/transforms/import-rewriter/strategies/bare-strategy.ts

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds an opt-in (default off) dependency version pinning mechanism for bare npm imports, aiming to make esm.sh URLs deterministic by resolving versions from project package.json and (best-effort) npm registry metadata, while preserving current behavior when the flag is disabled.

Changes:

  • Introduces VERYFRONT_DEPENDENCY_PINNING flag and a new in-process npm registry client with per-project caching and non-blocking background resolution.
  • Expands project dependency detection to cache the full merged dependencies map (for synchronous pin lookups during rewriting).
  • Updates multiple import-rewrite surfaces (browser bare strategy, SSR adapter, dev esbuild plugin, http bundler) to optionally inject pinned versions, with new/updated tests.

Verification

  • Not run here (no runtime available). Suggested next step:
    • deno test --no-check --allow-all src/transforms/esm/npm-registry-client.test.ts src/transforms/import-rewriter/strategies/bare-strategy.test.ts src/transforms/esm/http-bundler.test.ts

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
src/transforms/import-rewriter/strategies/bare-strategy.ts Adds optional pin resolution for browser bare imports under the env flag.
src/transforms/import-rewriter/strategies/bare-strategy.test.ts Adds regression + pinning ladder tests for the bare strategy.
src/transforms/import-rewriter/ssr-adapter.ts Adds optional pin injection for SSR bare import rewrites when projectDir is provided.
src/transforms/esm/package-registry.ts Extends dependency cache to store full dependency map and adds sync accessor for rewriters.
src/transforms/esm/npm-registry-client.ts New npm registry client with caching, background fetch, and optional write-back POST.
src/transforms/esm/npm-registry-client.test.ts Unit tests for env flag behavior, cache semantics, and mocked background resolution.
src/transforms/esm/http-bundler.ts Adds optional pin injection for esm.sh URL construction via a callback.
src/transforms/esm/http-bundler.test.ts Adds unit tests for the new bare-specifier URL builder.
src/server/handlers/dev/files/esbuild-plugins.ts Injects pinned versions into dev bundling bare import rewrites when available.
src/release-assets/constants.ts Adds the DEPENDENCY_PINNING_ENV_FLAG constant.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/server/handlers/dev/files/esbuild-plugins.ts Outdated
Comment thread src/transforms/esm/http-bundler.ts Outdated
Comment thread src/transforms/esm/http-bundler.test.ts Outdated
Comment thread src/transforms/esm/npm-registry-client.ts Outdated
…240)

Addresses four PR bot comments on veryfront-code #3114:

1. SSR wiring (P1): add projectDir to all ssrRewriteOptions construction
   sites (module-server.ts ×3, module-batch-handler.ts) so that bare
   import pin resolution actually runs on the SSR code path.

2. esbuild browser-bundler wiring (P1): pass projectDir to
   createBareExternalPlugin in browser-module-bundler.ts so that
   buildPinnedEsmUrl can consult the dependency cache.

3. Range policy (P1): never strip a semver range prefix (^, ~, >=) to
   manufacture a pin that package.json didn't literally contain.
   - npm-registry-client.ts: scheduleNpmVersionResolution now uses
     isExactSemver(rangeHint) — only short-circuits when the raw hint is
     already an exact version; caret/tilde ranges go to the registry client.
   - bare-strategy.ts, ssr-adapter.ts, esbuild-plugins.ts: resolvePinnedVersion
     / resolveBareImportPin / buildPinnedEsmUrl all changed to
     isExactSemver(rawPin) on the raw package.json value (no stripping).
   - stripSemverRange import removed from npm-registry-client.ts,
     bare-strategy.ts, ssr-adapter.ts, esbuild-plugins.ts.

4. Dist-tag preservation (P2): hasVersionSpecifier now delegates to
   parseBarePackageSpecifier so that specifiers like pkg@next, pkg@beta,
   pkg@canary carry a non-null version and are never treated as unversioned
   candidates for pin injection.

Tests updated / added:
- npm-registry-client.test.ts: two caret-range tests rewritten to assert
  the new policy (caret range leaves cache cold; background fetch is scheduled).
- bare-strategy.test.ts: new test verifies pkg@next is preserved and the
  cached numeric version (4.0.0) is not substituted.
Copilot AI review requested due to automatic review settings July 25, 2026 21:49
Guard pin injection with parsed.version === null so specifiers that
already carry an inline version (lodash@4.17.21) or dist-tag (pkg@next)
are never overridden by the getPinVersion callback.

Adds two new tests:
- does not override an inline numeric version even when a pin callback is provided
- does not override an inline dist-tag (pkg@next) with a numeric pin

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.

Comments suppressed due to low confidence (5)

src/transforms/esm/http-bundler.ts:73

  • buildBareSpecifierEsmUrl() injects a pin even when the specifier already includes an inline version or dist-tag (e.g. "pkg@4.17.21" or "pkg@next"), which breaks the stated priority order (inline specifier must win). Only inject a pin when the parsed specifier has no version component.
    const parsed = parseBarePackageSpecifier(path);
    // Only inject a pin when the specifier has no inline version specifier.
    // An inline version (including dist-tags like @next) must be preserved as-is.
    if (parsed && parsed.version === null) {
      const pinVersion = getPinVersion(parsed.packageName);

src/transforms/esm/http-bundler.test.ts:116

  • buildBareSpecifierEsmUrl() has tests for pin injection, but no regression test asserting that inline versions and dist-tags are preserved (and not overridden by the pin callback). Adding this test would prevent reintroducing the override bug.
    it("injects pin version for a simple package name", () => {
      assertEquals(
        buildBareSpecifierEsmUrl("lodash", () => "4.17.21"),
        "https://esm.sh/lodash@4.17.21",
      );

src/transforms/import-rewriter/ssr-adapter.ts:133

  • When VERYFRONT_DEPENDENCY_PINNING is enabled and there is no exact package.json pin, resolveBareImportPin returns undefined without scheduling an npm registry lookup. This prevents the cache from ever warming for SSR rewrites. Schedule a background resolution on cache miss so subsequent renders can use the resolved pin.
  // semver — never strip range prefixes to manufacture a pin.
  if (rawPin && isExactSemver(rawPin)) return rawPin;

  return getCachedNpmVersion(parsed.packageName, projectDir);
}

src/server/handlers/dev/files/esbuild-plugins.ts:325

  • buildPinnedEsmUrl falls back to an unversioned esm.sh URL when both the package.json exact pin and the npm version cache are missing, but it does not schedule a registry lookup. Without scheduling, the npm cache stays cold and pinning never activates for range-based dependencies. Schedule scheduleNpmVersionResolution() on cache miss.
      const version = (rawPin && isExactSemver(rawPin))
        ? rawPin
        : getCachedNpmVersion(parsed.packageName, projectDir);
      if (version) {

src/transforms/esm/npm-registry-client.ts:67

  • The scheduleNpmVersionResolution() JSDoc says it stores an exact version "after stripping range chars", but the implementation intentionally does not strip range prefixes (it only short-circuits when the raw hint is already an exact semver). Update the comment to match the actual policy.
 * - If rangeHint is an exact semver (after stripping range chars), it is stored
 *   directly without a network fetch and onResolved fires synchronously.

Comment thread src/transforms/import-rewriter/ssr-adapter.ts Outdated
Comment thread src/server/handlers/dev/files/esbuild-plugins.ts Outdated
Comment thread src/transforms/esm/npm-registry-client.ts Outdated
Copilot AI review requested due to automatic review settings July 25, 2026 21:55
Expose _pendingResolutions() from npm-registry-client — returns a promise
that settles when all background npm registry fetches complete. Tracks each
fire-and-forget resolution promise in a module-level Set; the finally clause
removes it after settlement.

Wire up in both test describe blocks:
- getCachedNpmVersion: add beforeEach fetch mock (503) so caret-range tests
  never open real network connections, and await _pendingResolutions() in
  afterEach before the sanitizer inspects the test teardown.
- scheduleNpmVersionResolution: replace the brittle setTimeout(r, 1) tick
  yield with await _pendingResolutions() for the same guarantee.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (4)

src/transforms/esm/npm-registry-client.ts:151

  • This implementation always resolves to npm's dist-tags.latest and does not attempt max-satisfying selection for semver ranges. That differs from the PR description ("dist-tags.latest / max-satisfying for ranges"). Either update the description/semver policy or extend the client to resolve ranges against the published versions list.
      headers: { Accept: "application/vnd.npm.install-v1+json" },
      signal: controller.signal,
    });

src/transforms/import-rewriter/ssr-adapter.ts:133

  • When pinning is enabled, this path only checks package.json exact pins and the in-process npm cache, but it never schedules a background npm resolution when the cache is cold. That means the SSR adapter will never hit the "npm-registry resolution client" step unless some other code path has already warmed the cache for the same project.
function resolveBareImportPin(bareSpecifier: string, projectDir: string): string | undefined {
  const parsed = parseBarePackageSpecifier(bareSpecifier);
  if (!parsed || parsed.version) return undefined; // already versioned inline

  const allDeps = getProjectDependenciesSync(projectDir);
  const rawPin = allDeps?.[parsed.packageName];
  // Only use the raw package.json value as a pin when it is already an exact
  // semver — never strip range prefixes to manufacture a pin.
  if (rawPin && isExactSemver(rawPin)) return rawPin;

  return getCachedNpmVersion(parsed.packageName, projectDir);
}

src/server/handlers/dev/files/esbuild-plugins.ts:325

  • When pinning is enabled, this plugin only reads package.json exact pins and the in-process npm cache; it never schedules a background npm lookup when both are cold. As a result, the dev-server esbuild path will not actually trigger npm resolution on its own (it can only benefit from cache warmed elsewhere).
function buildPinnedEsmUrl(path: string, projectDir: string | undefined): string {
  if (isDependencyPinningEnabled() && projectDir) {
    const parsed = parseBarePackageSpecifier(path);
    if (parsed && !parsed.version) {
      const allDeps = getProjectDependenciesSync(projectDir);
      const rawPin = allDeps?.[parsed.packageName];
      // Only use the raw package.json value when it is already an exact semver —
      // never strip range prefixes to manufacture a pin.
      const version = (rawPin && isExactSemver(rawPin))
        ? rawPin
        : getCachedNpmVersion(parsed.packageName, projectDir);
      if (version) {

src/transforms/import-rewriter/strategies/bare-strategy.test.ts:305

  • This comment refers to "strip" behavior, but the current strategy intentionally does not strip semver ranges; it simply ignores non-exact pins via isExactSemver(). Updating the comment will keep the test aligned with the actual policy.
      // Compound ranges like ">=1.0.0 <2.0.0" strip to "1.0.0 <2.0.0" which is
      // not a valid semver and would produce a malformed esm.sh URL. The strategy
      // must skip it and fall through to the npm registry cache instead.

Comment thread src/transforms/esm/npm-registry-client.ts Outdated
Comment thread src/transforms/import-rewriter/strategies/bare-strategy.test.ts Outdated
Copilot AI review requested due to automatic review settings July 25, 2026 21:59

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (4)

src/transforms/esm/npm-registry-client.ts:76

  • The scheduleNpmVersionResolution() JSDoc still says the exact-version short-circuit happens "after stripping range chars", but the implementation intentionally does not strip ranges anymore (it only short-circuits when the raw hint is already an exact semver). Update the doc to match the actual policy so callers/tests don’t rely on the old behavior.
/**
 * Schedule a non-blocking npm registry lookup for the package.
 *
 * - If rangeHint is an exact semver (after stripping range chars), it is stored
 *   directly without a network fetch and onResolved fires synchronously.
 * - Otherwise a background fetch is started. The result is stored in the cache
 *   and onResolved is called when it completes.

src/transforms/esm/npm-registry-client.ts:117

  • scheduleNpmVersionResolution() ignores rangeHint when doing a network lookup (it always fetches dist-tags.latest). For package.json ranges (for example "^1.2.3") or dist-tags (for example "next"), this can pin a version that does not satisfy the project’s declared constraint, which conflicts with the PR description (max-satisfying / dist-tag resolution) and can introduce breaking upgrades behind the flag.
  const resolution: Promise<void> = fetchLatestNpmVersion(packageName)
    .then((version) => {
      if (version) {
        setCachedVersion(projectDir, packageName, version);
        onResolved?.(version, packageName, projectDir);
      }

src/transforms/import-rewriter/strategies/bare-strategy.test.ts:236

  • afterEach() should await _pendingResolutions() before restoring fetch/clearing caches. Otherwise, background npm lookups triggered by rewrite() can still be in flight when teardown completes, leading to flaky leak-sanitizer failures.
    afterEach(() => {
      setEnv(DEPENDENCY_PINNING_ENV_FLAG, originalFlag ?? "");
      _clearNpmVersionCache();
      clearReactVersionCache();
      globalThis.fetch = originalFetch;
    });

src/transforms/import-rewriter/ssr-adapter.ts:133

  • resolveBareImportPin() can return getCachedNpmVersion(), but this file never schedules a background resolution when the cache is cold. In this PR, scheduleNpmVersionResolution() is only called from the browser BareStrategy, so SSR-only flows will never warm the npm cache and will always fall back to unversioned esm.sh URLs even when pinning is enabled.
function resolveBareImportPin(bareSpecifier: string, projectDir: string): string | undefined {
  const parsed = parseBarePackageSpecifier(bareSpecifier);
  if (!parsed || parsed.version) return undefined; // already versioned inline

  const allDeps = getProjectDependenciesSync(projectDir);
  const rawPin = allDeps?.[parsed.packageName];
  // Only use the raw package.json value as a pin when it is already an exact
  // semver — never strip range prefixes to manufacture a pin.
  if (rawPin && isExactSemver(rawPin)) return rawPin;

  return getCachedNpmVersion(parsed.packageName, projectDir);
}

Comment thread src/transforms/import-rewriter/strategies/bare-strategy.test.ts
@kojiwakayama
kojiwakayama enabled auto-merge July 25, 2026 22:10
…und version cache

SSR path (ssr-adapter.ts): resolveBareImportPin now calls
scheduleNpmVersionResolution when both the package.json pin and the
npm registry cache are cold. The first SSR render falls back to an
unversioned esm.sh URL; subsequent renders use the pinned version.
New test in ssr-adapter.test.ts verifies both renders.

Dev-server path (esbuild-plugins.ts): buildPinnedEsmUrl calls
scheduleNpmVersionResolution when no cached version is available,
warming the cache for the next bundler run. New test verifies the
background fetch warms the npm cache via createBareExternalPlugin.

Memory bound (npm-registry-client.ts): versionCache is now capped at
VERSION_CACHE_MAX_PROJECTS = 256 project directories. When the cap is
reached, the oldest entry (Map insertion order) is evicted. Prevents
unbounded growth in long-running servers with many projects.

Docstring fix (npm-registry-client.ts): JSDoc for
scheduleNpmVersionResolution no longer says "after stripping range
chars" — the implementation checks isExactSemver on the raw value.

Test hygiene (bare-strategy.test.ts):
- Import and await _pendingResolutions() in the flag-on afterEach so
  any background fetch triggered by cold-cache tests is drained before
  the Deno leak sanitizer runs.
- Rename the test that calls scheduleNpmVersionResolution with an exact
  version to not imply a registry fetch happened.
- Fix the compound-range test comment to remove the stale "strip to"
  language.
Copilot AI review requested due to automatic review settings July 26, 2026 04:13

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

src/transforms/esm/npm-registry-client.ts:176

  • fetchLatestNpmVersion() always returns dist-tags.latest and scheduleNpmVersionResolution() does not use rangeHint to select a max-satisfying version for semver ranges. This conflicts with the PR description's "dist-tags.latest / max-satisfying for ranges" behavior. Either update the description/policy to state that ranges currently resolve to latest, or extend the implementation to resolve max-satisfying for ranges (requires fetching versions and applying semver range matching).
    const data = await res.json() as { "dist-tags"?: Record<string, string> };
    const latest = data["dist-tags"]?.latest ?? null;
    if (latest) logger.debug("npm registry resolved version", { packageName, version: latest });
    return latest;

Comment thread src/transforms/esm/npm-registry-client.ts Outdated
…cribe

The describe block added in e8f33c1 carried { sanitizeResources: false,
sanitizeOps: false } which pushed the opt-out count from 408 to 410,
exceeding the lint:sanitizer-baseline ratchet.

The test is already leak-clean: fetch is mocked in beforeEach, the afterEach
awaits _pendingResolutions() (draining in-flight registry fetches) and then
calls esbuild.stop() — same pattern as the existing sanitizer-clean describes
in the same file. Remove the opt-out; baseline is back at 408/408.
Copilot AI review requested due to automatic review settings July 26, 2026 04:20

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

src/transforms/esm/npm-registry-client.ts:130

  • scheduleNpmVersionResolution accepts a rangeHint, but it is never used for non-exact values: the code always calls fetchLatestNpmVersion(packageName) and caches dist-tags.latest. This can pin a version that does not satisfy a package.json range (or dist-tag), and it diverges from the PR description’s “max-satisfying for ranges” behavior.
  const resolution: Promise<void> = fetchLatestNpmVersion(packageName)
    .then((version) => {
      if (version) {
        setCachedVersion(projectDir, packageName, version);
        onResolved?.(version, packageName, projectDir);
      }

Comment thread src/transforms/esm/http-bundler.ts Outdated
@kojiwakayama
kojiwakayama disabled auto-merge July 26, 2026 06:36
@kojiwakayama

Copy link
Copy Markdown
Contributor Author

⚠️ Auto-merge disabled — holding for a correctness fix. Please do not approve-and-merge until the follow-up commit lands.

Pre-merge verification against a real on-disk package.json (not the _primeDependenciesCache test helper) found that pins are silently ignored for a class of projects.

Probe, flag ON, temp project with dependencies: { lodash: "4.17.20" }:

Config getProjectDependenciesSync Emitted URL
config = null {"lodash":"4.17.20"} https://esm.sh/lodash@4.17.20 ✅
config = {react:{version:"19.2.4"}} undefined https://esm.sh/lodash ❌

Root cause: getProjectDependenciesSync (package-registry.ts:125) is a pure lookup over dependencyVersionCache, which is populated only by the async readProjectDependencyVersions. In the render path that runs only as a side effect of resolveProjectReactVersion, which early-returns at step 1 (config.react.version) and step 2 (config.client.cdn.versions.react) without ever reading package.json. The same hazard exists at build-app-route-renderer.ts:124-125, where explicitReactVersion ?? await resolveProjectReactVersion(...) skips the call whenever an explicit version is passed down.

Impact: any project that sets react.version in veryfront.config.ts gets its declared pins ignored, and the background resolver then caches dist-tags.latest — so the user's explicit pin is actively replaced by latest. That is precisely the failure mode this PR exists to eliminate.

Fix in progress: make dependency-cache warm-up explicit and flag-gated at the transform/render entry points (a shared ensureProjectDependenciesLoaded(projectDir)), independent of how reactVersion was obtained, covering the app route renderer (both branches), module server + batch handlers, SSR adapter callers, and the dev esbuild plugin. Regression tests will use a real temp package.json and assert pinned output in all three configurations — deliberately not using _primeDependenciesCache, since that helper is what masked this.

…act config

Bug: getProjectDependenciesSync (the sync pin lookup used by bare-import
rewriting) relies on dependencyVersionCache, which is populated only by
readProjectDependencyVersions. That call was buried inside
resolveProjectReactVersion — specifically at its step 3 (package.json
detection). When config.react.version is set (step 1 early-return) or when
explicitReactVersion is supplied to the caller, readProjectDependencyVersions
was never called, leaving the cache permanently cold for those projects. The
result: every bare npm import was emitted without a version pin, and the
background resolver then cached dist-tags.latest — actively replacing the
declared package.json pin with latest.

Fix: add ensureProjectDependenciesLoaded(projectDir) to package-registry.ts.
It is flag-gated (no-op when VERYFRONT_DEPENDENCY_PINNING is off) and
mtime-cached (repeat calls within the same process are cheap). Call it at
every entry point that rewrites bare imports, independent of how reactVersion
was obtained:
  - build-app-route-renderer.ts (covers both explicitReactVersion and resolved branches)
  - module-server-handler.ts (in parallel with resolveProjectReactVersion)
  - batch-module-handler.ts (no resolveProjectReactVersion call exists here)
  - esbuild-plugins.ts createBareExternalPlugin (via onStart hook before onResolve fires)

Regression tests in package-registry.test.ts use a real temp dir and a real
package.json (no _primeDependenciesCache) and assert pinned URLs are emitted
in both the config-null path and the config.react.version path that was
previously broken.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot wasn't able to review this pull request because it exceeds the maximum number of lines (20,000). Try reducing the number of changed lines and requesting a review from Copilot again.

@kojiwakayama
kojiwakayama marked this pull request as draft July 27, 2026 06:57
auto-merge was automatically disabled July 27, 2026 06:57

Pull request was converted to draft

Copilot AI review requested due to automatic review settings July 27, 2026 07:22

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot wasn't able to review this pull request because it exceeds the maximum number of lines (20,000). Try reducing the number of changed lines and requesting a review from Copilot again.

@kojiwakayama

Copy link
Copy Markdown
Contributor Author

Merge readiness audit

Readiness: 54/100
Verdict: Not ready
Reviewed head: 4b4e028

The branch has strong test evidence, every check on the current head is green, no review threads remain, and the earlier page-module snapshot gap was fixed in 5ebb383. The default-off rollout and explicit snapshot model reduce immediate production exposure.

The PR is still draft, changes 243 files with 21,965 insertions, and conflicts with current main in 12 files across the lockfile, build pipeline, cache keys, HTML injection, release assets, RSC hydration, and generated bundles. Resolve those conflicts deliberately and rerun the full suite on the integrated head. Before enabling the flag, repeat flag-off equivalence checks plus flag-on staging for browser, SSR, build, cache, and RSC paths. The size and conflict surface require fresh code-owner review after integration.

Merge current origin/main into phase0/dependency-pinning with explicit conflict resolution across build, cache, HTML injection, release assets, RSC hydration, and lock/generated artifacts. The resolution keeps main's release-asset module maps, route-directory handling, logging changes, generated bundles, and dependency graph while preserving the PR's dependency snapshot threading, pin headers, cache identity, and recovery behavior.

Constraint: Normal merge history required; no rebase or force push.

Constraint: Current-main release asset hydration fixes must remain intact.

Rejected: Prefer one conflict side wholesale | would drop either release asset hydration or dependency snapshot threading.

Confidence: high

Scope-risk: moderate

Tested: deno task generate

Tested: focused dependency-pinning unit slice, 19 files, 462 steps

Tested: deno task test:e2e:rsc-browser, 3 files, 6 steps

Tested: SSR/RSC server slice, 16 files, 333 steps

Tested: deno task audit

Tested: deno task typecheck

Tested: git diff --check and git diff --cached --check

Not-tested: Full repository test suite before push; normal git push hook and CI remain to run exact-head coverage.
Copilot AI review requested due to automatic review settings July 29, 2026 21:11
@kojiwakayama
kojiwakayama marked this pull request as ready for review July 29, 2026 21:12

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot wasn't able to review this pull request because it exceeds the maximum number of lines (20,000). Try reducing the number of changed lines and requesting a review from Copilot again.

@kojiwakayama

Copy link
Copy Markdown
Contributor Author

Exact-head readiness update for 045b1fd:

  • Current main was integrated with a normal merge; all 12 conflicts were resolved deliberately.
  • Local focused dependency, build, cache, HTML, SSR, RSC, browser, audit, typecheck, format, and diff checks passed.
  • Exact-head GitHub CI is fully green, including unit, integration, RSC browser E2E, binary E2E, npm smoke, coverage, CodeQL, and dependency audit.
  • All 24 review threads are resolved.
  • Independent integration review recommends approval.
  • The PR is open, non-draft, and mergeable. No auto-merge or queue action is configured.

@kwakayama Please review and approve this exact head when ready. The protected-branch approval is the only remaining merge gate.

Review findings on #3114:

- bare-strategy's local isPinningEnabledForRewrite now applies the same
  "on:unknown" guard as the canonical helper in dependency-resolution.ts.
  An unknown dependency state (unreadable package.json) previously entered
  the pinning-on branch during SSR and emitted an esm.sh URL instead of
  stripping the version for node_modules resolution. Pinned by a test.
- resolveDependencyWritebackTarget rejects any present releaseId,
  including an empty string, instead of relying on truthiness
  (fail closed), and now has unit tests covering every gate.
- The flag-off mtime cache shortcut no longer requires a non-null mtime,
  restoring main's caching behavior for filesystems that do not report
  mtime. The pinning-on path never used this shortcut.
Copilot AI review requested due to automatic review settings July 30, 2026 03:15

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot wasn't able to review this pull request because it exceeds the maximum number of lines (20,000). Try reducing the number of changed lines and requesting a review from Copilot again.

Mirrors the same removal on fix/ssr-import-coverage (#3175 review):
the bare-import matcher's first-character class means a specifier
starting with "/" never reaches this function, so the startsWith("//")
branch was dead code. Keeping both branches byte-identical preserves
the clean merge posture once #3175 lands.
Copilot AI review requested due to automatic review settings July 30, 2026 04:48

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot wasn't able to review this pull request because it exceeds the maximum number of lines (20,000). Try reducing the number of changed lines and requesting a review from Copilot again.

…ning

# Conflicts:
#	src/transforms/import-rewriter/ssr-adapter.test.ts
#	src/transforms/import-rewriter/ssr-adapter.ts
Copilot AI review requested due to automatic review settings July 30, 2026 05:08

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot wasn't able to review this pull request because it exceeds the maximum number of lines (20,000). Try reducing the number of changed lines and requesting a review from Copilot again.

@kojiwakayama
kojiwakayama enabled auto-merge July 30, 2026 05:09
@kojiwakayama
kojiwakayama added this pull request to the merge queue Jul 30, 2026
Merged via the queue into main with commit 0e2f403 Jul 30, 2026
30 checks passed
@kojiwakayama
kojiwakayama deleted the phase0/dependency-pinning branch July 30, 2026 05:23
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.

3 participants