fix(audit): deferred batch — H1/H3/H4/H5/H6/H9 + remove dead ACP config - #121
Merged
Conversation
Continues from PR #120 (security-critical). Six deferred items shipped. ## H1: edit + apply_patch unbounded file reads `edit.rs` and `apply_patch.rs` (apply_update) called `tokio::fs::read` / `std::fs::read_to_string` with no size guard. An LLM pointing them at a multi-GB log/binary would OOM the agent process. Added 100 MiB pre-flight metadata check in both. The cap is generous (every realistic source file fits) but rejects the pathological case fast with a clear error pointing the agent at `bash + sed/awk` for huge files. ## H3: apply_patch blocking std::fs in async All four `apply_*` helpers (create / update / delete / rename) used synchronous `std::fs` calls inside an async `call` method. On the tokio runtime with limited worker threads, slow I/O can starve the event loop. Switched each to `tokio::fs` and made them async. Tests updated from `#[test]` to `#[tokio::test]` (8 tests touched). ## H4 + H5: PermissionConfig fields for missing tools `PermissionConfig` only had fields for 11 tools (bash, read, write, edit, grep, find_files, list_dir, write_todo_list, apply_patch, lsp, question). 12 other tools the agent can call had no way to be individually gated — they always fell through to the global default action. Added 12 fields covering the missing surface: - Web: `webfetch`, `websearch` - Subagent / state: `task`, `memory`, `skill` - Semantic (tree-sitter): `list_symbols`, `get_symbol_body`, `find_definition`, `find_callers`, `find_callees` - MCP umbrella: `mcp_tool` — pattern rules match against the full `mcp_tool:<server>:<tool>` key, so `{"mcp_tool:filesystem:*": "deny"}` blocks every tool from a filesystem MCP server. `PermissionChecker::new` now loads rules for all 23 tool names. `is_path_tool_name` extended to mark the path-bearing semantic tools (`list_symbols`, `get_symbol_body`, `find_callees`) plus `grep`/`find_files`/`glob` whose new path-side perm check (PR #120) uses path-glob semantics. The `/allow` slash command's whitelist grew to match — typos on the new tool names still get caught. ## H6: MCP connect / list_tools errors visible to user `McpClientManager::connect_all` and `collect_tools` logged failures at `tracing::warn!` only. Default-log users (no `RUST_LOG` / `--verbose`) saw nothing — configured MCP servers just silently failed to register tools. Added an `eprintln!` alongside each warn so the failure surfaces to stderr unconditionally. Message names the server and tells the user the tools won't be available this session. ## H9 + H10: removed dead acp_host / acp_port config keys Both keys were parsed by serde but never consumed anywhere. ACP transport is stdio-only by design. Removed from `config::Config`. CONFIG.md already (PR #112) documents the removal; no further docs change needed. `acp_servers` kept — it has a CONFIG.md entry, holds future TCP / Unix-socket config, and isn't actively misleading users. ## CONFIG.md Rewrote the Permission tool-keys list from a single sentence into a categorized list covering all 23 tools + the `mcp_tool:<server>:<tool>` pattern syntax. ## Tests 8 `apply_patch` tests converted to `#[tokio::test]` (no new assertions — the conversion was mechanical so the existing coverage still applies). 813 all-features / 730 plugin / 604 default pass — same as PR #120 plus one new test there. ## Not in this PR (still deferred) - **C4**: unregistered-tool default-allow. This PR ADDS fields for the 12 missing tools, so the original "unregistered" set is now mostly covered. The remaining gap (future MCP tools, third-party plugins) is by design — `mcp_tool` is the umbrella key. - **C7**: LSP didClose. Needs lifecycle design (when to fire? on file delete? session end?). Will tackle as a focused PR. - **H8**: LSP `extensions` config field ignored. Real gap but bigger plumbing change; separate PR. ## Verified false positives (re-confirmed from earlier review) - H2 empty bash: `quote_aware_split` filters empty segments. - H7 LSP spawn silent: broken-set + backoff IS the feedback. - H11 plugin dedup: summary IS emitted. - H12 token counter: estimate_tokens on partial is correct.
yogthos
added a commit
that referenced
this pull request
May 21, 2026
…122) Two LSP gaps from the audit, both real. ## C7: missing didClose → server-side state leak LSP servers retain parse trees + diagnostic caches keyed on every `textDocument/didOpen`. dirge never sent `textDocument/didClose`, so a long session editing dozens of files left rust-analyzer / pyright / etc. holding all that state for the lifetime of the server process. Added two methods on `LspClient`: - `notify_close(path)`: send didClose for one file, drop the associated FileState / push + pull diagnostics / last-push timestamp from local tracking. Returns Ok on never-opened files (a stray didClose is harmless per the spec). - `close_all()`: fan didClose across every tracked file. Used at session shutdown. `LspManager::close_all_files` fans across every attached client. Called from main.rs after the interactive loop returns. Best-effort: an already-dead server isn't re-contacted and individual notify_close failures are swallowed. The `lsp_manager` Arc is cloned before being moved into `ui::run_interactive` so the shutdown call site still has a handle. Two new regression tests pin the behavior: - `notify_close_emits_did_close_and_clears_state` — checks that subsequent `notify_open` after close emits a fresh didOpen (version 0) rather than a didChange. - `close_all_emits_did_close_for_every_open_file` — two open files → two didClose frames sent. ## H8: LspServerConfig.extensions ignored `LspServerConfig` carries an `extensions: Option<Vec<String>>` field, parsed by serde, but `LspManager` always used the hardcoded `builtin_servers()` registry. User config like `{ "lsp": { "rust": { "extensions": ["rs", "rlib"] } } }` was silently dropped. Two changes: 1. `ServerInfo.extensions` changed from `&'static [&'static str]` to `Vec<String>` so it can be mutated post-construction. `builtin_servers()` now produces owned vecs; this is a one-time cost paid at session startup. 2. New `apply_extension_overrides` function in `lsp::server` walks the user's per-server config and replaces builtin extension lists (or removes the server entirely if `disabled: true`). Lowercases + strips leading dots to match how `servers_for_extension` looks up by extension. 3. `LspManager::with_servers` constructor accepts the modified server list. The existing `new` delegates with the unmodified builtin list. 4. `main.rs::build_channels` applies overrides between `builtin_servers()` and `LspManager::with_servers(...)`. The `compile_lsp_commands` docstring previously called the extensions field a follow-up; updated to point at the new override path. A trait shim (`AsExtensionOverride`) keeps the `lsp::server` module from depending directly on `config::LspServerConfig` — keeps the dep graph clean and lets the tests use a stub. 4 new regression tests: - Extensions override replaces builtin list. - `disabled: true` removes server from the active set. - Unknown server id silently ignored (no panic, no spurious entry added). - User-supplied extensions are normalized (leading-dot strip + lowercase). ## Status 736 plugin / 610 default pass (was 730 / 604). All build profiles + fmt clean. This closes the last two real items in the original audit's deferred list. PR #120 → C1/C2/C3/C5/C6. PR #121 → H1/H3/H4/H5/ H6/H9/H10. This PR → C7/H8. All verified false positives (H2, H7, H11, H12) remain unchanged. Co-authored-by: Yogthos <yogthos@gmail.com>
allen-munsch
pushed a commit
to allen-munsch/dirge
that referenced
this pull request
Jun 3, 2026
…ig (dirge-code#121) Continues from PR dirge-code#120 (security-critical). Six deferred items shipped. ## H1: edit + apply_patch unbounded file reads `edit.rs` and `apply_patch.rs` (apply_update) called `tokio::fs::read` / `std::fs::read_to_string` with no size guard. An LLM pointing them at a multi-GB log/binary would OOM the agent process. Added 100 MiB pre-flight metadata check in both. The cap is generous (every realistic source file fits) but rejects the pathological case fast with a clear error pointing the agent at `bash + sed/awk` for huge files. ## H3: apply_patch blocking std::fs in async All four `apply_*` helpers (create / update / delete / rename) used synchronous `std::fs` calls inside an async `call` method. On the tokio runtime with limited worker threads, slow I/O can starve the event loop. Switched each to `tokio::fs` and made them async. Tests updated from `#[test]` to `#[tokio::test]` (8 tests touched). ## H4 + H5: PermissionConfig fields for missing tools `PermissionConfig` only had fields for 11 tools (bash, read, write, edit, grep, find_files, list_dir, write_todo_list, apply_patch, lsp, question). 12 other tools the agent can call had no way to be individually gated — they always fell through to the global default action. Added 12 fields covering the missing surface: - Web: `webfetch`, `websearch` - Subagent / state: `task`, `memory`, `skill` - Semantic (tree-sitter): `list_symbols`, `get_symbol_body`, `find_definition`, `find_callers`, `find_callees` - MCP umbrella: `mcp_tool` — pattern rules match against the full `mcp_tool:<server>:<tool>` key, so `{"mcp_tool:filesystem:*": "deny"}` blocks every tool from a filesystem MCP server. `PermissionChecker::new` now loads rules for all 23 tool names. `is_path_tool_name` extended to mark the path-bearing semantic tools (`list_symbols`, `get_symbol_body`, `find_callees`) plus `grep`/`find_files`/`glob` whose new path-side perm check (PR dirge-code#120) uses path-glob semantics. The `/allow` slash command's whitelist grew to match — typos on the new tool names still get caught. ## H6: MCP connect / list_tools errors visible to user `McpClientManager::connect_all` and `collect_tools` logged failures at `tracing::warn!` only. Default-log users (no `RUST_LOG` / `--verbose`) saw nothing — configured MCP servers just silently failed to register tools. Added an `eprintln!` alongside each warn so the failure surfaces to stderr unconditionally. Message names the server and tells the user the tools won't be available this session. ## H9 + H10: removed dead acp_host / acp_port config keys Both keys were parsed by serde but never consumed anywhere. ACP transport is stdio-only by design. Removed from `config::Config`. CONFIG.md already (PR dirge-code#112) documents the removal; no further docs change needed. `acp_servers` kept — it has a CONFIG.md entry, holds future TCP / Unix-socket config, and isn't actively misleading users. ## CONFIG.md Rewrote the Permission tool-keys list from a single sentence into a categorized list covering all 23 tools + the `mcp_tool:<server>:<tool>` pattern syntax. ## Tests 8 `apply_patch` tests converted to `#[tokio::test]` (no new assertions — the conversion was mechanical so the existing coverage still applies). 813 all-features / 730 plugin / 604 default pass — same as PR dirge-code#120 plus one new test there. ## Not in this PR (still deferred) - **C4**: unregistered-tool default-allow. This PR ADDS fields for the 12 missing tools, so the original "unregistered" set is now mostly covered. The remaining gap (future MCP tools, third-party plugins) is by design — `mcp_tool` is the umbrella key. - **C7**: LSP didClose. Needs lifecycle design (when to fire? on file delete? session end?). Will tackle as a focused PR. - **H8**: LSP `extensions` config field ignored. Real gap but bigger plumbing change; separate PR. ## Verified false positives (re-confirmed from earlier review) - H2 empty bash: `quote_aware_split` filters empty segments. - H7 LSP spawn silent: broken-set + backoff IS the feedback. - H11 plugin dedup: summary IS emitted. - H12 token counter: estimate_tokens on partial is correct. Co-authored-by: Yogthos <yogthos@gmail.com>
allen-munsch
pushed a commit
to allen-munsch/dirge
that referenced
this pull request
Jun 3, 2026
…irge-code#122) Two LSP gaps from the audit, both real. ## C7: missing didClose → server-side state leak LSP servers retain parse trees + diagnostic caches keyed on every `textDocument/didOpen`. dirge never sent `textDocument/didClose`, so a long session editing dozens of files left rust-analyzer / pyright / etc. holding all that state for the lifetime of the server process. Added two methods on `LspClient`: - `notify_close(path)`: send didClose for one file, drop the associated FileState / push + pull diagnostics / last-push timestamp from local tracking. Returns Ok on never-opened files (a stray didClose is harmless per the spec). - `close_all()`: fan didClose across every tracked file. Used at session shutdown. `LspManager::close_all_files` fans across every attached client. Called from main.rs after the interactive loop returns. Best-effort: an already-dead server isn't re-contacted and individual notify_close failures are swallowed. The `lsp_manager` Arc is cloned before being moved into `ui::run_interactive` so the shutdown call site still has a handle. Two new regression tests pin the behavior: - `notify_close_emits_did_close_and_clears_state` — checks that subsequent `notify_open` after close emits a fresh didOpen (version 0) rather than a didChange. - `close_all_emits_did_close_for_every_open_file` — two open files → two didClose frames sent. ## H8: LspServerConfig.extensions ignored `LspServerConfig` carries an `extensions: Option<Vec<String>>` field, parsed by serde, but `LspManager` always used the hardcoded `builtin_servers()` registry. User config like `{ "lsp": { "rust": { "extensions": ["rs", "rlib"] } } }` was silently dropped. Two changes: 1. `ServerInfo.extensions` changed from `&'static [&'static str]` to `Vec<String>` so it can be mutated post-construction. `builtin_servers()` now produces owned vecs; this is a one-time cost paid at session startup. 2. New `apply_extension_overrides` function in `lsp::server` walks the user's per-server config and replaces builtin extension lists (or removes the server entirely if `disabled: true`). Lowercases + strips leading dots to match how `servers_for_extension` looks up by extension. 3. `LspManager::with_servers` constructor accepts the modified server list. The existing `new` delegates with the unmodified builtin list. 4. `main.rs::build_channels` applies overrides between `builtin_servers()` and `LspManager::with_servers(...)`. The `compile_lsp_commands` docstring previously called the extensions field a follow-up; updated to point at the new override path. A trait shim (`AsExtensionOverride`) keeps the `lsp::server` module from depending directly on `config::LspServerConfig` — keeps the dep graph clean and lets the tests use a stub. 4 new regression tests: - Extensions override replaces builtin list. - `disabled: true` removes server from the active set. - Unknown server id silently ignored (no panic, no spurious entry added). - User-supplied extensions are normalized (leading-dot strip + lowercase). ## Status 736 plugin / 610 default pass (was 730 / 604). All build profiles + fmt clean. This closes the last two real items in the original audit's deferred list. PR dirge-code#120 → C1/C2/C3/C5/C6. PR dirge-code#121 → H1/H3/H4/H5/ H6/H9/H10. This PR → C7/H8. All verified false positives (H2, H7, H11, H12) remain unchanged. 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.
Continues PR #120. (H1) 100 MiB cap on edit + apply_patch reads. (H3) apply_patch switched to tokio::fs throughout. (H4+H5) 12 new
PermissionConfigfields covering web/subagent/semantic/MCP tools. (H6) MCP connect failures now print to stderr. (H9+H10) deadacp_host/acp_portremoved. CONFIG.md permission tool-keys list rewritten as categories. 813/730/604 tests pass. C4 effectively closed by the new fields; C7 + H8 still deferred to focused PRs.