fix: audit r4 — websearch timeout, todo cap, modified-files cap, perm denial msgs, temp warning - #105
Merged
Merged
Conversation
… denial messages, temp clamp warning
Six verified bugs from the fourth 6-agent audit pass.
## Bug 1 — websearch had no HTTP timeout (HIGH)
`src/agent/tools/websearch.rs:119` used `reqwest::Client::new()`
with no timeout. A hung Exa endpoint would stall the agent turn
indefinitely. `webfetch.rs` already had a 15s timeout via
`Client::builder().timeout()`; websearch is now matched.
## Bug 2 — TODO_LIST unbounded (MEDIUM)
`src/agent/tools/todo.rs:19` is a `static Mutex<Vec<TodoItem>>`
with no cap. An agent could spam 1000s of todos that then ride
along in every subsequent prompt as context. New `MAX_TODOS =
50` cap with an actionable error ("Trim the list or split the
work across multiple turns").
## Bug 3 — Temperature silently clamped (LOW-MEDIUM)
`src/agent/builder.rs:150` clamped temperature to 0.0..=2.0
without warning. A user with `temperature: 3.5` got 2.0 and
never knew. Now warns to stderr ONCE per process (OnceLock-
guarded) when clamping fires.
## Bug 4 — MODIFIED_FILES set unbounded (MEDIUM)
`src/agent/tools/modified.rs` had an IndexSet with no cap. A
long session editing hundreds of files would grow this set
indefinitely (panel only displays last few entries, but memory
grew). New `MAX_MODIFIED = 256` cap; oldest entries shift out
when full. Panel display unaffected since it already shows only
the tail.
## Bug 5 — Permission denial messages were generic (MEDIUM)
`src/permission/checker.rs::check` and `check_path` returned
bare `"Blocked by permission rules"` with no info on which
rule matched. User had no way to identify what to edit.
`matched: Vec<Action>` now tracks `Vec<(Action, String)>`
where the string is the matching pattern's `original`. Deny
message becomes `"Blocked by rule: <tool> \"<pattern>\" → deny"`
or `"Blocked: <tool> denied by default action"` when no
explicit rule matched.
## Bug 6 — Doom-loop messages didn't name the call (MEDIUM)
`"Doom loop: repeated identical tool call"` left the user
guessing. Now includes the tool name + 60/80-char input preview:
`"Doom loop: repeated identical bash call (echo hi)"`.
## Tests
2 new tests in `tests/checker_tests.rs`:
- `deny_message_names_the_matching_rule` — uses default bash
deny rule on `rm -rf /home/user/project`; asserts the deny
message references the rule (not bare generic).
- `doom_loop_deny_names_the_call` — fires three identical
bash calls; asserts the doom-loop deny names both the tool
("bash") and the call preview ("echo hi").
718 pass (was 716). All build profiles + fmt clean.
## False positives / non-bugs from this round
- `on-session-load` hook "documented but never dispatched":
searched README + docs/PLUGINS.md, no mention. Agent was wrong.
- Markdown re-render thrash during streaming: needs careful
verification + the fix is a complex architectural change.
Deferred.
- Subagent isolation "not schema-enforced": intentional design;
documented in docs/PLUGINS.md.
- Per-provider error classification gaps: speculative without
per-provider testing.
## Other valid findings deferred (feature/architecture work)
- MCP tool name collisions across servers (need namespacing)
- LSP server crash detection via broken-pipe handling
- Memory tool concurrent-write file locking
- Plugin context :turn_index field
- Config JSON parse errors with line/column info
- Outdated model defaults for Anthropic/Gemini
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>
allen-munsch
pushed a commit
to allen-munsch/dirge
that referenced
this pull request
Jun 3, 2026
… denial messages, temp clamp warning (dirge-code#105) Six verified bugs from the fourth 6-agent audit pass. ## Bug 1 — websearch had no HTTP timeout (HIGH) `src/agent/tools/websearch.rs:119` used `reqwest::Client::new()` with no timeout. A hung Exa endpoint would stall the agent turn indefinitely. `webfetch.rs` already had a 15s timeout via `Client::builder().timeout()`; websearch is now matched. ## Bug 2 — TODO_LIST unbounded (MEDIUM) `src/agent/tools/todo.rs:19` is a `static Mutex<Vec<TodoItem>>` with no cap. An agent could spam 1000s of todos that then ride along in every subsequent prompt as context. New `MAX_TODOS = 50` cap with an actionable error ("Trim the list or split the work across multiple turns"). ## Bug 3 — Temperature silently clamped (LOW-MEDIUM) `src/agent/builder.rs:150` clamped temperature to 0.0..=2.0 without warning. A user with `temperature: 3.5` got 2.0 and never knew. Now warns to stderr ONCE per process (OnceLock- guarded) when clamping fires. ## Bug 4 — MODIFIED_FILES set unbounded (MEDIUM) `src/agent/tools/modified.rs` had an IndexSet with no cap. A long session editing hundreds of files would grow this set indefinitely (panel only displays last few entries, but memory grew). New `MAX_MODIFIED = 256` cap; oldest entries shift out when full. Panel display unaffected since it already shows only the tail. ## Bug 5 — Permission denial messages were generic (MEDIUM) `src/permission/checker.rs::check` and `check_path` returned bare `"Blocked by permission rules"` with no info on which rule matched. User had no way to identify what to edit. `matched: Vec<Action>` now tracks `Vec<(Action, String)>` where the string is the matching pattern's `original`. Deny message becomes `"Blocked by rule: <tool> \"<pattern>\" → deny"` or `"Blocked: <tool> denied by default action"` when no explicit rule matched. ## Bug 6 — Doom-loop messages didn't name the call (MEDIUM) `"Doom loop: repeated identical tool call"` left the user guessing. Now includes the tool name + 60/80-char input preview: `"Doom loop: repeated identical bash call (echo hi)"`. ## Tests 2 new tests in `tests/checker_tests.rs`: - `deny_message_names_the_matching_rule` — uses default bash deny rule on `rm -rf /home/user/project`; asserts the deny message references the rule (not bare generic). - `doom_loop_deny_names_the_call` — fires three identical bash calls; asserts the doom-loop deny names both the tool ("bash") and the call preview ("echo hi"). 718 pass (was 716). All build profiles + fmt clean. ## False positives / non-bugs from this round - `on-session-load` hook "documented but never dispatched": searched README + docs/PLUGINS.md, no mention. Agent was wrong. - Markdown re-render thrash during streaming: needs careful verification + the fix is a complex architectural change. Deferred. - Subagent isolation "not schema-enforced": intentional design; documented in docs/PLUGINS.md. - Per-provider error classification gaps: speculative without per-provider testing. ## Other valid findings deferred (feature/architecture work) - MCP tool name collisions across servers (need namespacing) - LSP server crash detection via broken-pipe handling - Memory tool concurrent-write file locking - Plugin context :turn_index field - Config JSON parse errors with line/column info - Outdated model defaults for Anthropic/Gemini 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>
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.
Six verified bugs from fourth audit pass. (1) websearch no HTTP timeout; (2) TODO_LIST unbounded; (3) temperature silently clamped; (4) MODIFIED_FILES set unbounded; (5) permission denial generic — now names the rule; (6) doom-loop names tool + call preview. 2 new tests, 718 pass.