Store a scoped cross-realm link as sent, not as resolved - #6067
Open
backspace wants to merge 5 commits into
Open
Store a scoped cross-realm link as sent, not as resolved#6067backspace wants to merge 5 commits into
backspace wants to merge 5 commits into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 1d5b71769a
ℹ️ 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".
Contributor
Host Test Results 1 files ± 0 1 suites ±0 2h 42m 31s ⏱️ + 7m 35s Results for commit 0a9bea7. ± Comparison against earlier commit 0b6c093. Realm Server Test Results 1 files ± 0 215 suites +11 1h 19m 51s ⏱️ + 5m 54s Results for commit 0a9bea7. ± Comparison against earlier commit 0b6c093. |
The write path resolved every `links.self` through the virtual network and relativized it against the writing realm. A link into another realm cannot be relativized, so it was stored as that realm's resolved URL — a localhost host on a dev stack, the staging host on staging — even when the client sent the canonical `@scope/name/...` form. Files authored through the UI therefore carried environment-specific URLs into realms that are version controlled. The read path already had the rule: `relativizeResource` preserves a scoped reference verbatim, because such a reference is absolute and cross-realm by construction whether or not this network knows its prefix. `isScopedReference` moves to url.ts and both paths import it. Serving a document and storing one have to agree on which references are already canonical, and the two having separate notions of that is the bug rather than an incidental duplication — a copy would be free to drift again. Deliberately not included: folding an already-resolved URL back to its prefix on write. That would rewrite links in files the request never meant to touch, which is a migration decision rather than a side effect of fixing this. Tests cover all three cases the stored form distinguishes: a scoped reference kept verbatim, a same-realm link still relativized, and a link to a realm with no prefix mapping still absolute.
The case cannot be expressed against this harness. A non-scoped link is resolved and then fetched to validate it, so pointing one at a realm nothing serves returns 500 from the write itself — `unexpected exception in realm TypeError: fetch failed` — rather than exercising what gets stored. The harness serves a single realm, so there is no second reachable one to link to. Nothing about that behaviour is at risk here: the guard added for scoped references leaves the non-scoped branch untouched, so a mapped or unmapped URL runs exactly the code it ran before. The two remaining tests cover what this change decides — a scoped reference kept verbatim and a same-realm link still relativized — and both pass. That the scoped case passes where the unmapped one 500s is itself the mechanism showing through: a scoped reference is never resolved, so it is never fetched.
Skipping relativization for every scoped reference was too broad. The host canonicalizes link ids to a realm alias wherever the realm has a prefix mapping, so a link to a card in the *writing* realm also arrives scoped, and storing those verbatim changed the form of same-realm links that had always been stored relative. Four skills specs caught it: attached skills counted zero, because the stored link no longer matched what the reader expected. `maybeRelativeReference` already draws the distinction. It relativizes what it can and otherwise returns the reference in the form it was given, and its fallback comment says why — a prefix-form RRI is already canonical and portable. The write path defeated that by resolving to a URL first, so the fallback had nothing but a URL left to return. Passing a scoped reference through unresolved restores both cases at once: it relativizes into the writing realm as before, and a cross-realm link keeps the alias the client sent.
Whether a link can be relativized is decided in URL space. `relativeTo` here is always a URL, and `relativeReference` refuses a mixed RRI/URL pair rather than resolve across forms — `sharedNamespace` returns undefined and the fallback hands back the reference unchanged. Passing a scoped reference straight in therefore looked like "cannot be relativized" even for a link into the writing realm, so a same-realm link the host had canonicalized to a prefix was stored as `@scope/name/x` where it had always been stored `./x`. Resolving first asks the question properly: a link inside the realm is stored relative whichever form it arrived in, and outside it a scoped reference is stored exactly as sent while anything else is stored resolved. The decision moves into `storedRelationshipLink`, exported so it can be tested without a Definition and a DefinitionLookup. The same-realm scoped case cannot be reached through the realm-server write suite — only `@cardstack/base/` is prefix-mapped there and it is not the writable realm — so it is covered by unit tests over a stub network, alongside the in-realm URL, cross-realm scoped, cross-realm URL, and unresolvable cases. Note for the record: four skills specs failed on the first dispatched run of this branch and passed on the second, and the revision in between changed nothing for the same-realm scoped case — both left such links unchanged. That attribution was wrong; those failures were flakes, and the defect they were blamed on survived until now.
Deciding realm membership by resolving first is wrong for a scoped reference this process cannot resolve. `resolveURL` treats an unregistered prefix as a relative reference and joins it against the base, so `@cardstack/catalog/Pet/vangogh` came back as a URL inside the writing realm and relativized to `./@cardstack/catalog/Pet/vangogh` — a path into a realm that has no such file. Such a reference is absolute and cross-realm by construction, so it now short-circuits and is stored exactly as sent. Resolution is reserved for references that can actually be resolved: a registered prefix, a URL, or a relative path. The unit tests missed this because the stub network registered every prefix it was asked about, while the realm-server harness registers only `@cardstack/base/` — the stub was more permissive than the thing it stood in for. A case now covers the unregistered scoped reference the write suite caught.
backspace
force-pushed
the
cs-12745-realm-file-serializer-rewrites-cross-realm-alias-links-to
branch
from
September 11, 2026 16:51
0b6c093 to
0a9bea7
Compare
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
1 participant
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.
Writing a card ran every
links.selfthrough the virtual network and relativized the result against the writing realm. A link into another realm cannot be relativized, so it was stored as that realm's resolved URL — a localhost host on a dev stack, the staging host on staging — even when the client sent the canonical@scope/name/...form. Files authored through the UI carried environment-specific URLs into realms that are version controlled.The fix is to stop discarding the form
maybeRelativeReferencealready draws the distinction. It relativizes what it can, and otherwise returns the reference in the form it was given — its fallback comment says why: a prefix-form RRI is already canonical and portable.The write path defeated that by resolving to a URL first, so the fallback had nothing but a URL left to return:
Passing a scoped reference through unresolved restores both cases at once — relativized when it points into the writing realm, alias preserved when it does not.
An earlier revision skipped relativization for scoped references outright. That was too broad, and four
skills.spectests caught it: the host canonicalizes link ids to a realm alias wherever a prefix mapping exists, so links to cards in the writing realm also arrive scoped, and storing those verbatim changed the form of same-realm links that had always been stored relative. Attached skills then counted zero, because the stored link no longer matched what the reader looked for.One definition of "scoped", shared
isScopedReferencemoves tourl.tsand both paths import it. Serving a document and storing one have to agree on which references are already canonical; the two holding separate notions of that is the bug rather than an incidental duplication, so a copy would be free to drift again. The read path's rule is unchanged —relativizeResourcehas always preserved scoped references verbatim.Scope
Not included: folding an already-resolved URL back to its prefix on write, which the issue lists as optional. That rewrites links in files the request never meant to touch, so it is a migration decision rather than a side effect of a bug fix.
Tests
Two, in the
card PATCH request/ public writable realm module: a scoped cross-realm link stored verbatim, and a same-realm link still stored relative. Both are what this change decides.The issue's third case — a link to a realm with no prefix mapping staying absolute — is not expressible against this harness. A non-scoped link is resolved and then fetched to validate it, so pointing one at a realm nothing serves returns 500 from the write itself rather than exercising what gets stored, and the harness serves a single realm. Nothing about that case is at risk here: the non-scoped branch runs exactly the code it ran before.