Skip to content

fix(terminal): resolve image, PTY, and async search defects - #67

Merged
simota merged 2 commits into
mainfrom
fix/terminal-review-findings
Sep 5, 2026
Merged

fix(terminal): resolve image, PTY, and async search defects#67
simota merged 2 commits into
mainfrom
fix/terminal-review-findings

Conversation

@simota

@simota simota commented Sep 5, 2026

Copy link
Copy Markdown
Owner

Kitty image transfers could delete ordinary temporary files, read invalid shared-memory ranges, exceed storage limits after failed placements, or reuse stale GPU textures. PTY exit could overtake final output, and search missed soft-wrapped text while scanning history on the UI thread. This fixes those paths and adds focused regressions.

  • Require both an approved temporary directory and the protocol filename marker, validate opened files and shared-memory ranges, and copy shared-memory data through the kernel.
  • Expand packed/palette PNGs and transparency with allocation limits; enforce GPU dimensions, image/frame quotas, and unique image revisions. Index image storage and reuse placement GPU resources.
  • Drain final PTY output before exit, bounded at two seconds when descendants retain the slave. Share an 8 MiB input/reply budget through the actual write and cancel blocked writes on shutdown.
  • Search immutable snapshots in a worker with 35 ms debounce and cancellation. Match and highlight text across soft wraps, preserve navigation during pending searches, and discard results after screen switches.
  • Ignore overflowing CSI parameter sequences and coalesce redraw notifications for panes sharing a deadline while retaining pending repaint work.

Validation

  • cargo fmt --all -- --check — passed.
  • cargo build --workspace --locked — passed.
  • cargo test --workspace --locked --quiet — 2,544 passed, 15 ignored. The sandbox blocked two POSIX shared-memory tests; the complete suite passed outside the sandbox.
  • git diff --check origin/main...HEAD — passed.

Regression coverage includes PNG transparency, shared-memory range checks, delayed PTY output, write-budget release, stale search cancellation, queued search navigation, wrapped highlight rendering, and GPU resource reuse.

No interactive macOS screenshot or recording was captured. Linux-specific shared-memory truncation behavior and native GUI latency remain unverified on this macOS host. Existing mechanism measurements and their limits are recorded in performance-measurements.md.

Review notes

No dependencies, configuration keys, or persisted formats change. Sustained terminal output can delay search-result publication; output from descendants can be discarded after the two-second exit drain deadline. Image metadata is capped at 4,096 images and 16,384 frames independently of the pixel-byte limit.

The fix/evidence map and operating policies are in review-fixes-2026-09-05.md; its recorded test count predates the final search regressions. Reverting requires no migration but restores the prior file-deletion and memory-boundary defects.

Prevent unintended temporary-file deletion, unsafe shared-memory access,
and image quota bypasses. Preserve PTY tail output and bound all pending
writes so a child that stops reading cannot grow the reply queue forever.

Correct PNG expansion, image epochs and dimensions, wrapped-line search,
and CSI overflow handling. Move history searches outside the UI thread
and terminal lock, and reduce redraw, image lookup and GPU resource work.

Add regression coverage, repeatable performance probes, and operating
limits for drain timeouts, write reservations and image metadata.

Validation: cargo build --workspace --locked --offline;
cargo test --workspace --locked --offline (2540 passed, 15 ignored);
cargo fmt --all -- --check; git diff --check.
Clip wrapped matches to each rendered row, bind search jobs to their submitted screen generation, and apply queued navigation after the matching results arrive.

Validate with regression tests, cargo build --workspace, cargo test --workspace, and cargo fmt --all -- --check.
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 5, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-09-05T06:29:19.866459Z 9586a9f PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@simota
simota merged commit c9dbec3 into main Sep 5, 2026
1 check passed

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9586a9f3f0

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +74 to +75
if let Some(worker) = &self.search_worker {
worker.cancel();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Scope search cancellation to the target pane

When window A has a pending query and focus moves to window B, invoking the menu or command-palette “Clear Search” targets B but this unconditional call cancels the single app-wide job belonging to A. Window A retains its open prompt and displayed query, yet its result will never publish until the query is edited again; cancellation should verify the target terminal or be maintained per pane.

Useful? React with 👍 / 👎.

Comment on lines 1038 to 1040
if changes_active_space {
self.screen_generation = self.screen_generation.wrapping_add(1);
self.invalidate_grid_coordinate_space();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Invalidate searches when repeated alt-screen entry clears them

When already on the alternate screen, a repeated DECSET 47 or 1047 makes changes_active_space false, so this generation is not advanced, although enter_alt_screen unconditionally clears both screens' search state below. An in-flight worker can consequently pass its generation and content checks and restore the query/highlights that the control sequence just cleared; the pending-search validity generation must also change whenever this clearing occurs.

Useful? React with 👍 / 👎.

@simota
simota deleted the fix/terminal-review-findings branch September 5, 2026 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant