feat(cli): smart paste — in-memory storage, multi-paste, marker reconstruction#4764
feat(cli): smart paste — in-memory storage, multi-paste, marker reconstruction#4764StefanIsMe wants to merge 2 commits intoNousResearch:mainfrom
Conversation
Code Review: PR #4764Thanks for the clean implementation! Here are my findings: Strengths:
Issues to Address:
Overall: Good implementation addressing a real bug. These are refinements, not blockers. |
Code Review: PR #4764 - Smart PasteOverall: Good implementation - The in-memory approach is cleaner than temp files and the multi-paste support is well designed. Strengths
Issues Found
Minor suggestions
Action needed: Fix the fallback paste detection bug before merge. |
…tion Fixes 7 issues from PR NousResearch#4764 review (britrik): 1. DRY: Extract _make_paste_marker() helper — marker format now defined once, used in handle_paste + _on_text_changed 2. Memory limit: Add MAX_PASTE_SIZE (100 KB) — prevents unbounded _paste_store growth from huge pastes 3. Deletion detection: Process ALL deleted markers per keystroke. Prior code returned after first deletion — remaining deleted markers were missed 4. Fallback paste: _on_text_changed now stores only the pasted portion, not full buffer text. Uses chars_added heuristic to detect paste events 5. Newline cleanup: Deletion wipe strips orphaned trailing newlines after removing marker + content block 6. / bypass fix: Check pasted_text.strip() not buf.text.strip(). Typing '/effort max' then Enter then pasting 5+ lines now correctly smart-pastes the pasted content 7. Regex: _PASTE_MARKER_RE now matches both line(s) and char(s) marker formats for robust reconstruction
569fff6 to
e85a0f3
Compare
…er reconstruction - Replace temp-file paste storage with in-memory _paste_store dict - Track active paste IDs in _active_paste_ids set for multi-paste support - Add _reconstruct_pastes() called at submit time to expand markers - Fix / bypass: now checks pasted_text (not buffer) starts with / - Add deletion detection: wiping any part of a paste marker wipes the whole block - Replace buf.insert_text() with buf.text = buf.text + placeholder to avoid post-cursor content being pushed to next line - Remove dependency on _hermes_home/pastes/ temp directory
…back Six bugs identified during PR NousResearch#4764 review, applied as an atomic commit: 1. DRY _make_paste_marker() helper — all 3 call sites (handle_paste, _on_text_changed, _reconstruct_pastes) now share one canonical function, eliminating the char vs line format mismatch that broke paste reconstruction. 2. MAX_PASTE_SIZE = 100_000 memory cap — pastes > 100 KB are inserted as plain text to prevent OOM from hoarding oversized payloads. 3. Deletion loop: remove early return — replaced with any_deleted flag so every active paste_id is checked per keystroke; one Delete can now clear multiple stale markers instead of stopping at the first match. 4. Fallback: store pasted_portion not full buffer — _on_text_changed now saves text[prev_len:] (the newly pasted chunk) rather than buf.text in its entirety, so pre-existing typed content is never hoisted into the paste store. 5. Unified _PASTE_MARKER_RE regex — _reconstruct_pastes uses a single regex matching both [+X line(s)] and [+X char(s)] formats, making it robust against any inconsistency between the bracketed-paste path and the fallback detection path. 6. Threshold aligned to 20 chars — both handle_paste and _on_text_changed now use char_count >= 20 as the collapse trigger, consistent with the documented contract.
Review fixes for #4764 — stacked PR@britrik I've addressed all 6 review items from #4764 in a stacked follow-up PR: Fixes applied (atomic commit)
Test plan is in the PR body. Ready for review. Forgive me for any mistakes. |
…back Six bugs identified during PR NousResearch#4764 review, applied as an atomic commit: 1. DRY _make_paste_marker() helper — all 3 call sites (handle_paste, _on_text_changed, _reconstruct_pastes) now share one canonical function, eliminating the char vs line format mismatch that broke paste reconstruction. 2. MAX_PASTE_SIZE = 100_000 memory cap — pastes > 100 KB are inserted as plain text to prevent OOM from hoarding oversized payloads. 3. Deletion loop: remove early return — replaced with any_deleted flag so every active paste_id is checked per keystroke; one Delete can now clear multiple stale markers instead of stopping at the first match. 4. Fallback: store pasted_portion not full buffer — _on_text_changed now saves text[prev_len:] (the newly pasted chunk) rather than buf.text in its entirety, so pre-existing typed content is never hoisted into the paste store. 5. Unified _PASTE_MARKER_RE regex — _reconstruct_pastes uses a single regex matching both [+X line(s)] and [+X char(s)] formats, making it robust against any inconsistency between the bracketed-paste path and the fallback detection path. 6. Threshold aligned to 20 chars — both handle_paste and _on_text_changed now use char_count >= 20 as the collapse trigger, consistent with the documented contract.
cb34642 to
a625209
Compare
…back Six bugs identified during PR NousResearch#4764 review, applied as an atomic commit: 1. DRY _make_paste_marker() helper — all 3 call sites (handle_paste, _on_text_changed, _reconstruct_pastes) now share one canonical function, eliminating the char vs line format mismatch that broke paste reconstruction. 2. MAX_PASTE_SIZE = 100_000 memory cap — pastes > 100 KB are inserted as plain text to prevent OOM from hoarding oversized payloads. 3. Deletion loop: remove early return — replaced with any_deleted flag so every active paste_id is checked per keystroke; one Delete can now clear multiple stale markers instead of stopping at the first match. 4. Fallback: store pasted_portion not full buffer — _on_text_changed now saves text[prev_len:] (the newly pasted chunk) rather than buf.text in its entirety, so pre-existing typed content is never hoisted into the paste store. 5. Unified _PASTE_MARKER_RE regex — _reconstruct_pastes uses a single regex matching both [+X line(s)] and [+X char(s)] formats, making it robust against any inconsistency between the bracketed-paste path and the fallback detection path. 6. Threshold aligned to 20 chars — both handle_paste and _on_text_changed now use char_count >= 20 as the collapse trigger, consistent with the documented contract.
…e / bypass - Change collapse trigger from 5+ lines to 20+ chars - Add URL detection (https?://) to bypass collapse always - Remove /-prefix bypass (commands still collapse if >= 20 chars) - Update placeholder to always show char count (was line-count branch) - Update docstrings to reflect new behavior - Fixes: URL pastes now paste verbatim, long command pastes collapse
a625209 to
3ae7bdf
Compare
Summary
Replaces the temp-file-based smart paste system with a clean in-memory implementation:
_paste_storedict, no temp files on disk_active_paste_idsset tracks which paste IDs are currently in the buffer_reconstruct_pastes()expands all markers back to full content when Enter is pressed/bypass: checks whetherpasted_text(notbuf.text) starts with/— old code would bypass when the buffer happened to start with/even if the paste itself did not. This also fixes the edge case where typing/effort maxthen pressing Enter then pasting 5+ lines of content now correctly smart-pastes the content.buf.text = existing_text + marker + '\n' + pasted_textinstead ofbuf.insert_text()to avoid pushing post-cursor content to the next lineChanges from v1 (addressing PR #4764 review by britrik)
1. DRY: Extracted
_make_paste_marker()helperMarker format logic was duplicated in 3 places. Now defined once as:
2. Memory limit:
MAX_PASTE_SIZE = 100_000_paste_storenow has a size limit — pastes exceeding 100 KB are inserted as plain text without collapsing.3. Deletion detection: process ALL deleted markers per keystroke
Prior code returned after processing the first deleted marker. Now uses a loop to handle multiple simultaneous deletions in one keystroke.
4. Fallback paste: stores only the newly pasted portion
_on_text_changed(fallback for terminals without bracketed paste) now trackschars_addedandnewlines_addedto detect paste events, rather than storing the entire buffer text.5. Newline cleanup in deletion wipe
When a marker + content block is deleted, the code now strips orphaned trailing newlines left behind.
6.
/bypass: checkpasted_textnotbuf.textBypass now correctly checks whether the pasted content itself starts with
/, not whether the buffer starts with/. This means:/effort maxverbatim → bypass (correct — it's a command)/effort maxon a prior line → smart paste (correct — the pasted text is not a command)7. Regex: support both
line(s)andchar(s)marker formats_PASTE_MARKER_REnow matches both[Pasted text #N +X line(s)]and[Pasted text #N +X char(s)]for robust reconstruction.What changed (cli.py only)
handle_paste(BracketedPaste handler): in-memory_paste_store+_active_paste_ids, correct bypass logic,MAX_PASTE_SIZEcheck,_make_paste_marker()helperhandle_enter: calls_reconstruct_pastes()before extracting text — agent sees full pasted content_paste_store,_active_paste_ids,MAX_PASTE_SIZE,_prev_newline_count_make_paste_marker()— consistent marker format for both line and char variants_reconstruct_pastes()— marker expansion + counter reset on submit_on_text_changed: deletion integrity check per-keystroke (no early return), fallback paste detection withchars_added/newlines_addedtracking, newline cleanup on deletionFiles Changed
cli.py(+857/-190)