⚠️ This sandbox change breaks HTML artifact scroll restoration, and the comment is backwards.

Regression: appendHtmlPreviewScrollRestoration (still called at line 496) injects a <script> into the blob HTML (preview.ts:216-253) that does window.parent.postMessage(...) to save/restore scroll position, and the parent listens for those messages (lines 460-488). With allow-scripts removed, that injected script never runs, so the iframe never emits save/restore-request events and scroll position is no longer preserved across re-renders. No test covers this - the e2e only asserts the sandbox attribute value, and the only scroll test is for markdown artifacts.

No security gain: the prior config (allow-scripts allow-forms, no allow-same-origin) was already the safe way to run sandboxed scripts - opaque origin can't reach parent.document/cookies, and postMessage(..., "*") works fine from it. So this trades a working feature for no real security benefit unless scroll restoration is reimplemented parent-side (which allow-same-origin would actually permit via contentDocument access).

Misleading comment: the rationale says staying same-origin prevents an opaque origin that would "break inline scripts / forms," but the new sandbox denies both scripts and forms. It should say they're intentionally disabled (matching the PR description) and explain why allow-same-origin is still needed.

Confirmed and fixed — allow-scripts allow-forms is restored (the opaque-origin config was indeed already the safe one), and the comment now explains that scripts are needed for the injected scroll-restoration postMessage bridge while allow-same-origin is deliberately omitted. The e2e asserts the restored sandbox value.

🟠 The isSafeHref guard runs after the citation branch (line 61), and CitationLink renders <a href={href}> directly (citation-link.tsx:34). So a prompt-injected [citation:x](javascript:alert(1)) matches ^citation:(.+)$ on the label, skips this guard, and renders a clickable javascript: anchor - exactly the threat this hardening targets.

Consider moving the isSafeHref check above the citation branch, or validating href inside CitationLink.

Fixed — the isSafeHref guard now runs before the citation branch, so [citation:x](javascript:...) renders the disabled span. Added a render-level regression test ("blocks unsafe hrefs before the citation branch") that renders the component with a citation-labelled javascript: href and asserts no anchor is produced.

🟡 Heads up: this allowlist also rejects mailto: and tel: schemes, so legitimate [contact](mailto:[email protected]) links now render as disabled spans. If that's intentional, fine - otherwise consider adding them. The unit test doesn't cover mailto:/tel:, so the behavior change is invisible.

Added mailto: and tel: to the allowlist, updated the stale docstring that still claimed http/https-only, and added unit cases covering both schemes so the behavior is pinned.

🟡 Minor: this destructures only { className, children, ...rest }, leaving target/rel (valid AnchorHTMLAttributes) in rest spread onto a <span>, which triggers React DOM warnings. The external branch below extracts target/rel explicitly; this branch should too. (node from react-markdown is a pre-existing spread issue, not new.)

Fixed more thoroughly: the span fallback no longer spreads ...rest at all, so anchor-only attributes (href/target/rel) and react-markdown's node prop never reach the <span>. The new render-level test actually caught href leaking onto the span in ArtifactLink via the spread — both fallbacks now only take className/children.

🟠 The guard throws on !res.ok, but the surrounding catch (err) { console.error(err); } (line 1720) swallows it, so loadMessages() resolves normally and the effect's .catch(() => toast.error(...)) never fires. This is also a manual useCallback, not a TanStack Query, so isError does not flip - contrary to the comment added here ("Surface the failure to TanStack Query so isError flips").

The real effect is only a cleaner console.error and avoiding res.json() on HTML error bodies. To actually surface failures to the user, rethrow past the inner catch (or remove it) so the toast fires.

You were right that the throw was swallowed and the TanStack comment was wrong. This is now moot: upstream #4065 rewrote useThreadHistory as a real TanStack useInfiniteQuery whose queryFn throws on !response.ok (via readResponseErrorMessage) and toasts on historyQuery.error — exactly what this change was groping toward. After rebasing, this PR no longer touches core/threads/hooks.ts.

🟡 This test reconstructs the .then guard inline and asserts it rejects - it never renders useThreadHistory. So removing the guard from hooks.ts would not fail this test (the test has its own copy of the guard). The comment here claiming it pins the contract ("dropping the !res.ok throw causes this expectation to flip ... and the test to fail loudly") is inaccurate; it only verifies the fetch wrapper passes ok/status/statusText through.

Consider an integration test that renders the hook with a stubbed fetch returning 500 and asserts the error surfaces.

