Skip to content

LSP Phase 4: orchestrator (lazy spawn + inflight dedup + fan-out) - #28

Merged
yogthos merged 2 commits into
mainfrom
feature/lsp-phase-4-orchestrator
May 20, 2026
Merged

LSP Phase 4: orchestrator (lazy spawn + inflight dedup + fan-out)#28
yogthos merged 2 commits into
mainfrom
feature/lsp-phase-4-orchestrator

Conversation

@yogthos

@yogthos yogthos commented May 19, 2026

Copy link
Copy Markdown
Collaborator

Phase 4 of 9. Stacked on #27 (Phase 3). Two commits:

  1. `Consolidate URI helpers + switch notify_open to tokio::fs` — moves the duplicate `path_to_file_uri` / `percent_encode_path` / `uri_to_path` / `percent_decode` helpers from `init.rs` and `client.rs` into a shared `src/lsp/uri.rs` module. Both call sites now import the same code. `notify_open` switches from `std::fs::canonicalize` + `std::fs::read_to_string` to the tokio equivalents so the orchestrator can fan `touch_file` out without blocking the runtime thread.

  2. `LSP Phase 4: orchestrator with lazy spawn, inflight dedup, fan-out` — the headliner.

What's in this PR

`src/lsp/spawn.rs` — Spawner trait + ProcessSpawner

  • Trait so the orchestrator unit-tests against in-memory duplex pipes (no real `rust-analyzer` in CI).
  • `Spawned { reader, writer, init_options, guard }` — the guard is `Box<dyn Any + Send + Sync>` holding the `tokio::process::Child` (or fake-server JoinHandle for tests). Drop terminates.
  • `ProcessSpawner` spawns via `tokio::process::Command` with `kill_on_drop(true)` and drains stderr in the background so chatty servers like rust-analyzer don't deadlock on a full pipe.

`src/lsp/manager.rs` — `LspManager`

  • `get_clients(file)`: walks the builtin registry, picks servers that claim the extension, resolves workspace root via `server.root()`, spawns lazily, caches by `(root, server_id)`. Inflight spawns are deduped via `Arc` — concurrent agent tool calls never race two rust-analyzer processes for the same workspace.
  • Broken-set blocks retry on failed spawns.
  • `touch_file(path, TouchMode)`: `Notify` (didOpen/didChange only) or `AwaitPush { after, timeout }` (additionally block on `wait_for_push` so the edit tool can surface fresh diagnostics in its tool result).
  • Fan-out helpers for the agent tool that lands in Phase 5: `hover`, `definition`, `references`, `implementation`, `document_symbol`, `workspace_symbol`, `prepareCallHierarchy` + `incoming/outgoingCalls`. Per-client errors are debug-logged and swallowed — one slow server doesn't poison the result.
  • On `Drop`, every cached client's `guard` drops and the child process is killed.

Test coverage

  • 3 spawn tests: initialize handshake round-trips through the mock, calls recorded, failure path works.
  • 8 manager tests including regressions:
    • First call spawns + caches; second call cache-hits without spawning.
    • Regression: concurrent `get_clients` only spawns ONE process — three parallel callers with a 40 ms delay all observe a single spawn count.
    • Regression: failed spawn marks broken + no retry — second call must not re-attempt.
    • No-server extension returns empty without spawning.
    • `touch_file` calls `notify_open` on the attached client.
    • Manager drop doesn't deadlock on the state mutex.

Code review fixes applied before push

  • Notify race: subscribers arriving after `notify_waiters` fires would hang. Added a 60 s safety timeout on the joiner-side await; the cache is authoritative either way, so re-check after the wait terminates regardless of how.
  • Lock-held-during-logging: moved `tracing::warn` calls in `get_or_spawn` out of the state-lock critical section via a `SpawnOutcome` enum that threads outcome past lock release.

Test plan

  • `cargo build` clean
  • `cargo test --bin dirge -- --skip plugin` → 391 passing (was 380; +11 from Phase 4)
  • `cargo fmt --check` clean
  • End-to-end against real rust-analyzer comes in Phase 6 once the edit tool wires this up.

