Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @NorfeldtKnowit on file. In order for us to review and merge your code, each contributor must visit https://cla.warp.dev to read and agree to our CLA. Once you have done so, please comment @cla-bot check to trigger another check.

@NorfeldtKnowit

I'm starting a first review of this spec-only pull request.

You can view the conversation on Warp.

I completed the review and no human review was requested for this pull request.

Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).

Powered by Oz

@cla-bot check

The cla-bot has been summoned, and re-checked this pull request!

Thanks @oz-for-oss. Pushed faa4860 addressing all three findings:

🚨 CRITICAL — tech.md sibling-list windowing. Rewrote list_sibling_images_natural_sorted to window around the clicked image (up to 512 entries before, 512 after, clamped at the directory's first/last entries) before any truncation. The clicked image is now guaranteed to be inside the kept slice for any directory size. Updated the unit-test list to cover left-edge, right-edge, and centered cases against a 2000-image fixture; dropped the misleading prose paragraph in the Notes section that described the algorithm without implementing it.

⚠️ IMPORTANT — product.md replace-in-place flow. The "click another image to replace overlay in place" behavior was internally inconsistent with the scrim-blocks-input invariant. Rewrote the section as "Opening a second image while one is already open": the file-tree click steals focus, fires LightboxViewEvent::FocusLost, the workspace handler tears down the open Lightbox, and the second click goes through cold open. The update_params reuse path in the OpenLightbox handler still exists and is preserved for non-focus-stealing dispatch sites (artifact / screenshot Lightboxes from agent block panes), but the file-tree path no longer relies on it. Manual-validation step renamed and reworded accordingly.

⚠️ IMPORTANT [SECURITY] — tech.md eager 1024-sibling decodes. Restructured change 3 to build only current ± PRELOAD_RADIUS (default PRELOAD_RADIUS = 2, so 5 entries) as LightboxImageSource::Resolved; the rest are LightboxImageSource::Loading. LightboxView::start_asset_loads only loads Resolved entries, so on initial open at most 5 parallel reads/decodes are queued regardless of directory size. On Left/Right, a small navigation hook in LightboxView::handle_action walks the new ±PRELOAD_RADIUS window and dispatches UpdateLightboxImage to promote each newly-visible neighbour from Loading to Resolved, which routes through the existing update_image_at → start_asset_load path. Peak transient allocation is now bounded at ~5 * MAX_DECODED_PIXELS * 4 bytes ≈ 1.2 GB (combined with change 4's per-decode cap), versus the pre-fix ~262 GB worst case. The Risks section is updated to call this out as the chosen mitigation rather than relying on cache eviction reacting to allocation pressure.

/oz-review

@NorfeldtKnowit

I'm re-reviewing this spec-only pull request in response to a review request.

You can view the conversation on Warp.

I completed the review and no human review was requested for this pull request.

Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).

Powered by Oz

Thanks for the second pass @oz-for-oss. Pushed 10e1835 addressing all five findings.

1. Animated GIF/WebP needs enable_animation_with_start_time to repaint (product.md:56). v1 is now scoped to first-frame static preview for animated formats. Continuous playback would require wiring Image::enable_animation_with_start_time(Instant) (crates/warpui_core/src/elements/image.rs:128) into the Lightbox image element plus a per-frame redraw loop, which is enough surface area (animation timing, navigation reset semantics, eventual pause/play interaction) to belong in a follow-up. Files still load successfully and the user sees a static, unambiguous preview. Behavior 9/10, the Success Criteria bullet, and the manual-validation step are all updated; the follow-up entry is now spelled out as "continuous playback via enable_animation_with_start_time" with play/pause as the next layer.

2. Lazy promotion data model is underspecified (tech.md:154). Picked one: extend LightboxImage in crates/ui_components/src/lightbox.rs:38-43 with path: Option<PathBuf>. Every entry on the file-tree path carries Some(p); existing call sites (artifacts, screenshots, agent block paths) pass None and continue working. The navigation hook in LightboxView::handle_action reads image.path directly, so there's no filename-reconstruction or parent-dir-stashing needed; the data model is the source of truth.

3. collect_frames() allocates everything before the cap (tech.md:191). Replaced with a streaming collect_animated_frames_with_limits helper that iterates decoder.into_frames() directly, accumulates width * height per frame, and bail!s as soon as either the per-frame dimension cap or the running total-pixel cap is exceeded. The remaining iterator items are dropped along with the function's stack frame; peak allocation is bounded by MAX_DECODED_PIXELS * 4 bytes ≈ 256 MB plus one in-flight frame, instead of the entire animation. Both the WebP-animated and GIF arms are updated.

4. ImageType::Unrecognized leaves the Lightbox spinning (tech.md:222). try_from_bytes now returns Err for any unknown format (Ok(other) => bail!("unsupported image format: {other:?}") and Err(_) => bail!("could not detect image format from header bytes")) instead of Ok(Unrecognized). The Unrecognized variant is removed entirely (single-call-site cleanup in size_in_bytes). The asset cache then surfaces AssetState::FailedToLoad, which LightboxView maps to the new LightboxImageSource::Error { message } variant from change 5; the user sees a per-entry inline error with the filename. This also catches the extension/content-mismatch case (a .png containing tarball bytes) that was previously silent. New unit-test bullet covers the new Err behavior.

5. End-to-end flow contradicts the bounded preload (tech.md:368). Rewrote step 5 to describe building both Resolved (preload window) and Loading (rest of siblings) entries with path: Some(p) carried on every entry. Rewrote step 6 to say start_asset_loads queues loads only for Resolved entries and noted the update_params reuse path is not exercised on the file-tree path. Rewrote step 8 to describe lazy promotion via UpdateLightboxImage after each Left/Right and to thread the new Unrecognized → Err → AssetState::FailedToLoad → LightboxImageSource::Error failure flow.

/oz-review

@NorfeldtKnowit

I'm re-reviewing this spec-only pull request in response to a review request.

You can view the conversation on Warp.

I completed the review and no human review was requested for this pull request.

Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).

