[akbash-bot]

Issue

An application in a pane still detects Kitty graphics support after the user disables it and starts a fresh Herdr server. It can discard its text fallback and leave a blank image area.

Problem

The embedded terminal enables image storage by default. Herdr skipped its graphics-enabling step when configured off, but left that default active. The pane therefore continued answering graphics probes positively.

How did we fix it?

Herdr now starts each embedded terminal with image storage disabled. Panes explicitly enable it when graphics is configured on, as before. This restores the existing opt-out without changing outer-terminal capability detection.

Verification

The reporter's exact probe returned OK in a fresh Linux session with graphics disabled before the fix. With the final build in a separate fresh session, it returns only DA1. Regression tests cover that reply, explicit enablement, reset, discarded uploads, and the empty-storage fast path. just check passed, including the full test suite and Windows cross-lint.

refs #4978

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Repository UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 7443402a-8173-4991-8e05-f0fb37e40b57

📥 Commits

Reviewing files that changed from the base of the PR and between 3d9d2b1 and 3a514c4.

📒 Files selected for processing (2)

• crates/ghostty-vt/src/lib.rs
• src/pane.rs

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

---

📝 Walkthrough

Walkthrough

Terminal::new now sets Kitty image storage to zero, returns an error if that configuration fails, and records the current graphics generation as empty. Kitty graphics remain disabled until explicitly enabled. Tests cover capability-query responses and image uploads while disabled, behavior after enablement and reset, storage-generation changes, and fixture setup.

Suggested reviewers: ogulcancelik

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to 3a514

Panes now start with Kitty graphics disabled until graphics are explicitly enabled, which matches the stated opt-out fix. No merge-blocking risk was found.

Security Architecture Review

Security architecture risk: ⚪ Minimal · up to 3a514

The change makes the graphics opt-out effective for newly created panes while preserving explicit enablement. No introduced or materially worsened security finding was identified.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — The relevant attacker-controlled input is terminal output containing Kitty commands. For newly constructed terminals whose graphics enablement is skipped, the change reduces access to image-command execution rather than granting additional authority or introducing a new recipient.

Trust Boundaries and Controls

• observed — A zero storage limit means graphics storage is disabled. The native command executor checks that state before dispatching graphics actions and returns no response, including for capability queries. Production enablement remains conditional on the existing graphics-enabled setting.

Resilience and Maintainability Implications

• observed — Initialization failures propagate before ownership is transferred, and Terminal destruction frees the native handle. Explicit enablement remains a multi-step, nontransactional operation inherited from the base, but both production constructors discard their local terminal on an enablement error rather than exposing partial configuration.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 77.78% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 2 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes the main change: honoring the Kitty graphics opt-out.
Description check	✅ Passed	The description explains the graphics opt-out issue, the fix, and the reported verification. It is directly related to the changeset.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

• Fix all pre-merge checks with AI

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[kangal-bot]

@coderabbitai review
@greptileai

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[greptile-apps]

Confidence Score: 5/5

[Medium risk] Disables kitty graphics storage by default in terminal initialization.

The PR appears safe to merge.

Summary

The PR disables Kitty image storage when an embedded terminal is created, then relies on the existing pane opt-in for graphics-enabled configurations. It adds regression coverage for capability replies, resets, discarded uploads, and storage-generation behavior.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  A[Create embedded terminal] --> B[Disable Kitty image storage]
  B --> C{Graphics configured on?}
  C -->|Yes| D[Enable Kitty graphics before pane output]
  C -->|No| E[Ignore graphics commands and queries]

Loading

_{Reviews (1) · Last reviewed commit: "fix: honor the kitty graphics opt-out"}

[akbash-bot]

Regarding the docstring-coverage warning: no new public API is introduced. The new functions are regression tests, and the constructor change has comments explaining both initialization decisions. I am leaving those tests and existing function documentation unchanged rather than adding docstrings solely to meet the percentage threshold.