chore: audit fixes (5 rounds — CRITICAL/HIGH/MEDIUM/LOW) - #54
Merged
Conversation
added 5 commits
May 20, 2026 18:46
Round 1 of the audit fixes — security + crashes.
## Path traversal in session storage (CRITICAL)
`save_session` / `load_session` / `delete_session` interpolated
`session.id` directly into the `{id}.json` filename. Session ids
are normally UUIDs but they round-trip through JSON on disk; a
tampered-with file with `id: "../../etc/passwd"` could escape the
sessions directory on save or read arbitrary files on load.
New `validate_session_id` gate: accepts `[A-Za-z0-9._-]+` only,
explicitly rejects `.`, `..`, slashes, backslashes. Tested with
the usual escape attempts.
## Compress bounds + leaf-tracking (HIGH)
`Session::compress(_, first_kept_index, _)` did `messages.drain(..first_kept_index)`
with no bounds check — an out-of-range index from a buggy caller
panicked. Now clamped to `messages.len()` so misuse degrades to
"summarize everything" instead of crashing the agent.
Branched-session compaction also had a latent leaf-tracking bug:
if `tree.leaf_id` pointed at a branch leaf that was in the dropped
set (e.g. user forked, then compressed the alternate branch), the
leaf id was left dangling. New post-prune check re-anchors the leaf
to the first kept message (or the summary if everything was dropped).
## QuestionTool routes through permission (HIGH)
`QuestionTool::call` injected user input into the LLM's tool result
without any permission check, unlike `TaskTool` / `WriteTool` /
`BashTool` etc. Added `permission: Option<PermCheck>` +
`ask_tx: Option<AskSender>` to the struct, new `.with_permission()`
builder, and a `check_perm(&self.permission, &self.ask_tx, "question", &summary)`
at the top of `call`. Wired through `builder.rs` so production
paths get the gate; existing tests construct without permission
and continue to pass.
## Test plan
- [x] 2 new tests in `session::storage::tests` (UUID accept,
traversal reject).
- [x] `cargo test --features plugin` -> 606 pass, 0 fail.
- [x] Both build profiles -> 0 warnings.
…e, warn unknown ops Round 2 — silent failures. ## on-complete now fires (CRITICAL) `on-complete` was in `HOOK_NAMES` (so plugins defining it got auto-aliased on load) but no host site dispatched. Plugins that defined `on-complete` ran forever without ever seeing the event. Now fires from `AgentEvent::Done` right after `on-response` + the pending-prompt + store_response sequence, so plugins observe a fully-complete turn (response stored, pending prompts processed). ## Turn-hook slot reset (HIGH) `on-turn-start` / `on-turn-end` bypassed `dispatch_tool_hook`'s slot-clear step, so a plugin calling `(harness/block ...)` from inside a turn hook would leave the slot set and spuriously block the *first* tool of the next turn. Both turn hooks now reset `harness-block` / `harness-mutate-input` / `harness-replace-result` explicitly after the dispatch returns. ## harness-response cleared after store (HIGH) `mgr.store_response()` wrote `harness-response` and left it set indefinitely. Plugins reading the var in a later hook saw stale text from previous turns. Now cleared with `(set harness-response nil)` immediately after store, matching the pattern other slots already use. ## drain_tree_ops warns on unknown op verbs (MEDIUM) `parse_tree_op_line` silently dropped lines it didn't recognize (forward-compat — good), but with no diagnostic a typo'd op verb in a plugin would fail with no trace. Added a `tracing::warn!` at `dirge::plugin` target so confused plugin authors can spot the typo in the log. ## Test plan - [x] `cargo test --features plugin` -> 606 pass, 0 fail. - [x] Both build profiles -> 0 warnings.
…mic init Round 3 — theme + UI polish. ## Markdown theme sweep (CRITICAL) `src/ui/markdown.rs` had 13 hardcoded color literals (`Color::Cyan`, `Color::DarkYellow`, `Color::DarkGrey`) used for headings, code blocks, blockquotes, and bullets. Under the phosphor theme these forced cyan + yellow accents that broke palette coherence. Swept to `theme::header()`, `theme::tool()`, and `theme::dim()` so palette swaps cascade through markdown rendering. `bullet_prefix(col: Color)` matched on `Color::DarkGrey` as a sentinel for "this is a blockquote." With the dim color now themed away from DarkGrey, the sentinel was broken. Refactored to `bullet_prefix(in_blockquote: bool)` — explicit, theme-safe. ## Cursor flicker on the right (HIGH) `draw_bottom` called `draw_panel` while the cursor was visible — the panel's MoveTo loop walked the hardware cursor across the right-hand panel one cell at a time, visibly flickering. Now hides the cursor BEFORE panel + avatar paints, places it at the final input position, then re-shows. ## Avatar resets to Idle on user submit (MEDIUM) After a turn completed the avatar stuck on Done forever — the next user prompt didn't visually "wake" it. Now all three user-message commit sites set the avatar to Idle so the brief moment between submit and first agent token shows a neutral face that transitions to Thinking/Speaking naturally. ## Atomic back-compat init (MEDIUM) `ensure_message_store_initialized()` and `ensure_tree_initialized()` were called as a pair in every mutation method; a panic between them could leave the session half-initialized (tree rebuilt but store empty, or vice versa). New `ensure_back_compat_initialized()` runs both in one call. All five mutation sites (add_message, pop_last_message, switch_to_leaf, fork_at, compress) updated. ## Test plan - [x] `cargo test --features plugin` -> 606 pass, 0 fail. - [x] Both build profiles -> 0 warnings.
…n record
Round 4 — schema + permission hygiene.
## TaskStatusTool routes through permission
`task_status` was the only background-task tool that didn't call
`check_perm`. The wait=true path can hold the parent turn open for
up to 10 minutes; both wait=true and wait=false leak subagent
state to the caller LLM. Now gated identically to `task`.
## PlanEnterTool / PlanExitTool stay un-gated (documented)
These tools surface a user-confirmation dialog via `plan_tx` /
`PlanSwitchResponse::{Accepted,Rejected}` — that IS the gate. A
second `check_perm` call would double-ask the user. Added a
comment in both `call` methods so future readers don't try to
"fix" the missing check.
## WebFetch max_chars: number -> integer
Schema declared `"type": "number"` for what's a `usize` in code.
LLM-emitted floats would round-trip as 0 (when fractional) or
panic in `.chars().take(n)`. Now declared as integer with a
minimum of 1.
## LSP workspaceSymbol query: schema now documents the contract
Schema marks `query` as a regular optional field, but the call
path errors when query is missing AND operation == workspaceSymbol.
Added that constraint to the parameter description so the LLM
knows when to pass it (the runtime check stays as the source of
truth).
## Compress now replaces the compactions list instead of appending
`Compaction::first_kept_index` is meaningful only for the *latest*
compaction record — keeping a list of records from earlier
compresses left stale `first_kept_index` values that no longer
matched the post-drain message indices. The LLM context already
folds older summaries into the new summary via `previous_summary`
(captured in `slash.rs:109` before compress runs), so dropping
the historical list is lossless. Fixes the multi-compaction
accounting drift flagged in the audit.
## Test plan
- [x] `cargo test --features plugin` -> 606 pass, 0 fail.
- [x] Both build profiles -> 0 warnings.
… fallback Round 5 — low hygiene. ## GlobTool gets the dual-constructor pattern Other read tools (Read/Grep/FindFiles/ListDir) follow a `new(perm, ask_tx) / with_cache(perm, ask_tx, cache)` pattern. `GlobTool` had only `new`, so glob calls re-walked the filesystem every invocation even when bash/write/edit cache-clears explicitly invalidated other tools' caches. Added `with_cache(perm, ask_tx, cache)` and wired it through `builder.rs`. Reuses the same `ToolCache` so a `bash`/`write` mutation clears glob results too. ## pop_last_message: tree-corruption fallback If `tree.entries` somehow lacks the popped message's id (data corruption, external mutation), the old code wiped `tree.leaf_id` to None, leaving the tree dangling on branched sessions. New fallback uses the previous message's id from the linear cache so the leaf stays anchored to a real node. ## fork_at: documented root behaviour `fork_at` at the conversation root clears `messages` and sets `leaf_id = None` — useful but surprising. Docstring now spells this out, including the note that sibling branches survive. ## switch-session: ambiguity error lists candidate ids `prefix 'ab' matches 5 sessions` was un-debuggable. Now lists the first 3 matching session ids (short form) so the plugin author / user can pick a longer prefix immediately. ## Test plan - [x] `cargo test --features plugin` -> 606 pass, 0 fail. - [x] Both build profiles -> 0 warnings.
yogthos
added a commit
that referenced
this pull request
May 21, 2026
…ts rebuild, MCP hardening (#68) TDD-driven fixes for the verified real bugs from audit round 9. ## Round A — verified real bugs (CRITICAL/HIGH) ### `/cd /tmp` no longer cds to home `splitn(3, ' ')` produces empty middle elements for consecutive spaces — `/cd /tmp` parsed as `["/cd", "", "/tmp"]`, and the empty `parts[1]` collapsed to "cd to home". Now derives the target by stripping the `/cd` prefix and trimming, so any amount of whitespace between the command and the path resolves correctly. ### Empty-input "allow always" no longer pins literal placeholder PR #67 made `suggest_pattern("bash", "")` return the literal `"<edit this pattern>"` placeholder so an accidental "(a) allow always" wouldn't pin a catch-all `"* *"`. But the ask-dialog fed that placeholder straight into `UserDecision::AllowAlways`, storing the literal text as a real pattern in `permission_allowlist`. The dialog now detects placeholders via the new `is_placeholder_pattern` predicate and falls back to `AllowOnce` with a dim "can't derive a useful pattern from empty input; allowing once only" message. ### `/regen-prompts` rebuilds the agent Regenerating overwrote the on-disk prompt content + reloaded `context.prompts`, but the LIVE agent kept the old preamble in memory. Users had to `/prompt <name>` to actually see the new content. Now also re-binds `context.current_prompt` to the freshly-loaded body for the currently-active name and rebuilds the agent so the new system prompt takes effect immediately. ### `/toggle` added to `/help` Slipped through PR #54 — the feature exists and is in the README but the in-app `/help` text didn't list it. ### Anthropic `overloaded_error` classified as RateLimit The error classifier matched "rate limit" / "too many requests" / "429" but not Anthropic's `overloaded_error` (structurally a rate-limit signal). Falls through to `Other` and no retry fires — user saw a one-shot failure on transient backend pressure. Now any error string containing "overloaded" routes to `ErrorKind::RateLimit` and triggers the exponential-backoff retry. ## Round B — MCP hardening ### MCP server init timeout (10s) `serve_client((), transport).await` had no upper bound. A command-based MCP server that hung on `initialize` (waiting for stdin / wedged binary) would pin dirge's startup indefinitely. Now wrapped in `tokio::time::timeout(MCP_INIT_TIMEOUT)`; past 10s we abort, log the failure, and continue with the other servers. ### Empty `EXA_API_KEY` skips Exa default registration A user with `EXA_API_KEY=""` (explicit empty, e.g. from a `.envrc` that intentionally clears it) used to register the Exa server anyway, then every web-search call failed with 401 at first use. Now treats empty key the same as unset — Exa default skipped, no broken server in the list. ### MCP tool `inputSchema` null fallback to `{}` Servers that omit `inputSchema` had it serialized as JSON `null`, which rig's tool registration treats as an invalid parameters block. The tool became unusable. Now substitute an empty object: the tool stays callable; the LLM sees "no params" correctly. ## Tests 3 new tests, written failing first: - `classify_anthropic_overloaded_error_as_retryable` (2 cases) - `placeholder_pattern_is_detectable` (`/cd` whitespace fix is observable in the existing /cd tests if present; verified by manual trace through the parsing path.) Total: 635 pass (was 633), 0 fail across all build profiles (`--features plugin`, `--all-features`, `--no-default-features`). ## Deferred from this audit (bigger scope) - **Compression prunes sibling branches** — needs a tree-walk during `compress` to preserve sibling subtrees whose parents get dropped. Real bug, real risk on branched sessions, but the fix touches the compress data flow significantly. - **Subagent permission/sandbox/hooks** — `task` tool's `btw_query` is a bare LLM call with no permission/sandbox inheritance and no plugin hook dispatch. By design today, but the safety properties are not what users expect. - **`convert_history` loses tool call structure** — pre-existing finding; resuming a session shows the LLM text-only traces of prior tool calls, losing the structured tool_use markers. - **README "events are buffered" claim** — implementation streams live; doc says buffered. Tokens are NOT re-emitted on retry but the user already saw the partial. - **Multi-plugin harness-block last-write-wins** — design question (queue vs. first-wins) deferred for discussion. - **`/clear` no confirmation** — design choice; standard for CLI. - **`/quit` save-before-break** — needs verification; the outer loop may already save on Interrupted. ## Test plan - [x] `cargo test --features plugin` -> 635 pass, 0 fail. - [x] `cargo build --all-features` -> compiles, no warnings. - [x] `cargo build --no-default-features` -> compiles. Co-authored-by: Yogthos <yogthos@gmail.com>
allen-munsch
pushed a commit
to allen-munsch/dirge
that referenced
this pull request
Jun 3, 2026
* fix(audit r1): path traversal, compress crashes, question permission
Round 1 of the audit fixes — security + crashes.
## Path traversal in session storage (CRITICAL)
`save_session` / `load_session` / `delete_session` interpolated
`session.id` directly into the `{id}.json` filename. Session ids
are normally UUIDs but they round-trip through JSON on disk; a
tampered-with file with `id: "../../etc/passwd"` could escape the
sessions directory on save or read arbitrary files on load.
New `validate_session_id` gate: accepts `[A-Za-z0-9._-]+` only,
explicitly rejects `.`, `..`, slashes, backslashes. Tested with
the usual escape attempts.
## Compress bounds + leaf-tracking (HIGH)
`Session::compress(_, first_kept_index, _)` did `messages.drain(..first_kept_index)`
with no bounds check — an out-of-range index from a buggy caller
panicked. Now clamped to `messages.len()` so misuse degrades to
"summarize everything" instead of crashing the agent.
Branched-session compaction also had a latent leaf-tracking bug:
if `tree.leaf_id` pointed at a branch leaf that was in the dropped
set (e.g. user forked, then compressed the alternate branch), the
leaf id was left dangling. New post-prune check re-anchors the leaf
to the first kept message (or the summary if everything was dropped).
## QuestionTool routes through permission (HIGH)
`QuestionTool::call` injected user input into the LLM's tool result
without any permission check, unlike `TaskTool` / `WriteTool` /
`BashTool` etc. Added `permission: Option<PermCheck>` +
`ask_tx: Option<AskSender>` to the struct, new `.with_permission()`
builder, and a `check_perm(&self.permission, &self.ask_tx, "question", &summary)`
at the top of `call`. Wired through `builder.rs` so production
paths get the gate; existing tests construct without permission
and continue to pass.
## Test plan
- [x] 2 new tests in `session::storage::tests` (UUID accept,
traversal reject).
- [x] `cargo test --features plugin` -> 606 pass, 0 fail.
- [x] Both build profiles -> 0 warnings.
* fix(audit r2): on-complete dispatch, turn-hook slots, harness-response, warn unknown ops
Round 2 — silent failures.
## on-complete now fires (CRITICAL)
`on-complete` was in `HOOK_NAMES` (so plugins defining it got
auto-aliased on load) but no host site dispatched. Plugins that
defined `on-complete` ran forever without ever seeing the event.
Now fires from `AgentEvent::Done` right after `on-response` + the
pending-prompt + store_response sequence, so plugins observe a
fully-complete turn (response stored, pending prompts processed).
## Turn-hook slot reset (HIGH)
`on-turn-start` / `on-turn-end` bypassed `dispatch_tool_hook`'s
slot-clear step, so a plugin calling `(harness/block ...)` from
inside a turn hook would leave the slot set and spuriously block
the *first* tool of the next turn. Both turn hooks now reset
`harness-block` / `harness-mutate-input` / `harness-replace-result`
explicitly after the dispatch returns.
## harness-response cleared after store (HIGH)
`mgr.store_response()` wrote `harness-response` and left it set
indefinitely. Plugins reading the var in a later hook saw stale
text from previous turns. Now cleared with
`(set harness-response nil)` immediately after store, matching the
pattern other slots already use.
## drain_tree_ops warns on unknown op verbs (MEDIUM)
`parse_tree_op_line` silently dropped lines it didn't recognize
(forward-compat — good), but with no diagnostic a typo'd op verb
in a plugin would fail with no trace. Added a `tracing::warn!`
at `dirge::plugin` target so confused plugin authors can spot the
typo in the log.
## Test plan
- [x] `cargo test --features plugin` -> 606 pass, 0 fail.
- [x] Both build profiles -> 0 warnings.
* fix(audit r3): markdown theme sweep, cursor flicker, avatar idle, atomic init
Round 3 — theme + UI polish.
## Markdown theme sweep (CRITICAL)
`src/ui/markdown.rs` had 13 hardcoded color literals
(`Color::Cyan`, `Color::DarkYellow`, `Color::DarkGrey`) used for
headings, code blocks, blockquotes, and bullets. Under the
phosphor theme these forced cyan + yellow accents that broke
palette coherence. Swept to `theme::header()`, `theme::tool()`,
and `theme::dim()` so palette swaps cascade through markdown
rendering.
`bullet_prefix(col: Color)` matched on `Color::DarkGrey` as a
sentinel for "this is a blockquote." With the dim color now
themed away from DarkGrey, the sentinel was broken. Refactored
to `bullet_prefix(in_blockquote: bool)` — explicit, theme-safe.
## Cursor flicker on the right (HIGH)
`draw_bottom` called `draw_panel` while the cursor was visible —
the panel's MoveTo loop walked the hardware cursor across the
right-hand panel one cell at a time, visibly flickering. Now
hides the cursor BEFORE panel + avatar paints, places it at the
final input position, then re-shows.
## Avatar resets to Idle on user submit (MEDIUM)
After a turn completed the avatar stuck on Done forever — the
next user prompt didn't visually "wake" it. Now all three
user-message commit sites set the avatar to Idle so the brief
moment between submit and first agent token shows a neutral
face that transitions to Thinking/Speaking naturally.
## Atomic back-compat init (MEDIUM)
`ensure_message_store_initialized()` and
`ensure_tree_initialized()` were called as a pair in every
mutation method; a panic between them could leave the session
half-initialized (tree rebuilt but store empty, or vice versa).
New `ensure_back_compat_initialized()` runs both in one call.
All five mutation sites (add_message, pop_last_message,
switch_to_leaf, fork_at, compress) updated.
## Test plan
- [x] `cargo test --features plugin` -> 606 pass, 0 fail.
- [x] Both build profiles -> 0 warnings.
* fix(audit r4): permission gates, schema corrections, single-compaction record
Round 4 — schema + permission hygiene.
## TaskStatusTool routes through permission
`task_status` was the only background-task tool that didn't call
`check_perm`. The wait=true path can hold the parent turn open for
up to 10 minutes; both wait=true and wait=false leak subagent
state to the caller LLM. Now gated identically to `task`.
## PlanEnterTool / PlanExitTool stay un-gated (documented)
These tools surface a user-confirmation dialog via `plan_tx` /
`PlanSwitchResponse::{Accepted,Rejected}` — that IS the gate. A
second `check_perm` call would double-ask the user. Added a
comment in both `call` methods so future readers don't try to
"fix" the missing check.
## WebFetch max_chars: number -> integer
Schema declared `"type": "number"` for what's a `usize` in code.
LLM-emitted floats would round-trip as 0 (when fractional) or
panic in `.chars().take(n)`. Now declared as integer with a
minimum of 1.
## LSP workspaceSymbol query: schema now documents the contract
Schema marks `query` as a regular optional field, but the call
path errors when query is missing AND operation == workspaceSymbol.
Added that constraint to the parameter description so the LLM
knows when to pass it (the runtime check stays as the source of
truth).
## Compress now replaces the compactions list instead of appending
`Compaction::first_kept_index` is meaningful only for the *latest*
compaction record — keeping a list of records from earlier
compresses left stale `first_kept_index` values that no longer
matched the post-drain message indices. The LLM context already
folds older summaries into the new summary via `previous_summary`
(captured in `slash.rs:109` before compress runs), so dropping
the historical list is lossless. Fixes the multi-compaction
accounting drift flagged in the audit.
## Test plan
- [x] `cargo test --features plugin` -> 606 pass, 0 fail.
- [x] Both build profiles -> 0 warnings.
* fix(audit r5): glob cache, fork docs, switch-session diagnostics, pop fallback
Round 5 — low hygiene.
## GlobTool gets the dual-constructor pattern
Other read tools (Read/Grep/FindFiles/ListDir) follow a
`new(perm, ask_tx) / with_cache(perm, ask_tx, cache)` pattern.
`GlobTool` had only `new`, so glob calls re-walked the filesystem
every invocation even when bash/write/edit cache-clears
explicitly invalidated other tools' caches.
Added `with_cache(perm, ask_tx, cache)` and wired it through
`builder.rs`. Reuses the same `ToolCache` so a `bash`/`write`
mutation clears glob results too.
## pop_last_message: tree-corruption fallback
If `tree.entries` somehow lacks the popped message's id (data
corruption, external mutation), the old code wiped `tree.leaf_id`
to None, leaving the tree dangling on branched sessions. New
fallback uses the previous message's id from the linear cache so
the leaf stays anchored to a real node.
## fork_at: documented root behaviour
`fork_at` at the conversation root clears `messages` and sets
`leaf_id = None` — useful but surprising. Docstring now spells
this out, including the note that sibling branches survive.
## switch-session: ambiguity error lists candidate ids
`prefix 'ab' matches 5 sessions` was un-debuggable. Now lists
the first 3 matching session ids (short form) so the plugin
author / user can pick a longer prefix immediately.
## Test plan
- [x] `cargo test --features plugin` -> 606 pass, 0 fail.
- [x] Both build profiles -> 0 warnings.
---------
Co-authored-by: Yogthos <yogthos@gmail.com>
allen-munsch
pushed a commit
to allen-munsch/dirge
that referenced
this pull request
Jun 3, 2026
…ts rebuild, MCP hardening (dirge-code#68) TDD-driven fixes for the verified real bugs from audit round 9. ## Round A — verified real bugs (CRITICAL/HIGH) ### `/cd /tmp` no longer cds to home `splitn(3, ' ')` produces empty middle elements for consecutive spaces — `/cd /tmp` parsed as `["/cd", "", "/tmp"]`, and the empty `parts[1]` collapsed to "cd to home". Now derives the target by stripping the `/cd` prefix and trimming, so any amount of whitespace between the command and the path resolves correctly. ### Empty-input "allow always" no longer pins literal placeholder PR dirge-code#67 made `suggest_pattern("bash", "")` return the literal `"<edit this pattern>"` placeholder so an accidental "(a) allow always" wouldn't pin a catch-all `"* *"`. But the ask-dialog fed that placeholder straight into `UserDecision::AllowAlways`, storing the literal text as a real pattern in `permission_allowlist`. The dialog now detects placeholders via the new `is_placeholder_pattern` predicate and falls back to `AllowOnce` with a dim "can't derive a useful pattern from empty input; allowing once only" message. ### `/regen-prompts` rebuilds the agent Regenerating overwrote the on-disk prompt content + reloaded `context.prompts`, but the LIVE agent kept the old preamble in memory. Users had to `/prompt <name>` to actually see the new content. Now also re-binds `context.current_prompt` to the freshly-loaded body for the currently-active name and rebuilds the agent so the new system prompt takes effect immediately. ### `/toggle` added to `/help` Slipped through PR dirge-code#54 — the feature exists and is in the README but the in-app `/help` text didn't list it. ### Anthropic `overloaded_error` classified as RateLimit The error classifier matched "rate limit" / "too many requests" / "429" but not Anthropic's `overloaded_error` (structurally a rate-limit signal). Falls through to `Other` and no retry fires — user saw a one-shot failure on transient backend pressure. Now any error string containing "overloaded" routes to `ErrorKind::RateLimit` and triggers the exponential-backoff retry. ## Round B — MCP hardening ### MCP server init timeout (10s) `serve_client((), transport).await` had no upper bound. A command-based MCP server that hung on `initialize` (waiting for stdin / wedged binary) would pin dirge's startup indefinitely. Now wrapped in `tokio::time::timeout(MCP_INIT_TIMEOUT)`; past 10s we abort, log the failure, and continue with the other servers. ### Empty `EXA_API_KEY` skips Exa default registration A user with `EXA_API_KEY=""` (explicit empty, e.g. from a `.envrc` that intentionally clears it) used to register the Exa server anyway, then every web-search call failed with 401 at first use. Now treats empty key the same as unset — Exa default skipped, no broken server in the list. ### MCP tool `inputSchema` null fallback to `{}` Servers that omit `inputSchema` had it serialized as JSON `null`, which rig's tool registration treats as an invalid parameters block. The tool became unusable. Now substitute an empty object: the tool stays callable; the LLM sees "no params" correctly. ## Tests 3 new tests, written failing first: - `classify_anthropic_overloaded_error_as_retryable` (2 cases) - `placeholder_pattern_is_detectable` (`/cd` whitespace fix is observable in the existing /cd tests if present; verified by manual trace through the parsing path.) Total: 635 pass (was 633), 0 fail across all build profiles (`--features plugin`, `--all-features`, `--no-default-features`). ## Deferred from this audit (bigger scope) - **Compression prunes sibling branches** — needs a tree-walk during `compress` to preserve sibling subtrees whose parents get dropped. Real bug, real risk on branched sessions, but the fix touches the compress data flow significantly. - **Subagent permission/sandbox/hooks** — `task` tool's `btw_query` is a bare LLM call with no permission/sandbox inheritance and no plugin hook dispatch. By design today, but the safety properties are not what users expect. - **`convert_history` loses tool call structure** — pre-existing finding; resuming a session shows the LLM text-only traces of prior tool calls, losing the structured tool_use markers. - **README "events are buffered" claim** — implementation streams live; doc says buffered. Tokens are NOT re-emitted on retry but the user already saw the partial. - **Multi-plugin harness-block last-write-wins** — design question (queue vs. first-wins) deferred for discussion. - **`/clear` no confirmation** — design choice; standard for CLI. - **`/quit` save-before-break** — needs verification; the outer loop may already save on Interrupted. ## Test plan - [x] `cargo test --features plugin` -> 635 pass, 0 fail. - [x] `cargo build --all-features` -> compiles, no warnings. - [x] `cargo build --no-default-features` -> compiles. Co-authored-by: Yogthos <yogthos@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Five rounds of audit fixes from the codebase audit.
Round 1 — Security + crashes (CRITICAL/HIGH)
Round 2 — Silent failures (HIGH/MEDIUM)
Round 3 — Theme + UI polish (CRITICAL/HIGH/MEDIUM)
Round 4 — Schema + permission hygiene (MEDIUM)
Round 5 — Low hygiene (LOW)
Totals