Powered by Oz

Pushed 2aff764 addressing all three notes from the third pass.

1. Animated decode still materializes every frame up to the cap (tech.md:255). Real bug; the previous round scoped product.md to first-frame-only display but left the decoder in change 4 happily collecting up to MAX_DECODED_PIXELS worth of frames per animated input. Collapsed both the WebP-animated and GIF-animated arms to first-frame-only decode, returning StaticBitmap directly:

• WebP: always go through decode_with_limits(data, image::ImageFormat::WebP) (same path as static WebP). The WebPDecoder yields the first frame whether or not the file is animated.
• GIF: read dimensions, cap-check, then DynamicImage::from_decoder(decoder).into_rgba8() for one frame; never call into_frames().collect_frames() or iterate frames at all.

ImageType::AnimatedBitmap is unconstructed in v1 (no in-tree caller actually drives animation today, since Image::enable_animation_with_start_time is not wired anywhere consuming it); the variant stays in the enum for the continuous-playback follow-up. The collect_animated_frames_with_limits helper from the previous round is dropped from the spec since it is no longer needed. Per-decode envelope is now MAX_DECODED_PIXELS * 4 bytes regardless of input being animated or static.

2. 1.2 GB transient is too generous (tech.md:177). Tightened both knobs:

• PRELOAD_RADIUS = 2 → 1 (window of 2 * 1 + 1 = 3 entries: prev, current, next).
• MAX_DECODED_PIXELS = 64M → 32M (~122 MB RGBA per decode).

Worst-case transient envelope is now ~3 * 122 MB ≈ 366 MB per file-tree click, down from ~1.2 GB. For typical photo directories (4000×3000 = 12M pixels, well under cap) actual allocation is a small fraction of that. Risks-section math is updated; reference "below cap" fixture for manual validation moves from 7000×7000 to 5000×5000 (25M pixels, just under the new cap); the rejection-path fixture stays at 10000×10000. Tighter envelopes (radius 0, smaller cap, or serializing decodes via a semaphore) are enumerated as a follow-up if real-world telemetry shows 366 MB is still too generous, but I want to leave one neighbour preloaded by default so Right-arrow is responsive in normal use.

