Add product + tech spec for OSC 8 hyperlinks (GH6393)#9850
Add product + tech spec for OSC 8 hyperlinks (GH6393)#9850th1nkful wants to merge 5 commits intowarpdotdev:masterfrom
Conversation
Warp does not currently parse OSC 8 hyperlink escape sequences, so output that uses the standard `ESC ] 8 ; params ; URI ESC \` form (e.g. wizcli's "Click to view scan report") is unclickable. The spec proposes parsing OSC 8, attaching the URI to the cells covered by the span via a per-grid hyperlink registry, and routing Cmd+click through the existing `ctx.open_url` path used for auto-detected URLs. product.md captures the behavior surface as 18 numbered, testable invariants (recognition / closing / params, hover and click affordances, right-click menu, copy semantics, coexistence with auto-detected URLs, block boundaries, streaming, scrollback, sharing as markdown, AI context, robustness, and a scheme allow-list). tech.md grounds the implementation in the existing OSC dispatcher (app/src/terminal/model/ansi/mod.rs:811-1230), the Handler trait's default-impl pattern, both cell-storage variants (FlatStorage and Cell/CellExtra), and the GridHighlightedLink hover/click flow in app/src/terminal/view/link_detection.rs. The plan is feature-flagged behind FeatureFlag::OscHyperlinks so the layers can land independently.
|
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 @claude 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 |
|
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 Powered by Oz |
There was a problem hiding this comment.
Overview
This PR adds product and technical specs for OSC 8 hyperlink support in terminal output. The specs cover the user-visible behavior, parser/storage/view-layer plan, rollout flag, testing, and follow-up work.
Concerns
- The proposed model lookup reuses
Linkas a single contiguous range, but the product spec requires same-idhyperlinks to behave as one logical link across non-contiguous runs. - Required persistence for restored/shared sessions is deferred as a follow-up, contradicting the product requirements.
- Raw copy semantics require preserving original OSC 8 bytes, but the proposed parser/registry model only stores normalized hyperlink data.
Security
- Terminal output is untrusted, so the scheme allow-list must be specified as an actual centralized validator instead of assuming
ctx.open_urlenforces one. - The registry DoS mitigation is not part of the proposed design; URI length and per-grid entry caps need to be required and tested.
Verdict
Found: 0 critical, 5 important, 0 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
Five Important findings from oz-for-oss on PR warpdotdev#9850: 1. Link is single-range. Narrow product invariant 5 to contiguous runs only and update tech.md to call out that Link's RangeInclusive shape cannot represent non-contiguous same-id spans. Cross-run id grouping moves to Follow-ups. 2. Centralize the scheme allow-list. Add tech section 5a defining a `link_security` module with `check_open_scheme` and `open_validated`, require every click/menu open path to route through it, and close the same gap for the existing auto-detected URL flow which today calls ctx.open_url directly. Includes a presubmit lint to flag raw ctx.open_url uses inside terminal/view. 3. Copy-as-bytes cannot preserve original OSC 8 bytes. Update product invariant 13 to specify Warp emits semantically equivalent OSC 8 (URI preserved, params normalized to canonical form), and update tech section 7 to explain why per-cell byte preservation is not worth the cost. 4. Registry caps belong in design, not deferred. Replace the "spec later" mitigation with three concrete caps (URI byte length, max distinct entries, refcounted reclamation) plus tests. Hostile output cannot OOM the process. 5. Session persistence cannot be a follow-up while invariant 12 requires it. New tech section 6a covers serialization of Cell -> CellExtra (already serde-derived) and the HyperlinkRegistry alongside grid persistence in Warp Drive, history, and the session-sharing protocol; uses #[serde(default)] for backward compatibility with older clients.
|
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 @claude 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 |
|
/oz-review |
|
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 Powered by Oz |
There was a problem hiding this comment.
Overview
This PR adds product and technical specs for OSC 8 hyperlink support, covering parsing, cell storage, view integration, persistence, sharing, AI context, feature gating, and security scheme validation.
Concerns
- The spec has internal contradictions around same-
idcross-run grouping: product behavior and follow-ups defer it, but the model and integration tests require it. - The security validation design is not consistently applied: one API cannot represent malformed-but-hoverable OSC 8 URIs, the feature-flag section may accidentally gate the existing URL hardening, and the end-to-end flow bypasses the validation chokepoint.
- The bounded registry/reclamation plan depends on refcount transitions that are not specified for overwrites, RLE mutations, reflow, eviction, and deserialization.
Verdict
Found: 0 critical, 5 important, 0 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
Five Important findings from oz-for-oss on PR warpdotdev#9850: 1. Registry reclamation accounting was under-specified across cell overwrite, row eviction, RLE split/merge, reflow, and deserialize. Switch to bounded no-reclaim: registry grows monotonically until the grid (block / alt-screen) is dropped. Bound is 4096 distinct entries x 4096-byte URI -> ~16 MB worst case per block. Tests updated to assert no shrinkage and that grid drop reclaims memory. 2. check_open_scheme(&Url) forced the caller to throw away unparseable URIs that product invariant 15 requires to remain hoverable and copyable. New API takes &str and returns SchemeCheck { Allowed | Rejected{ Unparseable | DisallowedScheme{scheme} } }. Tooltip and "Copy link" always operate on the literal URI; only the open path gates on validation. 3. Layer (5a) scheme allow-list is explicitly NOT gated by FeatureFlag::OscHyperlinks. Disabling OSC 8 must not regress security on the existing auto-detected URL flow. The flag is restricted to the b"8" dispatch arm and the Hyperlink variant of GridHighlightedLink. 4. The sequence diagram now routes through link_security (open_validated) and shows both the Allowed and Rejected branches, so implementers can't copy an unsafe ctx.open_url pattern from the spec. 5. Dropped osc8_split_id_grouping integration test (required non-contiguous same-id grouping that's now in Follow-ups). Replaced with osc8_soft_wrap_keeps_one_span (contiguous span across a soft wrap, in scope) and added osc8_unparseable_uri_inert. Unit test for the registry reworded to match v1 scope.
|
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 @claude 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 |
|
/oz-review |
|
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 Powered by Oz |
There was a problem hiding this comment.
Overview
This PR adds product and technical specs for OSC 8 hyperlink support, including parser/storage plumbing, UI integration, persistence, sharing, AI context, feature flagging, and security hardening for link opening.
Concerns
- The fixed scheme allow-list in the tech spec conflicts with the product spec's compatibility/no-regression requirements for existing URL opening behavior.
- Persistence compatibility is asserted but not tied to the concrete serialization formats or old-client behavior needed to make it safe.
- The semicolon-in-URI mitigation is buried in Risks instead of being part of the parser contract and tests.
Security
- Shared markdown/block export needs explicit escaping and scheme handling so untrusted terminal output cannot create unsafe or deceptive rendered links.
- AI/agent context needs a clear trust-boundary requirement: OSC 8 URIs are untrusted data and must not be treated as automatic instructions to fetch, open, or act.
Verdict
Found: 0 critical, 4 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
|
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 Powered by Oz |
Four Important + one Suggestion finding from oz-for-oss on PR warpdotdev#9850: 1. Markdown sharing was under-specified for untrusted URIs. Product invariant 13 now mandates: (a) only schemes that pass the same allow-list used for clicks are emitted as clickable markdown — others become plain text — and (b) markdown-escape the visible text and URL-encode the URI so the link can't break out of its anchor. 2. AI / agent context (invariant 14) now states explicitly that OSC 8 URIs are untrusted data, must not be auto-fetched/opened/executed, and must use the same validation/confirmation boundaries as direct user clicks. URIs are presented to the model with provenance labeling so prompt-injection embedded in visible text or URI cannot trick the agent into auto-following. 3. Tech 5a's fixed allow-list and product invariant 16's "existing allow-list" language reconciled. Product 16 now enumerates the allow-list directly (http/https/mailto/ftp); tech 5a documents that today's auto-detected URL flow has no explicit allow-list but the detection regex only matches http/https, so applying the new allow-list to that path is not a regression. Adds a LinkRejectedScheme telemetry event for rollout monitoring. 4. Persistence compat split into three concrete per-format stories (sqlite history, Warp Drive, session-sharing protocol), with both compat directions covered for each. sqlite and Warp Drive ride for free because stylized_output is the raw ANSI byte stream including OSC 8. Session-sharing gets a SetHyperlink event with explicit skip-unknown audit per framing and a fallback capability negotiation. CI matrix tests pinned-old-client x new-client both directions. 5. (Suggestion) Semicolon-in-URI rule promoted from Risks to the parser contract in tech 1. URI is now defined as the `b";"`-rejoin of params[1..]; a uri_with_semicolons_is_rejoined unit test is mandatory and called out as the cardinal indicator of an implementation that took the two-field shortcut.
|
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 @claude 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 |
There was a problem hiding this comment.
Overview
This PR adds product and technical specs for OSC 8 hyperlink support, including parsing, cell storage, hover/click behavior, persistence, sharing, AI context, and a URL scheme allow-list.
Concerns
- Copy semantics are internally inconsistent: default selection copy is visible text, but the sharing invariant says plain copy-block emits OSC 8 escape bytes.
- The registry API and bounded-registry behavior contradict each other about whether
interncan fail. - Active hyperlink ownership is unclear across
Block,BlockGrid, andGridHandler, which risks a broken implementation where the handler hook updates different state than the cell writer uses. - Cell mutation semantics for erase/insert/overwrite operations need to be specified to prevent stale hyperlink ids.
Security
- OSC 8 URIs are attacker-controlled hidden terminal data; the AI/agent context requirement needs an explicit trust-boundary rule before agents are told to act on those URIs.
- The scheme allow-list is not consistent between product and tech specs, and the hardening applies to existing auto-detected URL clicks, so the no-regression expectation needs a single source of truth.
Verdict
Found: 0 critical, 5 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
Five Important + one Suggestion finding from oz-for-oss on PR warpdotdev#9850: 1. Default copy was conflated with explicit byte-export. Product invariant 13 now enumerates three distinct user actions: text selection copy (visible text only, per invariant 8), Copy as markdown (markdown link with scheme gating + escaping), and Copy as terminal bytes (semantically equivalent OSC 8 with scheme gating). None of them produces OSC bytes by default. 2. Product invariant 14 tightened. OSC 8 URIs in agent context are "untrusted metadata," presented to the model in a structured form (e.g. `<untrusted-uri>` tag) it cannot mistake for an instruction. Any tool fetch/open/navigation goes through the existing user-approval and scheme-validation boundaries, regardless of prompt-injection in visible text or URI. 3. Registry intern signature now `Option<HyperlinkId>` to match the bounded-cap behavior. URI byte-length cap is enforced in the parser on the raw &[u8] before allocating the String, so a 1 GB OSC 8 sequence never produces a 1 GB allocation; the registry's distinct-entries cap is enforced at intern time. set_hyperlink uses and_then so a None intern result lands as None active id. 4. New tech section 3d spells out hyperlink_id behavior under every cell-mutating operation: input, erase_chars, clear_line, clear_screen, delete_chars, insert_blank, delete_lines, insert_blank_lines, scroll_up/down, reverse_index, decaln, reset_state, and prompt-marker block boundary. One unit test per row of the table. Closes the implementation hazard where erased blanks could stay clickable or inserted blanks could inherit the active link. 5. Tech 3c rewritten with a single-owner delegation table. GridHandler is the sole owner of active_hyperlink_id and HyperlinkRegistry; BlockGrid and Block forward set_hyperlink to the inner GridHandler. AltScreen owns its own state. closes the risk that set_hyperlink updates one copy while input reads another. 6. Allow-list parameterized by LinkSource (OscHyperlink | AutoDetected). OSC 8 keeps the conservative list (http/https/mailto/ftp); auto-detected mirrors what urlocator emits today (http/https/ftp/ftps/file/git/ssh/mailto/news/gopher) to honor invariant 18's no-regression promise. Product invariant 16 now defines two named lists; tech 5a adds the LinkSource enum and the AUTO_DETECTED_ALLOWED_SCHEMES const, with a telemetry plan to detect drift between urlocator and our list.
|
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 @claude 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 |
|
@oss-maintainers — bringing this in for a maintainer pass per CONTRIBUTING.md. Have used the three oz review rounds and would appreciate a human review on the spec. |
Note
the issue was labelled bug + ready-to-implement but the scope of the change seemed pretty big, so I’m creating a spec first to make sure it aligns fully with the warp team
Warp does not currently parse OSC 8 hyperlink escape sequences, so output that uses the standard
ESC ] 8 ; params ; URI ESC \form (e.g. wizcli's "Click to view scan report") is unclickable. The spec proposes parsing OSC 8, attaching the URI to the cells covered by the span via a per-grid hyperlink registry, and routing Cmd+click through the existingctx.open_urlpath used for auto-detected URLs.product.md captures the behavior surface as 18 numbered, testable invariants (recognition / closing / params, hover and click affordances, right-click menu, copy semantics, coexistence with auto-detected URLs, block boundaries, streaming, scrollback, sharing as markdown, AI context, robustness, and a scheme allow-list).
tech.md grounds the implementation in the existing OSC dispatcher (app/src/terminal/model/ansi/mod.rs:811-1230), the Handler trait's default-impl pattern, both cell-storage variants (FlatStorage and Cell/CellExtra), and the GridHighlightedLink hover/click flow in app/src/terminal/view/link_detection.rs. The plan is feature-flagged behind FeatureFlag::OscHyperlinks so the layers can land independently.
Description
Linked Issue
also related: #176
ready-to-specorready-to-implement.Screenshots / Videos
Testing
Agent Mode