Add question tool — agent asks user for clarification mid-execution by yogthos · Pull Request #13 · dirge-code/dirge

yogthos · 2026-05-19T16:40:05Z

Summary

Adds a question tool the agent calls to ask the user one or more structured questions (label/description options, optional multi-select, optional custom free-text). Channel-based dispatch from tool → UI via mpsc<QuestionRequest> + oneshot reply, mirroring the existing ask permission flow.
UI handler in run_interactive renders an interactive picker (↑↓ navigate, Enter select, Space toggle for multi, Esc reject) and pipes the answer back as tool output.
Subagents are not given the tool (parent only) to avoid nested prompts.

Test plan

cargo build
Tool unit tests (single-select, multi-select, rejection, multiple questions)
Manual: agent invokes question, picker renders, arrow keys redraw in place (no duplicate option blocks), Enter/Esc both round-trip correctly

- Add QuestionTool that blocks until user answers via oneshot channel - Add question UI in interactive loop: arrow-key navigation, single/multi select, custom text input - Wire question_tx/question_rx channels through builder, provider, and main - Add .gitignore entry for .DS_Store - Tests: single-select, multi-select, reject, multiple questions, tool definition

…ppend Option lines and custom text input now use replace_from to overwrite prior rendering instead of appending on each keystroke. Previously each arrow key painted another full copy of the options into the viewport.

The id field was generated via Uuid::new_v4() and marked #[allow(dead_code)] but never read. Dropped along with the uuid::Uuid import to simplify the request shape.

Adds targeted regression tests for each bug fix and behavioral guarantee introduced across the eight feature PRs. Total: 205 → 254 tests (+49). - background-tasks: 0 → 11 tests. Cover get()-evicts-on-read, truncation-by-chars on Completed/Failed, running tasks NOT evicted, shared state across clones (Arc<Mutex>), no-op on missing. - apply-patch: 8 → 16 tests. Multi-op stop-on-failure with prior ops staying applied, ambiguous-update rejection, 1MB create cap (incl. off-by-one), nested-dir creation, PatchOp deserialization. - glob-tool: 5 → 11 tests. Real-FS integration via TempTree: walks files, empty result returns empty string (not 'no files matched'), mtime sort with explicit path != CWD, respects .gitignore, regex metachars escaped, * doesn't cross directory boundaries. - plan-tools: 5 → 8 tests. Source-level regression test that plan_exit has no fs::write / PLAN.md side-effects, channel-unavailable + reply-dropped error paths. - web-tools: 7 → 12 / 4 → 7 tests. Wrap-width regression (long paragraph must wrap), input validation (empty/too many URLs), partial-field formatting in search results, 500-char snippet cap. - task-status: 6 → 9 tests. Completed task evicts after one read, wait=true returns on Failed, wait=true on missing errors promptly (bounded timeout guard against infinite-loop regression). - question-tool: 5 → 10 tests. Header → markdown ## heading, channel + reply error paths, default custom=true, multi-select comma-join. - agent-reminders: 0 → 5 tests. Extracted append_mode_reminder() pure fn so the reminder injection is unit-testable. Covers plan/review/ code modes, PLAN.md gate on code mode, unknown prompts pass through unchanged, section-separator prefix. Each regression test has a comment explaining the bug it guards against.

Track F-MEDIUM #13 from ROADMAP.md. ## Problem `/compress` reported `compressed N messages (saved ~M tokens)` where `M = tokens_before` (the messages it replaced) — not the NET savings after accounting for the summary's own token cost. If the LLM returned a summary longer than what it replaced (rare but possible with custom compress instructions that ask for too much detail), the user got a misleadingly positive message while context actually GREW. ## Fix After compress, estimate the summary's tokens via the existing `Session::estimate_tokens` heuristic. Compare against `tokens_before`. Two render branches: - `net_saved >= 0`: standard message now shows both the savings AND the summary cost so the user knows where the new tokens went: `compressed N messages (saved ~M tokens; summary uses Tt)`. - `net_saved < 0`: red error-color message naming the misfire explicitly: `compressed N messages but the summary (T1t) is LARGER than the replaced messages (T0t); net cost +Δt. Consider lowering keep_recent_tokens or refining compress instructions.` opencode (`compaction.ts:136-294`) validates the budget BEFORE issuing the LLM call. dirge validates AFTER because we don't know the summary's size until the LLM returns; the compress still proceeds (the shape is what the LLM expects), but the misfire is no longer silent. ## Tests No new unit test — the scenario requires a real LLM returning a pathologically long summary, which can't be deterministically forced in tests. Build + existing suite verify the format strings compile and don't regress. 671 pass. All build profiles clean. Co-authored-by: Yogthos <yogthos@gmail.com>