3. A11y label requirement not implemented in tech (tech.md:311). Added a sub-section under change 5 spelling out the implementation:

• Lightbox::render per-image render branch sets the rendered Image element's accessible label to current_image.description.as_deref().unwrap_or("image").
• Loading entries get an accessible label of "loading {description}".
• Error entries get an accessible label of "image preview failed: {description}: {message}".
• Implementer should verify the GPUI element accessor name against crates/warpui_core/src/elements/image.rs and use the same accessor that other in-tree labeled elements (e.g. lightbox.rs:128-143 close button) use; the spec mandates the what not the exact API name.

Added a manual validation step: enable VoiceOver, click screenshot.png, confirm the announcement includes the filename instead of a generic "image" label.

This is the third /oz-review so the auto-retrigger budget is exhausted. Leaving the PR up for human review at this point; happy to iterate on anything reviewers raise.

@NorfeldtKnowit

I'm re-reviewing this spec-only pull request in response to a review request.

You can view the conversation on Warp.

I completed the review and no human review was requested for this pull request.

Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).

Powered by Oz

Thanks for the fourth-pass review @oz-for-oss. Pushed 112e581, which is a substantive narrowing rather than a touch-up; quick summary so you have context if you pick it up.

Critical (modality contradiction). Cut sibling navigation, the bounded-preload window, and the click-to-swap flow entirely. v1 is now a single, fully modal, single-image Lightbox: while open, the workspace (file tree, terminals, code editor, tab bar) is inert; opening another file means dismiss-first. That removes the inert-vs-click-through contradiction at its source rather than papering over it.

Important (pre-read DoS surface). Added a unified pre-read file-size cap and a bounded AssetCache read, so a multi-gigabyte file or a symlink to /dev/zero is rejected at the metadata stat before any bytes are read, and a TOCTOU grow-after-stat is bounded by the read cap. Files above the cap surface as the per-entry error state without ever allocating the bytes.

Important (animated frame materialization). The previous round's first-frame-only collapse would have regressed the changelog's animated rendering, which I missed. Reverted that: AnimatedBitmap frame collection is unchanged so the changelog still animates. Hardened it instead with image::Limits plus an explicit frame-count and total-pixel budget enforced during iteration (bail! mid-stream, drop the iterator), so a pathological animated WebP/GIF cannot allocate every frame before the cap is checked.

Important (SVG decompression bomb). Added an SVG intrinsic-dimension cap at parse time, so a 200-byte SVG declaring <svg width=\"200000\" height=\"200000\"> is rejected before rasterization.

Suggestion (a11y label honesty). Verified against crates/warpui_core/src/elements/image.rs: GPUI's Image element does not currently expose an accessibility-label API. Descoped the rendered-image screen-reader label from v1 and listed it as the first follow-up rather than mandating an API the codebase does not have.

Quick procedural question: my read of the prompt is that the "up to 3 times on the same pull request" budget is aimed at re-review spam where nothing has changed between requests. Does it apply identically when the retrigger follows a substantive code push, or only to re-reviews against the same revision? Happy to leave this for human review either way; mostly want to make sure I am reading the rule correctly.

/oz-review

@NorfeldtKnowit

I'm re-reviewing this spec-only pull request in response to a review request.

You can view the conversation on Warp.

I completed the review and no human review was requested for this pull request.

Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).

Powered by Oz

Thanks for the fifth-pass review @oz-for-oss. Pushed b6349ff addressing both findings.

