Skip to content

test(cross-version): load releases that import a package main dropped - #25731

Merged
brennanb2025 merged 3 commits into
mainfrom
brennanb2025/xver-harness-release-deps
Oct 6, 2026
Merged

brennanb2025 merged 3 commits into
mainfrom
brennanb2025/xver-harness-release-deps

Conversation

@brennanb2025

@brennanb2025 brennanb2025 commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor
Files Added Deleted Net
Test 5 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​345 $\color{#cf222e}{\Huge{\mathbf{−}}}$​11 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​334
Prod 0 0 0 0

ELI5

Every PR's "cross-version wire compatibility" CI job has been red since 22:53 UTC on 2026-10-05, and no assertion is failing. The harness simply can't load the newest release's code anymore.

The harness checks that today's code still talks to the last shipped release. It does that by running the release's source files against today's installed packages. The newest release, v1.4.221, uses a JSON parsing package (@streamparser/json) that main removed in #25202. So three test files die at import with Cannot find package '@streamparser/json' (for example, run 37364132538, attempt 3, on #25624).

This PR lets the harness load that release code again. Using the removed package still fails, with a clear message.

What Changed

What changes for users: nothing. This only touches the test harness, and it unblocks CI on every open PR.

Why the release reaches that package: the harness imports the release's RPC dispatcher, and that file statically imports the release's whole method table. That pulls in about 3,800 release files, including the session scanner in ai-vault/session-document-stream.ts, which imports @streamparser/json. The three failing files are cross-version-agent-session-send, cross-version-agent-session-wire and cross-version-terminal-wire.

New mechanism: while extracting a release, the harness already reads every source file to rewrite renderer aliases. In the same pass it now records each package the release imports at runtime and the names it imports. Type-only imports, comment text, and scripts embedded in strings are skipped.

A package gets a stand-in only if all of these hold: the release imports it at runtime, the current package.json does not declare it, and it does not resolve from the checkout (Node's lookup, walking up node_modules from the checkout, which sits inside the repository).

The stand-in goes into the extracted checkout's own node_modules and exports the same names, so the release loads. Any real use (calling, constructing, or reading a property) throws: release <commit> imports '<package>', which the current tree does not install, so this release code path cannot run here.

A package still reachable from the checkout keeps the real install, even when the current tree no longer declares it. A package the current tree declares but cannot resolve is a broken install, and it still fails loudly at import.

The bound: if a wire suite actually uses a stand-in package, it fails with that package named. So a real wire dependency can't be silently hidden; only release code the suites never run gets past the missing package.

On the real trees this gives exactly @streamparser/json for v1.4.221 and v1.4.211, and psl plus @electron-toolkit/preload for v1.4.190.

CHECKOUT_FORMAT goes from 4 to 5, so cached release trees are rebuilt with the stand-ins.

Why

Cutting the import edge isn't possible: it lives inside the frozen release source (rpc/dispatcher.ts → methods/index.ts), not in harness code. The agent-session suites also need the release's real method table on purpose.

Pinning an older release tag would silently stop testing against what users actually run, and it breaks again on the next dropped package.

Re-adding @streamparser/json to main would undo #25202 just for a test harness.

Installing the release's exact dependencies at test time would need network access and a package install inside the test run, which is slow and flaky in CI.

Skipping files that can't load would drop the terminal and agent-session wire coverage, which matters most here.

The stand-in is narrow: it applies only to packages that cannot load, and it refuses at the point of use with the reason. None of today's wire suites run the release's JSON stream parsing, so no suite reaches a stand-in.

Known trade-off: if release code catches a stand-in's error and quietly falls back, a test could exercise that fallback instead of failing. The stand-in error names the package, so a log search finds it.

Temporary: the stand-in is a stopgap. The structural follow-up gives each release its own dependencies: one pnpm install --frozen-lockfile --ignore-scripts per distinct release lockfile into a cache next to the checkouts, with the checkouts linked to it and a warm-up step for cold installs. That removes the stand-ins, and it also stops old release code from silently running on newer package versions, which this PR does not address. No ticket yet.

Linked Issue

None. This is CI breakage on main, found from run 37364132538.

Visual Proof

N/A: test-harness-only change, nothing visible to users.

Testing

New release-missing-packages.unit.test.ts uses a synthetic release. Release code importing default, named and subpath exports of an uninstalled package loads, and each use throws with the package name. A package that resolves from the checkout keeps the real module. A package the current tree declares but cannot resolve still fails the import. Type-only imports, comment prose and an ObjC.import('AppKit') string get no stand-in.

New case in release-checkout.unit.test.ts uses the real v1.4.221 tree in the default in-repository cache. Its parseRipgrepMatchJson runs normally on plain JSON, and its parseDenseRipgrepMatchJson (the @streamparser/json path) throws the stand-in message.

Ablation: with no stand-ins written, both positive tests fail, and cross-version-terminal-wire fails with the exact CI error (Cannot find package '@streamparser/json').

Before the fix, the three CI-failing files failed locally with the same Cannot find package error. After the fix, they pass locally together with both harness test files (5 files, 46 tests).

I ran the full tests/e2e/cross-version-wire/ file set locally, the same set CI runs. Every remaining local failure was tar -xf timed out after 45000ms during a cold release extraction, or a 30 s test timeout behind it, on a machine at load average 60–80. None were import or assertion failures. The format bump makes every cached release tree cold, so CI is the authority here.

Also ran: oxlint and pnpm run check:code-quality:changed, both clean. In typecheck:e2e, the only new error from this change is the .ts-extension import, the same one release-checkout.ts already has. The extension is needed because plain-Node child processes import this chain directly.

Platforms: macOS locally; CI covers Linux.

  • I manually tested these changes locally
  • Automated tests added/updated, or explained why not below

Review

Agent skill upstream boundary

  • Not applicable, or this change follows docs/reference/agent-skill-sharing-upstream-boundary.md and copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.

Notes

Security: the stand-ins are written only inside the gitignored tests/e2e/.cross-version-checkouts/ cache. Cross-platform: paths use path.join. SSH, mobile and the shipped app are not touched.

PR author: @BrennanKB5.

Checklist

  • This PR is small and focused
  • I explained what changed and why (ELI5, the user-facing before/after, the mechanism, and why over the alternatives)
  • Before/after screenshots or videos attached for UI changes, or N/A with reason
  • Self-reviewed for correctness, security, and performance
  • Cross-platform, SSH/remote, and path/shortcut impact considered (or N/A)
  • pnpm lint, pnpm typecheck, pnpm test, and pnpm build pass (or CI will cover; local preferred)

brennanb2025 and others added 3 commits October 5, 2026 17:27
…n dropped

The newest release (v1.4.221) imports @streamparser/json, which main removed.
The harness runs release source against the current install, and the release's
RPC dispatcher imports its whole method table, so every wire suite failed at
import time. Extraction now gives each package the release declares and the
current tree no longer declares a stand-in in the checkout's own node_modules
that loads but throws, naming the package, on any use.

Co-Authored-By: Claude <noreply@anthropic.com>
…fest diff

A package gets a stand-in only when release source imports it at runtime,
the current tree does not declare it, and it does not resolve from the
checkout. A package still reachable from the checkout keeps the real install,
and one the current tree declares but cannot resolve stays a loud failure.
Type-only imports, comment prose and scripts in strings are ignored.

Co-Authored-By: Claude <noreply@anthropic.com>
@brennanb2025
brennanb2025 marked this pull request as ready for review October 6, 2026 03:22
@coderabbitai

coderabbitai Bot commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

📝 Walkthrough

Walkthrough

Release checkout preparation now collects package imports and installs stand-ins for eligible packages that are unavailable in the checkout. The change updates renderer-alias rewriting to return rewritten source text and increments the checkout format. Tests cover package import handling, stand-in behavior, and loading a historical release with an unavailable package.

Priority: ⬇️ Low

Merge Risk: 🔵 Low · up to f2f9d

This only changes the cross-version test harness. A narrow edge case yields a less clear error message but still fails, so the merge risk is low.

Security Architecture Review

Security architecture risk: 🔵 Low · up to f2f9d

The change affects compatibility testing rather than shipped application behavior. Declared dependencies remain protected from replacement, and checkout recovery is preserved. Handling of every older-release import pattern has not been established.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The inspected change affects release-code loading within compatibility-test processes and their checkout caches. It does not grant additional execution privileges: the harness already imported release modules. Effective runner credentials and sandbox containment were not established by the supplied evidence.

Trust Boundaries and Controls

  • observed — Release-derived specifiers are filtered before collection. Generated directories use the extracted package name rather than the full subpath, generated filenames are fixed, and diagnostic strings are JSON-encoded. Current manifest declarations form a no-replacement boundary; the inspected tests assert that declared-but-uninstalled packages still fail to import.

Resilience and Maintainability Implications

  • observed — Stand-in generation cannot publish a checkout before completion stamping. Publication still removes the destination and then renames staging, rather than atomically replacing it for unlocked readers. That sequence predates this PR and was not changed by the stand-in integration.

Hardening Proposals

  • proposed — Consider explicitly limiting stand-in eligibility for imports without collected bindings, allowing side-effect-only substitution only when the package is deliberately approved. This would make missing-initialization behavior auditable rather than implying a universal refusal guarantee.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 15 functions across 5 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the cross-version test change: loading releases that import a package removed from the current tree.
Description check ✅ Passed The description covers the required sections and gives detailed change rationale, testing results, visual-proof status, and compatibility notes. It says there is no linked issue; the template requires…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot 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.

🧹 Nitpick comments (1)
tests/e2e/cross-version-wire/release-missing-packages.ts (1)

17-19: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value

Detect value-level re-exports with a named clause correctly, and cover bare export * from.

STATIC_IMPORT requires from after a clause made only of [\w$*{},\s]. This matches export { x } from 'mod' and export * from 'mod'. It does not match export * as ns from 'mod', because the * branch in importedNames is skipped, but the regex itself does match. The gap is elsewhere. export * from 'mod' records the package with an empty name set, so the stand-in has no named exports. A release that re-exports names through export { x } from 'mod' works, but a consumer that imports a name through export * fails at link time with a missing-export error, not the clear stand-in message.

This is an edge case with a limited effect on the harness. Record this limitation in a comment, or add a test for it.

Source: Learnings


ℹ️ Review info
⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 91912716-0247-466e-b79b-5f57e176f430
📥 Commits

Reviewing files that changed from the base of the PR and between 9756dae and f2f9d9e.

📒 Files selected for processing (5)
  • tests/e2e/cross-version-wire/release-checkout-tree.ts
  • tests/e2e/cross-version-wire/release-checkout.ts
  • tests/e2e/cross-version-wire/release-checkout.unit.test.ts
  • tests/e2e/cross-version-wire/release-missing-packages.ts
  • tests/e2e/cross-version-wire/release-missing-packages.unit.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 4 remain after this review.

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