…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>

Follow-up to PR #111. Tier-2 items from the 23-bug audit batch: docs corrections and two small correctness/UX fixes. ## Docs - **#12 temperature** — CONFIG.md claimed "parsed but not currently applied". Actually applied since PR #105 with a clamp warning. Rewrote the cell. - **#13 --api-key** — flag existed but neither README nor CONFIG.md mentioned it. Added a Quick-start example noting the process-list visibility caveat. - **#14 acp_host/acp_port** — CONFIG.md documented both keys but the CLI flags were intentionally removed (stdio-only transport). Removed both from the keys table + ACP section. - **#6 tools** — `Config::tools` (per-tool enable map) was fully wired in code but undocumented. Added a row to the keys table covering `tools.websearch` and `tools.webfetch`. - **#21 find_callers** — README claimed "word-boundary regex" but the impl uses the tree-sitter symbol index. Updated to reflect actual behavior; the user-visible word-boundary semantics are preserved. ## Code - **#16 semantic index skip_dir** — `SymbolIndex::find_callers` filter had its own hardcoded `matches!(name, "node_modules" | "target" | ".git" | "__pycache__")` while the rest of the codebase uses `agent::tools::is_skip_dir`. Switched to the shared helper so future additions stay in lockstep. - **#18 context::load_file** — silently swallowed `read_to_string` errors via `.ok()`. A permission-denied AGENTS.md looked identical to a missing file. Now emits a stderr warning naming the path + reason; still returns None so callers' behavior is unchanged. 725 plugin / 599 default pass. All build profiles clean. ## Remaining audit items (deferred to feature work) - **#8 LSP no crash restart**: needs broken-pipe IO error handling + exponential backoff. Touches manager state machine. - **#10 task tool fire-and-forget**: needs timeout + cleanup coordination via JoinHandle tracking. - **#25 MCP no reconnection**: similar architectural concern to #8. - **#27 LSP didClose**: client lifecycle hook missing. - **#29 token estimation len/4**: needs per-provider usage extraction (Phase 6 work). - **#5 MCP shutdown**: rmcp Drop semantics need verification. - **#38/39/40 semantic test gaps**: get_symbol_body untested, list_symbols kind_filter untested, find_definition test vacuous. Sat down to add but each requires a fixture build. Together with PR #111 (10 code fixes), 17 of the 23 verified items are now shipped. Remaining 6 are architectural or test-infrastructure work better tackled as discrete PRs. Co-authored-by: Yogthos <yogthos@gmail.com>

…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.

…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.