Important (product/tech consistency on global decoder). The honest answer is that change 4 does harden the shared ImageType::try_from_bytes path globally, and centralizing the caps in the decoder is the right place for them rather than a bug. The product.md line claiming "no global decoder behavior changes" was the inconsistency. Reworded the Inside-the-Lightbox bullet to state plainly that v1 hardens the shared decoder against pathological inputs (oversized dimensions, animated frame-count and total-pixel budgets, SVG intrinsic-dimension cap), that the budgets are sized well above any in-tree consumer's legitimate inputs, and that the only user-visible global effect is pathological/malicious files surfacing as decode errors at every consumer of ImageType::try_from_bytes instead of silently allocating gigabytes of memory. The changelog's animated rendering continues unchanged. Existing Success Criterion 8 and the changelog regression-check in Validation step 9 are already aligned with this wording.

Important / Security (post-open regular-file revalidation). Real TOCTOU gap. Updated change 5 in tech.md to require a post-open is_file() check on the opened descriptor (fstat against the open file, not a fresh path-based stat) before any byte is read. This closes the swap-between-stat-and-open window: even if the path is replaced with a FIFO, character device, or directory between change 2's pre-read path stat and change 5's open syscall, the check runs against the already-open descriptor and rejects before read_to_end can hang on a FIFO or stream until OOM on /dev/zero. Spelled out in the change-5 prose, the code block, the Risks > Pre-read size envelope section, a new unit test local_file_read_rejects_post_open_non_regular_file, and a new manual validation step 7a (race-replace the path with a FIFO between stat and open). The post-open check is in addition to, not a replacement for, the pre-read size cap and the bounded MAX + 1 read; together they cover oversize regular files, file-grows-after-stat, file-type-swaps-after-stat, and FIFO-was-the-original-target.

/oz-review

@NorfeldtKnowit

I'm re-reviewing this spec-only pull request in response to a review request.

You can view the conversation on Warp.

I completed the review and no human review was requested for this pull request.

Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).

Powered by Oz

Thanks for the sixth-pass review @oz-for-oss. Pushed 9a043b5 addressing all three findings.

Important / Security (open() blocks on FIFO before is_file check). Real gap; the round-5 post-open check only runs if open() itself returns. On Linux/macOS, open(O_RDONLY) of a FIFO with no writer blocks indefinitely. Updated change 5 in tech.md to apply O_NONBLOCK via OpenOptions::custom_flags(libc::O_NONBLOCK) gated #[cfg(unix)]. POSIX read(2) on a regular file completes fully regardless of the flag, so no fcntl-clear step is needed before the bounded take(). Documented why on Windows the post-open is_file() is sufficient on its own (named pipes do not present as regular files there). Added a #[cfg(unix)] unit test local_file_read_does_not_block_on_fifo that creates a writer-less FIFO via mkfifo, passes the path to the asset-cache load future, and asserts the future returns Err quickly. Added manual validation step 11b for an explicit FIFO race-replace and updated the Risks > Pre-read size envelope section to enumerate the three coordinated mitigations (pre-read stat in change 2, O_NONBLOCK + post-open is_file() in change 5, bounded MAX + 1 read).

Important / Security (SVG parser CPU/memory under 64 MB cap). Layered defense in tech.md. (a) Introduced MAX_SVG_BYTES = 4 * 1024 * 1024 (4 MB) in change 5; the bounded read picks MAX_SVG_BYTES + 1 for .svg paths and MAX_ASSET_LOCAL_FILE_BYTES + 1 for raster. 4 MB is comfortable for any legitimate icon, diagram, or illustration SVG (real-world is usually a few hundred KB). (b) Added a looks_like_svg_xml content-sanity prefix check in change 4 that, before usvg::Tree::from_data is invoked, peeks the first 1 KB of bytes and confirms a UTF-8-BOM-tolerant whitespace-tolerant <?xml or <svg prefix. This catches the "binary blob renamed to .svg" case and is bounded-time / no-allocation. (c) Added unit tests local_file_read_caps_svg_at_smaller_limit, decode_svg_rejects_non_xml_prefix, and decode_svg_accepts_xml_prelude_with_bom_and_whitespace. (d) Added manual validation step 11a for a 3.5 MB SVG containing 50000 nested <g> elements (rejected by the byte cap before parse) and a binary-blob renamed .svg (rejected by content-sanity). Tracked a usvg/resvg parse-budget API as a follow-up; if it ever lands, it becomes the right primary defense and the 4 MB cap can relax.

