fix(annotate): scope the framed 404 to paths that name a file, so a VS Code session renders the app - #1565
Merged
Conversation
#1561's catch-all guard fired on every framed request, including the app document at `/`. The VS Code extension frames the session URL, so an annotate session opened from the editor rendered "404 Not found" instead of the app. One shared predicate (pathNamesEmbeddedDocument / isFramedEmbeddedDocumentRequest in packages/shared/html-assets.ts, vendored to Pi) now decides it on the SHAPE OF THE PATH, and the Pi mirror uses it instead of its inline copy.
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.
Fixes a confirmed high regression from #1561 (
1954a19a): an annotate session opened from VS Code shows "404 Not found" instead of the app.Repro
framedDocumentNotFound(packages/server/html-assets.ts:47, used atpackages/server/annotate.ts:1267; identical inline predicate in the Pi mirrorapps/pi-extension/server/serverAnnotate.ts:1167) applied the framed-404 document to every path whose request carriedSec-Fetch-Dest: iframe|frame|embed|object— including the app root/. The VS Code extension renders the session URL inside an<iframe>(apps/vscode-extension/src/panel-manager.ts:75) behind its cookie proxy (cookie-proxy.ts:57forwards headers verbatim), and every subcommand launched from a VS Code terminal is routed there (extension.ts:110setsPLANNOTATOR_BROWSER). The panel's one auto-reload 404s again, so the session cannot be decided from the editor.Against
origin/main(865d6791), a real annotate server:Plan and review servers are unaffected (no guard).
The rule
One shared predicate in
packages/shared/html-assets.ts(vendored to Pi; the Pi mirror now uses it instead of its inline copy):pathNamesEmbeddedDocumentis true only when the path could name a missing embedded document:/,/?x=1,///settings— one bare segment, no extension/prototype-slash.html— last segment has an extension/assets/frame— under a directory segmentA non-framed request for a missing path is untouched and still gets the app, exactly as before #1561. The asset route's own 404 for
/api/html-assets/<token>/<missing>is unchanged.Why the path shape and not
Sec-Fetch-SiteAn annotated page is a sandboxed srcdoc with an opaque origin, so its nested-document requests carry
Sec-Fetch-Site: cross-site— the same value the VS Code webview wrapper produces (andnoneon both sides for a pasted URL). Site can never separate the two. The path can: the app only ever loads at/, while a relative embed is anchored at/api/html-assets/<token>/by #1561's<base href>(which has its own 404), so only a root-relative embed reaches the catch-all at all — and that is written as a file reference. A bare single-segment word stays with the app so a future SPA route cannot 404 inside a frame.Proof
Headless Chromium (Playwright), the QA agent's
v-vscode-frame.mjsshape: a cross-origin wrapper page iframing the session URL, and separately through the extension's realcreateCookieProxyfromapps/vscode-extension/src/cookie-proxy.ts. Bun and Pi annotate servers, markdown and raw-HTML sessions:The #1561 embed fixture (a report with five embedded sibling documents — static
src, runtimedata-srcwith?step=result,<object data>,<embed src>, asub/deep.html) still renders all five in both runtimes, and a framed missing sibling (/prototype-slash.html, root-relative so it reaches the catch-all) still gets the 404 document naming the file:PLANNOTATOR_BROWSER=/usr/bin/true, headless only.Tests
packages/shared/html-assets.test.ts— the predicate:/and/?x=1are never a missing embed; file references and directory-segment paths are; a bare word is not;isFramedEmbeddedDocumentRequestneeds both halves.packages/server/annotate-html-assets.test.ts— the matrix over a real Bun annotate server: framed/and/?x=1→ 200 app HTML; framed/prototype-slash.html→ 404 document; framed/assets/frame→ 404, framed/settings→ app; plain requests for missing paths → app. The existingframedDocumentNotFoundunit case stays and gains the root assertion.apps/pi-extension/server/serverAnnotate-embeds.test.ts— the same three cases over the Node transport.bun run typecheck;bun test packages/shared packages/server apps/pi-extension(1099 / 866 / 291 pass, 0 fail);bun run --cwd apps/review build && bun run build:hook.Docs
AGENTS.md, "Embedded local documents" → Never the app in a frame: states the exact rule, the/and bare-word exemptions, and whySec-Fetch-Siteis not the signal.