Expand input box to show full multi-line draft + Ctrl+J fallback - #11
Merged
Conversation
added 4 commits
May 19, 2026 01:06
Previously the input bar was always a single row: typing across multiple logical lines kept only the cursor's line visible, with a '[N lines]' indicator on the status row. Hard to review a draft. Now the bottom panel grows from 1 row up to MAX_INPUT_VISIBLE_LINES (8) as the user adds newlines. Continuation rows show a dimmer '·' prompt so the user can see which row is row-0 of the draft. Beyond 8 lines the panel stays capped and scrolls internally to keep the cursor row visible; the status line shows '[N lines, K hidden]'. The chat viewport above shrinks by the same amount via the new 'input_rows' field on Renderer; visible_lines() / content_row() / render_viewport() all consult it. Also added Ctrl+J as a third newline trigger alongside Shift+Enter / Meta+Enter. Many terminals don't deliver Shift+Enter as a distinct keystroke, but Ctrl+J is universally delivered as a control character on Unix terminals.
Two layout bugs in the multi-line input feature: - FilePicker drew at `rows.saturating_sub(3)`, hardcoding the pre-multi-line assumption that the input bar sits at `rows - 2`. When the user typed several lines and triggered the picker on a later line, the picker overlapped with the input rows. Picker now takes an `input_top: u16` parameter and anchors one row above whatever the renderer reports as the input top. - Token counter could overdraw the rightmost characters of the cursor's line on a wide screen with a long line. Reserved a 12-col band at the right edge for the counter by reducing `visible_width` globally, so input rendering and counter rendering never collide.
When a paste source delivered \r\n or \r-only line endings, the
matches('\n').count() check returned 0, the threshold (>= 4 lines) was
never met, and the raw text — including embedded \r chars — got
inserted into the buffer. The terminal then rendered those \r chars as
carriage-returns (cursor-to-col-0), making subsequent text overwrite
earlier text and producing a garbled single 'line' on screen.
Normalize \r\n → \n and any remaining \r → \n before stripping
PASTE_MARK and counting lines. Multi-line pastes now collapse correctly
regardless of which line ending the source used.
Repro: paste 5 lines → Ctrl+U kills the marker into kill-ring → Enter on empty buffer (still ran pastes.clear()) → Ctrl+Y yanks marker bytes back into the buffer → submit. The marker bytes referenced a now-empty pastes vec, so expanded() silently omitted them and the agent received only the surrounding text. The user saw '[N lines pasted]' on screen but their paste was gone in transit. Fix: before clearing self.pastes on submit, walk the kill ring and expand any marker blocks in-place to their stored bodies. Kill-ring entries become self-contained raw text and survive the pastes wipe. Also factored expand_with_pastes() out as a free helper so both expanded() and the kill-ring flatten share the same logic. Added 10 robustness tests covering: paste at start / middle, two distinct pastes, Home/End across markers, multi-line Up/Down with a marker on a middle row, empty paste no-op, paste-of-only-PASTE_MARK no-op, Delete-at-start-of-marker removes whole block, multibyte chars adjacent to a marker, and the yank-after-submit regression itself. 184 tests passing.
yogthos
added a commit
that referenced
this pull request
May 21, 2026
…aths (#111) 23 audit findings verified REAL via parallel agent verification + cross-check against opencode/pi reference patterns. Shipping the 10 most concrete fixes here; the rest go in a follow-up docs/test batch. ## Security - **#9 bash quote_aware_split missed bare `|`** — `safe_cmd | rm -rf /` was treated as one segment; only the LHS got permission-checked. Pipe RHS rode in unchecked under the fallback (non-semantic-bash) path. Added single-byte `|` split after `||` is matched. The tree-sitter path was already correct. - **#4 read.rs no binary detection** — feeding a PDF/ELF/.pyc into the LLM as lossy UTF-8 wasted tokens and confused the model. Ported opencode `read.ts:153-198`: reject by extension list (zip/exe/.o/.pdf/.png/etc.), then sniff the first 4 KiB — null byte = binary, >30% non-printable = binary. Clear error message tells the agent to use bash + xxd instead. ## Correctness - **#2 skill override inverted** — README contract: "Project skills override global skills by name". Code used `map.entry(name).or_insert(skill)` which KEEPS the first (global) value and silently drops project overrides. Switch to `map.insert` (last-write-wins) since globals iterate first and project iterates second. - **#37 skill empty name** — frontmatter `name:` with empty value parsed to "", which then matched any `skill ""` call silently. Fall back to directory name when frontmatter name is empty/whitespace-only. - **#1 session_tree.janet hook never fired** — plugin defined `(defn on-message ...)` but `(def hooks [])` was empty AND the hook name doesn't exist (dirge uses `on-message-update`). `/label` was permanently broken ("no entry yet"). Fix: rename to `on-message-update` + register in hooks vector. - **#7 workflow.janet hooks vector missing entries** — plugin defined `workflow-on-tool-end`, `-on-error`, `-on-complete` but only registered the first four hook names. Three hooks were dead. Added them. - **#26 MCP malformed JSON silently empty args** — `serde_json::from_str(&args).unwrap_or_default()` turned bad JSON into None, sending the server an empty argument set. Server then errored with confusing "missing required field" instead of dirge surfacing the actual parse error. Now returns ToolError with the parse error message + first 200 chars of the offending JSON. - **#22 /prompt default unreachable** — README documents `default` as a built-in prompt (prompts/default.md exists), but `/prompt default` was intercepted as a magic "clear" keyword. If `default` is registered in `context.prompts`, the new branch falls through to the normal name-lookup. Only acts as clear-keyword when no `default` prompt is present (legacy fallback). - **#23 /allow add accepted invalid tools** — typo `/allow add bsah ...` silently created an inert rule the user couldn't debug. Added a known-tools whitelist matching PermissionConfig fields; unknown tools error with the valid list. ## Performance + correctness - **#11 grep loaded whole files into memory** — no size cap meant a 9MB file got fully buffered. Added 10 MiB per-file cap via metadata pre-check. - **#15 Python dunder methods marked non-exported** — `!name.starts_with('_')` treats `__init__`/`__call__`/etc. as private, even though they're Python's standard public protocol. Recognize `__x__` dunder pattern as exported. ## UI - **#36 panel char-count truncation vs Unicode width** — panel truncation used `chars().count()` while wide emoji and CJK take 2 cells. A status line with an emoji overflowed the right border by one cell. Switched to `UnicodeWidthStr::width` for both truncation and padding. ## Tests 4 new regression tests: - `test_is_binary_extension_known` — pdf/tgz/.so/.jpg/.pyc - `test_is_binary_content_null_byte` — null byte trigger, UTF-8 Japanese stays clean, all-non-printable triggers - `quote_aware_split_splits_on_bare_pipe` — pipe security - `quote_aware_split_or_and_pipe_distinct` — `a || b | c` produces 3 segments, not 2 725 plugin / 599 default pass. All build profiles clean. ## Verified false positives (not fixed, audit was wrong) - #3 cache.rs clear() race — generation counter gating in `get` makes stale entries invisible, no correctness impact. - #17 DeepSeek auto-detect priority — auto-detect only fires when env vars present; default-default is still OpenRouter. - #19 semantic tools in collision filter — semantic tools added separately, can't be shadowed by MCP. - #20 glob global gitignore — intentionally disabled to match grep behavior. - #28 nearest_root blocking std::fs — function doesn't exist in current code. - #32 ReadArgs.path vs GrepArgs.path — semantically different by design (file vs dir), documented in schema. - #33 install_plugin_providers dead-without-feature — gated with explicit `#[cfg_attr(not(feature), allow(dead_code))]`. - #34 websearch double-gated — config + API key serve distinct purposes (enable + auth). ## Deferred to follow-up batches Docs-only fixes (#6 CONFIG.md tools, #12 temperature, #13 --api-key, #14 acp_host/port), MCP/LSP architecture (#8, #25, #27), test gaps (#38-40), and lower-priority polish — all in a follow-up PR. Co-authored-by: Yogthos <yogthos@gmail.com>
yogthos
pushed a commit
that referenced
this pull request
May 22, 2026
…rf cache Self-review of the last 4 commits flagged 11 findings. Addressing all in one batch. - **#1 HIGH — leading whitespace dropped on first row of soft_wrap**: `current.is_empty()` at row start unconditionally dropped `token.leading_ws`, including on the first row of a logical line. Option lines `" ▶ label …"` lost their ` ` margin; the green ` allowed …` confirmation lost its indent too. First-row branch now preserves leading_ws (with a ws-overflow fallback to keep the token). - **#2 HIGH — drain_events early-broke before reader quiesced**: the Ok(false) shortcut fired on the first quiet poll, which was often "the background reader currently holds crossterm's internal mutex" rather than "terminal is quiet". A delayed OSC 11 / DA1 response could still escape past our drain. Now requires at least one observed event before the Ok(false) shortcut; honors the full budget otherwise. - **#3 HIGH — MODIFIED section clipped its own bottom border at `available == 4`**: row_budget=1, footer + 1 file = 2 items, +3 frame rows = 5 total in a 4-row budget. draw_panel clipped the `╰────╯`. Bumped MIN_MOD_SECTION_ROWS from 4 → 5 so the bottom border is always painted. - **#4 MEDIUM — `\r` not stripped from CRLF input**: Windows / some-MCP tool output left `\r` in tokens, producing terminal redraw artifacts. `soft_wrap` now strips a trailing `\r` per logical line. - **#5 MEDIUM — Show cursor on alt screen was a no-op**: `Show` was issued while still on the alt screen; `LeaveAlternateScreen` restores the main screen's saved DECTCEM state, discarding the Show. Moved Show to AFTER LeaveAlternateScreen + disable_raw_mode. - **#6 MEDIUM — recent(256) clones + locks on every redraw**: panel redraws on every streamed token. Added `modified::version()` monotonic counter (bumped on mark / clear); panel-side cache in `panel_modified_cached` keyed by (version, cwd) skips the lock + 256-PathBuf clone + path-strip when nothing changed. - **#7 MEDIUM — break_long_token could emit wide-glyph row > budget at max_width<2**: floored max_width at 2 inside soft_wrap. A 1-cell terminal is unusable anyway; this just removes a sharp edge. - **#8 MEDIUM — all-whitespace first row collapsed to empty**: same root cause as #1, fixed by the same change. Indented blank separators now preserve their indentation. - **#9 LOW — `allowed …` confirmation flush against alert `╰─╯`**: added a blank-line breathing row before the green confirmation so the alert's bottom border and the confirmation don't read as one block. - **#10 LOW — head_w used chars().count() not display width**: switched to `UnicodeWidthStr::width(head)` so future wide-glyph markers won't under-pad the continuation indent. - **#11 LOW — single-select marker width inconsistent**: cursor marker was `▶` (w=1), non-cursor was ` ` (w=2), so wrapped tails of adjacent options drifted by one column. Cursor marker padded to `▶ ` so all markers in a question share display width. 5 new tests: - preserves_leading_whitespace_on_first_row - strips_carriage_returns_from_crlf_input - wide_glyph_respects_max_width_at_floor - preserves_leading_whitespace_only_line - version_bumps_on_mark_and_clear 651 tests pass; fmt clean.
yogthos
pushed a commit
that referenced
this pull request
May 22, 2026
…nds, error masking Adversarial review of `1c341e9`/`8e60553`/`69318b7` flagged 14 findings. Addressing all. - **#1 HIGH — C1 controls bypassed MCP stderr sanitizer**: the original filter was `b == 0x09 || (0x20..0x7f).contains(&b) || b >= 0x80`. That second clause let through every non-ASCII byte including the C1 control range (U+0080..=U+009F). In particular U+009B is single-byte CSI, behaves identically to `\x1b[` on iTerm2/xterm in 8-bit mode. A misbehaving MCP child could write `\u{9b}2J` and repaint the screen — exactly the smuggling vector the commit message claimed to close. Filter now blocks C0 controls (except `\t`), DEL, and the full C1 range. - **#2 HIGH — MCP stderr was silently dropped at default verbosity**: emit was `tracing::info!`, but dirge's default EnvFilter is `warn,rig=off`. Users diagnosing MCP server panics or init errors saw nothing — real regression vs the old `Stdio::inherit()`. Raised to `tracing::warn!` so it surfaces on the default config. - **#3 HIGH — `parse_ddg_html` panicked on truncated input**: `&html[tag_start..abs_start + 32]` blew up when `abs_start + 32 > html.len()` or landed mid-codepoint. Rewrote the scanner to anchor on `<a ` tags and walk only within bounded slices via `tag_end.min(html.len())`. Added regression test for the truncated case. - **#5 MEDIUM — MCP stderr forwarder had no per-line cap**: `BufReader::lines()` buffers until `\n`. A buggy child writing a GB without newline would OOM dirge. Replaced with a manual read loop, 16 KiB per-line cap, emit `…[truncated]` past the cap and skip until next `\n`. - **#6 MEDIUM — provider rotation race on first call**: two concurrent first-callers both saw `AtomicU8 = 0`, both rolled a fresh entropy pick, both stored. Last writer won — inconsistent contract. Switched to `compare_exchange` from 0 to candidate; loser re-reads the winner's value. - **#7 MEDIUM — both-providers-fail error masked secondary + DDG errors**: only `primary_err` was returned. Now concatenates all three failures so the user can diagnose without chasing the wrong cause. - **#8 MEDIUM — `parse_ddg_html` false-positives on substring match**: previously matched `class="result__a"` anywhere in the HTML, including inside `<script>` blocks or quoted text. Walked backward via `rfind("<a ")` could grab an unrelated anchor. Now anchors on `<a ` first and inspects the tag's attributes — proper containment check. - **#9 MEDIUM — DDG snippet control bytes flowed into LLM prompt**: `strip_tags_and_decode` decoded entities but didn't filter ESC / C1 controls. A malicious or mojibake search result could ship ANSI styling into the agent's context. Added control-byte filter to the decoder's output pass. - **#10 LOW — `PARALLEL_API_KEY` read per-call**: was `std::env::var` inside `call`, inconsistent with how `EXA_API_KEY` was captured at construction. Moved to `WebSearchTool::new`. - **#11 LOW — whitespace-only key passed empty-filter**: `EXA_API_KEY=" "` produced a malformed `?exaApiKey=%20%20` URL. Now trims keys at construction. - **#12 LOW — tool description out of date**: still mentioned Exa-only + DDG fallback, missed Parallel.ai rotation and the keyless default. Rewritten to reflect the actual contract. - **#13 LOW — `urlencode_query` renamed to `percent_encode`**: the function is a generic percent-encoder (RFC 3986 unreserved set), not a form-encoder. Misleading name. - **#14 DESIGN — no tests for new code paths**: added 11 regression tests covering ddg parser bounds + happy path + anti-script-block, control-byte filter, key trim, MCP response parser (plain JSON + SSE + malformed), DDG redirect unwrap, percent encoding, provider env override. - **#15 LOW — bot-identifying DDG User-Agent**: swapped `compatible; dirge-agent/1.0` for a real Firefox 133 UA. DDG aggressively rate-limits identifiable scrapers. 695 tests pass (684 + 11 new); fmt clean.
yogthos
pushed a commit
that referenced
this pull request
May 22, 2026
…h_bg width, sanitization Correctness + security review of `dc21de7`/`cd701a8` flagged 15 findings. Addressing all 15. - **#1 HIGH — startup race: MCP forwarders fired before `install()`**. `connect_all` spawns stderr forwarders in `main` BEFORE `run_interactive` reached the old `install()` call. Lines emitted during MCP-server handshake hit `sender() == None` and were silently dropped. Moved `install()` to the very top of `main()` so the channel is live by the time any forwarder starts. Split the API: `install()` (creates channel) + `take_receiver()` (UI loop claims the rx). - **#2 HIGH — orphaned-sender footgun on UI restart**. `OnceLock` meant a re-entry could never replace the sender; producers holding clones would send into a dead channel forever. Switched to `RwLock<Option<Sender>>`. Producers also self-heal: when `try_send` returns Err because the receiver was dropped, `notify_send` clears the slot so subsequent producers see `None` and skip. - **#3 HIGH — `row_with_bg` was still char-count-based**. The refactor claim was "unifies display-width vs char-count" but the bg-tinted variant still used `chars().count()`. A diff row with CJK / emoji drifted the right border. Now uses the same display-width budget as the plain `row`. Regression test `row_with_bg_width_invariant` pins it. - **#4 HIGH — unbounded channel + no backpressure → OOM**. A buggy / hostile MCP child spamming stderr would grow the queue unboundedly. Switched to `mpsc::channel(1024)` (bounded) with `try_send` so the producer drops on overflow rather than unboundedly queuing. Test `bounded_channel_drops_on_full` pins the contract. - **#5 MEDIUM — multi-colon MCP tool names**. `splitn(3, ':')` on `mcp_tool:server:do:thing` parsed correctly but the comment explanation was off. Clarified; behavior unchanged (the wildcarded server pattern is the desired semantics). - **#6 MEDIUM — mcp_tool umbrella check case-sensitive**. `umbrella == "mcp_tool"` would miss `MCP_TOOL:…` if a future caller surfaces uppercase. Switched to `eq_ignore_ascii_case`. - **#7 MEDIUM — receiver-side sanitization for ALL Notification variants**. MCP variant was pre-sanitized at the producer, but Info/Warn/Error had no producer-side contract. Adding receiver-side `ansi::strip_controls(KEEP_NEWLINE)` makes the rule un-bypassable: nothing reaches `write_line` carrying escape bytes regardless of how careful a future producer is. - **#8 MEDIUM — websearch `KEEP_BOTH` + `\n` broke chamber border**. Tabs survived into chamber rows where they interacted poorly with the wrap math. Switched to `KEEP_NEWLINE` and replace `\t` with single space. - **#9 MEDIUM — whitespace-only MCP lines dropped**. The blank-line collapse used `trim().is_empty()` which also ate legitimate indented continuation lines. Now uses `is_empty()` post-sanitize. - **#10 LOW — `top()` with empty title rendered `╭─ ─…─╮`** (two spaces with no glyph between). Empty title now matches the bottom-border shape `╭{horizontals}╮`. Test pins it. - **#11 LOW — `expand_tabs` precondition undocumented**. Added comment that input should be control-byte free; callers must sanitize first. - **#12 LOW — `BoxBuilder::row("a\nb")` produced one row containing a literal `\n`**. Now splits on `\n` and emits one row per logical line. Test `builder_splits_embedded_newlines` pins it. - **#13 LOW — `BoxBuilder` had no labelled-row variant**. Added `row_labelled(label, sep, value)` that indents wrapped tails under the value column. Mirrors the alert chamber's `labelled_rows` shape so a future alert migration to BoxBuilder is unblocked. Test pins continuation indent. - **#14 LOW — `strip_controls` allocated on no-op path**. Fast path returns the input unchanged when no chars would be filtered. - **#15 DESIGN — sender caching deferred**. Per-call `sender()` is the right semantics for the orphan-detection case (#2); caching would skip the slot-clear behavior. Kept as is. 8 new tests; 715 total. Two-test serialisation via TEST_GATE for the notification tests since they mutate global TX/RX_HOLDER state. fmt clean.
yogthos
pushed a commit
that referenced
this pull request
May 22, 2026
Three issues from the post-cutover code review against pi: **Bug #1**: stream.rs:186-194 — defensive fallback (stream closed without Done/Error) skipped emitting message_start / message_end. Pi at agent-loop.ts:359-366 emits both. Fix: route the fallback through `finalize()` so it follows the same emit path as Done/Error. Updated the existing test that documented the wrong behavior as "intentional Rust deviation" — it's now pi-faithful. **Bug #4**: integration.rs:411 — orphaned inner loop task. `spawn_loop_runner` spawned `run_agent_loop` as a NESTED `tokio::spawn`. A `task.abort()` on the outer task would kill it but leave the nested task running silently — tools could keep executing after the user thought they'd cancelled. Fix: collapse to `tokio::join!(loop_future, pump_future)` in the same outer task. Shared fate; outer abort drops both futures at their next .await. Tools that poll the AbortSignal still observe cancellation cooperatively. **Gap #3**: run.rs prepareNextTurn — pi at agent-loop.ts:229-238 rebuilds config with the new model / reasoning. We accepted the fields but silently ignored them. Surfacing a tracing warning per ignored swap so users wiring the hook know their change didn't take effect. Full fix requires the StreamFn to be a factory `Fn(Context) -> StreamFn` (so the loop can rebuild it on swap) — flagged for follow-up when a real consumer demands it. Items NOT addressed (documented in review): - #2 get_api_key receives empty string (no production caller) - #5/#6 timing / ordering changes (observable but not bugs) - #7-9 efficiency micro-optimizations - #10/#11 UI-side wiring + Agent.preamble defensiveness Gates: - cargo build (default) clean - cargo build --all-features clean - cargo test (default) 841 green (unchanged) - cargo fmt clean
yogthos
pushed a commit
that referenced
this pull request
May 27, 2026
…r dedup Two review findings from a clean-context audit of ca3bb42 / ba253b1. **Bug (MEDIUM): tool-call gap timeout penalized any chunk while a tool call was open** The prior implementation narrowed `effective_timeout` to 30s whenever `open_tool_calls` was non-empty. A provider that emits one ToolCallDelta then takes 25s emitting reasoning/text deltas (legitimate forward progress) would be killed at 30s — even though the model was making progress, just not on the tool call. Fix: track `last_chunk_at: Instant`. The gap budget for the next wait = `TOOL_CALL_GAP_TIMEOUT.saturating_sub(last_chunk_at.elapsed())`. Every chunk arrival (text, reasoning, tool-call delta, final ToolCall) refreshes `last_chunk_at`, so the gap timer only counts true silence — not gaps filled by other chunks. Regression test `gap_timeout_resets_on_interleaved_text_delta`: ToolCallDelta → 20s sleep → TextDelta → 20s sleep → TextDelta → done. Total elapsed 40s, but no single chunk gap exceeds 30s, so the gap timeout MUST NOT fire. Passes. **Cosmetic (INFO): counter inflation on multi-null-strip calls** `strip_null_optionals` pushes `RepairKind::NullStripped` once per removed key. A single tool call with 3 null fields was registering `null_stripped += 3`. The UI summary then read "repaired 3 input(s): 3 null-strip" for what was one call with three strips. Fix: dedupe `rr.kinds` per-call inside the counter-record loop in `tools.rs`. The full kinds vec still flows to the tracing event for per-call detail; the aggregate counter now measures "tool calls touched" which is the user-meaningful metric. Added `Hash` derive on `RepairKind` to support the dedupe HashSet. Review findings not addressed (deferred / not bugs): - #2 apply_patch hint phrasing (low; the shared hint is defensible since each operations[].path IS absolute) - #4-6 open_tool_calls lifecycle on stream-end / final-without- delta paths (all confirmed correct in original impl) - #9 additional test coverage (would catch nothing new; the new test exercises the previously-buggy interleave path) - #10 missing hints for task/skill/memory/etc. (defensible — those tools don't take path args) - #11 Debug-formatted validation_errors (minor; structured-log consumers can normalize) Full test suite: 1720 pass / 0 fail / 0 ignored (was 1719).
allen-munsch
pushed a commit
to allen-munsch/dirge
that referenced
this pull request
Jun 3, 2026
…ge-code#11) * feature: expand input box to show full multi-line draft Previously the input bar was always a single row: typing across multiple logical lines kept only the cursor's line visible, with a '[N lines]' indicator on the status row. Hard to review a draft. Now the bottom panel grows from 1 row up to MAX_INPUT_VISIBLE_LINES (8) as the user adds newlines. Continuation rows show a dimmer '·' prompt so the user can see which row is row-0 of the draft. Beyond 8 lines the panel stays capped and scrolls internally to keep the cursor row visible; the status line shows '[N lines, K hidden]'. The chat viewport above shrinks by the same amount via the new 'input_rows' field on Renderer; visible_lines() / content_row() / render_viewport() all consult it. Also added Ctrl+J as a third newline trigger alongside Shift+Enter / Meta+Enter. Many terminals don't deliver Shift+Enter as a distinct keystroke, but Ctrl+J is universally delivered as a control character on Unix terminals. * fix: picker overlay anchors above the multi-line input Two layout bugs in the multi-line input feature: - FilePicker drew at `rows.saturating_sub(3)`, hardcoding the pre-multi-line assumption that the input bar sits at `rows - 2`. When the user typed several lines and triggered the picker on a later line, the picker overlapped with the input rows. Picker now takes an `input_top: u16` parameter and anchors one row above whatever the renderer reports as the input top. - Token counter could overdraw the rightmost characters of the cursor's line on a wide screen with a long line. Reserved a 12-col band at the right edge for the counter by reducing `visible_width` globally, so input rendering and counter rendering never collide. * fix: normalize \r and \r\n in pastes before line-count check When a paste source delivered \r\n or \r-only line endings, the matches('\n').count() check returned 0, the threshold (>= 4 lines) was never met, and the raw text — including embedded \r chars — got inserted into the buffer. The terminal then rendered those \r chars as carriage-returns (cursor-to-col-0), making subsequent text overwrite earlier text and producing a garbled single 'line' on screen. Normalize \r\n → \n and any remaining \r → \n before stripping PASTE_MARK and counting lines. Multi-line pastes now collapse correctly regardless of which line ending the source used. * fix: yank after submit no longer drops paste body silently Repro: paste 5 lines → Ctrl+U kills the marker into kill-ring → Enter on empty buffer (still ran pastes.clear()) → Ctrl+Y yanks marker bytes back into the buffer → submit. The marker bytes referenced a now-empty pastes vec, so expanded() silently omitted them and the agent received only the surrounding text. The user saw '[N lines pasted]' on screen but their paste was gone in transit. Fix: before clearing self.pastes on submit, walk the kill ring and expand any marker blocks in-place to their stored bodies. Kill-ring entries become self-contained raw text and survive the pastes wipe. Also factored expand_with_pastes() out as a free helper so both expanded() and the kill-ring flatten share the same logic. Added 10 robustness tests covering: paste at start / middle, two distinct pastes, Home/End across markers, multi-line Up/Down with a marker on a middle row, empty paste no-op, paste-of-only-PASTE_MARK no-op, Delete-at-start-of-marker removes whole block, multibyte chars adjacent to a marker, and the yank-after-submit regression itself. 184 tests passing. --------- Co-authored-by: Yogthos <yogthos@gmail.com>
allen-munsch
pushed a commit
to allen-munsch/dirge
that referenced
this pull request
Jun 3, 2026
…aths (dirge-code#111) 23 audit findings verified REAL via parallel agent verification + cross-check against opencode/pi reference patterns. Shipping the 10 most concrete fixes here; the rest go in a follow-up docs/test batch. ## Security - **dirge-code#9 bash quote_aware_split missed bare `|`** — `safe_cmd | rm -rf /` was treated as one segment; only the LHS got permission-checked. Pipe RHS rode in unchecked under the fallback (non-semantic-bash) path. Added single-byte `|` split after `||` is matched. The tree-sitter path was already correct. - **#4 read.rs no binary detection** — feeding a PDF/ELF/.pyc into the LLM as lossy UTF-8 wasted tokens and confused the model. Ported opencode `read.ts:153-198`: reject by extension list (zip/exe/.o/.pdf/.png/etc.), then sniff the first 4 KiB — null byte = binary, >30% non-printable = binary. Clear error message tells the agent to use bash + xxd instead. ## Correctness - **#2 skill override inverted** — README contract: "Project skills override global skills by name". Code used `map.entry(name).or_insert(skill)` which KEEPS the first (global) value and silently drops project overrides. Switch to `map.insert` (last-write-wins) since globals iterate first and project iterates second. - **dirge-code#37 skill empty name** — frontmatter `name:` with empty value parsed to "", which then matched any `skill ""` call silently. Fall back to directory name when frontmatter name is empty/whitespace-only. - **#1 session_tree.janet hook never fired** — plugin defined `(defn on-message ...)` but `(def hooks [])` was empty AND the hook name doesn't exist (dirge uses `on-message-update`). `/label` was permanently broken ("no entry yet"). Fix: rename to `on-message-update` + register in hooks vector. - **dirge-code#7 workflow.janet hooks vector missing entries** — plugin defined `workflow-on-tool-end`, `-on-error`, `-on-complete` but only registered the first four hook names. Three hooks were dead. Added them. - **dirge-code#26 MCP malformed JSON silently empty args** — `serde_json::from_str(&args).unwrap_or_default()` turned bad JSON into None, sending the server an empty argument set. Server then errored with confusing "missing required field" instead of dirge surfacing the actual parse error. Now returns ToolError with the parse error message + first 200 chars of the offending JSON. - **dirge-code#22 /prompt default unreachable** — README documents `default` as a built-in prompt (prompts/default.md exists), but `/prompt default` was intercepted as a magic "clear" keyword. If `default` is registered in `context.prompts`, the new branch falls through to the normal name-lookup. Only acts as clear-keyword when no `default` prompt is present (legacy fallback). - **dirge-code#23 /allow add accepted invalid tools** — typo `/allow add bsah ...` silently created an inert rule the user couldn't debug. Added a known-tools whitelist matching PermissionConfig fields; unknown tools error with the valid list. ## Performance + correctness - **dirge-code#11 grep loaded whole files into memory** — no size cap meant a 9MB file got fully buffered. Added 10 MiB per-file cap via metadata pre-check. - **dirge-code#15 Python dunder methods marked non-exported** — `!name.starts_with('_')` treats `__init__`/`__call__`/etc. as private, even though they're Python's standard public protocol. Recognize `__x__` dunder pattern as exported. ## UI - **dirge-code#36 panel char-count truncation vs Unicode width** — panel truncation used `chars().count()` while wide emoji and CJK take 2 cells. A status line with an emoji overflowed the right border by one cell. Switched to `UnicodeWidthStr::width` for both truncation and padding. ## Tests 4 new regression tests: - `test_is_binary_extension_known` — pdf/tgz/.so/.jpg/.pyc - `test_is_binary_content_null_byte` — null byte trigger, UTF-8 Japanese stays clean, all-non-printable triggers - `quote_aware_split_splits_on_bare_pipe` — pipe security - `quote_aware_split_or_and_pipe_distinct` — `a || b | c` produces 3 segments, not 2 725 plugin / 599 default pass. All build profiles clean. ## Verified false positives (not fixed, audit was wrong) - #3 cache.rs clear() race — generation counter gating in `get` makes stale entries invisible, no correctness impact. - dirge-code#17 DeepSeek auto-detect priority — auto-detect only fires when env vars present; default-default is still OpenRouter. - dirge-code#19 semantic tools in collision filter — semantic tools added separately, can't be shadowed by MCP. - dirge-code#20 glob global gitignore — intentionally disabled to match grep behavior. - dirge-code#28 nearest_root blocking std::fs — function doesn't exist in current code. - dirge-code#32 ReadArgs.path vs GrepArgs.path — semantically different by design (file vs dir), documented in schema. - dirge-code#33 install_plugin_providers dead-without-feature — gated with explicit `#[cfg_attr(not(feature), allow(dead_code))]`. - dirge-code#34 websearch double-gated — config + API key serve distinct purposes (enable + auth). ## Deferred to follow-up batches Docs-only fixes (dirge-code#6 CONFIG.md tools, dirge-code#12 temperature, dirge-code#13 --api-key, dirge-code#14 acp_host/port), MCP/LSP architecture (dirge-code#8, dirge-code#25, dirge-code#27), test gaps (dirge-code#38-40), and lower-priority polish — all in a follow-up PR. Co-authored-by: Yogthos <yogthos@gmail.com>
allen-munsch
pushed a commit
to allen-munsch/dirge
that referenced
this pull request
Jun 3, 2026
…rf cache Self-review of the last 4 commits flagged 11 findings. Addressing all in one batch. - **#1 HIGH — leading whitespace dropped on first row of soft_wrap**: `current.is_empty()` at row start unconditionally dropped `token.leading_ws`, including on the first row of a logical line. Option lines `" ▶ label …"` lost their ` ` margin; the green ` allowed …` confirmation lost its indent too. First-row branch now preserves leading_ws (with a ws-overflow fallback to keep the token). - **#2 HIGH — drain_events early-broke before reader quiesced**: the Ok(false) shortcut fired on the first quiet poll, which was often "the background reader currently holds crossterm's internal mutex" rather than "terminal is quiet". A delayed OSC 11 / DA1 response could still escape past our drain. Now requires at least one observed event before the Ok(false) shortcut; honors the full budget otherwise. - **#3 HIGH — MODIFIED section clipped its own bottom border at `available == 4`**: row_budget=1, footer + 1 file = 2 items, +3 frame rows = 5 total in a 4-row budget. draw_panel clipped the `╰────╯`. Bumped MIN_MOD_SECTION_ROWS from 4 → 5 so the bottom border is always painted. - **#4 MEDIUM — `\r` not stripped from CRLF input**: Windows / some-MCP tool output left `\r` in tokens, producing terminal redraw artifacts. `soft_wrap` now strips a trailing `\r` per logical line. - **dirge-code#5 MEDIUM — Show cursor on alt screen was a no-op**: `Show` was issued while still on the alt screen; `LeaveAlternateScreen` restores the main screen's saved DECTCEM state, discarding the Show. Moved Show to AFTER LeaveAlternateScreen + disable_raw_mode. - **dirge-code#6 MEDIUM — recent(256) clones + locks on every redraw**: panel redraws on every streamed token. Added `modified::version()` monotonic counter (bumped on mark / clear); panel-side cache in `panel_modified_cached` keyed by (version, cwd) skips the lock + 256-PathBuf clone + path-strip when nothing changed. - **dirge-code#7 MEDIUM — break_long_token could emit wide-glyph row > budget at max_width<2**: floored max_width at 2 inside soft_wrap. A 1-cell terminal is unusable anyway; this just removes a sharp edge. - **dirge-code#8 MEDIUM — all-whitespace first row collapsed to empty**: same root cause as #1, fixed by the same change. Indented blank separators now preserve their indentation. - **dirge-code#9 LOW — `allowed …` confirmation flush against alert `╰─╯`**: added a blank-line breathing row before the green confirmation so the alert's bottom border and the confirmation don't read as one block. - **dirge-code#10 LOW — head_w used chars().count() not display width**: switched to `UnicodeWidthStr::width(head)` so future wide-glyph markers won't under-pad the continuation indent. - **dirge-code#11 LOW — single-select marker width inconsistent**: cursor marker was `▶` (w=1), non-cursor was ` ` (w=2), so wrapped tails of adjacent options drifted by one column. Cursor marker padded to `▶ ` so all markers in a question share display width. 5 new tests: - preserves_leading_whitespace_on_first_row - strips_carriage_returns_from_crlf_input - wide_glyph_respects_max_width_at_floor - preserves_leading_whitespace_only_line - version_bumps_on_mark_and_clear 651 tests pass; fmt clean.
allen-munsch
pushed a commit
to allen-munsch/dirge
that referenced
this pull request
Jun 3, 2026
…nds, error masking Adversarial review of `349afb6`/`b9de3d5`/`2fac609` flagged 14 findings. Addressing all. - **#1 HIGH — C1 controls bypassed MCP stderr sanitizer**: the original filter was `b == 0x09 || (0x20..0x7f).contains(&b) || b >= 0x80`. That second clause let through every non-ASCII byte including the C1 control range (U+0080..=U+009F). In particular U+009B is single-byte CSI, behaves identically to `\x1b[` on iTerm2/xterm in 8-bit mode. A misbehaving MCP child could write `\u{9b}2J` and repaint the screen — exactly the smuggling vector the commit message claimed to close. Filter now blocks C0 controls (except `\t`), DEL, and the full C1 range. - **#2 HIGH — MCP stderr was silently dropped at default verbosity**: emit was `tracing::info!`, but dirge's default EnvFilter is `warn,rig=off`. Users diagnosing MCP server panics or init errors saw nothing — real regression vs the old `Stdio::inherit()`. Raised to `tracing::warn!` so it surfaces on the default config. - **#3 HIGH — `parse_ddg_html` panicked on truncated input**: `&html[tag_start..abs_start + 32]` blew up when `abs_start + 32 > html.len()` or landed mid-codepoint. Rewrote the scanner to anchor on `<a ` tags and walk only within bounded slices via `tag_end.min(html.len())`. Added regression test for the truncated case. - **dirge-code#5 MEDIUM — MCP stderr forwarder had no per-line cap**: `BufReader::lines()` buffers until `\n`. A buggy child writing a GB without newline would OOM dirge. Replaced with a manual read loop, 16 KiB per-line cap, emit `…[truncated]` past the cap and skip until next `\n`. - **dirge-code#6 MEDIUM — provider rotation race on first call**: two concurrent first-callers both saw `AtomicU8 = 0`, both rolled a fresh entropy pick, both stored. Last writer won — inconsistent contract. Switched to `compare_exchange` from 0 to candidate; loser re-reads the winner's value. - **dirge-code#7 MEDIUM — both-providers-fail error masked secondary + DDG errors**: only `primary_err` was returned. Now concatenates all three failures so the user can diagnose without chasing the wrong cause. - **dirge-code#8 MEDIUM — `parse_ddg_html` false-positives on substring match**: previously matched `class="result__a"` anywhere in the HTML, including inside `<script>` blocks or quoted text. Walked backward via `rfind("<a ")` could grab an unrelated anchor. Now anchors on `<a ` first and inspects the tag's attributes — proper containment check. - **dirge-code#9 MEDIUM — DDG snippet control bytes flowed into LLM prompt**: `strip_tags_and_decode` decoded entities but didn't filter ESC / C1 controls. A malicious or mojibake search result could ship ANSI styling into the agent's context. Added control-byte filter to the decoder's output pass. - **dirge-code#10 LOW — `PARALLEL_API_KEY` read per-call**: was `std::env::var` inside `call`, inconsistent with how `EXA_API_KEY` was captured at construction. Moved to `WebSearchTool::new`. - **dirge-code#11 LOW — whitespace-only key passed empty-filter**: `EXA_API_KEY=" "` produced a malformed `?exaApiKey=%20%20` URL. Now trims keys at construction. - **dirge-code#12 LOW — tool description out of date**: still mentioned Exa-only + DDG fallback, missed Parallel.ai rotation and the keyless default. Rewritten to reflect the actual contract. - **dirge-code#13 LOW — `urlencode_query` renamed to `percent_encode`**: the function is a generic percent-encoder (RFC 3986 unreserved set), not a form-encoder. Misleading name. - **dirge-code#14 DESIGN — no tests for new code paths**: added 11 regression tests covering ddg parser bounds + happy path + anti-script-block, control-byte filter, key trim, MCP response parser (plain JSON + SSE + malformed), DDG redirect unwrap, percent encoding, provider env override. - **dirge-code#15 LOW — bot-identifying DDG User-Agent**: swapped `compatible; dirge-agent/1.0` for a real Firefox 133 UA. DDG aggressively rate-limits identifiable scrapers. 695 tests pass (684 + 11 new); fmt clean.
allen-munsch
pushed a commit
to allen-munsch/dirge
that referenced
this pull request
Jun 3, 2026
…h_bg width, sanitization Correctness + security review of `ea042b1`/`524c90c` flagged 15 findings. Addressing all 15. - **#1 HIGH — startup race: MCP forwarders fired before `install()`**. `connect_all` spawns stderr forwarders in `main` BEFORE `run_interactive` reached the old `install()` call. Lines emitted during MCP-server handshake hit `sender() == None` and were silently dropped. Moved `install()` to the very top of `main()` so the channel is live by the time any forwarder starts. Split the API: `install()` (creates channel) + `take_receiver()` (UI loop claims the rx). - **#2 HIGH — orphaned-sender footgun on UI restart**. `OnceLock` meant a re-entry could never replace the sender; producers holding clones would send into a dead channel forever. Switched to `RwLock<Option<Sender>>`. Producers also self-heal: when `try_send` returns Err because the receiver was dropped, `notify_send` clears the slot so subsequent producers see `None` and skip. - **#3 HIGH — `row_with_bg` was still char-count-based**. The refactor claim was "unifies display-width vs char-count" but the bg-tinted variant still used `chars().count()`. A diff row with CJK / emoji drifted the right border. Now uses the same display-width budget as the plain `row`. Regression test `row_with_bg_width_invariant` pins it. - **#4 HIGH — unbounded channel + no backpressure → OOM**. A buggy / hostile MCP child spamming stderr would grow the queue unboundedly. Switched to `mpsc::channel(1024)` (bounded) with `try_send` so the producer drops on overflow rather than unboundedly queuing. Test `bounded_channel_drops_on_full` pins the contract. - **dirge-code#5 MEDIUM — multi-colon MCP tool names**. `splitn(3, ':')` on `mcp_tool:server:do:thing` parsed correctly but the comment explanation was off. Clarified; behavior unchanged (the wildcarded server pattern is the desired semantics). - **dirge-code#6 MEDIUM — mcp_tool umbrella check case-sensitive**. `umbrella == "mcp_tool"` would miss `MCP_TOOL:…` if a future caller surfaces uppercase. Switched to `eq_ignore_ascii_case`. - **dirge-code#7 MEDIUM — receiver-side sanitization for ALL Notification variants**. MCP variant was pre-sanitized at the producer, but Info/Warn/Error had no producer-side contract. Adding receiver-side `ansi::strip_controls(KEEP_NEWLINE)` makes the rule un-bypassable: nothing reaches `write_line` carrying escape bytes regardless of how careful a future producer is. - **dirge-code#8 MEDIUM — websearch `KEEP_BOTH` + `\n` broke chamber border**. Tabs survived into chamber rows where they interacted poorly with the wrap math. Switched to `KEEP_NEWLINE` and replace `\t` with single space. - **dirge-code#9 MEDIUM — whitespace-only MCP lines dropped**. The blank-line collapse used `trim().is_empty()` which also ate legitimate indented continuation lines. Now uses `is_empty()` post-sanitize. - **dirge-code#10 LOW — `top()` with empty title rendered `╭─ ─…─╮`** (two spaces with no glyph between). Empty title now matches the bottom-border shape `╭{horizontals}╮`. Test pins it. - **dirge-code#11 LOW — `expand_tabs` precondition undocumented**. Added comment that input should be control-byte free; callers must sanitize first. - **dirge-code#12 LOW — `BoxBuilder::row("a\nb")` produced one row containing a literal `\n`**. Now splits on `\n` and emits one row per logical line. Test `builder_splits_embedded_newlines` pins it. - **dirge-code#13 LOW — `BoxBuilder` had no labelled-row variant**. Added `row_labelled(label, sep, value)` that indents wrapped tails under the value column. Mirrors the alert chamber's `labelled_rows` shape so a future alert migration to BoxBuilder is unblocked. Test pins continuation indent. - **dirge-code#14 LOW — `strip_controls` allocated on no-op path**. Fast path returns the input unchanged when no chars would be filtered. - **dirge-code#15 DESIGN — sender caching deferred**. Per-call `sender()` is the right semantics for the orphan-detection case (#2); caching would skip the slot-clear behavior. Kept as is. 8 new tests; 715 total. Two-test serialisation via TEST_GATE for the notification tests since they mutate global TX/RX_HOLDER state. fmt clean.
allen-munsch
pushed a commit
to allen-munsch/dirge
that referenced
this pull request
Jun 3, 2026
Three issues from the post-cutover code review against pi: **Bug #1**: stream.rs:186-194 — defensive fallback (stream closed without Done/Error) skipped emitting message_start / message_end. Pi at agent-loop.ts:359-366 emits both. Fix: route the fallback through `finalize()` so it follows the same emit path as Done/Error. Updated the existing test that documented the wrong behavior as "intentional Rust deviation" — it's now pi-faithful. **Bug #4**: integration.rs:411 — orphaned inner loop task. `spawn_loop_runner` spawned `run_agent_loop` as a NESTED `tokio::spawn`. A `task.abort()` on the outer task would kill it but leave the nested task running silently — tools could keep executing after the user thought they'd cancelled. Fix: collapse to `tokio::join!(loop_future, pump_future)` in the same outer task. Shared fate; outer abort drops both futures at their next .await. Tools that poll the AbortSignal still observe cancellation cooperatively. **Gap #3**: run.rs prepareNextTurn — pi at agent-loop.ts:229-238 rebuilds config with the new model / reasoning. We accepted the fields but silently ignored them. Surfacing a tracing warning per ignored swap so users wiring the hook know their change didn't take effect. Full fix requires the StreamFn to be a factory `Fn(Context) -> StreamFn` (so the loop can rebuild it on swap) — flagged for follow-up when a real consumer demands it. Items NOT addressed (documented in review): - #2 get_api_key receives empty string (no production caller) - dirge-code#5/dirge-code#6 timing / ordering changes (observable but not bugs) - dirge-code#7-9 efficiency micro-optimizations - dirge-code#10/dirge-code#11 UI-side wiring + Agent.preamble defensiveness Gates: - cargo build (default) clean - cargo build --all-features clean - cargo test (default) 841 green (unchanged) - cargo fmt clean
allen-munsch
pushed a commit
to allen-munsch/dirge
that referenced
this pull request
Jun 3, 2026
…r dedup Two review findings from a clean-context audit of 7dc35f9 / 32b439e. **Bug (MEDIUM): tool-call gap timeout penalized any chunk while a tool call was open** The prior implementation narrowed `effective_timeout` to 30s whenever `open_tool_calls` was non-empty. A provider that emits one ToolCallDelta then takes 25s emitting reasoning/text deltas (legitimate forward progress) would be killed at 30s — even though the model was making progress, just not on the tool call. Fix: track `last_chunk_at: Instant`. The gap budget for the next wait = `TOOL_CALL_GAP_TIMEOUT.saturating_sub(last_chunk_at.elapsed())`. Every chunk arrival (text, reasoning, tool-call delta, final ToolCall) refreshes `last_chunk_at`, so the gap timer only counts true silence — not gaps filled by other chunks. Regression test `gap_timeout_resets_on_interleaved_text_delta`: ToolCallDelta → 20s sleep → TextDelta → 20s sleep → TextDelta → done. Total elapsed 40s, but no single chunk gap exceeds 30s, so the gap timeout MUST NOT fire. Passes. **Cosmetic (INFO): counter inflation on multi-null-strip calls** `strip_null_optionals` pushes `RepairKind::NullStripped` once per removed key. A single tool call with 3 null fields was registering `null_stripped += 3`. The UI summary then read "repaired 3 input(s): 3 null-strip" for what was one call with three strips. Fix: dedupe `rr.kinds` per-call inside the counter-record loop in `tools.rs`. The full kinds vec still flows to the tracing event for per-call detail; the aggregate counter now measures "tool calls touched" which is the user-meaningful metric. Added `Hash` derive on `RepairKind` to support the dedupe HashSet. Review findings not addressed (deferred / not bugs): - #2 apply_patch hint phrasing (low; the shared hint is defensible since each operations[].path IS absolute) - #4-6 open_tool_calls lifecycle on stream-end / final-without- delta paths (all confirmed correct in original impl) - dirge-code#9 additional test coverage (would catch nothing new; the new test exercises the previously-buggy interleave path) - dirge-code#10 missing hints for task/skill/memory/etc. (defensible — those tools don't take path args) - dirge-code#11 Debug-formatted validation_errors (minor; structured-log consumers can normalize) Full test suite: 1720 pass / 0 fail / 0 ignored (was 1719).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Multi-line input was already wired into `InputEditor`, but the renderer kept a single-row input bar — only the current logical line of a draft was visible, with a `[N lines]` indicator on the status row. Hard to review a paragraph you're composing.
This PR makes the bottom panel grow with the draft (Claude Code-style):
Also added Ctrl+J as a third newline trigger alongside Shift+Enter / Meta+Enter. Many terminals (default macOS Terminal, vanilla xterm, etc.) don't deliver Shift+Enter as a distinct keystroke; Ctrl+J is universal.
Test plan