Suggestion (product spec conflates pre-read with decode-time caps). Reworded the Performance posture and Limits sections in product.md to call out the two distinct caps. Pre-read size cap rejects oversize files before any read; decode-time dimension and pixel caps reject oversize decoded images during decode. Added concrete examples covering the matrix: a 3 MB JPEG with normal pixel content passes both; a 100 MB PNG with normal pixel content fails pre-read; a 3 MB JPEG header decoding to 100000×100000 passes pre-read but fails decode; a 100 MB PNG decoding to 100000×100000 fails pre-read (decode never runs). Updated Success criteria 5, 6, 7, and 10 with explicit cap attribution. Extended the Validation fixtures list with FIFO, deeply-nested SVG, and binary-blob .svg cases.

/oz-review

@NorfeldtKnowit

I'm re-reviewing this spec-only pull request in response to a review request.

You can view the conversation on Warp.

I completed the review and no human review was requested for this pull request.

Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).

Powered by Oz

Thanks for the seventh-pass review @oz-for-oss. Pushed 1b79a5d addressing both findings.

Important / Security (extension-keyed SVG cap is bypassable). Real bypass; the round-6 design picked the cap from path extension, so an evil.png containing 50 MB of SVG XML would have read at the 64 MB raster cap. Switched the cap to content-keyed at the asset-cache layer in change 5: after open + post-open is_file(), peek the first 1 KB, run looks_like_svg_xml on the peek, and pick MAX_SVG_BYTES + 1 (4 MB) if the prefix looks XML/SVG-shaped, otherwise MAX_ASSET_LOCAL_FILE_BYTES + 1 (64 MB). Extension is no longer used to choose the cap. The reused helper (looks_like_svg_xml) is the same one change 4 uses to gate usvg::Tree::from_data, so the asset-cache cap and the parser gate share one signal by construction. Reconciled the spec text in change 4's SVG section and Risks > Pre-read size envelope to describe the cap as content-keyed and to spell out why extension-keying was discarded. The parser route in try_from_bytes remains extension-keyed (only .svg reaches usvg) AND content-checked (looks_like_svg_xml must pass), so evil.png with SVG content still cannot reach usvg; the content-keyed read cap is defense-in-depth even if try_from_bytes is ever changed to content-sniff. Added unit tests local_file_read_caps_svg_content_under_png_extension and local_file_read_uses_raster_cap_for_non_svg_content, plus a manual-validation fixture for the same case.

Important (global LocalFile cap under-surveyed). Surveyed every current AssetSource::LocalFile consumer in the codebase and documented each in a new "Other LocalFile consumers and rollout impact" subsection in change 5: image preview (this PR), markdown image embeds in the editor and notebooks, custom theme background images, theme-deletion preview, settings profile image, blocklist images. For each consumer the typical legitimate input envelope (icons / avatars / screenshots / theme backgrounds in the 0.5–5 MB range) sits at least an order of magnitude below the 64 MB raster cap and well below the 4 MB SVG cap, so a single global cap does not regress any current consumer. Decision: keep the global cap as the simpler design. The parameterized AssetSource::LocalFile { path, max_bytes: Option<u64> } shape is documented as a follow-up for if/when a video-preview or large-export consumer ever lands and legitimately needs higher caps.

/oz-review

@NorfeldtKnowit

I'm re-reviewing this spec-only pull request in response to a review request.

You can view the conversation on Warp.

I completed the review and no human review was requested for this pull request.

Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).

Powered by Oz

Thanks for the eighth-pass review @oz-for-oss. Pushed 759a5bd addressing all three consistency findings from the round-7 content-keyed switch.

