Skip to content

LSP Phase 2: JSON-RPC framing + request correlation + initialize handshake - #26

Merged
yogthos merged 1 commit into
mainfrom
feature/lsp-phase-2-jsonrpc
May 20, 2026
Merged

LSP Phase 2: JSON-RPC framing + request correlation + initialize handshake#26
yogthos merged 1 commit into
mainfrom
feature/lsp-phase-2-jsonrpc

Conversation

@yogthos

@yogthos yogthos commented May 19, 2026

Copy link
Copy Markdown
Collaborator

Phase 2 of 9. Stacked on #25 (Phase 1).

Builds the JSON-RPC plumbing every LSP server speaks. All unit-tested against in-memory `tokio::io::duplex` pipes — no real LSP server needed for CI.

What's in this PR

`src/lsp/jsonrpc.rs` — Content-Length framing

  • `encode_frame(writer, body)`: writes `Content-Length: N\r\n\r\n` in one async go.
  • `decode_frame(reader)`: parses header lines (lenient on bare `\n` for non-spec-conformant servers), reads body, errors on missing/malformed `Content-Length`, bodies > 16 MB, or EOF mid-frame.
  • 14 tests: multibyte payload round-trip, multi-frame from one reader, byte-at-a-time streaming, error cases.

`src/lsp/rpc.rs` — request/response correlation

  • `RpcClient` spawns a background reader task. Frames are routed:
    • response (has `id`, no `method`) → resolve pending oneshot
    • notification (has `method`, no `id`) → dispatch to registered handler
    • server→client request (has both) → ack with null result (we register no client-side request capabilities in v1)
  • Outbound writes serialize through a `Mutex` so concurrent callers can't interleave frames.
  • `request<P, R>(method, params, timeout)`, `notify

    (method, params)`, `on_notification(method, handler)`.

  • 10 tests covering round-trip, server-error responses, in-flight correlation by id (regression: out-of-order responses must not resolve the wrong future), timeout cleanup, peer-close handling.

`src/lsp/init.rs` — initialize handshake

  • `initialize(client, root, pid, init_options) -> InitializeResult`
  • 45 s timeout matching opencode.
  • Sends both `rootUri` (deprecated but universally read) and `workspaceFolders`.
  • Sends `initialized` notification after the response — some servers stall without it.
  • Conservative client capabilities (sync, publishDiagnostics, pull diagnostic, workspace configuration).
  • 6 tests covering capability round-trip, rootUri propagation (regression: rust-analyzer attaches at wrong directory otherwise), initializationOptions propagation, `null` options OMIT the field (some servers reject explicit null), notification ordering, percent-encoding for special chars.

Code review fixes applied before push

  • Dropped a confusing cast chain on `process_id` — `lsp-types` already uses `Option`.
  • Added RFC 3986 `percent_encode_path` so paths with `#`/`?`/space produce valid `file://` URIs; 2 dedicated tests.
  • Doc-noted the small race window in `RpcClient::request` where a peer close interleaves with a new request (worst case: caller waits for its own timeout).

Dependencies

  • `lsp-types 0.97` (typed Initialize structs only — wire layer stays untyped `Value`)
  • `tokio` `io-util` feature for the async I/O extension traits

Test plan

  • `cargo build` clean
  • `cargo test --bin dirge -- --skip plugin` → 365 passing (was 334; +31 from Phase 2)
  • `cargo fmt --check` clean
  • Manual against rust-analyzer comes in Phase 4 once the orchestrator wires this to real processes.

Next: Phase 3 (file lifecycle: didOpen / didChange with version tracking + push/pull diagnostic state with dedupe).

This was referenced May 19, 2026
Merged
Merged
@yogthos
yogthos changed the base branch from feature/lsp-phase-1-registry to main May 20, 2026 01:46
Yogthos
LSP Phase 2: JSON-RPC framing + request correlation + initialize hand…
7036cbf 
…shake

Three new modules layered on top of Phase 1's read-only pieces. All
unit-tested against in-memory duplex pipes — no real LSP server needed
for CI.

src/lsp/jsonrpc.rs (in-house framing)
- encode_frame: Content-Length + body, single async write.
- decode_frame: parses headers, reads body, errors on missing/
  malformed Content-Length, on bodies > 16 MB cap, on EOF mid-frame.
- Tolerant of bare \n line endings (some servers don't follow spec).
- 14 tests including roundtrip with multibyte payloads, multi-frame
  decoding from one reader, byte-at-a-time streaming, error cases.

src/lsp/rpc.rs (request correlation)
- RpcClient: spawns a background reader task, routes incoming frames
  to pending requests (by id), notification handlers (by method), or
  acks server-to-client requests with null results.
- Outbound writes serialize through a Mutex so concurrent callers
  can't interleave frames.
- request<P, R>(method, params, timeout) -> Future<Result<R, RpcError>>
- notify<P>(method, params) -> Future<Result<(), RpcError>>
- on_notification(method, handler) registers handlers
- 10 tests covering: round-trip, server-error responses, in-flight
  correlation by id (regression for out-of-order responses), timeout
  fires + clears pending entry, fire-and-forget notify, server-pushed
  notifications dispatch, server-to-client requests acked with null,
  peer-close fails in-flight + after-close requests with
  ConnectionClosed.

src/lsp/init.rs (initialize handshake)
- initialize(client, root, pid, init_options) -> InitializeResult
- 45s timeout matching opencode.
- path_to_file_uri does RFC-3986 percent-encoding so paths with #, ?,
  spaces, etc. produce valid file:// URIs.
- Sends rootUri (deprecated but universally read) AND workspaceFolders
  for compat with both old and modern servers.
- Sends 'initialized' notification after the response — some servers
  stall without it.
- 6 tests: capabilities round-trip, rootUri carries the provided path,
  initializationOptions propagate, null options OMIT the field (some
  servers reject explicit null), notification follows response,
  percent-encoding of special chars and preservation of safe chars.

Deps added:
- lsp-types 0.97 for InitializeParams/InitializeResult typing
- tokio io-util feature for AsyncReadExt/AsyncWriteExt/AsyncBufReadExt

Code review fixes (applied before push):
- Dropped a tortured cast chain on process_id (lsp-types already uses
  Option<u32>).
- Added percent_encode_path so paths with #/?/space don't produce
  malformed URIs; 2 dedicated tests.
- Doc-noted the tiny race between RpcClient::request closed-check and
  read_loop's pending drain on peer close.

Phase 1: 29 tests, Phase 2: +31 → 60 LSP tests total.
Suite: 305 -> 365 passing.
@yogthos
yogthos force-pushed the feature/lsp-phase-2-jsonrpc branch from 5cf5dc6 to 7036cbf Compare May 20, 2026 01:47
@yogthos
yogthos merged commit 80356d0 into main May 20, 2026
1 check passed
@yogthos
yogthos deleted the feature/lsp-phase-2-jsonrpc branch May 20, 2026 01:47
yogthos added a commit that referenced this pull request May 21, 2026
@yogthos Yogthos
fix: audit batch — 10 verified bugs across security, UI, and plugin p…
742e875 
…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
@yogthos Yogthos
fix: audit batch — 10 verified bugs across security, UI, and plugin p…
86506ec 
…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

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@yogthos