fix: close three gaps the artifact-frame review found - #6481
Conversation
Follow-ups to #6461, each an internal contradiction that change introduced rather than a new capability. The dead-frame retry never fired in the case it exists for. The grace window was armed only when no document had reported yet, and that flag reset only when a NEW url landed — so once a document rendered and reported its height, an engine-initiated renavigation of the same single-use url (a back/forward-cache eviction, the case the spec names) bumped the load counter, hit the guard, and left a 404 inside the frame with no notice and no retry. The window now re-arms on every load. A link inside an artifact navigating the frame arms it too, and that is the right outcome: in both cases the frame has stopped showing the artifact, and offering the retry is milder than silently re-minting over an action the reader took. The dashboard shell was the one ancestor-emitting site nothing validated. Two of the three built their `frame-ancestors` through `origin.frame_ancestors_value`; the shell hand-joined `'self'` with the extras, so the helper's guarantee that no future ancestor can reintroduce an inexpressible entry did not actually hold there. That class of bug is not hypothetical — a bracketed IPv6 literal was being emitted, and an engine that sees one refuses the whole source expression and drops every ancestor with it. Two module docstrings documented a design that was reverted before merge, and worse, restated a mechanism this work disproved: they said the header names the request's own origin because `'self'` resolves against the response's opaque origin and matches no ancestor. A probe showed `'self'` matching fine at single-level nesting; the real cause was that `frame-ancestors` is matched against EVERY ancestor. The code three hunks below each docstring emits `'self'`, and four tests pin it. Both new guards are mutation-verified: reverting the shell to a hand-join and dropping the per-load reset each turn exactly their own test red.
github-actions
Bot
added
the
readiness: checking
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsNo findings. False positive or not applicable? A repository writer can comment: |
Design Review (Fable 5) — ✅ PASSDesign-level review of Design-Verdict: PASS Three real internal contradictions from #6461, each fixed at its root cause — one validated ancestor builder, per-load re-arm — with mutation-verified tests. [DESIGN-REVIEWED] f902941 |
First Principles Review (Fable 5) — ✅ PASSPremise-level review of All claims verified. The review is complete; here is the structured output the contract requires. First-Principles-Verdict: PASS Three declared fixes, each aimed at a nameable cause, zero new surface — code brought back into line with spec, validator, and shipped behavior. What this change shipsIntent: make the artifact-frame retry fire in the eviction case the spec already promises, and stop the shell CSP from emitting ancestors browsers refuse — a FIX.
Item 1 sits at cause level: the observation was keyed on the URL when the invariant is per-document, and the fix re-keys it per load rather than special-casing eviction. Item 3 removes the class, not the instance: grepped [FIRST-PRINCIPLES-REVIEWED] f902941 |
UX Review (Fable 5) — 🟡 CONCERNSUX-level review of UX-Verdict: CONCERNS Re-arming the failure notice on every load makes a deliberate in-frame link click surface "Couldn't render this artifact" — a message that misstates what happened. Watch
The bfcache-eviction fix itself (404 in the frame now gets a retry instead of nothing) and the backend header/comment changes are solid and add no UX surface. [UX-REVIEWED] f902941 |
Opus 4.8 Review — ✅ no blocking findingsReviewed Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
|
The UX finding is correct, and the defect is this PR's. Verified against source rather than reasoned about: the frame is sandboxed with an opaque origin, so nothing in the component can read Concretely wrong in the link case: the artifact did render, "Couldn't render this artifact" says otherwise, and "Retry" re-mints — discarding the page the reader chose to open. The suggested remedy is the right shape. One correction to it: the neutral copy should apply only to the silence path, not to both.
Not applied in this PR, deliberately, and this is the one judgment call worth a human: the copy lands in 14 locale files under a zero-tolerance i18n gate, and choosing the wording is a product decision rather than a mechanical fix. Everything else on this PR is green with no blocking verdict, so the choice is whether to hold it for the copy change or take it now and follow up — that is the author's call, not mine to guess. The rest of the review's read matches mine: the bfcache-eviction fix is the point of the change, and the header/docstring corrections add no UX surface. |
|
Tracked as #6489 — the cause-neutral copy for the silence path, with the two-keys-not-one correction, the 14-locale constraint, and the falsification for each existing test. Not blocking this PR: the reachable case it describes is the link-navigation one, and the notice firing there is a wrong message rather than a broken frame. |
github-actions
Bot
added
readiness: passed
github-actions
Bot
removed
the
readiness: passed
2 participants
Follow-ups to #6461. Each of these is an internal contradiction that change introduced, found by its own review round after merge — not a new capability. Six files, no new surface.
The dead-frame retry never fired in the case it exists for
#6461 added a grace window: a frame whose document never reports its height is showing something this surface did not build (the document URL is single-use, so an engine-initiated renavigation lands on a 404 that fires
loadlike any other navigation), and the reader gets the existing retry.It was armed only when no document had reported yet, and that flag reset only when a new URL landed. So once a document rendered and reported, a renavigation of the same spent URL — the back/forward-cache eviction case
artifacts.mdexplicitly claims is covered — bumped the load counter, hit the guard, and left a 404 inside the frame with no notice and no retry. Code and spec disagreed, and the disagreement hid the whole feature.The window now re-arms on every load. A link inside an artifact navigating the frame arms it too, and that is the right outcome rather than a cost: in both cases the frame has stopped showing the artifact, and offering the retry is milder than silently re-minting over an action the reader took.
The dashboard shell was the one ancestor source nothing validated
Three sites build a
frame-ancestorsvalue from_extra_frame_ancestors. Two route throughorigin.frame_ancestors_value, which re-validates every entry against a strict origin form; the shell CSP hand-joined'self'with the extras, so the helper's own guarantee — that no future ancestor source can reintroduce an inexpressible entry — did not hold for the shell.Not a hypothetical class:
http://[::1]:<port>was being emitted before #6461. A CSP host-source admits only letters, digits and hyphens, so an engine that sees a bracketed IPv6 literal refuses the whole source expression and drops every ancestor with it — which is why that grant never worked and why it produced a console warning per framed response.The new test asserts this by behaviour, not by reading the source: with
_extra_frame_ancestorsstubbed to offer both an inexpressible and an expressible entry, the header must carry the expressible one and'self'while the IPv6 literal never appears. That distinguishes a filter from a blanket refusal.Two docstrings documented a reverted design, and a disproved mechanism
sandbox_doc.pyandwebapp_preview.pyboth said the header names the request's own origin rather than'self', because'self'would resolve against the response's opaque origin and match no ancestor at all.Both halves are wrong. A probe with
frame-ancestors 'self'loaded fine at single-level nesting in Chrome 152, so the opaque-origin explanation is disproved; the real cause of the original blanking is that the directive is matched against every ancestor, which'self'alone cannot satisfy at nesting depth. And the server-derived-origin design those paragraphs describe was reverted before #6461 merged — it namedhttp://localhost:<port>while a phone was onhttps://<tunnel-host>and blanked every frame on that path. The code three hunks below each docstring emits'self', and four tests pin exactly that.A stale docstring that restates a falsified mechanism is worse than none: it is what a future reader would reason from.
Tests
Both new guards mutation-verified — each mutation turns exactly its own target red, suite green when restored:
" ".join(["'self'", *extra])Backend 211 passed across the four touched modules;
isort/flake8/mypy/black/docs-lintclean.tscclean,eslint0 errors (2 pre-existing warnings in untouched branches of the same file), 37 frontend cases passed across the touched files,npm run buildclean.Why no screenshot: no UI is added or changed. Two of the three fixes are a response header and two docstrings; the third makes an already-shipped failure notice fire in one more case, using the same markup and copy #6461 already carries evidence for. The one behaviour a screenshot could show — the retry appearing after an engine-initiated renavigation — cannot be provoked in a headless Chromium run, since it depends on the engine deciding to evict and reload the frame; it is pinned by the test instead.