Fix catalog remix leaving base-realm module refs unresolvable in the install destination#5478
Fix catalog remix leaving base-realm module refs unresolvable in the install destination#5478richardhjtan wants to merge 5 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 214355e4b1
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Preview deploymentsHost Test Results 1 files ±0 1 suites ±0 3h 4m 59s ⏱️ + 6m 49s Results for commit dfe284b. ± Comparison against earlier commit b6059fb. Realm Server Test Results 1 files ±0 1 suites ±0 11m 59s ⏱️ +17s Results for commit dfe284b. ± Comparison against earlier commit b6059fb. |
richardhjtan
force-pushed
the
fix/catalog-remix-adopts-from-base-realm
branch
from
2ccc65a to
a0bf30a
Compare
There was a problem hiding this comment.
Pull request overview
This PR fixes catalog remix/install behavior where base-realm module references could be misclassified as “needs to be copied,” leading to unresolvable module refs in the install destination. It does this by recognizing base-realm module identifiers in both their literal base URL form and their canonical RRI-prefix form.
Changes:
- Add
isInBaseRealm()to treat base-realm module refs as base-realm whether they appear ashttps://cardstack.com/base/...or as an RRI like@cardstack/base/...(viaVirtualNetwork.unresolveURL()), and use it throughout install planning. - In live tests, mutate the shared
@cardstack/host/config/environmentdefault export so all app instances created bysetupApplicationTestsee the correctresolvedCatalogRealmURL.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/runtime-common/catalog.ts | Fixes base-realm detection for module/instance install planning by handling both literal base URLs and RRI-prefixed identifiers. |
| packages/host/tests/live-test.js | Ensures live-test bootstrapping updates the shared host config so subsequent test app instances resolve the catalog realm URL correctly. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b6059fb233
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // later by individual tests' setupApplicationTest — which import | ||
| // '@cardstack/host/config/environment' as a normal static import, not | ||
| // through this loader — also sees the real catalog realm address. | ||
| hostConfigEnvironment.default.resolvedCatalogRealmURL = realmURL; |
There was a problem hiding this comment.
When liveTest runs a realm test that uses the host helper/mock-matrix modules, those modules have already been imported by the Promise.all immediately above this assignment. Several of them snapshot ENV.resolvedCatalogRealmURL at module evaluation (for example mock-matrix/_client.ts builds publicRealmURLs, and helpers re-export catalogRealmURL from host/lib/utils), so setting the shared config here is too late: those helpers still behave as if the catalog realm is absent even though app instances see the new value. Import the config module and set it before importing the helpers, or make the helper reads lazy, so live catalog tests get consistent config.
Useful? React with 👍 / 👎.
| // A codeRef.module for a base-realm class may show up in either form: the | ||
| // literal symbolic `https://cardstack.com/base/` URL, or the base realm's | ||
| // real backing URL (e.g. `https://localhost:4201/base/skill`). | ||
| // VirtualNetwork.unresolveURL() can canonicalize either form to the | ||
| // `@cardstack/base/` RRI prefix, but only when the base realm's URL and | ||
| // realm mappings are actually registered on that VirtualNetwork instance — | ||
| // a bare VirtualNetwork (e.g. the plan-install unit test) leaves both | ||
| // forms unchanged, so the literal-URL check below is still needed. | ||
| // Without it, a base-realm module can get wrongly treated as something | ||
| // that needs to be copied into the install destination. |
There was a problem hiding this comment.
hmmmm, this makes me feel like we are papering over a bug. i would expect that the actual materialized base realm URL is only visible in the virtual network layer. they fact that it leaks out is probably not good. i think that might be the bug you need to solve. also this probably touches the base realm RRI work that @backspace is doing
There was a problem hiding this comment.
We have CS-12078 to follow up on this; Claude agrees it’s papering over, but will need a deeper fix
…install destination planInstanceInstall/resolveTargetCodeRef checked base-realm membership against the symbolic https://cardstack.com/base/ URL, but a resolved codeRef.module is the base realm's real backing URL, so a base-realm instance (e.g. a Skill) was wrongly treated as needing to be copied into the install destination — producing an adoptsFrom.module relative to the new install folder instead of the actual base realm module. Also restores the @cardstack/catalog/ virtual-network mapping for live test runs specifically (resolvedCatalogRealmURL is nulled for environment === 'test', which breaks module resolution for content that imports catalog-realm commands, like the Listing card class). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ealmURL param live-test.js already passes the catalog realm's real address into every live-test browser session via the realmURL query param (used to discover which realm to load test files from) — no need for a parallel rawCatalogRealmURL build-time config field duplicating the same skipCatalog logic. NetworkService now reads realmURL directly, the same way live-test.js's own config/environment shim already does. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
network.ts already reads config.resolvedCatalogRealmURL normally — the gap was that live-test.js only patched a spread copy of the config module for its own bootstrapping loader, never the shared module object that every test's own app instance (via setupApplicationTest) imports normally. Mutating that shared object directly means every later-created app instance sees the real catalog realm address with no production runtime code touched at all. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
isInBaseRealm only normalized via VirtualNetwork.unresolveURL(), which requires a registered realm mapping to recognize a base-realm module — but a module ref built without one (the existing plan-install unit test uses a bare VirtualNetwork with no mappings, and this is also how base refs appear in serialized cards/specs) stays in the literal https://cardstack.com/base/ form and was wrongly treated as needing to be copied into the install destination. Check the literal form first, matching what the RealmPaths-based check did before. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ontract unresolveURL() can canonicalize either the literal symbolic base-realm URL or its real backing URL — the deciding factor is whether the VirtualNetwork has the base realm's mappings registered, not which URL form it's given. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
richardhjtan
force-pushed
the
fix/catalog-remix-adopts-from-base-realm
branch
from
b6059fb to
dfe284b
Compare
Summary
isInBaseRealm(used byplanInstanceInstall/resolveTargetCodeRefto decide whether a module needs to be copied into the install destination) only recognized a base-realm module via its resolved real backing URL (e.g.https://localhost:4201/base/skill, viaVirtualNetwork.unresolveURL()). A module still in its literal symbolichttps://cardstack.com/base/form — which is how base refs actually appear in serialized cards/specs, and in aVirtualNetworkwith no realm mappings registered — fell through and got wrongly treated as needing to be copied, producing a brokenadoptsFrom.modulerelative to the new install folder instead of the actual base realm module. Now checks both forms.live-test.jsalready patchedresolvedCatalogRealmURLfor its own bootstrapping loader, but only on a throwaway spread copy of the config module — not the shared module object every test's own app instance imports viasetupApplicationTest. One-line fix: mutate the shared object directly. No production runtime code (network.ts) needs to change at all.Companion fix in boxel-catalog#652 addresses the actual
adoptsFromrewrite bug this uncovered; both are needed for a catalog remix/install to correctly resolve module references end to end.Test plan
pnpm lint:typesclean inpackages/runtime-commonUnit | Catalog | Install Plan Buildersuite (13/13 pass, including the base-realm-literal-URL case flagged by Codex review)