Agreed it was a strawman — the test re-implemented the guard instead of exercising it. The file was removed during the rebase: the manual .then guard it mirrored no longer exists after upstream #4065 replaced loadMessages with a TanStack useInfiniteQuery.

🟡 Only core/settings/local.ts is wrapped. core/agents/feature-cache.ts:21,33 and app/workspace/agents/new/page.tsx:133,137 still call window.localStorage directly and can throw into render handlers. Fine if intentionally scoped to settings, but the "best-effort storage" robustness is inconsistent across the app.

Made it consistent: safeLocalStorage is now exported and the agent-create save-hint effect (app/workspace/agents/new/page.tsx) uses it — that was the only remaining unguarded call site. core/agents/feature-cache.ts already wraps its window.localStorage access in try/catch internally, so it was left as is.

This guard is good, but it only covers createMarkdownLinkComponent (used by message content + message-list-item). Markdown artifacts render through a separate, unguarded component: artifact-file-detail.tsx renders .md previews via <SafeStreamdown components={{ a: ArtifactLink }}> (artifact-file-detail.tsx:518), and ArtifactLink (citations/artifact-link.tsx) renders <a {...rest} /> with no scheme check.

A prompt-injected [x](javascript:...) in a .md artifact would then execute in the main document, where sessionStorage / CSRF cookies are reachable - exactly what this guard prevents elsewhere. Consider applying the exported isSafeHref at the top of ArtifactLink (same span fallback), or having ArtifactLink delegate to this shared component.

Fixed — ArtifactLink now applies the shared isSafeHref guard up front and renders the same disabled-span fallback, so javascript:/data: hrefs in .md artifact previews can't reach a real anchor in the main document. Added render-level tests for both the unsafe-span and safe-anchor paths (which also caught the href leaking onto the fallback span through the rest spread — fixed).

The rethrow does enable the auto-load toast at loadMessages().catch(() => toast.error(...)) (hooks.ts:1750) - that's the real improvement here. But two issues:

1. The comment on the !res.ok block above says this surfaces the failure "to TanStack Query so isError flips on the caller side." loadMessages isn't a queryFn (it uses local refs + setMessageRows), and the hook's return object (hooks.ts:1772) has no isError/error field - so isError never flips anywhere. The comment is misleading.

2. loadMessages is also exposed as loadMore and called as a bare loadMore?.() without .catch() from LoadMoreHistoryIndicator (message-list.tsx:133/147, wired via loadMore={loadMoreHistory} at :708). Before this change the catch swallowed the error so it never rejected; now a 5xx on the scroll/manual path produces an unhandled promise rejection (and no toast on that path), and the loadMore?: () => void signature understates the new contract.

Suggest catching + toasting inside loadMessages and not rethrowing (keeps the toast, removes the unhandled-rejection surface), or adding .catch() at both call sites. If callers need to observe failures, expose an explicit error/isError field rather than throwing through a () => void callback.

Both points were valid — and both are now moot after the rebase: upstream #4065 replaced loadMessages with useInfiniteQuery, so loadMore is historyQuery.fetchNextPage (which never throws through the callback — TanStack captures failures in query state) and errors surface through historyQuery.error + toast. This PR no longer touches core/threads/hooks.ts.

This new error-surfacing path (the !res.ok throw here + the throw err rethrow below) doesn't have test coverage. Worth adding a test asserting a 5xx / 404 surfaces the failure to the caller and doesn't leave loading stuck true.

After rebasing onto main, the error path here belongs to upstream #4065's useInfiniteQuery queryFn (throws on !response.ok, toasts via historyQuery.error, and loading derives from isLoading/isFetchingNextPage so it can't stick). Our duplicate guard and its test were dropped rather than shadow-testing upstream's implementation from this PR.

Good to assert the sandbox on the HTML preview iframe. The PR also adds sandbox="" to the other artifact iframe - the urlOfArtifact one at artifact-file-detail.tsx:363 - but there's no assertion covering it. A parallel toHaveAttribute("sandbox", "") would catch a future regression (e.g. someone adding allow-scripts) on that iframe too.

Done — added an e2e that opens a PDF write_file artifact and asserts sandbox="" on the urlOfArtifact iframe. Writing it surfaced that non-code browser-previewable write_file artifacts were forced into the code editor (iframe never rendered — the CI failure you saw); fixed so they render in the sandboxed iframe. Full Playwright suite passes locally (95/95).