LSP Phase 9: README + CONFIG docs + manual test plan - #33
Merged
Conversation
README.md - New ## LSP integration section above semantic-tools. Covers the tool matrix (read warms / write+edit surface diagnostics / lsp tool for ad-hoc queries), the four built-in servers + their root resolution, and the disable mechanism (--no-lsp / lsp:false in config). CONFIG.md - New ## LSP configuration section between MCP and ACP. Documents the three accepted forms (bool / per-server map), per-server fields, CLI flag, built-in server commands, lazy spawn + dedupe behavior, known limitations (extensions override currently ignored; 4 built-ins in v1). - Links to docs/LSP_MANUAL_TEST.md for the live-server smoke test. docs/LSP_MANUAL_TEST.md (new) - Six scenarios for verifying the end-to-end LSP path that unit tests can't reach (real rust-analyzer process): 1. Diagnostic surfacing on a deliberately broken edit 2. lsp tool: hover at a known position 3. workspaceSymbol search returns matches 4. Concurrent edits don't spawn duplicate servers 5. --no-lsp + --no-default-features path 6. Broken spawn doesn't retry (broken-set behavior) - Failure-mode notes for each scenario. - Explicit list of what the manual test does NOT cover (already exercised by unit tests). No source changes. Tests still pass (426). End of the LSP stack.
yogthos
force-pushed
the
feature/lsp-phase-9-docs
branch
from
May 20, 2026 01:57
cf5aad5 to
2b5e560
Compare
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>
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>
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.
Phase 9 of 9 — last one. Stacked on #32 (Phase 8).
Pure docs. No source changes.
README.md
New `## LSP integration` section above semantic-tools. Tool matrix, the four built-in servers (rust-analyzer / typescript-language-server / pyright / clojure-lsp) and their per-server root-finding rules, disable mechanism.
CONFIG.md
New `## LSP configuration` section between MCP and ACP. Documents:
Links to the manual test plan.
docs/LSP_MANUAL_TEST.md (new)
Six scenarios that exercise the live rust-analyzer path that CI can't reach:
Each scenario lists failure modes to watch for. Explicit "what this plan doesn't cover" section pointing at the unit tests for the things they already exercise (framing, dedupe, URI round-trips, etc.).
Test plan
End of the LSP stack
PRs #25 → #26 → #27 → #28 → #29 → #30 → #31 → #32 → #33 form the dependency chain. Once #25 lands the rest auto-retarget to main (assuming they're merged in order).
Total: ~6000 lines including tests, 120 LSP-specific unit tests, default-on `lsp` feature, opt-out via `--no-lsp` or `--no-default-features --features 'loop git-worktree mcp'`.