refactor(server): explicit module boundaries for the master server

[16bit-ykiko]

What

Pure refactor of the master server: no behavior change, no LSP semantics change, no error-message change. Function bodies move verbatim (only mechanical this/reference/namespace adjustments); zero snapshot changes across all test suites.

The server core previously concentrated transport wiring, domain logic, and state management inside MasterServer + LSPClient/AgentClient (via friend access), with components reaching up into the transport through raw JsonPeer* pointers. This PR draws explicit boundaries:

• Domain → transport communication is now signal-based. Compile results (diagnostics, inactive regions) are materialized into Session::output; the compiler emits a typed Signal<T> and the transport subscribes with an RAII connection and pushes. Background-index progress works the same way (state + Signal<>), so no component holds a peer pointer anymore (set_peer is gone). Data lives in state, signals only wake subscribers, and a missed signal is harmless.
• Domain → domain hooks are wired in one place. MasterServer::wire() is the composition root's wiring diagram (pool.on_crash, pool.on_evicted, compiler.on_indexing_needed).
• friend access is gone. Transports drive the server through an explicit public surface, matching the project's struct-style state classes.

New module layout

src/server/
├── service/    transport: LSPClient (registration grouped by category), AgentClient, MasterServer (composition root)
├── session/    SessionStore: open-document table + buffer sync (single owner of editor buffer truth)
├── context/    ContextResolver: header compilation contexts (resolution, synthesis, query/switch protocol)
├── compiler/   Compiler: AST/PCH/PCM compilation lifecycle (no longer owns context resolution)
├── index/      IndexQuery (pure reads, incl. agent symbol location) + BackgroundIndexer (queue/schedule/merge)
├── feature/    FeatureRouter: multi-source feature assembly (preamble caches + index + live AST)
├── worker/     worker pool (unchanged)
└── workspace/  project-wide state (unchanged)

• support/signal.h: minimal single-threaded Signal<Args...> with RAII Connection (unit-tested).
• SessionStore replaces the is_open/each_session callback bridges; the index-validity filter moved verbatim to its consumer (IndexQuery::visit_sessions).
• IndexQuery absorbs the agent client's three-strategy symbol locator; the server/project path-pool translations converge on two private helpers.
• FeatureRouter owns the documentLink preamble merge and the five-step definition chain with its ast_dirty gates; transports only translate protocol. New multi-source feature assembly belongs there, never in handlers.
• Dependency directions are enforced: context/, index/, session/, feature/ do not include service/ (transport).

Testing

• Unit: 784 passed (new: Signal and SessionStore suites).
• Integration: 238 passed. Smoke: 3/3.
• Both Debug and RelWithDebInfo configurations build clean; pixi run format is idempotent.

[coderabbitai]

📝 Walkthrough

Walkthrough

This PR splits server responsibilities into dedicated components for signals, sessions, header context resolution, read-side index queries, background indexing, and feature routing, then rewires compiler and service entry points to use them.

Changes

Server subsystem decomposition and rewiring

Layer / File(s)	Summary
Signal and SessionStore src/support/signal.h, src/server/session/session.{h}, src/server/session/session_store.{h,cpp}, tests/unit/server/session_store_tests.cpp, tests/unit/support/signal_tests.cpp	Adds typed signals, session output storage, session buffer lifecycle/update helpers, and unit tests for both new utilities.
ContextResolver src/server/context/context_resolver.{h,cpp}	Adds header compile-context resolution, saved-context restoration, and context query/switch APIs.
Compiler refactor src/server/compiler/compiler.{h,cpp}	Injects ContextResolver, delegates header-context argument filling, and switches diagnostics/inactive-region publication to CompileOutput plus on_output.
IndexQuery src/server/index/query.{h,cpp}	Replaces Indexer read-path APIs with IndexQuery, session iteration, and agentic symbol lookup.
BackgroundIndexer src/server/index/background_indexer.{h,cpp}	Adds background indexing merge, persistence, scheduling, and per-file worker dispatch.
FeatureRouter src/server/feature/feature_router.{h,cpp}	Combines compiler output, cached preamble links, and index results for document links and definition lookups.
Service wiring src/server/service/master_server.{h,cpp}, src/server/service/lsp_client.{h,cpp,cpp}, src/server/service/agent_client.cpp	Rewires server construction, session handling, LSP requests, progress reporting, and agent requests to the new components.

