Skip to content

fix: token counter uses full_input after multi-line rename - #8

Merged
yogthos merged 1 commit into
mainfrom
fix/token-counter-build
May 19, 2026
Merged

fix: token counter uses full_input after multi-line rename#8
yogthos merged 1 commit into
mainfrom
fix/token-counter-build

Conversation

@yogthos

@yogthos yogthos commented May 19, 2026

Copy link
Copy Markdown
Collaborator

Summary

Test plan

  • `./build.sh` succeeds
  • `cargo check` clean

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.
@yogthos
yogthos merged commit 6373f79 into main May 19, 2026
1 check passed
yogthos added a commit that referenced this pull request May 21, 2026
…iag (#83)

Track F-HIGH #8 from ROADMAP.md.

## Problem

`load_session` (`storage.rs:98-104`) deserialized raw JSON with
no schema versioning. Two pain points:

1. **Corrupted files** (truncated mid-write before atomic save
   shipped, or external mutation) surfaced as
   `expected ',' or '}' at line N column M` with no file path.
   A user with many sessions had to grep to figure out which
   one was broken.
2. **No migration path** for future schema bumps. Field
   additions so far used `#[serde(default)]` so they migrated
   transparently, but a real shape change (renaming a field,
   restructuring `tree.entries`) would silently corrupt old
   sessions on first load with no way to detect.

## Fix

`Session` gains `pub schema_version: u32` with
`#[serde(default)]` so pre-F8 files load with `schema_version
= 0`. `Session::new` sets it to the current `SCHEMA_VERSION`
constant (initial value: 1 — first versioned schema).

`load_session` now:

1. Wraps the parse error with `path.display()` context so the
   user sees `failed to parse <path>: <serde error>`.
2. After successful deserialize, compares `session.schema_version`
   against `SCHEMA_VERSION`:
   - Lower → call `migrate_session(&mut session)` then bump.
   - Equal → no-op.
   - Higher (file from newer dirge) → warn via tracing but
     still load. Most fields default-migrate via serde; only
     truly new fields get default values.

`migrate_session` is a single function with version gates ready
for future schema work. The v0 → v1 step is a no-op (current
schema is structurally identical to pre-F8; only the field
introduction marks the version).

## Tests

Two new tests in `session::storage::tests`:

- `load_session_migrates_pre_f8_files`: writes a minimal pre-F8
  session JSON (no schema_version field) to disk, loads it,
  asserts schema_version is bumped to SCHEMA_VERSION and the
  data is intact.
- `load_session_corrupted_file_includes_path_in_error`: writes
  a truncated JSON, asserts the error mentions the path so the
  user can grep `dirge` logs and identify the broken file.

664 pass (was 662). 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 added a commit that referenced this pull request May 21, 2026
Follow-up to PR #111. Tier-2 items from the 23-bug audit batch:
docs corrections and two small correctness/UX fixes.

## Docs

- **#12 temperature** — CONFIG.md claimed "parsed but not
  currently applied". Actually applied since PR #105 with a
  clamp warning. Rewrote the cell.
- **#13 --api-key** — flag existed but neither README nor
  CONFIG.md mentioned it. Added a Quick-start example noting
  the process-list visibility caveat.
- **#14 acp_host/acp_port** — CONFIG.md documented both keys
  but the CLI flags were intentionally removed (stdio-only
  transport). Removed both from the keys table + ACP section.
- **#6 tools** — `Config::tools` (per-tool enable map) was
  fully wired in code but undocumented. Added a row to the
  keys table covering `tools.websearch` and `tools.webfetch`.
- **#21 find_callers** — README claimed "word-boundary regex"
  but the impl uses the tree-sitter symbol index. Updated to
  reflect actual behavior; the user-visible word-boundary
  semantics are preserved.

## Code

- **#16 semantic index skip_dir** — `SymbolIndex::find_callers`
  filter had its own hardcoded `matches!(name, "node_modules"
  | "target" | ".git" | "__pycache__")` while the rest of
  the codebase uses `agent::tools::is_skip_dir`. Switched to
  the shared helper so future additions stay in lockstep.

- **#18 context::load_file** — silently swallowed
  `read_to_string` errors via `.ok()`. A permission-denied
  AGENTS.md looked identical to a missing file. Now emits a
  stderr warning naming the path + reason; still returns None
  so callers' behavior is unchanged.

725 plugin / 599 default pass. All build profiles clean.

## Remaining audit items (deferred to feature work)

- **#8 LSP no crash restart**: needs broken-pipe IO error
  handling + exponential backoff. Touches manager state
  machine.
- **#10 task tool fire-and-forget**: needs timeout + cleanup
  coordination via JoinHandle tracking.
- **#25 MCP no reconnection**: similar architectural concern
  to #8.
- **#27 LSP didClose**: client lifecycle hook missing.
- **#29 token estimation len/4**: needs per-provider usage
  extraction (Phase 6 work).
- **#5 MCP shutdown**: rmcp Drop semantics need verification.
- **#38/39/40 semantic test gaps**: get_symbol_body untested,
  list_symbols kind_filter untested, find_definition test
  vacuous. Sat down to add but each requires a fixture build.

Together with PR #111 (10 code fixes), 17 of the 23 verified
items are now shipped. Remaining 6 are architectural or
test-infrastructure work better tackled as discrete PRs.

Co-authored-by: Yogthos <yogthos@gmail.com>
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
…nds, error masking

Adversarial review of `1c341e9`/`8e60553`/`69318b7` flagged 14
findings. Addressing all.

- **#1 HIGH — C1 controls bypassed MCP stderr sanitizer**: the
  original filter was `b == 0x09 || (0x20..0x7f).contains(&b) ||
  b >= 0x80`. That second clause let through every non-ASCII byte
  including the C1 control range (U+0080..=U+009F). In particular
  U+009B is single-byte CSI, behaves identically to `\x1b[` on
  iTerm2/xterm in 8-bit mode. A misbehaving MCP child could write
  `\u{9b}2J` and repaint the screen — exactly the smuggling vector
  the commit message claimed to close. Filter now blocks C0
  controls (except `\t`), DEL, and the full C1 range.

- **#2 HIGH — MCP stderr was silently dropped at default verbosity**:
  emit was `tracing::info!`, but dirge's default EnvFilter is
  `warn,rig=off`. Users diagnosing MCP server panics or init
  errors saw nothing — real regression vs the old
  `Stdio::inherit()`. Raised to `tracing::warn!` so it surfaces
  on the default config.

- **#3 HIGH — `parse_ddg_html` panicked on truncated input**:
  `&html[tag_start..abs_start + 32]` blew up when `abs_start +
  32 > html.len()` or landed mid-codepoint. Rewrote the scanner
  to anchor on `<a ` tags and walk only within bounded slices
  via `tag_end.min(html.len())`. Added regression test for the
  truncated case.

- **#5 MEDIUM — MCP stderr forwarder had no per-line cap**:
  `BufReader::lines()` buffers until `\n`. A buggy child writing
  a GB without newline would OOM dirge. Replaced with a manual
  read loop, 16 KiB per-line cap, emit `…[truncated]` past the
  cap and skip until next `\n`.

- **#6 MEDIUM — provider rotation race on first call**: two
  concurrent first-callers both saw `AtomicU8 = 0`, both rolled
  a fresh entropy pick, both stored. Last writer won —
  inconsistent contract. Switched to `compare_exchange` from 0
  to candidate; loser re-reads the winner's value.

- **#7 MEDIUM — both-providers-fail error masked secondary +
  DDG errors**: only `primary_err` was returned. Now
  concatenates all three failures so the user can diagnose
  without chasing the wrong cause.

- **#8 MEDIUM — `parse_ddg_html` false-positives on substring
  match**: previously matched `class="result__a"` anywhere in
  the HTML, including inside `<script>` blocks or quoted text.
  Walked backward via `rfind("<a ")` could grab an unrelated
  anchor. Now anchors on `<a ` first and inspects the tag's
  attributes — proper containment check.

- **#9 MEDIUM — DDG snippet control bytes flowed into LLM
  prompt**: `strip_tags_and_decode` decoded entities but didn't
  filter ESC / C1 controls. A malicious or mojibake search
  result could ship ANSI styling into the agent's context.
  Added control-byte filter to the decoder's output pass.

- **#10 LOW — `PARALLEL_API_KEY` read per-call**: was
  `std::env::var` inside `call`, inconsistent with how
  `EXA_API_KEY` was captured at construction. Moved to
  `WebSearchTool::new`.

- **#11 LOW — whitespace-only key passed empty-filter**:
  `EXA_API_KEY="  "` produced a malformed
  `?exaApiKey=%20%20` URL. Now trims keys at construction.

- **#12 LOW — tool description out of date**: still mentioned
  Exa-only + DDG fallback, missed Parallel.ai rotation and the
  keyless default. Rewritten to reflect the actual contract.

- **#13 LOW — `urlencode_query` renamed to `percent_encode`**:
  the function is a generic percent-encoder (RFC 3986
  unreserved set), not a form-encoder. Misleading name.

- **#14 DESIGN — no tests for new code paths**: added 11
  regression tests covering ddg parser bounds + happy path +
  anti-script-block, control-byte filter, key trim, MCP
  response parser (plain JSON + SSE + malformed), DDG redirect
  unwrap, percent encoding, provider env override.

- **#15 LOW — bot-identifying DDG User-Agent**: swapped
  `compatible; dirge-agent/1.0` for a real Firefox 133 UA. DDG
  aggressively rate-limits identifiable scrapers.

695 tests pass (684 + 11 new); fmt clean.
yogthos pushed a commit that referenced this pull request May 22, 2026
…h_bg width, sanitization

Correctness + security review of `dc21de7`/`cd701a8` flagged 15
findings. Addressing all 15.

- **#1 HIGH — startup race: MCP forwarders fired before
  `install()`**. `connect_all` spawns stderr forwarders in `main`
  BEFORE `run_interactive` reached the old `install()` call. Lines
  emitted during MCP-server handshake hit `sender() == None` and
  were silently dropped. Moved `install()` to the very top of
  `main()` so the channel is live by the time any forwarder
  starts. Split the API: `install()` (creates channel) +
  `take_receiver()` (UI loop claims the rx).

- **#2 HIGH — orphaned-sender footgun on UI restart**. `OnceLock`
  meant a re-entry could never replace the sender; producers
  holding clones would send into a dead channel forever. Switched
  to `RwLock<Option<Sender>>`. Producers also self-heal: when
  `try_send` returns Err because the receiver was dropped,
  `notify_send` clears the slot so subsequent producers see `None`
  and skip.

- **#3 HIGH — `row_with_bg` was still char-count-based**. The
  refactor claim was "unifies display-width vs char-count" but
  the bg-tinted variant still used `chars().count()`. A diff row
  with CJK / emoji drifted the right border. Now uses the same
  display-width budget as the plain `row`. Regression test
  `row_with_bg_width_invariant` pins it.

- **#4 HIGH — unbounded channel + no backpressure → OOM**. A
  buggy / hostile MCP child spamming stderr would grow the queue
  unboundedly. Switched to `mpsc::channel(1024)` (bounded) with
  `try_send` so the producer drops on overflow rather than
  unboundedly queuing. Test `bounded_channel_drops_on_full` pins
  the contract.

- **#5 MEDIUM — multi-colon MCP tool names**. `splitn(3, ':')` on
  `mcp_tool:server:do:thing` parsed correctly but the comment
  explanation was off. Clarified; behavior unchanged (the
  wildcarded server pattern is the desired semantics).

- **#6 MEDIUM — mcp_tool umbrella check case-sensitive**.
  `umbrella == "mcp_tool"` would miss `MCP_TOOL:…` if a future
  caller surfaces uppercase. Switched to `eq_ignore_ascii_case`.

- **#7 MEDIUM — receiver-side sanitization for ALL Notification
  variants**. MCP variant was pre-sanitized at the producer,
  but Info/Warn/Error had no producer-side contract. Adding
  receiver-side `ansi::strip_controls(KEEP_NEWLINE)` makes the
  rule un-bypassable: nothing reaches `write_line` carrying
  escape bytes regardless of how careful a future producer is.

- **#8 MEDIUM — websearch `KEEP_BOTH` + `\n` broke chamber
  border**. Tabs survived into chamber rows where they
  interacted poorly with the wrap math. Switched to
  `KEEP_NEWLINE` and replace `\t` with single space.

- **#9 MEDIUM — whitespace-only MCP lines dropped**. The
  blank-line collapse used `trim().is_empty()` which also ate
  legitimate indented continuation lines. Now uses `is_empty()`
  post-sanitize.

- **#10 LOW — `top()` with empty title rendered `╭─  ─…─╮`**
  (two spaces with no glyph between). Empty title now matches
  the bottom-border shape `╭{horizontals}╮`. Test pins it.

- **#11 LOW — `expand_tabs` precondition undocumented**. Added
  comment that input should be control-byte free; callers must
  sanitize first.

- **#12 LOW — `BoxBuilder::row("a\nb")` produced one row
  containing a literal `\n`**. Now splits on `\n` and emits one
  row per logical line. Test `builder_splits_embedded_newlines`
  pins it.

- **#13 LOW — `BoxBuilder` had no labelled-row variant**. Added
  `row_labelled(label, sep, value)` that indents wrapped tails
  under the value column. Mirrors the alert chamber's
  `labelled_rows` shape so a future alert migration to
  BoxBuilder is unblocked. Test pins continuation indent.

- **#14 LOW — `strip_controls` allocated on no-op path**. Fast
  path returns the input unchanged when no chars would be
  filtered.

- **#15 DESIGN — sender caching deferred**. Per-call `sender()`
  is the right semantics for the orphan-detection case (#2);
  caching would skip the slot-clear behavior. Kept as is.

8 new tests; 715 total. Two-test serialisation via TEST_GATE for
the notification tests since they mutate global TX/RX_HOLDER state.

fmt clean.
yogthos pushed a commit that referenced this pull request May 22, 2026
…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 added a commit that referenced this pull request Jun 2, 2026
… critic transcript [dirge-ep1f] (#357)

Round D, scoped HONESTLY after a rigorous feasibility pass: most of the
proposed text-helper merges are intentional per-consumer variation with
load-bearing, thinly-tested formats — consolidating them would risk silently
changing model/critic-facing output. So:

#8 (boundary helpers, the one pure win): promote char_boundary_at_or_before/
after to crate::text; compression.rs drops its private copies, summarize.rs's
head_tail_truncate replaces its two inline boundary loops. Zero output change,
no marker/ratio touched. The 3 head/tail TRUNCATORS themselves are LEFT —
different ratios + model-facing markers, intentionally per-consumer.

#6 (byte-budget … cluster): text::ellipsize(s, max_bytes) absorbs
skills::truncate + memory_store::truncate_for_error (display/error text, not
model-facing). The other 6 ellipsis sites are LEFT — their char-unit /
whitespace-collapse / marker variations are genuinely per-site.

#7 (transcript builders): NOT consolidated — 3 different input types
(SessionMessage/LoopMessage/serde_json::Value), 3 intentional formats, and a
trait+style abstraction would add more complexity than it removes.

PLUS the highest-value action the audit surfaced: a byte-EXACT pinning test
for build_critic_transcript (load-bearing critic format, previously only
.contains()-tested) so a future refactor can't silently shift its labels.
+ ellipsize/char_boundary unit tests. 2457 default / 2551 all-features pass.

Co-authored-by: Yogthos <yogthos@gmail.com>
allen-munsch pushed a commit to allen-munsch/dirge that referenced this pull request 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 pushed a commit to allen-munsch/dirge that referenced this pull request Jun 3, 2026
…iag (dirge-code#83)

Track F-HIGH dirge-code#8 from ROADMAP.md.

## Problem

`load_session` (`storage.rs:98-104`) deserialized raw JSON with
no schema versioning. Two pain points:

1. **Corrupted files** (truncated mid-write before atomic save
   shipped, or external mutation) surfaced as
   `expected ',' or '}' at line N column M` with no file path.
   A user with many sessions had to grep to figure out which
   one was broken.
2. **No migration path** for future schema bumps. Field
   additions so far used `#[serde(default)]` so they migrated
   transparently, but a real shape change (renaming a field,
   restructuring `tree.entries`) would silently corrupt old
   sessions on first load with no way to detect.

## Fix

`Session` gains `pub schema_version: u32` with
`#[serde(default)]` so pre-F8 files load with `schema_version
= 0`. `Session::new` sets it to the current `SCHEMA_VERSION`
constant (initial value: 1 — first versioned schema).

`load_session` now:

1. Wraps the parse error with `path.display()` context so the
   user sees `failed to parse <path>: <serde error>`.
2. After successful deserialize, compares `session.schema_version`
   against `SCHEMA_VERSION`:
   - Lower → call `migrate_session(&mut session)` then bump.
   - Equal → no-op.
   - Higher (file from newer dirge) → warn via tracing but
     still load. Most fields default-migrate via serde; only
     truly new fields get default values.

`migrate_session` is a single function with version gates ready
for future schema work. The v0 → v1 step is a no-op (current
schema is structurally identical to pre-F8; only the field
introduction marks the version).

## Tests

Two new tests in `session::storage::tests`:

- `load_session_migrates_pre_f8_files`: writes a minimal pre-F8
  session JSON (no schema_version field) to disk, loads it,
  asserts schema_version is bumped to SCHEMA_VERSION and the
  data is intact.
- `load_session_corrupted_file_includes_path_in_error`: writes
  a truncated JSON, asserts the error mentions the path so the
  user can grep `dirge` logs and identify the broken file.

664 pass (was 662). All build profiles clean.

Co-authored-by: Yogthos <yogthos@gmail.com>
allen-munsch pushed a commit to allen-munsch/dirge that referenced this pull request Jun 3, 2026
…aths (dirge-code#111)

23 audit findings verified REAL via parallel agent verification +
cross-check against opencode/pi reference patterns. Shipping the
10 most concrete fixes here; the rest go in a follow-up docs/test
batch.

## Security

- **dirge-code#9 bash quote_aware_split missed bare `|`** —
  `safe_cmd | rm -rf /` was treated as one segment; only the
  LHS got permission-checked. Pipe RHS rode in unchecked under
  the fallback (non-semantic-bash) path. Added single-byte `|`
  split after `||` is matched. The tree-sitter path was already
  correct.

- **#4 read.rs no binary detection** — feeding a PDF/ELF/.pyc
  into the LLM as lossy UTF-8 wasted tokens and confused the
  model. Ported opencode `read.ts:153-198`: reject by
  extension list (zip/exe/.o/.pdf/.png/etc.), then sniff the
  first 4 KiB — null byte = binary, >30% non-printable = binary.
  Clear error message tells the agent to use bash + xxd instead.

## Correctness

- **#2 skill override inverted** — README contract: "Project
  skills override global skills by name". Code used
  `map.entry(name).or_insert(skill)` which KEEPS the first
  (global) value and silently drops project overrides. Switch
  to `map.insert` (last-write-wins) since globals iterate
  first and project iterates second.

- **dirge-code#37 skill empty name** — frontmatter `name:` with empty
  value parsed to "", which then matched any `skill ""` call
  silently. Fall back to directory name when frontmatter name
  is empty/whitespace-only.

- **#1 session_tree.janet hook never fired** — plugin defined
  `(defn on-message ...)` but `(def hooks [])` was empty AND
  the hook name doesn't exist (dirge uses `on-message-update`).
  `/label` was permanently broken ("no entry yet"). Fix:
  rename to `on-message-update` + register in hooks vector.

- **dirge-code#7 workflow.janet hooks vector missing entries** — plugin
  defined `workflow-on-tool-end`, `-on-error`, `-on-complete`
  but only registered the first four hook names. Three hooks
  were dead. Added them.

- **dirge-code#26 MCP malformed JSON silently empty args** —
  `serde_json::from_str(&args).unwrap_or_default()` turned bad
  JSON into None, sending the server an empty argument set.
  Server then errored with confusing "missing required field"
  instead of dirge surfacing the actual parse error. Now returns
  ToolError with the parse error message + first 200 chars of
  the offending JSON.

- **dirge-code#22 /prompt default unreachable** — README documents
  `default` as a built-in prompt (prompts/default.md exists),
  but `/prompt default` was intercepted as a magic "clear"
  keyword. If `default` is registered in `context.prompts`,
  the new branch falls through to the normal name-lookup. Only
  acts as clear-keyword when no `default` prompt is present
  (legacy fallback).

- **dirge-code#23 /allow add accepted invalid tools** — typo
  `/allow add bsah ...` silently created an inert rule the
  user couldn't debug. Added a known-tools whitelist matching
  PermissionConfig fields; unknown tools error with the valid
  list.

## Performance + correctness

- **dirge-code#11 grep loaded whole files into memory** — no size cap
  meant a 9MB file got fully buffered. Added 10 MiB per-file
  cap via metadata pre-check.

- **dirge-code#15 Python dunder methods marked non-exported** —
  `!name.starts_with('_')` treats `__init__`/`__call__`/etc.
  as private, even though they're Python's standard public
  protocol. Recognize `__x__` dunder pattern as exported.

## UI

- **dirge-code#36 panel char-count truncation vs Unicode width** — panel
  truncation used `chars().count()` while wide emoji and CJK
  take 2 cells. A status line with an emoji overflowed the
  right border by one cell. Switched to
  `UnicodeWidthStr::width` for both truncation and padding.

## Tests

4 new regression tests:
- `test_is_binary_extension_known` — pdf/tgz/.so/.jpg/.pyc
- `test_is_binary_content_null_byte` — null byte trigger,
  UTF-8 Japanese stays clean, all-non-printable triggers
- `quote_aware_split_splits_on_bare_pipe` — pipe security
- `quote_aware_split_or_and_pipe_distinct` — `a || b | c`
  produces 3 segments, not 2

725 plugin / 599 default pass. All build profiles clean.

## Verified false positives (not fixed, audit was wrong)

- #3 cache.rs clear() race — generation counter gating in
  `get` makes stale entries invisible, no correctness impact.
- dirge-code#17 DeepSeek auto-detect priority — auto-detect only fires
  when env vars present; default-default is still OpenRouter.
- dirge-code#19 semantic tools in collision filter — semantic tools
  added separately, can't be shadowed by MCP.
- dirge-code#20 glob global gitignore — intentionally disabled to match
  grep behavior.
- dirge-code#28 nearest_root blocking std::fs — function doesn't exist
  in current code.
- dirge-code#32 ReadArgs.path vs GrepArgs.path — semantically different
  by design (file vs dir), documented in schema.
- dirge-code#33 install_plugin_providers dead-without-feature — gated
  with explicit `#[cfg_attr(not(feature), allow(dead_code))]`.
- dirge-code#34 websearch double-gated — config + API key serve distinct
  purposes (enable + auth).

## Deferred to follow-up batches

Docs-only fixes (dirge-code#6 CONFIG.md tools, dirge-code#12 temperature, dirge-code#13
--api-key, dirge-code#14 acp_host/port), MCP/LSP architecture (dirge-code#8, dirge-code#25,
dirge-code#27), test gaps (dirge-code#38-40), and lower-priority polish — all in
a follow-up PR.

Co-authored-by: Yogthos <yogthos@gmail.com>
allen-munsch pushed a commit to allen-munsch/dirge that referenced this pull request Jun 3, 2026
…de#112)

Follow-up to PR dirge-code#111. Tier-2 items from the 23-bug audit batch:
docs corrections and two small correctness/UX fixes.

## Docs

- **dirge-code#12 temperature** — CONFIG.md claimed "parsed but not
  currently applied". Actually applied since PR dirge-code#105 with a
  clamp warning. Rewrote the cell.
- **dirge-code#13 --api-key** — flag existed but neither README nor
  CONFIG.md mentioned it. Added a Quick-start example noting
  the process-list visibility caveat.
- **dirge-code#14 acp_host/acp_port** — CONFIG.md documented both keys
  but the CLI flags were intentionally removed (stdio-only
  transport). Removed both from the keys table + ACP section.
- **dirge-code#6 tools** — `Config::tools` (per-tool enable map) was
  fully wired in code but undocumented. Added a row to the
  keys table covering `tools.websearch` and `tools.webfetch`.
- **dirge-code#21 find_callers** — README claimed "word-boundary regex"
  but the impl uses the tree-sitter symbol index. Updated to
  reflect actual behavior; the user-visible word-boundary
  semantics are preserved.

## Code

- **dirge-code#16 semantic index skip_dir** — `SymbolIndex::find_callers`
  filter had its own hardcoded `matches!(name, "node_modules"
  | "target" | ".git" | "__pycache__")` while the rest of
  the codebase uses `agent::tools::is_skip_dir`. Switched to
  the shared helper so future additions stay in lockstep.

- **dirge-code#18 context::load_file** — silently swallowed
  `read_to_string` errors via `.ok()`. A permission-denied
  AGENTS.md looked identical to a missing file. Now emits a
  stderr warning naming the path + reason; still returns None
  so callers' behavior is unchanged.

725 plugin / 599 default pass. All build profiles clean.

## Remaining audit items (deferred to feature work)

- **dirge-code#8 LSP no crash restart**: needs broken-pipe IO error
  handling + exponential backoff. Touches manager state
  machine.
- **dirge-code#10 task tool fire-and-forget**: needs timeout + cleanup
  coordination via JoinHandle tracking.
- **dirge-code#25 MCP no reconnection**: similar architectural concern
  to dirge-code#8.
- **dirge-code#27 LSP didClose**: client lifecycle hook missing.
- **dirge-code#29 token estimation len/4**: needs per-provider usage
  extraction (Phase 6 work).
- **dirge-code#5 MCP shutdown**: rmcp Drop semantics need verification.
- **dirge-code#38/39/40 semantic test gaps**: get_symbol_body untested,
  list_symbols kind_filter untested, find_definition test
  vacuous. Sat down to add but each requires a fixture build.

Together with PR dirge-code#111 (10 code fixes), 17 of the 23 verified
items are now shipped. Remaining 6 are architectural or
test-infrastructure work better tackled as discrete PRs.

Co-authored-by: Yogthos <yogthos@gmail.com>
allen-munsch pushed a commit to allen-munsch/dirge that referenced this pull request 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 pushed a commit to allen-munsch/dirge that referenced this pull request 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 pushed a commit to allen-munsch/dirge that referenced this pull request 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 pushed a commit to allen-munsch/dirge that referenced this pull request Jun 3, 2026
…rf cache

Self-review of the last 4 commits flagged 11 findings. Addressing all
in one batch.

- **#1 HIGH — leading whitespace dropped on first row of soft_wrap**:
  `current.is_empty()` at row start unconditionally dropped
  `token.leading_ws`, including on the first row of a logical line.
  Option lines `"  ▶ label …"` lost their `  ` margin; the green
  `  allowed …` confirmation lost its indent too. First-row branch
  now preserves leading_ws (with a ws-overflow fallback to keep the
  token).

- **#2 HIGH — drain_events early-broke before reader quiesced**: the
  Ok(false) shortcut fired on the first quiet poll, which was often
  "the background reader currently holds crossterm's internal mutex"
  rather than "terminal is quiet". A delayed OSC 11 / DA1 response
  could still escape past our drain. Now requires at least one
  observed event before the Ok(false) shortcut; honors the full
  budget otherwise.

- **#3 HIGH — MODIFIED section clipped its own bottom border at
  `available == 4`**: row_budget=1, footer + 1 file = 2 items, +3
  frame rows = 5 total in a 4-row budget. draw_panel clipped the
  `╰────╯`. Bumped MIN_MOD_SECTION_ROWS from 4 → 5 so the bottom
  border is always painted.

- **#4 MEDIUM — `\r` not stripped from CRLF input**: Windows /
  some-MCP tool output left `\r` in tokens, producing terminal
  redraw artifacts. `soft_wrap` now strips a trailing `\r` per
  logical line.

- **dirge-code#5 MEDIUM — Show cursor on alt screen was a no-op**: `Show` was
  issued while still on the alt screen; `LeaveAlternateScreen`
  restores the main screen's saved DECTCEM state, discarding the
  Show. Moved Show to AFTER LeaveAlternateScreen + disable_raw_mode.

- **dirge-code#6 MEDIUM — recent(256) clones + locks on every redraw**: panel
  redraws on every streamed token. Added `modified::version()`
  monotonic counter (bumped on mark / clear); panel-side cache in
  `panel_modified_cached` keyed by (version, cwd) skips the lock +
  256-PathBuf clone + path-strip when nothing changed.

- **dirge-code#7 MEDIUM — break_long_token could emit wide-glyph row > budget
  at max_width<2**: floored max_width at 2 inside soft_wrap. A
  1-cell terminal is unusable anyway; this just removes a sharp
  edge.

- **dirge-code#8 MEDIUM — all-whitespace first row collapsed to empty**:
  same root cause as #1, fixed by the same change. Indented blank
  separators now preserve their indentation.

- **dirge-code#9 LOW — `allowed …` confirmation flush against alert `╰─╯`**:
  added a blank-line breathing row before the green confirmation
  so the alert's bottom border and the confirmation don't read as
  one block.

- **dirge-code#10 LOW — head_w used chars().count() not display width**:
  switched to `UnicodeWidthStr::width(head)` so future wide-glyph
  markers won't under-pad the continuation indent.

- **dirge-code#11 LOW — single-select marker width inconsistent**: cursor
  marker was `▶` (w=1), non-cursor was `  ` (w=2), so wrapped tails
  of adjacent options drifted by one column. Cursor marker padded
  to `▶ ` so all markers in a question share display width.

5 new tests:
- preserves_leading_whitespace_on_first_row
- strips_carriage_returns_from_crlf_input
- wide_glyph_respects_max_width_at_floor
- preserves_leading_whitespace_only_line
- version_bumps_on_mark_and_clear

651 tests pass; fmt clean.
allen-munsch pushed a commit to allen-munsch/dirge that referenced this pull request Jun 3, 2026
…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 pushed a commit to allen-munsch/dirge that referenced this pull request Jun 3, 2026
…nds, error masking

Adversarial review of `349afb6`/`b9de3d5`/`2fac609` flagged 14
findings. Addressing all.

- **#1 HIGH — C1 controls bypassed MCP stderr sanitizer**: the
  original filter was `b == 0x09 || (0x20..0x7f).contains(&b) ||
  b >= 0x80`. That second clause let through every non-ASCII byte
  including the C1 control range (U+0080..=U+009F). In particular
  U+009B is single-byte CSI, behaves identically to `\x1b[` on
  iTerm2/xterm in 8-bit mode. A misbehaving MCP child could write
  `\u{9b}2J` and repaint the screen — exactly the smuggling vector
  the commit message claimed to close. Filter now blocks C0
  controls (except `\t`), DEL, and the full C1 range.

- **#2 HIGH — MCP stderr was silently dropped at default verbosity**:
  emit was `tracing::info!`, but dirge's default EnvFilter is
  `warn,rig=off`. Users diagnosing MCP server panics or init
  errors saw nothing — real regression vs the old
  `Stdio::inherit()`. Raised to `tracing::warn!` so it surfaces
  on the default config.

- **#3 HIGH — `parse_ddg_html` panicked on truncated input**:
  `&html[tag_start..abs_start + 32]` blew up when `abs_start +
  32 > html.len()` or landed mid-codepoint. Rewrote the scanner
  to anchor on `<a ` tags and walk only within bounded slices
  via `tag_end.min(html.len())`. Added regression test for the
  truncated case.

- **dirge-code#5 MEDIUM — MCP stderr forwarder had no per-line cap**:
  `BufReader::lines()` buffers until `\n`. A buggy child writing
  a GB without newline would OOM dirge. Replaced with a manual
  read loop, 16 KiB per-line cap, emit `…[truncated]` past the
  cap and skip until next `\n`.

- **dirge-code#6 MEDIUM — provider rotation race on first call**: two
  concurrent first-callers both saw `AtomicU8 = 0`, both rolled
  a fresh entropy pick, both stored. Last writer won —
  inconsistent contract. Switched to `compare_exchange` from 0
  to candidate; loser re-reads the winner's value.

- **dirge-code#7 MEDIUM — both-providers-fail error masked secondary +
  DDG errors**: only `primary_err` was returned. Now
  concatenates all three failures so the user can diagnose
  without chasing the wrong cause.

- **dirge-code#8 MEDIUM — `parse_ddg_html` false-positives on substring
  match**: previously matched `class="result__a"` anywhere in
  the HTML, including inside `<script>` blocks or quoted text.
  Walked backward via `rfind("<a ")` could grab an unrelated
  anchor. Now anchors on `<a ` first and inspects the tag's
  attributes — proper containment check.

- **dirge-code#9 MEDIUM — DDG snippet control bytes flowed into LLM
  prompt**: `strip_tags_and_decode` decoded entities but didn't
  filter ESC / C1 controls. A malicious or mojibake search
  result could ship ANSI styling into the agent's context.
  Added control-byte filter to the decoder's output pass.

- **dirge-code#10 LOW — `PARALLEL_API_KEY` read per-call**: was
  `std::env::var` inside `call`, inconsistent with how
  `EXA_API_KEY` was captured at construction. Moved to
  `WebSearchTool::new`.

- **dirge-code#11 LOW — whitespace-only key passed empty-filter**:
  `EXA_API_KEY="  "` produced a malformed
  `?exaApiKey=%20%20` URL. Now trims keys at construction.

- **dirge-code#12 LOW — tool description out of date**: still mentioned
  Exa-only + DDG fallback, missed Parallel.ai rotation and the
  keyless default. Rewritten to reflect the actual contract.

- **dirge-code#13 LOW — `urlencode_query` renamed to `percent_encode`**:
  the function is a generic percent-encoder (RFC 3986
  unreserved set), not a form-encoder. Misleading name.

- **dirge-code#14 DESIGN — no tests for new code paths**: added 11
  regression tests covering ddg parser bounds + happy path +
  anti-script-block, control-byte filter, key trim, MCP
  response parser (plain JSON + SSE + malformed), DDG redirect
  unwrap, percent encoding, provider env override.

- **dirge-code#15 LOW — bot-identifying DDG User-Agent**: swapped
  `compatible; dirge-agent/1.0` for a real Firefox 133 UA. DDG
  aggressively rate-limits identifiable scrapers.

695 tests pass (684 + 11 new); fmt clean.
allen-munsch pushed a commit to allen-munsch/dirge that referenced this pull request Jun 3, 2026
…h_bg width, sanitization

Correctness + security review of `ea042b1`/`524c90c` flagged 15
findings. Addressing all 15.

- **#1 HIGH — startup race: MCP forwarders fired before
  `install()`**. `connect_all` spawns stderr forwarders in `main`
  BEFORE `run_interactive` reached the old `install()` call. Lines
  emitted during MCP-server handshake hit `sender() == None` and
  were silently dropped. Moved `install()` to the very top of
  `main()` so the channel is live by the time any forwarder
  starts. Split the API: `install()` (creates channel) +
  `take_receiver()` (UI loop claims the rx).

- **#2 HIGH — orphaned-sender footgun on UI restart**. `OnceLock`
  meant a re-entry could never replace the sender; producers
  holding clones would send into a dead channel forever. Switched
  to `RwLock<Option<Sender>>`. Producers also self-heal: when
  `try_send` returns Err because the receiver was dropped,
  `notify_send` clears the slot so subsequent producers see `None`
  and skip.

- **#3 HIGH — `row_with_bg` was still char-count-based**. The
  refactor claim was "unifies display-width vs char-count" but
  the bg-tinted variant still used `chars().count()`. A diff row
  with CJK / emoji drifted the right border. Now uses the same
  display-width budget as the plain `row`. Regression test
  `row_with_bg_width_invariant` pins it.

- **#4 HIGH — unbounded channel + no backpressure → OOM**. A
  buggy / hostile MCP child spamming stderr would grow the queue
  unboundedly. Switched to `mpsc::channel(1024)` (bounded) with
  `try_send` so the producer drops on overflow rather than
  unboundedly queuing. Test `bounded_channel_drops_on_full` pins
  the contract.

- **dirge-code#5 MEDIUM — multi-colon MCP tool names**. `splitn(3, ':')` on
  `mcp_tool:server:do:thing` parsed correctly but the comment
  explanation was off. Clarified; behavior unchanged (the
  wildcarded server pattern is the desired semantics).

- **dirge-code#6 MEDIUM — mcp_tool umbrella check case-sensitive**.
  `umbrella == "mcp_tool"` would miss `MCP_TOOL:…` if a future
  caller surfaces uppercase. Switched to `eq_ignore_ascii_case`.

- **dirge-code#7 MEDIUM — receiver-side sanitization for ALL Notification
  variants**. MCP variant was pre-sanitized at the producer,
  but Info/Warn/Error had no producer-side contract. Adding
  receiver-side `ansi::strip_controls(KEEP_NEWLINE)` makes the
  rule un-bypassable: nothing reaches `write_line` carrying
  escape bytes regardless of how careful a future producer is.

- **dirge-code#8 MEDIUM — websearch `KEEP_BOTH` + `\n` broke chamber
  border**. Tabs survived into chamber rows where they
  interacted poorly with the wrap math. Switched to
  `KEEP_NEWLINE` and replace `\t` with single space.

- **dirge-code#9 MEDIUM — whitespace-only MCP lines dropped**. The
  blank-line collapse used `trim().is_empty()` which also ate
  legitimate indented continuation lines. Now uses `is_empty()`
  post-sanitize.

- **dirge-code#10 LOW — `top()` with empty title rendered `╭─  ─…─╮`**
  (two spaces with no glyph between). Empty title now matches
  the bottom-border shape `╭{horizontals}╮`. Test pins it.

- **dirge-code#11 LOW — `expand_tabs` precondition undocumented**. Added
  comment that input should be control-byte free; callers must
  sanitize first.

- **dirge-code#12 LOW — `BoxBuilder::row("a\nb")` produced one row
  containing a literal `\n`**. Now splits on `\n` and emits one
  row per logical line. Test `builder_splits_embedded_newlines`
  pins it.

- **dirge-code#13 LOW — `BoxBuilder` had no labelled-row variant**. Added
  `row_labelled(label, sep, value)` that indents wrapped tails
  under the value column. Mirrors the alert chamber's
  `labelled_rows` shape so a future alert migration to
  BoxBuilder is unblocked. Test pins continuation indent.

- **dirge-code#14 LOW — `strip_controls` allocated on no-op path**. Fast
  path returns the input unchanged when no chars would be
  filtered.

- **dirge-code#15 DESIGN — sender caching deferred**. Per-call `sender()`
  is the right semantics for the orphan-detection case (#2);
  caching would skip the slot-clear behavior. Kept as is.

8 new tests; 715 total. Two-test serialisation via TEST_GATE for
the notification tests since they mutate global TX/RX_HOLDER state.

fmt clean.
allen-munsch pushed a commit to allen-munsch/dirge that referenced this pull request Jun 3, 2026
…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 pushed a commit to allen-munsch/dirge that referenced this pull request Jun 3, 2026
… critic transcript [dirge-ep1f] (dirge-code#357)

Round D, scoped HONESTLY after a rigorous feasibility pass: most of the
proposed text-helper merges are intentional per-consumer variation with
load-bearing, thinly-tested formats — consolidating them would risk silently
changing model/critic-facing output. So:

dirge-code#8 (boundary helpers, the one pure win): promote char_boundary_at_or_before/
after to crate::text; compression.rs drops its private copies, summarize.rs's
head_tail_truncate replaces its two inline boundary loops. Zero output change,
no marker/ratio touched. The 3 head/tail TRUNCATORS themselves are LEFT —
different ratios + model-facing markers, intentionally per-consumer.

dirge-code#6 (byte-budget … cluster): text::ellipsize(s, max_bytes) absorbs
skills::truncate + memory_store::truncate_for_error (display/error text, not
model-facing). The other 6 ellipsis sites are LEFT — their char-unit /
whitespace-collapse / marker variations are genuinely per-site.

dirge-code#7 (transcript builders): NOT consolidated — 3 different input types
(SessionMessage/LoopMessage/serde_json::Value), 3 intentional formats, and a
trait+style abstraction would add more complexity than it removes.

PLUS the highest-value action the audit surfaced: a byte-EXACT pinning test
for build_critic_transcript (load-bearing critic format, previously only
.contains()-tested) so a future refactor can't silently shift its labels.
+ ellipsize/char_boundary unit tests. 2457 default / 2551 all-features pass.

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