M5: MCP, LSP, commands/skills #6

2026-07-10T17:13:12+02:00

darman commented

2026-07-10 17:13:12 +02:00

LSP pool + diagnostics (harness-core::lsp seam, harness-lsp pool, harness-tools::diagnostics), surfaced in edit/write/bash/glob/grep output
rmcp stdio client + namespaced tool adapters
skills: SKILL.md parsing + skill tool
slash commands (command/*.md)

- LSP pool + diagnostics (`harness-core::lsp` seam, `harness-lsp` pool, `harness-tools::diagnostics`), surfaced in edit/write/bash/glob/grep output - rmcp stdio client + namespaced tool adapters - skills: `SKILL.md` parsing + skill tool - slash commands (`command/*.md`)

darman added 4 commits 2026-07-10 17:13:13 +02:00

M5: LSP pool and diagnostics wiring f71a347061

Adds the DiagnosticsSource seam trait in harness-core::lsp (kept out of harness-tools so tools never link the LSP crate directly), and implements it in harness-lsp as a pool that lazily spawns one language server per file extension (rust-analyzer, typescript-language-server, gopls, pyright-langserver) only when the binary is on PATH. Adds harness-tools::diagnostics and wires diagnostics into edit/write/bash/glob/grep output.

M5: rmcp stdio client and tool adapters fe6d00ce6d

Adds an rmcp-based stdio MCP client in harness-mcp plus adapters exposing a configured server's tools as namespaced harness-tools, with an echo-server fixture and integration test, and wiring through harness-app.

M5: skills (SKILL.md and skill tool) d6af65f494

Adds harness-core::config::markdown to parse SKILL.md frontmatter/body, a skill tool in harness-tools to invoke them, and system-prompt wiring so available skills are advertised to the model. Includes the rustfmt pass over the M5 files touched by this and the preceding LSP/rmcp work.

M5: slash commands (command/*.md) dbc676332a

Adds slash-command support: markdown command files under command/*.md, expansion in harness-app, and TUI input handling so typing /mycommand runs it.

darman requested review from claude-reviewer 2026-07-11 14:42:15 +02:00

claude-reviewer commented

2026-07-11 14:51:52 +02:00

Review: M5 — MCP, LSP, commands/skills

Clean integration work: MCP servers are isolated (one failing server is logged and skipped, the rest connect), the LSP reader auto-acks server→client requests so rust-analyzer's registerCapability/progress requests don't stall the handshake, skills load on demand, and project-over-global layering is consistent across agents/commands/skills. A few issues, the LSP one worth fixing.

LSP `wait_diagnostics` has a `Notify` race that adds full-timeout latency (medium)

client.rs:1160:

loop {
    if let Some(diags) = self.diagnostics.lock().unwrap().get(&uri) { return diags.clone(); }
    let notified = self.diag_notify.notified();          // registered AFTER the map check
    ...
    if tokio::time::timeout(remaining, notified).await.is_err() { break; }
}

tokio::Notify::notify_waiters() (used in dispatch) only wakes waiters already registered — it stores no permit. If the server's publishDiagnostics lands in the window between the map read and self.diag_notify.notified(), the notification is missed and this call blocks for the entire wait duration before falling through and returning the (by-then-present) diagnostics. So it's correct but pays the full timeout on the common fast-publish path, slowing every edit/write that waits on diagnostics. Fix: create the notified() future before checking the map (the standard tokio pattern), or switch to a watch/permit-based signal.

LSP `path_to_uri` doesn't percent-encode (low-medium)

client.rs:1280 builds file:// URIs by raw string concat. The server echoes back a percent-encoded uri in publishDiagnostics, which is then used as the diagnostics map key. For any path containing a space, #, or non-ASCII char, our key (file:///a b.rs) won't match the server's (file:///a%20b.rs), so wait_diagnostics silently returns empty for that file. Percent-encode the path (and decode incoming URIs, or normalize both sides).

`CommandDef::expand` mangles literal `$N` in templates (low)

markdown.rs:483 does out.replace(&format!("${i}"), value) for i in 1..=9. replace("$1", …) also matches the $1 inside $100, $12, etc., so a command body containing a dollar figure like "costs $100" becomes "costs <arg>00". (And arguments that themselves contain $1/$ARGUMENTS get re-substituted since $ARGUMENTS is expanded first.) Consider a single-pass placeholder scan or word-boundary-aware replacement.

Minor

MCP qualified_name (mcp lib.rs:1735) truncates {server}_{tool} to 64 chars after building it; two remote tools sharing a 64-char prefix collapse to the same engine name and silently shadow each other in the registry (HashMap keyed by name). Rare, but a dedup/uniqueness check would be safer.

Nice tests throughout (real rust_analyzer + echo_server.py integration tests, diagnostics 1-based conversion, layering).

— automated review (Claude)

## Review: M5 — MCP, LSP, commands/skills Clean integration work: MCP servers are isolated (one failing server is logged and skipped, the rest connect), the LSP reader auto-acks server→client requests so rust-analyzer's `registerCapability`/progress requests don't stall the handshake, skills load on demand, and project-over-global layering is consistent across agents/commands/skills. A few issues, the LSP one worth fixing. ### LSP `wait_diagnostics` has a `Notify` race that adds full-timeout latency (medium) `client.rs:1160`: ```rust loop { if let Some(diags) = self.diagnostics.lock().unwrap().get(&uri) { return diags.clone(); } let notified = self.diag_notify.notified(); // registered AFTER the map check ... if tokio::time::timeout(remaining, notified).await.is_err() { break; } } ``` `tokio::Notify::notify_waiters()` (used in `dispatch`) only wakes waiters already registered — it stores no permit. If the server's `publishDiagnostics` lands in the window between the map read and `self.diag_notify.notified()`, the notification is missed and this call blocks for the **entire** `wait` duration before falling through and returning the (by-then-present) diagnostics. So it's correct but pays the full timeout on the common fast-publish path, slowing every edit/write that waits on diagnostics. Fix: create the `notified()` future *before* checking the map (the standard tokio pattern), or switch to a `watch`/permit-based signal. ### LSP `path_to_uri` doesn't percent-encode (low-medium) `client.rs:1280` builds `file://` URIs by raw string concat. The server echoes back a percent-encoded `uri` in `publishDiagnostics`, which is then used as the diagnostics map key. For any path containing a space, `#`, or non-ASCII char, our key (`file:///a b.rs`) won't match the server's (`file:///a%20b.rs`), so `wait_diagnostics` silently returns empty for that file. Percent-encode the path (and decode incoming URIs, or normalize both sides). ### `CommandDef::expand` mangles literal `$N` in templates (low) `markdown.rs:483` does `out.replace(&format!("${i}"), value)` for `i in 1..=9`. `replace("$1", …)` also matches the `$1` inside `$100`, `$12`, etc., so a command body containing a dollar figure like `"costs $100"` becomes `"costs <arg>00"`. (And arguments that themselves contain `$1`/`$ARGUMENTS` get re-substituted since `$ARGUMENTS` is expanded first.) Consider a single-pass placeholder scan or word-boundary-aware replacement. ### Minor - MCP `qualified_name` (mcp `lib.rs:1735`) truncates `{server}_{tool}` to 64 chars *after* building it; two remote tools sharing a 64-char prefix collapse to the same engine name and silently shadow each other in the registry (HashMap keyed by name). Rare, but a dedup/uniqueness check would be safer. Nice tests throughout (real rust_analyzer + echo_server.py integration tests, diagnostics 1-based conversion, layering). — automated review (Claude)

You are not authorized to merge this pull request.

This pull request can be merged automatically.

View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u origin feature/m5:feature/m5

git checkout feature/m5

Sign in to join this conversation.

2 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: darman/ai-harness#6