Estimated code review effort: 5 (Critical) | ~120 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Compiler
  participant Session
  participant LSPClient
  participant peer

  Compiler->>Session: set session.output
  Compiler->>LSPClient: on_output.emit(session)
  LSPClient->>peer: publish diagnostics
  LSPClient->>peer: publish inactive regions

Loading

sequenceDiagram
  participant LSPClient
  participant SessionStore
  participant ContextResolver
  participant Compiler

  LSPClient->>SessionStore: apply_open / apply_change
  LSPClient->>ContextResolver: restore_saved_context / switch_context
  Compiler->>ContextResolver: fill_header_context_args(...)
  ContextResolver-->>Compiler: arguments and directory

Loading

Possibly related PRs

• clice-io/clice#382: Shares the same index-system restructuring and query-path changes that this PR builds on.
• clice-io/clice#406: Overlaps on compiler header-context handling and fill_header_context_args refactoring.
• clice-io/clice#480: Touches the same LSP navigation and symbol-resolution handlers updated here.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the PR’s main theme: a server refactor that splits the master server into explicit module boundaries.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch refactor/server-stage1-boundaries

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 8

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/server/index/query.cpp (1)

103-127: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Use the merged-index line map for merged-index lookups.

Line 119 computes the lookup offset from the dirty session text, then queries occurrence ranges stored for merged_index.content(). When the open buffer differs from disk, offsets no longer refer to the same bytes and can resolve the wrong symbol.

Proposed fix

-    // Fallback to MergedIndex. Position -> offset uses the session text when
-    // one exists (open but not yet compiled); for closed files the shard's
-    // own stored content provides the mapping.
+    // Fallback to MergedIndex. Position -> offset must use the shard's stored
+    // content because its occurrence ranges are expressed in that buffer.
@@
-    auto offset = session ? session->line_map().to_offset(position) : map.to_offset(position);
+    auto offset = map.to_offset(position);

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/server/index/query.cpp` around lines 103 - 127, The merged-index fallback
in query handling is mixing session text offsets with occurrences from
merged_index.content(), so use the merged-index line map for both offset
conversion and range translation in the lookup path. Update the lookup logic in
the query function around merged_index.line_starts(), lsp::LineMap, and
merged_index.lookup so that position->offset and offset->range are consistently
based on the merged index data, not session->line_map() when querying
merged_index.

🧹 Nitpick comments (3)

src/server/index/query.h (1)

151-155: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Clarify that with_session skips dirty or unindexed sessions.

Because this delegates to visit_sessions, the callback is also skipped when a Session exists but lacks a current file_index/symbols or is ast_dirty.

Suggested clarification

-    /// The callback is not invoked if no Session exists for that path_id.
+    /// The callback is not invoked if no queryable Session exists for that path_id
+    /// (missing, unindexed, or dirty Sessions are skipped by visit_sessions()).

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/server/index/query.h` around lines 151 - 155, Clarify the `with_session`
contract in `query.h` so it states that the callback is not only skipped when no
`Session` exists, but also when `visit_sessions` filters out a session that is
`ast_dirty` or missing a current `file_index`/`symbols`. Update the comment on
`with_session` to match the actual behavior delegated through `visit_sessions`
and `Session`, so callers know the callback only runs for fully indexed, clean
sessions.

src/server/compiler/compiler.cpp (1)

436-462: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consider unit-testing the newly extracted format_diagnostics/format_inactive_regions.

These are now pure free functions (no Compiler state needed) encapsulating the phantom-suffix-line filtering and "inferred compile command" guidance-diagnostic logic — both of which affect what the user sees on every compile. They're easy to unit-test directly with a synthetic CompileOutput/Session, independent of the worker pipeline.