Next: Phase 5 (agent-facing `lsp` tool that exposes the orchestrator to the LLM).

@yogthos
yogthos changed the base branch from feature/lsp-phase-3-lifecycle to main May 20, 2026 01:46
Yogthos added 2 commits May 19, 2026 21:48
Move path_to_file_uri / uri_to_path / percent_encode_path /
percent_decode out of init.rs and client.rs into a shared src/lsp/uri.rs
module. Both files now import the same helpers. Tests for the URI
behavior consolidated alongside the implementation.

Switch LspClient::notify_open from std::fs to tokio::fs::canonicalize +
tokio::fs::read_to_string so the orchestrator can parallelize touches
across multiple clients without blocking the runtime thread.

New uri.rs tests (8): safe-char passthrough, special-char encoding
regression, round-trip for hash/space/question-mark paths, non-file
scheme rejection, invalid percent escape passthrough, lsp-types Uri
parse, multibyte UTF-8 per-byte encoding.

Removed 4 duplicate tests from init.rs/client.rs that now live in uri.rs.

Suite: 380 -> 382 (net +2 from the new uri.rs tests after dedup).
src/lsp/spawn.rs
- Spawner trait + Spawned struct. Trait is unit-testable via MockSpawner
  (duplex pipes + fake server task that answers initialize); production
  uses ProcessSpawner with tokio::process::Command + kill_on_drop(true).
- Spawned.guard is Box<dyn Any + Send + Sync> — opaque to the manager,
  holds whatever the spawner needs to live for the child's lifetime
  (Child or JoinHandle). Drop terminates the connection.
- ProcessSpawner drains stderr in the background so a chatty LSP server
  (rust-analyzer logs there) doesn't fill the pipe and stall the child.
- 3 spawn tests: initialize handshake round-trips through the mock,
  spawn calls recorded, failure path works.

src/lsp/manager.rs (LspManager)
- One per agent session. Threaded through main like BackgroundStore in
  the bg-notifications work.
- get_clients(file): walks the builtin registry, picks servers that
  claim the extension, resolves workspace root via server.root(),
  spawns lazily, caches by (root, server_id). Inflight spawns are
  deduped via Arc<Notify> so concurrent agent tool calls never race
  two rust-analyzer processes for the same workspace. Broken-set
  blocks retry on failed spawns.
- touch_file(path, mode): notify_open on each claiming client.
  TouchMode::AwaitPush additionally blocks on wait_for_push so the
  edit tool can surface fresh diagnostics in its tool result.
- Fan-out helpers for Phase 5's agent tool: hover, definition,
  references, implementation, document_symbol, workspace_symbol,
  prepareCallHierarchy + incoming/outgoingCalls. Per-client errors
  are logged at debug and swallowed — one slow server doesn't poison
  the result.
- all_diagnostics() aggregates across attached clients.

8 manager tests including regressions:
- concurrent get_clients only spawns ONE process (regression — without
  inflight dedup, every parallel tool call would race a fresh process)
- failed spawn marks (root, server_id) broken and no retry (regression
  — hammering a broken server every tool call would be expensive)
- no-server extension returns empty without spawning
- touch_file calls notify_open on the attached client
- manager drop doesn't deadlock on the state mutex

Code review fixes applied before push:
- Notify race: subscribers arriving AFTER notify_waiters would hang.
  Added a 60s safety timeout on the joiner-side await; cache is
  authoritative regardless of how the wait terminates.
- Moved tracing log calls out of the state-lock critical section in
  get_or_spawn so spawn-failure logging doesn't extend lock hold time.
  Introduced a SpawnOutcome enum to thread the outcome past lock release.

Phases 1-3: 75 tests; Phase 4: +11 → 86 LSP tests. Suite: 380 -> 391.
@yogthos
yogthos force-pushed the feature/lsp-phase-4-orchestrator branch from b5424e5 to 5451563 Compare May 20, 2026 01:48
@yogthos
yogthos merged commit 4c54633 into main May 20, 2026
1 check passed
@yogthos
yogthos deleted the feature/lsp-phase-4-orchestrator branch May 20, 2026 01:49
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>
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