refactor(server): split into service layer, add agentic protocol, adopt task_group - #437
Merged
Conversation
…tic protocol Reorganize the flat src/server/ directory into four subdirectories: - workspace/ (workspace, config) - compiler/ (compiler, compile_graph, indexer, protocol) - worker/ (worker_pool, stateful/stateless_worker, worker_common) - lsp/ (master_server, session) Add initial agentic protocol support: - --agentic flag enables a TCP listener (reusing --host/--port) for agent connections alongside the LSP pipe, with validation that it is only allowed in pipe mode - Multi-peer support: agent connections/disconnections are handled independently without affecting the LSP session - First agentic request: agentic/compileCommand returns the compile command (file, directory, arguments) for a given filesystem path Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…s to src/protocol/ Reorganize server architecture: extract LSP handler registration into LSPClient and agentic handlers into AgentClient, keeping MasterServer as pure state. Move all protocol definitions (worker, extension, agentic) into src/protocol/. Add TCP listener for agentic connections in pipe mode and stub --mode agentic for future CLI agent client. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors server architecture by reorganizing header locations into modular subdirectories, decoupling peer initialization from Compiler/Indexer construction, extracting LSP handler logic into a dedicated LSPClient class, introducing AgentClient for agentic protocol support, and adding a new agentic command mode for remote compilation queries via JSON-RPC. Changes
Sequence Diagram(s)sequenceDiagram
participant AgenticCLI as Agentic CLI<br/>(client)
participant TCP as TCP Socket
participant MasterServer as MasterServer<br/>(listener)
participant JsonPeer
participant Compiler
AgenticCLI->>TCP: Connect to host:port
activate TCP
MasterServer->>TCP: Accept connection
TCP->>JsonPeer: Wrap in StreamTransport/JsonPeer
deactivate TCP
AgenticCLI->>JsonPeer: Send CompileCommandParams {path}
JsonPeer->>AgentClient: Route to registered handler
AgentClient->>Compiler: fill_compile_args(path)
Compiler-->>AgentClient: {directory, arguments} or error
AgentClient->>JsonPeer: co_return CompileCommandResult or Error
JsonPeer->>AgenticCLI: Send JSON response
AgenticCLI->>TCP: Close connection
AgenticCLI->>AgenticCLI: Print JSON to stdout, exit(0 or 1)
sequenceDiagram
participant Main as main()
participant EventLoop as kota::event_loop
participant MasterServer as MasterServer
participant Workspace
participant Compiler
participant Indexer
Main->>MasterServer: new MasterServer(loop, argv[0])
activate MasterServer
MasterServer->>Workspace: construct
MasterServer->>Compiler: construct (no peer)
MasterServer->>Indexer: construct (no peer)
deactivate MasterServer
Main->>MasterServer: initialize(workspace_root)
activate MasterServer
MasterServer->>Workspace: load config, create cache dirs
MasterServer->>Compiler: load compile_commands.json
MasterServer->>Workspace: scan dependency graph
MasterServer->>Indexer: build module map, load index
deactivate MasterServer
Main->>+Main: run_server_mode(ServerOptions)
Main->>EventLoop: setup TCP listen or stdio transport
loop Accept connections
EventLoop->>JsonPeer: new JsonPeer(stream)
JsonPeer->>Compiler: set_peer(peer)
JsonPeer->>Indexer: set_peer(peer)
JsonPeer->>LSPClient: new LSPClient(server, peer)
JsonPeer->>AgentClient: new AgentClient(server, peer)
EventLoop->>EventLoop: run async connection handler
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes The diff encompasses significant architectural refactoring with heterogeneous changes across many files: new protocol definitions and server service classes with non-trivial logic (MasterServer initialization, connection handling, peer decoupling), extraction of LSP handlers into a new class, header reorganization across multiple directories, and new agentic mode implementation. While some changes are repetitive (header updates), the core logic density in server initialization, connection dispatch, and LSP/agentic client implementations, combined with the variety of concerns (protocols, services, tests, CLI), demands careful cross-file reasoning. Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
src/server/service/agent_client.cpp (1)
22-27: PassSession*when the path is an open document.
fill_compile_argsaccepts an optionalSession*for header-context fallback on open files (percompiler.hLines 65-70). Calling it without a session means an agent query for a header that's currently open in the editor will skip the header-context fallback and may fail or return suboptimal arguments, while the same query via LSP would succeed. Consider resolving the path to an openSession(viaworkspace.path_poolandserver.sessions) and passing it through.♻️ Sketch
- std::string directory; - std::vector<std::string> arguments; - if(!this->server.compiler.fill_compile_args(params.path, directory, arguments)) { + std::string directory; + std::vector<std::string> arguments; + // Resolve to an open Session so header-context fallback works for + // headers currently open in the editor. + Session* session = nullptr; + auto path_id = this->server.workspace.path_pool.find(params.path); + if(path_id) { + if(auto it = this->server.sessions.find(*path_id); + it != this->server.sessions.end()) { + session = &it->second; + } + } + if(!this->server.compiler.fill_compile_args(params.path, + directory, arguments, + session)) { co_return kota::outcome_error( kota::ipc::Error{std::format("no compile command found for {}", params.path)}); }(Adapt to the actual
path_pool::find/sessionsaccessors — may require friending or a small accessor onMasterServer.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/service/agent_client.cpp` around lines 22 - 27, When calling compiler.fill_compile_args in agent_client.cpp, detect if params.path corresponds to an open document and pass the associated Session* so header-context fallback is used; resolve the path via the workspace.path_pool (e.g., path_pool.find or equivalent) and look up the Session in server.sessions (or expose a small accessor on MasterServer if necessary), then call fill_compile_args(params.path, directory, arguments, sessionPtr) instead of the current call without a Session to ensure open-file header contexts are considered.src/server/compiler/compiler.h (1)
57-59: Document the deferred peer-wiring contract.Switching
peerfrom a reference to a nullable pointer enablesMasterServerto constructCompilerbefore any client connection exists, but it also means any peer-using code (e.g.,publish_diagnostics,clear_diagnostics) silently no-ops untilset_peeris called. A short comment here would help future maintainers avoid surprises (especially in--mode agenticwhere no LSP peer ever attaches).📝 Suggested doc comment
+ /// Wire up the LSP peer used to publish diagnostics. Must be called + /// before any compile path that emits diagnostics; until set, diagnostic + /// notifications are dropped (intentional for agentic-only mode). void set_peer(kota::ipc::JsonPeer* p) { peer = p; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/compiler/compiler.h` around lines 57 - 59, Add a brief doc comment for the deferred peer-wiring contract on the Compiler class: explain that the former reference was changed to a nullable pointer named peer and that any methods using the peer (e.g., publish_diagnostics, clear_diagnostics) will silently no-op until set_peer(kota::ipc::JsonPeer* p) is called; note that MasterServer may construct Compiler before any client attaches and in --mode agentic no LSP peer may ever be provided, so callers should not assume peer is non-null.src/server/service/agent_client.h (1)
9-16: Minor: document lifetime contract betweenAgentClientandpeer.The constructor (in
agent_client.cpp) registers a handler that capturesthis. SoAgentClientmust outlive the registered handler onpeer, i.e.,peermust be destroyed (or its handlers cleared) beforeAgentClient. A short comment on the class would make this contract explicit for future maintainers wiring up connection lifecycles.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/service/agent_client.h` around lines 9 - 16, Add a brief lifetime contract comment to AgentClient explaining that the constructor registers a handler on kota::ipc::JsonPeer which captures this, so AgentClient must outlive the registered handler (i.e., peer handlers should be cleared or peer destroyed before destroying AgentClient); place this comment above the class declaration in agent_client.h and reference AgentClient, its constructor, and kota::ipc::JsonPeer to make the ownership/teardown expectation explicit for maintainers.src/server/service/lsp_client.cpp (1)
189-189: Redundantindexer.set_peercall.
server.indexer.set_peer(&peer)is already invoked in theLSPClientconstructor (line 41), so calling it again here oninitializedis dead code. Recommend removing for clarity — the constructor is the single, well-defined wiring point.♻️ Proposed cleanup
- srv.indexer.set_peer(&this->peer); srv.indexer.set_max_concurrency(cfg.stateless_worker_count.value);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/service/lsp_client.cpp` at line 189, Remove the redundant call to srv.indexer.set_peer(&this->peer) from the initialized method: the LSPClient constructor already wires the peer via indexer.set_peer(&peer), so delete the duplicate srv.indexer.set_peer(&this->peer) line in the initialized function to avoid dead code and keep wiring confined to the constructor.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/protocol/extension.h`:
- Around line 20-40: QueryContextResult::total and SwitchContextResult::success
are left uninitialized which can produce indeterminate values;
default-initialize these trivial scalar fields (set total = 0 and success =
false) in their struct definitions so any partially constructed or
early-returned instances are safe by default, updating the declarations for
QueryContextResult::total and SwitchContextResult::success accordingly.
In `@src/server/service/agent_client.cpp`:
- Around line 19-35: Validate and authorize incoming agent requests before
calling server.compiler.fill_compile_args: ensure CompileCommandParams::path is
non-empty, absolute, and canonicalize it then check it lies inside the server
workspace root (use this->server.workspace_root or equivalent) before
proceeding; if out-of-bounds return a generic kota::ipc::Error without echoing
the raw path. Additionally, gate this endpoint behind connection restrictions
and authentication: make the peer listener bind to loopback by default and
require a token/credential check on the peer (or validate an initial auth
header) before executing the peer.on_request handler. Finally, remove verbatim
path interpolation in the error message returned from the lambda (currently
creating kota::ipc::Error via std::format) and replace with a sanitized or
generic message.
In `@src/server/service/lsp_client.cpp`:
- Around line 39-41: LSPClient stores &peer into server.compiler and
server.indexer but never clears them, leaving dangling
Compiler::peer/Indexer::peer when an LSPClient is destroyed; add a LSPClient
destructor (~LSPClient) in lsp_client.h/cpp that resets those pointers by
calling server.compiler.set_peer(nullptr) and server.indexer.set_peer(nullptr)
only if they still reference this LSPClient's peer (i.e., guard so you don't
clear a peer that another LSPClient has since set), to prevent subsequent
publish_diagnostics/clear_diagnostics dereferencing freed memory.
In `@src/server/service/master_server.cpp`:
- Around line 220-225: The code currently ignores failure from kota::tcp::listen
when creating acceptor, so add a visible warning log when listen returns
empty/false; specifically, after calling auto acceptor =
kota::tcp::listen(opts.host, opts.port, {}, loop); check the falsy case and emit
a warning via LOG_WARN or LOG_ERROR indicating the agentic listener failed to
bind on opts.host:opts.port and include errno/strerror or the underlying error
message, otherwise proceed with the existing block that logs success and
schedules accept_connections(loop, server, std::move(*acceptor), false,
connections).
- Around line 175-194: accept_connections currently constructs an LSPClient for
every TCP connection, and LSPClient's constructor writes raw &peer into
Compiler::peer and Indexer::peer causing latest-wins overwrites and
use-after-free when the Connection is erased; fix by preventing multiple
concurrent LSP clients: gate LSPClient construction in accept_connections (or
inside LSPClient factory) on server.lifecycle == ServerLifecycle::Uninitialized
or on a single-client flag and reject additional connections instead of creating
LSPClient, and implement LSPClient::~LSPClient to clear Compiler::peer and
Indexer::peer on teardown; additionally, in the agent listener path (pipe mode)
log a warning (LOG_WARN) if listen fails so a busy port is visible.
---
Nitpick comments:
In `@src/server/compiler/compiler.h`:
- Around line 57-59: Add a brief doc comment for the deferred peer-wiring
contract on the Compiler class: explain that the former reference was changed to
a nullable pointer named peer and that any methods using the peer (e.g.,
publish_diagnostics, clear_diagnostics) will silently no-op until
set_peer(kota::ipc::JsonPeer* p) is called; note that MasterServer may construct
Compiler before any client attaches and in --mode agentic no LSP peer may ever
be provided, so callers should not assume peer is non-null.
In `@src/server/service/agent_client.cpp`:
- Around line 22-27: When calling compiler.fill_compile_args in
agent_client.cpp, detect if params.path corresponds to an open document and pass
the associated Session* so header-context fallback is used; resolve the path via
the workspace.path_pool (e.g., path_pool.find or equivalent) and look up the
Session in server.sessions (or expose a small accessor on MasterServer if
necessary), then call fill_compile_args(params.path, directory, arguments,
sessionPtr) instead of the current call without a Session to ensure open-file
header contexts are considered.
In `@src/server/service/agent_client.h`:
- Around line 9-16: Add a brief lifetime contract comment to AgentClient
explaining that the constructor registers a handler on kota::ipc::JsonPeer which
captures this, so AgentClient must outlive the registered handler (i.e., peer
handlers should be cleared or peer destroyed before destroying AgentClient);
place this comment above the class declaration in agent_client.h and reference
AgentClient, its constructor, and kota::ipc::JsonPeer to make the
ownership/teardown expectation explicit for maintainers.
In `@src/server/service/lsp_client.cpp`:
- Line 189: Remove the redundant call to srv.indexer.set_peer(&this->peer) from
the initialized method: the LSPClient constructor already wires the peer via
indexer.set_peer(&peer), so delete the duplicate
srv.indexer.set_peer(&this->peer) line in the initialized function to avoid dead
code and keep wiring confined to the constructor.
🪄 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: 76b3ad22-158f-4397-8d17-1a247b82da9f
📒 Files selected for processing (37)
src/clice.ccsrc/protocol/agentic.hsrc/protocol/extension.hsrc/protocol/worker.hsrc/server/compiler/compile_graph.cppsrc/server/compiler/compile_graph.hsrc/server/compiler/compiler.cppsrc/server/compiler/compiler.hsrc/server/compiler/indexer.cppsrc/server/compiler/indexer.hsrc/server/master_server.hsrc/server/service/agent_client.cppsrc/server/service/agent_client.hsrc/server/service/lsp_client.cppsrc/server/service/lsp_client.hsrc/server/service/master_server.cppsrc/server/service/master_server.hsrc/server/service/session.hsrc/server/worker/stateful_worker.cppsrc/server/worker/stateful_worker.hsrc/server/worker/stateless_worker.cppsrc/server/worker/stateless_worker.hsrc/server/worker/worker_common.hsrc/server/worker/worker_pool.cppsrc/server/worker/worker_pool.hsrc/server/workspace/config.cppsrc/server/workspace/config.hsrc/server/workspace/workspace.cppsrc/server/workspace/workspace.htests/unit/server/compile_graph_integration_tests.cpptests/unit/server/compile_graph_tests.cpptests/unit/server/config_tests.cpptests/unit/server/module_worker_tests.cpptests/unit/server/pch_worker_tests.cpptests/unit/server/stateful_worker_tests.cpptests/unit/server/stateless_worker_tests.cpptests/unit/server/worker_test_helpers.h
💤 Files with no reviewable changes (2)
- src/protocol/worker.h
- src/server/master_server.h
Add AgenticClient utility for JSON-RPC over TCP and three integration tests for agentic/compileCommand: known file, unknown file fallback, and multiple sequential requests. Update conftest to pass --port with a dynamic free port so the agentic listener starts in pipe mode. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/conftest.py (1)
168-173:⚠️ Potential issue | 🟠 Major
make_clientnot updated with--host/--port; will break multi-session tests.The agentic TCP listener is unconditionally started in pipe mode (lines 203-228 of master_server.cpp) and defaults to
127.0.0.1:50051when--host/--portare not provided. Theclientfixture was updated to pass dynamic--host/--portarguments, butmake_clientstill only passes--mode pipe. When multi-session tests (e.g.,test_persistent_cache.py) callmake_clientmultiple times sequentially, the second call will fail to bind because the first server is already holding port 50051.🛡️ Proposed fix
-async def make_client(executable: Path, workspace: Path) -> CliceClient: +async def make_client(executable: Path, workspace: Path) -> CliceClient: """Spawn a fresh clice server and initialize it. For multi-session tests.""" c = CliceClient() - await c.start_io(str(executable), "--mode", "pipe") + port = _find_free_port() + await c.start_io( + str(executable), "--mode", "pipe", "--host", "127.0.0.1", "--port", str(port) + ) await c.initialize(workspace) return c🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/conftest.py` around lines 168 - 173, The make_client helper currently always starts the server in pipe mode and omits dynamic host/port, causing port collisions in multi-session tests; update the async function make_client(executable: Path, workspace: Path) -> CliceClient to accept host and port parameters (e.g., host: str, port: int) and forward them into the start_io invocation (call CliceClient.start_io with the existing "--mode", "pipe" plus "--host", host, "--port", str(port)); ensure any callers (tests/fixtures that previously relied on make_client) pass the dynamic host/port values so each spawned server binds to the intended address instead of the default 127.0.0.1:50051.
🧹 Nitpick comments (6)
tests/integration/utils/agentic_client.py (3)
51-63: Consider raising on missingContent-Lengthinstead of silently returningNone.When the header lacks
Content-Length,_read_messagereturnsNone, which then tripsrequest()'s "connection closed before response" assertion — a misleading message for what is actually a protocol violation. Distinguishing the two cases would make protocol regressions easier to debug.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/utils/agentic_client.py` around lines 51 - 63, The _read_message function currently returns None when the header lacks a Content-Length, causing downstream code (like request()) to misinterpret it as a closed connection; change _read_message to raise a clear exception (e.g., ValueError or a custom ProtocolError) when the Content-Length header is missing so callers can distinguish a protocol violation from a closed connection; update the exception message to include the raw header and reference the _read_message function so logs/tests can pinpoint the malformed header case, and ensure request() or its callers either let the exception propagate or handle it to produce a distinct error for protocol violations.
28-49:request()swallows JSON-RPC error responses, leading to confusing test failures.The current implementation only asserts
idcorrespondence and returns the raw response. If the server returns a JSON-RPCerror(e.g. thekota::outcome_errorpath inagent_client.cppfor unknown files), callers blindly doresp["result"]and raise an opaqueKeyErrorinstead of a clear failure. Consider either raising onerrorhere, or at minimum returning the parsed object after asserting one ofresult/erroris present so callers can branch.response = await asyncio.wait_for(self._read_message(), timeout=timeout) assert response is not None, "connection closed before response" assert response.get("id") == msg_id + assert "result" in response or "error" in response, f"malformed response: {response}" return response🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/utils/agentic_client.py` around lines 28 - 49, The request() method currently returns raw JSON-RPC responses which hides server-side errors; update request() (which calls _read_message()) to check the parsed response for an "error" field after verifying the id and, if present, raise a clear exception (e.g., a RuntimeError or a small JSONRPCError) that includes the error object/message and the original response id so tests fail with a meaningful message; alternatively, if you prefer not to raise, assert that either "result" or "error" is present and return a normalized dict containing one of those keys to force callers to handle errors explicitly.
16-26: Replaceasyncio.get_event_loop()withasyncio.get_running_loop()inside the async coroutine.
asyncio.get_event_loop()is deprecated inside coroutines in Python 3.12+ and emits a DeprecationWarning. The official asyncio documentation explicitly recommendsasyncio.get_running_loop()as the replacement for coroutines and callbacks, as it guarantees the loop is running and raises RuntimeError otherwise.♻️ Proposed change
`@classmethod` async def connect(cls, host: str, port: int, *, timeout: float = 10.0): - deadline = asyncio.get_event_loop().time() + timeout + loop = asyncio.get_running_loop() + deadline = loop.time() + timeout while True: try: reader, writer = await asyncio.open_connection(host, port) return cls(reader, writer) except (ConnectionRefusedError, OSError): - if asyncio.get_event_loop().time() >= deadline: + if loop.time() >= deadline: raise await asyncio.sleep(0.1)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/utils/agentic_client.py` around lines 16 - 26, The connect classmethod uses asyncio.get_event_loop() which is deprecated inside coroutines; replace both calls to asyncio.get_event_loop().time() with asyncio.get_running_loop().time() inside AgenticClient.connect (the classmethod named connect) so the deadline calculation and the loop time check use the running loop; keep the retry/sleep logic unchanged (await asyncio.sleep(0.1)) and ensure you import nothing new.tests/integration/agentic/test_agentic.py (1)
6-13: Strengthen theargumentsassertion.
assert len(result["arguments"]) > 0will pass even if the server returns a junk single-element list. Since the workspace'scompile_commands.jsonis generated withclang++ -std=c++17 -fsyntax-only main.cpp(per_generate_test_data_cdbsinconftest.py), you can pin down a few invariants cheaply:assert result["file"] == main_cpp assert result["directory"] == str(workspace) assert len(result["arguments"]) > 0 + assert any("-std=c++17" in a for a in result["arguments"]) + assert main_cpp in result["arguments"] or any(main_cpp in a for a in result["arguments"])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/agentic/test_agentic.py` around lines 6 - 13, The current assertion on result["arguments"] is too weak; update test_compile_command to assert concrete invariants from the generated compile_commands.json: verify that the command name (e.g., "clang++") appears in result["arguments"] (check result["arguments"][0] or that any arg contains "clang++"), that "-std=c++17" and "-fsyntax-only" are present in result["arguments"], and that the source file argument equals or ends with "main.cpp" (compare to the main_cpp variable) so the arguments must contain those expected tokens rather than just having a non-empty list.tests/conftest.py (2)
96-104: Free-port lookup has an unavoidable TOCTOU window.
_find_free_portbinds-then-closes-then-returns the port, leaving a race where another process (or another parallel pytest worker, e.g. underpytest-xdist) can claim it before the server binds. For local CI this is usually fine, but if you start seeing flakyAddress already in usefailures, switch toSO_REUSEADDRand have the server inherit the listening socket, or pass a port range and retry. Mentioning here so it's on the radar — no action required if tests run serially.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/conftest.py` around lines 96 - 104, The free-port helper _find_free_port (used by agentic_port fixture) opens, binds and closes a socket which creates a TOCTOU race where another process can claim the port before the test server binds; fix by changing the approach to avoid closing the listening socket early—either create and set SO_REUSEADDR and pass the open listening socket into the server (have the server inherit the socket created by _find_free_port) or replace the helper with a port-range+retry strategy that attempts bind, catches Address already in use, and retries until success; update _find_free_port (and any code that consumes agentic_port) to use the chosen approach so the socket remains reserved until the server binds.
138-143: Unusedclientparameter — make the dependency explicit.The
clientfixture is requested only to ensure the server is up before the agentic connect; it isn't otherwise referenced. That's fine, but a brief comment will save future readers from "remove unused parameter" refactors that would silently break ordering.`@pytest.fixture` async def agentic(agentic_port: int, client) -> AgenticClient: - """Connect to the agentic TCP endpoint of a running server.""" + """Connect to the agentic TCP endpoint of a running server. + + The ``client`` fixture is required (even though unused here) to ensure the + server process is started and listening before we attempt to connect. + """ ac = await AgenticClient.connect("127.0.0.1", agentic_port) yield ac await ac.close()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/conftest.py` around lines 138 - 143, The agentic fixture currently takes an unused parameter client only to ensure server startup order; update the Agentic fixture (agentic) to document this by adding a brief comment or note in its docstring that the client fixture is intentionally required to guarantee the server is running before AgenticClient.connect is called, so future editors know not to remove the dependency; reference the fixture names agentic and client in the comment to make the intent explicit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/integration/agentic/test_agentic.py`:
- Around line 16-22: The test expects a fallback for unknown paths but the
server returns an error when fill_compile_args (compiler.cpp) fails
(resolve_header_context returns nullopt) and the agent handler in
agent_client.cpp returns an outcome error; update test_compile_command_fallback
to assert an error response instead of accessing resp["result"] — e.g., call
agentic.request("agentic/compileCommand", {"path": "/nonexistent/file.cpp"}),
assert that "error" is present in resp (or that resp.get("result") is None) and
check the error code/message matches the agent handler's error, and remove the
assertion that result["file"] == "/nonexistent/file.cpp".
---
Outside diff comments:
In `@tests/conftest.py`:
- Around line 168-173: The make_client helper currently always starts the server
in pipe mode and omits dynamic host/port, causing port collisions in
multi-session tests; update the async function make_client(executable: Path,
workspace: Path) -> CliceClient to accept host and port parameters (e.g., host:
str, port: int) and forward them into the start_io invocation (call
CliceClient.start_io with the existing "--mode", "pipe" plus "--host", host,
"--port", str(port)); ensure any callers (tests/fixtures that previously relied
on make_client) pass the dynamic host/port values so each spawned server binds
to the intended address instead of the default 127.0.0.1:50051.
---
Nitpick comments:
In `@tests/conftest.py`:
- Around line 96-104: The free-port helper _find_free_port (used by agentic_port
fixture) opens, binds and closes a socket which creates a TOCTOU race where
another process can claim the port before the test server binds; fix by changing
the approach to avoid closing the listening socket early—either create and set
SO_REUSEADDR and pass the open listening socket into the server (have the server
inherit the socket created by _find_free_port) or replace the helper with a
port-range+retry strategy that attempts bind, catches Address already in use,
and retries until success; update _find_free_port (and any code that consumes
agentic_port) to use the chosen approach so the socket remains reserved until
the server binds.
- Around line 138-143: The agentic fixture currently takes an unused parameter
client only to ensure server startup order; update the Agentic fixture (agentic)
to document this by adding a brief comment or note in its docstring that the
client fixture is intentionally required to guarantee the server is running
before AgenticClient.connect is called, so future editors know not to remove the
dependency; reference the fixture names agentic and client in the comment to
make the intent explicit.
In `@tests/integration/agentic/test_agentic.py`:
- Around line 6-13: The current assertion on result["arguments"] is too weak;
update test_compile_command to assert concrete invariants from the generated
compile_commands.json: verify that the command name (e.g., "clang++") appears in
result["arguments"] (check result["arguments"][0] or that any arg contains
"clang++"), that "-std=c++17" and "-fsyntax-only" are present in
result["arguments"], and that the source file argument equals or ends with
"main.cpp" (compare to the main_cpp variable) so the arguments must contain
those expected tokens rather than just having a non-empty list.
In `@tests/integration/utils/agentic_client.py`:
- Around line 51-63: The _read_message function currently returns None when the
header lacks a Content-Length, causing downstream code (like request()) to
misinterpret it as a closed connection; change _read_message to raise a clear
exception (e.g., ValueError or a custom ProtocolError) when the Content-Length
header is missing so callers can distinguish a protocol violation from a closed
connection; update the exception message to include the raw header and reference
the _read_message function so logs/tests can pinpoint the malformed header case,
and ensure request() or its callers either let the exception propagate or handle
it to produce a distinct error for protocol violations.
- Around line 28-49: The request() method currently returns raw JSON-RPC
responses which hides server-side errors; update request() (which calls
_read_message()) to check the parsed response for an "error" field after
verifying the id and, if present, raise a clear exception (e.g., a RuntimeError
or a small JSONRPCError) that includes the error object/message and the original
response id so tests fail with a meaningful message; alternatively, if you
prefer not to raise, assert that either "result" or "error" is present and
return a normalized dict containing one of those keys to force callers to handle
errors explicitly.
- Around line 16-26: The connect classmethod uses asyncio.get_event_loop() which
is deprecated inside coroutines; replace both calls to
asyncio.get_event_loop().time() with asyncio.get_running_loop().time() inside
AgenticClient.connect (the classmethod named connect) so the deadline
calculation and the loop time check use the running loop; keep the retry/sleep
logic unchanged (await asyncio.sleep(0.1)) and ensure you import nothing new.
🪄 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: 401686bb-be6a-441e-8c09-21f060a2cb59
📒 Files selected for processing (4)
tests/conftest.pytests/integration/agentic/__init__.pytests/integration/agentic/test_agentic.pytests/integration/utils/agentic_client.py
…free Temporary lambdas used as coroutines have their captures accessed through a dangling this pointer after the lambda is destroyed. Pass values as function parameters instead (copied into the coroutine frame by the standard). Also make the agentic TCP listener opt-in (--port > 0) to avoid acceptor leaks in smoke tests, add LSPClient destructor to reset peer pointers, and default-initialize extension protocol scalars. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/server/service/lsp_client.cpp (1)
414-427:⚠️ Potential issue | 🟡 MinorEmpty PCH-link array produces malformed JSON.
The merge path assumes
pch_jsonhas at least one element. Ifpch_jsonis"[]"(or, more generally, any array of length 2), thenpop_back()+,+append(pch_json.begin() + 1, pch_json.end())produces"[..,]"— invalid JSON that the client will reject. The outer guard only checks!document_links_json.empty(), notsize() > 2.🛡️ Proposed fix
- if(!links.data.empty() && links.data != "null" && links.data.size() > 2) { + if(pch_json.size() <= 2) { + // PCH contributes no links; keep the original result as-is. + } else if(!links.data.empty() && links.data != "null" && links.data.size() > 2) { links.data.pop_back(); links.data += ','; links.data.append(pch_json.begin() + 1, pch_json.end()); } else { links.data = pch_json; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/service/lsp_client.cpp` around lines 414 - 427, The merge logic for PCH document links can produce malformed JSON when pch_json is an empty array ("[]") because the code assumes pch_json has more than just the brackets; update the condition around the merge to ensure pch_json.size() > 2 (and keep the existing checks for links.data size) before doing pop_back()/add comma/append of pch_json.begin() + 1; otherwise assign links.data = pch_json or skip the comma/append path. Refer to sit2, sit2->second.pch_ref, pch_it (from srv.workspace.pch_cache), pch_json, and links.data when locating and fixing the code.
♻️ Duplicate comments (1)
src/server/service/master_server.cpp (1)
175-178:⚠️ Potential issue | 🟠 MajorMulti-LSP-client hazard in socket mode remains unmitigated despite the destructor fix.
The added
LSPClient::~LSPClient(lsp_client.cpp:778-781) closes the dangling-pointer hole on disconnect, but insocketmoderegister_lsp=truestill means every accepted TCP connection constructs a freshLSPClientthat overwritesserver.compiler.peer/server.indexer.peer(lsp_client.cpp:40-41). As long as ≥ 2 LSP clients are connected concurrently, the second connection's constructor silently redirects diagnostics/progress for client A to client B's transport, and the secondinitializerequest is only rejected after the peer pointers have already been clobbered.Recommend gating the
LSPClientconstruction so socket mode admits at most one LSP client, refusing extra LSP attempts at the connection layer rather than at the protocol layer:🛡️ Suggested guard
std::unique_ptr<LSPClient> lsp; - if(register_lsp) - lsp = std::make_unique<LSPClient>(server, *peer); + if(register_lsp) { + if(server.lifecycle == ServerLifecycle::Uninitialized) { + lsp = std::make_unique<LSPClient>(server, *peer); + } else { + LOG_WARN("Refusing additional LSP client; server already initialized"); + } + } auto agent = std::make_unique<AgentClient>(server, *peer);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/service/master_server.cpp` around lines 175 - 178, Current code in master_server.cpp always constructs a new LSPClient when register_lsp is true, which lets a second socket-mode client overwrite server.compiler.peer / server.indexer.peer; before creating the LSPClient (the std::make_unique<LSPClient> call) add a gate that checks whether an existing LSP is already bound (e.g., test server.compiler.peer and/or server.indexer.peer or an explicit server.has_lsp flag) and if one exists refuse the new connection at the accept/connection layer (close the socket or send a connection-refused response) and skip constructing LSPClient, otherwise proceed to construct and set the peer; ensure this check is done prior to any code that might mutate server.compiler.peer/server.indexer.peer so the second client cannot clobber the first.
🧹 Nitpick comments (3)
tests/integration/utils/agentic_client.py (1)
16-26: Useasyncio.get_running_loop()instead ofasyncio.get_event_loop()in this coroutine.
asyncio.get_event_loop()emits aDeprecationWarningin Python 3.12+ and is not recommended inside async functions. Theconnectmethod is always awaited, soasyncio.get_running_loop()is a direct replacement with clearer semantics.♻️ Proposed refactor
`@classmethod` async def connect(cls, host: str, port: int, *, timeout: float = 10.0): - deadline = asyncio.get_event_loop().time() + timeout + loop = asyncio.get_running_loop() + deadline = loop.time() + timeout while True: try: reader, writer = await asyncio.open_connection(host, port) return cls(reader, writer) except (ConnectionRefusedError, OSError): - if asyncio.get_event_loop().time() >= deadline: + if loop.time() >= deadline: raise await asyncio.sleep(0.1)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/utils/agentic_client.py` around lines 16 - 26, The connect classmethod uses asyncio.get_event_loop() inside an async coroutine which triggers DeprecationWarnings; replace both asyncio.get_event_loop() calls with asyncio.get_running_loop() in the connect method so the deadline calculation and time checks use the running event loop (i.e., update the deadline = asyncio.get_event_loop().time() and the subsequent asyncio.get_event_loop().time() comparisons inside the connect coroutine to use asyncio.get_running_loop().time()).src/server/service/lsp_client.cpp (1)
188-192: Redundantset_peeron the indexer.
server.indexer.set_peer(&peer)was already called in the constructor at line 41. Calling it again here in theinitializednotification handler is redundant — the peer has not changed. Also note the inconsistency: the constructor wires bothcompilerandindexer, but theinitializedhandler only re-wiresindexer.If this second call is intentional (e.g., to re-arm progress reporting after some teardown), please add a comment; otherwise, drop it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/service/lsp_client.cpp` around lines 188 - 192, The call to srv.indexer.set_peer(&this->peer) inside the initialized notification handler is redundant because set_peer(&peer) is already done in the constructor (where both compiler and indexer are wired); remove the duplicate srv.indexer.set_peer(&this->peer) from the initialized handler unless there is an intentional reason to re-arm progress reporting—if intentional, replace the call with a brief inline comment explaining why the indexer needs re-wiring here and also ensure symmetry with the compiler wiring (i.e., document why only indexer is re-wired while compiler is not) so readers understand the difference; leave srv.load_workspace() as-is.src/server/service/master_server.cpp (1)
64-77: Silent fallthrough when a configured path is a directory withoutcompile_commands.json.In the directory branch, if
candidatedoes not exist, we fall through to the next configured entry without a warning — only the "path doesn't exist at all" branch logs. Users who misconfigure a directory entry get no diagnostic and end up with the auto-discovery message at line 102 (or none if discovery succeeds elsewhere), making misconfiguration hard to spot.♻️ Proposed refinement
if(llvm::sys::fs::is_directory(configured)) { auto candidate = path::join(configured, "compile_commands.json"); if(llvm::sys::fs::exists(candidate)) { cdb_path = std::move(candidate); break; } + LOG_WARN("No compile_commands.json under configured directory: {}", configured); } else if(llvm::sys::fs::exists(configured)) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/service/master_server.cpp` around lines 64 - 77, The loop over cfg.compile_commands_paths silently ignores directory entries that lack a compile_commands.json, making misconfiguration hard to detect; update the branch inside the for loop (the llvm::sys::fs::is_directory(configured) case) to check if candidate exists and if not call LOG_WARN (similar to the else branch) with a clear message mentioning the configured directory and that compile_commands.json was not found, so users see a diagnostic when candidate does not exist before falling through to auto-discovery; ensure you reference candidate and configured in the log and leave the existing cdb_path assignment and breaks unchanged when a match is found.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/server/service/lsp_client.cpp`:
- Around line 414-427: The merge logic for PCH document links can produce
malformed JSON when pch_json is an empty array ("[]") because the code assumes
pch_json has more than just the brackets; update the condition around the merge
to ensure pch_json.size() > 2 (and keep the existing checks for links.data size)
before doing pop_back()/add comma/append of pch_json.begin() + 1; otherwise
assign links.data = pch_json or skip the comma/append path. Refer to sit2,
sit2->second.pch_ref, pch_it (from srv.workspace.pch_cache), pch_json, and
links.data when locating and fixing the code.
---
Duplicate comments:
In `@src/server/service/master_server.cpp`:
- Around line 175-178: Current code in master_server.cpp always constructs a new
LSPClient when register_lsp is true, which lets a second socket-mode client
overwrite server.compiler.peer / server.indexer.peer; before creating the
LSPClient (the std::make_unique<LSPClient> call) add a gate that checks whether
an existing LSP is already bound (e.g., test server.compiler.peer and/or
server.indexer.peer or an explicit server.has_lsp flag) and if one exists refuse
the new connection at the accept/connection layer (close the socket or send a
connection-refused response) and skip constructing LSPClient, otherwise proceed
to construct and set the peer; ensure this check is done prior to any code that
might mutate server.compiler.peer/server.indexer.peer so the second client
cannot clobber the first.
---
Nitpick comments:
In `@src/server/service/lsp_client.cpp`:
- Around line 188-192: The call to srv.indexer.set_peer(&this->peer) inside the
initialized notification handler is redundant because set_peer(&peer) is already
done in the constructor (where both compiler and indexer are wired); remove the
duplicate srv.indexer.set_peer(&this->peer) from the initialized handler unless
there is an intentional reason to re-arm progress reporting—if intentional,
replace the call with a brief inline comment explaining why the indexer needs
re-wiring here and also ensure symmetry with the compiler wiring (i.e., document
why only indexer is re-wired while compiler is not) so readers understand the
difference; leave srv.load_workspace() as-is.
In `@src/server/service/master_server.cpp`:
- Around line 64-77: The loop over cfg.compile_commands_paths silently ignores
directory entries that lack a compile_commands.json, making misconfiguration
hard to detect; update the branch inside the for loop (the
llvm::sys::fs::is_directory(configured) case) to check if candidate exists and
if not call LOG_WARN (similar to the else branch) with a clear message
mentioning the configured directory and that compile_commands.json was not
found, so users see a diagnostic when candidate does not exist before falling
through to auto-discovery; ensure you reference candidate and configured in the
log and leave the existing cdb_path assignment and breaks unchanged when a match
is found.
In `@tests/integration/utils/agentic_client.py`:
- Around line 16-26: The connect classmethod uses asyncio.get_event_loop()
inside an async coroutine which triggers DeprecationWarnings; replace both
asyncio.get_event_loop() calls with asyncio.get_running_loop() in the connect
method so the deadline calculation and time checks use the running event loop
(i.e., update the deadline = asyncio.get_event_loop().time() and the subsequent
asyncio.get_event_loop().time() comparisons inside the connect coroutine to use
asyncio.get_running_loop().time()).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 52013694-f6d0-4c35-8fb0-91a71786881c
📒 Files selected for processing (8)
src/clice.ccsrc/protocol/extension.hsrc/server/service/lsp_client.cppsrc/server/service/lsp_client.hsrc/server/service/master_server.cppsrc/server/service/master_server.htests/conftest.pytests/integration/utils/agentic_client.py
✅ Files skipped from review due to trivial changes (1)
- src/protocol/extension.h
🚧 Files skipped from review as they are similar to previous changes (3)
- src/server/service/master_server.h
- tests/conftest.py
- src/clice.cc
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…odule - Rewrite `--mode agentic` as a one-shot TCP client that connects to a running server's agentic port, sends a compileCommand request, prints the JSON result to stdout, and exits. - Extract agentic client code into src/server/service/agentic.cpp/h. - Replace inline coroutine lambdas with named functions and when_all. - Rewrite integration tests to use subprocess.run instead of a custom JSON-RPC client; delete agentic_client.py. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/conftest.py (1)
125-128:_find_free_porthas a TOCTOU race; consider passing port=0 to the server instead.Binding to port 0 then closing and handing the number to a freshly spawned
cliceis racy — under CI load another process can claim the freed port beforeclicecallslisten. The cleanest fix is to letclicebind port 0 itself and report back the chosen port, but that requires server-side support. As a pragmatic interim, setSO_REUSEADDRor accept that occasional flakes are possible.def _find_free_port() -> int: with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as s: s.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1) s.bind(("127.0.0.1", 0)) return s.getsockname()[1]Not a blocker — this is a common pytest pattern — but worth noting if the agentic suite ever flakes in CI.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/conftest.py` around lines 125 - 128, _find_free_port suffers a TOCTOU race because it binds then closes the socket before the test server (clice) binds; either change test harness so the server binds to port 0 and reports the chosen port back, or as a pragmatic interim set SO_REUSEADDR on the temporary socket to reduce races: update the _find_free_port function to call s.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1) before s.bind, or preferably modify clice/server startup to accept port=0 and return the bound port so callers do not rely on a transient free-port probe.src/clice.cc (1)
28-31:--porthelp text and validation are inconsistent across modes.
--portis consumed by socket mode (line 249 ofsrc/server/service/master_server.cpp) as the LSP listen port and by agentic mode as the connect target, but the help string only describes the "Agentic TCP port (0 = disabled)" case. Additionally, socket mode does not validateport > 0the way agentic mode does at line 146 — passing--mode socketwith no--portfalls through tokota::tcp::listen(host, 0, ...), which (if it succeeds) binds an ephemeral port the user has no way to discover.🛠️ Suggested adjustments
DecoKV(style = KVStyle::JoinedOrSeparate, - help = "Agentic TCP port (0 = disabled)", + help = "TCP port: required for socket/agentic modes; 0 disables agent listener in pipe mode", required = false) <int> port = 0;if(mode == "pipe" || mode == "socket") { + if(mode == "socket" && opts.port.value_or(0) <= 0) { + LOG_ERROR("--port is required for socket mode"); + return 1; + } clice::ServerOptions server_opts;Also applies to: 132-140
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/clice.cc` around lines 28 - 31, The --port DecoKV (port variable declared with DecoKV style = KVStyle::JoinedOrSeparate) has a help string that only describes agentic mode and lacks validation for socket mode; update the help text to describe both uses (agentic: connect target where 0 disables; socket: LSP listen port) and change behavior so socket mode validates port > 0 before calling kota::tcp::listen (mirror the existing agentic-mode port > 0 check), returning a clear error if missing/zero; keep the existing agentic-mode validation intact and ensure any error messages reference the port variable and mode to make debugging obvious.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/clice.cc`:
- Around line 28-31: The --port DecoKV (port variable declared with DecoKV style
= KVStyle::JoinedOrSeparate) has a help string that only describes agentic mode
and lacks validation for socket mode; update the help text to describe both uses
(agentic: connect target where 0 disables; socket: LSP listen port) and change
behavior so socket mode validates port > 0 before calling kota::tcp::listen
(mirror the existing agentic-mode port > 0 check), returning a clear error if
missing/zero; keep the existing agentic-mode validation intact and ensure any
error messages reference the port variable and mode to make debugging obvious.
In `@tests/conftest.py`:
- Around line 125-128: _find_free_port suffers a TOCTOU race because it binds
then closes the socket before the test server (clice) binds; either change test
harness so the server binds to port 0 and reports the chosen port back, or as a
pragmatic interim set SO_REUSEADDR on the temporary socket to reduce races:
update the _find_free_port function to call s.setsockopt(socket.SOL_SOCKET,
socket.SO_REUSEADDR, 1) before s.bind, or preferably modify clice/server startup
to accept port=0 and return the bound port so callers do not rely on a transient
free-port probe.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 20fca714-1515-436e-8979-10e7dfc13d0d
📒 Files selected for processing (7)
src/clice.ccsrc/server/service/agentic.cppsrc/server/service/agentic.hsrc/server/service/master_server.cppsrc/server/service/master_server.htests/conftest.pytests/integration/agentic/test_agentic.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/server/service/master_server.h
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ic client Move JsonPeer ownership to the caller via unique_ptr& out-parameter so the peer outlives the when_all scope and avoids use-after-free. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Compiler: detached compile tasks now spawn into a task_group member, with stop() for graceful shutdown instead of raw loop.schedule() - MasterServer: connection handlers use a local task_group inside accept_connections instead of fire-and-forget scheduling - Indexer: replace hand-written inflight/finished/completion_event counters with task_group; monitor_resources uses cancellation_token via with_token instead of generation counter polling - WorkerPool: monitor tasks use task_group, removing manual alive_count_/all_exited_ tracking; stop() simplified to join() - Agentic client: remove unnecessary loop parameter, use event_loop::current() inside coroutine Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Pin to e024f3b which includes the variant dispatch fix (#129). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Remove `friend class LSPClient/AgentClient` from MasterServer, replace with explicit public accessors and coordination methods (open_session, close_session, find_session, on_file_saved, schedule_shutdown, etc.) - Enforce single LSP connection in socket mode - Delete dead MasterServer::initialize() and duplicate set_peer call - Unify lambda capture style in lsp_client.cpp (this-> consistently) - Fix stale doc reference from src/server/protocol.h to src/server/protocol/ - Add agentic error-path integration tests (connection_refused, concurrent) - Fix flaky test_touch_without_content_change_skips_recompile: default cache_dir to workspace/.clice in CliceClient.initialize() to prevent stale PCH from global cache causing silent compilation failures Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…or comments Trivial getters/setters on MasterServer added no value over direct member access via friend — revert to `friend class LSPClient; friend class AgentClient;`. Also remove all `// --- ... ---` decorative separator comments project-wide. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
16bit-ykiko
changed the title
5 tasks
This was referenced Jun 5, 2026
4 tasks
6 tasks
16bit-ykiko
added a commit
that referenced
this pull request
Jul 3, 2026
## Background Server-side error feedback is the operability foundation for v1.0 (roadmap: server task 3 "error handling & logging"). Before this PR, internal failures died silently in logs (or as silent `null` responses), users got no signal when their setup was broken, and there was no structured way to profile the server on real codebases. This PR supersedes #436 (reimplemented on current main rather than rebased — it predates the #437/#447/#449 restructures). ## Feedback channels **Anomaly (soft assertions, `src/support/anomaly.h`)** — `LOG_ANOMALY(id, fmt, ...)` for internal states that must be unreachable when clice works correctly. Debug builds abort after logging (the CI Debug matrix and local dev catch bugs earliest; `CLICE_ANOMALY_NO_TRAP` exists for tests); Release logs `[anomaly:<id>]`, pushes `window/logMessage` (master) and continues. Per-ID rate limit with a final suppression notice; the gate runs **before** format arguments are evaluated (lazy contract locked by unit tests with side-effecting args). IDs: `PCHBuildFail`, `PCMBuildFail`, `CompileFail`, `WorkerRequestFail`, `WorkerCrash`, `WorkerSpawnFail`, `PositionMapFail` (markers are the enumerator names, rendered by the reflective enum formatter and pinned by `MarkerNamesStable`). Situations reachable by user input or normal operation are deliberately **not** anomalies. That split is structural: worker build failures carry `BuildResult.has_user_errors`, and master-side dispatch failures carry `dispatch_errc` codes (`cancelled` for memory-pressure preemption, `worker_unavailable` for crash/restart windows) so `is_operational_error()` keeps them out of the anomaly channel. **Guidance** — `LOG_GUIDANCE(...)` (`[guidance]` + Warning logMessage) for user-actionable situations: no compile_commands.json, invalid initializationOptions, config problems. **LSP errors instead of silent null** — feature requests on closed documents → `Document not open`; unresolvable call/type hierarchy items → error; reversed ranges → `InvalidParams`; empty hierarchy/workspace-symbol results return `[]` instead of `null`. Out-of-range positions **clamp** per the LSP spec (character → line end, line → end of content) instead of erroring. **Guidance diagnostics** — `fill_compile_args()` returns a `CommandSource` (`CDBExact`/`IncludeGraph`/`Inferred` (reserved)/`Fallback`) and emits a per-file decision log (tiers tried, tier hit, args hash). When a *guessed* command produces file-not-found errors, the publish merges a file-top Warning explaining it (code `inferred-compile-command`, linking the quick-start guide). Exact CDB matches never get it. Config rule appends now reach synthesized fallback commands, so `-I` rules work without a CDB. **clice.toml diagnostics** — parse/type errors (Error, defaults apply) and unknown keys (Warning via a strict second decode pass, config still applies) are published on the config file's URI. Line/column ranges wait for the kotatsu TOML location feature (FIXMEs mark the re-enable points); until then diagnostics anchor at the file top. ## Logging system - **Design doc in `logging.h`**: channel selection is decision-oriented — pick by *who must act and whether it is clice's fault*. Levels, perf topics and process ownership are specified there. - **`LOG_PERF(topic, ...)`** emits greppable `[perf:<topic>] key=value` lines for profiling on real codebases: `startup` (CDB load, dep scan, index load), `index` (per-file index/merge timings, run summary, save), `cache` (PCH/PCM hit/miss with reason, evictions), `request` (per-request `wait_ms`/`total_ms` for query/build/format/compile). - **Per-process log files**: workers log only to their own `<session>/<worker>.log` (no stderr mirror), so the master log no longer duplicates worker output. Worker stderr is reserved for unexpected third-party output (sanitizers, libc asserts), relayed line-by-line into the master log by the pool. - **Crash backtraces** land in the owning process's log file (`install_crash_handler`, covers SIGABRT so `LOG_FATAL` and Debug anomaly traps too); a worker's backtrace goes *only* to its own log, verified by an integration test that SIGABRTs a worker and asserts the trace stays out of the master log. - **Startup flow** is fully logged: master banner (pid/mode/workspace), config source, session log dir, worker pool summary, CDB/dep-scan/index-load timings. ## Notable fixes uncovered along the way - `CompilationDatabase::lookup()` synthesizes a command for unknown files, so the old `results.empty()` checks were dead: the automatic include-graph header-context tier was unreachable, and headers without entries silently compiled as bare `clang`. Tier selection now uses `has_entry()`; background indexing skips Fallback-sourced files. - initializationOptions now overlay **before** `apply_defaults()`: a client-provided `cache_dir` previously didn't propagate into the derived `logging_dir`/`index_dir`. - Six unchecked `*map.to_range()` dereferences (empty-optional UB) in feature code converted to checked helpers reporting `PositionMapFail`. - Anomaly notify/trap hooks are mutex-synchronized (copy under lock, invoke outside), making reporting safe from any thread. ## Test infrastructure - `CliceClient` records `window/logMessage`; **`assert_no_anomaly()` runs in every integration teardown** (all fixtures and `make_client` sessions), scanning notifications and master/worker log files even when shutdown fails. `tests/replay.py` fails a smoke trace on any anomaly push. - E2E: guidance-diagnostic lifecycle (no CDB → appears; add CDB + restart → gone while the include error stays), fallback rule-append (no CDB + clice.toml `-I` rule → clean compile), clice.toml error/unknown-key/clear scenarios, worker SIGABRT → `WorkerCrash` anomaly + backtrace ownership, closed/unknown documents via `pytest.raises`, position/range clamping. - Unit triggers for `PositionMapFail`, `WorkerCrash`, `WorkerSpawnFail`; `dispatch_errc` classification; fallback append; lazy-evaluation and rate-limit contracts. - Named timing constants (`MTIME_GRANULARITY`/`SETTLE_TIME`/`IDLE_TIMEOUT`) replace hardcoded sleeps; yield-based workspace cleanup; log dump on test failure. ## Out of scope Module-cycle guidance diagnostics wait for the module refactor; kotatsu TOML line/column re-enable after the kotatsu-side PR merges; worker logMessage forwarding and request-correlation IDs belong to the worker-pool follow-up. ## Test plan - [x] RelWithDebInfo: 732 unit / 187 integration / 3 smoke — all green locally - [x] Debug: 732 unit / 187 integration / 3 smoke — all green locally (anomalies abort in Debug, so this run is the "zero anomalies on regular paths" acceptance) - [x] `pixi run format` clean; two rounds of 3 parallel review subagents (correctness / style / tests) — all findings fixed or triaged - [x] Full CI matrix green (Linux/macOS/Windows native, aarch64/x64/arm64 cross, editor test) Closes #436 (superseded).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
src/server/into subdirectories (service/,compiler/,worker/,workspace/,protocol/) to separate concerns: transport/session management, compilation, worker orchestration, and persistent workspace state.JsonPeer&reference or registers handlers itself. NewLSPClientandAgentClientclasses own their peer references and register protocol handlers, accessing MasterServer internals viafriend class.agentic/compileCommand) that lets external tools (AI agents, build systems) query compile commands from a running clice server. Includes a CLI client mode (--mode agentic --port N --path FILE), server-side listener when--portis specified in pipe mode, and integration tests for happy path, fallback, concurrency, and connection-refused.loop.schedule()withkota::task_group: Compiler compile tasks, Indexer background indexing + resource monitor, WorkerPool worker monitors, and socket accept loops now use structured concurrency. This eliminates manualalive_count_/generation counters and ensures all spawned tasks are joined on shutdown.CliceClient.initialize()now always setscache_dirto a workspace-local.clice/directory, preventing stale PCH artifacts from the global~/.cache/clice/from polluting test runs.Details
Compiler peer lifetime:
CompilerandIndexerpreviously tookJsonPeer&in their constructors, coupling them to a single connection. They now store aJsonPeer*set viaset_peer(), with null checks before sending diagnostics/progress. This supports the multi-connection model where agentic clients don't need diagnostics.Socket mode single-LSP enforcement:
accept_connections()takes aregister_lspflag; when true, only the first connection gets anLSPClient. All connections get anAgentClient. This prevents multiple LSP sessions from racing on shared server state.Structured shutdown:
Compiler::stop()cancels in-flight compile tasks and joins them.WorkerPool::stop()signals workers and joins the monitor task group.Indexeruses acancellation_sourceto stop its resource monitor when a background indexing run completes.Pin kotatsu: Changed from
GIT_TAG main+GIT_SHALLOW TRUEto an exact commit hash for reproducible builds.