…ser_rx writes Code review of `b508658`/`8b48bfd`/`f9285ad` flagged 15 findings, including one **active regression** I shipped: `write_outside_chamber` reused `close_tool_chamber_if_open` which always painted "⚠ tool denied · aborted · no result". So every notification arriving while a tool was in-flight would falsely brand that tool as denied. Fixed. Headline: of 9 tokio::select! arms, only 2 were using the new chokepoint. 3 others (question_rx, dialog_rx, plan_rx) carried the SAME X-inside-chamber bug the helper was built to eliminate. Migrated them. - **#4 HIGH (regression I shipped)**: split chamber-close into two variants: - `close_tool_chamber_abort` — paints the "⚠ tool denied" row + bottom border. Used by permission-deny / agent error / interjection / context-overflow paths (the tool is being actively rejected). - `close_tool_chamber_passive` — emits ONLY the bottom border. Used by `write_outside_chamber` (the tool isn't being denied; we just need to terminate the visual frame so notification text doesn't land inside). - `close_tool_chamber_if_open` kept as back-compat alias for the abort variant — existing call sites (4 of them, all in abort-shaped contexts) keep their previous behavior. - **#1 / #2 / #3 CRITICAL — three arms migrated**: - `question_rx` (3537): a `question` tool's chamber was open when the prompt header was painted; header + stem + option grid landed inside. - `dialog_rx` (3811): plugin `harness/confirm` / `harness/select` fires from inside on-tool-start hooks while a tool chamber is open; the dialog rendered inside. - `plan_rx` (3955): plan-switch prompt could be delivered while a tool chamber was open; prompt landed inside. - **#5 HIGH — user_rx interactive writes migrated**: - Ctrl+C interrupt msg (1135) - "copied selection" (1150) - Ctrl+X dropped-interjection trailer (1168) - "agent is busy" × 2 (1533, 1598) - **#7 MEDIUM — defense-in-depth sanitization**: `write_outside_chamber` now runs `strip_controls(KEEP_NEWLINE)` on `text` before writing. A future caller that forgets producer-side sanitization can't smuggle ANSI escapes. - **#12 LOW — notification amplification cap**: the bounded channel limits NOTIFICATIONS but not ROWS per notification. A single `Notification::McpLog` carrying 10k `\n`s would expand to 10k chamber rows. After 200 lines we truncate and emit a `[N more lines suppressed]` marker. - **#6 audit** revealed the 4 remaining manual sites (1984/2602/2713/2884) all ARE abort-shaped and correctly use the abort variant via the back-compat alias. No migration needed. - **#8 / #9 / #10 / #13 / #14 / #15** noted as design trade-offs or already verified clean. 3 new regression tests: - `close_passive_does_not_paint_abort_row` pins the new no-abort-label contract - `close_abort_paints_warning_and_bottom` pins the abort variant still emits 2 rows - existing `write_outside_chamber_closes_chamber_first` still passes; helper now uses passive close 718 tests pass (716 + 2 new); fmt clean.

Adds targeted regression tests for each bug fix and behavioral guarantee introduced across the eight feature PRs. Total: 205 → 254 tests (+49). - background-tasks: 0 → 11 tests. Cover get()-evicts-on-read, truncation-by-chars on Completed/Failed, running tasks NOT evicted, shared state across clones (Arc<Mutex>), no-op on missing. - apply-patch: 8 → 16 tests. Multi-op stop-on-failure with prior ops staying applied, ambiguous-update rejection, 1MB create cap (incl. off-by-one), nested-dir creation, PatchOp deserialization. - glob-tool: 5 → 11 tests. Real-FS integration via TempTree: walks files, empty result returns empty string (not 'no files matched'), mtime sort with explicit path != CWD, respects .gitignore, regex metachars escaped, * doesn't cross directory boundaries. - plan-tools: 5 → 8 tests. Source-level regression test that plan_exit has no fs::write / PLAN.md side-effects, channel-unavailable + reply-dropped error paths. - web-tools: 7 → 12 / 4 → 7 tests. Wrap-width regression (long paragraph must wrap), input validation (empty/too many URLs), partial-field formatting in search results, 500-char snippet cap. - task-status: 6 → 9 tests. Completed task evicts after one read, wait=true returns on Failed, wait=true on missing errors promptly (bounded timeout guard against infinite-loop regression). - question-tool: 5 → 10 tests. Header → markdown ## heading, channel + reply error paths, default custom=true, multi-select comma-join. - agent-reminders: 0 → 5 tests. Extracted append_mode_reminder() pure fn so the reminder injection is unit-testable. Covers plan/review/ code modes, PLAN.md gate on code mode, unknown prompts pass through unchanged, section-separator prefix. Each regression test has a comment explaining the bug it guards against.

