Skip to content

LSP Phase 8: feature gate + Channels struct refactor - #32

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

LSP Phase 8: feature gate + Channels struct refactor#32
yogthos merged 1 commit into
mainfrom
feature/lsp-phase-8-feature-gate

Conversation

@yogthos

@yogthos yogthos commented May 20, 2026

Copy link
Copy Markdown
Collaborator

Phase 8 of 9. Stacked on #31 (Phase 7).

Two related changes in one PR. Splitting them would have required disentangling overlapping diffs in the same files (both touch the LSP-related fields in tool structs / call sites / Channels).

Channels struct refactor

Phase 7's `build_channels` returned a 10-tuple that grew every time we added a slot. Now it returns a named struct:

```rust
#[derive(Default)]
struct Channels {
permission: Option,
ask_tx: Option,
ask_rx: Option,
question_tx: Option,
question_rx: Option,
plan_tx: Option,
plan_rx: Option,
bg_store: Option,
lifecycle_rx: Option,
#[cfg(feature = "lsp")]
lsp_manager: Option<std::sync::Arc>,
}
```

main.rs destructures the same locals it already used. No behavior change, but the call site is no longer a positional 10-tuple unpacking.

Also doc-noted: `compile_lsp_commands` currently ignores the `extensions` field on per-server overrides — the claimed-extensions list lives in the static `builtin_servers()` registry. Follow-up.

Phase 8: `feature = "lsp"` (default-on)

Added `lsp` to the default feature set. With it off (`cargo build --no-default-features --features 'loop git-worktree mcp'`):

  • `lsp` module not compiled; `lsp-types` dep stays optional + skipped.
  • `Channels.lsp_manager` field gated out (also gated out of destructure in main.rs).
  • `build_agent` / `build_agent_inner` / `run_interactive` drop their `lsp_manager` arg via `#[cfg(feature = "lsp")]` on the param.
  • Read/Write/Edit tools drop their `lsp_manager` field + integration call.
  • `LspTool` not registered; `--no-lsp` CLI flag not exposed.
  • `LspConfig` / `LspServerConfig` types gated; `cfg.lsp` field gated.
  • ACP path, slash sub-rebuilds, plan-switch all use `#[cfg(feature = "lsp")]` on the `None` args.

Verified configurations

Command Result
`cargo build --no-default-features --features 'loop git-worktree mcp'` Clean, no lsp deps
`cargo build` (default) Clean, all 4 LSP servers wired
`cargo test --bin dirge --no-default-features --features 'loop git-worktree mcp' -- --skip plugin` 305 passing
`cargo test --bin dirge -- --skip plugin` (default) 426 passing

The +121 tests with the feature on are the LSP module's tests + the diagnostic integration tests, correctly excluded when the feature is off.

Test plan

  • Both feature configurations build clean
  • Both feature configurations test clean
  • `cargo fmt --check` clean

Next: Phase 9 (docs + manual end-to-end test against real rust-analyzer).

@yogthos
yogthos changed the base branch from feature/lsp-phase-7-plumbing to main May 20, 2026 01:46
Two related changes — splitting into separate commits would have
required disentangling overlapping diffs in the same files.

## Refactor: build_channels returns Channels struct

10-tuple → `struct Channels { ... }`. Same fields, just named.
build_channels returns Channels; main.rs destructures the same locals
it already used. No behavior change, but the call site is no longer a
positional 10-tuple unpacking that breaks every time we add a slot.

Doc-noted in main.rs: `compile_lsp_commands` currently ignores the
`extensions` field on per-server overrides. The claimed-extensions
list lives in the static `builtin_servers()` registry; making it
instance-overridable requires plumbing a per-session server set down
through LspManager. Follow-up; users who need new extensions today
must edit `server.rs`.

## Phase 8: feature gate

Adds `feature = "lsp"` to Cargo.toml's default set. With it off:
- `lsp` module is not compiled; `lsp-types` dep stays optional + skipped.
- `Channels.lsp_manager` field gated out (also gated out of destructure
  in main.rs).
- `build_agent` / `build_agent_inner` / `run_interactive` drop their
  `lsp_manager` arg via `#[cfg(feature = "lsp")]` on the param.
- Read/Write/Edit tools drop their `lsp_manager` field + integration
  call.
- LspTool not registered; `--no-lsp` CLI flag not exposed.
- `LspConfig` / `LspServerConfig` types gated; `cfg.lsp` field gated.
- ACP / slash sub-rebuilds / plan-switch all use `#[cfg(feature = "lsp")]`
  None args.

Verified:
- `cargo build --no-default-features --features 'loop git-worktree mcp'`
  → clean (no lsp deps pulled in).
- `cargo build` (default) → clean, all 4 LSP servers wired.
- `cargo test --no-default-features --features 'loop git-worktree mcp'`
  → 305 passing (LSP module's 121 tests correctly excluded).
- `cargo test` (default) → 426 passing.
@yogthos
yogthos force-pushed the feature/lsp-phase-8-feature-gate branch from f80c293 to 0f00599 Compare May 20, 2026 01:56
@yogthos
yogthos merged commit 7f7f0ba into main May 20, 2026
@yogthos
yogthos deleted the feature/lsp-phase-8-feature-gate branch May 20, 2026 01:56
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