Also applies to: 488-509

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/server/compiler/compiler.cpp` around lines 436 - 462, Cover the newly
extracted pure helpers by adding direct unit tests for format_diagnostics and
format_inactive_regions, using synthetic CompileOutput and Session inputs
instead of Compiler state. Verify the phantom-suffix line filtering behavior,
the inferred compile-command guidance insertion rules, and the inactive-region
formatting logic so these user-visible paths are exercised independently of the
worker pipeline.

src/server/context/context_resolver.cpp (1)

1-735: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consider adding unit tests for ContextResolver.

This file implements header-context resolution/synthesis, the multi-config CDB command-selection logic, and the queryContext/currentContext/switchContext protocol handlers — all business-critical and non-trivial (chain resolution, preamble/suffix caching, staleness invalidation). The PR's commit summary shows new tests were added only for SessionStore; ContextResolver has no dedicated unit coverage in this stack. Given it is now a standalone, dependency-injectable class (Workspace& only), it should be straightforward to unit-test against a fake/mock Workspace.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/server/context/context_resolver.cpp` around lines 1 - 735, Add dedicated
unit tests for ContextResolver, since this change introduces complex
header-context resolution/synthesis and context switching behavior without
coverage. Create tests around the main entry points fill_header_context_args,
resolve_header_context, query_contexts, current_context, and switch_context
using a fake Workspace, covering synthesized vs self-contained headers,
multi-config host selection, cached context invalidation, and stale epoch
handling. Include assertions for command-hash matching, occurrence handling, and
persistence/reset behavior so the critical protocol and cache paths stay
protected.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/server/index/background_indexer.cpp`:
- Around line 422-442: The final completion count in BackgroundIndexer is stale
when files are skipped because `completed` is incremented but
`progress_data.completed` is only refreshed inside the worker lambda. Update
`progress_data.completed` (and emit `on_progress_changed`) after the skip path
in the `for` loop and again before setting `Progress::Stage::End` so the final
report reflects all skipped items. Keep the change localized around
`background_indexer.cpp`, `progress_data`, `completed`, and the
`workers.spawn`/`workers.join` flow.
- Around line 444-461: The run completion path in background_indexer.cpp leaves
queued work stranded when files are added during an active indexing round,
because schedule() exits early on indexing_active and nothing retriggers it
afterward. Update the indexing flow around the run/queue handling in
background_indexer::run and schedule() so that, after indexing_active is cleared
and the current batch finishes, any remaining items in index_queue automatically
schedule another pass. Use the existing index_queue, index_queue_pos,
pending_ids, and indexing_active state to detect pending work and enqueue the
next run without duplicating a currently running round.

In `@src/server/index/query.cpp`:
- Around line 755-771: The persisted-index branch in query.cpp’s symbol lookup
stops at the first matching Definition, unlike the open-session path that
collects all path+line matches and then filters them. Update the loop around
workspace.project_index.symbols and merged_index.lookup so it accumulates every
symbol whose definition resolves to the requested target_line, then apply the
same parameter/label exclusion logic used by the session path before resolving
uniqueness. Ensure resolve_unique() sees all persisted matches instead of an
early return from the first found symbol.
- Around line 36-40: The IndexQuery::is_proj_path_open helper is treating every
open session as enough to suppress the disk shard, even when the session is
dirty or otherwise not queryable. Update this check to mirror the session
filtering used by visit_sessions so only queryable open sessions return true,
and leave dirty/unindexed sessions as false. Use the existing symbols
IndexQuery::is_proj_path_open, visit_sessions, and sessions to align the logic.

In `@src/server/service/agent_client.cpp`:
- Around line 304-306: The session iteration in agent_client’s index query path
dereferences session.symbols and session.file_index without checking whether the
open session has materialized indexes yet. Update the visit_sessions handling in
the relevant agent_client logic (including the later fallback path) to skip
sessions whose symbols/file_index are not available, or use the persisted shard
instead of accessing null/empty in-memory state. Keep the guard close to the
Session access so try_symbol and related lookups only run on fully indexed
sessions.

In `@src/server/service/lsp_client.cpp`:
- Around line 677-687: The detached handshake in lsp_client.cpp is still using
IndexProgressState::reporter after peer teardown, so it must be tied to the peer
lifetime instead of relying on abandoned alone. Update the
index_progress/create() flow to cancel or join the in-flight handshake before
Connection erases peer, or refactor IndexProgressState and the scheduled lambda
so the reporter has a shared cancellation/lifetime handle bound to JsonPeer. Use
the existing symbols index_progress, IndexProgressState::reporter, and the
scheduled create() coroutine to locate the fix.

In `@src/support/signal.h`:
- Around line 94-102: The Signal::emit path leaves state->emitting stuck on
exceptions, so update the emit() implementation to guarantee the flag is cleared
even if a handler throws. Add a scope guard or equivalent try/finally-style
cleanup around the loop in Signal::emit so state->emitting is reset after
invoking all slot.handler calls, and keep the reentrancy assertion behavior
intact.
- Around line 21-23: `Signal` currently allows `connect()`/`disconnect()` to
mutate `state->slots` during `emit()`, which can invalidate the range iteration
in release builds. Update the `Signal` implementation to make reentrant slot
mutation safe by either snapshotting the current slots before invoking handlers
or deferring add/remove operations until emission completes. Use the
`Signal::emit`, `Signal::connect`, and `Signal::disconnect` paths to locate the
fix and ensure the iterator/storage cannot be modified mid-loop.

---

Outside diff comments:
In `@src/server/index/query.cpp`:
- Around line 103-127: The merged-index fallback in query handling is mixing
session text offsets with occurrences from merged_index.content(), so use the
merged-index line map for both offset conversion and range translation in the
lookup path. Update the lookup logic in the query function around
merged_index.line_starts(), lsp::LineMap, and merged_index.lookup so that
position->offset and offset->range are consistently based on the merged index
data, not session->line_map() when querying merged_index.

---

Nitpick comments:
In `@src/server/compiler/compiler.cpp`:
- Around line 436-462: Cover the newly extracted pure helpers by adding direct
unit tests for format_diagnostics and format_inactive_regions, using synthetic
CompileOutput and Session inputs instead of Compiler state. Verify the
phantom-suffix line filtering behavior, the inferred compile-command guidance
insertion rules, and the inactive-region formatting logic so these user-visible
paths are exercised independently of the worker pipeline.

In `@src/server/context/context_resolver.cpp`:
- Around line 1-735: Add dedicated unit tests for ContextResolver, since this
change introduces complex header-context resolution/synthesis and context
switching behavior without coverage. Create tests around the main entry points
fill_header_context_args, resolve_header_context, query_contexts,
current_context, and switch_context using a fake Workspace, covering synthesized
vs self-contained headers, multi-config host selection, cached context
invalidation, and stale epoch handling. Include assertions for command-hash
matching, occurrence handling, and persistence/reset behavior so the critical
protocol and cache paths stay protected.

In `@src/server/index/query.h`:
- Around line 151-155: Clarify the `with_session` contract in `query.h` so it
states that the callback is not only skipped when no `Session` exists, but also
when `visit_sessions` filters out a session that is `ast_dirty` or missing a
current `file_index`/`symbols`. Update the comment on `with_session` to match
the actual behavior delegated through `visit_sessions` and `Session`, so callers
know the callback only runs for fully indexed, clean sessions.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 54d5052c-f7b0-4a07-bcf9-07fe073fda12

📥 Commits

Reviewing files that changed from the base of the PR and between 87024bb and 68a0c69.

📒 Files selected for processing (21)

• src/server/compiler/compiler.cpp
• src/server/compiler/compiler.h
• src/server/context/context_resolver.cpp
• src/server/context/context_resolver.h
• src/server/feature/feature_router.cpp
• src/server/feature/feature_router.h
• src/server/index/background_indexer.cpp
• src/server/index/background_indexer.h
• src/server/index/query.cpp
• src/server/index/query.h
• src/server/service/agent_client.cpp
• src/server/service/lsp_client.cpp
• src/server/service/lsp_client.h
• src/server/service/master_server.cpp
• src/server/service/master_server.h
• src/server/session/session.h
• src/server/session/session_store.cpp
• src/server/session/session_store.h
• src/support/signal.h
• tests/unit/server/session_store_tests.cpp
• tests/unit/support/signal_tests.cpp