Track F-MEDIUM dirge-code#13 from ROADMAP.md. ## Problem `/compress` reported `compressed N messages (saved ~M tokens)` where `M = tokens_before` (the messages it replaced) — not the NET savings after accounting for the summary's own token cost. If the LLM returned a summary longer than what it replaced (rare but possible with custom compress instructions that ask for too much detail), the user got a misleadingly positive message while context actually GREW. ## Fix After compress, estimate the summary's tokens via the existing `Session::estimate_tokens` heuristic. Compare against `tokens_before`. Two render branches: - `net_saved >= 0`: standard message now shows both the savings AND the summary cost so the user knows where the new tokens went: `compressed N messages (saved ~M tokens; summary uses Tt)`. - `net_saved < 0`: red error-color message naming the misfire explicitly: `compressed N messages but the summary (T1t) is LARGER than the replaced messages (T0t); net cost +Δt. Consider lowering keep_recent_tokens or refining compress instructions.` opencode (`compaction.ts:136-294`) validates the budget BEFORE issuing the LLM call. dirge validates AFTER because we don't know the summary's size until the LLM returns; the compress still proceeds (the shape is what the LLM expects), but the misfire is no longer silent. ## Tests No new unit test — the scenario requires a real LLM returning a pathologically long summary, which can't be deterministically forced in tests. Build + existing suite verify the format strings compile and don't regress. 671 pass. All build profiles clean. Co-authored-by: Yogthos <yogthos@gmail.com>

…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>

…de#112) Follow-up to PR dirge-code#111. Tier-2 items from the 23-bug audit batch: docs corrections and two small correctness/UX fixes. ## Docs - **dirge-code#12 temperature** — CONFIG.md claimed "parsed but not currently applied". Actually applied since PR dirge-code#105 with a clamp warning. Rewrote the cell. - **dirge-code#13 --api-key** — flag existed but neither README nor CONFIG.md mentioned it. Added a Quick-start example noting the process-list visibility caveat. - **dirge-code#14 acp_host/acp_port** — CONFIG.md documented both keys but the CLI flags were intentionally removed (stdio-only transport). Removed both from the keys table + ACP section. - **dirge-code#6 tools** — `Config::tools` (per-tool enable map) was fully wired in code but undocumented. Added a row to the keys table covering `tools.websearch` and `tools.webfetch`. - **dirge-code#21 find_callers** — README claimed "word-boundary regex" but the impl uses the tree-sitter symbol index. Updated to reflect actual behavior; the user-visible word-boundary semantics are preserved. ## Code - **dirge-code#16 semantic index skip_dir** — `SymbolIndex::find_callers` filter had its own hardcoded `matches!(name, "node_modules" | "target" | ".git" | "__pycache__")` while the rest of the codebase uses `agent::tools::is_skip_dir`. Switched to the shared helper so future additions stay in lockstep. - **dirge-code#18 context::load_file** — silently swallowed `read_to_string` errors via `.ok()`. A permission-denied AGENTS.md looked identical to a missing file. Now emits a stderr warning naming the path + reason; still returns None so callers' behavior is unchanged. 725 plugin / 599 default pass. All build profiles clean. ## Remaining audit items (deferred to feature work) - **dirge-code#8 LSP no crash restart**: needs broken-pipe IO error handling + exponential backoff. Touches manager state machine. - **dirge-code#10 task tool fire-and-forget**: needs timeout + cleanup coordination via JoinHandle tracking. - **dirge-code#25 MCP no reconnection**: similar architectural concern to dirge-code#8. - **dirge-code#27 LSP didClose**: client lifecycle hook missing. - **dirge-code#29 token estimation len/4**: needs per-provider usage extraction (Phase 6 work). - **dirge-code#5 MCP shutdown**: rmcp Drop semantics need verification. - **dirge-code#38/39/40 semantic test gaps**: get_symbol_body untested, list_symbols kind_filter untested, find_definition test vacuous. Sat down to add but each requires a fixture build. Together with PR dirge-code#111 (10 code fixes), 17 of the 23 verified items are now shipped. Remaining 6 are architectural or test-infrastructure work better tackled as discrete PRs. Co-authored-by: Yogthos <yogthos@gmail.com>

…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.

…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.

