Skip to content

LSP Phase 5: agent-facing lsp tool - #29

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

LSP Phase 5: agent-facing lsp tool#29
yogthos merged 1 commit into
mainfrom
feature/lsp-phase-5-tool

Conversation

@yogthos

@yogthos yogthos commented May 19, 2026

Copy link
Copy Markdown
Collaborator

Phase 5 of 9. Stacked on #28 (Phase 4).

The agent-facing tool that dispatches to the Phase 4 `LspManager`. Mirrors opencode's `tool/lsp.ts` surface so agents' mental model carries over.

What's in this PR

`src/agent/tools/lsp.rs` (LspTool)

One tool, one `operation` parameter. Operations:

  • `definition` / `references` / `hover`
  • `documentSymbol` / `workspaceSymbol`
  • `implementation` / `prepareCallHierarchy` / `incomingCalls` / `outgoingCalls`

Accepts both camelCase (`goToDefinition`) and short (`definition`) aliases.

Coordinates: 1-based line/character (matches editor display); converts to 0-based before sending to the LSP wire. `saturating_sub` guards against agents passing `0`.

Paths: relative `file_path` resolved against the dirge worktree cwd. Permission check via `check_perm_path` on the resolved absolute path.

File sync: `touch_file(TouchMode::Notify)` before each request so the file's in sync with the server. `Notify` (not `AwaitPush`) — diagnostics are the edit tool's concern in Phase 6.

Empty results: reported as `(no results from )` rather than `[]` so the agent doesn't have to special-case.

Test coverage (14 tests, includes regressions)

  • Unknown operation returns clear error (regression — bad name shouldn't panic or produce a cryptic parse error)
  • Position op without `file_path` errors (regression — would crash a `manager.hover(None, ...)` otherwise)
  • Position op without line/character errors
  • `workspaceSymbol` requires `file_path` (regression — without one, no LSP attaches and the request silently returns nothing)
  • `workspaceSymbol` requires `query`
  • Missing file on disk returns clean error
  • 1-based → 0-based coordinate conversion (regression — off-by-one would land the LSP cursor on the wrong identifier)
  • `documentSymbol` accepts no position (regression — `needs_position` must not include doc/workspace symbol)
  • Successful hover returns pretty-printed JSON
  • Empty array / null response reports `(no results)`
  • Accepts opencode camelCase aliases
  • Relative `file_path` resolves against cwd (regression)
  • `line=0` / `character=0` doesn't panic (saturating_sub guards)

Code review fixes applied before push

  • `workspaceSymbol` silent-failure mode: tool initially marked `file_path` optional, but the manager picks which LSP server to ask based on the file's extension. With no `file_path` (cwd has no extension) no server matches and the request returns silently. Fixed: `file_path` is now required for every operation (matches opencode). Added regression test.
  • `Operation::from_str`: renamed to `parse` to avoid shadowing `std::str::FromStr`.

Scope

This PR lands the tool + tests. The `builder.rs` wiring that actually attaches the tool to a running agent comes in Phase 7 with the rest of the CLI/config plumbing (Option<Arc> threading through `build_channels` / `build_agent` / slash rebuilds, ProcessSpawner construction with default server commands). The Phase 5 tool symbols carry `#![allow(dead_code)]` until then.

Test plan

  • `cargo build` clean
  • `cargo test --bin dirge -- --skip plugin` → 405 passing (was 391; +14 from Phase 5)
  • `cargo fmt --check` clean
  • End-to-end against real rust-analyzer comes in Phase 6+9 once the orchestrator is wired into the agent.

Next: Phase 6 (write/edit tool diagnostic integration — surface LSP errors in tool output via `TouchMode::AwaitPush`).

@yogthos
yogthos changed the base branch from feature/lsp-phase-4-orchestrator to main May 20, 2026 01:46
src/agent/tools/lsp.rs (LspTool)
- One tool, one `operation` parameter. Operations mirror opencode's
  surface so agents' mental model carries over:
    definition / references / hover / documentSymbol / workspaceSymbol /
    implementation / prepareCallHierarchy / incomingCalls / outgoingCalls
- Accepts both camelCase (goToDefinition) and short (definition) aliases.
- 1-based line/character (matches editor display); converts to 0-based
  before sending to the LSP wire. saturating_sub guards against agents
  passing 0.
- Relative file_path resolved against the dirge worktree cwd.
- Permission check via check_perm_path on the resolved absolute path.
- touch_file(TouchMode::Notify) before each request so the file is in
  sync with the server. Notify (not AwaitPush) — diagnostics are the
  edit tool's concern in Phase 6.
- Empty result reported as '(no results from <op>)' rather than '[]'
  so the agent doesn't have to special-case.

14 tool tests including regressions:
- unknown operation returns clear error (regression — bad name shouldn't
  panic or produce a cryptic parse error)
- position op without file_path errors (regression — would crash a
  manager.hover(None, ...) otherwise)
- position op without line/character errors
- workspaceSymbol requires file_path (regression — without one, no LSP
  attaches; silent empty result)
- workspaceSymbol requires query
- missing file on disk returns clean error
- 1-based -> 0-based coordinate conversion (regression for off-by-one
  that would land the LSP cursor on the wrong identifier)
- documentSymbol accepts no position (regression — needs_position must
  not include doc/workspace symbol)
- successful hover returns pretty-printed JSON
- empty array / null response reports '(no results)'
- accepts opencode camelCase aliases
- relative file_path resolves against cwd (regression)
- line=0 / character=0 doesn't panic (saturating_sub guards)

Builder.rs wiring (Option<Arc<LspManager>> threaded through, tool
attached when manager is present) lands in Phase 7 with the rest of the
CLI/config plumbing. Phase 5's tool symbols are #![allow(dead_code)]
until then to keep the warning surface clean.

Phase 4: 86, Phase 5: +14 -> 100 LSP tests. Suite: 391 -> 405.
@yogthos
yogthos force-pushed the feature/lsp-phase-5-tool branch from 3c912cb to eb1761e Compare May 20, 2026 01:52
@yogthos
yogthos merged commit c31cc58 into main May 20, 2026
@yogthos
yogthos deleted the feature/lsp-phase-5-tool branch May 20, 2026 01:52
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
…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>
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