The third appearance of one Foundation trap: a URLComponents setter that re-encodes its whole input. T-875 fixed it for paths, T-1624 for absolute URLs, and this commit fixes it for root-relative ones. The fix is correct, minimal, and tested — but all three review agents independently found the same bug still standing one branch below, on the shape markdown authors write far more often.
URLComponents.query and .fragment are non-encoded setters — they treat whatever you hand them as fully raw text and percent-encode all of it. Give one a value that is already escaped and the escape itself gets escaped: token=a%20b becomes token=a%2520b. The image never loads; the link opens the wrong page.URLComponentHelpers.encodingRawCharacters (keeps existing %XX triplets verbatim, encodes only genuinely-raw characters) and assign via the percentEncoded* setters, which take the value as-is. Two call sites in ImagePathResolver, one in LinkPathResolver.%, %zz, %2, %C0%80, NUL, emoji, empty string — all produce a valid URL, none crash the percentEncoded* setter. The helper is closed under valid percent-encoding, so the setter can never receive something it would reject. Empty query/fragment is byte-identical to the old behaviour.LinkPathResolver clears the fragment because it carries it separately in the enum for in-app anchor scrolling; ImagePathResolver keeps it inline because the URL is the whole result. Both match how their results are consumed..url branches hand the raw query and fragment to reassembleURL and then URL(string:relativeTo:), which applies the same wholesale re-encode. ImagePathResolver.swift:329,338 and LinkPathResolver.swift:198,209. Not ticketed.Ready to push — file the follow-up first
Nothing in this diff is wrong. The fix is correct, provably crash-free, minimal, and matches the pattern its two predecessors established. 182 targeted tests pass, SwiftLint is clean, and both platform builds succeed with no new warnings. As a fix for T-1663 as ticketed, it is complete.
The reservation is about what it leaves behind. Three review agents, working independently, converged on the same finding: the identical double-escape survives in the plain-relative branch of both resolvers — four call sites, roughly twenty lines below the ones being fixed, in the same function. Empirically reproduced on this toolchain: images/logo.png?token=a%20b c still resolves to ?token=a%2520b%20c. Relative paths are more common in real markdown than root-relative ones, so the larger half of the bug ships unfixed while the smaller half is closed.
That is a scope decision, not a defect in the diff, so it is the author's call rather than the reviewer's — but it should be made deliberately before merge, because this is exactly how T-1663 came to exist: T-1624 fixed absolute URLs and left root-relative ones, with the residual recorded only in an agent-note. Either extend this PR by four call sites, or file the ticket now. The Transit MCP server was unreachable during this review, so the ticket could not be filed automatically; the residual has been written into docs/agent-notes/link-handling.md instead.
290918a Fix T-1663: Root-relative query and fragment double-escape working-tree Fixes applied in this review Web addresses cannot contain spaces, so a space is written as %20. That is called percent-encoding, and % is its escape character — which means % itself has to be escaped too, as %25.
Prism has to build addresses out of pieces. Apple's toolkit offers two ways to set the ?query part of an address: one that says "this text is raw, please escape it for me" and one that says "this text is already escaped, use it as-is." Prism was calling the first one with text that was already escaped. So the toolkit dutifully escaped it again — and the % in the existing %20 turned into %25, leaving %2520.
The address came out subtly wrong. An image with that address failed to load; a link with it opened the wrong page.
This only bites when the address mixes the two styles — part already escaped, part not — which is exactly what happens when a person hand-writes a link in a markdown file. And it only bites on documents opened from a URL, where addresses beginning with / are resolved against the site the document came from.
% plus two hex digits. Like doubling a quote mark inside a quoted string: the escape character needs escaping too./, meaning "from the top of this website," as opposed to "next to the current page."Two production hunks, both in the .url arm of the root-relative (leading /) branch of resolveRelativePath:
prism/Services/ImagePathResolver.swift:294-299 — query and fragmentprism/Services/LinkPathResolver.swift:163-165 — query only (the fragment is cleared here and carried in the enum)Plus seven regression tests across two suites.
URLComponents exposes each component twice: query (raw in, encoded out) and percentEncodedQuery (encoded in, encoded out). The old code used the first with input that was already partly encoded, so the setter's wholesale re-encode doubled every existing escape.
The fix uses URLComponentHelpers.encodingRawCharacters(in:allowedCharacters:), introduced by T-1624. It walks the component and, for each position, either recognises a valid %XX triplet and copies it through verbatim, or accumulates the character into a pending run that gets addingPercentEncoding'd when the run ends. The result goes to the percentEncoded* setter, which does not touch it.
The critical detail is why query and fragment need this helper rather than the treatment the path gets. The path, one line above, is normalised by decode-then-encode — a pair that is idempotent for raw, encoded, and mixed input alike. Applying that to a query would be actively wrong: decoding turns an escaped %26 into a literal &, and re-encoding cannot know it was ever escaped, so an escaped literal silently becomes a parameter separator. The selective encoder exists precisely to avoid that round-trip.
Reusing the T-1624 helper over a local solution. This is the third site to need it, which is the strongest argument for it being the right abstraction. The cost is that the two-line Optional.map + CharacterSet pairing now appears at four sites — see the reuse finding below.
Fixing root-relative only. The ticket scoped it that way, and the diff honours the scope faithfully. The counter-argument is that the same function contains the same bug twenty lines further down.
The underlying behaviour is Foundation's per-component validity fallback. Its URL parser and the URLComponents non-encoded setters both treat a component as an atom: if any character in it is disallowed-and-raw, the component is deemed wholly unencoded and re-encoded in full, which promotes every pre-existing % to %25. A fully-valid component passes through untouched. That is why the bug is invisible for uniformly-raw and uniformly-encoded input and only surfaces on mixed input — and why it has now been found three separate times (T-875 path, T-1624 absolute, T-1663 root-relative) rather than once.
Crash safety. The percentEncoded* setters reject syntactically invalid input in some Foundation versions, so accepting markdown-controlled text into them deserves scrutiny. It is safe by construction: encodingRawCharacters emits only (a) verbatim %XX triplets it validated against ASCII hex — the explicit isASCII guard matters, since isHexDigit alone accepts fullwidth forms — and (b) the output of addingPercentEncoding, which by definition contains only allowed characters and well-formed triplets. The image is closed under valid percent-encoding. Empirically confirmed non-crashing and correct for %, %zz, %2, %GG, a%, %C0%80 (overlong UTF-8, preserved — valid syntax is all the setter checks), NUL, U+007F, BOM, and astral-plane emoji.
Empty-string equivalence. splitURLComponents returns a non-nil empty string for a source ending in ? or #. encodingRawCharacters("") returns "", and percentEncodedQuery = "" yields a trailing ? — byte-identical to the old query = "". The .map preserves nil-vs-empty, so no shape changes behaviour except the mixed one being fixed.
Non-structural: three call sites now use a helper that already existed for exactly this purpose. Its cost is a single O(n) pass over a short component, behind an Optional.map, so a source with no ? or # — the overwhelming majority — pays nothing. Neither resolver sits in a hot path: ImagePathResolver.resolve runs once per image subresource request from WebKit and once per detail-window open; LinkPathResolver.resolve runs once per link activation.
The more interesting architectural fact is what this commit reveals about the class. There is no single chokepoint through which markdown-supplied URL text must pass; instead there are six or seven independent assembly sites, each of which has to remember the rule. Three have now been fixed one at a time, each fix leaving the others standing and each residual recorded only in an agent-note. A structural fix would be a single "resolve a markdown-supplied reference against a base" entry point that owns the encoding rules once. That is a refactor, not a bugfix, and is out of scope here — but it is the reason the residual below matters more than its line count suggests.
ImagePathResolver.swift:329,338 and LinkPathResolver.swift:198,209 reassemble with the raw query/fragment and hand the string to URL(string:relativeTo:), which applies the same per-component fallback. Reproduced: images/logo.png?token=a%20b c → ?token=a%2520b%20c. Note the path in that same string comes out correct, so the failure is now confined to query and fragment — the exact T-1663 signature. Existing coverage misses it because the T-875 relative tests use ?ref=main, which has no escape to double.ResolvedLink.fragment has no encoding contract. The absolute branch yields a decoded fragment (url.fragment), .anchorOnly decodes explicitly, and the root-relative branch passes the source text through verbatim — so the new test correctly pins fragment: "frag%20ment" as encoded. Harmless today because the sole consumer, DocumentSession.scrollToAnchor, decodes before slugging. Worth a doc comment on the enum case rather than a code change./a%2Fb.png → /a/b.png silently changes path structure. Pre-existing, out of scope, but the in-code comment's blanket idempotence claim overstates it.ImagePathResolver.swift
Why it matters. This is the bug. Both components were being assigned through non-encoded setters that re-encode wholesale, so any already-escaped value doubled its escapes and the image silently failed to load.
What to look at. prism/Services/ImagePathResolver.swift:294-299
LinkPathResolver.swift
Why it matters. The apparent asymmetry with the image resolver — one clears the fragment, one keeps it — looks like an oversight and is not. Getting this wrong in either direction would duplicate the fragment or lose in-app anchor scrolling.
What to look at. prism/Services/LinkPathResolver.swift:163-167
ImagePathResolverTests.swift
Why it matters. The mixed cases prove the fix; the fully-encoded case is the one that catches an over-correction, which is the realistic way a fix like this regresses.
What to look at. prismTests/ImagePathResolverTests.swift:1099-1160, prismTests/LinkPathResolverRootRelativeQueryFragmentTests.swift:1-61
ImagePathResolver.swift
Why it matters. Found independently by all three review agents and reproduced empirically. Relative paths are more common in markdown than root-relative ones, so the unfixed half is the larger half — and it now sits in the same function as the fixed half, behaving differently.
What to look at. prism/Services/ImagePathResolver.swift:329,338 and prism/Services/LinkPathResolver.swift:198,209
The path one line above is normalised by removingPercentEncoding then addingPercentEncoding — a pair that is idempotent across raw, encoded, and mixed input. Query and fragment deliberately do not get that treatment: decoding a query turns an escaped %26 into a literal &, and re-encoding cannot know it was ever escaped, so an escaped literal would silently become a parameter separator. encodingRawCharacters instead preserves existing %XX triplets verbatim and encodes only genuinely-raw characters.
Calling normalizedAbsoluteURL from the root-relative branch would mean synthesising origin + source and abandoning the explicit per-component assignment that T-620 introduced to stop base-URL query/fragment leaking through. The narrower reuse — just the encoder — keeps the T-620 guarantee intact.
The commit message, both code comments, and the test names all say "root-relative", and the diff does exactly that and no more. The reviewer's position is that this is defensible as a scoping decision but should not be silent: the residual is now recorded in docs/agent-notes/link-handling.md, and a ticket should be filed before merge. See the verdict.
The image half was appended to the existing ImagePathResolverTests.swift; the link half became a new file. No rationale is recorded for the split, and the sibling T-1624 suite lives inside LinkPathResolverTests.swift. It compiles either way — prismTests is a PBXFileSystemSynchronizedRootGroup, so no pbxproj edit is needed.
| Severity | Area | Finding | Resolution |
|---|---|---|---|
| major | ImagePathResolver.swift:329,338 / LinkPathResolver.swift:198,209 | The identical double-escape survives in the plain-relative .url branches of both resolvers. They pass the raw query (and, in ImagePathResolver, the raw fragment) to reassembleURL and then URL(string:relativeTo:), which applies Foundation's per-component wholesale re-encode. Reproduced on this toolchain: images/logo.png?token=a%20b c resolves to ?token=a%2520b%20c. Relative paths are more common in markdown than root-relative ones, so this is the larger half of the bug. Found independently by all three review agents. | Not fixed here — extending production behaviour past the ticket's scope is the author's decision, not the reviewer's. Recorded as an explicit residual in docs/agent-notes/link-handling.md, including the reproduction, why the existing T-875 tests miss it, and the constraint that the fix must be selective encoding rather than the path's decode-then-encode. The Transit MCP server was unreachable throughout this review, so the follow-up ticket could not be filed automatically and must be raised by hand before merge. |
| major | docs/agent-notes/link-handling.md:51 | The agent-note ended with 'never feed an already-encoded query to it — the root-relative branches currently do (latent, narrower bug)'. That sentence *is* T-1663. Once this commit lands the note actively misinforms the next session, telling it a bug exists where it no longer does. | Rewrote the clause, added a bullet recording that T-1663 closed the root-relative instance, and added a second bullet documenting the still-open relative-branch residual with its reproduction. A one-word deletion would have been wrong here — the note needed to gain the new residual as it lost the old one. |
| minor | CHANGELOG.md | No entry, breaking an unbroken convention: every recent bugfix on main (T-1822, T-1669, T-1812, T-1840) adds a '### Fixed' line under [Unreleased], and the sibling T-1624 has one. This is user-visible behaviour, which is precisely what that section documents. | Added an entry in house style at the top of the Fixed section — concrete symptom, the address that broke, why it broke, and an explicit note that the document-relative form of the same shape is still affected and tracked separately. |
| minor | URLComponentHelpers.swift:110-115 and the three new call sites | The two-line binding of component kind to CharacterSet now appears at four sites. An Optional-taking pair (encodingRawQuery / encodingRawFragment) would make it impossible for a call site to pass .urlQueryAllowed for a fragment — the one mistake this shape invites. | Skipped. The algorithm is already shared; only the pairing repeats. Adding wrappers is an eight-line additive change with five call-site edits, which is a reasonable cleanup but not one to fold into a bugfix diff that a review round has already approved. |
| minor | prismTests/LinkPathResolverRootRelativeQueryFragmentTests.swift | Standalone new file, while the image half of the same commit was appended in-file and the sibling T-1624 suite lives at LinkPathResolverTests.swift:1005 next to an existing T-875 root-relative case. Splitting a matched pair of regression suites across two conventions makes the link half harder to find. | Skipped. Organisational only — it compiles and runs (prismTests is a filesystem-synchronized group, so no pbxproj edit is needed), and moving passing tests for tidiness is churn the pre-push constraints discourage. |
| minor | prismTests/LinkPathResolverRootRelativeQueryFragmentTests.swift:34 | The expected fragment payload 'frag%20ment' is percent-encoded, whereas the absolute branch yields a decoded fragment and .anchorOnly decodes explicitly. ResolvedLink's fragment payload therefore has no defined encoding contract. | Skipped as a code change — the test pins current behaviour correctly and the only consumer, DocumentSession.scrollToAnchor, decodes before slugging, so nothing is broken. Worth a doc comment on the enum case in some future pass. |
| nit | ImagePathResolver.swift:288 / LinkPathResolver.swift:158 | The comment claims the path's decode-then-encode pair is idempotent for raw, encoded, and mixed inputs. Not true for encoded reserved delimiters: %2F decodes to a live separator, so /a%2Fb.png becomes /a/b.png. | Skipped. Pre-existing, untouched by this diff, and the overstatement is harmless in the shapes the resolvers actually meet. |
| nit | specs/bugfixes/link-path-resolver-absolute-query/report.md, specs/bugfixes/imagepathresolver-root-relative-leakage/report.md | Both older reports recommend or describe URLComponents(string:) for parsing markdown-supplied URLs. The code now deliberately does the opposite via splitURLComponents, because that initialiser is one of the things that doubles escapes on mixed input (T-875). | Skipped. Historical bugfix reports record what was true when written; rewriting them is out of scope. The current rule is stated correctly in the agent-note, which is what a future session reads first. |
Click to expand.
diff --git a/prism/Services/ImagePathResolver.swift b/prism/Services/ImagePathResolver.swiftindex cc893c2..6bebabb 100644--- a/prism/Services/ImagePathResolver.swift+++ b/prism/Services/ImagePathResolver.swift@@ -286,8 +286,17 @@ enum ImagePathResolver { // re-encode it. Setting .path with already-encoded text would // double the existing escapes (T-875). components.path = pathPart.removingPercentEncoding ?? pathPart- components.query = queryPart- components.fragment = fragmentPart+ // The non-encoded `.query`/`.fragment` setters treat their input+ // as fully raw and re-encode it wholesale, doubling any existing+ // escape (`token=a%20b` → `token=a%2520b`). Selectively encode+ // only genuinely-raw characters instead (T-1663, follow-up to+ // T-1624, which fixed the same class of bug for absolute URLs).+ components.percentEncodedQuery = queryPart.map {+ URLComponentHelpers.encodingRawCharacters(in: $0, allowedCharacters: .urlQueryAllowed)+ }+ components.percentEncodedFragment = fragmentPart.map {+ URLComponentHelpers.encodingRawCharacters(in: $0, allowedCharacters: .urlFragmentAllowed)+ } guard let resolved = components.url else { return .failed(.invalidURL) }diff --git a/prism/Services/LinkPathResolver.swift b/prism/Services/LinkPathResolver.swiftindex f43fdb5..79033ef 100644--- a/prism/Services/LinkPathResolver.swift+++ b/prism/Services/LinkPathResolver.swift@@ -155,7 +155,14 @@ enum LinkPathResolver { // re-encode it. Setting .path with already-encoded text would // double the existing escapes (T-875). components.path = pathPart.removingPercentEncoding ?? pathPart- components.query = queryPart+ // The non-encoded `.query` setter treats its input as fully raw+ // and re-encodes it wholesale, doubling any existing escape+ // (`token=a%20b` → `token=a%2520b`). Selectively encode only+ // genuinely-raw characters instead, mirroring embeddingFragment+ // below (T-1663, follow-up to T-1624).+ components.percentEncodedQuery = queryPart.map {+ URLComponentHelpers.encodingRawCharacters(in: $0, allowedCharacters: .urlQueryAllowed)+ } // Clear fragment from URL — it's carried separately in the enum. components.fragment = nil guard let resolved = components.url else {diff --git a/prismTests/ImagePathResolverTests.swift b/prismTests/ImagePathResolverTests.swiftindex 5a21835..20e5836 100644--- a/prismTests/ImagePathResolverTests.swift+++ b/prismTests/ImagePathResolverTests.swift@@ -1095,3 +1095,66 @@ struct ImagePathResolverMixedAbsoluteURLTests { )) } }++// MARK: - T-1663: Mixed Encoded And Raw Root-Relative Query/Fragment++/// Bug T-1663 (follow-up to T-1624, PR #309): the root-relative (leading+/// `/`) `.url`-source branch assigned query and fragment straight to the+/// non-encoded `URLComponents.query`/`.fragment` setters, which treat their+/// input as fully raw and re-encode it wholesale — doubling an existing+/// `%20` to `%2520`, the same failure class T-1624 fixed for absolute URLs.+@Suite("ImagePathResolver mixed encoded root-relative query/fragment")+struct ImagePathResolverRootRelativeQueryFragmentTests {++ @Test("Mixed encoded/raw query on a root-relative remote source normalizes encoding (T-1663)")+ func mixedEncodedQueryRootRelativeRemote() {+ let baseURL = URL(string: "https://example.com/docs/page.md")!+ let result = ImagePathResolver.resolve(+ source: "/images/logo.png?token=a%20b c",+ baseURL: baseURL,+ sourceType: .url+ )+ #expect(result == .remote(+ URL(string: "https://example.com/images/logo.png?token=a%20b%20c")!+ ))+ }++ @Test("Mixed encoded/raw fragment on a root-relative remote source normalizes encoding (T-1663)")+ func mixedEncodedFragmentRootRelativeRemote() {+ let baseURL = URL(string: "https://example.com/docs/page.md")!+ let result = ImagePathResolver.resolve(+ source: "/images/logo.svg#sec%20one two",+ baseURL: baseURL,+ sourceType: .url+ )+ #expect(result == .remote(+ URL(string: "https://example.com/images/logo.svg#sec%20one%20two")!+ ))+ }++ @Test("Mixed encoded/raw query and fragment on a root-relative remote source normalize encoding (T-1663)")+ func mixedEncodedQueryAndFragmentRootRelativeRemote() {+ let baseURL = URL(string: "https://example.com/docs/page.md")!+ let result = ImagePathResolver.resolve(+ source: "/images/logo.png?token=a%20b c#frag%20ment two",+ baseURL: baseURL,+ sourceType: .url+ )+ #expect(result == .remote(+ URL(string: "https://example.com/images/logo.png?token=a%20b%20c#frag%20ment%20two")!+ ))+ }++ @Test("Fully encoded root-relative remote source is unchanged (T-1663)")+ func fullyEncodedRootRelativeRemoteUnchanged() {+ let baseURL = URL(string: "https://example.com/docs/page.md")!+ let result = ImagePathResolver.resolve(+ source: "/images/logo.png?token=a%20b#sec%20one",+ baseURL: baseURL,+ sourceType: .url+ )+ #expect(result == .remote(+ URL(string: "https://example.com/images/logo.png?token=a%20b#sec%20one")!+ ))+ }+}diff --git a/prismTests/LinkPathResolverRootRelativeQueryFragmentTests.swift b/prismTests/LinkPathResolverRootRelativeQueryFragmentTests.swiftnew file mode 100644index 0000000..30d97ec--- /dev/null+++ b/prismTests/LinkPathResolverRootRelativeQueryFragmentTests.swift@@ -0,0 +1,61 @@+//+// LinkPathResolverRootRelativeQueryFragmentTests.swift+// prismTests+//+// Created by Claude on 15/8/2026.+//++import Foundation+import Testing+@testable import prism++// MARK: - T-1663: Mixed Encoded And Raw Root-Relative Query/Fragment++/// Bug T-1663 (follow-up to T-1624, PR #309): the root-relative (leading+/// `/`) `.url`-source branch assigned the query straight to the non-encoded+/// `URLComponents.query` setter, which treats its input as fully raw and+/// re-encodes it wholesale — doubling an existing `%20` to `%2520`, the same+/// failure class T-1624 fixed for absolute URLs.+@Suite("LinkPathResolver root-relative query/fragment")+struct LinkPathResolverRootRelativeQueryFragmentTests {++ @Test("Mixed encoded/raw query on a root-relative markdown destination normalizes encoding (T-1663)")+ func mixedEncodedQueryRootRelativeMarkdown() {+ let baseURL = URL(string: "https://example.com/docs/page.md")!+ let result = LinkPathResolver.resolve(+ destination: "/docs/file.md?token=a%20b c#frag%20ment",+ baseURL: baseURL,+ sourceType: .url+ )+ #expect(result == .remoteMarkdown(+ URL(string: "https://example.com/docs/file.md?token=a%20b%20c")!,+ fragment: "frag%20ment"+ ))+ }++ @Test("Mixed encoded/raw query and fragment on a root-relative non-markdown destination normalize encoding (T-1663)")+ func mixedEncodedQueryAndFragmentRootRelativeExternal() {+ let baseURL = URL(string: "https://example.com/docs/page.md")!+ let result = LinkPathResolver.resolve(+ destination: "/docs/page.html?token=a%20b c#sec%20one two",+ baseURL: baseURL,+ sourceType: .url+ )+ #expect(result == .externalURL(+ URL(string: "https://example.com/docs/page.html?token=a%20b%20c#sec%20one%20two")!+ ))+ }++ @Test("Fully encoded root-relative markdown destination is unchanged (T-1663)")+ func fullyEncodedRootRelativeMarkdownUnchanged() {+ let baseURL = URL(string: "https://example.com/docs/page.md")!+ let result = LinkPathResolver.resolve(+ destination: "/docs/file.md?token=a%20b",+ baseURL: baseURL,+ sourceType: .url+ )+ #expect(result == .remoteMarkdown(+ URL(string: "https://example.com/docs/file.md?token=a%20b")!+ ))+ }+}The Transit MCP server was unreachable for the whole review, so this could not be done automatically. The relative-branch double-escape is documented in docs/agent-notes/link-handling.md but has no ticket. Given that T-1663 exists because T-1624's residual lived only in an agent-note, this is the one action item that should not be deferred.
The follow-up fix is four call sites using a helper that already exists — mechanically about the size of this diff. Extending here means one internally consistent function and no third round of this bug; splitting means keeping a reviewed, approved, minimal diff intact. Both are defensible. What is not defensible is shipping without choosing.
CHANGELOG.md and docs/agent-notes/link-handling.md were modified during this review and left uncommitted for your inspection. Neither touches Swift source, so the test and lint results above still hold.
GitHub Actions is billing-blocked, so everything was validated locally. 182 tests across ImagePathResolverTests, ImagePathResolverRootRelativeQueryFragmentTests, ImagePathResolverMixedAbsoluteURLTests, LinkPathResolverTests, LinkPathResolverCredentialTests, LinkPathResolverMixedAbsoluteURLTests, LinkPathResolverRootRelativeQueryFragmentTests and URLComponentHelpersNormalizedAbsoluteURLTests, in two runs of 79 and 103 — both confirmed through Tools/check-test-results.sh against the result bundle, zero failed, zero skipped, and non-zero test counts. SwiftLint: 0 violations across 533 files. iOS Simulator and macOS builds both succeed; the 101 build warnings are pre-existing and none are in the changed files.