…ser_rx writes Code review of `a70fb03`/`8b0688c`/`afc76eb` flagged 15 findings, including one **active regression** I shipped: `write_outside_chamber` reused `close_tool_chamber_if_open` which always painted "⚠ tool denied · aborted · no result". So every notification arriving while a tool was in-flight would falsely brand that tool as denied. Fixed. Headline: of 9 tokio::select! arms, only 2 were using the new chokepoint. 3 others (question_rx, dialog_rx, plan_rx) carried the SAME X-inside-chamber bug the helper was built to eliminate. Migrated them. - **#4 HIGH (regression I shipped)**: split chamber-close into two variants: - `close_tool_chamber_abort` — paints the "⚠ tool denied" row + bottom border. Used by permission-deny / agent error / interjection / context-overflow paths (the tool is being actively rejected). - `close_tool_chamber_passive` — emits ONLY the bottom border. Used by `write_outside_chamber` (the tool isn't being denied; we just need to terminate the visual frame so notification text doesn't land inside). - `close_tool_chamber_if_open` kept as back-compat alias for the abort variant — existing call sites (4 of them, all in abort-shaped contexts) keep their previous behavior. - **#1 / #2 / #3 CRITICAL — three arms migrated**: - `question_rx` (3537): a `question` tool's chamber was open when the prompt header was painted; header + stem + option grid landed inside. - `dialog_rx` (3811): plugin `harness/confirm` / `harness/select` fires from inside on-tool-start hooks while a tool chamber is open; the dialog rendered inside. - `plan_rx` (3955): plan-switch prompt could be delivered while a tool chamber was open; prompt landed inside. - **dirge-code#5 HIGH — user_rx interactive writes migrated**: - Ctrl+C interrupt msg (1135) - "copied selection" (1150) - Ctrl+X dropped-interjection trailer (1168) - "agent is busy" × 2 (1533, 1598) - **dirge-code#7 MEDIUM — defense-in-depth sanitization**: `write_outside_chamber` now runs `strip_controls(KEEP_NEWLINE)` on `text` before writing. A future caller that forgets producer-side sanitization can't smuggle ANSI escapes. - **dirge-code#12 LOW — notification amplification cap**: the bounded channel limits NOTIFICATIONS but not ROWS per notification. A single `Notification::McpLog` carrying 10k `\n`s would expand to 10k chamber rows. After 200 lines we truncate and emit a `[N more lines suppressed]` marker. - **dirge-code#6 audit** revealed the 4 remaining manual sites (1984/2602/2713/2884) all ARE abort-shaped and correctly use the abort variant via the back-compat alias. No migration needed. - **dirge-code#8 / dirge-code#9 / dirge-code#10 / dirge-code#13 / dirge-code#14 / dirge-code#15** noted as design trade-offs or already verified clean. 3 new regression tests: - `close_passive_does_not_paint_abort_row` pins the new no-abort-label contract - `close_abort_paints_warning_and_bottom` pins the abort variant still emits 2 rows - existing `write_outside_chamber_closes_chamber_first` still passes; helper now uses passive close 718 tests pass (716 + 2 new); fmt clean.

Yogthos added 3 commits May 19, 2026 10:54

Cleanup question-tool: drop dead QuestionRequest.id field

961b23a

The id field was generated via Uuid::new_v4() and marked #[allow(dead_code)] but never read. Dropped along with the uuid::Uuid import to simplify the request shape.

yogthos mentioned this pull request May 19, 2026

Add websearch and webfetch tools #14

Merged

4 tasks

Run cargo fmt

bfdd6d2

yogthos merged commit 0505dfa into main May 19, 2026
1 check passed

yogthos deleted the feature/question-tool branch May 19, 2026 16:56

yogthos mentioned this pull request May 21, 2026

fix(F13): surface negative net savings on compress #86

Merged

yogthos mentioned this pull request May 21, 2026

fix: audit batch 2 — docs accuracy + 2 small code touch-ups #112

Merged

Navigation Menu

Uh oh!

Add question tool — agent asks user for clarification mid-execution - #13

Add question tool — agent asks user for clarification mid-execution#13
yogthos merged 4 commits into
mainfrom
feature/question-tool

yogthos commented May 19, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

yogthos commented May 19, 2026

Summary

Test plan

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant