Skip to content

Multi-line input: Shift/Meta+Enter, line navigation - #4

Merged
yogthos merged 1 commit into
mainfrom
feature/multi-line-input
May 19, 2026
Merged

Multi-line input: Shift/Meta+Enter, line navigation#4
yogthos merged 1 commit into
mainfrom
feature/multi-line-input

Conversation

@yogthos

@yogthos yogthos commented May 19, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Shift+Enter / Meta+Enter inserts a literal newline; plain Enter still submits
  • Up/Down navigates between logical lines inside a multi-line buffer; falls through to history at boundaries
  • Status bar shows `[N lines]` when buffer is multi-line; input row displays the current logical line
  • Renderer scrolls the visible line based on cursor column within that line

Test plan

  • Unit tests: Shift/Meta+Enter inserts newline; plain Enter submits; Up/Down moves between lines; history loads multi-line text
  • Manual: type a multi-line message, navigate with Up/Down, submit with plain Enter
  • Manual: Up at line 0 falls through to history; Down at last line continues to history

- Shift+Enter and Meta+Enter insert newline in input buffer
- Plain Enter submits full multi-line text
- Up/Down navigate between logical lines within multi-line buffer
- At top/bottom of buffer, Up/Down fall through to history navigation
- Renderer extracts current logical line for display
- Status bar shows [N lines] indicator for multi-line input
- 7 integration tests + 3 unit tests for line boundary helpers
@yogthos
yogthos force-pushed the feature/multi-line-input branch from ceeceb3 to aaf03b2 Compare May 19, 2026 04:00
@yogthos
yogthos merged commit 155dedc into main May 19, 2026
1 check passed
yogthos added a commit that referenced this pull request May 19, 2026
The squash merge of #6 (token counter) referenced 'input_line', but #4
(multi-line input) renamed the renderer's locals to 'full_input' /
'visible_line'. Use 'full_input' so the count also reflects multi-line
drafts.

Co-authored-by: Yogthos <yogthos@gmail.com>
yogthos added a commit that referenced this pull request May 21, 2026
…t tool count + token parity (#66)

TDD: tests first, all 6 new tests failed initially, then implemented.

## Round A — hook error sanitize + dedup

Two bugs in PR #64's hook-error notification path:

1. Multi-line / tab-containing Janet errors broke the
   `level\tmsg\n` wire format. A `(error "trace\n  at file:42")`
   produced multiple malformed notification entries (one per source
   line), the first with truncated content and the rest filtered
   out as malformed. drain_notifications splits raw on `\n` per
   entry and on first `\t` per level/msg — both control chars now
   sanitized in Janet before push.

2. A buggy `on-message-update` hook (fires ~every 16 streamed
   tokens) flooded the chat with thousands of identical "[plugin]
   hook X.Y errored: ..." banners during a single long response.
   Now deduped: two new Janet vars track the most-recent sanitized
   error msg + a consecutive-repeat count; identical errors just
   bump the count instead of pushing. On drain, any outstanding
   count is flushed as a "(repeated N times)" summary entry.

Implementation:
- `harness/sanitize-hook-err` (new) normalizes `\t` → space and
  `\n`/`\r\n` → ` | `. Distinct hook errors stay separate; only
  consecutive identical ones collapse. Wrote with explicit nested
  `string/replace-all` calls — Janet's `->` threading macro
  would pass the string in the wrong arg position
  (string/replace-all expects `(patt subst str)`).
- `harness/push-hook-err` (new) does the dedup check using
  `harness-last-hook-err-msg` + `harness-last-hook-err-count`
  module-level vars.
- `drain_notifications` flushes pending dedup count before reading
  the notif list so a 50× repeat shows up as a single
  "(repeated 50 times)" entry in the next drain.
- The catch arm in `dispatch` calls these instead of appending
  directly. Wrapped in explicit `(do ...)` for Janet's
  single-form catch-body semantics.

## Round B — partial-on-abort trailer notes tool calls

PR #65 saved the streamed assistant text on abort but didn't
indicate that tool calls had also run in the same turn (whose
results aren't in `response_buf` — only Token events accumulate
there). The LLM on next turn would see the partial as a definitive
"this was my reply" and could re-run side-effecting tools.

`capture_partial_on_abort` now takes a `tool_calls_in_turn: u32`
parameter. When non-zero, the trailer reads:
  [interrupted by user (Ctrl+C); 2 tool calls ran in this turn — results not preserved]
Singular case ("1 tool call ran") uses the right noun.

UI loop tracks `tool_calls_this_run: u32`, incremented on every
`AgentEvent::ToolCall`, reset on `Done`/`Interjected`/both abort
sites (since each marks the end of one agent run).

## Round C — token-accumulator parity on abort

`Done` and `Interjected` branches both update `session.total_tokens`
alongside the message add. The abort path didn't — made aborted
turns look like zero-token contributions in the placeholder
field. Fixed with an explicit
`session.total_tokens.saturating_add(Session::estimate_tokens(&stashed))`
inside `capture_partial_on_abort`.

Both fields stay under the `TODO(cost-tracking)` comment but at
least they're now internally consistent.

## Test plan

- [x] 6 new tests (3 plugin dispatch + 3 capture_partial_on_abort
      + 2 updated existing tests with new signature).
- [x] `cargo test --features plugin` -> 622 pass, 0 fail.
- [x] `cargo build --all-features` -> compiles.

## Skipped (observational, not bugs)

- #3 print/loop mode notifications never drained: print mode is
  non-interactive; tracing::warn (via `--verbose`) is the right
  channel.
- #4 Janet `err` non-string-coerced: `(string ...)` calls Janet's
  `tostring` which handles any value type. Documented behavior.
- #7 markdown rendering of `[interrupted by user (Ctrl+C)]`: not
  a link by pulldown-cmark's rules; visually acceptable inline.

Co-authored-by: Yogthos <yogthos@gmail.com>
yogthos added a commit that referenced this pull request May 21, 2026
Track F-HIGH #4 from ROADMAP.md.

## Problem

`read.rs:91-98` rejected files >10MB outright with
`File too large (N bytes). Max 10MB.` Agents couldn't sample
large logs, generated outputs, build artifacts, or test fixtures.
Workaround was a bash `head`/`tail` invocation, which obscures
intent and skips the LSP warmup that read provides.

## Fix

Replace eager `tokio::fs::read_to_string` with a streaming
`tokio::io::BufReader::lines()`:

- Stream line-by-line, tracking total count for the header.
- Truncate any individual line longer than `MAX_LINE_BYTES = 16384`
  to defend against pathological minified-JS / accidental binary
  reads. UTF-8 boundary-safe truncation; trailing
  ` …[line truncated]` marker so the LLM sees the cut.
- Keep an excerpt buffer of just `[offset, offset+limit)` lines —
  doesn't grow with file size.
- New safety net: `MAX_FILE_BYTES = 1GB`. Beyond that we still
  refuse but the error suggests bash + head/tail/grep instead.

Matches opencode's `read.ts:119-150` stream + early-terminate
shape and pi's `read.ts:215-328` smart truncation.

## Tests

Two new tests in `agent::tools::read::tests`:

- `read_truncates_pathological_long_lines`: writes a file with a
  100KB single line plus normal lines; asserts the long line is
  truncated with the marker and total output is <100KB.
- `read_handles_files_larger_than_old_10mb_cap`: 1MB fixture
  (10k × 99-byte lines); asserts read succeeds, header shows
  the true total line count, and the excerpt is just the
  requested 5 lines.

656 → 658 pass. All build profiles clean.

Co-authored-by: Yogthos <yogthos@gmail.com>
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
…nd hygiene

Self-review of the chamber + collapse work found 8 actionable issues.
Addressing all in one round.

- **#1 CRITICAL — ContextOverflow infinite loop on compress no-op**
  (`ui/slash.rs`, `ui/mod.rs`): `handle_compress` had three `Ok` paths
  that returned without compacting (within-limits, nothing-to-cut,
  summary-too-large). The auto-recovery branch treated any `Ok` as
  success and respawned the run against the SAME history, which
  immediately re-emitted `ContextOverflow` → loop. Added
  `CompressOutcome::{Compacted, NoOp{reason}}`; auto-recovery now
  respawns only on `Compacted` and surfaces a "made no progress"
  error otherwise. The interactive `/compress` and auto-Done paths
  ignore the discriminant — only the success/error path matters
  there.

- **#2 HIGH — ContextOverflow re-runs side-effecting tools**: the
  interactive retry loop already refuses to retry once
  `had_tool_calls=true`; the new auto path bypassed that safety.
  Now gates respawn on `tool_calls_this_run == 0`; if any tool ran
  in the failed turn, compact happens but auto-retry is refused and
  the user must re-issue.

- **#3 HIGH — char-truncated body wasn't stashed for Ctrl+O**:
  `render_tool_output` returned `None` when only `chars_truncated >
  0` (no line truncation) — Ctrl+O reported "nothing to expand"
  despite the visible `+N chars truncated` footer. Now stashes
  whenever EITHER signal indicates hidden content. Regression test:
  `render_tool_output_stashes_on_char_truncation_alone`.

- **#4 HIGH — `last_collapsed` persisted across turns**: a Ctrl+O
  press would expand a collapsed result from any prior unrelated
  turn. Cleared on prompt-send (every user submission starts a new
  turn) and on ContextOverflow respawn.

- **#5 MEDIUM — Ctrl+O was one-shot via `.take()`**: switched to
  `.as_ref().cloned()` so a second Ctrl+O re-emits the same expand;
  stash overwrites on the next collapse or clears on next turn.

- **#6 MEDIUM — empty `resolved_name` painted unnamed chamber**:
  added an early-out: when name resolution drops to empty (no
  `last_tool_name`, no buffered call by id), emit a dim
  `(unresolved tool)` trailer + chamber bottom and skip the body
  paint.

- **#7 MEDIUM — edit colorized diff lost when `last_tool_name`
  drained**: `is_edit` was gated on `last_tool_name`, falling
  through to plain `render_tool_output` when the slot drained
  (same shape as the chamber-orphan bug). Now gates on
  `resolved_name`.

- **#8 DESIGN/SEC — `banner_value` unsanitized at expand time**:
  the initial chamber TOP sanitizes the banner; the Ctrl+O reprint
  did not. ANSI-bearing tool args (MCP, plugin, attacker-shaped
  filename) could paint raw at expand. Sanitize once at stash
  time in `render_tool_output`.

Also (review #9, reviewer recommendation): swapped `apply_patch` out
of `tool_skips_collapse` and `read` in. `read` is what the user/LLM
explicitly asked for — defaulting to 4 lines defeated the request.
`apply_patch` output is usually a short "N ops" summary; the rare
per-op-failure spew is the right place for Ctrl+O to engage.

Tests: 625 pass (3 added — char-truncation stash, apply_patch
collapses, exempt-set without apply_patch). Updated 2 existing
tests for the exempt-set change. Fmt clean.
yogthos pushed a commit that referenced this pull request May 22, 2026
…view

Adversarial review of the per-prompt deny-list architecture flagged
two real bypasses + several defense-in-depth gaps. Addressing.

- **#1 CRITICAL — MCP tools bypassed the deny-list entirely**:
  `McpTool::call` passes the umbrella name `"mcp_tool"` to
  `check_perm`. The deny-list match is literal `==` (now case-
  insensitive), so a prompt declaring `deny_tools: [edit]` would
  NOT match an MCP server's `edit` tool — the LLM could route
  filesystem writes through any MCP server unscathed. Added
  `PermissionChecker::any_prompt_denied(&[name1, name2, ...])`
  public probe; `McpTool::call` now checks (concrete tool name,
  `mcp_tool:<server>:<name>` qualified form, umbrella `mcp_tool`)
  before invoking `check_perm`. Any hit returns a hard denial.

- **#2 CRITICAL — ACP never installed the prompt deny-list**:
  the ACP bridge built a fresh `PermissionChecker` per session
  and never wired in `context.current_prompt_deny_tools`. Plan
  mode was a no-op for editor clients. Mirror the
  `apply_prompt_deny` call from `main.rs::build_channels` into
  the ACP `run_prompt` path right after `build_acp_permission`.

- **#5 MEDIUM — `glob` / `repo_overview` added to PermissionConfig**:
  both were filesystem walkers reachable via the perm checker but
  not declared as user-configurable in `PermissionConfig`. User-
  level `permission.glob = "deny"` would silently fall through to
  the `*` default. Added the fields + the per-tool rule loop entry.

- **#6 MEDIUM — `plan_enter` / `plan_exit` now consult deny-list**:
  both intentionally skip `check_perm` (the confirmation dialog
  IS the user-prompt). But the prompt deny-list should still apply
  — a strict-mode prompt that says `deny_tools: [plan_exit]` should
  refuse the call WITHOUT opening the dialog. Added a thin
  `check_prompt_deny` helper that queries `any_prompt_denied`
  before opening the channel.

- **#7 MEDIUM — case-insensitive tool-name matching**:
  `deny_tools: [Edit]` (typo capitalization) used to silently no-op.
  `is_prompt_denied` now uses `eq_ignore_ascii_case`; the frontmatter
  parser also lowercases at load so the stored list is canonical
  in every consumer (status line, UI, etc.).

- **#9 LOW — warn on unknown tool names in `deny_tools`**: at prompt
  load time, cross-check every `deny_tools` entry against a
  `KNOWN_TOOLS` list. Warns once per unknown entry, with the full
  known set printed for guidance. MCP-server-exported tool names
  will trigger this benignly; documented inline.

- **#3 HIGH — pin order with tests**: three new checker tests pin
  the contract:
  - `prompt_deny_any_matches_concrete_and_qualified_mcp_names`
    (locks the MCP bypass fix)
  - `prompt_deny_is_case_insensitive`
  - existing tests continue passing

- **#4 plugin trust boundary documented**: per the review's
  recommendation, added a "Plugin trust boundary" section to
  CONFIG.md acknowledging that plugins are inside the trust
  boundary and not sandboxed.

Not addressed (intentional):
- #8 doom-loop UI nudge on repeated deny-list hits — UX polish.
- #10 `/prompt default` clear confirmation — user-typed; would
  have to confirm every clear, including the legitimate ones.

634 tests pass; fmt clean.
yogthos pushed a commit that referenced this pull request May 22, 2026
…ry, repo_overview walk

Self-review of the last 5 commits surfaced 9 findings. Addressing all.

- **#1 MEDIUM — `repo_overview` permission patterns got command-glob
  semantics**: `is_path_tool_name` was updated for `glob` but not
  `repo_overview`, so a config rule like `"repo_overview": {
  "/etc/**": "deny" }` parsed `**` as command-glob (no path-spanning
  match). Added `repo_overview` to the path-tool arm.

- **#2 MEDIUM — `providers.<name>` override silently missed on
  capitalised provider names**: `parse_provider` is case-insensitive
  but `HashMap::get(provider)` is not, so `--provider Anthropic`
  built the client fine but the `providers.anthropic` chunk-timeout
  override was a no-op. Lowercased the lookup key in
  `resolve_stream_chunk_timeout` and `resolve_provider_info` (with
  the original key as a fallback so existing exact-case configs
  keep working).

- **#3 MEDIUM — `Prompt.description` parsed but never displayed**:
  the frontmatter `description: "..."` was being captured into a
  `#[allow(dead_code)]` field. A user writing a description expected
  it to appear somewhere. Wired into the `/prompt` list output —
  each prompt now renders as `name  description` (padded for
  alignment) when a description is set, falling back to the bare
  name otherwise.

- **#4 MEDIUM — `stream_chunk_timeout_secs` undocumented**: the
  knob landed in 6c395d0 with the error message pointing users at
  it, but neither CONFIG.md nor README explained the resolution
  ladder. Added a "Streaming timeouts" section to CONFIG.md
  covering the precedence order (custom_providers > providers >
  top-level > 300s default) and noting the case-insensitive
  matching.

- **#5 LOW — Unicode lowercase mismatch with ASCII-only matcher**:
  the frontmatter parser called `to_lowercase()` (Unicode-aware)
  while `is_prompt_denied` used `eq_ignore_ascii_case`. Swapped
  the parser to `make_ascii_lowercase` so both ends share the same
  contract — non-ASCII bytes pass through unchanged on both sides.

- **#6 LOW — MCP bare-name deny semantics undocumented**: the
  3-name probe `any_prompt_denied(&[concrete, qualified,
  "mcp_tool"])` matches an MCP server's `edit` tool when the
  prompt denies `edit` (intended for the built-in). README now
  spells this out and recommends the qualified
  `mcp_tool:<server>:<name>` form for surgical denies.

- **#7 LOW — `KNOWN_TOOLS` duplicated `BUILTIN_TOOL_NAMES`**:
  two hand-maintained lists of every built-in tool, used for the
  MCP collision filter and the frontmatter `deny_tools` warn
  respectively. Drift would produce either spurious warnings or
  (worse) an unsafely shadowable name. Extracted a single
  `pub const BUILTIN_TOOL_NAMES` in `agent/tools/mod.rs`; both
  sites now import it.

- **#8 DESIGN — ACP can't switch prompts mid-session**: the
  30604a3 fix installed the deny-list correctly per-request but
  ACP has no protocol message for `/prompt <name>`, so the deny-
  list is effectively locked at boot. Documented in the README
  ACP bullet — recommends `--prompt <name>` at launch for
  restricted modes.

- **#9 LOW — `repo_overview` ancestor walk went above crawl root**:
  `compute_dir_file_counts` bumped every parent up to `/`,
  including dirs that would never be printed. Now stops at the
  first ancestor not in the printed-dir set.

634 tests pass; fmt clean.
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
…e migration, model-shrink, unicode tokens

Self-review of the last 6 substantive commits flagged 7 findings.
Addressing all in one batch.

- **#1 HIGH — Accept mode silently bypassed mcp_tool's default-Ask**:
  `SecurityMode::Accept` coerced `Ask → Allow` for every non-path
  tool, so the new default-Ask rule for `mcp_tool` (27bd70a) was
  effective only in Standard mode. `dirge --accept` + an MCP
  server still allowed every call silently. Added
  `is_high_risk_non_path_tool(tool)` (matches `mcp_tool` and
  `bash`) which forces the Ask to survive the coercion. The
  general non-path coercion still applies to `question`, etc.
  Regression test pins both directions.

- **#2 MEDIUM — Pre-9a044ce sessions resumed with under-counted
  `estimated_tokens`**: stored values were computed under the
  old text-only logic. Bumped `SCHEMA_VERSION` to 2 and added a
  v1 → v2 migration step that calls a new
  `Session::recompute_all_estimates` (also exposed
  `estimate_message_tokens` as the per-message helper). Tested
  via `v1_to_v2_recomputes_under_counted_estimates`.

- **#3 MEDIUM — `/model` switch to a smaller window didn't warn
  about over-capacity**: switching from a 1M to a 200k model
  with the session already exceeding the new budget would have
  errored mid-stream on the next prompt. The /model handler now
  surfaces a warning recommending `/compress` when
  `total_estimated_tokens > new_ctx - reserve`.

- **#4 LOW — Highlight tokenizer mis-split non-ASCII identifiers**:
  `bytes[i] as char` for a UTF-8 lead byte produced a Latin-1 char
  that failed `is_ascii_alphanumeric`, terminating the identifier
  mid-word (`naïve` → `na` + punctuation + `ve`). Walk via
  `line[i..].chars().next()` and broadened `is_ident_cont` to
  accept any non-ASCII non-control letter. ASCII path unchanged.

- **#5 LOW — `looks_like_type` colored short capitalized words as
  types**: `Ok`/`No`/`Hi`/`Id` all hit. Tightened the floor to
  ≥3 chars. Added `Ok`/`Err`/`Some`/`None` to the Rust types
  table so idiomatic 2-char Rust constructors still get type
  color via the explicit table.

- **#6 INFO — Yolo bypass documented**: README permission section
  now says explicitly that `--yolo` skips rule eval, the
  per-tool default-Ask, and the doom-loop detector — but that
  `deny_tools` frontmatter STILL applies (that gate runs BEFORE
  the yolo short-circuit by design). Accept-mode bullet also
  updated to note `bash` and `mcp_tool` keep their Ask.

- **#7 + #8 CLEARED**: doom-loop Allow + `"*": "allow"` default
  config both verified NOT to bypass mcp_tool's Ask.

684 tests pass; fmt clean.
yogthos pushed a commit that referenced this pull request May 22, 2026
Audit response covering the user's 6-item duplication list. Three
substantive modules + thin wrappers; two items deferred as
non-issues; one already complete from earlier commits.

**#4 — ANSI / control-byte filter (new `src/ui/ansi.rs`)**:
Previously three independent filters: MCP forwarder (`emit_mcp_line`),
websearch (`strip_tags_and_decode`), chat (`sanitize_output`). Each
drifted in coverage — one blocked C0 but not C1, another stripped
`\r` only, etc. New `ansi::strip_controls(s, policy)` with a
`StripPolicy { keep_newline, keep_tab }` knob is the single source
of truth. MCP forwarder + websearch routed through it. Chat
`sanitize_output` left as-is — it does ANSI-escape PARSING (consumes
`\x1b[…m` as a unit so the payload disappears) which is more
specific than what `strip_controls` does (drops just ESC, leaving
`[31m` as visible text). The two coexist intentionally.

**#1 — Reusable box component (new `src/ui/box_render.rs`)**:
Three implementations of chamber/box math previously: tool
chambers (`chamber_row` / `chamber_row_with_bg` / `chamber_bottom`),
permission alert (inline `row` closure), panel sections
(`push_section` closure). Inconsistent — `chamber_row` was
display-width-aware, `chamber_row_with_bg` was char-count-based;
each treated tabs and width math differently.

New module:
  - `BoxStyle` enum (currently only `Rounded`)
  - `top(style, title, total_w)`,
    `bottom(style, total_w)`,
    `divider(style, total_w)` — frame primitives
  - `row(style, content, total_w)` — display-width-aware
    content row with tab expansion + truncate-with-`…`
  - `row_with_bg(style, content, total_w, bg_idx)` — for diff
    backgrounds
  - `expand_tabs(s, tab_stop)` — moved here from `mod.rs`
  - `BoxBuilder` — fluent API for callers that build a box
    all-at-once (notifications, alerts, panel sections). Long
    rows soft-wrap via `wrap::soft_wrap` instead of truncating.

`chamber_row`, `chamber_row_with_bg`, `chamber_bottom` in `mod.rs`
are now thin wrappers around `box_render`'s primitives — existing
call sites unchanged, but the underlying math is shared. 7 new
unit tests covering frame width invariants, tab handling, CJK,
builder construction, and soft-wrap.

**#2 — Single output chokepoint** — already done in commit
`dc21de7` (the `ui::notifications` module + channel). The
remaining stray `eprintln!` sites all run during STARTUP (config
parsing, skill discovery, MCP connect_all) BEFORE `TerminalGuard`
is installed, so they don't paint over the UI. Audited and
confirmed clean.

**#3 — Permission check chokepoint** — already centralized.
`check_perm` / `check_perm_path` / `check_perm_path_resolve` in
`agent/tools/mod.rs` cover every tool's input-checked path;
`any_prompt_denied` covers MCP-style multi-name lookup;
`check_prompt_deny` covers plan tools. No duplication worth
extracting.

**#5 — Layout module** — deferred. `chamber_widths`,
`Renderer::content_width`, `Renderer::line_width`,
`Renderer::max_line_width` are already in their proper homes;
forcing them into a new module would be churn, not clarification.

**#6 — Soft-wrap inside chambers** — deferred. The current
chamber rows truncate with `…` (matches user expectation for
single-row tool result lines); the `BoxBuilder` path soft-wraps
when the caller wants that explicitly. Forcing all chamber rows
to soft-wrap would change the visual feel of tool output and
needs a separate UX call.

709 tests pass (702 + 7 box_render); 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
…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.
yogthos pushed a commit that referenced this pull request May 22, 2026
…rupt on crash

External review flagged that `write.rs:105`, `edit.rs:234`, and
`apply_patch.rs:93,155` all called `tokio::fs::write` directly,
which opens with O_TRUNC and writes in-place. A crash between the
truncation and the final byte (power loss / OOM-kill / SIGKILL /
panic) leaves the file corrupted with no recovery. The irony:
`session/storage.rs` already had the correct pattern — temp +
fsync + rename — but it wasn't shared.

Extracted the pattern into new top-level module `src/fs_atomic.rs`:
  - `atomic_write_sync(path, content)` — sync, used by storage
  - `atomic_write(path, content)` — async, used by tools (delegates
    to spawn_blocking so the create + fsync + chmod + rename
    sequence runs atomically in one blocking task)
  - `next_temp(target)` — hidden sibling temp path with
    pid+nanos+counter nonce so two concurrent saves don't collide
    on the temp filename (counter is the load-bearing piece —
    same-nanosecond firings still get distinct names)
  - Unix mode preservation: stat the existing target's perms BEFORE
    rename, chmod the temp to match. Without this, an atomic
    overwrite of an executable script would silently drop the +x
    bit (default temp perms are 0644 minus umask).

Migrated four call sites:
  - `agent/tools/write.rs:105`     — full-file write
  - `agent/tools/edit.rs:234`      — edit-tool output
  - `agent/tools/apply_patch.rs:93`  — apply_create
  - `agent/tools/apply_patch.rs:155` — apply_update
Plus `session/storage.rs` now also uses the shared helper (was
the original site with the pattern; now consolidated).

Return type: `io::Result<()>` so existing `From<io::Error>` impls
on `ToolError` / `anyhow::Error` continue to work. The async
variant maps spawn_blocking join failures to `io::Error::other`.

Six new regression tests:
  - `atomic_write_creates_new_file`
  - `atomic_write_overwrites_existing`
  - `temp_is_hidden_sibling` — verifies same-fs + dot-prefix
  - `next_temp_is_unique` — 1000-call distinct-name check
  - `atomic_write_preserves_mode` (Unix) — +x stays on across
    overwrite
  - `target_untouched_on_failed_rename` — atomicity guarantee

Tests use `std::env::temp_dir` + a `TestDir` RAII helper (matches
the codebase convention; dirge doesn't pull in `tempfile`).

724 tests pass (718 + 6); fmt clean.

#2 (ToolStarted event), #3 (prepareNextTurn hook), #4 (structured
tool output) follow in separate commits.
#5 from the review was a false positive — `skill` IS registered at
`builder.rs:239`.
yogthos pushed a commit that referenced this pull request May 22, 2026
External-review #3 and #4 implemented as minimal, honestly-scoped
versions.

**#3 — `prepare-next-run` plugin hook**

New hook fires AFTER `Done` (run complete) and BEFORE the next
user prompt is processed. Plugins read this to signal session-
level state changes for the next run. Currently the only
supported mutation slot is `harness-next-model` (Janet:
`(harness/set-next-model "claude-opus-4.7")`).

Scope honesty: the request is SURFACED to the user as a
notification (`"[plugin] requested model swap to 'X' — apply
with /model X"`) rather than auto-applied. Auto-apply is
deferred because:
  - The agent rebuild path is non-trivial across cfg-feature
    combinations.
  - The existing `/model` slash already does it correctly.
  - "Plugins propose, user disposes" is the safer default —
    a plugin can't silently swap to a more expensive model
    without the user noticing.

Mid-stream model swap is explicitly UNSUPPORTED — rig's
multi-turn stream owns state that doesn't survive a swap. The
hook is scoped to between-runs only, documented in the comment
next to the slot.

Slot infrastructure:
  - `harness-next-model` declared in `worker.rs` startup blob
  - `harness/set-next-model` helper in the same blob
  - `take_pending_next_model()` on `PluginManager` clears the
    slot and returns its value

**#4 — `ToolContent` classification on `ToolResult`**

New enum `event::ToolContent { Text, File }`. Added as an
additive field on `AgentEvent::ToolResult { id, output, kind }`.

`output: CompactString` remains the authoritative payload for
the LLM and the default UI rendering path — `kind` is purely
metadata for richer consumers (ACP resource links, future UI
file-card components).

The runner classifies by tool name: `read` / `find_files` /
`list_dir` produce `File`, everything else `Text`. Tracked via
a per-stream `id → name` HashMap populated at each `ToolCall`,
drained at the matching `ToolResult` (1:1 call/result pairing
within a turn).

Coarse on purpose — no per-tool `type Output` change required
across ~20 tools. A future refactor could thread the variant
through the rig `Tool` trait for finer-grained control.

Consumers:
  - `extras/acp/mod.rs` reads `kind` (currently no-op; comment
    flags `ResourceLink` migration as a follow-up)
  - `ui/mod.rs` uses `{ .. }` rest pattern; ignores `kind` for
    now

**#5 from the review remains a false positive** — `skill` IS
registered at `builder.rs:239`.

724 tests pass; all-features build 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 24, 2026
Closes the gap the article calls out: open models (DeepSeek-flash,
v4-pro, GLM, Qwen) produce a small finite set of tool-call shape
mistakes that strict JSON Schema validation rejects without a hint
the model can recover from. With this layer in place the article
author saw DeepSeek v4-pro beat Opus 4.7 6/10 on their evals.

What lands

* Phase 1 — validate-then-repair layer
  (src/agent/agent_loop/tool_input_repair.rs, 535 lines + 36 tests)
  Wraps `prepare_tool_call` (tools.rs:194-232). Valid inputs pass
  through untouched; on schema failure, walks each issue path and
  applies the four shape repairs in this exact order:
    1. Null-strip for optional fields
    2. JSON-string-as-array  (MUST run before #4)
    3. Empty-object-to-array
    4. Bare-string-to-singleton-array
  The ordering invariant — `'["a","b"]'` becomes `["a","b"]`, NOT
  `['["a","b"]']` — is pinned by ordering_json_string_before_bare_string.

* Phase 2 — markdown auto-link unwrap for path fields
  Schema-driven via the known path-field name set (`path`,
  `file_path`, `filename`, `paths`, `dir`) plus an opt-in
  `x-dirge-kind: "path"` annotation. Only the degenerate case
  (`[notes.md](http://notes.md)` where link-text equals
  url-without-protocol, or is a suffix of the URL path) is unwrapped;
  real markdown like `[click](https://example.com)` passes through.
  Never applied to content/text fields — verified by
  md_unwrap_only_path_fields_via_validate.

* Phase 3 — read_file relational defaulting
  When only one of `offset`/`limit` is provided, fill the other
  (limit → offset=line 1, offset → limit=2000) and surface the
  choice as a `Note:` line in the result body (not `Error:` — TUI
  doesn't paint Note red). Phrasing uses 1-indexed line numbers to
  match the schema description. 4 new tests cover all combinations.

* Phase 4 — defense-in-depth error formatting
  `format_structured_error` produces a model-readable
  Tool/Expected/Got/Try block instead of raw `serde_json::Error`
  diagnostics. `format_tool_error` (rig_tool.rs:192) wraps any
  leak through with the same structured retry hint.

* Phase 5 — `(model, tool, repair_kind)` telemetry
  `tracing::info!(target: "tool_repair", model=…, tool=…,
  repair=…)` fires on every repair (success or failure). Picked up
  by `RUST_LOG=tool_repair=info` or `--verbose`. Threads model_name
  through `AnyAgent` → `LoopSpawnConfig` → `LoopConfig` so per-(model,
  tool) regression detection works.

Review fixes from the first pass

* B1 — Note text uses "line 1" / "1-indexed" phrasing (matches the
  schema description; avoids the model retrying with `offset=0` and
  hitting a different cache key).
* B2 — `navigate_schema` descends into array items via numeric
  pointer segments, so the structured-error `Expected:` line shows
  the per-item schema for nested-array tools instead of falling
  back to "(see tool schema)".
* B4 — Removed redundant `strip_null_optionals` top-level call;
  `strip_null_recursive` already handles Object and Array roots.

Two new tests pin B2: `navigate_schema_descends_into_array_items`
and `structured_error_uses_array_item_schema`.

Skipped (deferred)

* B3 — `args.clone()` on every call is wasted for valid inputs. Would
  require deferring content-normalizers behind validation; not a
  correctness issue, just a perf opportunity. Most tool args are small.
* Schema-vs-serde divergence (e.g. JSON Schema `integer` accepts
  negative; Rust `Option<usize>` rejects) — tool-author responsibility;
  add `"minimum": 0` to schemas for unsigned fields.

Plus

* docs/DEEPSEEK_TOOL_INPUT.md — full writeup of findings + the
  5-phase plan, with file:line citations into this code.

Verified

* cargo test --bin dirge --features plugin  → 1215 passed
* cargo test --bin dirge                    → 1001 passed
* cargo build --bin dirge --features semantic-elixir → clean
* cargo fmt --all --check                   → clean
yogthos pushed a commit that referenced this pull request May 27, 2026
…tests

SESS-2 follow-up #1 — UI session-mutation on ContextCompacted:
ContextCompacted event now carries summary + first_kept_index. The
UI consumer mutates session.id in-place, calls
Session::compress_reporting() to push a Compaction entry, and runs
save_session() so the rotated id and summary are persisted on disk.
Mirrors hermes-agent/conversation_compression.py lines 380-397.
Without this the on-disk session kept the OLD id and the
compaction was lost on next resume.

SESS-2 follow-up #4 — /compress <focus> argument wire-through:
- build_summary_prompt now honors focus_topic: when supplied, the
  Hermes-style "FOCUS TOPIC: …" framing is appended to the prompt,
  asking the model to allocate ~60-70% of its summary budget to
  the topic (verbatim port of hermes context_compressor.py:1050-1054).
- compress_messages (existing slash-command path) gets the same
  treatment: any free-form text after /compress is wrapped in the
  FOCUS TOPIC framing instead of the generic "Additional
  instructions" placeholder.
- run_compaction_pass exposes the focus parameter via a new
  with_focus wrapper (auto-trigger path still uses None).
- Slash help text updated: "/compress [focus]   compress; focus
  text guides what to preserve".

SESS-2 follow-ups #2 (background spawn) and #3 (multi-generation
chaining) closed as "matches reference impl": hermes is also inline
(no background spawn) and only chains the most-recent prior summary
via _find_latest_context_summary. Our implementation already matches.

H7_SMOKE: remove the 6 #[ignore] markers from the real-API
integration tests. Each test already has a runtime
`detect_provider()` check that bails with `[skipped]` + Ok when no
provider key is set; #[ignore] was blocking that check from ever
running. Removing the markers means: in CI without keys, the
tests run, hit the skip path, pass (1713 pass / 0 fail / 0
ignored). With keys present, they exercise the real provider as
designed. Header doc updated.

Tests: 1713 pass / 0 fail / 0 ignored (was 1707 / 0 / 6).
yogthos pushed a commit that referenced this pull request May 27, 2026
…contract hints

Per docs/AGENTIC_LOOP_PLAN.md. Three of the four Phase-1 items;
item #4 (per-tool-call reassembly timeout in rig_stream) is more
invasive and lands separately.

**1. Repair-rate counters**

New `RepairStats` (per-RepairKind `AtomicU64` + `invalid` counter)
threaded through `LoopConfig::repair_stats`. `prepare_tool_call`
increments on each successful repair AND on repair exhaustion.
At AgentEnd, run.rs sends `LoopEvent::RepairStats { snapshot }`
when the snapshot is non-empty. Bridge translates to
`AgentEvent::RepairStats`; UI prints a one-liner:

    ⊕ repaired 3 input(s): 2 md-link, 1 null-strip; 1 invalid

Empty snapshots are skipped so clean sessions don't print
"repaired 0 inputs".

**2. Structured `tool_input_invalid` log**

When repair exhausts, the original args (up to 16 KiB) AND the
full validator error list land in a dedicated
`tracing::warn!(target: "tool_input_invalid", …)` event,
separate from the existing `tool_repair = "failed"` info log.
Structured-log consumers can filter on the target directly.

**3. `with_contract_hint` helper**

Centralises the "absolute path, not a markdown link" / "plain
UTF-8 string, not a JSON object" cues that built-in tools were
each writing into their own descriptions. Wired into the 10
built-in tools' `Tool::definition` impls:

    description: with_contract_hint(
        "read",
        "Read the contents of a file. …",
    ),

Tools without a registered hint get the base description back
unchanged.

Future Phase-1 dashboard / per-(model, tool) breakdown can read
the existing tracing fields (`model`, `tool`, `repair`,
`original_args`) without further code changes — the aggregate
counter is the visible-at-session-end summary, the tracing
events are the offline-analysis surface.

Build clean, no warnings. Full test suite: 1718 pass / 0 fail
/ 0 ignored.
yogthos pushed a commit that referenced this pull request May 27, 2026
…eams

When a provider stalls emitting `ToolCallDelta` events mid-tool-
call (a common DeepSeek failure mode the catalogue §2.2 calls
out), the broad `stream_chunk_timeout_secs` (default 300s) is
the wrong knob — it conflates "long reasoning gap" with
"reassembly is stuck." This narrows the effective timeout to
30s ONLY while a tool call is mid-assembly.

Implementation in `wrap_streamed_assistant`:

- Track `open_tool_calls: HashSet<String>` of tool calls whose
  `ToolCallEnd` hasn't fired.
- Insert on first `ToolCallDelta` for an internal_call_id;
  remove on the `ToolCall` arm's `ToolCallEnd` emit.
- In the per-chunk timeout selection, when `open_tool_calls`
  is non-empty, narrow to `min(configured, 30s)`. When
  `chunk_timeout` is None but a tool call is open, still
  enforce the 30s cap.
- Error message explains the narrowing so the user knows it's
  the tool-call-gap path, not the broad chunk timeout. Still
  contains "timed out" so `recovery::classify_error` routes it
  to Network for retry.

Test `tool_call_gap_timeout_fires_within_30s_even_with_large_chunk_timeout`
pins the behavior: 300s configured + ToolCallDelta + stall →
timeout fires at 31s with the mid-assembly explanation.

Phase-1 of docs/AGENTIC_LOOP_PLAN.md now complete:
1. ✅ Repair-rate aggregate counters (RepairStats + RepairStatsSnapshot)
2. ✅ Structured tool_input_invalid log target
3. ✅ with_contract_hint helper wired into 10 built-in tools
4. ✅ This — tighter per-tool-call reassembly timeout

Full test suite: 1719 pass / 0 fail / 0 ignored.
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).
yogthos pushed a commit that referenced this pull request May 28, 2026
Independent verification turned up 6 gaps in the Phase 2.5 parity work.
This commit closes all of them.

#1 HIGH — `truncations_fixed` now bumps on hard-fallback too. Reasonix
   counts both success (`repair/index.ts:105`) and unrecoverable
   (`repair/index.ts:99`) under the same counter; dirge was dropping
   the latter, under-reporting exactly the cases operators need most.
   `apply_truncation_repair` now records the kind whenever the closer
   ran, not just on successful repair.

#2 MEDIUM — closer notes are now surfaced to the model. Reasonix
   pushes `r.notes` into `report.notes` with `[<tool>]` prefix on
   success and `[<tool>] ⚠️ TRUNCATION UNRECOVERABLE: ...` on
   fallback (`repair/index.ts:100-101, :106`), then carries them
   into the next-turn assistant input. Dirge now stashes them
   per-call-id on a new `LoopConfig.truncation_notes` shared map;
   `prepare_tool_call` drains them and appends to `repair_notes`,
   which `prepend_notes_to_result` (already in place for
   relational-default notes) prepends to the tool result content
   so the model sees the repair in the same turn.

#3 MEDIUM — added end-to-end wiring tests through `run_agent_loop`.
   The prior 7 tests proved the helpers worked in isolation; they
   did not prove the loop calls them in the right order. Two new
   tests drive the full canned-stream loop:
   - `dirge_7bwx_end_to_end_storm_dedupes_after_truncation_repair`:
     three tool calls with different truncated raw strings that
     heal identically. Storm threshold=3 → the third must be
     suppressed (only possible if truncation runs before storm).
   - `dirge_ngic_end_to_end_orphan_dsml_in_text_dispatches`:
     DSML invoke in `ContentBlock::Text` ONLY (no Thinking, no
     declared ToolCall) must dispatch (only possible if
     `build_scavenge_source` includes Text).

#4 MEDIUM — removed dead `try_truncation_repair`. It was kept as
   "defense in depth" but marked `#[allow(dead_code)]`, so the
   safety claim was illusory. Now actually gone; direct callers
   can use `repair_truncated_json` for the brace-closer if needed.
   The `validate_and_repair` block-comment was updated to reflect
   the new contract.

#5 LOW — `truncation_repair_canonicalizes_divergent_streams_before_storm`
   tested canonicalization in isolation; the new end-to-end #3 tests
   exercise the actual storm dedupe path that depends on the
   String→Object promotion. The promotion itself is now also
   covered with an explicit note in `apply_truncation_repair`'s
   doc — it has no Reasonix analog (their args are always strings)
   and is dirge-specific compensation for mixed arg representations.

#6 LOW — added a comment near `storm.rs::inspect` documenting the
   implicit dependency on `serde_json` being built without the
   `preserve_order` feature. If feature unification ever enables
   it, storm dedupe regresses silently; the comment points at the
   workaround (`run::canonical_json`) and notes Reasonix has the
   same fragility at `repair/index.ts:127`.

`LoopConfig` gained the new `truncation_notes` field; all
constructors (production + tests) were updated. `Clone` impl
threaded through.

1632 tests pass with `-D warnings` (was 1630; +3 new, -1 removed).
yogthos added a commit that referenced this pull request May 29, 2026
…loop

feat(agent-loop): snip-tokens-freed feedback loop (#4)
yogthos added a commit that referenced this pull request Jun 2, 2026
* refactor: now_unix_secs/nanos + LockExt::lock_ignore_poison helpers [dirge-xhoo]

#4 time_util: SystemTime::now().duration_since(UNIX_EPOCH).map(|d| d.as_X())
.unwrap_or(0) was hand-rolled ~16 times (secs + nanos). Hoisted to
crate::time_util::now_unix_secs() / now_unix_nanos(); converted the uniform
multi-line sites. The variant forms (file-mtime, subsec_nanos as u64,
non-.map error handling) are genuinely different and left as-is.

#lock: the cryptic .lock().unwrap_or_else(|e| e.into_inner()) poison-recovery
idiom appeared 123 times across 43 files. Replaced with a named
LockExt::lock_ignore_poison() extension method (crate::sync_util) so the
intent ('dirge never relies on poison') is legible at every call site.
All sites are std::sync::Mutex; behavior identical.

2456 default / 2550 all-features tests pass (incl. a new poison-recovery
test); clean under -D warnings in both default + all-features.

* fix: allow(unused_imports) on LockExt imports for minimal feature configs

The windows-default config (--no-default-features) gates out the feature
blocks (mcp/plugin/lsp/…) that hold many lock_ignore_poison() call sites,
leaving the LockExt import unused there. The trait is genuinely
conditionally-used per feature set, so annotate the imports. Verified with
cargo build --no-default-features --features windows-default.

* test(dap): fix flaky dap_perm_check_roundtrips race on the DAP_PERM_CHECK global

The two tests that mutate the process-global DAP_PERM_CHECK static used
per-statement locks, so under parallel execution no_perm_check_when_none
could set None between dap_perm_check_roundtrips' write and read-back —
failing read_back.is_some() intermittently. Both tests now hold the global's
mutex for their whole body (one critical section), serializing them on that
lock so they can't interleave. Uses lock_ignore_poison for poison-robustness.
Stress-ran 25x clean.

* refactor: convert remaining multi-line .lock().unwrap_or_else(into_inner) stragglers

The single-line perl in the prior commit missed the multi-line form
(.lock() and .unwrap_or_else() on separate lines) — ~32 sites. Converted
them to lock_ignore_poison() too (+ LockExt import in the 3 files that had
ONLY the multi-line form). The 3 remaining unwrap_or_else(into_inner) sites
are RwLock .read()/.write() (notifications.rs) — a different lock type, left
as-is (not worth a second ext-trait for 3 sites). All configs build clean.

---------

Co-authored-by: Yogthos <yogthos@gmail.com>
allen-munsch referenced this pull request in allen-munsch/dirge Jun 3, 2026
- Shift+Enter and Meta+Enter insert newline in input buffer
- Plain Enter submits full multi-line text
- Up/Down navigate between logical lines within multi-line buffer
- At top/bottom of buffer, Up/Down fall through to history navigation
- Renderer extracts current logical line for display
- Status bar shows [N lines] indicator for multi-line input
- 7 integration tests + 3 unit tests for line boundary helpers

Co-authored-by: Yogthos <yogthos@gmail.com>
allen-munsch referenced this pull request in allen-munsch/dirge Jun 3, 2026
)

The squash merge of dirge-code#6 (token counter) referenced 'input_line', but #4
(multi-line input) renamed the renderer's locals to 'full_input' /
'visible_line'. Use 'full_input' so the count also reflects multi-line
drafts.

Co-authored-by: Yogthos <yogthos@gmail.com>
allen-munsch referenced this pull request in allen-munsch/dirge Jun 3, 2026
…t tool count + token parity (dirge-code#66)

TDD: tests first, all 6 new tests failed initially, then implemented.

## Round A — hook error sanitize + dedup

Two bugs in PR dirge-code#64's hook-error notification path:

1. Multi-line / tab-containing Janet errors broke the
   `level\tmsg\n` wire format. A `(error "trace\n  at file:42")`
   produced multiple malformed notification entries (one per source
   line), the first with truncated content and the rest filtered
   out as malformed. drain_notifications splits raw on `\n` per
   entry and on first `\t` per level/msg — both control chars now
   sanitized in Janet before push.

2. A buggy `on-message-update` hook (fires ~every 16 streamed
   tokens) flooded the chat with thousands of identical "[plugin]
   hook X.Y errored: ..." banners during a single long response.
   Now deduped: two new Janet vars track the most-recent sanitized
   error msg + a consecutive-repeat count; identical errors just
   bump the count instead of pushing. On drain, any outstanding
   count is flushed as a "(repeated N times)" summary entry.

Implementation:
- `harness/sanitize-hook-err` (new) normalizes `\t` → space and
  `\n`/`\r\n` → ` | `. Distinct hook errors stay separate; only
  consecutive identical ones collapse. Wrote with explicit nested
  `string/replace-all` calls — Janet's `->` threading macro
  would pass the string in the wrong arg position
  (string/replace-all expects `(patt subst str)`).
- `harness/push-hook-err` (new) does the dedup check using
  `harness-last-hook-err-msg` + `harness-last-hook-err-count`
  module-level vars.
- `drain_notifications` flushes pending dedup count before reading
  the notif list so a 50× repeat shows up as a single
  "(repeated 50 times)" entry in the next drain.
- The catch arm in `dispatch` calls these instead of appending
  directly. Wrapped in explicit `(do ...)` for Janet's
  single-form catch-body semantics.

## Round B — partial-on-abort trailer notes tool calls

PR dirge-code#65 saved the streamed assistant text on abort but didn't
indicate that tool calls had also run in the same turn (whose
results aren't in `response_buf` — only Token events accumulate
there). The LLM on next turn would see the partial as a definitive
"this was my reply" and could re-run side-effecting tools.

`capture_partial_on_abort` now takes a `tool_calls_in_turn: u32`
parameter. When non-zero, the trailer reads:
  [interrupted by user (Ctrl+C); 2 tool calls ran in this turn — results not preserved]
Singular case ("1 tool call ran") uses the right noun.

UI loop tracks `tool_calls_this_run: u32`, incremented on every
`AgentEvent::ToolCall`, reset on `Done`/`Interjected`/both abort
sites (since each marks the end of one agent run).

## Round C — token-accumulator parity on abort

`Done` and `Interjected` branches both update `session.total_tokens`
alongside the message add. The abort path didn't — made aborted
turns look like zero-token contributions in the placeholder
field. Fixed with an explicit
`session.total_tokens.saturating_add(Session::estimate_tokens(&stashed))`
inside `capture_partial_on_abort`.

Both fields stay under the `TODO(cost-tracking)` comment but at
least they're now internally consistent.

## Test plan

- [x] 6 new tests (3 plugin dispatch + 3 capture_partial_on_abort
      + 2 updated existing tests with new signature).
- [x] `cargo test --features plugin` -> 622 pass, 0 fail.
- [x] `cargo build --all-features` -> compiles.

## Skipped (observational, not bugs)

- #3 print/loop mode notifications never drained: print mode is
  non-interactive; tracing::warn (via `--verbose`) is the right
  channel.
- #4 Janet `err` non-string-coerced: `(string ...)` calls Janet's
  `tostring` which handles any value type. Documented behavior.
- dirge-code#7 markdown rendering of `[interrupted by user (Ctrl+C)]`: not
  a link by pulldown-cmark's rules; visually acceptable inline.

Co-authored-by: Yogthos <yogthos@gmail.com>
allen-munsch referenced this pull request in allen-munsch/dirge Jun 3, 2026
)

Track F-HIGH #4 from ROADMAP.md.

## Problem

`read.rs:91-98` rejected files >10MB outright with
`File too large (N bytes). Max 10MB.` Agents couldn't sample
large logs, generated outputs, build artifacts, or test fixtures.
Workaround was a bash `head`/`tail` invocation, which obscures
intent and skips the LSP warmup that read provides.

## Fix

Replace eager `tokio::fs::read_to_string` with a streaming
`tokio::io::BufReader::lines()`:

- Stream line-by-line, tracking total count for the header.
- Truncate any individual line longer than `MAX_LINE_BYTES = 16384`
  to defend against pathological minified-JS / accidental binary
  reads. UTF-8 boundary-safe truncation; trailing
  ` …[line truncated]` marker so the LLM sees the cut.
- Keep an excerpt buffer of just `[offset, offset+limit)` lines —
  doesn't grow with file size.
- New safety net: `MAX_FILE_BYTES = 1GB`. Beyond that we still
  refuse but the error suggests bash + head/tail/grep instead.

Matches opencode's `read.ts:119-150` stream + early-terminate
shape and pi's `read.ts:215-328` smart truncation.

## Tests

Two new tests in `agent::tools::read::tests`:

- `read_truncates_pathological_long_lines`: writes a file with a
  100KB single line plus normal lines; asserts the long line is
  truncated with the marker and total output is <100KB.
- `read_handles_files_larger_than_old_10mb_cap`: 1MB fixture
  (10k × 99-byte lines); asserts read succeeds, header shows
  the true total line count, and the excerpt is just the
  requested 5 lines.

656 → 658 pass. All build profiles clean.

Co-authored-by: Yogthos <yogthos@gmail.com>
allen-munsch referenced this pull request in allen-munsch/dirge 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 referenced this pull request in allen-munsch/dirge Jun 3, 2026
…nd hygiene

Self-review of the chamber + collapse work found 8 actionable issues.
Addressing all in one round.

- **#1 CRITICAL — ContextOverflow infinite loop on compress no-op**
  (`ui/slash.rs`, `ui/mod.rs`): `handle_compress` had three `Ok` paths
  that returned without compacting (within-limits, nothing-to-cut,
  summary-too-large). The auto-recovery branch treated any `Ok` as
  success and respawned the run against the SAME history, which
  immediately re-emitted `ContextOverflow` → loop. Added
  `CompressOutcome::{Compacted, NoOp{reason}}`; auto-recovery now
  respawns only on `Compacted` and surfaces a "made no progress"
  error otherwise. The interactive `/compress` and auto-Done paths
  ignore the discriminant — only the success/error path matters
  there.

- **#2 HIGH — ContextOverflow re-runs side-effecting tools**: the
  interactive retry loop already refuses to retry once
  `had_tool_calls=true`; the new auto path bypassed that safety.
  Now gates respawn on `tool_calls_this_run == 0`; if any tool ran
  in the failed turn, compact happens but auto-retry is refused and
  the user must re-issue.

- **#3 HIGH — char-truncated body wasn't stashed for Ctrl+O**:
  `render_tool_output` returned `None` when only `chars_truncated >
  0` (no line truncation) — Ctrl+O reported "nothing to expand"
  despite the visible `+N chars truncated` footer. Now stashes
  whenever EITHER signal indicates hidden content. Regression test:
  `render_tool_output_stashes_on_char_truncation_alone`.

- **#4 HIGH — `last_collapsed` persisted across turns**: a Ctrl+O
  press would expand a collapsed result from any prior unrelated
  turn. Cleared on prompt-send (every user submission starts a new
  turn) and on ContextOverflow respawn.

- **dirge-code#5 MEDIUM — Ctrl+O was one-shot via `.take()`**: switched to
  `.as_ref().cloned()` so a second Ctrl+O re-emits the same expand;
  stash overwrites on the next collapse or clears on next turn.

- **dirge-code#6 MEDIUM — empty `resolved_name` painted unnamed chamber**:
  added an early-out: when name resolution drops to empty (no
  `last_tool_name`, no buffered call by id), emit a dim
  `(unresolved tool)` trailer + chamber bottom and skip the body
  paint.

- **dirge-code#7 MEDIUM — edit colorized diff lost when `last_tool_name`
  drained**: `is_edit` was gated on `last_tool_name`, falling
  through to plain `render_tool_output` when the slot drained
  (same shape as the chamber-orphan bug). Now gates on
  `resolved_name`.

- **dirge-code#8 DESIGN/SEC — `banner_value` unsanitized at expand time**:
  the initial chamber TOP sanitizes the banner; the Ctrl+O reprint
  did not. ANSI-bearing tool args (MCP, plugin, attacker-shaped
  filename) could paint raw at expand. Sanitize once at stash
  time in `render_tool_output`.

Also (review dirge-code#9, reviewer recommendation): swapped `apply_patch` out
of `tool_skips_collapse` and `read` in. `read` is what the user/LLM
explicitly asked for — defaulting to 4 lines defeated the request.
`apply_patch` output is usually a short "N ops" summary; the rare
per-op-failure spew is the right place for Ctrl+O to engage.

Tests: 625 pass (3 added — char-truncation stash, apply_patch
collapses, exempt-set without apply_patch). Updated 2 existing
tests for the exempt-set change. Fmt clean.
allen-munsch referenced this pull request in allen-munsch/dirge Jun 3, 2026
…view

Adversarial review of the per-prompt deny-list architecture flagged
two real bypasses + several defense-in-depth gaps. Addressing.

- **#1 CRITICAL — MCP tools bypassed the deny-list entirely**:
  `McpTool::call` passes the umbrella name `"mcp_tool"` to
  `check_perm`. The deny-list match is literal `==` (now case-
  insensitive), so a prompt declaring `deny_tools: [edit]` would
  NOT match an MCP server's `edit` tool — the LLM could route
  filesystem writes through any MCP server unscathed. Added
  `PermissionChecker::any_prompt_denied(&[name1, name2, ...])`
  public probe; `McpTool::call` now checks (concrete tool name,
  `mcp_tool:<server>:<name>` qualified form, umbrella `mcp_tool`)
  before invoking `check_perm`. Any hit returns a hard denial.

- **#2 CRITICAL — ACP never installed the prompt deny-list**:
  the ACP bridge built a fresh `PermissionChecker` per session
  and never wired in `context.current_prompt_deny_tools`. Plan
  mode was a no-op for editor clients. Mirror the
  `apply_prompt_deny` call from `main.rs::build_channels` into
  the ACP `run_prompt` path right after `build_acp_permission`.

- **dirge-code#5 MEDIUM — `glob` / `repo_overview` added to PermissionConfig**:
  both were filesystem walkers reachable via the perm checker but
  not declared as user-configurable in `PermissionConfig`. User-
  level `permission.glob = "deny"` would silently fall through to
  the `*` default. Added the fields + the per-tool rule loop entry.

- **dirge-code#6 MEDIUM — `plan_enter` / `plan_exit` now consult deny-list**:
  both intentionally skip `check_perm` (the confirmation dialog
  IS the user-prompt). But the prompt deny-list should still apply
  — a strict-mode prompt that says `deny_tools: [plan_exit]` should
  refuse the call WITHOUT opening the dialog. Added a thin
  `check_prompt_deny` helper that queries `any_prompt_denied`
  before opening the channel.

- **dirge-code#7 MEDIUM — case-insensitive tool-name matching**:
  `deny_tools: [Edit]` (typo capitalization) used to silently no-op.
  `is_prompt_denied` now uses `eq_ignore_ascii_case`; the frontmatter
  parser also lowercases at load so the stored list is canonical
  in every consumer (status line, UI, etc.).

- **dirge-code#9 LOW — warn on unknown tool names in `deny_tools`**: at prompt
  load time, cross-check every `deny_tools` entry against a
  `KNOWN_TOOLS` list. Warns once per unknown entry, with the full
  known set printed for guidance. MCP-server-exported tool names
  will trigger this benignly; documented inline.

- **#3 HIGH — pin order with tests**: three new checker tests pin
  the contract:
  - `prompt_deny_any_matches_concrete_and_qualified_mcp_names`
    (locks the MCP bypass fix)
  - `prompt_deny_is_case_insensitive`
  - existing tests continue passing

- **#4 plugin trust boundary documented**: per the review's
  recommendation, added a "Plugin trust boundary" section to
  CONFIG.md acknowledging that plugins are inside the trust
  boundary and not sandboxed.

Not addressed (intentional):
- dirge-code#8 doom-loop UI nudge on repeated deny-list hits — UX polish.
- dirge-code#10 `/prompt default` clear confirmation — user-typed; would
  have to confirm every clear, including the legitimate ones.

634 tests pass; fmt clean.
allen-munsch referenced this pull request in allen-munsch/dirge Jun 3, 2026
…ry, repo_overview walk

Self-review of the last 5 commits surfaced 9 findings. Addressing all.

- **#1 MEDIUM — `repo_overview` permission patterns got command-glob
  semantics**: `is_path_tool_name` was updated for `glob` but not
  `repo_overview`, so a config rule like `"repo_overview": {
  "/etc/**": "deny" }` parsed `**` as command-glob (no path-spanning
  match). Added `repo_overview` to the path-tool arm.

- **#2 MEDIUM — `providers.<name>` override silently missed on
  capitalised provider names**: `parse_provider` is case-insensitive
  but `HashMap::get(provider)` is not, so `--provider Anthropic`
  built the client fine but the `providers.anthropic` chunk-timeout
  override was a no-op. Lowercased the lookup key in
  `resolve_stream_chunk_timeout` and `resolve_provider_info` (with
  the original key as a fallback so existing exact-case configs
  keep working).

- **#3 MEDIUM — `Prompt.description` parsed but never displayed**:
  the frontmatter `description: "..."` was being captured into a
  `#[allow(dead_code)]` field. A user writing a description expected
  it to appear somewhere. Wired into the `/prompt` list output —
  each prompt now renders as `name  description` (padded for
  alignment) when a description is set, falling back to the bare
  name otherwise.

- **#4 MEDIUM — `stream_chunk_timeout_secs` undocumented**: the
  knob landed in 186628b with the error message pointing users at
  it, but neither CONFIG.md nor README explained the resolution
  ladder. Added a "Streaming timeouts" section to CONFIG.md
  covering the precedence order (custom_providers > providers >
  top-level > 300s default) and noting the case-insensitive
  matching.

- **dirge-code#5 LOW — Unicode lowercase mismatch with ASCII-only matcher**:
  the frontmatter parser called `to_lowercase()` (Unicode-aware)
  while `is_prompt_denied` used `eq_ignore_ascii_case`. Swapped
  the parser to `make_ascii_lowercase` so both ends share the same
  contract — non-ASCII bytes pass through unchanged on both sides.

- **dirge-code#6 LOW — MCP bare-name deny semantics undocumented**: the
  3-name probe `any_prompt_denied(&[concrete, qualified,
  "mcp_tool"])` matches an MCP server's `edit` tool when the
  prompt denies `edit` (intended for the built-in). README now
  spells this out and recommends the qualified
  `mcp_tool:<server>:<name>` form for surgical denies.

- **dirge-code#7 LOW — `KNOWN_TOOLS` duplicated `BUILTIN_TOOL_NAMES`**:
  two hand-maintained lists of every built-in tool, used for the
  MCP collision filter and the frontmatter `deny_tools` warn
  respectively. Drift would produce either spurious warnings or
  (worse) an unsafely shadowable name. Extracted a single
  `pub const BUILTIN_TOOL_NAMES` in `agent/tools/mod.rs`; both
  sites now import it.

- **dirge-code#8 DESIGN — ACP can't switch prompts mid-session**: the
  4dd17d7 fix installed the deny-list correctly per-request but
  ACP has no protocol message for `/prompt <name>`, so the deny-
  list is effectively locked at boot. Documented in the README
  ACP bullet — recommends `--prompt <name>` at launch for
  restricted modes.

- **dirge-code#9 LOW — `repo_overview` ancestor walk went above crawl root**:
  `compute_dir_file_counts` bumped every parent up to `/`,
  including dirs that would never be printed. Now stops at the
  first ancestor not in the printed-dir set.

634 tests pass; fmt clean.
allen-munsch referenced this pull request in allen-munsch/dirge 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 referenced this pull request in allen-munsch/dirge Jun 3, 2026
…e migration, model-shrink, unicode tokens

Self-review of the last 6 substantive commits flagged 7 findings.
Addressing all in one batch.

- **#1 HIGH — Accept mode silently bypassed mcp_tool's default-Ask**:
  `SecurityMode::Accept` coerced `Ask → Allow` for every non-path
  tool, so the new default-Ask rule for `mcp_tool` (f62a89b) was
  effective only in Standard mode. `dirge --accept` + an MCP
  server still allowed every call silently. Added
  `is_high_risk_non_path_tool(tool)` (matches `mcp_tool` and
  `bash`) which forces the Ask to survive the coercion. The
  general non-path coercion still applies to `question`, etc.
  Regression test pins both directions.

- **#2 MEDIUM — Pre-5dc036d sessions resumed with under-counted
  `estimated_tokens`**: stored values were computed under the
  old text-only logic. Bumped `SCHEMA_VERSION` to 2 and added a
  v1 → v2 migration step that calls a new
  `Session::recompute_all_estimates` (also exposed
  `estimate_message_tokens` as the per-message helper). Tested
  via `v1_to_v2_recomputes_under_counted_estimates`.

- **#3 MEDIUM — `/model` switch to a smaller window didn't warn
  about over-capacity**: switching from a 1M to a 200k model
  with the session already exceeding the new budget would have
  errored mid-stream on the next prompt. The /model handler now
  surfaces a warning recommending `/compress` when
  `total_estimated_tokens > new_ctx - reserve`.

- **#4 LOW — Highlight tokenizer mis-split non-ASCII identifiers**:
  `bytes[i] as char` for a UTF-8 lead byte produced a Latin-1 char
  that failed `is_ascii_alphanumeric`, terminating the identifier
  mid-word (`naïve` → `na` + punctuation + `ve`). Walk via
  `line[i..].chars().next()` and broadened `is_ident_cont` to
  accept any non-ASCII non-control letter. ASCII path unchanged.

- **dirge-code#5 LOW — `looks_like_type` colored short capitalized words as
  types**: `Ok`/`No`/`Hi`/`Id` all hit. Tightened the floor to
  ≥3 chars. Added `Ok`/`Err`/`Some`/`None` to the Rust types
  table so idiomatic 2-char Rust constructors still get type
  color via the explicit table.

- **dirge-code#6 INFO — Yolo bypass documented**: README permission section
  now says explicitly that `--yolo` skips rule eval, the
  per-tool default-Ask, and the doom-loop detector — but that
  `deny_tools` frontmatter STILL applies (that gate runs BEFORE
  the yolo short-circuit by design). Accept-mode bullet also
  updated to note `bash` and `mcp_tool` keep their Ask.

- **dirge-code#7 + dirge-code#8 CLEARED**: doom-loop Allow + `"*": "allow"` default
  config both verified NOT to bypass mcp_tool's Ask.

684 tests pass; fmt clean.
allen-munsch referenced this pull request in allen-munsch/dirge Jun 3, 2026
Audit response covering the user's 6-item duplication list. Three
substantive modules + thin wrappers; two items deferred as
non-issues; one already complete from earlier commits.

**#4 — ANSI / control-byte filter (new `src/ui/ansi.rs`)**:
Previously three independent filters: MCP forwarder (`emit_mcp_line`),
websearch (`strip_tags_and_decode`), chat (`sanitize_output`). Each
drifted in coverage — one blocked C0 but not C1, another stripped
`\r` only, etc. New `ansi::strip_controls(s, policy)` with a
`StripPolicy { keep_newline, keep_tab }` knob is the single source
of truth. MCP forwarder + websearch routed through it. Chat
`sanitize_output` left as-is — it does ANSI-escape PARSING (consumes
`\x1b[…m` as a unit so the payload disappears) which is more
specific than what `strip_controls` does (drops just ESC, leaving
`[31m` as visible text). The two coexist intentionally.

**#1 — Reusable box component (new `src/ui/box_render.rs`)**:
Three implementations of chamber/box math previously: tool
chambers (`chamber_row` / `chamber_row_with_bg` / `chamber_bottom`),
permission alert (inline `row` closure), panel sections
(`push_section` closure). Inconsistent — `chamber_row` was
display-width-aware, `chamber_row_with_bg` was char-count-based;
each treated tabs and width math differently.

New module:
  - `BoxStyle` enum (currently only `Rounded`)
  - `top(style, title, total_w)`,
    `bottom(style, total_w)`,
    `divider(style, total_w)` — frame primitives
  - `row(style, content, total_w)` — display-width-aware
    content row with tab expansion + truncate-with-`…`
  - `row_with_bg(style, content, total_w, bg_idx)` — for diff
    backgrounds
  - `expand_tabs(s, tab_stop)` — moved here from `mod.rs`
  - `BoxBuilder` — fluent API for callers that build a box
    all-at-once (notifications, alerts, panel sections). Long
    rows soft-wrap via `wrap::soft_wrap` instead of truncating.

`chamber_row`, `chamber_row_with_bg`, `chamber_bottom` in `mod.rs`
are now thin wrappers around `box_render`'s primitives — existing
call sites unchanged, but the underlying math is shared. 7 new
unit tests covering frame width invariants, tab handling, CJK,
builder construction, and soft-wrap.

**#2 — Single output chokepoint** — already done in commit
`ea042b1` (the `ui::notifications` module + channel). The
remaining stray `eprintln!` sites all run during STARTUP (config
parsing, skill discovery, MCP connect_all) BEFORE `TerminalGuard`
is installed, so they don't paint over the UI. Audited and
confirmed clean.

**#3 — Permission check chokepoint** — already centralized.
`check_perm` / `check_perm_path` / `check_perm_path_resolve` in
`agent/tools/mod.rs` cover every tool's input-checked path;
`any_prompt_denied` covers MCP-style multi-name lookup;
`check_prompt_deny` covers plan tools. No duplication worth
extracting.

**dirge-code#5 — Layout module** — deferred. `chamber_widths`,
`Renderer::content_width`, `Renderer::line_width`,
`Renderer::max_line_width` are already in their proper homes;
forcing them into a new module would be churn, not clarification.

**dirge-code#6 — Soft-wrap inside chambers** — deferred. The current
chamber rows truncate with `…` (matches user expectation for
single-row tool result lines); the `BoxBuilder` path soft-wraps
when the caller wants that explicitly. Forcing all chamber rows
to soft-wrap would change the visual feel of tool output and
needs a separate UX call.

709 tests pass (702 + 7 box_render); fmt clean.
allen-munsch referenced this pull request in allen-munsch/dirge 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 referenced this pull request in allen-munsch/dirge Jun 3, 2026
…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.
allen-munsch referenced this pull request in allen-munsch/dirge Jun 3, 2026
…rupt on crash

External review flagged that `write.rs:105`, `edit.rs:234`, and
`apply_patch.rs:93,155` all called `tokio::fs::write` directly,
which opens with O_TRUNC and writes in-place. A crash between the
truncation and the final byte (power loss / OOM-kill / SIGKILL /
panic) leaves the file corrupted with no recovery. The irony:
`session/storage.rs` already had the correct pattern — temp +
fsync + rename — but it wasn't shared.

Extracted the pattern into new top-level module `src/fs_atomic.rs`:
  - `atomic_write_sync(path, content)` — sync, used by storage
  - `atomic_write(path, content)` — async, used by tools (delegates
    to spawn_blocking so the create + fsync + chmod + rename
    sequence runs atomically in one blocking task)
  - `next_temp(target)` — hidden sibling temp path with
    pid+nanos+counter nonce so two concurrent saves don't collide
    on the temp filename (counter is the load-bearing piece —
    same-nanosecond firings still get distinct names)
  - Unix mode preservation: stat the existing target's perms BEFORE
    rename, chmod the temp to match. Without this, an atomic
    overwrite of an executable script would silently drop the +x
    bit (default temp perms are 0644 minus umask).

Migrated four call sites:
  - `agent/tools/write.rs:105`     — full-file write
  - `agent/tools/edit.rs:234`      — edit-tool output
  - `agent/tools/apply_patch.rs:93`  — apply_create
  - `agent/tools/apply_patch.rs:155` — apply_update
Plus `session/storage.rs` now also uses the shared helper (was
the original site with the pattern; now consolidated).

Return type: `io::Result<()>` so existing `From<io::Error>` impls
on `ToolError` / `anyhow::Error` continue to work. The async
variant maps spawn_blocking join failures to `io::Error::other`.

Six new regression tests:
  - `atomic_write_creates_new_file`
  - `atomic_write_overwrites_existing`
  - `temp_is_hidden_sibling` — verifies same-fs + dot-prefix
  - `next_temp_is_unique` — 1000-call distinct-name check
  - `atomic_write_preserves_mode` (Unix) — +x stays on across
    overwrite
  - `target_untouched_on_failed_rename` — atomicity guarantee

Tests use `std::env::temp_dir` + a `TestDir` RAII helper (matches
the codebase convention; dirge doesn't pull in `tempfile`).

724 tests pass (718 + 6); fmt clean.

#2 (ToolStarted event), #3 (prepareNextTurn hook), #4 (structured
tool output) follow in separate commits.
dirge-code#5 from the review was a false positive — `skill` IS registered at
`builder.rs:239`.
allen-munsch referenced this pull request in allen-munsch/dirge Jun 3, 2026
External-review #3 and #4 implemented as minimal, honestly-scoped
versions.

**#3 — `prepare-next-run` plugin hook**

New hook fires AFTER `Done` (run complete) and BEFORE the next
user prompt is processed. Plugins read this to signal session-
level state changes for the next run. Currently the only
supported mutation slot is `harness-next-model` (Janet:
`(harness/set-next-model "claude-opus-4.7")`).

Scope honesty: the request is SURFACED to the user as a
notification (`"[plugin] requested model swap to 'X' — apply
with /model X"`) rather than auto-applied. Auto-apply is
deferred because:
  - The agent rebuild path is non-trivial across cfg-feature
    combinations.
  - The existing `/model` slash already does it correctly.
  - "Plugins propose, user disposes" is the safer default —
    a plugin can't silently swap to a more expensive model
    without the user noticing.

Mid-stream model swap is explicitly UNSUPPORTED — rig's
multi-turn stream owns state that doesn't survive a swap. The
hook is scoped to between-runs only, documented in the comment
next to the slot.

Slot infrastructure:
  - `harness-next-model` declared in `worker.rs` startup blob
  - `harness/set-next-model` helper in the same blob
  - `take_pending_next_model()` on `PluginManager` clears the
    slot and returns its value

**#4 — `ToolContent` classification on `ToolResult`**

New enum `event::ToolContent { Text, File }`. Added as an
additive field on `AgentEvent::ToolResult { id, output, kind }`.

`output: CompactString` remains the authoritative payload for
the LLM and the default UI rendering path — `kind` is purely
metadata for richer consumers (ACP resource links, future UI
file-card components).

The runner classifies by tool name: `read` / `find_files` /
`list_dir` produce `File`, everything else `Text`. Tracked via
a per-stream `id → name` HashMap populated at each `ToolCall`,
drained at the matching `ToolResult` (1:1 call/result pairing
within a turn).

Coarse on purpose — no per-tool `type Output` change required
across ~20 tools. A future refactor could thread the variant
through the rig `Tool` trait for finer-grained control.

Consumers:
  - `extras/acp/mod.rs` reads `kind` (currently no-op; comment
    flags `ResourceLink` migration as a follow-up)
  - `ui/mod.rs` uses `{ .. }` rest pattern; ignores `kind` for
    now

**dirge-code#5 from the review remains a false positive** — `skill` IS
registered at `builder.rs:239`.

724 tests pass; all-features build clean.
allen-munsch referenced this pull request in allen-munsch/dirge 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 referenced this pull request in allen-munsch/dirge Jun 3, 2026
Closes the gap the article calls out: open models (DeepSeek-flash,
v4-pro, GLM, Qwen) produce a small finite set of tool-call shape
mistakes that strict JSON Schema validation rejects without a hint
the model can recover from. With this layer in place the article
author saw DeepSeek v4-pro beat Opus 4.7 6/10 on their evals.

What lands

* Phase 1 — validate-then-repair layer
  (src/agent/agent_loop/tool_input_repair.rs, 535 lines + 36 tests)
  Wraps `prepare_tool_call` (tools.rs:194-232). Valid inputs pass
  through untouched; on schema failure, walks each issue path and
  applies the four shape repairs in this exact order:
    1. Null-strip for optional fields
    2. JSON-string-as-array  (MUST run before #4)
    3. Empty-object-to-array
    4. Bare-string-to-singleton-array
  The ordering invariant — `'["a","b"]'` becomes `["a","b"]`, NOT
  `['["a","b"]']` — is pinned by ordering_json_string_before_bare_string.

* Phase 2 — markdown auto-link unwrap for path fields
  Schema-driven via the known path-field name set (`path`,
  `file_path`, `filename`, `paths`, `dir`) plus an opt-in
  `x-dirge-kind: "path"` annotation. Only the degenerate case
  (`[notes.md](http://notes.md)` where link-text equals
  url-without-protocol, or is a suffix of the URL path) is unwrapped;
  real markdown like `[click](https://example.com)` passes through.
  Never applied to content/text fields — verified by
  md_unwrap_only_path_fields_via_validate.

* Phase 3 — read_file relational defaulting
  When only one of `offset`/`limit` is provided, fill the other
  (limit → offset=line 1, offset → limit=2000) and surface the
  choice as a `Note:` line in the result body (not `Error:` — TUI
  doesn't paint Note red). Phrasing uses 1-indexed line numbers to
  match the schema description. 4 new tests cover all combinations.

* Phase 4 — defense-in-depth error formatting
  `format_structured_error` produces a model-readable
  Tool/Expected/Got/Try block instead of raw `serde_json::Error`
  diagnostics. `format_tool_error` (rig_tool.rs:192) wraps any
  leak through with the same structured retry hint.

* Phase 5 — `(model, tool, repair_kind)` telemetry
  `tracing::info!(target: "tool_repair", model=…, tool=…,
  repair=…)` fires on every repair (success or failure). Picked up
  by `RUST_LOG=tool_repair=info` or `--verbose`. Threads model_name
  through `AnyAgent` → `LoopSpawnConfig` → `LoopConfig` so per-(model,
  tool) regression detection works.

Review fixes from the first pass

* B1 — Note text uses "line 1" / "1-indexed" phrasing (matches the
  schema description; avoids the model retrying with `offset=0` and
  hitting a different cache key).
* B2 — `navigate_schema` descends into array items via numeric
  pointer segments, so the structured-error `Expected:` line shows
  the per-item schema for nested-array tools instead of falling
  back to "(see tool schema)".
* B4 — Removed redundant `strip_null_optionals` top-level call;
  `strip_null_recursive` already handles Object and Array roots.

Two new tests pin B2: `navigate_schema_descends_into_array_items`
and `structured_error_uses_array_item_schema`.

Skipped (deferred)

* B3 — `args.clone()` on every call is wasted for valid inputs. Would
  require deferring content-normalizers behind validation; not a
  correctness issue, just a perf opportunity. Most tool args are small.
* Schema-vs-serde divergence (e.g. JSON Schema `integer` accepts
  negative; Rust `Option<usize>` rejects) — tool-author responsibility;
  add `"minimum": 0` to schemas for unsigned fields.

Plus

* docs/DEEPSEEK_TOOL_INPUT.md — full writeup of findings + the
  5-phase plan, with file:line citations into this code.

Verified

* cargo test --bin dirge --features plugin  → 1215 passed
* cargo test --bin dirge                    → 1001 passed
* cargo build --bin dirge --features semantic-elixir → clean
* cargo fmt --all --check                   → clean
allen-munsch referenced this pull request in allen-munsch/dirge Jun 3, 2026
…tests

SESS-2 follow-up #1 — UI session-mutation on ContextCompacted:
ContextCompacted event now carries summary + first_kept_index. The
UI consumer mutates session.id in-place, calls
Session::compress_reporting() to push a Compaction entry, and runs
save_session() so the rotated id and summary are persisted on disk.
Mirrors hermes-agent/conversation_compression.py lines 380-397.
Without this the on-disk session kept the OLD id and the
compaction was lost on next resume.

SESS-2 follow-up #4 — /compress <focus> argument wire-through:
- build_summary_prompt now honors focus_topic: when supplied, the
  Hermes-style "FOCUS TOPIC: …" framing is appended to the prompt,
  asking the model to allocate ~60-70% of its summary budget to
  the topic (verbatim port of hermes context_compressor.py:1050-1054).
- compress_messages (existing slash-command path) gets the same
  treatment: any free-form text after /compress is wrapped in the
  FOCUS TOPIC framing instead of the generic "Additional
  instructions" placeholder.
- run_compaction_pass exposes the focus parameter via a new
  with_focus wrapper (auto-trigger path still uses None).
- Slash help text updated: "/compress [focus]   compress; focus
  text guides what to preserve".

SESS-2 follow-ups #2 (background spawn) and #3 (multi-generation
chaining) closed as "matches reference impl": hermes is also inline
(no background spawn) and only chains the most-recent prior summary
via _find_latest_context_summary. Our implementation already matches.

H7_SMOKE: remove the 6 #[ignore] markers from the real-API
integration tests. Each test already has a runtime
`detect_provider()` check that bails with `[skipped]` + Ok when no
provider key is set; #[ignore] was blocking that check from ever
running. Removing the markers means: in CI without keys, the
tests run, hit the skip path, pass (1713 pass / 0 fail / 0
ignored). With keys present, they exercise the real provider as
designed. Header doc updated.

Tests: 1713 pass / 0 fail / 0 ignored (was 1707 / 0 / 6).
allen-munsch referenced this pull request in allen-munsch/dirge Jun 3, 2026
…contract hints

Per docs/AGENTIC_LOOP_PLAN.md. Three of the four Phase-1 items;
item #4 (per-tool-call reassembly timeout in rig_stream) is more
invasive and lands separately.

**1. Repair-rate counters**

New `RepairStats` (per-RepairKind `AtomicU64` + `invalid` counter)
threaded through `LoopConfig::repair_stats`. `prepare_tool_call`
increments on each successful repair AND on repair exhaustion.
At AgentEnd, run.rs sends `LoopEvent::RepairStats { snapshot }`
when the snapshot is non-empty. Bridge translates to
`AgentEvent::RepairStats`; UI prints a one-liner:

    ⊕ repaired 3 input(s): 2 md-link, 1 null-strip; 1 invalid

Empty snapshots are skipped so clean sessions don't print
"repaired 0 inputs".

**2. Structured `tool_input_invalid` log**

When repair exhausts, the original args (up to 16 KiB) AND the
full validator error list land in a dedicated
`tracing::warn!(target: "tool_input_invalid", …)` event,
separate from the existing `tool_repair = "failed"` info log.
Structured-log consumers can filter on the target directly.

**3. `with_contract_hint` helper**

Centralises the "absolute path, not a markdown link" / "plain
UTF-8 string, not a JSON object" cues that built-in tools were
each writing into their own descriptions. Wired into the 10
built-in tools' `Tool::definition` impls:

    description: with_contract_hint(
        "read",
        "Read the contents of a file. …",
    ),

Tools without a registered hint get the base description back
unchanged.

Future Phase-1 dashboard / per-(model, tool) breakdown can read
the existing tracing fields (`model`, `tool`, `repair`,
`original_args`) without further code changes — the aggregate
counter is the visible-at-session-end summary, the tracing
events are the offline-analysis surface.

Build clean, no warnings. Full test suite: 1718 pass / 0 fail
/ 0 ignored.
allen-munsch referenced this pull request in allen-munsch/dirge Jun 3, 2026
…eams

When a provider stalls emitting `ToolCallDelta` events mid-tool-
call (a common DeepSeek failure mode the catalogue §2.2 calls
out), the broad `stream_chunk_timeout_secs` (default 300s) is
the wrong knob — it conflates "long reasoning gap" with
"reassembly is stuck." This narrows the effective timeout to
30s ONLY while a tool call is mid-assembly.

Implementation in `wrap_streamed_assistant`:

- Track `open_tool_calls: HashSet<String>` of tool calls whose
  `ToolCallEnd` hasn't fired.
- Insert on first `ToolCallDelta` for an internal_call_id;
  remove on the `ToolCall` arm's `ToolCallEnd` emit.
- In the per-chunk timeout selection, when `open_tool_calls`
  is non-empty, narrow to `min(configured, 30s)`. When
  `chunk_timeout` is None but a tool call is open, still
  enforce the 30s cap.
- Error message explains the narrowing so the user knows it's
  the tool-call-gap path, not the broad chunk timeout. Still
  contains "timed out" so `recovery::classify_error` routes it
  to Network for retry.

Test `tool_call_gap_timeout_fires_within_30s_even_with_large_chunk_timeout`
pins the behavior: 300s configured + ToolCallDelta + stall →
timeout fires at 31s with the mid-assembly explanation.

Phase-1 of docs/AGENTIC_LOOP_PLAN.md now complete:
1. ✅ Repair-rate aggregate counters (RepairStats + RepairStatsSnapshot)
2. ✅ Structured tool_input_invalid log target
3. ✅ with_contract_hint helper wired into 10 built-in tools
4. ✅ This — tighter per-tool-call reassembly timeout

Full test suite: 1719 pass / 0 fail / 0 ignored.
allen-munsch referenced this pull request in allen-munsch/dirge 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).
allen-munsch referenced this pull request in allen-munsch/dirge Jun 3, 2026
…asonix parity

Independent verification turned up 6 gaps in the Phase 2.5 parity work.
This commit closes all of them.

#1 HIGH — `truncations_fixed` now bumps on hard-fallback too. Reasonix
   counts both success (`repair/index.ts:105`) and unrecoverable
   (`repair/index.ts:99`) under the same counter; dirge was dropping
   the latter, under-reporting exactly the cases operators need most.
   `apply_truncation_repair` now records the kind whenever the closer
   ran, not just on successful repair.

#2 MEDIUM — closer notes are now surfaced to the model. Reasonix
   pushes `r.notes` into `report.notes` with `[<tool>]` prefix on
   success and `[<tool>] ⚠️ TRUNCATION UNRECOVERABLE: ...` on
   fallback (`repair/index.ts:100-101, :106`), then carries them
   into the next-turn assistant input. Dirge now stashes them
   per-call-id on a new `LoopConfig.truncation_notes` shared map;
   `prepare_tool_call` drains them and appends to `repair_notes`,
   which `prepend_notes_to_result` (already in place for
   relational-default notes) prepends to the tool result content
   so the model sees the repair in the same turn.

#3 MEDIUM — added end-to-end wiring tests through `run_agent_loop`.
   The prior 7 tests proved the helpers worked in isolation; they
   did not prove the loop calls them in the right order. Two new
   tests drive the full canned-stream loop:
   - `dirge_7bwx_end_to_end_storm_dedupes_after_truncation_repair`:
     three tool calls with different truncated raw strings that
     heal identically. Storm threshold=3 → the third must be
     suppressed (only possible if truncation runs before storm).
   - `dirge_ngic_end_to_end_orphan_dsml_in_text_dispatches`:
     DSML invoke in `ContentBlock::Text` ONLY (no Thinking, no
     declared ToolCall) must dispatch (only possible if
     `build_scavenge_source` includes Text).

#4 MEDIUM — removed dead `try_truncation_repair`. It was kept as
   "defense in depth" but marked `#[allow(dead_code)]`, so the
   safety claim was illusory. Now actually gone; direct callers
   can use `repair_truncated_json` for the brace-closer if needed.
   The `validate_and_repair` block-comment was updated to reflect
   the new contract.

dirge-code#5 LOW — `truncation_repair_canonicalizes_divergent_streams_before_storm`
   tested canonicalization in isolation; the new end-to-end #3 tests
   exercise the actual storm dedupe path that depends on the
   String→Object promotion. The promotion itself is now also
   covered with an explicit note in `apply_truncation_repair`'s
   doc — it has no Reasonix analog (their args are always strings)
   and is dirge-specific compensation for mixed arg representations.

dirge-code#6 LOW — added a comment near `storm.rs::inspect` documenting the
   implicit dependency on `serde_json` being built without the
   `preserve_order` feature. If feature unification ever enables
   it, storm dedupe regresses silently; the comment points at the
   workaround (`run::canonical_json`) and notes Reasonix has the
   same fragility at `repair/index.ts:127`.

`LoopConfig` gained the new `truncation_notes` field; all
constructors (production + tests) were updated. `Clone` impl
threaded through.

1632 tests pass with `-D warnings` (was 1630; +3 new, -1 removed).
allen-munsch referenced this pull request in allen-munsch/dirge Jun 3, 2026
The pre-send snip (cap_oversized_tool_results) freed tokens but never
reported how many, so the post-response fold always fired at 75% even
when the snip had just bought plenty of headroom.

- Add `cap_oversized_tool_results_counted` — wraps the unchanged capper
  and reports tokens freed (measured with the same estimator the fold
  decision uses). Existing callers stay on the Vec-returning fn.
- run_loop tracks `snip_tokens_freed` from the (now tiered) cap site.
- In the post-usage Fold path, a pure `snip_bought_enough(freed,
  ctx_max, aggressive)` skips a NORMAL fold when the snip freed
  > SNIP_SUFFICIENT_FRACTION (10%) of the window. Aggressive /
  force-summary folds still fire. The credit resets after each
  post-usage decision so a stale snip can't suppress a later fold.

Tests: snip_bought_enough gating (normal vs aggressive, <10%, div-0
guard) and cap_counted freed-token accuracy. 2138 pass at -D warnings.

Stacked on the aggressive-prune branch (PR dirge-code#221).
allen-munsch referenced this pull request in allen-munsch/dirge Jun 3, 2026
…-feedback-loop

feat(agent-loop): snip-tokens-freed feedback loop (#4)
allen-munsch referenced this pull request in allen-munsch/dirge Jun 3, 2026
From the review of PR dirge-code#231:

- #1 (run.rs): the cap notice's new_messages.push is a legitimate part of
  run_agent_loop's returned message list, but the comment overclaimed that
  headless/subagent collectors and resumed models read it — they drive display
  from the LoopEvent stream and discard the return value. Comment corrected to
  describe it as a return-value contract nicety, not the display mechanism.

- #2 (provider/mod.rs): headless run_print dropped SystemNotice into its
  catch-all, so a --print run hitting the turn cap reported a clean success with
  no truncation signal. It now prints the notice to stderr.

- #3 (text_output.rs): documented the deliberate divergence between the live
  <system>/warning-color notice and persisted <sys>/system-color session
  history, cross-referencing render_session.

- #4 + altitude (text_output.rs): extracted write_prefixed_lines() shared by
  write_user_lines and write_system_lines, removing the copy-paste and making
  blank/empty-line handling identical by construction.
allen-munsch referenced this pull request in allen-munsch/dirge Jun 3, 2026
Addresses the six findings from the post-merge review of the
harness/lsp API (PR dirge-code#245).

#1 (dirge-5qqo) Load-time deadlock. The LSP responder used to spawn
after the plugin-load loop, so a plugin querying LSP at load time
blocked the worker forever on a reply with no drainer. Build the
LspManager standalone (`build_lsp_manager`) and wire the responder
BEFORE plugins load. On the current_thread runtime a load-time call
still can't be serviced while the loader blocks, but it now falls back
(see dirge-code#6) instead of hanging forever, and the disabled case returns nil
instantly. Documented the load-time caveat alongside dialogs.

#2 (dirge-m0zm) Honest availability. `(harness/lsp?)` checked only
compile-time symbol existence, so with LSP disabled at runtime it
reported available and queries silently returned nil (crashing plugins
that then json/decode). New `harness/__lsp-live` C-fn reports liveness
via the request channel's `is_closed()`, so the predicate reflects a
real, wired bridge.

#3 (dirge-38q7) Coordinate validation. `harness/lsp` now asserts
line/char are positive 1-based integers (surfacing a plugin bug) rather
than `read_uint_arg` silently clamping negatives/NaN to line 0.

#4 (dirge-d098) Shared dispatch. Extracted `lsp::query` (Operation +
parse + run) so the `lsp` tool and the harness share one op→method
match and one coordinate-conversion point. Removes ~140 lines of
duplication and the drift that left the harness without the tool's
goToDefinition/findReferences aliases.

dirge-code#5 (dirge-lfxd) Cheap diagnostics. Added `LspManager::diagnostics_for`
(O(one file)); the harness no longer clones the whole project
diagnostic map per single-file query.

dirge-code#6 (dirge-eehk) Query timeout. `send_lsp` is now bounded by a 30s
`LSP_QUERY_TIMEOUT` (via the testable `lsp_should_abort` helper) so a
wedged language server returns nil instead of freezing the worker
thread.

Tests: liveness predicate false on dropped receiver; query nil on
dropped receiver; nonpositive-coordinate rejection; abort-decision
helper; operation-alias acceptance; Operation parse/needs_position.
Full feature matrix green at -D warnings; default 2193, all-features
2214 tests pass.
allen-munsch referenced this pull request in allen-munsch/dirge Jun 3, 2026
…de#356)

* refactor: now_unix_secs/nanos + LockExt::lock_ignore_poison helpers [dirge-xhoo]

#4 time_util: SystemTime::now().duration_since(UNIX_EPOCH).map(|d| d.as_X())
.unwrap_or(0) was hand-rolled ~16 times (secs + nanos). Hoisted to
crate::time_util::now_unix_secs() / now_unix_nanos(); converted the uniform
multi-line sites. The variant forms (file-mtime, subsec_nanos as u64,
non-.map error handling) are genuinely different and left as-is.

#lock: the cryptic .lock().unwrap_or_else(|e| e.into_inner()) poison-recovery
idiom appeared 123 times across 43 files. Replaced with a named
LockExt::lock_ignore_poison() extension method (crate::sync_util) so the
intent ('dirge never relies on poison') is legible at every call site.
All sites are std::sync::Mutex; behavior identical.

2456 default / 2550 all-features tests pass (incl. a new poison-recovery
test); clean under -D warnings in both default + all-features.

* fix: allow(unused_imports) on LockExt imports for minimal feature configs

The windows-default config (--no-default-features) gates out the feature
blocks (mcp/plugin/lsp/…) that hold many lock_ignore_poison() call sites,
leaving the LockExt import unused there. The trait is genuinely
conditionally-used per feature set, so annotate the imports. Verified with
cargo build --no-default-features --features windows-default.

* test(dap): fix flaky dap_perm_check_roundtrips race on the DAP_PERM_CHECK global

The two tests that mutate the process-global DAP_PERM_CHECK static used
per-statement locks, so under parallel execution no_perm_check_when_none
could set None between dap_perm_check_roundtrips' write and read-back —
failing read_back.is_some() intermittently. Both tests now hold the global's
mutex for their whole body (one critical section), serializing them on that
lock so they can't interleave. Uses lock_ignore_poison for poison-robustness.
Stress-ran 25x clean.

* refactor: convert remaining multi-line .lock().unwrap_or_else(into_inner) stragglers

The single-line perl in the prior commit missed the multi-line form
(.lock() and .unwrap_or_else() on separate lines) — ~32 sites. Converted
them to lock_ignore_poison() too (+ LockExt import in the 3 files that had
ONLY the multi-line form). The 3 remaining unwrap_or_else(into_inner) sites
are RwLock .read()/.write() (notifications.rs) — a different lock type, left
as-is (not worth a second ext-trait for 3 sites). All configs build clean.

---------

Co-authored-by: Yogthos <yogthos@gmail.com>
yogthos added a commit that referenced this pull request Jun 18, 2026
* memory: confidence axis + contradiction supersession

Re-introduce a confidence (truth-likelihood) column on memories, dropped
as write-only in v9 (dirge-lerb). This time it's genuinely read: it folds
into the eviction effective-salience (a tiebreak among equal-salience
entries, bounded so it never jumps the kind hierarchy), breaks ties in
search ordering, is surfaced in view meta, and is annotated in the
curator render so the curator can act on contested facts.

The producer is supersession. A new 'supersede' action retires a
contradicted fact to status='superseded' (kept as an audit record via
superseded_by/superseded_at, excluded from the snapshot/views/search/
eviction like a tombstone) and writes the corrected fact as a new entry.
'harsh' distinguishes a flat denial (successor held at reduced
confidence, 0.5) from a natural update (0.7). The background review pass
drives it on contradictions; supersede is in the tool enum but kept off
SYSTEM_PROMPT. Supersession is terminal by design — recovery is to re-add.

Schema v13. add_entry's hot-tier compaction is extracted to a shared
insert_into_hot so supersede reuses the working-reserve rules. v9 dropped
confidence and v13 re-adds it, so the chain round-trips to a fresh
default; the v9 test now covers that.

Idea #4 from the Elastic agent-memory writeup, bundled with supersession
per the dirge-lerb lesson that a bare confidence column gets reverted.

bd: dirge-fa10

* bd: track PR #437 on dirge-fa10

---------

Co-authored-by: Yogthos <yogthos@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant