spec: render inline image protocols at shell startup (#10020)#10126
spec: render inline image protocols at shell startup (#10020)#10126lonexreb wants to merge 1 commit intowarpdotdev:masterfrom
Conversation
Adds product.md + tech.md for issue warpdotdev#10020: inline image-protocol escape sequences (Kitty graphics, iTerm2 inline image) emitted from shell init files (~/.zshrc, etc.) are silently dropped. The same bytes typed manually post-prompt render correctly. Investigation traces the drop to the block-level delegate! macro (app/src/terminal/model/block.rs:2882): when the active block is in BlockState::BeforeExecution (which covers the entire ScriptExecution bootstrap stage and the post-prompt early-output window), image-protocol completions are routed to header_grid instead of output_grid. The header_grid is the prompt area and its image placement is not rendered in the visible block body. The reporter's background-subshell test case (`( sleep 0.2 && fastfetch ) &!`) hits the same branch via a different session phase, which is why deferring past the first prompt does not help. Spec proposes a fix at the block-level delegate (not BlockList, not GridHandler) with explicit reasoning against the alternatives. Carves out the WarpInput hidden bootstrap block so Warp's own bootstrap-script bytes never accidentally render. Adds warn-level telemetry on every silent-drop path so this class of bug regresses noisily, not silently. Test plan covers four unit cases (BlockState routing) and six integration cases (per-stage byte-feed end-to-end), including a negative-space test that the WarpInput stage continues to drop and a feature-flag-off regression guard. Three open questions for maintainer review on macro naming, telemetry level, and whether the existing bootstrap-stage gates at blocks.rs:3789/3724/3763 are intentional or symptoms of the same family.
github-actions
Bot
added
the
external-contributor
|
/oz-review |
![oz-for-oss[bot]](https://avatars.githubusercontent.com/in/3450139?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overview
Reviewed the spec-only PR for rendering inline image protocols during shell startup. The product/tech split covers the reported phases and proposes a plausible routing fix, but the telemetry requirements and implementation details need tightening before this is safe to hand to implementation.
Concerns
- The product spec contradicts itself on whether feature-flag-off drops should warn or stay quiet.
- The tech spec requires BootstrapStage in GridHandler logging without specifying the plumbing or alt-screen behavior.
- The test plan does not cover the Kitty feature-flag-off path despite the cross-protocol/flag invariant.
- Security: malformed PTY-controlled image sequences should not be able to create unbounded warn-level log volume or log image payload data.
Verdict
Found: 0 critical, 3 important, 1 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
|
|
||
| If an image-protocol completion is reached for any reason that prevents | ||
| placement (zero dimensions, feature flag off, decode failure, missing | ||
| target grid), the event is logged at `warn` level with the protocol |
There was a problem hiding this comment.
| - protocol name (`"iterm"` / `"kitty"`), | ||
| - failure reason (`"feature_flag_off"`, `"zero_dimension"`, | ||
| `"decode_failure"`), | ||
| - the active `BootstrapStage` (passed in via existing handler context |
There was a problem hiding this comment.
| Asserts the image is placed in the new active block's `output_grid`. | ||
| - IT4: Existing post-prompt typed-command path continues to render | ||
| (regression guard). | ||
| - IT5: With `FeatureFlag::ITermImages` disabled, IT2's bytes produce |
There was a problem hiding this comment.
| `bootstrap_stage()` (used by `bootstrap_block_contents`, | ||
| blocks.rs:3325), so no new accessor is needed. | ||
|
|
||
| ### Change 4: warn-level telemetry on silent drop |
There was a problem hiding this comment.
💡 [SUGGESTION] [SECURITY] Specify that drop telemetry never includes image payload/control data and is sampled or rate-limited for repeated malformed sequences, since PTY output can generate many failures quickly.
1 participant
Summary
Spec for issue #10020 — inline image-protocol escape sequences (Kitty graphics, iTerm2 inline image) emitted from shell init files (
~/.zshrc,~/.bashrc, etc.) are silently dropped, while the same bytes typed manually after the prompt render correctly. Other terminals (iTerm2, WezTerm, Kitty, Ghostty) render images at shell startup as documented in their respective protocol specs.Investigation
Traced the routing end-to-end:
TerminalModel::handle_completed_iterm_image(terminal_model.rs:3354) →delegate!→BlockList.BlockList::handle_completed_iterm_image(blocks.rs:3794) →delegate_to_block!→ active block (intentionally bypasses the early-output gate).Block::handle_completed_iterm_image(block.rs:3292) → block-leveldelegate!(block.rs:2882).delegate!routes toheader_gridwhenstate == BlockState::BeforeExecution. Theheader_gridis the prompt area; its image placement is not rendered in the visible block body.BeforeExecutionand image bytes land inoutput_grid, which renders. This is why typing the same command works.The reporter's background-subshell case (
( sleep 0.2 && fastfetch ) &!) hits the same branch via a different session phase: the post-prompt block is itself inBeforeExecutionwaiting for the user's first keystroke, so background output emitted during theis_early_output()window is also routed toheader_grid.What's in the spec
product.md— 7 testable behavior invariants (B1–B7), 6 acceptance criteria (A1–A6), explicit non-goals, and a preserved record of the reporter's diagnostic facts (verified byte sequences, alternative approaches that were tried and ruled out).tech.md— picks the fix site with explicit reasoning against the two alternatives, names every file and line range touched, and lays out four unit tests + six integration tests (including a negative-space test for theWarpInputhidden bootstrap block and a feature-flag-off regression guard).Architectural choices
delegate!macro, not atBlockListand not atGridHandler. Rationale:BlockListdoes not know aboutheader_gridvsoutput_grid; pushing the routing decision up would leak grid identity.GridHandleris downstream of the routing decision; pushing it down would require a redirect message back up. The block-leveldelegate!is exactly where the (state, grid) routing decision belongs.BootstrapStage::WarpInput. Warp's own injected bootstrap-script bytes must never accidentally render an image even after the fix.Test plan
block_test.rs— verify routing perBlockStatevalue and perbootstrap_stage.image_protocol_at_startup_test.rs(new file) — feed the exact byte sequences from the issue at each session phase, assertEvent::ImageReceiveddispatch and grid placement.~/.zshrcwith--logo-type kitty-direct(A1),--logo-type iterm(A2), and the background-subshell variant (A3).fastfetchcontinues to render (A4 regression guard).FeatureFlag::ITermImagesdisabled produces no render and no warn-level log noise (A5 + B7).Open questions for maintainer review
delegate_image_completion!(sibling macro) or a boolean parameter on the existingdelegate!?log::warn!vslog::debug!+ a separate metric?blocks.rs:3789(on_reset_grid),3724(precmd), and3763(preexec) are left alone. Confirm they are intentional and not symptoms of the same family.Closes (spec-only) #10020 — implementation PR will follow once spec direction is confirmed.