Important (product.md describes SVG cap as pre-read). The cap is post-open, after the first-1-KB content peek; the pre-read metadata cap is uniform at 64 MB across formats because no bytes are read at that point. Reworded the relevant Limits and Performance-posture bullets in product.md to state plainly that the SVG-specific cap (MAX_SVG_BYTES = 4 MB) fires at the asset-cache bounded read step, not at the pre-read metadata stage, and that the two caps are complementary defense-in-depth (a 50 MB file is rejected by the pre-read cap whether or not it is SVG; a 5 MB SVG is rejected by the post-open content-keyed cap; a 5 MB JPEG is read normally and proceeds to decode). Adjusted Success criterion 10 to match.

Important (end-to-end flow describes extension-keyed cap). End-to-end-flow step 7 in tech.md previously read "cap is MAX_SVG_BYTES for .svg paths and MAX_ASSET_LOCAL_FILE_BYTES otherwise" — pure extension-keyed wording carried over from the round-6 design. Replaced with content-keyed wording: after open + post-open is_file(), peek the first 1 KB of the file, run looks_like_svg_xml on the peek, and select the cap from content regardless of extension. The asset-cache code block in change 5 was already content-keyed since round 7; only the narrative around it needed reconciling. Audit grep confirms no stale design-as-current statements survive; remaining extension-keyword hits are anti-pattern contrast wording ("content-keyed, not extension-keyed"), historical context for the round-6→7 switch, or unrelated supported-format-list strings.

Important (manual fixture below the 4 MB cap). Bumped the SVG parser-DoS fixture in Manual validation step 11a from "around 3.5 MB" (which would have parsed successfully and not exercised the cap) to "4.5 MB above MAX_SVG_BYTES", with the explicit expected outcome "rejected by the content-keyed asset-cache cap, no parse attempted, per-entry error state shown". Added an evil.png-renamed parallel (same 4.5 MB SVG payload under a .png extension) so the manual run also exercises the content-keyed-not-extension-keyed contract end-to-end, and an under-cap counter-fixture (100 KB legitimate SVG, expected to render normally). Verified the existing unit test local_file_read_caps_svg_content_under_png_extension already uses a 4.5 MB fixture so it actually exercises the cap.

/oz-review

@NorfeldtKnowit

I'm re-reviewing this spec-only pull request in response to a review request.

You can view the conversation on Warp.

I completed the review and no human review was requested for this pull request.

Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).

Powered by Oz

Thanks for the approve-with-nits @oz-for-oss. Pushed 6bc517b addressing the one suggestion.

Suggestion (SVG prefix predicate broadening). Real concern: legitimate SVGs in the wild often start with authoring-tool comments (<!-- Generated by Inkscape -->) or DOCTYPE declarations (<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1//EN" ...>), and the round-6 predicate only accepted <?xml or <svg after optional BOM and whitespace, so those files would have hit the per-entry error path instead of rendering. Broadened looks_like_svg_xml to accept four prelude tokens after the optional UTF-8 BOM and bounded ASCII whitespace scan: <?xml, <svg, <!--, and <!DOCTYPE. Added an explicit "Supported prelude subset" subsection in tech.md enumerating the four accepted forms, noting that the predicate is a coarse content sniff (not a full XML lexer) and that usvg::Tree::from_data remains the actual XML/SVG validator. Out-of-scope forms (lowercase <!doctype, non-<?xml processing instructions, leading-data attributes, etc.) are documented as falling through to the per-entry decode error rather than being rejected at the content-sanity gate. Same helper is used at both layers (asset-cache cap selection in change 5, parser gate in change 4) so broadening covers both. Added unit tests decode_svg_accepts_doctype_prelude and decode_svg_accepts_xml_comment_prelude; the BOM-and-whitespace and binary-prefix tests are retained.

Leaving the PR for human review at this point. The retrigger budget across these passes has been generous and substantive — thanks for the careful read on the threat model in particular (the FIFO open-blocks-before-fstat finding and the content-keyed-vs-extension-keyed SVG cap bypass were both real bugs in the spec that I would not have caught on my own).